fix: Only validate ambient-runner-secrets when Vertex AI is disabled

[sallyom]

Fixes a regression where ambient-runner-secrets validation occurs unconditionally, causing session creation to fail even when CLAUDE_CODE_USE_VERTEX=1.

This fix wraps the runner secret validation in a !vertexEnabled check, since the secret is only needed when Vertex AI is disabled. When Vertex is enabled, the ambient-vertex secret is used instead.

Changes:

• Wrap runner secret validation in 'if !vertexEnabled' block
• Add descriptive log messages for both Vertex enabled/disabled cases
• Maintain error handling and status conditions for non-Vertex case

[sallyom]

testing now

[github-actions]

Claude Code Review

Summary

This PR fixes a critical regression where Vertex AI-enabled deployments fail session creation due to unconditional validation of ambient-runner-secrets. The fix correctly wraps the validation in a !vertexEnabled check, since this secret is only needed when using Anthropic's API directly.

Verdict: ✅ Approved with minor recommendations

Issues by Severity

🚫 Blocker Issues

None - the fix is sound and resolves the regression.

🔴 Critical Issues

1. Incorrect status condition message when Vertex is enabled (lines 846-851)

The code unconditionally sets a status condition with message "Runner secret available" even when Vertex is enabled and the runner secret was never checked:

// This runs AFTER the conditional check
statusPatch.AddCondition(conditionUpdate{
    Type:    conditionSecretsReady,
    Status:  "True",
    Reason:  "AllRequiredSecretsFound",
    Message: "Runner secret available",  // ❌ Misleading when Vertex enabled
})

Recommendation: Make the message dynamic based on vertexEnabled:

var secretsMessage string
if vertexEnabled {
    secretsMessage = "Vertex AI secret available"
} else {
    secretsMessage = "Runner secret available"
}

statusPatch.AddCondition(conditionUpdate{
    Type:    conditionSecretsReady,
    Status:  "True",
    Reason:  "AllRequiredSecretsFound",
    Message: secretsMessage,
})

🟡 Major Issues

1. Missing test coverage for Vertex-enabled path

The PR modifies critical validation logic but doesn't include tests for the new conditional behavior. The existing test file (sessions_test.go) focuses on secret copying but not validation.

Recommendation: Add a test case:

func TestRunnerSecretValidation_VertexEnabled(t *testing.T) {
    // Test that runner secret validation is skipped when CLAUDE_CODE_USE_VERTEX=1
    // and session creation succeeds even without ambient-runner-secrets
}

func TestRunnerSecretValidation_VertexDisabled(t *testing.T) {
    // Test that runner secret validation fails when CLAUDE_CODE_USE_VERTEX!=1
    // and ambient-runner-secrets is missing
}

2. Inconsistent error path vs. success path

The error path (lines 822-829) updates status and returns an error, but the success path doesn't explicitly set a success status for the conditional check - it relies on the unconditional status set later at line 846.

Recommendation: For consistency, consider setting the success condition inside the !vertexEnabled block after successful validation:

if !vertexEnabled {
    if _, err := config.K8sClient.CoreV1().Secrets(...).Get(...); err != nil {
        // error handling
    }
    log.Printf("Found runner secret %s in %s (Vertex disabled)", runnerSecretsName, sessionNamespace)
    
    // Set success condition here for clarity
    statusPatch.AddCondition(conditionUpdate{
        Type:    conditionSecretsReady,
        Status:  "True",
        Reason:  "RunnerSecretFound",
        Message: "Runner secret available",
    })
}

🔵 Minor Issues

1. Log message redundancy

The log messages at lines 831 and 833 are descriptive but slightly verbose. Consider:

log.Printf("Found runner secret %s (Vertex disabled)", runnerSecretsName)
log.Printf("Vertex AI enabled, using ambient-vertex secret")

2. Comment clarity

The comment at line 810 says "ANTHROPIC_API_KEY only (ignored when Vertex enabled)" - while technically correct, it could be clearer:

const runnerSecretsName = "ambient-runner-secrets"  // ANTHROPIC_API_KEY (only used when Vertex AI is disabled)

Positive Highlights

✅ Correct conditional logic: The !vertexEnabled check is in the right place and follows the established pattern from lines 1261-1270 where secrets are conditionally injected.

✅ Proper error handling: Maintains existing error handling patterns with status updates and descriptive error messages.

✅ Good logging: The log messages clearly indicate which path is taken (Vertex enabled vs. disabled).

✅ Follows CLAUDE.md standards:

• Uses log.Printf with context (follows error handling pattern from .claude/patterns/error-handling.md)
• No panics in production code
• Proper use of errors.IsNotFound check

✅ Consistent with codebase patterns: The conditional check mirrors the pattern already used for secret injection (lines 1261-1270).

Recommendations

Priority 1 (Before Merge):

1. Fix the status condition message to be conditional on vertexEnabled (Critical Issue Outcome: Reduce Refinement Time with agent System #1)

Priority 2 (Follow-up PR):
2. Add test coverage for both Vertex-enabled and Vertex-disabled paths
3. Refactor status condition setting for better clarity

Priority 3 (Optional):
4. Minor log message cleanup
5. Comment clarity improvements

Architecture Alignment

✅ Follows operator patterns from CLAUDE.md section "Operator Patterns":

• Proper error handling with status updates
• Structured logging with context
• Uses errors.IsNotFound correctly

✅ Security standards from .claude/context/security-standards.md:

• No sensitive data in logs
• Proper error messages without exposing internals

✅ Error handling from .claude/patterns/error-handling.md:

• Pattern 1 (Resource Not Found) correctly applied
• Pattern 2 (Log + return error) correctly applied

---

Overall Assessment: This is a solid fix that resolves the regression. The only critical issue is the misleading status message when Vertex is enabled. Once that's addressed, this should be safe to merge.

Great work catching and fixing this regression! 🎉

[sallyom]

@Gkrumbach07 not sure about the Amber review, but this fixes the issue with Vertex-enabled clusters.