diff --git a/docs/ENGINE.md b/docs/ENGINE.md index f651bcc2a..6b91b95a5 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -854,6 +854,19 @@ 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 (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, 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 + 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 774d4f0cf..88672cf03 100644 --- a/src/vogt/application/models.py +++ b/src/vogt/application/models.py @@ -4226,7 +4226,26 @@ 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 " + "(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 " + "`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 d7bc1c17f..13f89b025 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 @@ -124,6 +126,7 @@ 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.core import delivery, oversight, runtime from vogt.core.auth import Scope, hash_token, issue, parse_scopes from vogt.core.branches import default_branch_name @@ -251,7 +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). + # 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). @@ -259,6 +273,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 +349,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 +3110,131 @@ def default_name(self) -> str: return self.work_item_ref or self.project_slug +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 = { + 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" + try: + completed = subprocess.run( # fixed argv, never a shell + [ + "git", + "-C", + str(root), + "-c", + "core.fsmonitor=", + "-c", + "core.hooksPath=/dev/null", + "rev-parse", + flag, + ], + 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). + + 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 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(): + 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 + 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) + 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) + try: + 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 " + 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) + 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) + + 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..bda533aaf 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -518,6 +518,134 @@ 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. + 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. + """ + 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() + decoy = tmp_path / "decoy" / "Active" / "evil" + decoy.mkdir(parents=True) + common = tmp_path / "common.git" + other = tmp_path / "other.git" + seen: list[Path] = [] + + nested = checkout / "src" + nested.mkdir() + + def fake_git(root: Path, flag: str) -> str: + seen.append(root) + 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_probe", fake_git) + register_project( + wired, + RegisterProjectParams(name="Checkout", root_path=str(checkout), reason=WHY), + ) + + result = start_session( + wired, + StartSessionParams(project="checkout", cwd=str(worktree), reason=WHY), + ) + assert engine.last_spec["cwd"] == str(worktree.resolve()) + assert "worktree" not in engine.last_spec + assert result.session.cwd == str(worktree.resolve()) + + 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="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, + StartSessionParams(project="checkout", cwd=str(decoy), 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(active_root / "missing"), + reason=WHY, + ), + ) + with pytest.raises(InvalidRequest, match="cannot be combined"): + start_session( + wired, + StartSessionParams( + project="checkout", + cwd=str(worktree), + worktree=True, + reason=WHY, + ), + ) + # 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( wired: AppContext, engine: StandInEngine ) -> None: