diff --git a/CHANGELOG.md b/CHANGELOG.md index dcc656d..24f1b5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,26 @@ All notable changes to the Dojo CLI are documented here. This project adheres to [Keep a Changelog v1.1.0](https://keepachangelog.com/en/1.1.0/) and [Semantic Versioning](https://semver.org/). +## 2026-07-17 — Hooks, Permissions, Delegation Routing, Guardrails, External Skills + +### Added + +- feat(hooks): blocking hooks — per-rule `"blocking": true` in `hooks/hooks.json`; only `command`-type hooks can veto (an `http` hook is fire-and-forget by design, and `prompt`/`agent` hook types are unimplemented and now print a once-per-plugin stderr warning instead of a silent no-op) +- feat(hooks): new `UserPromptSubmit` event fires on free-text chat input before it reaches the Gateway, matched against the literal command `"chat"`; blocking takes effect there and at `PreCommand` only — `PostCommand`/`SessionStart`/`SessionEnd` stay log-only regardless of a rule's `blocking` flag. The prompt text rides to command hooks via `DOJO_PROMPT` (truncated to 4096 bytes, delivered through the process environment, never shell-interpolated) +- feat(permissions): new action-permission gate (`internal/permissions/`) — `permissions.mode` (`default` / `allowlist` / `yolo`; env `DOJO_PERMISSIONS_MODE`; `--yolo` flag forces yolo in-memory for the run with a loud stderr warning) and `permissions.allowed` (exact, `craft.*`-style glob, or bare `*` dot-path patterns) gate `code.undo`, `plugin.install`, `plugin.rm`, `craft.adr`, `craft.claude-md` (the `--fix` write path only), and `craft.scaffold` +- feat(agent): per-dispatch model override — `/agent dispatch` and `/agent chat` accept `--model ` / `--model=` in any position; precedence is flag > `delegation.model` (env `DOJO_DELEGATION_MODEL`) > Gateway default, printed as `model: (flag|delegation default)` before the stream; `/model ls` shows the delegation default when set +- feat(guardrail): advisory circuit breaker (`internal/guardrail/`) — escalating one-line notices (never blocking) after `guardrails.warn_after` (default 3) / `guardrails.hard_after` (default 5) consecutive identical failures of a slash command or a same-signature chat-stream tool call +- feat(skills): read-only external skill discovery (`internal/skills/external.go`) — `skills.external_dirs` (default `[".claude/skills"]`) scanned for `SKILL.md` files from foreign agent ecosystems; `/skill ls` appends an "External (read-only)" section, `/skill get ext:` forces external resolution, plain `/skill get` falls back to external only on a Gateway CAS miss, and `package-all` never sweeps external dirs +- feat(config): new `permissions` / `delegation` / `guardrails` / `skills` `settings.json` sections, with `DOJO_PERMISSIONS_MODE` / `DOJO_DELEGATION_MODEL` env overrides that are never persisted back to `settings.json` + +### Docs + +- docs: document the `/craft` command suite (previously undocumented) and the six capabilities above in README; extend the `Configuration` example with the four new settings.json sections; correct a stale `plugin.json`/`hooks.json` example in Plugin System that predated the `hooks/hooks.json` file split + +Inspired by Claude Code (hooks/permissions/model-override), Hermes fleet (loop guardrails, delegation lane), and Charm's Crush (permission tiers, Agent Skills interop). + +--- + ## [Unreleased] ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 63349dc..40973b8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,19 +75,31 @@ Place in map: one of four active DojoGenesis products; `desktop/` subdir is HIBE cli/ cmd/dojo/main.go -- entrypoint; flags + one-shot + REPL launch internal/ + activity/ -- timestamped NDJSON activity log (~/.dojo/activity.log) + art/ -- ASCII art assets + artifacts/ -- skill output + workflow result store (~/.dojo/projects/) + bootstrap/ -- first-run setup: ~/.dojo dir, plugins, MCP config, seeds client/ -- Gateway HTTP + SSE client - repl/ -- interactive REPL and TUI - commands/ -- slash command handlers - skills/ -- /skill commands + CAS bridge - plugins/ -- plugin scanner + installer + commands/ -- slash command Registry + Dispatch (own dispatcher, not Cobra) + config/ -- ~/.dojo/settings.json loader/validator + disposition presets + guardrail/ -- advisory REPL circuit breaker (warn/hard-stop on repeat failures) + guide/ -- interactive step-by-step feature guides + XP hooks/ -- PreCommand/PostCommand hook runner - orchestration/ -- /run + DAG (nlparse.go) + ioutilx/ -- filesystem utilities not in the stdlib + mdrender/ -- markdown document rendering for the terminal + orchestration/ -- DAG execution plans: built-in templates + heuristic NL parsing (nlparse.go) + permissions/ -- action-permission gate (silent / confirm / refuse) + plugins/ -- plugin scanner + installer project/ -- /project lifecycle + protocol/ -- injects the "genius protocol" doctrine into chat/agent turns + providers/ -- static provider/model catalog + direct gateway-bypass chat clients + repl/ -- interactive REPL and TUI + skills/ -- /skill commands + CAS bridge, semantic clustering spirit/ -- belt, XP, achievements, koans - providers/ -- model provider routing (last modified 2026-06-08) - artifacts/ -- artifact store (last modified 2026-06-09) - config/ -- ~/.dojo/settings.json loader + DojoDir() state/ -- ~/.dojo/state.json (sessions, agents, XP) + telemetry/ -- batched async telemetry sink (SSE events to D1 ingest API) + trace/ -- lightweight HTTP tracing for the gateway client + tui/ -- Bubbletea terminal UI dashboards desktop/ -- HIBERNATED Wails v2 + Svelte 5 scaffold scripts/ -- install.sh + release helpers Makefile -- build / test / vet / install targets diff --git a/README.md b/README.md index 5aec7f3..7fb55d0 100644 --- a/README.md +++ b/README.md @@ -55,10 +55,27 @@ Settings are loaded from `~/.dojo/settings.json`. A missing file is not an error "provider": "", "disposition": "balanced", "model": "" + }, + "permissions": { + "mode": "default", + "allowed": ["craft.*", "plugin.install"] + }, + "delegation": { + "model": "" + }, + "guardrails": { + "enabled": true, + "warn_after": 3, + "hard_after": 5 + }, + "skills": { + "external_dirs": [".claude/skills"] } } ``` +`permissions`, `delegation`, `guardrails`, and `skills` are covered in detail in [Permissions](#permissions), [Guardrails](#guardrails), [Agents & Orchestration](#agents--orchestration), and [Skills & CAS](#skills--cas) below. + **Environment variable overrides:** | Variable | Overrides | @@ -71,6 +88,10 @@ Settings are loaded from `~/.dojo/settings.json`. A missing file is not an error | `DOJO_SKILLS_PATH` | default skill dir for `/skill package-all` | | `DOJO_PROTOCOL_DISABLED` | any non-empty value disables the genius protocol (`protocol.enabled`) | | `DOJO_PROTOCOL_PATH` | `protocol.path` — explicit override doc, takes precedence over `./DOJO.md` | +| `DOJO_PERMISSIONS_MODE` | `permissions.mode` — `default`, `allowlist`, or `yolo` | +| `DOJO_DELEGATION_MODEL` | `delegation.model` — default model for `/agent dispatch` and `/agent chat` | + +`guardrails` and `skills.external_dirs` have no environment override (not requested for this release). ## Requirements @@ -145,8 +166,8 @@ Type a message without `/` to chat with the gateway. Use slash commands for stru | Command | Description | |----------------------------------------------|-----------------------------------------------------------| | `/agent ls` | List agents from gateway + recently used local agents | -| `/agent dispatch [mode] ` | Create agent and stream response | -| `/agent chat ` | Chat with an existing agent by ID | +| `/agent dispatch [mode] [--model ] ` | Create agent and stream response | +| `/agent chat [--model ] ` | Chat with an existing agent by ID | | `/agent info ` | Show agent detail: disposition, channels, config | | `/agent channels ` | List bound channels for an agent | | `/agent bind ` | Bind a channel to an agent | @@ -156,6 +177,8 @@ Type a message without `/` to chat with the gateway. Use slash commands for stru | `/pilot` | Live SSE event dashboard (Ctrl+C to stop) | | `/pilot plain` | Live event stream in plain text | +**Per-dispatch model override:** `--model ` (or `--model=`, either form works anywhere in the argument list) beats `delegation.model` in `~/.dojo/settings.json` (env `DOJO_DELEGATION_MODEL`), which beats the Gateway's own default. When a model is in effect, a `model: (flag|delegation default)` line prints just before the response streams. `/model ls` shows the delegation default when one is set. The Gateway may not honor the field yet — the CLI sends it regardless, as forward compatibility. + ### Memory & Seeds | Command | Description | @@ -191,6 +214,8 @@ Session IDs follow the format `dojo-cli-YYYYMMDD-HHmmss` when created via `/sess | `/skill tags` | List all CAS tags (name, version, ref) | | `/skill package-all [dir]` | Walk a directory for SKILL.md files and push all to CAS | +`/skill ls` appends a supplementary **External (read-only)** section (silent when empty) discovered from `skills.external_dirs` (default `[".claude/skills"]`) — `SKILL.md` files from foreign agent ecosystems (Claude Code, Cursor, etc.), recognized at `/SKILL.md` and `//SKILL.md`, with `~` expansion. `/skill get ext:` forces external resolution; a plain `/skill get ` falls back to external only when the gateway CAS lookup misses. External skills are read-only reference material — `package-all` never walks `skills.external_dirs`. + ### Projects | Command | Description | @@ -226,10 +251,12 @@ Track statuses: `pending`, `in-progress`, `completed`, `blocked`. | Command | Description | |---------------------------|---------------------------------------------------------| | `/plugin ls` | List installed plugins with skill count and hook rules | -| `/plugin install ` | Clone a plugin from a git URL into `~/.dojo/plugins/` | -| `/plugin rm ` | Remove an installed plugin | +| `/plugin install ` | Clone a plugin from a git URL into `~/.dojo/plugins/` (gated: `plugin.install`) | +| `/plugin rm ` | Remove an installed plugin (gated: `plugin.rm`) | | `/hooks ls` | List loaded hook rules from all plugins | -| `/hooks fire ` | Manually fire a hook event (for testing) | +| `/hooks fire ` | Manually fire a hook event (for testing), e.g. `/hooks fire SessionStart` | + +See [Plugin System](#plugin-system) below for the `hooks.json` format, blocking hooks, and the `UserPromptSubmit` event; see [Permissions](#permissions) for what the gates above mean. ### Protocol & Harnesses @@ -292,21 +319,72 @@ See [Genius Protocol](#genius-protocol) below for how the protocol doc is resolv | `/code build` | Run `go build ./...` | | `/code vet` | Run `go vet ./...` | | `/code gate` | Run the full build gate: build + test + vet | -| `/code undo` | Preview unstaged changes to tracked files, then revert on confirmation | +| `/code undo` | Preview unstaged changes to tracked files, then revert (gated: `code.undo`) | + +### Craft Workbench + +`/craft` is the DojoCraft practitioner workbench — strategic thinking, codebase intelligence, and memory curation in one command group. + +| Command | Description | +|---------------------------------------|----------------------------------------------------------------------| +| `/craft` | Show the `/craft` subcommand list | +| `/craft adr ` | Stream an ADR from the gateway, write it to `decisions/NNN-slug.md` (gated: `craft.adr`) | +| `/craft scout <tension>` | Tension → routes → synthesis → decision, streamed from the gateway | +| `/craft claude-md` | Analyse every `CLAUDE.md` under cwd (depth ≤ 3) for gaps, contradictions, and stale rules | +| `/craft claude-md --fix` | Same analysis, then rewrite the flagged files in place (gated: `craft.claude-md`) | +| `/craft memory ls` | List Gateway memory entries | +| `/craft memory add <text>` | Store a new memory entry | +| `/craft memory rm <id>` | Delete a memory entry by ID | +| `/craft memory prune [type]` | Preview and confirm-delete memories, optionally filtered by type | +| `/craft memory search <query>` | Search memory entries | +| `/craft seed ls` | List memory garden seeds | +| `/craft seed harvest` | Alias for `seed ls` — no distinct curation logic yet | +| `/craft seed plant <text>` | Plant a new seed | +| `/craft seed search <query>` | Search seeds locally by name/content | +| `/craft seed elevate [target]` | Promote a seed (or freeform text) to durable memory, with confirm | +| `/craft view [path]` | Codebase overview: top-level tree, go.mod, entry points, source/test file counts, git status | +| `/craft scaffold` | List available scaffold templates | +| `/craft scaffold <template>` | Create a project layout from a template (gated: `craft.scaffold`) | +| `/craft converge` | Git + memory health report: RED/YELLOW/GREEN signal | + +Templates for `/craft scaffold`: `go-service`, `fullstack`, `orchestration`, `plugin`, `minimal`. + +`craft.adr`, `craft.claude-md` (the `--fix` write path only — plain analysis is ungated), and `craft.scaffold` go through the permissions gate — see [Permissions](#permissions). `/craft memory prune` and `/craft seed elevate` keep their own y/N confirm instead: they mutate Gateway-side memory state, not local files, and sit outside this wave's gates. ## Directory Structure ``` cli/ -├── cmd/ # Cobra command tree +├── cmd/dojo/ # Entrypoint (main.go): flags, one-shot mode, REPL launch ├── internal/ -│ ├── repl/ # Interactive REPL and TUI panels -│ ├── client/ # Gateway HTTP + SSE client -│ ├── plugin/ # Plugin loader and hook runner -│ ├── skill/ # Skill and CAS commands -│ └── spirit/ # Belt, XP, achievements, koans -├── desktop/ # Desktop app (Wails v2 + Svelte 5) -└── scripts/ # Install and release scripts +│ ├── activity/ # Timestamped NDJSON activity log (~/.dojo/activity.log) +│ ├── art/ # ASCII art assets +│ ├── artifacts/ # Skill output + workflow result store (~/.dojo/projects/) +│ ├── bootstrap/ # First-run setup: ~/.dojo dir, plugins, MCP config, seeds +│ ├── client/ # Gateway HTTP + SSE client +│ ├── commands/ # Slash command Registry + Dispatch (own dispatcher, not Cobra) +│ ├── config/ # ~/.dojo/settings.json loader/validator + disposition presets +│ ├── guardrail/ # Advisory REPL circuit breaker (warn/hard-stop on repeat failures) +│ ├── guide/ # Interactive step-by-step feature guides + XP +│ ├── hooks/ # Runs hook rules from loaded plugins +│ ├── ioutilx/ # Filesystem utilities not in the stdlib +│ ├── mdrender/ # Markdown document rendering for the terminal +│ ├── orchestration/ # DAG execution plans: built-in templates + heuristic NL parsing +│ ├── permissions/ # Action-permission gate (silent / confirm / refuse) +│ ├── plugins/ # Plugin scanner + git-based installer +│ ├── project/ # Project lifecycle: phases, tracks, decisions, artifacts +│ ├── protocol/ # Injects the "genius protocol" doctrine into chat/agent turns +│ ├── providers/ # Static provider/model catalog + direct gateway-bypass chat clients +│ ├── repl/ # Interactive REPL and TUI panels +│ ├── skills/ # Skill and CAS commands, semantic clustering +│ ├── spirit/ # Belt, XP, achievements, koans +│ ├── state/ # Session/agent state across REPL invocations (~/.dojo/state.json) +│ ├── telemetry/ # Batched async telemetry sink (SSE events to D1 ingest API) +│ ├── trace/ # Lightweight HTTP tracing for the gateway client +│ └── tui/ # Bubbletea terminal UI dashboards +├── desktop/ # Desktop app (Wails v2 + Svelte 5) — HIBERNATED +├── scripts/ # Install and release scripts +└── docs/ # Audits, improvement plans, testing-loop notes ``` ## Desktop App @@ -388,34 +466,106 @@ Or drop a `DOJO.md` file in the project root (or `~/.dojo/DOJO.md` for a machine Plugins extend the CLI with hook rules and skills. Place plugin directories under `~/.dojo/plugins/` (or the path configured in `plugins.path`), or use `/plugin install` to clone from a git URL. -Each plugin directory must contain a `plugin.json` manifest: +Each plugin directory must contain a `plugin.json` manifest, checked at `.claude-plugin/plugin.json` first, then the plugin root: ```json { "name": "my-plugin", "version": "1.0.0", - "hooks": [ + "description": "" +} +``` + +Hook rules live in a separate `hooks/hooks.json` file inside the plugin directory, keyed by event name: + +```json +{ + "PreCommand": [ { - "event": "session.start", - "type": "command", - "command": "/usr/local/bin/my-hook" + "matcher": "craft*", + "if": "", + "blocking": true, + "hooks": [ + { "type": "command", "command": "/usr/local/bin/my-hook" } + ] } ] } ``` +A Claude-Code-style wrapped file (`{"hooks": {"<Event>": [...]}}`) is also accepted; its event names are translated where a dojo equivalent exists (`PreToolUse`→`PreCommand`, `PostToolUse`→`PostCommand`, `SubagentStop`→`PostAgent`; `SessionStart`/`SessionEnd` pass through unchanged) and skipped with a log line otherwise (`Notification`, `UserPromptSubmit`, `Stop`, and `PreCompact` have no dojo equivalent when they arrive via the wrapped schema). + +Event names dojo-cli itself fires: `PreCommand`, `PostCommand`, `PostSkill`, `PostAgent`, `SessionStart`, `SessionEnd`, `UserPromptSubmit`. + +`matcher` is a glob matched against the command name (leading `/` stripped, so `"garden*"` matches both `/garden ls` and `garden ls`); empty or `"*"` matches everything. `if` is `""`/`"true"` (always fire), `"false"` (never), or an environment variable name (fires when it's set and non-empty). + +**Blocking hooks.** Set `"blocking": true` on a rule to let a failing hook veto the action it guards. Only a `command`-type hook can actually block — a non-zero exit or error aborts the caller; `http` hooks are fire-and-forget by design, and `prompt`/`agent` hook types are not implemented and print a one-time-per-plugin stderr warning instead of silently no-op'ing. Blocking has effect on exactly two events: + +- **`PreCommand`** — a blocked slash command never dispatches. +- **`UserPromptSubmit`** — fires on free-text chat input before anything is sent to the Gateway; a block returns you to the prompt with nothing sent. This event has no command name to match against, so its rules match the literal string `"chat"`. The chat text itself rides in the `DOJO_PROMPT` environment variable for command hooks (truncated to 4096 bytes) — delivered via the process environment, never shell-interpolated, so metacharacters in your message are inert. + +`PostCommand`, `SessionStart`, and `SessionEnd` hooks always run log-only, regardless of `blocking`. + +A blocked action prints: `[hooks] blocked by <plugin>/<event>: <reason>`. + Plugin management commands: ``` /plugin ls # list installed plugins -/plugin install https://github.com/... # clone a plugin from git -/plugin rm my-plugin # remove an installed plugin +/plugin install https://github.com/... # clone a plugin from git (gated: plugin.install) +/plugin rm my-plugin # remove an installed plugin (gated: plugin.rm) /hooks ls # list all hook rules -/hooks fire session.start # manually fire a hook event +/hooks fire SessionStart # manually fire a hook event ``` Plugins are rescanned live after install and remove operations — no restart needed. +## Permissions + +Risky actions — file writes, plugin install/remove, working-tree reverts — go through a permission gate before they run. Configure the strategy under `permissions` in `~/.dojo/settings.json`: + +| Mode | Behavior | +|-------------|--------------------------------------------------------------------------------| +| `default` | Prompt per action (`allow <action>? <detail> [y/N]`) unless it matches `permissions.allowed`. A non-interactive context (no TTY) refuses instead of hanging. | +| `allowlist` | Silently allow actions matching `permissions.allowed`; deny everything else, with a pointer to settings. | +| `yolo` | Allow everything, no prompts. Prints one warning to stderr per process. | + +`permissions.allowed` is a list of dot-path patterns matched against an action name: exact (`"plugin.install"`), trailing-star glob (`"craft.*"`), or a bare `"*"` for everything. + +Gated actions this release: + +| Action | Command | +|--------------------|------------------------------------------------------| +| `code.undo` | `/code undo` | +| `plugin.install` | `/plugin install` | +| `plugin.rm` | `/plugin rm` | +| `craft.adr` | `/craft adr` (the file write) | +| `craft.claude-md` | `/craft claude-md --fix` (the write path only — plain analysis is ungated) | +| `craft.scaffold` | `/craft scaffold <template>` | + +`/craft memory prune` and `/craft seed elevate` keep their own y/N confirm — they mutate Gateway-side memory state, not local files, and sit outside this gate for now. + +Override the mode for one run with `DOJO_PERMISSIONS_MODE`, or use the `--yolo` flag (sets `yolo` in memory only for that run — never persisted to `settings.json` — and prints a loud stderr warning). A denied or declined action prints a one-line message naming both escape hatches: add the action (or a covering glob) to `permissions.allowed`, or re-run with `--yolo`. + +## Guardrails + +An advisory circuit breaker watches for repeated, identical failures and escalates a one-line notice — it never blocks or refuses an action, only gets louder. Inspired by Hermes' `tool_loop_guardrails`. + +Two independent trackers share the same `guardrails` settings: + +- **Slash commands** — consecutive failures of the same command, keyed by command name plus its first subcommand token (`/code test` and `/code build` count as separate streaks). +- **Chat-stream tool calls** — consecutive failures of the same tool with the same error signature during a direct (non-`/agent`) chat turn. + +A success resets that streak to zero. Configure in `~/.dojo/settings.json`: + +| Key | Default | Meaning | +|--------------------------|---------|------------------------------------------------------------------------| +| `guardrails.enabled` | `true` | Master on/off switch | +| `guardrails.warn_after` | `3` | Consecutive failures before the first notice | +| `guardrails.hard_after` | `5` | Consecutive failures before the escalated notice (repeats every time the streak is at or past this count) | + +No environment override is provided for this section. + ## Dojo Spirit Dojo Spirit is the engagement system built into the CLI. It tracks XP, belt ranks, daily streaks, achievements, and unlockable koans — all stored locally in `~/.dojo/state.json`. diff --git a/cmd/dojo/main.go b/cmd/dojo/main.go index abf6135..7310ef1 100644 --- a/cmd/dojo/main.go +++ b/cmd/dojo/main.go @@ -33,6 +33,7 @@ func main() { flagSession = flag.String("session", "", "Resume a specific session ID instead of the most recent one (implies --resume; see /session ls)") flagJSON = flag.Bool("json", false, "Output JSON lines in one-shot mode (for scripted pipelines)") flagPlain = flag.Bool("plain", false, "Plain text output (no ANSI colors, for piped/CI usage)") + flagYolo = flag.Bool("yolo", false, "Skip all permission prompts for this run (dangerous; never persisted to settings.json)") ) flag.Parse() @@ -81,6 +82,14 @@ func main() { if *flagDisposition != "" { cfg.Defaults.Disposition = *flagDisposition } + if *flagYolo { + // In-memory only for this run — cfg.Save() is never called on this + // path, and nothing here mutates settings.json, so the override can + // never leak to disk the way an env-sourced field could (see + // envOverride and Config.Save() in internal/config/config.go). + cfg.Permissions.Mode = "yolo" + fmt.Fprintln(os.Stderr, "dojo: YOLO mode: all permission prompts skipped") + } // Ensure ~/.dojo exists if err := os.MkdirAll(config.DojoDir(), 0700); err != nil { diff --git a/internal/client/client.go b/internal/client/client.go index 178bb91..8d17022 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -582,6 +582,10 @@ func (c *Client) PilotStream(ctx context.Context, clientID string, onChunk func( type CreateAgentRequest struct { WorkspaceRoot string `json:"workspace_root"` ActiveMode string `json:"active_mode,omitempty"` // "focused"|"balanced"|"exploratory"|"deliberate" + // Model requests a specific model to back this agent. Forward-compat: + // today's gateway POST /v1/gateway/agents has no model field and ignores + // it — same caveat as ChatRequest.SystemPrompt above. + Model string `json:"model,omitempty"` } // CreateAgentResponse is the response from POST /v1/gateway/agents. @@ -616,6 +620,10 @@ type AgentChatRequest struct { DocumentID string `json:"document_id,omitempty"` DocumentContent string `json:"document_content,omitempty"` Stream bool `json:"stream"` + // Model requests a specific model for this chat turn. Forward-compat: + // today's gateway POST /v1/gateway/agents/:id/chat has no model field + // and ignores it — same caveat as ChatRequest.SystemPrompt above. + Model string `json:"model,omitempty"` } // AgentChatStream sends a message to a specific agent and streams the SSE response. diff --git a/internal/commands/cmd_agent.go b/internal/commands/cmd_agent.go index 5fa6f40..8db06c1 100644 --- a/internal/commands/cmd_agent.go +++ b/internal/commands/cmd_agent.go @@ -30,19 +30,25 @@ func (r *Registry) agentCmd() Command { switch sub { case "dispatch": - // /agent dispatch [mode] <msg...> - // mode is optional — defaults to "balanced" + // /agent dispatch [mode] [--model <name>|--model=<name>] <msg...> + // mode is optional — defaults to "balanced"; --model is + // position-independent among the remaining args and is + // stripped before mode-detection and message-joining. validModes := map[string]bool{ "focused": true, "balanced": true, "exploratory": true, "deliberate": true, } + rest, modelFlag, modelFlagSet, flagErr := extractModelFlag(args[1:]) + if flagErr != nil { + return fmt.Errorf("usage: /agent dispatch [mode] [--model <name>] <message>: %w", flagErr) + } mode := "balanced" var msgArgs []string - if len(args) >= 2 && validModes[args[1]] { - mode = args[1] - msgArgs = args[2:] + if len(rest) >= 1 && validModes[rest[0]] { + mode = rest[0] + msgArgs = rest[1:] } else { - msgArgs = args[1:] + msgArgs = rest } if len(msgArgs) == 0 { return fmt.Errorf("usage: /agent dispatch [focused|balanced|exploratory|deliberate] <message>") @@ -52,8 +58,11 @@ func (r *Registry) agentCmd() Command { // Append explicit completion criteria so agents don't self-terminate prematurely. message = message + "\n\nCompletion requirements: (1) Do not stop after reading files — you must create or modify files to complete the task. (2) After making changes, run `make test` or the relevant test command. (3) Your final response must include the list of files you created or modified. If you cannot complete the task, say why explicitly." + model, modelSource := r.resolveModel(modelFlag, modelFlagSet) + fmt.Println() fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" Creating agent (mode: %s)...", mode)) + printModelLine(model, modelSource) wd, wdErr := os.Getwd() if wdErr != nil { @@ -62,6 +71,7 @@ func (r *Registry) agentCmd() Command { agentResp, err := r.gw.CreateAgent(ctx, client.CreateAgentRequest{ WorkspaceRoot: wd, ActiveMode: mode, + Model: model, }) if err != nil { return fmt.Errorf("could not create agent: %w", err) @@ -95,18 +105,26 @@ func (r *Registry) agentCmd() Command { } } - return r.streamAgentChat(ctx, agentResp.AgentID, message) + return r.streamAgentChat(ctx, agentResp.AgentID, message, model) case "chat": - // /agent chat <id> <msg...> - if len(args) < 3 { + // /agent chat [--model <name>|--model=<name>] <id> <msg...> + rest, modelFlag, modelFlagSet, flagErr := extractModelFlag(args[1:]) + if flagErr != nil { + return fmt.Errorf("usage: /agent chat [--model <name>] <agent-id> <message>: %w", flagErr) + } + if len(rest) < 2 { return fmt.Errorf("usage: /agent chat <agent-id> <message>") } - agentID := args[1] - message := strings.Join(args[2:], " ") + agentID := rest[0] + message := strings.Join(rest[1:], " ") + + model, modelSource := r.resolveModel(modelFlag, modelFlagSet) + fmt.Println() + printModelLine(model, modelSource) gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprint(" dojo ")) - chatErr := r.streamAgentChat(ctx, agentID, message) + chatErr := r.streamAgentChat(ctx, agentID, message, model) // Update last_used for this agent. if st, loadErr := state.Load(); loadErr == nil { @@ -259,13 +277,78 @@ func (r *Registry) agentCmd() Command { } } +// resolveModel implements the /agent dispatch and /agent chat model +// precedence: an explicit --model flag wins, then cfg.Delegation.Model (the +// workspace's dispatch-time default), then "" — meaning the Gateway picks +// its own default and Model is omitted from the JSON body via `omitempty`. +// source is a human-readable label for printModelLine ("flag" or +// "delegation default"); it is "" whenever model is "" too, so callers can +// pass both straight through without an extra check. +func (r *Registry) resolveModel(flagValue string, flagSet bool) (model, source string) { + if flagSet { + return flagValue, "flag" + } + if r.cfg.Delegation.Model != "" { + return r.cfg.Delegation.Model, "delegation default" + } + return "", "" +} + +// printModelLine prints the one-line dim/info banner /agent dispatch and +// /agent chat show before streaming begins, naming which model the +// precedence resolved to. No-op when model is "" — the Gateway default was +// left alone, so there is nothing to announce. +func printModelLine(model, source string) { + if model == "" { + return + } + fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" model: %s (%s)", model, source)) +} + +// extractModelFlag scans args for a "--model <value>" or "--model=<value>" +// flag anywhere in the slice — position-independent among the mode token +// and message words — and returns args with every occurrence of the flag +// (and, for the two-token form, its paired value) removed. present reports +// whether the flag was seen at all; when given more than once, the last +// occurrence's value wins, matching ordinary flag-parsing convention. err is +// non-nil only when an occurrence carries an empty value — a bare trailing +// "--model" with nothing after it, or "--model=" with nothing after the "=" +// — in which case the caller must surface err as a usage error and not +// dispatch. +func extractModelFlag(args []string) (rest []string, value string, present bool, err error) { + rest = make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + a := args[i] + switch { + case a == "--model": + if i+1 >= len(args) { + return nil, "", true, fmt.Errorf("--model requires a value") + } + present = true + value = args[i+1] + i++ + case strings.HasPrefix(a, "--model="): + v := strings.TrimPrefix(a, "--model=") + if v == "" { + return nil, "", true, fmt.Errorf("--model= requires a value") + } + present = true + value = v + default: + rest = append(rest, a) + } + } + return rest, value, present, nil +} + // streamAgentChat sends a message to an agent and streams the SSE response. // Thinking and tool-call events are rendered in dim colors; text is printed inline. -func (r *Registry) streamAgentChat(ctx context.Context, agentID, message string) error { +func (r *Registry) streamAgentChat(ctx context.Context, agentID, message, model string) error { req := client.AgentChatRequest{ Message: message, UserID: r.cfg.Auth.UserID, Stream: true, + Model: model, } err := r.gw.AgentChatStream(ctx, agentID, req, func(chunk client.SSEChunk) { diff --git a/internal/commands/cmd_agent_model_test.go b/internal/commands/cmd_agent_model_test.go new file mode 100644 index 0000000..776a5fc --- /dev/null +++ b/internal/commands/cmd_agent_model_test.go @@ -0,0 +1,352 @@ +package commands + +// cmd_agent_model_test.go — tests for /agent dispatch and /agent chat's +// --model flag: both forms (--model x, --model=x), position independence, +// stripping the flag from the outgoing message, the flag > delegation +// default > empty precedence, the empty-value usage-error gate, and that the +// resolved model actually lands on the wire in CreateAgentRequest.Model / +// AgentChatRequest.Model. +// +// Seams reused from existing test files (same package, no edits there): +// captureProtocolStdout (cmd_protocol_test.go) for stdout+gcolor capture, +// mirroring cmd_skill_external_test.go's httptest-fake-gateway style and its +// newExternalSkillRegistry-shaped Registry builder. + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/DojoGenesis/cli/internal/client" + "github.com/DojoGenesis/cli/internal/config" + "github.com/DojoGenesis/cli/internal/plugins" +) + +// agentModelCapture records the decoded JSON bodies the fake gateway received +// for the two endpoints /agent dispatch and /agent chat exercise, guarded by +// a mutex since the httptest.Server handler runs on its own goroutine. +type agentModelCapture struct { + mu sync.Mutex + create map[string]any + chat map[string]any +} + +func (c *agentModelCapture) setCreate(body []byte) { + var m map[string]any + _ = json.Unmarshal(body, &m) + c.mu.Lock() + c.create = m + c.mu.Unlock() +} + +func (c *agentModelCapture) setChat(body []byte) { + var m map[string]any + _ = json.Unmarshal(body, &m) + c.mu.Lock() + c.chat = m + c.mu.Unlock() +} + +func (c *agentModelCapture) Create() map[string]any { + c.mu.Lock() + defer c.mu.Unlock() + return c.create +} + +func (c *agentModelCapture) Chat() map[string]any { + c.mu.Lock() + defer c.mu.Unlock() + return c.chat +} + +// newAgentModelGateway fakes POST /v1/gateway/agents (agent creation) and +// POST /v1/gateway/agents/:id/chat (SSE chat) — the two endpoints /agent +// dispatch and /agent chat drive — capturing each request's decoded JSON +// body for assertions. The chat endpoint always ends the stream immediately +// with "data: [DONE]" so callers never block on the streamStallTimeout +// watchdog. +func newAgentModelGateway(t *testing.T) (*httptest.Server, *agentModelCapture) { + t.Helper() + capture := &agentModelCapture{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + body, _ := io.ReadAll(req.Body) + switch { + case req.Method == http.MethodPost && req.URL.Path == "/v1/gateway/agents": + capture.setCreate(body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"agent_id":"agent-under-test","status":"created"}`)) + case req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/chat"): + capture.setChat(body) + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("data: [DONE]\n\n")) + default: + http.NotFound(w, req) + } + })) + t.Cleanup(srv.Close) + return srv, capture +} + +// newAgentModelRegistry builds a Registry wired to gatewayURL with +// cfg.Delegation.Model set to delegationModel. Mirrors +// cmd_skill_external_test.go's newExternalSkillRegistry. HOME is isolated +// per-test (on top of the package-wide TestMain in commands_test.go) since +// /agent dispatch and /agent chat both persist to ~/.dojo/state.json. +func newAgentModelRegistry(t *testing.T, gatewayURL, delegationModel string) *Registry { + t.Helper() + t.Setenv("HOME", t.TempDir()) + session := "agent-model-test-session" + cfg := &config.Config{ + Gateway: config.GatewayConfig{URL: gatewayURL, Timeout: "5s"}, + Delegation: config.DelegationConfig{Model: delegationModel}, + } + r := &Registry{ + cfg: cfg, + gw: client.New(gatewayURL, "", "5s"), + cmds: make(map[string]Command), + plgs: []plugins.Plugin{}, + session: &session, + } + r.register() + return r +} + +// ─── /agent dispatch — flag parsing (both forms) ───────────────────────────── + +func TestAgentDispatchModelFlagSpaceForm(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch --model gpt-5-cheap do the thing") + }) + if err != nil { + t.Fatalf("agent dispatch: %v", err) + } + if got := capture.Create()["model"]; got != "gpt-5-cheap" { + t.Errorf("CreateAgentRequest.Model = %v, want gpt-5-cheap", got) + } + if got := capture.Chat()["model"]; got != "gpt-5-cheap" { + t.Errorf("AgentChatRequest.Model = %v, want gpt-5-cheap", got) + } + msg, _ := capture.Chat()["message"].(string) + if strings.Contains(msg, "--model") { + t.Errorf("--model flag leaked into the outgoing message: %q", msg) + } + if !strings.Contains(msg, "do the thing") { + t.Errorf("message missing expected words, got: %q", msg) + } + if !strings.Contains(out, "model: gpt-5-cheap (flag)") { + t.Errorf("banner line missing from output:\n%s", out) + } +} + +func TestAgentDispatchModelFlagEqualsForm(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch --model=gpt-5-cheap do the thing") + }) + if err != nil { + t.Fatalf("agent dispatch: %v", err) + } + if got := capture.Create()["model"]; got != "gpt-5-cheap" { + t.Errorf("CreateAgentRequest.Model = %v, want gpt-5-cheap", got) + } + msg, _ := capture.Chat()["message"].(string) + if strings.Contains(msg, "--model") { + t.Errorf("--model= flag leaked into the outgoing message: %q", msg) + } + if !strings.Contains(out, "model: gpt-5-cheap (flag)") { + t.Errorf("banner line missing from output:\n%s", out) + } +} + +func TestAgentDispatchModelFlagPositionIndependent(t *testing.T) { + cases := []struct { + name string + input string + }{ + {"flag_before_mode", "agent dispatch --model gpt-5-cheap focused do the thing"}, + {"flag_after_mode_mid_message", "agent dispatch focused do the --model gpt-5-cheap thing"}, + {"flag_at_end", "agent dispatch focused do the thing --model gpt-5-cheap"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), tc.input) + }) + if err != nil { + t.Fatalf("agent dispatch: %v", err) + } + if got := capture.Create()["active_mode"]; got != "focused" { + t.Errorf("ActiveMode = %v, want focused (mode-detection broke around the flag)", got) + } + if got := capture.Create()["model"]; got != "gpt-5-cheap" { + t.Errorf("CreateAgentRequest.Model = %v, want gpt-5-cheap", got) + } + msg, _ := capture.Chat()["message"].(string) + if strings.Contains(msg, "--model") { + t.Errorf("--model flag leaked into the outgoing message: %q", msg) + } + if !strings.Contains(msg, "do the thing") { + t.Errorf("message reconstruction broke around the flag, got: %q", msg) + } + if !strings.Contains(out, "Creating agent (mode: focused)") { + t.Errorf("mode banner missing or wrong:\n%s", out) + } + }) + } +} + +// ─── precedence: flag > delegation default > empty ────────────────────────── + +func TestAgentDispatchPrecedenceDelegationDefault(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "sonnet-delegate") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch do the thing") + }) + if err != nil { + t.Fatalf("agent dispatch: %v", err) + } + if got := capture.Create()["model"]; got != "sonnet-delegate" { + t.Errorf("CreateAgentRequest.Model = %v, want sonnet-delegate (delegation default)", got) + } + if !strings.Contains(out, "model: sonnet-delegate (delegation default)") { + t.Errorf("banner line missing from output:\n%s", out) + } +} + +func TestAgentDispatchPrecedenceFlagOverridesDelegationDefault(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "sonnet-delegate") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch --model opus-frontier do the thing") + }) + if err != nil { + t.Fatalf("agent dispatch: %v", err) + } + if got := capture.Create()["model"]; got != "opus-frontier" { + t.Errorf("CreateAgentRequest.Model = %v, want opus-frontier (flag must win over delegation default)", got) + } + if !strings.Contains(out, "model: opus-frontier (flag)") { + t.Errorf("banner line missing or wrong source:\n%s", out) + } +} + +func TestAgentDispatchPrecedenceEmptyOmitsModel(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch do the thing") + }) + if err != nil { + t.Fatalf("agent dispatch: %v", err) + } + if _, ok := capture.Create()["model"]; ok { + t.Errorf("CreateAgentRequest.Model should be omitted (empty+omitempty), got present: %v", capture.Create()["model"]) + } + if _, ok := capture.Chat()["model"]; ok { + t.Errorf("AgentChatRequest.Model should be omitted (empty+omitempty), got present: %v", capture.Chat()["model"]) + } + if strings.Contains(out, "model:") { + t.Errorf("no model banner expected when nothing resolved:\n%s", out) + } +} + +// ─── empty-value usage error ───────────────────────────────────────────────── + +func TestAgentDispatchModelFlagEmptyValueSpaceForm(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + _, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch --model") + }) + if err == nil { + t.Fatal("expected a usage error for a trailing --model with no value") + } + if !strings.Contains(err.Error(), "usage:") || !strings.Contains(err.Error(), "--model") { + t.Errorf("unexpected error text: %v", err) + } + if capture.Create() != nil { + t.Errorf("gateway must not be called on a malformed --model flag, got: %v", capture.Create()) + } +} + +func TestAgentDispatchModelFlagEmptyValueEqualsForm(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + _, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent dispatch --model= do the thing") + }) + if err == nil { + t.Fatal("expected a usage error for --model= with nothing after the equals") + } + if !strings.Contains(err.Error(), "usage:") { + t.Errorf("unexpected error text: %v", err) + } + if capture.Create() != nil { + t.Errorf("gateway must not be called on a malformed --model= flag, got: %v", capture.Create()) + } +} + +// ─── /agent chat — same flag surface on a different subcommand ────────────── + +func TestAgentChatModelFlagAndPrecedence(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "sonnet-delegate") + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent chat --model opus-frontier existing-agent-id hello there") + }) + if err != nil { + t.Fatalf("agent chat: %v", err) + } + if got := capture.Chat()["model"]; got != "opus-frontier" { + t.Errorf("AgentChatRequest.Model = %v, want opus-frontier (flag must win over delegation default)", got) + } + msg, _ := capture.Chat()["message"].(string) + if msg != "hello there" { + t.Errorf("message = %q, want %q (flag and agent-id must both be stripped)", msg, "hello there") + } + if !strings.Contains(out, "model: opus-frontier (flag)") { + t.Errorf("banner line missing from output:\n%s", out) + } + if capture.Create() != nil { + t.Errorf("/agent chat must not create a new agent, got a CreateAgent call: %v", capture.Create()) + } +} + +func TestAgentChatModelFlagEmptyValueUsageError(t *testing.T) { + srv, capture := newAgentModelGateway(t) + r := newAgentModelRegistry(t, srv.URL, "") + + _, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "agent chat --model= existing-agent-id hello") + }) + if err == nil { + t.Fatal("expected a usage error for --model= with nothing after the equals") + } + if !strings.Contains(err.Error(), "usage:") { + t.Errorf("unexpected error text: %v", err) + } + if capture.Chat() != nil { + t.Errorf("gateway must not be called on a malformed --model flag, got: %v", capture.Chat()) + } +} diff --git a/internal/commands/cmd_code.go b/internal/commands/cmd_code.go index da69d2b..2c2398b 100644 --- a/internal/commands/cmd_code.go +++ b/internal/commands/cmd_code.go @@ -11,9 +11,55 @@ import ( "path/filepath" "strings" + "github.com/DojoGenesis/cli/internal/permissions" gcolor "github.com/gookit/color" ) +// ─── permissions gate (shared by cmd_code.go, cmd_craft.go, cmd_plugin.go) ──── + +// permissionsModeAllowed returns the permission mode and allowlist from the +// registry config, tolerating a nil registry or nil cfg (zero Registry values +// appear in tests) by falling back to default-mode semantics. +func (r *Registry) permissionsModeAllowed() (string, []string) { + if r == nil || r.cfg == nil { + return "", nil + } + return r.cfg.Permissions.Mode, r.cfg.Permissions.Allowed +} + +// permissionGate evaluates action (a dot-path like "code.undo") against +// cfg.Permissions and reports whether the caller may proceed. It implements +// the standard gate flow for risky write/exec commands: +// +// - Allow (yolo, or matched by permissions.allowed): proceed silently. +// - Deny (allowlist mode, unmatched): print the one-line deny message and +// stop — callers return nil, the same clean exit other handlers use for +// user-facing cancellations. +// - Confirm (default mode, unmatched): prompt via +// permissions.ConfirmInteractive; a declined or non-interactive (no TTY) +// prompt prints the same deny message. +// +// preConfirmed marks consent the user already gave inline (e.g. the +// --yes / -y flag on /plugin install): it satisfies the Confirm case without +// re-prompting, but never overrides a Deny — an allowlist-mode refusal is +// policy, not a question. +func (r *Registry) permissionGate(action, detail string, preConfirmed bool) bool { + mode, allowed := r.permissionsModeAllowed() + switch permissions.Check(mode, allowed, action) { + case permissions.Allow: + return true + case permissions.Confirm: + if preConfirmed || permissions.ConfirmInteractive(action, detail) { + return true + } + } + // Deny, or a Confirm that was declined / had no terminal to ask on. + fmt.Println() + fmt.Println(" " + permissions.Explain(action)) + fmt.Println() + return false +} + func (r *Registry) codeCmd() Command { return Command{ Name: "code", @@ -40,7 +86,20 @@ func (r *Registry) codeCmd() Command { case "gate": return codeGate() case "undo": - return codeUndo() + // Permission gate: "code.undo" reverts working-tree changes. + // Allow (allowlisted / yolo) skips the in-function y/N prompt + // too — the gate already carries the consent; Confirm asks + // once here and then proceeds prompt-free, so no mode ever + // double-prompts. Direct codeUndo() calls (tests, future + // callers) keep the legacy prompt-below-preview behavior. + cwd, cwdErr := os.Getwd() + if cwdErr != nil || cwd == "" { + cwd = "the current directory" + } + if !r.permissionGate("code.undo", "revert all unstaged changes in "+cwd, false) { + return nil + } + return codeUndoPreconfirmed() default: return fmt.Errorf("unknown subcommand %q — try: read, diff, test, build, vet, gate, undo", sub) } @@ -189,6 +248,21 @@ func codeGate() error { // codeRead enforces for file reads). Neither step ever leaves that scope // or the enclosing git repository. func codeUndo() error { + return codeUndoRun(false) +} + +// codeUndoPreconfirmed is codeUndo with the in-function y/N prompt skipped: +// the /code dispatch path calls it after the "code.undo" permission gate has +// already carried the user's consent (Allow via allowlist/yolo, or an +// answered Confirm). Skipping the prompt here is what keeps every mode at +// one prompt maximum. +func codeUndoPreconfirmed() error { + return codeUndoRun(true) +} + +// codeUndoRun holds the shared preview + revert flow. preconfirmed=false is +// the legacy direct-call behavior: prompt below the preview via craftConfirm. +func codeUndoRun(preconfirmed bool) error { if _, err := exec.LookPath("git"); err != nil { return fmt.Errorf("git not found in PATH — /code undo requires git") } @@ -225,7 +299,8 @@ func codeUndo() error { // Reuses the same y/N stdin idiom as plugins.InstallConfirmed (see // cmd_plugin.go) — craftConfirm lives in cmd_craft.go but is // package-visible, so no duplicate confirm helper is needed here. - if !craftConfirm("Revert these unstaged changes?") { + // Skipped when the "code.undo" permission gate already confirmed. + if !preconfirmed && !craftConfirm("Revert these unstaged changes?") { gcolor.HEX("#94a3b8").Print(" Cancelled — no changes made.") fmt.Println() fmt.Println() diff --git a/internal/commands/cmd_craft.go b/internal/commands/cmd_craft.go index 885c80f..dc7a2b9 100644 --- a/internal/commands/cmd_craft.go +++ b/internal/commands/cmd_craft.go @@ -50,7 +50,7 @@ func (r *Registry) craftCmd() Command { case "view": return craftView(rest) case "scaffold": - return craftScaffold(rest) + return r.craftScaffold(rest) case "converge": return r.craftConverge(ctx) default: @@ -151,9 +151,10 @@ Be concise and precise. Focus on the decision, not the technology overview.`, ne fmt.Println() gcolor.HEX("#94a3b8").Printf(" target: %s\n\n", path) - if !craftConfirm(fmt.Sprintf("Write ADR to %s?", path)) { - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" Cancelled — ADR not written.")) - fmt.Println() + // Permission gate "craft.adr" replaces the old ad-hoc y/N confirm at the + // same site: the write is the risky part, so the generated ADR streams to + // screen regardless and only the file write is gated (one prompt max). + if !r.permissionGate("craft.adr", "write "+path, false) { return nil } @@ -375,9 +376,13 @@ Be specific. Do not suggest generic "add more documentation".` } fmt.Println() - if !craftConfirm(fmt.Sprintf("Write %d fixed file(s)?", len(relPaths))) { - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" Cancelled — no files modified.")) - fmt.Println() + // Permission gate "craft.claude-md" replaces the old ad-hoc y/N confirm + // at the same site. Only the --fix write path is gated — the analysis + // above is read-only and prints for every mode, exactly like a run + // without --fix. + if !r.permissionGate("craft.claude-md", + fmt.Sprintf("rewrite %d CLAUDE.md file(s) in place under %s", len(relPaths), cwd), + false) { return nil } @@ -864,7 +869,10 @@ func craftView(args []string) error { // ─── /craft scaffold ───────────────────────────────────────────────────────── -func craftScaffold(args []string) error { +// craftScaffold is a Registry method (not a plain func) solely so the +// "craft.scaffold" permission gate can reach r.cfg.Permissions; the no-args +// template listing below stays ungated (read-only help). +func (r *Registry) craftScaffold(args []string) error { if len(args) == 0 { fmt.Println() gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprint(" Available scaffold templates:")) @@ -952,9 +960,12 @@ func craftScaffold(args []string) error { fmt.Println() gcolor.HEX("#94a3b8").Printf(" target: %s\n\n", cwd) - if !craftConfirm(fmt.Sprintf("Create %d dir(s) and %d file(s) in %s?", len(dirs), len(files), cwd)) { - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" Cancelled — nothing created.")) - fmt.Println() + // Permission gate "craft.scaffold" replaces the old ad-hoc y/N confirm at + // the same site, after the create-plan listing above so a Confirm-mode + // user decides with the full file list on screen. + if !r.permissionGate("craft.scaffold", + fmt.Sprintf("create %d dir(s) and %d file(s) in %s", len(dirs), len(files), cwd), + false) { return nil } fmt.Println() @@ -1094,10 +1105,14 @@ func (r *Registry) craftConverge(ctx context.Context) error { // craftConfirm prints a y/N prompt to stderr and blocks for a line of input on // stdin, returning true only for an explicit "y" or "yes" (case-insensitive). -// Mirrors the confirmation style of plugins.InstallConfirmed (see cmd_plugin.go -// / internal/plugins/installer.go) but stays file-local: every /craft -// write/delete path below needs the same guard, and cmd_craft.go is the only -// file this session is allowed to touch. +// Mirrors the confirmation style plugins.InstallConfirmed uses internally. +// +// Since the permissions gate landed (see Registry.permissionGate in +// cmd_code.go), this helper no longer guards the FILE-writing craft actions — +// adr, claude-md --fix, and scaffold now flow through permissionGate with +// keys "craft.<action>". It remains the guard for gateway-side (remote +// memory-state) mutations that are not file writes — /craft memory prune and +// /craft seed elevate — and for the legacy direct-call path of codeUndo(). func craftConfirm(prompt string) bool { fmt.Fprintf(os.Stderr, " %s [y/N]: ", prompt) reader := bufio.NewReader(os.Stdin) diff --git a/internal/commands/cmd_model.go b/internal/commands/cmd_model.go index 893eca9..046233b 100644 --- a/internal/commands/cmd_model.go +++ b/internal/commands/cmd_model.go @@ -65,6 +65,7 @@ func (r *Registry) modelList(ctx context.Context) error { // Gateway is offline — that's fine, catalog was already shown. fmt.Println(gcolor.HEX("#94a3b8").Sprint(" (gateway unreachable — showing catalog only)")) fmt.Println() + r.printDelegationDefault() return nil } gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Gateway models (%d)", len(gwModels))) @@ -77,6 +78,7 @@ func (r *Registry) modelList(ctx context.Context) error { ) } fmt.Println() + r.printDelegationDefault() return nil } @@ -92,9 +94,27 @@ func (r *Registry) modelList(ctx context.Context) error { fmt.Printf(" %s %s%s\n", gcolor.HEX("#f4a261").Sprintf("%-20s", p.Name), status, caps) } fmt.Println() + r.printDelegationDefault() return nil } +// printDelegationDefault appends one line to /model ls output naming the +// /agent dispatch delegation-model default (cfg.Delegation.Model), when set. +// This is local config state — independent of gateway connectivity — so +// modelList calls it from all three of its exit paths (catalog-only, +// gwModels fallback, and gwProviders), and it is a silent no-op when the +// delegation default is unset (""). +func (r *Registry) printDelegationDefault() { + if r.cfg.Delegation.Model == "" { + return + } + fmt.Printf(" %s\n", gcolor.HEX("#94a3b8").Sprintf( + "delegation default: %s (used by /agent dispatch unless --model is given)", + r.cfg.Delegation.Model, + )) + fmt.Println() +} + func (r *Registry) modelSet(ctx context.Context, args []string) error { // /model set <model> OR /model set <provider> <model> var newProvider, newModel, oldModel string diff --git a/internal/commands/cmd_permissions_gate_test.go b/internal/commands/cmd_permissions_gate_test.go new file mode 100644 index 0000000..6b3b337 --- /dev/null +++ b/internal/commands/cmd_permissions_gate_test.go @@ -0,0 +1,267 @@ +package commands + +// cmd_permissions_gate_test.go — end-to-end tests for the permissions gate on +// risky write/exec commands (code.undo, plugin.install, plugin.rm, +// craft.scaffold). New file per the phase-2 track rules: existing test files +// are never edited; shared helpers from them are reused — +// testRegistry (commands_test.go), initTempGitRepo / runGitSetup / +// withTempCwd / withStdin (cmd_code_test.go), and captureProtocolStdout +// (cmd_protocol_test.go), which also captures the gookit/color writer. + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/DojoGenesis/cli/internal/config" +) + +// permTestRegistry builds the standard test registry and applies the given +// permissions config to it. +func permTestRegistry(t *testing.T, mode string, allowed []string) *Registry { + t.Helper() + r, _ := testRegistry() + r.cfg.Permissions = config.PermissionsConfig{Mode: mode, Allowed: allowed} + return r +} + +// dirtyGitRepo creates a temp git repo (chdir'd into, cleaned up +// automatically), commits tracked.txt at "v1\n", then leaves an unstaged edit +// "v2 unstaged\n" — the exact state /code undo exists to revert. +func dirtyGitRepo(t *testing.T) { + t.Helper() + initTempGitRepo(t) + if err := os.WriteFile("tracked.txt", []byte("v1\n"), 0o600); err != nil { + t.Fatalf("setup tracked.txt: %v", err) + } + runGitSetup(t, "add", "tracked.txt") + runGitSetup(t, "commit", "-q", "-m", "initial") + if err := os.WriteFile("tracked.txt", []byte("v2 unstaged\n"), 0o600); err != nil { + t.Fatalf("unstaged edit: %v", err) + } +} + +func mustReadTracked(t *testing.T) string { + t.Helper() + got, err := os.ReadFile("tracked.txt") + if err != nil { + t.Fatalf("reading tracked.txt: %v", err) + } + return string(got) +} + +// ─── code.undo ──────────────────────────────────────────────────────────────── + +// TestPermGateCodeUndoAllowlistDeny — allowlist mode without "code.undo" +// listed refuses the revert: the Explain message reaches output, the handler +// exits cleanly (nil error), and the working tree is untouched. +func TestPermGateCodeUndoAllowlistDeny(t *testing.T) { + dirtyGitRepo(t) + // An unrelated pattern proves deny comes from non-matching, not an empty list. + r := permTestRegistry(t, "allowlist", []string{"craft.*"}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "code undo") + }) + if err != nil { + t.Fatalf("denied /code undo should exit cleanly, got error: %v", err) + } + if !strings.Contains(out, `permission denied: "code.undo"`) { + t.Errorf("output missing Explain message for code.undo:\n%s", out) + } + if got := mustReadTracked(t); got != "v2 unstaged\n" { + t.Errorf("tracked.txt = %q after denied undo, want untouched %q", got, "v2 unstaged\n") + } +} + +// TestPermGateCodeUndoAllowlistAllow — with "code.undo" allowlisted the gate +// lets the handler proceed AND skips the interactive prompt entirely: no +// stdin is provided here, so if any prompt were still attempted the answer +// would read as empty/cancel and the file would not revert. +func TestPermGateCodeUndoAllowlistAllow(t *testing.T) { + dirtyGitRepo(t) + r := permTestRegistry(t, "allowlist", []string{"code.undo"}) + + _, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "code undo") + }) + if err != nil { + t.Fatalf("allowlisted /code undo: unexpected error: %v", err) + } + if got := mustReadTracked(t); got != "v1\n" { + t.Errorf("tracked.txt = %q after allowlisted undo, want reverted %q", got, "v1\n") + } +} + +// TestPermGateCodeUndoDefaultNonTTYRefuses — in default (Confirm) mode with a +// non-terminal stdin, ConfirmInteractive must short-circuit to refusal +// WITHOUT reading stdin: an affirmative "y" sits unread in the pipe, and the +// tree must stay untouched with the Explain message printed. +func TestPermGateCodeUndoDefaultNonTTYRefuses(t *testing.T) { + dirtyGitRepo(t) + r := permTestRegistry(t, "default", nil) + + withStdin(t, "y\n") // a pipe is never a terminal; the "y" must not be consumed + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "code undo") + }) + if err != nil { + t.Fatalf("non-TTY refused /code undo should exit cleanly, got error: %v", err) + } + if !strings.Contains(out, `permission denied: "code.undo"`) { + t.Errorf("output missing Explain message for non-TTY confirm refusal:\n%s", out) + } + if got := mustReadTracked(t); got != "v2 unstaged\n" { + t.Errorf("tracked.txt = %q, want untouched %q — the piped 'y' must not authorize a revert", got, "v2 unstaged\n") + } +} + +// ─── plugin.install ─────────────────────────────────────────────────────────── + +// TestPermGatePluginInstallAllowlistDeny — allowlist mode without +// "plugin.install" listed refuses before any clone activity: Explain reaches +// output, no "Cloning" line is printed, and the handler exits cleanly. +func TestPermGatePluginInstallAllowlistDeny(t *testing.T) { + r := permTestRegistry(t, "allowlist", []string{"code.undo"}) + r.cfg.Plugins.Path = t.TempDir() + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "plugin install https://example.invalid/some-plugin.git") + }) + if err != nil { + t.Fatalf("denied /plugin install should exit cleanly, got error: %v", err) + } + if !strings.Contains(out, `permission denied: "plugin.install"`) { + t.Errorf("output missing Explain message for plugin.install:\n%s", out) + } + if strings.Contains(out, "Cloning") { + t.Errorf("denied install must not reach the clone step, but output shows Cloning:\n%s", out) + } + entries, readErr := os.ReadDir(r.cfg.Plugins.Path) + if readErr != nil { + t.Fatalf("reading plugins dir: %v", readErr) + } + if len(entries) != 0 { + t.Errorf("plugins dir should stay empty after a denied install, has %d entries", len(entries)) + } +} + +// TestPermGatePluginInstallAllowlistAllow — with "plugin.install" allowlisted +// the gate lets the handler proceed to the actual clone (no prompt: stdin is +// not consulted). The clone itself fails — the URL points at a nonexistent +// local path — which is exactly the proof that execution got PAST the gate: +// the failure is git's, not the permission system's. +func TestPermGatePluginInstallAllowlistAllow(t *testing.T) { + r := permTestRegistry(t, "allowlist", []string{"plugin.install"}) + r.cfg.Plugins.Path = t.TempDir() + bogusURL := filepath.Join(t.TempDir(), "no-such-repo.git") // local path: fails fast, no network + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "plugin install "+bogusURL) + }) + if err == nil { + t.Fatal("expected a git clone failure error, got nil") + } + if !strings.Contains(err.Error(), "git clone failed") { + t.Errorf("error = %v; want a 'git clone failed' error proving the gate was passed", err) + } + if strings.Contains(out, "permission denied") { + t.Errorf("allowlisted install must not print a permission denial:\n%s", out) + } + if !strings.Contains(out, "Cloning") { + t.Errorf("allowlisted install should reach the Cloning step:\n%s", out) + } +} + +// ─── plugin.rm ──────────────────────────────────────────────────────────────── + +// TestPermGatePluginRmAllowlistDeny — the gate fires before Uninstall even +// looks for the plugin: a denied rm of a nonexistent plugin returns nil (no +// "not found" error) with the Explain message printed. +func TestPermGatePluginRmAllowlistDeny(t *testing.T) { + r := permTestRegistry(t, "allowlist", nil) + r.cfg.Plugins.Path = t.TempDir() + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "plugin rm ghost-plugin") + }) + if err != nil { + t.Fatalf("denied /plugin rm should exit cleanly, got error: %v", err) + } + if !strings.Contains(out, `permission denied: "plugin.rm"`) { + t.Errorf("output missing Explain message for plugin.rm:\n%s", out) + } +} + +// TestPermGatePluginRmYoloAllow — yolo mode passes the gate for everything; +// the surviving "not found" error from Uninstall proves the handler executed +// past the gate. +func TestPermGatePluginRmYoloAllow(t *testing.T) { + r := permTestRegistry(t, "yolo", nil) + r.cfg.Plugins.Path = t.TempDir() + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "plugin rm ghost-plugin") + }) + if err == nil { + t.Fatal("expected 'not found' error from Uninstall after passing the yolo gate, got nil") + } + if !strings.Contains(err.Error(), "not found") { + t.Errorf("error = %v; want the Uninstall 'not found' error proving the gate was passed", err) + } + if strings.Contains(out, "permission denied") { + t.Errorf("yolo mode must not print a permission denial:\n%s", out) + } +} + +// ─── craft.scaffold ─────────────────────────────────────────────────────────── + +// TestPermGateCraftScaffoldAllowlistDeny — a denied scaffold prints Explain +// after the create-plan listing and writes nothing. +func TestPermGateCraftScaffoldAllowlistDeny(t *testing.T) { + withTempCwd(t) + r := permTestRegistry(t, "allowlist", []string{"plugin.install"}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "craft scaffold minimal") + }) + if err != nil { + t.Fatalf("denied /craft scaffold should exit cleanly, got error: %v", err) + } + if !strings.Contains(out, `permission denied: "craft.scaffold"`) { + t.Errorf("output missing Explain message for craft.scaffold:\n%s", out) + } + if _, statErr := os.Stat("go.mod"); statErr == nil { + t.Error("go.mod was created despite the scaffold being denied") + } + if _, statErr := os.Stat("src"); statErr == nil { + t.Error("src/ was created despite the scaffold being denied") + } +} + +// TestPermGateCraftScaffoldGlobAllow — "craft.*" in the allowlist covers +// craft.scaffold via the glob form; the scaffold proceeds prompt-free and +// actually writes its files. +func TestPermGateCraftScaffoldGlobAllow(t *testing.T) { + withTempCwd(t) + r := permTestRegistry(t, "allowlist", []string{"craft.*"}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "craft scaffold minimal") + }) + if err != nil { + t.Fatalf("glob-allowlisted /craft scaffold: unexpected error: %v", err) + } + if strings.Contains(out, "permission denied") { + t.Errorf("glob-allowlisted scaffold must not print a permission denial:\n%s", out) + } + if _, statErr := os.Stat("go.mod"); statErr != nil { + t.Errorf("go.mod missing after allowed scaffold: %v", statErr) + } + if info, statErr := os.Stat("src"); statErr != nil || !info.IsDir() { + t.Errorf("src/ missing after allowed scaffold (err=%v)", statErr) + } +} diff --git a/internal/commands/cmd_plugin.go b/internal/commands/cmd_plugin.go index d5c8616..2539b8a 100644 --- a/internal/commands/cmd_plugin.go +++ b/internal/commands/cmd_plugin.go @@ -74,12 +74,26 @@ func (r *Registry) pluginList(ctx context.Context) error { // pluginInstall clones a plugin from a git URL and rescans. // A single URL may yield multiple plugins (monorepo case). -// noConfirm skips the interactive trust prompt (--yes / -y flag). +// +// The "plugin.install" permission gate is the single confirmation for this +// command: it replaced the ad-hoc y/N prompt inside plugins.InstallConfirmed +// (which is now always called with noConfirm=true — one prompt, never two). +// noConfirm here (--yes / -y) pre-answers the gate's Confirm case, matching +// the flag's historical "skip the interactive prompt" contract; it does not +// override an allowlist-mode Deny. InstallConfirmed itself is retained (it +// lives in internal/plugins and still prints the security warning banner), +// but its prompt path is dead from this call site. func (r *Registry) pluginInstall(ctx context.Context, gitURL string, noConfirm bool) error { + if !r.permissionGate("plugin.install", + fmt.Sprintf("clone %s into %s and load its hooks/skills", gitURL, r.cfg.Plugins.Path), + noConfirm) { + return nil + } + fmt.Println() fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" Cloning %s ...", gitURL)) - results, err := plugins.InstallConfirmed(gitURL, r.cfg.Plugins.Path, noConfirm) + results, err := plugins.InstallConfirmed(gitURL, r.cfg.Plugins.Path, true) if err != nil { return fmt.Errorf("plugin install: %w", err) } @@ -112,8 +126,16 @@ func (r *Registry) pluginInstall(ctx context.Context, gitURL string, noConfirm b return nil } -// pluginRemove removes an installed plugin by name and rescans. +// pluginRemove removes an installed plugin by name and rescans. Gated as +// "plugin.rm" — removal deletes the plugin directory from disk, and this +// command historically had no confirmation at all. func (r *Registry) pluginRemove(ctx context.Context, name string) error { + if !r.permissionGate("plugin.rm", + fmt.Sprintf("delete plugin %q from %s", name, r.cfg.Plugins.Path), + false) { + return nil + } + if err := plugins.Uninstall(name, r.cfg.Plugins.Path); err != nil { return fmt.Errorf("plugin remove: %w", err) } diff --git a/internal/commands/cmd_skill.go b/internal/commands/cmd_skill.go new file mode 100644 index 0000000..d167e15 --- /dev/null +++ b/internal/commands/cmd_skill.go @@ -0,0 +1,612 @@ +package commands + +// cmd_skill.go — the /skill command: list, fetch, and inspect skills from the +// gateway CAS, plus read-only discovery of external skills (foreign agent +// ecosystems' SKILL.md dirs — Claude Code's .claude/skills, Cursor's +// .cursor/skills, ~/.agents/skills) via cfg.Skills.ExternalDirs. +// +// The gateway-backed subcommands moved here verbatim from cmd_workflow.go +// (pure extraction; no behavior change to them). External skills are strictly +// supplementary and READ-ONLY: they are listed after the gateway listing, +// resolvable via the explicit `ext:` prefix (or as a fallback when the +// gateway tag lookup misses), and are never offered for packaging or pushing +// — promoted knowledge is reference material, not configuration. + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "time" + + "github.com/DojoGenesis/cli/internal/client" + "github.com/DojoGenesis/cli/internal/mdrender" + "github.com/DojoGenesis/cli/internal/skills" + gcolor "github.com/gookit/color" +) + +// ─── /skill ───────────────────────────────────────────────────────────────── + +func (r *Registry) skillCmd() Command { + return Command{ + Name: "skill", + Aliases: []string{"skills"}, + Usage: "/skill [ls [filter]|search <query>|get <name>|inspect <hash>|tags|package-all <dir>]", + Short: "List, fetch, or inspect skills from CAS", + Run: func(ctx context.Context, args []string) error { + sub := "ls" + if len(args) > 0 { + sub = strings.ToLower(args[0]) + } + + switch sub { + case "search": + // /skill search <query> + if len(args) < 2 { + return fmt.Errorf("usage: /skill search <query>") + } + query := strings.Join(args[1:], " ") + skills, err := r.gw.SearchSkills(ctx, query) + if err != nil { + return fmt.Errorf("search failed: %w", err) + } + + fmt.Println() + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Skills matching %q (%d)\n\n", query, len(skills))) + + if len(skills) == 0 { + fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No matching skills found.")) + fmt.Println() + return nil + } + + for _, s := range skills { + name := gcolor.HEX("#f4a261").Sprintf("%-40s", s.Name) + cat := "" + if s.Category != "" { + cat = gcolor.HEX("#94a3b8").Sprintf("[%s]", s.Category) + } + plugin := "" + if s.Plugin != "" { + plugin = gcolor.HEX("#64748b").Sprintf(" (%s)", s.Plugin) + } + fmt.Printf(" %s %s%s\n", name, cat, plugin) + if s.Description != "" { + desc := s.Description + if len(desc) > 100 { + desc = desc[:97] + "..." + } + fmt.Printf(" %s\n", gcolor.HEX("#94a3b8").Sprint(" "+desc)) + } + } + fmt.Println() + return nil + + case "get": + // /skill get <name> + if len(args) < 2 { + return fmt.Errorf("usage: /skill get <name>") + } + name := args[1] + // Explicit external namespace: /skill get ext:<name> never + // touches the gateway — read-only local resolution only. + if extName, isExt := strings.CutPrefix(name, "ext:"); isExt { + ext := skills.FindExternal(r.externalSkillDirs(), extName) + if ext == nil { + return fmt.Errorf("external skill %q not found", extName) + } + return printExternalSkill(ext) + } + tag, err := r.gw.CASResolveTag(ctx, name, "latest") + if err != nil { + // Gateway lookup missed: fall back to external skills + // (read-only). A miss on both surfaces the existing + // gateway error unchanged. + if ext := skills.FindExternal(r.externalSkillDirs(), name); ext != nil { + return printExternalSkill(ext) + } + return fmt.Errorf("could not resolve tag %q: %w", name, err) + } + content, err := r.gw.CASGetContent(ctx, tag.Ref) + if err != nil { + return fmt.Errorf("could not fetch content for ref %q: %w", tag.Ref, err) + } + fmt.Println() + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Skill: %s @ %s\n\n", tag.Name, tag.Version)) + printKV("ref", tag.Ref) + fmt.Println() + fmt.Println(mdrender.RenderMarkdown(string(content))) + fmt.Println() + return nil + + case "inspect": + // /skill inspect <hash> + if len(args) < 2 { + return fmt.Errorf("usage: /skill inspect <hash>") + } + ref := args[1] + content, err := r.gw.CASGetContent(ctx, ref) + if err != nil { + return fmt.Errorf("could not fetch content for ref %q: %w", ref, err) + } + fmt.Println() + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" CAS ref: %s\n\n", ref)) + fmt.Println(gcolor.White.Sprint(string(content))) + fmt.Println() + return nil + + case "tags": + // /skill tags + tags, err := r.gw.CASListTags(ctx) + if err != nil { + return fmt.Errorf("could not list CAS tags: %w", err) + } + fmt.Println() + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" CAS Tags (%d)\n\n", len(tags))) + if len(tags) == 0 { + fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No tags found.")) + fmt.Println() + return nil + } + // Table header + fmt.Printf(" %s %s %s\n", + gcolor.HEX("#94a3b8").Sprintf("%-32s", "Name"), + gcolor.HEX("#94a3b8").Sprintf("%-12s", "Version"), + gcolor.HEX("#94a3b8").Sprint("Ref"), + ) + fmt.Printf(" %s\n", gcolor.HEX("#64748b").Sprint(strings.Repeat("─", 72))) + for _, t := range tags { + fmt.Printf(" %s %s %s\n", + gcolor.HEX("#f4a261").Sprintf("%-32s", truncate(t.Name, 32)), + gcolor.White.Sprintf("%-12s", truncate(t.Version, 12)), + gcolor.HEX("#94a3b8").Sprint(truncate(t.Ref, 20)), + ) + } + fmt.Println() + return nil + + case "package-all": + // /skill package-all [dir] + // Walk a directory for SKILL.md files, put each into CAS, and create tags. + // Default source: $DOJO_SKILLS_PATH or current directory. + // NOTE: this deliberately walks ONLY its explicit arg or + // $DOJO_SKILLS_PATH — never cfg.Skills.ExternalDirs. External + // skills are read-only and are never packaged or pushed. + dir := os.Getenv("DOJO_SKILLS_PATH") + if len(args) >= 2 { + dir = args[1] + } + if dir == "" { + return fmt.Errorf("usage: /skill package-all <dir>\n or set DOJO_SKILLS_PATH") + } + + // Walk for SKILL.md files. + var skills []string + err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return nil // skip inaccessible + } + if !info.IsDir() && info.Name() == "SKILL.md" { + skills = append(skills, path) + } + return nil + }) + if err != nil { + return fmt.Errorf("walking %s: %w", dir, err) + } + + if len(skills) == 0 { + fmt.Println() + fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" No SKILL.md files found in %s", dir)) + fmt.Println() + return nil + } + + fmt.Println() + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Packaging %d skills from %s\n\n", len(skills), dir)) + + var succeeded, failed int + for _, path := range skills { + // Derive skill name from parent directory. + skillName := filepath.Base(filepath.Dir(path)) + + content, err := os.ReadFile(path) + if err != nil { + gcolor.HEX("#ef4444").Printf(" [FAIL] %s: %s\n", skillName, err) + failed++ + continue + } + + // Put content into CAS. + // Sleep before each API call: gateway allows 300 req/min (5/sec) sustained, + // burst 50. Each skill makes 2 calls, so 250ms per call = 4 req/sec total. + ref, err := r.gw.CASPutContent(ctx, content) + time.Sleep(250 * time.Millisecond) + if err != nil { + gcolor.HEX("#ef4444").Printf(" [FAIL] %s: CAS put: %s\n", skillName, err) + failed++ + continue + } + + // Create tag: name@latest -> ref. + if err := r.gw.CASCreateTag(ctx, skillName, "latest", ref); err != nil { + gcolor.HEX("#ef4444").Printf(" [FAIL] %s: tag create: %s\n", skillName, err) + failed++ + time.Sleep(250 * time.Millisecond) + continue + } + time.Sleep(250 * time.Millisecond) + + gcolor.HEX("#22c55e").Printf(" [OK] %s → %s\n", skillName, shortRef(ref)) + succeeded++ + } + + fmt.Println() + summary := fmt.Sprintf(" Done: %d succeeded, %d failed", succeeded, failed) + if failed > 0 { + gcolor.HEX("#eab308").Println(summary) + } else { + gcolor.HEX("#22c55e").Println(summary) + } + fmt.Println() + return nil + + default: // ls / all / filter — main skill browser + filter, showAll, page := parseSkillLsArgs(args) + + rawSkills, err := r.gw.Skills(ctx) + if err != nil { + return fmt.Errorf("could not fetch skills: %w", err) + } + // Fill in missing categories via semantic clustering so skills + // group correctly regardless of gateway metadata completeness. + skillList := skills.EnrichCategories(rawSkills) + + if len(skillList) == 0 { + fmt.Println() + fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No skills found.")) + fmt.Println() + r.printExternalSkillsSection() + return nil + } + + // Category summary: no filter, no explicit "all", page 1. + if filter == "" && !showAll && page == 1 { + if err := printSkillCategorySummary(skillList); err != nil { + return err + } + r.printExternalSkillsSection() + return nil + } + + // Filter by name, category, or plugin. + displaySkills := skillList + if filter != "" { + fl := strings.ToLower(filter) + var matched []client.Skill + for _, s := range skillList { + if strings.Contains(strings.ToLower(s.Name), fl) || + strings.Contains(strings.ToLower(s.Category), fl) || + strings.Contains(strings.ToLower(s.Plugin), fl) { + matched = append(matched, s) + } + } + displaySkills = matched + } + + if err := printSkillsPage(displaySkills, filter, page); err != nil { + return err + } + r.printExternalSkillsSection() + return nil + } + }, + } +} + +// ─── /skill helpers ────────────────────────────────────────────────────────── + +const skillPageSize = 30 + +// shortRef returns the first 12 characters of a CAS ref for compact display, +// or ref unchanged if it is shorter than that. ref comes straight from the +// gateway response (CASPutContent) with no length guarantee — an unguarded +// ref[:12] slice panics (and previously crashed the whole CLI mid +// /skill package-all) whenever the gateway returns a short ref. +func shortRef(ref string) string { + if len(ref) >= 12 { + return ref[:12] + } + return ref +} + +// parseSkillLsArgs extracts (filter, showAll, page) from the args slice. +// Recognises: "all", "p<N>" or plain integers as page numbers, everything else +// is joined as the filter term. +func parseSkillLsArgs(args []string) (filter string, showAll bool, page int) { + page = 1 + var filterParts []string + for _, a := range args { + al := strings.ToLower(a) + if al == "ls" { + continue + } + if al == "all" { + showAll = true + continue + } + // p<N> — page number + if strings.HasPrefix(al, "p") { + if n, err := strconv.Atoi(al[1:]); err == nil && n >= 1 { + page = n + continue + } + } + // bare integer — page number + if n, err := strconv.Atoi(a); err == nil && n >= 1 { + page = n + continue + } + filterParts = append(filterParts, a) + } + filter = strings.Join(filterParts, " ") + return +} + +// skillCategoryOrder groups skills by category. Returns (map, ordered keys sorted by count desc). +func skillCategoryOrder(skills []client.Skill) (map[string][]client.Skill, []string) { + cats := map[string][]client.Skill{} + for _, s := range skills { + cat := s.Category + if cat == "" { + cat = "general" + } + cats[cat] = append(cats[cat], s) + } + keys := make([]string, 0, len(cats)) + for k := range cats { + keys = append(keys, k) + } + sort.Slice(keys, func(i, j int) bool { + ci, cj := len(cats[keys[i]]), len(cats[keys[j]]) + if ci != cj { + return ci > cj + } + return keys[i] < keys[j] + }) + return cats, keys +} + +// printSkillCategorySummary renders the landing page for /skill ls (no args). +func printSkillCategorySummary(skills []client.Skill) error { + cats, order := skillCategoryOrder(skills) + + // Count distinct plugins. + pluginSet := map[string]struct{}{} + for _, s := range skills { + if s.Plugin != "" { + pluginSet[s.Plugin] = struct{}{} + } + } + + fmt.Println() + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf( + " Skills %d total · %d categories · %d plugins\n\n", + len(skills), len(order), len(pluginSet), + )) + + divider := gcolor.HEX("#334155").Sprint(strings.Repeat("─", 66)) + fmt.Println(" " + divider) + + for _, cat := range order { + count := len(cats[cat]) + bar := buildMiniBar(count, len(skills), 12) + fmt.Printf(" %s %s %s\n", + gcolor.HEX("#f4a261").Sprintf("%-28s", cat), + gcolor.HEX("#94a3b8").Sprintf("%3d", count)+" "+gcolor.HEX("#334155").Sprint(bar), + gcolor.HEX("#64748b").Sprintf("/skill ls %s", cat), + ) + } + + fmt.Println(" " + divider) + fmt.Println() + fmt.Printf(" %s /skill ls all %s /skill ls all p2\n", + gcolor.HEX("#94a3b8").Sprint("list all:"), + gcolor.HEX("#94a3b8").Sprint("next page:"), + ) + fmt.Printf(" %s /skill search <query>\n", + gcolor.HEX("#94a3b8").Sprint("search: "), + ) + fmt.Println() + return nil +} + +// buildMiniBar returns a fixed-width ASCII progress bar. +func buildMiniBar(count, total, width int) string { + if total == 0 || width == 0 { + return strings.Repeat("░", width) + } + filled := count * width / total + if filled == 0 && count > 0 { + filled = 1 + } + return strings.Repeat("█", filled) + strings.Repeat("░", width-filled) +} + +// printSkillsPage renders a paginated grouped skill list. +// Pages are built by complete category: a new page starts only after a full +// category has been rendered, so categories are never split mid-display. +func printSkillsPage(skills []client.Skill, filter string, page int) error { + if len(skills) == 0 { + fmt.Println() + if filter != "" { + fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" No skills matching %q.", filter)) + } else { + fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No skills found.")) + } + fmt.Println() + return nil + } + + cats, order := skillCategoryOrder(skills) + + // Build pages by grouping complete categories until skillPageSize is reached. + type pageSlice struct { + cats []string + count int + } + var pages []pageSlice + var cur pageSlice + for _, cat := range order { + cur.cats = append(cur.cats, cat) + cur.count += len(cats[cat]) + if cur.count >= skillPageSize { + pages = append(pages, cur) + cur = pageSlice{} + } + } + if len(cur.cats) > 0 { + pages = append(pages, cur) + } + + totalPages := len(pages) + if page < 1 { + page = 1 + } + if page > totalPages { + page = totalPages + } + pg := pages[page-1] + + // Count skills on this page. + pageCount := 0 + for _, cat := range pg.cats { + pageCount += len(cats[cat]) + } + + fmt.Println() + label := "Skills" + if filter != "" { + label = fmt.Sprintf("Skills › %s", filter) + } + pageLabel := "" + if totalPages > 1 { + pageLabel = fmt.Sprintf(" · page %d of %d", page, totalPages) + } + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" %s (%d of %d%s)\n\n", label, pageCount, len(skills), pageLabel)) + + // Render each category group. + for _, cat := range pg.cats { + fmt.Printf(" %s %s %s\n", + gcolor.HEX("#334155").Sprint("────"), + gcolor.HEX("#e8b04a").Sprint("["+cat+"]"), + gcolor.HEX("#334155").Sprint("────────────────────────────────────────────────"), + ) + for _, s := range cats[cat] { + plugin := "" + if s.Plugin != "" { + plugin = gcolor.HEX("#64748b").Sprintf("(%s)", s.Plugin) + } + fmt.Printf(" %s %s\n", + gcolor.HEX("#f4a261").Sprintf("%-40s", truncate(s.Name, 40)), + plugin, + ) + } + } + + // Footer navigation hints. + fmt.Println() + if totalPages > 1 { + base := "/skill ls" + if filter != "" { + base += " " + filter + } else { + base += " all" + } + if page < totalPages { + fmt.Printf(" %s %s p%d\n", + gcolor.HEX("#94a3b8").Sprint("next:"), + gcolor.HEX("#64748b").Sprint(base), + page+1, + ) + } + if page > 1 { + fmt.Printf(" %s %s p%d\n", + gcolor.HEX("#94a3b8").Sprint("prev:"), + gcolor.HEX("#64748b").Sprint(base), + page-1, + ) + } + } + fmt.Printf(" %s /skill search <query>\n", gcolor.HEX("#94a3b8").Sprint("search:")) + fmt.Println() + return nil +} + +// ─── external skills (read-only) ───────────────────────────────────────────── + +// externalSkillDirs returns the configured external skill directories +// (cfg.Skills.ExternalDirs), or nil when no config is attached. +func (r *Registry) externalSkillDirs() []string { + if r.cfg == nil { + return nil + } + return r.cfg.Skills.ExternalDirs +} + +// printExternalSkillsSection appends the supplementary "External (read-only)" +// section to /skill ls output. An empty scan prints nothing at all — the +// section is supplementary, so its definitive empty state is silence, not an +// empty header. +func (r *Registry) printExternalSkillsSection() { + ext := skills.ScanExternal(r.externalSkillDirs()) + if len(ext) == 0 { + return + } + gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprint(" External (read-only)\n\n")) + for _, s := range ext { + desc := "" + if s.Description != "" { + desc = " — " + clipRunes(s.Description, 100) + } + fmt.Printf(" %s%s %s\n", + gcolor.HEX("#f4a261").Sprint("ext:"+s.Name), + gcolor.HEX("#94a3b8").Sprint(desc), + gcolor.HEX("#64748b").Sprintf("(%s)", s.SourceDir), + ) + } + fmt.Println() +} + +// printExternalSkill renders one external skill's SKILL.md through the same +// markdown path /skill get uses for CAS skills, prefixed by a read-only +// header naming the source file. External skills are never packaged, pushed, +// or otherwise written — display only. +func printExternalSkill(ext *skills.ExternalSkill) error { + content, err := os.ReadFile(ext.Path) + if err != nil { + return fmt.Errorf("could not read external skill %q: %w", ext.Name, err) + } + fmt.Println() + fmt.Printf(" %s\n\n", gcolor.HEX("#94a3b8").Sprintf("external skill (read-only): %s", ext.Path)) + fmt.Println(mdrender.RenderMarkdown(string(content))) + fmt.Println() + return nil +} + +// clipRunes cuts s to at most n runes for single-line terminal display, +// appending "..." when clipped (matching the house style used by +// /skill search descriptions), but rune-safe rather than byte-sliced. +func clipRunes(s string, n int) string { + runes := []rune(s) + if len(runes) <= n { + return s + } + if n <= 3 { + return string(runes[:n]) + } + return string(runes[:n-3]) + "..." +} diff --git a/internal/commands/cmd_skill_external_test.go b/internal/commands/cmd_skill_external_test.go new file mode 100644 index 0000000..db52d9e --- /dev/null +++ b/internal/commands/cmd_skill_external_test.go @@ -0,0 +1,250 @@ +package commands + +// cmd_skill_external_test.go — tests for the read-only external-skill surface +// of /skill: the supplementary "External (read-only)" section on /skill ls, +// the explicit ext:<name> namespace on /skill get, the external fallback when +// the gateway tag lookup misses, and the preserved miss behavior when both +// sides miss. +// +// Seams reused from existing test files (same package, no edits there): +// captureProtocolStdout (cmd_protocol_test.go) for stdout+gcolor capture and +// protoWriteFile (cmd_protocol_test.go) for fixture files. The gateway is +// faked with httptest, mirroring internal/client's own test style. + +import ( + "context" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + + "github.com/DojoGenesis/cli/internal/client" + "github.com/DojoGenesis/cli/internal/config" + "github.com/DojoGenesis/cli/internal/plugins" +) + +// newExternalSkillRegistry builds a Registry wired to gatewayURL (empty => +// nil client; only safe for code paths that never touch the gateway) with +// cfg.Skills.ExternalDirs set. Mirrors commands_test.go's testRegistry. +func newExternalSkillRegistry(gatewayURL string, externalDirs []string) *Registry { + session := "skill-external-test-session" + cfg := &config.Config{ + Gateway: config.GatewayConfig{URL: gatewayURL, Timeout: "5s"}, + Skills: config.SkillsConfig{ExternalDirs: externalDirs}, + } + var gw *client.Client + if gatewayURL != "" { + gw = client.New(gatewayURL, "", "5s") + } + r := &Registry{ + cfg: cfg, + gw: gw, + cmds: make(map[string]Command), + plgs: []plugins.Plugin{}, + session: &session, + } + r.register() + return r +} + +// oneSkillGateway serves GET /api/skills with a single gateway skill (total=1 +// so client.SkillsAll stops after one page) and 404 for everything else. +func oneSkillGateway(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if strings.HasPrefix(req.URL.Path, "/api/skills") { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"skills":[{"id":"s1","name":"gw-skill","description":"a gateway skill","category":"engineering","inputs":[],"outputs":[]}],"total":1,"limit":100,"offset":0}`)) + return + } + http.NotFound(w, req) + })) + t.Cleanup(srv.Close) + return srv +} + +// missGateway 404s every request — CAS tag resolution always misses. +func missGateway(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + http.NotFound(w, req) + })) + t.Cleanup(srv.Close) + return srv +} + +// writeExternalFixture creates <root>/<child>/SKILL.md and returns (root, path). +func writeExternalFixture(t *testing.T, child, frontmatter string) (string, string) { + t.Helper() + root := t.TempDir() + path := filepath.Join(root, child, "SKILL.md") + protoWriteFile(t, path, frontmatter) + return root, path +} + +// ─── /skill ls — External (read-only) section ──────────────────────────────── + +func TestSkillLsAppendsExternalSection(t *testing.T) { + srv := oneSkillGateway(t) + root, _ := writeExternalFixture(t, "ext-alpha", + "---\nname: ext-alpha\ndescription: External alpha does alpha things.\n---\n# ext-alpha\nbody\n") + r := newExternalSkillRegistry(srv.URL, []string{root}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill ls") + }) + if err != nil { + t.Fatalf("skill ls: %v", err) + } + // Existing gateway listing still renders first: bare `/skill ls` shows + // the category summary (categories, not skill names). + if !strings.Contains(out, "Skills 1 total") || !strings.Contains(out, "engineering") { + t.Errorf("gateway category summary missing from output:\n%s", out) + } + if !strings.Contains(out, "External (read-only)") { + t.Errorf("External (read-only) section header missing:\n%s", out) + } + if !strings.Contains(out, "ext:ext-alpha") { + t.Errorf("ext:<name> line missing:\n%s", out) + } + if !strings.Contains(out, "External alpha does alpha things.") { + t.Errorf("external description missing:\n%s", out) + } + if !strings.Contains(out, "("+root+")") { + t.Errorf("SourceDir suffix missing:\n%s", out) + } + // Section must come after the gateway listing. + if strings.Index(out, "External (read-only)") < strings.Index(out, "engineering") { + t.Errorf("External section rendered before the gateway listing:\n%s", out) + } +} + +func TestSkillLsEmptyScanPrintsNothingExtra(t *testing.T) { + srv := oneSkillGateway(t) + // ExternalDirs points at a dir that exists but holds no SKILL.md. + r := newExternalSkillRegistry(srv.URL, []string{t.TempDir()}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill ls") + }) + if err != nil { + t.Fatalf("skill ls: %v", err) + } + if strings.Contains(out, "External (read-only)") { + t.Errorf("empty scan must print no External section:\n%s", out) + } + if strings.Contains(out, "ext:") { + t.Errorf("empty scan must print no ext: lines:\n%s", out) + } +} + +func TestSkillLsGatewayErrorUnchanged(t *testing.T) { + // Gateway unreachable: /skill ls must surface the existing error + // unchanged, with no External section around it. + root, _ := writeExternalFixture(t, "ext-alpha", "---\nname: ext-alpha\n---\n") + srv := missGateway(t) // /api/skills 404s → Skills() errors + r := newExternalSkillRegistry(srv.URL, []string{root}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill ls") + }) + if err == nil { + t.Fatal("expected the existing gateway error, got nil") + } + if !strings.Contains(err.Error(), "could not fetch skills") { + t.Errorf("gateway error text changed: %v", err) + } + if strings.Contains(out, "External (read-only)") { + t.Errorf("External section must not render on the gateway error path:\n%s", out) + } +} + +// ─── /skill get ext:<name> ─────────────────────────────────────────────────── + +func TestSkillGetExtPrefixRendersWithReadOnlyHeader(t *testing.T) { + root, path := writeExternalFixture(t, "ext-alpha", + "---\nname: ext-alpha\ndescription: External alpha does alpha things.\n---\n# Alpha Heading\nalpha body text\n") + // Gateway never touched on the ext: path — nil client proves it. + r := newExternalSkillRegistry("", []string{root}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill get ext:ext-alpha") + }) + if err != nil { + t.Fatalf("skill get ext:ext-alpha: %v", err) + } + if !strings.Contains(out, "external skill (read-only): "+path) { + t.Errorf("read-only header with path missing:\n%s", out) + } + if !strings.Contains(out, "alpha body text") { + t.Errorf("rendered SKILL.md body missing:\n%s", out) + } +} + +func TestSkillGetExtPrefixCaseInsensitive(t *testing.T) { + root, _ := writeExternalFixture(t, "ext-alpha", "---\nname: ext-alpha\n---\nbody\n") + r := newExternalSkillRegistry("", []string{root}) + + _, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill get ext:EXT-ALPHA") + }) + if err != nil { + t.Fatalf("case-insensitive ext: lookup failed: %v", err) + } +} + +func TestSkillGetExtPrefixMiss(t *testing.T) { + r := newExternalSkillRegistry("", []string{t.TempDir()}) + + _, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill get ext:nope") + }) + if err == nil { + t.Fatal("expected error for unknown external skill, got nil") + } + if !strings.Contains(err.Error(), `external skill "nope" not found`) { + t.Errorf("unexpected ext: miss error: %v", err) + } +} + +// ─── /skill get <name> — gateway first, external fallback ──────────────────── + +func TestSkillGetPlainNameFallsBackToExternalOnGatewayMiss(t *testing.T) { + srv := missGateway(t) // CASResolveTag misses + root, path := writeExternalFixture(t, "ext-alpha", + "---\nname: ext-alpha\n---\nfallback body\n") + r := newExternalSkillRegistry(srv.URL, []string{root}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill get ext-alpha") + }) + if err != nil { + t.Fatalf("expected external fallback to succeed, got: %v", err) + } + if !strings.Contains(out, "external skill (read-only): "+path) { + t.Errorf("read-only header missing on fallback render:\n%s", out) + } + if !strings.Contains(out, "fallback body") { + t.Errorf("fallback body missing:\n%s", out) + } +} + +func TestSkillGetMissesBothPrintsExistingMiss(t *testing.T) { + srv := missGateway(t) + root, _ := writeExternalFixture(t, "ext-alpha", "---\nname: ext-alpha\n---\nbody\n") + r := newExternalSkillRegistry(srv.URL, []string{root}) + + out, err := captureProtocolStdout(t, func() error { + return r.Dispatch(context.Background(), "skill get definitely-not-anywhere") + }) + if err == nil { + t.Fatal("expected the existing miss error, got nil") + } + if !strings.Contains(err.Error(), `could not resolve tag "definitely-not-anywhere"`) { + t.Errorf("existing miss behavior changed: %v", err) + } + if strings.Contains(out, "external skill (read-only)") { + t.Errorf("no external render expected on a double miss:\n%s", out) + } +} diff --git a/internal/commands/cmd_workflow.go b/internal/commands/cmd_workflow.go index 7f7b857..79b51b4 100644 --- a/internal/commands/cmd_workflow.go +++ b/internal/commands/cmd_workflow.go @@ -1,15 +1,13 @@ package commands -// cmd_workflow.go — /workflow, /run, /doc, /pilot, /practice, /skill, /tools commands. +// cmd_workflow.go — /workflow, /run, /doc, /pilot, /practice, /tools commands. +// (/skill moved to cmd_skill.go.) import ( "context" "encoding/json" "fmt" "os" - "path/filepath" - "sort" - "strconv" "strings" "time" @@ -19,7 +17,6 @@ import ( "github.com/DojoGenesis/cli/internal/client" "github.com/DojoGenesis/cli/internal/mdrender" "github.com/DojoGenesis/cli/internal/orchestration" - "github.com/DojoGenesis/cli/internal/skills" "github.com/DojoGenesis/cli/internal/tui" gcolor "github.com/gookit/color" ) @@ -475,497 +472,6 @@ func (r *Registry) practiceCmd() Command { } } -// ─── /skill ───────────────────────────────────────────────────────────────── - -func (r *Registry) skillCmd() Command { - return Command{ - Name: "skill", - Aliases: []string{"skills"}, - Usage: "/skill [ls [filter]|search <query>|get <name>|inspect <hash>|tags|package-all <dir>]", - Short: "List, fetch, or inspect skills from CAS", - Run: func(ctx context.Context, args []string) error { - sub := "ls" - if len(args) > 0 { - sub = strings.ToLower(args[0]) - } - - switch sub { - case "search": - // /skill search <query> - if len(args) < 2 { - return fmt.Errorf("usage: /skill search <query>") - } - query := strings.Join(args[1:], " ") - skills, err := r.gw.SearchSkills(ctx, query) - if err != nil { - return fmt.Errorf("search failed: %w", err) - } - - fmt.Println() - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Skills matching %q (%d)\n\n", query, len(skills))) - - if len(skills) == 0 { - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No matching skills found.")) - fmt.Println() - return nil - } - - for _, s := range skills { - name := gcolor.HEX("#f4a261").Sprintf("%-40s", s.Name) - cat := "" - if s.Category != "" { - cat = gcolor.HEX("#94a3b8").Sprintf("[%s]", s.Category) - } - plugin := "" - if s.Plugin != "" { - plugin = gcolor.HEX("#64748b").Sprintf(" (%s)", s.Plugin) - } - fmt.Printf(" %s %s%s\n", name, cat, plugin) - if s.Description != "" { - desc := s.Description - if len(desc) > 100 { - desc = desc[:97] + "..." - } - fmt.Printf(" %s\n", gcolor.HEX("#94a3b8").Sprint(" "+desc)) - } - } - fmt.Println() - return nil - - case "get": - // /skill get <name> - if len(args) < 2 { - return fmt.Errorf("usage: /skill get <name>") - } - name := args[1] - tag, err := r.gw.CASResolveTag(ctx, name, "latest") - if err != nil { - return fmt.Errorf("could not resolve tag %q: %w", name, err) - } - content, err := r.gw.CASGetContent(ctx, tag.Ref) - if err != nil { - return fmt.Errorf("could not fetch content for ref %q: %w", tag.Ref, err) - } - fmt.Println() - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Skill: %s @ %s\n\n", tag.Name, tag.Version)) - printKV("ref", tag.Ref) - fmt.Println() - fmt.Println(mdrender.RenderMarkdown(string(content))) - fmt.Println() - return nil - - case "inspect": - // /skill inspect <hash> - if len(args) < 2 { - return fmt.Errorf("usage: /skill inspect <hash>") - } - ref := args[1] - content, err := r.gw.CASGetContent(ctx, ref) - if err != nil { - return fmt.Errorf("could not fetch content for ref %q: %w", ref, err) - } - fmt.Println() - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" CAS ref: %s\n\n", ref)) - fmt.Println(gcolor.White.Sprint(string(content))) - fmt.Println() - return nil - - case "tags": - // /skill tags - tags, err := r.gw.CASListTags(ctx) - if err != nil { - return fmt.Errorf("could not list CAS tags: %w", err) - } - fmt.Println() - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" CAS Tags (%d)\n\n", len(tags))) - if len(tags) == 0 { - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No tags found.")) - fmt.Println() - return nil - } - // Table header - fmt.Printf(" %s %s %s\n", - gcolor.HEX("#94a3b8").Sprintf("%-32s", "Name"), - gcolor.HEX("#94a3b8").Sprintf("%-12s", "Version"), - gcolor.HEX("#94a3b8").Sprint("Ref"), - ) - fmt.Printf(" %s\n", gcolor.HEX("#64748b").Sprint(strings.Repeat("\u2500", 72))) - for _, t := range tags { - fmt.Printf(" %s %s %s\n", - gcolor.HEX("#f4a261").Sprintf("%-32s", truncate(t.Name, 32)), - gcolor.White.Sprintf("%-12s", truncate(t.Version, 12)), - gcolor.HEX("#94a3b8").Sprint(truncate(t.Ref, 20)), - ) - } - fmt.Println() - return nil - - case "package-all": - // /skill package-all [dir] - // Walk a directory for SKILL.md files, put each into CAS, and create tags. - // Default source: $DOJO_SKILLS_PATH or current directory. - dir := os.Getenv("DOJO_SKILLS_PATH") - if len(args) >= 2 { - dir = args[1] - } - if dir == "" { - return fmt.Errorf("usage: /skill package-all <dir>\n or set DOJO_SKILLS_PATH") - } - - // Walk for SKILL.md files. - var skills []string - err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return nil // skip inaccessible - } - if !info.IsDir() && info.Name() == "SKILL.md" { - skills = append(skills, path) - } - return nil - }) - if err != nil { - return fmt.Errorf("walking %s: %w", dir, err) - } - - if len(skills) == 0 { - fmt.Println() - fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" No SKILL.md files found in %s", dir)) - fmt.Println() - return nil - } - - fmt.Println() - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" Packaging %d skills from %s\n\n", len(skills), dir)) - - var succeeded, failed int - for _, path := range skills { - // Derive skill name from parent directory. - skillName := filepath.Base(filepath.Dir(path)) - - content, err := os.ReadFile(path) - if err != nil { - gcolor.HEX("#ef4444").Printf(" [FAIL] %s: %s\n", skillName, err) - failed++ - continue - } - - // Put content into CAS. - // Sleep before each API call: gateway allows 300 req/min (5/sec) sustained, - // burst 50. Each skill makes 2 calls, so 250ms per call = 4 req/sec total. - ref, err := r.gw.CASPutContent(ctx, content) - time.Sleep(250 * time.Millisecond) - if err != nil { - gcolor.HEX("#ef4444").Printf(" [FAIL] %s: CAS put: %s\n", skillName, err) - failed++ - continue - } - - // Create tag: name@latest -> ref. - if err := r.gw.CASCreateTag(ctx, skillName, "latest", ref); err != nil { - gcolor.HEX("#ef4444").Printf(" [FAIL] %s: tag create: %s\n", skillName, err) - failed++ - time.Sleep(250 * time.Millisecond) - continue - } - time.Sleep(250 * time.Millisecond) - - gcolor.HEX("#22c55e").Printf(" [OK] %s → %s\n", skillName, shortRef(ref)) - succeeded++ - } - - fmt.Println() - summary := fmt.Sprintf(" Done: %d succeeded, %d failed", succeeded, failed) - if failed > 0 { - gcolor.HEX("#eab308").Println(summary) - } else { - gcolor.HEX("#22c55e").Println(summary) - } - fmt.Println() - return nil - - default: // ls / all / filter — main skill browser - filter, showAll, page := parseSkillLsArgs(args) - - rawSkills, err := r.gw.Skills(ctx) - if err != nil { - return fmt.Errorf("could not fetch skills: %w", err) - } - // Fill in missing categories via semantic clustering so skills - // group correctly regardless of gateway metadata completeness. - skillList := skills.EnrichCategories(rawSkills) - - if len(skillList) == 0 { - fmt.Println() - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No skills found.")) - fmt.Println() - return nil - } - - // Category summary: no filter, no explicit "all", page 1. - if filter == "" && !showAll && page == 1 { - return printSkillCategorySummary(skillList) - } - - // Filter by name, category, or plugin. - displaySkills := skillList - if filter != "" { - fl := strings.ToLower(filter) - var matched []client.Skill - for _, s := range skillList { - if strings.Contains(strings.ToLower(s.Name), fl) || - strings.Contains(strings.ToLower(s.Category), fl) || - strings.Contains(strings.ToLower(s.Plugin), fl) { - matched = append(matched, s) - } - } - displaySkills = matched - } - - return printSkillsPage(displaySkills, filter, page) - } - }, - } -} - -// ─── /skill helpers ────────────────────────────────────────────────────────── - -const skillPageSize = 30 - -// shortRef returns the first 12 characters of a CAS ref for compact display, -// or ref unchanged if it is shorter than that. ref comes straight from the -// gateway response (CASPutContent) with no length guarantee — an unguarded -// ref[:12] slice panics (and previously crashed the whole CLI mid -// /skill package-all) whenever the gateway returns a short ref. -func shortRef(ref string) string { - if len(ref) >= 12 { - return ref[:12] - } - return ref -} - -// parseSkillLsArgs extracts (filter, showAll, page) from the args slice. -// Recognises: "all", "p<N>" or plain integers as page numbers, everything else -// is joined as the filter term. -func parseSkillLsArgs(args []string) (filter string, showAll bool, page int) { - page = 1 - var filterParts []string - for _, a := range args { - al := strings.ToLower(a) - if al == "ls" { - continue - } - if al == "all" { - showAll = true - continue - } - // p<N> — page number - if strings.HasPrefix(al, "p") { - if n, err := strconv.Atoi(al[1:]); err == nil && n >= 1 { - page = n - continue - } - } - // bare integer — page number - if n, err := strconv.Atoi(a); err == nil && n >= 1 { - page = n - continue - } - filterParts = append(filterParts, a) - } - filter = strings.Join(filterParts, " ") - return -} - -// skillCategoryOrder groups skills by category. Returns (map, ordered keys sorted by count desc). -func skillCategoryOrder(skills []client.Skill) (map[string][]client.Skill, []string) { - cats := map[string][]client.Skill{} - for _, s := range skills { - cat := s.Category - if cat == "" { - cat = "general" - } - cats[cat] = append(cats[cat], s) - } - keys := make([]string, 0, len(cats)) - for k := range cats { - keys = append(keys, k) - } - sort.Slice(keys, func(i, j int) bool { - ci, cj := len(cats[keys[i]]), len(cats[keys[j]]) - if ci != cj { - return ci > cj - } - return keys[i] < keys[j] - }) - return cats, keys -} - -// printSkillCategorySummary renders the landing page for /skill ls (no args). -func printSkillCategorySummary(skills []client.Skill) error { - cats, order := skillCategoryOrder(skills) - - // Count distinct plugins. - pluginSet := map[string]struct{}{} - for _, s := range skills { - if s.Plugin != "" { - pluginSet[s.Plugin] = struct{}{} - } - } - - fmt.Println() - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf( - " Skills %d total · %d categories · %d plugins\n\n", - len(skills), len(order), len(pluginSet), - )) - - divider := gcolor.HEX("#334155").Sprint(strings.Repeat("─", 66)) - fmt.Println(" " + divider) - - for _, cat := range order { - count := len(cats[cat]) - bar := buildMiniBar(count, len(skills), 12) - fmt.Printf(" %s %s %s\n", - gcolor.HEX("#f4a261").Sprintf("%-28s", cat), - gcolor.HEX("#94a3b8").Sprintf("%3d", count)+" "+gcolor.HEX("#334155").Sprint(bar), - gcolor.HEX("#64748b").Sprintf("/skill ls %s", cat), - ) - } - - fmt.Println(" " + divider) - fmt.Println() - fmt.Printf(" %s /skill ls all %s /skill ls all p2\n", - gcolor.HEX("#94a3b8").Sprint("list all:"), - gcolor.HEX("#94a3b8").Sprint("next page:"), - ) - fmt.Printf(" %s /skill search <query>\n", - gcolor.HEX("#94a3b8").Sprint("search: "), - ) - fmt.Println() - return nil -} - -// buildMiniBar returns a fixed-width ASCII progress bar. -func buildMiniBar(count, total, width int) string { - if total == 0 || width == 0 { - return strings.Repeat("░", width) - } - filled := count * width / total - if filled == 0 && count > 0 { - filled = 1 - } - return strings.Repeat("█", filled) + strings.Repeat("░", width-filled) -} - -// printSkillsPage renders a paginated grouped skill list. -// Pages are built by complete category: a new page starts only after a full -// category has been rendered, so categories are never split mid-display. -func printSkillsPage(skills []client.Skill, filter string, page int) error { - if len(skills) == 0 { - fmt.Println() - if filter != "" { - fmt.Println(gcolor.HEX("#94a3b8").Sprintf(" No skills matching %q.", filter)) - } else { - fmt.Println(gcolor.HEX("#94a3b8").Sprint(" No skills found.")) - } - fmt.Println() - return nil - } - - cats, order := skillCategoryOrder(skills) - - // Build pages by grouping complete categories until skillPageSize is reached. - type pageSlice struct { - cats []string - count int - } - var pages []pageSlice - var cur pageSlice - for _, cat := range order { - cur.cats = append(cur.cats, cat) - cur.count += len(cats[cat]) - if cur.count >= skillPageSize { - pages = append(pages, cur) - cur = pageSlice{} - } - } - if len(cur.cats) > 0 { - pages = append(pages, cur) - } - - totalPages := len(pages) - if page < 1 { - page = 1 - } - if page > totalPages { - page = totalPages - } - pg := pages[page-1] - - // Count skills on this page. - pageCount := 0 - for _, cat := range pg.cats { - pageCount += len(cats[cat]) - } - - fmt.Println() - label := "Skills" - if filter != "" { - label = fmt.Sprintf("Skills › %s", filter) - } - pageLabel := "" - if totalPages > 1 { - pageLabel = fmt.Sprintf(" · page %d of %d", page, totalPages) - } - gcolor.Bold.Print(gcolor.HEX("#e8b04a").Sprintf(" %s (%d of %d%s)\n\n", label, pageCount, len(skills), pageLabel)) - - // Render each category group. - for _, cat := range pg.cats { - fmt.Printf(" %s %s %s\n", - gcolor.HEX("#334155").Sprint("────"), - gcolor.HEX("#e8b04a").Sprint("["+cat+"]"), - gcolor.HEX("#334155").Sprint("────────────────────────────────────────────────"), - ) - for _, s := range cats[cat] { - plugin := "" - if s.Plugin != "" { - plugin = gcolor.HEX("#64748b").Sprintf("(%s)", s.Plugin) - } - fmt.Printf(" %s %s\n", - gcolor.HEX("#f4a261").Sprintf("%-40s", truncate(s.Name, 40)), - plugin, - ) - } - } - - // Footer navigation hints. - fmt.Println() - if totalPages > 1 { - base := "/skill ls" - if filter != "" { - base += " " + filter - } else { - base += " all" - } - if page < totalPages { - fmt.Printf(" %s %s p%d\n", - gcolor.HEX("#94a3b8").Sprint("next:"), - gcolor.HEX("#64748b").Sprint(base), - page+1, - ) - } - if page > 1 { - fmt.Printf(" %s %s p%d\n", - gcolor.HEX("#94a3b8").Sprint("prev:"), - gcolor.HEX("#64748b").Sprint(base), - page-1, - ) - } - } - fmt.Printf(" %s /skill search <query>\n", gcolor.HEX("#94a3b8").Sprint("search:")) - fmt.Println() - return nil -} - // ─── /tools ───────────────────────────────────────────────────────────────── func (r *Registry) toolsCmd() Command { diff --git a/internal/commands/commands.go b/internal/commands/commands.go index 3820217..8324732 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -10,8 +10,10 @@ import ( "github.com/DojoGenesis/cli/internal/activity" "github.com/DojoGenesis/cli/internal/client" "github.com/DojoGenesis/cli/internal/config" + "github.com/DojoGenesis/cli/internal/guardrail" "github.com/DojoGenesis/cli/internal/hooks" "github.com/DojoGenesis/cli/internal/plugins" + gcolor "github.com/gookit/color" ) // Registry maps slash command names to handler functions. @@ -22,6 +24,18 @@ type Registry struct { plgs []plugins.Plugin runner *hooks.Runner session *string // pointer to REPL's active session ID + + // guard counts consecutive per-command failures and escalates advisory + // notices (never blocks — see internal/guardrail). Built lazily on first + // Dispatch from cfg.Guardrails rather than in New because tests construct + // Registry literals directly; nil cfg means disabled. Held for the + // Registry lifetime so counts persist across the whole REPL session. + guard *guardrail.Tracker + + // advicePrint renders one guardrail advice line. nil means the default + // stdout printer (same surface the REPL prints command errors to); + // overridable so tests can capture advice without hijacking os.Stdout. + advicePrint func(msg string) } // Command is a callable slash command. @@ -105,6 +119,7 @@ func (r *Registry) Dispatch(ctx context.Context, input string) error { logArgs := redactSecretArgs(name, args) activity.Log(activity.CommandRun, fmt.Sprintf("/%s %s", name, strings.Join(logArgs, " "))) } + r.guardAdvise(cmd.Name, args, err) return err } // Alias scan @@ -116,6 +131,9 @@ func (r *Registry) Dispatch(ctx context.Context, input string) error { logArgs := redactSecretArgs(cmd.Name, args) activity.Log(activity.CommandRun, fmt.Sprintf("/%s %s", name, strings.Join(logArgs, " "))) } + // Canonical name, not the typed alias, so every spelling of + // the same command feeds one failure streak. + r.guardAdvise(cmd.Name, args, err) return err } } @@ -123,6 +141,48 @@ func (r *Registry) Dispatch(ctx context.Context, input string) error { return fmt.Errorf("unknown command /%s — type /help for a list", name) } +// guardKey builds the guardrail key for one dispatched command: "cmd:" plus +// the canonical command name, plus the first subcommand token (lowercased, +// matching how subcommand matching works elsewhere in this package) when one +// is present — e.g. "cmd:code test". Keying on the subcommand keeps +// "/code test" and "/code build" as independent failure streaks. +func guardKey(name string, args []string) string { + key := "cmd:" + name + if len(args) > 0 { + key += " " + strings.ToLower(args[0]) + } + return key +} + +// guardAdvise records one dispatched command's outcome with the guardrail +// tracker and prints any resulting advice on its own line. Advisory only: +// the command's error is returned to the caller untouched by Dispatch, and +// the advice line goes to stdout — the same surface the REPL prints command +// errors to (repl.go's `gcolor.Red.Printf(" error: ...")`) — styled with +// the warning amber used by the renderer's EventWarning. +func (r *Registry) guardAdvise(name string, args []string, runErr error) { + if r.guard == nil { + // Lazy build (see Registry.guard doc). Dispatch runs on the single + // REPL read-loop goroutine, so an unguarded nil check is safe here; + // the Tracker itself is mutex-guarded regardless. + if r.cfg == nil { + r.guard = guardrail.New(0, 0, false) // nil cfg → disabled + } else { + g := r.cfg.Guardrails + r.guard = guardrail.New(g.WarnAfter, g.HardAfter, g.Enabled) + } + } + adv := r.guard.Record(guardKey(name, args), runErr != nil) + if adv.Level == guardrail.None { + return + } + if r.advicePrint != nil { + r.advicePrint(adv.Msg) + return + } + fmt.Printf(" %s\n", gcolor.HEX("#f4a261").Sprint(adv.Msg)) +} + func (r *Registry) add(cmd Command) { r.cmds[cmd.Name] = cmd } diff --git a/internal/commands/guardrail_wrap_test.go b/internal/commands/guardrail_wrap_test.go new file mode 100644 index 0000000..6c9fb92 --- /dev/null +++ b/internal/commands/guardrail_wrap_test.go @@ -0,0 +1,188 @@ +package commands + +// guardrail_wrap_test.go — tests for the advisory guardrail wrap around +// Registry.Dispatch (see guardAdvise in commands.go). NEW file per the +// advancement-wave ownership rules; existing test files are not modified. +// TestMain in commands_test.go (same package) already isolates $HOME, so the +// activity log writes from successful dispatches stay out of the real ~/.dojo. + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/DojoGenesis/cli/internal/config" +) + +// guardTestSetup builds a bare Registry (no gateway, no real commands) with a +// single fake "boom" command whose failure is switchable per dispatch, plus a +// capture slice wired into the advicePrint seam. +func guardTestSetup(cfg *config.Config) (r *Registry, printed *[]string, fail *bool) { + var lines []string + f := true + r = &Registry{ + cfg: cfg, + cmds: make(map[string]Command), + advicePrint: func(msg string) { + lines = append(lines, msg) + }, + } + r.cmds["boom"] = Command{ + Name: "boom", + Aliases: []string{"b"}, + Short: "test command that fails on demand", + Run: func(ctx context.Context, args []string) error { + if f { + return errors.New("kaboom") + } + return nil + }, + } + return r, &lines, &f +} + +func warnMsg(key string, n int) string { + return fmt.Sprintf("guardrail: %s has failed %d times consecutively with the same signature — consider a different approach", key, n) +} + +func hardMsg(key string, n int) string { + return fmt.Sprintf("guardrail: %s is stuck (%d consecutive identical failures) — stop and change strategy, or check config vs code (see debugging gate)", key, n) +} + +// TestDispatchGuardrail_WarnHardAndReset drives the fake failing command +// through Dispatch and asserts warn fires exactly at WarnAfter, hard fires at +// and after HardAfter, and a success resets the streak. +func TestDispatchGuardrail_WarnHardAndReset(t *testing.T) { + cfg := &config.Config{ + Guardrails: config.GuardrailsConfig{Enabled: true, WarnAfter: 2, HardAfter: 4}, + } + key := "cmd:boom go" + + steps := []struct { + name string + fail bool + wantAdvice string // "" = no advice line expected from this dispatch + }{ + {"failure 1: below warn", true, ""}, + {"failure 2: warn fires at WarnAfter", true, warnMsg(key, 2)}, + {"failure 3: between warn and hard", true, ""}, + {"failure 4: hard fires at HardAfter", true, hardMsg(key, 4)}, + {"failure 5: hard keeps firing past HardAfter", true, hardMsg(key, 5)}, + {"success: resets, no advice", false, ""}, + {"failure 1 after reset: below warn", true, ""}, + {"failure 2 after reset: warn fires again", true, warnMsg(key, 2)}, + } + + r, printed, fail := guardTestSetup(cfg) + for _, step := range steps { + *fail = step.fail + before := len(*printed) + err := r.Dispatch(context.Background(), "boom go") + if step.fail && err == nil { + t.Fatalf("%s: Dispatch returned nil error; the command error must pass through unchanged", step.name) + } + if !step.fail && err != nil { + t.Fatalf("%s: Dispatch returned %v; want nil", step.name, err) + } + got := (*printed)[before:] + if step.wantAdvice == "" { + if len(got) != 0 { + t.Fatalf("%s: unexpected advice %q", step.name, got) + } + continue + } + if len(got) != 1 || got[0] != step.wantAdvice { + t.Fatalf("%s: advice = %q; want exactly [%q]", step.name, got, step.wantAdvice) + } + } +} + +// TestDispatchGuardrail_SubcommandKeysIndependent verifies that the first +// subcommand token is part of the key, so different subcommands hold +// independent streaks, and that a bare command keys without a trailing space. +func TestDispatchGuardrail_SubcommandKeysIndependent(t *testing.T) { + cfg := &config.Config{ + Guardrails: config.GuardrailsConfig{Enabled: true, WarnAfter: 2, HardAfter: 4}, + } + r, printed, _ := guardTestSetup(cfg) // fail stays true + + // Alternate subcommands: each key sees one failure at a time, so nothing + // may fire until a single key reaches 2. + for _, in := range []string{"boom alpha", "boom beta", "boom alpha"} { + if err := r.Dispatch(context.Background(), in); err == nil { + t.Fatalf("Dispatch(%q) = nil error; want failure", in) + } + } + if len(*printed) != 1 || (*printed)[0] != warnMsg("cmd:boom alpha", 2) { + t.Fatalf("advice = %q; want exactly [%q]", *printed, warnMsg("cmd:boom alpha", 2)) + } + + // Bare command (no args) keys as just "cmd:boom". + r2, printed2, _ := guardTestSetup(cfg) + r2.Dispatch(context.Background(), "boom") + r2.Dispatch(context.Background(), "boom") + if len(*printed2) != 1 || (*printed2)[0] != warnMsg("cmd:boom", 2) { + t.Fatalf("bare-command advice = %q; want exactly [%q]", *printed2, warnMsg("cmd:boom", 2)) + } +} + +// TestDispatchGuardrail_AliasSharesCanonicalKey verifies dispatching via an +// alias feeds the same streak as the canonical name (key uses cmd.Name). +func TestDispatchGuardrail_AliasSharesCanonicalKey(t *testing.T) { + cfg := &config.Config{ + Guardrails: config.GuardrailsConfig{Enabled: true, WarnAfter: 2, HardAfter: 4}, + } + r, printed, _ := guardTestSetup(cfg) + + r.Dispatch(context.Background(), "boom go") // canonical: streak 1 + r.Dispatch(context.Background(), "b go") // alias: same key, streak 2 → warn + if len(*printed) != 1 || (*printed)[0] != warnMsg("cmd:boom go", 2) { + t.Fatalf("advice = %q; want exactly [%q] (alias must share the canonical key)", *printed, warnMsg("cmd:boom go", 2)) + } +} + +// TestDispatchGuardrail_DisabledAndNilCfg verifies the disabled short-circuit +// and that a nil cfg is treated as disabled (no panic, no advice). +func TestDispatchGuardrail_DisabledAndNilCfg(t *testing.T) { + cases := []struct { + name string + cfg *config.Config + }{ + {"explicitly disabled", &config.Config{ + Guardrails: config.GuardrailsConfig{Enabled: false, WarnAfter: 1, HardAfter: 2}, + }}, + {"nil cfg", nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r, printed, _ := guardTestSetup(tc.cfg) + for i := 0; i < 6; i++ { + if err := r.Dispatch(context.Background(), "boom go"); err == nil { + t.Fatal("Dispatch = nil error; want failure") + } + } + if len(*printed) != 0 { + t.Fatalf("advice = %q; want none", *printed) + } + }) + } +} + +// TestDispatchGuardrail_UnknownCommandNotTracked verifies that an unknown +// command (which never reaches a handler) records nothing — the guardrail +// wraps handler outcomes, not typos. +func TestDispatchGuardrail_UnknownCommandNotTracked(t *testing.T) { + cfg := &config.Config{ + Guardrails: config.GuardrailsConfig{Enabled: true, WarnAfter: 1, HardAfter: 3}, + } + r, printed, _ := guardTestSetup(cfg) + for i := 0; i < 3; i++ { + if err := r.Dispatch(context.Background(), "nosuchcmd"); err == nil { + t.Fatal("Dispatch(unknown) = nil error; want error") + } + } + if len(*printed) != 0 { + t.Fatalf("advice = %q; want none for unknown commands", *printed) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 16b09e2..6aaa308 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,6 +24,10 @@ type Config struct { Protocol ProtocolConfig `json:"protocol"` Verify VerifyConfig `json:"verify"` Auth AuthConfig `json:"auth,omitempty"` + Permissions PermissionsConfig `json:"permissions"` + Delegation DelegationConfig `json:"delegation"` + Guardrails GuardrailsConfig `json:"guardrails"` + Skills SkillsConfig `json:"skills"` DispositionProfiles map[string]DispositionPreset `json:"disposition_profiles,omitempty"` // envOverrides records which fields Load() populated from an environment @@ -79,6 +83,59 @@ type DefaultsConfig struct { Model string `json:"model"` } +// PermissionsConfig controls the CLI's action-permission gate: which command +// patterns require an interactive prompt before running. Mode selects the +// gating strategy — "default" prompts per the harness's normal per-command +// rules, "allowlist" skips the prompt only for patterns listed in Allowed, +// "yolo" skips every prompt. Mode defaults to "default" (pre-seeded in +// defaults()); Allowed holds glob-style command patterns, e.g. "code.undo", +// "plugin.install", "craft.*". See --yolo in cmd/dojo/main.go, which sets +// Mode to "yolo" in memory for the run only and never persists it via +// Save() — the same transient-override discipline as the env-overridden +// fields below (see envOverride and Save()), just triggered by a flag +// instead of an environment variable. +type PermissionsConfig struct { + Mode string `json:"mode"` + Allowed []string `json:"allowed"` +} + +// DelegationConfig controls the default model used for agent dispatch (the +// cheap lane for sub-agent work) — distinct from Defaults.Model, which is +// the interactive chat default. Model defaults to "" (unset): an empty +// value means dispatch call sites fall back to their own default rather +// than a config-pinned one. +type DelegationConfig struct { + Model string `json:"model"` +} + +// GuardrailsConfig controls the repeated-failure nudge (warn, then hard +// stop) surfaced during long-running command loops. Enabled is a plain bool +// — not *bool — so it can reuse the exact "pre-seed true in defaults(), let +// json.Unmarshal only overwrite keys actually present" merge trick already +// documented on ProtocolConfig, rather than introduce a second optional-bool +// convention in this file (a *bool field literally named Enabled would also +// collide with an Enabled() accessor method — Go forbids a type having both +// a field and a method of the same name). A settings.json that omits the +// whole block, or omits just "enabled", keeps guardrails on; only an +// explicit "enabled": false turns them off. WarnAfter/HardAfter default to +// 3/5, also pre-seeded in defaults(). +type GuardrailsConfig struct { + Enabled bool `json:"enabled"` + WarnAfter int `json:"warn_after"` + HardAfter int `json:"hard_after"` +} + +// SkillsConfig controls skill discovery beyond the gateway-hosted CAS skill +// set. ExternalDirs lists additional local directories to scan for +// SKILL.md-bearing skill folders, defaulting to [".claude/skills"] +// (pre-seeded in defaults()). This is a different knob from DOJO_SKILLS_PATH +// (see internal/commands/cmd_workflow.go's `/skill package-all`), which +// names a single directory to push to the gateway CAS, not a discovery +// search path — no config section existed for skills before this one. +type SkillsConfig struct { + ExternalDirs []string `json:"external_dirs"` +} + // envOverride records one Config field that Load() populated from an // environment variable: get/set read and write the field, preEnv is the // value the field held immediately before the override was applied (from @@ -197,6 +254,23 @@ func Load() (*Config, error) { func(c *Config, v bool) { c.Verify.AfterAgent = v }, true) } + // DOJO_PERMISSIONS_MODE / DOJO_DELEGATION_MODEL: same transient, + // run-scoped override contract as the knobs above — tracked via + // noteEnvOverride so an unrelated Save() (e.g. /model set) never bakes + // either one into settings.json. Guardrails and Skills.ExternalDirs + // deliberately have no env override (not requested). + if v := os.Getenv("DOJO_PERMISSIONS_MODE"); v != "" { + noteEnvOverride(cfg, + func(c *Config) string { return c.Permissions.Mode }, + func(c *Config, v string) { c.Permissions.Mode = v }, + v) + } + if v := os.Getenv("DOJO_DELEGATION_MODEL"); v != "" { + noteEnvOverride(cfg, + func(c *Config) string { return c.Delegation.Model }, + func(c *Config, v string) { c.Delegation.Model = v }, + v) + } // Gateway settings are load-bearing for connectivity — a malformed URL // or timeout is a genuine configuration error, so it still stops @@ -283,6 +357,9 @@ func (c *Config) Validate() error { if err := c.validateVerify(); err != nil { return err } + if err := c.validatePermissions(); err != nil { + return err + } return nil } @@ -341,6 +418,28 @@ func (c *Config) validateDisposition() error { return nil } +// validPermissionsModes lists the allowed values for Permissions.Mode. "" +// is valid (mirrors validDispositions) and means "unset" — Load()/defaults() +// normalize it to "default" before any real use. +var validPermissionsModes = map[string]bool{ + "": true, + "default": true, + "allowlist": true, + "yolo": true, +} + +// validatePermissions checks that Permissions.Mode is one of the known +// gating strategies (or empty). +func (c *Config) validatePermissions() error { + if !validPermissionsModes[c.Permissions.Mode] { + return fmt.Errorf( + "invalid permissions.mode %q: must be one of default, allowlist, yolo", + c.Permissions.Mode, + ) + } + return nil +} + // EffectiveString returns a human-readable dump of the active config // showing each setting as a key=value line. func (c *Config) EffectiveString() string { @@ -465,5 +564,30 @@ func defaults() *Config { Verify: VerifyConfig{ AfterAgent: false, }, + // Permissions.Mode defaults to "default" — normal per-command prompt + // rules apply until a settings.json or DOJO_PERMISSIONS_MODE (or + // --yolo, in-memory only) says otherwise. + Permissions: PermissionsConfig{ + Mode: "default", + }, + // Delegation.Model defaults to "" (unset), stated explicitly for + // symmetry with DefaultsConfig above. + Delegation: DelegationConfig{ + Model: "", + }, + // Guardrails on by default, same pre-seed-true trick as + // Protocol.Enabled above (see GuardrailsConfig doc). WarnAfter/ + // HardAfter need pre-seeding too since their zero value (0) is not + // the intended default. + Guardrails: GuardrailsConfig{ + Enabled: true, + WarnAfter: 3, + HardAfter: 5, + }, + // Skills.ExternalDirs defaults to the conventional Claude Code + // project skills directory. + Skills: SkillsConfig{ + ExternalDirs: []string{".claude/skills"}, + }, } } diff --git a/internal/config/config_advance_test.go b/internal/config/config_advance_test.go new file mode 100644 index 0000000..a9d3e78 --- /dev/null +++ b/internal/config/config_advance_test.go @@ -0,0 +1,373 @@ +package config + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// This file covers the advancement-wave config sections (Permissions, +// Delegation, Guardrails, Skills) added alongside the contracts for the +// 2026-07-17 dojo-cli-advance feature wave. Every test isolates HOME to a +// fresh t.TempDir() (see commit 3a7cd64 and clearProtocolEnv/clearAllConfigEnv +// above in config_test.go) so `go test ./...` never reads or writes the +// developer's real ~/.dojo/settings.json. + +// clearAdvanceEnv points HOME at a fresh temp dir (so there is no +// settings.json) and wipes every env var this file's Load() calls could pick +// up — the existing DOJO_* knobs via clearAllConfigEnv (config_test.go) plus +// the two new ones added for this wave. +func clearAdvanceEnv(t *testing.T) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + clearAllConfigEnv(t) + t.Setenv("DOJO_PERMISSIONS_MODE", "") + t.Setenv("DOJO_DELEGATION_MODEL", "") +} + +// ─── Defaults ──────────────────────────────────────────────────────────────── + +func TestLoad_Permissions_DefaultsToDefaultMode(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Permissions.Mode != "default" { + t.Errorf("Permissions.Mode: got %q, want %q", cfg.Permissions.Mode, "default") + } + if len(cfg.Permissions.Allowed) != 0 { + t.Errorf("Permissions.Allowed: got %v, want empty", cfg.Permissions.Allowed) + } +} + +func TestLoad_Delegation_DefaultsToEmptyModel(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Delegation.Model != "" { + t.Errorf("Delegation.Model: got %q, want empty", cfg.Delegation.Model) + } +} + +func TestLoad_Guardrails_Defaults(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if !cfg.Guardrails.Enabled { + t.Error("Guardrails.Enabled should default to true when settings.json omits the block") + } + if cfg.Guardrails.WarnAfter != 3 { + t.Errorf("Guardrails.WarnAfter: got %d, want 3", cfg.Guardrails.WarnAfter) + } + if cfg.Guardrails.HardAfter != 5 { + t.Errorf("Guardrails.HardAfter: got %d, want 5", cfg.Guardrails.HardAfter) + } +} + +func TestLoad_Skills_DefaultsToClaudeSkillsDir(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if len(cfg.Skills.ExternalDirs) != 1 || cfg.Skills.ExternalDirs[0] != ".claude/skills" { + t.Errorf("Skills.ExternalDirs: got %v, want [%q]", cfg.Skills.ExternalDirs, ".claude/skills") + } +} + +// ─── settings.json merge semantics (absent key keeps the default) ─────────── + +func TestLoad_Guardrails_PresentBlockWithoutEnabled_StaysOn(t *testing.T) { + clearAdvanceEnv(t) + + dojoDir := filepath.Join(os.Getenv("HOME"), ".dojo") + if err := os.MkdirAll(dojoDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + settings := map[string]any{ + "guardrails": map[string]any{"warn_after": 7}, // no "enabled" key + } + data, _ := json.Marshal(settings) + if err := os.WriteFile(filepath.Join(dojoDir, "settings.json"), data, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if !cfg.Guardrails.Enabled { + t.Error("a guardrails block without an 'enabled' key must keep the default (true)") + } + if cfg.Guardrails.WarnAfter != 7 { + t.Errorf("Guardrails.WarnAfter from settings: got %d, want 7", cfg.Guardrails.WarnAfter) + } + if cfg.Guardrails.HardAfter != 5 { + t.Errorf("Guardrails.HardAfter should keep its default: got %d, want 5", cfg.Guardrails.HardAfter) + } +} + +func TestLoad_Guardrails_SettingsDisables(t *testing.T) { + clearAdvanceEnv(t) + + dojoDir := filepath.Join(os.Getenv("HOME"), ".dojo") + if err := os.MkdirAll(dojoDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + settings := map[string]any{ + "guardrails": map[string]any{"enabled": false}, + } + data, _ := json.Marshal(settings) + if err := os.WriteFile(filepath.Join(dojoDir, "settings.json"), data, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Guardrails.Enabled { + t.Error(`settings.json "guardrails":{"enabled":false} should disable`) + } +} + +// ─── Round trip (Save then Load) ───────────────────────────────────────────── + +func TestSaveLoad_Permissions_RoundTrip(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + cfg.Permissions.Mode = "allowlist" + cfg.Permissions.Allowed = []string{"code.undo", "plugin.install", "craft.*"} + if err := cfg.Save(); err != nil { + t.Fatalf("Save() error: %v", err) + } + + reloaded, err := Load() + if err != nil { + t.Fatalf("reload error: %v", err) + } + if reloaded.Permissions.Mode != "allowlist" { + t.Errorf("Permissions.Mode: got %q, want %q", reloaded.Permissions.Mode, "allowlist") + } + want := []string{"code.undo", "plugin.install", "craft.*"} + if len(reloaded.Permissions.Allowed) != len(want) { + t.Fatalf("Permissions.Allowed length: got %d, want %d", len(reloaded.Permissions.Allowed), len(want)) + } + for i, p := range want { + if reloaded.Permissions.Allowed[i] != p { + t.Errorf("Permissions.Allowed[%d]: got %q, want %q", i, reloaded.Permissions.Allowed[i], p) + } + } +} + +func TestSaveLoad_Delegation_RoundTrip(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + cfg.Delegation.Model = "claude-haiku-cheap-lane" + if err := cfg.Save(); err != nil { + t.Fatalf("Save() error: %v", err) + } + + reloaded, err := Load() + if err != nil { + t.Fatalf("reload error: %v", err) + } + if reloaded.Delegation.Model != "claude-haiku-cheap-lane" { + t.Errorf("Delegation.Model: got %q, want %q", reloaded.Delegation.Model, "claude-haiku-cheap-lane") + } +} + +func TestSaveLoad_Guardrails_RoundTrip(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + cfg.Guardrails.Enabled = false + cfg.Guardrails.WarnAfter = 10 + cfg.Guardrails.HardAfter = 20 + if err := cfg.Save(); err != nil { + t.Fatalf("Save() error: %v", err) + } + + reloaded, err := Load() + if err != nil { + t.Fatalf("reload error: %v", err) + } + if reloaded.Guardrails.Enabled { + t.Error("Guardrails.Enabled explicit false should survive round trip") + } + if reloaded.Guardrails.WarnAfter != 10 { + t.Errorf("Guardrails.WarnAfter: got %d, want 10", reloaded.Guardrails.WarnAfter) + } + if reloaded.Guardrails.HardAfter != 20 { + t.Errorf("Guardrails.HardAfter: got %d, want 20", reloaded.Guardrails.HardAfter) + } +} + +func TestSaveLoad_Skills_RoundTrip(t *testing.T) { + clearAdvanceEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + cfg.Skills.ExternalDirs = []string{".claude/skills", "custom/skills-dir"} + if err := cfg.Save(); err != nil { + t.Fatalf("Save() error: %v", err) + } + + reloaded, err := Load() + if err != nil { + t.Fatalf("reload error: %v", err) + } + want := []string{".claude/skills", "custom/skills-dir"} + if len(reloaded.Skills.ExternalDirs) != len(want) { + t.Fatalf("Skills.ExternalDirs length: got %d, want %d", len(reloaded.Skills.ExternalDirs), len(want)) + } + for i, d := range want { + if reloaded.Skills.ExternalDirs[i] != d { + t.Errorf("Skills.ExternalDirs[%d]: got %q, want %q", i, reloaded.Skills.ExternalDirs[i], d) + } + } +} + +// ─── Env overrides ──────────────────────────────────────────────────────────── + +func TestLoad_EnvVar_PermissionsMode(t *testing.T) { + clearAdvanceEnv(t) + t.Setenv("DOJO_PERMISSIONS_MODE", "yolo") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Permissions.Mode != "yolo" { + t.Errorf("Permissions.Mode: got %q, want %q", cfg.Permissions.Mode, "yolo") + } +} + +func TestLoad_EnvVar_DelegationModel(t *testing.T) { + clearAdvanceEnv(t) + t.Setenv("DOJO_DELEGATION_MODEL", "claude-sonnet-cheap") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Delegation.Model != "claude-sonnet-cheap" { + t.Errorf("Delegation.Model: got %q, want %q", cfg.Delegation.Model, "claude-sonnet-cheap") + } +} + +// Env override discipline (b89b9c5): a transient env-sourced value must not +// get baked into settings.json by an unrelated Save(). + +func TestSave_StripsEnvOverride_PermissionsMode(t *testing.T) { + clearAdvanceEnv(t) + t.Setenv("DOJO_PERMISSIONS_MODE", "yolo") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Permissions.Mode != "yolo" { + t.Fatalf("sanity check failed: Permissions.Mode = %q", cfg.Permissions.Mode) + } + + // An unrelated save (e.g. /model set) must not bake the transient + // env-sourced mode into settings.json. + cfg.Defaults.Model = "claude-sonnet-4-6" + if err := cfg.Save(); err != nil { + t.Fatalf("Save() error: %v", err) + } + + t.Setenv("DOJO_PERMISSIONS_MODE", "") + reloaded, err := Load() + if err != nil { + t.Fatalf("reload error: %v", err) + } + if reloaded.Permissions.Mode != "default" { + t.Errorf("Permissions.Mode after reload: got %q, want default %q — Save() must not persist a still-in-effect env override", reloaded.Permissions.Mode, "default") + } + if reloaded.Defaults.Model != "claude-sonnet-4-6" { + t.Errorf("the real edit should persist: Defaults.Model = %q, want %q", reloaded.Defaults.Model, "claude-sonnet-4-6") + } +} + +func TestSave_StripsEnvOverride_DelegationModel(t *testing.T) { + clearAdvanceEnv(t) + t.Setenv("DOJO_DELEGATION_MODEL", "env-transient-model") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error: %v", err) + } + if cfg.Delegation.Model != "env-transient-model" { + t.Fatalf("sanity check failed: Delegation.Model = %q", cfg.Delegation.Model) + } + + cfg.Defaults.Model = "claude-sonnet-4-6" + if err := cfg.Save(); err != nil { + t.Fatalf("Save() error: %v", err) + } + + t.Setenv("DOJO_DELEGATION_MODEL", "") + reloaded, err := Load() + if err != nil { + t.Fatalf("reload error: %v", err) + } + if reloaded.Delegation.Model != "" { + t.Errorf("Delegation.Model after reload: got %q, want empty — Save() must not persist a still-in-effect env override", reloaded.Delegation.Model) + } +} + +// ─── Validate() ─────────────────────────────────────────────────────────────── + +func TestValidate_Permissions_ValidModes(t *testing.T) { + for _, mode := range []string{"", "default", "allowlist", "yolo"} { + cfg := &Config{ + Gateway: GatewayConfig{URL: "http://localhost:7340", Timeout: "60s"}, + Defaults: DefaultsConfig{Disposition: "balanced"}, + Permissions: PermissionsConfig{Mode: mode}, + } + if err := cfg.Validate(); err != nil { + t.Errorf("Validate() with permissions.mode=%q: unexpected error: %v", mode, err) + } + } +} + +func TestValidate_Permissions_InvalidMode(t *testing.T) { + cfg := &Config{ + Gateway: GatewayConfig{URL: "http://localhost:7340", Timeout: "60s"}, + Defaults: DefaultsConfig{Disposition: "balanced"}, + Permissions: PermissionsConfig{Mode: "sudo-everything"}, + } + err := cfg.Validate() + if err == nil { + t.Fatal("expected Validate() to fail for invalid permissions.mode, got nil") + } + if !strings.Contains(err.Error(), "permissions.mode") { + t.Errorf("error should mention permissions.mode, got: %v", err) + } +} diff --git a/internal/guardrail/guardrail.go b/internal/guardrail/guardrail.go new file mode 100644 index 0000000..4273c4e --- /dev/null +++ b/internal/guardrail/guardrail.go @@ -0,0 +1,129 @@ +// Package guardrail implements an advisory circuit breaker for the human +// REPL. It counts consecutive failures per caller-defined key and +// escalates a ready-to-print one-liner from silence, to a warning, to a +// "you're stuck" hard-stop message. It never blocks or refuses an action — +// Dojo's guardrail only gets louder, matching the advisory contract on +// internal/config.GuardrailsConfig (Enabled/WarnAfter/HardAfter, default +// enabled/3/5). +// +// Inspiration: Hermes' tool_loop_guardrails, which warn/hard-stop a tool +// loop after N exact_failure / same_tool_failure / idempotent_no_progress +// outcomes against the same key — a stuck-loop circuit breaker. Dojo's +// Tracker is the counting primitive underneath that idea, scoped down for +// a human-in-the-loop REPL: callers decide what a "key" is (a command +// string, a tool+error signature, ...) and what "failed" means at their +// call site; the tracker only ever advises, never blocks. +package guardrail + +import ( + "fmt" + "strings" + "sync" +) + +// Level is the escalation tier returned by Tracker.Record. +type Level int + +const ( + // None means no advice: the tracker is disabled, the outcome was a + // success (which resets the key), or the streak hasn't reached + // WarnAfter yet. + None Level = iota + // Warn means the key has failed exactly WarnAfter times in a row. + Warn + // Hard means the key has failed HardAfter or more times in a row — + // a stuck loop. Returned every time the count is at or past + // HardAfter, so a truly stuck loop keeps being told. + Hard +) + +// Advice is the result of one Tracker.Record call: an escalation Level and +// a ready-to-print message. Msg is empty when Level == None. +type Advice struct { + Level Level + Msg string // ready-to-print one-liner; empty when Level == None +} + +// Tracker counts CONSECUTIVE failures per key. A success for a key resets +// that key's counter to zero. Keys are caller-defined, e.g. +// "cmd:/code test" or "tool:web_fetch|conn refused" (see Signature for +// building the error-derived half of such a key). Safe for concurrent use. +type Tracker struct { + mu sync.Mutex + counts map[string]int + warnAfter int + hardAfter int + enabled bool +} + +// New builds a Tracker. Sanity clamps guard against a malformed config +// producing a tracker that never fires or fires nonsensically: +// warnAfter < 1 becomes 3; hardAfter <= warnAfter becomes warnAfter + 2 — +// the same 3/5 shape internal/config.GuardrailsConfig defaults to. +func New(warnAfter, hardAfter int, enabled bool) *Tracker { + if warnAfter < 1 { + warnAfter = 3 + } + if hardAfter <= warnAfter { + hardAfter = warnAfter + 2 + } + return &Tracker{ + counts: make(map[string]int), + warnAfter: warnAfter, + hardAfter: hardAfter, + enabled: enabled, + } +} + +// Record registers one outcome for key and returns advice. Rules: +// +// - disabled: always Advice{Level: None}. +// - failed=false: resets key's counter, returns None. +// - failed=true: increments key's counter; at exactly warnAfter +// returns Warn; at and after hardAfter returns Hard (every time, so a +// truly stuck loop keeps being told); strictly between warnAfter and +// hardAfter, returns None. +func (t *Tracker) Record(key string, failed bool) Advice { + if !t.enabled { + return Advice{Level: None} + } + + t.mu.Lock() + defer t.mu.Unlock() + + if !failed { + delete(t.counts, key) + return Advice{Level: None} + } + + t.counts[key]++ + n := t.counts[key] + + switch { + case n >= t.hardAfter: + return Advice{ + Level: Hard, + Msg: fmt.Sprintf("guardrail: %s is stuck (%d consecutive identical failures) — stop and change strategy, or check config vs code (see debugging gate)", key, n), + } + case n == t.warnAfter: + return Advice{ + Level: Warn, + Msg: fmt.Sprintf("guardrail: %s has failed %d times consecutively with the same signature — consider a different approach", key, n), + } + default: + return Advice{Level: None} + } +} + +// Signature normalizes an error string into a stable short key component: +// lowercase, whitespace runs collapsed to a single space, truncated to the +// first 80 bytes. Helper for callers building keys, e.g. +// "tool:web_fetch|" + Signature(err.Error()). +func Signature(err string) string { + s := strings.ToLower(err) + s = strings.Join(strings.Fields(s), " ") + if len(s) > 80 { + s = s[:80] + } + return s +} diff --git a/internal/guardrail/guardrail_test.go b/internal/guardrail/guardrail_test.go new file mode 100644 index 0000000..dac306f --- /dev/null +++ b/internal/guardrail/guardrail_test.go @@ -0,0 +1,174 @@ +package guardrail + +import ( + "fmt" + "strings" + "testing" +) + +func TestRecord_ResetOnSuccess(t *testing.T) { + tr := New(3, 5, true) + + tr.Record("k", true) + tr.Record("k", true) + if adv := tr.Record("k", false); adv.Level != None || adv.Msg != "" { + t.Fatalf("Record(success) = %+v; want {None \"\"}", adv) + } + + // Counter must have reset to 0: two more failures should NOT reach + // warnAfter (3) yet. + tr.Record("k", true) + adv := tr.Record("k", true) + if adv.Level != None { + t.Fatalf("after reset, 2nd failure Level = %v; want None (not yet at warnAfter)", adv.Level) + } +} + +func TestRecord_WarnFiresExactlyOnceAtThreshold(t *testing.T) { + tr := New(3, 5, true) + + var warnCount int + var levels []Level + for i := 0; i < 5; i++ { + adv := tr.Record("k", true) + levels = append(levels, adv.Level) + if adv.Level == Warn { + warnCount++ + want := fmt.Sprintf("guardrail: k has failed %d times consecutively with the same signature — consider a different approach", i+1) + if adv.Msg != want { + t.Fatalf("Warn Msg = %q; want %q", adv.Msg, want) + } + } + } + if warnCount != 1 { + t.Fatalf("warnCount over 5 consecutive failures = %d; want exactly 1 (levels=%v)", warnCount, levels) + } + if levels[2] != Warn { // 3rd consecutive failure (0-indexed: i==2) + t.Fatalf("levels[2] (3rd failure) = %v; want Warn", levels[2]) + } +} + +func TestRecord_HardFiresAtAndAfterHardAfter(t *testing.T) { + tr := New(3, 5, true) + + for i := 0; i < 4; i++ { + adv := tr.Record("k", true) + if adv.Level == Hard { + t.Fatalf("failure #%d prematurely returned Hard", i+1) + } + } + + adv := tr.Record("k", true) // 5th consecutive failure == hardAfter + if adv.Level != Hard { + t.Fatalf("5th consecutive failure Level = %v; want Hard", adv.Level) + } + want := "guardrail: k is stuck (5 consecutive identical failures) — stop and change strategy, or check config vs code (see debugging gate)" + if adv.Msg != want { + t.Fatalf("Hard Msg = %q; want %q", adv.Msg, want) + } + + // A truly stuck loop keeps being told: 6th, 7th failures stay Hard. + for i, wantCount := range []int{6, 7} { + adv := tr.Record("k", true) + if adv.Level != Hard { + t.Fatalf("failure #%d Level = %v; want Hard (past hardAfter)", wantCount, adv.Level) + } + want := fmt.Sprintf("guardrail: k is stuck (%d consecutive identical failures) — stop and change strategy, or check config vs code (see debugging gate)", wantCount) + if adv.Msg != want { + t.Fatalf("failure #%d (i=%d) Msg = %q; want %q", wantCount, i, adv.Msg, want) + } + } +} + +func TestRecord_Disabled(t *testing.T) { + // warnAfter=1, hardAfter=3 would fire immediately if enabled -- proves + // the disabled short-circuit, not just a threshold never reached. + tr := New(1, 3, false) + + for i := 0; i < 5; i++ { + adv := tr.Record("k", true) + if adv.Level != None || adv.Msg != "" { + t.Fatalf("disabled tracker Record #%d = %+v; want {None \"\"}", i, adv) + } + } +} + +func TestNew_Clamps(t *testing.T) { + tests := []struct { + name string + warnAfter int + hardAfter int + wantWarnAfter int + wantHardAfter int + }{ + {"warnAfter zero", 0, 5, 3, 5}, + {"warnAfter negative", -1, 5, 3, 5}, + {"hardAfter equal warnAfter", 3, 3, 3, 5}, + {"hardAfter less than warnAfter", 4, 2, 4, 6}, + {"both invalid", 0, 0, 3, 5}, + {"already valid, untouched", 2, 4, 2, 4}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tr := New(tc.warnAfter, tc.hardAfter, true) + if tr.warnAfter != tc.wantWarnAfter { + t.Errorf("warnAfter = %d; want %d", tr.warnAfter, tc.wantWarnAfter) + } + if tr.hardAfter != tc.wantHardAfter { + t.Errorf("hardAfter = %d; want %d", tr.hardAfter, tc.wantHardAfter) + } + }) + } +} + +func TestSignature(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"lowercases", "Connection REFUSED", "connection refused"}, + {"collapses whitespace", "conn refused\n\tretry", "conn refused retry"}, + {"truncates to first 80 bytes", strings.Repeat("a", 100), strings.Repeat("a", 80)}, + {"empty string", "", ""}, + {"already short and clean", "conn refused", "conn refused"}, + {"leading and trailing whitespace", " conn refused ", "conn refused"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := Signature(tc.in) + if got != tc.want { + t.Errorf("Signature(%q) = %q; want %q", tc.in, got, tc.want) + } + if len(got) > 80 { + t.Errorf("Signature(%q) len = %d; want <= 80", tc.in, len(got)) + } + }) + } +} + +// TestRecord_Concurrency runs many t.Parallel goroutines hammering Record +// against a shared Tracker (some sharing keys, to exercise the mutex on +// the same map entries). It makes no assertion on final counts -- ordering +// across goroutines is nondeterministic by design -- its job is to prove +// no data race and no panic. Run locally with the race detector: +// +// go test ./internal/guardrail -count=1 -race +func TestRecord_Concurrency(t *testing.T) { + tr := New(3, 5, true) + keys := []string{"a", "b", "c", "d"} + + for g := 0; g < 20; g++ { + g := g + t.Run(fmt.Sprintf("worker-%d", g), func(t *testing.T) { + t.Parallel() + key := keys[g%len(keys)] + for i := 0; i < 200; i++ { + _ = tr.Record(key, i%2 == 0) + } + _ = Signature("concurrent CALL failing\tagain") + }) + } +} diff --git a/internal/hooks/runner.go b/internal/hooks/runner.go index 5cad226..8af4d0d 100644 --- a/internal/hooks/runner.go +++ b/internal/hooks/runner.go @@ -13,24 +13,36 @@ import ( "path" "runtime" "strings" + "sync" "time" + "unicode/utf8" "github.com/DojoGenesis/cli/internal/plugins" ) // Event names that dojo-cli fires. const ( - EventPreCommand = "PreCommand" - EventPostCommand = "PostCommand" - EventPostSkill = "PostSkill" - EventPostAgent = "PostAgent" - EventSessionStart = "SessionStart" - EventSessionEnd = "SessionEnd" + EventPreCommand = "PreCommand" + EventPostCommand = "PostCommand" + EventPostSkill = "PostSkill" + EventPostAgent = "PostAgent" + EventSessionStart = "SessionStart" + EventSessionEnd = "SessionEnd" + EventUserPromptSubmit = "UserPromptSubmit" ) // Runner executes hook rules for a given event. type Runner struct { plugins []plugins.Plugin + + // warned de-dupes the "hook type X is not implemented" warning for the + // unimplemented prompt/agent types: keys are "<plugin>\x00<type>", so the + // warning fires at most once per (plugin, type) for the life of this + // Runner rather than on every fire. The REPL builds exactly one Runner per + // process (commands.Registry.Runner), so per-Runner is per-process in + // practice. Zero value is ready to use; sync.Map so the async goroutine + // path can warn concurrently. + warned sync.Map } // New creates a Runner from a list of loaded plugins. @@ -38,12 +50,73 @@ func New(ps []plugins.Plugin) *Runner { return &Runner{plugins: ps} } -// Fire runs all hook rules matching the given event name across all plugins. -// Supported hook types: command, prompt, agent, http. -// Async hooks are run in a goroutine. Sync hooks block until completion. +// FireResult is the classified outcome of FireChecked. The zero value means +// every matched hook ran cleanly (or none matched). Exactly one of the failure +// channels is ever set, and Blocked takes priority: FireChecked short-circuits +// on the first command-hook failure and reports it as Blocked when its rule is +// marked Blocking, or as a non-blocking Err otherwise. +type FireResult struct { + // Blocked is true when a command hook in a Blocking:true rule failed or + // exited non-zero — the caller MUST abort the action it was about to take. + // Plugin, Event, and Reason name the offending hook for the abort message. + Blocked bool + Plugin string // plugin that owns the blocking hook (set when Blocked) + Event string // event the blocking hook fired on (set when Blocked) + Reason string // failure detail — the hook's error/stderr (set when Blocked) + + // Err is a NON-blocking hook failure: the pre-Blocking behavior in which a + // sync command hook failed. The caller logs it and continues — it does NOT + // abort. Nil unless a non-blocking hook failed. Blocked and Err are never + // both set (Blocked short-circuits first). + Err error +} + +// Message renders the standard one-line "blocked by <plugin>/<event>: <reason>" +// string the REPL prints when aborting. The reason is flattened to a single +// line (a hook can print multi-line stderr) and length-capped so a wall of +// output never floods the prompt. +func (fr FireResult) Message() string { + reason := strings.Join(strings.Fields(fr.Reason), " ") + const maxReason = 200 + if utf8.RuneCountInString(reason) > maxReason { + reason = string([]rune(reason)[:maxReason]) + "…" + } + return fmt.Sprintf("[hooks] blocked by %s/%s: %s", fr.Plugin, fr.Event, reason) +} + +// Fire runs all hook rules matching the given event name across all plugins and +// collapses the outcome into a single error, preserving the pre-Blocking +// contract for callers that only care whether "something failed" (e.g. +// /hooks fire in internal/commands, and the SessionStart/SessionEnd/PostCommand +// call sites that log-and-continue). A blocking failure and a non-blocking +// failure both surface here as a non-nil error; callers that must actually +// honor a block (abort the caller's action) use FireChecked instead. +func (r *Runner) Fire(ctx context.Context, event string, payload map[string]any) error { + res := r.FireChecked(ctx, event, payload) + if res.Blocked { + return fmt.Errorf("hook error (%s/%s): %s", res.Plugin, res.Event, strings.TrimSpace(res.Reason)) + } + return res.Err +} + +// FireChecked runs all hook rules matching the given event name across all +// plugins and returns a classified FireResult distinguishing (a) all-clean, +// (b) a non-blocking failure to log-and-continue, and (c) a Blocking-rule +// command hook that failed and must abort the caller. +// +// Supported hook types: command, prompt, agent, http. Async hooks run in a +// goroutine (errors logged, never block). Sync hooks run to completion. // ctx cancellation prevents new async hooks from starting and kills // already-running processes (exec.CommandContext sends SIGKILL). -func (r *Runner) Fire(ctx context.Context, event string, payload map[string]any) error { +// +// Blocking is command-only and synchronous: a command hook in a Blocking:true +// rule runs to completion (ignoring its Async flag — you cannot block on a +// fire-and-forget hook) and a non-zero exit returns a Blocked result. An http +// hook is fire-and-forget by design (runHTTPHook always succeeds, logging any +// error) and prompt/agent are unimplemented no-ops, so none of those can ever +// block — even inside a Blocking rule. Like the pre-Blocking Fire(), the first +// command-hook failure short-circuits the remaining rules and hooks. +func (r *Runner) FireChecked(ctx context.Context, event string, payload map[string]any) FireResult { for _, p := range r.plugins { for _, rule := range p.HookRules { if !strings.EqualFold(rule.Event, event) { @@ -63,9 +136,23 @@ func (r *Runner) Fire(ctx context.Context, event string, payload map[string]any) for _, h := range rule.Hooks { pluginName := p.Name pluginPath := p.Path - isAsync := h.Async - if isAsync { + // Blocking gate: a command hook in a Blocking rule runs + // synchronously and a failure vetoes the caller. Only command + // hooks reach this branch (see doc comment). + if rule.Blocking && h.Type == "command" { + if err := r.runHook(ctx, h, pluginName, pluginPath, payload); err != nil { + return FireResult{ + Blocked: true, + Plugin: pluginName, + Event: event, + Reason: err.Error(), + } + } + continue + } + + if h.Async { hCopy := h go func() { select { @@ -73,19 +160,23 @@ func (r *Runner) Fire(ctx context.Context, event string, payload map[string]any) return default: } - if err := runHook(ctx, hCopy, pluginPath, payload); err != nil { + if err := r.runHook(ctx, hCopy, pluginName, pluginPath, payload); err != nil { log.Printf("[hooks] async hook error (%s/%s): %v", pluginName, event, err) } }() - } else { - if err := runHook(ctx, h, pluginPath, payload); err != nil { - return fmt.Errorf("hook error (%s/%s): %w", pluginName, event, err) - } + continue + } + + // Sync, non-blocking: the pre-Blocking behavior — a failure is + // reported to the caller, which logs it and continues (the + // command still runs). Short-circuits the rest, as before. + if err := r.runHook(ctx, h, pluginName, pluginPath, payload); err != nil { + return FireResult{Err: fmt.Errorf("hook error (%s/%s): %w", pluginName, event, err)} } } } } - return nil + return FireResult{} } // matcherMatches returns true if the matcher glob matches the command in the payload, @@ -138,20 +229,17 @@ func conditionTrue(cond string) bool { } // runHook dispatches to the appropriate executor based on hook type. -func runHook(ctx context.Context, h plugins.HookDef, pluginRoot string, payload map[string]any) error { +func (r *Runner) runHook(ctx context.Context, h plugins.HookDef, pluginName, pluginRoot string, payload map[string]any) error { switch h.Type { case "command": - return runCommand(ctx, h.Command, pluginRoot) - case "prompt": - fmt.Printf("[hook:prompt] %s\n", h.Prompt) - return nil - case "agent": - // Use Command as agent ID/description if Prompt is empty (fallback). - desc := h.Prompt - if desc == "" { - desc = h.Command - } - fmt.Printf("[hook:agent] %s\n", desc) + return runCommand(ctx, h.Command, pluginRoot, promptFromPayload(payload)) + case "prompt", "agent": + // prompt/agent dispatch is not implemented. These used to print a + // "[hook:prompt] …" / "[hook:agent] …" label to stdout that looked like + // the hook had done something — a silent no-op wearing a success mask. + // Warn honestly instead, once per (plugin, type) so a hook that fires + // every turn doesn't spam the stream. + r.warnUnimplemented(pluginName, h.Type) return nil case "http": return runHTTPHook(ctx, h.URL, payload) @@ -161,6 +249,47 @@ func runHook(ctx context.Context, h plugins.HookDef, pluginRoot string, payload } } +// warnUnimplemented prints, at most once per (plugin, hook-type) for this +// Runner, a stderr warning that a hook type is not implemented and is being +// skipped. Concurrency-safe: the async path calls runHook from a goroutine. +func (r *Runner) warnUnimplemented(pluginName, hookType string) { + key := pluginName + "\x00" + hookType + if _, loaded := r.warned.LoadOrStore(key, struct{}{}); loaded { + return + } + fmt.Fprintf(os.Stderr, "[hooks] hook type %q is not implemented; skipping (plugin %s)\n", hookType, pluginName) +} + +// promptFromPayload extracts the user prompt an event carries under the +// "prompt" key. Only UserPromptSubmit sets it (see the REPL chat path); every +// other event's payload lacks the key, so this returns "" and DOJO_PROMPT is +// left unset for those hooks. +func promptFromPayload(payload map[string]any) string { + if payload == nil { + return "" + } + s, _ := payload["prompt"].(string) + return s +} + +// maxPromptBytes caps the DOJO_PROMPT environment value handed to +// UserPromptSubmit command hooks, so an arbitrarily long chat message can't +// bloat the child process's environment block. +const maxPromptBytes = 4096 + +// truncateBytes returns s clamped to at most maxBytes bytes, trimmed back to a +// UTF-8 rune boundary so a multi-byte rune is never split mid-sequence. +func truncateBytes(s string, maxBytes int) string { + if len(s) <= maxBytes { + return s + } + truncated := s[:maxBytes] + for len(truncated) > 0 && !utf8.ValidString(truncated) { + truncated = truncated[:len(truncated)-1] + } + return truncated +} + // runHTTPHook POSTs the payload as JSON to the given URL. // HTTP errors are logged but do not fail the command. func runHTTPHook(ctx context.Context, url string, payload map[string]any) error { @@ -206,12 +335,23 @@ func shellFor(goos string) (exe string, flag string) { // the plugin's directory. The command is run through sh -c (cmd /C on // Windows — see shellFor) so it can use shell expansion (e.g. variable // substitution, quoting). -func runCommand(ctx context.Context, command, pluginRoot string) error { +// +// When prompt is non-empty (UserPromptSubmit hooks), the user's chat text is +// exposed to the hook as DOJO_PROMPT, truncated to maxPromptBytes. It is passed +// through the process environment exactly like CLAUDE_PLUGIN_ROOT and is NEVER +// interpolated into the command string, so a prompt containing shell +// metacharacters ($(...), backticks, ;, |, &) is inert — the hook reads it via +// $DOJO_PROMPT, it is never evaluated as part of the command. +func runCommand(ctx context.Context, command, pluginRoot, prompt string) error { exe, flag := shellFor(runtime.GOOS) cmd := exec.CommandContext(ctx, exe, flag, command) - cmd.Env = append(cmd.Environ(), + env := append(cmd.Environ(), "CLAUDE_PLUGIN_ROOT="+pluginRoot, ) + if prompt != "" { + env = append(env, "DOJO_PROMPT="+truncateBytes(prompt, maxPromptBytes)) + } + cmd.Env = env out, err := cmd.CombinedOutput() if err != nil { if len(out) > 0 { diff --git a/internal/hooks/runner_blocking_test.go b/internal/hooks/runner_blocking_test.go new file mode 100644 index 0000000..6530fa9 --- /dev/null +++ b/internal/hooks/runner_blocking_test.go @@ -0,0 +1,314 @@ +package hooks + +import ( + "bytes" + "context" + "io" + "os" + "path/filepath" + "strings" + "testing" + "unicode/utf8" + + "github.com/DojoGenesis/cli/internal/plugins" +) + +// ─── captureStderr ──────────────────────────────────────────────────────────── + +// captureStderr redirects os.Stderr for the duration of fn and returns whatever +// was written. Used to assert warnUnimplemented's fmt.Fprintf(os.Stderr, …). +// The log package holds its own os.Stderr reference captured at init, so log +// output is NOT redirected here — the capture stays clean of log noise. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + old := os.Stderr + rd, wr, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stderr = wr + done := make(chan string, 1) + go func() { + var buf bytes.Buffer + _, _ = io.Copy(&buf, rd) + done <- buf.String() + }() + fn() + _ = wr.Close() + os.Stderr = old + out := <-done + _ = rd.Close() + return out +} + +// ─── Blocking semantics ─────────────────────────────────────────────────────── + +// TestFireChecked_BlockingHookFailure_Blocks proves a command hook in a +// Blocking:true rule that exits non-zero returns a Blocked result naming the +// plugin+event, and that the Fire() wrapper still surfaces it as an error for +// the /hooks fire path in internal/commands. +func TestFireChecked_BlockingHookFailure_Blocks(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "gatekeeper", + Path: t.TempDir(), + HookRules: []plugins.HookRule{{ + Event: EventPreCommand, + Blocking: true, + Hooks: []plugins.HookDef{{Type: "command", Command: "echo denied >&2; exit 3"}}, + }}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventPreCommand, map[string]any{"command": "/deploy"}) + if !res.Blocked { + t.Fatalf("expected Blocked, got %+v", res) + } + if res.Plugin != "gatekeeper" { + t.Errorf("Plugin = %q, want gatekeeper", res.Plugin) + } + if res.Event != EventPreCommand { + t.Errorf("Event = %q, want %q", res.Event, EventPreCommand) + } + if res.Reason == "" { + t.Error("Reason should carry the hook's failure detail") + } + if res.Err != nil { + t.Errorf("Blocked result should not also set Err, got %v", res.Err) + } + if msg := res.Message(); !strings.Contains(msg, "blocked by gatekeeper/PreCommand") { + t.Errorf("Message() = %q, want it to name plugin/event", msg) + } + + // Backward compat: the Fire() wrapper collapses a block into an error so + // /hooks fire (internal/commands/cmd_system.go) still reports it. + if err := r.Fire(context.Background(), EventPreCommand, map[string]any{"command": "/deploy"}); err == nil { + t.Error("Fire() should return an error when a blocking hook fails") + } +} + +// TestFireChecked_BlockingHookSucceeds_NotBlocked proves a Blocking rule whose +// command succeeds still runs and does not block. +func TestFireChecked_BlockingHookSucceeds_NotBlocked(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "ran.txt") + ps := []plugins.Plugin{{ + Name: "allow", + Path: tmp, + HookRules: []plugins.HookRule{{Event: EventPreCommand, Blocking: true, Hooks: []plugins.HookDef{{Type: "command", Command: "touch " + marker}}}}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventPreCommand, map[string]any{"command": "/ok"}) + if res.Blocked || res.Err != nil { + t.Fatalf("a clean blocking hook should not fail: %+v", res) + } + if _, err := os.Stat(marker); os.IsNotExist(err) { + t.Error("a blocking hook that succeeds should still have run") + } +} + +// TestFireChecked_NonBlockingCommandFailure_ContinuesNotBlocked proves a +// failing command hook whose rule is NOT Blocking surfaces as Err (log-and- +// continue), never as Blocked — the pre-Blocking behavior. +func TestFireChecked_NonBlockingCommandFailure_ContinuesNotBlocked(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "noisy", + Path: t.TempDir(), + HookRules: []plugins.HookRule{{Event: EventPreCommand, Blocking: false, Hooks: []plugins.HookDef{{Type: "command", Command: "exit 1"}}}}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventPreCommand, map[string]any{"command": "/x"}) + if res.Blocked { + t.Fatal("a non-blocking failure must NOT block") + } + if res.Err == nil { + t.Fatal("a non-blocking failure should surface as Err (logged and continued)") + } +} + +// TestFireChecked_BlockingHTTPHook_CannotBlock proves only command hooks block: +// an http hook in a Blocking rule is still fire-and-forget even against a dead +// endpoint. +func TestFireChecked_BlockingHTTPHook_CannotBlock(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "http-block", + HookRules: []plugins.HookRule{{Event: EventPreCommand, Blocking: true, Hooks: []plugins.HookDef{{Type: "http", URL: "http://127.0.0.1:0"}}}}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventPreCommand, map[string]any{"command": "/x"}) + if res.Blocked { + t.Error("an http hook in a Blocking rule must not block (fire-and-forget by design)") + } +} + +// TestFireChecked_BlockingCommandHook_IgnoresAsyncAndBlocks proves a command +// hook in a Blocking rule runs synchronously even when marked Async:true — you +// cannot block on a fire-and-forget hook — so its failure still blocks. +func TestFireChecked_BlockingCommandHook_IgnoresAsyncAndBlocks(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "async-but-blocking", + Path: t.TempDir(), + HookRules: []plugins.HookRule{{Event: EventPreCommand, Blocking: true, Hooks: []plugins.HookDef{{Type: "command", Command: "exit 1", Async: true}}}}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventPreCommand, map[string]any{"command": "/x"}) + if !res.Blocked { + t.Error("a Blocking command hook must run synchronously and block on failure, even when Async:true") + } +} + +// ─── UserPromptSubmit: env delivery, non-interpolation, truncation ──────────── + +// TestFireChecked_UserPromptSubmit_DeliversDojoPromptEnv proves the prompt text +// reaches a command hook via $DOJO_PROMPT, and that matching on the literal +// "chat" works. +func TestFireChecked_UserPromptSubmit_DeliversDojoPromptEnv(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "prompt.txt") + const prompt = "hello from the user" + ps := []plugins.Plugin{{ + Name: "ups-plugin", + Path: tmp, + HookRules: []plugins.HookRule{{ + Event: EventUserPromptSubmit, + Matcher: "chat", + Hooks: []plugins.HookDef{{Type: "command", Command: `printf '%s' "$DOJO_PROMPT" > ` + marker}}, + }}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventUserPromptSubmit, map[string]any{"command": "chat", "prompt": prompt}) + if res.Blocked || res.Err != nil { + t.Fatalf("FireChecked returned failure: %+v", res) + } + got, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("reading marker: %v", err) + } + if string(got) != prompt { + t.Errorf("DOJO_PROMPT delivered %q, want %q", got, prompt) + } +} + +// TestFireChecked_UserPromptSubmit_PromptNotShellInterpolated proves the prompt +// is passed through the environment, never spliced into the command string: a +// $(...) subshell in the prompt must NOT execute. +func TestFireChecked_UserPromptSubmit_PromptNotShellInterpolated(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "out.txt") + pwned := filepath.Join(tmp, "pwned.txt") + prompt := "safe $(touch " + pwned + ") text" + ps := []plugins.Plugin{{ + Name: "safe-plugin", + Path: tmp, + HookRules: []plugins.HookRule{{ + Event: EventUserPromptSubmit, + Matcher: "chat", + Hooks: []plugins.HookDef{{Type: "command", Command: `printf '%s' "$DOJO_PROMPT" > ` + marker}}, + }}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventUserPromptSubmit, map[string]any{"command": "chat", "prompt": prompt}) + if res.Blocked || res.Err != nil { + t.Fatalf("unexpected failure: %+v", res) + } + if _, err := os.Stat(pwned); !os.IsNotExist(err) { + t.Fatal("prompt was shell-interpolated: the $(...) subshell executed") + } + got, _ := os.ReadFile(marker) + if string(got) != prompt { + t.Errorf("DOJO_PROMPT = %q, want the literal prompt %q", got, prompt) + } +} + +// TestFireChecked_UserPromptSubmit_TruncatesDojoPrompt proves an oversized +// prompt is clamped to maxPromptBytes before it reaches the hook environment. +func TestFireChecked_UserPromptSubmit_TruncatesDojoPrompt(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "len.txt") + big := strings.Repeat("a", maxPromptBytes+512) + ps := []plugins.Plugin{{ + Name: "trunc-plugin", + Path: tmp, + HookRules: []plugins.HookRule{{ + Event: EventUserPromptSubmit, + Matcher: "chat", + Hooks: []plugins.HookDef{{Type: "command", Command: `printf '%s' "$DOJO_PROMPT" > ` + marker}}, + }}, + }} + r := New(ps) + + res := r.FireChecked(context.Background(), EventUserPromptSubmit, map[string]any{"command": "chat", "prompt": big}) + if res.Blocked || res.Err != nil { + t.Fatalf("unexpected failure: %+v", res) + } + got, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("reading marker: %v", err) + } + if len(got) != maxPromptBytes { + t.Errorf("DOJO_PROMPT length = %d, want %d (truncated)", len(got), maxPromptBytes) + } +} + +// ─── Honest no-op warnings (warn-once per plugin+type) ──────────────────────── + +// TestWarnUnimplemented_OncePerPluginType proves prompt/agent hooks emit a +// single stderr warning per (plugin, type) for the life of the Runner — never +// the old silent stdout label, and never spammed on repeated fires. +func TestWarnUnimplemented_OncePerPluginType(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "warn-plugin", + HookRules: []plugins.HookRule{{ + Event: EventPreCommand, + Hooks: []plugins.HookDef{ + {Type: "prompt", Prompt: "do X"}, + {Type: "prompt", Prompt: "do Y"}, // same (plugin, type) → still one warning + {Type: "agent", Command: "agent-1"}, + }, + }}, + }} + r := New(ps) + + out := captureStderr(t, func() { + _ = r.Fire(context.Background(), EventPreCommand, nil) + _ = r.Fire(context.Background(), EventPreCommand, nil) // fire again — must NOT re-warn + }) + + if n := strings.Count(out, `hook type "prompt" is not implemented`); n != 1 { + t.Errorf("prompt warning fired %d times, want exactly 1; stderr:\n%s", n, out) + } + if n := strings.Count(out, `hook type "agent" is not implemented`); n != 1 { + t.Errorf("agent warning fired %d times, want exactly 1; stderr:\n%s", n, out) + } + if !strings.Contains(out, "(plugin warn-plugin)") { + t.Errorf("warning should name the plugin; stderr:\n%s", out) + } +} + +// ─── truncateBytes ──────────────────────────────────────────────────────────── + +func TestTruncateBytes(t *testing.T) { + if got := truncateBytes("hello", maxPromptBytes); got != "hello" { + t.Errorf("short string changed: %q", got) + } + if got := truncateBytes(strings.Repeat("a", maxPromptBytes+904), maxPromptBytes); len(got) != maxPromptBytes { + t.Errorf("ascii truncation len = %d, want %d", len(got), maxPromptBytes) + } + // 3-byte runes: truncating 4096 bytes must land on a rune boundary. 1365*3 = + // 4095 is the largest multiple of 3 <= 4096, so the split rune is dropped. + euro := truncateBytes(strings.Repeat("€", 3000), maxPromptBytes) + if len(euro) > maxPromptBytes { + t.Errorf("multibyte truncation exceeded cap: %d", len(euro)) + } + if !utf8.ValidString(euro) { + t.Error("multibyte truncation split a rune (invalid UTF-8)") + } + if len(euro) != 4095 { + t.Errorf("euro truncation len = %d, want 4095 (rune boundary)", len(euro)) + } +} diff --git a/internal/permissions/confirm.go b/internal/permissions/confirm.go new file mode 100644 index 0000000..96d861b --- /dev/null +++ b/internal/permissions/confirm.go @@ -0,0 +1,46 @@ +package permissions + +import ( + "bufio" + "fmt" + "io" + "os" + "strings" + + "golang.org/x/term" +) + +// ConfirmInteractive prompts `allow <action>? <detail> [y/N] ` on stdout and +// reads a single line from stdin, returning true only when the trimmed +// response is "y" or "yes" (case-insensitive). +// +// If stdin is not attached to a terminal — the CLI is running one-shot, +// piped, or otherwise non-interactively — this does not print the prompt or +// block waiting for input: it returns false immediately, since there is no +// human present to answer. Callers should follow a false return by printing +// Explain(action)'s message, which advises --yolo or permissions.allowed as +// the non-interactive paths forward. +func ConfirmInteractive(action, detail string) bool { + if !isTerminal(os.Stdin) { + return false + } + + fmt.Printf("allow %s? %s [y/N] ", action, detail) + + line, err := bufio.NewReader(os.Stdin).ReadString('\n') + if err != nil && err != io.EOF { + return false + } + line = strings.TrimSpace(line) + if line == "" { + return false + } + return strings.EqualFold(line, "y") || strings.EqualFold(line, "yes") +} + +// isTerminal reports whether f is attached to a terminal. It is checked at +// call time rather than cached, so tests can swap os.Stdin for a pipe (never +// a terminal) and observe ConfirmInteractive's non-interactive short-circuit. +func isTerminal(f *os.File) bool { + return term.IsTerminal(int(f.Fd())) +} diff --git a/internal/permissions/explain.go b/internal/permissions/explain.go new file mode 100644 index 0000000..706eb8f --- /dev/null +++ b/internal/permissions/explain.go @@ -0,0 +1,15 @@ +package permissions + +import "fmt" + +// Explain returns the one-line message describing why action was denied — +// by allowlist mode, or because ConfirmInteractive returned false — naming +// both escape hatches available to the user: adding the action (or a +// covering glob) to permissions.allowed in ~/.dojo/settings.json, or +// re-running with --yolo. +func Explain(action string) string { + return fmt.Sprintf( + "permission denied: %q (add to permissions.allowed in ~/.dojo/settings.json, or run with --yolo)", + action, + ) +} diff --git a/internal/permissions/permissions.go b/internal/permissions/permissions.go new file mode 100644 index 0000000..4a1064c --- /dev/null +++ b/internal/permissions/permissions.go @@ -0,0 +1,128 @@ +// Package permissions implements the dojo CLI's action-permission gate — the +// logic that decides whether a given action may proceed silently, must be +// confirmed interactively, or is refused outright. +// +// Actions are named as dot-paths following the convention +// <command>.<subcommand-or-action>, e.g. "code.undo", "plugin.install", +// "craft.scaffold". Patterns in permissions.allowed (see +// internal/config.PermissionsConfig) match a dot-path either exactly, or — +// if the pattern ends in "*" — as a prefix: "craft.*" matches +// "craft.scaffold" and "craft.go-service"; a bare "*" (empty prefix) matches +// every action. +// +// Check takes mode and allowed as plain arguments rather than importing +// internal/config directly, so this package stays dependency-light and +// trivially testable in isolation. Callers thread cfg.Permissions.Mode and +// cfg.Permissions.Allowed through at the call site. +package permissions + +import ( + "fmt" + "os" + "strings" + "sync" +) + +// Decision is the outcome of Check: whether an action may proceed, must be +// confirmed interactively, or is refused outright. +type Decision int + +const ( + // Allow means the action proceeds silently — no prompt, no output. + Allow Decision = iota + // Confirm means the caller must obtain interactive confirmation (see + // ConfirmInteractive) before the action proceeds. + Confirm + // Deny means the action is refused; callers should surface Explain's + // message and stop. + Deny +) + +// String renders d for logs and test failure messages. +func (d Decision) String() string { + switch d { + case Allow: + return "Allow" + case Confirm: + return "Confirm" + case Deny: + return "Deny" + default: + return fmt.Sprintf("Decision(%d)", int(d)) + } +} + +// yoloWarnOnce guards the single per-process YOLO warning emitted by Check +// — see warnYolo. +var yoloWarnOnce sync.Once + +// Check evaluates action (a dot-path like "code.undo", "plugin.install", +// "craft.scaffold") against the permission configuration described by mode +// and allowed, and reports whether it may proceed. +// +// mode is validated defensively: any value other than "allowlist" or "yolo" +// — including "", "default", or an unrecognized string — is treated as +// "default". This mirrors internal/config.Config, where Permissions.Mode +// defaults to "default" and Validate() rejects unknown modes before they +// would reach here — but Check does not assume its caller validated first. +// +// - yolo: always Allow, for every action, regardless of allowed. +// Emits one YOLO warning to stderr per process (see warnYolo). +// - allowlist: Allow if action matches any pattern in allowed (see the +// package doc comment for match rules); otherwise Deny. +// - default: Allow if action matches any pattern in allowed; otherwise +// Confirm — the caller should then call ConfirmInteractive. +func Check(mode string, allowed []string, action string) Decision { + switch mode { + case "yolo": + warnYolo() + return Allow + case "allowlist": + if matchAny(allowed, action) { + return Allow + } + return Deny + default: // "default", "", or any unrecognized value + if matchAny(allowed, action) { + return Allow + } + return Confirm + } +} + +// warnYolo prints the YOLO warning to stderr the first time it is called in +// this process, and does nothing on every subsequent call. +func warnYolo() { + yoloWarnOnce.Do(func() { + fmt.Fprintln(os.Stderr, "permissions: YOLO mode active — skipping all confirmations") + }) +} + +// matchAny reports whether action matches any pattern in allowed. +func matchAny(allowed []string, action string) bool { + for _, pattern := range allowed { + if matchPattern(pattern, action) { + return true + } + } + return false +} + +// matchPattern reports whether action matches pattern. A pattern matches +// either exactly, or — if it ends in "*" — as a prefix, where everything +// before the trailing "*" must prefix action. A bare "*" has an empty +// prefix and therefore matches every action; an empty pattern matches +// nothing. +func matchPattern(pattern, action string) bool { + if pattern == "" { + return false + } + if pattern == action { + return true + } + if strings.HasSuffix(pattern, "*") { + prefix := strings.TrimSuffix(pattern, "*") + return strings.HasPrefix(action, prefix) + } + return false +} diff --git a/internal/permissions/permissions_test.go b/internal/permissions/permissions_test.go new file mode 100644 index 0000000..8e359bf --- /dev/null +++ b/internal/permissions/permissions_test.go @@ -0,0 +1,235 @@ +package permissions + +import ( + "bytes" + "fmt" + "io" + "os" + "strings" + "sync" + "testing" +) + +// captureStderr redirects os.Stderr to a pipe for the duration of f and +// returns everything written to it. Used to observe (or rule out) the YOLO +// warning without depending on go test's own stderr capture behavior. +func captureStderr(f func()) string { + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + panic(err) + } + os.Stderr = w + f() + w.Close() + os.Stderr = old + + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + panic(err) + } + r.Close() + return buf.String() +} + +// captureStdout is captureStderr's stdout counterpart, used to confirm +// ConfirmInteractive prints nothing when it short-circuits. +func captureStdout(f func()) string { + old := os.Stdout + r, w, err := os.Pipe() + if err != nil { + panic(err) + } + os.Stdout = w + f() + w.Close() + os.Stdout = old + + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + panic(err) + } + r.Close() + return buf.String() +} + +func TestCheck(t *testing.T) { + tests := []struct { + name string + mode string + allowed []string + action string + want Decision + }{ + // --- default mode --- + {"default/unmatched, no allowed -> confirm", "default", nil, "code.undo", Confirm}, + {"default/exact match -> allow", "default", []string{"code.undo"}, "code.undo", Allow}, + {"default/exact mismatch -> confirm", "default", []string{"code.undo"}, "code.redo", Confirm}, + {"default/glob prefix match (scaffold) -> allow", "default", []string{"craft.*"}, "craft.scaffold", Allow}, + {"default/glob prefix match (go-service) -> allow", "default", []string{"craft.*"}, "craft.go-service", Allow}, + {"default/glob prefix mismatch -> confirm", "default", []string{"craft.*"}, "plugin.install", Confirm}, + {"default/bare star matches everything -> allow", "default", []string{"*"}, "plugin.install", Allow}, + + // --- allowlist mode --- + {"allowlist/unmatched, no allowed -> deny", "allowlist", nil, "code.undo", Deny}, + {"allowlist/exact match -> allow", "allowlist", []string{"code.undo"}, "code.undo", Allow}, + {"allowlist/exact mismatch -> deny", "allowlist", []string{"code.undo"}, "code.redo", Deny}, + {"allowlist/glob prefix match (scaffold) -> allow", "allowlist", []string{"craft.*"}, "craft.scaffold", Allow}, + {"allowlist/glob prefix match (go-service) -> allow", "allowlist", []string{"craft.*"}, "craft.go-service", Allow}, + {"allowlist/glob prefix mismatch -> deny", "allowlist", []string{"craft.*"}, "plugin.install", Deny}, + {"allowlist/bare star matches everything -> allow", "allowlist", []string{"*"}, "plugin.install", Allow}, + {"allowlist/second pattern in list matches -> allow", "allowlist", []string{"plugin.install", "craft.*"}, "craft.scaffold", Allow}, + {"allowlist/no pattern in list matches -> deny", "allowlist", []string{"plugin.install", "craft.*"}, "code.undo", Deny}, + + // --- yolo mode: always Allow, allowed is irrelevant --- + {"yolo/allow with no allowed list", "yolo", nil, "plugin.install", Allow}, + {"yolo/allow even when action not in allowed", "yolo", []string{"code.undo"}, "plugin.install", Allow}, + + // --- unknown / unset mode falls back to default semantics --- + {"empty mode behaves as default: matched -> allow", "", []string{"code.undo"}, "code.undo", Allow}, + {"empty mode behaves as default: unmatched -> confirm", "", nil, "code.undo", Confirm}, + {"unrecognized mode behaves as default: matched -> allow", "banana", []string{"code.undo"}, "code.undo", Allow}, + {"unrecognized mode behaves as default: unmatched -> confirm", "banana", nil, "code.undo", Confirm}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Route stderr to a discard pipe for yolo cases so the warning + // (if the once hasn't already fired process-wide) doesn't spam + // test output; correctness of the warning itself is covered by + // TestCheck_YoloWarnsOnlyOnce below. + var got Decision + captureStderr(func() { + got = Check(tt.mode, tt.allowed, tt.action) + }) + if got != tt.want { + t.Errorf("Check(%q, %v, %q) = %s, want %s", tt.mode, tt.allowed, tt.action, got, tt.want) + } + }) + } +} + +func TestCheck_YoloWarnsOnlyOnce(t *testing.T) { + // Reset the package-level guard so this test observes a clean first + // call, independent of whatever earlier subtests in this process (e.g. + // TestCheck's yolo cases) already exercised yolo mode. + yoloWarnOnce = sync.Once{} + + const wantMsg = "permissions: YOLO mode active — skipping all confirmations" + + first := captureStderr(func() { + Check("yolo", nil, "plugin.install") + }) + if !strings.Contains(first, wantMsg) { + t.Fatalf("first yolo Check() stderr = %q, want it to contain %q", first, wantMsg) + } + + second := captureStderr(func() { + Check("yolo", []string{"anything"}, "some.other.action") + }) + if second != "" { + t.Fatalf("second yolo Check() in the same process wrote %q to stderr, want silence (warn-once)", second) + } +} + +func TestMatchPattern(t *testing.T) { + tests := []struct { + pattern string + action string + want bool + }{ + {"code.undo", "code.undo", true}, + {"code.undo", "code.redo", false}, + {"*", "anything.at.all", true}, + {"*", "", true}, + {"craft.*", "craft.scaffold", true}, + {"craft.*", "craft.go-service", true}, + {"craft.*", "craftx.scaffold", false}, // no dot before the differing char: must not match + {"craft.*", "plugin.install", false}, + {"craft.*", "craft", false}, // missing the separator dot entirely + {"", "anything", false}, + {"", "", false}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%q~%q", tt.pattern, tt.action), func(t *testing.T) { + got := matchPattern(tt.pattern, tt.action) + if got != tt.want { + t.Errorf("matchPattern(%q, %q) = %v, want %v", tt.pattern, tt.action, got, tt.want) + } + }) + } +} + +func TestExplain(t *testing.T) { + tests := []struct { + action string + want string + }{ + { + action: "plugin.install", + want: `permission denied: "plugin.install" (add to permissions.allowed in ~/.dojo/settings.json, or run with --yolo)`, + }, + { + action: "code.undo", + want: `permission denied: "code.undo" (add to permissions.allowed in ~/.dojo/settings.json, or run with --yolo)`, + }, + } + for _, tt := range tests { + t.Run(tt.action, func(t *testing.T) { + if got := Explain(tt.action); got != tt.want { + t.Errorf("Explain(%q) = %q, want %q", tt.action, got, tt.want) + } + }) + } +} + +// withNonTTYStdin swaps os.Stdin for an os.Pipe read-end (never a terminal) +// for the duration of f, writing body to the pipe first (or closing it +// immediately if body is empty) so any accidental read has deterministic +// content to observe. +func withNonTTYStdin(t *testing.T, body string, f func()) { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + if body != "" { + if _, err := w.WriteString(body); err != nil { + t.Fatalf("write to pipe: %v", err) + } + } + w.Close() + + old := os.Stdin + os.Stdin = r + defer func() { + os.Stdin = old + r.Close() + }() + + f() +} + +func TestConfirmInteractive_NonTTYShortCircuits(t *testing.T) { + // A "y" answer sits ready in the pipe. If ConfirmInteractive's TTY check + // were broken (e.g. it read stdin unconditionally), this would read as + // an affirmative answer and the test would catch the regression as a + // false positive (true) instead of the required false. + withNonTTYStdin(t, "y\n", func() { + got := ConfirmInteractive("plugin.install", "installs a third-party plugin") + if got { + t.Errorf("ConfirmInteractive() with non-TTY stdin = true, want false even though the pipe held an affirmative answer") + } + }) +} + +func TestConfirmInteractive_NonTTYPrintsNoPrompt(t *testing.T) { + withNonTTYStdin(t, "", func() { + out := captureStdout(func() { + ConfirmInteractive("plugin.install", "installs a third-party plugin") + }) + if out != "" { + t.Errorf("ConfirmInteractive() with non-TTY stdin printed %q, want no prompt at all", out) + } + }) +} diff --git a/internal/plugins/scanner.go b/internal/plugins/scanner.go index 1a60250..1c42450 100644 --- a/internal/plugins/scanner.go +++ b/internal/plugins/scanner.go @@ -26,6 +26,16 @@ type HookRule struct { Matcher string If string Hooks []HookDef + // Blocking marks this rule as requiring synchronous completion (as + // opposed to the per-HookDef Async flag, which controls fire-and-forget + // vs. block-until-done for one individual hook action within the rule). + // When true, a command-type hook in this rule that fails or exits non-zero + // vetoes the caller's action: hooks.Runner.FireChecked returns a Blocked + // result and the REPL aborts the command (PreCommand) or the send + // (UserPromptSubmit). Only command hooks can block — http is fire-and-forget + // and prompt/agent are no-ops. Populated from hooks.json's "blocking" key by + // loadHooks below; consumed by FireChecked in internal/hooks/runner.go. + Blocking bool `json:"blocking,omitempty"` } // HookDef is an individual hook action within a rule. @@ -47,9 +57,10 @@ type pluginMeta struct { // hookEntry is one element inside an event's array in hooks.json. type hookEntry struct { - Matcher string `json:"matcher"` - If string `json:"if"` - Hooks []HookDef `json:"hooks"` + Matcher string `json:"matcher"` + If string `json:"if"` + Blocking bool `json:"blocking,omitempty"` + Hooks []HookDef `json:"hooks"` } // Scan reads a plugins root directory and returns all discovered plugins. @@ -203,10 +214,11 @@ func loadHooks(pluginDir, pluginName string) []HookRule { } for _, entry := range entries { rules = append(rules, HookRule{ - Event: ruleEvent, - Matcher: entry.Matcher, - If: entry.If, - Hooks: entry.Hooks, + Event: ruleEvent, + Matcher: entry.Matcher, + If: entry.If, + Blocking: entry.Blocking, + Hooks: entry.Hooks, }) } } diff --git a/internal/plugins/scanner_blocking_test.go b/internal/plugins/scanner_blocking_test.go new file mode 100644 index 0000000..5d670b7 --- /dev/null +++ b/internal/plugins/scanner_blocking_test.go @@ -0,0 +1,103 @@ +package plugins + +import ( + "path/filepath" + "testing" +) + +// TestLoadHooks_PopulatesBlockingFromJSON proves the "blocking" key in a +// hooks.json rule is parsed into HookRule.Blocking, and that its absence +// defaults to false. This is the wiring the runner's FireChecked relies on to +// know which rules may veto a command. +func TestLoadHooks_PopulatesBlockingFromJSON(t *testing.T) { + root := t.TempDir() + pluginDir := filepath.Join(root, "gatekeeper") + mustMkdir(t, pluginDir) + writeJSON(t, filepath.Join(pluginDir, "plugin.json"), map[string]any{ + "name": "gatekeeper", + "version": "1.0", + }) + mustMkdir(t, filepath.Join(pluginDir, "hooks")) + // Two rules on the same event: one blocking, one not (blocking key omitted). + writeJSON(t, filepath.Join(pluginDir, "hooks", "hooks.json"), map[string]any{ + "PreCommand": []map[string]any{ + { + "matcher": "deploy*", + "blocking": true, + "hooks": []map[string]any{{"type": "command", "command": "guard.sh"}}, + }, + { + "matcher": "*", + "hooks": []map[string]any{{"type": "command", "command": "log.sh"}}, + }, + }, + }) + + plugins, err := Scan(root) + if err != nil { + t.Fatalf("Scan() returned error: %v", err) + } + if len(plugins) != 1 { + t.Fatalf("expected 1 plugin, got %d", len(plugins)) + } + + var blocking, nonBlocking *HookRule + for i := range plugins[0].HookRules { + r := &plugins[0].HookRules[i] + switch r.Matcher { + case "deploy*": + blocking = r + case "*": + nonBlocking = r + } + } + if blocking == nil || nonBlocking == nil { + t.Fatalf("expected both rules to load; got %+v", plugins[0].HookRules) + } + if !blocking.Blocking { + t.Error(`rule with "blocking": true should have Blocking == true`) + } + if nonBlocking.Blocking { + t.Error("rule with no blocking key should default to Blocking == false") + } +} + +// TestLoadHooks_BlockingUnderWrappedSchema proves the blocking key is also read +// from the Claude-Code wrapper schema ({"hooks": {"<Event>": [...]}}), not only +// the flat dojo-native schema. +func TestLoadHooks_BlockingUnderWrappedSchema(t *testing.T) { + root := t.TempDir() + pluginDir := filepath.Join(root, "wrapped") + mustMkdir(t, pluginDir) + writeJSON(t, filepath.Join(pluginDir, "plugin.json"), map[string]any{ + "name": "wrapped", + "version": "1.0", + }) + mustMkdir(t, filepath.Join(pluginDir, "hooks")) + writeJSON(t, filepath.Join(pluginDir, "hooks", "hooks.json"), map[string]any{ + "hooks": map[string]any{ + "PreToolUse": []map[string]any{ + { + "matcher": "*", + "blocking": true, + "hooks": []map[string]any{{"type": "command", "command": "guard.sh"}}, + }, + }, + }, + }) + + plugins, err := Scan(root) + if err != nil { + t.Fatalf("Scan() returned error: %v", err) + } + if len(plugins) != 1 || len(plugins[0].HookRules) != 1 { + t.Fatalf("expected 1 plugin with 1 rule, got %+v", plugins) + } + rule := plugins[0].HookRules[0] + if rule.Event != "PreCommand" { // PreToolUse → PreCommand + t.Errorf("wrapped event = %q, want PreCommand", rule.Event) + } + if !rule.Blocking { + t.Error("blocking key under the wrapped schema should populate Blocking") + } +} diff --git a/internal/repl/renderer.go b/internal/repl/renderer.go index ef9bb26..9348431 100644 --- a/internal/repl/renderer.go +++ b/internal/repl/renderer.go @@ -8,6 +8,8 @@ import ( "strings" "github.com/DojoGenesis/cli/internal/client" + "github.com/DojoGenesis/cli/internal/config" + "github.com/DojoGenesis/cli/internal/guardrail" gcolor "github.com/gookit/color" ) @@ -213,6 +215,118 @@ func (re RenderEvent) Render(plain bool) string { return re.Content } +// ─── stream guardrail (advisory repeated-tool-failure watch) ───────────────── + +// streamGuardWindow is how many leading bytes of a tool result are examined, +// both for the failure heuristic and for the signature handed to +// guardrail.Signature (which further truncates to 80 bytes). +const streamGuardWindow = 200 + +// failureMarkers are the case-insensitive substrings that mark a tool result +// as failing when any appears within the first streamGuardWindow bytes. +// Deliberately conservative and advisory-only: a missed failure costs nothing, +// while over-triggering would train the user to ignore the guardrail. +var failureMarkers = []string{"error", "failed", "exception"} + +// StreamGuard watches classified agent-stream events for repeated identical +// tool failures and escalates advisory notices through a guardrail.Tracker +// (internal/guardrail — warn at WarnAfter, hard at/after HardAfter, per +// cfg.Guardrails). It never mutates, drops, or reorders the original events: +// Observe only ever returns an ADDITIONAL synthetic warning for the caller to +// render through the normal EventWarning path. +// +// Scope choice: chunk classification here is stateless free functions with no +// renderer instance to hang state on, so the guard lives one-per-REPL (a +// field set in repl.New) — one interactive session lifetime, letting failure +// streaks span turns the way the Registry's command guard does. It is driven +// only from the single ChatStream callback goroutine, so the small +// bookkeeping fields need no lock (the Tracker inside is mutex-guarded +// anyway). +type StreamGuard struct { + tracker *guardrail.Tracker + + // pendingTool is the tool name from the most recent EventToolCall; the + // next EventToolResult is attributed to it. Results follow their calls on + // this stream and the SSE protocol carries no call-id on results, so this + // simple last-call pairing is the best available attribution. + pendingTool string + + // lastFailSig remembers, per tool, the signature of its most recent + // failure. Tracker keys embed the failure signature, so a SUCCESSFUL + // result cannot rebuild the failing key from its own (healthy) content — + // instead the reset replays the remembered key with failed=false, then + // forgets it. A success for a tool with nothing remembered is a no-op. + lastFailSig map[string]string +} + +// NewStreamGuard builds a StreamGuard from cfg.Guardrails; a nil cfg is +// treated as disabled (the Tracker short-circuits every Record call). +func NewStreamGuard(cfg *config.Config) *StreamGuard { + enabled := false + warnAfter, hardAfter := 0, 0 + if cfg != nil { + enabled = cfg.Guardrails.Enabled + warnAfter = cfg.Guardrails.WarnAfter + hardAfter = cfg.Guardrails.HardAfter + } + return &StreamGuard{ + tracker: guardrail.New(warnAfter, hardAfter, enabled), + lastFailSig: make(map[string]string), + } +} + +// Observe feeds one classified event through the guard. When the tracker +// escalates it returns (synthetic EventWarning event, true); otherwise +// (zero RenderEvent, false). The input event is never modified. A nil +// receiver is inert, so tests that build bare REPL literals stay safe. +func (g *StreamGuard) Observe(ev RenderEvent) (RenderEvent, bool) { + if g == nil || g.tracker == nil { + return RenderEvent{}, false + } + switch ev.Type { + case EventToolCall: + name := ev.Meta["tool"] + if name == "" { + name = "unknown" + } + g.pendingTool = name + + case EventToolResult: + tool := g.pendingTool + if tool == "" { + tool = "unknown" + } + window := ev.Content + if len(window) > streamGuardWindow { + window = window[:streamGuardWindow] + } + if looksFailing(window) { + sig := guardrail.Signature(window) + g.lastFailSig[tool] = sig + adv := g.tracker.Record("tool:"+tool+"|"+sig, true) + if adv.Level != guardrail.None { + return RenderEvent{Type: EventWarning, Content: adv.Msg}, true + } + } else if sig, ok := g.lastFailSig[tool]; ok { + g.tracker.Record("tool:"+tool+"|"+sig, false) + delete(g.lastFailSig, tool) + } + } + return RenderEvent{}, false +} + +// looksFailing reports whether the already window-truncated tool-result +// prefix carries any failureMarkers substring, case-insensitively. +func looksFailing(window string) bool { + lower := strings.ToLower(window) + for _, m := range failureMarkers { + if strings.Contains(lower, m) { + return true + } + } + return false +} + // ─── internal content extraction ───────────────────────────────────────────── // extractContentFromData pulls readable text from a raw SSE data string. diff --git a/internal/repl/renderer_guardrail_test.go b/internal/repl/renderer_guardrail_test.go new file mode 100644 index 0000000..3a0da15 --- /dev/null +++ b/internal/repl/renderer_guardrail_test.go @@ -0,0 +1,238 @@ +package repl + +// renderer_guardrail_test.go — tests for StreamGuard (renderer.go), the +// advisory repeated-tool-failure watch over classified agent-stream events. +// NEW file per the advancement-wave ownership rules; renderer_test.go and the +// other existing test files are not modified. + +import ( + "fmt" + "reflect" + "strings" + "testing" + + "github.com/DojoGenesis/cli/internal/client" + "github.com/DojoGenesis/cli/internal/config" + "github.com/DojoGenesis/cli/internal/guardrail" +) + +func guardCfg(warnAfter, hardAfter int, enabled bool) *config.Config { + return &config.Config{ + Guardrails: config.GuardrailsConfig{Enabled: enabled, WarnAfter: warnAfter, HardAfter: hardAfter}, + } +} + +// toolCallEv classifies a synthetic tool_call chunk for the named tool. +func toolCallEv(tool string) RenderEvent { + return ClassifyChunk(client.SSEChunk{Event: "tool_call", Data: fmt.Sprintf(`{"name":%q}`, tool)}) +} + +// toolResultEv classifies a synthetic tool_result chunk carrying raw content. +func toolResultEv(content string) RenderEvent { + return ClassifyChunk(client.SSEChunk{Event: "tool_result", Data: content}) +} + +func streamWarnMsg(key string, n int) string { + return fmt.Sprintf("guardrail: %s has failed %d times consecutively with the same signature — consider a different approach", key, n) +} + +func streamHardMsg(key string, n int) string { + return fmt.Sprintf("guardrail: %s is stuck (%d consecutive identical failures) — stop and change strategy, or check config vs code (see debugging gate)", key, n) +} + +// TestStreamGuard_WarnAndHardThresholds feeds repeated failing ToolResults +// with an identical signature and asserts the advisory Warning appears +// exactly at WarnAfter, then at and after HardAfter — and that the original +// events pass through Observe untouched. +func TestStreamGuard_WarnAndHardThresholds(t *testing.T) { + g := NewStreamGuard(guardCfg(3, 5, true)) + + failure := "Error: connection refused" + key := "tool:web_fetch|" + guardrail.Signature(failure) + + // The tool_call itself must never produce advice. + call := toolCallEv("web_fetch") + if _, ok := g.Observe(call); ok { + t.Fatal("Observe(tool_call) produced advice; want none") + } + + wants := []struct { + name string + wantMsg string // "" = no advice + wantLvl EventType + }{ + {"failure 1", "", EventEmpty}, + {"failure 2", "", EventEmpty}, + {"failure 3: warn at WarnAfter", streamWarnMsg(key, 3), EventWarning}, + {"failure 4: between", "", EventEmpty}, + {"failure 5: hard at HardAfter", streamHardMsg(key, 5), EventWarning}, + {"failure 6: hard keeps firing", streamHardMsg(key, 6), EventWarning}, + } + for _, step := range wants { + ev := toolResultEv(failure) + orig := RenderEvent{Type: ev.Type, Content: ev.Content, Meta: ev.Meta} + adv, ok := g.Observe(ev) + if !reflect.DeepEqual(ev, orig) { + t.Fatalf("%s: Observe mutated the input event: %+v -> %+v", step.name, orig, ev) + } + if step.wantMsg == "" { + if ok { + t.Fatalf("%s: unexpected advice %+v", step.name, adv) + } + continue + } + if !ok { + t.Fatalf("%s: no advice; want %q", step.name, step.wantMsg) + } + if adv.Type != EventWarning { + t.Fatalf("%s: advice type = %s; want warning (must reuse the Warning rendering path)", step.name, adv.Type) + } + if adv.Content != step.wantMsg { + t.Fatalf("%s: advice = %q; want %q", step.name, adv.Content, step.wantMsg) + } + // The advisory line renders through the existing Warning path. + if plain := adv.Render(true); plain != "[warning] "+step.wantMsg { + t.Fatalf("%s: plain render = %q; want %q", step.name, plain, "[warning] "+step.wantMsg) + } + } +} + +// TestStreamGuard_ResetOnSuccess verifies a healthy-looking ToolResult for a +// tool resets that tool's remembered failing streak (via the last-failure +// signature bookkeeping), and that a success with no tracked failure is a +// harmless no-op. +func TestStreamGuard_ResetOnSuccess(t *testing.T) { + g := NewStreamGuard(guardCfg(2, 4, true)) + + // Success before any failure: nothing tracked, nothing fires, no panic. + g.Observe(toolCallEv("web_fetch")) + if _, ok := g.Observe(toolResultEv("fetched 200 OK")); ok { + t.Fatal("success with no tracked failure produced advice") + } + + failure := "Error: connection refused" + key := "tool:web_fetch|" + guardrail.Signature(failure) + + g.Observe(toolResultEv(failure)) // streak 1 + if adv, ok := g.Observe(toolResultEv(failure)); !ok || adv.Content != streamWarnMsg(key, 2) { + t.Fatalf("2nd failure: advice = %+v ok=%v; want warn %q", adv, ok, streamWarnMsg(key, 2)) + } + + // Success resets the streak under the remembered failing key. + if _, ok := g.Observe(toolResultEv("fetched 200 OK")); ok { + t.Fatal("resetting success produced advice") + } + + // After reset the streak restarts: 1 then warn again at 2. + if _, ok := g.Observe(toolResultEv(failure)); ok { + t.Fatal("1st failure after reset produced advice; streak did not reset") + } + if adv, ok := g.Observe(toolResultEv(failure)); !ok || adv.Content != streamWarnMsg(key, 2) { + t.Fatalf("2nd failure after reset: advice = %+v ok=%v; want warn %q", adv, ok, streamWarnMsg(key, 2)) + } +} + +// TestStreamGuard_FailureHeuristic tables the conservative failure detector: +// case-insensitive "error"/"failed"/"exception" within the first 200 bytes. +func TestStreamGuard_FailureHeuristic(t *testing.T) { + tests := []struct { + name string + content string + failing bool + }{ + {"lowercase error", "error: no route to host", true}, + {"capitalized Error", "Error: connection refused", true}, + {"uppercase FAILED", "FAILED to connect after 3 attempts", true}, + {"mixed-case Exception", "Unhandled Exception in worker", true}, + {"healthy result", "fetched 3 documents, 200 OK", false}, + {"marker beyond the 200-byte window", strings.Repeat("a", 200) + " error", false}, + {"marker inside the 200-byte window", strings.Repeat("a", 190) + " error", true}, + {"empty result", "", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Fresh guard with warnAfter=1 so a single detected failure fires + // Warn immediately — turning the detector itself into the assert. + g := NewStreamGuard(guardCfg(1, 3, true)) + g.Observe(toolCallEv("web_fetch")) + _, ok := g.Observe(toolResultEv(tc.content)) + if ok != tc.failing { + t.Fatalf("content %q: detected failing = %v; want %v", tc.content, ok, tc.failing) + } + }) + } +} + +// TestStreamGuard_ToolAttribution verifies keys carry the tool name from the +// preceding ToolCall, different tools track independently, and a result with +// no preceding call falls back to "unknown". +func TestStreamGuard_ToolAttribution(t *testing.T) { + failure := "error: boom" + + // No preceding tool_call → attributed to "unknown". + g := NewStreamGuard(guardCfg(1, 3, true)) + adv, ok := g.Observe(toolResultEv(failure)) + wantKey := "tool:unknown|" + guardrail.Signature(failure) + if !ok || adv.Content != streamWarnMsg(wantKey, 1) { + t.Fatalf("orphan result: advice = %+v ok=%v; want warn %q", adv, ok, streamWarnMsg(wantKey, 1)) + } + + // Two tools with identical error text keep independent streaks. + g2 := NewStreamGuard(guardCfg(2, 4, true)) + g2.Observe(toolCallEv("alpha")) + g2.Observe(toolResultEv(failure)) // alpha streak 1 + g2.Observe(toolCallEv("beta")) + if _, ok := g2.Observe(toolResultEv(failure)); ok { // beta streak 1 + t.Fatal("beta's first failure fired advice; tools must track independently") + } + g2.Observe(toolCallEv("alpha")) + adv2, ok2 := g2.Observe(toolResultEv(failure)) // alpha streak 2 → warn + alphaKey := "tool:alpha|" + guardrail.Signature(failure) + if !ok2 || adv2.Content != streamWarnMsg(alphaKey, 2) { + t.Fatalf("alpha 2nd failure: advice = %+v ok=%v; want warn %q", adv2, ok2, streamWarnMsg(alphaKey, 2)) + } +} + +// TestStreamGuard_DisabledNilCfgNilReceiver verifies every inert mode: config +// disabled, nil config, and nil *StreamGuard receiver (bare REPL literals in +// existing tests have a nil streamGuard field). +func TestStreamGuard_DisabledNilCfgNilReceiver(t *testing.T) { + failure := "error: boom" + + for _, tc := range []struct { + name string + g *StreamGuard + }{ + {"disabled via config", NewStreamGuard(guardCfg(1, 2, false))}, + {"nil config", NewStreamGuard(nil)}, + {"nil receiver", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + tc.g.Observe(toolCallEv("web_fetch")) + for i := 0; i < 5; i++ { + if adv, ok := tc.g.Observe(toolResultEv(failure)); ok { + t.Fatalf("inert guard produced advice %+v", adv) + } + } + }) + } +} + +// TestStreamGuard_NonToolEventsIgnored verifies text/thinking/warning/done +// events flow past the guard without advice or state changes. +func TestStreamGuard_NonToolEventsIgnored(t *testing.T) { + g := NewStreamGuard(guardCfg(1, 3, true)) + events := []RenderEvent{ + {Type: EventText, Content: "error in your code is likely"}, // text mentioning "error" is NOT a tool failure + {Type: EventThinking, Content: "thinking..."}, + {Type: EventWarning, Content: "some upstream warning"}, + {Type: EventError, Content: "gateway exploded"}, + {Type: EventDone}, + {Type: EventEmpty}, + } + for _, ev := range events { + if adv, ok := g.Observe(ev); ok { + t.Fatalf("non-tool event %s produced advice %+v", ev.Type, adv) + } + } +} diff --git a/internal/repl/repl.go b/internal/repl/repl.go index 90cc14f..c172f21 100644 --- a/internal/repl/repl.go +++ b/internal/repl/repl.go @@ -65,6 +65,11 @@ type REPL struct { lastErrSig string errRepeat int + // streamGuard watches classified stream events for repeated identical + // tool failures (advisory only — see StreamGuard in renderer.go). + // nil-safe: Observe on a nil guard is inert. + streamGuard *StreamGuard + mu sync.Mutex // guards turnCancel turnCancel context.CancelFunc // cancels the in-flight streaming turn; nil when idle } @@ -119,7 +124,8 @@ func New(cfg *config.Config, gw *client.Client, resume bool, plain bool) *REPL { plain: plain, // Resolve the protocol context once at session start (project ./DOJO.md // > ~/.dojo/DOJO.md > embedded default; empty when disabled). - protocol: protocol.NewInjector(cfg), + protocol: protocol.NewInjector(cfg), + streamGuard: NewStreamGuard(cfg), } if resume { @@ -477,8 +483,17 @@ func (r *REPL) handle(ctx context.Context, line string) error { if strings.HasPrefix(line, "/") { payload := map[string]any{"command": line} - if err := r.runner.Fire(ctx, hooks.EventPreCommand, payload); err != nil { - log.Printf("[hooks] PreCommand error: %v", err) + // PreCommand is the one place a hook can veto a command. A Blocking:true + // command hook that fails aborts BEFORE dispatch, naming the offending + // plugin+hook; a non-blocking failure logs and continues (the command + // still runs) — the pre-Blocking behavior. + res := r.runner.FireChecked(ctx, hooks.EventPreCommand, payload) + if res.Blocked { + r.printHookBlock(res) + return nil + } + if res.Err != nil { + log.Printf("[hooks] PreCommand error: %v", res.Err) } cmdErr := r.registry.Dispatch(ctx, line[1:]) @@ -591,6 +606,14 @@ func (r *REPL) handle(ctx context.Context, line string) error { // chat sends a freeform message to the gateway and streams the response. func (r *REPL) chat(ctx context.Context, message string) error { + // UserPromptSubmit fires before anything leaves for the gateway. A + // Blocking:true command hook that fails vetoes the send — return to the + // prompt without contacting the gateway (the REPL stays alive); non-blocking + // failures log and continue. + if r.fireUserPromptSubmit(ctx, message) { + return nil + } + workspaceRoot, _ := os.Getwd() req := client.ChatRequest{ Message: message, @@ -636,6 +659,11 @@ func (r *REPL) chat(ctx context.Context, message string) error { fmt.Print(rendered) fullText.WriteString(ev.Content) } + // Advisory tool-loop guardrail: purely additive — the original event + // was already rendered above, untouched (see StreamGuard in renderer.go). + if adv, ok := r.streamGuard.Observe(ev); ok { + fmt.Printf("\n%s\n", adv.Render(r.plain)) + } // Per-turn cost/token readout: opportunistically check every chunk for // a usage payload (see extractUsage) rather than gating on a specific // event name — in practice the gateway, if it sends usage at all, only @@ -720,6 +748,45 @@ func (r *REPL) chat(ctx context.Context, message string) error { return nil } +// ─── Hook blocking (PreCommand veto + UserPromptSubmit veto) ────────────────── + +// printHookBlock prints the standard one-line "[hooks] blocked by …" message +// when a Blocking hook vetoes a command or a chat send. This is critical user +// feedback, not decoration, so it prints in plain mode too — just without the +// red styling. +func (r *REPL) printHookBlock(res hooks.FireResult) { + if r.plain { + fmt.Printf(" %s\n", res.Message()) + return + } + fmt.Println(gcolor.HEX("#ef4444").Sprintf(" %s", res.Message())) +} + +// fireUserPromptSubmit fires the UserPromptSubmit hooks for a free-text chat +// message just before it is sent to the gateway, and reports whether a Blocking +// rule vetoed the send. On a veto it prints the block line and returns true — +// chat() then returns to the prompt without sending, leaving the REPL running. +// A non-blocking hook failure is logged and returns false (the send proceeds). +// +// This event has no command name, so its rules are matched against the literal +// "chat" (matcherMatches reads payload["command"]); the prompt text rides in +// payload["prompt"], which the runner exposes to command hooks as $DOJO_PROMPT +// (never interpolated into the command — see hooks.runCommand). +func (r *REPL) fireUserPromptSubmit(ctx context.Context, message string) (blocked bool) { + res := r.runner.FireChecked(ctx, hooks.EventUserPromptSubmit, map[string]any{ + "command": "chat", + "prompt": message, + }) + if res.Blocked { + r.printHookBlock(res) + return true + } + if res.Err != nil { + log.Printf("[hooks] UserPromptSubmit error: %v", res.Err) + } + return false +} + // ─── JIT tell-triggered protocol nudge ──────────────────────────────────────── // maybeProtocolNudge surfaces the single most relevant protocol gate as a dim diff --git a/internal/repl/repl_hooks_test.go b/internal/repl/repl_hooks_test.go new file mode 100644 index 0000000..ded22c2 --- /dev/null +++ b/internal/repl/repl_hooks_test.go @@ -0,0 +1,122 @@ +package repl + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/DojoGenesis/cli/internal/hooks" + "github.com/DojoGenesis/cli/internal/plugins" +) + +// fireUserPromptSubmit is the exact seam chat() uses before sending a free-text +// message to the gateway. Testing it directly — with a bare REPL that has only +// the runner set (no gateway, no ~/.dojo state) — covers the block/continue +// decision and the DOJO_PROMPT env delivery end-to-end through repl → runner → +// exec, hermetically. chat() itself is not unit-tested here for the same reason +// Run() isn't (see repl_test.go): it owns a real gateway stream. The blocking +// decision it depends on lives entirely in this seam. + +// TestFireUserPromptSubmit_BlockingHookVetoesSend proves a failing Blocking +// UserPromptSubmit hook vetoes the send (returns true → chat() returns to the +// prompt without contacting the gateway). +func TestFireUserPromptSubmit_BlockingHookVetoesSend(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "prompt-gate", + Path: t.TempDir(), + HookRules: []plugins.HookRule{{ + Event: hooks.EventUserPromptSubmit, + Blocking: true, + Matcher: "chat", + Hooks: []plugins.HookDef{{Type: "command", Command: "exit 1"}}, + }}, + }} + r := &REPL{runner: hooks.New(ps)} + + if !r.fireUserPromptSubmit(context.Background(), "some message") { + t.Error("a failing Blocking UserPromptSubmit hook must veto the send (return true)") + } +} + +// TestFireUserPromptSubmit_NonBlockingFailureAllowsSend proves a failing +// non-Blocking hook logs and continues (returns false → the send proceeds). +func TestFireUserPromptSubmit_NonBlockingFailureAllowsSend(t *testing.T) { + ps := []plugins.Plugin{{ + Name: "prompt-warn", + Path: t.TempDir(), + HookRules: []plugins.HookRule{{ + Event: hooks.EventUserPromptSubmit, + Matcher: "chat", + Hooks: []plugins.HookDef{{Type: "command", Command: "exit 1"}}, // non-blocking + }}, + }} + r := &REPL{runner: hooks.New(ps)} + + if r.fireUserPromptSubmit(context.Background(), "hello") { + t.Error("a non-blocking failure must NOT veto the send (return false)") + } +} + +// TestFireUserPromptSubmit_DeliversPromptToHookEnv proves the user's message +// reaches a command hook via $DOJO_PROMPT through the repl seam. +func TestFireUserPromptSubmit_DeliversPromptToHookEnv(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "prompt.txt") + const msg = "the user's words" + ps := []plugins.Plugin{{ + Name: "prompt-capture", + Path: tmp, + HookRules: []plugins.HookRule{{ + Event: hooks.EventUserPromptSubmit, + Matcher: "chat", + Hooks: []plugins.HookDef{{Type: "command", Command: `printf '%s' "$DOJO_PROMPT" > ` + marker}}, + }}, + }} + r := &REPL{runner: hooks.New(ps)} + + if r.fireUserPromptSubmit(context.Background(), msg) { + t.Fatal("a clean hook should not veto the send") + } + got, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("reading marker: %v", err) + } + if string(got) != msg { + t.Errorf("hook saw DOJO_PROMPT=%q, want %q", got, msg) + } +} + +// TestFireUserPromptSubmit_NoHooks_AllowsSend proves the common case (no +// UserPromptSubmit hooks defined) is a silent pass-through. +func TestFireUserPromptSubmit_NoHooks_AllowsSend(t *testing.T) { + r := &REPL{runner: hooks.New(nil)} + if r.fireUserPromptSubmit(context.Background(), "hi") { + t.Error("with no hooks the send must proceed (return false)") + } +} + +// TestFireUserPromptSubmit_NonChatMatcherDoesNotFire proves the event matches +// rules against the literal "chat": a rule scoped to a different matcher must +// not fire on a chat prompt. +func TestFireUserPromptSubmit_NonChatMatcherDoesNotFire(t *testing.T) { + tmp := t.TempDir() + marker := filepath.Join(tmp, "should-not-exist.txt") + ps := []plugins.Plugin{{ + Name: "scoped", + Path: tmp, + HookRules: []plugins.HookRule{{ + Event: hooks.EventUserPromptSubmit, + Matcher: "deploy*", // does not match "chat" + Hooks: []plugins.HookDef{{Type: "command", Command: "touch " + marker}}, + }}, + }} + r := &REPL{runner: hooks.New(ps)} + + if r.fireUserPromptSubmit(context.Background(), "hello") { + t.Fatal("a deploy*-scoped rule should not veto") + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Error("a rule matched to deploy* must not fire on the chat prompt event") + } +} diff --git a/internal/skills/external.go b/internal/skills/external.go new file mode 100644 index 0000000..490b434 --- /dev/null +++ b/internal/skills/external.go @@ -0,0 +1,273 @@ +// External-skill discovery: read-only scanning of SKILL.md-bearing +// directories that live outside the gateway-hosted CAS skill set, per +// cfg.Skills.ExternalDirs (internal/config/config.go's SkillsConfig). +// +// Rationale (Hermes analog "skills.external_dirs"): promoted knowledge is +// reference material, not configuration. dojo may READ skills authored for +// foreign agent ecosystems -- Claude Code's ".claude/skills", Cursor's +// ".cursor/skills", the emerging "~/.agents/skills" convention popularized +// by Crush -- but this package never creates, modifies, or deletes anything +// under them. +// +// Frontmatter is hand-parsed rather than pulled in via a YAML library: no +// YAML package is a direct dependency of this module (checked against +// go.mod before writing this file), and the SKILL.md dialect only ever +// needs two top-level scalar keys (name, description), so a small +// hand-rolled scanner avoids adding a dependency for two string fields. +package skills + +import ( + "os" + "path/filepath" + "sort" + "strings" +) + +// maxDescriptionRunes caps ExternalSkill.Description length. 200 runes is +// enough for a one-line summary without letting a runaway SKILL.md balloon +// output in list views. +const maxDescriptionRunes = 200 + +// maxExternalSkills defensively bounds the total number of skills a single +// ScanExternal call will return, in case a misconfigured external dir (e.g. +// a home directory or a symlink loop target) contains far more +// SKILL.md-bearing entries than any real skill set would. It is a package +// var rather than a const so tests can shrink it and exercise the cap +// without staging 500 fixture files. +var maxExternalSkills = 500 + +// ExternalSkill is one skill discovered under a configured external +// directory (cfg.Skills.ExternalDirs). It is intentionally a much thinner +// shape than client.Skill (the gateway CAS skill entry): external skills +// are foreign, read-only reference material, not first-class dojo skills, +// so there is no ID/version/category/ports to carry. +type ExternalSkill struct { + // Name is the frontmatter `name:` value, or the containing directory + // name when frontmatter is absent, malformed, or leaves name empty. + Name string + // Description is the frontmatter `description:` value, truncated to + // maxDescriptionRunes runes. May be empty. + Description string + // Path is the absolute path to the SKILL.md file itself. + Path string + // SourceDir is the configured external dir this skill was found + // under, exactly as configured (pre-expansion) -- e.g. "~/.agents/skills", + // not the $HOME-expanded absolute form. + SourceDir string +} + +// ScanExternal walks each configured dir (missing dirs are silently +// skipped) and returns all discovered skills, stable-sorted by +// (SourceDir, Name). +// +// Two layouts are recognized per configured dir, depth-limited to avoid +// runaway recursion into an unrelated directory tree: +// +// - <dir>/SKILL.md -- the configured dir IS a single skill. +// - <dir>/<child>/SKILL.md -- the configured dir is a collection; each +// immediate subdirectory may itself be one skill. +// +// Both patterns are checked for every configured dir; there is no +// recursion beyond the one child level (no <dir>/<child>/<grandchild>/...). +// +// Path expansion: a leading "~/" (or a bare "~") expands to $HOME via +// os.UserHomeDir; other relative paths resolve against the process cwd via +// filepath.Abs; absolute paths pass through unchanged. +// +// ScanExternal is READ-ONLY: it never creates, modifies, or deletes +// anything on disk. +func ScanExternal(dirs []string) []ExternalSkill { + var results []ExternalSkill + + for _, dir := range dirs { + if len(results) >= maxExternalSkills { + break + } + + expanded, err := expandExternalDir(dir) + if err != nil { + continue + } + info, err := os.Stat(expanded) + if err != nil || !info.IsDir() { + // Missing (or not-a-directory) configured dirs are silently + // skipped -- an unconfigured foreign ecosystem on this + // machine is the common case, not an error. + continue + } + + // Layout: <dir>/SKILL.md -- the dir itself is one skill. + if sk, ok := loadSkillMD(filepath.Join(expanded, "SKILL.md"), dir); ok { + results = append(results, sk) + if len(results) >= maxExternalSkills { + break + } + } + + // Layout: <dir>/<child>/SKILL.md -- one level of skill + // subdirectories. No recursion past this depth. + entries, err := os.ReadDir(expanded) + if err != nil { + continue + } + for _, entry := range entries { + if len(results) >= maxExternalSkills { + break + } + if !entry.IsDir() { + continue + } + childSkillMD := filepath.Join(expanded, entry.Name(), "SKILL.md") + if sk, ok := loadSkillMD(childSkillMD, dir); ok { + results = append(results, sk) + } + } + } + + sort.SliceStable(results, func(i, j int) bool { + if results[i].SourceDir != results[j].SourceDir { + return results[i].SourceDir < results[j].SourceDir + } + return results[i].Name < results[j].Name + }) + return results +} + +// FindExternal returns the first skill whose Name matches (case-insensitive), +// or nil. "First" follows ScanExternal's (SourceDir, Name) order. +func FindExternal(dirs []string, name string) *ExternalSkill { + target := strings.ToLower(name) + for _, sk := range ScanExternal(dirs) { + if strings.ToLower(sk.Name) == target { + return &sk + } + } + return nil +} + +// expandExternalDir resolves a configured external dir to an absolute path. +// A leading "~/" or a bare "~" expands to $HOME; other relative paths +// resolve against the process cwd; absolute paths pass through unchanged +// (modulo filepath.Clean). +func expandExternalDir(dir string) (string, error) { + if dir == "~" || strings.HasPrefix(dir, "~/") { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + if dir == "~" { + return home, nil + } + return filepath.Join(home, strings.TrimPrefix(dir, "~/")), nil + } + return filepath.Abs(dir) +} + +// loadSkillMD reads and parses a single SKILL.md at path, attributing it to +// sourceDir (the pre-expansion configured dir). It reports ok=false when +// the file does not exist, is a directory, or cannot be read -- all of +// which are silently-skipped conditions per ScanExternal's contract. +func loadSkillMD(path, sourceDir string) (ExternalSkill, bool) { + data, err := os.ReadFile(path) + if err != nil { + return ExternalSkill{}, false + } + + name, description := parseFrontmatter(data) + if name == "" { + // Containing dir name: for both recognized layouts this is simply + // the parent directory of the SKILL.md file itself. + name = filepath.Base(filepath.Dir(path)) + } + description = truncateRunes(description, maxDescriptionRunes) + + absPath, err := filepath.Abs(path) + if err != nil { + absPath = path + } + + return ExternalSkill{ + Name: name, + Description: description, + Path: absPath, + SourceDir: sourceDir, + }, true +} + +// parseFrontmatter hand-parses the leading "---" ... "---" block of a +// SKILL.md file and extracts only the top-level `name:` and `description:` +// scalar values. Everything else -- nested keys, arrays, multiline `|`/`>` +// block scalars, unrecognized keys -- is deliberately ignored rather than +// fully parsed as YAML. +// +// If the file has no leading "---" line, or the block is never closed by a +// following "---" before EOF (malformed), no frontmatter is recognized and +// both return values are empty -- callers fall back to the containing +// directory name and an empty description, exactly as if no frontmatter +// existed at all. +func parseFrontmatter(data []byte) (name, description string) { + lines := strings.Split(string(data), "\n") + if len(lines) == 0 || strings.TrimRight(lines[0], "\r") != "---" { + return "", "" + } + + closed := false + for _, raw := range lines[1:] { + line := strings.TrimRight(raw, "\r") + if line == "---" { + closed = true + break + } + if v, ok := topLevelScalar(line, "name"); ok { + name = v + } + if v, ok := topLevelScalar(line, "description"); ok { + description = v + } + } + if !closed { + return "", "" + } + return name, description +} + +// topLevelScalar reports whether line is a top-level (non-indented) +// "key: value" line for the given key, returning the trimmed, unquoted +// value. A multiline block scalar marker ("|" or ">", with any chomping +// suffix) resolves to an empty string per parseFrontmatter's contract, +// rather than attempting to consume the following indented lines. +func topLevelScalar(line, key string) (string, bool) { + if line == "" || line[0] == ' ' || line[0] == '\t' { + return "", false // empty or indented => not a top-level key + } + prefix := key + ":" + if !strings.HasPrefix(line, prefix) { + return "", false + } + v := strings.TrimSpace(line[len(prefix):]) + if strings.HasPrefix(v, "|") || strings.HasPrefix(v, ">") { + return "", true + } + return trimQuotes(v), true +} + +// trimQuotes strips one matching pair of surrounding double or single +// quotes, if present. +func trimQuotes(s string) string { + if len(s) >= 2 { + if (s[0] == '"' && s[len(s)-1] == '"') || (s[0] == '\'' && s[len(s)-1] == '\'') { + return s[1 : len(s)-1] + } + } + return s +} + +// truncateRunes cuts s to at most n runes (not bytes), leaving it unchanged +// if it is already within the limit. +func truncateRunes(s string, n int) string { + r := []rune(s) + if len(r) <= n { + return s + } + return string(r[:n]) +} diff --git a/internal/skills/external_test.go b/internal/skills/external_test.go new file mode 100644 index 0000000..eafa5f1 --- /dev/null +++ b/internal/skills/external_test.go @@ -0,0 +1,272 @@ +package skills + +import ( + "os" + "path/filepath" + "strings" + "testing" + "unicode/utf8" +) + +// writeSkillMD creates (with parent dirs) a SKILL.md at dir/SKILL.md with +// the given raw content. +func writeSkillMD(t *testing.T, dir, content string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + path := filepath.Join(dir, "SKILL.md") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +func TestScanExternal_DirLevelLayout(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "solo-skill") + writeSkillMD(t, skillDir, "---\nname: Alpha\ndescription: Does alpha things.\n---\nbody\n") + + got := ScanExternal([]string{skillDir}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1 (got=%+v)", len(got), got) + } + want := ExternalSkill{ + Name: "Alpha", + Description: "Does alpha things.", + Path: filepath.Join(skillDir, "SKILL.md"), + SourceDir: skillDir, + } + if got[0] != want { + t.Fatalf("got %+v, want %+v", got[0], want) + } +} + +func TestScanExternal_ChildLevelLayout(t *testing.T) { + root := filepath.Join(t.TempDir(), "skills") + writeSkillMD(t, filepath.Join(root, "foo"), "---\nname: Foo\ndescription: foo skill\n---\n") + writeSkillMD(t, filepath.Join(root, "bar"), "---\nname: Bar\ndescription: bar skill\n---\n") + + got := ScanExternal([]string{root}) + if len(got) != 2 { + t.Fatalf("len(got) = %d, want 2 (got=%+v)", len(got), got) + } + // (SourceDir, Name) sort => Bar before Foo (same SourceDir, "Bar" < "Foo"). + if got[0].Name != "Bar" || got[1].Name != "Foo" { + t.Fatalf("got names [%s, %s], want [Bar, Foo]", got[0].Name, got[1].Name) + } + for _, sk := range got { + if sk.SourceDir != root { + t.Errorf("SourceDir = %q, want %q", sk.SourceDir, root) + } + } +} + +func TestScanExternal_NoRecursionBeyondOneChildLevel(t *testing.T) { + root := filepath.Join(t.TempDir(), "skills") + // Grandchild SKILL.md must NOT be discovered -- depth cap is one child level. + writeSkillMD(t, filepath.Join(root, "child", "grandchild"), "---\nname: TooDeep\n---\n") + + got := ScanExternal([]string{root}) + if len(got) != 0 { + t.Fatalf("len(got) = %d, want 0 (grandchild SKILL.md should not be found): %+v", len(got), got) + } +} + +func TestScanExternal_MissingDirSkipped(t *testing.T) { + root := t.TempDir() + valid := filepath.Join(root, "valid") + writeSkillMD(t, valid, "---\nname: Valid\n---\n") + missing := filepath.Join(root, "does-not-exist") + + got := ScanExternal([]string{missing, valid}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1 (got=%+v)", len(got), got) + } + if got[0].Name != "Valid" { + t.Fatalf("got name %q, want Valid", got[0].Name) + } +} + +func TestScanExternal_TildeExpansion(t *testing.T) { + fakeHome := t.TempDir() + t.Setenv("HOME", fakeHome) + + skillDir := filepath.Join(fakeHome, ".agents", "skills") + writeSkillMD(t, skillDir, "---\nname: Tilde\n---\n") + + got := ScanExternal([]string{"~/.agents/skills"}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1 (got=%+v)", len(got), got) + } + if got[0].SourceDir != "~/.agents/skills" { + t.Errorf("SourceDir = %q, want the pre-expansion literal %q", got[0].SourceDir, "~/.agents/skills") + } + wantPath := filepath.Join(skillDir, "SKILL.md") + if got[0].Path != wantPath { + t.Errorf("Path = %q, want %q", got[0].Path, wantPath) + } +} + +func TestScanExternal_FrontmatterPresentWithNestedDecoy(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "quoted") + content := "---\n" + + "name: \"Quoted Name\"\n" + + "description: A thing that does stuff.\n" + + " name: this-is-nested-and-must-be-ignored\n" + + "tags:\n" + + " - one\n" + + " - two\n" + + "---\n" + + "# Quoted Name\nBody text.\n" + writeSkillMD(t, skillDir, content) + + got := ScanExternal([]string{skillDir}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1", len(got)) + } + if got[0].Name != "Quoted Name" { + t.Errorf("Name = %q, want %q (quotes trimmed, nested decoy ignored)", got[0].Name, "Quoted Name") + } + if got[0].Description != "A thing that does stuff." { + t.Errorf("Description = %q, want %q", got[0].Description, "A thing that does stuff.") + } +} + +func TestScanExternal_FrontmatterAbsent(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "plain-dir") + writeSkillMD(t, skillDir, "# Just a heading\n\nNo frontmatter here.\n") + + got := ScanExternal([]string{skillDir}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1", len(got)) + } + if got[0].Name != "plain-dir" { + t.Errorf("Name = %q, want fallback to containing dir name %q", got[0].Name, "plain-dir") + } + if got[0].Description != "" { + t.Errorf("Description = %q, want empty", got[0].Description) + } +} + +func TestScanExternal_FrontmatterMalformedUnclosed(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "broken-dir") + // Opening "---" with no closing "---" before EOF. + writeSkillMD(t, skillDir, "---\nname: Broken\ndescription: Never closed\n") + + got := ScanExternal([]string{skillDir}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1", len(got)) + } + if got[0].Name != "broken-dir" { + t.Errorf("Name = %q, want fallback to containing dir name %q (unclosed frontmatter treated as absent)", got[0].Name, "broken-dir") + } + if got[0].Description != "" { + t.Errorf("Description = %q, want empty", got[0].Description) + } +} + +func TestScanExternal_MultilineDescriptionMarker(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "multiline") + content := "---\nname: Multiline\ndescription: |\n line one\n line two\n---\n" + writeSkillMD(t, skillDir, content) + + got := ScanExternal([]string{skillDir}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1", len(got)) + } + if got[0].Description != "" { + t.Errorf("Description = %q, want empty string for a `|` block scalar marker", got[0].Description) + } +} + +func TestScanExternal_DescriptionTruncation(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "long-desc") + // Multi-byte rune repeated so a byte-based (rather than rune-based) + // truncation would produce a visibly different, shorter rune count. + long := strings.Repeat("é", 250) + writeSkillMD(t, skillDir, "---\nname: LongDesc\ndescription: "+long+"\n---\n") + + got := ScanExternal([]string{skillDir}) + if len(got) != 1 { + t.Fatalf("len(got) = %d, want 1", len(got)) + } + gotRunes := utf8.RuneCountInString(got[0].Description) + if gotRunes != 200 { + t.Fatalf("rune count = %d, want 200", gotRunes) + } + wantRunes := []rune(long)[:200] + if got[0].Description != string(wantRunes) { + t.Errorf("Description does not match first 200 runes of source") + } +} + +func TestFindExternal_CaseInsensitive(t *testing.T) { + root := t.TempDir() + writeSkillMD(t, filepath.Join(root, "mixed"), "---\nname: MixedCase\n---\n") + dirs := []string{root} + + for _, query := range []string{"mixedcase", "MIXEDCASE", "MixedCase", "mIxEdCaSe"} { + sk := FindExternal(dirs, query) + if sk == nil { + t.Fatalf("FindExternal(%q) = nil, want a match", query) + } + if sk.Name != "MixedCase" { + t.Errorf("FindExternal(%q).Name = %q, want MixedCase", query, sk.Name) + } + } + + if sk := FindExternal(dirs, "nonexistent"); sk != nil { + t.Errorf("FindExternal(nonexistent) = %+v, want nil", sk) + } +} + +func TestScanExternal_SortStability(t *testing.T) { + root := filepath.Join(t.TempDir(), "skills") + // Three child dirs that all resolve to the SAME Name (and SourceDir), + // named so alphabetical directory-read order is unambiguous. A stable + // sort must preserve this discovery order for the tied keys. + for _, child := range []string{"a-first", "m-mid", "z-last"} { + writeSkillMD(t, filepath.Join(root, child), "---\nname: Tied\n---\n") + } + + got := ScanExternal([]string{root}) + if len(got) != 3 { + t.Fatalf("len(got) = %d, want 3", len(got)) + } + wantOrder := []string{"a-first", "m-mid", "z-last"} + for i, want := range wantOrder { + gotChild := filepath.Base(filepath.Dir(got[i].Path)) + if gotChild != want { + t.Errorf("position %d: child dir = %q, want %q (order=%v)", i, gotChild, want, childDirsOf(got)) + } + } +} + +func childDirsOf(skills []ExternalSkill) []string { + out := make([]string, len(skills)) + for i, sk := range skills { + out[i] = filepath.Base(filepath.Dir(sk.Path)) + } + return out +} + +func TestScanExternal_Cap(t *testing.T) { + orig := maxExternalSkills + maxExternalSkills = 2 + defer func() { maxExternalSkills = orig }() + + root := filepath.Join(t.TempDir(), "skills") + for _, child := range []string{"s1", "s2", "s3", "s4", "s5"} { + writeSkillMD(t, filepath.Join(root, child), "---\nname: "+child+"\n---\n") + } + + got := ScanExternal([]string{root}) + if len(got) != 2 { + t.Fatalf("len(got) = %d, want 2 (cap not enforced)", len(got)) + } +}