mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
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 <trusted-clone>, <clone> 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.
This commit is contained in:
Shaojin Wen committed
1 parent
fed6afb717
commit
b5c717a5d5
12 files changed
+466
-136
No files matched your search
@@ -160,10 +160,11 @@ creates `tmp/pr<n>-verify-<ts>/` 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 `<pr-clone>`: the proxy, the prepare preflight,
|
||||
the credential scan, the isolation gate and the checker all come from
|
||||
`<trusted-clone>`. 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 `<trusted-clone>`. 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 <pr-clone> && node <trusted-clone>/scripts/verify-pr-prepare.mjs <n> --repo <owner>/<repo>
|
||||
mkdir -p ~/.cache/qwen-verify/pr<n>/qwen-base
|
||||
cp <pr-clone>/tmp/verify-context/pr<n>.json ~/.cache/qwen-verify/pr<n>/pr.json
|
||||
gh api "repos/<owner>/<repo>/issues/<n>/comments" --paginate --slurp \
|
||||
--jq 'flatten | map(select(.body | test("Verdict:"))) | last | .body // empty' \
|
||||
> ~/.cache/qwen-verify/pr<n>/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<n>/pr.json)"
|
||||
git -C <pr-clone> archive "$BASE_OID" -- .qwen \
|
||||
| tar -x -C ~/.cache/qwen-verify/pr<n>/qwen-base
|
||||
@@ -247,30 +251,46 @@ certify the round.
|
||||
`cwd=<pr-clone>` 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 `<pr-clone>`.
|
||||
`$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
|
||||
`<pr-clone>` — 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=<upstream-url> \
|
||||
node <trusted-clone>/scripts/verify-pr-model-proxy.mjs \
|
||||
--listen <bridge-ip> > ~/.cache/qwen-verify/pr<n>/proxy.json 2>&1 &
|
||||
echo $! > ~/.cache/qwen-verify/pr<n>/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<n>/proxy.json 2>/dev/null && break
|
||||
sleep 0.1
|
||||
done
|
||||
chmod 600 ~/.cache/qwen-verify/pr<n>/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<n>/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<n>/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<n>/proxy.json)"
|
||||
test -n "$QWEN_VERIFY_PROXY_TOKEN" || { echo "proxy.json has no token" >&2; exit 1; }
|
||||
docker run -d --rm --name qwen-verify-<n> \
|
||||
--user "$(id -u):$(id -g)" -e HOME=/tmp/qwen-home \
|
||||
-v <pr-clone>:/work \
|
||||
@@ -333,24 +357,27 @@ certify the round.
|
||||
|
||||
```bash
|
||||
docker exec -i qwen-verify-<n> 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-<n> bash -lc '
|
||||
@@ -380,6 +407,7 @@ certify the round.
|
||||
docker rm -f qwen-verify-<n>
|
||||
kill "$(cat ~/.cache/qwen-verify/pr<n>/proxy.pid)"
|
||||
rm -f ~/.cache/qwen-verify/pr<n>/proxy.json ~/.cache/qwen-verify/pr<n>/proxy.pid
|
||||
unset QWEN_VERIFY_PROXY_TOKEN
|
||||
```
|
||||
|
||||
`<round-dir>` is `<pr-clone>/tmp/pr<n>-verify-<ts>`. 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"`.
|
||||
|
||||
`<clone>` 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 <n> --repo <owner>/<repo> --snapshot
|
||||
**Run both halves of that guard through
|
||||
`<trusted-clone>/scripts/verify-pr-guard.mjs`, because the hand-run version
|
||||
fails open.** `--pr <n> --repo <owner>/<repo> --snapshot
|
||||
<file>` at round start and again before posting, then `--compare --start <a>
|
||||
--now <b>`; 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 —
|
||||
|
||||
@@ -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. `<trusted-clone>` supplies every script the
|
||||
round runs, including the prepare preflight, the credential scan, the
|
||||
isolation gate and the post-round checker; `<pr-clone>` 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; `<pr-clone>`
|
||||
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 `<trusted-clone>` with `--context <host copy>`,
|
||||
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
|
||||
|
||||
@@ -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，而不是为无人读取的补全付费。
|
||||
- 配方使用两个检出。`<trusted-clone>` 提供轮次运行的每个脚本，包括 prepare 预检、凭据扫描、隔离门禁与轮次后的检查器；`<pr-clone>` 是停在 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，而不是为无人读取的补全付费。
|
||||
- 配方使用两个检出。`<trusted-clone>` 提供轮次运行的每个脚本，包括 prepare 预检、凭据扫描、隔离门禁、发布守卫与轮次后的检查器；`<pr-clone>` 是停在 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`。轮次结束后，检查器从 `<trusted-clone>` 运行并带 `--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。** 否决：打印值必须是客户端实际拨号的地址。
|
||||
- **仅把树挂成只读。** 否决：轮次必须在树里构建并写产物。信任边界靠「特权宿主步骤从 `<trusted-clone>` 运行」以及「从宿主快照钉住期望 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 已注入。
|
||||
@@ -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 <snapshot>` 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 <n> --repo <owner>/<repo> --snapshot <file>` fetches the head, the PR
|
||||
state and all three comment lists with `--paginate`, and writes the file only
|
||||
|
||||
@@ -25,7 +25,7 @@
|
||||
新增三个独立脚本，并让 skill 在流程需要的位置指向它们，而不是放在文末。
|
||||
|
||||
- `scripts/verify-pr-prepare.mjs <pr> --repo <owner>/<repo>` 负责解析坐标：用 lane 相同的字段列表抓取快照、校验两个 OID 在本克隆中存在、拒绝不是 PR head 干净检出的树、创建带 `evidence/` 子目录的产物目录，并报告当前进程能观测到的隔离信号。它不据此设闸：解析元数据需要 `gh`，而 `gh` 与边界互斥，因此这条路径总是在宿主上运行。闸门是 `--require-isolation`，用于在边界内、轮次执行任何东西之前运行。
|
||||
- `scripts/verify-pr-check.mjs <dir>` 把发布侧的规则套到本地产物目录：必需文件、四个判定词、为正且自洽的断言三元组、以及不允许带失败断言的 `merge-ready`。报告形态与证据命名只作为警告，因为纯文档轮次没有截图，检查器也无法知道是否跑过 harness。
|
||||
- `scripts/verify-pr-check.mjs <dir>` 把发布侧的规则套到本地产物目录：必需文件、四个判定词、为正且自洽的断言三元组、以及不允许带失败断言的 `merge-ready`。报告形态与证据命名只作为警告，因为纯文档轮次没有截图，检查器也无法知道是否跑过 harness。带 `--context <snapshot>` 时期望 head 来自该宿主持有的文件，而不是产物里可写的 `context.json`；不可用快照是硬性 exit 1 而非警告：产物目录可被轮次刚执行的 PR 代码写入。
|
||||
- `scripts/verify-pr-guard.mjs` 接管发布前守卫的两个半程。`--pr <n> --repo <owner>/<repo> --snapshot <file>` 抓取 head、PR 状态以及三个评论列表（带 `--paginate`），只有每次调用都成功才写文件。`--compare --start <a> --now <b>` 列出本轮 dirty 的全部原因并退出 1。
|
||||
|
||||
检查器在产物契约要求轮次写出的产物上新增三条规则。产物契约里三者都要点名：产物清单没要求的文件，轮次就不会产出，而对一个不存在的文件做检查是静默的。
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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:[email protected]/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(
|
||||
[
|
||||
|
||||
@@ -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 <trusted-clone>/scripts/verify-pr-model-proxy.mjs',
|
||||
);
|
||||
expect(skill).toContain('<trusted-clone>/scripts/verify-pr-guard.mjs');
|
||||
expect(skill).toContain(
|
||||
'`<clone>` 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
|
||||
// <pr-clone>/scripts.
|
||||
@@ -568,38 +662,54 @@ describe('local container recipe pins', () => {
|
||||
expect(checkerCalls.length).toBeGreaterThanOrEqual(3);
|
||||
for (const prefix of checkerCalls) expect(prefix).toBe('<trusted-clone>');
|
||||
expect(skill).not.toMatch(
|
||||
/node <pr-clone>\/scripts\/verify-pr-(prepare|check|model-proxy)/,
|
||||
/node <pr-clone>\/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 <pr-clone> archive "$BASE_OID" -- .qwen');
|
||||
expect(skill).toContain(
|
||||
'-v ~/.cache/qwen-verify/pr<n>/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 <pr-clone>:/work');
|
||||
expect(skill).toContain('-v <trusted-clone>/scripts:/trusted-scripts:ro');
|
||||
expect(skill).toContain('-v ~/.cache/qwen-verify/pr<n>:/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=<real-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=<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<n>/pr.json');
|
||||
it('snapshots the previous report and pins both checker call sites', () => {
|
||||
expect(skill).toContain('previous-report.md');
|
||||
expect(skill).toContain('issues/<n>/comments');
|
||||
const contextCalls = [
|
||||
...skill.matchAll(/verify-pr-check\.mjs --context <host snapshot>/g),
|
||||
];
|
||||
expect(contextCalls.length).toBeGreaterThanOrEqual(2);
|
||||
});
|
||||
});
|
||||
@@ -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',
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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:<pat>@…` 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,
|
||||
|
||||
@@ -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=<url> \
|
||||
* node <trusted-clone>/scripts/verify-pr-model-proxy.mjs \
|
||||
* [--listen <host>] [--port <n>]
|
||||
* [--listen <host>]
|
||||
*
|
||||
* 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 <host> address to bind (default 127.0.0.1; use the Docker
|
||||
* bridge address, e.g. 172.17.0.1, for container clients)
|
||||
* --port <n> 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(
|
||||
|
||||
@@ -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}`);
|
||||
|
||||
Reference in new issue
Block a user