feat(all): add --active-session-dir to decouple Active Sessions coordination from --temp

[simtel12]

Summary

• Active Sessions coordination (the lock + discovery files that let independent Lich processes find each other) previously always lived under TEMP_DIR. Anyone overriding --temp per-character to keep debug logs separate ends up with an isolated Active Sessions registry per character instead of one shared one, defeating the point of a cross-process session list.
• Adds --active-session-dir=PATH to point coordination at a shared directory independently of --temp. Unset, behavior is unchanged (falls back to TEMP_DIR, or Dir.tmpdir if that's undefined).
• Fixes two gaps caught in review before this landed: coordination_dir had no boot-time mkdir the way TEMP_DIR does, so a not-yet-existing --active-session-dir path silently degraded to "service unavailable" with no trace of the real cause; and SessionLauncher's optional-flag forwarding assumed every path flag has a constants.rb default a spawned child re-derives on its own, which isn't true for this one and caused it to be silently dropped when forwarded value matched the parent's.
• Second commit: an explicit --active-session-dir also implicitly enables the Active Sessions service for that launch, without persisting anything to lich_settings — removing the flag on a later launch reverts to the persisted default (disabled). Kept as a separate commit in case the "implicit enable" behavior specifically needs to be reverted independently.

Review round 2 (@OSXLich-Doug)

Four follow-up commits, one per finding:

• [P1] Propagate the parent's active-session directory to GUI children — confirmed, and broader than the report: no production caller populates any of the nine OPTIONAL_PATH_FLAGS context keys, since GUI launch contexts are login entries (account/frontend), not directory overrides. For the other eight that's harmless — a child re-derives them from constants.rb. --active-session-dir has no such default by design, so it was dropped on every GUI-launched child. Marked :inherit and now falls back to this process's ACTIVE_SESSION_DIR when the context omits the key; an explicitly empty value stays an opt-out.
• [P2] Correct the "writes nothing to disk" claim — the service does create the coordination dir, the lock, and the discovery record. Help text now says it persists no setting, and spells out that coordination files are still written. Corrected in the summary above too.
• [P3] Unused :active_session_dir launch-context path — resolved by the P1 fix, which makes the path reachable from production.
• [P3] Release the ownership lock before deleting its temp dir — done in the example's own ensure, ahead of rm_rf.
• [P3] Remove the uncalled spawn_login helper — removed, along with its now-unused ruby_executable require (both remaining RubyExecutable callers require it themselves). Predates this branch; folded in here since it's a second child-launch path already drifted from SessionLauncher. Happy to split it out if you'd rather keep the branch narrow.

Test plan

• bundle exec rspec (full suite): 6346 examples, 0 failures (rebased onto main through fix(all): terminate preserved quiet-state tags with a newline #1532 / 5.20.1)
• bundle exec rubocop on all touched files: no offenses
• Manually reproduced both review findings (ENOENT on a missing coordination dir; flag silently dropped when forwarded value matches the parent's own) before and after the fix
• Manually verified the implicit-enable behavior against a real lich_settings row forced to false, confirming it stays untouched on disk
• Round 2: new specs cover inheriting ACTIVE_SESSION_DIR when the context omits it, when no context is passed at all, the explicit-empty opt-out, and the no-constant case
• Re-ran every manual verification against the post-rebase base, driving the real library code rather than specs:
  • missing coordination dir is created and the lock acquired; with the mkdir_p removed the same call raises Errno::ENOENT and degrades to "service unavailable"
  • real detached child processes spawned via SessionLauncher, asserting on the child's actual ARGV: inherits the parent's --active-session-dir from a production-shaped GUI context, still forwards it when the explicit value equals the parent's, honours the explicit-empty opt-out, and emits nothing when the parent has no ACTIVE_SESSION_DIR. --data matching its constants.rb default is still suppressed in the same run, confirming the suppression logic is live and only bypassed for this flag
  • against a real SQLite lich_settings row forced to false: ActiveSessions.enabled? returns true with the flag set, the row still reads false, no rows are added, the db file is byte-identical, and dropping the flag reverts to the persisted false

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: f8c4a3f2-0706-413f-adf7-5f7116d82b7b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[OSXLich-Doug]

@simtel12 - I wanted to share my thoughts on a few issues. This includes a local repository coderabbit run. After you correct these, we'll run CR here.

[P1] Propagate the parent’s active-session directory to GUI children — session_launcher.rb (line 160)
Production GUI contexts omit :active_session_dir, so persistent children lose the parent’s explicit directory. They either create an isolated registry under TEMP_DIR or remain untracked.

[P2] Correct the claim that the flag writes nothing to disk — help_text.rb (line 199)
The service can create the coordination directory, lock file, and discovery JSON. The accurate guarantee is that it does not persist the feature setting. This incorporates CodeRabbit’s duplicate finding.

[P3] Wire up or remove the unused :active_session_dir launch-context path — session_launcher.rb (line 20)
Only tests produce this key; no production caller does. This is the maintenance consequence of the P1 and will be resolved if the P1 fix makes the path reachable.

[P3] Release the ownership lock before deleting its temporary directory — active_sessions_spec.rb (line 126)
The new example deletes base_dir while @lock_file remains open. The outer after hook eventually releases it, but only after the deletion attempt. This can leak the temporary directory on platforms that cannot remove open locked files. Release the lock in the example’s ensure block before rm_rf. This is CodeRabbit’s new finding.

[P3] Remove the uncalled spawn_login helper — login_helpers.rb (line 622)
Only its unit test calls it. It is a second child-launch implementation that has already drifted from SessionLauncher. This predates the PR, so it is an adjacent maintenance finding rather than a branch blocker.

[simtel12]

@OSXLich-Doug — thanks, these were all real. Pushed as four commits, one per finding, so any of them can be dropped independently.

[P1] Propagate the parent's active-session directory to GUI children (3796a3a7) — confirmed, and it turned out to be a bit broader than the report. It isn't just that GUI contexts omit :active_session_dir: no production caller populates any of the nine OPTIONAL_PATH_FLAGS keys, because the context is a login entry (account, character, frontend, custom launch) plus the :dark_mode merge in handle_play_action. For the other eight flags that's harmless — the child re-derives the same path from constants.rb. --active-session-dir is the one with no default to re-derive, which is exactly why default_path_value special-cases it, so it was being dropped on every GUI-launched child.

Rather than have gui_login stuff the key in (which would miss the login_context.nil? branch), I marked the flag inherit: true and had overridden_path_value fall back to this process's ACTIVE_SESSION_DIR when the context doesn't mention the key. An explicitly empty value is still an opt-out, so a caller can suppress the flag deliberately. Four new specs: inherit-when-omitted, inherit-with-no-context-at-all, the explicit-empty opt-out, and no-flag-when-the-constant-is-undefined.

[P2] Correct the "writes nothing to disk" claim (c0775afc) — you're right, and the wording was wrong in the PR summary too. The service does create the coordination dir, the lock, and the discovery record. Help text now says it persists no setting (so lich_settings is untouched and dropping the flag reverts to the persisted default), and states plainly that coordination files are still written while it runs. I've fixed the same overstatement in the PR description.

[P3] Unused :active_session_dir launch-context path — resolved by the P1 fix, as you predicted; the path is reachable from production now.

[P3] Release the ownership lock before deleting its temp dir (ecb273cb) — done, in the example's own ensure ahead of the rm_rf, rather than leaving it to the outer after hook.

[P3] Remove the uncalled spawn_login helper (64d4d15d) — removed, along with its now-unused require_relative '../ruby_executable'; I checked that both remaining RubyExecutable callers (reconnect_command.rb, session_launcher.rb) require the file themselves, and grepped the DR script repos for outside callers, none. Agreed it predates this branch — say the word and I'll pull it out into its own PR to keep this one narrow. I only folded it in because it's a second child-launch path already drifted from SessionLauncher (no path flags, no dark-mode propagation, and a --start-scripts flag SessionLauncher doesn't emit), and P1 is precisely the class of bug that gets fixed in one spawn path and missed in the other.

Full suite green and rubocop clean on all six touched files.

One thing I ran into while verifying, unrelated to this branch: running login_helpers_spec.rb and session_launcher_spec.rb together fails to load with uninitialized constant WindowsCredentialManager::FFI. It reproduces at this branch's base with nothing applied, and the full suite masks it, so CI never sees it — two competing install_gem_requirements stubs, whichever loads first wins. Filed separately as #1542 rather than widening this branch.

Ready for a CodeRabbit run here whenever you are.

---

Edit: rebased onto latest main (through #1532 / 5.20.1) — clean, no conflicts. SHAs above updated to the post-rebase ones. Re-verified on the new base: 6346 examples, 0 failures, rubocop clean on all nine touched files. #1542 still reproduces on current main, so that one's still live.