fatalf called os.Exit(1), which does not run deferred functions, so
every defer db.Close() was dead on the fatal path: the SQLite WAL was
left uncheckpointed and the -wal/-shm sidecars were left for the
next process to recover. It also made those paths impossible to
exercise in-process.
What changed
fatalf is gone. runScan, runReport, runTrees, loadRecords
and resolveRoots return their errors, so the deferred db.Close() — and with it the WAL checkpoint — always runs. Message
text is unchanged; the fmt.Errorf wrappers use %w.
The single exit point is run(args []string, stderr io.Writer) int
in main.go, called as os.Exit(run(os.Args[1:], os.Stderr)).
Error classification, which is the trap here: cobra prints the error and the usage text for anything RunE returns, and main
previously mapped every root.Execute() error to exit 2. A runtime
failure is not a usage problem, so a runE adapter sets SilenceUsage/SilenceErrors on the subcommand and wraps its error
in fatalError. run reports a fatalError as sfdupes: <err> on
stderr and exits 1; everything else out of Execute is cobra's own
argument/flag/unknown-command error, already printed by cobra with
its usage text, and exits 2. Bare sfdupes still prints usage and
exits 2.
No behaviour change: exit 0 on success even with per-file warnings,
1 fatal, 2 usage, exactly as README §Error handling specifies, and
stdout still carries data only (README design goal 4).
Definition of done
runScan/runReport/runTrees return errors; the exit point is run in main.go — done.
db.Close() runs on success, per-file-warning and fatal paths —
done; nothing between the open and the return calls os.Exit any
more.
Exit codes unchanged — covered by tests and by a manual run of the
built binary (below).
A test triggers a fatal error after the database is open and
asserts no -wal is left — TestRunFatalAfterOpenClosesDatabase,
one subtest each for scan, report and trees.
make check green — plus make docker.
Verification
make check: green (tests 0.3s, whole suite well under the 20s
budget and the 30s timeout; coverage 64% to 86.9%). Local golangci-lint is v2.10.1, so this is the weaker gate (#24).
make docker: green — that is the authoritative one, with the
digest-pinned v2.12.2 lint stage and make check run as non-root.
It caught six goconst findings v2.10.1 did not; the subcommand
names are now the constants cmdScan/cmdReport/cmdTrees.
Manual run of the built binary, checking $? and the stdout/stderr
split for each case: bare sfdupes 2 (usage on stderr, stdout
empty), unknown subcommand 2, report x 2, nonexistent PATH
operand 1 with no usage text, report with no database 1, scan 0, report/trees 0 with only TSV on stdout. After the scan the
database directory holds db.sqlite alone — no -wal, no -shm.
New tests (main_test.go)
TestOpenDatabaseKeepsWALWhileOpen establishes the premise of the
rest: an open database does have a -wal sidecar, so its absence
afterwards is evidence of a close rather than a vacuous pass.
TestRunFatalAfterOpenClosesDatabase: a database that opens cleanly
and passes the schema-version check but has no files table, so the
first query fails with the database open. Asserts exit 1, no -wal/-shm, sfdupes: on stderr, no usage text, nothing on
stdout, and that the message is the one only a query on the open
database can produce.
TestRunMissingOperandIsFatalNotUsage, TestRunUsageErrors, TestRunHelpAndVersionSucceed, TestRunScanSucceedsDespiteWarnings
(unreadable file skipped, still exit 0), TestRunReportSucceeds/TestRunTreesSucceeds (exit 0, exact TSV on
stdout).
Out of scope
Deliberately untouched, per the tracker: #5 (signals), #6 (pool leak
on error), #8 (read-only database), #16 (CLI surface tests), #24
(local linter version).
`fatalf` called `os.Exit(1)`, which does not run deferred functions, so
every `defer db.Close()` was dead on the fatal path: the SQLite WAL was
left uncheckpointed and the `-wal`/`-shm` sidecars were left for the
next process to recover. It also made those paths impossible to
exercise in-process.
## What changed
- `fatalf` is gone. `runScan`, `runReport`, `runTrees`, `loadRecords`
and `resolveRoots` return their errors, so the deferred
`db.Close()` — and with it the WAL checkpoint — always runs. Message
text is unchanged; the `fmt.Errorf` wrappers use `%w`.
- The single exit point is `run(args []string, stderr io.Writer) int`
in `main.go`, called as `os.Exit(run(os.Args[1:], os.Stderr))`.
- Error classification, which is the trap here: cobra prints the error
*and* the usage text for anything `RunE` returns, and `main`
previously mapped every `root.Execute()` error to exit 2. A runtime
failure is not a usage problem, so a `runE` adapter sets
`SilenceUsage`/`SilenceErrors` on the subcommand and wraps its error
in `fatalError`. `run` reports a `fatalError` as `sfdupes: <err>` on
stderr and exits 1; everything else out of `Execute` is cobra's own
argument/flag/unknown-command error, already printed by cobra with
its usage text, and exits 2. Bare `sfdupes` still prints usage and
exits 2.
- No behaviour change: exit 0 on success even with per-file warnings,
1 fatal, 2 usage, exactly as README §Error handling specifies, and
stdout still carries data only (README design goal 4).
## Definition of done
1. `runScan`/`runReport`/`runTrees` return errors; the exit point is
`run` in `main.go` — done.
2. `db.Close()` runs on success, per-file-warning and fatal paths —
done; nothing between the open and the return calls `os.Exit` any
more.
3. Exit codes unchanged — covered by tests and by a manual run of the
built binary (below).
4. A test triggers a fatal error after the database is open and
asserts no `-wal` is left — `TestRunFatalAfterOpenClosesDatabase`,
one subtest each for `scan`, `report` and `trees`.
5. `make check` green — plus `make docker`.
## Verification
- `make check`: green (tests 0.3s, whole suite well under the 20s
budget and the 30s timeout; coverage 64% to 86.9%). Local
`golangci-lint` is v2.10.1, so this is the weaker gate (#24).
- `make docker`: green — that is the authoritative one, with the
digest-pinned v2.12.2 lint stage and `make check` run as non-root.
It caught six `goconst` findings v2.10.1 did not; the subcommand
names are now the constants `cmdScan`/`cmdReport`/`cmdTrees`.
- Manual run of the built binary, checking `$?` and the stdout/stderr
split for each case: bare `sfdupes` 2 (usage on stderr, stdout
empty), unknown subcommand 2, `report x` 2, nonexistent `PATH`
operand 1 with no usage text, `report` with no database 1, `scan` 0,
`report`/`trees` 0 with only TSV on stdout. After the scan the
database directory holds `db.sqlite` alone — no `-wal`, no `-shm`.
## New tests (`main_test.go`)
- `TestOpenDatabaseKeepsWALWhileOpen` establishes the premise of the
rest: an open database *does* have a `-wal` sidecar, so its absence
afterwards is evidence of a close rather than a vacuous pass.
- `TestRunFatalAfterOpenClosesDatabase`: a database that opens cleanly
and passes the schema-version check but has no `files` table, so the
first query fails with the database open. Asserts exit 1, no
`-wal`/`-shm`, `sfdupes: ` on stderr, no usage text, nothing on
stdout, and that the message is the one only a query on the open
database can produce.
- `TestRunMissingOperandIsFatalNotUsage`, `TestRunUsageErrors`,
`TestRunHelpAndVersionSucceed`, `TestRunScanSucceedsDespiteWarnings`
(unreadable file skipped, still exit 0),
`TestRunReportSucceeds`/`TestRunTreesSucceeds` (exit 0, exact TSV on
stdout).
## Out of scope
Deliberately untouched, per the tracker: #5 (signals), #6 (pool leak
on error), #8 (read-only database), #16 (CLI surface tests), #24
(local linter version).
fatalf called os.Exit(1), which does not run deferred functions, so
every defer db.Close() was dead on the fatal path: the SQLite WAL was
left uncheckpointed and the -wal/-shm sidecars were left for the next
process to recover. It also made those paths impossible to exercise
in-process.
fatalf is gone. runScan, runReport, runTrees, loadRecords and
resolveRoots return their errors, so the deferred close always runs,
and the only exit point is run() in main.go.
Mapping errors to exit codes needs care: cobra prints the error and
the command's usage text for anything RunE returns, and main mapped
every Execute() error to exit 2. A runtime failure is not a usage
problem, so the runE adapter silences both for the subcommands and
marks their errors fatalError; run() reports a fatalError as
"sfdupes: ..." on stderr and exits 1, and leaves everything else --
cobra's own argument, flag and unknown-command errors, which cobra has
already reported with its usage text -- on exit 2. The bare
"sfdupes" invocation still prints usage and exits 2.
Exit codes and message text are unchanged: 0 on success even with
per-file warnings, 1 fatal, 2 usage, per README section "Error
handling and exit codes". Everything on stdout is still data only.
main_test.go drives the CLI in-process and covers all three: a fatal
error raised after the database is open (a database with no files
table) closes it and leaves no -wal or -shm behind for scan, report
and trees; a nonexistent PATH operand is fatal, not usage, and prints
no usage text; the usage errors still exit 2; and a scan that skipped
an unreadable file still exits 0.
clawbot
self-assigned this 2026-08-09 04:27:43 +02:00
make check (local, golangci-lint v2.10.1): green. ok sneak.berlin/go/sfdupes 0.3s coverage: 86.9% of statements (was
64%); the suite runs in well under a second, so it stays far inside
the 20-second budget and the 30-second timeout.
make docker: green. This is the authoritative gate — the
digest-pinned v2.12.2 lint stage plus make check as the
unprivileged builder user. It rejected the first version of the
branch with six goconst findings that v2.10.1 (issue #24) did not
report, in main.go and main_test.go; fixed by naming the
subcommands cmdScan/cmdReport/cmdTrees and the Usage: marker
in the tests, then re-run to completion.
Behavioural check of the built binary (make build, real process,
real $?, stdout and stderr captured separately):
invocation
exit
stdout
stderr
sfdupes
2
empty
usage
sfdupes bogus
2
empty
Error: unknown command + usage
sfdupes report x
2
empty
Error: unknown command "x" ... + usage
sfdupes scan /nonexistent
1
empty
sfdupes: lstat ...: no such file or directory, no usage
sfdupes report (no database)
1
empty
sfdupes: ...: no database (run "sfdupes scan" first, ...), no usage
sfdupes scan TREE
0
empty
progress + summary
sfdupes report
0
TSV only
summary
sfdupes trees
0
TSV only
summary
After the scan the database directory contained db.sqlite and
nothing else — the WAL was checkpointed and the sidecars removed on
close.
The subtle part, called out for the reviewer: the exit-1 rows
above are exactly the regression risk in this refactor. Cobra prints
the error and the usage text for anything RunE returns, and the
old main.go mapped every root.Execute() error to exit 2, so a
naive RunE conversion turns every runtime failure into an exit-2
usage error with a usage dump. runE in main.go silences both on
the subcommand and tags the error fatalError; run exits 1 for
those and 2 for everything else cobra produces. TestRunMissingOperandIsFatalNotUsage and the assertFatalOutput
helper (no Usage: on stderr, nothing on stdout) lock that down, and TestRunUsageErrors locks down the other direction.
One thing noticed and deliberately left alone (not filed, not
fixed): with stdout closed outright (sfdupes report >&-) the run
still exits 0 rather than failing on the write, because fd 1 gets
handed to the next file the process opens. That behaviour is identical
before and after this branch and is unrelated to the database
lifetime, so it is out of scope here; say the word and I will file it.
Built and verified as follows.
**Gates**
- `make check` (local, `golangci-lint` v2.10.1): green. `ok
sneak.berlin/go/sfdupes 0.3s coverage: 86.9% of statements` (was
64%); the suite runs in well under a second, so it stays far inside
the 20-second budget and the 30-second timeout.
- `make docker`: green. This is the authoritative gate — the
digest-pinned v2.12.2 lint stage plus `make check` as the
unprivileged `builder` user. It rejected the first version of the
branch with six `goconst` findings that v2.10.1 (issue #24) did not
report, in `main.go` and `main_test.go`; fixed by naming the
subcommands `cmdScan`/`cmdReport`/`cmdTrees` and the `Usage:` marker
in the tests, then re-run to completion.
**Behavioural check of the built binary** (`make build`, real process,
real `$?`, stdout and stderr captured separately):
| invocation | exit | stdout | stderr |
| --- | --- | --- | --- |
| `sfdupes` | 2 | empty | usage |
| `sfdupes bogus` | 2 | empty | `Error: unknown command` + usage |
| `sfdupes report x` | 2 | empty | `Error: unknown command "x" ...` + usage |
| `sfdupes scan /nonexistent` | 1 | empty | `sfdupes: lstat ...: no such file or directory`, no usage |
| `sfdupes report` (no database) | 1 | empty | `sfdupes: ...: no database (run "sfdupes scan" first, ...)`, no usage |
| `sfdupes scan TREE` | 0 | empty | progress + summary |
| `sfdupes report` | 0 | TSV only | summary |
| `sfdupes trees` | 0 | TSV only | summary |
After the scan the database directory contained `db.sqlite` and
nothing else — the WAL was checkpointed and the sidecars removed on
close.
**The subtle part**, called out for the reviewer: the exit-1 rows
above are exactly the regression risk in this refactor. Cobra prints
the error *and* the usage text for anything `RunE` returns, and the
old `main.go` mapped every `root.Execute()` error to exit 2, so a
naive `RunE` conversion turns every runtime failure into an exit-2
usage error with a usage dump. `runE` in `main.go` silences both on
the subcommand and tags the error `fatalError`; `run` exits 1 for
those and 2 for everything else cobra produces.
`TestRunMissingOperandIsFatalNotUsage` and the `assertFatalOutput`
helper (no `Usage:` on stderr, nothing on stdout) lock that down, and
`TestRunUsageErrors` locks down the other direction.
**One thing noticed and deliberately left alone** (not filed, not
fixed): with stdout closed outright (`sfdupes report >&-`) the run
still exits 0 rather than failing on the write, because fd 1 gets
handed to the next file the process opens. That behaviour is identical
before and after this branch and is unrelated to the database
lifetime, so it is out of scope here; say the word and I will file it.
The reviewer did not author this change and is working in its own throwaway worktree. It has been pointed at the specific trap in this refactor: cobra's RunE prints usage alongside the error by default, and the old main mapped every root.Execute() error to exit 2, so the easy way to get this wrong is to silently reclassify a fatal (exit 1) as a usage error (exit 2), or to start dumping usage text on ordinary runtime failures. It has been asked to verify every documented exit path by running the built binary and checking real $?, and to try reverting the production change while keeping the new tests to confirm they actually go red rather than passing vacuously.
Worth recording from the implementation, because it is the clearest evidence yet for #24: make check passed locally, then make dockerrejected the first version with six goconst findings. The local linter is v2.10.1; the repo pins v2.12.2. A green local make check in this repo is currently not evidence of anything, and until #24 is fixed make docker is the only gate that counts. The reviewer has been told to treat it that way.
Coverage moved 64% to 86.9% on this branch.
One thing found during implementation and deliberately kept out of this PR, now filed as #30: sfdupes report >&- exits 0 rather than failing the write, which README §Error handling lists as an exit-1 condition. It predates this branch and is unrelated to database lifetime, so excluding it was the right call. The wider version of that bug — unchecked writes meaning report | head and a full disk both silently truncate the report — is the part that actually matters.
Manager note — independent adversarial review dispatched.
The reviewer did not author this change and is working in its own throwaway worktree. It has been pointed at the specific trap in this refactor: cobra's `RunE` prints usage alongside the error by default, and the old `main` mapped every `root.Execute()` error to exit 2, so the easy way to get this wrong is to silently reclassify a fatal (exit 1) as a usage error (exit 2), or to start dumping usage text on ordinary runtime failures. It has been asked to verify every documented exit path by running the built binary and checking real `$?`, and to try reverting the production change while keeping the new tests to confirm they actually go red rather than passing vacuously.
Worth recording from the implementation, because it is the clearest evidence yet for #24: `make check` passed locally, then `make docker` **rejected** the first version with six `goconst` findings. The local linter is v2.10.1; the repo pins v2.12.2. A green local `make check` in this repo is currently not evidence of anything, and until #24 is fixed `make docker` is the only gate that counts. The reviewer has been told to treat it that way.
Coverage moved 64% to 86.9% on this branch.
One thing found during implementation and deliberately kept out of this PR, now filed as #30: `sfdupes report >&-` exits 0 rather than failing the write, which README §Error handling lists as an exit-1 condition. It predates this branch and is unrelated to database lifetime, so excluding it was the right call. The wider version of that bug — unchecked writes meaning `report | head` and a full disk both silently truncate the report — is the part that actually matters.
Review of PR #29 (head 73841c9, base main at ce6d29d)
Verdict: PASS — no blocking findings. Six non-blocking notes below.
Reviewed independently against issue #4, README.md (§Error handling
and exit codes, §Design goal 4, §scan mode, §report mode, §trees
mode), REPO_POLICIES.md and TODO.md. All verification was done in
a throwaway worktree at the PR head; nothing in the shared checkout
was touched.
runScan/runReport/runTrees return errors; single exit point
is run in main.go — met. grep -rn "os.Exit" *.go finds
exactly one call site, main.go:55.
db.Close() guaranteed on success, per-file-warning and fatal
paths — met, verified empirically (below).
Exit codes unchanged — met, verified empirically against a binary
built from ce6d29d.
A test triggers a fatal error after the database is open and
asserts no -wal is left — met, and the test is not vacuous
(mutation-tested, below).
make check green — met, and make docker green too.
Empirical exit-code and stream check
Built binaries from ce6d29d (base) and 73841c9 (head) and ran 15
invocations through each, capturing real $? with stdout and stderr
separated:
invocation
exit
stdout
usage text on stderr
bare sfdupes
2
empty
yes
bogus
2
empty
yes
scan (no operand)
2
empty
yes
report x
2
empty
yes
trees x
2
empty
yes
scan --nope /tmp
2
empty
yes
completion
2
empty
yes
scan /nonexistent-xyz
1
empty
no
report (no database)
1
empty
no
trees (no database)
1
empty
no
report with stdout on /dev/full
1
n/a
no
trees with stdout on /dev/full
1
n/a
no
--help / -h / help / scan --help
0
empty
yes (stderr)
--version
0
empty
no
scan TREE / report / trees (success)
0
empty / TSV / TSV
no
Base and head agree on every exit code, and the stderr bytes hash
identically for every case except two: --version (different git describe string, expected) and bare sfdupes (see note 2).
No fatal became a usage error, no usage became a fatal, nothing is
swallowed to 0. Usage-text leakage is correct in both directions.
Stream separation holds: stdout is empty for every failing, usage,
help and scan invocation, and carries only TSV for report/trees.
The database actually closes
Built a database with PRAGMA journal_mode=wal; PRAGMA user_version=1
and no files table, then ran each subcommand against it:
base ce6d29d: db.sqlite, db.sqlite-wal, db.sqlite-shm left
behind for scan, report and trees.
head 73841c9: db.sqlite alone for all three.
Error text is byte-identical between base and head on all three paths
(sfdupes: database ...: read records: SQL logic error: no such table: files (1), and the update database ... variant for scan). After a
successful scan the database directory holds db.sqlite alone.
The new tests are not vacuous
Mutation-tested: in a scratch copy of the head tree I reintroduced the
defect the PR fixes — dropped the deferred db.Close() in runScan
and loadRecords and closed only on the success return, so the close
is skipped exactly on the fatal path — and kept main_test.go
unchanged. make test goes red:
--- FAIL: TestRunFatalAfterOpenClosesDatabase/scan
main_test.go:172: ...db.sqlite-wal still present: the database was not closed
main_test.go:172: ...db.sqlite-shm still present: the database was not closed
--- FAIL: TestRunFatalAfterOpenClosesDatabase/report (same two)
--- FAIL: TestRunFatalAfterOpenClosesDatabase/trees (same two)
All three subtests fail on the sidecar assertion, which is the
property under test. TestOpenDatabaseKeepsWALWhileOpen does its job
as the premise: it asserts a -wal exists beside an open database,
so "no -wal afterwards" is evidence of a close and not of a database
that never had a WAL. The fatal-path subtests also assert the message
is no such table: files, which only a query against the already-open
database can produce, so they cannot pass by failing before the open.
Gates
make check: green. ok sneak.berlin/go/sfdupes 0.176s coverage: 86.9% of statements, 0 issues, fmt-check clean.
make docker: green, exit 0. Lint stage (digest-pinned v2.12.2) 0 issues after make fmt-check and make lint; builder stage make check0 issues at 86.9% coverage, run as the unprivileged builder user; image exported.
CI on 73841c9: check / check (push) success.
Mergeable: the PR base ce6d29d is current origin/main; the
branch is a single commit on top of it, git merge-tree reports
zero conflicts. Fast-forward.
The goconst constants are legitimate, not churn
Verified rather than taken on trust. Restored the string literals
(removed the cmdScan/cmdReport/cmdTrees block, put "scan [--workers N] [-x] PATH...", "report", "trees" back in the Use:
fields, and the literals back in main_test.go) in a scratch copy and
re-ran make docker: it fails with exit 2 and 5 goconst findings at
v2.12.2 —
main.go:127:10: string `report` has 5 occurrences, make it a constant (goconst)
main.go:136:10: string `trees` has 5 occurrences, make it a constant (goconst)
main_test.go:149:3: string `scan` has 7 occurrences, make it a constant (goconst)
main_test.go:150:3: string `report` has 5 occurrences, make it a constant (goconst)
main_test.go:151:3: string `trees` has 5 occurrences, make it a constant (goconst)
Two of the five are in production code, so this is not purely a
test-driven accommodation. The constants are the minimal fix and
preferable to nolint directives.
Scope
Diff is six files: main.go, main_test.go, report.go, scan.go, trees.go, TODO.md. Nothing from #5 (signals), #6 (pool leak on
error), #8 (read-only database) or #16 (CLI surface tests) is present.
No unrelated refactoring. Dockerfile is untouched: the deliberate
non-root builder user and the comment explaining why (root would
bypass the chmod(0) the permission tests rely on) are intact, and
the permission-dependent assertions in scan_test.go:806-814
(st.skipped != 2 fails under root) still run in the docker gate.
Hygiene
No mention of any AI assistant or vendor anywhere in the diff, the
commit message, or the PR body; no attribution trailers. Author and
committer are sneak <sneak@sneak.berlin>.
Commit title ends with (closes #4); body wraps at 70 columns.
TODO.md has an accurate Completed Steps entry at the top, in the
same commit, in the established style.
No non-inclusive terminology introduced.
make fmt-check clean.
Non-blocking findings
main_test.go:185 — misleading test name. TestRunMissingOperandIsFatalNotUsage tests a nonexistentPATH
operand, which README §scan mode calls a fatal error (exit 1). But
"missing operand" is README's phrase for scan with no operand,
which is a usage error (exit 2) and is covered separately as the "scan without paths" case in TestRunUsageErrors. The name
asserts the opposite of the spec for the case it appears to name.
Acceptable: TestRunNonexistentPathIsFatalNotUsage.
Bare sfdupes gains one trailing newline on stderr. The old
code called cmd.Usage(); cobra now prints the usage for the
returned errNoSubcommand via c.Println(cmd.UsageString()),
which appends a newline. diff of base vs head stderr for the bare
invocation is exactly one added blank line at EOF. Exit code, text
and stream are unchanged and README specifies nothing here, so this
is cosmetic — but the implementation-plan comment on #4 claims
"Cobra's own usage output is therefore byte-for-byte what it is
today", and that claim is not quite true.
run's injected stderr is only half-honoured.run(args []string, stderr io.Writer) routes cobra's error and usage output
to the writer, but runScan, runReport and runTrees still
write their warnings, progress and summaries to the package-global os.Stderr, and the reports write to os.Stdout. The tests
therefore have to monkey-patch os.Stdout (captureStdout), and scanFixture/TestRunScanSucceedsDespiteWarnings cannot assert
that the warning actually happened — it only asserts exit 0. Under
a root test runner the chmod(0) is a no-op and that test degrades
silently into an ordinary clean scan while still passing. The
repo-wide root guard survives via scan_test.go:806-814, so this
is not a hole in the suite, but the new test does not carry its own
weight; asserting 1 skipped in the summary would fix it. Probably
best folded into #16, which owns threading the writers through.
runE is opt-in, and misuse fails silently in the wrong
direction. A future subcommand wired with a bare RunE (or Run) would not be wrapped in fatalError, so run's default
branch would classify its runtime failure as exit 2 and cobra would
dump the usage text — the exact regression this PR exists to
prevent. Worth a note on newRootCommand, or a test that walks root.Commands() and asserts every leaf command's RunE is a runE product.
errNoSubcommand's message is dead text. The root RunE sets cmd.SilenceErrors = true before returning it, so "no subcommand" is never printed anywhere. Harmless, but the sentinel
exists only to be non-nil; the doc comment could say so outright
(it half does).
Pre-existing, not caused by this PR: the docker lint stage
emits The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2. Worth a tracker
item alongside #24 rather than a change here.
## Review of PR #29 (head `73841c9`, base `main` at `ce6d29d`)
**Verdict: PASS** — no blocking findings. Six non-blocking notes below.
Reviewed independently against issue #4, `README.md` (§Error handling
and exit codes, §Design goal 4, §scan mode, §report mode, §trees
mode), `REPO_POLICIES.md` and `TODO.md`. All verification was done in
a throwaway worktree at the PR head; nothing in the shared checkout
was touched.
### Definition of done (issue #4)
1. `runScan`/`runReport`/`runTrees` return errors; single exit point
is `run` in `main.go` — met. `grep -rn "os.Exit" *.go` finds
exactly one call site, `main.go:55`.
2. `db.Close()` guaranteed on success, per-file-warning and fatal
paths — met, verified empirically (below).
3. Exit codes unchanged — met, verified empirically against a binary
built from `ce6d29d`.
4. A test triggers a fatal error after the database is open and
asserts no `-wal` is left — met, and the test is not vacuous
(mutation-tested, below).
5. `make check` green — met, and `make docker` green too.
### Empirical exit-code and stream check
Built binaries from `ce6d29d` (base) and `73841c9` (head) and ran 15
invocations through each, capturing real `$?` with stdout and stderr
separated:
| invocation | exit | stdout | usage text on stderr |
| --- | --- | --- | --- |
| bare `sfdupes` | 2 | empty | yes |
| `bogus` | 2 | empty | yes |
| `scan` (no operand) | 2 | empty | yes |
| `report x` | 2 | empty | yes |
| `trees x` | 2 | empty | yes |
| `scan --nope /tmp` | 2 | empty | yes |
| `completion` | 2 | empty | yes |
| `scan /nonexistent-xyz` | 1 | empty | **no** |
| `report` (no database) | 1 | empty | **no** |
| `trees` (no database) | 1 | empty | **no** |
| `report` with stdout on `/dev/full` | 1 | n/a | **no** |
| `trees` with stdout on `/dev/full` | 1 | n/a | **no** |
| `--help` / `-h` / `help` / `scan --help` | 0 | empty | yes (stderr) |
| `--version` | 0 | empty | no |
| `scan TREE` / `report` / `trees` (success) | 0 | empty / TSV / TSV | no |
Base and head agree on every exit code, and the stderr bytes hash
identically for every case except two: `--version` (different
`git describe` string, expected) and bare `sfdupes` (see note 2).
No fatal became a usage error, no usage became a fatal, nothing is
swallowed to 0. Usage-text leakage is correct in both directions.
Stream separation holds: stdout is empty for every failing, usage,
help and `scan` invocation, and carries only TSV for `report`/`trees`.
### The database actually closes
Built a database with `PRAGMA journal_mode=wal; PRAGMA user_version=1`
and no `files` table, then ran each subcommand against it:
- base `ce6d29d`: `db.sqlite`, `db.sqlite-wal`, `db.sqlite-shm` left
behind for `scan`, `report` and `trees`.
- head `73841c9`: `db.sqlite` alone for all three.
Error text is byte-identical between base and head on all three paths
(`sfdupes: database ...: read records: SQL logic error: no such table:
files (1)`, and the `update database ...` variant for `scan`). After a
successful scan the database directory holds `db.sqlite` alone.
### The new tests are not vacuous
Mutation-tested: in a scratch copy of the head tree I reintroduced the
defect the PR fixes — dropped the deferred `db.Close()` in `runScan`
and `loadRecords` and closed only on the success return, so the close
is skipped exactly on the fatal path — and kept `main_test.go`
unchanged. `make test` goes red:
--- FAIL: TestRunFatalAfterOpenClosesDatabase/scan
main_test.go:172: ...db.sqlite-wal still present: the database was not closed
main_test.go:172: ...db.sqlite-shm still present: the database was not closed
--- FAIL: TestRunFatalAfterOpenClosesDatabase/report (same two)
--- FAIL: TestRunFatalAfterOpenClosesDatabase/trees (same two)
All three subtests fail on the sidecar assertion, which is the
property under test. `TestOpenDatabaseKeepsWALWhileOpen` does its job
as the premise: it asserts a `-wal` exists beside an *open* database,
so "no `-wal` afterwards" is evidence of a close and not of a database
that never had a WAL. The fatal-path subtests also assert the message
is `no such table: files`, which only a query against the already-open
database can produce, so they cannot pass by failing before the open.
### Gates
- `make check`: green. `ok sneak.berlin/go/sfdupes 0.176s coverage:
86.9% of statements`, `0 issues`, `fmt-check` clean.
- `make docker`: green, exit 0. Lint stage (digest-pinned v2.12.2)
`0 issues` after `make fmt-check` and `make lint`; builder stage
`make check` `0 issues` at 86.9% coverage, run as the unprivileged
`builder` user; image exported.
- CI on `73841c9`: `check / check (push)` success.
- Mergeable: the PR base `ce6d29d` is current `origin/main`; the
branch is a single commit on top of it, `git merge-tree` reports
zero conflicts. Fast-forward.
### The `goconst` constants are legitimate, not churn
Verified rather than taken on trust. Restored the string literals
(removed the `cmdScan`/`cmdReport`/`cmdTrees` block, put `"scan
[--workers N] [-x] PATH..."`, `"report"`, `"trees"` back in the `Use:`
fields, and the literals back in `main_test.go`) in a scratch copy and
re-ran `make docker`: it fails with exit 2 and 5 `goconst` findings at
v2.12.2 —
main.go:127:10: string `report` has 5 occurrences, make it a constant (goconst)
main.go:136:10: string `trees` has 5 occurrences, make it a constant (goconst)
main_test.go:149:3: string `scan` has 7 occurrences, make it a constant (goconst)
main_test.go:150:3: string `report` has 5 occurrences, make it a constant (goconst)
main_test.go:151:3: string `trees` has 5 occurrences, make it a constant (goconst)
Two of the five are in production code, so this is not purely a
test-driven accommodation. The constants are the minimal fix and
preferable to `nolint` directives.
### Scope
Diff is six files: `main.go`, `main_test.go`, `report.go`, `scan.go`,
`trees.go`, `TODO.md`. Nothing from #5 (signals), #6 (pool leak on
error), #8 (read-only database) or #16 (CLI surface tests) is present.
No unrelated refactoring. `Dockerfile` is untouched: the deliberate
non-root `builder` user and the comment explaining why (root would
bypass the `chmod(0)` the permission tests rely on) are intact, and
the permission-dependent assertions in `scan_test.go:806-814`
(`st.skipped != 2` fails under root) still run in the docker gate.
### Hygiene
- No mention of any AI assistant or vendor anywhere in the diff, the
commit message, or the PR body; no attribution trailers. Author and
committer are `sneak <sneak@sneak.berlin>`.
- Commit title ends with ` (closes #4)`; body wraps at 70 columns.
- `TODO.md` has an accurate Completed Steps entry at the top, in the
same commit, in the established style.
- No non-inclusive terminology introduced.
- `make fmt-check` clean.
---
## Non-blocking findings
1. **`main_test.go:185` — misleading test name.**
`TestRunMissingOperandIsFatalNotUsage` tests a *nonexistent* `PATH`
operand, which README §scan mode calls a fatal error (exit 1). But
"missing operand" is README's phrase for `scan` with *no* operand,
which is a usage error (exit 2) and is covered separately as the
`"scan without paths"` case in `TestRunUsageErrors`. The name
asserts the opposite of the spec for the case it appears to name.
Acceptable: `TestRunNonexistentPathIsFatalNotUsage`.
2. **Bare `sfdupes` gains one trailing newline on stderr.** The old
code called `cmd.Usage()`; cobra now prints the usage for the
returned `errNoSubcommand` via `c.Println(cmd.UsageString())`,
which appends a newline. `diff` of base vs head stderr for the bare
invocation is exactly one added blank line at EOF. Exit code, text
and stream are unchanged and README specifies nothing here, so this
is cosmetic — but the implementation-plan comment on #4 claims
"Cobra's own usage output is therefore byte-for-byte what it is
today", and that claim is not quite true.
3. **`run`'s injected `stderr` is only half-honoured.** `run(args
[]string, stderr io.Writer)` routes cobra's error and usage output
to the writer, but `runScan`, `runReport` and `runTrees` still
write their warnings, progress and summaries to the package-global
`os.Stderr`, and the reports write to `os.Stdout`. The tests
therefore have to monkey-patch `os.Stdout` (`captureStdout`), and
`scanFixture`/`TestRunScanSucceedsDespiteWarnings` cannot assert
that the warning actually happened — it only asserts exit 0. Under
a root test runner the `chmod(0)` is a no-op and that test degrades
silently into an ordinary clean scan while still passing. The
repo-wide root guard survives via `scan_test.go:806-814`, so this
is not a hole in the suite, but the new test does not carry its own
weight; asserting `1 skipped` in the summary would fix it. Probably
best folded into #16, which owns threading the writers through.
4. **`runE` is opt-in, and misuse fails silently in the wrong
direction.** A future subcommand wired with a bare `RunE` (or
`Run`) would not be wrapped in `fatalError`, so `run`'s `default`
branch would classify its runtime failure as exit 2 and cobra would
dump the usage text — the exact regression this PR exists to
prevent. Worth a note on `newRootCommand`, or a test that walks
`root.Commands()` and asserts every leaf command's `RunE` is a
`runE` product.
5. **`errNoSubcommand`'s message is dead text.** The root `RunE` sets
`cmd.SilenceErrors = true` before returning it, so `"no
subcommand"` is never printed anywhere. Harmless, but the sentinel
exists only to be non-nil; the doc comment could say so outright
(it half does).
6. **Pre-existing, not caused by this PR:** the docker lint stage
emits `The linter 'gomodguard' is deprecated (since v2.12.0) due
to: new major version. Replaced by gomodguard_v2.` Worth a tracker
item alongside #24 rather than a change here.
Review verdict PASS, no blocking findings, so this landed via a non-fast-forward merge commit. Branch db-close-on-fatal deleted; origin carries only main. Not assigned to sneak — main is unprotected on this repo, so the manager merges once the gate passes.
Post-merge verification on main:
make check green — tests ok in 0.406s, coverage 86.9% (up from 64%), 0 issues. from the linter.
Working tree clean, one worktree, no leftover refs.
This was a strong review. Rather than reading the diff and reasoning about it, it built binaries from both ce6d29d and 73841c9, ran 15 invocations through each with separated streams, and hashed the stderr output to compare — establishing that exit codes and messages are identical except for two explainable differences. It then mutation-tested the new assertions: it reintroduced the original defect in a scratch copy with main_test.go untouched, and confirmed all three TestRunFatalAfterOpenClosesDatabase subtests go red. That is the difference between "the tests pass" and "the tests would catch a regression", and it is the check most reviews skip.
It also independently confirmed the goconst constants were forced rather than churn — restoring the string literals and re-running make docker fails with five findings at v2.12.2, two of them in main.go itself.
Non-blocking findings and dispositions:
Test named TestRunMissingOperandIsFatalNotUsage actually covers a nonexistent operand, while "missing operand" is README's phrase for scan with no operand at all. Misleading name; folded into #16.
run threads its writers only into cobra — subcommands still write to the package-global os.Stdout/os.Stderr, so tests must monkey-patch. Folded into #16, where it properly belongs.
runE is opt-in, so a future subcommand wired with a bare RunE would reintroduce exactly the misclassification this PR fixes. Folded into #16 as a guard test.
Bare sfdupes gains one trailing newline on stderr; cosmetic, but it makes the plan comment's "byte-for-byte identical" claim slightly overstated. No action.
errNoSubcommand's message is never printed because the root sets SilenceErrors. Harmless dead string. No action.
gomodguard deprecated in favour of gomodguard_v2 — already tracked as #26 and assigned to sneak, since the fix belongs in the canonical config.
Next unit is #6, the hash-phase worker-pool leak. It is the natural follow-on: now that hashPhase errors propagate to a returning runScan instead of dying via os.Exit, the abandoned feeder goroutine and blocked workers are a real leak rather than a theoretical one.
Manager note — merged as `2a055c0`.
Review verdict PASS, no blocking findings, so this landed via a non-fast-forward merge commit. Branch `db-close-on-fatal` deleted; `origin` carries only `main`. Not assigned to `sneak` — `main` is unprotected on this repo, so the manager merges once the gate passes.
Post-merge verification on `main`:
- `make check` green — tests `ok` in 0.406s, coverage 86.9% (up from 64%), `0 issues.` from the linter.
- Working tree clean, one worktree, no leftover refs.
- #4 closed automatically by the merge title.
This was a strong review. Rather than reading the diff and reasoning about it, it built binaries from both `ce6d29d` and `73841c9`, ran 15 invocations through each with separated streams, and hashed the stderr output to compare — establishing that exit codes and messages are identical except for two explainable differences. It then mutation-tested the new assertions: it reintroduced the original defect in a scratch copy with `main_test.go` untouched, and confirmed all three `TestRunFatalAfterOpenClosesDatabase` subtests go red. That is the difference between "the tests pass" and "the tests would catch a regression", and it is the check most reviews skip.
It also independently confirmed the `goconst` constants were forced rather than churn — restoring the string literals and re-running `make docker` fails with five findings at v2.12.2, two of them in `main.go` itself.
Non-blocking findings and dispositions:
- Test named `TestRunMissingOperandIsFatalNotUsage` actually covers a *nonexistent* operand, while "missing operand" is README's phrase for `scan` with no operand at all. Misleading name; folded into #16.
- `run` threads its writers only into cobra — subcommands still write to the package-global `os.Stdout`/`os.Stderr`, so tests must monkey-patch. Folded into #16, where it properly belongs.
- `runE` is opt-in, so a future subcommand wired with a bare `RunE` would reintroduce exactly the misclassification this PR fixes. Folded into #16 as a guard test.
- Bare `sfdupes` gains one trailing newline on stderr; cosmetic, but it makes the plan comment's "byte-for-byte identical" claim slightly overstated. No action.
- `errNoSubcommand`'s message is never printed because the root sets `SilenceErrors`. Harmless dead string. No action.
- `gomodguard` deprecated in favour of `gomodguard_v2` — already tracked as #26 and assigned to `sneak`, since the fix belongs in the canonical config.
Next unit is #6, the hash-phase worker-pool leak. It is the natural follow-on: now that `hashPhase` errors propagate to a returning `runScan` instead of dying via `os.Exit`, the abandoned feeder goroutine and blocked workers are a real leak rather than a theoretical one.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
fatalfcalledos.Exit(1), which does not run deferred functions, soevery
defer db.Close()was dead on the fatal path: the SQLite WAL wasleft uncheckpointed and the
-wal/-shmsidecars were left for thenext process to recover. It also made those paths impossible to
exercise in-process.
What changed
fatalfis gone.runScan,runReport,runTrees,loadRecordsand
resolveRootsreturn their errors, so the deferreddb.Close()— and with it the WAL checkpoint — always runs. Messagetext is unchanged; the
fmt.Errorfwrappers use%w.run(args []string, stderr io.Writer) intin
main.go, called asos.Exit(run(os.Args[1:], os.Stderr)).and the usage text for anything
RunEreturns, andmainpreviously mapped every
root.Execute()error to exit 2. A runtimefailure is not a usage problem, so a
runEadapter setsSilenceUsage/SilenceErrorson the subcommand and wraps its errorin
fatalError.runreports afatalErrorassfdupes: <err>onstderr and exits 1; everything else out of
Executeis cobra's ownargument/flag/unknown-command error, already printed by cobra with
its usage text, and exits 2. Bare
sfdupesstill prints usage andexits 2.
1 fatal, 2 usage, exactly as README §Error handling specifies, and
stdout still carries data only (README design goal 4).
Definition of done
runScan/runReport/runTreesreturn errors; the exit point isruninmain.go— done.db.Close()runs on success, per-file-warning and fatal paths —done; nothing between the open and the return calls
os.Exitanymore.
built binary (below).
asserts no
-walis left —TestRunFatalAfterOpenClosesDatabase,one subtest each for
scan,reportandtrees.make checkgreen — plusmake docker.Verification
make check: green (tests 0.3s, whole suite well under the 20sbudget and the 30s timeout; coverage 64% to 86.9%). Local
golangci-lintis v2.10.1, so this is the weaker gate (#24).make docker: green — that is the authoritative one, with thedigest-pinned v2.12.2 lint stage and
make checkrun as non-root.It caught six
goconstfindings v2.10.1 did not; the subcommandnames are now the constants
cmdScan/cmdReport/cmdTrees.$?and the stdout/stderrsplit for each case: bare
sfdupes2 (usage on stderr, stdoutempty), unknown subcommand 2,
report x2, nonexistentPATHoperand 1 with no usage text,
reportwith no database 1,scan0,report/trees0 with only TSV on stdout. After the scan thedatabase directory holds
db.sqlitealone — no-wal, no-shm.New tests (
main_test.go)TestOpenDatabaseKeepsWALWhileOpenestablishes the premise of therest: an open database does have a
-walsidecar, so its absenceafterwards is evidence of a close rather than a vacuous pass.
TestRunFatalAfterOpenClosesDatabase: a database that opens cleanlyand passes the schema-version check but has no
filestable, so thefirst query fails with the database open. Asserts exit 1, no
-wal/-shm,sfdupes:on stderr, no usage text, nothing onstdout, and that the message is the one only a query on the open
database can produce.
TestRunMissingOperandIsFatalNotUsage,TestRunUsageErrors,TestRunHelpAndVersionSucceed,TestRunScanSucceedsDespiteWarnings(unreadable file skipped, still exit 0),
TestRunReportSucceeds/TestRunTreesSucceeds(exit 0, exact TSV onstdout).
Out of scope
Deliberately untouched, per the tracker: #5 (signals), #6 (pool leak
on error), #8 (read-only database), #16 (CLI surface tests), #24
(local linter version).
Built and verified as follows.
Gates
make check(local,golangci-lintv2.10.1): green.ok sneak.berlin/go/sfdupes 0.3s coverage: 86.9% of statements(was64%); the suite runs in well under a second, so it stays far inside
the 20-second budget and the 30-second timeout.
make docker: green. This is the authoritative gate — thedigest-pinned v2.12.2 lint stage plus
make checkas theunprivileged
builderuser. It rejected the first version of thebranch with six
goconstfindings that v2.10.1 (issue #24) did notreport, in
main.goandmain_test.go; fixed by naming thesubcommands
cmdScan/cmdReport/cmdTreesand theUsage:markerin the tests, then re-run to completion.
Behavioural check of the built binary (
make build, real process,real
$?, stdout and stderr captured separately):sfdupessfdupes bogusError: unknown command+ usagesfdupes report xError: unknown command "x" ...+ usagesfdupes scan /nonexistentsfdupes: lstat ...: no such file or directory, no usagesfdupes report(no database)sfdupes: ...: no database (run "sfdupes scan" first, ...), no usagesfdupes scan TREEsfdupes reportsfdupes treesAfter the scan the database directory contained
db.sqliteandnothing else — the WAL was checkpointed and the sidecars removed on
close.
The subtle part, called out for the reviewer: the exit-1 rows
above are exactly the regression risk in this refactor. Cobra prints
the error and the usage text for anything
RunEreturns, and theold
main.gomapped everyroot.Execute()error to exit 2, so anaive
RunEconversion turns every runtime failure into an exit-2usage error with a usage dump.
runEinmain.gosilences both onthe subcommand and tags the error
fatalError;runexits 1 forthose and 2 for everything else cobra produces.
TestRunMissingOperandIsFatalNotUsageand theassertFatalOutputhelper (no
Usage:on stderr, nothing on stdout) lock that down, andTestRunUsageErrorslocks down the other direction.One thing noticed and deliberately left alone (not filed, not
fixed): with stdout closed outright (
sfdupes report >&-) the runstill exits 0 rather than failing on the write, because fd 1 gets
handed to the next file the process opens. That behaviour is identical
before and after this branch and is unrelated to the database
lifetime, so it is out of scope here; say the word and I will file it.
Manager note — independent adversarial review dispatched.
The reviewer did not author this change and is working in its own throwaway worktree. It has been pointed at the specific trap in this refactor: cobra's
RunEprints usage alongside the error by default, and the oldmainmapped everyroot.Execute()error to exit 2, so the easy way to get this wrong is to silently reclassify a fatal (exit 1) as a usage error (exit 2), or to start dumping usage text on ordinary runtime failures. It has been asked to verify every documented exit path by running the built binary and checking real$?, and to try reverting the production change while keeping the new tests to confirm they actually go red rather than passing vacuously.Worth recording from the implementation, because it is the clearest evidence yet for #24:
make checkpassed locally, thenmake dockerrejected the first version with sixgoconstfindings. The local linter is v2.10.1; the repo pins v2.12.2. A green localmake checkin this repo is currently not evidence of anything, and until #24 is fixedmake dockeris the only gate that counts. The reviewer has been told to treat it that way.Coverage moved 64% to 86.9% on this branch.
One thing found during implementation and deliberately kept out of this PR, now filed as #30:
sfdupes report >&-exits 0 rather than failing the write, which README §Error handling lists as an exit-1 condition. It predates this branch and is unrelated to database lifetime, so excluding it was the right call. The wider version of that bug — unchecked writes meaningreport | headand a full disk both silently truncate the report — is the part that actually matters.Review of PR #29 (head
73841c9, basemainatce6d29d)Verdict: PASS — no blocking findings. Six non-blocking notes below.
Reviewed independently against issue #4,
README.md(§Error handlingand exit codes, §Design goal 4, §scan mode, §report mode, §trees
mode),
REPO_POLICIES.mdandTODO.md. All verification was done ina throwaway worktree at the PR head; nothing in the shared checkout
was touched.
Definition of done (issue #4)
runScan/runReport/runTreesreturn errors; single exit pointis
runinmain.go— met.grep -rn "os.Exit" *.gofindsexactly one call site,
main.go:55.db.Close()guaranteed on success, per-file-warning and fatalpaths — met, verified empirically (below).
built from
ce6d29d.asserts no
-walis left — met, and the test is not vacuous(mutation-tested, below).
make checkgreen — met, andmake dockergreen too.Empirical exit-code and stream check
Built binaries from
ce6d29d(base) and73841c9(head) and ran 15invocations through each, capturing real
$?with stdout and stderrseparated:
sfdupesbogusscan(no operand)report xtrees xscan --nope /tmpcompletionscan /nonexistent-xyzreport(no database)trees(no database)reportwith stdout on/dev/fulltreeswith stdout on/dev/full--help/-h/help/scan --help--versionscan TREE/report/trees(success)Base and head agree on every exit code, and the stderr bytes hash
identically for every case except two:
--version(differentgit describestring, expected) and baresfdupes(see note 2).No fatal became a usage error, no usage became a fatal, nothing is
swallowed to 0. Usage-text leakage is correct in both directions.
Stream separation holds: stdout is empty for every failing, usage,
help and
scaninvocation, and carries only TSV forreport/trees.The database actually closes
Built a database with
PRAGMA journal_mode=wal; PRAGMA user_version=1and no
filestable, then ran each subcommand against it:ce6d29d:db.sqlite,db.sqlite-wal,db.sqlite-shmleftbehind for
scan,reportandtrees.73841c9:db.sqlitealone for all three.Error text is byte-identical between base and head on all three paths
(
sfdupes: database ...: read records: SQL logic error: no such table: files (1), and theupdate database ...variant forscan). After asuccessful scan the database directory holds
db.sqlitealone.The new tests are not vacuous
Mutation-tested: in a scratch copy of the head tree I reintroduced the
defect the PR fixes — dropped the deferred
db.Close()inrunScanand
loadRecordsand closed only on the success return, so the closeis skipped exactly on the fatal path — and kept
main_test.gounchanged.
make testgoes red:All three subtests fail on the sidecar assertion, which is the
property under test.
TestOpenDatabaseKeepsWALWhileOpendoes its jobas the premise: it asserts a
-walexists beside an open database,so "no
-walafterwards" is evidence of a close and not of a databasethat never had a WAL. The fatal-path subtests also assert the message
is
no such table: files, which only a query against the already-opendatabase can produce, so they cannot pass by failing before the open.
Gates
make check: green.ok sneak.berlin/go/sfdupes 0.176s coverage: 86.9% of statements,0 issues,fmt-checkclean.make docker: green, exit 0. Lint stage (digest-pinned v2.12.2)0 issuesaftermake fmt-checkandmake lint; builder stagemake check0 issuesat 86.9% coverage, run as the unprivilegedbuilderuser; image exported.73841c9:check / check (push)success.ce6d29dis currentorigin/main; thebranch is a single commit on top of it,
git merge-treereportszero conflicts. Fast-forward.
The
goconstconstants are legitimate, not churnVerified rather than taken on trust. Restored the string literals
(removed the
cmdScan/cmdReport/cmdTreesblock, put"scan [--workers N] [-x] PATH...","report","trees"back in theUse:fields, and the literals back in
main_test.go) in a scratch copy andre-ran
make docker: it fails with exit 2 and 5goconstfindings atv2.12.2 —
Two of the five are in production code, so this is not purely a
test-driven accommodation. The constants are the minimal fix and
preferable to
nolintdirectives.Scope
Diff is six files:
main.go,main_test.go,report.go,scan.go,trees.go,TODO.md. Nothing from #5 (signals), #6 (pool leak onerror), #8 (read-only database) or #16 (CLI surface tests) is present.
No unrelated refactoring.
Dockerfileis untouched: the deliberatenon-root
builderuser and the comment explaining why (root wouldbypass the
chmod(0)the permission tests rely on) are intact, andthe permission-dependent assertions in
scan_test.go:806-814(
st.skipped != 2fails under root) still run in the docker gate.Hygiene
commit message, or the PR body; no attribution trailers. Author and
committer are
sneak <sneak@sneak.berlin>.(closes #4); body wraps at 70 columns.TODO.mdhas an accurate Completed Steps entry at the top, in thesame commit, in the established style.
make fmt-checkclean.Non-blocking findings
main_test.go:185— misleading test name.TestRunMissingOperandIsFatalNotUsagetests a nonexistentPATHoperand, which README §scan mode calls a fatal error (exit 1). But
"missing operand" is README's phrase for
scanwith no operand,which is a usage error (exit 2) and is covered separately as the
"scan without paths"case inTestRunUsageErrors. The nameasserts the opposite of the spec for the case it appears to name.
Acceptable:
TestRunNonexistentPathIsFatalNotUsage.Bare
sfdupesgains one trailing newline on stderr. The oldcode called
cmd.Usage(); cobra now prints the usage for thereturned
errNoSubcommandviac.Println(cmd.UsageString()),which appends a newline.
diffof base vs head stderr for the bareinvocation is exactly one added blank line at EOF. Exit code, text
and stream are unchanged and README specifies nothing here, so this
is cosmetic — but the implementation-plan comment on #4 claims
"Cobra's own usage output is therefore byte-for-byte what it is
today", and that claim is not quite true.
run's injectedstderris only half-honoured.run(args []string, stderr io.Writer)routes cobra's error and usage outputto the writer, but
runScan,runReportandrunTreesstillwrite their warnings, progress and summaries to the package-global
os.Stderr, and the reports write toos.Stdout. The teststherefore have to monkey-patch
os.Stdout(captureStdout), andscanFixture/TestRunScanSucceedsDespiteWarningscannot assertthat the warning actually happened — it only asserts exit 0. Under
a root test runner the
chmod(0)is a no-op and that test degradessilently into an ordinary clean scan while still passing. The
repo-wide root guard survives via
scan_test.go:806-814, so thisis not a hole in the suite, but the new test does not carry its own
weight; asserting
1 skippedin the summary would fix it. Probablybest folded into #16, which owns threading the writers through.
runEis opt-in, and misuse fails silently in the wrongdirection. A future subcommand wired with a bare
RunE(orRun) would not be wrapped infatalError, sorun'sdefaultbranch would classify its runtime failure as exit 2 and cobra would
dump the usage text — the exact regression this PR exists to
prevent. Worth a note on
newRootCommand, or a test that walksroot.Commands()and asserts every leaf command'sRunEis arunEproduct.errNoSubcommand's message is dead text. The rootRunEsetscmd.SilenceErrors = truebefore returning it, so"no subcommand"is never printed anywhere. Harmless, but the sentinelexists only to be non-nil; the doc comment could say so outright
(it half does).
Pre-existing, not caused by this PR: the docker lint stage
emits
The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2.Worth a trackeritem alongside #24 rather than a change here.
Manager note — merged as
2a055c0.Review verdict PASS, no blocking findings, so this landed via a non-fast-forward merge commit. Branch
db-close-on-fataldeleted;origincarries onlymain. Not assigned tosneak—mainis unprotected on this repo, so the manager merges once the gate passes.Post-merge verification on
main:make checkgreen — testsokin 0.406s, coverage 86.9% (up from 64%),0 issues.from the linter.This was a strong review. Rather than reading the diff and reasoning about it, it built binaries from both
ce6d29dand73841c9, ran 15 invocations through each with separated streams, and hashed the stderr output to compare — establishing that exit codes and messages are identical except for two explainable differences. It then mutation-tested the new assertions: it reintroduced the original defect in a scratch copy withmain_test.gountouched, and confirmed all threeTestRunFatalAfterOpenClosesDatabasesubtests go red. That is the difference between "the tests pass" and "the tests would catch a regression", and it is the check most reviews skip.It also independently confirmed the
goconstconstants were forced rather than churn — restoring the string literals and re-runningmake dockerfails with five findings at v2.12.2, two of them inmain.goitself.Non-blocking findings and dispositions:
TestRunMissingOperandIsFatalNotUsageactually covers a nonexistent operand, while "missing operand" is README's phrase forscanwith no operand at all. Misleading name; folded into #16.runthreads its writers only into cobra — subcommands still write to the package-globalos.Stdout/os.Stderr, so tests must monkey-patch. Folded into #16, where it properly belongs.runEis opt-in, so a future subcommand wired with a bareRunEwould reintroduce exactly the misclassification this PR fixes. Folded into #16 as a guard test.sfdupesgains one trailing newline on stderr; cosmetic, but it makes the plan comment's "byte-for-byte identical" claim slightly overstated. No action.errNoSubcommand's message is never printed because the root setsSilenceErrors. Harmless dead string. No action.gomodguarddeprecated in favour ofgomodguard_v2— already tracked as #26 and assigned tosneak, since the fix belongs in the canonical config.Next unit is #6, the hash-phase worker-pool leak. It is the natural follow-on: now that
hashPhaseerrors propagate to a returningrunScaninstead of dying viaos.Exit, the abandoned feeder goroutine and blocked workers are a real leak rather than a theoretical one.