fix(ci): run the Python suites; harden the release baseline capture

[jbaruch]

Author-Model: claude-opus-4-8

Summary

Two independent ways the CI and release gates reported success without checking anything. Found by an audit of the repo against its own rules.

1. Thirty-two tests have never run in CI

scripts/run-tests.sh discovered test_*.sh only. These were orphaned:

suite	tests
skills/eval-curation/tests/test_compute_lift.py	22
skills/release/tests/test_stamp_changelog.py	10

Both pass when run by hand — which is precisely what made this dangerous. compute-lift.py and stamp-changelog.py looked covered while being untested in CI, and stamp-changelog.py is wired into the publish path (publish.yml:57). rules/testing-standards.md:21 requires tests "that run in CI"; these didn't.

Discovery now matches test_*.{sh,py} and dispatches by extension. Both suite kinds self-drive via the entry-point guard rules/file-hygiene.md requires, so the runner picks the interpreter and reads the exit code — no framework glue. Suite count 17 → 19.

2. The release baseline was a pasted pipeline with none of its own hardening

SKILL.md:130 had the operator paste:

PRE=$(tessl plugin info <w>/<p> | grep "Latest Version" | awk '{print $NF}')

verify-publish-landed.sh:151 performs the identical parse, but guarded: stderr separated so a tessl warning can't poison it, plus an explicit parse-miss diagnostic. The pasted copy had neither, violating rules/script-delegation.md:36-38 (deterministic work belongs in a script, not a code block for the agent to reproduce).

The empty-baseline case is the dangerous one. A parse miss silently yields an empty PRE, which is then passed to verify-publish-landed.sh as the baseline — and every version compares as "advanced past empty", so the release contract's conjunct 2 passes vacuously and an unpublished release reports as confirmed. A gate that cannot fail.

capture-registry-baseline.sh emits {"version": "<semver>"} or exits 2. It never emits an empty baseline.

Two bugs my own tests caught while writing them

Both in capture-registry-baseline.sh, both worth recording:

• err_file as a main() local. The EXIT trap fires after main() returns on the happy path, so under set -u the cleanup died on an unbound variable and reddened a successful run. verify-publish-landed.sh survives the same pattern only incidentally — every one of its paths exits from inside main(). Now script-global.
• The EXIT trap's final status leaked into the exit code. A bare [[ -n "$ERR_FILE" ]] && rm -f ... guard returns 1 when the path was never set, silently rewriting a deliberate exit 2 into exit 1. Cleanup now ends return 0.

Test plan

• t_python-only tree -> discovered — fails pre-fix with code=2, "no test suites found": a tree of passing Python tests read as empty.
• t_mixed sh+py tree — fails pre-fix with suites:1; the .py was silently dropped.
• t_failing python suite -> exit 1 — discovery alone is insufficient; a dropped exit code would count a red suite as passed.
• capture-registry-baseline.sh: 5/5 — happy path, stderr-warning-does-not-poison-parse, parse-miss → exit 2 with no empty stdout, tessl failure, wrong arity.
• Full suite: {"suites":20,"passed":20,"failed":0}. tessl plugin lint valid. shellcheck clean on all four files.
• Paired cross-family policy review + Copilot

Not in scope

rules/error-handling.md is entirely silent on shell — no set -euo pipefail, no suppression ban. That discipline currently lives only in the maintainer's private config, so consumers of the published plugin inherit none of it. Fixing it is a rule addition plus a ~35-site refactor (|| true / 2>/dev/null across verify-moderation-cleared.sh, scaffold.sh, preflight.sh, adopt.sh, migrate.sh, and most test harnesses), since rules/context-artifacts.md Post-Edit Rule Audit forbids landing a rule the repo doesn't already satisfy. Separate PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

[Copilot]

Pull request overview

This PR closes two CI/release safety gaps: (1) Python test suites were present but never executed by CI due to shell-only discovery, and (2) the release “registry baseline” capture relied on an unguarded pasted pipeline that could silently produce an empty baseline.

Changes:

• Extend scripts/run-tests.sh to discover test_*.{sh,py} suites and dispatch by file extension; expand runner tests to cover Python discovery/exit-code propagation.
• Introduce skills/release/capture-registry-baseline.sh and a dedicated test to ensure baseline capture is hardened (no empty baseline; exit 2 with diagnostics on failures).
• Update skills/release/SKILL.md and CHANGELOG.md to reflect the hardened baseline capture and the CI discovery fix.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
skills/release/tests/test_capture_registry_baseline.sh	Adds end-to-end tests for the hardened baseline capture behavior via a stubbed tessl.
skills/release/SKILL.md	Replaces pasted `tessl
skills/release/capture-registry-baseline.sh	New script to deterministically capture a non-empty registry version baseline or fail loudly.
scripts/tests/test_run_tests.sh	Extends runner contract tests to include Python suite discovery and failure propagation.
scripts/run-tests.sh	Expands discovery to .py and adds dispatch by extension.
CHANGELOG.md	Documents the CI Python-suite coverage fix and the baseline-capture hardening.

Comments suppressed due to low confidence (1)

scripts/run-tests.sh:132

• run_suite() can return 2 for an unrecognized suite extension, but the loop treats any non-zero as a test failure and the script ultimately returns 1 (via [[ ${#failed[@]} -eq 0 ]]). This contradicts the documented contract/comment that dispatch mismatches are setup errors (exit 2) and should produce the setup-error JSON shape with an error field.

  local failed=()
  for s in "${suites[@]}"; do
    echo "▶ $s" >&2
    if run_suite "$s" >&2; then
      echo "  ✓ $s" >&2
    else
      echo "  ✗ $s" >&2
      failed+=("$s")
    fi

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jbaruch]

Fixed in ca0073f — correct, and thank you: the header described a sourced-function harness while the test actually runs the script as a subprocess against a stub tessl on PATH. I wrote the header before switching approaches and it never caught up.

Rewritten to describe the subprocess form and to say why it's the right one: the sourced approximation would have missed both defects this script hit in development — an EXIT trap firing after main() returns (unbound local under set -u) and the trap's final status leaking into the exit code.

(Apologies for the noise — this thread briefly carried a reply meant for the gh-aw thread.)

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for one release-gate violation: the new Step 7 baseline-capture wrapper can ignore capture-registry-baseline.sh failure because the pipeline status comes from jq unless pipefail is set, allowing an empty PRE after a failed pre-merge capture. That violates rules/ci-safety.md's requirement to capture the registry baseline before merge and contradicts the new script contract that a capture failure stops the release.

Generated by PR Policy Review (OpenAI) for #183 · 27.1 AIC · ⌖ 9.32 AIC · ⊞ 14.6K

[github-actions]

This wrapper can still proceed with an empty PRE when capture-registry-baseline.sh fails. In bash, without set -o pipefail, the assignment's pipeline status is jq's status; jq -r '.version' on empty stdin exits successfully with no output, so the || { ... } branch is skipped and Step 7 can merge without the required pre-merge baseline.

This violates rules/ci-safety.md: "Before merge: capture the registry's Latest Version as baseline" and "If any conjunct fails, the publish is not confirmed". It also contradicts this new paragraph's own contract that a capture failure "stops the release".

Split capture from parsing so the script's exit code gates before jq, e.g.:

baseline_json=$(skills/release/capture-registry-baseline.sh <workspace> <plugin>) \
  || { echo "Baseline capture failed — see stderr" >&2; exit 1; }
PRE=$(jq -r '.version' <<<"$baseline_json") \
  || { echo "Baseline JSON parse failed" >&2; exit 1; }
[[ -n "$PRE" ]] || { echo "Baseline capture returned an empty version" >&2; exit 1; }

[jbaruch]

Fixed in ca0073f — you're right, and it's worse than a near-miss: the wrapper reintroduced the exact bug this PR exists to remove.

Verified empirically before fixing. A stub exiting 2 with empty stdout produced:

GUARD SKIPPED — PRE='' (empty baseline flows to conjunct 2)

Exactly your reasoning: jq -r exits 0 on empty stdin, the pipeline status is jq's, the || never fires. I wrote a script whose entire contract is "never emit an empty baseline" and then wrapped it in a pipeline that manufactures one from its failure.

Took your suggested shape (capture → parse → assert non-empty). Also applied it to resolve-publish-run.sh at SKILL.md:186 — same pipeline three lines away, with no || guard at all, so a failed resolve yielded an empty run_id and gh run watch "", replacing the resolver's actionable diagnostic with a confusing one. Lines 200–201 already used the herestring form and were correct.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

scripts/run-tests.sh:131

• run_suite documents/returns 2 for a suite dispatch mismatch, but main() currently treats any non-zero from run_suite as a test failure (adds to failed[] and ultimately exits 1). That means a dispatch/setup error would be misclassified as a test failure and would not produce the exit-2 + error JSON contract described earlier in the script.

    if run_suite "$s" >&2; then
      echo "  ✓ $s" >&2
    else
      echo "  ✗ $s" >&2
      failed+=("$s")

[jbaruch]

Fixed in e647bea — correct. jq was unguarded, so a jq failure (missing binary, malformed resolver output) fell through to [[ -n "$run_id" ]] and reported "Resolver returned no run id" — blaming the resolver for jq's failure and sending the operator to debug the wrong script. Now guarded with its own diagnostic before the emptiness check.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for two policy violations: the new release baseline script does not enforce the semver/tool-error contract it documents under rules/script-delegation.md, and the new changelog archive entries appear to point at the wrong PR number under rules/context-artifacts.md changelog hygiene. I left inline comments with the concrete fixes.

Generated by PR Policy Review (OpenAI) for #183 · 18.3 AIC · ⊞ 43.4K

[github-actions]

rules/script-delegation.md requires deterministic scripts to be self-error-handling: "exit non-zero on failure, write a diagnostic message to stderr." This script's own contract says stdout is {"version": "<semver>"} and unparseable registry output exits 2, but the implementation treats any non-empty last field on a Latest Version line as success. A changed registry shape such as Latest Version unavailable would emit {"version":"unavailable"} and feed a malformed baseline into verify-publish-landed.sh instead of stopping at the baseline capture. Please validate the parsed token against the semver shape this script promises before printing it, and add a malformed-Latest Version fixture that exits 2 with empty stdout.

[jbaruch]

Fixed in e647bea — this is the sharpest catch on the PR, because it's the same defect the PR removes, one layer up: a contract that promises a shape and ships whatever it found. awk '{print $NF}' on Latest Version unavailable yields unavailable, which is non-empty, so my only guard passed it straight through as a baseline.

The consequence is worse than a bad value — verify-publish-landed.sh compares baselines with version_gt, so an unorderable token turns a registry-side outage into a bogus conjunct-2 verdict rather than an error anyone sees.

Now validated against the semver shape before printing. Three fixtures, per your suggestion and one beyond it:

• Latest Version unavailable → exit 2, empty stdout
• Latest Version see registry status page → exit 2 (prose is the nastier case: $NF reduces it to page, a plausible-looking single token that only a shape check catches)
• 1.2.3-rc.1+build.5 → accepted, so the check doesn't reject versions the registry legitimately serves

[jbaruch]

Fixed in 4835799 — and this is the best catch on the PR, because the bug is one I introduced while fixing its sibling. I widened the contract past what the system can consume, and the test I added blessing 1.2.3-rc.1+build.5 is what created it.

Verified, and the real failure is worse than a syntax error:

version_gt "1.2.4" "1.2.3-rc.1+build.5"
((: 3-rc.1+build.5: arithmetic syntax error
returned FALSE (rc=1)

It doesn't raise. The arithmetic error makes (( a3 > b3 )) return non-zero, so version_gt falls through to return 1 — "not greater". verify-publish-landed.sh then reports "registry's Latest Version is not greater than baseline — investigate the registry state" and exits 1. A landed publish reported as failed, sending the operator after a registry that did nothing wrong.

Took your first option — restrict to numeric major.minor.patch. Extending version_gt to full semver precedence (prerelease < release, build ignored) is a real change with its own tests and doesn't belong in a PR about gate fidelity. The script now says so explicitly: the accepted shape and the comparator move together or not at all.

Test flipped from prerelease+build accepted to prerelease+build -> exit 2, with the reason recorded so nobody "fixes" it back.

[github-actions]

rules/context-artifacts.md says CHANGELOG entries are the archive and should "retain load-bearing facts" while the PR body/commits carry the broader motivation. These two new entries both point readers to PR #181, but this review is running on PR #183. If #181 is not intentionally the authoritative archive for these exact changes, update the reference to PR #183 (or remove the PR-specific pointer until the correct archive reference is known) so the changelog does not send future readers to the wrong context.

[jbaruch]

Fixed in e647bea — both entries now point at #183. I wrote #181 by guessing the PR number before the PR existed, which is exactly the kind of pointer that rots silently: the number is plausible, the reference resolves to a real PR, and nothing catches it except a reader following the link and landing somewhere unrelated. Good catch.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

scripts/run-tests.sh:131

• The runner’s documented exit semantics say setup/dispatch errors should return 2, but the loop treats any non-zero suite exit the same way (adds it to failed and continues), which forces the overall exit code to 1. This breaks the contract for suite-dispatch mismatches (e.g., run_suite returning 2) and can mask infrastructure problems as test failures.

    if run_suite "$s" >&2; then
      echo "  ✓ $s" >&2
    else
      echo "  ✗ $s" >&2
      failed+=("$s")

[jbaruch]

Fixed in 4835799 — correct, and the fix needed two halves, not one. Adding a command -v check to run_suite wasn't enough: the caller's if run_suite ...; then / else folded any non-zero into failed, so rc 2 still surfaced as "1 suite failed" — red CI pointing at a test that never executed, on a runner that simply lacks python3.

Both halves now: run_suite checks the interpreter and returns 2, and the loop branches on rc 2 to emit the contract's setup error (suites:0, error field, rc 2) rather than counting it as a verdict.

Interpreters are also overridable via SH_BIN/PY_BIN — it makes the missing-interpreter path testable without emptying PATH (which breaks the harness's own bash), and incidentally helps runners that only ship python3.11. Regression test asserts rc 2 and suites:0, not rc 1.

[jbaruch]

Fixed in 935df19 — you're right, and the collision was mine. run_suite returned 2 for its own setup errors, but the harnesses in this repo exit 2 on a fatal precondition (fatal: <script> not executable), so a genuinely failing suite read as a broken dispatcher, reported suites=0, and stopped the run. A red suite hidden behind a setup error nobody could act on — the same vacuous-signal class this PR exists to remove.

Took your suggestion: DISPATCH_FAULT=125 as the private sentinel (outside the range any suite here produces; git-bisect uses 125 for the same "the launcher failed, not the thing it launched" distinction), mapped to the documented runner exit 2. Every other non-zero, 2 included, is the suite's own verdict.

Two regression tests as asked — a shell suite exiting 2 and a python suite exiting 2 — both asserting exit 1 / failed=1, not exit 2.

[jbaruch]

Fixed in 4835799 — correct, and the inconsistency is the tell: I guarded the run-id snippet against jq's literal "null" four lines below and didn't carry it back up to the baseline. jq -r '.version' on a JSON object with no version field prints null, which is non-empty, so [[ -n "$PRE" ]] passed it straight through as a baseline string.

Now [[ -n "$PRE" && "$PRE" != "null" ]], matching the run-id guard.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for one release-contract inconsistency: the new baseline capture script now accepts prerelease/build semver values, but the existing publish verification comparator only supports numeric major.minor.patch; that means a baseline the new script can emit may make the post-merge release gate fail with shell arithmetic rather than a deterministic verdict. Align the accepted baseline shape with the verifier, or extend the verifier and tests to handle full semver.

Generated by PR Policy Review (OpenAI) for #183 · 51.9 AIC · ⌖ 10.5 AIC · ⊞ 14.6K

[github-actions]

rules/ci-safety.md requires the release contract to confirm that after merge the registry Latest Version advanced past the baseline. rules/script-delegation.md also requires scripts to be self-error-handling: "exit non-zero on failure, write a diagnostic message to stderr." This regex accepts prerelease/build baselines, and the new test blesses 1.2.3-rc.1+build.5, but skills/release/verify-publish-landed.sh compares versions by splitting major.minor.patch and using bash arithmetic on the patch field. A baseline like 1.2.3-rc.1+build.5 reaches arithmetic with a value like 3-rc.1+build.5, producing a syntax/runtime failure instead of the release verdict. Either restrict this script to the numeric x.y.z shape the verifier can compare, or update verify-publish-landed.sh plus its tests to compare full semver before accepting prerelease/build here.

[jbaruch]

Already fixed in 4835799, before this re-review ran — the regex is now ^[0-9]+\.[0-9]+\.[0-9]+$ and the test asserts 1.2.3-rc.1+build.5 → exit 2 rather than blessing it. Full reasoning on the thread at r3598907217.

Restating the confirmation for the record, since it's the sharpest finding on the PR: I verified version_gt "1.2.4" "1.2.3-rc.1+build.5" and it does not raise — the arithmetic error makes (( a3 > b3 )) return non-zero, so the function falls through to return 1 ("not greater") and a landed publish reports as "Latest Version is not greater than baseline — investigate the registry state". A false failure pointing at an innocent registry, not a crash.

[jbaruch]

Fixed in ae7126f — and grepping for the rest of it found a fourth.

You caught the script's own header still advertising {"version": "<semver>"} after the implementation narrowed to numeric major.minor.patch. When I narrowed the contract I fixed the one place you'd pointed at (SKILL.md) and never grepped for the others — so the same claim stayed wrong in the script header, and in the CHANGELOG entry too.

A contract stated in four places is four things to keep true, and "fix where you're told" leaves three lying. All four now agree: script header, SKILL.md, tests, CHANGELOG. Verified with grep -rn '"<semver>"' returning nothing.

(The regex itself was already fixed in 4835799 — thread r3598907217 has the verification that version_gt returns "not greater" rather than raising on a prerelease baseline.)

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

[jbaruch]

Declining — rules/context-artifacts.md CHANGELOG Hygiene prescribes exactly this shape:

• With a wired stamp step: authors add un-headed ### entry blocks at the top of CHANGELOG.md
• The stamp step writes the ## <version> — <date> heading above those blocks before publish

The stamp step is wired (.github/actions/stamp-changelog, invoked before tesslio/patch-version-publish), so an un-headed ### Skills at the top is the correct authored state — the heading is the publish step's job, not mine. Writing ## 0.3.93 — <date> by hand is what the rule forbids here; it also guesses a version number the bump step owns.

You can see the mechanism in this file's own history: ## 0.3.92 — 2026-07-17 below my block is what the stamp produced for #179's un-headed entries an hour ago.

[jbaruch]

Fixed in 935df19 — you're right and I undercounted twice over. It's 20 (18 shell + 2 Python), not 19: I forgot this PR adds a suite of its own (test_capture_registry_baseline.sh). Verified against the runner rather than by hand this time — {"suites":20,"passed":20,"failed":0}.

Entry now reads "Suite count 17 → 20 (18 shell + 2 Python, including this PR's new baseline suite)".

[jbaruch]

Fixed in 935df19 — good catch, and the reason it bites here specifically is that the harness deliberately runs without set -e (a failure-counting harness must not abort on its first red assertion), so nothing was catching the mktemp -d. STUBDIR would be empty, "" goes on PATH, and every subsequent assertion fails in a way that looks like the script's fault.

Now fails fast with an explicit diagnostic, plus a [[ -n && -d ]] guard for a mktemp that succeeds but returns nothing usable.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for two concrete policy issues: the test runner currently conflates a suite’s own exit 2 with dispatcher setup errors, violating the meaningful-exit-code contract in rules/file-hygiene.md, and the release skill documents the new baseline script as emitting <semver> even though the implementation intentionally accepts only numeric major.minor.patch, violating the script-contract accuracy required by rules/script-as-black-box.md.

One off-diff follow-up to include while touching this area: .github/workflows/tests.yml still describes scripts/run-tests.sh as running “every module’s bash unit suite”; after this PR it should say shell and Python suites.

Generated by PR Policy Review (OpenAI) for #183 · 68.4 AIC · ⌖ 3.48 AIC · ⊞ 14.6K

[github-actions]

rules/file-hygiene.md says scripts should use meaningful exit codes: “Exit 0 on success, non-zero on failure” and “Use meaningful exit codes when the platform supports them.” Here run_suite returns the child suite’s exit code unchanged, so a real suite that exits 2 is indistinguishable from the dispatcher’s own setup errors and gets reported as suites=0/setup failure instead of a failed suite. Use a private dispatcher sentinel (for example return 125 for missing interpreter/unknown extension, then map that to runner exit 2) and treat every actual suite non-zero, including 2, as a test failure; add a regression test for a suite that exits 2.

[jbaruch]

Fixed in 935df19 — you're right, and the collision was mine. run_suite returned 2 for its own setup errors, but the harnesses in this repo exit 2 on a fatal precondition (fatal: <script> not executable), so a genuinely failing suite read as a broken dispatcher, reported suites=0, and stopped the run. A red suite hidden behind a setup error nobody could act on — the same vacuous-signal class this PR exists to remove.

Took your suggestion: DISPATCH_FAULT=125 as the private sentinel (outside the range any suite here produces; git-bisect uses 125 for the same "the launcher failed, not the thing it launched" distinction), mapped to the documented runner exit 2. Every other non-zero, 2 included, is the suite's own verdict.

Two regression tests as asked — a shell suite exiting 2 and a python suite exiting 2 — both asserting exit 1 / failed=1, not exit 2.

[github-actions]

rules/script-as-black-box.md requires skill prose to name the script’s contract: “Skill prose names the script’s contract — required inputs, output shape, exit codes...” This contract says the script emits <semver>, but the new script intentionally rejects semver prerelease/build metadata and only accepts numeric major.minor.patch. Make the SKILL contract say numeric major.minor.patch (or widen verify-publish-landed.sh/capture-registry-baseline.sh together before documenting full semver).

[jbaruch]

Fixed in 935df19 — correct, and it's a clean script-as-black-box catch: I narrowed the script to numeric major.minor.patch in response to your earlier finding and left the SKILL prose still promising <semver>. The contract and the implementation disagreed in the direction that matters — prose promising more than the script accepts.

SKILL.md now reads {"version": "<major.minor.patch>"} and says why the narrowing exists: verify-publish-landed.sh's comparator orders baselines with bash arithmetic, so a prerelease value silently returns "not greater".

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for a script-contract inconsistency under rules/script-delegation.md: the new baseline script documents a semver output shape while the executable contract and release skill intentionally allow only numeric major.minor.patch. Please align the script header, and also update the matching CHANGELOG wording so the published archive does not repeat the stale shape.

Generated by PR Policy Review (OpenAI) for #183 · 28.9 AIC · ⌖ 10.4 AIC · ⊞ 28.1K

[github-actions]

rules/script-delegation.md requires script-facing prose to carry the real contract: "Skill prose names the script's contract — required inputs, output shape, exit codes" and script requirements include "JSON-producing: output structured data, not prose." This header advertises {"version": "<semver>"}, but the implementation rejects semver prerelease/build metadata and only accepts numeric major.minor.patch (matching skills/release/SKILL.md). Please change the output contract here to {"version": "<major.minor.patch>"} and keep any published prose that mentions the emitted shape in sync.

[jbaruch]

Fixed in ae7126f — and grepping for the rest of it found a fourth.

You caught the script's own header still advertising {"version": "<semver>"} after the implementation narrowed to numeric major.minor.patch. When I narrowed the contract I fixed the one place you'd pointed at (SKILL.md) and never grepped for the others — so the same claim stayed wrong in the script header, and in the CHANGELOG entry too.

A contract stated in four places is four things to keep true, and "fix where you're told" leaves three lying. All four now agree: script header, SKILL.md, tests, CHANGELOG. Verified with grep -rn '"<semver>"' returning nothing.

(The regex itself was already fixed in 4835799 — thread r3598907217 has the verification that version_gt returns "not greater" rather than raising on a prerelease baseline.)

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[jbaruch]

Fixed in ae7126f (before this review ran) — the CHANGELOG entry now says {"version": "<major.minor.patch>"}. You and gh-aw both caught the <semver> drift; grepping for the rest turned up four places stating that contract (script header, SKILL.md, tests, CHANGELOG), of which I'd fixed only the one a reviewer had pointed at. All four agree now.

[jbaruch]

Fixed in ae7126f (before this review ran) — the header now documents numeric major.minor.patch and says why the shape is narrower than semver: verify-publish-landed.sh's comparator orders baselines with bash arithmetic, so a prerelease value reaches (( )) as 3-rc.1+build.5 and the resulting error makes the comparison return "not greater" — a landed publish reported as failed.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes because the PR edits skills/release/SKILL.md, which is governed by rules/script-as-black-box.md; the new prose restates script internals and rationale that should remain in the script header/CHANGELOG while the skill carries only the command contract and reference. I did not find additional blocking issues in the changed shell scripts or tests by policy review.

Generated by PR Policy Review (OpenAI) for #183 · 45.4 AIC · ⌖ 7.12 AIC · ⊞ 15.5K

[github-actions]

rules/script-as-black-box.md says: “Skill prose does not restate the script's internal logic — thresholds, predicates, formulas, allowlists, source lists, filter rules” and “Rationale paragraphs explaining why a constant is set at its current value — that belongs in the script's docstring or the CHANGELOG, not the skill.”

This added paragraph explains the comparator implementation and why the accepted version shape is narrower than semver. That rationale is already in skills/release/capture-registry-baseline.sh; keep the skill to the executable contract, for example: Run skills/release/capture-registry-baseline.sh <workspace> <plugin>. Exit 0 emits {"version":"<major.minor.patch>"}; exit 2 means no usable baseline was captured. See the script header for accepted-version details.

[jbaruch]

Fixed in a5aa46e — same clause, the baseline paragraph. The comparator rationale (why the accepted shape is narrower than semver) is script-header content; the header already carried it in full. Skill now names the shape and exit codes and points there.

[github-actions]

rules/script-as-black-box.md says: “Skill prose does not restate the script's internal logic” and “Name the file and an anchor inside it — top-of-file constants, named frozenset, docstring, function name.”

This new paragraph explains the failure mechanics of piping the resolver into jq. The skill only needs to prescribe the safe calling pattern and point to skills/release/resolve-publish-run.sh for the contract. Drop the rationale sentence or move it to the script header/CHANGELOG, leaving the capture-then-parse snippet intact.

[jbaruch]

Fixed in bca5fc6 — correct. The paragraph explained the mechanism (why piping the resolver into jq swallows its exit code) when script-as-black-box says the skill prescribes the calling pattern and points at the script. Now: "Capture, then parse — never one pipeline (rules/error-handling.md Shell Error Handling)" plus the snippet.

Then swept the section instead of the line you named, and found two pre-existing violations of the same clause in the paragraphs I was already editing: (2s interval, 30s budget) restates script-owned constants, and filters on \headSha == $merge_sha` AND `event == push`restates the resolver's predicate. Both now point at its docstring.rules/script-as-black-box.mdnamesresolve-publish-run.sh` as its example of how to reference a script — and the skill was restating that very script's internals three lines from the pointer.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[jbaruch]

Fixed in 62cbfba — and this one stings: it's the same defect this PR fixes in capture-registry-baseline.sh, in the harness I wrote to test that fix.

You're right about the mechanism, including the part I'd have gotten wrong: set -e is deliberately off in a harness, so a failing rm -rf can't abort the handler — but the trap's final command status still becomes the process's exit status, so an all-green run exits non-zero and flakes CI. The abort and the rewrite are separate failure modes and only one of them depends on set -e.

Named handler ending return 0, matching the production scripts.

Swept the rest rather than fixing only this one: six more harnesses carry the same bare trap. They violate the rule #184 adds ("end cleanup handlers with return 0") and that PR's own Post-Edit Rule Audit missed them — fixed there, in f901b2d. No bare EXIT traps remain in skills/ or scripts/.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for one release-flow consistency violation: the PR correctly moves baseline capture into skills/release/capture-registry-baseline.sh, but skills/release/verify-publish-landed.sh:107 still tells operators who pass an empty baseline to capture with the old inline tessl plugin info | grep "Latest Version" | awk ... pipeline. That reintroduces the exact unguarded deterministic parse this PR removes from SKILL.md. This violates rules/script-delegation.md ("Scripts are executable files that live in the plugin ... not code blocks ... The skill references the script and runs it; the script does the work") and leaves an actionable error message pointing at the unsafe path. Update that diagnostic to point at skills/release/capture-registry-baseline.sh <workspace> <plugin> and the .version extraction contract instead. I did not post this inline because skills/release/verify-publish-landed.sh is not in CHANGED_FILES for this PR.

Generated by PR Policy Review (OpenAI) for #183 · 86.3 AIC · ⌖ 2.34 AIC · ⊞ 18.6K

[jbaruch]

Fixed in 2cf70f3 — legitimate find, and not a nit. verify-publish-landed.sh:107's empty-baseline error still told operators to capture with the raw tessl plugin info | grep | awk pipeline — the exact unguarded parse this PR extracts into capture-registry-baseline.sh. A diagnostic pointing at the unsafe path the PR exists to remove.

Now points at skills/release/capture-registry-baseline.sh <workspace> <plugin> and the .version contract. Swept the repo: no other live inline-pipeline references (the one remaining is in the script's own header, documenting the pattern it replaces).

Noted that verify-publish-landed.sh isn't in this PR's changed-files — but the finding is squarely this PR's responsibility, since it's the PR that makes the inline pipeline the wrong path. Thanks for flagging it in the review body rather than skipping it for being out-of-diff.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

The release baseline change is valid in the touched SKILL.md/script path, but the PR leaves other policy surfaces still teaching or accepting the unsafe baseline capture shape this PR is removing. Under rules/plugin-evals.md Fixture Hygiene and rules/script-delegation.md Scripts Are Real Files, those surfaces need to move with the new guarded script rather than continuing to reward/recommend raw tessl ... | grep | awk parsing.

Findings:

• evals/pr-merge-and-post-merge-cleanup/criteria.json:48 still gives the baseline-capture criterion an example of PRE=$(tessl tile info <workspace>/<tile> | grep 'Latest Version' | awk '{print $NF}'). This violates rules/plugin-evals.md Fixture Hygiene: "Update fixtures when the skill's contract changes — stale fixtures produce false passes." With this PR, that exact pattern is now the failure mode extracted into skills/release/capture-registry-baseline.sh, so the eval can falsely pass a generated merge wrapper that reproduces the unsafe parse. Update the criterion to require/credit the guarded baseline script or an equivalently hardened deterministic script, and stop using the raw pipeline as the exemplar.

• skills/release/SCRIPTING.md:25 still tells unattended merge wrappers to capture the Tessl baseline via tessl plugin info <workspace>/<plugin> instead of routing through the new script. This contradicts rules/script-delegation.md Scripts Are Real Files: "The skill references the script and runs it; the script does the work." This reference is the wrapper-authoring contract, so leaving it generic invites exactly the pasted parse the PR removes from SKILL.md. Point wrapper authors at skills/release/capture-registry-baseline.sh <workspace> <plugin> and its JSON/exit-code contract.

Generated by PR Policy Review (OpenAI) for #183 · 95.2 AIC · ⊞ 14.6K

[jbaruch]

Fixed in 0a5ee8a (review-body finding, no inline). You're right that other surfaces still taught the shape this PR removes.

The real one was the pr-merge eval criterion Pre-merge registry baseline captured, whose description gave tessl tile info | grep | awk as the "e.g." — literally scoring an agent for reproducing the fragile parse this PR extracts into a guarded script. Fixed by making the criterion behavior-focused: capturing the pre-merge baseline is graded, the mechanism is not. That also removes a latent plugin-evals No-Leaking issue (a criterion shouldn't reference a plugin-internal implementation shape). max_score still sums to 100.

I left evals/migrate-to-plugin-keeps-contracts/task.md's pipeline as-is, and want to flag the reasoning explicitly in case you'd push back: that snippet is legacy fixture content in a MIGRATION eval — it depicts a repo being migrated, i.e. real-world code the agent must handle (Fixture Hygiene), not a recommendation the plugin makes. It's a different eval about a different skill, out of this PR's baseline-capture scope. If you read it as still teaching the shape, say so and I'll reconsider — but a migration fixture showing a legacy script seems correct.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes: the PR fixes the release baseline path in the skill and scripts, but the corresponding eval criterion still says the capture mechanism is not scored. Under rules/plugin-evals.md, criteria should grade the specific manner the plugin prescribes; as written, the eval can still reward the raw hand-parse shape this PR is trying to retire.

Generated by PR Policy Review (OpenAI) for #183 · 63.4 AIC · ⌖ 1.6 AIC · ⊞ 14.6K

[github-actions]

rules/plugin-evals.md says: “Criteria grade whether the output matches the specific manner this plugin prescribes.” This changed criterion explicitly says “The capture mechanism is not scored,” so a script can still get full credit for hand-parsing the registry instead of following the release skill’s guarded baseline-capture path. That keeps rewarding the unsafe shape this PR is trying to remove. Please make the criterion score the prescribed manner: capture before merge via the guarded helper or, at minimum, capture stdout first, parse second, and reject the raw tessl ... | grep | awk pipeline shape.

[jbaruch]

Fixed in 9c60764 — you're right, I over-corrected. Removing the raw-pipeline example, I made the criterion mechanism-neutral ("capture mechanism is not scored") — but plugin-evals says criteria grade "the specific manner this plugin prescribes". Neutral means it can still reward the raw hand-parse. The criterion now grades the prescribed manner: capture via the skill's guarded baseline-capture step, not tessl plugin info | grep | awk; a raw pipeline that can silently yield an empty baseline scores partial at most. This grades application, not a leaked plugin-internal path — it names the behavior. max_score still sums to 100.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for one discovery-contract bug in scripts/run-tests.sh: the PR narrows the documented suite shape to direct **/tests/test_*.{sh,py} files, but the new find -path pattern can still match nested helpers under tests/test_* directories because find -path wildcards can span /. That conflicts with the newly added comment/contract and can make CI execute helper or fixture files as suites. The fix should enforce direct children of a tests directory and add a regression for a nested tests/test_fixtures/helper.{sh,py} path.

Generated by PR Policy Review (OpenAI) for #183 · 49 AIC · ⌖ 5.94 AIC · ⊞ 42.2K

[github-actions]

rules/boy-scout.md says: "Applies to anything you'd flag if a colleague had just written it: ... stale comments, lying type signatures..." The new comment and diagnostics claim direct suites under **/tests/test_*.{sh,py}, but GNU find -path wildcards do not treat / specially, so */tests/test_*.sh can still match a nested helper such as .../tests/test_fixtures/helper.sh and dispatch it as a suite. That reintroduces the over-broad discovery case this block says it avoids. Please make the matcher enforce dirname(file) ending in /tests (or an equivalent direct-child check) and add a regression fixture proving tests/test_fixtures/helper.{sh,py} is not counted.

[jbaruch]

Fixed in 06dcfb5 — exactly right, and proven. find's * matches /, so -path '*/tests/test_*.sh' still matches tests/test_fixtures/helper.sh. My earlier narrowing (from -path '*/tests/*' -name) only fixed one level. The direct-child requirement isn't portably expressible in -path, so the collection loop now enforces dirname == */tests. Regression test added: a nested test_fixtures/test_helper (.sh and .py) — it fails against the pre-fix code (suites=3, two spurious failures) and passes after; real discovery still finds all 20.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).

All rules pass — no violations found.

Reviewed the changed allowlist: CHANGELOG.md, evals/pr-merge-and-post-merge-cleanup/criteria.json, scripts/run-tests.sh, scripts/tests/test_run_tests.sh, skills/release/SKILL.md, skills/release/capture-registry-baseline.sh, skills/release/tests/test_capture_registry_baseline.sh, and skills/release/verify-publish-landed.sh. The release skill changes keep script-owned internals in script headers while exposing the script contracts, the eval criterion grades the plugin-prescribed behavior, and the new shell scripts/tests are consistent with the loaded rules.

Generated by PR Policy Review (OpenAI) for #183 · 56.2 AIC · ⌖ 10.2 AIC · ⊞ 15.5K

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

scripts/run-tests.sh:135

• The example path in this discovery comment is incorrect: -path '*/tests/test_*.sh' won’t match tests/test_fixtures/helper.sh because the leaf name isn’t test_*. The nested-case that actually matches is something like tests/test_fixtures/test_helper.sh (because * in -path can span /). Updating the example will keep the comment aligned with the behavior the subsequent dirname == */tests filter is guarding against.

  # `-path '*/tests/test_*.sh'` narrows to a tests/ dir, but `find`'s `*`
  # matches `/` too, so it still matches a NESTED file like
  # `tests/test_fixtures/helper.sh`. The direct-child requirement can't be
  # expressed portably in `-path`, so discovery casts wide here and the
  # collection loop below enforces `dirname == */tests`.