From 7dbf0182757861d81f23fa706770ce2657b19db2 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 1 Sep 2026 18:12:07 -0700 Subject: [PATCH 1/4] fix(gh-aw): fast-path /squad activate artifact integrity (E4 preflight A) Closes the gap where the recommended /squad activate fast path (squad-plan-accept, artifacts plan-accepted/phases-accepted) created and labeled issues identically to the granular /squad plan activate path but was not covered by the Activation bindings: contract, the deterministic checker, or the CI collector. - workflows/squad.md: fast path Step 4 now requires the same non-empty Activation bindings: JSON block (shape/quoting/omission semantics) as squad-plan-activate. Renamed both paths' label-reporting heading to the identical "Label operations accepted" text. Reworded the "label set it should have received" trap phrase to drop the forbidden certainty word "received" while preserving meaning. - workflows/shared/squad-planning-ontology.md: the bindings mapping is now documented as mandatory for phases-accepted/plan-accepted too, not just phases-activated/activated. - scripts/check-agent-binding.mjs: ACTIVATION_ARTIFACTS now includes plan-accepted/phases-accepted so missing/empty bindings fail closed for fast-path artifacts exactly like granular ones. Added assertAcceptedOnlyLabelWording(), which rejects standalone certainty claims (applied, received, landed, verified, confirmed, checked) in label-operation reporting. It is scoped to lines that actually mention label vocabulary, strips fenced/inline code and quoted strings first, skips markdown table rows, and treats explicit negations as compliant -- never a blanket whole-comment scan -- so quoted titles, unrelated sections, negations, and substrings (unverified) don't false-positive. Wired into main()'s per-comment loop. - .github/workflows/squad-agent-binding-check.yml: the collector filter matched only /activated/i, which a plan-accepted/phases-accepted comment never contains, so fast-path artifacts were silently never passed to the checker. Now matches the squad_artifact field precisely for all four activation artifact types. - test/gh-aw-activation-artifact-integrity.test.ts (new): heading parity, fast-path bindings-contract presence + mutation kills against committed source, checker/runtime parity across all four artifact types (missing and [] bindings), a full parseStructuredData+validateActivation pipeline test against realistic comment strings (including bindings-block deletion/emptying), parameterized forbidden-word rejection (6 words x case/punctuation), negative tests for quoted titles/table rows/unrelated sections/negations/substrings, and collector-regex parity/regression tests. - test/gh-aw-activation-capacity.test.ts: updated the one existing assertion pinned to the reworded trap phrase. Out of scope per the E4 preflight package A brief: workflow discriminators, docs sequencing, draft PR UX, and all other E4 findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/squad-agent-binding-check.yml | 3 +- scripts/check-agent-binding.mjs | 84 ++- ...h-aw-activation-artifact-integrity.test.ts | 492 ++++++++++++++++++ test/gh-aw-activation-capacity.test.ts | 8 +- workflows/shared/squad-planning-ontology.md | 5 +- workflows/squad.md | 35 +- 6 files changed, 613 insertions(+), 14 deletions(-) create mode 100644 test/gh-aw-activation-artifact-integrity.test.ts 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..0c6fa8fe6 100644 --- a/scripts/check-agent-binding.mjs +++ b/scripts/check-agent-binding.mjs @@ -1,11 +1,36 @@ 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 line 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 lines 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; + function normalize(value) { return typeof value === 'string' ? value.trim().toLowerCase() : ''; } @@ -41,7 +66,7 @@ function resolveIssueReference(value, description, invalidMessage) { 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 +90,60 @@ 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, ' '); +} + +/** + * 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 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. + * - Only lines that actually mention label vocabulary (`label`/`labels`/`squad:*`) are + * scanned — an unrelated line elsewhere in the comment is out of scope. + * - A line 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); + if (!LABEL_OPERATION_CONTEXT.test(scrubbed)) continue; + if (NEGATION_NEARBY.test(scrubbed)) continue; + for (const word of FORBIDDEN_LABEL_CLAIM_WORDS) { + if (new RegExp(`\\b${word}\\b`, 'i').test(scrubbed)) { + 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,6 +329,7 @@ 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) : []; 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..2d39696e3 --- /dev/null +++ b/test/gh-aw-activation-artifact-integrity.test.ts @@ -0,0 +1,492 @@ +/** + * 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 to actual label-operation lines, never + * a blanket whole-comment scan, so quoted titles, unrelated sections, negations, and + * substrings never false-positive. + * 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 }); + }); +}); + +// --------------------------------------------------------------------------- +// 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 af2aff929..4384638fc 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1703,7 +1703,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 @@ -1720,16 +1720,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 @@ -2266,7 +2283,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 @@ -2276,7 +2293,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 @@ -2344,7 +2361,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. @@ -2403,7 +2420,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 From 76d8d33648e9f1ce4813d97f97927bf42a5eb429 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 1 Sep 2026 18:30:03 -0700 Subject: [PATCH 2/4] fix(gh-aw): scope negation/claim detection to clauses in label-wording check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FIDO rejected the first revision of #1980 with two reproduced defects in assertAcceptedOnlyLabelWording (scripts/check-agent-binding.mjs): 1. False negative: NEGATION_NEARBY matched a negation anywhere on the whole line, so an unrelated negation earlier in the line (e.g. "No issues were skipped") blanket-suppressed a real, separate certainty claim later in the same line ("Label squad:kint was verified for #42"). 2. False positive: a forbidden word describing an unrelated subject sharing a line with a label token ("its verified real number" describing a reused issue number, not the label operation) was incorrectly flagged. Fix: split each line into clauses on clause/sentence-ending punctuation (., ,, ;, :), protecting squad:{agent} tokens so their embedded colon is never mistaken for a clause boundary. Label-operation context, the forbidden word, and any negation must now all be found within the *same* clause before a claim is flagged. This lets an unrelated negation in a different clause fail to suppress a real claim, and lets a forbidden word in a clause that carries no label-operation context correctly fall out of scope, without weakening the existing quoted-title/table-row/code-fence/substring/negation guards or the six forbidden standalone certainty claims (applied, received, landed, verified, confirmed, checked). Adds two permanent regression tests reproducing FIDO's exact reported strings in test/gh-aw-activation-artifact-integrity.test.ts. Testing: - npx vitest run test/gh-aw-activation-artifact-integrity.test.ts — 90/90 passed (88 previously + 2 new regression tests) - npx vitest run test/check-agent-binding.test.ts test/gh-aw-activation-summary-outcomes.test.ts test/gh-aw-activation-capacity.test.ts test/gh-aw-quality.test.ts test/gh-aw-activate-roster-binding.test.ts test/gh-aw-agent-binding-correspondence.test.ts test/gh-aw-activation-artifact-integrity.test.ts — 377/377 passed - npx vitest run (full suite) — only pre-existing, unrelated failures remain (externalized-state CLI tests), confirmed identical on the pre-revision commit - npx eslint scripts/check-agent-binding.mjs test/gh-aw-activation-artifact-integrity.test.ts test/gh-aw-activation-capacity.test.ts — clean - npm run build — succeeds; build-metadata/version-bump and skill-template CRLF sync side effects reverted before commit, not part of this diff Scope: only scripts/check-agent-binding.mjs and test/gh-aw-activation-artifact-integrity.test.ts touched, per FIDO's required narrow revision. Closes the FIDO REJECT on #1980. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- scripts/check-agent-binding.mjs | 67 ++++++++++++++----- ...h-aw-activation-artifact-integrity.test.ts | 33 ++++++++- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/scripts/check-agent-binding.mjs b/scripts/check-agent-binding.mjs index 0c6fa8fe6..900a6123e 100644 --- a/scripts/check-agent-binding.mjs +++ b/scripts/check-agent-binding.mjs @@ -21,15 +21,23 @@ const RESOLVED_REFERENCE = /^#?(\d+)$/; // 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 line is treated as a compliant, honest non-claim (never flagged) when it explicitly +// 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 lines that are actually reporting a label operation — +// 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() : ''; @@ -102,6 +110,26 @@ function stripQuotedAndCode(line) { .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. @@ -109,14 +137,19 @@ function stripQuotedAndCode(line) { * 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 scan: + * 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. - * - Only lines that actually mention label vocabulary (`label`/`labels`/`squad:*`) are - * scanned — an unrelated line elsewhere in the comment is out of scope. - * - A line that explicitly negates the claim ("was not verified", "never confirmed") is the - * honest, compliant statement the contract requires, so it is never flagged. + * - 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`). @@ -129,15 +162,17 @@ export function assertAcceptedOnlyLabelWording(commentBody, artifactType) { const trimmed = rawLine.trim(); if (!trimmed || trimmed.startsWith('|')) continue; const scrubbed = stripQuotedAndCode(trimmed); - if (!LABEL_OPERATION_CONTEXT.test(scrubbed)) continue; - if (NEGATION_NEARBY.test(scrubbed)) continue; - for (const word of FORBIDDEN_LABEL_CLAIM_WORDS) { - if (new RegExp(`\\b${word}\\b`, 'i').test(scrubbed)) { - 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}"`, - ); + 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}"`, + ); + } } } } diff --git a/test/gh-aw-activation-artifact-integrity.test.ts b/test/gh-aw-activation-artifact-integrity.test.ts index 2d39696e3..e77606150 100644 --- a/test/gh-aw-activation-artifact-integrity.test.ts +++ b/test/gh-aw-activation-artifact-integrity.test.ts @@ -18,9 +18,11 @@ * 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 to actual label-operation lines, never - * a blanket whole-comment scan, so quoted titles, unrelated sections, negations, and - * substrings never false-positive. + * "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 @@ -439,6 +441,31 @@ Label squad:kint: add_labels accepted for #42. 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 }); + }); }); // --------------------------------------------------------------------------- From 8f7e0e107ec31aa7c12a8e3c3b446dcdfb1112ba Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 1 Sep 2026 18:40:59 -0700 Subject: [PATCH 3/4] fix(gh-aw): resolve quoted issue references before prefetching labels main() filtered binding.issue/epic_issue with Number.isInteger, but compliant activation artifacts write these as quoted "#42" strings per the resolveIssueReference() contract. That produced an empty issues list, skipped label fetching entirely, and made validateActivation() fail with "labels could not be resolved" for otherwise-valid bindings. Add extractIssueNumber(), a non-throwing counterpart to resolveIssueReference() that also accepts the "#42" string form, and use it when building the label-prefetch list in main(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/check-agent-binding.mjs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/scripts/check-agent-binding.mjs b/scripts/check-agent-binding.mjs index 900a6123e..fbc3e2284 100644 --- a/scripts/check-agent-binding.mjs +++ b/scripts/check-agent-binding.mjs @@ -71,6 +71,22 @@ 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. + */ +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) { @@ -366,7 +382,7 @@ async function main() { 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); From e11e9bab9c15f3e5eee62d48924e73880c9f88ec Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 1 Sep 2026 18:51:16 -0700 Subject: [PATCH 4/4] test(gh-aw): cover quoted activation references Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- scripts/check-agent-binding.mjs | 2 +- test/check-agent-binding.test.ts | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/scripts/check-agent-binding.mjs b/scripts/check-agent-binding.mjs index fbc3e2284..975eda3ef 100644 --- a/scripts/check-agent-binding.mjs +++ b/scripts/check-agent-binding.mjs @@ -78,7 +78,7 @@ function resolveIssueReference(value, description, invalidMessage) { * real error (e.g. an unresolved temporary ID) instead of a misleading * "labels could not be resolved" failure. */ -function extractIssueNumber(value) { +export function extractIssueNumber(value) { if (Number.isInteger(value)) return value; if (typeof value === 'string') { const resolved = RESOLVED_REFERENCE.exec(value.trim()); 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]); + }); +});