Skip to content

chore: annotate pre-existing shellcheck findings that fail CI lint - #10

Merged
rub-a-dub-dub merged 3 commits into
mainfrom
fm/firstmate-lint-debt-blocking-prs
Sep 20, 2026
Merged

rub-a-dub-dub merged 3 commits into
mainfrom
fm/firstmate-lint-debt-blocking-prs

Conversation

@rub-a-dub-dub

@rub-a-dub-dub rub-a-dub-dub commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Intent

Workflow delivery was enabled on this fork tonight, after the captain chose to fix CI rather than merge unchecked. Within minutes the Lint job failed on an in-flight pull request - and the failure is main's own debt, not anything that change introduced.

Proven, not assumed: the SC2034 warnings name FF_UPDATE_FAILED, REMOTE_UPDATE_FAILED_STATUS and FF_RUN_VERIFIED, which live in bin/fm-ff-lib.sh, bin/fm-update.sh and bin/fm-remote-secondmate-control.sh. FF_RUN_VERIFIED appears three times on origin/main. The pull request reddened by it touches only bootstrap-diagnostics/SKILL.md, bin/fm-backlog-transition-lib.sh, bin/fm-bootstrap.sh and tests/fm-backlog-atomicity.test.sh - none of the implicated files.

CI had never run on a pull request in this repository: lifetime run count was 1, a manual dispatch, because a GitHub fork-workflow gate suppressed every event. So the gate is doing exactly its job on first contact - finding the debt that accumulated while nothing was looking. Two unrelated changes are currently blocked behind it.

What Changed

  • Added shellcheck disable=SC2034 annotations in bin/fm-ff-lib.sh for FF_UPDATE_FAILED, REMOTE_UPDATE_FAILED_STATUS, and FF_RUN_VERIFIED, each naming the sourcing script (bin/fm-update.sh, bin/fm-remote-secondmate-control.sh) that reads the global.
  • Added shellcheck disable annotations for two false positives in the test suite: SC2100 on a literal task-id assignment in tests/fm-daemon.test.sh, and SC2031 on six $root uses in tests/fm-update.test.sh that are tainted by an unrelated local root in tests/lib.sh rather than modified in a subshell.
  • Updated the shellcheck style rule in .agents/skills/firstmate-coding-guidelines/SKILL.md from bin/*.sh and bin/backends/*.sh to all tracked shell scripts including tests, pointing at bin/fm-lint.sh's header as the owner of the exact file set and of which codes run only in CI.

Risk Assessment

✅ Low: The change adds only ShellCheck directive comments — no executable line is altered, so runtime behavior cannot change — and I verified that each directive maps one-to-one onto a finding present at the base commit, that every claimed false positive really is one, and that the intent's acceptance criterion holds: both CI=true bin/fm-lint.sh and an unsharded full-canonical-set shellcheck --norc --external-sources sweep now exit 0.

Testing

I ran the two suites the change touches — tests/fm-update.test.sh (also the behavioral owner of bin/fm-ff-lib.sh) and tests/fm-daemon.test.sh — and both pass, including every test that guards an annotated region: T3f/T3f2/T3g/T3h/T3i around the SC2031 comments and the cross-file status protocol, and the enriched-wedge pause-cadence test that owns the SC2100 line at tests/fm-daemon.test.sh:697 (the user-selected round-1 fix, now applied). Because the SC2034 suppressions assert that FF_UPDATE_FAILED, REMOTE_UPDATE_FAILED_STATUS and FF_RUN_VERIFIED are read cross-file after sourcing, I drove the real CLIs in a sandbox to prove those reads are live at the user-visible surface rather than masked dead code: fm-update.sh prints origin-verified: yes and flips to no on an unreachable origin, its process exit status is FF_UPDATE_FAILED, and fm-remote-secondmate-control.sh answers the distinct exit 3 when the fork-sync classifier fires versus a generic exit 1 for an ordinary skip. That transcript is the reviewer-visible artifact. This change has no UI surface, so no screenshot applies. The one piece of evidence I did not produce is a green bin/fm-lint.sh run showing the SC2034/SC2031/SC2100 warnings gone — that is a linter invocation this test phase is barred from running, and it belongs to the lint phase and remote CI. Everything I exercised passed and the worktree is clean.

Evidence: CLI transcript: the three SC2034-exempted globals driving real firstmate CLI output and exit codes

Source: CLI transcript: the three SC2034-exempted globals driving real firstmate CLI output and exit codes

$ . bin/fm-ff-lib.sh; echo "$REMOTE_UPDATE_FAILED_STATUS / $FF_RUN_VERIFIED / $FF_UPDATE_FAILED" 3 / yes / 0 $ FM_ROOT_OVERRIDE=<sandbox>/main FM_HOME=<sandbox>/home bin/fm-update.sh firstmate: already current origin-verified: yes $ echo $? # <- this exit status IS FF_UPDATE_FAILED 0 --- case 1: this host's own fork-sync classifier fired --- $ bin/fm-remote-secondmate-control.sh update sm1 fork sync: FAILED protected fork branch error: remote code root update failed $ echo $? # <- REMOTE_UPDATE_FAILED_STATUS (3), the distinct fleet-wide status 3 --- case 2: an ordinary skip, NOT the classifier --- $ bin/fm-remote-secondmate-control.sh update sm1 firstmate: skipped: dirty working tree error: remote code root did not complete a safe origin update $ echo $? # <- stays the generic 1, which the parent sweep reads as a skip 1 --- FF_RUN_VERIFIED varies: unreachable origin flips it to 'no' --- $ FM_ROOT_OVERRIDE=<sandbox>/main FM_HOME=<sandbox>/home bin/fm-update.sh firstmate: skipped: fetch failed origin-verified: no $ echo $? 0

==============================================================
 bin/fm-ff-lib.sh declares the protocol constant it exempts
==============================================================
$ . bin/fm-ff-lib.sh; echo "$REMOTE_UPDATE_FAILED_STATUS / $FF_RUN_VERIFIED / $FF_UPDATE_FAILED"
3 / yes / 0

==============================================================
 FF_RUN_VERIFIED + FF_UPDATE_FAILED: read by bin/fm-update.sh
==============================================================
$ FM_ROOT_OVERRIDE=<sandbox>/main FM_HOME=<sandbox>/home bin/fm-update.sh
firstmate: already current
origin-verified: yes
reread-firstmate: no
restart-secondmates: none
nudge-secondmates: none
$ echo $?   # <- this exit status IS FF_UPDATE_FAILED (fm-update.sh: exit "$FF_UPDATE_FAILED")
0

==============================================================
 REMOTE_UPDATE_FAILED_STATUS: read by fm-remote-secondmate-control.sh
==============================================================
--- case 1: this host's own fork-sync classifier fired ---
$ FM_HOME=<sandbox>/sm1 FM_ROOT_OVERRIDE=<sandbox>/coderoot bin/fm-remote-secondmate-control.sh update sm1
fork sync: FAILED protected fork branch
error: remote code root update failed
$ echo $?   # <- must be REMOTE_UPDATE_FAILED_STATUS (3), the distinct fleet-wide status
3

--- case 2: an ordinary skip, NOT the classifier ---
$ FM_HOME=<sandbox>/sm1 FM_ROOT_OVERRIDE=<sandbox>/coderoot bin/fm-remote-secondmate-control.sh update sm1
firstmate: skipped: dirty working tree
error: remote code root did not complete a safe origin update
$ echo $?   # <- must stay the generic 1, which the parent sweep reads as a skip
1

==============================================================
 FF_RUN_VERIFIED varies: an unreachable origin flips it to 'no'
==============================================================
$ FM_ROOT_OVERRIDE=<sandbox>/main FM_HOME=<sandbox>/home bin/fm-update.sh
firstmate: skipped: fetch failed
origin-verified: no
reread-firstmate: no
restart-secondmates: none
nudge-secondmates: none
$ echo $?   # <- still FF_UPDATE_FAILED; an unreachable origin is a skip, not a failure
0

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info
  • ℹ️ tests/fm-update.test.sh:395 - Three of the seven new SC2031 rationale comments (above head lines 396, 414 and 421) suppress two findings each, not one: at base commit 6339e2d, shellcheck under CI posture reports both root was modified in a subshell and home was modified in a subshell at base lines 393, 408 and 414. The comment text names only "fm_test_tmproot() local root". Both are equally false positives (home=&#34;$w/sm1&#34; at line 377 is a plain function-local assignment, read in the same shell), and the commit message states it correctly ("local root/home", "SC2031 (10x)"), so nothing is mis-suppressed. The only cost is that a future reader auditing whether the directive is still warranted would check root alone and could prematurely narrow it. No action needed; noting the asymmetry between the accurate commit message and the narrower in-code comment.
✅ **Test** - passed

✅ No issues found.

  • bin/fm-test-run.sh tests/fm-update.test.sh tests/fm-daemon.test.sh — both suites exit 0 (total=2 failed=0 skipped_gate=0)
  • bash tests/fm-daemon.test.sh — confirmed ok - an enriched wedge under a declared wait uses the pause cadence and restores wedge detection on resume, the test owning the SC2100-annotated line 697
  • Confirmed passing within fm-update.test.sh: T3f a remote fork-synchronization failure fails the whole run, T3f2, T3g an unverified remote host never reports already current, T3h, and T3i the host-local update leg raises the fork-sync status only for its own classifier&#39;s verdict (the SC2031-annotated function)
  • Manual CLI verification: FM_ROOT_OVERRIDE=&lt;sandbox&gt;/main FM_HOME=&lt;sandbox&gt;/home bin/fm-update.sh in a sandboxed git clone — observed origin-verified: yes and exit 0 (the exit status is FF_UPDATE_FAILED)
  • Manual CLI verification: same command after git remote set-url origin &lt;missing&gt; — observed firstmate: skipped: fetch failed / origin-verified: no, proving FF_RUN_VERIFIED varies and reaches stdout
  • Manual CLI verification: FM_HOME=&lt;sandbox&gt;/sm1 FM_ROOT_OVERRIDE=&lt;sandbox&gt;/coderoot bin/fm-remote-secondmate-control.sh update sm1 against a stubbed code root — exit 3 (REMOTE_UPDATE_FAILED_STATUS) when the fork-sync classifier fires, exit 1 for an ordinary skip
  • . bin/fm-ff-lib.sh then printed REMOTE_UPDATE_FAILED_STATUS / FF_RUN_VERIFIED / FF_UPDATE_FAILED — 3 / yes / 0
  • git status --porcelain — worktree clean, no transient test artifacts left behind
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

CI check-board note

The forge reports 15 check runs on this PR's head commit, 14 successful and 1
failed - not 15/15 green. The 1 failure is the "PR must be raised via
no-mistakes" attestation check's first run (started 05:59:46), superseded on
the identical commit by a second run of the same check (started 06:00:27)
that passed once the attestation was re-signed against this exact head. Every
other check, including the previously-cancelled "Behavior portable serial 1"
(re-run directly and completed with conclusion=success), has exactly one run
and it is green. Confirmed via gh api repos/rub-a-dub-dub/firstmate/commits/<sha>/check-runs.

FF_UPDATE_FAILED, REMOTE_UPDATE_FAILED_STATUS, and FF_RUN_VERIFIED are
each set in fm-ff-lib.sh and read only by scripts that source it
(fm-update.sh, and REMOTE_UPDATE_FAILED_STATUS also by
fm-remote-secondmate-control.sh), so ShellCheck cannot see the
consumption and flags each as unused. A repo-wide full-dataflow sweep
found no other SC2034 offenders.
… Lint

CI's full shellcheck posture (--norc --external-sources, whole canonical
set) caught two classes of pre-existing debt in tests/fm-update.test.sh
and tests/fm-daemon.test.sh, both byte-identical to origin/main and thus
predating this branch:

- SC2031 (10x, tests/fm-update.test.sh:386,391,393,401,406,408,414):
  false positive. ShellCheck's cross-file dataflow analysis conflates
  this function's plain, never-subshelled local root/home with an
  unrelated local root in tests/lib.sh's fm_test_tmproot(), which is
  reassigned via command substitution at its own line 183.

- SC2100 (1x, tests/fm-daemon.test.sh:697): false positive. The literal
  hyphenated task id "paused-wedge-w1" is misread as a chained
  subtraction expression; it is a string, not arithmetic.

Both are exempted with narrowly scoped shellcheck disable directives
matching this repo's established convention (e.g.
tests/fm-remote-job.test.sh:114, tests/fm-trace-context-spawn.test.sh:380).
A full unfiltered sweep of the whole canonical set (bin/*.sh,
bin/backends/*.sh, tests/*.sh) under CI's exact posture found no other
offenders beyond these 11 findings plus the 3 SC2034 findings fixed in
the prior commit.
@rub-a-dub-dub rub-a-dub-dub changed the title chore(bin): silence SC2034 on fm-ff-lib.sh's cross-file globals chore: annotate pre-existing shellcheck findings that fail CI lint Sep 20, 2026
@rub-a-dub-dub
rub-a-dub-dub merged commit 126eaec into main Sep 20, 2026
27 of 29 checks passed
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