Skip to content

feat(worktree): add durable conversation workspace core - #94079

Open
mrkillbob wants to merge 11 commits into
NousResearch:mainfrom
mrkillbob:codex/conversation-worktree-core-split-20260824
Open

feat(worktree): add durable conversation workspace core#94079
mrkillbob wants to merge 11 commits into
NousResearch:mainfrom
mrkillbob:codex/conversation-worktree-core-split-20260824

Conversation

@mrkillbob

Copy link
Copy Markdown

Summary\n- resolve fail-closed conversation-worktree policy and precedence\n- persist durable root-session/worktree bindings\n- create and bootstrap manager-owned worktrees with bounded lifecycle handling\n- keep this PR limited to policy, state, and manager foundations\n\nThis is the independently reviewable core extracted from #93620. CLI, gateway, desktop, cleanup, and GC integration remain in #93620 until this core lands; they can then be rebased into a separate integration PR without duplicating this diff.\n\n## Verification\n- policy/state/config focused suites passed\n- manager suite: 16 passed\n- targeted Ruff: passed\n- git diff --check: clean\n\nThe separate process-probe suite still has three macOS live-system-guard diagnostics when run under the canonical wrapper; the conversation manager suite itself passes with the required bounded process-tree cleanup.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 90e320edb78c38e72ca2c853905c6c3c6f492713 against exact/current main 74ad422d50e6d76e26ba5ce3b4d2a4d520e923dd. This is nine commits directly atop current main, 11 changed files, and there were no prior submitted reviews on this PR at review time.

The split from #93620 is the right publication shape. I checked the policy precedence/fail-closed validation, deterministic root-session path/branch identity, immutable SQLite binding contract, task/delegated bypass, repository/root lock separation, bootstrap process-tree timeout handling, and the real thread/process concurrency tests. The core is substantially more reviewable than the umbrella. I found two ownership/concurrency blockers before this should become the foundation the later CLI/gateway/Desktop integration rebases onto.

Blocker 1 — same-repository is not the same thing as an unowned output location

_validate_worktree_root_ownership() rejects a root owned by a different Git common dir, but explicitly accepts any nearest ancestor with the same common dir. The only containment check before it is _is_within(path, source), which protects the configured source_worktree specifically. That leaves every sibling linked worktree of the same repository admissible as worktree_root.

This is not theoretical Git behavior. I reproduced the exact topology with stock Git: create a sibling linked worktree, then run git worktree add -b nested <sibling>/nested HEAD. Git accepts it, registers the nested worktree, and the parent sibling immediately reports ?? nested/ in git status --porcelain. In this manager, the same shape passes the common-dir check, so a conversation workspace can be physically created inside another live session/task worktree and contaminate the mutable tree it is supposed to isolate from. That is the other side of #46303's shared-worktree clobber class.

There is a second regression in the same ownership class: the manager creates a Hermes-owned worktree but never runs git worktree lock. Merged #48699 already established and live-tested that Hermes-created worktrees need a Git-level lock while in use so another process / git worktree remove --force / pruning cannot delete the tree out from under the session. The manager's hermes-conversation-worktree.lock is an internal cooperative metadata mutex; Git itself does not honor it.

Required shape: derive exclusion from the registered worktree topology, not only common-dir equality. Reject a configured root/target that is inside any registered worktree (source or sibling), while allowing a dedicated external parent for managed worktrees. After creation/identity validation, take the Git worktree lock and keep it until the later explicit cleanup/GC owner proves it may unlock/remove the exact binding. Please add a real-Git regression with a sibling linked worktree and a retention regression proving an active managed tree cannot be externally removed. Preserve #48699 / @JoaoMarcos44 provenance for the already-shipped lock invariant rather than making this look like greenfield work.

Blocker 2 — first binding identity is selected outside the lock that is supposed to make same-root creation idempotent

For an unbound root, bind_new_root_session() currently does:

  1. get_conversation_worktree(root_session_id)None;
  2. read source HEAD into base_commit;
  3. call claim_conversation_worktree(..., base_commit=...);
  4. only after that enter _repository_lock() and re-read the row.

The state layer correctly treats base_commit as immutable identity and raises ConversationWorktreeConflict when a second claim supplies a different base. That means two processes creating the same root can both observe no row, capture different source HEADs if the stable source advances between those reads, and race to claim: one wins; the other receives a raw conflict instead of reusing the first durable identity. The existing same-root thread/process tests hold source HEAD static, so they cannot exercise this window. The claim also occurs before the manager's try/except ConversationWorktreeError, so this escape is not normalized into the manager's failure/event contract.

The repository lock needs to cover absence re-check + base selection + first claim. Inside that lock, re-read the row; only if it is still absent read HEAD and claim; once a row exists, its persisted base/path/branch/common-dir must become the authority for every contender. Add an adversarial two-process/barrier regression that advances source HEAD between the contenders and proves both resolve the same first-claimed binding rather than one throwing an identity conflict.

Topology / merge-order

  • #46303 is the canonical concurrent-session/worktree-isolation defect this core materially addresses; the nested-sibling hole above is the same defect class at the new boundary.
  • Merged #48699 is complementary prior worktree-retention hardening, not superseded history. Its contributor credit needs to survive in this new lifecycle.
  • #80034 is the complementary live-session presence/awareness half; it does not duplicate per-conversation workspace ownership.
  • #93620 is now the integration continuation/umbrella. Once this core is corrected and lands, that PR should be rebased/split so the CLI/gateway/Desktop/cleanup/GC publication consumes this owner rather than carrying a second copy of the core.
  • Closed-unmerged #93337 is the broader predecessor that #93620 narrowed; #93631 (task-worktree trust/cwd), #93778 (delegated read cwd), and #93216 (Desktop workspace/branch projection) are adjacent consumers, not substitutes for this core.

Exact-head hosted state is not red, but it is not evidence yet: CI 32758662916, Docker 32758661368, and Nix 32758661406 are all action_required, and the CI run exposes zero jobs. The author's focused policy/state/config and 16-manager-test evidence is useful, but there is currently no executed hosted exact-head matrix to cite.

Once the two ownership races above are closed, this remains the right layer for the durable creation/binding foundation; cleanup/GC can stay in the follow-on integration as long as the Git lock acquisition is paired here with an explicit later unlock owner.

@alt-glitch alt-glitch added type/feature New feature or request comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 24, 2026
@mrkillbob

Copy link
Copy Markdown
Author

Fixed both blockers in 9a9087bdab.

Ownership and retention:

  • Registered Git worktree topology is now checked under the repository lock. A configured root or unbound target inside any registered source or sibling worktree is rejected before claim, including ready-binding reuse.
  • Newly created or recovered active conversation worktrees receive a Git worktree lock after identity validation. The code preserves the lifecycle provenance from feat(cli): lock hermes worktrees so concurrent processes can't clobber them #48699 / @JoaoMarcos44; the later cleanup owner must explicitly unlock before removal.
  • Real-Git regressions prove a sibling cannot be contaminated, an unbound registered target cannot be adopted, a ready binding cannot bypass topology policy, and ordinary git worktree remove cannot remove an active managed tree.

First-binding identity:

  • Absence re-check, source HEAD selection, and the immutable first claim now occur inside the repository-wide process lock.
  • A persisted binding becomes authoritative for every later contender. A residual SQLite conflict is normalized and re-read instead of escaping raw.
  • The adversarial spawn-process test pauses the first contender after selecting the old HEAD, advances the source, starts a second contender, and proves both return the same first binding.
  • Lock acquisition failure now happens before first claim, so it leaves no half-claimed row.

Fresh post-commit verification:

  • ./scripts/run_tests.sh tests/agent/test_conversation_worktree_manager.py tests/agent/test_conversation_worktree_policy.py tests/state/test_conversation_worktree_bindings.py
  • 42 passed, 0 failed
  • targeted Ruff: clean
  • git diff --check: clean

Standalone mypy remains environment-blocked in this local checkout by missing optional repository dependencies and stubs (238 import/stub errors across 94 files); it did not produce an isolated exact-file receipt.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the exact fix delta 90e320edb78c38e72ca2c853905c6c3c6f492713..9a9087bdab4563e12ca8910f431efa29a4c58136 and the exact current PR head 9a9087bdab4563e12ca8910f431efa29a4c58136.

The first-claim race is closed. Absence re-check, source HEAD selection, and the immutable first claim now occur under the repository-wide process lock, and the new spawn-process regression actually advances the source while the first contender is paused after selecting the old HEAD. The creation-side registered-worktree topology checks also close the sibling/unbound-target cases identified in the first review.

One lifecycle blocker remains before this core is safe for the integration continuation.

Ready-session resolution bypasses the new ownership and retention invariant

bind_new_root_session() now performs the topology check under _repository_lock(), but a ready record returns immediately from _validated_ready_binding() before _ensure_git_worktree_locked() runs. More importantly, the public resolve_existing_session() path still calls _validated_ready_binding() directly, without:

  • _repository_lock();
  • _validate_worktree_root_ownership(); or
  • _ensure_git_worktree_locked().

That is the path the continuation in #93620 uses for existing-entry revalidation, CLI handoff verification, and ordinary resume/switch flows. Consequently, the actual resume path can still accept either:

  1. a durable ready binding whose configured root is inside a sibling registered worktree; or
  2. a valid ready worktree that was externally unlocked after initial creation.

In the second case, identity/ancestry validation succeeds and the session resumes with no Git lifecycle lock, so the retention half of the original blocker is no longer enforced. The new test_ready_binding_is_rejected_when_configured_root_is_inside_sibling() only exercises bind_new_root_session(), not resolve_existing_session(). Likewise, test_active_managed_worktree_is_git_locked_against_external_removal() proves only initial creation plus ordinary git worktree remove; it does not cover ready reuse/recovery or the single---force removal case explicitly cited in the first review.

Required shape:

  • centralize ready-binding resolution under the repository lock: registered-topology validation → ready identity/ancestry validation → verify/reacquire the Git worktree lock → return the binding;
  • use that same path from both the record.state == "ready" branch in bind_new_root_session() and resolve_existing_session();
  • add a real-Git regression proving resolve_existing_session() rejects a ready binding rooted inside a sibling worktree;
  • add a regression that unlocks a normal ready binding, then proves both bind-reuse and resolve-reuse re-lock it (or fail closed) before returning; and
  • exercise git worktree remove --force once in the retention regression. A double --force is Git's explicit administrative override and is not the invariant being claimed.

Hosted exact-head evidence is also still absent: CI 32763248117, Docker 32763247101, and Nix 32763247156 are action_required, and the CI run exposes zero jobs. Current main is now cd297653fa4fac85f45f7d3ad8e361db0f14e9be, eight commits ahead of merge base 74ad422d50e6d76e26ba5ce3b4d2a4d520e923dd; after the resolver/retention fix, this needs a rebase and an executed exact-head hosted matrix.

So: Blocker 2 is resolved, and the creation half of Blocker 1 is resolved. The ready/recovery half of Blocker 1 is not yet closed.

@mrkillbob
mrkillbob force-pushed the codex/conversation-worktree-core-split-20260824 branch from 9a9087b to 0f6cc46 Compare August 24, 2026 20:40
@mrkillbob

Copy link
Copy Markdown
Author

Fixed the remaining ready/recovery lifecycle blocker and rebased the complete series onto current main.

New exact head: 0f6cc46
Current base: f14059f
Pre-rebase recovery tag: pre-review-ready-rebase-20260824-491e3f4b7a

Ready resolution now uses one repository-locked path from both bind_new_root_session and resolve_existing_session:

  1. validate registered-worktree topology;
  2. validate ready identity and ancestry;
  3. verify or reacquire the Git worktree lock;
  4. return the binding.

New real-Git regressions prove:

  • resolve_existing_session rejects a ready binding rooted inside a sibling registered worktree;
  • bind reuse reacquires an externally removed Git lock before returning;
  • resolve reuse reacquires an externally removed Git lock before returning; and
  • a single git worktree remove --force cannot remove the active locked tree.

Red evidence at 9a9087bdab:

  • resolve topology test: DID NOT RAISE;
  • bind and resolve relock tests: both returned with the worktree still unlocked.

Fresh post-rebase verification at 0f6cc46:

  • conversation-worktree manager, policy, and state suites: 45 passed, 0 failed;
  • targeted Ruff: clean;
  • git diff origin/main...HEAD --check: clean;
  • range-diff preserves the ownership/claim fix and ready-session retention fix across the rebase.

Hosted CI, Docker, and Nix still require repository workflow authorization; no local product-code failure is being represented as hosted evidence.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 0f6cc463109b09a878db98d09512f063b4533b0e, including the final fix delta from rebased parent 286096c3964ed295ba5a32de8d37606b0868b8e3 and the full current 11-file series.

The remaining ready/recovery lifecycle blocker is closed.

  • bind_new_root_session() now handles an already-ready record inside _repository_lock() and routes it through _resolve_ready_binding_locked() instead of returning directly from _validated_ready_binding().
  • resolve_existing_session() now resolves source/configured identity, takes the same repository lock, re-reads the durable row, and uses that same centralized ready path.
  • The centralized order is correct: registered-worktree topology validation → durable identity/branch/base ancestry validation → Git worktree lock verification or reacquisition → return.
  • The new real-Git regressions cover the previously missing surfaces: resolve rejects a ready binding nested in a sibling registered worktree; both bind reuse and resolve reuse reacquire an externally removed Git lock; and one git worktree remove --force still cannot remove the active locked tree.

I also rechecked the earlier blocker fix as preserved in rebased commit 286096c3964ed295ba5a32de8d37606b0868b8e3: absence re-check, source HEAD selection, and first immutable claim remain under the repository-wide process lock; sibling/unbound-target exclusion remains derived from registered worktree topology; active trees remain Git-locked with the #48699 / @JoaoMarcos44 lifecycle provenance intact; and the adversarial advancing-HEAD process test remains present. I do not see a regression from the rebase or a new code blocker in the final delta.

main advanced after the author's rebase from f14059fad20e17acf2512785114791566e70bd06 to b106a09b9731331430c675a39d0044531903dc46 through #94219. That merged gateway-replay work touches seven disjoint gateway/Desktop files, none of this PR's 11 files, and GitHub currently reports the PR mergeable. I do not see a semantic merge-order conflict from that post-rebase advance. #93620 remains the integration continuation and should consume this owner after landing rather than retaining a duplicate core.

The exact-head hosted runs are still authorization-gated rather than executed: CI 32775278283, Nix 32775276729, and Docker 32775276836 are action_required, with the CI run exposing zero jobs. Code-review result: the prior blockers are closed at this head; an authorized, executed hosted matrix remains the final repository merge gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants