Repository navigation
Refresh SPDD specification evidence and coverage - #74554
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
What this change does: This refreshes safe-output outcome evidence and fixtures, adds governance planning detail, and reconciles the security documentation with a current lock check. I reviewed the changed lines only for unnecessary maintenance surface.
Should fix:
-
The deprecated scratchpad becomes a second source of truth (
scratchpad/safe-outputs-specification.md:L1295-L1319)- What this is: The archive now contains a full §3–§5 implementation map and a rule to keep it synchronized.
- Problem: Every mapped implementation change now needs a second documentation update even though the banner says the canonical spec is authoritative and this file is stale.
- Fix: Remove the new mapping and follow-up, retaining only a short historical link.
- If we skip it: The archive will drift or turn every future change into duplicate maintenance.
-
The outcome map repeats a 30-item inventory (
specs/safe-output-outcome-evaluation.md:L866-L897)- What this is: The patch adds a per-operation path/test table beside the existing acceptance map and the new safeguards matrix.
- Problem: Adding or renaming one output now requires keeping three operation lists aligned.
- Fix: Fold the path/test columns into the existing map, or link to it, and remove this duplicate table.
- If we skip it: The specification will accumulate stale cross-references as safe-output types change.
-
The file-level plan duplicates the roadmap (
specs/intent-attribution-agent-governance.md:L1357-L1365)- What this is: A seven-row file/expected-output plan is added immediately after the existing phase deliverables.
- Problem: Phases 2 and 4 repeat the same resolver, formal-test, README, and fixture paths, while the new paragraph also creates a second synchronization obligation for the compliance README.
- Fix: Keep the phase goals here and move file targets to the implementation tracker when a phase starts.
- If we skip it: Future implementation moves will require updating both the roadmap and this speculative file list.
Verdict: fix 1, 2, and 3 first.
Not checked: correctness, security, performance, and generated/lock content, per the review scope.
Lean: -68 lines possible.
Generated by ✂️ Ponytail Reviewer for #74554 · codex · routed: gpt56 max · ~4 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
|
|
||
| This mapping is retained for historical traceability only; the canonical specification linked in the archival notice remains authoritative. | ||
|
|
||
| | Specification area | Implementation mapping | |
There was a problem hiding this comment.
Lean: This deprecated archive now adds a 14-row §3–§5 implementation map and explicitly asks maintainers to keep it synchronized, so every mapped implementation change needs a second doc update even though the banner says the canonical spec is authoritative. Remove the new map/follow-up and keep only a historical link.
There was a problem hiding this comment.
Removed the new archive mapping and sync follow-up; the scratchpad retains its historical mappings only. Applied in 67625f4.
| | `## Formal Model` | `outcomes/OutcomeEvaluation.tla`, `outcomes/OutcomeEvaluation.cfg`, `outcomes/check.mjs` | | ||
| | `## Legacy Runtime Safeguard Tests`, `## Runtime Test Suite` | `pkg/cli/outcome_eval_formal_test.go`, `pkg/cli/outcome_conformance_test.go` | | ||
|
|
||
| | Section | Safe output | Configuration/compilation | Outcome evaluator | Focused outcome test | |
There was a problem hiding this comment.
Lean: This starts a second 30-operation inventory beside the existing Current Default Acceptance Map and the new Safeguards matrix; adding or renaming one output now requires keeping all three lists aligned. Fold the path/test columns into the existing map (or link to it) and delete this duplicate table.
There was a problem hiding this comment.
Removed the duplicate per-operation traceability inventory; the safeguards matrix and existing acceptance map remain. Applied in 67625f4.
| | Phase | Target files | Expected output | | ||
| |---|---|---| | ||
| | 1. Current foundation | `pkg/intent/resolver.go`, `pkg/intent/policy.go`, `pkg/cli/outcome_eval.go`, `pkg/cli/outcome_eval_enrich.go`; `specs/intent-attribution-compliance/README.md`, `explicit-intent-wins.yaml` | Preserve compatibility mapping and weighted outcome reporting while documenting the baseline fixture contract. | | ||
| | 2. Honest attribution model | `pkg/intent/resolver.go`, `pkg/intent/resolver_test.go`, `pkg/intent/intent_formal_test.go`; `specs/intent-attribution-compliance/README.md`, `ambiguous-root-closing-issues.yaml`, `unlinked-pr-fail-closed.yaml` | Emit explicit attribution states, source, root identity, nullable unknown weight, and conformance fixtures for ambiguous/unlinked cases. | |
There was a problem hiding this comment.
Lean: The file-level plan repeats the roadmap immediately above—phases 2 and 4 both list the same resolver, formal-test, README, and two fixture paths—and also introduces a second synchronization obligation for the compliance README. Keep the existing phase deliverables here and move file targets to the implementation tracker when a phase starts.
There was a problem hiding this comment.
Removed the speculative file-level phase table to avoid duplicating the roadmap. Applied in 67625f4.
There was a problem hiding this comment.
🟡 Changes recommended
The new fixture types are missing from the formal checker’s kinds map, causing projection to abort before model checks can run.
6 open issues
What changed in this PR
This PR refreshes gh-aw’s specification evidence and implementation traceability, addressing the maintenance work in #74553 without changing runtime implementation.
Changes:
- Updates security validation evidence and distinguishes historical validation from the current partial check.
- Documents failure and unknown outcomes for 30 safe-output operations and adds 12 conformance fixtures.
- Clarifies governance questions, defines file-level implementation plans, and extends archival traceability mappings.
| File | Description |
|---|---|
specs/security-architecture-spec-validation.md |
Refreshes lock-file evidence, permission observations, and validation status. |
specs/security-architecture-spec-summary.md |
Adds architecture headings and reconciles validation qualifications. |
specs/safe-output-outcome-evaluation.md |
Adds safeguards and maps operations to evaluators and tests. |
specs/intent-attribution-agent-governance.md |
Records open questions and file-level plans for seven phases. |
scratchpad/safe-outputs-specification.md |
Extends historical mappings while preserving canonical-spec authority. |
pkg/cli/testdata/outcome_conformance.json |
Adds unsupported, generic-unknown, and API-failure fixtures. |
Review effort: Balanced (auto)
| { "name": "unsupported update discussion", "item": { "type": "update_discussion", "number": 1 }, "responses": {}, "expected": { "outcome_status": "unknown", "evidence_strength": "none", "signal": "unsupported_evaluator" } }, | ||
| { | ||
| "name": "generic reply target existence is unknown", | ||
| "item": { "type": "reply_to_pull_request_review_comment", "number": 1 }, |
There was a problem hiding this comment.
Registered all five fixture types as generic in specs/outcomes/check.mjs in 67625f4. The checker could not run here because the pinned TLA2TOOLS_JAR is unavailable.
|
|
||
| | Section | Safe output | Failure outcome | Unknown or unverifiable outcome | | ||
| |---|---|---|---| | ||
| | 1 | `create_pull_request` | Primary-object `404` is `rejected`/`deleted`; other API failures are `error`/`evaluation_error`. | Missing PR reference is `unknown`/`missing_reference`. | |
There was a problem hiding this comment.
Clarified that supplementary-effort failures do not downgrade an accepted merged-PR result; they only withhold zero-touch proof. Applied in 67625f4.
|
|
||
| The normative requirements remain in [`security-architecture-spec.md`](security-architecture-spec.md), Sections 4–12. This summary describes their architecture and evidence; it does not replace those requirements. | ||
|
|
||
| ### Evidence |
There was a problem hiding this comment.
Added the missing Entities, Approach, and Structure headings and content alongside the other REASONS dimensions in 67625f4.
|
|
||
| ### Current Compiled Job Structure (2026-11-03) | ||
|
|
||
| Evidence was checked in `.github/workflows/security-review.lock.yml`. The generated file contains all six named jobs; their YAML declaration order is not execution order. |
There was a problem hiding this comment.
Expanded the validation evidence to all ten jobs and included the omitted permission scopes in 67625f4.
| **Implementation Validation** (`.github/workflows/security-review.lock.yml`, checked 2026-11-03): The `head.repo.id == github.repository_id` comparison appears in step-level conditions at lines 252 and 326. These conditions do not establish a job-wide fork block for every trigger path. | ||
|
|
||
| **Status**: ✅ **VERIFIED** - Repository ID comparison present in multiple job conditions. | ||
| **Status**: ⚠️ **UNVERIFIED** — The current lock evidence is insufficient to verify the complete PM-08 claim. The evidence gap is recorded in [#74553](https://github.com/github/gh-aw/issues/74553); do not rely on the former job-level assertion. |
There was a problem hiding this comment.
PM-08 remains UNVERIFIED and no dedicated follow-up issue has been created or linked; the report still says it is to be created. The PR description still references #74553. I could not create or edit GitHub issues/PR metadata with the available operations, so this concern remains open.
| | Workflow | Current group pattern | Queue setting | Evidence | | ||
| |----------|----------------------|--------------|----------| | ||
| | Security Review Agent | Workflow plus issue/PR/run ID | `queue: max` | `.github/workflows/security-review.lock.yml:78-80` (2026-11-03) | | ||
| | Security Compliance | Workflow plus issue number | Default queue behavior | `.github/workflows/security-compliance.lock.yml:83-85` | |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs to the spec changes and /diagnosing-bugs to the checker gap. Requesting changes: the evidence claims about the compiled lock file are inaccurate, and the new formal-checker coverage will fail.
📋 Key Themes & Highlights
Key Themes
- Job topology is wrong:
security-review.lock.ymlhas 10 top-level jobs, not six. Four are missing from the validation table (admission,conclusion_admission,evals,push_evals_state), and thesix-jobwording repeats across both security specs. - Dependency and concurrency rows contradict the cited locks:
activationalso needsadmission, andsecurity-compliance.lock.yml:85setsqueue: max, not default queueing. - Checker will fail on the new corpus:
specs/outcomes/check.mjshas nokindsentries for five fixture types the PR adds.project()throws on 10 of 94 cases. I verified this statically; the TLC jar was not available to run it. - Duplicate heading:
intent-attribution-agent-governance.mdhas two### Open Questions and Proposed Resolutionssections.
Positive Highlights
- ✅ All 96 external
uses:refs in the lock are SHA-pinned. - ✅ The 94-case conformance fixture set has no duplicate names, and all 34 cited
TestOutcomeConformancesubtests exist. - ✅
go test ./pkg/cli/passes for the outcome conformance tests. - ✅ The cited source paths under
pkg/cliandpkg/workflowexist.
🔍 Verification scope
I checked each claim against the lock file, the fixture file, the checker source, and the diff. I did not run TLC. Concurrency behaviour beyond the cited config was not established. Inline comments cover the highest-impact locations; a full sweep of every six-job mention is left to the author.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~8 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
|
|
||
| ### Current Compiled Job Structure (2026-11-03) | ||
|
|
||
| Evidence was checked in `.github/workflows/security-review.lock.yml`. The generated file contains all six named jobs; their YAML declaration order is not execution order. |
There was a problem hiding this comment.
[/grill-with-docs] The claim that the lock contains all six named jobs is inaccurate. .github/workflows/security-review.lock.yml declares 10 top-level jobs: activation, admission, agent, conclusion, conclusion_admission, detection, evals, pre_activation, push_evals_state, and safe_outputs. The table below omits admission, conclusion_admission, evals, and push_evals_state, and the six-job wording repeats at lines 61, 63, 145, 661, and 676.
💡 Suggested fix
Reword to 10 top-level jobs, add the four missing rows (lock lines: admission 583, conclusion_admission 2279, evals 2708, push_evals_state 2951), and update every six-job occurrence in this file and in security-architecture-spec-summary.md.
@copilot please address this.
There was a problem hiding this comment.
Updated the validation report and related wording to reflect the ten-job structure. Applied in 67625f4.
| | Job | Current evidence | Observed structure and permissions | | ||
| |---|---|---| | ||
| | `pre_activation` | `security-review.lock.yml:2884-2949` | Checks command position and membership; grants `contents: read` and `pull-requests: read`. | | ||
| | `activation` | `security-review.lock.yml:93-107`, `:298-310` | Depends on `pre_activation`; checks the lock timestamp. It also has `issues: write` and `pull-requests: write`. | |
There was a problem hiding this comment.
[/grill-with-docs] The activation row says it depends only on pre_activation. In the lock, its needs are [admission, pre_activation], and its if: checks needs.admission.result, so admission is a missing prerequisite.
💡 Suggested fix
Change the row to Depends on admission and pre_activation, and add an admission row (security-review.lock.yml:583).
@copilot please address this.
There was a problem hiding this comment.
Added the admission job and corrected activation to depend on both admission and pre_activation. Applied in 67625f4.
| | Workflow | Current group pattern | Queue setting | Evidence | | ||
| |----------|----------------------|--------------|----------| | ||
| | Security Review Agent | Workflow plus issue/PR/run ID | `queue: max` | `.github/workflows/security-review.lock.yml:78-80` (2026-11-03) | | ||
| | Security Compliance | Workflow plus issue number | Default queue behavior | `.github/workflows/security-compliance.lock.yml:83-85` | |
There was a problem hiding this comment.
[/grill-with-docs] The Security Compliance row says Default queue behavior, but .github/workflows/security-compliance.lock.yml:85 sets queue: max, the same as the security-review lock. The row contradicts the file it cites.
💡 Suggested fix
Change the cell to queue: max. The :83-85 reference already points at the right lines.
@copilot please address this.
There was a problem hiding this comment.
Corrected the Security Compliance queue entry to queue: max in 67625f4.
| | `### Sync Notes` | This section-to-implementation and test table | | ||
| | `### OTel Backend Unavailability` | No outcome OTel exporter is currently implemented; outcome records are produced by `pkg/cli/outcome_eval.go` | | ||
| | `### Conformance Safeguard Coverage Requirements` | `pkg/cli/outcome_conformance_test.go`, `pkg/cli/testdata/outcome_conformance.json` | | ||
| | `## Formal Model` | `outcomes/OutcomeEvaluation.tla`, `outcomes/OutcomeEvaluation.cfg`, `outcomes/check.mjs` | |
There was a problem hiding this comment.
[/diagnosing-bugs] This section says the checker projects the shared fixture corpus, but specs/outcomes/check.mjs has no kinds entries for reply_to_pull_request_review_comment, autofix_code_scanning_alert, create_code_scanning_alert, link_sub_issue, or update_release. project() throws Unmapped fixture type (line 75) for them, which covers 10 of the 94 cases added here, so the checker will fail on this corpus.
💡 Suggested fix
Add the five types to kinds with their TLA+ mappings, then run node specs/outcomes/check.mjs with the pinned TLA2TOOLS_JAR. I could only verify this statically, since the jar is not available here.
@copilot please address this.
There was a problem hiding this comment.
Added the five missing generic fixture mappings to the checker in 67625f4; the pinned TLC JAR was unavailable for running the model check locally.
|
|
||
| ### Operations | ||
|
|
||
| The representative workflow orders its security jobs as `pre_activation` → `activation` → `agent` → `detection` → `safe_outputs`, followed by `conclusion`. The conclusion job is terminal reporting/cleanup, not a required security layer. See the current job table in the [validation report](security-architecture-spec-validation.md#current-compiled-job-structure-2026-11-03). |
There was a problem hiding this comment.
[/grill-with-docs] The execution order omits admission, evals, and push_evals_state. The lock runs evals after detection, and push_evals_state after evals, so this sequence is incomplete.
💡 Suggested fix
Use the lock graph: pre_activation → admission → activation → agent → detection → (evals, safe_outputs) → push_evals_state, with conclusion last.
@copilot please address this.
There was a problem hiding this comment.
Updated the execution order to include admission, evals, and push_evals_state, with safe_outputs parallel after detection and conclusion jobs last. Applied in 67625f4.
| | Sync PM-11 formal coverage into behavioral coverage map | ✅ Done (2026-07-15) | Added `TestFormalPM11_PreActivationContainsMembershipStep` to the behavioral coverage map and generated suite notes; formal suite now tracks 16 tests in `pkg/workflow/security_architecture_sg_formal_test.go` | | ||
| | Sync §12 CS/RS coverage update from daily SPDD queue | ✅ Done (2026-07-28) | Added formal coverage map/test-suite entries for T-CS-001/002 and T-RS-003..008; synced to `specs/security-architecture-spec-validation.md` §12 matrix | | ||
| | Add RS-05a for workflow_dispatch + aw_context PR checkout | ✅ Done (2026-08-01) | Security threat review cleared; added RS-05a to §11.3 (repository scope, actor trust, parse resilience, ref isolation); validation §7a added with 9-test unit evidence; §12 matrix updated; spec bumped to v1.0.1 | | ||
| | Recheck compiled six-job structure | ✅ Targeted check (2026-11-03) | `.github/workflows/security-review.lock.yml` confirms the six jobs; full revalidation remains pending and PM-08 is UNVERIFIED in the validation report | |
There was a problem hiding this comment.
[/grill-with-docs] This row marks the job recheck as ✅ and says it confirms the six jobs. The lock has 10 top-level jobs, so the claim is wrong and the ✅ status is not supported.
💡 Suggested fix
Reword to the verified count of 10 and list the job names, matching the corrected table in security-architecture-spec-validation.md.
@copilot please address this.
There was a problem hiding this comment.
Changed the recheck statement to identify all ten jobs rather than six. Applied in 67625f4.
|
|
||
| A lower-precedence rule may not weaken a higher-precedence constraint. | ||
|
|
||
| ### Open Questions and Proposed Resolutions |
There was a problem hiding this comment.
[/grill-with-docs] ### Open Questions and Proposed Resolutions now appears twice in this file (lines 658 and 764). The numbering runs 1 to 3 and then 4 to 6, so the content is split across two identical headings. That breaks anchor links and makes the section harder to find.
💡 Suggested fix
Merge the two sections into one, or give the second heading a distinct name that describes its scope, and update any anchors that point to it.
@copilot please address this.
There was a problem hiding this comment.
Renamed the two question sections to distinct Multiple-Root and Policy Precedence headings in 67625f4.
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74603 PR finisherSubmitted for publication (not confirmed pushed): merged
Local validation: Remaining: CI needs a maintainer re-trigger. A maintainer should create the dedicated PM-08 follow-up issue and link it. Review replies and thread resolutions are deferred until the push is published. Failing PR finisher dispatch requested for the following outstanding work: (see Sous-chef lines below) Sous-chef head: 9ade5bf
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74630 PR FinisherSubmitted for publication (not confirmed pushed): merged latest
Validation: Deferred or not changed:
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74635 PR Finisher pass Submitted for publication (not yet confirmed pushed): a merge of
Validation: Review replies and resolutions are deferred until the fix is published. Remaining human actions: create or link a dedicated PM-08 tracker, re-trigger CI, review. Sous-chef head: 9ade5bf
|
|
Commit pushed:
|
PR Finisher summaryChanges are submitted for publication, not confirmed pushed:
Local validation: Remaining human actions:
Sous-chef head: 9ade5bf
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74659 PR finisherSubmitted for publication (staged, not confirmed pushed): a merge of Already present in head (no change needed): checker Validation: Sous-chef head: 67625f4
|
|
@copilot run pr-finisher skill |
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74664 PR FinisherSubmitted for publication (not confirmed pushed): merge of Not done (hand-off): the archive-map comment (scratchpad Sync Notes was already on Re-trigger CI after publication. Sous-chef head: 67625f4
|
Ran the PR finisher pass. Formatting, lint, the final impacted-unit gate, and |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This update addresses specification gaps in implementation traceability, explicit failure and unknown outcomes, governance ambiguity, and validation freshness.
UNVERIFIED.