Protect claude-custom main from untracked config drift

forgejo_admin commented

2026-03-07 18:06:13 +00:00

Author

Contributor

PR #61 Review

BLOCKERS

None found.

NITS

check-claude-custom-clean.sh line 26 — misleading wording. The warning says "These changes are untracked and at risk of being lost." The word "untracked" is confusing here because the script explicitly filters OUT untracked files (grep -v '^??'). The changes it reports are uncommitted tracked changes. Suggest: "These tracked files have uncommitted changes and are at risk of being lost."
forgejo_owner_repo hostname matching is less strict than is_forgejo_repo. is_forgejo_repo uses regex to extract the hostname from the URL, then checks *forgejo* against just the hostname. But forgejo_owner_repo does a simpler *forgejo* substring match against the entire URL ([[ "$url" == *"forgejo"* ]]). This would false-positive on a repo path containing "forgejo" (e.g. github.com/user/my-forgejo-tools.git). Low risk given current usage, but worth noting the inconsistency.
check-note-template.sh line 62 — stale error message. The deny reason still says "Note html_content is empty" but the field being checked is now .content // .html_content. Minor user-facing inconsistency if someone passes content but not html_content.

SOP COMPLIANCE

Branch named after issue number — Branch is fix/claude-custom-main-protection, not {issue-number}-description. There is no corresponding issue in the claude-custom repo issue list. The PR references a pal-e-docs plan slug (phase-postgres-7d1-claude-custom-protection) but no Forgejo issue. This appears to be a cleanup/protection PR done directly off a sub-phase plan rather than a tracked issue. Acceptable given this is config hardening, but noting the deviation.
PR body follows template — Has Summary, Changes, Test Plan, Related sections. Well-structured.
Related references plan slug — References phase-postgres-7d1-claude-custom-protection, bug-claude-custom-worktree-pollution, and sop-claude-config-development.
No secrets committed — No credentials, .env files, or sensitive data in the diff. The deleted known_marketplaces.json contained only plugin metadata (marketplace source, install location) — no secrets.
No unnecessary file changes — All 8 changed files are in scope: .gitignore fix, new hook, settings.json registration, 3 triaged hook improvements, 2 plugin file removals.
Commit messages — Not visible in diff, but PR description is descriptive.

CODE REVIEW

.gitignore (correct): Adding plugins/known_marketplaces.json to the ignore list alongside the already-ignored blocklist.json and install-counts-cache.json is correct. These are auto-updated by Claude Code with timestamps that change every session, causing permanent dirty state. The two deleted files (plugins/blocklist.json, plugins/known_marketplaces.json) are properly removed from tracking to match.

check-claude-custom-clean.sh (correct): Clean implementation. set -euo pipefail is good. The || true on the grep prevents set -e from killing the script when there are no dirty files. Exits 0 unconditionally so it warns but never blocks session start. Correctly skips if the repo directory does not exist.

settings.json (correct): New hook appended to the existing SessionStart hooks array, after session-start-context.sh. Ordering is sensible — context loads first, then the cleanliness check.

block-mcp-merge.sh (correct and improved): Switching from "ask" to "allow_with_user_confirmation" is the right call. "ask" can be bypassed in dontAsk mode; "allow_with_user_confirmation" always prompts. The added context (repo name and PR number in the confirmation message) is a good UX improvement. Reading stdin into INPUT before using jq is correct.

check-note-template.sh (correct): The change from .tool_input.html_content to .tool_input.content // .tool_input.html_content fixes template validation for create_note calls that use the content field. The jq // operator correctly falls back: try content first, then html_content. This is a real bug fix.

forgejo-helper.sh (correct): Three functions updated:

is_forgejo_repo: Now iterates all remotes instead of just origin. Correct fix for repos where Forgejo is a non-origin remote.
is_github_repo: Now calls is_forgejo_repo first and returns 1 if any Forgejo remote exists. This ensures Forgejo priority, which matches the documented convention.
forgejo_owner_repo: Now scans all remotes for a Forgejo URL before falling back to origin. Correct.

VERDICT: APPROVED

Clean, well-scoped PR. The .gitignore fix and new SessionStart hook directly address the config drift problem. The three triaged hook improvements are all legitimate fixes with clear rationale. No secrets, no scope creep. The nits above are non-blocking suggestions.

## PR #61 Review ### BLOCKERS None found. ### NITS 1. **check-claude-custom-clean.sh line 26 — misleading wording.** The warning says "These changes are untracked and at risk of being lost." The word "untracked" is confusing here because the script explicitly filters OUT untracked files (`grep -v '^??'`). The changes it reports are *uncommitted tracked* changes. Suggest: "These tracked files have uncommitted changes and are at risk of being lost." 2. **forgejo_owner_repo hostname matching is less strict than is_forgejo_repo.** `is_forgejo_repo` uses regex to extract the hostname from the URL, then checks `*forgejo*` against just the hostname. But `forgejo_owner_repo` does a simpler `*forgejo*` substring match against the entire URL (`[[ "$url" == *"forgejo"* ]]`). This would false-positive on a repo path containing "forgejo" (e.g. `github.com/user/my-forgejo-tools.git`). Low risk given current usage, but worth noting the inconsistency. 3. **check-note-template.sh line 62 — stale error message.** The deny reason still says "Note html_content is empty" but the field being checked is now `.content // .html_content`. Minor user-facing inconsistency if someone passes `content` but not `html_content`. ### SOP COMPLIANCE - [ ] **Branch named after issue number** — Branch is `fix/claude-custom-main-protection`, not `{issue-number}-description`. There is no corresponding issue in the claude-custom repo issue list. The PR references a pal-e-docs plan slug (`phase-postgres-7d1-claude-custom-protection`) but no Forgejo issue. This appears to be a cleanup/protection PR done directly off a sub-phase plan rather than a tracked issue. Acceptable given this is config hardening, but noting the deviation. - [x] **PR body follows template** — Has Summary, Changes, Test Plan, Related sections. Well-structured. - [x] **Related references plan slug** — References `phase-postgres-7d1-claude-custom-protection`, `bug-claude-custom-worktree-pollution`, and `sop-claude-config-development`. - [x] **No secrets committed** — No credentials, .env files, or sensitive data in the diff. The deleted `known_marketplaces.json` contained only plugin metadata (marketplace source, install location) — no secrets. - [x] **No unnecessary file changes** — All 8 changed files are in scope: .gitignore fix, new hook, settings.json registration, 3 triaged hook improvements, 2 plugin file removals. - [x] **Commit messages** — Not visible in diff, but PR description is descriptive. ### CODE REVIEW **`.gitignore` (correct):** Adding `plugins/known_marketplaces.json` to the ignore list alongside the already-ignored `blocklist.json` and `install-counts-cache.json` is correct. These are auto-updated by Claude Code with timestamps that change every session, causing permanent dirty state. The two deleted files (`plugins/blocklist.json`, `plugins/known_marketplaces.json`) are properly removed from tracking to match. **`check-claude-custom-clean.sh` (correct):** Clean implementation. `set -euo pipefail` is good. The `|| true` on the grep prevents `set -e` from killing the script when there are no dirty files. Exits 0 unconditionally so it warns but never blocks session start. Correctly skips if the repo directory does not exist. **`settings.json` (correct):** New hook appended to the existing SessionStart hooks array, after `session-start-context.sh`. Ordering is sensible — context loads first, then the cleanliness check. **`block-mcp-merge.sh` (correct and improved):** Switching from `"ask"` to `"allow_with_user_confirmation"` is the right call. `"ask"` can be bypassed in dontAsk mode; `"allow_with_user_confirmation"` always prompts. The added context (repo name and PR number in the confirmation message) is a good UX improvement. Reading stdin into `INPUT` before using jq is correct. **`check-note-template.sh` (correct):** The change from `.tool_input.html_content` to `.tool_input.content // .tool_input.html_content` fixes template validation for `create_note` calls that use the `content` field. The jq `//` operator correctly falls back: try `content` first, then `html_content`. This is a real bug fix. **`forgejo-helper.sh` (correct):** Three functions updated: - `is_forgejo_repo`: Now iterates all remotes instead of just origin. Correct fix for repos where Forgejo is a non-origin remote. - `is_github_repo`: Now calls `is_forgejo_repo` first and returns 1 if any Forgejo remote exists. This ensures Forgejo priority, which matches the documented convention. - `forgejo_owner_repo`: Now scans all remotes for a Forgejo URL before falling back to origin. Correct. ### VERDICT: APPROVED Clean, well-scoped PR. The .gitignore fix and new SessionStart hook directly address the config drift problem. The three triaged hook improvements are all legitimate fixes with clear rationale. No secrets, no scope creep. The nits above are non-blocking suggestions.

2026-03-07 18:04:56 +00:00

Summary

Gitignore auto-updated plugin files that create permanent dirty state on main
Add SessionStart hook that warns on uncommitted tracked changes in ~/claude-custom
Triage and commit 3 legitimate hook improvements that were floating uncommitted

Changes

.gitignore: add plugins/known_marketplaces.json to ignored auto-updated files
hooks/check-claude-custom-clean.sh (new): SessionStart hook — runs git status --porcelain on tracked files, warns if dirty
settings.json: register new SessionStart hook
hooks/block-mcp-merge.sh: use allow_with_user_confirmation instead of ask (ensures merge gate works in dontAsk mode)
hooks/check-note-template.sh: check .content field (not just .html_content) — fixes template validation for create_note
hooks/forgejo-helper.sh: check ALL remotes for Forgejo detection, not just origin. Forgejo takes priority over GitHub.
plugins/blocklist.json, plugins/known_marketplaces.json: removed from tracking (now gitignored)

Test Plan

New session starts without error
If ~/claude-custom has dirty tracked files, session start shows warning
Plugin files no longer appear in git status
is_forgejo_repo correctly detects repos with Forgejo as non-origin remote
MCP merge gate still prompts for confirmation

Review Checklist

No secrets committed
No unnecessary file changes
Commit messages are descriptive

phase-postgres-7d1-claude-custom-protection — sub-phase plan
bug-claude-custom-worktree-pollution — prior incident this prevents
sop-claude-config-development — config change SOP

## Summary - Gitignore auto-updated plugin files that create permanent dirty state on main - Add SessionStart hook that warns on uncommitted tracked changes in `~/claude-custom` - Triage and commit 3 legitimate hook improvements that were floating uncommitted ## Changes - `.gitignore`: add `plugins/known_marketplaces.json` to ignored auto-updated files - `hooks/check-claude-custom-clean.sh` (new): SessionStart hook — runs `git status --porcelain` on tracked files, warns if dirty - `settings.json`: register new SessionStart hook - `hooks/block-mcp-merge.sh`: use `allow_with_user_confirmation` instead of `ask` (ensures merge gate works in dontAsk mode) - `hooks/check-note-template.sh`: check `.content` field (not just `.html_content`) — fixes template validation for create_note - `hooks/forgejo-helper.sh`: check ALL remotes for Forgejo detection, not just origin. Forgejo takes priority over GitHub. - `plugins/blocklist.json`, `plugins/known_marketplaces.json`: removed from tracking (now gitignored) ## Test Plan - [ ] New session starts without error - [ ] If `~/claude-custom` has dirty tracked files, session start shows warning - [ ] Plugin files no longer appear in `git status` - [ ] `is_forgejo_repo` correctly detects repos with Forgejo as non-origin remote - [ ] MCP merge gate still prompts for confirmation ## Review Checklist - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `phase-postgres-7d1-claude-custom-protection` — sub-phase plan - `bug-claude-custom-worktree-pollution` — prior incident this prevents - `sop-claude-config-development` — config change SOP

forgejo_admin added 1 commit

Protect claude-custom main from untracked config drift f2585060e5

- Gitignore auto-updated plugin files (blocklist.json, known_marketplaces.json)
  that change timestamps every session
- Add SessionStart hook to warn on dirty tracked files in ~/claude-custom
- Register new hook in settings.json
- Triage 3 legitimate hook improvements that were floating uncommitted:
  - block-mcp-merge.sh: use allow_with_user_confirmation instead of ask
  - check-note-template.sh: check .content field (not just .html_content)
  - forgejo-helper.sh: check all remotes (not just origin) for Forgejo detection

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

2026-03-07 18:11:19 +00:00

Fix QA nits: wording, hostname matching, error message 70a6fabbe4

- check-claude-custom-clean.sh: "untracked" → "tracked files have uncommitted changes"
- forgejo-helper.sh: forgejo_owner_repo now uses hostname extraction
  (consistent with is_forgejo_repo) instead of full URL substring match
- check-note-template.sh: error message says "content" instead of "html_content"

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin merged commit a91ef6cb8b into main

2026-03-07 18:13:19 +00:00

forgejo_admin deleted branch fix/claude-custom-main-protection

forgejo_admin referenced this pull request from a commit

Protect claude-custom main from untracked config drift (#61)

Rows
Columns

Protect claude-custom main from untracked config drift #61