Repository navigation
fix(review-swarm): require the verdict marker as the transcript's final line #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fc5bb47
fix(review-swarm): require the verdict marker as the transcript's fin…
b8c771c
test(review-swarm): prove the gate can still fail before it judges an…
066e2de
fix(review-swarm): say plainly that step verification does not enforc…
8d03df4
fix(review-swarm): make self-test freshness reliable
f0e8e2a
fix(review-evidence): exercise negated timestamp comparisons
9258719
fix(review-swarm): address remaining maintainability findings
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| #!/usr/bin/env bash | ||
| # Regression test for the review-swarm gate's verdict path. | ||
| # | ||
| # Why this exists: the gate is three lines of shell deciding whether code | ||
| # merges. The failure mode that matters is not "the gate is red" -- a red gate | ||
| # announces itself. It is a gate that has quietly become incapable of being | ||
| # red, which announces nothing and is discovered only after something broken | ||
| # ships behind it. So this suite asserts BOTH directions: a genuine objection | ||
| # must fail, and a genuine pass must pass. A suite that only checked the happy | ||
| # path would itself be the vacuous green it is meant to prevent. | ||
| # | ||
| # Hermetic: `agent-relay` and `gh` are stubbed on PATH, so this runs offline | ||
| # and exercises the real swarm-post.sh / swarm-verdict.sh, not a paraphrase. | ||
| set -uo pipefail | ||
|
|
||
| script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | ||
| pass=0 | ||
| fail=0 | ||
|
|
||
| ok() { pass=$((pass+1)); printf ' ok %s\n' "$1"; } | ||
| notok(){ fail=$((fail+1)); printf ' FAIL %s\n expected: %s\n actual: %s\n' "$1" "$2" "$3"; } | ||
|
|
||
| expect_eq() { | ||
| local label=$1 want=$2 got=$3 | ||
| [ "$want" = "$got" ] && ok "$label" || notok "$label" "$want" "$got" | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Unit: verdict extraction from a transcript's final non-empty line. | ||
| # --------------------------------------------------------------------------- | ||
| # shellcheck source=swarm-verdict.sh | ||
| source "$script_dir/swarm-verdict.sh" | ||
|
|
||
| verdict_of() { | ||
| local tmp; tmp=$(mktemp) | ||
| printf '%s' "$1" > "$tmp" | ||
| swarm_transcript_verdict "$tmp" | ||
| rm -f "$tmp" | ||
| } | ||
|
|
||
| echo "== verdict extraction ==" | ||
| expect_eq "a bare REVIEW_FAILED is FAILED" \ | ||
| FAILED "$(verdict_of 'Findings: P1 leak. | ||
| REVIEW_FAILED')" | ||
|
|
||
| expect_eq "a bare REVIEW_PASSED is PASSED" \ | ||
| PASSED "$(verdict_of 'Looks good. | ||
| REVIEW_PASSED')" | ||
|
|
||
| expect_eq "trailing blank lines do not hide the marker" \ | ||
| FAILED "$(verdict_of 'REVIEW_FAILED | ||
|
|
||
| ')" | ||
|
|
||
| expect_eq "surrounding whitespace is trimmed" \ | ||
| PASSED "$(verdict_of ' REVIEW_PASSED ')" | ||
|
|
||
| # The fix is prompt-side: agents must not append a sign-off. This test pins | ||
| # the parser's correct refusal of trailing text; it does not prove agents obey | ||
| # the prompt in a live run (the failure observed on PR #240). | ||
| expect_eq "a marker followed by a sign-off is UNCLEAR (PR #240 bug)" \ | ||
| UNCLEAR "$(verdict_of 'REVIEW_PASSED | ||
|
|
||
| **Review completed:** 2026-09-09 08:45')" | ||
|
|
||
| # The safety property that must survive the #248 prompt change. If a lens | ||
| # genuinely objects, no amount of prompt wording may turn that into a pass. | ||
| expect_eq "REVIEW_FAILED is never upgraded by surrounding prose" \ | ||
| UNCLEAR "$(verdict_of 'REVIEW_FAILED | ||
| structure-only review.')" | ||
|
|
||
| expect_eq "an empty transcript is UNCLEAR, not PASSED" \ | ||
| UNCLEAR "$(verdict_of '')" | ||
|
|
||
| expect_eq "a transcript merely containing the word is UNCLEAR" \ | ||
| UNCLEAR "$(verdict_of 'I considered emitting REVIEW_PASSED but did not.')" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Unit: lens selection -- missing and stale transcripts must be fail-closed. | ||
| # --------------------------------------------------------------------------- | ||
| echo "== lens selection ==" | ||
| work=$(mktemp -d); trap 'rm -rf "$work"' EXIT | ||
| mkdir -p "$work/ops/reviews" | ||
|
|
||
| expect_eq "no reviews directory yields MISSING" \ | ||
| "MISSING " "$(swarm_lens_result "$work/nope" 246 structure "")" | ||
|
|
||
| expect_eq "an absent transcript yields MISSING" \ | ||
| "MISSING " "$(swarm_lens_result "$work/ops/reviews" 246 structure "")" | ||
|
|
||
| # Fixed timestamps exercise mtime-based freshness without a wall-clock race. | ||
| # They do not test invalidation of reviews by a Git force-push. | ||
| marker="$work/marker"; touch -t 202601010001 "$marker" | ||
| old="$work/ops/reviews/20260101-0000-pr246-structure.md" | ||
| printf 'REVIEW_PASSED\n' > "$old" | ||
| touch -t 202601010000 "$old" # older than the marker | ||
| expect_eq "a transcript predating the run yields STALE" \ | ||
| STALE "$(swarm_lens_result "$work/ops/reviews" 246 structure "$marker" | cut -f1)" | ||
| touch -r "$marker" "$old" | ||
| expect_eq "a transcript with the marker's exact mtime yields STALE" \ | ||
| STALE "$(swarm_lens_result "$work/ops/reviews" 246 structure "$marker" | cut -f1)" | ||
|
|
||
| fresh="$work/ops/reviews/20260909-1200-pr246-structure.md" | ||
| printf 'REVIEW_PASSED\n' > "$fresh" | ||
| touch -t 202601010002 "$fresh" | ||
| expect_eq "the newest fresh transcript wins" \ | ||
| PASSED "$(swarm_lens_result "$work/ops/reviews" 246 structure "$marker" | cut -f1)" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # End-to-end: the real swarm-post.sh, with agent-relay and gh stubbed. | ||
| # --------------------------------------------------------------------------- | ||
| echo "== swarm-post.sh end to end ==" | ||
|
|
||
| # $1 = sync behaviour: 'writes:<lens>=<verdict>,...' or 'nochanges' | ||
| run_post() { | ||
| local spec=$1 sandbox bin | ||
| sandbox=$(mktemp -d) | ||
| bin="$sandbox/bin"; mkdir -p "$bin" "$sandbox/repo" | ||
|
|
||
| cat > "$bin/agent-relay" <<STUB | ||
| #!/usr/bin/env bash | ||
| # Stubs \`agent-relay cloud sync\`. Real CLI exits 1 and prints | ||
| # "No changes to sync" when the workflow modified nothing -- the shape seen on | ||
| # runs 34274491229 (#247) and 34331239850 (#248). | ||
| # The exit status and message propagation matter here, not the CLI's complete | ||
| # prose. The assertion below checks only the diagnostic substring. | ||
| if [ "\$1" = cloud ] && [ "\$2" = sync ]; then | ||
| if [ "$spec" = nochanges ]; then | ||
| echo "No changes to sync — the workflow did not modify any files." | ||
| exit 1 | ||
| fi | ||
| # swarm-post.sh creates its marker immediately before calling this stub. | ||
| # Two seconds provide margin beyond the whole-second precision modeled by | ||
| # the comparator fixture. This is not a claim about arbitrary clock changes. | ||
| sleep 2 | ||
| mkdir -p ops/reviews | ||
| for pair in \$(echo "${spec#writes:}" | tr ',' ' '); do | ||
| lens=\${pair%%=*}; v=\${pair##*=} | ||
| printf 'Full review body.\n%s\n' "\$v" \ | ||
| > "ops/reviews/20260909-1200-pr999-\$lens.md" | ||
| done | ||
| echo "Synced." | ||
| fi | ||
| exit 0 | ||
| STUB | ||
|
|
||
| # gh must never reach the network from a test. Record calls instead. | ||
| cat > "$bin/gh" <<STUB | ||
| #!/usr/bin/env bash | ||
| printf '%s\n' "gh \$*" >> "$sandbox/gh-calls.log" | ||
| exit 0 | ||
| STUB | ||
| chmod +x "$bin/agent-relay" "$bin/gh" | ||
|
|
||
| ( cd "$sandbox/repo" && PATH="$bin:$PATH" \ | ||
| bash "$script_dir/swarm-post.sh" test-run-id 999 >"$sandbox/out" 2>&1 ) | ||
| local rc=$? | ||
| post_out=$(cat "$sandbox/out") | ||
| post_log=$(cat "$sandbox/gh-calls.log" 2>/dev/null || true) | ||
| rm -rf "$sandbox" | ||
| return $rc | ||
| } | ||
|
|
||
| # Did the run report its verdict to the PR at all? A gate that fails silently | ||
| # is only half a gate: the check is red but nothing says which lens objected. | ||
| posted_a_rollup() { case "$post_log" in *"pr comment"*) return 0 ;; *) return 1 ;; esac; } | ||
|
|
||
| # THE load-bearing assertion. A genuine objection from one lens must fail the | ||
| # gate even when the other two pass. | ||
| run_post 'writes:maintainability=REVIEW_PASSED,history=REVIEW_PASSED,structure=REVIEW_FAILED' | ||
| expect_eq "one lens REVIEW_FAILED fails the gate (exit 1)" 1 "$?" | ||
| case "$post_log" in | ||
| *"- structure: FAILED"*) ok "the objection is reported as FAILED, not STALE" ;; | ||
| *) notok "the objection is reported as FAILED, not STALE" "structure: FAILED" "$post_log" ;; | ||
| esac | ||
| posted_a_rollup \ | ||
| && ok "a failing run still reports its verdict to the PR" \ | ||
| || notok "a failing run still reports its verdict to the PR" "a gh comment" "none" | ||
|
|
||
| # The counterweight: a gate that cannot pass is as broken as one that cannot | ||
| # fail. Without this, every assertion above is satisfiable by \`exit 1\`. | ||
| run_post 'writes:maintainability=REVIEW_PASSED,history=REVIEW_PASSED,structure=REVIEW_PASSED' | ||
| expect_eq "three clean passes pass the gate (exit 0)" 0 "$?" | ||
|
|
||
| run_post 'writes:maintainability=REVIEW_PASSED,history=REVIEW_PASSED,structure=UNCLEAR_JUNK' | ||
| expect_eq "an UNCLEAR lens fails the gate" 1 "$?" | ||
|
|
||
| run_post 'writes:maintainability=REVIEW_PASSED,history=REVIEW_PASSED' | ||
| expect_eq "a lens with no transcript at all fails the gate" 1 "$?" | ||
|
|
||
| # The #246/#247/#248 production shape: swarm died, nothing synced. | ||
| run_post nochanges | ||
| rc=$? | ||
| [ "$rc" -ne 0 ] && ok "an empty sync fails the gate (exit $rc)" \ | ||
| || notok "an empty sync fails the gate" "non-zero" "$rc" | ||
|
|
||
| case "$post_out" in | ||
| *"No changes to sync"*) ok "the empty-sync reason reaches the step log" ;; | ||
| *) notok "the empty-sync reason reaches the step log" "the CLI message" "$post_out" ;; | ||
| esac | ||
|
|
||
| # TODO (PR #248): give empty syncs a current failure comment. `set -e` kills | ||
| # swarm-post.sh at `agent-relay cloud sync`, so when the swarm dies the PR gets | ||
| # no comment from this run and the previous run's rollup stays visible. The | ||
| # gate is still red -- `Enforce swarm result` is a separate step keyed on | ||
| # swarm_status -- but a reader looking only at PR comments sees a stale verdict. | ||
| # This is deliberately outside pass/fail accounting. When the posting path is | ||
| # repaired, add a positive assertion for the new contract; never require the bug. | ||
|
|
||
| echo | ||
| printf '%d passed, %d failed\n' "$pass" "$fail" | ||
| [ "$fail" -eq 0 ] | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| $ bash .github/workflows/scripts/swarm-gate.test.sh | ||
| == verdict extraction == | ||
| ok a bare REVIEW_FAILED is FAILED | ||
| ok a bare REVIEW_PASSED is PASSED | ||
| ok trailing blank lines do not hide the marker | ||
| ok surrounding whitespace is trimmed | ||
| ok a marker followed by a sign-off is UNCLEAR (PR #240 bug) | ||
| ok REVIEW_FAILED is never upgraded by surrounding prose | ||
| ok an empty transcript is UNCLEAR, not PASSED | ||
| ok a transcript merely containing the word is UNCLEAR | ||
| == lens selection == | ||
| ok no reviews directory yields MISSING | ||
| ok an absent transcript yields MISSING | ||
| ok a transcript predating the run yields STALE | ||
| ok a transcript with the marker's exact mtime yields STALE | ||
| ok the newest fresh transcript wins | ||
| == swarm-post.sh end to end == | ||
| ok one lens REVIEW_FAILED fails the gate (exit 1) | ||
| ok the objection is reported as FAILED, not STALE | ||
| ok a failing run still reports its verdict to the PR | ||
| ok three clean passes pass the gate (exit 0) | ||
| ok an UNCLEAR lens fails the gate | ||
| ok a lens with no transcript at all fails the gate | ||
| ok an empty sync fails the gate (exit 1) | ||
| ok the empty-sync reason reaches the step log | ||
| NOTE PR #248 limitation: empty sync posts no comment; not a passing assertion | ||
|
|
||
| 21 passed, 0 failed | ||
|
|
||
| EXIT_CODE=0 |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| """Run the original PR self-test with the corrected whole-second comparator.""" | ||
| import os | ||
| from pathlib import Path | ||
| import subprocess | ||
| import tempfile | ||
|
|
||
| root = Path.cwd() | ||
| Path('.relayflow').mkdir(exist_ok=True) | ||
| with tempfile.TemporaryDirectory(dir='.relayflow', prefix='swarm-baseline-') as directory: | ||
| target = Path(directory).resolve() | ||
| for name in ['swarm-gate.test.sh', 'swarm-post.sh', 'swarm-verdict.sh']: | ||
| source = f'066e2deecea5ffb88fdce088a98da111b547d803:.github/workflows/scripts/{name}' | ||
| (target / name).write_bytes(subprocess.check_output(['git', 'show', source])) | ||
| env = dict(os.environ, BASH_ENV=str(root / 'ops/runtime-evidence/swarm-threads-0909-coarse.bash')) | ||
| # This is a timing race; preserve every attempt, including passing ones. | ||
| for attempt in range(1, 6): | ||
| result = subprocess.run(['bash', str(target / 'swarm-gate.test.sh')], env=env, | ||
| text=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) | ||
| print(f'BASELINE_ATTEMPT={attempt}') | ||
| print(result.stdout, end='') | ||
| print(f'EXIT_CODE={result.returncode}') | ||
| if result.returncode: | ||
| raise SystemExit(result.returncode) | ||
| print('NO_FAILURE_OBSERVED_IN_FIVE_ATTEMPTS') |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| $ bash .github/workflows/scripts/swarm-gate.test.sh | ||
| == verdict extraction == | ||
| ok a bare REVIEW_FAILED is FAILED | ||
| ok a bare REVIEW_PASSED is PASSED | ||
| ok trailing blank lines do not hide the marker | ||
| ok surrounding whitespace is trimmed | ||
| ok a marker followed by a sign-off is UNCLEAR (PR #240 bug) | ||
| ok REVIEW_FAILED is never upgraded by surrounding prose | ||
| ok an empty transcript is UNCLEAR, not PASSED | ||
| ok a transcript merely containing the word is UNCLEAR | ||
| == lens selection == | ||
| ok no reviews directory yields MISSING | ||
| ok an absent transcript yields MISSING | ||
| ok a transcript predating the run yields STALE | ||
| ok the newest fresh transcript wins | ||
| == swarm-post.sh end to end == | ||
| ok one lens REVIEW_FAILED fails the gate (exit 1) | ||
| ok a failing run still reports its verdict to the PR | ||
| ok three clean passes pass the gate (exit 0) | ||
| ok an UNCLEAR lens fails the gate | ||
| ok a lens with no transcript at all fails the gate | ||
| ok an empty sync fails the gate (exit 1) | ||
| ok the empty-sync reason reaches the step log | ||
| ok KNOWN: an empty sync posts no comment; the red check is the only signal | ||
|
|
||
| 20 passed, 0 failed | ||
|
|
||
| EXIT_CODE=0 |
35 changes: 35 additions & 0 deletions
35
ops/runtime-evidence/swarm-threads-0909-bootstrap-probe.mjs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { execFileSync, spawnSync } from 'node:child_process'; | ||
| import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; | ||
| import { createRequire } from 'node:module'; | ||
| import { join, resolve } from 'node:path'; | ||
|
|
||
| const require = createRequire(resolve('packages/sdk/package.json')); | ||
| const source = process.argv[2] | ||
| ? execFileSync('git', ['show', `${process.argv[2]}:.github/workflows/review-swarm.yml`], { encoding: 'utf8' }) | ||
| : readFileSync('.github/workflows/review-swarm.yml', 'utf8'); | ||
| const flow = require('js-yaml').load(source); | ||
| const command = flow.jobs.review.steps.find(step => step.name === "Self-test the gate's verdict logic").run; | ||
| mkdirSync('.relayflow', { recursive: true }); | ||
| for (const [label, pr, script, expected] of [ | ||
| ['introducing PR without test', '248', null, 0], | ||
| ['later PR without test', '249', null, 1], | ||
| ['present passing test', '249', 'echo SELF_TEST_RAN; exit 0', 0], | ||
| ['present failing test', '249', 'echo SELF_TEST_RAN; exit 7', 7], | ||
| ]) { | ||
| const cwd = mkdtempSync(resolve('.relayflow/bootstrap-probe-')); | ||
| try { | ||
| if (script !== null) { | ||
| const directory = join(cwd, 'gate-files/.github/workflows/scripts'); | ||
| mkdirSync(directory, { recursive: true }); | ||
| writeFileSync(join(directory, 'swarm-gate.test.sh'), script); | ||
| } | ||
| const result = spawnSync('bash', ['-e', '-c', command], { | ||
| cwd, encoding: 'utf8', env: { ...process.env, REVIEW_PR_NUMBER: pr }, | ||
| }); | ||
| console.log(`${label}: exit=${result.status}, expected=${expected}`); | ||
| process.stdout.write(result.stdout + result.stderr); | ||
| assert.equal(result.status, expected); | ||
| if (script !== null) assert.match(result.stdout, /SELF_TEST_RAN/); | ||
| } finally { rmSync(cwd, { recursive: true, force: true }); } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.