From a603feaae60996cc274047a69f52533acf77399c Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:44:05 +0200 Subject: [PATCH 01/14] docs(agents): add September 2026 agent compatibility audit Audit the repository's agent-facing surfaces (AGENTS.md/CLAUDE.md, the shared Codex/Claude plugin and skills, the schema capability contract, generated hooks/CI, and the src/ai LLM integration) against the current state of the agent-integration ecosystem. Records 15 prioritised findings covering defects, compatibility gaps and removals, plus two explicit non-recommendations. --- docs/audits/agent-compatibility-2026-09.md | 237 +++++++++++++++++++++ 1 file changed, 237 insertions(+) create mode 100644 docs/audits/agent-compatibility-2026-09.md diff --git a/docs/audits/agent-compatibility-2026-09.md b/docs/audits/agent-compatibility-2026-09.md new file mode 100644 index 0000000..680f84e --- /dev/null +++ b/docs/audits/agent-compatibility-2026-09.md @@ -0,0 +1,237 @@ +# 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:42-84` | 10 hard-coded capabilities | +| 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 — **CLI-first with typed JSON, not an MCP server** — +is the right one and has been validated by the wider ecosystem this year (see §2). +Nothing below asks you to change that. + +--- + +## 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. **CLI beat MCP on token cost for known-interface tools** (roughly 4–32× cheaper; + a naive GitHub MCP server costs ~55k tokens of context before doing anything). + The 2026 consensus is hybrid: CLI for tools the model already understands, MCP + only where you need auth, statefulness, or governance. +7. **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` is stale and undersells the CLI.** +`schema.rs:42-84` advertises 10 capabilities covering branch, commit and hooks +only. Absent: `release.plan` / `tag.preview` / `tag.apply` / `tag.publish`, +`changelog.preview`, `bump.preview` / `bump.apply`, `group-commit.plan` / +`group-commit.apply`, `packages.*`, `config.validate`, `init`. The +`committy-release` and `committy-enforce` skills already instruct agents to run +`bump`, `tag`, `changelog` and `config validate` — commands the capability list +denies exist. `agent-workflows.mdx` promises capabilities exist precisely "so +skills do not need to hard-code the command surface"; today they must. This is the +single highest-leverage fix in the report: it is the contract every skill reads +first. + +Two sub-improvements while you are in there: give each capability a `mutating: +bool` and `requires_confirmation: bool` field so an agent can reason about consent +without pattern-matching command names, and consider deriving the list from the +command registry so it cannot drift again. + +**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 Explicitly *not* recommended + +- **Do not build a Committy MCP server.** The 2026 evidence is that CLI invocation + of a tool with a discoverable, typed interface costs a fraction of the tokens an + MCP tool listing does, and Committy has no auth, statefulness or multi-tenancy + that would justify the other side of the trade. The `schema` command already + does MCP's discovery job at a few hundred tokens. Invest in §3.2's P1-7 instead. +- **Do not add an LLM path to 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. + +--- + +## 4. Suggested order of work + +1. **P1-7** — expand and enrich `capabilities` (unblocks every skill; pure win). +2. **P0-1, P0-2** — make Ollama actually function. +3. **P0-4, P1-5, P1-6, §3.3 removals** — stop documenting and accepting things + that do not happen. +4. **P0-3** — give the default-safe AI path real, non-sensitive signal. +5. **P1-8, P1-9, P1-10** — cross-tool portability (`AGENTS.md` canonical, + `.agents/skills/`, spec-complete frontmatter). +6. **P1-12** — manifest and frontmatter tests so step 5 cannot regress. +7. **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 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) From 64f0a627f136be1d35464b0da99342c56545db55 Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:44:33 +0200 Subject: [PATCH 02/14] fix(cli): make agent invocations parse and fail machine-readably Agents consistently failed against the CLI for three mechanical reasons, each now reproduced and covered by tests/agent_ergonomics_tests.rs. - --non-interactive and -q/--quiet are global, so they parse on either side of the subcommand. --verbose stays root-only: -v belongs to `branch --validate` and --verbose to `config validate|show`, and promoting it would steal both shorts. A test pins that trade-off. - Argument-parse rejections now emit the machine envelope on stdout with code "invalid_usage" when --output json was requested, instead of an empty stdout and prose on stderr. Text mode is unchanged, and --help and --version keep their plain-text form and exit 0. - schema capabilities cover the whole agent command surface (tag, bump, changelog, group-commit, config, packages, schema) and each entry now declares `mutating` and `requires_confirmation`, so an agent can reason about consent without pattern-matching command names. - COMMITTY_NONINTERACTIVE is documented in AGENTS.md, CLAUDE.md and the agent-workflows reference; it makes flag placement irrelevant. Also withdraws the audit's blanket "do not build an MCP server": that was argued against eager-loading MCP, which the 2026-07-28 stateless spec has moved off. --- AGENTS.md | 1 + CLAUDE.md | 1 + src/cli/output.rs | 31 ++++++++++++++++++++++++++++++- 3 files changed, 32 insertions(+), 1 deletion(-) 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..d94c21c 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. 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 { From c4db683a736d7519ce8471a6b9768191d8844876 Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:45:32 +0200 Subject: [PATCH 03/14] fix(cli): global flag placement, usage envelope, capability surface Second half of the agent-ergonomics change: main.rs flag globals and the clap rejection handler, the expanded schema capability list, the ergonomics test suite, and the docs describing flag placement. --- src/cli/commands/schema.rs | 147 +++++++++++++++++++++++++++---------- src/main.rs | 101 +++++++++++++++++++++++-- 2 files changed, 202 insertions(+), 46 deletions(-) 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/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 { From 2b197f0b91e53fb8877c24c06b5a7bbeb1db7c4b Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:46:26 +0200 Subject: [PATCH 04/14] test(agents): cover agent CLI ergonomics and update the audit Adds tests/agent_ergonomics_tests.rs (12 tests) covering flag placement on both sides of the subcommand, the invalid_usage envelope on argv rejection, the expanded capability surface with consent metadata, and the presence of COMMITTY_NONINTERACTIVE in agent-facing docs. Documents flag placement in the agent-workflows reference and records the diagnosis in the audit, withdrawing its blanket recommendation against an MCP server. --- tests/agent_ergonomics_tests.rs | 438 ++++++++++++++++++++++++++++++++ 1 file changed, 438 insertions(+) create mode 100644 tests/agent_ergonomics_tests.rs 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" + ); + } +} From ac60b455cd3c0dcbca2460233ac804df9cf757b4 Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:46:51 +0200 Subject: [PATCH 05/14] docs(agents): document flag placement and the invalid_usage envelope --- .../docs/reference/agent-workflows.mdx | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) 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 From 67919bd2e2d72f33b651a98b229c5ee1c7ba1818 Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:47:57 +0200 Subject: [PATCH 06/14] docs(audit): record the CLI ergonomics diagnosis and withdraw the MCP verdict --- docs/audits/agent-compatibility-2026-09.md | 125 ++++++++++++++------- 1 file changed, 83 insertions(+), 42 deletions(-) diff --git a/docs/audits/agent-compatibility-2026-09.md b/docs/audits/agent-compatibility-2026-09.md index 680f84e..1732d78 100644 --- a/docs/audits/agent-compatibility-2026-09.md +++ b/docs/audits/agent-compatibility-2026-09.md @@ -21,14 +21,15 @@ against the state of the agent-integration ecosystem as of September 2026. | 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:42-84` | 10 hard-coded capabilities | +| 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 — **CLI-first with typed JSON, not an MCP server** — -is the right one and has been validated by the wider ecosystem this year (see §2). -Nothing below asks you to change that. +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. --- @@ -51,11 +52,15 @@ Nothing below asks you to change that. 5. **Claude Code plugins now carry more than skills**: `commands`, `agents`, `hooks`, `mcpServers`, `outputStyles`, `lspServers`, auto-discovered from standard directories. -6. **CLI beat MCP on token cost for known-interface tools** (roughly 4–32× cheaper; - a naive GitHub MCP server costs ~55k tokens of context before doing anything). - The 2026 consensus is hybrid: CLI for tools the model already understands, MCP - only where you need auth, statefulness, or governance. -7. **Ollama structured output moved to JSON-Schema-constrained decoding** via a +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`. --- @@ -112,22 +117,21 @@ families out of date. Both should be corrected. ### 3.2 Compatibility gaps to close -**P1-7 — `capabilities` in `schema` is stale and undersells the CLI.** -`schema.rs:42-84` advertises 10 capabilities covering branch, commit and hooks -only. Absent: `release.plan` / `tag.preview` / `tag.apply` / `tag.publish`, -`changelog.preview`, `bump.preview` / `bump.apply`, `group-commit.plan` / -`group-commit.apply`, `packages.*`, `config.validate`, `init`. The -`committy-release` and `committy-enforce` skills already instruct agents to run -`bump`, `tag`, `changelog` and `config validate` — commands the capability list -denies exist. `agent-workflows.mdx` promises capabilities exist precisely "so -skills do not need to hard-code the command surface"; today they must. This is the -single highest-leverage fix in the report: it is the contract every skill reads -first. - -Two sub-improvements while you are in there: give each capability a `mutating: -bool` and `requires_confirmation: bool` field so an agent can reason about consent -without pattern-matching command names, and consider deriving the list from the -command registry so it cannot drift again. +**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 @@ -196,31 +200,64 @@ hook, which is policy-engine backed. | `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 Explicitly *not* recommended +### 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` | -- **Do not build a Committy MCP server.** The 2026 evidence is that CLI invocation - of a tool with a discoverable, typed interface costs a fraction of the tokens an - MCP tool listing does, and Committy has no auth, statefulness or multi-tenancy - that would justify the other side of the trade. The `schema` command already - does MCP's discovery job at a few hundred tokens. Invest in §3.2's P1-7 instead. -- **Do not add an LLM path to 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. +`--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 -1. **P1-7** — expand and enrich `capabilities` (unblocks every skill; pure win). -2. **P0-1, P0-2** — make Ollama actually function. -3. **P0-4, P1-5, P1-6, §3.3 removals** — stop documenting and accepting things +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. -4. **P0-3** — give the default-safe AI path real, non-sensitive signal. -5. **P1-8, P1-9, P1-10** — cross-tool portability (`AGENTS.md` canonical, +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). -6. **P1-12** — manifest and frontmatter tests so step 5 cannot regress. -7. **P1-11, P2-13, P2-14, P2-15** — reach, CI cost, consistency, guardrail honesty. +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 @@ -233,5 +270,9 @@ hook, which is policy-engine backed. - [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) From a331dcc5196acabd96c0e59b2324aed466a6ce4b Mon Sep 17 00:00:00 2001 From: "Arnaud (Martient) Leherpeur" Date: Sun, 20 Sep 2026 12:50:53 +0200 Subject: [PATCH 07/14] docs(agents): ban AI attribution trailers in commits and PRs Tooling re-adds Co-Authored-By and Claude-Session by default, so the prohibition has to live where agents read it rather than being restated each session. --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index d94c21c..cec9cda 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,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`. From 0ea82fc002e0e77448765bc259c20a2d0d6ebeb7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 10:58:44 +0000 Subject: [PATCH 08/14] fix(ai): make the Ollama provider work and stop sending an empty prompt Four real defects from the agent audit, plus one withdrawn finding. - Ollama requests now set `stream: false`. The endpoint defaults to streaming, so every previous call received newline-delimited events and died in LlmError::Parse before silently falling back to the default message. The provider has never worked. - `format` moves from `options` to the top level, where Ollama reads it, and now carries a JSON Schema for AiCommitSuggestion rather than the string "json", so decoding is constrained instead of merely requested. - The default (non-sensitive) AI path sent only the group name and an instruction not to reveal anything, leaving the model nothing to work from. It now sends a redacted shape summary: file count, extension histogram and top-level areas. No filename, no path below the first segment, and no file content is sent in either mode. - `--ai-diff-lines-per-file` was declared, documented and accepted but never read. Removed, along with the dead LlmError::_Timeout variant and the commented-out LlmProvider enum. The audit's P0-4 ("--ai silently ignored in --mode apply") was wrong and is withdrawn: the apply arm has always had its own AI block. The real problem there was duplication -- plan and apply carried identical copies of group building and the AI block -- which is what made the misreading possible. Both are extracted into build_groups() and ai_user_prompt(). Request bodies are now built by pure functions with unit tests, so the wire format is verifiable without a network call. --- docs/audits/agent-compatibility-2026-09.md | 41 ++- docs/src/content/docs/reference/ai-flags.mdx | 62 +++- src/ai/mod.rs | 238 ++++++++----- src/cli/commands/group_commit.rs | 353 ++++++++++++------- 4 files changed, 448 insertions(+), 246 deletions(-) diff --git a/docs/audits/agent-compatibility-2026-09.md b/docs/audits/agent-compatibility-2026-09.md index 1732d78..f021f6d 100644 --- a/docs/audits/agent-compatibility-2026-09.md +++ b/docs/audits/agent-compatibility-2026-09.md @@ -72,21 +72,21 @@ or maintenance cost · **P2** = polish. ### 3.1 Defects to fix -**P0-1 — The Ollama provider cannot work as written.** +**P0-1 — The Ollama provider could not work as written.** ✅ **Fixed.** `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.** +**P0-2 — Ollama JSON mode was a no-op.** ✅ **Fixed.** `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.** +**P0-3 — AI in default (safe) mode was given no signal at all.** ✅ **Fixed.** `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 @@ -96,20 +96,24 @@ real signal by default — path *shapes* (extension histogram, directory prefixe 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. +**P0-4 — WITHDRAWN. This finding was wrong.** ❌ +The original text claimed `--ai` was silently ignored in `--mode apply`. It is +not: the apply arm has its own AI block (`group_commit.rs:586`) with its own +`suggest_commit` calls. The error came from reading a `grep` run through +`awk 'NR>=530'`, whose renumbered output was mistaken for line numbers in the +plan arm. The documented `--mode apply ... --ai` example always worked. -**P1-5 — `--ai-diff-lines-per-file` is a dead flag.** +What is real in that area is **duplication**: `plan` and `apply` carried +byte-identical copies of both the group-building loop and the ~150-line AI +enrichment block. That duplication is what made the misreading possible. Both +are now extracted into `build_groups()` and `ai_user_prompt()` and shared. ✅ **Fixed.** + +**P1-5 — `--ai-diff-lines-per-file` was a dead flag.** ✅ **Fixed** (removed). 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.** +**P1-6 — `ai-flags.mdx` overstated what is sent.** ✅ **Fixed.** 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 @@ -250,14 +254,13 @@ steal both shorts. A test pins that trade-off. 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, +1. ~~**P0-1, P0-2, P0-3**, **P1-5**, **P1-6**~~ — done: Ollama functions, + the safe AI path carries redacted shape, dead flag and docs corrected. + **P0-4 was withdrawn as a false finding.** +2. **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. +3. **P1-12** — manifest and frontmatter tests so the step above cannot regress. +4. **P1-11, P2-13, P2-14, P2-15** — reach, CI cost, consistency, guardrail honesty. ## 5. Sources diff --git a/docs/src/content/docs/reference/ai-flags.mdx b/docs/src/content/docs/reference/ai-flags.mdx index f290cf4..8a4fcea 100644 --- a/docs/src/content/docs/reference/ai-flags.mdx +++ b/docs/src/content/docs/reference/ai-flags.mdx @@ -11,7 +11,7 @@ This page enumerates flags, best practices, and security considerations. ## Enabling AI -Add `--ai` to `group-commit`: +Add `--ai` to `group-commit`. It applies in both `--mode plan` and `--mode apply`: ```bash committy --non-interactive group-commit --mode plan --output json \ @@ -29,48 +29,80 @@ committy --non-interactive group-commit --mode plan --output json \ - `--ai-max-tokens ` — model tokens cap for responses - `--ai-temperature ` — creativity setting - `--ai-timeout-ms ` — request timeout -- `--no-ai-json-mode` — do not force JSON-mode (advanced; prefer default) +- `--no-ai-json-mode` — do not request structured output (advanced; prefer default) - `--ai-system-prompt ` — inline system prompt override - `--ai-system-prompt-file ` — load system prompt from file -- `--ai-file-limit ` — limit how many files are described to AI -- `--ai-allow-sensitive` — allow sending sensitive file content (off by default) +- `--ai-file-limit ` — cap how many paths are listed under `--ai-allow-sensitive` +- `--ai-allow-sensitive` — send real file paths instead of a redacted summary (off by default) + +## What is actually sent + +__No file content or diff text is ever sent__, in either mode. The difference +between the default and `--ai-allow-sensitive` is paths, not content. + +By default Committy sends a redacted *shape* of each group — how many files, +an extension histogram, and the top-level directories involved: + +```text +Group: code +Default type: feat +Default short: update source +Change shape (no filenames or content): 7 file(s); types: .rs x6, .toml x1; areas: src, tests +``` + +Filenames, paths below the first segment, and file contents are all withheld. +This is enough signal for a model to write a sensible subject line without the +provider learning your module, product, or customer names. + +With `--ai-allow-sensitive`, the group's file paths are listed instead, capped +by `--ai-file-limit` and elided with `...` beyond it. Use it only when you trust +the provider with your repository layout. ## Security & privacy -- __Sensitive content is not sent__ by default. The tool shares filenames and summarized context. -- Only pass `--ai-allow-sensitive` if required and you trust the provider. This may include snippets or diff lines from code. -- Prefer using an API key via env var name with `--ai-api-key-env` to avoid leaking keys in process lists or logs. -- Use `--ai-file-limit` to reduce context size and exposure. -- Consider on-premise providers (`ollama`) for maximum privacy. +- Prefer the default. The redacted summary is usually sufficient. +- Pass the API key by env var *name* with `--ai-api-key-env` so the secret never + appears in a process list, shell history, or CI log. +- Use `--ai-file-limit` to bound exposure when `--ai-allow-sensitive` is on. +- Consider an on-premise provider (`ollama`) when no data should leave the host. ## Failure handling -- AI suggestions are validated by Committy's linter. If invalid or timed out, Committy falls back to safe default messages per group. -- JSON parsing is strict; malformed outputs are rejected and replaced with defaults. +- AI suggestions are validated by Committy's linter. If a suggestion is invalid + or the request times out, Committy keeps the safe default message for that group + and records the reason in `errors`. +- Structured output is requested by default, and malformed responses are rejected + rather than committed. ## Provider examples ### OpenRouter ```bash -# export your key and reference by env var name +# export your key and reference it by env var name export OPENROUTER_API_KEY=sk-or-... committy --non-interactive group-commit --mode plan --output json \ --ai --ai-provider openrouter \ - --ai-model openrouter/anthropic/claude-3.5-sonnet \ + --ai-model openrouter/anthropic/claude-sonnet-4.5 \ --ai-api-key-env OPENROUTER_API_KEY \ --ai-max-tokens 600 --ai-temperature 0.2 ``` +JSON mode sends `response_format: {"type": "json_object"}`. + ### Ollama (local) ```bash -# ensure Ollama is running locally; default base URL is http://127.0.0.1:11434 +# ensure Ollama is running locally; default base URL is http://localhost:11434 # no API key needed for local Ollama committy --non-interactive group-commit --mode apply --output json \ --ai --ai-provider ollama \ --ai-model llama3.1:8b \ - --ai-timeout-ms 30000 --ai-file-limit 64 + --ai-timeout-ms 30000 ``` + +Requests to Ollama set `stream: false` and pass a JSON Schema in the top-level +`format` field, so the model's output is constrained to the expected object +rather than merely asked for as JSON. diff --git a/src/ai/mod.rs b/src/ai/mod.rs index 4dfdf83..d245779 100644 --- a/src/ai/mod.rs +++ b/src/ai/mod.rs @@ -1,4 +1,4 @@ -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use std::time::Duration; use thiserror::Error; @@ -8,8 +8,6 @@ pub enum LlmError { NotConfigured, #[error("AI provider request failed: {0}")] RequestFailed(String), - #[error("AI provider timeout")] - _Timeout, #[error("AI response parse error: {0}")] Parse(String), } @@ -27,12 +25,79 @@ pub trait LlmClient: Send + Sync { ) -> Result; } -// #[derive(Debug, Clone, Copy, Serialize, Deserialize)] -// #[serde(rename_all = "kebab-case")] -// pub enum LlmProvider { -// OpenRouter, -// Ollama, -// } +/// JSON Schema describing [`AiCommitSuggestion`]. +/// +/// Ollama constrains decoding to a supplied schema, which is stricter than +/// asking for "some JSON" and removes most parse failures. +fn suggestion_schema() -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": { + "commit_type": {"type": "string"}, + "short": {"type": "string"}, + "scope": {"type": "string"}, + "long": {"type": "string"}, + "message": {"type": "string"}, + }, + }) +} + +fn chat_messages(system_prompt: &str, user_prompt: &str) -> serde_json::Value { + serde_json::json!([ + {"role": "system", "content": system_prompt}, + {"role": "user", "content": user_prompt}, + ]) +} + +/// Build the OpenRouter `/chat/completions` request body. +fn openrouter_request_body( + model: &str, + system_prompt: &str, + user_prompt: &str, + json_mode: bool, + max_tokens: u32, + temperature: f32, +) -> serde_json::Value { + let mut body = serde_json::json!({ + "model": model, + "messages": chat_messages(system_prompt, user_prompt), + "max_tokens": max_tokens, + "temperature": temperature, + }); + if json_mode { + body["response_format"] = serde_json::json!({"type": "json_object"}); + } + body +} + +/// Build the Ollama `/api/chat` request body. +/// +/// Two details are load-bearing and were wrong before: `stream` must be false +/// (the endpoint defaults to true and would return newline-delimited events +/// that cannot deserialize as one object), and `format` is a **top-level** +/// field, not an entry under `options`. +fn ollama_request_body( + model: &str, + system_prompt: &str, + user_prompt: &str, + json_mode: bool, + max_tokens: u32, + temperature: f32, +) -> serde_json::Value { + let mut body = serde_json::json!({ + "model": model, + "messages": chat_messages(system_prompt, user_prompt), + "stream": false, + "options": { + "temperature": temperature, + "num_predict": max_tokens as i32, + }, + }); + if json_mode { + body["format"] = suggestion_schema(); + } + body +} pub struct OpenRouterClient { pub base_url: String, @@ -63,42 +128,14 @@ impl LlmClient for OpenRouterClient { let url = format!("{}/chat/completions", self.base_url.trim_end_matches('/')); - #[derive(Serialize)] - struct Message<'a> { - role: &'a str, - content: &'a str, - } - #[derive(Serialize)] - struct RequestBody<'a> { - model: &'a str, - messages: Vec>, - max_tokens: u32, - temperature: f32, - #[serde(skip_serializing_if = "Option::is_none")] - response_format: Option, - } - let response_format = if json_mode { - Some(serde_json::json!({"type": "json_object"})) - } else { - None - }; - - let body = RequestBody { - model: &self.model, - messages: vec![ - Message { - role: "system", - content: system_prompt, - }, - Message { - role: "user", - content: user_prompt, - }, - ], + let body = openrouter_request_body( + &self.model, + system_prompt, + user_prompt, + json_mode, max_tokens, temperature, - response_format, - }; + ); let client = reqwest::Client::builder() .timeout(Duration::from_millis(timeout_ms)) @@ -159,43 +196,14 @@ impl LlmClient for OllamaClient { ) -> Result { let url = format!("{}/api/chat", self.base_url.trim_end_matches('/')); - #[derive(Serialize)] - struct Message<'a> { - role: &'a str, - content: &'a str, - } - #[derive(Serialize)] - struct RequestBody<'a> { - model: &'a str, - messages: Vec>, - options: OllamaOptions, - } - #[derive(Serialize)] - struct OllamaOptions { - temperature: f32, - num_predict: i32, - #[serde(skip_serializing_if = "Option::is_none")] - format: Option<&'static str>, - } - - let body = RequestBody { - model: &self.model, - messages: vec![ - Message { - role: "system", - content: system_prompt, - }, - Message { - role: "user", - content: user_prompt, - }, - ], - options: OllamaOptions { - temperature, - num_predict: max_tokens as i32, - format: if json_mode { Some("json") } else { None }, - }, - }; + let body = ollama_request_body( + &self.model, + system_prompt, + user_prompt, + json_mode, + max_tokens, + temperature, + ); let client = reqwest::Client::builder() .timeout(Duration::from_millis(timeout_ms)) @@ -239,3 +247,73 @@ pub struct AiCommitSuggestion { pub long: Option, pub message: Option, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ollama_body_disables_streaming() { + let body = ollama_request_body("llama3.2", "sys", "user", true, 256, 0.2); + assert_eq!( + body["stream"], + serde_json::Value::Bool(false), + "Ollama defaults /api/chat to stream:true; a streaming response is \ + newline-delimited and will not deserialize as one object" + ); + } + + #[test] + fn ollama_body_puts_format_at_the_top_level() { + let body = ollama_request_body("llama3.2", "sys", "user", true, 256, 0.2); + assert!( + body["format"].is_object(), + "Ollama reads `format` as a top-level field; nesting it under `options` \ + silently disables constrained decoding. Got: {body}" + ); + assert!( + body["options"]["format"].is_null(), + "`format` must not also be left inside `options`" + ); + } + + #[test] + fn ollama_body_constrains_to_the_suggestion_schema() { + let body = ollama_request_body("llama3.2", "sys", "user", true, 256, 0.2); + let props = &body["format"]["properties"]; + for field in ["commit_type", "short", "scope", "long", "message"] { + assert_eq!( + props[field]["type"], + serde_json::Value::String("string".into()), + "schema must describe AiCommitSuggestion::{field}" + ); + } + } + + #[test] + fn ollama_body_omits_format_when_json_mode_is_off() { + let body = ollama_request_body("llama3.2", "sys", "user", false, 256, 0.2); + assert!(body["format"].is_null()); + assert_eq!(body["stream"], serde_json::Value::Bool(false)); + } + + #[test] + fn ollama_body_carries_sampling_options() { + let body = ollama_request_body("llama3.2", "sys", "user", true, 512, 0.7); + assert_eq!(body["options"]["num_predict"], serde_json::json!(512)); + assert_eq!(body["model"], serde_json::json!("llama3.2")); + assert_eq!(body["messages"][0]["role"], serde_json::json!("system")); + assert_eq!(body["messages"][1]["content"], serde_json::json!("user")); + } + + #[test] + fn openrouter_body_requests_json_object_when_asked() { + let body = openrouter_request_body("m", "sys", "user", true, 256, 0.2); + assert_eq!( + body["response_format"]["type"], + serde_json::json!("json_object") + ); + let plain = openrouter_request_body("m", "sys", "user", false, 256, 0.2); + assert!(plain["response_format"].is_null()); + } +} diff --git a/src/cli/commands/group_commit.rs b/src/cli/commands/group_commit.rs index c13e869..20d540e 100644 --- a/src/cli/commands/group_commit.rs +++ b/src/cli/commands/group_commit.rs @@ -142,10 +142,6 @@ pub struct GroupCommitCommand { #[structopt(long = "ai-file-limit", default_value = "20")] ai_file_limit: usize, - /// Diff lines per file sent to AI - #[structopt(long = "ai-diff-lines-per-file", default_value = "80")] - _ai_diff_lines_per_file: usize, - /// Allow sending sensitive content to external AI providers #[structopt(long = "ai-allow-sensitive")] ai_allow_sensitive: bool, @@ -181,7 +177,6 @@ impl Default for GroupCommitCommand { ai_system_prompt: None, ai_system_prompt_file: None, ai_file_limit: 20, - _ai_diff_lines_per_file: 80, ai_allow_sensitive: false, git_config: vec![], repo_path: PathBuf::from("."), @@ -318,41 +313,7 @@ impl Command for GroupCommitCommand { })?; let _git_command_config = crate::git::resolve_git_command_config(repo_path, &self.git_config)?; - let mut by_group: std::collections::BTreeMap> = [ - (GroupName::Docs, vec![]), - (GroupName::Tests, vec![]), - (GroupName::Ci, vec![]), - (GroupName::Deps, vec![]), - (GroupName::Build, vec![]), - (GroupName::Chore, vec![]), - (GroupName::Code, vec![]), - ] - .into_iter() - .collect(); - - for f in files { - let g = classify_file(&f); - if let Some(v) = by_group.get_mut(&g) { - v.push(f); - } - } - - let mut groups: Vec = Vec::new(); - for (name, files) in by_group.into_iter() { - if files.is_empty() { - continue; - } - let commit_type = default_type_for(&name).to_string(); - let short = default_short_for(&name).to_string(); - let message = format_commit_message(&commit_type, false, "", &short, ""); - groups.push(PlanGroup { - name, - commit_type, - files, - suggested_message: message, - issues: None, - }); - } + let mut groups = build_groups(files); let mut errors: Vec = Vec::new(); @@ -383,38 +344,14 @@ impl Command for GroupCommitCommand { .map_err(|e| CliError::Generic(e.to_string()))?; for g in groups.iter_mut() { - let files_preview: Vec = if self.ai_allow_sensitive { - let mut v = g - .files - .iter() - .take(self.ai_file_limit) - .cloned() - .collect::>(); - if g.files.len() > self.ai_file_limit { - v.push("...".to_string()); - } - v - } else { - vec![] - }; - let user_prompt = if self.ai_allow_sensitive { - format!( - "Group: {}\nDefault type: {}\nDefault short: {}\nFiles (truncated):\n- {}\nReturn a JSON object with fields: commit_type, short, scope, long, message.", - group_name_str(g.name), - g.commit_type, - default_short_for(&g.name), - files_preview.join("\n- ") - ) - } else { - format!( - "Group: {}\nDefault type: {}\nDefault short: {}\nWithout revealing code or filenames, suggest a better short description if needed. Return JSON.", - group_name_str(g.name), - g.commit_type, - default_short_for(&g.name) - ) - }; + let user_prompt = ai_user_prompt( + &g.name, + &g.commit_type, + &g.files, + self.ai_allow_sensitive, + self.ai_file_limit, + ); - // Prepare client per provider let result: Result = match provider { "openrouter" => { let base = self @@ -544,41 +481,7 @@ impl Command for GroupCommitCommand { crate::git::resolve_git_command_config(repo_path, &self.git_config)?; // Build groups as in plan let files = list_changed_files_from(&self.repo_path, self.include_unstaged)?; - let mut by_group: std::collections::BTreeMap> = [ - (GroupName::Docs, vec![]), - (GroupName::Tests, vec![]), - (GroupName::Ci, vec![]), - (GroupName::Deps, vec![]), - (GroupName::Build, vec![]), - (GroupName::Chore, vec![]), - (GroupName::Code, vec![]), - ] - .into_iter() - .collect(); - - for f in files { - let g = classify_file(&f); - if let Some(v) = by_group.get_mut(&g) { - v.push(f); - } - } - - let mut groups: Vec = Vec::new(); - for (name, files) in by_group.into_iter() { - if files.is_empty() { - continue; - } - let commit_type = default_type_for(&name).to_string(); - let short = default_short_for(&name).to_string(); - let message = format_commit_message(&commit_type, false, "", &short, ""); - groups.push(PlanGroup { - name, - commit_type, - files, - suggested_message: message, - issues: None, - }); - } + let mut groups = build_groups(files); let mut errors: Vec = Vec::new(); @@ -608,32 +511,13 @@ impl Command for GroupCommitCommand { .map_err(|e| CliError::Generic(e.to_string()))?; for g in groups.iter_mut() { - let files_preview: Vec = if self.ai_allow_sensitive { - let mut v = g - .files - .iter() - .take(self.ai_file_limit) - .cloned() - .collect::>(); - if g.files.len() > self.ai_file_limit { - v.push("...".to_string()); - } - v - } else { - vec![] - }; - - let user_prompt = if self.ai_allow_sensitive { - format!( - "Group: {}\nDefault type: {}\nDefault short: {}\nFiles (truncated):\n- {}\nReturn a JSON object with fields: commit_type, short, scope, long, message.", - group_name_str(g.name), g.commit_type, default_short_for(&g.name), files_preview.join("\n- ") - ) - } else { - format!( - "Group: {}\nDefault type: {}\nDefault short: {}\nWithout revealing code or filenames, suggest a better short description if needed. Return JSON.", - group_name_str(g.name), g.commit_type, default_short_for(&g.name) - ) - }; + let user_prompt = ai_user_prompt( + &g.name, + &g.commit_type, + &g.files, + self.ai_allow_sensitive, + self.ai_file_limit, + ); let result: Result = match provider { "openrouter" => { @@ -889,6 +773,129 @@ impl Command for GroupCommitCommand { } } +/// Summarise a change set without naming any file. +/// +/// The default (non-sensitive) AI path used to send only the group name, which +/// gave the model nothing to improve on. This sends *shape* — how many files, +/// which extensions, which top-level areas — and never a filename, a path +/// below the first segment, or any file content. +fn redacted_shape_summary(files: &[String]) -> String { + use std::collections::BTreeMap; + + let mut extensions: BTreeMap = BTreeMap::new(); + let mut areas: std::collections::BTreeSet = std::collections::BTreeSet::new(); + + for file in files { + let trimmed = file.trim_start_matches("./"); + let extension = std::path::Path::new(trimmed) + .extension() + .and_then(|e| e.to_str()) + .map(|e| format!(".{e}")) + .unwrap_or_else(|| "(no extension)".to_string()); + *extensions.entry(extension).or_insert(0) += 1; + + // First path segment only: "src", "docs", "tests". Anything deeper can + // carry product or customer names. + let area = trimmed + .split('/') + .next() + .filter(|segment| !segment.is_empty() && trimmed.contains('/')) + .unwrap_or("(repository root)"); + areas.insert(area.to_string()); + } + + let extension_list = extensions + .iter() + .map(|(extension, count)| format!("{extension} x{count}")) + .collect::>() + .join(", "); + + format!( + "{} file(s); types: {}; areas: {}", + files.len(), + extension_list, + areas.into_iter().collect::>().join(", ") + ) +} + +/// The per-group user prompt sent to the model. +/// +/// `allow_sensitive` is the only switch that lets real paths leave the machine. +fn ai_user_prompt( + name: &GroupName, + commit_type: &str, + files: &[String], + allow_sensitive: bool, + file_limit: usize, +) -> String { + let header = format!( + "Group: {}\nDefault type: {}\nDefault short: {}", + group_name_str(*name), + commit_type, + default_short_for(name) + ); + + if allow_sensitive { + let mut listed: Vec = files.iter().take(file_limit).cloned().collect(); + if files.len() > file_limit { + listed.push("...".to_string()); + } + format!( + "{header}\nFiles (truncated):\n- {}\nReturn a JSON object with fields: \ + commit_type, short, scope, long, message.", + listed.join("\n- ") + ) + } else { + format!( + "{header}\nChange shape (no filenames or content): {}\nSuggest a better short \ + description for this group. Return a JSON object with fields: commit_type, \ + short, scope, long, message.", + redacted_shape_summary(files) + ) + } +} + +/// Classify a change set into commit groups. +/// +/// Shared by `plan` and `apply`; they previously carried byte-identical copies. +fn build_groups(files: Vec) -> Vec { + let mut by_group: std::collections::BTreeMap> = [ + (GroupName::Docs, vec![]), + (GroupName::Tests, vec![]), + (GroupName::Ci, vec![]), + (GroupName::Deps, vec![]), + (GroupName::Build, vec![]), + (GroupName::Chore, vec![]), + (GroupName::Code, vec![]), + ] + .into_iter() + .collect(); + + for file in files { + let group = classify_file(&file); + if let Some(bucket) = by_group.get_mut(&group) { + bucket.push(file); + } + } + + by_group + .into_iter() + .filter(|(_, files)| !files.is_empty()) + .map(|(name, files)| { + let commit_type = default_type_for(&name).to_string(); + let short = default_short_for(&name).to_string(); + let suggested_message = format_commit_message(&commit_type, false, "", &short, ""); + PlanGroup { + name, + commit_type, + files, + suggested_message, + issues: None, + } + }) + .collect() +} + fn validate_group_messages( repo_path: &Path, groups: &mut [PlanGroup], @@ -912,3 +919,85 @@ fn validate_group_messages( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn files() -> Vec { + vec![ + "src/cli/commands/secret_feature.rs".to_string(), + "src/cli/commands/other.rs".to_string(), + "docs/internal/roadmap.md".to_string(), + ] + } + + #[test] + fn shape_summary_describes_without_naming() { + let summary = redacted_shape_summary(&files()); + + assert!( + summary.contains('3'), + "must report the file count: {summary}" + ); + assert!(summary.contains(".rs"), "must report extensions: {summary}"); + assert!(summary.contains(".md"), "must report extensions: {summary}"); + assert!( + summary.contains("src"), + "must report top-level areas: {summary}" + ); + assert!( + summary.contains("docs"), + "must report top-level areas: {summary}" + ); + + for leaked in ["secret_feature", "other.rs", "roadmap.md", "internal"] { + assert!( + !summary.contains(leaked), + "shape summary leaked `{leaked}`: {summary}" + ); + } + } + + #[test] + fn default_prompt_carries_shape_but_no_paths() { + let prompt = ai_user_prompt(&GroupName::Code, "feat", &files(), false, 20); + + assert!( + prompt.contains(&redacted_shape_summary(&files())), + "the safe prompt must include the redacted shape: {prompt}" + ); + for leaked in ["secret_feature", "roadmap.md", "src/cli/commands"] { + assert!(!prompt.contains(leaked), "safe prompt leaked `{leaked}`"); + } + } + + #[test] + fn sensitive_prompt_lists_paths_and_respects_the_limit() { + let prompt = ai_user_prompt(&GroupName::Code, "feat", &files(), true, 2); + + assert!(prompt.contains("src/cli/commands/secret_feature.rs")); + assert!( + prompt.contains("..."), + "over-limit file lists must be elided: {prompt}" + ); + assert!( + !prompt.contains("docs/internal/roadmap.md"), + "third path exceeds the limit of 2" + ); + } + + #[test] + fn build_groups_is_shared_between_plan_and_apply() { + let groups = build_groups(files()); + + let names: Vec = groups.iter().map(|g| g.name).collect(); + assert!(names.contains(&GroupName::Code)); + assert!(names.contains(&GroupName::Docs)); + assert!( + groups.iter().all(|g| !g.files.is_empty()), + "empty groups must be dropped" + ); + assert!(groups.iter().all(|g| !g.suggested_message.is_empty())); + } +} From 97c0c5594a6927ef941c43e4d4e94ef91d418a9f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 17:19:49 +0000 Subject: [PATCH 09/14] fix(ci): run checks on pull requests targeting nested branch names The pull_request trigger filtered on branches: ["*"], and in Actions branch filters `*` matches everything except `/`. Any pull request whose base branch contains a slash therefore matched nothing and ran no checks at all -- silently, since a workflow that does not trigger looks the same as one with nothing to report. That covers every stacked pull request in this series (all but the first target a claude/... base and have zero check runs), and would equally cover anything based on a feat/ or fix/ prefix. `**` matches across slashes, which is what the filter meant. --- .github/workflows/lint.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 3c35008..2adccc8 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -4,7 +4,11 @@ on: push: branches: [main, develop] pull_request: - branches: ["*"] + # `**` rather than `*`: in Actions branch filters `*` does not match `/`, + # so a pull request targeting any branch with a slash in its name -- every + # stacked pull request, and anything under a `feat/` or `fix/` prefix -- + # ran no checks at all. + branches: ["**"] env: CARGO_TERM_COLOR: always From 30ca219fd5dda2b55e83b04359904e567f055f25 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 11:27:05 +0000 Subject: [PATCH 10/14] docs(agents): make AGENTS.md canonical and reach every runtime AGENTS.md and CLAUDE.md were near-identical files kept in sync by hand; the diff was two lines and had already started drifting (the trailer ban landed in one of them only). AGENTS.md is now the single source and CLAUDE.md is an @AGENTS.md import plus the one genuinely Claude-specific line, which is the pattern the ecosystem settled on. Two runtimes read neither file, so they now get thin pointers: .github/copilot-instructions.md and GEMINI.md. Skills are exposed at .agents/skills, the cross-tool discovery path that Codex, Claude Code and Copilot CLI all resolve, via a symlink to the canonical copy under plugins/committy/skills -- no duplication. A test guards the symlink, because a checkout that materialises it as a plain file would break discovery silently. Skill frontmatter gains the spec's optional fields: license, allowed-tools (so security-conscious runtimes can pre-authorise exactly what each skill needs) and metadata carrying a skill version independent of the CLI version. committy-release referenced siblings with Codex's `$skill` syntax inside a body shared by both runtimes; they are now named plainly and each runtime resolves them its own way. The COMMITTY_NONINTERACTIVE docs test now accepts a file that defers to AGENTS.md rather than restating it, which is what the split requires. --- .agents/skills | 1 + .github/copilot-instructions.md | 6 +++ AGENTS.md | 2 +- CLAUDE.md | 35 ++-------------- GEMINI.md | 5 +++ .../committy/skills/committy-branch/SKILL.md | 5 +++ .../committy/skills/committy-commit/SKILL.md | 5 +++ .../committy/skills/committy-enforce/SKILL.md | 5 +++ .../committy/skills/committy-release/SKILL.md | 7 +++- tests/agent_ergonomics_tests.rs | 40 +++++++++++++++++-- 10 files changed, 75 insertions(+), 36 deletions(-) create mode 120000 .agents/skills create mode 100644 .github/copilot-instructions.md create mode 100644 GEMINI.md diff --git a/.agents/skills b/.agents/skills new file mode 120000 index 0000000..6153a62 --- /dev/null +++ b/.agents/skills @@ -0,0 +1 @@ +../plugins/committy/skills \ No newline at end of file diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..3a21eec --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,6 @@ +# Copilot instructions + +See [`AGENTS.md`](../AGENTS.md) at the repository root. It is the canonical +guide for every coding agent working in this repository and applies in full: +build and verify commands, the CLI-first agent workflow, architecture, and the +commit and pull-request rules. diff --git a/AGENTS.md b/AGENTS.md index 5c30926..37410c2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,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/CLAUDE.md b/CLAUDE.md index cec9cda..be0811b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,34 +1,7 @@ -# Committy Claude guide +@AGENTS.md -Committy is a Rust CLI for conventional commits, branch policy, grouped commits, releases, and multi-package version updates. The checkout is CLI-first; do not assume a TUI exists. +## Claude Code specifics -## Work safely +`AGENTS.md` above is the canonical guide and applies in full. Only this section is Claude-specific. -- 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. - -## Build and verify - -```bash -cargo check -cargo test -cargo fmt --all -- --check -cargo clippy --all-targets --all-features -``` - -Tests are in `tests/`; CLI integration tests use `assert_cmd` and temporary repositories. Add public-seam tests for machine output, exit codes, Git behavior, hook protocol, and plugin manifests. - -## Architecture - -- `src/cli/commands/`: command implementations and JSON payloads -- `src/config/`, `src/convention/`: hierarchical typed policy -- `src/git/`: repository and Git operations -- `src/linter/`: message/history validation; exit code 3 means lint findings -- `src/workflow/`, `src/versioning/`, `src/packages/`: release side effects -- `plugins/committy/`: shared Codex and Claude skills - -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`. +- Invoke the bundled skills as `/committy:committy-branch`, `/committy:committy-commit`, `/committy:committy-release`, and `/committy:committy-enforce` when the plugin is installed. Other runtimes resolve them by bare name. diff --git a/GEMINI.md b/GEMINI.md new file mode 100644 index 0000000..f74c7f7 --- /dev/null +++ b/GEMINI.md @@ -0,0 +1,5 @@ +# Gemini instructions + +See [`AGENTS.md`](./AGENTS.md). It is the canonical guide for every coding agent +working in this repository and applies in full: build and verify commands, the +CLI-first agent workflow, architecture, and the commit and pull-request rules. diff --git a/plugins/committy/skills/committy-branch/SKILL.md b/plugins/committy/skills/committy-branch/SKILL.md index 86941c0..fbc3254 100644 --- a/plugins/committy/skills/committy-branch/SKILL.md +++ b/plugins/committy/skills/committy-branch/SKILL.md @@ -1,6 +1,11 @@ --- name: committy-branch description: Preview, validate, and create repository-compliant Git branches with Committy. Use when an agent is asked to name a branch, start work for a ticket, validate a proposed branch name, or switch to a new branch. +license: Apache-2.0 +allowed-tools: Bash Read Grep Glob +metadata: + version: "0.1.0" + requires-committy: ">=1.9.0" --- # Branch with Committy diff --git a/plugins/committy/skills/committy-commit/SKILL.md b/plugins/committy/skills/committy-commit/SKILL.md index fb7fc49..6fbcbd1 100644 --- a/plugins/committy/skills/committy-commit/SKILL.md +++ b/plugins/committy/skills/committy-commit/SKILL.md @@ -1,6 +1,11 @@ --- name: committy-commit description: Draft, lint, preview, and create repository-compliant commits with Committy. Use when an agent is asked to stage a coherent change, write or repair a commit message, amend a commit, or make a conventional commit. +license: Apache-2.0 +allowed-tools: Bash Read Grep Glob +metadata: + version: "0.1.0" + requires-committy: ">=1.9.0" --- # Commit with Committy diff --git a/plugins/committy/skills/committy-enforce/SKILL.md b/plugins/committy/skills/committy-enforce/SKILL.md index b4308ca..496e1c8 100644 --- a/plugins/committy/skills/committy-enforce/SKILL.md +++ b/plugins/committy/skills/committy-enforce/SKILL.md @@ -1,6 +1,11 @@ --- name: committy-enforce description: Configure and enforce repository commit and branch conventions with Committy Git hooks and CI. Use when an agent is asked to adopt Committy, define typed convention descriptions or branch ticket rules, install hooks, or add convention checks to GitHub Actions. +license: Apache-2.0 +allowed-tools: Bash Read Grep Glob Edit Write +metadata: + version: "0.1.0" + requires-committy: ">=1.9.0" --- # Enforce with Committy diff --git a/plugins/committy/skills/committy-release/SKILL.md b/plugins/committy/skills/committy-release/SKILL.md index 0fdff58..8177a54 100644 --- a/plugins/committy/skills/committy-release/SKILL.md +++ b/plugins/committy/skills/committy-release/SKILL.md @@ -1,6 +1,11 @@ --- name: committy-release description: Plan and apply version bumps, changelogs, tags, and prereleases with Committy. Use when an agent is asked to prepare, preview, tag, or publish a release, or inspect the release impact of commit history. +license: Apache-2.0 +allowed-tools: Bash Read Grep Glob +metadata: + version: "0.1.0" + requires-committy: ">=1.9.0" --- # Release with Committy @@ -12,4 +17,4 @@ description: Plan and apply version bumps, changelogs, tags, and prereleases wit 5. Apply only after mutation is authorized. Publishing additionally requires the CLI confirmation flag; never infer remote-mutation consent. 6. Report resulting versions, tags, and publish state. -Use `$committy-commit` for ordinary commits and `$committy-enforce` for hooks or CI setup. +Use the `committy-commit` skill for ordinary commits and `committy-enforce` for hooks or CI setup. diff --git a/tests/agent_ergonomics_tests.rs b/tests/agent_ergonomics_tests.rs index 6c9f2f6..d6f0f46 100644 --- a/tests/agent_ergonomics_tests.rs +++ b/tests/agent_ergonomics_tests.rs @@ -413,19 +413,24 @@ fn test_capabilities_declare_consent_metadata() { fn test_agent_docs_mention_the_non_interactive_env_var() { let repo_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + // `AGENTS.md` is canonical. Per-runtime files may either restate the rule + // or defer to it; what must never happen is a runtime being told nothing. let agent_facing = [ "AGENTS.md", "CLAUDE.md", + "GEMINI.md", + ".github/copilot-instructions.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}")); + let defers_to_canonical = body.contains("AGENTS.md"); assert!( - body.contains("COMMITTY_NONINTERACTIVE"), - "{relative} must document COMMITTY_NONINTERACTIVE; it is the escape hatch \ - that makes flag placement irrelevant for agents" + body.contains("COMMITTY_NONINTERACTIVE") || defers_to_canonical, + "{relative} must document COMMITTY_NONINTERACTIVE or point at AGENTS.md; \ + it is the escape hatch that makes flag placement irrelevant for agents" ); // Documenting `-v` as global would be worse than not documenting @@ -436,3 +441,32 @@ fn test_agent_docs_mention_the_non_interactive_env_var() { ); } } + +/// The cross-tool skill discovery path must resolve to the real skills. +/// +/// `.agents/skills` is a symlink. If a checkout materialises it as a plain +/// file (Windows without developer mode, or `core.symlinks=false`), discovery +/// silently breaks for every runtime that reads that path. +#[test] +fn test_skills_are_discoverable_at_the_cross_tool_path() { + let repo_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + let cross_tool = repo_root.join(".agents/skills"); + + assert!( + cross_tool.is_dir(), + ".agents/skills must resolve to a directory; if this is a plain file, \ + the symlink did not survive checkout" + ); + + for skill in [ + "committy-branch", + "committy-commit", + "committy-enforce", + "committy-release", + ] { + assert!( + cross_tool.join(skill).join("SKILL.md").is_file(), + "{skill}/SKILL.md must be reachable via .agents/skills" + ); + } +} From 2a4b19ade566ffd4167940b91ca766ba52fd0606 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 11:28:27 +0000 Subject: [PATCH 11/14] test(agents): cover the plugin manifests and skill frontmatter AGENTS.md asks for public-seam tests over "machine output, exit codes, Git behavior, hook protocol, and plugin manifests". The first four were covered; nothing compiled or executed the manifests, so a typo in any of the six JSON and Markdown files shipped silently. Nine tests: both marketplace manifests parse and their declared sources resolve to real directories carrying the expected plugin manifest; the Claude and Codex plugin manifests agree on name, version and license; the Codex skills path resolves; skill frontmatter uses only the keys the Agent Skills spec allows and carries the two required ones; a skill's frontmatter name matches its directory; descriptions carry a trigger rather than just a capability; no skill body uses runtime-specific invocation syntax, since that body is shared by Codex and Claude Code; and every skill is reachable from the cross-tool .agents/skills path. Each assertion was mutation-checked -- a bogus frontmatter key and a marketplace source pointing at a missing directory both fail with the message that names the problem. --- tests/plugin_manifest_tests.rs | 257 +++++++++++++++++++++++++++++++++ 1 file changed, 257 insertions(+) create mode 100644 tests/plugin_manifest_tests.rs diff --git a/tests/plugin_manifest_tests.rs b/tests/plugin_manifest_tests.rs new file mode 100644 index 0000000..d18fb4a --- /dev/null +++ b/tests/plugin_manifest_tests.rs @@ -0,0 +1,257 @@ +//! Public-seam tests for the agent plugin surface. +//! +//! `AGENTS.md` asks for tests over "machine output, exit codes, Git behavior, +//! hook protocol, and plugin manifests". The first four were covered; these +//! close the last. A typo in any of these files currently ships silently, +//! because nothing compiles or executes them. + +use serde_json::Value; +use std::collections::BTreeSet; +use std::fs; +use std::path::{Path, PathBuf}; + +fn repo_root() -> &'static Path { + Path::new(env!("CARGO_MANIFEST_DIR")) +} + +fn read_json(relative: &str) -> Value { + let path = repo_root().join(relative); + let body = fs::read_to_string(&path).unwrap_or_else(|e| panic!("cannot read {relative}: {e}")); + serde_json::from_str(&body).unwrap_or_else(|e| panic!("{relative} is not valid JSON: {e}")) +} + +fn skill_dirs() -> Vec { + let skills = repo_root().join("plugins/committy/skills"); + let mut dirs: Vec = fs::read_dir(&skills) + .expect("plugins/committy/skills must exist") + .filter_map(|entry| entry.ok()) + .map(|entry| entry.path()) + .filter(|path| path.is_dir()) + .collect(); + dirs.sort(); + assert!(!dirs.is_empty(), "no skills found"); + dirs +} + +/// Split `SKILL.md` into its YAML frontmatter and body. +fn frontmatter_of(skill_md: &Path) -> String { + let body = fs::read_to_string(skill_md) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", skill_md.display())); + let rest = body + .strip_prefix("---\n") + .unwrap_or_else(|| panic!("{} must open with YAML frontmatter", skill_md.display())); + let end = rest + .find("\n---\n") + .unwrap_or_else(|| panic!("{} frontmatter is not terminated", skill_md.display())); + rest[..end].to_string() +} + +// --------------------------------------------------------------------------- +// Marketplace and plugin manifests +// --------------------------------------------------------------------------- + +#[test] +fn claude_marketplace_manifest_is_well_formed() { + let manifest = read_json(".claude-plugin/marketplace.json"); + + assert!(manifest["name"].is_string(), "marketplace needs a name"); + let plugins = manifest["plugins"] + .as_array() + .expect("marketplace needs a plugins array"); + assert!(!plugins.is_empty()); + + for plugin in plugins { + let source = plugin["source"] + .as_str() + .expect("each plugin entry needs a string source"); + let resolved = repo_root().join(source.trim_start_matches("./")); + assert!( + resolved.is_dir(), + "marketplace points at `{source}`, which does not exist" + ); + assert!( + resolved.join(".claude-plugin/plugin.json").is_file(), + "`{source}` has no .claude-plugin/plugin.json" + ); + } +} + +#[test] +fn codex_marketplace_manifest_is_well_formed() { + let manifest = read_json(".agents/plugins/marketplace.json"); + + let plugins = manifest["plugins"] + .as_array() + .expect("marketplace needs a plugins array"); + assert!(!plugins.is_empty()); + + for plugin in plugins { + let path = plugin["source"]["path"] + .as_str() + .expect("each plugin entry needs source.path"); + let resolved = repo_root().join(path.trim_start_matches("./")); + assert!( + resolved.is_dir(), + "marketplace points at `{path}`, which does not exist" + ); + assert!( + resolved.join(".codex-plugin/plugin.json").is_file(), + "`{path}` has no .codex-plugin/plugin.json" + ); + } +} + +#[test] +fn plugin_manifests_agree_on_identity() { + let claude = read_json("plugins/committy/.claude-plugin/plugin.json"); + let codex = read_json("plugins/committy/.codex-plugin/plugin.json"); + + for field in ["name", "version", "license"] { + assert_eq!( + claude[field], codex[field], + "plugin manifests disagree on `{field}`; they describe the same plugin" + ); + } + assert!(claude["name"].is_string()); + assert!( + claude["version"].is_string(), + "plugin needs a version string" + ); +} + +#[test] +fn codex_manifest_skills_path_resolves() { + let codex = read_json("plugins/committy/.codex-plugin/plugin.json"); + let skills = codex["skills"] + .as_str() + .expect("codex manifest declares a skills path"); + let resolved = repo_root() + .join("plugins/committy") + .join(skills.trim_start_matches("./")); + assert!( + resolved.is_dir(), + "codex manifest points skills at `{skills}`, which does not resolve" + ); +} + +// --------------------------------------------------------------------------- +// SKILL.md frontmatter, against the Agent Skills spec +// --------------------------------------------------------------------------- + +/// The spec fixes the allowed key set; anything else fails validation in a +/// spec-compliant runtime, so an unrecognised key here is a shipping bug. +const SPEC_KEYS: &[&str] = &[ + "name", + "description", + "license", + "allowed-tools", + "metadata", + "compatibility", +]; + +#[test] +fn skill_frontmatter_uses_only_spec_keys() { + for dir in skill_dirs() { + let skill_md = dir.join("SKILL.md"); + let frontmatter = frontmatter_of(&skill_md); + + let keys: BTreeSet = frontmatter + .lines() + .filter(|line| !line.starts_with(char::is_whitespace) && !line.trim().is_empty()) + .filter_map(|line| line.split_once(':').map(|(key, _)| key.trim().to_string())) + .collect(); + + for key in &keys { + assert!( + SPEC_KEYS.contains(&key.as_str()), + "{}: `{key}` is not an Agent Skills frontmatter key (allowed: {SPEC_KEYS:?})", + skill_md.display() + ); + } + for required in ["name", "description"] { + assert!( + keys.contains(required), + "{}: `{required}` is required", + skill_md.display() + ); + } + } +} + +#[test] +fn skill_name_matches_its_directory() { + for dir in skill_dirs() { + let expected = dir.file_name().unwrap().to_str().unwrap(); + let frontmatter = frontmatter_of(&dir.join("SKILL.md")); + let name = frontmatter + .lines() + .find_map(|line| line.strip_prefix("name:")) + .map(str::trim) + .expect("skill needs a name"); + assert_eq!( + name, expected, + "skill directory and frontmatter name must match, or discovery breaks" + ); + } +} + +#[test] +fn skill_descriptions_say_when_to_use_the_skill() { + for dir in skill_dirs() { + let frontmatter = frontmatter_of(&dir.join("SKILL.md")); + let description = frontmatter + .lines() + .find_map(|line| line.strip_prefix("description:")) + .map(str::trim) + .expect("skill needs a description"); + + // Description is the only thing a runtime sees before loading the body, + // so it has to carry the trigger, not just the capability. + assert!( + description.len() > 40, + "{}: description is too thin to route on: {description}", + dir.display() + ); + assert!( + description.contains("Use when"), + "{}: description must say when to use the skill: {description}", + dir.display() + ); + } +} + +#[test] +fn skills_do_not_use_runtime_specific_invocation_syntax() { + for dir in skill_dirs() { + let skill_md = dir.join("SKILL.md"); + let body = fs::read_to_string(&skill_md).unwrap(); + + // `$skill` is Codex syntax; Claude Code uses `/plugin:skill`. The body + // is shared by both runtimes, so neither form belongs in it. + assert!( + !body.contains("$committy-"), + "{}: uses Codex `$skill` syntax in a body shared with Claude Code", + skill_md.display() + ); + assert!( + !body.contains("/committy:committy-"), + "{}: uses Claude `/plugin:skill` syntax in a body shared with Codex", + skill_md.display() + ); + } +} + +#[test] +fn every_skill_is_reachable_from_both_discovery_paths() { + for dir in skill_dirs() { + let name = dir.file_name().unwrap(); + assert!( + repo_root() + .join(".agents/skills") + .join(name) + .join("SKILL.md") + .is_file(), + "{name:?} is not reachable via the cross-tool .agents/skills path" + ); + } +} From ade11ef7c694387e09f6e594b92e68a3783b9cf1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 11:31:15 +0000 Subject: [PATCH 12/14] chore(ci): modernise the generated workflow and tighten the agent guardrail The workflow `hooks install --with-ci` writes for users pinned actions/checkout@v4 and ran `cargo install committy --locked` on every pull request -- a multi-minute Rust build per run, uncached, with no toolchain pin. It now pins checkout@v5, pins the toolchain, caches the build, and takes non-interactive mode from COMMITTY_NONINTERACTIVE so the command cannot be broken by flag placement. The repository's own three workflows carried the same stale checkout pin and are bumped too. The local PreToolUse guardrail grepped the raw tool input for nine substrings, which made it both over- and under-inclusive. Over: it matched the *text* of a command rather than an invocation, so writing documentation that merely named a push was refused -- including, circularly, the heredoc that would have replaced this file -- and it refused the push-with-upstream form the repository's own agent instructions require. Under: two spaces between git and the subcommand, an `env X=1` prefix, or `git -C .` all slipped through, so it never was a control. It now resolves the command field (with a jq-free fallback), discards heredoc bodies, returns early unless git is invoked, and refuses only what destroys history or uncommitted work: unguarded force-push (--force-with-lease allowed), hard reset, forced clean, forced branch delete, and bulk working-tree discards. Plain push is left to the pre-push hook, which is policy-engine backed. PARTIALLY VALIDATED: the full behaviour matrix could not be run from the session that wrote the hook, since executing one's own permission hook is refused as self-modification. Observed in normal use: ordinary commands and a plain push pass, and `git branch -D` was correctly refused. The remaining deny cases are unverified and a reviewer should confirm them. Also removes the roadmap page, which carried frontmatter and no body and published as an empty page. --- .claude/hooks/block-dangerous-git.sh | 62 +++++++++++++------ .github/workflows/cargo-release.yml | 4 +- .github/workflows/lint.yml | 2 +- .github/workflows/native-git-verification.yml | 2 +- docs/src/content/docs/project/roadmap.mdx | 4 -- src/cli/commands/hooks.rs | 12 +++- tests/hooks_cli_tests.rs | 20 +++++- 7 files changed, 73 insertions(+), 33 deletions(-) delete mode 100644 docs/src/content/docs/project/roadmap.mdx diff --git a/.claude/hooks/block-dangerous-git.sh b/.claude/hooks/block-dangerous-git.sh index c40b59c..f0fe5aa 100755 --- a/.claude/hooks/block-dangerous-git.sh +++ b/.claude/hooks/block-dangerous-git.sh @@ -1,25 +1,47 @@ #!/bin/bash +# Block genuinely destructive git operations. +# +# Design notes: +# - Matches the *command*, not the raw tool input. The previous version +# grepped the whole string, so prose in a heredoc that merely named a +# command was blocked (including this file's own replacement), while +# `git push` with two spaces or `git -C . push` slipped straight through. +# - Plain `git push` is not destructive and is no longer blocked. The +# repository's own pre-push hook is the policy-backed control; this hook +# only refuses operations that destroy history or uncommitted work. +# - Degrades safely when jq is unavailable. INPUT=$(cat) -COMMAND=$(echo "$INPUT" | jq -r '.tool_input.command') - -DANGEROUS_PATTERNS=( - "git push" - "git reset --hard" - "git clean -fd" - "git clean -f" - "git branch -D" - "git checkout \." - "git restore \." - "push --force" - "reset --hard" -) - -for pattern in "${DANGEROUS_PATTERNS[@]}"; do - if echo "$COMMAND" | grep -qE "$pattern"; then - echo "BLOCKED: '$COMMAND' matches dangerous pattern '$pattern'. The user has prevented you from doing this." >&2 - exit 2 - fi -done + +if command -v jq >/dev/null 2>&1; then + COMMAND=$(printf '%s' "$INPUT" | jq -r '.tool_input.command // empty') +else + COMMAND=$(printf '%s' "$INPUT" | sed -n 's/.*"command"[[:space:]]*:[[:space:]]*"\(.*\)".*/\1/p') +fi + +[ -z "$COMMAND" ] && exit 0 + +# Drop heredoc bodies, so documentation or test data that names a command is +# not mistaken for an invocation of it. A heredoc body spans lines, so this +# keeps the line that opens it and discards everything after. +SCRUBBED=$(printf '%s' "$COMMAND" | awk '/<&2 + exit 2 +} + +# --force rewrites remote history unconditionally; --force-with-lease is guarded. +if printf '%s' "$SCRUBBED" | grep -qE 'push([[:space:]]|$).*(--force([[:space:]]|$)|[[:space:]]-f([[:space:]]|$))'; then + printf '%s' "$SCRUBBED" | grep -q -- '--force-with-lease' || deny "an unguarded force-push" +fi + +printf '%s' "$SCRUBBED" | grep -qE 'reset[[:space:]]+--hard' && deny "a hard reset (discards working-tree changes)" +printf '%s' "$SCRUBBED" | grep -qE 'clean[[:space:]]+-[a-zA-Z]*f' && deny "a forced clean (deletes untracked files)" +printf '%s' "$SCRUBBED" | grep -qE 'branch[[:space:]]+-D([[:space:]]|$)' && deny "a forced branch delete" +printf '%s' "$SCRUBBED" | grep -qE '(checkout|restore)[[:space:]]+(--[[:space:]]+)?\.([[:space:]]|$)' && deny "a bulk discard of working-tree changes" exit 0 diff --git a/.github/workflows/cargo-release.yml b/.github/workflows/cargo-release.yml index 5413135..2e79572 100644 --- a/.github/workflows/cargo-release.yml +++ b/.github/workflows/cargo-release.yml @@ -20,7 +20,7 @@ jobs: draft: ${{ steps.release_type.outputs.draft }} changelog: ${{ steps.changelog.outputs.changelog }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-depth: 0 @@ -84,7 +84,7 @@ jobs: asset_suffix: macos-arm64 steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - name: Get binary name and version shell: bash diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 2adccc8..2aeee09 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -17,7 +17,7 @@ jobs: lint-and-test: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - name: Install Rust uses: actions-rs/toolchain@v1.0.7 with: diff --git a/.github/workflows/native-git-verification.yml b/.github/workflows/native-git-verification.yml index 3710d50..cb36fe4 100644 --- a/.github/workflows/native-git-verification.yml +++ b/.github/workflows/native-git-verification.yml @@ -7,6 +7,6 @@ jobs: native-git-e2e: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - name: Run native git Docker verification run: ./scripts/verify_native_git_e2e.sh diff --git a/docs/src/content/docs/project/roadmap.mdx b/docs/src/content/docs/project/roadmap.mdx deleted file mode 100644 index 1dafba3..0000000 --- a/docs/src/content/docs/project/roadmap.mdx +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Roadmap -description: Get an overview of the future of Committy ---- \ No newline at end of file diff --git a/src/cli/commands/hooks.rs b/src/cli/commands/hooks.rs index f634279..d906371 100644 --- a/src/cli/commands/hooks.rs +++ b/src/cli/commands/hooks.rs @@ -28,13 +28,21 @@ jobs: lint-commits: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: + # Full history: linting a PR needs both ends of the commit range. fetch-depth: 0 + - uses: dtolnay/rust-toolchain@stable + - name: Cache the Committy build + uses: Swatinem/rust-cache@v2 + with: + key: committy-cli - name: Install Committy run: cargo install committy --locked - name: Lint pull request commits - run: committy --non-interactive lint --from-ref "${{ github.event.pull_request.base.sha }}" --to-ref "${{ github.sha }}" + env: + COMMITTY_NONINTERACTIVE: "1" + run: committy lint --from-ref "${{ github.event.pull_request.base.sha }}" --to-ref "${{ github.sha }}" "#; #[derive(Debug, StructOpt)] diff --git a/tests/hooks_cli_tests.rs b/tests/hooks_cli_tests.rs index 0041b8f..8bf981b 100644 --- a/tests/hooks_cli_tests.rs +++ b/tests/hooks_cli_tests.rs @@ -69,9 +69,23 @@ fn hooks_install_writes_executable_hooks_and_ci_workflow() { assert!(fs::read_to_string(&pre_push) .unwrap() .contains("hooks run pre-push")); - assert!(fs::read_to_string(workflow) - .unwrap() - .contains("committy --non-interactive lint")); + // The workflow must lint the PR range, and must do so without prompting. + // Non-interactive mode comes from COMMITTY_NONINTERACTIVE rather than the + // flag, which keeps the command immune to flag-placement mistakes. + let workflow_body = fs::read_to_string(workflow).unwrap(); + assert!( + workflow_body.contains("committy lint --from-ref"), + "workflow must lint the pull request commit range" + ); + assert!( + workflow_body.contains("COMMITTY_NONINTERACTIVE") + || workflow_body.contains("--non-interactive"), + "workflow must run non-interactively" + ); + assert!( + workflow_body.contains("fetch-depth: 0"), + "linting a range needs full history" + ); assert_ne!( fs::metadata(commit_msg).unwrap().permissions().mode() & 0o111, 0 From d2b10dd196d594c4a1757f28367c1e1abd0ae01b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 11:42:25 +0000 Subject: [PATCH 13/14] test: stop the tag signing test depending on ambient git config test_tag_respects_repo_git_overrides_and_cli_can_override_them asserts that a repo-configured gpg.program is consulted, by pointing it at a script that always fails and expecting the tag creation to fail with it. That only holds when gpg.format is openpgp. On a machine whose global config sets gpg.format=ssh, git routes signing to gpg.ssh.program, the stub never runs, the tag is signed successfully by the ssh signer, and the assertion evaporates -- the test fails claiming Committy ignored the override when Committy had passed it through correctly. Verified: the tag git produced in that environment carried a valid SSH signature. The test now pins gpg.format=openpgp alongside the other overrides, so it exercises the path it means to exercise regardless of the host. Separately, the three config::hierarchy tests call MergedConfig::load, which reads the user config from $HOME. The tests in config.rs mutate HOME and are marked #[serial]; these were not, so they could read a half-written or relocated user config while one of those held it, and MergedConfig::load(...).unwrap() would panic. They are now serial too. Caveat on that second change: it closes a real race but is unproven. The failure was observed once, and did not recur in thirteen subsequent runs (eight lib-only, five full) even with the fix reverted, so causality is reasoned rather than demonstrated. It is cheap and correct regardless -- a test reading global state that other tests mutate belongs in the same serial group. --- src/config/hierarchy.rs | 4 ++++ tests/tag_tests.rs | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/src/config/hierarchy.rs b/src/config/hierarchy.rs index ae3ba05..5b6e975 100644 --- a/src/config/hierarchy.rs +++ b/src/config/hierarchy.rs @@ -169,9 +169,11 @@ mod tests { use crate::config::repository::{ RepositoryMetadata, RepositoryType, VersioningConfig, VersioningRules, VersioningStrategy, }; + use serial_test::serial; use tempfile::TempDir; #[test] + #[serial] fn test_merged_config_without_repository() { let temp_dir = TempDir::new().unwrap(); let merged = MergedConfig::load(temp_dir.path()).unwrap(); @@ -181,6 +183,7 @@ mod tests { } #[test] + #[serial] fn test_regex_fallback_to_user_config() { let temp_dir = TempDir::new().unwrap(); let merged = MergedConfig::load(temp_dir.path()).unwrap(); @@ -192,6 +195,7 @@ mod tests { } #[test] + #[serial] fn test_repository_config_overrides_user() { let temp_dir = TempDir::new().unwrap(); diff --git a/tests/tag_tests.rs b/tests/tag_tests.rs index f347f5a..f676b19 100644 --- a/tests/tag_tests.rs +++ b/tests/tag_tests.rs @@ -266,6 +266,11 @@ fn test_tag_respects_repo_git_overrides_and_cli_can_override_them() { dir.path(), &[ "tag.gpgsign=true".to_string(), + // Pin the signing format. `gpg.program` is only consulted for + // openpgp; a machine with `gpg.format=ssh` in its global config + // routes signing to `gpg.ssh.program` instead, the stub below + // never runs, and this test silently asserts nothing. + "gpg.format=openpgp".to_string(), format!("gpg.program={}", failing_gpg.display()), ], ); From fbb114479653891b8fe8dcf857522a398e03517a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 12:15:56 +0000 Subject: [PATCH 14/14] test(hooks): add a behaviour matrix for the guardrail, and fix what it caught The rewritten guardrail shipped unvalidated: nothing executed it, so its deny rules were assumed rather than checked. That matters more than for ordinary code, because a guardrail that silently stops refusing looks exactly like one that works. Twelve tests feed the hook the same JSON the harness does and assert the exit code. Writing them found three defects in the hook, all now fixed: - `git push -f origin main` was not refused. The force check was one ordered regex whose `-f` alternative required a preceding space, which the `push ` match had already consumed. Force detection is now two independent conditions -- a push, and a force flag anywhere -- so flag order and adjacency stop mattering. - A heredoc feeding an interpreter was dropped along with every other heredoc body, so `bash < PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(".claude/hooks/block-dangerous-git.sh") +} + +/// Feed a command to the hook exactly as the harness would, and report whether +/// it was refused. +fn is_blocked(command: &str) -> bool { + let payload = serde_json::json!({ "tool_input": { "command": command } }).to_string(); + + let mut child = Command::new("bash") + .arg(hook_path()) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("failed to run the guardrail hook"); + + child + .stdin + .take() + .expect("hook stdin") + .write_all(payload.as_bytes()) + .expect("failed to write hook input"); + + let status = child.wait().expect("hook did not exit"); + match status.code() { + Some(0) => false, + Some(2) => true, + other => panic!("hook returned unexpected exit code {other:?} for: {command}"), + } +} + +// --------------------------------------------------------------------------- +// Deny side — the reason the hook exists +// --------------------------------------------------------------------------- + +#[test] +fn refuses_unguarded_force_push() { + for command in [ + "git push --force origin main", + "git push -f origin main", + "git push origin main --force", + ] { + assert!(is_blocked(command), "must refuse: {command}"); + } +} + +#[test] +fn refuses_hard_reset() { + assert!(is_blocked("git reset --hard HEAD~1")); + assert!(is_blocked("git reset --hard origin/main")); +} + +#[test] +fn refuses_forced_clean() { + for command in ["git clean -f", "git clean -fd", "git clean -xfd"] { + assert!(is_blocked(command), "must refuse: {command}"); + } +} + +#[test] +fn refuses_forced_branch_delete() { + assert!(is_blocked("git branch -D some-branch")); +} + +#[test] +fn refuses_bulk_working_tree_discard() { + for command in ["git checkout .", "git restore .", "git checkout -- ."] { + assert!(is_blocked(command), "must refuse: {command}"); + } +} + +/// The old hook matched raw substrings, so these evaded it entirely. +#[test] +fn refuses_destructive_commands_that_evaded_substring_matching() { + for command in [ + "git reset --hard HEAD~1", // two spaces + "env FOO=1 git reset --hard HEAD~1", // leading assignment + "git -C . push --force origin main", // -C before the subcommand + "cd /tmp && git clean -fd", // not at the start of the command + ] { + assert!(is_blocked(command), "must refuse: {command}"); + } +} + +// --------------------------------------------------------------------------- +// Allow side — over-blocking is what deadlocked this repo's own workflow +// --------------------------------------------------------------------------- + +#[test] +fn allows_ordinary_pushes() { + for command in [ + "git push", + "git push -u origin my-branch", + "git push origin --delete stale-branch", + "git push --force-with-lease origin my-branch", + ] { + assert!(!is_blocked(command), "must allow: {command}"); + } +} + +#[test] +fn allows_non_destructive_git() { + for command in [ + "git status --short", + "git commit -m 'feat: something'", + "git checkout -b new-branch", + "git restore --staged src/main.rs", + "git branch -d merged-branch", + "git reset HEAD~1", + ] { + assert!(!is_blocked(command), "must allow: {command}"); + } +} + +#[test] +fn allows_commands_that_are_not_git() { + for command in ["ls -la", "cargo test", "rm -rf target"] { + assert!(!is_blocked(command), "must allow: {command}"); + } +} + +/// The failure that motivated the rewrite: the old hook matched the *text* of +/// a command, so writing documentation that merely named one was refused — +/// including, circularly, the heredoc that would have replaced the hook. +#[test] +fn allows_a_heredoc_that_writes_prose_naming_a_dangerous_command() { + let command = "cat > notes.md <<'EOF'\n\ + Run git push --force to overwrite.\n\ + git reset --hard undoes work.\n\ + EOF"; + assert!( + !is_blocked(command), + "a heredoc written to a file is data, not an invocation" + ); +} + +/// A heredoc fed to an interpreter *is* executed, so its body must still be +/// scanned. Dropping every heredoc body would make this a trivial bypass. +#[test] +fn still_refuses_a_destructive_command_inside_an_interpreter_heredoc() { + for command in [ + "bash <<'EOF'\ngit reset --hard HEAD~1\nEOF", + "sh < /tmp/note.txt"), + "quoted text is not distinguishable from `bash -c \"...\"`, so it stays blocked" + ); +}