docs: update README.md,improve Qwen API key configuration instructions, including the use of environment variables and .env files.#4
Open
BingqingLyu wants to merge 2 commits into
Open
Conversation
…s, including the use of environment variables and .env files.
BingqingLyu
pushed a commit
that referenced
this pull request
May 7, 2026
…wenLM#3831 PR-1 of 3) (QwenLM#3842) * feat(core): add signal.reason convention for ShellExecutionService.execute() Foundation for QwenLM#3831 Phase D (b) — Ctrl+B promote of a running foreground shell to background. Defines a discriminated `ShellAbortReason` union that the AbortSignal carries; default behavior (no reason / `{ kind: 'cancel' }`) keeps the existing tree-kill on abort. `{ kind: 'background' }` is a takeover signal — execute() skips the kill, drops the child from its active set (so cleanup() won't kill it later), flushes a snapshot of captured output, and resolves the result Promise immediately with `promoted: true` so the awaiting caller unblocks. Pure plumbing: no caller sets the reason yet, so this is a zero-behavior change for existing call sites. The `promoted?: boolean` field is optional on ShellExecutionResult so existing consumers compile against the new shape without source changes. Tests pin both branches in both childProcessFallback and executeWithPty: default abort still SIGTERM-tree-kills; `{ kind: 'cancel' }` is identical to default (pin against accidental routing through the background branch); `{ kind: 'background' }` skips the kill, snapshot output is preserved, mockProcessKill / mockPtyProcess.kill are NOT called. Part of QwenLM#3831 (Phase D part b — Ctrl+B promote running shell to background). PR-1 of 3. * fix(core): detach service listeners on background-promote (resolve review) Addresses 4 Critical + 2 Suggestion findings on PR-1 of QwenLM#3831: - **childProcess listener detach** (review line 555 + 573): Anonymous arrow listeners on stdout/stderr/error/exit could not be off()'d. After background-promote, post-promote bytes would re-enter handleOutput, which then calls decoder.decode() on a now-finalized text decoder (cleanup() already called .decode() without stream:true) → TypeError crash. Even without the crash, old onOutputEvent would fire for new data → ownership contract violation + duplication. Fix: extract named handler refs (stdoutHandler / stderrHandler / errorHandler / exitHandler) and call off() on all four in the background-promote branch via a detachServiceListeners() helper. - **PTY listener detach** (review line 967 + 990): node-pty's onData / onExit return IDisposable handles; the abort handler now captures dataDisposable / exitDisposable and calls .dispose() in the background-promote branch. ptyProcess.on('error') is EventEmitter-style (not IDisposable) — extract a named ptyErrorHandler ref and off() it. Without these, post-promote PTY error throws → Node.js crash; post-promote data continues writing to headlessTerminal and calling old onOutputEvent → ownership violation. - **PTY in-flight chain item ownership** (related to review line 990): processingChain may have already-enqueued callbacks past the early listenersDetached check. Refactored from "early-return short-circuit" to "guard each onOutputEvent emit individually" so in-flight writes still LAND in headlessTerminal (snapshot reflects them) but no events leak to the foreground onOutputEvent. Also clear renderTimeout in the abort handler so a pending throttled render doesn't fire post-promote. - **PTY snapshot freshness** (review line 972, suggestion): The original abort handler called serializeTerminalToText immediately. Now we await Promise.race([processingChain drain, SIGKILL_TIMEOUT_MS]) first (mirrors the onExit finalize pattern at ~line 970) so in-flight headlessTerminal.write callbacks land before serialization. Skipped render(true) intentionally because it would emit final onOutputEvent data (renderFn calls onOutputEvent), violating the "no emit post-promote" invariant — added a comment explaining why direct serialize is correct. - **Handoff-boundary tests** (review line 1257, suggestion): Added 4 new tests pinning the ownership contract — 2 for child_process (post-promote stdout/stderr does NOT route to onOutputEvent; child exit does NOT re-resolve result), 2 for PTY (data/exit disposables ARE called; result shape stays promoted: true even if post-promote events fire). Also: test setup now stubs mockPtyProcess.onData / .onExit to return { dispose: vi.fn() } so the background-promote path's dispose() calls don't crash on undefined (the stub's mock.results[0].value is then inspected by the new handoff tests). 58 / 58 tests pass (50 baseline + 4 first-pass + 4 handoff). Total +235 / -35 on top of the prior commit. * fix(core): defensive hardening for ShellExecutionService background-promote (resolve 2nd review pass) Addresses 6 follow-up [Suggestion] threads on PR-1 of QwenLM#3831 — all substantive code-quality issues raised by the second-pass review of the dispose-based detach commit (8e8e18c): - **Exhaustive switch on `ShellAbortReason.kind`** (both abort handlers). Earlier `if (reason?.kind === 'background')` form silently fell through to kill for any unrecognized variant — a future `{ kind: 'suspend' }` would have killed the process with zero compile-time signal. Switched to `switch (kind)` with a `never`-typed default that runs `debugLogger.warn` and falls back to the safest behavior (cancel/kill). Each branch is now extracted into a named helper (`performBackgroundPromote` / `performCancelKill`) so the switch body stays a single screenful. - **Each `dispose()` wrapped in its own try/catch** (PTY). node-pty's `IDisposable` contract doesn't guarantee no-throw. Without per-dispose try/catch a single throwing dispose() would skip subsequent cleanup (the other dispose, off('error'), activePtys.delete, drain, resolve) and the caller would hang forever on `await result`. Each call now logs via debugLogger.warn on failure but continues. - **`.catch(() => undefined)` on the processingChain side of the drain race** (PTY). `Promise.race([processingChain.then(drain).then(drain), timeout])` would propagate a chain rejection out of the race; since `addEventListener` doesn't await our handler, the rejection became unhandled and `resolve()` was never called → caller hung. Now the rejection is swallowed; the timeout side still terminates the race on time. - **Drain-timeout truncation now emits a diagnostic warning** (PTY). Previously the 200ms drain timeout could fire, the snapshot would be taken with the buffer in mid-write state, and the result.output would be silently truncated. Race result is now observed via a symbol sentinel; when the timeout side wins, debugLogger.warn fires pointing the user at rawOutput as the un-truncated fallback. - **Snapshot serialize failure logs instead of swallowing silently** (PTY). Empty `catch {}` made result.output indistinguishable from "command produced no output" if serializeTerminalToText threw. Now `debugLogger.warn` with the error message leaves a trail for support bundles. - **Dedicated `PROMOTE_DRAIN_TIMEOUT_MS` constant** separated from `SIGKILL_TIMEOUT_MS`. Both are 200ms today, but they have unrelated reasons-to-change (kill escalation timing vs. promote drain ceiling) — sharing the constant means tuning one would silently change the other. Also adds a module-level `debugLogger = createDebugLogger('SHELL_EXECUTION')` since the service had no logging surface before this commit. 58 / 58 tests pass; tsc clean; ESLint clean. No new tests added: the new behaviors (timeout sentinel firing, dispose throw, exhaustive switch default) are defensive log-only paths; existing handoff tests already cover the happy path. Adding mock-throw tests is reasonable follow-up but not blocking. * fix(core): real bug — ptyProcess.off → removeListener; defensive abort-reason read Resolves the third review pass on PR-1 of QwenLM#3831 — 1 real bug + 2 defensive hardenings: - **Real bug: `ptyProcess.off('error', ...)` throws TypeError at runtime** (line ~1074). `@lydell/node-pty`'s `IPty` interface exposes the legacy Node EventEmitter `removeListener`, not the modern `off` alias. Previous form threw, the surrounding try/catch swallowed it (post-prior-pass dispose hardening), but the old `ptyErrorHandler` stayed registered — so a post-promote PTY error would still hit our foreground handler and `throw err`, breaking the handoff contract that PR-1's whole listener-detach work is supposed to enforce. Switched to `removeListener`. The catch + warn stays as defense-in-depth; the message wording is updated. - **Prototype-pollution-safe `kind` read** (extracted to module-level helper `getShellAbortReasonKind`). The previous `reason?.kind` walked the prototype chain — a polluted `Object.prototype.kind = 'background'` would silently route `abortController.abort({})` (any plain object reason) into the promote branch and skip the kill. Lifecycle/safety branch deserves the extra check. Helper now: rejects non-object reasons; reads `kind` only as an OWN property (`hasOwnProperty`); whitelists against `'background' | 'cancel'`; defaults to `'cancel'` (the safe historical behavior) for everything else. Both abort handlers (childProcess + PTY) now share this helper. - **`streamStdout: true` + background-promote = silent empty snapshot** (childProcess `performBackgroundPromote`). The promote snapshot reads from the `stdout` / `stderr` string accumulators; but in `streamStdout` mode `handleOutput` forwards bytes through `onOutputEvent` and skips the accumulators entirely. Today PR-1's only call site (foreground shell.ts) uses `streamStdout: false`, so the combination is unreachable — but if a future caller pairs the two, `result.output` would be empty with no diagnostic. Added a `debugLogger.warn` when the combination occurs, pointing the caller at `rawOutput` as the fallback. Cheaper than building a parallel accumulator just for this latent case. 58 / 58 tests pass; tsc clean; ESLint clean. * fix(core): liveness check + throw-safe abort-reason read + encoding-aware PTY snapshot (resolve 4th review pass) Resolves 6 threads on PR-1 of QwenLM#3831 — 1 Critical + 1 real bug + 2 quality + 2 test-coverage: - **[Critical] `getShellAbortReasonKind` throw-safe property read.** Previous form read `reason.kind` after only checking that `kind` is an own property. An own accessor that throws (or a Proxy with a trapping getter) would throw before the helper reached either the cancel kill path or the background promote path. Abort handlers are dispatched async and not awaited by AbortSignal, so a leaked throw here would have left the shell process alive instead of being killed on cancel — quietly. Wrapped the property read in try/catch with a fall-back to the safe 'cancel' kill behavior. - **Real bug: child_process post-exit race in background-promote** (`performBackgroundPromote`). The child may have already exited but the 'exit' event hasn't reached our handler yet (Node delivers events on the next microtask). Promoting in that window would detach our exit listener and report `promoted: true` for a process that's already dead — the caller would hold an inert pid expecting to take over. Now we read `child.exitCode` / `child.signalCode` before detaching: if either is non-null, fall through and let the pending exit handler resolve normally with the real exit info. Mirrored mock setup so `exitCode` / `signalCode` default to `null` (matching real ChildProcess) instead of `undefined`. - **PTY snapshot: re-decode + replay (mirror exit-path encoding).** The promoted snapshot was serializing `headlessTerminal` directly, which was fed by a streaming decoder initialized from the first-chunk encoding heuristic. When early output is ASCII-only but later output is in a different encoding (GBK / Shift-JIS / etc.), this produces mojibake — and the normal exit path doesn't, because it re-decodes `finalBuffer` with `getCachedEncodingForBuffer` and replays through a fresh terminal. Now mirrors that logic so `result.output` shape matches across the two paths. Direct-serialize remains as a last-ditch fallback if replay throws. - **Switch `default` no longer emits a runtime warn.** Reviewer noted the helper's whitelist made the `default: { _exhaustive: never }` branch unreachable at runtime — the `debugLogger.warn` in it could never fire. Kept the `_: never = kind` type assertion (so a future ShellAbortReason variant forces a TS error here, directing the developer to extend BOTH the helper's whitelist AND add a `case`), removed the unreachable warn. Added a comment that the assertion is the static-only safety net the union expansion would trigger. - **Direct unit tests for `getShellAbortReasonKind`** (8 cases). The helper's prototype-pollution defense is the main reason it exists; if `hasOwnProperty` is accidentally removed the regression would silently send `abortController.abort({})` (any plain reason) into the promote path. Exported the helper and added direct tests for: null / undefined, non-object, empty object (no own kind), prototype- only kind (pollution), unknown kind value, throwing accessor, Proxy trap, and the two happy paths. - **`removeListener` regression guard.** The fix to call `ptyProcess.removeListener('error', ...)` instead of `.off(...)` matters because `@lydell/node-pty`'s IPty interface only exposes `removeListener` — `.off()` throws TypeError on a real PTY but the EventEmitter mock tolerates both. Added a test that spies on both methods and asserts the production code uses `removeListener` for the 'error' event, so a future swap back to `.off()` regresses loudly under the mock instead of silently. 68 / 68 tests pass (58 baseline + 9 helper boundary + 1 removeListener guard + 1 post-exit race); tsc clean; ESLint clean. * fix(core): PTY background-promote post-exit race guard (resolve 5th review pass) Mirrors the child_process post-exit race fix from 4cc558b into the PTY path — addresses 1 [Critical] thread on PR-1 of QwenLM#3831: The PTY may have already exited but our `exitDisposable` (onExit callback) hasn't run yet — node-pty delivers the exit event asynchronously after the PTY's native SIGCHLD, so there's a window between "PTY actually dead" and "service onExit fires". Promoting in that window detaches our exit listener and reports `promoted: true` for a dead PTY, losing the real exit status; the caller would hold an inert pid expecting to take over. The IPty interface doesn't expose an `exitCode` field we can read directly (unlike `child.exitCode` / `child.signalCode` for child_process), so use `process.kill(pid, 0)` as a best-effort liveness check via the existing `ShellExecutionService.isPtyActive` helper. If kill(pid, 0) throws ESRCH, the pid is gone — log at debug level and fall through, letting the pending onExit callback resolve normally with the real exit info. Also adds a unit test mirroring the child_process race test: mocks `process.kill(pid, 0)` to throw ESRCH on the liveness probe, asserts the result has no `promoted: true` and reports the real exitCode. 69 / 69 tests pass; tsc clean; ESLint clean. * docs(core): correct getShellAbortReasonKind boundary-test count in JSDoc Doc said 'all six edge cases' but the test suite has 8 cases (added Proxy-trap and undefined later). Off-by-2 cosmetic only — no behavior change. Caught during a multi-round self-audit of PR-1 of QwenLM#3831. Audit summary: 7 rounds (correctness / reverse / consistency / coverage / build / exception paths / style) found one false-positive (a sync- abort registration-order race I initially thought existed). Verified that Node's WHATWG AbortSignal does NOT auto-fire 'abort' listeners on already-aborted signals, so the race window cannot open. No code change needed for that scenario; this commit is just the JSDoc fix. 69 / 69 tests still pass; tsc + ESLint clean. * docs(core): document the helper / union / switch sync invariant explicitly Multi-round self-audit found that `getShellAbortReasonKind`'s value whitelist has no compile-time tie to the `ShellAbortReason` union: when the union grows, TypeScript's `_exhaustive: never` in each switch forces #3 (the case arm) to be added, but the helper's whitelist (#2) silently keeps degrading the new variant to 'cancel', and the new case arm is never reached at runtime. Reviewer #4 raised this on the second pass; the original commit chose to accept it (option B in that thread) but didn't leave a strong in-code signal for future contributors. Added an INVARIANT block inside the helper enumerating the three sites that must be kept in sync, so the next person extending `ShellAbortReason` sees the coupling at the place where they're most likely to forget it. No behavior change — comment-only. 69 / 69 tests still pass; tsc + ESLint clean. Audit summary (this round + prior round): 18 angles total over two sweeps and one reverse-attack pass. Found: - 0 real bugs - 1 false-positive race (sync-abort registration order — Node WHATWG AbortSignal does NOT auto-fire on already-aborted signals; investigated, reverted) - 1 cosmetic doc fix (boundary-test count off-by-2) - 1 cosmetic INVARIANT block (this commit) Areas reviewed without finding new issues: caller-side ShellExecutionResult shape compatibility (optional `promoted?` field, existing callers spread-untouched); `exited` flag lifecycle (monotonic, cleanup() idempotent); processingChain in-flight ownership (listenersDetached guards every onOutputEvent emit including the renderFn-rendered case via the same flag); race between exit event and abort handler (both microtasks, FIFO ordering gives correct outcome either way); Node version dependence (`AbortSignal.reason` is Node 17.2+, engines: >=20 covers it); test isolation (mockImplementationOnce + module-level mockProcessKill clears each beforeEach); `process.kill(pid, 0)` Windows liveness reliability (best-effort, acceptable for PR-1 plumbing); PID reuse race on the PTY liveness check (theoretically possible, microsecond window, unavoidable at the OS level — rejected in spec discussion); PR-2/PR-3 contract surface (caller MUST attach listeners before abort — documented; any future caller violating this is its own bug). * test(core): align mockChildProcess.exitCode/signalCode in second beforeEach The 'execution method selection' describe block has its own beforeEach (separate from 'child_process fallback') that builds mockChildProcess but does not set `exitCode` / `signalCode = null`. Real Node `ChildProcess.exitCode` / `signalCode` are `null` while the process is alive — and production now reads these in the background-promote race guard. The current tests in this block don't exercise the promote path, so they pass regardless, but any future promote-related test landing here would silently trip the guard (`undefined !== null` is true) and fall through to the normal-exit branch instead of promoting. Mirror the `child_process fallback` block's mock setup so the two beforeEach hooks produce equivalent ChildProcess shapes, eliminating a quiet foot-gun for future contributors. Comment-only / test-fixture change. 69 / 69 tests still pass; tsc clean. Found during a deeper third-round self-audit of PR-1 of QwenLM#3831.
BingqingLyu
pushed a commit
that referenced
this pull request
May 11, 2026
…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 warning rather than over-attribute. - `git commit --message "..."` and `--message="..."` were silently skipped because the regex only recognised the short `-m` form. The flag prefix now matches both alternatives via `(?:-[a-zA-Z]*m|--message)\s*=?\s*` (non-capturing inner group so the existing `[full, prefix, body]` destructure still works). - `gh pr create -b "..."` (the short alias for `--body`) was the same gap on the PR side; `(?:--body|-b)[\s=]+` now covers both forms. - `.d.ts` was an over-broad blanket exclusion in EXCLUDED_EXTENSIONS — declaration files are commonly authored (ambient declarations, asset shims like `*.d.ts` for `import './x.svg'`); the repo even contains `packages/vscode-ide-companion/src/assets.d.ts`. Removed `.d.ts` from the extensions Set and adjusted the test to assert the new behavior. Auto-generated `.d.ts` (e.g. `tsc --declaration` output) still gets caught by the build-directory rules. Tests added: `--amend` plumbing covered by the new branch in getCommittedFileInfo (no targeted unit test — the diff invocation goes through ShellExecutionService and is exercised by the existing post-command path); `--message`/`--message="..."`/-b/-b="..."` all have positive trailer-injection assertions; `.d.ts` test split into "hand-authored" (negative) and "in dist" (positive). * fix(attribution): cd-subdir, scope --body, multi-commit count guard, /clear reset Four bugs flagged this round: - gitCommitContext / findAttributableCommitSegment used a blanket "any cd shifts cwd" gate, breaking the very common `cd subdir && git commit -m "..."` flow even though the commit lands in the same repo. New `cdTargetMayChangeRepo` heuristic: treat relative paths that don't escape upward (no leading `..`, no absolute path, no `~`/`$VAR` expansion, no bare `cd`/`cd -`) as in-repo and let attribution proceed. Conservative on anything it can't statically verify. - addAttributionToPR was running the `--body`/`-b` regex against the FULL compound command string. In `curl -b "session=abc" && gh pr create --body "summary"` the regex would match curl's `-b` cookie flag and inject attribution into the cookie value, corrupting the curl call. Added `findGhPrCreateSegment` (analog of `findAttributableCommitSegment`) and scoped the body regex to that segment, splicing back into the original command via offsetting the in-segment match index. - The multi-commit guard treated `runGitCount === 0` as "single commit" and bypassed itself. After `commitCreated === true`, a count of 0 is impossible in normal operation — it means rev-list errored or timed out. Now we bail on `commitCount !== 1` with a tailored message: anything other than exactly 1 commit is suspicious and refuses the note. - The CommitAttributionService singleton survives across `Config.startNewSession()` (the `/clear` and resume paths). New `CommitAttributionService.resetInstance()` call alongside the existing chat-recording / file-cache resets in startNewSession prevents pending attributions from a prior session attaching to a commit in the new one. Three regression tests added: `cd src && git commit` produces a trailer (in-repo cd), `cd .. && git commit` does not (could escape repo root), and `curl -b "..." && gh pr create --body "..."` leaves curl's cookie value untouched while attribution lands in gh's body. * fix(attribution): cd embedded .., env wrapper, Windows ARG_MAX, segment-locator warn Four review items, all small but real: - cdTargetMayChangeRepo missed embedded `..` traversal — `cd foo/../../escape` and similar would slip past the leading-`..` check and be treated as in-repo. Added an `includes('/..')` / `includes('\\..')` check (catches POSIX and Windows separators without false-positiving on `..` chars inside ordinary names, which only escape when followed by a separator). - tokeniseSegment now recognises `env` as a safe wrapper alongside `sudo`/`command`, so `env GIT_COMMITTER_DATE=now git commit ...` resolves to `git`. After the wrapper detection we also skip any `KEY=VALUE` argv entries (env's own argument syntax for setting vars before the program). - buildGitNotesCommand's MAX_NOTE_BYTES dropped from 128 KB to 30 KB. Windows' CreateProcess lpCommandLine is capped around 32,768 UTF-16 chars including the executable path and other argv entries; a 128 KB note would still fail to spawn even though the function returned a command instead of null. 30 KB leaves ~2 KB of headroom for the rest of the argv on Windows and is larger than any real commit's metadata in practice. - findAttributableCommitSegment / findGhPrCreateSegment now log a debugLogger.warn when `command.indexOf(sub, cursor)` returns -1 — splitCommands strips line continuations (`\<newline>`), so a multi-line command can have the trimmed segment text fail to match its source. Previously the segment was silently skipped with no signal; the warn makes the failure observable when QWEN_DEBUG_LOG_FILE is set. Two regression tests added: `cd foo/../../escape && git commit` gets no trailer (embedded-`..` heuristic catches it), and `env GIT_COMMITTER_DATE=now git commit` does (env wrapper skipped). * fix(attribution): scope isAmendCommit to attributable segment only `git -C ../other commit --amend && git commit -m x` would previously flag the second (fresh) commit as an amend, causing attachCommitAttribution to diff `HEAD@{1}..HEAD` against an unrelated reflog entry. Mirror findAttributableCommitSegment's cd/cwd tracking so only the first commit segment that runs in the original cwd determines amend status. * fix(attribution): last-match --body, symlink leaf canonicalisation, scoped prompt count - addAttributionToPR: use matchAll/last-match for `--body`/`-b` so the trailer lands in the gh-honoured (final) body when multiple flags are present. Mirrors addCoAuthorToGitCommit. Adds regression test. - attachCommitAttribution: also fs.realpathSync the per-file resolved path (not just the repo root) so files behind intermediate symlinks are matched against canonical keys recordEdit stored, instead of silently zeroing attribution and leaking entries past commit. - incrementPromptCount: scope to SendMessageType.UserQuery — ToolResult, Retry, Hook, Cron, Notification are model/background re-entries of the same logical turn. Tracking them all inflated the "N-shotted" trailer (one user message could become 10-shotted with 10 tool calls). - AttributionSnapshot: add `version: 1` field; restoreFromSnapshot now refuses incompatible versions and validates per-field types so a partially-written snapshot can't seed `Math.min(undefined, n) === NaN` into git-notes payloads. - Drop unused permission/escape counters (declared, persisted, never read or incremented) — fields, snapshot tolerance, and clear-method bookkeeping all removed; AttributionSnapshot interface simplifies. - isGeneratedFile: switch directory rule from substring `.includes('/dist/')` to segment-boundary check (split on `/`) so project dirs like `my-dist/` or `xbuild/` don't match. `.lock` removed from the blanket extension exclusion — well-known lockfiles already covered by EXCLUDED_FILENAMES; hand-authored `.lock` files (e.g. `.terraform.lock.hcl`) now stay attributable. - getClientSurface: document `QWEN_CODE_ENTRYPOINT` as the embedder override hook so the always-`'cli'` default is intentional. * fix(attribution): skip values for env -u NAME and -S string `env`'s value-taking flags (`-u`/`--unset`, `-S`/`--split-string`) were not in the wrapper's flag-skip allowlist, so `env -u FOO git commit ...` left FOO as the next token and the parser treated it as the program — masking the real `git commit` from attribution detection. Add an ENV_FLAGS_WITH_VALUE table mirroring the sudo allowlist. Regression test added. * fix(attribution): submodule leak, PR body nesting, shallow-clone bail, schema default - attachCommitAttribution: when HEAD didn't move in our cwd, leave pending attributions alone instead of dropping them. The case can be a failed commit, `git reset HEAD~1`, OR `cd submodule && git commit` (inner repo's HEAD moves, ours doesn't). Dropping was overly aggressive and silently lost outer-repo edits in the submodule case. - addAttributionToPR: mirror addCoAuthorToGitCommit's nested-match rejection so `gh pr create --body "docs mention -b 'flag'"` picks the outer `--body`, not the inner literal `-b`. Splicing into the inner match would corrupt the body. Regression test added. - getCommittedFileInfo: when `rev-parse --verify HEAD~1` fails, also check `rev-list --count HEAD === 1` to confirm HEAD is the true root commit. In a shallow clone, HEAD~1 is unreadable but the commit has a parent recorded — falling back to `diff-tree --root` would diff against the empty tree and over-attribute the entire commit. Bail with a debug warning instead. - generate-settings-schema: lift `default` (and `description`) out of the inner `anyOf[N]` schema to the outer level when wrapping with `legacyTypes`. Most JSON-schema-driven editors only surface top-level defaults; burying the default under `anyOf` lost the "enabled by default" hint. Also extend the default filter to publish non-empty plain objects (so `gitCoAuthor`'s default can appear). gitCoAuthor's source default updated to the runtime shape `{commit: true, pr: true}` to match `normalizeGitCoAuthor`. * fix(attribution): drop unsafe full-clear, tag analysis-failure with null ju1p (Copilot): the `else if (commitCtx.hasCommit)` branch fully cleared the singleton on `cd /abs/same-repo/subdir && git commit` (or `git -C . commit`), losing pending AI edits the user hadn't staged. We can't tell which files were in the commit from this branch, and the next attributable commit's partial-clear handles cleanup correctly anyway. Drop the branch entirely. ju2D (Copilot): `getCommittedFileInfo` returned the same empty StagedFileInfo for both "could not analyze" (shallow clone, --amend without reflog, --numstat partial failure, exception) and "intentionally empty" (--allow-empty). The caller couldn't tell them apart, so the partial clear became a no-op on analysis failure and the just-committed AI edits leaked to the next commit. Switch the return type to `StagedFileInfo | null` and have the caller treat null as "fall back to full clear" while empty StagedFileInfo (--allow-empty) leaves attributions intact for the next real commit. * fix(attribution): dedup snapshot writes, cap excludedGenerated, doc commit toggle scope rsf- (Copilot): recordAttributionSnapshot wrote a full snapshot to the JSONL on every non-retry turn, even when the tracked state was unchanged. Long-running sessions accumulated thousands of identical snapshot copies, inflating session size and slowing /resume hydrate. Dedup by JSON-equality with the prior write — first write always goes through, identical successors are no-ops. rsgo (Copilot): excludedGenerated path list was unbounded. A commit churning thousands of generated artifacts (large dist/ rebuild) could push the JSON note past MAX_NOTE_BYTES (30KB) and lose attribution for the real source files in the same commit. Cap the serialized sample at MAX_EXCLUDED_GENERATED_SAMPLE (50) and add excludedGeneratedCount for the true total. rsg9 + rshM (Copilot): the gitCoAuthor.commit description claimed the toggle only controlled the Co-authored-by trailer, but attachCommitAttribution also gates the per-file git-notes payload on the same flag. Update both the schema description and the settings.md table to mention both effects so disabling the option isn't a silent surprise. * fix(attribution): depth-1 shallow detection, snapshot dedup post-rewind/post-failure sfGz (Copilot): rev-list --count HEAD === 1 cannot distinguish a true root commit from a depth-1 shallow clone — both report 1 because rev-list only walks locally available objects. Switch to git log -1 --pretty=%P HEAD which reads the parent SHA directly from commit metadata: empty means a real root, non-empty means a parent is recorded (whether or not its object is local). The shallow-clone bail is now reliable. sfIm (Copilot): the dedup key persisted across rewindRecording, so the previous snapshot living on the now-abandoned branch would match the next post-rewind snapshot and silently skip the write, leaving /resume on the rewound session with no attribution state. Reset lastAttributionSnapshotJson when rewindRecording fires. sfJE (Copilot): dedup key was committed before the async write settled. A transient write failure would update the key, then permanently suppress all future identical snapshots even though nothing was ever persisted. Switch to optimistic-set then rollback on appendRecord rejection — synchronous identical calls dedup cleanly, but a failed write clears the key so the next identical snapshot retries. appendRecord now returns the per-record write promise (writeChain still has its swallow-catch for chain liveness) so callers needing per-write success can react to it. Tests added in chatRecordingService.test.ts for both rewind-reset and rollback-on-failure paths. * fix(attribution): preHead race, regex apostrophe-escape, surface failures, dead code t2G0 (deepseek-v4-pro): addCoAuthorToGitCommit single-quote regex now matches the bash close-escape-reopen apostrophe form using ((?:[^']|'\\'')*) — the same pattern bodySinglePattern uses for gh pr create. Input like git commit -m 'don'\''t' was previously silently un-rewritten because the negative lookahead bailed; the trailer now lands at the FINAL closing quote. Test updated. tMBP (gpt-5.5): preHead capture switched from concurrent async getGitHead to a synchronous getGitHeadSync (execFileSync) BEFORE ShellExecutionService.execute spawns the user's command. A fast hot-cached git commit could move HEAD before the async rev-parse resolved, leaving preHead === postHead and silently skipping the attribution note. Trade ~10–50 ms event-loop block per commit-shaped command for correctness of the post-command HEAD comparison. t2Gv (deepseek-v4-pro): attribution write failures (note exec non-zero, payload too large, diff-analysis exception, shallow clone / amend-without-reflog) are now surfaced on the shell tool's returnDisplay AND llmContent so the user and agent both see when their commit succeeded but the per-file git note didn't land. attachCommitAttribution now returns string | null (warning text or null for intentional skips like no-tracked-edits). Co-authored-by trailer is unaffected — only the note is gated by these failures. t2Gy (deepseek-v4-pro): committedAbsolutePaths now matches against the canonical keys already stored in fileAttributions (matchCommittedFiles iterates by relative path against the canonical repo root) instead of re-resolving each diff path on the fly. realpathSync(resolved) failed for deleted files and didn't follow intermediate symlinks, leaving stale per-file attribution alive past commit and inflating AI percentages on subsequent commits. t2HI (deepseek-v4-pro): removed dead sessionBaselines / FileBaseline / contentHash / computeContentHash infrastructure (~40 lines). The fields were written, persisted, and restored but never read for any computation or decision. AttributionSnapshot schema stays at version 1 — restore tolerates pre-fix snapshots that carried the now-ignored baselines field. t2HM (deepseek-v4-pro): extracted the duplicated lastMatch helper in addCoAuthorToGitCommit and addAttributionToPR into a single module-level lastMatchOf so future fixes can't be applied to only one copy. * chore(schema): regenerate settings.schema.json to match gitCoAuthor.commit description The settingsSchema.ts source for `gitCoAuthor.commit.description` was updated in 3c0e3293b but the JSON schema only picked up the OUTER description rewrite and missed this inner property's. The Lint check ("Check settings schema is up-to-date") fails on that drift; this commit re-runs `npm run generate:settings-schema` to sync them. * fix(attribution): preserve unstaged AI edits across cleanup branches uxU5 + uxVQ + uxUO (Copilot): every cleanup branch in attachCommitAttribution that called clearAttributions(true) was wholesale-erasing pending AI edits for files the user never staged in this commit. Reviewer scenarios: - multi-commit chain (`commit a && commit b`) bails out without writing a note, but unstaged edits to file Z (touched by neither commit) get cleared along with the chain's committed files. - attribution toggle off: same — toggling the flag wipes pending unstaged work. - analysis failure (shallow clone, --amend without reflog, partial diff failure): the finally-block fallback wholesale-cleared every pending file, consuming unrelated AI edits. - 0%-AI commit: when no file in the commit was AI-touched, generateNotePayload was emitting an "0% AI" note attached to a commit that legitimately had no AI involvement — actively misleading metadata. Add `noteCommitWithoutClearing()` to the service: snapshots the prompt counter as the new "at last commit" but leaves the per-file map alone. Use it in the multi-commit, no-tracked-edits, toggle-off, and analysis-failure paths. The committed-files partial-clear (clearAttributedFiles) still runs in the success path. The 0%-AI no-match case now skips the note write entirely. * fix(attribution): runGit null-on-failure, versionless v3→v4 migration z54M (Copilot): runGit returned '' on both successful-empty-output and silent failure, so a `--name-only` that errored mid-way through the diff fan-out aliased to a real `--allow-empty` commit. The empty-commit branch then preserved pending attributions, leaving the just-committed file's tracked AI edit alive to re-attribute on the next commit. Switch runGit to `Promise<string | null>`, distinguishing exit code 0 (any output, including '') from non-zero (null). The diff-stage fan-out and ancillary probes now treat null as analysis failure and bail with `return null` instead of falling into the empty-commit path. z539 (Copilot): the v3→v4 `shouldMigrate` only fired on `$version === 3`. A versionless settings file carrying the legacy `general.gitCoAuthor: false` boolean would skip every migration (gitCoAuthor isn't in V1_INDICATOR_KEYS — it post-dates V2), get its `$version` normalized to 4 by the loader, and leave the boolean in place. The settings dialog then reads the V4 `{commit, pr}` shape, sees missing keys, defaults both to true, and silently overwrites the user's opt-out on the next save. Also fire when `$version` is absent AND the value at `general.gitCoAuthor` is a boolean. Tests cover the new path and confirm the existing versioned/object-shape paths are untouched. * fix(attribution): toggle-off partial clear, normalizeGitCoAuthor type-check, terraform lockfile 0oAK (Copilot): the gitCoAuthor.commit toggle-off branch returned before computing the committed file set, leaving the just-committed files' tracked AI work in the singleton. Re-enabling the toggle and committing the same file again would re-attribute earlier (already- committed) AI edits to the new commit. Move the toggle gate AFTER matchCommittedFiles so the finally block does a proper partial clear of the just-committed files even when the note write is skipped. 0oAg (Copilot): normalizeGitCoAuthor copied value?.commit / value?.pr without type-checking. settings.json is hand-editable; a stored `{ commit: "false" }` reached runtime as a truthy string and behaved as if attribution were enabled. Add a per-field bool coercion that falls back to the schema default (true) for any non-boolean, matching what the dialog and IDE schema already imply. Tests cover the string / number / null cases. 0oAo (Copilot): v3→v4 shouldMigrate only special-cased versionless legacy booleans — versionless files with invalid gitCoAuthor values (`"off"`, `[]`, etc.) skipped the migration and the loader stamped `$version: 4` over the bad value. Runtime normalization then silently re-enabled attribution. Extend shouldMigrate to fire on ANY versionless non-object value at general.gitCoAuthor; the existing migrate() body's drop-and-warn path resets it. Already-object shapes (hand-edited to v4) still skip cleanly. Tests added. 0oAt (Copilot): `.terraform.lock.hcl` got dropped from generated-file exclusion when `.lock` was removed from the blanket extension list in 3c0e3293b. It's a generated provider lockfile in the same class as `package-lock.json` and dominates Terraform-repo commits. Re-add to EXCLUDED_FILENAMES and add a regression test covering both repo-root and module-nested locations. * fix(attribution): harden restoreFromSnapshot against corrupt payloads 1KMY (Copilot): snapshot.surface was copied without type validation. A corrupted/partially-written snapshot with a non-string surface (e.g. {}, 42, null) would later be serialized into the git note as "[object Object]" and used as a Map key downstream, breaking the expected payload shape. Type-check and fall back to the current client surface for any non-string (or empty-string) value. 1KLq (Copilot): per-field sanitiseCount enforced `promptCount >= 0` and `promptCountAtLastCommit >= 0` independently, but never the cross-field invariant. A snapshot with promptCountAtLastCommit > promptCount would surface a negative getPromptsSinceLastCommit() and propagate as a "(-N)-shotted" trailer into PR text. Clamp atLastCommit to total on restore. 1KL_ (Copilot): when a snapshot carried both the symlinked and canonical paths for the same file (a session straddling the canonicalisation fix), `set(realpathOrSelf(k), ...)` overwrote the first entry with the second, silently dropping the AI contribution the first form had accumulated. Merge instead: sum aiContribution and OR aiCreated when collapsing duplicate keys. Tests cover all three branches: non-string surface fallback, promptCount clamp, and duplicate-key merge. * fix(attribution): roll back snapshot dedup key on sync appendRecord failure 1UMh (Copilot): appendRecord can throw synchronously before returning a promise — e.g. when ensureConversationFile() rethrows a non-EEXIST writeFileSync error. The async .catch() handler attached to the promise never runs in that case, so the optimistic dedup-key set sticks on a write that never landed and permanently suppresses identical retries. Roll back lastAttributionSnapshotJson in the outer catch too. Regression test forces writeFileSync to throw EACCES on the first invocation, then asserts the second identical snapshot attempt fires a fresh write rather than getting deduped. * docs(attribution): align cleanup-branch comments with noteCommitWithoutClearing Three doc/test-fixture stale-after-refactor cleanups (Copilot 4MDx / 4MEI / 4MEa): - shell.ts:1944 (around the stagedInfo === null branch): the comment still claimed the finally block "falls back to a full clear", but 1ece87438 switched analysis-failure cleanup to noteCommitWithoutClearing(). Update the comment so the reasoning matches what the code actually does (and so a future reader doesn't reintroduce the wholesale clear thinking it's already there). - shell.ts: getCommittedFileInfo docstring carried the same stale "full clear" claim for the `null` return value. Update to describe the noteCommitWithoutClearing() fallback and the smaller-evil trade-off for the just-committed file. - chatRecordingService.test.ts: baseSnapshot fixture for the recordAttributionSnapshot tests still carried `baselines: {}`, even though that field was removed from AttributionSnapshot in 296fb55ae's dead-code purge. Structural typing let it compile, but the fixture didn't reflect the production shape — drop it. * fix(attribution): restore fire-and-forget appendRecord, route rollback via callback 6OcJ (Copilot): refactor in 715c258fb returned a Promise from appendRecord so the snapshot dedup-key path could chain rollback — but recordUserMessage / recordAssistantTurn / recordAtCommand / recordSlashCommand / rewindRecording all call appendRecord without await or .catch(). A transient jsonl.writeLine rejection on any of those would surface as an unhandled-promise-rejection (warning, or crash on --unhandled-rejections=throw). Restore the original fire-and-forget semantics: appendRecord again returns void and internally swallows async failures (logging via debugLogger). Per-record failure reactions are routed through an optional onError callback — recordAttributionSnapshot uses this to roll back lastAttributionSnapshotJson when the write that set it ends up rejecting. Tests: add a fire-and-forget regression that mocks writeLine to reject and asserts no unhandledRejection events fire while the existing snapshot rollback tests (sync + async) still pass via the new callback path. * fix(attribution): GIT_DIR repo-shift bail, snapshot envelope validation, narrow legacyTypes 80ME (gpt-5.5 /review, [Critical]): tokeniseSegment unconditionally stripped every leading KEY=value token. `GIT_DIR=elsewhere/.git git commit ...` was therefore treated as an in-cwd commit, picked up the Co-authored-by trailer, and produced a per-file note that landed against our cwd's HEAD even though the actual commit went to a different repo. Define a GIT_ENV_SHIFTS_REPO set (GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR, GIT_INDEX_FILE, GIT_NAMESPACE) and have tokeniseSegment refuse to parse any segment whose leading env block (including the env-wrapper's KEY=VALUE block) carries one of these. Identity / date variables (GIT_AUTHOR_*, GIT_COMMITTER_*) are deliberately NOT in the set — they tweak metadata but don't relocate the repo. Tests cover plain prefix, env-wrapped prefix, and a GIT_COMMITTER_DATE positive control that should still get the trailer. 8EeQ (Copilot): restoreFromSnapshot received `snapshot as AttributionSnapshot` from a structural cast off `unknown` (the resume path), so its TS-typed shape was only a hint. A corrupted JSONL line (non-object / array / wrong type discriminator / missing type) would skip past the version check straight into Object.entries(snapshot.fileStates) — and a non-object fileStates (an array, say) seeded fileAttributions with numeric-string keys. Add envelope-level shape gates (isPlainObject + type discriminator) and a fileStates plain-object check before iterating; both bail to a clean reset rather than poisoning the singleton. Tests added. 8Eej (Copilot): SettingDefinition.legacyTypes was typed as SettingsType[] which includes 'enum' and 'object' — JSON Schema's `type` keyword doesn't accept those values. Adding `legacyTypes: ['enum']` would silently produce an invalid settings.schema.json. Narrow the field's type to ReadonlyArray<'boolean' | 'string' | 'number' | 'array'> (the JSON-Schema-primitive subset). Future complex-shape legacy support should land its own branch in convertSettingToJsonSchema. * docs(attribution): correct legacyTypes / EXCLUDED_DIRECTORY_SEGMENTS comments 9Ta_ (Copilot): the JSDoc on legacyTypes claimed JSON Schema's `type` keyword does not accept `'object'` — that's wrong; `'object'` IS a valid JSON Schema type. Reword to reflect the actual rationale: `'enum'` is not a valid JSON Schema `type` value at all (enum constraints use the `enum` keyword), and a bare `{type: 'object'}` would accept any object regardless of what the field's pre-expansion shape actually allowed. The narrowed `boolean | string | number | array` set is exactly what the one-liner generator can faithfully emit; richer legacy shapes belong in their own branch of convertSettingToJsonSchema. 9Tbs (Copilot): the comment in generatedFiles.ts referenced `EXCLUDED_DIRECTORIES`, but the constant is `EXCLUDED_DIRECTORY_SEGMENTS` (renamed during the segment-boundary refactor). Update the reference so a future maintainer scanning for the rule doesn't chase a non-existent identifier. * fix(attribution): SHA-pin git notes, on-disk hash divergence detection, env -C cwd-shift tanzhenxin review #1 — Note targets symbolic HEAD, not captured SHA: buildGitNotesCommand hard-coded 'HEAD' as the target; postHead was captured at commit-detection time but only used for the !== preHead diff. Between that capture and the execFile, three more awaited git calls run — anything that moves HEAD in the same cwd (post-commit hook, chained `commit && tag -m`, parallel process) silently lands the note on the wrong commit because of `-f`. Thread postHead through buildGitNotesCommand as a required `targetCommit` arg. Test asserts the targeted SHA, not the symbolic ref. tanzhenxin review #2 — Accumulator has no baseline: recordEdit was monotonic per-path with no reset for out-of-band mutations. Re-instate FileAttribution.contentHash and: - recordEdit hashes the input `oldContent` and resets the per-file accumulator if it doesn't match what AI's last write recorded (catches paste-replace via external editor, manual save, etc. WHEN AI subsequently edits the same file again). - New validateOnDiskHashes() rehashes every tracked file's CURRENT on-disk content and drops entries whose hash diverged. Called from attachCommitAttribution before matchCommittedFiles so a commit can never credit AI for a human-only diff. Deleted files (readFileSync throws) are left alone — the commit's deletion record is what the note should reflect. tanzhenxin review #4 — Failed-commit / staleness leak: The recordEdit divergence check above + commit-time validateOnDiskHashes together catch tanzhenxin's exact scenario (AI edits a.ts → hook rejects → user manually edits a.ts → user commits → no AI credit because validateOnDiskHashes drops the stale entry). The !commitCreated branch still preserves attributions to keep the submodule case working — the staleness problem is now solved at the next commit's validation step. Self-review item — env -C / --chdir treated as repo-shifting: Added ENV_FLAGS_SHIFT_CWD set covering -C / --chdir. tokeniseSegment returns null for `env -C DIR git commit ...` segments — same contract as a leading GIT_DIR=... assignment. Without this we'd either misidentify /elsewhere as the program (silently dropping attribution) or, worse if -C went into the value-skip set, trailer-inject onto a commit that lands in /elsewhere's repo. Tests added alongside the existing GIT_DIR repo-shift cases. 339 tests pass; typecheck clean. * fix(attribution): pickBool intent-aware, shouldClear gate, ETIMEDOUT surface, drop dead exports -wgA + -wg0 (deepseek): pickBool defaulted non-boolean to true, turning a hand-edited `{ commit: "false" }` into enabled attribution. Replace with intent-aware parsing: "true"/"yes"/"on"/ "1" → true, "false"/"no"/"off"/"0"/"" → false, anything else (unknown strings, non-1 numbers, objects, arrays, null) → false. Genuinely-absent sub-fields still default to true (schema default). Migration test scenarios covered. Tests now cover ~17 input cases across both string/number/null/object/unknown forms. -wgq (deepseek): when buildGitNotesCommand returned null (oversized payload) or git notes itself failed, the finally block called clearAttributedFiles(committedAbsolutePaths) — irreversibly deleting per-file attribution data the user might need to amend & retry. Introduce a separate `shouldClear` set that's only assigned on successful note write OR explicit toggle-off. Failure paths (oversized, exitCode != 0, exception, analysis failure) leave shouldClear null so the finally block calls noteCommitWithoutClearing instead — preserving per-file state for the user's recovery. 9p7W (Copilot): execFile callback coerced ETIMEDOUT / SIGTERM (timeout) into a generic exitCode=1 warning. Detect both `error.code === 'ETIMEDOUT'` and `error.killed === true && error.signal === 'SIGTERM'` so the user-visible warning correctly names "timed out after 5s" instead of "exited 1". -wg7 (deepseek): formatAttributionSummary and getAttributionNotesRef were exported but had zero production callers (only tests). Remove the dead exports + their tests (~40 LOC). If/when a logging surface needs them, they can be re-introduced. -wgb (deepseek): tokeniseSegment doesn't recursively unwrap `bash -c '...'` / `sh -c` / `zsh -c`, so addCoAuthorToGitCommit won't splice the trailer into a wrapped command. The background refusal AND the post-commit note path DO catch the wrapped commit because stripShellWrapper at the top of execute peels the wrapper before gitCommitContext / getGitHead run — so the worst-case ("background bash -c 'git commit' bypasses the guard") doesn't materialize. The remaining gap (no Co-authored-by trailer for bash -c-wrapped commits) requires recursively splicing into the inner script with proper bash single-quote re-quoting; significant enough that it's worth its own PR. Documented as a partial-coverage limitation. 3…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
improve Qwen API key configuration instructions, including the use of environment variables and .env files.
TLDR
Dive Deeper
Reviewer Test Plan
Testing Matrix
Linked issues / bugs