feat(acceptance): accept benign repo-hygiene passengers instead of refusing the folder#242
Conversation
…fusing the folder A GitTarget subtree is operator-exclusive: any non-YAML file that is not an operator artifact (README.md/.sops.yaml) or a build directive was refused as foreign, stalling the whole folder. For the reverser's core case — adopting an existing repo, which has no .gittargetignore — a stray LICENSE, .gitkeep, or non-README markdown refused everything on first scan. Add a closed benign-passenger set accepted by default (isBenignPassenger, RoleBenignPassenger): *.md/*.markdown, LICENSE/LICENCE/COPYING/NOTICE, and .gitignore/.gitattributes/.gitkeep/.keep. Matched after the .gittargetignore filter so it stays user-suppressible and never muddies the operator-artifact role. Loose application data (notes.txt, values.json, deploy.sh), symlinks, and submodules stay refused — the acceptance ratchet holds because the set holds only files no future "own the subtree" behaviour would ever claim. Reverses the original minimal-set decision for the adoption case; recorded as D-foreign-7 in the stringency spec (role 3b). Regenerated corpus baseline shows repo-per-environment's three env folders flip refused→accepted (each blocked only by a .gitignore) and mixed-and-hostile's .gitkeep no longer counts, while deployment.json correctly stays foreign. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a closed benign-passenger role for inert repository-hygiene files, preserves ChangesBenign passenger content
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Worktree as scanWorktreeSubtree
participant Classifier as ClassifyEntry
participant Analyzer as collectFiles
participant Writer as commitPendingWrites
Worktree->>Classifier: classify repository entry
Classifier->>Analyzer: return RoleBenignPassenger
Analyzer->>Worktree: record entry as NonYAML
Worktree->>Writer: provide accepted worktree
Writer->>Writer: commit pending write
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/manifestanalyzer/gittargetignore_test.go`:
- Around line 88-92: Extend the expected values in the non-YAML inventory
assertion loop of the relevant test to include COPYING, docs/guide.markdown, and
.gitattributes, while preserving the existing entries and containsString
validation.
- Around line 63-96: Rename the affected tests to clearly identify the component
and scenario: internal/manifestanalyzer/gittargetignore_test.go:63-96 to
TestCollectFiles_BenignPassengerAcceptedByDefault, 98-114 to
TestCollectFiles_BenignPassengerStillUserSuppressible, and 116-136 to
TestIsBenignPassenger_AcceptedAndRefused; rename
internal/git/gittargetignore_writer_test.go:92-111 to
TestCommitPendingWrites_BenignPassengerAllowsWrite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7565cbcd-d673-4713-8b09-80bfae9dba61
📒 Files selected for processing (11)
.coverage-baselinedocs/design/support-boundary/README.mddocs/spec/gitpath-foreign-content-stringency.mdinternal/git/bootstrapped-repo-template/.gittargetignoreinternal/git/gittargetignore_writer_test.gointernal/git/plan_flush.gointernal/manifestanalyzer/analyzer.gointernal/manifestanalyzer/gittargetignore.gointernal/manifestanalyzer/gittargetignore_test.gointernal/manifestanalyzer/scan_repo.gotest/fixtures/gitops-layouts/support-today.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Problem
A GitTarget subtree is operator-exclusive: any non-YAML file that isn't an operator artifact (
README.md/.sops.yaml) or a build directive is refused as foreign, stalling the whole folder (GitPathAccepted=False). For the reverser's core case — adopting an existing repo, which has no.gittargetignore— a strayLICENSE, a.gitkeep, or a non-README.mdrefuses everything on first scan. As raised: "it's unwise to not accept a folder because of a few markdowns or .gitignore/.gitkeep files."Change
A new
RoleBenignPassenger+isBenignPassenger— a closed hygiene set accepted by default:*.md/*.markdownLICENSE/LICENCE/COPYING/NOTICE(+.txtvariants).gitignore/.gitattributes/.gitkeep/.keepMatched after the
.gittargetignorefilter, so a benign passenger is user-suppressible and never muddies the operator-artifact role (role 3 stays "operator-authored, un-suppressible"). Wired through the one sharedClassifyEntry, so the folder scan and the live-writer worktree scan agree.Still refused (ratchet preserved): loose application data (
notes.txt,values.json,deploy.sh), symlinks, submodules, foreign YAML. The set holds only files no future "own the subtree" behaviour (wrap-to-ConfigMap, faithful sweep) would ever want to claim.Design
Reverses the original minimal-set stance (D-foreign-3) for the adoption case, recorded as D-foreign-7 and role 3b in
gitpath-foreign-content-stringency.md. Bootstrap.gittargetignoreand the support-boundary README updated to match.Evidence
Regenerated corpus baseline (
support-today.md) shows exactly which real fixtures moved:1-desired-state/repo-per-environment: the threegitops-*env folders flip refused → accepted (each was blocked only by a.gitignore).6-hostile/mixed-and-hostile: the.gitkeepno longer counts as non-KRM, whiledeployment.jsoncorrectly stays foreign.Validation
task fmt/task vetcleantask lint— doccheck OK, golangci-lint 0 issuestask test— all unit + writer-integration tests pass; coverage 76.3% → 76.4% (baseline bumped)task test-e2e— 56 passed, 0 failed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
.gittargetignore.Documentation
Bug Fixes
.gitignore, are no longer incorrectly refused.