fix: enforce FileHelper policy for shared infrastructure by mnriem · Pull Request #4951 · github/spec-kit

mnriem · 2026-10-13T17:58:28Z

Description

Migrate project-side filesystem operations owned by install_shared_infra() and refresh_shared_templates() to explicit, root-scoped FileHelper operations, including their shared manifest reads, hashes, recording and saves.

Denied symlink destinations previously warned and silently skipped installation, even with force. They now fail explicitly. Allowed contained targets retain link identity and existing missing-only, managed/unmodified refresh, customization/recovery and force-overwrite decisions. Stale leaf links remain preserved customizations.

The bounded migration adds policy-aware metadata access and explicit per-operation mutation observers, optional helper-backed manifest I/O, and helper handoff from init and integration install/upgrade/switch. Init uses the current requested flag before persistence; other consumers read current project policy. Bootstrap config hierarchy remains deny-only.

Lifecycle changes validate exactly the shared scripts/templates directory scopes and managed .specify/.gitignore file scope through the selected helper before nofollow census. File rollback journals actual contained targets; separate directory bookkeeping removes only identified newly created empty directories, without claiming their contents. Installer/journal containment checks, archive verification, locking, extension/preset registration and integration uninstall remain unchanged. External adapter transactions still reject linked .specify/integrations scope roots.

The baseline includes persisted-policy merge 117fb577fde50d4b787748d24877743367c25aa5. This PR changes 17 files: 11 source, five test files and design/file-helper.md; it does not change template or script content.

Testing

Tested locally with uv run specify --help
Ran existing tests with uv sync && uv run pytest
Tested with a sample project (if applicable)

The existing suite was run with the worktree-owned virtualenv instead of the checklist's generic pytest invocation. Public adapter and real lifecycle tests exercise temporary projects; no additional manual sample-project run was performed.

Command	Result
`uv sync --extra test --quiet`	Passed; restored the initially missing worktree virtualenv
`uv run specify --help`	Passed
`.venv/bin/python -m pytest tests/specify_cli/test_shared_infra.py -q` before production changes, with the initial six deny regressions	Six failures: `DID NOT RAISE SymlinkDeniedError`, establishing the prior skip-success behavior
`.venv/bin/python -m pytest tests/specify_cli/test_shared_infra.py -k real_lifecycle -q` before lifecycle fixes	Five failed, three passed; exposed linked scope rejection and leaked new target parents
`.venv/bin/python -m pytest tests/specify_cli/test_shared_infra.py tests/test_file_helper.py tests/integrations/test_cli.py tests/test_shared_infra_gitignore.py -q`	383 passed, 61 skipped
`.venv/bin/python -m pytest tests/specify_cli/integrations/test_installed_adapters.py -k 'shared_helper_allowed_links or current_deny_flag' -q`	14 passed
`LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/specify_cli/integrations/test_installed_adapters.py tests/specify_cli/integrations/test_command_switch.py tests/specify_cli/test_shared_infra.py -k 'external_init_package_directories or package_post_copy_failure or rejects_symlinked_parent or shared_helper_allowed_links or current_deny_flag or real_lifecycle or directory_' -q`	112 passed
`LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q`	12,348 passed, 79 skipped, 62 warnings
`.venv/bin/python -m pytest --collect-only -q`	Collection increased from 12,271 to 12,427, with no decrease
`LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/specify_cli/test_shared_infra.py tests/test_file_helper.py tests/specify_cli/integrations/test_installed_adapters.py -q -k 'shared_helper_allowed_links or current_deny_flag or real_lifecycle or directory_ or stat or observer or manifest or stale or allowed_links'`	Independent parent review: 172 passed, eight skipped, 864 deselected
`uvx ruff@0.15.0 check src tests`	All checks passed using the CI-pinned version
`git diff --check`	Passed

Regression coverage includes leaf/parent links, contained/external/dangling/cyclic targets, nofollow metadata, missing/nondirectory parents, callback ordering/failures, overwrite/recovery decisions, manifest formatting and policy errors, stale cleanup, current init flag precedence, and real external-lifecycle rollback restoring bytes/modes and preserving links and unrelated directory contents.

Validation was local macOS. Portable Windows/reparse and newline cases are covered; native Windows filesystem/junction cases were skipped and require CI validation. These checks do not claim race-free filesystem confinement. Earlier unpinned Ruff diagnostics differed from the CI-pinned tool; pinned Ruff passes.

AI Disclosure

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

AI disclosure: GitHub Copilot, powered by GPT-6.1 Sol (gpt-6.1-sol), implemented the code, tests and documentation, ran validation, and authored the commit and PR text on behalf of @mnriem. The agent executed autonomously within user-approved scope and explicit design approval gates, with coordinating-agent technical review; this is not a claim of human line-by-line review. Reasoning effort used the runtime setting and was not explicitly selected in the request.

Route shared infrastructure and scoped manifest operations through explicit project helpers. Preserve allowed link identity and lifecycle rollback without recursively claiming newly created directories. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 084bd222-8ae8-474a-9dcc-6e6c44e031f4

Copilot

🟡 Changes recommended

Template-only refresh can incorrectly fail because helper-backed manifest checks traverse unrelated tracked scripts.

1 open finding

Modification checks incorrectly inspect unrelated tracked scripts · New

What changed in this PR

Migrates shared infrastructure filesystem operations to policy-aware FileHelper access with transactional rollback support.

Changes:

Adds policy-aware metadata and mutation observers.
Routes shared manifests and lifecycle commands through scoped helpers.
Adds extensive symlink, rollback, and policy regression coverage.

File	Description
`design/file-helper.md`	Documents shared-infrastructure policy and observers.
`src/specify_cli/__init__.py`	Passes helpers through shared wrappers.
`src/specify_cli/command_init.py`	Uses the current init symlink flag.
`src/specify_cli/file_helper.py`	Adds scoped stat and mutation observers.
`src/specify_cli/shared_infra.py`	Migrates shared filesystem operations.
`src/specify_cli/integrations/_file_changes.py`	Adds directory observation.
`src/specify_cli/integrations/_helpers.py`	Propagates project policy.
`src/specify_cli/integrations/_lifecycle.py`	Journals helper mutations and directories.
`src/specify_cli/integrations/command_install.py`	Supplies the project helper.
`src/specify_cli/integrations/command_switch.py`	Supplies the project helper.
`src/specify_cli/integrations/command_upgrade.py`	Supplies the project helper.
`src/specify_cli/integrations/manifest.py`	Adds optional helper-backed manifest I/O.
`tests/integrations/test_cli.py`	Updates shared-infrastructure failure expectations.
`tests/specify_cli/integrations/test_command_switch.py`	Verifies denied links abort switching.
`tests/specify_cli/integrations/test_installed_adapters.py`	Covers public lifecycle rollback and policy.
`tests/specify_cli/test_shared_infra.py`	Adds comprehensive shared-infrastructure regressions.
`tests/test_file_helper.py`	Covers stat and observer behavior.

🧠 Review effort: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Restrict manifest modification queries to destinations selected by the refresh caller. Preserve default queries and unrelated tracked entries without expanding FileHelper or lifecycle responsibilities. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 084bd222-8ae8-474a-9dcc-6e6c44e031f4

mnriem · 2026-10-13T18:32:56Z

Addressed review 5507872999 in bfe1311.

The refresh caller now restricts modification checks to the destinations it actually selects. Unrelated tracked scripts and other excluded entries are not inspected; their recorded hashes and ownership/recovery records are preserved. The optional manifest query filter leaves default consumers unchanged. FileHelper remains domain-neutral symlink-aware CRUD; this correction changes neither its implementation nor the lifecycle implementation. Template-specific documentation is on the refresh function, not added to the FileHelper contract.

Regression evidence: the pre-fix test failed because refresh inspected an unrelated tracked script. The new matrix covers seven excluded path categories, deny/allow, default/force, and contained/dangling/external/cyclic links, while selected denied paths still fail explicitly.

Local validation on the published correction:

LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/specify_cli/test_shared_infra.py tests/integrations/test_manifest.py tests/integrations/test_cli.py tests/specify_cli/integrations/test_installed_adapters.py tests/specify_cli/integrations/test_command_switch.py -q: 1,079 passed.
uvx ruff@0.15.0 check src tests: all checks passed.
.venv/bin/python -m pytest --collect-only -q: 12,543 collected, up 116 from the previous PR head.
git diff --check: passed before commit.

CI/CD was not awaited, as explicitly requested; this comment makes no claim about its result. Validation is local macOS, not native Windows.

AI disclosure: posted on behalf of @mnriem by GitHub Copilot using GPT-6.1 Sol (gpt-6.1-sol), autonomous execution within user-approved scope and design gates, with runtime reasoning settings not explicitly selected. The agent authored the bounded code/tests/docs correction, ran local validation, and authored the commit and this summary; no human line-by-line review is claimed.

Copilot

🟡 Changes recommended

Public install/switch coverage and consistent lifecycle design documentation are still needed.

1 open finding

Align lifecycle design with FileHelper shared infrastructure behavior · New

1 resolved since last review

Modification checks incorrectly inspect unrelated tracked scripts

Previously missed (1)

In code that hasn't changed since last review

Add positive install and switch tests for persisted symlink policy

tests/specify_cli/integrations/test_installed_adapters.py:192

This public-lifecycle regression only covers init and upgrade, so the new persisted-policy handoffs in integration install and integration switch remain unverified. A deny-only switch test cannot catch either command accidentally constructing a deny-mode helper or dropping files; add positive public-path cases showing those commands update a contained linked shared target when the project policy allows symlinks, as required by CONTRIBUTING.md:227-240.

🧠 Review effort: Balanced

+The integration lifecycle journals file/package mutations through its existing
+observer and tracks directory creation separately without claiming contents.
+Rollback restores observed target bytes/modes while preserving links, then
+removes only identified empty new directories; unrelated contents survive.
+Shared scope census retains lexical link entries and does not traverse linked
+scope roots or descendants. Other adapter/package/registry/lock restrictions
+are unchanged; operational permission does not waive those boundaries.
+In particular, external integration transactions still reject a linked


Add public install/switch allow and deny cases while preserving existing overwrite behavior. Qualify integration lifecycle documentation for the already-migrated shared infrastructure without changing FileHelper or runtime code. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 084bd222-8ae8-474a-9dcc-6e6c44e031f4

Copilot

🟡 Changes recommended

Stale-path policy failures can occur after shared files have already been overwritten, leaving non-transactional installs partially updated.

2 open findings

🧠 Review effort: Balanced

Check stale-path policy during shared infrastructure planning so static policy failures precede file overwrites. Retain post-write leaf and ownership checks, without changing FileHelper or lifecycle behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 084bd222-8ae8-474a-9dcc-6e6c44e031f4

Copilot

🟡 Changes recommended

Stale manifest validation can permit drive-relative path escape on Windows.

2 open findings

1 resolved since last review

Preflight stale entries before writes to prevent partial updates

🧠 Review effort: Balanced

Reject anchored native path components before constructing stale shared destinations. Add portable Windows parsing regressions and preserve valid POSIX colon filenames without changing FileHelper or lifecycle code. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 084bd222-8ae8-474a-9dcc-6e6c44e031f4

Copilot

🟡 Changes recommended

Observed directory creation can misattribute and later remove a concurrently created directory.

2 open findings

1 resolved since last review

Reject drive-relative stale keys before path construction

🧠 Review effort: Balanced

mnriem · 2026-10-13T19:46:50Z

+            if self._path(path, allow_missing=True) != destination:
+                raise FileHelperError(f"Destination changed during mkdir: {path}")
+            observer(directory, True, False)
+            directory.mkdir(exist_ok=exist_ok if directory == destination else True)


This scenario requires another process to create the same directory between the observation and mkdir. Conflicting simultaneous operations on the same project are explicitly outside this bounded CLI migration's supported assumptions, so we will not add concurrency handling or change mkdir semantics for this finding.

FileHelper remains limited to symlink-aware file/directory CRUD. Existing locks and rollback protections remain unchanged; design/file-helper.md already states that the portable checks do not provide race-free protection.

AI disclosure: reply posted on behalf of @mnriem by GitHub Copilot using GPT-6.1 Sol (gpt-6.1-sol), autonomous execution under user-directed scope, with the runtime reasoning setting not explicitly selected. The agent analyzed the finding and authored this reply; no code changes or human line-by-line review are claimed.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 17:58

Copilot started reviewing on behalf of mnriem October 13, 2026 17:59 View session