feat: per-session environment variables for every tool#1547
Open
bassner wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds per-session environment variable support end-to-end: a new ChangesPer-session env vars
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
bassner
force-pushed
the
feat/session-launch-overrides
branch
from
65dab4c to
d017ef4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/home.go (1)
9411-9435: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply non-empty command overrides after the tool change.
The preflight checks the final tool, but
orderedChangesstill appliesFieldCommandOverridebeforeFieldTool. A same-submit edit fromshell→claudepluscommand-override=claudodexcan be rejected against the oldshelltool even though the final tool supports overrides.Proposed ordering fix
- orderedChanges := make([]Change, 0, len(changes)) + orderedChanges := make([]Change, 0, len(changes)) + var nonEmptyCommandOverrideChanges []Change for _, c := range changes { - if c.Field != session.FieldTool { + if c.Field == session.FieldCommandOverride && strings.TrimSpace(c.Value) != "" { + nonEmptyCommandOverrideChanges = append(nonEmptyCommandOverrideChanges, c) + continue + } + if c.Field != session.FieldTool { orderedChanges = append(orderedChanges, c) } } for _, c := range changes { if c.Field == session.FieldTool { orderedChanges = append(orderedChanges, c) } } + orderedChanges = append(orderedChanges, nonEmptyCommandOverrideChanges...)🤖 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 `@internal/ui/home.go` around lines 9411 - 9435, The change ordering in the session edit flow still applies FieldCommandOverride before FieldTool, which can make same-submit shell-to-claude edits fail against the old tool state. Update the ordering logic in the handler that builds orderedChanges so Tool is applied before any non-empty command override, while preserving the existing validation flow in editOverrideUnsupportedAfter and the final SetField application behavior.
🧹 Nitpick comments (7)
internal/session/storage_test.go (1)
12-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
LoadLitetoo.Both new round-trip tests only exercise
LoadWithGroups, butinternal/session/storage.goadded separateUnmarshalToolDatawiring inLoadLiteas well. A positional unpack regression there would still pass this suite while breakingLoadLiteconsumers in the CLI.Also applies to: 582-613
🤖 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 `@internal/session/storage_test.go` around lines 12 - 37, The new Env round-trip test only covers LoadWithGroups, but the same UnmarshalToolData path was added in LoadLite too, so a regression there would be missed. Extend the storage tests in storage_test.go by adding a LoadLite assertion for the same instance, using the existing TestInstanceEnvRoundTrip setup and checking that Instance.Env survives through LoadLite as well.internal/session/instance_test.go (1)
4595-4634: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an SSH-remote-path case here.
prepareCommandintentionally skips per-session env injection whenSSHRemotePathis set, but these tests only lock down the local path. A small remote-session case would pin that limitation and catch future quoting regressions. As per path instructions,internal/session/**: "RemoteSession vs LocalSession parity".🤖 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 `@internal/session/instance_test.go` around lines 4595 - 4634, The prepareCommand tests currently cover only local-session env injection, so add a remote-session case that sets SSHRemotePath and verifies per-session env is not injected in that mode. Extend the existing TestPrepareCommand_* coverage around prepareCommand/NewInstanceWithTool to assert the SSH-remote path keeps env out of the generated command, so future quoting or wrapping changes do not break the intended LocalSession vs RemoteSession behavior.Source: Path instructions
internal/statedb/migrate.go (1)
233-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a struct-based payload instead of growing positional parameter lists.
MarshalToolData/UnmarshalToolDatanow carry ~30 positional parameters/return values each. Every new field (this PR adds two) requires updating all call sites with carefully counted_placeholders — exactly the kind of error-prone pattern visible in the accompanying tests (multi_repo_test.go,session_persistence_test.go), where a single miscount would silently shift unrelated fields. AtoolDataBlob-shaped struct parameter/return (or a request/response struct) would eliminate this class of bug and make future additions additive rather than positional.Also applies to: 302-371
🤖 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 `@internal/statedb/migrate.go` around lines 233 - 298, The positional argument and return lists in MarshalToolData and UnmarshalToolData are too long and error-prone, so refactor them to use a struct-shaped payload based on toolDataBlob (or dedicated request/response structs). Update the MarshalToolData/UnmarshalToolData signatures and all call sites to pass named fields instead of counted placeholders, so adding future fields only requires extending the struct and avoids silent field shifts in tests like multi_repo_test and session_persistence_test.internal/session/command_override_review_fixes_test.go (1)
68-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining supported tools in the mutator matrix.
This test only exercises
claude,gemini,codex,cursor,pi, andcrush, but the PR also wires command overrides throughcopilot,hermes, andopencode. SinceSetField(FieldCommandOverride)is the shared gate used by CLI/TUI/web, a regression that rejects one of those tools would currently slip past the suite.🤖 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 `@internal/session/command_override_review_fixes_test.go` around lines 68 - 82, The SetField(FieldCommandOverride) coverage is missing several supported tools, so regressions for those paths can slip through. Update TestSetField_CommandOverride_AllowsSupportedTools to include copilot, hermes, and opencode alongside the existing tool list, using NewInstanceWithTool and SetField to verify the override is accepted and persisted for each. This keeps the shared command-override gate exercised across the full supported-tool matrix.internal/session/mutators_test.go (1)
188-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for tool changes clearing the override.
The new
FieldToolbranch also dropsinst.CommandOverridewhen the destination tool has no binary slot, but the added tests only cover set/clear/reject. A focusedclaude -> shellcase would lock down the cross-field invariant the CLI, TUI, and web paths now depend on.🤖 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 `@internal/session/mutators_test.go` around lines 188 - 236, Add a regression test that covers changing FieldTool from a binary-backed tool to a tool without a binary slot and verifies CommandOverride is cleared as part of SetField/FieldTool handling. Use NewInstanceWithTool and SetField in a focused claude-to-shell scenario, and assert the old override is returned and inst.CommandOverride becomes empty to lock down the cross-field behavior.cmd/agent-deck/command_override_cmd_test.go (1)
11-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
--helpcoverage for the new CLI surface.These subprocess tests exercise behavior well, but they do not protect the repo-required contract that
add,launch, andsession set --helpreflect the new command-override behavior. As per path instructions,cmd/agent-deck/**: CLI surface. Verify:--helpoutput reflects the new behavior.🤖 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 `@cmd/agent-deck/command_override_cmd_test.go` around lines 11 - 175, Add subprocess coverage for the new help text on the CLI entry points exercised here, since the current tests only verify behavior. Update the relevant command definitions behind add, launch, and session set so their --help output mentions command-override support, then add assertions in these tests (or nearby CLI help tests) that the help text reflects the new option/behavior. Use the existing runAgentDeck helper and reference the add, launch, and session set command paths so the help contract is protected.Source: Path instructions
internal/ui/newdialog_override_guard_test.go (1)
23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid coupling this test to preset ordering.
Find
claudeinpresetCommandsand assertValidate()returns an empty string; otherwise an unrelated validation error could pass this test.Proposed test hardening
- d.commandCursor = 1 // claude preset + foundClaude := false + for i, c := range d.presetCommands { + if c == "claude" { + d.commandCursor = i + foundClaude = true + break + } + } + if !foundClaude { + t.Fatal("claude preset not found") + } d.commandOverrideInput.SetValue("claudodex") - if msg := d.Validate(); strings.Contains(strings.ToLower(msg), "command override") { + if msg := d.Validate(); msg != "" { t.Fatalf("claude should accept override, got %q", msg) }🤖 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 `@internal/ui/newdialog_override_guard_test.go` around lines 23 - 34, Update the test in NewNewDialog/Validate to avoid relying on commandCursor preset ordering; instead locate the claude preset by searching presetCommands for the entry named claude and set commandCursor from that match before calling Validate(). Keep the existing assertion that Validate() returns an empty string when commandOverrideInput is set, so the test only passes for the intended claude preset and not an unrelated validation path.
🤖 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 `@cmd/agent-deck/launch_cmd.go`:
- Around line 119-129: The repeated `--env` handling in `handleLaunch` appends
duplicate key/value pairs instead of replacing an existing key, so align it with
the same upsert behavior used elsewhere. Update the `fs.Func("env", ...)` path
to normalize each parsed pair with `session.UpsertSessionEnv` (or the same
helper used by `handleAdd`) before storing it in `envFlags`, so later flags for
the same key overwrite earlier ones in place rather than creating duplicates.
Keep the validation via `session.ParseSessionEnvPair`, but ensure the final
persisted session env list contains only one entry per key.
In `@cmd/agent-deck/main.go`:
- Around line 1257-1268: The create path is preserving repeated --env entries
verbatim, which leaves duplicate keys in persisted session state; update the env
parsing flow in main.go to canonicalize the slice the same way the mutation path
does via UpsertSessionEnv semantics. In the fs.Func("env", ...) handler,
normalize each KEY=VALUE pair into a single entry per key so the last value wins
before envFlags is persisted and later consumed by prepareCommand. Apply the
same fix anywhere the create/update env slice is assembled so add and mutation
paths produce consistent, deduplicated session env state.
In `@cmd/agent-deck/session_cmd.go`:
- Around line 1364-1366: The help text for the session env setting is missing
the restart requirement, which can mislead users about when changes apply.
Update the `session set env` help line in `session_cmd.go` so it explicitly
states that per-session env changes only take effect after restart, matching the
wording style used by `command-override`. Keep the `--help` output aligned with
this behavior by modifying the same CLI help entry that documents `env`.
In `@internal/session/claude.go`:
- Around line 629-630: The Claude override check in GetToolCommandForInstance is
too strict because it compares the full override string against "claude", so
wrapper prefixes or extra args like environment assignments or flags still get
treated as non-Claude commands. Update the logic around inst.CommandOverride
handling to detect the actual launched binary token from the override before
deciding whether to suppress Claude config, and use that parsed token in the
existing CLAUDE_CONFIG_DIR gating path.
In `@internal/session/instance.go`:
- Around line 8375-8384: The SSHRemotePath branch in the session launch flow is
silently skipping i.Env, which allows remote sessions to start without required
per-session variables. Update the session-start logic around the env export
handling to either apply the exports through SSH with safe quoting in
wrapForSSH/buildSessionEnvExports or fail fast before launch when SSHRemotePath
is set and i.Env is non-empty, so RemoteSession behavior matches
LocalSession/ContainerSession parity.
In `@internal/ui/edit_session_dialog_test.go`:
- Around line 602-614: The no-op regression in
TestEditSessionDialog_GetChanges_EnvUntouchedNoOp does not exercise the
whitespace-sensitive case described in its comment. Update the fixture in this
test to seed a space-bearing env value through NewEditSessionDialog and
GetChanges, so envChangeValues still returns zero changes when an existing value
like FOO=bar baz is shown and saved without edits.
In `@internal/ui/edit_session_dialog.go`:
- Around line 97-103: The Env field in the edit session dialog currently
flattens values into a space-separated string, which breaks round-tripping when
an environment value contains spaces. Update the session env editor used by
editFieldEnv for session.FieldEnv so it uses the same multiline/newline-friendly
representation as NewDialog.GetSessionEnv, or add proper escaping/quoting before
parsing and re-rendering. Make sure the existing inst.Env value can be edited
and saved without splitting valid entries like MSG=hello world into invalid
tokens.
In `@internal/ui/newdialog.go`:
- Around line 2801-2828: The footer help in the new dialog does not reflect the
actual behavior when focusEnv is active, since Enter inserts a newline instead
of creating the item. Update the help text branch in newdialog.go where focusEnv
is handled so it shows an env-textarea-specific hint rather than the generic
“Enter create” message, keeping the displayed shortcut aligned with the envInput
and focusEnv behavior.
In `@internal/web/handlers_sessions.go`:
- Around line 425-431: The PATCH path in WebMutator.UpdateSession is serializing
req.Env with newline joins, but parseWebEnvList splits on the same delimiter, so
multiline env values cannot round-trip safely. Update the env handling around
validateSessionEnv, UpdateSession, and parseWebEnvList to either reject any
\r/\n in env values at both create and update boundaries, or change the
transport/encoding so values can be preserved losslessly without using raw
newlines as separators.
- Around line 439-445: Normalize session env entries consistently in
validateSessionEnv so nonblank values are trimmed before calling
session.ParseSessionEnvPair, matching the downstream web-mutator behavior.
Update the POST/PATCH handling path that stores session env values to persist
the same trimmed form, so inputs like " FOO=bar " are accepted and stored
consistently across create and update flows.
In `@internal/web/parity_test.go`:
- Around line 575-578: `session.FieldEnv` is still treated as a no-op in the
parity store, so PATCH env edits won’t be observable in `parity_test.go`. Update
the state handling in the parity store path around the `session.FieldEnv` case
so it actually mutates `sess.Env` (and any related session state) instead of
only assigning `oldValue = value`, and make the comparison reflect the real
before/after change. Ensure the `FieldEnv` branch in the parity harness verifies
env edits as a state change, consistent with the mutator parity behavior
exercised by the session update flow.
In `@internal/web/session_data_service.go`:
- Around line 112-115: The `MenuSession` payload is exposing plaintext session
env values through the web list/menu responses, which broadens disclosure
unnecessarily. Remove `Env` from the general `MenuSession` shape in
`session_data_service.go` and keep the env data only in a dedicated edit/detail
path (or replace it with a redacted summary) used by `EditSessionDialog`; update
the related serialization code around the `MenuSession`/session listing logic
accordingly.
In `@internal/web/static/app/EditSessionDialog.js`:
- Around line 34-36: Trim the commandOverride value in EditSessionDialog before
comparing it to original.commandOverride and before adding it to the PATCH
payload, so whitespace-only or trailing-space input does not produce a false
change or backend validation failure. Update the edit flow in the same place
that builds out.commandOverride to normalize the field like the create flow
does, using the existing form.commandOverride/original.commandOverride
comparison logic as the reference point.
---
Outside diff comments:
In `@internal/ui/home.go`:
- Around line 9411-9435: The change ordering in the session edit flow still
applies FieldCommandOverride before FieldTool, which can make same-submit
shell-to-claude edits fail against the old tool state. Update the ordering logic
in the handler that builds orderedChanges so Tool is applied before any
non-empty command override, while preserving the existing validation flow in
editOverrideUnsupportedAfter and the final SetField application behavior.
---
Nitpick comments:
In `@cmd/agent-deck/command_override_cmd_test.go`:
- Around line 11-175: Add subprocess coverage for the new help text on the CLI
entry points exercised here, since the current tests only verify behavior.
Update the relevant command definitions behind add, launch, and session set so
their --help output mentions command-override support, then add assertions in
these tests (or nearby CLI help tests) that the help text reflects the new
option/behavior. Use the existing runAgentDeck helper and reference the add,
launch, and session set command paths so the help contract is protected.
In `@internal/session/command_override_review_fixes_test.go`:
- Around line 68-82: The SetField(FieldCommandOverride) coverage is missing
several supported tools, so regressions for those paths can slip through. Update
TestSetField_CommandOverride_AllowsSupportedTools to include copilot, hermes,
and opencode alongside the existing tool list, using NewInstanceWithTool and
SetField to verify the override is accepted and persisted for each. This keeps
the shared command-override gate exercised across the full supported-tool
matrix.
In `@internal/session/instance_test.go`:
- Around line 4595-4634: The prepareCommand tests currently cover only
local-session env injection, so add a remote-session case that sets
SSHRemotePath and verifies per-session env is not injected in that mode. Extend
the existing TestPrepareCommand_* coverage around
prepareCommand/NewInstanceWithTool to assert the SSH-remote path keeps env out
of the generated command, so future quoting or wrapping changes do not break the
intended LocalSession vs RemoteSession behavior.
In `@internal/session/mutators_test.go`:
- Around line 188-236: Add a regression test that covers changing FieldTool from
a binary-backed tool to a tool without a binary slot and verifies
CommandOverride is cleared as part of SetField/FieldTool handling. Use
NewInstanceWithTool and SetField in a focused claude-to-shell scenario, and
assert the old override is returned and inst.CommandOverride becomes empty to
lock down the cross-field behavior.
In `@internal/session/storage_test.go`:
- Around line 12-37: The new Env round-trip test only covers LoadWithGroups, but
the same UnmarshalToolData path was added in LoadLite too, so a regression there
would be missed. Extend the storage tests in storage_test.go by adding a
LoadLite assertion for the same instance, using the existing
TestInstanceEnvRoundTrip setup and checking that Instance.Env survives through
LoadLite as well.
In `@internal/statedb/migrate.go`:
- Around line 233-298: The positional argument and return lists in
MarshalToolData and UnmarshalToolData are too long and error-prone, so refactor
them to use a struct-shaped payload based on toolDataBlob (or dedicated
request/response structs). Update the MarshalToolData/UnmarshalToolData
signatures and all call sites to pass named fields instead of counted
placeholders, so adding future fields only requires extending the struct and
avoids silent field shifts in tests like multi_repo_test and
session_persistence_test.
In `@internal/ui/newdialog_override_guard_test.go`:
- Around line 23-34: Update the test in NewNewDialog/Validate to avoid relying
on commandCursor preset ordering; instead locate the claude preset by searching
presetCommands for the entry named claude and set commandCursor from that match
before calling Validate(). Keep the existing assertion that Validate() returns
an empty string when commandOverrideInput is set, so the test only passes for
the intended claude preset and not an unrelated validation path.
🪄 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 Plus
Run ID: e4f1ae55-593b-4299-a3dd-53ed8b95c740
⛔ Files ignored due to path filters (8)
CHANGELOG.mdis excluded by!**/*.md,!CHANGELOG.mdREADME.mdis excluded by!**/*.mddocs/superpowers/plans/2026-06-29-per-session-launch-overrides.mdis excluded by!**/*.mddocs/superpowers/specs/2026-06-29-per-session-launch-overrides-design.mdis excluded by!**/*.mdskills/agent-deck/SKILL.mdis excluded by!**/*.mdskills/agent-deck/references/cli-reference.mdis excluded by!**/*.mdskills/agent-deck/references/config-reference.mdis excluded by!**/*.mdskills/agent-deck/references/tui-reference.mdis excluded by!**/*.md
📒 Files selected for processing (56)
cmd/agent-deck/command_override_cmd_test.gocmd/agent-deck/env_cmd_test.gocmd/agent-deck/launch_cmd.gocmd/agent-deck/main.gocmd/agent-deck/session_cmd.gocmd/agent-deck/web_cmd_test.gointernal/session/claude.gointernal/session/command_override_codex_test.gointernal/session/command_override_fork_test.gointernal/session/command_override_opencode_generic_test.gointernal/session/command_override_pi_cursor_crush_test.gointernal/session/command_override_review_fixes_test.gointernal/session/command_override_test.gointernal/session/copilot_hooks.gointernal/session/crush.gointernal/session/cursor_override_fix_test.gointernal/session/env.gointernal/session/env_precedence_test.gointernal/session/env_test.gointernal/session/fork_env_test.gointernal/session/hermes.gointernal/session/instance.gointernal/session/instance_test.gointernal/session/mutators.gointernal/session/mutators_test.gointernal/session/session_persistence_test.gointernal/session/storage.gointernal/session/storage_test.gointernal/statedb/migrate.gointernal/statedb/multi_repo_test.gointernal/tmux/redact_inline_test.gointernal/tmux/tmux.gointernal/ui/confirm_dialog.gointernal/ui/edit_session_dialog.gointernal/ui/edit_session_dialog_test.gointernal/ui/env_validation_fix_test.gointernal/ui/home.gointernal/ui/newdialog.gointernal/ui/newdialog_override_guard_test.gointernal/ui/newdialog_test.gointernal/ui/ordered_update_fields_test.gointernal/ui/remote_override_reject_test.gointernal/ui/web_mutator.gointernal/ui/web_mutator_rollback_test.gointernal/ui/web_mutator_test.gointernal/web/api_types.gointernal/web/command_override_create_validation_test.gointernal/web/handlers_sessions.gointernal/web/handlers_sessions_test.gointernal/web/parity_test.gointernal/web/server.gointernal/web/session_data_service.gointernal/web/static/app/CreateSessionDialog.jsinternal/web/static/app/EditSessionDialog.jsinternal/web/static_files_test.gotests/web/fixtures/cmd/web-fixture/main.go
Comment on lines
+8375
to
+8384
| // SSHRemotePath sessions are skipped: wrapForSSH re-escapes the whole command | ||
| // for the `cd '<path>' && <cmd>` remote shape, which mangles the inline | ||
| // `export KEY='value'` quoting. Per-session env is therefore not applied to | ||
| // remote-path SSH sessions (documented limitation); local and container | ||
| // sessions are unaffected. | ||
| if i.SSHRemotePath == "" { | ||
| if prefix := buildSessionEnvExports(i.Env); prefix != "" { | ||
| cmd = prefix + cmd | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t silently drop per-session env on SSH remote launches.
Lines 8380-8384 let the session start successfully while omitting i.Env whenever SSHRemotePath is set. Either apply the exports through SSH with correct quoting or fail before launch so users don’t get a remote session missing required env vars. As per path instructions, internal/session/**: “RemoteSession vs LocalSession parity”.
Proposed fail-fast guard
- if i.SSHRemotePath == "" {
- if prefix := buildSessionEnvExports(i.Env); prefix != "" {
- cmd = prefix + cmd
- }
+ if i.SSHRemotePath != "" && len(i.Env) > 0 {
+ return "", "", fmt.Errorf("per-session env is not supported for SSH remote-path sessions")
+ }
+ if prefix := buildSessionEnvExports(i.Env); prefix != "" {
+ cmd = prefix + cmd
}📝 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.
Suggested change
| // SSHRemotePath sessions are skipped: wrapForSSH re-escapes the whole command | |
| // for the `cd '<path>' && <cmd>` remote shape, which mangles the inline | |
| // `export KEY='value'` quoting. Per-session env is therefore not applied to | |
| // remote-path SSH sessions (documented limitation); local and container | |
| // sessions are unaffected. | |
| if i.SSHRemotePath == "" { | |
| if prefix := buildSessionEnvExports(i.Env); prefix != "" { | |
| cmd = prefix + cmd | |
| } | |
| } | |
| // SSHRemotePath sessions are skipped: wrapForSSH re-escapes the whole command | |
| // for the `cd '<path>' && <cmd>` remote shape, which mangles the inline | |
| // `export KEY='value'` quoting. Per-session env is therefore not applied to | |
| // remote-path SSH sessions (documented limitation); local and container | |
| // sessions are unaffected. | |
| if i.SSHRemotePath != "" && len(i.Env) > 0 { | |
| return "", "", fmt.Errorf("per-session env is not supported for SSH remote-path sessions") | |
| } | |
| if prefix := buildSessionEnvExports(i.Env); prefix != "" { | |
| cmd = prefix + cmd | |
| } |
🤖 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 `@internal/session/instance.go` around lines 8375 - 8384, The SSHRemotePath
branch in the session launch flow is silently skipping i.Env, which allows
remote sessions to start without required per-session variables. Update the
session-start logic around the env export handling to either apply the exports
through SSH with safe quoting in wrapForSSH/buildSessionEnvExports or fail fast
before launch when SSHRemotePath is set and i.Env is non-empty, so RemoteSession
behavior matches LocalSession/ContainerSession parity.
Source: Path instructions
Comment on lines
+439
to
+445
| func validateSessionEnv(env []string) error { | ||
| for _, kv := range env { | ||
| if strings.TrimSpace(kv) == "" { | ||
| continue | ||
| } | ||
| if _, _, err := session.ParseSessionEnvPair(kv); err != nil { | ||
| return err |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize surrounding whitespace consistently.
The downstream web-mutator parser trims each env line before validating it, but validateSessionEnv checks the raw string. As written, inputs like [" FOO=bar "] are rejected on POST/PATCH even though the update path is otherwise written to normalize them. Please trim nonblank entries here, and keep create-time storage in sync with the same rule.
🤖 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 `@internal/web/handlers_sessions.go` around lines 439 - 445, Normalize session
env entries consistently in validateSessionEnv so nonblank values are trimmed
before calling session.ParseSessionEnvPair, matching the downstream web-mutator
behavior. Update the POST/PATCH handling path that stores session env values to
persist the same trimmed form, so inputs like " FOO=bar " are accepted and
stored consistently across create and update flows.
Comment on lines
+112
to
+115
| // Env holds per-session environment variables ("KEY=VALUE") for every tool. | ||
| // Surfaced so the web EditSessionDialog can seed its env editor. Values are | ||
| // stored in plaintext at rest. | ||
| Env []string `json:"env,omitempty"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid returning plaintext session env in the general MenuSession payload.
MenuSession is what the web list/menu endpoints serialize, so adding Env here means every refresh now ships raw KEY=VALUE pairs for all sessions. For API tokens and similar values, that's a much larger disclosure surface than necessary just to prefill the edit dialog. Prefer a dedicated edit/detail fetch or a redacted shape for list responses.
Also applies to: 270-274
🤖 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 `@internal/web/session_data_service.go` around lines 112 - 115, The
`MenuSession` payload is exposing plaintext session env values through the web
list/menu responses, which broadens disclosure unnecessarily. Remove `Env` from
the general `MenuSession` shape in `session_data_service.go` and keep the env
data only in a dedicated edit/detail path (or replace it with a redacted
summary) used by `EditSessionDialog`; update the related serialization code
around the `MenuSession`/session listing logic accordingly.
Owner
|
🤖 autopilot: no CI checks found yet for this PR. Cannot review or merge until CI runs. Please push a commit or re-trigger CI so the checks can run. Generated by Claude Code |
bassner
added a commit
to bassner/agent-deck
that referenced
this pull request
Jul 1, 2026
…sheshgoplani#1547) Review-response round on the per-session launch-overrides PR. env: - `add`/`launch --env` upsert repeated keys (last wins) instead of appending duplicates - ban CR/LF in env values at the shared validator so the web newline-joined transport round-trips losslessly - trim entries on web create to match the PATCH path - collapse duplicate keys via a shared DedupeSessionEnv on every whole-list surface (web create/PATCH, TUI new-session) - reject env-set on SSH remote-path sessions at the mutator AND web PATCH, not just the proactive create surfaces command override: - token-aware claude-binary detection so `claude --flag` / `FOO=bar claude` keep CLAUDE_CONFIG_DIR (applied to the inline continue/resume/restart config-dir gates too) - split build-path routing (any override -> full treatment) from config-dir suppression (non-claude binary only) - trim command-override before diff on the TUI edit dialog (mirrors web) TUI edit dialog: - env field is now a multi-line textarea so values with spaces round-trip; ^S saves while it's focused, Enter inserts a newline, up/down move within it; canonical no-op guard avoids spurious restarts web: - keep env in MenuSession with a documented localhost/same-user tradeoff - FieldEnv parity test now applies changes observably help text / footer hints updated for env + command-override. New/updated unit tests cover every fix; go build, go vet, and golangci-lint (0 issues) clean. Cross-vendor reviewed (Codex gpt-5.5, xhigh) to explicit sign-off. Claude-Session: https://claude.ai/code/session_01EnuX6bBEpEtPsXfxJNvvNJ
bassner
added a commit
to bassner/agent-deck
that referenced
this pull request
Jul 1, 2026
…sheshgoplani#1547) Review-response round on the per-session launch-overrides PR. env: - `add`/`launch --env` upsert repeated keys (last wins) instead of appending duplicates - ban CR/LF in env values at the shared validator so the web newline-joined transport round-trips losslessly - trim entries on web create to match the PATCH path - collapse duplicate keys via a shared DedupeSessionEnv on every whole-list surface (web create/PATCH, TUI new-session) - reject env-set on SSH remote-path sessions at the mutator AND web PATCH, not just the proactive create surfaces command override: - token-aware claude-binary detection so `claude --flag` / `FOO=bar claude` keep CLAUDE_CONFIG_DIR (applied to the inline continue/resume/restart config-dir gates too) - split build-path routing (any override -> full treatment) from config-dir suppression (non-claude binary only) - trim command-override before diff on the TUI edit dialog (mirrors web) TUI edit dialog: - env field is now a multi-line textarea so values with spaces round-trip; ^S saves while it's focused, Enter inserts a newline, up/down move within it; canonical no-op guard avoids spurious restarts web: - keep env in MenuSession with a documented localhost/same-user tradeoff - FieldEnv parity test now applies changes observably help text / footer hints updated for env + command-override. New/updated unit tests cover every fix; go build, go vet, and golangci-lint (0 issues) clean. Cross-vendor reviewed (Codex gpt-5.5, xhigh) to explicit sign-off. Claude-Session: https://claude.ai/code/session_01EnuX6bBEpEtPsXfxJNvvNJ
bassner
force-pushed
the
feat/session-launch-overrides
branch
from
1b7e1ad to
3f257ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/session/claude.go`:
- Around line 553-556: The Claude-compatible branch is routing all compatible
tools through GetClaudeCommandForInstance(), which makes custom tools lose their
own default command when CommandOverride is empty. Update the switch handling in
claude.go so only the built-in “claude” case uses GetClaudeCommandForInstance(),
while custom Claude-compatible tools keep their configured/default binary path.
Keep the existing compatibility checks via IsClaudeCompatible and ensure
start/restart/fork still resolve commands from the instance-specific config for
custom tools.
In `@internal/session/env.go`:
- Around line 146-148: The session env export clause is being appended as a raw
semicolon-separated string in the env assembly logic, which breaks the intended
fail-fast chaining with `&&`. Update the `internal/session/env.go` flow around
`buildSessionEnvExports` so multi-export session envs are wrapped as a single
grouped shell clause before being added to `sources`, ensuring the whole export
block is treated atomically with the surrounding `&&` chain.
In `@internal/ui/newdialog_test.go`:
- Around line 2536-2576: Add coverage for the remote-session create path in the
NewDialog tests, since the TUI launch controls now reject env and command
override inputs for remote sessions. Update the relevant tests around
NewNewDialog, GetSessionEnv, and GetSessionCommandOverride to either exercise
RemoteSession/remote-create behavior and assert the inputs are ignored/rejected
there, or add an explicit t.Skip in this test file explaining why remote-session
TUI coverage is out of scope.
In `@internal/web/static/app/CreateSessionDialog.js`:
- Around line 126-129: The create flow in CreateSessionDialog is still sending
commandOverride even when the selected tool is unsupported, so align the web UI
with the TUI by blocking that state before submit. Update the payload-building
logic around the envRowsToList/commandOverride handling to suppress or clear
commandOverride when the active tool is shell or aider, and disable the command
override input whenever those tools are selected so users cannot enter an
invalid combination. Make the same state/validation adjustment in the related
create-session flow referenced by the same dialog to keep mutator parity.
In `@internal/web/static/app/EditSessionDialog.js`:
- Around line 34-39: Clear the stale command override when the selected tool no
longer supports it: in EditSessionDialog.js, update diffUpdates to derive
commandOverride from form.tool, not just form.commandOverride, so unsupported
tools like shell or aider send commandOverride: ''. Also disable the
commandOverride input for unsupported tools to match the TUI mutator behavior
and prevent backend validation failures.
🪄 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 Plus
Run ID: 686a3042-e172-47d7-8823-c8e09e8d1a0c
⛔ Files ignored due to path filters (8)
CHANGELOG.mdis excluded by!**/*.md,!CHANGELOG.mdREADME.mdis excluded by!**/*.mddocs/superpowers/plans/2026-06-29-per-session-launch-overrides.mdis excluded by!**/*.mddocs/superpowers/specs/2026-06-29-per-session-launch-overrides-design.mdis excluded by!**/*.mdskills/agent-deck/SKILL.mdis excluded by!**/*.mdskills/agent-deck/references/cli-reference.mdis excluded by!**/*.mdskills/agent-deck/references/config-reference.mdis excluded by!**/*.mdskills/agent-deck/references/tui-reference.mdis excluded by!**/*.md
📒 Files selected for processing (56)
cmd/agent-deck/command_override_cmd_test.gocmd/agent-deck/env_cmd_test.gocmd/agent-deck/launch_cmd.gocmd/agent-deck/main.gocmd/agent-deck/session_cmd.gocmd/agent-deck/web_cmd_test.gointernal/session/claude.gointernal/session/command_override_codex_test.gointernal/session/command_override_fork_test.gointernal/session/command_override_opencode_generic_test.gointernal/session/command_override_pi_cursor_crush_test.gointernal/session/command_override_review_fixes_test.gointernal/session/command_override_test.gointernal/session/copilot_hooks.gointernal/session/crush.gointernal/session/cursor_override_fix_test.gointernal/session/env.gointernal/session/env_precedence_test.gointernal/session/env_test.gointernal/session/fork_env_test.gointernal/session/hermes.gointernal/session/instance.gointernal/session/instance_test.gointernal/session/mutators.gointernal/session/mutators_test.gointernal/session/session_persistence_test.gointernal/session/storage.gointernal/session/storage_test.gointernal/statedb/migrate.gointernal/statedb/multi_repo_test.gointernal/tmux/redact_inline_test.gointernal/tmux/tmux.gointernal/ui/confirm_dialog.gointernal/ui/edit_session_dialog.gointernal/ui/edit_session_dialog_test.gointernal/ui/env_validation_fix_test.gointernal/ui/home.gointernal/ui/newdialog.gointernal/ui/newdialog_override_guard_test.gointernal/ui/newdialog_test.gointernal/ui/ordered_update_fields_test.gointernal/ui/remote_override_reject_test.gointernal/ui/web_mutator.gointernal/ui/web_mutator_rollback_test.gointernal/ui/web_mutator_test.gointernal/web/api_types.gointernal/web/command_override_create_validation_test.gointernal/web/handlers_sessions.gointernal/web/handlers_sessions_test.gointernal/web/parity_test.gointernal/web/server.gointernal/web/session_data_service.gointernal/web/static/app/CreateSessionDialog.jsinternal/web/static/app/EditSessionDialog.jsinternal/web/static_files_test.gotests/web/fixtures/cmd/web-fixture/main.go
✅ Files skipped from review due to trivial changes (3)
- internal/ui/web_mutator_test.go
- internal/tmux/redact_inline_test.go
- cmd/agent-deck/session_cmd.go
🚧 Files skipped from review as they are similar to previous changes (40)
- internal/session/command_override_codex_test.go
- internal/ui/env_validation_fix_test.go
- internal/ui/remote_override_reject_test.go
- tests/web/fixtures/cmd/web-fixture/main.go
- internal/session/copilot_hooks.go
- internal/web/server.go
- internal/session/crush.go
- internal/session/fork_env_test.go
- internal/statedb/multi_repo_test.go
- internal/session/hermes.go
- internal/web/static_files_test.go
- internal/session/env_precedence_test.go
- internal/session/command_override_opencode_generic_test.go
- internal/session/cursor_override_fix_test.go
- internal/ui/ordered_update_fields_test.go
- internal/web/session_data_service.go
- cmd/agent-deck/env_cmd_test.go
- internal/session/session_persistence_test.go
- internal/ui/newdialog_override_guard_test.go
- internal/session/env_test.go
- internal/web/api_types.go
- internal/session/storage.go
- cmd/agent-deck/web_cmd_test.go
- internal/session/mutators_test.go
- internal/ui/confirm_dialog.go
- internal/session/command_override_pi_cursor_crush_test.go
- cmd/agent-deck/launch_cmd.go
- cmd/agent-deck/command_override_cmd_test.go
- internal/session/instance_test.go
- internal/session/storage_test.go
- internal/tmux/tmux.go
- internal/session/command_override_review_fixes_test.go
- internal/web/handlers_sessions.go
- cmd/agent-deck/main.go
- internal/session/mutators.go
- internal/web/handlers_sessions_test.go
- internal/ui/web_mutator.go
- internal/web/parity_test.go
- internal/ui/newdialog.go
- internal/session/instance.go
Comment on lines
+553
to
+556
| switch { | ||
| case inst.Tool == "claude" || IsClaudeCompatible(inst.Tool): | ||
| return GetClaudeCommandForInstance(inst) // conductor/group/global chain | ||
| case IsCodexCompatible(inst.Tool): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep Claude-compatible custom tools on their own default binary.
This branch collapses every Claude-compatible tool to GetClaudeCommandForInstance(). For custom compatible tools, that changes the no-override start/restart/fork path from the tool’s configured command to the global Claude binary/alias whenever CommandOverride is empty.
💡 Suggested fix
func defaultToolCommand(inst *Instance) string {
switch {
- case inst.Tool == "claude" || IsClaudeCompatible(inst.Tool):
+ case inst.Tool == "claude":
return GetClaudeCommandForInstance(inst) // conductor/group/global chain
+ case IsClaudeCompatible(inst.Tool):
+ if cmd := strings.TrimSpace(inst.Command); cmd != "" {
+ return cmd
+ }
+ if td := GetToolDef(inst.Tool); td != nil && strings.TrimSpace(td.Command) != "" {
+ return strings.TrimSpace(td.Command)
+ }
+ return GetClaudeCommandForInstance(inst)
case IsCodexCompatible(inst.Tool):
return inst.resolveCodexCommand(inst.Command) // METHOD, mirrors dispatch📝 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.
Suggested change
| switch { | |
| case inst.Tool == "claude" || IsClaudeCompatible(inst.Tool): | |
| return GetClaudeCommandForInstance(inst) // conductor/group/global chain | |
| case IsCodexCompatible(inst.Tool): | |
| switch { | |
| case inst.Tool == "claude": | |
| return GetClaudeCommandForInstance(inst) // conductor/group/global chain | |
| case IsClaudeCompatible(inst.Tool): | |
| if cmd := strings.TrimSpace(inst.Command); cmd != "" { | |
| return cmd | |
| } | |
| if td := GetToolDef(inst.Tool); td != nil && strings.TrimSpace(td.Command) != "" { | |
| return strings.TrimSpace(td.Command) | |
| } | |
| return GetClaudeCommandForInstance(inst) | |
| case IsCodexCompatible(inst.Tool): |
🤖 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 `@internal/session/claude.go` around lines 553 - 556, The Claude-compatible
branch is routing all compatible tools through GetClaudeCommandForInstance(),
which makes custom tools lose their own default command when CommandOverride is
empty. Update the switch handling in claude.go so only the built-in “claude”
case uses GetClaudeCommandForInstance(), while custom Claude-compatible tools
keep their configured/default binary path. Keep the existing compatibility
checks via IsClaudeCompatible and ensure start/restart/fork still resolve
commands from the instance-specific config for custom tools.
Comment on lines
+2536
to
+2576
| func TestNewDialog_GetSessionEnv(t *testing.T) { | ||
| d := NewNewDialog() | ||
| d.Show() | ||
| d.envInput.SetValue("FOO=bar\nBAZ=qux") | ||
|
|
||
| got := d.GetSessionEnv() | ||
| want := []string{"FOO=bar", "BAZ=qux"} | ||
| if !reflect.DeepEqual(got, want) { | ||
| t.Errorf("GetSessionEnv() = %v, want %v", got, want) | ||
| } | ||
| } | ||
|
|
||
| func TestNewDialog_GetSessionEnv_SkipsInvalidAndBlankLines(t *testing.T) { | ||
| d := NewNewDialog() | ||
| d.Show() | ||
| // "1BAD" is an invalid env key (leading digit); blank lines are dropped; | ||
| // values may contain '=' (only the first splits). | ||
| d.envInput.SetValue("FOO=bar\n\n1BAD=x\nURL=https://a.b?x=1") | ||
|
|
||
| got := d.GetSessionEnv() | ||
| want := []string{"FOO=bar", "URL=https://a.b?x=1"} | ||
| if !reflect.DeepEqual(got, want) { | ||
| t.Errorf("GetSessionEnv() = %v, want %v", got, want) | ||
| } | ||
| } | ||
|
|
||
| // GetSessionCommandOverride returns the trimmed per-session binary override; a | ||
| // blank/whitespace-only field yields "" (the created instance keeps no override). | ||
| func TestNewDialog_GetSessionCommandOverride(t *testing.T) { | ||
| d := NewNewDialog() | ||
| d.Show() | ||
| d.commandOverrideInput.SetValue(" claudodex ") | ||
| if got := d.GetSessionCommandOverride(); got != "claudodex" { | ||
| t.Errorf("GetSessionCommandOverride() = %q, want %q", got, "claudodex") | ||
| } | ||
|
|
||
| d.commandOverrideInput.SetValue(" ") | ||
| if got := d.GetSessionCommandOverride(); got != "" { | ||
| t.Errorf("GetSessionCommandOverride() = %q, want empty", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add remote-session coverage or an explicit skip for these TUI launch controls.
These tests cover the local dialog getters, but the PR also changes TUI create behavior for remote sessions by rejecting env/command override input. Please add a RemoteSession/remote-create test for that path, or an explicit t.Skip documenting why it is out of scope. As per coding guidelines, internal/ui/**/*_test.go changes that affect TUI behavior should verify RemoteSession handling or include an explicit t.Skip documenting why.
🤖 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 `@internal/ui/newdialog_test.go` around lines 2536 - 2576, Add coverage for the
remote-session create path in the NewDialog tests, since the TUI launch controls
now reject env and command override inputs for remote sessions. Update the
relevant tests around NewNewDialog, GetSessionEnv, and GetSessionCommandOverride
to either exercise RemoteSession/remote-create behavior and assert the inputs
are ignored/rejected there, or add an explicit t.Skip in this test file
explaining why remote-session TUI coverage is out of scope.
Source: Coding guidelines
Comment on lines
+126
to
+129
| const env = envRowsToList(envRows) | ||
| if (env.length) payload.env = env | ||
| const co = commandOverride.trim() | ||
| if (co) payload.commandOverride = co |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Block unsupported command overrides in the create flow.
This still posts commandOverride for every tool. If a user types an override and then switches to shell or aider, the create request is guaranteed to fail on backend validation instead of preventing the invalid combination in the UI. Please suppress or clear the field for unsupported tools, and disable the input while those tools are selected.
Suggested fix
+const COMMAND_OVERRIDE_UNSUPPORTED_TOOLS = new Set(['shell', 'aider'])
+
export function CreateSessionDialog() {
const [title, setTitle] = useState('')
const [tool, setTool] = useState('claude')
@@
function selectTool(nextTool) {
setTool(nextTool)
setModelId('')
setCustomModel('')
+ if (COMMAND_OVERRIDE_UNSUPPORTED_TOOLS.has(nextTool)) setCommandOverride('')
}
@@
const shownTools = resolveCreateSessionPickerTools(pickerToolsSignal.value)
const needsCustomModel = modelId === CUSTOM_MODEL
+ const commandOverrideSupported = !COMMAND_OVERRIDE_UNSUPPORTED_TOOLS.has(tool)
const submitDisabled = submitting || !title || !path || (needsCustomModel && !customModel.trim())
@@
- const co = commandOverride.trim()
+ const co = commandOverrideSupported ? commandOverride.trim() : ''
if (co) payload.commandOverride = co
@@
- <input value=${commandOverride} onInput=${e => setCommandOverride(e.target.value)}
+ <input value=${commandOverride}
+ onInput=${e => setCommandOverride(e.target.value)}
+ disabled=${!commandOverrideSupported}
placeholder=${commandOverrideHint(tool)}/>As per path instructions, "mutator parity with TUI (any state change in TUI must work in Web too)."
Also applies to: 232-239
🤖 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 `@internal/web/static/app/CreateSessionDialog.js` around lines 126 - 129, The
create flow in CreateSessionDialog is still sending commandOverride even when
the selected tool is unsupported, so align the web UI with the TUI by blocking
that state before submit. Update the payload-building logic around the
envRowsToList/commandOverride handling to suppress or clear commandOverride when
the active tool is shell or aider, and disable the command override input
whenever those tools are selected so users cannot enter an invalid combination.
Make the same state/validation adjustment in the related create-session flow
referenced by the same dialog to keep mutator parity.
Source: Path instructions
Comment on lines
+34
to
+39
| // Per-session command override applies to every tool (restart-required). Trim | ||
| // before diffing so whitespace-only or padded input matches the create dialog | ||
| // (which trims before POST) and never becomes a spurious update that the | ||
| // backend allow-list validator would then reject. | ||
| const nextCommandOverride = form.commandOverride.trim() | ||
| if (nextCommandOverride !== (original.commandOverride || '')) out.commandOverride = nextCommandOverride |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear commandOverride when the edited tool no longer supports it.
diffUpdates still derives the override from the raw field, not from form.tool. If a session already has an override and the user switches the tool to shell or aider, this PATCH will leave the stale override attached unless the user manually deletes it first, so the save can fail once the backend validates the final state. Treat unsupported tools as commandOverride: '' and disable the input for them.
Suggested fix
- const nextCommandOverride = form.commandOverride.trim()
+ const nextCommandOverride =
+ form.tool === 'shell' || form.tool === 'aider'
+ ? ''
+ : form.commandOverride.trim()
if (nextCommandOverride !== (original.commandOverride || '')) out.commandOverride = nextCommandOverrideAs per path instructions, "mutator parity with TUI (any state change in TUI must work in Web too)."
Also applies to: 197-205
🤖 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 `@internal/web/static/app/EditSessionDialog.js` around lines 34 - 39, Clear the
stale command override when the selected tool no longer supports it: in
EditSessionDialog.js, update diffUpdates to derive commandOverride from
form.tool, not just form.commandOverride, so unsupported tools like shell or
aider send commandOverride: ''. Also disable the commandOverride input for
unsupported tools to match the TUI mutator behavior and prevent backend
validation failures.
Source: Path instructions
…#1547) Attach `KEY=VALUE` environment variables to a single session — at create (`add`/`launch --env`, the TUI/web new-session dialogs) or later (`session set <id> env KEY=VALUE`, the edit dialogs) — exported into the spawned process for any tool (claude, gemini, codex, opencode, copilot, hermes, crush, pi, cursor, custom). Per-session env is the most specific scope, so it WINS on a key collision: it is exported after the config env layers (`env_file`/inline/group/ conductor) and the login shell, and before Docker/SSH wrapping. Injected at the universal `prepareCommand` chokepoint plus, for builder paths, as a grouped `{ …; }` clause in `buildEnvSourceCommand` (so a bare `;` can't break the `&&` fail-fast chain, and exports persist in the current shell). Inherited by forks (copied, not aliased); skipped for remote-path SSH (rejected proactively at every create surface and the mutator/web PATCH). Safety: keys validated (`[A-Za-z_][A-Za-z0-9_]*`), values shell-escaped, CR/LF rejected, duplicate keys deduped (last wins) on whole-list surfaces; the restart/respawn command is suppressed from logs when a session carries env (keys only), and tmux inline `export` values are redacted (incl. shell-escaped quotes). Stored plaintext in the session DB `tool_data` blob (no schema migration), documented as such. Note: an earlier revision of this PR also proposed a per-session *command override*. That was dropped as redundant — agent-deck already ships a per-tool/per-group/per-conductor `[claude].command` + claude-compatible `[tools.*]` mechanism (asheshgoplani#1483/asheshgoplani#1044) that runs a wrapper like claudodex side-by-side with normal Claude and handles CLAUDE_CONFIG_DIR suppression. The README and config reference now document that path. Cross-vendor reviewed (Codex gpt-5.5, xhigh) to explicit sign-off. Claude-Session: https://claude.ai/code/session_01EnuX6bBEpEtPsXfxJNvvNJ
bassner
changed the title
bassner
force-pushed
the
feat/session-launch-overrides
branch
from
bb1ac74 to
97d4dd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/agent-deck/main.go (1)
1962-1983: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
list --jsonexposes raw per-session env values (including any secrets) unredacted.
sessionJSON.Envis populated directly frominst.Env, soagent-deck list --jsonprints fullKEY=VALUEpairs verbatim. The PR's own design already treats per-session env as sensitive enough to warrant a plaintext-storage warning at creation time and a dedicated redaction layer for tmux exports/command logging (per the cohort'sloggableCommand/tmux redaction work), but this CLI-facing JSON output has no equivalent safeguard. Output fromlist --jsonis commonly redirected to files, piped into scripts/CI logs, or pasted into bug reports, all of which increase exposure of values users were warned "you can't rotate."Consider redacting values (e.g., emit only keys, or mask as
KEY=***) by default, gated behind an explicit opt-in flag (e.g.,--reveal-env) for callers that truly need the values.💡 Possible fix: expose keys only by default
SSHHost: inst.SSHHost, SSHRemotePath: inst.SSHRemotePath, Channels: inst.Channels, ExtraArgs: inst.ExtraArgs, - Env: inst.Env, + Env: session.RedactedSessionEnvKeys(inst.Env), // keys only; use --reveal-env for values Color: inst.Color,Also applies to: 1988-2007
🤖 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 `@cmd/agent-deck/main.go` around lines 1962 - 1983, `list --json` is leaking raw per-session environment values through `sessionJSON.Env`, so update the JSON rendering in `main.go` to redact sensitive env entries by default. Adjust the code that populates `sessionJSON` from `inst.Env` so it emits only keys or masked values (for example via a helper used by the list output), and add an explicit opt-in flag such as `--reveal-env` for callers that need the full `KEY=VALUE` pairs. Use the existing `sessionJSON` struct and the list/JSON serialization path as the place to apply the redaction.
🧹 Nitpick comments (5)
cmd/agent-deck/main.go (1)
1716-1730: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralizing the SSH remote-path env guard. The same
SSHRemotePath != "" && env != ""rule is enforced ininternal/session/mutators.goandinternal/ui/web_mutator.goas well, so keeping another inline check here risks drift if the rule changes later.🤖 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 `@cmd/agent-deck/main.go` around lines 1716 - 1730, The SSH remote-path plus per-session env validation is duplicated here, alongside the same rule already enforced in the session mutators and web mutator paths, so centralize it into one shared guard instead of keeping another inline check. Update the creation flow around newInstance.SSHRemotePath and newInstance.Env to call the same validation logic used by internal/session/mutators.go and internal/ui/web_mutator.go, and keep the error handling/message consistent across all SSH session entry points.internal/tmux/redact_inline_test.go (1)
29-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
strings.Containsinstead of a hand-rolled substring search.♻️ Proposed simplification
-func contains(s, sub string) bool { - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - return true - } - } - return false -} +func contains(s, sub string) bool { + return strings.Contains(s, sub) +}(Add
"strings"to the import block.)🤖 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 `@internal/tmux/redact_inline_test.go` around lines 29 - 36, The helper contains currently reimplements substring matching manually; replace that logic with strings.Contains and add strings to the import block. Update the contains function in redact_inline_test.go to delegate to the standard library so the test helper is simpler and consistent.internal/session/mutators_test.go (1)
10-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the SSH remote-path rejection branch.
This test exercises upsert/unset/invalid-key paths but not the new
inst.SSHRemotePath != ""guard inSetField(FieldEnv, ...)(mutators.go lines 293-302), which is the mechanism that now enforces RemoteSession/LocalSession env parity. Given this guard is exactly what closes the gap from the previously-flagged "silently dropped env on SSH sessions" issue, a regression here would silently reopen that bug.✅ Suggested additional case
func TestSetField_Env_RejectedForSSHRemotePath(t *testing.T) { inst := NewInstanceWithTool("s", t.TempDir(), "claude") inst.SSHRemotePath = "/remote/path" if _, _, err := SetField(inst, FieldEnv, "FOO=bar", nil); err == nil { t.Fatal("expected rejection for SSH remote-path session") } // Clearing must still be allowed. inst.Env = []string{"FOO=bar"} if _, _, err := SetField(inst, FieldEnv, "FOO=", nil); err != nil { t.Fatalf("unset should still be allowed: %v", err) } }🤖 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 `@internal/session/mutators_test.go` around lines 10 - 33, Add test coverage for the new SSH remote-path rejection in SetField for FieldEnv. Extend internal/session/mutators_test.go with a case around SetField and the SSHRemotePath guard so that when inst.SSHRemotePath is set, setting a non-empty env value is rejected, while the unset path still succeeds. Use the existing TestSetField_EnvUpsertUnsetAndValidation and SetField/FieldEnv symbols to place the new regression coverage near the current env tests.internal/web/parity_test.go (1)
32-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo end-to-end parity case exercises PATCH
env.The
FieldEnvno-op bug inparityStore.UpdateSessionis now fixed, but thecasestable inTestParity_WebActionMatchesDirectMutatorhas noupdate_env-style case that actually PATCHes env through the web HTTP path vs. the direct mutator (unlikecreate_session,stop_session, etc.). Without it, a future regression in the web PATCH env-encoding path (e.g.handlers_sessions.go's newline-join, orWebMutator.UpdateSession'sparseWebEnvList) wouldn't be caught by this parity suite.Also applies to: 229-325
🤖 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 `@internal/web/parity_test.go` around lines 32 - 41, Add an end-to-end parity case to TestParity_WebActionMatchesDirectMutator that exercises PATCH env via both the web HTTP path and the direct mutator path. Extend the cases table with an update_env-style entry that uses parityFixture and compares the changed session after calling WebMutator.UpdateSession against the direct UpdateSession flow, so regressions in handlers_sessions.go’s env encoding or WebMutator.parseWebEnvList are covered. Ensure the new case targets the existing parity helpers and asserts the env change on the same session record.internal/ui/edit_session_dialog_test.go (1)
637-652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a duplicate-key regression case.
Once
GetChangesdedupes (see companion comment inedit_session_dialog.go), a test likesetEnvField(t, d, "FOO=bar\nFOO=baz")asserting onlyFOO=bazsurvives would guard the fix and prevent regressing to duplicate-preserving behavior.🤖 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 `@internal/ui/edit_session_dialog_test.go` around lines 637 - 652, Add a regression test in TestEditSessionDialog_GetChanges_EnvUpserts that covers duplicate env keys in the editor input, using EditSessionDialog and GetChanges to verify only the last value for a repeated key is kept. Populate the env field with duplicate entries such as the same key appearing twice, then assert envChangeValues/GetChanges returns a single whole-list FieldEnv change whose content preserves only the final assignment, preventing duplicate-preserving behavior from returning.
🤖 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 `@internal/ui/edit_session_dialog.go`:
- Around line 303-337: The editFieldEnv path in GetChanges currently trims and
filters lines but still lets duplicate env keys survive into the whole-list
Change, so canonicalize the textarea using the same per-key deduping behavior as
NewDialog.GetSessionEnv and mutators.SetField’s FieldEnv handling. Update the
editFieldEnv branch to collapse duplicate keys with last-wins semantics before
comparing/joining and appending the session.FieldEnv Change, so inst.Env is
stored uniquely and matches the existing env canonicalization rules.
In `@internal/ui/home.go`:
- Around line 9380-9468: The edit-session commit path can partially mutate the
shared Instance before Env validation fails, leaving in-memory changes unsaved.
In commitEditSessionDialog, validate the FieldEnv change up front (or otherwise
defer all SetField mutations until after parseWebEnvList and the SSH-remote-path
check succeed) so that no Title/Pin/other changes are applied if Env is invalid.
Keep the atomic behavior aligned with WebMutator.UpdateSession by ensuring any
early error exits from commitEditSessionDialog happen before mutating inst and
before forceSaveInstances/rebuildFlatItems.
In `@internal/ui/web_mutator.go`:
- Around line 508-541: The update handling in web_mutator currently applies
`FieldTool` too late, so tool-dependent fields like `plugins`, `channels`, and
`extra-args` can be validated against the old tool value. Update the ordering
logic in `orderedUpdateFields`/the mutation loop so `session.FieldTool` is
processed before any claude-only fields, ensuring the new tool is set first and
incompatible settings are rejected or adjusted against the correct value.
---
Outside diff comments:
In `@cmd/agent-deck/main.go`:
- Around line 1962-1983: `list --json` is leaking raw per-session environment
values through `sessionJSON.Env`, so update the JSON rendering in `main.go` to
redact sensitive env entries by default. Adjust the code that populates
`sessionJSON` from `inst.Env` so it emits only keys or masked values (for
example via a helper used by the list output), and add an explicit opt-in flag
such as `--reveal-env` for callers that need the full `KEY=VALUE` pairs. Use the
existing `sessionJSON` struct and the list/JSON serialization path as the place
to apply the redaction.
---
Nitpick comments:
In `@cmd/agent-deck/main.go`:
- Around line 1716-1730: The SSH remote-path plus per-session env validation is
duplicated here, alongside the same rule already enforced in the session
mutators and web mutator paths, so centralize it into one shared guard instead
of keeping another inline check. Update the creation flow around
newInstance.SSHRemotePath and newInstance.Env to call the same validation logic
used by internal/session/mutators.go and internal/ui/web_mutator.go, and keep
the error handling/message consistent across all SSH session entry points.
In `@internal/session/mutators_test.go`:
- Around line 10-33: Add test coverage for the new SSH remote-path rejection in
SetField for FieldEnv. Extend internal/session/mutators_test.go with a case
around SetField and the SSHRemotePath guard so that when inst.SSHRemotePath is
set, setting a non-empty env value is rejected, while the unset path still
succeeds. Use the existing TestSetField_EnvUpsertUnsetAndValidation and
SetField/FieldEnv symbols to place the new regression coverage near the current
env tests.
In `@internal/tmux/redact_inline_test.go`:
- Around line 29-36: The helper contains currently reimplements substring
matching manually; replace that logic with strings.Contains and add strings to
the import block. Update the contains function in redact_inline_test.go to
delegate to the standard library so the test helper is simpler and consistent.
In `@internal/ui/edit_session_dialog_test.go`:
- Around line 637-652: Add a regression test in
TestEditSessionDialog_GetChanges_EnvUpserts that covers duplicate env keys in
the editor input, using EditSessionDialog and GetChanges to verify only the last
value for a repeated key is kept. Populate the env field with duplicate entries
such as the same key appearing twice, then assert envChangeValues/GetChanges
returns a single whole-list FieldEnv change whose content preserves only the
final assignment, preventing duplicate-preserving behavior from returning.
In `@internal/web/parity_test.go`:
- Around line 32-41: Add an end-to-end parity case to
TestParity_WebActionMatchesDirectMutator that exercises PATCH env via both the
web HTTP path and the direct mutator path. Extend the cases table with an
update_env-style entry that uses parityFixture and compares the changed session
after calling WebMutator.UpdateSession against the direct UpdateSession flow, so
regressions in handlers_sessions.go’s env encoding or WebMutator.parseWebEnvList
are covered. Ensure the new case targets the existing parity helpers and asserts
the env change on the same session record.
🪄 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 Plus
Run ID: 5b34cc8d-00fc-4dac-a5fd-2d3bc182c5fa
⛔ Files ignored due to path filters (6)
CHANGELOG.mdis excluded by!**/*.md,!CHANGELOG.mdREADME.mdis excluded by!**/*.mdskills/agent-deck/SKILL.mdis excluded by!**/*.mdskills/agent-deck/references/cli-reference.mdis excluded by!**/*.mdskills/agent-deck/references/config-reference.mdis excluded by!**/*.mdskills/agent-deck/references/tui-reference.mdis excluded by!**/*.md
📒 Files selected for processing (40)
cmd/agent-deck/env_cmd_test.gocmd/agent-deck/launch_cmd.gocmd/agent-deck/main.gocmd/agent-deck/session_cmd.gocmd/agent-deck/web_cmd_test.gointernal/session/env.gointernal/session/env_precedence_test.gointernal/session/env_test.gointernal/session/fork_env_test.gointernal/session/instance.gointernal/session/instance_test.gointernal/session/mutators.gointernal/session/mutators_test.gointernal/session/session_persistence_test.gointernal/session/storage.gointernal/session/storage_test.gointernal/statedb/migrate.gointernal/statedb/multi_repo_test.gointernal/tmux/redact_inline_test.gointernal/tmux/tmux.gointernal/ui/confirm_dialog.gointernal/ui/edit_session_dialog.gointernal/ui/edit_session_dialog_test.gointernal/ui/env_validation_fix_test.gointernal/ui/home.gointernal/ui/newdialog.gointernal/ui/newdialog_test.gointernal/ui/web_mutator.gointernal/ui/web_mutator_rollback_test.gointernal/ui/web_mutator_test.gointernal/web/api_types.gointernal/web/handlers_sessions.gointernal/web/handlers_sessions_test.gointernal/web/parity_test.gointernal/web/server.gointernal/web/session_data_service.gointernal/web/static/app/CreateSessionDialog.jsinternal/web/static/app/EditSessionDialog.jsinternal/web/static_files_test.gotests/web/fixtures/cmd/web-fixture/main.go
✅ Files skipped from review due to trivial changes (2)
- cmd/agent-deck/session_cmd.go
- internal/ui/web_mutator_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/statedb/multi_repo_test.go
- cmd/agent-deck/env_cmd_test.go
- cmd/agent-deck/web_cmd_test.go
- internal/ui/env_validation_fix_test.go
- internal/ui/web_mutator_rollback_test.go
- internal/session/env_precedence_test.go
- internal/session/env_test.go
- internal/session/storage.go
- internal/web/server.go
- internal/session/instance_test.go
- internal/session/env.go
- internal/tmux/tmux.go
- internal/session/fork_env_test.go
Comment on lines
+303
to
+337
| case editFieldEnv: | ||
| // Untouched guard FIRST (before canonicalization): if the textarea | ||
| // still holds exactly what Show() seeded, emit nothing. This preserves | ||
| // a value that canonicalization would otherwise mutate — e.g. a stored | ||
| // "FOO=bar " (trailing space in the value) must survive an untouched | ||
| // save rather than being trimmed to "FOO=bar". | ||
| if f.area.Value() == f.orig { | ||
| continue | ||
| } | ||
| // This textarea holds the WHOLE env list (one KEY=VALUE per line). | ||
| // Canonicalize — trim each line, drop blank/unparseable lines — so a | ||
| // cosmetic edit (trailing newline, blank line, padded line) that leaves | ||
| // the effective env unchanged does not emit a spurious change. | ||
| desired := make([]string, 0) | ||
| for _, line := range strings.Split(f.area.Value(), "\n") { | ||
| line = strings.TrimSpace(line) | ||
| if line == "" { | ||
| continue | ||
| } | ||
| if _, _, err := session.ParseSessionEnvPair(line); err != nil { | ||
| continue | ||
| } | ||
| desired = append(desired, line) | ||
| } | ||
| // No-op guard: the canonical desired list equals the current env | ||
| // (inst.Env is already canonical — validated on the way in). | ||
| joined := strings.Join(desired, "\n") | ||
| if joined == strings.Join(inst.Env, "\n") { | ||
| continue | ||
| } | ||
| // Emit ONE whole-list Change (newline-joined). The commit path replaces | ||
| // inst.Env wholesale (like the web PATCH) rather than routing rows | ||
| // through SetField's single-pair upsert/unset — which would treat a | ||
| // desired empty-valued entry ("FOO=") as an unset and drop it. | ||
| changes = append(changes, Change{Field: session.FieldEnv, Value: joined, IsLive: isLive}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Whole-list env commit doesn't dedupe duplicate keys.
GetChanges's editFieldEnv canonicalization trims/drops blank/unparseable lines but never collapses duplicate keys. If a user's textarea has two lines for the same key (e.g. FOO=bar then FOO=baz), both survive into the single whole-list Change, so inst.Env ends up storing both entries. This diverges from NewDialog.GetSessionEnv() (uses session.DedupeSessionEnv, last-wins) and from mutators.SetField's FieldEnv case (uses UpsertSessionEnv), which both maintain per-key uniqueness. Validate() doesn't catch this either, so it's silently persisted.
🐛 Proposed fix — reuse the same canonicalization as NewDialog
- desired := make([]string, 0)
- for _, line := range strings.Split(f.area.Value(), "\n") {
- line = strings.TrimSpace(line)
- if line == "" {
- continue
- }
- if _, _, err := session.ParseSessionEnvPair(line); err != nil {
- continue
- }
- desired = append(desired, line)
- }
+ // DedupeSessionEnv trims, drops blank/invalid lines, and collapses
+ // duplicate keys (last wins) — mirrors NewDialog.GetSessionEnv so
+ // both surfaces produce the same canonical env shape.
+ desired := session.DedupeSessionEnv(strings.Split(f.area.Value(), "\n"))Also applies to: 365-375
🤖 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 `@internal/ui/edit_session_dialog.go` around lines 303 - 337, The editFieldEnv
path in GetChanges currently trims and filters lines but still lets duplicate
env keys survive into the whole-list Change, so canonicalize the textarea using
the same per-key deduping behavior as NewDialog.GetSessionEnv and
mutators.SetField’s FieldEnv handling. Update the editFieldEnv branch to
collapse duplicate keys with last-wins semantics before comparing/joining and
appending the session.FieldEnv Change, so inst.Env is stored uniquely and
matches the existing env canonicalization rules.
Comment on lines
+9380
to
+9468
| // commitEditSessionDialog validates and applies the edit-session dialog's | ||
| // pending changes, then hides it (auto-restarting on restart-required edits). | ||
| // Shared by the Enter and ^S key paths. | ||
| func (h *Home) commitEditSessionDialog() (tea.Model, tea.Cmd) { | ||
| if errMsg := h.editSessionDialog.Validate(); errMsg != "" { | ||
| h.editSessionDialog.SetError(errMsg) | ||
| return h, nil | ||
| } | ||
|
|
||
| sessionID := h.editSessionDialog.SessionID() | ||
| inst := h.getInstanceByID(sessionID) | ||
| if inst == nil { | ||
| h.editSessionDialog.Hide() | ||
| return h, nil | ||
| } | ||
|
|
||
| changes := h.editSessionDialog.GetChanges(inst) | ||
| fields := make([]string, len(changes)) | ||
| for i, c := range changes { | ||
| fields[i] = c.Field | ||
| } | ||
| uiLog.Debug("edit_session_commit", | ||
| slog.String("session_id", sessionID), | ||
| slog.Any("fields", fields)) | ||
| if len(changes) == 0 { | ||
| h.editSessionDialog.Hide() | ||
| return h, nil | ||
| } | ||
|
|
||
| // Apply Tool last so claude-only validation (Skip/Auto/ExtraArgs) | ||
| // sees the pre-edit Tool — otherwise Tool=claude→shell with a | ||
| // Skip toggle in the same submit fails IsClaudeCompatible on | ||
| // the toggle. | ||
| orderedChanges := make([]Change, 0, len(changes)) | ||
| for _, c := range changes { | ||
| if c.Field != session.FieldTool { | ||
| orderedChanges = append(orderedChanges, c) | ||
| } | ||
| for _, c := range changes { | ||
| if c.Field == session.FieldTool { | ||
| orderedChanges = append(orderedChanges, c) | ||
| } | ||
| } | ||
| for _, c := range changes { | ||
| if c.Field == session.FieldTool { | ||
| orderedChanges = append(orderedChanges, c) | ||
| } | ||
| } | ||
|
|
||
| titleChanged := false | ||
| hadRestartRequired := false | ||
| var postCommits []func() | ||
| h.instancesMu.Lock() | ||
| for _, c := range orderedChanges { | ||
| _, postCommit, err := session.SetField(inst, c.Field, c.Value, nil) | ||
| if err != nil { | ||
| titleChanged := false | ||
| hadRestartRequired := false | ||
| var postCommits []func() | ||
| h.instancesMu.Lock() | ||
| for _, c := range orderedChanges { | ||
| // Env is applied as an atomic WHOLE-LIST replacement (the dialog emits one | ||
| // newline-joined FieldEnv change), mirroring WebMutator.UpdateSession. | ||
| // Routing rows through SetField's single-pair path would treat a desired | ||
| // empty-valued entry ("FOO=") as an unset and silently drop it. | ||
| if c.Field == session.FieldEnv { | ||
| newEnv, perr := parseWebEnvList(c.Value) | ||
| if perr != nil { | ||
| h.instancesMu.Unlock() | ||
| h.editSessionDialog.SetError(err.Error()) | ||
| h.editSessionDialog.SetError(perr.Error()) | ||
| return h, nil | ||
| } | ||
| if postCommit != nil { | ||
| postCommits = append(postCommits, postCommit) | ||
| } | ||
| if c.Field == session.FieldTitle { | ||
| titleChanged = true | ||
| if len(newEnv) > 0 && inst.SSHRemotePath != "" { | ||
| h.instancesMu.Unlock() | ||
| h.editSessionDialog.SetError("per-session env is not supported for SSH remote-path sessions") | ||
| return h, nil | ||
| } | ||
| inst.Env = newEnv | ||
| if !c.IsLive { | ||
| hadRestartRequired = true | ||
| } | ||
| continue | ||
| } | ||
| h.instancesMu.Unlock() | ||
| // postCommits run AFTER unlocking so slow tmux subprocesses don't | ||
| // stall the status worker / preview cache / reconciler. | ||
| for _, fn := range postCommits { | ||
| fn() | ||
| _, postCommit, err := session.SetField(inst, c.Field, c.Value, nil) | ||
| if err != nil { | ||
| h.instancesMu.Unlock() | ||
| h.editSessionDialog.SetError(err.Error()) | ||
| return h, nil | ||
| } | ||
|
|
||
| // Mirror the rename-path #697 race fix: queue title so a watcher | ||
| // reload can re-apply it after the load swap. | ||
| if titleChanged { | ||
| h.pendingTitleChanges[sessionID] = inst.Title | ||
| h.invalidatePreviewCache(sessionID) | ||
| if postCommit != nil { | ||
| postCommits = append(postCommits, postCommit) | ||
| } | ||
| h.rebuildFlatItems() | ||
| // forceSaveInstances bypasses the isReloading no-op in | ||
| // saveInstances. Title-only loss is caught by pendingTitleChanges | ||
| // re-application; non-Title fields have no such net. | ||
| h.forceSaveInstances() | ||
|
|
||
| h.editSessionDialog.Hide() | ||
| // Auto-restart on restart-required edits — Tool/Skip/Auto/ExtraArgs | ||
| // only take effect on next launch, so deferring would just leave | ||
| // the user staring at old behavior. Mirrors the manual `R` path, | ||
| // skipping when an animation is in flight or CanRestart says no. | ||
| if hadRestartRequired { | ||
| if h.hasActiveAnimation(sessionID) { | ||
| uiLog.Debug("edit_session_skip_restart_active_anim", slog.String("session_id", sessionID)) | ||
| h.setError(fmt.Errorf("saved — restart skipped, session is starting; press R when ready")) | ||
| return h, nil | ||
| } | ||
| if !inst.CanRestart() { | ||
| uiLog.Debug("edit_session_skip_restart_cannot", slog.String("session_id", sessionID)) | ||
| h.setError(fmt.Errorf("saved — start the session to apply tool/extra-args/permission changes")) | ||
| return h, nil | ||
| } | ||
| uiLog.Debug("edit_session_auto_restart", slog.String("session_id", sessionID)) | ||
| h.resumingSessions[sessionID] = time.Now() | ||
| return h, h.restartSession(inst) | ||
| if c.Field == session.FieldTitle { | ||
| titleChanged = true | ||
| } | ||
| return h, nil | ||
|
|
||
| case "esc": | ||
| h.editSessionDialog.Hide() | ||
| return h, nil | ||
| if !c.IsLive { | ||
| hadRestartRequired = true | ||
| } | ||
| } | ||
| h.instancesMu.Unlock() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Env validation failure mid-loop leaves earlier field mutations applied to inst without persisting.
orderedChanges places FieldEnv after Title/Pin in field order. If parseWebEnvList errors or the SSH-remote-path guard trips, the function returns early (SetError + return h, nil) — but any prior SetField calls in the same loop (e.g. Title, Pin) have already mutated the shared *Instance in place, and forceSaveInstances()/rebuildFlatItems() are never reached. The change sits silently in memory until an unrelated future save flushes it, even though the user was told the edit failed. The comment above (9430-9434) says this "mirrors WebMutator.UpdateSession", which per the PR does atomic PATCH rollback — this path doesn't have that guarantee.
Since GetChanges emits at most one FieldEnv change, pre-validating it before the mutation loop is a small, targeted fix:
🛡️ Proposed fix — validate Env before mutating other fields
+ // Pre-validate the (at most one) FieldEnv change before mutating any
+ // other field, so a rejected env update can't leave earlier fields
+ // already applied to inst without being persisted.
+ var pendingEnv []string
+ envIsLive := true
+ for _, c := range orderedChanges {
+ if c.Field != session.FieldEnv {
+ continue
+ }
+ newEnv, perr := parseWebEnvList(c.Value)
+ if perr != nil {
+ h.editSessionDialog.SetError(perr.Error())
+ return h, nil
+ }
+ if len(newEnv) > 0 && inst.SSHRemotePath != "" {
+ h.editSessionDialog.SetError("per-session env is not supported for SSH remote-path sessions")
+ return h, nil
+ }
+ pendingEnv, envIsLive = newEnv, c.IsLive
+ }
+
h.instancesMu.Lock()
for _, c := range orderedChanges {
- if c.Field == session.FieldEnv {
- newEnv, perr := parseWebEnvList(c.Value)
- if perr != nil {
- h.instancesMu.Unlock()
- h.editSessionDialog.SetError(perr.Error())
- return h, nil
- }
- if len(newEnv) > 0 && inst.SSHRemotePath != "" {
- h.instancesMu.Unlock()
- h.editSessionDialog.SetError("per-session env is not supported for SSH remote-path sessions")
- return h, nil
- }
- inst.Env = newEnv
- if !c.IsLive {
- hadRestartRequired = true
- }
+ if c.Field == session.FieldEnv {
+ inst.Env = pendingEnv
+ if !envIsLive {
+ hadRestartRequired = true
+ }
continue
}📝 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.
Suggested change
| // commitEditSessionDialog validates and applies the edit-session dialog's | |
| // pending changes, then hides it (auto-restarting on restart-required edits). | |
| // Shared by the Enter and ^S key paths. | |
| func (h *Home) commitEditSessionDialog() (tea.Model, tea.Cmd) { | |
| if errMsg := h.editSessionDialog.Validate(); errMsg != "" { | |
| h.editSessionDialog.SetError(errMsg) | |
| return h, nil | |
| } | |
| sessionID := h.editSessionDialog.SessionID() | |
| inst := h.getInstanceByID(sessionID) | |
| if inst == nil { | |
| h.editSessionDialog.Hide() | |
| return h, nil | |
| } | |
| changes := h.editSessionDialog.GetChanges(inst) | |
| fields := make([]string, len(changes)) | |
| for i, c := range changes { | |
| fields[i] = c.Field | |
| } | |
| uiLog.Debug("edit_session_commit", | |
| slog.String("session_id", sessionID), | |
| slog.Any("fields", fields)) | |
| if len(changes) == 0 { | |
| h.editSessionDialog.Hide() | |
| return h, nil | |
| } | |
| // Apply Tool last so claude-only validation (Skip/Auto/ExtraArgs) | |
| // sees the pre-edit Tool — otherwise Tool=claude→shell with a | |
| // Skip toggle in the same submit fails IsClaudeCompatible on | |
| // the toggle. | |
| orderedChanges := make([]Change, 0, len(changes)) | |
| for _, c := range changes { | |
| if c.Field != session.FieldTool { | |
| orderedChanges = append(orderedChanges, c) | |
| } | |
| for _, c := range changes { | |
| if c.Field == session.FieldTool { | |
| orderedChanges = append(orderedChanges, c) | |
| } | |
| } | |
| for _, c := range changes { | |
| if c.Field == session.FieldTool { | |
| orderedChanges = append(orderedChanges, c) | |
| } | |
| } | |
| titleChanged := false | |
| hadRestartRequired := false | |
| var postCommits []func() | |
| h.instancesMu.Lock() | |
| for _, c := range orderedChanges { | |
| _, postCommit, err := session.SetField(inst, c.Field, c.Value, nil) | |
| if err != nil { | |
| titleChanged := false | |
| hadRestartRequired := false | |
| var postCommits []func() | |
| h.instancesMu.Lock() | |
| for _, c := range orderedChanges { | |
| // Env is applied as an atomic WHOLE-LIST replacement (the dialog emits one | |
| // newline-joined FieldEnv change), mirroring WebMutator.UpdateSession. | |
| // Routing rows through SetField's single-pair path would treat a desired | |
| // empty-valued entry ("FOO=") as an unset and silently drop it. | |
| if c.Field == session.FieldEnv { | |
| newEnv, perr := parseWebEnvList(c.Value) | |
| if perr != nil { | |
| h.instancesMu.Unlock() | |
| h.editSessionDialog.SetError(err.Error()) | |
| h.editSessionDialog.SetError(perr.Error()) | |
| return h, nil | |
| } | |
| if postCommit != nil { | |
| postCommits = append(postCommits, postCommit) | |
| } | |
| if c.Field == session.FieldTitle { | |
| titleChanged = true | |
| if len(newEnv) > 0 && inst.SSHRemotePath != "" { | |
| h.instancesMu.Unlock() | |
| h.editSessionDialog.SetError("per-session env is not supported for SSH remote-path sessions") | |
| return h, nil | |
| } | |
| inst.Env = newEnv | |
| if !c.IsLive { | |
| hadRestartRequired = true | |
| } | |
| continue | |
| } | |
| h.instancesMu.Unlock() | |
| // postCommits run AFTER unlocking so slow tmux subprocesses don't | |
| // stall the status worker / preview cache / reconciler. | |
| for _, fn := range postCommits { | |
| fn() | |
| _, postCommit, err := session.SetField(inst, c.Field, c.Value, nil) | |
| if err != nil { | |
| h.instancesMu.Unlock() | |
| h.editSessionDialog.SetError(err.Error()) | |
| return h, nil | |
| } | |
| // Mirror the rename-path #697 race fix: queue title so a watcher | |
| // reload can re-apply it after the load swap. | |
| if titleChanged { | |
| h.pendingTitleChanges[sessionID] = inst.Title | |
| h.invalidatePreviewCache(sessionID) | |
| if postCommit != nil { | |
| postCommits = append(postCommits, postCommit) | |
| } | |
| h.rebuildFlatItems() | |
| // forceSaveInstances bypasses the isReloading no-op in | |
| // saveInstances. Title-only loss is caught by pendingTitleChanges | |
| // re-application; non-Title fields have no such net. | |
| h.forceSaveInstances() | |
| h.editSessionDialog.Hide() | |
| // Auto-restart on restart-required edits — Tool/Skip/Auto/ExtraArgs | |
| // only take effect on next launch, so deferring would just leave | |
| // the user staring at old behavior. Mirrors the manual `R` path, | |
| // skipping when an animation is in flight or CanRestart says no. | |
| if hadRestartRequired { | |
| if h.hasActiveAnimation(sessionID) { | |
| uiLog.Debug("edit_session_skip_restart_active_anim", slog.String("session_id", sessionID)) | |
| h.setError(fmt.Errorf("saved — restart skipped, session is starting; press R when ready")) | |
| return h, nil | |
| } | |
| if !inst.CanRestart() { | |
| uiLog.Debug("edit_session_skip_restart_cannot", slog.String("session_id", sessionID)) | |
| h.setError(fmt.Errorf("saved — start the session to apply tool/extra-args/permission changes")) | |
| return h, nil | |
| } | |
| uiLog.Debug("edit_session_auto_restart", slog.String("session_id", sessionID)) | |
| h.resumingSessions[sessionID] = time.Now() | |
| return h, h.restartSession(inst) | |
| if c.Field == session.FieldTitle { | |
| titleChanged = true | |
| } | |
| return h, nil | |
| case "esc": | |
| h.editSessionDialog.Hide() | |
| return h, nil | |
| if !c.IsLive { | |
| hadRestartRequired = true | |
| } | |
| } | |
| h.instancesMu.Unlock() | |
| // commitEditSessionDialog validates and applies the edit-session dialog's | |
| // pending changes, then hides it (auto-restarting on restart-required edits). | |
| // Shared by the Enter and ^S key paths. | |
| func (h *Home) commitEditSessionDialog() (tea.Model, tea.Cmd) { | |
| if errMsg := h.editSessionDialog.Validate(); errMsg != "" { | |
| h.editSessionDialog.SetError(errMsg) | |
| return h, nil | |
| } | |
| sessionID := h.editSessionDialog.SessionID() | |
| inst := h.getInstanceByID(sessionID) | |
| if inst == nil { | |
| h.editSessionDialog.Hide() | |
| return h, nil | |
| } | |
| changes := h.editSessionDialog.GetChanges(inst) | |
| fields := make([]string, len(changes)) | |
| for i, c := range changes { | |
| fields[i] = c.Field | |
| } | |
| uiLog.Debug("edit_session_commit", | |
| slog.String("session_id", sessionID), | |
| slog.Any("fields", fields)) | |
| if len(changes) == 0 { | |
| h.editSessionDialog.Hide() | |
| return h, nil | |
| } | |
| // Apply Tool last so claude-only validation (Skip/Auto/ExtraArgs) | |
| // sees the pre-edit Tool — otherwise Tool=claude→shell with a | |
| // Skip toggle in the same submit fails IsClaudeCompatible on | |
| // the toggle. | |
| orderedChanges := make([]Change, 0, len(changes)) | |
| for _, c := range changes { | |
| if c.Field != session.FieldTool { | |
| orderedChanges = append(orderedChanges, c) | |
| } | |
| } | |
| for _, c := range changes { | |
| if c.Field == session.FieldTool { | |
| orderedChanges = append(orderedChanges, c) | |
| } | |
| } | |
| titleChanged := false | |
| hadRestartRequired := false | |
| var postCommits []func() | |
| // Pre-validate the (at most one) FieldEnv change before mutating any | |
| // other field, so a rejected env update can't leave earlier fields | |
| // already applied to inst without being persisted. | |
| var pendingEnv []string | |
| envIsLive := true | |
| for _, c := range orderedChanges { | |
| if c.Field != session.FieldEnv { | |
| continue | |
| } | |
| newEnv, perr := parseWebEnvList(c.Value) | |
| if perr != nil { | |
| h.editSessionDialog.SetError(perr.Error()) | |
| return h, nil | |
| } | |
| if len(newEnv) > 0 && inst.SSHRemotePath != "" { | |
| h.editSessionDialog.SetError("per-session env is not supported for SSH remote-path sessions") | |
| return h, nil | |
| } | |
| pendingEnv, envIsLive = newEnv, c.IsLive | |
| } | |
| h.instancesMu.Lock() | |
| for _, c := range orderedChanges { | |
| // Env is applied as an atomic WHOLE-LIST replacement (the dialog emits one | |
| // newline-joined FieldEnv change), mirroring WebMutator.UpdateSession. | |
| // Routing rows through SetField's single-pair path would treat a desired | |
| // empty-valued entry ("FOO=") as an unset and silently drop it. | |
| if c.Field == session.FieldEnv { | |
| inst.Env = pendingEnv | |
| if !envIsLive { | |
| hadRestartRequired = true | |
| } | |
| continue | |
| } | |
| _, postCommit, err := session.SetField(inst, c.Field, c.Value, nil) | |
| if err != nil { | |
| h.instancesMu.Unlock() | |
| h.editSessionDialog.SetError(err.Error()) | |
| return h, nil | |
| } | |
| if postCommit != nil { | |
| postCommits = append(postCommits, postCommit) | |
| } | |
| if c.Field == session.FieldTitle { | |
| titleChanged = true | |
| } | |
| if !c.IsLive { | |
| hadRestartRequired = true | |
| } | |
| } | |
| h.instancesMu.Unlock() |
🤖 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 `@internal/ui/home.go` around lines 9380 - 9468, The edit-session commit path
can partially mutate the shared Instance before Env validation fails, leaving
in-memory changes unsaved. In commitEditSessionDialog, validate the FieldEnv
change up front (or otherwise defer all SetField mutations until after
parseWebEnvList and the SSH-remote-path check succeed) so that no
Title/Pin/other changes are applied if Env is invalid. Keep the atomic behavior
aligned with WebMutator.UpdateSession by ensuring any early error exits from
commitEditSessionDialog happen before mutating inst and before
forceSaveInstances/rebuildFlatItems.
Comment on lines
+508
to
+541
| for _, field := range orderedUpdateFields(updates) { | ||
| value := updates[field] | ||
| // FieldEnv from the web is a whole-list replacement (the edit form | ||
| // submits the full desired env, newline-joined by updatesFromRequest), | ||
| // not the single KEY=VALUE upsert/unset that session.SetField(FieldEnv) | ||
| // expects. Handle it separately so the web replaces the env atomically. | ||
| if field == session.FieldEnv { | ||
| newEnv, perr := parseWebEnvList(value) | ||
| if perr != nil { | ||
| rollbackAll() | ||
| m.h.instancesMu.Unlock() | ||
| return nil, false, perr | ||
| } | ||
| // This path bypasses session.SetField, so re-apply its SSH remote-path | ||
| // guard here: per-session env is silently dropped at spawn for | ||
| // SSHRemotePath sessions (wrapForSSH re-escaping mangles the inline | ||
| // export quoting), so reject a non-empty replacement instead of | ||
| // persisting env that would never take effect. An empty list (clear) | ||
| // stays allowed. | ||
| if len(newEnv) > 0 && inst.SSHRemotePath != "" { | ||
| rollbackAll() | ||
| m.h.instancesMu.Unlock() | ||
| return nil, false, &session.MutationError{Field: session.FieldEnv, Msg: "per-session env is not supported for SSH remote-path sessions"} | ||
| } | ||
| if strings.Join(inst.Env, "\n") == strings.Join(newEnv, "\n") { | ||
| continue | ||
| } | ||
| inst.Env = newEnv | ||
| changed = append(changed, field) | ||
| if session.RestartPolicyFor(field) == session.FieldRestartRequired { | ||
| restartRequired = true | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'Field(Title|Tool|Plugins|Channels|ExtraArgs|Notes|Color|SkipPermissions|AutoMode|Env)\s*=' internal/session/mutators.goRepository: asheshgoplani/agent-deck
Length of output: 549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- internal/ui/web_mutator.go (orderedUpdateFields and UpdateSession) ---'
sed -n '480,620p' internal/ui/web_mutator.go
echo
echo '--- internal/session/mutators.go (SetField validations) ---'
sed -n '1,220p' internal/session/mutators.goRepository: asheshgoplani/agent-deck
Length of output: 12414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- orderedUpdateFields in internal/ui/web_mutator.go ---'
rg -n -A20 -B5 'func orderedUpdateFields|sort\.Strings|sort\.Slice' internal/ui/web_mutator.go
echo
echo '--- session field validation in internal/session/mutators.go ---'
rg -n -A80 -B10 'case Field(Tool|Plugins|Channels|ExtraArgs)|RestartPolicyFor|MutationError' internal/session/mutators.goRepository: asheshgoplani/agent-deck
Length of output: 19128
Apply tool before tool-dependent fields
In internal/ui/web_mutator.go:586-595, sort.Strings puts tool after plugins, channels, and extra-args. A multi-field PATCH can therefore validate and persist claude-only settings against the old tool value, leaving incompatible state behind. Special-case FieldTool first.
🤖 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 `@internal/ui/web_mutator.go` around lines 508 - 541, The update handling in
web_mutator currently applies `FieldTool` too late, so tool-dependent fields
like `plugins`, `channels`, and `extra-args` can be validated against the old
tool value. Update the ordering logic in `orderedUpdateFields`/the mutation loop
so `session.FieldTool` is processed before any claude-only fields, ensuring the
new tool is set first and incompatible settings are rejected or adjusted against
the correct value.
Owner
|
🤖 autopilot: no CI checks found for this PR. Autopilot cannot assess safety without CI results — please trigger CI and ensure all checks pass before this is eligible for merge. Generated by Claude Code |
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
2 participants
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.
Summary
Adds per-session environment variables for every supported tool —
KEY=VALUEpairs attached to a single session (at creation or later) that are exported into the spawned process.This closes a real gap: env previously came only from group/tool
env_files and a few hardcoded vars, so a one-offHTTPS_PROXY, feature flag, or API-base override for a single session was impossible. Per-session env is the most-specific scope, so it wins over the config-level env layers on a key collision.What's included
Instance.Env []string(KEY=VALUE, stored in thetool_dataJSON blob; no schema migration), modeled onExtraArgs.ValidateSessionEnvKey,ParseSessionEnvPair,UpsertSessionEnv,UnsetSessionEnv,DedupeSessionEnv,buildSessionEnvExports.prepareCommandchokepoint (covering raw/custom sessions) plus, for builder paths, as the final grouped clause inbuildEnvSourceCommand's precedence chain — so a per-session value is exported after the config env layers and the login shell, and therefore wins over a collidingenv_file/inline/group/conductor/~/.zshrcvalue, for all tools. Survives SSH/Docker wrapping; skipped for remote-path SSH.exportvalues are redacted.Surfaces
agent-deck session set <id> env KEY=VALUE/env KEY=(unset);add/launch --env KEY=VALUE(repeatable, upserts on repeat);list --jsonexposes it.KEY=VALUE, one per line) and edit-session dialog (P→Env).MenuSessionprojection + server-side validation, and the create/edit dialogs (key/value editor).README.mdandskills/agent-deck/references/*(precedence, plaintext-at-rest note, remote-path limitation).Security & safety
[A-Za-z_][A-Za-z0-9_]*); values are shell-escaped at every injection site; CR/LF in values is rejected at the shared validator so the newline-joined web transport round-trips losslessly.Known limitations & scope
SSHRemotePath) — the remotecd '<path>' && …re-quoting would mangle the inline exports. The proactive surfaces (CLIadd --ssh --remote-path --env, TUI/web remote-create) reject it up front;session set/web PATCH also reject setting env on such a session.env_file/inline/group/conductor env and with~/.zshrc;TELEGRAM_*neutralisation still applies last for safety.Validation
go build ./...,go vet ./..., andgolangci-lint(v2) are clean.&&-chain grouping, precedence, log suppression, tmux redaction, persistence round-trips, fork inheritance, dedup of duplicate keys, the mutators (set/unset/invalid-key, SSH-remote-path rejection), CLI flags, web create/patch (+ rollback atomicity), and the TUI/web dialogs.xhigh) across multiple rounds.Summary by CodeRabbit
New Features
Bug Fixes