-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(autofix): require isolated targeted E2E proof #8318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b8e27b0
f06ecad
25ea11c
dad1b5a
00bc2bb
b48e4f9
7d710a1
f4545ee
26c6f39
e5f317e
902878f
16fc07c
1ed5439
f2b350e
fa69200
d591517
6ea2b27
8517bdf
a68a368
460291c
bc0e4cf
c0ed2ff
2897fee
8a21369
bfd7e49
3d1e19e
a6d0445
83fb5f6
268873b
4259fe3
a2f1da3
618c2a1
f328ca7
42730ec
4790b85
c24a47d
b3d176d
241625b
dd1d0af
53c0a29
6f3c70e
f60f820
300e308
86f8f27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| const candidateCli = process.env['AUTOFIX_CANDIDATE_CLI']; | ||
| const uid = Number(process.env['AUTOFIX_VERIFY_UID']); | ||
| const gid = Number(process.env['AUTOFIX_VERIFY_GID']); | ||
|
|
||
| if ( | ||
| !candidateCli || | ||
| !Number.isInteger(uid) || | ||
| uid <= 0 || | ||
| !Number.isInteger(gid) || | ||
| gid <= 0 | ||
| ) { | ||
| throw new Error('Missing isolated candidate CLI configuration'); | ||
| } | ||
|
|
||
| if (process.getuid() === 0) { | ||
| process.setgroups([]); | ||
| process.setgid(gid); | ||
| process.setuid(uid); | ||
| } else if (process.getuid() !== uid || process.getgid() !== gid) { | ||
| throw new Error('Isolated candidate CLI is running as an unexpected user'); | ||
| } | ||
|
|
||
| const candidate = await import(candidateCli); | ||
| if (typeof candidate.runCliEntryPoint !== 'function') { | ||
| throw new Error('Candidate CLI does not export runCliEntryPoint'); | ||
| } | ||
| await candidate.runCliEntryPoint(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import { resolve } from 'node:path'; | ||
|
|
||
| const workspace = process.env.AUTOFIX_WORKSPACE; | ||
|
|
||
| if (!workspace) { | ||
| throw new Error('Missing isolated Vitest configuration'); | ||
| } | ||
|
|
||
| export default { | ||
| root: resolve(workspace, 'integration-tests'), | ||
| test: { | ||
| // Mirror the integration-tests config default (TB_TIMEOUT_MINUTES=5); | ||
| // that env var is not on the wrapper's env allowlist. Without this, | ||
| // Vitest's 5 s default would fail any future allowlisted case that does | ||
| // not declare its own timeout. | ||
| testTimeout: 5 * 60 * 1000, | ||
| retry: 0, | ||
| fileParallelism: false, | ||
| pool: 'forks', | ||
| poolOptions: { | ||
| forks: { | ||
| singleFork: true, | ||
| isolate: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,18 +23,34 @@ fail() { | |
| exit 1 | ||
| } | ||
|
|
||
| # Guard the generator itself: if it CRASHES (e.g. a type error the agent | ||
| # introduced in the schema source), a caller running under set -eo pipefail | ||
| # would abort before outcome=failed is written, leaving OUTCOME unset. Handle | ||
| # it here so the failure is explicit, not inferred from job.status. | ||
| if ! npm run generate:settings-schema; then | ||
| echo "❌ Settings schema generator failed to run." | ||
| fail | ||
| fi | ||
|
|
||
| if [[ -n "$(git status --porcelain "${SCHEMA_FILE}")" ]]; then | ||
| echo "❌ ${SCHEMA_FILE} is out of date. Run: npm run generate:settings-schema" | ||
| git --no-pager diff -- "${SCHEMA_FILE}" || true | ||
| git checkout -- "${SCHEMA_FILE}" || true | ||
| fail | ||
| # Autofix rejects changes to the committed schema and to a best-effort | ||
| # snapshot of the sources that feed it (the protected-path allowlist in | ||
| # validate-autofix-verification-outputs.mjs) before this gate runs; the | ||
| # snapshot is hand-curated, and the normal schema gate still runs on the | ||
| # published PR. Executing the candidate's schema module graph here would let | ||
| # module initialization short-circuit the trusted comparison. | ||
| # TODO: run-autofix-review-verification.sh still executes the generator | ||
| # on candidate code without this wrapper or the protected-path allowlist; the | ||
| # review-address chain was scoped out of the targeted E2E redesign and needs | ||
| # the same isolation before its schema gate is trusted the same way. | ||
| if [[ -n "${AUTOFIX_VERIFY_COMMAND:-}" ]]; then | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The AUTOFIX_VERIFY_COMMAND skip branch is pinned only structurally (toContain), while sibling check-autofix-contracts.sh got a behavioural wrapper test in the same file; no test executes this script with AUTOFIX_VERIFY_COMMAND set. — Failure scenario: A future edit hoisting the generator out of the else keeps every structural assertion green; at runtime the sealed issue-autofix-verify job runs the generator outside the isolated wrapper after finalize made tracked files root-owned/read-only → EACCES crash, and every autofix verification fails at the schema gate with a misleading error. Suggested fix: Mirror the contracts-script harness: run check-settings-schema.sh with a fake AUTOFIX_VERIFY_COMMAND and a fake npm on PATH; assert exit 0 and an empty npm log. 中文说明AUTOFIX_VERIFY_COMMAND 跳过分支只有结构化固定(toContain),而兄弟脚本 check-autofix-contracts.sh 在同文件中有行为学包装测试;没有任何测试在设置 AUTOFIX_VERIFY_COMMAND 时执行本脚本并断言退出 0 且不调用 npm。若未来把生成器移出 else(结构化断言仍全绿),密封 issue-autofix-verify job 会在 finalize 之后以只读 schema 文件运行生成器 → EACCES 崩溃,每次 autofix 验证都在 schema 门禁处以误导性错误失败。建议:仿照 contracts 脚本补行为学测试(假 npm + 假 AUTOFIX_VERIFY_COMMAND,断言 exit 0 且 npm 日志为空)。 — qwen3.8-max via Qwen Code /review (v0.21.4) |
||
| echo 'Skipping settings-schema freshness check: Autofix rejects changes to the committed schema and its protected sources before this gate runs.' | ||
| exit 0 | ||
| else | ||
| # Guard the generator itself: if it CRASHES (e.g. a type error introduced in | ||
| # the schema source), report an explicit gate failure. | ||
| if ! npm run generate:settings-schema; then | ||
| echo "❌ Settings schema generator failed to run." | ||
| fail | ||
| fi | ||
| if ! schema_status="$(git status --porcelain "${SCHEMA_FILE}")"; then | ||
| echo "❌ Failed to inspect ${SCHEMA_FILE} after generation." | ||
| fail | ||
| fi | ||
| if [[ -n "${schema_status}" ]]; then | ||
| echo "❌ ${SCHEMA_FILE} is out of date. Run: npm run generate:settings-schema" | ||
| git --no-pager diff -- "${SCHEMA_FILE}" || true | ||
| git checkout -- "${SCHEMA_FILE}" || true | ||
| fail | ||
| fi | ||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Neither side of the contract-gate isolation pairing is tested: the behavioural test for this script never sets
AUTOFIX_VERIFY_COMMAND(so it always exercises the bare"$@"branch), and no workflow test pins thatissue-autofix-verifyinvokes the gate withAUTOFIX_VERIFY_COMMAND="${verify_cmd}"— the onlyAUTOFIX_VERIFY_COMMANDassertions in the suite are forcheck-settings-schema.sh. — Concrete cost: collapsingrun_candidateto"$@"(an easy refactor, since the indirection looks redundant without caller context) leaves every test green; in the sealed verify job the i18n and tool-drift checks would then run as the runner user with its full environment instead of through the credential-free isolated-UID wrapper, violating the design doc's "every command receives a fresh isolated HOME through the trusted credential-free command wrapper" invariant.Fix: in the existing behavioural test, add a case that sets
AUTOFIX_VERIFY_COMMANDto a mock that logs its argv and assert the logged invocation is<wrapper> ${GITHUB_WORKSPACE} run check-i18n; and assert the verify-job text containsAUTOFIX_VERIFY_COMMAND="${verify_cmd}"adjacent tocheck-autofix-contracts.sh.中文说明
[Suggestion] 合约门禁隔离配对的两侧都没有测试:本脚本的行为测试从不设置
AUTOFIX_VERIFY_COMMAND(因此总是走裸"$@"分支),也没有任何 workflow 测试固定issue-autofix-verify会以AUTOFIX_VERIFY_COMMAND="${verify_cmd}"调用该门禁——套件中唯一的AUTOFIX_VERIFY_COMMAND断言是针对check-settings-schema.sh的。— 具体代价:把run_candidate折叠成"$@"(由于没有调用方上下文,这层间接看起来冗余,是个很容易发生的重构)后所有测试仍全绿;在密封的 verify job 中,i18n 与工具漂移检查将以 runner 用户及其完整环境运行,而非通过无凭据的隔离 UID wrapper,违反设计文档“每条命令都通过可信的无凭据命令 wrapper 获得一个全新隔离 HOME”的不变量。修复:在行为测试中新增一个设置AUTOFIX_VERIFY_COMMAND为记录 argv 的 mock 的用例,并断言 verify job 文本中check-autofix-contracts.sh附近含有AUTOFIX_VERIFY_COMMAND="${verify_cmd}"。— qwen3.8-max-preview via Qwen Code /review (v0.21.3)