Close 2026-07-06 spec maintenance gaps for parser vectors, security notes, AI Credits sync gating, and SDK driver sync guidance#43839
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey
If you’d like a hand, you can assign this prompt to your coding agent:
|
🤖 PR Triage
Summary: DRAFT. Spec maintenance across 4 areas: bash parser test vectors, security-architecture follow-ups, AI Credits mirror-consistency gate, Copilot SDK driver sync. 9 files. No CI.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43839 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR updates several gh-aw specifications and maintenance artifacts: it adds canonical bash command parser conformance vectors, clarifies/aligns the security architecture spec with current compiled workflow structure, introduces a models.json mirror-consistency validation target, and adds sync guidance + normative tightening to the Copilot SDK driver spec.
Changes:
- Added versioned bash command parser conformance vectors (
v1.1.0) and documented sync/revalidation triggers in the spec. - Clarified security-architecture runtime gating semantics (
pre_activation,detection, optionalconclusion) and updated the validation + summary tracking docs accordingly. - Added
make validate-models-json-syncand documented mirror-consistency requirements in the AI credits spec; added Sync Notes to multiple specs.
Show a summary per file
| File | Description |
|---|---|
| specs/test-vectors/bash-command-parser/v1.1.0-verification.json | Adds verification-focused parser vectors (robustness + metamorphic relations). |
| specs/test-vectors/bash-command-parser/v1.1.0-model-based.json | Adds model-based canonical vectors covering S/E/P core requirements. |
| specs/security-architecture-spec.md | Clarifies runtime role validation, threat-detection job role, optional conclusion, and companion spec references. |
| specs/security-architecture-spec-validation.md | Updates validation date/results and records the completed maintenance follow-ups. |
| specs/security-architecture-spec-summary.md | Marks previously pending maintenance follow-ups as done. |
| Makefile | Adds validate-models-json-sync to compare normalized models.json mirrors. |
| docs/src/content/docs/specs/copilot-sdk-driver-specification.md | Adds Sync Notes and makes denial counting mandatory to match reference behavior. |
| docs/src/content/docs/specs/bash-command-parser-specification.md | Adds Sync Notes + changelog entry for canonical vector corpus. |
| docs/src/content/docs/specs/ai-credits-specification.md | Strengthens mirror-consistency requirement and adds Sync Notes/revalidation triggers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Low
| .PHONY: validate-models-json-sync | ||
| validate-models-json-sync: | ||
| @echo "Validating models.json mirror consistency..." | ||
| @tmp1=$$(mktemp); tmp2=$$(mktemp); \ |
There was a problem hiding this comment.
REQUEST_CHANGES — Two blocking correctness issues must be resolved before merge
The spec maintenance is well-intentioned but introduces a normative CI requirement with no enforcement, and a new Makefile gate that silently passes on the exact failure case it is supposed to catch. The test vector files also have structural issues.
### Blocking issues (2)
1. CI gate not wired ( line 49): The spec now states CI MUST run make validate-models-json-sync, but no workflow file was changed in this PR and no existing workflow calls this target. A MUST without an enforcement point is misleading spec text.
2. Makefile silent false-positive ( line 16): If jq is not installed or both source files are missing, both temp files are left empty, cmp -s considers them identical, and the gate exits 0. The existing refresh-models-json target uses set -e; this one does not. Non-empty guards on the jq output are also needed.
### High-severity structural issues (2)
3. Test vectors inert (): The new files use a flat "function" field schema that is incompatible with the function-keyed schema of actions/setup/js/bash_command_parser_spec_vectors.json. No test harness, workflow, or Makefile target loads them. They are documentation, not executed tests.
4. Duplicate metamorphic vectors (): BP-VF-MR-001/002/003 are character-for-character duplicates of BP-MR-001/005/004 in the existing spec_vectors file. Once a harness is wired up, two authoritative sources will silently diverge.
### Medium issue (1)
5. Rationale in normative body ( line 146): The paragraph explaining the SHOULD→MUST change is editorial prose embedded between normative MUST clauses. Use a > Note (non-normative): block or move it to the Change Log.
🔎 Code quality review by PR Code Quality Reviewer · 152.8 AIC · ⌖ 5.81 AIC · ⊞ 5.5K
Comment /review to run again
Comments that could not be inline-anchored
docs/src/content/docs/specs/ai-credits-specification.md:49
Spec mandates CI MUST run make validate-models-json-sync but no workflow file runs it. This MUST-level requirement is unenforceable dead text on merge.
<details>
<summary>💡 Details</summary>
The diff adds this normative text to §5.4:
> CI MUST run make validate-models-json-sync (or an equivalent normalized-content comparison gate).
No .github/workflows/*.yml file was changed in this PR, and a grep of the entire .github/ tree confirms no workflow calls this target. A divergence …
Makefile:16
validate-models-json-sync silently passes when jq is absent or when both source files are missing: both temp files are empty, cmp reports them identical, and the gate exits 0.
<details>
<summary>💡 Details and suggested fix</summary>
The current logic:
jq -S . pkg/cli/data/models.json > "$$tmp1"; jq -S . actions/setup/js/models.json > "$$tmp2"; cmp -s "$$tmp1" "$$tmp2" || { ... exit 1; }If jq is not installed, both invocations silently fail (exit non-zero, write…
specs/test-vectors/bash-command-parser/v1.1.0-model-based.json:5
These vector files use a different schema from the existing bash_command_parser_spec_vectors.json and no test runner loads them. They are inert documentation.
<details>
<summary>💡 Details</summary>
The authoritative vector file is actions/setup/js/bash_command_parser_spec_vectors.json, loaded by actions/setup/js/bash_command_parser_spec_vectors.test.cjs. That file uses a function-keyed schema:
{ "vectors": { "splitOnPipelineOperators": [...], "extractCommandName": [...] },…
</details>
<details><summary>specs/test-vectors/bash-command-parser/v1.1.0-verification.json:18</summary>
**BP-VF-MR-001, BP-VF-MR-002, and BP-VF-MR-003 are semantic duplicates of entries already in `bash_command_parser_spec_vectors.json`**, creating a silent-drift trap.
<details>
<summary>💡 Details</summary>
The existing `actions/setup/js/bash_command_parser_spec_vectors.json` already contains these metamorphic relations:
| New vector | Existing entry | Relation |
|---|---|---|
| BP-VF-MR-001 | BP-MR-001 | whitespace-invariance — identical `left`/`right`/`expected` |
| BP-VF-MR-002 | BP-MR-005…
</details>
<details><summary>docs/src/content/docs/specs/copilot-sdk-driver-specification.md:146</summary>
**Rationale paragraph is embedded in the normative spec body** — future readers cannot distinguish this prose from binding conformance text.
<details>
<summary>💡 Details</summary>
The added paragraph reads:
> This counting requirement tightens the specification to match the existing gh-aw reference implementation... It does not change gh-aw runtime behavior, but implementations that previously treated denial counting as optional would need to align...
This is rationale/editorial commentary…
</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs and /tdd — requesting changes on three issues: a normative-but-never-run CI gate, a SHOULD/MUST inconsistency in the SDK spec table, and a gap between the new test vectors and the live test runner.
📋 Key Themes & Highlights
Blocking Issues
-
validate-models-json-sync is never called (
Makefileline 909) — the AI Credits spec §5.4 now declaresCI MUST run make validate-models-json-sync, but the target is not wired intolint,agent-report-progress, or any GitHub Actions workflow. The normative claim is unsatisfied. -
Test runner does not consume the new vectors — spec §9 says the native test runner MUST consume vectors from
specs/test-vectors/bash-command-parser/once the collection exists.bash_command_parser.test.cjshas no such loading logic; the 20 new vectors are dead spec artifacts unless this is added. -
SHOULD/MUST split in SDK driver spec table — the env-var reference table still says
SHOULDforGH_AW_MAX_TOOL_DENIALSinput while §4.5 prose now mandatesMUST count. The ambiguity will confuse conformance authors.
Secondary Issues
-
Incomplete metamorphic coverage — the verification file covers 3 of the 6 relations listed in §8. Missing:
env-prefix-invariance(§8.3) andredirection-suffix-invariance(§8.4). -
Vector count alignment is coincidental, not spec-faithful — model-based.json has 15 vectors by expanding E-CORE and P-CORE into branch cases, not by including R-CORE. A conformance note would prevent misreading.
Positive Highlights
- ✅ All 15 model-based vectors verified correct against the live
bash_command_parser.cjsimplementation. - ✅ All MR and R-CORE vectors in the verification file produce correct expected outputs.
- ✅
jq -Snormalization in the Makefile target is the right approach for JSON mirror comparison. - ✅ Security spec maintenance items are well-scoped and the PM-11/trusted-users forward references are precise.
- ✅ The SHOULD→MUST change in §4.5 is correctly annotated with a non-breaking note.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 125.1 AIC · ⌖ 8.53 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
Makefile:909
[/grill-with-docs] validate-models-json-sync is not wired into any CI chain — the spec it enforces is now normative but the gate will never run automatically.
The AI Credits spec (§5.4) now states: "CI MUST run make validate-models-json-sync (or an equivalent normalized-content comparison gate)." But validate-models-json-sync is absent from lint, agent-report-progress, and no GitHub Actions workflow calls it.
<details>
<summary>💡 Suggested fix</summary>
Add it to the lint …
specs/test-vectors/bash-command-parser/v1.1.0-verification.json:57
[/tdd] The verification file covers only 3 of the 6 metamorphic relations defined in §8 — missing env-prefix-invariance (§8.3) and redirection-suffix-invariance (§8.4).
§8 requires verification vectors for: whitespace-invariance ✅, quoted-operator-shielding ✅, env-prefix-invariance ❌, redirection-suffix-invariance ❌, duplicate-collapse-invariance ✅, no-throw robustness ✅. The spec says verification MUST include metamorphic/property-derived vectors for all listed relations.
<details>
<…
docs/src/content/docs/specs/bash-command-parser-specification.md:363
[/tdd] The spec (§9) says the native test runner MUST consume vectors from specs/test-vectors/bash-command-parser/ when the collection is present — but bash_command_parser.test.cjs does not load these files at all.
The canonical vector files are now present in the repo, which activates the MUST requirement, yet the existing test file uses only inline hardcoded cases. No test connects the new v1.1.0-model-based.json or v1.1.0-verification.json to the runner.
<details>
<summary>💡 S…
docs/src/content/docs/specs/copilot-sdk-driver-specification.md:146
[/grill-with-docs] The reference table at line 146 still says SHOULD for GH_AW_MAX_TOOL_DENIALS, while §4.5 prose (line 185) now says MUST count — the same env var carries two different RFC 2119 levels depending on where you read.
This is confusing for conformance authors: the table implies optional input, but the prose makes counting mandatory. The table describes input validation, not counting behaviour, so the conflict may be unintentional — but it should be resolved explicitly.
…
specs/test-vectors/bash-command-parser/v1.1.0-model-based.json:129
[/tdd] The model-based file has 15 total vectors (S×4 + E×6 + P×5), but §6.1 counts the mandatory suite as S-CORE (4) + E-CORE (5) + P-CORE (4) + R-CORE (2) = 15. The model-based file has no R-CORE and expands E to 6 and P to 5 by splitting individual requirements into two branches.
The spec counts branches as single tests: e.g., E-CORE-4 is one test ("clause keywords continue extraction while structural keywords return null") with two sub-cases. Encoding these as separate vectors is fine,…
There was a problem hiding this comment.
Review Summary
The PR cleanly closes four maintenance gaps. Most changes are documentation-only clarifications that accurately reflect existing implementation behavior. One blocking issue was found:
🔴 Blocking: Spec claims normative CI gate that does not exist
File: docs/src/content/docs/specs/ai-credits-specification.md, line 254
The new §5.4 text contains: "CI MUST run make validate-models-json-sync". However, make validate-models-json-sync is not wired into any CI workflow file (.github/workflows/*.yml). The spec is asserting a MUST-level conformance requirement that the repository itself does not currently satisfy.
Fix options:
- Add a CI step (e.g. in
cjs.ymlor a new workflow) that runsmake validate-models-json-sync - Change the MUST to SHOULD in the spec until the CI job is added
All other changes look correct:
- ✅ Test vectors for bash-command-parser v1.1.0 are accurate (spot-checked S-CORE, E-CORE, P-CORE, R-CORE, MR cases against implementation)
- ✅ Security spec PM-11 forward reference to
pre_activationis consistent with implementation - ✅ Appendix D
detectionjob naming is accurate - ✅ SDK driver §4.5 SHOULD→MUST tightening matches existing reference implementation
- ✅ Companion spec references (
scratchpad/files) exist in the repository
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 104.6 AIC · ⌖ 6.03 AIC · ⊞ 4.9K
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in
Reran local checks on the updated branch: I can’t reliably rerun/verify fresh GitHub checks from this agent context after pushing; please re-trigger CI from GitHub (close/reopen PR, workflow_dispatch, or maintainer push) to validate the new HEAD. |
|
@copilot please run the Unresolved review feedback:
|
Done. I ran the pr-finisher flow on the current branch head ( For CI: I reviewed recent workflow runs/logs on this branch. The latest PR workflows are in |
| | Track pre_activation note from validation doc | ⏳ Pending | Add an explicit note tying role validation to the separate `pre_activation` job in `specs/security-architecture-spec.md` | | ||
| | Track detection job naming note from validation doc | ⏳ Pending | Add an Appendix D example that names the `detection` job explicitly as the runtime threat-detection layer | | ||
| | Track conclusion job note from validation doc | ⏳ Pending | Document optional cleanup/reporting jobs such as `conclusion` without making them normative | | ||
| | Audit trusted-users runtime enforcement coverage | ⏳ Pending | `trusted-users` runtime enforcement is not yet called out in Sections 8-9 of `specs/security-architecture-spec.md`; decide whether to document it there or defer to the companion GitHub MCP access-control spec | |
There was a problem hiding this comment.
Updated in bbc2031: the summary now states the trusted-users runtime enforcement scope directly in this document instead of forwarding readers to companion specs.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
This PR closes the outstanding SPDD maintenance items across four specs: the missing bash parser conformance artifacts, unresolved security-architecture follow-ups, the missing AI Credits mirror-consistency gate, and the Copilot SDK driver sync/normative cleanup. It also updates the associated tracking docs so the spec set and maintenance state stay aligned.
Bash command parser conformance artifacts
specs/test-vectors/bash-command-parser/v1.1.0-model-based.jsonspecs/test-vectors/bash-command-parser/v1.1.0-verification.jsonS-CORE,E-CORE,P-CORE, andR-COREcases from §6.1.Sync Notesto the spec and records the canonical vector path plus revalidation triggers.Security architecture spec maintenance
pre_activation→ runtime role-validation linkage explicit under PM-11.detectionjob explicitly as the runtime threat-detection layer in Appendix D guidance.conclusionas optional, non-normative post-processing.trusted-usersruntime enforcement to the companion MCP access-control specs.AI Credits mirror consistency gate
make validate-models-json-syncas the repository gate forpkg/cli/data/models.jsonandactions/setup/js/models.json.Sync Noteswith concrete revalidation triggers for model additions, pricing changes, and spec minor bumps.Copilot SDK driver spec cleanup
Sync Notestied to the harness/driver implementation files and revalidation conditions.Example: new mirror-consistency gate