Make the preservation guard's wiring testable, and pin all three call sites - #23
Conversation
No behaviour change: the block that resolved the destructive authority, pre-flighted the preservation guard over the whole plan, and ran the two per-artifact delete loops moves verbatim out of pruneBackups into an exported applyPrunePlan, which pruneBackups now tail-calls. The reason is testability of the guard's WIRING, not tidiness. All three preservation call sites in the apply phase — the pre-flight loop, the assertNotPreserved backstop inside deletePruneArtifact, and both "PreservationGuardError is re-thrown, not collected into errors[]" branches — could each be deleted with bun test staying at 100 pass / 0 fail, because retention pins every marked run before the expiry plan is assembled, so no marked artifact can reach the deletion boundary through pruneBackups. That symmetry between the two layers is correct and is deliberately left intact; what was missing was any way to drive the deletion phase with a plan this module did not build, which is the only caller a backstop exists for. applyPrunePlan is not added to src/runtime.ts's export list, so it stays out of the published API surface. It relaxes nothing: the run-level authority is still resolved before the first delete, and every artifact still passes resolveDestructiveAuthority, assertNotPreserved, and the containment assertions at the boundary. Refs: todos 9c93913b
Three mutations each left bun test at 100 pass / 0 fail on main d32e5cb: deleting the whole-plan pre-flight loop, deleting assertNotPreserved inside deletePruneArtifact, and deleting both "PreservationGuardError is re-thrown" branches. The third re-introduces exactly the downgrade-into-errors[] behaviour #21 exists to prevent. The guard's EFFECT was heavily covered; its invocation was not covered at all, so every call site was free to be removed. Six tests in tests/prune-guard-wiring.test.ts drive applyPrunePlan with a real plan, real files, and a mkdtemp home, and each one asserts a property that only the call site under test provides: - pre-flight: the marked artifact is planted LAST and the evidence is that the two ordinary artifacts AHEAD of it survive. "It still throws" cannot be the assertion, because deleting the loop leaves the refusal intact and merely moves it after two real deletions. - boundary: the marked artifact is in a decision but absent from plan.expiredArtifacts, so the pre-flight is blind to it. That is a stale or foreign plan — the only caller a backstop exists for — not a narrowed guard. Carries no preservedBy, so the marker must be re-derived from sourcePath. - first re-throw: two expired runs, refusal in the one reached first; the evidence is that the SECOND run is untouched. A downgraded refusal still produces a message, so only the fate of the next backup separates them. - second re-throw: a marked catalog manifest planted ahead of the real one in the catalog loop, which has no break, so the real manifest surviving is the evidence. - negative control: an ordinary artifact failure is still collected into errors[] and the next backup still runs, so "stops the run" stays scoped to the three named error types. - fidelity control: an UNMODIFIED plan through applyPrunePlan deletes exactly what prune --apply deletes on an identical catalog, so the hand-assembled apply input cannot be the reason a refusal fires. The symmetry between the retention layer and the deletion boundary — both reading source name, source path and archive name — is deliberately left untouched. It is what makes the boundary unreachable through pruneBackups, and weakening it to create a reachable path would undo #21's most important fix. No live home is resolved anywhere: every BackupHome is built from an explicit mkdtemp path, so selectedBy is "option" and every deletion target is outside the live installation. Refs: todos 9c93913b
Splitting the deletion phase out of pruneBackups opened one surface that was closed while it was inline: `if (!options.apply) return result` stood between a dry-run plan and the first delete, and a separately callable phase can be handed one. `prune --plan` is the read an operator makes immediately before deleting, so acting on a dry-run plan would delete precisely what that operator was just shown as safe. Refused before the authority is resolved, so nothing is even declared, and unreachable from pruneBackups, which only calls this with dryRun already false. The test asserts the refusal, that no file was touched, that no authority was recorded, and — as the control — that the same plan marked as the apply-run plan it would be does delete all three artifacts. Found by self-review of this branch, not by a reviewer. Refs: todos 9c93913b
Update: third commit
|
| mutation | rc | result | failing test(s) |
|---|---|---|---|
| M1 — delete the pre-flight loop | 1 | 106 pass / 1 fail | the pre-flight refuses a plan containing a marked artifact before anything is deleted |
M2 — delete assertNotPreserved in deletePruneArtifact |
1 | 104 pass / 3 fail | the deletion boundary refuses a marked artifact the pre-flight never saw; a preservation refusal at the boundary stops the run instead of being collected into errors[]; a preservation refusal on a catalog manifest also stops the run |
M3 — delete both if (error instanceof PreservationGuardError) throw error; |
1 | 104 pass / 3 fail | same three |
M4 — disable the new dryRun refusal |
1 | 106 pass / 1 fail | applyPrunePlan refuses a plan still marked dryRun |
null (reverted, git diff HEAD empty) |
0 | 107 pass / 0 fail | — |
Needle count asserted N→0 and replacement-marker count == N before every write; tree confirmed clean after every revert; grep -c -i 'timed out' = 0 in all four mutation logs. bun run check → rc=0.
Two things a reviewer should look at rather than take on trust
- The pre-flight vets
plan.expiredArtifacts, not the decisions' own lists. That is pre-existing behaviour from Refuse to prune preservation-marked archives, and let a source capture git history #21 and is unchanged here — but it is precisely why the boundary matters, and it is what these tests exploit to reach it. Tightening the pre-flight to iterate the decisions would be strictly safer and would make the backstop unreachable again, so it is a deliberate non-change, not an oversight. Worth a decision from the reviewer rather than from me. - M2 and M3 are co-killed by the same three tests. No single test separates "the boundary raises" from "the raise is not downgraded"; that needs a
PreservationGuardErrorraised inside the delete loop by something other than the boundary's own guard, i.e. an injected stub deleter inapplyPrunePlan's signature. Both mutations die, which is the acceptance criterion, but the discrimination gap is real.
Merge-conflict parity with #22 re-verified at 5573e5d: identical conflict set and identical single src/backup.ts hunk whether #22's head is merged into main d32e5cb or into this branch. Still zero new conflicts. Live catalog re-verified byte-identical (62 manifests, 234774 bytes, six md5 recipes unchanged).
Adversarial review — independent reproduction, PR #23 @ 5573e5dReviewer worked in a fresh clone ( Verdict: APPROVE-WITH-FIXES. Every measurable claim in the PR reproduces exactly. Two fixes are wanted before merge, both measured below, and one of them changes the answer to the question the author escalated. 1. What reproduced, independentlySurvivors on unmodified main
At PR head
Matches the author's matrix line for line. Refactor is verbatim — proved mechanically, not read. Extracting main's apply phase (the 92 lines after Not published API — confirmed. The fidelity control is not vacuous. Breaking only the test-side apply-input assembly ( Zero new conflicts against #22 — confirmed, against #22's current head Integrity, with the recipe (the circulating Secrets scan over 2. FIX 1 (blocking) — the boundary's pin is contingent on the pre-flight staying looseTests 3 and 4 plant the marked artifact first ( Measured: tighten the pre-flight to also iterate each decision's own list — a change the author explicitly flagged as "strictly safer", i.e. the change a future engineer is being invited to make: So once the pre-flight is tightened, deleting the deletion backstop and both re-throws adds zero new failures — three of the four call sites become survivors again and the suite collapses to one test. Worse, the one test that does break is The reason test 5 is the one that breaks is also the fix: test 5 plants the marked artifact behind two ordinary ones and asserts they were deleted ( Fix: in tests 3 and 4, stop planting the marked artifact first — put at least one ordinary artifact ahead of it in the same decision and assert that artifact was deleted before the refusal. That makes "the refusal came from the boundary, after the pre-flight passed" a property of the test rather than of the pre-flight's current scope. Cheap, local to 3. FIX 2 (should-fix, measured zero-cost) — one position-only guard was not restored
Measured fix, and it is free — re-derive the matcher inside const { plan: result, home, destinations: artifactDestinations, confirmHome } = input;
const preservation = compilePreservationMatcher(loadConfig(home).policy.preserveMarkers ?? []);
4. Ruling on the pre-flight-scope trade (the author's escalation): DO NOT TIGHTENNot "safer but untestable" — on the evidence above, tightening it as things stand is net less safe, because it re-opens three of the four call sites (§2). Three further reasons:
Note for the record: "pre-existing #21 behaviour, unchanged" is true of production but understates it. On main the two lists could not diverge at all, because there was no way to hand 5. On the M2/M3 co-kill gap: agreed — and the result is better than statedSplitting the two re-throws into separate mutations (the author only measured them together):
So all four call sites have distinct kill signatures (M1 106/1 · M2 104/3 · M3a 105/2 · M3b 106/1 catalog · M4 106/1 dryRun) — each re-throw is individually pinned, which the PR's own write-up does not claim. The only residual indistinguishability is M2 vs M3-both (identical 104/3, same three names), and that is intrinsic: deleting the boundary assert removes the source of the exception the re-throws classify, so no test can observe the classification without the raise. I agree with the author's trade — an injected stub deleter would add an injection surface to the very function whose job is to refuse caller-supplied inputs, which is worse than an unavoidable overlap. 6. Merge order#23 first, then rebased #22. Unverifiable: parity against the rebased #22. PR #22 still reports head No CHANGELOG entry: correct, leave it. Reviewer did not modify or merge this branch. |
…ething refused" The two tests that target `deletePruneArtifact`'s backstop and the first `PreservationGuardError` re-throw planted their marked artifact FIRST in the decision, so their evidence was "it throws, the marked file survives, nothing behind it was deleted, errors[] is empty". A whole-plan pre-flight refusal satisfies all four identically, so nothing in either test attributed the refusal to the call site it named. Measured: with the pre-flight tightened to iterate each decision's own expiredArtifacts, deleting the backstop AND both re-throws added zero new failures — the probe alone, probe+M2 and probe+M3both all gave rc=1 at 106 pass / 1 fail with the identical single failing name. Three of the four call sites became survivors again. Both tests now plant the marked artifact BEHIND an ordinary artifact of the same decision and assert that ordinary artifact WAS deleted before the refusal. Only a refusal raised inside the delete loop can leave a partially deleted run; the pre-flight, by construction, deletes nothing. That is the technique the catalog-manifest test already used, which is exactly why it was the one test the probe broke. Artifact order after each splice is asserted, not assumed, since the attribution rests on exactly one artifact preceding the marked one. No production code changed.
…cept one
The second guard the extraction lost by POSITION. While the deletion phase was
inline, `compilePreservationMatcher(config.policy.preserveMarkers)` stood
upstream of the delete loops and no caller could get past it — the construction
is fail-closed and additive, so configuration can widen the marker set and can
never narrow it. Once the phase became separately callable,
`PruneApplyInput.preservation` was a caller-supplied `PreservationMatcher`, and
that is an interface: `{ markers: [], match: () => [] }` type-checks and
silently disarms all four preservation call sites in the phase at once. Same
class of loss as the `dryRun` gate restored in 5573e5d.
The matcher is now compiled inside `applyPrunePlan` from the home's own stored
policy, and the field is gone from `PruneApplyInput` and from the `pruneBackups`
call site. No production behaviour change: `pruneBackups` passed exactly this
matcher.
Compiled AFTER the `dryRun` refusal on purpose — `loadConfig` writes a default
config when none exists, so a plan this phase is refusing to act on must not
cause a write to the home it just refused.
Two tests pin it, one per layer, each forcing a disarmed matcher past the type
with a cast: the whole-plan pre-flight (nothing deleted) and the deletion
boundary (marked artifact absent from the plan summary, planted behind an
ordinary artifact so the refusal is attributed to the delete loop). Both layers
are pinned separately because "they share one const" is a structural argument,
and a structural argument is what left this matcher and the dryRun gate
unguarded to begin with.
Review fixes applied — both of them, with the verification, at
|
| run | rc | pass/fail | failing name(s) |
|---|---|---|---|
| baseline | 0 | 107 / 0 | — |
| PROBE-B | 1 | 106 / 1 | a preservation refusal on a catalog manifest also stops the run |
| PROBE-B + M2 | 1 | 106 / 1 | identical single name |
| PROBE-B + M3both | 1 | 106 / 1 | identical single name |
The fix, as specified: tests 3 and 4 now plant the marked artifact behind an ordinary
artifact of the same decision (splice(1, 0, ...) instead of unshift) and assert that
ordinary artifact was deleted before the refusal — the technique test 5 already used.
Artifact order after each splice is asserted, not assumed. Test-file only; no production
code in 233ac8d.
Re-running the probe at 233ac8d:
| run | rc | pass/fail | failing names |
|---|---|---|---|
| PROBE-B | 1 | 104 / 3 | boundary test, re-throw#1 test, catalog test |
| PROBE-B + M2 | 1 | 104 / 3 | same three |
| PROBE-B + M3both | 1 | 104 / 3 | same three |
| M2 alone (no probe) | 1 | 104 / 3 | unchanged — the three still die |
The single failing name is gone: 106/1-with-one-name became 104/3-with-three-names. The
two-step failure the review found — tighten the pre-flight, CI stays green, now delete
three call sites and CI still stays green — is broken at step one. Tightening is no longer
a green change.
But the literal criterion in the brief ("M2 and both M3 variants must now produce distinct
additional failures") is not satisfiable by any test suite, and here is the measurement
that proves it rather than an argument that asserts it. I instrumented the boundary's
assertNotPreserved to append every refusal it raises to a file, and ran the suite twice:
- boundary instrumented, no probe → suite rc=0, 109/0, and 4 refusals recorded, one
per test that names the boundary:archive .../bkp_marked_pathonly-archive.tgz,
archive .../bkp_marked_predelete-archive.tgz,
catalog-manifest .../bkp_marked_predelete.json,
archive .../bkp_marked_disarm_boundary-archive.tgz. - boundary instrumented, with PROBE-B → suite rc=1, 105/4, and 0 refusals recorded
(the file is never even created).
Under a tightened pre-flight the boundary is never reached. It iterates exactly the list
both delete loops iterate, assertNotPreserved is a pure function of (artifact, matcher),
and after Fix 2 both layers use the same matcher — so anything the boundary would refuse has
already been refused before the loop is entered. M2 and M3 then delete unreachable code,
which cannot change an observable. That is the reviewer's own stated reason for ruling
against tightening ("would ALSO make the backstop unreachable again"), now measured rather
than inferred.
Fix 2 (SHOULD-FIX) — the position-only matcher guard, restored
PruneApplyInput.preservation is gone. applyPrunePlan compiles its own from the home's
stored policy, and pruneBackups no longer passes one. Placed after the dryRun
refusal, not at the destructure as the brief's snippet showed — one-line deviation, stated
deliberately: loadConfig writes a default config when none exists (src/config.ts:329-333),
so a plan this phase is refusing to act on must not cause a write to the home it refused.
Behaviour is identical for every valid input.
Two tests pin it, one per layer — the pre-flight (nothing deleted) and the deletion
boundary (marked artifact absent from the plan summary, planted behind an ordinary one so
the refusal is attributed to the delete loop). Both force a disarmed
{ markers: [], match: () => [] } past the type with a cast. Both layers are pinned
separately on purpose: "they share one const" is a structural argument, and a structural
argument is what left this matcher and the dryRun gate unguarded in the first place.
The mutation that proves it (M5) — restore the caller-supplied read
((input as { preservation?: PreservationMatcher }).preservation ?? compilePreservationMatcher(...)):
bun test rc=1, 107 pass / 2 fail, failing exactly the two new tests and nothing else.
Correction to the review, measured: the claim "typecheck rc=0, bun test rc=0 at 107
pass / 0 fail, no test modified" is not reproducible. Dropping preservation from
PruneApplyInput makes the existing helper applyInputFor a type error, because it supplies
that field:
tests/prune-guard-wiring.test.ts(549,32): error TS2353: Object literal may only specify
known properties, and 'preservation' does not exist in type 'Omit<PruneApplyInput, "plan">'.
bun run typecheck rc=2, measured by restoring the field on the fixed tree. Fix 2
requires a test change. (Silver lining, and it is load-bearing: excess-property checking
means a caller cannot pass a disarmed matcher by accident through an object literal — only
by a deliberate cast, which is what the two new tests do.)
Full mutation matrix at 85c0155 — every call site keeps a distinct signature
Needle discipline throughout: exact expected line text asserted before each write, the
written line compared against the intended text, marker count == 1, revert from a committed
tree, git status --porcelain empty and HEAD unmoved after every round. Unpiped rc.
| # | mutation | rc | pass/fail | failing names |
|---|---|---|---|---|
| — | null control | 0 | 109 / 0 | — |
| M1 | delete the whole-plan pre-flight loop | 1 | 107 / 2 | pre-flight test, disarm-pre-flight test |
| M2 | delete assertNotPreserved in deletePruneArtifact |
1 | 105 / 4 | boundary, re-throw#1, catalog, disarm-boundary |
| M3a | delete the FIRST re-throw only | 1 | 106 / 3 | boundary, re-throw#1, disarm-boundary |
| M3b | delete the SECOND re-throw only | 1 | 108 / 1 | catalog |
| M3both | delete both re-throws | 1 | 105 / 4 | as M2 |
| M4 | disable the dryRun refusal |
1 | 108 / 1 | dryRun test |
| M5 | restore the caller-supplied matcher | 1 | 107 / 2 | the two disarm tests |
grep -c -i 'timed out' == 0 in every log, so no failure was contention-shaped. Only M2 vs
M3both remain indistinguishable, which is intrinsic and already agreed in the review.
Method disclosure, because it is exactly the failure mode the harness exists to prevent:
my first M4 attempt passed \&\& through printf, so the replacement text itself arrived
corrupted and src/backup.ts stopped parsing — 0 pass / 9 fail, Ran 9 tests across 9 files.
That is a harness gap, not a survivor: the harness verifies the written line equals the
replacement it was handed, but cannot know the intended text was already mangled upstream.
Caught by the shape of the result (9 files failing to load, not tests failing) and re-run as
M4v2 → 108/1 as tabled. Anyone reusing /tmp/pr23fix-mutate.sh should pass replacement text
through a shell variable and %s, never inline in a printf format string.
The pre-flight is NOT tightened — the ruling is carried, unchanged
Left exactly as it is, for the three reasons in the review, and the boundary-reachability
measurement above is now the hard evidence for reason (1): tightening is net less safe
because it makes three call sites unobservable. The loose-run-level / tight-per-artifact
split mirrors resolveDestructiveAuthority (run-level at src/backup.ts:398 on main,
per-artifact at :1240), and the residual risk is atomicity, not preservation.
Carrying the review's correction: the author's "pre-existing #21 behaviour, unchanged" is
true of production but understates it — on main the two lists could not diverge,
because no caller could hand pruneBackups a plan. The declared-vs-actual distinction is
new with this branch. The better hardening — refusing a plan whose declared
expiredArtifacts disagrees with its decisions — is incompatible with the current seam, so
it is filed, not built.
No CHANGELOG.md entry, unchanged and deliberate.
Live-catalog integrity — with the recipes, before and after all work
find $HOME/.hasna/backup/manifests -type f -printf '%P %s\n' | LC_ALL=C sort | md5sum
-> dc00e84fd543e3610fbb67c76296899a (before == after)
find $HOME/.hasna/backup/manifests -type f | LC_ALL=C sort | xargs md5sum | md5sum
-> 84aacd70b5313b70ed913a09a96fc0ef (before == after)
count=62 bytes=234774
find $HOME/.hasna/backup/local | wc -l -> 171 (before == after)
Note for anyone reproducing the 171: it is the recursive find count. ls -1 on the
same directory gives 30 (top-level run dirs); find -type f gives 140. The circulating
7e4d2912f84e55ee369ad7fd1dfcd5c2 remains unreproducible and should be retired in favour of
the two recipes above.
Secrets scan over git diff d32e5cb..85c0155 → no match (rc=1). No Co-Authored-By.
…-target guard
Adversarial review round 2 returned APPROVE-WITH-CONDITIONS from two reviewers.
This closes every blocking condition, plus the new `prune --help` defect
(todos 9a2b3a22) which the pre-dispatch help gate already covered.
REFUSE A DUPLICATE --compare-source KEY (reviewer 1, worst finding). Marking the
flag repeatable fixed the argv layer, but a duplicate KEY still let the last
mapping win: `--compare-source t=<decoy> --compare-source t=<real>` returned rc=0
ok=true missing=0, while the reverse order returned rc=1 — the same input, order
dependent, one of them a GREEN verification, and nothing said about the discarded
mapping. Which tree an archive is diffed against decides what the proof means.
SPEC-DRIFT SCANNER NOW COVERS listFlag. The alternation omitted it, leaving the
readers for ALL FOUR repeatable flags unscanned — the highest-risk class, since an
undeclared flag fails closed at runtime. Planting `listFlag(parsed,
"sentinel-list")` left the suite green at 180/180 while the flag exited 1. The
reader list is now asserted to match the helpers the CLI actually defines, so a
fifth reader cannot silently narrow the scan again.
assertKeepsSecrets IS NOW ACTUALLY PINNED, and its docblock no longer overstates.
It was live (broadening the subtraction made it refuse) but untested, while the
comment claimed "a test pins it" and that the SECRETS_EXCLUDES-subset property
"is itself asserted". Neither was true. Added: the subset property, that the
modifier removes exactly `.git` while retaining every secrets pattern, that the
guard throws for each secrets pattern, and that it does NOT throw for the real
subtraction. Exported so it can be asserted directly.
Deleting the guard's CALL still left the suite green, because the guard only fires
when the invariant is already violated — so the call site is asserted at the
SOURCE level, and the comment now says that is what it is rather than implying a
behavioural test.
INLINE `=` NO LONGER TRUNCATES. `split("=", 2)` discarded everything past the
second `=`: `--name=a=b=c` stored "a", `--exclude='p=1,q=2'` stored ["p"]. Two
patterns silently reduced to one at rc=0 — the same class as the repeated-flag
collapse. Now splits on the first `=` and keeps the remainder. `--home=`,
`--preserve-markers=` and inline `--home=<path>` all still behave.
ONE SHARED RESTORE-TARGET GUARD, src/lib/restore-target.ts, used by BOTH
`verify --deep` and `restore apply`, which had drifted exactly as a duplicated
rule does: `--deep` refused a 0777 target while `restore apply --target <the same
path> --yes` returned rc=0 and left `creds.env` at mode 664.
- CONTAINMENT, new and the more serious half: a target inside the backup home (or
containing it) is refused on both commands, with no override. Measured before:
`verify --deep --target <home>/manifests` rc=0, leaving a restored directory
among the manifest files — content mixed into the catalog `verify` and `restore`
both read. Symlink-resolved, because a symlink into the home reads as outside it
lexically.
- EXPOSURE differs by command, deliberately and recorded rather than hidden.
`--deep` refuses (throwaway scratch tree the tool owns; 0700 is natural).
`restore apply` WARNS (the caller's destination for their own data; an ordinary
`mkdir` is 0755). Refusing in both places was my first attempt and it broke
tests/backup.test.ts:201, whose target is a plainly-created directory — a guard
that fails the common case only trains everyone to pass the override.
`verify` stays `mutating: false` because plain verify only reads, but its notes now
say `--deep` WRITES a restored copy, and the read-only-commands sentence in
UnnamedBackupHomeError is qualified the same way.
The `plan` note claiming the pre-flight "names every pattern that drops content"
now says it names the patterns and is an ESTIMATE using the inventory's matcher
rather than GNU tar's globber — the CHANGELOG already carried that caveat and the
spec contradicted it.
Mutation-proven, each with needle counts asserted changed: duplicate-key refusal +
`=` truncation planted together -> 9 pass / 2 fail; containment disabled -> 12 pass
/ 3 fail; the reviewer's `listFlag` plant now fails the drift test naming
"sentinel-list" (it previously left 180/180 green); deleting the
assertKeepsSecrets call now fails the wiring assertion (previously green);
broadening the subtraction to drop `.env` -> 14 pass / 2 fail.
Rebased onto b2c4809 (PR #23 merged) with ZERO conflicts.
Gates: bun test --timeout 60000 rc=0, 206 pass / 0 fail / 17 files / 1531 expect().
typecheck rc=0, contracts:conformance rc=0, build rc=0.
1 participant
Closes the test gap filed as todos
9c93913b: on mergedmaind32e5cb, each of the three preservation-guard call sites in the prune apply phase could be deleted withbun testat rc=0, 100 pass / 0 fail. The third of them re-introduces exactly the downgrade-into-errors[]behaviour #21 exists to prevent.This is not a behaviour defect. The guard's effect is heavily covered; its wiring was covered nowhere, so nothing stopped a future refactor from removing every call site.
Why the wiring was untestable, and what is deliberately NOT changed
After #21 both layers read the same three fields — source name, source path, archive name. Retention therefore pins every run the deletion boundary would refuse, so no marked artifact can enter an expiry plan
pruneBackupsbuilt, and the boundary is structurally unreachable through the public path. That is the point of a backstop and it is #21's most important fix: the reviewer found the boundary matching on name + archive name while retention matched name + path + archive name, so a path-only-marked live source fell straight through andassertNotPreservedprovably did not throw. One of the seven live marked sources was in exactly that state.That symmetry is left exactly as it is. Nothing here narrows either layer to manufacture a reachable path.
The change
refactor: split the prune deletion phase into applyPrunePlan— the block that resolves the destructive authority, pre-flights the guard over the whole plan, and runs the two per-artifact delete loops moves verbatim out ofpruneBackupsinto an exportedapplyPrunePlan, whichpruneBackupsnow tail-calls. Behaviour-neutral: the suite stayed at 100 pass / 0 fail across the refactor commit.That makes it possible to drive the deletion phase with a plan this module did not build — which is not a test convenience, it is the only caller the backstop exists for ("a stale plan, or a plan built by other code").
applyPrunePlanis not added tosrc/runtime.ts's export list, so it stays off the published API surface (package.jsonexportsonly maps.→dist/index.js). It relaxes nothing: the run-level authority is still resolved before the first delete, and every artifact still passesresolveDestructiveAuthority,assertNotPreserved, and the containment assertions at the boundary.The tests, and why each assertion is the one that discriminates
tests/prune-guard-wiring.test.ts— 6 tests, real plans, real files,mkdtemphome. EveryBackupHomeis built from an explicit path, soselectedByis"option"and every target is outside the live installation. No default home is ever resolved.plan.expiredArtifacts— a plan whose summary and decisions have drifted apart. NopreservedBy, so the marker must be re-derived fromsourcePath.errors[]break; evidence is the real manifest surviving.errors[]and the next backup still runs, so "stops the run" stays scoped to the three named error types.applyPrunePlandeletes exactly whatprune --applydeletes on an identical catalog, so the hand-assembled apply input cannot be why a refusal fires.Mutation evidence (all commands unpiped; needle count asserted
N→0and replacement-marker count== Nbefore writing)Reproduced on unmodified
main d32e5cbfirst — all three survive:Against this branch (
668dfc1), each mutation applied, suite run, reverted, tree confirmed clean:✗ the pre-flight refuses a plan containing a marked artifact before anything is deletedassertNotPreservedindeletePruneArtifact✗ the deletion boundary refuses a marked artifact the pre-flight never saw,✗ a preservation refusal at the boundary stops the run instead of being collected into errors[],✗ a preservation refusal on a catalog manifest also stops the runif (error instanceof PreservationGuardError) throw error;git diff HEADempty)No failure was timeout-shaped (
grep -c -i 'timed out\|timeout'= 0 in all three logs).bun run check(typecheck +bun test+ contracts conformance + build) → rc=0.Stated limitation, not papered over: M2 and M3 are killed by the same three tests — no single test separates "the boundary raises" from "the raise is not downgraded". Doing so needs a
PreservationGuardErrorraised inside the delete loop by something other than the boundary's own guard, i.e. an injected stub deleter inapplyPrunePlan's signature. I judged that a worse trade than a test set where the two co-kill. Both mutations die, which is the acceptance criterion.Coordination with #22
Touches
src/backup.ts(insidepruneBackups/ immediately after it) and adds one new test file. Measured, not assumed: trial-merging #22's head (402a3fb) intomain d32e5cband into this branch produces the identical conflict set —— and the identical single conflict hunk in
src/backup.ts(lines 60–72, the./preservation.jsimport block). This branch adds zero new conflicts against #22; every one of those already exists between #22 and mergedmain. My hunks inpruneBackupsmerge cleanly either way. Does not touchsrc/preservation.tsorsrc/types.ts.Not done, deliberately
No
CHANGELOG.mdentry: there is no user-visible behaviour change andapplyPrunePlanis not published API, so a bullet would be noise — and it would add a gratuitous conflict with #22, which also editsUnreleased. One line if a reviewer disagrees.Safety
No
backup runand nobackup prune --applywas invoked at any point. Every deletion in these tests is a library call against amkdtemphome. Live catalog fingerprint re-verified byte-identical before and after all work: 62 manifests, 234774 bytes, six independent md5 recipes unchanged (D_sorted_lines=82bf18181813b7eeb7ce1cbde3136eea,G_concat_content=a1b622902f8ab85ba5d568d8632c0243). Staged secrets scan run clean before both commits.Refs: todos
9c93913bNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.