feat(desktop+config): AO_KEEP_DAEMON keep-alive + per-role worker env profile (claude-code)#2621
Conversation
🔍 Local review (cycle 1) — cleanReviewed locally across both layers (Claude subagent per layer + Codex adversarial on the combined diff), no bots pinged. Verdict: no critical issues. Ship it. All high-risk areas verified airtight by all three reviewers: Desktop layer (
Backend layer (per-role env profile, closes #2195):
No |
5789f25 to
ded1bbe
Compare
| let keepDaemonLogFd: number | undefined; | ||
| const stdio: "pipe" | "ignore" | ["ignore", number, number] = keep | ||
| ? (() => { | ||
| const logPath = path.join(os.homedir(), ".ao", "daemon.log"); |
There was a problem hiding this comment.
This ignores the selected daemon state root. For example, with AO_RUN_FILE=/tmp/ao-a/running.json AO_KEEP_DAEMON=1, persistent-mode logs still append to ~/.ao/daemon.log; a dev and an installed daemon also share that file. Derive the log path from dirname(runFilePath()) (and create the directory) so isolated runs do not mix or write state in the wrong location.
|
|
||
| // isPluginURL reports whether s is an http(s) plugin URL rather than a local | ||
| // path, deciding --plugin-url vs --plugin-dir. | ||
| func isPluginURL(s string) bool { |
There was a problem hiding this comment.
URL schemes are case-insensitive, but this only recognizes lowercase prefixes. A valid HTTPS://example.com/plugin.zip value is emitted as --plugin-dir instead of --plugin-url. Parse the value as a URL and compare the scheme case-insensitively, with a regression test.
| // AgentConfig so an empty (default) config stays a zero value for storage and | ||
| // resolution, while a present-but-empty MCPConfig is meaningful: Strict on its | ||
| // own isolates a worker from every MCP source. | ||
| type MCPConfig struct { |
There was a problem hiding this comment.
This new named contract type is missing its required schemaNames entry in backend/internal/httpd/apispec/specgen/build.go. The generated public API therefore exposes DomainMCPConfig rather than MCPConfig. Add the mapping and regenerate the OpenAPI spec and frontend types.
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. You are also welcome to join the AO Discord community. We do daily Discord calls at 10 PM IST, except Saturday. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
| // must be skipped — otherwise it would defeat the whole point on quit. | ||
| process.on("exit", () => { | ||
| if (daemonProcess && !supervisorLink?.connected) { | ||
| if (daemonProcess && !supervisorLink?.connected && !keepDaemonAlive(process.env)) { |
There was a problem hiding this comment.
This still re-reads process.env at process exit, while the spawn path correctly captures const keep = keepDaemonAlive(process.env) as the daemon’s spawn-mode decision. If this Electron process starts a daemon with AO_KEEP_DAEMON=1 and process.env.AO_KEEP_DAEMON is later changed or unset before exit, the daemon was spawned unlinked/persistent but this fallback cleanup will kill it. Please persist the spawn-time keep-alive/owner state alongside daemonProcess and use that here, clearing it when the child exits or is explicitly stopped.
… profile (claude-code) Two related capabilities, rebased clean onto current main. 1. AO_KEEP_DAEMON (desktop): keep the daemon alive after the Electron app closes. The app records a "persistent" owner in the run-file; on exit it redirects the daemon stdio to a log fd (allowlisted AO_KEEP_DAEMON env) so the process survives the parent exiting. The keep decision is persisted in the owner record and reused on relaunch. 2. Per-role worker environment profile (claude-code): AgentConfig gains SystemPrompt / Env / MCP / PluginDirs fields surfaced through the CLI (--worker-mcp-config, --worker-strict-mcp, --worker-plugin-dir) and the OpenAPI schema. effectiveAgentConfig deep-copies the project Env (mergeEnv) so per-role overrides never mutate the project config; the worker role prompt is rendered before the standing guard. Closes AgentWrapper#2195, Closes AgentWrapper#2230 Co-Authored-By: Claude <noreply@anthropic.com>
f23d057 to
2eef759
Compare
|
Hi @neversettle17-101 @somewherelostt — this is the last open PR from the per-role env + keep-daemon work (#2195 + #2230). I just rebased it clean onto current What it adds:
Would appreciate a review when you have a moment — happy to address anything. Thanks! |
Local review (Claude subagent + Codex companion), cycle 1, both approve with no critical findings. Three non-blocking cleanups applied: - Reuse the spawn-path `agentConfig` for `.Env` instead of recomputing effectiveAgentConfig a second time (manager.go) — matches the restore path, which already reuses it. - Deep-copy `merged.Env` via mergeEnv whenever either the project base or the role override carries Env, so a role override can never alias/mutate the project's base Env. Guards both inputs empty so a project with no config still resolves to a zero AgentConfig (defense-in-depth, matches the MCP pointer copy). - Clarify the AO_KEEP_DAEMON comment in main.ts to note the process.on exit handler re-reads process.env because the spawn-captured `keep` is function-scoped (AO_KEEP_DAEMON is set once at startup, never mutated). Deferred (out of scope for cleanup): daemon.log rotation, role-Env in the workspace-cleanup hook path — no current consumer; follow-ups. Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Local review (cycle 1)Reviewed locally (Claude subagent + Codex companion), no bots pinged. Critical-only bar.
No
Deferred (out of scope, no current consumer): daemon.log rotation; role-Env in the workspace-cleanup hook path. Gates green: gofmt clean, go build ok, session_manager/domain/cli/runfile/claudecode tests pass, 26 daemon-owner tests pass. |
The stdio-selection was an IIFE inside a ternary solely to get a block scope, but the openSync side effect escaped to the outer keepDaemonLogFd anyway — the closure bought nothing over a normal if. Linear statements read more directly than re-running a ternary-IIFE mentally. No behavior change; 26 daemon-owner tests still pass, tsc clean. Co-Authored-By: Claude <noreply@anthropic.com>
illegalcall
left a comment
illegalcall
left a comment
There was a problem hiding this comment.
Request changes after a first-principles review of a2d4ab6b...21ce0a03c.
The blocking findings are attached inline. Four existing maintainer threads also remain applicable and unresolved, so I am referencing rather than duplicating them:
- persistent log output ignores the
AO_RUN_FILEstate root: #2621 (comment) - URL schemes are matched case-sensitively: #2621 (comment)
MCPConfiglacks the requiredschemaNamesmapping and leaks asDomainMCPConfig: #2621 (comment)- exit cleanup re-reads
process.envrather than retaining the spawn-time mode: #2621 (comment)
PR-wide standards issue: the PR explicitly combines two independent issues, contrary to the repository's “one issue per PR” and focused-change rules.
Non-blocking standards notes from the review: daemon ownership remains raw string state across Go/TS; up is an unclear prompt variable; and several one-call helpers/duplicated append-cleanup shapes conflict with the repository's surgical-change guidance.
Verification performed locally: fresh touched Go package tests, go build ./..., go vet ./..., golangci-lint, API regeneration, frontend typecheck, 26 focused daemon-owner tests, and the complete frontend suite (723 tests) passed. The full Go and race suites both hit the same timeout in unchanged kilocode.TestAuthStatusUnknownWhenKeyOnlyComesFromInteractiveShell; that test passed 3/3 in isolation.
| // (owner "app"). The keep decision is durable: a daemon spawned keep-alive | ||
| // is owner "persistent" and stays unlinked even when the app reopens | ||
| // without AO_KEEP_DAEMON, so closing it does not stop the persistent daemon. | ||
| if (shouldLinkOnAttach(existing.owner)) { |
There was a problem hiding this comment.
[P1] AO_KEEP_DAEMON is silently ignored when an app-owned daemon is already alive. A common transition is: quit the normal desktop app, immediately relaunch it with AO_KEEP_DAEMON=1, and hit the daemon's ~5s shutdown grace window. The run file still says owner:"app", so this branch reconnects the supervisor and the next quit still stops the daemon. The option therefore depends on timing or a prior manual ao stop.
Please handle the app -> persistent transition explicitly (for example, restart/adopt with a durable mode change) while preserving the intentional persistent -> unset behavior, and cover both directions in tests.
| // MCPConfig emits nothing, so an unset config inherits the global MCP set as | ||
| // before. Strict alone (empty Configs) is valid: it means "no MCP at all". | ||
| func appendMCPFlags(cmd *[]string, mcp *domain.MCPConfig) { | ||
| if mcp == nil { |
There was a problem hiding this comment.
[P1] This pins the opposite of issue #2195's priority-one MCP behavior. The issue says workers should default to no MCP and opt in explicitly, but a nil config returns without --strict-mcp-config, so every globally configured MCP server remains available. The new ‘unset inherits global’ test makes that mismatch deliberate.
The issue also contains broader compatibility wording, so please resolve the acceptance semantics explicitly. If least privilege is still the requirement, workers need a strict-empty default plus an explicit way to request inheritance.
| // http(s):// entry maps to --plugin-url (a fetched zip); any other value is | ||
| // treated as a local path and mapped to --plugin-dir. Both flags are repeatable, | ||
| // so one is emitted per entry. Empty/whitespace entries are skipped. | ||
| func appendPluginFlags(cmd *[]string, dirs []string) { |
There was a problem hiding this comment.
[P1] These flags add plugins; they do not scope the set a worker can load. User/project/global plugins and standalone skills remain active, so a configured worker still carries the orchestrator's globally enabled capabilities. That leaves the central least-privilege requirement from #2195 unimplemented.
Anthropic documents --plugin-dir / --plugin-url as session plugin additions, independently of installed user/project/local scopes: https://code.claude.com/docs/en/plugins-reference
Please add an isolation mechanism for other plugin/skill sources, or narrow the feature and PR claims so they do not promise plugin scoping.
| } | ||
| keepDaemonLogFd = undefined; | ||
| } | ||
| if (keep) child.unref(); |
There was a problem hiding this comment.
[P1] Persistent mode can attach a new desktop build to an old daemon after auto-update. quitAndInstallUpdate() exits and relaunches Electron, while this unref plus the exit guard intentionally preserves the old bundled daemon. On relaunch, bundled identity checks compare the executable path—not a build/API version—so the new generated frontend client can attach to the previous backend until the user discovers they must run ao stop.
Please add daemon/frontend version negotiation or explicitly stop/restart an app-spawned persistent daemon during the update flow.
| // second instance would kill the supposedly-persistent daemon. The decision | ||
| // is read only from the daemon's record, never from the current process env. | ||
| it("does NOT re-link a persistent daemon on attach, even when AO_KEEP_DAEMON is unset now", () => { | ||
| expect(shouldLinkOnAttach("persistent")).toBe(false); |
There was a problem hiding this comment.
This covers the important persistent -> unset relaunch, but not the reverse app -> AO_KEEP_DAEMON=1 transition. The current attach path still calls shouldLinkOnAttach("app") === true, so enabling the flag during the existing daemon's grace window has no effect. Please add a test at the attach-decision boundary, not only predicate tests.
| // TestBuildProjectConfigWorkerProfileFlags: the worker MCP/plugin flags are | ||
| // assembled into the per-role AgentConfig that the daemon validates and | ||
| // claude-code maps to --mcp-config / --strict-mcp-config / --plugin-dir[-url]. | ||
| func TestBuildProjectConfigWorkerProfileFlags(t *testing.T) { |
There was a problem hiding this comment.
This bypasses Cobra and only tests the options-to-struct helper. The repository requires command tests for CLI changes; please add an executeCLI/HTTP-capture case that parses the three new flags and verifies the emitted JSON wire shape. That would catch flag registration, repeatability, validation/usage exit behavior, and client/daemon DTO drift.
|
Closing this combined PR in favor of the split, per @illegalcall's one-issue-per-PR feedback (AGENTS.md: "Keep one issue per PR. If asked for separate work, create a separate branch and PR."). The two halves now live as their own focused PRs:
Splitting rather than force-pushing this branch so the inline review comments here keep their line context. Not merging — just dropping the combined scope. Thanks for the thorough review; the findings all landed in the split PRs above. |
4 participants
Combines two independent, non-overlapping changes into one PR (frontend desktop layer + backend config layer; no shared files). Each was previously open separately (#2231 and #2611) and closed by accident when their head branches were removed during a fork cleanup — this re-opens both, with all maintainer review comments addressed.
Closes #2230. Closes #2195.
Part 1 —
AO_KEEP_DAEMON(desktop keep-alive) — closes #2230Opt-in env var: when set, the desktop app spawns its daemon without the OS-native supervisor link and skips the orphan-cleanup kill on exit, so the daemon persists across app quit and stops only on an explicit
ao stop. Default (unset) is byte-for-byte unchanged — the daemon self-stops shortly after the app quits.Beyond "power users with long sessions," the persistent-daemon mode is a prerequisite for a detached / remote topology (a headless daemon in a container, attached from the desktop over an SSH tunnel): without it, closing the desktop window arms-then-fires the supervisor watchdog and kills every running agent session in the container.
Both maintainer (illegalcall) review comments are addressed in the included commits:
running.jsonowner record (\"persistent\"vs\"app\"), never the current Electron process env. A daemon spawned keep-alive stays persistent even when the app is later reopened withoutAO_KEEP_DAEMON.shouldLinkOnAttach(\"persistent\") === falseis encoded as a regression test.keepDaemonLogFdis closed immediately afterspawn()(the child holds its own inherited copy via stdio) and on the spawn-throw path, so each keep-alive start/stop cycle no longer accumulates descriptors.~/.ao/daemon.log(stdin ignored) and the child isunref()'d, so Electron-owned pipes no longer kill the kept-alive daemon on the next stderr log write.keepDaemonAliveuses an explicit allowlist (1/true/yes/on);off/noand any unrecognized string keep the default self-stop behavior.Files:
frontend/src/main.ts,frontend/src/main/daemon-owner.ts(.test.ts),frontend/src/shared/daemon-discovery.ts,backend/internal/runfile/runfile.go,docs/cli/README.md.Part 2 — per-role worker environment profile (claude-code) — closes #2195
Widens the per-role
RoleOverridebeyond{agent, model, permissions}into a full worker environment profile: system prompt, env, MCP servers, plugins — scoped per role (worker/orchestrator), defaulting to inherit-everything so nothing breaks. claude-code ships a CLI flag for each aspect (--append-system-prompt,--mcp-config,--strict-mcp-config,--plugin-dir/--plugin-url).agentconfig.go):AgentConfiggainsSystemPrompt,Env,MCP *MCPConfig,PluginDirs;MCPConfig{Configs, Strict}(Strict alone = isolation, valid).effectiveAgentConfigmerges the new fields — Env per-key via deep-copyingmergeEnv(a regression test pins that it never mutates the project config), and the MCP pointer / PluginDirs slice are copied rather than aliased (same defense-in-depth).buildSystemPromptappends the per-role prompt; roleEnvlayers ontoproject.Envat spawn + restore.appendMCPFlags/appendPluginFlagsemit in bothGetLaunchCommandandGetRestoreCommand(resume rebuilds from flags); both paths callcfg.Config.Validate()for symmetry. nil/empty → inherit global, unchanged.--worker-mcp-config(repeatable),--worker-strict-mcp,--worker-plugin-dir(repeatable); full surface also via--config-json. UI deferred to a follow-up.openapi.yaml+frontend/src/api/schema.ts.Scope of Part 2: claude-code only this pass. codex/other adapters inherit as today (follow-up).
Verification
gofmt+go vetclean; affected Go suites pass (session_manager,claudecode,domain,cli,runfile).tsc --noEmitclean; 26daemon-ownertests pass (incl. the cross-launch and allowlist cases).Note
claude-code requires
--mcp-configvalues in the{\"mcpServers\": {...}}shape (not a bare server JSON) — a user-config detail worth documenting when the UI lands.