feat: session profiles (shell, cwd, env, startup command) - #349
Open
attson wants to merge 11 commits into
Open
Conversation
First L2 sealed content: profiles carry environment variables, so the whole record travels in an account_key envelope under a new AAD tag. Leads with a cross-process registration checklist rather than an implementation sketch -- item 21 shipped end-to-end broken because every enumeration stopped at the desktop process boundary. Two of the seven registration points still have no automated guard (AAD tag allocation and its protocol.md table), so the design makes building that guard part of the work. Also specifies the env merge rule the parent spec omits: without it, 'env does not sync by default' degrades into 'one pull wipes local env'.
Review round 1 finding: a separate stripUnsyncedEnv step that Task 3's caller had to remember to invoke made "env does not sync by default" a convention instead of a guarantee. Move the strip inside sealProfiles so no caller can skip it, mirroring composeFontFamily always appending its CJK fallback chain rather than trusting call sites to do it.
Registers profiles_encrypted across prefssync's syncedKeys, the relay's allowedPreferenceKeys whitelist, the desktop adapter's ReadValue/WriteValue, and isPrefCustomized. WriteValue validates inbound payloads (drops individually-malformed entries rather than rejecting the batch, mirroring the item-21 blast-radius lesson) and merges rather than replaces, so a pull never wipes a machine's unsynced env. NewSession now resolves a profile (explicit > default > today's default_shell/HOME), applies its Shell/Cwd/Env with TERM protected, and injects StartupCmd via the existing SetOnFirstPrompt PTY-write mechanism (never a frontend sendInput, per AGENTS.md redline #28), gated off when an AI-restore session already owns that callback slot. Also fixes mergeProfiles aliasing the local env map instead of copying it.
The profile StartupCmd injection was gated on req.AIKind == "", but SetOnFirstPrompt is only actually claimed by the AI-restore path when computeResumeArgs returns non-nil (requires InitialAISessionID). A freshly classified AI command (AIKind set, no InitialAISessionID) claims nothing, so the old gate silently skipped a default profile's startup command on every such session. Track resumeClaimed at the point the callback is actually registered and gate on !resumeClaimed instead. Also: log each entry filterValidProfiles drops (id + reason), since a dropped profile silently erases something the user created with no other error path; and add a real end-to-end test proving a profile's Env reaches the spawned child process, not just applyProfileEnv in isolation.
SettingsProfiles.vue (list/add/edit/delete/set-default) plus a TabBar picker wired into App.vue's spawnLocalShell/startNewTab/onSplit, closing out roadmap item 22's frontend half. Ticks roadmap item 22, adds the DefaultProfileID sealed-bundling decision to the design doc. web-dist is built from desktop/frontend/src (web/vite.config.ts aliases '@' and the main entry straight into it), not from web/src, so this frontend-only change still requires a web-dist rebuild even though no web/src file changed.
Picker now refreshes on local profile/default edits even when the relay push fails (profiles-changed emit forwarded App <- SettingsDialog <- SettingsProfiles), and the selected-profile picker clears itself when its profile disappears. SetProfiles can no longer strand DefaultProfileID. Shell/cwd from a synced profile get deliberately asymmetric handling on a machine where they don't exist: shell falls back to the default shell with a warning, cwd fails loudly and names the profile in the error. Adds table tests for the four profile bindings (previously zero coverage) and regression tests for the shell-fallback/cwd-fail-loud behavior. Rewrites the AAD-tag test that was actually testing a session-key mismatch instead of the tag byte it claimed to guard. Stops logging a profile's StartupCmd verbatim (leaked via ExportDiagnostics). saveEdit() now surfaces an error instead of silently no-opping on a whitespace-only name. Fixes stale "design doc §7.2" citations and a protocol.md payload-shape typo, and annotates (without rewriting) two outdated snippets in the session-profiles plan doc.
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
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.
Roadmap item 22. Stacked on #348 → #346 → #345. Bases retarget as each merges; merge in order.
What
Named launch configurations, picked when opening a tab or split, one settable as default. Closes three Backlog entries at once — 默认 shell 设置改进 / 启动目录设置 / 环境变量设置 — because those were three fields of the same thing.
Precedence: explicitly picked profile > default profile >
default_shell+ HOME.default_shell(synced since #348) is not replaced; it is what runs when no profile applies. The Settings copy says this, because it is the question this feature will otherwise generate.Environment variables do not sync unless you opt in, per profile
Env can hold tokens, so
sync_envdefaults off andsealProfilesstrips unsynced env internally — no caller can bypass it. That turned out to matter: the original plan had stripping as a separate step a caller must remember, and its own test asserted that unsynced env survived a seal. The test was asserting the bug.The merge rule on pull is the other half. A payload with env stripped is indistinguishable from a profile that never had env, so replacing wholesale would destroy this machine's local env. Merge is per-ID: incoming env wins when present; when incoming has none and local has some, local's survives. Env is only ever overwritten by an explicitly synced env, never cleared by the absence of one.
Machine-specific fields, handled asymmetrically on purpose
A profile carrying
/opt/homebrew/bin/fishand/Users/alice/worksyncs verbatim to a machine where neither exists.default_shell.Infrastructure this item added first
AAD discriminator bytes lived in two places and were documented in a third with nothing connecting them. Since this item adds the second synthetic tag,
internal/e2eecrypto/aadtags.gois now the single registry, pinned todocs/spec/protocol.mdby a test — a missing row is a replay risk, not a documentation nit (redline #22).That guard earned its keep the day it was written:
protocol.mdwas already missing0x37PASTE_FILE, redline #22's own list was missing three entries, and the registry's first draft omitted0x02and0x33— two bytes that are live and tested, missed because their call site passesbyte(f.Type)as a variable rather than a literal. All pre-existing, none findable by reading the redline, since the redline was the incomplete list.Testing
go vet ./...clean and the full Go tree passing; frontend 166 files / 2088 tests; web 232;web-distrebuilt and verified drift-free.Every task got its own review, plus a whole-branch review and a scoped re-review of the fix wave. Behavioral claims are proved by observing real child processes — the startup-command and env tests spawn
/bin/shand read a marker file it writes, rather than asserting on the arguments handed to spawn.Never run against a real GUI. The four manual checks (profile takes effect, default applies,
TERMnot overridable,sync_envoff keeps env local) were not performed — headless agents, and this app cannot screenshot its own window.One unreproduced test failure. During final verification a single frontend run reported one failing test file; five subsequent runs passed and the failing test's name was not captured. It may have been a one-off, or an intermittent flake in the new tests that spawn real child processes and poll for marker files. Recording it rather than assuming the passing runs are the truth.