5 Commits

Include renames

Author	SHA1	Message	Date
3 people	d2423aa35c	MUL-7590: fix(execenv): support OpenClaw agents.entries per-agent workspaces (#8678) ... * fix(execenv): pin per-agent workspaces under the OpenClaw agents.entries schema OpenClaw 2026.8 retired the agents.list config path and reports it as "Unknown config path: agents.list", moving the agents to a map keyed by agent id under agents.entries. The per-task wrapper only ever wrote agents.defaults.workspace, and an agents.entries.<id>.workspace entry outranks it verbatim: on a host where adding a second agent pinned the gateway agent to an absolute workspace, every task ran in that pinned directory instead of its prepared workdir, so the workdir skills/ never loaded and the agent edited the wrong tree. Read agents.entries when agents.list is gone, pin every entry workspace to the task workdir on write-back, and keep the registry fallback for the generations that have neither config path. The stdout-envelope branch now matches the path-aware wording, so a 2026.8 host that prints its envelope without exiting is read instead of failed closed, and the entries probe shares the agents.list deadline so the worst case stays three CLI deadlines. Co-authored-by: multica-agent <[email protected]> * test(execenv): cover the agents.entries probe, write-back and shared deadline Adds the regressions the review of the earlier attempt asked for: the current "Unknown config path" envelope with err == nil selects the entries probe and never the registry, both the plain and the managed-MCP flows pin every agents.entries.<id>.workspace to the task workdir, a valid-but-unset entries path falls back to the registry, and an envelope naming an unrelated path still fails closed. The worst-case budget test now drives the five-invocation chain and pins the config-schema pair to one shared deadline, so a fourth budget fails loudly. Co-authored-by: multica-agent <[email protected]> * fix(execenv): pin agents.entries workspaces without copying the entry Review feedback on the write-back: `config get` redacts the config before it returns the path it was asked for, so every sensitive per-agent field comes back as `__OPENCLAW_REDACTED__`. The wrapper is a sibling of the user's config in the $include merge and siblings win, so copying the resolved entry would write that sentinel over the user's real secret — memory search API key, web fetch headers, sandbox SSH credentials — for every task on the host, and nothing on OpenClaw's load path restores it. `agents.entries` is an object keyed by id and objects merge recursively, so `{"<id>": {"workspace": "<workdir>"}}` is enough to pin the workspace while every other field still arrives from the user's own config. The same reasoning drops the resolved payload from the row representation: an entries row now carries the id marker alone, so no redacted copy of the user's agent config is held in memory or written to the discovery cache file. Since only the source still decides the write-back, it is now carried explicitly (`openclawAgentsSource`: list / entries / registry) rather than as a boolean that effectively meant "not agents.list", and the source is recorded in the cache entry so a hit rebuilds the same wrapper. The cache shape version is bumped, so an entry written by an older daemon is a clean miss. Co-authored-by: multica-agent <[email protected]> * test(execenv): assert the entries write-back emits workspace alone Review feedback: the tests encoded the old contract (name / model carried into the wrapper verbatim). They now assert the wrapper carries exactly one key per entry — `workspace` — and that nothing else from the resolved payload reaches the file OpenClaw reads. The new regression drives the redaction shapes upstream's own redact-snapshot suite covers (memory search apiKey, web fetch headers, sandbox SSH credentials) through the entries probe of a real preparation, and fails if any of them ends up in the wrapper: with the copying write-back restored it reports the sentinel in `agents.entries.main`. The unit test for the write-back gate also covers a half-marked list, which a quiet per-row filter would have pinned anyway. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: tianshuai1 <[email protected]> Co-authored-by: multica-agent <[email protected]>	2026-09-22 16:22:28 +08:00
3 people	4dfa08917b	fix(daemon): isolate managed OpenClaw MCP servers without reading user config (MUL-6684, MUL-6857) (#7279) ... * fix(daemon): build the managed-MCP config from `config get mcp --json` Preparing a task for an OpenClaw agent with a managed `mcp_config` reads the user's resolved configuration so the wrapper can replace `mcp.servers` while keeping the user's other `mcp.*` settings. That read asks for the resolved root, `openclaw config get --json` with no key path, and current releases do not answer it: measured against 2026.5.27 the CLI writes zero bytes to stdout and never exits, and the same shape is reported against 2026.7.1-2. Every managed-MCP task on such a host therefore fails preparation. `config get mcp --json` is answered, and it is also the only part of the configuration this code needs. Switching to it means the snapshot can no longer be a flat copy of the user's resolved config, so it becomes an include chain: 1. `$include` the user's live config, 2. merge `{"mcp": null}`, which OpenClaw's include merge treats as a wholesale replacement of the user's MCP object, 3. restore the resolved non-server MCP settings. The wrapper then contributes the managed server set, which is the only server definition the chain can yield. `sessionIdleTtlMs` and its siblings still resolve; user-only servers do not. Security posture moves in both directions, so both are worth stating: - Better: the task directory no longer receives a copy of the user's resolved configuration. That copy was a flat snapshot of everything the loader produced, which the code it replaces acknowledged could carry API keys and model-provider tokens, mitigated only by 0o600. The chain writes include paths and MCP tuning, and nothing else. - Worse: because the chain reaches out to the user's real config, the daemon has to grant its directory through OPENCLAW_INCLUDE_ROOTS. The flat copy was self-contained and needed no grant. The trade is a read grant on one directory instead of duplicating that directory's secrets into per-task state, which also means the include never goes stale relative to the user's config. The MCP read uses the same deadline as every other call in the preparation rather than a shorter budget of its own. It is one of the openclawMaxCLICallsPerPreparation calls the budget arithmetic already accounts for, it is the same class of invocation as the discovery calls — measured at 4.3s on the slow host in #7142 — and it has to honour MULTICA_OPENCLAW_CLI_TIMEOUT for the same reason those do. A shorter private budget would fail closed on exactly the machines that raise the override because they need it. `openclawResolvedFullConfig` and `stripUserMcpServers` lose their only caller. The discovery cache's note about deliberately not caching the resolved config is updated: the reason is no longer that the payload holds secrets, but that the MCP subtree is per-agent state that has to reflect the user's configuration as it is now. Tests: the three strict-replace tests move to the MCP subtree and assert the chain rather than a copy, including that the snapshot contains none of the user's server bytes. TestPrepareOpenclawConfigIncludeRootGrants covers the grant for all three include targets, because getting it wrong fails at OpenClaw's include-confinement check with nothing pointing back here. The real-CLI test behind the agentintegration tag asks the installed binary to resolve the generated chain, since the correctness of the reset bridge lives in OpenClaw's merge semantics rather than in the JSON this package writes. * fix(ci): compile the agentintegration tests, and correct the snapshot doc Two review follow-ups on the managed-MCP include chain. The `openclawUserSnapshotFile` comment still described the flat sanitized copy this PR replaced — "the user's config minus the `mcp` block". The file now holds no user config bytes at all, only an include chain, which is also why its 0o600 is about the paths it names rather than secrets it carries. The name stays because the wrapper's include target is not worth churning for a rename; the comment says why. The design's load-bearing claim — that `{"mcp": null}` makes OpenClaw's include merge replace the user's MCP object rather than merge into it — is verified by openclaw_mcp_real_integration_test.go, and nothing compiled that file. `agentintegration` guards nine files across the module and no CI job referenced the tag, so a helper renamed in an ordinary test file, or two branches each defining one in the same package, breaks the tagged build with every untagged job still green. That is not hypothetical here: #6275 adds openclaw_real_integration_test.go to this package with its own `realOpenclawBin`, and dropping it in beside this one leaves `go build ./...` passing while the new step reports vet: openclaw_real_integration_test.go:18:6: realOpenclawBin redeclared vet type-checks test files, so the gate needs no agent CLI and no runner that can execute one. It sits next to Build, before the suite that would otherwise be the first thing to notice. * docs(execenv): scope the root-read claim to what was actually measured Two comments asserted that "current releases" do not answer `config get --json`, citing a report against 2026.7.1-2 as corroboration. The measurement behind them is one host on 2026.5.27, and the citation does not support the claim: the 2026.7.1-2 reports (@kaverjody in #6275, #7308) are both about `openclaw config file` — cold-start timings and a descendant holding stdout — not about the root read. #7308 in fact measures `config file` exiting cleanly in 4.2s on that version. Which command is which is the whole premise of this change: `config get mcp --json` is answered while the root is not, so treating an observation of one as evidence about the other undercuts the argument it was meant to support. Scoped both comments to the measurement, and stated that the root read's behaviour on 2026.7.1-2 is unmeasured. The decision does not need the broader claim: the subtree is the only part of the configuration this code needs, and the fail-open branch being removed is a silent-degradation bug on its own terms, reachable from any cause that makes the read come back empty. * fix(daemon): harden OpenClaw MCP isolation * fix(execenv): correct the root-read diagnosis against measured channels Measured 2026-08-26 on Windows 10.0.19045.6466 against the three npm channels. Two claims in this branch were wrong, and the corrected version is a stronger argument for the change rather than a weaker one. **The root read does not hang; it is not a valid invocation.** `config get` requires a path, and the pathless form is rejected outright: 2026.6.34 (extended-stable) exit 1 in 2.9s Missing required argument "path". 2026.7.1-2 (latest) exit 1 in 3.0s same message 2026.8.1-beta.3 (beta) exit 1 in 4.4s same message, JSON envelope So `openclawResolvedFullConfig` did not degrade on some hosts — it failed preparation for every managed-MCP task on all three channels shipping today. 2026.5.27 writing nothing and never exiting within 60s is one version differing from every current one, and is now recorded as that rather than as the general case. Switching to the subtree stops being a workaround for a hang and becomes the request this code should always have made. **The "exits 0 with empty stdout" shape is unobserved.** It needs exit 0; all three channels exit 1. The fail-open branch removal still stands on its own — it is a silent-degradation path and the new chain has no equivalent — but it is no longer offered as the mechanism behind the reported `Unknown model`, which remains a real symptom with an unestablished cause. Also corrected from the same measurements: - The missing-key transports are not a migration. 2026.6.34 and 2026.7.1-2 are stderr-only with empty stdout; only 2026.7.2-beta.7 onward puts the envelope on stdout. Both paths are load-bearing for channels shipping today, which the detector doc now states with the version table. - `config get agents.list --json` on 2026.6.34 and 2026.7.1-2 emits "Config path not found: agents.list" and names the path, so requiring the diagnostic to name the requested key does not regress the registry fallback on current stable. `agents list --json` answers on all three. - 2026.8.1-beta.3's `mcp` schema is exactly `servers` + `apps` with additionalProperties:false; `sessionIdleTtlMs` is gone. Stage 3 of the chain can only carry keys the same CLI just returned, which is what keeps it safe against that constraint. Comments now use `mcp.apps` as the current example. New regressions, all from verbatim measured output: - the pathless usage error must not read as a missing key, in both the stderr and the JSON-envelope form. This is the shape that actually occurs, and misreading it would turn a broken invocation into a silent "user has no mcp block"; - beta.3's invalid-config envelope (schema violation in a sibling `issues` array) must not read as a missing key either. The smoke workflow now fails when the test skips. A skipped Go test reports `ok` at the package level, so the job would have gone green whenever the CLI was absent or the -run pattern stopped matching — green exactly when nothing was verified. #6897's Windows step documents the same hazard. The real-CLI test's doc records the platform, the three resolved versions and their PASS durations (36.9s / 40.6s / 34.2s), and the hand-run control: dropping the reset stage leaks `user-only` back into the resolved server map on all three channels, which is what makes those passes mean something. * test(execenv): pin the reset-stage/wrapper-mcp pairing the loader depends on The reset stage is only safe because the wrapper always contributes its own mcp object under the same condition: OpenClaw merges sibling keys over the include result, and a null that instead survives to the resolved root fails schema validation (zod .strict().optional() rejects null), taking the whole config load down. The pairing held by construction but nothing named it; this assertion fails in the PR that breaks it, instead of a week later in the scheduled smoke. Co-Authored-By: Claude Fable 5 <[email protected]> * docs(execenv): match the deadline-budget arithmetic to the constant openclawMaxCLIDeadlinesPerPreparation dropped from 4 to 3 when the managed-MCP read step was removed, but the worst-case explanation on openclawCLIMaxTimeout still said 4 x 60s = 4m. Say 3 x 60s = 3m so the documented safety arithmetic matches the constant and its test. Co-Authored-By: Claude Fable 5 <[email protected]> --------- Co-authored-by: weiweiwei <[email protected]> Co-authored-by: Claude Fable 5 <[email protected]>	2026-09-03 12:33:39 +08:00
VvVandweiweiwei	7d128b5a45	fix(daemon): own the openclaw CLI process tree so its deadline is enforceable (MUL-5467, MUL-5630, MUL-6736) (#6275) ... * fix(daemon): own the openclaw CLI process tree so its deadline is enforceable (MUL-5467) openclawCLITimeout has carried a "Known gap (deliberately not fixed here)" note since #6084: the 5s deadline could not actually bound an `openclaw config ...` invocation. This closes it, and fixes a second OpenClaw misbehaviour found alongside it. ## Bug 1 — the deadline was decorative exec.CommandContext kills only the direct child. cmd.Output() blocks in Wait() until the output pipes os/exec manages reach EOF, and cancelling the context does not unblock that io.Copy. So when openclaw leaves a descendant holding stdout — its `openclaw-config` helper, or cmd.exe → node on Windows — the call runs for the *descendant's* lifetime. #6084 measured this on linux/dash (a shim whose backgrounded child slept 6s took 6.01s against a 150ms deadline), tried a cmd.WaitDelay backstop, and reverted it on review because it bounds the call by leaving the descendant running — trading a hang for a process leak that nothing on Unix reaps. The note left behind names the real fix: process-tree ownership. Three call sites had the shape: - execenv.execOpenclawCLI on the task's critical path (Prepare → prepareOpenclawConfig) - agent.detectCLIVersion runs for every provider inside the daemon's blocking registration preflight - agent.discoverOpenclawAgents four invocations per call ## Bug 2 — the answer was already on stdout Measured on an OpenClaw host (openclaw 2026.5.27): openclaw --version 258ms exits cleanly openclaw config file 60s correct path printed, then never exits openclaw agents list 60s correct list printed, then never exits Waiting for exit turns two working commands into a task-fatal error: agent_error.process_failure (prepare execution environment: execenv: prepare openclaw config: locate openclaw active config: openclaw config file: context deadline exceeded (process: signal: killed)) The contract of these commands is "print a value". Once the value has arrived and nothing more is coming, whether the process tidies itself up is not the caller's business, and it should not fail a chat task. ## The fix Two helpers in server/pkg/agent, both built on the process-group primitives codex/claude/opencode already use — no new platform-specific functions: - RunCollect: hands os/exec an *os.File instead of a buffer, so os/exec starts no copy goroutine and Wait returns the instant the direct child exits. Then signals the child's process group, which reaps the helper and releases the last write end, so our own readers see EOF. Unlike WaitDelay this reports the CLI's real exit status, which matters because a failed `--version` probe skips runtime registration entirely, and because openclawShimDiagnostic type-switches on *exec.ExitError. - RunCollectQuiet: returns when the process exits, or when stdout has produced output that then stayed idle for 400ms — whichever comes first. Reports quiet=true on the idle path so callers can log the CLI's failure to exit without failing on it. The idle shortcut deliberately cannot mask a silent CLI: with no output there is nothing to salvage, so the deadline still governs and the call still fails. A non-zero exit still fails too, with stderr intact. execOpenclawCLI keeps its shim diagnostics and its context-attribution order byte-for-byte; only the exec mechanism changes. discoverOpenclawAgents keeps its existing 30s function-level budget rather than gaining a per-attempt one. Windows: there is no process group to signal, so calls are bounded (by the drain grace) but a descendant can still outlive one. Owning descendants there needs a Job Object and is not attempted here. ## Verification - Both packages green with -race; 6 consecutive runs, no flakes. - All 51 existing Openclaw* tests in execenv pass, including TestExecOpenclawCLITimeoutIsNotMisdiagnosedAsMissingInterpreter and TestExecOpenclawCLICancellationIsWrapped — their shims produce no stdout, so the salvage path is not reachable and the errors.Is(err, context.DeadlineExceeded) contract is preserved. - 13 new tests. Both mechanisms are mutation-verified: * RunCollect reverted to buffers with no group reap → the guard test hangs and the dump is the production stack: cmd.Wait → awaitGoroutines blocked, writerDescriptor parked in io.Copy. * RunCollectQuiet's idle path disabled → both the unit test and the execenv end-to-end test fail after a full 60s. * fix(daemon): address review on the openclaw CLI collector (MUL-5467) Three blocking items from review of #6275. All three were reproduced before being fixed, and the first one was a genuine correctness hole rather than a style objection. ## 1. The deadline branch salvaged partial answers RunCollectQuiet returned success from the ctx.Done() branch whenever stdout was non-empty, without requiring the idle grace or any check that the answer was finished. A CLI still streaming when the deadline arrived therefore had its truncated output reported as a completed response. Reproduced with a stub emitting a JSON document forever against a 250ms deadline: 9 runs in 10 came back as success carrying an unparseable fragment. The early return now needs two independent conditions, neither sufficient alone: - an OutputComplete rule supplied by the caller accepts the buffer, and - the buffer has since been idle for idleGrace. Reaching the deadline is never success. The captured bytes are still returned alongside the error so a caller with its own rule can inspect them, but the runner no longer decides on the caller's behalf. A nil rule disables the early return entirely, which is the conservative default for a command whose output shape has no completeness rule. The rule cannot be inferred inside the runner, which is the point: only the caller knows whether `{"agents":[{"id":"a"},` is truncated or legitimate. - `--json` subcommands -> agent.JSONOutputComplete (whole buffer parses). - `openclaw config file` -> openclawConfigPathComplete (last non-empty line looks like a path). Deliberately stricter than openclawParseActiveConfigPath, which resolves a relative line through filepath.Abs and would therefore accept a Doctor warning border as an answer. That matters because OpenClaw prints Doctor UI *before* the path (MUL-3136), so an early return on idle output alone hands back the banner as the config path — and nothing downstream catches it. - `agents list` text fallback -> RunCollect (wait for exit). Decorated text has no completeness rule and a silently short agent catalog is worse than a slow one; this path still gains the reaping and the bounded return. ## 2. quietWriter published bytes before their timestamp The buffer write and the last-write timestamp were updated in separate critical sections, so a reader could observe new bytes carrying a stale timestamp and conclude the stream had gone quiet at the moment it was producing — truncating an answer mid-write. Both now move in one critical section (outputBuffer). ## 3. The drain-timeout path raced the readers, and could park a goroutine RunCollect read its buffers after the drain grace expired while io.Copy could still be appending to them; the deferred Close on the read ends ran only after the return values had been evaluated. Separately, RunCollectQuiet handed os/exec io.Writers, so its cmd.Wait goroutine could sit forever on a descendant holding the pipe — on Windows nothing released it, since there is no process group to signal. Both are now structural rather than patched. A single collector owns the pipes for both helpers (the *os.File ownership RunCollect already had), and finish() guarantees that when it returns, every goroutine startCollector spawned has been joined and nothing can still append to the buffers: it reaps the tree, waits bounded for Wait, and if the readers still have not seen EOF it closes the read ends outright — which makes the in-flight Read return — and only then joins them. ## Verification - Both packages pass with -race under CI's flags (-p 2 -parallel 2). - All pre-existing Openclaw* tests still pass, including the timeout attribution and shim diagnostic ones. - New regression tests, all three named in the review: continuous output until the deadline (must not be success), a prompt followed by a delayed body (must not return the prompt), and a late non-zero exit within the grace (must be reported as the failure it is). Plus the completeness rules per subcommand, the banner-then-path case end to end through execOpenclawCLI, goroutine collection on both helpers, and the two outputBuffer contracts. - Windows contract coverage added (run_collect_windows_test.go) with a scoped step in the existing windows-execenv CI job, since the close-the-read-ends path is unreachable on Unix where the group kill releases the pipe. - Mutation-verified: restoring the unconditional salvage fails both the unit and the execenv regression; dropping the completeness rule from execOpenclawCLI makes the Doctor banner come back as the config path. * test(daemon): make the openclaw config-path fixtures hermetic The two execenv fixtures printed the literal path the host had printed, `/root/.openclaw/openclaw.json`. openclawParseActiveConfigPath stats what it parses and tolerates only os.ErrNotExist, so that path resolves differently by environment: ENOENT on a developer machine, where /root does not exist, but EACCES on a Linux CI runner, where it exists and is unreadable. The second case is a hard error, so TestExecOpenclawCLIWaitsForThePathAfterDoctorBanner failed in CI for a reason unrelated to the boundary it covers. Both fixtures now print a real file inside t.TempDir(), which stats cleanly everywhere. The second test only inspects raw stdout today, but a hard-coded path there would be a trap for whoever next adds a parse to it. The literal path stays in the completeness-rule table, which calls the predicate directly and never stats, so it keeps documenting the shape the CLI actually emits. * fix(agent): own one-shot CLI process trees on Windows * fix(daemon): support current OpenClaw config discovery * Address review, and narrow this branch to the process-tree fix Blocking items from the review: 1. The deadline branch no longer salvages. It returns an error unconditionally, and callers supply a per-subcommand completeness rule (openclawOutputComplete) that decides whether the captured bytes are a finished answer. Regression coverage for the three shapes named in the review: output continuing right up to the deadline, a banner ahead of the real answer, and a late non-zero exit. finish() now reports, rather than being wired to a field that was never assigned. Two cases do not converge and both cost the caller something it would otherwise never learn: a process still outstanding in Wait after the tree kill, and a descendant still holding the pipe when the read ends had to be force-closed, which can truncate trailing output. collectorError joins that with the command's own error, so nothing is displaced. 2. outputBuffer publishes bytes and the write timestamp inside one critical section, so a poller cannot see new bytes with a stale timestamp and conclude the stream went quiet while it was in fact producing. 3. The drain-timeout path closes the read ends and waits for both absorb loops before any snapshot is taken, so no io.Copy can be appending to a buffer the caller is about to read. Windows contract coverage runs as its own CI job. The goroutine guarantee is stated at its real strength instead of "no goroutine outlives the call, on any platform". Convergence holds whenever the tree kill takes. When it does not, one goroutine can stay parked in cmd.Wait on a process nothing can terminate; that residue is not removable here, so it is reported instead of asserted away. On Windows the same case extends the call rather than leaking, because an anonymous pipe is not pollable and closing the read end waits for an in-flight Read to release it. Also from review: - A JSON error envelope is valid JSON, so JSONOutputComplete accepts one and a CLI that prints `{"error": ...}` and then lingers past the idle grace hands back err == nil with the envelope in stdout — where the exit-status path never sees it. All three JSON readers now check for that explicitly. It matters most for `config get agents.list --json`, where the envelope is how a 2026.6+ host says the key moved to the registry: decoding it as data turned a graceful fallback into a failed preparation. For the object-target read the failure is silent, since the envelope would be sanitized and written into the task's snapshot as if it were the user's config. - RunCollectCmd selects on ctx.Done(). It takes a command the caller built, so enforcement must not depend on that caller having used exec.CommandContext; the ordering matches RunCollectQuiet so the two entry points cannot report different error shapes for the same situation. startCollector drops its unused ctx parameter rather than implying it enforces one. - checkOpenclawVersion keeps stderr in the parse. It replaced CombinedOutput(), and a build that prints its version there would otherwise fail the gate on empty output, which skips runtime registration entirely. extractVersionLine is bypassed on purpose: parseOpenclawVersion already scans the whole text, while picking the first line containing a semver lets unrelated stderr noise answer for it. The probe is bounded because the caller's ctx is the task's, and ExplainExecError is applied here too so an ENOEXEC shim is still named (MUL-6164). - TestRunCollectQuietWithoutCompletenessRuleWaitsForExit synchronizes on a marker the CLI writes after its answer instead of a deadline. Its two assertions want opposite budgets, and the reviewer reproduced the resulting flake 3/3 under load. Waiting for the marker also proves more than a timeout could: that the runner was still blocked at a moment when the answer was already fully available. Scope: the MCP work that shared this branch is reverted to main's behaviour and will come back on its own, because it needs a discussion this one does not. It changed which command supplies the resolved MCP settings, restructured the include chain, and moved where the user's config bytes live — none of which the deadline fix depends on. Preparation for a managed-MCP agent behaves exactly as it does on main here. $OPENCLAW_HOME path expansion stays: openclawConfigPathComplete calls openclawHomeRest directly, so the completeness rule for `config file` cannot tell whether that shape is a finished answer without it. * fix(agent): converge the collector's tree kill and stop failing calls on cleanup (MUL-5467) Review follow-up on #6275. All three findings share one root cause: whether the caller got its answer depended on whether cleanup converged, and cleanup convergence depended on a single kill(-pgid) landing. - Retry the tree kill across collectReapWindow. One pass loses a descendant whose fork completes between the kill's enumeration of the process group and the signal's delivery — measured 3 misses in 10 runs of the forking stub, each leaving a `sleep 300` reparented to init and stalling the call until the drain budget expired. The window is deliberately short so it does not widen the pid-reuse window reapProcessTree already documents. - Replace the 5s EOF budget with a 400ms settle. Once the direct child has exited, at most one pipe buffer of its output can still be unread — a child that had written more would have blocked in write() and could not have exited — so a short cap cannot truncate a well-behaved CLI's answer. What it cuts short is a surviving descendant's trailing output, which is not the answer. - Stop reporting cleanup residue as a call error. finish() now logs and returns nothing: a probe whose output arrived and whose helper the kernel would not kill is exactly the case #6084's cmd.WaitDelay got wrong, and a failed version probe skips runtime registration entirely. This drops finishErr, collectorError and the drainForced/waitPending split, which the four call sites were interpreting two different ways by accident. - Fix the reaping tests. They asserted on `$$` captured inside `( )`, which POSIX keeps at the invoking shell's pid, so they named the direct child that cmd.Wait reaps regardless — all of them passed with reapProcessTree removed, while the run leaked 7 orphans. Record `$!` instead, and add TestRunCollectRetriesTheTreeKill, which injects a missed pass because the race lives inside one syscall and cannot be forced from a stub. Verified: collector group 0/10 failures (was 3/10) with 0 orphans, 0/5 under -race, full ./pkg/agent -race green, execenv Openclaw -race green, and both mutations — retry removed, reaping disabled — fail the tests that now cover them, on their own assertions rather than on a cleanup error. * refactor: move the `$OPENCLAW_HOME` expansion out of this branch It is not process-tree work. `openclaw config file` printing `$OPENCLAW_HOME\...` instead of the expanded path is the same class of bug as #6630 — a path the daemon cannot stat, reported as a fresh install, wrapper without the user's `$include` — and #6643 shipped the tilde half of it as its own change. This half belongs there too, not folded into a branch about owning the CLI's process tree. Removed here: openclawHomeRest, its branch in expandOpenclawPath, the unit test, and the completeness-rule row that depended on it. Neither PR now depends on the other. openclawConfigPathComplete therefore does not recognize the variable shape, which is correct in isolation: `main` cannot expand it either, so a rule that accepted the line would hand the caller a path the parser then fails on. Such a host keeps `main`'s behaviour — no rule match means RunCollectQuiet waits for exit — so it loses the hang tolerance and nothing else. The comment on that function says so and names the change that should close it, because a gap nobody wrote down is one a rebase re-opens silently. * fix(daemon): teach the config-path rule the `$OPENCLAW_HOME` shape (#7310 hand-off) #7310 landed the parser side of this, so the deferred half is now due — and it was recorded in the code precisely so a rebase could not drop it silently. While expandOpenclawPath could not resolve `$OPENCLAW_HOME\...`, this rule had to refuse the shape: accepting it would have reported a finished answer and then handed the caller a path the parser failed on. The cost was that a host with that variable set lost the early return entirely and went back to waiting for the CLI to exit — which is the hang this branch exists to survive. With the parser fixed, the refusal was the only thing left standing between those hosts and the tolerance. Resolved through openclawHomeFromEnv rather than read raw from the environment. The value may itself be a tilde path that upstream expands before printing, so `filepath.IsAbs` on the raw string answers false for `OPENCLAW_HOME=~/svc` and silently withholds the early return on exactly the configuration #7310's review was about. Mutation-checked: the raw-env version fails the new `tilde-valued OPENCLAW_HOME` row with `rule(...) = false, want true`. TestOpenclawOutputCompleteRules becomes subtests so a row can carry the environment it is about, and gains three: an absolute OPENCLAW_HOME, a tilde-valued one, and an unset one — the last staying false, so a line with nothing to resolve it against still waits for exit and lets the parser report the real error instead of this rule guessing. * docs(agent): site the collector against outputOwned, not cmd.Output() #7531 moved every one-shot probe in this package onto launch.go's runOwned / outputOwned / combinedOutputOwned, which makes three claims in this branch stale rather than wrong-in-detail: - RunCollect called itself "the safe replacement for cmd.Output()". There are no cmd.Output() call sites left in pkg/agent. The comparison a reader needs is against outputOwned, which owns the same process tree and differs only in who owns the pipes — so that is what the doc compares, including why the probeWaitDelay bound is the right default for the probes it serves and the wrong one here. - RunCollect's path-taking form advertised itself as the migration entry point for "the remaining cmd.Output() call sites (models.go's catalog probes, codex.go, deveco_models.go, thinking.go)". #7531 migrated all four to the owned helpers. It stays as the form for a caller holding only a path, and for a probe that turns out to need pipe ownership. - collector's doc now says outright which of the two properties launch.go cannot provide, since "os/exec owns the draining" now describes in-package code rather than a stdlib call nobody here makes. Also adds the `if: needs.changes.outputs.backend == 'true'` guard the new Windows CI step was missing. Every other step in that job has it; without it the step ran on frontend-only changes. * docs(agent): cite upstream for the hang tolerance, not just the timings The measured "prints its answer and then never exits" for `config file` and `agents list` came from one host on openclaw 2026.5.27, which invites the obvious objection: this is an upstream bug, so why compensate for it here rather than wait for a fix. Read at openclaw v2026.7.1, it is not a bug in a build — it is the exit model. `src/cli/one-shot-exit.ts` provides an explicit opt-in, `requestExitAfterOneShotOutput`, which `runCli` flushes after both streams drain. In the shipped 2026.7.1 tarball exactly three modules take it: `models list`, `models status`, and the hooks CLI. The commands this package invokes — `config file`, `config get`, `agents list` — do not: their success paths only print, and `runtime.exit` appears solely on their failure paths, so termination is whatever the event loop happens to do. A version that exits promptly is not promising the next one will. Also pins openclawHomeRest's citation forward. `runConfigFile` still prints shortenHomePath at v2026.7.1, and resolveHomeDisplayPrefix there still selects `$OPENCLAW_HOME` on a non-empty trimmed variable and `~` otherwise. Worth recording that the separator comes from the original path rather than being inserted by the prefix, since that is what the rest-slice in this function relies on. * fix(daemon): size the config-file idle grace to the real warning gap E2E on a real OpenClaw host (2026.7.1-2, plus 2026.6.8 and 2026.7.2-beta.7) falsified two claims this branch was making. Both are corrected here; one needed a code change. **The 400ms idle grace was sized against the wrong gap.** The claim was that a one-shot response arrives in back-to-back writes, so 400ms is "orders of magnitude longer than the gap between those writes". The first half is confirmed — 0.012-0.033ms between writes. The gap that matters is a different one: with a config that produces Doctor and plugin warnings, `openclaw config file` spread its stdout over 3289ms, of which a single 3285ms silence sat between the end of the warning block and the path. The default grace is eligible to fire inside that silence, on whatever the warnings left in the buffer. Why it lands on `config file` alone: upstream routes incidental console output to stderr when argv carries `--json` (withConsoleLogsRoutedToStderrForJson, src/cli/json-output-mode.ts at v2026.7.1), so every `--json` caller here gets a stdout that is the JSON document and nothing else, and a partial document cannot parse however long the writer pauses. `config file` has no `--json`, so the warnings share its stdout with the answer. openclawConfigPathComplete is still the guarantee — a box-drawing border is not an absolute path — and a grace can only be defence in depth, since the gap is a property of the host's plugin and Doctor work. But it should not be defence that loses by three seconds, so openclawQuietIdleGrace gives `config file` 5s and leaves `--json` on the default. The cost is paid only by a CLI that hangs: `config file` then returns in path + 5s rather than path + 400ms, against a 30s deadline. **RunCollect guarantee 2 overstated what a group kill can reach.** The helper was measured holding the same stdout pipe as its parent (both `pipe:[135202]`) while running with its own PGID and SID. `kill(-pgid)` never reaches that, and neither does #7531's ownership, which uses the same group. "Invoking a CLI on a timer cannot accumulate orphans" was therefore wrong on Unix, and the three comments that repeated it are corrected. What holds regardless is guarantee 1: it is pipe ownership, not the kill, that makes Wait return and the deadline enforceable. Windows is unaffected — a Job Object owns the tree irrespective of process groups. Tests: TestOpenclawQuietIdleGraceCoversTheWarningGap pins the per-command grace and asserts the `config file` value exceeds the measured gap; TestExecOpenclawCLISurvivesTheWarningGapBeforeThePath drives a stub with a warning block, a gap past the default grace, then the path, and requires the path back. * fix(agent): make pipe EOF the completion signal, and bound what is kept Second review round found three blockers on b9b39eaea. All three reproduce exactly as reported, and all three are cases where this branch was *worse* than the launch.go helpers it replaced — which is the bar a replacement has to clear, so the fix narrows the branch rather than patching around them. **1. Leader exit was treated as answer completion (worst of the three).** RunCollect*Cmd reaped the process group as soon as the direct child exited. A wrapper can exit 0 while the real CLI — its descendant, holding the inherited stdout — has not printed yet, which is the normal shape for an npm or PowerShell shim. Measured here: the wrapper exits 0, its child prints the version 500ms later, and this head returned an empty version with a *nil error* in 427ms where outputOwned returns the version in 517ms. Empty-and-nil is the worst available shape, and detectCLIVersion routes all 23 providers through it. Pipe EOF is the only signal that means "no more output is coming", so: - detectCLIVersion, checkOpenclawVersion and discoverOpenclawAgents go back to launch.go's outputOwned / combinedOutputOwned, i.e. back to os/exec's own EOF wait. What this branch still fixes there is the *reporting*: a lingering descendant makes Wait report exec.ErrWaitDelay over an answer that arrived, and a failed --version probe skips runtime registration entirely (#6084 reverted a WaitDelay backstop over exactly that). So salvageProbeAnswer drops the error when, and only when, the caller's own parse found the answer — never on a non-zero exit, never on a deadline. - The collector keeps its reason to exist (execenv's task-critical path) and gains awaitOutputAfterExit: after the direct child exits it waits for pipe EOF, bounded by collectDrainGrace (2s, matching probeWaitDelay), unless the caller's completeness rule says the answer is already in — so a CLI that prints and exits with a helper on the pipe still pays nothing. - RunCollect / RunCollectCmd are deleted. With the probes back on outputOwned they had no production caller, and an exported wait-for-exit collector is exactly the API that invited this bug. **2. A path-shaped warning could be returned as the config path.** openclawConfigPathComplete accepted any absolute-looking last line, and the previous commit's answer — a 5s grace — was the wrong dimension, as the review pointed out by quoting this branch's own comment that no fixed grace can bound the host's plugin work. Reproduced: a CLI printing an existing plugin-cache.json path, pausing 6s, then printing the real path returned the warning path after 5.51s. Fixed by not asking a question content cannot answer. `openclaw config validate --json` reports the path in a named `path` field, on every branch (valid / invalid / file-not-found / error), in every version from v2026.5.5 — which is minOpenclawVersion — through v2026.8.1-beta.3, and `--json` makes upstream route incidental output to stderr so stdout is the document alone. openclawActiveConfigPath asks that first and keeps `config file` as a fallback with no completeness rule at all: it waits for exit and fails closed on the deadline. Upstream runs the same readConfigFileSnapshot() for both commands, so preferring the JSON form costs no extra work; it costs one extra invocation only on a CLI too old for either, which is pinned in TestPrepareOpenclawConfigFallsBackWhenConfigFileUnsupported. openclawConfigPathComplete, openclawQuietIdleGrace and openclawConfigFileIdleGrace are gone. `--json` is now the only shape with a rule, and the doc says why that is the design and not a gap. **3. Retention was unbounded.** absorb appended without limit, where outputOwned keeps the last probeStderrSampleBytes. Measured: 13,107,400 bytes of stderr retained from a CLI writing continuously, ~400x the bound it replaced. stderr now keeps its tail at collectStderrTail (= 32 KiB, so moving a call site between mechanisms cannot change what a failed probe reports) and stdout is capped at collectStdoutLimit and reports overflow as an error — deliberately asymmetric, because a head-truncated answer is how a partial catalog becomes a confident empty one. Every new guard was mutation-verified by restoring the mechanism it covers: | Mutation | Test | Failure | |---|---|---| | collector + reap on leader exit | TestDetectCLIVersionWaitsForAWrapperDescendant | `version = ""` in 0.42s | | awaitOutputAfterExit returns immediately | TestRunCollectQuietWaitsForAWrapperDescendant | `stdout = ""` | | drop the rule short-circuit | TestRunCollectQuietDoesNotWaitWhenTheAnswerIsIn | pays the full 2s | | unbounded buffers | TestCollectedStderrKeepsOnlyItsTail | retained 13,107,417 bytes | | unbounded buffers | TestCollectedStdoutOverflowIsReportedNotTruncated | 524,296 bytes, nil error | | shape rule for `config file` | TestOpenclawActiveConfigPathIgnoresAPathShapedWarning | returns plugin-cache.json | The new fallback test also caught a bug in this commit before it shipped: openclawValidatedConfigPath accepted upstream's `CONFIG_PATH ?? "openclaw.json"` literal, because expandOpenclawPath turned that relative name into a confident path under the daemon's cwd. It now requires the reported value to be absolute before expansion, allowing only the `~` and `$OPENCLAW_HOME` forms. * fix(agent): repair the Windows-only collector test call sites The mechanical conversion of the deleted RunCollect call sites over-widened three tuples in run_collect_windows_test.go, which already used RunCollectQuiet's four results. Host `go build` and `go vet` never compiled them — the file is `//go:build windows` — so the break surfaced on the windows-execenv job as `assignment mismatch: 5 variables but RunCollectQuiet returns 4 values`. Verified this time the way it should have been the first time: `GOOS=windows go build ./...` and `GOOS=windows go vet ./...`, plus darwin and linux. That is the check to run whenever this package's exported signatures move, since a third of its process-lifecycle tests only compile on Windows. * test(execenv): pin the config-path payload shapes E2E measured E2E on OpenClaw 2026.7.1-2 confirmed `config validate --json` as the config-path source and corrected two things this branch had inferred from source. **The exit status is load-bearing in the wrong direction, and this was untested.** Two of the three real states exit *non-zero* while carrying the answer: a missing config file gives `{"valid":false,"path":"…","error":"file not found"}` and an invalid one `{"valid":false,"path":"…","issues":[…]}`, both exit 1 with stderr empty. A fresh install is the missing-file case, so reading a non-zero exit as "no answer" would break first run. openclawValidatedConfigPath already ignored the status, but nothing pinned it — writeOpenclawConfigStub could only exit 0, so the tests covered the rarest branch. It now takes an exit code, and TestOpenclawActiveConfigPathReadsThePathFromANonZeroExit drives all three measured payloads. **Two source-based claims corrected.** - "Preferring the JSON form costs nothing extra" understated it. Measured over three runs each on the same host and config: 1.65/1.66/1.65s for `config validate --json` against 6.36/4.16/4.06s for `config file`. The JSON form is ~2.5-4x faster, which tracks the output it does not render — 715 bytes of JSON against 2663 bytes and 28 lines of warning UI. - The reason a `--json` stdout is clean was attributed to upstream's console-log reroute (`withConsoleLogsRoutedToStderrForJson`). That function exists, but it is not what was observed doing the work: the Doctor and plugin warnings arrive as a `warnings` array *inside* the payload. The distinction matters for why JSONOutputComplete is a real signal — the stream is structurally one document, not merely quieter — so the comments now describe the measurement rather than the mechanism inferred from it. Unrelated and pre-existing: `./pkg/agent`'s ACP timing tests (1-6s wall-clock budgets) flake under load, and which ones fail varies per run. Control run of `origin/main`'s whole package on this machine, same conditions: fails TestCodexExecuteSemanticInactivityAllowsContinuousMessages and TestCodexExecuteLegacyFirstTurnMessageSatisfiesProgress. Not introduced here. * fix(agent): require a recognised version before salvaging a probe answer The ErrWaitDelay salvage in detectCLIVersion was gated on `version != ""`, and extractVersionLine falls back to the trimmed raw output when no line carries a semver token — so any non-empty text satisfied it, including whatever a wrapper printed before the real version existed. Review reproduced it with a stub that prints a banner, exits 0, and has a descendant print the version past the 2s WaitDelay: detectCLIVersion returned "initializing plugins" with a nil error, and logged "CLI answered but left its output pipes open" — the opposite of what happened. That value is then persisted as the runtime version, for every provider, since DetectVersion routes all 23 through here. extractVersionLine now reports whether the semver scan actually matched, and only that answers the salvage question. The trimmed-raw fallback still stands on the success path: a CLI that exits 0 with an unusual version format is reporting its version (#2516), and it is only unusable as evidence that a bounded, incomplete read already holds the answer. Mutation-checked: with the old gate, TestDetectCLIVersionDoesNotSalvageABannerAsTheVersion fails with exactly the reviewed shape — version="initializing plugins", nil error, misleading warn. * test(agent): derive the collected-stdout cap from the largest answer collectStdoutLimit argued for why a fail-closed cap is needed but not for its value, and where it falls is a product compatibility boundary rather than only a memory bound: a legal config past the cap stops a task from starting at all. The derivation is now recorded and executable. Measured answers on 2026.7.1-2 are 715 B (`config validate --json`) and 2663 B (`config file`); the largest answer any call site asks for is the resolved config from `config get --json`, which upstream puts no ceiling on. So the worst case is constructed — 1000 agents and 250 MCP servers with the fields OpenClaw carries, 547,291 bytes, already orders of magnitude past a real deployment — and 8 MiB leaves 15x on top of it while matching what the daemon already holds for a comparable per-task payload (maxLocalSkillBundleSize). Two tests pin it: the constructed high-water mark must round-trip through the collector byte-identically with the stated headroom, and the boundary itself is exact — at the limit the answer is returned whole, one byte past it fails closed with errCollectStdoutTooLarge. * fix(execenv): share one CLI deadline across both path-resolution attempts openclawActiveConfigPath asks `config validate --json` first and falls back to `config file`, and the fallback carried a fresh full deadline. That made the worst case five budgets against a ceiling derived from four: at the 60s override ceiling, 5m of CLI time alone, landing exactly on daemon.defaultTaskPrepareTimeout — so the specific, non-retryable ErrOpenclawCLITimeout collapses back into the generic retryable prepare timeout, which is the outcome openclawCLIMaxTimeout exists to prevent. Both invocations answer the same question, so they now share one context. The multiplier is renamed to openclawMaxCLIDeadlinesPerPreparation because that is what it always measured — the budget is per step, not per invocation. The consequence is deliberate and asserted: a validate that burns the whole budget leaves the fallback none, os/exec reports the expired context on Start, and the failure still carries ErrOpenclawCLITimeout. A CLI that cannot say where its config is within the entire deadline will not answer the same question on a fresh one. TestPrepareOpenclawConfigWorstCaseCLIBudgets now drives the real worst case (validate fails, config file answers, registry fallback, managed MCP) and counts distinct ctx deadlines rather than calls — the old test synthesized a validate success from the `config file` response, so the two never fired in the same run. Mutation-checked: with separate deadlines it reports 5 budgets against a constant of 4. --------- Co-authored-by: weiweiwei <[email protected]>	2026-08-28 14:04:10 +08:00
3 people	728ba9c58e	fix(daemon): stop concurrent tasks evicting each other's OpenClaw discovery cache (#7204) ... loadOpenclawDiscoveryCache rejected any entry stamped later than the reader's clock sample, treating it as a clock jump. Callers sample time.Now() once and pass it down, so on a daemon starting several tasks at once a peer routinely commits a valid entry dated microseconds after the reader sampled — and the reader, whose own store just succeeded, threw it away and re-ran discovery for nothing. TestOpenclawDiscoveryCacheConcurrentPreparations flakes on CI for exactly this reason; it reproduces locally in a few hundred runs. Tolerate a bounded skew (1s) instead of any future dating at all. That is far above the interleaving window and far below the clock jump the guard exists to catch, and it bounds the cost of being wrong: an entry dated ahead outlives its TTL by at most the tolerance. TestOpenclawDiscoveryCacheFutureDatedEntry pins both edges — a concurrent writer's entry is a hit, a jumped clock is still a miss — so the tolerance cannot be widened into 'never expires' or narrowed back into the flake. Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]>	2026-08-19 14:59:40 +08:00
Bohan Jiang	4d5a679b9b	fix(openclaw): unblock the OpenClaw runtime on slow hosts (MUL-6321) (#7142) ... Fixes #7112. Task preparation ran `openclaw config file` and `config get agents.list --json` with a hardcoded 5s deadline; on a 2020 Intel MacBook Pro those take 8.4-10.9s and 4.3s every time, so the OpenClaw runtime was unusable there with no way to raise the limit. - Deadline 5s -> 30s, overridable via MULTICA_OPENCLAW_CLI_TIMEOUT / backends.openclaw.cli_timeout, clamped to [1s, 60s] so the worst case (4 serial calls) stays inside the 5-minute preparation budget. - Config discovery cached per daemon profile (on disk, since preparation runs in a one-shot helper process), invalidated by binary identity, OpenClaw path env, active config stat, and a 60s TTL. Only complete successes are cached; any cache fault degrades to a live CLI run. - The timeout is classified structurally into a new platform-side, non-retryable reason (runtime_cli_timeout) instead of being text-matched into agent_error.provider_network, which showed "check your network" copy and auto-retried a deterministic local failure. Copy added for web/desktop/mobile; a NormalizeDaemonReason shim upgrades reports from older daemons.	2026-08-18 18:33:23 +08:00