feat(daemon): add session artifact content retention - #3
Closed
chiga0 wants to merge 1 commit into
Closed
Conversation
Owner
Author
|
This PR is intentionally stacked on QwenLM#6259. Please review it as the content-retention delta only; metadata persistence remains in the base PR. Current stacked diff: Scope in this PR: pin/unpin, retained content reads, content refs and hashes, quota/GC behavior, and REST/ACP/SDK exposure for retained content. Local validation passed: cd packages/acp-bridge && npm run typecheck
cd packages/sdk-typescript && npm run typecheck
cd packages/acp-bridge && npm run build
cd packages/cli && npm run typecheck
cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts test/unit/DaemonSessionClient.test.ts test/unit/acpRouteTable.test.ts
cd packages/cli && npx vitest run src/serve/server.test.ts -t "session artifact routes" --coverage.enabled=false
cd packages/cli && npx vitest run src/serve/acp-http/transport.test.ts -t "_qwen/session/artifacts" --coverage.enabled=false
cd packages/acp-bridge && npx vitest run src/bridge.test.ts src/sessionArtifacts.test.ts
npx prettier --check packages/acp-bridge/src/bridge.test.ts
git diff --check |
Owner
Author
|
CI note: the current failing check is the fork auto-review workflow, not a code/test failure. The failed job exits before reviewing with: I am leaving the code unchanged. Local targeted validation for this stacked diff passed as listed above; the failure appears to be fork workflow/auth configuration for |
Owner
Author
|
Superseded by the corrected upstream stacked PR: https://github.com/QwenLM/qwen-code/pull/6346\n\nClosing this fork-only PR because review should happen in QwenLM/qwen-code, with QwenLM#6259 as the stack base. |
wenshao
added a commit
that referenced
this pull request
Aug 3, 2026
* feat(cli): add a Java/JVM performance path rule to /review The review's dimensions are domain-blind, and a Java diff's most expensive regressions are decided by the JVM, not by anything visible in the source: HotSpot chooses what to inline and what to compile by the callee's bytecode size (MaxTrivialSize 6 / MaxInlineSize 35 / FreqInlineSize 325 / HugeMethodLimit 8000), and a one-line change can flip it on a hot path. Like the GitHub Actions rule before it, the checklist attaches to *.java paths and reaches every code-reviewing agent whose territory contains one, scoped so non-Java diffs pay nothing. It carries: - the correctness traps dressed as perf/concurrency code (shared SimpleDateFormat, two-call ConcurrentHashMap compounds, DCL without volatile) at Critical; - the JVM-cost defects provable from source (per-call regex compiles, loop string +=, hot-path boxing, capturing lambdas in loops, unconditional log-message building, unpresized collections, legacy synchronized types, exceptions as control flow, per-call reflection) at Suggestion; - the JIT inlining thresholds with a two-tier verification discipline: measure with javap against base and head (never estimate bytecode from source), or run -XX:+PrintInlining / JMH when the code is runnable; unmeasured inlining claims are reported as mechanism at low confidence. Dogfooded against alibaba/fastjson2#3992 (BigDecimal parsing perf): the performance agent applied the threshold reasoning correctly — readBigDecimal was already far above FreqInlineSize before the diff and the change shrinks it, so no crossing was possible and no measurement was owed, stated with exactly that justification. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(cli): name hot/cold splitting as the fix for a grown hot method An A/B experiment measured what the checklist adds on top of the model. Four crafted Java diffs (a hot method grown past FreqInlineSize, a map-backed cache, a per-element append loop, a precompiled regex parser) were each reviewed by a blind Agent-4 equivalent with and without the Java path rule, on qwen3.8-max-preview. Without the rule, the performance agent never once considered inlining across all four diffs, and filed a high-confidence finding that a constant long division costs 20-90 cycles per iteration — bytecode-true, but C2 strength-reduces constant division to a multiply-by-magic-number, so the cost does not survive the JIT. With the rule, it measured every diff with javap (base 80 bytes, head 338, crossing FreqInlineSize at 325), reported the crossing at low confidence with the tier-2 check named, dismissed the division with the correct mechanism, and proposed the fix as a hot/cold split with the exact bytecode range to extract. The fix shape is the part the model does not supply on its own, so the checklist now names it: move cold paths into a private helper, never @ForceInline (which bloats every caller), and state the extraction as a bytecode range and a resulting size. The same experiment cut three candidate additions — dense-key cache container choice, per-element-to-bulk loops, and regex-for-fixed-formats — because the control runs reached the same findings without them. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address review on the Java/JVM path rule Seven findings from the PR review, all verified before fixing: - The static measurement tier prescribed 'compile the base revision the same way', which reads as git checkout/stash in the one worktree nine agents share concurrently — or in the user's own checkout in local mode, where reviewsCode agents get no 'do not build the main checkout' guard (that guard is role-7-only). Rewritten as a non-mutating procedure: extract the base side with git show into a scratch dir, javac -d there, never checkout/stash/build in place. - A full mvn/gradle build runs the branch's contributor-controlled build logic; the checklist now says to prefer javac on the extracted file and treat any build it does run as untrusted code (Agent 7's brief already carried this caveat; the rule extended the capability to nine agents without it). - InlineSmallCode cited as ~1000 (the pre-JDK-11 value); measured 2500 on a live JVM. HugeMethodLimit is a develop flag gated by the product DontCompileHugeMethods, and the boundary is > 8000, not >= 8000. - 'Megamorphic -> no inlining at any size' overstated C2: a dominant receiver (TypeProfileMajorReceiverPercent, 90%) is still inlined behind a guard with an uncommon trap. - pathRulesFor listed every triggering path in the heading of every agent's brief; a 200-file Java PR put ~11 KB of paths there. Capped at ten plus a count, for both rules. - The flat 'performance findings are Suggestions' carried no escape hatch; added the one the workflow rule already needed — unbounded cost on attacker-reachable input is a DoS hole, graded Critical. - Nits: the split fast-path excludes regex metacharacters (split(".") does not take it); test and generated sources are out of scope for the hot-path items; the Java match rows fold into the shared governed-table test so both rules assert through PATH_RULES.matches. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): second-round review on the Java/JVM path rule Six findings plus nits and three inline coverage probes, all verified: - The 'safe' javac tier still executed contributor code: annotation processors on the classpath run at compile time. Add -proc:none (not optional), and make mvn/gradle a prohibition rather than a discouraged preference — on a stranger's PR branch the build logic is the attack surface. - /tmp/<scratch> was a placeholder agents converge on; two compiling different revisions of one class into the same dir measure each other's bytecode. Prescribe SCRATCH=$(mktemp -d), with a %TEMP% note for Windows. - 'Extract and javac' fails on any class with imports. Name the classpath path (-sourcepath at the module root, an existing target/classes, or mvn dependency:build-classpath which resolves without building) and a graceful fall-back to the mechanism tier instead of escalating to a project build. - @ForceInline was ruled out for bloating callers — true but secondary, and the annotation is JDK-internal, not general. Lead with the anti-pattern that actually bites app code (reaching for -XX:FreqInlineSize / -XX:CompileCommand=inline, runtime knobs a PR cannot ship) and note @ForceInline only as unavailable. - describePaths capped in diff order, so a test-heavy PR could name ten test files the rule scopes out and no production path. Stable-partition production first. - Header: a rule earns its place by naming an invisible defect AND pays a per-agent token cost; say so, before rule #3 arrives. - Nits: HashMap.newHashMap(n) (JDK 19+) for the presize arithmetic; the split fast path also covers the escaped two-char form. - Coverage probes (inline): the correctness-traps block and the nine Suggestion patterns had zero test coverage — deletion left all tests green. Pin the load-bearing strings of both, plus the new tier flags and the production-first ordering. * fix(cli): third-round review on the Java/JVM path rule Drop the mvn dependency:build-classpath recommendation — Maven extensions execute during any invocation, a strictly larger execution surface than the annotation processors -proc:none exists to close. Fall through to the mechanism tier when no pre-built classpath exists. Add --release <N> at the project's target level: the same source compiles to different bytecode at different levels (61 vs 16 bytes for a five-+ concatenation), so measuring without it produces a threshold verdict on bytecode the shipped artifact does not contain. Name -proc:none as a fidelity hazard: on a Lombok/Dagger project the compiled class is missing generated members, so the static tier is void. Add clauses for new files (no base side to compare), the base-side target/classes caveat, and the Windows uniqueness primitive. Fix the vacuous ordering assertion (indexOf returns -1 outside the cap, and -1 < n passes). Add a GHA cap test and a --release/new-file test. * fix(cli): fourth-round review on the Java/JVM path rule * fix(cli): fifth-round review on the Java/JVM path rule * fix(cli): sixth-round review on the Java/JVM path rule * fix(cli): seventh-round review on the Java/JVM path rule --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This is the stacked content-retention follow-up to QwenLM#6259. It restores artifact content retention on top of the metadata-persistence base: content pinning and unpinning, retained content reads, quota and cleanup behavior, hash-backed content references, REST/ACP/SDK exposure, and focused coverage for the content-retention paths.
Why it's needed
QwenLM#6259 has been narrowed to metadata persistence so reviewers can validate restart recovery, journal rebuild, and fork-remap behavior separately from filesystem content retention. This PR isolates the higher-risk content surface so path safety, ownership, quota, content hashing, lease, and garbage-collection behavior can be reviewed on their own.
Reviewer Test Plan
How to verify
Review this PR as a stacked delta on top of QwenLM#6259. Confirm metadata persistence remains in the base PR, then verify this PR adds only content-retention behavior: pinning content, reading retained content after source changes, unpin cleanup behavior, quota rejection, GC warnings, and REST/ACP/SDK routing for retained content.
Evidence (Before & After)
N/A for UI. Local checks passed on macOS:
Tested on
Environment (optional)
Local Node/npm workspace; no sandbox runtime required for the targeted checks above.
Risk & Scope
Linked Issues
Stacked on QwenLM#6259.
中文说明
这个 PR 做什么
这是 QwenLM#6259 之后的 stacked content-retention PR。在 metadata persistence 基础上恢复 artifact content retention：content pin/unpin、保留内容读取、quota 和清理行为、基于 hash 的 content reference、REST/ACP/SDK 暴露，以及 content-retention 路径的重点覆盖。
为什么需要
QwenLM#6259 已经收窄为 metadata persistence，方便 reviewer 单独验证 restart recovery、journal rebuild 和 fork-remap。这个 PR 把风险更高的 content surface 单独拆出，让 path safety、ownership、quota、content hashing、lease 和 garbage collection 行为可以独立 review。
Reviewer Test Plan
请把这个 PR 作为基于 QwenLM#6259 的 stacked delta review。先确认 metadata persistence 留在 base PR，再验证这个 PR 只增加 content-retention 行为：pin content、源文件变化后读取 retained content、unpin cleanup、quota rejection、GC warning，以及 retained content 的 REST/ACP/SDK routing。
Evidence (Before & After)
UI 不适用。macOS 本地已通过上面列出的 targeted checks。
Risk & Scope
主要风险是 retained artifact content 触及 filesystem safety、quota、deletion/GC 和 SDK/API compatibility，所以它从 metadata persistence PR 中拆出单独 review。未覆盖完整跨平台 CI 和更完整的交互式 E2E。没有预期 breaking change；这个 PR stacked on QwenLM#6259，不应脱离 base 独立合并。