mirror of
https://gh-proxy.org/https://github.com/klinsmaya/multica.git
synced 2026-11-08 01:16:45 +08:00
main
105
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
22a05d832b |
MUL-8423 chore(ui-lab): remove the UI Lab design workbench (#9746)
Co-authored-by: Cursor <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
802f0894e0 |
refactor(attachments): generate text preview whitelist (MUL-8248) (#9527)
* refactor(attachments): generate text preview whitelist Co-authored-by: Cursor <[email protected]> * fix(attachments): gate text previews on shared whitelist (MUL-8248) Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Cursor <[email protected]> Co-authored-by: Multica Eve <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
76b274e898 |
MUL-8117: fix(wecom): preserve streaming media on Windows (#9395)
* fix(wecom): preserve streaming media on Windows Co-authored-by: multica-agent <[email protected]> * test(wecom): check closed media files portably Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Sol-Boy <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
ce5a2ba472 |
ci: upgrade Go build toolchains to 1.27.3 (#9420)
Co-authored-by: Yushen <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
Danila Katalshov
|
f66237fbdf | fix(scripts): preserve check failures in exit cleanup (#9032) | ||
|
|
3d021489f4 |
MUL-7697: fix(daemon): keep host TMP/TEMP for every Windows task (#9197)
* fix(daemon): keep host TMP/TEMP for every Windows task Git Bash's /tmp usertemp mount is shared by all live MSYS processes of a user, whichever runtime started them. #8985 stopped Windows Claude tasks from seeding it with a task-private dir, but any other provider still could, and later tasks then lost /tmp once that dir was deleted. Apply the rule to every provider on Windows: TMP/TEMP inherit the host temp, TMPDIR stays task-private, and Claude keeps its CLAUDE_CODE_TMPDIR default. The Git Bash integration test now alternates providers. Closes #8824 Co-authored-by: multica-agent <[email protected]> * test(daemon): expect inherited TMP/TEMP on Windows in env test Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: multica-eve <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
makesomethingshit
|
eb59a69722 | fix(daemon): preserve host temp for Windows Claude Git Bash (#8985) | ||
|
Bohan Jiangandmultica-agent
|
e5d4acfaef |
fix(ci): require Go 1.26.9 security patches (#9143)
Co-authored-by: multica-agent <[email protected]> |
||
|
Bohan Jiangandmultica-agent
|
e31da86c90 |
MUL-7801 fix(daemon): follow directory junctions when authorizing repo checkout workdirs (#8958)
* fix(daemon): follow directory junctions when authorizing repo checkout workdirs (#8946) On Windows, a workspaces root moved to another drive and left behind as a directory junction put a junction in the middle of every task path. filepath.EvalSymlinks fails with ENOTDIR on such a path under the winsymlink semantics Go 1.23+ selects, so the repo checkout authorization added in v0.4.30 refused every checkout as "not owned by the active task", and the workdir-reuse check silently declined every follow-up. Add util.ResolveSymlinks, the strict counterpart of ResolveSymlinksBestEffort: it follows symlinks and junctions and fails on any missing component. On Unix it is filepath.EvalSymlinks; on Windows a junction-free path keeps the spelling EvalSymlinks gives it. Use it in the checkout authorization and both workdir-reuse checks, and log the real reason for a 403 and include it in the response. Co-authored-by: multica-agent <[email protected]> * fix(util): walk extended-length paths in ResolveSymlinks, refuse device-namespace ones evalPath returns a `\\?\` input unwalked and joins the rest of a path lexically onto a volume-GUID link target. That is the fail-closed answer for the best-effort resolver, but ResolveSymlinks feeds a containment check that can compare two device-namespace spellings with each other: with the active workdir spelled `\\?\C:\task\workdir`, `\\?\C:\task\workdir\escape\sub` read as inside it although escape is a junction to another directory, and canonicalSpelling's fallback kept that unwalked string. The strict path now converts an extended-length path with a Win32 spelling and walks it component by component, and returns ErrUnresolvablePath for any input or result that stays in the device namespace. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: multica-agent <[email protected]> |
||
|
|
cf2b84708d |
fix(release): replace assets on same-tag reruns (MUL-7683) (#8812)
* fix(release): replace assets on same-tag reruns Co-authored-by: multica-agent <[email protected]> * fix(release): allow late desktop asset replacement Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Sol-Boy <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
ZIceandmultica-agent
|
7a3f29b4be |
fix(installer): preserve Windows PowerShell 5.1 disk parsing (#8663)
Co-authored-by: multica-agent <[email protected]> |
||
|
VvVandweiweiwei
|
2b62b2b52e |
fix(cli): compare workdir paths in one namespace (MUL-4252, MUL-7193) (#8028)
* fix(cli): compare workdir paths in one namespace (MUL-4252)
The containment guard behind --content-file, --description-file and --attachment
canonicalizes the workdir but could not canonicalize a candidate path that does
not exist yet, and compared the two sides in different namespaces. That is wrong
in three ways, and only the first is merely a wrong answer.
## A path inside the workdir read as outside
filepath.EvalSymlinks fails outright when any component is missing. The fallback
resolved one level up (filepath.Dir) and otherwise called filepath.Clean, which
does not follow symlinks at all.
os.Getwd() returns the LOGICAL working directory whenever $PWD names the current
directory, and everything that writes $PWD carries the path as typed: a shell's
`cd`, and the PWD=<workdir> this repo exports to agent processes
(pkg/agent/opencode.go, deveco.go, codearts.go). So on a host whose workdir path
crosses a symlink, the base was /private/var/... while the candidate stayed
/var/..., and `--content-file reports/summary.md` was rejected as a guardrail
violation when the real problem was that `reports/` did not exist. The only
remedy the error offers is --allow-external-file, so the caller's next move is to
disable the guard and collect an unrelated not-found error on the retry.
## A path outside the workdir read as inside
With an already-canonical workdir the same fallback inverts. `escape/sub/x.md`,
where `escape` is a symlink pointing out of the workdir, read as INSIDE it: the
unresolvable tail fell back to a lexical clean that never sees the symlink.
## ".." was applied to the string, not to the followed path
filepath.Join and filepath.Clean collapse "escape/.." before any symlink is
followed, while the kernel follows `escape` out of the workdir first and then
goes up. So the guard judged a different path than os.ReadFile then opened:
guard says within=true ; os.ReadFile got "another run's file"
That is the MUL-4252 failure this guard exists to prevent — reading a file left
behind by another run — reachable with a file that exists and no race.
## A path on another volume read as an internal failure
Windows only: filepath.Rel cannot relate two absolute paths on different volumes
and returns an error, which surfaced as `Rel: can't make Z:\report.md relative to
C:\Users\...\workdir`. Two absolute paths Rel cannot relate are on different
volumes, which is as far outside the workdir as a path gets, so the guard now
answers that instead.
## The fix
util.ResolveSymlinksBestEffort canonicalizes as far as the filesystem allows: it
resolves the path as given first — the only step that can see ".." the way the
kernel does — then walks up to the deepest existing ancestor, resolves that, and
re-attaches the missing tail lexically. A mismatch inside the missing tail is
inert: a path whose intermediate directories do not exist cannot be opened at
all. Both sides of the comparison go through it.
internal/daemon/execenv already had this algorithm as resolvePathBestEffort,
unexported and documented as Python's Path.resolve(strict=False). It now
delegates, so there is one owner, and the ".." handling makes it more faithful to
that documented contract rather than less.
## Coverage
TestFileWithinWorkingDir runs its table under two working-directory shapes, and
both are load-bearing: under a symlinked workdir an unresolved candidate always
looks outside, so the symlink-escape rows can only fail under a canonical
workdir. Reverting one part of the mechanism at a time:
candidate unresolved (Clean) 5 tests / 15 subtests fail
one-level fallback 3 tests / 8 subtests fail
base side left unresolved 5 tests / 9 subtests fail
clean before resolve ("..") 2 tests / 5 subtests fail
helper walks only one level 4 tests / 9 subtests fail
The lint side is covered too: classifyLocalPathTarget uses the same predicate,
and there the same false negative flips direction — a workdir-local path with a
missing intermediate directory matched neither of its signals, so the link was
published instead of blocked.
cmd/multica has no Windows job, so the guard's Windows-only behaviour ran
nowhere: a candidate on another volume, and a directory junction, which
filepath.EvalSymlinks resolves to its own name rather than descending through it
(internal/daemon/config_windows_test.go asserts that refusal directly). A
junction inside the workdir pointing out of it therefore still reads as inside,
before this change and after it. Failing closed on a component that exists but
cannot be canonicalized would reject every path under a pnpm-style node_modules
junction, and on a host whose workdir is itself reached through a junction it
would reject every --content-file and --attachment on that host — a larger break
than the one it prevents. The windows-tagged test pins the gap with the
observations it rests on and fails with instructions if the platform starts
resolving junctions. Its CI step is scoped and runs with -v so a silent skip is
visible rather than passing as "ok".
* fix(cli): lead with the missing file when a rejected path does not exist (MUL-4252)
The workdir containment guard answers one question and reports it as if it
answered two. A path outside the workdir that does not exist got the same error as
a stale file sitting in /tmp:
--content-file path "/tmp/desc.md" resolves outside the current working
directory; write agent temp files inside the task workdir ... Pass
--allow-external-file to override.
The only remedy that error offers is the flag, so the caller's reasonable next
move is to disable the guard and retry — earning a second, unrelated "no such
file or directory". The diagnosis points away from the actual problem, and the
lesson it teaches an agent is to turn the guard off.
MUL-4252 was about reading a file that exists and belongs to another run. When
the path does not exist there is no such file to protect anyone from, so the
missing file leads, with the containment problem kept as the second clause since
the caller still needs to know the flag would not help.
pathExists treats any stat error other than "not exists" as "exists", so a
permission wall or a broken mount keeps the guard's own wording and stays
fail-closed rather than being reported as a missing file.
Both halves are pinned by mutation: dropping the branch fails
TestExternalFileErrorLeadsWithMissingFile on the missing-file assertion, and
making pathExists report everything as missing fails the same test on the
assertion that an existing external file must still be told about
--allow-external-file.
* fix(cli): preserve Windows path kinds and resolve junctions in the containment guard (MUL-4252)
Addresses the two blockers in the review on 3ec1781e:
1. absNoClean prefixed the working directory onto every non-IsAbs path,
but on Windows a root-relative path (\tmp\desc.md) resolves against
the volume ROOT and a drive-relative path (C:tmp\desc.md) against the
current directory on that drive. The guard inspected
C:\task\workdir\tmp\desc.md while os.ReadFile opened C:\tmp\desc.md,
so an in-workdir shadow could pass an outside read. absNoClean now
splits per platform (path_other.go / path_windows.go) and preserves
the kinds; a drive-relative path on an unobservable drive is handed
back uncanonicalized and fails closed through the Rel branch.
2. The junction case in workdir_guard_windows_test.go pinned a known
contract violation (EvalSymlinks refuses to descend through junctions
under the winsymlink semantics this module ships with). Resolution is
now component-wise on Windows (evalPath) and follows junctions via
os.Readlink — which answers under both winsymlink settings — with a
bounded follow budget; reparse points Readlink cannot speak for
(OneDrive placeholders) stay opaque. A junction out of the workdir is
rejected, one into it (the pnpm shape) is still admitted, and the
known-gap test is replaced by cases asserting both directions plus a
chain and a dead target.
The shared walk in ResolveSymlinksBestEffort is now component-wise on
both platforms: it applies ".." to the resolved prefix even when the
tail is missing (previously the second phase started from filepath.Abs,
which cleans first), and it never depends on what EvalSymlinks does at
a filesystem root. Unix behaviour is unchanged for every shape the
existing tests cover; the new dot-dot-in-the-walk case pins the
difference.
* fix(cli): classify link targets with the full Windows path grammar and resolve unknown reparse points by handle (MUL-4252)
Addresses the two blockers in the review on d3d370377:
1. Link targets were spliced onto the link's parent unless
filepath.IsAbs said otherwise, but the kernel applies the full path
grammar to targets: a root-relative target (\outside) resolves from
the root of the volume the link sits on, and a volume-GUID target
(\\?\Volume{GUID}\...) is absolute in the device namespace. The
resolver now classifies targets through classifyTarget, which reuses
the same grammar as absNoClean; device-namespace targets without a
Win32 spelling are kept verbatim so the containment comparison fails
closed across the namespace instead of splicing a mangled relative
path (the prefix rules mirror internal/daemon's #6883 handling).
2. An os.Readlink failure on a ModeIrregular reparse point no longer
means "does not redirect". Readlink decodes only the symlink and
mount-point tags; other name-surrogates (e.g. container-isolation
links) redirect through a filter while the lexical location stays
behind. Such components are now resolved through
GetFinalPathNameByHandle (zero desired access, so no hydration, and
FILE_FLAG_BACKUP_SEMANTICS for directories); if the handle names the
same path the component is opaque, otherwise the walk continues from
the final path. When even the handle cannot name the location the
resolver returns the new util.ErrUnresolvablePath instead of
guessing: fileWithinWorkingDir treats it as outside (fail closed)
and execenv's delegate falls back to the input, which its
containment comparison rejects.
ResolveSymlinksBestEffort now returns an error so the fail-closed case
is explicit rather than a look-alike path. Negative tests cover a
root-relative symlink target with an in-workdir shadow (the kernel's
target is verified by content before asserting rejection) and a
volume-GUID junction target with the prefix-stripped shadow.
* fix(cli): classify drive-absolute link targets before the volume check (MUL-4252)
classifyTarget asked VolumeName before IsAbs, but VolumeName is
non-empty for a drive-absolute path (C:\dir) too. A junction's absolute
target was therefore misread as drive-relative and resolved against the
working directory — a nonexistent spelling — which errored the
resolution and left the escape to be judged lexically inside the
workdir. The IsAbs branch is asked first, and the load-bearing ordering
is pinned by TestClassifyTarget, a fixture-free table over the target
grammar that would have caught this without a filesystem.
* ci: run the util link-target classification table on Windows
TestClassifyTarget pins the branch ordering classifyTarget's link-target
grammar depends on, but windows-tagged tests in internal/util had no
execution point: the windows-execenv job only ran the guard tests in
cmd/multica. Scoped to the table by name, matching the job's pattern.
* test(cli): correct classifyTarget expectations for root and base shapes
The walk base is the volume root and the working directory's components
live in the segments for a drive-relative target, and the plain-relative
branch returns the link's directory verbatim without a trailing
separator. The expectations said otherwise; the code was right.
---------
Co-authored-by: weiweiwei <[email protected]>
|
||
|
Bohan Jiangandmultica-agent
|
6dbcc257df |
MUL-7547 fix(ci): prune the Go build cache to the run's entries and deflake telegram lease tests (#8624)
* MUL-7547 ci(backend): prune the Go build cache to the run's entries before saving Each main push restored the newest go-build-race entry and saved the whole cache back, so every save carried the previous entry plus the commit's new objects. Go only trims entries unused for five days, so the entry held every recent main commit's race objects: 277 MB on 9/11, 3.5 GB by 9/21. Restoring it took ~2 min and saving it 1-4.5 min, which put backend-tests back at 5-11 minutes, and two such entries filled 7 GB of the repository's 10 GB cache. Mark an epoch an hour before the first compile, and before the save drop every `-a`/`-d` entry no newer than it. Go refreshes the mtime of any entry it uses once that mtime is an hour old, so those entries are exactly the ones this run did not touch; the rule is Go's own trim with the run as the window. Co-authored-by: multica-agent <[email protected]> * MUL-7547 test(telegram): give lease-takeover cases a call budget Postgres can meet Six delivery cases shorten the lease to 100ms so they can really wait out its expiry. callContext sizes each provider call's budget at a third of the lease, and the send claim, a real Postgres write, runs inside that budget: 33ms. On a slow -race CI runner the write took longer, the claim timed out, the reply ended before its first chunk, and TestAuditReplayAfterInterruptionBetweenChunks failed on main. Share one test lease of 600ms (a 200ms budget) and one lapse wait across the cases. Injecting 40ms into the claim reproduces the CI failure with the old lease and passes with the new one. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: multica-agent <[email protected]> |
||
|
Bohan Jiangandmultica-agent
|
53578958bc |
fix(selfhost): pass MULTICA_DINGTALK_SECRET_KEY through and guard integration keys (#8617)
The backend service environment in docker-compose.selfhost.yml is an explicit allowlist, and MULTICA_DINGTALK_SECRET_KEY was never on it: Docker self-hosters who set the key in .env still got a disabled DingTalk integration. Telegram had the same gap until #8611. - Map MULTICA_DINGTALK_SECRET_KEY into the backend service. - Document MULTICA_SLACK_SECRET_KEY and MULTICA_TELEGRAM_SECRET_KEY in .env.example next to the other channel keys. - Extend selfhost-config.test.sh: every key the server loads through secretbox.LoadKey must be mapped in docker-compose.selfhost.yml and listed in .env.example. The list comes from the server, not the docs, because Telegram's key was missing from both. - Run script-checks when server/cmd/server/router.go changes, where the integration gates live, so a new channel key trips the guard in the PR that adds it. Co-authored-by: multica-agent <[email protected]> |
||
|
leilei3167
|
f9f5e3b81f |
MUL-6734 fix(daemon): resolve Windows set-path overrides via LookPath (#7623)
`multica runtime profile set-path` was accepted on Windows but the daemon never used the override: the gate required a Unix executable bit, which Go never sets on Windows files (0666/0444 for regular files; 0111 only on directories). Every override — including a plain .exe — read as not executable, so the daemon fell back to PATH: either failing registration with a message that blamed the user's filesystem, or silently registering a different install than the pinned one. appendProfileRuntimes now resolves the override through resolveAgentExecutablePath (exec.LookPath) — the same contract agent launches use — and records the resolved path rather than the raw override, so Windows PATHEXT completion picks up .cmd shims and extension-less pins. Unix exec-bit checks are unchanged, and a stale or mistyped override still falls back to PATH with the existing warning. Regression coverage runs on the real Windows runner via the existing windows-execenv job. Fixes #7613 |
||
|
|
0cb64a77aa |
feat(ui-lab): add design system workbench (MUL-7425) (#8388)
* feat(ui-lab): add shared component design workbench * feat(ui-lab): support English and Chinese * feat(ui-lab): scaffold design system catalog * fix(ui-lab): cover narrow headers and CI path filtering * feat(ui-lab): add dialog motion and component usage rules * feat(ui-lab): add semantic color workbench * fix(ui): update brand color to #0070E3 * feat(ui-lab): add color usage and live contrast checks * feat(ui-lab): support collapsible sidebars * fix(ui-lab): place collapse controls in sidebar headers * fix(ui-lab): avoid repeating page titles in breadcrumbs * test(agent): synchronize Cursor background finalization fixture * fix(ui-lab): preserve contrast and list scroll restoration (MUL-7425) Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Lambda <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
d18cec01c3 |
MUL-7415 ci: retire the standalone Cursor lifecycle job (#8465)
* MUL-7415 ci: retire the standalone Cursor lifecycle job cursor-background-lifecycle ran a macos-latest and a windows-latest runner on the runtime scope, i.e. on nearly every backend change, to cover one integration's background shell. Neither leg needed to be its own job. Windows: windows-execenv is already a windows-latest job on the same runtime scope testing ./pkg/agent process-tree ownership. The Cursor patterns move there as a step, so a backend PR spins up one Windows runner instead of two. The ./internal/daemon watchdog command does not come along: every test it selected is untagged except cursor_uncaptured_watchdog_test.go, which is `linux || darwin` and never built on Windows, so the ubuntu backend suite already runs all of them under -race. macOS: nothing else in this workflow runs Go on macOS, and cursor_background_process_darwin.go is 208 lines of XNU identity signalling whose two tests are `//go:build darwin`. Deleting outright would leave it with no coverage anywhere, so it becomes macos-runtime, gated on `full` — the daily cron and workflow_dispatch that #8451 added for exactly this trade. Its single-count finalization step is dropped as dead: `full` is now always true here, so the -count=5 stress pass that was already the full-run variant is the only one left. The backend gate tracks both moves; ci-scope's production gate test parses ci.yml, so it verifies the new needs/if/scope wiring directly. Co-authored-by: multica-agent <[email protected]> * MUL-7415 test(agent): stop building the Cursor late-descendant test on Windows TestCaptureCursorBackgroundLateDescendant failed twice in the last 43 main pushes on windows-latest and zero times in 40 on ubuntu. The two Windows failures reported different errors -- `Access is denied.` and `not an owned descendant` -- from the same captureCursorBackgroundProcess call, which is one race wearing two faces: cursorWindowsProcessParents snapshots every PID->PPID pair on the machine, Windows keeps a child's recorded PPID after the parent exits, and that PID is then free to be reused, so the ancestry walk can reach a PID someone else now owns. Narrowing the tag to `linux || darwin` keeps the Linux and Darwin arms, which have never flaked and still cover the whole contract: a descendant forked after capture is owned and terminated, the leader's death does not orphan it, and an unrelated process is never signalled. Windows keeps TestCaptureCursorBackgroundProcessRejectsForeignJob for Job membership. This is deliberately a mute, not a fix. MUL-7417 carries the race itself and restores the tag. Co-authored-by: multica-agent <[email protected]> * MUL-7415 ci: run the Windows Cursor step last in windows-execenv Folding the Cursor job into windows-execenv means Cursor's failures now truncate windows-execenv's own steps, which the separate job could never do. Eight Windows-only checks -- bounded output collection, the Pi session lock, the OpenClaw npm shim, junction resolution -- sat after it and would be skipped by a single Cursor failure. Moving the step behind `Build Windows CLI helper entrypoint` keeps the one-runner saving while leaving the pre-existing coverage first in line. Also adds the -timeout=5m this job's other test steps carry. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
d7f8c3f2b6 |
MUL-7404: ci: scope validation and remove duplicate lifecycle runs (#8451)
* ci: scope validation and remove duplicate lifecycle runs Co-authored-by: multica-agent <[email protected]> * fix(migrations): renumber the Triage validate migration to 489 #8449 and #8436 merged minutes apart and both took 485, so TestMigrationNumericPrefixesAreUnique fails on main and every PR branched from it. #8436 owns 485-488, so the single validate file moves to 489. Renaming an applied migration makes databases that already recorded 485_issue_triage_state_validate run it once more under the new version string. That is safe here: VALIDATE CONSTRAINT on an already-validated constraint is a no-op, verified against PostgreSQL 17. Co-authored-by: multica-agent <[email protected]> (cherry picked from commit 09140a04aea06e8ca1cf008433cad3600d851cef) * fix(ci): gate installers and reuse frontend quality setup Co-authored-by: multica-agent <[email protected]> * docs(ci): remove redundant CI overview Co-authored-by: multica-agent <[email protected]> * refactor(ci): remove unused output and duplicate test fixtures Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Lambda <[email protected]> Co-authored-by: multica-agent <[email protected]> Co-authored-by: J <[email protected]> |
||
|
Dante
|
8b7a4fb68a | test(daemon): isolate local skill homes on Windows (#8338) | ||
|
|
b54e665295 |
MUL-7280: ci(backend): cache race builds per commit and shard pkg/agent onto its own runner (#8302)
* MUL-7280: ci(backend): cache race builds per commit and shard pkg/agent onto its own runner backend-tests never warmed its Go build cache: setup-go shares one go.sum-keyed entry across every Linux job, go-vulnerability-scan saved a 95 MB one first, and the test job race-compiled the whole module from scratch on every run. Own the caches explicitly (modules by go.sum, race build objects by commit SHA with prefix fallback, published from main only), compile build and vet in race mode so the objects are shared, and pin -count=1 so a rolling cache cannot replay results against a changed schema. pkg/agent ran as a 139s serial tail at -parallel 2; it is timer-bound and reads no database, so it moves to its own runner without service containers. test-go.sh gains --only regular|agent for that split; the default still runs both halves. Co-authored-by: multica-agent <[email protected]> * MUL-7280: ci(backend): make backend-tests the sole Go cache writer and key build objects by image backend-agent-tests restored the module cache through the combined actions/cache action with the same go.sum key as backend-tests. Being faster, it saved first with only pkg/agent's slice of the module graph, and backend-tests then failed to reserve the immutable key: the same first-writer-wins problem this change set out to fix. The agent job now restores both caches read-only, leaving backend-tests as the only writer. The build-cache key also gains ImageOS. Cached cgo and race objects are linked against the runner image's glibc, so an entry must not cross an Ubuntu release when ubuntu-latest rolls forward; setup-go keys on ImageOS for the same reason (actions/setup-go#368). ImageOS is a runner environment variable rather than a context value, so each job resolves the key prefix once in a shell step and the restore and save steps reuse it. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
6af2268bd9 |
MUL-7227 test(perf): measure comment typing while agent runs stream (#8276)
* test(perf): measure comment typing while runs stream (MUL-7227) The regression that made typing a comment take tens of seconds was a summary animation replacing a DOM node per streamed message, and nothing we had could see it. The component suite runs in jsdom, which has no style recalculation, no layout and no main-thread contention, so several thousand passing tests said nothing about the thing users felt. #8260 pinned the DOM node reuse; this pins what the user actually waits for. One browser scenario, on the real page: app shell, shared CSS, the ProseMirror composer, inline runs, a production Next.js build. Only HTTP and the WebSocket are synthetic, so no API server, database, daemon, agent or account is involved. Nothing disables animations or trims the DOM — the cost being measured lives in exactly that machinery. Fixed workload, identical for both builds under comparison: 80 comments with prose and code, three running agents, 1000 seeded transcript messages each, 60 ticks of live messages 100ms apart, and 172 keystrokes at 25ms. Messages are scheduled from Node rather than the page, so a stalling build cannot quietly measure less work than the one it is being compared against. `typing_elapsed_ms` is what the user waits for: first keystroke to the editor holding the whole string, main-thread queueing included. Style recalculation, layout, task time and long tasks say where it went. An empty page types very fast, so a run whose messages never reached the UI, or whose composer did not end up holding every character, is reported as `invalid` or `timeout` and never as a duration — including when the scenario blows its budget, which is the case whose numbers matter most. Reports rather than gates. `scripts/perf-compare.mjs` builds and measures two refs in sequence on one machine, each installed against its own lockfile, with the spec, fixture and browser always taken from the running tree so the product is the only difference. The workflow is separate and not required: one sample per ref cannot separate a small regression from machine noise, and the noise floor has not been calibrated on CI hardware yet. Verified against the known regression: restoring the pre-#8260 summary animation moves style recalculation from ~130ms to 1577ms and typing from ~4.9s to 6.2s, against a spread of ~1% across three runs of the fixed build. Disconnecting the message injection reports `timeout: run 0 end marker`, and typing fewer characters reports `timeout: editor content` — neither can pass as fast. #8260's DOM reuse test is untouched and still green. Tests and run configuration only: no product change. Co-authored-by: multica-agent <[email protected]> * test(perf): always build for real when comparing two refs (MUL-7227) The comparison reports a build time, and turbo's cache made it read as ~1.7s when the real build is ~60s — understating what this costs CI by a factor of thirty. Worse, a run where one ref hit the cache and the other missed would put that difference in the timing split as if it belonged to the products. `--force` on both sides: two real builds, two comparable numbers. Co-authored-by: multica-agent <[email protected]> * test(perf): report cached builds instead of forcing real ones (MUL-7227) Forcing a real build on both sides cost minutes on every local run to remove an ambiguity that a single line of output removes instead. A restored build is byte-identical to the one that produced it, so the cache cannot move the numbers this script collects — only the build time it reports, which is now labelled when it came from the cache. It buys nothing on CI either: the runner is cold every time, so both builds are real whether or not they are forced. Locally a repeat comparison drops from roughly three minutes to under one. Co-authored-by: multica-agent <[email protected]> * test(perf): run the comparison on demand, and let it exit (MUL-7227) The comparison never exited on success. The frontend server is spawned detached, and its teardown hung off `process.on("exit")` — but a live child keeps the event loop running, so `exit` never fired and the child was never killed. The report was written and the process then waited on the server it was supposed to stop. On CI that ran until the job was cancelled at sixty minutes; locally it left the servers running after being killed. The failure path only terminated because it happened to call `process.exit(1)`. Teardown is explicit now. Each side stops its server, waits until the process has exited and the port has stopped answering, and removes its worktree before the next side starts — so the second build is also measured on a machine with nothing of the first one running. The script then exits explicitly, and the `exit` and signal handlers remain only as a last resort for cancellation. Checked on every path, with a harness that records the gap between the report and the exit and looks for anything left behind: success, a scenario failure with a server running, an unknown ref, and SIGTERM mid-run all exit promptly with no server, worktree or temp directory remaining. Run through the same harness, the previous script wrote its report, then was still running 247s later and left three servers and two worktrees behind. The workflow is manual only, capped at fifteen minutes. On CI one comparison took 340s — 292s of it the two production builds, 26s the two measurements — which is a lot to spend on every frontend PR for a report that gates nothing. The routine guard stays the component test from #8260, which pins the exact failure mode and runs with the normal suite; this is for changes that touch summary animation, long lists, transcript rendering or global styles. No change to the scenario or its metrics. Co-authored-by: multica-agent <[email protected]> * test(perf): only trust this run's report, and wait for the whole server (MUL-7227) Two ways the comparison could report something that was not true. A report left in the output directory by an earlier run was read back when this run's scenario failed before writing one, so two failed scenarios showed the previous run's timings, were marked a usable comparison, and exited 0. And a scenario that wrote `ok` and then failed was passed through too: the exit code was recorded as `spec_failed` and then ignored. Each side's report is now deleted before the scenario runs, the files this script writes are cleared up front, and a failing scenario process overrides an `ok` report — whatever failed after the numbers were written is exactly what nobody has looked at. And a server that stopped answering was taken for one that had stopped. Any fetch error counted as a closed port, a timeout included, and the server was dropped from the last-resort cleanup before anything had been confirmed. With the pnpm wrapper gone but a child still holding the port, the script exited 0 and left two servers running. Teardown now waits for the whole process group to exit — escalating to SIGKILL if it outlives SIGTERM — keeps the server registered until then, and treats the port as free only when a connection is refused, since a hung server still completes the handshake. `scripts/perf-compare.test.sh` drives the real script with a fake pnpm — no build, no browser, 9 seconds — through success, a scenario that writes nothing, a stale report, `ok` followed by failure, a child that ignores SIGTERM, and an unknown ref. Each of the three new cases fails against the previous script for the reason above. It runs in `frontend-build` like the other script tests; the browser comparison itself stays manual. `PERF_STOP_GRACE_MS` sets the SIGTERM grace (default 10s) so the test can exercise the SIGKILL path without waiting on it. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
378e3bd24b |
MUL-7154: unify the global radius system (#8175)
* fix(ui): unify global radius tokens Co-authored-by: multica-agent <[email protected]> * fix(ui): reduce global radius scale Co-authored-by: multica-agent <[email protected]> * fix(ui): address radius review findings Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Lambda <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
8661e586f6 |
MUL-7063 fix(cursor): bound stalled runs after background shell launch (#8050)
* fix(agent): supervise Cursor background shells with a liveness guard Cursor's stream-json reports a launched background shell as the `isBackground` boolean at the root of its completed tool-call result, and the adapter acted on none of it: a top-level execution that went quiet after launching one kept holding its runtime slot until the execution deadline (upstream #7833). Bound that shape instead of condemning it. The structural observation now arms a Cursor-private liveness guard: a rolling semantic-progress deadline (10 minutes in production, overridable per backend so tests do not have to wait a minute-scale timeout) while execution continues normally. Only a supervised run that stops progressing and never emits its terminal result is ended, and it fails closed with a dedicated, content-free reason rather than a generic cancellation. Precedence is decided when the cancellation happens, not guessed at finalization: fire() declines to claim a run the execution deadline or the caller had already ended, so `timeout` and `aborted` keep their classifications, while a cancellation the guard provoked is never rewritten into "execution cancelled". A valid terminal Cursor result outranks a latched background observation and still completes. Detection is structural only — no reading of the command string, no substring of captured output, no assistant text — and malformed or non-object result metadata leaves the call foreground, so it cannot arm the guard or fail the rest of the tool parse. Raw result bytes are forwarded unchanged. The guard owns one goroutine and one timer per execution, started on the first background observation and waited for before the reader hands over its last channel; the overwhelmingly common foreground run never arms it and pays nothing. Tests run cross-platform by re-executing the test binary as a fake cursor-agent, so the pathological stream stays alive and only the daemon can end it. Test A is RED on unpatched main, where the same run is bounded solely by the 6s execution deadline with status "timeout". Co-authored-by: multica-agent <[email protected]> * fix(agent): keep the Cursor liveness guard on its own evidence Cross-verification of 15605057e found three places where the guard acted on weaker evidence than the spec allows, plus one helper that did not do what it documented. - cursorSemanticProgressEvent believed every `tool_call` subtype. Execute handles `started` and `completed` and counts anything else as unhandled, so a stream of subtypes it drops could extend the deadline forever. Progress is now limited to the subtypes that are actually handled. - The lifecycle observation read `isBackground` off any completed tool result. The flag the spec names lives on the shell payload, and another tool reporting it is not evidence that Cursor detached a process, so the read is gated on the `shellToolCall` key. An unrecognised payload key now leaves the run unsupervised, which is the pre-existing behaviour, not a new failure. - Precedence between a native terminal result and the guard is now decided when the result is observed. A result that lands after the guard ended the run is late evidence: it no longer rewrites the failure to `completed`, its text is not exposed as output, and the run keeps the guard's own reason. A cause the guard declined to claim — a real deadline or a caller cancellation — still leaves the result authoritative. - cursorBackgroundGuard.Stop closes its channels through sync.Once, matching the idempotency its doc claims, and returns the guard's cause as of that moment, which is what makes the ordering above decidable at all: after fire() the guard's cancellation is otherwise indistinguishable from the caller's. Tests: the guard's own contract (fire-once, progress extension, yielding to real causes, Stop's report, concurrent Stop) is unit-tested directly; the observation rules are pinned by structural table cases and by two subprocess fakes — a stalled non-shell `isBackground` must still end on the execution timeout, not the guard. Co-authored-by: multica-agent <[email protected]> * chore(agent): clarify Cursor fake dispatch comment in TestMain Co-authored-by: multica-agent <[email protected]> * fix(cursor): recheck observed progress before liveness cancellation Record semantic progress under the guard mutex and recheck its deadline when handling timer or progress notifications, so a stale tick cannot cancel an active run. Keep the background-liveness failure content-free. Add a deterministic virtual-time regression for the expiry boundary and strengthen the noisy subprocess regression to check the exact error. Validation: focused regressions fail before the fix and pass after it; Cursor/ParseCursor tests pass with -race; go vet ./pkg/agent passes. Co-authored-by: multica-agent <[email protected]> * fix(cursor): supervise background tools with the daemon tool budget Co-authored-by: multica-agent <[email protected]> * fix(cursor): retain background ownership and refresh watchdog state Co-authored-by: multica-agent <[email protected]> * fix(cursor): validate unreaped Darwin group anchors Co-authored-by: multica-agent <[email protected]> * fix(cursor): distinguish empty Darwin groups from lost ownership Co-authored-by: multica-agent <[email protected]> * test(cursor): isolate reviewed baseline regression checks Co-authored-by: multica-agent <[email protected]> * test(cursor): stop platform checks at the first failure Co-authored-by: multica-agent <[email protected]> * fix(cursor): verify Darwin process identities during cleanup Co-authored-by: multica-agent <[email protected]> * test(cursor): use standard agent limits on macOS Co-authored-by: multica-agent <[email protected]> * test(cursor): keep lifecycle reproductions ahead of full suite Co-authored-by: multica-agent <[email protected]> * test(cursor): keep macOS CI focused on lifecycle Co-authored-by: multica-agent <[email protected]> * chore(ci): drop the PR-specific Cursor regression reproduction steps The two steps gated on `github.event.pull_request.number == 8050` proved the new regression tests were RED on earlier heads of this PR. That is useful while the PR is open and dead weight once it merges: the condition can never hold again on main, and both steps pin commit SHAs that only exist on a closed PR's branch. The permanent coverage stays: the lifecycle job still runs on ubuntu, macOS and Windows, and macOS still repeats the finalization test and checks the cgo-free ownership path. Co-authored-by: multica-agent <[email protected]> * docs(daemon): state what a zero tool budget means for a background shell Keeping a Cursor background shell in flight for the launched process's whole lifetime is what puts a legitimate long background job on the tool budget instead of the shorter idle one. It also changes what MULTICA_AGENT_TOOL_WATCHDOG=0 means in practice: such a run is now bounded only by MULTICA_AGENT_TIMEOUT, which is 0 by default. That follows from the invariant rather than contradicting it — a foreground tool that never returns already behaves this way — but an operator reading "never force-stop during a tool call" would not predict it, so say it where the knob is defined. Co-authored-by: multica-agent <[email protected]> * test(daemon): pin the captured-background zero tool budget outcome TestCursorBackgroundUncapturedDisabledToolWatchdog covers the case where nothing was claimed, and releases the tool so the idle budget still applies. Its mirror was untested: when ownership IS held the process is genuinely in flight, so a zero tool budget deliberately force-stops nothing. Without this test the two cases look like an inconsistency and invite a later "fix" that quietly changes what the knob promises. Co-authored-by: multica-agent <[email protected]> * fix(daemon): let an observed terminal result outrank the idle watchdog A Cursor run could still be reported as a hang after it had already succeeded. On reading its authoritative result the backend called background.Close() synchronously, before Session.Result reached the daemon. That call takes the tracker lock, so it can queue behind a tool-watchdog Interrupt already in progress — and that Interrupt is allowed to return false without moving native accounting or its timestamp. The watchdog's revalidation then saw an unchanged count, an unchanged timestamp and an empty queue, cancelled the run, and executeAndDrain re-tagged the completed result as idle_watchdog. The missing piece was a lifecycle boundary the daemon could see: cleanup succeeding is not the same fact as the outcome being decided, and only the latter should silence a liveness policy. Session.TerminalObserved carries it, the Cursor backend publishes it before any cleanup that can block or fail, and the watchdog checks it both before interrupting and immediately before force-stopping. The re-tag at Session.Result is gated on it too, so even a watchdog that wins the last instant cannot rewrite a decided outcome. Both regression tests drive the exact interleaving through channel handshakes rather than sleeps: the cleanup callback itself publishes the terminal observation, so the terminal result provably lands while cleanup is in progress. They fail on the parent commit with 'fired=true' and 'Status:idle_watchdog' respectively. Co-authored-by: multica-agent <[email protected]> * docs: sync the Cursor background watchdog contract to ja/ko/zh The English page gained three operator-facing contracts that the other three locales still described with the pre-change one-liner: that a Cursor background shell stays in flight until its owned processes exit, that normal finalization stops those processes, and what the per-platform fail-closed limits are. These decide whether an operator sets an overall timeout, sets a tool budget, and expects a background server to survive a run, so they cannot ship in one locale only — the same watchdog settings are already maintained in four languages. Also states explicitly, in all four, that a zero tool budget leaves claimed background work in flight for as long as it lives, bounding the run only by MULTICA_AGENT_TIMEOUT, which is itself 0 by default. Co-authored-by: multica-agent <[email protected]> * fix(daemon): keep a decided outcome through the drain-timeout classifier The terminal boundary was checked in the watchdog and on the Result arm, but not on the one path a losing race actually takes. The watchdog reads terminalObserved and only then writes fired and cancels, so Cursor can publish its terminal result in between. The cancellation that follows does not surface on the Result arm — the backend is still finalizing and cannot deliver Result yet — it surfaces on drainCtx.Done(), which classified purely on the fired flag and returned idle_watchdog with an empty output for a run that had already succeeded. Check the boundary there too and wait, bounded, for the real result. Falling through on expiry keeps the previous behaviour, so the backstop cannot hang a run; the backend bounds its own finalization, which is what makes the wait short in practice. The earlier regression test could not reach this: there the watchdog returned at its final gate, so fired was never set and neither the rewrite guard nor this classifier ran. The new test makes TerminalObserved publish on the read that the watchdog's final gate performs while still answering false to that reader, which is the losing race exactly, and asserts the watchdog really did cancel before checking the outcome. It fails on the parent commit with Status:idle_watchdog and an emptied output. Co-authored-by: multica-agent <[email protected]> * fix(agent): bound the whole of Cursor background cleanup at close Each process termination was bounded, but the number of background shells a run may launch is not, so finalization as a whole was not. That matters only after a terminal result — precisely when the daemon's watchdog has deliberately stopped supervising the run and MULTICA_AGENT_TIMEOUT is 0 by default, leaving nothing else to bound it. Give Close() one budget for the whole pass. Work still unconfirmed when it expires takes the existing unconfirmed-at-close path, so its launch result is still surfaced and its cleanup is still logged as unconfirmed rather than reported as successful. Reap and Interrupt keep their previous behaviour. The test drives 40 tools whose termination never confirms; it takes ~8s on the parent commit and stays inside the budget here. Co-authored-by: multica-agent <[email protected]> * docs(ja): match the repo's existing wording in the Cursor watchdog contract Uses 最終結果 as elsewhere in the docs, makes the two signalling clauses say シグナル送信 explicitly, and names プロセス捕捉時 and the background process rather than the launch. Co-authored-by: multica-agent <[email protected]> * fix(daemon): linearize the post-force-stop verdict on the result hand-off Checking terminalObserved before classifying only moved the window: the backend can publish between that read and the classifier that follows it. Reading a flag and then acting on it cannot be made safe by adding more read sites. Enter the hand-off unconditionally once the watchdog has fired, and decide from what arrives. The result's delivery is then the linearization point, and the backend contract — publish the observation before sending Result — is what makes the check after delivery reliable rather than lucky. The Result arm already read in that order; this brings the drain arm in line. An expired budget still linearizes to the liveness verdict, so the branch stays bounded whatever a backend does. The previous test could not reach this: its backend sent Result immediately on cancellation, so the outer select could satisfy itself on the Result arm and the drain classifier never ran. The new one withholds Result until the daemon has entered the hand-off, using the hand-off's own log record as both the gate and the evidence that the branch executed — otherwise a passing status proves nothing about which arm produced it. On the parent commit it fails with 'the post-force-stop hand-off never ran'. Co-authored-by: multica-agent <[email protected]> * fix(agent): bound every locked background-cleanup pass, not just the closing one Close() took its deadline before the lock, which is correct, but the deadline could not shorten the wait for it. A concurrent Interrupt() still scanned an unbounded number of tools with no deadline of its own, so an unbounded pass held by anyone else was an unbounded Close() one level down — and interrupt-holds-the-lock-while-the-terminal-result-arrives is the exact topology this boundary exists for. Bound every pass that holds the lock by the same budget. If waiting for the lock consumed it, the closing pass takes a fresh one rather than pushing every tool onto the unconfirmed path without trying; two budgets is the honest ceiling, and terminalResultHandoffBudget is derived from exactly that. The existing test had no concurrent holder, so it could not see this. The new one puts an Interrupt() pass on the lock first: it takes ~1.1s on the parent commit against a 100ms budget. Co-authored-by: multica-agent <[email protected]> * fix(daemon): scope the result hand-off to backends that publish a boundary Entering the hand-off on every force stop taxed the case the watchdog exists for. A wedged backend has no result that could outrank the stop, so waiting for one only delays freeing the runtime slot — the three existing idle-watchdog latency tests caught this immediately, each taking the full budget instead of one window. Nil TerminalObserved is the signal and is now kept distinguishable rather than collapsed into a false-returning shim at the point of use. Backends without a boundary keep exactly their previous behaviour. Cursor, which has one, always closes Result, so even a wedged Cursor run ends this wait through the closed channel rather than the budget; the budget remains as the bound for a backend that neither delivers nor closes. The new test pins the fast path so the hand-off cannot quietly become a tax on every force stop again. Co-authored-by: multica-agent <[email protected]> * test(agent): prove the interrupting pass holds the lock before Close asks The gate was closed just before calling Interrupt(), which only says the goroutine was about to try. Close() could win the lock, finish on its own, and the test would pass even against an unbounded Interrupt() — a regression test that can go green without exercising what it guards. Close the gate from inside the first process's terminate() instead: finish() holds mu for the whole pass, so reaching it is proof the pass owns the lock. The ordering is now a fact rather than a scheduling habit. It did not misbehave in practice — with the old gate the test still failed 110/110 against an unbounded Interrupt(), including at GOMAXPROCS=1 and 2, because close() does not yield the P. It is now deterministic by construction: 20/20 at GOMAXPROCS 1, 2 and 8. Also corrects the handoff budget derivation: closing the reaper's stop channel wakes it immediately, so its tick is not a term. What deserves the margin is an already-started termination overshooting its pass's deadline by its own bound. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: worker-opencode <[email protected]> Co-authored-by: multica-agent <[email protected]> Co-authored-by: J <[email protected]> |
||
|
|
603519d158 |
MUL-6977: test: remove implementation-only coverage (#7932)
* test: remove implementation-only coverage Co-authored-by: multica-agent <[email protected]> * test: remove remaining implementation-only coverage Co-authored-by: multica-agent <[email protected]> * test: restore compatibility and fragment navigation coverage Co-authored-by: multica-agent <[email protected]> * test: preserve behavioral cleanup contracts Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Sol-Boy <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
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]>
|
||
|
|
438a30ef00 |
MUL-6902 fix(agent): fix Pi v0.4.37 Windows session regression (#7843)
* fix(agent): avoid locking Pi transcript on Windows Co-authored-by: multica-agent <[email protected]> * test(pi): cover sidecar lock contention and release * test(ci): run the Windows Pi session-lock regression tests The windows-execenv job runs ./pkg/agent through an explicit -run allowlist, and the ubuntu backend job cannot compile a windows-tagged file. Without this step the two tests added for #7840 would never execute in CI, leaving the same coverage gap that let the v0.4.37 regression ship. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: worker-opencode <[email protected]> Co-authored-by: multica-agent <[email protected]> Co-authored-by: J <[email protected]> |
||
|
mikyan
|
fe59262188 |
feat(agent): add Huawei Cloud CodeArts support (#6985)
* feat(agent): add Huawei Cloud CodeArts support * fix(migrations): resolve CodeArts prefix collision * fix(agent): address CodeArts review feedback * fix(agent): tighten CodeArts integration scope * fix(agent): resolve CodeArts follow-up review blockers * fix(agent): redact CodeArts launch arguments * docs: sync supported runtime counts * fix(agent): own CodeArts model probe process tree |
||
|
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]> |
||
|
|
bdcb231ba0 |
MUL-5466 fix(agent): deliver the qwen prompt on stdin, not argv (#7343)
* fix(agent): route qwen launch through the Windows invocation chooser On Windows npm installs qwen as qwen.cmd, a batch shim. Spawning a .cmd goes through cmd.exe, whose %* forwarding re-tokenises the raw command line, so the `-p <prompt>` value and the `--output-format stream-json` flags buildQwenArgs puts behind it are mangled whenever the daemon's structured chat prompt carries newlines, quotes or `&`. Qwen then runs without a usable prompt or without stream-json, exits 0 after emitting a single unparseable stdout line, and finalizeStreamResult reports "qwen stream ended without terminal result". A short manual `qwen -p "hallo"` never hits the mangling, which is why the CLI looked healthy in isolation. Mirror the existing cursor/pi/copilot fix instead of inventing a new mechanism: resolve qwen.ps1 next to the .cmd and invoke PowerShell with `-File <ps1>` directly so Go hands each argv token whole, staying on the official launch path. qwenBackend.Execute now builds its process through Command.execVia(chooseQwenInvocation, ...) rather than Command.exec; on macOS/Linux the chooser is a passthrough and behaviour is unchanged. Fixes #6082 * test(agent): cover qwen's stdin prompt delivery and update signatures buildQwenArgs dropped its prompt parameter in the prior commit (the prompt now goes on stdin, not argv), so the existing unit tests needed their call sites and assertions updated to match — including an explicit check that "-p" never appears in argv again. Adds TestQwenBackendDeliversPromptOnStdin: a round-trip regression guard proving a prompt containing embedded double quotes, an em dash, and shell metacharacters reaches the child process byte-for-byte via stdin, the way cursor-agent and Pi already do (#5649's fix). The fake qwen script now optionally captures stdin to a file the same way it already captures argv. * test(agent): cover qwen Windows PowerShell launch Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
8d2da6b8db |
MUL-6479 feat(dev): make a local environment a named object with one verb per lifecycle step (#7325)
* MUL-6479 feat(dev): make a local environment a named object with one verb per lifecycle step Local development had six entry points, thirty-four make targets and no way to see or delete what they created. `make up` / `status` / `list` / `down` / `destroy` / `gc` / `env-exec` replace that with one set of verbs, shared by humans and agents, with `C=api,web,daemon,desktop` selecting components. Three defects made the old flow untrustworthy rather than merely verbose. 1. The database the tooling created was not the one the application reached. `ensure-postgres.sh` creates through `docker exec`; when a native PostgreSQL owns 5432 the container never binds the host port, so the create lands in a server the backend never talks to. It printed "✓ PostgreSQL ready" and the next step died with `SQLSTATE 3D000`. On the machine this was written on the two servers had already diverged: 94 same-named databases, 69 more in the container with no counterpart. `up` now creates through DATABASE_URL and treats a successful `migrate up` — which pings that same string — as the only proof; failure diagnoses the port owner instead of reporting a missing database. 2. Identity was computed, not allocated. Ports, database names and CLI profiles all came from `cksum($PWD) % 1000`, and a collision was silent: the loser fails to bind while the winner keeps answering 200. Allocation now happens under a lock, probing from the same path hash so a checkout keeps its familiar numbers, and requires the slot to be both unregistered and free of a live listener. The manifest in ~/.multica/dev/envs/<name>/ is what every later command reads. 3. Creating an environment left no record of how to destroy it, so there was no destroy — only archaeology. `down` stops processes and keeps the data; `destroy` consumes the manifest: processes, database, profile, slot. `list` makes leaks visible and `gc` collects environments whose checkout is gone or whose TTL passed. /health now reports pid, commit and started_at. A 200 alone proves something is listening, which is exactly what let a failed restart keep serving the previous build; `up` compares started_at against its own launch and refuses an answer that predates it. Two constraints are enforced rather than documented: the daemon is started from a built server/bin/multica, never `go run` (the daemon re-execs its own path for every task, and `go run` deletes that binary when the launcher exits), and `C=daemon` under a .multica/daemon_task_context.json marker stops with that explanation up front instead of spending a login on a refusal. scripts/dev-env.test.sh covers the registry verbs against a throwaway MULTICA_DEV_HOME with no services running, and pins the regression that made `down` exit 1 after reporting success: on bash 3.2 a command substitution whose function ends in a failing command aborts the script under `set -e`, and "no process is listening" is that function's normal answer. `make dev` is unchanged and still runs in the foreground. Co-authored-by: multica-agent <[email protected]> * fix(dev): close local environment lifecycle gaps Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Lambda <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
520359a2c9 |
MUL-6518 fix(daemon): reclaim orphaned per-task temp dirs in the GC (#7391)
* refactor(daemon): give the lock and dir-stat helpers neutral names openEnvRootLockFile, releaseEnvRootLock and codexStoreStat are all generic: they take a path, and nothing in them knows about env roots or Codex stores. The next commit reuses the same execution lock for per-task temp directories, where the old names would read as a bug at the call site. No behaviour change. Co-authored-by: multica-agent <[email protected]> * fix(daemon): reclaim orphaned per-task temp dirs in the GC Every task gets a private temp dir (multica-task-*) under the system temp base, exported to the agent as TMPDIR/TMP/TEMP. Its only exit was the RemoveAll deferred at the end of runTask, which does not run when the daemon is killed and does not succeed while a file inside is still open — the Windows case reported in #7364, where one machine had accumulated 174 leftover directories. The GC never covered them: it scans WorkspacesRoot, and the temp base is a different root, so multica-task- had exactly one create site and zero reclaim sites. Sweep the temp base as a fourth out-of-root pruner. Liveness is decided by the directory's .task_lock — the same OS advisory lock an env root uses — not by age: the sweep removes a directory only once it has itself acquired the lock the owner would still be holding, which the kernel grants only after that owner is gone. Using the lock rather than a marker or a TTL is what makes this correct rather than probabilistic. It is held by an open file description, so it answers for a live task in this very process as well as one owned by a different daemon sharing /tmp, which no in-memory active set could see. And it needs no upper bound on task runtime — there is none to assume, since DefaultAgentTimeout is 0. Directories left by daemons predating the lock carry no lock file, so age is the only signal available for them; they are reclaimed after MULTICA_GC_TASK_TEMP_LEGACY_TTL (72h). That branch exists to clear the existing leftovers and can be dropped in a release or two. The end-of-run RemoveAll stays exactly as it was, and nothing was added to the task-completion path: the run releases the lock before removing, so a removal that fails there costs nothing and the next GC cycle finishes the job. Co-authored-by: multica-agent <[email protected]> * fix(daemon): keep the temp dir marker when cleanup cannot finish Two fixes from review. 1. A failed cleanup destroyed the marker the next sweep needs. os.RemoveAll keeps walking after a failure, so removing a directory whose payload is still open deleted the .task_lock sitting next to it and then returned the error. The directory came back as one with no marker — indistinguishable from a pre-lock leftover, so it was no longer reclaimable on liveness at all. That is exactly #7364's failure path, and it made the claim this change is built on ("a failed removal costs nothing, the next cycle retries") false. RemoveTaskTempDir removes contents first and stops at the first one it cannot remove, leaving the marker behind; the lock file goes last, once everything else is gone. Both the task-completion path and the GC use it. A cleanup that fails now leaves a directory that is still lock-bearing and still unlocked, so the next cycle takes the lock and retries — for as many cycles as the holder needs. 2. The legacy TTL could delete a live task's TMPDIR. Pre-lock directories were reclaimed on age after 72h by default, but age cannot establish liveness: DefaultAgentTimeout is 0, so a task may legitimately run far longer than any TTL; a daemon on another profile can still be running the old binary during an upgrade; and every daemon on the machine shares one temp base. The default is now 0 — leave them alone — and reclaiming them is an operator's explicit opt-in. Each cycle logs how many were left in place so the choice stays visible rather than silent. Classification also fails closed: only os.ErrNotExist means "no lock file". Any other stat error (another user's dir under a shared /tmp) now reads as in-use instead of legacy. Regressions for both: a Windows test that makes a real sharing violation with CreateFile minus FILE_SHARE_DELETE, asserting the marker survives the failed cleanup and the directory goes once the handle closes (wired into the Windows CI job, which runs targeted filters rather than whole packages); and unix equivalents driving the same paths through an unlinkable payload, plus fail-closed coverage for an unreadable directory. Co-authored-by: multica-agent <[email protected]> * fix(daemon): publish the temp dir marker atomically, restore it on rmdir failure Two more windows in the marker's lifecycle, from review. 1. A directory could be swept between marker creation and lock acquisition. LockTaskTempDir created .task_lock and locked it as two steps, so there was an instant where the marker existed unlocked. A sweep landing there reads exactly what a dead owner leaves behind: it takes the lock, concludes the owner is gone, and deletes the directory — after which the starting task's own lock call succeeds and it runs with a TMPDIR that no longer exists. The marker is now locked under .task_lock.claiming and renamed into place once held. Rename within one directory is atomic and the lock rides on the open file description, not the name, so the marker becomes visible and held in the same step. Before that rename the directory carries no marker, which classifies as legacy — and legacy only reclaims what is OLDER than legacyTTL. A directory created microseconds ago is younger than any positive TTL, and a TTL of 0 disables the branch outright, so no configuration can sweep the pre-publish state either. 2. A failed rmdir left a directory with no marker. The marker has to be deleted before the directory can be removed, so a failure at that last step — a Windows process holding a handle to the directory, or content an orphaned child recreated — left a directory nothing could classify again. With the legacy branch disabled by default that is a permanent leak, and it was the one path where cleanup got furthest. The marker is now put back when that removal fails, so every failure mode leaves the retry available. Regressions: the publication order and the mid-claim on-disk state (spared under a disabled TTL and under positive ones), a sweeper running flat out against 200 publish/use/cleanup cycles, and the rmdir-failure branch — which fails without the restore, verified by removing it. Co-authored-by: multica-agent <[email protected]> * fix(daemon): decide the legacy sweep on content, not on age alone The publication window was still open. The claim-then-rename ordering stopped .task_lock from ever existing unlocked, but a directory mid-claim carries no .task_lock at all, so the sweep still reaches it through the legacy branch — and the argument that protected it there was wrong. That argument was "a directory created microseconds ago is younger than any positive TTL". MULTICA_GC_TASK_TEMP_LEGACY_TTL takes any duration, down to a nanosecond, and an owner stalled between MkdirTemp and the rename outlasts a large TTL just as easily. Reproduced at 1ns, 1ms, 1h and 72h alike: the sweep deleted a directory whose claim was held. Age cannot separate a pre-lock leftover from a directory being published, so stop asking it to. Content can: a directory being published holds nothing but its own lock files, because the task has not been handed the path yet and cannot have written anything, while a leftover worth reclaiming is by definition one with something in it. The legacy branch now reclaims only directories that hold task content, which covers both halves of the window — the empty directory before the claim exists, and the claim-only directory after — at every TTL value, with no timing assumption left in the argument. The cost is that a pre-lock daemon which crashed before its task wrote anything leaves an empty directory nothing reclaims. That is a few bytes against deleting the temp dir of a task that is starting. Tests: the mid-claim cases now run at 1ns/1ms/1h/72h with now pushed PAST the TTL, so they fail if anything reintroduces a timing argument; the pre-claim (empty directory) half is covered the same way; and the invariant itself is pinned directly, so relaxing it breaks a test rather than a race. The existing legacy TTL test now models a real leftover by giving it content. Verified all of these fail with the content rule removed. Co-authored-by: multica-agent <[email protected]> * fix(daemon): re-check the lock immediately before removing a legacy dir The legacy branch's decision is several separate observations — classify, then walk for content, then walk for mtime — and a directory can finish publishing in the middle of them. Classified as legacy while its marker was still unpublished, then published and written to by its task, and the removal went ahead without ever looking at the marker that had appeared: the sweep deletes a live task's TMPDIR. What was accidentally holding this together was that the task's own write refreshes the mtime — a timing crutch, in the branch whose whole point is not to rely on one. Re-read the classification immediately before removing, and skip unless it is still legacy. That closes the window rather than narrowing it, because of what content means: a task only writes into the directory after it has been handed the path, which happens only after LockTaskTempDir returns and therefore only after the marker is published and held. So content observed at the top of the branch, plus no marker seen at the bottom, proves no live task owns this directory — and none can arrive later either, since MkdirTemp only ever hands out a fresh empty directory under a new name. The regression drives that exact interleaving through a barrier: the sweep commits to removing the directory, the owner then publishes and its task writes, and the sweep must notice. `now` is far past the TTL so age cannot mask it. Verified it fails without the re-check. Also from review: drop a stale test comment that still claimed a positive TTL can only be configured far above a directory's age, and record in the config and operator docs that a directory holding no task content is never reclaimed on age — including a shell left by a CURRENT daemon that died between creating the directory and publishing its lock, not just an old daemon's empty leftover. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
53e15b35c6 |
MUL-6502: fix startup recovery during PostgreSQL outages (#7372)
* fix(startup): retry transient database outages Co-authored-by: multica-agent <[email protected]> * fix(startup): address database retry review Co-authored-by: multica-agent <[email protected]> * test(startup): address retry review nits Co-authored-by: multica-agent <[email protected]> * fix(startup): preserve native pgx connect timeout Co-authored-by: multica-agent <[email protected]> * fix(startup): apply timeout fallback to services Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Sol-Boy <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
1ea40db1c4 |
fix(build): honor GOOS as a Make variable when naming build outputs (#7309)
#7271 derives the Windows `.exe` suffix from a parse-time `go env GOOS`, which cannot see GOOS passed as a Make variable: `make build GOOS=windows` still emits PE binaries named `bin/server`, `bin/multica`, `bin/migrate` — the exact artifacts #7255 reported as unable to re-exec themselves. The top-level `export` puts the Make variable in the recipe's environment, so `go build` targets Windows while the name says otherwise. Reading `$(GOOS)` first covers both invocation forms, and making the assignment target-specific keeps the toolchain probe on `build` alone: a global assignment is expanded on every make invocation — `export` expands even a recursive one — so `make help` / `make clean` printed `go: Command not found` on checkouts without Go. Adds scripts/makefile-build.test.sh to pin both invocation forms, the non-Windows names, and the no-Go case, wired into the backend CI job that already runs the other Makefile-adjacent shell tests. Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
Bohan Jiangandmultica-agent
|
f7abcdded3 |
docs: publish the community-maintained areas and their maintainers (#7249)
Until now, "community-maintained" appeared in the docs without ever naming who maintains what, and the actual maintainers were recorded only in Go package comments and PR threads. Users filing an issue had no way to learn any of it. Add a Community-maintained areas page in all four locales listing each area, its code path, its maintainers, and the retirement rule, and make that page the single source of truth: channels, the DingTalk and Telegram guides, the README, and the three package doc comments now point at it instead of restating it. Those package comments no longer carry maintainer handles or a second copy of the policy, so ownership changes are recorded in exactly one place. The page is deliberately a credit-and-expectations page, not a support directory — it asks readers to file a GitHub issue rather than page a volunteer directly, since what we promised these maintainers is that we would route work to them, not that they are on the hook for it. It also avoids the term "code owner": we are not adding a GitHub CODEOWNERS file, and that term carries a specific automated meaning there. Both issue forms gain a required Area field so a report can be routed without asking the reporter to read source, with the community-maintained areas marked as such in the option list. Co-authored-by: multica-agent <[email protected]> |
||
|
|
801c671973 |
chore(assets): convert bitmaps to WebP, dedupe, and gate image size (MUL-6352) (#7149)
* chore(assets): convert web and docs bitmaps to WebP The repo carried 21.7MB of PNG/JPG and zero modern formats. next/image already serves visitors AVIF/WebP, but the raw masters are still paid for on every clone, every Docker build context, and every deploy upload. Re-encode with cwebp and cap the source width at the largest size any layout can request: 1920px for the decorative landing backgrounds (they render behind content under object-cover), 2640px for the landing hero (2x its 1320px slot), 1600px for screenshots (2x the ~720px docs and use-case column). EXIF goes with it — the four feature backgrounds were 144dpi print exports. Encoder choice is per image rather than uniform. Most compress best as lossy; two of the docs screenshots are flat palettized PNGs that encode smaller with `-lossless -z 9`, so they use that instead. Screenshots are q80 to keep UI text crisp, decorative art q72. apps/web/public 10.70MB -> 1.57MB apps/docs/public 7.47MB -> 2.44MB Verified: 1:1 crops against losslessly-downscaled references show no visible difference; all 266 image references in the tree resolve; docs prerenders all 177 pages and the landing/use-case pages render with no broken images. Co-authored-by: multica-agent <[email protected]> * chore(assets): drop the duplicated README hero image docs/assets/hero-board.png was byte-for-byte identical to the docs site's workspace-overview screenshot — 633KB stored twice. Only the two READMEs referenced the copy, so point them at the canonical file instead. Co-authored-by: multica-agent <[email protected]> * chore(assets): losslessly recompress app icons oxipng -o max --strip safe over the app icon PNGs: -640KB with pixels untouched (verified by comparing lossless-WebP encodes of the before and after of every file — all 16 identical). Deliberately not switched to WebP and not regenerated: the prerendered sizes under apps/desktop/build/icons/ exist because electron-builder's auto-derivation silently shipped only the 1024x1024 source in the v0.2.31 .deb (#2515), and the PWA manifest and .icns/.ico containers want PNG. Filenames, dimensions, and the electron-builder config are unchanged, so packaging behaves exactly as before. Co-authored-by: multica-agent <[email protected]> * ci: gate pull requests on a 300KB image budget Keeps the previous three commits from being undone one screenshot at a time. A bitmap added — or grown — past 300KB fails the check until the PR description carries an `Oversized image exemption:` line saying why. Shrinking an oversized file always passes: the check compares against the size at the base ref, not just the absolute size. Path-filtered to PRs that touch a bitmap, so the full-history checkout it needs to diff against the base is only paid for when it is relevant. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Lambda <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
Bohan Jiang
|
4040345e09 |
perf(ci): path-filter the backend jobs to skip irrelevant PRs (MUL-6389) (#7198)
Adds a `backend` path filter to the `changes` job and gates backend-tests, go-vulnerability-scan and windows-execenv on it, so a PR that never touches server/ no longer pays for a `go test -race` run, govulncheck and a Windows runner. The filter is deliberately wider than `server/**`: four Go tests read three files outside the module (packages/core/runtimes/cli-version.ts, scripts/agent-cli-command-names.txt, scripts/go-test-with-agent-cli-guard.sh), and each must be listed or the PR that breaks such a contract is exactly the PR that skips the test. Steps are gated rather than jobs, matching the existing frontend/sqlc-check pattern: the aggregate check reports a genuine green instead of staying pending, and still fails when a job goes missing for a reason other than the path filter. |
||
|
|
41ddf6c0a1 |
refactor(frontend): drop dead UI components and phantom deps, add knip (MUL-6353) (#7147)
* refactor(frontend): drop dead UI components and phantom deps, add knip `.npmrc` sets shamefully-hoist=true, which hides both halves of dependency drift: an undeclared import still resolves, and an obsolete declaration still installs. Neither build, typecheck, nor lint reads the import graph, so package.json stopped being the source of truth for what is actually used. Removed 16 zero-reference components from packages/ui (1,907 lines, all `pnpm ui:add` output that never got wired up) plus mention-hover-card.tsx, which was written but never imported — member and agent mentions render as a plain `.mention` span. The comment in issue-hover-card.tsx claiming otherwise was stale; corrected to match rich-content.tsx. Deleting pagination.tsx orphaned its two i18n keys, dropped from all four locales and from the `ui` namespace type. Dependency declarations removed where nothing imports them: - vaul, embla-carousel-react — only the deleted drawer/carousel used them - date-fns, @tanstack/react-query-devtools — zero imports repo-wide - motion, tw-animate-css from packages/ui — real users are views/desktop/web - 44 of apps/web's 58 declarations, including all 15 @tiptap/* entries. One had already drifted: apps/web pinned tailwind-merge ^3.5.0 against the catalog's ^3.4.0, a second source of truth for the same package. Added @types/hast to packages/views, which imports it directly. knip enforces this going forward, scoped to files/dependencies/unlisted — `exports` is unusable here because ui and views are consumed through per-file exports maps. The CI step runs in warn mode (continue-on-error) and reports 9 pre-existing findings left out of this cleanup; clear those before promoting it to blocking. Verified: build, typecheck, lint green across web/desktop/ui/core/views. 6,503 tests pass; the 9 failures are pre-existing on a clean tree (a local jsdom localStorage issue). Web dev server boots and serves /login 200. Co-authored-by: multica-agent <[email protected]> * fix(ci): cover the packages/ui blind spot knip cannot see Review caught that knip provided no regression protection for the exact thing the preceding cleanup was about. knip derives entry points from a workspace's package.json `exports` — graph/build.js calls `getEntrySpecifiersFromManifest` unconditionally, with no config flag to opt out — and packages/ui exports four wildcards. Each expands to a glob over its whole directory, so every file under components/ui, components/common, markdown, and hooks registers as an entry point and can never be reported unused. A zero-reference probe component confirmed it: knip's output was byte-identical with and without it. `entry: []`, negated `entry` patterns, `--production`, and `--strict` were all tried. Manifest entries come from a separate code path that none of them subtract from. packages/views is unaffected because 44 of its 45 exports name a specific file, which is why knip does flag dead files there. Added scripts/check-ui-wildcard-exports.mjs to cover those four directories, blocking from the start since it reports nothing once the one file it found is gone. It reads the wildcards from the manifest, so narrowing an export hands that directory back to knip without the two overlapping. Its test asserts all three states: clean tree passes, a stranded component fails and is named, and importing that component clears it again. The check found packages/ui/hooks/use-auto-scroll.ts — 80 lines whose only occurrence repo-wide was its own definition. Same category as the 16 components already removed, and invisible to knip for the same reason. Also from review: - The path filter missed knip.jsonc and apps/docs/**, so config-only and docs-only PRs skipped knip even though it analyzes docs. - The knip step paired `continue-on-error` with `|| true`, which zeroed the exit code before Actions saw it — no warning surfaced, only log text. Dropping `|| true` makes the warn phase visible and the later promotion to blocking a one-line change. - Corrected the knip.jsonc comment that claimed apps were the only entry points for all three shared packages. True for views and core, never true for packages/ui. Verified: build, typecheck, lint green (12/12, 0 errors). 6,503 tests pass; the same 9 pre-existing failures as before, reproduced on a clean tree. Co-authored-by: multica-agent <[email protected]> * fix(ci): resolve import paths in the ui export check instead of matching names Review found the check passed a dead component whenever any file elsewhere in the repo imported a same-named module. The relative-import branch was a whole-repo text regex on the basename, so `../components/option-card` in packages/views vouched for an untouched packages/ui/components/ui/option-card. That is not a corner case: 17 of the 59 guarded files already share a basename with another file in the repo — `button`, `card`, `input`, `label`, `avatar`, `tabs`, `switch`, `skeleton`, `separator`, `theme-provider` among them. shadcn names are generic by construction, so the hole covered the most likely names a future `pnpm ui:add` would produce. Specifiers are now resolved against each importing file's own directory and compared against real paths, with `@multica/ui/...` mapped through the same manifest wildcards the candidate list is built from. Liveness is now reachability from outside the guarded directories rather than a reference count, so two dead components importing only each other no longer vouch for one another. Also implements the exemption the failure message promises: a file named by a non-wildcard export is an entry point in its own right and is skipped. The candidate loop previously read the wildcard directories unconditionally, so following that advice would not have cleared the check. Tests cover all five states: clean tree, stranded component named, cleared by a real import, the option-card basename collision, and a mutually-importing dead pair reported as two files. Verified: check and its tests pass, knip unchanged at 8 files + 1 dependency, build/typecheck/lint 12/12 green. Co-authored-by: multica-agent <[email protected]> * fix(ci): parse imports with the TypeScript parser, not a source regex Review found the scanner counted commented-out imports as real edges. It ran a regex over raw source, so a component kept its liveness from a line that does not execute — and "delete the last real import, leave the comment behind" is a normal step in a refactor, which makes the false edge appear at exactly the moment the component stops being used. Edges now come from real syntax nodes: static import/export declarations, `import()`, `require()`, and `import("x")` type nodes. `vi.mock("...")` is excluded on purpose — it names a module without importing it, so a component whose only remaining mention is a test mock is dead, and counting it would let one outlive its last real use. Scope, measured rather than assumed: 52 specifiers in this repo appear only in comments or prose, spread over 43 files, so the mechanism is live. None of them currently resolve into packages/ui, so no dead component is being missed today — this closes a latent hole, not an active miss. (An earlier count of 3 was wrong: those were `lazy(() => import(...))` calls that my throwaway measurement script failed to treat as real.) Test gains a sixth state: a component referenced only by a commented-out import and a string containing one must still be reported. Verified: six-state test passes, clean tree passes in 0.9s over 2,089 files, knip unchanged at 8 files + 1 dependency, build/typecheck/lint 12/12 green. `typescript` is already a root devDependency, so no new declaration. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Lambda <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
a1cbd7c2ef |
MUL-6354: tighten build and generation guardrails (#7150)
* chore(build): tighten repository hygiene guardrails Co-authored-by: multica-agent <[email protected]> * chore(ci): remove snapshot ignore check Co-authored-by: multica-agent <[email protected]> * chore(ci): address repository hygiene review nits Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Sol-Boy <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
b946f3a729 |
MUL-6290: split views tests onto a dedicated runner (#7072)
* perf(ci): isolate views tests on a dedicated runner Co-authored-by: multica-agent <[email protected]> * perf(ci): shard views tests across runners Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
d0bd6683f1 |
MUL-5733: keep Go security patches current (#6409)
* fix(release): keep Go security patches current Co-authored-by: multica-agent <[email protected]> * fix(release): address vulnerability review Co-authored-by: multica-agent <[email protected]> * fix(release): restrict vulnerability bypass Co-authored-by: multica-agent <[email protected]> * fix(release): require Go 1.26.6 Co-authored-by: multica-agent <[email protected]> * docs: align Go prerequisite with security floor Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Eve <[email protected]> Co-authored-by: multica-agent <[email protected]> Co-authored-by: Sol-Boy <[email protected]> |
||
|
YYClaw
|
e9528c722a |
MUL-5855: feat(dev): add worktree database cleanup (#6560)
* feat(dev): add worktree database cleanup * fix(dev): handle database cleanup cancellation * fix(dev): protect nested current worktree |
||
|
|
e7e01638ad |
MUL-6118: fix(agent): own Codex process trees with a Job Object on Windows (#6897)
* fix(agent): own Codex process trees with a Job Object on Windows Windows has no process groups, so configureProcessGroup was a no-op, waitProcessGroupGone always reported false, and signalProcessGroup killed only the direct child. That child is usually a cmd.exe wrapping a .cmd shim, so the real tree — the Node wrapper, the native app-server, and the sandbox helpers under them — was left running as orphans. Assign the started process to a Job Object with KILL_ON_JOB_CLOSE, which every process it later creates inherits. Termination now targets the job, emptiness is read from job accounting, and ownership is dropped only once the tree has been reaped. The process handle is held for the lifetime of the entry so Windows cannot recycle the pid out from under the key. Assignment failure is not fatal: cleanup degrades to the previous kill-the-child behaviour, and the cleanup_confirmed gate keeps suppressing initialize retries on any host where ownership could not be taken. Refs #6883 Co-authored-by: multica-agent <[email protected]> * fix(agent): close the pre-attach window and stop keying trees by pid Two defects in the Job Object ownership added by the previous commit. Windows grants job membership only to processes created after the assignment, never retroactively. Assigning after cmd.Start left a window in which the direct child — usually a cmd.exe wrapping a .cmd shim — had already spawned the real agent outside the job. That is worse than owning nothing: the job then holds a wrapper that exits immediately, so accounting reports the tree empty while the escaped app-server is still running and cleanup is reported as confirmed. startOwnedProcessTree now creates the child suspended, assigns it, and only then resumes it, so it cannot create anything before it belongs to the job. A child is never left suspended: ownership that cannot be taken is logged and the child runs unowned, and a child that cannot be resumed is killed and reported. Ownership was also keyed by pid. A pid is unique only while its process is alive, so a launch that outlived its process could look up — and terminate — an unrelated task that inherited the number. Key by the *exec.Cmd of one launch instead; signalProcessGroup and waitProcessGroupGone take the command on both platforms. The tests assert job membership through the job's own accounting rather than through timing, so an escaped descendant fails deterministically. Refs #6883 Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
b5fdbd56f9 |
MUL-6118: fix(daemon): keep npm .cmd agent entry points launchable on Windows (#6895)
* fix(daemon): keep npm .cmd agent entry points launchable on Windows GetFinalPathNameByHandleW returns the extended-length form, so every Windows launch target became \\?\C:\... . That prefix is required past MAX_PATH, but cmd.exe rejects it — and an agent installed through npm is entered via a .cmd shim that Windows can only run by handing the path to the command interpreter. Drop the prefix whenever the plain Win32 path can stand on its own, and keep it when it is what makes the path nameable or when the remainder is not drive-qualified. Existing Windows coverage stages only real .exe files, which hides the difference entirely, so add the .cmd shim shape at both canonicalExecutablePath and the launch boundary. Refs #6883 Co-authored-by: multica-agent <[email protected]> * fix(daemon): measure Windows path length in utf-16 code units MAX_PATH counts UTF-16 code units and includes the terminating NUL, but the check used len(string), which counts UTF-8 bytes. Every non-ASCII character costs more bytes than code units, so a short path under a CJK user directory was over-counted, judged too long, and kept an extended- length prefix it did not need — reintroducing the .cmd launch failure for exactly the users least likely to be tested against. Encode to UTF-16 and compare against MAX_PATH directly, and cover a 100-character CJK path: 323 UTF-8 bytes but only 124 code units with its NUL, so the old rule kept the prefix and the new one does not. Refs #6883 Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
Jamison-J
|
d582dca062 |
MUL-6025: fix(daemon): resolve Windows installer junctions (#6802)
Windows standalone Codex exposes its installer bin directory as an IO_REPARSE_TAG_MOUNT_POINT junction, which filepath.EvalSymlinks cannot traverse, so the daemon launched the shim path and Codex could not find its sandbox helpers. Discovery now keeps the stable entry point and the final target is resolved per launch via GetFinalPathNameByHandleW, so installer retargets take effect while old releases remain installed. Resolution failures fail closed on both the initial and rediscovery paths. Refs #6762 |
||
|
|
a1da8e536f |
MUL-6000 fix(daemon): link user Codex skills into codex-home instead of copying them (#6742)
* fix(daemon): link user Codex skills into codex-home instead of copying them Every Codex task got a full recursive copy of ~/.codex/skills/ inside its per-task CODEX_HOME. For a user with npm-backed skills that is ~100 MB per task directory, re-copied on every task start — hydrateCodexSkills wipes and re-seeds on both the Prepare and the Reuse path — and never reclaimed: the GC artifact sweep only matches node_modules / .next / .turbo, and whole-directory reclaim needs the issue to reach a terminal status first. Total cost scales with the number of live task directories. Link instead. The same per-task home already links its other two shared resources, sessions and the plugin cache, and a skill directory is read-only input to the CLI, so the write isolation a copy incidentally bought has no requester. EvalSymlinks still runs first, so an installer-shipped symlink (lark-cli into ~/.agents/skills/) resolves to the real directory rather than producing a link to a link. The three paths that could reach through the new link are unchanged in behavior: os.RemoveAll deletes a link and not its target, writeSkillFiles probes with os.Lstat so a workspace skill colliding with a linked user skill lands in a `-multica` sibling, and the GC refuses to walk into links. Tests pin all three, plus a windows-tagged test for the junction form of the link. Co-authored-by: multica-agent <[email protected]> * fix(daemon): stop GC walks from descending into Windows directory junctions The GC's artifact sweep and its size accounting both refuse to follow links out of a task directory, but they tested for os.ModeSymlink alone. That misses the link shape Windows actually produces: createDirLink falls back to a directory junction (mklink /J) whenever os.Symlink is denied, which is the default without Developer Mode, and since Go 1.23 os.Lstat reports a junction as ModeDir|ModeIrregular with no ModeSymlink bit while its DirEntry still answers IsDir() == true. filepath.WalkDir therefore descends straight into the link target, and the sweep would delete a node_modules / .next / .turbo directory that lives inside the user's real home. Match both mode bits in all three walks. The task directory links user content in three places today — the Codex session store, the plugin cache, and now the user's skills — so this is reachable on any Windows host running Codex tasks. Windows-tagged tests create the junction with mklink /J directly, so the junction shape is covered even on a runner where symlinks are permitted, and they run in the existing windows-execenv CI job. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
734ad51c93 |
MUL-5774 fix(daemon): isolate Git metadata for Windows Codex checkouts (#6565)
* fix(daemon): isolate Git metadata for Windows Codex checkouts Codex's sandbox resolves a linked worktree's gitdir to the shared .repos cache and keeps it read-only even when the task workdir is an explicit writable root, so `git add` / `git commit` fail from inside a `multica repo checkout`. Linux was fixed by moving those tasks to a task-local clone (#2925); Windows native sandbox still took the linked worktree path and hits the same wall (#6449). - repoCheckoutModeFor now returns isolated for Windows Codex as well as Linux Codex. The MULTICA_REPO_CHECKOUT_MODE -> CLI -> daemon -> WorktreeParams.IsolatedGitMetadata chain already carries the mode, so no new API surface is needed. - The isolated clone passes --no-hardlinks on Windows. NTFS hard links only exist within one volume and every link shares one underlying file and security descriptor, so linking would both break a cross-drive cache/workdir pair and let the sandbox re-permission the daemon-owned cache's objects. Objects are copied there instead. The shared cache stays read-only; this does not widen ACLs, add a writable root, or lean on safe.directory. Co-authored-by: multica-agent <[email protected]> * fix(daemon): correct Codex sandbox facts and prove Windows checkout in CI Review follow-up on the Windows isolated-checkout fix. The source map claimed both Linux and Windows sandboxes keep a linked worktree's external gitdir read-only. That is not what the daemon does: execenv's codexSandboxPolicyFor defaults BOTH platforms to danger-full-access, so only a user who opted into windows.sandbox actually trips the read-only gitdir. Linux keeping the isolated checkout is a retained layout, not a live workaround. Agents read this file to decide whether their checkout can be committed to, so it must not contradict the sandbox policy. Corrected the source map, the stale "(Linux Codex)" comment in partial_clone_test.go, and the repoCheckoutModeFor doc — which now also records why the layout is a per-platform choice rather than a per-policy one. builtin_skills_test.go gains "Linux and Windows Codex" and "task-local Git metadata" anchors; the previous assertions covered only generic repo wording and would not have caught this platform contract regressing. Verified the anchor fails when the SKILL.md sentence is reverted. Windows evidence: cache_windows_test.go asserts on a real Windows host that the gitdir resolves inside the task checkout, that branch/add/commit succeed, that the shared cache base ref is unchanged, and that objects are private copies rather than NTFS hard links — plus a cross-volume checkout, which is what a hard link cannot express at all. Wired into the existing windows-execenv CI job, which exists for exactly this class of "only a real Windows host proves it" regression. The shared no-hardlink assertion is now one helper used by both the cross-platform and Windows tests. This does not replace a manual smoke under a real Codex elevated sandbox identity; CI cannot assume that identity. Co-authored-by: multica-agent <[email protected]> * ci: scope the Windows repo-checkout step to the windows-tagged tests The step used a prefix match, which also selected the package's cross-platform isolated-checkout test. That test cannot run on Windows: repocache derives a cache directory name from the full source repo path, so a t.TempDir() path already embedding a long test name doubles and exceeds MAX_PATH, and git fails with "Filename too long". It belongs to the ubuntu backend job, which runs the whole package. Both windows-tagged tests already passed in the failing run, including the cross-volume one, which used the runner's real C:/D: pair rather than skipping. Anchoring the selector to their exact names turns the job green without weakening any assertion. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
3c7221492b |
fix(agent): deliver the OpenCode prompt on stdin, not argv (MUL-5841) (#6552)
* fix(agent): deliver the OpenCode prompt on stdin, not argv (#6538) The daemon appended the whole task prompt as an argv element, so on Windows every OpenCode task whose command line cleared CreateProcess's 32,767 character lpCommandLine limit failed to start at all. Go surfaces the resulting ERROR_FILENAME_EXCED_RANGE (206) as "The filename or extension is too long", which reads as an exe-path problem and is not one. A workspace with a realistic set of models and skills clears that ceiling on the prompt alone (39,629 bytes in the report), leaving no usable in-app workaround. `opencode run` merges its variadic [message..] positional with whatever is piped in, so passing no positional makes the piped text the entire run message. That path is present in the reported v1.18.14 and back to v1.4.10. Write the prompt from its own goroutine, as the Pi and Cursor backends already do: a prompt larger than the pipe buffer blocks mid-write until OpenCode drains it, and OpenCode cannot drain while nobody consumes its stdout. OpenCode reads stdin to EOF, so stdin is closed exactly once as the end-of-prompt signal — including on the cancellation path, where closing it releases a writer still blocked on a full pipe. The existing process-group SIGTERM -> SIGKILL ordering from #4533 is unchanged. Keeping the prompt off argv also stops it being echoed into the "agent command" log line; prompt_bytes replaces it there. Scoped to OpenCode. DevEco, OpenClaw (#6032), Qwen, Copilot and Antigravity inline their prompts the same way, but each CLI's input contract needs verifying on its own before it moves. Co-authored-by: multica-agent <[email protected]> * fix(agent): do not fail a completed run on a benign prompt-write EPIPE The backend CI run for this PR failed in TestPiExecuteRetainsOnlyLastTurnOutput with "pi prompt write failed: write |1: broken pipe". The cause is a latent race shared by pi and the OpenCode code added in the previous commit, which copied pi's error handling. A child that exits without draining stdin closes the read end while the parent is still writing the prompt, so the parent's write returns EPIPE. Both backends converted that into a failed result even when the agent had already emitted a complete, successful stream — making the outcome depend on whether the write landed before or after the child exited, i.e. on machine load. OpenCode reads stdin to EOF before it does any work, so a run that produced a complete stream necessarily received the whole prompt; an EPIPE recorded after that only means the pipe closed on the way out. Report the write error only for a run that did NOT complete, where it can actually explain the failure, and append rather than overwrite so the stream's own diagnosis survives. cursor.go already guards this correctly by ignoring writeErr once a result was seen. Test fakes now drain stdin, matching what the real CLIs do. A fake that exits without reading is what manufactures the spurious EPIPE, and that is what broke CI — pi's piEventStreamScript is the fixture that failed. Adds a deterministic regression test: a child that emits a full successful stream and never reads a 1.2 MB prompt. Against the previous commit it reproduces the CI signature exactly (status "failed", "broken pipe"); with this change it passes. pi.go's production handling has the same defect and still needs the equivalent fix, but that belongs to whoever owns #6485 — this change keeps to OpenCode plus the test fixture that was breaking the build. Co-authored-by: multica-agent <[email protected]> * fix(agent): gate prompt-write suppression on a positive terminal signal Review caught a false-success path opened by the previous commit. It suppressed a failed prompt write whenever status was "completed", on the reasoning that a completed run must have received the whole prompt. That reasoning does not hold: processEvents starts finalStatus at "completed" and only fails closed on structural evidence — an open step, or a step awaiting a continuation. A child that emits NOTHING and exits 0 sets neither, so the default survives. The full path: the parent writes a large prompt, the child exits 0 without reading a byte, the write returns EPIPE, stdout hits EOF with no events, and the run is reported as completed with empty output and no error. The prompt never reached the agent and nothing said so. Reproduced deterministically before the fix: status="completed" output="" error="". Absence of a failure signal is not proof of success, so record the positive one instead. eventResult.sawTerminalSignal is set only when a step_finish closed the last step with no continuation pending, and the write error is suppressed only on that evidence. It is deliberately not the negation of noTerminalSignal: an empty stream sets neither, because there is nothing to fail closed on and nothing proving completion either. When the signal is absent the write error now also flips the default "completed" to "failed". Adds the empty-stream regression alongside the existing complete-stream one, so both directions of the branch are pinned. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
25d26c2bf7 |
fix(agent): send Pi prompts over stdin (MUL-5779) (#6485)
Co-authored-by: Eve <[email protected]> Co-authored-by: multica-agent <[email protected]> |
||
|
|
cac453fe01 |
MUL-5505: fix(selfhost): one source of truth for the backend host port (#6168)
* fix(selfhost): read the published host port back from Compose (MUL-5505)
`make selfhost` polled the wrong port when the backend host port was passed
through the environment. Make and Compose disagree on variable precedence: an
assignment in an included makefile (the Makefile `include`s .env) overrides a
real environment variable, while for Compose the environment overrides .env.
Because .env.example ships BACKEND_PORT uncommented, .env always pinned the
recipe's view of the port, so `BACKEND_PORT=9000 make selfhost` published the
stack on 9000 while the health check hammered 8080 for 60s and then reported
"Services are still starting" on a perfectly healthy install. The success
message printed the same wrong backend URL. FRONTEND_PORT had the same
inversion, affecting the printed frontend URL.
Stop re-deriving the host port in the recipe and ask Compose, which is the only
authority on what it published. The wait-and-report block (three port
references per target, duplicated across selfhost and selfhost-build) moves into
scripts/selfhost-wait.sh, which resolves both host ports via `docker compose
port` and falls back to the compose default chain when the stack is not up.
Also fix the input contract: `PORT` was the only port variable documented in
SELF_HOSTING_ADVANCED.md, yet compose read only BACKEND_PORT, so setting the
documented variable was silently ignored. The backend host port mapping now
falls back to `${PORT:-8080}`, matching Makefile and scripts/local-env.sh, and
the docs describe both variables as host ports.
Container-internal ports (8080/3000), the global PORT derivation used by local
dev, and the 127.0.0.1 bindings are unchanged; the default self-host experience
is byte-identical.
Fixes #6145
Co-authored-by: multica-agent <[email protected]>
* fix(selfhost): make PORT the real backend port and test the whole chain
Addresses review feedback on #6168.
The first round's root-cause claim was wrong. It said `BACKEND_PORT=9100 make
selfhost` published on 9100 while the health check probed 8080. It does not:
when make drives Compose, Compose inherits make's exported values, so both
sides agree on 8080 and the override is silently dropped instead. Re-measured
through the real recipe, the genuine disagreements were:
- `.env` setting PORT with no BACKEND_PORT: probe followed PORT, Compose
published ${BACKEND_PORT:-8080} — 9100 vs 8080.
- `make selfhost PORT=8080` over a .env with BACKEND_PORT=9100: probe 8080,
Compose published 9100.
Both are the "wrong port variable" defect the GitHub issue names, and reading
the port back from Compose fixes both. The comments and PR text that blamed
make/Compose precedence are corrected.
The PORT fallback added to docker-compose.selfhost.yml was also unreachable:
.env.example shipped BACKEND_PORT=8080 uncommented, so the fallback never
engaged and `SELF_HOSTING_AI.md`'s "edit PORT and FRONTEND_PORT" instruction did
nothing. Pick one contract and apply it everywhere: PORT is the backend port to
edit, BACKEND_PORT/API_PORT/SERVER_PORT are optional aliases that override it.
That is already what Makefile, scripts/local-env.sh, scripts/install.sh and
install.ps1 do; compose and the web dev fallback were the outliers.
- .env.example ships BACKEND_PORT commented out, like its sibling aliases, so
editing PORT works out of the box.
- apps/web/config/runtime-urls.ts walks the same alias chain instead of
reading BACKEND_PORT alone, so `pnpm dev` follows an edited PORT.
- SELF_HOSTING_AI.md and SELF_HOSTING_ADVANCED.md state the contract.
Configuration that cannot take effect is now reported instead of ignored.
scripts/selfhost-preflight.sh warns when an alias in the env file overrides an
edited PORT, and when a port from the shell environment is overridden by the env
file. The Makefile captures the pristine origins before its include, because
afterwards there is no way to tell that `PORT=9000 make selfhost` was asked for.
Tests now cross environment -> make -> compose -> report for real. The docker
stub is no longer told what to publish: on `up` it asks the real
`docker compose config` to interpolate the compose file with the environment the
recipe actually handed it, records that, and answers `port` from the recording.
Verified failing on the old code — with the old recipe and compose file the
suite reports published 8080 against probe 9100, the exact defect.
Co-authored-by: multica-agent <[email protected]>
* ci: gate the self-host test on scripts/selfhost-preflight.sh too
The new preflight script was missing from the frontend path filter, so a
change to it alone would skip the test that covers it.
Co-authored-by: multica-agent <[email protected]>
* fix(selfhost): honour the full backend port alias chain in Compose
Addresses the second review round on #6168.
The contract this PR documents is
BACKEND_PORT -> API_PORT -> SERVER_PORT -> PORT -> 8080
and Makefile, scripts/local-env.sh, scripts/install.sh, scripts/install.ps1 and
apps/web/config/runtime-urls.ts all implement it. docker-compose.selfhost.yml
implemented only BACKEND_PORT -> PORT -> 8080, so `API_PORT=9100` or
`SERVER_PORT=9200` in .env still published 8080.
That is not only a direct-Compose concern. scripts/install.sh:407 derives its
health-check port from the full chain and then runs this compose file, so an
alias it honours but Compose ignores puts the probe on 9100 while the stack
listens on 8080 — the original #6145 defect, reached through the installer.
- docker-compose.selfhost.yml publishes the full nested chain, verified to
keep the same precedence and the same 8080 default.
- scripts/selfhost-wait.sh's fallback matches, so the degraded path agrees
with what Compose would have published.
- The Makefile captures the pristine origins of API_PORT and SERVER_PORT too,
and the preflight reports them, so no alias can be silently overridden by
the env file while the others are reported.
Tests pin the chain on the boundaries a recipe-level test cannot see, because
Makefile normalises all four aliases into PORT before any recipe runs: a matrix
over the direct Compose path asserts each alias moves the published port with
the right precedence, and every case additionally asserts that
scripts/install.sh's resolver returns the same value Compose published. That
invariant is the one that actually protects the installer.
Verified failing without the fix: with the two-level chain restored the suite
reports `expected 9200 / compose published 8080 / installer resolved 9200`, and
with the alias warnings narrowed it reports the missing API_PORT notice.
Co-authored-by: multica-agent <[email protected]>
* fix(selfhost): resolve one winner before reporting ignored port config
Addresses the third review round on #6168.
The preflight walked each alias independently, so it made a separate "wins"
claim per alias. With
PORT=9000
BACKEND_PORT=8000
API_PORT=7000
SERVER_PORT=6000
in .env it announced that BACKEND_PORT, API_PORT and SERVER_PORT each won, when
only BACKEND_PORT=8000 can. Two of the three notices were simply false.
It also compared only same-named shell and file variables, so shadowing across
aliases stayed silent: .env with PORT=8000 and BACKEND_PORT=8000 plus a shell
API_PORT=7000 produced no output at all, even though API_PORT was discarded.
Resolve the winner along the documented chain first — within a variable the env
file beats the environment, then BACKEND_PORT beats API_PORT beats SERVER_PORT
beats PORT — and report only the inputs that winner actually shadows. Output now
names one winning input and lists the rest, whichever variable or source they
came from. An input carrying the winning value is not reported: it is redundant
but the user still gets the port they asked for, so there is nothing to fix.
Tests cover both cases the old logic got wrong: every alias set at once must
yield exactly one winner claim and list the others as unused, and an env-file
alias beating a lower-priority shell alias must be reported. Also asserted that
a redundant same-value alias and a default configuration both stay silent.
Measured against the previous implementation, phrasing aside: with all four
values set it made 3 winner claims where there is now 1, and on the cross-alias
case it printed nothing where API_PORT is now reported.
Co-authored-by: multica-agent <[email protected]>
* fix(selfhost): track env-file existence so empty port assignments resolve right
Addresses the fourth review round on #6168.
The Makefile passed only values to the preflight, so the script inferred "the
env file does not set this" from an empty string. An explicit empty assignment is
a different input from an absent one: make lets `BACKEND_PORT=` in the env file
override `BACKEND_PORT=9000` from the environment, and the variable then drops out
of the alias chain instead of setting a port.
With .env holding PORT=8080 and BACKEND_PORT=, invoked as
`BACKEND_PORT=9000 make selfhost`, the stack published 8080 and the health check
probed 8080 — correct — while the preflight announced
the backend host port resolves to 9000 from BACKEND_PORT (environment)
Set but unused: PORT=8080 (.env)
naming the ignored value as the winner and the winning value as unused. A report
that contradicts the startup is worse than no report. FRONTEND_PORT= had the
matching hole: it shadowed the environment, Compose fell back to 3000, and the
preflight said nothing.
The Makefile now captures ENV_FILE_<VAR>_IS_SET from $(origin) alongside the
value, for all five port variables via one $(foreach) instead of hand-written
lines. The preflight treats a variable the file defines as taking the file's
value even when empty, so it shadows the environment, and an empty value never
wins — resolution continues down the chain and falls back to 8080. Inputs the
winner shadows are still listed, including ones shadowed by an empty assignment.
The frontend check mirrors this and now names the port that actually resolved.
Recipe-level tests cover an empty alias over a shell value, every link of the
chain emptied at once, and the frontend equivalent — each asserting the reported
winner, the Compose published port and the probed port all agree. Against the
previous implementation the first case fails with `reported: 9000 / actual: 8080`.
Co-authored-by: multica-agent <[email protected]>
* fix(selfhost): installers read the published port; drop the second parser
Addresses all four blockers from the consolidated review on #6168.
1. Both installers probed and printed the wrong port. scripts/install.sh:402 and
scripts/install.ps1 start Compose inheriting the current environment, and
Compose lets that environment outrank .env — but the installers then derived
the probe port and the summary URL from .env alone. Measured on this branch
before the fix, every port variable diverged:
ambient PORT=9100 -> compose 9100, installer 8080
ambient BACKEND_PORT=9200 -> compose 9200, installer 8080
ambient API_PORT=9300 -> compose 9300, installer 8080
ambient SERVER_PORT=9400 -> compose 9400, installer 8080
ambient FRONTEND_PORT=3100 -> compose 3100, installer 3000
Both now read the ports back once with `docker compose port` after `up -d`
and reuse that single result for the health check and the summary. If the
query fails they fail loudly instead of claiming success. The .env-derived
helpers are deleted rather than left beside the new path.
2. The Make preflight is removed entirely, as the review recommended. It
modelled `origin=environment` and `origin=file` but not `command line`, so
`make selfhost BACKEND_PORT=9000` over a .env with API_PORT=7000 announced
7000 while Compose published 9000. That was its fourth wrong report in four
rounds, because it was a second parser of precedence rules — the very thing
this PR removes elsewhere. `docker compose port` is the runtime truth, so the
script, the Makefile captures and the macro are gone.
3. Tests now cover the installer paths that were unguarded. scripts/install.test.sh
gains a `--with-server` matrix (defaults, .env PORT, all four backend aliases,
FRONTEND_PORT, ambient overriding .env for all five, and explicit-empty
fallback) plus a case proving an unresolvable port fails loudly. A new
scripts/install.ps1.test.ps1 drives the same matrix through Start-LocalInstall.
Every case asserts Compose's published port == the probed URL == the printed
URL. Ambient variables are explicitly cleared per case so a runner's own PORT
cannot leak. The Makefile matrix gains the command-line origin cases. CI runs
the PowerShell suite on windows-latest in the always-on installer job, and the
frontend filter now includes both installers.
4. Docs state the real contract: the alias order is shared, but which *source*
wins is per entry point — Compose prefers the environment, make prefers the
included env file, and a make command-line assignment outranks both. The
removed preflight's promise is gone from SELF_HOSTING_AI.md, and the compose
header no longer claims the installer derives its own health-check port.
Verified failing without the fixes: the Bash matrix reports
`[ambient PORT beats .env] compose published 9500 / probed 9100 / printed 9100`
against the old installer, and the PowerShell matrix reports
`printed backend=8080` against an injected summary divergence.
Co-authored-by: multica-agent <[email protected]>
* fix(selfhost): keep web dev proxy off frontend port
Co-authored-by: multica-agent <[email protected]>
---------
Co-authored-by: Eve <[email protected]>
Co-authored-by: multica-agent <[email protected]>
|
||
|
|
a0ef1a43f3 |
fix(daemon): diagnose silent OpenClaw npm shim failures on Windows (MUL-5422) (#6084)
* fix(daemon): diagnose silent OpenClaw npm shim failures on Windows (MUL-5422)
#6061 reported every OpenClaw task failing in execenv prep on Windows with
a bare `exit status 1` and no stderr, leaving the user nothing to act on.
An npm-installed `openclaw.cmd` is a batch shim that re-execs OpenClaw's
`openclaw.mjs` entrypoint through `node`, resolved from PATH. The daemon pins
`openclaw` to an absolute path, so the shim always looks correct — but that
interpreter lookup is a second, invisible resolution step that can fail on its
own. The reporter had to run their own subprocess experiments to find it.
Enrich the error instead of guessing at a fix: when a `.cmd`/`.bat` shim exits
non-zero with no stderr, report whether the interpreter resolves. Both
directions are useful — missing names the likely cause with a next step,
present clears PATH of blame and points at the remaining hypotheses (PATH
drift between the runtime `--version` gate and task prep, or a broken install).
Deliberately NOT included: rebuilding or freezing a Windows PATH. The
version-probe gate (probeBuiltinRuntime skips a provider whose `--version`
fails) and the prep helper both inherit the same daemon environment, so a
daemon that could not resolve `node` would never have registered OpenClaw at
all. That contradiction is unresolved, and a boot-time PATH snapshot would also
fight the MUL-4486 self-heal design, which re-resolves per attempt on purpose.
This change collects the evidence needed to settle it.
- Error text only; no control flow change, and real stderr still wins.
- PATH summarised as an entry count, never dumped, so daemon logs and pasted
bug reports carry no environment detail.
- Tests: shim detection (case, spaces, Unicode), both diagnostic directions,
out-of-scope no-ops (timeout, missing binary, native exe), and end-to-end
through execOpenclawCLI. A windows-tagged file reproduces a real npm shim
with and without node on PATH, and pins TEMP/TMP as not load-bearing — the
originally reported root cause, since retracted upstream.
- New scoped step in the existing ci.yml windows-execenv job.
Co-authored-by: multica-agent <[email protected]>
* fix(daemon): address review on OpenClaw shim diagnostics (MUL-5422)
Four must-fix items from PR review, each verified against the real behaviour
rather than assumed.
1. Timeout was misdiagnosed as a missing interpreter. openclawCLITimeout kills
the child via CommandContext, and a killed process surfaces as
*exec.ExitError ("signal: killed") — the same type a genuine exit 1
produces. The errors.As gate accepted it and appended "install Node.js",
sending users to fix something that was never broken. execOpenclawCLI now
attributes ctx.Err() before consulting the diagnostic. Confirmed locally:
`signal: killed`, errors.As(*exec.ExitError)=true, ctx.Err() set. The old
test passed context.DeadlineExceeded directly and so never saw the real
shape; replaced with a genuine CommandContext timeout regression on both
Linux and Windows.
2. The interpreter lookup did not match npm's. npm's cmd-shim template emits
`IF EXIST "%dp0%\node.exe" (...) ELSE ( SET "_prog=node" )`, so a co-located
node.exe wins over PATH entirely. Checking only LookPath reported "node is
not resolvable" for installs that actually run fine — confidently wrong,
which is worse than silence. Now resolves co-located `node.exe`/`node`
first, then PATH, and reports which. Wording is also conditional now
("if <name> is an npm-generated shim"): a batch extension does not prove npm
authorship, since MULTICA_OPENCLAW_PATH can point at any batch file.
3. The message leaked local paths off-box. On prep failure this text is not
log-local — it travels reportTerminalTask → Client.FailTask and is persisted
server-side as the task error, so an absolute Windows shim path uploads the
account name and install layout. Now reports only the shim's base name,
whether the interpreter resolved and from where, and a PATH entry count.
Never an absolute path, never PATH contents.
4. Windows CI was green without exercising the new code. The job log showed
`cmd.exe stderr DID reach Go's pipe` with `'node' is not recognized`, so the
missing-node case takes the existing stderr branch and the diagnostic never
ran — masked by an "either branch passes" assertion. That disjunction is
gone: the missing-node test now asserts the observed stderr behaviour
(disproving #6061's premise), and a new test drives a genuinely silent shim
to prove the diagnostic branch itself works on Windows. Also added Windows
coverage for the co-located interpreter and the timeout case.
The windows-tagged shim is now npm's real generated template rather than a
hand-simplified `node ...` one-liner, so the co-located branch is reproduced
faithfully instead of hidden.
Co-authored-by: multica-agent <[email protected]>
* test(daemon): make the OpenClaw timeout regression PATH-independent
The new timeout test stripped PATH (so a stray interpreter lookup would report
"missing") while its hanging shim invoked `sleep` through a PATH lookup. macOS
`sh` quietly falls back to a default PATH so this passed locally; dash on Linux
does not, so CI failed with `exit status 127 (stderr: sleep: not found)` — the
shim died instantly instead of hanging, and the assertion never saw a timeout.
Resolve `sleep` before PATH is stripped and embed it by absolute path, so the
shim needs no PATH of its own. Verified the failure mode and the fix directly:
`env -i /bin/sh -c 'PATH=/nonexistent; sleep 0.05'` reproduces
"sleep: command not found", while the absolute path runs fine with the same
empty PATH.
Windows is skipped here and covered by TestWindowsOpenclawShimTimeoutIsNotMisdiagnosed,
which has a real cmd.exe host and a System32 PATH that can resolve its own helper.
Co-authored-by: multica-agent <[email protected]>
* fix(daemon): bound execOpenclawCLI so its 5s timeout is actually enforceable
The new timeout regression exposed a real bug in the code it was testing, not
just a flaky test: openclawCLITimeout could not bound the call at all.
CommandContext kills only the direct child, and cmd.Output() blocks in Wait()
until the stdout pipe closes. Any grandchild that inherited stdout keeps the
call parked for its own lifetime. Verified on linux/dash: a shim whose child
slept 5s ran the FULL 5.01s against a 150ms deadline. With a WaitDelay backstop
the same case returns in ~2.17s.
This is not a hypothetical shape — it is precisely an npm shim on Windows
(cmd.exe → node), so a wedged node could stall task prep far past the 5s cap
that comment claims. detectCLIVersion already carries this exact backstop for
the `--version` probe for the same reason; execOpenclawCLI now matches it.
Also corrected the test comment: an earlier revision claimed a trailing
`exit 0` was needed to force the grandchild. Docker showed otherwise — dash
hangs either way and macOS reproduces neither, which is why CI caught this and
local runs did not. The comment now records the measured behaviour.
Verified in a linux/dash container (the CI platform, not just macOS): the full
execenv package passes with -race, and the timeout case takes 2.17s.
Co-authored-by: multica-agent <[email protected]>
* fix(daemon): drop the WaitDelay change and wrap the context error (MUL-5422)
Round-2 review: take option 1 — keep this PR to diagnostics and split the
timeout/process-tree work out.
The reviewer is right that WaitDelay traded a hang for a process leak, and I
had the mechanism wrong. Measured on linux/dash by recording the grandchild PID
and reading /proc/<pid>/stat at the moment Output returns:
no WaitDelay: elapsed 6.01s (6s sleep, 150ms deadline), grandchild state Z
with WaitDelay: elapsed 2.17s (60s sleep, 150ms deadline), grandchild state S
So without WaitDelay the call is hostage to the descendant's lifetime but no
live process is left behind — it returned precisely because the descendant had
exited. With WaitDelay the call is bounded but a live descendant survives. My
earlier claim that the orphan pre-existed was an artifact of a sleep duration
that happened to equal the return time.
Go's WaitDelay contract covers killing the direct child and closing our pipe
ends; it does not reap orphans. Closing this properly needs process-tree
ownership (Unix process group, Windows Job Object) so the deadline can terminate
the whole tree — and on Unix nothing else will, since
preparationProcessController.finish() is a no-op there (isolation_unix.go).
That is its own change with its own risk surface, so it is tracked separately
and openclawCLITimeout now documents the gap with the measurements rather than
shipping half a fix.
Also fixes the round-2 nit: the context branch %w-wrapped the process error
while printing ctxErr with %v, so errors.Is(err, context.DeadlineExceeded) was
false despite the text containing it. The context error is now the wrapped
cause and the process error is attached for diagnosis:
openclaw config file: context deadline exceeded (process: signal: killed)
Tests: the timeout cases no longer depend on WaitDelay and no longer leave a
live process — short sleeps keep them about attribution, which is what they are
for. Added an explicit errors.Is assertion for both DeadlineExceeded and
Canceled. Verified in a linux/dash container (the CI platform): full execenv
package passes with -race and `ps` shows no leftover sleep processes.
Co-authored-by: multica-agent <[email protected]>
* docs(daemon): correct two stale comments on the OpenClaw CLI timeout (MUL-5422)
Both nits from the third review. Comment-only; no code change.
1. openclawCLITimeout's doc contradicted itself — it opened with "caps ...
without letting a hung CLI stall task dispatch indefinitely" and then
explained that the deadline cannot actually bound the call. Reworded to say
what it is (a 5s context deadline) and to point at the gap rather than assert
a guarantee it does not provide. Also names MUL-5467 instead of the vague
"tracked separately".
2. The two timeout tests claimed a long wait would "leave a live process
behind". That described the reverted WaitDelay behaviour, not the current
code. Without WaitDelay, cmd.Output() returns only once the descendant has
closed stdout — its exit is what produces the EOF — so a long wait makes the
test slow, it does not leak. Re-verified on linux/dash after the fix: the
case takes 1.01s for a 1s sleep and `ps` shows no leftover process, and the
earlier PID probe recorded the grandchild in state Z at the return point.
Rebased onto
|