Skip to content

style: clear standards-check lint debt (8x SC2312, 1x MD041) - #34

Merged
twistedmelonman merged 2 commits into
mainfrom
claude/fix-standards-check-lint-7799158b
Sep 9, 2026
Merged

twistedmelonman merged 2 commits into
mainfrom
claude/fix-standards-check-lint-7799158b

Conversation

@twistedmelonman

Copy link
Copy Markdown
Member

Clears this repo's standards-check debt so the check passes. Wave 3 of the
fleet lint rollout (smartwatermelon/dev-env#100).

standards-check scans the whole repo on every PR, so these pre-existing
findings failed the check on any PR regardless of what it touched. Clearing
them in one shot is what makes the check green here.

What changed

shellcheck — 8x SC2312 in install.sh (lines 175, 277, 323, 393, 413,
520, 591, 603). Each is a command substitution whose exit status was already
discarded: $(...) inside [[ ]] or echo, and < <(...) process
substitution. Adding || true inside the substitution is SC2312's own
suggested alternative and adds no # shellcheck disable.

markdownlint — MD041 on LICENSE.md: added a # MIT License heading. The
text of the license is unchanged.

Comments at the four mapfile sites explaining why the guard is inert.

Why not the other SC2312 fix

The alternative — assign to a variable, then use it — was rejected because it
changes behavior twice over:

  • mapfile -t x <<< "$out" yields a 1-element array containing "" on
    empty input, where < <(cmd) yields an empty array. read_manifest and
    parse_mirrors_file can legitimately produce no output.
  • a bare assignment is subject to set -e, so a failing uname would exit
    instead of falling through to the || fatal guard at line 175.

On the "swallowed errors" reading

The local full-diff reviewer initially flagged these as a data-integrity
regression. They are not, and the second commit documents why in the source:

  • the callees cannot fail — parse_mirrors_file and read_manifest each
    already end in || true; get_desired_mirrors either prints or calls fatal
  • process substitution never propagates exit status under set -e regardless.
    Verified directly: set -euo pipefail; mapfile -t x < <(false; echo hi) does
    not abort.

The reviewer's underlying design concern is real but pre-existing, and is
filed separately as #33: read_manifest's inner || true conflates grep
exit 1 (no match) with exit 2 (unreadable file), so an unreadable manifest
reads as "nothing is managed". That one is load-bearing; these four are not.

Verification

  • standards/run-standards.sh --repo . — exit 1 before, exit 0 after,
    all six linters clean. Known-bad control run first.
  • bash -n install.sh passes; install.sh --help exits 0.
  • The touched uname/readlink/date guards and the empty-input mapfile
    case smoke-tested to behave identically.
  • shellcheck version parity confirmed: CI pins 0.11.0, local is 0.11.0.

https://claude.ai/code/session_01UaPoEix1iED8ENCZCa12jy

Claude Code Bot added 2 commits September 9, 2026 13:02
…41-clean

`standards-check` runs `shellcheck -S info` and markdownlint over the whole
repo, so these pre-existing findings fail the check on every PR regardless of
what the PR touches. Clearing them in one commit is what lets the check pass on
this PR (wave 3, dev-env#100).

shellcheck: 8x SC2312 in install.sh, at lines 175, 277, 323, 393, 413, 520,
591 and 603. Each is a command substitution whose exit status was already
discarded — `$(...)` inside `[[ ]]` or `echo`, and `< <(...)` process
substitution, none of which propagate status under `set -euo pipefail`.
Adding `|| true` inside the substitution documents that existing discard; it
is SC2312's own suggested alternative and adds no `# shellcheck disable`.

Deliberately NOT rewritten as a separate assignment (`out=$(cmd)` then use
`"$out"`), which is the other way to silence SC2312, because that would change
behavior in two ways:
  - `mapfile -t x <<< "$out"` yields a 1-element array containing "" on empty
    input, where `< <(cmd)` yields an empty array. read_manifest and
    parse_mirrors_file can legitimately produce no output.
  - a bare assignment is subject to `set -e`, so a failing `uname` would exit
    instead of falling through to the `|| fatal` guard at line 175.

Note parse_mirrors_file (349) and read_manifest (379) already end in `|| true`
and cannot fail, so the four mapfile sites were never masking a real error.

markdownlint: MD041 on LICENSE.md — added a `# MIT License` heading. Text of
the license is unchanged.

Verified: `standards/run-standards.sh --repo .` exits 1 before and 0 after
(all six linters clean); `bash -n install.sh` passes; `install.sh --help`
exits 0; the touched uname/readlink/date guards and the empty-input mapfile
case were smoke-tested to behave identically.

Claude-Session: https://claude.ai/code/session_01UaPoEix1iED8ENCZCa12jy
…sites

The previous commit added `|| true` inside four `mapfile -t x < <(...)`
substitutions to satisfy SC2312. Read at the call site alone, that pattern
looks like it suppresses a real error — the local full-diff reviewer read it
that way and flagged it as a data-integrity regression in do_uninstall.

It is not one, but nothing at the call site says so. These comments record the
two facts a reader needs:

  - the callee cannot fail. parse_mirrors_file and read_manifest each end in
    `|| true`; get_desired_mirrors either prints or calls fatal.
  - process substitution never propagates exit status under `set -e` anyway,
    so `< <(cmd)` and `< <(cmd || true)` are identical. Verified directly:
    `set -euo pipefail; mapfile -t x < <(false; echo hi)` does not abort.

No behavior change; comments only. `run-standards.sh --repo .` still exits 0.

Claude-Session: https://claude.ai/code/session_01UaPoEix1iED8ENCZCa12jy
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
All changes add `|| true` to command/process substitutions to satisfy ShellCheck SC2312, plus a cosmetic `# MIT License` heading fix (MD041). No behavioral changes: `|| true` in `$(uname -s || true)` only sets subshell exit code to 0 — stdout and the `== "Darwin"` comparison are unaffected. Process substitution exit codes don't propagate under `set -e` in Bash regardless, so the `mapfile` additions are correctly inert.

VERDICT: PASS

@twistedmelonman
twistedmelonman merged commit 5de3cd0 into main Sep 9, 2026
5 checks passed
@twistedmelonman
twistedmelonman deleted the claude/fix-standards-check-lint-7799158b branch September 9, 2026 20:56
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