Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions .github/scripts/qwen-triage-workflow.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1014,14 +1014,52 @@ describe('qwen-triage: flakiness gate (#9125)', () => {
// executes, so it is a changed test file exactly like M.
assert.match(
recordStep.run,
/^\s*git -c core\.quotePath=false diff -z --name-only --diff-filter=ACMRT "\$BASE_OID" HEAD \\\n\s*> "\$GATE_HOME\/files-all"$/m,
/^\s*\/usr\/bin\/git -c core\.quotePath=false diff -z --name-only --diff-filter=ACMRT "\$BASE_OID" HEAD \\\n\s*> "\$\{RUNNER_TEMP:\?\}\/flake-record-files-all"$/m,
Comment thread
yiliang114 marked this conversation as resolved.
'the NUL diff must flow straight into its file — $( ) strips NUL bytes, a pipeline swallows the exit status',
);
assert.match(
recordStep.run,
/^\s*BASE_OID="\$\(cat "\$\{RUNNER_TEMP:\?\}\/verify-base-oid"\)"$/m,
/^\s*rm -rf -- "\$\{RUNNER_TEMP:\?\}\/flake-record-files-all"\n\s*\/usr\/bin\/git -c core\.quotePath=false diff -z/m,
'the staging path must be unlinked immediately before the redirect — a planted symlink or directory there makes root write through it or hard-fail the record step',
);
assert.match(
recordStep.run,
/^\s*BASE_OID="\$\(\/usr\/bin\/cat "\$\{RUNNER_TEMP:\?\}\/verify-base-oid"\)"$/m,
Comment thread
yiliang114 marked this conversation as resolved.
'the record step must diff against the base OID captured while .git was root-owned, not re-resolve HEAD^1',
);
assert.match(
recordStep.run,
/^\s*case "\$BASE_OID" in$/m,
'the base OID must be shape-validated in the parent arm before the diff',
);
assert.match(
recordStep.run,
/^\s*\[0-9a-f\]\[0-9a-f\]\[0-9a-f\]\[0-9a-f\]\[0-9a-f\]\[0-9a-f\]\[0-9a-f\]\[0-9a-f\]\*\) ;;/m,
'the base OID shape must be an 8+-hex prefix — a planted valid OID would yield an empty diff and starve the gate into n/a',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The pins added for R2-3 cover the case header and the 8-hex arm, but the *) refusal arm of the moved case "$BASE_OID" validation (qwen-triage.yml:2920) has no assertion anywhere in this suite, even though neighbouring refusal branches (node-survivor refusal, grep-status refusal) are pinned. Mutation-verified: deleting the *) arm leaves the suite green (118 pass / 0 fail before and after). With that mutant live, a ${RUNNER_TEMP}/verify-base-oid holding a valid git revision that is not 8-hex — e.g. HEAD, plantable per the threat model this step's own comments document — falls through the case silently; git diff HEAD HEAD exits 0 with empty output; the child copies the empty staging file, grep no-matches (status 1, permitted), and the step logs "Recorded 0 changed test file(s)" — silently starving the flakiness gate into n/a instead of refusing loudly, which is exactly the outcome this line's own assertion message warns about. Add a presence pin for the refusal arm beside these assertions, e.g.:

assert.match(
  recordStep.run,
  /^\s*\*\) \/usr\/bin\/printf '::error::No trusted base OID recorded; refusing to record the flakiness-gate file list\.\\n'; exit 1 ;;$/m,
  'a malformed or planted base OID must refuse the record, never fall through into the diff',
);
中文说明

为 R2-3 新增的 pin 覆盖了 case 头部和 8-hex 分支,但移入父进程臂的 case "$BASE_OID" 校验(qwen-triage.yml:2920)的 *) 拒绝分支在整个套件里没有任何断言,尽管相邻的拒绝分支(node 进程幸存拒绝、grep 状态拒绝)都有 pin。变异验证:删除 *) 分支后套件仍然全绿(前后均为 118 通过 / 0 失败)。在该变异体存活时,${RUNNER_TEMP}/verify-base-oid 里若是一个合法但非 8-hex 的 git 修订——例如 HEAD,按本步骤自己注释写明的威胁模型这是可被种植的——会静默落空 case;git diff HEAD HEAD 以空输出退出码 0;子进程拷走空的暂存文件,grep 无匹配(状态 1,被允许),步骤记录 "Recorded 0 changed test file(s)"——flake gate 被静默饿成 n/a 而不是大声拒绝,这正是本行断言消息自己警告的结果。请在这些断言旁为拒绝分支补一条存在性 pin,例如上方代码块。

— qwen3.8-max via Qwen Code /review (v0.21.14)

);
assert.match(
recordStep.run,
/^\s*cp "\$\{RUNNER_TEMP:\?\}\/flake-record-files-all" "\$GATE_HOME\/files-all"$/m,
'the scrubbed child must copy the parent-recorded diff, never re-run git under env -i',
);
Comment thread
yiliang114 marked this conversation as resolved.
const recordDiffAt = recordStep.run.search(/^\s*\/usr\/bin\/git -c core\.quotePath=false diff -z/m);
Comment thread
yiliang114 marked this conversation as resolved.
const recordReExecAt = recordStep.run.search(/exec \/usr\/bin\/env -i/);
const recordCpAt = recordStep.run.search(/^\s*cp "\$\{RUNNER_TEMP:\?\}\/flake-record-files-all" "\$GATE_HOME\/files-all"$/m);
Comment thread
yiliang114 marked this conversation as resolved.
const recordInstallAt = recordStep.run.search(/^\s*install -d -m 0700 -o root -g root "\$GATE_HOME"$/m);
assert.ok(
recordDiffAt !== -1 && recordReExecAt !== -1 && recordCpAt !== -1 && recordInstallAt !== -1 &&
recordDiffAt < recordReExecAt && recordReExecAt < recordInstallAt && recordInstallAt < recordCpAt,
'the diff must be recorded in the parent arm before the env -i re-exec, and copied into the recreated root-only home',
);
// The scrubbed child must never re-run git under env -i: the ordering
// pin uses first-match semantics, so it cannot by itself forbid a
// second git in the child. Strip comments first (the child's own docs
// name `git diff` when describing what NOT to do) before asserting.
assert.doesNotMatch(
recordStep.run.slice(recordReExecAt).replace(/^\s*#.*$/gm, '').replace(/\\\n/g, ' '),
/\bgit\b[^\n]*\b(diff|log|show|whatchanged)\b/,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The new no-git-in-child assertion hand-enumerates four git subcommands (diff|log|show|whatchanged), leaving the rest of git's read surface unguarded when the class-closing check — banning \bgit\b outright — is available and false-positive-free. A sweep over the transformed child slice (comment-strip + continuation-fold at the actual re-exec offset) finds zero \bgit\b occurrences today, so the blanket ban admits no false positive; mutant bodies with git cat-file -p, git rev-list HEAD, git archive HEAD | tar -x, or git --no-pager grep all slip through the current regex green, while the blanket ban catches every one. Any of those reintroduced in the scrubbed child resurrects exactly the env -i failure this PR fixes: git under env -i cannot read the shallow merge-ref objects, the record step errors, and the flakiness gate degrades.

Suggested change
/\bgit\b[^\n]*\b(diff|log|show|whatchanged)\b/,
/\bgit\b/,
中文说明

新的“子进程禁止运行 git”断言只手工枚举了 4 个 git 子命令(diff|log|show|whatchanged),而类闭合的检查——直接禁止 \bgit\b——是可行的且不会误报,这让 git 其余的读取面处于无防护状态。对变换后的子进程切片(按真实 re-exec 位置截取、去注释、折叠续行)的扫描显示其中今天没有任何 \bgit\b 出现,因此全面禁止不会误报;带有 git cat-file -pgit rev-list HEADgit archive HEAD | tar -xgit --no-pager grep 的变异体都能绿着通过当前正则,而全面禁止能全部捕获。这些命令中任何一个被重新引入清空后的子进程,都会复活本 PR 所修复的那个 env -i 失败:env -i 下 git 读不到浅克隆 merge-ref 对象,record 步骤报错,flake gate 降级。

— qwen3.8-max via Qwen Code /review (v0.21.14)

'the scrubbed child must never re-run git under env -i — that is the failure shape of run 32227155960',
);
assert.ok(
recordStep.run.includes(
"grep -zE '\\.(test|spec)\\.(ts|tsx|js|jsx|mjs|cjs|mts|cts)$'",
Expand Down
39 changes: 24 additions & 15 deletions .github/workflows/qwen-triage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2903,6 +2903,25 @@ jobs:
/usr/bin/printf '::error::flake-gate record: step script changed between open and re-exec snapshot — refusing to run\n'
exit 1
fi
# Record the changed-test list in the PARENT's normal environment,
# before the env -i re-exec: the scrubbed child cannot read the
# shallow merge-ref objects — git's `safe.directory` global config
# lives under HOME, which `env -i` strips, so git refuses to read
# the base commit object and the diff fails with
# "Could not access <base-oid>". The diff reads git metadata only
# and executes no PR code, so running it here is safe, and the
# "Pin agent inputs" step already proved this environment reads
# the base. Write the NUL-delimited list under RUNNER_TEMP, which
# the re-exec forwards; the clean child copies it into the gate
# home below.
BASE_OID="$(/usr/bin/cat "${RUNNER_TEMP:?}/verify-base-oid")"
case "$BASE_OID" in
[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]*) ;;
*) /usr/bin/printf '::error::No trusted base OID recorded; refusing to record the flakiness-gate file list.\n'; exit 1 ;;
esac
rm -rf -- "${RUNNER_TEMP:?}/flake-record-files-all"
/usr/bin/git -c core.quotePath=false diff -z --name-only --diff-filter=ACMRT "$BASE_OID" HEAD \
> "${RUNNER_TEMP:?}/flake-record-files-all"
LD_PRELOAD= LD_AUDIT= LD_LIBRARY_PATH= exec /usr/bin/env -i \
PATH="$PATH" RUNNER_TEMP="${RUNNER_TEMP:-}" \
GITHUB_OUTPUT="${GITHUB_OUTPUT:-}" GITHUB_STEP_SUMMARY="${GITHUB_STEP_SUMMARY:-}" \
Expand Down Expand Up @@ -2975,21 +2994,11 @@ jobs:
# T (typechange) included: a symlink->regular flip changes what
# the runner executes, so it is a changed test file exactly
# like M — excluding it silently drops the file from the gate.
# Diff against the base OID the "Pin agent inputs" step recorded
# while .git was still root-owned, never `HEAD^1` again: this
# step runs in the env -i scrubbed child, and resolving the `^1`
# parent there intermittently fails with "Could not access
# 'HEAD^1'" on the persistent pool — the shallow merge-ref object
# store is left in a state prior --depth=2 fetches made
# unreadable. The OID is content-addressed and needs no parent
# walk, so the diff works from the already-captured value.
BASE_OID="$(cat "${RUNNER_TEMP:?}/verify-base-oid")"
case "$BASE_OID" in
[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]*) ;;
*) echo "::error::No trusted base OID recorded; refusing to record the flakiness-gate file list."; exit 1 ;;
esac
git -c core.quotePath=false diff -z --name-only --diff-filter=ACMRT "$BASE_OID" HEAD \
> "$GATE_HOME/files-all"
# The diff was already computed in the parent (before the env -i
# re-exec, where git can read the shallow merge-ref objects) and
# staged under RUNNER_TEMP; this scrubbed child only copies it
# into the root-only gate home.
cp "${RUNNER_TEMP:?}/flake-record-files-all" "$GATE_HOME/files-all"
# .mts/.cts included: vitest's default include set collects them.
# Only a no-match (status 1) may yield an empty list: a grep
# error (status 2, e.g. ENOSPC opening the output) is
Expand Down
Loading