mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-25 16:44:36 +00:00
3 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
易良
|
56db17bd4c
|
refactor(cli): enforce utils leaf-layer dependency direction (#9146) (#9737)
* refactor(cli): enforce utils leaf-layer dependency direction (#9146) Move domain-coupled modules out of packages/cli/src/utils into the directories that own them: config/ (dialogScopeUtils, settingsUtils), i18n/ (languageUtils), ui/ (handleAutoUpdate, standalone-update, systemInfo, systemInfoFields, update-relaunch, commands, doctorChecks), nonInteractive/ (nonInteractiveHelpers, chat-recording-failure, tool-result-boundary-diagnostics, permission-suggestions), serve/ (sandbox), services/housekeeping/ (scheduler, non-interactive-scheduler), and commands/review/ (findings). Extract the generic normalizePartList helper into utils/normalize-part-list.ts so utils consumers keep importing downward, and move the MergeStrategy enum into utils/deepMerge.ts (its owner). Add an eslint architecture rule (no-utils-upward-import) that forbids value imports from utils/ back up into a domain directory. Type-only imports stay exempt: they are erased at compile time and cannot create a runtime cycle (Settings in modelConfigUtils, CommandContext in sessionPaths). No behavior change: typecheck, build, and the affected unit tests pass. * fix: use Qwen Team 2026 license header on new files (#9146) * chore: refresh stale utils/ path references after leaf-layer move (#9146) * docs: reconcile no-utils-upward-import header with the allowed type-only set (#9146) * fix(cli): allowlist sandbox process.env accesses after leaf-layer move (#9146) * chore(ci): re-record qwen-autofix.yml size baseline after #9677 (#9146) #9677 recorded qwen-autofix.yml at 392111 bytes while the file it committed was already 397656, so every PR that merged main after it tripped the growth ratchet. Re-record the actual size; the file itself is unchanged by this PR. * fix(review): drop the stale utils/findings.ts digest root after the leaf-layer move (#9146) The #9146 move returned findings.ts to commands/review/, but the digest root lists merged from main still pinned it under utils/, where the file no longer exists — the absent root darkened every review's staleness check and failed review-source-digest.test.ts. Drop the stale file-shaped root from both digest copies and their pins; the commands/review/ directory root covers the validator at its new home, and the two utils helpers keep their file-shaped roots. * fix(review): colocate seatbelt profiles with the sandbox module (#9146) * fix(review): exempt inline type-only specifiers from the utils upward-import rule (#9146) * fix(review): report upward inline type-specifier imports under verbatimModuleSyntax (#9146) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(review): pin mixed-specifier and zero-specifier upward imports in the utils rule (#9146) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(review): anchor the nested-checkout utils rule fixture on the last marker (#9146) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(review): pin that the utils/findings.ts digest root stays removed (#9146) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): reword stale-bundle SCOPE header to the post-move helper shape (#9146) * test(review): drop the pre-move utils/findings.ts from the skill-parity fixture (#9146) * test(serve): derive the seatbelt colocation tripwire from BUILTIN_SEATBELT_PROFILES (#9146) * fix(architecture): fail closed on computed dynamic imports in the utils leaf rule (#9146) * fix(cli): point settings.test.ts at the post-move settingsUtils path (#9146) main updated settings.test.ts after this branch moved settingsUtils.ts from utils/ into config/, and the merge kept main's old import specifier, which vite fails to resolve. Repoint it at ./settingsUtils.js; every other consumer already uses the new path. * fix(cli): close utils boundary review gaps * test(cli): cover utils boundary allow paths --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
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
|
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> |