diff --git a/AGENTS.md b/AGENTS.md index 1dd5276..5c30926 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,6 +6,7 @@ Committy is a Rust CLI for conventional commits, branch policy, grouped commits, - Discover active policy with `cargo run -- schema --output json`. - Prefer `--non-interactive`, `--repo-path`, `--dry-run`, and `--output json`. +- Set `COMMITTY_NONINTERACTIVE=1` in the environment instead of relying on flag placement. `--non-interactive` and `-q`/`--quiet` are global and work on either side of the subcommand; every other flag (`--repo-path`, `--output`, `--dry-run`, and `--verbose`) must follow it. - Inspect `api_version`, `ok`, `errors`, and the command-specific plan before applying it. - Use the `committy-branch`, `committy-commit`, `committy-release`, and `committy-enforce` plugin skills when available. - Never infer permission for a push, publish, destructive Git action, or unrelated staging. diff --git a/CLAUDE.md b/CLAUDE.md index 524df5f..cec9cda 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,6 +6,7 @@ Committy is a Rust CLI for conventional commits, branch policy, grouped commits, - Discover active policy with `cargo run -- schema --output json`. - Prefer `--non-interactive`, `--repo-path`, `--dry-run`, and `--output json`. +- Set `COMMITTY_NONINTERACTIVE=1` in the environment instead of relying on flag placement. `--non-interactive` and `-q`/`--quiet` are global and work on either side of the subcommand; every other flag (`--repo-path`, `--output`, `--dry-run`, and `--verbose`) must follow it. - Inspect `api_version`, `ok`, `errors`, and the command-specific plan before applying it. - Use `/committy:committy-branch`, `/committy:committy-commit`, `/committy:committy-release`, and `/committy:committy-enforce` when the plugin is installed. - Never infer permission for a push, publish, destructive Git action, or unrelated staging. @@ -30,4 +31,4 @@ Tests are in `tests/`; CLI integration tests use `assert_cmd` and temporary repo - `src/workflow/`, `src/versioning/`, `src/packages/`: release side effects - `plugins/committy/`: shared Codex and Claude skills -PRs target `develop`. Commits follow Conventional Commits. The canonical agent reference is `docs/src/content/docs/reference/agent-workflows.mdx`. +PRs target `develop`. Commits follow Conventional Commits. Never add AI attribution trailers — no `Co-Authored-By:`, no `Claude-Session:`, no equivalent — to commit messages or PR descriptions, even when tooling suggests them. The canonical agent reference is `docs/src/content/docs/reference/agent-workflows.mdx`. diff --git a/docs/audits/agent-compatibility-2026-09.md b/docs/audits/agent-compatibility-2026-09.md new file mode 100644 index 0000000..1732d78 --- /dev/null +++ b/docs/audits/agent-compatibility-2026-09.md @@ -0,0 +1,278 @@ +# Agent compatibility & AI capability audit — September 2026 + +Scope: everything in this repository that an AI coding agent touches — the agent +instruction files, the shared Codex/Claude plugin, the machine-readable CLI +contract (`--output json` / `schema`), the generated enforcement (hooks + CI), +and the in-process LLM integration under `src/ai/`. + +Method: full read of the agent-facing surfaces at `caf53e3` (v1.9.1), compared +against the state of the agent-integration ecosystem as of September 2026. + +--- + +## 1. What exists today + +| Surface | Location | State | +|---|---|---| +| Agent instructions | `AGENTS.md`, `CLAUDE.md` | Two near-identical files (diff is 2 lines) | +| Claude marketplace | `.claude-plugin/marketplace.json` | Valid, points at `./plugins/committy` | +| Codex marketplace | `.agents/plugins/marketplace.json` | Valid, local source | +| Plugin manifests | `plugins/committy/.claude-plugin/plugin.json`, `.codex-plugin/plugin.json` | Skills only; no commands/agents/hooks/mcpServers | +| Skills | 4 × `SKILL.md` (branch, commit, release, enforce) | Well-scoped, progressive disclosure, safety rules present | +| Codex interface shims | 4 × `agents/openai.yaml` | Display metadata only | +| Machine contract | `src/cli/output.rs` (`api_version = 1`) + per-command payloads | Stable envelope: `api_version`, `command`, `ok`, `dry_run`, `errors` | +| Capability discovery | `src/cli/commands/schema.rs` | 21 capabilities with consent metadata (was 10, bare) | +| Repo enforcement | `src/cli/commands/hooks.rs:14-37` | `commit-msg`, `pre-push`, GitHub Actions template | +| Local agent guardrail | `.claude/settings.json` + `.claude/hooks/block-dangerous-git.sh` | PreToolUse Bash denylist | +| LLM integration | `src/ai/mod.rs`, `src/cli/commands/group_commit.rs:98-151, 356-507` | OpenRouter + Ollama, `group-commit` only | + +The core architectural bet is **CLI-first with typed JSON**. That still holds, +but not for the reason an earlier draft gave: see §3.4, where the blanket +"never build an MCP server" recommendation is withdrawn, and §3.5, where the +CLI's own agent ergonomics turned out to be the real problem. + +--- + +## 2. What changed in the space (relevant deltas) + +1. **`AGENTS.md` is now the cross-tool standard.** Stewarded by the Agentic AI + Foundation (Linux Foundation), read by 30+ agents (Codex, Copilot, Cursor, + Gemini CLI, Jules, Aider, Zed, Windsurf, Devin), 60k+ repositories. Claude Code + remains the outlier reading `CLAUDE.md`; the accepted pattern is a one-line + `@AGENTS.md` import rather than a maintained duplicate. +2. **Agent Skills has a published spec.** Frontmatter keys are fixed to + `name`, `description`, `license`, `allowed-tools`, `metadata`, `compatibility`; + unknown keys fail validation. `allowed-tools` is the security-relevant addition. +3. **`.agents/skills/` is the cross-tool skill discovery path.** Codex CLI, the + Codex app, Claude Code and Copilot CLI all resolve it. Skills are now the most + portable plugin component; per-vendor interface files are the least portable. +4. **Codex shipped a plugin marketplace** (26 Mar 2026) bundling skills, MCP + servers and app integrations; self-serve publishing to the official directory + is still "coming soon" as of mid-2026. +5. **Claude Code plugins now carry more than skills**: `commands`, `agents`, + `hooks`, `mcpServers`, `outputStyles`, `lspServers`, auto-discovered from + standard directories. +6. **The MCP token argument has a shelf life.** The benchmarks (35× token cost, + reliability dropping to 72% on hard tasks, ~55k tokens for a naive GitHub + server) measure *eager-loading* MCP, where every tool schema is injected at + session start. Progressive disclosure recovers a reported 60–85%. +7. **The 2026-07-28 MCP spec is the largest revision since launch**: stateless + core, sessions gone, Roots/Sampling/Logging deprecated, MCP Apps and Tasks + added, progressive discovery and tool search on the roadmap. Recommendations + written against the old full-bundle design no longer apply cleanly. +8. **Ollama structured output moved to JSON-Schema-constrained decoding** via a + **top-level** `format` field; `/api/chat` still defaults to `stream: true`. + +--- + +## 3. Findings + +Severity: **P0** = broken or misleading in the field · **P1** = real compatibility +or maintenance cost · **P2** = polish. + +### 3.1 Defects to fix + +**P0-1 — The Ollama provider cannot work as written.** +`src/ai/mod.rs:160-205` posts to `/api/chat` without `stream: false`. Ollama +defaults to `stream: true`, so the response is newline-delimited JSON events, and +`resp.json::()` will fail to deserialize on essentially every call. +Every `--ai-provider ollama` run therefore ends in `LlmError::Parse` and silently +falls back to the default message. Fix: send `stream: false`. + +**P0-2 — Ollama JSON mode is a no-op.** +`format` is nested inside `options` (`src/ai/mod.rs:174-196`). Ollama reads +`format` as a **top-level** request field, so JSON mode has never taken effect. +Fix: hoist `format` to the request body, and prefer passing the +`AiCommitSuggestion` JSON Schema rather than the string `"json"` — Ollama has +supported schema-constrained decoding since 0.5. + +**P0-3 — AI in default (safe) mode is given no signal at all.** +`src/cli/commands/group_commit.rs:400-413`: unless `--ai-allow-sensitive` is set, +the user prompt contains only the group name and the default type/short, and +instructs the model to work "without revealing code or filenames". The model is +asked to improve a description it cannot see the basis for. In practice this +either reproduces the default or produces a lint failure. Fix: send redacted but +real signal by default — path *shapes* (extension histogram, directory prefixes, +add/modify/delete counts, scope mappings from `.committy/config.toml`) — none of +which is file content. Reserve `--ai-allow-sensitive` for actual diff hunks. + +**P0-4 — `--ai` is silently ignored in `--mode apply`.** +The AI block lives only in the `"plan"` arm (`group_commit.rs:308-507`); the +`"apply"` arm (`:533+`) rebuilds groups from scratch with no LLM call. +`docs/src/content/docs/reference/ai-flags.mdx` documents +`--mode apply ... --ai --ai-provider ollama` as a working example. Either wire +apply to reuse the planned messages, or reject `--ai` with `--mode apply` and fix +the doc. Silently dropping a flag is the worst of the three options. + +**P1-5 — `--ai-diff-lines-per-file` is a dead flag.** +Declared at `group_commit.rs:143-147` as `_ai_diff_lines_per_file` and never read. +It is documented in `ai-flags.mdx` and accepted on the command line. Remove it, or +implement it as part of P0-3. + +**P1-6 — `ai-flags.mdx` overstates what is sent and understates what is not.** +It claims `--ai-allow-sensitive` "may include snippets or diff lines from code" — +no diff is ever sent, in either mode. It also recommends +`--ai-model openrouter/anthropic/claude-3.5-sonnet`, a model generation that is two +families out of date. Both should be corrected. + +### 3.2 Compatibility gaps to close + +**P1-7 — `capabilities` in `schema` was stale and undersold the CLI.** ✅ **Fixed.** +`schema.rs` advertised 10 capabilities covering branch, commit and hooks only, +while the `committy-release` and `committy-enforce` skills instructed agents to +run `bump`, `tag`, `changelog` and `config validate` — commands the capability +list denied existed. `agent-workflows.mdx` promises capabilities exist precisely +"so skills do not need to hard-code the command surface"; they had to. + +Now 21 capabilities cover the whole agent surface, and each declares `mutating` +and `requires_confirmation` so an agent can reason about consent without +pattern-matching command names (`tag.publish` is the only one gated on an +explicit confirmation flag). Covered by +`tests/agent_ergonomics_tests.rs`. + +Still open: the list is hand-maintained and can drift again. Deriving it from +the command registry would close that for good. + +**P1-8 — `AGENTS.md` and `CLAUDE.md` are maintained duplicates.** +The diff is two lines. This will drift. Make `AGENTS.md` canonical and reduce +`CLAUDE.md` to an `@AGENTS.md` import plus the Claude-specific skill-invocation +line — the pattern the ecosystem settled on. + +**P1-9 — No skills are exposed at `.agents/skills/`.** +`.agents/plugins/marketplace.json` covers Codex plugin installation, but the +cross-tool discovery path that Copilot CLI and others read is `.agents/skills/`. +Exposing the four skills there (symlink, or move the canonical copy there and +point both plugin manifests at it) makes them portable to every agent that +implements the spec, at zero maintenance cost. + +**P1-10 — Skill frontmatter omits the spec's optional security fields.** +All four `SKILL.md` files carry only `name` and `description`. The spec now +defines `allowed-tools`, `license`, `metadata` and `compatibility`. Adding +`allowed-tools` to each skill pre-authorizes exactly what the skill needs in +security-conscious runtimes, and `metadata` gives you a version handle for the +skills independent of the CLI version. + +**P1-11 — Only two agents are addressed; the instruction file is the cheap part.** +There is no `.github/copilot-instructions.md`, no `GEMINI.md`, no Cursor rules. +Given `AGENTS.md` already exists, these are thin pointer files. Worth doing for +Copilot specifically — it is the widest-deployed agent that does not read +`AGENTS.md` natively. + +**P1-12 — No tests cover the agent plugin surface.** +`CLAUDE.md` explicitly asks for "public-seam tests for machine output, exit codes, +Git behavior, hook protocol, and **plugin manifests**." `tests/agent_cli_tests.rs` +covers the first four well (21 tests); nothing validates the marketplace JSON, +plugin manifests, or SKILL.md frontmatter against their schemas. A manifest typo +currently ships silently. Add a test that parses all six JSON/YAML manifests and +asserts every `SKILL.md` frontmatter uses only spec-allowed keys. + +**P2-13 — Generated CI template is dated and slow.** +`hooks.rs:31-36` pins `actions/checkout@v4` (v5 is current) and installs via +`cargo install committy --locked` on every PR — a multi-minute Rust build per run +with no caching and no toolchain pin. Prefer downloading the release binary from +`install.sh`, or add `Swatinem/rust-cache`. The same `checkout@v4` pin appears in +all three of this repo's own workflows. + +**P2-14 — `$committy-commit` syntax leaks Codex convention into shared skills.** +`committy-release/SKILL.md` closes with "Use `$committy-commit`". Claude Code +invokes these as `/committy:committy-commit` (as `CLAUDE.md` itself documents). +Since the `SKILL.md` body is shared across both runtimes, reference sibling skills +by bare name and let each runtime resolve it. + +**P2-15 — The local `block-dangerous-git.sh` guardrail is easily evaded and +over-blocks.** `.claude/hooks/block-dangerous-git.sh` greps the raw command string +for nine substrings. It matches the *text* of a command, not its effect — writing +this very report through a heredoc was blocked because the prose names the push +command. It misses trivial variants (double-space, `env X=1` prefix, `git -C .`), +and it blocks the push-with-upstream form that this repository's own agent +workflow instructions require. It also assumes `jq` is installed with no fallback. +Treat it as a speed bump, not a control: the real enforcement is the `pre-push` +hook, which is policy-engine backed. + +### 3.3 Removals + +| Remove | Where | Why | +|---|---|---| +| `--ai-diff-lines-per-file` | `group_commit.rs:143-147`, `ai-flags.mdx` | Never read; pure surface area (or implement under P0-3) | +| `LlmError::_Timeout` | `src/ai/mod.rs:12` | Dead variant; timeouts surface as `RequestFailed` | +| Commented-out `LlmProvider` enum | `src/ai/mod.rs:30-35` | Dead code; provider is a `&str` match at the call site | +| Duplicated body of `CLAUDE.md` | `CLAUDE.md` | Replace with `@AGENTS.md` import (P1-8) | +| `agents/openai.yaml` × 4 | `plugins/committy/skills/*/agents/` | Candidate only — verify Codex still reads these; if the marketplace entry supplies display metadata, they are redundant per-vendor files on the least portable layer | +| Empty roadmap page | `docs/src/content/docs/project/roadmap.mdx` | Frontmatter only, no body — publishes an empty page | + +### 3.4 On building an MCP server + +An earlier revision of this report said flatly "do not build one". That was +argued against eager-loading MCP, which the protocol has since moved off: the +2026-07-28 spec makes the core stateless, drops sessions, and puts progressive +discovery and tool search on the roadmap. The headline benchmarks against MCP — +35× token cost, reliability falling to 72% on hard tasks — measure the old +full-bundle design; progressive disclosure reportedly recovers 60–85% of that. +So the blanket recommendation was wrong and is withdrawn. + +What remains true is the ordering. The observed agent struggle traced to three +mechanical CLI faults (§3.5), not to the CLI *being* a CLI, and all three were +fixable in an afternoon. An MCP server would have fixed them incidentally, by +deleting argv from the problem, while adding a server to build, distribute, and +keep in version lockstep with the binary — and it would not help the Git hook and +CI path at all, which must stay CLI and is where Committy's actual enforcement +lives. + +Revisit if agents still struggle now that §3.5 is fixed. If built, shape it as +roughly six stateless tools mapping to capability groups (`policy.discover`, +`commit.preview`/`apply`, `branch.preview`/`apply`, `lint`) rather than eighteen +tools mirroring subcommands, and have `policy.discover` serve the existing +`schema --output json` payload instead of re-describing the surface. + +Still not recommended: **an LLM path on the single-commit `commit` command.** The +calling agent already has the diff in context and writes better messages than a +256-token side-channel call. Committy's differentiated value is *validating* the +agent's message, not competing with it. + +### 3.5 CLI ergonomics — diagnosed, fixed, and under test + +Reproduced against v1.9.1 and now covered by `tests/agent_ergonomics_tests.rs`: + +| # | Fault | Fix | +|---|---|---| +| E-1 | `--non-interactive`, `-q` were root-only, so `committy schema --non-interactive …` — the form agents write — was rejected by clap | `global = true` on both | +| E-2 | Argument-parse rejections wrote nothing to stdout and prose to stderr, even with `--output json` | `invalid_usage` envelope on stdout; text mode unchanged | +| E-3 | `COMMITTY_NONINTERACTIVE=1` makes placement irrelevant but appeared in no agent-facing doc | documented in `AGENTS.md`, `CLAUDE.md`, `agent-workflows.mdx` | + +`--verbose` was deliberately left root-only: `-v` is `branch --validate` and +`--verbose` belongs to `config validate|show`, so promoting it would silently +steal both shorts. A test pins that trade-off. + +--- + +## 4. Suggested order of work + +0. ~~**P1-7**, **E-1**, **E-2**, **E-3**~~ — done: capabilities expanded with + consent metadata, global flag placement, `invalid_usage` envelope, env var + documented. See §3.5. +1. **P0-1, P0-2** — make Ollama actually function. +2. **P0-4, P1-5, P1-6, §3.3 removals** — stop documenting and accepting things + that do not happen. +3. **P0-3** — give the default-safe AI path real, non-sensitive signal. +4. **P1-8, P1-9, P1-10** — cross-tool portability (`AGENTS.md` canonical, + `.agents/skills/`, spec-complete frontmatter). +5. **P1-12** — manifest and frontmatter tests so the step above cannot regress. +6. **P1-11, P2-13, P2-14, P2-15** — reach, CI cost, consistency, guardrail honesty. + +## 5. Sources + +- [AGENTS.md Field Guide, 2026 edition](https://www.iuriio.com/blog/posts/2026/05/agents-md-field-guide-2026) +- [AGENTS.md Specification](https://asdlc.io/practices/agents-md-spec/) +- [Agent Skills — Specification](https://agentskills.io/specification) +- [SKILL.md YAML frontmatter reference](https://github.com/shalomb/agent-skills/blob/main/docs/reference/yaml-frontmatter.md) +- [Claude Code plugins reference](https://code.claude.com/docs/en/plugins-reference) +- [Codex CLI plugin marketplace](https://codex.danielvaughan.com/2026/04/24/codex-cli-plugin-marketplace-building-distributing-extending/) +- [OpenAI's Codex gets plugins](https://thenewstack.io/openais-codex-gets-plugins/) +- [Ollama structured outputs](https://docs.ollama.com/capabilities/structured-outputs) +- [Ollama /api/chat reference](https://docs.ollama.com/api/chat) +- [MCP roadmap: progressive discovery and agent auth](https://www.developersdigest.tech/blog/mcp-roadmap-progressive-discovery-agent-auth) +- [Progressive tool loading is the new MCP context pattern](https://usewire.io/blog/progressive-tool-loading-mcp-context-pattern/) +- [MCP context bloat fix 2026: tool search, code mode, progressive disclosure](https://mcp.directory/blog/mcp-context-bloat-fix-2026-tool-search-code-mode-progressive-disclosure) +- [MCP servers use 35x more tokens than CLI tools](https://www.mindstudio.ai/blog/mcp-servers-35x-more-tokens-cli-tools-reliability-benchmark) +- [MCP vs CLI for AI agents](https://www.firecrawl.dev/blog/mcp-vs-cli) +- [CLI vs MCP: should your agent call tools or run commands?](https://blog.mcpservers.org/posts/cli-vs-mcp) diff --git a/docs/src/content/docs/reference/agent-workflows.mdx b/docs/src/content/docs/reference/agent-workflows.mdx index e170a05..fbe1bde 100644 --- a/docs/src/content/docs/reference/agent-workflows.mdx +++ b/docs/src/content/docs/reference/agent-workflows.mdx @@ -19,6 +19,36 @@ Preferred sequence: 5. Require explicit confirmation for any remote mutation. 6. Use `--repo-path` when the agent should operate on another checkout. +## Flag placement + +Committy has two flag namespaces, and mixing them up is the most common way an +agent invocation fails: + +- **Global flags** — `--non-interactive` and `-q`/`--quiet` — are accepted on + either side of the subcommand. +- **Everything else** — `--repo-path`, `--output`, `--dry-run`, and every + command-specific flag — must follow the subcommand. +- **`-v`/`--verbose` must precede the subcommand.** It is deliberately not + global: `-v` already means `--validate` on `branch`, and `--verbose` is a real + flag on `config validate` and `config show`. + +The most reliable option is to stop passing `--non-interactive` altogether and +set the environment variable instead, which makes placement irrelevant: + +```bash +export COMMITTY_NONINTERACTIVE=1 +committy schema --repo-path . --output json +``` + +`CI=1` has the same effect, so Committy is already non-interactive on most CI +runners. + +When an invocation is rejected outright — an unknown flag, a misplaced argument +— Committy still writes one JSON document to stdout if `--output json` was +requested, with `"code": "invalid_usage"` and clap's explanation as the message. +Agents can parse the rejection and self-correct rather than falling back to +scraping stderr. + ## Policy discovery ```bash diff --git a/src/cli/commands/schema.rs b/src/cli/commands/schema.rs index 492971d..f8754c2 100644 --- a/src/cli/commands/schema.rs +++ b/src/cli/commands/schema.rs @@ -37,50 +37,117 @@ struct SchemaOutput { struct CapabilityDefinition { name: &'static str, description: &'static str, + /// Whether applying this capability changes the repository or a remote. + mutating: bool, + /// Whether the CLI refuses to proceed without an explicit confirmation + /// flag. An agent must never infer this consent from the task alone. + requires_confirmation: bool, +} + +/// Read-only capability: previews and validation. +const fn read_only(name: &'static str, description: &'static str) -> CapabilityDefinition { + CapabilityDefinition { + name, + description, + mutating: false, + requires_confirmation: false, + } +} + +/// Local mutation: changes this checkout, needs no confirmation flag. +const fn local_write(name: &'static str, description: &'static str) -> CapabilityDefinition { + CapabilityDefinition { + name, + description, + mutating: true, + requires_confirmation: false, + } +} + +/// Remote mutation: gated behind an explicit confirmation flag. +const fn remote_write(name: &'static str, description: &'static str) -> CapabilityDefinition { + CapabilityDefinition { + name, + description, + mutating: true, + requires_confirmation: true, + } } fn capabilities() -> Vec { vec![ - CapabilityDefinition { - name: "branch.preview", - description: "Resolve and validate a branch without changing git", - }, - CapabilityDefinition { - name: "branch.apply", - description: "Create a validated branch", - }, - CapabilityDefinition { - name: "branch.lint", - description: "Validate structured or policy-enforced branch names", - }, - CapabilityDefinition { - name: "commit.preview", - description: "Resolve and lint a commit without changing git", - }, - CapabilityDefinition { - name: "commit.apply", - description: "Create a validated conventional commit", - }, - CapabilityDefinition { - name: "commit.amend", - description: "Preview or apply a validated amend", - }, - CapabilityDefinition { - name: "commit.lint", - description: "Lint one message or repository history", - }, - CapabilityDefinition { - name: "hooks.install", - description: "Preview or install native Git hooks and CI enforcement", - }, - CapabilityDefinition { - name: "hooks.commit-msg", - description: "Enforce commit messages through a native git hook", - }, - CapabilityDefinition { - name: "hooks.pre-push", - description: "Enforce outgoing commit history through a native git hook", - }, + read_only( + "schema.discover", + "Report active convention, types, and this capability list", + ), + read_only( + "branch.preview", + "Resolve and validate a branch without changing git", + ), + local_write("branch.apply", "Create a validated branch"), + read_only( + "branch.lint", + "Validate structured or policy-enforced branch names", + ), + read_only( + "commit.preview", + "Resolve and lint a commit without changing git", + ), + local_write("commit.apply", "Create a validated conventional commit"), + local_write("commit.amend", "Preview or apply a validated amend"), + read_only("commit.lint", "Lint one message or repository history"), + read_only( + "group-commit.plan", + "Group a dirty worktree into coherent commits without changing git", + ), + local_write( + "group-commit.apply", + "Create the planned grouped commits; pushing additionally requires confirmation", + ), + read_only( + "bump.preview", + "Compute the next version from commit history without writing files", + ), + local_write( + "bump.apply", + "Write the computed version to the configured version files", + ), + read_only( + "changelog.preview", + "Render a changelog from git history without writing files", + ), + read_only( + "tag.preview", + "Compute the next tag and release plan without changing git", + ), + local_write( + "tag.apply", + "Create the computed tag in the local repository", + ), + remote_write( + "tag.publish", + "Push tags and publish the release; requires an explicit confirmation flag", + ), + read_only( + "config.validate", + "Validate repository configuration and report findings", + ), + read_only( + "packages.list", + "List configured packages and their resolved versions", + ), + local_write( + "hooks.install", + "Preview or install native Git hooks and CI enforcement", + ), + read_only( + "hooks.commit-msg", + "Enforce commit messages through a native git hook", + ), + read_only( + "hooks.pre-push", + "Enforce outgoing commit history through a native git hook", + ), ] } diff --git a/src/cli/output.rs b/src/cli/output.rs index 0fb74c6..b70710d 100644 --- a/src/cli/output.rs +++ b/src/cli/output.rs @@ -18,12 +18,41 @@ struct MachineError<'a> { #[derive(Debug, Serialize)] struct ErrorEnvelope<'a> { api_version: u8, - command: &'static str, + command: &'a str, ok: bool, dry_run: bool, errors: [MachineError<'a>; 1], } +/// Strip ANSI styling so the envelope carries plain text. +/// +/// Clap colours its rejections, and those escape sequences would otherwise end +/// up inside a JSON string value. +fn strip_ansi(text: &str) -> String { + static ANSI: once_cell::sync::Lazy = + once_cell::sync::Lazy::new(|| regex::Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]").unwrap()); + ANSI.replace_all(text, "").into_owned() +} + +/// Emit the machine envelope for an argument-parse rejection. +/// +/// `command` is attributed from raw argv and falls back to `"unknown"`, because +/// clap rejects the invocation before any value is parsed. +pub fn print_usage_error(command: &str, message: &str) { + let message = strip_ansi(message); + let payload = ErrorEnvelope { + api_version: API_VERSION, + command, + ok: false, + dry_run: false, + errors: [MachineError { + code: "invalid_usage", + message: &message, + }], + }; + println!("{}", serde_json::to_string(&payload).unwrap()); +} + pub fn print_error(context: MachineContext, error: &CliError) { let message = error.to_string(); let payload = ErrorEnvelope { diff --git a/src/main.rs b/src/main.rs index d55d427..8f97af2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -56,25 +56,91 @@ struct Opt { )] pre_release: bool, - #[structopt(long = "non-interactive", help = "Run in non-interactive mode")] + #[structopt( + long = "non-interactive", + global = true, + help = "Run in non-interactive mode" + )] non_interactive: bool, #[structopt(long = "metrics-toggle", help = "Toggle metrics collection on/off")] metrics_toggle: bool, + // Not global: `-v` is already taken by `branch --validate` and `--verbose` + // by `config validate|show`. Promoting it would silently steal those + // shorts. Agents use `-q` and `--non-interactive`, which are global below. #[structopt( short = "v", long = "verbose", parse(from_occurrences), - help = "Increase verbosity (-v, -vv)" + help = "Increase verbosity (-v, -vv); must precede the subcommand" )] verbose: u8, - #[structopt(short = "q", long = "quiet", help = "Reduce verbosity (errors only)")] + #[structopt( + short = "q", + long = "quiet", + global = true, + help = "Reduce verbosity (errors only)" + )] quiet: bool, } +/// Subcommand names as they appear on the command line. +/// +/// Used only to attribute an argv rejection to a command, since clap gives us +/// no parsed value in that case. +const SUBCOMMAND_NAMES: &[&str] = &[ + "commit", + "amend", + "tag", + "bump", + "changelog", + "lint", + "lint-message", + "example", + "info", + "ls", + "schema", + "version", + "branch", + "group-commit", + "hooks", + "init", + "config", + "packages", +]; + +/// Whether the caller asked for machine output, determined from raw argv. +/// +/// An argv rejection happens before any flag is parsed, so this is the only way +/// to know whether the caller is an agent expecting one JSON document. +fn requested_json_output(args: &[String]) -> bool { + args.iter().enumerate().any(|(index, arg)| { + arg == "--output=json" + || (arg == "--output" && args.get(index + 1).map(String::as_str) == Some("json")) + }) +} + +/// Best-effort command attribution for an argv rejection. +fn command_from_args(args: &[String]) -> &str { + args.iter() + .skip(1) + .find(|arg| !arg.starts_with('-')) + .map(String::as_str) + .filter(|candidate| SUBCOMMAND_NAMES.contains(candidate)) + .unwrap_or("unknown") +} + fn main() { + // Parse argv before touching configuration so a malformed invocation is + // rejected without side effects. + let args: Vec = std::env::args().collect(); + let opt = match Opt::from_iter_safe(&args) { + Ok(opt) => opt, + Err(error) => exit_on_parse_error(error, &args), + }; + // Load configuration let mut config = Config::load().unwrap_or_else(|_| { let default_config = Config::default(); @@ -84,7 +150,7 @@ fn main() { default_config }); - if let Err(e) = run(&mut config) { + if let Err(e) = run(&mut config, opt) { eprintln!("{e}"); let exit_code = e .downcast_ref::() @@ -94,9 +160,32 @@ fn main() { } } -fn run(config: &mut Config) -> Result<()> { - let opt = Opt::from_args(); +/// Report a clap rejection and terminate. +/// +/// `--help` and `--version` arrive here as errors too; they are successful +/// output and keep their plain-text form. A genuine rejection honours +/// `--output json` so that an agent's parser sees the same envelope it sees for +/// every other failure, instead of an empty stdout and prose on stderr. +fn exit_on_parse_error(error: structopt::clap::Error, args: &[String]) -> ! { + use structopt::clap::ErrorKind; + + if matches!( + error.kind, + ErrorKind::HelpDisplayed | ErrorKind::VersionDisplayed + ) { + println!("{}", error.message); + std::process::exit(0); + } + + if requested_json_output(args) { + cli::output::print_usage_error(command_from_args(args), &error.message); + } else { + eprintln!("{}", error.message); + } + std::process::exit(1); +} +fn run(config: &mut Config, opt: Opt) -> Result<()> { // Initialize logger based on verbosity flags let mut builder = Builder::from_env(Env::default().default_filter_or("info")); let level = if opt.quiet { diff --git a/tests/agent_ergonomics_tests.rs b/tests/agent_ergonomics_tests.rs new file mode 100644 index 0000000..6c9f2f6 --- /dev/null +++ b/tests/agent_ergonomics_tests.rs @@ -0,0 +1,438 @@ +//! Agent ergonomics: the seams where AI coding agents actually trip. +//! +//! Three classes of failure are covered here: +//! 1. Global flags must parse on either side of the subcommand. +//! 2. Argument-parse failures must honour `--output json` like every other error. +//! 3. `schema` must advertise the whole command surface, with consent metadata. + +mod common; + +use serde_json::Value; +use std::fs; +use std::process::Command as StdCommand; +use tempfile::tempdir; + +fn setup_repo() -> tempfile::TempDir { + common::setup_test_env(); + + let dir = tempdir().expect("Failed to create temp directory"); + + for args in [ + vec!["init"], + vec!["config", "user.name", "Test User"], + vec!["config", "user.email", "test@example.com"], + ] { + StdCommand::new("git") + .args(&args) + .current_dir(&dir) + .output() + .unwrap_or_else(|e| panic!("git {args:?} failed: {e}")); + } + + fs::write(dir.path().join("tracked.txt"), "initial\n").expect("Failed to write tracked file"); + StdCommand::new("git") + .args(["add", "tracked.txt"]) + .current_dir(&dir) + .output() + .expect("Failed to stage tracked file"); + StdCommand::new("git") + .args(["commit", "-m", "feat: initial"]) + .current_dir(&dir) + .output() + .expect("Failed to create initial commit"); + + dir +} + +fn stdout_of(assert: &assert_cmd::assert::Assert) -> String { + String::from_utf8(assert.get_output().stdout.clone()).expect("stdout was not valid UTF-8") +} + +// --------------------------------------------------------------------------- +// 1. Global flag placement +// --------------------------------------------------------------------------- + +/// The documented form. Guards against a regression while fixing the rest. +#[test] +fn test_global_flags_before_subcommand_still_parse() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args([ + "--non-interactive", + "-q", + "schema", + "--repo-path", + ".", + "--output", + "json", + ]) + .assert() + .success(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()).unwrap(); + assert_eq!(v["command"], Value::String("schema".into())); +} + +/// The form agents actually write: global flags after the subcommand. +#[test] +fn test_non_interactive_parses_after_subcommand() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args([ + "schema", + "--non-interactive", + "--repo-path", + ".", + "--output", + "json", + ]) + .assert() + .success(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()).unwrap(); + assert_eq!(v["command"], Value::String("schema".into())); +} + +#[test] +fn test_quiet_parses_after_subcommand() { + let temp_dir = setup_repo(); + + for flag in ["-q", "--quiet"] { + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args(["schema", flag, "--repo-path", ".", "--output", "json"]) + .assert() + .success(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()) + .unwrap_or_else(|e| panic!("`schema {flag}` did not emit JSON: {e}")); + assert_eq!(v["command"], Value::String("schema".into())); + } +} + +/// `--verbose` is deliberately *not* global. +/// +/// `-v` is `branch --validate` and `--verbose` is a real flag on +/// `config validate|show`. Promoting the root flag would silently steal both. +/// This test pins that trade-off so a later change has to confront it. +#[test] +fn test_verbose_stays_root_only_because_subcommands_own_it() { + let temp_dir = setup_repo(); + + common::committy_cmd() + .current_dir(&temp_dir) + .args([ + "--verbose", + "schema", + "--repo-path", + ".", + "--output", + "json", + ]) + .assert() + .success(); + + // `branch -v` must still mean `--validate`, not root verbosity. + common::committy_cmd() + .current_dir(&temp_dir) + .args([ + "branch", + "-v", + "--name", + "feat-keeps-validate", + "--output", + "json", + ]) + .assert() + .success(); +} + +/// Mutating commands take the same treatment, not just read-only ones. +#[test] +fn test_global_flags_after_subcommand_on_branch_and_commit() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args([ + "branch", + "--non-interactive", + "--name", + "feat-agent-ergonomics", + "--dry-run", + "--output", + "json", + ]) + .assert() + .success(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()).unwrap(); + assert_eq!(v["command"], Value::String("branch".into())); + assert_eq!(v["dry_run"], Value::Bool(true)); + + // `commit` reports a command-level error here (nothing staged), but it must + // be a *parsed* command-level error, not an argv rejection. + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args([ + "commit", + "--non-interactive", + "--type", + "feat", + "--message", + "agent ergonomics", + "--dry-run", + "--output", + "json", + ]) + .assert(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()) + .expect("commit must emit a JSON envelope, not an argv rejection"); + assert_eq!(v["command"], Value::String("commit".into())); +} + +// --------------------------------------------------------------------------- +// 2. Argument-parse errors must honour --output json +// --------------------------------------------------------------------------- + +#[test] +fn test_unknown_flag_with_json_output_emits_error_envelope() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args(["schema", "--output", "json", "--definitely-not-a-flag"]) + .assert() + .failure(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()) + .expect("an argv rejection must still produce one JSON document on stdout"); + + assert_eq!(v["api_version"], Value::from(1)); + assert_eq!(v["command"], Value::String("schema".into())); + assert_eq!(v["ok"], Value::Bool(false)); + assert_eq!( + v["errors"][0]["code"], + Value::String("invalid_usage".into()) + ); + assert!( + v["errors"][0]["message"] + .as_str() + .expect("message must be a string") + .contains("--definitely-not-a-flag"), + "the envelope must name the offending argument so an agent can self-correct" + ); +} + +#[test] +fn test_unknown_flag_with_equals_form_json_output_emits_envelope() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args(["schema", "--output=json", "--definitely-not-a-flag"]) + .assert() + .failure(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()) + .expect("--output=json must be recognised as well as --output json"); + assert_eq!(v["ok"], Value::Bool(false)); + assert_eq!( + v["errors"][0]["code"], + Value::String("invalid_usage".into()) + ); +} + +/// An unknown subcommand cannot be attributed to a command; the envelope still +/// has to be valid so the agent's parser does not blow up. +#[test] +fn test_unknown_subcommand_with_json_output_emits_envelope() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args(["nonexistent-command", "--output", "json"]) + .assert() + .failure(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()) + .expect("an unknown subcommand must still produce one JSON document"); + assert_eq!(v["ok"], Value::Bool(false)); + assert_eq!(v["command"], Value::String("unknown".into())); +} + +/// Humans keep the terminal-friendly behaviour: plain text on stderr, no JSON. +#[test] +fn test_unknown_flag_without_json_output_stays_human_readable() { + let temp_dir = setup_repo(); + + let assert = common::committy_cmd() + .current_dir(&temp_dir) + .args(["schema", "--definitely-not-a-flag"]) + .assert() + .failure(); + + assert!( + stdout_of(&assert).trim().is_empty(), + "text mode must not print a JSON document to stdout" + ); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).unwrap(); + assert!( + stderr.contains("--definitely-not-a-flag"), + "text mode must still explain the problem on stderr" + ); +} + +// --------------------------------------------------------------------------- +// 3. Capability discovery +// --------------------------------------------------------------------------- + +fn capabilities_of(temp_dir: &tempfile::TempDir) -> Vec { + let assert = common::committy_cmd() + .current_dir(temp_dir) + .args(["--non-interactive", "schema", "--output", "json"]) + .assert() + .success(); + + let v: Value = serde_json::from_str(stdout_of(&assert).trim()).unwrap(); + v["capabilities"] + .as_array() + .expect("schema must expose a capabilities array") + .clone() +} + +#[test] +fn test_capabilities_cover_the_whole_agent_command_surface() { + let temp_dir = setup_repo(); + let capabilities = capabilities_of(&temp_dir); + + let names: Vec<&str> = capabilities + .iter() + .map(|c| c["name"].as_str().expect("capability needs a name")) + .collect(); + + // Every command the shipped skills instruct an agent to run. + for expected in [ + "branch.preview", + "branch.apply", + "branch.lint", + "commit.preview", + "commit.apply", + "commit.amend", + "commit.lint", + "group-commit.plan", + "group-commit.apply", + "bump.preview", + "bump.apply", + "changelog.preview", + "tag.preview", + "tag.apply", + "tag.publish", + "config.validate", + "packages.list", + "hooks.install", + "hooks.commit-msg", + "hooks.pre-push", + "schema.discover", + ] { + assert!( + names.contains(&expected), + "capability `{expected}` is missing; skills hard-code it today. Present: {names:?}" + ); + } +} + +#[test] +fn test_capabilities_declare_consent_metadata() { + let temp_dir = setup_repo(); + let capabilities = capabilities_of(&temp_dir); + + for capability in &capabilities { + let name = capability["name"].as_str().unwrap(); + assert!( + capability["mutating"].is_boolean(), + "capability `{name}` must declare `mutating`" + ); + assert!( + capability["requires_confirmation"].is_boolean(), + "capability `{name}` must declare `requires_confirmation`" + ); + assert!( + capability["description"].is_string(), + "capability `{name}` must keep its description" + ); + } + + let by_name = |needle: &str| -> Value { + capabilities + .iter() + .find(|c| c["name"] == Value::String(needle.into())) + .unwrap_or_else(|| panic!("missing capability {needle}")) + .clone() + }; + + // Previews never touch the repository. + for read_only in ["branch.preview", "commit.preview", "schema.discover"] { + assert_eq!( + by_name(read_only)["mutating"], + Value::Bool(false), + "{read_only} must not be marked mutating" + ); + assert_eq!( + by_name(read_only)["requires_confirmation"], + Value::Bool(false) + ); + } + + // Local writes mutate but do not need the remote-mutation confirmation flag. + assert_eq!(by_name("commit.apply")["mutating"], Value::Bool(true)); + assert_eq!( + by_name("commit.apply")["requires_confirmation"], + Value::Bool(false) + ); + + // Remote mutation is the case an agent must never infer consent for. + assert_eq!(by_name("tag.publish")["mutating"], Value::Bool(true)); + assert_eq!( + by_name("tag.publish")["requires_confirmation"], + Value::Bool(true), + "publishing must advertise that it needs explicit confirmation" + ); +} + +// --------------------------------------------------------------------------- +// 4. The escape hatch must be discoverable where agents read +// --------------------------------------------------------------------------- + +#[test] +fn test_agent_docs_mention_the_non_interactive_env_var() { + let repo_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + + let agent_facing = [ + "AGENTS.md", + "CLAUDE.md", + "docs/src/content/docs/reference/agent-workflows.mdx", + ]; + + for relative in agent_facing { + let body = fs::read_to_string(repo_root.join(relative)) + .unwrap_or_else(|e| panic!("cannot read {relative}: {e}")); + assert!( + body.contains("COMMITTY_NONINTERACTIVE"), + "{relative} must document COMMITTY_NONINTERACTIVE; it is the escape hatch \ + that makes flag placement irrelevant for agents" + ); + + // Documenting `-v` as global would be worse than not documenting + // placement at all: it reads as permission to write `schema -v`. + assert!( + !body.contains("`--non-interactive`, `-q`, `-v`"), + "{relative} lists -v among the global flags, but --verbose is root-only" + ); + } +}