From fafc84e4b62807093e51029a5ef9d37e5b01c170 Mon Sep 17 00:00:00 2001 From: rub-a-dub-dub Date: Sat, 19 Sep 2026 20:57:16 -0700 Subject: [PATCH 1/3] fix(bin): exempt fm-ff-lib.sh's externally-read globals from SC2034 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. --- bin/fm-ff-lib.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/fm-ff-lib.sh b/bin/fm-ff-lib.sh index f3ac3ba069e..fecf575014c 100644 --- a/bin/fm-ff-lib.sh +++ b/bin/fm-ff-lib.sh @@ -379,6 +379,7 @@ fetch_once() { fi FF_FETCH_ERROR="fetch failed" if ! ff_sync_origin_fork "$dir"; then + # shellcheck disable=SC2034 # Read by bin/fm-update.sh (exit "$FF_UPDATE_FAILED") after sourcing this library. FF_UPDATE_FAILED=1 return 1 fi @@ -433,6 +434,7 @@ remote_sync_failure_reason() { # # FF_UPDATE_FAILED result the local route already sets, instead of collapsing # into an ordinary skip the caller cannot distinguish from ubiquitous # transport failure. +# shellcheck disable=SC2034 # Read by bin/fm-update.sh and bin/fm-remote-secondmate-control.sh after sourcing this library. REMOTE_UPDATE_FAILED_STATUS=3 dirty_status() { @@ -508,6 +510,7 @@ ff_target() { echo "$label: skipped: $FF_FETCH_ERROR" return 0 fi + # shellcheck disable=SC2034 # Read by bin/fm-update.sh (echo "origin-verified: $FF_RUN_VERIFIED") after sourcing this library. [ -z "$FF_FORK_UNVERIFIED" ] || FF_RUN_VERIFIED=no fi default=$(default_branch "$dir") || { From adbf2c3a04e3bbf775e899123e346fb42cfacb19 Mon Sep 17 00:00:00 2001 From: rub-a-dub-dub Date: Sat, 19 Sep 2026 22:17:20 -0700 Subject: [PATCH 2/3] fix(tests): exempt pre-existing SC2031/SC2100 false positives from CI 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. --- tests/fm-daemon.test.sh | 1 + tests/fm-update.test.sh | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/tests/fm-daemon.test.sh b/tests/fm-daemon.test.sh index 510a4320988..ab5088fda9c 100755 --- a/tests/fm-daemon.test.sh +++ b/tests/fm-daemon.test.sh @@ -694,6 +694,7 @@ test_enriched_wedge_under_declared_wait_uses_pause_cadence() { local dir state fakebin task win pane key reason i escalations dir=$(make_supercase enriched-wedge-declared-wait) state="$dir/state"; fakebin="$dir/fakebin" + # shellcheck disable=SC2100 # Literal task id, not arithmetic. task=paused-wedge-w1; win="sess:fm-$task"; pane="$dir/pane.txt" key=$(printf '%s' "$task" | tr ':/.' '___') fm_write_meta "$state/$task.meta" "window=$win" "backend=tmux" diff --git a/tests/fm-update.test.sh b/tests/fm-update.test.sh index 09b287c3785..22eaa3920eb 100755 --- a/tests/fm-update.test.sh +++ b/tests/fm-update.test.sh @@ -383,13 +383,16 @@ test_remote_leg_raises_the_fork_sync_status() { expected_status=$(. "$ROOT/bin/fm-ff-lib.sh"; printf '%s\n' "$REMOTE_UPDATE_FAILED_STATUS") # This host's own update exits nonzero: its FF_UPDATE_FAILED classifier fired. + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. cat > "$root/bin/fm-update.sh" <<'SH' #!/usr/bin/env bash printf 'fork sync: FAILED protected fork branch\n' exit 1 SH + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. chmod +x "$root/bin/fm-update.sh" rc=0 + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. out=$(FM_HOME="$home" FM_ROOT_OVERRIDE="$root" "$control" update sm1 2>&1) || rc=$? expect_code "$expected_status" "$rc" \ "a fork-synchronization failure on this host must raise the distinct status, not the generic die" @@ -398,19 +401,23 @@ SH # The root update completes but proves no safe origin result: an ordinary # failure, not the classifier, so it stays exit 1 and the parent reads a skip. + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. cat > "$root/bin/fm-update.sh" <<'SH' #!/usr/bin/env bash printf 'firstmate: skipped: dirty working tree\n' exit 0 SH + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. chmod +x "$root/bin/fm-update.sh" rc=0 + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. out=$(FM_HOME="$home" FM_ROOT_OVERRIDE="$root" "$control" update sm1 2>&1) || rc=$? expect_code 1 "$rc" "an unsafe root origin update is an ordinary skip, not the fork-sync status" # A home guard rejection never reaches this host's update at all, so it cannot # be escalated into a fleet-wide fork-sync failure either. rc=0 + # shellcheck disable=SC2031 # False positive: cross-file taint from tests/lib.sh's unrelated fm_test_tmproot() local root; not modified in a subshell here. out=$(FM_HOME="$home" FM_ROOT_OVERRIDE="$root" "$control" update sm2 2>&1) || rc=$? expect_code 1 "$rc" "an unusable remote home is an ordinary skip, not the fork-sync status" assert_contains "$out" "remote home belongs to sm1, not sm2" \ From 56773f3db02cf5df683857fde3f1604e12976628 Mon Sep 17 00:00:00 2001 From: rub-a-dub-dub Date: Sat, 19 Sep 2026 22:59:30 -0700 Subject: [PATCH 3/3] no-mistakes(document): scope shellcheck style rule to tracked test scripts --- .agents/skills/firstmate-coding-guidelines/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.agents/skills/firstmate-coding-guidelines/SKILL.md b/.agents/skills/firstmate-coding-guidelines/SKILL.md index 0ed6d4b8f52..5885617b108 100644 --- a/.agents/skills/firstmate-coding-guidelines/SKILL.md +++ b/.agents/skills/firstmate-coding-guidelines/SKILL.md @@ -3,7 +3,7 @@ name: firstmate-coding-guidelines description: >- Agent-only reference for changing firstmate's shared, tracked material per AGENTS.md section 1. Use before editing any of that material, whether working as firstmate directly or as a crewmate briefed on a firstmate-repo task. - Covers the knowledge-placement decision tree, the one-owner rule for contracts, the inline-stub pattern for content moved into a skill, AGENTS.md size discipline, trigger hygiene for new skills, and repo style rules (one sentence per line, plain dash, no agent co-author, shellcheck-clean bin scripts, colocated tests, and maintainer-verification evidence). + Covers the knowledge-placement decision tree, the one-owner rule for contracts, the inline-stub pattern for content moved into a skill, AGENTS.md size discipline, trigger hygiene for new skills, and repo style rules (one sentence per line, plain dash, no agent co-author, shellcheck-clean shell scripts, colocated tests, and maintainer-verification evidence). user-invocable: false metadata: internal: true @@ -124,7 +124,7 @@ Firstmate PR #3644 demonstrated the cost: pinning a 75-162-script walk took 32.7 - Never wrap multiple sentences onto one physical line. - Plain dash `-`, never an em dash. - Never add an agent name as a commit co-author. -- `bin/*.sh` and `bin/backends/*.sh` must pass `shellcheck`. +- Tracked shell scripts must pass `shellcheck`, test scripts included; `bin/fm-lint.sh`'s header owns the exact file set and which codes only run in CI. - Run `bin/fm-lint.sh` before treating a script change as done; it is the single owner of the lint definition that CI and the no-mistakes pre-push gate both invoke, its own header owns what that definition covers, and it refuses to run under any other version of either linter. - When a task names a specific tool, implement the work with that tool, or explicitly flag the substitution and its new dependency footprint for review before shipping. - Colocate tests with the existing pattern in `tests/`, name them `.test.sh`, and extend an existing script rather than inventing a new runner.