feat(cmd): add retriever graph dump tooling#103
Open
zinic wants to merge 1 commit into
Open
Conversation
WalkthroughAdds the retriever CLI for dumping, loading, verifying, and benchmarking Dawgs graphs, including manifest files, compression, archive packaging, scrubbing, and metrics. Also updates docs, dependency versions, and query ordering behavior. ChangesRetriever CLI Tool
Query OrderBy Fix
Coding Standard and README Updates
Dependency Maintenance
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant main.go
participant Dump
participant Load
participant Verify
participant Bench
User->>main.go: retriever dump/load/verify/bench
main.go->>Dump: Dump(ctx, db, targets, options)
main.go->>Load: Load(ctx, db, options)
main.go->>Verify: Verify(ctx, db, options)
main.go->>Bench: Bench(ctx, db, targets, options)
Dump-->>main.go: dumpResult / manifest
Load-->>main.go: loadResult
Verify-->>main.go: verifyResult / metricsMismatchError
Bench-->>main.go: benchReport
sequenceDiagram
participant Dump
participant writeCollectionTar
participant encryptedArchiveWriter
participant unpackEncryptedCollectionArchive
participant unpackTar
Dump->>writeCollectionTar: stream manifest+fragments as tar
writeCollectionTar->>encryptedArchiveWriter: write tar bytes into HPKE envelope
encryptedArchiveWriter-->>Dump: encrypted archive file
unpackEncryptedCollectionArchive->>encryptedArchiveWriter: read frames, decrypt via HPKE
encryptedArchiveWriter->>unpackTar: decrypted tar stream
unpackTar-->>unpackEncryptedCollectionArchive: extracted collection files
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
zinic
force-pushed
the
kpom/retriever-additions
branch
from
0a49819 to
809e585
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
cmd/retriever/defaults.toml (1)
1-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSingle-source the scrubber defaults
defaults.tomlis only a documented-configtemplate;newScrubberreads an explicitly supplied path and nothing embeds or auto-loads this file. Since it mirrorsdefaultScrubberConfig(), derive one from the other to avoid drift.🤖 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/retriever/defaults.toml` around lines 1 - 64, The scrubber defaults are duplicated between the TOML template and the runtime config source, so they can drift over time. Update the configuration path used by newScrubber and defaultScrubberConfig so both come from a single source of truth, and make defaults.toml reflect that same structure instead of independently maintaining values like scrub, classifier, and value_shapes.cmd/retriever/main_test.go (1)
85-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp dir leaks if assertions fail before manual
cleanup()call.
cleanupis only invoked at line 96, after thet.Fatalfchecks at lines 87, 90, and 93. If any of those fail, the test halts before cleanup runs, leaking the temp input directory created byprepareLoadInput.♻️ Suggested fix: guarantee cleanup runs
cfg, cleanup, err := prepareLoadInput(cfg) if err != nil { t.Fatalf("prepare load input from archive: %v", err) } + t.Cleanup(cleanup) if cfg.InputDir == "" { t.Fatalf("expected temp input dir") } if _, err := readManifest(cfg.InputDir); err != nil { t.Fatalf("read prepared load manifest: %v", err) } inputDir := cfg.InputDir - cleanup() if _, err := os.Stat(inputDir); !os.IsNotExist(err) { t.Fatalf("expected load temp dir cleanup, got %v", err) }Note: with
t.Cleanup, theos.Statassertion would need to run inside a registered cleanup ordered after the primary one, or be restructured to call cleanup explicitly in the success path while still registering it viat.Cleanupfor the failure paths as a safety net.🤖 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/retriever/main_test.go` around lines 85 - 99, The test in main_test.go leaks the temp input directory when an earlier assertion fails because cleanup is only called in the success path. Update the prepareLoadInput test flow to guarantee cleanup runs by registering the returned cleanup function with t.Cleanup (or otherwise ensuring it executes on failure), and keep the post-cleanup os.Stat check in a separate ordered cleanup/assertion path so the temp dir deletion is still verified.cmd/retriever/metrics.go (1)
297-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent diff truncation with no indicator.
appendMetricDiffcaps atmaxMetricDiffs(20) and silently drops further differences. WhenVerifysurfaces this list directly as the error message, a user debugging a large mismatch has no way to know the reported list is incomplete.♻️ Suggested fix: append a truncation marker
func appendMetricDiff(target []string, value string) []string { - if len(target) >= maxMetricDiffs { + if len(target) >= maxMetricDiffs { + if len(target) == maxMetricDiffs { + return append(target, "... additional differences omitted") + } return target } return append(target, value) }🤖 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/retriever/metrics.go` around lines 297 - 302, The appendMetricDiff helper currently stops adding entries at maxMetricDiffs and silently truncates the diff list, so update it to append a clear truncation marker when the limit is reached. Adjust appendMetricDiff (and any Verify path that uses its output) so the returned slice explicitly indicates there are more differences than shown, making the Metric verification error message obviously incomplete rather than silently cut off.cmd/retriever/bench.go (2)
294-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate byte-accounting logic between
benchNodeBatchandbenchRelationshipBatch.Both functions build a slice of fragment items, then call
compressedJSONSizeon a wrapped{Phase, Items}struct with identical guard/timing logic. This is a good candidate to consolidate alongside thebenchNodes/benchEdgesrefactor above.🤖 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/retriever/bench.go` around lines 294 - 359, `benchNodeBatch` and `benchRelationshipBatch` duplicate the same batch byte-accounting flow: build fragment items, time `compressedJSONSize`, and populate `benchPhaseResult`. Refactor this shared logic into a common helper that accepts the phase/type-specific item builder for `fragmentNode` and `fragmentEdge`, then have both functions delegate to it while preserving the existing `Count`, timing, and byte-size fields.
200-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate keyset-scan loop between
benchNodesandbenchEdges.The two functions are structurally identical (planned-count loop, keyset advancement check, batch dispatch, progress logging) aside from calling
readDatabaseNodes/readDatabaseRelationshipsandbenchNodeBatch/benchRelationshipBatch. This duplication risks the two loops silently diverging on bug fixes (e.g., the keyset-advancement guard) in the future.Consider extracting a shared generic helper that accepts the batch-read and batch-process functions as parameters, keyed by
graph.ID.🤖 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/retriever/bench.go` around lines 200 - 292, `benchNodes` and `benchEdges` duplicate the same keyset-scan loop, so factor the shared flow into a helper that takes the read and batch handlers as parameters. Keep the common planned-count loop, `lastID` advancement check, progress logging, and elapsed-time tracking in one place, and let `benchNodes`/`benchEdges` only supply `readDatabaseNodes` vs `readDatabaseRelationships` and `benchNodeBatch` vs `benchRelationshipBatch`. Ensure the helper still works with `graph.ID` and preserves the existing `retrieverInitialProgressAt`, `retrieverBatchLimit`, and `logBenchPhaseProgress` behavior.cmd/retriever/README.md (1)
177-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBench's
-all-graphsflag is undocumented.
main.go'srunBenchwires-all-graphs(flags.BoolVar(&cfg.AllGraphs, "all-graphs", false, "Benchmark every graph discoverable by the selected driver.")), mirroring the dump command, but the Bench section here doesn't mention it (only the Dump section at Line 23-26 documents-all-graphs).As per coding guidelines, "Keep
README.mdin lock-step with test, build, setup, and user-facing workflow changes."retriever bench \ -connection "$CONNECTION_STRING" \ -graph default \ -workers 1 \ -batch-size 10000 \ -sample-size 1000000+Use
-all-graphsto benchmark every graph discoverable by the selected driver,
+mirroring the dump command's discovery behavior.
+
Benchmark mode performs read-only scans and reports node and relationship<details> <summary>🤖 Prompt for AI Agents</summary>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/retriever/README.mdaround lines 177 - 203, The Bench README is missing
documentation for the user-facing -all-graphs flag wired in runBench. Update the
Bench section in cmd/retriever/README.md to mention that -all-graphs benchmarks
every graph discoverable by the selected driver, matching the behavior already
documented for dump, and place it alongside the existing bench flags description
so the CLI docs stay in sync with main.go.</details> <!-- cr-comment:v1:8ac1582bcf141dbd0d10face --> _Source: Coding guidelines_ </blockquote></details> <details> <summary>cmd/retriever/bench_test.go (1)</summary><blockquote> `1-71`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Missing coverage for `benchNodeBatch`/`benchRelationshipBatch`/`logBenchPhaseProgress`.** These are pure, DB-free functions (compression accounting and progress-threshold logging) that fall squarely within the project's stated testing policy of covering "compression/checksum behavior... and other pure helpers" (per `cmd/retriever/README.md`), yet they have no direct test here. In particular, `benchNodeBatch`/`benchRelationshipBatch` exercise the `compressionNone` short-circuit and the `compressedJSONSize` call path, which would benefit from at least one assertion each. <details> <summary>🤖 Prompt for AI Agents</summary>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/retriever/bench_test.goaround lines 1 - 71, The bench helper test
coverage is missing direct assertions for the pure DB-free helpers
benchNodeBatch, benchRelationshipBatch, and logBenchPhaseProgress. Extend
TestBenchSamplingHelpers or add focused tests to exercise the compressionNone
short-circuit and compressedJSONSize path in benchNodeBatch and
benchRelationshipBatch, and verify logBenchPhaseProgress emits the expected
threshold-based progress behavior using the existing helper symbols.</details> <!-- cr-comment:v1:daeaeb76d18556bd196ba2e3 --> </blockquote></details> <details> <summary>cmd/retriever/compression.go (1)</summary><blockquote> `24-38`: _🎯 Functional Correctness_ | _🔵 Trivial_ | _💤 Low value_ **Rename `compressionNone` if it’s only a sentinel.** `validateCompression`, `newCompressionWriter`, and `newDecompressionReader` intentionally reject it, while `bench` treats it as the uncompressed path. The name makes it look like a general “store uncompressed” codec even though dump/load don’t support that. <details> <summary>🤖 Prompt for AI Agents</summary>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/retriever/compression.goaround lines 24 - 38, Rename the
compressionNonesentinel to something that clearly indicates “no compression
supported” and update its uses invalidateCompression,newCompressionWriter,
newDecompressionReader, and thebenchpath. Keep the behavior the same, but
make the enum/value name reflect that dump/load reject it while benchmarking
treats it as the uncompressed case.</details> <!-- cr-comment:v1:538174c05fc0aaff8cde5c9b --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@cmd/retriever/archive_keys.go:
- Around line 3-11: The archive key handling in archive_keys.go imports
crypto/hpke, which is not available with the current Go toolchain version in
go.mod. Update the project’s Go version/toolchain to 1.26+ if HPKE from the
standard library is required, or replace the crypto/hpke usage in the archive
key code with a compatible external HPKE implementation so the package builds
again.In
@cmd/retriever/load.go:
- Around line 94-152: The load loop in load.go currently appends graph data
without verifying the target is empty, so reruns can duplicate earlier fragments
after a partial failure. Add a preflight check in the main load path before
iterating nextManifest.Graphs (or clear existing graph data there) so load
either fails fast on non-empty state or becomes explicitly idempotent/resumable.
Use the loadResult and graph-loading flow around loadGraphNodes/loadGraphEdges
to place the check where it guards all fragment commits.In
@cmd/retriever/main.go:
- Around line 1-12: The new crypto/hpke import in main makes the module require
Go 1.26+, but the repo still targets an older toolchain, so update the declared
Go version and any CI setup that reads go.mod (including the workflows using
go-version-file) to 1.26 or newer, or remove the hpke usage from
cmd/retriever/main.go if you want to keep the older version. Locate the change
in main and align the module/tooling versions accordingly.In
@cmd/retriever/scrubber.go:
- Around line 587-589: The object_sid handling in scrubber.go can panic when the
value is routed into the shape == "object_sid" branch but does not actually
match objectSIDPattern, because FindStringSubmatch may return nil and matches[1]
is accessed unconditionally. Update the object_sid case in the scrubber logic to
verify the submatch result before using it, and fall back safely when no match
is found; keep the fix localized around the objectSIDPattern match and
pseudonymizeString call.In
@cmd/retriever/types.go:
- Around line 135-201: The manifest validation in validate() is missing a
whitelist check for s.Scrub.Mode, so invalid or corrupted scrub modes can slip
through. Add a scrub mode validation alongside the existing Format, IDStrategy,
and Compression checks, and reject any value other than the supported scrubNone
and scrubFull symbols used by manifest validation. Keep the change in validate()
so readManifest, load, and verify all benefit from the same enforcement.In
@GOLANG_CODING_STANDARD.md:
- Around line 3-4: The intro text in the Go coding standard still references the
old repository name and should be updated to match the current project. Edit the
opening sentence in GOLANG_CODING_STANDARD.md so it uses dawgs instead of gtfo,
keeping the rest of the guidance unchanged.
Nitpick comments:
In@cmd/retriever/bench_test.go:
- Around line 1-71: The bench helper test coverage is missing direct assertions
for the pure DB-free helpers benchNodeBatch, benchRelationshipBatch, and
logBenchPhaseProgress. Extend TestBenchSamplingHelpers or add focused tests to
exercise the compressionNone short-circuit and compressedJSONSize path in
benchNodeBatch and benchRelationshipBatch, and verify logBenchPhaseProgress
emits the expected threshold-based progress behavior using the existing helper
symbols.In
@cmd/retriever/bench.go:
- Around line 294-359:
benchNodeBatchandbenchRelationshipBatchduplicate
the same batch byte-accounting flow: build fragment items, time
compressedJSONSize, and populatebenchPhaseResult. Refactor this shared
logic into a common helper that accepts the phase/type-specific item builder for
fragmentNodeandfragmentEdge, then have both functions delegate to it while
preserving the existingCount, timing, and byte-size fields.- Around line 200-292:
benchNodesandbenchEdgesduplicate the same
keyset-scan loop, so factor the shared flow into a helper that takes the read
and batch handlers as parameters. Keep the common planned-count loop,lastID
advancement check, progress logging, and elapsed-time tracking in one place, and
letbenchNodes/benchEdgesonly supplyreadDatabaseNodesvs
readDatabaseRelationshipsandbenchNodeBatchvsbenchRelationshipBatch.
Ensure the helper still works withgraph.IDand preserves the existing
retrieverInitialProgressAt,retrieverBatchLimit, andlogBenchPhaseProgress
behavior.In
@cmd/retriever/compression.go:
- Around line 24-38: Rename the
compressionNonesentinel to something that
clearly indicates “no compression supported” and update its uses in
validateCompression,newCompressionWriter,newDecompressionReader, and the
benchpath. Keep the behavior the same, but make the enum/value name reflect
that dump/load reject it while benchmarking treats it as the uncompressed case.In
@cmd/retriever/defaults.toml:
- Around line 1-64: The scrubber defaults are duplicated between the TOML
template and the runtime config source, so they can drift over time. Update the
configuration path used by newScrubber and defaultScrubberConfig so both come
from a single source of truth, and make defaults.toml reflect that same
structure instead of independently maintaining values like scrub, classifier,
and value_shapes.In
@cmd/retriever/main_test.go:
- Around line 85-99: The test in main_test.go leaks the temp input directory
when an earlier assertion fails because cleanup is only called in the success
path. Update the prepareLoadInput test flow to guarantee cleanup runs by
registering the returned cleanup function with t.Cleanup (or otherwise ensuring
it executes on failure), and keep the post-cleanup os.Stat check in a separate
ordered cleanup/assertion path so the temp dir deletion is still verified.In
@cmd/retriever/metrics.go:
- Around line 297-302: The appendMetricDiff helper currently stops adding
entries at maxMetricDiffs and silently truncates the diff list, so update it to
append a clear truncation marker when the limit is reached. Adjust
appendMetricDiff (and any Verify path that uses its output) so the returned
slice explicitly indicates there are more differences than shown, making the
Metric verification error message obviously incomplete rather than silently cut
off.In
@cmd/retriever/README.md:
- Around line 177-203: The Bench README is missing documentation for the
user-facing -all-graphs flag wired in runBench. Update the Bench section in
cmd/retriever/README.md to mention that -all-graphs benchmarks every graph
discoverable by the selected driver, matching the behavior already documented
for dump, and place it alongside the existing bench flags description so the CLI
docs stay in sync with main.go.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `f666e22b-9c23-4cdd-8b0a-6a7c17b14d07` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 44b68ea4dee770c469835ec27565e56d8fb9a6ca and 0a498192ab8b478f57468edbc2204d2c6a8a3abf. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (39)</summary> * `GOLANG_CODING_STANDARD.md` * `README.md` * `cmd/benchdiff/compare_test.go` * `cmd/retriever/README.md` * `cmd/retriever/archive_envelope.go` * `cmd/retriever/archive_envelope_test.go` * `cmd/retriever/archive_keys.go` * `cmd/retriever/archive_keys_test.go` * `cmd/retriever/archive_tar.go` * `cmd/retriever/archive_tar_test.go` * `cmd/retriever/bench.go` * `cmd/retriever/bench_test.go` * `cmd/retriever/compression.go` * `cmd/retriever/compression_test.go` * `cmd/retriever/config.go` * `cmd/retriever/config_test.go` * `cmd/retriever/database.go` * `cmd/retriever/database_test.go` * `cmd/retriever/defaults.toml` * `cmd/retriever/dump.go` * `cmd/retriever/dump_test.go` * `cmd/retriever/load.go` * `cmd/retriever/load_test.go` * `cmd/retriever/main.go` * `cmd/retriever/main_test.go` * `cmd/retriever/manifest.go` * `cmd/retriever/manifest_test.go` * `cmd/retriever/metrics.go` * `cmd/retriever/metrics_test.go` * `cmd/retriever/packaging_plan.md` * `cmd/retriever/progress.go` * `cmd/retriever/retriever_integration_test.go` * `cmd/retriever/scrubber.go` * `cmd/retriever/scrubber_test.go` * `cmd/retriever/types.go` * `cmd/retriever/verify.go` * `go.mod` * `query/model.go` * `query/neo4j/neo4j_test.go` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
zinic
force-pushed
the
kpom/retriever-additions
branch
2 times, most recently
from
068719f to
bde58b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/retriever/load_test.go (1)
167-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrong-phase sub-test doesn't isolate the phase-check from checksum validation.
Unlike the count-mismatch case just above (which reuses valid
CompressedBytes/SHA256to isolate solely the count check), this sub-test passes afileManifestwith zero-valueCompressedBytes/SHA256. If checksum validation runs before phase validation, this test would pass due to a checksum failure rather than the intended phase mismatch, masking regressions in phase-validation logic.♻️ Proposed fix
- if _, err := readNodeFragmentFile(dir, compressionGzip, fileManifest{ - Path: "wrong-phase.gz", - Count: 1, - }); err == nil { - t.Fatalf("expected node phase mismatch") - } + wrongPhaseFile := fileManifest{ + Path: "wrong-phase.gz", + Count: 1, + } + if _, err := readNodeFragmentFile(dir, compressionGzip, wrongPhaseFile); err == nil { + t.Fatalf("expected node phase mismatch") + } else if !strings.Contains(err.Error(), "phase") { + t.Fatalf("expected phase mismatch error, got: %v", 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 `@cmd/retriever/load_test.go` around lines 167 - 182, The wrong-phase sub-test in readNodeFragmentFile is not isolating the phase check because the fileManifest uses zero-value CompressedBytes and SHA256, so a checksum failure could satisfy the test before phase validation runs. Update the sub-test to reuse the same valid manifest metadata pattern as the count-mismatch case by providing the correct compressed bytes and checksum for "wrong-phase.gz" while keeping the phase set to phaseEdges, so the assertion specifically verifies the node-phase mismatch path.
🤖 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 `@cmd/retriever/archive_envelope.go`:
- Around line 278-289: promoteUnpackStagingDirectory currently deletes outputDir
before moving stagingDir into place, which can lose the original contents if
os.Rename fails. Update this function to use a safe swap flow: rename outputDir
to a temporary backup first, then rename stagingDir to outputDir, and only
remove the backup after the second rename succeeds; if the staging rename fails,
restore the backup so the force path in promoteUnpackStagingDirectory and its
callers never leaves the target missing.
- Around line 389-403: The size check in writeEncryptedArchiveFrame uses
int(^uint32(0)), which can fail to compile on 32-bit builds. Update the length
guard to compare len(ciphertext) using an unsigned type such as uint64 against a
32-bit maximum, then keep the uint32 cast for frameHeader[1:] only after that
check. Use writeEncryptedArchiveFrame as the reference point when making the
fix.
---
Nitpick comments:
In `@cmd/retriever/load_test.go`:
- Around line 167-182: The wrong-phase sub-test in readNodeFragmentFile is not
isolating the phase check because the fileManifest uses zero-value
CompressedBytes and SHA256, so a checksum failure could satisfy the test before
phase validation runs. Update the sub-test to reuse the same valid manifest
metadata pattern as the count-mismatch case by providing the correct compressed
bytes and checksum for "wrong-phase.gz" while keeping the phase set to
phaseEdges, so the assertion specifically verifies the node-phase mismatch path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35d7e160-4518-412b-bdf7-a3d229758be4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
GOLANG_CODING_STANDARD.mdREADME.mdcmd/benchdiff/compare.gocmd/benchdiff/compare_test.gocmd/retriever/README.mdcmd/retriever/archive_envelope.gocmd/retriever/archive_envelope_test.gocmd/retriever/archive_keys.gocmd/retriever/archive_keys_test.gocmd/retriever/archive_tar.gocmd/retriever/archive_tar_test.gocmd/retriever/bench.gocmd/retriever/bench_test.gocmd/retriever/compression.gocmd/retriever/compression_test.gocmd/retriever/config.gocmd/retriever/config_test.gocmd/retriever/database.gocmd/retriever/database_test.gocmd/retriever/defaults.tomlcmd/retriever/dump.gocmd/retriever/dump_test.gocmd/retriever/load.gocmd/retriever/load_test.gocmd/retriever/main.gocmd/retriever/main_test.gocmd/retriever/manifest.gocmd/retriever/manifest_test.gocmd/retriever/metrics.gocmd/retriever/metrics_test.gocmd/retriever/packaging_plan.mdcmd/retriever/progress.gocmd/retriever/retriever_integration_test.gocmd/retriever/scan.gocmd/retriever/scan_test.gocmd/retriever/scrubber.gocmd/retriever/scrubber_test.gocmd/retriever/types.gocmd/retriever/verify.gogo.modquery/model.goquery/neo4j/neo4j_test.go
✅ Files skipped from review due to trivial changes (2)
- README.md
- GOLANG_CODING_STANDARD.md
🚧 Files skipped from review as they are similar to previous changes (33)
- cmd/benchdiff/compare_test.go
- query/model.go
- query/neo4j/neo4j_test.go
- cmd/retriever/progress.go
- cmd/retriever/archive_keys_test.go
- cmd/retriever/dump_test.go
- cmd/retriever/database.go
- cmd/retriever/config_test.go
- cmd/retriever/archive_tar_test.go
- cmd/retriever/manifest.go
- cmd/retriever/database_test.go
- cmd/retriever/metrics_test.go
- cmd/retriever/README.md
- cmd/retriever/archive_tar.go
- cmd/retriever/main_test.go
- go.mod
- cmd/retriever/verify.go
- cmd/retriever/bench.go
- cmd/retriever/compression_test.go
- cmd/retriever/compression.go
- cmd/retriever/manifest_test.go
- cmd/retriever/types.go
- cmd/retriever/main.go
- cmd/retriever/archive_keys.go
- cmd/retriever/metrics.go
- cmd/retriever/defaults.toml
- cmd/retriever/retriever_integration_test.go
- cmd/retriever/config.go
- cmd/retriever/dump.go
- cmd/retriever/scrubber_test.go
- cmd/retriever/load.go
- cmd/retriever/scrubber.go
- cmd/retriever/archive_envelope_test.go
zinic
force-pushed
the
kpom/retriever-additions
branch
2 times, most recently
from
0574efb to
8ad1d4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/retriever/metrics_test.go (1)
286-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused test helper.
buildFingerprintFixtureis not referenced anywhere else in this file; onlybuildNamedFingerprintFixtureis used by the actual tests.🤖 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/retriever/metrics_test.go` around lines 286 - 289, The test helper buildFingerprintFixture is unused and should be removed from the test file. Keep buildNamedFingerprintFixture as the shared fixture builder used by the tests, and delete buildFingerprintFixture unless you find an actual test reference that should be updated to call it instead.
🤖 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 `@cmd/retriever/bench.go`:
- Around line 200-284: The `workers` parameter in `benchNodes` and `benchEdges`
is only used by `logBenchPhaseProgress`, while `scanEntityBatches` still runs
the same sequential path. Update the batching/handling flow so `workers`
actually controls concurrent scanning or batch processing in the
`benchNodes`/`benchEdges` pipeline, or remove the option entirely if concurrency
is not intended. Use the existing `scanEntityBatches`, `benchNodeBatch`, and
`benchRelationshipBatch` symbols to wire the change in the right place.
---
Nitpick comments:
In `@cmd/retriever/metrics_test.go`:
- Around line 286-289: The test helper buildFingerprintFixture is unused and
should be removed from the test file. Keep buildNamedFingerprintFixture as the
shared fixture builder used by the tests, and delete buildFingerprintFixture
unless you find an actual test reference that should be updated to call it
instead.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1aa49e60-7ada-41e0-ad4b-8b44eac1ad92
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
GOLANG_CODING_STANDARD.mdREADME.mdcmd/retriever/README.mdcmd/retriever/archive_envelope.gocmd/retriever/archive_envelope_test.gocmd/retriever/archive_keys.gocmd/retriever/archive_keys_test.gocmd/retriever/archive_tar.gocmd/retriever/archive_tar_test.gocmd/retriever/bench.gocmd/retriever/bench_test.gocmd/retriever/compression.gocmd/retriever/compression_test.gocmd/retriever/config.gocmd/retriever/config_test.gocmd/retriever/database.gocmd/retriever/database_test.gocmd/retriever/defaults.tomlcmd/retriever/dump.gocmd/retriever/dump_test.gocmd/retriever/load.gocmd/retriever/load_test.gocmd/retriever/main.gocmd/retriever/main_test.gocmd/retriever/manifest.gocmd/retriever/manifest_test.gocmd/retriever/metrics.gocmd/retriever/metrics_test.gocmd/retriever/packaging_plan.mdcmd/retriever/progress.gocmd/retriever/retriever_integration_test.gocmd/retriever/scan.gocmd/retriever/scan_test.gocmd/retriever/scrubber.gocmd/retriever/scrubber_test.gocmd/retriever/types.gocmd/retriever/verify.gogo.modquery/model.goquery/neo4j/neo4j_test.go
✅ Files skipped from review due to trivial changes (3)
- cmd/retriever/README.md
- cmd/retriever/dump_test.go
- GOLANG_CODING_STANDARD.md
🚧 Files skipped from review as they are similar to previous changes (28)
- cmd/retriever/defaults.toml
- query/model.go
- cmd/retriever/scan.go
- query/neo4j/neo4j_test.go
- cmd/retriever/database.go
- cmd/retriever/compression_test.go
- cmd/retriever/scan_test.go
- cmd/retriever/archive_keys_test.go
- cmd/retriever/compression.go
- cmd/retriever/manifest_test.go
- cmd/retriever/main_test.go
- cmd/retriever/main.go
- cmd/retriever/verify.go
- cmd/retriever/archive_tar.go
- cmd/retriever/retriever_integration_test.go
- cmd/retriever/types.go
- cmd/retriever/archive_tar_test.go
- cmd/retriever/manifest.go
- cmd/retriever/archive_keys.go
- cmd/retriever/dump.go
- cmd/retriever/metrics.go
- cmd/retriever/load.go
- cmd/retriever/config_test.go
- cmd/retriever/database_test.go
- cmd/retriever/config.go
- cmd/retriever/scrubber_test.go
- go.mod
- cmd/retriever/scrubber.go
zinic
force-pushed
the
kpom/retriever-additions
branch
from
8ad1d4d to
473c2cc
Compare
zinic
force-pushed
the
kpom/retriever-additions
branch
from
473c2cc to
080b7b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/retriever/archive_envelope.go`:
- Around line 311-315: The cleanup in the archive promotion path should not turn
a successful unpack into a failure if removing the backup directory fails.
Update the backup removal logic in the promotion flow around
os.RemoveAll(backupDir) to log a warning instead of returning an error after the
rename has already succeeded, so unpackEncryptedCollectionArchive still reports
success while preserving visibility into cleanup issues.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b50bea76-ee87-48dc-a484-0fc36c150af9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
GOLANG_CODING_STANDARD.mdREADME.mdcmd/retriever/README.mdcmd/retriever/archive_envelope.gocmd/retriever/archive_envelope_test.gocmd/retriever/archive_keys.gocmd/retriever/archive_keys_test.gocmd/retriever/archive_tar.gocmd/retriever/archive_tar_test.gocmd/retriever/bench.gocmd/retriever/bench_test.gocmd/retriever/compression.gocmd/retriever/compression_test.gocmd/retriever/config.gocmd/retriever/config_test.gocmd/retriever/database.gocmd/retriever/database_test.gocmd/retriever/defaults.tomlcmd/retriever/dump.gocmd/retriever/dump_test.gocmd/retriever/load.gocmd/retriever/load_test.gocmd/retriever/main.gocmd/retriever/main_test.gocmd/retriever/manifest.gocmd/retriever/manifest_test.gocmd/retriever/metrics.gocmd/retriever/metrics_test.gocmd/retriever/packaging_plan.mdcmd/retriever/progress.gocmd/retriever/retriever_integration_test.gocmd/retriever/scan.gocmd/retriever/scan_test.gocmd/retriever/scrubber.gocmd/retriever/scrubber_test.gocmd/retriever/types.gocmd/retriever/verify.gogo.modquery/model.goquery/neo4j/neo4j_test.go
✅ Files skipped from review due to trivial changes (3)
- GOLANG_CODING_STANDARD.md
- README.md
- cmd/retriever/README.md
🚧 Files skipped from review as they are similar to previous changes (32)
- query/model.go
- cmd/retriever/dump_test.go
- cmd/retriever/compression_test.go
- cmd/retriever/archive_keys_test.go
- cmd/retriever/scan_test.go
- cmd/retriever/manifest.go
- query/neo4j/neo4j_test.go
- cmd/retriever/progress.go
- cmd/retriever/verify.go
- cmd/retriever/database_test.go
- cmd/retriever/main_test.go
- cmd/retriever/config_test.go
- cmd/retriever/database.go
- cmd/retriever/archive_tar_test.go
- cmd/retriever/defaults.toml
- cmd/retriever/manifest_test.go
- cmd/retriever/scrubber_test.go
- cmd/retriever/types.go
- cmd/retriever/scan.go
- cmd/retriever/load_test.go
- cmd/retriever/bench.go
- cmd/retriever/retriever_integration_test.go
- cmd/retriever/metrics.go
- cmd/retriever/metrics_test.go
- cmd/retriever/main.go
- cmd/retriever/archive_keys.go
- cmd/retriever/compression.go
- cmd/retriever/archive_tar.go
- cmd/retriever/scrubber.go
- cmd/retriever/load.go
- cmd/retriever/archive_envelope_test.go
- go.mod
Comment on lines
+311
to
+315
| if backupDir != "" { | ||
| if err := os.RemoveAll(backupDir); err != nil { | ||
| return fmt.Errorf("remove output directory backup: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Don't return an error after the promotion has already succeeded.
If os.RemoveAll(backupDir) fails at this point, the staging directory has already been renamed to outputDir — the unpack operation succeeded. Returning an error here misleads the caller into thinking the operation failed, and the deferred os.RemoveAll(stagingDir) in unpackEncryptedCollectionArchive becomes a no-op (the path no longer exists). The backup directory is leaked but the data is correct.
Log a warning instead of propagating a fatal error.
🔧 Proposed fix: warn instead of error on backup cleanup failure
if backupDir != "" {
- if err := os.RemoveAll(backupDir); err != nil {
- return fmt.Errorf("remove output directory backup: %w", err)
- }
+ if err := os.RemoveAll(backupDir); err != nil {
+ slog.Warn("retriever archive backup cleanup failed",
+ slog.String("backup_dir", backupDir),
+ slog.String("error", err.Error()),
+ )
+ }
}
return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if backupDir != "" { | |
| if err := os.RemoveAll(backupDir); err != nil { | |
| return fmt.Errorf("remove output directory backup: %w", err) | |
| } | |
| } | |
| if backupDir != "" { | |
| if err := os.RemoveAll(backupDir); err != nil { | |
| slog.Warn("retriever archive backup cleanup failed", | |
| slog.String("backup_dir", backupDir), | |
| slog.String("error", err.Error()), | |
| ) | |
| } | |
| } |
🤖 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/retriever/archive_envelope.go` around lines 311 - 315, The cleanup in the
archive promotion path should not turn a successful unpack into a failure if
removing the backup directory fails. Update the backup removal logic in the
promotion flow around os.RemoveAll(backupDir) to log a warning instead of
returning an error after the rename has already succeeded, so
unpackEncryptedCollectionArchive still reports success while preserving
visibility into cleanup issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
cmd/retriever, a CLI for exporting and importing live Dawgs graphdatabases as manifest-based OpenGraph-derived collections. Retriever supports
PostgreSQL and Neo4j graph targets, deterministic dump ordering, gzip/zstd
fragment compression, checksum validation, metrics/fingerprint verification,
optional deterministic property scrubbing, read-throughput benchmarking, and
single-file encrypted TAR archive workflows using HPKE/ML-KEM.
This also documents retriever usage, adds a Go coding standard, and updates
query.OrderByso bare criteria default to ascending sort items.Resolves: N/A
Type of Change
Testing
Unit tests added / updated
Integration tests added / updated
Full test suite run (
make test_allwithCONNECTION_STRINGset)make formatgo test ./cmd/retrieverScreenshots (if appropriate):
N/A
Driver Impact
drivers/pg)drivers/neo4j)Checklist
go.mod/go.sumare up to date if dependencies changedSummary by CodeRabbit
retrieverCLI for dumping, loading, verifying, benchmarking, key generation, unpacking, and encrypted archive workflows.