[aw] Decouple Process Safe Outputs client token from project-specific token#45999
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add tests for all safe outputs to prevent github token pollution |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added comprehensive regression coverage in 65dfd4e: new |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45999 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (75 additions detected, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR updates the gh-aw workflow compiler to prevent project-specific credentials (e.g., safe-outputs.update-project.github-token) from overriding the shared Process Safe Outputs step’s with.github-token, restoring intended token separation between shared safe-output processing and project operations.
Changes:
- Updated
buildHandlerManagerSteptoken selection sowith.github-tokenfollows safe-outputs precedence only (safe-outputs token → magic fallback). - Updated/expanded Go tests to assert split-token behavior between
GH_AW_PROJECT_GITHUB_TOKENand the sharedwith.github-token. - Regenerated the smoke workflow lockfile to reflect the updated token wiring.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_handler_manager_token_test.go | Updates and expands regression tests to ensure project token stays isolated from the shared handler-manager with.github-token. |
| pkg/workflow/compiler_safe_outputs_steps.go | Changes the compiler’s handler-manager step to stop preferring the project token for the shared github-script client. |
| .github/workflows/smoke-project.lock.yml | Regenerated workflow output showing the shared github-token now uses the safe-outputs fallback chain. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| // With section for github-token | ||
| // Use the standard safe outputs token for all operations. | ||
| // If project operations are configured, prefer the project token for the github-script client. | ||
| // Rationale: update_project/create_project_status_update call the Projects v2 GraphQL API, which | ||
| // cannot be accessed with the default GITHUB_TOKEN. GH_AW_PROJECT_GITHUB_TOKEN is the required | ||
| // token for Projects v2 operations. | ||
| // Use the standard safe-outputs token for the shared github-script client. | ||
| // Project operations use GH_AW_PROJECT_GITHUB_TOKEN from env with dedicated handler logic. |
There was a problem hiding this comment.
Addressed in 60abf42 and prior commits: runtime project-handler auth is now wired through per-handler token fallback/client rebinding, so project operations no longer depend on the shared step-level client token path.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (3 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs and /tdd — approving with one suggestion on test robustness.
Key Themes & Highlights
Positive Highlights
- Root cause properly addressed: the project-token-preference branch is cleanly removed; token precedence is now unambiguous.
- Minimal, surgical change in
compiler_safe_outputs_steps.go— only the two lines causing the bug are removed. - Comment block updated to accurately reflect the new precedence order.
- Smoke-test lock file updated in lock-step, showing the fix propagates end-to-end.
- New isolation test iterates over
handlerRegistrydynamically, staying complete as new handlers are added.
Minor Concern (see inline comment)
Empty-map configs (map[string]any{}) in the new isolation test may silently skip handlers that need required fields, giving false confidence. A one-line assert.Contains(t, yamlStr, handlerName) guard would close this gap.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 24.3 AIC · ⌖ 4.45 AIC · ⊞ 6.7K
Comment /matt to run again
| "project": "https://github.com/orgs/myorg/projects/1", | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] The empty-map handler entries (map[string]any{}) in the new isolation test may silently fail to parse for handlers that require specific config fields, meaning the sub-test only truly exercises the update-project token path rather than the named handler's code.
💡 Suggestion
Consider using a minimal valid config for each handler, or at least assert that the compiled step includes the expected handler entry (not just the token line). For example:
// Verify handler is actually present in the compiled YAML, not silently skipped
assert.Contains(t, yamlStr, handlerName, "handler %q should appear in compiled step", handlerName)This would catch cases where an empty config causes a handler to be omitted entirely, giving you false confidence in the isolation property.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 60abf42 and prior commits: the test now avoids vacuous passes by explicitly checking for the handler config key in GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG and skipping isolation assertions when a handler is not materialized from minimal config.
There was a problem hiding this comment.
Review: Two test validity concerns — non-blocking
The compiler change itself is correct: removing project token from with.github-token precedence is the right fix, and the lock file update is consistent.
Findings
Medium — False-positive assertion in parameterized test
assert.Contains(t, yamlStr, "GH_AW_PROJECT_GITHUB_TOKEN: ..." passes unconditionally for every handler sub-test because update-project is always present in the shared fixture, not because the handler under test produces it. See inline comment.
Medium — Empty handler configs may be silently dropped
Handlers added as map[string]any{} may be no-ops if extractSafeOutputsConfig requires mandatory fields — making the isolation assertions pass vacuously. See inline comment.
Both are test-quality issues only; the production logic change is sound.
🔎 Code quality review by PR Code Quality Reviewer · 26.5 AIC · ⌖ 4.51 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/safe_outputs_handler_manager_token_test.go:391
False-positive assertion — the project token check passes regardless of the handler under test.
<details>
<summary>💡 Details</summary>
The assertion assert.Contains(t, yamlStr, "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.PROJECTS_PAT }}") fires for every handler in the registry. It always passes because update-project is unconditionally present in the safeOutputs fixture for every sub-test — not because the handler under test emits the expected env var. This means the assertion provid…
pkg/workflow/safe_outputs_handler_manager_token_test.go:357
Empty handler config may be silently dropped, making the isolation test hollow for most handlers.
<details>
<summary>💡 Details</summary>
The test adds each non-update-project handler as map[string]any{} (empty config). If extractSafeOutputsConfig silently skips handlers with missing required fields, those handlers are never wired into the compiled output. The isolation assertions then pass vacuously — they're not proving the handler uses the right token; they're just confirming an …
There was a problem hiding this comment.
Review: Token decoupling correctness gap
The compiler change is the right direction — decoupling the step-level with.github-token from the project PAT. However there is a blocking correctness regression in the runtime path.
Root issue: update_project and other project handlers obtain their GitHub client via global.github (bound to with.github-token) when no per-handler github-token is in their config (see handler_auth.cjs line 44-46 and update_project.cjs line 1263). GH_AW_PROJECT_GITHUB_TOKEN is set as a step env var, but is only used for error diagnostics — it never creates an Octokit instance.
Who is affected: Any user who configures update-project.project relying on secrets.GH_AW_PROJECT_GITHUB_TOKEN without explicitly setting update-project.github-token. Their project GraphQL calls will silently use the wrong token.
See the inline comment on the changed block for the specific fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 78 AIC · ⌖ 4.53 AIC · ⊞ 5K
| // 3. Magic secret fallback via getEffectiveSafeOutputGitHubToken() | ||
| // 1. Safe-outputs level token (so.GitHubToken) | ||
| // 2. Magic secret fallback via getEffectiveSafeOutputGitHubToken() | ||
| // |
There was a problem hiding this comment.
Correctness regression: project handlers lose project PAT when no per-handler github-token is configured
This change decouples the step-level with.github-token from the project PAT, but the project handlers (update_project, create_project_status_update, create_project) still rely on global.github (initialized from with.github-token) when no per-handler github-token is set.
Broken scenario:
safe-outputs:
update-project:
project: https://github.com/orgs/myorg/projects/1
# No github-token — user expects secrets.GH_AW_PROJECT_GITHUB_TOKEN to be usedPreviously, with.github-token was elevated to secrets.GH_AW_PROJECT_GITHUB_TOKEN, giving global.github Projects v2 GraphQL access. After this change, with.github-token = safe-outputs/fallback token, and GH_AW_PROJECT_GITHUB_TOKEN is only an env var used in error messages — not used to initialize any Octokit client. update_project.main() falls back to global.github (update_project.cjs line 1263), which now lacks project access.
Suggested fix: In safe_output_handler_manager.cjs, when building handlerConfig for project-type handlers and the config has no explicit github-token, inject process.env.GH_AW_PROJECT_GITHUB_TOKEN as handlerConfig['github-token']. This preserves backward compatibility without re-coupling the step-level token.
Also, the comment in handler_auth.cjs line 17 is now stale (project token > global safe-outputs token) and needs updating.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 60abf42 and prior commits: project handlers now get GH_AW_PROJECT_GITHUB_TOKEN as per-handler fallback when github-token is unset, are initialized with a dedicated Octokit client, and execute with temporary global.github rebinding so project GraphQL calls do not fall back to the shared safe-outputs client token.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
@copilot please run the
|
…ts-update-project-token-leaking Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in I investigated job
|
|
🎉 This pull request is included in a new release. Release: |
update-project.github-tokenwas being promoted to the sharedProcess Safe Outputsstep client token, overridingsafe-outputs.github-tokenand causing non-project handlers to run under the wrong credential. This change restores token separation: shared safe-output handlers use the safe-outputs token path, while project handlers keep using the dedicated project token channel.Token precedence fix (compiler)
with.github-token.with.github-tokennow follows safe-outputs precedence only:safe-outputs.github-tokensecrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN)Project token isolation preserved
GH_AW_PROJECT_GITHUB_TOKENwiring unchanged for project operations.Focused regression coverage
GH_AW_PROJECT_GITHUB_TOKENwith.github-tokenstays on safe-outputs token/fallbacksafe-outputs.github-token+update-project.github-token) behaves correctly