feat(cli): forward --skills flag in ACP mode - #57511
Conversation
Related: #24466 (issue), #30560 (open competing PR). #30560 is the broader canonical fix — it forwards |
|
Thanks for the triage, @alt-glitch. Agreed that #30560 covers the same ground more broadly. We intentionally scoped this PR to the CLI layer only because:
So #57511 (CLI-forward) + #57510 (adapter plumbing) together deliver the same outcome as #30560, just split across two focused PRs. Marking as related rather than duplicate. Updated the PR body to clarify this relationship. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the CLI handoff. The reported gap is present on current main, but this change is not functional by itself.
Problems
hermes_cli/main.py:12815-12826currently dropsargs.skills, so the premise is valid. However, the proposed forwarded--skillsis not accepted byacp_adapter/entry.py:116-146; its parser defines no such argument and callsparse_args()at line 146.- The documented
hermes acp --skills ...form is also not parsed by the ACP subcommand:hermes_cli/subcommands/acp.py:14-52declares no--skillsflag. - Even after parsing, current ACP startup constructs
HermesACPAgent()without startup skills (acp_adapter/entry.py:260), and session agent construction has no preload-prompt input (acp_adapter/session.py:584-645).
Suggested changes
- Integrate this handoff with the adapter/session preload path described in companion #57510 (or the broader #30560), and add an end-to-end regression test that verifies the startup skill reaches ACP session construction.
- Support or correct the post-subcommand syntax shown in the PR body.
Automated hermes-sweeper review.
| acp_argv.append("--setup-browser") | ||
| if getattr(args, "assume_yes", False): | ||
| acp_argv.append("--yes") | ||
| if getattr(args, "skills", None): |
There was a problem hiding this comment.
Current acp_adapter.entry._parse_args() has no --skills argument (acp_adapter/entry.py:116-146), so this makes globally supplied skills fail argument parsing rather than preload them. Please land this with the adapter/session parser and injection path, plus coverage.
|
ปิด PR นี้ตามคำขอ (no longer needed / cleanup) |
Description
Forward the
--skillsflag from thehermes acpCLI command to the ACP adapter's argument parser, enabling users to preload skills when launching the ACP server.Changes
hermes_cli/main.py— forwardargs.skillstoacp_main()1 file, +3 lines.
Related
--skillsthrough the full ACP stack (CLI → entry → HermesACPAgent → SessionManager → build_preloaded_skills_prompt) with tests. This PR is intentionally scoped to the CLI layer only — the adapter-side skills plumbing is handled by companion PR feat(acp): adapter compliance v1 — capabilities, resource blocks, config modes, skills #57510 (feat(acp): adapter compliance v1). Together feat(acp): adapter compliance v1 — capabilities, resource blocks, config modes, skills #57510 + feat(cli): forward --skills flag in ACP mode #57511 cover the same ground as fix(acp): preload startup skills from --skills #30560, split across two focused PRs by concern.