Skip to content

install: _worktree_local_exclude does unguarded filesystem work outside its only try #359

Description

@pbean

Surfaced while fixing the park-path guards in #355. That PR contains the fault for its own caller; the helper itself is still unguarded for everyone else.

The gap

install._worktree_local_exclude describes itself as "Best-effort — skipped if git can't be queried". Its try covers only the git rev-parse --git-common-dir subprocess:

    try:
        common = subprocess.run([...], check=True).stdout.strip()
    except (subprocess.SubprocessError, OSError):
        return
    common_dir = Path(common)
    if not common_dir.is_absolute():
        common_dir = (worktree / common_dir).resolve()   # <-- outside the try
    exclude = common_dir / "info" / "exclude"
    exclude.parent.mkdir(parents=True, exist_ok=True)    # <-- outside the try
    existing = exclude.read_text(encoding="utf-8") if exclude.is_file() else ""
    ...
    exclude.write_text(prefix + "\n".join(new) + "\n", encoding="utf-8")   # <-- outside the try

Everything after the subprocess is unguarded filesystem work: a .resolve(), a mkdir, a read_text, and a write_text. Any of them can raise, and none of them is "git could not be queried" — so a function documented as best-effort propagates on a read-only .git, a permission fault, or an undecodable exclude file.

The .resolve() is the sharpest of the four because it raises a type the callers' except OSError guards do not catch: below Python 3.13 Path.resolve reports a symlink loop as RuntimeError (probed on this box — 3.11/3.12 raise it in default non-strict mode; 3.13 and 3.14 return normally). The repo floor is 3.11 and CI runs 3.11/3.12, so it is live on supported legs. It is reached only on the relative---git-common-dir branch, which is the linked-worktree case.

Callers

  • operatoractions._exclude_from_git (the park index) — its caller engine._index_park was widened to except (OSError, RuntimeError) in feat(cli): bmad-loop confirm completes awaiting-operator stories #355, so that path is contained. The containment is at the call site, not here.
  • worktree provisioning, which seeds its tool files through the same helper.

Fix shape

Widen the helper's own guard to cover its filesystem tail rather than leaving each caller to discover which exception types leak out of a function that says it is best-effort. That keeps the documented contract true at one place instead of N, and matches the observe-degrade / repair-write asymmetry in AGENTS.md — this is bookkeeping, not a repair write, so degrading is correct.

Worth checking whether the worktree-provisioning caller wants the same degrade or would rather hear about it; they may not have the same answer, in which case the return type should say which happened.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:engineOrchestrator engine and run lifecyclebugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions