Skip to content

fix(ci): the v1.139 review's findings — injectable dispatch input, two gates that stop measuring, a machine-dependent verdict (#458) - #460

Merged
avrabe merged 1 commit into
mainfrom
fix/v1139-review-findings
Sep 18, 2026
Merged

avrabe merged 1 commit into
mainfrom
fix/v1139-review-findings

Conversation

@avrabe

@avrabe avrabe commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Verify-Filter: (= id "FV-RELAY-EVIDENCE-001")

Triage of the v1.139 release-candidate review (#458). Refs #458, #459.

Fixed here (4 real findings)

# finding why it matters
1 soak.yml interpolated ${{ github.event.inputs.gz_max_s }} into a run: block Actions expands ${{ }} before bash sees the text, so a dispatch-capable actor could end the assignment and run commands with the job's environment. The repo's own convention — stated in fleet-status.yml, followed by release.yml — is to bind inputs through env:.
2 the analytic soak tier lost its rungs to -e Same trap as the gz ladder: the bench exits 1 on a FAIL verdict, so OUT=$(...) ended the step before the FAIL=1; continue arm could walk the remaining durations. A red step with no measurement — for a job whose entire purpose is to publish a number.
3 check-evidence.rs read the tool's own environment to decide whether a $VAR was an unfilled placeholder The census disagreed with itself across machines: $WITNESS refused on a laptop, accepted on any runner exporting that name. Now it resolves only what the step assigns itself plus gate-supplied names. Verified identical output with and without WITNESS exported.
4 three nits, all real the two relay_ scanners disagreed on an identifier boundary (my_relay_foo → phantom relay-foo root); setup-gh cached under $RUNNER_TEMP, which Actions empties at both ends of every job, so gh was re-downloaded twice per fleet tick; release.yml's --generate-notes fallback could not fire.

On that last one I did not simply delete the dead branch: that would leave a silent no-op if the notes body were ever empty. It now fails loudly, which is what the dead branch was pretending to guarantee.

Dismissed (3 phantoms, all caused by how I sliced the candidate)

The review PRs carry only part of the tree, so the reviewer correctly reported what it could see and drew conclusions that do not hold on main:

  • "read_battery_v → Option<f32> breaks two impls and a call site" (REVIEW SLICE 2/2 — v1.139 substantive flight-path changes. DO NOT MERGE #459). On main both impls already return Option<f32>; they were excluded from the slice because their diffs are under 50 lines.
  • "three gate scripts are never invoked" (REVIEW SLICE 1/2 — v1.139 gate tooling + CI workflows. DO NOT MERGE #458). They are cited in rivet artifact steps (FV-FALCON-CLAIMS-001/003/004, FV-RELAY-EVIDENCE-001, FV-FALCON-WASMEQ-002) — the slice excluded artifacts/.
  • "check-component-claims.rs exits 1 for six of eight components; capability-reachability.rs exits 2 on a missing doc". Measured on main: all three exit 0. The slice lacked the very wasm/cm/*/Cargo.toml rewrites and docs/CAPABILITY-REACHABILITY.md that make them pass.

Lesson recorded: a partial slice invites confident findings about files it cannot see. Any future split needs the slice's own PR body to say so, or to be closed under the changes it references.

Still open from the review, deliberately not fixed here

#459's second finding is real and is NOT addressed by this PR: if read_battery_v starts returning None mid-flight, batt_state freezes at its last healthy snapshot and the low-battery failsafe can never fire — batt_present is only consulted at arm time. That is the same "absence is not health" defect #413 was about, on the in-flight side. It needs its own artifact and a failing test first, and a maintainer decision on whether it lands in v1.139 or v1.140.

All seven gate scripts' own test suites pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HvusAXYbHLyv3uTzfBcMbG

… two gates that stop measuring, and a machine-dependent verdict

From the review of the release candidate (#458). Four real findings and
three phantoms; the phantoms were artefacts of how the candidate was
sliced for review and are recorded in the PR, not here.

1. INJECTION. soak.yml interpolated `${{ github.event.inputs.gz_max_s }}`
   straight into a run block, which Actions expands before bash sees it:
   a dispatch-capable actor could end the assignment and run commands
   with the job's environment. The repo's own convention (stated in
   fleet-status.yml, followed by release.yml) is to bind inputs through
   `env:` and dereference a quoted shell variable. Now it does.

2. THE ANALYTIC TIER LOST ITS RUNGS the same way the gz ladder did.
   `shell: bash` runs with -e, the bench exits 1 on a FAIL verdict, so
   `OUT=$(...)` ended the step before the FAIL=1/continue arm could walk
   the remaining durations — a red step with no measurement. The exit
   code is captured now, as in the gz ladder.

3. A MACHINE-DEPENDENT VERDICT. check-evidence.rs decided whether a
   `$VAR` in an artifact step was an unfilled placeholder by reading the
   TOOL'S OWN environment, so `$WITNESS` was refused on a laptop and
   accepted on any runner that happened to export it — the census
   disagreed with itself across machines. It now resolves only what the
   step assigns itself plus the gate-supplied names; measured identical
   with and without WITNESS exported.

4. The two `relay_` scanners disagreed on an identifier boundary
   (`my_relay_foo` synthesised a phantom `relay-foo` root in one of
   them); setup-gh cached under $RUNNER_TEMP, which Actions empties at
   both ends of every job, so the guard never hit and gh was
   re-downloaded twice per fleet tick; and release.yml's
   `--generate-notes` fallback could not fire. That last one is now a
   loud failure rather than a dead branch — deleting it outright would
   have left a silent no-op if the notes body were ever empty, which is
   worse than the dead code.

All seven gate scripts' own test suites pass (7/4/6/0/8/4/4).

Refs #458

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HvusAXYbHLyv3uTzfBcMbG
@avrabe
avrabe merged commit f9ea446 into main Sep 18, 2026
12 checks passed
@avrabe
avrabe deleted the fix/v1139-review-findings branch September 18, 2026 06:53
avrabe added a commit that referenced this pull request Sep 18, 2026
… dispositioned (#461)

* verify(v1.139): the review checkpoint is done — 9 findings, every one dispositioned

/code-review ultra ran on the candidate falcon-v1.138.0..9f1df80 on
2026-09-18. It could not be reviewed as one diff (186 files, 21 420
lines, ~9.5 k of them lockfile churn), so it was split against a base
pinned at the previous tag: #458 (gate tooling + CI, 19 files) and #459
(the 28 source files with 50+ changed lines). Neither was ever merged.

  FIXED in #460 (f9ea446, on main before any tag): an injectable
  workflow_dispatch input; the analytic soak tier losing its rungs to
  -e; check-evidence.rs reading its own environment to decide whether a
  $VAR was a placeholder; two relay_ scanners disagreeing on an
  identifier boundary; setup-gh caching where Actions wipes it; a dead
  --generate-notes fallback, now a loud failure rather than a silent
  no-op.

  FILED into v1.140 by maintainer decision: losing the battery sense in
  flight raised no failsafe (#413's in-flight half) — reproduced, fixed
  on the v1.140 branch, SWREQ-FALCON-BATTERY-P03 + FV-FALCON-BATTERY-003.

  DISMISSED with measurements: three findings that were artefacts of the
  slicing (impls already updated on main; scripts cited in rivet steps
  the slice excluded; all three scripts exit 0 on main at 9f1df80).

NOT claimed: the reviewed diff excluded artifact YAML, the component
descriptions, wasm/, tests/, examples/, docs and lockfiles — the claims
TEXT a partner reads was checked mechanically, not by a reader.

Code-free promotion (two-commit rule): SWREQ-RELAY-REVIEW-P01 and
FV-RELAY-REVIEW-139 proposed -> verified.

Refs #458, #459

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HvusAXYbHLyv3uTzfBcMbG

* verify(v1.139): put the review record in the repo — the gate cannot run `gh`

The first draft of FV-RELAY-REVIEW-139's steps cited
`gh pr view 458/459` as the evidence of the findings. The verification
gate failed them with rc=127: gh is not installed on the self-hosted
runners — the same lesson as #153/#436, walked into again by writing a
step that only runs on a laptop.

The deeper problem is that it made a release's audit trail depend on
GitHub PR comments, which can be edited and belong to PRs that can be
deleted. The findings and their nine dispositions now live in
docs/reviews/falcon-v1.139.0-review.md, and the steps check that file:
it exists, and it carries exactly nine dispositions.

All four steps verified by running them as the gate does (sh -c).

Refs #458, #459

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HvusAXYbHLyv3uTzfBcMbG

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
avrabe added a commit that referenced this pull request Sep 18, 2026
…otes call that DARK (#463)

#460 fixed the ANALYTIC tier's -e trap and left the gz ladder's on the
v1.140 branch, so every soak run on main still dies at the first
breaking rung: exit 1, no rung printed, no reason, and no
longest-holding number — the value this job exists to publish.

The cost is not just a red job. scripts/verification-tracks.rs renders
the track into the release body, so falcon-v1.139.0 would have shipped
notes reading "Nightly endurance soak — DARK, failure", which a reader
takes as "the hold is broken". The truth is narrower and already
documented in #403: the gz hold breaks at 60 s and holds 40 s, and the
ladder threw that measurement away.

Same fix as the analytic tier: capture the rung's exit code, and say
which failure it was (timeout, verdict FAIL, or a bench that exited
without a verdict).

Found while reading what the release notes would actually say.

Refs #403, #458


Claude-Session: https://claude.ai/code/session_01HvusAXYbHLyv3uTzfBcMbG

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant