mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-10 17:27:10 +00:00
186812694c
* feat(review): publish-assets — evidence images for PR review comments
GitHub's API cannot attach images to review comments (the web UI's
drag-and-drop upload has no API equivalent), so a review whose evidence is a
screenshot — a TUI rendering, a before/after comparison — had no way to show
it. New `qwen review publish-assets` hosts evidence images in a
user-designated repository and hands back URLs a comment can embed.
Grew from the maintainer's manual workflow (screenshots pushed to
`pr-assets/<PR>-verify` branches over HTTPS), and inherits the shape of the
skill's only other public write (`submit`) deliberately:
- Designated destination: writes only to QWEN_REVIEW_ASSETS_REPO, an
owner/repo the user set by hand — the reviewed repo for maintainers, a fork
or scratch repo otherwise (fork-vs-in-repo becomes a configuration
difference, not two code paths). A separate variable from
QWEN_REVIEW_SCRATCH_REPO on purpose: that contract forbids PR-derived
content, and evidence screenshots are exactly that. Unset → exit 3.
- Authorised run: the same args-file re-parse and target binding as submit,
now extracted to a shared lib/authorization.ts so the two gates cannot
drift (the target-binding lesson lives in one place). Since an effective
--comment forces high effort, low/medium runs can never publish.
- Images only, capped, all-or-nothing: extension allowlist (SVG excluded — a
script container), per-file and per-batch size caps, one refused file
refuses the batch before anything is pushed.
- Immutable references: files land on pr-assets/<pr>-review via the Contents
API (HTTPS via gh; no clone, no SSH), content-hash-named so re-runs are
idempotent, and every URL is pinned to the commit — a posted comment's
evidence cannot be changed from under it. The web-host /raw/ URL form works
unchanged on GitHub Enterprise.
- Auditable: a manifest names every file pushed and the landing commit,
swept by cleanup with the other review artifacts.
The findings artifact gains per-finding `assetFiles` (local evidence paths)
and `assets` (published URLs); `publish-assets --findings/--findings-out`
publishes everything referenced and weaves the URLs back in, so the comment
builder reads the artifact rather than hand-carrying URLs.
What the command cannot check is stated in SKILL.md instead: image content.
Publish only evidence the review itself produced — never a capture of the
user's own terminal, which can hold an env dump in the scrollback.
Tests: 45 files / 1394 assertions — new suites for the assets naming and
validation rules and the command's gates (refusal without designation,
refusal without authorisation, target binding, branch creation, idempotent
re-run, batch refusal, findings weaving); submit's 42 pass unchanged on the
extracted gate.
* fix(review): publish-assets round-1 self-review — six findings
Round-1 review of this branch, walked with the angles the author-side pass
does not cover:
- submit.ts kept its parseReviewArgs import after the authorization
extraction; vitest does not typecheck, `tsc --build` does, and CI's build
leg failed on TS6133. (The whole first CI round's failures cascade from
this one break.)
- ensureBranch %2F-encoded the slashed ref path; GitHub's documented form is
literal slashes and %2F routes inconsistently across endpoints — a 404
here reads as "branch missing" and turns every re-run into a 422 on the
create. Ref paths are now literal (the branch name is built from a
validated integer, so interpolation is safe); the contents `?ref=` query
VALUE keeps its encoding, which is the correct position for it.
- The authorization gate bound URL-shaped `--comment` arguments against the
ASSETS repo, refusing legitimately authorised runs whenever the assets repo
is a fork rather than the reviewed repo. The shared gate's repo binding is
now optional — submit still always binds it; publish-assets binds the PR
number (and host) alone, with a new optional --reviewed-repo to restore
the stronger binding when the orchestrator knows the reviewed repo.
- URLs were pinned to the last PUT response's commit.sha; on an
identical-content update that field's shape is GitHub's to decide, not
ours to assume. The head is now read from the branch ref after the
uploads — one extra call for independence from the response shape.
- putContent's catch-all retried EVERY failure through the exists path,
answering a 401 with a confusing secondary error from the sha lookup; the
retry now fires only on the 422/needs-sha shape and rethrows the rest.
- --findings without --findings-out silently skipped the URL weaving; it
now warns, and --findings-out implies --findings.
New tests: literal-ref assertion, non-exists rethrow, URL-shaped
authorisation without assets-repo binding, --reviewed-repo mismatch refusal.
45 files / 1399 assertions green; `tsc --build` clean.
* fix(review): publish-assets round-2 — empty-findings no-op, reviewed-repo hint
Round-2 findings on this branch:
- A findings artifact carrying no assetFiles is the ORDINARY case for most
reviews, but publish-assets answered it with exit 3 — a refusal an
orchestrator calling the command unconditionally on every posting run
would read as a failure to repair. It is now a no-op (exit 0,
{published:false, count:0}); a bare --files with nothing named keeps the
exit-3 refusal, because there the emptiness IS the caller error.
- SKILL.md's example now names --reviewed-repo for URL-target reviews, so
the stronger authorisation binding is used where the orchestrator knows
the reviewed repo.
44 files / 1387 assertions green; tsc --build clean.
* test(review): fix invalid two-argument expect in assets.test.ts
Round-3 sweep: vitest's expect takes one argument — the message-style second
argument was a lint error and a weak assertion both. The offending value now
rides inside the asserted object, so a regression names which shape slipped
through instead of reporting 'expected true'.
* test(review): pin the findings schema's evidence-asset validation directly
Round-4 sweep: assetFiles/assets were exercised only through publish-assets'
weaving test — the schema's own rejection paths (non-array, empty-string
entry, empty-array drop) had no direct case, so a validation regression
would have surfaced as a confusing weaving failure two layers up.
* fix(review): address all six findings from the automatic review (R1-1..R1-6)
The /review pipeline's own round-1 findings on this PR, each confirmed and
fixed:
- R1-1 (the real catch): the host-binding check sat nested inside the
`req.repo !== undefined` guard, so a caller omitting --reviewed-repo also
silently skipped the HOST binding — contradicting the documented "binds
the PR number (and host) alone". The host check now stands on its own;
a new test pins an Enterprise-host mismatch refusal with the repo binding
absent.
- R1-2: --pr accepted whatever yargs `type:'number'` passed through (NaN,
0, 3.5), and --user-authorized bypasses the gate that would have
re-parsed the target — `pr-assets/NaN-review` was reachable. A Gate-0
positive-integer check now refuses first, matching submit's sibling
discipline.
- R1-3: the suite drove the skillArgs seam without clearing
QWEN_CODE_SESSION_ID, so running it inside an active Qwen Code session
spuriously failed eight tests. beforeEach now saves/clears the variable
and afterEach restores it.
- R1-4: the 40MB aggregate cap was enforced inline and untested (a mutation
deleting it stayed green). The per-file rules and the total cap now live
in one pure ruling, validateAssetBatch, unit-tested with five 9MB sizes
and no fixtures.
- R1-5: the asset_files snake_case alias was the one untested member of an
otherwise-tested alias family; pinned.
- R1-6: the setGhHost wiring had no command-level assertion; a GHE test now
pins both the call and the host-carrying manifest URLs.
44 files / 1397 assertions green; tsc --build and eslint clean.
* fix(review): address all ten round-2 findings from the automatic review
Round-2 of the /review pipeline on this PR: 2 Critical, 8 Suggestions,
every one confirmed against the code and fixed.
Criticals:
- The round-2 test block added for the empty-findings no-op omitted the
QWEN_CODE_SESSION_ID save/delete/restore its two sibling blocks perform,
so the suite spuriously failed inside an active Qwen Code session — the
exact dogfooding environment this repo reviews from.
- The gh routing and the returned URLs read the host from two different
sources: with --host absent, gh children inherit an operator-exported
GH_HOST (routing at Enterprise) while rawAssetUrl defaulted to
github.com — every returned URL a 404. One effectiveHost (flag, then
GH_HOST env) now feeds both.
Suggestions:
- putContent's retry discriminator matched a bare `422` anywhere in
err.message — which execFileSync fills with the full command line,
including the PR-numbered remote path: evidence for PR #4220 would read
a 401 as "already exists". Anchored to `HTTP 422`.
- ensureBranch's bare catch read every ref-lookup failure (401, 403
rate-limit) as "branch missing"; only HTTP 404 takes the create path
now, and an empty assets repo — whose default_branch resolves while its
head ref 404s — is named as the condition it is, with the fix stated.
- Validation refusals threw (yargs exit 1, stack trace, empty stdout)
while every other gate in the command answers exit 3 +
{"published": false}; unreadable files and batch refusals now speak the
same refusal language.
- The command's idempotent writes (content-hashed PUTs, a ref create
whose duplicate is tolerated) now go through a new ghWithInputRetried —
sharing gh()'s transient-error retry — and ghWithInput's no-retry
docstring names the two-caller split instead of claiming a sole caller.
- parseAssetsRepo admitted dot-segment repos (`owner/..`) its docstring
claimed were path-safe; segments now exclude `.`/`..`, mirroring
submit's isRepo.
- stringArray accepted whitespace-only evidence paths; trim(), matching
the sibling asString.
- The GHE test asserted setGhHost was called but not WHEN; it now asserts
the call precedes the first API invocation.
44 files / 1403 assertions green; tsc --build and eslint clean.
* refactor(review): one refusal helper for every publish-assets gate
Round-2 of this branch's fresh review: the refuse() helper existed below
seven inline copies of the identical three-line refusal — the drift shape
where one site eventually forgets the exit code. Hoisted to the top of
runPublishAssets and used by every gate; message content unchanged where
tests pin it. 26/26; tsc clean.
* fix(review): address the round-3 review — bidirectional host binding and 14 more
The automatic review's third round on this PR: 1 Critical + 14
Suggestions, each verified and addressed.
The Critical (host binding, both halves):
- The gate's `req.host &&` guard bound the host in one direction only —
an Enterprise-URL authorisation admitted a host-less write routed at
github.com (or wherever GH_HOST pointed). The gate now compares the
authorised host against the write's EFFECTIVE host, defaulting an
absent req.host to github.com: a host is a host, not an exemption.
- Both callers fed the gate the flag rather than the route: publish-assets
computed effectiveHost (--host ?? GH_HOST) AFTER the gate and bound
args.host; submit bound args.host while its gh child inherited GH_HOST.
publish-assets now resolves effectiveHost before Gate 2 and binds it;
submit binds the same resolution.
The rest:
- pr-assets/<N>-review registered in the asset-branch cleanup workflow,
per its own every-producer-must-be-added-here rule — a branch nothing
deletes is permanent.
- ghWithInputRetried had been inserted between ghWithInput and its JSDoc,
leaving the does-NOT-retry comment attached to the function that DOES
retry; each function now carries its own doc.
- putContent's retry-path contents-GET is wrapped: when the 422 was not
the sha-missing shape and the path does not exist, the GET's 404 no
longer replaces the PUT error the user needs.
- stringArray treats null as absent like every sibling parser, so an
artifact rendering "no assets" as null canonicalizes instead of
crashing.
- Test isolation, all four describe blocks: GH_HOST save/delete/restore,
setGhHostMock.mockReset (a sibling's persistent throwing implementation
survives mockClear — the malformed-host test also switched to
mockImplementationOnce), and full mock resets in the blocks that lacked
them.
- The two regression-pin tests the review measured vacuous now
discriminate: each fails only the one call under test and asserts the
pipeline stopped THERE (no contents PUT after a bad create; exactly one
gh call after a 403 lookup).
- New positive pins: a double-fired create ("Reference already exists")
succeeds; the canonical report shape this command's own --findings-out
writes round-trips; an Enterprise-URL authorisation refuses a host-less
write while a github.com-URL one passes it.
Not changed: the finding that reverting the Finding-interface hunk leaves
tests green — the fields are type-level and their removal fails
tsc --build (the CI leg that caught this branch's own TS6133); a runtime
pin would duplicate what the round-trip tests already exercise.
47 files / 1495 assertions green; tsc --build 0 errors; actionlint clean
on the cleanup workflow.
* fix(review): address the round-4 review — empty-GH_HOST passthrough and four test pins
Round 4 came back COMMENTED (down from CHANGES_REQUESTED), 5 Suggestions,
0 Criticals — all five confirmed and fixed:
- An exported-but-empty GH_HOST ("" from an unset workflow var) survives
`??`, being non-nullish: effectiveHost became "" and the gate compared
the authorised host against "", refusing a legitimate github.com write.
Both call sites now collapse an empty trim to undefined (`|| undefined`,
parenthesized).
- The gate's URL-shaped repo/host binding was exercised only via
publish-assets' suite; submit.test.ts now pins both directions of the
host binding and the repo binding at its own call site.
- ghWithInputRetried had no retry-contract test; gh.test.ts adds the
symmetric block to ghWithInput's does-NOT-retry pin (transient 500
retried once then succeeds; non-transient 401 single call).
- The publish-assets mock aliased ghWithInput and ghWithInputRetried to
one mock, hiding which variant a write used; they are two mocks now,
and the happy path asserts the non-retrying variant is never touched.
- The Prepared interface's dead `name` field is gone.
46 files / 1476 assertions green; tsc --build and eslint clean.
|
||
|---|---|---|
| .. | ||
| configuration | ||
| extension | ||
| features | ||
| ide-integration | ||
| reference | ||
| support | ||
| _meta.ts | ||
| common-workflow.md | ||
| integration-github-action.md | ||
| integration-jetbrains.md | ||
| integration-vscode.md | ||
| integration-zed.md | ||
| overview.md | ||
| quickstart.md | ||
| qwen-serve-deploy-local.md | ||
| qwen-serve.md | ||