Skip to content

Derive omitted GitHub App owners from effective checkout target#37976

Merged
pelikhan merged 8 commits into
mainfrom
copilot/multi-org-github-app-support
Jun 9, 2026
Merged

Derive omitted GitHub App owners from effective checkout target#37976
pelikhan merged 8 commits into
mainfrom
copilot/multi-org-github-app-support

Conversation

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

When a workflow targets repositories across multiple orgs, github-app.owner could only be a static literal, which made app-token minting fail for dispatch-time owner/repo targets. This change teaches gh-aw to derive the installation owner from the workflow’s effective checkout target when owner: is omitted, so multi-org workflows no longer need a redundant trigger_org input.

  • What changed

    • Added owner derivation for GitHub App token minting when github-app.owner is omitted.
    • The derived owner comes from the single effective checkout repository for the workflow.
    • For workflow_call workflows with no explicit checkout.repository, owner derivation now falls back to ${{ needs.activation.outputs.target_repo }}.
    • Added security-focused logging for owner-source inference and runtime owner derivation to improve auditability and reduce confusion in misconfiguration scenarios.
  • Where derivation now applies

    • checkout[*].github-app
    • top-level safe-outputs.github-app (including conclusion/notify-comment token minting)
    • tools.github.github-app
  • Derivation behavior

    • For literal owner/repo values, gh-aw emits the owner directly at compile time.
    • For expression-based repositories, gh-aw emits a generated pre-step that extracts the owner at runtime and feeds it into actions/create-github-app-token.
    • Explicit github-app.owner still takes precedence unchanged.
    • Repository expressions are whitespace-normalized before being embedded in generated YAML env blocks.
  • Implementation notes

    • Refactored GitHub App token step generation to accept an optional repository source for owner inference.
    • Updated checkout and GitHub MCP token generation to use stable step IDs directly instead of post-processing generic safe-output step metadata.
    • Updated notify-comment conclusion job token minting to use repository-aware owner inference.
    • Added compile-time logs for inference decisions (single repo, ambiguous repos, workflow_call fallback, default fallback).
    • Added runtime logs in generated owner-derivation steps (start, normalized candidate, derived owner, and explicit failure path).
    • Left activation / pre-activation token owner behavior unchanged.
  • Tests

    • Added coverage for:
      • empty owner-source fallback to ${{ github.repository_owner }}
      • ambiguous multi-repo checkout behavior
      • repeated same-repo checkout behavior
      • workflow_call fallback to ${{ needs.activation.outputs.target_repo }}
      • owner-derivation step ordering before token minting
      • bare repository literal handling
      • .wiki suffix handling in runtime derivation
      • whitespace normalization in generated repository expressions
  • Example

    checkout:
      repository: ${{ github.event.inputs.trigger_ref }}
      github-app:
        client-id: ${{ secrets.MY_APP_ID }}
        private-key: ${{ secrets.MY_APP_KEY }}
        repositories: ["*"]
    
    tools:
      github:
        github-app:
          client-id: ${{ secrets.MY_APP_ID }}
          private-key: ${{ secrets.MY_APP_KEY }}
          repositories: ["*"]
    
    safe-outputs:
      github-app:
        client-id: ${{ secrets.MY_APP_ID }}
        private-key: ${{ secrets.MY_APP_KEY }}
        repositories: ["*"]

    This compiles to generated token-minting steps that derive owner from the effective target repository rather than defaulting to github.repository_owner.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Add support for multi-org GitHub App to derive owner Derive omitted GitHub App owners from checkout.repository Jun 9, 2026
Copilot AI requested a review from pelikhan June 9, 2026 00:24
@pelikhan pelikhan marked this pull request as ready for review June 9, 2026 01:04
Copilot AI review requested due to automatic review settings June 9, 2026 01:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends gh-aw’s GitHub App token minting so that when github-app.owner is omitted, the installation owner can be derived from the workflow’s effective checkout.repository (including expression-based repositories via a generated runtime pre-step). This targets multi-org workflows where the target repo is only known at dispatch time.

Changes:

  • Added owner-derivation logic (compile-time for literal owner/repo, runtime pre-step for expression-based repositories).
  • Refactored GitHub App token step generation to accept an optional repository source for owner inference, and updated safe-outputs + GitHub MCP token minting to use stable step IDs directly.
  • Added tests covering owner inference and ensuring key workflow builders emit the derivation pre-step.
Show a summary per file
File Description
pkg/workflow/safe_outputs_jobs.go Routes safe-output job GitHub App token minting through the repository-aware builder for owner inference.
pkg/workflow/safe_outputs_app_config.go Refactors token mint step generation to resolve/derive owner (optionally via new pre-step).
pkg/workflow/github_app_owner_derivation.go Introduces repository-to-owner derivation helpers and runtime owner extraction step generation.
pkg/workflow/github_app_owner_derivation_test.go Adds unit tests for owner derivation and integration-style assertions for generated steps.
pkg/workflow/compiler_yaml_step_generation.go Updates OTLP token mint step helper call signature.
pkg/workflow/compiler_safe_outputs_job.go Uses repository-aware GitHub App token minting for consolidated safe-outputs job.
pkg/workflow/compiler_github_mcp_steps.go Switches to explicit step IDs (no post-rewrite) and passes repository source for owner inference.
pkg/workflow/checkout_step_generator.go Switches checkout app-token generation to use explicit step IDs and pass repository source for owner inference.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 8/8 changed files
  • Comments generated: 2

Comment on lines +224 to +225
owner, ownerSteps := resolveGitHubAppOwner(app, ownerSourceRepository, stepName, stepID)
steps = append(steps, ownerSteps...)
Comment on lines +12 to +33
func inferSingleCheckoutRepositoryForGitHubAppOwner(data *WorkflowData) string {
if data == nil {
return ""
}

checkoutMgr := NewCheckoutManager(data.CheckoutConfigs)
var repository string
for _, entry := range checkoutMgr.ordered {
if entry.key.repository == "" {
continue
}
if repository == "" {
repository = entry.key.repository
continue
}
if entry.key.repository != repository {
return ""
}
}

return repository
}
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (269 new lines under pkg/) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/37976-derive-github-app-owner-from-checkout-repository.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next
  1. Review the draft ADR committed to your branch — it was generated from the PR diff.
  2. Complete the missing sections — confirm the context, refine the decision rationale, and validate the alternatives you actually considered.
  3. Commit the finalized ADR to docs/adr/ on your branch.
  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-37976: Derive Omitted GitHub App Owner from checkout.repository

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

ADRs create a searchable, permanent record of why the codebase looks the way it does. This change introduces a non-obvious owner-derivation rule (single distinct checkout repo, compile-time literal vs. runtime pre-step, explicit-owner precedence) that future contributors will need the rationale for.

📋 Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

ADRs are stored in docs/adr/ as Markdown files numbered by PR number.

🔒 This gate is blocking until an ADR is linked in the PR body.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · 88.8 AIC · ⌖ 9.78 AIC ·

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 85/100 — Excellent

Analyzed 2 test function(s) across 1 new file: 2 design tests, 0 implementation tests, 2 coding-guideline violations (missing build tag + missing assertion messages).

📊 Metrics & Test Classification (2 tests analyzed)
Metric Value
New/modified tests analyzed 2
✅ Design tests (behavioral contracts) 2 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 1 (50%)
Duplicate test clusters 0
Test inflation detected No (test:prod ratio = 1.52:1)
🚨 Coding-guideline violations 2 (missing build tag; missing assertion messages)

Test Classification Details

Test File Classification Issues Detected
TestBuildGitHubAppTokenMintStepOwnerDerivation (3 table rows) pkg/workflow/github_app_owner_derivation_test.go:12 ✅ Design No assertion messages; no explicit error cases
TestWorkflowBuildersDeriveGitHubAppOwnerFromCheckoutRepository pkg/workflow/github_app_owner_derivation_test.go:79 ✅ Design No assertion messages; happy-path only

Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 2 tests — unit/integration classification unclear: ⚠️ missing //go:build constraint
🚨 Flagged Items — Requires Review (2 issue(s))

🚨 pkg/workflow/github_app_owner_derivation_test.go — Missing Build Tag (Hard Violation)

Issue: This newly added file begins with package workflow instead of a //go:build constraint. Every other *_test.go file in pkg/workflow/ starts with either //go:build !integration or //go:build integration on line 1. Without it, the CI system cannot correctly classify this file as a unit or integration test.

Suggested fix — add the appropriate tag as line 1:

(go/redacted):build !integration

package workflow

(Use //go:build integration instead if this test exercises real external dependencies.)


⚠️ All assertions in both test functions — Missing Assertion Messages

Issue: All assert.* and require.* calls in both test functions omit the optional descriptive message argument. The project guideline requires a message on every assertion so failures are self-explanatory in CI output.

Affected examples:

  • assert.Contains(t, stepsStr, tt.expectedOwner) — no message makes it hard to diagnose when the assertion fails in a subtest
  • assert.Contains(t, checkoutSteps, "id: checkout-app-token-0-owner") — no context about which subsystem or scenario

Suggested fix (apply to all assert.*/require.* calls):

// table-driven test — include case name for context
assert.Contains(t, stepsStr, tt.expectedOwner, "case %q: steps should contain expected owner field", tt.name)
assert.Contains(t, stepsStr, tt.expectedContains, "case %q: steps should contain expected helper step", tt.name)
assert.NotContains(t, stepsStr, tt.unexpectedContains, "case %q: steps should not contain unexpected helper step", tt.name)

// integration test — describe what is being checked
assert.Contains(t, checkoutSteps, "id: checkout-app-token-0-owner", "checkout steps should include owner-derivation helper step")
require.NoError(t, err, "buildConsolidatedSafeOutputsJob should not fail with valid workflow data")
assert.Contains(t, safeOutputsSteps, "owner: ${{ steps.safe-outputs-app-token-owner.outputs.owner }}", "safe-outputs steps should reference owner from helper step")

Verdict

Check failed. Coding-guideline violation: pkg/workflow/github_app_owner_derivation_test.go is missing the mandatory //go:build !integration or //go:build integration build tag on line 1 (required by every Go test file in this repository). Additionally, all assert.*/require.* calls in both test functions lack descriptive message arguments. Both issues must be resolved before merging.

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §27177149718

🧪 Test quality analysis by Test Quality Sentinel · 238.4 AIC · ⌖ 29.5 AIC ·

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Test Quality Sentinel: 85/100. Coding-guideline violation detected. The new file pkg/workflow/github_app_owner_derivation_test.go is missing the mandatory //go:build !integration or //go:build integration build tag on line 1 (every other *_test.go in pkg/workflow/ carries this tag). Additionally, all assert.*/require.* calls in both test functions lack the required descriptive message argument. Please review the flagged items in the comment above.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete migration of buildGitHubAppTokenMintStep callers

notify_comment.go:64 still calls buildGitHubAppTokenMintStep (which hard-codes ownerSourceRepository = ""), silently bypassing the cross-org owner derivation this PR introduces. Both safe_outputs_jobs.go and compiler_safe_outputs_job.go — which mint the same data.SafeOutputs.GitHubApp token — were updated; this one was not. For any multi-org workflow that uses the notify-comment feature, the minted token will target the wrong GitHub App installation. See inline comment on safe_outputs_app_config.go:213.

Other findings

Missing test coverage for the conflicting-checkout fallback

inferSingleCheckoutRepositoryForGitHubAppOwner returns "" for both no explicit checkouts and conflicting repos. Only the single-checkout path is tested. The silent github.repository_owner fallback on a multi-repo conflict has no test, making it easy to regress. See inline comment on github_app_owner_derivation_test.go:138.

repositoryExpr not whitespace-normalised before YAML embedding

The codebase applies githubExpressionWhitespaceReplacer before embedding expressions into generated YAML conditions; the new GH_AW_TARGET_REPOSITORY: %s line skips this. Low probability but inconsistent — see inline comment on github_app_owner_derivation.go:53.

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.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

🔎 Code quality review by PR Code Quality Reviewer · ⌖ 14.1 AIC

// owner/repo slug — actions/create-github-app-token expects repo names only when owner is also set).
func (c *Compiler) buildGitHubAppTokenMintStep(app *GitHubAppConfig, permissions *Permissions, fallbackRepoExpr string) []string {
return c.buildGitHubAppTokenMintStepWithMeta(app, permissions, fallbackRepoExpr, "Generate GitHub App token", "safe-outputs-app-token")
return c.buildGitHubAppTokenMintStepWithMeta(app, permissions, fallbackRepoExpr, "", "Generate GitHub App token", "safe-outputs-app-token")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notify_comment.go not updated: buildGitHubAppTokenMintStep (which passes ownerSourceRepository = "") still has three callers in unchanged files; notify_comment.go:64 appears to be an unintentional omission.

💡 Details

Both safe_outputs_jobs.go and compiler_safe_outputs_job.go were updated to call buildGitHubAppTokenMintStepForRepository with the inferred checkout repository, enabling cross-org owner derivation. However, notify_comment.go line 64 mints the same data.SafeOutputs.GitHubApp token through the old path:

// notify_comment.go:64 — still uses empty ownerSourceRepository
steps = append(steps, c.buildGitHubAppTokenMintStep(data.SafeOutputs.GitHubApp, permissions, appTokenFallbackRepo)...)

For multi-org workflows where checkout.repository belongs to a different org, the notify-comment job will mint a token scoped to ${{ github.repository_owner }} (the workflow repo org) instead of the checkout repo org. The token will be valid but targeted at the wrong installation, causing a 404 from actions/create-github-app-token or silently using a fallback installation.

Suggested fix: mirror the same change made to safe_outputs_jobs.go:

steps = append(steps, c.buildGitHubAppTokenMintStepForRepository(
    data.SafeOutputs.GitHubApp,
    permissions,
    appTokenFallbackRepo,
    inferSingleCheckoutRepositoryForGitHubAppOwner(data),
)...)

The two other unchanged callers (create_code_scanning_alert.go with the default checkout app and compiler_safe_outputs_steps.go with CreateCheckRun.GitHubApp) are likely intentional exclusions, but notify_comment.go is not.

assert.Contains(t, safeOutputsSteps, "id: safe-outputs-app-token-owner")
assert.Contains(t, safeOutputsSteps, "owner: ${{ steps.safe-outputs-app-token-owner.outputs.owner }}")
assert.Contains(t, safeOutputsSteps, "GH_AW_TARGET_REPOSITORY: ${{ github.event.inputs.trigger_ref }}")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test for conflicting-checkout fallback: inferSingleCheckoutRepositoryForGitHubAppOwner returns "" when multiple checkouts target different repositories, silently falling back to ${{ github.repository_owner }} — but this critical path has no test coverage.

💡 Suggested test case

Add to TestWorkflowBuildersDeriveGitHubAppOwnerFromCheckoutRepository (or a dedicated unit test for inferSingleCheckoutRepository...):

// conflicting repos → fall back to github.repository_owner
conflictData := &WorkflowData{
    CheckoutConfigs: []*CheckoutConfig{
        {Repository: "org-a/repo-1", GitHubApp: &GitHubAppConfig{...}},
        {Repository: "org-b/repo-2", GitHubApp: &GitHubAppConfig{...}},
    },
    ...
}
assert.Equal(t, "", inferSingleCheckoutRepositoryForGitHubAppOwner(conflictData))
// and verify the compiled step uses ${{ github.repository_owner }} as owner

Also worth adding:

  • Same repo repeated: two checkouts with identical repository values should still derive the literal owner.
  • No explicit checkouts (all repository == ""): should return "" → fallback to github.repository_owner.

Without these cases, a future refactor of inferSingleCheckoutRepositoryForGitHubAppOwner could accidentally change the fallback behaviour (e.g. returning the first repo instead of "") without any test failing.

steps = append(steps, fmt.Sprintf(" - name: %s\n", ownerStepName))
steps = append(steps, fmt.Sprintf(" id: %s\n", ownerStepID))
steps = append(steps, " env:\n")
steps = append(steps, fmt.Sprintf(" GH_AW_TARGET_REPOSITORY: %s\n", repositoryExpr))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unescaped repositoryExpr injected into YAML env block: repositoryExpr is embedded into the generated YAML without whitespace normalisation, inconsistent with how githubExpressionWhitespaceReplacer is used elsewhere in this package for the same pattern.

💡 Details

safe_outputs_app_config.go already defines githubExpressionWhitespaceReplacer to strip \r\n, \n, \r, \t from expressions before they are embedded in YAML conditions (see buildIgnoreIfMissingCondition). The same normalisation is not applied here:

// current — raw expression injected verbatim
steps = append(steps, fmt.Sprintf("          GH_AW_TARGET_REPOSITORY: %s\n", repositoryExpr))

A repositoryExpr value containing a newline (e.g. from a multi-line GitHub expression, or a workflow author accidentally adding one) would break the YAML block structure by injecting additional keys into the env: map.

Suggested fix — normalise before embedding:

cleanExpr := githubExpressionWhitespaceReplacer.Replace(repositoryExpr)
steps = append(steps, fmt.Sprintf("          GH_AW_TARGET_REPOSITORY: %s\n", cleanExpr))

This is a low-probability event in practice (YAML scalars in source workflows cannot contain literal newlines), but the inconsistency with the established codebase pattern is worth fixing.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /tdd, /zoom-out, /grill-with-docs, and /improve-codebase-architecture — requesting changes primarily on a correctness concern in the multi-repo ambiguity path and test coverage gaps.

📋 Key Themes & Highlights

Blocking

  • Silent wrong-owner fallback on multi-repo ambiguity (github_app_owner_derivation.go:28): when multiple distinct checkout repositories are configured, inferSingleCheckoutRepositoryForGitHubAppOwner returns "" and the caller silently mints a token for github.repository_owner — potentially the wrong org for cross-org workflows.

Test Coverage Gaps (/tdd)

  • No test for ownerSourceRepo: "" (empty → github.repository_owner fallback)
  • No test for multi-distinct-repos case (boundary condition of inferSingleCheckoutRepositoryForGitHubAppOwner)
  • No test for bare repo name without org prefix (e.g., "just-a-repo")
  • .wiki suffix stripping in the bash pre-step is untested
  • buildSafeOutputsJobFromParts path not exercised (only buildConsolidatedSafeOutputsJob is tested)
  • Step ordering (owner-derivation step before mint step) not validated by any assertion

Architecture / Style (/improve-codebase-architecture, /grill-with-docs)

  • buildGitHubAppTokenMintStepForRepository adds a third wrapper level without meaningful abstraction
  • Owner-step name is derived via a fragile hard-coded string replacement

Positive Highlights

  • ✅ Clean refactor replacing string post-processing (strings.ReplaceAll on YAML output) with direct parameter passing
  • ✅ Good env-variable isolation in the bash pre-step (GH_AW_TARGET_REPOSITORY) mitigates injection risk
  • ✅ Explicit owner still takes precedence — backward-compatible
  • ✅ Three derivation paths (literal, expression, absent) are well-structured and independently documented

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 555.5 AIC · ⌖ 26 AIC

continue
}
if entry.key.repository != repository {
return ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] Silent fallback on multi-repo ambiguity may produce the wrong token owner — the multi-checkout cross-org case this PR enables.

💡 Analysis

inferSingleCheckoutRepositoryForGitHubAppOwner returns "" for two distinct situations:

  1. No explicit checkout repository configured → fallback to ${{ github.repository_owner }} is correct.
  2. Multiple distinct checkout repos configured → also returns "", silently using the caller's repo owner.

Case 2 is the crux: checkout: [{repository: org-a/repo}, {repository: org-b/repo}] mints a token for the calling repo's org instead of either target. Consider returning a distinct sentinel or emitting a compile-time diagnostic so this ambiguity is surfaced rather than silently swallowed.

}

ownerStepID := stepID + "-owner"
ownerStepName := strings.Replace(stepName, "Generate GitHub App token", "Derive GitHub App owner", 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/grill-with-docs] The owner-step name is derived by replacing a hard-coded magic string, creating a hidden coupling to the calling step's name format.

💡 Suggestion
ownerStepName := strings.Replace(stepName, "Generate GitHub App token", "Derive GitHub App owner", 1)
if ownerStepName == stepName {
    ownerStepName = "Derive GitHub App owner"
}

This silently falls back to a generic name whenever the parent step name doesn't contain the magic string — making it fragile to caller-side renames. Consider accepting a ownerStepName parameter directly (parallel to ownerStepID), or defining the name as a constant so the dependency is explicit.

expectedOwner: "owner: ${{ steps.safe-outputs-app-token-owner.outputs.owner }}",
expectedContains: "id: safe-outputs-app-token-owner",
},
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing test case for ownerSourceRepo: "" — no test verifies that a blank owner-source falls back to ${{ github.repository_owner }} and emits no pre-step.

💡 Suggested addition to the table
{
    name: "no owner source falls back to github.repository_owner",
    app: &GitHubAppConfig{
        AppID:      "${{ vars.APP_ID }}",
        PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
    },
    ownerSourceRepo:    "",
    expectedOwner:      "owner: ${{ github.repository_owner }}",
    unexpectedContains: "-owner",
},

This baseline case documents the default behavior and catches any regression where an empty ownerSourceRepository accidentally triggers a pre-step.

"Generate GitHub App token",
"safe-outputs-app-token",
)
stepsStr := strings.Join(steps, "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Tests join all steps into a flat string and assert substring presence, so they cannot detect wrong step ordering. The owner-derivation step must appear before the token-mint step — a regression swapping the order would go undetected.

💡 Suggestion

Assert on the slice directly before joining, or use strings.Index to compare positions:

stepsStr := strings.Join(steps, "")
// Verify order: owner derivation step precedes the mint step
ownerIdx := strings.Index(stepsStr, "id: safe-outputs-app-token-owner")
mintIdx  := strings.Index(stepsStr, "id: safe-outputs-app-token\n")
require.Greater(t, mintIdx, ownerIdx, "owner derivation step must precede token mint step")

}
}

func TestWorkflowBuildersDeriveGitHubAppOwnerFromCheckoutRepository(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] TestWorkflowBuildersDeriveGitHubAppOwnerFromCheckoutRepository combines three independent concerns (checkout, MCP, safe-outputs) into one test function. A failure in any one of them obscures where the problem is.

💡 Suggestion

Split into three focused sub-tests or table entries:

  • TestCheckoutAppTokenStepsDerivesOwnerFromRepository
  • TestMCPAppTokenStepsDerivesOwnerFromRepository
  • TestSafeOutputsAppTokenStepsDerivesOwnerFromRepository

Each can share the same data fixture but assert independently, giving precise failure messages and making it easy to add edge-case variants per path.

assert.Contains(t, mcpSteps, "owner: ${{ steps.github-mcp-app-token-owner.outputs.owner }}")
assert.Contains(t, mcpSteps, "GH_AW_TARGET_REPOSITORY: ${{ github.event.inputs.trigger_ref }}")

job, _, err := compiler.buildConsolidatedSafeOutputsJob(data, string(constants.AgentJobName), "test.md")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] buildConsolidatedSafeOutputsJob is tested, but buildSafeOutputsJobFromParts (in compiler_safe_outputs_job.go) is a separate code path that also calls buildGitHubAppTokenMintStepForRepository. It has different appTokenFallbackRepo logic for workflow_call triggers and is not exercised here.

💡 Suggestion

Add a test that drives buildSafeOutputsJobFromParts (or a higher-level caller that routes through it) with a workflow_call trigger and an explicit checkout repository, and assert that the inferred owner is emitted correctly. This closes the gap between the two safe-outputs compilation paths.

PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
},
ownerSourceRepo: "acme/project",
expectedOwner: "owner: acme",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing test for a bare repository name without an org prefix (e.g., ownerSourceRepo: "just-a-repo"). The bash pre-step correctly guards against this with [[ "$owner" == "$repo" ]], but the behavior is not specified by any test.

💡 Suggested test case
{
    name: "bare repo name without slash emits pre-step that will fail at runtime",
    app: &GitHubAppConfig{
        AppID:      "${{ vars.APP_ID }}",
        PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
    },
    ownerSourceRepo:  "just-a-repo",
    // deriveLiteralGitHubAppOwner returns false (no slash), so a pre-step is emitted
    expectedContains: "id: safe-outputs-app-token-owner",
    expectedContains: "GH_AW_TARGET_REPOSITORY: just-a-repo",
},

This documents that a non-owner/repo literal triggers a runtime step (which will fail with a clear message), rather than silently emitting a wrong owner.

}

func (c *Compiler) buildGitHubAppTokenMintStepWithMeta(app *GitHubAppConfig, permissions *Permissions, fallbackRepoExpr string, stepName string, stepID string) []string {
func (c *Compiler) buildGitHubAppTokenMintStepForRepository(app *GitHubAppConfig, permissions *Permissions, fallbackRepoExpr string, ownerSourceRepository string) []string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/improve-codebase-architecture] buildGitHubAppTokenMintStepForRepository is a thin wrapper that differs from buildGitHubAppTokenMintStep only by the ownerSourceRepository argument, making the API surface larger without adding clarity.

💡 Suggestion

The two wrappers could be merged by making ownerSourceRepository a parameter of buildGitHubAppTokenMintStep (replacing the old signature that always passed ""), or by collapsing buildGitHubAppTokenMintStepForRepository back into a direct call to buildGitHubAppTokenMintStepWithMeta at its call sites. Either way, having three levels (WithMeta, ForRepository, plain) for what is essentially one operation with optional arguments increases cognitive overhead.

}
})
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] No test exercises the multi-distinct-repos path where inferSingleCheckoutRepositoryForGitHubAppOwner returns "" due to ambiguity (two different repositories configured). This is a boundary condition of the new derivation logic.

💡 Suggested test
func TestInferSingleCheckoutRepositoryMultipleDistinctRepos(t *testing.T) {
    data := &WorkflowData{
        CheckoutConfigs: []*CheckoutConfig{
            {Repository: "org-a/repo"},
            {Repository: "org-b/other"},
        },
    }
    got := inferSingleCheckoutRepositoryForGitHubAppOwner(data)
    assert.Equal(t, "", got, "multiple distinct repos should return empty string")
}

Without this, the documented behavior of the function (returns "" for ambiguous case) is unspecified by tests, and the silent fallback to ${{ github.repository_owner }} in that scenario is unverified.

steps = append(steps, " shell: bash\n")
steps = append(steps, " run: |\n")
steps = append(steps, " set -euo pipefail\n")
steps = append(steps, " repo=\"${GH_AW_TARGET_REPOSITORY%.wiki}\"\n")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The .wiki suffix stripping (${GH_AW_TARGET_REPOSITORY%.wiki}) is implemented but not tested. A future change to this line could silently break derivation for wiki repositories.

💡 Suggested test case

Add a table entry in TestBuildGitHubAppTokenMintStepOwnerDerivation:

{
    name: "wiki repository expression emits pre-step with wiki stripped",
    app: &GitHubAppConfig{
        AppID:      "${{ vars.APP_ID }}",
        PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
    },
    ownerSourceRepo:  "${{ inputs.wiki_repo }}",
    expectedContains: "GH_AW_TARGET_REPOSITORY%.wiki",
},

Alternatively, test the bash step content directly for the presence of the .wiki stripping line when a non-literal source is used.

@pelikhan

pelikhan commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Ran the PR finisher pass on HEAD 93c85e7.

  • Local validation is green: make fmt, make lint, make test-unit, make test, and make recompile all passed.
  • No new actionable @copilot review request required code changes in this pass, so no new commit was created.
  • Latest PR-head workflow runs are action_required with no failed jobs; CI on this HEAD still needs a human-triggered re-run if required by branch policy.

@pelikhan

pelikhan commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

@copilot review all review comments from GitHub-actions bot

@pelikhan

pelikhan commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

@copilot do a security analysis for this change to determine if it enables some malicious scenarios or confusion. Ensure extensive logging.

Copilot AI and others added 3 commits June 9, 2026 02:16
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot review all review comments from GitHub-actions bot

Reviewed all current GitHub-actions bot review comments and pushed follow-up fixes in commits e7b5ed8, 16f19c6, and 7ac4821.

Implemented:

  • notify_comment now derives GitHub App owner from inferred checkout repository source.
  • Owner-source inference now falls back to ${{ needs.activation.outputs.target_repo }} for workflow_call when no explicit checkout repository is set.
  • Repository expression is whitespace-normalized before emitting GH_AW_TARGET_REPOSITORY.
  • Added/expanded tests for fallback behavior, ambiguous multi-checkout handling, ordering, workflow_call path, bare repo input, wiki stripping, and whitespace normalization.
  • Added //go:build !integration and assertion messages in the new test file.

Validation: make lint, targeted go test ./pkg/workflow runs, and make test-unit all pass locally.

Copilot AI changed the title Derive omitted GitHub App owners from checkout.repository Derive omitted GitHub App owners from effective checkout target Jun 9, 2026
Copilot AI and others added 2 commits June 9, 2026 02:36
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot do a security analysis for this change to determine if it enables some malicious scenarios or confusion. Ensure extensive logging.

Implemented in commits 2be8e74 and 4b3501b.

Security analysis outcome: owner derivation now has stronger auditability around confusion/misrouting risks (ambiguous checkout sources, workflow_call fallback source, expression-based runtime derivation). Added extensive logging at both compile time and runtime:

  • compile-time logs for owner-source selection decisions and fallbacks
  • runtime logs in generated owner-derivation step for derivation start, normalized repository candidate, derived owner, and explicit failure path

Validation run: targeted workflow owner-derivation tests passed, full lint/test/build suite had passed on this branch before these final log-only refinements, and parallel validation returned no actionable security findings.

@pelikhan pelikhan merged commit aefe06f into main Jun 9, 2026
36 checks passed
@pelikhan pelikhan deleted the copilot/multi-org-github-app-support branch June 9, 2026 03:48
github-actions Bot added a commit that referenced this pull request Jun 9, 2026
…erivation

- Document object form of hide-older-comments with match list (#37977)
- Document github-app owner derivation when owner is omitted (#37976)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-org GitHub App support: derive github-app.owner per target, or allow a computed value

3 participants