OLS-3589: Wire LIGHTSPEED_REASONING_CONFIG env var to sandbox pod - #339
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@onmete: This pull request references OLS-3589 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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 (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change serializes ChangesReasoning configuration propagation
Sequence Diagram(s)sequenceDiagram
participant AgentSpec
participant EnsureAgentTemplate
participant computeTemplateHash
participant SandboxTemplate
participant PodSpecBuilder
AgentSpec->>EnsureAgentTemplate: ReasoningConfig
EnsureAgentTemplate->>computeTemplateHash: reasoningConfigJSON
computeTemplateHash-->>EnsureAgentTemplate: template hash
EnsureAgentTemplate->>SandboxTemplate: Set or remove LIGHTSPEED_REASONING_CONFIG
AgentSpec->>PodSpecBuilder: ReasoningConfig
PodSpecBuilder->>PodSpecBuilder: JSON marshal configuration
PodSpecBuilder-->>SandboxTemplate: Pod environment includes LIGHTSPEED_REASONING_CONFIG
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🤖 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 `@controller/agenticrun/sandbox_templates.go`:
- Around line 246-250: Update the reasoning configuration handling around
setEnvVar so an empty reasoningConfigJSON removes LIGHTSPEED_REASONING_CONFIG
from the derived environment inherited via base.DeepCopy(). Preserve the
existing setEnvVar behavior for non-empty configurations, and add coverage for a
base template that already defines the variable.
🪄 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: bb74898f-6960-4206-90da-ce9a7d569be6
📒 Files selected for processing (4)
controller/agenticrun/podspec_builder.gocontroller/agenticrun/podspec_builder_test.gocontroller/agenticrun/sandbox_templates.gocontroller/agenticrun/sandbox_templates_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
onmete
left a comment
onmete
left a comment
There was a problem hiding this comment.
OLS-3589 review (adversarial)
Verdict: needs_review — 97/100 (jira mode)
Adherence: 4/4 ACs pass. Wiring matches spec rule 16a for bare-pod and sandbox-claim.
Code quality
- should-fix — Sandbox-claim path has no unit test asserting
LIGHTSPEED_REASONING_CONFIGis set/omitted on the derived template (only hash churn). A regression that hashes but skipssetEnvVarwould still pass CI. - nice-to-have — Absent
reasoningConfigdoes not strip an inherited base-template env var (set-only afterDeepCopy()). Latent; same pattern as other optionalLIGHTSPEED_*vars; no in-repo base sets this. - nice-to-have — Hash test only compares empty vs one JSON string.
Note: GitHub blocks REQUEST_CHANGES on own PRs, so this is posted as COMMENT.
Full write-up: .ols/reviews/OLS-3589.md
4b5dff3 to
638f390
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.ols/reviews/OLS-3589.md:
- Line 18: Update the review findings in lines 18, 33, and 44–49 to remove the
stale inherited-environment/Issue 2 defect, since the empty reasoningConfigJSON
path calls removeEnvVar and sandbox_templates_test.go covers base-template
cleanup. Retain only the valid gap concerning missing end-to-end environment
assertions.
🪄 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: becb1747-e3aa-4572-8838-7bfe3b78ced4
📒 Files selected for processing (5)
.ols/reviews/OLS-3589.mdcontroller/agenticrun/podspec_builder.gocontroller/agenticrun/podspec_builder_test.gocontroller/agenticrun/sandbox_templates.gocontroller/agenticrun/sandbox_templates_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- controller/agenticrun/sandbox_templates.go
- controller/agenticrun/podspec_builder.go
- controller/agenticrun/sandbox_templates_test.go
Serialize Agent.spec.reasoningConfig as JSON and propagate as LIGHTSPEED_REASONING_CONFIG env var on the sandbox container. Covers both bare-pod and sandbox-claim template modes. When reasoningConfig is absent, the env var is omitted entirely. Template hash includes the serialized config for proper cache invalidation. Co-authored-by: Cursor <cursoragent@cursor.com>
638f390 to
8160f33
Compare
|
@onmete: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Agent.spec.reasoningConfigas JSON and setsLIGHTSPEED_REASONING_CONFIGenv var on the sandbox containerbare-podmode (PodSpecBuilder) andsandbox-claimmode (EnsureAgentTemplate)reasoningConfigis absent, the env var is omitted entirelyImplements OLS-3589. Part of the reasoning effort epic OLS-3587.
Spec:
sandbox-execution.mdrule 16a (merged in #332).Depends on: OLS-3588 (#335, merged)
Test plan
make testpasses (all unit tests)TestPodSpecBuilder_ReasoningConfig_Present— env var set with correct JSONTestPodSpecBuilder_ReasoningConfig_Absent— env var omittedTestComputeTemplateHash_DifferentReasoningConfig— hash changes when config differsreasoningConfig→ pod hasLIGHTSPEED_REASONING_CONFIGenv varreasoningConfig→ pod does not have the env varMade with Cursor