mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix: use missing_ok for temp file cleanup to avoid masking errors (#3803)
* fix(agent-context): recurse for nested plans in Python mtime fallback 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 (#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 #3733 * test(agent-context): cover symlink containment in the mtime fallback 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. * fix: use missing_ok for temp file cleanup to avoid masking errors
This commit is contained in:
@@ -11,8 +11,8 @@ Usage: update_agent_context.py [plan_path]
|
||||
|
||||
When ``plan_path`` is omitted, the script derives it from
|
||||
``.specify/feature.json`` (written by /speckit-specify). Falls back to the most
|
||||
recently modified ``plan.md`` anywhere under ``specs/`` (including nested scoped
|
||||
layouts such as ``specs/<scope>/<feature>/plan.md``) only when feature.json is
|
||||
recently modified ``plan.md`` found anywhere under ``specs/`` — scoped layouts
|
||||
nest it as ``specs/<scope>/<feature>/plan.md`` — only when feature.json is
|
||||
absent or its plan does not exist yet.
|
||||
"""
|
||||
|
||||
@@ -173,16 +173,31 @@ def _resolve_plan_path(project_root: str) -> str:
|
||||
|
||||
if not plan_path:
|
||||
root = Path(project_root).resolve()
|
||||
plans = sorted(
|
||||
(root / "specs").rglob("plan.md"),
|
||||
key=lambda p: p.stat().st_mtime,
|
||||
reverse=True,
|
||||
)
|
||||
if plans:
|
||||
specs = root / "specs"
|
||||
|
||||
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:
|
||||
plan_path = plans[0].relative_to(root).as_posix()
|
||||
except ValueError:
|
||||
plan_path = ""
|
||||
return p.resolve().relative_to(root)
|
||||
except (OSError, ValueError):
|
||||
return None
|
||||
|
||||
# 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 = []
|
||||
for p in specs.rglob("plan.md"):
|
||||
rel = _resolved_rel(p)
|
||||
if rel is not None:
|
||||
candidates.append((p, rel))
|
||||
candidates.sort(key=lambda pr: pr[0].stat().st_mtime, reverse=True)
|
||||
if candidates:
|
||||
plan_path = candidates[0][1].as_posix()
|
||||
return plan_path
|
||||
|
||||
|
||||
|
||||
@@ -192,8 +192,8 @@ def handle_vscode_settings(sub_item, dest_file, rel_path, verbose=False, tracker
|
||||
|
||||
os.replace(temp_path, target_file)
|
||||
except Exception:
|
||||
if temp_path and temp_path.exists():
|
||||
temp_path.unlink()
|
||||
if temp_path:
|
||||
temp_path.unlink(missing_ok=True)
|
||||
raise
|
||||
|
||||
try:
|
||||
|
||||
@@ -451,8 +451,7 @@ class IntegrationManifest:
|
||||
_ensure_safe_manifest_destination(self.project_root, path)
|
||||
os.replace(temp_path, path)
|
||||
finally:
|
||||
if temp_path.exists():
|
||||
temp_path.unlink()
|
||||
temp_path.unlink(missing_ok=True)
|
||||
return path
|
||||
|
||||
@classmethod
|
||||
|
||||
@@ -278,8 +278,7 @@ def _write_shared_bytes(
|
||||
_ensure_safe_shared_destination(project_path, dest)
|
||||
os.replace(temp_path, dest)
|
||||
finally:
|
||||
if temp_path.exists():
|
||||
temp_path.unlink()
|
||||
temp_path.unlink(missing_ok=True)
|
||||
|
||||
|
||||
_BASH_FORMAT_COMMAND_RE = re.compile(
|
||||
|
||||
@@ -344,14 +344,19 @@ def test_python_mtime_fallback_matching_bash(tmp_path: Path) -> None:
|
||||
|
||||
|
||||
@requires_posix_bash
|
||||
def test_python_mtime_fallback_finds_nested_plan_matching_bash(tmp_path: Path) -> None:
|
||||
# Regression: the mtime fallback must discover plan.md in nested scoped
|
||||
# layouts (specs/<scope>/<feature>/plan.md), matching the Bash/PowerShell
|
||||
# ports and the documented recursive-discovery contract (see #3024). A
|
||||
# one-level scan (specs/*/plan.md) would miss this and omit the plan link.
|
||||
def test_python_mtime_fallback_finds_nested_plan_matching_bash(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""The mtime fallback must recurse into scoped layouts.
|
||||
|
||||
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.
|
||||
"""
|
||||
repo_a, repo_b = twin_projects(tmp_path, context_file="AGENTS.md")
|
||||
for repo in (repo_a, repo_b):
|
||||
plan = repo / "specs" / "scope-a" / "002-nested" / "plan.md"
|
||||
plan = repo / "specs" / "backend" / "001-nested" / "plan.md"
|
||||
plan.parent.mkdir(parents=True, exist_ok=True)
|
||||
plan.write_text("# plan\n", encoding="utf-8")
|
||||
|
||||
@@ -361,7 +366,39 @@ def test_python_mtime_fallback_finds_nested_plan_matching_bash(tmp_path: Path) -
|
||||
assert_parity(bash, py, repo_a, repo_b)
|
||||
content = (repo_b / "AGENTS.md").read_bytes()
|
||||
assert content == (repo_a / "AGENTS.md").read_bytes()
|
||||
assert b"at specs/scope-a/002-nested/plan.md" in content
|
||||
assert b"at specs/backend/001-nested/plan.md" in content
|
||||
|
||||
|
||||
@requires_posix_bash
|
||||
def test_python_mtime_fallback_skips_plan_reached_through_escaping_symlink(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""A plan reached via a specs/ symlink out of the project is not selected.
|
||||
|
||||
``relative_to()`` is lexical, so ``specs/linked/001-x/plan.md`` looks
|
||||
in-project even when ``specs/linked`` points outside the tree. Resolving
|
||||
before the containment check rejects it, so the fallback finds nothing and
|
||||
the ``at <plan>`` line is omitted rather than naming an out-of-project file
|
||||
with an in-project-looking path. Mirrors the bash twin's ``_resolved_rel``.
|
||||
"""
|
||||
repo_a, repo_b = twin_projects(tmp_path, context_file="AGENTS.md")
|
||||
for repo in (repo_a, repo_b):
|
||||
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()
|
||||
|
||||
bash = run_bash(repo_a)
|
||||
py = run_python(repo_b)
|
||||
|
||||
assert_parity(bash, py, repo_a, repo_b)
|
||||
content = (repo_b / "AGENTS.md").read_bytes()
|
||||
assert content == (repo_a / "AGENTS.md").read_bytes()
|
||||
assert b"\nat " not in content
|
||||
|
||||
|
||||
@requires_posix_bash
|
||||
@@ -508,6 +545,31 @@ def test_python_fresh_context_file_matches_powershell(tmp_path: Path) -> None:
|
||||
assert (repo_a / "AGENTS.md").read_bytes() == (repo_b / "AGENTS.md").read_bytes()
|
||||
|
||||
|
||||
@pytest.mark.skipif(not POWERSHELL, reason="no PowerShell available")
|
||||
def test_python_mtime_fallback_finds_nested_plan_matches_powershell(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Python's mtime fallback must recurse like the PowerShell twin.
|
||||
|
||||
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).
|
||||
"""
|
||||
repo_a = make_project(tmp_path / "proj-ps", context_file="AGENTS.md")
|
||||
repo_b = make_project(tmp_path / "proj-py", context_file="AGENTS.md")
|
||||
for repo in (repo_a, repo_b):
|
||||
plan = repo / "specs" / "backend" / "001-nested" / "plan.md"
|
||||
plan.parent.mkdir(parents=True, exist_ok=True)
|
||||
plan.write_text("# plan\n", encoding="utf-8")
|
||||
|
||||
ps = run_powershell(repo_a)
|
||||
py = run_python(repo_b)
|
||||
|
||||
assert ps.returncode == py.returncode == 0, ps.stderr + py.stderr
|
||||
assert (repo_a / "AGENTS.md").read_bytes() == (repo_b / "AGENTS.md").read_bytes()
|
||||
assert b"at specs/backend/001-nested/plan.md" in (repo_b / "AGENTS.md").read_bytes()
|
||||
|
||||
|
||||
@pytest.mark.skipif(not POWERSHELL, reason="no PowerShell available")
|
||||
def test_python_upsert_matches_powershell(tmp_path: Path) -> None:
|
||||
repo_a = make_project(tmp_path / "proj-ps", context_file="AGENTS.md")
|
||||
|
||||
Reference in New Issue
Block a user