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/.claude/hooks/block-dangerous-git.sh b/.claude/hooks/block-dangerous-git.sh index c40b59c..5ca2568 100755 --- a/.claude/hooks/block-dangerous-git.sh +++ b/.claude/hooks/block-dangerous-git.sh @@ -1,25 +1,61 @@ #!/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. +# +# Exception: if the heredoc feeds an interpreter, the body IS executed, and +# dropping it would be a bypass (`bash <&2 + exit 2 +} + +# --force rewrites remote history unconditionally; --force-with-lease is guarded. +# Checked as two independent conditions rather than one ordered regex: the flag +# may precede or follow the remote, and `push -f` leaves no space for a pattern +# that expects one before the flag. +if printf '%s' "$SCRUBBED" | grep -qE '(^|[;&|[:space:]])push([[:space:]]|$)' && + printf '%s' "$SCRUBBED" | grep -qE '(^|[[:space:]])(--force|-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/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/.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 3c35008..2aeee09 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 @@ -13,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/AGENTS.md b/AGENTS.md index 1dd5276..37410c2 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. @@ -30,4 +31,4 @@ Tests are in `tests/`; CLI integration tests use `assert_cmd` and temporary repo - `src/workflow/`, `src/versioning/`, `src/packages/`: release side effects - `plugins/committy/`: shared Codex and Claude skills -PRs target `develop`. Commits follow Conventional Commits. The canonical agent reference is `docs/src/content/docs/reference/agent-workflows.mdx`. +PRs target `develop`. Commits follow Conventional Commits. Never add AI attribution trailers — no `Co-Authored-By:`, no `Claude-Session:`, no equivalent — to commit messages or PR descriptions, even when tooling suggests them. The canonical agent reference is `docs/src/content/docs/reference/agent-workflows.mdx`. diff --git a/CLAUDE.md b/CLAUDE.md index 524df5f..be0811b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,33 +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`. -- 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. 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/docs/audits/agent-compatibility-2026-09.md b/docs/audits/agent-compatibility-2026-09.md new file mode 100644 index 0000000..f021f6d --- /dev/null +++ b/docs/audits/agent-compatibility-2026-09.md @@ -0,0 +1,281 @@ +# Agent compatibility & AI capability audit — September 2026 + +Scope: everything in this repository that an AI coding agent touches — the agent +instruction files, the shared Codex/Claude plugin, the machine-readable CLI +contract (`--output json` / `schema`), the generated enforcement (hooks + CI), +and the in-process LLM integration under `src/ai/`. + +Method: full read of the agent-facing surfaces at `caf53e3` (v1.9.1), compared +against the state of the agent-integration ecosystem as of September 2026. + +--- + +## 1. What exists today + +| Surface | Location | State | +|---|---|---| +| Agent instructions | `AGENTS.md`, `CLAUDE.md` | Two near-identical files (diff is 2 lines) | +| Claude marketplace | `.claude-plugin/marketplace.json` | Valid, points at `./plugins/committy` | +| Codex marketplace | `.agents/plugins/marketplace.json` | Valid, local source | +| Plugin manifests | `plugins/committy/.claude-plugin/plugin.json`, `.codex-plugin/plugin.json` | Skills only; no commands/agents/hooks/mcpServers | +| Skills | 4 × `SKILL.md` (branch, commit, release, enforce) | Well-scoped, progressive disclosure, safety rules present | +| Codex interface shims | 4 × `agents/openai.yaml` | Display metadata only | +| Machine contract | `src/cli/output.rs` (`api_version = 1`) + per-command payloads | Stable envelope: `api_version`, `command`, `ok`, `dry_run`, `errors` | +| Capability discovery | `src/cli/commands/schema.rs` | 21 capabilities with consent metadata (was 10, bare) | +| Repo enforcement | `src/cli/commands/hooks.rs:14-37` | `commit-msg`, `pre-push`, GitHub Actions template | +| Local agent guardrail | `.claude/settings.json` + `.claude/hooks/block-dangerous-git.sh` | PreToolUse Bash denylist | +| LLM integration | `src/ai/mod.rs`, `src/cli/commands/group_commit.rs:98-151, 356-507` | OpenRouter + Ollama, `group-commit` only | + +The core architectural bet is **CLI-first with typed JSON**. That still holds, +but not for the reason an earlier draft gave: see §3.4, where the blanket +"never build an MCP server" recommendation is withdrawn, and §3.5, where the +CLI's own agent ergonomics turned out to be the real problem. + +--- + +## 2. What changed in the space (relevant deltas) + +1. **`AGENTS.md` is now the cross-tool standard.** Stewarded by the Agentic AI + Foundation (Linux Foundation), read by 30+ agents (Codex, Copilot, Cursor, + Gemini CLI, Jules, Aider, Zed, Windsurf, Devin), 60k+ repositories. Claude Code + remains the outlier reading `CLAUDE.md`; the accepted pattern is a one-line + `@AGENTS.md` import rather than a maintained duplicate. +2. **Agent Skills has a published spec.** Frontmatter keys are fixed to + `name`, `description`, `license`, `allowed-tools`, `metadata`, `compatibility`; + unknown keys fail validation. `allowed-tools` is the security-relevant addition. +3. **`.agents/skills/` is the cross-tool skill discovery path.** Codex CLI, the + Codex app, Claude Code and Copilot CLI all resolve it. Skills are now the most + portable plugin component; per-vendor interface files are the least portable. +4. **Codex shipped a plugin marketplace** (26 Mar 2026) bundling skills, MCP + servers and app integrations; self-serve publishing to the official directory + is still "coming soon" as of mid-2026. +5. **Claude Code plugins now carry more than skills**: `commands`, `agents`, + `hooks`, `mcpServers`, `outputStyles`, `lspServers`, auto-discovered from + standard directories. +6. **The MCP token argument has a shelf life.** The benchmarks (35× token cost, + reliability dropping to 72% on hard tasks, ~55k tokens for a naive GitHub + server) measure *eager-loading* MCP, where every tool schema is injected at + session start. Progressive disclosure recovers a reported 60–85%. +7. **The 2026-07-28 MCP spec is the largest revision since launch**: stateless + core, sessions gone, Roots/Sampling/Logging deprecated, MCP Apps and Tasks + added, progressive discovery and tool search on the roadmap. Recommendations + written against the old full-bundle design no longer apply cleanly. +8. **Ollama structured output moved to JSON-Schema-constrained decoding** via a + **top-level** `format` field; `/api/chat` still defaults to `stream: true`. + +--- + +## 3. Findings + +Severity: **P0** = broken or misleading in the field · **P1** = real compatibility +or maintenance cost · **P2** = polish. + +### 3.1 Defects to fix + +**P0-1 — The Ollama provider 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 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 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 +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 — 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. + +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` 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 +families out of date. Both should be corrected. + +### 3.2 Compatibility gaps to close + +**P1-7 — `capabilities` in `schema` was stale and undersold the CLI.** ✅ **Fixed.** +`schema.rs` advertised 10 capabilities covering branch, commit and hooks only, +while the `committy-release` and `committy-enforce` skills instructed agents to +run `bump`, `tag`, `changelog` and `config validate` — commands the capability +list denied existed. `agent-workflows.mdx` promises capabilities exist precisely +"so skills do not need to hard-code the command surface"; they had to. + +Now 21 capabilities cover the whole agent surface, and each declares `mutating` +and `requires_confirmation` so an agent can reason about consent without +pattern-matching command names (`tag.publish` is the only one gated on an +explicit confirmation flag). Covered by +`tests/agent_ergonomics_tests.rs`. + +Still open: the list is hand-maintained and can drift again. Deriving it from +the command registry would close that for good. + +**P1-8 — `AGENTS.md` and `CLAUDE.md` are maintained duplicates.** +The diff is two lines. This will drift. Make `AGENTS.md` canonical and reduce +`CLAUDE.md` to an `@AGENTS.md` import plus the Claude-specific skill-invocation +line — the pattern the ecosystem settled on. + +**P1-9 — No skills are exposed at `.agents/skills/`.** +`.agents/plugins/marketplace.json` covers Codex plugin installation, but the +cross-tool discovery path that Copilot CLI and others read is `.agents/skills/`. +Exposing the four skills there (symlink, or move the canonical copy there and +point both plugin manifests at it) makes them portable to every agent that +implements the spec, at zero maintenance cost. + +**P1-10 — Skill frontmatter omits the spec's optional security fields.** +All four `SKILL.md` files carry only `name` and `description`. The spec now +defines `allowed-tools`, `license`, `metadata` and `compatibility`. Adding +`allowed-tools` to each skill pre-authorizes exactly what the skill needs in +security-conscious runtimes, and `metadata` gives you a version handle for the +skills independent of the CLI version. + +**P1-11 — Only two agents are addressed; the instruction file is the cheap part.** +There is no `.github/copilot-instructions.md`, no `GEMINI.md`, no Cursor rules. +Given `AGENTS.md` already exists, these are thin pointer files. Worth doing for +Copilot specifically — it is the widest-deployed agent that does not read +`AGENTS.md` natively. + +**P1-12 — No tests cover the agent plugin surface.** +`CLAUDE.md` explicitly asks for "public-seam tests for machine output, exit codes, +Git behavior, hook protocol, and **plugin manifests**." `tests/agent_cli_tests.rs` +covers the first four well (21 tests); nothing validates the marketplace JSON, +plugin manifests, or SKILL.md frontmatter against their schemas. A manifest typo +currently ships silently. Add a test that parses all six JSON/YAML manifests and +asserts every `SKILL.md` frontmatter uses only spec-allowed keys. + +**P2-13 — Generated CI template is dated and slow.** +`hooks.rs:31-36` pins `actions/checkout@v4` (v5 is current) and installs via +`cargo install committy --locked` on every PR — a multi-minute Rust build per run +with no caching and no toolchain pin. Prefer downloading the release binary from +`install.sh`, or add `Swatinem/rust-cache`. The same `checkout@v4` pin appears in +all three of this repo's own workflows. + +**P2-14 — `$committy-commit` syntax leaks Codex convention into shared skills.** +`committy-release/SKILL.md` closes with "Use `$committy-commit`". Claude Code +invokes these as `/committy:committy-commit` (as `CLAUDE.md` itself documents). +Since the `SKILL.md` body is shared across both runtimes, reference sibling skills +by bare name and let each runtime resolve it. + +**P2-15 — The local `block-dangerous-git.sh` guardrail is easily evaded and +over-blocks.** `.claude/hooks/block-dangerous-git.sh` greps the raw command string +for nine substrings. It matches the *text* of a command, not its effect — writing +this very report through a heredoc was blocked because the prose names the push +command. It misses trivial variants (double-space, `env X=1` prefix, `git -C .`), +and it blocks the push-with-upstream form that this repository's own agent +workflow instructions require. It also assumes `jq` is installed with no fallback. +Treat it as a speed bump, not a control: the real enforcement is the `pre-push` +hook, which is policy-engine backed. + +### 3.3 Removals + +| Remove | Where | Why | +|---|---|---| +| `--ai-diff-lines-per-file` | `group_commit.rs:143-147`, `ai-flags.mdx` | Never read; pure surface area (or implement under P0-3) | +| `LlmError::_Timeout` | `src/ai/mod.rs:12` | Dead variant; timeouts surface as `RequestFailed` | +| Commented-out `LlmProvider` enum | `src/ai/mod.rs:30-35` | Dead code; provider is a `&str` match at the call site | +| Duplicated body of `CLAUDE.md` | `CLAUDE.md` | Replace with `@AGENTS.md` import (P1-8) | +| `agents/openai.yaml` × 4 | `plugins/committy/skills/*/agents/` | Candidate only — verify Codex still reads these; if the marketplace entry supplies display metadata, they are redundant per-vendor files on the least portable layer | +| Empty roadmap page | `docs/src/content/docs/project/roadmap.mdx` | Frontmatter only, no body — publishes an empty page | + +### 3.4 On building an MCP server + +An earlier revision of this report said flatly "do not build one". That was +argued against eager-loading MCP, which the protocol has since moved off: the +2026-07-28 spec makes the core stateless, drops sessions, and puts progressive +discovery and tool search on the roadmap. The headline benchmarks against MCP — +35× token cost, reliability falling to 72% on hard tasks — measure the old +full-bundle design; progressive disclosure reportedly recovers 60–85% of that. +So the blanket recommendation was wrong and is withdrawn. + +What remains true is the ordering. The observed agent struggle traced to three +mechanical CLI faults (§3.5), not to the CLI *being* a CLI, and all three were +fixable in an afternoon. An MCP server would have fixed them incidentally, by +deleting argv from the problem, while adding a server to build, distribute, and +keep in version lockstep with the binary — and it would not help the Git hook and +CI path at all, which must stay CLI and is where Committy's actual enforcement +lives. + +Revisit if agents still struggle now that §3.5 is fixed. If built, shape it as +roughly six stateless tools mapping to capability groups (`policy.discover`, +`commit.preview`/`apply`, `branch.preview`/`apply`, `lint`) rather than eighteen +tools mirroring subcommands, and have `policy.discover` serve the existing +`schema --output json` payload instead of re-describing the surface. + +Still not recommended: **an LLM path on the single-commit `commit` command.** The +calling agent already has the diff in context and writes better messages than a +256-token side-channel call. Committy's differentiated value is *validating* the +agent's message, not competing with it. + +### 3.5 CLI ergonomics — diagnosed, fixed, and under test + +Reproduced against v1.9.1 and now covered by `tests/agent_ergonomics_tests.rs`: + +| # | Fault | Fix | +|---|---|---| +| E-1 | `--non-interactive`, `-q` were root-only, so `committy schema --non-interactive …` — the form agents write — was rejected by clap | `global = true` on both | +| E-2 | Argument-parse rejections wrote nothing to stdout and prose to stderr, even with `--output json` | `invalid_usage` envelope on stdout; text mode unchanged | +| E-3 | `COMMITTY_NONINTERACTIVE=1` makes placement irrelevant but appeared in no agent-facing doc | documented in `AGENTS.md`, `CLAUDE.md`, `agent-workflows.mdx` | + +`--verbose` was deliberately left root-only: `-v` is `branch --validate` and +`--verbose` belongs to `config validate|show`, so promoting it would silently +steal both shorts. A test pins that trade-off. + +--- + +## 4. Suggested order of work + +0. ~~**P1-7**, **E-1**, **E-2**, **E-3**~~ — done: capabilities expanded with + consent metadata, global flag placement, `invalid_usage` envelope, env var + documented. See §3.5. +1. ~~**P0-1, P0-2, 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). +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 + +- [AGENTS.md Field Guide, 2026 edition](https://www.iuriio.com/blog/posts/2026/05/agents-md-field-guide-2026) +- [AGENTS.md Specification](https://asdlc.io/practices/agents-md-spec/) +- [Agent Skills — Specification](https://agentskills.io/specification) +- [SKILL.md YAML frontmatter reference](https://github.com/shalomb/agent-skills/blob/main/docs/reference/yaml-frontmatter.md) +- [Claude Code plugins reference](https://code.claude.com/docs/en/plugins-reference) +- [Codex CLI plugin marketplace](https://codex.danielvaughan.com/2026/04/24/codex-cli-plugin-marketplace-building-distributing-extending/) +- [OpenAI's Codex gets plugins](https://thenewstack.io/openais-codex-gets-plugins/) +- [Ollama structured outputs](https://docs.ollama.com/capabilities/structured-outputs) +- [Ollama /api/chat reference](https://docs.ollama.com/api/chat) +- [MCP roadmap: progressive discovery and agent auth](https://www.developersdigest.tech/blog/mcp-roadmap-progressive-discovery-agent-auth) +- [Progressive tool loading is the new MCP context pattern](https://usewire.io/blog/progressive-tool-loading-mcp-context-pattern/) +- [MCP context bloat fix 2026: tool search, code mode, progressive disclosure](https://mcp.directory/blog/mcp-context-bloat-fix-2026-tool-search-code-mode-progressive-disclosure) +- [MCP servers use 35x more tokens than CLI tools](https://www.mindstudio.ai/blog/mcp-servers-35x-more-tokens-cli-tools-reliability-benchmark) +- [MCP vs CLI for AI agents](https://www.firecrawl.dev/blog/mcp-vs-cli) +- [CLI vs MCP: should your agent call tools or run commands?](https://blog.mcpservers.org/posts/cli-vs-mcp) diff --git a/docs/src/content/docs/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/docs/src/content/docs/reference/agent-workflows.mdx b/docs/src/content/docs/reference/agent-workflows.mdx index e170a05..fbe1bde 100644 --- a/docs/src/content/docs/reference/agent-workflows.mdx +++ b/docs/src/content/docs/reference/agent-workflows.mdx @@ -19,6 +19,36 @@ Preferred sequence: 5. Require explicit confirmation for any remote mutation. 6. Use `--repo-path` when the agent should operate on another checkout. +## Flag placement + +Committy has two flag namespaces, and mixing them up is the most common way an +agent invocation fails: + +- **Global flags** — `--non-interactive` and `-q`/`--quiet` — are accepted on + either side of the subcommand. +- **Everything else** — `--repo-path`, `--output`, `--dry-run`, and every + command-specific flag — must follow the subcommand. +- **`-v`/`--verbose` must precede the subcommand.** It is deliberately not + global: `-v` already means `--validate` on `branch`, and `--verbose` is a real + flag on `config validate` and `config show`. + +The most reliable option is to stop passing `--non-interactive` altogether and +set the environment variable instead, which makes placement irrelevant: + +```bash +export COMMITTY_NONINTERACTIVE=1 +committy schema --repo-path . --output json +``` + +`CI=1` has the same effect, so Committy is already non-interactive on most CI +runners. + +When an invocation is rejected outright — an unknown flag, a misplaced argument +— Committy still writes one JSON document to stdout if `--output json` was +requested, with `"code": "invalid_usage"` and clap's explanation as the message. +Agents can parse the rejection and self-correct rather than falling back to +scraping stderr. + ## Policy discovery ```bash diff --git a/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/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/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())); + } +} 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/src/cli/commands/schema.rs b/src/cli/commands/schema.rs index 492971d..f8754c2 100644 --- a/src/cli/commands/schema.rs +++ b/src/cli/commands/schema.rs @@ -37,50 +37,117 @@ struct SchemaOutput { struct CapabilityDefinition { name: &'static str, description: &'static str, + /// Whether applying this capability changes the repository or a remote. + mutating: bool, + /// Whether the CLI refuses to proceed without an explicit confirmation + /// flag. An agent must never infer this consent from the task alone. + requires_confirmation: bool, +} + +/// Read-only capability: previews and validation. +const fn read_only(name: &'static str, description: &'static str) -> CapabilityDefinition { + CapabilityDefinition { + name, + description, + mutating: false, + requires_confirmation: false, + } +} + +/// Local mutation: changes this checkout, needs no confirmation flag. +const fn local_write(name: &'static str, description: &'static str) -> CapabilityDefinition { + CapabilityDefinition { + name, + description, + mutating: true, + requires_confirmation: false, + } +} + +/// Remote mutation: gated behind an explicit confirmation flag. +const fn remote_write(name: &'static str, description: &'static str) -> CapabilityDefinition { + CapabilityDefinition { + name, + description, + mutating: true, + requires_confirmation: true, + } } fn capabilities() -> Vec { vec![ - CapabilityDefinition { - name: "branch.preview", - description: "Resolve and validate a branch without changing git", - }, - CapabilityDefinition { - name: "branch.apply", - description: "Create a validated branch", - }, - CapabilityDefinition { - name: "branch.lint", - description: "Validate structured or policy-enforced branch names", - }, - CapabilityDefinition { - name: "commit.preview", - description: "Resolve and lint a commit without changing git", - }, - CapabilityDefinition { - name: "commit.apply", - description: "Create a validated conventional commit", - }, - CapabilityDefinition { - name: "commit.amend", - description: "Preview or apply a validated amend", - }, - CapabilityDefinition { - name: "commit.lint", - description: "Lint one message or repository history", - }, - CapabilityDefinition { - name: "hooks.install", - description: "Preview or install native Git hooks and CI enforcement", - }, - CapabilityDefinition { - name: "hooks.commit-msg", - description: "Enforce commit messages through a native git hook", - }, - CapabilityDefinition { - name: "hooks.pre-push", - description: "Enforce outgoing commit history through a native git hook", - }, + read_only( + "schema.discover", + "Report active convention, types, and this capability list", + ), + read_only( + "branch.preview", + "Resolve and validate a branch without changing git", + ), + local_write("branch.apply", "Create a validated branch"), + read_only( + "branch.lint", + "Validate structured or policy-enforced branch names", + ), + read_only( + "commit.preview", + "Resolve and lint a commit without changing git", + ), + local_write("commit.apply", "Create a validated conventional commit"), + local_write("commit.amend", "Preview or apply a validated amend"), + read_only("commit.lint", "Lint one message or repository history"), + read_only( + "group-commit.plan", + "Group a dirty worktree into coherent commits without changing git", + ), + local_write( + "group-commit.apply", + "Create the planned grouped commits; pushing additionally requires confirmation", + ), + read_only( + "bump.preview", + "Compute the next version from commit history without writing files", + ), + local_write( + "bump.apply", + "Write the computed version to the configured version files", + ), + read_only( + "changelog.preview", + "Render a changelog from git history without writing files", + ), + read_only( + "tag.preview", + "Compute the next tag and release plan without changing git", + ), + local_write( + "tag.apply", + "Create the computed tag in the local repository", + ), + remote_write( + "tag.publish", + "Push tags and publish the release; requires an explicit confirmation flag", + ), + read_only( + "config.validate", + "Validate repository configuration and report findings", + ), + read_only( + "packages.list", + "List configured packages and their resolved versions", + ), + local_write( + "hooks.install", + "Preview or install native Git hooks and CI enforcement", + ), + read_only( + "hooks.commit-msg", + "Enforce commit messages through a native git hook", + ), + read_only( + "hooks.pre-push", + "Enforce outgoing commit history through a native git hook", + ), ] } diff --git a/src/cli/output.rs b/src/cli/output.rs index 0fb74c6..b70710d 100644 --- a/src/cli/output.rs +++ b/src/cli/output.rs @@ -18,12 +18,41 @@ struct MachineError<'a> { #[derive(Debug, Serialize)] struct ErrorEnvelope<'a> { api_version: u8, - command: &'static str, + command: &'a str, ok: bool, dry_run: bool, errors: [MachineError<'a>; 1], } +/// Strip ANSI styling so the envelope carries plain text. +/// +/// Clap colours its rejections, and those escape sequences would otherwise end +/// up inside a JSON string value. +fn strip_ansi(text: &str) -> String { + static ANSI: once_cell::sync::Lazy = + once_cell::sync::Lazy::new(|| regex::Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]").unwrap()); + ANSI.replace_all(text, "").into_owned() +} + +/// Emit the machine envelope for an argument-parse rejection. +/// +/// `command` is attributed from raw argv and falls back to `"unknown"`, because +/// clap rejects the invocation before any value is parsed. +pub fn print_usage_error(command: &str, message: &str) { + let message = strip_ansi(message); + let payload = ErrorEnvelope { + api_version: API_VERSION, + command, + ok: false, + dry_run: false, + errors: [MachineError { + code: "invalid_usage", + message: &message, + }], + }; + println!("{}", serde_json::to_string(&payload).unwrap()); +} + pub fn print_error(context: MachineContext, error: &CliError) { let message = error.to_string(); let payload = ErrorEnvelope { diff --git a/src/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/src/main.rs b/src/main.rs index d55d427..8f97af2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -56,25 +56,91 @@ struct Opt { )] pre_release: bool, - #[structopt(long = "non-interactive", help = "Run in non-interactive mode")] + #[structopt( + long = "non-interactive", + global = true, + help = "Run in non-interactive mode" + )] non_interactive: bool, #[structopt(long = "metrics-toggle", help = "Toggle metrics collection on/off")] metrics_toggle: bool, + // Not global: `-v` is already taken by `branch --validate` and `--verbose` + // by `config validate|show`. Promoting it would silently steal those + // shorts. Agents use `-q` and `--non-interactive`, which are global below. #[structopt( short = "v", long = "verbose", parse(from_occurrences), - help = "Increase verbosity (-v, -vv)" + help = "Increase verbosity (-v, -vv); must precede the subcommand" )] verbose: u8, - #[structopt(short = "q", long = "quiet", help = "Reduce verbosity (errors only)")] + #[structopt( + short = "q", + long = "quiet", + global = true, + help = "Reduce verbosity (errors only)" + )] quiet: bool, } +/// Subcommand names as they appear on the command line. +/// +/// Used only to attribute an argv rejection to a command, since clap gives us +/// no parsed value in that case. +const SUBCOMMAND_NAMES: &[&str] = &[ + "commit", + "amend", + "tag", + "bump", + "changelog", + "lint", + "lint-message", + "example", + "info", + "ls", + "schema", + "version", + "branch", + "group-commit", + "hooks", + "init", + "config", + "packages", +]; + +/// Whether the caller asked for machine output, determined from raw argv. +/// +/// An argv rejection happens before any flag is parsed, so this is the only way +/// to know whether the caller is an agent expecting one JSON document. +fn requested_json_output(args: &[String]) -> bool { + args.iter().enumerate().any(|(index, arg)| { + arg == "--output=json" + || (arg == "--output" && args.get(index + 1).map(String::as_str) == Some("json")) + }) +} + +/// Best-effort command attribution for an argv rejection. +fn command_from_args(args: &[String]) -> &str { + args.iter() + .skip(1) + .find(|arg| !arg.starts_with('-')) + .map(String::as_str) + .filter(|candidate| SUBCOMMAND_NAMES.contains(candidate)) + .unwrap_or("unknown") +} + fn main() { + // Parse argv before touching configuration so a malformed invocation is + // rejected without side effects. + let args: Vec = std::env::args().collect(); + let opt = match Opt::from_iter_safe(&args) { + Ok(opt) => opt, + Err(error) => exit_on_parse_error(error, &args), + }; + // Load configuration let mut config = Config::load().unwrap_or_else(|_| { let default_config = Config::default(); @@ -84,7 +150,7 @@ fn main() { default_config }); - if let Err(e) = run(&mut config) { + if let Err(e) = run(&mut config, opt) { eprintln!("{e}"); let exit_code = e .downcast_ref::() @@ -94,9 +160,32 @@ fn main() { } } -fn run(config: &mut Config) -> Result<()> { - let opt = Opt::from_args(); +/// Report a clap rejection and terminate. +/// +/// `--help` and `--version` arrive here as errors too; they are successful +/// output and keep their plain-text form. A genuine rejection honours +/// `--output json` so that an agent's parser sees the same envelope it sees for +/// every other failure, instead of an empty stdout and prose on stderr. +fn exit_on_parse_error(error: structopt::clap::Error, args: &[String]) -> ! { + use structopt::clap::ErrorKind; + + if matches!( + error.kind, + ErrorKind::HelpDisplayed | ErrorKind::VersionDisplayed + ) { + println!("{}", error.message); + std::process::exit(0); + } + + if requested_json_output(args) { + cli::output::print_usage_error(command_from_args(args), &error.message); + } else { + eprintln!("{}", error.message); + } + std::process::exit(1); +} +fn run(config: &mut Config, opt: Opt) -> Result<()> { // Initialize logger based on verbosity flags let mut builder = Builder::from_env(Env::default().default_filter_or("info")); let level = if opt.quiet { diff --git a/tests/agent_ergonomics_tests.rs b/tests/agent_ergonomics_tests.rs new file mode 100644 index 0000000..d6f0f46 --- /dev/null +++ b/tests/agent_ergonomics_tests.rs @@ -0,0 +1,472 @@ +//! 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")); + + // `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") || 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 + // 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" + ); + } +} + +/// 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" + ); + } +} diff --git a/tests/guardrail_hook_tests.rs b/tests/guardrail_hook_tests.rs new file mode 100644 index 0000000..bea25c9 --- /dev/null +++ b/tests/guardrail_hook_tests.rs @@ -0,0 +1,180 @@ +//! Behaviour matrix for the local agent guardrail. +//! +//! `.claude/hooks/block-dangerous-git.sh` is a PreToolUse hook: it reads the +//! tool input as JSON on stdin and exits 2 to refuse the command. Nothing +//! executed it before, so its deny rules were unverified — which matters more +//! than for ordinary code, because a guardrail that silently stops refusing +//! looks exactly like one that works. +//! +//! Exit codes: 0 = allowed, 2 = blocked. + +#![cfg(unix)] + +use std::io::Write; +use std::path::PathBuf; +use std::process::{Command, Stdio}; + +fn hook_path() -> 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" + ); +} 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 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" + ); + } +} 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()), ], );