[ci-coach] Fix compiler-threat-detection-spec.md section numbering to restore TestFormal_* conformance - #59740
Conversation
…stFormal_* conformance The spec's section structure had drifted from the numbering scheme expected by pkg/workflow's formal conformance tests (TestFormal_RuleTestIDBijection, TestFormal_NoOrphanTestID, TestFormal_ActiveRuleCoverageComplete, TestFormal_DeprecationPolicy_SpecArtifactsAreParsed), which parse the spec by exact heading string (### 5.1 Core Rule Catalog, ### 7.1 Baseline Rule Mapping, ### 8.1 Test ID Catalog, ## 10. Change Log). The old spec used a different numbering (### 3.1 Rule Catalog, ## 5. Implementation Mapping, ## 6. Compliance Testing, ## 8. Change Log) causing these four unit tests to fail during pre-flight validation. Renumbered the document to 10 top-level sections with the expected subsections, preserving all normative content (rule catalog, mapping table, optimizer protocol, change log) and adding brief new subsections (Threat Model Overview, Governance and Responsibilities, Deprecated Test ID Retirement) to fill structural gaps. Also expanded the compliance testing section into a full per-rule Test ID Catalog (### 8.1) matching the format the formal tests' regex requires, while keeping the existing optimizer-protocol summary table as ### 8.2. Verified against the tests' actual parsing logic (regexes and section extraction helpers) via an equivalent Python simulation, since the sandboxed environment's outbound network policy blocks the Go module proxy needed to run `go test` directly here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
Pull request overview
Reorganizes the compiler threat detection specification to add clearer threat-model/governance context, expand compliance artifacts, and renumber sections to reflect the expanded structure.
Changes:
- Adds a Threat Model Overview and Governance/Responsibilities sections, shifting the rule catalog and requirements later in the document.
- Reworks the Rule Catalog into a “Core Rule Catalog” list and introduces explicit compiler response requirements and deprecation policy sections.
- Expands compliance documentation with a detailed Test ID catalog and clarifies optimizer protocol test coverage references.
Show a summary per file
| File | Description |
|---|---|
| specs/compiler-threat-detection-spec.md | Restructures and renumbers the spec; adds threat model/governance context; expands mapping and compliance test catalogs. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
| | **T-CTR-001** | CTR-001 Privilege Escalation | Reject unauthorized generated-job write permissions | Reject unauthorized generated-job write permissions. | `CTR-001` | | ||
| | **T-CTR-002** | CTR-002 Unpinned Action Integrity | Reject unpinned action references in strict contexts | Reject unpinned action references in strict contexts. | `CTR-002` | | ||
| | **T-CTR-003** | CTR-003 Unsafe Tool Scope Expansion | Reject or warn on policy-violating wildcard or overbroad tool scope | Reject or warn on policy-violating wildcard or overbroad tool scope. | `CTR-003` | | ||
| | **T-CTR-004** | CTR-004 Sandbox Bypass Configuration | Reject generated configuration that disables required sandboxing | Reject generated configuration that disables required sandboxing. | `CTR-004` | | ||
| | **T-CTR-005** | CTR-005 Unsafe Output Route | Reject direct write paths that bypass safe outputs | Reject direct write paths that bypass safe outputs. | `CTR-005` | | ||
| | **T-CTR-006** | CTR-006 Template Injection | Reject user-controlled expressions directly embedded in shell commands | Reject user-controlled expressions directly embedded in shell commands. | `CTR-006` | | ||
| | **T-CTR-007** | CTR-007 Markdown Content Security | Detect unsafe external markdown, including obfuscation, scripts, and social engineering | Detect unsafe external markdown, including obfuscation, scripts, and social engineering. | `CTR-007` | | ||
| | **T-CTR-008** | CTR-008 Pull Request Target Safety | Reject unsafe `pull_request_target` checkout patterns | Reject unsafe `pull_request_target` checkout patterns. | `CTR-008` | | ||
| | **T-CTR-009** | CTR-009 Shell Expansion in Safe-Outputs | Reject dangerous shell expansion in safe-output scripts | Reject dangerous shell expansion in safe-output scripts. | `CTR-009` | | ||
| | **T-CTR-010** | CTR-010 Expression Safety Allowlist | Reject unauthorized or multiline GitHub Actions expressions | Reject unauthorized or multiline GitHub Actions expressions. | `CTR-010` | | ||
| | **T-CTR-011** | CTR-011 Network Firewall Configuration | Reject missing firewall prerequisites and strict-mode wildcard domains | Reject missing firewall prerequisites and strict-mode wildcard domains. | `CTR-011` | |
| - **CTR-020 Conditional Import Security**: Reject `imports` entries containing `if`. | ||
| - **CTR-021 Workflow Run Trigger Branch Scope**: Warn, or reject in strict mode, for unscoped `workflow_run`; always reject missing `workflows`. | ||
| - **CTR-022 Git Subprocess Argument Injection**: Reject unsafe remote ref/path arguments before invoking Git. | ||
| - **CTR-023 Bash Command Allowlist Illusion**: Reject explicit bash restrictions for engines that cannot enforce them. |
| | **T-CTR-023** | CTR-023 Bash Command Allowlist Illusion | Reject explicit bash restrictions for engines that cannot enforce them | Reject explicit bash restrictions for engines that cannot enforce them. | `CTR-023` | | ||
| | **T-CTR-039** | CTR-025 Framework Self-Prompt Misattribution | Strip only a leading framework `<system>` block before analysis | Strip only a leading framework `<system>` block before analysis. | `CTR-025` | | ||
| | **T-CTR-041** | CTR-026 Generated Job Timeout Expression Injection | Reject non-positive or expression job timeout values | Reject non-positive or expression job timeout values. | `CTR-026` | | ||
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
This unblocks the current formal-heading failures, but it does it by widening the spec's drift surface and by reusing the existing published version for a materially different artifact shape.
Blocking themes
- Section 8.1 is now a second hand-maintained per-rule catalog, but the formal sync tests still only verify
Test ID -> Rule ID; the trigger/action/diagnostic columns can now drift silently. - The spec version/changelog were not advanced even though the document structure is part of the consumed surface for the conformance suite.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 68.7 AIC · ⌖ 5.69 AIC · ⊞ 22.3K
Comment /review to run again
|
|
||
| | Test ID | Rule | Detection Trigger | Expected Compiler Action | Stable Diagnostic ID | | ||
| |---------|------|--------------------|---------------------------|----------------------| | ||
| | **T-CTR-001** | CTR-001 Privilege Escalation | Reject unauthorized generated-job write permissions | Reject unauthorized generated-job write permissions. | `CTR-001` | |
There was a problem hiding this comment.
This table now duplicates normative trigger/action/diagnostic text for every CTR rule, but the formal sync suite still only validates the test ID-to-rule ID mapping, so a future rule edit can silently make Section 8.1 lie while these tests stay green.
💡 Add a real sync guard or keep this section as a crosswalk only
The PR fixes the current heading mismatch by turning Section 8.1 into a second per-rule catalog. That restores the parser, but it also creates a new source of truth for rule names, triggers, actions, and diagnostics without any test comparing those columns to Section 5.1 or the actual compiler diagnostics.
A safer direction is either to keep Section 8.1 as a compact Test ID -> Rule crosswalk, or to add a formal test that mechanically compares every Section 8.1 row against the canonical rule catalog instead of only extracting the first two columns.
| - gh-aw security architecture and safe-output specifications | ||
|
|
||
| ## 8. Change Log | ||
| ## 10. Change Log |
There was a problem hiding this comment.
The changelog still presents this as the same published spec version even though this PR changes the section layout and test-catalog shape that other artifacts and formal tests consume, so downstream consumers cannot tell which 1.0.31 document they actually validated against.
💡 Give the structural rewrite its own versioned changelog entry
The formal tests here parse headings by exact string, which means the document structure is part of the spec surface, not just editorial formatting. Reusing 1.0.31 for a materially different layout makes the spec version ambiguous and weakens reproducibility for anything that keys off the published version.
Please bump the spec version and add a matching Section 2 / Section 10 entry for this restructuring so the artifact remains version-stable.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — this is a clean, well-scoped documentation-only fix.
📋 Verification & Notes
Verification
I independently ran the exact target conformance tests against the PR's branch (not just the author's Python simulation) and confirmed all pass:
TestFormal_RuleTestIDBijection PASS
TestFormal_NoOrphanTestID PASS
TestFormal_ActiveRuleCoverageComplete PASS
TestFormal_DeprecationPolicy_* PASS (all subtests)
TestFormal_ComplianceReadmeNormTestNamesStaySynced PASS
Also confirmed no other file in the repo (Go source, other specs, docs) references the old section numbers via anchor links (#3-rule-model..., #5-implementation-mapping, etc.) — specs/compiler-threat-detection-compliance/README.md was already pointing at the new #71-.../#81-... anchors, consistent with the PR description's root-cause analysis.
Positive Highlights
- ✅ Root cause correctly diagnosed (spec drifted, not the tests or compliance README)
- ✅ No normative content deleted — only renumbered/expanded to fill structural gaps
- ✅ Scope discipline: left the 3 unrelated
TestWasmGolden_*version-pin failures untouched, as called out explicitly - ✅ Thorough validation methodology given the sandboxed network block on
proxy.golang.org
Minor, non-blocking note
Left one inline comment: the Section 10 Change Log doesn't record this renumbering itself, even though Section 2 says version changes MUST update the sync table. Since no normative content changed, this doesn't block merge, but a one-line editorial changelog entry would keep the audit trail complete for future readers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 88 AIC · ⌖ 15 AIC · ⊞ 10.4K
Comment /matt to run again
Comments that could not be inline-anchored
specs/compiler-threat-detection-spec.md:216
[/grill-with-docs] The Section 10 Change Log and Section 2 Spec-to-Implementation Sync table still only show 1.0.31 (audit-only), with no entry acknowledging this PR's section renumbering. Since Section 2 states "A version change MUST update this table," a future reader diffing history may wonder when/why numbering shifted from ### 3.1 to ### 5.1 etc.
<details>
<summary>💡 Suggestion</summary>
This is non-blocking since no normative content changed (only structural numbering to matc…
Pre-flight repair: fix
TestFormal_*conformance failuresPriority-1 fix (per CI Coach's pre-flight repair path):
test-unitfailed pre-flight validation with 7 failing tests, 4 of which were caused byspecs/compiler-threat-detection-spec.mddrifting out of sync with the section-numbering scheme that severalpkg/workflowformal conformance tests parse by exact heading string.Root cause
TestFormal_RuleTestIDBijectionTestFormal_NoOrphanTestIDTestFormal_ActiveRuleCoverageCompleteTestFormal_DeprecationPolicy_SpecArtifactsAreParsedall extract sections of
compiler-threat-detection-spec.mdby literal heading match (e.g.### 5.1 Core Rule Catalog,### 7.1 Baseline Rule Mapping,### 8.1 Test ID Catalog,## 10. Change Log). The spec had drifted to an older numbering scheme (### 3.1 Rule Catalog,## 5. Implementation Mapping,## 6. Compliance Testing,## 8. Change Log), so these headings weren't found and the tests failed.The compliance README (
specs/compiler-threat-detection-compliance/README.md) already referenced the new numbering scheme in its cross-links, confirming the spec file — not the tests or README — was the artifact that had drifted.Fix
Renumbered
specs/compiler-threat-detection-spec.mdinto 10 top-level sections matching what the tests expect, preserving all existing normative content (rule catalog, implementation mapping, optimizer protocol, change log) and adding a few small new subsections to fill structural gaps required by the renumbering:### 3Threat Model Overview (new, brief)### 4Governance and Responsibilities (new, brief)### 8.1Test ID Catalog — expanded into a full per-rule table (previously a coarser summary) to match the exact row format the tests' regex requires### 8.3Deprecated Test ID Retirement (new, documents existingrequiredTestIDs()exclusion behavior)No implementation code, other specs, or the compliance README were changed — only
specs/compiler-threat-detection-spec.md.Validation
The sandboxed environment's outbound network policy blocks
proxy.golang.org(403 Forbidden for missing module dependencies), sogo testcould not be run directly here. Instead, validated correctness by re-implementing the exact parsing logic used by each failing test (formalSpecSection,formalActiveRules,formalCatalogTests,formalComplianceMap,specSection,parseCTRSpecArtifacts,verifyDeprecationPolicy, and their associated regexes) in a Python simulation run against the new spec file, and confirmed all assertions pass:Out of scope
3 additional pre-existing
TestWasmGolden_*failures (smoke-copilot,playwright-cli-mode,gemini) are unrelated version-pin drift (@playwright/cli0.1.18→0.1.19, gemini engine 0.55.1→0.59.0) in golden fixtures, not caused by this spec — left untouched per the "don't bundle unrelated fixes" guidance.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.