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 ed63ba0..499e61d 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 on this step? Re-lay its clean starter files with the [Reset current step](https://github.com/{{OWNER}}/{{REPO}}/actions/workflows/reset-current-step.yml) workflow, or run `python .workshop/scripts/advance_step.py --reset-current --auto-commit` locally — you stay on this step. To start the whole workshop over instead, use [Reset workshop](https://github.com/{{OWNER}}/{{REPO}}/actions/workflows/reset-workshop.yml) or `python .workshop/scripts/advance_step.py --reset --auto-commit`. + +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 360bef1..b6a9974 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. + **Pull the latest workshop machinery (without advancing):** ```bash @@ -303,6 +311,7 @@ uv run python .workshop/scripts/preflight.py ```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 reset-current # re-lay the current step's clean files (auto-commits) make preflight # run environment checks diff --git a/.workshop/scripts/advance_step.py b/.workshop/scripts/advance_step.py index ad174e7..87c59a4 100644 --- a/.workshop/scripts/advance_step.py +++ b/.workshop/scripts/advance_step.py @@ -43,6 +43,21 @@ # 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). 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 @@ -78,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. @@ -214,6 +238,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. @@ -267,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. @@ -285,6 +347,11 @@ def _root_overlay_targets() -> set[str]: if not overlay.is_dir(): continue for entry in overlay.iterdir(): + 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." + ) targets.add(entry.name) return targets @@ -316,21 +383,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: @@ -339,10 +407,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) @@ -351,11 +421,201 @@ 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 + # 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) + ) - _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, 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 + 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") + 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 final.exists(): + 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 + + +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. + + 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) + travel_source = backup_dir / BACKUP_AGENT_SUBDIR + _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(): + 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: + _replace_path_with(entry, _path(entry.name)) + + +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: @@ -587,7 +847,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 @@ -625,11 +886,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) @@ -710,11 +972,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) @@ -866,9 +1129,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)}") @@ -897,6 +1159,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 _plan_relay_current(current_step: int) -> list[str]: """Return human-readable reset-current actions for dry-run output.""" @@ -946,9 +1314,8 @@ def _relay_current(*, dry_run: bool, auto_commit: bool) -> int: print(f"Would auto-commit workshop-owned paths with message '{commit_message}'.") return 0 - timestamp = datetime.now(timezone.utc).strftime("%Y%m%d%H%M%S") - backup_destination = _path(BACKUPS_DIR) / f"reset-current-{current_step:02d}-{timestamp}" - if _backup_travel_assistant(backup_destination, skip_if_empty=False): + backup_destination = _reserve_backup_dir(f"reset-current-{current_step:02d}") + 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)}") @@ -999,6 +1366,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", @@ -1045,6 +1422,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.reset_current: return _relay_current(dry_run=args.dry_run, auto_commit=args.auto_commit) if args.on_push: diff --git a/.workshop/scripts/tests/test_advance_step.py b/.workshop/scripts/tests/test_advance_step.py index 9296f08..371c602 100644 --- a/.workshop/scripts/tests/test_advance_step.py +++ b/.workshop/scripts/tests/test_advance_step.py @@ -123,7 +123,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" @@ -201,7 +201,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): @@ -255,7 +255,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): @@ -272,7 +272,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() @@ -297,7 +297,7 @@ def test_reset_current_relays_current_step_and_backs_up(workshop_repo, monkeypat # Learner edits backed up, then replaced by the clean starter; stray file gone. backups = list((workshop_repo / ".workshop_instance" / "workshop_backups").glob("reset-current-05-*")) assert len(backups) == 1 - assert (backups[0] / "stub.txt").read_text(encoding="utf-8") == "my edits" + assert (backups[0] / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "my edits" assert (workshop_repo / "travel_assistant" / "stub.txt").read_text(encoding="utf-8") == "step 5 starter" assert not (workshop_repo / "travel_assistant" / "scratch.txt").exists() assert github_env.read_text(encoding="utf-8") == "NEW_STEP=5\n" @@ -809,7 +809,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" @@ -828,7 +828,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): @@ -1044,6 +1044,564 @@ def test_on_push_missing_state_after_init_fails_loudly(workshop_repo, monkeypatc 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_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.""" + + _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}" + + # --- machinery-only push classification (advance-on-push guard #4) ----------- diff --git a/Makefile b/Makefile index b719b86..afb3bfb 100644 --- a/Makefile +++ b/Makefile @@ -9,11 +9,12 @@ PYTHON ?= python -.PHONY: help advance reset reset-current preflight sync-template +.PHONY: help advance back reset reset-current preflight sync-template 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 reset-current Re-lay the CURRENT step's clean starter files and" @echo " re-render its README, then auto-commit (backs up" @@ -26,6 +27,7 @@ help: @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/advance_step.py --reset-current --auto-commit" @echo " $(PYTHON) .workshop/scripts/preflight.py" @@ -34,6 +36,9 @@ help: 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 ```