Skip to content

fix: address code-review findings from #13 (hardening + dead-code removal)#25

Merged
NickMoignard merged 1 commit into
masterfrom
fix/13-review-hardening
Jul 13, 2026
Merged

fix: address code-review findings from #13 (hardening + dead-code removal)#25
NickMoignard merged 1 commit into
masterfrom
fix/13-review-hardening

Conversation

@NickMoignard

Copy link
Copy Markdown
Owner

Addresses the still-valid findings from #13 (see the re-review update). Scope agreed: must-fix correctness items + safe robustness/tests + removing the H1 dead plumbing.

Correctness / data-loss

  • M1 — unbounded join recursion. join.resolve now caps !include nesting at maxIncludeDepth (100), surfaced as a clean InputError, so a corrupt/hostile split tree fails cleanly instead of a fatal goroutine-stack overflow. New deep-nesting regression test.
  • M3 — non-atomic writes. yamltree.SaveFile/SaveRaw now write a temp file in the same dir and os.Rename into place. Protects both join --out (overwrite) and split's multi-file output tree from truncation/partial state on crash/interrupt/disk-full.
  • M2 — swallowed filepath.Abs error. No longer keys the cycle guard by an ambiguous relative path; the (rare) Abs failure is surfaced.

H1 — remove dead plumbing

  • Dropped the unused opts *globalOptions parameter from newSplitCmd/newJoinCmd/newVerifyCmd.
  • Removed the built-but-unused internal/progress package + its CLI wrapper, and the now-orphaned schollz/progressbar dependency (go mod tidy).

Robustness / tests

  • L2logLevelRank reports unknown levels ((int, bool)) instead of silently defaulting to error; EffectiveLogLevel falls back to the default level explicitly (with new rank constants).
  • L1 — a missing root.yml now reports the directory the user passed, not a synthesized <dir>/root.yml.
  • n3 — one shared package-level discard logger instead of per-miss allocation.
  • n2 — comment explaining the fresh per-directory slug.NewNamer().
  • g1 — the in-file anchor/alias test now joins and asserts the alias round-trips (*base preserved), not just that split accepts it.
  • New guard — a test pinning canReconstruct's no-wrap precondition: a long single-line scalar nested deep must round-trip unwrapped. Fails if anyone adds SetWidth to yamltree.Save.

Deferred

  • H1 full wiring (--output json + progress for subcommands) — not implemented; the plumbing was removed rather than wired, per the agreed scope. Worth a separate feature issue if -o json output is wanted.
  • L3 (assert no subcommand overrides PersistentPreRunE) and cosmetic usage-string drift — left for a follow-up.

Validation

go build ./..., go vet ./..., go test -race ./..., and golangci-lint run ./... all clean.

Refs #13.

🤖 Generated with Claude Code

…oval)

Correctness / data-loss (must-fix):
- join: bound !include resolution depth (maxIncludeDepth) so a corrupt or
  hostile split tree fails as a clean InputError instead of exhausting the
  goroutine stack (M1). Adds a deep-nesting regression test.
- yamltree: make SaveFile and SaveRaw atomic (temp file in the same dir +
  os.Rename), so a crash/interrupt/disk-full mid-write can no longer truncate
  an existing document or leave a partial split tree (M3).
- join: stop swallowing filepath.Abs failure; surface it rather than keying the
  cycle guard by an ambiguous relative path (M2).

Dead-code removal (H1):
- Drop the unused `opts *globalOptions` parameter from newSplitCmd/newJoinCmd/
  newVerifyCmd, and remove the built-but-unused internal/progress package and
  its cli wrapper (and the now-unused progressbar dependency).

Robustness / tests:
- cli: logLevelRank now reports unknown levels instead of silently defaulting to
  "error"; EffectiveLogLevel falls back to the default level explicitly (L2).
- cli: report the directory the user passed when a split root.yml is missing,
  not a synthesized path they never typed (L1).
- cli: share one package-level discard logger instead of allocating per miss (n3).
- split: add a regression test pinning canReconstruct's no-wrap precondition
  (fails if yamltree.Save ever sets an encoder width); comment the per-dir namer (n2).
- split: assert the in-file anchor/alias round-trips through join, not just that
  split accepts it (g1).

Refs #13. go build/vet/test/-race and golangci-lint all clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@NickMoignard NickMoignard merged commit b1cf186 into master Jul 13, 2026
6 checks passed
@NickMoignard NickMoignard deleted the fix/13-review-hardening branch July 13, 2026 08:55
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