backup: cmd/elastickv-snapshot-decode CLI + CHECKSUMS writer#810
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 19 minutes and 33 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new CLI command ChangesSnapshot Decode CLI
Sequence DiagramsequenceDiagram
participant CLI as CLI Entrypoint
participant Parser as Flag Parser
participant Decoder as backup.DecodeSnapshot
participant Manifest as Manifest Emitter
participant Checksum as WriteChecksums
CLI->>Parser: parseFlags(argv)
Parser->>Parser: validate input/output and adapter set
Parser->>Decoder: run decodeOne with config
Decoder->>Decoder: open FSM snapshot
Decoder->>Decoder: decode snapshot bytes
Decoder->>Manifest: emitManifest (version, cluster-id, timestamp, snapshot_index)
Manifest->>Manifest: populate adapter scopes and write MANIFEST.json
Decoder->>Checksum: WriteChecksums(output root)
Checksum->>Checksum: walk tree, hash files, write CHECKSUMS
Decoder-->>CLI: return success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the elastickv-snapshot-decode CLI tool and supporting internal/backup package logic to decode Pebble snapshots into a vendor-independent directory structure. The implementation includes manifest generation, snapshot index extraction, and a sha256 checksum utility. Review feedback identifies a path traversal vulnerability and memory inefficiency in the checksum verification logic, recommends using filepath.Join for cross-platform path handling, and advises against silently ignoring errors from Close() calls.
| body, err := os.ReadFile(filepath.Join(root, CHECKSUMSFilename)) //nolint:gosec // operator-supplied output dir | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| lines := strings.Split(strings.TrimRight(string(body), "\n"), "\n") | ||
| for i, line := range lines { | ||
| if line == "" { | ||
| continue | ||
| } | ||
| want, rel, ok := splitChecksumLine(line) | ||
| if !ok { | ||
| return errors.Wrapf(ErrChecksumsMalformedLine, "line %d: %q", i+1, line) | ||
| } | ||
| got, err := sha256File(filepath.Join(root, rel)) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "verifying %s", rel) | ||
| } | ||
| if got != want { | ||
| return errors.Wrapf(ErrChecksumMismatch, "%s: have %s want %s", rel, got, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation has two significant issues:
- Efficiency & Scalability:
os.ReadFilefollowed bystrings.Splitreads the entireCHECKSUMSfile into memory and creates multiple copies of the data. For large dump trees with many files, this can lead to excessive memory consumption or OOM. Usingbufio.Scannerto process the file line-by-line is much more efficient. - Security (Path Traversal): The
relpath parsed from the checksum file is joined withrootwithout validation. A maliciousCHECKSUMSfile could contain absolute paths or paths with..segments, allowing an attacker to trick the tool into reading and hashing arbitrary files on the system. Usefilepath.IsLocal(rel)to ensure the path is relative and stays within the intended directory.
Note: You will need to add "bufio" to your imports.
f, err := os.Open(filepath.Join(root, CHECKSUMSFilename)) //nolint:gosec // operator-supplied output dir
if err != nil {
return errors.WithStack(err)
}
defer func() { _ = f.Close() }()
scanner := bufio.NewScanner(f)
for i := 1; scanner.Scan(); i++ {
line := scanner.Text()
if line == "" {
continue
}
want, rel, ok := splitChecksumLine(line)
if !ok {
return errors.Wrapf(ErrChecksumsMalformedLine, "line %d: %q", i, line)
}
if !filepath.IsLocal(rel) {
return errors.Wrapf(ErrChecksumsMalformedLine, "line %d: path %q is not local", i, rel)
}
got, err := sha256File(filepath.Join(root, rel))
if err != nil {
return errors.Wrapf(err, "verifying %s", rel)
}
if got != want {
return errors.Wrapf(ErrChecksumMismatch, "%s: have %s want %s", rel, got, want)
}
}
if err := scanner.Err(); err != nil {
return errors.WithStack(err)
}| if cfg.bundleJSONL { | ||
| m.DynamoDBLayout = backup.DynamoDBLayoutJSONL | ||
| } | ||
| out, err := os.Create(cfg.outputRoot + "/MANIFEST.json") //nolint:gosec // operator-supplied path |
There was a problem hiding this comment.
Use filepath.Join instead of string concatenation for path construction. This ensures cross-platform compatibility and avoids issues with redundant or missing path separators.
| out, err := os.Create(cfg.outputRoot + "/MANIFEST.json") //nolint:gosec // operator-supplied path | |
| out, err := os.Create(filepath.Join(cfg.outputRoot, "MANIFEST.json")) //nolint:gosec // operator-supplied path |
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| defer func() { _ = out.Close() }() |
There was a problem hiding this comment.
Do not silently ignore errors from Close() methods on resources. Log them to ensure resource leaks or other cleanup problems are visible. Since this is a writer, a failure in Close() could indicate a problem flushing final data to disk, even after a successful Sync().
if err := out.Close(); err != nil {
log.Errorf("failed to close file: %v", err)
}References
- Do not silently ignore errors from Close() methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c3cd69fa0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer func() { _ = in.Close() }() | ||
|
|
||
| res, err := backup.DecodeSnapshot(in, backup.DecodeOptions{ | ||
| OutRoot: cfg.outputRoot, |
There was a problem hiding this comment.
The CLI calls DecodeSnapshot without ScratchRoot, which makes the dispatcher default scratch to OutRoot; in the S3 path that becomes <output>/s3, and S3Encoder.Finalize removes that scratch tree. With default adapters (which include S3), any snapshot containing S3 records can lose the emitted s3/ output before CHECKSUMS is written, producing incomplete backups even though decode otherwise succeeds.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return errors.Wrapf(ErrChecksumsMalformedLine, "line %d: %q", i+1, line) | ||
| } | ||
| got, err := sha256File(filepath.Join(root, rel)) |
There was a problem hiding this comment.
VerifyChecksums uses filepath.Join(root, rel) on unvalidated paths from CHECKSUMS. A tampered line like ../... is accepted by splitChecksumLine, so verification can read files outside the dump and still report success, which breaks the guarantee that verification only attests to files inside the backup tree.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/elastickv-snapshot-decode/main_test.go (1)
216-216: 💤 Low valueConsider refactoring to remove
//nolint:gocritic.The guideline states to avoid
//nolintannotations unless absolutely required and prefer refactoring. The append pattern can be replaced with explicit allocation:♻️ Proposed refactor
- encKey := append([]byte(backup.RedisStringPrefix+userKey), make([]byte, snapshotTSSize)...) //nolint:gocritic // explicit append for clarity + prefix := backup.RedisStringPrefix + userKey + encKey := make([]byte, len(prefix)+snapshotTSSize) + copy(encKey, prefix)As per coding guidelines: "Avoid adding
//nolintunless absolutely required and prefer refactoring instead."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/elastickv-snapshot-decode/main_test.go` at line 216, The test uses append with a nolint marker when building encKey: replace the append pattern with an explicit allocation and copy to avoid the //nolint:gocritic; specifically allocate a byte slice of length len(backup.RedisStringPrefix)+len(userKey)+snapshotTSSize, copy the prefix (backup.RedisStringPrefix) and userKey into it, and leave the trailing snapshotTSSize bytes zeroed so encKey is constructed deterministically without using append (update the code that sets encKey in the test accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/backup/checksums.go`:
- Around line 150-156: In VerifyChecksums, validate that the parsed rel path
cannot escape root by calling the existing safeJoinUnderRoot(root, rel) instead
of filepath.Join; on any error from safeJoinUnderRoot return
errors.Wrapf(ErrChecksumsMalformedLine, "line %d: %q", i+1, line) (i.e. map the
helper error to ErrChecksumsMalformedLine), and pass the safe absolute path
returned by safeJoinUnderRoot to sha256File. Also add a unit test for
VerifyChecksums that supplies a CHECKSUMS entry with escaping/absolute rel
values (e.g. "../../etc/passwd" or "/etc/passwd") and assert VerifyChecksums
returns ErrChecksumsMalformedLine.
---
Nitpick comments:
In `@cmd/elastickv-snapshot-decode/main_test.go`:
- Line 216: The test uses append with a nolint marker when building encKey:
replace the append pattern with an explicit allocation and copy to avoid the
//nolint:gocritic; specifically allocate a byte slice of length
len(backup.RedisStringPrefix)+len(userKey)+snapshotTSSize, copy the prefix
(backup.RedisStringPrefix) and userKey into it, and leave the trailing
snapshotTSSize bytes zeroed so encKey is constructed deterministically without
using append (update the code that sets encKey in the test accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fbee340-dc70-4747-87bd-7da346cf1d01
📒 Files selected for processing (6)
cmd/elastickv-snapshot-decode/main.gocmd/elastickv-snapshot-decode/main_test.gointernal/backup/checksums.gointernal/backup/checksums_test.gointernal/backup/source.gointernal/backup/source_test.go
|
@claude review |
|
Claude finished @bootjp's task in 3m 13s —— View job Code Review: PR #810 —
|
| Severity | Issue | File:Line |
|---|---|---|
| P0 | S3 scratch root = OutRoot → Finalize deletes S3 output |
main.go:222 |
| P1 | VerifyChecksums reads files outside dump root via path traversal |
checksums.go:154 |
| P2 | String concat for MANIFEST path instead of filepath.Join |
main.go:281 |
| P2 | Silent Close error on MANIFEST writer |
main.go:285 |
| P2 | os.ReadFile memory inefficiency in VerifyChecksums |
checksums.go:141 |
| P3 | //nolint:gocritic in test, per project guideline |
main_test.go:216 |
The P0 and P1 issues are the priority — both have straightforward fixes and the P0 will silently corrupt any dump that contains S3 data. Everything else (source.go, parseFlags, parseAdapterSet, manifest structure) looks correct.
Three reviewer-flagged issues on PR #810: 1. (codex P1, data loss) The dispatcher defaulted ScratchRoot to OutRoot when blank. S3Encoder.Finalize then ran RemoveAll(scratchRoot+"/s3") and wiped the emitted S3 dump subtree before CHECKSUMS could be written. Fix: newDispatcher.ensureS3Scratch now allocates a fresh os.MkdirTemp when the caller leaves ScratchRoot blank, parents it on the dispatcher, and removes it in finalize via a deferred cleanupScratch. Caller-supplied ScratchRoot is unchanged. Test: TestDecodeSnapshot_S3OutputSurvivesDefaultScratch — feeds a synthetic bucket-meta + manifest + blob through DecodeSnapshot with ScratchRoot blank and asserts the assembled S3 body is on disk under <OutRoot>/s3/<bucket>/<obj>. With the bug the file is wiped and the read fails. 2. (coderabbit critical, path traversal) VerifyChecksums joined CHECKSUMS relative paths to root without validating them, so a malicious CHECKSUMS line like `<hex> ../../etc/shadow` would direct the verifier to fingerprint files outside the dump root. Fix: validateChecksumRelPath rejects absolute paths and any path whose filepath.Clean'd form starts with `..` or equals `..`/`.`/empty. Returns ErrChecksumsPathTraversal so callers can branch on errors.Is. Test: TestVerifyChecksums_RejectsTraversalPath — table-driven over five attack shapes (parent-traversal, deep-traversal, absolute, current-dir, empty); each is rejected before any sha256 is computed. 3. (gemini security-high + high, OOM vector) VerifyChecksums used os.ReadFile + strings.Split, materialising the whole CHECKSUMS in memory. A multi-million-file dump or an adversarial newline-free CHECKSUMS could OOM the verifier. Fix: switched to bufio.Scanner with a fixed 8 KiB per-line budget. Streaming throughout — even with a TB of dump artefacts the verifier holds at most one line plus the in-flight sha256 hash state. Test: TestVerifyChecksums_StreamsLargeFile exercises the scanner path with 64 files (a realistic file count for the per-line-scan code path; the cost of a literal-large fixture in CI is not justified vs. the assertion that the streaming API works at all). Caller audit (CLAUDE.md "semantic-change → audit all callers"): - VerifyChecksums: only internal/test callers; no production call sites in the CLI yet. Path-validation is purely a rejection-side tightening; honest CHECKSUMS files are unaffected. Streaming semantics are unobservable to callers. - DecodeSnapshot / newDispatcher: one external caller (cmd/elastickv-snapshot-decode/main.go:222), which leaves ScratchRoot blank and now gets the safe tempdir behavior. No signature change. Self-review: 1. Data loss — fixed (the S3 wipe was data-loss; tempdir isolation prevents it). New temp leaves on hard crash but live system temp eviction handles that. 2. Concurrency — no shared state introduced. 3. Performance — streaming verify is strictly better. 4. Data consistency — path traversal closed; encryption-state handling unchanged. 5. Test coverage — three regression tests, one per finding.
|
@claude review Addressed three reviewer findings in 34970f0:
Caller audit per CLAUDE.md (semantic-change → grep all callers):
|
|
Claude finished @bootjp's task in 2m 18s —— View job Code Review: PR #810 — fixes in
|
| Severity | Issue | Location |
|---|---|---|
| P2 | cfg.outputRoot + "/MANIFEST.json" — string concat instead of filepath.Join |
main.go:281 |
| P2 | Silent Close error on MANIFEST writer |
main.go:285 |
| P3 | //nolint:gocritic in test helper |
main_test.go:216 |
These are low-risk (P2 string concat is cosmetic on POSIX; P2 silent Close is guarded by out.Sync() two lines above; P3 nolint violates project guidelines). Worth cleaning up before merge but not blockers if stacked PRs are landing fast.
Summary: All three targeted fixes (S3 data loss, path traversal, OOM vector) are correctly implemented and covered by regression tests. The three items above remain open from the previous review round.
Adds the Phase 0a operator-facing binary on top of the dispatcher landed in PR #806. The CLI reads a .fsm snapshot, dispatches it through DecodeSnapshot, and emits MANIFEST.json + CHECKSUMS into the chosen output root. - cmd/elastickv-snapshot-decode/main.go - flag surface: --input, --output, --adapter, --cluster-id, --include-incomplete-uploads, --include-orphans, --preserve-sqs-visibility, --include-sqs-side-records, --rename-collisions, --dynamodb-bundle-mode - testable run() entrypoint (argv slice in, error out; no os.Exit) so main_test.go can drive the binary directly - structured slog warning sink wired into the encoders - MANIFEST.json stamped with snapshot_index parsed from filename, last_commit_ts from the dispatcher header, exclusion flags from CLI, and a non-nil pointer to Adapter{} for every enabled adapter so the on-disk shape is already correct when per-scope enumeration lands - cmd/elastickv-snapshot-decode/main_test.go - end-to-end synthetic Redis-string snapshot round-trips through run(), produces the expected blob file, manifest, and CHECKSUMS, and passes VerifyChecksums - flag-validation paths: missing --input, unknown adapter, zero adapter list, unparseable .fsm filename - parseAdapterSet table test - internal/backup/checksums.go - WriteChecksums(root): walks root, computes sha256, writes a sha256sum(1)-compatible CHECKSUMS file (two-space separator, forward-slash paths, lowercase hex, lexicographic order, excludes CHECKSUMS itself) - VerifyChecksums(root): re-reads the file and surfaces the first mismatch as ErrChecksumMismatch (typed sentinel for the Phase 0b encoder self-test path) - internal/backup/source.go - SnapshotIndexFromPath: parses the <uint64>.fsm filename convention the live writer emits and the CLI stamps into MANIFEST.snapshot_index; soft-fails for off-convention names via ErrSnapshotIndexUnparseable - internal/backup/{checksums,source}_test.go - basic listing, self-exclusion, determinism across runs, happy-path verify, tamper detection - SnapshotIndexFromPath table test covering live-writer layout, hand-rolled names, max-uint64, missing suffix, empty stem, non-numeric, negative, hex stems Self-review: 1. Data loss: no — CHECKSUMS is written last so a crash mid-dump leaves an unchecked but otherwise intact tree the operator can re-checksum; the MkdirAll on outputRoot is idempotent so an operator-supplied pre-existing directory is preserved. 2. Concurrency: single-threaded; main is sequential. 3. Performance: sha256 over each output file streams via io.Copy so multi-GB dump artifacts pass without buffering. CHECKSUMS walk is O(files). 4. Data consistency: MANIFEST.last_commit_ts comes from the dispatcher header (now surfaced via ReadSnapshotWithHeader so empty-snapshot dumps still populate it); snapshot_index parsed from the filename per the documented live-writer convention; encryption-state failures propagate from DecodeSnapshot as ErrSnapshotEncryptedEntry. 5. Test coverage: five CLI tests (golden path + four flag- validation surfaces); seven backup-package tests covering the new helpers; existing backup-package tests still pass.
…ning)
Two reviewer-flagged issues on the CHECKSUMS path. The Codex P1
("default ScratchRoot aliased OutRoot") was already fixed on main
by PR #806 follow-ups (c9f23c7 + b9fa4a7 — default to
<OutRoot>/.scratch + reject ScratchRoot==OutRoot), so this commit
ships only the remaining two findings.
1. (coderabbit critical, path traversal) VerifyChecksums joined
CHECKSUMS relative paths to root without validating them, so a
malicious CHECKSUMS line like `<hex> ../../etc/shadow` would
direct the verifier to fingerprint files outside the dump root.
Fix: validateChecksumRelPath rejects absolute paths and any
path whose filepath.Clean'd form starts with `..` or equals
`..`/`.`/empty. Returns ErrChecksumsPathTraversal so callers
can branch on errors.Is.
Test: TestVerifyChecksums_RejectsTraversalPath — table-driven
over five attack shapes (parent-traversal, deep-traversal,
absolute, current-dir, empty); each is rejected before any
sha256 is computed.
2. (gemini security-high + high, OOM vector) VerifyChecksums used
os.ReadFile + strings.Split, materialising the whole CHECKSUMS
in memory. A multi-million-file dump or an adversarial
newline-free CHECKSUMS could OOM the verifier.
Fix: switched to bufio.Scanner with a fixed 8 KiB per-line
budget. Streaming throughout — even with a TB of dump
artefacts the verifier holds at most one line plus the
in-flight sha256 hash state.
Test: TestVerifyChecksums_StreamsLargeFile exercises the
scanner path with 64 files; CI cost of a literal-large
fixture is not justified vs. the assertion that the streaming
API works at all.
Caller audit (CLAUDE.md "semantic-change → grep all callers"):
- VerifyChecksums: zero production callers. Test-only consumers.
Path-validation is rejection-side tightening; honest CHECKSUMS
unaffected. Streaming semantics unobservable to callers.
- WriteChecksums (unchanged in this commit) has one caller
(cmd/elastickv-snapshot-decode/main.go:252) — still passes
through.
Self-review:
1. Data loss — no regression (S3 scratch fix already on main).
2. Concurrency — none.
3. Performance — streaming verify is strictly better.
4. Data consistency — path traversal closed.
5. Test coverage — two regression tests, one per finding.
There was a problem hiding this comment.
💡 Codex Review
elastickv/internal/backup/decode.go
Lines 150 to 151 in 34970f0
Clean up owned scratch dir on snapshot read failures
When ReadSnapshotWithHeader returns an error, DecodeSnapshot exits before calling d.finalize(), but cleanupScratch is only deferred inside finalize; with default S3 scratch allocation this leaves scratchOwned (and any partially written chunk files) behind in /tmp. Repeated failed decodes can accumulate large temp data and leak decoded remnants, because the new temp-scratch lifecycle is not executed on the error path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err != nil { | ||
| return errors.Wrapf(err, "line %d", lineNum) | ||
| } | ||
| got, err := sha256File(filepath.Join(root, cleaned)) |
There was a problem hiding this comment.
VerifyChecksums validates .. traversal in the textual path, but then hashes filepath.Join(root, cleaned) via os.Open, which follows symlinks; an untrusted dump can include CHECKSUMS entry safe/file where safe/file is a symlink to a host path outside the backup root, and verification will read/hash that external file. This breaks the intended guarantee that verification is confined to the dump tree and can produce false trust decisions based on external filesystem state.
Useful? React with 👍 / 👎.
bootjp
force-pushed
the
backup/snapshot-decode-cmd
branch
from
34970f0 to
b60f468
Compare
|
@claude review r1 (force-pushed b60f468 after rebase onto main):
Caller audit: |
|
Claude finished @bootjp's task in 3m 0s —— View job Code Review: PR #810 — r1 (b60f468)Todo list
Verified fixes ✅Codex P1 (ScratchRoot) — correctly dropped. CodeRabbit Critical (path traversal) — correctly fixed. Gemini OOM — correctly fixed. Codex P2 "scratch dir leak" — incorrect findingThe Codex comment on fErr := d.finalize() // called unconditionally, AFTER ReadSnapshotWithHeader
if err == nil {
err = fErr
}The code comment even says "finalize() runs unconditionally so encoders (notably the S3 encoder's scratch tree) get cleanup even when the stream is truncated." The scratch dir ( P1 — Symlink bypass in
|
| Severity | Issue | Location |
|---|---|---|
| P2 | cfg.outputRoot + "/MANIFEST.json" — string concat instead of filepath.Join |
main.go:281 |
| P2 | defer func() { _ = out.Close() }() — silent Close error; Sync() doesn't fully cover it |
main.go:285 |
| P3 | //nolint:gocritic in test violates "Avoid //nolint — refactor instead" |
main_test.go:216 |
The P3 nolint refactor (allocate + copy instead of append) and P2 items are all low-risk; the symlink bypass is the only new substantive finding this round.
Codex r2 P1: after the r1 textual `..`-traversal guard, VerifyChecksums still followed symlinks when sha256-ing the joined path. A CHECKSUMS line `<hex> safe/file` could direct the verifier to hash a host file outside the dump root via a planted symlink at `safe/file` (or any intermediate parent). Fix: refuseSymlinkComponents walks the relative path segment by segment from root, os.Lstat()s each component, and returns ErrChecksumsSymlinkEscape the moment any component is a symlink. Catches BOTH terminal symlinks AND intermediate ones (a parent directory along the way being symlinked). The per-component walk is stricter than syscall O_NOFOLLOW (which only covers the terminal case) and avoids the filepath.EvalSymlinks resolution we are trying to refuse. Phase 0a dumps contain only regular files by design, so refusing every symlink found in the path is the right policy for the only honest dump shape. Tests: - TestVerifyChecksums_RejectsTerminalSymlink — plant a symlink at safe/escape pointing at a host file, add a CHECKSUMS line for it; assert ErrChecksumsSymlinkEscape. - TestVerifyChecksums_RejectsIntermediateSymlink — plant a symlink at safe/ (a directory link to an outside dir), reference safe/target.txt in CHECKSUMS; same assertion. - Both tests skip on Windows (os.Symlink requires SeCreateSymbolicLinkPrivilege there; the lstat-walk fix itself runs on every platform). Caller audit (CLAUDE.md "semantic-change → grep all callers"): - refuseSymlinkComponents is only called from verifyOneChecksumLine, which is only called from VerifyChecksums. VerifyChecksums has no production callers (test-only); CLI uses WriteChecksums. Self-review: 1. Data loss — none (read-side hardening). 2. Concurrency — none. 3. Performance — one extra Lstat per path component during verify; verify is offline-only and rare. 4. Data consistency — symlink-escape closed. 5. Test coverage — two regression tests (terminal + intermediate symlink); existing five-shape traversal-path table test still passes.
|
@claude review Addressed codex r2 P1 (symlink escape) in 2bb8cd7:
Caller audit: |
|
Claude finished @bootjp's task in 1m 27s —— View job Code Review: PR #810 — r2 (
|
| Severity | Issue | Location |
|---|---|---|
| P2 | cfg.outputRoot + "/MANIFEST.json" — string concat instead of filepath.Join |
main.go:281 |
| P2 | defer func() { _ = out.Close() }() — silent Close error on MANIFEST writer |
main.go:285 |
| P3 | //nolint:gocritic in test helper |
main_test.go:216 |
The P2 silent-Close pattern also appears in WriteChecksums at checksums.go:75 (same defer func() { _ = out.Close() }(), though that one is protected by out.Sync() on line 81 too). Neither is a blocker given the Sync() guard, but they're worth noting for consistency.
Overall
The symlink escape fix is correctly implemented and well-tested. The three remaining items are all low-risk and non-blocking — the security-relevant findings (path traversal, symlink escape, OOM) have all been resolved. This is ready to merge once the stacking dependency (#806) lands.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bb8cd7fb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if filepath.IsAbs(rel) { | ||
| return "", errors.Wrapf(ErrChecksumsPathTraversal, "absolute path: %q", rel) | ||
| } | ||
| cleaned := filepath.Clean(rel) |
There was a problem hiding this comment.
validateChecksumRelPath only rejects absolute and ..-prefixed paths, but it still accepts non-local Windows names like CON, CONOUT$, or COM1. During VerifyChecksums, those names are opened via os.Open(filepath.Join(root, cleaned)), which can access device paths outside the backup tree instead of regular files, so verification can succeed/fail based on host device state rather than dump contents. Replace this guard with a locality check (for example filepath.IsLocal on the original/cleaned path) to enforce the intended “confined to dump root” guarantee across platforms.
Useful? React with 👍 / 👎.
…sLocal) Codex r3 P1: validateChecksumRelPath rejected `..`-traversal and absolute paths but accepted Windows reserved device names like CON / NUL / COM1 / CONOUT$. On Windows, os.Open of those names yields the host's console / device rather than a regular file under the dump root, so verification can succeed/fail based on host device state instead of dump contents. Fix: replace the explicit absolute/traversal checks with filepath.IsLocal (Go 1.20+), which delegates path-locality to the stdlib. On Windows IsLocal additionally rejects the reserved device names; on Unix the names look like ordinary files (and would be hashed if they exist under root, the honest dump shape). Empty / `.` are still explicitly rejected because IsLocal considers `.` local (it stays within the subtree, but it names the root directory, which is not a hashable regular file). The post-Clean recheck below IsLocal is belt-and-braces against a future stdlib change. Tests: - TestValidateChecksumRelPath_RejectsWindowsDeviceNames pins the rejection. The reserved-name check is Windows-only by filepath.IsLocal's contract, so the test skips on non-Windows; the lexical fix itself runs on every platform. - TestValidateChecksumRelPath_AcceptsHonestPaths cross-checks the IsLocal switchover does not regress the five legitimate Phase 0a dump-tree shapes (Redis blob, S3 nested object, DynamoDB schema, SQS messages.jsonl). Caller audit (CLAUDE.md "semantic-change → grep all callers"): - validateChecksumRelPath only called from verifyOneChecksumLine → VerifyChecksums; the latter has no production callers (test-only). The semantic change is purely rejection-side tightening on Windows; honest CHECKSUMS unaffected. Self-review: 1. Data loss — none (read-side hardening). 2. Concurrency — none. 3. Performance — IsLocal is O(path-len); a single pass. 4. Data consistency — Windows device-name escape closed. 5. Test coverage — two new tests; existing five-shape traversal-path table test still passes.
|
@claude review Addressed codex r3 P1 in b05ec87:
Caller audit: |
|
Claude finished @bootjp's task in 1m 12s —— View job Code Review: PR #810 — r3 (
|
| Severity | Issue | Location |
|---|---|---|
| P2 | cfg.outputRoot + "/MANIFEST.json" — string concat instead of filepath.Join |
main.go:281 |
| P2 | defer func() { _ = out.Close() }() — silent Close error; Sync() doesn't fully cover it |
main.go:285 |
| P3 | //nolint:gocritic in test violates "Avoid //nolint — refactor instead" |
main_test.go:216 |
None are blockers (the P2 string concat is cosmetic on POSIX; Sync() on line 289 covers the data-flush concern; the P3 nolint is in a test helper). Worth cleaning before merge but not holding up the security-relevant work.
Summary: The r3 fix is clean. filepath.IsLocal is the right delegation point for Windows reserved-name rejection. The call-order (raw rel before Clean) and belt-and-braces post-Clean guards are both correct. The two new tests are well-targeted and the Windows-only skip is properly justified. No new issues. All security-relevant findings across r1–r3 are now resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b05ec8784c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| defer func() { _ = out.Close() }() | ||
| for _, e := range entries { | ||
| if _, err := fmt.Fprintf(out, "%s %s\n", e.hex, e.relPath); err != nil { |
There was a problem hiding this comment.
WriteChecksums writes each filename verbatim into a newline-delimited CHECKSUMS line ("%s %s\n"), but dump paths can legally contain newline characters (for example, S3 object keys are accepted without filtering \n), so one file can be serialized as multiple lines and make VerifyChecksums/sha256sum -c parse a corrupted manifest. In that case the backup becomes unverifiable even though file contents are intact; paths need sha256sum-compatible escaping (or an explicit rejection of newline-bearing names) before emitting the line format.
Useful? React with 👍 / 👎.
Three remaining reviewer findings on PR #810 (one P2, two mediums) that the prior rounds did not address. None is a fail-closed semantic change; together they tighten the CHECKSUMS on-disk format and the cmd's manifest write path against operational shapes the design promises to support. 1. (codex r4 P2) WriteChecksums wrote filenames verbatim into a newline-delimited file. A path containing `\n`, `\r`, or `\\` would corrupt the line-based output and let a single adversarial entry inject a second CHECKSUMS row that the verifier would accept. Fix: formatChecksumLine implements the GNU coreutils sha256sum(1) escape convention — leading `\` on the line plus `\n` / `\r` / `\\` substitutions. splitChecksumLine reverses the convention via unescapeChecksumPath at parse time. `sha256sum -c CHECKSUMS` from the dump root parses both forms identically, preserving the vendor-independent recovery property. Tests: - TestChecksumLine_EscapeRoundTrip — five paths covering `\n`, `\r`, `\\`, multiple escapes mixed, and the no-escape passthrough. - TestChecksumLine_EscapePrefixMatchesGNUFormat — pins the exact `\` + hex + ` ` + escaped-body shape so a future change cannot drift from sha256sum compatibility. - TestSplitChecksumLine_RejectsDanglingEscape — guards against a tampered CHECKSUMS using `\<EOL>` or `\x` for unknown `x`. 2. (gemini r1 medium, main.go:281) emitManifest concatenated `cfg.outputRoot + "/MANIFEST.json"`. On Windows this produces wrong path separators and would refuse a Windows-style output root. Fix: filepath.Join. Standard cross-platform shape. 3. (gemini r1 medium, main.go:285) emitManifest deferred a `_ = out.Close()` that swallowed the Close error. On NFS / FUSE filesystems Close is the authoritative durability signal (writeback errors surface there, not at Write time), so a silently-discarded Close error means the dump-tree invariant (MANIFEST.json on disk) can fail without the cmd surfacing it. Fix: explicit Sync() then Close() with both errors propagated; the defer'd close on error paths runs through `_ = out.Close()` because we have already surfaced the primary failure. Caller audit: - formatChecksumLine / unescapeChecksumPath are new helpers called only from WriteChecksums / splitChecksumLine. No cross-package use. - splitChecksumLine's exported behavior is rejection-side tightening (dangling escape) plus correct round-trip of escape-needing paths. No production caller of VerifyChecksums exists (test-only). - emitManifest is internal to cmd/elastickv-snapshot-decode. The Close-error surface is purely additive — previously- successful runs stay successful. Self-review: 1. Data loss — none; the new escape path PREVENTS data loss (line injection); the Close-error surface PREVENTS silent manifest-write loss on NFS. 2. Concurrency — none. 3. Performance — single Builder per escape-needing line; the no-escape fast path is unchanged. 4. Data consistency — sha256sum(1) escape round-trip is byte-identical (round-trip test pins it). 5. Test coverage — three new tests; existing seven CHECKSUMS tests still pass.
|
@claude review Addressed three additional reviewer findings (codex P2 + 2 gemini mediums) in ec358ab:
Three new tests pin the round-trip and the parse-side guards ( Caller audit: |
|
Claude finished @bootjp's task in 2m 42s —— View job Code Review: PR #810 — r4 (
|
| Severity | Issue | Location | Status |
|---|---|---|---|
| ✅ Fixed | WriteChecksums newline-injection via unescaped filenames |
checksums.go |
r4 |
| ✅ Fixed | filepath.Join for MANIFEST path |
main.go:284 |
r4 |
| ✅ Fixed | Silent Close error on MANIFEST writer |
main.go:285 |
r4 |
| P3 | //nolint:mnd in formatChecksumLine (new) |
checksums.go:103 |
Open |
| P3 | //nolint:gocritic in test helper |
main_test.go:216 |
Open (prior) |
| P3 | Doc: "binary mode" should be "text mode" in WriteChecksums |
checksums.go:52 |
Open |
All security-relevant findings across r1–r4 are resolved. The two //nolint violations and the doc wording are the only remaining items — none are blockers. This is ready to merge once #806 lands and the stacking dependency is resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/backup/checksums_test.go (1)
107-117: ⚡ Quick winPin
ErrChecksumMismatchin the tampering test.This currently passes on any non-nil error, so a regression to a generic parse or I/O failure would still look green.
Suggested change
- if err := VerifyChecksums(root); err == nil { - t.Fatalf("expected VerifyChecksums to detect tampering, got nil") - } + err := VerifyChecksums(root) + if !errors.Is(err, ErrChecksumMismatch) { + t.Fatalf("err = %v, want ErrChecksumMismatch", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/backup/checksums_test.go` around lines 107 - 117, Update TestVerifyChecksums_DetectsTampering to assert the returned error specifically indicates a checksum mismatch rather than any non-nil error: after calling VerifyChecksums(root) replace the generic nil check with an errors.Is(err, ErrChecksumMismatch) (or equivalent comparison) and fail the test if that check is false; reference the test name TestVerifyChecksums_DetectsTampering, the VerifyChecksums call, and the ErrChecksumMismatch sentinel so the test only passes when VerifyChecksums returns the expected checksum-mismatch error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/backup/checksums.go`:
- Around line 71-82: The WriteChecksums function currently defers out.Close()
and only returns the error from out.Sync(), which discards Close() failures;
replace the deferred close with explicit handling: remove the defer, call
syncErr := out.Sync() after writing, then call closeErr := out.Close(), and
return a wrapped/combined error that surfaces closeErr when present (e.g., if
closeErr != nil return errors.WithStack(fmt.Errorf("%v; close error: %w",
syncErr, closeErr)) else return errors.WithStack(syncErr)). Refer to the file
writer variable out and the existing out.Sync() call when making this change.
---
Nitpick comments:
In `@internal/backup/checksums_test.go`:
- Around line 107-117: Update TestVerifyChecksums_DetectsTampering to assert the
returned error specifically indicates a checksum mismatch rather than any
non-nil error: after calling VerifyChecksums(root) replace the generic nil check
with an errors.Is(err, ErrChecksumMismatch) (or equivalent comparison) and fail
the test if that check is false; reference the test name
TestVerifyChecksums_DetectsTampering, the VerifyChecksums call, and the
ErrChecksumMismatch sentinel so the test only passes when VerifyChecksums
returns the expected checksum-mismatch error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9ec94dd-3a71-43ec-b607-205c3f1e43fb
📒 Files selected for processing (6)
cmd/elastickv-snapshot-decode/main.gocmd/elastickv-snapshot-decode/main_test.gointernal/backup/checksums.gointernal/backup/checksums_test.gointernal/backup/source.gointernal/backup/source_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec358ab9ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // decodeOne runs the dispatcher against the configured input and | ||
| // writes MANIFEST.json + CHECKSUMS into the output tree. | ||
| func decodeOne(cfg *config, logger *slog.Logger) error { | ||
| if err := os.MkdirAll(cfg.outputRoot, defaultOutputDirMode); err != nil { |
There was a problem hiding this comment.
decodeOne only calls os.MkdirAll and then writes new adapter files into whatever is already under --output. Because WriteChecksums later hashes all regular files under that root, rerunning the CLI into a previously used directory can silently include stale files from an older snapshot in the new CHECKSUMS and dump tree. This makes the resulting backup represent a mixed state rather than the requested snapshot; consider failing fast when --output is non-empty (or cleaning adapter subtrees first).
Useful? React with 👍 / 👎.
Coderabbit r5 Major (checksums.go:82): WriteChecksums returned out.Sync() but discarded the deferred out.Close() error. Same shape as the r4 gemini medium on emitManifest, applied here too. On NFS / some FUSE backends Close is where buffered-writeback failures surface, so a swallowed Close means the CLI can declare success on a truncated or unreadable CHECKSUMS file. Fix: explicit Sync() then Close() with both errors propagated, with the pre-existing defer'd Close swallowed only on the error-return paths (the primary failure has already been surfaced). Extracted writeAllChecksumLines so the parent stays a thin open / write / sync / close skeleton with the durability contract spelled out. Caller audit (CLAUDE.md "semantic-change → grep all callers"): - WriteChecksums has one caller (cmd/elastickv-snapshot-decode/main.go:253), which already surfaces the returned error verbatim via errors.Wrap. The semantic tightening (Close errors now surface) is honored by the existing wrapping; no caller change needed. Also addresses coderabbit r5 nitpick: typed-sentinel assertion in TestVerifyChecksums_DetectsTampering (was checking err != nil generically; now pins ErrChecksumMismatch so a future change that turns the tamper signal into a different error class surfaces as a test failure). Self-review: 1. Data loss — fix prevents silent CHECKSUMS-truncation on NFS / FUSE. 2. Concurrency — none. 3. Performance — no new I/O; just surfaces an existing error. 4. Data consistency — durability contract now honest. 5. Test coverage — typed-sentinel assertion added; existing round-trip tests still pass.
|
@claude review Addressed coderabbit r5 Major + nitpick in 7a8e2f3:
Caller audit: |
|
Claude finished @bootjp's task in 1m 50s —— View job Code Review: PR #810 — r5 (
|
| Severity | Issue | Location | Status |
|---|---|---|---|
| P2 | Non-empty --output dir mixes stale files into new CHECKSUMS (Codex r4) |
main.go:214 |
Open |
| P3 | //nolint:mnd in formatChecksumLine — named constant would fix |
checksums.go:129 |
Open (r4) |
| P3 | //nolint:gocritic in test helper |
main_test.go:216 |
Open (early rounds) |
| P3 | Doc: two-space separator is text mode, not "binary" mode in sha256sum(1) | checksums.go:49 |
Open (r4) |
The P2 non-empty-output-root finding (Codex r4) is worth noting: if the operator reruns decodeOne into an existing --output directory, WriteChecksums will hash all regular files found there — including stale files from a prior decode run — and produce a CHECKSUMS that attests to a mixed-snapshot tree. The fix would be a preflight check that rejects a non-empty outputRoot (or documents "caller must use a fresh directory"). Not a blocker if the CLI docs/--help call this out, but it's easy to misuse silently.
The three P3 items are cosmetic. The security-relevant findings across all five rounds are resolved. This is ready to merge once #806 lands.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a8e2f308e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if line == "" { | ||
| return nil |
There was a problem hiding this comment.
verifyOneChecksumLine treats blank lines as success, and VerifyChecksums never enforces that at least one checksum row was parsed. As a result, a truncated/corrupted CHECKSUMS file containing only empty lines returns nil, so integrity verification can report success without checking any file content. This diverges from sha256sum -c semantics for malformed/empty manifests and can hide backup corruption.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return errors.WithStack(err) |
There was a problem hiding this comment.
When a listed file is missing, refuseSymlinkComponents returns the raw os.Lstat error, so callers cannot detect it via errors.Is(err, ErrChecksumMismatch) and it bypasses the verifier's typed checksum-failure path. In practice, partial restores or deleted files produce an unclassified path error instead of a checksum verification failure, which breaks error handling that keys on checksum result types.
Useful? React with 👍 / 👎.
…sification) Two codex P2 findings on r5 (commit 7a8e2f3): 1. (codex P2, checksums.go:256) VerifyChecksums returned nil on a CHECKSUMS file with zero parsed rows (empty file, blank lines only). Phase 0a dumps are non-empty by construction (WriteChecksums lists every regular file), so a zero-row CHECKSUMS is always a producer-side corruption signal, never a benign shape. The verifier was hiding partial-write or truncation failures behind a "no rows checked == OK" answer. Fix: VerifyChecksums now counts verified rows and returns ErrChecksumsEmpty when the count is zero. Blank lines still skip silently (matching sha256sum -c's tolerance of trailing newlines) but do not satisfy the at-least-one-row guard. Test: TestVerifyChecksums_RejectsEmptyChecksumsFile covers three corruption shapes (empty file, blank lines only, trailing-newline only). 2. (codex P2, checksums.go:306) refuseSymlinkComponents returned raw fs.ErrNotExist when a CHECKSUMS-listed file was missing from disk. Callers branching on errors.Is(err, ErrChecksumMismatch) missed this case — partial restores and tamper-deletes produced an unclassified path error instead. Fix: detect fs.ErrNotExist at the refuseSymlinkComponents call site (and at sha256File's call site as belt-and-braces) and promote to ErrChecksumMismatch. The design's contract ("Files referenced by CHECKSUMS but missing on disk surface as the same error class so a partial-restore is obvious") is now honored. Test: TestVerifyChecksums_MissingTargetIsMismatch — writes a CHECKSUMS row, deletes the listed file, asserts ErrChecksumMismatch via errors.Is. API shape: verifyOneChecksumLine returns (bool, error) so the parent loop can count "real" verified rows separately from blank-line skips. Internal helper; signature change does not escape the package. Caller audit: - verifyOneChecksumLine is internal to checksums.go; the signature change is contained. - VerifyChecksums has no production callers yet (test-only). The new ErrChecksumsEmpty and the promoted ErrChecksumMismatch shapes are additive on the typed-sentinel surface; honest CHECKSUMS still verify cleanly. Self-review: 1. Data loss — none; both fixes catch silent-corruption signals. 2. Concurrency — none. 3. Performance — one int counter per file; sha256 streaming unchanged. 4. Data consistency — empty-CHECKSUMS hole closed; missing- target now classified. 5. Test coverage — two new tests; existing thirteen CHECKSUMS tests still pass.
|
@claude review Addressed two codex r5 P2 findings in 2822f2d:
Two new regression tests: Caller audit: |
|
Claude finished @bootjp's task in 1m 54s —— View job Code Review: PR #810 — r6 (
|
| Severity | Issue | Location | Status |
|---|---|---|---|
| P3 | //nolint:mnd in formatChecksumLine — named constant would fix |
checksums.go:130 |
Open (r4) |
| P3 | //nolint:gocritic in test helper |
main_test.go:216 |
Open (early rounds) |
| P3 | Doc: two-space separator is text mode, not "binary" mode in sha256sum(1) | checksums.go:50 |
Open (r4) |
| P2 | Non-empty --output dir mixes stale files into new CHECKSUMS |
main.go:214 |
Open (r5) |
The P3 items are cosmetic and the P2 stale-output-root finding is operator-ergonomics (not a correctness bug — the CHECKSUMS is accurate for whatever files are present; it just attests to a mixed tree). None are blockers.
Summary: All security-relevant findings across r1–r6 are resolved. The two ErrNotExist promotion sites are both necessary (primary path + TOCTOU belt-and-braces). The ErrChecksumsEmpty guard correctly distinguishes "zero verified rows" from "some malformed rows" by exiting early on parse errors before reaching the empty guard. Ready to merge once #806 lands.
1 participant
Summary
Adds the Phase 0a operator-facing binary on top of the dispatcher in #806. Plus the CHECKSUMS writer / verifier and the
<uint64>.fsmfilename → snapshot-index parser the CLI needs to stamp MANIFEST.json.Stacked on
backup/snapshot-dispatcher— once #806 merges, this PR's diff collapses to just the CLI + helpers and I'll retarget tomain.What lands
cmd/elastickv-snapshot-decode/main.goDecodeSnapshotinvocation,MANIFEST.json+CHECKSUMSemissioncmd/elastickv-snapshot-decode/main_test.goparseAdapterSettable testinternal/backup/checksums.goWriteChecksums(root)/VerifyChecksums(root)— sha256sum(1)-compatibleinternal/backup/source.goSnapshotIndexFromPath—<uint64>.fsmfilename parser used to stampMANIFEST.snapshot_indexinternal/backup/checksums_test.go,source_test.goCLI surface
Matches the design's §"Tooling" decoder section, minus the deferred
--input-snap-dir,--scope, and--dynamodb-bundle-sizeflags (called out in the design as later additions).Output shape
MANIFEST.jsoncarriesphase = "phase0-snapshot-decode", the filename-derivedsnapshot_index, the dispatcher-surfacedlast_commit_ts, exclusion flags from the CLI, and a non-nilAdapter{}pointer for every enabled adapter (per-scope enumeration is a follow-up).CHECKSUMSis the standardsha256sum --binaryshape — verifiable withsha256sum -c CHECKSUMSfrom the dump root, no elastickv binary involved.Test plan
go test -race ./internal/backup/ ./cmd/elastickv-snapshot-decode/(passes)golangci-lint run ./...over the touched packages (clean — 0 issues)go build ./...(clean)!redis|str|greeting→redis/db_0/strings/greeting.bin+MANIFEST.json+CHECKSUMS+VerifyChecksumsround-tripSelf-review (CLAUDE.md §"Self-review of code changes")
MkdirAll(outputRoot)is idempotent.io.Copy; multi-GB files pass without buffering. CHECKSUMS walk is O(files).MANIFEST.last_commit_tscomes from the dispatcher header (now surfaced viaReadSnapshotWithHeaderso empty-snapshot dumps still populate it);snapshot_indexparsed from the filename per the documented live-writer convention; encryption-state failures propagate fromDecodeSnapshotasErrSnapshotEncryptedEntry.Summary by CodeRabbit
New Features
Bug Fixes / Security
Tests