Support thinking levels for subagent profiles#60
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an optional ChangesThinking Level Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as devspace agents run
participant Targets as resolveLocalAgentTarget
participant Store as LocalAgentStore
participant Runtime as LocalAgentRuntime
participant Adapter as LocalAgentAdapter
participant Provider as Provider runtime
CLI->>Targets: parse --thinking and resolve target
Targets-->>CLI: thinking value
CLI->>Store: create/update session with thinking
Store-->>CLI: persisted record
CLI->>Runtime: runLocalAgentProvider(thinking)
Runtime->>Adapter: pass modelReasoningEffort / provider option
Adapter->>Provider: apply thinking setting
Provider-->>Adapter: run result
Adapter-->>Runtime: provider response
Runtime-->>CLI: execution output
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/local-agent-runtime.ts (1)
57-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
thinkingbefore passing it to Codex.
input.thinkingis an untyped string here, butModelReasoningEffortonly allowsminimal,low,medium,high, andxhigh. Reject unsupported values up front so typos or provider-specific values likemaxfail with a clear DevSpace error instead of surfacing later from the SDK.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-runtime.ts` around lines 57 - 65, `threadOptionsFor` is passing the untyped `input.thinking` straight into `modelReasoningEffort`, so invalid values can reach Codex unnoticed. Add upfront validation in `threadOptionsFor` (or a small helper it calls) to only accept the allowed `ModelReasoningEffort` values: minimal, low, medium, high, and xhigh. If `input.thinking` is anything else, reject it with a clear DevSpace error before building the `ThreadOptions`, so bad inputs fail immediately instead of later in the SDK.
🧹 Nitpick comments (3)
src/local-agent-store.test.ts (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
update()persistence ofthinkingtoo.This only verifies
thinkinground-trips throughcreate()/get(). Theupdate()method has its own separate SQL statement (src/local-agent-store.tslines 165-194) with independent column ordering/placeholder counts, so a regression there (e.g., a misplaced?) wouldn't be caught by this test alone.♻️ Suggested addition
assert.equal(store.get(created.id)?.thinking, "high"); + + const updated = store.update(created.id, { thinking: "medium" }); + assert.equal(updated.thinking, "medium"); + assert.equal(store.get(created.id)?.thinking, "medium");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-store.test.ts` around lines 19 - 24, The current test only covers `create()`/`get()` for `thinking`, so it won’t catch regressions in `LocalAgentStore.update()`’s separate SQL path. Extend `src/local-agent-store.test.ts` to update an existing agent via `update()` and then assert the persisted `thinking` value is returned by `get()`/readback, using the `LocalAgentStore` methods and `created.id` to verify the update round-trips correctly.skills/subagent-delegation/SKILL.md (1)
51-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting that valid
--thinkinglevels are provider-specific.The doc doesn't mention that accepted
<level>values differ per provider (e.g. Codex'sminimal/low/medium/high/xhighvs. Claude'slow/medium/high/max, ACP's dynamically-discovered options). Given the underlying code caststhinkingwithout cross-provider validation for Codex/Claude (seesrc/local-agent-runtime.tsandsrc/local-agent-adapters.ts), an agent following this doc could pass an invalid level for the chosen provider and only find out via a provider-side error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/subagent-delegation/SKILL.md` around lines 51 - 62, Clarify that the accepted values for the `--thinking` flag are provider-specific and may differ between Codex, Claude, and ACP. Update the guidance around `devspace agents run` in SKILL.md to mention that users must choose a level supported by the selected provider, and reference the `--thinking` option along with the relevant runtime/adapters behavior in `src/local-agent-runtime.ts` and `src/local-agent-adapters.ts` so the documentation makes this constraint explicit.src/local-agent-adapters.test.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing error-path coverage for
resolveAcpThinkingConfigUpdate.Tests cover the two valid-mapping cases and one "unsupported value" throw, but not the "session did not return session metadata", "session did not return a session id", or "ACP server does not expose a thinking option" branches. Since this helper is now exported and used by both the
cursorandcopilotadapters, covering those error paths would guard against regressions.Also applies to: 31-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-adapters.test.ts` at line 11, Add missing negative-path tests for resolveAcpThinkingConfigUpdate in local-agent-adapters.test.ts: the current coverage only verifies valid mappings and one unsupported-value throw, but it should also assert the branches where session metadata is absent, the session id is missing, and the ACP server does not expose a thinking option. Reuse resolveAcpThinkingConfigUpdate directly in the test cases and ensure the cursor and copilot adapter scenarios are represented so these exported helper error paths stay covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/local-agent-adapters.ts`:
- Line 4: Validate input.thinking in local-agent-adapters before forwarding it
to Claude, since the current cast can let unsupported values from profiles or
CLI reach the SDK. Use the same allowed-options check ACP uses for the session,
and only pass through a supported EffortLevel in the local adapter path. Locate
this in the code that imports EffortLevel and builds the Claude request payload,
ensuring invalid thinking values are rejected or omitted before the SDK call.
---
Outside diff comments:
In `@src/local-agent-runtime.ts`:
- Around line 57-65: `threadOptionsFor` is passing the untyped `input.thinking`
straight into `modelReasoningEffort`, so invalid values can reach Codex
unnoticed. Add upfront validation in `threadOptionsFor` (or a small helper it
calls) to only accept the allowed `ModelReasoningEffort` values: minimal, low,
medium, high, and xhigh. If `input.thinking` is anything else, reject it with a
clear DevSpace error before building the `ThreadOptions`, so bad inputs fail
immediately instead of later in the SDK.
---
Nitpick comments:
In `@skills/subagent-delegation/SKILL.md`:
- Around line 51-62: Clarify that the accepted values for the `--thinking` flag
are provider-specific and may differ between Codex, Claude, and ACP. Update the
guidance around `devspace agents run` in SKILL.md to mention that users must
choose a level supported by the selected provider, and reference the
`--thinking` option along with the relevant runtime/adapters behavior in
`src/local-agent-runtime.ts` and `src/local-agent-adapters.ts` so the
documentation makes this constraint explicit.
In `@src/local-agent-adapters.test.ts`:
- Line 11: Add missing negative-path tests for resolveAcpThinkingConfigUpdate in
local-agent-adapters.test.ts: the current coverage only verifies valid mappings
and one unsupported-value throw, but it should also assert the branches where
session metadata is absent, the session id is missing, and the ACP server does
not expose a thinking option. Reuse resolveAcpThinkingConfigUpdate directly in
the test cases and ensure the cursor and copilot adapter scenarios are
represented so these exported helper error paths stay covered.
In `@src/local-agent-store.test.ts`:
- Around line 19-24: The current test only covers `create()`/`get()` for
`thinking`, so it won’t catch regressions in `LocalAgentStore.update()`’s
separate SQL path. Extend `src/local-agent-store.test.ts` to update an existing
agent via `update()` and then assert the persisted `thinking` value is returned
by `get()`/readback, using the `LocalAgentStore` methods and `created.id` to
verify the update round-trips correctly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5b6fad4-666f-4391-88b9-9d5484f3c0e7
📒 Files selected for processing (21)
docs/agent-profile-schema.mddocs/chatgpt-coding-workflow.mddocs/configuration.mdexamples/agents/claude-implementer.mdexamples/agents/pi-reviewer.mdskills/subagent-delegation/SKILL.mdsrc/cli.test.tssrc/cli.tssrc/db/migrations.tssrc/db/schema.tssrc/local-agent-adapters.test.tssrc/local-agent-adapters.tssrc/local-agent-profiles.test.tssrc/local-agent-profiles.tssrc/local-agent-runtime.test.tssrc/local-agent-runtime.tssrc/local-agent-store.test.tssrc/local-agent-store.tssrc/local-agent-targets.test.tssrc/local-agent-targets.tssrc/server.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/local-agent-adapters.test.ts (1)
32-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid coverage for the ACP select-config resolvers.
Assertions for
resolveAcpModelConfigUpdateandresolveAcpThinkingConfigUpdatecorrectly exercise flat-select, grouped-select, and invalid-value error paths, matching the shared resolver contract ({sessionId, configId, value}and the "Available values" error suffix).One optional note: the two blocks (32-94 and 95-157) are structurally near-identical, differing only in category/fixture data. Consider extracting a small parameterized helper to reduce duplication, though this is purely a maintainability nit given the current size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-adapters.test.ts` around lines 32 - 157, The test blocks for resolveAcpModelConfigUpdate and resolveAcpThinkingConfigUpdate are duplicated and only differ by category and fixture data. Extract a small parameterized helper in local-agent-adapters.test.ts that can run the shared flat-select, grouped-select, and invalid-value assertions for both resolvers, while keeping the existing expected sessionId/configId/value and error-message checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/local-agent-adapters.test.ts`:
- Around line 32-157: The test blocks for resolveAcpModelConfigUpdate and
resolveAcpThinkingConfigUpdate are duplicated and only differ by category and
fixture data. Extract a small parameterized helper in
local-agent-adapters.test.ts that can run the shared flat-select,
grouped-select, and invalid-value assertions for both resolvers, while keeping
the existing expected sessionId/configId/value and error-message checks intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2068856b-6c1a-42b1-87d5-edbd0db3f6d7
📒 Files selected for processing (2)
src/local-agent-adapters.test.tssrc/local-agent-adapters.ts
|
Addressed CodeRabbit review feedback in
Intentionally skipped hardcoded Claude/Codex thinking enum validation. That would make DevSpace own provider naming and drift from the local harnesses. ACP remains dynamically validated because ACP advertises its available options at runtime. |
1 participant
Summary
thinkingfrontmatter and CLI override for subagent profilesValidation
Summary by CodeRabbit
open_workspacecatalogs, anddevspace agents runvia--thinking(including override behavior).devspace agents showoutput.