Repository navigation
Agent compatibility: audit, CLI ergonomics, Ollama fixes, cross-tool portability, and test coverage - #49
Merged
Conversation
Audit the repository's agent-facing surfaces (AGENTS.md/CLAUDE.md, the shared Codex/Claude plugin and skills, the schema capability contract, generated hooks/CI, and the src/ai LLM integration) against the current state of the agent-integration ecosystem. Records 15 prioritised findings covering defects, compatibility gaps and removals, plus two explicit non-recommendations.
Agents consistently failed against the CLI for three mechanical reasons, each now reproduced and covered by tests/agent_ergonomics_tests.rs. - --non-interactive and -q/--quiet are global, so they parse on either side of the subcommand. --verbose stays root-only: -v belongs to `branch --validate` and --verbose to `config validate|show`, and promoting it would steal both shorts. A test pins that trade-off. - Argument-parse rejections now emit the machine envelope on stdout with code "invalid_usage" when --output json was requested, instead of an empty stdout and prose on stderr. Text mode is unchanged, and --help and --version keep their plain-text form and exit 0. - schema capabilities cover the whole agent command surface (tag, bump, changelog, group-commit, config, packages, schema) and each entry now declares `mutating` and `requires_confirmation`, so an agent can reason about consent without pattern-matching command names. - COMMITTY_NONINTERACTIVE is documented in AGENTS.md, CLAUDE.md and the agent-workflows reference; it makes flag placement irrelevant. Also withdraws the audit's blanket "do not build an MCP server": that was argued against eager-loading MCP, which the 2026-07-28 stateless spec has moved off.
Second half of the agent-ergonomics change: main.rs flag globals and the clap rejection handler, the expanded schema capability list, the ergonomics test suite, and the docs describing flag placement.
Adds tests/agent_ergonomics_tests.rs (12 tests) covering flag placement on both sides of the subcommand, the invalid_usage envelope on argv rejection, the expanded capability surface with consent metadata, and the presence of COMMITTY_NONINTERACTIVE in agent-facing docs. Documents flag placement in the agent-workflows reference and records the diagnosis in the audit, withdrawing its blanket recommendation against an MCP server.
Tooling re-adds Co-Authored-By and Claude-Session by default, so the prohibition has to live where agents read it rather than being restated each session.
Four real defects from the agent audit, plus one withdrawn finding.
- Ollama requests now set `stream: false`. The endpoint defaults to
streaming, so every previous call received newline-delimited events and
died in LlmError::Parse before silently falling back to the default
message. The provider has never worked.
- `format` moves from `options` to the top level, where Ollama reads it,
and now carries a JSON Schema for AiCommitSuggestion rather than the
string "json", so decoding is constrained instead of merely requested.
- The default (non-sensitive) AI path sent only the group name and an
instruction not to reveal anything, leaving the model nothing to work
from. It now sends a redacted shape summary: file count, extension
histogram and top-level areas. No filename, no path below the first
segment, and no file content is sent in either mode.
- `--ai-diff-lines-per-file` was declared, documented and accepted but
never read. Removed, along with the dead LlmError::_Timeout variant and
the commented-out LlmProvider enum.
The audit's P0-4 ("--ai silently ignored in --mode apply") was wrong and
is withdrawn: the apply arm has always had its own AI block. The real
problem there was duplication -- plan and apply carried identical copies
of group building and the AI block -- which is what made the misreading
possible. Both are extracted into build_groups() and ai_user_prompt().
Request bodies are now built by pure functions with unit tests, so the
wire format is verifiable without a network call.
The pull_request trigger filtered on branches: ["*"], and in Actions branch filters `*` matches everything except `/`. Any pull request whose base branch contains a slash therefore matched nothing and ran no checks at all -- silently, since a workflow that does not trigger looks the same as one with nothing to report. That covers every stacked pull request in this series (all but the first target a claude/... base and have zero check runs), and would equally cover anything based on a feat/ or fix/ prefix. `**` matches across slashes, which is what the filter meant.
AGENTS.md and CLAUDE.md were near-identical files kept in sync by hand; the diff was two lines and had already started drifting (the trailer ban landed in one of them only). AGENTS.md is now the single source and CLAUDE.md is an @AGENTS.md import plus the one genuinely Claude-specific line, which is the pattern the ecosystem settled on. Two runtimes read neither file, so they now get thin pointers: .github/copilot-instructions.md and GEMINI.md. Skills are exposed at .agents/skills, the cross-tool discovery path that Codex, Claude Code and Copilot CLI all resolve, via a symlink to the canonical copy under plugins/committy/skills -- no duplication. A test guards the symlink, because a checkout that materialises it as a plain file would break discovery silently. Skill frontmatter gains the spec's optional fields: license, allowed-tools (so security-conscious runtimes can pre-authorise exactly what each skill needs) and metadata carrying a skill version independent of the CLI version. committy-release referenced siblings with Codex's `$skill` syntax inside a body shared by both runtimes; they are now named plainly and each runtime resolves them its own way. The COMMITTY_NONINTERACTIVE docs test now accepts a file that defers to AGENTS.md rather than restating it, which is what the split requires.
AGENTS.md asks for public-seam tests over "machine output, exit codes, Git behavior, hook protocol, and plugin manifests". The first four were covered; nothing compiled or executed the manifests, so a typo in any of the six JSON and Markdown files shipped silently. Nine tests: both marketplace manifests parse and their declared sources resolve to real directories carrying the expected plugin manifest; the Claude and Codex plugin manifests agree on name, version and license; the Codex skills path resolves; skill frontmatter uses only the keys the Agent Skills spec allows and carries the two required ones; a skill's frontmatter name matches its directory; descriptions carry a trigger rather than just a capability; no skill body uses runtime-specific invocation syntax, since that body is shared by Codex and Claude Code; and every skill is reachable from the cross-tool .agents/skills path. Each assertion was mutation-checked -- a bogus frontmatter key and a marketplace source pointing at a missing directory both fail with the message that names the problem.
…rdrail The workflow `hooks install --with-ci` writes for users pinned actions/checkout@v4 and ran `cargo install committy --locked` on every pull request -- a multi-minute Rust build per run, uncached, with no toolchain pin. It now pins checkout@v5, pins the toolchain, caches the build, and takes non-interactive mode from COMMITTY_NONINTERACTIVE so the command cannot be broken by flag placement. The repository's own three workflows carried the same stale checkout pin and are bumped too. The local PreToolUse guardrail grepped the raw tool input for nine substrings, which made it both over- and under-inclusive. Over: it matched the *text* of a command rather than an invocation, so writing documentation that merely named a push was refused -- including, circularly, the heredoc that would have replaced this file -- and it refused the push-with-upstream form the repository's own agent instructions require. Under: two spaces between git and the subcommand, an `env X=1` prefix, or `git -C .` all slipped through, so it never was a control. It now resolves the command field (with a jq-free fallback), discards heredoc bodies, returns early unless git is invoked, and refuses only what destroys history or uncommitted work: unguarded force-push (--force-with-lease allowed), hard reset, forced clean, forced branch delete, and bulk working-tree discards. Plain push is left to the pre-push hook, which is policy-engine backed. PARTIALLY VALIDATED: the full behaviour matrix could not be run from the session that wrote the hook, since executing one's own permission hook is refused as self-modification. Observed in normal use: ordinary commands and a plain push pass, and `git branch -D` was correctly refused. The remaining deny cases are unverified and a reviewer should confirm them. Also removes the roadmap page, which carried frontmatter and no body and published as an empty page.
test_tag_respects_repo_git_overrides_and_cli_can_override_them asserts that a repo-configured gpg.program is consulted, by pointing it at a script that always fails and expecting the tag creation to fail with it. That only holds when gpg.format is openpgp. On a machine whose global config sets gpg.format=ssh, git routes signing to gpg.ssh.program, the stub never runs, the tag is signed successfully by the ssh signer, and the assertion evaporates -- the test fails claiming Committy ignored the override when Committy had passed it through correctly. Verified: the tag git produced in that environment carried a valid SSH signature. The test now pins gpg.format=openpgp alongside the other overrides, so it exercises the path it means to exercise regardless of the host. Separately, the three config::hierarchy tests call MergedConfig::load, which reads the user config from $HOME. The tests in config.rs mutate HOME and are marked #[serial]; these were not, so they could read a half-written or relocated user config while one of those held it, and MergedConfig::load(...).unwrap() would panic. They are now serial too. Caveat on that second change: it closes a real race but is unproven. The failure was observed once, and did not recur in thirteen subsequent runs (eight lib-only, five full) even with the fix reverted, so causality is reasoned rather than demonstrated. It is cheap and correct regardless -- a test reading global state that other tests mutate belongs in the same serial group.
…t caught The rewritten guardrail shipped unvalidated: nothing executed it, so its deny rules were assumed rather than checked. That matters more than for ordinary code, because a guardrail that silently stops refusing looks exactly like one that works. Twelve tests feed the hook the same JSON the harness does and assert the exit code. Writing them found three defects in the hook, all now fixed: - `git push -f origin main` was not refused. The force check was one ordered regex whose `-f` alternative required a preceding space, which the `push ` match had already consumed. Force detection is now two independent conditions -- a push, and a force flag anywhere -- so flag order and adjacency stop mattering. - A heredoc feeding an interpreter was dropped along with every other heredoc body, so `bash <<EOF` / `git reset --hard` / `EOF` sailed through. Heredoc bodies are still discarded when the opener writes to a file, but scanned when it names a shell or interpreter. - Quoted prose naming a dangerous command is still refused. This one is deliberate and now recorded as a decision: stripping quoted segments would fix it and would also let `bash -c "git reset --hard"` through, and for a guardrail a false refusal costs far less than a false pass. Coverage includes the variants the previous substring hook missed entirely -- doubled spaces, a leading env assignment, `git -C .`, and a git call after `&&`.
This was referenced Sep 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes and replaces #42, #43, #44, #45, #46 and #47 — same 14 signed commits, collapsed into one PR so it needs a single approval instead of six.
cargo test421 passed, 0 failed;fmtandclippy --all-targets --all-features -D warningsclean.1. The audit
docs/audits/agent-compatibility-2026-09.mdreviews every surface an AI coding agent touches: the instruction files, the shared Codex/Claude plugin and skills, the machine-readable CLI contract, generated hooks and CI, and thesrc/aiintegration — against the state of the agent ecosystem as of September 2026.Two of its findings turned out to be wrong, and both are recorded as withdrawn rather than quietly dropped:
--aiwas silently ignored in--mode apply. It never was — the apply arm has always had its own AI block. I'd misread agreppiped throughawk 'NR>=530'and taken the renumbered output for plan-arm line numbers.tag_tests"bug" was not a Committy bug (see §5).2. Agents couldn't drive the CLI
Three mechanical faults, each reproduced before fixing:
--non-interactiveand-q/--quietwere root-only, socommitty schema --non-interactive …— the form agents write by default — was rejected by clap. Both are nowglobal.--output json. It now emits the standard envelope with"code": "invalid_usage". Text mode unchanged;--help/--versionkeep plain text and exit 0.COMMITTY_NONINTERACTIVE=1makes placement irrelevant but appeared in no agent-facing doc.--verbosewas deliberately not made global:-visbranch --validateand--verbosebelongs toconfig validate|show. Promoting it panics the binary at startup on a short-flag collision. A test pins that trade-off.Capability discovery went 10 → 21 entries, covering the whole surface the shipped skills already tell agents to use. Each declares
mutatingandrequires_confirmation, so an agent can reason about consent without pattern-matching command names.3. The Ollama provider had never worked
Two independent bugs, either fatal:
streamwas never set./api/chatdefaults tostream: true, so responses were newline-delimited events andresp.json::<ResponseBody>()failed — every run ended inLlmError::Parseand silently fell back to the default message.formatwas nested underoptions, where Ollama does not read it. It is top-level, and now carries a JSON Schema forAiCommitSuggestionrather than the bare string"json".The default AI path also had no signal: unless
--ai-allow-sensitivewas set, the prompt held only the group name and an instruction to improve it "without revealing code or filenames". It now sends a redacted shape — file count, extension histogram, top-level areas. No filename, no path below the first segment, and no file content in either mode;--ai-allow-sensitivecontrols paths, not content, which the docs got wrong in both directions.Request bodies are now pure functions with unit tests, so the wire format is verifiable without a network call.
Removed:
--ai-diff-lines-per-file(declared, documented, accepted, never read),LlmError::_Timeout, the commented-outLlmProviderenum, and an empty roadmap page.planandapplyno longer carry byte-identical copies of group building and the ~150-line AI block.4. Cross-tool portability
AGENTS.mdis now canonical;CLAUDE.mdis an@AGENTS.mdimport plus the one Claude-specific line. They were hand-synced duplicates that had already drifted. Pointers added for the two runtimes that read neither:.github/copilot-instructions.mdandGEMINI.md.Skills are exposed at
.agents/skills(symlink to the canonical copy, no duplication) and carry the Agent Skills spec's optional fields —license,allowed-tools,metadata..agents/skillsas a plain file (Windows without developer mode,core.symlinks=false) breaks discovery silently. A test fails loudly instead.5. Tests, including two pre-existing failures on
developtag_tests::test_tag_respects_repo_git_overrides…— not a Committy bug. The test assertsgpg.programis consulted, which only holds whengpg.formatis openpgp. On a host withgpg.format=sshgit routes signing elsewhere, the failing stub never runs, and the test blames Committy for correctly forwarding the override. Now pinsgpg.format=openpgp.config::hierarchy::tests::test_repository_config_overrides_user— hardened, causality unproven. Those tests read$HOMEviaMergedConfig::loadwhile#[serial]tests inconfig.rsmutate it. They're now#[serial]too. But I saw the failure once and could not reproduce it in thirteen later runs, including with the fix reverted. Read it as hardening, not a verified fix.New coverage: 12 agent-ergonomics tests, 9 plugin-manifest/frontmatter tests (both mutation-checked against deliberately broken inputs), and a 12-case behaviour matrix for the local guardrail hook.
6. CI and the guardrail
CI was not running on stacked PRs at all.
lint.ymlfilteredbranches: ["*"], and in Actions filters*does not match/. Every PR based on aclaude/…branch had zero check runs — silently, since a workflow that doesn't trigger looks like one with nothing to report. Fixed to["**"]; it would equally have hidden CI on anyfeat/orfix/prefix.The generated
hooks install --with-ciworkflow pinnedactions/checkout@v4and ran an uncachedcargo installper PR; nowcheckout@v5, pinned toolchain,Swatinem/rust-cache.The local guardrail hook was rewritten and then actually tested. It had grepped raw tool input for nine substrings — matching the text of a command rather than an invocation (documentation naming a push was refused) while missing
git push,env X=1 git pushandgit -C . pushentirely. Writing the behaviour matrix found three further defects in my own rewrite, all fixed here: an unblockedgit push -f, abash <<EOFheredoc bypass, and a deliberate over-block on quoted prose now recorded as a decision.Generated by Claude Code