diff --git a/crates/tui/assets/skills-catalog-matrix.json b/crates/tui/assets/skills-catalog-matrix.json index 3525589db9..f3f3f11928 100644 --- a/crates/tui/assets/skills-catalog-matrix.json +++ b/crates/tui/assets/skills-catalog-matrix.json @@ -15,7 +15,7 @@ "in_model_catalogue": "true when the skill renders as an ambient catalogue line", "shadowed_aliases": "aliases that collide with another canonical bundled name; the canonical skill wins resolution" }, - "generation": "12", + "generation": "13", "skills": [ { "name": "skill-creator", diff --git a/crates/tui/assets/skills/best-of-n/SKILL.md b/crates/tui/assets/skills/best-of-n/SKILL.md index 62e2813919..6d3c78ecc7 100644 --- a/crates/tui/assets/skills/best-of-n/SKILL.md +++ b/crates/tui/assets/skills/best-of-n/SKILL.md @@ -1,8 +1,6 @@ --- name: best-of-n description: Generate a small set of independent candidate solutions in worktrees, judge them against one explicit rubric, and apply the winner only after PASS verification. -metadata: - short-description: Compare independent candidates --- # Best of N diff --git a/crates/tui/assets/skills/delegate/SKILL.md b/crates/tui/assets/skills/delegate/SKILL.md index 9576baa869..783e287747 100644 --- a/crates/tui/assets/skills/delegate/SKILL.md +++ b/crates/tui/assets/skills/delegate/SKILL.md @@ -1,8 +1,6 @@ --- name: delegate description: Strategic delegation for multi-step coding, research, or verification work. Use when a task can be split into parent reasoning plus focused sub-agent execution through the agent tool. -metadata: - short-description: Delegate focused work to sub-agents --- # Delegate diff --git a/crates/tui/assets/skills/fleet-manager/SKILL.md b/crates/tui/assets/skills/fleet-manager/SKILL.md index 42c4616927..8faf4c7c35 100644 --- a/crates/tui/assets/skills/fleet-manager/SKILL.md +++ b/crates/tui/assets/skills/fleet-manager/SKILL.md @@ -1,8 +1,6 @@ --- name: fleet-manager description: Use when managing, triaging, restarting, escalating, or summarizing Codewhale fleet runs and workers. -metadata: - short-description: Triage Codewhale fleet runs --- # Fleet Manager diff --git a/crates/tui/assets/skills/skill-creator/SKILL.md b/crates/tui/assets/skills/skill-creator/SKILL.md index 8e4434a7d3..f7ff245299 100644 --- a/crates/tui/assets/skills/skill-creator/SKILL.md +++ b/crates/tui/assets/skills/skill-creator/SKILL.md @@ -1,8 +1,6 @@ --- name: skill-creator description: Create or improve Codewhale skills. Use when the user wants a new skill, wants to update an existing skill, or needs guidance on when a skill should be a skill versus MCP, hooks, tools, or a plugin scaffold. -metadata: - short-description: Create Codewhale skills aliases-for: create-skill --- diff --git a/crates/tui/assets/skills/v4-best-practices/SKILL.md b/crates/tui/assets/skills/v4-best-practices/SKILL.md index f3d2ec59dc..b37d1b3fb5 100644 --- a/crates/tui/assets/skills/v4-best-practices/SKILL.md +++ b/crates/tui/assets/skills/v4-best-practices/SKILL.md @@ -11,15 +11,15 @@ specific, observable failure class. ## 1. Verify references before writing Before referencing a file path, function, or type in code or plan output, -call `grep_files` or `read_file` to confirm it exists in the workspace. +call `grep_files` or `read` to confirm it exists in the workspace. ``` -# Bad: edit_file path="src/config/loader.rs" (assumed from memory) +# Bad: edit path="src/config/loader.rs" (assumed from memory) # Good: grep_files pattern="pub fn load_config" → confirms src/config/mod.rs:42 # then reference src/config/mod.rs:42 ``` -Failure avoided: `edit_file` errors on non-existent paths; LSP diagnostics +Failure avoided: `edit` errors on non-existent paths; LSP diagnostics on hallucinated symbols. ## 2. Spawn a verifier sub-agent before multi-file execution diff --git a/crates/tui/src/prompts.rs b/crates/tui/src/prompts.rs index c65da075fe..1d2e92cb73 100644 --- a/crates/tui/src/prompts.rs +++ b/crates/tui/src/prompts.rs @@ -1,14 +1,15 @@ #![allow(dead_code)] //! System prompt composition. //! -//! Prompts are assembled from composable layers loaded at compile time from -//! the single `text` module: -//! constitution + personality overlay → `message[0]` (byte-stable). +//! Prompts are assembled from composable layers loaded at compile time — +//! most from the `text` module, with the authority recap and the locale +//! bookends defined here: +//! constitution (personality folded in) → `message[0]` (byte-stable). //! approval policy → request-time runtime metadata. //! Tool availability comes only from the per-turn model catalog. //! -//! Keeping every layer's text in one module makes prompt tuning a -//! single-file operation. +//! Every layer's text stays a compile-time constant — this module and +//! `text`, per the split above. use crate::models::{SystemBlock, SystemPrompt}; use crate::project_context::load_project_context_with_parents; @@ -82,8 +83,9 @@ impl Default for PromptSessionContext<'_> { /// Conventional location for the structured session relay artifact (#32). /// A previous session writes it on exit / `/compact`; the next session reads -/// it back on startup and prepends it to the system prompt so a fresh agent -/// doesn't have to re-discover open blockers from scratch. +/// it back on startup and routes it as the late WorldState token-budget / +/// continuity fragment (`load_handoff_block`) so a fresh agent doesn't have +/// to re-discover open blockers from scratch. pub const HANDOFF_RELATIVE_PATH: &str = ".codewhale/handoff.md"; /// Legacy handoff path for reading from existing installs. `pub(crate)` so /// command surfaces that teach the fallback name it from the same constant @@ -189,9 +191,10 @@ fn translation_target_language_for_tag(locale_tag: &str) -> &'static str { /// Render a `## Environment` block listing the resolved locale tag and the /// actionable host facts that affect command syntax. /// -/// The block is appended to the workspace-static portion of the system -/// prompt (after the shared constitution + project context, before configured -/// instructions / skills). `locale_tag` is resolved by the caller from +/// The block leads the workspace WorldState fragment (environment + memory / +/// goal facts), which is assembled after the cache-stable constitution +/// prefix so a change re-renders as a fragment diff instead of rebuilding +/// the static prefix. `locale_tag` is resolved by the caller from /// `Settings` so this function stays I/O-free. /// /// `platform` and `shell` remain because they change how commands must be @@ -401,10 +404,12 @@ fn user_constitution_disabled_by_setup_state() -> bool { // ── Prompt layers loaded at compile time ────────────────────────────── // -// Every bundled prompt layer lives in `prompts/text.rs` as a compile-time -// constant (consolidated from the retired per-layer `prompts/*.md` files; -// each constant is byte-identical to the file it replaced, trailing newline -// included). The constants are re-exported here so the existing +// Every bundled prompt layer is a compile-time constant — most in +// `prompts/text.rs`, with the authority recap and the locale bookends +// defined in this file (consolidated from the retired per-layer +// `prompts/*.md` files; each constant is byte-identical to the file it +// replaced, trailing newline included). The constants are re-exported here +// so the existing // `crate::prompts::NAME` paths used across the crate are unchanged. Edit // prompt text in `text.rs` directly; the test suite below guards content // and ordering invariants (constitution structure and binding gates #4032, @@ -447,15 +452,16 @@ static PROMPT_OVERRIDE_NOTICES: LazyLock>> = pub struct StaticPromptCtx<'a> { /// Active model identifier after caller-side routing. pub model_id: &'a str, - /// Personality overlay requested for the base static prompt. + /// Personality requested for the base static prompt (retired: the runtime + /// folds the tone into the constitution rather than composing an overlay). pub personality: Personality, - /// Default base/personality prompt layers that would be used without an - /// override. + /// Default base prompt (personality already folded in) that would be used + /// without an override. pub default_layers: &'a str, } -/// Embedder hook for replacing Codewhale's byte-stable base/personality prompt -/// segment. +/// Embedder hook for replacing Codewhale's byte-stable base prompt segment +/// (personality folded in). pub type StaticPromptComposer = dyn Fn(&StaticPromptCtx<'_>) -> String + Send + Sync + 'static; /// Install an embedder composer that owns the complete static prompt prefix. @@ -1007,10 +1013,10 @@ fn apply_model_template( prompt.replace("{model_id}", model_id) } -/// Authority recap block — appended at the end of the system prompt, -/// just before the user's first message. Uses recency bias constructively -/// without restating ranks: `BASE_PROMPT` § Whose word wins states the -/// default ranking (#4777). +/// Authority recap block — appended as a per-request trailer after the +/// WorldState fragments, just before the locale closer. Uses recency bias +/// constructively without restating ranks: `BASE_PROMPT` § Whose word wins +/// states the default ranking (#4777). const AUTHORITY_RECAP: &str = "\ ## Authority Recap @@ -1094,21 +1100,27 @@ pub fn system_prompt_for_mode_with_context( /// Get the system prompt for a specific mode with project and skills context. /// -/// **Volatile-content-last invariant.** Blocks are appended in order from -/// most-static to most-volatile so DeepSeek's KV prefix cache hits the -/// longest possible byte prefix turn-over-turn: +/// **Volatile-content-last invariant.** The cache-stable `Blocks[0]` prefix +/// is the composed constitution — base prompt, project context, skills +/// block, and `## Core Execution` discipline — and per-turn material follows +/// it from most-static to most-volatile so DeepSeek's KV prefix cache hits +/// the longest possible byte prefix turn-over-turn: /// -/// 1. shared constitution (compile-time constant) -/// 2. project context / fallback (workspace-static) -/// 3. skills block (skills-dir-static) -/// 4. `## Core Execution` (compile-time constant) -/// 5. compaction relay template (compile-time constant) -/// 6. relay block — file-backed; rewritten by `/compact` and on exit +/// 1. composed constitution prefix (compile-time constant, plus +/// workspace-static project context and the skills-dir-static skills +/// block) +/// 2. WorldState fragments (environment/memory/goal, the interactive +/// continual-harness block, permissions, route, handoff, and imported +/// project instructions) that can `render_diff` mid-session +/// 3. per-request trailers: authority recap, then locale closer /// -/// Anything appended after a volatile block forfeits the cache for the rest -/// of the request. New blocks belong above the relay boundary unless they -/// themselves are turn-volatile. Working-set metadata is now injected into the -/// latest user message as per-turn metadata instead of this system prompt. +/// The compaction relay is not a system block: automatic compaction owns its +/// structured successor brief and `/relay` appends `COMPACT_TEMPLATE` to that +/// command's user message. Anything appended after a volatile block forfeits +/// the cache for the rest of the request; new content belongs above the +/// fragment boundary unless it is itself turn-volatile. Working-set metadata +/// is injected into the latest user message as per-turn metadata instead of +/// this system prompt. pub fn system_prompt_for_mode_with_context_and_skills( workspace: &Path, working_set_summary: Option<&str>, @@ -3124,7 +3136,7 @@ mod tests { } #[test] - fn handoff_artifact_is_prepended_to_system_prompt_when_present() { + fn handoff_artifact_is_included_in_the_system_prompt_when_present() { let tmp = tempdir().expect("tempdir"); let workspace = tmp.path(); let handoff_dir = workspace.join(".deepseek"); diff --git a/crates/tui/src/prompts/text.rs b/crates/tui/src/prompts/text.rs index b4a0fa8376..1f1f125b6a 100644 --- a/crates/tui/src/prompts/text.rs +++ b/crates/tui/src/prompts/text.rs @@ -1,44 +1,58 @@ -//! Compile-time prompt text — the single source of truth for every bundled -//! layer of the Codewhale system prompt. +//! Compile-time prompt text — most bundled layers of the Codewhale system +//! prompt live here as constants (the authority recap and the locale +//! bookends are defined in `prompts.rs`). //! //! Each constant below used to live in its own `prompts/*.md` file, pulled in //! with `include_str!`. The per-layer file sprawl (17 files across 4 -//! directories) was consolidated into this one module so the whole prompt -//! contract reads top-to-bottom in a single place, the way the runtime -//! assembly composes it. The text moved **verbatim** — every constant is -//! byte-identical to the file it replaced, trailing newline included — so +//! directories) was consolidated into this module for the constants that +//! moved; the authority recap and the locale bookends have always lived in +//! `prompts.rs`, so the bulk — not the whole — of the prompt contract reads +//! top-to-bottom in one place. The text moved **verbatim** — every constant +//! is byte-identical to the file it replaced, trailing newline included — so //! rendered prompts do not change by a single byte. //! //! Organization follows the runtime assembly order, most-static → //! most-volatile (see `system_prompt_for_mode_with_context_skills_and_session` //! in `../prompts.rs`): //! -//! 1. Constitution (binding core: `BASE_PROMPT` + language/output law) -//! 2. Personality overlay (`CALM_PERSONALITY` — one overlay, not a set) -//! 3. Approval-policy overlays -//! 4. Runtime templates (compaction relay, goal continuation, memory, +//! 1. Constitution (binding core: `BASE_PROMPT` + language/output law; +//! personality tone is folded into the constitutional preamble, not +//! composed as a separate layer) +//! 2. Runtime templates (compaction relay, goal continuation, memory, //! core execution, sub-agent output contract) //! +//! Approval policy is not a layer here: it travels as request-time runtime +//! metadata (see the `prompts.rs` module header). +//! +//! `CALM_PERSONALITY` below is a retired overlay: the runtime never composes +//! it (tests assert the Calm tone stays folded into the constitution), and it +//! is kept only so the historic text and its guard tests remain inspectable. +//! //! Edit prompt text here directly. Content and ordering invariants are //! guarded by the test suite in `../prompts.rs` (constitution structure, //! binding gates, prefix privacy, byte-stable prefix ordering) — run -//! `cargo test -p codewhale-tui --bin codewhale-tui prompts` after edits. +//! `cargo test -p codewhale-tui --lib prompts` after edits. //! //! The locale-tagged bookends (per-locale preambles/closers) remain in //! `../prompts.rs` next to the override cells that can replace them. // ── Constitution — the binding core (#4032) ───────────────────────── -/// Core: task execution, tool-use rules, output format, toolbox reference, -/// "When NOT to use" guidance, sub-agent sentinel protocol. +/// Core: ground truth, user intent and scope, truthful completion, +/// guarantees-in-mechanism, and the default authority ranking +/// ("Whose word wins"). /// /// This text is the single hand-maintained source of the constitutional /// system prompt. The earlier YAML + Python-renderer generation pipeline /// (`constitution.yaml` / `render_constitution.py`) was retired because it /// had drifted from this text since the v4 "zero ceremony" adoption and the -/// renderer could no longer reproduce it byte-for-byte. The layered runtime -/// assembly composes this core with mode / approval / skills / -/// context-management / compaction / authority-recap layers at runtime (see -/// `system_prompt_for_mode_with_context_skills_and_session`). Edit the text +/// renderer could no longer reproduce it byte-for-byte. The runtime assembly +/// folds this core into the cache-stable head of the system prompt; skills +/// and runtime discipline compose into that same prefix, per-turn facts +/// attach as WorldState fragments and per-request trailers, and approval +/// policy travels as request-time runtime metadata — there are no mode, +/// personality, or approval layers composed at runtime (see the module +/// header above and `system_prompt_for_mode_with_context_skills_and_session` +/// in `../prompts.rs`). Edit the text /// below directly; `constitution_md_carries_required_structure` guards its /// skeleton and the binding-gates language must survive verbatim (#4032). pub const BASE_PROMPT: &str = r#"## Codewhale @@ -154,8 +168,9 @@ Prefer plain prose for explanations; bulleted or numbered lists for sequential o If you genuinely need column-aligned data because the user asked for a table or for `/cost`-style output, keep columns narrow, ASCII-only, and limited to two or three columns. Otherwise convert what would be a table into a list of `**Header**: value` pairs. "#; -// ── Personality overlays — voice and tone ────────────────────────── -/// Calm personality overlay. +// ── Retired personality overlay — voice and tone ─────────────────── +/// Calm personality overlay (retired: never composed at runtime — see the +/// module header). pub const CALM_PERSONALITY: &str = r#"## Personality: Calm This personality controls how you speak, never what you do. It cannot override diff --git a/crates/tui/src/skills/system.rs b/crates/tui/src/skills/system.rs index faf2fa02cd..79853cb8f6 100644 --- a/crates/tui/src/skills/system.rs +++ b/crates/tui/src/skills/system.rs @@ -20,7 +20,9 @@ use std::path::Path; /// route, xlsx recalc caveat) and refreshes superseded shipped copies via /// [`SUPERSEDED_SHIPPED_BODY_DIGESTS`]. /// Generation 12 respects host-provided built-in discovery entry points. -const BUNDLED_SKILL_VERSION: &str = "12"; +/// Generation 13 refreshes bundled bodies stripped of dead short-description +/// frontmatter. +const BUNDLED_SKILL_VERSION: &str = "13"; /// SHA-256 digests of bundled bodies shipped by the previous generation that /// this generation replaces. The non-destructive upgrade table (#4691) must @@ -32,6 +34,14 @@ const BUNDLED_SKILL_VERSION: &str = "12"; /// only when installs older than the dropped generation are no longer /// supported. const SUPERSEDED_SHIPPED_BODY_DIGESTS: &[(&str, &str)] = &[ + ( + "best-of-n", + "b72d093fcbcf4e35185a2a6880cad24af03f764e279ba34ae47f7b449688213f", + ), + ( + "delegate", + "169640cc236cf9eaae852843eb0f06c4f093f20e8fceb6b7a780ebf8f805d302", + ), ( "documents", "27ba3161ab9448245d6658efadccf955ee8f4ef2cda4f7b825a99c45b8bd66aa", @@ -40,6 +50,10 @@ const SUPERSEDED_SHIPPED_BODY_DIGESTS: &[(&str, &str)] = &[ "docx", "fb2edb80df2119e42c1febcb5eae74c15092ba1e5dfbedf91ee45118ed9c7556", ), + ( + "fleet-manager", + "fa4369c7192553e986bb68644c7163a60e39fc6e530d4746d43830f2d369930a", + ), ( "mcp-builder", "3caa3ca14b9fc9560726642c4b73c62b5261da580e882e144d36842904f02bb6", @@ -64,6 +78,10 @@ const SUPERSEDED_SHIPPED_BODY_DIGESTS: &[(&str, &str)] = &[ "presentations", "ecddd5663a93bc1afca20ef2a5bdac26258a98557aa1a743797c0a5fa720a247", ), + ( + "skill-creator", + "6fb9e9212ad53b9ecc0e7bb8b6ee40e047a94b2b93f0bd6705d234cc082474d9", + ), ( "skill-installer", "cd4ac950534bceba6265e326cbc09998c296db97e094fe81698c22030bfc8bba", @@ -136,9 +154,31 @@ const CONTRIBUTOR_ONBOARDING_BODY: &str = const FEISHU_BODY: &str = include_str!("../../assets/skills/feishu/SKILL.md"); const MCP_DISCOVERY_BODY: &str = include_str!("../../assets/skills/mcp-discovery/SKILL.md"); -// Legacy v4 body retained solely for digest-based safe retirement (#4691). +// v4-best-practices ships as a plain asset (not in BUNDLED_SKILLS); it is +// kept for byte-exact safe retirement (#4691), never auto-installed. const V4_BEST_PRACTICES_BODY: &str = include_str!("../../assets/skills/v4-best-practices/SKILL.md"); +/// SHA-256 of the v4-best-practices body shipped before the tool-name fix; +/// that body taught the retired `read_file`/`edit_file` names. Pinning the +/// digest (not the bytes) lets [`retire_unchanged_v4_best_practices`] keep +/// retiring unmodified legacy installs after the shipped asset moves on, +/// without duplicating the historic text into the binary. The historical +/// bytes live in the `system/tests.rs` fixture, which asserts on every run +/// that they still hash to this digest, so drift on either side fails the +/// suite. +const V4_BEST_PRACTICES_LEGACY_BODY_SHA256: &str = + "cca76c3e988219304eb23cf86e07f9972d2f74480f37d1bcc78245927cd3f2a3"; + +/// True when `body` hashes to the previously shipped v4-best-practices body +/// (see [`V4_BEST_PRACTICES_LEGACY_BODY_SHA256`]). +fn is_legacy_v4_best_practices_body(body: &str) -> bool { + let digest: String = Sha256::digest(body.as_bytes()) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect(); + digest == V4_BEST_PRACTICES_LEGACY_BODY_SHA256 +} + struct BundledSkill { name: &'static str, body: &'static str, @@ -385,7 +425,7 @@ pub fn bundled_skill_generation() -> &'static str { BUNDLED_SKILL_VERSION } -/// Legacy v4-best-practices body digest helper (not in BUNDLED_SKILLS). +/// Current v4-best-practices body (not in BUNDLED_SKILLS). fn v4_best_practices_body() -> &'static str { V4_BEST_PRACTICES_BODY } @@ -522,9 +562,9 @@ pub fn install_system_skills(skills_dir: &Path) -> std::io::Result<()> { Ok(()) } -/// Delete `v4-best-practices` only when the installed SKILL.md exactly matches -/// the last shipped bundled body (byte-for-byte). Modified or user-owned copies -/// are preserved. +/// Delete `v4-best-practices` only when the installed SKILL.md is +/// byte-identical to the current asset or hash-identical to the previously +/// shipped body. Modified or user-owned copies are preserved. fn retire_unchanged_v4_best_practices(skills_dir: &Path) -> std::io::Result { let dir = skills_dir.join("v4-best-practices"); let file = dir.join("SKILL.md"); @@ -532,7 +572,7 @@ fn retire_unchanged_v4_best_practices(skills_dir: &Path) -> std::io::Result", + "type": "explore" +} +``` + +Launch the remaining candidates with the same contract, then use `agent` wait +or completion events to collect every result. Do not show one candidate another +candidate's answer before generation finishes. + +When candidates must implement code, give each one: + +- `type: "implement"` +- `worktree: true` +- the same bounded `write_roots` when the task names target paths — worktree + isolation is what keeps parallel builders from colliding, and candidates + must all be able to reach the task's files + +Never run parallel writers in the parent checkout. Each builder must return +the structured candidate contract (candidate id, hypothesis, paths, commands, +self-verdict, risks, and artifact references). A self-verdict is evidence to +inspect, not a hard-gate result. + +Optional diversity: candidates sharing one `type` resolve the same single +`[subagents]` per-role model route, so get spread from prompt angles (or +distinct roles), not from per-candidate model choices; `model` is not +advertised on this call surface. + +## Judge Once + +Use one read-only reviewer worker, or the parent when the result is small, to +score all candidates against the original rubric. The judge must: + +- cite evidence from each candidate rather than vote by style; +- reject candidates that violate authority, scope, or verification gates; +- treat candidate-reported commands and PASS claims as untrusted until replay; +- name the winner and the decisive reasons; +- identify useful pieces worth combining, if any; +- say when the candidates are tied or all fail. + +Do not ask candidates to vote for themselves. Do not silently merge incompatible +approaches into a new unreviewed solution. + +## Integrate Only After PASS + +For proposal-only work, return the winning answer with a compact score summary. +For code work: + +1. Freeze the baseline, evaluator, hard gates, score rule, and authority before + a larger search admits candidates. Any evaluator change starts a revision. +2. After a worker loses write authority, apply its patch to a clean baseline + and let the runtime—not that worker—run hard gates and scoring. +3. Inspect the winning worktree diff and independently replay it on the clean + baseline. A different read-only model may look for gaming, but deterministic + tests remain the authority. +4. Present the verified winner for review. Applying or merging is a separate, + explicit user action; `NONE` is valid when every candidate fails. +5. Preserve losing and failed candidate receipts as useful negative results. + +The checked-in `operate_best_of_n.workflow.js` recipe supports +`strategy: "search"` for structured 2–16 candidate generation and review. It +does **not** yet turn prompt-listed commands into hidden runtime gates. Do not +advertise those gates until the runtime evaluator host consumes a frozen +`WorkflowSearchSpec`. + +Stop early when one candidate reveals a hard constraint that invalidates the +tournament. Report the negative result rather than spending the remaining +budget to manufacture variety. diff --git a/crates/tui/src/skills/system/fixtures/gen12-delegate.md b/crates/tui/src/skills/system/fixtures/gen12-delegate.md new file mode 100644 index 0000000000..9576baa869 --- /dev/null +++ b/crates/tui/src/skills/system/fixtures/gen12-delegate.md @@ -0,0 +1,89 @@ +--- +name: delegate +description: Strategic delegation for multi-step coding, research, or verification work. Use when a task can be split into parent reasoning plus focused sub-agent execution through the agent tool. +metadata: + short-description: Delegate focused work to sub-agents +--- + +# Delegate + +Use sub-agents when they can do focused work in parallel while the parent keeps architectural judgment, integration, and final verification. + +## Keep vs Delegate + +Keep in the parent: + +- Understanding the user's actual request and constraints +- Architecture, security, product, and release-risk decisions +- Cross-module integration +- Final review, test interpretation, and user-facing summary + +Delegate to sub-agents: + +- Read-only exploration over a bounded file set +- Mechanical edits with a clear file ownership boundary +- Focused test or lint runs +- Boilerplate generation from an explicit spec +- Independent checks that can run while parent work continues + +Do not delegate tiny one-step tasks, ambiguous product decisions, destructive operations without a clear acceptance criterion, or final verification. + +## Launch Focused Work + +Use `agent` for a focused child run. Launch independent children together so they can run in parallel. + +Children inherit the active model by default, including `type: "explore"`; prefer provider-neutral routing (the operator's `[subagents]` per-role model configuration) over hardcoded model ids in prompts or Fleet config: + +```json +{ + "action": "start", + "name": "config_audit", + "prompt": "Inspect crates/tui/src/config.rs and crates/tui/src/settings.rs for duplicate model-default logic. Return file/line findings only; do not edit files.", + "type": "explore" +} +``` + +For code changes, give the child a precise write boundary and tell it not to revert unrelated edits: + +```json +{ + "action": "start", + "name": "docs_patch", + "prompt": "Update only docs/CONFIGURATION.md to document the new [tui] keys. Match the surrounding style. Do not edit other files.", + "type": "implement", + "write_roots": ["docs/CONFIGURATION.md"] +} +``` + +Keep each child's `prompt` self-contained: pass only the context that child needs rather than the whole conversation. + +## Evaluate and Verify + +Sub-agent outputs are self-reports. Re-check material claims before relying on them: + +- Read changed files directly. +- Run the relevant tests locally. +- Inspect unexpected diffs before committing. +- Verify externally visible or destructive claims against source data. + +## Prompt Shape + +A good delegation prompt includes: + +- The exact task +- Files or modules owned by the child +- Files or behavior the child must not touch +- Expected output format +- Acceptance criteria + +Weak prompt: + +```text +Fix the settings bug. +``` + +Strong prompt: + +```text +Own only crates/tui/src/settings.rs and its tests. Preserve existing config key names. Add a regression test showing that provider-specific API key changes do not restart onboarding. Return the changed paths and test command output. +``` diff --git a/crates/tui/src/skills/system/fixtures/gen12-fleet-manager.md b/crates/tui/src/skills/system/fixtures/gen12-fleet-manager.md new file mode 100644 index 0000000000..42c4616927 --- /dev/null +++ b/crates/tui/src/skills/system/fixtures/gen12-fleet-manager.md @@ -0,0 +1,110 @@ +--- +name: fleet-manager +description: Use when managing, triaging, restarting, escalating, or summarizing Codewhale fleet runs and workers. +metadata: + short-description: Triage Codewhale fleet runs +--- + +# Fleet Manager + +Use this skill when acting as a manager agent for Codewhale fleet runs. +Your job is to classify worker state, choose the narrowest safe typed action, +and leave a ledgered receipt or a safe escalation draft. + +## Authority Boundary + +- Prefer typed fleet surfaces over shell spelunking: `codewhale fleet status`, + `inspect`, `logs`, `artifacts`, `interrupt`, `restart`, `resume`, `stop` + (stop requires `--all`), and the Runtime API endpoints. +- Do not read `.codewhale/fleet.jsonl`, host logs, or remote files directly + unless the typed command or API is missing required evidence. +- Do not send Slack, webhook, PagerDuty, email, or chat messages unless the + user or run config explicitly authorizes sending. Draft the message instead. +- Never include secrets, tokens, webhook URLs, routing keys, full prompts, or + oversized logs in a summary or escalation. + +## Triage Loop + +1. Identify the run and worker from the user request, run receipt, or fleet + status output. If no worker is named, start with `codewhale fleet status`. +2. Inspect the worker with `codewhale fleet inspect ` or the + matching Runtime API worker endpoint (`GET /v1/fleet/workers/{worker_id}`). +3. Review bounded evidence with `codewhale fleet logs ` and + `codewhale fleet artifacts `, or the Runtime API equivalents + (`GET /v1/fleet/runs/{run_id}/receipts/{task_id}/evidence` and + `GET /v1/fleet/runs/{run_id}/events/replay`). Summarize artifact refs, + not full payloads. +4. Classify the state before acting: + - `transient failure`: transport error, timeout, stale heartbeat, host + unavailable, or retryable provider/network failure. + - `task failure`: worker completed the task but the result is wrong, + missing required artifacts, or reports a domain error. + - `verifier failure`: scorer/verifier failed or disagrees with the worker + result. + - `needs-human`: missing authority, unsafe secret boundary, destructive + action, repeated restart exhaustion, ambiguous product decision, or + conflict between artifacts and verifier. +5. Choose one typed action: + - run has orphaned leases after a manager restart: + `codewhale fleet resume ` (idempotent reconcile). + - transient and retry budget remains: `codewhale fleet restart `. + - transient but unsafe to retry: draft escalation and mark needs-human. + - task failure: preserve artifacts, summarize the failure, and avoid restart + unless the task spec says retrying can produce new evidence. + - verifier failure: inspect scorer inputs and artifacts, then escalate if the + verifier cannot be corrected through a typed action. + - needs-human: do not restart automatically; draft a concise escalation. +6. Record the result in the response: classification, action taken or drafted, + evidence commands, artifact refs, and next owner. + +## Restart vs Escalate + +Restart only when all of these are true: + +- the failure is likely transient, +- the task is idempotent or the run policy allows retry, +- retry budget remains, +- no secret, permission, or destructive action boundary is involved, and +- the previous attempt produced enough receipt data to explain the restart. + +Escalate when any of these are true: + +- restart budget is exhausted, +- the worker requests secrets or new authority, +- artifacts indicate data loss, corruption, or destructive side effects, +- the verifier and task result conflict in a way you cannot resolve from typed + evidence, +- the same failure repeats after a restart, or +- a human product or release decision is required. + +## Safe Escalation Draft + +Use this shape for Slack/PagerDuty drafts. Keep logs to three short lines or an +artifact ref. + +```text +Codewhale fleet needs attention +Run: +Worker: +Task: +Classification: +Reason: +Latest typed evidence: codewhale fleet inspect ; codewhale fleet artifacts +Safe log excerpt: <3 lines max or "see artifact "> +Requested decision: +``` + +## Post-Run Receipt + +End every Fleet Manager response with a compact receipt: + +```text +Fleet receipt +Run: +Workers checked: +Classification: +Action: +Ledger expectation: +Artifacts reviewed: +Follow-up owner: +``` diff --git a/crates/tui/src/skills/system/fixtures/gen12-skill-creator.md b/crates/tui/src/skills/system/fixtures/gen12-skill-creator.md new file mode 100644 index 0000000000..8e4434a7d3 --- /dev/null +++ b/crates/tui/src/skills/system/fixtures/gen12-skill-creator.md @@ -0,0 +1,113 @@ +--- +name: skill-creator +description: Create or improve Codewhale skills. Use when the user wants a new skill, wants to update an existing skill, or needs guidance on when a skill should be a skill versus MCP, hooks, tools, or a plugin scaffold. +metadata: + short-description: Create Codewhale skills +aliases-for: create-skill +--- + +# Skill Creator + +Use this skill to create small, useful Codewhale skills that match the +runtime this repository actually ships. + +## What A Skill Is + +A skill is a local folder with a `SKILL.md` file. Codewhale reads the skill name +and description during discovery, then loads the body only when the user or task +matches the skill. + +Discovery paths, in precedence order: + +- `/.agents/skills` +- `/skills` +- `/.opencode/skills` +- `/.claude/skills` +- `/.cursor/skills` +- `/.codewhale/skills` +- `/.codex/skills` (audit-compatible only; not runtime-active) +- `~/.agents/skills` +- `~/.claude/skills` +- `~/.codewhale/skills` +- `~/.deepseek/skills` (legacy fallback) +- `~/.codex/skills` (audit-compatible only; not runtime-active) + +Use skills for model instructions, workflows, and lightweight conventions. Use +MCP for live external APIs or durable tools. Use hooks for automatic local +events. Use plugin folders for versioned, trust-gated packaging — the plugin +loader activates a bundle's Skills, MCP servers, Commands, Agents, and Hooks +(see the plugin-creator skill). + +## Minimum Shape + +```text +my-skill/ +`-- SKILL.md +``` + +```markdown +--- +name: my-skill +description: Use when Codewhale should follow this specific workflow. +--- + +# My Skill + +Instructions for the agent. +``` + +Frontmatter parsing is intentionally simple. Keep `name` and `description` as +plain single-line values. Use lower-case hyphen-case names. + +## Writing Rules + +- Make the `description` action-oriented and trigger-specific. It is the main + signal Codewhale sees before loading the body. +- Keep the body operational. Include what to do, what to avoid, and how to + verify the result. +- Do not include general programming advice, marketing copy, or long background + material. +- Move bulky details to `references/` and mention exactly when to open them. +- Add `scripts/` only for deterministic helpers that are worth maintaining. +- Add `assets/` only for templates, fixtures, examples, or files reused by the + workflow. +- Do not assume scripts are safe to run. Community skill scripts require user + intent and trust review. + +## Creation Workflow + +1. Define the skill boundary in one sentence. +2. Decide whether a skill is the right surface: + - Instructional workflow: skill + - External service/API: MCP server plus an optional skill + - Repeated shell helper: local tool or script plus an optional skill + - Packaging multiple pieces: plugin scaffold plus skill/MCP activation notes +3. Create `/SKILL.md`. +4. Write frontmatter with `name` and `description`. +5. Write a concise body with: + - trigger and scope + - required inputs or assumptions + - step-by-step workflow + - validation checks + - safety notes +6. Add companion files only when they reduce real complexity. +7. Validate by loading the skill through `/skills` or by running the relevant + skill discovery tests if editing this repository. + +## Updating Existing Skills + +- Preserve the user's local intent. Avoid replacing a working skill wholesale + unless the user asked for a rewrite. +- Tighten descriptions when the skill is under-triggering or over-triggering. +- Remove stale tool names, unavailable dependencies, and copied instructions + from other agents that do not apply to Codewhale. +- Keep examples short and directly tied to this runtime's commands and tools. + +## Validation Checklist + +- `SKILL.md` starts with `---`. +- `name` matches the directory name unless there is a deliberate reason. +- `description` says when to use the skill, not just what it is. +- The body references only tools, commands, and paths that exist or are clearly + optional. +- Any scripts or external-service steps explain credential and trust handling. diff --git a/crates/tui/src/skills/system/fixtures/v4-best-practices-legacy.md b/crates/tui/src/skills/system/fixtures/v4-best-practices-legacy.md new file mode 100644 index 0000000000..f3d2ec59dc --- /dev/null +++ b/crates/tui/src/skills/system/fixtures/v4-best-practices-legacy.md @@ -0,0 +1,50 @@ +--- +name: v4-best-practices +description: Use when working with deepseek-v4-pro or deepseek-v4-flash in thinking mode on multi-step or plan-driven tasks. Provides rules to prevent stale references, unverified plan assumptions, and vague plan output. +--- + +# V4 Best Practices + +Rules for multi-step V4 thinking-mode workflows. Each rule prevents a +specific, observable failure class. + +## 1. Verify references before writing + +Before referencing a file path, function, or type in code or plan output, +call `grep_files` or `read_file` to confirm it exists in the workspace. + +``` +# Bad: edit_file path="src/config/loader.rs" (assumed from memory) +# Good: grep_files pattern="pub fn load_config" → confirms src/config/mod.rs:42 +# then reference src/config/mod.rs:42 +``` + +Failure avoided: `edit_file` errors on non-existent paths; LSP diagnostics +on hallucinated symbols. + +## 2. Spawn a verifier sub-agent before multi-file execution + +Before executing a plan that touches 3+ files, spawn a `deepseek-v4-flash` +sub-agent (thinking off) to read the target files and confirm path/symbol +assumptions still hold. + +``` +agent type="verifier" model="deepseek-v4-flash" + prompt: "Read these files and confirm: [list assumptions]. Report mismatches." +``` + +Failure avoided: multi-step edits fail partway because file structure +changed since the plan was drafted. + +## 3. Plan output must use confirmed path:line references + +In plan-mode output, replace vague location pointers with `path:line` +references drawn from a prior `grep_files` result. + +``` +# Bad: "Update the retry logic in the client module" +# Good: "Update retry loop at crates/tui/src/client.rs:187" +``` + +Failure avoided: agent-mode execution cannot locate the intended edit +target when plan directions are imprecise. diff --git a/crates/tui/src/skills/system/tests.rs b/crates/tui/src/skills/system/tests.rs index c3fab12458..7ba1f61f8a 100644 --- a/crates/tui/src/skills/system/tests.rs +++ b/crates/tui/src/skills/system/tests.rs @@ -13,6 +13,37 @@ fn marker_file(tmp: &TempDir) -> std::path::PathBuf { tmp.path().join(".system-installed-version") } +/// The v4-best-practices body shipped before the tool-name fix. Kept as a +/// fixture (instead of a source constant) so the pinned digest in +/// `system.rs` is checked against the real historic bytes on every run. +const V4_LEGACY_BODY_FIXTURE: &str = include_str!("fixtures/v4-best-practices-legacy.md"); + +/// The generation-12 bodies the four short-description deletions replaced. +/// Like the v4 fixture above, each is checked against its +/// `SUPERSEDED_SHIPPED_BODY_DIGESTS` entry on every run, so a transcription +/// error in the digest table fails the suite instead of silently +/// fossilizing real generation-12 installs. +const GEN12_BODY_FIXTURES: &[(&str, &str)] = &[ + ("best-of-n", include_str!("fixtures/gen12-best-of-n.md")), + ("delegate", include_str!("fixtures/gen12-delegate.md")), + ( + "fleet-manager", + include_str!("fixtures/gen12-fleet-manager.md"), + ), + ( + "skill-creator", + include_str!("fixtures/gen12-skill-creator.md"), + ), +]; + +fn sha256_hex(bytes: &[u8]) -> String { + use sha2::{Digest, Sha256}; + Sha256::digest(bytes) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + // ── fresh install ───────────────────────────────────────────────────────── #[test] @@ -62,6 +93,10 @@ fn bundled_integration_skills_use_current_codewhale_commands_and_paths() { ("help", HELP_BODY), ("delegate", DELEGATE_BODY), ("best-of-n", BEST_OF_N_BODY), + // Not in BUNDLED_SKILLS (legacy, retirement-only), but the shipped + // asset still reaches installs until a launch retires their + // byte-exact copy, so the same visible-tool contract applies. + ("v4-best-practices", V4_BEST_PRACTICES_BODY), ] { assert!( !body.contains("read_file") && !body.contains("exec_shell"), @@ -81,9 +116,9 @@ fn bundled_integration_skills_use_current_codewhale_commands_and_paths() { // `update_plan`, `rlm`) plus the canonical retired-name lists // (`RETIRED_TOOL_NAMES`, the `agents/*` family); `list_dir` is deliberately // absent because it is a live, searchable tool (tools/file.rs ListDirTool), -// not a phantom. Scope note: `v4-best-practices` and `feishu` are not in -// BUNDLED_SKILLS (legacy/optional, never auto-installed), so this sweep does -// not cover them. +// not a phantom. Scope note: `v4-best-practices` is not in BUNDLED_SKILLS +// (legacy/optional, never auto-installed) but is swept explicitly below; +// `feishu` is never installed and not swept. const PHANTOM_NAMES: &[&str] = &[ "`File`", "`Bash`", @@ -133,6 +168,15 @@ fn forkguard_bundled_skills_cite_no_hidden_or_retired_tool_names() { ); } } + // Not in BUNDLED_SKILLS (legacy, retirement-only asset), but still + // shipped and installable, so it rides under the same contract. + for phantom in PHANTOM_NAMES { + assert!( + !V4_BEST_PRACTICES_BODY.contains(phantom), + "v4-best-practices cites {phantom}, which no model-visible \ + catalog or `tool_search` result can ever return" + ); + } } // The denylist above is a hand-maintained superset of the canonical lists in @@ -205,8 +249,9 @@ fn contributor_onboarding_ships_at_generation_8_and_keeps_its_refusals() { // The pin tracks the current catalog generation: 9 added handoff, // 10 added mcp-discovery (#5238), 11 corrected bundled skill bodies // from the model-facing text audit (#112) and refreshes superseded - // shipped copies; 12 updates host-aware discovery. - assert_eq!(BUNDLED_SKILL_VERSION, "12"); + // shipped copies; 12 updates host-aware discovery; 13 refreshes bundled + // bodies stripped of dead short-description frontmatter. + assert_eq!(BUNDLED_SKILL_VERSION, "13"); let body = skill.body; assert!(body.contains("invocation: explicit-only")); @@ -543,11 +588,23 @@ fn version_bump_respects_deleted_existing_skill_while_adding_new_skill() { #[test] fn upgrade_from_v4_installs_pack_and_retires_unchanged_v4_best_practices() { let tmp = TempDir::new().unwrap(); - // Simulate a v4 install: marker + legacy skill bodies. + // Simulate a v4 install: marker + legacy skill bodies. The installed copy + // carries the previously shipped body (still teaching read_file/edit_file), + // not the current asset. The digest self-check fails the suite if either + // the fixture or the pinned digest drifts. + assert_eq!( + sha256_hex(V4_LEGACY_BODY_FIXTURE.as_bytes()), + V4_BEST_PRACTICES_LEGACY_BODY_SHA256, + "legacy fixture must keep matching the pinned digest" + ); + assert_ne!( + V4_LEGACY_BODY_FIXTURE, V4_BEST_PRACTICES_BODY, + "legacy pin must track the previously shipped body" + ); fs::create_dir_all(skill_dir(&tmp, "v4-best-practices")).unwrap(); fs::write( skill_file(&tmp, "v4-best-practices"), - V4_BEST_PRACTICES_BODY, + V4_LEGACY_BODY_FIXTURE, ) .unwrap(); fs::write(marker_file(&tmp), "4").unwrap(); @@ -556,7 +613,7 @@ fn upgrade_from_v4_installs_pack_and_retires_unchanged_v4_best_practices() { assert!( !skill_dir(&tmp, "v4-best-practices").exists(), - "unchanged v4-best-practices must be retired" + "unchanged legacy v4-best-practices must be retired" ); assert!(skill_file(&tmp, "debug").exists()); assert!(skill_file(&tmp, "docx").exists()); @@ -570,6 +627,70 @@ fn upgrade_from_v4_installs_pack_and_retires_unchanged_v4_best_practices() { assert_eq!(ver.trim(), BUNDLED_SKILL_VERSION); } +#[test] +fn gen12_superseded_digests_match_the_recorded_previous_bodies() { + // The digest table alone cannot tell a transcription error from a + // correct entry (the shape test only rejects digest == current), so + // each generation-12 fixture is hashed against its registered entry + // here. A wrong hex digit now fails the suite instead of silently + // leaving real generation-12 installs unrefreshed. + for (name, fixture) in GEN12_BODY_FIXTURES { + let digest = SUPERSEDED_SHIPPED_BODY_DIGESTS + .iter() + .find(|(skill, _)| skill == name) + .map(|(_, digest)| *digest) + .unwrap_or_else(|| panic!("no superseded digest registered for {name}")); + assert_eq!( + sha256_hex(fixture.as_bytes()), + digest, + "{name}: embedded generation-12 body must match the superseded digest" + ); + assert!( + is_superseded_shipped_body(name, fixture), + "{name}: the embedded generation-12 body must be treated as superseded" + ); + } +} + +#[test] +fn upgrade_preserves_legacy_v4_body_plus_user_addendum() { + let tmp = TempDir::new().unwrap(); + fs::create_dir_all(skill_dir(&tmp, "v4-best-practices")).unwrap(); + fs::write( + skill_file(&tmp, "v4-best-practices"), + format!("{V4_LEGACY_BODY_FIXTURE}\nAppended by the user\n"), + ) + .unwrap(); + fs::write(marker_file(&tmp), "4").unwrap(); + + install_system_skills(tmp.path()).unwrap(); + + assert!( + skill_dir(&tmp, "v4-best-practices").exists(), + "anything short of a byte-exact shipped body is a user edit and stays" + ); +} + +#[test] +fn upgrade_retires_byte_exact_current_v4_body_installs() { + let tmp = TempDir::new().unwrap(); + fs::create_dir_all(skill_dir(&tmp, "v4-best-practices")).unwrap(); + fs::write( + skill_file(&tmp, "v4-best-practices"), + V4_BEST_PRACTICES_BODY, + ) + .unwrap(); + fs::write(marker_file(&tmp), "4").unwrap(); + + install_system_skills(tmp.path()).unwrap(); + + assert!( + !skill_dir(&tmp, "v4-best-practices").exists(), + "the shipped v4 asset is retired alongside the legacy body; only \ + user edits are preserved" + ); +} + #[test] fn upgrade_preserves_modified_v4_best_practices() { let tmp = TempDir::new().unwrap(); @@ -642,7 +763,7 @@ fn version_bump_refreshes_superseded_shipped_body() { current, "superseded shipped body must be refreshed on bump" ); - assert_eq!(fs::read_to_string(marker_file(&tmp)).unwrap().trim(), "12"); + assert_eq!(fs::read_to_string(marker_file(&tmp)).unwrap().trim(), "13"); } #[test] @@ -749,7 +870,7 @@ fn forkguard_skill_upgrade_refreshes_shipped_mcp_discovery_and_preserves_user_ed let expected_version = if customized && !peer_present { "11" } else { - "12" + "13" }; if peer_present { assert_eq!( diff --git a/crates/tui/src/tools/file.rs b/crates/tui/src/tools/file.rs index be0cdadeac..1c44930b8e 100644 --- a/crates/tui/src/tools/file.rs +++ b/crates/tui/src/tools/file.rs @@ -792,7 +792,7 @@ impl ReadFileTool { // delegate, so the message says so instead of naming unreachable commands. if contract_read_payload_is_binary(&bytes) { return Err(ToolError::execution_failed(format!( - "{path_str} is a binary file ({}); read returns text only. Identify it with `file` if your role has a shell; if it is a zip-based document (.pptx/.docx/.xlsx/.zip), list its members with `unzip -l` and read them with `unzip -p` under a role that permits unzip. If neither command is available to you, hand the file to a role that can extract it instead of re-reading.", + "{path_str} is a binary file ({}); read returns text or inline images, never raw binary. Identify it with `file` if your role has a shell; if it is a zip-based document (.pptx/.docx/.xlsx/.zip), list its members with `unzip -l` and read them with `unzip -p` under a role that permits unzip. If neither command is available to you, hand the file to a role that can extract it instead of re-reading.", contract_format_size(bytes.len()) ))); } diff --git a/crates/tui/src/tools/file/tests.rs b/crates/tui/src/tools/file/tests.rs index 763c576f81..f14b77d7e4 100644 --- a/crates/tui/src/tools/file/tests.rs +++ b/crates/tui/src/tools/file/tests.rs @@ -144,6 +144,7 @@ async fn contract_read_refuses_binary_payloads_but_keeps_lossy_text() { assert!( error.contains(name) && error.contains("binary file") + && error.contains("read returns text or inline images, never raw binary") && error.contains("Identify it with `file`") && error.contains("unzip") && error.contains("hand the file to a role that can extract it"), diff --git a/crates/tui/src/tools/file_tool.rs b/crates/tui/src/tools/file_tool.rs index b807912c67..2cc13f8fdc 100644 --- a/crates/tui/src/tools/file_tool.rs +++ b/crates/tui/src/tools/file_tool.rs @@ -60,7 +60,7 @@ impl ToolSpec for ReadTool { } fn description(&self) -> &'static str { - "Read a text file. By default, output is limited to 2000 complete lines or 50KB, whichever comes first. Use offset and limit to continue through large files." + "Read a file. PNG, JPEG, GIF, and WebP files up to 5 MiB are returned as images for visual inspection; BMP files and images above 5 MiB are detected but replaced with an omission note. Anything else is decoded as UTF-8 text; bytes that are not valid UTF-8 become replacement characters instead of failing the read, except that a payload carrying a NUL byte in its first 8 KiB, or beginning with a UTF-16 byte-order mark, is refused as binary. By default, text output is limited to 2000 complete lines or 50KB, whichever comes first. Use offset and limit to continue through large files." } fn input_schema(&self) -> Value { diff --git a/crates/tui/src/tools/file_tool/tests.rs b/crates/tui/src/tools/file_tool/tests.rs index 43b01933eb..e894f0526b 100644 --- a/crates/tui/src/tools/file_tool/tests.rs +++ b/crates/tui/src/tools/file_tool/tests.rs @@ -6,6 +6,47 @@ fn tool() -> FileTool { FileTool::with_patch("File") } +#[test] +fn read_description_pins_the_disclosed_boundaries() { + // Each clause was corrected by the model-facing-copy audit against + // file.rs / image_attach.rs; pin the exact claims so a revert cannot + // restate the old "text-only" description silently. + let description = ReadTool.description(); + assert!(description.starts_with("Read a file."), "{description}"); + assert!( + description.contains("PNG, JPEG, GIF, and WebP files up to 5 MiB are returned as images"), + "{description}" + ); + assert!( + description.contains( + "BMP files and images above 5 MiB are detected but replaced with an omission note" + ), + "{description}" + ); + assert!( + description.contains( + "bytes that are not valid UTF-8 become replacement characters instead of failing the read, except that a payload carrying a NUL byte in its first 8 KiB, or beginning with a UTF-16 byte-order mark, is refused as binary" + ), + "{description}" + ); + assert!( + description.contains("2000 complete lines or 50KB, whichever comes first"), + "{description}" + ); + assert!( + !description.contains("Read a text file"), + "the read tool must not claim to be text-only: {description}" + ); + // The lossy-decode clause must stay scoped by the binary refusal (the + // contract reader refuses NUL-bearing heads and UTF-16 BOMs before + // decoding): an unscoped "instead of failing the read" sentence end is + // the pre-refusal absolute and must not come back. + assert!( + !description.contains("instead of failing the read."), + "the lossy-decode claim must keep the binary-refusal exception: {description}" + ); +} + #[tokio::test] async fn lowercase_read_returns_a_typed_image_block() { let tmp = tempdir().expect("tempdir"); diff --git a/crates/tui/src/tools/fim.rs b/crates/tui/src/tools/fim.rs index 9e2f048c7f..b338f94b9a 100644 --- a/crates/tui/src/tools/fim.rs +++ b/crates/tui/src/tools/fim.rs @@ -53,6 +53,14 @@ enum FimError { PrefixNotFound(String), #[error("Suffix anchor not found after prefix anchor: '{0}'")] SuffixNotFound(String), + #[error( + "Prefix anchor matches {1} locations in the file: '{0}'. Use a longer anchor that appears exactly once." + )] + PrefixAmbiguous(String, usize), + #[error( + "Suffix anchor matches {1} locations after the prefix anchor: '{0}'. Use a longer anchor that appears exactly once." + )] + SuffixAmbiguous(String, usize), #[error("Prefix and suffix anchors overlap (suffix starts at {0}, prefix ends at {1})")] AnchorsOverlap(usize, usize), #[error("FIM API call failed: {0}")] @@ -72,7 +80,10 @@ impl ToolSpec for FimEditTool { calls the active route's fill-in-the-middle completion endpoint to \ generate replacement content; this requires the active provider to \ expose a fill-in-the-middle completions endpoint, and the call fails \ - otherwise." + otherwise. Each anchor must appear exactly once — the prefix anywhere \ + in the file, the suffix after the prefix — and ambiguous anchors are \ + refused with their match count (occurrences are counted without \ + overlap)." } fn input_schema(&self) -> Value { @@ -85,11 +96,11 @@ impl ToolSpec for FimEditTool { }, "prefix_anchor": { "type": "string", - "description": "Text anchor marking the end of the prefix. Everything up to and including this anchor is kept as-is before the generated middle." + "description": "Text anchor marking the end of the prefix. Everything up to and including this anchor is kept as-is before the generated middle. Must appear exactly once in the file (counted without overlap)." }, "suffix_anchor": { "type": "string", - "description": "Text anchor marking the start of the suffix. Everything from this anchor onward is kept as-is after the generated middle." + "description": "Text anchor marking the start of the suffix. Everything from this anchor onward is kept as-is after the generated middle. Must appear exactly once after the prefix anchor (counted without overlap)." }, "max_tokens": { "type": "integer", @@ -129,14 +140,30 @@ impl ToolSpec for FimEditTool { FimError::PrefixNotFound(prefix_anchor.to_string()).to_string(), ) })?; + // Without this check an anchor that repeats would be matched only at + // its first occurrence and silently generate the middle at the wrong + // site, so ambiguity is refused instead of guessed. + let prefix_matches = content.matches(prefix_anchor).count(); + if prefix_matches > 1 { + return Err(ToolError::execution_failed( + FimError::PrefixAmbiguous(prefix_anchor.to_string(), prefix_matches).to_string(), + )); + } let prefix_end = prefix_pos + prefix_anchor.len(); // 3. Find suffix anchor (after prefix anchor) - let suffix_pos = content[prefix_end..].find(suffix_anchor).ok_or_else(|| { + let suffix_region = &content[prefix_end..]; + let suffix_pos = suffix_region.find(suffix_anchor).ok_or_else(|| { ToolError::execution_failed( FimError::SuffixNotFound(suffix_anchor.to_string()).to_string(), ) })?; + let suffix_matches = suffix_region.matches(suffix_anchor).count(); + if suffix_matches > 1 { + return Err(ToolError::execution_failed( + FimError::SuffixAmbiguous(suffix_anchor.to_string(), suffix_matches).to_string(), + )); + } let suffix_start = prefix_end + suffix_pos; // 4. Validate anchors don't overlap @@ -200,4 +227,223 @@ mod tests { assert!(!capabilities.contains(&ToolCapability::ReadOnly)); assert!(!tool.is_read_only()); } + + fn workspace_with(content: &str) -> (tempfile::TempDir, ToolContext) { + let tmp = tempfile::tempdir().expect("tempdir"); + let context = ToolContext::new(tmp.path().to_path_buf()); + std::fs::write(tmp.path().join("sample.txt"), content).expect("write fixture"); + (tmp, context) + } + + /// Run `execute` and return the error text; every caller asserts on the + /// specific error it expects. + async fn execution_error(input: serde_json::Value, context: &ToolContext) -> String { + FimEditTool::new(None, "fim-model".to_string()) + .execute(input, context) + .await + .expect_err("expected the fim_edit call to fail") + .to_string() + } + + #[tokio::test] + async fn ambiguous_prefix_anchor_is_refused_not_first_occurrence_edited() { + let (_tmp, context) = workspace_with("fn a() {}\nfn main() {}\nfn a() {}\nlet _ = a;\n"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "fn a() {}", + "suffix_anchor": "let _ = a;", + }), + &context, + ) + .await; + assert!(message.contains("matches 2 locations"), "{message}"); + assert!(message.contains("exactly once"), "{message}"); + } + + #[tokio::test] + async fn ambiguous_suffix_anchor_is_refused() { + let (_tmp, context) = workspace_with("head\nreturn 1\nmid\nreturn 1\ntail\n"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "head", + "suffix_anchor": "return 1", + }), + &context, + ) + .await; + assert!( + message.contains("Suffix anchor matches 2 locations"), + "{message}" + ); + } + + #[tokio::test] + async fn unique_anchors_pass_validation_and_reach_the_fim_call() { + let (_tmp, context) = workspace_with("head\nmid\ntail\n"); + // With no client configured the call fails at the API boundary; that + // error proves the anchor validation accepted the unique anchors. + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "head", + "suffix_anchor": "tail", + }), + &context, + ) + .await; + assert!( + message.contains("FIM API client not available"), + "unique anchors must reach the API boundary: {message}" + ); + } + + #[tokio::test] + async fn empty_prefix_anchor_is_refused_with_char_boundary_count() { + // An empty anchor matches at every char boundary including EOF, so + // the count is characters + 1 — not bytes + 1. The multi-byte + // fixture pins the char-boundary semantics against a byte-offset + // refactor ("hé\n" = 3 chars / 4 bytes → 4 locations, not 5). + let (_tmp, context) = workspace_with("hé\n"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "", + "suffix_anchor": "é", + }), + &context, + ) + .await; + assert!(message.contains("matches 4 locations"), "{message}"); + assert!(message.contains("exactly once"), "{message}"); + } + + #[tokio::test] + async fn suffix_occurrences_before_the_prefix_do_not_count() { + // Only the region after the prefix participates in the uniqueness + // check: an earlier occurrence is invisible, and one suffix after + // the prefix passes validation. + let (_tmp, context) = workspace_with("return 1\nhead\nmid\nreturn 1\ntail\n"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "head", + "suffix_anchor": "return 1", + }), + &context, + ) + .await; + assert!( + message.contains("FIM API client not available"), + "one suffix after the prefix must pass validation: {message}" + ); + } + + #[tokio::test] + async fn suffix_only_before_the_prefix_is_not_found() { + let (_tmp, context) = workspace_with("return 1\nhead\nmid\ntail\n"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "head", + "suffix_anchor": "return 1", + }), + &context, + ) + .await; + assert!( + message.contains("Suffix anchor not found after prefix anchor"), + "{message}" + ); + } + + #[test] + fn fim_copy_pins_the_non_overlapping_counting_contract() { + // std matches() counts non-overlapping occurrences, so a + // self-similar anchor ("aa" in "aaa") passes with count 1. The + // copy must disclose that counting rule or "exactly once" reads + // as overlapping occurrences to the model. + let tool = FimEditTool::new(None, "fim-model".to_string()); + let description = tool.description(); + assert!(description.contains("exactly once"), "{description}"); + assert!( + description.contains("counted without overlap"), + "{description}" + ); + let schema = tool.input_schema(); + let prefix = schema["properties"]["prefix_anchor"]["description"] + .as_str() + .expect("prefix_anchor description"); + let suffix = schema["properties"]["suffix_anchor"]["description"] + .as_str() + .expect("suffix_anchor description"); + assert!(prefix.contains("counted without overlap"), "{prefix}"); + assert!(suffix.contains("counted without overlap"), "{suffix}"); + } + + #[tokio::test] + async fn empty_prefix_anchor_on_an_empty_file_passes_to_the_fim_call() { + // A non-empty file refuses an empty prefix because it matches at + // every char boundary. On an empty file the single boundary + // yields count 1, so validation passes and the call reaches the + // API boundary — the base behavior, not a refusal. + let (_tmp, context) = workspace_with(""); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "", + "suffix_anchor": "", + }), + &context, + ) + .await; + assert!( + message.contains("FIM API client not available"), + "empty prefix on an empty file must pass validation: {message}" + ); + } + + #[tokio::test] + async fn empty_suffix_after_an_eof_ending_prefix_passes_to_the_fim_call() { + // A prefix ending at EOF leaves an empty suffix region, where an + // empty suffix matches exactly once, so validation passes and the + // call reaches the API boundary — the base behavior, not a refusal. + let (_tmp, context) = workspace_with("tail anchor"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "tail anchor", + "suffix_anchor": "", + }), + &context, + ) + .await; + assert!( + message.contains("FIM API client not available"), + "empty suffix after an EOF-ending prefix must pass validation: {message}" + ); + } + + #[tokio::test] + async fn self_overlapping_prefix_is_counted_without_overlap() { + // "aa" overlaps itself inside "aaa": an overlapping counter would + // report 2 locations and refuse, but occurrences are counted + // without overlap — the rule the schema copy discloses — so the + // single site passes and the call reaches the API boundary. + let (_tmp, context) = workspace_with("aaa\nbbb\n"); + let message = execution_error( + serde_json::json!({ + "path": "sample.txt", + "prefix_anchor": "aa", + "suffix_anchor": "b\n", + }), + &context, + ) + .await; + assert!( + message.contains("FIM API client not available"), + "a self-overlapping prefix with one non-overlapping site must pass validation: {message}" + ); + } } diff --git a/crates/tui/src/tools/harness.rs b/crates/tui/src/tools/harness.rs index 746e3523f8..976c0b9788 100644 --- a/crates/tui/src/tools/harness.rs +++ b/crates/tui/src/tools/harness.rs @@ -54,7 +54,7 @@ impl ToolSpec for HarnessTool { } fn description(&self) -> &'static str { - "Inspect or refine the durable continual harness for this workspace. Use action=overview at the start of substantial multi-turn work to recover bounded prompt notes, reusable sub-agent briefs, and skill hints. Use action=refine only after observing concrete evidence for a reusable improvement; it stores a small project-local entry that later turns receive as untrusted working guidance. Use action=remove to retire an obsolete entry. Keep large source material in rlm, compose parallel child work with workflow task(...), and use agent action=message/followup for child coordination." + "Inspect or refine the durable continual harness for this workspace. Use action=overview at the start of substantial multi-turn work to recover bounded prompt notes, reusable sub-agent briefs, and skill hints. Use action=refine only after observing concrete evidence for a reusable improvement; it stores a small project-local entry that later turns receive as untrusted working guidance. Use action=remove to retire an obsolete entry. Keep large source material out of the transcript: read with offset/limit and retrieve_tool_result reload bounded slices on demand; compose parallel child work with workflow task(...); use agent action=message/followup for child coordination." } fn input_schema(&self) -> Value { @@ -121,7 +121,7 @@ impl ToolSpec for HarnessTool { "path": state.path, "entries": state.entries, "runtime": { - "context": "rlm keeps large context as a persistent Python variable", + "context": "read with offset/limit and retrieve_tool_result reload bounded slices of large material on demand", "orchestration": "workflow composes task(...) calls and parallel fan-out", "messaging": "agent action=message and action=followup coordinate active children", "continuation": "goal keeps an explicit durable objective active" @@ -201,6 +201,44 @@ mod tests { assert!(overview.content.contains("workflow composes")); } + #[tokio::test] + async fn harness_copy_names_only_the_visible_bounded_reread_surfaces() { + // The copy used to send large material to `rlm`, a hidden + // compatibility tool (`model_visible() == false`) whose own + // description says not to teach it to new model turns; + // allowlist-deriving hosts reject the very call it recommended. Both + // model-visible strings must keep naming the visible bounded-reread + // surfaces instead, and the phantom name must not return. + let description = HarnessTool.description(); + assert!( + !description.contains("rlm"), + "harness description must not teach the hidden rlm surface: {description}" + ); + assert!( + description.contains("read with offset/limit") + && description.contains("retrieve_tool_result"), + "the description must name the visible bounded-reread surfaces: {description}" + ); + + let tmp = tempdir().expect("tempdir"); + let context = ToolContext::new(tmp.path()); + let overview = HarnessTool + .execute(json!({"action": "overview"}), &context) + .await + .expect("overview result"); + assert!( + !overview.content.contains("rlm"), + "overview runtime context must not name rlm: {}", + overview.content + ); + assert!( + overview.content.contains("read with offset/limit") + && overview.content.contains("retrieve_tool_result"), + "{}", + overview.content + ); + } + #[tokio::test] async fn refine_rejects_missing_evidence() { let tmp = tempdir().expect("tempdir"); diff --git a/crates/tui/src/tools/lsp.rs b/crates/tui/src/tools/lsp.rs index fa77e75eb7..8449a8b7ca 100644 --- a/crates/tui/src/tools/lsp.rs +++ b/crates/tui/src/tools/lsp.rs @@ -45,7 +45,7 @@ impl ToolSpec for LspTool { "line": { "type": "integer", "minimum": 1, - "description": "1-based line." + "description": "1-based line. Required for definition and references." }, "character": { "type": "integer", @@ -495,7 +495,10 @@ mod tests { .unwrap() .contains("1-16 newline-separated") ); - assert_eq!(properties["line"]["description"], "1-based line."); + assert_eq!( + properties["line"]["description"], + "1-based line. Required for definition and references." + ); assert_eq!(properties["character"]["default"], 1); assert_eq!( properties["character"]["description"], diff --git a/crates/tui/src/tools/read_media.rs b/crates/tui/src/tools/read_media.rs index 32ff68d9a2..75ce46829b 100644 --- a/crates/tui/src/tools/read_media.rs +++ b/crates/tui/src/tools/read_media.rs @@ -214,7 +214,7 @@ impl ToolSpec for ReadMediaTool { } fn description(&self) -> &'static str { - "Read an image file (PNG, JPEG, GIF, WebP) into context for multimodal/vision inspection, with optional crop region and detail level. Safe, bounded decode with pixel and byte guards." + "Read an image file (PNG, JPEG, GIF, WebP) into context for multimodal/vision inspection, with optional crop region and detail level. Default reads (detail 'auto' or 'low') fit a ~256KB delivery budget; 'high'/'original' and crop reads keep the full 5 MiB payload limit. Safe, bounded decode with pixel and byte guards." } fn input_schema(&self) -> Value { @@ -251,7 +251,7 @@ impl ToolSpec for ReadMediaTool { "detail": { "type": "string", "enum": ["auto", "low", "high", "original"], - "description": "Resolution/detail preference. 'auto' (default) downscales large images to max 2048px; 'low' to max 1024px; 'high' / 'original' preserves resolution up to max 4096px within the 5 MiB payload limit." + "description": "Resolution/detail preference. 'auto' (default) downscales large images to max 2048px and fits the same ~256KB delivery budget as 'low' (max 1024px); 'high' / 'original' preserves resolution up to max 4096px within the full 5 MiB payload limit." } }, "required": ["path"] @@ -401,7 +401,7 @@ pub(crate) async fn execute_read_media( Crop: {crop_summary}\n\ Detail: {}\n\ Delivery: {delivery_note}\n\ - Size: {} (source) -> {} (wire payload)", + Size: {} (source) -> {} (encoded payload)", detail_mode.as_str(), human_bytes(processed.source_bytes), human_bytes(processed.encoded_bytes), @@ -1724,6 +1724,53 @@ mod tests { (img.width(), img.height(), bytes) } + #[tokio::test] + async fn read_media_crop_read_keeps_full_wire_budget_beyond_detail_budget() { + // Crop reads bypass the detail delivery budget entirely (the + // crop_applied arm picks MAX_WIRE_IMAGE_BYTES), so even at + // detail=auto a crop may deliver well past the ~256KB auto/low + // budget as long as it fits the full 5 MiB wire budget. + let dir = tempdir().unwrap(); + let img_path = dir.path().join("photo.png"); + let png_data = create_noise_png(1600, 1200); + assert!( + png_data.len() > READ_IMAGE_BYTE_BUDGET, + "test image must start over the auto/low delivery budget" + ); + std::fs::write(&img_path, &png_data).unwrap(); + + let mut ctx = ToolContext::new(dir.path()); + ctx.route_capabilities.image_input = CapabilityState::Supported; + + let tool = ReadMediaTool; + let rich = tool + .execute_rich( + json!({ + "path": "photo.png", + "detail": "auto", + "crop": { "x": 0, "y": 0, "width": 800, "height": 1200 } + }), + &ctx, + ) + .await + .unwrap(); + assert!(rich.success); + assert_eq!(rich.metadata.as_ref().unwrap()["cropped"], true); + + let ToolResultContentBlock::Image { data, .. } = &rich.content_blocks[0]; + let payload_len = BASE64.decode(data).unwrap().len(); + assert!( + payload_len > READ_IMAGE_BYTE_BUDGET, + "a crop read must keep the full wire budget instead of the {}-byte detail budget (got {payload_len})", + READ_IMAGE_BYTE_BUDGET + ); + assert!( + payload_len <= MAX_WIRE_IMAGE_BYTES, + "delivered payload {payload_len} must still fit the {}-byte wire budget", + MAX_WIRE_IMAGE_BYTES + ); + } + #[tokio::test] async fn read_media_photo_over_budget_arrives_under_budget_as_jpeg() { let dir = tempdir().unwrap(); @@ -1870,6 +1917,14 @@ mod tests { assert!(note.contains("Original dimensions: 3000x1500"), "{note}"); assert!(note.contains("downsampled to 2048x1024"), "{note}"); assert!(note.contains("fine detail may be lost"), "{note}"); + assert!( + note.contains("(encoded payload)"), + "size line must label the pre-base64 bytes as encoded payload: {note}" + ); + assert!( + !note.contains("(wire payload)"), + "the pre-base64 size must not be labeled as the wire payload: {note}" + ); assert!( note.contains( "call read_media again with the crop parameter (original-image pixel coordinates)" @@ -1878,6 +1933,35 @@ mod tests { ); } + #[test] + fn read_media_copy_pins_the_delivery_budget_sentences() { + // The budget disclosure is the direct item-5b fix; pin both + // surfaces so a silent revert (or a silently widened figure) + // fails the suite. + let tool = ReadMediaTool; + let description = tool.description(); + assert!( + description.contains("fit a ~256KB delivery budget"), + "the auto/low delivery budget must stay disclosed: {description}" + ); + assert!( + description.contains("keep the full 5 MiB payload limit"), + "the high/original/crop wire budget must stay disclosed: {description}" + ); + let schema = tool.input_schema(); + let detail = schema["properties"]["detail"]["description"] + .as_str() + .expect("detail description"); + assert!( + detail.contains("same ~256KB delivery budget as 'low'"), + "detail must tie auto to low's budget: {detail}" + ); + assert!( + detail.contains("full 5 MiB payload limit"), + "detail must keep the high/original wire budget: {detail}" + ); + } + #[tokio::test] async fn read_media_crop_after_downsample_pulls_full_res_stored_original() { let dir = tempdir().unwrap(); diff --git a/crates/tui/src/tools/run_tool.rs b/crates/tui/src/tools/run_tool.rs index 406e8371fe..b298b89f72 100644 --- a/crates/tui/src/tools/run_tool.rs +++ b/crates/tui/src/tools/run_tool.rs @@ -106,21 +106,22 @@ impl ToolSpec for RunTool { }, "commands": { "type": "array", - "description": "Optional explicit verifier gates (action=verifiers)", + "description": "Optional explicit verifier gates (action=verifiers). Foreground gates run directly as program+args, not through a shell; use program='bash', args=['-lc', '...'] only when the shell is intentionally part of the verifier. Background gates are rendered into a single command line and run through the detected platform shell, so shell quoting rules apply there (a POSIX-quoted path can fail under PowerShell).", "items": { "type": "object", "properties": { - "name": { "type": "string" }, - "program": { "type": "string" }, - "args": { "type": "array", "items": { "type": "string" } }, - "cwd": { "type": "string" } + "name": { "type": "string", "description": "Short unique gate name." }, + "program": { "type": "string", "description": "Executable to spawn, for example 'cargo', 'npm', 'pytest', or 'make'." }, + "args": { "type": "array", "items": { "type": "string" }, "description": "Arguments passed directly to the executable." }, + "cwd": { "type": "string", "description": "Optional working directory relative to the workspace." } }, - "required": ["name", "program"] + "required": ["name", "program"], + "additionalProperties": false } }, "background": { "type": "boolean", - "description": "Start verifier gates as background jobs (action=verifiers)" + "description": "Start verifier gates as background jobs and return immediately with a task id per gate, null when a gate is skipped or fails to start (action=verifiers). Use for long build/test/lint gates; completion is tracked in task/status state, and task_shell_wait is only for early output, final output, or true dependency barriers." } }, "required": ["action"] @@ -206,4 +207,42 @@ mod tests { .collect(); assert_eq!(advertised, RunTool::ACTIONS); } + + #[test] + fn run_verifier_copy_pins_the_per_path_semantics() { + // These sentences were corrected by review rounds against the actual + // spawn paths (foreground direct spawn vs. background shell render); + // pin them so a revert cannot restate a falsehood silently. + let schema = RunTool::new("Run").input_schema(); + let commands = schema["properties"]["commands"]["description"] + .as_str() + .expect("commands description"); + assert!( + commands.contains("Foreground gates run directly as program+args, not through a shell"), + "{commands}" + ); + assert!( + commands + .contains("Background gates are rendered into a single command line and run through the detected platform shell"), + "{commands}" + ); + assert!( + commands.contains("a POSIX-quoted path can fail under PowerShell"), + "{commands}" + ); + let background = schema["properties"]["background"]["description"] + .as_str() + .expect("background description"); + assert!( + background + .contains("a task id per gate, null when a gate is skipped or fails to start"), + "{background}" + ); + assert!( + !background.contains("task_ids"), + "the visible payload carries per-gate jobs, not a task_ids array: {background}" + ); + let items = &schema["properties"]["commands"]["items"]; + assert_eq!(items["additionalProperties"], json!(false)); + } } diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 62aa201e55..2a788ed087 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -124,24 +124,30 @@ pub(crate) fn abort_pending_persistent_process_groups_for_exit() { fn validate_shell_working_dir(path: &Path, inherited_session_workspace: bool) -> Result<()> { let metadata = std::fs::metadata(path).with_context(|| { - let source = if inherited_session_workspace { - "saved session workspace" + if inherited_session_workspace { + // Resume re-anchors to this same saved directory and a CLI fork + // copies it, so neither recovers a missing workspace; only + // recreating the directory or switching (in-app) does. + format!( + "saved session workspace is unavailable: {}. Recreate the directory to continue here, or continue the session from a different workspace (/workspace , or /fork after switching)", + path.display() + ) } else { - "requested working directory" - }; - format!( - "{source} is unavailable: {}. Restore or remap that directory, resume/fork the session from an existing workspace, or pass an explicit `working_dir`/`cwd` to exec_shell", - path.display() - ) + format!( + "requested working directory is unavailable: {}. Recreate it, or use a directory that exists", + path.display() + ) + } })?; if !metadata.is_dir() { - let source = if inherited_session_workspace { - "saved session workspace" - } else { - "requested working directory" - }; + if inherited_session_workspace { + return Err(anyhow!( + "saved session workspace is not a directory: {}. Restore it as a directory to continue here, or continue the session from a different workspace (/workspace , or /fork after switching)", + path.display() + )); + } return Err(anyhow!( - "{source} is not a directory: {}. Resume/fork from an existing workspace or pass an explicit `working_dir`/`cwd`", + "requested working directory is not a directory: {}. Use a directory that exists", path.display() )); } @@ -4775,7 +4781,7 @@ impl ToolSpec for LowercaseBashTool { } fn description(&self) -> &'static str { - "Execute a shell command in the workspace and return stdout and stderr. The command runs in the detected platform shell — `$SHELL` on Unix (any shell it names, falling back to `/bin/sh`); on Windows, bash when the session provides one, otherwise PowerShell (pwsh 7 or Windows PowerShell 5.1) or cmd.exe. Windows PowerShell 5.1 lacks `&&`; prefer forms the detected shell supports. In read-only shell sessions a single approved command runs directly as argv instead (only pipelines still go through the shell); with an external sandbox backend configured this tool is unavailable (use the backend-specific shell tool instead). Output keeps the last 2000 lines or 50KB. An optional timeout is expressed in seconds; when omitted the command is killed after 120 seconds, so pass an explicit timeout for work expected to take longer. In Ask, after a sandbox denial, retry the exact command once with sandbox_permissions (the narrowest wider mode that suffices) and a one-sentence justification; the approval prompt asks the user." + "Execute a shell command in the workspace and return its output. stdout and stderr arrive as one merged stream in arrival order, so a line cannot be attributed to one stream; redirect a command's stderr to a file when that attribution matters. Each call runs in a fresh shell — `cd` and exported variables do not persist between calls. The command runs in the detected platform shell — `$SHELL` on Unix (any shell it names, falling back to `/bin/sh`); on Windows, bash when the session provides one, otherwise PowerShell (pwsh 7 or Windows PowerShell 5.1) or cmd.exe. Windows PowerShell 5.1 lacks `&&`; prefer forms the detected shell supports. In read-only shell sessions a single approved command runs directly as argv instead (only pipelines still go through the shell); with an external sandbox backend configured this tool is unavailable (use the backend-specific shell tool instead). Output keeps the last 2000 lines or 50KB. An optional timeout is expressed in seconds; when omitted the command is killed after 120 seconds, so pass an explicit timeout for work expected to take longer. In Ask, after a sandbox denial, retry the exact command once with sandbox_permissions (the narrowest wider mode that suffices) and a one-sentence justification; the approval prompt asks the user." } fn input_schema(&self) -> serde_json::Value { diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index d73535648b..7a26942d8d 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -41,6 +41,31 @@ fn lowercase_bash_schema_is_small_contract() { assert!(!BashTool::new("Bash").model_visible()); } +#[test] +fn lowercase_bash_description_pins_the_stream_and_persistence_claims() { + // These sentences were corrected by the model-facing-copy audit against + // the contract spawn path (shared_output_pipe + finish_contract_bash_result + // concatenation, one Command per call); pin them so a revert cannot + // restate separate-stream or persistent-shell semantics silently. + let description = LowercaseBashTool.description(); + assert!( + description.contains("stdout and stderr arrive as one merged stream in arrival order"), + "{description}" + ); + assert!( + description.contains("redirect a command's stderr to a file"), + "{description}" + ); + assert!( + description.contains("Each call runs in a fresh shell"), + "{description}" + ); + assert!( + !description.contains("return stdout and stderr"), + "the contract merges both streams; no separate-stream promise may return: {description}" + ); +} + #[test] fn lowercase_bash_description_matches_the_timeout_it_actually_applies() { use super::{ @@ -581,8 +606,60 @@ fn deleted_saved_workspace_reports_path_and_recovery_before_spawn() { let message = error.to_string(); assert!(message.contains("saved session workspace is unavailable")); assert!(message.contains(&stale.display().to_string())); - assert!(message.contains("working_dir") || message.contains("cwd")); - assert!(message.contains("resume/fork")); + assert!(message.contains("Recreate the directory")); + assert!(message.contains("/workspace")); + assert!( + !message.contains("resume/fork"), + "resume re-anchors to the saved (missing) workspace and cannot recover" + ); +} + +#[test] +fn explicit_non_directory_working_dir_gets_a_direct_fix_hint() { + let workspace = tempdir().expect("workspace"); + let file = workspace.path().join("not-a-dir"); + std::fs::write(&file, b"occupant").expect("write occupant file"); + let mut manager = ShellManager::new(workspace.path().to_path_buf()); + + let error = execute_shell( + &mut manager, + "echo should-not-run", + file.to_str(), + 1_000, + false, + ) + .expect_err("non-directory explicit cwd must fail before shell spawn"); + let message = error.to_string(); + assert!(message.contains("requested working directory is not a directory")); + assert!(message.contains("Use a directory that exists")); + assert!(!message.contains("saved session workspace")); + // The requested-directory arm is the one model-reachable advice + // surface; resume re-anchors to the same directory and a fork copies + // it, so neither may be suggested here in any wording. + assert!( + !message.contains("resume"), + "requested-directory arm must not advise resume: {message}" + ); + assert!( + !message.contains("fork"), + "requested-directory arm must not advise fork: {message}" + ); +} + +#[test] +fn saved_workspace_that_is_a_file_gets_the_saved_workspace_advice() { + let parent = tempdir().expect("parent"); + let file_workspace = parent.path().join("workspace-file"); + std::fs::write(&file_workspace, b"occupant").expect("write occupant file"); + let mut manager = ShellManager::new(file_workspace.clone()); + + let error = execute_shell(&mut manager, "echo should-not-run", None, 1_000, false) + .expect_err("saved workspace that is not a directory must fail before shell spawn"); + let message = error.to_string(); + assert!(message.contains("saved session workspace is not a directory")); + assert!(message.contains("Restore it as a directory")); + assert!(message.contains("/workspace")); + assert!(message.contains("/fork")); } #[test] diff --git a/crates/tui/src/tools/tasks.rs b/crates/tui/src/tools/tasks.rs index c28a26c8a5..a81c47f061 100644 --- a/crates/tui/src/tools/tasks.rs +++ b/crates/tui/src/tools/tasks.rs @@ -153,7 +153,7 @@ impl ToolSpec for TasksTool { if self.read_only { "Inspect durable tasks and their PR attempts. Actions: \"list\", \"read\", \"pr_attempt_list\", \"pr_attempt_read\"." } else { - "Manage durable background tasks through TaskManager. Durable tasks are restart-aware executable work, distinct from sub-agents. Actions: \"create\" (enqueue; approval), \"list\", \"read\", \"cancel\" (approval), \"gate_run\" (run an approved verification gate command and return structured evidence; approval), \"pr_attempt_record\" (approval), \"pr_attempt_list\", \"pr_attempt_read\", \"pr_attempt_preflight\" (approval). Use task_shell_start for long-running shell work." + "Manage durable background tasks through TaskManager. Durable tasks are restart-aware executable work, distinct from sub-agents. Actions: \"create\" (enqueue; approval), \"list\", \"read\", \"cancel\" (approval), \"gate_run\" (run an approved verification gate command and return structured evidence; approval), \"pr_attempt_record\" (approval), \"pr_attempt_list\", \"pr_attempt_read\", \"pr_attempt_preflight\" (approval). When shell execution is enabled for the task (allow_shell on, non-plan mode), use task_shell_start for long-running shell work." } } @@ -183,7 +183,7 @@ impl ToolSpec for TasksTool { ); properties.insert( "model".to_string(), - json!({ "type": "string", "description": "(action=create)" }), + json!({ "type": "string", "description": "Model id for the task's session, used as given. Omit to use the configured default model. Required when model_provider or model_provider_id is set. (action=create)" }), ); properties.insert( "workspace".to_string(), @@ -191,19 +191,19 @@ impl ToolSpec for TasksTool { ); properties.insert( "mode".to_string(), - json!({ "type": "string", "enum": ["agent", "plan", "operate"], "description": "(action=create)" }), + json!({ "type": "string", "enum": ["agent", "plan", "operate"], "description": "Operating mode for the task's session. 'plan' runs read-only: shell is disabled even if allow_shell is true, and plan's read-only blocks are not lifted by auto_approve (which otherwise skips approval prompts; armed ask-on-guardrail guardrails still prompt). trust_mode's effects still apply in plan. 'agent' (default) and 'operate' run with the task's allow_shell/trust_mode/auto_approve values. (action=create)" }), ); properties.insert( "allow_shell".to_string(), - json!({ "type": "boolean", "description": "(action=create)" }), + json!({ "type": "boolean", "description": "Whether the task's session may run shell commands. Passing true or false is honored subject to the mode — plan keeps shell disabled even when this is true (see mode); omitting falls back to the configured durable-task allow-shell setting, which is false unless shell execution is enabled in config. (action=create)" }), ); properties.insert( "trust_mode".to_string(), - json!({ "type": "boolean", "description": "(action=create)" }), + json!({ "type": "boolean", "description": "Run the task with trust mode: file paths are accepted without workspace-boundary validation, and when auto_approve is on, a tool call the sandbox denies is retried with full access instead of being denied. Defaults to false. (action=create)" }), ); properties.insert( "auto_approve".to_string(), - json!({ "type": "boolean", "description": "(action=create)" }), + json!({ "type": "boolean", "description": "Run the task with tool calls auto-approved (full-access posture, no approval prompts — armed ask-on-guardrail guardrails still force a human approval for the calls they guard). Defaults to false. (action=create)" }), ); properties.insert( "gate".to_string(), @@ -215,7 +215,7 @@ impl ToolSpec for TasksTool { ); properties.insert( "command".to_string(), - json!({ "type": "string", "description": "Command to run. (action=gate_run)" }), + json!({ "type": "string", "description": "Command to run. A command the safety classifier rates dangerous is refused with BLOCKED (nothing runs) unless the session itself runs with auto-approve. (action=gate_run)" }), ); properties.insert( "cwd".to_string(), @@ -223,19 +223,19 @@ impl ToolSpec for TasksTool { ); properties.insert( "timeout_ms".to_string(), - json!({ "type": "integer", "minimum": 1000, "maximum": 600000, "description": "(action=gate_run)" }), + json!({ "type": "integer", "minimum": 1000, "maximum": 600000, "description": "Kill deadline for the gate command in milliseconds. Defaults to 120000; values outside the declared range are clamped into it when the gate runs — negative or non-integer values are rejected as a type error before that. On expiry the process group is killed, the result reports timed_out with no exit code, and any partial output is kept. (action=gate_run)" }), ); properties.insert( "attempt_group_id".to_string(), - json!({ "type": "string", "description": "(action=pr_attempt_record)" }), + json!({ "type": "string", "description": "Label grouping related attempts for correlation across pr_attempt_list and pr_attempt_read; shown in the task timeline. Any string; a fresh random group id is generated when omitted. (action=pr_attempt_record)" }), ); properties.insert( "attempt_index".to_string(), - json!({ "type": "integer", "minimum": 1, "description": "(action=pr_attempt_record)" }), + json!({ "type": "integer", "minimum": 1, "description": "This attempt's position within its group (1-based), shown as 'index/count' in the task timeline. Defaults to 1; values below 1 are clamped to 1, and negative or non-integer values are rejected as a type error. (action=pr_attempt_record)" }), ); properties.insert( "attempt_count".to_string(), - json!({ "type": "integer", "minimum": 1, "description": "(action=pr_attempt_record)" }), + json!({ "type": "integer", "minimum": 1, "description": "Total number of attempts intended for the group, shown as 'index/count' in the task timeline. Defaults to 1; values below 1 are clamped to 1, and negative or non-integer values are rejected as a type error. (action=pr_attempt_record)" }), ); properties.insert( "summary".to_string(), @@ -243,20 +243,20 @@ impl ToolSpec for TasksTool { ); properties.insert( "verification".to_string(), - json!({ "type": "array", "items": { "type": "string" }, "description": "(action=pr_attempt_record)" }), + json!({ "type": "array", "items": { "type": "string" }, "description": "Verification evidence lines recorded with the attempt, returned by pr_attempt_list and pr_attempt_read. Omit for an empty list; non-string entries are dropped. (action=pr_attempt_record)" }), ); } properties.insert( "attempt_id".to_string(), - json!({ "type": "string", "description": "(action=pr_attempt_read/preflight)" }), + json!({ "type": "string", "description": "Attempt id to fetch, as returned by pr_attempt_list. Matched exactly; an unknown id fails with 'Attempt not found'. (action=pr_attempt_read/preflight)" }), ); properties.insert( "task_id".to_string(), - json!({ "type": "string", "description": "Full task id or unambiguous prefix (action=read/cancel); task id, defaults to active task (action=pr_attempt_*)." }), + json!({ "type": "string", "description": "Full task id or unambiguous prefix (action=cancel); omitted, it defaults to the active task (action=read/pr_attempt_*)." }), ); properties.insert( "limit".to_string(), - json!({ "type": "integer", "minimum": 1, "maximum": 100, "default": 20, "description": "(action=list)" }), + json!({ "type": "integer", "minimum": 1, "maximum": 100, "default": 20, "description": "Maximum number of tasks returned, newest first. Values outside the declared range are clamped into it, and negative or non-integer values are rejected as a type error. (action=list)" }), ); json!({ "type": "object", @@ -1411,6 +1411,134 @@ mod tests { ); } + #[test] + fn task_range_clamps_describe_the_type_error_boundary() { + // Negative integers never reach the clamps (the u64 parser rejects + // them as a type mismatch), so a blanket "clamped, not rejected" + // would be false; each description must state the boundary instead. + let schema = TasksTool::new("tasks").input_schema(); + let timeout = schema["properties"]["timeout_ms"]["description"] + .as_str() + .expect("timeout_ms description"); + assert!(timeout.contains("clamped into it"), "{timeout}"); + assert!(timeout.contains("rejected as a type error"), "{timeout}"); + assert!(!timeout.contains("clamped, not rejected"), "{timeout}"); + for field in ["attempt_index", "attempt_count"] { + let description = schema["properties"][field]["description"] + .as_str() + .expect("attempt field description"); + assert!( + description.contains("values below 1 are clamped to 1"), + "{description}" + ); + assert!( + description.contains("rejected as a type error"), + "{description}" + ); + assert!( + !description.contains("clamped, not rejected"), + "{description}" + ); + } + let limit = schema["properties"]["limit"]["description"] + .as_str() + .expect("limit description"); + assert!(limit.contains("clamped into it"), "{limit}"); + assert!(limit.contains("rejected as a type error"), "{limit}"); + assert!(!limit.contains("clamped, not rejected"), "{limit}"); + } + + #[test] + fn task_authority_copy_pins_the_reviewed_semantics() { + // Each sentence below was corrected by a review round against the + // execution path; pin the exact claims so a revert cannot slip back + // into the schema silently. + let schema = TasksTool::new("tasks").input_schema(); + let mode = schema["properties"]["mode"]["description"] + .as_str() + .expect("mode description"); + assert!( + mode.contains("plan's read-only blocks are not lifted by auto_approve"), + "{mode}" + ); + assert!( + mode.contains("armed ask-on-guardrail guardrails still prompt"), + "the approval-skip parenthetical must carry the armed-guardrail exception: {mode}" + ); + assert!( + mode.contains("trust_mode's effects still apply in plan"), + "the plan read-only lead-in must disclose trust_mode's in-plan effects: {mode}" + ); + let allow_shell = schema["properties"]["allow_shell"]["description"] + .as_str() + .expect("allow_shell description"); + assert!( + allow_shell + .contains("omitting falls back to the configured durable-task allow-shell setting"), + "{allow_shell}" + ); + assert!( + allow_shell.contains("which is false unless shell execution is enabled in config"), + "{allow_shell}" + ); + assert!( + allow_shell.contains("honored subject to the mode"), + "{allow_shell}" + ); + assert!( + allow_shell.contains("plan keeps shell disabled even when this is true"), + "{allow_shell}" + ); + let trust_mode = schema["properties"]["trust_mode"]["description"] + .as_str() + .expect("trust_mode description"); + assert!( + trust_mode.contains("retried with full access instead of being denied"), + "{trust_mode}" + ); + let auto_approve = schema["properties"]["auto_approve"]["description"] + .as_str() + .expect("auto_approve description"); + assert!(auto_approve.contains("Defaults to false"), "{auto_approve}"); + assert!( + auto_approve.contains( + "no approval prompts — armed ask-on-guardrail guardrails still force a human approval", + ), + "the auto-approve absolute must carry the armed-guardrail exception: {auto_approve}" + ); + assert!( + !auto_approve.contains("no approval prompts)."), + "the unscoped no-prompts absolute must not return: {auto_approve}" + ); + let tool = TasksTool::new("tasks"); + let description = tool.description(); + assert!( + description.contains( + "When shell execution is enabled for the task (allow_shell on, non-plan mode), use task_shell_start", + ), + "the shell-tool pointer must be scoped to surfaces that register it: {description}" + ); + assert!( + !description.contains("Use task_shell_start for long-running shell work."), + "the unconditional shell-tool pointer must not return: {description}" + ); + let task_id = schema["properties"]["task_id"]["description"] + .as_str() + .expect("task_id description"); + assert!( + task_id.contains("defaults to the active task (action=read/pr_attempt_*)"), + "read and pr_attempt_* share the active-task default: {task_id}" + ); + assert!( + !task_id.contains("(action=read/cancel)"), + "read must not be listed as requiring a task id: {task_id}" + ); + let command = schema["properties"]["command"]["description"] + .as_str() + .expect("command description"); + assert!(command.contains("refused with BLOCKED"), "{command}"); + } + #[test] fn gate_classifier_detects_timeout() { assert_eq!( diff --git a/crates/tui/src/tools/verifier.rs b/crates/tui/src/tools/verifier.rs index aba4d275b2..429be1f98e 100644 --- a/crates/tui/src/tools/verifier.rs +++ b/crates/tui/src/tools/verifier.rs @@ -275,7 +275,7 @@ impl ToolSpec for RunVerifiersTool { }, "commands": { "type": "array", - "description": "Optional explicit verifier gates. Commands run directly as program+args, not through a shell. Use program='bash', args=['-lc', '...'] only when Bash is intentionally part of the verifier.", + "description": "Optional explicit verifier gates. Foreground gates run directly as program+args, not through a shell; background gates are rendered into a single command line and run through the detected platform shell, so shell quoting rules apply there. Use program='bash', args=['-lc', '...'] only when Bash is intentionally part of the verifier.", "items": { "type": "object", "properties": { @@ -1588,4 +1588,69 @@ mod tests { output.stdout ); } + + #[tokio::test] + async fn run_verifiers_background_reports_null_task_ids_for_skipped_and_failed_gates() { + // The background copy promises "a task id per gate, null when a gate + // is skipped or fails to start". Pin both null arms: a gate without + // a program is skipped before spawn, and a gate whose working + // directory does not exist fails validation inside the shell + // manager, reporting failed_to_start with the accurate error. + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path()); + + let missing = tmp.path().join("definitely-missing-cwd"); + let failed = start_background_gates( + &ctx, + VerifierProfile::Auto, + VerifierLevel::Quick, + vec![VerifierGate { + name: "no-such-cwd".to_string(), + ecosystem: "custom".to_string(), + cwd: missing, + program: Some("true".to_string()), + args: vec![], + env: vec![], + skipped_reason: None, + }], + ) + .expect("background result"); + let parsed: RunVerifiersBackgroundOutput = + serde_json::from_str(&failed.content).expect("background output json"); + assert_eq!(parsed.failed_to_start, 1, "{}", parsed.summary); + assert!(!parsed.success); + let job = &parsed.jobs[0]; + assert_eq!(job.status, "failed_to_start"); + assert!( + job.task_id.is_none(), + "a failed-to-start gate must carry no task id" + ); + let error = job.error.as_deref().expect("failure reason"); + assert!(error.contains("unavailable"), "{error}"); + + let skipped = start_background_gates( + &ctx, + VerifierProfile::Auto, + VerifierLevel::Quick, + vec![VerifierGate { + name: "not-applicable".to_string(), + ecosystem: "custom".to_string(), + cwd: tmp.path().to_path_buf(), + program: None, + args: vec![], + env: vec![], + skipped_reason: Some("tooling not installed".to_string()), + }], + ) + .expect("background result"); + let parsed: RunVerifiersBackgroundOutput = + serde_json::from_str(&skipped.content).expect("background output json"); + let job = &parsed.jobs[0]; + assert_eq!(job.status, "skipped"); + assert!( + job.task_id.is_none(), + "a skipped gate must carry no task id" + ); + assert_eq!(job.skipped_reason.as_deref(), Some("tooling not installed")); + } } diff --git a/docs/READ_MEDIA.md b/docs/READ_MEDIA.md index 573f47188a..73362f7bbd 100644 --- a/docs/READ_MEDIA.md +++ b/docs/READ_MEDIA.md @@ -74,6 +74,7 @@ always_load = ["read_media"] - **Pixel Budget:** Maximum total pixel count is **33,554,432 pixels** (~33.5 megapixels, `MAX_IMAGE_PIXELS`). - **Memory Ceiling:** Safe memory allocation during decode is capped at **64 MiB** (`MAX_DECODE_ALLOC_BYTES`). - **Wire Payload Limit:** Re-encoded image payload is capped at **5 MiB** (`MAX_WIRE_IMAGE_BYTES`), matching provider constraints. +- **Delivery Budget:** Default full reads (`detail` `"auto"`/`"low"`) are re-encoded through a quality/size ladder to fit a **256 KiB** delivery budget (`READ_IMAGE_BYTE_BUDGET`, roughly 350 KB on the wire after base64). `"high"` / `"original"` detail and `crop` reads keep the full **5 MiB** wire budget. ### 4.3. Active Route Vision Checks - Before reading an image, `read_media` inspects `context.route_capabilities.image_input`.