docs(source-control): correct stale-base gate coverage and drop the strict recommendation - #2830
Merged
Merged
Conversation
…trict recommendation
The stale-base overlap gate's header cited claude-code-plugins#2691's
2026-08-15 incidents ("#2635 then #2639 lost inside ten minutes") as its
motivating case, and named GitHub's `requiredStatusChecks.strict` as "the
durable prevention". Both claims are wrong, and the wording had propagated
into ci.yml, the plugin CHANGELOG, and two source-control skill references.
Coverage. Those incidents were stale in CONTENT, not in BASE: `git merge-base
--is-ancestor f603880 refs/pull/2641/head` exits 0, so #2639 was already in
#2641's ancestry, while the tree carried none of its work. Run against those
exact branches the gate exits 0 ("fresh"). The class that covers them is
`scripts/check-silent-revert.sh`. The two detectors cover disjoint classes and
neither subsumes the other; the docs now say so explicitly.
Prevention. Recommending `requiredStatusChecks.strict` contradicts an accepted
ADR in the org's IaC repo (implemented as a non-strict ruleset), and the same
evidence shows strict — and a merge queue — would have passed all three
incidents. Replaced with a neutral statement of what the gate does and does not
cover; nothing recommends flipping the setting.
Wording only: no control flow, job wiring, or exit codes changed. The two
altered lines inside the script are stderr prose in `echo` strings.
`scripts/check-stale-base-overlap.test.sh` passes (ALL PASS, exit 0).
Context: #2691
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 38s —— View job Security review
Result: No security issues found. This PR is a wording-only correction across 6 files:
No injection surfaces, permission changes, credential handling, or Actions trigger/permission changes are present in this diff. Skip gate condition 2 applies (no security-relevant surface). |
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a6a0dd12e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude finished @kyle-sexton's task in 2m 9s —— View job Code review complete ✅
Scope note (skip gate): this is a wording-only correction (comments, doc prose, a changelog entry, and a version bump — no executable logic changed in What I checked
I was not able to run FindingsNone. The changes are internally consistent:
No correctness, maintainability, or consistency issues found in the changed lines. |
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ass-wide Review feedback on #2830: saying the content-stale class "is caught post-merge by scripts/check-silent-revert.sh" over-promises twice over. That detector is threshold-bounded (SILENT_REVERT_THRESHOLD=200 deleted lines from a single culprit, SILENT_REVERT_WINDOW=40 first-parent commits — check-silent-revert.sh lines 152-153), so smaller or older content reverts produce no finding. And freshness.md / merge.md ship to consuming repos, where a repo-root scripts/check-silent-revert.sh need not exist at all. Both now say the class needs its own post-merge detector, name this repo's as a bounded heuristic rather than class-wide coverage, and state that a consuming repo may have none. Prose only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 45s —— View job Security review
Result: No security issues found. Skip gate condition 2 applies — no security-relevant surface. The only commit since the last-reviewed head (
Both edits are wording-only: they clarify that the silent-revert detector is repo-specific and threshold-bounded, and that a consuming repo may lack it entirely. No logic, permissions, triggers, or injection surfaces are touched. This is consistent with the full-PR review already posted for |
|
Claude finished @kyle-sexton's task in 2m 1s —— View job Code review complete ✅
Scope note (skip gate): wording-only correction — comments, doc prose, a changelog entry, and a version bump; no executable logic changed ( What I checked
I was not able to execute FindingsNone. The corrected framing is internally consistent across all five prose surfaces ( |
This was referenced Aug 16, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 16, 2026
…2839) ## What The two portability suites build their diff-mode fixtures like this: ```bash out="$( cd "$fx" && git init -q && git config user.email test@example.com && # <- names no repository ... )" ``` This PR names the fixture on every fixture-writing git command, adopting the `git -C "$fixture" ...` idiom the sibling fixtures in the same directory already establish (`scripts/check-stale-base-overlap.test.sh:22`, `scripts/sync-standards-contract.test.sh:69`). 21 lines changed, all of them git invocations. No assertion or coverage change. ## Honest finding: the named sites were already cwd-scoped The motivating report described these as unscoped commands writing into the caller's `.git/config`. **They are not, under normal invocation.** The `cd "$fx" &&` guard at the head of the `&&` chain scopes them, and I could not reproduce a leak: ``` $ # verbatim replay of the pre-fix block, run from a worktree of this repo BEFORE: 0 local user.* keys subshell rc=0 out=695e71c3ea35179a224c36992b6408d6de284d94 AFTER: 0 local user.* keys (none in caller repo) fixture config: user.email=test@example.com user.name=test ``` Whole-suite before/after, against the shared `.git/config` this worktree and the main clone both use: | phase | suite | caller `user.*` before | caller `user.*` after | result | |---|---|---|---|---| | pre-fix (`HEAD~1` copies) | check-shell-portability.test.sh | none | none | PASS=333 FAIL=0 | | pre-fix (`HEAD~1` copies) | check-skill-portability.test.sh | none | none | PASS=89 FAIL=0 | | post-fix | check-shell-portability.test.sh | none | none | PASS=333 FAIL=0 | | post-fix | check-skill-portability.test.sh | none | none | PASS=89 FAIL=0 | So this is **not** a demonstrated bug fix. It is an idiom-consistency and copy-paste-hazard change: a line reading literally `git config user.email test@example.com` at statement position in a test file is one paste away from poisoning a real repository, which is the plausible route to the #2827 damage. (The branch name says `fix/`; the change is hygiene. The landed squash commit takes this PR's title and body, so history records it accurately.) Note the `cd "$fx"` is retained deliberately and stays load-bearing — the relative `printf … > 'plugins/…'` writes and the relative `bash scripts/check-*-portability.sh` invocation all need cwd = `$fx`. So `-C` here is a second, explicit scoping mechanism alongside the existing implicit one, not a replacement for it. `$fx` is always `mktemp -d` output, i.e. absolute, so `-C "$fx"` resolves correctly from any cwd. ## Follow-up finding: `-C` is not a fixture-isolation guarantee The one way I *could* manufacture the leak is an exported `GIT_DIR` — the environment git hands to every hook it invokes. `git config`'s default `--local` scope follows `GIT_DIR`, not the working directory: ``` GIT_DIR=D:/repos/.../claude-code-plugins/.git BEFORE: (no local user.* keys) subshell rc=0 gitdir-it-used=D:/repos/.../claude-code-plugins/.git AFTER: user.email=test@example.com user.name=test ``` **`git -C "$dir"` does not fix that** — I verified the prescribed idiom is equally vulnerable (the fixture ends up with no `.git` at all and the caller's config is poisoned): ``` == git -C "$fx" init/config under exported GIT_DIR == fixture has .git? NO caller config user.*: user.email=test@example.com user.name=test == git -C with GIT_DIR/GIT_WORK_TREE unset in the subshell == fixture2 has .git? yes caller config user.*: (clean) ``` Only `unset GIT_DIR GIT_WORK_TREE` (or `env -u`) actually isolates a fixture. I did **not** build that here — it is a new shape across ~30 test files and is not justified by any demonstrated need in this repo (no `core.hooksPath`, no installed hooks, and CI invokes the suites as plain steps). Filing it as a finding instead: *the established `git -C` fixture idiom is a readability and copy-paste guard, not an isolation guarantee.* ## Sweep Three passes, all for repo-writing git verbs (`config|init|add|commit|checkout|switch|branch|remote|tag|reset|rm|mv|stash|update-ref|worktree|clone|push|fetch`) at shell-statement position without `-C`: 1. All 254 `*.test.sh` / `tests/` / `run-tests.sh` files — 285 candidate hits. 2. All `*.ps1 *.psm1 *.py *.js *.ts` — every hit is a comment, docstring, or string literal; **no executable git write command** in any of them. 3. The `*.sh` files that build `mktemp -d` fixtures but are *not* named like tests (so pass 1 would have missed them) — 2 hits, both `git init -q -b main "$repo"` in `plugins/source-control/skills/worktree/fixtures/`. **Every site is scoped**, by one of: - `-C <dir>` — e.g. `sync-standards-contract.test.sh:69`. - An explicit path *operand* — `git init -q "$REPO"`, `git clone -q <src> <dest>` (the repo-hygiene, source-control, docs-hygiene and preflight fixtures). - A guarded `cd` into the fixture, `(cd "$d" || exit 1; …)` or `(cd "$d" && …)`, which also covers the `git init -q .` sites (`exec-bit-check.test.sh:25`, `block-noncanonical-commit.test.sh:358`) — those are cwd-scoped by the enclosing `cd`, not by their `.` argument. - An explicit `git config -f <file>` (`preflight.test.sh:416,417,495`). Everything else that matched is a quoted test payload — permission-rule strings like `Bash(git push)`, PowerShell here-string bodies, `make_skill` heredoc content — never executed. No remaining unscoped site. One structural note, reported not touched (the file is owned by a concurrent lane): `scripts/check-stale-base-overlap.test.sh` scopes its bare `git checkout` / `git add` / `git commit` with a **top-level** `cd "$repo" || exit 1` rather than a subshell-local one. It is guarded, so it is scoped — but it is more fragile than the sibling pattern, since the cwd persists into everything after it. ## Verification - `bash scripts/check-shell-portability.test.sh` -> PASS=333 FAIL=0 - `bash scripts/check-skill-portability.test.sh` -> PASS=89 FAIL=0 - No plugin manifest touched, so the changelog-parity gate does not apply (it is scoped to `plugins/<name>/`; there is no root CHANGELOG.md). `changelog-parity-gate` passes on this PR, confirming it. Pass counts are identical pre- and post-change (333 and 89), which is the coverage-unchanged proof: the diff touches 21 lines, all of them git invocations, and no `ok "` / `fail "` assertion line. ## Related No linked issue. This is fixture-isolation hygiene in two test suites; it closes no GitHub issue. - #2840 — the follow-up finding this PR surfaced but deliberately does not fix: `git -C` does not survive an exported `GIT_DIR`, so the established idiom is a copy-paste guard rather than an isolation guarantee. - #2827 / #2830 — the abandoned-and-rebuilt PR pair that motivated this work; a commit re-authored to `test@example.com` failed `required_signatures` with `no_user` and could not be force-pushed over. - `scripts/check-stale-base-overlap.test.sh` and `scripts/sync-standards-contract.test.sh` — the sibling fixtures whose idiom this PR adopts. Neither file is modified here (both are owned by concurrent lanes); they are read-only prior art. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018S8a1S71VxhLTRWBtMuEvp Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 16, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 17, 2026
…#2876) ## Summary A test suite that builds a git fixture can write that fixture's identity into the **caller's** repository. This makes that impossible in the shared harnesses, fixes the Python suite that actually caused it, and adds a CI gate so a new suite cannot reintroduce it. **A correction to the framing in #2840 first, because the fix depends on it.** The issue says "`GIT_DIR` is exactly what git exports into every hook it invokes", implying hooks are the footgun. Measured, that is not the mechanism: - `git -C <dir>` **does** chdir. What an exported **absolute** `GIT_DIR` overrides is repository **discovery**, and `git config` writes its default `--local` scope to whatever `--git-dir` finally resolves to. So under an absolute `GIT_DIR`, `git -C <fixture> config user.email X` writes the **caller's** config and leaves the fixture with **no `.git` at all**. - The **relative** form (`GIT_DIR=.git`) is safe, because `-C` chdirs first and `.git` then resolves against the new cwd. So the footgun is the **absolute** form, not the mere presence of `GIT_DIR`. Confirmed on this machine: no git hook exists at any scope, `core.hooksPath` is unset everywhere, and the sole Claude Code PreToolUse hook invokes no git — the exported `GIT_DIR` in the real incident came from an ad-hoc command, not a hook. The general shape is worth stating, because it is what justifies clearing the environment rather than patching call sites: **an ambient environment variable silently redirected a git operation that looked correct at the call site.** The same class bit this repo a second time this week through `MSYS_NO_PATHCONV`, where an exported value leaked across a compound command and made `git worktree add` create a directory off the wrong drive. Test fixtures must not inherit ambient git-relevant environment at all. Two adjacent leak paths, both live here, that the issue does not mention and that rule out the narrower `-c` transient fix: - `git -C <linked worktree> config user.email` writes the **main** repo's **shared** `.git/config` — git creates no `config.worktree`. This repo runs dozens of linked worktrees. - `git -C <non-repo dir nested inside a repo> config` walks **upward** and writes the enclosing repo's config. `git -C <dir> -c user.email=... <cmd>` prevents config poisoning but still operates on the wrong repository, and does nothing for either path above. Clearing the environment is the fix that covers all three. Why this matters operationally: a poisoned `user.email` silently re-authors commits. Such a commit fails this repo's `required_signatures` rule with `no_user` and **cannot be force-pushed over** — the branch has to be abandoned and rebuilt. That is #2827 -> #2830. Closes #2840 ## Fix **1. `scripts/test-git-helpers.sh`** — clears `GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_COMMON_DIR GIT_PREFIX GIT_OBJECT_DIRECTORY` once at **source time**, so every current and future consumer is immunized without a per-call-site edit. The same clear is added to `scripts/run-plugin-tests.sh` and the two plugin test harnesses (`claude-ops`, `guardrails`). The guard lives **inside** the harnesses and test files, not as a CI-side `env -u` wrapper. The poisoning incident was an ad-hoc **local** run, and no single test runner exists (CI names each suite individually). A wrapper would not have prevented the actual incident. **2. Python** — `plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py` is the file that fired. Its `create_checkout` runs `git -C <checkout> config user.email test@example.com`. One module-level `os.environ.pop` loop at import, not 46 call-site edits: fixtures are built from two `TestCase` classes with no shared `setUp`, none of the git subprocess calls pass an explicit `env=`, and spawned subprocesses inherit `os.environ`, so one clear covers every path. Because it runs at **import**, it takes effect under CI's `GuardTests`-only invocation too. The same guard is added to the only two other Python suites in the repo that build git fixtures: `test_prune_babysit_worktrees.py`, which also builds a **linked worktree**, and `test_check_contract_clause_coverage.py`, which runs `git init` with `cwd=` and no path argument at all — under a leaked `GIT_DIR` that initializes the caller's gitdir and stages into the caller's index. **3. `scripts/check-fixture-git-isolation.sh`** — a new gate, wired into `ci.yml` as `fixture-git-isolation-gate` and registered in `ci-status.needs`. **Why a standalone gate and not a class inside `scripts/check-shell-portability.sh`** (which was the original plan): that gate is scoped to `*.sh`. The file that caused the incident is a `test_*.py`. **A gate that structurally cannot see the file that fired is not a gate.** The standalone `scripts/check-*.sh` + `scripts/*-baseline.txt` pair is also this repo's established shape for exactly this kind of check (`check-orphaned-fixtures.sh`, `check-changelog-parity.sh`), so this conforms to the existing pattern rather than adding a second way. A useful side effect: `scripts/check-shell-portability.sh` is **untouched**, so this PR does not contend with the other lane working there. Gate details: - Covers `*.test.sh`, `test_*.py`, `*_test.py`. The Python clearing check is **file-scoped**, not line-scoped, because the idiom spans lines. - Detection intents cover the identity-**write** spellings a `config`-adjacent pattern missed (`git config --local user.email`, `git -c <k>=<v> init`) while **excluding** the `--get` / `--list` **read** spellings, which cannot poison anything. - A suite whose subject IS this mechanism opts out with a line-anchored `fixture-isolation-scope:` declaration, mirroring the existing `portability-scope:` precedent. Anchoring at the start of the comment content is deliberate: prose mentioning the token must not silently exempt a file. This is distinct from the baseline — a declaration records a permanent property, the baseline records drainable debt. ## Verification **Discrimination proof — the regression test fails pre-fix and passes post-fix.** `scripts/test-git-helpers.test.sh` covers all three leak paths. It honors `TEST_GIT_HELPERS_UNDER_TEST` so the pre-fix run needs no tracked-file edit. Every assertion reads the caller identity with `config --local --get`; a plain `--get` falls through to `~/.gitconfig` at rc=0 and would mask the leak. Pre-fix (the `origin/main` harness): ``` harness under test: .../prefix/test-git-helpers.sh FAIL: A absolute GIT_DIR: caller identity POISONED (before=sentinel@example.invalid after=t@t.test) FAIL: A absolute GIT_DIR: fixture has NO .git — the work went to the caller instead FAIL: B linked worktree: caller identity POISONED (before=sentinel@example.invalid after=t@t.test) FAIL: B linked worktree: fixture has NO .git — the work went to the caller instead FAIL: C nested non-repo dir: caller identity POISONED (before=sentinel@example.invalid after=nested@example.invalid) FAIL: C nested non-repo dir: fixture has NO .git — the work went to the caller instead FAIL: harness does not clear: GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_COMMON_DIR GIT_OBJECT_DIRECTORY passed: 0 failed: 7 ``` Post-fix (the shipped harness): ``` harness under test: .../scripts/test-git-helpers.sh ok: A absolute GIT_DIR: caller identity intact (sentinel@example.invalid) ok: A absolute GIT_DIR: fixture owns its own .git ok: B linked worktree: caller identity intact (sentinel@example.invalid) ok: B linked worktree: fixture owns its own .git ok: C nested non-repo dir: caller identity intact (sentinel@example.invalid) ok: C nested non-repo dir: fixture owns its own .git ok: harness clears every discovery-redirecting variable passed: 7 failed: 0 ``` Scenario B independently confirms the shared-config path: with `GIT_DIR` set to a **linked worktree's** gitdir, the write landed in the **main** clone's config and `extensions.worktreeConfig` was never created. **Gate self-test — 23 cases, all pass**, including a live-corpus canary: ``` ok - unknown flag exits 2 with usage ok - unisolated fixture suite is a violation (exit 1, names the file) ok - suite that unsets GIT_DIR/GIT_WORK_TREE passes ok - unsetting GIT_DIR alone is still a violation ok - sourcing a harness that clears the environment passes ok - a harness that stops clearing re-exposes its sourcing suites ok - a baselined violation is grandfathered ok - a stale baseline entry fails (a line cannot outlive its debt) ok - a suite that builds no fixture is not conscripted ok - a comment mentioning git init does not conscript a suite ok - --list reports verdicts and exits 0 ok - an identity write via 'config --local' is a violation ok - an identity read alone does not conscript a suite ok - a read sharing a line does not suppress an identity write ok - 'config --local --get user.email' is a read, not a fixture write ok - 'git -c <k>=<v> init' is a violation ok - python: unisolated fixture suite is a violation ok - python: popping GIT_DIR and GIT_WORK_TREE passes ok - python: popping GIT_DIR alone is still a violation ok - python: naming the variables without popping is still a violation ok - a declared fixture-isolation scope exempts the suite ok - a mid-line mention of the scope token does not exempt ok - live corpus passes against scripts/fixture-git-isolation-baseline.txt ALL PASS ``` **Gate against the real tree:** ``` $ scripts/check-fixture-git-isolation.sh --check fixture git isolation: OK (45 isolated, 36 baselined) # rc=0 $ scripts/check-fixture-git-isolation.sh --list | tail -1 45 isolated, 36 baselined, 0 violating ``` **False-positive rate, measured and disclosed.** The shipped detector classifies **81 files as fixture-builders (78 shell + 3 Python)**, re-derived from the shipped code rather than quoted from an earlier revision. An independent audit of the flagged set found **0 read-only false positives**, **0** files selected only by a string literal, and **0** selected only by the broad `worktree add` intent — the added intents conscript no file on their own. (That audit ran against an 80-file snapshot taken one commit earlier; the single added file is this gate's own self-test, which gained fixture text when the cases below were written. An earlier pass reporting 137 identity triggers across 75 shell suites predates the broadened detection entirely and is superseded.) Three known limits are recorded rather than hidden: - A harness sourced through a **variable** (`. "$HELPER"`) is invisible to the harness-resolution branch, which needs a literal `.sh` token. Only `scripts/test-git-helpers.test.sh` does this, and it carries an explicit scope declaration instead. - A file holding a `fixture-isolation-scope:` line as **test data** would be exempted by it. Verdict order mitigates this — actually clearing outranks declaring — so the only file reported `declared` is the one counter-fixture that genuinely cannot clear. - `--list` exits 0 unconditionally by design; `--check` is the failing mode. `--list` also prints the baselined count net of stale entries, so while draining the baseline the authoritative count is that file's own line count. Two false positives were found and closed during development rather than shipped: - `audit-fleet.test.sh`, conscripted by `config --get-all user.name` — a read. That is why the read spellings are excluded. - A line-wide read exclusion that would have let one `--get` suppress a real identity write sharing the line. Reads are now removed extent by extent. Measured before changing anything: four lines in the tracked corpus carry both shapes and all four are pure reads, so this closed a latent hole rather than a live one. Both directions carry test cases. ## Test plan All run locally on Windows / Git Bash; the output above is real, not paraphrased. - [x] `bash scripts/test-git-helpers.test.sh` — 7 pass / 0 fail (rc=0) - [x] same with `TEST_GIT_HELPERS_UNDER_TEST` = the `origin/main` harness — 0 pass / 7 fail (rc=1), proving discrimination - [x] `bash scripts/check-fixture-git-isolation.test.sh` — 21 cases, ALL PASS (rc=0) - [x] `scripts/check-fixture-git-isolation.sh --check` — rc=0 - [x] `python -m unittest test_hygiene.GuardTests` (the class CI runs) — `Ran 127 tests ... OK (skipped=3)` - [x] `python -m unittest test_hygiene` (whole module) — `Ran 303 tests`, 2 failures, both reproducing **identically on unmodified `origin/main`**, so pre-existing and not introduced here (tracked in #2871) - [x] `python -m unittest discover -s tests -p 'test_*.py'` (babysit-prs) — `Ran 643 tests ... OK` - [x] `python scripts/test_check_contract_clause_coverage.py` — `Ran 24 tests ... OK` - [x] `shellcheck --rcfile=.shellcheckrc -x` on every changed `.sh` — rc=0 each - [x] `shfmt -d -i 2 -ci` on every changed `.sh` — clean - [x] `ruff check` on all three changed `.py` — `All checks passed!` - [x] exec bits: both new `.sh` recorded `100755` in the index, the baseline `.txt` `100644` Not verified locally: GitHub-side execution of the new `ci.yml` job — that is what this PR's own CI run establishes. ## Related - #2840 — the issue this closes. Its "hooks export `GIT_DIR`" framing is corrected above; the absolute-vs-relative distinction is the actual mechanism. - #2827 / #2830 — the abandoned-and-rebuilt PR pair this leak produced. - #2839 — adopted the `git -C` idiom and deliberately deferred this hardening. - #2871 — **filed by this lane**: nine of ten `TestCase` classes in `test_hygiene.py` never run in CI, including the one holding the function that caused the incident. Measured; flipping to the whole module would land red, so it needs the staged baseline path. Deliberately **not** changed here — this PR is a fixture-isolation fix, not a CI-topology change. - #2872 — **filed by this lane**: drain the 36 grandfathered entries in `scripts/fixture-git-isolation-baseline.txt`. Staged behind the gate so it lands green; a gate that lands red is a gate on its way to being disabled. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Wording-only correction to two defects in how
scripts/check-stale-base-overlap.sh(shipped in #2799) describes itself, and to the same framing where it had propagated.The gate itself is unchanged and stays required. It correctly detects the stale-base class.
Defect 1 — false coverage citation
The header cited
claude-code-plugins#2691: #2635 then #2639 lost inside ten minutesas its motivating incident. Those incidents are a different class:The bases were fresh; the trees were stale. Run against those exact branches this gate exits 0 ("fresh"). A required check was naming incidents it does not detect.
The class that covers them is
scripts/check-silent-revert.sh(#2808). The two cover disjoint classes; neither subsumes the other. Every touched surface now says that explicitly.Defect 2 — a recommendation barred by an accepted ADR
The header called GitHub's
requiredStatusChecks.strict"the durable prevention". That is wrong twice over:melodic-software/github-iac→docs/adr/0001-relax-strict-required-status-checks.mdis ACCEPTED and implemented as a non-strict ruleset; its revisit trigger prescribes per-repo granularity, not a global flip.Replaced with a neutral statement of scope. Nothing here endorses the setting, softly or otherwise, and nothing recommends flipping it.
.github/workflows/silent-revert-canary.ymlandscripts/check-silent-revert.shalready carried the accurate framing and were deliberately left untouched.Files
scripts/check-stale-base-overlap.sh.github/workflows/ci.ymlneedstouchedplugins/source-control/skills/babysit-prs/reference/freshness.mdstrictsetting is not yet on" framingplugins/source-control/skills/pull-request/reference/merge.mdstrict: true"plugins/source-control/CHANGELOG.mdplugins/source-control/.claude-plugin/plugin.jsonVerification
bash -n scripts/check-stale-base-overlap.sh— OK. The header sits directly aboveset -uo pipefail; parsing is intact.bash scripts/check-stale-base-overlap.test.sh— ALL PASS, exit 0 (all five cases).echostrings.plugins/disk-hygiene/CHANGELOG.md:83describes the incidents as "squash-merged from stale bases" — the same wrong premise, but it is another plugin's release history and makes no claim about this gate's coverage. Left alone deliberately.Related
No linked issue. This corrects wording on surfaces shipped by an already-merged PR; nothing here closes a GitHub issue.
scripts/check-silent-revert.sh, the detector that actually covers the disjoint class.melodic-software/github-iac→docs/adr/0001-relax-strict-required-status-checks.md— the ACCEPTED ADR that bars the recommendation removed here. Not touched by this PR.