mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-25 08:33:55 +00:00
17 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Shaojin Wen
|
133cf8bfcf
|
refactor(cli): consolidate shared helpers ahead of the legacy audit skill (#9345)
* refactor(cli): consolidate shared helpers ahead of the legacy audit skill
Move the pieces the upcoming /audit skill needs out of command-group
ownership so no skill imports across command groups:
- the findings schema moves from commands/review/ to cli/src/utils/ as-is;
every review consumer imports it from the new home, and the stale-bundle
digest, bundle-asset list, and artifact comment track the move
- safeTarget (traversal-safe slug) and tokenizeArgs (quoted argument
splitting) lift to cli/src/utils/paths.ts and shell-args.ts, with
review's copies re-exporting/redirecting
- the two private git check-ignore copies (review test-plan, team memory)
consolidate into one fresh-by-default helper in core utils; the memo
stays caller-side so a remedy re-check observes the flip
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): restore review ledger machinery dropped by the consolidation
The shared-helper consolidation silently reverted three behaviors PR #9175
added to compose-review, against the PR's stated "no behavior change"
intent: the unreviewed-dimension anchor exemption (scopeUnproven /
dimensionGapsAreDepthOnly / isNonDiffDimensionGap), the LEDGER_MAX_ROUND
stamp clamp, and the bilingual budget-stop phrase splice. Restore them with
the tests that pin them; SKILL.md, ledger.ts, and deadline.ts still
document all three.
Also harden the new helper tests:
- safeTarget: the deep-path fixtures now share a flattened prefix longer
than the kept window, so a truncation-only slug (no digest) collides
instead of shipping green.
- isGitIgnored: each GIT_* scrub arm now carries a discriminating fixture
(three arms previously passed with their scrub line deleted), and every
foreign git init scrubs ambient repo-placement selectors.
* fix(core): scrub git config-injection channels from the ignore probe
Ambient GIT_CONFIG_COUNT (inline KEY/VALUE injection) and
GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM (config-file redirects) can aim
core.excludesFile at a foreign rules file and flip the probe's verdict
for the -C worktree — the same leak class the existing selector scrubs
close. Measured on the pristine probe: both channels turn a
not-ignored path into an ignored one.
Also pin the guards the review found unpinned: the two config
channels, the `--` separator for dash-leading paths, the timeoutMs
wiring, safeTarget's hash-of-original-target property, and
tree-existence of the lifted review helpers.
* fix(core): scrub pathspec-magic env channels from the ignore probe
* fix(core): drop the whole GIT_* env family from the ignore probe
Two more leak channels surfaced on the probe's per-variable scrub list:
GIT_ICASE_PATHSPECS (the fourth pathspec modifier — ambient, makes
check-ignore reject every pathspec with exit 128, which the catch reads
as not-ignored) and GIT_CONFIG_PARAMETERS (the inline -c channel git
itself uses to propagate config to children — ambient, can aim
core.excludesFile at a foreign rules file). Measured through the real
function: the first flips a genuinely ignored path to not-ignored, the
second flips a not-ignored path to ignored.
Since the channel list grew by one leak per review round, drop the
whole GIT_* family instead of enumerating, and close the system config
tier explicitly (GIT_CONFIG_NOSYSTEM=1) so host policy in
/etc/gitconfig can no longer answer for the -C worktree — that ambient
dependency also made the config-redirect arm red on any host whose
system config matches the probe path. Pin the icase member, the
PARAMETERS channel, the default 5 s deadline (previously unpinned), and
add a lower timing bound to the caller-deadline arm so it cannot pass
vacuously when the shim is not executable.
* fix(cli): make the safeTarget slug space prefix-free at the dash boundary
Review's cleanup sweeps .qwen/tmp/ by qwen-review-<slug>- prefix. A slug
that itself carried '-' — natively (pr-6771 vs pr) or via the truncation
join — could extend a shorter slug, letting one target's cleanup delete a
DISTINCT target's artifacts (R8-7). The truncation branch this PR carries
newly lands deep targets inside the cap instead of dying ENAMETOOLONG,
which turned the latent collision live.
Drop '-' from the slug alphabet entirely (dashes flatten like separators)
and join the truncation digest with '_': with '-' out of every slug, the
qwen-review-<slug>- boundary is unambiguous by construction — no slug can
start with another slug plus '-'. prev-ledger side files keep their
hardcoded dashed name on both writer and reader, untouched by the slug.
Tests pin the prefix-free property (short-vs-short, short-vs-truncated)
and the fixture names follow the new slugs.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* revert(cli): restore safeTarget byte-identical to the pre-lift behavior
Reverts the slug alphabet change (
|
||
|
Shaojin Wen
|
b286875e72
|
fix(review): harden the pipeline against four live-run failures (#9086)
* fix(review): harden the pipeline against four live-run failures
Measured on three parallel PR reviews (qwen3.8-max, 2026-08-13, PRs
#9013/#9014/#9045) run via `qwen review run`:
- run.ts: pin the composed-verdict and report scans to the run's own
target. The generic newest-composed scan captured a concurrent run's
artifact — two of the three runs republished a neighbour PR's verdict
(one reported REQUEST_CHANGES for a review whose own report said
Comment). Also keep re-reading while the child runs: a coverage
re-check legitimately recomposed a verdict 12 minutes after the first
write, and the first-snapshot capture would republish the superseded
one.
- budget.ts: drop placeholder gaps whose completion word carries a
trailing budget adverbial. Three "none — all checks … completed
within budget" non-answers reached two posted bodies because the
completion idiom required the completion word to end the text.
- coverage.ts: label a non-chunk agent by the brief codename found
anywhere in its launch prompt. Launchers prepend context lines, so the
first-line label gave twelve finders one shared PR-summary sentence,
and every budget-gap disclosure rendered as the same truncated PR
quote instead of a name.
- copy_bundle_assets.js: emit dist/cli.js with a shebang and the execute
bit. shellContextEnv blanks a QWEN_CODE_CLI a POSIX shell cannot exec,
so every review subcommand issued from a session launched off the
bundle silently fell back to the PATH's global install — all three
runs executed the machine's auto-updated release instead of the tree
they were launched from.
* test(review): follow the codename label into compose-review's fixtures
The backtick-collapse fixture's first line was itself the brief codename
shape, so the new codename extraction labels it `agent security` and the
first-line assertion no longer holds. Keep the sanitization intent on a
prose-only first line, and pin the codename behaviour — a prepended
context line must not reintroduce the shared-PR-quote label — as its own
case.
* fix(review): classify the run target with the child's own parser
Review feedback on the target pin: prNumberFromTarget re-derived PR
classification with a narrower regex than parse-args — /pull/<n>/files
URLs went unpinned, 0042 pinned pr-0042- while the child writes pr-42-,
and docs/pull/42 pinned a file target as a PR — so a completed (and
posted) review could be reported as one that produced no verdict.
Delegate to parseReviewArgs, whose verdict is what the child names its
artifacts from, and pin the divergent shapes as tests.
Also gate the bundle's shebang/exec-bit block with a package-assets
case (mode asserted off-win32, double-run must not stack shebangs), and
document the accepted same-target residual race on composedPatternFor.
* fix(review): pin run artifacts by exact target identity, share the identity-line parser
Round-2 review feedback, all six findings:
- run.ts: replace name-shape pins with the exact composed filename each
target class produces (pr-<n> / file basename / the fixed 'local' token,
per the skill's --out template). The (?!pr-\d+-) lookahead rejected a
file run's own artifact whenever the reviewed file was named
pr-<digits>-…, the PR branch's .* wildcard claimed that same artifact,
and the pooled null class let concurrent file and no-target runs
cross-capture each other's verdicts. Target classification now comes
from classifyRunTarget (parse-args' verdict, basename for files).
- run.ts: newestArtifactSince returns {path, mtime}, so the capture poll
reuses the scan's own stat instead of re-statting the path — the
scan-vs-sweep window (and its untestable catch branch) is gone
structurally.
- lib/agent-identity.ts: one parser for the identity line agent-prompt
bakes into every launch, shared by cost-ledger's row labels and
coverage's disclosure labels — the two hand-rolled copies could drift,
and coverage's copy dropped the (round N) and owned-file suffixes,
folding reverse-audit rounds into indistinguishable disclosure lines.
cost-ledger still feeds it only the first line (quoted identity lines
below must never be credited); coverage scans for the first
line-anchored identity line (launchers prepend context lines).
- lib/budget.ts: one vocabulary for the budget-idiom family — 'below'
joins the completion tail's position words, and the stayed idiom takes
the same qualifiers ('stayed inside the tool-call budget').
- run.test.ts: handler-level assertion that the report scan is pinned
(a strictly newer neighbour report must not become this run's
reportPath), alongside the pattern-level cases for every shape the
review probed.
* fix(review): round-3 review polish — CRLF identity lines, producer-side no-gap rule, edge-case pins
- agent-identity: tolerate a trailing CR (CRLF-recorded prompts fed
through \n-splitting callers failed every parse and fell back to
first-line prose); scan the launch prompt with one multiline match
instead of materializing a line array per agent record; pin the
round-over-file precedence with a both-suffixes test.
- agent-prompt: state the no-gap rule at the producer — write NO
'Budget gap:' line when nothing was cut short — instead of leaving
each agent to improvise a 'none' phrasing the consumer-side
placeholder filter must chase forever.
- run.ts: strip trailing path separators before taking a file target's
basename (a tab-completed 'src/' pinned 'qwen-review--composed.json',
which no child artifact carries — fail-closed exit 1 on a completed
review); pin file-run reports by their filename slot so a file named
'pr-1234.md' claims its own report; document the two collision
classes the basename-keyed pin defines (same-basename files,
basenames spelling 'local'/'pr-<n>').
- budget.test: pin the parenthesis-form exception keep case beside the
dash form.
* fix(review): round-4 polish — one budget vocabulary, named pin expectation, honest prompt claim
- budget.ts: spell the budget-position vocabulary once (BUDGET_QUALIFIED /
COMPLETION_TAIL) and build PLACEHOLDER_GAP_RE from it — the literal
carried three hand-copies that had already drifted twice in two review
rounds; the space-separated 'tool call' form is pinned in both branches.
- agent-prompt.ts: the no-gap rule now states what actually happens to a
'none' disclosure — at best filtered, and any unrecognized wording is
published as a phantom coverage gap — instead of claiming the parser
treats it as a gap, which was the negation of the filter shipped beside
it.
- run.ts: derive the composed pin from composedNameFor and name the
expected filename in both the no-verdict prose and the JSON result
(expectedComposedName) — a naming drift between the pin and the skill's
template was undiagnosable once Step 9 swept the near-miss.
- compose-review.ts: publicAgentSubject's provenance note now describes
the codename labels coverage prefers, with first-line prose as the
fallback.
* test(review): guard the no-verdict diagnostic and the pinned capture; align the chunk-role grammar
- run.test.ts: assert the no-verdict report names the artifact it waited
for, in prose and as expectedComposedName — mutation-verified: dropping
the suffix now fails.
- run.test.ts: force the neighbour's composed artifact strictly NEWER in
the concurrent-run fixture. With it older, an unpinned newest-composed
scan landed on the right file anyway and the regression passed;
mutation-verified: reverting composedPatternFor to the generic scan now
fails the handler test, not only the pattern units.
- agent-identity.ts: CHUNK_ROLE_RE takes coverage's CHUNK_RE shape
(whitespace-tolerant, case-insensitive) so a hand-edited 'Chunk 3 of 7'
cannot resolve as a chunk owner in the posted body and a role agent in
the ledger row.
* fix(review): keep the bundle's write time across the shebang rewrite; hold the pins to the skill
Round-6 review feedback, all five findings:
- copy_bundle_assets.js: preserve dist/cli.js's atime/mtime across the
shebang rewrite. stampReviewSourceDigest reads that mtime as the build
time, so a bumped one certifies a bundle as newer than review sources
edited before it and the staleness warning the skill's Step 0 stops on
never fires. A full bundle stamps before reaching here, but a
standalone run of this script — a flow the gate's own comment
contemplates — was exposed.
- package-assets.test.js: pin both halves the block owes. The mtime is
asserted against a fixture built 60s in the past, and the second run
now arrives at mode 0644 so the exec bit must be re-set — demoting the
chmod inside the shebang guard previously stayed green.
- run-skill-parity.test.ts: new. composedNameFor and reportPatternFor
encode the bundled skill's Step 6 --out template and Step 8 report
stems, and were pinned only against self-referential literals. This
reads the templates out of SKILL.md and renders them per target class,
so a skill-side edit fails next to the code that must follow it
instead of silently in a later review.
- cost-ledger.test.ts: pin the first-line-only invariant — a launch
whose prepended context sits above the identity line keeps the
transcript's own id, never a label lifted from below. Consolidating
both callers on labelFromLaunchPrompt now fails.
- agent-identity.test.ts: assert the two entry points genuinely differ
on that prompt, so neither caller's policy can be collapsed into the
other unnoticed.
* fix(review): stop the mtime assertion from pinning libuv's timespec truncation
The assertion compared the recorded mtime against the Date handed to
utimesSync, so it also pinned libuv's double-seconds → timespec
conversion: about half of all millisecond values read back 1 ns low
(X - 0.001), and builtAt is a fresh Date.now() - 60_000 every run — a
~50% coin flip that would have landed intermittent reds on unrelated
PRs through test:ci.
Capture what the filesystem actually recorded after the setup and
compare against that; the invariant under test is only whether the
shebang rewrite moves the stored time. 10/10 green through the CI entry
point, and it keeps its teeth: removing the production
fs.utimesSync(cliEntry, atime, mtime) restore fails it 3/3.
|
||
|
Shaojin Wen
|
7edc16ba11
|
feat(review): say so when the bundle is older than the review it runs (#8390)
* feat(review): say so when the bundle is older than the review it runs Every `qwen review …` step runs the BUILT bundle, not the working tree. So editing a review command, or switching to a branch that contains one, changes nothing about the run until someone rebuilds -- and the failure is silent and total: the run behaves like the last build, and every conclusion drawn from it is a conclusion about that build. Measured on 2026-08-02, dogfooding /review against #8368 from a checkout whose bundle was fourteen hours old. Three things were invalidated at once and none announced itself: `drive` and `mock-provider` had merged that morning and were absent from the binary, so "the agent never reached for them" measured nothing; and #8345's guard against scoring a mutant `survived` when its own collocated test was red had merged too, so the run reproduced the bug it fixed and filed three findings the current code holds as `inconclusive`. The round was discarded and re-run after a rebuild. `parse-args` is the first command of every review, which makes it the only place a notice reaches a reader before they act on a result. It names the file that is ahead, by how much, what actually runs from the bundle, and the command to rebuild -- "rebuild" without evidence is advice nobody can check. mtime, not git: the question is whether this bundle was built from this source, and a git comparison answers a different one. A margin absorbs a checkout, which writes everything at once in no guaranteed order. An installed package has no sources beside it, finds nothing to compare, and stays silent -- a check that cannot see the files must not accuse the build. Also documents `findings --test-delta` for users: it can lower a severity, and therefore change what the verdict is computed from, so it belongs beside `--outcomes` rather than only in the skill. * fix(review): watch the file every subcommand is registered in `packages/cli/src/commands/review.ts` is where all 30-odd subcommands are imported and registered, and it sits beside the directory rather than in it -- so a new command, or a changed dispatch, was exactly the change this check could not see. A root may now be a single file, which is what that one is. Confirmed end to end: with `review.ts` three hours ahead of a fresh bundle, the warning names it. Also two comments that did not match the code: symlinks of every kind are skipped, not only directories (`isFile()` is false for a symlinked file too), and the module now says what `QWEN_CODE_CLI` already covers -- talking to a different program -- so it is clear this guards the other half, the right program built before the change. * fix(review): compare content, because a timestamp check cried wolf The first version compared the bundle's mtime against the newest review source, and it was wrong in the direction that matters most. `git checkout` rewrites every file that differs between two commits, so returning to the branch a bundle was built from re-stamps exactly those files and the check calls a byte-for-byte correct bundle stale. Measured: with the sources untouched and the bundle two minutes older, it warned. A line that fires when nothing is wrong teaches its reader to skip the line, which would have made this worse than absent. The build now stamps a digest of the review sources it bundled into `dist/review-sources.sha256`, and the check re-derives that digest from the tree and compares. No margin to tune, no clock to trust, and no answer but the true one. Verified end to end across all five cases: a clean tree is silent, a source touched but unchanged is silent, and a real change under any of the three roots -- the command directory, the `review.ts` that registers them, the bundled skill -- warns. The digest is now one rule stated twice, since the build script cannot import the package it runs before building. `scripts/tests/review-source-digest.test.ts` holds the two equal, on this repo and on a synthetic tree that exercises the file-shaped root; a package test may not reach into `scripts/`, so it lives on the side of the boundary that may. Paths are folded relative to the repo root with separators normalised, and the file list is sorted -- `readdir` order is a property of the filesystem, so without it a bundle built in CI and a tree cloned locally would hash the same source differently and every run would warn. * fix(review): a diagnostic must not kill the run, and tests are not the bundle Two Criticals and five suggestions from review, all verified before changing anything. `writeStderrLine` throws on EPIPE, so stderr piped to `head` would have killed the review before it parsed a single argument -- a warning that destroys the run it was warning about, and the opposite of this change's own invariant. `writeStderrLineSafe` is the convention for diagnostics in this subsystem and is what it calls now. `reviewSourceRoots` builds paths with the platform `join`, and the test asserted forward-slash literals, so all three elements would have failed on the merge queue's Windows leg -- which the pull_request event never runs, so the green CI here proved nothing about it. Test files left the digest. esbuild follows imports from the CLI entry and no test is reachable that way, so folding them in fired the warning for an edit that cannot change a byte of the bundle -- the false positive this module already rejected once. 112 files became 61, and a test-only edit is now silent while a production one still warns. The handler wiring is tested at last, against a real temp tree rather than a mock of the reads under test: the derivation from `process.argv[1]`, the stamp read, and the warning. All three mutations the review named -- dropping the call, reading the stamp from the wrong directory, collapsing repoRoot to distDir -- now redden it. Also: the stamp's filename is pinned across the boundary it crosses (the build wrote a literal while the check read `DIGEST_FILE`, so a one-sided rename would have silenced the feature with every test green); the digest is computed only when there is a stamp to compare it against, instead of hashing a hundred files for a value the first guard discards; the `rebuildCommand` parameter no caller ever set is gone; and the build script's comment no longer claims a code-sharing relationship that does not exist. * fix(review): fixtures are not in the bundle either The same false positive, a third time and one directory over. Excluding tests from the digest was right and incomplete: `review/__fixtures__` holds four files — three responder modules and a captured comment — that a test loads at runtime, from no import the bundler follows. Measured against `dist`: none of the four appears in it, so editing one changed the digest while the bundle stayed byte-identical and the warning claimed a review command had changed. Both walks skip the directory now, and the parity test's synthetic tree grows a fixture and a `.spec.tsx` so the two implementations are held equal on the whole exclusion, not just the part the first case exercised. Reverting one side reddens the local case AND both parity cases, which is what that guard is for. Verified the other direction too, since an exclusion can overshoot: every review source that reaches `dist` is still covered. `DESIGN.md` and `SKILL.md` both ship and both remain in the digest — checked, not assumed, after two rounds of this exact mistake. Six cases end to end after a rebuild: a clean tree, a test edit and a fixture edit are silent; a production edit, a `review.ts` edit and a `DESIGN.md` edit each warn. * fix(review): allowlist the stamp, and stop guessing what the bundle holds The Critical first: `create-standalone-package.js` fails on any top-level dist entry outside its allowlist, and `review-sources.sha256` was on neither list. The next release would have aborted the standalone archive on all five targets, and no PR-time job runs the packager, which is why this suite is green. Allowlisted -- shipping it is harmless, since a standalone install has no `packages/` to compare against and the check stays silent there. `lib/test-utils.ts` was in the digest: test support with a production-looking name, imported by two test files and nothing else. That is the fourth patch to one rule -- `.test.ts`, then `__fixtures__/`, then this, plus `.DS_Store` -- and each was found by a reviewer after it shipped. So the rule stops being a list somebody remembers to extend: a new test asserts the property the list approximates, that every file the digest folds in is reachable from production code and nothing reachable is left out. Dropping `test-utils.ts` from the exclusion reddens it, which is the fifth instance failing in CI instead of in a review. Three branches that no test reached, each with a mutant the review measured surviving the whole suite: the walk's symlink skip (a directory cycle would send the first command of every review into unbounded recursion), the read-failure path (hashing the survivors of a concurrent checkout would accuse a tree that is merely mid-change), and the build's stamp call site (removing it left the scripts suite green while `npm run bundle` silently stopped writing the stamp). All three now redden. And `unmeasured` had no reader, so the one edge this check cannot measure but can see -- sources present, stamp absent -- passed in silence. That is the state of every existing checkout the moment this ships, and it is exactly the silent failure the change was written to end. It now says so, while an installed package, which has no sources either, still says nothing. * fix(review): the guard was shallower than the property it claimed The guard added last round asserts that every file in the digest is reachable from production code. It did not: a file imported by nothing passed, because the filter also required some test to import it; only `.ts` was inspected, so a test-only `.tsx` or `.mts` helper walked through; and it read static imports only, while this directory has nine `await import('./…')` edges. It asserts the property now — every extension, orphans included, dynamic edges seen — and the tree has no violators, so the strictness cost nothing today and is there for the next file. `__snapshots__` joins the exclusions. `vitest --update` regenerating a snapshot would have moved the digest with the bundle byte-identical; none exists under the review roots today only by chance, and 120 `toMatchSnapshot()` calls live elsewhere in this package. Three couplings that no test held: - the allowlist entry that fixed the release-breaking R2-1 -- reverting those five lines left the whole scripts suite green, and the next failure would have been a release aborting on all five targets. `isAllowedDistEntry` is exported and the stamp's own name is asserted against it, so a one-sided rename fails here instead; - the `.DS_Store` member of `NOT_BUNDLED_FILE`, absent from the repo and so from the parity tree -- one-sided removal stayed green while a macOS checkout would digest differently on the two sides forever; - each `unmeasured` reason. Swapping the two arguments at the single call site kept all 76 tests green while telling a pre-stamp checkout its sources were missing. And two comments that said the opposite of the code beneath them: the digest is computed unconditionally on purpose (the pre-stamp notice needs it), and `NOT_BUNDLED_FILE` helpers are deliberately not importers, since nothing reaches the bundle through a file the bundle does not contain. The two stderr diagnostics are documented for users, beside the sibling paragraph this PR already added. * fix(review): measure only the layout that can carry a stamp `npm start` launches `node <root>/packages/cli`, and node sets `argv[1]` to that directory -- so the derivation found sources under `<root>` with no stamp beside them and printed "could not check" on every review, forever, with advice that could never make it stop. That is the fires-when-nothing-is-wrong failure this change argues against, on the path `start.js` sets `QWEN_CODE_CLI` to precisely so reviews reach that build. Only a `<root>/dist/cli.js` layout is measured now; anything else has no stamp to find and no way to grow one. The build-side digest could kill `npm run bundle` where the check side degrades gracefully: a file vanishing mid-walk threw out of the hash loop, and the stamp is the copier's last step, so the build would fail with every asset already in place. Caught and skipped -- a missing stamp is `unmeasured`, which the runtime already treats as an acceptable answer. The skill now says what to do with the warning, which is the half that makes it reach a human: `parse-args` runs inside an agent's shell tool, the user reads the agent's summary rather than raw stderr, and a line nobody repeats is a line nobody sees -- which is how the 2026-08-02 round went wrong in the first place. It also records that the instruction cannot help the run that needs it, since the skill comes from the same bundle. And the scope is stated where silence could be over-read: the digest covers the review commands, the file that registers them, and the bundled skill -- not the shared helpers those import. A quiet run means the review code matches the bundle, not that the tree does. * fix(review): refuse to certify a bundle the copier may not describe The stamp described the tree as the COPIER saw it, and the copier runs after esbuild -- so a source edited in between, or `copy_bundle_assets.js` run on its own (it self-executes), wrote a digest certifying a `cli.js` built from something else. Silence then means "verified fresh" when it is not, and that is the only direction here where a quiet run is affirmatively wrong rather than merely uninformative: every other gap degrades to `unmeasured`. Timestamps are the wrong tool for judging staleness and the right one for judging whether this stamp can be honest at all, so the build refuses when any source is newer than the bundle it would attest to, and says why. Driven for real: touching a review source and running the copier alone now prints "skipped the source digest rather than certify a bundle it may not describe". `it('counts the same files')` compared nothing -- it asserted `> 50` on the build side while the check side exposes no count, so the title claimed a parity the body never checked, and the margin over the real 56 made it a future false alarm in `scripts/` for an unrelated change. Removed; the digest parity already holds the file set. "Root is a file" was inferred from `readdirSync` raising ENOTDIR, an assumption about every platform's libuv on the one root that is a file -- `review.ts`, where "a new subcommand was registered" lives. `statSync(root).isFile()` says it instead. And the check itself moves out of the handler into `bundleStalenessNotices`, which is where the rest of it already lived. `parse-args` is about parsing arguments again, the wording is testable without the yargs harness, and a second caller -- an agent resuming a review never runs step 1 -- is one line. * fix(review): align the twin walk, and stop a test from passing on nothing The build side still inferred "this root is a file" from `readdirSync` raising ENOTDIR, one commit after the check side stopped doing exactly that and said why. A platform that maps the case differently would drop `commands/review.ts` from one digest and not the other, and a byte-for-byte correct bundle would warn on every review forever, on that platform alone, with rebuilding reproducing the same one-sided walk. Both sides ask `statSync(...).isFile()` now. Fixing one half of a pair and not the other is the mistake this file keeps making. The filename parity test had been passing on nothing since the previous commit: it matched `writeFileSync(join(distDir, '…'))` against the script's source, the literal moved into a `stampPath` variable, and the regex returned `undefined` so the assertion compared against nothing. It runs the build against a fixture now and reads the name off `dist/`, so it measures what the build does instead of what its source looks like. Renaming the stamp on one side reddens it. Also from review: the duplicated comment block in `parse-args`; an unreadable source now says the check could not run rather than passing in the same silence as an installed package, which is what the docstring already promised; the "could not check" line no longer asserts that the checkout predates the feature, since the build has three refusal paths and one of them means the opposite; every refusal removes an existing stamp, because leaving an older attestation beside a newer bundle is a weaker form of the certifying it refuses; and `drive` calls the check, which the module comment argued for and the diff had not done -- a resumed review never runs step 1, and that is where the long work starts. * fix(review): pin the regex group the parity tree missed, and say source, not command * fix(review): allowlist what the bundle holds, and cover the drive notice (#8390) * fix(review): treat unreadable review sources as unmeasured (#8390) * test(review): pin the stamp guard mutations that survived the suite (#8390) * fix(review): close staleness-check gaps and pin the round-4 survivors (#8390) * fix(review): close round-5 staleness gaps for parity, refusals, and partial checkouts (#8390) * fix(review): close round-6 gaps in the clause classifier, symlink layout, and pin honesty (#8390) * fix(review): close round-7 gaps in the closure oracle, parity pin, and refusal pins (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): close round-8 gaps from the maintainer review (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): close round-9 nits from the maintainer review (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): pin the lease root in the synthetic digest parity case (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Autofix <autofix@qwen-code.dev> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
10291e16f2
|
refactor(core): move review skill incident narratives to DESIGN.md (#8499)
* perf(review): issue independent setup calls in one response Measured on a real small-PR run: the stretch from parse-args to the first agent launch took 7 minutes of wall clock, one round-trip at a time, on calls that never needed an order — pr-context, comment-status and the Step 2 rules load are mutually independent reads. Step 1 now tells the orchestrator to issue all three in a single response (the same rule Step 3 already enforces for the agent fan-out) and to page their outputs in shared responses too. comment-status loses its wait-for-the-context-file guard in worktree mode: learning whether inline comments exist cost a serial round-trip, while running it on a commentless PR just writes an empty index. Step 6's two deterministic gates (script-lint, test-plan) get the same one-response note. The orderings that matter are kept explicit: fetch-pr before everything (it creates the worktree and the plan), the roster after the rules load (it bakes the rules into every brief). * refactor(core): move review skill incident narratives to DESIGN.md SKILL.md is injected wholesale into the review orchestrator's context on every /review run and re-billed on each of its turns, and ~16KB of it was incident narrative — accounts of past dogfood failures and measurements that justify rules but are not themselves instructions. Move 50 such narrative blocks into a new 'Measured incidents (moved from SKILL.md)' section of DESIGN.md (47 anchors, not loaded at runtime), leaving every rule in place with a short '(measured; DESIGN.md — <anchor>)' pointer. Force-bearing figures stay inline where the number is the argument (e.g. the ~161s cold npm ci, the 41% test-code median, the PR #6457 one-of-five checklist measurement). No instruction, gate, format, flag, threshold, or ordering changed; the YAML frontmatter and all 35 fenced code blocks are byte-identical, and the MUST / Do not / never imperative counts are unchanged outside the moved narrative text (verified by script). SKILL.md: 237,847 -> 228,266 bytes; DESIGN.md: 106,708 -> 125,184 bytes. * fix(review): keep DESIGN.md out of the runtime bundle and pin pointers The slim refactor left DESIGN.md shipped beside SKILL.md in dist/bundled/, so one curious read_file of the 125 KB maintainer document would cost more context than the refactor saves. The bundle copy now skips DESIGN.md, and SKILL.md gains a one-line guard telling the orchestrator the pointers are for humans auditing a rule. Also addresses review feedback: a test pins both directions of the SKILL.md incident-pointer mapping, the transcribed-argument narrative keeps its referent after the move, the incidents section title loses its changelog suffix, and Step 2 no longer asks for a base fetch that fetch-pr already performed. * fix(review): gate setup batching by effort and consolidate incident blocks Address round-1 review feedback on the skill-slim PR: - Gate the ONE-response setup batch and the comment-status call to high and medium effort, matching Step 2's low-effort skip. - Scope the Step 6 lint/test-plan batching to same-repo PR reviews. - Merge same-run incident blocks (self-composed Approve into the paraphrased roster prompt; archive verdict into the narrated-away cap), cross-reference the roster-size and relocated-Critical tellings, and state the #8368 path in its block plus the pointer it was missing. - Pointer-ize the last inline QQChannel narrative and fix the scripts-nobody-ran summary to match its block. - Extend the DESIGN.md exclusion to copy_files.js so the transpiled dist/src build and the published core tarball stop shipping it. - Pin the no-read_file guard and the batch ordering constraints in SKILL.test.ts, and fail loudly on pointers the regex cannot parse. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> |
||
qqqys
|
62e11a5732
|
feat(core): add dataviz bundled skill (#6198)
* feat(core): add dataviz bundled skill * fix(core): harden dataviz palette validator * fix(core): harden dataviz validator review gaps * test(core): declare node global in dataviz test * fix(core): harden dataviz validator cli * fix(core): tighten dataviz validator packaging * fix(core): tighten dataviz skill packaging |
||
|
Shaojin Wen
|
715ef938f5
|
feat(cli): serve the Web Shell UI from qwen serve (#5392)
* feat(cli): serve the Web Shell UI from `qwen serve` `qwen serve` now serves the built Web Shell SPA at its root on the same origin as the API, so a released binary exposes the browser terminal without the dev-only Vite server (the `npm run dev:daemon` two-process setup is unchanged for development). - New `webShellStatic.ts` mounts `/`, `/assets/*` and an SPA deep-link fallback. The fallback uses the same document-navigation discriminator as the Vite dev proxy so it never shadows API JSON 404s. - The static shell is registered BEFORE bearerAuth (a browser can't attach a token to a `<script>` subresource or an address-bar navigation; the shell carries no secrets and every API route stays token-gated). HTML responses set CSP + X-Frame-Options + Referrer-Policy + no-cache. - `--open` launches the browser at the daemon URL (with `?token=` when set) once the listener is up, guarded by `shouldLaunchBrowser()`. - `--no-web` opts out for an API-only daemon. - Bundle / npm publish / standalone packaging now ship `dist/web-shell/`. Missing assets degrade to API-only with a breadcrumb, never a hard fail. Tests: +6 cases in server.test.ts (root shell, assets, SPA fallback, non-navigation 404 passthrough, security headers, --no-web off). * fix(cli): address review on Web Shell serving Review fixes for #5392 (qwen-code-ci-bot): - [Critical] SPA fallback no longer shadows /health or /demo on non-loopback binds — those paths fall through to their own routes / bearerAuth instead of receiving index.html. - [Critical] --open trims the bearer token before putting it in the browser URL, matching runQwenServe's own trimming, so a trailing newline from `$(cat token.txt)` no longer makes every API call 401. - --open is wrapped in its own try/catch so a failed browser launch can't take down the already-listening daemon; it normalizes wildcard binds (0.0.0.0 / ::) to loopback, and only fires when the UI is actually mounted (new RunHandle.webShellMounted). - resolveWebShellDir() now requires BOTH index.html and assets/, so a partial build degrades to API-only instead of serving a shell whose chunks 404. - runQwenServe logs a positive "Web Shell UI served from <dir>" breadcrumb, and warns that on a non-loopback bind without --allow-origin the shell is read-only (same-origin POSTs are blocked by the CORS wall). - Document the --open token-in-process-list exposure in help text + a stderr note when a token is forwarded. - Tests: POST method guard, sec-fetch navigation signal, /health not shadowed, sendFile 500 path, plus isDocumentNavigation and resolveWebShellDir units. * fix(cli): harden Web Shell asset resolution and send-error logging Second-round review (claude /qreview on the initial commit): - resolveWebShellDir() now walks up from this module to find a sibling packages/web-shell/dist, covering the transpiled layouts the previous fixed `..` depth missed — per-package `tsc` output and the integration daemon harness (packages/cli/dist/index.js), which would otherwise resolve to nonexistent paths and silently run API-only. - sendFile failures are no longer silent: log the error (matching the /demo handler — previously the only 5xx path that emitted nothing) and res.end() a half-streamed response instead of leaving the client on a 200 with a partial body. The remaining comment (open-browser inside the boot try) was already fixed in |
||
kkhomej33-netizen
|
45efb1d3aa
|
fix(cli): bundle extension examples (#4719) | ||
Yan Shen
|
9bd5a0180b
|
feat(cli): core built-in i18n coverage (#3871)
* feat(i18n): expand built-in locale coverage * feat(cli): add dynamic slash command translation * test(cli): stabilize session picker assertions * fix(core): close jsonl readers before cleanup * fix: address i18n review regressions * fix(cli): address dynamic i18n review findings * fix(cli): address i18n review follow-ups * fix(cli): address i18n review feedback * test(cli): align i18n parity coverage with strict locales * fix(cli): address i18n review findings |
||
LaZzyMan
|
7a6b725b0c |
feat: replace qwen-settings-config with bundled qc-helper skill
- Remove project-level qwen-settings-config skill and its references/ - Create bundled qc-helper skill at packages/core/src/skills/bundled/ that references docs/users/ for answering usage/config questions - Update copy_bundle_assets.js to copy docs/users/ into dist/bundled/qc-helper/docs/ - Update dev.js to create symlink for dev mode docs access - Add bundled docs directory verification in prepare-package.js - Revert doc-update skills (docs-audit-and-refresh, docs-update-from-diff) to main branch versions |
||
|
Shaojin Wen
|
1359563f45
|
feat(skills): add bundled /review skill for out-of-the-box code review (#2348)
feat(skills): add bundled /review skill for out-of-the-box code review |
||
tanzhenxin
|
be633a80cc
|
📦 Release qwen-code CLI as a Standalone Bundled Package (#866) | ||
Pascal Birchler
|
ee4feea006
|
chore: consistently import node modules with prefix (#3013)
Co-authored-by: N. Taylor Mullen <ntaylormullen@google.com> |
||
Shreya Keshive
|
3a87712c1a
|
Launch VS Code IDE Integration (#6063) | ||
|
Shreya Keshive
|
e4ed1aabac
|
Include companion VS Code extension VSIX as part of build/release (#4254) | ||
Billy Biggs
|
25cdf9b762
|
Inline the description and schema of the shell tool in the source (#1709) | ||
|
Billy Biggs
|
e717c51aa1
|
Avoid import.meta.dirname to be backwards compatible to Node.js 18+ (#1058) | ||
matt korwel
|
3b943c1582
|
Windows: Refactor Shell Scripts to Node.js for Cross-Platform Compatibility (#784) |