feat(ci): wire the contract-slice prune gate the topic-docs convention specifies - #1429
Merged
Merged
Conversation
|
Claude encountered an error after 5s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
![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: 0e45ef2405
ℹ️ 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".
…n specifies Closes #1417 ## Summary `docs/conventions/topic-docs/README.md` specifies a required check that a merged PR carries no path under the contract-slice dir. The check was never built, so the convention has been unenforced for its entire life and 17 slices reached `main` — 6 of them on a single day. The only place `docs/topics/` reached CI at all was `scripts/docs-only-paths.txt`, as a docs-only ALLOWLIST entry, which makes such a PR cheaper to merge rather than blocking it. Evidence the rule is real and was being enforced by hand: PR #1286 was closed rather than merged, explicitly because its content was contract tier under `docs/topics/`. ## The deletion exemption The convention's own step 4 is a final commit that PRUNES the slice, so a literal "no path under the contract dir appears in the diff" reading would red-line the very commit that satisfies it. This gate keys on where a path LANDS: removals pass, a history-preserving `git mv` out of the contract dir (step 3's graduation) passes, and only an add, edit, or rename-into is red-lined. That requires knowing a path's status, which `--name-only` cannot express, so the gate reads `--name-status`. Deliberate deviation from the letter of the convention in service of its intent; the three-dot `base...HEAD` range is unchanged. ## Existing debt The 17 pre-existing slices are grandfathered by slug in `scripts/contract-slice-baseline.txt`, using the same stale-guarded idiom as `changelog-parity-baseline.txt` and `orphaned-fixtures-baseline.txt`: `--check` fails on an entry whose slice no longer exists, so an exemption cannot outlive its debt and a future slice cannot inherit a grandfathered slug. Graduating and pruning them is tracked separately. This is why the gate can land now instead of after a 71-file cleanup: it stops the bleed immediately while each slice graduates on its own PR, by whoever owns it. ## Verification The 11-case suite covers the add, pure-deletion, untouched, grandfathered, new-slug-despite-baseline, graduation-out, rename-into, unresolvable-base, live-baseline, stale-baseline, and usage paths. Measured against the four open PRs that carry `docs/topics/` paths, rather than asserted: #1318, #1252, and #1096 pass on their baseline exemptions; #1400 fails, correctly, because it adds two slices that are not pre-existing debt. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ontract_dir Two review findings, both reproduced before fixing. ## P1 — the gate was bypassable by self-grandfathering The baseline was loaded from the working tree, so ONE change set could add `docs/topics/<slug>/` and add `<slug>` to the baseline together: `--check` accepted it because the directory existed, `--check-diff` exempted the path because the line existed, and the required gate passed. That is the exact edit the baseline's own header prohibits, and prose cannot enforce it. Exemptions now resolve from the BASE revision, so only debt already grandfathered before the change set can exempt anything. A baseline line grants nothing to the PR that adds it. The failure message says so, since adding the slug is the obvious next thing a blocked author would try. ## P2 — contract_dir ignored the tracked concern file `docs/conventions/topic-docs/README.md` makes `.claude/topic-docs.yaml` the runtime authority for `contract_dir`, resolved before the documented default. The gate hardcoded `docs/topics`, so a repo that relocated its contract root would have had the real root silently unpoliced while the gate reported green against a directory nothing writes to — a fail-open on the check's whole subject. It now resolves from the concern file, also read from the base revision (for the same reason as the baseline: relocating the root in the same PR that adds a slice under the old one would otherwise dodge the gate). A root-equivalent value exits 2 rather than policing the entire repo. ## Baseline correction #1400 merged while this PR was open, landing `interview-batch-rounds` and `shadowed-skill-renames` on main. They are now pre-existing debt rather than incoming work, so they join the baseline: 17 -> 19. Without this the gate would red-line unrelated PRs for slices they did not introduce. Five cases added (16 total): self-grandfathering rejected, a pre-existing entry still exempting, contract_dir resolved from the concern file, a relocated root moving the gate's scope, and a root-equivalent value exiting 2. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
force-pushed
the
feat/1417-contract-slice-prune-gate
branch
from
July 26, 2026 00:28
0e45ef2 to
5ec5aee
Compare
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ec5aeeacb
ℹ️ 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".
Two more review findings, both reproduced before fixing.
## P1 — a relocation left the root it selected uninspected
Resolving contract_dir from the base revision stopped a PR narrowing its own
scope, but introduced the mirror gap: a PR that RELOCATES contract_dir could
migrate the grandfathered slices to the new root and add an unpruned slice
alongside them, and --check-diff would still be reading the old root.
Reproduced: relocating docs/topics to docs/slices while adding
docs/slices/newslug passed both workflow steps. --check saw the head root and
reported healthy; --check-diff saw the base root and found nothing.
The diff is now checked against the UNION of the base root and the head root,
so neither side of a relocation can carry an unpruned slice. Identical values
collapse to one, and messages name every root actually policed rather than
implying a single one. A legitimate relocation that carries its debt across
still passes.
## P2 — the intended end state crashed the gate
`declare -A grandfathered` leaves the variable unset under `set -u`, so
`${#grandfathered[@]}` aborted with "unbound variable" whenever the baseline
held no slugs. That is exactly the state the debt burn-down is driving toward:
the gate would have broken the moment the last slice was pruned. Now explicitly
initialized empty, with the key expansion guarded the same way.
Three cases added (19 total): a slug-less baseline surviving --check, a slice
under a same-PR-relocated root being red-lined, and a grandfathered slice
migrating to a relocated root still passing.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d9a91309e
ℹ️ 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".
Comment on lines
+135
to
+139
| add_contract_dir() { | ||
| local dir="${1%/}" | ||
| [[ -z "$dir" || "$dir" == "." || "$dir" == "/" ]] && { | ||
| echo "check-contract-slice-prune: resolved contract_dir is root-equivalent ('$1'); refusing to run." >&2 | ||
| exit 2 |
There was a problem hiding this comment.
When a valid repo-relative setting is written as contract_dir: ./docs/topics (or contains reducible segments such as docs/x/../topics), Git emits diff paths as canonical repo-relative names without those segments, while slug_of compares them with this raw value. I reproduced an edit under docs/topics/foo/PLAN.md passing the workflow checks with contract_dir: ./docs/topics; canonicalize the configured path and reject escapes or root-equivalent results before storing it.
Useful? React with 👍 / 👎.
Comment on lines
+89
to
+92
| value="$(printf '%s\n' "$content" | | ||
| sed -n 's/^[[:space:]]*contract_dir[[:space:]]*:[[:space:]]*//p' | | ||
| head -n1 | | ||
| sed 's/[[:space:]]*#.*$//; s/^["'"'"']//; s/["'"'"']$//; s/[[:space:]]*$//')" |
There was a problem hiding this comment.
When a valid plain YAML scalar contains an adjacent literal hash, such as contract_dir: docs/a#b, this unconditional comment stripping resolves it as docs/a; both workflow steps then ignore changes under the actual docs/a#b root. The repository's lib/parse-concern-value.sh already distinguishes a comment-starting # from an adjacent #, so use equivalent quote-aware parsing here rather than truncating every hash.
Useful? React with 👍 / 👎.
Comment on lines
+180
to
+183
| "$dir"/*) | ||
| rest="${path#"$dir"/}" | ||
| printf '%s' "${rest%%/*}" | ||
| return 0 |
There was a problem hiding this comment.
When a PR relocates docs/topics beneath a grandfathered slice—for example to docs/topics/legacy while legacy is baselined—this returns the base root's slug on the first match and never evaluates the more-specific head root. Fresh evidence beyond the prior relocation fix is that migrating the old slice to docs/topics/legacy/legacy and adding docs/topics/legacy/newslug makes both workflow checks pass because newslug is misclassified as the exempt outer slug legacy; evaluate every matching root (or at least the most-specific one) before granting an exemption.
Useful? React with 👍 / 👎.
This was referenced Jul 26, 2026
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
Closes #1444 ## Problem The contract-slice prune gate is a required check, and three separate defects let it report success over a contract root it never actually examined. All three were live P2 review threads on #1429 and landed unaddressed when that PR merged. ## What changed **The concern-file parse is delegated to `lib/parse-concern-value.sh`.** The gate carried a local `s/[[:space:]]*#.*$//` fork that truncates a valid scalar at an ADJACENT hash — `contract_dir: docs/a#b` resolved to `docs/a`, leaving the real root unpoliced. That helper is this repository's single source of truth for reading a topic-docs concern scalar and already distinguishes a comment-starting ` #` from an adjacent one. It takes a file, so a revision's content is materialized to a temp file first; a missing helper is fail-closed, never a fallback to the weaker fork. **The resolved root is lexically canonicalized before matching.** Git emits diff paths canonically, so `./docs/topics` and `docs/x/../topics` — both valid settings — matched nothing and every slice under them passed. An absolute or repo-escaping result is now refused with a distinct message rather than silently policing nothing. The canonicalization is purely lexical: the root need not exist yet. **`slug_of` now picks the MOST SPECIFIC matching root.** Contract roots can nest, because the gate deliberately polices the union of the base root and the head root and a PR may relocate `contract_dir` beneath the base root. Returning the first match named the inner root's own directory as the slug, so a baselined outer slug exempted every new slice inside the relocated root: migrating a grandfathered slice to `docs/topics/legacy/legacy` and adding `docs/topics/legacy/newslug` alongside it passed both workflow checks. Roots are canonical and non-empty, so longest-prefix is unambiguous. ## Test plan Five new tests, each written to the reported reproduction. Run against the pre-fix script they fail; against this one the suite is 26 passed, 0 failed: ```text FAIL: a './'-prefixed contract_dir must still police its root FAIL: a contract_dir with a '..' segment must resolve to its real root FAIL: an escaping contract_dir must exit 2, got rc=0 FAIL: an adjacent '#' must stay part of contract_dir FAIL: a new slice under a nested relocated root must be red-lined ``` Two further cases pin behaviour the fixes must not break — a trailing ` # comment` is still stripped, and a grandfathered slice may still migrate into a nested root. The test fixture now copies `lib/parse-concern-value.sh` into each throwaway repo, since the gate depends on it. `shellcheck` and `shfmt -d` are clean on both changed files. ## Related - #1444 — the defects this fixes - #1429 — the PR that landed the gate - #1417 — the convention the gate enforces - #1419 — the baseline debt burn-down the exemptions cover
Contributor
Author
|
Post-merge note for anyone auditing this PR: the three P2 review threads above (baseline-relative rejection aside, they cover the They are tracked as #1444 and fixed in #1445, which also carries the four further findings the review surfaced while fixing them. Each fix has a test that fails against the script as merged here. |
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
… A) (#1459) *This was generated by AI during work-loop execution.* No related issue: this PR is Phase A of the multi-phase parent issue #1424 and deliberately does not close it — Phases B (graduate-then-prune) and C (register hygiene) remain open under #1424; see `## Related` below. ## Summary Phase A of #1424: mechanically prunes the 8 `docs/topics/` contract slices the issue classifies as done, shipped, and un-linked — no graduation judgment needed. Per the issue's own "Suggested PR shape" ("One PR per phase... Phase B and C carry judgment and should not ride along with it"), this PR is Phase A only; Phases B (graduate-then-prune, 3 slices) and C (register hygiene, 6 slices) remain open under #1424. Deleted: `plugin-fleet-sync-skill`, `underspecification`, `handoff-bg-cutover`, `plugin-organization`, `github-plugin-candidates`, `dometrain-mcp`, `babysit-prs-migration`, `boris-video-absorption`. Two of the eight carried an explicit precondition in the issue ("route to the tracker or drop deliberately before deleting" / "decide explicitly whether any are worth a tracker issue first") — discharged by filing #1456 (dometrain-mcp's two open MCP-host behavior questions) and #1457 (boris-video-absorption's three deferred-with-trigger items), rather than letting deletion silently drop them. Also drops the corresponding 8 slugs from `scripts/contract-slice-baseline.txt` (the prune gate added by #1429, merged after #1424 was filed) — the file's own header requires the slug line to go in the same PR that prunes the slice, and its stale-guard (`--check`) would otherwise flag these 8 entries as pointing at nothing. ### Known deviation from the issue's literal verification bullet `docs/topics/` on `main` actually held **19** directories at the time this PR was authored, not 17 — `interview-batch-rounds` and `shadowed-skill-renames` landed via #1400, which merged 2 minutes before #1424 was filed (23:43:47Z vs. 23:45:57Z), so #1424's inventory predates them. This isn't a gap in #1424's classification: both are already accounted for as pre-existing debt in `scripts/contract-slice-baseline.txt` and tracked under #1419 (confirmed via that issue's own inventory-correction comment). No action needed here — noting it so the discrepancy reads as known, not missed. After this PR, `docs/topics/` holds 11 directories: the 6 Phase C register-hygiene slices, the 3 Phase B graduate-then-prune slices, plus `interview-batch-rounds` and `shadowed-skill-renames` (outside this issue's 17, tracked under #1419). ## Test plan - [x] `bash scripts/check-contract-slice-prune.sh --check` (working tree) — passes, 11 grandfathered entries all still name existing slices. - [x] `bash scripts/check-contract-slice-prune.sh --check-diff origin/main` — passes: "this change set leaves no path under docs/topics/". - [x] `git grep` for markdown links into the 8 deleted slugs, repo-wide — zero hits before and after (matches the issue's "zero inbound markdown links" claim). - [x] Spot-checked each of the 6 unconditional slices' claimed shipped artifact exists on disk (`plugins/claude-ops/skills/plugins/`, `plugins/session-flow/skills/continue-in-background/`, `plugins/github/skills/{audit,advise,setup}/`, `plugins/source-control/skills/{babysit-loop,babysit-prs}/`, dometrain plugin, `underspecification`'s self-declared COMPLETED marker, `plugin-organization`'s literal `(unfilled)` `## Plan`) — all present. - [x] Filed #1456 and #1457 before deleting `dometrain-mcp` / `boris-video-absorption`, discharging the issue's "every USER-RESERVED question and un-routed follow-up... exists as a tracker issue before its slice is deleted" verification bullet for these two. - This is a docs-only deletion diff; `docs/topics/` is on `scripts/docs-only-paths.txt:43` so heavy CI lanes should report evaluated-and-not-applicable per the issue's own PR-shape note — will confirm once CI runs. ## Related - #1424 (parent issue — Phases B and C remain) - #1419 (tracking issue for the full 19-slice burn-down) - #1456 (dometrain-mcp open questions, filed by this PR) - #1457 (boris-video-absorption deferred-with-trigger items, filed by this PR) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 9, 2026
…hell-portability token gaps (#2064) Discharges four stranded review findings against the repo-level `scripts/` gates. Every finding was reproduced through its real consumer with a pre-fix control before the fix, and each control discriminates — it passes on `origin/main`'s artifact and changes verdict on this branch's. ## Finding 1 — `check-contract-slice-prune.sh` accepts Windows-dialect absolute roots `PRRT_kwDOTCGFQM6Tz-jR` (#1445), `scripts/check-contract-slice-prune.sh:119`. `canonicalize_repo_path()` judged absoluteness only in the POSIX dialect (`/*`). A drive-qualified or backslash-rooted `contract_dir` was therefore read as repo-relative. Git names repo-relative diff paths with `/` separators and never with a drive qualifier or a raw backslash, so such a root can match no diff path at all — `--check-diff` reports success over an empty match set, policing nothing. That is precisely the fail-open the gate exists to prevent. The irony is worth stating plainly: **`19d736bf` (#1445) both discharged the three sibling threads on this file and introduced this one** — same harm class, different route in, inside the very function it added. Reproduced with three fixtures — `C:/outside`, `C:\outside`, `\\server\share` — all of which passed **silently** under `--check-diff` before the fix. The legitimate-root control fails correctly on the same machinery, so the reproduction discriminates rather than merely erroring. Absoluteness is now judged in both dialects. A backslash **anywhere** is refused, not only a leading one: it is a separator in the Windows dialect and an escaped character in Git's own output, so no value carrying one is comparable to a diff path. Note `c:outside` is drive-qualified but *relative* in Windows semantics — it is refused for the same Git-comparability reason, not because it is absolute. ## Finding 2 — the `--sort` token missed a fully quoted option word `PRRT_kwDOTCGFQM6T1s2n` (#1530), `scripts/shell-portability-tokens.txt`, filed at `:98`, live at `:173`. The pattern demanded whitespace immediately before `--sort`, so a quote wrapping the *whole* option word sat outside the newly added optional-quote position. `sort '--sort=version' "$file"` and `sort "--sort=version" "$file"` hand GNU sort the identical argument after quote removal, yet the scanner returned clean. An optional quote is now admitted at both positions. `git tag --sort=version:refname` stays clean for the reason it already did — the WORD boundary still rejects the longer `version:refname`. ## Findings 3 and 4 — the sed tokens, landed together Both edit the same live token at `:216`, so they cannot be split. **Finding 3** (`PRRT_kwDOTCGFQM6T1rfG`, #1534, filed `:117`, live at `:216` and `:232`): the unrestricted `[[:space:]][^\n]*` command gap crossed `;`, `&&` and `|`, so a *later* command's options armed the sed token. `sed -n 'p' "$file"; grep -Ei pattern "$file"` was reported even though only `grep` receives `-Ei`. The reporter also called the second site: *"the `--in-place` pattern has the same issue"* — so the `:232` edit is in scope as filed, not creep. Both gaps now stop at a shell command separator, matching what the `date -d` / `stat -c` / `mktemp -p` tokens already do. Quoted separators are neutralized before the token matches, so a `;` inside a sed script stays ordinary data. **Finding 4** (`PRRT_kwDOTCGFQM6T1rfI`, #1534, filed `:117`, live at `:188` and `:216`): scope is broader than filed. Two tokens read `sed -ni` clean — one keyed on a literal `-i` substring, which `-ni` does not contain; the other on an `E` earlier in the cluster. GNU sed 4.9's `--help` documents `-i[SUFFIX]` alongside the no-argument short options `-n`, `-b`, `-E`, `-r`, `-s`, `-u`, `-z`, so any cluster built from those letters and ending in `i` is the same unsuffixed in-place edit. Premise verified live against GNU sed 4.9: `sed -ni` rewrote a file in place, 3 lines to 1. The two narrower predecessors are therefore consolidated into **one** token — `-i` standing alone and `-i` ending a cluster are the same option. The argument-taking letters `-e`, `-f`, `-l` are deliberately outside the class: GNU accepts their value attached, so `sed -ei` passes the script `i` rather than editing in place. The removed `plain -i (no E) does not double-fire this cluster token` test goes with the consolidation — with one token there is nothing to double-fire. ## Verification Every row below ran through the real consumer, `scripts/check-shell-portability.sh --paths <fixture>`, never by hand-running `grep -E` against the token file. `PRE` uses `origin/main`'s token file via `SHELL_PORTABILITY_TOKENS`; `POST` uses this branch's. `0` = gate passes, `1` = gate reports the construct. | Fixture | PRE (main) | POST (branch) | Meaning | | --- | --- | --- | --- | | fully quoted `--sort=version` (4 spellings) | 0 | 1 | finding 2 false negative reproduced, then closed | | `sed -n 'p' f; grep -Ei p f` | 1 | 0 | finding 3 false positive reproduced, then closed | | `sed -n 'p' f && tool --in-place x` | 1 | 0 | finding 3's `--in-place` site, same shape | | `sed -ni '/keep/p' f` | 0 | 1 | finding 4 false negative reproduced, then closed | | `sed -i '' 's/foo/bar/' f` | 1 | 1 | **regression guard** — the space-separated empty-suffix catch survives consolidation | | `sed -i.bak` / `sed -Ei.bak` | 0 | 0 | **regression guard** — the dual-compatible escape hatch stays clean | The last two rows exist because this change *deletes* a token. The empty-suffix idiom looks BSD-safe but is not (GNU consumes the empty string as sed's script argument and exits 2), and the attached-nonempty-suffix form is the one genuinely portable spelling. Neither may shift. ### The self-gating trap `shell-portability-lint` gates this PR, and these edits change the lint that runs against this PR's own diff. A fixture-only check would not have caught a token that newly flags existing legitimate code, so the whole-repo audit was run under both token files and diffed: - `scripts/check-shell-portability.sh --all` with `origin/main`'s tokens and with this branch's tokens produce a **byte-identical** hit set. - The remaining hits are pre-existing whole-repo debt present identically on main (`\b` / `\w` / `\s` / `\S` in four test files); `--all` exits 1 on both. CI's gate is changed-file scoped, so that debt is not this PR's to carry. - CI's actual invocation, `scripts/check-shell-portability.sh origin/main`, is green: *No unexcused GNU-only constructs in 3 shell file(s).* Nothing was suppressed and no `portability-ok:` marker was added. ## Changelog parity **Not applicable to this PR**, determined by reading the gate rather than assuming. `scripts/check-changelog-parity.sh` scopes itself to plugins: `--check` and `--check-bump` glob `plugins/*/.claude-plugin/plugin.json`, and `--check-order` globs `plugins/*/CHANGELOG.md docs/conventions/*/CHANGELOG.md`. This diff touches only repo-level `scripts/`, which has no plugin manifest, and the repo has no root `CHANGELOG.md`. Worth closing the one loose end explicitly: `--check-bump` does take `origin/$BASE_REF`, so a diff-scoped gate could in principle fire on a scripts-only diff. It cannot here — the diff scope is used only to detect *manifest version changes*, and this PR changes no manifest version. ## Scope Four hunks, four findings, no unmapped changes. The three sibling threads on `check-contract-slice-prune.sh` (all #1429) are **already fixed** by `19d736bf` (#1445), confirmed an ancestor of main with pre-fix controls at `19d736bf^` reproducing both bugs; this PR deliberately contains no fix for them. ## Related No linked issue --------- Co-authored-by: Claude Opus 5 (1M context) <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.
Closes #1417
Problem
docs/conventions/topic-docs/README.md:305-309specifies a required check that the net PR diffcarries no path under the contract-slice dir:
It was never built. The convention has been unenforced for its entire life, and
maincurrentlyholds 19 contract slices — 8 of them landed on 2026-07-25 alone, two of those while this PR
was open.
The only place
docs/topics/reaches CI today isscripts/docs-only-paths.txt:43, and it is thereas a docs-only allowlist entry — which makes a PR confined to it cheaper to merge by skipping
the heavy lanes. Nothing blocks it.
The rule is real and people have been enforcing it by hand: PR #1286 was closed rather than
merged, explicitly because its content was contract tier under
docs/topics/. That is the costof the missing gate — correct behaviour depending on whoever is looking remembering an unenforced
rule, and 19 directories showing how reliably that scales.
#1400 merged while this PR was open, landing two more slices. It was flagged there, and it is
the sharpest available evidence for the gate: the failure mode is live, not historical.
Why the gate permits deletions
The convention's own step 4 is a final commit that prunes the slice. A literal reading — "no path
under the contract dir appears in the diff" — would red-line the very commit that satisfies the
convention.
So the gate keys on where a path lands, not on whether it appears:
docs/topics/git mv docs/topics/x/PLAN.md docs/adr/…docs/topics/docs/topics/docs/topics/Knowing a path's status requires
--name-status;--name-onlycannot express it. That is adeliberate deviation from the letter of the convention in service of its intent, called out in the
script header. The three-dot
base...HEADrange is unchanged, so a slicemaingained after abranch forked stays out of scope and no stale branch is forced to merge-from-main over someone
else's violation.
Why this lands before the cleanup
scripts/contract-slice-baseline.txtgrandfathers the 19 existing slugs, using the samestale-guarded idiom as
changelog-parity-baseline.txtandorphaned-fixtures-baseline.txt:--checkfails on an entry whose slice no longer exists, so an exemption cannot outlive its debtand a future slice cannot silently inherit a grandfathered slug.
Exemptions are resolved from the base revision, not the working tree, so a PR cannot add a
slice and grandfather its own slug in the same diff. The diff is judged against the union of the
base and head contract roots, so a PR that relocates
contract_dircannot leave the root itselected uninspected either. Both bypasses were live in earlier pushes and were caught in review.
The alternative — prune all 19 first, then gate — is a ~1.3 MB change requiring a graduation
judgement on each slice by whoever owns it, and it would conflict every open PR that carries those
paths. Gating first stops the bleed immediately while each slice graduates on its own PR at its own
pace. The burn-down is #1419; each prune PR drops its own baseline line, and the stale guard means
the debt cannot be quietly abandoned half-done.
Verification
scripts/check-contract-slice-prune.test.sh— 19 cases, all green: add, pure deletion, untouchedtree, grandfathered slug, new slug despite a baseline, graduation out, rename in,
unresolvable-base-ref (fail-closed, exit 2), live baseline entry, stale baseline entry, usage,
self-grandfathering rejected, a pre-existing entry still exempting,
contract_dirresolved fromthe concern file, a relocated root moving the gate's scope, a root-equivalent value exiting 2, a
slug-less baseline surviving
set -u, both base and head roots policed, and a grandfathered slicemigrating to a relocated root.
Four review findings were raised across two rounds and all four were reproduced before being
fixed — two bypasses (self-grandfathering; a relocation leaving its own root uninspected) and two
fail-open / crash defects (
contract_dirignoring the concern file; the gate aborting underset -uonce the baseline empties, which is the exact end state #1419 drives toward). See theresolved threads; each carries its reproduction and the case that pins it.
Measured against the open PRs that actually carry
docs/topics/paths, rather than asserted.Because the gate reads the baseline from the base revision, these were run against a base that
already carries it — the post-merge condition:
plugin-audit-portfresh-eyes-checkpoint-auditcontext-engineering-claude-5+ the two slices #1400 landedmaincarrying the updated baseline#1318's failure is an artefact of it predating #1400's merge, not a defect: its branch adds those
two files relative to its own fork point. Once rebased, they are on
mainand in the baseline, sothey leave its diff entirely.
The self-grandfathering bypass was verified closed by re-running the reviewer's own reproduction
against the fix.
Also verified:
shellcheckclean,actionlintclean,shfmtclean, the org comment-hygiene policyreports zero violations in the new files, and both scripts carry the executable bit.
Wiring
contract-slice-prune-gateis added toci-status'sneeds:list. That aggregate derives itslane list from the needs graph, and
ci-statusis already a required status check on the ruleset,so the new gate becomes required with no ruleset edit. Job naming matches the existing
precedent (
silent-skip-gate,orphaned-fixture-gate,changelog-parity-gate). The self-test runsunconditionally so a broken gate cannot mask a regression; the PR-diff step is event-gated.
Related
and this gate's stale guard fails once an entry outlives its slice. Not closed by this PR. Its
inventory needs updating to 19 once this lands.
debt rather than treated as incoming work. Flagged there before it merged.
exemptions. feat(claude-config): add audit-pass, one coordinated resumable instruction-surface pass #1318 needs a rebase past docs(topics): land two topic plans that existed only in a local stash #1400 (see Verification).
gate replaces.
docs/conventions/topic-docs/README.md— the convention specifying this check as step 5.