From e24c49b91bbd11b8776ae99917bccd86b724c6be Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:53:21 -0300 Subject: [PATCH 1/9] test: require exact toolchains and discover fixture runtimes The test-contract check accepted a registry row that left out a toolchain the suite needs: repo-plan-cli lists bun-1 because the fake gh runs on Bun, but removing bun-1 still passed, because the check looked only at the runners in scripts/ci.mjs and only asked that each of them be listed. The check now also discovers the #! interpreter of each executable fixture under scripts/tests/fixtures/ that a suite's files name, and it requires each row's toolchains to equal the discovered set: a missing or an extra toolchain fails with the row id and the evidence. Refs #102 --- scripts/AGENTS.md | 5 ++ scripts/tests/test-contracts.mjs | 113 +++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 0375883a..d35fe443 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -116,6 +116,11 @@ repo-wide guards section. When one suite has multiple gate-selected cases, its selector joins the sorted case values with `|`, and `expected_min` records the number of distinct invocations. +Each row's `toolchains` must equal the toolchains of its discovered runners: the +runners that `scripts/ci.mjs` uses for the suite, plus the `#!` interpreter of each +executable fixture under `scripts/tests/fixtures/` that the suite's files name. A +missing or an extra toolchain fails with the row id and the evidence. + ### Host-derived resource envelope `scripts/lib/host-resources.mjs` sizes the gate to the host that actually runs it. diff --git a/scripts/tests/test-contracts.mjs b/scripts/tests/test-contracts.mjs index 94dee445..aa786535 100755 --- a/scripts/tests/test-contracts.mjs +++ b/scripts/tests/test-contracts.mjs @@ -431,6 +431,80 @@ function bunToolchain(packageJson) { return `bun-${major}`; } +const FIXTURE_ROOT = 'scripts/tests/fixtures'; + +// Sorted repository-relative POSIX paths, so that evidence reads the same on each platform. +function filesUnder(relativeDirectory) { + const absolute = path.join(REPO, relativeDirectory); + if (!fs.existsSync(absolute) || !fs.statSync(absolute).isDirectory()) return []; + const files = []; + for (const entry of fs.readdirSync(absolute, { withFileTypes: true })) { + const relative = `${relativeDirectory}/${entry.name}`; + if (entry.isDirectory()) files.push(...filesUnder(relative)); + else if (entry.isFile()) files.push(relative); + } + return files.sort(); +} + +function shebangInterpreter(source) { + const line = /^#!(?[^\n]*)/u.exec(source)?.groups.line; + if (line === undefined) return null; + const [program, ...words] = line.trim().split(/\s+/u); + // `env` finds the interpreter on PATH, and its options and NAME=value words come before the interpreter. + const interpreter = + path.posix.basename(program) === 'env' + ? words.find((word) => !word.startsWith('-') && !word.includes('=')) + : program; + return path.posix.basename(interpreter ?? ''); +} + +// A suite can run a fixture program, as the plan CLI cases run the fake gh from PATH. The suite then needs the +// program's interpreter where it runs, so that interpreter is a runner of each suite that names the fixture. +function fixturePrograms() { + const programs = []; + for (const fixture of filesUnder(FIXTURE_ROOT)) { + const absolute = path.join(REPO, fixture); + // Only an executable file runs through its #! line; a suite reads each other fixture as data. + if ((fs.statSync(absolute).mode & 0o111) === 0) continue; + const interpreter = shebangInterpreter(fs.readFileSync(absolute, 'utf8')); + if (interpreter === null) continue; + assert.ok(interpreter.length > 0, `${fixture} has a #! line with no interpreter; name the program that runs it`); + // A suite names the top folder of a fixture, which it can put on PATH, or a file at the fixture root. The match + // starts at `fixtures/`, because a suite can name the path from the repository root or from its own folder. + const [top] = fixture.slice(FIXTURE_ROOT.length + 1).split('/'); + const escaped = `fixtures/${top}`.replace(/[\\^$.*+?()[\]{}|/]/gu, '\\$&'); + programs.push({ + fixture, + folder: `${FIXTURE_ROOT}/${top}`, + interpreter, + reference: new RegExp(String.raw`(? /\.[cm]?js$/u.test(file))]; +} + +function fixtureRunners(suite, programs) { + const runners = []; + for (const file of suiteFiles(suite)) { + const source = fs.readFileSync(path.join(REPO, file), 'utf8'); + for (const { fixture, folder, interpreter, reference } of programs) { + if (!reference.test(source)) continue; + runners.push({ + runner: interpreter, + source: `${file} names ${folder}, where ${fixture} runs with ${interpreter}`, + }); + } + } + return runners; +} + const registry = readJson('scripts/config/test-contracts.json'); const packageJson = readJson('package.json'); const ciSource = fs.readFileSync(path.join(REPO, 'scripts/ci.mjs'), 'utf8'); @@ -458,7 +532,8 @@ const toolchainByRunner = new Map([ ['node', nodeToolchain()], ['bun', bunToolchain(packageJson)], ]); -const staleToolchains = []; +const programs = fixturePrograms(); +const toolchainDifferences = []; for (const contract of registry.contracts) { const evidence = descriptorById.get(contract.id); assert.ok(evidence, `${contract.id} has no discovered selection evidence`); @@ -472,13 +547,33 @@ for (const contract of registry.contracts) { ); assert.ok(fs.existsSync(path.join(REPO, contract.owner.suite)), `${contract.id} owner suite does not exist`); assert.ok(evidence.runners.length > 0, `${contract.id} has no discovered runner`); - for (const runner of evidence.runners) { + const runners = [ + ...evidence.runners.map((runner) => ({ + runner, + source: `${evidence.runnerSource} runs ${evidence.suite} with ${runner}`, + })), + ...fixtureRunners(evidence.suite, programs), + ]; + const sourcesByToolchain = new Map(); + for (const { runner, source } of runners) { const toolchain = toolchainByRunner.get(runner); - assert.ok(toolchain, `${contract.id} runs with ${runner}, which has no known toolchain token`); + assert.ok( + toolchain, + `${contract.id} runs with ${runner}, which has no known toolchain token (${source}); map ${runner} to its pinned toolchain in toolchainByRunner`, + ); + const sources = sourcesByToolchain.get(toolchain) ?? new Set(); + sources.add(source); + sourcesByToolchain.set(toolchain, sources); + } + // An exact list, so that a toolchain which a runner needs cannot go missing, and one which nothing needs cannot stay. + for (const [toolchain, sources] of [...sourcesByToolchain].sort(([left], [right]) => left.localeCompare(right))) { if (!contract.toolchains.includes(toolchain)) { - staleToolchains.push( - `${contract.id}.toolchains must list ${toolchain}: ${evidence.runnerSource} runs ${evidence.suite} with ${runner}`, - ); + toolchainDifferences.push(`${contract.id}.toolchains must list ${toolchain}: ${[...sources].join('; ')}`); + } + } + for (const toolchain of contract.toolchains) { + if (!sourcesByToolchain.has(toolchain)) { + toolchainDifferences.push(`${contract.id}.toolchains lists ${toolchain}, which no discovered runner needs`); } } selected.add(contract.id); @@ -488,6 +583,10 @@ assert.ok(selected.size > 0, 'selected set must not be empty'); for (const id of selected) assert.ok(registered.has(id), `selected contract is not registered: ${id}`); assertSetEqual(executed, selected, 'executed and selected sets differ'); // Collected rather than asserted per row so one run names every stale contract. -assert.deepEqual(staleToolchains, [], `contract toolchains omit the gate runner:\n${staleToolchains.join('\n')}`); +assert.deepEqual( + toolchainDifferences, + [], + `contract toolchains differ from the discovered runners; make each row of scripts/config/test-contracts.json list exactly the toolchains that its runners need:\n${toolchainDifferences.join('\n')}`, +); console.log(`PASS test_contracts contracts=${registered.size} discovered=${discovered.size} selected=${selected.size}`); From 2fe28f52ce0b0294cd369e86b4de2a8f8a600d67 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:53:21 -0300 Subject: [PATCH 2/9] test: return only the selected pull request fields from the fake gh The fake gh built every pull request node with fixed fields, whether or not the GraphQL query selected them. A helper query that dropped mergedAt and baseRefName still passed all 66 plan CLI tests, although GitHub would then omit both fields and the helper would reject every closing pull request as malformed. Closers, associated pull requests, and Repository.pullRequest now return only the selected fields, with one node builder for all three. Refs #102 --- scripts/tests/fixtures/gh-stub/gh | 119 ++++++++++++++++++++++-------- 1 file changed, 88 insertions(+), 31 deletions(-) diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index 9564e057..d602c0cb 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -231,30 +231,79 @@ function graphqlArguments(text) { return args; } +// Each `(...)` of a query, with its alias, its arguments, and the text of its selection set ('' if it has none). function graphqlSelections(query, field) { const selections = []; - const pattern = new RegExp(`(?:(\\w+)\\s*:\\s*)?${field}\\s*\\(([^)]*)\\)`, 'g'); + const pattern = new RegExp(`(?:(\\w+)\\s*:\\s*)?${field}\\s*\\(([^)]*)\\)(\\s*\\{)?`, 'g'); for (const match of query.matchAll(pattern)) { - selections.push({ alias: match[1] ?? field, args: graphqlArguments(match[2]) }); + selections.push({ + alias: match[1] ?? field, + args: graphqlArguments(match[2]), + fields: match[3] ? selectionSetAt(query, match.index + match[0].length) : '', + }); } return selections; } -// The fields that `... on {...}` selects at its own level, without the text of nested selections. -function ownFields(query, type) { - const opened = new RegExp(`\\.\\.\\.\\s*on\\s+${type}\\s*\\{`).exec(query); - if (!opened) return ''; - let own = ''; +// The text of a selection set from `start`, the index after its opening brace, to its matching closing brace. +function selectionSetAt(text, start) { let depth = 1; - for (let index = opened.index + opened[0].length; index < query.length && depth > 0; index += 1) { - const char = query[index]; + for (let index = start; index < text.length; index += 1) { + if (text[index] === '{') depth += 1; + else if (text[index] === '}' && (depth -= 1) === 0) return text.slice(start, index); + } + return text.slice(start); +} + +// The text of the selection set that the first match of `opener` opens; `opener` ends with the opening brace. +function selectionSet(text, opener) { + const opened = opener.exec(text); + return opened ? selectionSetAt(text, opened.index + opened[0].length) : ''; +} + +// The selection set of the first inline fragment `... on {...}` in `text`. +function fragmentSet(text, type) { + return selectionSet(text, new RegExp(`\\.\\.\\.\\s*on\\s+${type}\\s*\\{`)); +} + +// The text that a selection set holds at its own level, without the text of nested selections. +function ownLevel(selection) { + let own = ''; + let depth = 0; + for (const char of selection) { if (char === '{') depth += 1; else if (char === '}') depth -= 1; - else if (depth === 1) own += char; + else if (depth === 0) own += char; } return own; } +// The fields that `... on {...}` selects at its own level, without the text of nested selections. +function ownFields(query, type) { + return ownLevel(fragmentSet(query, type)); +} + +// `object` as GitHub returns it for a selection set: only the fields that the set names at its own level, and each +// object field only with the fields of its nested set. A fixture field that the query does not select must not reach +// the helper, or a test passes for a query that GitHub would answer without that field. +function selectFields(object, selection) { + const own = ownLevel(selection); + const selected = {}; + for (const [name, value] of Object.entries(object)) { + if (!new RegExp(`\\b${name}\\b`).test(own)) continue; + if (value === null || typeof value !== 'object') { + selected[name] = value; + continue; + } + const nested = selectionSet(selection, new RegExp(`\\b${name}\\s*\\{`)); + // GitHub refuses a query that selects an object field without its subfields. + if (!nested.trim()) + die(`Field must have selections (field '${name}' has no selections. Did you mean '${name} { ... }'?)`); + selected[name] = selectFields(value, nested); + } + return selected; +} + function graphqlPage(nodes, selection, variables, allowed) { for (const name of selection.args.keys()) { if (!allowed.includes(name)) die(`Unknown GraphQL argument: "${name}"`); @@ -596,28 +645,38 @@ if (group === 'api' && command === 'graphql') { (reference.repository?.owner?.login && reference.repository?.name ? `${reference.repository.owner.login}/${reference.repository.name}` : state.repo.nameWithOwner); - const pullRequestNode = (reference) => { + // A pull request node, for a closer, an associated pull request, or Repository.pullRequest, with only the fields + // that `selection`, the selection at the node's level, names. + const pullRequestNode = (reference, selection) => { const repo = referenceRepo(reference); const pullRequest = (state.prs ?? []).find( (entry) => entry.number === reference.number && entry.repository === repo, ); - return { - number: reference.number, - url: pullRequest?.url ?? reference.url ?? `https://github.com/${repo}/pull/${reference.number}`, - body: pullRequest?.body ?? reference.body ?? null, - state: pullRequest?.state ?? reference.state ?? (pullRequest?.mergedAt || reference.mergedAt ? 'MERGED' : 'OPEN'), - mergedAt: pullRequest?.mergedAt ?? reference.mergedAt ?? null, - baseRefName: pullRequest?.baseRefName ?? reference.baseRefName ?? null, - repository: { nameWithOwner: repo }, - }; + const prState = + pullRequest?.state ?? reference.state ?? (pullRequest?.mergedAt || reference.mergedAt ? 'MERGED' : 'OPEN'); + return selectFields( + { + __typename: 'PullRequest', + number: reference.number, + url: pullRequest?.url ?? reference.url ?? `https://github.com/${repo}/pull/${reference.number}`, + body: pullRequest?.body ?? reference.body ?? null, + state: prState, + merged: prState === 'MERGED', + mergedAt: pullRequest?.mergedAt ?? reference.mergedAt ?? null, + baseRefName: pullRequest?.baseRefName ?? reference.baseRefName ?? null, + repository: { nameWithOwner: repo }, + }, + selection, + ); }; const commitAssociations = (oid, selection) => { const commit = (state.commits ?? []).find( (entry) => entry.oid === oid && (entry.repository ?? state.repo.nameWithOwner) === state.repo.nameWithOwner, ); if (!commit) die(`commit not found: ${state.repo.nameWithOwner}@${oid}`); + const nodeFields = selectionSet(selection.fields, /\bnodes\s*\{/); return graphqlPage( - (commit.associatedPullRequests ?? []).map(pullRequestNode), + (commit.associatedPullRequests ?? []).map((reference) => pullRequestNode(reference, nodeFields)), selection, variables, ['first', 'after'], @@ -686,7 +745,12 @@ if (group === 'api' && command === 'graphql') { const closer = latest?.closer; let renderedCloser = closer; if (closer?.__typename === 'PullRequest') { - renderedCloser = { __typename: 'PullRequest', ...pullRequestNode(closer) }; + // GitHub merges the fields that the closer selects at its own level with those of its `... on PullRequest`. + const closerFields = selectionSet(query, /\bcloser\s*\{/); + renderedCloser = pullRequestNode( + closer, + `${ownLevel(closerFields)} ${fragmentSet(closerFields, 'PullRequest')}`, + ); } if (closer?.__typename === 'Commit') { const association = graphqlSelections(query, 'associatedPullRequests'); @@ -774,15 +838,8 @@ if (group === 'api' && command === 'graphql') { }, ]); } - const node = { - number, - url: pullRequest.url ?? `https://github.com/${state.repo.nameWithOwner}/pull/${number}`, - state: pullRequest.state, - merged: pullRequest.state === 'MERGED', - mergedAt: pullRequest.mergedAt ?? null, - baseRefName: pullRequest.baseRefName ?? null, - }; - for (const page of graphqlSelections(query, 'closingIssuesReferences')) { + const node = pullRequestNode({ number }, selection.fields); + for (const page of graphqlSelections(selection.fields, 'closingIssuesReferences')) { const references = (pullRequest.closingIssuesReferences ?? []).map((issueNumber) => ({ number: issueNumber, repository: { nameWithOwner: state.repo.nameWithOwner }, From f0a82ab78e981deda1792a2061da833462e934f7 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:53:21 -0300 Subject: [PATCH 3/9] test: cover the settle-wait refusal and check reopen results only No test reached "plan # changed during the settle wait; run reopen again". Two tests now reach its two triggers without a helper change: a later close by the same pull request during the wait, and a close reason that changes to not planned. Each asserts the refusal, one settle wait, no write, and the stored issue. The commit-close test no longer inspects the request text for a claim mutation; the stored issue and the single write already show that an owned plan gets no claim. Refs #102 --- scripts/tests/plan-cli/reopen.test.mjs | 44 ++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/scripts/tests/plan-cli/reopen.test.mjs b/scripts/tests/plan-cli/reopen.test.mjs index 587ae4df..948cce35 100644 --- a/scripts/tests/plan-cli/reopen.test.mjs +++ b/scripts/tests/plan-cli/reopen.test.mjs @@ -16,6 +16,10 @@ function stuckRefusal(number) { return `GitHub left plan #${number} closed with reason REOPENED and refuses every state change; ask GitHub Support to repair it`; } +function settleRefusal(number) { + return `plan #${number} changed during the settle wait; run reopen again`; +} + // Every reopen loads the skip-waits hook, so its pauses and its settle wait return at once and get recorded. function reopen({ runWith }, plan) { return runWith(['--import', SKIP_WAITS], 'reopen', String(plan.number), '--pr', String(plan.pr)); @@ -64,12 +68,7 @@ test('reopen accepts the close by a commit of the merged pull request and leaves assignees: ['plan-agent'], comments: [{ author: 'plan-agent', body: reopenComment(plan.pr) }], }); - const writes = context.writes(); - assert.equal(writes.length, 1); - assert.ok( - !writes[0].some((argument) => argument.includes('addAssigneesToAssignable')), - 'an owned plan needs no claim', - ); + assert.equal(context.writes().length, 1); }); test('reopen writes nothing when the merged pull request does not close the plan', (t) => { @@ -221,6 +220,39 @@ test('reopen prints the open status and writes nothing when someone reopens the assert.deepEqual(context.writes(), []); }); +test('reopen refuses a plan whose latest close changes during the settle wait', (t) => { + const context = planCliContext(t); + const plan = context.partialMerge('closed again during the settle wait'); + // A later close by the same pull request replaces the latest close, so only the time of the close differs. + const laterClose = '2026-08-21T00:00:09Z'; + context.schedule( + plan.number, + { afterReads: 0, ...closedBy(plan.closer) }, + { afterReads: 1, ...closedBy(plan.closer, laterClose) }, + ); + refuse(reopen(context, plan), settleRefusal(plan.number)); + assert.deepEqual(context.waits(), [SETTLE_WAIT]); + assert.deepEqual(stored(context, plan.number), { ...ONGOING, state: 'CLOSED', stateReason: 'COMPLETED' }); + assert.equal(context.issue(plan.number).timelineItems.at(-1).createdAt, laterClose); + assert.deepEqual(context.writes(), []); +}); + +test('reopen refuses a plan that someone closes as not planned during the settle wait', (t) => { + const context = planCliContext(t); + const plan = context.partialMerge('not planned during the settle wait'); + // The latest close stays the same. A closed plan takes its status from the close reason, so the plan reads + // retired instead of finished. + context.schedule( + plan.number, + { afterReads: 0, ...closedBy(plan.closer) }, + { afterReads: 1, stateReason: 'NOT_PLANNED' }, + ); + refuse(reopen(context, plan), settleRefusal(plan.number)); + assert.deepEqual(context.waits(), [SETTLE_WAIT]); + assert.deepEqual(stored(context, plan.number), { ...ONGOING, state: 'CLOSED', stateReason: 'NOT_PLANNED' }); + assert.deepEqual(context.writes(), []); +}); + test('reopen refuses after 34 reads when GitHub never closes the plan for the merged pull request', (t) => { const context = planCliContext(t); const plan = context.partialMerge('never closed'); From 33c45f33727a8cc562a0ecfc2545aa6637898187 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:53:21 -0300 Subject: [PATCH 4/9] fix: check the Report section of every skill in the skills guard The Report section rule gave a shell command as its check, and that command printed one line with the full count, which the rule calls a pass, also when every skill lacked the section. No gate ran it. The skills guard, which the gate runs for each plugin, now fails when a SKILL.md does not end with the text of scripts/config/report-section.md byte for byte, and it names each file and whether the section is missing or differs. The docks skills node and the validator table name the guard as the check. Refs #102 --- plugins/docks/skills/AGENTS.md | 2 +- scripts/AGENTS.md | 2 +- scripts/config/report-section.md | 3 ++ scripts/skills/guard.mjs | 50 ++++++++++++++++++++++++++++++-- 4 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 scripts/config/report-section.md diff --git a/plugins/docks/skills/AGENTS.md b/plugins/docks/skills/AGENTS.md index ef5ae1d8..d38d43d7 100644 --- a/plugins/docks/skills/AGENTS.md +++ b/plugins/docks/skills/AGENTS.md @@ -104,7 +104,7 @@ rule and the command that finds every copy. ## Report section (every shipped skill) -Every shipped skill of the docks and plan-lifecycle plugins ends with one byte-identical `## Report a problem with this skill` section, so an agent that uses a skill sees where and how to report a problem with it. The section points to the docks `report-skill-issue` skill. For a runtime without docks, it allows only a draft for the user, because the publication safeguards live in that skill. A new skill copies the section unchanged; a change to the section changes every copy in the same commit. Verify: `for f in plugins/docks/skills/*/*/SKILL.md plugins/plan-lifecycle/skills/*/*/SKILL.md; do sed -n '/^## Report a problem with this skill$/,$p' "$f" | sha256sum; done | sort | uniq -c` must print one line whose count equals the number of shipped skills. +Every shipped skill of the docks and plan-lifecycle plugins ends with one byte-identical `## Report a problem with this skill` section, so an agent that uses a skill sees where and how to report a problem with it. The section points to the docks `report-skill-issue` skill. For a runtime without docks, it allows only a draft for the user, because the publication safeguards live in that skill. A new skill copies the section unchanged. The skills guard checks every copy against `scripts/config/report-section.md` (verify: `bun scripts/skills/guard.mjs plugins/docks/skills` and `bun scripts/skills/guard.mjs plugins/plan-lifecycle/skills`); a change to the section changes that file and every copy in the same commit. ## Cross-tool wording (Claude Code + Codex) diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index d35fe443..b68b066c 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -48,7 +48,7 @@ Plugin behavior stays registry-driven: extend descriptor capabilities rather tha | Script | Purpose | Floor | |---|---|---| | `ci.mjs` | the authoritative gate entry point — without `--plugin`, runs repo-wide checks once and gates every registry entry; `--plugin ` skips repo-wide sections and runs only that plugin's owned author checks, shell-hook lint, and `gatePlugin` validation (manifest/version coherence, `claude plugin validate`, Codex parity, and the checks below) | — | -| `skills/guard.mjs` | runs the skill frontmatter validators (codex + claude via `lib/validate-skills.mjs`) + `codex-facts.mjs` + `refs-guard.mjs` | pass/fail | +| `skills/guard.mjs` | runs the skill frontmatter validators (codex + claude via `lib/validate-skills.mjs`) + `codex-facts.mjs` + `refs-guard.mjs`, then checks that every `SKILL.md` ends with the Report section of `config/report-section.md`, byte for byte | pass/fail | | `lib/validate-skills.mjs` | skill frontmatter per runtime — name/description, 1024-char cap, no `#` truncation, CSO `Use when` prefix, `user-invocable`, `metadata.updated`, `references/` one level deep | pass/fail | | `skills/codex-facts.mjs` | pins canonical Codex model ids / `sandbox_mode` / `model_reasoning_effort` + the `agents.max_depth` fact in the skill-agent-pipeline refs (self-skips when absent) | pass/fail | | `skills/refs-guard.mjs` | reference hygiene: broken local `references/`/`assets/` links, orphan reference files, missing `## Contents` TOC on `references/*.md` > 100 lines with ≥3 doc-level headings | pass/fail | diff --git a/scripts/config/report-section.md b/scripts/config/report-section.md new file mode 100644 index 00000000..20ff13b0 --- /dev/null +++ b/scripts/config/report-section.md @@ -0,0 +1,3 @@ +## Report a problem with this skill + +When this skill gives a wrong, outdated, or unsafe instruction, or a script that it ships fails, report the problem in the public DocksDocks/docks repository. Use the docks `report-skill-issue` skill for the report. If that skill is not installed, do not file anything: give the user a draft instead. The draft names this skill and its plugin version, quotes the instruction, and states what happened and what you expected. Leave out secrets and private names. diff --git a/scripts/skills/guard.mjs b/scripts/skills/guard.mjs index 9366e7cb..6ecb9e4f 100644 --- a/scripts/skills/guard.mjs +++ b/scripts/skills/guard.mjs @@ -1,12 +1,17 @@ #!/usr/bin/env bun // guard.mjs — run the combined skill frontmatter validator, the Codex-fact -// drift guard, and the reference-hygiene guard. Usage: guard.mjs [skills-dir] +// drift guard, and the reference-hygiene guard, then check that every skill +// ends with the canonical Report section. Usage: guard.mjs [skills-dir] import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; import path from 'node:path'; +import { findSkillFiles } from '../lib/skills-walk.mjs'; const SCRIPT_DIR = path.dirname(new URL(import.meta.url).pathname); const REPO_DIR = path.resolve(SCRIPT_DIR, '../..'); const DIR = process.argv[2] || ''; +// One canonical copy, so that the copies in all plugins cannot drift apart unseen. +const REPORT_SECTION = 'scripts/config/report-section.md'; function run(args) { const result = spawnSync('bun', args, { stdio: 'inherit' }); @@ -26,10 +31,51 @@ function run(args) { if (result.status !== 0) process.exit(result.status); } +// Every shipped skill ends with the same Report section, so that an agent that uses any skill +// can find how to report a problem with it. Read the files as latin1 (one character per byte), +// so that the comparison is byte for byte, also for line endings and trailing newlines. +function reportSectionFailures(skillsRoot) { + const canonicalFile = path.join(REPO_DIR, REPORT_SECTION); + let canonical; + try { + canonical = fs.readFileSync(canonicalFile, 'latin1'); + } catch (error) { + console.error(`FAIL: cannot read the canonical Report section ${canonicalFile}: ${error.message}`); + process.exit(2); + } + const heading = canonical.split('\n', 1)[0]; + if (!heading.startsWith('## ')) { + console.error(`FAIL: ${REPORT_SECTION}: the first line must be the \`## \` heading of the Report section`); + process.exit(2); + } + const escapedHeading = heading.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const headingLine = new RegExp(`^${escapedHeading}$`, 'gm'); + const failures = []; + for (const file of findSkillFiles(skillsRoot)) { + const content = fs.readFileSync(file, 'latin1'); + // The last heading starts the section: a skill can quote the heading in its body. + const start = [...content.matchAll(headingLine)].at(-1)?.index; + const shown = path.relative(process.cwd(), file) || file; + if (start === undefined) { + failures.push(`${shown}: missing the Report section; end the file with the text of ${REPORT_SECTION}`); + } else if (content.slice(start) !== canonical) { + failures.push(`${shown}: the Report section differs from ${REPORT_SECTION}; copy that file's text over it`); + } + } + return failures; +} + const lib = path.join(REPO_DIR, 'scripts/lib/validate-skills.mjs'); const target = DIR || path.join(REPO_DIR, 'plugins/docks/skills'); run([lib, '--runtime', 'all', target]); run([path.join(REPO_DIR, 'scripts/skills/codex-facts.mjs')]); run([path.join(REPO_DIR, 'scripts/skills/refs-guard.mjs'), ...(DIR ? [DIR] : [])]); -console.log('Guard PASSED: skills match Codex and Claude conventions'); +const reportFailures = reportSectionFailures(target); +if (reportFailures.length > 0) { + for (const failure of reportFailures) console.error(`FAIL: ${failure}`); + console.error(`Guard FAILED: ${reportFailures.length} skill file(s) do not end with the canonical Report section`); + process.exit(1); +} + +console.log('Guard PASSED: skills match Codex and Claude conventions and end with the canonical Report section'); From 8d104d9359c61c63820be65274df48f66bf281ec Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:08:45 -0300 Subject: [PATCH 5/9] test: cover the rejection paths of the registry and Report section checks Code review round 1 of #102 found that no committed test ran the new rejection paths: a change that disabled them would keep CI green and bring back D3 or D7. Author-tooling now runs the registry check on copies that lack bun-1 for repo-plan-cli and that list an unneeded toolchain, and runs the skills guard on a copy of the docks skills with one section removed, one byte changed, and every section removed. The registry check takes `--registry ` for those copies; without it, it reads the committed registry as before. Refs #102 --- scripts/AGENTS.md | 4 +- scripts/tests/author-tooling.mjs | 127 +++++++++++++++++++++++++++++++ scripts/tests/test-contracts.mjs | 8 +- 3 files changed, 136 insertions(+), 3 deletions(-) diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index b68b066c..27d45f93 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -65,8 +65,8 @@ Plugin behavior stays registry-driven: extend descriptor capabilities rather tha | `tests/idempotency.mjs` | content-hash determinism + every stored hash in sync | pass/fail | | `tests/plan-cli.mjs` | runs the plan-CLI case files in `tests/plan-cli/` in parallel through Node's test runner against the shipped plan helper (`plan.mjs`) | pass/fail | | `tests/ci-observability.mjs` | validates command timing records, wall-time reconstruction, and CI host metadata | pass/fail | -| `tests/test-contracts.mjs` | validates the closed test-contract registry and its discovered, registered, selected, and executed sets | pass/fail | -| `tests/author-tooling.mjs` | author-tool contracts: Biome rejects a syntax defect, combined skill validation, tree/guard operational failures, skills-guard spawn failure; this table lists every `scripts/**/*.mjs` outside `lib/`, `tests/unit/`, `tests/plan-cli/`, and the non-validator entry points `release.mjs`/`ci-target.mjs`/`capture-tdd-red.mjs`, and every first-column `.mjs` path resolves from `scripts/` or the repo root | pass/fail | +| `tests/test-contracts.mjs` | validates the closed test-contract registry and its discovered, registered, selected, and executed sets; `--registry ` checks another registry file (author-tooling uses it for the rejection cases) | pass/fail | +| `tests/author-tooling.mjs` | author-tool contracts: Biome rejects a syntax defect, combined skill validation, tree/guard operational failures, skills-guard spawn failure, registry toolchain rejections, and skills-guard Report section rejections; this table lists every `scripts/**/*.mjs` outside `lib/`, `tests/unit/`, `tests/plan-cli/`, and the non-validator entry points `release.mjs`/`ci-target.mjs`/`capture-tdd-red.mjs`, and every first-column `.mjs` path resolves from `scripts/` or the repo root | pass/fail | | `tests/ci-plugin-targeting.mjs` | CI targeting and release contracts: shard selection, `ci.yml` trigger block, release module and dry-run safety (`--unit` in the gate) | pass/fail | | shellcheck (target-selected) | `-S warning` over selected plugins' `hooks/*.sh`, via `shellHooks(p)`; a full invocation selects every plugin | pass/warn | diff --git a/scripts/tests/author-tooling.mjs b/scripts/tests/author-tooling.mjs index 7d084e3c..58d947d3 100755 --- a/scripts/tests/author-tooling.mjs +++ b/scripts/tests/author-tooling.mjs @@ -11,6 +11,8 @@ const BIOME = path.join(ROOT, 'node_modules', '.bin', process.platform === 'win3 const VALIDATE_SKILLS = path.join(ROOT, 'scripts/lib/validate-skills.mjs'); const TREE_GUARD = path.join(ROOT, 'scripts/tree/guard.mjs'); const SKILLS_GUARD = path.join(ROOT, 'scripts/skills/guard.mjs'); +const TEST_CONTRACTS = path.join(ROOT, 'scripts/tests/test-contracts.mjs'); +const REPORT_SECTION = path.join(ROOT, 'scripts/config/report-section.md'); function runNode(script, args, options = {}) { return spawnSync(process.execPath, [script, ...args], { @@ -26,6 +28,10 @@ function assertStarted(result) { assert.equal(result.signal, null); } +function escapeRegExp(text) { + return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + function testBiomeRejectsSyntaxDefect() { const fixtureRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'docks-author-tooling-biome-')); const fixture = path.join(fixtureRoot, 'broken-syntax.mjs'); @@ -250,9 +256,130 @@ function testSkillsGuardSpawnFailure() { ); } +function runTestContracts(fixtureRoot, name, registry) { + const file = path.join(fixtureRoot, name); + fs.writeFileSync(file, `${JSON.stringify(registry, null, 2)}\n`); + return runNode(TEST_CONTRACTS, ['--registry', file]); +} + +// The registry check must reject a row that does not list a toolchain that its suite needs, and a row that lists a +// toolchain that nothing needs. Each negative case changes one row in a copy of the registry. +function testTestContractToolchains() { + const registry = JSON.parse(fs.readFileSync(path.join(ROOT, 'scripts/config/test-contracts.json'), 'utf8')); + const passed = runNode(TEST_CONTRACTS, []); + assertStarted(passed); + assert.equal(passed.status, 0, passed.stderr); + assert.match(passed.stdout, /^PASS test_contracts /); + + // The plan CLI suite runs with Node, and its cases run the fake gh, which runs with Bun. + const planCli = registry.contracts.find(({ id }) => id === 'repo-plan-cli'); + assert.ok(planCli, 'the registry must hold the repo-plan-cli row'); + const bunToken = planCli.toolchains.find((token) => token.startsWith('bun-')); + const nodeToken = planCli.toolchains.find((token) => token.startsWith('node-')); + assert.ok(bunToken && nodeToken, `repo-plan-cli must list a Bun and a Node toolchain: ${planCli.toolchains}`); + const bunOnly = registry.contracts.find(({ toolchains }) => toolchains.length === 1 && toolchains[0] === bunToken); + assert.ok(bunOnly, `the registry must hold a row that lists only ${bunToken}`); + + const fixtureRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'docks-author-tooling-contracts-')); + try { + const lacksBun = structuredClone(registry); + const lacksBunRow = lacksBun.contracts.find(({ id }) => id === planCli.id); + lacksBunRow.toolchains = lacksBunRow.toolchains.filter((token) => token !== bunToken); + const missing = runTestContracts(fixtureRoot, 'lacks-bun.json', lacksBun); + assertStarted(missing); + assert.notEqual(missing.status, 0); + assert.match( + `${missing.stdout}\n${missing.stderr}`, + new RegExp( + `repo-plan-cli\\.toolchains must list ${escapeRegExp(bunToken)}: .*scripts/tests/fixtures/gh-stub/gh runs with bun`, + ), + ); + + const listsNode = structuredClone(registry); + const listsNodeRow = listsNode.contracts.find(({ id }) => id === bunOnly.id); + listsNodeRow.toolchains = [bunToken, nodeToken].sort(); + const extra = runTestContracts(fixtureRoot, 'lists-node.json', listsNode); + assertStarted(extra); + assert.notEqual(extra.status, 0); + assert.match( + `${extra.stdout}\n${extra.stderr}`, + new RegExp( + `${escapeRegExp(bunOnly.id)}\\.toolchains lists ${escapeRegExp(nodeToken)}, which no discovered runner needs`, + ), + ); + } finally { + fs.rmSync(fixtureRoot, { recursive: true, force: true }); + } +} + +// Every docks skill must end with the canonical Report section. The cases change a full copy of the docks skills: the +// copy passes the sub-guards that run before the Report check, so that each run gets to that check. +function testSkillsGuardReportSection() { + const canonical = fs.readFileSync(REPORT_SECTION, 'utf8'); + const fixtureRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'docks-author-tooling-report-')); + try { + const skillsRoot = path.join(fixtureRoot, 'skills'); + fs.cpSync(path.join(ROOT, 'plugins/docks/skills'), skillsRoot, { recursive: true }); + const skillFiles = fs + .readdirSync(skillsRoot, { recursive: true }) + .filter((relative) => path.basename(relative) === 'SKILL.md') + .sort(); + assert.ok(skillFiles.length >= 2, `the fixture needs two skills, found ${skillFiles.length}`); + const originals = new Map( + skillFiles.map((relative) => [relative, fs.readFileSync(path.join(skillsRoot, relative), 'utf8')]), + ); + for (const [relative, content] of originals) + assert.ok(content.endsWith(canonical), `${relative} must end with the canonical Report section`); + const write = (relative, content) => fs.writeFileSync(path.join(skillsRoot, relative), content); + const withoutSection = (relative) => originals.get(relative).slice(0, -canonical.length); + const failLine = (relative, reason) => new RegExp(`^FAIL: .*/${escapeRegExp(relative)}: ${reason}`, 'm'); + const failedCount = (count) => + new RegExp(`Guard FAILED: ${count} skill file\\(s\\) do not end with the canonical Report section\\n$`); + + const unchanged = runNode(SKILLS_GUARD, [skillsRoot]); + assertStarted(unchanged); + assert.equal(unchanged.status, 0, unchanged.stderr); + assert.match( + unchanged.stdout, + /Guard PASSED: skills match Codex and Claude conventions and end with the canonical Report section\n$/, + ); + + const [missingSkill, changedSkill] = skillFiles; + write(missingSkill, withoutSection(missingSkill)); + const missing = runNode(SKILLS_GUARD, [skillsRoot]); + assertStarted(missing); + assert.equal(missing.status, 1); + assert.match(missing.stderr, failLine(missingSkill, 'missing the Report section')); + assert.match(missing.stderr, failedCount(1)); + write(missingSkill, originals.get(missingSkill)); + + // The byte goes before the last byte of the file, which is in the section. + const changedOriginal = originals.get(changedSkill); + write(changedSkill, `${changedOriginal.slice(0, -1)}x${changedOriginal.slice(-1)}`); + const changed = runNode(SKILLS_GUARD, [skillsRoot]); + assertStarted(changed); + assert.equal(changed.status, 1); + assert.match(changed.stderr, failLine(changedSkill, 'the Report section differs from')); + assert.match(changed.stderr, failedCount(1)); + + // No skill has the section: a check that only looks for a file that differs finds nothing to report here. + for (const relative of skillFiles) write(relative, withoutSection(relative)); + const allMissing = runNode(SKILLS_GUARD, [skillsRoot]); + assertStarted(allMissing); + assert.equal(allMissing.status, 1); + for (const relative of skillFiles) + assert.match(allMissing.stderr, failLine(relative, 'missing the Report section')); + assert.match(allMissing.stderr, failedCount(skillFiles.length)); + } finally { + fs.rmSync(fixtureRoot, { recursive: true, force: true }); + } +} + testBiomeRejectsSyntaxDefect(); testCombinedSkillValidation(); testTreeGuardOperationalFailures(); testSkillsGuardSpawnFailure(); +testSkillsGuardReportSection(); +testTestContractToolchains(); testValidatorTable(); console.log('author tooling contracts passed'); diff --git a/scripts/tests/test-contracts.mjs b/scripts/tests/test-contracts.mjs index aa786535..13e10758 100755 --- a/scripts/tests/test-contracts.mjs +++ b/scripts/tests/test-contracts.mjs @@ -3,6 +3,7 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { parseArgs } from 'node:util'; import { parseDocument } from 'yaml'; import { PLUGINS } from '../lib/plugins.mjs'; @@ -505,7 +506,12 @@ function fixtureRunners(suite, programs) { return runners; } -const registry = readJson('scripts/config/test-contracts.json'); +// The gate checks the committed registry. A test can give `--registry ` to check a changed copy, so that it can +// show that each check rejects the defect that it covers. +const { values: options } = parseArgs({ + options: { registry: { type: 'string', default: 'scripts/config/test-contracts.json' } }, +}); +const registry = JSON.parse(fs.readFileSync(path.resolve(REPO, options.registry), 'utf8')); const packageJson = readJson('package.json'); const ciSource = fs.readFileSync(path.join(REPO, 'scripts/ci.mjs'), 'utf8'); validateRegistry(registry); From 24396e043aaa930537d31594c29a9903e9a4def7 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:08:45 -0300 Subject: [PATCH 6/9] test: read GraphQL aliases in the fake gh and test its field projection Code review round 1 of #102 found that the fake gh treated an alias as a field name, so `{mergedAt: url}` returned the stored mergedAt and an unrequested url, and that no test checked the projection itself. The fake gh now reads `alias: field` at each level, puts the stored value under the response key, and refuses one key for two fields. New tests call it directly and check that pull request nodes, closers, and associated pull requests hold only the selected keys, also with aliases. All six fail against the stub before #102, and the three alias tests fail against the stub before this commit. Refs #102 --- scripts/tests/fixtures/gh-stub/gh | 91 +++++++++++++++---- scripts/tests/plan-cli/gh-stub.test.mjs | 113 ++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 scripts/tests/plan-cli/gh-stub.test.mjs diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index d602c0cb..1c7c2b2c 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -283,23 +283,85 @@ function ownFields(query, type) { return ownLevel(fragmentSet(query, type)); } -// `object` as GitHub returns it for a selection set: only the fields that the set names at its own level, and each -// object field only with the fields of its nested set. A fixture field that the query does not select must not reach -// the helper, or a test passes for a query that GitHub would answer without that field. +// The fields that a selection set names at its own level, in query order. Each field has its response key (its alias, +// or else its name), its name, and the text of its nested selection set (null if it has none). An inline fragment +// `... on {...}` adds its fields only when is `typename`, as GitHub applies only the fragments that match +// the object. Commas are white space in GraphQL. Like `graphqlSelections`, this reads arguments up to the first `)`. +function selectionFields(selection, typename) { + const fields = []; + let index = 0; + const skipSpace = () => { + while (index < selection.length && /[\s,]/.test(selection[index])) index += 1; + }; + const readName = () => { + const match = /^[_A-Za-z]\w*/.exec(selection.slice(index)); + if (!match) die(`fake gh cannot read the GraphQL selection at: ${selection.slice(index)}`); + index += match[0].length; + skipSpace(); + return match[0]; + }; + const readSet = () => { + const set = selectionSetAt(selection, index + 1); + index += set.length + 2; + return set; + }; + for (skipSpace(); index < selection.length; skipSpace()) { + if (selection.startsWith('...', index)) { + index += 3; + skipSpace(); + const on = readName(); + const type = on === 'on' ? readName() : ''; + if (!type || selection[index] !== '{') + die(`fake gh reads only inline fragments '... on {...}': ${selection}`); + const set = readSet(); + if (type === typename) fields.push(...selectionFields(set, typename)); + continue; + } + const key = readName(); + let name = key; + if (selection[index] === ':') { + index += 1; + skipSpace(); + name = readName(); + } + if (selection[index] === '(') { + const end = selection.indexOf(')', index); + if (end < 0) die(`fake gh cannot read the GraphQL arguments of '${name}': ${selection}`); + index = end + 1; + skipSpace(); + } + fields.push({ key, name, nested: selection[index] === '{' ? readSet() : null }); + } + return fields; +} + +// `object` as GitHub returns it for a selection set: only the fields that the set names at its own level, each under +// its response key, and each object field only with the fields of its nested set. For `{landed: mergedAt}` GitHub +// returns `landed` with the value of `mergedAt`, and no `mergedAt`. A fixture field that the query does not select must +// not reach the helper, or a test passes for a query that GitHub would answer without that field. function selectFields(object, selection) { - const own = ownLevel(selection); + // GitHub merges the selections of one response key, and refuses a key that names two different fields. + const keys = new Map(); + for (const { key, name, nested } of selectionFields(selection, object.__typename)) { + const merged = keys.get(key); + if (merged && merged.name !== name) die(`Field '${key}' has a field conflict: ${merged.name} or ${name}?`); + if (merged) merged.sets.push(nested); + else keys.set(key, { name, sets: [nested] }); + } const selected = {}; - for (const [name, value] of Object.entries(object)) { - if (!new RegExp(`\\b${name}\\b`).test(own)) continue; + for (const [key, { name, sets }] of keys) { + // The caller adds the fields that the fixture object does not store, such as a connection. + if (!Object.hasOwn(object, name)) continue; + const value = object[name]; if (value === null || typeof value !== 'object') { - selected[name] = value; + selected[key] = value; continue; } - const nested = selectionSet(selection, new RegExp(`\\b${name}\\s*\\{`)); + const nested = sets.filter(Boolean).join(' '); // GitHub refuses a query that selects an object field without its subfields. if (!nested.trim()) die(`Field must have selections (field '${name}' has no selections. Did you mean '${name} { ... }'?)`); - selected[name] = selectFields(value, nested); + selected[key] = selectFields(value, nested); } return selected; } @@ -646,7 +708,7 @@ if (group === 'api' && command === 'graphql') { ? `${reference.repository.owner.login}/${reference.repository.name}` : state.repo.nameWithOwner); // A pull request node, for a closer, an associated pull request, or Repository.pullRequest, with only the fields - // that `selection`, the selection at the node's level, names. + // that `selection`, the selection set of the node, names, each under its response key. const pullRequestNode = (reference, selection) => { const repo = referenceRepo(reference); const pullRequest = (state.prs ?? []).find( @@ -745,12 +807,9 @@ if (group === 'api' && command === 'graphql') { const closer = latest?.closer; let renderedCloser = closer; if (closer?.__typename === 'PullRequest') { - // GitHub merges the fields that the closer selects at its own level with those of its `... on PullRequest`. - const closerFields = selectionSet(query, /\bcloser\s*\{/); - renderedCloser = pullRequestNode( - closer, - `${ownLevel(closerFields)} ${fragmentSet(closerFields, 'PullRequest')}`, - ); + // `selectFields` merges the fields that the closer selects at its own level with those of its + // `... on PullRequest`, and ignores its `... on Commit`. + renderedCloser = pullRequestNode(closer, selectionSet(query, /\bcloser\s*\{/)); } if (closer?.__typename === 'Commit') { const association = graphqlSelections(query, 'associatedPullRequests'); diff --git a/scripts/tests/plan-cli/gh-stub.test.mjs b/scripts/tests/plan-cli/gh-stub.test.mjs new file mode 100644 index 00000000..577da1ff --- /dev/null +++ b/scripts/tests/plan-cli/gh-stub.test.mjs @@ -0,0 +1,113 @@ +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import path from 'node:path'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { planCliContext } from './harness.mjs'; + +// These cases call the fake gh directly. A fixture field that the query does not select must not reach the +// helper, or a plan CLI test passes for a query that GitHub would answer without that field. +const GH_STUB = fileURLToPath(new URL('../fixtures/gh-stub/gh', import.meta.url)); +const PR = 7; +const PR_URL = `https://github.com/DocksDocks/fixture/pull/${PR}`; +const MERGED_AT = '2026-08-21T00:00:00Z'; +const OID = 'c0ffee0000000000000000000000000000000007'; + +// Run `gh api graphql` on the fake gh with the state file that `planCliContext` keeps in its scratch folder. +// A number variable goes with -F, as the helper sends it, and a string variable with -f. +function graphql({ scratch }, query, variables = {}) { + const args = ['api', 'graphql', '-f', `query=${query}`, '-f', 'owner=DocksDocks', '-f', 'name=fixture']; + for (const [key, value] of Object.entries(variables)) { + args.push(typeof value === 'number' ? '-F' : '-f', `${key}=${value}`); + } + return spawnSync(GH_STUB, args, { + cwd: scratch, + encoding: 'utf8', + env: { ...process.env, GH_STUB_STATE: path.join(scratch, 'gh-state.json') }, + }); +} + +function responseData(result) { + assert.equal(result.status, 0, result.stderr); + return JSON.parse(result.stdout).data; +} + +// A merged pull request that stores every field that the fake gh can return. +function seedPullRequest({ updateState }) { + updateState((state) => { + state.prs.push({ + number: PR, + url: PR_URL, + state: 'MERGED', + mergedAt: MERGED_AT, + baseRefName: 'main', + body: 'Closes #1', + repository: 'DocksDocks/fixture', + closingIssuesReferences: [1], + }); + }); +} + +function pullRequest(context, fields) { + const query = `query($owner:String!,$name:String!,$pr:Int!){repository(owner:$owner,name:$name){pullRequest(number:$pr){${fields}}}}`; + return graphql(context, query, { pr: PR }); +} + +test('the fake gh returns only the selected fields of Repository.pullRequest', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + const data = responseData(pullRequest(context, 'number url')); + assert.deepEqual(data.repository.pullRequest, { number: PR, url: PR_URL }); +}); + +test('the fake gh returns only the selected fields of a pull request closer', (t) => { + const context = planCliContext(t); + // The stored closer has a URL, a state, a merge time, a base branch, a body, and a repository. + const { number, closer } = context.closedPlan('closer fields'); + for (const [closerFields, expected] of [ + ['__typename ... on PullRequest{number} ... on Commit{oid}', { __typename: 'PullRequest', number: closer.number }], + ['... on PullRequest{number}', { number: closer.number }], + ]) { + const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){issue(number:$number){timelineItems(last:1,itemTypes:[CLOSED_EVENT]){nodes{... on ClosedEvent{closer{${closerFields}}}}}}}}`; + const data = responseData(graphql(context, query, { number })); + assert.deepEqual(data.repository.issue.timelineItems, { nodes: [{ closer: expected }] }, closerFields); + } +}); + +test('the fake gh returns only the selected fields of the pull requests associated with a commit', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + context.updateState((state) => { + state.commits = [{ oid: OID, associatedPullRequests: [{ number: PR }] }]; + }); + const query = `query($owner:String!,$name:String!,$oid:String!){repository(owner:$owner,name:$name){commit:object(expression:$oid){... on Commit{associatedPullRequests(first:100){nodes{number}}}}}}`; + const data = responseData(graphql(context, query, { oid: OID })); + assert.deepEqual(data.repository.commit.associatedPullRequests.nodes, [{ number: PR }]); +}); + +test('the fake gh returns an aliased pull request field only under its alias', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + // GitHub returns `landed` with the merge time and no `mergedAt`, also for an alias in a nested selection. + const data = responseData(pullRequest(context, 'landed: mergedAt number repo: repository{fullName: nameWithOwner}')); + assert.deepEqual(data.repository.pullRequest, { + landed: MERGED_AT, + number: PR, + repo: { fullName: 'DocksDocks/fixture' }, + }); +}); + +test('the fake gh returns the value of the selected field for an alias that names another field', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + const data = responseData(pullRequest(context, 'mergedAt: url')); + assert.deepEqual(data.repository.pullRequest, { mergedAt: PR_URL }); +}); + +test('the fake gh refuses a response key that names two different fields', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + const result = pullRequest(context, 'number: url number'); + assert.equal(result.status, 1); + assert.equal(result.stderr, "Field 'number' has a field conflict: url or number?\n"); +}); From b6ebe1676542851ae601b35a9450d9b7d3c1da8c Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:21:21 -0300 Subject: [PATCH 7/9] test: project each fake gh node from its own place in the query Code review round 2 of #102 found that the fake gh took a node's selection from the first matching `name{` in the whole query, so two aliased timeline connections gave both closers the first selection and `nodes{number} nodes{url}` lost the second one; and that a pull request's nested repository had no `__typename`, so an inline fragment on it returned nothing. The fake gh now builds each object and keeps only what its own field selects, merging repeated selections of one response key at every level, and the nested repository carries its type. Five new tests cover the cases; all fail against the fake gh before this commit. Refs #102 --- scripts/tests/fixtures/gh-stub/gh | 185 ++++++++++-------------- scripts/tests/plan-cli/gh-stub.test.mjs | 73 +++++++++- 2 files changed, 145 insertions(+), 113 deletions(-) diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index 1c7c2b2c..46b3d7dc 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -255,38 +255,11 @@ function selectionSetAt(text, start) { return text.slice(start); } -// The text of the selection set that the first match of `opener` opens; `opener` ends with the opening brace. -function selectionSet(text, opener) { - const opened = opener.exec(text); - return opened ? selectionSetAt(text, opened.index + opened[0].length) : ''; -} - -// The selection set of the first inline fragment `... on {...}` in `text`. -function fragmentSet(text, type) { - return selectionSet(text, new RegExp(`\\.\\.\\.\\s*on\\s+${type}\\s*\\{`)); -} - -// The text that a selection set holds at its own level, without the text of nested selections. -function ownLevel(selection) { - let own = ''; - let depth = 0; - for (const char of selection) { - if (char === '{') depth += 1; - else if (char === '}') depth -= 1; - else if (depth === 0) own += char; - } - return own; -} - -// The fields that `... on {...}` selects at its own level, without the text of nested selections. -function ownFields(query, type) { - return ownLevel(fragmentSet(query, type)); -} - // The fields that a selection set names at its own level, in query order. Each field has its response key (its alias, -// or else its name), its name, and the text of its nested selection set (null if it has none). An inline fragment -// `... on {...}` adds its fields only when is `typename`, as GitHub applies only the fragments that match -// the object. Commas are white space in GraphQL. Like `graphqlSelections`, this reads arguments up to the first `)`. +// or else its name), its name, its arguments, and the text of its nested selection set (null if it has none). An +// inline fragment `... on {...}` adds its fields only when is `typename`, as GitHub applies only the +// fragments that match the object. Commas are white space in GraphQL. Like `graphqlSelections`, this reads arguments up +// to the first `)`. function selectionFields(selection, typename) { const fields = []; let index = 0; @@ -324,13 +297,15 @@ function selectionFields(selection, typename) { skipSpace(); name = readName(); } + let fieldArgs = new Map(); if (selection[index] === '(') { const end = selection.indexOf(')', index); if (end < 0) die(`fake gh cannot read the GraphQL arguments of '${name}': ${selection}`); + fieldArgs = graphqlArguments(selection.slice(index + 1, end)); index = end + 1; skipSpace(); } - fields.push({ key, name, nested: selection[index] === '{' ? readSet() : null }); + fields.push({ key, name, args: fieldArgs, nested: selection[index] === '{' ? readSet() : null }); } return fields; } @@ -338,34 +313,41 @@ function selectionFields(selection, typename) { // `object` as GitHub returns it for a selection set: only the fields that the set names at its own level, each under // its response key, and each object field only with the fields of its nested set. For `{landed: mergedAt}` GitHub // returns `landed` with the value of `mergedAt`, and no `mergedAt`. A fixture field that the query does not select must -// not reach the helper, or a test passes for a query that GitHub would answer without that field. +// not reach the helper, or a test passes for a query that GitHub would answer without that field. A stored function +// gives the value of a field that reads its arguments, such as a connection; it gets the response key and arguments. function selectFields(object, selection) { - // GitHub merges the selections of one response key, and refuses a key that names two different fields. + // GitHub merges the selections of one response key, and refuses a key that names two different fields. The merged + // field reads the arguments of its first selection; the fake gh does not compare the arguments. const keys = new Map(); - for (const { key, name, nested } of selectionFields(selection, object.__typename)) { - const merged = keys.get(key); - if (merged && merged.name !== name) die(`Field '${key}' has a field conflict: ${merged.name} or ${name}?`); - if (merged) merged.sets.push(nested); - else keys.set(key, { name, sets: [nested] }); + for (const field of selectionFields(selection, object.__typename)) { + const merged = keys.get(field.key); + if (merged && merged.name !== field.name) + die(`Field '${field.key}' has a field conflict: ${merged.name} or ${field.name}?`); + if (merged) merged.sets.push(field.nested); + else keys.set(field.key, { name: field.name, args: field.args, sets: [field.nested] }); } const selected = {}; - for (const [key, { name, sets }] of keys) { - // The caller adds the fields that the fixture object does not store, such as a connection. + for (const [key, { name, args, sets }] of keys) { + // The caller adds the fields that the fixture object does not store. if (!Object.hasOwn(object, name)) continue; - const value = object[name]; - if (value === null || typeof value !== 'object') { - selected[key] = value; - continue; - } - const nested = sets.filter(Boolean).join(' '); - // GitHub refuses a query that selects an object field without its subfields. - if (!nested.trim()) - die(`Field must have selections (field '${name}' has no selections. Did you mean '${name} { ... }'?)`); - selected[key] = selectFields(value, nested); + const stored = object[name]; + const value = typeof stored === 'function' ? stored({ alias: key, args }) : stored; + selected[key] = selectValue(value, name, sets.filter(Boolean).join(' ')); } return selected; } +// The value of field `name` as GitHub returns it for `nested`, the merged text of the field's nested selection sets. +// Each object in a list gets the same nested selection set. +function selectValue(value, name, nested) { + if (Array.isArray(value)) return value.map((item) => selectValue(item, name, nested)); + if (value === null || typeof value !== 'object') return value; + // GitHub refuses a query that selects an object field without its subfields. + if (!nested.trim()) + die(`Field must have selections (field '${name}' has no selections. Did you mean '${name} { ... }'?)`); + return selectFields(value, nested); +} + function graphqlPage(nodes, selection, variables, allowed) { for (const name of selection.args.keys()) { if (!allowed.includes(name)) die(`Unknown GraphQL argument: "${name}"`); @@ -707,43 +689,46 @@ if (group === 'api' && command === 'graphql') { (reference.repository?.owner?.login && reference.repository?.name ? `${reference.repository.owner.login}/${reference.repository.name}` : state.repo.nameWithOwner); - // A pull request node, for a closer, an associated pull request, or Repository.pullRequest, with only the fields - // that `selection`, the selection set of the node, names, each under its response key. - const pullRequestNode = (reference, selection) => { + // A pull request, for a closer, an associated pull request, or Repository.pullRequest, with every field that the fake + // gh can return. `selectFields` then keeps the fields that the query selects. The nested repository has its type, so + // that `__typename` and `... on Repository{...}` read it as on GitHub. + const pullRequestObject = (reference) => { const repo = referenceRepo(reference); const pullRequest = (state.prs ?? []).find( (entry) => entry.number === reference.number && entry.repository === repo, ); const prState = pullRequest?.state ?? reference.state ?? (pullRequest?.mergedAt || reference.mergedAt ? 'MERGED' : 'OPEN'); - return selectFields( - { - __typename: 'PullRequest', - number: reference.number, - url: pullRequest?.url ?? reference.url ?? `https://github.com/${repo}/pull/${reference.number}`, - body: pullRequest?.body ?? reference.body ?? null, - state: prState, - merged: prState === 'MERGED', - mergedAt: pullRequest?.mergedAt ?? reference.mergedAt ?? null, - baseRefName: pullRequest?.baseRefName ?? reference.baseRefName ?? null, - repository: { nameWithOwner: repo }, - }, - selection, - ); - }; - const commitAssociations = (oid, selection) => { - const commit = (state.commits ?? []).find( - (entry) => entry.oid === oid && (entry.repository ?? state.repo.nameWithOwner) === state.repo.nameWithOwner, - ); - if (!commit) die(`commit not found: ${state.repo.nameWithOwner}@${oid}`); - const nodeFields = selectionSet(selection.fields, /\bnodes\s*\{/); - return graphqlPage( - (commit.associatedPullRequests ?? []).map((reference) => pullRequestNode(reference, nodeFields)), - selection, - variables, - ['first', 'after'], - ); + return { + __typename: 'PullRequest', + number: reference.number, + url: pullRequest?.url ?? reference.url ?? `https://github.com/${repo}/pull/${reference.number}`, + body: pullRequest?.body ?? reference.body ?? null, + state: prState, + merged: prState === 'MERGED', + mergedAt: pullRequest?.mergedAt ?? reference.mergedAt ?? null, + baseRefName: pullRequest?.baseRefName ?? reference.baseRefName ?? null, + repository: { __typename: 'Repository', nameWithOwner: repo }, + }; }; + // A commit, for a closer or Repository.object. Its associatedPullRequests connection pages with the arguments of the + // field that selects it, and `selectFields` gives each page the fields that this field selects. + const commitObject = (oid) => ({ + __typename: 'Commit', + oid, + associatedPullRequests: (selection) => { + const commit = (state.commits ?? []).find( + (entry) => entry.oid === oid && (entry.repository ?? state.repo.nameWithOwner) === state.repo.nameWithOwner, + ); + if (!commit) die(`commit not found: ${state.repo.nameWithOwner}@${oid}`); + return graphqlPage( + (commit.associatedPullRequests ?? []).map((reference) => pullRequestObject(reference)), + selection, + variables, + ['first', 'after'], + ); + }, + }); const repository = { nameWithOwner: state.repo.nameWithOwner, visibility: state.repo.visibility, @@ -804,37 +789,21 @@ if (group === 'api' && command === 'graphql') { const latest = (issue.timelineItems ?? []) .filter((event) => event.__typename === 'ClosedEvent' || Object.hasOwn(event, 'closer')) .at(-1); - const closer = latest?.closer; - let renderedCloser = closer; - if (closer?.__typename === 'PullRequest') { - // `selectFields` merges the fields that the closer selects at its own level with those of its - // `... on PullRequest`, and ignores its `... on Commit`. - renderedCloser = pullRequestNode(closer, selectionSet(query, /\bcloser\s*\{/)); - } - if (closer?.__typename === 'Commit') { - const association = graphqlSelections(query, 'associatedPullRequests'); - renderedCloser = { - __typename: 'Commit', - oid: closer.oid, - ...(association.length && { - associatedPullRequests: commitAssociations(closer.oid, association[0]), - }), - }; - } + let closer = latest?.closer; + if (closer?.__typename === 'PullRequest') closer = pullRequestObject(closer); + else if (closer?.__typename === 'Commit') closer = commitObject(closer.oid); // GitHub stamps every close; a fixture close without a time reads as the issue's last update. - const stamped = /\bcreatedAt\b/.test(ownFields(query, 'ClosedEvent')); - const node = { ...(stamped && { createdAt: latest?.createdAt ?? issue.updatedAt }), closer: renderedCloser }; - payload[selection.alias] = { nodes: latest ? [node] : [] }; + const event = { __typename: 'ClosedEvent', createdAt: latest?.createdAt ?? issue.updatedAt, closer }; + // Each connection reads its nodes with its own selection set, so two aliased connections can select different + // fields of the same close. + payload[selection.alias] = selectFields({ nodes: latest ? [event] : [] }, selection.fields); } else die(`unsupported timeline item types: ${types ?? '(missing)'}`); } repository.issue = payload; } else if (objectSelection.length) { - const oid = graphqlValue(objectSelection[0].args.get('expression'), variables); - const association = graphqlSelections(query, 'associatedPullRequests'); - if (association.length !== 1) die('graphql object requires associatedPullRequests'); - repository[objectSelection[0].alias] = { - associatedPullRequests: commitAssociations(oid, association[0]), - }; + const [selection] = objectSelection; + const oid = graphqlValue(selection.args.get('expression'), variables); + repository[selection.alias] = selectFields(commitObject(oid), selection.fields); } else if (issuesSelection.length) { // Repository.issues: `labels` keeps issues that carry any named label; `states` and `orderBy` as on GitHub. const [selection] = issuesSelection; @@ -897,7 +866,7 @@ if (group === 'api' && command === 'graphql') { }, ]); } - const node = pullRequestNode({ number }, selection.fields); + const node = selectFields(pullRequestObject({ number }), selection.fields); for (const page of graphqlSelections(selection.fields, 'closingIssuesReferences')) { const references = (pullRequest.closingIssuesReferences ?? []).map((issueNumber) => ({ number: issueNumber, diff --git a/scripts/tests/plan-cli/gh-stub.test.mjs b/scripts/tests/plan-cli/gh-stub.test.mjs index 577da1ff..454e1968 100644 --- a/scripts/tests/plan-cli/gh-stub.test.mjs +++ b/scripts/tests/plan-cli/gh-stub.test.mjs @@ -53,6 +53,16 @@ function pullRequest(context, fields) { return graphql(context, query, { pr: PR }); } +// Repository.object for a commit that is associated with the seeded pull request, as the helper reads the pull +// requests of a commit closer. `fields` is the selection set of the associatedPullRequests connection. +function associatedPullRequests(context, fields) { + context.updateState((state) => { + state.commits = [{ oid: OID, associatedPullRequests: [{ number: PR }] }]; + }); + const query = `query($owner:String!,$name:String!,$oid:String!){repository(owner:$owner,name:$name){commit:object(expression:$oid){... on Commit{associatedPullRequests(first:100){${fields}}}}}}`; + return graphql(context, query, { oid: OID }); +} + test('the fake gh returns only the selected fields of Repository.pullRequest', (t) => { const context = planCliContext(t); seedPullRequest(context); @@ -74,17 +84,70 @@ test('the fake gh returns only the selected fields of a pull request closer', (t } }); +test('the fake gh returns only the selected fields of a closer for each aliased timeline connection', (t) => { + const context = planCliContext(t); + const { number, closer } = context.closedPlan('aliased closers'); + // Each connection selects other fields of the same close, and GitHub answers each with its own selection. + const closures = (fields) => `timelineItems(last:1,itemTypes:[CLOSED_EVENT]){nodes{... on ClosedEvent{${fields}}}}`; + const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){issue(number:$number){a:${closures('closer{... on PullRequest{number}}')} b:${closures('createdAt closer{... on PullRequest{url}}')}}}}`; + const { issue } = responseData(graphql(context, query, { number })).repository; + assert.deepEqual(issue.a, { nodes: [{ closer: { number: closer.number } }] }); + // The stored close has no time, so the fake gh stamps it with the issue's last update. + assert.deepEqual(issue.b, { nodes: [{ createdAt: issue.updatedAt, closer: { url: closer.url } }] }); +}); + test('the fake gh returns only the selected fields of the pull requests associated with a commit', (t) => { const context = planCliContext(t); seedPullRequest(context); - context.updateState((state) => { - state.commits = [{ oid: OID, associatedPullRequests: [{ number: PR }] }]; - }); - const query = `query($owner:String!,$name:String!,$oid:String!){repository(owner:$owner,name:$name){commit:object(expression:$oid){... on Commit{associatedPullRequests(first:100){nodes{number}}}}}}`; - const data = responseData(graphql(context, query, { oid: OID })); + const data = responseData(associatedPullRequests(context, 'nodes{number}')); assert.deepEqual(data.repository.commit.associatedPullRequests.nodes, [{ number: PR }]); }); +test('the fake gh merges the repeated node selections of a connection', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + const data = responseData(associatedPullRequests(context, 'nodes{number} nodes{url}')); + assert.deepEqual(data.repository.commit.associatedPullRequests, { nodes: [{ number: PR, url: PR_URL }] }); +}); + +test('the fake gh gives the repository of a pull request its type', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + const data = responseData(pullRequest(context, 'repository{__typename ... on Repository{nameWithOwner}}')); + assert.deepEqual(data.repository.pullRequest, { + repository: { __typename: 'Repository', nameWithOwner: 'DocksDocks/fixture' }, + }); +}); + +test('the fake gh merges the repeated selections of an object field', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + const repository = { a: 'DocksDocks/fixture', b: 'DocksDocks/fixture' }; + const data = responseData(pullRequest(context, 'repository{a:nameWithOwner} repository{b:nameWithOwner}')); + assert.deepEqual(data.repository.pullRequest, { repository }); + // The selections also merge when two selections of the parent field hold them. + const nested = responseData( + associatedPullRequests(context, 'nodes{repository{a:nameWithOwner}} nodes{repository{b:nameWithOwner}}'), + ); + assert.deepEqual(nested.repository.commit.associatedPullRequests, { nodes: [{ repository }] }); +}); + +test('the fake gh refuses an object field without subfields', (t) => { + const context = planCliContext(t); + seedPullRequest(context); + // The bare field is in the selection set of the pull request, and also in the second of two `nodes` selections. + for (const result of [ + pullRequest(context, 'number repository'), + associatedPullRequests(context, 'nodes{number} nodes{repository}'), + ]) { + assert.equal(result.status, 1); + assert.equal( + result.stderr, + "Field must have selections (field 'repository' has no selections. Did you mean 'repository { ... }'?)\n", + ); + } +}); + test('the fake gh returns an aliased pull request field only under its alias', (t) => { const context = planCliContext(t); seedPullRequest(context); From ad9068d4e97f1fed8b1b8e52148e7bdb10c658dd Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:21:21 -0300 Subject: [PATCH 8/9] test: parse env options in fixture shebangs and show extra-toolchain evidence Code review round 2 of #102 found that `#!/usr/bin/env -S -u NODE_OPTIONS bun` gave the interpreter NODE_OPTIONS, and that the extra-toolchain diagnostic named no evidence. The shebang parser now skips env options and their operands as GNU and BSD env do, and the diagnostic lists the discovered runners with their sources. The parser is exported and tested in author-tooling, and the registry check runs only when the file is executed. Refs #102 --- scripts/tests/author-tooling.mjs | 30 +++- scripts/tests/test-contracts.mjs | 228 +++++++++++++++++++------------ 2 files changed, 168 insertions(+), 90 deletions(-) diff --git a/scripts/tests/author-tooling.mjs b/scripts/tests/author-tooling.mjs index 58d947d3..78386911 100755 --- a/scripts/tests/author-tooling.mjs +++ b/scripts/tests/author-tooling.mjs @@ -5,6 +5,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { shebangInterpreter } from './test-contracts.mjs'; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); const BIOME = path.join(ROOT, 'node_modules', '.bin', process.platform === 'win32' ? 'biome.cmd' : 'biome'); @@ -301,10 +302,12 @@ function testTestContractToolchains() { const extra = runTestContracts(fixtureRoot, 'lists-node.json', listsNode); assertStarted(extra); assert.notEqual(extra.status, 0); + // The line names the runners that discovery found, so that the reader sees why the row does not need the token. + const runnerEvidence = `\\(discovered: [^\\n]*\\b${escapeRegExp(bunOnly.owner.suite)} with bun\\b[^\\n]*\\)`; assert.match( `${extra.stdout}\n${extra.stderr}`, new RegExp( - `${escapeRegExp(bunOnly.id)}\\.toolchains lists ${escapeRegExp(nodeToken)}, which no discovered runner needs`, + `${escapeRegExp(bunOnly.id)}\\.toolchains lists ${escapeRegExp(nodeToken)}, which no discovered runner needs ${runnerEvidence}`, ), ); } finally { @@ -312,6 +315,30 @@ function testTestContractToolchains() { } } +// A fixture program runs with the interpreter of its #! line, and that interpreter decides which toolchain a suite +// needs. `env` reads its options, their operands, and NAME=VALUE words before the interpreter, so none of them is it. +function testShebangInterpreter() { + const cases = [ + ['#!/usr/bin/env bun\n', 'bun'], + ['#!/usr/local/bin/bun\n', 'bun'], + ['#!/usr/bin/env -S -u NODE_OPTIONS bun\n', 'bun'], + ['#!/usr/bin/env -S -uNODE_OPTIONS bun --smol\n', 'bun'], + ['#!/usr/bin/env --split-string=-u NODE_OPTIONS bun\n', 'bun'], + ['#!/usr/bin/env --unset=NODE_OPTIONS node\n', 'node'], + ['#!/usr/bin/env --unset NODE_OPTIONS node\n', 'node'], + ['#!/usr/bin/env -i FORCE_COLOR=0 node\n', 'node'], + ['#!/usr/bin/env - --ignore-environment node\n', 'node'], + ['#!/usr/bin/env -iu NODE_OPTIONS /opt/bun/bin/bun\n', 'bun'], + ['#!/usr/bin/env -- bun\n', 'bun'], + ['#!/usr/bin/env -S -u NODE_OPTIONS\n', ''], + ['#!\n', ''], + ['console.log(1);\n', null], + ]; + for (const [source, interpreter] of cases) { + assert.equal(shebangInterpreter(source), interpreter, `interpreter of ${JSON.stringify(source)}`); + } +} + // Every docks skill must end with the canonical Report section. The cases change a full copy of the docks skills: the // copy passes the sub-guards that run before the Report check, so that each run gets to that check. function testSkillsGuardReportSection() { @@ -381,5 +408,6 @@ testTreeGuardOperationalFailures(); testSkillsGuardSpawnFailure(); testSkillsGuardReportSection(); testTestContractToolchains(); +testShebangInterpreter(); testValidatorTable(); console.log('author tooling contracts passed'); diff --git a/scripts/tests/test-contracts.mjs b/scripts/tests/test-contracts.mjs index 13e10758..7a27d80c 100755 --- a/scripts/tests/test-contracts.mjs +++ b/scripts/tests/test-contracts.mjs @@ -447,16 +447,51 @@ function filesUnder(relativeDirectory) { return files.sort(); } -function shebangInterpreter(source) { +// The `env` options that take an operand, as GNU and BSD `env` read them: -u NAME unsets, -C DIR changes folder, +// -P PATH sets the search path. -S STRING is also an operand, but its words are more `env` arguments. +const ENV_OPERAND_FLAGS = new Set(['u', 'C', 'P']); +const ENV_OPERAND_LONG = new Set(['--unset', '--chdir']); + +// The interpreter of a #! line: null if the source has no #! line, and '' if the line names no interpreter. +// `#!/usr/bin/env [options] [NAME=VALUE...] ` finds on PATH, so the options, their operands, and the +// assignments come before the interpreter and are not it. +export function shebangInterpreter(source) { const line = /^#!(?[^\n]*)/u.exec(source)?.groups.line; if (line === undefined) return null; - const [program, ...words] = line.trim().split(/\s+/u); - // `env` finds the interpreter on PATH, and its options and NAME=value words come before the interpreter. - const interpreter = - path.posix.basename(program) === 'env' - ? words.find((word) => !word.startsWith('-') && !word.includes('=')) - : program; - return path.posix.basename(interpreter ?? ''); + const [program = '', ...words] = line.trim().split(/\s+/u).filter(Boolean); + if (path.posix.basename(program) !== 'env') return path.posix.basename(program); + const pending = [...words]; + while (pending.length > 0) { + const word = pending.shift(); + if (word === '--') return path.posix.basename(pending[0] ?? ''); + if (word === '-' || word === '--ignore-environment') continue; + if (word.startsWith('--')) { + const [name, value] = word.split(/=(.*)/su); + // The kernel gives `env` the rest of the #! line as one word, and -S splits it; this split already did that. + if (name === '--split-string') pending.unshift(...(value ?? '').split(/\s+/u).filter(Boolean)); + else if (ENV_OPERAND_LONG.has(name) && value === undefined) pending.shift(); + continue; + } + if (word.startsWith('-')) { + // A cluster such as -iu NAME: the first flag that takes an operand takes the rest of the word or the next word. + for (let index = 1; index < word.length; index += 1) { + const flag = word[index]; + const rest = word.slice(index + 1); + if (flag === 'S') { + if (rest.length > 0) pending.unshift(rest); + break; + } + if (ENV_OPERAND_FLAGS.has(flag)) { + if (rest.length === 0) pending.shift(); + break; + } + } + continue; + } + if (/^[^=]+=/u.test(word)) continue; + return path.posix.basename(word); + } + return ''; } // A suite can run a fixture program, as the plan CLI cases run the fake gh from PATH. The suite then needs the @@ -508,91 +543,106 @@ function fixtureRunners(suite, programs) { // The gate checks the committed registry. A test can give `--registry ` to check a changed copy, so that it can // show that each check rejects the defect that it covers. -const { values: options } = parseArgs({ - options: { registry: { type: 'string', default: 'scripts/config/test-contracts.json' } }, -}); -const registry = JSON.parse(fs.readFileSync(path.resolve(REPO, options.registry), 'utf8')); -const packageJson = readJson('package.json'); -const ciSource = fs.readFileSync(path.join(REPO, 'scripts/ci.mjs'), 'utf8'); -validateRegistry(registry); - -const gateInvocations = gateInvocationArguments(ciSource); -const discoveredDescriptors = [ - ...unitDescriptors(packageJson), - ...repoSuiteDescriptors(gateInvocations), - ...pluginDescriptors(gateInvocations), -]; -const descriptorById = new Map(); -for (const discovered of discoveredDescriptors) { - assert.equal(descriptorById.has(discovered.id), false, `duplicate discovered contract id: ${discovered.id}`); - descriptorById.set(discovered.id, discovered); -} - -const discovered = new Set(descriptorById.keys()); -const registered = new Set(registry.contracts.map(({ id }) => id)); -assertSetEqual(registered, discovered, 'discovered and registered sets differ'); - -const selected = new Set(); -const executed = new Set(); -const toolchainByRunner = new Map([ - ['node', nodeToolchain()], - ['bun', bunToolchain(packageJson)], -]); -const programs = fixturePrograms(); -const toolchainDifferences = []; -for (const contract of registry.contracts) { - const evidence = descriptorById.get(contract.id); - assert.ok(evidence, `${contract.id} has no discovered selection evidence`); - assert.equal(contract.owner.suite, evidence.suite, `${contract.id} owner suite differs from discovery`); - assert.equal(contract.selection.kind, evidence.kind, `${contract.id} selection kind differs from discovery`); - assert.equal(contract.selection.selector, evidence.selector, `${contract.id} selector differs from gate descriptor`); - assert.ok(evidence.selectedCount > 0, `${contract.id} selected no tests`); - assert.ok( - evidence.selectedCount >= contract.selection.expected_min, - `${contract.id} selected ${evidence.selectedCount}, below expected minimum ${contract.selection.expected_min}`, - ); - assert.ok(fs.existsSync(path.join(REPO, contract.owner.suite)), `${contract.id} owner suite does not exist`); - assert.ok(evidence.runners.length > 0, `${contract.id} has no discovered runner`); - const runners = [ - ...evidence.runners.map((runner) => ({ - runner, - source: `${evidence.runnerSource} runs ${evidence.suite} with ${runner}`, - })), - ...fixtureRunners(evidence.suite, programs), +function main() { + const { values: options } = parseArgs({ + options: { registry: { type: 'string', default: 'scripts/config/test-contracts.json' } }, + }); + const registry = JSON.parse(fs.readFileSync(path.resolve(REPO, options.registry), 'utf8')); + const packageJson = readJson('package.json'); + const ciSource = fs.readFileSync(path.join(REPO, 'scripts/ci.mjs'), 'utf8'); + validateRegistry(registry); + + const gateInvocations = gateInvocationArguments(ciSource); + const discoveredDescriptors = [ + ...unitDescriptors(packageJson), + ...repoSuiteDescriptors(gateInvocations), + ...pluginDescriptors(gateInvocations), ]; - const sourcesByToolchain = new Map(); - for (const { runner, source } of runners) { - const toolchain = toolchainByRunner.get(runner); + const descriptorById = new Map(); + for (const discovered of discoveredDescriptors) { + assert.equal(descriptorById.has(discovered.id), false, `duplicate discovered contract id: ${discovered.id}`); + descriptorById.set(discovered.id, discovered); + } + + const discovered = new Set(descriptorById.keys()); + const registered = new Set(registry.contracts.map(({ id }) => id)); + assertSetEqual(registered, discovered, 'discovered and registered sets differ'); + + const selected = new Set(); + const executed = new Set(); + const toolchainByRunner = new Map([ + ['node', nodeToolchain()], + ['bun', bunToolchain(packageJson)], + ]); + const programs = fixturePrograms(); + const toolchainDifferences = []; + for (const contract of registry.contracts) { + const evidence = descriptorById.get(contract.id); + assert.ok(evidence, `${contract.id} has no discovered selection evidence`); + assert.equal(contract.owner.suite, evidence.suite, `${contract.id} owner suite differs from discovery`); + assert.equal(contract.selection.kind, evidence.kind, `${contract.id} selection kind differs from discovery`); + assert.equal( + contract.selection.selector, + evidence.selector, + `${contract.id} selector differs from gate descriptor`, + ); + assert.ok(evidence.selectedCount > 0, `${contract.id} selected no tests`); assert.ok( - toolchain, - `${contract.id} runs with ${runner}, which has no known toolchain token (${source}); map ${runner} to its pinned toolchain in toolchainByRunner`, + evidence.selectedCount >= contract.selection.expected_min, + `${contract.id} selected ${evidence.selectedCount}, below expected minimum ${contract.selection.expected_min}`, ); - const sources = sourcesByToolchain.get(toolchain) ?? new Set(); - sources.add(source); - sourcesByToolchain.set(toolchain, sources); - } - // An exact list, so that a toolchain which a runner needs cannot go missing, and one which nothing needs cannot stay. - for (const [toolchain, sources] of [...sourcesByToolchain].sort(([left], [right]) => left.localeCompare(right))) { - if (!contract.toolchains.includes(toolchain)) { - toolchainDifferences.push(`${contract.id}.toolchains must list ${toolchain}: ${[...sources].join('; ')}`); + assert.ok(fs.existsSync(path.join(REPO, contract.owner.suite)), `${contract.id} owner suite does not exist`); + assert.ok(evidence.runners.length > 0, `${contract.id} has no discovered runner`); + const runners = [ + ...evidence.runners.map((runner) => ({ + runner, + source: `${evidence.runnerSource} runs ${evidence.suite} with ${runner}`, + })), + ...fixtureRunners(evidence.suite, programs), + ]; + const sourcesByToolchain = new Map(); + for (const { runner, source } of runners) { + const toolchain = toolchainByRunner.get(runner); + assert.ok( + toolchain, + `${contract.id} runs with ${runner}, which has no known toolchain token (${source}); map ${runner} to its pinned toolchain in toolchainByRunner`, + ); + const sources = sourcesByToolchain.get(toolchain) ?? new Set(); + sources.add(source); + sourcesByToolchain.set(toolchain, sources); } - } - for (const toolchain of contract.toolchains) { - if (!sourcesByToolchain.has(toolchain)) { - toolchainDifferences.push(`${contract.id}.toolchains lists ${toolchain}, which no discovered runner needs`); + // An exact list, so that a toolchain which a runner needs cannot go missing, and one which nothing needs cannot + // stay. + for (const [toolchain, sources] of [...sourcesByToolchain].sort(([left], [right]) => left.localeCompare(right))) { + if (!contract.toolchains.includes(toolchain)) { + toolchainDifferences.push(`${contract.id}.toolchains must list ${toolchain}: ${[...sources].join('; ')}`); + } + } + // The discovered runners are the evidence: they show which toolchains the row needs instead. + const discoveredRunners = [...new Set(runners.map(({ source }) => source))].join('; '); + for (const toolchain of contract.toolchains) { + if (!sourcesByToolchain.has(toolchain)) { + toolchainDifferences.push( + `${contract.id}.toolchains lists ${toolchain}, which no discovered runner needs (discovered: ${discoveredRunners})`, + ); + } } + selected.add(contract.id); + if (evidence.executed) executed.add(contract.id); } - selected.add(contract.id); - if (evidence.executed) executed.add(contract.id); + assert.ok(selected.size > 0, 'selected set must not be empty'); + for (const id of selected) assert.ok(registered.has(id), `selected contract is not registered: ${id}`); + assertSetEqual(executed, selected, 'executed and selected sets differ'); + // Collected rather than asserted per row so one run names every stale contract. + assert.deepEqual( + toolchainDifferences, + [], + `contract toolchains differ from the discovered runners; make each row of scripts/config/test-contracts.json list exactly the toolchains that its runners need:\n${toolchainDifferences.join('\n')}`, + ); + + const counts = `contracts=${registered.size} discovered=${discovered.size} selected=${selected.size}`; + console.log(`PASS test_contracts ${counts}`); } -assert.ok(selected.size > 0, 'selected set must not be empty'); -for (const id of selected) assert.ok(registered.has(id), `selected contract is not registered: ${id}`); -assertSetEqual(executed, selected, 'executed and selected sets differ'); -// Collected rather than asserted per row so one run names every stale contract. -assert.deepEqual( - toolchainDifferences, - [], - `contract toolchains differ from the discovered runners; make each row of scripts/config/test-contracts.json list exactly the toolchains that its runners need:\n${toolchainDifferences.join('\n')}`, -); - -console.log(`PASS test_contracts contracts=${registered.size} discovered=${discovered.size} selected=${selected.size}`); + +// author-tooling.mjs imports shebangInterpreter to test it, and the import must not run the registry check. +if (import.meta.main) main(); From 848a28637ceb99932cb83fb005395c79660c1536 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:25:20 -0300 Subject: [PATCH 9/9] test: skip env assignments after -- and select every field a test reads Code review round 3 of #102 found two small gaps: `#!/usr/bin/env -S -- FORCE_COLOR=0 bun` gave the interpreter FORCE_COLOR=0, because the parser stopped at `--` although env still reads assignments after it; and the aliased-timeline test read `issue.updatedAt` without selecting it. The parser now skips assignments after `--`, with a new case, and the test selects `updatedAt`. Refs #102 --- scripts/tests/author-tooling.mjs | 1 + scripts/tests/plan-cli/gh-stub.test.mjs | 2 +- scripts/tests/test-contracts.mjs | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/tests/author-tooling.mjs b/scripts/tests/author-tooling.mjs index 78386911..f57ede4d 100755 --- a/scripts/tests/author-tooling.mjs +++ b/scripts/tests/author-tooling.mjs @@ -329,6 +329,7 @@ function testShebangInterpreter() { ['#!/usr/bin/env -i FORCE_COLOR=0 node\n', 'node'], ['#!/usr/bin/env - --ignore-environment node\n', 'node'], ['#!/usr/bin/env -iu NODE_OPTIONS /opt/bun/bin/bun\n', 'bun'], + ['#!/usr/bin/env -S -- FORCE_COLOR=0 bun\n', 'bun'], ['#!/usr/bin/env -- bun\n', 'bun'], ['#!/usr/bin/env -S -u NODE_OPTIONS\n', ''], ['#!\n', ''], diff --git a/scripts/tests/plan-cli/gh-stub.test.mjs b/scripts/tests/plan-cli/gh-stub.test.mjs index 454e1968..e0d116fd 100644 --- a/scripts/tests/plan-cli/gh-stub.test.mjs +++ b/scripts/tests/plan-cli/gh-stub.test.mjs @@ -89,7 +89,7 @@ test('the fake gh returns only the selected fields of a closer for each aliased const { number, closer } = context.closedPlan('aliased closers'); // Each connection selects other fields of the same close, and GitHub answers each with its own selection. const closures = (fields) => `timelineItems(last:1,itemTypes:[CLOSED_EVENT]){nodes{... on ClosedEvent{${fields}}}}`; - const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){issue(number:$number){a:${closures('closer{... on PullRequest{number}}')} b:${closures('createdAt closer{... on PullRequest{url}}')}}}}`; + const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){issue(number:$number){updatedAt a:${closures('closer{... on PullRequest{number}}')} b:${closures('createdAt closer{... on PullRequest{url}}')}}}}`; const { issue } = responseData(graphql(context, query, { number })).repository; assert.deepEqual(issue.a, { nodes: [{ closer: { number: closer.number } }] }); // The stored close has no time, so the fake gh stamps it with the issue's last update. diff --git a/scripts/tests/test-contracts.mjs b/scripts/tests/test-contracts.mjs index 7a27d80c..7723c2ac 100755 --- a/scripts/tests/test-contracts.mjs +++ b/scripts/tests/test-contracts.mjs @@ -463,7 +463,8 @@ export function shebangInterpreter(source) { const pending = [...words]; while (pending.length > 0) { const word = pending.shift(); - if (word === '--') return path.posix.basename(pending[0] ?? ''); + // `--` ends the options, but `env` still reads NAME=VALUE assignments after it. + if (word === '--') return path.posix.basename(pending.find((rest) => !/^[^=]+=/u.test(rest)) ?? ''); if (word === '-' || word === '--ignore-environment') continue; if (word.startsWith('--')) { const [name, value] = word.split(/=(.*)/su);