fix(operator): apply feature flags when scheduled sessions trigger#1572
fix(operator): apply feature flags when scheduled sessions trigger#1572janisz wants to merge 19 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 <[email protected]>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds workspace-level feature-flag overrides: operator reads the 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 | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 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}
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
- 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 <[email protected]>
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
Claude Code ReviewSummaryClean, targeted fix that correctly ports the feature-flag override pattern from FindingsBlockerNone. CriticalNone. Major1.
Minor2. RBAC not verified in diff 3. Removed explanatory comment on name-budget constraint Nit
Positive Highlights
Recommendations
Amber · standards loaded from CLAUDE.md, specs/standards/control-plane/ |
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Review — PR #1572
Summary
Correct approach overall — mirrors the backend CreateAgenticSession pattern for feature-flag injection into scheduled sessions. However, there is an unresolved structural bug that means JIRA_READ_ONLY_MODE is never actually applied at runtime. Both the implementation and its tests have the same mismatch.
Findings
Blocker — Wrong nesting level in applyFeatureFlagOverrides
template is already the AgenticSession spec — confirmed by:
template["displayName"]accessed directly increateNewSession(nottemplate["spec"]["displayName"])template["initialPrompt"]accessed directly intryReuseLastSessioncreateNewSessionwraps it as"spec": templatewhen building the CR
So template["spec"] produces an extra level of nesting: the env var lands at spec.spec.environmentVariables. The operator reads spec.environmentVariables → JIRA_READ_ONLY_MODE is never applied. The feature flag fix is silently a no-op.
trigger.go 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"] = envVarsBlocker — Tests validate the wrong contract
The tests build template = {"spec": map{}} and assert template["spec"]["environmentVariables"]. But the real caller passes a flat spec map with no outer "spec" key. The tests exercise the wrong path, so they pass regardless of whether the production bug exists.
trigger_test.go 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
}And update all assertions to read from template["environmentVariables"] directly (remove the template["spec"] intermediary).
Score
2 blockers. REJECT — the fix is mechanically small but the tests need to be corrected too, otherwise the same class of bug can recur silently.
— Amber (code review agent)
Amber Code Review — REJECTReviewing at head SHA FindingsBlocker 1 —
|
…ides CodeRabbit and Amber reviews identified critical bug: template parameter is already the AgenticSession spec, not a wrapper containing "spec" key. Before: wrote to template["spec"]["environmentVariables"] After: write to template["environmentVariables"] This was causing JIRA_READ_ONLY_MODE to be written to the wrong path (spec.spec.environmentVariables instead of spec.environmentVariables), making the feature flag fix a silent no-op at runtime. Also fixed unit tests that mirrored the production bug by validating the wrong contract. Fixes CodeRabbit review comments on PR ambient-code#1572. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
@coderabbitai full review |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
|
🤖 Amber Analysis — PR #1572: fix(operator): apply feature flags when scheduled sessions trigger Solid bug fix. The root cause (scheduled sessions bypassing workspace feature-flag evaluation) is correctly diagnosed, and the follow-up commit catching the nesting bug ( Verdict: LGTM — two minor nits below, neither blocks merge. FindingsBlockerNone. CriticalNone. MajorNone. Minor1. The function signature is Two clean options:
Relevant standard: error-handling.spec.md — misleading signatures hide intent; silent-failure anti-pattern applies even at the call site. 2. Unreachable nil guard in if config.K8sClient != nil {
// apply feature flags
}
The project standard (CLAUDE.md) is explicit: "Don't add error handling... for scenarios that can't happen." Removing the guard makes the intent clear; if future callers pass an uninitialized client, they should get a panic early rather than a silent skip. Positive Highlights
Files reviewed
Standards applied: control-plane/conventions.spec.md, error-handling.spec.md, security/security.spec.md Confidence: High (95%) — bug fix in a well-understood path, comprehensive tests, follows established patterns. |
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Review — LGTM ✓
PR: fix(operator): apply feature flags when scheduled sessions trigger
Verdict: LGTM
Summary
Clean fix for #1506. applyFeatureFlagOverrides() correctly reads the workspace feature-flag-overrides ConfigMap at trigger time and injects JIRA_READ_ONLY_MODE=false when jira-write: "true". The critical nesting bug (writing to spec.spec.environmentVariables instead of spec.environmentVariables) was caught and fixed before merge. Non-fatal error handling (log+continue) is appropriate for a degraded-operation path.
Checks Passed
- ✓ B1: No
panic()in production code - ✓ B3: No direct type assertions on K8s unstructured objects (template is local map, not
unstructured.Unstructured) - ✓ B4: Errors logged, not silently dropped
- ✓ B6: No tokens in logs
- ✓ B7: Uses
%wwhere errors propagate - ✓ Tests: 6 table-driven cases covering enabled, disabled, missing ConfigMap, existing vars preserved, other flags ignored, non-"true" value
Minor Notes (non-blocking)
applyFeatureFlagOverrideshandles onlyjira-write. If more workspace flags are added, this function grows. A map offlag → envVarcould scale better, but for one flag YAGNI applies.- The function signature returns
errorbut always returnsnil(errors are swallowed with log). The return type is future-proofing — acceptable, but callers should not expect the error path to be reachable today.
— Amber (code review agent)
2 participants
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