feat(web-shell): add task auth and goal workflows#4856
Conversation
|
Thanks for the PR, @ytahdn! Template looks good ✓ — all required sections present and filled out, including bilingual summary and test plan. On direction: web-shell surfacing of daemon task/auth/goal workflows is clearly aligned with the On approach: +4393/-254 across 48 files is a big PR, but the three features (tasks panel, auth wizard, goal status) are genuinely coupled through the daemon REST endpoints and session types they add. The layering looks right — bridge types → CLI handlers → SDK client → web-shell UI. One thought: the auth provider install flow ( The background task notification queue in Moving on to code review. 🔍 中文说明感谢贡献，@ytahdn！ 模板完整 ✓ — 所有必需章节均已填写，包括双语摘要和测试计划。 方向：web-shell 对 daemon tasks/auth/goal 工作流的支持与 方案：+4393/-254 跨 48 个文件是一个大型 PR，但三个功能（任务面板、认证向导、goal 状态）通过 daemon REST 端点和 session 类型紧密耦合。分层结构正确 — bridge types → CLI handlers → SDK client → web-shell UI。一个想法：auth provider 安装流程（
进入代码审查 🔍 — Qwen Code · qwen3.7-max |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds task auth and goal workflows to the web-shell, including interactive panels for /auth, /tasks, and /goal commands, background task notification delivery, and a StatusBar task pill. The overall architecture is sound and the code follows existing patterns well.
Positive observations:
- Auth flow correctly uses
type="password"for API key input andmaskApiKey()in the review step - Settings mutations are protected by
withSettingsLock - New HTTP endpoints use
mutate()middleware consistent with existing routes - Input validation in
parseAuthProviderInstallRequestis thorough - The task cancel flow has proper double-press confirmation for foreground agents
- Background sub-agent classification correctly excludes from the floating agent panel
- The
withSettingsLockserialization prevents concurrent auth install races
Findings (2 issues, 0 critical):
See inline comments for details.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Notification drain deadlock cycle (Session.ts:650)
When a background task finishes during #executePrompt, the notification callback schedules #processBackgroundTaskNotifications which awaits pendingPromptCompletion. After #executePrompt, prompt() calls #drainBackgroundTaskNotifications which awaits notificationProcessing. But pendingPromptCompletion is only resolved after the drain completes → deadlock cycle.
The 1-second BACKGROUND_TASK_NOTIFICATION_DRAIN_TIMEOUT_MS timeout breaks the cycle eventually but causes a guaranteed 1-second stall per affected prompt.
Fix options:
- Move
resolveCompletion()before the drain call - Remove
await this.pendingPromptCompletioninside#processBackgroundTaskNotifications(serialization is already ensured by the drain) - Add a shorter timeout to
await this.notificationProcessing
(Posted as body-level comment because this line overlaps with an existing R1 comment about a different concern.)
— qwen3.7-max via Qwen Code /review
|
Addressed the body-level notification drain deadlock review as well. The notification callback now only queues completions while a user prompt is active, so it does not start a background processor that waits on pendingPromptCompletion while prompt() is waiting for the drain. The prompt drain remains the single consumer for notifications produced during the prompt. Also verified the related notification path with Session.test.ts and the full Session test file now passes. |
ytahdn
force-pushed
the
feat/tasks_auth_goal_completion
branch
from
e98d9e0 to
d0764af
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Test failure: auth_provider_install was added to SERVE_CAPABILITY_REGISTRY but EXPECTED_STAGE1_FEATURES / EXPECTED_REGISTERED_FEATURES in server.test.ts were not updated, causing 3 test failures ("returns a fresh ordered registered feature list", "advertises current-protocol features", "marks every current feature with its historical v1 origin").
Add 'auth_provider_install' to both expected arrays at the correct position.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] 11 test failures block merge
(1) 3 tests fail in server.test.ts: auth_provider_install was added to SERVE_CAPABILITY_REGISTRY but EXPECTED_STAGE1_FEATURES / EXPECTED_REGISTERED_FEATURES were not updated — the registry now has 61 features but the test expects 57.
(2) 8 runQwenServe integration tests timeout (Error: Hook timed out in 10000ms): the new startup path triggers ACP preheat failed, will retry on first session: AcpSessionBridge initialize timed out after 10000ms on every server start in test mode. The new runQwenServe startup sequence needs a test-friendly fast-path that skips ACP preheat when no bridge is configured.
— qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs. |
|
Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Still unaddressed from previous review rounds — verified still present at 9692796 (kept out of inline comments to avoid duplicating the existing open threads):
taskKind(bridge:bridgeTypes.ts:384,bridge.ts:3410) vskind(REST bodyserver.ts:2016, SDK, data model) — same field renamed mid-pipeline- Goal-status sentinel
'web-shell:goal-status:v1:'duplicated producer/consumer (DaemonSessionProvider.tsx:1097/GoalStatusMessage.tsx:29) with the producer payload typedRecord<string, unknown> - Dead
default: break+ trailingreturn { cancelled: false }after the exhaustive switch (acpAgent.ts:3137-3140) - Unnecessary cast
(connection as { displayName?: string })(App.tsx:769) — the PR itself addeddisplayNametoDaemonConnectionState - Dead
.modelCSS class (StatusBar.module.css:74) - Duplicate independent 3s task polling (
StatusBar.tsx:212+TasksStatusMessage.tsx:219) - Observability set:
sessionTaskCancelzero logging (acpAgent.ts:3084); notification tool-call warning logs only a count, no tool names/task context (Session.ts:1818); provider install rewrites settings.json with no audit log (runQwenServe.ts:948); drain timeout logged at debug level (Session.ts:1626); install error log lacksproviderId(server.ts:1654); goal set dispatched optimistically with no rollback onsendPromptfailure (App.tsx:1296) - Untested contracts: the task-cancel chain (route strict gate, three kinds, paused case) and the
POST /workspace/auth/providererror envelope (501/400 codes, numeric-validation bounds)
Pre-existing on base (informational, not attributable to this PR) — verified by running the suites against the merge base: the capability suite is already red on daemon_mode_b_main (3 stale baseline entries), the runQwenServe describe block has a flaky 10s afterEach close timeout, and acpAgent.test.ts has 2 pre-existing failures (settings.forScope) plus 2 teardown unhandled rejections.
— claude-fable-5 via Claude Code /qreview
| params.prompt, | ||
| ); | ||
| if (isGoalCommand) { | ||
| this.#installGoalTerminalObserver(); |
There was a problem hiding this comment.
[Critical] Goal terminal observer only installed for /goal slash commands
#installGoalTerminalObserver() is called exclusively here, gated by isGoalCommand (line 835: /^\/goal(?:\s|$)/i). Goals set through other paths — daemon API, session restore, or internal mechanisms — will never have the observer installed.
Without the observer, goal terminal events (achieved/failed/aborted) won't reach the web-shell UI, causing the goal status display to remain stale. The daemon continues evaluating the goal, but the user sees no progress updates.
Impact: Users who set goals via the daemon API (or whose goals persist across session restarts) will see a broken goal status UI — the goal pill shows "active" indefinitely even after the goal reaches a terminal state.
Suggested fix: Install the observer whenever the session has an active goal, not just on /goal commands. Consider checking getActiveGoal(this.sessionId) at session initialization and installing the observer if a goal exists. Alternatively, install it in #executePrompt whenever getActiveGoal() returns a non-null value.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
先不修。Impact is low — only affects ACP session restore with an active goal, which is a narrow path. The goal loop itself works correctly; only the UI status card is missing. Deferred to follow-up PR.
There was a problem hiding this comment.
后续补充，session restore 路径的 goal observer 安装需要更完整的方案。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
This round found 3 unique issues not covered by the 31 existing inline comments from prior rounds:
Suggestion
-
Shell task cancel returns
cancelled: truewhile process still running —acpAgent.tssessionTaskCancel shell case:requestCancel(taskId)is asynchronous (signals but doesn't wait for termination), but the handler immediately returns{ cancelled: true, status: 'running' }. Callers see a misleading success response — the task may still be consuming CPU/memory. Return{ cancelled: true, status: 'cancelling' }or document that cancelled does not guarantee immediate termination. -
Goal terminal observer never cleaned up (memory leak) —
Session.ts:#installGoalTerminalObserver: the observer is registered in a globalMap<string, GoalTerminalObserver>butclearGoalTerminalObserver(this.sessionId)is never called. Over the daemon lifecycle, stale observers accumulate and prevent GC of Session instances. If the daemon reuses a sessionId, old observers could fire on the new session's SSE channel. CallclearGoalTerminalObserverincancelPendingPrompt()and any session-dispose path. -
Cron prompt doesn't install notification callback —
Session.ts:#executeCronPrompt:#installBackgroundTaskNotificationCallback()is only called in#executePrompt, not in the cron path. Background task notifications triggered during cron execution are silently lost — users may never learn about tasks that complete while a cron job runs. Install and tear down the callback in#executeCronPromptas well.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Addressed review findings — see individual thread replies for details.
|
Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs. |
- Add /auth command with interactive authentication panel for daemon serve mode, supporting login/logout/refresh flows - Add /tasks command with interactive background tasks panel aligned with CLI's BackgroundTasksDialog (list/detail views, keyboard navigation, cancel/stop with double-press confirm) - Add daemon-side task cancel endpoint and SDK client method - Fix background agent notification delivery in ACP Session so completed agents trigger new model turns via SSE stream - Add task status polling with 2s auto-refresh while panel open - Support dynamic hints based on selected task state - Classify background sub-agent tool calls to exclude from floating agent panel
…ate prompt errors - Add recentActivities, stats, prompt fields to agent task data chain (acp-bridge types → CLI serialization → SDK types → web-shell UI) - Fix broadcastTurnError extracting "[object Object]" from JSON-RPC error objects by reading data.details for the actual error message - Fix duplicate error display in web-shell by marking errors already dispatched by sendPrompt and skipping them in reportError - Remove "Prompt failed" prefix from prompt error messages - Add StatusBar task pill, tasks command enhancements, i18n additions
- Add POST /session/:id/goal/clear API so /goal clear during active generation no longer destroys in-progress work (bypasses cancel+sendPrompt) - Snapshot/restore chat history around notification prompts to prevent polluting shared conversation context - Null pendingPrompt in finally block to prevent stale controller on error - Wrap notification .finally() body in try/catch to prevent unhandled rejection - Add identity guard to dispatchGoalCleared to prevent race with new goal set - Strip trailing dot from hostname in SSRF blocklist check - Suppress per-iteration goal checking events from transcript - Validate goal status kind against known union members - Clean up goal hook on session close to prevent observer leak - Show actionError in task detail view - Cross-reference duplicated GOAL_CLEAR_KEYWORDS constant
ytahdn
force-pushed
the
feat/tasks_auth_goal_completion
branch
from
8452769 to
5e9c305
Compare
Add deadline check inside inner notification drain loop to prevent unbounded processing when new notifications arrive during drain.
ytahdn
force-pushed
the
feat/tasks_auth_goal_completion
branch
from
6467f6a to
2e20612
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] dispose() incomplete — old notification state not cleaned up
dispose() aborts notificationAbortController, clears notificationQueue, and resets boolean notificationProcessing — these are the base branch's notification fields. But the PR adds a parallel set of fields (notificationAbort, notificationDraining, notificationResolve, notificationProcessing: Promise<void> | null at lines ~399-412) that are not cleaned up here.
If a background task notification is mid-processing when dispose() runs (via /clear or session reload):
notificationAbortis never aborted → orphaned model call continues consuming API tokensnotificationResolveis never called → drain loop may hang or fire on disposed statenotificationDrainingis never reset →.finally()at ~line 1904 may reschedule processing becausenotificationQueue.length > 0is checked without athis.disposedguard
Add cleanup for all PR-introduced notification state, and add if (this.disposed) return; guards to #scheduleBackgroundTaskNotificationProcessing(), #processBackgroundTaskNotifications(), and #drainBackgroundTaskNotifications().
[Suggestion] SSRF blocklist omits 100.64.0.0/10 Shared Address Space (RFC 6598)
isBlockedAuthProviderHost at server.ts:368-407 checks RFC 1918 (10/8, 172.16/12, 192.168/16), 127/8, 169.254/16, and 0/8, but not 100.64/10 (CGN range). This range is routable inside AWS VPCs, GCP networks, and Kubernetes clusters. An authenticated user can install an auth provider with baseUrl: "http://100.64.x.x/..." to SSRF cloud-internal services. Fix: add (a === 100 && b !== undefined && b >= 64 && b <= 127) to the IPv4 return expression.
| private readonly createdAt: number = Date.now(); | ||
| private readonly runtimeBaseDir: string; | ||
|
|
||
| // Background task notification state |
There was a problem hiding this comment.
[Critical] Duplicate property declarations with incompatible types — two notification state machines collide
The PR adds new notification fields here (notificationQueue: Array<{ displayText: string; modelText: string }>, notificationProcessing: Promise<void> | null), but the base branch (daemon_mode_b_main) already declares the same names at lines ~428-431 with incompatible types:
| Field | PR (here) | Base branch (line ~428) |
|---|---|---|
notificationQueue |
Array<{ displayText, modelText }> |
BackgroundNotificationQueueItem[] (has taskId, status, kind) |
notificationProcessing |
Promise<void> | null |
boolean |
This produces TS2717/TS2300 compilation errors and means the two notification subsystems — the PR's background-task drain and the base branch's cron-style processor — are silently fighting over the same state. The PR's new code paths that reference this.notificationAbort, this.notificationDraining, this.notificationResolve, and this.notificationCounter operate on a separate set of fields that are not declared in the base, so they either reference undefined or shadow the base fields depending on declaration order.
Additionally, GoalTerminalEvent is imported twice: as a type at line ~28 and again as type GoalTerminalEvent at line ~92. With verbatimModuleSyntax, this may produce TS2300.
| // Background task notification state | |
| // Unify the two notification systems into one set of fields. | |
| // Option A: Remove the PR's duplicate declarations and adapt the new drain | |
| // logic to use the existing BackgroundNotificationQueueItem type and the | |
| // existing notificationAbortController/notificationCompletion fields. | |
| // Option B: Rename the PR's fields (e.g., taskNotificationQueue, | |
| // taskNotificationAbort) to avoid collision, and register separate callbacks. | |
| // Also: remove the duplicate GoalTerminalEvent import (keep only the one at line ~92). |
— qwen3.7-max via Qwen Code /review
| }); | ||
| } | ||
|
|
||
| async #processBackgroundTaskNotifications(): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] Notification processor TOCTOU race can stall user prompts
#processBackgroundTaskNotifications checks this.pendingPrompt before scheduling, but does NOT re-check it after await pendingPromptCompletion. A user prompt that arrives in the same microtask window will find this.notificationAbort === null (not yet assigned by the processor), making the abort in sendPrompt a no-op. The user prompt then blocks on await this.notificationProcessing for the full duration of the notification's model round-trip (potentially 30+ seconds).
In a daemon with active monitors producing rapid output, each monitor event triggers a sequential model call that blocks the next user prompt. This can be sustained as a denial-of-service by writing to a monitored file.
Fix: after await pendingPromptCompletion, add if (this.pendingPrompt) return;. Also have sendPrompt set this.notificationAbort before await this.notificationProcessing.
— qwen3.7-max via Qwen Code /review
| ToolConfirmationOutcome.ProceedOnce, | ||
| ); | ||
| if (approvalMode === ApprovalMode.AUTO) { | ||
| const before = this.config.getAutoModeDenialState(); |
There was a problem hiding this comment.
[Suggestion] Auto mode denial state preservation is a no-op
const before = this.config.getAutoModeDenialState();
const after = before;
// ...
this.config.setAutoModeDenialState(after);getAutoModeDenialState() returns a reference to the internal object, so after === before and setAutoModeDenialState(after) writes back the identical reference. The debugLogger.warn call is the only side effect, producing a misleading warn-level log on every hook approval in AUTO mode.
If the intent is to prevent denial counters from being reset after hook approval, the actual reset-prevention logic is missing. Either implement the intended transformation or remove the block and demote the log to debug level.
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| app.post( | ||
| '/session/:id/goal/clear', |
There was a problem hiding this comment.
[Suggestion] POST /session/:id/goal/clear and sessionGoalClear ext method — zero test coverage
This new route (~24 lines) calls bridge.clearSessionGoal(sessionId) but server.test.ts has no test cases for it, and acpAgent.test.ts has no tests for the sessionGoalClear ext method. The fakeBridge in server.test.ts does not implement clearSessionGoal.
Untested paths: happy path (200 + { cleared: true }), SessionNotFoundError propagation, missing sessionId param guard, cleared: false when no goal hook is registered.
Similarly, the ~276 lines of new notification queue/drain code in Session.ts (#drainBackgroundTaskNotifications, #processBackgroundTaskNotifications, #executeNotificationPrompt, #installBackgroundTaskNotificationCallback) have zero dedicated test coverage.
— qwen3.7-max via Qwen Code /review
| private readonly runtimeBaseDir: string; | ||
|
|
||
| // Background task notification state | ||
| private notificationQueue: Array<{ displayText: string; modelText: string }> = |
There was a problem hiding this comment.
[Critical] Duplicate class member declarations — build-breaking
The PR adds a new notification system (lines 398–408) with fields notificationQueue, notificationProcessing, notificationAbort, and notificationDraining. But a pre-existing notification system already declares notificationQueue (line 423: BackgroundNotificationQueueItem[]), notificationProcessing (line 424: boolean), and notificationAbortController (line 425).
The two systems have incompatible types:
| Field | New (line 398/406) | Pre-existing (line 423/424) |
|---|---|---|
notificationQueue |
Array<{ displayText; modelText }> |
BackgroundNotificationQueueItem[] |
notificationProcessing |
Promise<void> | null |
boolean |
npx tsc --noEmit reports 13 compilation errors (TS2300 duplicate identifier, TS2717 type mismatch, TS2322 boolean-not-assignable-to-Promise, TS2339 property missing). The package cannot build.
This looks like a merge artifact — two branches each implemented a background notification system, and both sets of declarations survived the merge. One system needs to be removed or they must be unified.
— qwen3.7-plus via Qwen Code /review
| * overwrite the active prompt's completion tracker. | ||
| */ | ||
| private notificationProcessing: Promise<void> | null = null; | ||
| private notificationAbort: AbortController | null = null; |
There was a problem hiding this comment.
[Suggestion] isIdle() does not check new notification system fields
isIdle() (line 487) checks !this.notificationProcessing and !this.notificationAbortController — both from the pre-existing notification system. But this new notification system uses different fields:
this.notificationAbort(this line) — NOT checked inisIdle()this.notificationDraining(line 408) — NOT checked inisIdle()
When the new notification system is actively draining or processing, isIdle() returns true incorrectly. Any caller gating on isIdle() — session lifecycle management, rewind/restore guards, session deletion — will believe the session is idle while a notification prompt is mid-stream.
Fix: add !this.notificationAbort && !this.notificationDraining to the isIdle() return expression.
— qwen3.7-plus via Qwen Code /review
| ); | ||
| const advancedConfig = rawAdvanced | ||
| ? { | ||
| ...(typeof rawAdvanced['enableThinking'] === 'boolean' |
There was a problem hiding this comment.
[Suggestion] POST /session/:id/goal/clear endpoint has zero test coverage
Unlike POST /session/:id/tasks/:taskId/cancel (which has 3+ tests covering happy path, invalid params, and bridge error mapping), the new goal/clear endpoint has no tests in server.test.ts. Missing scenarios:
- Happy path:
bridge.clearSessionGoal()returns{ cleared: true }→ 200 response SessionNotFoundErrorfrom bridge → 404 mapping- Missing or invalid
sessionIdroute parameter → 400 response
Similarly, the sessionGoalClear case in acpAgent.ts (the SERVE_CONTROL_EXT_METHODS handler) has no test coverage — compare with sessionTaskCancel which has 5 tests.
— qwen3.7-plus via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
7 Session.test.ts failures (3 auto-compress drain + 4 AUTO-mode review) and a paused-agent cancel bug block merge. The duplicate property declarations in Session.ts suggest a merge-conflict artifact that should be cleaned up.
7 test failures in Session.test.ts:
- 3 auto-compress drain tests fail —
#drainMidTurnUserMessages()was removed from the auto-compress retry path - 4 AUTO-mode review tests fail — L5 approval logic restructuring broke classifier/dispatch expectations
Test coverage gaps: POST /session/:id/goal/clear route and sessionGoalClear ext method have no tests.
— qwen3.7-max via Qwen Code /review
| @@ -389,6 +396,20 @@ export class Session implements SessionContext { | |||
| private readonly createdAt: number = Date.now(); | |||
| private readonly runtimeBaseDir: string; | |||
|
|
|||
There was a problem hiding this comment.
[Critical] Duplicate property declarations — merge-conflict artifact
The class declares notificationQueue and notificationProcessing twice with incompatible types:
| Field | Line 398 (new, PR) | Line 425 (pre-existing) |
|---|---|---|
notificationQueue |
Array<{displayText, modelText}> |
BackgroundNotificationQueueItem[] |
notificationProcessing |
Promise<void> | null |
boolean |
Additionally, GoalTerminalEvent is imported twice — value import at line 28 and type import at line 90.
TypeScript allows duplicate class properties (last declaration wins), but this is clearly a merge-conflict artifact. The first set (lines 398-408) appears to be dead code superseded by the pre-existing notification drain subsystem.
| // Background task notification state — removed: superseded by the | |
| // notification drain subsystem below (lines 425-428). |
Also remove the duplicate GoalTerminalEvent import at line 28 (keep only line 90).
— qwen3.7-max via Qwen Code /review
| ); | ||
| return { cancelled: false, reason, status: task?.status }; | ||
| } | ||
| config.getBackgroundTaskRegistry().cancel(taskId); |
There was a problem hiding this comment.
[Critical] Paused agent task cancel is a no-op — cancel() ignores paused tasks
BackgroundTaskRegistry.cancel() (background-tasks.ts:455) returns immediately for non-running entries: if (!entry || entry.status !== 'running') return;. Yet this guard allows paused tasks through and calls cancel(), which silently no-ops. The handler then returns { cancelled: true, status: 'paused' }, falsely claiming success.
The correct method for paused tasks is abandon() (background-tasks.ts:502), which sets status to cancelled and marks notified = true.
| config.getBackgroundTaskRegistry().cancel(taskId); | |
| if (task.status === 'paused') { | |
| config.getBackgroundTaskRegistry().abandon(taskId); | |
| } else { | |
| config.getBackgroundTaskRegistry().cancel(taskId); | |
| } |
— qwen3.7-max via Qwen Code /review
| await confirmationDetails.onConfirm( | ||
| ToolConfirmationOutcome.ProceedOnce, | ||
| ); | ||
| if (approvalMode === ApprovalMode.AUTO) { |
There was a problem hiding this comment.
[Suggestion] Auto mode denial state "preservation" is a no-op
This block reads the denial state after onConfirm has already been called (line 3260), aliases it with const after = before (same reference), and writes it back unchanged. The log message says "preserved" but no preservation actually occurs.
If the intent is to prevent onConfirm(ProceedOnce) from mutating the denial counters, the snapshot must be taken before onConfirm:
| if (approvalMode === ApprovalMode.AUTO) { | |
| if (approvalMode === ApprovalMode.AUTO) { | |
| const snapshot = { ...this.config.getAutoModeDenialState() }; | |
| await confirmationDetails.onConfirm( | |
| ToolConfirmationOutcome.ProceedOnce, | |
| ); | |
| this.config.setAutoModeDenialState(snapshot); | |
| debugLogger.warn( | |
| `Auto mode denial counters preserved after hook approval: ` + | |
| `${formatDenialStateLog(snapshot)}`, | |
| ); | |
| } |
If this was dead scaffolding from a refactor, remove the entire block.
— qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully: Failed to determine state for PR #4856. See workflow logs. |
| const [a, b] = parts; | ||
| return ( | ||
| a === 0 || | ||
| a === 10 || |
There was a problem hiding this comment.
SSRF bypass: missing 100.64.0.0/10 (CGNAT) in isBlockedAuthProviderHost
The IPv4 block list in isBlockedAuthProviderHost covers the standard RFC 1918 private ranges plus link-local and loopback, but omits 100.64.0.0/10 (Shared Address Space, RFC 6598). This range is widely used by cloud providers for internal services (e.g., AWS/GCP internal endpoints, carrier-grade NAT, VPC-internal routes).
An authenticated client can POST to /workspace/auth/provider with baseUrl: "http://100.64.1.1/secret-api" and the SSRF guard will allow it, since 100.x doesn't match any of the existing block conditions.
Suggested fix — add the CGNAT range to the block list:
return (
a === 0 ||
a === 10 ||
a === 127 ||
(a === 100 && b !== undefined && b >= 64 && b <= 127) || // 100.64.0.0/10
(a === 169 && b === 254) ||
(a === 172 && b !== undefined && b >= 16 && b <= 31) ||
(a === 192 && b === 168)
);Severity: Medium — requires bearer auth to reach the endpoint, and --allow-private-auth-base-url defaults to false (so the block list IS enforced in production). But when the block list is enforced, this is a genuine gap.
| } from '@qwen-code/qwen-code-core'; | ||
| import { NOT_CURRENTLY_GENERATING_CANCEL_MESSAGE } from '@qwen-code/acp-bridge/bridgeErrors'; | ||
| import { | ||
| GoalTerminalEvent, |
There was a problem hiding this comment.
[Critical] All imports from @qwen-code/qwen-code-core collapsed into a single import type block
The base branch had two separate import blocks: import type { ... } for pure types and import { ... } for runtime values (functions, enums, constants). This PR merged them into one import type { ... } block, which converts ~60 runtime symbols — including createDebugLogger, ApprovalMode, ToolConfirmationOutcome, StreamEventType, Storage, DiscoveredMCPTool, firePreToolUseHook, firePostToolUseHook, convertToFunctionResponse, readManyFiles, clampInlineMediaPart, and dozens more — into type-only imports.
TypeScript erases import type at compile time, so every value usage produces TS1361: 'X' cannot be used as a value because it was imported using 'import type'. This causes 454 compilation errors in packages/cli and prevents the package from building.
Additionally, GoalTerminalEvent appears twice: once as a regular member (this line) and once as type GoalTerminalEvent (line 90), producing TS2300: Duplicate identifier.
| GoalTerminalEvent, | |
| import type { | |
| Content, | |
| FunctionCall, | |
| GenerateContentResponseUsageMetadata, | |
| Part, | |
| } from '@google/genai'; | |
| import type { | |
| Config, | |
| GeminiChat, | |
| ToolCallConfirmationDetails, | |
| ToolResult, | |
| ChatRecord, | |
| AgentEventEmitter, | |
| StopHookOutput, | |
| HookExecutionRequest, | |
| HookExecutionResponse, | |
| MessageBus, | |
| StreamEvent, | |
| ChatCompressionInfo, | |
| AutoModeDecision, | |
| AutoModeOutcome, | |
| GoalTerminalEvent, | |
| ConversationFinishedEvent, | |
| PromptSuggestionEvent, | |
| UserPromptEvent, | |
| } from '@qwen-code/qwen-code-core'; | |
| import { | |
| AuthType, | |
| ApprovalMode, | |
| CompressionStatus, | |
| convertToFunctionResponse, | |
| createDebugLogger, | |
| DiscoveredMCPTool, | |
| StreamEventType, | |
| ToolConfirmationOutcome, | |
| generatePromptSuggestion, | |
| logPromptSuggestion, | |
| logToolCall, | |
| logUserPrompt, | |
| getErrorStatus, | |
| readManyFiles, | |
| clampInlineMediaPart, | |
| Storage, | |
| ToolNames, | |
| fireNotificationHook, | |
| firePermissionRequestHook, | |
| firePreToolUseHook, | |
| firePostToolUseHook, | |
| firePostToolUseFailureHook, | |
| injectPermissionRulesIfMissing, | |
| NotificationType, | |
| persistPermissionOutcome, | |
| createHookOutput, | |
| generateToolUseId, | |
| MessageBusType, | |
| getPlanModeSystemReminder, | |
| getArenaSystemReminder, | |
| getStartupContextLength, | |
| isSystemReminderContent, | |
| evaluatePermissionFlow, | |
| needsConfirmation, | |
| isPlanModeBlocked, | |
| abortGoalForStopHookCap, | |
| formatStopHookBlockingCapWarning, | |
| applyAutoModeDecision, | |
| evaluateAutoMode, | |
| getAutoModePermissionDeniedReason, | |
| isApproveOutcome, | |
| MAX_TRANSCRIPT_MESSAGES, | |
| formatDenialStateLog, | |
| recordAllow, | |
| recordFallbackApprove, | |
| shouldFallback, | |
| shouldFirePermissionDeniedForAutoMode, | |
| shouldRunAutoModeForCall, | |
| extractDaemonTraceContext, | |
| withInteractionSpan, | |
| startToolSpan, | |
| endToolSpan, | |
| runInToolSpanContext, | |
| startToolExecutionSpan, | |
| endToolExecutionSpan, | |
| logConversationFinishedEvent, | |
| acquireSleepInhibitor, | |
| setGoalTerminalObserver, | |
| } from '@qwen-code/qwen-code-core'; |
— qwen3.7-plus via Qwen Code /review
5 participants
What this PR does
Adds web-shell support for richer daemon-driven workflows around tasks, auth, and goals. The web shell can now surface task status with structured rendering, expose auth and goal status panels, and consume the new daemon/session APIs needed for those workflows. It also hardens related task and goal interactions, keeps task status rendering shared, and restores the transcript block hook required by pending permission extraction.
Why it's needed
These workflows were already available through the daemon/CLI side but were awkward or incomplete in the web-shell experience. Bringing them into web-shell makes task visibility, authentication state, and goal progress available without falling back to textual command output or separate terminal flows.
Reviewer Test Plan
How to verify
Start the daemon and web-shell against this branch, then verify that
/tasks,/auth, and/goalrender their corresponding interactive/status surfaces. Trigger a background task and confirm task status updates are shown in the footer/status UI and the/taskscommand output uses the same structured rendering. Open a session with pending permissions and confirm the composer hides correctly instead of crashing from a missing transcript blocks hook.Evidence (Before & After)
Before: tasks/auth/goal workflows were not fully represented in web-shell and the task command duplicated text formatting. After: web-shell surfaces these workflows through structured panels/messages and shared task status rendering.
Tested on
Environment (optional)
Verified locally with
npm run buildinpackages/web-shell.Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
为 web-shell 增加围绕 tasks、auth 和 goal 的更完整 daemon 工作流支持。web-shell 现在可以用结构化方式展示任务状态，展示 auth 和 goal 状态面板，并消费这些工作流需要的 daemon/session API。本 PR 也加固了相关 task 和 goal 交互，复用 task 状态渲染逻辑，并恢复 pending permission 提取所需的 transcript blocks hook。
Why it's needed
这些工作流在 daemon/CLI 侧已经具备，但 web-shell 体验里还不完整，用户需要依赖文本命令输出或额外终端流程。把它们补到 web-shell 后，任务可见性、认证状态和 goal 进度都可以直接在 web-shell 中查看和操作。
Reviewer Test Plan
How to verify
基于该分支启动 daemon 和 web-shell，然后验证
/tasks、/auth、/goal能展示对应的交互或状态界面。触发一个后台任务，确认 footer/status UI 会展示任务状态，并且/tasks命令输出复用同一套结构化渲染。打开带 pending permission 的 session，确认 composer 会正确隐藏，而不是因为缺少 transcript blocks hook 崩溃。Evidence (Before & After)
Before：tasks/auth/goal 工作流在 web-shell 中没有完整结构化展示，task 命令也有重复的文本格式化逻辑。After：web-shell 通过结构化 panel/message 展示这些工作流，并复用 task 状态渲染。
Tested on
Environment (optional)
本地在
packages/web-shell下执行npm run build验证通过。Risk & Scope
Linked Issues
N/A