fix(gates): close the review findings in the gates and their docs
Two reproduced bugs in check-skill-version-bump: - The origin/main-tip check fired even when the pushed skill was byte-identical to main's tip, so a cherry-pick or backport failed a push that ships nothing. The merge-base intersectionea119d8added covers that only when some base carries the content, which a criss-cross history gives and a linear one does not. A new same_subtree compares tree object ids, so the exemption holds whatever route the history took. - The failure line reported "baseline: none" when the skill was absent at every merge-base but present at the tip, and the Fix: line then named no version. The author writes the natural 1.0.0 and gets a second blocked push. It now falls back to the tip's version. ADR-0022 is not amended: the documented behaviour does not change, andea119d8set the precedent by fixing the same failure class script-only.1614bceverified that executables.allow grants are version-blind and corrected ADR-0019, gates.md and apm.yml, but missed the gate script's own header and its operator-facing FAIL message, which still told the reader deployment was silently broken, and gates.md's hook summary, which still called it a silent-failure guard. All three now match. Also: README's offline guarantee carries the populated-apm_modules condition gates.md and AGENTS.md already state; the scripts/ layout row drops "sync" for the three deleted sync scripts; the check-rtk-prefix README rationale names the 12 subdirectory READMEs that survive rather than the skill-root ones this branch deleted; gates.md re-cites its three head -1 sites by enclosing function per its own :238 rule; and deploy-manifest drops a pointer to a provider-manifest.sh that has never existed on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwD8Egs5r4ndqeFLmhusX2
This commit is contained in:
@@ -57,8 +57,9 @@ Eight hooks, grouped below by what they guard rather than by the order `.pre-com
|
||||
| `check-scope-walkup-sync` | `validate.sh`, `validate-provenance.sh`, `new-agent.sh` and `new-skill.sh`'s four independent `$HOME`/`.git`/`apm.yml` walk-up ports still agree behaviorally |
|
||||
| `check-executables-allow-sync` | root `apm.yml`'s `executables.allow` key names kyberforge's actual version (see [apm gates](#apm-gates)) |
|
||||
|
||||
`check-executables-allow-sync` is the odd one in this group: it guards a *silent failure* rather than
|
||||
drift in generated text.
|
||||
`check-executables-allow-sync` is the odd one in this group: the drift it guards is in a
|
||||
hand-written key rather than in generated text, and it is a record-keeping gate — the grant itself is
|
||||
version-blind, so a stale key deploys fine (see [apm gates](#apm-gates)).
|
||||
|
||||
**Artifact validators**
|
||||
|
||||
@@ -105,7 +106,10 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu
|
||||
version at the tip of the same `main` ref (ADR-0022's second 2026-09-16 amendment). The tip
|
||||
check stops two branches that make the same bump (`1.0.0` → `1.0.1`) with different content from
|
||||
both landing, since the identical version lines merge without a conflict. A skill absent at the
|
||||
tip is held to the merge-base alone; when `main` has not moved, the two are the same commit. Each
|
||||
tip is held to the merge-base alone, and so is one whose directory at the pushed commit is the
|
||||
*same tree object* as at the tip: it ships exactly what main ships, whatever route the history
|
||||
took there — a criss-cross merge, a cherry-pick, a backport — so there is nothing for a bump to
|
||||
announce. When `main` has not moved, the two baselines are the same commit. Each
|
||||
failure line names the baseline it missed: `(not above merge-base)` or
|
||||
`(not above origin/main tip)`. The tip is `origin/main` as last fetched.
|
||||
- **It fails closed when it has no trustworthy baseline:** neither `origin/main` nor `main`
|
||||
@@ -634,8 +638,10 @@ boundary, and a stricter form would only move the same trust to a different stri
|
||||
- **Prose bullets.** Most of `branch-operations.md`, `merging.md` and `rewrite-history.md` instruct
|
||||
in list items, not fences. Those are clause-1 sites the gate cannot see, because it cannot
|
||||
distinguish them from clause-2 mentions in the same list.
|
||||
- **`README.md`, excluded by pattern.** A skill-directory README is consumer-facing prose no agent
|
||||
loads, and the `git clone https://github.com/bats-core/…` lines in the six `tests/README.md`
|
||||
- **`README.md`, excluded by pattern.** The skill-directory READMEs the exclusion was first written
|
||||
for are deleted; what it still covers is the 12 `README.md` files inside a skill's `scripts/`,
|
||||
`tests/` and `assets/` subdirectories — consumer-facing prose no agent loads — and the
|
||||
`git clone https://github.com/bats-core/…` lines in the six `tests/README.md`
|
||||
files are setup instructions for a third party who has no `rtk`. Prefixing those would be actively
|
||||
wrong, not merely noisy — see ADR-0023's consumer section.
|
||||
- **Quoting.** The line splitter breaks on `;`, `|`, `&&`, `||`, `$(` and backticks without tracking
|
||||
@@ -939,8 +945,8 @@ pass, and a skip fails the push.
|
||||
|
||||
`test-vale-wrap.sh` without Vale skips only its Vale-dependent cases, not the whole suite. The cases
|
||||
that are plain greps and awk over the Vale config and `.pre-commit-config.yaml` still run: case 0, 16, 26,
|
||||
27, the static half of 28, 31 Parts A and B, 32 and 34. A static failure exits 1, because a
|
||||
real defect is not a setup error. Only an all-static-pass run exits 77.
|
||||
27, the static half of 28, 31 Parts A and B, 32, 34 and the static half of 35. A static failure
|
||||
exits 1, because a real defect is not a setup error. Only an all-static-pass run exits 77.
|
||||
|
||||
### Mentioning banned phrasing without tripping the rule
|
||||
|
||||
@@ -1010,9 +1016,10 @@ near-miss negatives it must leave alone, and the suite from 5 cases to **7**.
|
||||
an `echo` or `printf` feeding any of them is the same race. Those are guarded by **convention** —
|
||||
absorb the writer's status with `|| true`, or take the verdict from a here-string — and deliberately
|
||||
not by this test: most legitimate uses of them in this tree are already absorbed, and the scanner
|
||||
cannot see absorption from the pipeline text alone, so flagging them would be noise. Two live
|
||||
`grep … | head -1` sites (`tests/test-vale-wrap.sh:620` and `:1046`) were fixed by hand with that
|
||||
idiom. Pipes from a non-builtin writer (`run_wrap … | grep -q`) are out of scope for the same reason:
|
||||
cannot see absorption from the pipeline text alone, so flagging them would be noise. The three live
|
||||
`grep … | head -1` sites in `tests/test-vale-wrap.sh` — in `unguarded_expansions()`, in case 20B's
|
||||
`--output line` line-number read, and in case 28's per-file `RESULTS28` lookup — carry that idiom by
|
||||
hand. Pipes from a non-builtin writer (`run_wrap … | grep -q`) are out of scope for the same reason:
|
||||
in practice they either absorb the writer's exit status with `|| true` or write only once, at exit.
|
||||
|
||||
**Known limitation: heredoc bodies are scanned as code.** A `cat <<'EOF'` body containing a
|
||||
|
||||
Reference in New Issue
Block a user