feat: implement issue #809 — upsert_gitignore_baseline must preserve baseline negations + migrate unmarkered baselines (blocks #800 apply) - #815
Conversation
…baseline negations + migrate unmarkered baselines (blocks #800 apply)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 51 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesGitignore baseline neutralization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExistingGitignore
participant upsert_gitignore_baseline
participant _gib_neutralize_l2
participant UpdatedGitignore
ExistingGitignore->>upsert_gitignore_baseline: provide marker-less or marker-present content
upsert_gitignore_baseline->>_gib_neutralize_l2: pass existing L2
_gib_neutralize_l2->>upsert_gitignore_baseline: return filtered L2 and effective negations
upsert_gitignore_baseline->>UpdatedGitignore: emit managed baseline plus neutralized L2
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a neutralization mechanism (_gib_neutralize_l2) in the gitignore baseline script to prevent duplicate or conflicting patterns in the L2 block from overriding baseline negations. It also adds corresponding tests in BATS to verify this behavior. Feedback suggests stripping carriage returns (\r) before passing text to awk to prevent CRLF issues, and utilizing $BATS_TEST_TMPDIR when creating temporary directories in tests to ensure proper isolation and cleanup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
There was a problem hiding this comment.
Pull request overview
This PR updates the org-wide .gitignore baseline upsert helper to address issue #809 by preventing baseline negations from being overridden by later per-repo patterns, and by folding legacy unmarker-ed baselines into the single managed marker-wrapped block (instead of duplicating them).
Changes:
- Add
_gib_neutralize_l2and apply it during both “insert-on-top” and “replace-in-place” upsert flows to drop L2 lines that duplicate baseline pattern lines. - Extend bats coverage with git-semantic assertions (
git check-ignore) for thepublic.pemre-hide regression and for unmarker-ed baseline migration/idempotency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/lib/gitignore-baseline.sh | Adds L2 “neutralization” logic and applies it when inserting/replacing the managed baseline block. |
| test/scripts/lib/gitignore-baseline.bats | Adds #809 regression tests using git check-ignore, plus migration + idempotency coverage. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
don-petry
added
the
dev-lead:hands-off
Dev-Lead — review-changes (applied)Changes committed and pushed. |
…ixtures #809 gave upsert_gitignore_baseline a negation re-allow tail (to defend against non-identical L2 globs like **/*.pem re-hiding !public.pem). That changed the idempotent steady state to block + neutralized-L2 + tail. Two older fixtures (#798 sync dry-run, #808 remediate) fed the raw canonical (tail-less) as 'already current', so the current-baseline skip/no-op detection saw a diff and planned a PR. Feed those fixtures the actual upsert steady state instead. Upsert is idempotent from the 2nd application (verified), so this is the true no-op input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fa6TNhxEXj6WAKpaJLHvNK
|
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 `@scripts/lib/gitignore-baseline.sh`:
- Around line 79-87: The migration currently preserves canonical header comments
from an old L2 baseline, allowing them to duplicate the newly emitted block.
Update _gib_neutralize_l2 to remove only exact matches of the block’s own
canonical comment lines while continuing to preserve unrelated comments;
document the chosen limitation if comments remain intentionally preserved. In
test/scripts/lib/gitignore-baseline.bats lines 280-315, extend the TalkTerm
fixture with the header comment and assert the migration produces the intended
non-duplicated result.
- Around line 88-131: Update _gib_neutralize_l2 so the block reaches awk without
-v escape processing, preserving literal gitignore backslashes such as \!, \#, \
, and foo\bar. Export the block through the GIB_NEUTRALIZE_BLOCK environment
variable and read it via ENVIRON inside awk, leaving the existing split and drop
matching behavior unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d939fb8-9f60-415d-a1fa-5006b406636d
📒 Files selected for processing (2)
scripts/lib/gitignore-baseline.shtest/scripts/lib/gitignore-baseline.bats
Superseded by automated re-review at
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 1a36dccafea8c34c2bfb826a0ce4bd94da394337
Review mode: triage-approved (single reviewer)
Summary
PR #815 fixes the #809 correctness regression in the shared .gitignore baseline lib: _gib_neutralize_l2 drops L2 lines that exactly duplicate block pattern lines (migrating old unmarkered baselines without duplication) and re-emits all baseline negations as a re-allow tail after L2, so broad L2 globs (.pem, .env, **/*.pem) can never re-hide baseline-negated paths (!public.pem, !.env.example). Both prior escalation reasons are now cleared: CodeRabbit's CHANGES_REQUESTED became an APPROVED review after its two findings were adjudicated and withdrawn, and the awk -v escape-processing finding was resolved by the repo owner as a latent non-issue (the canonical baseline contains no backslash-bearing patterns) with CodeRabbit formally withdrawing the comment. No new commits since; head SHA unchanged. Risk MEDIUM (non-trivial logic on a shared shell lib), no HIGH-risk surface.
Linked issue analysis
Closes #809. All required acceptance criteria are substantively met with bats tests asserting real git semantics via git check-ignore in temp git-init dirs: (1) markets-like and google-app-scripts-like inputs — public.pem NOT ignored after upsert while secret.pem stays ignored; (2) TalkTerm-like unmarkered baseline — exactly one BEGIN/END marker pair, .env and .pem appear once, genuine L2 (node_modules/, dist/, .log, .DS_Store) preserved; (3) idempotency — second upsert on its own output is byte-identical; (4) beyond the issue's ask, non-identical glob cases (.env vs !.env.example, **/.pem vs !public.pem) are covered by the re-allow tail with dedicated tests. Two pre-existing steady-state tests were correctly updated to define "current" as the upsert steady state.
Findings
- No blocking findings.
- Prior cascade's major finding (awk -v block escape-processing could mangle backslash-bearing block lines) is LATENT with no present-day impact: the canonical baseline carries only glob patterns with no backslashes. The owner resolved the thread as a non-issue and CodeRabbit withdrew the comment and approved. Reasonable adjudication; ENVIRON would be a stylistic hardening, not a correctness fix.
- Prior cascade's minor finding (old unmarkered baseline header comments may duplicate in L2) was accepted by the owner as a cosmetic limitation — comments have no gitignore semantics; dropping them would risk clobbering repo-authored comments. CodeRabbit concurred and withdrew.
- All 6 inline review threads (Gemini CRLF, BATS temp-dir isolation, Copilot non-identical-glob gap, Copilot core.excludesfile determinism, CodeRabbit comment-dup, CodeRabbit awk -v) are resolved, with fixes verified present at head (tr -d '\r' at lib lines 41-42; mktemp under BATS_TEST_TMPDIR; re-allow tail + tests; git -c core.excludesfile=/dev/null).
- run_secret_scanning MCP tool not exposed in this environment; gitleaks CI passed and the diff contains no secret material.
- Neutralization logic reviewed for idempotency and edge cases (comment-only L2 emits no tail, blank-squeeze, marker replace-in-place path also neutralized) — sound.
CI status
All required checks green at head 1a36dcc: ShellCheck, Lint, Lint and bats, CodeQL (actions), SonarCloud Quality Gate, Secret scan (gitleaks), Agent Security Scan, AgentShield, npm audit, CodeRabbit (SUCCESS). A few dev-lead dispatch/ci-relay runs show CANCELLED/SKIPPED — these are superseded duplicate runs; the latest same-name runs succeeded. Ecosystem audits (pip/cargo/govulncheck/pnpm) skipped as not applicable. mergeStateStatus BLOCKED solely reflects the pending required review, which this approval addresses.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #809
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Bug Fixes
.gitignorepatterns from overriding baseline exceptions.Tests