Repository navigation
fix: make StepRegistry atomic and raise on corruption - #3922
Open
Quratulain-bilal wants to merge 6 commits into
Open
Quratulain-bilal wants to merge 6 commits into
Quratulain-bilal wants to merge 6 commits into
Conversation
…nt data loss 1. _load() now raises OSError on symlinked paths and corrupted files instead of silently returning empty defaults (matches WorkflowRegistry) 2. save() now writes to a temp file and atomically replaces, preventing data loss from truncated writes on crash
Contributor
There was a problem hiding this comment.
Pull request overview
Makes StepRegistry fail closed on corruption and persist data atomically.
Changes:
- Raises
OSErrorfor unsafe or corrupted registries. - Writes through a temporary file using
os.replace. - Cleans up temporary files after failures.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/catalog.py |
Hardens registry loading and saving. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
mnriem
requested changes
Aug 31, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback and fix test and lint errors
Verifies: - StepRegistry raises OSError on symlinked registry files - StepRegistry raises OSError on corrupted JSON - StepRegistry.save() uses atomic write via mkstemp + os.replace Addresses Copilot review feedback on github#3922.
…mic-save Resolve conflicts by taking main's split of workflows/catalog.py: the StepRegistry code in this branch belongs in workflows/step/catalog/_domain.py, so main's workflows/catalog/_domain.py is taken as-is instead of carrying a duplicate StepRegistry, and tests/test_workflows.py takes main's version with the branch's new tests re-added on top. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
Port the branch's StepRegistry hardening to where main now keeps the class (workflows/step/catalog/_domain.py): _load raises OSError for a symlinked path, an unreadable file, unparseable JSON, or a non-registry shape instead of silently returning an empty registry, and save writes through a temp file plus os.replace so a crash cannot leave a half-written registry. The new constructor errors would otherwise surface as tracebacks, and the step commands used to reach their directory-symlink check only because the registry swallowed the fault. A shared _load_step_registry_or_exit helper now prints a clean error, and add/remove resolve the steps base directory first so the symlinked-directory message keeps its specific wording. Five tests that pinned the silent-reset behaviour are updated to the fail-loud contract, and the branch's three regression tests (symlink, corrupted JSON, atomic save) come along. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
…mic-save Take main's refactor in the conflicted step files: the registry hardening in this branch lives in workflows/step/catalog/_domain.py, while main has moved the CLI-coupled logic into workflows/step/installer.py (resolve_steps_base_dir, StepInstallError, the install transaction) and rewritten workflow_step_remove around _remove_step_locked. tests/test_workflows.py merges with the branch's registry tests kept on top of main's version. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
StepRegistry._load returned an empty registry for a symlinked path, an unreadable file, unparseable JSON, or a non-registry shape, so corruption reached callers as a silently reset registry. It now raises OSError with the registry path in the message; save() already writes atomically on main. Because the constructor now raises, translate the error where it surfaces: installer._load_step_registry maps OSError onto StepInstallError so the existing command handlers keep printing a clean error, and the new _helpers._load_step_registry_or_exit does the same for the step commands. workflow_step_remove resolves the steps base directory before loading the registry so the symlinked-directory message keeps its specific wording. Tests: the three fail-loud regression tests (symlinked file, corrupted JSON, atomic save) are kept, and five tests that pinned the silent-reset behaviour now assert the raised OSError instead. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
Comment on lines
+60
to
+64
| try: | ||
| return StepRegistry(project_root) | ||
| except OSError as exc: | ||
| cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") | ||
| raise cli.typer.Exit(1) from exc |
Comment on lines
+176
to
+179
| try: | ||
| return StepRegistry(project_root) | ||
| except OSError as exc: | ||
| raise StepInstallError(str(exc)) from exc |
Comment on lines
+11449
to
+11462
| def test_save_uses_atomic_write(self, tmp_path): | ||
| from specify_cli.workflows.step.catalog import StepRegistry | ||
|
|
||
| registry = StepRegistry(tmp_path) | ||
| registry.add("test-step", {"type": "command", "config": {}}) | ||
| registry.save() | ||
|
|
||
| registry_path = ( | ||
| tmp_path / ".specify" / "workflows" / "steps" / StepRegistry.REGISTRY_FILE | ||
| ) | ||
| assert registry_path.exists() | ||
| data = json.loads(registry_path.read_text(encoding="utf-8")) | ||
| assert "test-step" in data.get("steps", {}) | ||
| assert not list(registry_path.parent.glob(".*.tmp")) |
| specify_dir = project_root / ".specify" | ||
|
|
||
| # Read installed custom steps from registry only — no dynamic imports | ||
| # Read installed custom steps from registry only �?" no dynamic imports |
Collaborator
|
Please address Copilot feedback |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
StepRegistry._load()silently returned empty defaults on corruption, causing installed steps to appear to vanish.save()wrote directly to the target file risking data loss.Fix
_load()now raisesOSErroron symlinked paths and corrupted files.save()now uses atomic writes via mkstemp + os.replace.