fix(operator): apply feature flags when scheduled sessions trigger#1572
Open
janisz wants to merge 14 commits into
Open
fix(operator): apply feature flags when scheduled sessions trigger#1572janisz wants to merge 14 commits into
janisz wants to merge 14 commits into
Conversation
Scheduled sessions were not respecting workspace feature flags like `jira-write` because the session-trigger command creates AgenticSession CRs directly from a pre-stored template without re-evaluating feature flags at trigger time. This fix adds applyFeatureFlagOverrides() to read the workspace's feature-flag-overrides ConfigMap and apply settings (e.g., setting JIRA_READ_ONLY_MODE=false when jira-write is enabled) before creating the session CR. The implementation follows the same pattern as CreateAgenticSession in the backend: read ConfigMap, check flag value, inject environment variable. Errors are logged but non-fatal to ensure degraded operation. Fixes ambient-code#1506 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds workspace-level feature-flag overrides: operator reads a ChangesWorkspace Feature-Flag Overrides for JIRA Write Access
Sequence DiagramsequenceDiagram
participant Operator as Operator (trigger)
participant ConfigClient as K8s API (ConfigMap)
participant Dynamic as DynamicClient (AgenticSession CR)
participant Template as Session Template
Operator->>ConfigClient: GET ConfigMap "feature-flag-overrides" (namespace)
alt ConfigMap exists and jira-write == "true"
ConfigClient-->>Operator: ConfigMap data
Operator->>Template: set spec.environmentVariables["JIRA_READ_ONLY_MODE"]="false"
else ConfigMap missing / jira-write != "true"
ConfigClient-->>Operator: NotFound or non-true value
Operator-->>Template: leave env vars unchanged
end
Operator->>Dynamic: CREATE AgenticSession CR (30s ctx) with Template
Dynamic-->>Operator: creation result
Possibly Related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/operator/internal/trigger/trigger_test.go`:
- Around line 179-185: The test fixture nests the session spec under "spec"
which doesn't match the real runtime input consumed by
applyFeatureFlagOverrides; update the test to pass the spec map directly (not
wrapped as {"spec": ...}) and adjust assertions to inspect top-level keys (e.g.,
check environmentVariables on the map passed to applyFeatureFlagOverrides),
replacing usages of template["spec"] and any other occurrences in this file
(also update the similar block around the other failing assertions) so the test
exercises the same contract as applyFeatureFlagOverrides.
In `@components/operator/internal/trigger/trigger.go`:
- Around line 268-282: The code is writing environmentVariables into
template["spec"] producing spec.spec.environmentVariables; treat template as the
AgenticSession spec instead and set environmentVariables directly on template.
Update the logic that creates/reads spec and envVars (the variables named
template, spec and envVars) so you either assign spec =
template.(map[string]interface{}) or read envVars from
template["environmentVariables"] (not template["spec"]["environmentVariables"]),
then set envVars["JIRA_READ_ONLY_MODE"]="false" and store it back to
template["environmentVariables"] (or spec["environmentVariables"] if you
switched spec to reference template).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 058bc2ee-3c97-41f4-9304-fb8add42c69b
📒 Files selected for processing (2)
components/operator/internal/trigger/trigger.gocomponents/operator/internal/trigger/trigger_test.go
Comment on lines
+179
to
+185
| template := map[string]interface{}{ | ||
| "spec": map[string]interface{}{}, | ||
| } | ||
| if tt.existingEnvVars != nil { | ||
| spec := template["spec"].(map[string]interface{}) | ||
| spec["environmentVariables"] = tt.existingEnvVars | ||
| } |
Contributor
There was a problem hiding this comment.
Test fixture uses a nested spec shape that does not match runtime input.
applyFeatureFlagOverrides is called with the session spec map in production, but this test passes {"spec": {...}} and asserts inside template["spec"]. That can mask real regressions and currently validates the wrong contract.
Suggested fix
- template := map[string]interface{}{
- "spec": map[string]interface{}{},
- }
+ template := map[string]interface{}{}
if tt.existingEnvVars != nil {
- spec := template["spec"].(map[string]interface{})
- spec["environmentVariables"] = tt.existingEnvVars
+ template["environmentVariables"] = tt.existingEnvVars
}
@@
- spec, ok := template["spec"].(map[string]interface{})
- if !ok {
- t.Fatal("template[spec] is not a map")
- }
-
- envVars, ok := spec["environmentVariables"].(map[string]interface{})
+ envVars, ok := template["environmentVariables"].(map[string]interface{})
if tt.expectedEnvVars == nil {Also applies to: 195-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/operator/internal/trigger/trigger_test.go` around lines 179 - 185,
The test fixture nests the session spec under "spec" which doesn't match the
real runtime input consumed by applyFeatureFlagOverrides; update the test to
pass the spec map directly (not wrapped as {"spec": ...}) and adjust assertions
to inspect top-level keys (e.g., check environmentVariables on the map passed to
applyFeatureFlagOverrides), replacing usages of template["spec"] and any other
occurrences in this file (also update the similar block around the other failing
assertions) so the test exercises the same contract as
applyFeatureFlagOverrides.
Comment on lines
+268
to
+282
| spec, ok := template["spec"].(map[string]interface{}) | ||
| if !ok { | ||
| spec = map[string]interface{}{} | ||
| template["spec"] = spec | ||
| } | ||
|
|
||
| envVars, ok := spec["environmentVariables"].(map[string]interface{}) | ||
| if !ok { | ||
| envVars = map[string]interface{}{} | ||
| } | ||
|
|
||
| // Set JIRA_READ_ONLY_MODE to false to enable write operations | ||
| envVars["JIRA_READ_ONLY_MODE"] = "false" | ||
| spec["environmentVariables"] = envVars | ||
|
|
Contributor
There was a problem hiding this comment.
Feature-flag override is written to the wrong object level (spec.spec).
template is already the AgenticSession spec (Line 337 sets "spec": template), but this function writes to template["spec"]["environmentVariables"]. That produces spec.spec.environmentVariables, so JIRA_READ_ONLY_MODE won’t be applied where runtime expects it.
Suggested fix
- // Get or create environmentVariables map
- spec, ok := template["spec"].(map[string]interface{})
- if !ok {
- spec = map[string]interface{}{}
- template["spec"] = spec
- }
-
- envVars, ok := spec["environmentVariables"].(map[string]interface{})
+ // template is already the session spec; write env vars at this level.
+ envVars, ok := template["environmentVariables"].(map[string]interface{})
if !ok {
envVars = map[string]interface{}{}
}
// Set JIRA_READ_ONLY_MODE to false to enable write operations
envVars["JIRA_READ_ONLY_MODE"] = "false"
- spec["environmentVariables"] = envVars
+ template["environmentVariables"] = envVarsAlso applies to: 337-337
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/operator/internal/trigger/trigger.go` around lines 268 - 282, The
code is writing environmentVariables into template["spec"] producing
spec.spec.environmentVariables; treat template as the AgenticSession spec
instead and set environmentVariables directly on template. Update the logic that
creates/reads spec and envVars (the variables named template, spec and envVars)
so you either assign spec = template.(map[string]interface{}) or read envVars
from template["environmentVariables"] (not
template["spec"]["environmentVariables"]), then set
envVars["JIRA_READ_ONLY_MODE"]="false" and store it back to
template["environmentVariables"] (or spec["environmentVariables"] if you
switched spec to reference template).
- Add constants for feature flag strings (FeatureFlagOverridesConfigMap, JiraWriteFlagKey, JiraReadOnlyModeEnvVar) to replace hardcoded values - Reuse config.K8sClient and config.DynamicClient initialized in RunSessionTrigger instead of creating clients on every session - Flatten nested conditionals in createNewSession using early returns - Remove WHAT comments that duplicate code, keeping only WHY comments This addresses code review findings: stringly-typed code, redundant client creation, and unnecessary comments. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/operator/internal/trigger/trigger.go (1)
256-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrite the override into the session spec, not
spec.spec.Line 256 treats
templateas if it contains a nestedspec, but Line 304 already storestemplateas the CRspec. This producesspec.spec.environmentVariables, soJIRA_READ_ONLY_MODEwon’t be applied on the expected path.Suggested fix
- spec, ok := template["spec"].(map[string]interface{}) - if !ok { - spec = map[string]interface{}{} - template["spec"] = spec - } - - envVars, ok := spec["environmentVariables"].(map[string]interface{}) + envVars, ok := template["environmentVariables"].(map[string]interface{}) if !ok { envVars = map[string]interface{}{} } envVars[types.JiraReadOnlyModeEnvVar] = "false" - spec["environmentVariables"] = envVars + template["environmentVariables"] = envVars🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/operator/internal/trigger/trigger.go` around lines 256 - 269, The code is writing the env override into a nested spec map (creating spec["environmentVariables"] under template["spec"]), which yields spec.spec.environmentVariables; instead, locate the session spec map that was stored as the CR spec (the top-level template map) and set its environment variable there—so remove the intermediate spec nesting and write types.JiraReadOnlyModeEnvVar = "false" directly into template["environmentVariables"] (or into the sessionSpec map variable if one exists), ensuring you update the same map that is later used as the CR spec (the template variable), not a nested spec field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@components/operator/internal/trigger/trigger.go`:
- Around line 256-269: The code is writing the env override into a nested spec
map (creating spec["environmentVariables"] under template["spec"]), which yields
spec.spec.environmentVariables; instead, locate the session spec map that was
stored as the CR spec (the top-level template map) and set its environment
variable there—so remove the intermediate spec nesting and write
types.JiraReadOnlyModeEnvVar = "false" directly into
template["environmentVariables"] (or into the sessionSpec map variable if one
exists), ensuring you update the same map that is later used as the CR spec (the
template variable), not a nested spec field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27f959f8-b42b-4b92-a943-4db44d18de89
📒 Files selected for processing (3)
components/operator/internal/trigger/trigger.gocomponents/operator/internal/trigger/trigger_test.gocomponents/operator/internal/types/resources.go
🚧 Files skipped from review as they are similar to previous changes (1)
- components/operator/internal/trigger/trigger_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1506 - Scheduled sessions were not respecting workspace feature flags like
jira-write.Root Cause
The session-trigger command creates AgenticSession CRs directly from a pre-stored template without re-evaluating workspace feature flags at trigger time. When regular sessions are created via the backend API, they check the
jira-writeflag and setJIRA_READ_ONLY_MODE=falseif enabled. Scheduled sessions skipped this check entirely.Solution
Added
applyFeatureFlagOverrides()function that:feature-flag-overridesConfigMap from the workspace namespacejira-writeis set to"true"JIRA_READ_ONLY_MODE=falsein the session template's environment variables before creating the CRThe implementation follows the same pattern as
CreateAgenticSessionin the backend: read ConfigMap, check flag value, inject environment variable. Errors are logged but non-fatal to ensure degraded operation.Summary by CodeRabbit
New Features
Bug Fixes
Tests