Repository navigation
Conversation
Installed-step refresh with network access now snapshots the package and registry entry, copies the backup, and removes the step inside _step_install_transaction, using command_remove._remove_step_locked so the same non-reentrant flock is not taken twice, while the catalog reinstall stays outside the lock. On BundlerError, rollback acquires that lock again, rereads step-registry.json, and restores the backup and the original metadata only when the step id is missing, assigning the saved entry verbatim so installed_at and updated_at stay unchanged. When the id is already present, the later package and the rest of the reread registry are left in place. A failed copy or registry write is attached with add_note and the original install error is re-raised with the backup kept on disk; lock failure before a snapshot is wrapped as BundlerError, offline and not-yet-installed refresh still delegate to install without a backup, and the tests assert those lock, concurrency, and error-preservation paths. Fixes github#4815 Assisted-by: AI
mnriem
added
the
triage-can-wait
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Rollback bypasses registry symlink protections, and new lock-failure branches lack coverage.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Makes bundle step-refresh rollback concurrency-safe while preserving original install errors.
Changes:
- Locks snapshot, removal, and rollback operations.
- Preserves newer commits and adds rollback regression tests.
| File | Description |
|---|---|
src/specify_cli/bundles/primitives.py |
Implements transactional refresh rollback. |
tests/specify_cli/bundles/test_primitives.py |
Adds concurrency and failure-path coverage. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except step_installer.StepInstallError as exc: | ||
| # Lock acquisition failed before any package or registry snapshot. | ||
| raise BundlerError( | ||
| f"Failed to refresh step '{component.id}': {exc}" | ||
| ) from exc |
|
Please address Copilot feedback and fix test & lint errors |
Rollback read step-registry.json directly, which bypassed StepRegistry's symlink checks. It now reloads through StepRegistry._load and resolves the steps base with resolve_steps_base_dir, _resolve_step_dir and _reject_unsafe_destination before removing or copying the package, so a steps directory or step directory swapped for a symlink during the unlocked reinstall is refused and the backup is kept. Adds tests for the initial lock failure (wrapped, no backup), a failed rollback lock (original error kept with a note and the backup), and both symlink swaps. Assisted-by: Grok Build (model: unknown) and Claude Code (model: Claude Opus 5.5), autonomous Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wSaUMZrtm6XSzwEcxavkv
…p-refresh-rollback # Conflicts: # tests/specify_cli/bundles/test_primitives.py
|
@mnriem addressed both Copilot comments in 772a8be, and merged main to clear the test_primitives.py conflict (both sides' new tests kept):
About test_catalog_versions.py::test_exact_add_uses_historical_url_digest_and_requirements: this PR doesn't touch it, and the same test fails on main too (the Oct 5 15:56 UTC Test & Lint Python run on main, windows-latest 3.13, and the fix/ps-probe-python3 PR run an hour later). I think _archive() is the cause: writestr stamps the zip entry with the current time, and the test builds the catalog digest and the served archive separately, so if the CLI call takes more than 2 seconds the digests differ. I left it alone since it is outside this PR. Happy to send a separate fix that pins the ZipInfo date_time if you want one. Lint is clean with ruff check src tests. The full pytest suite ran green before the merge, and test_primitives.py again after it. Disclosure: posted on behalf of @mvanhorn. The change was written by the Grok coding agent (model unknown, autonomous) and reviewed and verified by Claude Code (Claude Opus 5.5, autonomous). |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The locked path still relies on a stale pre-lock installation snapshot, causing refresh to fail after a concurrent removal.
Review effort: Balanced
Findings: 1
Open (2)
Resolved since last review (1)
| with step_installer._step_install_transaction(self._root): | ||
| registry = StepRegistry(self._root) | ||
| entry = registry.get(component.id) | ||
| metadata = copy.deepcopy(entry) if entry is not None else None |
|
Please address Copilot feedback |
Description
Installed-step refresh with network access now snapshots the package and registry entry, copies the backup, and removes the step inside _step_install_transaction, using command_remove._remove_step_locked so the same non-reentrant flock is not taken twice, while the catalog reinstall stays outside the lock. On BundlerError, rollback acquires that lock again, rereads step-registry.json, and restores the backup and the original metadata only when the step id is missing, assigning the saved entry verbatim so installed_at and updated_at stay unchanged. When the id is already present, the later package and the rest of the reread registry are left in place. A failed copy or registry write is attached with add_note and the original install error is re-raised with the backup kept on disk; lock failure before a snapshot is wrapped as BundlerError, offline and not-yet-installed refresh still delegate to install without a backup, and the tests assert those lock, concurrency, and error-preservation paths.
Refreshing an installed custom step and then failing the reinstall could replace a newer copy of that step and its registry entry with the backup taken at the start of the refresh, and it could drop other step entries committed in between. If copying the backup back or writing the registry failed, that secondary error is what the caller saw, and the temporary backup was deleted. The package and metadata were snapshotted and the step directory was backed up before removal, outside the lock shared with step add and step remove, and the failure path then copied that backup onto the step directory with dirs_exist_ok. StepRegistry.save() replaces the entire registry file from the in-memory document loaded in the constructor, so a snapshot taken before a concurrent update overwrites entries committed later. An exception from the copy or the save propagated in place of the original BundlerError, and the cleanup that followed always removed the backup directory.
Fixes #4815
Testing
uv run specify --helpNot claimed: ran
uv run python -m pytest tests/test_agent_config_consistency.py -qlocally; it fails the same way on the base branch, so the failure predates this change (it fails the same way on main).uv sync && uv run pytestNot claimed:
uv sync && uv run pytestwas not run locally either.Not verified: this needs a person on the named hardware or environment.
Ran
uv run python -m pytest tests/test_agent_config_consistency.py -qlocally; it fails the same way on the base branch, so the failure predates this change (it fails the same way on main). Tests for this live intests/specify_cli/bundles/test_primitives.py.AI Disclosure
AI disclosure
AI was used for assistance.