mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-05 07:10:55 +00:00
09248c0745
Eleven findings from /qreview (claude-opus-4-7), grouped by area:
CORRECTNESS
- C1: appendAdditionalContext silently dropped reminders for any tool
whose llmContent is a single non-array Part (read-file returning
inlineData for images / PDFs is the canonical case). Both the
ConditionalRulesRegistry rule reminder and the path-conditional
skill activation reminder were lost. Wrap the single-Part case
into an array so the addition still lands.
- S2: Legacy tool-name aliases (`replace` → `edit`,
`search_file_content` → `grep_search`, `task` → `agent`) bypassed
FS_PATH_TOOL_NAMES. The registry resolves the alias at execute time
but `request.name` keeps the alias, so `replace({ file_path: ... })`
produced empty candidates and missed activation. Canonicalize via
`ToolNamesMigration` before the allowlist check.
- S5: `new SkillActivationRegistry(...)` ran picomatch unguarded —
pathological patterns (oversize / broken extglob) could throw and
abort all of `refreshCache`. Wrap each picomatch call in try/catch
inside the constructor; drop the bad pattern, keep the rest of
the skill, log via debugLogger.
- S7: Extension parser (skill-load.ts) silently dropped
`disable-model-invocation` and `when_to_use`. Now that we have
`paths:`, that meant an extension SKILL.md with both `paths:` and
`disable-model-invocation: true` would still fire path-activation
reminders for a skill the model can't invoke — directly
contradicting the bug_004 fix at the project/user level.
- S8: SkillTool discarded the `addChangeListener` cleanup function
and had no `dispose()`. Subagents share the parent's SkillManager
via `InProcessBackend.createPerAgentConfig`, so each per-subagent
SkillTool registered another listener; with the listener pipeline
now async, every path activation serialized through every stale
subagent's refresh chain. Mirror AgentTool: store the cleanup,
expose `dispose()`.
SECURITY / SUPPLY-CHAIN
- S11: `validateSkillName`'s `/^[a-zA-Z0-9_:.-]+$/` rejected every
non-ASCII name on upgrade, silently dropping CJK / Cyrillic /
accented Latin skills. The structural-injection guard targets
`<>"'/\n\r\t` etc; entire Unicode planes are not the threat.
Widen to `/^[\p{L}\p{N}_:.-]+$/u`. Update docs/users/features/
skills.md to match.
- S10: `parsePathsField` only validated shape (must-be-array). Now
also reject leading-slash absolute patterns and `..` parent-escape
patterns at parse time — these silently never match anything in
the activation registry, so an author who writes `paths:
['/etc/passwd']` or `['../*.ts']` would otherwise see the skill in
/skills and never understand why it never activates.
ROBUSTNESS
- S3: `coreToolScheduler` emitted "skill X is now available via the
Skill tool" even when the calling subagent's tool registry did not
expose SkillTool (subagent's `tools:` allowlist excluded `skill`).
Gate the reminder on `toolRegistry.getTool(ToolNames.SKILL)`.
- S4: `extensionManager.refreshMemory` used `Promise.all` so a
rejection from skill or subagent refresh nuked the other leg AND
the hierarchical-memory refresh below it. Switch to
`Promise.allSettled`, log each rejection, and `await` the
hierarchical refresh too (the comment justifies awaiting; the
code didn't).
- S9 / S12: `docs/users/features/skills.md` claimed `paths:` only
gates model discovery and slash invocation always works. True for
the user-side path itself, but if the model then tries to chain
off the user's invocation (call `Skill { skill: ... }` itself),
validateToolParams returns "gated by path-based activation" —
contradicting the doc. Rephrase to call out the model-side
limitation explicitly.
DEFERRED
- S6: notifyChangeListeners swallows per-listener errors and the
reminder still fires. Real concern but the fix needs an API
shape change (listener-failure signal back to the scheduler);
worth its own design discussion. Logged here for follow-up.
Adds 12 regression tests across the 7 affected files. 632 tests
pass; types and lint clean.
|
||
|---|---|---|
| .. | ||
| channels | ||
| _meta.ts | ||
| approval-mode.md | ||
| arena.md | ||
| checkpointing.md | ||
| code-review.md | ||
| commands.md | ||
| dual-output.md | ||
| followup-suggestions.md | ||
| headless.md | ||
| hooks.md | ||
| language.md | ||
| lsp.md | ||
| mcp.md | ||
| memory.md | ||
| sandbox.md | ||
| scheduled-tasks.md | ||
| skills.md | ||
| status-line.md | ||
| sub-agents.md | ||
| tips.md | ||
| token-caching.md | ||
| tool-use-summaries.md | ||