diff --git a/scripts/check-agent-binding.mjs b/scripts/check-agent-binding.mjs index 93b3bf00a..e7db8c6b7 100644 --- a/scripts/check-agent-binding.mjs +++ b/scripts/check-agent-binding.mjs @@ -3,11 +3,41 @@ import { pathToFileURL } from 'node:url'; const ACTIVATION_ARTIFACTS = new Set(['activated', 'phases-activated']); const VALID_OMISSIONS = new Set(['multi-owner', 'non-roster']); +const TEMPORARY_ID = /^#?aw_[A-Za-z0-9_]{3,12}$/i; +const RESOLVED_REFERENCE = /^#?(\d+)$/; function normalize(value) { return typeof value === 'string' ? value.trim().toLowerCase() : ''; } +/** + * Resolve a binding issue reference to a real issue number. + * + * Activation writes these as quoted `#`-prefixed strings so gh-aw's temporary-ID + * substitution — a plain text replacement that does not skip fenced code blocks and keeps + * the `#` — yields valid JSON. A reference the runtime resolved arrives as `"#42"`; one it + * could not resolve arrives verbatim as `"#aw_task3"`, which means that `create-issue` + * never landed. Fail closed on the latter rather than skipping the binding. Bare integers + * stay accepted so artifacts written before this contract still validate. + */ +function resolveIssueReference(value, description, invalidMessage) { + if (Number.isInteger(value)) { + if (value < 1) throw new Error(invalidMessage); + return value; + } + if (typeof value === 'string') { + const trimmed = value.trim(); + if (TEMPORARY_ID.test(trimmed)) { + throw new Error( + `${description} "${trimmed}" is an unresolved temporary ID — the referenced issue was never created`, + ); + } + const resolved = RESOLVED_REFERENCE.exec(trimmed); + if (resolved && Number(resolved[1]) >= 1) return Number(resolved[1]); + } + throw new Error(invalidMessage); +} + export function parseStructuredData(comment) { const blocks = [...comment.matchAll(/Structured data:\s*```json\s*([\s\S]*?)```/gi)]; if (blocks.length === 0) { @@ -82,26 +112,29 @@ function validateReportedOutcome(binding, prefix, expected) { } function validateTaskBinding(binding, roster) { - if (!binding || typeof binding !== 'object' || !Number.isInteger(binding.issue) || binding.issue < 1) { + if (!binding || typeof binding !== 'object') { throw new Error('binding has no valid issue number'); } - if (!Number.isInteger(binding.epic_issue) || binding.epic_issue < 1) { - throw new Error(`issue #${binding.issue}: missing epic issue linkage`); - } - if (!normalize(binding.task)) throw new Error(`issue #${binding.issue}: binding has no plan task number`); - if (!normalize(binding.epic)) throw new Error(`issue #${binding.issue}: missing epic linkage`); + const issue = resolveIssueReference(binding.issue, 'binding issue', 'binding has no valid issue number'); + const epicIssue = resolveIssueReference( + binding.epic_issue, + `issue #${issue}: epic_issue`, + `issue #${issue}: missing epic issue linkage`, + ); + if (!normalize(binding.task)) throw new Error(`issue #${issue}: binding has no plan task number`); + if (!normalize(binding.epic)) throw new Error(`issue #${issue}: missing epic linkage`); const agent = normalize(binding.agent); - if (!agent) throw new Error(`issue #${binding.issue}: binding has no agent`); + if (!agent) throw new Error(`issue #${issue}: binding has no agent`); if (!Array.isArray(binding.epic_agents) || binding.epic_agents.length === 0) { - throw new Error(`issue #${binding.issue}: epic_agents must be a non-empty array`); + throw new Error(`issue #${issue}: epic_agents must be a non-empty array`); } const epicAgents = [...new Set(binding.epic_agents.map(normalize).filter(Boolean))].sort(); if (epicAgents.length !== binding.epic_agents.length || !epicAgents.includes(agent)) { - throw new Error(`issue #${binding.issue}: epic_agents are empty, duplicated, or exclude the task agent`); + throw new Error(`issue #${issue}: epic_agents are empty, duplicated, or exclude the task agent`); } const expected = expectedLabel(agent, roster); - validateReportedOutcome(binding, '', expected); - return { epicAgents, expected }; + validateReportedOutcome({ ...binding, issue }, '', expected); + return { issue, epicIssue, epicAgents, expected }; } function validateActualLabels(issue, labels, expected) { @@ -139,32 +172,33 @@ export function validateActivation(artifact, roster, labelsByIssue, expectedOrig const seen = new Set(); const epics = new Map(); const epicIssuesByIdentifier = new Map(); - for (const binding of artifact.bindings) { - const { epicAgents, expected } = validateTaskBinding(binding, roster); - if (seen.has(binding.issue)) throw new Error(`issue #${binding.issue}: duplicate binding`); - seen.add(binding.issue); - validateActualLabels(binding.issue, labelsByIssue.get(binding.issue), expected); + for (const rawBinding of artifact.bindings) { + const { issue, epicIssue, epicAgents, expected } = validateTaskBinding(rawBinding, roster); + const binding = { ...rawBinding, issue, epic_issue: epicIssue }; + if (seen.has(issue)) throw new Error(`issue #${issue}: duplicate binding`); + seen.add(issue); + validateActualLabels(issue, labelsByIssue.get(issue), expected); const epicIdentifier = normalize(binding.epic); const priorEpicIssue = epicIssuesByIdentifier.get(epicIdentifier); - if (priorEpicIssue !== undefined && priorEpicIssue !== binding.epic_issue) { + if (priorEpicIssue !== undefined && priorEpicIssue !== epicIssue) { throw new Error(`epic ${epicIdentifier}: maps to multiple epic issue numbers`); } - epicIssuesByIdentifier.set(epicIdentifier, binding.epic_issue); + epicIssuesByIdentifier.set(epicIdentifier, epicIssue); - const epic = epics.get(binding.epic_issue) ?? { + const epic = epics.get(epicIssue) ?? { epic: epicIdentifier, agents: epicAgents, bindings: [], }; if (epic.epic !== epicIdentifier) { - throw new Error(`epic issue #${binding.epic_issue}: conflicting epic identifiers`); + throw new Error(`epic issue #${epicIssue}: conflicting epic identifiers`); } if (epic.agents.join('\0') !== epicAgents.join('\0')) { - throw new Error(`epic issue #${binding.epic_issue}: inconsistent epic_agents sets`); + throw new Error(`epic issue #${epicIssue}: inconsistent epic_agents sets`); } epic.bindings.push(binding); - epics.set(binding.epic_issue, epic); + epics.set(epicIssue, epic); } for (const [epicIssue, epic] of epics) { diff --git a/test/check-agent-binding.test.ts b/test/check-agent-binding.test.ts index 7d2403156..b17ff77fa 100644 --- a/test/check-agent-binding.test.ts +++ b/test/check-agent-binding.test.ts @@ -287,7 +287,7 @@ Structured data: const workflow = readFileSync(join(process.cwd(), 'workflows', 'squad.md'), 'utf8').replace(/\r\n/g, '\n'); expect(workflow).toContain('Activation bindings:'); expect(workflow).toContain('"task":"{plan # cell}"'); - expect(workflow).toContain('"epic_issue":{created epic issue number}'); + expect(workflow).toContain('"epic_issue":"{epic issue reference}"'); expect(workflow).toContain('"epic_agents":["{all distinct lowercased Agent cells'); }); diff --git a/test/gh-aw-activate-fast-path-label-provisioning.test.ts b/test/gh-aw-activate-fast-path-label-provisioning.test.ts index 58b1b04fb..a06fbd4a5 100644 --- a/test/gh-aw-activate-fast-path-label-provisioning.test.ts +++ b/test/gh-aw-activate-fast-path-label-provisioning.test.ts @@ -315,8 +315,13 @@ describe('#1959: fast-path label sets stay at parity with squad-plan-activate', }); it('reports labels actually applied, never merely intended', () => { + // #1963 tightened this sentence: a run cannot know a label was "actually applied" — + // safe outputs land after the agent turn — so the claim is now an *accepted* + // `add_labels` call for that same issue, with create-issue explicitly ruled out as + // evidence. #1959's original guarantee (never skipped/deferred/merely intended) is + // preserved verbatim inside the tightened sentence. expect(acceptProse).toMatch( - /Report only the labels a successful `add_labels` call actually applied; never a label that was skipped, deferred, or merely intended/i, + /Report only the labels an accepted `add_labels` call carried for that same issue; never a label that was skipped, deferred, or merely intended, and never one attributed to `create-issue`/i, ); }); }); diff --git a/test/gh-aw-activation-summary-outcomes.test.ts b/test/gh-aw-activation-summary-outcomes.test.ts new file mode 100644 index 000000000..db6cb958c --- /dev/null +++ b/test/gh-aw-activation-summary-outcomes.test.ts @@ -0,0 +1,416 @@ +/** + * Activation summaries report actual accepted label-operation outcomes (#1963). + * + * Parent: #1957. Stacks on #1962 (`gh-aw-activation-temporary-ids.test.ts`, temporary-ID + * targeting for `/squad plan activate`) and #1959 + * (`gh-aw-activate-fast-path-label-provisioning.test.ts`, the `/squad activate` fast path's + * own `add_labels` provisioning). Those two made the label *operations* correct. They did + * not make the *summary* correct. + * + * Two defects remained, one per activation path, plus a latent data-contract bug: + * + * 1. Over-claim by attribution. Both paths permitted naming a `squad:{agent}` label once + * `create-issue` "returned successfully carrying it". `create-issue`'s `labels:` field + * cannot land a label GitHub does not already have — the exact failure #1959 fixed — + * so that sentence licensed reporting labels that were never applied. A label is now + * reportable only when an accepted `add_labels` call carried it for that same issue. + * + * 2. Invalid `Activation bindings:` JSON. The block required bare `{created task issue + * number}` / `{created epic issue number}`. The agent cannot know a created issue's + * real number during its turn, and gh-aw's temporary-ID substitution is a plain text + * replacement over the whole comment body that does *not* skip fenced code blocks and + * *keeps* the `#`. A bare `"issue":#aw_task1` therefore becomes `"issue":#42` — + * invalid JSON that fails the entire block. Verified empirically against the pinned + * runtime (`github/gh-aw-actions@v0.87.2`, `setup/js/temporary_id.cjs`). Quoting is + * the narrowest correct fix: `"issue":"#aw_task1"` → `"issue":"#42"`, which parses. + * + * 3. Unresolved references were undefined behavior. An `#aw_…` surviving substitution + * means that `create-issue` never landed. The checker now fails closed on it instead + * of coercing, skipping, or repairing it. + * + * The runtime limitation this suite deliberately encodes: safe outputs are applied in a + * post-agent job, so an activation run has evidence only that a call was *accepted for a + * specific target* — never the GitHub API result. "Accepted" is the strongest honest claim. + * These tests fail on any summary language that claims verification the runtime cannot + * provide, in either direction (over-claim or silent under-claim). + * + * Out of scope: operation-capacity policy (#1961) beyond compatibility, the broad + * behavioral contract suite (#1960), checker distribution, post-activation implementation, + * and E4 (#1958). + */ + +import { afterAll, describe, it, expect } from 'vitest'; +import { cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { join } from 'node:path'; +import { parseRoster, validateBindings } from '../scripts/check-agent-binding.mjs'; + +const WORKFLOWS_DIR = join(process.cwd(), 'workflows'); +const SQUAD_WORKFLOW = join(WORKFLOWS_DIR, 'squad.md'); +const ONTOLOGY = join(WORKFLOWS_DIR, 'shared', 'squad-planning-ontology.md'); +const TEST_WORKSPACES_DIR = join(process.cwd(), '.test-workspaces-activation-summary'); + +afterAll(() => { + rmSync(TEST_WORKSPACES_DIR, { recursive: true, force: true }); +}); + +/** Read a text file with line endings normalized to LF (Windows checkouts materialize CRLF). */ +function readText(filePath: string): string { + return readFileSync(filePath, 'utf8').replace(/\r\n/g, '\n'); +} + +const workflow = readText(SQUAD_WORKFLOW); +const ontologyProse = readText(ONTOLOGY).replace(/\s+/g, ' '); + +/** `squad-plan-activate` — the `/squad plan activate` path. Has an explicit end marker. */ +const ACTIVATE_START = workflow.indexOf('## skill: `squad-plan-activate`'); +const ACTIVATE_END = workflow.indexOf('## end skill: `squad-plan-activate`'); +expect(ACTIVATE_START, '"## skill: `squad-plan-activate`" is missing from workflows/squad.md').toBeGreaterThan(-1); +expect(ACTIVATE_END, '"## end skill: `squad-plan-activate`" is missing').toBeGreaterThan(ACTIVATE_START); +const activateSkill = workflow.slice(ACTIVATE_START, ACTIVATE_END); +const activateProse = activateSkill.replace(/\s+/g, ' '); + +/** + * `squad-plan-accept` — the recommended `/squad activate` fast path. It has no + * `## end skill:` marker, so its body runs to the next `## skill:` heading. + */ +const ACCEPT_START = workflow.indexOf('## skill: `squad-plan-accept`'); +const ACCEPT_END = workflow.indexOf('## skill: `squad-plan-revise`'); +expect(ACCEPT_START, '"## skill: `squad-plan-accept`" is missing from workflows/squad.md').toBeGreaterThan(-1); +expect(ACCEPT_END, '"## skill: `squad-plan-revise`" is missing from workflows/squad.md').toBeGreaterThan(ACCEPT_START); +const acceptSkill = workflow.slice(ACCEPT_START, ACCEPT_END); +const acceptProse = acceptSkill.replace(/\s+/g, ' '); + +/** Both activation paths, so parity assertions cannot pass by covering only one. */ +const BOTH_PATHS: ReadonlyArray = [ + ['/squad plan activate (squad-plan-activate)', activateProse], + ['/squad activate fast path (squad-plan-accept)', acceptProse], +]; + +describe('gh-aw: activation summaries report accepted outcomes, not intent (#1963)', () => { + it('routes both activation commands to the two skills this suite inspects', () => { + // If routing moves, every parity assertion below is inspecting the wrong prose. + expect(workflow).toContain('| `activate` | `squad-plan-accept` |'); + expect(workflow).toContain('| `plan activate` | `squad-plan-activate` |'); + }); + + it.each(BOTH_PATHS)( + '%s: refuses to treat a successful create-issue as label evidence', + (_path, prose) => { + expect( + prose, + 'A summary that credits create-issue for a label reports work that may never have ' + + 'happened: create-issue silently drops label names the repository lacks (#1959). ' + + 'Each path must state that create-issue is not evidence.', + ).toMatch(/A successful `create-issue` is \*\*not\*\* evidence/); + expect(prose).toMatch(/`labels:` field cannot land a label on a fresh repository/); + }, + ); + + it.each(BOTH_PATHS)( + '%s: conditions every reported label on an accepted add_labels call for that same issue', + (_path, prose) => { + expect( + prose, + 'The reportable condition must be an accepted add_labels operation targeting the ' + + 'same issue — not a computed value, not another item\'s call.', + ).toMatch(/`add_labels` call (?:that )?carr(?:ied|ying) that label(?: and targeting| and targeted)? that same issue|`add_labels` call carrying that label was accepted for that same issue/); + expect( + prose, + 'Targeting must name both forms: temporary ID for a created issue, verified real ' + + 'number for a reused one.', + ).toMatch(/by its own `temporary_id`, or by its verified real number for a reused issue/); + }, + ); + + it('removes the create-issue attribution that Label Pre-flight Step 8 used to license', () => { + // The exact pre-#1963 sentence. Its survival anywhere means the over-claim is still + // reachable, regardless of what was added elsewhere. + expect( + activateProse, + 'Label Pre-flight Step 8 previously allowed naming a label once "that issue\'s ' + + 'create-issue call returned successfully carrying it". That is the root over-claim.', + ).not.toMatch(/`create-issue` call returned\s*successfully carrying it/); + expect(activateProse).not.toMatch(/create-issue` call returned successfully carrying it/); + }); + + it.each(BOTH_PATHS)( + '%s: claims acceptance only, never verification the runtime cannot provide', + (_path, prose) => { + expect( + prose, + 'Safe outputs are applied after the agent turn. A run that says it verified or ' + + 'confirmed a label on the issue is claiming observability gh-aw does not offer.', + ).toMatch(/never write that a label was (?:"verified", "confirmed on the issue", or\s*"checked"|verified, confirmed, or checked)/); + expect(prose).toMatch(/nothing here reads labels back/); + }, + ); + + it.each(BOTH_PATHS)('%s: rejects under-claiming as well as over-claiming', (_path, prose) => { + expect( + prose, + 'Reporting an omission for an item whose add_labels call was accepted manufactures a ' + + 'defect that did not occur — the mirror image of the over-claim, equally wrong.', + ).toMatch(/never (?:report an accepted operation as an omission|emit the heading for an owner that\s*\*did\* become an accepted label)/); + }); + + it.each(BOTH_PATHS)( + '%s: still requires the Non-roster agent values heading when an owner got no label', + (_path, prose) => { + // Preserved from the base stack. #1963 must not weaken it while tightening the + // positive direction. + expect(prose).toMatch(/`Non-roster agent values` heading is \*\*required\*\*/); + expect(prose).toMatch(/naming the value and\s*the issue it applied to|naming the value\s*and the issue it applied to/); + expect(prose).toMatch(/reports a\s*clean run that did not happen/); + }, + ); +}); + +describe('gh-aw: Activation bindings carry resolvable issue references (#1963)', () => { + it('replaces the bare created-issue-number placeholders that produced invalid JSON', () => { + expect( + workflow, + 'A bare number placeholder cannot be satisfied: the agent never learns a created ' + + "issue's real number during its turn.", + ).not.toContain('"issue":{created task issue number}'); + expect(workflow).not.toContain('"epic_issue":{created epic issue number}'); + }); + + it('specifies both binding references as quoted JSON strings', () => { + expect(activateSkill).toContain('"issue":"{task issue reference}"'); + expect(activateSkill).toContain('"epic_issue":"{epic issue reference}"'); + expect(activateProse).toMatch(/`issue` and `epic_issue` are \*\*JSON strings\*\*, never bare numbers/); + }); + + it('names invalid JSON as the reason quoting is mandatory, not style', () => { + expect( + activateProse, + 'Without the reason stated, a future edit "simplifies" the quotes away and silently ' + + 'breaks every bindings block.', + ).toMatch(/\*\*The quoting is load-bearing\.\*\*/); + expect(activateProse).toMatch(/it does not skip fenced code blocks/); + expect(activateProse).toMatch(/`"issue":#aw_task1` becomes `"issue":#42`, which is invalid JSON/); + expect(activateProse).toMatch(/`"issue":"#aw_task1"` becomes `"issue":"#42"`, which parses/); + }); + + it('uses temporary IDs for created items and verified real numbers for reused ones', () => { + expect(activateProse).toMatch(/\*\*Created this run:\*\* that item's own `temporary_id`, quoted/); + expect(activateProse).toMatch(/\*\*Reused or pre-existing\*\*[^.]*its verified real\s*number in the same quoted form/); + expect( + activateProse, + 'Inventing a number for a created issue is the failure mode this whole design avoids.', + ).toMatch(/never a real number\s*for an issue this run created/); + expect(activateProse).toMatch(/never infer an issue number/); + }); + + it('treats a surviving temporary ID as a failure rather than repairing it', () => { + expect(activateProse).toMatch(/was never resolved — that `create-issue` did\s*not land/); + expect(activateProse).toMatch(/Leave it rather than repairing it by hand/); + }); + + it('documents the same reference and label contract in the shared ontology', () => { + // The ontology is the artifact contract the deterministic checker consumes; a rule + // stated only in the skill body is invisible to that consumer's spec. + expect(ontologyProse).toMatch(/\*\*Issue references are quoted strings, never bare numbers\.\*\*/); + expect(ontologyProse).toMatch(/it does not skip fenced code\s*blocks/); + expect(ontologyProse).toMatch(/\*\*Reported labels mean accepted label operations\.\*\*/); + expect(ontologyProse).toMatch(/It does not assert the label was\s*observed on the issue/); + expect(ontologyProse).toMatch(/consumers MUST treat it as a failure rather than skipping or repairing it/); + }); + + it('never lets the ontology describe bindings as recording labels that were applied', () => { + // Regression guard for a real drift caught in review on this PR: the `Activation + // bindings:` paragraph said bindings record "epic labels reported as applied" while + // the paragraph directly below it correctly defined reported labels as *accepted* + // operations. The ontology is the file most likely to be read standalone, so the + // strong claim there silently re-introduced the exact defect this change removes — + // one consistently-wrong place became two inconsistently-wrong ones. + // + // This is deliberately a narrow, shape-targeted negative rather than a ban on the + // word "applied". The same file legitimately says safe outputs "are applied after + // the agent turn" — that sentence is the *justification* for the weaker claim, so a + // blanket match would forbid the correct prose along with the incorrect prose. + expect( + ontologyProse, + 'The ontology must not say bindings record labels "reported as applied". Reported ' + + 'labels assert an accepted add_labels operation, never observed application.', + ).not.toMatch(/(?:reported|recorded|listed)\s+as\s+applied/i); + expect( + ontologyProse, + 'No binding field may be described as a label already applied to, or present on, the issue.', + ).not.toMatch(/labels?\s+(?:already\s+)?(?:applied\s+to|present\s+on)\s+the\s+issue/i); + + // Positive half: the binding paragraph must state the accepted-operation semantics + // and point a standalone reader at the definition, so the fix cannot be reverted to + // a vaguer wording that merely dodges the negatives above. + expect(ontologyProse).toMatch( + /epic labels reported as accepted label operations \(defined below\)/, + ); + }); +}); + +describe('post-activation checker resolves binding references and fails closed (#1963)', () => { + const roster = parseRoster(` +## Members +| Name | Role | +|------|------| +| Kint | Lead | +`); + + function binding(issue: unknown, epicIssue: unknown) { + return { + task: '1', + issue, + epic: '2.1', + epic_issue: epicIssue, + agent: 'Kint', + epic_agents: ['kint'], + label: 'squad:kint', + epic_label: 'squad:kint', + }; + } + + function activated(bindings: object[]) { + return { + squad_artifact: 'activated', + schema_version: '1', + origin_issue: 1, + phases: [], + bindings, + }; + } + + const presentLabels = new Map([ + [42, new Set(['squad', 'squad:kint'])], + [43, new Set(['squad', 'squad:kint'])], + ]); + + it('accepts the resolved quoted form gh-aw substitution produces', () => { + // '"issue":"#aw_task1"' becomes '"issue":"#42"' after substitution — the shape the + // checker actually receives in production. + const result = validateBindings(activated([binding('#42', '#43')]), roster, presentLabels); + expect(result).toMatchObject({ skipped: false, checked: 1, epics: 1 }); + }); + + it('still accepts bare integers, so artifacts written before this contract validate', () => { + const result = validateBindings(activated([binding(42, 43)]), roster, presentLabels); + expect(result).toMatchObject({ skipped: false, checked: 1 }); + }); + + it('fails closed on an unresolved temporary ID instead of skipping or coercing it', () => { + expect( + () => validateBindings(activated([binding('#aw_task1', '#43')]), roster, presentLabels), + 'A surviving temporary ID means that create-issue never landed. Silently skipping it ' + + 'would report a clean activation for an issue that does not exist.', + ).toThrow('unresolved temporary ID'); + }); + + it('fails closed on an unresolved epic reference too', () => { + expect(() => validateBindings(activated([binding('#42', '#aw_epic1')]), roster, presentLabels)) + .toThrow('unresolved temporary ID'); + }); + + it('rejects a reference that is neither a number nor a resolvable reference', () => { + // Cross-repo substitution yields `owner/repo#42`; Squad activates same-repo only, so + // that form means something went wrong upstream. Reject rather than parse loosely. + expect(() => validateBindings(activated([binding('other/repo#42', '#43')]), roster, presentLabels)) + .toThrow('binding has no valid issue number'); + expect(() => validateBindings(activated([binding('', '#43')]), roster, presentLabels)) + .toThrow('binding has no valid issue number'); + }); + + it('keeps per-item correspondence across resolved references', () => { + // Two tasks in one epic, distinct issues: a checker that resolved references sloppily + // could collapse them and pass a duplicate. + const two = [ + { ...binding('#42', '#43'), task: '1' }, + { ...binding('#42', '#43'), task: '2' }, + ]; + expect(() => validateBindings(activated(two), roster, presentLabels)) + .toThrow('duplicate binding'); + }); +}); + +// --------------------------------------------------------------------------- +// Compiled-artifact verification +// --------------------------------------------------------------------------- +// +// The prose assertions above lock the reporting contract. This section proves the +// workflow carrying it still strict-compiles and that the safe outputs the contract +// depends on stay wired. Fails closed (never skips) per this repo's #1833/#1834 +// convention: an unmeasured contract is indistinguishable from a violated one. + +const GH_AW_INSTALL_HINT = + '`gh aw` is required to compile the workflow this gate inspects. Install it with ' + + '`gh extension install --pin v0.87.10 github/gh-aw` (the same pin ' + + '.github/workflows/squad-ci.yml uses, so local strict-compile and lock output match ' + + 'CI). This gate fails closed rather than skipping: an unmeasured contract is ' + + 'indistinguishable from a violated one (#1834).'; + +describe('gh-aw: the activation summary contract compiles in strict mode (#1963)', () => { + let compiledLock: string | null = null; + + function lockText(): string { + if (compiledLock !== null) return compiledLock; + + const versionProbe = spawnSync('gh', ['aw', '--version'], { encoding: 'utf8' }); + if (versionProbe.status !== 0) { + throw new Error(`gh aw --version failed. ${GH_AW_INSTALL_HINT}`); + } + + mkdirSync(TEST_WORKSPACES_DIR, { recursive: true }); + const workspace = mkdtempSync(join(TEST_WORKSPACES_DIR, 'activation-summary-')); + execFileSync('git', ['init', '--quiet'], { cwd: workspace }); + // gh-aw resolves cross-workflow references against a `.github/workflows/` directory + // relative to the compiled file. This repo ships gh-aw *source* from a top-level + // `workflows/` dir; consumers install it via `gh aw add`. Mirror that layout. + cpSync(WORKFLOWS_DIR, join(workspace, '.github', 'workflows'), { recursive: true }); + execFileSync( + 'gh', + ['aw', 'compile', '.github/workflows/squad.md', '--strict', '--approve', '--no-check-update'], + { cwd: workspace, encoding: 'utf8', stdio: 'pipe' }, + ); + + const lockPath = join(workspace, '.github', 'workflows', 'squad.lock.yml'); + if (!existsSync(lockPath)) { + throw new Error( + 'gh aw compile produced no squad.lock.yml — the compiled artifact this gate ' + + `inspects is absent, so the contract is unmeasured. ${GH_AW_INSTALL_HINT}`, + ); + } + compiledLock = readText(lockPath); + return compiledLock; + } + + it('strict-compiles the workflow carrying the reporting contract', () => { + expect(lockText().length).toBeGreaterThan(0); + }); + + it('keeps add_labels callable — the only operation a reported label may cite', () => { + expect(lockText()).toMatch(/"tools":\[[^\]]*"add_labels"[^\]]*\]/); + }); + + it('keeps temporary-ID substitution reachable for the quoted binding references', () => { + // Quoted `#aw_…` references only resolve because create_issue requires a temporary ID + // and gh-aw rewrites references to it. Without this, every binding stays unresolved. + const compiled = lockText().replace(/\\"/g, '"'); + expect(compiled).toMatch(/"create_issue":\{[^}]*"require_temporary_id":true/); + }); + + it('leaves label writes in the safe-output job, which is why "accepted" is the honest claim', () => { + // The agent job holds only `issues: read`; a separate post-agent job performs the + // write. That job boundary is precisely why no summary may claim a verified GitHub + // result — the agent turn ends before any label is applied. + const compiled = lockText(); + const agentJobAt = compiled.indexOf('\n agent:\n'); + const safeOutputsJobAt = compiled.indexOf('\n safe_outputs:\n'); + expect(agentJobAt, '"agent:" job is missing from the compiled lock file').toBeGreaterThan(-1); + expect(safeOutputsJobAt, '"safe_outputs:" job is missing from the compiled lock file').toBeGreaterThan(agentJobAt); + + const permsMatch = compiled.slice(agentJobAt, safeOutputsJobAt).match(/permissions:\n((?:\s{6}\S.*\n)+)/); + expect(permsMatch, 'agent job permissions block not found').not.toBeNull(); + const permsBlock = permsMatch ? permsMatch[1] : ''; + expect(permsBlock).toContain('issues: read'); + expect(permsBlock).not.toContain('issues: write'); + }); +}); diff --git a/test/gh-aw-agent-binding-correspondence.test.ts b/test/gh-aw-agent-binding-correspondence.test.ts index 798f1db9e..0cb006832 100644 --- a/test/gh-aw-agent-binding-correspondence.test.ts +++ b/test/gh-aw-agent-binding-correspondence.test.ts @@ -155,12 +155,26 @@ describe('gh-aw: agent-binding correspondence (#1859, #1860)', () => { }); it('requires the summary to report labels applied, not intended', () => { + // #1860 required the summary to record what happened rather than restate the plan, + // and pinned the sentence that made `create-issue` the evidence. #1963 found that + // evidence to be unsound: `create-issue`'s `labels:` field silently drops names the + // repository lacks (#1959), so a successful call proves nothing about labels. The + // condition is now an accepted `add_labels` call for that same issue — strictly + // stronger, and #1860's own defect (squad:kint attributed to an epic that never + // received it) is still caught, now for the right reason. expect( - /only after that issue's `create-issue` call returned successfully carrying it/i.test(prose), + /only after an `add_labels` call carrying that label\s+was accepted for that same issue/i.test(prose), `#1860: the summary attributed squad:kint to epic #6, which never received it. ` + `The summary must be a record of what happened, not a restatement of the plan.`, ).toBe(true); + expect( + /A successful `create-issue` is \*\*not\*\* evidence/i.test(prose), + `#1963: create-issue must be ruled out explicitly. Leaving it unaddressed lets a ` + + `run cite issue creation as proof a label landed, which is exactly the ` + + `over-claim #1860 reported.`, + ).toBe(true); + expect( /Omitting the heading while omitting the label/i.test(prose), `The two omissions compound: dropping a label AND its "Non-roster agent values" ` + diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index 5677e4507..90c2bf8e7 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -884,7 +884,22 @@ describe('gh-aw: prompt budget & planning import regression', () => { // names as making a raise legitimate. #1962's follow-up trims bought some room back // but not enough: combined source measures 164.8 KB after both changes, still over // 160, so the raise stays. 170 KB leaves a usable margin without removing the signal. - const SOURCE_GROWTH_BUDGET_KB = 170; + // + // Raised 170 → 173 KB by #1963, which makes both activation paths report actual + // accepted label-operation outcomes and fixes the `Activation bindings:` JSON to carry + // quoted temporary-ID references. Nearly all of that prose lands inside the + // `squad-plan-activate` and `squad-plan-accept` inline skill blocks; only the shared + // ontology's binding contract is ambient, and "keeps the ambient prompt under 40 KB" + // still passes at ~32 KB — again the condition above that makes a raise legitimate. + // + // Measured after rebasing onto dev (i.e. with #1966 already squash-merged, so this + // counts #1963's bytes only): 175 519 B = 171.4 KB. 172 KB would leave 609 B of + // headroom, which reproduces the near-zero-margin failure mode described above; 173 KB + // leaves 1 633 B. Deliberately not set higher: #1964 is projected to push the combined + // total to ~180 002 B = 175.8 KB, but it has not merged, and pre-raising this guard for + // an unmerged branch would hide growth that has not happened yet. #1964 raises it when + // it lands, against its own measurement. + const SOURCE_GROWTH_BUDGET_KB = 173; const SOURCE_GROWTH_BUDGET_BYTES = SOURCE_GROWTH_BUDGET_KB * 1024; it('squad-planning-ontology.md is in the imports list', () => { diff --git a/workflows/shared/squad-planning-ontology.md b/workflows/shared/squad-planning-ontology.md index 7620c012d..c85b49920 100644 --- a/workflows/shared/squad-planning-ontology.md +++ b/workflows/shared/squad-planning-ontology.md @@ -310,17 +310,35 @@ been run, and an omitted row is not a pass. The activation artifact body also carries an `Activation bindings:` fenced JSON block containing a non-empty array. Each entry -maps a plan task number and raw agent assignment to its returned task issue number, -epic identifier, returned epic issue number, and the epic's complete distinct agent +maps a plan task number and raw agent assignment to its task issue reference, +epic identifier, epic issue reference, and the epic's complete distinct agent set from the full accepted plan (including other activation phases). It records both task and derived -epic labels actually applied, or their omission reasons (`multi-owner` or -`non-roster`) when policy requires bare `squad`. The special `@copilot` assignment +epic labels reported as accepted label operations (defined below), or their omission reasons +(`multi-owner` or `non-roster`) when policy requires bare `squad`. The special `@copilot` assignment records the actual `squad:copilot` label. This mapping is mandatory for `phases-activated` and `activated` artifacts. It remains in the body rather than the safe-output `data` envelope because gh-aw expands nested data schemas beyond GitHub's expression-size limit. The post-activation checker can still fail closed without matching model-authored titles. +**Issue references are quoted strings, never bare numbers.** `issue` and `epic_issue` +carry a `#`-prefixed reference in a JSON string: an item's own gh-aw `temporary_id` +(`"#aw_task3"`) when this run created it, or its verified real number (`"#123"`) when the +item was reused or matched by title. The agent never learns a created issue's real number +during its turn, so it never writes one; gh-aw rewrites `#aw_…` references in a comment body +to `#{real number}` once the issue exists. Quoting is required for validity: that +substitution is plain text replacement across the whole body — it does not skip fenced code +blocks — and preserves the `#`, so bare `"issue":#aw_task3` becomes invalid `"issue":#42` +while quoted becomes `"issue":"#42"`. A reference still matching `#aw_…` was never resolved; +consumers MUST treat it as a failure rather than skipping or repairing it. + +**Reported labels mean accepted label operations.** A `label` / `epic_label` asserts that an +`add_labels` safe output carrying that label was accepted for that same issue, targeted by +its temporary ID or verified real number. It does not assert the label was observed on the +issue — safe outputs are applied after the agent turn — and never means it was carried by +`create-issue`, whose `labels:` field cannot create a missing label. Verifying bindings +against the labels actually present is the post-activation checker's job. + --- ## 4. Structured Artifact Registry diff --git a/workflows/squad.md b/workflows/squad.md index 2bf5c76bb..6ec67b752 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1345,8 +1345,9 @@ semantics, so this is safe under Step 1a's idempotency path. Labels must have descriptions and intentional colors when they already exist; a label auto-provisioned by `create-if-missing` on a fresh repository instead receives gh-aw's deterministic color and an empty description — that is expected, not a failure, and must not be -reported as one. Report only the labels a successful `add_labels` call actually -applied; never a label that was skipped, deferred, or merely intended. +reported as one. Report only the labels an accepted `add_labels` call carried for that +same issue; never a label that was skipped, deferred, or merely intended, and never one +attributed to `create-issue` (see Step 4, Label reporting). ##### Step 3: Preserve Dependencies @@ -1371,11 +1372,26 @@ and whether dependencies use native edges or the body-reference fallback. Never claim an epic, phase issue, sub-issue relationship, or native dependency edge that was not created. +**Label reporting — accepted operations only.** Identical semantics to +`squad-plan-activate` Step 4. A label reaches an activated issue through exactly one route: +an accepted `add_labels` operation targeting that issue. Report `squad:{owner}` only when +this run made an `add_labels` call carrying that label and targeting that same issue — by +its own `temporary_id`, or by its verified real number for a reused issue. A successful +`create-issue` is **not** evidence: its `labels:` field cannot land a label on a fresh +repository, so no summary may say a label was carried by, applied by, or included in issue +creation. Never report a label merely computed, intended, skipped, or deferred, never borrow +another item's label operation, and never report an accepted operation as an omission. + +`add_labels` is a safe output: this run knows only that the call was accepted for a specific +target, never the GitHub API result. State it at that strength — never write that a label +was verified, confirmed, or checked on the issue, because nothing here reads labels back. + Whenever an accepted `Owner` did not become a `squad:{owner}` label — a multi-owner phase issue, or a value certified by neither the roster nor `@copilot` — a `Non-roster agent values` heading is **required** in this summary, naming the value and the issue it applied to. Omitting the label while omitting the heading reports a -clean run that did not happen. +clean run that did not happen. Conversely, never emit the heading for an owner that +*did* become an accepted label — that manufactures a defect that did not occur. ##### Step 5: Update Fast-Path Lifecycle @@ -1871,12 +1887,16 @@ Count expected issues before starting. If total > 50: recommend phased activatio `Agent` cell, an epic's derived task-set — and never from the row above it, the parent epic, or the previous call. Verify per issue; membership across the run is not evidence. 8. **Report what was applied, not what was intended.** The activation summary may name a - `squad:{agent}` label for an issue only after that issue's `create-issue` call returned - successfully carrying it. Never state a label that was skipped, omitted, deferred, or - assumed. Whenever an `Agent` value did not become a label — multi-owner epic, uncertified - name, unavailable label — the `Non-roster agent values` heading is **required**, and must - name the value and the issue it applied to. Omitting the heading while omitting the label - reports a clean run that did not happen. + `squad:{agent}` label for an issue only after an `add_labels` call carrying that label + was accepted for that same issue — targeted by its own `temporary_id`, or by its verified + real number for a reused issue. A successful `create-issue` is **not** evidence: its + `labels:` field cannot land a label on a fresh repository, so a label is never "carried + by" issue creation. Never state a label that was skipped, omitted, deferred, or assumed, + and never report an accepted label operation as an omission. Whenever an `Agent` value did + not become a label — multi-owner epic, uncertified name, unavailable label — the + `Non-roster agent values` heading is **required**, and must name the value and the issue + it applied to. Omitting the heading while omitting the label reports a clean run that did + not happen. See Step 4's Label reporting section for the full contract. **Label provisioning.** The `add-labels` safe output (`allowed: [squad, "squad:*"]`, `create-if-missing: true`) auto-creates `squad` and any `squad:{agent}` label the first @@ -1958,9 +1978,31 @@ activation over edge creation. Phase artifact: `data: {"squad_artifact":"phases-activated","schema_version":"1","origin_issue":{issue_number},"phases":[{accumulated}]}` → `## ✅ Phase {N} Activated — {count} issues` + issue table + remaining phases table. -Every phase and full activation artifact body MUST include an `Activation bindings:` fenced JSON block containing a non-empty array built only from successful `create-issue` results. Emit one object per created/recognized task: +Every phase and full activation artifact body MUST include an `Activation bindings:` fenced JSON block containing a non-empty array built only from accepted activation operations. Emit one object per created/recognized task: + +`{"task":"{plan # cell}","issue":"{task issue reference}","epic":"{Epic cell}","epic_issue":"{epic issue reference}","agent":"{raw Agent cell}","epic_agents":["{all distinct lowercased Agent cells for this epic across the full accepted plan}"],"label":"squad:{lowercased Agent cell}","epic_label":"squad:{sole lowercased epic task agent}"}`. For `@copilot`, use `squad:copilot`. Every binding for one epic MUST carry the same complete `epic_agents` set, including agents assigned in other activation phases. + +###### Issue references in bindings — quoted, never bare -`{"task":"{plan # cell}","issue":{created task issue number},"epic":"{Epic cell}","epic_issue":{created epic issue number},"agent":"{raw Agent cell}","epic_agents":["{all distinct lowercased Agent cells for this epic across the full accepted plan}"],"label":"squad:{lowercased Agent cell}","epic_label":"squad:{sole lowercased epic task agent}"}`. For `@copilot`, use `squad:copilot`. Every binding for one epic MUST carry the same complete `epic_agents` set, including agents assigned in other activation phases. +`issue` and `epic_issue` are **JSON strings**, never bare numbers, and never a real number +for an issue this run created. + +- **Created this run:** that item's own `temporary_id`, quoted — `"issue":"#aw_task{N}"`, + `"epic_issue":"#aw_epic{K}"`. gh-aw rewrites an `#aw_…` reference in a comment body to + `#{real number}` once the issue exists, so the posted artifact carries the real number + without this run predicting one. +- **Reused or pre-existing** (Step 1 idempotent rerun, dedup-by-title): its verified real + number in the same quoted form — `"issue":"#123"`. One shape covers both. + +**The quoting is load-bearing.** gh-aw's substitution is a plain text replacement over the +whole body — it does not skip fenced code blocks — and it keeps the `#`. Bare, +`"issue":#aw_task1` becomes `"issue":#42`, which is invalid JSON and fails the whole block. +Quoted, `"issue":"#aw_task1"` becomes `"issue":"#42"`, which parses. Never emit a bare +`#aw_…`, a bare number, or a `{created task issue number}` placeholder in these two fields. + +An `#aw_…` surviving into the posted artifact was never resolved — that `create-issue` did +not land. Leave it rather than repairing it by hand: the consumer fails closed on it, which +is correct. For a multi-owner epic, omit `epic_label` and set `"epic_omission_reason":"multi-owner"` on each of its task bindings. For a task whose agent is not certified by TG-2, omit `label` and set `"omission_reason":"non-roster"`; if that task is the epic's sole owner, likewise omit `epic_label` and set `"epic_omission_reason":"non-roster"`. Never omit a created task from `bindings`, never infer an issue number, and never emit an empty array. The deterministic post-activation workflow treats missing, empty, malformed, or unresolved bindings as a failure. The safe-output schema deliberately uses one uniform task-binding shape because gh-aw's data schema dialect does not support conditional `if`/`then` or `allOf`; the checker enforces activation-only presence and cross-row epic consistency. @@ -1970,6 +2012,40 @@ Full artifact: `data: {"squad_artifact":"activated","schema_version":"1","origin Terminal (last phase): emit `data: {"squad_artifact":"activated","schema_version":"1","origin_issue":{issue_number},"phases":[{all_phases}]}` with an "All Phases Activated" heading and the accumulated `Activation bindings:` JSON array. +###### Label reporting — accepted operations only + +A label reaches an activated issue through exactly one route: an accepted `add_labels` +operation targeting that issue. `create-issue`'s `labels:` field never lands a label this +workflow can claim — it silently drops names the repository lacks — so it is never evidence. + +**The rule.** Report `squad:{agent}` for an issue only when this run made an `add_labels` +call that carried that label and targeted that same issue — by its own `temporary_id`, or by +its verified real number for a reused issue. Every `label` and `epic_label` in the bindings +block, and every label named in the prose or issue tables, MUST trace to such a call. + +**Forbidden:** reporting a label because `create-issue` succeeded or its `labels:` field +named it (a successful `create-issue` means an issue was requested — nothing more); reporting +a label that was computed or intended but whose `add_labels` call was never made, was +skipped, or was rejected; reporting a label from another item's `add_labels` call +(per-issue correspondence holds exactly as in Label Pre-flight Step 7); and reporting an +omission when that item's call was in fact made and accepted — a silent under-claim is as +wrong as an over-claim. + +**What "accepted" means.** `add_labels` is a safe output: the call is accepted and queued +this turn, and gh-aw applies it in the post-agent job. This run has evidence only that the +operation was accepted *for a specific target*, never the GitHub API result. State it at +that strength — never write that a label was "verified", "confirmed on the issue", or +"checked", because nothing here reads labels back. The deterministic post-activation checker +compares these bindings against the labels actually present; over-claiming defeats it. + +**Omission is reported, never inferred.** Whenever an `Agent` value did not become a +`squad:{agent}` label — multi-owner epic, uncertified name, or a label operation not made or +not accepted — the `Non-roster agent values` heading is **required**, naming the value and +the issue it applied to, and the matching binding carries its `omission_reason` / +`epic_omission_reason`. Applying bare `squad` while omitting the heading reports a clean run +that did not happen. Conversely, never emit the heading for an owner that *did* become an +accepted label — that manufactures a defect. + ##### Step 5: Update Lifecycle Phase: `🔄 Phase {N} of {total} activated`. Next: accept/activate next phase.