diff options
Diffstat (limited to 'internal/engine/apply_test.go')
| -rw-r--r-- | internal/engine/apply_test.go | 201 |
1 files changed, 91 insertions, 110 deletions
diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go index 142e4d2..6c724b1 100644 --- a/internal/engine/apply_test.go +++ b/internal/engine/apply_test.go @@ -20,14 +20,12 @@ import ( // applyFixture builds a directory with two files and a rule moving pdfs into // Work, then plans it. It returns the home, the plan and an open journal. // -// Adapted from the brief to this package's actual writeConfig helper, which -// takes a main-file body and a dirs map keyed by name (see -// TestLoadRejectsUnsuppliedCaptures's comment in engine_test.go for the same -// adaptation elsewhere in this package): the brief's fixture wrote -// `(path ...)` and `(rule ...)` straight into what it called the main file, -// but the real config language (docs/design.md §4.2-4.3) requires those in a -// directory file reached through `(include ...)`. Every assertion below is -// unchanged from the brief; only this setup plumbing differs. +// This uses writeConfig, which takes a main-file body and a dirs map keyed +// by name (see TestLoadRejectsUnsuppliedCaptures's comment in +// engine_test.go for the same shape used elsewhere in this package): the +// real config language (docs/design.md §4.2-4.3) requires `(path ...)` and +// `(rule ...)` in a directory file reached through `(include ...)`, not in +// the main file directly. func applyFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { t.Helper() h := sandbox(t) @@ -205,25 +203,24 @@ func TestPlanUndoRefusesPermanentDelete(t *testing.T) { } } -// TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete is the Critical -// finding from Task 8's review: a file the ORIGINAL forward run declined -// (spec §9: its steps are still logged, status "declined") has no "ok" -// entries at all, so planUndoFile's last-to-first walk skips every one of -// them and returns an UndoFile with Steps == nil and Refused == "" - a file -// that was never touched, not a reversible one. Before the fix, PlanUndo -// appended that empty UndoFile anyway, and undoActionableCount (cmd/krino) -// counts every Refused == "" file as "to reverse" regardless of whether it -// has any steps - inflating the header's count while the table renders no -// row for it and the final tally comes up one short, silently, at exit 0. +// TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete: a file the +// ORIGINAL forward run declined (spec §9: its steps are still logged, +// status "declined") has no "ok" entries at all, so planUndoFile's +// last-to-first walk skips every one of them and returns an UndoFile with +// Steps == nil and Refused == "" - a file that was never touched, not a +// reversible one. Without the guard against that, PlanUndo would append +// that empty UndoFile anyway, and undoActionableCount (cmd/krino) counts +// every Refused == "" file as "to reverse" regardless of whether it has +// any steps - inflating the header's count while the table renders no row +// for it and the final tally comes up one short, silently, at exit 0. // -// The two halves in one test, deliberately, per the review: a fix that -// dropped every zero-step UndoFile instead of the correct +// The two halves live in one test, deliberately: a fix that dropped every +// zero-step UndoFile instead of the correct // "len(Steps) == 0 && Refused == \"\"" condition would also drop a // permanently deleted file (zero steps, but Refused IS set - spec §10 // requires it to stay visible with its reason) - so both conditions live // in the same test, and a future "simplification" that breaks either one -// fails this one test immediately rather than needing two separate reviews -// to notice. +// fails this one test immediately. func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -294,10 +291,9 @@ func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { } } -// TestPlanUndoAcceptsIntactRun pins the trust Task 1 established but never -// itself exercised through PlanUndo: journal.Entries returning a nil error -// for a run whose run-start and run-end both parsed cleanly is the signal -// that the chain is intact, and PlanUndo must build a usable plan from it +// TestPlanUndoAcceptsIntactRun: journal.Entries returning a nil error for a +// run whose run-start and run-end both parsed cleanly is the signal that +// the chain is intact, and PlanUndo must build a usable plan from it // rather than refuse. func TestPlanUndoAcceptsIntactRun(t *testing.T) { h, e, dp, j, run := applyFixture(t) @@ -326,8 +322,8 @@ func TestPlanUndoAcceptsIntactRun(t *testing.T) { // TestPlanUndoAcceptsCrashedRun: a run-start with no run-end (the process // died mid-run) must still yield a usable undo plan, per Entries' documented -// window-to-EOF behaviour. If this refused, Task 1's contract and this -// task's assumption would disagree - worth a ruling, not a workaround. +// window-to-EOF behaviour. A refusal here would mean PlanUndo's assumption +// and Entries' actual behaviour have drifted apart. func TestPlanUndoAcceptsCrashedRun(t *testing.T) { h, e, dp, j, run := applyFixture(t) if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { @@ -424,7 +420,7 @@ func TestApplyChecksContextBetweenFilesNotWithinOne(t *testing.T) { } // TestApplyUndoRestoresMovedFile: the smallest possible round trip through -// ApplyUndo, since Task 9's is the only other test that exercises it. +// ApplyUndo. func TestApplyUndoRestoresMovedFile(t *testing.T) { h, e, dp, j, run := applyFixture(t) if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { @@ -543,8 +539,8 @@ func TestApplyUndoSkipsRefusedFiles(t *testing.T) { } } -// TestApplyUndoLogsDeclinedFile is fix round 2026-09-12, item 2 of Task 8's -// review: a file the front end's own review chose not to reverse (Refused +// TestApplyUndoLogsDeclinedFile: a file the front end's own review chose +// not to reverse (Refused // empty, Declined set by the caller - PlanUndo itself never sets it) must // still be logged, spec §9's "declined files are logged even though nothing // happens to them" extended to undo. The file must come back untouched, the @@ -620,9 +616,8 @@ func TestApplyUndoLogsDeclinedFile(t *testing.T) { } } -// TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone is fix wave -// item 2 (Important): reproduced by the reviewer via pty as `1 moved -// (undone)` with the file still filed. The mechanism is +// TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone: reproduced +// via pty as `1 moved (undone)` with the file still filed. The mechanism is // journal.Runs' own (see TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined // for that unit-level pin); this is the same defect exercised end to end // through a real forward run, a real declined undo, and e.Runs() itself - @@ -693,12 +688,10 @@ func TestRunsDelegatesToJournal(t *testing.T) { } } -// --- Fix round 1 --- - -// TestApplyLogsTrashEntryNameInDetail: fix round 1, item 3. The trash entry -// name must be logged explicitly (Detail), not left to be re-derived from -// Dst's basename - Dst's shape is internal/apply's contract, not undo's, and -// the two must not be secretly coupled. +// TestApplyLogsTrashEntryNameInDetail: the trash entry name must be logged +// explicitly (Detail), not left to be re-derived from Dst's basename - +// Dst's shape is internal/apply's contract, not undo's, and the two must +// not be secretly coupled. func TestApplyLogsTrashEntryNameInDetail(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -773,10 +766,10 @@ func TestApplyLogsTrashEntryNameInDetail(t *testing.T) { } } -// TestRunUndoStepTrashReadsEntryNameFromDetailNotDst: fix round 1, item 3, -// isolated. Src is deliberately a path whose basename names no real trash -// entry; only Original.Detail names the real one. If runUndoStep ever goes -// back to deriving the name from Dst (or Src), this fails. +// TestRunUndoStepTrashReadsEntryNameFromDetailNotDst: Src is deliberately a +// path whose basename names no real trash entry; only Original.Detail +// names the real one. If runUndoStep ever goes back to deriving the name +// from Dst (or Src), this fails. func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -794,7 +787,7 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { // Original is what the run logged for the trash step: the entry name in // Detail, and the entry's own path, size and mtime (checked again at - // execution time, review undo F8). + // execution time). entryPath := filepath.Join(trash.Dir(), "files", entry) fi, err := os.Stat(entryPath) if err != nil { @@ -815,8 +808,8 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { } } -// TestPlanUndoRefusesFileModifiedWithinSameSecond: fix round 1, item 4. The -// journal now records ModTime with sub-second precision (RFC3339Nano), so a +// TestPlanUndoRefusesFileModifiedWithinSameSecond: the journal now records +// ModTime with sub-second precision (RFC3339Nano), so a // file rewritten within the same whole second as the run must still be // detected as changed - a .Unix()-granularity comparison would miss this // and undo would silently move the edited file back over the user's data. @@ -851,8 +844,8 @@ func TestPlanUndoRefusesFileModifiedWithinSameSecond(t *testing.T) { } } -// TestUndoFileStopsAfterFailedFileAffectingStep: fix round 1, item 2. A -// failed undo-move must stop the rest of that file's reversal - continuing +// TestUndoFileStopsAfterFailedFileAffectingStep: a failed undo-move must +// stop the rest of that file's reversal - continuing // would leave it half undone (spec §10), even though the later step // (undo-copy) would, in isolation, have succeeded. func TestUndoFileStopsAfterFailedFileAffectingStep(t *testing.T) { @@ -891,9 +884,9 @@ func TestUndoFileStopsAfterFailedFileAffectingStep(t *testing.T) { } } -// TestUndoFileContinuesPastFailedMkdir: fix round 1, item 2's other half - -// a failed undo-mkdir (directory not empty) must NOT stop the rest of the -// file's reversal, unlike every other action. +// TestUndoFileContinuesPastFailedMkdir: a failed undo-mkdir (directory not +// empty) must NOT stop the rest of the file's reversal, unlike every other +// action. func TestUndoFileContinuesPastFailedMkdir(t *testing.T) { h := sandbox(t) nonEmpty := filepath.Join(h, "nonempty") @@ -940,11 +933,9 @@ func TestUndoFileContinuesPastFailedMkdir(t *testing.T) { } } -// --- Fix round 2 --- - -// TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution: fix round 2, -// item 1 (Critical). Spec §10 says an undo plan is shown and approved like -// any other, so there is a real, human-length window between PlanUndo's +// TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution: spec §10 +// says an undo plan is shown and approved like any other, so there is a +// real, human-length window between PlanUndo's // refuseIfSrcExists check and ApplyUndo actually running - long enough for // something else to create a file at the reversal's destination in between. // undo-move/undo-rename must re-check at execution time rather than let a @@ -993,11 +984,10 @@ func TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution(t *testing.T) { } } -// TestApplyUndoDoesNotCountFailedMkdirAsFailed: fix round 2, item 3. A file -// whose only failure is an undo-mkdir (a shared directory not yet empty) -// must not flip ApplyResult.Failed - Task 7 maps that to krino undo's exit -// code, and ruling 4 (fix round 1, item 1) established that this specific -// refusal is tidiness, not a hazard. +// TestApplyUndoDoesNotCountFailedMkdirAsFailed: a file whose only failure +// is an undo-mkdir (a shared directory not yet empty) must not flip +// ApplyResult.Failed - ApplyResult maps to krino undo's exit code, and +// this specific refusal is tidiness, not a hazard. func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { h := sandbox(t) dir := filepath.Join(h, "Work") @@ -1044,15 +1034,13 @@ func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { } } -// --- Fix wave (2026-09-12) --- - // overwriteFixture builds a directory where a forward move under // (on-conflict overwrite) will displace a pre-existing file at its // destination: dl/incoming.pdf moves to dl/Work/incoming.pdf, which already // holds a different file (the "victim") the move must trash first. This is // the one shape that makes a step's Displaces and another step's Dst name // the exact same path (internal/plan/conflict.go's resolveConflict, -// deliberately), which is what fix wave item 1 (Critical) is about. +// deliberately). func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { t.Helper() h := sandbox(t) @@ -1097,21 +1085,19 @@ func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, return h, e, dp, j, journal.NewRunID(time.Now()) } -// TestApplyUndoReversesOverwriteRoundTrip is fix wave item 1 (CRITICAL): the -// end-to-end reproduction of the defect the final-plan review found - -// `krino undo` could not reverse a run that used (on-conflict overwrite) at -// all, by construction. reverseStep's planning-time occupancy check judged -// the displace reversal against the world exactly as it stood before any -// reversal had run, while the move-back that frees the contested path is -// ordered to execute first (reversal is last-original-step-first), so the -// displace reversal was refused every time and, being file-affecting, -// aborted the whole file's reversal - including the otherwise-safe -// move-back. This is the first coverage of undo-displace anywhere in the -// repo (grep undo-displace across every prior test returns nothing), and it -// is built from a REAL forward run through overwriteFixture's real -// displacing apply, per the brief: a hand-assembled journal.Entry is -// exactly what would let a narrower, wrong fix pass while still being -// wrong. +// TestApplyUndoReversesOverwriteRoundTrip: `krino undo` could not reverse a +// run that used (on-conflict overwrite) at all, by construction. +// reverseStep's planning-time occupancy check judged the displace reversal +// against the world exactly as it stood before any reversal had run, while +// the move-back that frees the contested path is ordered to execute first +// (reversal is last-original-step-first), so the displace reversal was +// refused every time and, being file-affecting, aborted the whole file's +// reversal - including the otherwise-safe move-back. This is the first +// coverage of undo-displace anywhere in the repo (grep undo-displace +// across every prior test returns nothing), and it is built from a REAL +// forward run through overwriteFixture's real displacing apply: a +// hand-assembled journal.Entry is exactly what would let a narrower, +// wrong fix pass while still being wrong. func TestApplyUndoReversesOverwriteRoundTrip(t *testing.T) { h, e, dp, j, run := overwriteFixture(t) if _, err := e.Apply(context.Background(), dp, map[string]bool{"incoming.pdf": true}, j, run); err != nil { @@ -1170,10 +1156,10 @@ func TestApplyUndoReversesOverwriteRoundTrip(t *testing.T) { } } -// TestApplyUndoStillRefusesGenuineOccupant is fix wave item 1's second -// required test: the projection must only excuse a path an earlier step of -// THIS SAME chain is about to vacate, never turn every occupancy refusal -// into a pass. Here something outside the chain entirely - not the +// TestApplyUndoStillRefusesGenuineOccupant: the projection must only excuse +// a path an earlier step of THIS SAME chain is about to vacate, never turn +// every occupancy refusal into a pass. Here something outside the chain +// entirely - not the // displaced original, not the incoming file itself - now occupies the // path the move-back needs, and no step of this file's reversal will ever // free it. @@ -1204,9 +1190,9 @@ func TestApplyUndoStillRefusesGenuineOccupant(t *testing.T) { } } -// TestTallyFileCountsAnAllSkippedFileAsDeclined is fix wave item 4 / Minor -// 6: a file every one of whose steps came back "skipped" - the shape an -// approved all-skipped chain used to take - set none of ok/failed/declined +// TestTallyFileCountsAnAllSkippedFileAsDeclined: a file every one of whose +// steps came back "skipped" - the shape an approved all-skipped chain used +// to take - set none of ok/failed/declined // in tallyFile, so it fell out of the outcome tally entirely: "0 applied · // 0 failed · 0 declined" for a file the user was asked about and approved. // tallyFile must land every file it is given in exactly one bucket; nothing @@ -1240,24 +1226,22 @@ func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) { } } -// --- Plan 5, Task 1 --- - // sharedDestUndoFixture builds a downloads directory with three pdf files // (a.pdf, b.pdf, c.pdf) and a single rule moving all of them into dest, // applies the move, and returns the sandbox home, the loaded engine, the // journal's path and the forward run's ID. // // All three files landing on one destination that this one run creates is -// the shape that exercises the run-wide directory retry (Task 1, plan 5): -// whichever file's chain first creates dest carries its undo-mkdir step(s), -// and that file's own reversal typically runs while its siblings still -// occupy dest - refusing the removal correctly, at first. dest may name a -// nested path ("Work/Sub"): apply.mkdirAllTracked then records every -// directory the move had to create, outermost first, and every one of them -// still lands on that same first file's chain. +// the shape that exercises the run-wide directory retry: whichever file's +// chain first creates dest carries its undo-mkdir step(s), and that file's +// own reversal typically runs while its siblings still occupy dest - +// refusing the removal correctly, at first. dest may name a nested path +// ("Work/Sub"): apply.mkdirAllTracked then records every directory the +// move had to create, outermost first, and every one of them still lands +// on that same first file's chain. // -// Extracted per fix round 1 (Important 2): TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses -// and TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone +// TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses and +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone // used to duplicate this setup verbatim; TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst // needed the identical shape with only dest varying, which is what named the // parameter rather than hard-coding "Filed" here. @@ -1268,10 +1252,9 @@ func sharedDestUndoFixture(t *testing.T, dest string) (h string, e *Engine, logP if err := os.MkdirAll(dl, 0o755); err != nil { t.Fatal(err) } - // Three files that all move into ONE created destination. The bug this - // task fixes is that only the file whose chain first creates it ever - // carries the undo-mkdir step, and that step is attempted while its - // siblings are still inside. + // Three files that all move into ONE created destination: only the file + // whose chain first creates it ever carries the undo-mkdir step, and + // that step is attempted while its siblings are still inside. for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { if err := os.WriteFile(filepath.Join(dl, n), []byte(n), 0o640); err != nil { t.Fatal(err) @@ -1342,8 +1325,8 @@ func TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses(t *testing.T) { } } -// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst pins fix round 1's -// Important 1: retryDirRemovals must retry deepest path first. All three +// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst: retryDirRemovals +// must retry deepest path first. All three // files move into Work/Sub, so Apply's single mkdirAllTracked call creates // both Work and Work/Sub on the FIRST file's own chain (outermost first), // which means that one file's reversal carries two undo-mkdir steps, one for @@ -1398,16 +1381,14 @@ func TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst(t *testing.T) { } } -// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins -// the journal half of the ruling on the point Task 1's brief left open: when -// retryDirRemovals succeeds in removing a directory, it appends an +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone: +// when retryDirRemovals succeeds in removing a directory, it appends an // ADDITIONAL journal entry for it - the original "failed" undo-mkdir entry, // recorded on whichever file's chain first created the directory, is never // rewritten or removed - and, because that entry's Action is still // "undo-mkdir" like the first, journal.ranAnyUndoStep continues to exclude -// it from what marks a run "(undone)" (the brief's constraint: it "cannot -// change whether a run shows as (undone); confirm that rather than assume -// it"). +// it from what marks a run "(undone)" (this must not change whether a run +// shows as (undone); the test confirms that rather than assuming it). func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *testing.T) { _, e, logPath, run := sharedDestUndoFixture(t, "Filed") |