feat: OpenCode 런타임 통합 + 모델 분산 실행 - #3
Open
hjshin-ubob wants to merge 4 commits into
Open
Conversation
Wire provider adapters (Claude Code / OpenCode / HTTP API) into
SessionManager, PromptBuilder, Dashboard API, and setup wizard.
- config.ts: env var interpolation (${VAR}), provider validation,
getProviderForAgent/getDefaultProvider helpers
- session-manager.ts: provider routing in startAgent/ephemeral/meeting,
ConcurrencyLimiter integration with slot tracking
- prompt-builder.ts: EnhancedAgentSkill instruction rendering for
non-CLI providers, slash commands preserved for Claude Code
- dashboard/server.ts: provider summary in /api/status
- setup.ts + prompts.ts: ZAI provider setup wizard
- Delete duplicate providers/provider-adapter.ts interface
- Fix adapter imports to use canonical ../provider-adapter.js
- 50 new tests (145 total, 0 failures)
Previous adapter used invalid flags (--agent with system prompt string, -p shorthand). New adapter uses `opencode run --dangerously-skip-permissions 'message'` with system prompt + task instruction combined into the message argument. Model is specified via --model when configured. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Previously startAgent() threw when concurrency slots were full, causing infinite retry loops in the orchestrator. Now acquire() queues tasks internally and waits for a slot to open. Also makes api.baseUrl and api.model optional in schemas since opencode providers only need api.model, not a full HTTP endpoint. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Setup wizard now generates 4 opencode providers with different GLM models: CTO=glm-5.1, CEO/PO=glm-4.7, Designer=glm-4.6v, QA/Marketer=glm-4.6. Each model has independent concurrency slots so 6 agents run in parallel. No ZAI API key or credits required — runs entirely on opencode coding plan. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
📝 WalkthroughWalkthroughThe PR integrates multi-provider support with configuration-driven provider selection during setup, replaces monolithic provider-adapter interfaces with distributed adapter implementations, introduces concurrency limiting by model/provider in SessionManager, and adds provider/model tracking to sessions and dashboard status APIs. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Setup
participant PM as ProjectManager
participant SM as SessionManager
participant PF as ProviderFactory
participant PA as ProviderAdapter
participant CL as ConcurrencyLimiter
participant Tmux as Tmux
CLI->>PM: saveConfig(multi-provider config)
PM->>PM: resolveEnvVars(apiKey)
PM->>PM: validateProviderConfig()
Note over CLI,PM: startAgent(agent role)
SM->>PM: loadConfig()
SM->>PM: getProviderForAgent(role)
PM-->>SM: ProviderConfigType
SM->>PF: createProviderAdapter(providerConfig)
PF->>PA: return adapter instance
PA-->>SM: ProviderAdapter
SM->>SM: toAdapterConfig(providerConfig)
SM->>PA: buildCommand(systemPrompt, ...)
PA-->>SM: { command, useScriptFile }
SM->>CL: configureConcurrency(config.concurrency.rules)
SM->>CL: acquire(model, provider)
CL-->>SM: slot acquired / queued
SM->>Tmux: new-session(command)
Tmux-->>SM: AgentSession { provider, model }
Note over SM,CL: Session running...
SM->>SM: stopAgent(role)
SM->>Tmux: kill-session
SM->>CL: release(slot)
CL-->>SM: slot released, queue processed
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/core/agent/prompt-builder.ts (2)
25-30:⚠️ Potential issue | 🟠 MajorMove the language rule before the agent template.
The working-language rule is currently pushed after
agent.system_prompt_template, so it is not the topmost instruction.Proposed fix
- // 에이전트 기본 프롬프트 - parts.push(agent.system_prompt_template); - // 언어 룰 — 가장 먼저 주입 (모든 출력 형식의 기본 전제) parts.push(this.buildLanguageRule(config)); + + // 에이전트 기본 프롬프트 + parts.push(agent.system_prompt_template);As per coding guidelines, “All agent system prompts must include the working language from config.localization at the topmost position”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/agent/prompt-builder.ts` around lines 25 - 30, The language rule is pushed after the agent template so it isn't the topmost system instruction; update the assembly order in the prompt builder (where parts.push(agent.system_prompt_template) and parts.push(this.buildLanguageRule(config)) are called) so that this.buildLanguageRule(config) is pushed before agent.system_prompt_template, ensuring the working language from config.localization is the first system-level prompt element.
13-23:⚠️ Potential issue | 🟠 MajorThread
providerTypethroughbuildSystemPrompt()and update session call sites.The new non-CLI skill formatting (full instructions instead of slash-commands) is only reachable by calling
buildSkillsSection()withproviderTypeset; production code always calls it without that parameter, so OpenCode/HTTP/AICP agents incorrectly receive slash-command instructions instead of formatted instructions.Add
providerType?: stringto thebuildSystemPrompt()params object and pass it tobuildSkillsSection(agent, providerType)at line 42. Then update all three session call sites to determine the provider type before building the system prompt:
- Line 139 (
startAgent): CallgetProviderForAgent(config, agent.role)beforebuildSystemPrompt(), extractadapter.type, and pass it.- Line 470 (
startEphemeralAgent): Same pattern—determine provider type first, pass tobuildSystemPrompt().- Line 606 (
startMeetingSession): Same pattern—determine provider type first, pass tobuildSystemPrompt().This requires reordering the logic at each site so
getProviderForAgent()→createProviderAdapter()→ extract.typehappens before the system prompt is built, not after.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/agent/prompt-builder.ts` around lines 13 - 23, Add an optional providerType?: string to buildSystemPrompt(params) and forward it into buildSkillsSection(agent, providerType) inside buildSystemPrompt; then update each session starter (startAgent, startEphemeralAgent, startMeetingSession) to determine the provider type before building the system prompt by calling getProviderForAgent(config, agent.role), creating the provider adapter (createProviderAdapter or equivalent) and extracting adapter.type, then pass that type into buildSystemPrompt; reorder the existing logic so provider lookup/adapter creation → const providerType = adapter.type → buildSystemPrompt({... , providerType }) happens before any prompt construction or session creation.src/core/session/session-manager.ts (2)
130-135:⚠️ Potential issue | 🟠 MajorUse the required
ip-<role>tmux session name.Persistent agent sessions are still created and looked up with the bare role name. This violates the one-session-per-role contract and can make role matching ambiguous. Use
ip-${agent.role}consistently forhasSession(),createTmuxSession(),killSession(), and log names. As per coding guidelines,Each role can have only one active tmux session at a time - session names must follow ip-<role> pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/session/session-manager.ts` around lines 130 - 135, The code uses the bare role as the tmux session name; change it to the required ip-<role> pattern by prefixing the role with "ip-" wherever session names are constructed or looked up (e.g., the sessionName variable used with this.tmux.hasSession and this.activeSessions.get), and ensure the same ip-<role> string is used when calling createTmuxSession(), killSession(), and in any log messages; update all references so session naming is consistent and enforces one-session-per-role.
325-328:⚠️ Potential issue | 🟠 MajorRelease limiter slots when stopping all sessions.
stopAll()kills tmux sessions but does not resolveslotResolvers, so providerexecute()promises can remain pending andConcurrencyLimiterrunning counts may never release. Resolve and clear slot/create-complete resolvers in afinallyblock.Minimal fix
async stopAll(): Promise<void> { - await this.tmux.killAllSessions(); - this.activeSessions.clear(); + try { + await this.tmux.killAllSessions(); + } finally { + for (const resolve of this.slotResolvers.values()) { + resolve(); + } + this.slotResolvers.clear(); + this.createCompleteResolvers.clear(); + this.activeSessions.clear(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/session/session-manager.ts` around lines 325 - 328, stopAll() currently kills tmux sessions via tmux.killAllSessions() and clears activeSessions but does not resolve pending slotResolvers/createCompleteResolvers, leaving ConcurrencyLimiter.execute() promises unresolved and counts stuck; update stopAll() to, in a finally block after killing sessions, iterate and call each resolver in slotResolvers and createCompleteResolvers (resolving with an appropriate error or undefined), then clear those maps/arrays so pending execute() callers are settled and the limiter's counts can release; reference stopAll(), tmux.killAllSessions(), slotResolvers, createCompleteResolvers, and ConcurrencyLimiter.execute() when making the change.
🧹 Nitpick comments (2)
tests/unit/cli/setup-provider.test.ts (1)
94-107: Validate the real concurrency rules instead of a copied list.This test only checks local fixtures, so changes to
createZaiDefaultConfig().concurrency.rulescan introduce an invalid regex without failing this suite.🧪 Proposed test improvement
-import { createDefaultConfig, validateProviderConfig } from '../../../src/core/project/config.js'; +import { createDefaultConfig, createZaiDefaultConfig, validateProviderConfig } from '../../../src/core/project/config.js'; @@ it('concurrency rules have valid regex patterns', () => { - const rules = [ - { model: 'glm-5-turbo', limit: 1 }, - { model: 'glm-5', limit: 2 }, - { model: 'glm-5\\.1', limit: 1 }, - { model: 'glm-4\\.5$', limit: 10 }, - { model: 'glm-4-plus', limit: 20 }, - ]; + const rules = createZaiDefaultConfig().concurrency.rules; for (const rule of rules) { expect(() => new RegExp(rule.model)).not.toThrow(); expect(rule.limit).toBeGreaterThan(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/cli/setup-provider.test.ts` around lines 94 - 107, The test currently validates a hard-coded rules array; instead load and validate the real rules from createZaiDefaultConfig().concurrency.rules: call createZaiDefaultConfig(), get its concurrency.rules, then for each rule assert that new RegExp(rule.model) does not throw and that rule.limit is > 0 (mirror the existing checks). Update the spec in tests/unit/cli/setup-provider.test.ts to reference createZaiDefaultConfig() and iterate its .concurrency.rules so any invalid regexes in the actual config fail the test.src/cli/utils/prompts.ts (1)
114-129: Collapse the duplicateProviderSetupAnswersdeclaration.TypeScript will merge these two interfaces, but keeping both makes future edits error-prone. Keep the second declaration (which includes the
providerChoiceproperty) and remove the first one.Proposed cleanup
-export interface ProviderSetupAnswers { - useZaiProvider: boolean; - apiKey?: string; - codingModel?: string; - generalModel?: string; -} - export type ProviderChoice = 'claude-code' | 'opencode' | 'zai-api' | 'mixed-opencode-zai' | 'mixed-claude-zai'; export interface ProviderSetupAnswers {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/utils/prompts.ts` around lines 114 - 129, There are two declarations of the interface ProviderSetupAnswers causing an accidental merge; remove the first (simpler) declaration and keep the second one that includes providerChoice so ProviderSetupAnswers (and usages) only exist once alongside the ProviderChoice type; update imports/usages if any tooling flagged the duplicate after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 198: The fenced code block in README.md is missing a language specifier
which triggers markdownlint; update the opening triple-backtick for the ASCII
architecture diagram (the code fence that currently starts with ```) to include
the language tag text (i.e., change ``` to ```text) so the block is marked as
plain text for the diagram.
In `@src/cli/commands/setup.ts`:
- Around line 47-49: When the user selects 'claude-code' the setup currently
only sets config.default_provider but leaves any existing
config.agent_providers.mapping in place, causing stale provider routing; update
the setup branch that handles choice === 'claude-code' (the code that sets
config.default_provider) to also clear or reset config.agent_providers.mapping
(e.g., assign an empty object or remove mappings) so that agent provider
resolution no longer prefers old ZAI/OpenCode entries.
- Around line 120-138: The mixed "opencode" provider lacks an explicit model, so
update config.providers for the 'opencode' entry to include an api.model (mirror
the coding model used for 'zai-coding') so the adapter can emit the model flag
and the session/concurrency layer can track CTO by model; specifically modify
the config.providers 'opencode' object (the one referenced by
agent_providers.mapping's 'cto') to include an api.model set from
providerAnswers.codingModel ?? 'glm-5-turbo' (or the project’s default coding
model).
In `@src/core/project/config.ts`:
- Around line 127-136: The getProviderForAgent function currently returns early
for missing/default or 'claude-code' before honoring role-specific overrides;
change the logic in getProviderForAgent so that you first check
config.agent_providers?.overrides[role] and return it if present (overrides →
mapping → default), then resolve providerName from mapping or default and only
then apply the guard that returns null for missing or 'claude-code'; update
references to mapping, overrides, providerName, and config.providers
accordingly.
In `@src/core/session/providers/opencode-adapter.ts`:
- Around line 77-80: The code pushes this.model directly into the shell argument
array (parts) which can allow whitespace or shell metacharacters to break or
inject into the executed command; update the logic around where
parts.push('--model', this.model) is used (the code that builds the parts array
in the Opencode adapter) to ensure the model value is safely shell-quoted or
escaped before being appended (e.g., use a robust escaping/quoting helper or
wrap the model in a safe-quote function) so the resulting argument is treated as
a single literal token by the shell.
In `@src/core/session/session-manager.ts`:
- Around line 195-215: The code passes mcpConfig (built by buildMcpConfigObject)
into adapter.buildCommand but the OpenCode provider ignores it, causing agents
with required_mcp_tools to start without required tooling; update the OpenCode
adapter's buildCommand implementation to accept and honor the mcpConfig
parameter (inject required_mcp_tools into the generated command/environment or
file mounts) or, before calling adapter.buildCommand from the session manager,
validate that agent.required_mcp_tools are satisfied by the adapter and emit a
clear error or reject the launch (use symbols mcpConfig, buildMcpConfigObject,
adapter.buildCommand, and the OpenCode adapter implementation) so launches
either include the MCP wiring or fail fast with a warning.
- Around line 620-642: The meeting-path currently sets commandToUse and
sessionProvider but never sets the session/model value from provider-backed
adapters; fetch adapterConfig.api?.model (like startAgent/startEphemeralAgent
do) and assign it to the session's model variable before returning/creating the
session so provider-backed meetings report the model; update the code inside the
adapter.isCLIBased branch (and the equivalent non-CLI branch around the 678-684
region) to mirror the model assignment logic used by startAgent() and
startEphemeralAgent(), using adapterConfig.api?.model as the source.
- Around line 42-50: configureConcurrency currently only adds 'http-api' rules
via concurrencyLimiter.addRule and never reads providers.*.maxConcurrency,
causing mismatch with acquire (which uses adapter.type + api.model) and leading
to missing or duplicate rules when startAgent calls it repeatedly; update
configureConcurrency to iterate configured providers and their maxConcurrency,
build rules keyed by adapter.type and api.model (the same keys used in acquire),
and replace or load rules atomically (e.g., clear existing provider/model rules
before adding) so rules are created once per provider+model and avoid
accumulating stale duplicates; ensure the implementation references
configureConcurrency, acquire, concurrencyLimiter.addRule, and startAgent to
keep keys consistent.
- Around line 241-245: The createComplete Promise can hang if
this.concurrencyLimiter.acquire(...) rejects (e.g., adapter.buildCommand() or
createTmuxSession() throws) because createComplete is never rejected; update
startAgent to propagate acquirePromise rejections into createComplete by either
wiring acquirePromise.catch(err => rejectCreateComplete(err)) when creating
createComplete, or replace await createComplete with awaiting
Promise.race([createComplete, acquirePromise]) so any rejection from
this.concurrencyLimiter.acquire(providerKey, modelKey, agent.role, execute) will
reject startAgent; reference the symbols acquirePromise, createComplete,
this.concurrencyLimiter.acquire, execute, adapter.buildCommand,
createTmuxSession, and startAgent when making the change.
- Around line 223-225: The provider path currently calls getLogDir() and
createTmuxSession() before this.projectRoot is assigned, causing logs to be
written to the process CWD and preventing the script-file fallback for very long
tmux commands; to fix, assign this.projectRoot = projectRoot immediately after
computing projectRoot and before invoking the provider.execute() branch so
getLogDir() reflects the correct root, and ensure the logic in
createTmuxSession() (or the code that builds fullCmd) follows the existing
guideline to split tmux command arguments > ~16KB into a shell script and
execute via bash /path rather than inlining a huge command string.
In `@tests/e2e/dashboard-api.e2e.test.ts`:
- Around line 157-195: The test currently never asserts provider/model and a
blank providers object passes; update the test to (1) configure the session to
use MockTmuxAdapter/explicit CLI provider and model before calling
sessionManager.startEphemeralAgent (so startEphemeralAgent receives known
provider/model values), (2) assert on the returned session from
startEphemeralAgent and the runningSession derived from agentsBody that
session.provider and session.model (and runningSession.provider and
runningSession.model) are defined and equal to the expected values, and (3)
tighten the /api/status assertion to check that statusBody.providers contains
the expected provider key(s) and non-empty model info rather than only using
toBeDefined().
In `@tests/unit/core/session-manager-provider.test.ts`:
- Around line 315-333: The test currently asserts a Claude CLI fallback even
when configWithProvider() sets an HTTP provider; update the assertions in the
test that call SessionManager.startEphemeralAgent (and the similar test at lines
390-409) to expect the HTTP adapter command produced by the HTTP provider rather
than checking for 'claude'. Locate usage of SessionManager (constructor/instance
sm), MockTmuxAdapter.createCalls and the variable cmd in these tests and replace
the expect(cmd).toContain('claude') checks with an assertion that verifies the
HTTP adapter command (e.g., check for the HTTP adapter executable or URL pattern
that the HTTP provider should use based on configWithProvider()).
---
Outside diff comments:
In `@src/core/agent/prompt-builder.ts`:
- Around line 25-30: The language rule is pushed after the agent template so it
isn't the topmost system instruction; update the assembly order in the prompt
builder (where parts.push(agent.system_prompt_template) and
parts.push(this.buildLanguageRule(config)) are called) so that
this.buildLanguageRule(config) is pushed before agent.system_prompt_template,
ensuring the working language from config.localization is the first system-level
prompt element.
- Around line 13-23: Add an optional providerType?: string to
buildSystemPrompt(params) and forward it into buildSkillsSection(agent,
providerType) inside buildSystemPrompt; then update each session starter
(startAgent, startEphemeralAgent, startMeetingSession) to determine the provider
type before building the system prompt by calling getProviderForAgent(config,
agent.role), creating the provider adapter (createProviderAdapter or equivalent)
and extracting adapter.type, then pass that type into buildSystemPrompt; reorder
the existing logic so provider lookup/adapter creation → const providerType =
adapter.type → buildSystemPrompt({... , providerType }) happens before any
prompt construction or session creation.
In `@src/core/session/session-manager.ts`:
- Around line 130-135: The code uses the bare role as the tmux session name;
change it to the required ip-<role> pattern by prefixing the role with "ip-"
wherever session names are constructed or looked up (e.g., the sessionName
variable used with this.tmux.hasSession and this.activeSessions.get), and ensure
the same ip-<role> string is used when calling createTmuxSession(),
killSession(), and in any log messages; update all references so session naming
is consistent and enforces one-session-per-role.
- Around line 325-328: stopAll() currently kills tmux sessions via
tmux.killAllSessions() and clears activeSessions but does not resolve pending
slotResolvers/createCompleteResolvers, leaving ConcurrencyLimiter.execute()
promises unresolved and counts stuck; update stopAll() to, in a finally block
after killing sessions, iterate and call each resolver in slotResolvers and
createCompleteResolvers (resolving with an appropriate error or undefined), then
clear those maps/arrays so pending execute() callers are settled and the
limiter's counts can release; reference stopAll(), tmux.killAllSessions(),
slotResolvers, createCompleteResolvers, and ConcurrencyLimiter.execute() when
making the change.
---
Nitpick comments:
In `@src/cli/utils/prompts.ts`:
- Around line 114-129: There are two declarations of the interface
ProviderSetupAnswers causing an accidental merge; remove the first (simpler)
declaration and keep the second one that includes providerChoice so
ProviderSetupAnswers (and usages) only exist once alongside the ProviderChoice
type; update imports/usages if any tooling flagged the duplicate after removal.
In `@tests/unit/cli/setup-provider.test.ts`:
- Around line 94-107: The test currently validates a hard-coded rules array;
instead load and validate the real rules from
createZaiDefaultConfig().concurrency.rules: call createZaiDefaultConfig(), get
its concurrency.rules, then for each rule assert that new RegExp(rule.model)
does not throw and that rule.limit is > 0 (mirror the existing checks). Update
the spec in tests/unit/cli/setup-provider.test.ts to reference
createZaiDefaultConfig() and iterate its .concurrency.rules so any invalid
regexes in the actual config fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feb729c7-6d9f-412c-8e2c-5767664d63de
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdsrc/cli/commands/setup.tssrc/cli/utils/prompts.tssrc/core/agent/prompt-builder.tssrc/core/project/config.tssrc/core/session/provider-adapter.tssrc/core/session/providers/claude-code-adapter.tssrc/core/session/providers/http-api-adapter.tssrc/core/session/providers/opencode-adapter.tssrc/core/session/providers/provider-adapter.tssrc/core/session/session-manager.tssrc/dashboard/server.tstests/e2e/dashboard-api.e2e.test.tstests/unit/cli/setup-provider.test.tstests/unit/core/concurrency-limiter.test.tstests/unit/core/config-zai.test.tstests/unit/core/prompt-builder-skills.test.tstests/unit/core/provider-factory.test.tstests/unit/core/session-manager-provider.test.ts
💤 Files with no reviewable changes (1)
- src/core/session/providers/provider-adapter.ts
| ## 🏗️ 아키텍처 | ||
| ## 아키텍처 | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language to the architecture code fence.
Markdownlint flags this fenced block. Use text for the ASCII diagram.
-```
+```text🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 198-198: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 198, The fenced code block in README.md is missing a
language specifier which triggers markdownlint; update the opening
triple-backtick for the ASCII architecture diagram (the code fence that
currently starts with ```) to include the language tag text (i.e., change ``` to
```text) so the block is marked as plain text for the diagram.
Comment on lines
+47
to
+49
| if (choice === 'claude-code') { | ||
| config.default_provider = 'claude-code'; | ||
| logger.info('Claude Code CLI를 사용합니다.'); |
There was a problem hiding this comment.
Clear provider routing when selecting Claude Code.
Line 48 only changes default_provider; existing agent_providers.mapping from a previous ZAI/OpenCode setup will still win during provider resolution, so rerunning setup and choosing Claude can continue routing agents to stale providers.
Proposed fix
if (choice === 'claude-code') {
config.default_provider = 'claude-code';
+ config.agent_providers = { mapping: {}, overrides: {} };
+ config.providers = {};
+ config.concurrency = { defaults: {}, rules: [] };
logger.info('Claude Code CLI를 사용합니다.');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (choice === 'claude-code') { | |
| config.default_provider = 'claude-code'; | |
| logger.info('Claude Code CLI를 사용합니다.'); | |
| if (choice === 'claude-code') { | |
| config.default_provider = 'claude-code'; | |
| config.agent_providers = { mapping: {}, overrides: {} }; | |
| config.providers = {}; | |
| config.concurrency = { defaults: {}, rules: [] }; | |
| logger.info('Claude Code CLI를 사용합니다.'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/setup.ts` around lines 47 - 49, When the user selects
'claude-code' the setup currently only sets config.default_provider but leaves
any existing config.agent_providers.mapping in place, causing stale provider
routing; update the setup branch that handles choice === 'claude-code' (the code
that sets config.default_provider) to also clear or reset
config.agent_providers.mapping (e.g., assign an empty object or remove mappings)
so that agent provider resolution no longer prefers old ZAI/OpenCode entries.
Comment on lines
+120
to
+138
| } else if (choice === 'mixed-opencode-zai') { | ||
| config.default_provider = 'zai-general'; | ||
| config.providers = { | ||
| opencode: { type: 'opencode', binary: 'opencode', maxConcurrency: 1 }, | ||
| 'zai-coding': { | ||
| type: 'http-api', | ||
| api: { baseUrl, apiKey: providerAnswers.apiKey, model: providerAnswers.codingModel ?? 'glm-5-turbo', headers: {} }, | ||
| maxConcurrency: 1, | ||
| }, | ||
| 'zai-general': { | ||
| type: 'http-api', | ||
| api: { baseUrl, apiKey: providerAnswers.apiKey, model: providerAnswers.generalModel ?? 'glm-4.5', headers: {} }, | ||
| maxConcurrency: 10, | ||
| }, | ||
| }; | ||
| config.agent_providers = { | ||
| mapping: { cto: 'opencode', ceo: 'zai-general', po: 'zai-general', designer: 'zai-general', qa: 'zai-general', marketer: 'zai-general' }, | ||
| overrides: {}, | ||
| }; |
There was a problem hiding this comment.
Give the mixed OpenCode provider an explicit model.
Line 123 configures CTO on OpenCode without api.model, so the adapter cannot emit the intended model flag and the session/concurrency layer cannot track CTO by model. This also diverges from the model-distribution behavior configured for the pure OpenCode path.
Proposed fix
- opencode: { type: 'opencode', binary: 'opencode', maxConcurrency: 1 },
+ opencode: {
+ type: 'opencode',
+ binary: 'opencode',
+ api: { model: 'zai-coding-plan/glm-5.1', headers: {} },
+ maxConcurrency: 1,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (choice === 'mixed-opencode-zai') { | |
| config.default_provider = 'zai-general'; | |
| config.providers = { | |
| opencode: { type: 'opencode', binary: 'opencode', maxConcurrency: 1 }, | |
| 'zai-coding': { | |
| type: 'http-api', | |
| api: { baseUrl, apiKey: providerAnswers.apiKey, model: providerAnswers.codingModel ?? 'glm-5-turbo', headers: {} }, | |
| maxConcurrency: 1, | |
| }, | |
| 'zai-general': { | |
| type: 'http-api', | |
| api: { baseUrl, apiKey: providerAnswers.apiKey, model: providerAnswers.generalModel ?? 'glm-4.5', headers: {} }, | |
| maxConcurrency: 10, | |
| }, | |
| }; | |
| config.agent_providers = { | |
| mapping: { cto: 'opencode', ceo: 'zai-general', po: 'zai-general', designer: 'zai-general', qa: 'zai-general', marketer: 'zai-general' }, | |
| overrides: {}, | |
| }; | |
| } else if (choice === 'mixed-opencode-zai') { | |
| config.default_provider = 'zai-general'; | |
| config.providers = { | |
| opencode: { | |
| type: 'opencode', | |
| binary: 'opencode', | |
| api: { model: 'zai-coding-plan/glm-5.1', headers: {} }, | |
| maxConcurrency: 1, | |
| }, | |
| 'zai-coding': { | |
| type: 'http-api', | |
| api: { baseUrl, apiKey: providerAnswers.apiKey, model: providerAnswers.codingModel ?? 'glm-5-turbo', headers: {} }, | |
| maxConcurrency: 1, | |
| }, | |
| 'zai-general': { | |
| type: 'http-api', | |
| api: { baseUrl, apiKey: providerAnswers.apiKey, model: providerAnswers.generalModel ?? 'glm-4.5', headers: {} }, | |
| maxConcurrency: 10, | |
| }, | |
| }; | |
| config.agent_providers = { | |
| mapping: { cto: 'opencode', ceo: 'zai-general', po: 'zai-general', designer: 'zai-general', qa: 'zai-general', marketer: 'zai-general' }, | |
| overrides: {}, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/setup.ts` around lines 120 - 138, The mixed "opencode"
provider lacks an explicit model, so update config.providers for the 'opencode'
entry to include an api.model (mirror the coding model used for 'zai-coding') so
the adapter can emit the model flag and the session/concurrency layer can track
CTO by model; specifically modify the config.providers 'opencode' object (the
one referenced by agent_providers.mapping's 'cto') to include an api.model set
from providerAnswers.codingModel ?? 'glm-5-turbo' (or the project’s default
coding model).
Comment on lines
+127
to
+136
| export function getProviderForAgent(config: ProjectConfig, role: string): ProviderConfigType | null { | ||
| const mapping = config.agent_providers?.mapping ?? {}; | ||
| const providerName = mapping[role] ?? config.default_provider; | ||
|
|
||
| if (!providerName || providerName === 'claude-code') return null; | ||
|
|
||
| const overrides = config.agent_providers?.overrides ?? {}; | ||
| if (overrides[role]) return overrides[role]; | ||
|
|
||
| return config.providers[providerName] ?? null; |
There was a problem hiding this comment.
Apply role overrides before mapped/default provider resolution.
Line 131 returns null for claude-code/missing defaults before Line 134 checks overrides, so a role-specific override is ignored unless the role also has a non-Claude mapping/default. That contradicts the intended precedence: overrides → mapping → default.
🐛 Proposed fix
export function getProviderForAgent(config: ProjectConfig, role: string): ProviderConfigType | null {
+ const overrides = config.agent_providers?.overrides ?? {};
+ if (overrides[role]) return overrides[role];
+
const mapping = config.agent_providers?.mapping ?? {};
const providerName = mapping[role] ?? config.default_provider;
if (!providerName || providerName === 'claude-code') return null;
- const overrides = config.agent_providers?.overrides ?? {};
- if (overrides[role]) return overrides[role];
-
return config.providers[providerName] ?? null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getProviderForAgent(config: ProjectConfig, role: string): ProviderConfigType | null { | |
| const mapping = config.agent_providers?.mapping ?? {}; | |
| const providerName = mapping[role] ?? config.default_provider; | |
| if (!providerName || providerName === 'claude-code') return null; | |
| const overrides = config.agent_providers?.overrides ?? {}; | |
| if (overrides[role]) return overrides[role]; | |
| return config.providers[providerName] ?? null; | |
| export function getProviderForAgent(config: ProjectConfig, role: string): ProviderConfigType | null { | |
| const overrides = config.agent_providers?.overrides ?? {}; | |
| if (overrides[role]) return overrides[role]; | |
| const mapping = config.agent_providers?.mapping ?? {}; | |
| const providerName = mapping[role] ?? config.default_provider; | |
| if (!providerName || providerName === 'claude-code') return null; | |
| return config.providers[providerName] ?? null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/project/config.ts` around lines 127 - 136, The getProviderForAgent
function currently returns early for missing/default or 'claude-code' before
honoring role-specific overrides; change the logic in getProviderForAgent so
that you first check config.agent_providers?.overrides[role] and return it if
present (overrides → mapping → default), then resolve providerName from mapping
or default and only then apply the guard that returns null for missing or
'claude-code'; update references to mapping, overrides, providerName, and
config.providers accordingly.
Comment on lines
+77
to
+80
| // 모델이 지정된 경우에만 --model 추가 | ||
| if (this.model) { | ||
| parts.push('--model', this.model); | ||
| } |
There was a problem hiding this comment.
Quote the configured model argument before shell execution.
this.model is config-derived and currently appended raw. A model value containing whitespace or shell metacharacters can break the command or inject extra shell syntax.
🛡️ Proposed fix
// 모델이 지정된 경우에만 --model 추가
if (this.model) {
- parts.push('--model', this.model);
+ parts.push('--model', `'${this.escapeSingleQuote(this.model)}'`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/session/providers/opencode-adapter.ts` around lines 77 - 80, The
code pushes this.model directly into the shell argument array (parts) which can
allow whitespace or shell metacharacters to break or inject into the executed
command; update the logic around where parts.push('--model', this.model) is used
(the code that builds the parts array in the Opencode adapter) to ensure the
model value is safely shell-quoted or escaped before being appended (e.g., use a
robust escaping/quoting helper or wrap the model in a safe-quote function) so
the resulting argument is treated as a single literal token by the shell.
Comment on lines
+223
to
+225
| const logFile = path.join(this.getLogDir(), `${sessionName}.log`); | ||
| const fullCmd = `cd '${projectRoot}' && ${commandToUse} 2>&1 | tee '${logFile}'`; | ||
| await this.createTmuxSession(sessionName, fullCmd); |
There was a problem hiding this comment.
Initialize projectRoot before provider tmux creation.
In the provider path, getLogDir() and createTmuxSession() run before Line 269 sets this.projectRoot. That can write logs to the process CWD and, for long commands, skip the script-file fallback because getLogDir() returns empty. Set this.projectRoot = projectRoot before entering the provider execute() path. As per coding guidelines, tmux command arguments longer than ~16KB must be split into separate shell script files and executed via bash /path.
Minimal fix
const sessionId = randomUUID();
const escapedPrompt = systemPrompt.replace(/'/g, "'\\''");
+ this.projectRoot = projectRoot;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/session/session-manager.ts` around lines 223 - 225, The provider
path currently calls getLogDir() and createTmuxSession() before this.projectRoot
is assigned, causing logs to be written to the process CWD and preventing the
script-file fallback for very long tmux commands; to fix, assign
this.projectRoot = projectRoot immediately after computing projectRoot and
before invoking the provider.execute() branch so getLogDir() reflects the
correct root, and ensure the logic in createTmuxSession() (or the code that
builds fullCmd) follows the existing guideline to split tmux command arguments >
~16KB into a shell script and execute via bash /path rather than inlining a huge
command string.
Comment on lines
+241
to
+245
| // enqueue/acquire slot — don't await full acquire (which resolves when session ends) | ||
| // but wait until the tmux session creation step finishes so callers observe created session. | ||
| const acquirePromise = this.concurrencyLimiter.acquire(providerKey, modelKey, agent.role, execute); | ||
| // wait until tmux session is created inside execute | ||
| await createComplete; |
There was a problem hiding this comment.
Propagate provider creation failures instead of waiting forever.
If adapter.buildCommand() or createTmuxSession() throws inside execute(), acquirePromise rejects but createComplete is never resolved/rejected. startAgent() then hangs on await createComplete. Wire the acquire rejection into the create-complete promise or race it explicitly.
Potential direction
- const createComplete = new Promise<void>((resolve) => this.createCompleteResolvers.set(agent.role, resolve));
+ let rejectCreateComplete!: (err: unknown) => void;
+ const createComplete = new Promise<void>((resolve, reject) => {
+ rejectCreateComplete = reject;
+ this.createCompleteResolvers.set(agent.role, resolve);
+ });
...
const acquirePromise = this.concurrencyLimiter.acquire(providerKey, modelKey, agent.role, execute);
+ acquirePromise.catch((err) => {
+ this.createCompleteResolvers.delete(agent.role);
+ rejectCreateComplete(err);
+ });
// wait until tmux session is created inside execute
await createComplete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/session/session-manager.ts` around lines 241 - 245, The
createComplete Promise can hang if this.concurrencyLimiter.acquire(...) rejects
(e.g., adapter.buildCommand() or createTmuxSession() throws) because
createComplete is never rejected; update startAgent to propagate acquirePromise
rejections into createComplete by either wiring acquirePromise.catch(err =>
rejectCreateComplete(err)) when creating createComplete, or replace await
createComplete with awaiting Promise.race([createComplete, acquirePromise]) so
any rejection from this.concurrencyLimiter.acquire(providerKey, modelKey,
agent.role, execute) will reject startAgent; reference the symbols
acquirePromise, createComplete, this.concurrencyLimiter.acquire, execute,
adapter.buildCommand, createTmuxSession, and startAgent when making the change.
Comment on lines
+620
to
+642
| let commandToUse: string; | ||
| let sessionProvider: string | undefined; | ||
|
|
||
| if (configProvider) { | ||
| const adapterConfig = this.toAdapterConfig(configProvider); | ||
| if (adapterConfig.api?.apiKey && adapterConfig.api.apiKey.includes('${')) { | ||
| throw new Error(`Provider for role '${initiator.role}' has unresolved environment variable in API key: ${adapterConfig.api.apiKey}`); | ||
| } | ||
|
|
||
| const adapter = createProviderAdapter(adapterConfig); | ||
| // Meetings require filesystem access; if adapter is CLI-based use it, otherwise fall back to Claude CLI | ||
| if (adapter.isCLIBased) { | ||
| const mcpConfig = this.buildMcpConfigObject(initiator, config); | ||
| const buildResult = adapter.buildMeetingCommand({ | ||
| systemPrompt, | ||
| sessionId, | ||
| projectRoot, | ||
| meetingAgenda: taskInstruction, | ||
| mcpConfig, | ||
| }); | ||
| commandToUse = buildResult.command; | ||
| sessionProvider = adapter.type; | ||
| } else { |
There was a problem hiding this comment.
Populate model for meeting sessions too.
Meeting sessions record provider but drop adapterConfig.api?.model, so dashboard/API model reporting is incomplete for provider-backed meetings. Mirror the startAgent() and startEphemeralAgent() behavior.
Minimal fix
let commandToUse: string;
let sessionProvider: string | undefined;
+ let sessionModel: string | undefined;
...
commandToUse = buildResult.command;
sessionProvider = adapter.type;
+ sessionModel = adapterConfig.api?.model;
...
status: 'working',
startedAt: new Date().toISOString(),
provider: sessionProvider,
+ model: sessionModel,
};Also applies to: 678-684
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/session/session-manager.ts` around lines 620 - 642, The meeting-path
currently sets commandToUse and sessionProvider but never sets the session/model
value from provider-backed adapters; fetch adapterConfig.api?.model (like
startAgent/startEphemeralAgent do) and assign it to the session's model variable
before returning/creating the session so provider-backed meetings report the
model; update the code inside the adapter.isCLIBased branch (and the equivalent
non-CLI branch around the 678-684 region) to mirror the model assignment logic
used by startAgent() and startEphemeralAgent(), using adapterConfig.api?.model
as the source.
Comment on lines
+157
to
+195
| it('Provider/model fields in agent response and /api/status providers summary', async () => { | ||
| // Start an agent using the SessionManager injected in beforeEach via DashboardServer constructor | ||
| // We'll start CTO with provider override from default project config (createZaiDefaultConfig used by ProjectManager.init) | ||
| const sessionManager = (dashboard as any).sessionManager as SessionManager; | ||
| const pm = new ProjectManager(project.root); | ||
| const config = await pm.loadConfig(); | ||
|
|
||
| // Start CTO agent session (ephemeral start not needed) — use startAgent to ensure provider/model are populated | ||
| const agentRegistry = await (async () => { const reg = (dashboard as any).agentRegistry as any; await reg.load(); return reg; })(); | ||
| const agents = agentRegistry.getAll(); | ||
| const ctoConfig = agents.find((a: any) => a.role === 'cto'); | ||
| expect(ctoConfig).toBeDefined(); | ||
|
|
||
| // startAgent requires many params; we'll call startEphemeralAgent which also sets provider/model | ||
| const session = await sessionManager.startEphemeralAgent({ | ||
| sessionName: 'cto-test', | ||
| agent: ctoConfig, | ||
| config, | ||
| projectRoot: project.root, | ||
| message: 'test provider model exposure', | ||
| }); | ||
|
|
||
| // verify /api/agents includes provider/model on current_session | ||
| const agentsRes = await fetch(`${baseUrl}/api/agents`); | ||
| expect(agentsRes.status).toBe(200); | ||
| const agentsBody = await agentsRes.json() as Array<any>; | ||
| const cto = agentsBody.find(a => a.role === 'cto'); | ||
| expect(cto).toBeDefined(); | ||
| // current_session may be null if sessionName differs; check for provider/model in running sessions list instead | ||
| const runningSession = agentsBody.map(a => a.current_session).find(s => s && s.role === 'cto-test'); | ||
| // For CLI-based adapters provider/model may be undefined; ensure fields exist (may be undefined) on session object | ||
| const statusRes = await fetch(`${baseUrl}/api/status`); | ||
| expect(statusRes.status).toBe(200); | ||
| const statusBody = await statusRes.json() as any; | ||
| expect(statusBody.providers).toBeDefined(); | ||
|
|
||
| // cleanup: stop the ephemeral session we created | ||
| await sessionManager.stopAgent('cto-test'); | ||
| }); |
There was a problem hiding this comment.
Assert the provider/model behavior this test is meant to cover.
runningSession and session are never asserted, and expect(statusBody.providers).toBeDefined() passes even for {}. This test can pass while provider/model exposure is broken.
🧪 Proposed assertion tightening
- const runningSession = agentsBody.map(a => a.current_session).find(s => s && s.role === 'cto-test');
- // For CLI-based adapters provider/model may be undefined; ensure fields exist (may be undefined) on session object
+ expect(session.role).toBe('cto-test');
+
const statusRes = await fetch(`${baseUrl}/api/status`);
expect(statusRes.status).toBe(200);
const statusBody = await statusRes.json() as any;
expect(statusBody.providers).toBeDefined();
+ expect(statusBody.agents_running).toEqual(
+ expect.arrayContaining([
+ expect.objectContaining({ role: 'cto-test' }),
+ ]),
+ );
+ expect(Object.values(statusBody.providers).reduce((sum: number, count: any) => sum + count, 0)).toBeGreaterThan(0);If the intent is to validate non-undefined provider/model, configure this test with a CLI provider/model explicitly before starting the session, then assert those exact values.
Based on learnings, “E2E tests must use MockTmuxAdapter for tmux mocking while keeping file system, chokidar, and Express as real implementations”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('Provider/model fields in agent response and /api/status providers summary', async () => { | |
| // Start an agent using the SessionManager injected in beforeEach via DashboardServer constructor | |
| // We'll start CTO with provider override from default project config (createZaiDefaultConfig used by ProjectManager.init) | |
| const sessionManager = (dashboard as any).sessionManager as SessionManager; | |
| const pm = new ProjectManager(project.root); | |
| const config = await pm.loadConfig(); | |
| // Start CTO agent session (ephemeral start not needed) — use startAgent to ensure provider/model are populated | |
| const agentRegistry = await (async () => { const reg = (dashboard as any).agentRegistry as any; await reg.load(); return reg; })(); | |
| const agents = agentRegistry.getAll(); | |
| const ctoConfig = agents.find((a: any) => a.role === 'cto'); | |
| expect(ctoConfig).toBeDefined(); | |
| // startAgent requires many params; we'll call startEphemeralAgent which also sets provider/model | |
| const session = await sessionManager.startEphemeralAgent({ | |
| sessionName: 'cto-test', | |
| agent: ctoConfig, | |
| config, | |
| projectRoot: project.root, | |
| message: 'test provider model exposure', | |
| }); | |
| // verify /api/agents includes provider/model on current_session | |
| const agentsRes = await fetch(`${baseUrl}/api/agents`); | |
| expect(agentsRes.status).toBe(200); | |
| const agentsBody = await agentsRes.json() as Array<any>; | |
| const cto = agentsBody.find(a => a.role === 'cto'); | |
| expect(cto).toBeDefined(); | |
| // current_session may be null if sessionName differs; check for provider/model in running sessions list instead | |
| const runningSession = agentsBody.map(a => a.current_session).find(s => s && s.role === 'cto-test'); | |
| // For CLI-based adapters provider/model may be undefined; ensure fields exist (may be undefined) on session object | |
| const statusRes = await fetch(`${baseUrl}/api/status`); | |
| expect(statusRes.status).toBe(200); | |
| const statusBody = await statusRes.json() as any; | |
| expect(statusBody.providers).toBeDefined(); | |
| // cleanup: stop the ephemeral session we created | |
| await sessionManager.stopAgent('cto-test'); | |
| }); | |
| it('Provider/model fields in agent response and /api/status providers summary', async () => { | |
| // Start an agent using the SessionManager injected in beforeEach via DashboardServer constructor | |
| // We'll start CTO with provider override from default project config (createZaiDefaultConfig used by ProjectManager.init) | |
| const sessionManager = (dashboard as any).sessionManager as SessionManager; | |
| const pm = new ProjectManager(project.root); | |
| const config = await pm.loadConfig(); | |
| // Start CTO agent session (ephemeral start not needed) — use startAgent to ensure provider/model are populated | |
| const agentRegistry = await (async () => { const reg = (dashboard as any).agentRegistry as any; await reg.load(); return reg; })(); | |
| const agents = agentRegistry.getAll(); | |
| const ctoConfig = agents.find((a: any) => a.role === 'cto'); | |
| expect(ctoConfig).toBeDefined(); | |
| // startAgent requires many params; we'll call startEphemeralAgent which also sets provider/model | |
| const session = await sessionManager.startEphemeralAgent({ | |
| sessionName: 'cto-test', | |
| agent: ctoConfig, | |
| config, | |
| projectRoot: project.root, | |
| message: 'test provider model exposure', | |
| }); | |
| // verify /api/agents includes provider/model on current_session | |
| const agentsRes = await fetch(`${baseUrl}/api/agents`); | |
| expect(agentsRes.status).toBe(200); | |
| const agentsBody = await agentsRes.json() as Array<any>; | |
| const cto = agentsBody.find(a => a.role === 'cto'); | |
| expect(cto).toBeDefined(); | |
| // current_session may be null if sessionName differs; check for provider/model in running sessions list instead | |
| expect(session.role).toBe('cto-test'); | |
| const statusRes = await fetch(`${baseUrl}/api/status`); | |
| expect(statusRes.status).toBe(200); | |
| const statusBody = await statusRes.json() as any; | |
| expect(statusBody.providers).toBeDefined(); | |
| expect(statusBody.agents_running).toEqual( | |
| expect.arrayContaining([ | |
| expect.objectContaining({ role: 'cto-test' }), | |
| ]), | |
| ); | |
| expect(Object.values(statusBody.providers).reduce((sum: number, count: any) => sum + count, 0)).toBeGreaterThan(0); | |
| // cleanup: stop the ephemeral session we created | |
| await sessionManager.stopAgent('cto-test'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/dashboard-api.e2e.test.ts` around lines 157 - 195, The test
currently never asserts provider/model and a blank providers object passes;
update the test to (1) configure the session to use MockTmuxAdapter/explicit CLI
provider and model before calling sessionManager.startEphemeralAgent (so
startEphemeralAgent receives known provider/model values), (2) assert on the
returned session from startEphemeralAgent and the runningSession derived from
agentsBody that session.provider and session.model (and runningSession.provider
and runningSession.model) are defined and equal to the expected values, and (3)
tighten the /api/status assertion to check that statusBody.providers contains
the expected provider key(s) and non-empty model info rather than only using
toBeDefined().
Comment on lines
+315
to
+333
| it('Ephemeral with HTTP API provider → falls back to CLI', async () => { | ||
| const tmux = new MockTmuxAdapter(); | ||
| const sm = new SessionManager(tmux); | ||
| const projectRoot = await setupProjectRoot(); | ||
|
|
||
| try { | ||
| const config = configWithProvider(); | ||
| const session = await sm.startEphemeralAgent({ | ||
| sessionName: 'ephemeral-ceo', | ||
| agent: baseAgent, | ||
| config, | ||
| projectRoot, | ||
| message: 'Please advise on roadmap', | ||
| }); | ||
|
|
||
| expect(session).toBeDefined(); | ||
| expect(tmux.createCalls.length).toBe(1); | ||
| const cmd = tmux.createCalls[0].command; | ||
| expect(cmd).toContain('claude'); |
There was a problem hiding this comment.
Don’t assert Claude fallback when an HTTP provider is configured.
These tests lock ephemeral and meeting sessions to Claude even when configWithProvider() selects http-api. That breaks provider-only setups where Claude is not installed/configured; assert the HTTP adapter command instead.
Proposed test adjustment
- it('Ephemeral with HTTP API provider → falls back to CLI', async () => {
+ it('Ephemeral with HTTP API provider → uses provider adapter', async () => {
@@
- expect(cmd).toContain('claude');
+ expect(cmd).toContain('node');
+ expect(cmd).not.toMatch(/\bclaude\b/);
@@
- it('Meeting with HTTP API provider → falls back to CLI', async () => {
+ it('Meeting with HTTP API provider → uses provider adapter', async () => {
@@
- expect(cmd).toContain('claude');
+ expect(cmd).toContain('node');
+ expect(cmd).not.toMatch(/\bclaude\b/);Also applies to: 390-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/core/session-manager-provider.test.ts` around lines 315 - 333, The
test currently asserts a Claude CLI fallback even when configWithProvider() sets
an HTTP provider; update the assertions in the test that call
SessionManager.startEphemeralAgent (and the similar test at lines 390-409) to
expect the HTTP adapter command produced by the HTTP provider rather than
checking for 'claude'. Locate usage of SessionManager (constructor/instance sm),
MockTmuxAdapter.createCalls and the variable cmd in these tests and replace the
expect(cmd).toContain('claude') checks with an assertion that verifies the HTTP
adapter command (e.g., check for the HTTP adapter executable or URL pattern that
the HTTP provider should use based on configWithProvider()).
sigco3111
requested changes
Apr 20, 2026
sigco3111
left a comment
sigco3111
left a comment
Owner
There was a problem hiding this comment.
🔍 Code Review — Request Changes
전반적으로 OpenCode 통합, 동시성 큐잉, 모델 분산 아키텍처는 잘 설계되었습니다. 테스트 커버리지도 좋습니다. 하지만 3개 Critical 이슈와 5개 Major 이슈가 있어 수정이 필요합니다.
🔴 Critical
C1. configureConcurrency가 provider를 항상 'http-api'로 하드코딩 — 모델 분산 동시성 제어 동작 안 함
- File:
session-manager.ts→configureConcurrency() provider: 'http-api'로 룰을 등록하는데, OpenCode 어댑터는type: 'opencode'를 반환합니다.getMaxConcurrency('opencode', 'glm-5.1')는 절대 매칭되지 않아 모델 분산 설정의 핵심 기능이 동작하지 않습니다.- Fix: config에서 실제 provider type을 읽어서 룰 등록, 또는 concurrency rule 매칭을 model-only로 변경
C2. configureConcurrency가 startAgent() 호출마다 실행 — 룰 무한 누적
- File:
session-manager.ts→startAgent() addRule()이 매번 push라서 100번 에이전트 시작하면 100x 중복 룰이 생깁니다. 메모리 릭이고 regex 매칭 성능이 저하됩니다.- Fix: constructor/setProjectRoot/init에 한 번만 호출, 또는
resetRules()추가
C3. stopAgent의 폴링 루프 race condition — 동시성 슬롯 해제 불안정
- File:
session-manager.ts→stopAgent() slotResolver()로 resolve 후,ConcurrencyLimiter의.finally(() => this.release(...))가 마이크로태스크로 스케줄링되는데, 50×20ms 폴링 윈도우 안에 실행된다는 보장이 없습니다. 타임아웃되면 다음startAgent()가 큐에서 영원히 대기하게 됩니다. 이 PR이 고치려던 버그와 동일한 클래스의 버그입니다.- Fix: 폴링 대신
release이벤트/콜백 패턴 사용, 또는acquire()promise를 직접 await
🟠 Major
M1. prompts.ts에 ProviderSetupAnswers 인터페이스 중복 정의
- 첫 번째 (incomplete) 선언을 제거하세요. TypeScript는 merge하지만 유지보수에 혼란을 줍니다.
M2. resolveEnvVars가 apiKey에만 적용 — baseUrl, binary 무시
baseUrl에도${VAR}사용 가능하게 하거나, 모든 string 필드에 recursive 적용하세요.
M3. getProviderForAgent가 'claude-code' 문자열을 sentinel로 사용
- config에
claude-code라는 이름의 프로바이더를 정의하면 null 반환. provider type이 아닌 존재 여부로 체크하세요.
M4. Ephemeral/Meeting 세션이 동시성 리미터를 우회
startEphemeralAgent(),startMeetingSession()에acquire()호출이 없습니다. 의도적이라면 문서화 필요.
M5. hasInstruction 타입 가드가 'instruction' in skill로 취약
- 명시적으로
(skill as any).instruction !== undefined체크가 더 안전합니다.
💡 Suggestions
- S1:
startEphemeralAgent,startMeetingSession,startAgent의 프로바이더 명령 생성 로직이 3중 복제 → 공통 메서드 추출 - S2:
acquire()에AbortSignal또는 timeout 추가 - S3:
configureConcurrency중복 호출 테스트 추가 - S4:
loadConfig()시점에validateProviderConfig()호출하여 조기 검증 - S5:
stopAll()에서 pending slot resolvers resolve 누락
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
opencode run서브커맨드 사용, 기존에 잘못된--agent/-p플래그 제거api.baseUrl불필요 (model만 지정 가능)Changes
Commit 1:
b0caff6— fix: OpenCode adapteropencode --agent '...' -p '...'→opencode run --dangerously-skip-permissions '...'Commit 2:
4e703fc— fix: concurrency queuingstartAgent()의 pre-check throw 제거 →acquire()큐잉으로 대기api.baseUrl,api.modeloptional화 (opencode는 model만 필요)Commit 3:
de424a7— feat: model-diverse setupTest Plan
npm test)npm run build)glm-4.7표시 확인Ultraworked with Sisyphus
Summary by CodeRabbit
New Features
Improvements