From 34fcde021640301c7f237644dc21426090473d90 Mon Sep 17 00:00:00 2001 From: sprooty Date: Thu, 15 Oct 2026 23:17:12 +0000 Subject: [PATCH 1/3] session.start: open in an existing worktree via cwd (WI-1671) worktree=true cannot be used when the engine cannot fetch the origin, and there was no way to name a worktree that already exists. cwd must exist, sit under the workspace Active/ directory, and be a git worktree of the project's repository; combining it with worktree=true is refused. --- docs/ENGINE.md | 8 +++ src/vogt/application/models.py | 17 ++++- src/vogt/application/services/sessions.py | 75 ++++++++++++++++++++++- src/vogt/registry/operations.py | 5 +- tests/test_sessions.py | 74 ++++++++++++++++++++++ 5 files changed, 176 insertions(+), 3 deletions(-) diff --git a/docs/ENGINE.md b/docs/ENGINE.md index f651bcc2a..f5c424927 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -854,6 +854,14 @@ than these routes. Absent means the session opens in `cwd`, byte-identical to a request from before the field existed. A wake returns to the worktree already cut and does not cut another. + `session.start`'s `cwd` (WI-1671) is the other way in: the caller names an + existing worktree and vogt-core sends it as this `cwd`, with no + `SessionSpec.worktree`. It must be an absolute path that exists, sit under + the workspace `Active/` directory, and be a git worktree of the same + repository as the project's registered checkout (the same git `commondir`). + Combining it with `worktree: true` is refused before the engine is asked, + because one start cannot both cut a checkout and open one that already + exists. - `POST /api/sessions/:id/conversation` `{"agent", "id", "ended"?}` -> `SessionSummary` (WI-962). Links the agent conversation running in the session, or with `ended` unlinks it while it is still the session's. See diff --git a/src/vogt/application/models.py b/src/vogt/application/models.py index 774d4f0cf..8d09aa7a1 100644 --- a/src/vogt/application/models.py +++ b/src/vogt/application/models.py @@ -4226,7 +4226,22 @@ class StartSessionParams(Params): "disturb it. Requires a project, or a work item that belongs to " "one. Unset, the purpose's dispatch profile decides (WI-1323), and " "with no purpose the session opens in the project's checkout, as " - "`false` does. Recorded on the audit row." + "`false` does. Cannot be combined with `cwd` (WI-1671). Recorded " + "on the audit row." + ), + ) + cwd: str | None = Field( + default=None, + description=( + "Open the session in this existing directory instead of the " + "project's registered checkout (WI-1671). Use it when a worktree " + "already exists and `worktree=true` cannot cut a new one — a " + "private origin the engine cannot fetch, for example. The path " + "must exist, must sit under the workspace's `Active/` directory, " + "and must be a git worktree of the same repository as the " + "project's checkout (the same `commondir`). Anything else is " + "refused, and so is combining it with `worktree=true`. Recorded " + "as the session's cwd." ), ) reason: Reason = Field(description="Why this write is being made (audited).") diff --git a/src/vogt/application/services/sessions.py b/src/vogt/application/services/sessions.py index d7bc1c17f..eb4343d94 100644 --- a/src/vogt/application/services/sessions.py +++ b/src/vogt/application/services/sessions.py @@ -124,6 +124,8 @@ from vogt.application.services.preferences import dispatch_settings from vogt.application.services.views import why from vogt.application.writes import WriteOutcome, audited_action, audited_write +from vogt.collectors.base import CollectorError +from vogt.collectors.git_local import git_output from vogt.core import delivery, oversight, runtime from vogt.core.auth import Scope, hash_token, issue, parse_scopes from vogt.core.branches import default_branch_name @@ -252,6 +254,14 @@ def start_session(ctx: AppContext, params: StartSessionParams) -> SessionResult: # role nor a permission posture, so naming a purpose can never raise a # start past what the caller could ask for directly (WI-1323). params, overrides, read_only = _resolve_purpose(ctx, params) + # Named before the engine is asked, so a bad path never starts a session + # (WI-1671). A purpose profile can set `worktree`; it cannot set `cwd`. + if params.cwd and params.worktree: + msg = ( + "cwd and worktree cannot be combined: worktree cuts a new " + "checkout, cwd opens one that already exists" + ) + raise InvalidRequest(msg) # After the profile fills the model, before anything reaches the engine: # a project's review policy is a rule, and a start that would run a model # it forbids is refused rather than spawned (WI-1590). @@ -259,6 +269,8 @@ def start_session(ctx: AppContext, params: StartSessionParams) -> SessionResult: started_by_session, started_by_kind = _started_by(ctx, engine) session_id = ctx.id_factory("ses") subject = _subject(ctx, params, session_id) + if params.cwd: + subject.cwd = _existing_worktree_cwd(params.cwd, subject.cwd) actor_ref = f"agent:session:{session_id}" session_scopes = _session_scopes(ctx) credential = issue(session_scopes) @@ -333,7 +345,7 @@ def start_session(ctx: AppContext, params: StartSessionParams) -> SessionResult: # at the registry's root, which is what the engine reports back anyway. cwd = ( started.cwd - if started.cwd and (params.resume or params.worktree) + if started.cwd and (params.resume or params.worktree or params.cwd) else subject.cwd ) @@ -3094,6 +3106,67 @@ def default_name(self) -> str: return self.work_item_ref or self.project_slug +def _existing_worktree_cwd(raw: str, project_root: str) -> str: + """The directory a `cwd` start opens in, or a refusal (WI-1671). + + Three checks, in the order a wrong path fails them: it exists, it sits + under the workspace's `Active/` directory, and it is a git worktree of + the same repository as the project's registered checkout. The comparison + is git's `commondir`, which a linked worktree and the main checkout of + one repository share and two repositories do not. + """ + path = Path(raw).expanduser() + if not path.is_absolute(): + msg = f"cwd {raw!r} must be an absolute path" + raise InvalidRequest(msg) + if not path.is_dir(): + msg = f"cwd {raw!r} does not exist" + raise InvalidRequest(msg) + try: + resolved = path.resolve(strict=True) + except OSError as exc: + msg = f"cwd {raw!r} cannot be resolved: {exc}" + raise InvalidRequest(msg) from exc + parts = resolved.parts + if "Active" not in parts: + msg = f"cwd {resolved} is not under the workspace Active/ directory" + raise InvalidRequest(msg) + checkout = Path(project_root) + if not checkout.is_dir(): + msg = ( + f"the project's checkout {project_root!r} does not exist, so " + "cwd cannot be checked against it" + ) + raise InvalidRequest(msg) + try: + asked = git_output(resolved, "rev-parse", "--git-common-dir", required=True) + owned = git_output(checkout, "rev-parse", "--git-common-dir", required=True) + except CollectorError as exc: + msg = ( + f"cwd {resolved} is not a git worktree of the project's " + f"repository ({project_root}): {exc}" + ) + raise InvalidRequest(msg) from exc + asked_dir = Path(asked) + owned_dir = Path(owned) + if not asked_dir.is_absolute(): + asked_dir = resolved / asked_dir + if not owned_dir.is_absolute(): + owned_dir = checkout / owned_dir + try: + same = asked_dir.resolve() == owned_dir.resolve() + except OSError as exc: + msg = f"cwd {raw!r} is not a git worktree of the project's repository: {exc}" + raise InvalidRequest(msg) from exc + if not same: + msg = ( + f"cwd {resolved} is not a git worktree of the project's " + f"repository ({project_root})" + ) + raise InvalidRequest(msg) + return str(resolved) + + def _worktree_branch(subject: _Subject, session_id: str) -> str: """The branch a session's own worktree is cut onto (WI-1298). diff --git a/src/vogt/registry/operations.py b/src/vogt/registry/operations.py index b694a56d2..ee7bc5782 100644 --- a/src/vogt/registry/operations.py +++ b/src/vogt/registry/operations.py @@ -1019,7 +1019,10 @@ def build_operations() -> list[Operation[Any, Any]]: "Open a coding session (a terminal) for a work item or a " "project. With `template` (e.g. claude) it runs that agent, " "and a `task` is delivered as the agent's first prompt, so it " - "starts working instead of opening idle. Returns both ids " + "starts working instead of opening idle. `cwd` opens it in an " + "existing worktree under the workspace Active/ directory " + "instead of the project's checkout; it cannot be combined " + "with `worktree=true`. Returns both ids " "(ses_… and engine_session_id); then poll session_screen " "until `ready`." ), diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 5d91debd3..1acb46916 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -518,6 +518,80 @@ def test_a_worktree_session_is_cut_from_the_integration_branch( assert "worktree" not in engine.last_spec +def test_a_cwd_opens_in_an_existing_worktree( + wired: AppContext, + engine: StandInEngine, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """WI-1671: name a worktree that already exists and start in it. + + The engine is handed that directory as cwd and is not asked to cut one. + A path that is not under Active/, is not a worktree of the project's + repository, or is combined with worktree=true is refused before any + session is started. + """ + checkout = tmp_path / "vogt" + checkout.mkdir() + active = tmp_path / "Active" / "vogt-wi-7" + active.mkdir(parents=True) + stranger = tmp_path / "Active" / "other" + stranger.mkdir() + + def fake_git(root: Path, *args: str, required: bool = False) -> str: + if args[:2] == ("rev-parse", "--git-common-dir"): + if root == active.resolve() or root == checkout.resolve(): + return str(tmp_path / "common.git") + return str(tmp_path / "other.git") + return "" + + monkeypatch.setattr("vogt.application.services.sessions.git_output", fake_git) + # The fixture project points at ROOT, which does not exist. Register one + # whose checkout is the fake repository so the commondir check has a + # tree to compare against. + register_project( + wired, + RegisterProjectParams(name="Checkout", root_path=str(checkout), reason=WHY), + ) + + result = start_session( + wired, + StartSessionParams(project="checkout", cwd=str(active), reason=WHY), + ) + assert engine.last_spec["cwd"] == str(active.resolve()) + assert "worktree" not in engine.last_spec + assert result.session.cwd == str(active.resolve()) + + with pytest.raises(InvalidRequest, match="Active"): + start_session( + wired, + StartSessionParams(project="checkout", cwd=str(checkout), reason=WHY), + ) + with pytest.raises(InvalidRequest, match="not a git worktree"): + start_session( + wired, + StartSessionParams(project="checkout", cwd=str(stranger), reason=WHY), + ) + with pytest.raises(InvalidRequest, match="does not exist"): + start_session( + wired, + StartSessionParams( + project="checkout", + cwd=str(tmp_path / "Active" / "missing"), + reason=WHY, + ), + ) + with pytest.raises(InvalidRequest, match="cannot be combined"): + start_session( + wired, + StartSessionParams( + project="checkout", cwd=str(active), worktree=True, reason=WHY + ), + ) + # None of the refusals reached the engine. + assert engine.counter == 1 + + def test_the_whole_session_clock_is_unset_unless_the_project_sets_it( wired: AppContext, engine: StandInEngine ) -> None: From 04627969ede4d30c6f0792d1fbb62136e7a59d76 Mon Sep 17 00:00:00 2001 From: sprooty Date: Thu, 15 Oct 2026 23:30:43 +0000 Subject: [PATCH 2/3] session.start cwd: keep purpose profiles and anchor Active/ (WI-1671) A purpose whose profile sets worktree (implement) no longer refuses a cwd: only an explicit worktree=true does, and the profile's worktree is left out. The Active/ check is the parent of the project's checkout, the shared checkout itself is refused, and the commondir probe runs git with repo config, hooks and fsmonitor disabled. --- docs/ENGINE.md | 14 ++-- src/vogt/application/models.py | 12 +-- src/vogt/application/services/sessions.py | 84 ++++++++++++++++---- tests/test_sessions.py | 96 ++++++++++++++++------- 4 files changed, 152 insertions(+), 54 deletions(-) diff --git a/docs/ENGINE.md b/docs/ENGINE.md index f5c424927..c339344c4 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -857,11 +857,15 @@ than these routes. `session.start`'s `cwd` (WI-1671) is the other way in: the caller names an existing worktree and vogt-core sends it as this `cwd`, with no `SessionSpec.worktree`. It must be an absolute path that exists, sit under - the workspace `Active/` directory, and be a git worktree of the same - repository as the project's registered checkout (the same git `commondir`). - Combining it with `worktree: true` is refused before the engine is asked, - because one start cannot both cut a checkout and open one that already - exists. + the workspace `Active/` directory (the parent of the project's registered + checkout, not any path component named `Active`), and be a git worktree of + the same repository as that checkout (the same git `commondir`). The + checkout itself is refused: a cwd start is how a worker stays off the + shared tree. A symlink is accepted as the directory it resolves to. + Combining it with an explicit `worktree: true` is refused before the + engine is asked. A purpose whose profile would have set `worktree` (the + `implement` profile does) does not: `cwd` means the checkout already + exists, so the profile's worktree is left out. - `POST /api/sessions/:id/conversation` `{"agent", "id", "ended"?}` -> `SessionSummary` (WI-962). Links the agent conversation running in the session, or with `ended` unlinks it while it is still the session's. See diff --git a/src/vogt/application/models.py b/src/vogt/application/models.py index 8d09aa7a1..f1a361ace 100644 --- a/src/vogt/application/models.py +++ b/src/vogt/application/models.py @@ -4237,11 +4237,13 @@ class StartSessionParams(Params): "project's registered checkout (WI-1671). Use it when a worktree " "already exists and `worktree=true` cannot cut a new one — a " "private origin the engine cannot fetch, for example. The path " - "must exist, must sit under the workspace's `Active/` directory, " - "and must be a git worktree of the same repository as the " - "project's checkout (the same `commondir`). Anything else is " - "refused, and so is combining it with `worktree=true`. Recorded " - "as the session's cwd." + "must exist, must sit under the workspace's `Active/` directory " + "(the parent of the project's checkout), must not be that " + "checkout itself, and must be a git worktree of the same " + "repository (the same `commondir`). Anything else is refused, " + "and so is combining it with an explicit `worktree=true`. A " + "purpose profile that would have set `worktree` does not, when " + "`cwd` is given. Recorded as the session's cwd." ), ) reason: Reason = Field(description="Why this write is being made (audited).") diff --git a/src/vogt/application/services/sessions.py b/src/vogt/application/services/sessions.py index eb4343d94..4217938fe 100644 --- a/src/vogt/application/services/sessions.py +++ b/src/vogt/application/services/sessions.py @@ -26,7 +26,9 @@ from __future__ import annotations +import os import re +import subprocess import time from datetime import datetime, timedelta from pathlib import Path @@ -125,7 +127,6 @@ from vogt.application.services.views import why from vogt.application.writes import WriteOutcome, audited_action, audited_write from vogt.collectors.base import CollectorError -from vogt.collectors.git_local import git_output from vogt.core import delivery, oversight, runtime from vogt.core.auth import Scope, hash_token, issue, parse_scopes from vogt.core.branches import default_branch_name @@ -253,15 +254,18 @@ def start_session(ctx: AppContext, params: StartSessionParams) -> SessionResult: # Resolved only after both guards above, and a profile carries neither a # role nor a permission posture, so naming a purpose can never raise a # start past what the caller could ask for directly (WI-1323). - params, overrides, read_only = _resolve_purpose(ctx, params) - # Named before the engine is asked, so a bad path never starts a session - # (WI-1671). A purpose profile can set `worktree`; it cannot set `cwd`. - if params.cwd and params.worktree: + # Before the profile fills unset fields (WI-1671). Only a worktree the + # caller asked for conflicts with `cwd`: the implement profile sets + # `worktree` itself, and that is the case `cwd` exists for. + if params.cwd and "worktree" in params.model_fields_set and params.worktree: msg = ( "cwd and worktree cannot be combined: worktree cuts a new " "checkout, cwd opens one that already exists" ) raise InvalidRequest(msg) + params, overrides, read_only = _resolve_purpose(ctx, params) + if params.cwd and params.worktree: + params = params.model_copy(update={"worktree": None}) # After the profile fills the model, before anything reaches the engine: # a project's review policy is a rule, and a start that would run a model # it forbids is refused rather than spawned (WI-1590). @@ -3106,14 +3110,55 @@ def default_name(self) -> str: return self.work_item_ref or self.project_slug +def _git_common_dir(root: Path) -> str: + """`git rev-parse --git-common-dir` with config that cannot run hooks. + + The directory is one the caller named, so the probe must not honour a + repo's fsmonitor, hooks, or system gitconfig. `-c` overrides the repo + config; `GIT_CONFIG_NOSYSTEM` drops the system one. + """ + + env = os.environ.copy() + env["GIT_CONFIG_NOSYSTEM"] = "1" + env["GIT_CONFIG_COUNT"] = "0" + try: + completed = subprocess.run( # fixed argv, never a shell + [ + "git", + "-C", + str(root), + "-c", + "core.fsmonitor=", + "-c", + "core.hooksPath=/dev/null", + "rev-parse", + "--git-common-dir", + ], + capture_output=True, + text=True, + timeout=20, + check=False, + env=env, + ) + except (OSError, subprocess.SubprocessError) as exc: + msg = f"git could not be run against {root}: {exc}" + raise CollectorError(msg) from exc + if completed.returncode != 0: + detail = completed.stderr.strip() or f"exit {completed.returncode}" + msg = f"git rev-parse could not be read in {root}: {detail}" + raise CollectorError(msg) + return completed.stdout.strip() + + def _existing_worktree_cwd(raw: str, project_root: str) -> str: """The directory a `cwd` start opens in, or a refusal (WI-1671). - Three checks, in the order a wrong path fails them: it exists, it sits - under the workspace's `Active/` directory, and it is a git worktree of - the same repository as the project's registered checkout. The comparison - is git's `commondir`, which a linked worktree and the main checkout of - one repository share and two repositories do not. + Four checks, in the order a wrong path fails them: it exists, it sits + under the workspace's real `Active/` directory (the parent of the + project's registered checkout, not any path component named `Active`), + it is not that checkout itself, and it is a git worktree of the same + repository. The comparison is git's `commondir`, which a linked worktree + and the main checkout of one repository share and two repositories do not. """ path = Path(raw).expanduser() if not path.is_absolute(): @@ -3127,10 +3172,6 @@ def _existing_worktree_cwd(raw: str, project_root: str) -> str: except OSError as exc: msg = f"cwd {raw!r} cannot be resolved: {exc}" raise InvalidRequest(msg) from exc - parts = resolved.parts - if "Active" not in parts: - msg = f"cwd {resolved} is not under the workspace Active/ directory" - raise InvalidRequest(msg) checkout = Path(project_root) if not checkout.is_dir(): msg = ( @@ -3138,9 +3179,20 @@ def _existing_worktree_cwd(raw: str, project_root: str) -> str: "cwd cannot be checked against it" ) raise InvalidRequest(msg) + checkout_resolved = checkout.resolve() + active = checkout_resolved.parent + if active.name != "Active" or not resolved.is_relative_to(active): + msg = f"cwd {resolved} is not under the workspace Active/ directory ({active})" + raise InvalidRequest(msg) + if resolved == checkout_resolved: + msg = ( + f"cwd {resolved} is the project's shared checkout; name a " + "worktree of it instead" + ) + raise InvalidRequest(msg) try: - asked = git_output(resolved, "rev-parse", "--git-common-dir", required=True) - owned = git_output(checkout, "rev-parse", "--git-common-dir", required=True) + asked = _git_common_dir(resolved) + owned = _git_common_dir(checkout_resolved) except CollectorError as exc: msg = ( f"cwd {resolved} is not a git worktree of the project's " diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 1acb46916..9796b64be 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -527,28 +527,34 @@ def test_a_cwd_opens_in_an_existing_worktree( """WI-1671: name a worktree that already exists and start in it. The engine is handed that directory as cwd and is not asked to cut one. - A path that is not under Active/, is not a worktree of the project's - repository, or is combined with worktree=true is refused before any - session is started. + The Active/ check is the parent of the project's checkout, so a directory + that merely has an `Active` component elsewhere is refused, and so is the + shared checkout itself. A relative path, a stranger repository, a missing + directory, and an explicit worktree=true are refused before any session + is started. A symlink is accepted as the directory it points at. """ - checkout = tmp_path / "vogt" - checkout.mkdir() - active = tmp_path / "Active" / "vogt-wi-7" - active.mkdir(parents=True) - stranger = tmp_path / "Active" / "other" + active_root = tmp_path / "Active" + checkout = active_root / "vogt" + checkout.mkdir(parents=True) + worktree = active_root / "vogt-wi-7" + worktree.mkdir() + linked = active_root / "vogt-link" + linked.symlink_to(worktree, target_is_directory=True) + stranger = active_root / "other" stranger.mkdir() - - def fake_git(root: Path, *args: str, required: bool = False) -> str: - if args[:2] == ("rev-parse", "--git-common-dir"): - if root == active.resolve() or root == checkout.resolve(): - return str(tmp_path / "common.git") - return str(tmp_path / "other.git") - return "" - - monkeypatch.setattr("vogt.application.services.sessions.git_output", fake_git) - # The fixture project points at ROOT, which does not exist. Register one - # whose checkout is the fake repository so the commondir check has a - # tree to compare against. + decoy = tmp_path / "decoy" / "Active" / "evil" + decoy.mkdir(parents=True) + common = tmp_path / "common.git" + other = tmp_path / "other.git" + seen: list[Path] = [] + + def fake_git(root: Path) -> str: + seen.append(root) + if root in {worktree.resolve(), checkout.resolve()}: + return str(common) + return str(other) + + monkeypatch.setattr("vogt.application.services.sessions._git_common_dir", fake_git) register_project( wired, RegisterProjectParams(name="Checkout", root_path=str(checkout), reason=WHY), @@ -556,17 +562,33 @@ def fake_git(root: Path, *args: str, required: bool = False) -> str: result = start_session( wired, - StartSessionParams(project="checkout", cwd=str(active), reason=WHY), + StartSessionParams(project="checkout", cwd=str(worktree), reason=WHY), ) - assert engine.last_spec["cwd"] == str(active.resolve()) + assert engine.last_spec["cwd"] == str(worktree.resolve()) assert "worktree" not in engine.last_spec - assert result.session.cwd == str(active.resolve()) + assert result.session.cwd == str(worktree.resolve()) - with pytest.raises(InvalidRequest, match="Active"): + linked_result = start_session( + wired, + StartSessionParams(project="checkout", cwd=str(linked), reason=WHY), + ) + assert linked_result.session.cwd == str(worktree.resolve()) + + with pytest.raises(InvalidRequest, match="absolute"): + start_session( + wired, + StartSessionParams(project="checkout", cwd="Active/vogt-wi-7", reason=WHY), + ) + with pytest.raises(InvalidRequest, match="shared checkout"): start_session( wired, StartSessionParams(project="checkout", cwd=str(checkout), reason=WHY), ) + with pytest.raises(InvalidRequest, match="not under the workspace Active"): + start_session( + wired, + StartSessionParams(project="checkout", cwd=str(decoy), reason=WHY), + ) with pytest.raises(InvalidRequest, match="not a git worktree"): start_session( wired, @@ -577,7 +599,7 @@ def fake_git(root: Path, *args: str, required: bool = False) -> str: wired, StartSessionParams( project="checkout", - cwd=str(tmp_path / "Active" / "missing"), + cwd=str(active_root / "missing"), reason=WHY, ), ) @@ -585,11 +607,29 @@ def fake_git(root: Path, *args: str, required: bool = False) -> str: start_session( wired, StartSessionParams( - project="checkout", cwd=str(active), worktree=True, reason=WHY + project="checkout", + cwd=str(worktree), + worktree=True, + reason=WHY, ), ) - # None of the refusals reached the engine. - assert engine.counter == 1 + # The implement profile sets worktree itself. A cwd still wins, and the + # engine is not asked to cut one. + purpose = start_session( + wired, + StartSessionParams( + project="checkout", + cwd=str(worktree), + purpose="implement", + reason=WHY, + ), + ) + assert purpose.session.cwd == str(worktree.resolve()) + assert "worktree" not in engine.last_spec + # The refusals never reached the engine: two accepted starts, then the + # purpose start. + assert engine.counter == 3 + assert decoy.resolve() not in seen def test_the_whole_session_clock_is_unset_unless_the_project_sets_it( From 9c96ec9719e2ae2b3758c3959df78b0a99628e64 Mon Sep 17 00:00:00 2001 From: sprooty Date: Thu, 15 Oct 2026 23:46:18 +0000 Subject: [PATCH 3/3] session.start cwd: refuse a subdirectory of the shared checkout (WI-1671) The path comparison only caught the checkout itself, so Active//src passed and the worker ran inside the shared tree. Compare git's show-toplevel with the checkout and refuse a match. A linked worktree reports its own toplevel, including one nested under the checkout. --- docs/ENGINE.md | 5 +- src/vogt/application/models.py | 6 ++- src/vogt/application/services/sessions.py | 56 +++++++++++++++-------- tests/test_sessions.py | 20 ++++++-- 4 files changed, 60 insertions(+), 27 deletions(-) diff --git a/docs/ENGINE.md b/docs/ENGINE.md index c339344c4..6b91b95a5 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -860,8 +860,9 @@ than these routes. the workspace `Active/` directory (the parent of the project's registered checkout, not any path component named `Active`), and be a git worktree of the same repository as that checkout (the same git `commondir`). The - checkout itself is refused: a cwd start is how a worker stays off the - shared tree. A symlink is accepted as the directory it resolves to. + checkout, and any subdirectory of it, is refused: its git toplevel is + the checkout, so a worker cannot start inside the shared tree. A linked + worktree nested under the checkout has its own toplevel and is accepted. A symlink is accepted as the directory it resolves to. Combining it with an explicit `worktree: true` is refused before the engine is asked. A purpose whose profile would have set `worktree` (the `implement` profile does) does not: `cwd` means the checkout already diff --git a/src/vogt/application/models.py b/src/vogt/application/models.py index f1a361ace..88672cf03 100644 --- a/src/vogt/application/models.py +++ b/src/vogt/application/models.py @@ -4238,8 +4238,10 @@ class StartSessionParams(Params): "already exists and `worktree=true` cannot cut a new one — a " "private origin the engine cannot fetch, for example. The path " "must exist, must sit under the workspace's `Active/` directory " - "(the parent of the project's checkout), must not be that " - "checkout itself, and must be a git worktree of the same " + "(the parent of the project's checkout), must not be inside " + "that checkout (a subdirectory shares its git toplevel; a " + "linked worktree nested under it does not), and must be a " + "git worktree of the same " "repository (the same `commondir`). Anything else is refused, " "and so is combining it with an explicit `worktree=true`. A " "purpose profile that would have set `worktree` does not, when " diff --git a/src/vogt/application/services/sessions.py b/src/vogt/application/services/sessions.py index 4217938fe..13f89b025 100644 --- a/src/vogt/application/services/sessions.py +++ b/src/vogt/application/services/sessions.py @@ -3110,17 +3110,22 @@ def default_name(self) -> str: return self.work_item_ref or self.project_slug -def _git_common_dir(root: Path) -> str: - """`git rev-parse --git-common-dir` with config that cannot run hooks. - - The directory is one the caller named, so the probe must not honour a - repo's fsmonitor, hooks, or system gitconfig. `-c` overrides the repo - config; `GIT_CONFIG_NOSYSTEM` drops the system one. +def _git_probe(root: Path, flag: str) -> str: + """One `git rev-parse` question, with nothing the repo can run. + + `rev-parse` runs no hooks. `core.fsmonitor` and `core.hooksPath` are + overridden so a repo's monitor and hooks are not consulted, and + `GIT_CONFIG_NOSYSTEM` drops the system gitconfig. Inherited `GIT_DIR`, + `GIT_WORK_TREE` and `GIT_COMMON_DIR` are removed so the engine's own + environment cannot point the probe at a different repository. """ - env = os.environ.copy() + env = { + key: value + for key, value in os.environ.items() + if key not in {"GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_CONFIG_COUNT"} + } env["GIT_CONFIG_NOSYSTEM"] = "1" - env["GIT_CONFIG_COUNT"] = "0" try: completed = subprocess.run( # fixed argv, never a shell [ @@ -3132,7 +3137,7 @@ def _git_common_dir(root: Path) -> str: "-c", "core.hooksPath=/dev/null", "rev-parse", - "--git-common-dir", + flag, ], capture_output=True, text=True, @@ -3156,9 +3161,11 @@ def _existing_worktree_cwd(raw: str, project_root: str) -> str: Four checks, in the order a wrong path fails them: it exists, it sits under the workspace's real `Active/` directory (the parent of the project's registered checkout, not any path component named `Active`), - it is not that checkout itself, and it is a git worktree of the same - repository. The comparison is git's `commondir`, which a linked worktree - and the main checkout of one repository share and two repositories do not. + it is not inside that checkout (its git toplevel differs, so a linked + worktree nested under the checkout still passes), and it is a git + worktree of the same repository. The comparison is git's `commondir`, + which a linked worktree and the main checkout of one repository share + and two repositories do not. """ path = Path(raw).expanduser() if not path.is_absolute(): @@ -3184,15 +3191,13 @@ def _existing_worktree_cwd(raw: str, project_root: str) -> str: if active.name != "Active" or not resolved.is_relative_to(active): msg = f"cwd {resolved} is not under the workspace Active/ directory ({active})" raise InvalidRequest(msg) - if resolved == checkout_resolved: - msg = ( - f"cwd {resolved} is the project's shared checkout; name a " - "worktree of it instead" - ) - raise InvalidRequest(msg) try: - asked = _git_common_dir(resolved) - owned = _git_common_dir(checkout_resolved) + asked = _git_probe(resolved, "--git-common-dir") + owned = _git_probe(checkout_resolved, "--git-common-dir") + # A subdirectory of the shared checkout shares its commondir and is + # under Active/, so the path comparison above misses it. Its + # toplevel is the checkout, which a linked worktree's is not. + toplevel = _git_probe(resolved, "--show-toplevel") except CollectorError as exc: msg = ( f"cwd {resolved} is not a git worktree of the project's " @@ -3216,6 +3221,17 @@ def _existing_worktree_cwd(raw: str, project_root: str) -> str: f"repository ({project_root})" ) raise InvalidRequest(msg) + try: + top = Path(toplevel).resolve() + except OSError as exc: + msg = f"cwd {resolved} is not a git worktree: {exc}" + raise InvalidRequest(msg) from exc + if top == checkout_resolved: + msg = ( + f"cwd {resolved} is inside the project's shared checkout; name a " + "worktree of it instead" + ) + raise InvalidRequest(msg) return str(resolved) diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 9796b64be..bda533aaf 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -548,13 +548,22 @@ def test_a_cwd_opens_in_an_existing_worktree( other = tmp_path / "other.git" seen: list[Path] = [] - def fake_git(root: Path) -> str: + nested = checkout / "src" + nested.mkdir() + + def fake_git(root: Path, flag: str) -> str: seen.append(root) - if root in {worktree.resolve(), checkout.resolve()}: + if flag == "--show-toplevel": + # A subdirectory of the checkout reports the checkout. A linked + # worktree reports itself. + if root.resolve() in {worktree.resolve(), linked.resolve()}: + return str(worktree.resolve()) + return str(checkout.resolve()) + if root.resolve() in {worktree.resolve(), checkout.resolve(), nested.resolve()}: return str(common) return str(other) - monkeypatch.setattr("vogt.application.services.sessions._git_common_dir", fake_git) + monkeypatch.setattr("vogt.application.services.sessions._git_probe", fake_git) register_project( wired, RegisterProjectParams(name="Checkout", root_path=str(checkout), reason=WHY), @@ -584,6 +593,11 @@ def fake_git(root: Path) -> str: wired, StartSessionParams(project="checkout", cwd=str(checkout), reason=WHY), ) + with pytest.raises(InvalidRequest, match="shared checkout"): + start_session( + wired, + StartSessionParams(project="checkout", cwd=str(nested), reason=WHY), + ) with pytest.raises(InvalidRequest, match="not under the workspace Active"): start_session( wired,