merge(upstream): sync QwenLM/qwen-code upstream changes Apr 18-19 2026#1
Conversation
…#3337) (QwenLM#3393) When MCP OAuth authentication falls back to the "copy this URL into your browser" path (e.g. remote/web terminal where the browser can't auto-open), long URLs wrap across lines inside the bordered dialog and the trailing │ border characters get selected alongside the URL, forcing the user to manually strip them out before pasting. Surface the URL on a dedicated event and let the user press 'c' to push it to the local clipboard via an OSC 52 escape sequence. Works through SSH and modern web terminals (iTerm2, Windows Terminal, xterm.js-based emulators, tmux with set-clipboard, etc.) without a subprocess, and falls back to a visible "copy the URL above manually" hint when the terminal is not a TTY or OSC 52 is blocked. Key points: - OAuth provider emits OAUTH_AUTH_URL_EVENT carrying the full URL. - AuthenticateStep listens, tracks it in state, and binds 'c' while authenticating (modifier/paste keys are filtered out). - copyToClipboardViaOsc52 writes to stderr when it's a TTY, falls back to stdout, and wraps the sequence for tmux/GNU screen via DCS passthrough so multiplexed sessions still work. - Honest feedback: distinct "copy request sent" / "cannot write to terminal" states with a short auto-revert so repeated presses reset the timer. Fixes QwenLM#3337
…2590) Preserve structured agent rawOutput through the VSCode session pipeline. Render dedicated agent execution cards from shared webui components.
…LM#2550) Extract message list into a React.memo component to prevent re-rendering the entire chat history on every keystroke. - Extract MessageList as a memoized component - Wrap UserMessage, AssistantMessage, ThinkingMessage with React.memo - Stabilize onFileClick callback with useCallback - Remove console.log from render path - Wrap handleToggleThinking with useCallback Fixes QwenLM#2395 Made-with: Cursor
…tartup (QwenLM#3319) * feat(cli): add early input capture to prevent keystroke loss during startup (QwenLM#3224) Start raw mode stdin listening immediately after setRawMode(true), buffer user input during REPL initialization (200-500ms), then replay it once KeypressProvider is mounted. Prevents keystrokes typed before the REPL is ready from being silently dropped. - Filter out terminal response sequences (DA, DA2, OSC, DCS, APC) while preserving real user input (arrow keys, function keys, etc.) - 64KB buffer limit for safety - Replay via setImmediate() to ensure subscribers are registered first - Disable via QWEN_CODE_DISABLE_EARLY_CAPTURE=1 - Add benchmark-startup.sh / benchmark-startup-simple.sh for baseline startup time measurement Co-authored-by: Qwen-Coder <[email protected]> * fix(cli): fix bugs and optimize early input capture - Fix getAndClearCapturedInput resetting captured flag, preventing potential re-arm - Fix passthrough mode replay bypassing paste marker handling in KeypressContext - Optimize buffer storage from O(n^2) concat to chunked collection - Optimize filterTerminalResponses to use pre-allocated Buffer instead of number[] - Add atomic stopAndGetCapturedInput API to prevent two-step usage errors - Remove unrelated benchmark shell scripts - Add test for stopAndGetCapturedInput Co-authored-by: Qwen-Coder <[email protected]> * fix(cli): fix listener leak, silent failures, and error handling in early input capture - Register cleanup for stdin listener in gemini.tsx to prevent orphaned listener on any error path before UI mounts - Add try-catch and cancellation guard to setImmediate replay in KeypressContext to handle component unmount and replay errors gracefully - Stop capture immediately and warn when buffer limit is reached instead of silently dropping data with a debug-level log - Capture stdin reference at registration time so removeListener always operates on the correct stream instance - Add debug log when early capture is skipped due to non-TTY stdin Co-authored-by: Qwen-Coder <[email protected]> * fix(cli): fix early input capture being lost under React StrictMode Move stopAndGetCapturedInput() from inside KeypressProvider's useEffect to before render() in startInteractiveUI. When DEBUG=1, React StrictMode deliberately runs effect→cleanup→effect, causing the first mount to drain the buffer and schedule a replay that the cleanup immediately cancels. The second mount found an empty buffer, silently discarding startup keystrokes. By draining once before render() and passing the bytes as a stable prop, StrictMode remounts always read the same data and can schedule replay on the second (stable) mount. Co-authored-by: Qwen-Coder <[email protected]> * fix: handle split ESC prefixes in early input capture Co-authored-by: Qwen-Coder <[email protected]> * fix: conditionally flush pending startup capture bytes Co-authored-by: Qwen-Coder <[email protected]> * fix: drop incomplete escape sequences instead of replaying as user input When capture stops with an incomplete ESC sequence in pendingTerminalResponse (e.g. lone \x1b or \x1b[), classifyEscapeSequence returns 'incomplete'. Previously shouldReplayPendingAtStop used !== 'terminal' which treated incomplete sequences as user input. Changed to === 'user' so only definitively-user input is replayed; ambiguous sequences are safely dropped. Co-authored-by: Qwen-Coder <[email protected]> --------- Co-authored-by: jinye.djy <[email protected]> Co-authored-by: Qwen-Coder <[email protected]>
…nLM#3237) * fix(build): invoke tsx directly via node --import instead of npx npx resolution breaks when scripts/build.js is invoked under bun (bun's npx wrapper intercepts and runs tsx inside bun's runtime, where tsx's CJS entry fails to resolve). Using 'node --import tsx/esm' skips the npx layer entirely and works under both npm and bun invocation. * fix(build): use node --import tsx/esm for generate:settings-schema script Matches the approach taken in scripts/build.js so running `bun run generate:settings-schema` directly bypasses bun's npx wrapper and avoids the `Cannot find module './cjs/index.cjs'` tsx CJS failure. Co-Authored-By: Claude Opus 4.7 <[email protected]> --------- Co-authored-by: Claude Opus 4.7 <[email protected]>
…3375) * ci(stale): enable 28+28 stale/close policy for pull requests - Fix the repository guard so the workflow actually runs on QwenLM/qwen-code (it was previously gated to google-gemini/gemini-cli and never executed in this repo). - Scope the behavior to pull requests for now; issue policy will be introduced separately once triage labels are in place. - Mark a PR stale after 4 weeks without activity, then close it after another 4 weeks. - Exempt pinned, security, status/blocked, status/on-hold, and status/ready-for-merge from auto-close. - Remove the stale label automatically when activity resumes, and process the oldest PRs first on each run. * ci(stale): loosen PR cadence from 28+28 to 35+35 Five weeks + five weeks gives contributors more slack around holidays and busy periods, and reduces the first-run impact on the existing backlog. The total window moves from 56 days to 70 days. * ci(stale): move cron from 01:30 UTC to 00:30 UTC Shift by one hour so results are ready before the Beijing work day starts (08:30 local), while still avoiding the top of the hour (the high-contention window for GitHub-hosted runners) and staying 30 minutes after release.yml at 00:00 UTC. * ci(stale): drop redundant repo guard and document ops-per-run - Remove the `github.repository == 'QwenLM/qwen-code'` job guard: scheduled runs are already disabled on forks by GitHub, and workflow_dispatch is manually-triggered so the guard adds no safety. - Add a comment explaining the `operations-per-run: 100` rationale (rate-limit headroom given the ~150-PR backlog).
Merged 6 upstream commits while preserving HopCode architecture: Features synced from upstream: - feat(mcp): OSC 52 copy hotkey for OAuth authorization URL (QwenLM#3393) Press 'c' during OAuth to copy URL via terminal clipboard, works over SSH - feat(cli): early input capture to prevent keystroke loss during startup (QwenLM#3319) Buffers keystrokes during REPL init, replays once UI is mounted - perf(vscode): fix input lag in long conversations via React.memo (QwenLM#2550) MessageList/UserMessage/AssistantMessage wrapped with React.memo - feat(vscode-ide-companion): agent execution tool display (QwenLM#2590) Render dedicated agent execution cards in webview - fix(build): invoke tsx via node --import instead of npx (QwenLM#3237) Fixes bun compatibility for generate:settings-schema script - ci(stale): enable 35+35 stale/close PR policy (QwenLM#3375) Conflict resolution: - packages/vscode-ide-companion/.../toolcalls/index.tsx: kept @hoptrendy/webui, added ToolCallData to imports (upstream added it) - All @qwen-code/* import paths preserved as @hoptrendy/* (HopCode arch) - HopCode branding, version, and CI workflows preserved Co-authored-by: Copilot <[email protected]>
|
Analysis CompleteGenerated ECC bundle from 7 commits | Confidence: 65% View Pull Request #2Repository Profile
Changed Files (30)
Top hotspots
Top directories
Likely Future Issues (2)
Suggested Follow-up Work (2)
Copy-ready bodies test: add browser coverage for packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx + packages/cli/src/ui/contexts/KeypressContext.tsx ## Summary
- Add browser or end-to-end coverage for the recently changed user-facing surface.
## Why
- Backfill browser coverage before another user-facing UI change lands on the touched surface.
## Touched paths
- `packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx`
- `packages/cli/src/ui/contexts/KeypressContext.tsx`
## Validation
- Add or extend browser / e2e coverage for the changed component, page, or flow.
- Exercise the visible user journey that depends on the touched UI surface.chore: refresh lockfile and validate CI after dependency updates ## Summary
- Refresh the lockfile and rerun CI after the dependency or workflow changes in this PR.
## Why
- Package or workflow changes without a lockfile refresh tend to turn into noisy follow-up fixes after merge.
## Touched paths
- `.github/workflows/stale.yml`
- `package.json`
## Validation
- Refresh the lockfile in the same package manager used by the repo.
- Run the repo typecheck / test / CI entrypoints that depend on the updated package graph.Detected Workflows (3)
Generated Instincts (27)
After merging, import with: Files
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b9d7806b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * user input being lost during startup. | ||
| */ | ||
|
|
||
| import { createDebugLogger } from '@qwen-code/qwen-code-core'; |
There was a problem hiding this comment.
Import early capture logger from the shipped core package
earlyInputCapture.ts imports createDebugLogger from @qwen-code/qwen-code-core, but this repo’s CLI package depends on @hoptrendy/hopcode-core (see packages/cli/package.json), so a normal install cannot resolve the new specifier. Because gemini.tsx imports this module at startup, interactive CLI startup can fail with module-resolution errors before UI initialization.
Useful? React with 👍 / 👎.
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { ToolCallRouter } from './index.js'; | ||
|
|
||
| vi.mock('@qwen-code/webui', async () => { |
There was a problem hiding this comment.
Mock the same webui module that ToolCallRouter imports
This test mocks @qwen-code/webui, but ToolCallRouter now imports from @hoptrendy/webui (index.tsx), so the mock is never applied. The assertions in this file depend on mocked label output (for example agent:...), which makes the new test fail or become brittle because it exercises real components instead of the intended stubbed ones.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Syncs upstream changes from QwenLM/qwen-code (Apr 18–19, 2026) into this repo while keeping HopCode package/import architecture, adding: structured agent tool-call rendering, CLI startup input capture, OSC52 OAuth URL copy support, and a stale-PR workflow policy.
Changes:
- Add structured
rawOutputplumbing + dedicatedAgentToolCallrenderer for subagent execution progress/cards across webui + VS Code companion. - Add CLI “early input capture” to prevent keystroke loss during startup, with replay into the keypress layer.
- Add OAuth authorization URL copy hotkey (OSC 52) and update stale workflow + build script.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/webui/src/index.ts | Re-exports new agent tool-call component/utilities and types. |
| packages/webui/src/components/toolcalls/shared/types.ts | Adds agent execution types and rawOutput to ToolCallData. |
| packages/webui/src/components/toolcalls/shared/index.ts | Re-exports new agent execution types. |
| packages/webui/src/components/toolcalls/index.ts | Exports AgentToolCall and type-guards from toolcalls entrypoint. |
| packages/webui/src/components/toolcalls/AgentToolCall.tsx | New dedicated UI card for structured agent execution output. |
| packages/webui/src/components/messages/UserMessage.tsx | Memoizes user message rendering to reduce rerenders. |
| packages/webui/src/components/messages/ThinkingMessage.tsx | Memoizes thinking message rendering to reduce rerenders. |
| packages/webui/src/components/messages/Assistant/AssistantMessage.tsx | Memoizes assistant message rendering to reduce rerenders. |
| packages/webui/src/components/ChatViewer/ChatViewer.tsx | Routes tool calls to AgentToolCall when rawOutput indicates agent execution. |
| packages/vscode-ide-companion/src/webview/hooks/useToolCalls.ts | Preserves/merges rawOutput across tool-call updates. |
| packages/vscode-ide-companion/src/webview/hooks/useToolCalls.test.tsx | Adds test coverage for rawOutput preservation in the hook. |
| packages/vscode-ide-companion/src/webview/components/messages/toolcalls/index.tsx | Routes to AgentToolCall by inspecting rawOutput (not just kind). |
| packages/vscode-ide-companion/src/webview/components/messages/toolcalls/index.test.tsx | Adds renderer routing tests for agent executions. |
| packages/vscode-ide-companion/src/webview/App.tsx | Memoizes the message list to reduce input lag in long conversations. |
| packages/vscode-ide-companion/src/types/chatTypes.ts | Adds rawOutput to tool-call update types. |
| packages/vscode-ide-companion/src/services/qwenSessionUpdateHandler.ts | Forwards rawOutput from session updates into callbacks. |
| packages/vscode-ide-companion/src/services/qwenSessionUpdateHandler.test.ts | Adds test coverage ensuring rawOutput forwarding. |
| packages/core/src/mcp/oauth-provider.ts | Emits an auth URL event for OAuth flows (for CLI copy hotkey). |
| packages/core/src/index.ts | Re-exports OAuth event constants. |
| packages/cli/src/utils/events.ts | Adds oauth-auth-url event to CLI event enum. |
| packages/cli/src/utils/earlyInputCapture.ts | New early-stdin capture module to buffer keystrokes during startup. |
| packages/cli/src/utils/earlyInputCapture.test.ts | Adds unit tests for early input capture and filtering logic. |
| packages/cli/src/ui/contexts/KeypressContext.tsx | Accepts and replays captured startup input into keypress pipeline. |
| packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx | Adds OSC52 copy-on-c hotkey and subscribes to OAuth URL event. |
| packages/cli/src/i18n/locales/zh.js | Adds translations for the copy-to-clipboard UX strings. |
| packages/cli/src/i18n/locales/en.js | Adds English strings for the copy-to-clipboard UX. |
| packages/cli/src/gemini.tsx | Starts early input capture and drains it before React render (StrictMode-safe). |
| package.json | Uses node --import tsx/esm for bun compatibility in settings schema generation. |
| docs/plans/2026-03-22-agent-tool-display-design.md | Adds an implementation plan doc for the agent tool display work. |
| .github/workflows/stale.yml | Updates stale PR policy and disables stale issue handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| appEvents.removeListener(AppEvent.OauthDisplayMessage, displayListener); | ||
| appEvents.removeListener(AppEvent.OauthAuthUrl, authUrlListener); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
The OAuth event subscription effect uses t(...) but has an empty dependency array. With react-hooks/exhaustive-deps enabled in this repo, this will raise a lint warning and can also capture a stale t implementation if locale/context ever changes. Include t in the dependency list (or refactor so the listeners don't close over changing values) to satisfy the hooks rule.
| }, []); | |
| }, [t]); |
| // Save original stdin | ||
| originalStdin = process.stdin; | ||
| originalIsTTY = process.stdin.isTTY ?? false; | ||
|
|
||
| // Create mock stdin | ||
| mockStdin = new PassThrough(); | ||
| Object.defineProperty(process, 'stdin', { | ||
| value: mockStdin, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| Object.defineProperty(process.stdin, 'isTTY', { | ||
| value: true, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| delete process.env['QWEN_CODE_DISABLE_EARLY_CAPTURE']; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| resetCaptureState(); | ||
|
|
||
| // Restore original stdin | ||
| Object.defineProperty(process, 'stdin', { | ||
| value: originalStdin, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| Object.defineProperty(process.stdin, 'isTTY', { | ||
| value: originalIsTTY, | ||
| writable: true, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
The test restores process.stdin.isTTY by writing a new boolean property descriptor. Other tests in this repo preserve and restore the original descriptor (e.g. packages/cli/src/gemini.test.tsx), which avoids errors if isTTY is non-writable/non-configurable in some Node/CI environments. Consider capturing Object.getOwnPropertyDescriptor(process.stdin, 'isTTY') and restoring that descriptor (or deleting the override when none existed) instead of forcing writable/configurable.
| > **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. | ||
|
|
||
| **Goal:** Add a dedicated VSCode/web UI display for Agent tool executions so subagent progress, summaries, and failures render from structured `rawOutput` instead of falling back to the generic tool card. | ||
|
|
||
| **Architecture:** Preserve ACP `rawOutput` through the VSCode session/update pipeline into `ToolCallData`, then let the shared web UI router detect `task_execution` payloads and render a dedicated `AgentToolCall` component. Keep the change shared in `packages/webui` so VSCode and `ChatViewer` stay aligned. | ||
|
|
||
| **Tech Stack:** TypeScript, React, Vitest, shared `@qwen-code/webui` tool-call components. |
There was a problem hiding this comment.
This plan doc contains tool-specific AI instructions ("For Claude... superpowers:executing-plans") and references @qwen-code/webui, but this repo’s public package import path is @hoptrendy/webui. To keep project documentation repo-agnostic and accurate, remove the AI-specific directive and update the package name/wording to match the HopCode packages used in the implementation.
| > **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. | |
| **Goal:** Add a dedicated VSCode/web UI display for Agent tool executions so subagent progress, summaries, and failures render from structured `rawOutput` instead of falling back to the generic tool card. | |
| **Architecture:** Preserve ACP `rawOutput` through the VSCode session/update pipeline into `ToolCallData`, then let the shared web UI router detect `task_execution` payloads and render a dedicated `AgentToolCall` component. Keep the change shared in `packages/webui` so VSCode and `ChatViewer` stay aligned. | |
| **Tech Stack:** TypeScript, React, Vitest, shared `@qwen-code/webui` tool-call components. | |
| **Goal:** Add a dedicated VSCode/web UI display for Agent tool executions so subagent progress, summaries, and failures render from structured `rawOutput` instead of falling back to the generic tool card. | |
| **Architecture:** Preserve ACP `rawOutput` through the VSCode session/update pipeline into `ToolCallData`, then let the shared web UI router detect `task_execution` payloads and render a dedicated `AgentToolCall` component. Keep the change shared in `packages/webui` so VSCode and `ChatViewer` stay aligned. | |
| **Tech Stack:** TypeScript, React, Vitest, shared `@hoptrendy/webui` tool-call components. |
| * user input being lost during startup. | ||
| */ | ||
|
|
||
| import { createDebugLogger } from '@qwen-code/qwen-code-core'; |
There was a problem hiding this comment.
earlyInputCapture.ts imports createDebugLogger from @qwen-code/qwen-code-core, but this repo consistently uses @hoptrendy/hopcode-core (and there are no other references to @qwen-code/qwen-code-core). This will fail to build/resolve. Switch the import to @hoptrendy/hopcode-core (or the local core entrypoint used elsewhere in the CLI package).
| import { createDebugLogger } from '@qwen-code/qwen-code-core'; | |
| import { createDebugLogger } from '@hoptrendy/hopcode-core'; |
| } | ||
|
|
||
| const minutes = Math.floor(durationMs / 60_000); | ||
| const seconds = Math.round((durationMs % 60_000) / 1000); |
There was a problem hiding this comment.
formatDuration() can produce an invalid "Xm 60s" display because seconds is computed with Math.round(...) (e.g. 119999ms -> 1m 60s). Use Math.floor for seconds (or normalize when rounding hits 60) so the minutes/seconds output stays within 0–59 seconds.
| const seconds = Math.round((durationMs % 60_000) / 1000); | |
| const seconds = Math.floor((durationMs % 60_000) / 1000); |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { ToolCallRouter } from './index.js'; | ||
|
|
||
| vi.mock('@qwen-code/webui', async () => { |
There was a problem hiding this comment.
This test mocks @qwen-code/webui, but the router under test imports tool-call components from @hoptrendy/webui. As written, the mock won't apply and the test will exercise the real webui package instead (or fail if @qwen-code/webui doesn't exist). Update the vi.mock(...) target to @hoptrendy/webui to match the implementation import path.
| vi.mock('@qwen-code/webui', async () => { | |
| vi.mock('@hoptrendy/webui', async () => { |
The global ProxyAgent was overriding per-request dispatchers used by the OpenAI SDK, causing 'API Error: Connection error. (cause: fetch failed)'. - Remove setGlobalDispatcher(new ProxyAgent()) from channel/start.ts - Remove unused undici import - Bump version to 0.17.4 across all packages Fixes #1
When no proxy is set, return undefined instead of creating a default undici Agent. This avoids potential compatibility issues with the OpenAI SDK's default fetch behavior. Fixes #1
* feat(core): wire background shells into the task_stop tool Phase B follow-up #1 from QwenLM#3634, unblocked by QwenLM#3471 (control plane) merging in. The model can now cancel a managed background shell with the same `task_stop` tool it uses for subagents — no more falling back to `kill <pid>` via BashTool. Lookup order: subagent registry first (existing behavior), then the background shell registry as a fallback. Agent IDs follow `<subagentName>-<suffix>` and shell IDs follow `bg_<8 hex chars>`, so the two namespaces cannot collide in practice; the order is fixed for determinism (a defensive test pins agent-wins-over-shell). The shell cancel path resolves through the entry's own AbortController (which `BackgroundShellRegistry.cancel` triggers); the child process exit handler then settles the registry to `cancelled` and the on-disk output file is preserved for inspection via `/tasks` or a direct `Read`. This matches Phase B's "registry's own AbortController is the cancellation source of truth" design without needing the in-flight notification framework that subagents use. Tests: 7 task-stop tests (was 4) — added cancel-shell happy path, NOT_RUNNING for already-exited shell, and a defensive agent-takes-precedence-on-id-collision case. * fix(core): defer shell terminal transition until spawn handler settles @doudouOUC noticed that the previous task_stop path called `BackgroundShellRegistry.cancel(id, Date.now())`, which marked the entry `cancelled` immediately. The spawn handler's settle path only records real exit info via cancel/complete/fail when the entry is still `running`, so the cancel-vs-exit race could permanently hide a real completed/failed result and `/tasks` would show a terminal endTime while the process was still draining. Add a `requestCancel(id)` method to `BackgroundShellRegistry` that triggers the entry's AbortController only; status stays `running` until the settle path observes the abort and records the real terminal state. The immediate-mark `cancel(id, endTime)` is reserved for `abortAll()` / shutdown, where the CLI process is tearing down anyway and there is no settle handler to wait for. Tests updated: - `task-stop.test.ts` cancel-shell happy path now asserts the entry stays `running` with `endTime` undefined post-stop, and the abort signal fires (the settle path's contract, not task_stop's, is the one that flips status). - 3 new `requestCancel` tests in `backgroundShellRegistry.test.ts`: running → abort+still-running, terminal entry no-op, unknown id no-op. --------- Co-authored-by: wenshao <[email protected]>
…rovider (QwenLM#3788) * fix(core): inject thinking blocks for DeepSeek anthropic-compatible provider DeepSeek's anthropic-compatible endpoint (https://api.deepseek.com/anthropic) rejects follow-up requests with HTTP 400 ("The content[].thinking in the thinking mode must be passed back to the API.") whenever a prior assistant turn carrying tool_use omits a thinking block. The model can legitimately return a tool round without thinking text, so qwen-code stored no thought parts and rebuilt the next request with no thinking block, tripping the API's check. Mirroring the existing OpenAI-side fix (QwenLM#3729, QwenLM#3747), the converter now detects DeepSeek by base URL or model name and prepends an empty { type: 'thinking', thinking: '', signature: '' } block to assistant turns missing one. Other anthropic-protocol providers are unaffected. Verified against the live api.deepseek.com/anthropic endpoint: - assistant with tool_use, no thinking → 400 (reproduces QwenLM#3786) - assistant with tool_use, empty thinking injected → 200 OK Refs QwenLM#3786 * fix(core): gate DeepSeek thinking-block injection on thinking mode Address PR review feedback: 1. (Critical) Gate empty-thinking injection on the same per-request condition that emits the top-level `thinking` parameter. The previous implementation injected unconditionally on DeepSeek providers, but `buildThinkingConfig()` may omit `thinking` when reasoning=false or `thinkingConfig.includeThoughts=false` — which is exactly what suggestionGenerator / ArenaManager / forkedAgent do. Shipping thinking blocks without enabling thinking mode is a protocol violation that DeepSeek may reject. Move the option from converter constructor to a per-request `convertGeminiRequestToAnthropic` parameter so the generator can compute the gate correctly. 2. (CodeQL) Replace `baseUrl.includes('api.deepseek.com')` with `new URL(baseUrl).hostname` exact-match. The substring check would accept spoofed hosts like `api.deepseek.com.evil.com`. 3. Document the empty-signature workaround inline. 4. Rename the misleading "redacted_thinking" test case. * fix(core): narrow DeepSeek thinking injection to tool_use turns + subdomain test Address PR review round 2: 1. Narrow injection scope to assistant turns containing tool_use. Live verification against api.deepseek.com/anthropic showed plain-text assistant turns without thinking are accepted unchanged — only tool_use turns trigger the HTTP 400. Injecting on every assistant turn unnecessarily bloats replay history with synthetic blocks the API does not require. Existing thinking blocks on any turn are still preserved untouched. 2. Add test coverage for the subdomain hostname branch (us.api.deepseek.com → matches), addressing the gap noted in review. 3. Update existing negative-case tests (non-deepseek / spoofed / reasoning=false / includeThoughts=false) to use tool_use scenarios so they actually exercise the gating logic instead of trivially passing under the narrowed scope. * docs(core): align DeepSeek thinking-injection comments with narrowed scope Address PR review round 3 (copilot-pull-request-reviewer × 3): comments in three locations still described the constraint as applying to "any prior assistant turn", which was true before commit 8721b41 but no longer matches the implementation. Update the doc comment on isDeepSeekAnthropicProvider and the two test-suite header comments to state the actual narrower contract: the API rejects only tool-use turns that omit thinking blocks; plain-text assistant turns are accepted unchanged. Comment-only change; 58 tests still pass. * fix(core): per-request DeepSeek detection + strip thinking when off Address PR review round 4 (copilot-pull-request-reviewer × 2): 1. Stale provider-detection cache (HCia). The constructor cached isDeepSeekProvider once, but Config.setModel() mutates contentGeneratorConfig.model in place. After a runtime /model switch from a non-DeepSeek model to a DeepSeek one on the same auth config, buildRequest() would keep using the stale flag. Move the detection into buildRequest so each call sees the current model. The detector is cheap (URL parse + string compare). 2. Real thought parts leak through when thinking is disabled (HCib). The previous gate only blocked synthetic injection — but the converter still replayed any existing `thought: true` parts in request.contents as thinking blocks. Code paths that disable thinking against a session whose history was built with thinking on (suggestionGenerator / ArenaManager / forkedAgent) would still emit thinking blocks alongside an absent top-level `thinking` config — the same protocol mismatch the gate was meant to avoid. Add a `stripAssistantThinking` converter option, set in buildRequest to `isDeepSeek && !thinking`. The converter strips thinking and redacted_thinking blocks from assistant messages before message construction completes. Mirror behavior is already proven safe by live verification (DeepSeek currently tolerates either shape, but stripping makes the request body internally consistent and robust to future validation tightening). 3 new tests: - converter strips thinking from assistant turns when option set - generator strips real thought parts when reasoning=false - generator reflects runtime model changes (no stale cache) 61 tests pass; lint + typecheck clean. * fix(core): preserve thinking-only assistant turns instead of emitting empty content Address PR review round 5 (copilot-pull-request-reviewer × 2 — code + test): stripThinkingFromAssistantMessages previously replaced message.content with the filtered array unconditionally. For an assistant turn whose only blocks are thinking/redacted_thinking (e.g. a round cut off by max_tokens before any text or tool_use was emitted), this left `content: []` — which Anthropic API rejects. Dropping the message entirely was considered but would break the required user/assistant alternation. Instead, fall back to leaving the original blocks in place when stripping would empty the message. DeepSeek empirically tolerates the residual `thinking-block + no-thinking-config` shape (verified against api.deepseek.com/anthropic in the V2/X scenarios), so leaving the message untouched is the safer choice than emitting invalid structure. Add regression test for the thinking-only turn shape. 62 tests pass; lint + typecheck clean. * fix(core): validate thinking-block signature, rename option, gate output_config Address PR review round 6 — five substantive items: 1. (Critical) Drop non-compliant thinking blocks lacking a `signature` field and replace them with a synthetic one. A `redacted_thinking` block round-tripped through Gemini Part format becomes `{ text: '', thought: true }` (no thoughtSignature) and converts back to `{ type: 'thinking', thinking: '' }` without `signature` — not spec-compliant. The previous `hasThinking` check accepted these as already-satisfying, leaving non-compliant blocks in the wire message. Tighten the check so they're filtered out and the synthetic injection runs. Live verification: DeepSeek currently tolerates both shapes (lenient), but normalizing is defensively correct against future tightening. 2. Rename converter option `ensureAssistantThinking` → `ensureThinkingOnToolUseTurns`. The new name reflects the actual contract (tool-use turns only, not every assistant turn). 3. Honor `thinkingConfig.includeThoughts: false` in `buildOutputConfig`. Previously a per-request opt-out dropped the top-level `thinking` parameter but still emitted `output_config.effort`, leaking a reasoning-shaped field into side queries that don't want it. 4. Add regression test for mixed text + tool_use assistant turns (common shape: model says something, then calls a tool). 5. Add explicit test for the signature-validation path: an existing compliant thinking block (with signature) is preserved untouched. 64 tests pass; lint + typecheck clean. * fix(core): clean up non-compliant thinking blocks on plain-text turns + assert output_config gating Address PR review round 7 (copilot-pull-request-reviewer × 2): 1. (Hp-x) Round-tripped redacted_thinking blocks were left malformed on assistant turns lacking tool_use. The previous structure only ran the cleanup pass when a tool_use block was present (early return on `!hasToolUse`), so plain-text turns kept the non-compliant `{ type: 'thinking', thinking: '' }` shape. Restructure into two sequential steps: a. Drop non-compliant thinking blocks (no `signature`) on every assistant turn — same fallback that avoids `content: []` if the message is thinking-only. b. Inject the synthetic empty thinking block on tool_use turns that still lack a compliant thinking block after step (a). 2. (Hp-7) The includeThoughts=false test asserted that the top-level `thinking` field is suppressed but didn't cover `output_config`, leaving regressions in the new `buildOutputConfig` gate uncaught. Tighten the assertion to also verify `output_config` is absent. 3. New converter test: cleanup runs on plain-text assistant turns too. 65 tests pass; lint + typecheck clean. * test(core): add explicit redacted_thinking injection-path coverage Address PR review round 8 (#30 — copilot reviewer). The converter treats `redacted_thinking` as already satisfying the thinking-block requirement (no synthetic injected), distinguished from a signature-less `thinking` block which is non-compliant and gets dropped/replaced. Existing tests covered the latter path; this adds explicit coverage of the former. processContent doesn't synthesize redacted_thinking from Gemini parts, so the test reaches into the private helper directly. (#31 — subdomain hostname coverage — already exists at line 602.) 66 tests pass; lint + typecheck clean. * fix(core): per-request anthropic-beta + normalize thinking-only turns Address PR review round 9 (copilot-pull-request-reviewer × 2): 1. (Hydz) thinking-only assistant turns (e.g. max_tokens cutoff or round-tripped redacted_thinking) hit the cleanup-empties fallback and kept the original non-compliant `{ type: 'thinking', thinking: '' }` block. The fallback now replaces the message with a synthetic empty thinking block (`signature: ''` included), which keeps the message non-empty AND spec-compliant. 2. (Hyd4) `anthropic-beta` was set once at construction from the global `reasoning` config, so requests with per-request `thinkingConfig.includeThoughts=false` still advertised interleaved-thinking / effort even though the body had dropped the matching fields. Move beta computation to a new `buildPerRequestHeaders` that derives the header from the actual `thinking` / `output_config` fields present in the request body, and pass it via `messages.create(..., { headers })`. The wire shape is now internally consistent. Test updates: - Drop the three constructor-time beta assertions; they no longer apply. - Add four per-request header tests covering: both betas present, only interleaved-thinking, reasoning=false (no betas), and per-request includeThoughts=false (no betas). 67 tests pass; lint + typecheck clean. * fix(core): preserve thinking text by normalizing in place + merge user beta flags Address PR review round 10 (copilot-pull-request-reviewer × 2): 1. (H0oF) The previous cleanup filtered out every thinking block missing a `signature` field. But that shape is the normal output from OpenAI/Gemini/agent-runtime generators, which only set `thought: true` without a signature. Users switching providers mid-session would silently lose preserved thinking text on the first DeepSeek request. Change Step 1 to NORMALIZE in place: when a thinking block has no signature, set `signature: ''` rather than dropping the block. The original `thinking` text is preserved; DeepSeek empirically accepts empty signatures so the wire shape stays valid. 2. (H0oL) `buildPerRequestHeaders()` overwrote `customHeaders['anthropic-beta']` whenever the per-request override fired, regressing the customHeaders escape hatch for unrelated Anthropic beta features. Merge the user's flags into the computed list (deduped) so users can stack their own betas alongside interleaved-thinking / effort. Test changes: - Renamed and rewrote "drops non-compliant... plain-text" test to assert in-place normalization that preserves thinking text. - Updated "replaces a non-compliant thinking block" comment + name to describe the normalization (the assertion was already correct because the test happened to use empty thinking text). - The empty-content fallback in Step 1 is no longer reachable under the new logic, so the dedicated thinking-only-turn test now exercises only the strip path (where it remains relevant). - Added 3 customHeaders[anthropic-beta] tests: merge with computed, passthrough when no thinking/effort, dedupe. 70 tests pass; lint + typecheck clean. * docs(core): align thinking-injection comments with normalize semantics + add stream test Address PR review round 11 (copilot-pull-request-reviewer × 3): 1. (H3Iw) Update the `ensureThinkingOnToolUseTurns` option docstring to describe in-place normalization (preserving thinking text by filling in `signature: ''`) instead of the old drop-and-replace semantics. 2. (H3I9) Same update on the `applyEmptyThinkingToToolUseTurns` helper JSDoc — clarify that signature-less thinking blocks are normalized in place (preserving original text), not dropped. Mention the common case of cross-provider history where non-Anthropic generators only set `thought: true`. 3. (H3I3) Add a streaming test asserting that `generateContentStream()` also attaches the per-request `anthropic-beta` header. The previous coverage only exercised `generateContent()`, leaving the streaming path's separate code path (line 144 in anthropicContentGenerator.ts) unverified. 71 tests pass; lint + typecheck clean. * refactor(core): split DeepSeek thinking option in two + add header coexistence test Address PR review round 12 (copilot-pull-request-reviewer × 2): 1. (H6ws) The single `ensureThinkingOnToolUseTurns` option was misleadingly narrow: the implementation also rewrote non-tool-use turns by normalizing malformed thinking blocks. Future callers could enable it expecting only the tool-use behavior. Split into two precisely-named options: - normalizeAssistantThinkingSignature: fill missing `signature` on every assistant `thinking` block (cross-provider history compat). - injectThinkingOnToolUseTurns: prepend synthetic empty thinking on tool_use turns missing one (issue QwenLM#3786 trigger). The generator wires both together for DeepSeek when thinking mode is on; either can be used independently if a future caller needs only one pass. 2. (H6w4) Add a test asserting that the per-request `headers` path coexists correctly with `customHeaders`: User-Agent and unrelated customHeaders entries stay in `defaultHeaders` while only the computed `anthropic-beta` rides on the per-request path. Defends against a future regression where header config might be routed through a code path that wipes the constructor defaults. 72 tests pass; lint + typecheck clean. * fix(core): case-insensitive customHeaders[anthropic-beta] merge Address yiliang114 review feedback (QwenLM#3788). HTTP header names are case-insensitive by spec, and the Anthropic SDK lower-cases them during merge. Previously buildPerRequestHeaders only read the lower-case `anthropic-beta` key from customHeaders, so a user-configured `Anthropic-Beta` or `ANTHROPIC-BETA` would be silently overwritten by the per-request computed value. Replace the direct dict lookup with collectCustomBetaFlags() which walks all customHeaders entries and matches the key case-insensitively. Multiple matching entries (unlikely but possible) are concatenated; the existing dedupe pass handles any duplicates. Add a regression test for both `Anthropic-Beta` and `ANTHROPIC-BETA` key shapes. 73 tests pass; lint + typecheck clean. * docs(core): align thinking-injection docs with normalize-in-place semantics + redacted_thinking strip test Address PR review round 14 (copilot-pull-request-reviewer × 4): 1. (IAl4) PR description still described "dropped here so synthetic injection takes over" but the implementation now normalizes signature-less thinking blocks in place (preserving text). PR description rewritten to describe the two-pass model: normalize-in-place + injection-when-truly-missing. 2. (IAl7) `injectThinkingOnToolUseTurns` option docstring claimed signature-less blocks would be "seen as missing" so the synthetic replaces them. Updated to describe the actual flow: the normalization pass runs first, blocks become compliant in place, the injector then sees them as already-satisfying and prepends nothing. Helper JSDoc on `injectEmptyThinkingOnToolUseTurns` fixed the same way. 3. (IAl8) Strip-path coverage missed `redacted_thinking` blocks. Added regression test that verifies both thinking and redacted_thinking blocks are removed when `stripAssistantThinking` is set. 4. (IAl-) Renamed the converter test suite from "thinking-mode injection + normalization (DeepSeek thinking on)" to "DeepSeek thinking-mode normalization, injection, and stripping" so the title accurately covers all behavior the block exercises (including `stripAssistantThinking` cases later in the same describe). 74 tests pass; lint + typecheck clean. * fix(core): exclude anthropic-beta variants from defaultHeaders to avoid wire duplication Address PR review round 15 (copilot-pull-request-reviewer #1). `buildHeaders()` previously spread the entire `customHeaders` map into the SDK's `defaultHeaders`. After moving anthropic-beta computation to the per-request path, a user-configured mixed-case `Anthropic-Beta` key would survive in defaultHeaders verbatim, while the per-request override added a lowercase `anthropic-beta`. The wire then carried two physical headers for the same logical name — SDK behavior on duplicate headers with different casings is undefined. `buildPerRequestHeaders()` already merges those user flags case-insensitively (commit 0d8b5de), so dropping the entry from defaultHeaders is the right boundary: the per-request path owns the header end-to-end. Other customHeaders entries continue to pass through. Add a regression test asserting no `Anthropic-Beta` (any casing) lands in defaultHeaders while unrelated customHeaders are kept. 75 tests pass; lint + typecheck clean.
…wenLM#3809) * feat(core): hint to background long-running foreground bash commands Phase D part (a) of Issue QwenLM#3634. When a foreground `shell` tool call runs ≥ 60 seconds and completes (succeeds or errors), append an advisory line to the LLM-facing tool result suggesting re-running with `is_background: true` next time. Why: today a foreground bash that takes minutes (build watcher, soak test, slow npm install, polling loop) blocks the agent indefinitely. The user is already paying for the wait; the agent's next turn could have started running in parallel under `is_background: true`. Sleep interception (QwenLM#3684) handled the egregious `sleep N` case at validate time; this handles the legitimate-but-long case at result time. Trade-offs: - Threshold = 60s. Half the existing 120s foreground timeout. Long enough that normal `npm install` / `pytest` runs don't trigger; short enough that the hint surfaces before the timeout hard-kills. - Advisory only — the command still runs to completion in the foreground for THIS invocation. The advice is for the agent's NEXT decision, not a corrective action on the current one. - Fires on success AND error completions. The advice is the same ("background it next time") in both cases. - Suppressed on aborted (timeout / user-cancel) — those paths already surface their own messaging and don't benefit from a "should have been background" reminder when the user / system already killed it. Implementation: - New constant `LONG_RUNNING_FOREGROUND_THRESHOLD_MS = 60000` in shell.ts, paired with the existing `DEFAULT_FOREGROUND_TIMEOUT_MS`. - Helper `buildLongRunningForegroundHint(elapsedMs)` exported so future surfaces (UI, telemetry) can render the same text without duplicating the threshold logic. - `Date.now()` bracketing around the spawn → `await resultPromise` block — mirrors what the background path already captures via `entry.startTime`. - Append happens inside the existing non-aborted result builder; zero changes to the cancel / timeout arms. Tests: 4 new cases — fires on long success, omits on short success, fires on long error completion, omits on aborted. Uses vi fake timers to drive wall-clock past the threshold without actually sleeping. * fix(core): tighten long-run hint suppression + boundary tests + post-truncation insertion Addresses 8 review threads on PR QwenLM#3809 — 6 from /review bots, 2 from copilot — covering doc accuracy, code quality, behavioural gaps, and test coverage. **Behavioural fixes (real bugs)**: - **Suppress on external signal kills** (`result.signal != null` with `aborted: false`). `shellExecutionService` only sets `aborted` when the AbortSignal we passed was triggered, so SIGTERM from container shutdown / k8s eviction / OOM killer / sibling process-group reap falls through to the non-aborted branch. The advisory shouldn't fire there — the process didn't run to its conclusion, so "next time, background it" doesn't fit. New test pins this with `signal: 15` (SIGTERM), `aborted: false`. - **Append AFTER `truncateToolOutput`**. Previously the hint was appended inside the non-aborted result builder, which meant for long outputs it got wrapped in the "Truncated part of the output:" envelope — the LLM might read the advisory as part of the command's own output. New post-truncation insertion + test that pins ordering by mocking `truncateToolOutput` directly (real path needs `fs.writeFile` to actually succeed for the replacement branch to fire). - **Hint wording mode-aware**. The dialog mention dropped the unconditional "(footer pill + Enter)" specifics, which would mislead non-TTY users (`-p` headless / ACP / SDK consumers — no dialog or pill exists there). Now qualified as "in interactive mode the Background tasks dialog also has...". `/tasks` and the on-disk output file are mentioned without qualifier (work in any mode). **Code quality**: - **Threshold programmatically coupled to timeout**: `LONG_RUNNING_FOREGROUND_THRESHOLD_MS = Math.floor(DEFAULT_FOREGROUND_TIMEOUT_MS / 2)`. If the timeout is tuned later, the threshold tracks automatically. - **Docstring corrected**: removed the misleading "before it gets killed by the timeout" claim — the hint is on non-aborted path only, so timeout-killed commands never see it. The new docstring enumerates all suppression paths explicitly. - **Removed stale line-number reference**: comment said "mirrors the background path's `entry.startTime` capture (line ~781)" which goes stale on file edits. Now refers conceptually. **Test coverage gaps closed**: - **Off-by-one boundary**: 59_999ms → no hint. Pairs with the existing 60_000ms-exactly test (which fires) to pin the boundary tightly. A regression flipping `>=` to `>` would fail loudly. - **Timeout path explicit**: previous "aborted" test exercised user- cancel only. With `vi.useFakeTimers({ toFake: ['Date'] })`, `AbortSignal.timeout()` doesn't fake (it depends on the real timer subsystem), so `combinedSignal.aborted` stayed false. New test follows the pre-existing `should handle timeout vs user cancellation correctly` pattern: stubs `AbortSignal.timeout` + `.any` to return an already-aborted combined signal, then verifies "Command timed out after Nms" appears AND no advisory. * fix(core): per-invocation long-run threshold + debug-mode + test isolation Six suggestions from /review's third pass on PR QwenLM#3809: **Real semantic fix**: - Long-run threshold now scales with the EFFECTIVE timeout, not the fixed default. A user who sets `timeout: 600_000` (10 min) gets the advisory at 5 min, not at 60s — respects the explicit timeout intent. Replaced the `LONG_RUNNING_FOREGROUND_THRESHOLD_MS` constant with a per-invocation `longRunThresholdFor(effectiveTimeout)` helper. **Debug-mode visibility**: - Debug mode previously snapshotted `returnDisplayMessage = llmContent` BEFORE the truncation + hint append, so debug-mode users saw the pre-hint content while the agent saw the advisory — agent suddenly suggesting `is_background: true` had no visible trigger in the TUI. Re-sync `returnDisplayMessage` after the hint append (debug-mode branch only) so the TUI mirrors what the agent sees. **Type-safety footgun**: - `if (typeof llmContent === 'string')` would silently drop the hint if `llmContent` ever becomes structured `Part[]`. Added an explicit `else` comment documenting the deliberate omission and the conditions under which to revisit (no string llmContent path exists today). **Style**: - Replaced the JSDoc `/** ... */` block on the (now-defunct) constant with a plain `//` comment block on the helper, matching the `DEFAULT_FOREGROUND_TIMEOUT_MS` / `OUTPUT_UPDATE_INTERVAL_MS` style. **Test hygiene**: - Wrapped both `vi.stubGlobal('AbortSignal', ...)` and `vi.spyOn(truncateToolOutput, ...)` in `try/finally` so failures during the test body don't leak the stub/spy into subsequent tests (would cause confusing cascading failures). - Dropped the internal-roadmap "Phase D part (a)" reference from the test comment — future maintainers don't have the context. **New test**: - `threshold scales with the user-supplied timeout (not the default)`: sets `timeout: 600_000`, advances 100s, verifies no hint. Pins the per-invocation coupling so a regression to a fixed constant would fail loudly here. * fix(core): tighten long-run hint suppression + boundary tests + post-truncation insertion (round 4) Six suggestions from /review's pai/glm-5-fp8 pass on PR QwenLM#3809: **Behavioural / UX**: - **Hint now visible in non-debug TUI too.** Previously only debug mode mirrored the hint into `returnDisplay`; non-debug users saw the agent suggest `is_background: true` with no visible trigger. Now the hint is appended to `returnDisplayMessage` in both modes (full mirror in debug, terse-append in non-debug to preserve the output-or-status form). **Test coverage**: - **Debug-mode re-sync test added.** All other long-run hint tests run with `getDebugMode → false`; this one flips it to true and asserts the hint appears in `returnDisplay` too. Pins the re-sync so a regression that drops the debug branch would fail loudly. - **Threshold-scaling positive test added.** The negative case (`timeout: 600_000`, advance 100s, no hint) was already pinned; paired now with the positive case (advance 305s, hint fires) so a regression to a fixed 60s threshold is caught at both ends. **Style / consistency**: - **`result.signal === null` (was `== null`).** Strict equality to match the rest of the file. The `signal` field is typed `number | null` so loose equality has identical semantics, but the inconsistency was noise. **Doc clarity (timing semantics)**: - **Comment explains why elapsedMs is computed BEFORE truncation.** Two reviewers disagreed on the timing — one read it as before truncation (correct, slightly under-reports), the other as after (incorrect read). The intent is to report the COMMAND's runtime, not the tool call's total time. Truncation is post-processing, not part of "agent blocking time", so excluding it is the right semantic. Inline comment now spells this out so future readers don't have to infer. * fix(core): error-path hint surfacing + clock-resilient elapsed + threshold floor + observability Round 5 of PR QwenLM#3809 review — 10 threads, mix of Critical and Suggestion: **Critical fixes**: 1. **Hint survives the error path** (`#OWbA`). When result.error is set, coreToolScheduler builds the model-facing functionResponse from `error.message` ONLY (not llmContent — see convertToFunctionResponse + the toolResult.error branch in scheduler:1648-1724). My hint was being silently dropped on long-command-failed cases. Now the hint is appended to error.message too so the advisory survives whichever branch the scheduler takes. 2. **Hint wording de-ambiguated** (`#OU6o`). "prefer re-running with is_background: true" was ambiguous — model could read it as "re-run THIS command in the background", which on stateful commands (DB migrations, deploys, git push) would cause double side effects. Reworded to "Next time you run a SIMILAR long-running process..." with an explicit parenthetical that warns against re-running the just-completed command. 3. **Debug observability** (`#OU6s`). Added `debugLogger.debug` at the hint decision point with elapsedMs / threshold / aborted / signal — when a user reports "my 65s command didn't get the hint" the suppression branch is now visible in DEBUG output. **Other behaviour fixes**: 4. **Threshold floor of 1000ms** (`#OU6r`). Pathological `timeout: 0` / `timeout: 1` would have given a 0-ms threshold, firing the hint on every invocation showing "ran for 0s". Floor at 1s makes that branch unreachable. 5. **`performance.now()` instead of `Date.now()`** (`#OU6v`). NTP corrections / VM clock drift between capture and read would silently make `elapsedMs` negative and skip the hint with no observable failure. Monotonic clock prevents that. 6. **Debug mode preserves truncation marker** (`#OU6w` / `#OWCq`). Previously `returnDisplayMessage = llmContent` after hint clobbered the "Output too long and was saved to: …" line appended during truncation. Switched to append-style re-sync in BOTH modes so prior content is preserved. **Test coverage gaps closed**: 7. **Non-debug returnDisplay test** (`#OWCo`). Pinned that the user TUI gets the hint in the default (non-debug) mode too. 8. **Test rename** (`#OWCl`). The "debug-mode TUI mirror" test passed in non-debug too after the recent refactor; split into two tests, one per branch. 9. **Error-path hint test**. Added a test that pins `result.error?.message` contains both the original error text AND the hint, covering the scheduler-routing-via-error.message path that was silently broken before fix #1. 10. **Test: faketimers also fakes `performance`**. Since we switched to `performance.now()`, `vi.useFakeTimers({ toFake: ['Date'] })` no longer covered the elapsed measurement; extended to `['Date', 'performance']` so the threshold tests can drive the wall-clock with `advanceTimersByTimeAsync`. #OU6t (else-comment for the type guard) was already addressed in the prior round — the explicit else-with-comment is in place; adding logging there would be noise. * test(core): cover the MIN_LONG_RUN_THRESHOLD_MS floor branch PR QwenLM#3809 review: the new `Math.max(MIN_LONG_RUN_THRESHOLD_MS, ...)` floor in `longRunThresholdFor` was untested — only default-timeout and large-custom-timeout cases existed. A regression that strips the floor would let `timeout: 1` produce a 0ms threshold and fire a "ran for 0s" advisory on every invocation; the test suite would not catch it. New test: build with `timeout: 1`, advance 500ms (below the 1000ms floor), resolve with `aborted: false` to isolate the threshold logic from the abort path. Asserts no hint appears. A regression that removes the floor flips the assertion to fail. * fix(core): structured delimiter on error.message hint + tighten timeout floor comment Two of three threads from the latest /review pass on PR QwenLM#3809 (the third — PR description / threshold scaling reconciliation — is fixed in the PR description update, not in code): - **`\n---\n` divider before hint in `error.message`** (`#Pt7C`). Downstream consumers of `error.message` (firePostToolUseFailureHook, telemetry grouping, SIEM alerting, hook-side error parsers) were receiving ~400 chars of advisory text mixed inline with the original error body — pattern-matching on error messages would absorb the advisory into the matched body. Added a `---` separator line so the boundary is unambiguous and split-able. - **Threshold-floor comment narrowed to `timeout: 1`** (`#Pu9o`). The comment said the floor guards `timeout: 0` / `timeout: 1`, but `validateToolParamValues` rejects `timeout <= 0` at validate time, so `timeout: 0` can't reach `longRunThresholdFor`. Updated the comment to mention only the actually-allowed pathological case (`timeout: 1` and any value `< 2` rounds to 0). Test updated to assert the `---` divider format with `toMatch`. * fix(core): capture executionStartTime AFTER spawn so PTY import isn't counted PR QwenLM#3809 review: copilot caught that `executionStartTime` was captured BEFORE `await ShellExecutionService.execute(...)`, which meant the elapsed measurement included `getPty()` dynamic-import setup (~50-200ms on first call). The hint's "ran for Xs" reading was slightly inflated, and the comment claiming "spawn → settle" wasn't strictly accurate. Moved the capture immediately after the execute() call returns its { result, pid } handle. The pid being set by that point confirms the process has been spawned, so the subtraction is true post-spawn-to- settle. Comment updated to reflect the actual semantics. The displayed accuracy gain is small (50-200ms on a 60s+ threshold is <1%), but the comment claim now matches what the code measures. Tests unaffected — fakeTimers don't drive real dynamic imports, so the threshold tests behave identically. * fix(core): align long-run hint code/tests with ShellExecutionResult.error semantics Four copilot threads on PR QwenLM#3809 — all rooted in the same observation: `ShellExecutionResult.error` is reserved for spawn/setup failures (per the field's doc comment in shellExecutionService.ts), NOT for non-zero exit codes. My existing code/tests conflated the two, making the error-path coverage less realistic and the inline comments inaccurate. **Test shape fixes**: - `appends the hint when a long-running foreground command exits with error` → `exits non-zero`. Changed `error: new Error('exit 1')` to `error: null` (the realistic shape for a non-zero exit without spawn failure). Added a comment explaining the field contract so future test authors don't repeat the conflation. - `hint survives the error path (appended to error.message)`: reframed the mock from `spawn ENOENT` (which would resolve in <1s in practice, making the long-elapsed scenario unrealistic) to `PTY initialization failed after 75s` — a slow-spawn-failure shape that COULD plausibly take 75s. Test still pins the same CODE PATH; comment now acknowledges the edge-case nature ("rare but real: PTY init dragging, remote-fs exec syscalls, security scanners interposing"). **Comment corrections**: - `returnDisplayMessage` build-order comment was misleading. It said "the hint is appended after both the truncation block and the returnDisplayMessage build" — but `returnDisplayMessage` is built BEFORE truncation. Replaced with a chronological enumeration (1. initial value, 2. truncation marker append, 3. hint append) that matches what the code actually does. - Error-path preservation comment now acknowledges the narrow applicability (spawn failures only, exit codes don't reach this branch). Code is unchanged — the path is still real, just rare. * test(core): pin empty-output success + background-no-hint paths Two defensive tests for the long-running foreground hint: - empty-output success at >=60s — exercises the returnDisplayMessage='' → hint append branch (write-only commands like `tar czf` / `cp -r` produce no stdout). Asserts the user- facing returnDisplay still surfaces the advisory even when the command produced nothing else to show. - background never includes the hint — the foreground hint logic lives in executeForeground only, so today this can't fail; the test guards against a future refactor hoisting the advisory into a shared post-execute path that would tag every background launch with a nonsensical "ran for 0s, consider is_background: true" suggestion.
…wenLM#3115) * feat: add commit attribution with per-file AI contribution tracking via git notes Track character-level AI vs human contributions per file and store detailed attribution metadata as git notes (refs/notes/ai-attribution) after each successful git commit. This enables open-source AI disclosure and enterprise compliance audits without polluting commit messages. * feat: enhance commit attribution with real AI/human ratios and generated file exclusion - Replace line-based diff with a prefix/suffix character-level algorithm for precise contribution calculation (e.g. "Esc"→"esc" = 1 char, not whole line) - Compute real AI vs human contribution percentages at commit time by analyzing git diff --stat output: humanChars = max(0, diffSize - trackedAiChars) - Add generated file exclusion (lock files, dist/, .min.js, .d.ts, etc.) ported from an existing generatedFiles.ts - Add file deletion tracking via recordDeletion() - Update git notes payload format: {aiChars, humanChars, percent} per file with real percentages instead of hardcoded 100% * feat: add surface tracking, prompt counting, session persistence, and PR attribution Align with the full attribution feature set: - Surface tracking: read QWEN_CODE_ENTRYPOINT env var (cli/ide/api/sdk), include surfaceBreakdown in git notes payload - Prompt counting: incrementPromptCount() hooked into client.ts message loop, tracks promptCount/permissionPromptCount/escapeCount - Session persistence: toSnapshot()/restoreFromSnapshot() for serializing attribution state; ChatRecordingService.recordAttributionSnapshot() writes to session JSONL; client.ts restores on session resume - PR attribution: addAttributionToPR() in shell.ts detects `gh pr create` and appends "🤖 Generated with Qwen Code (N-shotted by Qwen-Coder)" - Session baseline: saves content hash on first AI edit of each file for precise human/AI contribution detection - generatePRAttribution() method for programmatic access * fix: audit fixes — initial commit handling, cron prompt exclusion, failed commit counter preservation - Handle initial commit (no HEAD~1) by detecting parent with rev-parse and falling back to --root for first commit in repo - Exclude Cron-triggered messages from promptCount (not user-initiated) - Add commitSucceeded parameter to clearAttributions() so failed/disabled commits don't reset the prompts-since-last-commit counter - Add test for clearAttributions(false) behavior * fix: cross-platform and correctness fixes from multi-round audit - Normalize path.relative() to forward slashes for Windows compatibility - Use diff-tree --root for initial commits (git diff --root is invalid) - Replace String.replace() with indexOf+slice to avoid $& special patterns - Fix clearAttributions(false→true) when co-author disabled but commit succeeded - Use real newlines instead of literal \n in PR attribution text - Add surface fallback in restoreFromSnapshot for version compatibility - Fix single-quote regex to not assume bash supports \' escaping - Case-insensitive directory matching in generated file detection - Handle renamed file brace notation in parseDiffStat * fix(attribution): also snapshot on ToolResult turns so resume keeps tool edits Previously, recordAttributionSnapshot() only ran at the start of UserQuery and Cron turns — before the tools for that turn had executed. A session that wrote a file in turn 1 and committed in turn 2 (across process boundaries via --resume) lost the tracked edit: the last persisted snapshot was the turn-1-start snapshot (empty fileStates), so on resume the attribution service restored empty state and no git notes were attached to the commit. Move the snapshot call out of the UserQuery/Cron conditional and run it on every non-Retry turn. ToolResult turns are scheduled right after tools execute, so their start-of-turn snapshot now captures any edits those tools made. Retry turns are skipped since the state is unchanged from the prior turn. Added unit tests asserting the snapshot fires for ToolResult/UserQuery turns and skips Retry turns. Verified end-to-end in a scratch repo: write-file in turn 1 (no commit) → exit → --resume → commit in turn 2 → git notes now contain the recorded file with correct aiChars and promptCount: 2. * refactor(attribution): merge duplicate retry guard and update stale doc Collapse the two back-to-back messageType !== Retry blocks in sendMessageStream into one, and refresh chatRecordingService's recordAttributionSnapshot doc comment to reflect that snapshots fire on every non-retry turn (not just after user prompts). * feat(attribution): split gitCoAuthor into independent commit and pr toggles Matches the shape used upstream in Claude Code's `attribution.{commit,pr}` so users can disable the PR body line without losing the commit-message Co-authored-by trailer (or vice versa). The previous boolean forced both to move together, which conflated two different surfaces. - settingsSchema: gitCoAuthor becomes an object with nested commit/pr booleans, each `showInDialog: true` so both appear in /settings. - Config constructor accepts legacy boolean (coerced to { commit: v, pr: v }) so stored preferences from the pre-split schema carry over. - shell.ts: attachCommitAttribution and addCoAuthorToGitCommit read .commit; addAttributionToPR reads .pr. * feat(settings): add v3→v4 migration for gitCoAuthor shape change Legacy gitCoAuthor was a single boolean and shipped ~4 months ago; the previous commit split it into { commit, pr } sub-toggles. Without a migration, users who had set gitCoAuthor: false would see the settings dialog show the default (true) for both sub-toggles — misleading and likely to flip their preference on the next save because getNestedValue returns undefined when asked for .commit on a boolean. - New v3-to-v4 migration expands boolean → { commit: v, pr: v }, preserves already-object values, resets invalid values to {} with a warning. - SETTINGS_VERSION bumped 3 → 4; existing integration assertions use the constant so the next bump is a single-line change. - Regenerate vscode-ide-companion settings.schema.json to reflect the new nested shape. - Docs: split the single gitCoAuthor row into .commit and .pr. * test(migration): cover null/array/number and partial object for v3-to-v4 The migration already treats any non-boolean, non-object value as invalid (reset to {} with warning), but the existing test only exercised the string "yes" branch. Add parameterized cases for null, array, and number so a future regression that accepts these in the valid bucket gets caught. Also cover partial objects — the migration must not paternalistically fill defaults; that responsibility lives in normalizeGitCoAuthor at the Config boundary. * fix(shell): address PR review for compound commits and PR body escaping Two critical issues called out in review: 1. attachCommitAttribution treated the final shell exit code as proof that `git commit` itself failed. For compound commands like `git commit -m "x" && npm test`, the commit can succeed and a later step can fail; the previous code then cleared attribution without writing the git note. Now we snapshot HEAD before the command (via `git rev-parse HEAD` through child_process.execFile, kept independent of the mockable ShellExecutionService) and detect commit creation by HEAD movement, so attribution lands whenever a new commit was created regardless of later steps. 2. addAttributionToPR spliced the configured generator name into the user-approved `gh pr create --body "..."` argument verbatim. A name containing `"`, `$`, a backtick, or `'` could break the command or be evaluated as command substitution. Now we shell-escape the appended text per the surrounding quote style before splicing. Tests cover the new escape paths for both double- and single-quoted bodies, including a generator name designed to break interpolation (`$(rm -rf /) "danger" \`eval\``) and one with an apostrophe. * fix(attribution): address Copilot review on shell, schema, and totals Six items called out on PR #3115 by Copilot: - shell.ts: addAttributionToPR's bash quote escaping doesn't apply to cmd.exe / PowerShell, where `\$` and `'\''` aren't honored. Skip the PR body rewrite entirely on Windows — losing PR attribution there is preferable to corrupting the user-approved `gh pr create` command. - attributionTrailer.ts + shell.ts call site: buildGitNotesCommand used bash-style single-quote escaping on the JSON note, which is broken on Windows. Switched to argv form (`{ command, args }`) and routed the invocation through child_process.execFile so shell quoting is bypassed entirely. Tests updated to assert the argv shape. - commitAttribution.ts: when a tracked file's aiChars exceeded the diff --stat-derived diffSize (long-line edits where diffSize ≈ lines * 40), humanChars clamped to 0 but aiChars stayed inflated, leaving aiChars + humanChars > the committed change magnitude. Clamp aiChars to diffSize so the totals stay consistent. - shell.ts parseDiffStat: only normalized rename brace notation (`{old => new}`). Cross-directory renames emit `old/path => new/path` without braces, leaving diffSizes keyed by the full string. Added a second normalization step. - shell.ts: addAttributionToPR docstring claimed `(X% N-shotted)` but the implementation only emits `(N-shotted by Generator)`. Updated the docstring to match the actual behavior. - settingsSchema.ts + generator: gitCoAuthor went from boolean to object in the V4 migration. The exported JSON Schema now wraps the field in `anyOf: [boolean, object]` (via a new `legacyTypes` hint on SettingDefinition) so users with a stored boolean don't see a spurious IDE warning before their next launch runs the migration. * fix(attribution): parse binary diffs, source generator from model, sync schema $version Three follow-up review items from Copilot: - parseDiffStat now handles git's binary-diff format (`path | Bin A -> B bytes`) using the byte delta with a floor of 1. Without this, binary edits arrived at the attribution payload as diffSize=0 and were silently dropped. Also extracted the parser to a top-level exported function so the binary path is unit-testable; added five targeted cases (text/binary/rename normalisation/summary skip). - attachCommitAttribution now passes `this.config.getModel()` into generateNotePayload instead of the user-configurable `gitCoAuthor.name`. The note's `generator` field reflects which model produced the changes — and CommitAttributionService's sanitizeModelName() actually has the codename to scrub now. - generate-settings-schema.ts imports SETTINGS_VERSION instead of hardcoding `default: 3`, so a future bump propagates to the emitted JSON schema in one place. Regenerated settings.schema.json bumps $version's default from 3 to 4 to match the V4 migration. * fix(attribution): repo-root baseDir, escape co-author trailer, switch to numstat Three Critical items called out by wenshao: - attachCommitAttribution was passing config.getTargetDir() as `baseDir` to generateNotePayload, but getCommittedFileInfo returns paths relative to `git rev-parse --show-toplevel`. When the working directory was a subdirectory of the repo, path.relative produced `../...` keys that never matched in the AI-attribution lookup, silently zeroing out attribution for every file outside getTargetDir. StagedFileInfo now carries an optional `repoRoot` (filled in by getCommittedFileInfo via `git rev-parse --show-toplevel`) and the caller prefers it over the target dir. - addCoAuthorToGitCommit interpolated `gitCoAuthorSettings.name` and `.email` into the rewritten command without escaping. A name containing `$()`, backticks, or `"` could be evaluated as command substitution under double quotes, or break the user-approved `git commit -m "..."` quoting. Now escapes per the surrounding quote style with the same helpers addAttributionToPR uses, gates on non-Windows for the same shell-quoting reason, and fixes the regex to accept `-m"msg"` shorthand (no space) so users who type the bash-shorthand form aren't silently denied a trailer. - parseDiffStat used `git diff --stat` output and approximated each line as ~40 chars by parsing a graphical text bar. Replaced with `git diff --numstat` which gives unambiguous integer additions+deletions per file; the heuristic remains but the parser is no longer fooled by the visual `++--` markers. Binary entries fall back to a fixed estimate so they still land in the map (rather than dropping out as diffSize=0). Suggestions also addressed: stale duplicate JSDoc on addCoAuthorToGitCommit removed, misleading `clearAttributions` comments rewritten to describe what the boolean argument actually does. Tests cover the new shorthand path, escape behavior, and numstat parsing (text/binary/rename/malformed). * fix(shell): shell-aware git-commit detection and apostrophe-escape handling Two more Critical items called out by wenshao plus the matching Copilot quote-handling notes: - attachCommitAttribution and addCoAuthorToGitCommit now go through a shell-aware `looksLikeGitCommit` helper instead of a raw `\bgit\s+commit\b` regex. The helper splits the command on shell separators (`splitCommands`) and checks each segment, so `echo "git commit"` no longer triggers attribution clearing or trailer injection. The same helper bails on any segment that contains `cd` or `git -C <path>`, since either could redirect the commit into a different repo than our cwd — writing notes or capturing HEAD there would corrupt unrelated state. - The post-command attribution call now runs regardless of whether the shell wrapper aborted. `git commit -m "x" && sleep 999` could move HEAD and then time out, leaving the new commit without its attribution note while the stale per-file attribution stayed around for a later unrelated commit. attachCommitAttribution still gates on HEAD movement, so it's a no-op when no commit was actually created. - The `-m '...'` and `--body '...'` regexes used to match only the first quote segment, so a command like `git commit -m 'don'\''t'` (bash's standard apostrophe-escape form) would have the trailer spliced mid-message and break the command's quoting. The single- quote patterns now use a negative lookahead / inner alternation to either skip those messages entirely (commit path) or match the whole escape-aware body (PR path). Tests cover the new behavior: quoted "git commit" is left alone, the `cd && git commit` and `git -C` patterns get no trailer, and the apostrophe-escape form passes through unchanged for both `-m` and `--body`. * fix(attribution): drop magic 100 fallback for empty deletions Deleted files with no AI tracking now use diffSize directly. With numstat as the input source, diffSize is an exact count, and an empty-file deletion legitimately reports zero — a magic fallback would only inflate totals. * fix(shell): broaden git-commit detection, gate background, drop dead helpers Five Copilot follow-ups: - looksLikeGitCommit now strips leading env-var assignments (`GIT_COMMITTER_DATE=now git commit ...`) and a small allowlist of safe wrappers (`sudo`, `command`) before matching. The previous exact-prefix match silently skipped trailer injection on common real-world commit forms. - A new looksLikeGhPrCreate (same shell-aware shape) replaces the raw `\bgh\s+pr\s+create\b` regex in addAttributionToPR, so quoted text like `echo "gh pr create --body \"x\""` no longer triggers a command-string rewrite. - executeBackground refuses to run `git commit` and tells the user to re-run foreground. The BackgroundShellRegistry lifecycle has no hook for the post-command pre/post-HEAD comparison or git-notes write, so allowing the commit through would create the new commit without notes and leak stale per-file attribution into the next foreground commit. - recordDeletion was unused outside its own test — removed (and the test). When AI-driven deletions need tracking we'll add it with an actual integration point rather than carrying dead API surface. - generatePRAttribution was likewise unused; addAttributionToPR builds the trailer string inline. The two formats had already diverged. Removed the helper and its tests; reviving from git history is straightforward if a future caller needs it. Tests: env-var and sudo prefixes now produce trailers; quoted "gh pr create" leaves the command unchanged; existing 81 shell tests still pass alongside the trimmed 25 commitAttribution tests. * fix(shell): unified git-commit detection split by intent Six items called out across CodeQL, Copilot, and wenshao: - The earlier `looksLikeGitCommit`/`stripCommandPrefix` returned a single yes/no and rejected ANY `cd` in the chain. That fixed the wrong-repo case but also disabled attribution for `git commit -m "x" && cd ..` (commit already landed safely in our cwd; the cd came after). It also conflated three distinct decisions onto one predicate. New `gitCommitContext` returns both `hasCommit` and `attributableInCwd`, walking segments in order so that a `cd` AFTER the commit doesn't invalidate it. Callers now pick the right arm: - background-mode refusal uses `hasCommit` (refuses even `cd /elsewhere && git commit` since we can't attribute it afterward either way) - HEAD snapshot, addCoAuthorToGitCommit, and the attachCommitAttribution gate use `attributableInCwd` - Tokenisation switches from a regex while-loop to `shell-quote`'s `parse`. Quoted env values like `FOO="a b" git commit` now skip correctly (the old `\S*\s+` form would cut after the opening quote). Eliminates the CodeQL polynomial-regex alert at the same time since the `\S*\s+` pattern is gone. - attachCommitAttribution now snapshots prompt counters via `clearAttributions(true)` whenever a commit lands, even if no per-file attributions were tracked. Previously the early-return on `hasAttributions() === false` meant `promptCountAtLastCommit` never advanced, so a later `gh pr create` reported an inflated N-shotted count spanning multiple commits. Tests: env-var and sudo prefixes still produce trailers; quoted "git commit" / "gh pr create" leave commands unchanged; cd BEFORE commit suppresses the rewrite while cd AFTER commit does not; `git -C <path> commit` is treated as a commit (refused in background) but not as attributable. * fix(shell): position-independent git subcommand detection + bash-shell guard Six review items, two of them critical: - gitCommitContext was checking fixed-position tokens (`arg1`, `arg3`) and missed every git invocation that puts a global flag between `git` and the subcommand: `git -c user.email=x@y commit`, `git --no-pager commit`, `git -C /p -c k=v commit`, etc. In background mode these would slip past the refusal guard; in foreground they got no co-author trailer, no git note, and no prompt-counter snapshot. New `parseGitInvocation` walks past git's global flags (with their values) before reading the subcommand, and reports `changesCwd` for `-C` / `--git-dir` / `--work-tree`. - The Windows guard on addCoAuthorToGitCommit and addAttributionToPR used `os.platform() === 'win32'`, which incorrectly skipped Windows + Git Bash (`getShellConfiguration().shell === 'bash'`). Switched both to gate on `getShellConfiguration().shell !== 'bash'` so Git Bash users keep the feature. - attachCommitAttribution was re-parsing `gitCommitContext(command)` even though `execute()` already gates on `commitCtx.attributableInCwd`. Removed the redundant re-parse — drift between the two checks would silently diverge trailer injection from git-notes writes. - tokeniseSegment (formerly tokeniseProgram) now logs via debugLogger on parse failure instead of swallowing silently. Easier to debug if shell-quote ever throws on something unusual. - Added a comment on `cwdShifted` documenting that it's a one-way latch — `cd src && cd ..` will still skip attribution. The trade-off matches the wrong-repo guard's "better miss than corrupt unrelated repos" intent. - Stale `--stat` reference in the aiChars-clamp comment updated to `--numstat` to match the actual git command in ShellToolInvocation.getCommittedFileInfo. Tests: `git -c key=val commit` and `git --no-pager commit` now produce a trailer; existing 82 shell tests still pass. * fix(shell): refuse multi-commit attribution; misc review follow-ups Five follow-ups from the latest review pass: - attachCommitAttribution now refuses to write a single git note for shell commands that produce more than one commit (e.g. `git commit -m a && git commit -m b`). The singleton's per-file attribution map can't be partitioned across the individual commits, so attaching the combined note to HEAD would mis-attribute earlier commits' changes to the last one. Walks `preHead..HEAD` via `git rev-list --count`; on multi-commit detection it snapshots the prompt counters and bails with a debug warning instead of writing a misleading note. - parseGitInvocation now recognises the attached `-C/path` form (e.g. `git -C/path commit -m x`). shell-quote tokenises that as a single `-C/path` token which previously fell to the generic flag branch with `changesCwd = false`, leaving an out-of-cwd commit classified as attributable. - attachCommitAttribution dropped its unused `command` parameter (the caller already gates on `commitCtx.attributableInCwd`, so re-parsing was removed earlier; the parameter became dead). - Added wiring guards in edit.test.ts and write-file.test.ts: AI-originated edits/writes hit `CommitAttributionService.recordEdit`, `modified_by_user: true` skips, and write-file's distinction between a true new file and an overwritten empty file (`null` vs `''` old content) is now pinned by `aiCreated` assertions. * fix(attribution): partial-commit clear, symlink baseDir, gh/git flag handling Two Critical items, two Copilot, and five wenshao Suggestions: - attachCommitAttribution's `finally` block used to call `clearAttributions()` unconditionally, wiping per-file tracking for files the AI had edited but the user excluded from this commit. Added `clearAttributedFiles(committedAbsolutePaths)` to the service and the call site now passes only the paths that actually landed in this commit; entries for un-`add`ed files stay pending for a later commit. - generateNotePayload now runs both `baseDir` and each tracked absolute path through `fs.realpathSync` before `path.relative`. On macOS in particular `/var` symlinks to `/private/var`, so the toplevel from `git rev-parse --show-toplevel` and the absolute path captured by edit/write-file tools could diverge — producing `../../actual/path` keys in the lookup that never matched and silently zeroed all per-file AI attribution. - tokeniseSegment now consumes value-taking sudo flags (`-u`, `-g`, `-h`, `-D`, `-r`, `-t`, `-C`, plus the long forms). Without this, `sudo -u other git commit` left `other` standing in for the program name and skipped the trailer entirely. - A duplicate JSDoc block above `countCommitsAfter` (a leftover from the earlier extraction of `getGitHead`) was removed; both helpers now have one accurate comment each. - attachCommitAttribution's multi-commit guard now also runs when `preHead === null` (brand-new repo), via `git rev-list --count HEAD`. A compound `git init && git commit -m a && git commit -m b` no longer slips through and mis-attributes combined data to the last commit. - addCoAuthorToGitCommit's `-m` matching switched to `matchAll` and takes the LAST match. `git commit -m "title" -m "body"` puts the trailer at the end of the body so `git interpret-trailers` recognises it; the previous first-match behaviour stuffed the trailer in the title where git treats it as plain message text. - addAttributionToPR's `--body` regex accepts both space and `=` separators (`--body "..."` and `--body="..."`); the `=` form is common with gh. - New `parseGhInvocation` walks past gh's global flags (`--repo`, `-R`, `--hostname`) so `gh --repo owner/repo pr create ...` is detected. The earlier fixed-position check at tokens[1]/tokens[2] missed any command with a global flag. - getCommittedFileInfo now fans out the two `rev-parse` calls and the three diff calls with `Promise.all`. They're independent and serialising them was paying spawn latency 5× per commit. Tests: sudo with `-u user`, multi `-m`, `gh --repo owner/repo`, `--body="..."`, plus the existing 84 shell tests still pass. * fix(attribution): canonicalize file paths centrally in CommitAttributionService Two related Copilot follow-ups: - recordEdit/getFileAttribution/clearAttributedFiles now run input paths through fs.realpathSync before storing/looking up, so a symlinked path (e.g. macOS /var ↔ /private/var) resolves to the same key regardless of which form the caller passes. Previously edit.ts/write-file.ts handed in non-realpath'd absolute paths while generateNotePayload tried to realpath only inside its lookup loop, leaving partial-clear and clear-on-finally paths unable to find entries when the forms diverged. - restoreFromSnapshot also canonicalises on the way in so a session resumed from a pre-fix snapshot (where keys may not have been canonical) ends up with the same shape as newly recorded entries — otherwise a single file could end up with two parallel records. - generateNotePayload's lookup loop dropped its per-entry realpath call (now redundant since keys are canonical at write time), keeping only the realpath of `baseDir` (which still comes from `git rev-parse --show-toplevel` and may be a symlink). - Updated `clearAttributedFiles` doc to describe the new semantics: callers can pass either the resolved repo-relative path or an already-canonical absolute path, and either will match. * fix(attribution): canonicalize-from-root cleanup; fix mixed-quote -m / gh -R= Five review items, one Critical: - attachCommitAttribution now canonicalises via the repo *root* (one realpath call) and resolves committed paths against that canonical root, rather than per-leaf realpath inside clearAttributedFiles. At cleanup time the leaf for a just-deleted file no longer exists, so per-leaf fs.realpathSync would fail and silently fall back to a non-canonical path that misses the stored canonical key — leaving stale attributions for deleted files. clearAttributedFiles drops its internal realpath and now documents the canonical-paths-required precondition explicitly. - addCoAuthorToGitCommit picks the LAST `-m` regardless of quote style. Previously `doubleMatch ?? singleMatch` always preferred the last double-quoted match, so `git commit -m "Title" -m 'Body'` injected the trailer into the title where git interpret-trailers would silently ignore it. Now compares match indices, and the escape helper follows the actually-selected match's quote style. - parseGhInvocation handles `-R=value` (the equals form of the short `--repo` alias). `--repo=...` and `--hostname=...` were already covered; `-R=...` previously fell through to the generic flag branch and skipped the value. - New tests for the symlink-aware canonicalisation: macOS-style `/var` ↔ `/private/var` mapping is mocked via vi.mock on node:fs, with cases for record-then-look-up under either form, generateNotePayload with a symlinked baseDir, partial clear via the canonical-root-derived path (deleted leaf), and snapshot restore canonicalisation. - Doc-only: integration-test header comments updated from "V1 -> V2 -> V3" / "migration to V3" to reflect the actual V4 end state (assertions already used the literal `4`). * fix(shell): scope -m rewrite to commit segment, reject nested matches Two Critical findings on addCoAuthorToGitCommit, plus a Copilot maintainability nit: - The `-m` regex used to scan the whole compound command, so `git commit -m "fix" && git tag -a v1 -m "release"` would target the LATER tag annotation (last -m wins) and splice the trailer there instead of the commit message. The rewrite now scopes to the actual `git commit` segment via a new findAttributableCommitSegment(): same shell-aware walk gitCommitContext does, but returning the segment's character range so the regex can be run on a slice and spliced back into the original command. - Within the segment, a literal `-m '...'` *inside* a quoted body was treated as a real later -m. For `git commit -m "docs mention -m 'flag' for completeness"`, the inner single-quoted -m sits at a higher index than the real outer -m, and the previous index comparison would have it win — splicing the trailer mid-message and corrupting the quoting. The new code checks whether the candidate is nested inside the other quote-style's range (start/end containment) and prefers the outer match when so. - Hoisted three constant Sets (sudo flag list, git global flags taking values, git global flags shifting cwd, gh global flags) out of the per-call scope to module constants. Functional no-op, but keeps the parsing helpers easier to read and avoids re-allocating the Sets on every command. Two regression tests added for the cases above: - inner `-m '...'` inside the outer message body is preserved literally and the trailer lands after the body - `git tag -a v1 -m "release notes"` after a real `git commit -m "fix"` is left untouched, with the trailer appended to "fix" only * fix(attribution): cd-leak, numstat partial failure, $() bailout, gh pr new alias Five Critical/Suggestion items: - `cd subdir && git commit` (or any non-attributable commit chain whose HEAD movement still happens in our cwd, e.g. cd into a subdirectory of the same repo) used to skip attribution AND fail to clear pending per-file entries. Those entries then leaked into the next foreground commit, inflating its AI percentage. New `else if (commitCtx.hasCommit)` branch in execute() compares pre- and post-HEAD; if HEAD moved we drop the per-file state. preHead is now snapshotted whenever ANY commit was attempted, not only attributable ones. - getCommittedFileInfo's three diff calls run in `Promise.all`. If `--numstat` failed while `--name-only` succeeded, every file's diffSize would be 0 and generateNotePayload would clamp aiChars to 0 — emitting a structurally valid note with all-zero AI percentages. Detect the partial-failure shape (files non-empty, diffSizes empty) and return empty so no note is written. - addCoAuthorToGitCommit and addAttributionToPR now bail when the captured `-m`/`--body` value contains `$(`. The tool description recommends `git commit -m "$(cat <<'EOF' ... EOF)"` for multi-line messages, but the regex's `(?:[^"\\]|\\.)*` body group stops at the first interior `"` from a nested shell token — splicing the trailer there breaks the command before it reaches the executor. - looksLikeGhPrCreate now accepts `gh pr new` as well — it's a documented alias for `gh pr create` and was silently skipped. - Removed `incrementPermissionPromptCount` / `incrementEscapeCount` and their getters: they had no production callers, so the backing fields just round-tripped through snapshots as 0. The four snapshot fields are now optional so pre-fix snapshots that carry non-zero values still load cleanly and just get ignored. Three regression tests added: heredoc-style `-m "$(cat <<EOF...)"` preserved literally, heredoc-style `--body` likewise, `gh pr new --body "..."` rewritten with attribution. * fix(attribution): --amend, --message/-b aliases, .d.ts over-exclusion Four Copilot follow-ups, three of them user-visible coverage gaps: - `git commit --amend` was diffing `HEAD~1..HEAD` for attribution, which spans the entire amended commit (parent → amended) rather than the actual amend delta. A message-only amend would emit a note attributing every file in the original commit to this amend. New `isAmendCommit` helper detects the flag and getCommittedFileInfo switches to `HEAD@{1}..HEAD` (the pre-amend HEAD vs the amended HEAD); if the reflog is GC'd we bail with a warni
Upstream Sync — Apr 18-19, 2026
Syncs 6 upstream commits from QwenLM/qwen-code while preserving HopCode architecture.
Changes merged from upstream
Architecture preserved
Conflict resolution
packages/vscode-ide-companion/.../toolcalls/index.tsx — kept @hoptrendy/webui, added ToolCallData to type imports (upstream addition needed for AgentToolCall support)