chore: backport upstream tier-1 fixes (#3640, #3407, #3699)#181
Merged
Conversation
WalkthroughRebrands telemetry/events and package identity to proto-cli, enforces opt-in OTLP/HTTP telemetry (otel.proto-labs.ai) with bearer-token support, removes legacy RUM/QwenLogger, adds streaming reasoning capture and thinking-token telemetry, implements async task-plan-aware message compaction, adjusts CLI UI gradient/interaction behavior, and bumps package versions to 0.31.2. ChangesTelemetry & Branding Modernization
Thinking Capture & Model-override
Task-Plan-Aware Message Compaction
CLI UI, Gradient & Dialog Improvements
Tool Validation & Types
Versioning, Packaging & Misc.
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pipeline as OpenAI Pipeline
participant Tracer as OpenTelemetry Tracer
participant OTLP as OTLP HTTP Exporter
rect rgba(100, 150, 200, 0.5)
Note over Client,OTLP: Streaming with reasoning capture
Client->>Pipeline: executeStream(request)
activate Pipeline
loop per stream chunk
Pipeline->>Pipeline: extract delta.reasoning_content or delta.reasoning
Pipeline->>Pipeline: append to reasoningParts
Pipeline->>Tracer: record chunk/completion progress
end
Pipeline->>Pipeline: concat reasoningParts (truncate to 10K)
Pipeline->>Tracer: set gen_ai.response.thinking and usage tokens
Pipeline->>Tracer: end span
deactivate Pipeline
Tracer->>OTLP: export span/log with thinking attribute
end
sequenceDiagram
participant Config as Config/Telemetry
participant SDK as OpenTelemetry SDK
participant HTTP as OTLP HTTP Exporter
participant Ingress as otel.proto-labs.ai
rect rgba(150, 100, 200, 0.5)
Note over Config,Ingress: Opt-in telemetry initialization
Config->>Config: check telemetry.enabled
alt enabled
Config->>Config: read OTEL_INGRESS_TOKEN
alt token present
Config->>HTTP: create exporter with Authorization: Bearer <token>
else token absent
Config->>Config: debug log (no token)
Config->>HTTP: create exporter without auth header
end
Config->>SDK: initialize SDK with exporters
SDK->>Ingress: send spans/logs (HTTP POST)
else disabled
Config->>Config: return early (debug if Langfuse vars present)
end
end
sequenceDiagram
participant User as User Input
participant Dialog as AskUserQuestionDialog
participant App as App Callback
rect rgba(100, 200, 150, 0.5)
Note over Dialog,App: Number-key selection behavior
User->>Dialog: press number key
alt single-select single-question
Dialog->>Dialog: select option and call selectAndAdvance(option.label)
Dialog->>App: onConfirm(ProceedOnce, details)
else multi-select or custom-option or multi-question
Dialog->>Dialog: update highlight/selectedIndex only
Note over Dialog: no auto-submit
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/core/src/core/openaiContentGenerator/pipeline.ts (1)
632-641: 💤 Low valueClarify the discrepancy between comment and test expectation.
The comment at Line 36-41 of
recapGenerator.tsstates thatthinkingConfig.includeThoughts: false"translates intoextra_body.enable_thinking: falseon the wire", but this code explicitly returns{}and does not injectextra_body.enable_thinking. The test atpipeline.test.ts:1344-1369correctly assertsextra_bodyis undefined.The recapGenerator comment should be updated to reflect that thinking is suppressed via model routing (protolabs/fast alias), not via a wire-level flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/core/openaiContentGenerator/pipeline.ts` around lines 632 - 641, The comment in recapGenerator (referenced near pipeline.ts return {} and tests in pipeline.test.ts) is outdated: update the recapGenerator comment to state that thinking is suppressed by routing to a non-thinking model alias (e.g., protolabs/fast) rather than by emitting extra_body.enable_thinking on the wire; make the text consistent with the actual behavior in pipeline.ts (which returns {} for reasoning/thinking) and with the tests that assert extra_body is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 30-33: The release workflow's PR branch check allows external
forks to trigger releases; update the conditional in the release.yml job to also
require the PR head repo be the main repository by adding
github.event.pull_request.head.repo.full_name == github.repository combined with
the existing branch checks (i.e., require repo match AND
(github.event.pull_request.head.ref == 'dev' ||
startsWith(github.event.pull_request.head.ref, 'auto-release/'))), so only
in-repo PR heads named dev or auto-release/* can trigger the release.
In `@docs/superpowers/plans/2026-04-30-p4-compaction-todo-preservation.md`:
- Around line 96-97: Update the example assertions that still expect the old
in-progress marker '→' to instead use the implemented marker '[~]'; specifically
change the expect(...) calls (e.g., expect(result).toContain('→')) and any other
example snippets referencing '→' to expect('[~]') so the docs/examples match the
actual code/tests that use '[~]'.
In `@packages/core/src/agents/runtime/agent-core.ts`:
- Around line 478-480: Guard the optional task-store retrieval so failures in
this non-mandatory path don't abort compaction: wrap the call to
this.runtimeContext.getTaskStore?.() in a try/catch and set a local taskStore
variable to undefined on error, then pass that taskStore into
compactMessages(masked, targetTokens, { taskStore }); ensure compacted is still
assigned from the (possibly awaited) result as before; reference
runtimeContext.getTaskStore, taskStore, compactMessages, masked, targetTokens,
and compacted when making the change.
In `@packages/core/src/agents/runtime/compaction.ts`:
- Around line 58-61: The task titles are injected raw into the `<task-plan>`
output in renderTask (and the other place where task.title is used), which can
break the structured block; create a small sanitizer (e.g., sanitizeTaskTitle or
escapeForTaskPlan) that replaces/encodes characters like <, >, &, and newlines
(either with HTML entities or safe replacements such as <, >, &, and a
single space for newlines) and use that sanitized value wherever task.title is
interpolated (replace `${task.title}` with the sanitized function call in
renderTask and the other occurrence).
In `@packages/core/src/mcp/token-storage/index.ts`:
- Around line 13-14: The env var rename causes silent breakage because
FORCE_ENCRYPTED_FILE_ENV_VAR now points to
'PROTO_CLI_FORCE_ENCRYPTED_FILE_STORAGE' but oauth-token-storage.ts still reads
process.env[FORCE_ENCRYPTED_FILE_ENV_VAR]; update FORCE_ENCRYPTED_FILE_ENV_VAR
handling to accept the old name as well (e.g., check
process.env['QWEN_CODE_FORCE_ENCRYPTED_FILE_STORAGE'] and
process.env['PROTO_CLI_FORCE_ENCRYPTED_FILE_STORAGE']), prefer the new name when
both present, and emit a deprecation warning (via logger or console.warn) when
the old variable is used so users see the change and can update; ensure the
constant or helper that resolves the env value is referenced by
oauth-token-storage.ts and other consumers.
In `@packages/core/src/recap/recapGenerator.test.ts`:
- Around line 91-103: Update the test description to accurately reflect
behavior: remove or correct the parenthetical that claims "lets pipeline.ts
inject extra_body.enable_thinking" since pipeline.ts does not inject that field;
keep the rest of the test unchanged (the test named "always sets
thinkingConfig.includeThoughts=false (lets pipeline.ts inject
extra_body.enable_thinking)" in recapGenerator.test.ts should be renamed to
something like "always sets thinkingConfig.includeThoughts=false" or similar),
ensuring references to generateRecap and the assertion on
generateContent.mock.calls[0][0].config.thinkingConfig?.includeThoughts remain
intact.
In `@packages/core/src/recap/recapGenerator.ts`:
- Around line 36-52: The docstring on pickRecapModel is incorrect about how
"thinking" is suppressed; update the comment to state that suppression is
achieved by routing to the fast model (PREFERRED_RECAP_MODEL_ID /
protolabs/fast) rather than via thinkingConfig.includeThoughts or pipeline.ts
injecting extra_body.enable_thinking, and mention that pipeline.ts currently
returns {} instead of adding that field; keep references to pickRecapModel,
PREFERRED_RECAP_MODEL_ID and pipeline.ts so reviewers can locate the related
code and clarify the actual behavior.
---
Nitpick comments:
In `@packages/core/src/core/openaiContentGenerator/pipeline.ts`:
- Around line 632-641: The comment in recapGenerator (referenced near
pipeline.ts return {} and tests in pipeline.test.ts) is outdated: update the
recapGenerator comment to state that thinking is suppressed by routing to a
non-thinking model alias (e.g., protolabs/fast) rather than by emitting
extra_body.enable_thinking on the wire; make the text consistent with the actual
behavior in pipeline.ts (which returns {} for reasoning/thinking) and with the
tests that assert extra_body is undefined.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23545239-0660-4552-bcc3-65ccc946a27a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/release.ymlREADME.mddocs/architecture/divergence-from-upstream.mddocs/contributing/telemetry.mddocs/superpowers/plans/2026-04-30-p4-compaction-todo-preservation.mdpackage.jsonpackages/cli/package.jsonpackages/cli/src/acp-integration/acpAgent.tspackages/cli/src/config/config.test.tspackages/cli/src/ui/components/Header.test.tsxpackages/cli/src/ui/components/Header.tsxpackages/cli/src/ui/components/StatsDisplay.test.tsxpackages/cli/src/ui/components/StatsDisplay.tsxpackages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsxpackages/cli/src/ui/components/messages/AskUserQuestionDialog.tsxpackages/cli/src/ui/components/messages/ConversationMessages.test.tsxpackages/cli/src/ui/components/messages/ConversationMessages.tsxpackages/cli/src/ui/utils/gradientUtils.tspackages/core/package.jsonpackages/core/src/agents/runtime/agent-core.tspackages/core/src/agents/runtime/compaction.test.tspackages/core/src/agents/runtime/compaction.tspackages/core/src/config/config.test.tspackages/core/src/config/config.tspackages/core/src/core/client.test.tspackages/core/src/core/openaiContentGenerator/pipeline.test.tspackages/core/src/core/openaiContentGenerator/pipeline.thinking.test.tspackages/core/src/core/openaiContentGenerator/pipeline.tspackages/core/src/extension/marketplace.tspackages/core/src/hooks/hookEventHandler.tspackages/core/src/index.tspackages/core/src/mcp/token-storage/file-token-storage.tspackages/core/src/mcp/token-storage/index.tspackages/core/src/recap/recapGenerator.test.tspackages/core/src/recap/recapGenerator.tspackages/core/src/skills/bundled/proto-helper/SKILL.mdpackages/core/src/telemetry/constants.tspackages/core/src/telemetry/index.tspackages/core/src/telemetry/integration.test.circular.tspackages/core/src/telemetry/loggers.test.tspackages/core/src/telemetry/loggers.tspackages/core/src/telemetry/qwen-logger/event-types.tspackages/core/src/telemetry/qwen-logger/qwen-logger.test.tspackages/core/src/telemetry/qwen-logger/qwen-logger.tspackages/core/src/telemetry/sdk.test.tspackages/core/src/telemetry/sdk.tspackages/core/src/telemetry/types.tspackages/core/src/tools/askUserQuestion.test.tspackages/core/src/tools/askUserQuestion.tspackages/core/src/tools/tools.tspackages/test-utils/package.jsonpackages/web-templates/package.jsonpackages/webui/package.json
💤 Files with no reviewable changes (7)
- packages/core/src/index.ts
- packages/core/src/core/client.test.ts
- packages/core/src/telemetry/qwen-logger/qwen-logger.ts
- packages/core/src/telemetry/qwen-logger/event-types.ts
- packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts
- packages/core/src/hooks/hookEventHandler.ts
- packages/core/src/telemetry/integration.test.circular.ts
Comment on lines
+30
to
+33
| ( | ||
| github.event.pull_request.head.ref == 'dev' || | ||
| startsWith(github.event.pull_request.head.ref, 'auto-release/') | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect release trigger condition and repo guard usage:"
rg -n -C3 "pull_request\.head\.ref|head\.repo\.full_name|startsWith\(github\.event\.pull_request\.head\.ref, 'auto-release/'\)" .github/workflows/release.yml
echo
echo "Inspect auto-release branch naming source:"
rg -n -C3 "BRANCH=\"auto-release/v\\$\\{VERSION\\}\"|gh pr create|--head" .github/workflows/auto-release.ymlRepository: protoLabsAI/protoCLI
Length of output: 1134
🏁 Script executed:
cat -n .github/workflows/release.yml | head -50Repository: protoLabsAI/protoCLI
Length of output: 1727
🏁 Script executed:
# Verify the proposed guard syntax is correct for GitHub Actions expressions
# Check if head.repo is accessible in pull_request events
cat > /tmp/test_syntax.yml << 'EOF'
if: >-
github.repository == 'test/repo' &&
(
github.event_name == 'workflow_dispatch' ||
(
github.event.pull_request.merged == true &&
github.event.pull_request.head.repo.full_name == github.repository &&
(
github.event.pull_request.head.ref == 'dev' ||
startsWith(github.event.pull_request.head.ref, 'auto-release/')
)
)
)
EOF
cat /tmp/test_syntax.ymlRepository: protoLabsAI/protoCLI
Length of output: 446
Restrict release trigger to in-repo PR heads.
The release workflow can be triggered by PRs with branch names matching dev or auto-release/* from external repositories or forks. Add a repo ownership check to ensure only PRs from the main repository can trigger releases.
Add github.event.pull_request.head.repo.full_name == github.repository to the condition at lines 30-33:
Proposed guard update
(
github.event.pull_request.merged == true &&
+ github.event.pull_request.head.repo.full_name == github.repository &&
(
github.event.pull_request.head.ref == 'dev' ||
startsWith(github.event.pull_request.head.ref, 'auto-release/')
)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 30 - 33, The release workflow's
PR branch check allows external forks to trigger releases; update the
conditional in the release.yml job to also require the PR head repo be the main
repository by adding github.event.pull_request.head.repo.full_name ==
github.repository combined with the existing branch checks (i.e., require repo
match AND (github.event.pull_request.head.ref == 'dev' ||
startsWith(github.event.pull_request.head.ref, 'auto-release/'))), so only
in-repo PR heads named dev or auto-release/* can trigger the release.
Comment on lines
+96
to
+97
| expect(result).toContain('→'); // in_progress marker | ||
| }); |
There was a problem hiding this comment.
Update in-progress marker in examples to match implementation ([~]).
Line 96 and Line 169 still use → for in-progress tasks, but code/tests use [~]. Please align the snippets to avoid confusion.
Also applies to: 169-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-30-p4-compaction-todo-preservation.md` around
lines 96 - 97, Update the example assertions that still expect the old
in-progress marker '→' to instead use the implemented marker '[~]'; specifically
change the expect(...) calls (e.g., expect(result).toContain('→')) and any other
example snippets referencing '→' to expect('[~]') so the docs/examples match the
actual code/tests that use '[~]'.
Comment on lines
+478
to
+480
| const taskStore = this.runtimeContext.getTaskStore?.(); | ||
| const result = compactMessages(masked, targetTokens, { taskStore }); | ||
| compacted = result instanceof Promise ? await result : result; |
There was a problem hiding this comment.
Guard task-store acquisition so optional compaction metadata can’t break the loop.
If this.runtimeContext.getTaskStore?.() throws (init/storage error), Line 478 will fail the turn. Since task-plan injection is optional, this path should degrade gracefully to plain compaction.
Suggested fix
- const taskStore = this.runtimeContext.getTaskStore?.();
+ let taskStore;
+ try {
+ taskStore = this.runtimeContext.getTaskStore?.();
+ } catch {
+ taskStore = undefined;
+ }
const result = compactMessages(masked, targetTokens, { taskStore });
compacted = result instanceof Promise ? await result : result;📝 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
| const taskStore = this.runtimeContext.getTaskStore?.(); | |
| const result = compactMessages(masked, targetTokens, { taskStore }); | |
| compacted = result instanceof Promise ? await result : result; | |
| let taskStore; | |
| try { | |
| taskStore = this.runtimeContext.getTaskStore?.(); | |
| } catch { | |
| taskStore = undefined; | |
| } | |
| const result = compactMessages(masked, targetTokens, { taskStore }); | |
| compacted = result instanceof Promise ? await result : result; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/agents/runtime/agent-core.ts` around lines 478 - 480, Guard
the optional task-store retrieval so failures in this non-mandatory path don't
abort compaction: wrap the call to this.runtimeContext.getTaskStore?.() in a
try/catch and set a local taskStore variable to undefined on error, then pass
that taskStore into compactMessages(masked, targetTokens, { taskStore }); ensure
compacted is still assigned from the (possibly awaited) result as before;
reference runtimeContext.getTaskStore, taskStore, compactMessages, masked,
targetTokens, and compacted when making the change.
Comment on lines
+58
to
+61
| function renderTask(task: (typeof tasks)[0], indent: string) { | ||
| const marker = STATUS_MARKERS[task.status] ?? '[ ]'; | ||
| const priority = task.priority ? ` (${task.priority})` : ''; | ||
| lines.push(`${indent}${marker} ${task.title}${priority}`); |
There was a problem hiding this comment.
Escape task titles before embedding them in <task-plan> output.
On Line 61 and Line 80, task.title is injected raw into a structured block. A title containing <, >, or newlines can corrupt the structure and leak arbitrary prompt content into the compaction summary.
Suggested fix
+function escapeTaskPlanText(value: string): string {
+ return value
+ .replaceAll('&', '&')
+ .replaceAll('<', '<')
+ .replaceAll('>', '>')
+ .replace(/\r?\n/g, ' ');
+}
+
function renderTask(task: (typeof tasks)[0], indent: string) {
const marker = STATUS_MARKERS[task.status] ?? '[ ]';
const priority = task.priority ? ` (${task.priority})` : '';
- lines.push(`${indent}${marker} ${task.title}${priority}`);
+ lines.push(
+ `${indent}${marker} ${escapeTaskPlanText(task.title)}${priority}`,
+ );
@@
const marker = STATUS_MARKERS[task.status] ?? '[ ]';
const priority = task.priority ? ` (${task.priority})` : '';
- lines.push(` ${marker} ${task.title}${priority} (orphan)`);
+ lines.push(
+ ` ${marker} ${escapeTaskPlanText(task.title)}${priority} (orphan)`,
+ );
}
}Also applies to: 78-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/agents/runtime/compaction.ts` around lines 58 - 61, The
task titles are injected raw into the `<task-plan>` output in renderTask (and
the other place where task.title is used), which can break the structured block;
create a small sanitizer (e.g., sanitizeTaskTitle or escapeForTaskPlan) that
replaces/encodes characters like <, >, &, and newlines (either with HTML
entities or safe replacements such as <, >, &, and a single space for
newlines) and use that sanitized value wherever task.title is interpolated
(replace `${task.title}` with the sanitized function call in renderTask and the
other occurrence).
Comment on lines
13
to
+14
| export const FORCE_ENCRYPTED_FILE_ENV_VAR = | ||
| 'QWEN_CODE_FORCE_ENCRYPTED_FILE_STORAGE'; | ||
| 'PROTO_CLI_FORCE_ENCRYPTED_FILE_STORAGE'; |
There was a problem hiding this comment.
Environment variable rename is a breaking change.
Renaming FORCE_ENCRYPTED_FILE_ENV_VAR from 'QWEN_CODE_FORCE_ENCRYPTED_FILE_STORAGE' to 'PROTO_CLI_FORCE_ENCRYPTED_FILE_STORAGE' means existing users who have set the old environment variable will have it silently ignored after upgrading.
Impact:
- Users who configured
QWEN_CODE_FORCE_ENCRYPTED_FILE_STORAGE=truewill experience a behavior change without warning - The code in
packages/core/src/mcp/oauth-token-storage.ts(line 32) readsprocess.env[FORCE_ENCRYPTED_FILE_ENV_VAR], so the old variable will no longer be recognized
Recommendation:
Document this breaking change in release notes and upgrade guides, instructing users to rename the environment variable if they have it configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/mcp/token-storage/index.ts` around lines 13 - 14, The env
var rename causes silent breakage because FORCE_ENCRYPTED_FILE_ENV_VAR now
points to 'PROTO_CLI_FORCE_ENCRYPTED_FILE_STORAGE' but oauth-token-storage.ts
still reads process.env[FORCE_ENCRYPTED_FILE_ENV_VAR]; update
FORCE_ENCRYPTED_FILE_ENV_VAR handling to accept the old name as well (e.g.,
check process.env['QWEN_CODE_FORCE_ENCRYPTED_FILE_STORAGE'] and
process.env['PROTO_CLI_FORCE_ENCRYPTED_FILE_STORAGE']), prefer the new name when
both present, and emit a deprecation warning (via logger or console.warn) when
the old variable is used so users see the change and can update; ensure the
constant or helper that resolves the env value is referenced by
oauth-token-storage.ts and other consumers.
Comment on lines
+91
to
+103
| it('always sets thinkingConfig.includeThoughts=false (lets pipeline.ts inject extra_body.enable_thinking)', async () => { | ||
| const ac = new AbortController(); | ||
| await generateRecap( | ||
| mockConfig, | ||
| [{ role: 'user', parts: [{ text: 'hi' }] }], | ||
| ac.signal, | ||
| ); | ||
|
|
||
| const callArgs = generateContent.mock.calls[0][0] as { | ||
| config: { thinkingConfig?: { includeThoughts?: boolean } }; | ||
| }; | ||
| expect(callArgs.config.thinkingConfig?.includeThoughts).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Test description is misleading.
The description says "lets pipeline.ts inject extra_body.enable_thinking", but pipeline.ts explicitly does not inject that field (returns {} at lines 631-641). The test itself is correct — it verifies includeThoughts: false is set — but the parenthetical should be removed or corrected.
📝 Suggested fix
- it('always sets thinkingConfig.includeThoughts=false (lets pipeline.ts inject extra_body.enable_thinking)', async () => {
+ it('always sets thinkingConfig.includeThoughts=false', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/recap/recapGenerator.test.ts` around lines 91 - 103, Update
the test description to accurately reflect behavior: remove or correct the
parenthetical that claims "lets pipeline.ts inject extra_body.enable_thinking"
since pipeline.ts does not inject that field; keep the rest of the test
unchanged (the test named "always sets thinkingConfig.includeThoughts=false
(lets pipeline.ts inject extra_body.enable_thinking)" in recapGenerator.test.ts
should be renamed to something like "always sets
thinkingConfig.includeThoughts=false" or similar), ensuring references to
generateRecap and the assertion on
generateContent.mock.calls[0][0].config.thinkingConfig?.includeThoughts remain
intact.
Comment on lines
+36
to
+52
| /** | ||
| * Decide which model to use for the recap. Prefers `protolabs/fast` if the | ||
| * user has it configured (it's the gateway alias for a fast non-thinking | ||
| * model). Falls back to the current session model — thinking is then | ||
| * suppressed via `thinkingConfig.includeThoughts: false` which pipeline.ts | ||
| * translates into `extra_body.enable_thinking: false` on the wire. | ||
| */ | ||
| function pickRecapModel(config: Config): { | ||
| model: string; | ||
| isOverride: boolean; | ||
| } { | ||
| const available = config.getModelsConfig().getAllConfiguredModels(); | ||
| if (available.some((m) => m.id === PREFERRED_RECAP_MODEL_ID)) { | ||
| return { model: PREFERRED_RECAP_MODEL_ID, isOverride: true }; | ||
| } | ||
| return { model: config.getModel(), isOverride: false }; | ||
| } |
There was a problem hiding this comment.
Update comment to match actual behavior.
The docstring states thinking is "suppressed via thinkingConfig.includeThoughts: false which pipeline.ts translates into extra_body.enable_thinking: false". However, pipeline.ts lines 631-641 explicitly return {} and do not inject that field. The actual suppression relies on model routing to protolabs/fast.
📝 Suggested comment fix
/**
* Decide which model to use for the recap. Prefers `protolabs/fast` if the
* user has it configured (it's the gateway alias for a fast non-thinking
- * model). Falls back to the current session model — thinking is then
- * suppressed via `thinkingConfig.includeThoughts: false` which pipeline.ts
- * translates into `extra_body.enable_thinking: false` on the wire.
+ * model). Falls back to the current session model with
+ * `thinkingConfig.includeThoughts: false` — note that the pipeline does NOT
+ * inject extra_body flags; suppression depends on model capabilities.
*/📝 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
| /** | |
| * Decide which model to use for the recap. Prefers `protolabs/fast` if the | |
| * user has it configured (it's the gateway alias for a fast non-thinking | |
| * model). Falls back to the current session model — thinking is then | |
| * suppressed via `thinkingConfig.includeThoughts: false` which pipeline.ts | |
| * translates into `extra_body.enable_thinking: false` on the wire. | |
| */ | |
| function pickRecapModel(config: Config): { | |
| model: string; | |
| isOverride: boolean; | |
| } { | |
| const available = config.getModelsConfig().getAllConfiguredModels(); | |
| if (available.some((m) => m.id === PREFERRED_RECAP_MODEL_ID)) { | |
| return { model: PREFERRED_RECAP_MODEL_ID, isOverride: true }; | |
| } | |
| return { model: config.getModel(), isOverride: false }; | |
| } | |
| /** | |
| * Decide which model to use for the recap. Prefers `protolabs/fast` if the | |
| * user has it configured (it's the gateway alias for a fast non-thinking | |
| * model). Falls back to the current session model with | |
| * `thinkingConfig.includeThoughts: false` — note that the pipeline does NOT | |
| * inject extra_body flags; suppression depends on model capabilities. | |
| */ | |
| function pickRecapModel(config: Config): { | |
| model: string; | |
| isOverride: boolean; | |
| } { | |
| const available = config.getModelsConfig().getAllConfiguredModels(); | |
| if (available.some((m) => m.id === PREFERRED_RECAP_MODEL_ID)) { | |
| return { model: PREFERRED_RECAP_MODEL_ID, isOverride: true }; | |
| } | |
| return { model: config.getModel(), isOverride: false }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/recap/recapGenerator.ts` around lines 36 - 52, The
docstring on pickRecapModel is incorrect about how "thinking" is suppressed;
update the comment to state that suppression is achieved by routing to the fast
model (PREFERRED_RECAP_MODEL_ID / protolabs/fast) rather than via
thinkingConfig.includeThoughts or pipeline.ts injecting
extra_body.enable_thinking, and mention that pipeline.ts currently returns {}
instead of adding that field; keep references to pickRecapModel,
PREFERRED_RECAP_MODEL_ID and pipeline.ts so reviewers can locate the related
code and clarify the actual behavior.
…wenLM#3407) Fixes QwenLM#500. Number keys in AskUserQuestionDialog previously only moved the highlight cursor without submitting, inconsistent with RadioButtonSelect and the standard tool approval dialog. Users pressed a number, saw the option highlight, and assumed it was selected, but the dialog was still waiting for Enter. - For single-select predefined options, pressing a number key now auto-submits immediately. - Multi-select, "Other" custom input, and the Submit tab remain highlight-only (unchanged). - Extracted a shared selectAndAdvance helper to deduplicate the select-and-submit/advance logic across 4 code paths (number key, Enter, multi-select submit, custom input submit). - Removed redundant isFocused guard inside the useKeypress callback; it is already handled via the isActive parameter. Tests cover all four behavioral branches: single-select auto-submits, multi-select does not, "Other" custom input does not, and the Submit tab does not.
Cherry-picked from QwenLM/qwen-code: 784b3ce Make `multiSelect` field optional in the askUserQuestion tool schema. Older / smaller models often omit the field entirely; treating it as required forced a validation failure when the model otherwise produced a well-formed question. Also refactors a pre-existing conditional expect that lint-staged now flags (no behavior change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f265c05 to
1f2915c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx (1)
200-262:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the new focus/highlight behavior here, not just “no submit.”
Both of these new cases only verify that
onConfirmis not called. That still lets regressions slip through if number-key navigation stops moving the selection/highlight for"Other"or multi-select mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6390e0b4-4fcc-44e8-b142-fd45144c8480
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.jsonpackages/cli/package.jsonpackages/cli/src/ui/components/Header.test.tsxpackages/cli/src/ui/components/Header.tsxpackages/cli/src/ui/components/StatsDisplay.test.tsxpackages/cli/src/ui/components/StatsDisplay.tsxpackages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsxpackages/cli/src/ui/components/messages/AskUserQuestionDialog.tsxpackages/cli/src/ui/utils/gradientUtils.tspackages/core/package.jsonpackages/core/src/tools/askUserQuestion.test.tspackages/core/src/tools/askUserQuestion.tspackages/core/src/tools/tools.tspackages/test-utils/package.jsonpackages/web-templates/package.jsonpackages/webui/package.json
✅ Files skipped from review due to trivial changes (8)
- packages/web-templates/package.json
- packages/cli/src/ui/utils/gradientUtils.ts
- packages/cli/package.json
- packages/core/package.json
- packages/test-utils/package.json
- packages/webui/package.json
- packages/cli/src/ui/components/Header.test.tsx
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/tools/askUserQuestion.ts
- packages/cli/src/ui/components/Header.tsx
- packages/core/src/tools/tools.ts
- packages/cli/src/ui/components/StatsDisplay.test.tsx
- packages/cli/src/ui/components/messages/AskUserQuestionDialog.tsx
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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
3 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
Path-of-least-resistance backports from QwenLM/qwen-code (last ~2.5 weeks of upstream activity).
fix(cli): guard gradient rendering without colors— addsgetRenderableGradientColorsutility, guards Header & StatsDisplay against empty gradient color sets (NO_COLOR=1, themes with too few colors). Merged with our X-axis per-line gradient (fix(ui): apply ASCII logo gradient by X column, not string index #72) so logo still renders correctly with no colors.fix(cli): auto-submit on number key press in AskUserQuestionDialog— pressing a number key now selects + auto-submits for single-select questions (previously required Enter). Adds guards: no auto-submit on "Other" custom input, multi-select mode, or Submit tab. NewselectAndAdvance()helper unifies the dialog's submit paths.fix(core): treat ask_user_question multiSelect as optional— relaxes the schema so models that omitmultiSelectaren't rejected. Compatible with our existing fix(ask_user_question): recover from stringified questions array, add example shape #177 stringified-array coercion. Pre-existing conditional-expect lint flag in the test was refactored totoMatchObject(no behavior change).Skipped from this batch
Monitor permission namespace— depends on the monitor tool which lives on a feature branch (not inmain). Will revisit when monitor lands.Test plan
npm run typecheckclean across all workspacesvitest runfor the four touched test files: 52/52 passedask_user_questionsingle-select dialog auto-submits; pressing Other does notNO_COLOR=1— logo renders as plain text, no crash🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Skills