fix(45,49): make the red gates report what they found, then fix what they found - #53
Merged
Merged
Conversation
#52 landed with both of its gates red on main, and each said only "Process completed with exit code 1". Two causes: * GitHub invokes bash with `-eo pipefail`. The commands these gates exist to watch fail — `cargo fmt --check` on a dirty tree, `cargo test` with a failing test, `diff` on a drifted fixture — so errexit killed the step before its own error handling ran. Every step now opens with `set +e`, handles its status explicitly, and ends in an explicit `exit`. * The detail only ever reached the run log. A shared `annotate` helper now re-emits the head of the failing output as a check annotation: the rustfmt diff, the `failures:` section of a test run, the fixture-currency diff, the shellcheck findings. A gate whose failure you cannot read is a gate you fix by guessing. The toolchain step also gains `components: clippy, rustfmt`; without rustfmt the drift diagnostic had nothing to run. This is the diagnostic half only — it does not fix whatever rustfmt and the suite are reporting. It makes them legible so the next commit can. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Contributor
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…he fixture
SC2034, SC2001 and SC2155 were reported against the committed fixture, but the
fixture is a faithful record of what `mint` writes, so all three are properties
of `templates/launcher.sh.tera` and reached every launcher in the estate. Fixed
at the source; the fixture was then re-minted, and is now clean at EVERY
shellcheck severity, not only at warning and above.
SC2034 — `APP_PORT` unused. Answered by deciding which of the two is true
rather than by disabling the lint, because the lint was pointing at a real
duplicate: `URL` hardcoded the port on the line above and `APP_PORT` restated
it, so a config carrying both could put two different ports in one script and
nothing would notice. The port now has ONE spelling. No explicit
`[runtime].url` → `APP_PORT` is emitted and `URL` is composed from it, so they
cannot disagree. Explicit URL → the URL is the whole answer and `APP_PORT` is
not emitted at all. `the_port_has_exactly_one_spelling_in_the_generated_script`
pins both arms, and `render` now warns at mint time when a config sets a URL and
a port that contradict each other — the disagreement the duplicate used to hide.
SC2001 — `echo "$body" | sed 's/^/ /'` becomes
`printf ' %s\n' "${body//$'\n'/$'\n '}"`: the same two-space indent on every
line including empty ones, with no subprocess per call. Fixed rather than
carrying an inline justification.
SC2155 — `local script_path="$(cd … && pwd)/$(basename …)"` takes its exit
status from `local`, so a failed `cd` was swallowed and the next line copied the
script to `$LAUNCHER_TARGET` from a path assembled out of nothing. Declared and
assigned separately, with the failure now reported. Of the three this is the one
that masked a real error.
Also reverts the previous commit's `app_license` collapse. #45 AC2 read the
pre-existing `cargo fmt --check` drift as rustfmt wanting that call on one line;
measured against the CI annotation it wants the opposite — the arguments exceed
`fn_call_width` (60), so the five-line form on main was already clean and the
collapse introduced the drift. The gate's own diagnostic is what settled it.
Non-vacuity, unsolicited: the shellcheck gate added in #52 caught SC2034 and
SC2155 in a launcher minted during the run BEFORE this commit existed, which is
the evidence #49 AC4 asks for — it fails at the point a template regression is
introduced, not at the point someone remembers to lint.
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
arena-ai-coding-agent
Bot
changed the title
hyperpolymath
approved these changes
Sep 25, 2026
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.
Part 1 — make the red gates on
mainreport what they found (#45)#52 landed with both of its gates red and each saying only
Process completed with exit code 1. Two causes:-eo pipefail, so the commands these gates exist towatch fail (
cargo fmt --check,cargo test,diff) killed their own stepbefore its error handling ran. Every step now opens with
set +e, handles itsstatus, and ends in an explicit
exit.annotatehelper re-emits thehead of the failing output as a check annotation: the rustfmt diff, the
failures:section, the fixture-currency diff, the shellcheck findings.That is what made the rest of this PR measurable instead of guesswork.
Part 2 — what the annotations then showed
cargo fmt(#45 AC2). The issue read the pre-existing drift as rustfmtwanting the
app_licenseinsert collapsed to one line. The annotation says theopposite: the arguments exceed
fn_call_width(60), so rustfmt wants thefive-line form — the code on
mainwas already clean and the collapse in #52introduced the drift. Reverted, with the reason recorded in the commit.
shellcheck (#49).
SC2034,SC2001,SC2155, all three fixed intemplates/launcher.sh.tera— not in the fixtures — and the currency-lockedfixture re-minted. It is now clean at every severity, not only warning and
above.
APP_PORTwas answered by deciding which of the two is true: the portnow has one spelling (composed from
APP_PORTwhen the config gives no URL; noAPP_PORTat all when it does), pinned bythe_port_has_exactly_one_spelling_in_the_generated_script, with a mint-timewarning for a config whose URL and port contradict each other.
Evidence
Build, count, mint, lint— build, the test-count floor, a launcher mintedduring the run, fixture currency byte-identical, shellcheck clean.
rust-ci / Cargo check + clippy + fmtandrust-ci / Cargo test— green.SC2034andSC2155in aminted launcher before the fix commit existed (templates/launcher.sh.tera: three Shellcheck findings reach every minted launcher #49 AC4, No workflow builds or tests the Rust crates — 59 tests have never run in CI #45 AC5).
Closes #49. Remaining from the six: #48 (world-writable
/tmpdefaults), #41(
REQUIRED_KEYSvs the deed's:required-fields), #42 (.deeddescriptordesign), #40 (verification write-up).