[devswha]

The next feature slice out of server/index.ts and shared/protocol.ts, after integrations (#1686), plugins (#1687) and background tasks (#1688): the agent catalog route and its type move into their own files. Behaviour, path, status codes and messages stay the same. The plan is .omo/plans/feature-slices-plan.md in the maintainer's workspace.

This slice waited for PR #1597 (merged as 86e0643), which changed this same /api/agents branch. The refactor brief puts a bug fix first and the move after it. The branch moves here exactly as PR #1597 left it, including its on-demand login PATH lookup through the server's shell-agent locator.

This PR moves responsibility, so it is not move-only.

Two commits: the check first, then the move

1. test(server): pin GET /api/agents before it moves adds server/agents-routes.test.ts.

   • Why it is needed: the existing route coverage is api.contract.test.ts (fallbacks, PATH, sort), which needs a live isolated herdr. server/shell-agent.test.ts covers the locator but not the route.
   • How it runs without herdr, in the unit lane:
     • a real createServer({ port: 0, hostname: "127.0.0.1", stateDir: mkdtempSync(…), …, shellAgents }), given a stand-in locator through createServer's own shellAgents option;
     • herdr's agentManifests stubbed with spyOn(herdr, "agentManifests");
     • PATH pointed at a temp folder that holds only a codex executable, and restored after each test.
   • What it pins:
     • herdr's agents (codex, pi, someagent) are offered, plus Oh My Pi and Claude Code always;
     • the shell agent the locator finds (gjc) is offered and the one it does not find (omo) is not, and the locator is asked about both;
     • each agent has its label (an unknown kind keeps its kind as its label) and its installed flag: true for codex on PATH and for gjc, which the locator found, false for the rest;
     • the order is by label, then kind;
     • herdr's own error passes through (502);
     • a POST answers 400 method_not_allowed.

   On unchanged code it gives 2 pass, 0 fail.

2. refactor(server): move the agent catalog route and type into their own slice

   • Moved: the branch, unchanged, into handleAgentsRequest(request, pathname, shellAgents): Promise<Response | null> in the new server/agents-routes.ts, the nullable route pattern of the earlier slices.
   • Moved with it: AGENT_LABELS, which only this branch used.
   • The locator stays the server's: index.ts passes its own shellAgents, the one launches use too. The route's type allows only executable, so nothing is constructed, the locator's stop() stays with the server, and startup order does not change.
   • Moved: AgentKind, into the new shared/agents.ts (types only), together with its doc comment ("GET /api/agents: one agent kind herdr can start …"). That comment sat a few declarations away in protocol.ts, above the directory listing type. This is the one line that changes place for a reason other than the move, so the comment now documents the type it describes.
   • Stays in index.ts: one call at the same place in the dispatch order, after the auth, device, update and diagnostics routes and before /api/fs/*. The now-unused agentManifests, SHELL_AGENTS and AgentKind imports leave.
   • Stays in protocol.ts: a type re-export of AgentKind, so server/api.contract.test.ts, src/lib/api.ts, AgentPicker.tsx, NewSessionDialog.tsx, SessionHistory.tsx and WorktreeDialog.tsx compile unchanged.

Checks

• After the move: HERDR_TEST_MODE=unit bun test server/agents-routes.test.ts server/shell-agent.test.ts src/lib/agentRoster.test.ts server/tasks-routes.test.ts server/startup-order.test.ts gives 26 pass, 0 fail. The pinning test is unchanged between the two commits.
• --color-moved on the move commit: git diff --color-moved=plain --color-moved-ws=allow-indentation-change HEAD~1 HEAD -- server shared shows 106 lines as moved: the branch (one indent level shallower in its new file), the label table, AgentKind and its comment. The 21 other lines are:
  • in index.ts:
    • the shortened protocol import;
    • the shell-agent.ts import without SHELL_AGENTS;
    • the new agents-routes.ts import;
    • the dropped agentManifests import line;
    • the 2-line call.
  • in the new route file:
    • its 5-line header;
    • 4 imports;
    • the function's doc comment and signature;
    • return null.
  • in protocol.ts: the re-export line.
• Client imports: bun run typecheck passes on both tsconfigs. No file in src/ or site/ imports shared/agents.ts. shared/agents.ts has no import and no value export.
• Sizes: server/index.ts goes from 763 to 724 lines and shared/protocol.ts from 1,410 to 1,404. The new modules are 55 and 7 lines.
• Risks checked:
  • Import cycles: none new. The route imports herdr/client.ts, http.ts and shell-agent.ts, none of which reaches index.ts.
  • Initialization order: unchanged. AGENT_LABELS is a constant, and the locator is created by createServer as before.
• bun run check fast: passes (unit 6153 pass, 0 fail, 12 skip in 438 files, 8 at a time, 57.5s).
• CI's integration lane runs the api.contract.test.ts agent cases against an isolated live herdr.

Overlapping open PRs

Open PRs that touch these files: #1598 and #1442 (index.ts and protocol.ts), and #1652 and #1427 (protocol.ts). Each diff was checked for the moved lines and for AgentKind, and none of them changes either one:

• fix(history): preserve Claude store identity through resume #1598 adds a path to the bridgePath list in index.ts. That list names /api/agents, but it stays in index.ts and this PR does not touch it.
• feat(tabs): move a one-pane tab to another workspace #1427 changes an import line in server/api.contract.test.ts that names AgentKind. That import still resolves through the re-export in protocol.ts.

No changelog fragment: users see no change.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: devswha/ovis
• Branch: refactor/slice-agents
• File: .coderabbit.yaml
• Commit: 3004b87
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a5ca4e10-f67c-4329-b1b7-1966abd8f9c3

📥 Commits

Reviewing files that changed from the base of the PR and between 40d0681 and 3004b87.

📒 Files selected for processing: 5

• server/agents-routes.test.ts
• server/agents-routes.ts
• server/index.ts
• shared/agents.ts
• shared/protocol.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

📝 Summary

Summary by CodeRabbit

• New Features
  • The agents list now includes available agent types with clear labels and indicates which ones are installed and ready to use.
  • Agent options are presented in a consistent, alphabetical order.
  • Requests for the agent list now return clearer responses when the request method is unsupported or the service encounters an error.

The change adds a dedicated handler for GET /api/agents, exports the shared AgentKind type, and adds tests for catalog results, Herdr errors, and unsupported methods.

Changes: Agent catalog endpoint

Layer / File(s)	Summary
Shared agent-kind contract shared/agents.ts, shared/protocol.ts	AgentKind is exported from shared/agents.ts and re-exported from shared/protocol.ts. The /api/agents documentation comment is removed.
Catalog handler and server integration server/agents-routes.ts, server/index.ts, server/agents-routes.test.ts	The handler combines manifest kinds with always-offered and available shell agents, sets installation status, sorts results, and handles errors and non-GET requests. server/index.ts delegates agent requests to the handler. Tests cover the returned catalog, Herdr errors, and method rejection.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: haeminway1

Merge Risk: ⚪ Minimal · up to 3004b

The agent catalog move appears ready to merge after normal checks; the new test does not use the user’s herdr session by default.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title "refactor(server): move the agent catalog route and type into their own slice" matches the main change: the /api/agents route moves from server/index.ts into server/agents-routes.ts and Agen…
Description check		The description follows the template's shape. It has a Change section that explains the move, the behavior kept the same, and the reason for the test-first commit. It has a Validation section that lis…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "refactor(server): move the agent catalog route and type into their own slice" matches the main change: the /api/agents route moves from server/index.ts into server/agents-routes.ts and AgentKind moves into shared/agents.ts. It is specific and conventional-commit formatted. It is slightly long at 76 characters, but it is clear.

Full details: Description check

Explanation

The description follows the template's shape. It has a Change section that explains the move, the behavior kept the same, and the reason for the test-first commit. It has a Validation section that lists the unit test command, the typecheck, the check fast run, and the CI integration lane. It does not link an issue, but the template only requires one for features, and this is a refactor. It does not mention contract-test coverage explicitly for the HTTP change, though it does say api.contract.test.ts runs in CI. The changelog note is present.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[devswha]

Merge-pass review (maintainer-approved merge). CodeRabbit had no actionable comments; a code-review agent also reviewed 3004b87: APPROVE, no blockers.

Behaviour is identical.

• handleAgentsRequest (server/agents-routes.ts:33-53) matches the old /api/agents branch on main (after fix(agents): share bounded login PATH discovery with shell launch #1597, 86e0643) apart from indentation. AGENT_LABELS matches byte for byte and has no other user.
• In --color-moved, only 21 lines are not moved: the new file's header, imports, signature and return null, the import edits, the 2-line call and the re-export.
• The call keeps its place (index.ts:495), after every access, origin, watch and timeout gate. The bridge path list and the /api/machines/local list are untouched.

One locator.

• index.ts passes the server's own shellAgents (created at :230), the same one launches use (:542), and only the server stops it (:643).
• The handler can only call executable, so it cannot create a second locator.
• There is no import cycle, and startup-order.test.ts passes.

Type move. AgentKind moved to shared/agents.ts (types only), re-exported with export type. bun run typecheck passes.

Tests.

• The four unit test files give 57 pass. The /api/agents cases of api.contract.test.ts, including fix(agents): share bounded login PATH discovery with shell launch #1597's login-shell case, give 4 pass on an isolated herdr session.
• 15 of 16 handler mutations fail a test.
• The survivor is sort-by-kind instead of by label: the stubbed agents come out in the same order either way. The sort line moved unchanged and api.contract.test.ts:1296 still checks label order, so it does not block. A test-only follow-up will add agents whose label order differs from their kind order, plus the plain-error 500 case.

No closing keyword before another PR number in the title, body or commits.