fix: thread agent command through model discovery endpoint#1905
fix: thread agent command through model discovery endpoint#1905mjaverto wants to merge 2 commits intopaperclipai:masterfrom
Conversation
…el discovery
The models listing endpoint called listOpenCodeModels() with no arguments,
so it always fell back to the default "opencode" command. When the default
binary was unavailable (e.g. stale nix store path), the endpoint returned []
even though agents had a working custom command in their adapterConfig.
Thread an optional { command } through the call chain. The route accepts an
optional agentId query param and resolves the command from the agent's
persisted adapterConfig — never from raw user input — to avoid command
injection.
Fixes paperclipai#1904
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR threads an agent's persisted Confidence Score: 4/5Not safe to merge as-is — the core feature silently does nothing due to the wrong argument being passed to getById. One P1 defect: svc.getById(companyId, agentId) passes companyId as the id, agentId is ignored, agent lookup always returns null, command is never extracted. The fix is a one-line change. All other parts of the PR (types, registry, models, test) are correct. server/src/routes/agents.ts — the getById call on line 673 must be corrected before merging. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: server/src/routes/agents.ts
Line: 673-676
Comment:
**Wrong argument passed to `getById` — feature is silently non-functional**
`svc.getById` (defined in `server/src/services/agents.ts` line 241) accepts exactly **one** parameter, `id: string`. Calling it as `svc.getById(companyId, agentId)` passes `companyId` as the `id` argument; JavaScript silently ignores the second argument `agentId`. As a result the DB query runs `WHERE agents.id = <companyId>`, which will never match an agent row. `agent` comes back `null`, `command` is never assigned, and the entire command-passthrough feature silently no-ops — the bug this PR aims to fix is still present.
All other call sites in `agents.ts` pass a single ID: `svc.getById(req.actor.agentId)`, `svc.getById(id)`, etc.
The call should be `svc.getById(agentId)` with an explicit company-ownership guard to prevent cross-company data leakage:
```suggestion
const agent = await svc.getById(agentId);
if (agent && agent.companyId === companyId && agent.adapterConfig && typeof agent.adapterConfig.command === "string") {
command = agent.adapterConfig.command;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: thread command option through listA..." | Re-trigger Greptile |
getById takes a single id param — was incorrectly called with (companyId, agentId) which silently passed companyId as the id, making the feature a no-op. Added explicit companyId ownership check to prevent cross-company data leakage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed in f41a8d3: Fixed Re: CONTRIBUTING.md Thinking Path format — will add if required by maintainers, but keeping the PR description focused on the technical fix for now. |
|
Closing in favor of a new PR with the correct approach — passing command directly from the UI form instead of looking up by agentId. |
The models endpoint previously required an agentId to look up the configured command binary from the database. This meant users had to save their agent config before the Model dropdown would work — broken UX when configuring a new command path. Now the UI sends the command value directly from the live form state, so model discovery works immediately as the user types — no save needed. Security note: we considered the agentId/DB-lookup approach (server- sourced command) vs direct client parameter. The direct approach was chosen because: - local_trusted mode (primary deployment) is single-user on localhost - The codebase already trusts user-supplied commands in agent config save and test-environment endpoints at the same privilege level - spawn() uses shell:false with hardcoded args ["models"], so the attacker controls only the binary path, not arguments - An attacker would need filesystem write access to place a malicious binary, at which point they already have code execution - Input validation rejects shell metacharacters as defense-in-depth Additional improvements from code review: - 800ms debounce on command input to prevent subprocess storm - staleTime: 60s on all adapter model queries (matches server cache) - Shell metacharacter validation on server route Replaces paperclipai#1905 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
GET /adapters/opencode_local/modelsendpoint calledlistOpenCodeModels()with no arguments, always falling back to the defaultopencodecommand. When the default binary was unavailable (e.g. stale nix store path), it returned[]even though agents had a working custom command in theiradapterConfig.agentIdquery param, looks up the agent's persistedadapterConfig.command, and passes it through — never exposing raw user input to process execution.Test plan
pnpm tsc --noEmitpassespnpm vitest run packages/adapters/opencode-local/src/server/models.test.ts— 4/4 passcommandin adapterConfig, hitGET /adapters/opencode_local/models?agentId=<id>, verify models populateFixes #1904
🤖 Generated with Claude Code