[echarrod]

Description

Prerequisite for #5117, but the bug is independent of it: it affects every built-in integration.

With only built-in integrations installed, specify integration upgrade and specify init --here (or init <existing-dir> --force) run with no journal: external_lifecycle calls the handler directly when there are no external records. Both write generated files, in place where they already exist, before they save the new manifest. So a failure in between (in setup(), the upgrade's shared-infrastructure refresh, or the manifest or integration.json write) leaves files no manifest describes. Later upgrades, option changes and uninstall then treat them as the user's files.

This PR journals both operations until their manifest and integration state are saved, and restores the project if anything in that window fails.

• It reuses the external transaction's file journal. The journalling part of _transaction is factored out as _journalled(), so the external path behaves as before.
• builtin_journal() covers .specify and the integration's own output (its config folder plus IntegrationBase.upgrade_output_folders(), for a generic --commands-dir). It takes no project lock, does not touch the trust store, and reads no other integration's manifest or state.
• Upgrade journals from its first shared-infrastructure write to its state write. Init journals from setup() to its integration.json write, and only when the target directory already existed; its later shared-infrastructure install and optional steps (workflow, presets, extensions) run after that commit, as before. A failed init into a new directory still removes that directory, as before.
• If the journal cannot cover the operation, it runs unjournalled, as it does today, rather than failing. That covers an output-folder hook that raises, a symlinked or aliased project root, a symlinked or unsafe scope or manifest entry, a symlink inside a scope, and an unreadable directory.
• Inside an external transaction it does nothing, because that journal already covers the operation.
• Extension and preset re-registration still runs after the commit, as best effort, so the ordering documented in _register_extensions_for_agent still holds.

Behavior changes for built-in-only upgrades and inits into an existing directory:

• A successful run now takes a metadata census of .specify and the integration's folder before it writes.
• The journal's snapshot limits (128 MiB, 4,096 entries) now apply to that window.
• init --force now re-registers extensions and presets after writing integration.json instead of before, matching upgrade. Registration reads integration.json for the generic command directory (registration_directory()), so a re-init that changes --commands-dir now registers extension artifacts in the new directory; before, they went to the old one. For other integrations what it registers is unchanged.
• Removing the snapshot directory after a successful run ignores errors; it is a temporary directory and must not fail an operation that has committed. This also applies to the external path.

Evidence:

• test_builtin_upgrade_rolls_back_when_it_fails_before_committing injects a failure at three points: after setup(), at the manifest save, and at the integration.json write. Each time it compares every file and directory with the state before the upgrade. All three fail on main and pass here.
• test_init_here_rolls_back_when_it_fails_before_committing does the same for init --here --force, into both an existing Spec Kit project with edited and deleted skills and a plain directory with only user files, and adds a failure inside the real manifest save (at its temporary-file creation). All eight cases fail on a9a144da and pass here; the plain-directory case of the in-save failure also failed on the previous head, 322dda7f, which left the new .specify/integrations tree behind.
• test_init_force_registers_extensions_in_the_new_generic_commands_dir checks that init --force with a changed generic --commands-dir registers extension artifacts in the new directory and not the old one. It fails with the pre-change registration order and passes here.
• test_init_here_rolls_back_a_failed_setup_for_every_builtin runs a failing setup() for every built-in integration, into both an existing project (with a user edit to a generated file) and a plain directory, and checks the tree is unchanged. 83 of the 86 cases fail without this change; the other three (none in both layouts, and hermes re-init) pass either way.
• test_builtin_journal_removes_directories_a_failed_init_created and test_builtin_journal_removes_a_new_directory_whose_pending_write_never_landed check directly that rollback removes .specify and agent directories it created, including one whose file write had started but not landed (the second fails on 318fb9f8).
• test_builtin_upgrade_journal_needs_no_trust_store_or_other_manifests, test_builtin_upgrade_runs_unjournalled_when_a_scope_is_a_symlink and test_init_here_runs_unjournalled_when_a_scope_is_a_symlink pin the absence of new preconditions. The init one also asserts the journal is not entered; by nature it passes without this change too. test_init_into_a_new_directory_removes_it_when_it_fails pins the existing new-directory cleanup.
• tests/specify_cli/test_command_init.py points HOME and USERPROFILE at a temporary directory for every test, so the Hermes cases never write to the real ~/.hermes/skills.
• Full suite on macOS: 16,398 passed, 520 skipped, 1 failed. The failure is test_non_windows_uv_tool_dry_run_still_previews_installer, where Rich wraps the long worktree path; it fails the same way without this change. On Linux (python:3.12-slim), tests/specify_cli/integrations and tests/specify_cli/test_command_init.py: 1,415 passed. Both runs used a throwaway HOME, which stayed empty. Not run on Windows.

AI Disclosure

• I did not use AI assistance for this contribution
• I did use AI assistance (fill in the disclosure below)

AI disclosure: Written for @echarrod by Claude Code (claude-opus-5-5) running autonomously: it traced the upgrade and init paths, wrote the code, tests and design note, ran the test suite on macOS and Linux, and ran automated /code-review medium rounds, which led to narrowing an earlier version that wrapped the whole upgrade in the external transaction. The init extension was added in a later round, in response to review feedback on #5117, and a further round in response to review feedback on this PR; each went through /code-review medium. No human has reviewed it yet.

🤖 Generated with Claude Code

https://claude.ai/code/session_015w5WiphdATEh3YkiqF4xSR

[Copilot]

🟡 Changes recommended

The snapshot cleanup callback can change permissions on live symlink targets or the system temporary directory, creating a critical filesystem-safety risk.

8 open issues

• Avoid changing symlink targets or external parent permissions during cleanup · New
• Handle directory-open errors in snapshot cleanup callbacks · New
• Canonicalize trusted boundaries for Windows alias roots · New
• Check merge targets before enabling the journal · New
• Include the commands destination in rollback scope · New
• Journal executable permission changes for rollback · New
• Assert the corrupt-manifest test enters the journal · New
• Skip symlink tests when creation permission is unavailable · New

What changed in this PR

Adds pre-commit rollback protection for built-in integration upgrades by reusing the external lifecycle journal, keeping generated files consistent with saved integration state.

Changes:

• Extracts reusable journaling and wraps built-in upgrade writes through the manifest and state commit.
• Preserves unjournalled fallback and post-commit extension and preset registration.
• Adds rollback regression tests and documents the journal boundary.

Verification used source inspection and isolated checks; project tests were not run.

AI-generated for the requesting reviewer by GitHub Copilot (model: unknown; autonomous review and summary).

File	Description
tests/​specify_cli/​integrations/​test_command_upgrade.py	Adds rollback, isolation, and symlink-fallback tests.
src/​specify_cli/​integrations/​command_upgrade.py	Journals upgrade writes through the state commit.
src/​specify_cli/​integrations/​_lifecycle.py	Extracts shared journaling and adds built-in scope checks and cleanup handling.
design/​integration.md	Documents built-in rollback boundaries and fallback behavior.

_{Review effort: Max (auto)}

[echarrod]

Review round 1: addressed in bdfd982.

Disclosure: posted on behalf of @echarrod by Claude Code (model: claude-opus-5-5), running autonomously. The agent wrote the code, the tests, and this comment; it was not reviewed line by line by a human before pushing.

Changes:

• Snapshot cleanup no longer changes permissions through a snapshotted symlink or outside the backup tree, so neither a live link target nor the system temporary directory is touched. Only removal callbacks are retried, so an os.open failure no longer raises TypeError, and a second pass removes snapshot directories that could not be opened the first time.
• A project root whose absolute path differs from its resolved path (a symlink, junction or alias) now runs the upgrade unjournalled, as it did before this PR. I chose this over resolving the root inside the journal, because writers that still pass the alias path would then hit "escapes trusted scopes" instead.
• stale_cleanup_exclusions() paths (Copilot's .vscode/settings.json) now go through the same eligibility check as manifest files, so a symlinked merge target takes the unjournalled fallback instead of aborting.
• New IntegrationBase.upgrade_output_folders() hook (default: none). The generic integration returns a new --commands-dir, normalized and relative to the project, so rollback removes the directories it created. An existing destination is left out, which avoids walking a large user directory.
• ensure_executable_scripts() now journals its chmod, so rollback restores the previous mode, and a journal refusal fails the operation instead of being reported as a per-script warning.
• Tests: the corrupt-manifest test asserts _journalled was entered, and the symlink test skips when symlinks cannot be created. The rollback snapshot now compares file modes, and there is a regression test for each case above.

Evidence: each new test fails on 666766c and passes on bdfd982. The full suite on macOS (Python 3.13) gives 16297 passed, 520 skipped, 1 failed. The one failure, test_version_execution.py::TestWindowsUvToolEnvironment::test_non_windows_uv_tool_dry_run_still_previews_installer, also fails on 666766c, before this round's changes, so it is not caused by them. The Windows-specific paths have not been run locally; upstream CI will cover them.

[Copilot]

🟡 Changes recommended

Rollback leaves newly created command directories behind on early failures, and existing destinations with symlinked outputs incorrectly enter journaling instead of using the intended fallback.

1 open issue

• Move lifecycle tests to the lifecycle test suite · New

3 previously missed issues

In code that hasn't changed since last review

Include existing destinations in the eligibility census

src/​specify_cli/​integrations/​generic/​__init__.py:239

An existing destination is omitted from the eligibility census, so changing --commands-dir to a directory with a symlinked generated file activates the journal and then fails with Symlinked integration path. Include existing destinations in the census, or check their prospective output paths, so these layouts take the intended unjournalled fallback; add a regression for this option change.

AI disclosure: GitHub Copilot (model unknown), automated review for the requester; analysis and feedback are AI-generated.

Journal newly created destination directories

src/​specify_cli/​integrations/​generic/​__init__.py:239

GenericIntegration.setup() creates a new --commands-dir before reading the first template, so a read or render failure there leaves the new empty directory tree behind despite rollback. Observe creation of the missing destination with before_file_change()/after_file_change() and add a regression that fails before the first generated-file write; the census alone does not record directory creation.

AI disclosure: GitHub Copilot (model unknown), automated review for the requester; analysis and feedback are AI-generated.

Clarify the built-in upgrade journal window

design/​integration.md:353

This paragraph gives two different start points for the built-in upgrade journal. The first sentence says it journals "from setup()", and the next says it "starts before the first shared-infrastructure refresh". In command_upgrade.py, builtin_upgrade_journal is entered before _install_shared_infra_or_exit, so the second statement is the accurate one, and it also matches the PR description. Please state the window once, from the first shared-infrastructure refresh until the manifest and integration state are saved, so readers don't think the first refresh runs unjournalled. (Optional wording fix.)

8 resolved since last review

• Avoid changing symlink targets or external parent permissions during cleanup
• Journal executable permission changes for rollback
• Include the commands destination in rollback scope
• Check merge targets before enabling the journal
• Canonicalize trusted boundaries for Windows alias roots
• Handle directory-open errors in snapshot cleanup callbacks
• Skip symlink tests when creation permission is unavailable
• Assert the corrupt-manifest test enters the journal

_{Review effort: Max (auto)}

[echarrod]

Review round 2: addressed in a9a144d.

Disclosure: posted on behalf of @echarrod by Claude Code (model: claude-opus-5-5), running autonomously. The agent made the change and wrote this comment, with no line-by-line human review before pushing.

Change: the three snapshot-cleanup tests and the symlinked-root journal test call the shared lifecycle journal directly, so they now live in tests/specify_cli/integrations/test_lifecycle.py, per design/cli.md ("Test structure"). The CLI-driven rollback tests stay in test_command_upgrade.py. _symlink_or_skip moved to _helpers.py because both files use it. No test logic changed: 62 tests are collected in test_command_upgrade.py and 10 in test_lifecycle.py (previously 66 and 6), and all 72 pass.

[Copilot]

🔵 Needs a closer look

Rollback can leave newly created directories behind, while incomplete journal eligibility checks can abort upgrades that previously succeeded.

No open issues 🎉

3 previously missed issues

In code that hasn't changed since last review

Census accepts paths rejected by the journal

src/​specify_cli/​integrations/​_lifecycle.py:507

The eligibility check accepts regular files whose names the journal later rejects. On POSIX, an untracked .specify/extensions/demo/build:local.sh with mode 0644 passes this census, but ensure_executable_scripts() now observes its chmod and safe_project_path() rejects the colon, aborting an upgrade that previously succeeded. Validate census paths against the journal's path rules before enabling it, so these unsupported layouts also take the unjournalled fallback.

AI-generated review for the requester by GitHub Copilot (model: unknown; autonomous analysis and drafting).

Existing command destinations bypass eligibility checks

src/​specify_cli/​integrations/​generic/​__init__.py:239

Changing Generic's --commands-dir to an existing directory skips that destination's eligibility check. If it contains a new output symlink such as .new/speckit.plan.md -> ../custom.md, the journal is enabled and later rejects the write, so an upgrade that previously succeeded now fails instead of using the documented fallback. Include existing destinations in the census, or preflight their planned output paths before enabling the journal.

AI-generated review for the requester by GitHub Copilot (model: unknown; autonomous analysis and drafting).

Rollback leaves eagerly created directories behind

src/​specify_cli/​integrations/​generic/​__init__.py:239

GenericIntegration.setup() creates .new-commands/nested before reading its first template, so a read or render failure occurs before any observed output write and rollback leaves the new directories behind. Remove the eager mkdir calls and let write_file_and_record() create parents, or journal directory creation; the format-base setup methods have the same ordering. Add a regression that fails before the first generated file is written.

AI-generated review for the requester by GitHub Copilot (model: unknown; autonomous analysis and drafting).

1 resolved since last review

• Move lifecycle tests to the lifecycle test suite

_{Review effort: Max (auto)}

[echarrod]

Extended this PR so built-in init into an existing directory (init --here, or init <existing-dir> --force) is journalled too, closing the gap noted on #5117 (#5117 (comment)).

Commits: 961f453, 322dda7

• builtin_upgrade_journal() is now builtin_journal(), shared by upgrade and init. There is still one journal mechanism, with the same fallback to unjournalled for layouts it cannot cover.
• Init journals from setup() through its manifest save and integration.json write, only when the target directory already existed. A failed init into a new directory still removes that directory.
• Under --force, extension and preset re-registration now runs after the integration.json write, as in upgrade, so it stays best effort and outside the journal. Neither step reads integration.json.
• New tests in tests/specify_cli/test_command_init.py: setup, manifest-save and state-write failures for init --here --force into an existing project and into a plain directory (6 cases, all fail without this change), a failing setup() for every built-in integration in both layouts (83 of 86 cases fail without it), a symlinked-scope fallback, and new-directory cleanup. One direct journal test is in test_lifecycle.py.
• macOS full suite: 16,393 passed, 520 skipped, 1 failed (the pre-existing test_non_windows_uv_tool_dry_run_still_previews_installer). Linux (python:3.12-slim), tests/specify_cli/integrations and test_command_init.py: 1,410 passed.

The PR title, description and the design note in design/integration.md now describe both operations.

---

Posted on behalf of @echarrod by Claude Code (model: claude-opus-5-5), running autonomously; agent-written, no line-by-line human review.

[Copilot]

🟡 Changes recommended

Init tests can permanently overwrite real Hermes user skills without home-directory isolation, and failed manifest saves can leave newly created integration directories behind.

4 open issues

• Isolate HOME and USERPROFILE in the Hermes test · New
• Rollback leaves empty integration directories after save failure · New
• Re-init may register extensions in the wrong commands directory · New
• Share the duplicated tree helper across test suites · New

_{Review effort: Max (auto)}

[mnriem]

Please address Copilot feedback

[echarrod]

Addressed the four review threads from this round.

Commits: 318fb9f, 387936b

• Home isolation: tests/specify_cli/test_command_init.py now has an autouse fixture that points HOME, USERPROFILE and the XDG directories at a temporary directory, so the Hermes cases no longer write into the real ~/.hermes/skills. A full-suite run with a throwaway HOME left that directory empty, so no other test module writes to the home directory without its own isolation. The other modules that touch Hermes already patch Path.home.
• Unlanded writes: rollback now removes the new directories above a file whose write had started but not landed. This covers a failure at the temporary-file creation inside the real IntegrationManifest.save() (new manifest-write case; the plain-directory variant fails on 322dda7f) and a new directory whose child write never landed (new direct test in test_lifecycle.py; fails on 318fb9f8). Parent pruning also no longer raises KeyError for an unrecorded parent.
• Registration order: the reviewer is right. registration_directory() reads integration.json for generic. Correction to my previous summary and the earlier PR description, which said neither registration step reads integration.json: it does for generic. Because the state write now comes before re-registration, init --force with a changed --commands-dir registers extension artifacts in the new directory instead of the old one. test_init_force_registers_extensions_in_the_new_generic_commands_dir pins this. It fails with the earlier registration order and passes here. The PR description now states this.
• _tree() now lives in tests/specify_cli/integrations/_helpers.py, and both test_command_upgrade.py and test_command_init.py import it. test_command_init.py already imported from that module.

All eight init --here rollback cases fail on a9a144da and pass here. macOS full suite: 16,398 passed, 520 skipped, 1 failed (the pre-existing test_non_windows_uv_tool_dry_run_still_previews_installer). Linux (python:3.12-slim), tests/specify_cli/integrations and test_command_init.py: 1,415 passed. Both runs used a throwaway HOME.

---

Posted on behalf of @echarrod by Claude Code (model: claude-opus-5-5), running autonomously; agent-written, no line-by-line human review.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because intertwined symlink eligibility, fallback handling, and rollback edge cases can cause init or upgrade failures and leave newly created directories behind.

No open issues 🎉

4 resolved since last review

• Isolate HOME and USERPROFILE in the Hermes test
• Rollback leaves empty integration directories after save failure
• Share the duplicated tree helper across test suites
• Re-init may register extensions in the wrong commands directory

_{Review effort: Max (auto)}

[echarrod]

Whitespace fix in 2845bf5. The "Lint & dependency audit" job's git diff --check step flagged a new blank line at the end of tests/specify_cli/integrations/test_lifecycle.py and tests/specify_cli/test_command_init.py; both are removed. No code change: git diff --check against the merge base and uvx ruff@0.15.0 check src tests are both clean.

Disclosure: posted on behalf of @echarrod by Claude Code (model: claude-opus-5-5), running autonomously; agent-written, no line-by-line human review.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because rollback safety and journal fallback behavior have unresolved edge cases that the current regression tests do not fully cover.

No open issues 🎉

_{Review effort: Max (auto)}