feat: expose all Claude Agent SDK options in session creation UI#1146
feat: expose all Claude Agent SDK options in session creation UI#1146jeremyeder wants to merge 7 commits intoambient-code:mainfrom
Conversation
Add a collapsible "Advanced SDK Options" section to the new session page, gated behind the `advanced-sdk-options` workspace feature flag (disabled by default). Allows users to configure temperature, max tokens, thinking tokens, turn limits, cost budgets, permission mode, allowed tools, system prompt, beta flags, and more. Data flow: frontend sends sdkOptions → backend serializes into SDK_OPTIONS env var (with server-side allowlist) → runner parses and merges into adapter options (with defense-in-depth denylist). Also adds: - GHA workflow for weekly SDK options drift detection (auto-PR with amber:auto-fix label) - SDK options manifest for tracking ClaudeAgentOptions fields - Removal of unused create-session-dialog.tsx (dead code) - Removal of Type column from feature flags settings page - Tests for frontend component, runner SDK_OPTIONS parsing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Advanced SDK Options end-to-end: frontend UI and types, request wiring and backend validation/allowlist, runner-side SDK_OPTIONS env var parsing/merge with denylist and prompt-append behavior, tests, a canonical manifest, and a weekly/manual CI workflow to detect and auto-PR manifest drift. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend UI
participant Backend as Backend API
participant Runner as Ambient Runner
participant Adapter as ClaudeAgentAdapter
User->>Frontend: Enter SDK options in AdvancedSdkOptions
Frontend->>Frontend: Omit undefined fields, build payload
Frontend->>Backend: POST /createSession { ..., sdkOptions }
Backend->>Backend: filterSdkOptions(allowedSdkOptionKeys)
Backend->>Backend: JSON-marshal filtered options -> set SDK_OPTIONS env/spec
Backend-->>Frontend: 201 Created
Runner->>Runner: Read SDK_OPTIONS env var
Runner->>Runner: Parse JSON, ignore invalid/non-object, skip nulls
Runner->>Runner: Remove denylisted keys
alt system_prompt present
Runner->>Runner: Append "## Custom Instructions" to configured prompt
end
Runner->>Adapter: Construct adapter with merged options
Adapter->>Adapter: Initialize with merged SDK options
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/claude-sdk-options-drift.yml:
- Around line 73-78: The FileNotFoundError catch prints "creating initial
manifest" but immediately exits, so no manifest is created and the drift output
isn't set; change the except block for manifest_path/FileNotFoundError to
actually create an initial manifest (build a minimal manifest dict into the
variable manifest and write it to manifest_path) and ensure the step sets the
drift output (steps.check.outputs.drift) to 'true' so the "Update manifest and
create PR" step runs; update the except handler in the try/except around
json.load(f) (manifest_path, manifest) to write the initial file and set the
drift flag instead of calling sys.exit(1).
In `@components/backend/handlers/sessions.go`:
- Around line 1280-1296: The current UpdateSession logic ignores an explicit
empty SdkOptions because UpdateAgenticSessionRequest uses `omitempty` and the
handler checks `len(req.SdkOptions) > 0`; add an explicit boolean field (e.g.,
ClearSdkOptions bool) to UpdateAgenticSessionRequest and update the handler to
check that flag first: if ClearSdkOptions is true delete `SDK_OPTIONS` from
`spec["environmentVariables"]` (or set it to an empty value) using the same
`envVars` map logic; otherwise keep the existing behavior of calling
`filterSdkOptions` and marshaling when `len(filtered) > 0`. Ensure you update
any decoding/validation paths to accept the new ClearSdkOptions flag and use
`filterSdkOptions`, `SDK_OPTIONS`, and `spec["environmentVariables"]` as the
referenced symbols when implementing the change.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx:
- Around line 6-11: The tests currently create a module-scoped QueryClient
(queryClient) and wrapper using QueryClientProvider which lets query cache/state
leak between tests; change to create a brand-new QueryClient for each test
(e.g., in a beforeEach or directly inside each test) and recreate the wrapper
that uses that fresh QueryClient so QueryClientProvider wraps with a fresh
client each time; update any references to the module-level queryClient/wrapper
so tests use the per-test instance to avoid cross-test pollution.
In `@components/frontend/src/components/__tests__/advanced-sdk-options.test.tsx`:
- Around line 6-14: defaultProps.onChange is a single vi.fn() defined at module
scope and not reset between tests, causing cross-test leakage; add a test setup
that resets the mock before each test (e.g., call vi.clearAllMocks() or reassign
defaultProps.onChange = vi.fn() in a beforeEach) so assertions against
defaultProps.onChange in the AdvancedSdkOptions tests are isolated and
deterministic.
In `@components/frontend/src/components/advanced-sdk-options.tsx`:
- Around line 317-325: The "default" SelectItem value is ambiguous: change the
SelectItem with value "default" to use a clearer value like "prompt" (i.e.,
value="prompt" with label "Prompt") or alternatively update its label to "Prompt
(explicit)" — and then update all places that read that value
(handlers/conditionals that check for "default") to use the new identifier;
reference the SelectItem entries with values "__default__", "default" (to be
renamed), "acceptEdits", and "bypassPermissions" in this component so consumers
stay consistent.
- Around line 366-382: The Output Format field (Textarea with id
"sdk-output-format", bound to value.output_format and updated via update)
currently accepts any string; add client-side JSON validation: attempt
JSON.parse on change (or on blur) and set a local validation state (e.g.,
outputFormatError) when parse fails, display a concise inline error message
below the Textarea and prevent submitting/updating the config with invalid JSON
(or convert empty string to undefined as already done). Ensure the validation
runs before calling update so update only receives valid JSON (or the undefined
empty state), and reuse the existing Textarea and update flow to keep UI
consistent.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 425-452: The code assumes json.loads(SDK_OPTIONS) yields a dict
and calls .items(), but non-dict JSON (e.g., list or string) will raise an
AttributeError; after loading into sdk_opts (inside the try), check if
isinstance(sdk_opts, dict) and if not, log a warning (e.g., "SDK_OPTIONS must be
a JSON object, ignoring") and skip the merging logic so the rest of the block
(loop over sdk_opts.items(), system_prompt handling, applied counting, and final
logger.info) only runs when sdk_opts is a dict; keep the existing except for
JSONDecodeError/TypeError as-is and continue to respect _SDK_OPTIONS_DENYLIST
and options merging behavior.
In `@components/runners/ambient-runner/sdk-options-manifest.json`:
- Around line 1-95: The manifest and backend disagree: backend symbol
allowedSdkOptionKeys permits "timeout" and "inactivity_timeout" but the
generated ClaudeAgentOptions manifest (JSON options block in
sdk-options-manifest.json) lacks them; inspect the ClaudeAgentOptions definition
in the claude-agent-sdk (or the code that constructs allowedSdkOptionKeys) to
confirm whether "timeout" and "inactivity_timeout" are real SDK fields, then
either add them to the manifest's "options" with appropriate types (e.g., "int"
or "float", and required:false) to match the SDK, or remove them from the
backend allowedSdkOptionKeys allowlist so both sides stay consistent.
In `@components/runners/ambient-runner/tests/test_sdk_options.py`:
- Around line 8-9: Add a unit test in TestSdkOptionsEnvVar that verifies
bridge.py's denylist filters platform-managed keys: set SDK_OPTIONS to include a
denied key (e.g., "cwd" or "resume"), call the code path that invokes
_ensure_adapter (or import/instantiate the adapter to trigger parsing), and
assert the resulting parsed options do not contain the denied key and that a
warning was logged; reference the denylist in bridge.py (keys: cwd, resume,
mcp_servers, setting_sources) and use the test harness' log-capturing/assertion
utilities to confirm the silent filtering and warning message.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe18fd96-3d48-4400-a798-eaccbfe6e392
📒 Files selected for processing (15)
.github/workflows/claude-sdk-options-drift.ymlcomponents/backend/handlers/sessions.gocomponents/backend/types/session.gocomponents/frontend/src/app/projects/[name]/new/page.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/new-session-view.tsxcomponents/frontend/src/components/__tests__/advanced-sdk-options.test.tsxcomponents/frontend/src/components/advanced-sdk-options.tsxcomponents/frontend/src/components/create-session-dialog.tsxcomponents/frontend/src/components/workspace-sections/feature-flags-section.tsxcomponents/frontend/src/types/api/sessions.tscomponents/manifests/base/core/flags.jsoncomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/sdk-options-manifest.jsoncomponents/runners/ambient-runner/tests/test_sdk_options.py
💤 Files with no reviewable changes (1)
- components/frontend/src/components/create-session-dialog.tsx
...rc/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx
Outdated
Show resolved
Hide resolved
components/frontend/src/components/__tests__/advanced-sdk-options.test.tsx
Show resolved
Hide resolved
components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py
Show resolved
Hide resolved
Feedback on Allowed Tools UINice work on exposing SDK options — this will be really useful for power users. One concern about the Allowed Tools section:
|
- Fix GHA FileNotFoundError message (was misleading about creating manifest) - Guard against non-dict JSON in SDK_OPTIONS parsing (runner) - Remove timeout/inactivity_timeout from SDK allowlist (platform-level, not SDK) - Per-test QueryClient in new-session-view tests (prevent cache leakage) - Add beforeEach mock reset in advanced-sdk-options tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/backend/handlers/sessions.go (1)
1278-1294:⚠️ Potential issue | 🟠 Major
UpdateSessionreplacessdkOptionsinstead of patching them.This handler otherwise behaves like a sparse update, but this block rewrites
SDK_OPTIONSwith only the keys present in the request. Updating one field drops previously stored settings, and an explicit empty object still cannot clear the env var. Merge against the existing JSON blob, or add an explicit replace/clear contract so partial updates are predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/sessions.go` around lines 1278 - 1294, The UpdateSession handler currently replaces SDK_OPTIONS with only keys from req.SdkOptions, causing partial updates to drop existing settings; instead, read the current spec["environmentVariables"]["SDK_OPTIONS"] JSON (if present), unmarshal to a map, merge/override with filterSdkOptions(req.SdkOptions) results, then marshal and store back into envVars["SDK_OPTIONS"]; also handle an explicit empty object in req.SdkOptions by treating it as a clear request (set SDK_OPTIONS to "{}" or delete the key) according to the API contract. Use the existing symbols filterSdkOptions, spec, envVars, and SDK_OPTIONS to locate and modify the logic in the UpdateSession handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/claude-sdk-options-drift.yml:
- Around line 83-96: The workflow currently only compares manifest_keys vs
current_keys (manifest_keys, current_keys, new_keys, removed_keys) so changes to
an option's metadata (type/required) are ignored; update the check to compare
the full option metadata objects for each key by computing diffs between
manifest.get('options', {}) and current (e.g., iterating option names in the
union of keys and for each name compare manifest_options[name] vs
current[name]), report three categories: new options (names in current not in
manifest), removed options (names in manifest not in current), and changed
options (names present in both but with differing metadata), print detailed
before/after metadata for any changed options and exit non-zero if any of the
three categories are non-empty so ClaudeAgentOptions metadata drift is detected.
In `@components/backend/handlers/sessions.go`:
- Around line 131-139: filterSdkOptions currently only allowlists key names but
doesn't validate value shapes; update it to validate types and ranges for each
allowed key (e.g. ensure "allowed_tools" is an array/slice of strings,
"temperature" is a numeric value within the expected range, and any other keys
in allowedSdkOptionKeys have the expected types/formats), return an error when
any value is malformed (change signature to return (map[string]interface{},
error) or equivalent), and propagate that error up to the session create/update
handler so it responds with HTTP 400 instead of passing invalid values into
SDK_OPTIONS or the runner.
In `@components/frontend/src/components/advanced-sdk-options.tsx`:
- Around line 25-39: The hard-coded DEFAULT_TOOLS array in
advanced-sdk-options.tsx diverges from the runner's actual allowlist; replace
the static DEFAULT_TOOLS constant with a single source of truth by fetching or
importing the runner's allowlist (the same contract used by
ambient_runner/bridges/claude/mcp.py::DEFAULT_ALLOWED_TOOLS) so the UI only
exposes tools the runner can grant; update the Advanced SDK options component to
load that list (via a backend endpoint that returns the runner allowlist or a
shared JS/JSON config module) and use that value for defaults and persistence
logic instead of the current hard-coded DEFAULT_TOOLS.
- Around line 66-72: The toggle logic treats value.allowed_tools undefined as an
empty override; change it to treat undefined as "use runner defaults": when
reading current (in toggleTool and the similar helpers/switches around 89-97 and
348-357) if value.allowed_tools is undefined, initialize current from the runner
default set (a DEFAULT_ALLOWED_TOOLS or getRunnerDefaultAllowedTools() source),
then compute next (add/remove). When writing, only serialize an explicit array
if the user is actually overriding defaults—i.e., if next differs from the
runner default set; otherwise call update({ allowed_tools: undefined }) to
preserve runner defaults. Ensure equality comparison uses set semantics
(order-insensitive).
---
Duplicate comments:
In `@components/backend/handlers/sessions.go`:
- Around line 1278-1294: The UpdateSession handler currently replaces
SDK_OPTIONS with only keys from req.SdkOptions, causing partial updates to drop
existing settings; instead, read the current
spec["environmentVariables"]["SDK_OPTIONS"] JSON (if present), unmarshal to a
map, merge/override with filterSdkOptions(req.SdkOptions) results, then marshal
and store back into envVars["SDK_OPTIONS"]; also handle an explicit empty object
in req.SdkOptions by treating it as a clear request (set SDK_OPTIONS to "{}" or
delete the key) according to the API contract. Use the existing symbols
filterSdkOptions, spec, envVars, and SDK_OPTIONS to locate and modify the logic
in the UpdateSession handler.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d80b3ded-34d9-42ea-ad80-84abf6135b43
📒 Files selected for processing (7)
.github/workflows/claude-sdk-options-drift.ymlcomponents/backend/handlers/sessions.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsxcomponents/frontend/src/components/__tests__/advanced-sdk-options.test.tsxcomponents/frontend/src/components/advanced-sdk-options.tsxcomponents/frontend/src/types/api/sessions.tscomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py
PR Fixer Report✅ Rebase CompleteRebased Resolved conflicts:
✅ Addressed Review FeedbackHuman reviewer (jwm4) - Critical UX issues:
Details: When Bot reviews (CodeRabbit): Most suggestions were style preferences or hypothetical concerns (e.g., "consider adding validation"). Already addressed in commit
📊 CI StatusAll checks passing (3/3):
🚨 Action RequiredCannot push directly - GitHub blocks bot tokens from modifying workflow files without To complete the fix: git fetch origin pull/1146/head:pr-1146
git checkout pr-1146
git pull --rebase origin main
# Resolve any conflicts (already done above)
git push fork feat/advanced-sdk-options --force-with-leaseOr pull these commits and push from your fork: git remote add ambient https://github.com/ambient-code/platform.git
git fetch ambient
git checkout feat/advanced-sdk-options
git reset --hard ambient/pr-1146
git push origin feat/advanced-sdk-options --force-with-leaseCommits ready to push:
Fixed by Claude Code PR Fixer |
PR Fixer Report✅ Rebase CompleteSuccessfully rebased PR #1146 onto latest
📝 Review Comments AddressedFixed Issues
Skipped (Not Blocking)
🧪 CI StatusAll checks passing ✅ 📦 Commits Added
|
Automated Review Fix ReportRebaseSuccessfully rebased
All 13 CodeRabbit Review Comments AddressedReview 1 (9 comments):
Review 2 (4 comments): Tests
|
|
Superseded by #1179 — rebased onto main with all 13 CodeRabbit review comments addressed. This PR could not be updated in place because the GitHub App token lacks the The drift-detection workflow ( |
Rebase Status Update (retry_count=0)The branch has been rebased onto GitHub App tokens cannot push commits that change The rebased code is available at:
To resolve on this PR, the author can run: git fetch upstream main
git rebase upstream/main
# Apply review fixes from #1179 or cherry-pick 8de71d38
git push --force-with-lease origin feat/advanced-sdk-optionsOr simply use #1179 as the replacement PR. |
Rebase Attempt (retry_count=1)Rebase status: The branch has been successfully rebased onto Push status: Cannot push to fork. The GitHub App integration has zero write permissions on Resolution: The fully rebased and fixed code is available in PR #1179 ( To fix this PR directly, the fork owner ( git remote add upstream https://github.com/ambient-code/platform.git
git fetch upstream feat/advanced-sdk-options
git checkout feat/advanced-sdk-options
git reset --hard upstream/feat/advanced-sdk-options
git push --force-with-lease origin feat/advanced-sdk-options |
Resolve merge conflicts preserving both sides: - Backend: keep jira-write flag + SDK options serialization - Frontend: merge branch/autoPush repo fields with sdkOptions - Tests: keep add-context-modal mock + feature-flags-admin mock - Flags: add scheduled-session.reuse.enabled + jira-write + advanced-sdk-options Includes enhanced CodeRabbit review fixes: - filterSdkOptions with type validation and error returns - ClearSdkOptions support in UpdateSession - Permission mode UI mapping fix - Output format JSON validation - Denylist filtering test + manifest timeout fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/backend/handlers/sessions.go (1)
833-867:⚠️ Potential issue | 🟠 MajorBlock direct
SDK_OPTIONSinjection throughenvironmentVariables.If
req.SdkOptionsis empty, a caller can still setenvironmentVariables.SDK_OPTIONSat Lines 839-841 and skipfilterSdkOptions()entirely. That sends an unfiltered payload to the runner and defeats the server-side allowlist this PR is adding.Guard the reserved env var
for k, v := range req.EnvironmentVariables { + if k == "SDK_OPTIONS" { + c.JSON(http.StatusBadRequest, gin.H{ + "error": "SDK_OPTIONS must be provided via sdkOptions", + }) + return + } envVars[k] = v }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/sessions.go` around lines 833 - 867, The code currently merges req.EnvironmentVariables into envVars before serializing req.SdkOptions, allowing callers to set SDK_OPTIONS directly; to fix, prevent direct injection by skipping the reserved key when merging user-provided env vars: in the loop that copies req.EnvironmentVariables into envVars (the block referencing envVars and req.EnvironmentVariables), do not copy any entry with key "SDK_OPTIONS" (and optionally also skip "sdk_options" if you want case-insensitive protection); keep the later logic that sets envVars["SDK_OPTIONS"] only when filterSdkOptions(req.SdkOptions) produces a non-empty result so SDK_OPTIONS can only come from req.SdkOptions via filterSdkOptions.
♻️ Duplicate comments (2)
components/frontend/src/components/advanced-sdk-options.tsx (1)
25-41:⚠️ Potential issue | 🟠 MajorAllowed-tools overrides are still inverted on a fresh form.
Line 73 treats
allowed_tools === undefinedas the full default set, but Line 98 and Line 376 still treat it as[]. On a new session every default tool renders off, the first click removes that tool from the default allowlist instead of enabling it, adding a custom tool drops the defaults entirely, and Line 77 collapses an explicit empty allowlist back to runner defaults. The hard-coded list is also still out of sync with the runner defaults called out in this PR (Skill/MCP wildcards missing, removed tools still shown).Also applies to: 69-78, 95-100, 369-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/advanced-sdk-options.tsx` around lines 25 - 41, The allowed-tools logic is inverted and the hard-coded DEFAULT_TOOLS is out of sync: update the DEFAULT_TOOLS constant to exactly match the runner's current allowed_tools (add Skill and MCP wildcard entries and remove deprecated tools), then change the form initialization and handlers so that undefined allowed_tools is treated as "use all defaults" (i.e., render defaults as checked) while an explicit [] remains empty; specifically adjust the component state/renderer that reads allowed_tools, the toggle handler that currently removes/adds entries (e.g., the tool checkbox onChange), the addCustomTool path that currently replaces defaults, and the collapse/restore logic that treats an explicit empty list as defaults—make those functions operate on a computed effectiveAllowlist (defaults when undefined, empty when []), and persist the actual allowed_tools value unchanged (undefined vs [] vs explicit list) when saving.components/backend/handlers/sessions.go (1)
147-185:⚠️ Potential issue | 🟠 MajorReject malformed numeric SDK options here, not in the runner.
The
float64branch at Lines 163-168 accepts fractional values like1.5formax_tokens,max_thinking_tokens, andmax_turns, and the numeric cases never enforce the UI bounds either. Those invalid values are then serialized intoSDK_OPTIONSand only fail later during runner startup instead of returning400at the API boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/sessions.go` around lines 147 - 185, The validateSdkOptionValue function currently allows fractional numbers for keys "max_tokens", "max_thinking_tokens", and "max_turns" and doesn't enforce UI bounds; update validateSdkOptionValue to reject non-integer numeric values (e.g., if value is float64 ensure it has no fractional part) for those keys and enforce the same min/max bounds the UI expects (use the existing constants or config used elsewhere, or add constants next to validateSdkOptionValue) so malformed numeric options return an error (400) at the API boundary rather than later in the runner; refer to the keys "max_tokens", "max_thinking_tokens", "max_turns" and the function validateSdkOptionValue to locate and change the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/backend/handlers/sessions.go`:
- Around line 833-867: The code currently merges req.EnvironmentVariables into
envVars before serializing req.SdkOptions, allowing callers to set SDK_OPTIONS
directly; to fix, prevent direct injection by skipping the reserved key when
merging user-provided env vars: in the loop that copies req.EnvironmentVariables
into envVars (the block referencing envVars and req.EnvironmentVariables), do
not copy any entry with key "SDK_OPTIONS" (and optionally also skip
"sdk_options" if you want case-insensitive protection); keep the later logic
that sets envVars["SDK_OPTIONS"] only when filterSdkOptions(req.SdkOptions)
produces a non-empty result so SDK_OPTIONS can only come from req.SdkOptions via
filterSdkOptions.
---
Duplicate comments:
In `@components/backend/handlers/sessions.go`:
- Around line 147-185: The validateSdkOptionValue function currently allows
fractional numbers for keys "max_tokens", "max_thinking_tokens", and "max_turns"
and doesn't enforce UI bounds; update validateSdkOptionValue to reject
non-integer numeric values (e.g., if value is float64 ensure it has no
fractional part) for those keys and enforce the same min/max bounds the UI
expects (use the existing constants or config used elsewhere, or add constants
next to validateSdkOptionValue) so malformed numeric options return an error
(400) at the API boundary rather than later in the runner; refer to the keys
"max_tokens", "max_thinking_tokens", "max_turns" and the function
validateSdkOptionValue to locate and change the checks.
In `@components/frontend/src/components/advanced-sdk-options.tsx`:
- Around line 25-41: The allowed-tools logic is inverted and the hard-coded
DEFAULT_TOOLS is out of sync: update the DEFAULT_TOOLS constant to exactly match
the runner's current allowed_tools (add Skill and MCP wildcard entries and
remove deprecated tools), then change the form initialization and handlers so
that undefined allowed_tools is treated as "use all defaults" (i.e., render
defaults as checked) while an explicit [] remains empty; specifically adjust the
component state/renderer that reads allowed_tools, the toggle handler that
currently removes/adds entries (e.g., the tool checkbox onChange), the
addCustomTool path that currently replaces defaults, and the collapse/restore
logic that treats an explicit empty list as defaults—make those functions
operate on a computed effectiveAllowlist (defaults when undefined, empty when
[]), and persist the actual allowed_tools value unchanged (undefined vs [] vs
explicit list) when saving.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f7addb9-e73e-44a5-9fa1-b41656b67621
📒 Files selected for processing (10)
components/backend/handlers/sessions.gocomponents/backend/types/session.gocomponents/frontend/src/app/projects/[name]/new/page.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/new-session-view.tsxcomponents/frontend/src/components/advanced-sdk-options.tsxcomponents/manifests/base/core/flags.jsoncomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/sdk-options-manifest.jsoncomponents/runners/ambient-runner/tests/test_sdk_options.py
✅ Files skipped from review due to trivial changes (2)
- components/runners/ambient-runner/sdk-options-manifest.json
- components/manifests/base/core/flags.json
🚧 Files skipped from review as they are similar to previous changes (3)
- components/frontend/src/app/projects/[name]/new/page.tsx
- components/backend/types/session.go
- components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/tests/new-session-view.test.tsx
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI StatusAn empty commit ( Workflows pending approval:
A maintainer needs to click "Approve and run" on any of these workflows at: Local verification (all passing):
|
Review Queue Status
Action needed: Resolve merge conflicts (rebase on main)
|
Pick up latest main changes (DataScienceCluster CRD wait, RHOAI MLflow). Workflow files kept as-is to avoid GitHub App token restrictions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change the Advanced SDK Options tools section from a hardcoded whitelist (DEFAULT_TOOLS) to a blocklist approach (BLOCKED_TOOLS + KNOWN_TOOLS). All known tools now show as checked by default. Toggling a tool off sends an explicit list of all tools minus the blocked one, preventing the UX footgun where toggling one tool on would replace all defaults. Adds Skill to the known tools list (was missing, needed for workflow users). BLOCKED_TOOLS is empty — all tools enabled by default. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/frontend/src/components/advanced-sdk-options.tsx (1)
31-46:⚠️ Potential issue | 🟠 Major
allowed_toolsstill overrides runner defaults incorrectly on first editLine 76 seeds from
KNOWN_TOOLSwhenallowed_toolsisundefined, and Line 80 then persists that as an explicit override. This can silently remove runner-default tools (including dynamicmcp__<name>__*entries) as soon as a user toggles one switch, andKNOWN_TOOLScurrently includes entries that are not part of the stated runner default set.Also applies to: 74-80, 379-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/advanced-sdk-options.tsx` around lines 31 - 46, The current logic in advanced-sdk-options.tsx seeds the user's allowed_tools state from KNOWN_TOOLS when allowed_tools is undefined, then persists that seed as an explicit override (causing runner defaults and dynamic mcp__... tools to be lost); change the behavior so that undefined allowed_tools is treated as "no override" and only write an explicit allowed_tools array when the user intentionally toggles a tool. Concretely, update the code paths that reference KNOWN_TOOLS and allowed_tools (the initialization/seed around the symbol allowed_tools and the save/patch logic used when toggling tools at the spots referenced near lines 74-80 and 379-383) to: 1) avoid writing back a seeded array when the initial value is undefined, 2) compute the UI state for switches by deriving from (allowed_tools ?? runnerDefaults) without mutating allowed_tools, and 3) only call the persistence/patch function to set allowed_tools when the user action genuinely changes an explicit override. Ensure KNOWN_TOOLS remains only a UI convenience list and not auto-saved as an override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/frontend/src/components/advanced-sdk-options.tsx`:
- Around line 31-46: The current logic in advanced-sdk-options.tsx seeds the
user's allowed_tools state from KNOWN_TOOLS when allowed_tools is undefined,
then persists that seed as an explicit override (causing runner defaults and
dynamic mcp__... tools to be lost); change the behavior so that undefined
allowed_tools is treated as "no override" and only write an explicit
allowed_tools array when the user intentionally toggles a tool. Concretely,
update the code paths that reference KNOWN_TOOLS and allowed_tools (the
initialization/seed around the symbol allowed_tools and the save/patch logic
used when toggling tools at the spots referenced near lines 74-80 and 379-383)
to: 1) avoid writing back a seeded array when the initial value is undefined, 2)
compute the UI state for switches by deriving from (allowed_tools ??
runnerDefaults) without mutating allowed_tools, and 3) only call the
persistence/patch function to set allowed_tools when the user action genuinely
changes an explicit override. Ensure KNOWN_TOOLS remains only a UI convenience
list and not auto-saved as an override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65fc332d-bebb-4bce-9282-bf2566820823
📒 Files selected for processing (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsxcomponents/frontend/src/components/advanced-sdk-options.tsx
Resolve conflicts with latest main (DSC v2 API, UBI 10 upgrade). Keep both sdkOptions and agentOptions fields in sessions.ts. Workflow files preserved from fork branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/runners/state-sync/Dockerfile (1)
1-13:⚠️ Potential issue | 🟠 MajorRun the state-sync image as non-root.
There is still no
USERhere, so the container defaults to UID 0. That is a security risk and can also fail underrunAsNonRootor similarly restricted cluster policies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/state-sync/Dockerfile` around lines 1 - 13, The Dockerfile runs as root by default; create a non-root user and switch to it before the ENTRYPOINT to satisfy runAsNonRoot policies. After installing packages and copying/making executable the scripts (hydrate.sh and sync.sh), add steps to create a dedicated user (e.g., addgroup/adduser or adduser -D -u 1000 runner), chown the scripts and any required directories to that user, set HOME if needed, and then add a USER runner (or chosen username) so the container runs the ENTRYPOINT ("/usr/local/bin/sync.sh") as non-root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/frontend/src/components/claude-agent-options/_components/hooks-editor.tsx`:
- Around line 59-61: The onChange for the timeout input currently writes
Number(e.target.value) directly into state allowing 0, negatives, and decimals
which violate the timeout >= 1 schema; update the handler in the timeout Input
so you parse the input (e.g. const v = Number(e.target.value)), check
Number.isFinite(v) and v >= 1, and if valid call updateHook(event, i, { ...hook,
timeout: Math.floor(v) }) (or leave undefined when the field is empty); if
invalid set timeout to undefined (or keep previous) to avoid storing values <1
or non-integers. Ensure you reference the timeout field on the hook and the
updateHook function when making this change.
In
`@components/frontend/src/components/claude-agent-options/_components/output-format-field.tsx`:
- Around line 27-47: The handler handleChange currently calls
onChange(undefined) on JSON parse failure which can flip value to undefined and
trigger the useEffect (watching value) to reset rawJson to "", erasing the
user's in-progress edits; remove the onChange(undefined) call in the catch block
and instead only call setJsonError(...) so parsing failures are flagged but the
external value is left unchanged (keep onChange(JSON.parse(text)) on success),
ensuring useEffect won't clear rawJson when the user types invalid JSON.
In `@components/frontend/src/components/claude-agent-options/schema.ts`:
- Around line 7-12: permissionModeSchema currently includes "plan", which is not
accepted by the SdkOptions.permission_mode type; remove "plan" from the z.enum
declaration for permissionModeSchema so it only lists "default", "acceptEdits",
and "bypassPermissions". Update any related usages that rely on the enum (e.g.,
the form option rendering code in options-form and any transformations in
AdvancedSdkOptions) to ensure they no longer expect or filter out "plan" so the
schema and SdkOptions.permission_mode remain consistent.
In `@components/runners/ambient-runner/Dockerfile`:
- Around line 17-23: The RUN block in the Dockerfile that downloads gh and glab
(using GH_VERSION and GLAB_VERSION) lacks checksum/signature verification;
update that RUN step to fetch the corresponding checksum or signature files from
the official release pages, verify the downloaded tarballs (e.g., with sha256sum
or gpg --verify) before extracting, and fail the build if verification fails;
ensure the verification uses the same ARCH and version variables (GH_VERSION,
GLAB_VERSION) and remove the tarball after successful verification/extraction to
keep the image clean.
In `@components/runners/ambient-runner/docs/UPDATE_PROCEDURE.md`:
- Around line 257-260: Update the Unleash flag name in the docs: replace the
incorrect flag string "advanced-agent-options" with the actual flag used by the
PR, "advanced-sdk-options", and ensure the instruction still notes the tag
requirement `scope: workspace` so the new session UI is correctly gated in
Unleash.
---
Outside diff comments:
In `@components/runners/state-sync/Dockerfile`:
- Around line 1-13: The Dockerfile runs as root by default; create a non-root
user and switch to it before the ENTRYPOINT to satisfy runAsNonRoot policies.
After installing packages and copying/making executable the scripts (hydrate.sh
and sync.sh), add steps to create a dedicated user (e.g., addgroup/adduser or
adduser -D -u 1000 runner), chown the scripts and any required directories to
that user, set HOME if needed, and then add a USER runner (or chosen username)
so the container runs the ENTRYPOINT ("/usr/local/bin/sync.sh") as non-root.
🪄 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: Pro
Run ID: 0952188b-d9fa-46cb-aef7-d4f9bbd5ed4c
⛔ Files ignored due to path filters (1)
components/runners/ambient-runner/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.specify/constitutions/runner.md.specify/sdd-manifest.yaml.specify/specs/runner.mdcomponents/frontend/src/components/claude-agent-options/_components/agents-editor.tsxcomponents/frontend/src/components/claude-agent-options/_components/hooks-editor.tsxcomponents/frontend/src/components/claude-agent-options/_components/key-value-editor.tsxcomponents/frontend/src/components/claude-agent-options/_components/mcp-servers-editor.tsxcomponents/frontend/src/components/claude-agent-options/_components/output-format-field.tsxcomponents/frontend/src/components/claude-agent-options/_components/plugins-editor.tsxcomponents/frontend/src/components/claude-agent-options/_components/sandbox-field.tsxcomponents/frontend/src/components/claude-agent-options/_components/section.tsxcomponents/frontend/src/components/claude-agent-options/_components/string-list-editor.tsxcomponents/frontend/src/components/claude-agent-options/_components/system-prompt-field.tsxcomponents/frontend/src/components/claude-agent-options/_components/thinking-field.tsxcomponents/frontend/src/components/claude-agent-options/index.tscomponents/frontend/src/components/claude-agent-options/options-form.tsxcomponents/frontend/src/components/claude-agent-options/schema.tscomponents/frontend/src/types/agentic-session.tscomponents/frontend/src/types/api/sessions.tscomponents/runners/ambient-runner/.mcp.jsoncomponents/runners/ambient-runner/Dockerfilecomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/handlers.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/reasoning_events.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/utils.pycomponents/runners/ambient-runner/ag_ui_gemini_cli/adapter.pycomponents/runners/ambient-runner/ag_ui_gemini_cli/types.pycomponents/runners/ambient-runner/ambient_runner/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/bridges/gemini_cli/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/gemini_cli/mcp.pycomponents/runners/ambient-runner/ambient_runner/bridges/gemini_cli/session.pycomponents/runners/ambient-runner/ambient_runner/bridges/langgraph/bridge.pycomponents/runners/ambient-runner/ambient_runner/endpoints/feedback.pycomponents/runners/ambient-runner/ambient_runner/endpoints/run.pycomponents/runners/ambient-runner/ambient_runner/middleware/tracing.pycomponents/runners/ambient-runner/ambient_runner/observability_models.pycomponents/runners/ambient-runner/ambient_runner/platform/config.pycomponents/runners/ambient-runner/ambient_runner/platform/context.pycomponents/runners/ambient-runner/ambient_runner/platform/utils.pycomponents/runners/ambient-runner/docs/UPDATE_PROCEDURE.mdcomponents/runners/ambient-runner/pyproject.tomlcomponents/runners/ambient-runner/tests/test_bridge_claude.pycomponents/runners/ambient-runner/tests/test_e2e_api.pycomponents/runners/ambient-runner/tests/test_gemini_auth.pycomponents/runners/ambient-runner/tests/test_google_drive_e2e.pycomponents/runners/state-sync/Dockerfile
✅ Files skipped from review due to trivial changes (21)
- components/runners/ambient-runner/.mcp.json
- components/runners/ambient-runner/ambient_runner/platform/utils.py
- components/runners/ambient-runner/ambient_runner/middleware/tracing.py
- components/frontend/src/components/claude-agent-options/index.ts
- .specify/sdd-manifest.yaml
- components/runners/ambient-runner/ambient_runner/platform/config.py
- components/runners/ambient-runner/ambient_runner/bridges/gemini_cli/bridge.py
- components/runners/ambient-runner/ambient_runner/platform/context.py
- components/runners/ambient-runner/ambient_runner/bridges/gemini_cli/mcp.py
- components/runners/ambient-runner/ambient_runner/endpoints/feedback.py
- components/runners/ambient-runner/ambient_runner/bridges/gemini_cli/session.py
- components/runners/ambient-runner/ambient_runner/bridges/claude/session.py
- .specify/constitutions/runner.md
- components/runners/ambient-runner/ambient_runner/bridge.py
- components/runners/ambient-runner/ag_ui_gemini_cli/types.py
- components/runners/ambient-runner/ag_ui_claude_sdk/utils.py
- components/runners/ambient-runner/ag_ui_gemini_cli/adapter.py
- components/runners/ambient-runner/ambient_runner/endpoints/run.py
- .specify/specs/runner.md
- components/runners/ambient-runner/ambient_runner/observability_models.py
- components/runners/ambient-runner/ag_ui_claude_sdk/reasoning_events.py
🚧 Files skipped from review as they are similar to previous changes (2)
- components/frontend/src/types/api/sessions.ts
- components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py
| <Input className="font-mono text-xs flex-1" placeholder="matcher (e.g. Bash)" value={hook.matcher ?? ""} onChange={(e) => updateHook(event, i, { ...hook, matcher: e.target.value || null })} /> | ||
| <Input className="font-mono text-xs w-24" type="number" placeholder="timeout" value={hook.timeout ?? ""} onChange={(e) => updateHook(event, i, { ...hook, timeout: e.target.value ? Number(e.target.value) : undefined })} /> | ||
| <Button type="button" variant="ghost" size="icon" className="h-8 w-8 shrink-0" onClick={() => removeHook(event, i)}><Trash2 className="h-3 w-3" /></Button> |
There was a problem hiding this comment.
Guard timeout values before writing them to form state
Line 60 writes parsed numbers directly, so 0, negatives, and decimals can enter state even though the schema requires timeout >= 1. This can cause preventable submit failures.
Suggested fix
- <Input className="font-mono text-xs w-24" type="number" placeholder="timeout" value={hook.timeout ?? ""} onChange={(e) => updateHook(event, i, { ...hook, timeout: e.target.value ? Number(e.target.value) : undefined })} />
+ <Input
+ className="font-mono text-xs w-24"
+ type="number"
+ min={1}
+ step={1}
+ placeholder="timeout"
+ value={hook.timeout ?? ""}
+ onChange={(e) => {
+ const n = e.target.valueAsNumber;
+ updateHook(event, i, {
+ ...hook,
+ timeout: Number.isNaN(n) ? undefined : Math.max(1, Math.trunc(n)),
+ });
+ }}
+ />As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Input className="font-mono text-xs flex-1" placeholder="matcher (e.g. Bash)" value={hook.matcher ?? ""} onChange={(e) => updateHook(event, i, { ...hook, matcher: e.target.value || null })} /> | |
| <Input className="font-mono text-xs w-24" type="number" placeholder="timeout" value={hook.timeout ?? ""} onChange={(e) => updateHook(event, i, { ...hook, timeout: e.target.value ? Number(e.target.value) : undefined })} /> | |
| <Button type="button" variant="ghost" size="icon" className="h-8 w-8 shrink-0" onClick={() => removeHook(event, i)}><Trash2 className="h-3 w-3" /></Button> | |
| <Input className="font-mono text-xs flex-1" placeholder="matcher (e.g. Bash)" value={hook.matcher ?? ""} onChange={(e) => updateHook(event, i, { ...hook, matcher: e.target.value || null })} /> | |
| <Input | |
| className="font-mono text-xs w-24" | |
| type="number" | |
| min={1} | |
| step={1} | |
| placeholder="timeout" | |
| value={hook.timeout ?? ""} | |
| onChange={(e) => { | |
| const n = e.target.valueAsNumber; | |
| updateHook(event, i, { | |
| ...hook, | |
| timeout: Number.isNaN(n) ? undefined : Math.max(1, Math.trunc(n)), | |
| }); | |
| }} | |
| /> | |
| <Button type="button" variant="ghost" size="icon" className="h-8 w-8 shrink-0" onClick={() => removeHook(event, i)}><Trash2 className="h-3 w-3" /></Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/components/claude-agent-options/_components/hooks-editor.tsx`
around lines 59 - 61, The onChange for the timeout input currently writes
Number(e.target.value) directly into state allowing 0, negatives, and decimals
which violate the timeout >= 1 schema; update the handler in the timeout Input
so you parse the input (e.g. const v = Number(e.target.value)), check
Number.isFinite(v) and v >= 1, and if valid call updateHook(event, i, { ...hook,
timeout: Math.floor(v) }) (or leave undefined when the field is empty); if
invalid set timeout to undefined (or keep previous) to avoid storing values <1
or non-integers. Ensure you reference the timeout field on the hook and the
updateHook function when making this change.
| useEffect(() => { | ||
| const external = value ? JSON.stringify(value, null, 2) : ""; | ||
| if (external !== rawJson) setRawJson(external); | ||
| // Only react to value changes, not rawJson | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [value]); | ||
|
|
||
| const handleChange = (text: string) => { | ||
| setRawJson(text); | ||
| if (!text.trim()) { | ||
| setJsonError(null); | ||
| onChange(undefined); | ||
| return; | ||
| } | ||
| try { | ||
| onChange(JSON.parse(text)); | ||
| setJsonError(null); | ||
| } catch (e) { | ||
| setJsonError(e instanceof Error ? e.message : "Invalid JSON"); | ||
| onChange(undefined); | ||
| } |
There was a problem hiding this comment.
Do not clear form value on JSON parse failure
On parse failure at Line 46, onChange(undefined) can flip value to undefined, and then the sync effect (Line 28-29) resets rawJson to "". That can erase in-progress edits and silently remove output_format.
Suggested fix
const handleChange = (text: string) => {
setRawJson(text);
if (!text.trim()) {
setJsonError(null);
onChange(undefined);
return;
}
try {
onChange(JSON.parse(text));
setJsonError(null);
} catch (e) {
setJsonError(e instanceof Error ? e.message : "Invalid JSON");
- onChange(undefined);
+ // Keep last valid external value; only surface local parse error.
}
};As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const external = value ? JSON.stringify(value, null, 2) : ""; | |
| if (external !== rawJson) setRawJson(external); | |
| // Only react to value changes, not rawJson | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [value]); | |
| const handleChange = (text: string) => { | |
| setRawJson(text); | |
| if (!text.trim()) { | |
| setJsonError(null); | |
| onChange(undefined); | |
| return; | |
| } | |
| try { | |
| onChange(JSON.parse(text)); | |
| setJsonError(null); | |
| } catch (e) { | |
| setJsonError(e instanceof Error ? e.message : "Invalid JSON"); | |
| onChange(undefined); | |
| } | |
| useEffect(() => { | |
| const external = value ? JSON.stringify(value, null, 2) : ""; | |
| if (external !== rawJson) setRawJson(external); | |
| // Only react to value changes, not rawJson | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [value]); | |
| const handleChange = (text: string) => { | |
| setRawJson(text); | |
| if (!text.trim()) { | |
| setJsonError(null); | |
| onChange(undefined); | |
| return; | |
| } | |
| try { | |
| onChange(JSON.parse(text)); | |
| setJsonError(null); | |
| } catch (e) { | |
| setJsonError(e instanceof Error ? e.message : "Invalid JSON"); | |
| // Keep last valid external value; only surface local parse error. | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/components/claude-agent-options/_components/output-format-field.tsx`
around lines 27 - 47, The handler handleChange currently calls
onChange(undefined) on JSON parse failure which can flip value to undefined and
trigger the useEffect (watching value) to reset rawJson to "", erasing the
user's in-progress edits; remove the onChange(undefined) call in the catch block
and instead only call setJsonError(...) so parsing failures are flagged but the
external value is left unchanged (keep onChange(JSON.parse(text)) on success),
ensuring useEffect won't clear rawJson when the user types invalid JSON.
| export const permissionModeSchema = z.enum([ | ||
| "default", | ||
| "acceptEdits", | ||
| "plan", | ||
| "bypassPermissions", | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how permission_mode flows from form to API request
rg -n "permission_mode" --type=ts -C3 components/frontend/src/Repository: ambient-code/platform
Length of output: 4626
🏁 Script executed:
cat -n components/frontend/src/components/claude-agent-options/schema.ts | head -30Repository: ambient-code/platform
Length of output: 1295
🏁 Script executed:
rg -n "claudeAgentOptionsSchema" --type=ts -A 50 components/frontend/src/components/claude-agent-options/schema.ts | head -80Repository: ambient-code/platform
Length of output: 2492
🏁 Script executed:
rg -n "claudeAgentOptionsForm\|claudeAgentOptionsSchema" --type=tsx -B 5 -A 10 components/frontend/src/ | grep -A 15 "onSubmit\|mutation\|api\|request"Repository: ambient-code/platform
Length of output: 92
🏁 Script executed:
rg -n "permission_mode.*plan\|plan.*permission" --type=ts --type=tsx components/frontend/src/Repository: ambient-code/platform
Length of output: 92
🏁 Script executed:
# Check if "plan" appears anywhere in options-form or how the form is submitted
cat -n components/frontend/src/components/claude-agent-options/options-form.tsx | grep -A 30 "onSubmit\|useForm"Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
rg -n "claudeAgentOptions\|permission_mode" --type=ts -A 5 components/frontend/src/ | grep -B 5 -A 5 "submit\|mutation\|payload"Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Find all files in claude-agent-options directory
find components/frontend/src/components/claude-agent-options -type f -name "*.tsx" -o -name "*.ts"Repository: ambient-code/platform
Length of output: 1234
🏁 Script executed:
# Read the options-form file to see how it's submitted
wc -l components/frontend/src/components/claude-agent-options/options-form.tsxRepository: ambient-code/platform
Length of output: 141
🏁 Script executed:
cat -n components/frontend/src/components/claude-agent-options/options-form.tsx | head -100Repository: ambient-code/platform
Length of output: 4208
🏁 Script executed:
cat -n components/frontend/src/components/claude-agent-options/options-form.tsx | tail -150Repository: ambient-code/platform
Length of output: 6420
🏁 Script executed:
# Find where the form data is submitted/sent to API
rg -n "claudeAgentOptions\|permission_mode" --type=ts -B 3 -A 3 components/frontend/src/ | head -200Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look for API calls related to sessions or agent options
rg -n "sessions\|agent.*option" --type=ts -B 2 -A 2 components/frontend/src/lib/ components/frontend/src/api/Repository: ambient-code/platform
Length of output: 133
🏁 Script executed:
# Find where ClaudeAgentOptionsForm is used
rg -n "ClaudeAgentOptionsForm\|AgentOptionsFields" --type=ts components/frontend/src/Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look for API request building or session creation
rg -n "createSession\|updateSession\|SdkOptions\|permission_mode" --type=ts components/frontend/src/ | head -100Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check the index file to see what's exported
cat -n components/frontend/src/components/claude-agent-options/index.tsRepository: ambient-code/platform
Length of output: 276
🏁 Script executed:
# Search for where the form data is submitted or used
rg -n "AgentOptionsFields\|claudeAgentOptionsSchema" --type=ts -B 5 -A 15 components/frontend/src/ | head -150Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look for where ClaudeAgentOptionsForm data is passed to API
fd -e ts components/frontend/src/ | xargs grep -l "permission_mode" | head -10Repository: ambient-code/platform
Length of output: 466
🏁 Script executed:
# Check if there's any validation or transformation of permission_mode before API
rg -n "permission_mode.*plan\|plan.*permission\|transform\|convert" --type=ts components/frontend/src/ -B 2 -A 2Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Find files that import ClaudeAgentOptionsForm
rg -l "ClaudeAgentOptionsForm" components/frontend/src/Repository: ambient-code/platform
Length of output: 492
🏁 Script executed:
# Look for session creation or form submission handlers
rg -n "useForm\|onSubmit\|mutation" --type=ts components/frontend/src/ | grep -i "session\|agent" | head -20Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Search in lib directory for API helpers
find components/frontend/src -type d -name libRepository: ambient-code/platform
Length of output: 163
🏁 Script executed:
# Check the lib directory structure
ls -la components/frontend/src/lib/Repository: ambient-code/platform
Length of output: 1330
🏁 Script executed:
# Search for where permission_mode is mapped or filtered
rg -n "permission_mode" --type=ts components/frontend/src/lib/ -B 3 -A 3Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look in sessions lib
ls -la components/frontend/src/app/projects/*/sessions/*/lib 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 233
🏁 Script executed:
# Search for how options are passed to SDK
rg -n "SdkOptions\|permission_mode" --type=ts components/frontend/src/lib/ components/frontend/src/types/ -B 2 -A 2Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Find where session/agent creation happens
rg -n "createSession\|updateSession\|mutation\|POST\|PUT" --type=ts components/frontend/src/app/ | head -30Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look for the session creation page/component
find components/frontend/src/app -name "*session*" -type f | head -20Repository: ambient-code/platform
Length of output: 1497
🏁 Script executed:
# Check the sessions.ts type file more thoroughly
cat -n components/frontend/src/types/api/sessions.ts | head -150Repository: ambient-code/platform
Length of output: 4883
🏁 Script executed:
# Look for session-settings-modal which likely uses the form
cat -n components/frontend/src/app/projects/\[name\]/sessions/\[sessionName\]/components/session-settings-modal.tsx | head -150Repository: ambient-code/platform
Length of output: 5138
🏁 Script executed:
# Search for how AgentOptionsFields are used
rg -n "AgentOptionsFields" --type=ts -B 10 -A 20 components/frontend/src/app/Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look at scheduled-session-form which may show the flow
cat -n components/frontend/src/app/projects/\[name\]/scheduled-sessions/_components/scheduled-session-form.tsx | grep -A 30 -B 5 "permission_mode\|AgentOptions\|SdkOptions"Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Find the scheduled-session-form file
find components/frontend/src -name "*scheduled-session-form*"Repository: ambient-code/platform
Length of output: 166
🏁 Script executed:
# Search for where permissions or form data is converted to SdkOptions
rg -n "SdkOptions\|sdkOptions" --type=ts components/frontend/src/ -B 5 -A 5 | head -100Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look for any conversion or mapping functions
rg -n "toPlanOptions\|toSdkOptions\|mapOptions\|convertOptions" --type=ts components/frontend/src/Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look at scheduled-session-form
cat -n components/frontend/src/app/projects/\[name\]/scheduled-sessions/_components/scheduled-session-form.tsx | head -200Repository: ambient-code/platform
Length of output: 9668
🏁 Script executed:
# Search specifically for where form data is submitted
rg -n "onSubmit\|mutation\|submit" --type=ts components/frontend/src/app/projects/\[name\]/scheduled-sessions/ -B 3 -A 10Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check if there's a new-session-view that creates sessions
cat -n components/frontend/src/app/projects/\[name\]/sessions/\[sessionName\]/components/new-session-view.tsx | head -200Repository: ambient-code/platform
Length of output: 9024
🏁 Script executed:
# Look at AdvancedSdkOptions to see how permission_mode is handled
cat -n components/frontend/src/components/advanced-sdk-options.tsx | grep -B 20 -A 20 "permission_mode"Repository: ambient-code/platform
Length of output: 2447
🏁 Script executed:
# Search for where AdvancedSdkOptions is rendered and how it updates sdkOptions
rg -n "AdvancedSdkOptions" --type=ts -B 5 -A 10 components/frontend/src/Repository: ambient-code/platform
Length of output: 14911
🏁 Script executed:
# Check if scheduled-session-form uses AgentOptionsFields or permissionModeSchema
cat -n components/frontend/src/app/projects/\[name\]/scheduled-sessions/_components/scheduled-session-form.tsx | grep -i "agent\|permission\|sdk"Repository: ambient-code/platform
Length of output: 174
🏁 Script executed:
# Look for where session creation/API calls use the form data
rg -n "useCreateScheduledSession\|useCreateSession\|useUpdateScheduledSession" --type=ts -A 30 components/frontend/src/services/ | head -100Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check if permission_mode value validation happens before API submission
rg -n "permission_mode.*=\|permission_mode.*:" --type=ts components/frontend/src/ -B 2 -A 2 | grep -A 5 -B 5 "plan\|validate\|filter"Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check if claudeAgentOptionsSchema data is actually used when creating sessions
rg -n "claudeAgentOptions" --type=ts -B 5 -A 10 components/frontend/src/app/projects/Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Search for any validation or filtering of permission_mode before API
rg -n "permission_mode" --type=ts components/frontend/src/app/projects/ -B 3 -A 3 | head -80Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check sessions.ts more thoroughly to understand the mismatch
cat -n components/frontend/src/types/api/sessions.ts | grep -A 20 -B 5 "permission_mode"Repository: ambient-code/platform
Length of output: 985
Remove "plan" from permissionModeSchema—not supported by SdkOptions API type.
permissionModeSchema includes "plan" but SdkOptions.permission_mode only accepts "default" | "acceptEdits" | "bypassPermissions". The form renders this option (options-form.tsx:77), and while AdvancedSdkOptions (used in session creation) filters it out, the mismatch creates a type inconsistency. Align permissionModeSchema with the actual API type to prevent future bugs if this form data is used elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/claude-agent-options/schema.ts` around
lines 7 - 12, permissionModeSchema currently includes "plan", which is not
accepted by the SdkOptions.permission_mode type; remove "plan" from the z.enum
declaration for permissionModeSchema so it only lists "default", "acceptEdits",
and "bypassPermissions". Update any related usages that rely on the enum (e.g.,
the form option rendering code in options-form and any transformations in
AdvancedSdkOptions) to ensure they no longer expect or filter out "plan" so the
schema and SdkOptions.permission_mode remain consistent.
| # Install GitHub CLI and GitLab CLI (binary downloads, pinned) | ||
| RUN ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') && \ | ||
| curl -fsSL "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_${ARCH}.tar.gz" \ | ||
| | tar -xz -C /usr/local/bin --strip-components=2 "gh_${GH_VERSION}_linux_${ARCH}/bin/gh" && \ | ||
| curl -fsSL "https://gitlab.com/gitlab-org/cli/-/releases/v${GLAB_VERSION}/downloads/glab_${GLAB_VERSION}_linux_${ARCH}.tar.gz" \ | ||
| | tar -xz -C /usr/local/bin --strip-components=1 bin/glab && \ | ||
| chmod +x /usr/local/bin/gh /usr/local/bin/glab |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the complete Dockerfile
fd -t f "Dockerfile" | grep -i ambient-runnerRepository: ambient-code/platform
Length of output: 109
🏁 Script executed:
# Read the complete Dockerfile to see full context
cat "components/runners/ambient-runner/Dockerfile"Repository: ambient-code/platform
Length of output: 2933
🏁 Script executed:
# Search for any checksum or signature validation patterns in the file
rg -i "(sha256|gpg|signature|checksum|verify)" "components/runners/ambient-runner/Dockerfile"Repository: ambient-code/platform
Length of output: 178
Add checksum or signature verification for binary downloads.
The gh and glab releases are downloaded without validating checksums or signatures. Both projects provide these (GitHub CLI checksums at releases, GitLab CLI checksums/signatures), and verifying them before extraction mitigates supply-chain and MITM attack vectors. A compromised artifact would persist in the built image.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/Dockerfile` around lines 17 - 23, The RUN
block in the Dockerfile that downloads gh and glab (using GH_VERSION and
GLAB_VERSION) lacks checksum/signature verification; update that RUN step to
fetch the corresponding checksum or signature files from the official release
pages, verify the downloaded tarballs (e.g., with sha256sum or gpg --verify)
before extracting, and fail the build if verification fails; ensure the
verification uses the same ARCH and version variables (GH_VERSION, GLAB_VERSION)
and remove the tarball after successful verification/extraction to keep the
image clean.
| - **Base image major version upgrades** (UBI 9 → UBI 10) — require manual testing for compatibility | ||
| - **Breaking API changes** in dependencies — require human judgment on migration | ||
| - **Unleash flag creation** — the `advanced-agent-options` flag must be created in Unleash with tag `scope: workspace` | ||
|
|
There was a problem hiding this comment.
The Unleash flag name is wrong here.
This PR uses advanced-sdk-options, not advanced-agent-options. Following this procedure would create the wrong flag and leave the new session UI ungated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/docs/UPDATE_PROCEDURE.md` around lines 257
- 260, Update the Unleash flag name in the docs: replace the incorrect flag
string "advanced-agent-options" with the actual flag used by the PR,
"advanced-sdk-options", and ensure the instruction still notes the tag
requirement `scope: workspace` so the new session UI is correctly gated in
Unleash.
3 participants
Summary
advanced-sdk-optionsworkspace feature flag (disabled by default)create-session-dialog.tsx(dead code) and Type column from feature flags settingsData Flow
CodeRabbit Review Fixes (all 13 items addressed)
filterSdkOptionswith type validation + error returns,ClearSdkOptionsin UpdateSessionTest plan
npx tsc --noEmitpassesgo vet ./...+go build ./...passaction_required)Generated with Claude Code
Summary by CodeRabbit