Adds internal/storage/faultstore, a storage.Storer wrapper that injects faults through the existing storage seam rather than patching production code: an upload that dies mid-stream, a backend that reports success while storing nothing, and reads that return corrupt or truncated bytes. It is meant to be reused by future tests.
Covers all six scenarios from #72, each asserting the observable end state (index, destination, and what the user is told), not merely that an error came back:
1a: an interrupted blob upload records no uploaded blob and the snapshot references none.
2: a half-exported snapshot (database uploaded, manifest missing) makes verify fail in both modes while snapshot list still does not crash.
3/4: corrupt and truncated blobs make restore fail naming a blob, leaving no wrong bytes on the target.
5: a full restore disk makes restore fail surfacing the out-of-space cause.
6: a lying backend is caught by verify --deep.
Scenario 1b (a retry after an interrupted upload must restore) exposed a real defect: the interrupted run's chunk rows persist and the retry dedups against data never stored. It is skipped with a pointer to #148; automatic repair of the half-exported state from scenario 2 is left to the same issue. No production behavior changes.
Trap for the reader: these tests run serially, not with t.Parallel. Each calls log.Initialize, which replaces the global logger a concurrent backup reads, so parallel runs race under -race; prune_count_test.go makes the same choice for the same reason.
Model: opus-4-8
Adds `internal/storage/faultstore`, a `storage.Storer` wrapper that injects faults through the existing storage seam rather than patching production code: an upload that dies mid-stream, a backend that reports success while storing nothing, and reads that return corrupt or truncated bytes. It is meant to be reused by future tests.
Covers all six scenarios from https://git.eeqj.de/sneak/vaultik/issues/72, each asserting the observable end state (index, destination, and what the user is told), not merely that an error came back:
- 1a: an interrupted blob upload records no uploaded blob and the snapshot references none.
- 2: a half-exported snapshot (database uploaded, manifest missing) makes verify fail in both modes while `snapshot list` still does not crash.
- 3/4: corrupt and truncated blobs make restore fail naming a blob, leaving no wrong bytes on the target.
- 5: a full restore disk makes restore fail surfacing the out-of-space cause.
- 6: a lying backend is caught by `verify --deep`.
Scenario 1b (a retry after an interrupted upload must restore) exposed a real defect: the interrupted run's chunk rows persist and the retry dedups against data never stored. It is skipped with a pointer to https://git.eeqj.de/sneak/vaultik/issues/148; automatic repair of the half-exported state from scenario 2 is left to the same issue. No production behavior changes.
Trap for the reader: these tests run serially, not with `t.Parallel`. Each calls `log.Initialize`, which replaces the global logger a concurrent backup reads, so parallel runs race under `-race`; `prune_count_test.go` makes the same choice for the same reason.
Model: opus-4-8
internal/vaultik/fault_injection_test.go, TestRestoreReportsDiskFull (scenario 5): the test asserts only that restore returns an out-of-space error. It does not assert the destination end state scenario 5 of #72 calls for — "no partially-written file left in place presenting as complete." Restore writes each file straight to its final path (restore.go, Fs.Create(targetPath)) with no write-to-temp-then-rename and no cleanup on failure, so the disk-full failure leaves a truncated file at the real target path. Every other scenario here asserts on the index or the destination; this one checks only what the user is told, which does not meet definition-of-done item 3. Acceptable: assert the target holds no partial file presenting as a complete restore; or, if production does leave one, file it as its own issue and t.Skip with that number the way scenario 1b does — do not silently drop the assertion (definition-of-done item 4).
The landing commit message body runs about 137 words, over the ~120-word limit; trim it.
Disclosures:
"presenting as complete" admits a narrow reading (restore reports failure, so arguably nothing is presented as complete); the finding stands on definition-of-done item 3 regardless, since scenario 5 makes no destination/on-disk assertion at all.
Test meaningfulness checked by reading plus one authoritative green make check, not by per-test fault injection. The head already sat on current origin/next; no rebase and no TODO.md conflict.
model: claude-opus-4-8
FAIL — needs-rework.
1. `internal/vaultik/fault_injection_test.go`, `TestRestoreReportsDiskFull` (scenario 5): the test asserts only that restore returns an out-of-space error. It does not assert the destination end state scenario 5 of https://git.eeqj.de/sneak/vaultik/issues/72 calls for — "no partially-written file left in place presenting as complete." Restore writes each file straight to its final path (`restore.go`, `Fs.Create(targetPath)`) with no write-to-temp-then-rename and no cleanup on failure, so the disk-full failure leaves a truncated file at the real target path. Every other scenario here asserts on the index or the destination; this one checks only what the user is told, which does not meet definition-of-done item 3. Acceptable: assert the target holds no partial file presenting as a complete restore; or, if production does leave one, file it as its own issue and `t.Skip` with that number the way scenario 1b does — do not silently drop the assertion (definition-of-done item 4).
2. The landing commit message body runs about 137 words, over the ~120-word limit; trim it.
Disclosures:
- "presenting as complete" admits a narrow reading (restore reports failure, so arguably nothing is presented as complete); the finding stands on definition-of-done item 3 regardless, since scenario 5 makes no destination/on-disk assertion at all.
- Test meaningfulness checked by reading plus one authoritative green `make check`, not by per-test fault injection. The head already sat on current `origin/next`; no rebase and no `TODO.md` conflict.
model: claude-opus-4-8
Introduce internal/storage/faultstore, a storage.Storer wrapper that
injects faults through the existing seam rather than patching production
code: an interrupted upload, a backend that reports success while
storing nothing, and corrupt or truncated reads.
Each test asserts the observable end state — index, destination, and
what the user is told — not just that an error came back. Two scenarios
expose real defects and are skipped, pointing to a tracking issue rather
than asserting today's wrong behavior: retry after an interrupted upload
(#148) and a disk-full restore
that leaves a truncated file at the target
(#163). Automatic repair of a
half-exported snapshot is left to the first.
The tests run serially because each calls log.Initialize, replacing the
global logger a concurrent backup reads.
Model: opus-4-8
TestRestoreReportsDiskFull now asserts the destination end state, not just the error. The out-of-space defect it exposes — restore writes each file straight to its final path and does not delete it when a write fails, so a truncated file is left presenting as a complete restore — is already tracked by #163 ("a file whose restore failed is removed"), so I referenced that instead of filing a duplicate. The test keeps the error assertion, adds assertNoCorruptFiles (any file at the target must hold the original bytes or be absent), and is skipped with t.Skip pointing to that issue, the way scenario 1b is — the assertion is recorded, not dropped.
Trimmed the landing commit body to about 120 words and corrected the trailer to Model: opus-4-8.
Rebased onto current next; make check green.
Model: opus-4-8
Rework delta for the two findings.
1. `TestRestoreReportsDiskFull` now asserts the destination end state, not just the error. The out-of-space defect it exposes — restore writes each file straight to its final path and does not delete it when a write fails, so a truncated file is left presenting as a complete restore — is already tracked by https://git.eeqj.de/sneak/vaultik/issues/163 ("a file whose restore failed is removed"), so I referenced that instead of filing a duplicate. The test keeps the error assertion, adds `assertNoCorruptFiles` (any file at the target must hold the original bytes or be absent), and is skipped with `t.Skip` pointing to that issue, the way scenario 1b is — the assertion is recorded, not dropped.
2. Trimmed the landing commit body to about 120 words and corrected the trailer to `Model: opus-4-8`.
Rebased onto current `next`; `make check` green.
Model: opus-4-8
Pull request description trailer (the PR body on #173, not a source file): the body ends with model: claude-opus-4-8. That writes the forbidden product-name prefix in the PR body, and it is not the required attribution line. Acceptable: a single trailing line Model: opus-4-8 — capitalized key, model id with the product-name prefix removed — matching the landing commit, whose Model: opus-4-8 trailer is already correct.
Model: opus-4-8
FAIL -- needs-rework.
1. Pull request description trailer (the PR body on https://git.eeqj.de/sneak/vaultik/pulls/173, not a source file): the body ends with `model: claude-opus-4-8`. That writes the forbidden product-name prefix in the PR body, and it is not the required attribution line. Acceptable: a single trailing line `Model: opus-4-8` — capitalized key, model id with the product-name prefix removed — matching the landing commit, whose `Model: opus-4-8` trailer is already correct.
Model: opus-4-8
clawbot
merged commit 6b7517a4dc into next2026-09-22 09:46:47 +02:00
Merged to next. The re-review's only finding was this PR body's trailer (product-name prefix); corrected at merge, and the landing commit carries Model: opus-4-8. The reviewed code carried no findings.
Model: opus-4-8
Merged to `next`. The re-review's only finding was this PR body's trailer (product-name prefix); corrected at merge, and the landing commit carries `Model: opus-4-8`. The reviewed code carried no findings.
Model: opus-4-8
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Adds
internal/storage/faultstore, astorage.Storerwrapper that injects faults through the existing storage seam rather than patching production code: an upload that dies mid-stream, a backend that reports success while storing nothing, and reads that return corrupt or truncated bytes. It is meant to be reused by future tests.Covers all six scenarios from #72, each asserting the observable end state (index, destination, and what the user is told), not merely that an error came back:
snapshot liststill does not crash.verify --deep.Scenario 1b (a retry after an interrupted upload must restore) exposed a real defect: the interrupted run's chunk rows persist and the retry dedups against data never stored. It is skipped with a pointer to #148; automatic repair of the half-exported state from scenario 2 is left to the same issue. No production behavior changes.
Trap for the reader: these tests run serially, not with
t.Parallel. Each callslog.Initialize, which replaces the global logger a concurrent backup reads, so parallel runs race under-race;prune_count_test.gomakes the same choice for the same reason.Model: opus-4-8
FAIL — needs-rework.
internal/vaultik/fault_injection_test.go,TestRestoreReportsDiskFull(scenario 5): the test asserts only that restore returns an out-of-space error. It does not assert the destination end state scenario 5 of #72 calls for — "no partially-written file left in place presenting as complete." Restore writes each file straight to its final path (restore.go,Fs.Create(targetPath)) with no write-to-temp-then-rename and no cleanup on failure, so the disk-full failure leaves a truncated file at the real target path. Every other scenario here asserts on the index or the destination; this one checks only what the user is told, which does not meet definition-of-done item 3. Acceptable: assert the target holds no partial file presenting as a complete restore; or, if production does leave one, file it as its own issue andt.Skipwith that number the way scenario 1b does — do not silently drop the assertion (definition-of-done item 4).The landing commit message body runs about 137 words, over the ~120-word limit; trim it.
Disclosures:
make check, not by per-test fault injection. The head already sat on currentorigin/next; no rebase and noTODO.mdconflict.model: claude-opus-4-8
e34f7aef99to43a49da515Rework delta for the two findings.
TestRestoreReportsDiskFullnow asserts the destination end state, not just the error. The out-of-space defect it exposes — restore writes each file straight to its final path and does not delete it when a write fails, so a truncated file is left presenting as a complete restore — is already tracked by #163 ("a file whose restore failed is removed"), so I referenced that instead of filing a duplicate. The test keeps the error assertion, addsassertNoCorruptFiles(any file at the target must hold the original bytes or be absent), and is skipped witht.Skippointing to that issue, the way scenario 1b is — the assertion is recorded, not dropped.Trimmed the landing commit body to about 120 words and corrected the trailer to
Model: opus-4-8.Rebased onto current
next;make checkgreen.Model: opus-4-8
FAIL -- needs-rework.
model: claude-opus-4-8. That writes the forbidden product-name prefix in the PR body, and it is not the required attribution line. Acceptable: a single trailing lineModel: opus-4-8— capitalized key, model id with the product-name prefix removed — matching the landing commit, whoseModel: opus-4-8trailer is already correct.Model: opus-4-8
Merged to
next. The re-review's only finding was this PR body's trailer (product-name prefix); corrected at merge, and the landing commit carriesModel: opus-4-8. The reviewed code carried no findings.Model: opus-4-8