fix: use missing_ok for temp file cleanup to avoid masking errors

[Quratulain-bilal]

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.

[Copilot]

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

[Copilot]

Review details

Comments suppressed due to low confidence (8)

src/specify_cli/shared_infra.py:265

• missing_ok=True suppresses only FileNotFoundError; another cleanup failure (for example, PermissionError) will still replace the exception raised by the write/replace operation. Since this finally block is intended not to mask that failure, suppress OSError around the best-effort cleanup.

        temp_path.unlink(missing_ok=True)

src/specify_cli/integrations/manifest.py:442

• missing_ok=True handles a vanished temporary file, but it still lets every other OSError from cleanup mask the exception that entered this finally block. Make removal best-effort by suppressing OSError so callers receive the original manifest-save failure.

            temp_path.unlink(missing_ok=True)

src/specify_cli/_utils.py:183

• If unlinking fails for any reason other than the file being absent, this cleanup still replaces the original exception despite the PR's stated goal. Catch OSError from this best-effort removal before re-raising the original failure.

            if temp_path:
                temp_path.unlink(missing_ok=True)

tests/extensions/test_update_agent_context_python_parity.py:393

• This fixture symlinks a directory, but Path.rglob() does not recurse through directory symlinks by default. The test therefore passes even without _resolved_rel() and does not cover the newly added containment check; symlink the discovered plan.md file itself to the outside file instead.

        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()

tests/extensions/test_update_agent_context_python_parity.py:556

• The removed Python code already used recursive rglob("plan.md"), so the claim that this PR fixes a one-level Python scan is inaccurate. Reword this as an ongoing parity check rather than attributing behavior to the old implementation.

    With no feature.json, discovery falls back to scanning under specs/. A plan
    at specs/<scope>/<feature>/plan.md sits more than one level deep; the old
    Python one-level glob missed it while PowerShell already recurses (#3024).

extensions/agent-context/scripts/python/update_agent_context.py:186

• This resolved-path filtering and its parity tests are outside the PR's stated temporary-file-cleanup scope. Please either move the agent-context behavior change to a dedicated PR or update the PR title/description to explain and justify the additional behavior.

        def _resolved_rel(p: Path) -> Path | None:
            # Resolve symlinks before checking containment: relative_to() is
            # lexical and would otherwise accept a plan reached through a specs/
            # symlink that points outside the project, emitting an
            # in-project-looking path for an out-of-project file (or picking it
            # as "most recent").
            try:
                return p.resolve().relative_to(root)
            except (OSError, ValueError):

tests/extensions/test_update_agent_context_python_parity.py:355

• This describes a one-level Python glob, but the removed implementation already used (root / "specs").rglob("plan.md"). Please avoid presenting this as a regression fixed by this PR; the implementation change here adds resolved-path containment filtering instead.

This issue also appears on line 554 of the same file.

    A plan created under specs/<scope>/<feature>/plan.md (as produced via
    SPECIFY_FEATURE_DIRECTORY) is more than one level below specs/. The old
    Python port used a one-level specs/*/plan.md glob and missed it, while the
    bash/PowerShell twins recurse (#3024). This locks in the parity.

src/specify_cli/commands/bundle/init.py:883

• The PR description limits the change to temporary-file cleanup in three files, but this adds a new 50 MiB behavior limit to remote bundle downloads. Either split this functional/security change into its own PR with focused bundle-level coverage, or update the title and description so reviewers and release notes capture it.

            raw = read_response_limited(resp, max_bytes=MAX_DOWNLOAD_BYTES, error_type=BundlerError)

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Medium

[Copilot]

Review details

Suppressed comments (4)

extensions/agent-context/scripts/python/update_agent_context.py:182

• The PR title and description scope this change to temporary-file cleanup in three CLI files, but this hunk independently changes agent-context plan selection and symlink-containment behavior, with substantial parity tests added elsewhere. This makes the submitted behavior materially broader than described; split the agent-context work into its own PR or update the PR metadata and rationale so reviewers can assess that behavior explicitly.

        def _resolved_rel(p: Path) -> Path | None:
            # Resolve symlinks before checking containment: relative_to() is
            # lexical and would otherwise accept a plan reached through a specs/
            # symlink that points outside the project, emitting an
            # in-project-looking path for an out-of-project file (or picking it

src/specify_cli/shared_infra.py:265

• missing_ok=True only suppresses FileNotFoundError; any other cleanup OSError (for example, a permission or I/O failure) will still replace the exception raised by the write/validation path. To meet the PR's stated goal of preserving the original failure, make this best-effort cleanup suppress OSError.

        temp_path.unlink(missing_ok=True)

src/specify_cli/integrations/manifest.py:454

• missing_ok=True handles only the race where the path is already absent. Other OSError subclasses from unlink() can still mask the exception from manifest writing or destination validation, so cleanup needs to be best-effort if the original error must remain visible.

            temp_path.unlink(missing_ok=True)

src/specify_cli/_utils.py:196

• This exception handler still risks replacing the original exception: missing_ok=True suppresses only FileNotFoundError, not permission, filesystem, or other OSError failures from cleanup. Suppress cleanup OSError here before re-raising the original exception.

            if temp_path:
                temp_path.unlink(missing_ok=True)

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced