feat(cli): add auto-improve command - #4161
Conversation
📋 Review SummaryThis PR introduces a well-architected 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
8b4852a to
9ac38dc
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
This is a supplementary review pass focusing on issues not covered by the prior review. The initial review already identified the core issues (stale loop recovery, getRepoRoot error handling, cron/state write ordering, test coverage gaps, path traversal). The following are additional findings:
New Critical findings:
-
currentRuntype safety instopSelfImprove— accessing.statuson a non-objectcurrentRunproducesundefined, causinghasActiveRun=trueand leaving the loop stuck instoppingstate. -
CronScheduler 3-day expiry silently kills long-running loops —
startSelfImprovenever warns that recurring cron jobs expire after 3 days. A--every 1dloop silently dies.
New Suggestions:
- Top-level
actionhandler duplicates subcommand routing (dead code) isRecordduplicated acrossselfImproveCommand.tsandselfImproveState.tsstartsubcommand reassembles string for regex re-parsing instead of passing structured paramsparseIntervalsilently upgrades seconds to minutes without informing the usertickSelfImproveuses misleading "loop is stopping" message for all non-running statesactive.jsonnot cleaned after graceful stop with an active run — user muststoptwice
— glm-5.1 via Qwen Code /review
DragonnZhang
changed the title
|
@DragonnZhang 这个 PR 跟最新 main 有冲突了，请 rebase / merge 一下 ~ 当前唯一的冲突文件是 |
3c7e949 to
3fcdc11
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
CI note: Test (macos-latest, Node 22.x) is failing.
Non-inline findings:
-
useDialogClose.tsis not registered for theauto-improve-sourcedialog. When the source dialog is open and the user presses Ctrl+C,closeAnyOpenDialog()does not match any known dialog, so it falls through to the exit-prompt path while the source dialog is still rendered. Fix: addisAutoImproveSourceDialogOpen/closeAutoImproveSourceDialogtoDialogCloseOptionsand handle incloseAnyOpenDialog, consistent with all other dialogs. -
submitPromptOnCompleteRefleak after tick API failure: when a cron-fired tick's API call fails (429, network error), the catch block does not clearsubmitPromptOnCompleteRef. The stalemarkRunCompletedcallback leaks into the next successful user turn, marking the failed run as'success'. Fix: resetsubmitPromptOnCompleteRef.current = nullat turn start (alongsidelastTurnUserItemRef) and in the error catch block.
Auto-improve tick summary (3 fixes pushed)1.
|
- compactAutoImproveRunIndex: add hysteresis (compact at >2×MAX, not >MAX)
so it stops re-firing the read+parse+write on every tick once the cap is
reached; update the threshold test + add a hysteresis-band no-op test
- getCurrentBranch: pass { timeout: 10_000 } to git symbolic-ref, matching
getRepoRoot, so a blocking git config can't hang the CLI
- nonInteractiveCli cron tick: distinguish SIGINT/AbortError from real
failures (abortController.signal.aborted → { cancelled: true }) so a
cancelled tick records as 'cancelled', not 'failed'; mirrors Session.ts
Clean merge — brings in main's fix for the yaml-parser 'known limitations' pin tests so the Test suite is green against current main.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR (author is the authenticated user). LGTM. — claude-sonnet-4-20250514 via Qwen Code /review
… SIGINT in -p path - cron tick: wrap partListToText in try/catch that fires slashOnComplete (cancelled on abort, else errored) before rethrowing — it runs outside the try/finally below, so a throw would otherwise strand currentRun - outer -p error path: distinguish SIGINT (cancelled) from real failures (errored) when firing the captured slashOnComplete, mirroring the cron-tick abort check
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Downgraded from Request Changes to Comment: self-PR (GitHub does not allow REQUEST_CHANGES on your own PR).
…-day expiry CronScheduler creates recurring jobs with a hard 3-day expiry and reaps them on tick(); nothing refreshed the auto-improve job, so a loop running longer than 3 days was silently killed. - core: add CronScheduler.refresh(id) — extends a recurring job's expiry to now + THREE_DAYS_MS (no-op for one-shot/unknown jobs) - cli: tickAutoImproveClaim refreshes the job's expiry on every active tick (ticks fire far more often than every 3 days), so the job persists for the life of the loop; only for active+running loops, so stopped/stale loops still expire - tests: refresh unit tests + assert an active tick refreshes the cron job
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR (GitHub does not allow self-approval). CI is partially pending (macOS/Windows tests still in progress). The code demonstrates thorough defensive engineering: atomic writes, TOCTOU guards (re-read + mutex), ownership checks (expectedRunId), prompt injection hardening (boundary marker neutralization, control char stripping), stale run reclaim, and comprehensive onComplete firing on all paths (success, error, cancel). All previously reported issues from earlier commits have been addressed. — Qwen Code /review
The PR added a 'cancelled' onComplete signal; propagate it to the remaining
consumers so a SIGINT/abort isn't mis-recorded:
- dreamCommand: guard writeDreamManualRun on cancelled too (not just errored),
so a cancelled /dream doesn't persist a false consolidation record
- Session.ts regular ACP path: map stopReason 'cancelled' to { cancelled: true }
instead of lumping all non-end_turn reasons into { errored: true }
- Session.ts catch: pass { cancelled: true } when pendingSend.signal.aborted,
mirroring the cron path
| }; | ||
| } | ||
|
|
||
| async function getRepoRoot(config: Config): Promise<string> { |
There was a problem hiding this comment.
[Suggestion] Three independent implementations of "resolve git repo root" exist across this file (getRepoRoot at line 195), autoImproveState.ts (resolveRepoRoot at line 798), and AutoImproveSourceDialog.tsx (resolveRepoRoot at line 42). All three spawn git rev-parse --show-toplevel with a 10-second timeout and cwd fallback. Only this version has caching via repoRootCache.
If the timeout, fallback logic, or caching policy needs to change, three locations must be updated in sync. The dialog and state module also make redundant subprocess calls without caching.
Consider extracting a single shared resolveRepoRoot(config) with caching into autoImproveState.ts and importing from the other two files.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fair point on the duplication. Holding off on consolidating it into this PR, though: the three call sites have intentionally different needs — getRepoRoot (cron tick / hot path) caches via repoRootCache, while AutoImproveSourceDialog's resolver runs once on dialog open and the autoImproveState one runs in a context where the cache isn't shared. Folding the dialog/state paths onto the cached resolver is a behavior change (cache lifetime/invalidation) that's better done deliberately as a focused refactor than bundled into this feature PR. Flagging for maintainer call on whether to require it here or track separately — leaving open.
| if (sendResult.stopReason === 'cancelled') { | ||
| slashOnCompleteCancelled = true; | ||
| } else { | ||
| slashOnCompleteErrored = true; |
There was a problem hiding this comment.
[Suggestion] When sendResult.stopReason === 'max_tokens' and responseStream is null, this code sets slashOnCompleteErrored = true. For auto-improve runs that completed substantial work before hitting the token limit, recording the run as 'failed' is misleading.
Users reviewing run history may incorrectly assume a run failed when it actually completed meaningful work. Consider distinguishing max_tokens from other failure modes — either record as success when the response contained meaningful output, or add a distinct status.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Valid semantics question, but it's a product call I'd rather not decide unilaterally: should a max_tokens run be recorded as 'failed', 'success', or a distinct status (e.g. 'truncated')? Today the loop treats any non-end_turn, non-cancelled stop as errored. Mapping max_tokens to success-when-meaningful-output needs a definition of "meaningful", and a new status touches the run-index schema + status UI. Leaving open for maintainer input on the preferred run-status semantics rather than guessing.
…onComplete
The interactive submitPrompt catch fired onComplete({ errored: true }) for all
errors. A process-shutdown abort can reach this catch with the onComplete ref
still set (unlike an interactive cancel, which clears it first), mis-recording
a cancelled run as 'failed'. Map AbortError to { cancelled: true }, consistent
with the ACP and non-interactive paths.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
PR: #4161 — feat: add auto-improve self-improvement loop
Review method: Automated (qwen3.7-max via Qwen Code)
Deterministic analysis: ✅ tsc 0 errors, eslint 0 errors
Build: ✅ passed | Tests: ✅ 341/341 passed
Overview
This is a large feature PR (+5745/-27, 33 files) adding an auto-improve self-improvement loop with cron scheduling, state persistence, worktree isolation, UI dialogs, and non-interactive CLI support. The architecture is sound overall, but there are several robustness and test-coverage concerns that should be addressed before merge.
Findings Summary
| # | Severity | File | Issue |
|---|---|---|---|
| F3 | Critical | autoImproveCommand.ts | Worktree cleanup relies on LLM following prompt rules, not programmatic enforcement |
| F4 | Suggestion | cronScheduler.ts | Cron job silently reaped after 3-day expiry with no logging |
| F7 | Nice to have | autoImproveState.ts | compactAutoImproveRunIndex double-reads file |
| F8 | Suggestion | autoImproveState.ts | listAutoImproveLoopStates is dead code (exported, zero callers) |
| F11 | Suggestion | autoImproveCommand.ts | Three separate resolveRepoRoot implementations |
| F13 | Suggestion | useGeminiStream.ts | Auto-improve-specific logic in general hook |
| F14 | Suggestion | cronScheduler.ts | parseCron called O(jitter) times per second — should cache |
| F17 | Critical | nonInteractiveCli.ts | Cron callback (~90 lines) entirely untested |
| F18 | Critical | Session.ts | Cron error/cancel branches (4 branches) only happy-path tested |
| F19 | Suggestion | autoImproveCommand.ts | withTickMutex has no concurrent-access test |
| RA | Suggestion | ui/types.ts | cancelled flag in SubmitPromptActionReturn but missing from SubmitPromptResult.onComplete |
Total: 3 Critical, 7 Suggestion, 1 Nice to have
CronScheduler findings (no inline — lines not in diff)
F4 — Silent job reaping: tick() deletes expired jobs via this.jobs.delete(job.id) at line 206 with zero log output. After 3-day expiry with no refresh() call, the job silently disappears leaving stale state files. Suggestion: Add a log entry before deletion.
F14 — parseCron per second: matches(job.cronExpr, candidateDate) at line 234 internally parses the cron expression on every call. For N jobs, this means O(N) parseCron calls per second. Suggestion: Cache parsed cron fields on the job object when registered.
Low-confidence findings (not inline)
These findings were identified at lower confidence and are listed for awareness only:
- Cross-process TOCTOU race in tick claiming (documented limitation in the code)
- Run record fields lack sanitization/length caps
worktreePathlacks path validation beforegit worktree remove- Silent error swallowing in
nonInteractiveCli.tspartListToTextpath markRunCompletedwrite failure strands run for up to 2h- Interactive cancel race on
lastRun.status
— qwen3.7-max via Qwen Code /review
| }; | ||
| } | ||
|
|
||
| async function markRunCompleted( |
There was a problem hiding this comment.
[Critical] Worktree cleanup is delegated to the LLM via prompt instructions rather than enforced programmatically.
markRunCompleted updates the run record status but does not programmatically remove the git worktree. The worktreePath field is stored in SelfImproveRunRef but never cleaned up on completion, failure, or cancellation. Cleanup is instead instructed to the AI agent in the tick prompt (lines ~549, 553, 556).
If a run fails, is cancelled, or the process crashes mid-tick, the worktree remains on disk. Over time, leaked worktrees accumulate in the repo directory.
Suggestion: Add explicit git worktree remove <worktreePath> in markRunCompleted and in the error/cancel paths of the tick handler. Don't rely on the LLM agent to execute cleanup commands.
There was a problem hiding this comment.
Real concern, but a deliberate design choice I won't flip unilaterally. Cleanup is delegated to the agent via the tick prompt so it can decide what to preserve; making markRunCompleted run git worktree remove --force unconditionally risks destroying a worktree that still holds uncommitted work, and races the agent's own cleanup. A safe programmatic GC (remove only on terminal failure/cancel, force-guarded, skip if unexpectedly dirty) is worth doing but needs a policy decision. Flagging for maintainer input — leaving open.
| import process from 'node:process'; | ||
|
|
||
| const debugLogger = createDebugLogger('GEMINI_STREAM'); | ||
|
|
There was a problem hiding this comment.
[Suggestion] parseAutoImproveTickLoopId introduces auto-improve-specific logic into the general useGeminiStream hook.
This parsing function is only relevant to the auto-improve feature but lives in a hook used by all streaming operations. As the feature grows, this coupling will increase.
Suggestion: Move this logic into autoImproveCommand.ts or a dedicated auto-improve hook, keeping useGeminiStream feature-agnostic.
There was a problem hiding this comment.
Fair coupling point. parseAutoImproveTickLoopId lives here because the loop-id must be recovered from the prompt at the exact point the hook submits it (to thread cancellation back to the right loop). Moving it to a dedicated auto-improve module is reasonable but re-exposes the submit-time seam; better as a focused decoupling pass than widening this PR. Leaving open for maintainer prioritization.
| // accumulated from process start rather than this tick. | ||
| const cronJobStart = Date.now(); | ||
| const label = job.prompt.slice(0, 40); | ||
| let modelText = job.prompt; |
There was a problem hiding this comment.
[Critical] The non-interactive cron callback (~90 lines, lines ~1348-1440) has zero test coverage.
This code path handles the entire auto-improve tick lifecycle in non-interactive (headless/CI) mode: reading state, creating worktrees, running the agent, processing results, and updating state. It's a critical path for CI/CD usage of the auto-improve feature.
Without tests, regressions in any of the ~5 branches (success, failure, cancellation, worktree error, state read error) will go undetected.
Suggestion: Add integration tests covering at minimum:
- Successful tick end-to-end
- Run failure → state updated to
failed - Cancellation → state updated to
cancelled - Worktree creation failure → graceful error handling
There was a problem hiding this comment.
Agreed — the headless -p cron callback is the biggest coverage gap (flagged as a tracked follow-up in earlier threads too). It needs a nonInteractiveCli test scaffold that drives the callback through success/failure/cancel/worktree-error/state-error; that's a substantial piece I'd rather land deliberately than rush. Leaving open as the tracked test-harness follow-up.
| cancelled?: boolean; | ||
| }) => Promise<void>) | ||
| | undefined; | ||
| let slashOnCompleteErrored = false; |
There was a problem hiding this comment.
[Critical] The cron error/cancel branches in Session.ts have only happy-path test coverage.
This block declares slashOnCompleteErrored and slashOnCompleteCancelled flags and manages at least 4 error/cancel paths (lines 1769, 1795, 1822, 1833, 1899). Only the success path (run completes, branch created, PR submitted) is tested.
The error branches handle real failure modes — agent errors, worktree failures, cancellation mid-tick — that users will encounter in practice.
Suggestion: Add tests for:
- Agent returns error →
slashOnCompleteErrored = truepath - User/system cancels →
slashOnCompleteCancelled = truepath - Worktree operation fails during tick
- State write failure after run completion
There was a problem hiding this comment.
Same theme as the non-interactive coverage gap — the Session.ts cron error/cancel branches only have happy-path coverage. The branches are now correct (errored vs cancelled distinguished); exercising agent-error / worktree-failure / cancel-mid-tick needs fixtures simulating those mid-stream. Tracking alongside the cron test-harness follow-up; leaving open.
| // sessions. (Cross-process races between separate CLI invocations still require | ||
| // on-disk file locking; this closes the common in-process case.) | ||
| const tickMutexes = new Map<string, Promise<unknown>>(); | ||
| function withTickMutex<T>(loopId: string, fn: () => Promise<T>): Promise<T> { |
There was a problem hiding this comment.
[Suggestion] withTickMutex has no test for concurrent access.
This mutex serializes ticks per loopId using a Map of chained promises. It's the only concurrency control preventing overlapping runs of the same loop, but there's no test verifying it actually serializes correctly under concurrent calls.
Suggestion: Add a test that fires multiple concurrent withTickMutex calls for the same loopId and asserts they execute sequentially.
There was a problem hiding this comment.
Reasonable coverage gap. withTickMutex is internal (not exported), so a direct unit test means exporting it purely for testing; the serialization is currently exercised indirectly via the tick tests. A proper concurrent test (two overlapping tick <id> calls asserting one claims and the other skips) is a good focused follow-up — flagging rather than adding a fragile concurrency test under time pressure.
- types: add 'cancelled?' to SubmitPromptResult.onComplete and NonInteractiveSlashCommandResult.onComplete so they match SubmitPromptActionReturn (Session.ts branches on both errored/cancelled) - compactAutoImproveRunIndex: reuse the JSON.parse from the record-count check via normalizeRunIndex instead of re-reading the file through readAutoImproveRunIndex (one fewer read+parse)
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Re-reviewed at f2d09ed. No high-confidence issues found.
The new commits add robust onComplete threading for error/cancellation paths across Session.ts (ACP), nonInteractiveCli.ts (headless), and useGeminiStream.ts (interactive). Key improvements since the last review:
- onComplete now fires on error and cancellation paths (not just success), preventing currentRun deadlocks when a tick fails or is cancelled.
- Cancellation vs. failure distinction is consistently propagated (AbortError -> cancelled, other errors -> failed) across all three execution modes.
- Cron scheduler gains a refresh() method to extend recurring job expiry, preventing silent reaping of long-running loops.
- Stale run reclamation (2h max age) with runId ownership guard provides a safe backstop for stuck runs.
- Prompt injection defense: fence markers in user input are neutralized (--- -> en dashes) with test coverage.
- Extensive test coverage for the new paths (concurrent tick mutex, TOCTOU re-read, stale completion handling, cancellation).
CI: 7 passing, 2 pending (CodeQL + review workflow itself). No failures.
Verdict: APPROVE
If active.json referenced a loop whose state.json was missing/corrupt, readAutoImproveLoopState returned null, the running/stopping guard was skipped, and startAutoImprove fell through to create a new loop — leaving the old loop directory orphaned under .qwen/auto-improve/loops/. Since readAutoImproveLoopState only returns null for ENOENT/SyntaxError (transient FS errors rethrow), null is genuinely unrecoverable: remove the orphaned dir and clear the dangling pointer before starting fresh.
Resolve conflicts where main refactored the same code my changes touch: - Session.ts cron handler: keep main's withInteractionSpan telemetry wrapper (cronHadError / turnCount / conversation_finished) AND re-integrate the auto-improve slashOnComplete lifecycle — slash-command resolution of the cron prompt, onComplete capture, and the cancelled/errored finally-fire (main's version had no slash handling, which auto-improve's cron tick needs) - Session.ts #executePrompt: take main's withInteractionSpan + logUserPrompt structure, re-add the pendingSlashOnComplete capture - Session.ts class field: keep both pendingSlashOnComplete and followupAbort - dreamCommand: keep main's recordDream helper + ACP eager-fire path, but guard the non-ACP onComplete on errored/cancelled Verified: Session/dream/auto-improve suites 202/202.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] slashCommandProcessor.test.ts (unchanged file) has 2 typecheck errors caused by this PR's type/signature changes:
- Line 137 (TS2739):
createMockActions()is missing 3 new properties:openSkillsManagerDialog,openAutoImproveSourceDialog,openStatsDialog - Line 1547 (TS2554):
useSlashCommandProcessor()call expects 16-17 arguments but receives 15 — missing the newupdateItemparameter
Both need to be updated to match the modified SlashCommandProcessorActions type and hook signature.
— qwen3.7-max via Qwen Code /review
…ompt
main's UserPromptExpansion hook can block a submit_prompt and return a
'message' result, discarding the captured onComplete. For an auto-improve
tick (/auto-improve tick → submit_prompt with onComplete=markRunCompleted),
that stranded currentRun in 'implementing' until the 2h stale reclaim. Fire
result.onComplete({ errored: true }) before returning the blocked result.
# Conflicts: # packages/cli/src/ui/hooks/useGeminiStream.ts
🧪 Local runtime verification (built CLI + real model, interactive TUI via tmux) —
|
| # | Scenario | Observed |
|---|---|---|
| 1 | ✅ Usage / routing | Bare /auto-improve → 4-line usage; tick stays hidden |
| 2 | ✅ End-to-end tick runs | 4 runs completed: TODO fix, sibling-function fix, boundary tests, README docs — each in an isolated worktree (EnterWorktree), checks run (6/6…15/15), committed to the loop default branch main per the local-source delivery rule, conventional commit messages, worktree removed after success |
| 3 | ✅ Run bookkeeping | summary.md, runs/index.json, per-run docs all written; status box "Recent runs" shows task/branch/commit/run-doc per run |
| 4 | ✅ Cron scheduling | */5 * * * * fired on schedule three times (:20, :25, :30), each submitting /auto-improve tick <loopId> into the session (the :25 submission line ● Cron: /auto-improve tick … captured directly) |
| 5 | ✅ Status box | running/stopped variants, loop id, cadence+cron, default branch, sources, cron job id, prompt, custom sources, last/recent runs — all accurate |
| 6 | ✅ Stop semantics | stop → "Auto-improve loop stopped.", cron job deleted, active pointer cleared; tick-after-stop → "skipped: loop is not active." |
| 7 | ✅ Duplicate start | second start while active → "An auto-improve loop is already active: " |
| 8 | ✅ Source dialog CRUD | toggle built-ins, add ×2, edit, delete, Save → config.json matches dialog state exactly; Esc discards cleanly |
| 9 | ✅ Legacy migration | userContext config → shown as custom source in dialog, snapshotted into a new loop, rendered in status box (end-to-end) |
| 10 | 🔍 Interval validation | 45m / 2d / 90s / banana → each gets its specific, correct error |
| 11 | 🔍 Hostile loop id | /auto-improve tick ../../etc/passwd → graceful "skipped: loop is not active." |
| 12 | 🔍 Cron kill switch | QWEN_CODE_DISABLE_CRON=1 → start correctly rejected (but see finding 4 on the message text) |
⚠️ Finding 1 (merge-blocking): runs are finalized as success seconds into the run — Esc-cancellation only works in a tiny window
Decisive observation (no cancel, no cron involved): start --every 30m → 21 seconds in, while the agent was visibly mid-worktree-work (esc to cancel spinner active), state.json already read:
currentRun: None | lastRun: {'runId': 'pending-2026-06-12T06-40-44Z-…', 'status': 'success', …}
The run is recorded success at the end of the model's first stream segment, not when the tick actually finishes. Everything the PR builds on top of run lifecycle then degrades:
- Esc during the tool phase (≈ the entire real duration of a tick): only "● Request cancelled." appears — no "Auto-improve run cancelled. The loop is still active…" reminder, no
cancelledstatus (run alreadysuccess). Reproduced on a cron-fired tick and on a start-submitted tick. - Esc within the first seconds (before the first segment ends) works exactly as designed — reminder shown,
lastRun.status: cancelled. Reproduced twice. The claimed behavior exists; its window is just a few seconds wide. - The "previous run is still active" tick-dedup guard can effectively never trip in-session after those first seconds (
currentRunis already cleared). Observed: a cron tick started a brand-new full run ~10s after I Esc'd the previous one, with no skip message. - Books end up clean-but-wrong: a later tick's agent even backfilled the cancelled run's index record as
success(per tick-prompt rule 11), so the cancellation leaves no trace anywhere.
Likely site: packages/cli/src/ui/hooks/useGeminiStream.ts — the submit_prompt onComplete fires after the first processGeminiStreamEvents returns (tool-call continuations re-enter submitQuery as ToolResult with the refs already cleared, so currentAutoImproveLoopIdRef is also nulled — which is why the Esc handler no longer recognizes the turn as an auto-improve tick). Suggested fix: fire onComplete (and keep the loop-id ref alive) only when the turn reaches a terminal state with no pending tool continuations.
Other findings
⚠️ User-queued messages lost at a cron boundary (observed once):/auto-improve status+stopqueued during tick 1 ("Press ↑ to edit queued messages" visible) were silently discarded when tick 1's completion coincided with the cron tick submission; pane showsRequest cancelled.-free transitionfinal response → ● Cron: … → > /auto-improve tick …with my two commands never echoed or executed. Negative control: with no cron racing the boundary, a queuedstatusdrained and executed normally. Worth checking the cron-submission vs message-queue interaction.- Esc mid-run orphans the worktree —
.qwen/worktrees/error-message-checks+ branchworktree-error-message-checksleft behind, and the footer still showed⎇ worktree-error-message-checksafter cancel. The PR's risk note says "a hard process kill could still leave orphaned worktrees"; a plain Esc suffices. - Stale gate-failure advice: with cron force-disabled the error says "Enable experimental.cron or QWEN_CODE_ENABLE_CRON=1" — on the merged head neither exists (
isCronEnabled()reads only the kill switch; cron is default-on). Same staleness in the PR description's validation steps. - Non-interactive mode is fully gated off —
-p "/auto-improve status"→The command "/auto-improve" is not supported in this mode.Yet the PR adds non-interactiveonCompleteplumbing with comments anticipating-p "/auto-improve start". If non-interactive is intentionally deferred, fine (the plumbing is just unreachable); if not,start/status/stop/tickneedsupportedModesincluding non-interactive. - Dialog nits: (a) entering edit mode places the cursor at the start of the existing text — typed text prepends ("… and tests/" became
and tests/Focus on TODO…); cursor-at-end is the usual convention. (b) Under synthetic rapid-fire keys (no inter-key delay), aspacecan toggle the row the state machine was on rather than the row the cursor shows — human typing speed doesn't hit this; paced keys behave perfectly. - Positive observation worth keeping: the four committed improvements were genuinely good, bounded changes (input validation + matching tests, boundary coverage, accurate README API docs) with passing checks each time, and the user's working tree was never touched.
Verdict (merge reference)
The loop machinery — scheduling, worktree isolation, source-aware local delivery, run docs, stop semantics, source dialog, legacy migration, input validation — all verify cleanly at the real surface, and the agent-side behavior is impressively disciplined. But finding 1 breaks one of the PR's explicitly claimed behaviors (Esc cancellation) for all but the first few seconds of every tick, and silently mis-records interrupted runs as successful — for a feature whose whole job is autonomous bookkeeping of unattended runs, I'd treat that as merge-blocking. Recommend: fix the onComplete timing, re-run the Esc matrix (early / tool-phase / cron-fired), then this is good to go. Findings 2–6 are follow-up grade.
🇨🇳 中文版（点击展开）
🧪 本地运行时验证（构建后 CLI + 真实模型，tmux 交互式 TUI）— ⚠️ 发现一个建议阻塞合并的缺陷（其余表面全部通过）
与我其他 PR 验证相同的流程：本地构建 PR head（df22089，92 个 commit；分支已合入 origin/main，落后最新 main 仅 2 个提交），在 tmux 中驱动构建产物 dist/cli.js 的交互式 TUI，使用真实 Qwen 模型，测试场地是一个预埋 TODO 的临时 git 仓库（tick 因此有有界、本地可验证的工作可做）。约 35 分钟真实驱动：创建 3 个循环、执行 7 次 tick（其中 2 次由 cron 按计划自动触发）、产生 4 个真实改进 commit。未重跑单测——纯运行时观察。
环境： macOS (arm64)、Node v22.22.2、独立 tmux socket、临时仓库 /tmp/ai-playground（无 remote → 推送在构造上不可能）、approvalMode: yolo。注意：在合并后的 head 上 cron 已默认开启（isCronEnabled() 只认 QWEN_CODE_DISABLE_CRON=1 杀开关），PR 描述中"启用 experimental.cron"的指引已过时。
通过项（且质量很高）
| # | 场景 | 观察结果 |
|---|---|---|
| 1 | ✅ 用法/路由 | 裸 /auto-improve → 4 行用法；tick 保持隐藏 |
| 2 | ✅ tick 端到端 | 4 次完整运行：TODO 修复、兄弟函数一致性修复、边界测试、README 文档——每次都在隔离 worktree（EnterWorktree）中进行，跑检查（6/6…15/15），按 local 源交付规则提交到循环默认分支 main，规范的 commit message，成功后删除 worktree |
| 3 | ✅ 运行记账 | summary.md、runs/index.json、每次运行的文档全部写齐；状态盒 "Recent runs" 展示每次的任务/分支/commit/run doc |
| 4 | ✅ cron 调度 | */5 * * * * 按计划触发三次（:20、:25、:30），每次向会话提交 /auto-improve tick <loopId>（:25 的提交行 ● Cron: /auto-improve tick … 有直接捕获） |
| 5 | ✅ 状态盒 | running/stopped 两种形态、loop id、节奏+cron、默认分支、源、cron job id、prompt、自定义源、last/recent runs——全部准确 |
| 6 | ✅ stop 语义 | stop → "Auto-improve loop stopped."，cron job 删除、活动指针清除；停止后 tick → "skipped: loop is not active." |
| 7 | ✅ 重复 start | 活动期间再次 start → "An auto-improve loop is already active: " |
| 8 | ✅ 源对话框 CRUD | 切换内置源、添加 ×2、编辑、删除、保存 → config.json 与对话框状态完全一致；Esc 干净放弃 |
| 9 | ✅ legacy 迁移 | userContext 配置 → 对话框中显示为自定义源、快照进新循环、状态盒中渲染（端到端） |
| 10 | 🔍 间隔校验 | 45m / 2d / 90s / banana → 各自得到具体、正确的报错 |
| 11 | 🔍 恶意 loop id | /auto-improve tick ../../etc/passwd → 优雅的 "skipped: loop is not active." |
| 12 | 🔍 cron 杀开关 | QWEN_CODE_DISABLE_CRON=1 → start 被正确拒绝（但文案见发现 4） |
⚠️ 发现 1（建议阻塞合并）：运行在开始数秒后即被定格为 success —— Esc 取消只在极小窗口内生效
决定性观察（无取消、无 cron 干扰）：start --every 30m → 第 21 秒、agent 明显还在 worktree 中干活（esc to cancel 转轮仍在）时，state.json 已经是：
currentRun: None | lastRun: {'runId': 'pending-2026-06-12T06-40-44Z-…', 'status': 'success', …}
运行在模型第一个流式段结束时就被记为 success，而非 tick 真正完成时。构建在运行生命周期之上的一切随之退化：
- 工具阶段按 Esc（≈ tick 的几乎全部真实时长）：只出现 "● Request cancelled." —— 没有"循环仍活跃"提醒、没有
cancelled状态（已被记为success）。在 cron 触发的 tick 和 start 提交的 tick 上均复现。 - 开始数秒内按 Esc（第一段流结束前）则完全符合设计——提醒出现、
lastRun.status: cancelled。复现两次。声明的行为存在，只是窗口只有几秒宽。 - "previous run is still active" 去重保护在会话内几乎永远不会触发（
currentRun早已清空）。实测：Esc 掉上一个 tick 约 10 秒后，cron tick 直接开启了全新一轮，没有任何 skip。 - 账面干净但错误：后续 tick 的 agent 甚至按 tick 提示词规则 11 把被取消运行的索引记录"补账"成了
success——取消行为在任何地方都不留痕。
可能位置：packages/cli/src/ui/hooks/useGeminiStream.ts —— submit_prompt 的 onComplete 在第一次 processGeminiStreamEvents 返回后即触发（工具调用续传以 ToolResult 重入 submitQuery，相关 ref 已被清空，因此 currentAutoImproveLoopIdRef 也被置空——这正是 Esc 处理器不再识别该回合为 auto-improve tick 的原因）。建议修复：仅在回合到达终态（无待续工具调用）时触发 onComplete 并保持 loop-id ref 存活。
其他发现
⚠️ cron 边界处用户排队消息丢失（观察到一次）： tick 1 运行期间排队的/auto-improve status+stop（"Press ↑ to edit queued messages" 可见）在 tick 1 结束恰逢 cron tick 提交时被静默丢弃；pane 显示最终响应 → ● Cron: … → > /auto-improve tick …的过渡，两条命令从未回显或执行。反向对照：无 cron 竞争时，排队的status正常排空执行。建议检查 cron 提交与消息队列的交互。- 运行中 Esc 会遗留孤儿 worktree ——
.qwen/worktrees/error-message-checks+ 分支worktree-error-message-checks残留，且取消后底栏仍显示⎇ worktree-error-message-checks。PR 风险声明说"硬杀进程才可能遗留孤儿 worktree"，实际普通 Esc 即可。 - 门控失败文案过时： cron 被强制关闭时报错说 "Enable experimental.cron or QWEN_CODE_ENABLE_CRON=1"——合并后的 head 上两者都不存在（
isCronEnabled()只读杀开关；cron 默认开启）。PR 描述的验证步骤同样过时。 - 非交互模式完全被挡 ——
-p "/auto-improve status"→The command "/auto-improve" is not supported in this mode.但 PR 添加了非交互onComplete管道，注释还预期-p "/auto-improve start"可用。若有意延后非交互支持则无妨（管道暂不可达）；否则start/status/stop/tick需要声明包含非交互的supportedModes。 - 对话框小问题：(a) 进入编辑模式时光标在已有文本行首——输入会变成前缀（"… and tests/" 变成了
and tests/Focus on TODO…）；常规习惯是光标在行尾。(b) 合成的无间隔连发按键下，space可能作用于状态机之前所在行而非光标显示行——人类手速不会触发；放慢节奏后行为完美。 - 值得保留的正面观察：四个落地的改进都是真正有价值、有边界的修改（输入校验+配套测试、边界覆盖、准确的 README API 文档），每次检查都通过，且用户工作区从未被碰过。
结论（合并参考）
循环机制——调度、worktree 隔离、源感知本地交付、运行文档、stop 语义、源对话框、legacy 迁移、输入校验——在真实表面全部干净通过，agent 侧行为也相当自律。但发现 1 使 PR 明确声明的行为之一（Esc 取消）在每个 tick 除最初几秒外全程失效，并把被中断的运行静默记为成功——对一个核心职责就是无人值守运行记账的功能而言，我认为这应当阻塞合并。建议：修复 onComplete 时机，重跑 Esc 矩阵（早期/工具阶段/cron 触发），之后即可合并。发现 2–6 属后续跟进级别。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
/review Summary
PR: feat(cli): add auto-improve command (#4161)
Reviewer: qwen3.7-max via Qwen Code
Scope: 34 files, +5786 lines
Overall Assessment
This is a large, ambitious feature adding session-scoped repository improvement loops. The code is well-structured with clear separation of concerns (state management, cron scheduling, command handling). Build and all existing tests pass; typecheck and lint are clean.
Findings
3 inline comments posted (1 Critical, 2 Suggestions):
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | nonInteractiveCli.ts |
Cron callback IIFE (~100 lines) has zero test coverage |
| 2 | 💡 Suggestion | Session.ts |
cronHadError not set on token-limit cutoff — misleading span status |
| 3 | 💡 Suggestion | dreamCommand.ts |
ACP path eagerly records dream without error/cancel guard |
8 findings flagged for human review (low confidence — not posted as inline comments):
- Cross-process TOCTOU on state.json (no file locking)
- Cron re-resolves commands every tick, overwriting shared Config
- Ctrl+C cancellation depends on string-scraping prompt text
- markRunCompleted write failure silently swallowed
- Worktree cleanup delegated to LLM — orphaned worktrees accumulate
- 3 independent
resolveRepoRootimplementations with different caching - parseStartArgs accepts day units but parseInterval rejects them
- startAutoImprove stale reclaim has no debug log
Existing Review Coverage
@wenshao has already posted 23+ inline comments covering many important issues (JSON.parse validation, stale loops, try-catch, cron boundaries, tick concurrency, path traversal, prompt injection, test gaps, etc.). The findings above are non-overlapping with existing comments.
— qwen3.7-max via Qwen Code /review
| sendMessageType: SendMessageType.Cron, | ||
| }); | ||
| drainLocalQueue().then(checkCronDone, onDrainError); | ||
| void (async () => { |
There was a problem hiding this comment.
[Critical] Cron callback IIFE (~100 lines of new async code) has zero test coverage.
This entire void (async () => { ... })() block — the primary execution path for headless/CI cron ticks — is never exercised by tests. The test file mocks cron as disabled and never tests:
submit_promptresolution withonCompletecallbackpartListToTexterror handling whensendResultis null- The
cancelledvserroredbranch distinction - The agent message emission on error
Bugs here mean ticks silently fail or deadlock future ticks for up to 2 hours with no user-visible signal.
Suggested fix: Add integration-level tests (even with mocked model) covering:
- Successful tick: submit_prompt → model response → onComplete fired
- Token-limit cutoff: sendResult null → slashOnCompleteErrored set
- Abort/cancel: ac.signal.aborted → slashOnCompleteCancelled set
- Error path: exception → cronHadError + debug log
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Agreed this headless cron-callback path needs coverage — it's the same gap as the existing open thread on the non-interactive cron callback. Adding it properly means integration-level tests (mocked model + temp worktree) exercising: successful tick (submit_prompt -> response -> onComplete fired), token-limit cutoff (sendResult null -> slashOnCompleteErrored), abort/cancel (signal.aborted -> slashOnCompleteCancelled), and the error path (exception -> cronHadError + debug log). Leaving this open pending maintainer direction on the preferred test harness rather than resolving it.
The ACP path fired recordDream() eagerly before the turn ran, while the interactive path correctly deferred it to a guarded onComplete. Now that Session.ts captures and fires the submit_prompt onComplete (via pendingSlashOnComplete) with errored/cancelled flags, ACP can use the same guarded onComplete — so a failed/cancelled /dream in ACP no longer persists a consolidation record as if it completed. Unifies both modes. Updates the two ACP tests to assert deferral instead of eager write.
When sendResult.responseStream is null and the stop reason is not 'cancelled' (e.g. max_tokens), the run is already recorded failed via slashOnCompleteErrored, but cronHadError stayed false so the interaction span reported 'ok'. Set cronHadError too so the span status matches the recorded run outcome.
🔁 Re-verification of the two commits pushed after my report this morning (real daemon/ACP surface, A/B against
|
Scenario (/dream via REST prompt) |
old df22089 |
new 13bb4df5c |
|---|---|---|
| Sampled mid-turn (provider holding the response open) | ❌ lastDreamAt already written — eager write before the turn ran |
✅ absent (deferred) |
| Turn succeeds → record at turn end | written (eagerly, earlier) | ✅ written 22 ms after "prompt turn completed" |
| Turn fails (provider 500 ×5 retries observed) | ❌ record persisted for a failed consolidation | ✅ never written (checked again minutes later) |
Turn cancelled (POST /session/:id/cancel mid-hold) |
❌ record persisted for a cancelled turn | ✅ never written |
Cancel closure (same session): cancel turn 1, then run a 2nd successful /dream |
n/a (eager writes make it moot) | ✅ turn 1 → daemon logs "prompt turn failed" 24 ms after cancel, no record; turn 2 → completes and records. Session and the pendingSlashOnComplete machinery stay healthy after a cancelled slash turn |
Unit-level pinning: the two updated ACP tests fail against the old implementation (defers writeDreamManualRun… and …cancelled are exactly the 2 failures with old dreamCommand.ts + new tests) and pass on the new head — the tests genuinely pin the fix. dreamCommand + Session suites: 132/132; autoImproveCommand/autoImproveState/useGeminiStream suites: 177/177; cronScheduler passes. Merge with today's main (b794d64f) is conflict-free (git merge-tree --write-tree clean), and the PR's GitHub CI is green on all three platforms.
13bb4df5c (cron span status) — verified structurally
The cron-tick interaction span resolves its status via ac.signal.aborted ? 'cancelled' : cronHadError ? 'error' : 'ok' (Session.ts:2192). The null-responseStream + non-cancelled stop-reason branch (e.g. max_tokens) recorded the run as failed but left cronHadError=false → span said ok; the commit sets cronHadError=true there, matching the other error branch. Code-path reading confirms the claim; note no test pins this branch's span status (minor — fine as is, or a one-liner assertion in Session.test.ts if you want it locked).
Why Finding 1 is still open (and unchanged)
git diff df22089..13bb4df5c touches only dreamCommand.ts/.test.ts and Session.ts — useGeminiStream.ts, autoImproveCommand.ts, autoImproveState.ts are byte-identical to the head I live-verified this morning, so every observation in that report still applies to the interactive surface: success-at-first-segment, the few-seconds Esc window, the dedup guard that can't trip, cancelled runs backfilled as success. One additional scoping note from this round: the new ACP deferral is correct because Session.ts fires onComplete at true turn end — the interactive path fires it at the end of the first stream segment, so even interactive /dream would record mid-turn on a consolidation that invokes tools (same root cause, not auto-improve-specific). Fixing the interactive onComplete timing resolves Finding 1 and this in one move.
Also observed (pre-existing daemon behavior, identical on both heads, not this PR's scope): cancelling a slash-submitted turn logs a noisy child RPC error "Not currently generating" even though the cancel takes effect, and the daemon WARN line renders the failure reason as [object Object].
Recommendation
Unchanged from this morning, now with the delta verified: the two new commits are solid — the PR still needs the interactive onComplete/Esc-cancellation fix (Finding 1) before merge. Findings 2–6 from the morning report remain follow-up grade.
🇨🇳 中文版（点击展开）
🔁 对今早报告后新推送的两个 commit 的复验（真实 daemon/ACP 表面,与 df22089 A/B 对比）
结论:两个新 commit(96e9fd01f /dream ACP 延迟记录、13bb4df5c cron span 状态)所声称的行为全部属实 —— 在真实 qwen serve daemon 驱动 ACP Session 的表面上端到端验证,并与上一个 head 做了决定性 A/B。建议保留。但两个 commit 都没有触碰交互式 useGeminiStream 路径,因此今早报告的 Finding 1(运行在第一个流式段结束时即被定格为 success;tick 其余全程 Esc 取消失效)在 13bb4df5c 上代码完全相同,仍是唯一的合并阻塞项。
环境
- A/B 产物:old =
df22089(今早报告验证的 head)vs new =13bb4df5c,均为完整真实构建(npm run build+npm run bundle,tsc 0 错误;两 bundle 可证不同 —— 旧 bundle 含 eagerrecordDream().catch调用点,新无)。 - 真实表面:每场景在 tmux 中起
node dist/cli.js serve,隔离$HOME+ 临时 git 仓库,QWEN_CODE_MEMORY_LOCAL=1(可观察产物为<proj>/.qwen/meta.json),mock OpenAI 兼容 provider 可控结局(挂起 / 500 / 立即成功)。REST 驱动:POST /session→POST /session/:id/prompt"/dream"→ 在决定性时刻采样meta.json(lastDreamAt是否存在)。dream已确认出现在GET /session/:id/supported-commands。
/dream ACP 延迟记录 —— 7 次运行,每格都有决定性结果
场景(REST 提交 /dream) |
old df22089 |
new 13bb4df5c |
|---|---|---|
| turn 进行中采样(provider 挂起响应) | ❌ lastDreamAt 已写入 —— turn 未跑完即 eager 写 |
✅ 缺席(延迟) |
| turn 成功 → turn 结束时记录 | 已写(更早、eager) | ✅ "prompt turn completed" 后 22ms 写入 |
| turn 失败(观察到 provider 500 ×5 重试) | ❌ 失败的 consolidation 仍被持久化记录 | ✅ 始终未写(数分钟后复查仍干净) |
turn 被取消(挂起中 POST /session/:id/cancel) |
❌ 被取消的 turn 仍被记录 | ✅ 始终未写 |
取消闭环(同一会话):取消 turn 1,再跑第 2 个成功的 /dream |
n/a(eager 写使其无意义) | ✅ turn 1 → cancel 后 24ms daemon 记 "prompt turn failed"、无记录;turn 2 → 完成并记录。被取消的 slash turn 之后,会话与 pendingSlashOnComplete 机制保持健康 |
单测钉住验证:两个更新后的 ACP 测试对旧实现恰好失败(旧 dreamCommand.ts × 新测试 → 失败的正是 defers writeDreamManualRun… 与 …cancelled 两条),新 head 上通过 —— 测试真实钉住了修复。dreamCommand + Session 套件 132/132;autoImproveCommand/autoImproveState/useGeminiStream 套件 177/177;cronScheduler 通过。与今日 main(b794d64f)合并无冲突(git merge-tree --write-tree 干净),GitHub CI 三平台全绿。
13bb4df5c(cron span 状态)—— 结构性验证
cron tick 的 interaction span 状态由 ac.signal.aborted ? 'cancelled' : cronHadError ? 'error' : 'ok'(Session.ts:2192)决定。responseStream 为 null 且 stop reason 非 cancelled(如 max_tokens)的分支此前把运行记为 failed 但 cronHadError 保持 false → span 报 ok;该 commit 在此分支补设 cronHadError=true,与另一错误分支一致。代码路径阅读证实其声称;注意该分支的 span 状态没有测试钉住(轻微 —— 可保持现状,或在 Session.test.ts 加一行断言锁定)。
Finding 1 为何仍然开放(且未变)
git diff df22089..13bb4df5c 只触及 dreamCommand.ts/.test.ts 与 Session.ts —— useGeminiStream.ts、autoImproveCommand.ts、autoImproveState.ts 与我今早实测的 head 逐字节相同,因此那份报告对交互式表面的全部观察依然成立:第一段即 success、Esc 仅数秒窗口、去重保护无法触发、被取消的运行被补账为 success。本轮新增一个范围说明:新的 ACP 延迟记录之所以正确,是因为 Session.ts 在真正的 turn 结束时触发 onComplete —— 而交互式路径在第一个流式段结束时就触发,所以交互式 /dream 在涉及工具调用的 consolidation 上同样会中途记录(同一根因,并非 auto-improve 独有)。修好交互式 onComplete 时机,Finding 1 与此一并解决。
另外观察到(预存在的 daemon 行为,新旧两侧一致,不属于本 PR 范围):取消 slash 提交的 turn 时,child 会记一条嘈杂的 RPC 错误 "Not currently generating"(尽管取消实际生效),且 daemon WARN 行把失败原因渲染为 [object Object]。
建议
与今早一致,且 delta 已验证:两个新 commit 可靠 —— PR 在合并前仍需修复交互式 onComplete/Esc 取消(Finding 1)。 今早报告的发现 2–6 仍为后续跟进级别。
Summary
/auto-improveslash command that lets Qwen Code run a session-scoped loop for small, locally verifiable repository improvements. The command supports source configuration, loop start/status/stop controls, scheduled ticks, and local state tracking for each run./auto-improve sourcesupports built-in source toggles plus an editable custom source list. Users can add multiple custom source hints, edit existing hints, delete hints, and save them into the repository-level auto-improve config./auto-improve stop.Validation
/auto-improve sourceopens source configuration, supports adding/editing/deleting custom source entries, and/auto-improve start --every <interval> [prompt]snapshots those custom sources into the loop and schedules/auto-improve tick <loop-id>. Tick instructions require PR-derived tasks to use PR head branches and prohibit push unless explicitly requested./auto-improve sourceand/auto-improve start --every 30min the CLI with cron enabled.Scope / Risk
/auto-improve. Existing draftuserContextconfig is migrated into the new custom source list on read.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
No linked issues.