diff --git a/.github/workflows/squad-agent-binding-check.yml b/.github/workflows/squad-agent-binding-check.yml index da5fe24eb..6373c5653 100644 --- a/.github/workflows/squad-agent-binding-check.yml +++ b/.github/workflows/squad-agent-binding-check.yml @@ -33,7 +33,8 @@ jobs: }); const activationComments = comments .filter(comment => new Date(comment.created_at) <= completed) - .filter(comment => /Structured data:/i.test(comment.body ?? '') && /activated/i.test(comment.body ?? '')) + .filter(comment => /Structured data:/i.test(comment.body ?? '') + && /"squad_artifact"\s*:\s*"?(?:phases-activated|activated|phases-accepted|plan-accepted)/i.test(comment.body ?? '')) .map(comment => ({ body: comment.body, issue: Number(comment.issue_url.split('/').at(-1)), diff --git a/scripts/check-agent-binding.mjs b/scripts/check-agent-binding.mjs index e7db8c6b7..975eda3ef 100644 --- a/scripts/check-agent-binding.mjs +++ b/scripts/check-agent-binding.mjs @@ -1,11 +1,44 @@ import { readFile } from 'node:fs/promises'; import { pathToFileURL } from 'node:url'; -const ACTIVATION_ARTIFACTS = new Set(['activated', 'phases-activated']); +// `activated` / `phases-activated` come from the granular `/squad plan activate` path +// (`squad-plan-activate`); `plan-accepted` / `phases-accepted` come from the recommended +// `/squad activate` fast path (`squad-plan-accept`). Both paths create or recognize issues +// and apply labels the same way, so both carry the same mandatory `Activation bindings:` +// contract — see workflows/shared/squad-planning-ontology.md and the E4 preflight package A +// fast-path artifact-integrity fix. +const ACTIVATION_ARTIFACTS = new Set(['activated', 'phases-activated', 'plan-accepted', 'phases-accepted']); +const ACTIVATION_ARTIFACT_PATTERN = new RegExp( + `"squad_artifact"\\s*:\\s*"?(?:${[...ACTIVATION_ARTIFACTS].join('|')})`, + 'i', +); const VALID_OMISSIONS = new Set(['multi-owner', 'non-roster']); const TEMPORARY_ID = /^#?aw_[A-Za-z0-9_]{3,12}$/i; const RESOLVED_REFERENCE = /^#?(\d+)$/; +// Standalone certainty claims a label-operation report may never make: safe outputs like +// `add_labels` are applied in a post-agent job, so an activation/acceptance run only ever +// knows a call was *accepted for a specific target* — never that GitHub applied it. Word +// boundaries keep this from matching substrings ("unverified", "prechecked"). +const FORBIDDEN_LABEL_CLAIM_WORDS = ['applied', 'received', 'landed', 'verified', 'confirmed', 'checked']; +// A clause is treated as a compliant, honest non-claim (never flagged) when it explicitly +// negates the outcome — e.g. "not verified", "never confirmed" — since that is the accurate, +// honest statement the contract requires, not an over-claim. +const NEGATION_NEARBY = /\b(never|not|no|n't|without|cannot|can't|won't|isn't|wasn't|doesn't|didn't|nor)\b/i; +// Scope the forbidden-word scan to clauses that are actually reporting a label operation — +// mentioning "label"/"labels" or a `squad:*` token — rather than scanning the whole comment. +// A blanket scan would false-positive on an unrelated quoted task title like "Verified Email +// Addresses" sitting in the created-issues table. +const LABEL_OPERATION_CONTEXT = /\blabels?\b|\bsquad:[a-z0-9_.-]+\b/i; +// Protects a `squad:{agent}` token's colon from being mistaken for a clause boundary while +// splitting a line into clauses. Requires each embedded `.` to be followed by another name +// character (e.g. `squad:kint.jones`), so a sentence-ending period right after an agent name +// (`squad:kint.`) is left alone as a real delimiter, not swallowed into the token. +const SQUAD_TOKEN = /\bsquad:([a-z0-9_-]+(?:\.[a-z0-9_-]+)*)/gi; +// Placeholder standing in for a protected token's colon while clauses are split; chosen to +// never collide with real comment text. +const SQUAD_COLON_PLACEHOLDER = '\u0000'; + function normalize(value) { return typeof value === 'string' ? value.trim().toLowerCase() : ''; } @@ -38,10 +71,26 @@ function resolveIssueReference(value, description, invalidMessage) { throw new Error(invalidMessage); } +/** + * Best-effort issue number extraction for label prefetching. Unlike + * `resolveIssueReference()`, this never throws: an unresolved/malformed + * reference is simply skipped here so `validateTaskBinding()` can report the + * real error (e.g. an unresolved temporary ID) instead of a misleading + * "labels could not be resolved" failure. + */ +export function extractIssueNumber(value) { + if (Number.isInteger(value)) return value; + if (typeof value === 'string') { + const resolved = RESOLVED_REFERENCE.exec(value.trim()); + if (resolved) return Number(resolved[1]); + } + return undefined; +} + export function parseStructuredData(comment) { const blocks = [...comment.matchAll(/Structured data:\s*```json\s*([\s\S]*?)```/gi)]; if (blocks.length === 0) { - if (/Structured data:/i.test(comment) && /"squad_artifact"\s*:\s*"?(?:phases-)?activated/i.test(comment)) { + if (/Structured data:/i.test(comment) && ACTIVATION_ARTIFACT_PATTERN.test(comment)) { throw new Error('activation structured data block could not be parsed'); } return null; @@ -65,6 +114,87 @@ export function parseStructuredData(comment) { return parsed; } +/** + * Strip content a forbidden-word scan must never see as a claim: fenced/inline code and + * quoted strings. A quoted work-item title ("Verified Email Addresses") or a JSON fragment + * inside backticks is data being described, not a certainty claim about a label operation. + */ +function stripQuotedAndCode(line) { + return line + .replace(/`[^`]*`/g, ' ') + .replace(/"[^"]*"/g, ' ') + .replace(/'[^']*'/g, ' '); +} + +/** + * Split a line into clauses on clause/sentence-ending punctuation (`.`, `,`, `;`, `:`) so a + * negation and a certainty claim are only ever treated as connected when they actually share + * the same clause. Without this, "No issues were skipped: Label squad:kint was verified for + * #42." would let the unrelated negation on "skipped" blanket-suppress the real, separate + * claim that follows the colon — and a claim about a *different* subject sharing a line with + * a label mention (e.g. "Reported squad:kint for issue #123, its verified real number") would + * wrongly inherit label-operation context from a clause it isn't part of. + * + * `squad:{agent}` tokens are protected first so the colon inside them is never itself treated + * as a clause boundary. + */ +function splitClauses(line) { + const guarded = line.replace(SQUAD_TOKEN, (_match, agent) => `squad${SQUAD_COLON_PLACEHOLDER}${agent}`); + return guarded + .split(/[.,;:]+/) + .map(clause => clause.replaceAll(SQUAD_COLON_PLACEHOLDER, ':')) + .filter(clause => clause.trim().length > 0); +} + +/** + * Reject standalone certainty claims ("applied", "received", "landed", "verified", + * "confirmed", "checked") in an activation/acceptance comment's label-operation reporting. + * `add_labels` is a safe output applied in a post-agent job — the run only ever knows a call + * was *accepted* for a target, never that GitHub actually applied it. A summary claiming more + * than that is over-claiming an outcome nothing here observed. + * + * Deliberately scoped, not a blanket whole-comment or whole-line scan: + * - Markdown table rows (`| ... |`) are skipped — that's where quoted work-item titles live. + * - Fenced code blocks, inline code, and quoted strings are stripped before matching, so a + * quoted title or JSON fragment containing a forbidden word never counts. + * - Each line is split into clauses (see `splitClauses`), and label-operation context + * (`label`/`labels`/`squad:*`), the forbidden word, and any negation must all be found + * within the *same* clause. This is what lets a genuinely unrelated negation elsewhere on + * the line ("No issues were skipped: ... was verified ...") fail to suppress a real claim, + * while a forbidden word describing an unrelated subject in a different clause on the same + * line as a label mention ("Reported squad:kint for issue #123, its verified real number") + * is correctly out of scope — its clause carries no label-operation context at all. + * - A clause that explicitly negates the claim ("was not verified", "never confirmed") is + * the honest, compliant statement the contract requires, so it is never flagged. + * + * No-op for artifact types outside the activation contract (fast-path `plan-accepted` / + * `phases-accepted` and granular `activated` / `phases-activated`). + */ +export function assertAcceptedOnlyLabelWording(commentBody, artifactType) { + if (!ACTIVATION_ARTIFACTS.has(artifactType)) return { skipped: true }; + const body = typeof commentBody === 'string' ? commentBody : ''; + const withoutFences = body.replace(/```[\s\S]*?```/g, ' '); + for (const rawLine of withoutFences.split(/\r?\n/)) { + const trimmed = rawLine.trim(); + if (!trimmed || trimmed.startsWith('|')) continue; + const scrubbed = stripQuotedAndCode(trimmed); + for (const clause of splitClauses(scrubbed)) { + if (!LABEL_OPERATION_CONTEXT.test(clause)) continue; + if (NEGATION_NEARBY.test(clause)) continue; + for (const word of FORBIDDEN_LABEL_CLAIM_WORDS) { + if (new RegExp(`\\b${word}\\b`, 'i').test(clause)) { + throw new Error( + `label-operation report uses forbidden certainty claim "${word}" — report label ` + + `operations as accepted only, never applied/received/landed/verified/confirmed/` + + `checked: "${trimmed}"`, + ); + } + } + } + } + return { skipped: false }; +} + export function parseRoster(teamMarkdown) { const lines = teamMarkdown.replace(/\r/g, '').split('\n'); const section = lines.findIndex(line => /^##\s+Members\s*$/.test(line.trim())); @@ -250,8 +380,9 @@ async function main() { for (const comment of comments) { const artifact = parseStructuredData(comment.body ?? ''); if (!artifact || !ACTIVATION_ARTIFACTS.has(artifact.squad_artifact)) continue; + assertAcceptedOnlyLabelWording(comment.body ?? '', artifact.squad_artifact); const issues = Array.isArray(artifact.bindings) - ? artifact.bindings.flatMap(binding => [binding?.issue, binding?.epic_issue]).filter(Number.isInteger) + ? artifact.bindings.flatMap(binding => [extractIssueNumber(binding?.issue), extractIssueNumber(binding?.epic_issue)]).filter(Number.isInteger) : []; const labels = await fetchLabels(args.repo, [...new Set(issues)], token); const result = validateActivation(artifact, roster, labels, comment.issue); diff --git a/test/check-agent-binding.test.ts b/test/check-agent-binding.test.ts index b17ff77fa..fb40f6e79 100644 --- a/test/check-agent-binding.test.ts +++ b/test/check-agent-binding.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; import { + extractIssueNumber, parseRoster, parseStructuredData, validateActivation, @@ -313,3 +314,35 @@ Structured data: }), 9)).toThrow('does not match comment issue'); }); }); + +describe('extractIssueNumber() label-prefetch reference resolution (#1980)', () => { + it('passes a bare integer through unchanged', () => { + expect(extractIssueNumber(42)).toBe(42); + }); + + it('resolves a quoted "#42" reference to 42', () => { + expect(extractIssueNumber('#42')).toBe(42); + }); + + it('returns undefined for an unresolved temporary ID like "#aw_task1"', () => { + expect(extractIssueNumber('#aw_task1')).toBeUndefined(); + }); + + it('returns undefined for non-numeric garbage', () => { + expect(extractIssueNumber('not-a-number')).toBeUndefined(); + }); + + it("extracts only the resolvable issue numbers from a mixed-reference binding list, matching main()'s label-prefetch usage", () => { + // Mirrors main()'s `bindings.flatMap(binding => [extractIssueNumber(binding?.issue), extractIssueNumber(binding?.epic_issue)]).filter(Number.isInteger)` + // with a quoted resolved reference, a bare integer, and an unresolved temporary ID mixed + // together, since that is exactly the shape a real activation artifact produces. + const bindings = [ + { issue: '#17', epic_issue: 6 }, + { issue: 18, epic_issue: '#aw_task2' }, + ]; + const issues = bindings + .flatMap(binding => [extractIssueNumber(binding?.issue), extractIssueNumber(binding?.epic_issue)]) + .filter(Number.isInteger); + expect(issues).toEqual([17, 6, 18]); + }); +}); diff --git a/test/gh-aw-activation-artifact-integrity.test.ts b/test/gh-aw-activation-artifact-integrity.test.ts new file mode 100644 index 000000000..e77606150 --- /dev/null +++ b/test/gh-aw-activation-artifact-integrity.test.ts @@ -0,0 +1,519 @@ +/** + * E4 preflight package A — fast-path `/squad activate` artifact integrity. + * + * Before this change, only the granular `/squad plan activate` path + * (`squad-plan-activate`, artifacts `activated` / `phases-activated`) was required to emit + * a non-empty `Activation bindings:` block, and only that path's checker/collector wiring + * covered it. The recommended fast path (`squad-plan-accept`, artifacts `plan-accepted` / + * `phases-accepted`) creates and labels issues identically but had no such requirement, no + * checker coverage, and the CI collector (`.github/workflows/squad-agent-binding-check.yml`) + * filtered on a literal `/activated/i` substring that a `plan-accepted`/`phases-accepted` + * comment never contains — so a fast-path activation with omitted or empty bindings was + * invisible to the deterministic post-activation check. + * + * This suite locks four things: + * 1. Both activation paths use the exact same `Label operations accepted` heading text. + * 2. The fast path's output template requires a non-empty `Activation bindings:` block, + * with the same shape/quoting/omission-reason contract as the granular path. + * 3. The deterministic checker (`scripts/check-agent-binding.mjs`) validates `plan-accepted` + * / `phases-accepted` bindings exactly like `activated` / `phases-activated`, and rejects + * standalone certainty claims ("applied", "received", "landed", "verified", "confirmed", + * "checked") in label-operation reporting — scoped per-clause (not per-line or a blanket + * whole-comment scan), so quoted titles, unrelated sections, substrings, a forbidden word + * describing an unrelated subject sharing a line with a label token, and negations never + * false-positive, while a negation elsewhere on the line never blanket-suppresses a real, + * separate claim. + * 4. The CI collector actually passes fast-path artifact comments to that checker. + * + * Out of scope (per the E4 preflight package A brief): workflow discriminators, docs + * sequencing, draft PR UX, and every other E4 finding. + */ + +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { + assertAcceptedOnlyLabelWording, + parseRoster, + parseStructuredData, + validateActivation, + validateBindings, +} from '../scripts/check-agent-binding.mjs'; + +/** 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 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 COLLECTOR_YML = join(process.cwd(), '.github', 'workflows', 'squad-agent-binding-check.yml'); + +const workflow = readText(SQUAD_WORKFLOW); +const ontology = readText(ONTOLOGY); + +/** `squad-plan-activate` — the `/squad plan activate` granular 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); + +/** + * `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); + +/** Both activation paths, so parity assertions cannot pass by covering only one. */ +const BOTH_PATHS: ReadonlyArray = [ + ['/squad plan activate (squad-plan-activate)', activateSkill], + ['/squad activate fast path (squad-plan-accept)', acceptSkill], +]; + +// --------------------------------------------------------------------------- +// 1. Heading parity — both paths use the exact same literal heading. +// --------------------------------------------------------------------------- + +describe('gh-aw: both activation paths use the exact "Label operations accepted" heading', () => { + it.each(BOTH_PATHS)('%s: renders the heading verbatim', (_path, skillText) => { + expect(skillText).toMatch(/^#{1,6} Label operations accepted\s*$/m); + }); + + it('never reintroduces the old, non-identical heading text in either path', () => { + expect(workflow).not.toContain('Label reporting — accepted operations only'); + expect(workflow).not.toContain('Label reporting'); + }); + + it('the two headings are byte-identical, not merely similar', () => { + const activateHeading = activateSkill.match(/^#{1,6} Label operations accepted\s*$/m)?.[0].replace(/^#+\s*/, '').trim(); + const acceptHeading = acceptSkill.match(/^#{1,6} Label operations accepted\s*$/m)?.[0].replace(/^#+\s*/, '').trim(); + expect(activateHeading).toBe('Label operations accepted'); + expect(acceptHeading).toBe('Label operations accepted'); + expect(activateHeading).toBe(acceptHeading); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Fast-path output template requires non-empty Activation bindings. +// --------------------------------------------------------------------------- + +/** + * Shared assertion used both as a direct positive check against the real committed skill + * text, and — mutated — as a mutation-test kill switch. A mutation that survives this + * function despite deleting or hollowing out the contract would mean the test suite cannot + * actually detect the regression it claims to guard. + */ +function assertBindingsContractPresent(skillText: string): void { + const prose = skillText.replace(/\s+/g, ' '); + expect(prose).toMatch( + /MUST include an `Activation bindings:` fenced JSON block containing a non-empty array/, + ); + expect(prose).toMatch(/never emit an empty array/); + expect(prose).toMatch(/#{1,6} Label operations accepted/); +} + +describe('gh-aw: fast-path (`squad-plan-accept`) requires non-empty Activation bindings (#1958 E4 preflight A)', () => { + it('passes against the real committed fast-path skill body', () => { + expect(() => assertBindingsContractPresent(acceptSkill)).not.toThrow(); + }); + + it('passes against the real committed granular skill body (parity baseline)', () => { + expect(() => assertBindingsContractPresent(activateSkill)).not.toThrow(); + }); + + it('states the identical binding shape/quoting/omission contract as the granular path', () => { + const prose = acceptSkill.replace(/\s+/g, ' '); + expect(prose).toMatch( + /identical binding shape, quoting, and omission-reason semantics as `squad-plan-activate` Step 4/, + ); + expect(prose).toMatch(/never bare numbers/); + expect(prose).toMatch(/a surviving `#aw_…` reference means `create-issue` never\s*landed/); + }); + + it('the checker treats a missing/empty fast-path bindings block as a failure, stated explicitly', () => { + const prose = acceptSkill.replace(/\s+/g, ' '); + expect(prose).toMatch( + /missing, empty, malformed, or unresolved bindings block on a `plan-accepted` or\s*`phases-accepted` artifact as a failure exactly as it does for `activated`\s*and `phases-activated`/, + ); + }); + + it('both artifact-data lines for the fast path carry the Activation bindings JSON array', () => { + expect(acceptSkill).toContain( + 'Phase-specific: `data: {"squad_artifact":"phases-accepted","schema_version":"1","origin_issue":{issue_number},"phases":[{accumulated}]}` → Phase accepted table + remaining phases table + the `Activation bindings:` JSON array.', + ); + expect(acceptSkill).toContain( + 'Full (no phases): `data: {"squad_artifact":"plan-accepted","schema_version":"1","origin_issue":{issue_number},"phases":[]}` → All issues table + the `Activation bindings:` JSON array.', + ); + }); + + // --- Mutation tests: prove the assertion above actually fails when the committed source + // --- is hollowed out, rather than only ever passing by construction. + it.each(BOTH_PATHS)( + '%s: mutation kill — deleting the Activation bindings requirement sentence fails the contract check', + (_path, skillText) => { + const mutated = skillText.replace( + /(?:\*\*Every phase and full acceptance artifact body|Every phase and full activation artifact body) MUST include an `Activation[\s\S]*?non-empty array[^.]*\./, + 'The summary should mention the created issues.', + ); + expect(mutated).not.toBe(skillText); + expect(() => assertBindingsContractPresent(mutated)).toThrow(); + }, + ); + + it.each(BOTH_PATHS)( + '%s: mutation kill — weakening "never emit an empty array" to permit an empty array fails the contract check', + (_path, skillText) => { + const mutated = skillText.replace(/never\s+emit an empty array/, 'an empty array is acceptable'); + expect(mutated).not.toBe(skillText); + expect(() => assertBindingsContractPresent(mutated)).toThrow(); + }, + ); + + it.each(BOTH_PATHS)( + '%s: mutation kill — renaming the Label operations accepted heading fails the contract check', + (_path, skillText) => { + const mutated = skillText.replace(/#{1,6} Label operations accepted/, '###### Label Notes'); + expect(mutated).not.toBe(skillText); + expect(() => assertBindingsContractPresent(mutated)).toThrow(); + }, + ); + + it('documents the fast-path mandatory-bindings contract in the shared ontology, not only the skill body', () => { + const prose = ontology.replace(/\s+/g, ' '); + expect(prose).toMatch( + /mandatory for\s*`phases-activated`, `activated`, `phases-accepted`, and `plan-accepted` artifacts/, + ); + expect(prose).toMatch(/neither path may omit it or ship an\s*empty array/); + }); +}); + +// --------------------------------------------------------------------------- +// Known-trap wording regression: "the label set it should have received". +// --------------------------------------------------------------------------- + +describe('gh-aw: the known "should have received" trap phrase is reworded (#1958 E4 preflight A)', () => { + it('never uses the exact trap phrase describing a label a report_incomplete item was targeting', () => { + // Narrowly scoped to the exact known-trap phrase, not a blanket ban on "received" — the + // word legitimately appears elsewhere in this file for unrelated meanings (e.g. "the + // right issue received the right value", "a rejection you never received"), and a + // blanket scan would be exactly the false-positive-prone pattern this change avoids. + expect(workflow).not.toMatch(/label set it should have received/); + }); + + it('replaces it with wording that names the same fact without a forbidden certainty word', () => { + expect(workflow).toContain('the label set that missing operation targeted'); + }); +}); + +// --------------------------------------------------------------------------- +// 3a. Checker: fast-path artifact types get the same bindings validation. +// --------------------------------------------------------------------------- + +const roster = parseRoster(` +## Members +| Name | Role | +|------|------| +| Kint | Lead | +`); + +function binding(overrides: Partial> = {}) { + return { + task: '1', + issue: 42, + epic: '2.1', + epic_issue: 43, + agent: 'Kint', + epic_agents: ['kint'], + label: 'squad:kint', + epic_label: 'squad:kint', + ...overrides, + }; +} + +const presentLabels = new Map([ + [42, new Set(['squad', 'squad:kint'])], + [43, new Set(['squad', 'squad:kint'])], +]); + +describe.each([ + ['plan-accepted', false], + ['phases-accepted', true], + ['activated', false], + ['phases-activated', true], +])('checker/runtime: %s bindings validation (E4 preflight A parity)', (squad_artifact, phased) => { + function artifact(bindings: unknown) { + return { + squad_artifact, + schema_version: '1', + origin_issue: 1, + phases: phased ? [1] : [], + bindings, + }; + } + + it('fails closed when bindings are absent', () => { + expect(() => validateBindings(artifact(undefined), roster, presentLabels)).toThrow( + 'bindings are missing or empty', + ); + }); + + it('fails closed when bindings are an empty array', () => { + expect(() => validateBindings(artifact([]), roster, presentLabels)).toThrow('bindings are missing or empty'); + }); + + it('accepts a valid non-empty bindings array', () => { + const result = validateBindings(artifact([binding()]), roster, presentLabels); + expect(result).toMatchObject({ skipped: false, checked: 1, epics: 1 }); + }); +}); + +it('non-activation artifact types remain untouched by the bindings contract', () => { + expect( + validateBindings( + { squad_artifact: 'triage', schema_version: '1', origin_issue: 1, phases: [] }, + roster, + presentLabels, + ), + ).toEqual({ skipped: true }); +}); + +describe('checker/runtime: full parseStructuredData + validateActivation pipeline against realistic fast-path comments', () => { + function fastPathComment(bindingsBlock: string): string { + return `## ✅ Plan Activated — 1 epic, 1 task + +| # | Title | Issue | Size | Agent | +|---|-------|-------|------|-------| +| 1 | Do the thing | #42 | S | Kint | + +${bindingsBlock} +Structured data: +\`\`\`json +{"squad_artifact":"plan-accepted","schema_version":"1","origin_issue":1,"phases":[]} +\`\`\` +`; + } + + const VALID_BINDINGS_BLOCK = `Activation bindings: +\`\`\`json +[{"task":"1","issue":"#42","epic":"2.1","epic_issue":"#43","agent":"Kint","epic_agents":["kint"],"label":"squad:kint","epic_label":"squad:kint"}] +\`\`\``; + + it('a realistic comment with a valid bindings block parses and validates cleanly', () => { + const artifact = parseStructuredData(fastPathComment(VALID_BINDINGS_BLOCK)); + expect(artifact).not.toBeNull(); + const result = validateActivation(artifact, roster, new Map([[42, new Set(['squad', 'squad:kint'])], [43, new Set(['squad', 'squad:kint'])]])); + expect(result).toMatchObject({ skipped: false, checked: 1 }); + }); + + it('runtime-shape mutation: deleting the Activation bindings block from a real comment fails validation', () => { + const artifact = parseStructuredData(fastPathComment('')); + expect(artifact).not.toBeNull(); + expect(() => validateActivation(artifact, roster, new Map())).toThrow('bindings are missing or empty'); + }); + + it('runtime-shape mutation: emptying the Activation bindings array in a real comment fails validation', () => { + const emptied = 'Activation bindings:\n```json\n[]\n```'; + const artifact = parseStructuredData(fastPathComment(emptied)); + expect(artifact).not.toBeNull(); + expect(() => validateActivation(artifact, roster, new Map())).toThrow('bindings are missing or empty'); + }); +}); + +// --------------------------------------------------------------------------- +// 3b. Checker: accepted-only label-operation wording, scoped (not blanket-scanned). +// --------------------------------------------------------------------------- + +const FORBIDDEN_WORDS = ['applied', 'received', 'landed', 'verified', 'confirmed', 'checked'] as const; + +describe('checker/runtime: rejects standalone certainty claims in label-operation reporting', () => { + describe.each(FORBIDDEN_WORDS)('forbidden word "%s"', word => { + it.each([ + word, + word.toUpperCase(), + `${word[0].toUpperCase()}${word.slice(1)}`, + `${word}.`, + `${word},`, + `${word}!`, + ])('flags a label-operation claim using %j', variant => { + const comment = `- Label squad:kint was ${variant} for #42`; + expect(() => assertAcceptedOnlyLabelWording(comment, 'activated')).toThrow( + /forbidden certainty claim/, + ); + }); + }); + + it.each([ + ['activated'], + ['phases-activated'], + ['plan-accepted'], + ['phases-accepted'], + ])('applies identically to the %s artifact type', artifactType => { + expect(() => + assertAcceptedOnlyLabelWording('Label squad:kint was verified for #42', artifactType), + ).toThrow(/forbidden certainty claim/); + }); + + it('is a no-op for artifact types outside the activation contract', () => { + expect(assertAcceptedOnlyLabelWording('Label squad:kint was verified for #42', 'triage')).toEqual({ + skipped: true, + }); + expect(assertAcceptedOnlyLabelWording('Label squad:kint was verified for #42', 'research')).toEqual({ + skipped: true, + }); + }); + + it('accepts compliant accepted-only wording', () => { + expect( + assertAcceptedOnlyLabelWording('- Label squad:kint: add_labels accepted for #42.', 'activated'), + ).toEqual({ skipped: false }); + }); + + // --- Negative cases: the whole point of scoping is that these must NOT false-positive. + + it('does not flag a forbidden word inside a quoted work-item title', () => { + expect( + assertAcceptedOnlyLabelWording( + 'Label squad:kint: title "Verified Payments Reconciliation" accepted.', + 'activated', + ), + ).toEqual({ skipped: false }); + }); + + it('does not flag a forbidden word inside a markdown table row (titles live there)', () => { + const comment = '| 1 | "Verified Email Addresses" | #42 | S | Kint |'; + expect(assertAcceptedOnlyLabelWording(comment, 'activated')).toEqual({ skipped: false }); + }); + + it('does not flag a forbidden word in a line unrelated to label-operation reporting', () => { + // No "label"/"labels"/"squad:*" mention — out of scope for this check, even though the + // word is a bare, unquoted claim about something else entirely. + expect( + assertAcceptedOnlyLabelWording('The build was verified successfully by CI.', 'activated'), + ).toEqual({ skipped: false }); + }); + + it('does not flag an explicit negation — the honest, compliant statement the contract requires', () => { + expect( + assertAcceptedOnlyLabelWording('Label squad:kint was not verified on the issue.', 'activated'), + ).toEqual({ skipped: false }); + expect( + assertAcceptedOnlyLabelWording( + 'Label squad:copilot: never confirmed by GitHub, only accepted.', + 'activated', + ), + ).toEqual({ skipped: false }); + }); + + it('does not flag a forbidden word appearing only as a substring of another word', () => { + expect( + assertAcceptedOnlyLabelWording('Label squad:kint operation unverified; retry recommended.', 'activated'), + ).toEqual({ skipped: false }); + expect( + assertAcceptedOnlyLabelWording('Label squad:kint remains prechecked for retry.', 'activated'), + ).toEqual({ skipped: false }); + }); + + it('does not use a blanket whole-comment scan: an unrelated section may say the word freely', () => { + const comment = `## ✅ Plan Activated — 1 epic, 1 task + +| # | Title | Issue | Size | Agent | +|---|-------|-------|------|-------| +| 1 | "Verified Email Addresses" | #42 | S | Kint | + +Label squad:kint: add_labels accepted for #42. +`; + expect(assertAcceptedOnlyLabelWording(comment, 'activated')).toEqual({ skipped: false }); + }); + + it('regression: a whole-comment scan would have false-positived on the table row above', () => { + const comment = `| 1 | "Verified Email Addresses" | #42 | S | Kint | + +Label squad:kint: add_labels accepted for #42. +`; + // Demonstrates why the check must not be a blanket substring scan: `\bverified\b` alone + // matches this comment, but the scoped checker must not flag it. + expect(/\bverified\b/i.test(comment)).toBe(true); + expect(assertAcceptedOnlyLabelWording(comment, 'activated')).toEqual({ skipped: false }); + }); + + // --- Regression: negation/claim scope must bind to a clause, not the whole line. + + it('regression: an unrelated negation earlier on the line must not blanket-suppress a real, separate claim', () => { + // "No issues were skipped" negates "skipped", not "verified" — the claim after the colon + // is a genuinely separate clause and is a real, standalone certainty claim. + expect(() => + assertAcceptedOnlyLabelWording( + 'No issues were skipped: Label squad:kint was verified for #42.', + 'activated', + ), + ).toThrow(/forbidden certainty claim/); + }); + + it('regression: a forbidden word describing a different subject on the same line as a label token must not flag', () => { + // "its verified real number" describes the reused issue number, not the label operation — + // this is the exact, sanctioned "or by its verified real number for a reused issue" wording + // from the Label operations accepted contract (workflows/squad.md), and must pass. + expect( + assertAcceptedOnlyLabelWording( + 'Reported squad:kint for issue #123, its verified real number (reused this run).', + 'activated', + ), + ).toEqual({ skipped: false }); + }); +}); + +// --------------------------------------------------------------------------- +// 4. CI collector actually passes fast-path artifacts to the checker. +// --------------------------------------------------------------------------- + +const COLLECTOR_ARTIFACT_REGEX_SOURCE = + '"squad_artifact"\\s*:\\s*"?(?:phases-activated|activated|phases-accepted|plan-accepted)'; + +describe('gh-aw: squad-agent-binding-check.yml collector passes fast-path artifacts to the checker (E4 preflight A)', () => { + const ymlContent = readText(COLLECTOR_YML); + + it('embeds the exact artifact-matching regex used to filter collected comments', () => { + expect(ymlContent).toContain(COLLECTOR_ARTIFACT_REGEX_SOURCE); + }); + + it('the embedded regex matches every activation artifact type (fast-path and granular)', () => { + const collectorRegex = new RegExp(COLLECTOR_ARTIFACT_REGEX_SOURCE, 'i'); + for (const artifactType of ['activated', 'phases-activated', 'plan-accepted', 'phases-accepted']) { + expect( + collectorRegex.test(`Structured data:\n\`\`\`json\n{"squad_artifact":"${artifactType}","schema_version":"1"}\n\`\`\``), + `expected collector regex to match ${artifactType}`, + ).toBe(true); + } + }); + + it('the embedded regex does not indiscriminately match unrelated non-activation artifacts', () => { + const collectorRegex = new RegExp(COLLECTOR_ARTIFACT_REGEX_SOURCE, 'i'); + for (const artifactType of ['research', 'triage', 'scope-accepted', 'impl-accepted', 'impl-phases-accepted', 'validation']) { + expect( + collectorRegex.test(`{"squad_artifact":"${artifactType}"}`), + `expected collector regex NOT to match ${artifactType}`, + ).toBe(false); + } + }); + + it('regression: the old blanket /activated/i filter would have missed every fast-path artifact', () => { + // This is the exact defect this change fixes: `plan-accepted` / `phases-accepted` + // comments never contain the substring "activated", so the old collector filter + // silently dropped them before the checker ever saw them. + const oldFilter = /activated/i; + expect(oldFilter.test('{"squad_artifact":"plan-accepted"}')).toBe(false); + expect(oldFilter.test('{"squad_artifact":"phases-accepted"}')).toBe(false); + }); + + it('still reads Node 22 and never grants issues: write (unchanged behavioral contract)', () => { + expect(ymlContent).toContain('node-version: 22'); + expect(ymlContent).toContain('issues: read'); + expect(ymlContent).not.toContain('issues: write'); + }); +}); diff --git a/test/gh-aw-activation-capacity.test.ts b/test/gh-aw-activation-capacity.test.ts index 35aac3440..ca92b1dae 100644 --- a/test/gh-aw-activation-capacity.test.ts +++ b/test/gh-aw-activation-capacity.test.ts @@ -371,8 +371,14 @@ describe('gh-aw: self-validation reconciles activated items with label operation const reconciliation = activateSkill.slice(activateSkill.indexOf('**2e. Label-Operation Reconciliation')); const reconciliationProse = reconciliation.replace(/\s+/g, ' '); expect(reconciliation).toContain('report_incomplete'); + // Wording was tightened by the E4 preflight package A fast-path artifact-integrity fix: + // "the label set it should have received" used the forbidden certainty word "received" + // to describe a label operation that never happened, even though it was worded as + // something the item merely lacked rather than a claim that it landed. The reworded + // phrase states the identical fact — which label set the missing operation targeted — + // without a word the runtime label-operation reporting contract now bans outright. expect( - /the identifier you used to target its `add_labels` call, its title, and the label set it should have received/i.test( + /the identifier you used to target its `add_labels` call, its title, and the label set that missing operation targeted/i.test( reconciliationProse, ), 'The incomplete report must identify affected work items specifically enough to act on.', diff --git a/workflows/shared/squad-planning-ontology.md b/workflows/shared/squad-planning-ontology.md index ef6f67860..41d169253 100644 --- a/workflows/shared/squad-planning-ontology.md +++ b/workflows/shared/squad-planning-ontology.md @@ -321,7 +321,10 @@ set from the full accepted plan (including other activation phases). It records 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 +`phases-activated`, `activated`, `phases-accepted`, and `plan-accepted` artifacts — +every fast-path (`/squad activate`) and granular (`/squad plan activate`) artifact +that creates or recognizes issues carries it; neither path may omit it or ship an +empty array. 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. diff --git a/workflows/squad.md b/workflows/squad.md index d06b3103b..8feae2d89 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1704,7 +1704,7 @@ by `create-if-missing` on a fresh repository instead receives gh-aw's determinis color and an empty description — that is expected, not a failure, and must not be 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). +attributed to `create-issue` (see Step 4, Label operations accepted). ##### Step 3: Preserve Dependencies @@ -1721,16 +1721,33 @@ were preserved in issue bodies without native edges. ##### Step 4: Post Summary Artifact data varies: -- Phase-specific: `data: {"squad_artifact":"phases-accepted","schema_version":"1","origin_issue":{issue_number},"phases":[{accumulated}]}` → Phase accepted table + remaining phases table -- Full (no phases): `data: {"squad_artifact":"plan-accepted","schema_version":"1","origin_issue":{issue_number},"phases":[]}` → All issues table +- Phase-specific: `data: {"squad_artifact":"phases-accepted","schema_version":"1","origin_issue":{issue_number},"phases":[{accumulated}]}` → Phase accepted table + remaining phases table + the `Activation bindings:` JSON array. +- Full (no phases): `data: {"squad_artifact":"plan-accepted","schema_version":"1","origin_issue":{issue_number},"phases":[]}` → All issues table + the `Activation bindings:` JSON array. Report the exact number of created task issues, their actual parent hierarchy, 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: +**Every phase and full acceptance artifact body MUST include an `Activation +bindings:` fenced JSON block containing a non-empty array** — the identical +binding shape, quoting, and omission-reason semantics as `squad-plan-activate` +Step 4's contract: one object per created/recognized work item with +`task`/`issue`/`epic`/`epic_issue`/`agent`/`epic_agents`, plus `label` or +`omission_reason`, and `epic_label` or `epic_omission_reason` (`multi-owner` or +`non-roster`). `issue` and `epic_issue` are quoted JSON strings — that item's own +`temporary_id` when created this run, its verified real number when reused — +never bare numbers; a surviving `#aw_…` reference means `create-issue` never +landed and must be left unresolved rather than repaired. Never omit a created +or recognized task from `bindings`, never infer an issue number, and never +emit an empty array — the deterministic post-activation checker treats a +missing, empty, malformed, or unresolved bindings block on a `plan-accepted` +or `phases-accepted` artifact as a failure exactly as it does for `activated` +and `phases-activated`. + +###### Label operations accepted + +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 @@ -2267,7 +2284,7 @@ Step 2e, not the cap machinery, is what notices. each `create-issue` call, re-read the agent from that issue's own source — a task's own `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 +8. **Report what was accepted, not what was intended.** The activation summary may name a `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 @@ -2277,7 +2294,7 @@ Step 2e, not the cap machinery, is what notices. 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. + not happen. See Step 4's Label operations accepted 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 @@ -2345,7 +2362,7 @@ Root → Epics → Tasks. Phase-specific: filter to matching phase heading. While Steps 2b/2c run, keep two counts: `activated` (issues created or recognized this run) and `labeled` (issues whose `add_labels` call was accepted). An `add_labels` call that was never made, was rejected, or returned an error counts as **unlabeled**. These counts track *label operations*, not labels present on GitHub: acceptance means the call was queued for a specific target this turn, and gh-aw applies it in the post-agent job. Never state or imply that a counted label was applied, landed, or was confirmed on the issue — nothing here reads labels back. At the end of Step 2: 1. `labeled == activated` → the activation is complete; proceed to Step 3. -2. `labeled < activated` → **this is an incomplete activation, not a successful one.** Call `report_incomplete` with a `reason` naming the shortfall (`{labeled} of {activated} activated issues had a label operation accepted`) and `details` listing **every affected work item — the identifier you used to target its `add_labels` call, its title, and the label set it should have received**. For an item created this run that identifier is the `temporary_id` you minted under the Temporary-ID Contract (`#aw_epic{K}` / `#aw_task{N}`) — not a GitHub issue number, because creation is deferred to the safe-output job and no real number exists yet. Quote a real number only where one is independently verified: an epic or task matched by dedup-by-title, or an issue recognized by Step 1's idempotent-rerun path. Never predict, infer, or invent a number. `report_incomplete` logs a warning and opens or updates a durable `[aw] ... reported incomplete result` tracking issue; it does **not** change the run's conclusion — the run still reports success. That record and the rule below keep a truncated activation from passing as clean, so never rely on a red run to carry the signal. +2. `labeled < activated` → **this is an incomplete activation, not a successful one.** Call `report_incomplete` with a `reason` naming the shortfall (`{labeled} of {activated} activated issues had a label operation accepted`) and `details` listing **every affected work item — the identifier you used to target its `add_labels` call, its title, and the label set that missing operation targeted**. For an item created this run that identifier is the `temporary_id` you minted under the Temporary-ID Contract (`#aw_epic{K}` / `#aw_task{N}`) — not a GitHub issue number, because creation is deferred to the safe-output job and no real number exists yet. Quote a real number only where one is independently verified: an epic or task matched by dedup-by-title, or an issue recognized by Step 1's idempotent-rerun path. Never predict, infer, or invent a number. `report_incomplete` logs a warning and opens or updates a durable `[aw] ... reported incomplete result` tracking issue; it does **not** change the run's conclusion — the run still reports success. That record and the rule below keep a truncated activation from passing as clean, so never rely on a red run to carry the signal. **Cap exhaustion is a reportable, nameable cause.** If the shortfall is because a cap was reached, say so in the `reason`, name which cap (`create-issue` 75 or `add_labels` 110) and list the work items that did not fit, and recommend `/squad plan activate phase {N}`. This is the one case where a cap may be named as the cause: it was observed, not guessed. Do not infer a cap from a rejection you never received, and do not treat the absence of an `E002` error as proof that every label operation was accepted: the count comparison, not the error stream, is the authority. @@ -2404,7 +2421,7 @@ 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 +###### Label operations accepted 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