diff --git a/docs/ENGINE.md b/docs/ENGINE.md index b38198a1..95d9340e 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -552,7 +552,18 @@ is absent, empty, or all whitespace writes no file and sets no variable. that escapes via `..` is `400` rather than a shell in `/etc`. `name` is trimmed and must be non-empty and at most 256 bytes, on creation and on rename alike; outside that it is `400`, never truncated. Names need not be unique — -duplicates are confusing, not invalid. The remaining `SessionSpec` fields — +duplicates are confusing, not invalid. `SessionSpec` also carries an optional `template` — a session template *name* +or *tag* the engine expands into `command` (and env) against its configured +`session_templates`, when no explicit `command` is given (the GUI copies a +template's command into the spec itself, so it never uses this). A name matches +case-insensitively; failing that a tag matches, and an `agent`-tagged template +wins an ambiguous tag so `template: "claude"` reaches the deployment's +protected `["vogt-agent-auth", "run", "--", "claude"]` rather than a bare +binary. An unknown name is `400` listing the configured templates, never +silently a shell. This is where vogt-core sends the bare agent name it was +asked for and lets the deployment decide what protected command it runs. + +The remaining `SessionSpec` fields — `command`, `env`, `cols`, `rows`, `scrollback_bytes` — each fall back to the server's configured default when omitted. diff --git a/engine/contract/src/lib.rs b/engine/contract/src/lib.rs index 6ebf5bd2..d4c27820 100644 --- a/engine/contract/src/lib.rs +++ b/engine/contract/src/lib.rs @@ -26,6 +26,16 @@ pub struct SessionSpec { pub name: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub command: Option>, + /// A session template to expand into `command` (and env), by name or by + /// tag, resolved against the engine's configured `session_templates`. + /// The engine owns the mapping because the command a template runs — a + /// `vogt-agent-auth run -- claude` wrapper, say — is that deployment's + /// configuration, not the caller's to spell out. Ignored when `command` + /// is given; an unknown name is refused. vogt-core sends the name it was + /// asked for (`claude`) and lets the engine turn it into the protected + /// command the deployment configured for it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub template: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub cwd: Option, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/engine/server/src/agent_tasks.rs b/engine/server/src/agent_tasks.rs index 9da512a7..9f955181 100644 --- a/engine/server/src/agent_tasks.rs +++ b/engine/server/src/agent_tasks.rs @@ -1461,6 +1461,7 @@ impl AgentTaskRegistry { let session = self.sessions.create(SessionSpec { name: session_name.clone(), command: Some(command), + template: None, cwd: task.cwd.clone(), env: Some(env), // A task run has already written its own prompt file above, with diff --git a/engine/server/src/assistant.rs b/engine/server/src/assistant.rs index fbd156d2..e0748516 100644 --- a/engine/server/src/assistant.rs +++ b/engine/server/src/assistant.rs @@ -2172,6 +2172,7 @@ mod tests { .create(SessionSpec { name: "cat".into(), command: Some(vec!["cat".into()]), + template: None, cwd: None, env: None, prompt: None, diff --git a/engine/server/src/sessions.rs b/engine/server/src/sessions.rs index a66d7d90..9a6c439e 100644 --- a/engine/server/src/sessions.rs +++ b/engine/server/src/sessions.rs @@ -16,6 +16,41 @@ use crate::{ workspace_path, }; +/// A configured session template matched by name (case-insensitive) or, +/// failing that, by tag — so a caller can say `claude` and reach the template +/// tagged `claude`. When a tag matches more than one template an `agent`-tagged +/// one wins, then the alphabetically-first name, so the choice is deterministic +/// rather than list-order-dependent. An unknown name is refused with the +/// configured names, never quietly started as a shell. +fn resolve_template_in<'a>( + templates: &'a [crate::config::SessionTemplate], + name: &str, +) -> Result<&'a crate::config::SessionTemplate> { + if let Some(t) = templates.iter().find(|t| t.name.eq_ignore_ascii_case(name)) { + return Ok(t); + } + let mut by_tag: Vec<&crate::config::SessionTemplate> = templates + .iter() + .filter(|t| t.tags.iter().any(|tag| tag.eq_ignore_ascii_case(name))) + .collect(); + by_tag.sort_by(|a, b| { + let a_agent = a.tags.iter().any(|g| g.eq_ignore_ascii_case("agent")); + let b_agent = b.tags.iter().any(|g| g.eq_ignore_ascii_case("agent")); + b_agent.cmp(&a_agent).then_with(|| a.name.cmp(&b.name)) + }); + if let Some(t) = by_tag.first() { + return Ok(t); + } + let known = templates + .iter() + .map(|t| t.name.as_str()) + .collect::>() + .join(", "); + Err(ApiError::BadRequest(format!( + "unknown session template {name:?}; configured: {known}" + ))) +} + pub struct SessionRegistry { cfg: Arc, bus: EventBus, @@ -47,6 +82,31 @@ impl SessionRegistry { pub fn create(&self, mut spec: SessionSpec) -> Result> { spec.name = normalize_session_name(&spec.name)?; + // Expand a template name into a command before anything downstream + // reads `command`. Only when the caller gave no explicit command — + // the GUI copies a template's command into the spec itself and sends + // that, so it never takes this path. vogt-core sends the bare name + // ("claude") and the deployment's config is where that becomes + // `vogt-agent-auth run -- claude`, keeping the wrapper out of the + // core and out of shipped code. + if spec.command.is_none() { + if let Some(name) = spec + .template + .as_deref() + .map(str::trim) + .filter(|t| !t.is_empty()) + { + let template = self.resolve_template(name)?; + spec.command = template.command.clone(); + if !template.env.is_empty() { + let mut env = template.env.clone(); + if let Some(existing) = spec.env.take() { + env.extend(existing); + } + spec.env = Some(env); + } + } + } // Resolve client-supplied cwd against workspace_root. Reject anything // that escapes the workspace via `..` so a stray API call can't spawn // a shell with cwd=/etc. @@ -222,6 +282,16 @@ impl SessionRegistry { out } + /// A configured session template matched by name (case-insensitive) or, + /// failing that, by tag — so a caller can say `claude` and reach the + /// template tagged `claude`. When a tag matches more than one template an + /// `agent`-tagged one wins, then the alphabetically-first name, so the + /// choice is deterministic rather than list-order-dependent. An unknown + /// name is refused with the configured names, never started as a shell. + fn resolve_template(&self, name: &str) -> Result<&crate::config::SessionTemplate> { + resolve_template_in(&self.cfg.session_templates, name) + } + pub fn rename(&self, id: Uuid, new_name: String) -> Result<()> { let s = self.get(id)?; let new_name = normalize_session_name(&new_name)?; @@ -298,4 +368,84 @@ mod tests { let err = normalize_session_name(&long).unwrap_err(); assert!(err.to_string().contains("at most 256 bytes")); } + #[test] + fn a_template_resolves_by_name_then_by_tag() { + use crate::config::SessionTemplate; + let templates = SessionTemplate::default_templates(); + // Exact name, case-insensitive. + assert_eq!( + super::resolve_template_in(&templates, "claude code (protected)") + .unwrap() + .name, + "Claude Code (protected)" + ); + // By tag: `claude` reaches the protected Claude template, whose + // command is the deployment's wrapper — the whole point. + let claude = super::resolve_template_in(&templates, "claude").unwrap(); + assert_eq!(claude.name, "Claude Code (protected)"); + assert!(claude + .command + .as_ref() + .unwrap() + .iter() + .any(|arg| arg == "claude")); + // A plain shell is still reachable by name. + assert_eq!( + super::resolve_template_in(&templates, "Shell") + .unwrap() + .name, + "Shell" + ); + } + + #[test] + fn an_unknown_template_is_refused_with_the_configured_names() { + use crate::config::SessionTemplate; + let templates = SessionTemplate::default_templates(); + let err = super::resolve_template_in(&templates, "kardashian").unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("unknown session template"), "{msg}"); + assert!(msg.contains("Claude Code (protected)"), "{msg}"); + } + + #[test] + fn the_agent_tag_wins_when_a_tag_is_ambiguous() { + use crate::config::SessionTemplate; + // Two templates share a tag; the agent one must win deterministically. + let templates = vec![ + SessionTemplate { + name: "Zeta Shell".into(), + description: String::new(), + command: Some(vec!["bash".into()]), + cwd: None, + env: vec![], + default_name: None, + match_repo_names: vec![], + match_path_prefixes: vec![], + tags: vec!["claude".into()], + }, + SessionTemplate { + name: "Alpha Agent".into(), + description: String::new(), + command: Some(vec![ + "vogt-agent-auth".into(), + "run".into(), + "--".into(), + "claude".into(), + ]), + cwd: None, + env: vec![], + default_name: None, + match_repo_names: vec![], + match_path_prefixes: vec![], + tags: vec!["agent".into(), "claude".into()], + }, + ]; + assert_eq!( + super::resolve_template_in(&templates, "claude") + .unwrap() + .name, + "Alpha Agent" + ); + } } diff --git a/src/vogt/adapters/engine/client.py b/src/vogt/adapters/engine/client.py index 3476c007..d538b8c6 100644 --- a/src/vogt/adapters/engine/client.py +++ b/src/vogt/adapters/engine/client.py @@ -356,7 +356,8 @@ def create_session( self, *, name: str, - command: list[str] | None, + command: list[str] | None = None, + template: str | None = None, cwd: str, env: dict[str, str] | None = None, prompt: str | None = None, @@ -374,6 +375,12 @@ def create_session( spec: dict[str, Any] = {"name": name, "cwd": cwd} if command: spec["command"] = command + if template: + # A template *name*, not a command: the engine expands it against + # its own `session_templates`, because the command a template runs + # (a `vogt-agent-auth run -- claude` wrapper, say) is that + # deployment's configuration and not Vogt's to spell out. + spec["template"] = template if prompt: # The engine writes this to a file on its own state directory and # tells the child where it is. Vogt sends the text rather diff --git a/src/vogt/application/models.py b/src/vogt/application/models.py index 39999b10..1679f8cb 100644 --- a/src/vogt/application/models.py +++ b/src/vogt/application/models.py @@ -2622,8 +2622,22 @@ class StartSessionParams(Params): template: str | None = Field( default=None, description=( - "Named session template to run, e.g. an agent CLI. Omitted means " - "the engine's default shell." + "Session template to run, by name or by tag — the engine expands " + "it against the deployment's templates. Use this to run an agent " + "rather than a plain shell: `claude` (or `codex`, `opencode`) " + "starts that agent under the deployment's protected wrapper, so " + "a request to *do* something in a session names one here. " + "Omitted means a plain shell, which does nothing until typed into." + ), + ) + task: str | None = Field( + default=None, + description=( + "What the session's agent should do, in the user's words. Folded " + "into the brief the agent opens with, so 'start a session on X " + "and check its containers' opens an agent already asked to check " + "them rather than an idle shell. Pair it with `template` (an " + "agent, not a plain shell) when the task is something to carry out." ), ) name: str | None = Field( diff --git a/src/vogt/application/services/sessions.py b/src/vogt/application/services/sessions.py index 56ed884e..45d052aa 100644 --- a/src/vogt/application/services/sessions.py +++ b/src/vogt/application/services/sessions.py @@ -94,7 +94,7 @@ def start_session(ctx: AppContext, params: StartSessionParams) -> SessionResult: template=params.template, cwd=subject.cwd, env=_session_env(ctx, session_id, credential.secret), - brief=subject.brief, + brief=_brief_with_task(subject.brief, params.task), model=params.model, effort=params.effort, ) @@ -558,6 +558,23 @@ def _existing(ctx: AppContext, session_id: str) -> CodingSession: return session +def _brief_with_task(brief: str, task: str | None) -> str: + """Fold an explicit task into the session's brief. + + The brief is context — the project, or the work item and why it ranks — + and by itself it asks the agent to do nothing (a project brief says so + in as many words). A spoken "start a session on komodo and check the + containers" carries the *doing* part separately; without it the agent + opens and waits, and the person has to type what they just said. + Appended rather than replacing so the agent keeps the context under + the task. + """ + task = (task or "").strip() + if not task: + return brief + return f"{brief.rstrip()}\n\n## Task\n\n{task}\n" + + def _start_on_engine( engine: EngineClient, *, @@ -572,10 +589,14 @@ def _start_on_engine( return engine.create_session( prompt=brief, name=name, - # A template names a command the *engine* knows; Vogt passes the name - # through rather than resolving it, because the command a template - # runs is that pod's configuration and not the estate's. - command=None if template is None else [template], + # A template names a command the *engine* knows; Vogt sends the + # name and the engine expands it against its own `session_templates`, + # because the command a template runs — a `vogt-agent-auth run -- + # claude` wrapper, say — is that pod's configuration and not the + # estate's. Sending the bare name is what lets "start an agent on + # this" reach the deployment's protected agent template rather than + # an unwrapped binary. + template=template, cwd=cwd, env=env, # Passed through for the same reason: *how* a model id diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 498db58b..4c9b684b 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -243,6 +243,53 @@ def test_a_session_can_be_opened_on_a_project( assert engine.last_spec["cwd"] == ROOT +def test_a_task_is_folded_into_the_session_brief( + wired: AppContext, engine: StandInEngine +) -> None: + """A spoken request carries the doing part; the brief must too. + + Without this the agent opens on the project brief, which says in as many + words that there is no task, and waits — the person then types what they + already said out loud. + """ + start_session( + wired, + StartSessionParams( + project="vogt", + task="check how many containers need an update", + reason=WHY, + ), + ) + brief = engine.last_spec["prompt"] + assert "## Task" in brief + assert "check how many containers need an update" in brief + # The project context is still there, under the task. + assert brief.index("# vogt") < brief.index("## Task") + + +def test_no_task_leaves_the_brief_untouched( + wired: AppContext, engine: StandInEngine +) -> None: + start_session(wired, StartSessionParams(project="vogt", reason=WHY)) + assert "## Task" not in engine.last_spec["prompt"] + + +def test_a_template_is_sent_by_name_for_the_engine_to_expand( + wired: AppContext, engine: StandInEngine +) -> None: + """The engine owns the command a template runs; Vogt sends the name. + + Sending `claude` (not `["claude"]`) is what lets the engine expand it to + the deployment's protected wrapper rather than an unwrapped binary. + """ + start_session( + wired, + StartSessionParams(project="vogt", template="claude", reason=WHY), + ) + assert engine.last_spec.get("template") == "claude" + assert "command" not in engine.last_spec + + def test_a_session_needs_exactly_one_subject(wired: AppContext) -> None: with pytest.raises(InvalidRequest): start_session(wired, StartSessionParams(reason=WHY))