What happened
On PR #804, the review agent approved a 2-line .gitignore change adding .claude and openspec entries. The agent found only a low-severity naming-convention issue (missing trailing slash) and applied the ready-for-merge label. Three hours later, human reviewer waynesun09 identified that the .claude gitignore entry would break the repo's documented .claude/skills symlink workflow — referenced in docs/review.md, agents/fix.md, and the REVIEW_PROTECTED_PATHS env var in harness/review.yaml which explicitly includes .claude/. The author closed the PR without merging.
What could go better
The pr-review orchestrator's protected-path check (step 6e in skills/pr-review/SKILL.md) only checks whether changed files ARE protected paths — it iterates over files in the diff and checks if their path starts with any REVIEW_PROTECTED_PATHS entry. Since .gitignore is not itself a protected path, the check passed. But the .gitignore addition of .claude would suppress version control of .claude/, which IS a protected path. The check is file-based, not semantic: it doesn't reason about whether .gitignore pattern additions would make protected paths invisible to git. This is a high-confidence finding — the gap is structural and clearly demonstrated by the PR. The human reviewer caught it by cross-referencing the gitignore pattern against three separate documentation sources that reference .claude/.
Proposed change
Extend the protected-path check in skills/pr-review/SKILL.md step 6e to add a .gitignore suppression sub-check. When the PR diff modifies .gitignore (or .git/info/exclude), the orchestrator should: (1) extract any newly added ignore patterns from the diff, (2) for each pattern, check whether it would match any path prefix in REVIEW_PROTECTED_PATHS, and (3) if a match is found, emit a protected-path finding with high severity noting that the gitignore addition would suppress version control of a protected path. The check should handle both bare patterns (e.g., .claude) and anchored patterns (e.g., /.claude/). This keeps protected-path logic centralized in the orchestrator per the existing constraint that 'PR-specific checks belong in the orchestrator only.'
Validation criteria
The next review of a PR that adds a protected path name to .gitignore should produce a high-severity protected-path finding and the verdict should NOT be approve. Test with a synthetic PR adding any REVIEW_PROTECTED_PATHS entry (e.g., agents/, harness/, .claude/) to .gitignore.
Generated by retro agent from #804
What happened
On PR #804, the review agent approved a 2-line
.gitignorechange adding.claudeandopenspecentries. The agent found only a low-severity naming-convention issue (missing trailing slash) and applied theready-for-mergelabel. Three hours later, human reviewer waynesun09 identified that the.claudegitignore entry would break the repo's documented.claude/skillssymlink workflow — referenced indocs/review.md,agents/fix.md, and theREVIEW_PROTECTED_PATHSenv var inharness/review.yamlwhich explicitly includes.claude/. The author closed the PR without merging.What could go better
The pr-review orchestrator's protected-path check (step 6e in
skills/pr-review/SKILL.md) only checks whether changed files ARE protected paths — it iterates over files in the diff and checks if their path starts with anyREVIEW_PROTECTED_PATHSentry. Since.gitignoreis not itself a protected path, the check passed. But the.gitignoreaddition of.claudewould suppress version control of.claude/, which IS a protected path. The check is file-based, not semantic: it doesn't reason about whether.gitignorepattern additions would make protected paths invisible to git. This is a high-confidence finding — the gap is structural and clearly demonstrated by the PR. The human reviewer caught it by cross-referencing the gitignore pattern against three separate documentation sources that reference.claude/.Proposed change
Extend the protected-path check in
skills/pr-review/SKILL.mdstep 6e to add a.gitignoresuppression sub-check. When the PR diff modifies.gitignore(or.git/info/exclude), the orchestrator should: (1) extract any newly added ignore patterns from the diff, (2) for each pattern, check whether it would match any path prefix inREVIEW_PROTECTED_PATHS, and (3) if a match is found, emit aprotected-pathfinding with high severity noting that the gitignore addition would suppress version control of a protected path. The check should handle both bare patterns (e.g.,.claude) and anchored patterns (e.g.,/.claude/). This keeps protected-path logic centralized in the orchestrator per the existing constraint that 'PR-specific checks belong in the orchestrator only.'Validation criteria
The next review of a PR that adds a protected path name to
.gitignoreshould produce a high-severity protected-path finding and the verdict should NOT be approve. Test with a synthetic PR adding anyREVIEW_PROTECTED_PATHSentry (e.g.,agents/,harness/,.claude/) to.gitignore.Generated by retro agent from #804