From 5f36188136e35fdcb7aea0f6bec55f7df3f6cce3 Mon Sep 17 00:00:00 2001 From: Pierre Malarme Date: Thu, 16 Jul 2026 12:08:06 +0200 Subject: [PATCH 1/2] Add local --back step for the workshop engine Local learners who advance without committing each step had no way to move back. Add `advance_step.py --back` (plus a `make back` alias, README docs, and a `back-workshop.yml` CI pipeline) that restores the learner's saved work from the previous step's snapshot, or rebuilds the canonical step files when no snapshot exists. Namespace the backup layout so the agent snapshot and repo-root overlay targets never collide: `step-/travel_assistant/`, `step-/_root/`, and a `backup.json` completion manifest written last. This makes root-name collisions structurally impossible (#1) and lets `--back` restore structurally rather than by name. Snapshots are now published atomically (stage -> copy -> manifest -> swap) and fully replace any prior snapshot, so files deleted on a revisit are never resurrected (#3). Also: gate `--back` restore on a valid manifest (legacy/partial backups degrade to a canonical rebuild with a clear warning), reject `_root` overlay targets that shadow protected repo paths, and make reset/back/step backup dirs unique within the same second. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fea011c8-1399-4f8a-a3bd-277408cae6c2 --- .github/workflows/back-workshop.yml | 108 +++++ .workshop/docs/partials/_push_to_advance.md | 2 + .workshop/docs/steps/00-intro.md | 11 +- .workshop/scripts/advance_step.py | 363 ++++++++++++++- .workshop/scripts/tests/test_advance_step.py | 444 ++++++++++++++++++- Makefile | 7 +- README.md | 11 +- 7 files changed, 914 insertions(+), 32 deletions(-) create mode 100644 .github/workflows/back-workshop.yml diff --git a/.github/workflows/back-workshop.yml b/.github/workflows/back-workshop.yml new file mode 100644 index 0000000..3a0150d --- /dev/null +++ b/.github/workflows/back-workshop.yml @@ -0,0 +1,108 @@ +name: Go back one workshop step + +on: + workflow_dispatch: + inputs: + confirm: + description: "Type BACK to confirm. This moves you back one step and overwrites travel_assistant/ — your current work is backed up to .workshop_instance/workshop_backups/back-/ first." + required: true + type: string + +permissions: + contents: write + +concurrency: + # Shared with init-workshop.yml, start-workshop.yml, reset-workshop.yml and + # advance-on-push.yml so every workflow that mutates the workshop state + # serializes on the same branch and never races another on `git push` or + # `.workshop_instance/.workshop-state.json`. + group: workshop-state-${{ github.ref }} + cancel-in-progress: false + +jobs: + back: + if: ${{ github.event.inputs.confirm == 'BACK' }} + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install dependencies + run: pip install -r .workshop/scripts/requirements.txt + + - name: Go back one step + id: back + shell: bash + # advance_step.py --back exits non-zero for expected situations (already + # at step 0, a state/README desync, or missing canonical step files). + # Capture the outcome so the summary can explain it instead of failing + # the run with a bare red X. NEW_STEP is exported to $GITHUB_ENV by the + # script only on success. + run: | + set +e + output="$(python .workshop/scripts/advance_step.py --back 2>&1)" + code=$? + echo "$output" + # Use a random heredoc delimiter so the captured script output can + # never contain a line that prematurely terminates the $GITHUB_OUTPUT + # multiline value (delimiter-injection hardening). + delim="WORKSHOP_EOF_$(openssl rand -hex 16)" + { + echo "reason<<${delim}" + echo "$output" + echo "${delim}" + } >> "$GITHUB_OUTPUT" + echo "code=$code" >> "$GITHUB_OUTPUT" + exit 0 + + - name: Commit & push + if: steps.back.outputs.code == '0' + run: | + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git add -A + git diff --cached --quiet || git commit -m "workshop: go back to step ${NEW_STEP}" + git push + + - name: Summarize move-back + if: steps.back.outputs.code == '0' + shell: bash + run: | + echo "## ✅ Moved back to step ${NEW_STEP}" >> "$GITHUB_STEP_SUMMARY" + echo "" >> "$GITHUB_STEP_SUMMARY" + echo "Your work from the step you were on is preserved under .workshop_instance/workshop_backups/back-/." >> "$GITHUB_STEP_SUMMARY" + echo "Run \`git pull\` to refresh your README.md and the step's files." >> "$GITHUB_STEP_SUMMARY" + + - name: Summarize failure + if: steps.back.outputs.code != '0' + shell: bash + env: + # Pass the captured reason through the environment rather than + # interpolating ${{ }} straight into the script, so its contents can + # never be treated as shell to run. + REASON: ${{ steps.back.outputs.reason }} + run: | + echo "## ⚠️ Could not move back a step" >> "$GITHUB_STEP_SUMMARY" + echo "" >> "$GITHUB_STEP_SUMMARY" + echo "The workshop state was left unchanged. Reason:" >> "$GITHUB_STEP_SUMMARY" + echo "" >> "$GITHUB_STEP_SUMMARY" + echo '```' >> "$GITHUB_STEP_SUMMARY" + echo "$REASON" >> "$GITHUB_STEP_SUMMARY" + echo '```' >> "$GITHUB_STEP_SUMMARY" + exit 1 + + confirm-required: + if: ${{ github.event.inputs.confirm != 'BACK' }} + runs-on: ubuntu-latest + steps: + - name: Summarize missing confirmation + shell: bash + run: | + echo "## ⚠️ Move-back not confirmed. Re-run and type BACK in the confirm field." >> "$GITHUB_STEP_SUMMARY" diff --git a/.workshop/docs/partials/_push_to_advance.md b/.workshop/docs/partials/_push_to_advance.md index a8059a2..0675761 100644 --- a/.workshop/docs/partials/_push_to_advance.md +++ b/.workshop/docs/partials/_push_to_advance.md @@ -22,3 +22,5 @@ After the **Advance workshop on push to main** Action finishes, run **`git pull` > **Prefer to stay local?** Run `python .workshop/scripts/advance_step.py --expected-current-step {{CURRENT_STEP}} --auto-commit` (or `make advance`) instead. That advances locally and records it in the same commit, so your next push won't advance again. See [Working fully locally](.workshop/docs/steps/00-intro.md#5-working-fully-locally-no-github-actions). Made a mistake? Use the [Reset workshop](https://github.com/{{OWNER}}/{{REPO}}/actions/workflows/reset-workshop.yml) workflow, or run `python .workshop/scripts/advance_step.py --reset --auto-commit` locally. + +Advanced too far? Use the [Go back one step](https://github.com/{{OWNER}}/{{REPO}}/actions/workflows/back-workshop.yml) workflow, or run `python .workshop/scripts/advance_step.py --back --auto-commit` locally. diff --git a/.workshop/docs/steps/00-intro.md b/.workshop/docs/steps/00-intro.md index 1b52719..44eb052 100644 --- a/.workshop/docs/steps/00-intro.md +++ b/.workshop/docs/steps/00-intro.md @@ -273,6 +273,14 @@ python .workshop/scripts/advance_step.py --reset --auto-commit Your previous `travel_assistant/` is preserved under `.workshop_instance/workshop_backups/reset-/`. +**Move back one step:** + +```bash +python .workshop/scripts/advance_step.py --back --auto-commit +``` + +Advancing has no built-in undo when you work locally without committing each step, so this is how you step back. It restores your saved work from `.workshop_instance/workshop_backups/step-/` (the snapshot advance takes before leaving a step); if that snapshot is missing it rebuilds the canonical step files instead and warns you. Your current work is always backed up to `.workshop_instance/workshop_backups/back-/` first, and it errors at step 0. + **Re-run preflight:** ```bash @@ -283,10 +291,11 @@ python .workshop/scripts/preflight.py uv run python .workshop/scripts/preflight.py ``` -**Shortcuts (optional):** the repo ships a `Makefile` with three aliases: +**Shortcuts (optional):** the repo ships a `Makefile` with these aliases: ```bash make advance # advance to the next step (auto-commits workshop paths) +make back # move back one step (auto-commits workshop paths) make reset # reset to step 0 (auto-commits workshop paths) make preflight # run environment checks ``` diff --git a/.workshop/scripts/advance_step.py b/.workshop/scripts/advance_step.py index 23b9c98..2ebfb6c 100644 --- a/.workshop/scripts/advance_step.py +++ b/.workshop/scripts/advance_step.py @@ -43,6 +43,23 @@ # copy into the deployed agent. ROOT_OVERLAY_DIR = "_root" BACKUPS_DIR = ".workshop_instance/workshop_backups" +# Backups are namespaced so the agent snapshot and the repo-root overlay targets +# never collide at the backup's top level: travel_assistant/ contents go under +# /travel_assistant/ and repo-root overlay targets under /_root/. +# The subdir names deliberately mirror the source layout. A completion manifest +# (BACKUP_MANIFEST, written last) marks a snapshot as a valid, fully-written +# new-format backup; --back only restores snapshots that carry it, so a partial +# write or an older flattened backup is never mistaken for restorable work. +BACKUP_AGENT_SUBDIR = TRAVEL_ASSISTANT_DIR +BACKUP_ROOT_SUBDIR = ROOT_OVERLAY_DIR +BACKUP_MANIFEST = "backup.json" +BACKUP_FORMAT_VERSION = 2 +# Repo-root paths a _root overlay target must never shadow — backing up, clearing, +# or restoring such a name would corrupt the repository or the backup store +# itself (e.g. a target named ".workshop_instance" would delete the backups). +_PROTECTED_ROOT_TARGETS = frozenset( + {TRAVEL_ASSISTANT_DIR, README_FILE, ".workshop", ".workshop_instance", ".git"} +) SCHEMA_VERSION = 1 # Paths that --auto-commit is allowed to stage. Limited to workshop-owned # locations so unrelated local edits, untracked files, or secrets are never @@ -139,6 +156,16 @@ def _compute_next_step(current_step: int) -> int: return current_step + 1 +def _compute_previous_step(current_step: int) -> int: + """Return the previous workshop step, inverting the final cleanup jump.""" + + if current_step == 0: + raise AdvanceError("Workshop is already at step 0; there is no previous step.") + if current_step == FINAL_STEP: + return TERMINAL_STEP + return current_step - 1 + + def _validate_expected(expected: str | None, current_step: int) -> None: """Validate the workflow's expected current step guard. @@ -210,6 +237,11 @@ def _root_overlay_targets() -> set[str]: if not overlay.is_dir(): continue for entry in overlay.iterdir(): + if entry.name in _PROTECTED_ROOT_TARGETS: + raise AdvanceError( + f".workshop/step_files/{step_dir.name}/{ROOT_OVERLAY_DIR}/{entry.name} " + f"targets a protected repo path ({entry.name}); rename the overlay entry." + ) targets.add(entry.name) return targets @@ -241,21 +273,22 @@ def _clear_root_targets() -> None: def _backup_root_targets(destination: Path) -> bool: - """Back up existing repo-root overlay targets under ``destination``. + """Back up existing repo-root overlay targets under ``destination/_root/``. Returns True when at least one target was backed up so callers can report it. """ backed_up = False + root_dest = destination / BACKUP_ROOT_SUBDIR for name in _root_overlay_targets(): source = _path(name) if not source.exists(): continue - dest = destination / name + dest = root_dest / name if source.is_dir(): _copy_tree(source, dest) else: - destination.mkdir(parents=True, exist_ok=True) + dest.parent.mkdir(parents=True, exist_ok=True) try: shutil.copy2(source, dest) except OSError as exc: @@ -264,10 +297,12 @@ def _backup_root_targets(destination: Path) -> bool: return backed_up -def _backup_travel_assistant(destination: Path, *, skip_if_empty: bool) -> bool: - """Back up ``travel_assistant`` to ``destination``. +def _backup_travel_assistant(destination: Path) -> bool: + """Back up ``travel_assistant`` under ``destination/travel_assistant/``. - Returns True when a backup was created. + Always copies (an empty snapshot is still a valid snapshot). Returns True when + the source had backup-worthy contents so callers can decide whether to report + it — the copy itself is unconditional. """ source = _path(TRAVEL_ASSISTANT_DIR) @@ -276,11 +311,173 @@ def _backup_travel_assistant(destination: Path, *, skip_if_empty: bool) -> bool: except OSError as exc: raise AdvanceError(f"Failed to create {source}: {exc}") from exc - if skip_if_empty and not _has_backup_worthy_contents(source): + _copy_tree(source, destination / BACKUP_AGENT_SUBDIR) + return _has_backup_worthy_contents(source) + + +def _write_backup_manifest(destination: Path, step: int) -> None: + """Write the completion manifest that marks ``destination`` as a valid snapshot. + + Written last so a snapshot is only ever considered restorable once both + namespaces have been copied in full (see :func:`_is_restorable_backup`). + """ + + payload = {"format_version": BACKUP_FORMAT_VERSION, "step": step} + _write_text(destination / BACKUP_MANIFEST, json.dumps(payload, indent=2) + "\n") + + +def _is_restorable_backup(backup_dir: Path) -> bool: + """Return True when ``backup_dir`` is a complete, new-format snapshot. + + Restoration is gated on a valid completion manifest rather than on directory + contents, so an exactly-empty snapshot is still restored verbatim and a + partially-written or legacy flattened backup is never misread as restorable. + """ + + manifest = backup_dir / BACKUP_MANIFEST + if not manifest.is_file(): return False + try: + data = json.loads(manifest.read_text(encoding="utf-8")) + except (OSError, ValueError): + return False + return isinstance(data, dict) and data.get("format_version") == BACKUP_FORMAT_VERSION - _copy_tree(source, destination) - return True + +def _snapshot_current_step(step: int) -> Path | None: + """Atomically (re)create ``workshop_backups/step-/`` for the current work. + + Builds the snapshot in a staging directory, copies ``travel_assistant/`` and + the root overlay targets into their namespaces, writes the completion + manifest last, then swaps it into place. Any prior ``step-/`` snapshot + is fully **replaced** — never merged — so files the learner deleted on a + revisit are not resurrected by a later ``--back``. The old snapshot is only + removed once the replacement is complete, so a mid-copy failure never leaves + the step without a valid backup. + + Returns the published path, or ``None`` when there was nothing worth backing + up (empty ``travel_assistant/`` and no root targets), in which case any stale + snapshot is dropped so ``--back`` reflects the emptied workspace. + """ + + final = _path(BACKUPS_DIR) / f"step-{step}" + source = _path(TRAVEL_ASSISTANT_DIR) + has_agent = source.exists() and _has_backup_worthy_contents(source) + has_root = any(_path(name).exists() for name in _root_overlay_targets()) + + if not has_agent and not has_root: + # Replace-with-empty: a revisited step the learner emptied must not keep a + # stale, restorable backup around. + if final.exists(): + try: + shutil.rmtree(final) + except OSError as exc: + raise AdvanceError(f"Failed to clear stale backup {final}: {exc}") from exc + return None + + staging = _reserve_backup_dir(f"step-{step}.staging") + if has_agent: + _backup_travel_assistant(staging) + if has_root: + _backup_root_targets(staging) + _write_backup_manifest(staging, step) + + try: + if final.exists(): + shutil.rmtree(final) + os.replace(staging, final) + except OSError as exc: + raise AdvanceError(f"Failed to publish backup {final}: {exc}") from exc + return final + + +def _restore_from_backup(backup_dir: Path) -> None: + """Restore ``travel_assistant`` and root overlay targets from ``backup_dir``. + + New-format snapshots keep the agent snapshot under ``travel_assistant/`` and + the repo-root overlay targets under ``_root/`` (see :func:`_snapshot_current_step`). + Restoration is therefore structural — the agent namespace is copied back into + ``travel_assistant/`` and each entry under ``_root/`` back to the repo root — + with no name-based routing, so a learner file inside ``travel_assistant/`` + that happens to share a root-target name is never misrouted. Callers clear + both destinations first so the restore is exact. + """ + + travel_dest = _path(TRAVEL_ASSISTANT_DIR) + try: + travel_dest.mkdir(parents=True, exist_ok=True) + except OSError as exc: + raise AdvanceError(f"Failed to create {travel_dest}: {exc}") from exc + + travel_source = backup_dir / BACKUP_AGENT_SUBDIR + if travel_source.is_dir(): + _copy_tree(travel_source, travel_dest) + + root_source = backup_dir / BACKUP_ROOT_SUBDIR + if not root_source.is_dir(): + return + try: + entries = sorted(root_source.iterdir(), key=lambda p: p.name) + except OSError as exc: + raise AdvanceError(f"Failed to read backup {root_source}: {exc}") from exc + for entry in entries: + destination = _path(entry.name) + if entry.is_dir(): + _copy_tree(entry, destination) + else: + destination.parent.mkdir(parents=True, exist_ok=True) + try: + shutil.copy2(entry, destination) + except OSError as exc: + raise AdvanceError(f"Failed to copy {entry} to {destination}: {exc}") from exc + + +def _validate_step_files_present(target_step: int) -> None: + """Ensure every canonical step directory ``0..target_step`` exists. + + ``--back`` clears ``travel_assistant/`` before rebuilding, so a missing step + directory must be fatal up front (advance merely warns). This runs before any + destructive work so a broken template never leaves a half-rebuilt workspace. + """ + + missing: list[str] = [] + for step in range(0, target_step + 1): + source = _path(STEP_FILES_DIR) / _step_dir_name(step) + if not source.exists(): + missing.append(_step_dir_name(step)) + elif not source.is_dir(): + raise AdvanceError( + f".workshop/step_files/{_step_dir_name(step)} exists but is not a directory." + ) + if missing: + joined = ", ".join(f".workshop/step_files/{name}/" for name in missing) + raise AdvanceError( + f"Cannot rebuild step {target_step}: missing canonical step files: {joined}." + ) + + +def _reserve_backup_dir(prefix: str) -> Path: + """Create and return a unique backup directory named ``-``. + + Uses an exclusive ``mkdir`` and appends ``-1``, ``-2``, ... on collision so + two backups taken within the same UTC second (e.g. a rapid back-then-back) + never merge into one directory. This keeps the "current work is always backed + up first" guarantee true even under fast repeated runs. + """ + + timestamp = datetime.now(timezone.utc).strftime("%Y%m%d%H%M%S") + base = _path(BACKUPS_DIR) / f"{prefix}-{timestamp}" + candidate = base + suffix = 1 + while True: + try: + candidate.mkdir(parents=True, exist_ok=False) + return candidate + except FileExistsError: + candidate = base.with_name(f"{base.name}-{suffix}") + suffix += 1 + except OSError as exc: + raise AdvanceError(f"Failed to create backup directory {candidate}: {exc}") from exc def _lay_down_step_files(step: int, *, clear_existing: bool = True) -> bool: @@ -512,7 +709,8 @@ def _plan_advance(current_step: int, next_step: int) -> list[str]: """Return human-readable advance actions for dry-run output.""" actions = [ - f"Would back up travel_assistant/ to .workshop_instance/workshop_backups/step-{current_step}/ if non-empty.", + f"Would snapshot travel_assistant/ (and any workshop root files) to " + f".workshop_instance/workshop_backups/step-{current_step}/, replacing any prior snapshot.", f"Would overlay .workshop/step_files/{_step_dir_name(next_step)}/ onto travel_assistant/ (keeping earlier work) if present.", ] overlay = _path(STEP_FILES_DIR) / _step_dir_name(next_step) / ROOT_OVERLAY_DIR @@ -550,11 +748,12 @@ def _advance(expected: str | None, *, dry_run: bool, auto_commit: bool) -> int: ) return 0 - backup_destination = _path(BACKUPS_DIR) / f"step-{current_step}" - if _backup_travel_assistant(backup_destination, skip_if_empty=True): - print(f"Backed up travel_assistant/ to {backup_destination.relative_to(REPO_ROOT)}") - if _backup_root_targets(backup_destination): - print(f"Backed up workshop root files to {backup_destination.relative_to(REPO_ROOT)}") + backup_destination = _snapshot_current_step(current_step) + if backup_destination is not None: + print( + "Backed up travel_assistant/ (and any workshop root files) to " + f"{backup_destination.relative_to(REPO_ROOT)}" + ) _lay_down_step_files(next_step, clear_existing=False) _write_text(_path(README_FILE), _render_readme(next_step)) _write_state(next_step) @@ -635,11 +834,12 @@ def _advance_on_push(*, dry_run: bool) -> int: _export_advanced(False) return 0 - backup_destination = _path(BACKUPS_DIR) / f"step-{current_step}" - if _backup_travel_assistant(backup_destination, skip_if_empty=True): - print(f"Backed up travel_assistant/ to {backup_destination.relative_to(REPO_ROOT)}") - if _backup_root_targets(backup_destination): - print(f"Backed up workshop root files to {backup_destination.relative_to(REPO_ROOT)}") + backup_destination = _snapshot_current_step(current_step) + if backup_destination is not None: + print( + "Backed up travel_assistant/ (and any workshop root files) to " + f"{backup_destination.relative_to(REPO_ROOT)}" + ) _lay_down_step_files(next_step, clear_existing=False) _write_text(_path(README_FILE), _render_readme(next_step)) _write_state(next_step) @@ -791,9 +991,8 @@ def _reset(*, dry_run: bool, auto_commit: bool) -> int: ) return 0 - timestamp = datetime.now(timezone.utc).strftime("%Y%m%d%H%M%S") - backup_destination = _path(BACKUPS_DIR) / f"reset-{timestamp}" - if _backup_travel_assistant(backup_destination, skip_if_empty=False): + backup_destination = _reserve_backup_dir("reset") + if _backup_travel_assistant(backup_destination): print(f"Backed up travel_assistant/ to {backup_destination.relative_to(REPO_ROOT)}") if _backup_root_targets(backup_destination): print(f"Backed up workshop root files to {backup_destination.relative_to(REPO_ROOT)}") @@ -822,6 +1021,112 @@ def _reset(*, dry_run: bool, auto_commit: bool) -> int: return 0 +def _plan_back(current_step: int, previous_step: int, *, restore_from_backup: bool) -> list[str]: + """Return human-readable back actions for dry-run output.""" + + actions = [ + "Would back up travel_assistant/ (and workshop root files) to " + ".workshop_instance/workshop_backups/back-/.", + "Would clear travel_assistant/ and workshop root overlay targets.", + ] + if restore_from_backup: + actions.append( + "Would restore travel_assistant/ (and workshop root files) from " + f".workshop_instance/workshop_backups/step-{previous_step}/ (your saved work)." + ) + else: + actions.append( + f"No .workshop_instance/workshop_backups/step-{previous_step}/ backup found; " + f"would rebuild canonical step files 0..{previous_step} onto travel_assistant/ " + "(in-place edits from those steps are not restored)." + ) + actions.extend( + [ + f"Would render README.md for step {previous_step}.", + f"Would update {STATE_FILE} to current_step {previous_step}.", + f"Would export NEW_STEP={previous_step} if GITHUB_ENV is set.", + f"Current step is {current_step}.", + ] + ) + return actions + + +def _back(*, dry_run: bool, auto_commit: bool) -> int: + """Move the workshop back one step. + + Prefers restoring the learner's saved work from + ``workshop_backups/step-/`` (the snapshot advance took before + moving off that step). When that backup is absent — e.g. the learner cloned + at a mid step or never advanced through the script — it falls back to + rebuilding the canonical step files with a clear warning. Either way the + current work is backed up first, so nothing is lost. + """ + + current_step = _load_state() + _validate_state_sync(current_step, _read_readme_marker()) + previous_step = _compute_previous_step(current_step) + + backup_source = _path(BACKUPS_DIR) / f"step-{previous_step}" + restore_from_backup = _is_restorable_backup(backup_source) + # A directory without a valid completion manifest is a legacy flattened backup + # (or a partial write): it cannot be restored safely, so we fall back to a + # canonical rebuild but say so precisely. + legacy_backup = not restore_from_backup and backup_source.is_dir() + + # Validate everything that could fail BEFORE any destructive work so a broken + # template or render never leaves travel_assistant/ half-rebuilt. + if not restore_from_backup: + _validate_step_files_present(previous_step) + readme_text = _render_readme(previous_step) + + if dry_run: + print(f"DRY RUN: going back step {current_step} -> {previous_step}") + for action in _plan_back(current_step, previous_step, restore_from_backup=restore_from_backup): + print(action) + if auto_commit: + print( + "Would auto-commit workshop-owned paths with message " + f"'workshop: go back to step {previous_step}'." + ) + return 0 + + backup_destination = _reserve_backup_dir("back") + if _backup_travel_assistant(backup_destination): + print(f"Backed up travel_assistant/ to {backup_destination.relative_to(REPO_ROOT)}") + if _backup_root_targets(backup_destination): + print(f"Backed up workshop root files to {backup_destination.relative_to(REPO_ROOT)}") + + _clear_travel_assistant() + _clear_root_targets() + + if restore_from_backup: + _restore_from_backup(backup_source) + print(f"Restored travel_assistant/ from {backup_source.relative_to(REPO_ROOT)}") + else: + reason = ( + f"a legacy {backup_source.relative_to(REPO_ROOT)} backup exists but predates the " + "restorable backup format and cannot be safely restored" + if legacy_backup + else f"no saved {backup_source.relative_to(REPO_ROOT)} backup found" + ) + print( + f"WARNING: {reason}; rebuilt canonical starter files for step {previous_step}. " + "In-place edits from those steps are not restored (your current work was backed " + f"up to {backup_destination.relative_to(REPO_ROOT)}).", + file=sys.stderr, + ) + for step in range(0, previous_step + 1): + _lay_down_step_files(step, clear_existing=False) + + _write_text(_path(README_FILE), readme_text) + _write_state(previous_step) + _export_new_step(previous_step) + print(f"Moved workshop back to step {previous_step}: {STEP_TITLES.get(previous_step, 'Unknown')}") + if auto_commit: + _auto_commit(f"workshop: go back to step {previous_step}") + return 0 + + def _parse_args(argv: Sequence[str]) -> argparse.Namespace: """Parse command-line arguments.""" @@ -844,6 +1149,16 @@ def _parse_args(argv: Sequence[str]) -> argparse.Namespace: action="store_true", help="Initialize a fresh template repository at step 0 without backing up travel_assistant/.", ) + mode_group.add_argument( + "--back", + action="store_true", + help=( + "Move back one workshop step. Restores your saved work from " + ".workshop_instance/workshop_backups/step-/ when present, otherwise " + "rebuilds the canonical step files. Current work is backed up first. " + "Errors at step 0." + ), + ) mode_group.add_argument( "--on-push", dest="on_push", @@ -877,6 +1192,8 @@ def main(argv: Sequence[str] | None = None) -> int: return _init(dry_run=args.dry_run) if args.reset: return _reset(dry_run=args.dry_run, auto_commit=args.auto_commit) + if args.back: + return _back(dry_run=args.dry_run, auto_commit=args.auto_commit) if args.on_push: return _advance_on_push(dry_run=args.dry_run) return _advance(args.expected, dry_run=args.dry_run, auto_commit=args.auto_commit) diff --git a/.workshop/scripts/tests/test_advance_step.py b/.workshop/scripts/tests/test_advance_step.py index daeaa34..4d3ad67 100644 --- a/.workshop/scripts/tests/test_advance_step.py +++ b/.workshop/scripts/tests/test_advance_step.py @@ -122,7 +122,7 @@ def test_happy_path_advances_and_exports_new_step(workshop_repo, monkeypatch): assert result == 0 assert json.loads((workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text())["current_step"] == 1 assert "# Synthetic step 1" in (workshop_repo / "README.md").read_text(encoding="utf-8") - assert (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-0" / "stub.txt").read_text(encoding="utf-8") == "old" + assert (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-0" / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "old" assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "new step 1" assert github_env.read_text(encoding="utf-8") == "NEW_STEP=1\n" @@ -200,7 +200,7 @@ def test_missing_step_files_warns_and_keeps_travel_assistant(workshop_repo, caps assert json.loads((workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text())["current_step"] == 5 assert "# Synthetic step 5" in (workshop_repo / "README.md").read_text(encoding="utf-8") assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "keep me" - assert (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-4" / "stub.txt").read_text(encoding="utf-8") == "keep me" + assert (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-4" / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "keep me" def test_empty_travel_assistant_skips_backup(workshop_repo): @@ -254,7 +254,7 @@ def test_advance_preserves_prior_step_files(workshop_repo): # Files carried forward from the prior step survive the advance (incremental). assert (workshop_repo / "travel_assistant" / "leftover.py").read_text(encoding="utf-8") == "# stale\n" # A backup is still taken before laying down the next step. - assert (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-0" / "leftover.py").exists() + assert (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-0" / "travel_assistant" / "leftover.py").exists() def test_reset_backs_up_and_returns_to_step_zero(workshop_repo): @@ -271,7 +271,7 @@ def test_reset_backs_up_and_returns_to_step_zero(workshop_repo): assert "# Synthetic step 0" in (workshop_repo / "README.md").read_text(encoding="utf-8") backups = list((workshop_repo / ".workshop_instance" / "workshop_backups").glob("reset-*")) assert len(backups) == 1 - assert (backups[0] / "stub.txt").read_text(encoding="utf-8") == "step 5 work" + assert (backups[0] / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "step 5 work" assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "reset starter" assert not (workshop_repo / "travel_assistant" / "old.txt").exists() @@ -705,7 +705,7 @@ def test_advance_backs_up_existing_root_overlay(workshop_repo): assert result == 0 assert ( - workshop_repo / ".workshop_instance" / "workshop_backups" / "step-1" / "travel_toolbox" / "toolbox.yaml" + workshop_repo / ".workshop_instance" / "workshop_backups" / "step-1" / "_root" / "travel_toolbox" / "toolbox.yaml" ).read_text(encoding="utf-8") == "v1" @@ -724,7 +724,7 @@ def test_reset_removes_and_backs_up_root_overlay_target(workshop_repo): assert not (workshop_repo / "travel_toolbox").exists() backups = list((workshop_repo / ".workshop_instance" / "workshop_backups").glob("reset-*")) assert backups, "reset should create a timestamped backup" - assert (backups[0] / "travel_toolbox" / "toolbox.yaml").read_text(encoding="utf-8") == "live" + assert (backups[0] / "_root" / "travel_toolbox" / "toolbox.yaml").read_text(encoding="utf-8") == "live" def test_dry_run_reports_root_overlay_without_writing(workshop_repo, capsys): @@ -938,3 +938,435 @@ def test_on_push_missing_state_after_init_fails_loudly(workshop_repo, monkeypatc assert result == 1 assert "workshop state" in captured.err.lower() + + +# === --back (local move-back-one-step) tests === + + +def _backups_dir(repo: Path) -> Path: + return repo / ".workshop_instance" / "workshop_backups" + + +def _seed_step_backup(repo: Path, step: int, relpath: str, content: str) -> None: + """Seed the agent snapshot ``step-/travel_assistant/`` + manifest. + + Mirrors what advance's atomic snapshot writes: the travel_assistant/ namespace + plus the completion manifest that marks the backup as restorable. + """ + + target = _backups_dir(repo) / f"step-{step}" / "travel_assistant" / relpath + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + _seed_backup_manifest(repo, step) + + +def _seed_step_root_backup(repo: Path, step: int, relpath: str, content: str) -> None: + """Seed a repo-root overlay target ``step-/_root/`` + manifest.""" + + target = _backups_dir(repo) / f"step-{step}" / "_root" / relpath + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + _seed_backup_manifest(repo, step) + + +def _seed_backup_manifest(repo: Path, step: int) -> None: + """Write the completion manifest that marks ``step-/`` as restorable.""" + + manifest = _backups_dir(repo) / f"step-{step}" / "backup.json" + manifest.parent.mkdir(parents=True, exist_ok=True) + manifest.write_text( + json.dumps({"format_version": 2, "step": step}, indent=2) + "\n", + encoding="utf-8", + ) + + +def test_back_restores_saved_work_from_backup(workshop_repo): + """Back prefers the step- snapshot so the learner's real work returns.""" + + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + # The snapshot advance took before leaving step 1 (the learner's edited work). + _seed_step_backup(workshop_repo, 1, "stub.txt", "my edited step 1") + # Current step-2 workspace: a carried file plus a step-2-only file. + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 2 work", encoding="utf-8") + (workshop_repo / "travel_assistant" / "step2_only.py").write_text("# step 2\n", encoding="utf-8") + + result = advance_step.main(["--back"]) + + assert result == 0 + assert json.loads( + (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + )["current_step"] == 1 + assert "# Synthetic step 1" in (workshop_repo / "README.md").read_text(encoding="utf-8") + # The learner's saved step-1 file is restored verbatim... + assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "my edited step 1" + # ...and the step-2-only file is gone (it wasn't part of step 1). + assert not (workshop_repo / "travel_assistant" / "step2_only.py").exists() + # Current work was snapshotted before the destructive restore. + back_backups = list(_backups_dir(workshop_repo).glob("back-*")) + assert len(back_backups) == 1 + assert (back_backups[0] / "travel_assistant" / "step2_only.py").read_text(encoding="utf-8") == "# step 2\n" + + +def test_back_at_step_zero_errors_without_writes(workshop_repo, capsys): + _write_state(workshop_repo, 0) + _write_readme(workshop_repo, 0) + (workshop_repo / "travel_assistant" / "stub.txt").write_text("setup", encoding="utf-8") + + result = advance_step.main(["--back"]) + captured = capsys.readouterr() + + assert result == 1 + assert "already at step 0" in captured.err + assert json.loads( + (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + )["current_step"] == 0 + assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "setup" + assert not list(_backups_dir(workshop_repo).glob("back-*")) + + +def test_back_from_final_step_returns_to_terminal(workshop_repo): + """99 -> 9 is the inverse of the cleanup jump and restores the step-9 snapshot.""" + + _write_state(workshop_repo, 99) + _write_readme(workshop_repo, 99) + _seed_step_backup(workshop_repo, 9, "stub.txt", "step 9 work") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("cleanup", encoding="utf-8") + + result = advance_step.main(["--back"]) + + assert result == 0 + assert json.loads( + (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + )["current_step"] == 9 + assert "# Synthetic step 9" in (workshop_repo / "README.md").read_text(encoding="utf-8") + assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "step 9 work" + + +def test_back_dry_run_has_no_side_effects(workshop_repo, capsys): + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + _seed_step_backup(workshop_repo, 1, "stub.txt", "my edited step 1") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 2 work", encoding="utf-8") + original_state = (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + original_readme = (workshop_repo / "README.md").read_text(encoding="utf-8") + + result = advance_step.main(["--back", "--dry-run"]) + captured = capsys.readouterr() + + assert result == 0 + assert "DRY RUN: going back step 2 -> 1" in captured.out + assert "would restore" in captured.out.lower() + assert (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") == original_state + assert (workshop_repo / "README.md").read_text(encoding="utf-8") == original_readme + assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "step 2 work" + assert not list(_backups_dir(workshop_repo).glob("back-*")) + + +def test_back_rebuilds_canonically_when_no_backup(workshop_repo, capsys): + """Without a step- snapshot, back rebuilds canonical files and warns.""" + + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + # No workshop_backups/step-1/ exists. Provide canonical step files for 0..1. + (workshop_repo / ".workshop" / "step_files" / "00").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "00" / "base0.txt").write_text("b0", encoding="utf-8") + (workshop_repo / ".workshop" / "step_files" / "01").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "01" / "base1.txt").write_text("b1", encoding="utf-8") + (workshop_repo / "travel_assistant" / "step2_only.py").write_text("# step 2\n", encoding="utf-8") + + result = advance_step.main(["--back"]) + captured = capsys.readouterr() + + assert result == 0 + assert "no saved" in captured.err.lower() + assert json.loads( + (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + )["current_step"] == 1 + # Canonical step 0 and step 1 files are laid down... + assert (workshop_repo / "travel_assistant" / "base0.txt").read_text(encoding="utf-8") == "b0" + assert (workshop_repo / "travel_assistant" / "base1.txt").read_text(encoding="utf-8") == "b1" + # ...and the step-2-only file is gone. + assert not (workshop_repo / "travel_assistant" / "step2_only.py").exists() + # Current work is still snapshotted before the rebuild. + back_backups = list(_backups_dir(workshop_repo).glob("back-*")) + assert len(back_backups) == 1 + assert (back_backups[0] / "travel_assistant" / "step2_only.py").exists() + + +def test_back_rebuild_missing_step_files_errors_before_clearing(workshop_repo, capsys): + """A canonical rebuild with a missing step dir must abort before any clear.""" + + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + # step_files/00 exists but step_files/01 is intentionally missing, and there + # is no workshop_backups/step-1/ to restore from. + (workshop_repo / ".workshop" / "step_files" / "00").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "00" / "base0.txt").write_text("b0", encoding="utf-8") + (workshop_repo / "travel_assistant" / "work.py").write_text("keep me", encoding="utf-8") + + result = advance_step.main(["--back"]) + captured = capsys.readouterr() + + assert result == 1 + assert ".workshop/step_files/01/" in captured.err + # Nothing destructive happened: state, workspace, and backups are untouched. + assert json.loads( + (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + )["current_step"] == 2 + assert (workshop_repo / "travel_assistant" / "work.py").read_text(encoding="utf-8") == "keep me" + assert not list(_backups_dir(workshop_repo).glob("back-*")) + + +def test_back_state_desync_exits_nonzero(workshop_repo, capsys): + """State/README disagreement aborts back before any destructive work.""" + + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 3) + (workshop_repo / "travel_assistant" / "stub.txt").write_text("work", encoding="utf-8") + + result = advance_step.main(["--back"]) + captured = capsys.readouterr() + + assert result == 1 + assert "State desync" in captured.err + assert not list(_backups_dir(workshop_repo).glob("back-*")) + + +def test_back_restores_root_overlay_target_to_repo_root(workshop_repo): + """Restore routes known root-overlay names to the repo root, not travel_assistant/.""" + + _write_state(workshop_repo, 5) + _write_readme(workshop_repo, 5) + # Declare travel_toolbox as a root overlay target (discovered from any step). + _create_root_overlay(workshop_repo, 4, "travel_toolbox/toolbox.yaml", "declared") + # The step-4 snapshot holds both travel_assistant/ content and the root target. + _seed_step_backup(workshop_repo, 4, "stub.txt", "step 4 work") + _seed_step_root_backup(workshop_repo, 4, "travel_toolbox/toolbox.yaml", "backed up") + # Current step-5 state on disk. + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 5 work", encoding="utf-8") + (workshop_repo / "travel_toolbox").mkdir() + (workshop_repo / "travel_toolbox" / "toolbox.yaml").write_text("live step 5", encoding="utf-8") + + result = advance_step.main(["--back"]) + + assert result == 0 + assert json.loads( + (workshop_repo / ".workshop_instance" / ".workshop-state.json").read_text(encoding="utf-8") + )["current_step"] == 4 + # The root target is restored at the repo root... + assert (workshop_repo / "travel_toolbox" / "toolbox.yaml").read_text(encoding="utf-8") == "backed up" + # ...and NOT copied inside travel_assistant/. + assert not (workshop_repo / "travel_assistant" / "travel_toolbox").exists() + assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "step 4 work" + # The live step-5 root target was snapshotted first. + back_backups = list(_backups_dir(workshop_repo).glob("back-*")) + assert (back_backups[0] / "_root" / "travel_toolbox" / "toolbox.yaml").read_text(encoding="utf-8") == "live step 5" + + +@_requires_git +def test_back_auto_commit_creates_go_back_commit(workshop_repo): + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + _seed_step_backup(workshop_repo, 1, "stub.txt", "my edited step 1") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 2 work", encoding="utf-8") + _git_init_with_identity(workshop_repo) + + result = advance_step.main(["--back", "--auto-commit"]) + + assert result == 0 + log = _git(workshop_repo, "log", "-1", "--format=%s").stdout.strip() + assert log == "workshop: go back to step 1" + + +def test_back_and_reset_are_mutually_exclusive(workshop_repo, capsys): + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + + with pytest.raises(SystemExit): + advance_step.main(["--back", "--reset"]) + captured = capsys.readouterr() + assert "not allowed with argument" in captured.err or "argument" in captured.err + + +def test_back_backups_do_not_merge_within_same_second(workshop_repo, monkeypatch): + """Two backs in the same UTC second must land in distinct backup dirs. + + ``_reserve_backup_dir`` uses an exclusive mkdir and a ``-N`` suffix so a + rapid back-then-back never merges the two snapshots into one directory, + keeping the "current work is always backed up first" guarantee true. + """ + + class _FrozenDatetime(advance_step.datetime): + @classmethod + def now(cls, tz=None): + return cls(2026, 7, 9, 12, 0, 0, tzinfo=tz) + + monkeypatch.setattr(advance_step, "datetime", _FrozenDatetime) + + # 3 -> 2 (backs up "step 3 work"), then 2 -> 1 (backs up restored "step 2 saved"). + _write_state(workshop_repo, 3) + _write_readme(workshop_repo, 3) + _seed_step_backup(workshop_repo, 2, "stub.txt", "step 2 saved") + _seed_step_backup(workshop_repo, 1, "stub.txt", "step 1 saved") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 3 work", encoding="utf-8") + + assert advance_step.main(["--back"]) == 0 + assert advance_step.main(["--back"]) == 0 + + back_dirs = sorted(p.name for p in _backups_dir(workshop_repo).glob("back-*")) + assert len(back_dirs) == 2, f"expected two distinct back dirs, got {back_dirs}" + contents = { + (p / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") + for p in _backups_dir(workshop_repo).glob("back-*") + } + assert contents == {"step 3 work", "step 2 saved"} + + +def test_back_does_not_misroute_agent_file_named_after_root_target(workshop_repo): + """#1: a travel_assistant/ entry sharing a root-target name round-trips back + into travel_assistant/, never the repo root. + + The namespaced backup layout keeps the agent snapshot under + ``travel_assistant/`` and root targets under ``_root/``, so restore is + structural and cannot misroute a learner dir that happens to be named after + a root overlay target. + """ + + _write_state(workshop_repo, 1) + _write_readme(workshop_repo, 1) + # travel_toolbox is a declared root overlay target... + _create_root_overlay(workshop_repo, 2, "travel_toolbox/toolbox.yaml", "declared") + _create_step_files(workshop_repo, 2, "step 2 file") + # ...but the learner also has a DIRECTORY named travel_toolbox INSIDE + # travel_assistant/ (the collision case). + inner = workshop_repo / "travel_assistant" / "travel_toolbox" + inner.mkdir() + (inner / "notes.md").write_text("my agent notes", encoding="utf-8") + + assert advance_step.main(["--expected", "1"]) == 0 # snapshots step-1 + assert advance_step.main(["--back"]) == 0 # restores step-1 + + # The learner's file returns INSIDE travel_assistant/, not at the repo root. + assert (workshop_repo / "travel_assistant" / "travel_toolbox" / "notes.md").read_text( + encoding="utf-8" + ) == "my agent notes" + assert not (workshop_repo / "travel_toolbox" / "notes.md").exists() + + +def test_back_after_revisit_does_not_resurrect_deleted_file(workshop_repo): + """#3: re-advancing a revisited step REPLACES its backup, so a file deleted + on the revisit is not resurrected by a later back.""" + + _write_state(workshop_repo, 1) + _write_readme(workshop_repo, 1) + (workshop_repo / "travel_assistant" / "keep.txt").write_text("keep", encoding="utf-8") + (workshop_repo / "travel_assistant" / "doomed.txt").write_text("delete me later", encoding="utf-8") + _create_step_files(workshop_repo, 2, "step 2 file") + + assert advance_step.main(["--expected", "1"]) == 0 # 1 -> 2, snapshots step-1 + assert advance_step.main(["--back"]) == 0 # 2 -> 1, restores both files + assert (workshop_repo / "travel_assistant" / "doomed.txt").exists() + + # On the revisit the learner deletes doomed.txt, then re-advances. + (workshop_repo / "travel_assistant" / "doomed.txt").unlink() + assert advance_step.main(["--expected", "1"]) == 0 # 1 -> 2, REPLACES step-1 + + assert advance_step.main(["--back"]) == 0 # 2 -> 1 again + assert (workshop_repo / "travel_assistant" / "keep.txt").read_text(encoding="utf-8") == "keep" + assert not (workshop_repo / "travel_assistant" / "doomed.txt").exists() + + +def test_advance_drops_stale_backup_when_workspace_emptied(workshop_repo): + """#3 (empty case): emptying a revisited workspace before re-advancing drops + the stale snapshot so a later back never restores removed files.""" + + _write_state(workshop_repo, 1) + _write_readme(workshop_repo, 1) + _seed_step_backup(workshop_repo, 1, "old.txt", "stale") + # The learner emptied travel_assistant/ down to the placeholder. + (workshop_repo / "travel_assistant" / ".gitkeep").write_text("", encoding="utf-8") + _create_step_files(workshop_repo, 2, "step 2 file") + + assert advance_step.main(["--expected", "1"]) == 0 + + # Nothing worth backing up -> the stale step-1 snapshot is gone. + assert not (workshop_repo / ".workshop_instance" / "workshop_backups" / "step-1").exists() + + +def test_back_treats_legacy_flattened_backup_as_unrestorable(workshop_repo, capsys): + """A legacy step- backup without a manifest is not restored; back falls + back to a canonical rebuild and says the backup is legacy.""" + + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + # Legacy flattened backup: files directly under step-1/, no manifest, no namespaces. + legacy = _backups_dir(workshop_repo) / "step-1" + legacy.mkdir(parents=True) + (legacy / "stub.txt").write_text("legacy work", encoding="utf-8") + # Canonical step files for the rebuild fallback. + (workshop_repo / ".workshop" / "step_files" / "00").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "00" / "base0.txt").write_text("b0", encoding="utf-8") + (workshop_repo / ".workshop" / "step_files" / "01").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "01" / "base1.txt").write_text("b1", encoding="utf-8") + (workshop_repo / "travel_assistant" / "step2_only.py").write_text("# step 2\n", encoding="utf-8") + + result = advance_step.main(["--back"]) + captured = capsys.readouterr() + + assert result == 0 + assert "legacy" in captured.err.lower() + # Canonical rebuild happened; the legacy content was NOT restored. + assert (workshop_repo / "travel_assistant" / "base1.txt").read_text(encoding="utf-8") == "b1" + assert not (workshop_repo / "travel_assistant" / "stub.txt").exists() + + +def test_back_restores_root_target_that_is_a_file(workshop_repo): + """A root overlay target that is a FILE (not a dir) restores from _root/.""" + + _write_state(workshop_repo, 5) + _write_readme(workshop_repo, 5) + _create_root_overlay(workshop_repo, 4, "toolbox.yaml", "declared") # a file target + _seed_step_backup(workshop_repo, 4, "stub.txt", "step 4 work") + _seed_step_root_backup(workshop_repo, 4, "toolbox.yaml", "backed up file") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 5 work", encoding="utf-8") + (workshop_repo / "toolbox.yaml").write_text("live", encoding="utf-8") + + assert advance_step.main(["--back"]) == 0 + + assert (workshop_repo / "toolbox.yaml").read_text(encoding="utf-8") == "backed up file" + assert not (workshop_repo / "travel_assistant" / "toolbox.yaml").exists() + + +def test_root_overlay_rejects_protected_target_name(workshop_repo): + """A _root overlay that targets a protected repo path is rejected up front.""" + + _create_root_overlay(workshop_repo, 2, "travel_assistant/oops.txt", "x") + + with pytest.raises(advance_step.AdvanceError, match="protected repo path"): + advance_step._root_overlay_targets() + + +def test_reset_backups_are_unique_within_same_second(workshop_repo, monkeypatch): + """Two resets in the same UTC second land in distinct backup dirs.""" + + class _FrozenDatetime(advance_step.datetime): + @classmethod + def now(cls, tz=None): + return cls(2026, 7, 9, 12, 0, 0, tzinfo=tz) + + monkeypatch.setattr(advance_step, "datetime", _FrozenDatetime) + + _create_step_files(workshop_repo, 0, "starter") + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + (workshop_repo / "travel_assistant" / "stub.txt").write_text("work a", encoding="utf-8") + + assert advance_step.main(["--reset"]) == 0 + (workshop_repo / "travel_assistant" / "stub.txt").write_text("work b", encoding="utf-8") + assert advance_step.main(["--reset"]) == 0 + + reset_dirs = sorted(p.name for p in _backups_dir(workshop_repo).glob("reset-*")) + assert len(reset_dirs) == 2, f"expected two distinct reset dirs, got {reset_dirs}" + + diff --git a/Makefile b/Makefile index 7e9726a..c069fa0 100644 --- a/Makefile +++ b/Makefile @@ -9,22 +9,27 @@ PYTHON ?= python -.PHONY: help advance reset preflight +.PHONY: help advance back reset preflight help: @echo "Workshop targets (fully-local flow):" @echo " make advance Advance to the next workshop step and auto-commit" + @echo " make back Move back one workshop step and auto-commit" @echo " make reset Reset the workshop to step 0 and auto-commit" @echo " make preflight Run environment preflight checks" @echo "" @echo "If 'make' is unavailable, run the scripts directly, e.g.:" @echo " $(PYTHON) .workshop/scripts/advance_step.py --expected-current-step 0 --auto-commit" + @echo " $(PYTHON) .workshop/scripts/advance_step.py --back --auto-commit" @echo " $(PYTHON) .workshop/scripts/advance_step.py --reset --auto-commit" @echo " $(PYTHON) .workshop/scripts/preflight.py" advance: $(PYTHON) .workshop/scripts/advance_step.py --auto-commit +back: + $(PYTHON) .workshop/scripts/advance_step.py --back --auto-commit + reset: $(PYTHON) .workshop/scripts/advance_step.py --reset --auto-commit diff --git a/README.md b/README.md index eb82ae4..61013d0 100644 --- a/README.md +++ b/README.md @@ -76,16 +76,25 @@ python .workshop/scripts/advance_step.py --reset --auto-commit Your previous `travel_assistant/` is preserved under `.workshop_instance/workshop_backups/reset-/`. +**Move back one step:** + +```bash +python .workshop/scripts/advance_step.py --back --auto-commit +``` + +Advancing has no built-in undo when you work locally without committing each step, so this is how you step back. It restores your saved work from `.workshop_instance/workshop_backups/step-/` (the snapshot advance takes before leaving a step); if that snapshot is missing it rebuilds the canonical step files instead and warns you. Your current work is always backed up to `.workshop_instance/workshop_backups/back-/` first, and it errors at step 0. + **Re-run preflight:** ```bash python .workshop/scripts/preflight.py ``` -**Shortcuts (optional):** the repo ships a `Makefile` with three aliases: +**Shortcuts (optional):** the repo ships a `Makefile` with these aliases: ```bash make advance # advance to the next step (auto-commits workshop paths) +make back # move back one step (auto-commits workshop paths) make reset # reset to step 0 (auto-commits workshop paths) make preflight # run environment checks ``` From b99ba252e6902de43c7455c722db14ef23c5c329 Mon Sep 17 00:00:00 2001 From: Pierre Malarme Date: Thu, 16 Jul 2026 13:25:36 +0200 Subject: [PATCH 2/2] Harden backup snapshot/restore per review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address rubber-duck findings on the local --back backup engine: - Atomic publish: snapshot now renames the prior step-/ aside, swaps the new snapshot in, and only deletes the old copy once the swap succeeds, with rollback on failure and staging cleanup on every failure path — so a mid-swap crash never leaves a step without a valid backup (#1). - Always capture the travel_assistant/ namespace, even a lone .gitkeep, so a root-only snapshot restores the placeholder instead of an empty dir (#2). - Protect repo machinery: _root overlay targets that resolve to MACHINERY_PATHS or .git are rejected up front, compared casefolded so case variants can't slip past on case-insensitive filesystems (#3). - Exact restore: clear each destination before copying so an undeclared stale target (e.g. left after a template sync) is replaced, not merged (#4). - Validate the backup manifest step is an integer before trusting it (#5). Add regression tests for each fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fea011c8-1399-4f8a-a3bd-277408cae6c2 --- .workshop/scripts/advance_step.py | 145 +++++++++++++------ .workshop/scripts/tests/test_advance_step.py | 128 ++++++++++++++++ 2 files changed, 232 insertions(+), 41 deletions(-) diff --git a/.workshop/scripts/advance_step.py b/.workshop/scripts/advance_step.py index be82fb8..87c59a4 100644 --- a/.workshop/scripts/advance_step.py +++ b/.workshop/scripts/advance_step.py @@ -55,11 +55,9 @@ BACKUP_MANIFEST = "backup.json" BACKUP_FORMAT_VERSION = 2 # Repo-root paths a _root overlay target must never shadow — backing up, clearing, -# or restoring such a name would corrupt the repository or the backup store -# itself (e.g. a target named ".workshop_instance" would delete the backups). -_PROTECTED_ROOT_TARGETS = frozenset( - {TRAVEL_ASSISTANT_DIR, README_FILE, ".workshop", ".workshop_instance", ".git"} -) +# or restoring such a name would corrupt the repository or the backup store itself +# (e.g. a target named ".workshop_instance" would delete the backups). Defined just +# below MACHINERY_PATHS, which it reuses as the single source of truth. SCHEMA_VERSION = 1 # Commit-message sentinel that advance-on-push.yml looks for to suppress an # auto-advance. reset-current re-lays the CURRENT step (it must not move to the @@ -95,6 +93,15 @@ "SUPPORT.md", "LICENSE", ) +# Repo-root names a _root overlay target must never shadow. A target that resolves +# to workshop machinery/platform scaffolding (MACHINERY_PATHS) or to the Git +# metadata directory would let reset/back back up, clear, or restore over the +# repository's own infrastructure — so those names are rejected up front. Stored +# casefolded and compared casefolded so a case variant (e.g. ".GIT" on a +# case-insensitive filesystem) cannot slip past the guard. +_PROTECTED_ROOT_TARGETS = frozenset( + name.casefold() for name in (*MACHINERY_PATHS, TRAVEL_ASSISTANT_DIR, ".git") +) # Paths that --auto-commit is allowed to stage. Limited to workshop-owned # locations so unrelated local edits, untracked files, or secrets are never # swept into a commit by accident. @@ -294,6 +301,34 @@ def _copy_tree(source: Path, destination: Path, *, ignore=None) -> None: raise AdvanceError(f"Failed to copy {source} to {destination}: {exc}") from exc +def _replace_path_with(source: Path | None, destination: Path) -> None: + """Clear ``destination`` then copy ``source`` onto it (exact replacement). + + Removes whatever is at ``destination`` first — a directory, a file, or a + symlink — so the copy never merges into leftover contents. When ``source`` is + ``None`` the destination is only cleared. Used by restore so a snapshot is + reproduced exactly even over a stale target the caller did not clear. + """ + + try: + if destination.is_dir() and not destination.is_symlink(): + shutil.rmtree(destination) + elif destination.exists() or destination.is_symlink(): + destination.unlink() + except OSError as exc: + raise AdvanceError(f"Failed to clear {destination}: {exc}") from exc + if source is None: + return + if source.is_dir(): + _copy_tree(source, destination) + else: + destination.parent.mkdir(parents=True, exist_ok=True) + try: + shutil.copy2(source, destination) + except OSError as exc: + raise AdvanceError(f"Failed to copy {source} to {destination}: {exc}") from exc + + def _root_overlay_targets() -> set[str]: """Return the top-level repo-root paths the workshop owns via ``_root`` overlays. @@ -312,7 +347,7 @@ def _root_overlay_targets() -> set[str]: if not overlay.is_dir(): continue for entry in overlay.iterdir(): - if entry.name in _PROTECTED_ROOT_TARGETS: + if entry.name.casefold() in _PROTECTED_ROOT_TARGETS: raise AdvanceError( f".workshop/step_files/{step_dir.name}/{ROOT_OVERLAY_DIR}/{entry.name} " f"targets a protected repo path ({entry.name}); rename the overlay entry." @@ -416,19 +451,31 @@ def _is_restorable_backup(backup_dir: Path) -> bool: data = json.loads(manifest.read_text(encoding="utf-8")) except (OSError, ValueError): return False - return isinstance(data, dict) and data.get("format_version") == BACKUP_FORMAT_VERSION + return ( + isinstance(data, dict) + and data.get("format_version") == BACKUP_FORMAT_VERSION + # A well-formed v2 manifest always records an integer step (bool is + # rejected explicitly); anything else is a malformed or forged manifest. + and isinstance(data.get("step"), int) + and not isinstance(data.get("step"), bool) + ) def _snapshot_current_step(step: int) -> Path | None: """Atomically (re)create ``workshop_backups/step-/`` for the current work. - Builds the snapshot in a staging directory, copies ``travel_assistant/`` and - the root overlay targets into their namespaces, writes the completion - manifest last, then swaps it into place. Any prior ``step-/`` snapshot - is fully **replaced** — never merged — so files the learner deleted on a - revisit are not resurrected by a later ``--back``. The old snapshot is only - removed once the replacement is complete, so a mid-copy failure never leaves - the step without a valid backup. + Builds the snapshot in a staging directory, always copies ``travel_assistant/`` + (even when it only holds a ``.gitkeep`` placeholder) plus any root overlay + targets into their namespaces, writes the completion manifest last, then swaps + it into place. Any prior ``step-/`` snapshot is fully **replaced** — never + merged — so files the learner deleted on a revisit are not resurrected by a + later ``--back``. + + Publication is crash-safe: the prior snapshot is renamed aside first, the new + snapshot is renamed into place, and only then is the old copy deleted. If the + swap fails, the prior snapshot is rolled back, so there is never a moment where + the step has no valid backup. The staging directory is removed on every failure + path. Returns the published path, or ``None`` when there was nothing worth backing up (empty ``travel_assistant/`` and no root targets), in which case any stale @@ -451,18 +498,38 @@ def _snapshot_current_step(step: int) -> Path | None: return None staging = _reserve_backup_dir(f"step-{step}.staging") - if has_agent: + superseded: Path | None = None + try: + # Always capture the agent namespace so a restore reproduces travel_assistant/ + # exactly — including a lone .gitkeep — even when the only real work this + # step was root-level. _backup_travel_assistant(staging) - if has_root: - _backup_root_targets(staging) - _write_backup_manifest(staging, step) + if has_root: + _backup_root_targets(staging) + _write_backup_manifest(staging, step) - try: if final.exists(): - shutil.rmtree(final) - os.replace(staging, final) - except OSError as exc: - raise AdvanceError(f"Failed to publish backup {final}: {exc}") from exc + superseded = final.with_name(f"{final.name}.superseded") + try: + if superseded.exists(): + shutil.rmtree(superseded) + os.replace(final, superseded) + except OSError as exc: + raise AdvanceError( + f"Failed to set aside previous backup {final}: {exc}" + ) from exc + try: + os.replace(staging, final) + except OSError as exc: + # Roll the prior snapshot back so a failed publish never loses it. + if superseded is not None and not final.exists(): + os.replace(superseded, final) + raise AdvanceError(f"Failed to publish backup {final}: {exc}") from exc + except BaseException: + shutil.rmtree(staging, ignore_errors=True) + raise + if superseded is not None: + shutil.rmtree(superseded, ignore_errors=True) return final @@ -474,19 +541,23 @@ def _restore_from_backup(backup_dir: Path) -> None: Restoration is therefore structural — the agent namespace is copied back into ``travel_assistant/`` and each entry under ``_root/`` back to the repo root — with no name-based routing, so a learner file inside ``travel_assistant/`` - that happens to share a root-target name is never misrouted. Callers clear - both destinations first so the restore is exact. + that happens to share a root-target name is never misrouted. + + Each destination is cleared immediately before it is copied, so the restore is + an exact replacement even for a backup entry whose target is no longer declared + by the current step files (e.g. after a template sync removed it) — the caller's + clear pass keys off the *currently* declared targets and would otherwise leave + such a directory to be merged into rather than replaced. """ travel_dest = _path(TRAVEL_ASSISTANT_DIR) - try: - travel_dest.mkdir(parents=True, exist_ok=True) - except OSError as exc: - raise AdvanceError(f"Failed to create {travel_dest}: {exc}") from exc - travel_source = backup_dir / BACKUP_AGENT_SUBDIR - if travel_source.is_dir(): - _copy_tree(travel_source, travel_dest) + _replace_path_with(travel_source if travel_source.is_dir() else None, travel_dest) + if not travel_dest.exists(): + try: + travel_dest.mkdir(parents=True, exist_ok=True) + except OSError as exc: + raise AdvanceError(f"Failed to create {travel_dest}: {exc}") from exc root_source = backup_dir / BACKUP_ROOT_SUBDIR if not root_source.is_dir(): @@ -496,15 +567,7 @@ def _restore_from_backup(backup_dir: Path) -> None: except OSError as exc: raise AdvanceError(f"Failed to read backup {root_source}: {exc}") from exc for entry in entries: - destination = _path(entry.name) - if entry.is_dir(): - _copy_tree(entry, destination) - else: - destination.parent.mkdir(parents=True, exist_ok=True) - try: - shutil.copy2(entry, destination) - except OSError as exc: - raise AdvanceError(f"Failed to copy {entry} to {destination}: {exc}") from exc + _replace_path_with(entry, _path(entry.name)) def _validate_step_files_present(target_step: int) -> None: diff --git a/.workshop/scripts/tests/test_advance_step.py b/.workshop/scripts/tests/test_advance_step.py index 28cf8fe..371c602 100644 --- a/.workshop/scripts/tests/test_advance_step.py +++ b/.workshop/scripts/tests/test_advance_step.py @@ -1442,6 +1442,134 @@ def test_back_restores_root_target_that_is_a_file(workshop_repo): assert not (workshop_repo / "travel_assistant" / "toolbox.yaml").exists() +def test_snapshot_publish_failure_preserves_previous_backup(workshop_repo, monkeypatch): + """#1: if the atomic publish swap fails, the prior step backup is rolled back + and no staging/superseded orphans are left behind. + + ``_snapshot_current_step`` renames the old snapshot aside, swaps the new one + into place, and only then deletes the old copy. A failure during the swap must + restore the old snapshot so ``--back`` always has a valid backup to restore. + """ + + _write_state(workshop_repo, 1) + _write_readme(workshop_repo, 1) + _seed_step_backup(workshop_repo, 1, "stub.txt", "old backup") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("current work", encoding="utf-8") + _create_step_files(workshop_repo, 2, "step 2 file") + + real_replace = advance_step.os.replace + + def failing_replace(src, dst, *args, **kwargs): + # Fail only on the publish swap (staging -> final); the rename-aside and + # rollback operate on step-1 / step-1.superseded, which do not match. + if ".staging-" in str(src): + raise OSError("simulated publish failure") + return real_replace(src, dst, *args, **kwargs) + + monkeypatch.setattr(advance_step.os, "replace", failing_replace) + + assert advance_step.main(["--expected", "1"]) != 0 + + backups = _backups_dir(workshop_repo) + # The previous backup is intact (rolled back), not lost or half-written. + assert (backups / "step-1" / "travel_assistant" / "stub.txt").read_text( + encoding="utf-8" + ) == "old backup" + # No staging or superseded directories leaked. + assert not list(backups.glob("*.staging*")) + assert not list(backups.glob("*.superseded*")) + + +def test_snapshot_always_captures_agent_namespace_for_root_only_work(workshop_repo): + """#2: a root-only snapshot still records travel_assistant/ (even a lone + .gitkeep) so a later --back restores the placeholder instead of an empty dir. + """ + + _write_state(workshop_repo, 1) + _write_readme(workshop_repo, 1) + # Agent workspace is just the placeholder; the real work this step is root-level. + (workshop_repo / "travel_assistant" / ".gitkeep").write_text("", encoding="utf-8") + _create_root_overlay(workshop_repo, 1, "travel_toolbox/tb.yaml", "declared") + (workshop_repo / "travel_toolbox").mkdir() + (workshop_repo / "travel_toolbox" / "tb.yaml").write_text("root work", encoding="utf-8") + _create_step_files(workshop_repo, 2, "step 2 file") + + assert advance_step.main(["--expected", "1"]) == 0 + + # The agent namespace is captured despite holding only the placeholder. + assert ( + _backups_dir(workshop_repo) / "step-1" / "travel_assistant" / ".gitkeep" + ).is_file() + + assert advance_step.main(["--back"]) == 0 + # Restore reproduces travel_assistant/ exactly: placeholder back, step-2 file gone. + assert (workshop_repo / "travel_assistant" / ".gitkeep").is_file() + assert not (workshop_repo / "travel_assistant" / "stub.txt").exists() + + +@pytest.mark.parametrize("relpath", [".github/workflows/ci.yml", "Makefile", ".GITHUB/ci.yml"]) +def test_root_overlay_rejects_machinery_target_name(workshop_repo, relpath): + """#3: a _root overlay that targets workshop machinery (or a case variant of + it) is rejected before any backup/clear/restore can corrupt the repo.""" + + _create_root_overlay(workshop_repo, 2, relpath, "x") + + with pytest.raises(advance_step.AdvanceError, match="protected repo path"): + advance_step._root_overlay_targets() + + +def test_back_replaces_stale_undeclared_root_target(workshop_repo): + """#4: restore is an exact replacement even for a backup target the current + step files no longer declare (so the caller's clear pass never touched it).""" + + _write_state(workshop_repo, 5) + _write_readme(workshop_repo, 5) + # travel_toolbox is captured in the step-4 backup but NOT declared by any + # current step_files/_root, so _clear_root_targets leaves the live copy alone. + _seed_step_backup(workshop_repo, 4, "stub.txt", "step 4 work") + _seed_step_root_backup(workshop_repo, 4, "travel_toolbox/keep.txt", "kept") + live = workshop_repo / "travel_toolbox" + live.mkdir() + (live / "keep.txt").write_text("stale", encoding="utf-8") + (live / "extra.txt").write_text("should be gone after restore", encoding="utf-8") + (workshop_repo / "travel_assistant" / "stub.txt").write_text("step 5 work", encoding="utf-8") + + assert advance_step.main(["--back"]) == 0 + + assert (live / "keep.txt").read_text(encoding="utf-8") == "kept" + # The stale, undeclared extra file is replaced away, not merged over. + assert not (live / "extra.txt").exists() + + +def test_back_treats_malformed_manifest_as_unrestorable(workshop_repo, capsys): + """#5: a manifest whose step is not an integer is rejected as malformed, so + --back falls back to a canonical rebuild rather than trusting it.""" + + _write_state(workshop_repo, 2) + _write_readme(workshop_repo, 2) + backup = _backups_dir(workshop_repo) / "step-1" + (backup / "travel_assistant").mkdir(parents=True) + (backup / "travel_assistant" / "forged.txt").write_text("do not restore", encoding="utf-8") + (backup / "backup.json").write_text( + json.dumps({"format_version": 2, "step": "oops"}) + "\n", encoding="utf-8" + ) + # Canonical step files for the rebuild fallback. + (workshop_repo / ".workshop" / "step_files" / "00").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "00" / "base0.txt").write_text("b0", encoding="utf-8") + (workshop_repo / ".workshop" / "step_files" / "01").mkdir(parents=True) + (workshop_repo / ".workshop" / "step_files" / "01" / "base1.txt").write_text("b1", encoding="utf-8") + (workshop_repo / "travel_assistant" / "step2_only.py").write_text("# step 2\n", encoding="utf-8") + + result = advance_step.main(["--back"]) + captured = capsys.readouterr() + + assert result == 0 + assert "legacy" in captured.err.lower() + # Canonical rebuild happened; the forged backup content was NOT restored. + assert (workshop_repo / "travel_assistant" / "base1.txt").read_text(encoding="utf-8") == "b1" + assert not (workshop_repo / "travel_assistant" / "forged.txt").exists() + + def test_root_overlay_rejects_protected_target_name(workshop_repo): """A _root overlay that targets a protected repo path is rejected up front."""