fix: pass adapter command from UI form for model discovery#1920
fix: pass adapter command from UI form for model discovery#1920mjaverto wants to merge 3 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>
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>
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>
Greptile SummaryThis PR fixes a broken UX where the Model dropdown was always empty for adapters with custom command binaries (e.g. Confidence Score: 4/5Do not merge until the missing debounce in NewAgent.tsx and OnboardingWizard.tsx is addressed — the stated subprocess-storm mitigation is currently ineffective on those two pages. Two P1 findings: both NewAgent.tsx and OnboardingWizard.tsx pass raw command state directly into React Query without debouncing, defeating the 800 ms debounce carefully added in AgentConfigForm.tsx. Every keystroke still spawns a new server-side child process from those pages. The rest of the change is correct and well-structured. ui/src/pages/NewAgent.tsx and ui/src/components/OnboardingWizard.tsx need a debounced command value before the query key/fn construction. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/pages/NewAgent.tsx
Line: 89-96
Comment:
**Debounce bypassed — subprocess storm still possible**
`NewAgent.tsx` uses `configValues.command` directly in both the `queryKey` and `queryFn`, with no debouncing. `AgentConfigForm.tsx` (also rendered on this page) has its own debounced query using `debouncedCommand`, but that only gates `AgentConfigForm.tsx`'s own fetch. Because `NewAgent.tsx`'s query fires immediately on every `configValues.command` change, each keystroke in the command field produces a new query key → new HTTP request → new server-side subprocess.
Since the two queries share a key only once `debouncedCommand` has caught up (after 800 ms of no typing), the debounce protection added in `AgentConfigForm.tsx` provides no benefit for the `NewAgent.tsx` create flow. `OnboardingWizard.tsx` has the same problem since its `command` state is also used directly without a debounce.
**Fix**: introduce a locally debounced command value in `NewAgent.tsx` (matching the 800 ms `useEffect` timer pattern already used in `AgentConfigForm.tsx`), and use that debounced value in the `queryKey` and `queryFn` instead of `configValues.command` directly.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/components/OnboardingWizard.tsx
Line: 200-208
Comment:
**No debounce on `command` — per-keystroke subprocess spawning**
Like `NewAgent.tsx`, this query uses the raw `command` state value directly. Each character the user types changes the query key, triggering a new HTTP request and a new server-side `opencode models` subprocess immediately. The 800 ms debounce added in `AgentConfigForm.tsx` has no effect here because this component manages its own `command` state and query independently.
A `debouncedCommand` local state (same 800 ms pattern) should be introduced for the query key and queryFn to prevent the subprocess storm on each keystroke.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: server/src/routes/agents.ts
Line: 673
Comment:
**`\` (backslash) and `..` (path traversal) not rejected**
The current regex blocks common shell metacharacters but does not block `\` (Windows path separator) or relative path traversal sequences like `../../tmp/malicious`. With `shell: false` none of these characters enable command injection, and the PR correctly acknowledges that reaching an arbitrary binary still requires pre-existing filesystem write access. However, if the goal is defense-in-depth, consider also rejecting `\` and possibly enforcing that the value is an absolute path.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: pass adapter command from UI form f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| queryKey: selectedCompanyId | ||
| ? queryKeys.agents.adapterModels(selectedCompanyId, configValues.adapterType) | ||
| : ["agents", "none", "adapter-models", configValues.adapterType], | ||
| queryFn: () => agentsApi.adapterModels(selectedCompanyId!, configValues.adapterType), | ||
| ? queryKeys.agents.adapterModels(selectedCompanyId, configValues.adapterType, configValues.command || undefined) | ||
| : ["agents", "none", "adapter-models", configValues.adapterType, configValues.command || ""], | ||
| queryFn: () => | ||
| agentsApi.adapterModels(selectedCompanyId!, configValues.adapterType, configValues.command ? { command: configValues.command } : undefined), | ||
| enabled: Boolean(selectedCompanyId), | ||
| staleTime: 60_000, | ||
| }); |
There was a problem hiding this comment.
NewAgent.tsx uses configValues.command directly in both the queryKey and queryFn, with no debouncing. AgentConfigForm.tsx (also rendered on this page) has its own debounced query using debouncedCommand, but that only gates AgentConfigForm.tsx's own fetch. Because NewAgent.tsx's query fires immediately on every configValues.command change, each keystroke in the command field produces a new query key → new HTTP request → new server-side subprocess.
Since the two queries share a key only once debouncedCommand has caught up (after 800 ms of no typing), the debounce protection added in AgentConfigForm.tsx provides no benefit for the NewAgent.tsx create flow. OnboardingWizard.tsx has the same problem since its command state is also used directly without a debounce.
Fix: introduce a locally debounced command value in NewAgent.tsx (matching the 800 ms useEffect timer pattern already used in AgentConfigForm.tsx), and use that debounced value in the queryKey and queryFn instead of configValues.command directly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/pages/NewAgent.tsx
Line: 89-96
Comment:
**Debounce bypassed — subprocess storm still possible**
`NewAgent.tsx` uses `configValues.command` directly in both the `queryKey` and `queryFn`, with no debouncing. `AgentConfigForm.tsx` (also rendered on this page) has its own debounced query using `debouncedCommand`, but that only gates `AgentConfigForm.tsx`'s own fetch. Because `NewAgent.tsx`'s query fires immediately on every `configValues.command` change, each keystroke in the command field produces a new query key → new HTTP request → new server-side subprocess.
Since the two queries share a key only once `debouncedCommand` has caught up (after 800 ms of no typing), the debounce protection added in `AgentConfigForm.tsx` provides no benefit for the `NewAgent.tsx` create flow. `OnboardingWizard.tsx` has the same problem since its `command` state is also used directly without a debounce.
**Fix**: introduce a locally debounced command value in `NewAgent.tsx` (matching the 800 ms `useEffect` timer pattern already used in `AgentConfigForm.tsx`), and use that debounced value in the `queryKey` and `queryFn` instead of `configValues.command` directly.
How can I resolve this? If you propose a fix, please make it concise.| } = useQuery({ | ||
| queryKey: createdCompanyId | ||
| ? queryKeys.agents.adapterModels(createdCompanyId, adapterType) | ||
| : ["agents", "none", "adapter-models", adapterType], | ||
| queryFn: () => agentsApi.adapterModels(createdCompanyId!, adapterType), | ||
| enabled: Boolean(createdCompanyId) && effectiveOnboardingOpen && step === 2 | ||
| ? queryKeys.agents.adapterModels(createdCompanyId, adapterType, command || undefined) | ||
| : ["agents", "none", "adapter-models", adapterType, command || ""], | ||
| queryFn: () => | ||
| agentsApi.adapterModels(createdCompanyId!, adapterType, command ? { command } : undefined), | ||
| enabled: Boolean(createdCompanyId) && effectiveOnboardingOpen && step === 2, | ||
| staleTime: 60_000, | ||
| }); |
There was a problem hiding this comment.
command — per-keystroke subprocess spawning
Like NewAgent.tsx, this query uses the raw command state value directly. Each character the user types changes the query key, triggering a new HTTP request and a new server-side opencode models subprocess immediately. The 800 ms debounce added in AgentConfigForm.tsx has no effect here because this component manages its own command state and query independently.
A debouncedCommand local state (same 800 ms pattern) should be introduced for the query key and queryFn to prevent the subprocess storm on each keystroke.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/OnboardingWizard.tsx
Line: 200-208
Comment:
**No debounce on `command` — per-keystroke subprocess spawning**
Like `NewAgent.tsx`, this query uses the raw `command` state value directly. Each character the user types changes the query key, triggering a new HTTP request and a new server-side `opencode models` subprocess immediately. The 800 ms debounce added in `AgentConfigForm.tsx` has no effect here because this component manages its own `command` state and query independently.
A `debouncedCommand` local state (same 800 ms pattern) should be introduced for the query key and queryFn to prevent the subprocess storm on each keystroke.
How can I resolve this? If you propose a fix, please make it concise.| const command = typeof req.query.command === "string" ? req.query.command : undefined; | ||
| if (command !== undefined) { | ||
| // Reject shell metacharacters and empty/whitespace-only values | ||
| if (!command.trim() || /[;&|`$(){}[\]!#~]/.test(command)) { |
There was a problem hiding this comment.
\ (backslash) and .. (path traversal) not rejected
The current regex blocks common shell metacharacters but does not block \ (Windows path separator) or relative path traversal sequences like ../../tmp/malicious. With shell: false none of these characters enable command injection, and the PR correctly acknowledges that reaching an arbitrary binary still requires pre-existing filesystem write access. However, if the goal is defense-in-depth, consider also rejecting \ and possibly enforcing that the value is an absolute path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/routes/agents.ts
Line: 673
Comment:
**`\` (backslash) and `..` (path traversal) not rejected**
The current regex blocks common shell metacharacters but does not block `\` (Windows path separator) or relative path traversal sequences like `../../tmp/malicious`. With `shell: false` none of these characters enable command injection, and the PR correctly acknowledges that reaching an arbitrary binary still requires pre-existing filesystem write access. However, if the goal is defense-in-depth, consider also rejecting `\` and possibly enforcing that the value is an absolute path.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
opencode_local). The server needed to run the configured binary to discover models, but the previous approach required anagentIdDB lookup — meaning users had to save first before models would populate.commandvalue directly from the live form state as a query parameter, so model discovery works immediately as the user types — no save required.Security considerations
We evaluated two approaches:
agentId(DB lookup)command(client param)We chose the direct
commandparameter because:local_trustedmode (primary deployment) is single-user on localhost — user already has shell accesstest-environmentendpoint and agent config save already accept user-supplied command paths at the same privilege levelspawn()withshell: false— attacker controls only the executable path, not arguments (hardcoded to["models"])Mitigations added
commandquery paramspawn()usesshell: falsewith fixed args["models"]staleTime: 60_000matches server-side discovery cache TTLTest plan
pnpm -r typecheck— 19 packages passpnpm test:run— 683 tests pass, 0 failurespnpm -r build— all packages buildReplaces #1905
🤖 Generated with Claude Code