mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-21 18:46:47 +00:00
13 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
易良
|
1936420dcb
|
ci(e2e): stabilize MCP/CLI flows and cancel stale main runs (#4039)
* test(e2e): stabilize MCP tool message flow * ci(e2e): cancel stale main E2E runs * test(e2e): accept paired MCP tool results * test(e2e): stabilize monitor tool check * test(e2e): stabilize run_shell_command file-listing assertion The model consistently picks list_directory over run_shell_command for file-listing prompts. Make the prompt explicit about which tool to use, matching the approach taken for the MCP tool flow test. |
||
tanzhenxin
|
d6fe59a3b5
|
fix(test): repair stale --json-schema integration assertion (#4075)
The "fails fast at CLI parse time on invalid JSON Schema" integration
test stopped exercising the Ajv strict-compile path once the
`--json-schema` root-accepts-object precheck landed. The precheck rejects
`{type: "this-is-not-a-real-type"}` before Ajv runs, so the CLI exits
with the "root must accept object-typed values" error instead of the
"is not a valid JSON Schema" error the test expects.
Move the bogus `type` into a property so the root precheck passes and
Ajv catches the unknown type, restoring the test's original intent.
|
||
|
易良
|
04729d646c
|
test: stabilize main e2e flakes (#3992)
* test: stabilize main e2e flakes * test: stabilize macos e2e assertions |
||
Shaojin Wen
|
ecc6828948
|
feat(tools): add ToolSearch for on-demand loading of deferred tool schemas (#3589)
* feat(tools): add ToolSearch for on-demand loading of deferred tool schemas Large MCP deployments push the function-declaration list past 15K tokens per request. This change lets tools opt out of the initial declaration list via `shouldDefer`, and adds a new `ToolSearch` tool the model calls to fetch schemas on demand — either by exact name (`select:Name1,Name2`) or keyword search with name/description/searchHint scoring. - `DeclarativeTool` gains `shouldDefer`, `alwaysLoad`, `searchHint` opts. - MCP tools default to `shouldDefer=true`; lsp, cron_*, ask_user_question, and exit_plan_mode are flagged too. - `ToolRegistry.getFunctionDeclarations()` filters deferred tools by default; `revealDeferredTool()` re-includes them after ToolSearch loads their schemas. - `getCoreSystemPrompt` appends a "Deferred Tools" list (names + first line of description) so the model knows what's reachable. - Subagent wildcard inheritance keeps including deferred tools so existing `tools: ['*']` configs still see MCP schemas. - Resume-session support: `startChat` scans history for prior calls to deferred tools and re-reveals them so the API doesn't reject follow-up calls. `resetChat` clears the revealed set for a clean slate. - Skipped when ToolSearch is filtered out by the permission manager. * feat(cli): add --json-schema for structured output in headless mode Registers a synthetic `structured_output` tool whose parameter schema IS the user-supplied JSON Schema. In headless mode (`qwen -p`), the first successful call terminates the session and exposes the validated payload via the result message's `structured_result` field. Invalid schemas are rejected at CLI parse time via a new strict Ajv compile helper so they can't silently no-op at runtime. * fix(tools): tighten ToolSearch schema + match invocation signature Resolves 2 #3589 review threads: - `max_results` schema: declared as unconstrained `number` but the implementation clamps to the integer range [1, 20]. Updated to `type: 'integer'` with `minimum: 1`, `maximum: HARD_MAX_RESULTS`, `default: DEFAULT_MAX_RESULTS` so the model gets accurate contract guidance and out-of-range values fail validation early instead of silently being clamped after a wasted call. - `execute()` signature now takes `_signal: AbortSignal` to match the base `ToolInvocation.execute` contract. The signal is unused today (the search is sync), but matching the shared signature avoids accidental divergence and makes future cancellation wiring trivial. Test: existing `enforces max_results cap` split into: - schema-rejection (`max_results: 100` → throws at build time) - clamp-on-in-range (`max_results: 20` capped on the candidate side) 21/21 tool-search.test.ts pass; tsc + ESLint clean. * fix(tools,cli): surface ToolSearch reveal failures + dedupe revealed tools Closes 3 #3589 review threads: - Critical: `setTools()` failure during reveal was silently swallowed via `debugLogger.warn` (off in production). Schemas appeared in `llmContent` so the model thought the tools were callable, but the chat's declaration list never updated — the next call surfaced as an "unknown tool" API error, leaving the session in an unrecoverable degraded state. Now returns a proper `ToolResult.error` with the concrete failure reason and instructions to retry; schemas are withheld from `llmContent` so the model doesn't act on a non-ready tool. - Critical: `collectCandidates` returned every deferred tool that matched `shouldDefer && !alwaysLoad` regardless of whether ToolSearch had already revealed it earlier in the session. Already-revealed tools are in the model's declaration list, so re-surfacing them in later keyword searches wasted tokens and risked the model retrying a load it already had. Filter now also skips tools where `registry.isDeferredToolRevealed(name) === true`. `select:<name>` mode is unaffected (the model may legitimately want to re-inspect the schema of a loaded tool). - Suggestion: `--json-schema` plain-text terminal path set `process.exitCode = 1` and emitted `isError: true` to the JSON adapter, but TEXT-mode users only saw a silent exit-code-1 with no visible context (`emitResult` is a no-op for the TEXT-mode error case). Echo the full `'Model produced plain text instead of calling the structured_output tool as required by --json-schema.'` line to stderr so headless runs are debuggable without scraping `--output-format json`. Tests: 2 new in `tool-search.test.ts`: - `keyword search excludes already-revealed deferred tools`: pins the dedupe behavior across two consecutive searches. - `returns an error result when setTools() throws`: pins that failures don't expose schemas as "ready" and the agent gets the underlying message in `error.message`. 23/23 tool-search.test.ts pass; tsc + ESLint clean. DEFERRED to follow-up PRs (replied on threads): - Critical: structured_output + side-effect-tool race in same turn — needs a pre-scan + synthesized "skipped" tool_results, design overlaps with #3598 PR-2's existing skippedOutput pattern. - Suggestion: `+` prefix parsing edge cases (C++, `+ slack`). - Suggestion: `instanceof DiscoveredMCPTool` hard couple — needs a type tag on AnyDeclarativeTool, broader API surface change. - Suggestion: SyntheticOutputTool registered in interactive mode. - Suggestion: resume scan O(history × parts) early-exit. - Suggestion: deferredToolsSection cap. * fix(cli): honor process.exitCode in headless main exit The two non-interactive exit paths in `main()` hardcoded `process.exit(0)` after `runNonInteractive` / `runNonInteractiveStreamJson` returned. This silently overwrote any `process.exitCode = 1` set inside the run — most visibly the `--json-schema` plain-text contract: the JSON adapter emits `isError: true` and stderr gets the explanation, but the shell saw exit code 0 and assumed success. Replace the hardcoded 0 with `process.exit(process.exitCode ?? 0)` on both paths so non-zero exits propagate. The success case is unchanged (exitCode is undefined → exits 0). * test(cli): add integration tests for --json-schema and ToolSearch Closes review-flagged coverage gaps for #3589: `json-schema.test.ts` (6 cases) covers the headless structured-output contract end-to-end: - structured_result emits when the model fills the schema (success path) - @path/to/schema.json file-load works - parse-time validation rejects invalid JSON, invalid JSON Schema, and missing files (no LLM, fast) - plain-text path: when structured_output is not callable (`--exclude-tools structured_output`), the run exits 1 with `is_error: true` and the contract error message — locks in the exit-code fix from the prior commit. `tool-search.test.ts` (3 cases) covers the deferred-tool flow: - select:<name> reveals a tool and the model can invoke it in the same turn (asserts call order so a missed reveal would surface as an unknown-tool API error instead of a silent pass) - keyword query (no select: prefix) hits the tool_search tool - feature-flag-off: with experimental.cron disabled, cron tools are never registered and never appear in tool calls LLM-dependent tests use the cron tools as a deterministic deferred target (gated by experimental.cron, no MCP server required). * fix(cli,core): tighten --json-schema validation Closes 3 #3589 review threads: - Schemas like `{"type":"string"}` and `{"type":"array"}` compiled fine (they're valid JSON Schemas in isolation), but the `--json-schema` value becomes the synthetic structured_output tool's parameter schema and tool-call arguments are object-shaped. Reject any non-undefined top-level type that is not "object" so the user sees the contract violation at parse time, not as an unrecoverable runtime mismatch. - `SchemaValidator.compileStrict` accepted arrays since `typeof [] === 'object'` — Ajv would later emit a confusing error. Add an explicit `Array.isArray` guard so the contract stated by the function name is honored at the boundary. - `compileStrict` shared the project-wide Ajv instances configured with `strictSchema: false` (intentionally lenient so MCP servers can ship custom keywords without breaking runtime validation). That leniency is wrong for the `--json-schema` surface — typos like `propertees` were silently ignored. Compile inside a dedicated `strict: true` Ajv so user-supplied schemas surface mistakes immediately. Tests: - jsonSchemaArg: rejects non-object top-level type ("string"/"array"). - schemaValidator.compileStrict: rejects arrays; flags unknown keywords (typos) under strict mode. * fix(tools): roll back ToolSearch reveals when setTools() throws Closes 1 #3589 review thread. `loadAndReturnSchemas` revealed each requested tool BEFORE calling `setTools()` because `getFunctionDeclarations()` filters by the revealedDeferred set — the reveal has to be in place when setTools() rebuilds the chat's declaration list. But if setTools() throws (e.g. chat not yet initialised), the registry was left holding orphaned reveals: the tool was marked "revealed" while the API never received its schema. Subsequent keyword searches would then exclude that tool from candidates (per `collectCandidates`'s isDeferredToolRevealed filter), making it unreachable until `/clear`. Track the names this call NEWLY revealed (skipping tools that were already revealed by an earlier ToolSearch in the same session) and unreveal them on setTools() failure. Added `unrevealDeferredTool` to the registry as the one-tool inverse of `revealDeferredTool`; `clearRevealedDeferredTools` is unchanged and still wipes the whole set on `/clear`. Test: extends the existing `setTools() throws` test to also assert that (a) the failed call's reveal is rolled back and (b) a tool revealed by an earlier call stays revealed (not whole-set wiped). * test(cli): unit-cover --json-schema runtime branches Closes one of the test-coverage gaps in #3589 reviews (gpt-5.5 review S8). Adds two deterministic L1 unit tests in nonInteractiveCli.test.ts that mock the LLM at sendMessageStream — no model API hit, no flake, ~10ms total. 1. structured_output success path: model fires the synthetic tool once, runtime sets structuredSubmission, aborts background tasks, and emitResult fires exactly once with `structuredResult` matching the model's args. No follow-up turn is issued (single-shot contract). 2. plain-text error path under --json-schema: model emits text only; runtime sets process.exitCode=1, writes the contract-violation line to stderr, and emits an isError result with the canonical "Model produced plain text" message. Both tests inject a stub adapter via runNonInteractive's `options.adapter` hook, so they assert against direct emitResult calls instead of parsing JSON stdout. process.exitCode is snapshot/restored to keep the test hermetic. The L2 integration tests in integration-tests/cli/json-schema.test.ts remain as smoke coverage against a real model. * fix(cli,core): support type-union arrays in --json-schema Resolves 2 regressions introduced by the previous schema-hardening commit ( |
||
|
tanzhenxin
|
78ad595581
|
feat(core): support QWEN_HOME env var to customize config directory (#2953)
* feat(core): support QWEN_CONFIG_DIR env var to customize config directory Allow users to override the default ~/.qwen config directory location via the QWEN_CONFIG_DIR environment variable. This enables users on dev machines with external disk mounts or custom home directory layouts to persist config at a location of their choosing. Changes: - Add QWEN_CONFIG_DIR check to Storage.getGlobalQwenDir() (absolute and relative path support) - Eliminate 11 redundant '.qwen' constant definitions across packages - Replace 16+ direct os.homedir() + '.qwen' path constructions with Storage.getGlobalQwenDir() calls - Inline env var checks for packages that cannot import from core (channels, vscode-ide-companion, standalone scripts) - Add unit tests for the new env var behavior - Project-level .qwen/ directories are NOT affected Closes #2951 * fix(core): use path.resolve/join in QWEN_CONFIG_DIR tests for Windows compat Hardcoded Unix paths like '/tmp/custom-qwen/settings.json' fail on Windows where path APIs produce backslash separators. Use path.resolve() for inputs and path.join() for assertions so the tests pass cross-platform. * test(cli): remove flaky 'should keep restart prompt when switching scopes' test Timing-sensitive UI test that fails intermittently on Windows CI due to async ANSI output not settling within the wait window. * feat(core): route remaining hardcoded ~/.qwen/ paths through Storage.getGlobalQwenDir() Update channel status, memory command, extension storage, skills discovery, and memory discovery to use Storage.getGlobalQwenDir() instead of hardcoded os.homedir()/.qwen paths, ensuring QWEN_CONFIG_DIR env var is respected throughout the codebase. * fix(tests): mock os.homedir before makeFakeConfig for Storage.getGlobalQwenDir Storage.getGlobalQwenDir() is now called during Config construction, which requires os.homedir() to be mocked before makeFakeConfig() is called. Also mock Storage.getGlobalQwenDir in memoryCommand tests since it uses a cross-package import that vi.spyOn doesn't intercept. * fix(core): respect QWEN_CONFIG_DIR for .env discovery and install source findEnvFile() walk-up would find legacy ~/.qwen/.env before checking QWEN_CONFIG_DIR/.env when the workspace was under $HOME. Skip the legacy path when a custom config dir is set so the fallback picks up the correct file. Also add a legacy fallback in readSourceInfo() since the installer always writes source.json to ~/.qwen/ regardless of QWEN_CONFIG_DIR. * refactor(core): rename QWEN_CONFIG_DIR to QWEN_HOME and fix runtime path resolution Rename the env var before it ships (zero existing users) to match the convention of CARGO_HOME, GRADLE_USER_HOME, etc. — "HOME" means "root of all tool state", not just config. Key changes: - Rename QWEN_CONFIG_DIR → QWEN_HOME across all packages and scripts - Add shared path utils in vscode-ide-companion and channels/base to eliminate scattered inline env var resolution - Fix runtime path mismatch: IDE lock files and session paths in the vscode extension now route through getRuntimeBaseDir() (checking QWEN_RUNTIME_DIR first), matching core Storage behavior - Fix telemetry_utils.js otel path to check QWEN_RUNTIME_DIR for tmp/ - Add E2E integration tests for QWEN_HOME scenarios * fix(core): address critical review issues for QWEN_HOME support Pass resolved QWEN_HOME as a dedicated QWEN_DIR sandbox parameter so macOS Seatbelt profiles allow writes to custom config directories. Fix hookRunner treating signal-killed hooks as success by using ?? -1 instead of || 0. Add QWEN_HOME and QWEN_RUNTIME_DIR to the env vars documentation table. * fix(sandbox): whitelist QWEN_RUNTIME_DIR in macOS Seatbelt profiles When QWEN_RUNTIME_DIR is set separately from QWEN_HOME, the sandbox was blocking writes to the runtime directory (debug logs, chat history, IDE locks, sessions). Pass RUNTIME_DIR as a sandbox parameter and add the corresponding subpath rule to all six .sb profiles. * fix(core): add tilde expansion to QWEN_HOME and align satellite path helpers - Extract resolvePath() from resolveRuntimeBaseDir() so QWEN_HOME gets the same ~/tilde expansion that QWEN_RUNTIME_DIR already had. - Port resolvePath() to vscode-ide-companion and channels/base mirrors, fixing tilde handling in getRuntimeBaseDir() for the IDE companion. - Add missing os.tmpdir() fallback in channels/base getGlobalQwenDir(). - Add unit tests for tilde expansion in QWEN_HOME. - Clarify prompts.ts comment that system.md default is global, not project-level. * fix(core): add tilde expansion to scripts and fix extension cache QWEN_HOME support Add resolvePath() helper to standalone JS scripts (sandbox_command.js, telemetry.js, telemetry_utils.js) so QWEN_HOME=~/custom expands consistently with core Storage.resolvePath(). Fix ExtensionManager.refreshCache() to use ExtensionStorage.getUserExtensionsDir() instead of hardcoded os.homedir(), so extensions installed under a custom QWEN_HOME are discoverable. * test: remove flaky InputPrompt tab-suggestion test on Windows * test: remove flaky tests that fail intermittently on Windows Removes 'does not accept the prompt suggestion on shift+tab' from InputPrompt.test.tsx and 'should keep restart prompt when switching scopes' from SettingsDialog.test.tsx. Both have been observed to fail intermittently on the Windows CI workers; the underlying behaviors are covered by adjacent assertions and end-to-end tests. * revert(core): keep system.md path project-local under .qwen/ The QWEN_HOME refactor incorrectly routed the QWEN_SYSTEM_MD default path through Storage.getGlobalQwenDir() (i.e. ~/.qwen/system.md or $QWEN_HOME/system.md). The original semantics — inherited from the upstream Gemini-CLI sync — are project-local: <cwd>/.qwen/system.md. System-prompt customization is intentionally per-project so that each repository can ship its own override without global side effects. Users who want a global override can still set QWEN_SYSTEM_MD to an absolute path. This revert keeps that behavior intact while leaving the rest of the QWEN_HOME plumbing (settings, credentials, extensions, skills, memory) unchanged. * refactor(core): unify QWEN_CONFIG_DIR into the canonical QWEN_DIR Three definitions of the literal '.qwen' string existed across the codebase: - QWEN_DIR in config/storage.ts (canonical, used by the Storage class) - QWEN_CONFIG_DIR in memory/const.ts - QWEN_CONFIG_DIR in tools/memory-config.ts (a near-clone of the above) The QWEN_CONFIG_DIR name also collided with a former env-var name (now renamed to QWEN_HOME on this branch), making it ambiguous whether call sites referred to a configurable env var or a hardcoded directory name. Drop the duplicates and route the only call sites (prompts.ts and its test) through QWEN_DIR from config/storage.ts. The mock factory in config.test.ts is updated to no longer expose the removed export. * fix(integration-tests): use 'extensions list' to trigger settings migration Tests 2b and 3a in cli/qwen-config-dir.test.ts relied on running \`qwen --help\` to invoke loadSettings() (and thus the V1→V3 settings migration). That worked when loadSettings() ran before parseArguments() in the CLI startup sequence. Main has since flipped the order: parseArguments() runs first, and yargs intercepts --help and exits the process before loadSettings() is reached, so migration never runs and the tests' migration probe always reads back V1. Switch to \`qwen extensions list\` instead. It is a yargs subcommand that runs through main() to loadSettings() without requiring an API key, so migration runs as expected. Update the inline comments to document why --help cannot be used and why this command works. * fix(memory): route auto-memory base dir through Storage.getGlobalQwenDir() The auto-memory subsystem (introduced on main in #3087) computed its base directory by hardcoding path.join(os.homedir(), QWEN_DIR). That bypassed QWEN_HOME entirely, so global auto-memory artifacts always landed in ~/.qwen/projects/... regardless of the user's configured QWEN_HOME path. Route the default through Storage.getGlobalQwenDir() so QWEN_HOME is honored. The QWEN_CODE_MEMORY_BASE_DIR test override stays as the highest-priority short-circuit. Discovered while running the QWEN_HOME e2e test plan against the merged branch — Group B test B3 (memory tool writes to QWEN_HOME) was the only failing scenario across A/B/C/D groups. * fix(cli): treat custom QWEN_HOME .env as user-level When QWEN_HOME points to a directory whose path does not contain `.qwen` (e.g., `/tmp/qwen-home`), the global `.env` was misclassified as a project-level env file. As a result, default-excluded variables such as `DEBUG` and `DEBUG_MODE` were silently dropped even though they came from the user-level config directory. The classification now reuses the same user-level path set computed by `findEnvFile`, so any `.env` inside the resolved global Qwen directory (or directly under `~/`) is recognized as user-level. Also drop the misleading "does not expand `~`" note from the QWEN_HOME documentation — `Storage.getGlobalQwenDir` does expand leading tildes via `Storage.resolvePath`. * fix(cli): drop legacy .qwen substring check from env-file classification The user-level env-file detection now keys solely off the precomputed user-level path set, which already covers ~/.env and ${QWEN_HOME}/.env. The legacy substring fallback misclassified <repo>/.qwen/.env as user-level, so excludedEnvVars no longer applied to it. * fix(core): align plain-text hook output with documented exit-code semantics Per docs/users/features/hooks.md, only exit code 2 is a blocking error; all other non-zero exit codes are non-blocking and execution should continue. The plain-text branch in convertPlainTextToHookOutput previously denied on every non-zero, non-1 exit code (3, 127, signal fallbacks), contradicting the documented behavior. Collapse all non-blocking non-zero codes to EXIT_CODE_NON_BLOCKING_ERROR before passing into the converter so they take the warning path consistently. * chore: trigger CI * fix(cli): pass QWEN_HOME and QWEN_RUNTIME_DIR into docker/podman sandbox The container CLI previously had no awareness of the host's QWEN_HOME or QWEN_RUNTIME_DIR values. The global qwen dir worked only because the mount target happens to match the default fallback inside the sandbox, and the runtime base dir was lost entirely when it diverged from the global qwen dir. * fix(cli): canonicalize sandbox QWEN/RUNTIME paths and pin IDE lock dir Two reviewer-flagged issues from PR #2953: * macOS Seatbelt was passed `path.resolve` for `QWEN_DIR`/`RUNTIME_DIR` while neighbouring directories used `fs.realpathSync`. With a symlinked `QWEN_HOME` or `QWEN_RUNTIME_DIR`, sandbox-exec would compare against the canonical kernel path and deny writes. Create the dirs (so `realpathSync` can succeed on first run) then canonicalize them like the surrounding entries. * The VS Code companion wrote IDE lock files via the runtime base dir while the CLI side resolves the runtime dir from settings too. That divergence silently desynced lock-file discovery whenever a user set `advanced.runtimeOutputDir` without `QWEN_RUNTIME_DIR`. Anchor both sides to `getGlobalQwenDir()` since the companion process can only see env vars, not CLI settings. * fix(cli): finish QWEN_HOME plumbing across env, memory, rules, sandbox Codex review surfaced four user-visible spots where QWEN_HOME wasn't threaded through: * `findEnvFile` walked through the user home dir before consulting the QWEN_HOME fallback, so `~/.env` shadowed `<QWEN_HOME>/.env` and reversed the qwen-specific precedence the default `~/.qwen/.env` path enjoys. Add a home-dir-step check that prefers the custom Qwen dir when set. * `MemoryDialog` displayed and edited `~/.qwen/QWEN.md` regardless of QWEN_HOME. Memory discovery already routes through Storage, so user edits via the dialog were silently ignored at runtime. Route the dialog through `Storage.getGlobalQwenDir()` to match. * `loadRules` looked up global rules at `~/.qwen/rules/`, ignoring QWEN_HOME entirely. Use the global Qwen dir like the rest of the config surfaces. * The Docker/Podman sandbox path called `mkdirSync(userSettingsDir)` without `recursive`. Pre-PR the dir was always `~/.qwen` and the parent existed; with a nested QWEN_HOME like `/tmp/qwen/config` the first run threw ENOENT before the mount could be added. * fix(cli): block project .env from redirecting QWEN_HOME and QWEN_RUNTIME_DIR A project `.env` could set QWEN_HOME after settings were already loaded from the real home, splitting global state: settings.json read from ~/.qwen but later writes (installation_id, OAuth credentials, MCP tokens) landed in the project-controlled directory. The user-configurable excludedEnvVars list isn't the right place for this — it's a correctness boundary, not a preference — so always exclude these two vars from project .env files. User-level .env files (~/.qwen/.env) are unaffected. * fix(cli): keep workspace .qwen/.env unfiltered and pre-resolve user QWEN_HOME The env-file classification conflated two concerns: which paths may override global state vars, and which paths are exempt from the user-configurable excludedEnvVars filter. Splitting them lets a workspace `<repo>/.qwen/.env` carry DEBUG/DEBUG_MODE per the documented contract while still being blocked from redirecting QWEN_HOME or QWEN_RUNTIME_DIR. A QWEN_HOME set in `~/.qwen/.env` or `~/.env` would also previously arrive too late: USER_SETTINGS_PATH was captured at module load and loadSettings migrated `~/.qwen/settings.json` before loadEnvironment applied the override, leaving credentials, MCP tokens, and installation_id pointed at the new directory while settings stayed at the legacy one. A pre-pass now reads those user-level files for the two storage-controlling vars before any user settings are loaded, and the user settings path is re-resolved locally so all global state lands in one place. * fix(cli): make user-settings paths lazy to pick up bootstrapped QWEN_HOME USER_SETTINGS_PATH/USER_SETTINGS_DIR in settings.ts and the duplicate USER_SETTINGS_DIR in trustedFolders.ts were top-level consts evaluated at module load — before preResolveHomeEnvOverrides() reads QWEN_HOME from ~/.env or ~/.qwen/.env. Callers (sandbox launcher, trusted-folders reader) saw the legacy ~/.qwen path while the main CLI had moved to the custom home, splitting state. Convert all three to lazy getter functions and add a regression test that pokes process.env.QWEN_HOME after import and asserts each getter reflects it — any future top-level capture turns the test red. Mirror the same ~/.env / ~/.qwen/.env bootstrap into scripts/sandbox_command.js, which previously only read process.env directly and could disagree with the main CLI on the sandbox setting. Addresses review threads #3159793469, #3177804507, and item #2 of the 2026-05-06 review summary. * fix(cli): address qwen home review follow-ups * test(cli): normalize path in QWEN_HOME freshness assertion for Windows `getUserSettingsDir()` returns `path.dirname(...)`, which on Windows uses backslash separators. The bare string comparison failed on Windows runners ("\tmp\qwen-lazy-test" vs "/tmp/qwen-lazy-test"). Wrap the expected value in `path.normalize()` to match the OS-native separator, mirroring the two sibling assertions that already use `path.join()`. * fix(cli): close storage-routing leaks via settings.env and project sandbox .env settings.env (merged) was being applied to process.env without filtering, so a workspace settings.json could redirect global state by setting env.QWEN_HOME or env.QWEN_RUNTIME_DIR after the home-scoped .env bootstrap ran. Apply PROJECT_ENV_HARDCODED_EXCLUSIONS to the settings.env path too. scripts/sandbox_command.js's project-walk fallback called dotenv.config() to find QWEN_SANDBOX, which injected every parsed key — including QWEN_HOME / QWEN_RUNTIME_DIR the main CLI hard-blocks. Replace with a manual parse that copies only QWEN_SANDBOX. Add a startup migration warning when QWEN_HOME points to a directory with no settings.json while ~/.qwen/settings.json exists, so users notice that their existing OAuth tokens / settings / memory aren't auto-migrated. * test: cover QWEN_HOME / QWEN_RUNTIME_DIR in duplicated path helpers Adds targeted unit tests for the two TypeScript mirrors of Storage.getGlobalQwenDir() / getRuntimeBaseDir() that live outside packages/core to avoid cross-package imports. Covers default, absolute, relative, ~/x, ~\x, and bare ~ inputs, plus the runtime/home priority chain in the IDE companion. * fix: bootstrap QWEN_HOME before yargs handlers and in VS Code companion Two storage-routing leaks surfaced by Codex review of feat/qwen-config-dir: - channel status/stop call readServiceInfo() inside yargs handlers that process.exit before loadSettings() runs, so QWEN_HOME defined only in ~/.qwen/.env or ~/.env never resolved for them. The same race exists for the duplicate-instance check at the top of channel start. Hoist preResolveHomeEnvOverrides() to the top of main() so all subcommand handlers see the bootstrapped env vars. - The VS Code companion's getGlobalQwenDir / getRuntimeBaseDir read process.env directly, missing the same .env pre-pass. If a user only configures QWEN_HOME via ~/.qwen/.env, the CLI looks under the redirected dir while the companion writes IDE lock files under ~/.qwen, breaking IDE discovery. Mirror the CLI pre-pass in the companion (lazy, idempotent) without importing from core. * fix(config): preserve credentials in legacy ~/.qwen/.env when QWEN_HOME redirects When QWEN_HOME is bootstrapped from `~/.qwen/.env`, the home-dir env walk previously skipped that file and never read `<QWEN_HOME>/.env` from the companion. This stranded non-routing credentials (e.g. OPENAI_API_KEY) left in `~/.qwen/.env` and let the companion write IDE lock files into a different runtime dir than the CLI was reading from. - CLI: fall back to `~/.qwen/.env` after `<QWEN_HOME>/.env` at both the home-dir step and the post-walk fallback in findEnvFile, and treat the legacy path as user-level for trust and exclusion semantics. - Companion: after the initial candidate pass discovers QWEN_HOME, also read `<QWEN_HOME>/.env` so QWEN_RUNTIME_DIR sourced from there matches what the CLI's findEnvFile would pick. * refactor(cli): simplify QWEN_HOME plumbing — dedupe helpers, latch, comments - replace local isSameOrChildPath with core's isSubpath in sandbox.ts - latch preResolveHomeEnvOverrides so it runs once per process - pass userLevelPaths from loadEnvironment into findEnvFile (no recompute) - collapse findEnvFile's home-dir branch and post-loop fallback into one shared candidate list (drops duplicate existsSync calls) - factor extensionManager's user-extensions loop into a private helper - use QWEN_DIR constant instead of '.qwen' literal in skill-manager - trim narrative / PR-history comments across changed files * fix(cli): align QWEN_HOME .env bootstrap across CLI, sandbox, telemetry Telemetry scripts previously read process.env.QWEN_HOME directly, so a QWEN_HOME set only in ~/.env or ~/.qwen/.env left telemetry writing to ~/.qwen while the CLI routed elsewhere. Extract the bootstrap into scripts/lib/qwen-home-bootstrap.js and have sandbox_command.js, telemetry.js, and telemetry_utils.js share it. Also add a third-pass <new QWEN_HOME>/.env read in preResolveHomeEnvOverrides so the CLI and VS Code companion agree on QWEN_RUNTIME_DIR when it is configured under the new home dir. * test(integration-tests): update QWEN_HOME assertions for v4 schema Settings schema was bumped to v4 on main (gitCoAuthor migration). The qwen-config-dir tests still asserted post-migration $version === 3, so they failed after the merge. Bump the assertions to 4 and the seed in 3a to match, and point a comment at SETTINGS_VERSION so the next bump is easy to find. |
||
|
Shaojin Wen
|
cfbcea1e88
|
feat: add commit attribution with per-file AI contribution tracking (#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
|
||
jinye
|
df594f75fe
|
feat(core): event monitor tool with throttled stdout streaming (Phase C) (#3684)
* feat(core): event monitor tool with throttled stdout streaming (Phase C) Add a new Monitor tool that spawns a long-running shell command and streams its stdout lines back to the agent as event notifications. This is Phase C from the background task management roadmap (#3634, #3666). What changes: - New MonitorRegistry (services/monitorRegistry.ts): per-monitor entry with lifecycle (running/completed/failed/cancelled), idle timeout auto-stop, max events auto-stop, AbortController-based cancellation. Follows the same structural pattern as BackgroundTaskRegistry. - New Monitor tool (tools/monitor.ts): spawns via child_process.spawn with independent AbortController (Ctrl+C won't kill monitors), separate stdout/stderr line buffers, token-bucket throttling (burst=5, sustain=1/s). Returns immediately with monitor ID; events stream as notifications. - Sleep interception in shell.ts: detectBlockedSleepPattern() blocks foreground `sleep N` (N>=2) and guides model to use Monitor or is_background instead. - Config integration: MonitorRegistry instantiation, accessor, shutdown cleanup (abortAll), lazy tool registration. - CLI wiring: notification callbacks in useGeminiStream.ts (interactive) and nonInteractiveCli.ts (headless), including hold-back loop abort on exit and SIGINT cleanup. What this PR doesn't do (gated on #3471/#3488): - Footer pill / dialog integration - task_stop / send_message integration Test plan: - 21 MonitorRegistry unit tests (lifecycle, idle timeout, max events, XML escaping, nonexistent ID guard, callback clearing) - 20 Monitor tool unit tests (validation, spawn, line buffering, separate stdout/stderr buffers, throttling, signal-killed path, turn isolation) - 7 detectBlockedSleepPattern unit tests - 2 E2E tests (monitor invocation, sleep interception) - Full core suite: 248 files / 6151 passed Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): hold-back loop waits for monitors + emit task_started for SDK Two fixes from Codex review: P1: The non-interactive hold-back loop now includes monitorRegistry.getRunning() in its wait condition, so monitors can stream events before the CLI exits. Previously monitors were aborted immediately after the agent's first reply. P2: MonitorRegistry gains setRegisterCallback(), and nonInteractiveCli wires it to emit task_started system messages. Stream-json/SDK consumers now see a task_started for each monitor, matching the backgroundTaskRegistry contract. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): Windows process kill + pipeline sleep false-positive Two fixes from Codex review: P1: Monitor abort handler now uses `taskkill /f /t` on Windows instead of POSIX-only `process.kill(-pid)`. Follows the existing pattern in ShellExecutionService.childProcessFallback. P2: detectBlockedSleepPattern no longer uses splitCommands (which splits on `|` pipes). Replaced with a regex that only matches sleep followed by sequential separators (&&, ||, ;, &, newline), not pipes. `sleep 5 | cat` is now correctly allowed. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(test): resolve TS errors in monitor.test.ts mock types Use Object.defineProperty for readonly ChildProcess.pid and proper Readable type for stdout/stderr mocks to satisfy strict tsc builds. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): remove false notification promise + add early-abort guard P1: Sleep interception guidance no longer promises "completion notification" for is_background — that wiring doesn't exist yet (follow-up from #3642). P2: Monitor.execute() now checks _signal.aborted before spawning, preventing a race where cancellation during tool scheduling still launches a monitor. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(test): add getMonitorRegistry mock to useGeminiStream tests The useGeminiStream hook now calls config.getMonitorRegistry() to wire up monitor notification callbacks. The test mock config was missing this method, causing 64 test failures with "config.getMonitorRegistry is not a function". Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(test): add getMonitorRegistry mock to nonInteractiveCli tests Same fix as useGeminiStream.test.tsx — the mock config needs getMonitorRegistry to avoid "is not a function" errors (29 failures). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): address PR review — CORE_TOOLS, directory param, test fix 1. Add 'monitor' to PermissionManager.CORE_TOOLS so coreTools allowlist correctly gates the monitor tool (same as run_shell_command). 2. Add optional 'directory' parameter to MonitorTool with workspace validation, mirroring ShellTool's directory support for multi-root workspaces. 3. Fix sleep-interception E2E test: readToolLogs() doesn't expose toolResult, so the old assertion was dead code. Now verifies via the model's output text instead. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): address MonitorTool review #4186888042 Addresses three [Critical] review comments on packages/core/src/tools/monitor.ts: 1. Partial-line buffer unbounded growth (processLines) MAX_LINE_LENGTH was only enforced after a newline, so a command emitting a long stream without newlines would grow buffer.value without bound and re-split the entire accumulated string on every chunk. Now, when the buffer has no newline and exceeds MAX_LINE_LENGTH, we force-emit a single truncated event through the throttled path and reset the buffer. 2. Missing type guard on params.command validateToolParamValues called params.command.trim() without a typeof check. Schema validation normally catches this, but SDK/direct callers could bypass it and hit an uncaught TypeError. Added typeof === 'string' guard, matching the pattern used for max_events / idle_timeout_ms. 3. Workspace check bypass via raw startsWith The directory validator used workspaceDirs.some(d => params.directory .startsWith(d)), which allowed prefix collisions (e.g. /tmp/project-evil against a /tmp/project workspace) and skipped canonicalisation / symlink resolution. Switched to WorkspaceContext.isPathWithinWorkspace, which already does fullyResolvedPath + segment-aware isPathWithinRoot matching and is the standard used elsewhere in the codebase. Test coverage: added 6 unit tests covering non-string command guard, non-absolute directory rejection, prefix-collision rejection, traversal rejection, workspace acceptance, and partial-line cap behaviour (including buffer reset). All 26 monitor.test.ts cases pass. The same startsWith pattern also exists in ShellTool and is tracked as a separate follow-up to keep this PR focused on Phase C scope. * fix(core): scope monitor always-allow permissions Populate Monitor confirmation permissionRules using the same command-rule extraction path as ShellTool, so ProceedAlways persists command-scoped Bash(...) rules instead of a broad monitor-level allow. Also add unit coverage for command-scoped rules, filtering already-allowed subcommands, and extractor fallback behavior. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): decouple monitor permission scope from Bash rules Remove pm.isCommandAllowed() from MonitorToolInvocation.getConfirmationDetails() to prevent existing Bash(...) allow rules from shrinking the monitor confirmation scope. Monitor is a long-running background process with a different risk profile than one-shot shell execution and should maintain its own permission boundary. Only AST-based read-only filtering is retained. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): unify monitor error/exit cleanup to prevent resource leaks Extract a shared cleanup() helper called from both the `exit` and `error` event handlers. Previously the `error` handler did not flush buffers, clear buffer values, remove the abort listener, or log dropped-line stats — causing potential memory leaks when `error` fires without a subsequent `exit` (e.g. ENOENT for missing commands). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): add user-skills-directory guard to monitor directory validation Mirror ShellTool's getUserSkillsDirs() check in MonitorTool's validateToolParamValues() to prevent monitor commands from running inside user skills directories. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(core): add Monitor(...) permission namespace for monitor tool (#3726) Introduce a dedicated Monitor(...) permission namespace so monitor and shell tools have independent permission boundaries. Previously monitor emitted Bash(...) rules, causing "Always Allow" to fail for future monitor invocations while unintentionally granting run_shell_command. Changes: - rule-parser.ts: add Monitor alias, SHELL_TOOL_NAMES entry, CANONICAL_TO_RULE_DISPLAY, DISPLAY_NAME_TO_VERB - permission-manager.ts: extract SHELL_LIKE_TOOLS set so evaluate(), evaluateSingle(), hasRelevantRules(), hasMatchingAskRule() handle both run_shell_command and monitor - monitor.ts: emit Monitor(...) instead of Bash(...) in permissionRules - Tests: parseRule, matchesRule, cross-tool isolation regression, buildPermissionRules, buildHumanReadableRuleLabel for Monitor Co-authored-by: jinye.djy <jinye.djy@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): decouple headless monitor lifetime from final result Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): stabilize stream-json monitor session shutdown Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): deny monitor in headless approval defaults Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): honor tool aliases in headless allow checks Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): address opus review — sleep regex, monitor cap, non-interactive cleanup - Fix sleep interception false positive for backgrounded sleep (`sleep 5 & echo done`). Remove bare `&` from separator character class so the background operator is not treated as a sequential separator. - Add MAX_CONCURRENT_MONITORS (16) check in MonitorRegistry.register() and early rejection in MonitorTool.execute() to prevent unbounded process spawning. - Widen monitorId from 8 to 16 hex chars to reduce birthday collision risk. - Abort all running monitors in nonInteractiveCli.ts success-path finally so piped stdio refs don't keep the Node event loop alive after result emission in one-shot (--print) mode. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): abort monitors and background shells on /clear Without this, long-running monitors from a previous session survive /clear and continue pushing events into the new session's notification queue. This enables cross-session prompt injection where a malicious monitor persists across the user's escape hatch. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): abort monitors on stream-json session shutdown Call monitorRegistry.abortAll() in both shutdown() and drainAndShutdown() so detached monitor child processes don't survive session termination. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): use content event type in stream tests Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): isolate session cleanup on clear and shutdown Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): finalize session cleanup after drain Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): close remaining monitor review gaps Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): preserve shell cwd in virtual permission checks Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): normalize trailing background ampersands Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): align monitor permission and wrapper handling Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): make monitor CI assertions cross-platform Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): align monitor wrapper normalization Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): normalize wrapped monitor commands Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): harden monitor headless edge cases Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): preserve monitor spawn errors Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): harden monitor register cleanup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): parse monitor wrapper script token Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): address PR review comments for monitor tool - Make Bash(...) permission rules cover monitor via toolMatchesRuleToolName, so deny rules like Bash(rm *) also block monitor({command: "rm ..."}) - Remove dead `normalizeRuleToolName` mock reference in config.test.ts - Fix tool description to mention stdout/stderr instead of just stdout - Export MAX_CONCURRENT_MONITORS from monitorRegistry and use it in monitor.ts instead of hardcoded 16 - Rename ambiguous MAX_LINE_LENGTH constants: PARTIAL_LINE_BUFFER_CAP (4096, monitor.ts) and EVENT_LINE_TRUNCATE (2000, monitorRegistry.ts) - Fix schema description text: "Max 80 characters" → "Truncated to 80 characters in display" - Add .unref() to SIGTERM→SIGKILL escalation timer to prevent 200ms exit delay Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): resolve clear command typecheck issues Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): preserve background tasks across shutdown abort Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): close monitor review gaps Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): address latest monitor review comments Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): handle monitors across session switches Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): cover aborted monitor startup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): address remaining monitor PR review comments Adopts four unresolved review threads on PR #3684: * shell: trim top-level trailing comments before validating sleep separator so 'sleep 5 # wait' no longer bypasses detectBlockedSleepPattern. * monitor: add sanitizeMonitorLine to strip C0/C1 control chars (except tab) and defang structural envelope tag names with U+200B before forwarding output to the model, blocking prompt-injection attempts hidden in monitored stdout/stderr. * monitor: declare line buffers and throttledEmit before abortHandler to avoid TDZ on synchronous abort paths, and add flushPartialLineBuffers called from both abortHandler (before kill) and cleanup (natural exit/error) so partial-line data is no longer silently dropped on cancel. * permissions: document that normalizePermissionContext relies on buildPermissionCheckContext to forward monitor's directory as cwd, and add regression tests proving relative-path Read(./...) allow and deny rules resolve against the monitor's explicit cwd. * fix(core): abort running monitors in MonitorRegistry.reset() reset() previously only cleared idle timers and emptied the map without aborting running monitors' AbortControllers. This could orphan child processes when reset() was called without a prior abortAll(), e.g. via useResumeCommand → resetBackgroundStateForSessionSwitch. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core): harden monitor notification XML and displayText - Extend escapeXml to escape " and ' as defense-in-depth: safe to reuse the helper in any future XML attribute context without re-auditing. - Strip C0 (except tab) and C1 control characters from the displayText surface before interpolation, so untrusted child-process output cannot leak ANSI escapes / NUL bytes into the operator's terminal even if a direct caller of MonitorRegistry.emitEvent skips sanitization. Adds unit tests for both hardening paths. * test(core): cover token-bucket throttling and commented-sleep bypass - Add 4 unit tests for the monitor token-bucket throttle (burst=5, 1 token/sec refill): burst cap, refill release, long-idle bucket cap, and whitespace lines not consuming budget. Uses vi.setSystemTime to exercise Date.now() without advancing pending setTimeouts. - Add an E2E case that feeds 'sleep 5 # wait for db' through the shell tool to lock in trimTrailingShellComment behavior end-to-end; the unit-level coverage in shell.test.ts remains authoritative but the E2E anchor prevents a regression from silently passing unit tests. * fix(core): address 3 remaining copilot review comments 1. shell.ts sleep interception: strip shell wrapper before detecting the blocked sleep pattern so `bash -c 'sleep 5'` / `sh -c ...` cannot route around the block. Mirrors every other sensitive check in shell.ts, which already normalizes through stripShellWrapper. 2. monitorRegistry.ts emitEvent auto-stop: settle the entry BEFORE aborting its controller so that any synchronous abort listener that flushes buffered output back through registry.emitEvent() (e.g. the Monitor tool's flushPartialLineBuffers) finds status !== 'running' and short-circuits instead of overshooting maxEvents and emitting a duplicate 'Max events reached' terminal notification. 3. monitorRegistry.ts truncateDescription: cap output at exactly MAX_DESCRIPTION_LENGTH by counting the ellipsis against the budget, instead of returning MAX_DESCRIPTION_LENGTH + 3 characters. Each fix is covered by a new unit test. * fix(core): address review comments — sanitize, notify, kill logging, throttle observability - Remove double normalize in buildPermissionCheckContext (PM is single source) - Add {notify:false} to Config.shutdown() and abortTaskRegistries() abortAll - Swap settle-before-abort in cancel() and resetIdleTimer() to prevent races - Add stripDisplayControlChars to emitTerminalNotification - Sanitize monitor description at entry creation via sanitizeMonitorLine - Surface throttle-dropped line count in terminal notification - Add .unref() to idle timer to allow clean process exit - Add error handler + stdio:ignore to Windows taskkill spawn - Log SIGTERM/SIGKILL kill failures via debugLogger.warn - Attach early child error handler to cover spawn-to-register window - Destroy child stdio on register failure to prevent handle leaks - Improve stripShellWrapper to handle absolute paths, combined flags, env prefix - Improve SHELL_TOOL_NAMES documentation and toolMatchesRuleToolName clarity Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): resolve monitor tool typecheck errors - Cast child.stdout/stderr to a minimal { destroy?: () => void } shape so the optional destroy() call compiles and still works with test mocks. - Initialize droppedLines: 0 in MonitorEntry test fixtures that predate the field becoming required. * fix(monitor): add missing stdio option in taskkill test assertions (#3784) * fix(core): address monitor review feedback * fix(core): harden monitor command lifecycle --------- Co-authored-by: jinye.djy <jinye.djy@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> |
||
顾盼
|
aeeb2976d6
|
feat(web-search): remove built-in web_search tool, replace with MCP-based approach (#3502)
* feat(web-search): add GLM (ZhipuAI) web search provider - Add GlmProvider class implementing BaseWebSearchProvider using the ZhipuAI Web Search API (https://open.bigmodel.cn/api/paas/v4/web_search) - Support multiple search engines: search_std, search_pro, search_pro_sogou, search_pro_quark - Support optional config: maxResults, searchIntent, searchRecencyFilter, contentSize, searchDomainFilter - Truncate query to 70 characters per API limit - Register 'glm' in the provider discriminated union (types.ts) and createProvider() switch (index.ts) - Add GlmProviderConfig to settingsSchema, ConfigParams, and Config class - Add --glm-api-key CLI flag and GLM_API_KEY env var support in webSearch.ts - Forward GLM_API_KEY in sandbox environment - Update provider priority list: Tavily > Google > GLM > DashScope - Add 17 unit tests for GlmProvider and 4 integration tests in index.test.ts - Update docs/developers/tools/web-search.md with GLM configuration, env vars, CLI args, pricing, and corrected DashScope billing info - Fix stale OAuth/free-tier references in web-search.md Closes #3496 * docs(web-search): fix DashScope note and add GLM server-side limitations * fix(web-search): make DashScope provider work with standard API key, remove qwen-oauth dependency - DashScopeProvider.isAvailable() now checks config.apiKey instead of authType - Remove OAuth credential file reading and resource_url requirement - Use standard DashScope endpoint: dashscope.aliyuncs.com/api/v1/indices/plugin/web_search - Read DASHSCOPE_API_KEY env var and --dashscope-api-key CLI flag - Forward DASHSCOPE_API_KEY into sandbox environment - Update integration test to detect DASHSCOPE_API_KEY - Update docs to reflect new API key based configuration * feat(web-search): remove built-in web search tool The web_search tool and all related provider implementations are removed. Web search functionality will be provided via MCP integrations instead, which is the direction the broader agent ecosystem is moving. Removed: - packages/core/src/tools/web-search/ (entire directory) - packages/cli/src/config/webSearch.ts - integration-tests/cli/web_search.test.ts - ToolNames.WEB_SEARCH, ToolErrorCode.WEB_SEARCH_FAILED - webSearch config in ConfigParams, Config class, settingsSchema - CLI options: --tavily-api-key, --google-api-key, --google-search-engine-id, --glm-api-key, --dashscope-api-key, --web-search-default - Sandbox env forwarding for TAVILY/GLM/DASHSCOPE/GOOGLE search keys - web_search from rule-parser, permission-manager, speculation gate, microcompact tool set, and builtin-agents tool list * fix: remove websearch reference * docs: remove websearch tool * docs: add break change guide * fix review |
||
|
tanzhenxin
|
8ae1efbf80
|
test(integration): switch settings-migration probe from --help to mcp list (#3486)
* test(integration): switch settings-migration probe from --help to mcp list --help is a purely informational command and intentionally does not load settings. The settings-migration integration test was leaning on a legacy side effect where --help happened to run loadSettings() during startup, which in turn persisted the migrated file back to disk. After the bare startup mode refactor reordered startup so that argument parsing runs before settings loading, yargs now exits inside parse() on --help before loadSettings() is ever called, and the test fixtures stayed at their original version on disk. Switch the probe to `mcp list`, which is a first-class subcommand that goes through loadSettings() (and therefore the migration chain and the write-back) and then exits without needing API credentials or network. On a fresh test rig with no configured servers it prints a single line and returns, so the test stays fast. No production code changes; --help remains side-effect-free. * test(cli): remove flaky right-arrow prompt suggestion test The test intermittently fails in CI because the render and stdin write race with the component's readiness window; covered by the other prompt suggestion tests in the same file. |
||
|
tanzhenxin
|
e03144ed34 |
fix: resolve punycode to userland package and skip env var test in sandbox
- Resolve punycode to userland package instead of deprecated node:punycode built-in to avoid deprecation warnings - Skip cron-tools env var test in sandbox mode since Docker containers don't receive environment variables set in the test process Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
tanzhenxin
|
1af0ef9a7c | Merge remote-tracking branch 'origin/main' into feat/in-session-cron-loops | ||
|
tanzhenxin
|
9dd22eb152 |
feat(cron): integrate cron scheduling into ACP session lifecycle
- Add cron queue and scheduler state management to Session class - Handle cron cancellation on user prompt and cancelPendingPrompt - Start cron scheduler after prompt execution completes - Drain cron queue sequentially to prevent concurrent chat access - Execute cron prompts with proper message echoing and tool handling - Add integration test for cron firing and sessionUpdate streaming This enables cron jobs created during an ACP session to fire in the background and stream results back to the client via sessionUpdate notifications, even after the originating prompt has returned. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
tanzhenxin
|
ded89618ec |
refactor(tests): reorganize integration tests by execution mode
Move non-interactive tests to cli/, interactive tests to interactive/. Add cron-interactive.test.ts wrapping terminal-capture E2E in vitest. Update npm scripts and release workflow for new directory layout. |