test: multi-checkout cross-org credential wiring integration tests#42937
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds new integration-test coverage in pkg/workflow/ for workflows that declare multiple checkout: entries across different organizations using heterogeneous authentication (github-token vs github-app). The goal is to ensure the compiler emits the expected checkout/app-token steps and correctly wires per-checkout token expressions into the checkout manifest.
Changes:
- Introduces integration tests for multi-checkout cross-org workflows using per-org PATs (
github-token). - Introduces integration tests for multi-checkout cross-org workflows using GitHub Apps (
github-app) and verifies minted-token wiring. - Adds a step-ordering integration test to ensure app-token mint steps precede the checkout steps that consume their outputs.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/multi_checkout_cross_org_integration_test.go |
New integration tests asserting compiler output for multi-checkout cross-org token/app wiring and step ordering. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Low
| // indexOf returns the byte offset of substr in s, or -1 if not found. | ||
| func indexOf(s, substr string) int { | ||
| if len(substr) > len(s) { | ||
| return -1 | ||
| } | ||
| for i := range len(s) - len(substr) + 1 { | ||
| if s[i:i+len(substr)] == substr { | ||
| return i | ||
| } | ||
| } | ||
| return -1 | ||
| } |
| // All cross-org checkout steps must disable credential persistence to prevent | ||
| // tokens leaking to subsequent steps that run without credentials by design. | ||
| assert.NotContains(t, compiled, "token: ${{ secrets.ORG_A_PAT }}\n persist-credentials: true", | ||
| "org-a checkout must not keep credentials") | ||
| } |
…ring strategy ADR-42937 documents the compiler decision to use index-based, heterogeneous credential wiring for multi-org checkouts: PATs inject directly, GitHub Apps emit a preceding checkout-app-token-N minting step, and a checkout manifest propagates token references to safe-outputs handlers.
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (399 new lines in pkg/workflow/) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed: docs/adr/42937-multi-checkout-cross-org-credential-wiring.md -- review and complete it before merging. This PR cannot merge until an ADR is linked in the PR body. 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.
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 93/100 — Excellent
📊 Metrics (4 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage gaps and a code quality issue.
📋 Key Themes & Highlights
Key Themes
-
Incomplete manifest assertions — All four tests assert
GH_AW_CHECKOUT_REPO_NandGH_AW_CHECKOUT_TOKEN_Nbut omitGH_AW_CHECKOUT_PATH_N, which is part of the full manifest schema. A regression dropping the path key would go undetected. -
Fragile
persist-credentialschecks — Where present, the negative assertions match a very specific multi-line string that is easy to silently bypass (e.g. key reordering, indentation change). Positive assertions in the style ofcheckout_persist_credentials_test.gowould be more robust. Two tests (TestMultiCheckoutCrossOrgGitHubApps,TestMultiCheckoutCrossOrgStepOrdering) have no persist-credentials check at all. -
Redundant
indexOfhelper — The customindexOffunction reimplementsstrings.Index, which is already in the standard library. It should be removed. -
Step-ordering test is incomplete — The ordering test checks that each mint step precedes its own checkout step, but does not verify that mint steps are emitted in declaration order relative to each other.
Positive Highlights
- ✅ Excellent scenario coverage: PAT-only, app-only, mixed, and ordering are all represented
- ✅ Clean test structure — each test has a focused manifest assertion block and a clear docstring
- ✅ Correct use of
require.NoErrorfor compile/file-read failures andassertfor non-fatal assertions - ✅ Step-ordering test uses byte-offset comparison, which is both precise and easy to understand
- ✅ PR description is detailed and includes a worked example — great for reviewers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 79.6 AIC · ⌖ 7.88 AIC · ⊞ 6.6K
Comment /matt to run again
| "Manifest must record org-b/repo-two as entry 1") | ||
| assert.Contains(t, compiled, "GH_AW_CHECKOUT_TOKEN_1: ${{ secrets.ORG_B_PAT }}", | ||
| "Manifest entry 1 must wire ORG_B_PAT") | ||
| assert.Contains(t, compiled, `GH_AW_CHECKOUT_REPO_2: "org-c/repo-three"`, |
There was a problem hiding this comment.
[/tdd] GH_AW_CHECKOUT_PATH_N entries are missing from the manifest assertions — safe-outputs handlers rely on these to resolve on-disk checkout paths, and a regression dropping them would go undetected here.
💡 Suggestion
Add GH_AW_CHECKOUT_PATH_N assertions alongside each REPO_N / TOKEN_N pair:
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_0: "repos/org-a"`,
"Manifest entry 0 must record the on-disk path for org-a")
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_1: "repos/org-b"`,
"Manifest entry 1 must record the on-disk path for org-b")
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_2: "repos/org-c"`,
"Manifest entry 2 must record the on-disk path for org-c")Existing tests in checkout_path_validation_test.go (line 232) show this key is emitted and expected. Omitting it here leaves a gap that a silent regression could exploit.
@copilot please address this.
| "org-b checkout must use ORG_B_PAT") | ||
| assert.Contains(t, compiled, "token: ${{ secrets.ORG_C_PAT }}", | ||
| "org-c checkout must use ORG_C_PAT") | ||
|
|
There was a problem hiding this comment.
[/tdd] The persist-credentials: false assertion uses a fragile NotContains pattern that only catches the exact string token: ... \n persist-credentials: true. A compiler change that reorders YAML keys or changes indentation would silently defeat this check.
💡 Stronger alternative
Use a positive assertion after extracting the per-checkout YAML block, matching the pattern already used in checkout_persist_credentials_test.go:
// Positive assertion: each cross-org checkout block must contain persist-credentials: false
assert.Contains(t, compiled, "persist-credentials: false",
"At least one checkout step must explicitly set persist-credentials: false")Or, for per-entry precision, extract the YAML block between the checkout step name and the next step and assert persist-credentials: false appears within it. This mirrors the approach in checkout_persist_credentials_test.go (line 218) which positively checks strings.Contains(checkoutContext, "persist-credentials: false").
@copilot please address this.
| } | ||
|
|
||
| // indexOf returns the byte offset of substr in s, or -1 if not found. | ||
| func indexOf(s, substr string) int { |
There was a problem hiding this comment.
[/codebase-design] The indexOf helper at line 389 reimplements strings.Index from the standard library — it is already imported and available in every Go file, making this dead weight that can confuse future readers.
💡 Suggestion
Delete the custom indexOf helper and use strings.Index directly in TestMultiCheckoutCrossOrgStepOrdering:
mintIdx0 := strings.Index(compiled, "id: checkout-app-token-0")
mintIdx1 := strings.Index(compiled, "id: checkout-app-token-1")
checkoutOrgA := strings.Index(compiled, "name: Checkout org-a/repo-one into repos/org-a")
checkoutOrgB := strings.Index(compiled, "name: Checkout org-b/repo-two into repos/org-b")Add "strings" to the import block. This removes ~15 lines of redundant code and aligns with every other test file in the package that already uses strings.Index.
@copilot please address this.
| require.NoError(t, os.WriteFile(mdPath, []byte(content), 0600)) | ||
|
|
||
| compiler := NewCompiler() | ||
| require.NoError(t, compiler.CompileWorkflow(mdPath), "Workflow with mixed github-token and github-app should compile") |
There was a problem hiding this comment.
[/tdd] TestMultiCheckoutCrossOrgMixedAuth only checks persist-credentials for the PAT checkout (org-a); the two github-app checkouts (org-b, org-c) have no equivalent assertion. An app-checkout regression that inadvertently keeps credentials would not be caught.
💡 Suggestion
Extend the persist-credentials checks to cover all three checkout entries:
// Also assert app-checkout entries disable credential persistence
assert.NotContains(t, compiled,
"token: ${{ steps.checkout-app-token-1.outputs.token }}\n persist-credentials: true",
"org-b app checkout must not keep credentials")
assert.NotContains(t, compiled,
"token: ${{ steps.checkout-app-token-2.outputs.token }}\n persist-credentials: true",
"org-c app checkout must not keep credentials")(Or adopt the stronger positive-assertion approach suggested in the other comment.)
@copilot please address this.
| mintIdx0 := indexOf(compiled, "id: checkout-app-token-0") | ||
| mintIdx1 := indexOf(compiled, "id: checkout-app-token-1") | ||
| checkoutOrgA := indexOf(compiled, "name: Checkout org-a/repo-one into repos/org-a") | ||
| checkoutOrgB := indexOf(compiled, "name: Checkout org-b/repo-two into repos/org-b") |
There was a problem hiding this comment.
[/tdd] TestMultiCheckoutCrossOrgStepOrdering verifies each minting step appears before its own checkout step, but does not assert that minting steps appear in declaration order relative to each other (checkout-app-token-0 before checkout-app-token-1). A compiler that emits them in reverse order would pass all current checks while producing incorrect output.
💡 Suggestion
Add a relative ordering assertion between the two minting steps:
assert.Less(t, mintIdx0, mintIdx1,
"checkout-app-token-0 minting step must appear before checkout-app-token-1")This closes the gap: the full invariant is mint-0 < mint-1 < checkout-0 < checkout-1 (or a similar valid ordering); right now only the per-pair constraints are checked.
@copilot please address this.
| require.NoError(t, os.WriteFile(mdPath, []byte(content), 0600)) | ||
|
|
||
| compiler := NewCompiler() | ||
| require.NoError(t, compiler.CompileWorkflow(mdPath), "Workflow with multiple cross-org github-apps should compile") |
There was a problem hiding this comment.
[/tdd] The two github-app checkout tests (TestMultiCheckoutCrossOrgGitHubApps and TestMultiCheckoutCrossOrgStepOrdering) do not assert persist-credentials: false at all — neither positively nor negatively — for any checkout step.
💡 Suggestion
Add at least one positive persist-credentials assertion per test to prevent a regression where app-token-minted checkouts accidentally retain credentials:
// Both app-token checkouts must disable credential persistence
assert.Contains(t, compiled, "persist-credentials: false",
"Cross-org app checkout steps must set persist-credentials: false")This is the same security invariant already tested by TestCheckoutPersistCredentials and TestMultiCheckoutCrossOrgGitHubTokens; it should be consistent across all checkout test scenarios.
@copilot please address this.
There was a problem hiding this comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 136 AIC · ⌖ 7.36 AIC · ⊞ 1.6K
Comment /review to run again
| "org-c checkout must not keep credentials") | ||
|
|
||
| // The checkout manifest must record all three repos so safe-outputs handlers can | ||
| // resolve the correct on-disk path and token for each org without network calls. |
There was a problem hiding this comment.
Missing GH_AW_CHECKOUT_PATH_N assertions: the manifest section checks GH_AW_CHECKOUT_REPO_N and GH_AW_CHECKOUT_TOKEN_N but never GH_AW_CHECKOUT_PATH_N — yet the comment two lines above explicitly says handlers need path to "resolve the correct on-disk path". The compiler emits GH_AW_CHECKOUT_PATH_N for every entry (see checkout_step_generator.go), and other existing tests (checkout_manager_test.go, checkout_path_validation_test.go) already verify it in simpler cases. If path wiring were silently broken in the multi-checkout/cross-org code path, all four new tests would still pass.
💡 Suggested fix
Add path assertions alongside the existing REPO and TOKEN assertions in every test, e.g. in TestMultiCheckoutCrossOrgGitHubTokens:
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_0: "repos/org-a"`,
"Manifest entry 0 must record the checkout path")
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_1: "repos/org-b"`,
"Manifest entry 1 must record the checkout path")
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_2: "repos/org-c"`,
"Manifest entry 2 must record the checkout path")Apply the same pattern in TestMultiCheckoutCrossOrgGitHubApps and TestMultiCheckoutCrossOrgMixedAuth.
|
|
||
| // All cross-repo checkouts must disable credential persistence. | ||
| // There must be no persist-credentials: true for any of the cross-org checkouts | ||
| // (safe-outputs job is the only place where persist-credentials: true is allowed). |
There was a problem hiding this comment.
Fragile persist-credentials negative assertion: the NotContains checks for the exact string "token: ${{ secrets.ORG_A_PAT }}\n persist-credentials: true" (10-space indent, immediately adjacent). If the YAML renderer changes indentation depth, adds an intervening field, or wraps differently, persist-credentials: true can be present in the compiled output and these assertions still pass — providing false confidence about credential hygiene.
💡 Suggested fix
A simpler and more robust negative check is to assert persist-credentials: true does not appear anywhere in the output (the PR description says the safe-outputs job is the only place it's allowed, and that job lives in a separate workflow file):
assert.NotContains(t, compiled, "persist-credentials: true",
"cross-org checkout steps must never persist credentials")If a future workflow legitimately needs persist-credentials: true in a different job, move the check to scope it to the checkout step block (e.g. extract the checkout step YAML substring and assert on that slice instead).
| require.Greater(t, checkoutOrgB, -1, "Checkout step for org-b must be present") | ||
|
|
||
| assert.Less(t, mintIdx0, checkoutOrgA, | ||
| "checkout-app-token-0 minting step must appear before the org-a checkout step") |
There was a problem hiding this comment.
indexOf duplicates strings.Index: the custom helper has identical semantics (returns byte offset or -1) to the stdlib function and is unlikely to be maintained in sync with it if edge-case behaviour ever changes.
💡 Suggested fix
Remove indexOf, add "strings" to the import block, and replace calls in TestMultiCheckoutCrossOrgStepOrdering directly:
mintIdx0 := strings.Index(compiled, "id: checkout-app-token-0")
mintIdx1 := strings.Index(compiled, "id: checkout-app-token-1")
checkoutOrgA := strings.Index(compiled, "name: Checkout org-a/repo-one into repos/org-a")
checkoutOrgB := strings.Index(compiled, "name: Checkout org-b/repo-two into repos/org-b")strings.Index returns -1 when not found, so the existing require.Greater(t, ..., -1) guards work unchanged.
| // The github-token checkout (index 0) must NOT produce an app-token minting step; | ||
| // app-token steps are only emitted for github-app entries. | ||
| assert.NotContains(t, compiled, "id: checkout-app-token-0", | ||
| "No app token minting step for PAT-authenticated checkout at index 0") |
There was a problem hiding this comment.
No step-ordering assertion in TestMultiCheckoutCrossOrgMixedAuth: this is the most complex scenario (PAT + app + app) and the PR description calls step ordering out as a distinct correctness requirement. The dedicated TestMultiCheckoutCrossOrgStepOrdering only covers the pure-app case; if a bug in the mixed-auth code path emitted checkout-app-token-1 after its checkout step, nothing here would catch it.
💡 Suggested fix
Add ordering assertions at the end of the test using strings.Index (or the indexOf helper that already exists in the file):
mintIdx1 := strings.Index(compiled, "id: checkout-app-token-1")
mintIdx2 := strings.Index(compiled, "id: checkout-app-token-2")
checkoutB := strings.Index(compiled, "name: Checkout org-b/repo-two into repos/org-b")
checkoutC := strings.Index(compiled, "name: Checkout org-c/repo-three into repos/org-c")
require.Greater(t, mintIdx1, -1, "checkout-app-token-1 must be present")
require.Greater(t, mintIdx2, -1, "checkout-app-token-2 must be present")
assert.Less(t, mintIdx1, checkoutB, "checkout-app-token-1 must precede org-b checkout")
assert.Less(t, mintIdx2, checkoutC, "checkout-app-token-2 must precede org-c checkout")
There was a problem hiding this comment.
Review: multi-checkout cross-org credential wiring integration tests
Good coverage direction — the four test scenarios (PAT-only, App-only, mixed, and step ordering) address real compiler contracts that had no integration-level tests. There are two blocking issues and one reliability concern to fix before merge.
🔴 Blocking: duplicate indexOf helper (compile error)
indexOf is already defined in action_cache_test.go (build tag //go:build !integration). Both files live in package workflow, so without the integration tag the compiler sees a duplicate symbol and fails. Use strings.Index directly at call sites and remove the helper. See inline comment at line 399.
🔴 Blocking: GH_AW_CHECKOUT_PATH_N manifest entries not asserted
The compiler emits GH_AW_CHECKOUT_PATH_N alongside every GH_AW_CHECKOUT_REPO_N / GH_AW_CHECKOUT_TOKEN_N. Safe-outputs handlers rely on these paths to locate the on-disk repo. All four tests omit path assertions — a silent regression in path wiring would go undetected. See inline comment at line 101.
🟡 Reliability: persist-credentials checks are whitespace-coupled and one-sided
The NotContains patterns embed a specific indentation string; an indentation change would silently skip the check. Also, no test asserts the positive form (persist-credentials: false is actually emitted), which is the security property. See inline comment at line 77.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 86.7 AIC · ⌖ 6.94 AIC · ⊞ 4.9K
| } | ||
| } | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
Duplicate indexOf helper — will cause a compile error when building without the integration tag.
An identical indexOf(s, substr string) int already exists in action_cache_test.go (build constraint //go:build !integration) at line 198. Both files are in package workflow, so when the integration tag is absent the compiler sees two definitions and fails.
Additionally, the helper is just a reimplementation of strings.Index; replace the body with:
func indexOf(s, substr string) int {
return strings.Index(s, substr)
}Or call strings.Index directly at each call site and remove this helper entirely.
@copilot please address this.
| "Manifest must record org-a/repo-one as entry 0") | ||
| assert.Contains(t, compiled, "GH_AW_CHECKOUT_TOKEN_0: ${{ secrets.ORG_A_PAT }}", | ||
| "Manifest entry 0 must wire ORG_A_PAT") | ||
| assert.Contains(t, compiled, `GH_AW_CHECKOUT_REPO_1: "org-b/repo-two"`, |
There was a problem hiding this comment.
Missing GH_AW_CHECKOUT_PATH_N assertions in the manifest checks.
The compiler emits GH_AW_CHECKOUT_PATH_0, GH_AW_CHECKOUT_PATH_1, etc. alongside each GH_AW_CHECKOUT_REPO_N and GH_AW_CHECKOUT_TOKEN_N in the manifest step (see checkout_step_generator.go lines 139–148). The new tests assert GH_AW_CHECKOUT_REPO_N and GH_AW_CHECKOUT_TOKEN_N but never verify that the path env-vars are emitted with the correct values.
Without these assertions a future refactor that silently drops GH_AW_CHECKOUT_PATH_N (which safe-outputs handlers rely on to locate the on-disk repo) would pass undetected. Add assertions such as:
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_0: "repos/org-a"`,
"Manifest entry 0 must record the checkout path")
assert.Contains(t, compiled, `GH_AW_CHECKOUT_PATH_1: "repos/org-b"`,
"Manifest entry 1 must record the checkout path")@copilot please address this.
|
|
||
| // Each checkout must inject its org-specific PAT via token:. | ||
| assert.Contains(t, compiled, "token: ${{ secrets.ORG_A_PAT }}", | ||
| "org-a checkout must use ORG_A_PAT") |
There was a problem hiding this comment.
Fragile whitespace-sensitive NotContains checks for persist-credentials.
assert.NotContains(t, compiled, "token: ${{ secrets.ORG_A_PAT }}\n persist-credentials: true", ...)This pattern embeds a specific indentation level ( ) and a newline. If the YAML emitter ever changes indentation (e.g. due to a refactor), the negative assertion silently passes even if the bad property is present.
Additionally, the current checks only assert that persist-credentials: true does not immediately follow the token line — they do not confirm that persist-credentials: false is emitted, which is the security-critical property.
Replace with two-sided assertions per checkout:
assert.Contains(t, compiled, "persist-credentials: false",
"All cross-org checkouts must disable credential persistence")
assert.NotContains(t, compiled, "persist-credentials: true",
"No checkout must retain credentials")@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher pass and pushed fixes in a8e069f. I addressed the open test-feedback items (removed duplicate index helper, hardened persist-credentials assertions, added GH_AW_CHECKOUT_PATH_N manifest assertions, and expanded step-ordering checks), and re-ran local validation (make fmt, make lint, make test-unit, make test, make recompile, make agent-report-progress). |
🤖 PR Triage
Score breakdown: Impact 28 · Urgency 15 · Quality 17 CI passing (19 success, 10 skipped). Adds 4 targeted integration tests for multi-checkout cross-org credential wiring with ADR. Review shows mixed bot signals — fast-track for human review.
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/28592810254
|
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>
Final pr-finisher pass complete. I reverted an unintended workflow lockfile regeneration in Current readiness:
ADR note: the PR body still does not include the ADR link line; I could verify this but could not patch the PR body from this environment due API permission limits. Please add: Hand-off: a maintainer should add the ADR link in the PR body (if still required) and re-trigger CI (close/reopen, workflow_dispatch, or maintainer push). |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
No integration tests existed for workflows declaring multiple checkouts across different organizations with heterogeneous credential types (
github-tokenvsgithub-app). This adds coverage that the compiler emits all required steps and wires tokens correctly through to the checkout manifest.New tests (
pkg/workflow/multi_checkout_cross_org_integration_test.go)TestMultiCheckoutCrossOrgGitHubTokens— three checkouts, each with a distinct PAT; asserts eachactions/checkoutstep injects its per-orgtoken:and the manifestGH_AW_CHECKOUT_TOKEN_Nenv-vars carry the correct secret references. No app-token minting steps should appear.TestMultiCheckoutCrossOrgGitHubApps— two checkouts with distinctgithub-appconfigs; asserts onecheckout-app-token-Nminting step per entry with the correctclient-id,private-key, and org-derivedowner:, and that each checkout'stoken:references${{ steps.checkout-app-token-N.outputs.token }}.TestMultiCheckoutCrossOrgMixedAuth— one PAT + two GitHub Apps; asserts no minting step is emitted for the PAT entry (checkout-app-token-0absent), app entries get minting steps at the correct indices, and all three manifest entries carry the right token expressions.TestMultiCheckoutCrossOrgStepOrdering— two GitHub App checkouts; asserts eachcheckout-app-token-Nminting step appears before its correspondingactions/checkoutstep so the step-output reference resolves at runtime.