feat(auth): decouple auth pipeline from harness-specific Go code (Phases 1-2)#321
feat(auth): decouple auth pipeline from harness-specific Go code (Phases 1-2)#321ptone wants to merge 3 commits into
Conversation
Implement Phases 1 and 2 of the auth pipeline decoupling: Phase 1 - Fix hydration ordering: - Add harness-config hydration to the env-gather pre-check path in handlers.go, before extractRequiredEnvKeys runs - Hub-managed harness-configs are now downloaded (with 10s timeout and graceful fallback) so config-driven auth metadata is available when the broker determines which env vars to request from the CLI - extractRequiredEnvKeys falls back to the hydrated path when on-disk search doesn't find the config Phase 2 - Generic env var passthrough in AuthConfig: - Add EnvVars map[string]string to api.AuthConfig for config-driven auth env vars - Modify GatherAuthWithEnv to accept optional *config.HarnessAuthMetadata and populate EnvVars from declared required_env keys - Modify ContainerScriptHarness.ResolveAuth and Generic.ResolveAuth to forward auth.EnvVars into the container environment - Make isAuthEnvKey config-aware via optional extraAuthKeys parameter - Update filterResolvedSecretsForResolvedAuth to recognize config-driven auth keys This is additive — existing hardcoded paths continue working for all built-in harnesses. The copilot harness's COPILOT_GITHUB_TOKEN / GH_TOKEN / GITHUB_TOKEN now flows through the auth pipeline via config alone, without any copilot-specific Go code.
There was a problem hiding this comment.
Code Review
This pull request introduces support for config-driven auth metadata, allowing harnesses to dynamically declare environment variable requirements without requiring Go code changes. It updates the auth gathering, filtering, and resolution pipelines to support these dynamic keys, and adds corresponding unit tests. The review feedback highlights two critical issues: potential nil pointer dereferences in pkg/agent/run.go and pkg/runtimebroker/handlers.go when accessing config fields, and a duplicate, potentially broken resolution of the harness config directory in pkg/agent/run.go that misses template paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if harnessConfigName != "" { | ||
| if hcDir, err := resolveHarnessConfigDir(ctx, harnessConfigName, projectDir); err == nil && hcDir.Config.Auth != nil { | ||
| authMeta = hcDir.Config.Auth | ||
| } | ||
| if authMeta == nil && settings != nil { | ||
| if hcEntry, err := settings.ResolveHarnessConfig(profileName, harnessConfigName); err == nil && hcEntry.Auth != nil { | ||
| authMeta = hcEntry.Auth | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Issues Identified:
- Duplicate Resolution & Missing Template Paths: This block duplicates the resolution of the harness config directory on disk (which was already performed at line 243). Furthermore, because it calls
resolveHarnessConfigDirwithout passing thetemplatePathsslice, it will fail to resolve the config directory if it is template-bundled (unlike the first resolution at line 243 which correctly passestemplatePaths...). - Defensive Programming (Nil Checks): The code accesses
hcDir.Config.AuthandhcEntry.Authwithout verifying ifhcDir,hcDir.Config, orhcEntryarenil. If any of these arenil, it will cause a runtime panic.
Recommended Refactoring:
To avoid duplicate disk I/O and fix the template-bundled resolution bug, you should declare authMeta before line 217 and populate it directly inside the first resolveHarnessConfigDir success block at line 243. Then, in this block, you only need to perform the fallback check against settings if authMeta is still nil.
For now, we have provided a code suggestion that adds the necessary defensive nil checks to prevent runtime panics.
| if harnessConfigName != "" { | |
| if hcDir, err := resolveHarnessConfigDir(ctx, harnessConfigName, projectDir); err == nil && hcDir.Config.Auth != nil { | |
| authMeta = hcDir.Config.Auth | |
| } | |
| if authMeta == nil && settings != nil { | |
| if hcEntry, err := settings.ResolveHarnessConfig(profileName, harnessConfigName); err == nil && hcEntry.Auth != nil { | |
| authMeta = hcEntry.Auth | |
| } | |
| } | |
| } | |
| if harnessConfigName != "" { | |
| if hcDir, err := resolveHarnessConfigDir(ctx, harnessConfigName, projectDir); err == nil && hcDir != nil && hcDir.Config != nil && hcDir.Config.Auth != nil { | |
| authMeta = hcDir.Config.Auth | |
| } | |
| if authMeta == nil && settings != nil { | |
| if hcEntry, err := settings.ResolveHarnessConfig(profileName, harnessConfigName); err == nil && hcEntry != nil && hcEntry.Auth != nil { | |
| authMeta = hcEntry.Auth | |
| } | |
| } | |
| } |
| if harnessType == "" && len(hydratedHarnessConfigPath) > 0 && hydratedHarnessConfigPath[0] != "" { | ||
| if hcDir, err := config.LoadHarnessConfigDir(hydratedHarnessConfigPath[0]); err == nil { | ||
| harnessType = hcDir.Config.Harness | ||
| authType = hcDir.Config.AuthSelectedType | ||
| if hcDir.Config.Auth != nil { | ||
| authMeta = hcDir.Config.Auth | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue Identified:
Defensive Programming (Nil Checks): The code accesses hcDir.Config.Harness, hcDir.Config.AuthSelectedType, and hcDir.Config.Auth without verifying if hcDir or hcDir.Config are nil. If either is nil, it will cause a runtime panic.
Recommended Fix:
Add defensive nil checks for both hcDir and hcDir.Config before accessing their fields.
| if harnessType == "" && len(hydratedHarnessConfigPath) > 0 && hydratedHarnessConfigPath[0] != "" { | |
| if hcDir, err := config.LoadHarnessConfigDir(hydratedHarnessConfigPath[0]); err == nil { | |
| harnessType = hcDir.Config.Harness | |
| authType = hcDir.Config.AuthSelectedType | |
| if hcDir.Config.Auth != nil { | |
| authMeta = hcDir.Config.Auth | |
| } | |
| } | |
| } | |
| if harnessType == "" && len(hydratedHarnessConfigPath) > 0 && hydratedHarnessConfigPath[0] != "" { | |
| if hcDir, err := config.LoadHarnessConfigDir(hydratedHarnessConfigPath[0]); err == nil && hcDir != nil && hcDir.Config != nil { | |
| harnessType = hcDir.Config.Harness | |
| authType = hcDir.Config.AuthSelectedType | |
| if hcDir.Config.Auth != nil { | |
| authMeta = hcDir.Config.Auth | |
| } | |
| } | |
| } |
- Fix gofmt: remove extra blank line in auth_test.go - Fix duplicate config resolution: reuse harness config dir already resolved during image resolution instead of re-resolving without templatePaths - Fix nil safety: add nil check on hcDir in hydrated config fallback in handlers.go
The SCION_TEST_UNSET_TOKEN key doesn't exist in the test environment, so there's nothing to unset. The assertion already verifies that a nonexistent key doesn't appear in EnvVars.
…ses 1-2) (GoogleCloudPlatform#516) * feat(auth): decouple auth pipeline from harness-specific Go code (#311) Implement Phases 1 and 2 of the auth pipeline decoupling: Phase 1 - Fix hydration ordering: - Add harness-config hydration to the env-gather pre-check path in handlers.go, before extractRequiredEnvKeys runs - Hub-managed harness-configs are now downloaded (with 10s timeout and graceful fallback) so config-driven auth metadata is available when the broker determines which env vars to request from the CLI - extractRequiredEnvKeys falls back to the hydrated path when on-disk search doesn't find the config Phase 2 - Generic env var passthrough in AuthConfig: - Add EnvVars map[string]string to api.AuthConfig for config-driven auth env vars - Modify GatherAuthWithEnv to accept optional *config.HarnessAuthMetadata and populate EnvVars from declared required_env keys - Modify ContainerScriptHarness.ResolveAuth and Generic.ResolveAuth to forward auth.EnvVars into the container environment - Make isAuthEnvKey config-aware via optional extraAuthKeys parameter - Update filterResolvedSecretsForResolvedAuth to recognize config-driven auth keys This is additive — existing hardcoded paths continue working for all built-in harnesses. The copilot harness's COPILOT_GITHUB_TOKEN / GH_TOKEN / GITHUB_TOKEN now flows through the auth pipeline via config alone, without any copilot-specific Go code. * fix(auth): address CI and review findings from PR #321 - Fix gofmt: remove extra blank line in auth_test.go - Fix duplicate config resolution: reuse harness config dir already resolved during image resolution instead of re-resolving without templatePaths - Fix nil safety: add nil check on hcDir in hydrated config fallback in handlers.go * fix(auth): remove unchecked os.Unsetenv call in test The SCION_TEST_UNSET_TOKEN key doesn't exist in the test environment, so there's nothing to unset. The assertion already verifies that a nonexistent key doesn't appear in EnvVars. --------- Co-authored-by: Scion Agent (hi-dev) <agent@scion.dev>
Summary
Implements Phases 1 and 2 of #311 — decouples the auth pipeline from harness-specific Go code and fixes hydration ordering.
Phase 1: Fix hydration ordering
extractRequiredEnvKeysin the env-gather (HTTP 202) pathextractRequiredEnvKeysaccepts an optional hydrated path to supplement on-disk searchPhase 2: Generic env var passthrough
EnvVars map[string]stringtoAuthConfigfor config-driven auth env varsGatherAuthWithEnvaccepts*config.HarnessAuthMetadataand populatesEnvVarsfrom declaredrequired_envkeysResolveAuth(bothContainerScriptHarnessandGeneric) forwardsauth.EnvVarsinto container env (hardcoded fields take precedence)isAuthEnvKeymade config-aware via variadicextraAuthKeysparameterfilterResolvedSecretsForResolvedAuthandisAuthCandidateSecretextended with config-driven key awarenessWhat this enables
New harnesses (e.g. GitHub Copilot CLI) can declare auth requirements in their
config.yamlauth:block and have tokens flow through the pipeline without any harness-specific Go code:What this does NOT change
Files changed (8 files, +403/-22)
pkg/api/types.goEnvVars map[string]stringtoAuthConfigpkg/harness/auth.goGatherAuthWithEnvaccepts auth metadata;gatherConfigEnvVarshelperpkg/harness/auth_test.gopkg/harness/container_script_harness.goResolveAuthforwardsauth.EnvVarspkg/harness/generic.goResolveAuthforwardsauth.EnvVarspkg/agent/run.goisAuthEnvKey,configAuthEnvKeySet, auth metadata resolutionpkg/agent/run_test.gopkg/runtimebroker/handlers.goextractRequiredEnvKeysaccepts hydrated pathTest plan
pkg/harness/...tests pass (0.156s) — 6 new config-driven auth testspkg/agent/...tests pass (4.870s) — 4 new testspkg/runtimebroker/...tests pass (2.556s)pkg/api/...tests pass (0.009s)go build ./...cleanCloses #311 (Phases 1-2)