Preserve Runner Guard suppressions for model inventory requests#52064
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ 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.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is clean and correct. preserveRunnerGuardStepSuppressions correctly injects suppression comments only when step names are unique in both frontmatter and generated YAML, preventing ambiguous matches. Indentation is derived from the generated line, and inline script comments that happen to contain the prefix are correctly ignored. Tests cover the positive case, duplicate-name guard, and misplaced inline comment. No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.8 AIC · ⌖ 6.13 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — approving with two minor suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Compiler integration is a single, well-placed call in
compiler_yaml.go - ✅ Duplicate-name and misplaced-script-comment guards are present and tested
- ✅ Tests cover happy path, script-comment false positive, and duplicate-name skipping
- ✅ Suppression justifications are endpoint-specific and accurate
Minor Issues (non-blocking)
- Silent map overwrite — when two different directives precede two identically-named steps in the frontmatter, the second overwrites the first, then the duplicate-name guard drops it. The net result is correct (no injection) but the overwrite path is undetected and untested.
- Per-line
workflowStepNamein the injection loop —countWorkflowStepNamesalready iterates all lines; the injection loop iterates them again callingworkflowStepNameper line. Minor inefficiency, but worth noting for very large workflows.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.6 AIC · ⌖ 6.7 AIC · ⊞ 7.1K
Comment /matt to run again
| if !strings.HasPrefix(directive, runnerGuardIgnorePrefix) { | ||
| continue | ||
| } | ||
| if stepName := workflowStepName(frontmatterLines[i+1]); stepName != "" { |
There was a problem hiding this comment.
[/diagnosing-bugs] If two different runner-guard:ignore directives precede two identically-named steps in the frontmatter, the map silently overwrites the first — then the duplicate-name guard on line 31 drops both. A directive for an ambiguous step name disappears with no warning.
💡 Suggested fix
Detect the collision and skip the duplicate explicitly:
if _, ok := suppressions[stepName]; ok {
delete(suppressions, stepName) // ambiguous: skip both
continue
}
suppressions[stepName] = directiveOr at minimum add a test documenting this silent-overwrite behaviour.
@copilot please address this.
| lines := strings.Split(workflowYAML, "\n") | ||
| output := make([]string, 0, len(lines)+len(suppressions)) | ||
| for _, line := range lines { | ||
| stepName := workflowStepName(line) |
There was a problem hiding this comment.
[/codebase-design] workflowStepName is called on every line of both the frontmatter and the generated YAML. For large workflows this is O(n2) in the number of lines × steps. Consider building the step-name index once per string (already done by countWorkflowStepNames) and reusing it when injecting, instead of calling workflowStepName again in the injection loop.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Preserves scoped Runner Guard suppressions when compiling workflow frontmatter into generated GitHub Actions workflows.
Changes:
- Adds suppression extraction and step-name matching.
- Adds unit coverage for preservation, duplicates, and script comments.
- Adds four RGS-012 suppressions and regenerates the model inventory workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/runner_guard_suppressions.go |
Implements suppression preservation. |
pkg/workflow/runner_guard_suppressions_test.go |
Tests suppression handling. |
pkg/workflow/compiler_yaml.go |
Integrates preservation into compilation. |
.github/workflows/daily-model-inventory.md |
Adds scoped RGS-012 directives. |
.github/workflows/daily-model-inventory.lock.yml |
Regenerates compiled workflow output. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| stepName := workflowStepName(line) | ||
| if directive := suppressions[stepName]; directive != "" && frontmatterNames[stepName] == 1 && generatedNames[stepName] == 1 { |
| if !strings.HasPrefix(directive, runnerGuardIgnorePrefix) { | ||
| continue | ||
| } | ||
| if stepName := workflowStepName(frontmatterLines[i+1]); stepName != "" { |
…compilation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (117 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
|
|
@copilot Please address the open review feedback, refresh the branch if needed, and use the pr-finisher skill before handing back. Open review threads (newest reviewer feedback first):
No failed checks were reported in the compact candidate set for this run.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all four open review threads in
Added five tests covering inline directives, standalone directives on unnamed steps, ambiguous directives, and block-scalar payloads on both the frontmatter and generated sides. |
|
@copilot Please address the open review feedback, resolve any addressed threads that remain open, refresh the branch if needed, and use the pr-finisher skill before handing back. Open review threads (newest reviewer feedback first):
No failed checks were reported in the compact candidate set for this run.
|
|
@copilot resolve the merge conflicts on this branch. |
PR #52083 landed the RGS-012 suppressions on main using the repo's established convention: the directive is placed inside the run script directly above the offending curl, where the compiler emits it verbatim. That makes the step-level comment preservation added here unnecessary, so the compiler post-processing, its tests, the ADR and the duplicate step-level suppressions are removed in favour of main's version. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
This PR is now superseded and should be closed rather than merged. While working the open threads I found that #52083 landed on main ( # runner-guard:ignore RGS-012 -- unauthenticated GET from a public read-only model index; no secrets are sent.
curl -fsS https://models.dev/api.json -o "$OUT/api.json"The directive goes inside the That invalidates the premise of this PR. My step-level comment preservation solved a problem that doesn't exist, and it did so at the wrong layer: the two reviewer threads about block-scalar payloads and unrecognised directive forms were both symptoms of trying to reconstruct, via raw-text post-processing, an association that the native in-script form never loses. Actions taken in
The branch is now byte-identical to I did not close the PR, as that is outside what I should decide unilaterally. |
…ecret-exfiltration-again Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in The conflicts were in Worth flagging before you spend review time on this: the branch is now byte-identical to That's because #52083 landed on main ( Recommend closing this as superseded by #52083. I'll leave that call to you. |
Runner Guard flagged four intentional, read-only model inventory requests as potential secret exfiltration. Existing source-level suppressions were discarded during workflow compilation.
Workflow
daily-model-inventory.lock.yml.Compiler