[WIP] Fix collect ndjson mention sanitizer for allowed teams#50379
Conversation
…ion allowed-teams resolution Co-authored-by: pelikhan <[email protected]>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ 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 (14 additions). |
|
✅ 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. ✅ |
There was a problem hiding this comment.
Pull request overview
Fixes mention sanitization so ingest uses the configured safe-output GitHub token when resolving allowed teams.
Changes:
- Adds effective safe-output token resolution to the ingest step.
- Regenerates workflow lockfiles with the explicit token input.
- A focused regression test is still needed.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml_step_lifecycle.go |
Adds ingest token wiring. |
.github/workflows/*.lock.yml |
Propagates the generated token input across all changed workflow lockfiles. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 274/274 changed files
- Comments generated: 1
- Review effort level: Balanced
| configToken := "" | ||
| if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { | ||
| configToken = data.SafeOutputs.GitHubToken | ||
| } | ||
| fmt.Fprintf(yaml, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(configToken)) |
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Infrastructure
📊 Analysis Summary
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: Request changes — the fix only covers the PAT/custom-token path, not the GitHub App path
Themes
- The one-line change correctly mirrors
getEffectiveSafeOutputGitHubTokenprecedence used by theProcess Safe Outputsstep for the non-App case, and is otherwise a clean, minimal, well-commented change (274 lock.yml files correctly recompiled). - However, when
safe-outputs.github-appis configured,Process Safe Outputsuses the minted app token (steps.safe-outputs-app-token.outputs.token), which this Ingest step — per its own added comment — cannot reference since it runs in a different job. That means for App-based configs (the case most likely needingread:orgfor allowed-teams resolution), the reported bug is still present. - No test coverage was added to lock in the new
github-token:line for the Ingest step, so this behavior (and its App-token gap) is unguarded against regression.
🔎 Code quality review by PR Code Quality Reviewer · auto · 46.6 AIC · ⌖ 5.51 AIC · ⊞ 7.9K
Comment /review to run again
| if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { | ||
| configToken = data.SafeOutputs.GitHubToken | ||
| } | ||
| fmt.Fprintf(yaml, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(configToken)) |
There was a problem hiding this comment.
This fix only closes the gap for the custom-token/GH_AW_GITHUB_TOKEN case — when safe-outputs.github-app is configured (the more privileged, read:org-capable path), the mismatch this PR claims to fix still exists.
💡 GitHub App token case is still unaddressed
getEffectiveGitHubTokenForSafeOutputStep (github_token.go:163-179) shows that when SafeOutputs.GitHubApp is set, Process Safe Outputs uses ${{ steps.safe-outputs-app-token.outputs.token }} almost exclusively (or combined with the PAT fallback only if shouldIgnoreMissingKey()). This Ingest step, however, runs in the main job and — per the added comment — cannot reference that step output at all, so it always falls back to configToken/GH_AW_GITHUB_TOKEN/GITHUB_TOKEN, never the app token.
For any workflow using github-app (the exact scenario likely to need read:org for allowed-teams), the original bug (mention sanitized with a token lacking org read access, then never un-escaped) is not actually fixed by this PR — only the "configured custom github-token" scenario is.
Suggested direction: either (a) mint a lightweight app token in the main job before this step too (duplicating minimal permissions), or (b) explicitly document/log that github-app-configured safe-outputs still cannot correctly sanitize mentions during ingest, so users aren't misled that this PR fully resolves #50282 for their setup.
| if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { | ||
| configToken = data.SafeOutputs.GitHubToken | ||
| } | ||
| fmt.Fprintf(yaml, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(configToken)) |
There was a problem hiding this comment.
No test asserts the emitted github-token: value for the Ingest agent output step, so this behavior change (and the App-token gap above) can silently regress.
💡 Missing regression coverage
generateOutputCollectionStep is exercised by agentic_output_test.go, allowed_domains_sanitization_test.go, and compile_config_test.go, but none of them grep the compiled YAML for a github-token: line under the Ingest agent output step. Since this fix's entire purpose is to make the token used here match the one used by Process Safe Outputs, a test should assert:
// e.g. in agentic_output_test.go
if !strings.Contains(lockContent, "github-token: ${{ secrets.MY_CUSTOM_TOKEN") {
t.Errorf("expected custom safe-outputs github-token to be wired into Ingest step")
}for at least: (1) default fallback case, (2) safe-outputs.github-token configured case. Without this, a future refactor of generateOutputCollectionStep or getEffectiveSafeOutputGitHubToken could silently drop the with: block or revert to the wrong token, and CI wouldn't catch it.
There was a problem hiding this comment.
The fix is correct and well-scoped.
The generateOutputCollectionStep now passes the same effective safe-outputs GitHub token to the actions/github-script "Ingest agent output" step, matching the token resolution used by the "Process Safe Outputs" handler manager. This ensures collect_ndjson_output.cjs resolves allowed-teams membership with a token that has read:org access, preventing premature backtick-escaping of valid team mentions.
The acknowledged limitation — that the GitHub App token (steps.safe-outputs-app-token.outputs.token) cannot be used here because it is minted in a separate job — is correctly documented, and the GH_AW_GITHUB_TOKEN || GITHUB_TOKEN fallback is the right approach. All lock files are regenerated consistently.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 42.1 AIC · ⌖ 12.1 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one actionable finding on test coverage.
📋 Key Themes & Highlights
Root cause
The fix is correct: generateOutputCollectionStep was emitting github-token: ${{ secrets.GITHUB_TOKEN }} (default) instead of the configured safe-outputs.github-token, causing allowed-teams resolution to fail with 404 during mention sanitization in the ingest step. Process Safe Outputs then received already-escaped bodies and never restored them.
Finding
The existing compile_config_test.go exercises generateOutputCollectionStep but does not assert the github-token: line is present or that a custom token is forwarded correctly. See the inline comment on line 330 for the suggested test additions.
Positive Highlights
- ✅ Token precedence logic reuses
getEffectiveSafeOutputGitHubToken, keeping it consistent with the Process Safe Outputs step - ✅ Code comment clearly explains why the App token from the other job can't be used
- ✅ 274 lock files correctly recompiled in one shot via
make recompile
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.1 AIC · ⌖ 8.02 AIC · ⊞ 7.1K
Comment /matt to run again
| if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { | ||
| configToken = data.SafeOutputs.GitHubToken | ||
| } | ||
| fmt.Fprintf(yaml, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(configToken)) |
There was a problem hiding this comment.
[/tdd] No test asserts that github-token: is emitted by generateOutputCollectionStep, or that SafeOutputs.GitHubToken is respected vs. the default fallback. A future refactor of the token-resolution chain could silently regress this fix.
💡 Suggested test cases
Add assertions in compile_config_test.go where generateOutputCollectionStep is already called:
// after generating YAML, assert:
if !strings.Contains(generatedYAML, "github-token:") {
t.Errorf("expected github-token line in ingest step output")
}And add a table case with safe-outputs.github-token set, asserting the custom token appears verbatim in the output.
@copilot please address this.
|
@copilot Please address the remaining blocking review feedback on this PR, refresh the branch if needed, then run the Open review feedback to close out:
Please resolve addressed review threads once the reviewer feedback has been handled.
|
Co-authored-by: gh-aw-bot <[email protected]>
Co-authored-by: gh-aw-bot <[email protected]>
Addressed in 0f3ad37. Added scoped compiler coverage that asserts the |
collect_ndjson_output.cjs(Ingest agent output step) sanitizes mentions using defaultGITHUB_TOKEN, ignoringsafe-outputs.github-token, causingallowed-teamsresolution to fail (404) and mentions to be backtick-escaped before Process Safe Outputs ever sees them with the correct token.github-token:input to the "Ingest agent output"actions/github-scriptstep ingenerateOutputCollectionStep(pkg/workflow/compiler_yaml_step_lifecycle.go), using the same effective safe-outputs token resolution (configToken>safe-outputs.github-token>GH_AW_GITHUB_TOKEN/GITHUB_TOKEN) as the "Process Safe Outputs" handler manager step..lock.ymlfiles viamake recompileto propagate the change.go test ./pkg/workflow/...) and JS tests (collect_ndjson_output.test.cjs,resolve_mentions_from_payload.test.cjs) pass.make agent-report-progress(includes lint + test-unit) successfully.Note: this step runs in the main job (not the consolidated safe-outputs job), so it cannot reference the
safe-outputs-app-tokenstep output minted in the other job — only the configured custom token /GH_AW_GITHUB_TOKENfallback chain is wired here, matching the non-GitHub-App precedence used elsewhere.