diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index d5c0819f537..2c178a26da3 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -78,6 +78,15 @@ jobs: # KEEP IN SYNC with review-pr.if (explicit-trigger branches). # Authorization is delegated to the `authorize` job (write+ permission); # this `if` only matches the /review command shape. + # + # The command may be followed by a newline and a body, so the shape match + # accepts that — via fromJSON, because expression string literals are NOT + # escape-processed: '\n' there is a literal backslash + n, and the branch + # written that way never matched anything. fromJSON('"\n"') is JSON, which + # IS escape-processed, so it yields a real newline. Both line endings are + # listed: the API sends LF, the web UI sends CRLF, and startsWith with an + # LF pattern does not match a CRLF body. Measured on a live runner, not + # assumed — see the PR that introduced this. needs: ['authorize'] if: |- !cancelled() && @@ -87,17 +96,20 @@ jobs: github.event.issue.state == 'open' && (github.event.comment.body == '@qwen-code /review' || startsWith(github.event.comment.body, '@qwen-code /review ') || - startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n')))) || + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) || + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"'))))) || (github.event_name == 'pull_request_review_comment' && github.event.pull_request.state == 'open' && (github.event.comment.body == '@qwen-code /review' || startsWith(github.event.comment.body, '@qwen-code /review ') || - startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n')))) || + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) || + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"'))))) || (github.event_name == 'pull_request_review' && github.event.pull_request.state == 'open' && (github.event.review.body == '@qwen-code /review' || startsWith(github.event.review.body, '@qwen-code /review ') || - startsWith(github.event.review.body, format('@qwen-code /review{0}', '\n'))))) + startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) || + startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\r"')))))) concurrency: group: 'qwen-pr-ack-${{ github.event.issue.number || github.event.pull_request.number }}' cancel-in-progress: false @@ -315,7 +327,8 @@ jobs: # review_requested events check the requester and skip delay): # - opened/synchronize uses delay-automatic-review # - reopened/ready_for_review runs immediately - # KEEP IN SYNC with ack-review-request.if (explicit-trigger branches). + # KEEP IN SYNC with ack-review-request.if (explicit-trigger branches) — + # including the fromJSON newline/CR pair, explained there. if: |- !cancelled() && ((github.event_name == 'workflow_dispatch' && @@ -335,19 +348,22 @@ jobs: github.event.issue.state == 'open' && (github.event.comment.body == '@qwen-code /review' || startsWith(github.event.comment.body, '@qwen-code /review ') || - startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n'))) && + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) || + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"')))) && needs.authorize.outputs.should_review == 'true') || (github.event_name == 'pull_request_review_comment' && github.event.pull_request.state == 'open' && (github.event.comment.body == '@qwen-code /review' || startsWith(github.event.comment.body, '@qwen-code /review ') || - startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n'))) && + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) || + startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"')))) && needs.authorize.outputs.should_review == 'true') || (github.event_name == 'pull_request_review' && github.event.pull_request.state == 'open' && (github.event.review.body == '@qwen-code /review' || startsWith(github.event.review.body, '@qwen-code /review ') || - startsWith(github.event.review.body, format('@qwen-code /review{0}', '\n'))) && + startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) || + startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\r"')))) && needs.authorize.outputs.should_review == 'true')) # The per-review budget auto-scales to QWEN_REVIEW_MAX_TIMEOUT_MINUTES # for any non-small PR (see "Run review"), and the shared retry budget @@ -450,7 +466,13 @@ jobs: # review" is an explicit ask (authorize write-permission-checks the # requester for exactly that action), so it is excluded below. AUTO_REVIEW=false + # First line only, then drop a trailing CR: comments written in the + # GitHub web UI arrive CRLF-terminated, so without this the command + # line keeps a `\r`. That rides through word splitting (IFS has no + # CR) into tokens like `--timeout=300`, which then fails the + # numeric check for no visible reason. TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}" + TRIGGER_COMMAND="${TRIGGER_COMMAND%$'\r'}" if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then PR_NUMBER="${{ github.event.inputs.pr_number }}" @@ -1604,6 +1626,8 @@ jobs: resolve-pr: needs: ['authorize'] + # The /resolve shape match uses the same fromJSON newline/CR pair as + # ack-review-request.if, and for the same reason — see the note there. if: |- !cancelled() && github.repository == 'QwenLM/qwen-code' && @@ -1616,7 +1640,8 @@ jobs: github.event.issue.state == 'open' && (github.event.comment.body == '@qwen-code /resolve' || startsWith(github.event.comment.body, '@qwen-code /resolve ') || - startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', '\n')))) + startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', fromJSON('"\n"'))) || + startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', fromJSON('"\r"'))))) ) # Pinned to an ephemeral hosted runner. The conflict-resolution agent step # runs with `sandbox: true`, which on Linux needs docker or podman to launch diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index affc71a6c0b..52c8c7cb17c 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2207,3 +2207,70 @@ describe('workflow expression length', () => { expect(runReviewStep()).not.toContain('${{'); }); }); + +describe('command shape matching', () => { + // A comment may be `@qwen-code /review` followed by a newline and a body. + // The `if`s tried to accept that with format('…{0}', '\n'), but expression + // string literals are NOT escape-processed: that '\n' is a literal + // backslash + n, so the branch matched nothing and every multi-line command + // was silently ignored — no run, no feedback. Measured on a live runner: + // startsWith(, format('…{0}', '\n')) => false + // startsWith(, format('…{0}', fromJSON('"\n"'))) => true + // startsWith(, format('…{0}', fromJSON('"\n"'))) => false + // startsWith(, format('…{0}', fromJSON('"\r"'))) => true + // Hence fromJSON (JSON *is* escape-processed) and both line endings: the + // REST API sends LF, the web UI sends CRLF. + const doc = parse(workflow); + const ifs = Object.entries(doc.jobs) + .filter(([, job]) => typeof job?.if === 'string') + .map(([id, job]) => [id, job.if]); + + it('never matches a command shape with a non-escaped literal newline', () => { + const broken = ifs.filter(([, cond]) => /'\\[nr]'/.test(cond)); + expect(broken.map(([id]) => id)).toEqual([]); + }); + + it('accepts both LF and CRLF after the command in every shape match', () => { + // `authorize` deliberately matches only a loose prefix — it is a filter to + // avoid spawning a job per comment, and delegates the exact shape to the + // downstream jobs. Jobs that do the shape match are the ones that use + // format('@qwen-code /{0}', …), so key off that. + const withShape = ifs.filter(([, cond]) => + cond.includes("format('@qwen-code /"), + ); + expect(withShape.length).toBeGreaterThan(0); + const missing = []; + for (const [id, cond] of withShape) { + for (const cmd of ['review', 'resolve']) { + // Only check commands this job actually matches on. + if (!cond.includes(`format('@qwen-code /${cmd}{0}'`)) continue; + const lf = cond.includes( + `format('@qwen-code /${cmd}{0}', fromJSON('"\\n"'))`, + ); + const cr = cond.includes( + `format('@qwen-code /${cmd}{0}', fromJSON('"\\r"'))`, + ); + if (!lf || !cr) missing.push(`${id}/${cmd} (LF:${lf} CR:${cr})`); + } + } + expect(missing).toEqual([]); + }); + + it('strips a trailing CR before parsing command tokens', () => { + // Word splitting uses IFS, which has no CR, so a CRLF comment would carry + // `\r` into tokens like `--timeout=300` and fail the numeric check. + // The command is parsed in "Resolve PR context", not in "Run review". + const run = parse(workflow).jobs['review-pr'].steps.find( + (s) => s.id === 'context', + ).run; + const firstLine = run.indexOf( + 'TRIGGER_COMMAND="${TRIGGER_BODY%%$\'\\n\'*}"', + ); + const stripCr = run.indexOf( + 'TRIGGER_COMMAND="${TRIGGER_COMMAND%$\'\\r\'}"', + ); + expect(firstLine).toBeGreaterThan(-1); + expect(stripCr).toBeGreaterThan(-1); + expect(stripCr).toBeGreaterThan(firstLine); + }); +});