Skip to content

fix(cli): refuse an --output path that already exists - #2217

Merged
cristim merged 1 commit into
mainfrom
fix/refuse-existing-output-csv
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/refuse-existing-output-csv

Conversation

@cristim

@cristim cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Closes #2050

Summary

An explicit --output <path> that already existed was silently truncated by os.Create. That meant a rerun with the same path destroyed the previous purchase report. This matters most on the --input-csv path, which writes no audit records.

  • validateFilePaths (cmd/validators.go) now refuses an explicit --output path that already exists. The message is output path already exists: <path> (it would be overwritten); choose a new --output path or remove the existing file.
    • It uses os.Lstat, so a file, a directory or a symlink is refused. A dangling symlink is refused too, because os.Create would follow it.
    • Any other stat error also fails, with its own message.
  • The check runs in validateFlags, the root command's PreRunE. That means it runs before any fetch, purchase or file creation.
  • Auto-generated timestamped filenames don't go through the check. generateCSVFilename returns CSVOutput only when --output is set.
  • Passing the same file to --input-csv and --output now fails with the same message.
  • CHANGELOG.md, README.md and docs/cli/README.md document the refusal.

Behaviour change: a script that reruns with the same explicit --output path now fails at validation. It needs a new path, or must remove the old file first. No in-repo script, Makefile target or CI step uses cudly --output.

Evidence (local, macOS, GOTOOLCHAIN=go1.26.9; unit tests on real temp files)

  • Fail before / pass after: with the validators.go change stashed, four new TestValidateFilePaths subtests fail: existing file, existing directory, dangling symlink, and input equal to output. With the change restored, they pass.
  • Mutation probe (independent reviewer): swapping Lstat for Stat fails the dangling-symlink subtest.
  • Checks:
    • make build: OK.
    • go vet ./...: clean.
    • go test -race ./cmd/...: pass, with no existing tests changed.
    • golangci-lint v2.10.1 (the CI pin): 0 issues.
    • scripts/test-git-secrets-allowlist.sh: 44/44.
  • Review: two passes by an independent non-author reviewer (sonnet, high).
    • Pass 1 checked reachability, the write paths and the TOCTOU window (small, and the file is written only at the end of the run). Its one low note, the behaviour change, is now documented.
    • Pass 2 found no actionable findings at 2e13d2c.

🤖 Generated with Claude Code

An explicit --output path was opened with os.Create, silently truncating
any existing file (including a previous report or the --input-csv file).
validateFilePaths only checked the parent directory.

Fail validation when the path exists (file, directory or symlink,
detected with Lstat so a dangling symlink cannot be followed and
overwritten) and on any stat error other than not-exist. The message
names the path and tells the operator to pick a new path or remove the
old file. Auto-generated timestamped filenames do not set CSVOutput and
are unaffected. --input-csv equal to --output now fails with the same
message.

Closes #2050

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 37 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 288cb174-5bbb-46f3-9678-b3996c24d991
📥 Commits

Reviewing files that changed from the base of the PR and between a2c4c7e and 2e13d2c.

📒 Files selected for processing: 5
  • CHANGELOG.md
  • README.md
  • cmd/validators.go
  • cmd/validators_test.go
  • docs/cli/README.md
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/m Days type/bug Defect triaged Item has been triaged labels Oct 16, 2026
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at 2e13d2c

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Non-blocking notes (possible follow-ups):

  1. TOCTOU: cmd/validators.go:298-303 checks at PreRunE, but os.Create (cmd/multi_service_csv.go:263) runs only after the fetch or purchase. A file created at that path in between would still be truncated. Opening with O_CREATE|O_EXCL in the writer would close the gap.
  2. Untested error branch: the "only ENOENT counts as free" branch (validators.go:301) has no test for a non-ENOENT stat error such as ENOTDIR or EACCES, and a mutation there survives. The behaviour was confirmed by hand below.
  3. Directory check: validators.go:292 uses os.Stat and handles only IsNotExist. This is harmless, because the following Lstat catches the rest.

What was attacked and how (local macOS, GOTOOLCHAIN=go1.26.9, offline; AWS config and credentials set to /dev/null, no --yes, no purchase):

  • Wiring: PreRunE: validateFlags is at cmd/main.go:100, and --output exists only on rootCmd (main.go:117). No subcommand overrides PreRunE or PersistentPreRunE.
    • CSVOutput is read only in generateCSVFilename (multi_service_helpers.go:233) and in the validators. Both report writers (multi_service.go:290, 812) go through generateCSVFilename and then writeMultiServiceCSVReport, which holds the only os.Create.
    • There are no other report or audit output flags. The check runs before any fetch or purchase.
  • go test -race -count=1 ./cmd/...: 1467 passed, no SKIP. go vet ./... was clean.
  • Real binary, built from ./cmd, using --input-csv with a header-only CSV:
    • --output set to an existing file, a dangling symlink, a directory, or the input CSV itself is refused with "output path already exists". The existing file kept its content.
    • --output t/e.csv/x fails with "cannot check output path ... not a directory".
    • A new --output path, and no --output at all, both pass validation with no network call.
  • Fail-before: with validators.go reverted, the 4 new subtests fail (existing file, directory, dangling symlink, input equal to output).
  • Mutation probes: Lstat changed to Stat is killed by the dangling-symlink case. The non-ENOENT-as-free mutation survives (note 2).
  • Docs: CHANGELOG, README and docs/cli/README.md match the behaviour, including the note on scripts that reuse a path.

Gate:

@cristim
cristim merged commit c5e5ae1 into main Oct 16, 2026
12 checks passed
@cristim
cristim deleted the fix/refuse-existing-output-csv branch October 16, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(cli): --output silently truncates an existing CSV (A10-010) [cli part]

1 participant