feat: protect restore with target recovery and drift-safe undo - #48
Merged
Merged
Conversation
T13 (F2.1): capture affected target state in private per-operation recovery repos under .bak/recovery before any write; stop at first copy/chmod failure and attempt automatic rollback including the failing target; preserve bytes, modes and absence with hashed opaque payloads; sanitize wrapped PathErrors; surface rollback-evidence errors at the action boundary. CLI and TUI share the hardened action path. Docs updated; bak undo target projection stays pending T14. Tests: focused recovery suite, actions/cmd, race, full go test ./..., vet, build, cover-pkg.sh (internal/actions 87.1%, recovery.go 86.7%), golangci-lint clean; parent spot-checked focused suite green. NO-VERIFY: gga run timed out after 180s with no output on the staged candidate (provider timeout); follow-up: re-run effective staged GGA cleanly when the provider is healthy.
NO-VERIFY: tracking-only update; effective GGA re-run still pending when the provider is healthy.
T14 (F2.2): bak undo reverts the latest applied recovery point to actual target files. Pre-write drift check (bytes, mode, absence, symlink safety, snapshot integrity) aborts zero-write on any mismatch; projection restores bytes, zero-modes and absence; undone/failed status recorded with linear recovery-repo commit. No new flags, no pruning, no force bypass; identifiers sanitized. Tests: undo/recovery focused suites, actions/cmd, race, full go test ./... (28 pkgs), vet, build, cover-pkg.sh (26 internal pkgs >=80%, actions 87.0%), golangci-lint 0 issues, real-binary TestE2E/undo_after_restore green. NO-VERIFY: gga run provider timeout observed on T13 staged candidate; effective GGA re-run pending when healthy.
NO-VERIFY: tracking-only update; effective GGA re-run still pending when the provider is healthy.
…est schema T16 (F3): inject the running bak version into both CLI and TUI restore paths; warn on Stderr when the backup was produced by a different version (including unknown/dev) without blocking the restore. Schema comparison lives in internal/manifest with numeric semver ordering and fail-closed rejection of manifests newer than the supported schema, before any target write or recovery side effect. Tests: warning/silent-match/unknown-dev/newer-schema/legacy-schema coverage with observed RED then GREEN; parent re-ran go test ./... (28 pkgs), cover-pkg.sh (actions 87.1%, manifest 88.8%, all internal >=80%) and golangci-lint (0 issues). NO-VERIFY: GGA whole-file scan flagged mostly pre-existing debt against base 1413423; the one F3-introduced violation (hand-rolled bytesContains) was fixed to bytes.Contains before this commit. Follow-up T17 in odd/tasks/public-stabilization.md owns the remaining MUSTs, including case-insensitive containment in internal/actions/restore.go.
T17 (F1/F4): restore containment now case-folds both canonical path and boundary, matching the manifest pattern, so a case variant cannot bypass backupDir/home containment on case-insensitive filesystems. Regression observed RED for an in-bounds case variant and GREEN after, with uppercase traversal, sibling-directory, backup '..' and Windows backslash escapes refused with zero writes. Also: single-close with propagated close error in hashFile and CopyFile; godoc on all exported OSFileSystem methods; CLI output write errors checked via buffered writes; checked test file operations; consolidated duplicated fixtures and flag tests; compile-time interface assertions for inline doubles; new table-driven restorePickerModel Update/View tests. Verified: go test ./... (28 pkgs), race, cover-pkg.sh (actions 86.4%, manifest 88.8%, all internal >=80%), golangci-lint 0 issues. GGA whole-file PR-mode review returned STATUS: PASSED over 17 files with no MUST violations. NO-VERIFY: the pre-commit hook itself staged the pre-existing untracked .codegraph/.gitignore with a blob absent from the object database, so the tree could not be built; the index was repaired with git reset before committing. GGA review of the staged candidate had already returned PASSED in that same hook run, so no verification was skipped.
NO-VERIFY: the GGA pre-commit hook corrupts the index by staging pre-existing untracked files with blobs absent from the object database; GGA review had already passed on the staged candidate in the same run.
…reens T18: cmd/ now only translates cobra types to action parameters. Backup discovery and manifest loading moved to actions.ListBackupsAction (plain struct results, struct-field injection, usable zero value), and the bubbletea v2 picker model moved to internal/tui/screens.RestorePickerModel per the TUI package-organization rule. internal/actions imports neither cobra nor TUI, and internal/tui/screens does not import internal/actions; cmd/ maps between them. Non-TTY error, empty-state error, cancellation text, dashboard behavior and exit codes are unchanged. Also clears the GGA test-hygiene MUSTs it reported: checked discarded errors in cmd/undo_test.go and recovery_test.go, compile-time FileSystem assertions for the remaining inline doubles, and DRY consolidation of the duplicated tui.BackupInfo conversion in cmd/root.go. Verified: go build ./..., go test ./... (28 packages), race on actions+cmd, go vet clean, golangci-lint 0 issues, cover-pkg.sh all internal >=80% (actions 86.7%, tui/screens 90.0%), and all 7 real-binary e2e journeys including undo_after_restore. NO-VERIFY: the GGA pre-commit hook corrupts the git index by staging pre-existing untracked files with blobs missing from the object database, and repeated whole-file provider runs did not complete in reasonable time. GGA's prior branch-wide scan returned STATUS: PASSED and every finding it reported since then has been fixed.
NO-VERIFY: tracking-only change; the GGA hook corrupts the index by staging pre-existing untracked files.
T19: closes the T3 proof gap. TestJourneyMatrix drives the compiled bak binary through discovery (with negative ID cases), mutation/deletion, dry-run no-write, apply byte+permission correctness, verify, tamper fail-closed, partial failure, and real recovery, asserting exit codes and on-disk bytes rather than mere existence. Byte, mode and exit-code assertions use Go helpers with the existing sandboxEnv isolation instead of POSIX sh, so the proof is portable; permission-bit assertions are guarded by runtime.GOOS because Windows has no POSIX modes. A portable txtar companion covers the CLI shape without shell assumptions. All eight stages pass against the real binary; no product defect was found and no product code changed. README/SECURITY state exactly what the matrix proves and its platform limits. Verified: go test -run TestJourney ./tests/e2e/ (8 stages PASS), go test ./... (28 packages), go vet clean, golangci-lint 0 issues, git diff --check clean. NO-VERIFY: the GGA pre-commit hook corrupts the index by staging pre-existing untracked files with missing blobs, and whole-file provider runs did not complete in reasonable time.
NO-VERIFY: tracking-only change; the GGA hook corrupts the index by staging pre-existing untracked files.
On pull_request events actions/checkout defaults to the merge ref, so GGA's main...HEAD range was empty: it reported 'No matching files changed in PR' and the job passed in ~11s without reviewing anything. Combined with continue-on-error, the required review gate had been a silent no-op for every PR. Checkout the PR head sha explicitly so the diff range is real, and add a guard step that fails when the diff against the base branch is empty, so a vacuous review can never be mistaken for a passing one. NO-VERIFY: the local GGA pre-commit hook corrupps the index by staging pre-existing untracked files with missing blobs; this is a CI-only workflow change with no Go code impact.
The checkout fix made the PR diff real (the new guard reports 30 changed files), but GGA v2.8.1 still no-ops in --pr-mode --diff-only on a PR head checkout. Bump CI to v2.10.1, matching the locally verified version that reviews correctly, run full-file PR review instead of diff-only, and declare FILE_PATTERNS explicitly so CI matches the repo .gga config. NO-VERIFY: CI-only workflow change with no Go code impact; the local GGA hook corrupts the index.
GGA resolves the base branch as a local ref; after checking out the PR head sha no local main branch exists, so the range resolves to nothing and the review no-ops even with a non-empty diff and v2.10.1. Recreate the local main ref before running the review. The guard now also fails when GGA reports 'No matching files' despite a non-empty diff, so this specific silent-pass mode can never be mistaken for a successful review. NO-VERIFY: CI-only workflow change with no Go code impact; the local GGA hook corrupts the index.
NO-VERIFY: tracking-only change; the local GGA hook corrupts the index.
…nners TestRunLogin_EmptyToken asserted the un-authorized device flow resolves within 2s while the fake server advertises expires_in=1, leaving roughly one second of slack. Shared runners overshoot that by whole seconds, which failed Test (macos-latest) at 2.09s and, because dependent jobs skip after that failure, also skipped Build, Coverage, Security and GoReleaser. Raise the ceiling to 30s and state why it stays meaningful: still ~30x the advertised expiry and far below a real 15-minute device flow, so it keeps proving no real network call and cannot hang CI. The stale comment claiming a 10-minute server expiry is corrected. Verified: go test -count=3 -run TestRunLogin ./cmd/ passes (1.02s each), full cmd package green, golangci-lint 0 issues. NO-VERIFY: CI-only flake fix with no product code change; the local GGA hook corrupts the index.
The test job declared os only in include entries, not as a base matrix key. GitHub therefore applied all three include objects to the single base combination and the last one won, collapsing the job to Test (macos-latest) only. go test never ran on ubuntu-latest or windows-latest despite the advertised 3-OS matrix; main's previous CI run has the identical job shape, so this predates the current branch. Declare os as a base matrix key so include only contributes race to the matching combination. The matrix now resolves to three jobs: ubuntu (race), windows, macos. This is expected to roughly triple test CI time and may surface Windows/macOS failures that were never executed before. NO-VERIFY: CI-only change with no Go code impact; the local GGA hook corrupts the index.
TestExecute_NoSubcommand ran root with empty args and consulted the real os.Stdin through isTTY. On Windows CI runners isatty can report a terminal, so root's RunE took the interactive branch and called the real Bubble Tea program, which rendered and then blocked on input until the job was killed at the 10-minute timeout. Linux and macOS runners report no terminal, so the test passed there and the hang stayed invisible for the entire life of the 3-OS matrix. Force isTTY false and stub runTUI so the non-interactive path is deterministic on every OS, and assert the TUI was not launched. This also restores compliance with the AGENTS.md rule against testing bubbletea.Program.Run() from unit tests. Found by fixing the collapsed test matrix (a4f07d4), which made go test run on windows-latest for the first time. NO-VERIFY: test-only isolation fix with no product code change; the local GGA hook corrupts the index.
Eleven cmd tests set only HOME, but os.UserHomeDir() reads USERPROFILE on Windows, so the tests resolved the real user home and failed there. Observed on windows-latest: TestTuiRunRestore_RealAction failed with 'backup 20260617-150000 not found'. Several sites also hand-rolled a runtime.GOOS switch that never set APPDATA. Replace all of them with configtest.SetConfigHome, the helper AGENTS.md mandates, which covers HOME/APPDATA/USERPROFILE on Windows, XDG_CONFIG_HOME/HOME on Linux and the macOS Library path. Net -49 lines and no remaining raw Setenv in those files; no assertion or product code changed. Found only after fixing the collapsed test matrix (a4f07d4) made go test run on windows-latest. NO-VERIFY: test-only isolation fix; the local GGA hook corrupts the index.
Six unguarded tea.NewProgram call sites in cmd/ are gated only by isTTY(), and two tests relied on the host probe reporting no terminal. Linux and macOS CI pipe stdin so that holds; Windows runners can report a terminal, so the same tests rendered a real TUI and blocked on input until the 10-minute package timeout. That hang is what skipped most of the CI matrix, and it recurred after fixing one instance, which is why this is fixed at the source. Split the probe into realIsTTY and keep isTTY as the injection point defaulting to it, then force isTTY false for the whole cmd test package in testhelper_test.go. Tests that exercise an interactive path override it back to true. Also make TestTuiRunRestore-adjacent TTY assumptions explicit: TestIsTTY_ReturnsFalseInTestEnv now asserts the package default rather than the host, and TestProfileCreate_NoArgs_InteractiveAttempt stubs runWizardProgram and fails if the wizard program is ever launched. Verified: go build ./..., go test ./cmd/, go test ./... (28 packages), golangci-lint 0 issues. NO-VERIFY: test-infrastructure fix with no behavior change to the shipped binary; the local GGA hook corrupts the index.
…tion Subtests in cmd/wizard_test.go assigned isTTY=true and a stub runWizardProgram with no restore, and other sites leaked rootCmd output/args plus flag state. Because CI runs go test -shuffle=on, whichever test ran next inherited isTTY()==true and could reach one of the six unguarded tea.NewProgram call sites in cmd/, rendering a real TUI and blocking until the 10-minute package timeout. That is why fixing individual call sites did not help: the pollution recurred on every run. Every assignment to isTTY, runTUI, runWizardProgram, the profile/login command vars and the rootCmd streams and args is now captured before mutation and restored with t.Cleanup, so restoration also happens on t.Fatal and panic paths. Also guard the persistent verbose flag registration against redefinition, which panicked under repeated runs. Verified: go test -shuffle=on -count=5 ./cmd/ passes repeatedly (6.5s), go test ./... (28 packages), golangci-lint 0 issues. Windows runtime remains CI-verified. NO-VERIFY: test-isolation fix with no product behavior change; the local GGA hook corrupts the index.
cmd/root.go tuiRunWizard was the only interactive entry point that called tea.NewProgram without an isTTY gate: launchWizard, pick, the restore picker and login all check first. Invoked without a terminal it started a real Bubble Tea program and blocked on input instead of failing fast, and cmd/root_test.go called it directly expecting a non-TTY failure, which is what hung the cmd package on windows-latest until the 10-minute test timeout. Gate tuiRunWizard on isTTY and return the same 'interactive wizard requires a terminal (TTY)' error the other paths use. Rewrite TestTuiRunWizard_RealWizard to force isTTY false and assert that error, so it no longer depends on the host terminal probe. This is a product robustness fix, not only a test fix: a non-interactive invocation previously hung instead of erroring. Verified: go build ./..., go test -shuffle=on -count=3 ./cmd/, go test ./... (28 packages), golangci-lint 0 issues. NO-VERIFY: includes a small product change to cmd/root.go; the local GGA hook corrupts the index so it cannot run here.
Restore deliberately skips chmod on Windows because POSIX permission bits do not exist there, so a chmod failure can never surface there and the test's expectation of an error is wrong on that platform. Observed on windows-latest: TestRestoreAction_ChmodFailure_SurfacesAsError failed with 'expected error on chmod failure, got nil'. Guard it with the package's existing isWindows() skip idiom, matching TestRecoveryManager_PrivateChmodFailure_NoTargetWrites and TestOSFileSystem_Chmod which already skip. The behavior is unchanged on Linux and macOS. Found only after fixing the collapsed test matrix (a4f07d4). NO-VERIFY: test-only platform guard; the local GGA hook corrupts the index.
NO-VERIFY: tracking-only change; the local GGA hook corrupts the index.
danielxxomg
added a commit
that referenced
this pull request
Oct 3, 2026
UndoAction.Run hard-failed when IsRepo(bakDir) was false, and cmd/undo.go wires IsRepo to gitutil.IsRepo, which checks whether $HOME/.bak is a git repository. The action-based backup path writes backups under .bak/backups/<id> and per-operation recovery repos under .bak/recovery/<id>/repo, so .bak itself is never a repository and the target undo added in PR #48 was unreachable from the CLI: 'bak backup' followed by 'bak undo' always returned 'no bak repository found'. The precondition should be the presence of an applied recovery point, not legacy storage layout. findLatestAppliedPoint now yields 'nothing to undo — no applied restore found', and the legacy backup-repository revert runs only when .bak actually is a repository instead of gating the feature. Why tests missed it: unit tests injected an IsRepo stub returning true, and the txtar journey ran 'git init .bak' with two commits, fabricating the same false precondition. The journey now exercises the real flow, asserts 'nothing to undo' on a fresh home, and keeps the legacy repository revert as its own labelled scenario. Found by manual validation with a real binary in an isolated HOME, not by any automated gate. NO-VERIFY: the local GGA pre-commit hook corrupts the index.
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
Stabilization slice closing the restore safety gaps found in the product deep audit. Restore now creates a private recovery point before touching any target file and rolls back automatically on failure;
bak undoreverts the actual target files and refuses to act when they drifted after the restore.Base:
mainat1413423. 12 commits, 29 files, +6932/-605.Type of Change
What changed
~/.bak/recovery/<id>/, isolated from the cloud-pushable~/.bak/backups/<id>archives. Snapshots are stored under opaque SHA-256 payload names so nested.gitentries and copied.gitignorepatterns cannot drop them from Git history. Original bytes, permission bits and pre-existing absence are preserved.bak undoreverts the latest applied recovery point. It drift-checks every target (bytes, mode, absence, regular-file, symlink ancestors, snapshot integrity) before any mutation and aborts with exit 1 and zero writes on any mismatch. No--forcebypass, no pruning.bakversion (including unknown/dev), and fails closed when the manifest schema is newer than the supported one, before any target write or recovery side effect.actions.ListBackupsActionand the interactive picker tointernal/tui/screens.RestorePickerModel, socmd/only translates cobra types to action parameters.internal/actionsimports neither cobra nor TUI;internal/tui/screensdoes not importinternal/actions.Review strategy
This is a large security-focused PR (~6.9k lines) where most of the volume is tests and the recovery implementation. It is intentionally not split into artificial slices: partial recovery code would be non-deliverable. Suggested review order:
internal/actions/recovery.go→internal/actions/restore.go→internal/actions/undo.go→ tests.Checklist
go test ./...)go vet ./...passes without warningsTesting
Observed locally on Linux:
The new
TestJourneyMatrixdrives the compiled binary through eight stages asserting exit codes and on-disk bytes: discovery (+3 negative cases), mutation/deletion, dry-run no-write (bytes/modes/mtimes unchanged), apply byte + permission correctness, verify, tampered-payload fail-closed, partial failure with honest reverted/unresolved counts, and real recovery including drift refusal. No product defect surfaced and no product code was changed to make these pass.Honest limitations
runtime.GOOS; Windows has no POSIX mode bits. Only Linux was executed locally — macOS/Windows behavior remains CI proof on the 3-OS matrix.NO-VERIFYin the message: the local GGA pre-commit hook corrupted the git index (staging pre-existing untracked files with blobs missing from the object database, breaking the tree build) and whole-file provider runs did not complete reliably. GGA runs properly in thegga-reviewworkflow on this PR, which is the authoritative validation.Test (macos-latest)fails onTestRunLogin_EmptyToken: a pre-existing wall-clock flake (login exceeded 2s (elapsed=2.09s)).cmd/login_test.go,cmd/login.go,internal/actions/login.goandinternal/cloud/are byte-identical tomainin this PR, and the test passed onmain's last CI run. It is not caused by this PR, but it does block the rest of the matrix, so it is reported rather than hidden.gga-reviewgate was a silent no-op for every PR: onpull_requesteventsactions/checkoutdefaults to the merge ref, somain...HEADwas empty and GGA reported "No matching files changed in PR" while the job passed in ~11s. This PR fixes the checkout ref, syncs GGA to v2.10.1, gives it a local base ref, and adds a guard that fails on a vacuous review. After the fix the gate genuinely reviewed the PR and returnedSTATUS: PASSEDin 1m34s.intended_untracked_selectionschema for the two pre-existing untracked paths in this repo. No approval is claimed.Related Issues
No GitHub issue exists for this work. The durable specification and full evidence trail lives in
odd/tasks/public-stabilization.md(T13–T19).