From cd04248b020497dee259c09c57395bbe90ff7024 Mon Sep 17 00:00:00 2001 From: Ouroboros Date: Thu, 24 Sep 2026 02:08:30 +0300 Subject: [PATCH] Close the delta-review findings on the snapshot-lock change Three delta reviews of e1727bc25 (Fable, grok-4.7, gpt-6-astra) agreed on the remainder: - The non-UTF-8 fixture name is created only off Windows: that platform decodes names with surrogatepass and raised before any OSError, which the guard did not catch; the fixture's DOS device names (`nul.*`) are renamed. - The three lock-scope sentences and the PR text say what the code does now: the acting self_worktree lane populates and deletes outside the lock (its post-checkout hook no longer fires at provision), only the boot-time prune_orphans sweep and the genesis init still work under it; the binary verdict is one process, two when empty files need their attribute verdict, with the per-file fallback named. - A busy FIRST lock section is a plain refusal: nothing was registered, so nothing is discarded (no second wait, no spurious warning); a failure after the row still discards row, pin and admin dir, now pinned by injections at update-ref and worktree add. - The START_FAILED row carries the producer's detail beside the typed facts; the refusal-fact keys have one owner (delegate_shared.REFUSAL_FACT_KEYS). - One `_deletable` guard for every pre-lock checkout delete; candidate order matches the per-file predicate (PEM head before the size cap). Co-authored-by: Ouroboros <311266734+ouroboros-agent@users.noreply.github.com> --- .../01-high-level-architecture.md | 2 +- docs/architecture/06-agent-core.md | 2 +- docs/development/06-rules-by-change-class.md | 6 ++-- docs/inventories/DATA_LAYOUT_INVENTORY.md | 2 +- ouroboros/delegate_shared.py | 5 +++ ouroboros/subagent_bootstrap.py | 9 ++--- ouroboros/subagent_worktrees.py | 35 ++++++++++++------- ouroboros/tools/delegate.py | 3 +- ouroboros/workspace_patch_capture.py | 12 ++++--- tests/test_reference_book_budgets.py | 4 +-- tests/test_subagent_worktrees_lock_scope.py | 26 +++++++++++++- tests/test_untracked_binary_verdict.py | 25 ++++++------- 12 files changed, 86 insertions(+), 45 deletions(-) diff --git a/docs/architecture/01-high-level-architecture.md b/docs/architecture/01-high-level-architecture.md index cf0f3de74..244d6f72b 100644 --- a/docs/architecture/01-high-level-architecture.md +++ b/docs/architecture/01-high-level-architecture.md @@ -236,7 +236,7 @@ server.py (Starlette+uvicorn) ← HTTP + WebSocket on configurable host:port (de ├── subagent_messages.py ← Bounded durable child-message identity shared by the final frame, recovery, persistence and replay; `executor_observation_meta` validates task-bound progress actor facts ├── subagents.py ← Subagent envelopes + bounded legacy compatibility; `configured_subagent` snapshots dispatch through subagent_runtime ├── subagent_history.py ← Existing compact helper receipt: dated API attempts, session settlement/recovery and typed unrun starts; dynamic context and owner UI only, never admission (§6 Route health) - ├── subagent_worktrees.py ← Worktree lifecycle + durable registry `state/subagent_worktrees.json`; `provision_genesis_project` (never registry/GC); execution snapshots pinned by `refs/ouroboros/delegated/`; standalone payload snapshots; removal only explicit or custody-cross-checked startup GC, fail-closed on an unreadable custody log; for delegated snapshots the ops lock covers only registry/admin-dir/ref writes (issue #1241; the acting checkout/delete still runs under it) (§6 Delegated subagents) + ├── subagent_worktrees.py ← Worktree lifecycle + durable registry `state/subagent_worktrees.json`; `provision_genesis_project` (never registry/GC); execution snapshots pinned by `refs/ouroboros/delegated/`; standalone payload snapshots; removal only explicit or custody-cross-checked startup GC, fail-closed on an unreadable custody log; the ops lock covers only registry/admin-dir/ref/branch writes — checkout, hashing, copy and deletion run outside it (issue #1241; boot-time `prune_orphans` and genesis excepted) (§6 Delegated subagents) ├── artifacts.py ← Attachment staging into `artifact_store/attachments/`; artifact records; scratch fingerprints (`.scratch_manifest.json`) that gate patch exclusion only while content matches; the undeclared-output guard; `delegated_capture_read_target`; partial tool evidence first reads its verified actor source, then a matching redacted observability projection; recovered text uses the existing exact-text source writer best-effort, and a later budget cut without a verified handle remains source-unavailable ├── retention.py ← Unified GC retention SSOT: clamp/age-cutoff + legacy-key seed picker ├── workspace_preflight.py ← Read-only external-workspace git/manifest/toolchain snapshot used by gateway task creation diff --git a/docs/architecture/06-agent-core.md b/docs/architecture/06-agent-core.md index 41abf96a8..2b1cbc8d7 100644 --- a/docs/architecture/06-agent-core.md +++ b/docs/architecture/06-agent-core.md @@ -323,7 +323,7 @@ WHERE a mutating run's changes are destined is the second, separate record — t A payload target gets a standalone private Git snapshot (`subagent_worktrees.provision_payload_snapshot`): the live payload is never initialized as Git, and capture trusts nothing under the child-writable snapshot's `.git`. Disposition (`integrate_payload_patch`) applies a live, index-free `git apply` in no-repository mode under a whole-payload content-hash CAS (drift = typed conflict; identical content = idempotent applied) — `GIT_CEILING_DIRECTORIES` is pinned at the payload's resolved PARENT, because git still searches the ceiling entry itself and an ancestor Git worktree above the runtime data root could otherwise make git skip every hunk at rc=0 — and reserved paths refuse the WHOLE apply as `blocked_reserved_paths` with the candidate preserved. The post-apply outcome set is complete: a live loader hash equal to the recorded RESULT hash is the success; a hash equal to the recorded BASELINE hash with a non-empty touched set is a provable non-mutation that RESOLVES the apply intent (typed `INTEGRATE_APPLY_NO_OP`, the `apply_no_op` arm: no success, no disposition, no reconcile queued, retry lane open); anything else is the ambiguous mismatch, whose intent stays PENDING and whose reconcile marker IS queued because the payload did mutate. A successful apply queues the extension reconcile (`request_extension_reconcile`) and the skill's review goes STALE pending fresh `skill_preflight`/`skill_review`; a run whose ONLY change is a mode flip is already refused at CAPTURE time as `unreviewable_metadata_change`, so a live hash still equal to the baseline means nothing was written. -**A mutating run normally executes in a PRIVATE EXECUTION SNAPSHOT** (metered children keep sharing the tree; their patches integrate through `integrate_subagent_patch` — sha256-bound, 3-way `--index`, protected-path gated, genesis refused, `coop_already_in_tree` a no-op). At `delegate_start` the host snapshots tracked, staged and eligible untracked state, deciding sensitive/credential vetoes BEFORE hashing because `git add -A` would put secrets such as `.env` in the shared object database; git's binary verdict for the whole untracked inventory comes from ONE index-versus-worktree `git diff --numstat` over a scratch index (`workspace_patch_capture.untracked_binary_verdicts`, shared with patch capture), never one process per file. The machine-wide worktree ops lock (`subagent_worktrees._ops_lock`) guards SHARED metadata only — the registry file, a target's `.git/worktrees` and its baseline pin — held twice, briefly (row FIRST, then the ref, then `worktree add --no-checkout`, so a crash after the row is GC-nameable; the acting `self_worktree` checkout/delete is the one path still doing its tree work under it, bounded by the body's own size); listing, classifying, hashing, populating (`reset --hard --no-recurse-submodules`: what `worktree add` runs internally, minus the target's `post-checkout` hook) and copying run outside it (issue #1241: one 67k-file provision held the lock 40 minutes and every other mutating start timed out). A held lock refuses typed (`cause: lock_busy` + the holder's pid/task/op), a SIGKILLed holder is evicted by the owner-aware stale check, every refused snapshot provision is `definitely_unrun` with a durable `START_FAILED` row, and the start receipt discloses `snapshot` size/time. Disclosed residuals: the registry read-modify-write under the lock is O(registered rows, their per-file baseline maps included — moving those maps out of the row is a follow-up), and every eligible untracked text file is hashed into the TARGET's own object database, where it stays unreachable after the snapshot is removed until that repository's own gc. The baseline is pinned by `refs/ouroboros/delegated/` and checked out as a detached worktree; `scope.root` remains the authority target and `execution.workspaceRoot` names the snapshot. The host appends a separate typed binding after the immutable work order: the snapshot is writable and the authority is read-only until integration. Directory-copy runs use the engine-created copy and never the source folder. Full native access has no filesystem sandbox; the binding names the writable root but does not enforce it. Terminal capture records authority drift as evidence: ready-no-changes stays no-change with unknown authorship, while ready-with-changes keeps its private artifact and the locked baseline proof decides integration. Nested Git directories are excluded and disclosed; skill payloads use content-hash CAS. The binding is durable before POST and retry replays it; a GC-collected snapshot is a typed `execution_snapshot_missing` refusal. Worktrees live in `state/subagent_worktrees.json`; removal is explicit or custody-cross-checked startup GC, fail-closed on unreadable custody. The run still uses `live` from the engine view, so the scoped-HOME/`delegated` marker below applies. +**A mutating run normally executes in a PRIVATE EXECUTION SNAPSHOT** (metered children keep sharing the tree; their patches integrate through `integrate_subagent_patch` — sha256-bound, 3-way `--index`, protected-path gated, genesis refused, `coop_already_in_tree` a no-op). At `delegate_start` the host snapshots tracked, staged and eligible untracked state, deciding sensitive/credential vetoes BEFORE hashing because `git add -A` would put secrets such as `.env` in the shared object database; git's binary verdict for the whole untracked inventory comes from one index-versus-worktree `git diff --numstat` over a scratch index (two when empty files need their attribute verdict; `workspace_patch_capture.untracked_binary_verdicts`, shared with patch capture; a failed batch falls back to the per-file verdict with a warning), never one process per file by design. The machine-wide worktree ops lock (`subagent_worktrees._ops_lock`) guards SHARED metadata only — the registry file, a target's `.git/worktrees` and its baseline pin — held twice, briefly (row FIRST, then the ref, then `worktree add --no-checkout`, so a crash after the row is GC-nameable); the acting `self_worktree` lane is split the same way (`worktree add --no-checkout -b` + branch under the lock, `reset --hard --no-recurse-submodules` and deletion outside it — the body's own `post-checkout` hook no longer fires at provision either), and only the boot-time `prune_orphans` sweep and the millisecond genesis `git init` still do their work under it; listing, classifying, hashing, populating (`reset --hard --no-recurse-submodules`: what `worktree add` runs internally, minus the target's `post-checkout` hook) and copying run outside it (issue #1241: one 67k-file provision held the lock 40 minutes and every other mutating start timed out). A held lock refuses typed (`cause: lock_busy` + the holder's pid/task/op), a SIGKILLed holder is evicted by the owner-aware stale check, every refused snapshot provision is `definitely_unrun` with a durable `START_FAILED` row, and the start receipt discloses `snapshot` size/time. Disclosed residuals: the registry read-modify-write under the lock is O(registered rows, their per-file baseline maps included — moving those maps out of the row is a follow-up), and every eligible untracked text file is hashed into the TARGET's own object database, where it stays unreachable after the snapshot is removed until that repository's own gc. The baseline is pinned by `refs/ouroboros/delegated/` and checked out as a detached worktree; `scope.root` remains the authority target and `execution.workspaceRoot` names the snapshot. The host appends a separate typed binding after the immutable work order: the snapshot is writable and the authority is read-only until integration. Directory-copy runs use the engine-created copy and never the source folder. Full native access has no filesystem sandbox; the binding names the writable root but does not enforce it. Terminal capture records authority drift as evidence: ready-no-changes stays no-change with unknown authorship, while ready-with-changes keeps its private artifact and the locked baseline proof decides integration. Nested Git directories are excluded and disclosed; skill payloads use content-hash CAS. The binding is durable before POST and retry replays it; a GC-collected snapshot is a typed `execution_snapshot_missing` refusal. Worktrees live in `state/subagent_worktrees.json`; removal is explicit or custody-cross-checked startup GC, fail-closed on unreadable custody. The run still uses `live` from the engine view, so the scoped-HOME/`delegated` marker below applies. At terminal, `delegate_wait` captures the run's diff against the baseline durably into the task's artifact store; NOTHING reaches the target automatically — the nanny explicitly applies or rejects through `integrate_delegated_patch`. Git and skill captures are whole-result operations: omitted `paths` and an exact empty list select the same captured result, disclosed when explicit; nonempty selectors remain directory-only. Engine-directory empty/subset semantics are unchanged. The staging substrate differs: a GIT workspace target applies under the repo git lock after PROVING no touched path drifted from `baseline_sha` — a plain `git apply` relocates hunks by offset, and the touched-path set is read from `git apply --numstat` in BOTH directions because each direction names only the paths it writes — then applies and STAGES, never commits. A SKILL-PAYLOAD target captures through the payload adapter over a parent-owned trusted index and applies LIVE into the non-Git payload — nothing is staged into any active root and no `.git` or index is created in the payload. The protected-path gate applies only when the target IS the Ouroboros body; a conflict (proven drift) is owned by the still-running nanny, with snapshot and patch persisting until explicit resolution or discard. Mutation rides an apply-intent protocol: a durable `delegate_run_patch_apply_started` row lands before any tree mutation, so on replay a pending intent without a disposition answers typed `INTEGRATE_DELEGATED_APPLY_AMBIGUOUS`, resolved only by explicit `acknowledge_ambiguous=true`, while the provably non-mutating outcomes (a lock error, proven baseline drift, a failed apply, a verified revert, a baseline-equal payload hash) RESOLVE the intent. `patch_verdict.py` is the ONE verdict writer for both pipelines: subjects are minted `run_` by the writer, never prefix-matched by readers, and each decision lands twice — artifact plus typed `delegate_run_patch_verdict` custody row — with a failed artifact write disclosed on the row. `artifacts.delegated_capture_read_target` narrowly rebinds `artifact_store` READS for the owning task's own `delegated_runs/` prefix, and `delegate_shared.orphan_capture_read_target` extends the same read-only, one-directory READ to the terminal-owner ORPHAN the disposition rule authorizes (confirmed by `orphan_disposition_status`), so the actor that may dispose a patch can inspect it without widened write authority. A read-only child stays in Claudexor's default envelope — one transport with one derived difference, not a second pipeline. diff --git a/docs/development/06-rules-by-change-class.md b/docs/development/06-rules-by-change-class.md index 1418a0da0..3748f9174 100644 --- a/docs/development/06-rules-by-change-class.md +++ b/docs/development/06-rules-by-change-class.md @@ -326,9 +326,9 @@ and 23 (`delegated_transport`), both critical. The imperatives: staged-never-committed) is unchanged (`tests/test_delegated_run_isolation_orphans.py`). A copy failure or a source change against the baseline leaves no registered snapshot or pinned - ref; on the delegated snapshot and payload paths no tree walk or per-file git - process runs under the worktree ops lock — the acting `self_worktree` - checkout/delete still does, bounded by the body's own size + ref; no tree walk or per-file git process runs under the worktree ops lock on + the delegated snapshot, payload and acting `self_worktree` paths (boot-time + `prune_orphans` and genesis excepted) (`tests/test_snapshot_file_inputs.py`, `tests/test_subagent_worktrees_lock_scope.py`). - Outcome honesty: a delegating parent must not produce a clean no-tool final answer while direct children run undecided — one bounded absorption diff --git a/docs/inventories/DATA_LAYOUT_INVENTORY.md b/docs/inventories/DATA_LAYOUT_INVENTORY.md index d30bcfde2..d837a259c 100644 --- a/docs/inventories/DATA_LAYOUT_INVENTORY.md +++ b/docs/inventories/DATA_LAYOUT_INVENTORY.md @@ -2,7 +2,7 @@ Machine extraction of the `docs/ARCHITECTURE.md` "Data layout (`~/Ouroboros/`)" tree — the durable-file orientation carrier (this tree's counterpart of the reference PERSISTENCE_OWNERS derivation checklist) — regenerated by `python scripts/regenerate_inventories.py`. Do not edit. Every entry is probed against reality: repo entries must exist as tracked paths; data-plane entries must appear as a literal in the runtime sources that construct them. A durable file renamed or removed in code while its tree row survives = red (`tests/test_generated_inventories.py`). -Source: `docs/architecture/01-high-level-architecture.md`, physical LF lines 596-685; UTF-8 SHA-256 `e2916a21ff25410a7825601414b1a8ee50f4aa8677eb680c628896430749349c`. +Source: `docs/architecture/01-high-level-architecture.md`, physical LF lines 596-685; UTF-8 SHA-256 `4c98e3c3a72a2b85384c0bbcf94dff1d2af14277fa4fac0d81893c768a66deb8`. - entries: **79** (code-ref: 72, repo-dir: 6, repo-path: 1) diff --git a/ouroboros/delegate_shared.py b/ouroboros/delegate_shared.py index 7c0950003..5cf5391c8 100644 --- a/ouroboros/delegate_shared.py +++ b/ouroboros/delegate_shared.py @@ -135,6 +135,11 @@ def _fail(tool: str, code: str, detail: str, **extra: Any) -> ToolResult: return delegate_result(payload) +# The typed facts a refused snapshot provision may carry; the same keys ride the +# refusal payload, the $0 terminal, the availability row and the START_FAILED row. +REFUSAL_FACT_KEYS = ("cause", "holder", "waited_sec", "retryable", "retry_hint") + + def lock_busy_facts(exc: BaseException) -> Dict[str, Any]: """Typed facts when a HELD worktree ops lock refused a snapshot provision (#1241): who holds it and for what (``subagent_worktrees.WorktreeOpsLockBusy``), so the diff --git a/ouroboros/subagent_bootstrap.py b/ouroboros/subagent_bootstrap.py index 2d44ce9a7..0aac96bcd 100644 --- a/ouroboros/subagent_bootstrap.py +++ b/ouroboros/subagent_bootstrap.py @@ -84,14 +84,11 @@ def _startup_refusal_definite(payload: Mapping[str, Any]) -> bool: ) -# The typed facts a refused snapshot provision carries (`delegate_shared.lock_busy_facts`): -# they ride the refusal payload, the $0 terminal and the START_FAILED row unchanged. -_REFUSAL_FACT_KEYS = ("cause", "holder", "waited_sec", "retryable", "retry_hint") - - def refusal_facts(payload: Mapping[str, Any]) -> Dict[str, Any]: """The producer's typed refusal facts present on ``payload`` (never a handle).""" - return {key: payload[key] for key in _REFUSAL_FACT_KEYS if key in payload} + from ouroboros.delegate_shared import REFUSAL_FACT_KEYS + + return {key: payload[key] for key in REFUSAL_FACT_KEYS if key in payload} def _record_startup_refusal( diff --git a/ouroboros/subagent_worktrees.py b/ouroboros/subagent_worktrees.py index fe2edbb57..f52fb0007 100644 --- a/ouroboros/subagent_worktrees.py +++ b/ouroboros/subagent_worktrees.py @@ -267,6 +267,14 @@ def _git(repo_dir: Path, *args: str, check: bool = True, ) +def _deletable(path: Path, root: Path) -> bool: + """A checkout path this module may delete: non-empty, not a root spelling, strictly + inside the worktree root — the same refusal ``_remove_paths`` applies, because the + registry is durable state and a malformed row must never name an arbitrary path.""" + text = str(path).strip() + return bool(text) and text not in (".", "/", "//") and _is_within(path, root) + + def _git_quiet(repo_dir: Path, *args: str) -> None: """Best-effort git: a failing command or a vanished repo is not an error here.""" try: @@ -325,7 +333,7 @@ def _discard_snapshot_checkout(target: Path, wt_path: Path, ref: str, snapshot_i no ``dlg_*`` directory). Best-effort: the startup GC reconciles what a crash leaves. ``lock_wait_sec`` shortens the wait when the failure WAS a busy lock: the same holder is still there, and the typed refusal must not wait twice.""" - if _is_within(wt_path, root) and wt_path.exists(): + if _deletable(wt_path, root) and wt_path.exists(): _force_rmtree(wt_path) try: with _ops_lock(root, op="discard", task_id=task_id, target=str(target), timeout_sec=lock_wait_sec): @@ -373,7 +381,7 @@ def provision_worktree( branch = f"{_BRANCH_PREFIX}{safe_task}" # A stale checkout left by a crashed run is plain files: deleted OUTSIDE the # lock (#1241); its admin dir and branch are replaced under it. - if _is_within(wt_path, root) and wt_path.exists(): + if _deletable(wt_path, root) and wt_path.exists(): _force_rmtree(wt_path) with _ops_lock(root, op="worktree", task_id=str(task_id or ""), target=str(repo_dir)): if base_sha: @@ -412,7 +420,8 @@ def provision_worktree( raise try: # Populate OUTSIDE the lock: what `worktree add` runs internally (no - # submodule recursion, no post-checkout hook of the body). + # submodule recursion; the body's post-checkout hook no longer fires at + # provision — a narrowing, named in ARCHITECTURE §6). _git(wt_path, "reset", "--hard", "--quiet", "--no-recurse-submodules") except Exception: try: @@ -618,7 +627,7 @@ def provision_execution_snapshot( root.mkdir(parents=True, exist_ok=True) # A stale checkout of the SAME snapshot id (a crashed earlier attempt) is plain # files; its admin dir and its pin are replaced under the lock below. - if wt_path.exists(): + if _deletable(wt_path, root) and wt_path.exists(): _force_rmtree(wt_path) head_proc = _git(target, "rev-parse", "--verify", "HEAD", check=False) target_head = head_proc.stdout.strip() if head_proc.returncode == 0 else "" @@ -653,7 +662,8 @@ def provision_execution_snapshot( untracked_baseline: Dict[str, Dict[str, Any]] = {} file_inputs: List[str] = [] capture_warnings: List[Dict[str, Any]] = [] - # git's binary verdict for the whole inventory in ONE process (#1241). + # git's binary verdict for the whole inventory in one process — two when + # empty files need their attribute verdict — never one per file (#1241). binary_verdicts = untracked_binary_verdicts( target, binary_verdict_candidates(target, untracked), warnings=capture_warnings) from ouroboros.workspace_file_outputs import _side @@ -710,6 +720,7 @@ def provision_execution_snapshot( manifest_digest=manifest_digest, target_head=target_head, created_at=time.time(), entry_count=entry_count, excluded_untracked=tuple(excluded), untracked_baseline=untracked_baseline, capture_warnings=tuple(capture_warnings)) + registered = False # nothing to discard until the row exists (a busy first section is a plain refusal) try: with _ops_lock(root, op="provision", task_id=task, target=str(target)): # Row FIRST, then the pin, then the admin dir: a crash after any of these @@ -720,6 +731,7 @@ def provision_execution_snapshot( _git_quiet(target, "worktree", "prune") _register_snapshot(ExecutionSnapshotHandle(**{**fields, "untracked_baseline": {}, "excluded_untracked": ()}), [], data_dir) + registered = True _git(target, "update-ref", baseline_ref, baseline_sha) wt_path.parent.mkdir(parents=True, exist_ok=True) _git(target, "worktree", "add", "--detach", "--no-checkout", str(wt_path), baseline_sha) @@ -757,8 +769,9 @@ def provision_execution_snapshot( with _ops_lock(root, op="provision", task_id=task, target=str(target)): _register_snapshot(handle, excluded, data_dir) except Exception as exc: - _discard_snapshot_checkout(target, wt_path, baseline_ref, snap, root=root, data_dir=data_dir, task_id=task, - lock_wait_sec=5.0 if isinstance(exc, WorktreeOpsLockBusy) else None) + if registered: + _discard_snapshot_checkout(target, wt_path, baseline_ref, snap, root=root, data_dir=data_dir, task_id=task, + lock_wait_sec=5.0 if isinstance(exc, WorktreeOpsLockBusy) else None) raise return handle @@ -1088,10 +1101,8 @@ def remove_execution_snapshot( root = _resolve_root(worktree_root) wt_path = Path(str(entry.get("path") or "")) # The checkout's files go first, OUTSIDE the lock (#1241: a 1.8 GB snapshot - # took minutes to delete and timed every other mutating start out). Only a - # path strictly inside the snapshot root is ever deleted: the registry is - # durable state and a malformed row must never name an arbitrary path. - if str(wt_path).strip() and _is_within(wt_path, root) and wt_path.exists(): + # took minutes to delete and timed every other mutating start out). + if _deletable(wt_path, root) and wt_path.exists(): _force_rmtree(wt_path) with _ops_lock(root, op="remove", task_id=str(entry.get("task_id") or ""), target=str(entry.get("target_root") or "")): @@ -1167,7 +1178,7 @@ def remove_worktree( return True return False wt_path = Path(match.get("path") or "") - if str(wt_path).strip() and _is_within(wt_path, root) and wt_path.exists(): + if _deletable(wt_path, root) and wt_path.exists(): _force_rmtree(wt_path) # the checkout's files go first, OUTSIDE the lock (#1241) with _ops_lock(root, op="remove_worktree", task_id=str(task_id or "")): _remove_paths(Path(match.get("repo_dir") or "."), wt_path, match.get("branch") or "", allowed_root=root) diff --git a/ouroboros/tools/delegate.py b/ouroboros/tools/delegate.py index d10e66907..c3cb7c8bf 100644 --- a/ouroboros/tools/delegate.py +++ b/ouroboros/tools/delegate.py @@ -715,7 +715,8 @@ def _settle_refused_provision(ctx: ToolContext, gateway: Any, refusal: ToolResul _retire_orphaned_registration( ctx, gateway, "", definite_refusal=True, invocation_id=invocation_id, reason=str(payload.get("reason") or "execution_snapshot_failed"), - history_facts={**(history_facts or {}), **refusal_facts(payload)}) + history_facts={**(history_facts or {}), **refusal_facts(payload), + "detail": str(payload.get("detail") or "")}) def _snapshot_facts(handle: Any) -> Dict[str, Any]: diff --git a/ouroboros/workspace_patch_capture.py b/ouroboros/workspace_patch_capture.py index 4fe623733..000413fd0 100644 --- a/ouroboros/workspace_patch_capture.py +++ b/ouroboros/workspace_patch_capture.py @@ -138,9 +138,9 @@ def write_workspace_patch_artifacts( except Exception: scratch_sha_by_rel = {} scratch_sha_by_abs = {} - # git's binary verdict for the whole inventory in ONE process (#1241); the loop - # below keeps its per-file order and reads the verdict instead of spawning - # ``git diff --numstat`` per file. + # git's binary verdict for the whole inventory in one process (two when empty + # files need their attribute verdict; #1241); the loop below keeps its per-file + # order and reads the verdict instead of spawning ``git diff --numstat`` per file. binary_verdicts = untracked_binary_verdicts(root, binary_verdict_candidates(root, untracked), warnings=diagnostics) for rel in untracked: _want_sha = scratch_sha_by_rel.get(rel) or scratch_sha_by_abs.get(os.path.normcase(str((root / rel).resolve(strict=False)))) @@ -724,9 +724,11 @@ def binary_verdict_candidates(root: pathlib.Path, rels: Sequence[str]) -> List[s info = os.lstat(root / rel) except OSError: continue - if not stat.S_ISREG(info.st_mode) or info.st_size > _PATCH_MAX_UNTRACKED_FILE_BYTES: + if not stat.S_ISREG(info.st_mode): continue - if not unrestricted and pem_private_key_reason(root, rel): + if not unrestricted and pem_private_key_reason(root, rel): # PEM before the size cap, as the predicate orders them + continue + if info.st_size > _PATCH_MAX_UNTRACKED_FILE_BYTES: continue out.append(rel) return out diff --git a/tests/test_reference_book_budgets.py b/tests/test_reference_book_budgets.py index e61fda9fe..821a42107 100644 --- a/tests/test_reference_book_budgets.py +++ b/tests/test_reference_book_budgets.py @@ -97,10 +97,10 @@ CHAPTER_BYTE_BUDGETS: dict[str, int] = { # the digest-selected historical read and the reader admission rule. # 295650 -> 297250: document the new diagnostic-only source/coverage contract, # unavailable evidence and no-effects ordering without removing review/custody rules. - # 297250 -> 299000: the private-snapshot paragraph now states the worktree ops + # 297250 -> 299500: the private-snapshot paragraph now states the worktree ops # lock's scope (issue #1241: shared metadata only, row-then-ref order, batched # binary verdict, typed busy refusal) — rationale-layer text BIBLE P6 requires. - "docs/architecture/06-agent-core.md": 299000, + "docs/architecture/06-agent-core.md": 299500, "docs/architecture/07-configuration.md": 36991, # 18947 -> 19287: CI failure collection now documents diagnostic desktop builds while release remains gated. "docs/architecture/08-git-branching-ci-and-build.md": 19287, diff --git a/tests/test_subagent_worktrees_lock_scope.py b/tests/test_subagent_worktrees_lock_scope.py index c2efcde92..a74116e3c 100644 --- a/tests/test_subagent_worktrees_lock_scope.py +++ b/tests/test_subagent_worktrees_lock_scope.py @@ -194,7 +194,7 @@ def test_a_live_holder_is_a_typed_refusal_and_a_dead_holder_is_evicted(tmp_path, lock_path.write_text(f"pid={holder.pid} task=t-dead op=provision since=x target=y", encoding="utf-8") started = time.monotonic() handle = _provision(target, snaps, data) - assert time.monotonic() - started < 10 and pathlib.Path(handle.path).is_dir() + assert time.monotonic() - started < 30 and pathlib.Path(handle.path).is_dir() # far below _LOCK_STALE_SEC=600 assert not _lock_held(snaps) @@ -253,6 +253,8 @@ def test_a_provisioning_refusal_leaves_a_durable_start_failed_row(full_run, monk failed = [row for row in rows if row.get("type") == custody.START_FAILED] assert len(failed) == 1 and failed[0]["definite"] is True and failed[0]["invocation_id"] assert failed[0]["reason"] == "execution_snapshot_failed" and failed[0]["run_id"] == "" + assert failed[0]["cause"] == "lock_busy" and failed[0]["holder"]["task"] == "t-other" # the facts ride the row + assert "t-other" in failed[0]["detail"] # and so does the producer's own sentence def test_configured_child_bootstrap_keeps_the_refusal_facts(tmp_path, monkeypatch): @@ -351,6 +353,28 @@ def test_removal_deletes_files_outside_the_lock_and_forgets_metadata_inside(tmp_ assert not _lock_held(snaps) +@pytest.mark.parametrize("failing", ["update-ref", "worktree"]) +def test_a_failure_inside_the_first_lock_section_after_the_row_leaves_nothing(tmp_path, monkeypatch, failing): + """The provisional row is written first; a failed pin or admin-dir creation right + after it (still inside the lock) must discard the row too, not only later phases.""" + target = _seed_target(tmp_path) + snaps, data = tmp_path / "snaps", tmp_path / "data" + real_git = wt._git + + def failing_git(repo_dir, *args, **kw): + if failing in args and (failing != "worktree" or "add" in args): + raise subprocess.CalledProcessError(128, ["git", *args]) + return real_git(repo_dir, *args, **kw) + + monkeypatch.setattr(wt, "_git", failing_git) + with pytest.raises(subprocess.CalledProcessError): + _provision(target, snaps, data, snapshot_id="snapLockB") + assert wt.find_execution_snapshot("snapLockB", data_dir=data) is None + assert _git(target, "for-each-ref", "refs/ouroboros/").stdout == "" + assert not list(snaps.glob("dlg_*")) and not list((target / ".git" / "worktrees").glob("dlg_*")) + assert not _lock_held(snaps) + + def test_a_failure_after_the_provisional_row_leaves_nothing_and_a_crash_is_gc_reclaimable(tmp_path, monkeypatch): target = _seed_target(tmp_path) snaps, data = tmp_path / "snaps", tmp_path / "data" diff --git a/tests/test_untracked_binary_verdict.py b/tests/test_untracked_binary_verdict.py index 5c7c3f346..795c7c1db 100644 --- a/tests/test_untracked_binary_verdict.py +++ b/tests/test_untracked_binary_verdict.py @@ -39,14 +39,14 @@ def _fixture_repo(root: pathlib.Path) -> tuple[pathlib.Path, list[str]]: files = { "plain.dat": b"hello\n", # -diff attribute: binary whatever the bytes "plain.bin": b"hello\n", # binary macro - "nul.txt": b"x\0y", # diff set: text despite the NUL - "nul.md": b"x\0y", # unspecified: NUL in the first 8000 bytes + "withnul.txt": b"x\0y", # diff set: text despite the NUL + "withnul.md": b"x\0y", # unspecified: NUL in the first 8000 bytes "late.md": b"a" * 9000 + b"\0", # NUL past git's probe: text "edge7999.md": b"a" * 7999 + b"\0", # NUL at offset 7999: binary "edge8000.md": b"a" * 8000 + b"\0", # NUL at offset 8000: text "nonul.drv": b"hello\n", # driver with binary=true and no NUL: binary - "nul.drt": b"x\0y", # driver with binary=false and a NUL: text - "nul.lfs": b"x\0y", # clean filter strips the NUL: text + "withnul.drt": b"x\0y", # driver with binary=false and a NUL: text + "withnul.lfs": b"x\0y", # clean filter strips the NUL: text "text.u16": "hi\n".encode("utf-16"), # working-tree encoding: text "empty.md": b"", "empty.dat": b"", # empty AND -diff: binary by attribute (grok triad finding) @@ -60,11 +60,12 @@ def _fixture_repo(root: pathlib.Path) -> tuple[pathlib.Path, list[str]]: files["new\nline.md"] = b"nl\n" # a newline in the name survives -z for name, data in files.items(): (repo / name).write_bytes(data) - try: # a non-UTF-8 byte round-trips through fsencode; APFS and NTFS refuse such names - (repo / os.fsdecode(b"caf\xe9.md")).write_bytes(b"latin\n") - files[os.fsdecode(b"caf\xe9.md")] = b"latin\n" - except OSError: - pass + if os.name != "nt": # Windows decodes names with surrogatepass and would raise before any OSError + try: # a non-UTF-8 byte round-trips through fsencode; APFS refuses such names, ext4 accepts them + (repo / os.fsdecode(b"caf\xe9.md")).write_bytes(b"latin\n") + files[os.fsdecode(b"caf\xe9.md")] = b"latin\n" + except OSError: + pass os.symlink("target.bin2", repo / "link_to_bin") os.symlink("nowhere", repo / "dangling") rels = sorted(files) + ["link_to_bin", "dangling"] @@ -92,7 +93,7 @@ def test_batch_verdict_matches_git_for_every_path_class(tmp_path): assert verdicts is not None and warnings == [] assert {rel: verdicts.get(rel, False) for rel in rels} == oracle # The classes that a NUL sniff or an attribute lookup alone would get wrong. - assert verdicts["nonul.drv"] and not verdicts["nul.drt"] and not verdicts["nul.lfs"] and not verdicts["text.u16"] + assert verdicts["nonul.drv"] and not verdicts["withnul.drt"] and not verdicts["withnul.lfs"] and not verdicts["text.u16"] assert verdicts["edge7999.md"] and not verdicts["edge8000.md"] assert verdicts["empty.dat"] and verdicts["empty.bin"] and verdicts["empty.drv"] assert not verdicts["empty.drt"] and not verdicts["empty.md"] @@ -149,7 +150,7 @@ def test_vetoed_and_oversized_files_never_reach_git(tmp_path, monkeypatch): assert manifest["status"] == "ready_with_changes", manifest["errors"] assert ".env" not in batched and "big.md" not in batched and "junk.pyc" not in batched - assert "plain.dat" in batched and "nul.md" in batched + assert "plain.dat" in batched and "withnul.md" in batched assert not (repo / "observed-filter-input").exists(), "an excluded file's bytes reached a clean filter" reasons = {row["path"]: row["reason"] for row in manifest["untracked_excluded"]} assert "size cap" in reasons["big.md"] and "junk" in reasons["junk.pyc"] @@ -169,7 +170,7 @@ def test_a_failed_batch_falls_back_to_the_per_file_verdict_with_a_warning(tmp_pa assert capture.untracked_binary_verdicts(repo, rels, warnings=warnings) is None assert warnings and warnings[0]["reason"] == "binary_verdict_batch_unavailable" # ``None`` keeps git's per-file verdict: the same answer, one process per file. - for rel in ("plain.dat", "nul.drt", "late.md"): + for rel in ("plain.dat", "withnul.drt", "late.md"): reason = capture.untracked_capture_veto_reason(repo, rel, binary_verdicts=None) assert (reason == "binary file") == _oracle(repo, rel), rel assert capture.untracked_binary_verdicts(repo, [], warnings=warnings) == {}