diff --git a/.github/scripts/resolve-owning-packages.sh b/.github/scripts/resolve-owning-packages.sh new file mode 100755 index 00000000000..3cad53df55e --- /dev/null +++ b/.github/scripts/resolve-owning-packages.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# Owning-workspace resolver, shared by the qwen-autofix verify steps +# (.github/workflows/qwen-autofix.yml) so the two gates cannot drift apart. +# +# Reads changed file paths on stdin (one per line, e.g. the output of +# `git diff --name-only`) and emits, sorted and unique on stdout, the OWNING +# npm workspace of each: the workspace whose location is the LONGEST matching +# path prefix of the file. +# +# The workspace set is expanded from the ON-DISK root package.json `workspaces` +# globs, NOT from `npm query`/node_modules: node_modules reflects the BASE +# checkout the gate installed, so a workspace the PR branch ADDS (a new channel +# adapter, a new sdk — the issue-fix job's whole purpose) would be invisible and +# its tests silently skipped. It is also NOT "any ancestor dir with a +# package.json": a fixture/example package inside a workspace's src tree (e.g. +# packages/cli/src/commands/extensions/examples/starter) has a package.json but +# is not a workspace, so resolving a change there to the fixture would skip +# packages/cli's own tests. Expanding the globs (shallow `dir/*` + literals, +# honouring `!` negations, keeping dirs that contain a package.json) matches +# what `npm run --workspace` accepts downstream and reflects the branch. +# +# Invoked with the repository as the working directory. Staged to RUNNER_TEMP +# from the trusted base checkout (never the PR branch) alongside +# check-settings-schema.sh. +set -euo pipefail + +workspaces="$(node -e ' + const fs = require("fs"); + const path = require("path"); + const pkg = JSON.parse(fs.readFileSync("package.json", "utf8")); + let globs = pkg.workspaces || []; + if (!Array.isArray(globs)) globs = globs.packages || []; + const positive = []; + const negative = []; + for (const g of globs) (g[0] === "!" ? negative : positive).push(g.replace(/^!/, "")); + const hasManifest = (d) => { + try { return fs.statSync(path.join(d, "package.json")).isFile(); } + catch { return false; } + }; + const expand = (g) => { + const star = g.indexOf("*"); + if (star === -1) return [g]; + const parent = g.slice(0, star).replace(/\/$/, ""); + let entries = []; + try { entries = fs.readdirSync(parent, { withFileTypes: true }); } + catch { return []; } + return entries.filter((e) => e.isDirectory()).map((e) => path.posix.join(parent, e.name)); + }; + const dirs = new Set(); + for (const g of positive) for (const d of expand(g)) if (hasManifest(d)) dirs.add(d); + for (const g of negative) { for (const d of expand(g)) dirs.delete(d); dirs.delete(g); } + process.stdout.write([...dirs].sort().join("\n")); +')" + +if [[ -z "${workspaces}" ]]; then + echo "resolve-owning-packages: no workspaces resolved from package.json" >&2 + exit 1 +fi + +while IFS= read -r f || [[ -n "${f}" ]]; do + [[ -n "${f}" ]] || continue + best='' + while IFS= read -r w; do + [[ -n "${w}" ]] || continue + if [[ "${f}" == "${w}"/* && "${#w}" -gt "${#best}" ]]; then + best="${w}" + fi + done <<< "${workspaces}" + # `if`, not `[[ ]] && printf`: an unmatched file (best empty) must leave the + # loop body's exit status 0, or under `set -o pipefail` a no-match on the LAST + # line makes `while … | sort` fail and (with `set -e`) aborts the script. + if [[ -n "${best}" ]]; then printf '%s\n' "${best}"; fi +done | sort -u diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index b850c51db2d..177cbb9a231 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -525,6 +525,7 @@ jobs: - name: 'Stage trusted schema gate' run: |- cp .github/scripts/check-settings-schema.sh "${RUNNER_TEMP}/check-settings-schema.sh" + cp .github/scripts/resolve-owning-packages.sh "${RUNNER_TEMP}/resolve-owning-packages.sh" - name: 'Check bot credentials' env: @@ -1035,12 +1036,26 @@ jobs: # Run changed/related tests for the packages this fix touches. # --changed follows the import graph so transitive breakage is caught. # Full regression is covered by regular CI on the PR after the push. + # Map each changed file to its OWNING npm workspace via the trusted + # staged resolver, shared with the other verify gate so both resolve + # packages identically. It expands the on-disk root package.json + # workspaces globs (so a workspace the branch ADDS is included) and + # takes each file's longest-prefix workspace — never a flat + # 'packages/' (ENOENT-crashes on nested packages) nor a fixture + # package.json inside a workspace's src tree (would skip the owning + # workspace's tests). No '|| true': a resolver error (missing node, an + # unreadable manifest) must fail the gate loudly rather than silently + # skip package tests; legitimate no-match input already exits 0 empty. CHANGED_PKGS="$(git diff --name-only "origin/main...${BRANCH}" \ - | grep -oE '^packages/[^/]+' | sort -u || true)" + | bash "${RUNNER_TEMP}/resolve-owning-packages.sh")" if [[ -z "${CHANGED_PKGS}" ]]; then echo 'No package changes detected; skipping package tests.' else for p in ${CHANGED_PKGS}; do + if [[ ! -f "${p}/package.json" ]]; then + echo "Skipping ${p}: no package.json." + continue + fi test_script="$(node -e 'const fs = require("node:fs"); const pkg = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); process.stdout.write(pkg.scripts?.test || "");' "${p}/package.json")" if [[ "${test_script}" != *vitest* ]]; then echo "Skipping ${p}: test script is not Vitest." @@ -1931,6 +1946,7 @@ jobs: - name: 'Stage trusted schema gate and agent runner' run: |- cp .github/scripts/check-settings-schema.sh "${RUNNER_TEMP}/check-settings-schema.sh" + cp .github/scripts/resolve-owning-packages.sh "${RUNNER_TEMP}/resolve-owning-packages.sh" # The agent step runs AFTER prepare checks out the PR branch, so # invoking the runner from the working tree would execute # branch-controlled code on the host with the model key in env @@ -2413,6 +2429,20 @@ jobs: if: |- ${{ always() && steps.prepare.outputs.stale != 'true' }} run: |- + # Record whether the agent left a commit FIRST — this is a ref-only + # diff, so it runs before the failure.md early-exits and covers an + # agent that commits and then aborts. The failure handoff keys its + # "was NOT pushed / commit discarded" wording on this, NOT on + # outcome=failed: abort / pre-commit-gate paths that never committed + # keep the neutral framing. `git diff --quiet` exits 1 for a real diff + # (committed) but 128 on a bad ref — only 1 counts as a commit, so a + # git error is not misreported as a discarded commit. + committed_rc=0 + git diff --quiet "origin/${BRANCH}...${BRANCH}" || committed_rc=$? + if [[ "${committed_rc}" -eq 1 ]]; then + echo "committed=true" >> "${GITHUB_OUTPUT}" + fi + if [[ -f "${WORKDIR}/failure.md" && -n "$(git status --porcelain)" ]]; then echo "❌ Agent wrote failure.md after leaving a dirty workspace:" git status --short @@ -2476,12 +2506,26 @@ jobs: # Test changed/related files for the packages this PR touches. # --changed follows the import graph so transitive breakage is caught. # Full regression is covered by regular CI on the PR after the push. + # Map each changed file to its OWNING npm workspace via the trusted + # staged resolver, shared with the other verify gate so both resolve + # packages identically. It expands the on-disk root package.json + # workspaces globs (so a workspace the branch ADDS is included) and + # takes each file's longest-prefix workspace — never a flat + # 'packages/' (ENOENT-crashes on nested packages) nor a fixture + # package.json inside a workspace's src tree (would skip the owning + # workspace's tests). No '|| true': a resolver error (missing node, an + # unreadable manifest) must fail the gate loudly rather than silently + # skip package tests; legitimate no-match input already exits 0 empty. CHANGED_PKGS="$(git diff --name-only "origin/main...${BRANCH}" \ - | grep -oE '^packages/[^/]+' | sort -u || true)" + | bash "${RUNNER_TEMP}/resolve-owning-packages.sh")" if [[ -z "${CHANGED_PKGS}" ]]; then echo 'No package changes detected; skipping package tests.' else for p in ${CHANGED_PKGS}; do + if [[ ! -f "${p}/package.json" ]]; then + echo "Skipping ${p}: no package.json." + continue + fi test_script="$(node -e 'const fs = require("node:fs"); const pkg = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); process.stdout.write(pkg.scripts?.test || "");' "${p}/package.json")" if [[ "${test_script}" != *vitest* ]]; then echo "Skipping ${p}: test script is not Vitest." @@ -2634,6 +2678,7 @@ jobs: ${{ always() && (needs.route.outputs.dry_run == 'true' || failure() || cancelled()) }} env: OUTCOME: '${{ steps.verify.outputs.outcome }}' + COMMITTED: '${{ steps.verify.outputs.committed }}' CONFLICT: '${{ steps.prepare.outputs.conflict }}' DRY_RUN: '${{ needs.route.outputs.dry_run }}' GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' @@ -2768,7 +2813,19 @@ jobs: echo "${HEADLINE}" echo if [[ -n "${DETAIL_FILE}" ]]; then - echo "**What I found before stopping:**" + if [[ "${COMMITTED}" == "true" ]]; then + # The agent committed (verify recorded committed=true before + # any gate could fail), but every path that reaches this + # handoff skipped "Push and report" — nothing landed on the + # branch. Say so before the agent's address-summary.md, which + # can read like a success and cite that now-discarded commit + # SHA. Keyed on committed, NOT outcome=failed: the abort/no-op + # paths (failure.md, dirty tree, unchanged branch, missing + # summary) made no commit and keep the neutral framing below. + echo "⚠️ This change was NOT pushed — any commit referenced below was made only in the runner workspace and has been discarded. What the agent reported:" + else + echo "**What I found before stopping:**" + fi # -c drops any partial multi-byte sequence a byte-level head -c may # have split, so the comment body stays valid UTF-8. iconv -c still # EXITS 1 when it discards a byte, which under this shell's diff --git a/scripts/tests/package-scripts.test.js b/scripts/tests/package-scripts.test.js index e04387f7005..6061f0abf8f 100644 --- a/scripts/tests/package-scripts.test.js +++ b/scripts/tests/package-scripts.test.js @@ -506,7 +506,9 @@ describe('package scripts', () => { expect(verifyStep).toContain( 'npm run test --workspace "${p}" --if-present -- --changed origin/main --passWithNoTests', ); - expect(verifyStep).toContain("grep -oE '^packages/[^/]+'"); + expect(verifyStep).toContain( + 'bash "${RUNNER_TEMP}/resolve-owning-packages.sh"', + ); expect(verifyStep).toContain('pkg.scripts?.test'); expect(verifyStep).toContain('!= *vitest*'); expect(verifyStep).not.toContain( diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index a070010bc40..64292cc0e70 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -15,7 +15,7 @@ import { writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; const workflow = readFileSync('.github/workflows/qwen-autofix.yml', 'utf8'); @@ -2566,6 +2566,16 @@ describe('qwen-autofix workflow', () => { expect(step).not.toContain( 'bash .github/scripts/check-settings-schema.sh', ); + // The owning-package resolver is likewise a shared script staged from the + // trusted base, invoked (not inlined) so the two gates cannot drift into + // resolving packages differently. The old inline detection must be gone. + expect(step).toContain( + 'bash "${RUNNER_TEMP}/resolve-owning-packages.sh"', + ); + expect(step).not.toContain("grep -oE '^packages/[^/]+'"); + expect(step).not.toContain( + 'bash .github/scripts/resolve-owning-packages.sh', + ); expect(step).toContain( 'No package changes detected; skipping package tests.', ); @@ -2578,6 +2588,12 @@ describe('qwen-autofix workflow', () => { /cp \.github\/scripts\/check-settings-schema\.sh "\$\{RUNNER_TEMP\}\/check-settings-schema\.sh"/g, ) ?? [], ).toHaveLength(2); + // The owning-package resolver is staged the same way, in the same steps. + expect( + workflow.match( + /cp \.github\/scripts\/resolve-owning-packages\.sh "\$\{RUNNER_TEMP\}\/resolve-owning-packages\.sh"/g, + ) ?? [], + ).toHaveLength(2); // In the issue-autofix job the staging must happen BEFORE the verify gate's // `git checkout "${BRANCH}"` (first occurrence in the file is the issue // job's): the agent's commits can touch .github/scripts, so a post-checkout @@ -2614,6 +2630,22 @@ describe('qwen-autofix workflow', () => { expect(schemaScript).toContain('is out of date'); expect(schemaScript).toContain('git status --porcelain'); expect(schemaScript).toContain('outcome=failed'); + // The owning-package resolver maps each changed path to the longest-prefix + // npm WORKSPACE, expanded from the ON-DISK root package.json workspaces + // globs (so a workspace the branch adds is included — node_modules is the + // base's), not "any ancestor dir with a package.json" (a fixture inside a + // workspace's src tree is not a workspace). It fails loudly on an empty set. + const resolveScript = readFileSync( + '.github/scripts/resolve-owning-packages.sh', + 'utf8', + ); + expect(resolveScript).toContain('readFileSync("package.json"'); + expect(resolveScript).not.toContain('npm query .workspace'); + expect(resolveScript).toContain( + '[[ "${f}" == "${w}"/* && "${#w}" -gt "${#best}" ]]', + ); + expect(resolveScript).toContain('no workspaces resolved from package.json'); + expect(resolveScript).toContain('sort -u'); // The review gate's freshness check is a STRUCTURAL guard: the script call // must run BEFORE the no-op/unchanged return, so a stale-schema PR the agent // wrongly no-ops fails (outcome=failed) instead of being reported as evaluated @@ -2778,6 +2810,184 @@ describe('qwen-autofix workflow', () => { } }); + it('resolver maps each changed file to its longest-prefix workspace from the on-disk manifest', () => { + // Reads the on-disk root package.json workspaces globs (NO npm install), so + // it sees workspaces the branch ADDS — node_modules would only have the + // base's. Set up a new top-level and a new nested workspace, a fixture + // package.json inside a workspace's src tree (NOT a workspace), a + // !-excluded workspace, and a non-workspace dir. + const script = resolve('.github/scripts/resolve-owning-packages.sh'); + const dir = mkdtempSync(join(tmpdir(), 'ws-')); + try { + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ + name: 'root', + private: true, + workspaces: [ + 'packages/*', + 'packages/channels/*', + '!packages/desktop', + ], + }), + ); + for (const pkg of [ + 'packages/cli', + 'packages/brandnew', // a new top-level workspace the branch adds + 'packages/channels/base', + 'packages/channels/newchannel', // a new nested workspace the branch adds + 'packages/desktop', // excluded by the ! glob + 'packages/cli/src/commands/examples/starter', // fixture, NOT a workspace + ]) { + mkdirSync(join(dir, pkg), { recursive: true }); + writeFileSync(join(dir, pkg, 'package.json'), '{}'); + } + mkdirSync(join(dir, 'packages/sdk-python'), { recursive: true }); // no manifest + const changed = + [ + 'packages/cli/src/commands/examples/starter/src/index.ts', // -> packages/cli + 'packages/brandnew/src/z.ts', // -> packages/brandnew (branch-added) + 'packages/channels/newchannel/src/y.ts', // -> newchannel (branch-added nested) + 'packages/desktop/src/d.ts', // excluded workspace -> dropped + 'packages/sdk-python/foo.py', // no manifest -> dropped + 'README.md', // outside packages/ -> dropped + ].join('\n') + '\n'; + const out = execFileSync('bash', [script], { + input: changed, + cwd: dir, + encoding: 'utf8', + }).trim(); + expect(out.split('\n').sort()).toEqual([ + 'packages/brandnew', + 'packages/channels/newchannel', + 'packages/cli', + ]); + expect(out).not.toContain('examples/starter'); // fixture never owns + expect(out).not.toContain('sdk-python'); + expect(out).not.toContain('packages/desktop'); // ! negation honoured + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('resolver fails loudly when the manifest declares no workspaces', () => { + // An empty workspace set (unreadable/missing workspaces) must be a hard, + // non-zero exit — not a silent empty output that reads as "no package + // changes" and skips the gate. The call sites carry no `|| true`. + const script = resolve('.github/scripts/resolve-owning-packages.sh'); + const dir = mkdtempSync(join(tmpdir(), 'nows-')); + try { + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ name: 'root' }), + ); + let threw = false; + let stderr = ''; + try { + execFileSync('bash', [script], { + input: 'packages/cli/src/x.ts\n', + cwd: dir, + encoding: 'utf8', + }); + } catch (e) { + threw = true; + stderr = e.stderr?.toString() ?? ''; + } + expect(threw).toBe(true); + expect(stderr).toContain('no workspaces resolved from package.json'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('handoff frames a committed-but-unpushed change as NOT pushed, an abort as neutral', () => { + // Keyed on COMMITTED, not OUTCOME. When the agent committed but nothing was + // pushed, the address-summary.md can read like a success and cite the + // now-discarded commit, so the handoff must say it was NOT pushed. An abort + // / pre-gate failure made no commit (COMMITTED unset) and must stay neutral, + // since there is no commit to call discarded. + const body = reviewAddressReportStep.match( + /if \[\[ -n "\$\{DETAIL_FILE\}" \]\]; then\n[\s\S]*?\n {14}fi/, + )?.[0]; + expect(body).toBeTruthy(); + const run = (committed) => { + const dir = mkdtempSync(join(tmpdir(), 'hoff-')); + try { + writeFileSync(join(dir, 'd.md'), 'Done. Single commit abc1234.\n'); + return execFileSync('bash', ['-c', body], { + env: { + ...process.env, + DETAIL_FILE: join(dir, 'd.md'), + COMMITTED: committed, + }, + encoding: 'utf8', + }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + const committed = run('true'); + expect(committed).toContain('This change was NOT pushed'); + // Do not assert the gate ran — a pre-gate failure.md abort also lands here. + expect(committed).not.toContain('did NOT pass the verification gate'); + expect(committed).not.toContain('What I found before stopping'); + // No commit (abort / pre-gate failure) keeps the neutral framing. + expect(run('')).toContain('What I found before stopping'); + expect(run('')).not.toContain('This change was NOT pushed'); + }); + + it('verify gate records committed=true only on a real diff (exit 1), not a git error (128)', () => { + // The handoff's "was NOT pushed" wording keys on this output; it is recorded + // at the top of the step, before any gate can exit. `git diff --quiet` exits + // 1 for a real diff but 128 on a bad ref — only 1 is a commit, so a git + // error must not be misreported as a discarded commit. Drive the extracted + // snippet with a stubbed git whose exit is scripted. + const snippet = verificationGateSteps[1].match( + /committed_rc=0[\s\S]*?committed=true[^\n]*\n\s*fi/, + )?.[0]; + expect(snippet).toBeTruthy(); + const run = (gitDiffExit) => { + const dir = mkdtempSync(join(tmpdir(), 'committed-')); + const out = join(dir, 'gh_output'); + const bin = join(dir, 'bin'); + writeFileSync(out, ''); + mkdirSync(bin); + // Stub git: `diff --quiet` exits 0 (no commit) or 1 (branch changed). + writeFileSync( + join(bin, 'git'), + `#!/usr/bin/env bash\nexit ${gitDiffExit}\n`, + ); + chmodSync(join(bin, 'git'), 0o755); + try { + execFileSync('bash', ['-c', `export BRANCH=feat\n${snippet}`], { + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + GITHUB_OUTPUT: out, + }, + encoding: 'utf8', + }); + } catch { + // The snippet's own `if` swallows git's exit; no throw expected. + } + const result = readFileSync(out, 'utf8'); + rmSync(dir, { recursive: true, force: true }); + return result; + }; + // git diff --quiet exits 1 => branch has a commit => committed=true. + expect(run(1)).toContain('committed=true'); + // exits 0 => no new commit => nothing recorded. + expect(run(0)).not.toContain('committed=true'); + // exits 128 => bad ref / git error => NOT treated as a commit. + expect(run(128)).not.toContain('committed=true'); + // Neither gate carries an EXIT trap: the wording keys on committed, so an + // outcome=failed-forcing trap (which would also fire on pre-commit + // failures) must not creep back into either verify step. + for (const gate of verificationGateSteps) { + expect(gate).not.toMatch(/\btrap\b/); + } + }); + it('still runs review verification reporting when the agent step fails', () => { expect(verificationGateSteps).toHaveLength(2); const reviewVerificationGateStep = verificationGateSteps[1];