Anthropic's Messages API rejects any tool `input_schema` carrying a
top-level `oneOf`/`allOf`/`anyOf` combinator with a hard 400:
input_schema does not support oneOf, allOf, or anyOf at the top level
send_message's schema used `oneOf: [{ required: ['to'] }, { required:
['task_id'] }]` to express "either a teammate recipient or a
background-task id is required." Since this schema is forwarded
verbatim as `tools[].input_schema` to Anthropic
(anthropicContentGenerator/converter.ts -> convertSchema), every call
to send_message on an Anthropic-backed model (native API, Vertex, or
an Anthropic-compatible proxy) failed outright -- not degraded, fully
unusable.
Verified live against the Anthropic Messages API (claude-sonnet-5):
the schema with `oneOf` 400s with the exact message above; the
identical request with `oneOf` removed succeeds (200, tool_use
returned).
The `oneOf` constraint was redundant defense-in-depth, not the only
enforcement: send_message's execute() already returns a clear runtime
error ("No active team and no task_id provided...") when neither `to`
nor `task_id` is supplied. Dropping the schema-level constraint leaves
that runtime validation intact and unchanged for OpenAI/Gemini-backed
models, while making the tool actually callable on Anthropic-backed
models.
Fixes#7984
Co-authored-by: Palanisamy, Dinesh <Dinesh.Palanisamy@netapp.com>
* feat(hooks): add security.allowPrivateNetworkHooks to bypass SSRF range checks for trusted scopes
HTTP hooks hard-block all private/link-local address ranges via ssrfGuard,
which makes them unusable in platform-managed environments where the hook
receiver is a first-party, VPC-internal endpoint (e.g. an internal API
gateway resolving to 172.16.0.0/12).
Add an opt-in setting, security.allowPrivateNetworkHooks (default false),
that skips the SSRF IP-range checks in urlValidator.isBlocked (literal IPs)
and validateResolvedHost (literal + post-DNS-resolution paths).
Security properties:
- Honored only from User/System/SystemDefaults scopes; the value is
stripped from Workspace settings during the merge (with a startup
warning), so a cloned repository can never self-grant the bypass.
- BLOCKED_HOSTS (169.254.169.254, metadata.google.internal, ...) remains
blocked even when the flag is on.
- Default false keeps every code path byte-for-byte compatible with
current behavior; bare/safe mode forces it off.
* fix(hooks): enforce metadata endpoint blocklist regardless of allowPrivateNetworkHooks
Address review findings on #7968: with the flag on, cloud metadata
endpoints were reachable through gaps in the relaxed checks.
- ssrfGuard: add METADATA_IPS (169.254.169.254, 100.100.100.200) and
isMetadataAddress(), which normalizes IPv4-mapped IPv6 forms
(::ffff:a9fe:a9fe, ::ffff:6464:64c8, ...) via the existing
extractMappedIPv4/expandIPv6Groups helpers.
- urlValidator.isBlocked: BLOCKED_HOSTS matching and the literal-IP
isMetadataAddress check now run unconditionally; only the general
range check (isBlockedAddress) is relaxed by the flag.
- httpHookRunner.validateResolvedHost: no longer returns early with the
flag on — DNS resolution still runs and resolved addresses are checked
against isMetadataAddress, so a hostname resolving to a metadata
endpoint is blocked. DNS failures still defer to fetch, as before.
- settings warning text now lists User/System/SystemDefaults, matching
the schema and docs.
- docs: precise wording — the flag relaxes only range checks; metadata
endpoints stay blocked in all serialized forms and after DNS resolution.
The flag now opens RFC1918/CGNAT/link-local ranges only; cloud metadata
endpoints (169.254.169.254, 100.100.100.200 in any form, plus the
BLOCKED_HOSTS hostnames) are unreachable in every configuration.
---------
Co-authored-by: 欢伯 <ri.xur@alibaba-inc.com>
* fix(serve): allow bounded reads of large text files
* fix(serve): bound large-text reads by scan cost, not by which knob was set
Follow-up to the bounded large-text read path. Three changes:
Gate on any explicit window argument, not on `limit`. Gating on `limit`
had the cost model backwards in both directions: `{ line: 900_000_000,
limit: 20 }` was admitted despite walking the whole file, while
`{ maxBytes: 4096 }` — satisfiable from the first 4 KiB — was refused. A
read with no window argument at all still fails, since a caller that
believes it holds the whole file may write it back truncated.
Add MAX_TEXT_SCAN_BYTES (8 MiB). MAX_READ_BYTES caps what a read
returns; nothing capped what it cost. Line offsets are resolved by
scanning from byte 0, so a query param could turn into an
uninterruptible multi-second scan of an arbitrarily large file — and on
Windows hold a read handle for that span, blocking renames and deletes.
Past the budget the read is refused with `file_too_large` pointing at
readBytes, which reaches any offset in O(1).
Tolerate appends on streamed windows. Requiring whole-file size/mtime
stability after reading a prefix rejected reads whose returned bytes
were still valid, and the case it rejected — tailing a live log — is the
one this path exists for. Streamed windows now assert inode identity
plus "did not shrink"; truncation and replacement are still rejected.
Also: non-UTF-8 large text now returns `binary_file` rather than
`file_too_large`, so a client retrying on 413 with a smaller window
can't loop forever; and `readFileWithLineAndLimit` throws instead of
silently ignoring a caller-supplied `fileHandle` on the by-path
fallback.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(serve): harden large text range snapshots
Treat caller-owned file handles as bounded streaming reads, cap them to the captured file size, and reuse the chunk buffer.
Restore strict Serve snapshot stability and align returned-slice metadata with the full-snapshot path.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(serve): make large text ranges snapshot-safe
* fix(serve): harden large-text ctime tests and document buffer reuse (#7947)
Address review feedback on the large-text range read PR:
- Pause before restoring mtime in the two ctime-dependent mutation tests so the change-time advances past the pre-read snapshot even on coarse-resolution filesystems, removing a latent flake in the same-size-overwrite precondition. The assertions are unchanged.
- Document at the readFileHandleChunks yield site that the 512 KiB buffer is reused across iterations, so yielded views must be decoded or copied before advancing the generator.
* docs(serve): soften same-size rewrite guarantee to coarse-clock best-effort (#7947)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
* fix(core): mark folders the item budget never expanded
A folder is pushed into its parent's subFolders and queued for expansion.
When it is later dequeued and the maxItems budget is already spent, the
BFS skips it with a bare `continue`, leaving hasMoreFiles and
hasMoreSubfolders unset. It then formats as a bare leaf -- byte for byte
how a genuinely empty directory formats.
So the same tree was described differently depending only on the budget.
The existing suite shows it: `level1/level2/level3/file.txt` renders
level3 with its file at maxItems 10, and as an empty directory at
maxItems 3. The subfolder test is worse -- folder-0 through folder-3 each
contain child.txt and all four render as empty.
The parent's hasMoreSubfolders flag does not cover this. It means "I could
not add every subfolder", and these folders were added; it is only the
expansion that never happened.
Set hasMoreSubfolders on the skipped node so it renders with the
truncation indicator. Only that one flag is set: formatStructure emits an
indicator per flag, and this one alone produces the single trailing `...`.
Three existing expectations encoded the old rendering and are updated.
* docs(core): state both meanings of hasMoreSubfolders on the interface
This PR widened the field: it already meant "subfolders were truncated" and
now also means "the item budget ran out before this folder was read." The
interface comment only described the first, so a second consumer reading just
the type could gate rendering on `subFolders.length > 0` and silently drop the
indicator for never-expanded folders, reintroducing the same
looks-like-an-empty-directory bug in a new path.
* fix(core): improve ripgrep runtime reliability
Make ripgrep failures distinguishable from true no-match results so the
model avoids unsafe conclusions from partial or failed searches. Add a
narrow recovery path for confirmed worker-thread EAGAIN failures.
- Retry confirmed thread EAGAIN once with a single worker thread
- Treat exit code 1 as no-match only when both streams are empty
- Mark partial runtime results as incomplete, separate from display limits
- Add privacy-safe telemetry and focused coverage for recovery behavior
# Conflicts:
# packages/core/src/utils/ripgrepUtils.test.ts
* fix(core): restore ripgrep runtime recovery tests
Restore the runRipgrep coverage that was lost during the rebase and keep
EAGAIN detection aligned with the runtime reliability design.
- Re-add coverage for strict no-match handling and incomplete output
- Verify single-thread retry behavior for confirmed EAGAIN failures
- Keep spawn, cancellation, timeout, and max-buffer paths covered
- Treat os error 11 as the short EAGAIN marker documented by the plan
* docs(core): document ripgrep recovery boundaries
Clarify the non-obvious runtime reliability edges around ripgrep recovery so
future changes preserve the intended narrow behavior.
- Document why only confirmed worker EAGAIN failures are retryable
- Explain incomplete-output handling for interrupted ripgrep output
- Note the privacy boundary for runtime recovery telemetry
* fix(core): correct exit-1 no-match gate for --json summary output (#7888)
* test(core): remove duplicate mockReset and add telemetry coverage (#7888)
* fix(core): address review feedback on ripgrep recovery semantics (#7888)
- Fix stale `truncated` property in test mock to match RipgrepRunResult
- Narrow `incomplete` flag to genuinely interrupted executions only
- Add test for exit code 1 with both stdout and stderr
- Add test for EAGAIN retry producing partial stdout
- Alias RipgrepRuntimeRecoveryFailureKind to RipgrepFailureKind
* docs(core): align exit-1 no-match spec with stderr-only gate (#7888)
* fix(core): mark exit-failed ripgrep searches with stdout as incomplete (#7888)
---------
Co-authored-by: heyang.why <heyang.why@alibaba-inc.com>
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
* feat(core): integrate Goal turn engine
* test(core): preserve Goal tool isolation in agent overrides
* fix(core): pause Goal at Stop hook cap
* fix(core): keep Goal recovery from blocking sessions
* fix(core): degrade failed Goal migration writes
* fix(core): reset loop detector in Goal stop hook continuation (#7895)
The goal-runtime stop hook continuation path was missing
loopDetector.reset(prompt_id) before recursing, causing tool calls
to accumulate across iterations and trip TURN_TOOL_CALL_CAP after
a handful of healthy iterations. The non-goal stop hook path already
had this reset.
Also simplifies the redundant conditional in Turn.run() — the two
near-identical sendMessageStream calls are collapsed into one since
sendMessageStream already handles an undefined goalContext internally.
* fix(core): align turn.test.ts assertion with unified sendMessageStream call (#7895)
* fix(core): preserve error cause in GoalPersistenceUnavailableError (#7895)
---------
Co-authored-by: qwen-code-dev-bot <269191875+qwen-code-dev-bot@users.noreply.github.com>
Co-authored-by: qwen-code-bot <qwen-code-bot@users.noreply.github.com>
* feat(web-shell): add git branch picker, commit dialog, and create PR flow
Add an IntelliJ-style branch picker popover to the web shell git
workspace, accessible from the branch chip in both the composer toolbar
and sidebar. The picker provides search-filtered branch listing (local,
remote, tags, recent), branch checkout, new branch creation, pull, push,
and a commit view integrated into the existing GitDialog.
The commit view reuses the diff panel (expandable file diffs with syntax
highlighting, fullscreen support) and adds a commit message textarea with
Commit / Commit and Push buttons. When a session is available (or one is
auto-created), the commit message and PR title/body are generated via the
model using session side-queries (btwSession), giving the agent full
conversation context for accurate generation.
The Create PR flow provides an inline form with auto-detected base branch
and model-generated title/description, backed by a new daemon route that
shells out to gh pr create.
New daemon routes:
- GET /workspaces/:workspace/git/branches
- POST /workspaces/:workspace/git/checkout
- POST /workspaces/:workspace/git/branch
- POST /workspaces/:workspace/git/push
- POST /workspaces/:workspace/git/pull
- POST /workspaces/:workspace/git/commit
- POST /workspaces/:workspace/github/prs/create
- GET /workspaces/:workspace/github/default-branch
* fix(web-shell): resolve correct workspace session for AI generation
The commit message and PR title/body generation now resolves the
most recent session for the target workspace via listWorkspaceSessions,
rather than using the globally active connection.sessionId which may
belong to a different workspace or session. Falls back to creating a
new session only when no sessions exist for the workspace.
Also improves the PR body editor with an Edit/Preview toggle using
the existing Markdown component, and updates the generation prompt
to follow the project PR template structure from AGENTS.md.
* fix(web-shell): stabilize session resolver prop to prevent infinite re-generation
Pass resolveSessionForWorkspace as a stable useCallback reference
instead of an inline arrow function. The inline function created a
new reference on every App render, causing the GitDialog useEffect
to abort and restart generation in an infinite loop.
* fix(web-shell): group remote branches by remote name and add PR target branch dropdown
Remote branches in the branch picker are now grouped by remote
(origin, upstream, etc.) with sub-headers, making fork workflows
clear. The PR create form's base branch field is now a select
dropdown populated from the workspace's branch list, grouped by
remote with optgroup labels, instead of a free-text input.
* fix(web-shell): use ref for session resolver to prevent effect re-run abort
When resolveSessionForWorkspace creates a new session, it updates
connection.sessionId in the provider, which changes the useCallback
reference, which triggers the useEffect to re-run and abort the
in-flight btwSession generation. Store the callback in a ref so the
effect never depends on its identity.
* fix(web-shell): resolve session per workspace, not from global active session
The commit/PR generation effects used connection.sessionId directly
without checking if it belongs to the target workspace. When opening
the commit dialog from a sidebar workspace different from the active
session's workspace, the wrong session was used for generation,
producing incorrect content. Now always routes through
resolveSessionForWorkspace(workspaceCwd) which checks workspace
membership before reusing the active session.
Also replaces 'PR' with 'Pull Request' / '合并请求' in all UI strings
and adds error logging to the generation catch blocks.
* fix(web-shell): retry btwSession with fresh session when stale session detected
When listWorkspaceSessions returns a session that no longer exists in
the daemon's memory (e.g. after daemon restart), btwSession fails with
'No session with id ...'. The generation effects now catch this error,
force-create a new session via resolveSessionForWorkspace(cwd, true),
and retry the btwSession call once. Both btwWithRetry and
resolveSessionForWorkspace are stored in refs to avoid useEffect
dependency chain aborts.
* fix(web-shell): base PR generation on branch diff, not working tree
PR title/body generation now fetches the commit log between the
resolved base branch and HEAD (git log <base>..HEAD) plus any
uncommitted changes, instead of only the working tree diff. The
base branch is resolved inside the effect's promise chain (not
from state) to avoid stale values and dependency warnings. Also
adds range parameter support to fetchGitLog and workspaceGitLog.
* feat(web-shell): replace PR base branch select with searchable popover
The native <select> for the PR target branch is replaced with a
custom searchable popover (BranchSelect) that shows a search input
and a filtered branch list grouped by remote. The default selection
is the target repository's main branch (resolved via
getDefaultBranch). Supports filtering by typing in the search box.
* fix(web-shell): show full remote ref in branch select (origin/main)
Branch select now stores and displays full remote refs like
origin/main instead of stripped names. getDefaultBranch returns
the full ref (origin/main) instead of stripping the prefix.
When creating the PR, the remote prefix is stripped for the
gh pr create --base flag (origin/main → main).
* fix(web-shell): stop pointer propagation in branch picker list to prevent popover dismiss
Radix Popover's outside-click detection was incorrectly firing when
clicking section headers (Recent/Local/Remote/Tags) inside the popover
content, causing the popover to close immediately. Adding
onPointerDown stopPropagation on the list container prevents pointer
events from reaching Radix's document-level handlers.
* fix(web-shell): use onPointerDownOutside guard for branch picker popover
The previous stopPropagation approach failed because Radix Popover
uses capture-phase document listeners for outside-click detection,
which fire before bubble-phase stopPropagation. The correct fix is
onPointerDownOutside on PopoverContent: when Radix incorrectly fires
the outside handler for a click that is actually inside the content
(can happen with portal containers), we check contentRef.contains()
and preventDefault to keep the popover open.
* fix(web-shell): stop click propagation on branch picker popover content
Root cause: the ChatEditor composer container has onClick that calls
core.focus(), stealing focus from the popover. React synthetic events
bubble through the React tree (not DOM tree), so portaled popover
clicks reach the container handler. Radix then detects focus-outside
and dismisses the popover.
Fix: onClick stopPropagation on PopoverContent, matching the existing
pattern in GitModePopover and ToolbarPopover which already have this
fix with an explanatory comment.
* docs: add PR verification screenshots for branch picker feature
* fix(web-shell): mock useWorkspace in tests for BranchPickerPopover
BranchPickerPopover calls useWorkspace() which requires
DaemonWorkspaceProvider context. The existing WorkspaceSection and
ChatEditor tests didn't provide this context, causing 5 test failures.
Added vi.mock with importActual to preserve other exports while
providing a mock useWorkspace. Also updated the git chip click test
to reflect that clicking now opens the branch picker popover instead
of directly calling onOpenGitDiff.
* fix: harden git write paths against argument injection
Address review feedback on the web-shell git surface:
- Reject option/pathspec injection in git checkout ref and branch start
point (isValidCheckoutRef), and terminate `git checkout` argv with `--`.
- Drop `git log` range values that start with `-` and terminate the argv
with `--` so a range can never be reinterpreted as `--output=<file>`.
- Fix getDefaultBranch always falling back to origin/main: the promisified
exec lacked `encoding: 'utf8'`, so stdout was a Buffer and .trim() threw.
- Parameterize ghErrorMessage so `gh pr create` timeouts name the right
command and duration; sanitize workspace paths in PR-create errors.
- GitDialog: guard doCommit against double-submit (button + keyboard), and
strip only a known remote prefix from the PR base so local branches with
"/" are not mangled; use theme tokens for commit button/success colors.
- BranchPickerPopover: guard checkout/new-branch behind busyAction, reset
inline-input text on reopen, and hide the commit action when unavailable.
Adds regression tests for the checkout/branch validation and the git log
range guard.
* fix(web-shell): address review feedback on branch picker PR (#7731)
- Fix Commit+Push error masking: split try/catch so push failure
reports alongside the successful commit SHA
- Replace hardcoded screenshot path with captureScreenshot harness
- Replace silent if-isVisible skip with explicit assertion in visual test
- Add focus-visible style for search input accessibility
- Fix CSS specificity for active PR tab hover state
- Add viewChanges to actionsVisible search filter
- Wrap toggleSection in useCallback to avoid unnecessary re-renders
- Add windowsHide: true to getDefaultBranch subprocess
- Fix trailing slash handling in mockDaemon git action routing
- Add git methods to top-level client mock in tests
- Remove dead branchPicker.commitSuccess i18n key
- Remove dead .actionShortcut CSS class
- Show generation failure feedback in commit message placeholder
* fix(web-shell): address review feedback on branch picker PR (#7731)
- Add workspace trust checks to bound git branch routes
- Use workspace-scoped client in BranchPickerPopover (fixes wrong-workspace mutation)
- Add branch name validation and -- terminator to gitCreateBranch
- Filter refs/remotes/*/HEAD from branch listings
- Force LC_ALL=C for reflog parsing (non-English locale fix)
- Narrow 'could not resolve' error regex to avoid DNS false positives
- Add range validation to git log (reject path traversal)
- Fix commit+push error i18n (dedicated key instead of concatenation)
- Add i18n for BranchSelect component strings
- Fix commit tab ARIA attributes
- Add onBranchChanged callback to handlePush
- Add btw to mockDaemon isDaemonPath regex
- Add workspace_github_prs to visuals spec capabilities
- Add -- terminator regression test
- Remove docs/pr-assets/ from repo
* fix(web-shell): address review feedback on branch picker PR (#7731)
* fix(cli): reject dash-prefixed branch name with 400 in branch route (#7731)
* fix(web-shell): address review feedback on git branch picker (#7731)
Security:
- Clear GIT_DIR/GIT_WORK_TREE/GIT_COMMON_DIR/GIT_INDEX_FILE from git
subprocess env to prevent repository redirection
- Add strict mutation gate to all POST git branch routes
- Add generation guard to qualified write routes
- Fail closed on invalid ?cwd= in mutation routes (resolveContainedCwdOrFail)
- Reject wrong-typed startPoint, fetchOnly, rebase, and PR options with 400
Correctness:
- Filter remote symbolic refs (origin/HEAD) by %(symref) instead of /HEAD
name suffix, preserving valid branches like feature/HEAD
- Add git rev-parse --git-dir probe so non-git dirs get 404 instead of
empty available:true
- Push preserves existing upstream; only adds --set-upstream when unset,
resolving the remote from branch config or the sole configured remote
- git commit -a replaced with git add -A + git commit so untracked files
displayed in the UI are included
- Always pass --body to gh pr create to prevent interactive prompts
- getDefaultBranch returns null instead of fabricating origin/main
- Memoize workspaceByCwd client in BranchPickerPopover to fix infinite
render loop
- Move sessionId to a ref in GitDialog effects to prevent self-abort
- Bound commit-message prompt to fit /btw 4096-char limit
- Mark all platforms as unverified in PR template (no fabricated ✅)
- Guard PR auto-fill effect against wiping user edits on reconnect
Accessibility:
- Add tabIndex and onKeyDown to commit-mode tab span
- Add aria-label to BranchSelect trigger and search input
Cleanup:
- Remove dead CSS (.prInputSmall, .prSelect)
- Remove 9 unused i18n keys
- Add ^ to git log range validation regex
- Add busyAction guard to handlePush/handlePull
- Add unit tests for gitCommit, gitPull, and route input validation
* fix(web-shell): address review feedback on git branch picker PR (#7731)
- Change commit tab from <span> to <button> for keyboard accessibility
- Move setCommitMsg('') to success-only branches so the message is
preserved when push fails after a successful commit
- Add mutate middleware and generationGuard to PR creation route,
matching all other POST mutation routes
- Set genFailed when session resolution returns undefined so the user
sees the failure indicator instead of a silent empty textarea
- Make PR number nullable when URL regex does not match instead of
returning a misleading 0
- Add LC_ALL=C and LANG=C to gitEnv() so for-each-ref upstream track
parsing is locale-independent
- Validate setUpstream and force as booleans in handlePush, matching
the existing validation in handlePull
- Add missing workspaceCwd and available fields to test mocks
- Use stable data-web-shell-git-branch attribute in e2e selector
* fix(web-shell): address R5 review feedback on git branch picker PR (#7731)
- Classify git errors on stdout+stderr instead of err.message to fix
false-positive no_upstream on every push failure and dead
nothing_to_commit classifier
- Sanitize workspace paths and cap error message length in sendGitError
- Fix remote branch checkout to strip remote prefix so git DWIM creates
a local tracking branch instead of detaching HEAD
- Restore keyboard accessibility on composer branch chip (span → button)
- Trim startPoint in handleCreateBranch before forwarding to git
- Return bare branch name from getDefaultBranch (strip remote prefix)
- Fix i18n shortcut hint to show ⌘/Ctrl+Enter for cross-platform
- Update aria-label to reflect git management menu, not just changes
- Add available: true to mockDaemon gitDiff default payload
- Add regression tests: upstream preservation, sole remote resolution,
strengthened fetch-only with divergent remote commit
- Add aria-expanded assertion to sidebar picker test
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(web-shell): address R6 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R7 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R8 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R9 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R10 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R11 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R12 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R13 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R14 review feedback on git branch picker PR (#7731)
* fix(web-shell): address R15 review feedback on git branch picker PR (#7731)
- Validate localName derived from remote-tracking ref to prevent option
injection (e.g. origin/-f → git checkout -f)
- Add gitCwd prop to BranchPickerPopover and pass it to all git SDK calls
so worktree sessions target the correct directory
- Add symlink-escape and non-existent-path tests for resolveContainedCwdOrFail
- Pin initial branch name in makeRepo() with git init -b master
- Add gitPull merge and rebase integration tests
* fix(web-shell): address git branch picker review feedback (#7731)
* fix(web-shell): address R6 review feedback on git branch picker PR (#7731)
* fix(web-shell): address review feedback on git branch picker PR (#7731)
- Strip GIT_CONFIG_GLOBAL/SYSTEM/NOSYSTEM in gitEnv to prevent
inherited config redirection (consistent with extension/github.ts)
- Pass gitCwd to workspaceGitBranches in GitDialog loadPrBranches
so worktree sessions fetch branches from the correct repository
- Add aria-expanded to collapsible branch section headers
- Add happy-path tests for PR create (201) and default-branch (200)
routes, including the null fallback to origin/main
* fix(cli): add sendGenerationClosedError to POST routes and cover untested branches (#7731)
* fix(web-shell): address review feedback on branch picker and PR creation (#7731)
- Refresh branch list after push/pull to avoid stale ahead/behind counts
- Add pre-flight check for unpushed branches before PR creation
- Fix base branch prefix stripping when branch list is unavailable
- Cap PR body file list at MAX_SUMMARY_CHARS to bound model prompt size
- Add qualified route tests: trust guard, input validation, cwd containment
* fix(web-shell): hoist MAX_SUMMARY_CHARS to module scope for PR body generation (#7731)
* fix(web-shell): address review feedback for git branch picker (#7731)
- Hoist onOpenCommit to useCallback to fix App.test.tsx prop stability test
- Keep commit tab visible after navigating away (startedInCommit ref)
- Add onClick handler to commit tab for navigation back to commit view
- Fix branch-prefix strip mangling local branch names containing '/'
- Update sessionIdRef after force-creating a stale session replacement
- Pin core.hooksPath in test makeRepo for reliable rollback tests
- Add test asserting --force-with-lease is used for force pushes
* fix(web-shell): target the worktree for sidebar commits and harden git actions (#7731)
Scope the sidebar commit dialog to the active session's worktree checkout
(matching the composer path) so linked-worktree sessions commit to the right
checkout, guard PR creation against a double-click race, and surface an error
when an invalid branch name is submitted. Adds focused coverage for the branch
picker action wiring and the git branch route validation paths.
* fix(web-shell): address review feedback for git branch picker (#7731)
---------
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com>
Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
`isApproveOutcome` is documented as the single source of truth for "the
user said yes", shared by the CLI scheduler and the ACP Session so the two
cannot drift. It listed every proceed_* outcome except
`proceed_always_server` and `proceed_always_tool`.
Those two are deprecated but live. web-shell's ToolApproval sends them,
and `background-tasks.ts`, `telemetry/tool-call-decision.ts`, the ACP
Session and `agent.ts` all treat them as approvals. This function was the
only place that did not.
Both consumers use it for the same thing: in AUTO mode, when a call fell
back to a manual prompt because denialTracking was armed, an approval
clears the counters so later calls return to classifier flow. A user who
approved with the "always allow for this server" or "for this tool" button
did not clear them, so the session kept prompting after the user had
granted the broadest permission of all.
The test now enumerates the proceed_* values from the enum instead of
hand-listing them -- the hand-written list is what drifted, while claiming
to cover "every proceed_* outcome".
Quote tracking was gated on `substitutionDepth === 0`, so quotes inside a
`$(...)` body were invisible to the parser. A `)` inside those quotes then
closed the substitution early, and the body's own closing quote -- now
seen at depth 0 -- flipped the parser into "in quote" state, which
swallowed every separator to the end of the line.
splitCommands(`echo $(echo ')') ; rm -rf /tmp/pwned`)
-> ["echo $(echo ')') ; rm -rf /tmp/pwned"] one segment
getCommandRoots(same)
-> ["echo"] rm is gone
The trailing command did not merely stay joined to the first, it vanished
from the roots. shell.ts uses these segments to decide which sub-commands
need confirmation, and to locate the git commit / gh pr create segment for
attribution, which silently no-ops on a mis-split.
Gating the closing paren on the surrounding quote state is not enough,
because `"$(...)"` puts a double quote around the whole substitution and
that quote belongs to the outer command. Save the enclosing quote state on
`$(` and restore it on the matching `)`, so each body is quoted
independently of its surroundings.
splitCommands had no direct unit tests; this adds them, including the
nesting and quoting shapes that must keep behaving as they do.
* fix(core): short-circuit the flush wait when the output stream cannot flush
Review follow-up on the settle flush wait:
- A destroyed stream (autoDestroy after an earlier EIO/ENOSPC write
error) or an already-finished one emits neither 'finish' nor 'error',
and .end() on it is a silent no-op — the settle stalled for the full
10s timeout with nothing left to flush, and an abortAll() landing in
that window (/clear, shutdown) would mark the entry cancelled.
Short-circuit before arming the timer. writableFinished, not
writableEnded: the latter is already true mid-flush.
- Hoist clearTimeout into runOnce so the synchronous-throw path no
longer leaves the timer armed to log a misleading flush-timeout
warning for a shell that settled long ago.
- Drop the vacuous duplicate-'finish' assertion (once semantics already
drained the handler; the guard is pinned by the 'error' emit and the
timeout test's late finish) and cover both short-circuit states with
deferred-stream tests asserting the immediate transition and that
.end() is never called.
* test(core): pin the mid-flush wait and the throw-path timer disarm
Closes the two coverage gaps from the verification review on #7905 —
both mutations previously survived the full suite:
- guard mutated to include writableEnded (short-circuit mid-flush, the
exact truncation the flush wait prevents) — killed by a deferred
stream with { writableEnded: true, writableFinished: false }
asserting the transition still waits for 'finish';
- hoisted clearTimeout dropped from runOnce — killed by a throwing
end() plus fake timers asserting no flush-timeout warning fires
after the timeout elapses.
---------
Co-authored-by: ComplexSimply <rudy.arrowsong@gmail.com>
* fix(safe-mode): preserve caller-supplied top-tier MCP servers
Safe mode is meant to distrust LOCAL/ambient state (settings.json,
extensions, project .mcp.json) so a user can isolate which local
customization is misbehaving. It was also unconditionally dropping
topTierMcpServers -- the caller-supplied servers from an ACP
session/new's mcpServers field or --mcp-config -- which are an
explicit, per-invocation argument, not ambient local state.
The real gate turned out to be in packages/core/src/config/config.ts's
Config.getMcpServers() (the accessor mcp-client-manager.ts actually
reads for discovery), not just the mcpServers assembly in
loadCliConfig -- fixed both so the raw field and the accessor agree.
Also guarded the hot-reload path (hot-reload.ts) with the same
bare/safe-mode check, so a live settings.json edit can't smuggle local
servers into an already-running bare/safe-mode session.
Fixes#7819
* docs: clarify safe-mode's MCP servers note distinguishes local vs caller-supplied
Per CONTRIBUTING.md guideline 5 (docs for user-facing changes).
* fix(safe-mode): apply allowedMcpServers to top-tier servers, skip pendingMcpServers I/O under safe mode
Address Copilot's review of PR #7827:
- getMcpServers() now runs the safe-mode top-tier map through the same
allowedMcpServers filter as the non-safe-mode path -- safe mode is
not an exemption from a session's own --allowed-mcp-server-names
upper bound (High severity finding, confirmed real).
- Re-added safeMode to pendingMcpServers' skip condition in
loadCliConfig. Functionally a no-op either way (top-tier servers are
never gated, #4615), but skips getMcpApprovals' local file read
entirely under safe mode instead of doing a no-op read -- safe mode
shouldn't touch local/ambient state at all, not even harmlessly
(Medium severity finding).
* fix(safe-mode): actually run MCP discovery for surviving top-tier servers
Config.getMcpServers() reporting a top-tier server as configured isn't
enough on its own -- something has to actually connect to it and
register its tools with the model. That's a separate gate in
initialize(): startMcpDiscoveryInBackground() was skipped outright
whenever isSafeMode() was true, written back when getMcpServers()
always returned {} under safe mode (so skipping discovery was a
harmless no-op). Left unpatched after the earlier fix, a caller-supplied
top-tier server survives getMcpServers() but is never actually
discovered/connected -- confirmed live against a real ACP session
before this commit: the agent reported the tool as "not configured"
even though Config.getMcpServers() already returned it.
Found by actually running the fix end-to-end against a live ACP
session (qwen --acp --safe-mode + a real stdio MCP fixture server)
instead of relying on unit tests of Config in isolation. After this
commit the same live session correctly discovers and calls the
caller-supplied tool.
Checks getMcpServers() (not topTierMcpServers directly) so the
allowedMcpServers filter still applies -- no discovery is kicked off
if the only top-tier server present is filtered out.
* fix(safe-mode): also run MCP discovery for surviving bare-mode top-tier servers
The discovery-kickoff gate in Config.initialize() special-cased safe
mode (skip only when there's nothing to discover) but left bare mode's
half of the same guard unconditional (!this.getBareMode()), even
though loadCliConfig feeds top-tier MCP servers into bare mode's
mcpServers assembly exactly the way it does safe mode's
topTierMcpServers field. A bare-mode session with a caller-supplied
server (qwen --bare --mcp-config, or ACP session/new under bare mode)
had that server reported as configured by getMcpServers() but never
actually connected/discovered — the same stranded-server regression
already fixed for safe mode in this PR, just the bare-mode twin of it.
Found by an automated review pass on PR #7827 after the safe-mode fix
had already landed. Added the same three-case regression coverage
(present / nothing supplied / filtered out by allowedMcpServers)
mirroring the existing safe-mode tests, using mcpServers (not
topTierMcpServers) since bare mode's "local sources dropped" guarantee
lives entirely in the CLI-layer assembly, not a core-level short-circuit.
* refactor(safe-mode): simplify the bare/safe discovery-gate condition
(!bare || has) && (!safe || has) reduces by distributivity to
!(bare || safe) || has, so factor the has-servers check into a single
hasMcpServers computed once, instead of calling getMcpServers() (which
allocates and filters) twice.
Suggested by an automated review pass on PR #7827, commit 25ddf8b.
No behavior change — same 23 discovery-gate tests pass unmodified.
* fix(safe-mode): stop reading settings.mcp.allowed/excluded under safe mode
allowedMcpServers/excludedMcpServers assembly in loadCliConfig() only
guarded the settings-sourced branch with `!bareMode`, missing `!safeMode`
— so a local settings.json mcp.allowed/excluded list (LOCAL/ambient state,
same category as settings.mcpServers itself, which safe mode already
drops) was still read under safe mode. Combined with getMcpServers()'s own
allowedMcpServers filter (added earlier in this PR for the
--allowed-mcp-server-names case), a settings.json mcp.allowed list
narrower than the caller's own top-tier servers would silently filter
them back out — defeating the guarantee this PR exists to provide, via
the filter's source rather than the mcpServers map directly.
The argv.allowedMcpServerNames branch is unaffected: that's an explicit
per-invocation argument, not local state, so it still applies under safe
mode same as topTierMcpServers itself.
Found by an automated review pass (doudouOUC, CHANGES_REQUESTED) on PR
#7827. Regression test confirmed red before the fix (session-supplied
server silently filtered out) and green after. Full targeted vitest run
(packages/cli config/: 1018 passed, same 3 pre-existing Windows-only
extension-file-watcher failures as before, unrelated), tsc --noEmit,
eslint, prettier --check all clean.
* fix(safe-mode): stop reading settings.mcp.allowed/excluded on hot-reload too
Same class of bug as the previous commit's loadCliConfig fix, found by an
automated review pass on the SAME PR: recomputeMcpGating (hot-reload.ts)
reads settings.merged.mcp.allowed/excluded unconditionally, with no
bare/safe guard of its own. registerMcpHotReload's existing bare/safe
guard only covered the servers map (`next`), not the admission lists
computed right after it — so a live settings.json edit narrowing
mcp.allowed during an already-running safe/bare session would flow
straight into setAllowedMcpServers and silently filter the caller's
top-tier server out of getMcpServers() mid-session. Same stranded-server
outcome as the boot-time bug, reached through the gating list's SOURCE
instead of the mcpServers map.
Fix: under bare/safe mode, skip recomputeMcpGating entirely and build the
gating directly from only the CLI --allowed-mcp-server-names bound
(explicit, per-invocation, not local state — same treatment as
topTierMcpServers itself); excluded/pending are irrelevant once nothing
but the never-gated top-tier servers can be present.
Regression tests (safe mode + bare mode) confirmed red before the fix
(setAllowedMcpServers called with the settings-sourced list) and green
after (called with the CLI bound, undefined here). Full targeted vitest
run (packages/cli config/: 1020 passed, same 3 pre-existing Windows-only
extension-file-watcher failures as before this PR touched anything,
unrelated), tsc --noEmit, eslint, prettier --check all clean.
* fix(safe-mode): stop reading settings.mcp.allowed/excluded on ACP reload too
Third instance of the same bug class found by an automated review pass on
this PR: reloadWorkspaceMcpDiscovery (packages/cli/src/acp-integration/
acpAgent.ts) — the ACP control-endpoint reload path (workspaceMcpReload),
distinct from registerMcpHotReload's settings-file-watcher path fixed in
the previous commit — called assembleMcpServers(settings.merged.mcpServers,
...) and recomputeMcpGating(settings, ...) unconditionally, per live Config
in liveConfigs, with no bare/safe guard. A workspaceMcpReload request
against an already-running safe/bare session would fold local
mcpServers/mcp.allowed/excluded back in, silently stranding or filtering
the caller's own top-tier server mid-session — same outcome as the two
prior fixes, reached through a third independent reload path.
Fix: per-config (liveConfigs holds a Set of potentially differently-moded
Configs — the base config, active session configs, and the discovery
config), skip assembleMcpServers/recomputeMcpGating under bare/safe mode
and build servers/gating directly from that config's own
getTopTierMcpServers()/getCliAllowedMcpServerNames() — same treatment as
the other two fixes.
Regression test (packages/cli/src/acp-integration/acpAgent.test.ts)
confirmed red before the fix (settings-sourced 'local' server leaked
into reinitializeMcpServers alongside the caller's 'probe') and green
after. Also added getBareMode/isSafeMode mocks (defaulting false) to the
two pre-existing Config-shaped mocks in this describe block that didn't
have them — reloadWorkspaceMcpDiscovery now calls these unconditionally
per config, which would otherwise throw "not a function" against any
mock missing them, even for a normal-mode test. Full targeted vitest run
(packages/cli/src/acp-integration/acpAgent.test.ts: 318 passed), tsc
--noEmit, eslint, prettier --check all clean.
* test(safe-mode): add bare-mode counterpart for the workspaceMcpReload guard
Suggested by an automated review pass on PR #7827: the previous commit's
regression test for reloadWorkspaceMcpDiscovery only exercised
isSafeMode: true, leaving the bare-mode half of
config.getBareMode() || config.isSafeMode() unverified at this layer —
unlike the hot-reload.ts tests, which already cover both modes for both
the servers map and the admission lists. A future change narrowing that
guard to isSafeMode() only would go undetected here.
Confirmed red before the fix (temporarily reverted acpAgent.ts to the
prior commit): settings-sourced 'local' leaked into reinitializeMcpServers
alongside the caller's 'probe', same as the safe-mode case. Green with
the fix restored. Full targeted vitest run (acp-integration/acpAgent.test.ts:
319 passed), tsc --noEmit, eslint, prettier --check all clean.
The Anthropic request previously used 3 of the 4 allowed cache_control
breakpoints: last tool, end of system, last user message. Because the
system prompt ends with volatile tails (git status, auto-memory), the
scope:'global' entry on the system end almost never hit across sessions
— only the tools breakpoint did.
Split the outgoing system prompt at the stable → context / volatile
layer boundary that GeminiClient already assembles: the client records
the gitStatus/autoMemory-free prefix on Config, and the converter emits
it as its own text block with an early breakpoint (scope:'global' when
enabled), demoting the system-end breakpoint to the per-session shape.
New sessions and in-session memory saves now re-bill only the small
volatile tail instead of the whole system prompt. The converter matches
the prefix via startsWith and fails open to the previous single-block
layout (subagent prompts, stale prefix), so the wire shape never
regresses. This fills all 4 breakpoints.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* fix(core): retry on transient network errors in defaultShouldRetry and API-call predicate
The retry classification layer (classifyRetryError) already recognizes
transport errors (ECONNRESET, ETIMEDOUT, etc.) as retryable, but neither
defaultShouldRetry nor the inline shouldRetryOnError in geminiChat.ts
consulted the classification for transport codes. TCP-level errors carry
no HTTP status, so they fell through every predicate and propagated as
raw [API Error: terminated (cause: read ECONNRESET)].
Wire the existing classifyRetryError transport detection into both
retry decision points:
1. defaultShouldRetry in retry.ts — append a transport-kind check after
the existing rate-limit / 5xx predicates, preserving all current
behavior (including bounded retry for fail-fast quota 429s).
2. Inline shouldRetryOnError in geminiChat.ts makeApiCallAndProcessStream —
add the same transport-kind check so the API-call-level retryWithBackoff
wrapper covers network errors.
The stream-level transport retry (gated on !streamYieldedChunk) already
exists and is unchanged.
Closes#7831
* fix(core): classify SDK-wrapped transport errors nested in the cause chain (#7898)
---------
Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
`Part.thought` is a boolean flag and the reasoning lives in `part.text`.
`partToString` declared it as `thought?: string` in a local cast and
interpolated the flag, so every thought part rendered verbose as the
literal `[Thought: true]` and the reasoning was dropped.
The local cast was the only thing making that compile -- the SDK types it
as `thought?: boolean`, `createOpenAIReasoningThoughtPart` builds
`{ text, thought: true }`, and every other consumer reads the text and
tests the flag for truthiness. Drop the cast and follow the same shape.
Testing `!== undefined` also caught a part carrying `thought: false`,
which is an ordinary part: it rendered as `[Thought: false]` rather than
as its own text. Use truthiness, matching the rest of the codebase.
Two existing tests asserted the old output and are corrected. Both were
built from shapes the SDK never emits: `{ thought: 'thinking' }`, which
only type-checks through `as unknown as Part`, and a bare `{ thought:
true }` expected to print its own flag.
* fix(core): apply maxDepth to flat-format memory imports
* fix(core): track import depth per file so a shallower route can re-expand
* docs(core): trim the historical narrative from the depth-guard comment
Per AGENTS.md comments default to none and exist to explain a non-obvious
why. The first paragraph does that -- it states which semantics the guard
matches. The account of what the old code did wrong belongs in the commit
message, where it already is, not inline where it can go stale.
* fix(core): name the file in the depth-limit warning, correct a stale comment
The summary comment still said the flat path deduplicates with a set; it
has used a depth-keyed map since the previous commit, and the whole point
of that map is that a shallower route may re-expand a file a set would
have dismissed.
memoryDiscovery processes memory files concurrently, so several chains can
hit the limit at once and every warning read identically. normalizedPath
is already in scope.
* fix(core): announce a re-expanded flat import only on its first visit
Letting a shallower route re-expand a file made onFileImported fire twice for
it: once from the truncated deep route and again from the shallow one, with a
different parentFilePath. The Set this replaced skipped the child outright, so
the callback ran once per file. Nothing downstream de-duplicates --
notifyInstructionsLoaded in memoryDiscovery forwards every call to the
consumer -- so one loaded file was reported as two.
The file itself is still emitted into the flat output once, so gate the
notification on the first visit to match. The test is the re-expansion fixture
with a callback attached: x.md was announced twice before this commit.
* fix(core): budget flat imports from the depth already spent
The flat path started its own counter at 0 and ignored importState.currentDepth,
so a caller entering with budget already spent got the whole limit over again
in flat mode while tree mode gave it only what was left. Now that flat enforces
the limit at all, the two counters have to share an origin.
Every caller today enters flat mode at depth 0 -- memoryDiscovery passes 0, and
the internal recursion at the tree branch is unreachable from flat -- so this is
a no-op in practice; it removes the discrepancy rather than any live symptom.
Test enters at currentDepth 2 of maxDepth 3 and asserts both formats stop at the
same file. Before this commit flat expanded [0,1,2] where tree gave [0].
Step 3 rewrites a numeric exclusiveMinimum/exclusiveMaximum into the
Draft 4 boolean form, but step 6 then skipped both keys unconditionally.
A schema that already used the boolean form lost the flag, silently
relaxing "> 10" into ">= 10", and the function stopped being idempotent
- feeding its own output back in dropped the flag it had just written.
Skip the key only when the value is a number, the one form step 3
consumes. MCP servers declaring draft-04 send the boolean form directly,
so this reached the wire through the OpenAI and Anthropic converters.
* fix(core): read the stash reflog from the common git dir
* fix(core): harden the commondir read the same way the HEAD read is
`resolveCommonGitDir` reads `<gitDir>/commondir` with a plain `readFile`,
while `gitDirect.ts` reads that same file through `readFirstLineNoFollow`,
which opens with `O_NOFOLLOW | O_NONBLOCK` and bounds the read. The two
reads had different protections against the same hostile input.
That matters because `getGitWorkingTreeStatus` polls unattended: a
`commondir` named pipe planted in a cloned repository would hang the
`readFile` forever and pin a libuv thread-pool slot, and a symlink would
be followed out of the repository. The adjacent `countStashEntries`
already guards against exactly this hazard for the reflog it reads.
Move `readFirstLineNoFollow` to `gitUtils.ts` and point both callers at
it. It cannot be exported from `gitDirect.ts` because `gitDirect.ts`
already imports `gitDiff.ts` — that would close an import cycle.
`gitUtils.ts` imports nothing but node builtins, so it can be shared by
both.
`previewChars` is meant to bound the preview that goes into the model's
context, but two fixed strings were emitted outside it.
The separator is 39 characters and was appended whatever the budget was,
while the head budget never subtracted it -- only the tail budget did. And
when fewer than three characters remained, the 3-character ellipsis marking
a cut line was still emitted. Together:
previewChars=0 -> 39 characters
previewChars=10 -> 42
previewChars=40 -> 47
Take the separator out of the budget before splitting head and tail, drop
it entirely when the budget cannot fit it alongside any content (the
wrapper message above already says the output was truncated), and skip the
ellipsis when there is no room for it either.
Reachable through truncateToolOutput's per-tool limits.previewChars.
Sweeping previewChars from 0 to 260 across all three keep directions now
reports nothing over budget; before, every value below ~47 was.
`getErrorMessage` truncates to MAX_STRINGIFIED_ERROR_MESSAGE_LENGTH on
every branch except one: an `instanceof Error` with no distinct cause
returned `error.message` untouched.
That made the cap depend on details unrelated to length. The same 5000
character string comes back at 1000 as `{ message }`, at 1000 once a
distinct `cause` is attached, at 1000 through the JSON.stringify path --
and at 5000 as a bare `Error`. The uncapped shape is exactly the one a
provider SDK throws when it packs a whole response body into the message,
and the result flows into `llmContent` at dozens of call sites, which is
what the cap exists to prevent.
Both external search strategies pass the user's pattern as a positional
argument, so a pattern that begins with a dash is parsed as an option.
`git grep --untracked -n -z -E --ignore-case '-n'` consumes the `-n` as a
flag and exits with `fatal: no pattern given`. That failure is caught and
falls through to the system grep, which has the same flaw with a much
quieter symptom: `grep -r -n -H -E --null '-n' .` treats `-n` as a flag
and promotes the `.` search path to be the pattern, so it matches every
line of every file and hands the whole tree back as if it were the result.
`validateToolParams` accepts these patterns -- `new RegExp('-n')` is a
valid regex -- so nothing upstream rejects them.
Introduce the pattern with `-e` in both strategies. `-e` is POSIX, and the
search path operand stays the literal `.`, so no `--` separator is needed.
The sibling ripgrep tool already does this via `--regexp`.
isSafeCombinedFlagArg documents that only forms with i last are safe,
because sed treats everything after -i in a combined flag as the backup
suffix. The check tested flags.startsWith('i'), which rejects i first
rather than i anywhere but last, so -Eir and -riE were accepted.
Real sed reads those as -E plus in-place with the backup suffix r / E and
writes an f.txtr or f.txtE alongside the edit. The parser reported them
safe, shell.ts intercepted the command, and the simulation wrote the file
without ever creating the backup the user asked for - the same hazard
-i.bak is already rejected for.
Test the documented rule instead. A declined command falls through to
real sed, so the cost is a lost fast path rather than a lost backup.
Two classes wrote '-' mid-class, where it is a range operator, and both
described something other than what was intended.
[*-+] was the range U+002A-U+002B, exactly {*, +}, so '- item' was not
seen as a list item and never reset tracking - a normal bulleted list
accumulated until it tripped the repetition check and halted a healthy
response.
[+-_=*] was the range U+002B-U+005F, every digit and every uppercase
letter, so 'SELECT', '12345' and '>>>' read as horizontal rules. A
divider also returns early, so that content left the history entirely
and a model chanting such a token could never be detected.
Place '-' first in both classes. The box-drawing span stays a range.
The SOCKS guard tested /^socks[45]?:\/\//, which misses socks5h:// and
socks4a:// - the hostname-resolving variants that curl, proxychains and
most tunnels emit. Those fell through to the http:// prefix and produced
'http://socks5h://127.0.0.1:1080', which parses as the host 'socks5h' on
port 80, so requests failed with ENOTFOUND for a nonexistent host and
nothing mentioned SOCKS.
Match the whole socks family instead. A scheme starting with 'socks' is
always a SOCKS proxy, and the pattern still requires '://' so a hostname
like socks.example.com is unaffected.
* feat: gate session writer lease behind opt-in
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(acp): freeze session writer lease per process
Snapshot the effective restart-required lease gate from the bootstrap Config and reuse it for every session Config in the ACP process.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(core): align recorder default lease gate
Use the effective session writer lease gate when ChatRecordingService is constructed without an explicit writer mode.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(core): fast-fail permanent quota-exhaustion 429s instead of silent retry
A 429 whose body signals a permanently exhausted quota — one that
carries a reset time, e.g. 'quota has been exhausted ... will reset at
...' — is a non-retryable condition. qwen-code classified it as a
transient rate-limit, retried silently through the full retry budget,
and surfaced no error, so the session appeared to hang with no output.
Detect such errors and fast-fail on the first attempt, surfacing a
friendly message that preserves the provider's reset time and points at
switching API key / auth. Transient 429s without a reset time still
retry as before.
- quotaErrorDetection: isQuotaExhaustedError + formatQuotaExhaustedMessage
- retry: fast-fail block (mirrors the existing Qwen-OAuth quota path)
- errorParsing: surface the friendly message verbatim, no [API Error:] wrap
* test(core): cover ApiError quota exhaustion
* fix(core): preserve quota error status
* fix(core): remove status from quota fast-fail, unwrap JSON error bodies (#7842)
- Remove .status=429 from the thrown quota-exhaustion error: it made
isRateLimitError() return true, re-triggering the stream-side retry
loop in geminiChat.ts (the exact bug this PR fixes). Use cause to
preserve the original error for diagnostics instead.
- Unwrap JSON error bodies in getQuotaMessage so openai-compatible
providers that emit 429s as JSON surface the human-readable message
instead of raw JSON.
- Add idempotency guard to formatQuotaExhaustedMessage.
- Pin all three detection clauses with negative fixtures, add
isRateLimitError(thrown)===false regression test, move quota tests
out of the Qwen OAuth describe block.
- Update stale isAlreadyFormatted JSDoc; simplify single-element
it.each to plain it.
* fix(core): fast-fail mid-stream quota exhaustion before rate-limit retry (#7842)
---------
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
* feat(core): add full-resolution image zoom tool
* chore(vscode): refresh third-party notices
* fix(ui): satisfy zoom image display drift checks
* fix(web-shell): translate zoom image tool
* fix(core): wire sharp into packaging and load it lazily (#7809)
Externalize sharp in esbuild and declare it (plus the @img platform
binaries) in the published package's optionalDependencies so an
npm-installed CLI resolves the native binding. Import sharp dynamically
inside execute() so a missing binding returns a bounded tool error
instead of crashing startup during strict tool warmup, and so the module
is only loaded when zoom_image actually runs. Also pin the EXIF
auto-orientation test with a discriminating fixture and cover the
.qwenignore and y1>=y2 validation paths.
* fix(core): gate zoom_image at execute time and cap tiny-crop upscale (#7809)
Register zoom_image unconditionally and move the image-modality check to
execute time so first-run sessions and hot /model switches resolve the
tool without re-running initialize(). Cap magnification at 8x so a tiny
crop no longer inflates to the full image-token budget. Drop the redundant
@img/* platform pins; sharp's own optionalDependencies install the matching
binary for each OS/arch.
* fix(core): emit zoom_image file telemetry and cover guard branches (#7809)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: qwen-code-autofix <qwen-code-autofix@users.noreply.github.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
executeBackground's settle handler called outputStream.end() and
transitioned the registry immediately. stream.end() is asynchronous —
pending writes can still be in the libuv queue when it returns — so
/tasks consumers and the status sidecar could observe a terminal status
and read the output file before the trailing bytes were on disk,
producing truncated logs.
The promote settle path already handled this (wait for 'finish' with
'error' and a 10s timeout as fallbacks). Extract that logic into a
shared endStreamThenSettle() helper and use it from both settle paths
so they cannot drift.
Covered by deterministic deferred-stream unit tests asserting the
registry transition is held until 'finish' (and still happens on
'error' or timeout), since the race does not reproduce reliably under
natural timing.
Follow-up ① from the #7669 verification report.
Co-authored-by: ComplexSimply <rudy.arrowsong@gmail.com>
* fix(web-shell): render background notifications as system messages
* fix(web-shell): use basic table rendering by default
---------
Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>
* feat(review): add script-lint — deterministic linters over a diff's executable scripts
A diff's shell — a `.sh` file, a Makefile recipe, a Dockerfile `RUN`, a GitHub
Actions `run:` block — is code, and its bugs (an unquoted `$x` that word-splits,
a `${PIPESTATUS[1]}` read after the array was reset) are the class a reviewer
misses by reading a long YAML and catches by running the checker. Measured: a
model told in prose to "run the workflow scripts" reads instead (0/4 executed).
So the execution is a command, not a request. `qwen review script-lint` reads the
plan, dispatches shellcheck / actionlint / hadolint by file type over the changed
executable files, filters every finding to whether its line is one the diff
changed (`inDiff`), and reports JSON. A linter that is not installed is disclosed
as skipped, never a clean bill. It is not GitHub-specific — shellcheck applies to
shell wherever it appears; actionlint/hadolint are front-ends for two embeds.
This is the command only; the agent, roster requirement and coverage gate that
make it a non-skippable step follow.
* feat(review): make script-lint a required, coverage-gated review step
The script-lint command exists; nothing ran it. This wires it into the review
the way build-test is wired, so a diff that changes an executable script cannot
be certified without its linters having run.
- A `script-lint` agent role (agent-briefs): reads no diff, runs the command,
reports from its JSON. `inDiff` findings are the PR's; `skipped` (a linter not
installed) is disclosed as unreviewed, never clean. Rules are not injected into
it, same as Build & Test — it reports a tool's verdict, not a read.
- agent-prompt welds the exact `qwen review script-lint --plan/--worktree/--out`
into that agent's brief with absolute paths, guarding an absent PR number out
of the --out name — the same treatment, and the same traps avoided, as the
build-test block it sits beside.
- The roster requires the agent whenever the diff carries a file a linter owns by
path (a `.sh`/`.bash`, a `.github/workflows/*`, a Dockerfile) and the review has
a worktree to lint in. Detected by the command's own `pathTool`, so the roster
and the command cannot disagree about what counts. A pure-TS diff does not
require it; a diff-only review (no tree) cannot run it, so does not.
- Coverage needs no new code: it derives missing roles from the roster generically
(`BRIEFS[role].label`), so a required script-lint agent that did not run exit-3s
check-coverage like any other. The role carries its three labels for that.
End-to-end on a crafted diff (a workflow plus a `deploy.sh` with `rm -rf $TARGET`):
the roster requires script-lint, and the command blocks on the SC2086 on the
changed line while disclosing the workflow as skipped where actionlint is absent.
SKILL.md documents the step; tests cover the roster requirement, the brief weld,
and the subcommand registration.
* fix(review): harden script-lint after review — context lines, fail-closed, symlinks, quoting
Addresses the review findings on this PR:
- inDiff was keyed off the plan's hunk ranges, which include git's three context
lines, so a pre-existing diagnostic near a real change was marked this PR's and
could block it. Classify off the diff's added-line ranges (context excluded),
parsed from the diff; fall back to the plan hunks only when the diff is absent.
- The command wrote the report to --out and printed only "Wrote ...", while the
agent's brief (and the roster's generated command, which passes --out) says to
read the JSON it prints. Match build-test: write the file AND always print JSON.
- runTool failed open — every non-ENOENT failure (EACCES, a signal, maxBuffer, an
unexpected status) fell through as empty stdout and became ok:true. Fail closed:
such a run is `errored`, which forces ok:false; ENOENT alone stays "not installed".
- The shebang read slurped the whole file and followed symlinks — a changed
`hang.sh` -> /dev/zero would hang the reviewer. Read only regular files (lstat,
no follow) and only the first block.
- The welded command interpolated plan/worktree/out as bare words; a worktree path
with a space would split. Quote them with shellQuotePath.
- Harden the checker environment: shellcheck --norc + drop SHELLCHECK_OPTS, so a
PR-controlled .shellcheckrc or inherited opts cannot suppress SC2086.
- The roster required the agent for a pure-deletion .sh (a mandatory no-op); gate
on added lines. Drop the Makefile-recipe claim (no detector backs it). Fix the
`ok` JSDoc (info blocks too, not just error/warning).
- Tests: inject the tool runner (no binary needed) to cover actionlint/hadolint
normalisation, the three fail-closed paths, and the context-line classification.
* feat(review): make script-lint a deterministic gate — orchestrator runs it, compose-review is the authority
The review of #7749 landed three architectural findings (#10/#11/#12): the
executable-script lint was run by an AGENT, so its execution rested on the model's
honor system, the model decided each finding's severity, and an uninstalled checker
was only disclosed in prose. All three are the exact "a rule a model is asked to
remember will eventually not be remembered" trap the feature exists to close — and a
measurement on PR #7724 confirmed it: the strongest model's Step-3 agents, given the
diff and the worktree, missed all three execution-confirmed bugs, one attacker
persona walking into a double-execute and declaring it correct.
So take the model out of the gate entirely:
- The orchestrator runs `qwen review script-lint` as a deterministic step (like
presubmit) and writes the report next to the plan. No agent.
- `compose-review` derives the report path from the plan and reads it as the SOLE
authority: a finding on a changed line above `style` is a pre-confirmed `[lint]`
Critical (needs no verifier — the tool already ran); an uninstalled or crashed
checker is unreviewed scope that caps a would-be Approve; and — proof it ran — a
diff that carries an executable script but produced no readable report is itself
unreviewed (fail closed). A diff-only review, which has no worktree to run it,
is exempt. Nothing here comes from the input JSON a model wrote.
Removed the `script-lint` agent role (agent-briefs, agent-prompt weld, roster
requirement) and its SKILL.md agent entry; the roster's `hasExecutableScript` is now
exported as the shared predicate the gate reads. The `script-lint` command itself is
unchanged — it was always the deterministic engine; only its trigger and consumer moved.
* fix(review): address the second review pass on script-lint
- parseFindings failed OPEN on non-empty unparseable output (a version skew or a
deprecation line before the JSON) → `[]` → recorded as a clean `checked` file.
Return null on that path and push it to `errored`, fail-closed like a bad exit.
- hadolint had no config isolation while shellcheck gets `--norc`; a PR-controlled
`.hadolint.yaml` could `ignored:` its findings away. Add `--no-config`.
- pathTool recognised only `.sh`/`.bash`, but the shebang regex matches four shells
— a `.ksh`/`.dash` file never required the lint. Add those extensions so the
gate's owed-predicate and the command cannot disagree.
- Pin `r.ok` in the not-installed test, and add a mock test for the unparseable path.
* fix(review): address the deterministic-gate re-review — real correctness bugs on the write path
The second review pass on the gate found real bugs, including one this PR
introduced. Fixed:
- hadolint has no `--no-config` flag — a prior round added it, so hadolint exited
2 (usage error) on EVERY Dockerfile and reported it `errored`. Reverted to the
plain working invocation; hadolint config isolation needs a verified mechanism
and is tracked separately.
- actionlint's JSON anchors each diagnostic at the `run:` key line (not the
changed shell line) and flattens ShellCheck severity, so a style nit read as a
blocking `error` and a real finding read as pre-existing. Until that source
mapping is parsed and verified, a workflow is deferred to `skipped` (unreviewed)
and actionlint is never run — shellcheck still covers standalone shell.
- A recognised path that `firstLineOf` refused (a `hook.sh` symlink, a fifo) was
silently dropped from the report, so an empty report read as clean. It is now
recorded as `skipped`. `firstLineOf` distinguishes a true deletion (skip) from
an irregular file (record).
- The gate's owed-predicate excluded any file with zero added lines, so a
deletion-only edit that breaks a surviving `.sh` was treated as not-owed. Keyed
on the post-image (`fileLines`) now: a surviving script is owed, only a true
deletion is exempt.
- compose-review trusted any body-Critical string containing `[lint]` as
deterministic — a model-written or injected claim could launder itself past
verification. Provenance decides now: only `scriptLintGate`'s own findings are
deterministic (tracked by count); `[lint]` is no longer a trusted tag.
- A stale report from an earlier review of an older commit could certify the
current one if the lint step were skipped. The report records its `headSha`
(`git rev-parse HEAD`); compose-review rejects it as stale when it disagrees
with the plan's `fetchedSha`.
* fix(review): third gate re-review — read the report before the owed-predicate, fail closed on all paths
- The gate returned early on `hasExecutableScript` (path-only), so a shebang
script the command DID lint (`.husky/pre-commit`, detected by `#!`) had its
findings dropped — the gate never read the report. Read the report first; the
path-predicate now only gates the no-report fail-closed case. Findings from any
script the report names are processed.
- The staleness guard was a no-op when `report.headSha` was absent (git
unreadable): `planSha && report.headSha && …` short-circuited, so an
unverifiable report certified new code. It now fails closed on a missing headSha
when the plan names a commit.
- The plan-parse catch failed OPEN (returned "nothing owed") while every other
path fails closed. It now discloses "could not read the plan" as unreviewed.
- hadolint had no config isolation. Point `HADOLINT_CONFIG` at an empty file so a
PR-added `.hadolint.yaml` cannot suppress findings — an env var, benign if the
tool ignores it (unlike the invalid `--no-config` flag from the last round).
- `buildNote` hardcoded "not installed" for every skipped file; `skipped` now
mixes reasons (missing tool, irregular file, deferred checker), so it summarises
by tool and leaves the specific reason on each entry.
* fix(review): R3 — deferred (non-capping) actionlint, local-flow staleness, disclosure
- actionlint deferral capped EVERY workflow-touching PR (≈15% of merges): a
deferred workflow went to `skipped` → unreviewed → no Approve, on a checker
present but deliberately not run, which "install the tool" cannot fix. Split a
third `deferred` state — disclosed in the note, but the gate never reads it, so
it does not cap. `skipped`/`errored` still cap.
- The staleness guard was PR-only: `fetchedSha` is written by fetch-pr, not
capture-local, so a local review short-circuited the check and a stale local
report could certify new code — the exact fail-open, in the local flow.
capture-local now records the local HEAD as `fetchedSha`.
- The local path was armed (reviewMode `local`) but SKILL's `--worktree` was
unfillable there. SKILL now covers a local review (worktree = the project root)
and the derived report name.
- Nits: the skipped/irregular reasons no longer lead with the path (the gate
prefixes it — was printing it twice); merged the two `./lib/roster.js` imports.
- Tests: deferred does not cap; the staleness guard protects a local review too.
* fix(review): R4 — disclose deferred lint without capping; bind report freshness to diff content
B1 — the deferred checker was silent. R3 split a third `deferred` state so a
workflow's embedded shell (actionlint, whose source-mapping this env can't
verify) no longer caps the verdict — but the disclosure half was never wired:
`scriptLintGate` returned only `{criticals, unreviewed}`, so a workflow-only PR
went quiet on a file no checker examined. That is #10's original complaint
("only disclosed in prose") returning as disclosed nowhere. Add a third channel:
the gate now returns `disclosed`, populated from `report.deferred`, and
`composeReview` renders it in the body on every verdict — including Approve —
without pushing it into `cappedBy`. SKILL.md's contract paragraph now documents
the deferred state alongside unreviewed scope.
B2 — the staleness guard keyed on HEAD. A local review is defined by uncommitted
work, so `git rev-parse HEAD` is not a content identity for what it reviews: two
reviews at the same HEAD with different working-tree content shared a `fetchedSha`
and a stale clean report could certify broken code. Bind freshness to the diff's
content instead: `runScriptLint` stamps the report with a sha256 of the captured
diff (`diffHashOf`), and the gate re-hashes the plan's current diff and rejects a
mismatch. Correct for a PR (a later commit → a different diff) and for local
uncommitted work alike. `capture-local`'s `fetchedSha` plumbing is removed.
Tests: the DEFERRED-only case now asserts the disclosure is present and
non-capping (it previously pinned the silence). New composeReview gate tests kill
the surviving mutants — the gate critical stands with no verifier (provenance,
not the `[lint]` marker), and an errored checker caps a would-be Approve to
Comment. New runScriptLint tests pin the diff-hash stamp. 972 review tests,
ESLint --max-warnings 0, tsc, Prettier clean.
* fix(review): R5 — freshness fails closed when neither side has a hash; pin config isolation
The staleness guard was `report.diffHash !== planDiffHash`. When the plan names no
readable diff AND the report carries no hash, both are `undefined` and
`undefined !== undefined` is false — so the guard did not fire and an arbitrary
hashless report was accepted as this review's, its findings promoted to `[lint]`
Criticals. Every other branch of this gate fails closed; this one inverted, on the
exact unverifiable case its own comment claimed to reject. Fixed with `!planDiffHash
|| …`, and the stale `headShaOf` JSDoc left stacked above `diffHashOf` is removed.
The gate's happy-path tests were green *because* of that hole: `writePlan`/
`writeReport` (and the composeReview `gateReadyPlan`/`writeGateReport`) wrote no
diff and no hash, so every test that didn't override them ran through the fail-open
branch — proving the gate works on an *unverifiable* report, not a fresh one. The
fixtures are now FRESH by default: a captured diff exists (`DIFF` is a real file;
coverage only string-matches it, so this is transparent to it) and both plan and
report bind to its hash. A freshness test overrides one side to model staleness, and
a new test pins the both-undefined case directly.
Also closes the last mutation gap R5 flagged three rounds running: the config
isolation (`--norc`, `SHELLCHECK_OPTS` scrub, `HADOLINT_CONFIG` → empty file) is a
security property — a PR-added linter config must not suppress the finding the gate
blocks on — but it lived inside `runTool`, behind the spawn an injected runner
bypasses. Extracted `buildToolInvocation` (pure argv + env) so all three defences
are asserted without a binary; each was hand-mutated to confirm its test fails when
the defence is deleted.
976 review tests, ESLint --max-warnings 0, tsc, Prettier clean.
* test(review): pin the plan-parse disclosure — the last surviving gate mutant
`scriptLintGate` on an unreadable plan fails closed and pushes its own reason into
`unreviewed`. The coverage machinery already caps an unreadable plan, so the verdict
and its cap are identical with or without this line — what it loses when deleted is
the specific "could not read the plan" sentence in the body. That made it a
disclosure guarantee with no test, and the one gate mutant left standing after five
rounds. Pin it directly on the gate. 977 review tests, ESLint/tsc/Prettier clean.
* fix(review): harden script-lint tmp-file + spawn, leak-proof the tests (R8 suggestions)
Four suggestions from the re-review, all on script-lint:
- Symlink race on the hadolint empty-config: it was written to a fixed
`tmpdir()/qwen-review-hadolint-empty.yaml`, so on a shared runner a pre-planted
symlink there would have `writeFileSync` follow it and truncate the target. Write
it inside a fresh `mkdtempSync` directory instead — a 0700 dir with a random
suffix that cannot pre-exist, so the write is safe and the path unpredictable.
- `runTool`'s `spawnSync` had no `timeout`, unlike the sibling runners in
build-test.ts and test-efficacy.ts: a crafted script that hangs a linter would
block the review until the outer CI job timeout reclaimed the runner. Added a
120s bound; a timeout kills with SIGTERM, which the existing `r.signal` branch
already turns into a fail-closed error.
- Two test-cleanup leaks: script-lint.test.ts's symlink test made a second temp
dir cleaned only by an inline `rmSync` a failing assertion would skip, and
script-lint.mock.test.ts used inline `fresh()`/`clean()` with no hook. Both now
tear down in `afterEach`, which runs even when a test throws.
977 review tests, ESLint --max-warnings 0, tsc, Prettier clean.
* fix(review): hadolint isolation fails closed, not to a plantable path (R8)
R8 caught that the previous commit's fallback reopened the vector it closed. When
`mkdtempSync` failed, `emptyHadolintConfig` returned a FIXED
`tmpdir()/qwen-review-hadolint-none/x`. That path is a config hadolint *reads*, so an
attacker who plants an `ignored:` file there gets those ignores honoured — the
opposite threat direction from the write-truncation the mkdtemp move fixed. There is
now no predictable fallback: on failure `emptyHadolintConfig` returns `undefined`, the
env var is set only for a hadolint run and only when a private config exists, and
`runTool` fails the hadolint run CLOSED (errored) rather than lint against a config it
cannot vouch for.
Also from R8:
- The timeout comment claimed the `r.signal` branch handles a timeout. On Node a
timeout sets BOTH `r.error` (ETIMEDOUT) and `r.signal` (SIGTERM), and `r.error` is
checked first — so it is reported through the error branch. Comment corrected; still
fail-closed either way.
- Both hardening changes were untested. `buildToolInvocation` now returns `timeoutMs`
so the bound is one asserted value, and two tests pin it: HADOLINT_CONFIG is a fresh
0700 mkdtemp path (not the old fixed name), and the timeout is 120s. Each fails under
the corresponding mutation.
- The mkdtemp dir is swept at process exit, so the fixed-file-to-per-run-dir change
does not leak into the OS tmpdir.
979 review tests, ESLint --max-warnings 0, tsc, Prettier clean.
* test(review): pin the hadolint fail-closed guard in its own file (R9)
The `if (tool === 'hadolint' && !emptyHadolintConfig())` guard is what the previous
commit exists to add, yet deleting it left the suite green: `emptyHadolintConfig`
caches at module scope, so by the time any test in script-lint.mock.test.ts runs the
cache is warm and the failure path is unreachable from that file. A dedicated file
gets a fresh module registry — it points TMPDIR at a path that does not exist before
importing, so the first `emptyHadolintConfig()` call fails to mkdtemp and the guard
fires. Two tests, split so a mutation attributes cleanly: (1) buildToolInvocation
sets no HADOLINT_CONFIG (the env logic, holds either way); (2) runScriptLint errors a
Dockerfile closed rather than lint it unisolated (the guard). Deleting the guard
reddens (2) and leaves (1) green.
Also from R9: register the process-exit cleanup right after mkdtempSync, before the
write, so a writeFileSync that throws still leaves the temp dir swept, not leaked.
981 review tests, ESLint --max-warnings 0, tsc, Prettier clean.
* fix(review): isolate hadolint via --config, not the ignored HADOLINT_CONFIG env (R10)
Verified against real hadolint 2.14.0: the binary does not read `HADOLINT_CONFIG`
(its `strings` list it among no `HADOLINT_*` config vars; `-V` reports "No
configuration was specified"). It reads `--config`, then a `.hadolint.yaml` in the
process CWD, then XDG. Because a local review runs with `--worktree .`, the linter
ran inside the reviewed tree and honoured the diff's OWN `.hadolint.yaml` — so the
env-based "isolation" was a silent no-op and a PR could suppress its own Dockerfile
findings (a DL3018 `ignored:` made the finding vanish with nothing disclosed).
Isolate on the channel hadolint actually reads: pass `--config <private neutral
file>`, which overrides both the cwd and XDG configs. The config content is now
`ignored: []` rather than empty — `--config` rejects an empty file ("empty YAML
stream"). The mkdtemp 0700 dir and the fail-closed guard are kept (the config is a
path hadolint reads, so both still matter); the no-op `HADOLINT_CONFIG` /
`HADOLINT_NO_COLOR` env vars are dropped.
Tests updated to assert the real channel: hadolint's argv carries `--config` at a
fresh 0700 mkdtemp path holding `ignored: []`, and the isolation-unavailable path
adds no `--config` (and still fails closed). shellcheck's `--norc` /
`SHELLCHECK_OPTS` isolation is unchanged — it was verified sound against the real
binary. 981 review tests, ESLint/tsc/Prettier clean.
* fix(review): address maintainer review — worktree containment, hunk-fallback false positive, zsh/shebang docs
From @yiliang114's review of the script-lint gate:
- P1 shebang/zsh: `toolFor` intentionally omits zsh (and fish) — shellcheck refuses
both (`SC1071: ShellCheck only supports sh/bash/dash/ksh`), so routing a
`#!/usr/bin/env zsh` hook to it would make every zsh file a bogus SC1071 `[lint]`
Critical on its shebang. Documented the deliberate exclusion rather than adding zsh.
- P1 hunk fallback: `inDiff` fell back to the plan's context-inclusive hunks whenever
`addedRanges.get(path)` missed — including when the diff parsed fine but a path was
unmatched, which could promote a pre-existing finding on a context line to a
blocker. Now a parsed diff that does not mention a path yields `[]` (nothing added
→ nothing blocks); the context-inclusive `hunksOf` fallback is used only when NO
diff parsed at all (a report the freshness guard already rejects as stale), and that
degraded path now logs a warning.
- P2 worktree containment: `resolve(join(worktree, path))` now refuses a path that
escapes the worktree (`../../etc/passwd`), disclosing it as skipped rather than
stat-ing/linting a file outside the reviewed tree.
- P2 roster/gate shebang gap: made explicit in the gate's no-report branch that a
shebang-only script (which `hasExecutableScript` cannot see) is covered by the
always-run contract, not the `owed` predicate.
Two items left as tracked follow-ups per the reviewer's own framing (both flagged
low-priority / design): an aggregate cross-file time budget (the per-file 120s bound
is the main defense), and per-severity finding classification (the v1 all-or-nothing
stance is documented and defensible). 983 review tests, ESLint/tsc/Prettier clean.
* fix(review): harden the script-lint gate against the adversarial review round
From the Codex GPT-5 /review pass. Five fixed:
- Provenance by IDENTITY, not count (compose-review): the gate's `[lint]` criticals
are now tracked as a separate list rather than pushed into `bodyCriticals` and
removed by a count subtraction. The old `(filtered) − gateCount` misfired when a
model claim carried a `[build]/[test]/[probe]` tag (filtered out before the
subtract) or a gate finding's own text contained one — erasing an unrelated
claim's verification requirement. Model criticals alone decide the verify count.
- Distinguish unknown size from deletion (roster): `fileLines: 0` is a real
post-image count only in pr-worktree; in local/diff-only the report builder writes
0 for EVERY file, so keying deletion on it read a surviving `deploy.sh` as deleted
and let a missing report pass uncapped. `hasExecutableScript` now trusts
`fileLines` only in pr-worktree and owes any path-detected script otherwise.
- Hash the bytes used for range mapping (script-lint): the diff was read twice —
ranges before the linters, hash after — a TOCTOU window where a concurrent
recapture could pair snapshot A's ranges with snapshot B's hash. Read the diff
once into one buffer and derive both from it; an unparseable diff now yields no
`diffHash` (gate fails closed) instead of context-inclusive hunks.
- Escape PR-controlled paths in the body (compose-review): a diff filename can carry
a newline, `@mention`, HTML or Markdown; the gate's criticals/unreviewed/disclosed
strings rendered it verbatim into the posted body. Paths and linter messages now
render in an inline code span with backticks and newlines stripped (`mdField`).
- Classify lstat failures (script-lint): only ENOENT/ENOTDIR is "missing" (deleted);
EACCES/EIO/ELOOP is now `irregular` (skipped, fail closed) rather than silently
read as a deletion into an `ok: true` "nothing changed".
Three left as reasoned follow-ups: verifier-provenance for `[build]/[test]/[probe]`
tags (a pre-existing verification-model concern, not this gate); actionlint's native
workflow diagnostics (deferring the whole tool loses them, but separating native
from embedded-shell output needs an actionlint-output parser this env can't verify);
and cryptographic authentication of the report file (the orchestrator runs the
deterministic command and overwrites any pre-planted file; full anti-forgery is a
broader harness-trust change). 985 review tests, ESLint/tsc/Prettier clean.
* fix(review): clean error from the script-lint handler on a bad plan
Wrap `runScriptLint` in the command handler in try/catch, matching sibling
build-test: a missing or invalid plan makes it throw, and yargs' default handler
would print a stack trace the orchestrator has to parse. Emit the one-line message
and a non-zero exit instead; the gate still fails closed on the absent report.
* fix(review): scrub HADOLINT_* env, canonicalize worktree paths, portable isolation tests
Follow-up adversarial round on the previous fixes:
- Scrub HADOLINT_* from the child env (script-lint): hadolint 2.14 MERGES config from
HADOLINT_IGNORE / HADOLINT_OVERRIDE_* / HADOLINT_CONFIG with the explicit --config,
so an inherited one could suppress the findings the neutral config forces. Drop
every HADOLINT_* var, mirroring the SHELLCHECK_OPTS scrub — configuration comes
from our --config alone. Hostile-env regression test added (this also makes the
HADOLINT_CONFIG assertion hermetic under an inherited value).
- Canonicalize the worktree-containment check (script-lint): the lexical startsWith
guard is defeated by a symlinked ANCESTOR — a directory replaced with a symlink
leaves a child path lexically inside but resolving outside, and lstat/the linter
follow it. Add a realpathSync check against the canonical worktree; a path that
cannot be canonicalised is handled as missing downstream. Symlink-escape test added.
- Portable isolation tests: override TEMP/TMP as well as TMPDIR (os.tmpdir() ignores
TMPDIR on Windows, leaving the fail-closed path unexercised there), and guard the
0o700 mode-bit assertion with process.platform !== 'win32'.
Left tracked: fail-closed on a PARTIALLY parsed diff — narrow (capture writes the
diff in one writeFileSync, not incrementally) and already mitigated (a truncation
that later completes changes the hash, so the freshness guard rejects the report).
987 review tests, ESLint/tsc/Prettier clean.
---------
Co-authored-by: verify <verify@local>
The OpenAPI 3.0 conversion walked the `properties` map as if it were a schema
node, so every step misfired on a property whose name collides with a JSON
Schema keyword. A property called `const` was deleted and replaced by a bogus
`enum: ["[object Object]"]` sibling, one called `default` was dropped by the
skip list, and one called `type` was copied verbatim instead of having its
`["string","null"]` union downgraded to `nullable` — the entire point of the
function, and a schema Gemini then rejects.
Handle `properties` / `$defs` / `definitions` as name->schema maps, which is
what the sibling `relaxSchemaForFunctionCalling` in this file already does.
* docs: add design placeholder for subagent model grade selection (#7685)
* feat(core): add subagent model grade selection
* test(subagent): cover resolveModelGrade deep guards and resume else branch
- subagent-manager: add tests for non-string grade values, blank values,
array-shaped modelGrades, and missing modelGrades (all return undefined)
- background-agent-resume: assert configured subagent model is preserved
(not forced to 'inherit') when launch flags (model + authType) are absent
Addresses test-coverage review findings.
* refactor(subagent): extract normalizeModelGradeSettings and merge model validate
- Extract normalizeModelGradeSettings helper shared by resolveModelGrade
and the Agent tool schema build, so the advertised grades and runtime
resolution cannot drift (addresses duplicated shape invariant).
- Merge the three model-parameter validate branches under a single
`params.model !== undefined` guard.
- Update agent.test.ts mock to preserve the real helper while still
mocking SubagentManager.
* refactor(core): simplify model grade resolution
* fix(core): reject unknown model grades
* docs(core): clarify model grade precedence
* docs: explain subagent model grades
* test(core): update subagent manager mock
* fix(core): list available model grades
* fix(core): trim model grade keys and cover schema removal
Grade keys were checked for emptiness via grade.trim() but stored in the
map and advertised in the tool schema enum untrimmed, while values were
trimmed. A padded key like ' small ' published a padded enum name the
model had to reproduce verbatim, and the allowlist check silently
excluded it. Normalize the key before storing, allowlist matching, and
schema publication.
Also adds a test for the delete schema.properties.model branch that fires
when grades transition from available to empty, so a regression that
breaks the delete leaves no stale model enum in the tool schema.
* fix(core): trim allowed model grade filters
---------
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
Measured on this repo: read-only verification is where the review misses its
hardest bugs. Handed the exact low-confidence finding for a `!` command that
executes twice, the strongest model's Step-3 agents traced the mechanism and
called it correct (0/3 across the full roster). Validated the fix directly — when
the same models (3.7-max and 3.8-max-preview) were allowed to WRITE AND RUN a
probe, both confirmed the double-execute from observed behaviour
(`sendShellCommand called twice with ["git push"]`), both did the probe-validity
self-check unprompted, and a plausible-but-false negative control was correctly
refuted with no fabrication.
So the verifier's brief now says: when a finding's failure scenario is a runnable
claim about a named unit and the repo has a fast unit harness (vitest/jest/pytest),
write a minimal probe, run it, and let the observed behaviour settle the verdict.
Two rules keep it evidence rather than theatre — a mandatory self-check that the
probe flips between buggy and correct, and leaving the tree exactly as found. A
finding a probe confirmed carries `Source: [probe]`, which compose-review treats
as deterministic (a run produced it) like `[build]`/`[test]`.
This is Phase 1: the agent-driven loop the validation exercised, no new command.
The deterministic runner + artifact and the finding-generation side (emitting a
probeable low-confidence finding rather than concluding "correct") are follow-ups.
Co-authored-by: verify <verify@local>
* fix(core): route id-less continuation chunks to a colliding tool-call opener's slot
StreamingToolCallParser remaps a provider index to a fresh slot when a second
tool call reuses an already-completed index. When that second call's id and
name arrive together on an empty opener delta (the standard OpenAI streaming
shape, function: { name, arguments: "" }), the index -> slot remap was never
recorded, because it was guarded by !meta.id and the id was already set. The
following id-less argument chunks then failed to find the remapped slot and
were routed to a brand-new orphan slot: the tool call was emitted with empty
args {}, the real arguments (in a nameless slot) were dropped by
getCompletedToolCalls, and the nameless slot made converter.ts raise
InvalidStreamError('MALFORMED_TOOL_CALL'), forcing up to four wasted retries of
an otherwise valid response.
Record the remap whenever the actual slot differs from the provider index, not
only before an id is known, so id-less continuations follow it. To keep a
genuinely new tool call from hijacking that slot, only adopt a pending remap
for a new id when the remapped slot has not itself been claimed by an id yet
(the existing id-arrives-late path); otherwise fall through to normal collision
handling.
Adds a regression test for the id+name-on-opener collision shape, which the
existing "late stable ID" collision tests did not cover.
* test(core): cover the block direction of the tool-call remap guard
Adds a case where a brand-new tool-call id reuses an index that already has a
0->N remap to an id-claimed slot: it must not hijack that slot and instead gets
its own. This exercises the `!toolCallMeta.get(remap)?.id` guard directly (the
prior test only covered id-less continuation routing). Fails without the guard.
* test(core): broaden tool-call collision coverage and refresh the remap doc
Per review: add a case for the content-bearing colliding opener (arguments
fragment arriving with the id/name, exercising the line-239 remap-record path
that the empty-opener test skipped), assert call_1's arguments survive the
collision in the id-less-continuation test, and update the pendingIndexRemaps
doc to describe its dual role (post-id adoption and id-less continuation
routing).
* refactor(core): drop the now-dead pending-remap delete on id adoption
Once an id adopts a remapped slot, the common-path re-registration keeps
the index->slot mapping alive for later id-less continuation chunks, so
the delete in the adoption branch is immediately re-created and has no
runtime effect. Remove it and note why the remap is intentionally kept,
so the mapping's lifetime reads straight through.
* test(core): pin id-less continuation routing across three colliding openers
The unconditional remap overwrite is deliberate: an id-less continuation after
the newest colliding opener must route to that opener's slot. A regression test
covers the three-call case so the overwrite is not later 'guarded' back into
misrouting the third call's continuation to an earlier slot.
