mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-11 04:02:45 -10:00
gwinthisandClaude Opus 4.7
7bed56b9b6
* docs(design): add RT optimization design doc Two-round review trail documenting the analysis path: original D1-D4 proposal, code-level verification in §6 that recanted the cost estimates, and §7 ROI reordering after DashScope ephemeral cache implementation was confirmed already in place — which collapsed D2's net benefit and led to deferring D2 and D4 as won't-fix. The doc is preserved as the canonical record of why the obvious-looking directions (fast-model routing, prevalidate scheduling) turn out to be dead ends, so future work doesn't relitigate the same conclusions. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * docs(design): add reduce-rounds-via-skill-design with spec-first gating Companion design to rt-optimization-design.md. The core argument: the real lever for reducing agent loop rounds is at the skill/tool design layer, not the agent framework. Round 2 in §1.2's baseline exists because Round 1's skill didn't return a complete answer — fixing that per-skill collapses 3 rounds into 2, an angle the original framework-centric proposal completely missed. Layout: - §0 acceptance spec is the front-loaded gate: engineering specs lock at P-1, statistical thresholds lock at P1.5 (after baseline), per-skill specs are data-driven and live in PR descriptions - §3-§4 three-layer plan: telemetry → per-skill rewrites → prompt guidance for concurrent tool calls; each layer is independently measurable and reversible - §5.3 stop-loss lines split into result + process metrics to catch the "looks like progress, no actual ROI" failure mode early The doc was reviewed by codex twice — once on initial draft (caught qwen-logger dead-code path, batch_size state-passing cost, prompts.ts line drift) and once after §0 was added (caught spec rigidity, missing per-skill template, framework boundary case). Both rounds' findings were either applied or explicitly recorded as not-adopted with reasons inline. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(telemetry): connect logSkillLaunch to QwenLogger logSkillLaunch in loggers.ts went only through the OTLP path, while QwenLogger.logSkillLaunchEvent in qwen-logger.ts had no callers anywhere in the repo — leaving the skill_launch event invisible to any backend that consumes from the qwen-logger pipeline rather than OTLP. Mirror the logToolCall pattern at loggers.ts:230: forward the event to QwenLogger before the OTLP path so the call still reaches QwenLogger when the OTEL SDK is not initialized. This is P0 of docs/design/rt-optimization/reduce-rounds-via-skill-design.md §4.1.1b — a prerequisite for the prompt_id propagation in P1 so the SkillLaunchEvent / ToolCallEvent join in §4.1.2 has data to query against. Tests: 2 new cases under describe('logSkillLaunch') covering forwarding to QwenLogger plus the OTLP-uninitialized branch; loggers.test.ts now 47/47 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * feat(telemetry): thread prompt_id through SkillLaunchEvent To join skill_launch events with the subsequent tool_call events they trigger, SkillLaunchEvent now carries the prompt_id of the user turn that fired the skill. The scheduler already holds the request and its prompt_id; the missing piece was getting that id into the invocation that does the actual logSkillLaunch call. Wiring: - SkillLaunchEvent constructor adds a required prompt_id parameter so the field can never be silently undefined in a backend join. - SkillToolInvocation exposes setPromptId(id) and stores the value; the four logSkillLaunch sites in execute() pass this.promptId through. - CoreToolScheduler.buildInvocation grew an optional fourth promptId argument and duck-types setPromptId on the freshly-built invocation, mirroring the existing setCallId hook. The two callers (setArgs path at L1036 and the main schedule path at L1497) pass request.prompt_id / reqInfo.prompt_id. - qwen-logger.logSkillLaunchEvent forwards prompt_id in the RUM event properties so the join works on the qwen-logger pipeline too. The empty-string default on SkillToolInvocation.promptId is deliberate: direct invocations (e.g. buildAndExecute in tests) that skip the scheduler still log a valid event, and downstream queries can filter prompt_id != '' to exclude non-scheduled launches from joins. Implements P1 of docs/design/rt-optimization/reduce-rounds-via-skill-design.md §4.1.1 — required prerequisite for the SkillFollowupRecord SQL in §4.1.2. Tests: 2 new cases in skill.test.ts cover the setPromptId path and the empty-default path; loggers.test.ts updated for the new 3-arg signature. 256 tests pass across loggers / skill / coreToolScheduler suites. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(scheduler): cover prompt_id propagation through buildInvocation The duck-typed setPromptId hook added in the previous commit went through unit tests on each side independently — SkillToolInvocation tests verified that setting the field changes the logged event, and the loggers tests verified the SkillLaunchEvent shape — but the integration point in CoreToolScheduler.buildInvocation that wires the two together was only exercised indirectly. Same is true of the older setCallId hook it mirrors, which had no test at all. Two cases here close that gap on the scheduler side: - A purpose-built PromptIdAwareTool whose invocation records every setPromptId call; the test schedules a request with a known prompt_id and asserts the invocation captured it. This is the positive contract. - The existing TestApprovalTool (no setPromptId) scheduled through the same path to confirm the duck-type guard does not throw when the method is absent. This is the backward-compatibility contract that lets every existing tool keep working unchanged. The two cases together pin both branches of the typeof check in buildInvocation, so future refactors of that hook cannot regress silently. 165 tests in the suite still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(telemetry,scheduler,skill): close P0/P1 coverage gaps Three blind spots remained after the initial P0+P1 work — each one was a path that the production code change had already touched mechanically but no test was pinning it against future regressions. qwen-logger.ts logSkillLaunchEvent now has two cases asserting that prompt_id reaches the RUM event properties, on both the success and failure branch. Previously the loggers.test.ts spy stopped at "method was called" and never inspected the payload qwen-logger built. skill.ts had four logSkillLaunch sites, but only the happy path and the empty-default path were tested. The commandExecutor-success branch (L386), not-found branch (L399), and thrown-exception branch (L482) now each have a test that sets promptId, drives execute() through that specific path, and asserts the emitted event carries both the right success flag and the right prompt_id. This catches the failure mode where someone later edits one of those branches and forgets the promptId argument — replace_all guaranteed today's correctness but no test would catch a regression tomorrow. CoreToolScheduler.buildInvocation now has two direct unit tests that exercise the method through a type-assertion cast. Reaching the L1036 setArgs path through the public API would require mocking modifyWithEditor + the filesystem + an editor type, which would dwarf the change under test. The direct call covers both L1036 and L1497 simultaneously: when promptId is supplied the duck-typed setPromptId is invoked; when it is omitted, the captured field stays undefined and no throw happens. 298 tests pass across loggers / qwen-logger / skill / scheduler suites. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * review(PR #4565): address Copilot + github-actions feedback Five spots flagged by the automated review on #4565. Three were real and worth fixing; two were comment-quality touch-ups that traveled along with the same patch. - SkillLaunchEvent.prompt_id is now optional with a default of '', removing the breaking-change footprint on the exported telemetry API. All current internal callers still pass the value explicitly through the SkillToolInvocation.promptId field, so the §0.1 spec ("prompt_id 串联") is still enforced in production paths — type-level enforcement just steps aside in favor of API stability, with §0.5 治理 covering the discipline at the process layer. - The skill-design doc §4.1.1 used to claim "BaseToolInvocation 已有 request.prompt_id" which is wrong: BaseToolInvocation only holds params, and the prompt_id flows through CoreToolScheduler's duck-typed setPromptId hook (mirroring setCallId). The doc now reflects the actual implementation and notes that the earlier text was the bug. - CoreToolScheduler.buildInvocation gained a short JSDoc explaining why the two extra args (callId, promptId) are optional — they match the existing duck-type pattern that lets older tools and non-scheduler call sites work without implementing the setters. - skill.test.ts adds a one-comment note next to the first setPromptId cast explaining that setPromptId is a scheduler-only hook, not part of the public ToolInvocation interface. - SkillToolInvocation.promptId field comment shrank from 8 lines to 2 with a pointer to the design doc so the inline noise drops without losing the empty-string semantics. Pre-existing scope-creep findings (Chinese-only doc, mock-config duplication in scheduler tests, redundant optional-chain comment, prompt_id sanitization for an internally-generated UUID) are deliberately not addressed here — see the reply on PR #4565 for disposition per item. 298 tests still pass across loggers / qwen-logger / skill / scheduler. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>