feat: add fast-agent built-in adapter#350
Conversation
|
Codex review: found issues before merge. Reviewed May 29, 2026, 1:31 PM ET / 17:31 UTC. Summary Reproducibility: not applicable. this is a feature PR rather than a bug report. The source check confirms current main lacks the built-in, and the PR body reports a live ACP startup proof for the new command. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land a maintainer-approved built-in only after the compatibility decision is explicit and the docs follow the repository ordering and neutrality rules; otherwise keep FastAgent usage documented as a custom agent override. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a feature PR rather than a bug report. The source check confirms current main lacks the built-in, and the PR body reports a live ACP startup proof for the new command. Is this the best way to solve the issue? Unclear: the registry mapping is narrow, but the stable name reservation and Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against f6de6dd18aae. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1a601d9d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - [Claude](Claude.md): built-in `claude -> npx -y @agentclientprotocol/claude-agent-acp` | ||
| - [Copilot](Copilot.md): built-in `copilot -> copilot --acp --stdio` | ||
| - [Droid](Droid.md): built-in `droid -> droid exec --output-format acp` with `factory-droid` and `factorydroid` aliases | ||
| - [FastAgent](FastAgent.md): built-in `fast-agent -> uvx fast-agent-acp` |
There was a problem hiding this comment.
Move FastAgent after the mandated prefix
In this harness-specific agent list, adding FastAgent here puts a non-prefix agent before cursor and gemini. The repo-level AGENTS.md Documentation Policy says that whenever multiple built-in agents appear in the same example set, the precedence pi, openclaw, codex, claude, gemini, cursor, copilot must not be broken, and PRs that introduce or preserve such ordering must be fixed before merge. Please place the new FastAgent entry after the mandated prefix order is satisfied.
Useful? React with 👍 / 👎.
f1a601d to
e1364f6
Compare
e1364f6 to
feec748
Compare
|
closing so @evalstate can take over |
* update capitalization, naming, use fast-agent.ai site * docs: normalize fast-agent naming
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Opened on behalf of Onur Solmaz (
osolmaz). This PR is ready for review.Summary
acpxdid not have a friendly built-in name for EvalState fast-agent.This change adds
acpx fast-agentand points it at the fast-agent ACP entrypoint,uvx fast-agent-mcp acp.The first convenience wrapper I tried,
uvx fast-agent-acp, failed in a live startup test with a missing Python module, so the PR now uses the main fast-agent package command that actually initializes and serves ACP.It also updates the supported-agent docs, the acpx skill, and tests so the new adapter is visible and verified like the other built-ins.
AI-assisted: yes.
I understand the change: it adds one registry mapping, documents the required
uvxdependency, and tests that the CLI resolves the new built-in command correctly.What Changed
The runtime registry now knows how to launch fast-agent through ACP.
The docs now list fast-agent anywhere the built-in registry is described.
fast-agent -> uvx fast-agent-mcp acpto the built-in agent registry.agents/FastAgent.mdand updated the README, docs site, install docs, acpx skill, and changelog.Testing
This was tested with focused checks, full repo validation, and a real local fast-agent ACP run.
The live test used
uvx,fast-agent-mcp0.7.13, and a temporaryfast-agent.yamlpointed at a local Ollamaqwen2.5:0.5bmodel.pnpm run build:test && node --test dist-test/test/agent-registry.test.js dist-test/test/integration.test.jspnpm run check:docspnpm run checknode dist/cli.js --timeout 60 --format json fast-agent exec "Reply with exactly: acpx fast-agent live ok"when the built-in still useduvx fast-agent-acp; it failed before initialize withModuleNotFoundError: No module named 'fast_agent_acp'.fast-agent.yaml(default_model: generic.qwen2.5:0.5b, local Ollama generic provider),node dist/cli.js --cwd "$tmp_cwd" --timeout 90 --format json fast-agent exec "Reply with exactly: acpx fast-agent live ok"initialized ACP, created a session, streamed assistant message chunks, and returnedstopReason: end_turn.Risks
The risk is low because this only adds a registry mapping and docs.
The main operational requirement is that users need
uvxavailable onPATHand fast-agent model/provider configuration for real prompts.