fix(gh-aw): fast-path /squad activate artifact integrity (E4 preflight A) - #1980
Merged
Conversation
…t 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>
Contributor
🟡 Impact Analysis — PR #1980Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (1 file)
root (2 files)
scripts (1 file)
tests (3 files)
This report is generated automatically for every PR. See #733 for details. |
Contributor
🏗️ Architectural Review
Automated architectural review — informational only. |
Contributor
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 5 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | 0 active Copilot thread(s) resolved (1 outdated skipped) |
| ✅ | CI passing | All checks passing |
Files Changed (7 files, +725 −15)
| File | +/− |
|---|---|
.github/workflows/squad-agent-binding-check.yml |
+2 −1 |
scripts/check-agent-binding.mjs |
+134 −3 |
test/check-agent-binding.test.ts |
+33 −0 |
test/gh-aw-activation-artifact-integrity.test.ts |
+519 −0 |
test/gh-aw-activation-capacity.test.ts |
+7 −1 |
workflows/shared/squad-planning-ontology.md |
+4 −1 |
workflows/squad.md |
+26 −9 |
Total: +725 −15
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The checker’s main() currently fetches labels only for integer binding.issue/binding.epic_issue values, which breaks validation for the contract-compliant quoted string references (e.g. "#42") and will cause false failures in CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
scripts/check-agent-binding.mjs — main() computes the list of issues to fetch labels for by filtering… |
What changed in this PR
This PR closes an E4 preflight package A integrity gap by bringing the recommended /squad activate fast path into parity with /squad plan activate for (1) mandatory non-empty Activation bindings: enforcement, (2) deterministic checker/CI collector coverage, and (3) accepted-only label-operation reporting language.
Changes:
- Updates
workflows/squad.mdand the shared planning ontology soplan-accepted/phases-acceptedartifacts also require a non-emptyActivation bindings:JSON block and use the unifiedLabel operations acceptedheading. - Expands
scripts/check-agent-binding.mjsand the CI collector workflow to include the fast-path activation artifact types, and adds a scoped wording check (assertAcceptedOnlyLabelWording) to prevent “certainty” claims in label-operation reporting. - Adds a new, comprehensive test suite covering heading parity, bindings contract presence/mutation kills, checker behavior across all 4 activation artifact types, collector regex regression, and wording-scope edge cases.
| File | Description |
|---|---|
| workflows/squad.md | Aligns fast-path activation summary contract with granular path: mandatory bindings + unified label-ops heading/wording. |
| workflows/shared/squad-planning-ontology.md | Documents bindings as mandatory for both fast-path and granular activation artifacts. |
| scripts/check-agent-binding.mjs | Extends activation artifact coverage and adds scoped accepted-only wording validation. |
| .github/workflows/squad-agent-binding-check.yml | Fixes collector filtering so fast-path artifact comments reach the checker. |
| test/gh-aw-activation-artifact-integrity.test.ts | New suite locking parity, bindings requirements, checker behavior, wording rules, and collector regression. |
| test/gh-aw-activation-capacity.test.ts | Updates assertion to match the reworded “missing operation targeted” phrasing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…g check 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>
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the E4 preflight package A gap: the recommended
/squad activatefast path (squad-plan-accept, artifactsplan-accepted/phases-accepted) creates and labels issues identically to the granular/squad plan activatepath (squad-plan-activate, artifactsactivated/phases-activated), but was not covered by theActivation bindings:contract, the deterministic checker, or the CI collector — so a fast-path activation with omitted or empty bindings was invisible to the deterministic post-activation check.Changes
workflows/squad.md: fast path Step 4 now requires the same non-emptyActivation bindings:JSON block (identical shape/quoting/omission-reason semantics) assquad-plan-activate. Both paths now use the exact, identical headingLabel operations accepted(wasLabel reporting — accepted operations onlyon the granular path and unheaded bold text on the fast path). Reworded the "the label set it should have received" trap phrase (dropped the forbidden certainty word "received" while preserving meaning) and "Report what was applied, not what was intended" → "Report what was accepted, not what was intended" for consistency with the accepted-only contract.workflows/shared/squad-planning-ontology.md: the bindings mapping is now documented as mandatory forphases-accepted/plan-acceptedtoo, not onlyphases-activated/activated.scripts/check-agent-binding.mjs:ACTIVATION_ARTIFACTSnow includesplan-accepted/phases-accepted, so missing/empty bindings fail closed for fast-path artifacts exactly like granular ones. AddedassertAcceptedOnlyLabelWording(), which rejects standalone certainty claims (applied,received,landed,verified,confirmed,checked) in label-operation reporting. It is deliberately scoped — not a blanket whole-comment scan: only lines mentioning label vocabulary (label/labels/squad:*) are scanned, fenced/inline code and quoted strings are stripped first, markdown table rows (where titles live) are skipped, and explicit negations ("was not verified") are treated as the honest, compliant statement they are. Wired intomain()'s per-comment CI loop..github/workflows/squad-agent-binding-check.yml: the collector filter matched only/activated/i, which aplan-accepted/phases-acceptedcomment never contains — fast-path artifacts were silently never passed to the checker. Now matches thesquad_artifactfield precisely for all four activation artifact types.test/gh-aw-activation-artifact-integrity.test.ts(new, 88 tests): heading parity between both paths, fast-path bindings-contract presence plus mutation kills against the committed source (not just synthetic objects), checker/runtime parity across all four artifact types (missing and[]bindings), a fullparseStructuredData+validateActivationpipeline test against realistic comment strings (including bindings-block deletion/emptying), parameterized forbidden-word rejection (6 words × case/punctuation variants), negative tests guarding 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.
Testing
npx vitest run test/gh-aw-activation-artifact-integrity.test.ts— 88/88 passednpx 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— all passed (375 tests total across the 7 targeted files)npx vitest run(full suite) — only pre-existing, unrelated failures remain (externalized-state CLI tests + one flaky observer test), confirmed identical with these changes stashed outgh aw compile --strictofworkflows/squad.mdin a scratch workspace (exercised by the existinggh-aw: the activation summary contract compiles in strict modetest) — passesnpx eslint scripts/check-agent-binding.mjs test/gh-aw-activation-artifact-integrity.test.ts test/gh-aw-activation-capacity.test.ts— cleannpx tsc --noEmitfor both packages — cleannpm run build— succeeds (build-metadata/version-bump side effects reverted before commit, not part of this diff)packages/squad-cli/src/orpackages/squad-sdk/src/Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com