fix: use missing_ok for temp file cleanup to avoid masking errors - #3803
Open
Quratulain-bilal wants to merge 4 commits into
Open
fix: use missing_ok for temp file cleanup to avoid masking errors#3803Quratulain-bilal wants to merge 4 commits into
Quratulain-bilal wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates temporary-file cleanup and also adds nested plan discovery for agent context.
Changes:
- Uses
Path.unlink(missing_ok=True)for temporary files. - Recursively discovers nested plans with symlink containment checks.
- Adds Bash/PowerShell parity tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/shared_infra.py |
Updates temporary-file cleanup. |
src/specify_cli/integrations/manifest.py |
Updates manifest cleanup. |
src/specify_cli/_utils.py |
Updates JSON-write cleanup. |
extensions/agent-context/scripts/python/update_agent_context.py |
Adds recursive plan discovery. |
tests/extensions/test_update_agent_context_python_parity.py |
Adds nested-plan parity tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Medium
| finally: | ||
| if temp_path.exists(): | ||
| temp_path.unlink() | ||
| temp_path.unlink(missing_ok=True) |
| finally: | ||
| if temp_path.exists(): | ||
| temp_path.unlink() | ||
| temp_path.unlink(missing_ok=True) |
Comment on lines
+182
to
+183
| if temp_path: | ||
| temp_path.unlink(missing_ok=True) |
Comment on lines
+189
to
+193
| # Recurse (rather than the old one-level specs/*/plan.md glob) so scoped | ||
| # layouts created via SPECIFY_FEATURE_DIRECTORY, e.g. | ||
| # specs/<scope>/<feature>/plan.md, are still discovered when | ||
| # feature.json is absent (#3024). Mirrors the bash and PowerShell twins. | ||
| candidates = [] |
Comment on lines
+360
to
+367
| outside = repo.parent / f"outside-{repo.name}" / "001-x" | ||
| outside.mkdir(parents=True, exist_ok=True) | ||
| (outside / "plan.md").write_text("# plan\n", encoding="utf-8") | ||
| specs = repo / "specs" | ||
| specs.mkdir(parents=True, exist_ok=True) | ||
| (specs / "linked").symlink_to(outside.parent, target_is_directory=True) | ||
| # Sanity: the plan really is reachable through the symlink. | ||
| assert (specs / "linked" / "001-x" / "plan.md").is_file() |
The Python port's mtime fallback discovered plans with a one-level specs/*/plan.md glob, so a scoped layout created via SPECIFY_FEATURE_DIRECTORY (specs/<scope>/<feature>/plan.md) was missed when feature.json is absent — the fallback returned no plan and the managed context section omitted the 'at <plan>' line. The bash and PowerShell twins were already fixed to recurse (github#3024); the Python twin was left behind. Switch to specs.rglob('plan.md') with the same symlink-safe containment check the bash twin uses (resolve each candidate and confirm it stays within the project root before ranking by mtime), so a plan reached through a specs/ symlink pointing outside the project is not selected. Adds parity regression tests (vs bash and vs PowerShell) covering a nested specs/<scope>/<feature>/plan.md; both fail on the pre-fix one-level glob. Fixes github#3733
The recursive fallback resolves each candidate before the relative_to() containment check, but nothing exercised that path. Add a parity test for a plan reachable only through a specs/ symlink pointing outside the project: relative_to() is lexical and would accept it, emitting an in-project-looking path for an out-of-project file. Both the bash twin and the Python port skip it, so the "at <plan>" line is omitted. Also correct the module docstring, which still described the fallback as scanning specs/*/plan.md one level deep.
Quratulain-bilal
force-pushed
the
fix/temp-file-unlink-safety
branch
from
July 28, 2026 22:40
117af4b to
868aa89
Compare
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.
Replace if temp_path.exists(): temp_path.unlink() with emp_path.unlink(missing_ok=True) in 3 files: integrations/manifest.py, shared_infra.py, _utils.py. The old pattern could raise OSError if unlink() fails, masking the original exception in inally/except blocks.