fix(core): sync loaded-skill state with history eviction - #9500
Conversation
Refs #6762 Split from PR #8900 per maintainer direction review: this keeps only the eviction-state sync half (loaded-skills tracking reconciled with history evictions across microcompaction, /compress-fast, memory pressure compaction, client retry, and ACP settle). The /unskill command is deferred to a separate change pending the #6762 design discussion.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @ZijianZhang989 — the PR body doesn't follow the PR template, so the gate stops here before any code review.
What's missing:
## What this PR doesand## Why it's needed— the body uses## Changes/## Motivationinstead- Under
## Reviewer Test Plan: the### How to verify,### Evidence (Before & After), and### Tested on(OS table) subsections ## Risk & Scope## Linked Issues— the #6762 / #8900 references are only inline in the prose- The bilingual
<details>summary
Your earlier PR (#8900) followed the template in full, so this looks like an oversight. Could you reformat the body into the template shape — the existing content maps over almost 1:1? Once it's updated, re-run with @qwen-code /triage and the gate will pick it up.
中文说明
@ZijianZhang989 你好——这个 PR 的描述没有按照 PR 模板填写，所以门禁先停在这里，暂不进入代码审查。
缺少的部分：
## What this PR does和## Why it's needed——目前写成了## Changes/## Motivation## Reviewer Test Plan下的### How to verify、### Evidence (Before & After)和### Tested on（操作系统表格）小节## Risk & Scope## Linked Issues——目前 #6762 / #8900 只是在正文里顺带提到- 双语
<details>中文总结
你之前的 PR（#8900）是完整按模板写的，这次看起来只是疏忽。麻烦把 PR 描述改成模板格式——现有内容基本可以一一对应地搬过去。改好后回复 @qwen-code /triage 重新触发门禁即可。
— Qwen Code · qwen3.8-max
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. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Code Review — PR #9500
PR: fix(core): sync loaded-skill state with history eviction
Scope: 20 files (+1919/-10) — 990 source lines, 1493 test lines
Summary
This PR makes the Skill tool's loaded-skills tracking agree with what is actually resident in conversation history. The core invariant — that the dedup guard's tracking must follow what history actually holds — is correctly established across all eviction paths: microcompaction, compress-fast, memory-pressure compaction, ACP session settlement, history truncation, orphan-entry stripping, and client retry restore.
Verdict
COMMENT — no Criticals found. The code is well-structured, the tests are comprehensive, and the invariant is correctly established. Two Suggestions and two Nice-to-haves below.
Issue Fidelity
The PR correctly addresses the eviction-state sync gap from issue #6762 (item 2: microcompaction of skill bodies). The root cause is a client bug: SkillTool.loadedSkillNames was not maintained in sync with history eviction paths. This is not an upstream provider issue.
Build & Test
packages/core: built successfully (59s)packages/cli: build blocked by pre-existingpackages/audio-capturenode-gypfailure (Python not found on this Windows machine — unrelated to the PR)- Test-efficacy probes confirmed 3 gated files (memoryPressureMonitor, microcompact, skill-utils) — tests would catch a revert of the change
Findings
Suggestion 1: Dead code — isSkillDedupConfirmation exported but unused
File: packages/core/src/tools/skill-utils.ts:61-69
The exported function isSkillDedupConfirmation is defined in the diff but never imported or called anywhere in the repository. The eviction-suppression logic in buildKeptSkillNames uses isSkillBodyOutput (positive matching) instead, so this function has no consumer.
Suggested fix: Remove the function (and its export) unless it has a planned consumer in a follow-up commit.
Suggestion 2: Untested unresolvable-body branch in unloadSkillsFromEntries
File: packages/core/src/tools/skill-utils.ts (in the unloadSkillsFromEntries function)
The blanket-clear branch (when any stripped body's call id cannot be resolved to a name) has no dedicated test. The geminiChat test exercises only the "all resolvable" path.
Failure scenario: If a future change introduces a bug in this branch, the unresolvable skill would stay tracked despite its body being gone, causing the dedup guard to return "already loaded in context" for a body that no longer exists.
Suggested fix: Add a test case where stripOrphanedUserEntriesFromHistory returns a skill body entry with a missing or unresolvable call id.
Nice to have 1: Missing isForkedChat guard on truncateHistory reconciliation
File: packages/core/src/core/geminiChat.ts:4369-4378
truncateHistory calls reconcileLoadedSkillTracking unconditionally but never checks isForkedChat. The tryCompress and hard-rescue restore paths both guard this. No existing caller passes a forked chat to truncateHistory, so this is a latent defect.
Nice to have 2: Missing isForkedChat guard on stripOrphanedUserEntriesFromHistory
File: packages/core/src/core/geminiChat.ts:4438-4442
Same pattern as above — the method calls unloadSkillsFromEntries unconditionally with no isForkedChat guard.
Vendor Verdict
Approved by all 13 review agents. 8 chunk agents, Issue Fidelity, Removed-behavior audit, Cross-file tracer, Test coverage matrix, and Build & Test all returned clean or non-blocking findings. The code is correct, well-tested, and maintains the established invariants.
— Qwen Code /review (v0.21.13)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
The body has since been reformatted to follow the PR template in full (all sections: What this PR does / Why it's needed / Reviewer Test Plan with How to verify, Evidence, Tested on, Environment / Risk & Scope / Linked Issues / bilingual Chinese summary). The stage 1a result above was run against the initial draft before the reformat. @qwen-code /triage |
The dedup identity keyed only on type + command/url, but the frontmatter admits multiple hooks per matcher distinguished solely by fields that key ignores (timeout/shell for command hooks, headers/timeout for HTTP hooks): the second of such a pair was silently skipped even on the skill's first registration. Key on the whole prepared config instead — frontmatter configs carry no functions, so the structural key stays stable across reload cycles. Adds regression tests for both shapes.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
[Critical] R2-2: [Critical] Wholesale history replacement has no loaded-skill reconcile — the PR's stated invariant ('the guard state must follow the history it guards', 'this PR establishes that invariant on every eviction and restore path') misses the restore entrances. Class finding, two verified entrances: (1) GeminiClient.setHistory (client.ts:785) — used by /restore (restoreCommand.ts:152) and the session-manager load_history action (slashCommandProcessor.ts:1317) — replaces history and clears FileReadCache but performs no skill-tracking reconcile; (2) ACP restoreSessionHistory → Session.restoreHistory (Session.ts:3515) calls GeminiChat.setHistory directly, bypassing the client wrapper, so a fix anchored only at GeminiClient.setHistory structurally cannot reach it. Every sibling rewrite path this diff touches syncs (pre-send microcompaction, /compress-fast, memory-pressure compact_history, tryCompress, truncateHistory, strip, retry restore, hard-rescue, ACP settle); the root GeminiChat.setHistory is the one door left open. Probe-verified at this commit: tracking a loaded skill then calling setHistory with a checkpoint history lacking its body leaves the name tracked with no resident body (probe: expected [ 'demo' ] to deeply equal [] on both the PR tree and the merge-base tree); adding a reconcile flips the probe green (Tests 1 passed (1)); second entrance verified end-to-end (setHistory(preLoadSnapshot): clearLoadedSkills=0 unloadSkills=0 trackSkills=0 / X still tracked with no resident body: true, distinguishing arm truncateHistory(2): clearLoadedSkills=1). Failure: load skill X, then /restore (or an ACP session restore) to a snapshot predating the load — the dedup guard returns Skill "X" is already loaded in context. on every subsequent invocation; X is unusable until /clear or session restart. This is the exact #6762 deadlock class this PR fixes on every other path, still reachable through the restore door. Lineage: the #8900 review deferred /restore + restoreHistory + startNewSession sync to a follow-up, but #8900 closed unmerged and no follow-up issue exists — the deferral currently has no tracked home, and this PR's thread had not discussed the class. Disposition to record explicitly: fix here (reconcile inside GeminiChat.setHistory covers all three entrances in one invariant — residency is fully knowable from the new history), or file the follow-up issue the #8900 deferral promised; correct the 'every eviction and restore path' description sentence either way.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
[Critical] R2-2: [Critical] Wholesale history replacement has no loaded-skill reconcile — the PR's stated invariant ('the guard state must follow the history it guards', 'this PR establishes that invariant on every eviction and restore path') misses the restore entrances. Class finding, two verified entrances: (1) GeminiClient.setHistory (client.ts:785) — used by /restore (restoreCommand.ts:152) and the session-manager load_history action (slashCommandProcessor.ts:1317) — replaces history and clears FileReadCache but performs no skill-tracking reconcile; (2) ACP restoreSessionHistory → Session.restoreHistory (Session.ts:3515) calls GeminiChat.setHistory directly, bypassing the client wrapper, so a fix anchored only at GeminiClient.setHistory structurally cannot reach it. Every sibling rewrite path this diff touches syncs (pre-send microcompaction, /compress-fast, memory-pressure compact_history, tryCompress, truncateHistory, strip, retry restore, hard-rescue, ACP settle); the root GeminiChat.setHistory is the one door left open. Probe-verified at this commit: tracking a loaded skill then calling setHistory with a checkpoint history lacking its body leaves the name tracked with no resident body (probe: expected [ 'demo' ] to deeply equal [] on both the PR tree and the merge-base tree); adding a reconcile flips the probe green (Tests 1 passed (1)); second entrance verified end-to-end (setHistory(preLoadSnapshot): clearLoadedSkills=0 unloadSkills=0 trackSkills=0 / X still tracked with no resident body: true, distinguishing arm truncateHistory(2): clearLoadedSkills=1). Failure: load skill X, then /restore (or an ACP session restore) to a snapshot predating the load — the dedup guard returns Skill "X" is already loaded in context. on every subsequent invocation; X is unusable until /clear or session restart. This is the exact #6762 deadlock class this PR fixes on every other path, still reachable through the restore door. Lineage: the #8900 review deferred /restore + restoreHistory + startNewSession sync to a follow-up, but #8900 closed unmerged and no follow-up issue exists — the deferral currently has no tracked home, and this PR's thread had not discussed the class. Disposition to record explicitly: fix here (reconcile inside GeminiChat.setHistory covers all three entrances in one invariant — residency is fully knowable from the new history), or file the follow-up issue the #8900 deferral promised; correct the 'every eviction and restore path' description sentence either way.
— qwen3.8-max via Qwen Code /review (v0.21.14)
E2E Verification: Skill Eviction SyncVerifier: Maintainer review Core Scenario Verified End-to-EndThe bug this PR fixes is: when history eviction blanks a skill body, Verification chain: E2E TUI Test (tmux capture)
Full tmux transcript: 2,288 lines with step-by-step snapshots. Unit Tests
Key Dedup Tests (4/4 PASS)
All 3 Eviction Paths Covered
Safety Guards Verified
ConclusionThe skill eviction sync is correct and complete. All eviction paths call |
|
Re R2-2 (Critical, from the round-2 review body): fixed. 关于 R2-2（Critical，位于第二轮 review body 中）：已修复。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-5 untested strip-path fork guard — already overlapped at geminiChat.test.ts:413 (comment 3816706380, by qwen-code-ci-bot)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/skill-utils.ts:24 — [review] residency-marker docstring references a nonexistent /unskill placeholder (unchanged since round 2 — deferred by the convergence posture)packages/core/src/services/memoryPressureMonitor.test.ts:1535 — [probe] compact_history test never asserts what history setHistory installs (unchanged since round 2 — deferred by the convergence posture)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Independent local verification — request changes
I tested the exact PR head 6f085bd9e11712871f3db456dd57c87c13748612 against merge-base a8a855914bc7f7d40161466b6aef24b7a6a62ffa in isolated worktrees on macOS 26.5.2 arm64 (Node.js 24.18.0).
What passed
- Both exact commits completed
npm ci --ignore-scripts, repositorypostinstall, CLI-only build, and bundle generation. I then ran the real bundled CLI through a PTY against a deterministic local OpenAI-compatible streaming server, with a real project Skill and real shell-tool turn. - The identical session loaded
pr9500-probe, added a newer shell result, ran/compress-fastwithQWEN_MC_KEEP_RECENT=1, and invoked the Skill again. At the merge base the second invocation returnedSkill "pr9500-probe" is already loaded in context.; at this PR head it returned the complete Skill body. This is user-visible E2E evidence that the primary #6762 failure mode is fixed. - A second in-process production-path probe using the real
SkillToolandGeminiChat.compressFast()produced the same base-fail/head-pass result. - Nine touched Core suites passed: 9 files / 1,329 tests (
client,geminiChat, skill hooks, permissions, memory-pressure compaction, microcompaction, skill utilities, SkillTool, and forked-agent cache).
What still fails
- Merge blocker — residency-marker spoofing is reproducible. A newer same-named command result containing the two public marker strings is accepted as a real resident Skill body. In a production
microcompactHistoryprobe, the genuine older body was cleared (toolsCleared = 1) butevictedSkillNameswas[]instead of['demo']. The dedup tracker therefore remains armed after the real body is gone. I added the exact local evidence to the existing line-level Critical thread rather than duplicating the finding. - Confirmed non-blocking risk — post-
setHistorydouble sync degrades ground truth. A stateful production-function probe first reconciled the surviving history to['skill-b']; the immediately followingsyncSkillEvictions(... unresolvedEvictedSkills: 1)changed it to[]. I added this evidence to the existing line-level thread.
Scope disclosure
The local E2E exercised the /compress-fast user-facing path. Retry restoration, ACP continuation, memory-pressure compaction, and fork isolation were covered by the passing focused Core suites but were not each driven through a separate external client. On the PR, the Ubuntu unit-test job is green, while Integration Tests (CLI, No Sandbox) is skipped.
Recommendation
Do not merge this head. The main fix is real, but residency needs authoritative provenance from the actual Skill-body production event (or an unforgeable/session-scoped identity), not two public text literals. After that is fixed, rerun the before/after production probe and the command-output collision probe. The redundant post-setHistory sync should also be removed or prevented from blanket-clearing an exact reconciliation.
中文版本
独立本地验证——请求修改
我在隔离 worktree 中对精确 PR head 6f085bd9e11712871f3db456dd57c87c13748612 与 merge-base a8a855914bc7f7d40161466b6aef24b7a6a62ffa 做了对照测试。环境为 macOS 26.5.2 arm64、Node.js 24.18.0。
已通过的部分
- 两个精确提交都完成了
npm ci --ignore-scripts、仓库postinstall、CLI-only build 与 bundle。我随后通过真实 PTY 运行 bundle，连接本地确定性 OpenAI-compatible 流式模型服务，并使用真实项目 Skill 和真实 shell 工具回合。 - 完全相同的会话先加载
pr9500-probe，再加入较新的 shell 结果，在QWEN_MC_KEEP_RECENT=1下执行/compress-fast，然后再次调用该 Skill。merge-base 的第二次调用返回Skill "pr9500-probe" is already loaded in context.；当前 PR head 则重新返回完整 Skill 正文。这是 #6762 主要故障路径已修复的用户可见 E2E 证据。 - 第二个使用真实
SkillTool与GeminiChat.compressFast()的进程内生产链路探针，也得到了相同的 base 失败、head 通过结果。 - 受影响的 9 个 Core 测试文件全部通过，共 1,329 个测试，覆盖 client、GeminiChat、Skill hooks、权限、内存压力压缩、微压缩、Skill 工具与 forked-agent cache。
仍然失败的部分
- 合并阻断项——可复现正文驻留标记伪造。 一个较新的同名命令结果只要包含两个公开字符串，就会被当成真正驻留的 Skill 正文。在生产
microcompactHistory探针中，真实旧正文已被清除（toolsCleared = 1），但evictedSkillNames得到[]，而不是['demo']。因此真实正文消失后，去重台账仍保持已加载状态。我已将精确本地证据回复到现有 Critical 行级线程，避免重复报告。 - 已确认的非阻断风险——
setHistory后的二次同步会破坏准确状态。 状态化生产函数探针先根据幸存历史准确对账为['skill-b']，紧接着执行syncSkillEvictions(... unresolvedEvictedSkills: 1)后变成[]。我已将证据补充到对应行级线程。
范围披露
本地 E2E 驱动了用户可见的 /compress-fast 路径。重试恢复、ACP continuation、内存压力压缩与 fork 隔离由已通过的聚焦 Core 测试覆盖，但没有分别通过外部客户端逐条驱动。PR 的 Ubuntu 单测任务通过，但 Integration Tests (CLI, No Sandbox) 被跳过。
建议
当前 head 不建议合并。 主修复真实有效，但 Skill 正文驻留应使用正文实际生成时记录的权威来源，或不可伪造/会话级身份，而不是两个公开文本常量。修复后应重新运行主路径前后对照探针和命令输出碰撞探针。同时建议移除 setHistory 后的冗余同步，或确保它不能整体清除刚得到的准确对账结果。
- Record exact body outputs in SkillTool as residency provenance; all
residency checks fail closed against a fresh process (empty set)
instead of trusting the two public markers (spoofable via the shared
functionResponse{name:'skill'} shape)
- Make setHistory's reconcile the single loaded-skill sync: remove the
five post-setHistory second writes (3x syncSkillEvictions, hard-rescue
reconcile, tryCompress blanket clear) and delete the now-dead helpers
- Correct the settle-reconcile comment to name its five pre-try skip
paths; resume stays intentionally un-enumerated (fail-closed reconcile
plus one bounded duplicate body beats a marker-based resume door that
would reopen the injection window)
- Tests: provenance gating (injection/genuine/spoof-stripped), stateful
Set oracle for clear-before-track order, microcompact F2 witness and
size-path kept-body twin, mixed-batch unload, isRetry settle arming,
wire-after-setup hard-rescue, reconcile-only tryCompress, and the
three second-write sites flipped to single-writer assertions
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D4-2 body-blanked/confirmation-kept suppression-ordering test gap — already reported at microcompact.test.ts:2307 (comment 3816706369)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 4 and 5 still reported findings.
Not explored to full depth (tool budget reached): "agent 1c": verify whether AgentHeadless.create (fork-resume path in background-agent-resume.ts ) rebuilds the ToolRegistry or leaves the parent's registry shared — this….
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.test.ts:443 — [probe] strip path's inline isForkedChat guard has zero test coverage (deleting it leaves 336 tests green)packages/core/src/tools/skill.ts:301 — [review] unloadSkills JSDoc names three callers that never call itpackages/core/src/services/microcompaction/microcompact.test.ts:2429 — [probe] R2-3 never pins unresolvedEvictedSkills (ambiguity branch of the counter unguarded)packages/core/src/services/microcompaction/microcompact.ts:761 — [probe] size-path pending-exclusion invariant untested — revert mutant ships green
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 4 and 5 still reported findings。
未探索到全部深度（达到工具调用预算）："agent 1c"：verify whether AgentHeadless.create (fork-resume path in background-agent-resume.ts ) rebuilds the ToolRegistry or leaves the parent's registry shared — this…。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
The scheduler's persistence gate rewrites large genuine skill bodies into <persisted-output> stubs before they enter history. The strip path's provenance/shape check skipped such stubs, leaving the skill tracked with no resident body — the #6762 deadlock. unloadSkillsFromEntries now classifies every stripped skill response by call-id resolution: fail-open direction, since over-un-tracking self-heals with one duplicate body while under-un-tracking deadlocks reload; the resident-sibling filter keeps precision. Also pins the R4 findings: provenance recording both halves through the real SkillTool, microcompact provenance wiring at the client call site, the ambiguous call-id policy on both strip and reconcile sides, provenance-mode targeted unload, compressFast's setHistory reconcile, and the forked-chat compression/restore observable effects. Rewrites the evictedSkillNames docstrings to the diagnostic-only contract.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1b": (none — no check was cut short); chunk 5: none — no checks were left unfinished..
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.test.ts:456 — [review] strip path's isForkedChat guard has zero test coverage (round-4 deferral re-surfaced)packages/core/src/core/geminiChat.test.ts:375 — [probe] tryCompress NOOP-path tracking test lacks a positive anchor that the branch ran (deferred by the code-age rule — anchor unchanged since round 4)packages/core/src/services/microcompaction/microcompact.test.ts:2548 — [probe] provenance promise's eviction half — blanked marker-spoof with provenance wired has no twin test (deferred by the code-age rule — anchor unchanged since round 4)packages/core/src/tools/skill-utils.ts:42 — [review] isSkillBodyOutput docstring claims marker matching 'Proves residency' — refuted by the PR's own provenance model (deferred by the code-age rule — anchor unchanged since round 4)packages/core/src/core/geminiChat.test.ts:458 — [probe] zero-skill strip arm (dropped.size === 0 early return) has no tracker-untouched test (deferred by the code-age rule — anchor unchanged since round 4)packages/core/src/tools/skill.ts:302 — [review] unloadSkills JSDoc names three rewrite callers that never call it (round-4 deferral re-surfaced)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1b"：(none — no check was cut short)；chunk 5：none — no checks were left unfinished.。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| genuineSkillBodyOutputs: getGenuineSkillBodyOutputs( | ||
| this.coreConfig.getToolRegistry(), | ||
| ), |
There was a problem hiding this comment.
[Suggestion] R4-4: Still stands — 2 of the 3 call sites remain unpinned. The round-4 finding asked for pinning all three production wirings of genuineSkillBodyOutputs into microcompactHistory; only the client pre-send site got a wiring test. Here (memory-pressure compact_history) and at the compressFast site (geminiChat.ts:2213), the dedicated tests stub the skill tool WITHOUT getGenuineSkillBodyOutputs and never spy on the options argument, so deleting this option ships green. Probe at this commit: dropping the option at both cited sites passes 524/524 tests, while the identical mutation at the pinned client site fails (expected undefined to be Set{ 'recorded-body' }). The R3-1 residency gate would then silently degrade to marker-only mode on this path, where a command-delegation output copying the two public marker strings could shield or fake a body residency — bounded today because evictedSkillNames is diagnostic-only, but the gate exists to stay correct when that changes.
Suggested fix: mirror the client-site wiring test here — give the stubbed skill tool getGenuineSkillBodyOutputs: () => sentinel returning a sentinel set, spy on microcompactHistory, and assert the sentinel reaches the options argument verbatim (calls[0][3].genuineSkillBodyOutputs).
中文说明
[建议] R4-4：仍然存在——三个调用点中仍有两个未被测试钉住。第 4 轮的发现要求为 genuineSkillBodyOutputs 传入 microcompactHistory 的全部三个生产调用点加钉住测试；最终只有 client 预发送调用点获得了接线测试。此处（内存压力 compact_history）与 compressFast 调用点（geminiChat.ts:2213）的专属测试在 stub skill tool 时未提供 getGenuineSkillBodyOutputs，也从未对 options 参数做 spy——因此删除该选项可以全绿通过。在本提交上探针验证：在两处被指位置删除该选项后 524/524 个测试全部通过；而在已钉住的 client 调用点做同样改动则失败（expected undefined to be Set{ 'recorded-body' }）。届时 R3-1 驻留门控将在本路径上静默退化为仅标记模式，复制两个公开标记字符串的命令委托输出即可掩护或伪造正文驻留——当前因 evictedSkillNames 仅为诊断用途而影响有界，但该门控的存在正是为了在有消费方时依然正确。
建议修复：在此处镜像 client 调用点的接线测试——为 stub 的 skill tool 提供 getGenuineSkillBodyOutputs: () => sentinel 返回哨兵集合，spy microcompactHistory，并断言哨兵原样到达 options 参数（calls[0][3].genuineSkillBodyOutputs）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| genuineSkillBodyOutputs: getGenuineSkillBodyOutputs( | ||
| this.config.getToolRegistry(), | ||
| ), |
There was a problem hiding this comment.
[Suggestion] R4-4: Twin location of the finding above — the compressFast call site. Same shape as the memory-pressure site: the R4-9 compressFast test stubs the skill tool without getGenuineSkillBodyOutputs and never observes the options argument, so deleting this wiring ships green (probe-verified: 524/524 pass with both sites dropped; the identical mutation at the pinned client site fails). The fix is the same pin: sentinel set on the stub, spy on microcompactHistory, assert the sentinel reaches calls[0][3].genuineSkillBodyOutputs verbatim.
中文说明
[建议] R4-4：上一条发现的孪生位置——compressFast 调用点。形态与内存压力调用点相同：R4-9 的 compressFast 测试在 stub skill tool 时未提供 getGenuineSkillBodyOutputs，也从不观察 options 参数，因此删除此接线可全绿通过（探针验证：两处同时删除后 524/524 通过；在已钉住的 client 调用点做同样改动则失败）。修复方式同为该钉住：stub 返回哨兵集合、spy microcompactHistory、断言哨兵原样到达 calls[0][3].genuineSkillBodyOutputs。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // Record the blanked skill so the caller un-tracks it from | ||
| // loadedSkillNames — otherwise the dedup guard keeps returning | ||
| // "already loaded in context" for a body that no longer exists. |
There was a problem hiding this comment.
[Suggestion] R5-4: This recording-site comment contradicts the normative contract the same diff states on the very fields it populates. evictedSkillNames/unresolvedEvictedSkills are documented "DIAGNOSTIC ONLY — no production consumer … callers must NOT second-write tracking from this field" and "Callers must NOT clear tracking based on this field" (microcompact.ts:624-643), while this comment says the recording exists "so the caller un-tracks it" and that "counting them as unresolved would force a blanket clear". The wording is carried over from the adjacent file-path block, whose contract really IS caller-consumed (evictedReadPaths/unresolvedEvictedReads feed disarmFileReadCacheAfterEviction); the two skill fields have zero production consumers (repo-wide sweep). Git provenance dates the staleness: the comment was accurate while a consumer existed in skill-utils.ts; the consumer was removed, and this round's docstring rewrite (the R4-2 fix) made the contradiction explicit. Failure shape: a maintainer debugging a dedup-guard deadlock reads this comment and wires a post-microcompaction consumer that un-tracks evictedSkillNames or blanket-clears on unresolvedEvictedSkills — precisely the removed post-setHistory second write the R3-2 tests pin as forbidden.
// Record the blanked skill for the diagnostic meta only — tracking
// itself is rebuilt from the applied history by setHistory's
// reconcileLoadedSkillTracking at every application site. Do NOT
// un-track from these fields. Only a body proves residency/eviction;
// non-body outputs (SkillTool errors, dedup confirmations) never created
// tracking, so counting them here would misreport residency.中文说明
[建议] R5-4：此记录点的注释与同一 diff 在其所填充字段上声明的规范契约相矛盾。evictedSkillNames/unresolvedEvictedSkills 的文档写明"仅诊断用途——无生产消费方……调用方不得据此二次写入台账""调用方不得基于本字段清除台账"（microcompact.ts:624-643），而此注释却说记录的存在是"为了让调用方取消跟踪"，并声称"计入 unresolved 会强制整体清除"。措辞沿用自相邻的文件路径代码块——那一块的契约确实由调用方消费（evictedReadPaths/unresolvedEvictedReads 供 disarmFileReadCacheAfterEviction 使用）；而两个 skill 字段在全仓扫描中零生产消费方。git 溯源确定了陈旧化的时间点：当 skill-utils.ts 中尚存在消费方时该注释是准确的；消费方被移除后，本轮的 docstring 重写（R4-2 修复）使矛盾显性化。故障形态：调试去重守卫死锁的维护者读到这条注释，进而接入一个微压缩后的消费方，按 evictedSkillNames 取消跟踪或按 unresolvedEvictedSkills 整体清除——恰好重建 R3-2 测试钉住为禁止项的 setHistory 后二次写入。
建议将注释改为上方代码块所示的诊断专用契约。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // Provenance recording, negative half (R4-3): delegated output | ||
| // must NOT enter the genuine set, or marker-shaped command text |
There was a problem hiding this comment.
[Suggestion] R5-6: The R4-3 fix pinned the provenance-recording negative half ("delegated output must NOT enter the genuine set") on the not-found command-fallback branch only. The disabled-skill fallback branch in skill.ts is a second, duplicated delegation return path, and the 'disabled-skill execute guard' tests assert neither getLoadedSkillNames nor getGenuineSkillBodyOutputs. Probe at this commit: a mutant adding recordGenuineBodyOutput(content) to the disabled-fallback string branch ships green (83/83); adding the two assertions below fails the mutant (expected [ 'MCP prompt body' ] to deeply equal []) and passes on unmodified code. Concrete path: skill foo disabled while a same-named MCP prompt exists → the prompt's output copies the two public marker literals and enters genuineSkillBodyOutputs → the next setHistory reconcile passes isProvenSkillBody for it and tracks foo → after re-enabling foo, the dedup guard returns "already loaded in context" while the resident text is the command output — the R1-14 deadlock vector reaching through the unpinned branch.
// in 'runs the same-named MCP prompt instead of loading a disabled skill':
expect([...skillTool.getLoadedSkillNames()]).toEqual([]);
expect([...skillTool.getGenuineSkillBodyOutputs()]).toEqual([]);中文说明
[建议] R5-6：R4-3 的修复只在"未找到时回退到命令"分支上钉住了凭证记录的负半部（"委托输出不得进入凭证集合"）。skill.ts 中被禁用技能的回退分支是第二条、重复的委托返回路径，而 'disabled-skill execute guard' 测试对 getLoadedSkillNames 与 getGenuineSkillBodyOutputs 均未断言。在本提交上探针：向禁用回退的字符串分支加入 recordGenuineBodyOutput(content) 的突变体全绿通过（83/83）；加入下述两条断言后该突变体失败（expected [ 'MCP prompt body' ] to deeply equal []），且在未改动代码上通过。具体链路：技能 foo 被禁用而同名 MCP prompt 存在 → prompt 输出复制两个公开标记字面量并进入 genuineSkillBodyOutputs → 下一次 setHistory 对账经 isProvenSkillBody 放行并把 foo 入册 → 重新启用 foo 后，去重守卫回答 "already loaded in context"，而驻留文本其实是命令输出——R1-14 死锁向量经由未钉住的分支复活。
修复：在 'runs the same-named MCP prompt instead of loading a disabled skill' 测试中镜像 not-found 测试的两条负断言（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| expect(trackSkills).toHaveBeenCalledWith(['demo']); | ||
| }); | ||
|
|
||
| it('a spoofed stripped response cannot un-track a skill with a resident body (R4-1)', () => { |
There was a problem hiding this comment.
[Suggestion] R5-9: The provenance gating of unloadSkillsFromEntries' resident-sibling shield (skill-utils.ts:552-554, resolveLoadedSkillNames(history, history, genuineOutputs)) has no discriminating test: every unload-path test runs either with a genuine resident — which a marker-only filter would shield just the same — or with a tracker mock lacking getGenuineSkillBodyOutputs (marker-only mode). Probe at this commit: the mutant "drop the genuineOutputs argument from the resident scan" ships green across skill-utils + microcompact + the full geminiChat suite (451/451); the twin below fails under the mutant (expected 'spy' to be called with arguments: [ [ 'demo' ] ] — the marker-only scan counts the spoof as resident and shields the un-track) and passes on correct code. Failure shape this would let ship: skill demo loaded (body B, tracked) → truncation drops B and reconcile un-tracks → demo disabled; a same-named MCP prompt's output S copying both public markers persists → demo re-enabled, genuine reload B2 → request fails right after B2 and the strip pops it. Correct code excludes S (not in the genuine set) and un-tracks demo; the marker-only mutant keeps demo tracked with no resident body → every later invocation returns "already loaded in context" — the #6762 deadlock shape.
// twin next to the R4-1 test:
// provenanceTracker(new Set([genuineBody]));
// history: [skillCall('s1','demo'), skillBody('s1', spoof), skillCall('s2','demo')]
// entries: [skillBody('s2', genuineBody)]
expect(clearLoadedSkills).not.toHaveBeenCalled();
expect(unloadSkills).toHaveBeenCalledWith(['demo']);中文说明
[建议] R5-9：unloadSkillsFromEntries 的驻留同胞掩护（skill-utils.ts:552-554，resolveLoadedSkillNames(history, history, genuineOutputs)）的凭证门控没有可区分的测试：所有 unload 路径测试要么使用真实正文作为驻留者（仅标记过滤同样会掩护），要么使用缺少 getGenuineSkillBodyOutputs 的 tracker mock（仅标记模式）。在本提交上探针：将驻留扫描的 genuineOutputs 实参删除的突变体在 skill-utils + microcompact + 完整 geminiChat 套件全绿（451/451）；下方孪生测试在该突变体下失败（expected 'spy' to be called with arguments: [ [ 'demo' ] ]——仅标记扫描把伪造结果计为驻留并掩护了取消跟踪），在正确代码上通过。该缺口可能放行的故障形态：技能 demo 加载（正文 B，入册）→ 截断丢弃 B，对账取消跟踪 → demo 被禁用，同名 MCP prompt 输出 S 复制两个公开标记并留存 → demo 重新启用、真实重载 B2 → B2 之后请求失败、剥离弹出该条。正确代码排除 S（不在凭证集）并取消跟踪 demo；仅标记突变体则让 demo 在无正文的情况下留在册中 → 之后每次调用都返回 "already loaded in context"——#6762 死锁形态。
修复：在 R4-1 测试旁新增上述孪生测试。
— qwen3.8-max via Qwen Code /review (v0.21.15)
E2E Verification — reproduced issues confirmed fixedTested on PR HEAD Setup
Test Flow & Terminal TranscriptStep 1: Load ponytail skill → body injected ✅Step 2: /context detail before compression → tracking matches residency ✅← Step 3: /compress → skill body evicted from history ✅After compression, /context detail shows: ← No Step 4: Re-invoke ponytail AFTER compression → full body returned (KEY TEST) ✅← No Step 5: /context detail after reload → tracking consistent again ✅← Tracking rebuilt correctly after reload. No double-sync corruption (jifeng #2). Verdict
Both issues reproduced on — Verified via tmux E2E on macOS arm64, Node.js v26.2.0 |
E2E Verification — Before/After comparison confirming jifeng's issues are fixedTested both Setup
Identical Test Flow
BEFORE (main branch) — Bug reproduced ❌The dedup guard believes the body is still resident because AFTER (PR HEAD
|
| Issue | Main | PR HEAD | Status |
|---|---|---|---|
| #6762 deadlock: skill body evicted but tracking not synced → "already loaded" forever | ❌ Reproduced | ✅ Fixed | Body returns on re-invoke |
| jifeng #1: residency-marker spoofing | N/A (no provenance gate) | ✅ Fixed | Provenance gate prevents spoofing |
| jifeng #2: post-setHistory double sync corrupts tracking | N/A (syncSkillEvictions exists) | ✅ Fixed | Function deleted entirely |
Both issues jifeng reproduced on 6f085bd9e are resolved at current HEAD 69454af. The #6762 deadlock is confirmed reproducible on main and confirmed fixed on this PR.
— Verified via tmux E2E on macOS arm64, Node.js v26.2.0
# Conflicts: # packages/cli/src/ui/hooks/session-swap-telemetry.test.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R6-2 hook-dedup reload false 'No hooks registered' WARN — already on the round-6 deferral list (review 5008875191)
- R6-3 R3-2 client test's no-second-write pin missing trackSkills — already on the round-6 deferral list (review 5008875191)
- R6-7 strip-path !isForkedChat guard unpinned — already on the round-6 deferral list (review 5008875191)
- R5-9 resident-sibling shield provenance gating unpinned — already on the round-6 deferral list (review 5008875191)
- size-path pending-exclusion invariant untested — already on the round-4 and round-6 deferral lists
- R6-1 NOOP tryCompress test omits the trackSkills pin — already on the round-6 deferral list (review 5008875191)
- R5-1 compressFast genuineSkillBodyOutputs wiring unpinned — already on the round-6 deferral list (review 5008875191)
- compact_history test never asserts what history setHistory installs — already on the round-2/round-3 deferral lists
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 2–5 still reported findings.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 9: executing skill-utils.test.ts (review worktree has no node_modules; a full npm ci + npm run build to satisfy the suite's globalSetup guard exceeded this c….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.test.ts:369 — [probe] R3-7 ordering pin conditional — track-without-clear mutant ships greenpackages/core/src/services/microcompaction/microcompact.test.ts:2548 — [probe] blanked-side provenance gate unpinned — marker-only mutant ships greenpackages/core/src/tools/skill-utils.ts:413 — [probe] buildCallIdToSkillName dedup invariant unpinned — guard-deletion mutant ships greenpackages/core/src/tools/skill-utils.ts:45 — [probe] SKILL_BODY_STATIC_LINE conjunct unpinned — prefix-only mutant ships green
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 8 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 2–5 still reported findings。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 9：executing skill-utils.test.ts (review worktree has no node_modules; a full npm ci + npm run build to satisfy the suite's globalSetup guard exceeded this c…。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
I agree with the bug being fixed, but I think this PR should stay narrowly scoped to synchronizing the in-memory dedup guard with the final conversation history. The invariant for this PR should be simple: after any history rewrite, if a genuine Skill body is no longer resident, its name must no longer be present in The broader Skill lifecycle should stay out of this change and be handled under #6762 in focused follow-up PRs:
|
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The main stale-loaded path is covered at this head, and I did not find another production history-rewrite path that leaves an evicted body tracked. Two issues remain in the provenance implementation; details are inline. The broader lifecycle and diagnostic work should remain in focused follow-up PRs under #6762, as noted above.
| ): boolean { | ||
| return ( | ||
| isSkillBodyOutput(output) && | ||
| (genuineOutputs === undefined || genuineOutputs.has(output as string)) |
There was a problem hiding this comment.
[P1] Bind provenance to the producing Skill invocation
genuineOutputs.has(output) proves only that these bytes appeared in some genuine Skill result earlier in this process; it does not prove that this response came from that invocation or even from the same skill. A reachable trace is: load a file Skill and record body B → /clear starts a new session but clears only loadedSkillNames → a same-named model-invocable command returns B through the normal command-delegation path → a later setHistory reconciliation pairs the current response call ID with the skill name and accepts it because B is still in this Set → after re-enabling the file Skill, the dedup guard returns already loaded although no genuine body is resident. The earlier marker-spoof fix does not cover replay of bytes already admitted to this Set. Please bind provenance to the actual producing call/session identity, such as call ID plus skill name, instead of using output bytes as a process-wide allowlist.
| private loadedSkillNames: Set<string> = new Set(); | ||
| // Exact body outputs produced this process (residency provenance; see | ||
| // getGenuineSkillBodyOutputs). Bounded by the session's load count. | ||
| private genuineSkillBodyOutputs: Set<string> = new Set(); |
There was a problem hiding this comment.
[P2] Release provenance when its history/session lifetime ends
Every successful load retains the complete Skill body in this Set. There is no delete or clear path: unloadSkills, history reconciliation, compaction, and /clear mutate only loadedSkillNames, while /clear reuses the same ToolRegistry. Large or edited Skill bodies therefore remain strongly reachable after history eviction and accumulate for the lifetime of the process, defeating part of memory-pressure compaction. Please use compact invocation provenance and prune it at authoritative history or session-reset boundaries.
|
After another simplification pass, I do not think the current 21-file, +2,776-line implementation is proportional to the bug. About 693 of those additions are production code; much of the rest is testing implementation details introduced by successive review rounds. The required failure direction is simpler: after an authoritative history rewrite may have removed a Skill body, the dedup guard must not leave that Skill permanently unreloadable. Conservatively allowing one duplicate body on the next invocation is acceptable; permanently returning Could we reduce this PR to the following behavior?
That should let us remove the process-wide full-body provenance Set and its history scans, the diagnostic-only I would keep #9500 focused on fixing the stale dedup flag. Exact Skill lifecycle semantics, explicit unload, resume reconstruction, and richer eviction diagnostics belong under #6762 in follow-up PRs. The conservative approach is intentionally less exact, but it is much smaller and fails in the safe, self-healing direction. |
…ewrite boundaries Per #9500 review: drop the process-wide body provenance set and history scans, the evictedSkillNames/unresolvedEvictedSkills diagnostics, the exact-reconcile vs targeted-unload algorithms, and the ACP settle machinery. Instead, conservatively clear loaded-skill tracking at the three destructive history-rewrite boundaries (setHistory covers every compaction path, truncateHistory, stripOrphanedUserEntriesFromHistory), guarded so forked chats never touch the parent's tracker. Over-clearing self-heals with at most one duplicate body on the next invoke; a stale entry made the skill permanently unreloadable. Kept: authoritative-vs-forked ownership guard, idempotent hook and allowedTools re-registration on reload, and a small behavioral test set. Exact lifecycle semantics, explicit unload, and richer eviction diagnostics belong under #6762 in follow-up PRs.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Current head is sufficient for the reported compaction ghost-state bug. I verified exact SHA 4e53e36d end to end: the first Skill call returned the full sentinel body, the immediate repeat returned the intended already loaded in context response, /compress-fast cleared both older Skill results, and the next call returned the full body again. The nine focused regression tests also passed.
The conservative tracker clear, fork ownership guard, and hook/permission idempotence are cohesive with that fix. Apart from the disclosed command-delegation state correction, I did not find unrelated production behavior; the telemetry hunk is formatting-only.
Non-blocking follow-up for #6762: interruption repair can still recreate a stale loaded flag through a different pre-existing path. SkillTool marks the name loaded before returning the body; if orphan repair synthesizes the same call ID first, useGeminiStream drops the late real result, leaving the name loaded while no body is present. This is outside the compaction reproduction and should stay out of this late review round, but it is worth tracking explicitly.
No blocker for the intended scope. The platform Node jobs are still running, so this does not certify the final CI matrix.
|
Checked and approved — no blockers at this head. (Body supplement: the review event was posted first; this comment carries the writeup.) CI
What I verified Core mechanism — three rewrite gates:
All three paths reach every compaction/restore entry point (
Command-delegation fix:
Tests — non-vacuous spot-checks:
Cross-check
Reviewed with AI assistance. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Second-pass review of head 4e53e36 after the simplification, with an independent correctness pass over the frozen 43d7596d...4e53e36d diff (base moved since; still MERGEABLE).
Verdict: no P0/P1/P2. The reduced design holds.
- All body-eviction routes are gated. Every compaction/restore application point routes through the three clear sites: compression (
geminiChat.tstryCompress/compressFast), hard-rescue rollback, pre-send microcompaction (client.ts), memory-pressure compaction,/restore/ session-manager / ACP history replacement — all land insetHistory; truncation and orphan-stripping gate on actual drops (history.length < prevLen,strippedEntries.length > 0). The fork guard holds on all three; theGeminiChatconstructor performs no rewrite, soisForkedChat = trueright after construction increateForkedChatis race-free. Resume starts a fresh process with an empty tracker, so the un-gated constructor door is self-healing as documented. - No dangling references to the removed machinery. Zero occurrences of
trackSkills/unloadSkills/getGenuineSkillBodyOutputs/reconcileLoadedSkillTrackingat this head;clearLoadedSkillTrackinghandles undefined registry/tool and wrong shapes. Old-round findings anchored togenuineOutputs,evictedSkillNames,buildCallIdToSkillNamereference symbols that no longer exist. - Main-drift check: since the branch's last merge, main touched two files this PR also changes —
permission-manager.ts(#9829 added the allowlist-caveat block) andskill-utils.ts(doc comment only). Both compose cleanly with this diff: the raw-string dedup sits after the caveat block and before the finalsessionRules.allow.push, mirroring the dangerous-stash dedup #9829 kept. - Verification basis: ubuntu Test green on this head (1376+ tests); core typecheck and eslint clean. macOS/Windows jobs were still running at review time — this doesn't certify the final matrix.
Three latent P3s, none blocking and none requiring action in this PR:
clearHistory()is an un-gated fourth rewrite boundary (geminiChat.ts:4737): it empties history without clearing tracking. No callers insidepackages/coreat this head and/clearclears the tracker itself, so there is no live bug — but any future caller of the publicclearHistory()would resurrect the #6762 deadlock. The same three-line guarded clear would make the invariant complete; fine to defer.stripThoughtsFromHistoryis un-gated but provably safe today (it only removes thought parts from model turns; skill bodies live in user turns). A one-line comment saying so would document the invariant instead of assuming it.- Hook dedup key (
matcher + skillRoot + JSON.stringify(config)): a first-load false positive would require two distinct skills sharing one skillRoot with byte-identical prepared configs, which the per-skill skillRoot layout prevents today. Addingskill.nameto the key would make it impossible regardless of layout.
Also noted, already on the deferral list: the R6-2 reload-time No hooks registered from skill warn fires when dedup skips every hook (skill.ts:655) — misleading debug output, not a correctness defect.
|
After re-evaluating the scope of #6762, I think this PR is sufficient to address the underlying actionable problem. Skill bodies are already reclaimable through microcompaction; the missing behavior was making an evicted Skill reloadable instead of leaving it stuck behind I do not think I would keep this PR focused and treat the actionable part of #6762 as addressed. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/core/src/core/client.ts:2473 — [review] no test replays #6762's canonical failure shape through the microcompaction entrancepackages/core/src/tools/skill-utils.ts:305 — [review] clearLoadedSkillTracking duplicates clearCommand's inline tracker-clear, and the two copies have already driftedpackages/core/src/hooks/registerSkillHooks.ts:75 — [review] hook dedup compensates for a missing unregistration lifecycle it does not close — changed frontmatter accumulates stale hooks across the reload cycles this PR makes routinepackages/core/src/hooks/registerSkillHooks.ts:87 — [review] fully-deduped reload returns 0, flipping skill.ts's 'No hooks registered' warn into routine noise on every eviction/reload cyclepackages/core/src/core/geminiChat.ts:4906 — [review] conservative clear misattributes still-resident skill bodies in /context until re-invoke
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Re-reviewed the current scope: this PR addresses the actionable part of #6762. After a destructive history rewrite evicts a Skill result, the loaded-skill tracker is reconciled from the rewritten history, so the Skill can be loaded again instead of incorrectly returning An explicit |
|
Released in v0.22.2. |
8 participants
What this PR does
This PR fixes the stale dedup flag from #6762 with the smallest safe behavior: whenever a destructive history rewrite happens — compaction (any path that lands in
setHistory), truncation, or orphan stripping — the Skill tool's loaded-skill tracking is conservatively cleared, so a skill whose body was evicted can never stay permanently unreloadable behindSkill "x" is already loaded in context.. Over-clearing is the intentionally accepted direction: a still-resident body costs at most one duplicate injection on the next invoke (self-healing), while a stale tracking entry made the body unrecoverable until session restart.Concretely:
clearLoadedSkillTracking(skill-utils) duck-typedly clears the SkillTool tracker at the three rewrite boundaries inGeminiChat(setHistory— the single choke point for every compaction/restore application —truncateHistory,stripOrphanedUserEntriesFromHistory).createForkedChatshare the parent's ToolRegistry, so their rewrites never touch the parent's tracking (isForkedChat).registerSkillHooksskips hooks a skill already registered (keyed on the whole prepared config), andPermissionManager.addSessionAllowRulededups session allow rules by raw string.A small standalone fix is included: command-delegation outputs (disabled-skill fallback, same-named MCP prompt / file command) are not tracked as loaded skills, since the result is raw command text rather than a skill body and would otherwise block a later same-named file skill behind the dedup guard.
Per maintainer review, this PR previously carried a much larger exact-reconciliation design; that has been removed: the process-wide full-body provenance set and history scans, the
evictedSkillNames/unresolvedEvictedSkillsdiagnostics and call-site wiring, the separate exact-reconcile vs targeted-unload algorithms, the ACP turn-settle machinery, and the review-round wiring tests. Exact Skill lifecycle semantics, explicit unload, resume reconstruction, and richer eviction diagnostics belong under #6762 in follow-up PRs.Why it's needed
Issue #6762: skill bodies evicted from history left the dedup guard believing the body was still resident. Re-invoking the skill then returned only
Skill "x" is already loaded in context.while the body was gone — permanently lost until session restart. The conservative clear makes the failure direction self-healing instead of deadlocking.Reviewer Test Plan
How to verify
already loaded in contextmessage.npx vitest runover the touched suites:geminiChat,skill,skill-utils,registerSkillHooks,permission-manager,forkedAgent.cache,client,microcompact,memoryPressureMonitor— all pass locally (1376 tests).npm run typecheck(core) and eslint clean.Evidence (Before & After)
N/A — internal state handling; behavior is covered by the unit tests above.
Tested on
Risk & Scope
/unskill, explicit unload, resume reconstruction, eviction diagnostics (Feature Request: Skill Context Lifecycle Management #6762 follow-ups).Linked Issues
Refs #6762 (does not close it)
Split from #8900
中文说明
本 PR 做了什么
按维护者评审意见，把 #6762 的修复收敛到最小安全行为：任何破坏性历史重写（所有落到
setHistory的压缩路径、截断、孤儿条目剥离）都保守地清空 Skill 工具的已加载台账，被逐出正文的技能绝不会永久卡在already loaded in context后面。宁可多清：仍驻留的正文最多多注入一次即自愈，而残留台账会让正文直到重启都找不回来。clearLoadedSkillTracking（skill-utils）在GeminiChat的三个重写边界（setHistory（所有压缩/恢复应用的唯一收口）、truncateHistory、stripOrphanedUserEntriesFromHistory）鸭子类型清台账。isForkedChat）。registerSkillHooks跳过已注册的同配置 hook，addSessionAllowRule按原始串去重。另含一个独立小修复：命令委托结果（disabled-skill 回退、同名 MCP prompt/文件命令）不计入已加载，避免同名文件技能被永久挡在去重守卫后。
此前 PR 中更大的精确对账设计已全部移除：进程级正文来源集合与历史扫描、
evictedSkillNames/unresolvedEvictedSkills诊断及其接线、精确对账与定向卸载两套算法、ACP 回合 settle 机制、以及各轮评审的接线测试。精确生命周期语义、显式卸载、恢复重建、更丰富的逐出诊断归入 #6762 后续 PR。为什么需要
#6762：技能正文被历史逐出后，去重守卫仍认为正文驻留，再次调用只返回
already loaded in context，正文直到重启会话才能恢复。保守清空让失败方向变成自愈而非死锁。风险与范围
/unskill、显式卸载、恢复重建、逐出诊断（Feature Request: Skill Context Lifecycle Management #6762 后续）。关联 Issue
Refs #6762（不关闭）
拆分自 #8900