Tighten SPDD compliance specs and fixtures - #52111
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
PR Triage: #52111
Automated triage — see labels for machine-readable classification.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (24 additions detected).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Tighten SPDD compliance specs and fixtures
Overall the changes are correct and well-structured.
Changes reviewed:
run_summary_partial_et.json— new T-FC-024 fixture for in-progress run with partial ET;total_effective_tokens: 2750satisfies the assertion. ✅empty-tool-name-deny.yaml— MCP access-control fixture;error_code: -32001and reason match the formal model. ✅- Spec cross-links, conformance-unit description, and compacted version table all look accurate. ✅
- Degraded-endpoint no-retry guarantee (point 4) is clear and unambiguous. ✅
Non-blocking observation: In specs/compiler-threat-detection-spec.md, the compact changelog paragraph is placed directly after the last table row with no blank line separator. Some Markdown renderers may attach it to the table footer rather than treating it as a new paragraph. Adding a blank line before the paragraph would be safer.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.2 AIC · ⌖ 6.24 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel AnalysisSummaryScore: 100/100 ✅ Excellent | Implementation Ratio: 0% (threshold: ≤30%) | Violations: None This PR ("Tighten SPDD compliance specs and fixtures") adds 1 high-quality specification-driven test and maintains fixture consistency across 2 test files. All new tests verify formal design contracts with comprehensive error handling. Test ClassificationNew Test Functions (1 total)
Detailed AnalysisTestFormal_FC_P11_PartialETFixtureDesign Invariant: Verifies that Quality Signals 🟢:
Assertions:
Implementation Classification: Design Test (not implementation-only) — validates user-visible behavior and system invariants. Fixture List MaintenanceThe PR also updates fixture enumerations in existing test functions to ensure consistency:
These are test infrastructure updates, not new behavioral test functions — necessary for enumeration consistency. Violation Scan
Quality Score Breakdown``` Thresholds: ≥80 ✅ Excellent Recommendation✅ APPROVE — High-quality specification-driven test with zero violations, perfect design alignment, and comprehensive error handling. Fixture maintenance is complete and consistent. Generated by Test Quality Sentinel — analyzing 1 new test function across Go test files. No JavaScript or infrastructure-only tests in scope.
|
There was a problem hiding this comment.
Pull request overview
Tightens SPDD compliance documentation, fixtures, and formal tests across AWF configuration, threat detection, forecasting, and GitHub MCP access control.
Changes:
- Improves specification traceability and synchronization guidance.
- Adds forecast and GitHub MCP edge-case fixtures.
- Extends formal fixture registration and validation.
Show a summary per file
| File | Description |
|---|---|
specs/github-mcp-access-control-compliance/README.md |
Pins the specification and documents fixture coverage. |
specs/github-mcp-access-control-compliance/empty-tool-name-deny.yaml |
Adds empty-tool-name denial coverage. |
specs/forecast-compliance-fixtures/run_summary_partial_et.json |
Adds an in-progress token snapshot fixture. |
specs/forecast-compliance-fixtures/README.md |
Links fixtures to specification test IDs. |
specs/compiler-threat-detection-spec.md |
Consolidates history and tightens retry safeguards. |
specs/awf-config-sources-spec.md |
Defines the conformance-unit structure. |
specs/awf-config-sources-compliance/README.md |
Cross-references structure and T-DR-005. |
pkg/workflow/github_mcp_access_control_formal_test.go |
Registers the new MCP fixture. |
pkg/cli/forecast_compliance_fixtures_formal_test.go |
Validates and registers the partial forecast fixture. |
docs/src/content/docs/specs/forecast-specification.md |
Defines partial-observation and overflow tests. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
docs/src/content/docs/specs/forecast-specification.md:935
- This reuses
T-ET-006for a different requirement, but that ID is already defined ineffective-tokens-specification.md:596for zero-ET leaf-node aggregation. The same ID would therefore map to two incompatible tests. Assign this overflow case an unused forecast ID (for exampleT-FC-025) and update the README/test references together.
- **T-ET-006**: A run with total effective tokens of at least 1,000,000 is handled without overflow.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
| - **T-FC-021**: Sampling respects `--days` historical window cutoff. | ||
| - **T-FC-022**: Run with missing `aw_info.json` artifact contributes zero ET and is still counted in `sampled_runs`. | ||
| - **T-FC-023**: Workflow with zero sampled runs produces nil projection with zero fields. | ||
| - **T-FC-024**: An in-progress run with a non-zero token usage snapshot is represented as a partial observation. |
| "run_summary_failed.json", | ||
| "run_summary_high_et.json", | ||
| "run_summary_cancelled.json", | ||
| "run_summary_partial_et.json", |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — commenting with a few targeted improvements, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test assertion completeness (
/tdd):TestFormal_FC_P11_PartialETFixtureverifiesstatusandtotal_effective_tokensbut notconclusion, leaving a silent drift path if the fixture later setsconclusion: null. - Naming consistency (
/grill-with-docs): Theempty-tool-name-deny.yamlfixture table row usesP1_ToolAllowedas its test ID while every other row usesT-GH-*IDs. - Spec term precision (
/grill-with-docs): Rule 4 of the degraded-endpoint policy introduces "independently authoritative endpoint" without a definition. - Changelog compaction trade-off (
/grill-with-docs): Collapsing1.0.15–1.0.19into one row removes per-version auditability for future bisects.
Positive Highlights
- ✅ New
run_summary_partial_et.jsonfixture is well-structured and covers the T-FC-024 partial-run scenario cleanly. - ✅ Linking fixture table entries directly to spec section anchors is a clear usability win.
- ✅ The
Structuresection inawf-config-sources-spec.mdis a good pattern for capturing the three-way conformance unit obligation. - ✅ Spec version pinning in the GitHub MCP README is a useful guardrail against fixture drift.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.9 AIC · ⌖ 7.22 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
specs/forecast-compliance-fixtures/run_summary_partial_et.json:6
[/tdd] The run_id field value 12345679 is one higher than sibling fixtures (which use 12345678). If intentional as a unique identifier, a short comment would prevent future readers from treating it as a typo.
<details>
<summary>💡 Suggestion</summary>
Add a JSON comment (or README note) clarifying the choice of ID, e.g. "intentionally distinct from run_summary_high_et.json (12345678)". Removes ambiguity when new fixtures are added.
</details>
@copilot please address this.
specs/forecast-compliance-fixtures/run_summary_partial_et.json:11
[/tdd] The fixture uses "conclusion": "" (empty string) for an in-progress run. If the spec allows both null and "", the test in TestFormal_FC_P11_PartialETFixture should also verify the conclusion field to make the expectation explicit.
<details>
<summary>💡 Suggestion</summary>
Add an assertion in the test:
assert.Equal(t, "", run["conclusion"],
"FC-P11 (T-FC-024): in-progress run must have an empty conclusion")This prevents a future fixture edit (changing to …
pkg/cli/forecast_compliance_fixtures_formal_test.go:57
[/tdd] TestFormal_FC_P10_MonteCarloInputCompleteness adds run_summary_partial_et.json with wantConclusion: "" and aicMustBeGT0: true. If AIC is expected > 0 for an in-progress run, the reasoning should be documented — is it because partial token usage is counted as AIC, or is this a boundary-condition test?
<details>
<summary>💡 Suggestion</summary>
Add a short comment above the entry explaining why aicMustBeGT0: true is expected for an in-progress run, tying it to T-FC-024 sema…
specs/compiler-threat-detection-spec.md:144
[/grill-with-docs] Collapsing 1.0.15–1.0.19 into a single row loses per-version auditability. A future bisect on a regression between e.g. 1.0.16 and 1.0.17 will have no row to consult.
<details>
<summary>💡 Suggestion</summary>
Consider keeping individual rows but collapsing their notes to a short delta form (e.g. "Same as 1.0.15; no new lock-file changes.") rather than removing the rows entirely. The compact changelog prose at the bottom partially compensates but is harder to scan…
specs/compiler-threat-detection-spec.md:156
[/grill-with-docs] The new rule 4 says the optimizer MUST NOT retry against a different degraded endpoint but leaves "independently authoritative endpoint" undefined. Without a clear definition, implementors may disagree about what qualifies.
<details>
<summary>💡 Suggestion</summary>
Add a parenthetical or footnote defining "independently authoritative endpoint", e.g.:
> (An endpoint is independently authoritative if it is declared in the workflow's authoritative-endpoints configu…
specs/github-mcp-access-control-compliance/empty-tool-name-deny.yaml:12
[/tdd] The fixture has a single scenario. An empty tool name against an allowed-tools: [] (empty list) is also an interesting boundary: the spec should clarify whether P1 fires in that case or falls through. A second scenario covering the empty-list case would make the fixture a complete spec for P1 edge cases.
<details>
<summary>💡 Suggested additional scenario</summary>
- scenario_id: "empty-tool-name-empty-allowed-list"
description: "Empty tool name with empty allowed-t…
</details>
<details><summary>specs/github-mcp-access-control-compliance/README.md:81</summary>
**[/grill-with-docs]** The new fixture table row for `empty-tool-name-deny.yaml` uses `P1_ToolAllowed` as the test ID rather than a `T-GH-*` identifier. All other rows in the table use `T-GH-*` IDs. Using a predicate name here breaks the ID convention and will confuse the fixture runner if it does ID-based lookup.
<details>
<summary>💡 Suggestion</summary>
Either assign a `T-GH-*` ID (e.g. `T-GH-034`) to this scenario in the spec and use that, or explicitly note that this fixture is predicate…
</details>
|
@copilot Please refresh this branch, verify checks, and use the pr-finisher skill before handing back. Unresolved review threads to address (newest first):
No failed checks were reported in the compact candidate set for this run. Branch update was requested by sous-chef in run https://github.com/github/gh-aw/actions/runs/31527012599.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed both review findings in e9fc3fc: added T-FC-024 to §12.2 and updated forecast fixtures/tests to unmarshal production-shaped RunSummary timestamps. The branch is refreshed; local lint, tests, and workflow recompilation all pass. |
|
@copilot Please refresh this branch, verify checks, and use the pr-finisher skill before handing back. Unresolved review threads to address (newest first):
No failed checks were reported in the compact candidate set for this run. Branch update was requested by sous-chef in run https://github.com/github/gh-aw/actions/runs/31531364207.
|
…ec-work-plan-2026-08-11 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed the branch from current |
|
@copilot Please refresh this branch, verify checks, and use the pr-finisher skill before handing back. No failed checks were reported in the compact candidate set for this run. Branch update was requested by sous-chef in run https://github.com/github/gh-aw/actions/runs/31535473039.
|
|
Great work! 🎯 This PR tightens SPDD compliance across multiple spec areas and includes formal test fixtures. Here's what stands out: ✅ Well-structured contribution:
✅ Follows project guidelines:
This PR is ready for review! 🚀
|
Verified |
|
@copilot This PR looks ready for another finishing pass. Please refresh the branch context, address any remaining reviewer feedback, and run the
|
Completed the finishing pass. |
Addresses remaining SPDD follow-ups across AWF config, compiler threat detection, forecast fixtures, and GitHub MCP access-control compliance.
AWF config conformance
Structuresection linking the spec, conformance index, andDriftRecordtests.Compiler threat detection
Forecast fixtures
run_summary_partial_et.jsonand formal validation for an in-progress, non-zero token snapshot.GitHub MCP access control
empty-tool-name-deny.yamlP1 fixture.Requested branch update from https://github.com/github/gh-aw/actions/runs/31535473039.> Generated by 👨🍳 PR Sous Chef · gpt54 · 8.59 AIC · ⌖ 5.18 AIC · ⊞ 8.5K · ◷