From b5c717a5d5d356849264f00e583311bc6d866a09 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 2 Nov 2026 21:36:52 +0800 Subject: [PATCH] fix(verify-pr): address round-5 review findings Four Criticals: - The key entry works without a TTY: an interactive shell prompts, an agent's shell reads REVIEW_OPENAI_KEY_FILE and fails with a named error. - prepare disables fsmonitor, hooks and credential helpers on the PR-controlled clone, and the scan refuses credential-bearing remote URLs. - The proxy token is re-derived from proxy.json in the docker run step, because each step is its own shell. - The credential scan covers .env~, *.p12, id_rsa*, gha-creds-*, and the other credential families the repo's own .gitignore names. Suggestions: - Guard call site moved to , defined for watch mode. - The newest verify comment is snapshotted as previous-report.md. - The real key is unset once the proxy owns it; the token is unset at cleanup. - `{1..100}` replaces seq for macOS; --user and the proxy token's non-credential status are pinned; the scan abort is structural (`|| exit 1`). - Step 6 wording matches the base-pin; the docs narrow the base-pin scope for root AGENTS.md/QWEN.md, list the real CI divergence, explain content-encoding in English, fix the zh token wording, and record the checker --context contract and the ownership attribution. - The readiness gate compares the health nonce; --port is removed. Test gaps closed: gzip header assertion, mid-body disconnect, reset event, PUT, bare-base exact path, positive header passthrough, 502 transport failure, full paused-stream equality, token shape, publish --context pin, checker second positional, scan path fixtures, headers-only idle window. Tests: 159 passed; twelve mutations verified red. --- .qwen/skills/verify-pr/SKILL.md | 94 ++++++---- docs/design/verify-pr-local-model-proxy.md | 88 +++++---- .../verify-pr-local-model-proxy.zh-CN.md | 12 +- docs/design/verify-pr-local-round.md | 5 +- docs/design/verify-pr-local-round.zh-CN.md | 2 +- scripts/tests/verify-pr-check.test.js | 10 ++ .../tests/verify-pr-credential-scan.test.js | 61 ++++++- scripts/tests/verify-pr-model-proxy.test.js | 168 +++++++++++++++--- scripts/tests/verify-pr-prepare.test.js | 32 ++++ scripts/verify-pr-credential-scan.mjs | 49 ++++- scripts/verify-pr-model-proxy.mjs | 37 ++-- scripts/verify-pr-prepare.mjs | 44 +++-- 12 files changed, 466 insertions(+), 136 deletions(-) diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index a717b3647c7..cd45a1cb909 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -160,10 +160,11 @@ creates `tmp/pr-verify-/` with a `context.json` holding `repo`, `pr`, `baseRefOid` and `headRefOid`, and reports the isolation signals it can see. It does not gate on them — resolving metadata needs `gh`, which the boundary excludes, so this path always runs on the host. -The boundary check belongs inside: run -`node /trusted-scripts/verify-pr-prepare.mjs --require-isolation` (the trusted -copy mounted read-only) as the round's first -command there, and treat exit 4 as "this is not a boundary". A container +The boundary check belongs inside: run the trusted copy of the preflight with +`--require-isolation` as the round's first command there, and treat exit 4 as +"this is not a boundary". The Docker recipe below mounts that copy at +`/trusted-scripts/verify-pr-prepare.mjs`; a VM boundary runs the same file +from the trusted checkout instead. A container marker alone does not pass: the CLI's own Docker/Podman launcher mounts `~/.config/gcloud`, the ADC file and every model API key it can see, so the check also requires `QWEN_VERIFY_CREDENTIAL_FREE=1` — set it only after @@ -226,10 +227,10 @@ Two checkouts are involved, and they are not interchangeable: this tree as its cwd and refuses any other HEAD. Never execute a script from ``: the proxy, the prepare preflight, -the credential scan, the isolation gate and the checker all come from -``. A PR can rewrite a script in its own tree, and these steps -run with the real key, the maintainer's `gh` access, or the authority to -certify the round. +the credential scan, the isolation gate, the publish guard and the checker all +come from ``. A PR can rewrite a script in its own tree, and +these steps run with the real key, the maintainer's `gh` access, or the +authority to certify the round. 1. **Prepare from the trusted copy with the PR clone as its cwd**, then copy the full snapshot out of the tree and base-pin `.qwen`: @@ -238,6 +239,9 @@ certify the round. cd && node /scripts/verify-pr-prepare.mjs --repo / mkdir -p ~/.cache/qwen-verify/pr/qwen-base cp /tmp/verify-context/pr.json ~/.cache/qwen-verify/pr/pr.json + gh api "repos///issues//comments" --paginate --slurp \ + --jq 'flatten | map(select(.body | test("Verdict:"))) | last | .body // empty' \ + > ~/.cache/qwen-verify/pr/previous-report.md BASE_OID="$(node -e 'const fs=require("node:fs"); console.log(JSON.parse(fs.readFileSync(process.argv[1], "utf8")).baseRefOid)' ~/.cache/qwen-verify/pr/pr.json)" git -C archive "$BASE_OID" -- .qwen \ | tar -x -C ~/.cache/qwen-verify/pr/qwen-base @@ -247,30 +251,46 @@ certify the round. `cwd=` and still verifies that this tree is the PR head. The artifact directory's `context.json` carries only the four coordinates; the round reads the full gh snapshot (title, body, author, commits) from - `$QWEN_VERIFY_CONTEXT`. The host copy is also what pins the expected head - after the round, because PR code can rewrite anything inside ``. + `$QWEN_VERIFY_CONTEXT`, and the newest substantive verify comment lands + beside it as `previous-report.md` so a follow-up round keeps its + carried-forward findings. The host copy is also what pins the expected + head after the round, because PR code can rewrite anything inside + `` — including `.git/config`. After a round the clone is + PR-controlled: re-clone before another round instead of trusting it + again, and note that prepare's host-side git commands disable + `core.fsmonitor`, hooks and credential helpers. Mounting the base commit's `.qwen` over the tree (step 3) keeps the round's instructions and project settings out of PR control, as the CI - lane's base-pin does. + lane's base-pin does; the root `AGENTS.md`/`QWEN.md` are not covered and + still come from the PR clone. 2. **Start the proxy from the trusted checkout, in the background**, with the key read rather than typed onto the command line (a literal assignment - lands in shell history): + lands in shell history). An interactive shell may prompt; an agent's shell + has no TTY, so the key comes from `REVIEW_OPENAI_KEY_FILE` (default + `~/.config/qwen-verify/review-openai-key`, mode 600): ```bash - read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo + if [ -t 0 ]; then + read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo + else + IFS= read -r REVIEW_OPENAI_API_KEY \ + < "${REVIEW_OPENAI_KEY_FILE:-$HOME/.config/qwen-verify/review-openai-key}" || + { echo "REVIEW_OPENAI_API_KEY is unset and no key file was readable; set the file named by REVIEW_OPENAI_KEY_FILE" >&2; exit 1; } + fi export REVIEW_OPENAI_API_KEY REVIEW_OPENAI_BASE_URL= \ node /scripts/verify-pr-model-proxy.mjs \ --listen > ~/.cache/qwen-verify/pr/proxy.json 2>&1 & echo $! > ~/.cache/qwen-verify/pr/proxy.pid - for _ in $(seq 1 100); do + unset REVIEW_OPENAI_API_KEY + for _ in {1..100}; do node -e 'const j=JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")); if (!j.token) process.exit(1)' ~/.cache/qwen-verify/pr/proxy.json 2>/dev/null && break sleep 0.1 done chmod 600 ~/.cache/qwen-verify/pr/proxy.json - export QWEN_VERIFY_PROXY_TOKEN="$(node -e 'const fs=require("node:fs"); console.log(JSON.parse(fs.readFileSync(process.argv[1], "utf8")).token)' ~/.cache/qwen-verify/pr/proxy.json)" - test -n "$QWEN_VERIFY_PROXY_TOKEN" || { echo "proxy did not become ready"; exit 1; } + node -e 'const fs=require("node:fs"); const j=JSON.parse(fs.readFileSync(process.argv[1], "utf8")); const u=new URL(j.baseUrl); fetch(`http://${u.hostname}:${u.port}/__health`).then(r=>r.text()).then(t=>process.exit(t===j.nonce?0:1)).catch(()=>process.exit(1))' ~/.cache/qwen-verify/pr/proxy.json || + { echo "proxy health check failed"; exit 1; } ``` Bind the Docker bridge address, not `127.0.0.1` — a container cannot reach @@ -278,14 +298,18 @@ certify the round. to every container on the bridge. Keep the proxy running for the whole round and stop it at the end — it holds the real key. The background process truncates `proxy.json` before it prints, so the loop waits for a - complete line with a token, and the `test -n` catches a read that `export` - would otherwise hide. The token is read from the file into the environment, - never typed; Docker copies it with the bare `-e` below. + complete line with a token; the nonce is checked at the origin because the + printed `baseUrl` carries the upstream path prefix. The key is unset as + soon as the proxy owns its copy, so later diagnostics cannot leak it, and + the token stays in the file rather than this shell. 3. **Start the boundary as a named, detached container** so every later step - can be issued without a TTY: + can be issued without a TTY. Each step is its own shell: the token is + re-derived from `proxy.json` here, not inherited from step 2. ```bash + export QWEN_VERIFY_PROXY_TOKEN="$(node -e 'const fs=require("node:fs"); console.log(JSON.parse(fs.readFileSync(process.argv[1], "utf8")).token)' ~/.cache/qwen-verify/pr/proxy.json)" + test -n "$QWEN_VERIFY_PROXY_TOKEN" || { echo "proxy.json has no token" >&2; exit 1; } docker run -d --rm --name qwen-verify- \ --user "$(id -u):$(id -g)" -e HOME=/tmp/qwen-home \ -v :/work \ @@ -333,24 +357,27 @@ certify the round. ```bash docker exec -i qwen-verify- bash -lc ' - set -e - node /trusted-scripts/verify-pr-credential-scan.mjs /work + node /trusted-scripts/verify-pr-credential-scan.mjs /work || exit 1 export QWEN_VERIFY_CREDENTIAL_FREE=1 node /trusted-scripts/verify-pr-prepare.mjs --require-isolation ' ``` - The scan names credential-shaped ignored files (nested ones included), - leaves installed dependencies and build output alone, and refuses a nested - git repository outright: git does not descend into it, so a credential - file inside one would otherwise pass silently. Exit 4 means this is not a - boundary: stop and run nothing else. + The scan names credential-shaped ignored files (nested ones included) and + credential-bearing remote URLs, leaves installed dependencies and build + output alone, and refuses a nested git repository outright: git does not + descend into it, so a credential file inside one would otherwise pass + silently. The abort is structural (`|| exit 1`), not a side effect of + `set -e`. Exit 4 means this is not a boundary: stop and run nothing else. 6. **Run the round inside**, installing the CLI into the fresh home because the container is not root. Each `docker exec` is a new shell, so the attestation is passed to the shell that starts the round; it stays off the - `docker run` line. The round's engine is the released CLI; the PR clone - supplies the workspace skill and code: + `docker run` line. The round's engine is the released CLI, and the PR + clone supplies the code under test; the round's instructions come from the + base commit's `.qwen` mounted in step 3, not from the PR. Root + `AGENTS.md`/`QWEN.md` do come from the PR clone — when the diff touches + them, treat the round's context as PR-influenced: ```bash docker exec -i -e QWEN_VERIFY_CREDENTIAL_FREE=1 qwen-verify- bash -lc ' @@ -380,6 +407,7 @@ certify the round. docker rm -f qwen-verify- kill "$(cat ~/.cache/qwen-verify/pr/proxy.pid)" rm -f ~/.cache/qwen-verify/pr/proxy.json ~/.cache/qwen-verify/pr/proxy.pid + unset QWEN_VERIFY_PROXY_TOKEN ``` `` is `/tmp/pr-verify-`. Publishing still @@ -400,6 +428,9 @@ directly: the probe also reports comments and failed checks, and every non-empty gate output starts a turn whether or not the head moved. The wrapper forwards only `action: "resync"` (a moved head) and `action: "stop"`. +`` below is the trusted checkout the watch scripts run from; its main +worktree holds the probe cursor under `.qwen/tmp/`. + Build the task in this order and stop at the first failure: 1. **Resolve a binary that carries the probe.** The gate runs in a fresh shell, @@ -632,8 +663,9 @@ GitHub rewrites the six-character escape text for NUL and SOH (a backslash, `u`, then `0000` or `0001`) into a caret form, inside code spans and fences too. -**Run both halves of that guard through `scripts/verify-pr-guard.mjs`, because -the hand-run version fails open.** `--pr --repo / --snapshot +**Run both halves of that guard through +`/scripts/verify-pr-guard.mjs`, because the hand-run version +fails open.** `--pr --repo / --snapshot ` at round start and again before posting, then `--compare --start --now `; it exits 1 on anything dirty. A fetch that errors returns no items, so "is anything new?" answers "no" and a hand-rolled comparison reads clean — diff --git a/docs/design/verify-pr-local-model-proxy.md b/docs/design/verify-pr-local-model-proxy.md index c7ede22e2cc..ab7c4a51e9c 100644 --- a/docs/design/verify-pr-local-model-proxy.md +++ b/docs/design/verify-pr-local-model-proxy.md @@ -39,31 +39,38 @@ A host-side relay plus a recipe that names its trust boundaries. relays only the exact `${basePath}/chat/completions` path. Clients present a per-run bearer token (`QWEN_VERIFY_PROXY_TOKEN`); the proxy swaps it for the real key upstream. A nonce-echoing health endpoint distinguishes the process - from a squatter. Hop-by-hop request headers — both the fixed set and any - name a `Connection` header nominates — and the upstream `content-encoding` - are stripped. The idle watchdog is per-chunk, matches the - qwen client's own 240 s stream tolerance, stays quiet while the downstream is - paused, and re-arms on drain; a disconnect before upstream headers aborts - the fetch instead of paying for a completion nobody reads. + from a squatter, and the local recipe compares that nonce before it uses + the proxy. Hop-by-hop request headers — both the fixed set and any name a + `Connection` header nominates — and the upstream `content-encoding` are + stripped; undici has already decoded the body, so forwarding that header + would make the client decompress it twice. The idle watchdog is per-chunk, + matches the qwen client's own 240 s stream tolerance, stays quiet while the + downstream is paused, and re-arms on drain; a disconnect before upstream + headers aborts the fetch instead of paying for a completion nobody reads. - The recipe uses two checkouts. `` supplies every script the round runs, including the prepare preflight, the credential scan, the - isolation gate and the post-round checker; `` is a standalone - clone at the PR head that the container mounts and that prepare is given as - its cwd. A linked worktree is rejected by construction because its gitfile - points outside the mount. -- The container is named and detached (`sleep infinity` plus `docker exec`), - runs as the host uid with `HOME=/tmp/qwen-home`, mounts the base commit's - `.qwen` read-only over the tree so the round's instructions and project - settings cannot come from the PR, and mounts the full snapshot from a host - copy outside the tree. The per-run proxy token is exported from the proxy's - JSON file rather than typed, so it stays out of shell history. The - credential-free attestation is passed to the shell that starts the round, - after `verify-pr-credential-scan.mjs` (trusted copy) rejects - credential-shaped ignored files; that scan lists ignored files one by one + isolation gate, the publish guard and the post-round checker; `` + is a standalone clone at the PR head that the container mounts and that + prepare is given as its cwd. The clone's `.git` is PR-controlled after a + round, so it is not reused, and host-side git commands disable fsmonitor, + hooks and credential helpers. A linked worktree is rejected by + construction because its gitfile points outside the mount. +- The container is named and detached (`sleep infinity` plus `docker exec`) + and runs with `--user "$(id -u):$(id -g)"` and `HOME=/tmp/qwen-home`, so + files written into the mounted tree stay maintainer-owned; the CLI installs + into the fresh home because there is no writable global prefix. The base + commit's `.qwen` is mounted read-only over the tree, so the round's skill + and project settings cannot come from the PR; the root `AGENTS.md`/`QWEN.md` + are not covered and still come from the PR clone. The full snapshot is + mounted from a host copy outside the tree, and the per-run proxy token is + derived from the proxy's JSON file in the step that starts the container, so + it stays out of shell history. The credential-free attestation is passed to + the shell that starts the round, after `verify-pr-credential-scan.mjs` + (trusted copy) rejects credential-shaped ignored files and + credential-bearing remote URLs; that scan lists ignored files one by one and leaves `node_modules`/`dist` alone, so a built tree is not refused. The - CLI installs into the fresh home, so nothing in the tree becomes - root-owned; the round's engine is the released CLI unless the tree's own - build is installed over it. + round's engine is the released CLI unless the tree's own build is installed + over it. - The round reads `$QWEN_VERIFY_CONTEXT` from the full snapshot. After the round, the checker runs from `` with `--context `, so a writable artifact `context.json` cannot pin its own expected head. @@ -78,8 +85,12 @@ the real key. keep it out, and PR code can read its own environment. - **Reuse the CI heredoc by extracting it and calling it from the workflow.** Rejected for the CI lane: the script would come from the PR checkout. The - local script is therefore a deliberate second copy, and the divergence is - limited to the listen flag, the JSON coordinates, and header stripping. + local script is therefore a deliberate second copy. It diverges in more than + transport wiring: it matches the exact path instead of a suffix, strips + `Connection`-nominated headers and `content-encoding`, uses a 240 s + pause-aware idle window instead of the CI copy's fixed 120 s, and aborts a + pre-header disconnect. A sync between the copies must carry those + behavioural differences, not just the flags. - **Print `0.0.0.0` or normalise the listen host to loopback.** Rejected: the printed value must be the address clients actually dial. - **Mount the tree read-only.** Rejected as the only control: the round must @@ -95,21 +106,26 @@ the real key. stripped, pre-header disconnect abort, paused-downstream survival, slow-but-healthy streams, upstream reset survival, IPv6 listen formatting, and the CLI's JSON coordinates. -- Credential scan: a nested ignored `.env`, `*.pem`, `credentials*`, `.npmrc` - or `.netrc` is refused; installed `node_modules`/`dist` are ignored at any - depth, including inside a workspace package, and their contents are a - documented exclusion; enumeration is path-limited so a large ignored - dependency tree is never buffered; a nested git repository is refused - because git does not descend into it; a missing `--ignored` mode or a - dropped pattern turns its test red. +- Credential scan: a nested ignored `.env`, `.env~`, `*.pem`, `*.key`, + `*.p12`, `id_rsa*`, `credentials*`, `.git-credentials`, `.npmrc`, + `.netrc` or `gha-creds-*.json` is refused, as is a remote URL carrying a + credential; installed `node_modules`/`dist` are ignored at any depth, + including inside a workspace package, and their contents are a documented + exclusion; enumeration is path-limited so a large ignored dependency tree + is never buffered; a nested git repository is refused because git does not + descend into it; a missing `--ignored` mode or a dropped pattern turns its + test red. - Checker: `--context` overrides the writable artifact copy, refuses an unusable snapshot instead of falling back (with a usable writable copy seeded), and keeps its existing exit codes. -- Recipe: every script names the trusted checkout, the base commit's `.qwen` - is mounted read-only, both credentials come from files rather than command - lines, the proxy's coordinates are awaited before the token is exported, and - the attestation is on the round's shell but not the `docker run` line; - reverting any of those turns a recipe pin red. +- Recipe: every script names the trusted checkout (including prepare, the + scan, the gate, the guard and the checker), the base commit's `.qwen` is + mounted read-only, both credentials come from files rather than command + lines (the key falls back to `REVIEW_OPENAI_KEY_FILE` when stdin is not a + TTY, and is unset once the proxy owns it), the proxy's coordinates and + nonce are awaited before the container starts, the token is re-derived in + the `docker run` step, and the attestation is on the round's shell but not + the `docker run` line; reverting any of those turns a recipe pin red. - Mutation evidence: removing each of those guards turns its pin red. - A real Docker smoke covers the gate passing with `QWEN_VERIFY_PROXY_TOKEN`, the gate refusing `OPENAI_API_KEY`, and a container request reaching a mock diff --git a/docs/design/verify-pr-local-model-proxy.zh-CN.md b/docs/design/verify-pr-local-model-proxy.zh-CN.md index e4c6272b4ca..7ba892f7419 100644 --- a/docs/design/verify-pr-local-model-proxy.zh-CN.md +++ b/docs/design/verify-pr-local-model-proxy.zh-CN.md @@ -18,9 +18,9 @@ 一个宿主机侧转发器，加一套点明信任边界的配方。 -- `scripts/verify-pr-model-proxy.mjs` 监听选定的宿主地址（容器客户端用 Docker 网桥），持有 `REVIEW_OPENAI_API_KEY`，只转发精确的 `${basePath}/chat/completions` 路径。客户端出示一次性 bearer token（`QWEN_VERIFY_PROXY_TOKEN`），代理在上游替换为真实 key。回显 nonce 的健康端点用于区分本进程与抢占者。hop-by-hop 请求头（固定集合与 `Connection` 点名的头）与上游 `content-encoding` 被剥离（undici 已解码，转发该头会让客户端二次解压）。空闲看门狗按块续期，与 qwen 客户端自身的 240 秒流容忍一致；下游暂停期间不中止，drain 时续期；上游响应头之前客户端断开时中止 fetch，而不是为无人读取的补全付费。 -- 配方使用两个检出。`` 提供轮次运行的每个脚本，包括 prepare 预检、凭据扫描、隔离门禁与轮次后的检查器；`` 是停在 PR head 的独立克隆，供容器挂载，并作为 prepare 的 cwd。链接 worktree 因其 gitfile 指向挂载外而在构造上被排除。 -- 容器有名字且后台运行（`sleep infinity` 加 `docker exec`），以宿主 uid 运行、`HOME=/tmp/qwen-home`，把 base 提交的 `.qwen` 只读挂到树上（轮次指令与项目设置不能来自 PR），并从树外的宿主副本挂载完整快照。一次性代理 token 从代理的 JSON 文件导出而非键入，避免进入 shell history。无凭据宣誓传给启动轮次的 shell，且在可信的 `verify-pr-credential-scan.mjs` 拒绝凭据形状 ignored 文件之后才设置；该扫描逐文件列出 ignored 条目并放过 `node_modules`/`dist`，因此已构建的树不会被误拒。CLI 安装进这个新 home，因此树里不会出现 root 属主文件；轮次的引擎是发布版 CLI，除非把树内构建装到同一前缀覆盖它。 +- `scripts/verify-pr-model-proxy.mjs` 监听选定的宿主地址（容器客户端用 Docker 网桥），持有 `REVIEW_OPENAI_API_KEY`，只转发精确的 `${basePath}/chat/completions` 路径。客户端出示每轮一个、整轮有效的 bearer token（`QWEN_VERIFY_PROXY_TOKEN`，非单次使用），代理在上游替换为真实 key。回显 nonce 的健康端点用于区分本进程与抢占者，本地配方在使用代理前比较该 nonce。hop-by-hop 请求头（固定集合与 `Connection` 点名的头）与上游 `content-encoding` 被剥离（undici 已解码，转发该头会让客户端二次解压）。空闲看门狗按块续期，与 qwen 客户端自身的 240 秒流容忍一致；下游暂停期间不中止，drain 时续期；上游响应头之前客户端断开时中止 fetch，而不是为无人读取的补全付费。 +- 配方使用两个检出。`` 提供轮次运行的每个脚本，包括 prepare 预检、凭据扫描、隔离门禁、发布守卫与轮次后的检查器；`` 是停在 PR head 的独立克隆，供容器挂载，并作为 prepare 的 cwd。轮次结束后该克隆的 `.git` 属 PR 控制，不复用；宿主机侧 git 命令禁用 fsmonitor、hooks 与 credential helper。链接 worktree 因其 gitfile 指向挂载外而在构造上被排除。 +- 容器有名字且后台运行（`sleep infinity` 加 `docker exec`），以 `--user "$(id -u):$(id -g)"` 与 `HOME=/tmp/qwen-home` 运行，因此写入挂载树的文件保持维护者所有；CLI 装进新 home 是因为容器没有可写的全局前缀。base 提交的 `.qwen` 只读挂到树上（轮次的 skill 与项目设置不能来自 PR）；根 `AGENTS.md`/`QWEN.md` 不在覆盖范围内，仍来自 PR 克隆。完整快照从树外宿主副本挂载，代理 token 在启动容器的那一步从 JSON 文件重新取得，避免进入 shell history。无凭据宣誓传给启动轮次的 shell，且在可信的 `verify-pr-credential-scan.mjs` 拒绝凭据形状 ignored 文件与带凭据的 remote URL 之后才设置；该扫描逐文件列出 ignored 条目并放过 `node_modules`/`dist`，因此已构建的树不会被误拒。轮次的引擎是发布版 CLI，除非把树内构建装到同一前缀覆盖它。 - 轮次从完整快照读取 `$QWEN_VERIFY_CONTEXT`。轮次结束后，检查器从 `` 运行并带 `--context <宿主副本>`，因此可写的产物 `context.json` 无法自证其期望 head。 CI 车道保留内联代理：那份副本运行在 base-pinned workflow 的 root 步骤里，若改为指向 PR 检出，PR 就能读到真实 key。 @@ -28,15 +28,15 @@ CI 车道保留内联代理：那份副本运行在 base-pinned workflow 的 roo ## 备选方案 - **把真实 key 导出进容器。** 否决：边界的存在就是为了把它挡在外面，而 PR 代码能读取自己的环境。 -- **抽取 CI heredoc 并由 workflow 调用。** 对 CI 车道否决：脚本会来自 PR 检出。因此本地脚本是有意的第二份副本，差异仅限监听参数、JSON 坐标与请求头剥离。 +- **抽取 CI heredoc 并由 workflow 调用。** 对 CI 车道否决：脚本会来自 PR 检出。因此本地脚本是有意的第二份副本。它的差异不止传输接线：精确路径而非后缀、剥离 `Connection` 点名的头与 `content-encoding`、240 秒 pause 感知空闲窗口而非 CI 的固定 120 秒、以及响应头前断连中止。两份同步时必须带上这些行为差异，而不只是参数。 - **打印 `0.0.0.0`，或把监听地址归一化成 loopback。** 否决：打印值必须是客户端实际拨号的地址。 - **仅把树挂成只读。** 否决：轮次必须在树里构建并写产物。信任边界靠「特权宿主步骤从 `` 运行」以及「从宿主快照钉住期望 head」来落实。 ## 验收 - 代理：bearer 拒绝、精确路径白名单、base path 前缀、真实 key 注入、chunked 请求体下 hop-by-hop 剥离（固定集合与 `Connection` 点名的头）、gzip 上游响应（头被剥离）、响应头前断连中止、下游暂停存活、慢但健康的流、上游重置存活、IPv6 监听格式化，以及 CLI 的 JSON 坐标。 -- 凭据扫描：嵌套 ignored 的 `.env`、`*.pem`、`credentials*`、`.npmrc`、`.netrc` 被拒绝；`node_modules`/`dist` 在任意深度（含 workspace 包内）被忽略，其内容属文档化取舍；枚举由 pathspec 限制，不会先缓冲整棵依赖树；嵌套 git 仓库被直接拒绝（git 不会进入其中）；去掉 `--ignored` 模式或删掉某条模式会让测试变红。 +- 凭据扫描：嵌套 ignored 的 `.env`、`.env~`、`*.pem`、`*.key`、`*.p12`、`id_rsa*`、`credentials*`、`.git-credentials`、`.npmrc`、`.netrc`、`gha-creds-*.json` 被拒绝，带凭据的 remote URL 同样被拒绝；`node_modules`/`dist` 在任意深度（含 workspace 包内）被忽略，其内容属文档化取舍；枚举由 pathspec 限制，不会先缓冲整棵依赖树；嵌套 git 仓库被直接拒绝（git 不会进入其中）；去掉 `--ignored` 模式或删掉某条模式会让测试变红。 - 检查器：`--context` 覆盖可写产物副本、不可用快照时拒绝而不是回退（产物副本先种一个合法 head），并保持既有退出码。 -- 配方：每个脚本都点名可信 checkout、base 提交的 `.qwen` 只读挂载、两个凭据都来自文件而非命令行、导出 token 前等待代理坐标就绪、宣誓在轮次 shell 但不在 `docker run` 行；任一被还原都会让配方 pin 变红。 +- 配方：每个脚本都点名可信 checkout（含 prepare、扫描、门禁、守卫与检查器）、base 提交的 `.qwen` 只读挂载、两个凭据都来自文件而非命令行（stdin 非 TTY 时 key 从 `REVIEW_OPENAI_KEY_FILE` 读取，代理拿到副本后立即 unset）、启动容器前等待代理坐标与 nonce、token 在 `docker run` 那一步重新取得、宣誓在轮次 shell 但不在 `docker run` 行；任一被还原都会让配方 pin 变红。 - 变异证据：移除上述任一守卫，对应 pin 必须变红。 - 真实 Docker 冒烟：`QWEN_VERIFY_PROXY_TOKEN` 时门禁通过、`OPENAI_API_KEY` 时门禁拒绝、容器请求到达 mock 上游且真实 key 已注入。 diff --git a/docs/design/verify-pr-local-round.md b/docs/design/verify-pr-local-round.md index 9fa2de664b5..29cd4b766f1 100644 --- a/docs/design/verify-pr-local-round.md +++ b/docs/design/verify-pr-local-round.md @@ -71,7 +71,10 @@ them rather than at the end of the document. self-consistent assertion triple, and no `merge-ready` carrying a failed assertion. Report shape and evidence naming are warnings, because a documentation-only round has no capture and a checker cannot know whether a - harness ran. + harness ran. With `--context ` the expected head comes from that + host-held file instead of the artifact's writable `context.json`, and an + unusable snapshot is a hard exit 1, not a warning: the artifact directory is + writable by the PR code the round just ran. - `scripts/verify-pr-guard.mjs` owns both halves of the pre-publish guard. `--pr --repo / --snapshot ` fetches the head, the PR state and all three comment lists with `--paginate`, and writes the file only diff --git a/docs/design/verify-pr-local-round.zh-CN.md b/docs/design/verify-pr-local-round.zh-CN.md index 238eb8707a7..e6e33f667a6 100644 --- a/docs/design/verify-pr-local-round.zh-CN.md +++ b/docs/design/verify-pr-local-round.zh-CN.md @@ -25,7 +25,7 @@ 新增三个独立脚本，并让 skill 在流程需要的位置指向它们，而不是放在文末。 - `scripts/verify-pr-prepare.mjs --repo /` 负责解析坐标：用 lane 相同的字段列表抓取快照、校验两个 OID 在本克隆中存在、拒绝不是 PR head 干净检出的树、创建带 `evidence/` 子目录的产物目录，并报告当前进程能观测到的隔离信号。它不据此设闸：解析元数据需要 `gh`，而 `gh` 与边界互斥，因此这条路径总是在宿主上运行。闸门是 `--require-isolation`，用于在边界内、轮次执行任何东西之前运行。 -- `scripts/verify-pr-check.mjs ` 把发布侧的规则套到本地产物目录：必需文件、四个判定词、为正且自洽的断言三元组、以及不允许带失败断言的 `merge-ready`。报告形态与证据命名只作为警告，因为纯文档轮次没有截图，检查器也无法知道是否跑过 harness。 +- `scripts/verify-pr-check.mjs ` 把发布侧的规则套到本地产物目录：必需文件、四个判定词、为正且自洽的断言三元组、以及不允许带失败断言的 `merge-ready`。报告形态与证据命名只作为警告，因为纯文档轮次没有截图，检查器也无法知道是否跑过 harness。带 `--context ` 时期望 head 来自该宿主持有的文件，而不是产物里可写的 `context.json`；不可用快照是硬性 exit 1 而非警告：产物目录可被轮次刚执行的 PR 代码写入。 - `scripts/verify-pr-guard.mjs` 接管发布前守卫的两个半程。`--pr --repo / --snapshot ` 抓取 head、PR 状态以及三个评论列表（带 `--paginate`），只有每次调用都成功才写文件。`--compare --start --now ` 列出本轮 dirty 的全部原因并退出 1。 检查器在产物契约要求轮次写出的产物上新增三条规则。产物契约里三者都要点名：产物清单没要求的文件，轮次就不会产出，而对一个不存在的文件做检查是静默的。 diff --git a/scripts/tests/verify-pr-check.test.js b/scripts/tests/verify-pr-check.test.js index 0676391484d..79ee22e55e6 100644 --- a/scripts/tests/verify-pr-check.test.js +++ b/scripts/tests/verify-pr-check.test.js @@ -825,5 +825,15 @@ describe.skipIf(process.platform === 'win32')('check (CLI)', () => { encoding: 'utf8', }).status, ).toBe(2); + // A forgotten --context turns the snapshot into a second positional: + // refuse instead of silently using the artifact's writable copy. + const hostSnapshot = join(tmpdir(), `verify-pr-host-${Date.now()}.json`); + tmpDirs.push(hostSnapshot); + writeFileSync(hostSnapshot, JSON.stringify({ headRefOid: HEAD })); + expect( + spawnSync(process.execPath, [SCRIPT, dir, hostSnapshot], { + encoding: 'utf8', + }).status, + ).toBe(2); }); }); diff --git a/scripts/tests/verify-pr-credential-scan.test.js b/scripts/tests/verify-pr-credential-scan.test.js index 14c46a5a142..9a01034188a 100644 --- a/scripts/tests/verify-pr-credential-scan.test.js +++ b/scripts/tests/verify-pr-credential-scan.test.js @@ -58,10 +58,15 @@ describe('credential basename matching', () => { '.env.local', 'app.env', 'secrets/prod.env', + '.env~', 'tls.pem', 'id.key', + 'auth.p12', + 'id_rsa', 'credentials', 'credentials.json', + '.git-credentials', + 'gha-creds-123.json', '.npmrc', '.netrc', ]) { @@ -84,6 +89,11 @@ describe('gitScanArgs', () => { const args = gitScanArgs('/repo'); expect(args).toContain(':(glob)**/.env'); expect(args).toContain(':(glob)**/*.pem'); + expect(args).toContain(':(glob)**/*.env~'); + expect(args).toContain(':(glob)**/*.p12'); + expect(args).toContain(':(glob)**/id_rsa*'); + expect(args).toContain(':(glob)**/*credentials*'); + expect(args).toContain(':(glob)**/gha-creds-*.json'); // Excludes apply at workspace depth, not only the repository root. expect(args).toContain(':(exclude,glob)**/node_modules/**'); expect(args).toContain(':(exclude,glob)**/dist/**'); @@ -93,27 +103,72 @@ describe('gitScanArgs', () => { describe('scanIgnoredCredentials', () => { it('reports nested and top-level ignored credential files', () => { const dir = makeRepo( - ['vault/.env', 'secrets/prod.env', 'tls.pem', '.npmrc', '.netrc'], [ - '.env', + 'vault/.env', + 'secrets/prod.env', + '.env~', + '.env.local', + 'tls.pem', + 'secrets/id.key', + 'auth.p12', + 'id_rsa', + 'secrets/credentials.json', + '.git-credentials', + 'gha-creds-123.json', + '.npmrc', + '.netrc', + ], + [ + '.env*', '*.env', '*.pem', '*.key', - 'credentials*', + '*.p12', + 'id_rsa*', + '*credentials*', '.npmrc', '.netrc', + 'gha-creds-*.json', 'vault/', ], ); expect(scanIgnoredCredentials(dir)).toEqual([ + '.env.local', + '.env~', + '.git-credentials', '.netrc', '.npmrc', + 'auth.p12', + 'gha-creds-123.json', + 'id_rsa', + 'secrets/credentials.json', + 'secrets/id.key', 'secrets/prod.env', 'tls.pem', 'vault/.env', ]); }); + it('refuses a remote URL that carries a credential', () => { + const dir = makeRepo(['src/app.ts'], ['node_modules/']); + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + 'https://x-access-token:ghp_secret@github.com/o/r.git', + ], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /credential-bearing remote URL/, + ); + const result = run(dir); + expect(result.status).toBe(1); + expect(result.stderr).toContain('credential-bearing remote URL'); + }); + it('does not refuse an installed tree or build output at any depth', () => { const dir = makeRepo( [ diff --git a/scripts/tests/verify-pr-model-proxy.test.js b/scripts/tests/verify-pr-model-proxy.test.js index 668db671e37..f8056e9b329 100644 --- a/scripts/tests/verify-pr-model-proxy.test.js +++ b/scripts/tests/verify-pr-model-proxy.test.js @@ -71,9 +71,9 @@ async function startUpstream({ delayMs = 0, chunkDelayMs = 0, resetAfterFirstChunk = false, - resetDelayMs = 50, respectBackpressure = false, gzipBody = false, + headersOnly = false, } = {}) { const requests = []; const events = []; @@ -106,11 +106,18 @@ async function startUpstream({ ), }); events.push('write-head'); + requests.at(-1).sentContentEncoding = 'gzip'; res.end(gzipSync(Buffer.from(chunks.join('')))); return; } res.writeHead(status, { 'content-type': 'text/event-stream' }); events.push('write-head'); + if (headersOnly) { + // Flush so the proxy's fetch resolves on headers; the test is the + // first-body idle window, not the pre-header one. + res.flushHeaders(); + return; + } for (const [index, chunk] of chunks.entries()) { if (chunkDelayMs) await sleep(chunkDelayMs); if (res.destroyed) { @@ -124,9 +131,11 @@ async function startUpstream({ await once(res, 'drain'); } if (resetAfterFirstChunk && index === 0) { - await sleep(resetDelayMs); + // Register on the socket, not unconditionally: the test asserts + // the upstream really reset after headers. + res.on('close', () => events.push('upstream-reset')); + await sleep(50); res.destroy(); - events.push('upstream-reset'); return; } } @@ -137,9 +146,9 @@ async function startUpstream({ return { port, requests, events }; } -async function startProxy(upstream, overrides = {}) { +async function startProxy(upstream, overrides = {}, basePath = '/v1') { const server = createModelProxy({ - baseUrl: `http://127.0.0.1:${upstream.port}/v1`, + baseUrl: `http://127.0.0.1:${upstream.port}${basePath}`, apiKey: REAL_KEY, token: TOKEN, nonce: NONCE, @@ -195,6 +204,13 @@ describe('verify-pr-model-proxy', () => { // /v1/admin/chat/completions. const admin = await fetch(`${proxy}/admin/chat/completions`, chatRequest()); expect(admin.status).toBe(403); + // The method half matters on its own: a PUT to the allowed path would + // otherwise spend the real key upstream. + const put = await fetch(`${proxy}/chat/completions`, { + ...chatRequest(), + method: 'PUT', + }); + expect(put.status).toBe(403); expect(upstream.requests).toHaveLength(0); }); @@ -226,6 +242,17 @@ describe('verify-pr-model-proxy', () => { expect(upstream.requests[0].url).toBe('/v1/chat/completions'); }); + it('matches the exact path even without a base-path prefix', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream, {}, ''); + const admin = await fetch(`${proxy}/admin/chat/completions`, chatRequest()); + expect(admin.status).toBe(403); + expect(upstream.requests).toHaveLength(0); + const ok = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(ok.status).toBe(200); + expect(upstream.requests[0].url).toBe('/chat/completions'); + }); + it('strips the upstream content-encoding instead of forwarding it', async () => { const upstream = await startUpstream({ gzipBody: true, @@ -234,6 +261,10 @@ describe('verify-pr-model-proxy', () => { const proxy = await startProxy(upstream); const res = await fetch(`${proxy}/chat/completions`, chatRequest()); expect(res.status).toBe(200); + // The fixture must really have compressed; otherwise this test would + // pass against an uncompressed upstream and pin nothing. + expect(upstream.requests[0].sentContentEncoding).toBe('gzip'); + expect(res.headers.get('content-type')).toBe('text/event-stream'); // undici already decoded the body; forwarding the header would make the // client gunzip plain text. expect(res.headers.get('content-encoding')).toBeNull(); @@ -278,6 +309,9 @@ describe('verify-pr-model-proxy', () => { expect(seen.headers.expect).toBeUndefined(); expect(seen.headers['x-client-hop']).toBeUndefined(); expect(seen.headers.connection).not.toBe('x-client-hop'); + // Passthrough in the positive direction: the relay must keep the + // request's content type, not only drop the hop-by-hop names. + expect(seen.headers['content-type']).toBe('application/json'); }); it('propagates an upstream error status and body', async () => { @@ -301,6 +335,28 @@ describe('verify-pr-model-proxy', () => { expect(res.status).toBe(504); }); + it('answers 502 and stays alive when the upstream is unreachable', async () => { + const proxy = await startProxy({ port: 9 }, {}, ''); + // Point at a closed port: port 9 is the discard service and is not + // listening in this environment, so fetch rejects with ECONNREFUSED. + const res = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(res.status).toBe(502); + const health = await fetch(`${proxy}/__health`); + expect(health.status).toBe(200); + }); + + it('aborts when headers arrive but no body follows', async () => { + const upstream = await startUpstream({ headersOnly: true }); + const proxy = await startProxy(upstream, { idleTimeoutMs: 100 }); + // The upstream flushes headers, then stalls: the first-body idle window + // must tear the stream down instead of letting it hang. + await expect( + fetch(`${proxy}/chat/completions`, chatRequest()), + ).rejects.toThrow(); + const health = await fetch(`${proxy}/__health`); + expect(health.status).toBe(200); + }); + it('refuses to start without the full configuration', () => { expect(() => createModelProxy({ @@ -339,6 +395,35 @@ describe('verify-pr-model-proxy', () => { expect(upstream.events).not.toContain('write-head'); }); + it('aborts the upstream when the client disconnects after headers', async () => { + const upstream = await startUpstream({ + chunks: ['data: first\n\n', 'data: second\n\n', 'data: third\n\n'], + chunkDelayMs: 100, + }); + const proxy = await startProxy(upstream, { idleTimeoutMs: 5000 }); + const { port } = new URL(proxy); + const req = http.request( + { + host: '127.0.0.1', + port, + path: '/chat/completions', + method: 'POST', + headers: { authorization: `Bearer ${TOKEN}` }, + }, + (res) => { + res.once('data', () => { + res.destroy(); + req.destroy(); + }); + res.on('error', () => {}); + }, + ); + req.on('error', () => {}); + req.end('{}'); + await waitFor(() => upstream.events.includes('aborted-mid-body'), 3000); + expect(upstream.events).toContain('aborted-mid-body'); + }); + it('does not abort a healthy stream while the downstream is paused', async () => { const chunks = Array.from( { length: 128 }, @@ -374,7 +459,7 @@ describe('verify-pr-model-proxy', () => { req.on('error', reject); req.end('{}'); }); - expect(body).toContain('TAIL-MARKER'); + expect(body).toBe(chunks.join('')); }); it('keeps a slow-but-healthy stream alive past the window', async () => { @@ -422,14 +507,19 @@ describe('verify-pr-model-proxy', () => { ); }); const info = JSON.parse(line); - await fetch(`http://127.0.0.1:${info.port}/chat/completions`, { + // The CLI-generated token is the container's only credential. + expect(info.token).toMatch(/^[0-9a-f]{48}$/); + const res = await fetch(`http://127.0.0.1:${info.port}/chat/completions`, { method: 'POST', headers: { authorization: `Bearer ${info.token}` }, body: '{}', - }).catch(() => {}); + }).catch(() => null); + if (res) await res.text().catch(() => {}); + await waitFor(() => upstream.events.includes('upstream-reset'), 2000); await sleep(200); // The client may see a torn stream; the proxy process must not die on it. expect(child.exitCode).toBeNull(); + expect(upstream.events).toContain('upstream-reset'); const health = await fetch(`http://127.0.0.1:${info.port}/__health`); expect(health.status).toBe(200); }); @@ -465,22 +555,22 @@ describe('proxy defaults and base url formatting', () => { describe('parseArgs', () => { it('defaults to loopback with an ephemeral port', () => { - expect(parseArgs([])).toEqual({ listen: '127.0.0.1', port: 0 }); + expect(parseArgs([])).toEqual({ listen: '127.0.0.1' }); }); - it('parses --listen and --port', () => { - expect(parseArgs(['--listen', '172.17.0.1', '--port', '8080'])).toEqual({ + it('parses --listen', () => { + expect(parseArgs(['--listen', '172.17.0.1'])).toEqual({ listen: '172.17.0.1', - port: 8080, }); }); - it('rejects unknown arguments, missing values and bad ports', () => { + it('rejects --port, which the recipe does not use', () => { + expect(() => parseArgs(['--port', '8080'])).toThrow('unknown argument'); + }); + + it('rejects unknown arguments and missing values', () => { expect(() => parseArgs(['--docker'])).toThrow('unknown argument'); expect(() => parseArgs(['--listen'])).toThrow('requires a value'); - expect(() => parseArgs(['--port', 'abc'])).toThrow('--port'); - expect(() => parseArgs(['--port', '70000'])).toThrow('--port'); - expect(() => parseArgs(['--port', '-1'])).toThrow('--port'); }); }); @@ -559,6 +649,10 @@ describe('local container recipe pins', () => { expect(skill).toContain( 'node /scripts/verify-pr-model-proxy.mjs', ); + expect(skill).toContain('/scripts/verify-pr-guard.mjs'); + expect(skill).toContain( + '`` below is the trusted checkout the watch scripts run from', + ); // Enumerate the checker call sites: the preamble also names the trusted // path, so a single positive assertion cannot detect step 7 reverting to // /scripts. @@ -568,38 +662,54 @@ describe('local container recipe pins', () => { expect(checkerCalls.length).toBeGreaterThanOrEqual(3); for (const prefix of checkerCalls) expect(prefix).toBe(''); expect(skill).not.toMatch( - /node \/scripts\/verify-pr-(prepare|check|model-proxy)/, + /node \/scripts\/verify-pr-(prepare|check|model-proxy|guard)/, ); expect(skill).not.toMatch( - /node scripts\/verify-pr-(prepare|check|model-proxy)\.mjs/, + /node scripts\/verify-pr-(prepare|check|model-proxy|guard)\.mjs/, ); }); - it('base-pins the round instructions and mounts them read-only', () => { + it('base-pins `.qwen` and states that root AGENTS/QWEN.md still come from the PR', () => { expect(skill).toContain('git -C archive "$BASE_OID" -- .qwen'); expect(skill).toContain( '-v ~/.cache/qwen-verify/pr/qwen-base/.qwen:/work/.qwen:ro', ); + expect(skill).toContain('`AGENTS.md`/`QWEN.md` do come from the PR clone'); }); it('mounts the round tree writable and the metadata snapshot read-only', () => { + expect(skill).toContain('--user "$(id -u):$(id -g)"'); expect(skill).toContain('-v :/work'); expect(skill).toContain('-v /scripts:/trusted-scripts:ro'); expect(skill).toContain('-v ~/.cache/qwen-verify/pr:/verify-context:ro'); expect(skill).toContain('QWEN_VERIFY_CONTEXT=/verify-context/pr.json'); }); - it('reads both credentials instead of putting them on a command line', () => { - expect(skill).toContain("read -rsp 'REVIEW_OPENAI_API_KEY: '"); + it('reads both credentials from files, including without a TTY', () => { + expect(skill).toContain('if [ -t 0 ]; then'); + expect(skill).toContain('REVIEW_OPENAI_KEY_FILE'); expect(skill).not.toContain('REVIEW_OPENAI_API_KEY='); + // The proxy owns its copy: later diagnostics must not see the key. + expect(skill).toContain('unset REVIEW_OPENAI_API_KEY'); expect(skill).toContain('QWEN_VERIFY_PROXY_TOKEN="$(node -e'); expect(skill).toContain('chmod 600'); // The background process truncates proxy.json before printing; the loop // and the non-empty check keep an empty read from reaching Docker. - expect(skill).toContain('for _ in $(seq 1 100); do'); + expect(skill).toContain('for _ in {1..100}; do'); + expect(skill).not.toContain('$(seq 1 100)'); expect(skill).toContain('test -n "$QWEN_VERIFY_PROXY_TOKEN"'); + // The nonce, not just the JSON shape, is checked before use. + expect(skill).toContain('/__health'); + expect(skill).toContain('fetch(`http://${u.hostname}:${u.port}/__health`)'); + expect(skill).toContain('process.exit(t===j.nonce?0:1)'); + // Each step is its own shell: the token is re-derived where it is used. + const dockerRun = + skill.match(/export QWEN_VERIFY_PROXY_TOKEN[\s\S]*?```/)?.[0] ?? ''; + expect(dockerRun).toContain('proxy.json'); + expect(dockerRun).toContain('test -n "$QWEN_VERIFY_PROXY_TOKEN"'); expect(skill).toMatch(/-e QWEN_VERIFY_PROXY_TOKEN[ \t]*\\?\n/); expect(skill).not.toContain('-e QWEN_VERIFY_PROXY_TOKEN='); + expect(skill).not.toContain('--port'); }); it('keeps the attestation off the docker run line and on the round shell', () => { @@ -613,7 +723,10 @@ describe('local container recipe pins', () => { ); const roundBlock = skill.split('qwen --auth-type openai')[0].split('```'); expect(roundBlock.at(-1)).toContain('QWEN_VERIFY_CREDENTIAL_FREE'); - expect(skill).toContain('verify-pr-credential-scan.mjs /work'); + // The credential hit aborts structurally, not via `set -e`. + expect(skill).toContain( + 'node /trusted-scripts/verify-pr-credential-scan.mjs /work || exit 1', + ); }); it('names the released CLI as the engine and gives the tree-build route', () => { @@ -626,7 +739,12 @@ describe('local container recipe pins', () => { expect(skill).toMatch(/nested\s+git repository outright/); }); - it('pins the checker head from the host snapshot', () => { - expect(skill).toContain('--context ~/.cache/qwen-verify/pr/pr.json'); + it('snapshots the previous report and pins both checker call sites', () => { + expect(skill).toContain('previous-report.md'); + expect(skill).toContain('issues//comments'); + const contextCalls = [ + ...skill.matchAll(/verify-pr-check\.mjs --context /g), + ]; + expect(contextCalls.length).toBeGreaterThanOrEqual(2); }); }); diff --git a/scripts/tests/verify-pr-prepare.test.js b/scripts/tests/verify-pr-prepare.test.js index 5318828e651..34682221e0d 100644 --- a/scripts/tests/verify-pr-prepare.test.js +++ b/scripts/tests/verify-pr-prepare.test.js @@ -20,9 +20,11 @@ import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterEach, describe, expect, it } from 'vitest'; import { + GIT_HARDENING, artifactDirName, detectCredentialSignals, detectIsolation, + gitStatusArgs, parseArgs, parseRepo, validateContext, @@ -213,6 +215,36 @@ describe('detectCredentialSignals', () => { '/home/t', ), ).toEqual(['SANDBOX_MOUNTS']); + // The recipe's proxy credential must stay outside the signal set, or a + // container round would fail the gate before it starts. + expect( + detectCredentialSignals( + { QWEN_VERIFY_PROXY_TOKEN: 'x' }, + () => false, + '/home/t', + ), + ).toEqual([]); + }); +}); + +describe('git hardening', () => { + it('disables fsmonitor, hooks and credential helpers on every git read', () => { + expect(GIT_HARDENING).toEqual([ + '-c', + 'core.fsmonitor=false', + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'credential.helper=', + ]); + expect(gitStatusArgs('/clone')).toEqual([ + ...GIT_HARDENING, + '-C', + '/clone', + 'status', + '--porcelain', + '--untracked-files=normal', + ]); }); }); diff --git a/scripts/verify-pr-credential-scan.mjs b/scripts/verify-pr-credential-scan.mjs index 2cc3626f562..3c185a727f0 100644 --- a/scripts/verify-pr-credential-scan.mjs +++ b/scripts/verify-pr-credential-scan.mjs @@ -38,20 +38,27 @@ import { basename } from 'node:path'; import { isEntryPoint } from './lib/entry-point.js'; const CREDENTIAL_BASENAME = - /^(?:\.env(?:\..*)?|.*\.env|.*\.pem|.*\.key|credentials.*|\.npmrc|\.netrc)$/; + /^(?:\.env.*|.*\.env|.*\.env~|.*\.pem|.*\.key|.*\.p12|.*\.pfx|id_rsa.*|id_ed25519.*|.*credentials.*|\.git-credentials|\.npmrc|\.netrc|gha-creds-.*\.json)$/; // Git applies these pathspecs while enumerating, before any output is // buffered: a full ignored listing of an installed checkout is several MB and // used to overflow spawnSync's buffer before the JS filter could run. const CREDENTIAL_PATHSPECS = [ ':(glob)**/.env', - ':(glob)**/.env.*', + ':(glob)**/.env*', ':(glob)**/*.env', + ':(glob)**/*.env~', ':(glob)**/*.pem', ':(glob)**/*.key', - ':(glob)**/credentials*', + ':(glob)**/*.p12', + ':(glob)**/*.pfx', + ':(glob)**/id_rsa*', + ':(glob)**/id_ed25519*', + ':(glob)**/*credentials*', + ':(glob)**/.git-credentials', ':(glob)**/.npmrc', ':(glob)**/.netrc', + ':(glob)**/gha-creds-*.json', ':(exclude,glob)**/node_modules/**', ':(exclude,glob)**/dist/**', ]; @@ -110,6 +117,36 @@ export function nestedGitRepos(root, { maxBuffer = 8 * 1024 * 1024 } = {}) { return result.stdout.split('\0').filter(Boolean).sort(); } +/** + * Returns local remote names whose URL embeds a credential. A clone made with + * `https://x-access-token:@…` keeps the PAT in `.git/config`, where no + * ignored-file listing reaches it and PR code can read it with `git config`. + */ +export function credentialBearingRemotes( + root, + { maxBuffer = 8 * 1024 * 1024 } = {}, +) { + const result = spawnSync( + 'git', + ['-C', root, 'config', '--local', '--get-regexp', '^remote\\..*\\.url$'], + { encoding: 'utf8', maxBuffer }, + ); + // Exit 1 means the pattern matched nothing; anything else is a real failure. + if (result.status === 1) return []; + if (result.status !== 0) { + throw new Error( + `could not read local remotes: ${(result.stderr || '').trim() || 'git failed'}`, + ); + } + const hits = []; + for (const line of result.stdout.split('\n')) { + if (!line) continue; + const [key, ...rest] = line.split(' '); + if (/\/\/[^/@\s]+:[^/@\s]+@/.test(rest.join(' '))) hits.push(key); + } + return hits; +} + /** * Returns the ignored credential-shaped files under `root`, sorted. An empty * array is the clean state; a git failure is thrown so the caller can refuse @@ -126,6 +163,12 @@ export function scanIgnoredCredentials( `nested git repository under ${nested[0]} — scan that clone separately or remove it`, ); } + const remotes = credentialBearingRemotes(root, { maxBuffer }); + if (remotes.length > 0) { + throw new Error( + `credential-bearing remote URL in ${remotes[0]} — remove the token from .git/config`, + ); + } const result = spawnSync('git', gitScanArgs(root), { encoding: 'utf8', maxBuffer, diff --git a/scripts/verify-pr-model-proxy.mjs b/scripts/verify-pr-model-proxy.mjs index aa94faaf18e..81e21050cdb 100644 --- a/scripts/verify-pr-model-proxy.mjs +++ b/scripts/verify-pr-model-proxy.mjs @@ -26,19 +26,26 @@ * * Usage (from a trusted checkout, never the PR-head tree — this process holds * the real key and a PR can rewrite a script in its own checkout): - * read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo + * if [ -t 0 ]; then + * read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo + * else + * IFS= read -r REVIEW_OPENAI_API_KEY \ + * < ~/.config/qwen-verify/review-openai-key + * fi * export REVIEW_OPENAI_API_KEY * REVIEW_OPENAI_BASE_URL= \ * node /scripts/verify-pr-model-proxy.mjs \ - * [--listen ] [--port ] + * [--listen ] * - * Reading the key instead of typing it as an inline assignment keeps it out - * of shell history, `set -x` traces, and terminal transcripts. + * Reading the key from a terminal when one exists and from a file otherwise + * keeps it out of shell history (an inline assignment is recorded) and keeps + * an agent-driven run, whose shell has no TTY, from blocking on `read`. * * Options: * --listen address to bind (default 127.0.0.1; use the Docker - * bridge address, e.g. 172.17.0.1, for container clients) - * --port port to bind (default 0 = ephemeral) + * bridge address, e.g. 172.17.0.1, for container clients). + * The port is always ephemeral: a fixed port is a binding + * race the CI lane avoids too. * * Environment: * REVIEW_OPENAI_BASE_URL upstream base URL, may carry a path prefix @@ -82,23 +89,15 @@ const STRIPPED_REQUEST_HEADERS = [ ]; export function parseArgs(argv) { - const options = { listen: '127.0.0.1', port: 0 }; + const options = { listen: '127.0.0.1' }; for (let i = 0; i < argv.length; i++) { const arg = argv[i]; - if (arg === '--listen' || arg === '--port') { + if (arg === '--listen') { const value = argv[++i]; if (value === undefined) { - throw new Error(`${arg} requires a value`); - } - if (arg === '--listen') { - options.listen = value; - } else { - const port = Number(value); - if (!Number.isInteger(port) || port < 0 || port > 65535) { - throw new Error(`--port must be an integer in [0, 65535]: ${value}`); - } - options.port = port; + throw new Error('--listen requires a value'); } + options.listen = value; } else { throw new Error(`unknown argument: ${arg}`); } @@ -309,7 +308,7 @@ async function main(argv) { const server = createModelProxy({ baseUrl, apiKey, token, nonce }); await new Promise((resolve, reject) => { server.once('error', reject); - server.listen(options.port, options.listen, resolve); + server.listen(0, options.listen, resolve); }); const { port } = server.address(); const localBaseUrl = formatLocalBaseUrl( diff --git a/scripts/verify-pr-prepare.mjs b/scripts/verify-pr-prepare.mjs index 542497c866f..eaee003f66e 100644 --- a/scripts/verify-pr-prepare.mjs +++ b/scripts/verify-pr-prepare.mjs @@ -272,6 +272,34 @@ function run(command, args) { return spawnSync(command, args, { encoding: 'utf8' }); } +// The tree under inspection is PR-controlled and can persist hostile git +// config in .git/config: fsmonitor, hooks and clean filters all execute on an +// ordinary `git status`. On the host these run as the maintainer, outside the +// boundary, so every git command disables all three before reading the tree. +export const GIT_HARDENING = [ + '-c', + 'core.fsmonitor=false', + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'credential.helper=', +]; + +export function gitStatusArgs(root) { + return [ + ...GIT_HARDENING, + '-C', + root, + 'status', + '--porcelain', + '--untracked-files=normal', + ]; +} + +function runGit(args) { + return run('git', [...GIT_HARDENING, ...args]); +} + function report(lines) { process.stdout.write(`${lines.join('\n')}\n`); } @@ -296,7 +324,7 @@ function main(argv) { const { owner, name } = parseRepo(options.repo); const slug = `${owner}/${name}`; - const toplevel = run('git', ['rev-parse', '--show-toplevel']); + const toplevel = runGit(['rev-parse', '--show-toplevel']); if (toplevel.status !== 0) { return fail(3, 'not inside a git worktree — run this from the clone'); } @@ -338,7 +366,7 @@ function main(argv) { ]; for (const [label, oid, ref] of probes) { if ( - run('git', ['-C', root, 'cat-file', '-e', `${oid}^{commit}`]).status !== 0 + runGit(['-C', root, 'cat-file', '-e', `${oid}^{commit}`]).status !== 0 ) { return fail( 3, @@ -351,7 +379,7 @@ function main(argv) { // than the PR head or an uncommitted change would measure code the report // does not name. Checked before the artifact directory exists, so a refusal // leaves nothing behind. - const treeHead = run('git', ['-C', root, 'rev-parse', 'HEAD']); + const treeHead = runGit(['-C', root, 'rev-parse', 'HEAD']); if (treeHead.status !== 0) { return fail(3, 'could not read HEAD in this clone'); } @@ -363,13 +391,7 @@ function main(argv) { } // Explicit mode: a maintainer's status.showUntrackedFiles=no would otherwise // hide the untracked files this check exists to refuse. - const dirty = run('git', [ - '-C', - root, - 'status', - '--porcelain', - '--untracked-files=normal', - ]); + const dirty = run('git', gitStatusArgs(root)); if (dirty.status !== 0) { const detail = (dirty.stderr || '').trim() || `exit ${dirty.status}`; return fail(3, `could not read the working tree status: ${detail}`); @@ -387,7 +409,7 @@ function main(argv) { // git status trusts the index: skip-worktree / assume-unchanged entries // (a sparse checkout sets them en masse) hide tracked divergence from it, // so refuse anything that is not a plain cached 'H' entry. - const flagged = run('git', ['-C', root, 'ls-files', '-v']); + const flagged = runGit(['-C', root, 'ls-files', '-v']); if (flagged.status !== 0) { const detail = (flagged.stderr || '').trim() || `exit ${flagged.status}`; return fail(3, `could not read the index flags: ${detail}`);