Conversation
An idle machine started and killed a real ACP agent process ~1700 times a day for `machine/acp-capabilities-refresh`, recomputing capability entries that were already stored under an unchanged `capabilitySourceVersion`. The machine now answers from the persisted entry when it came from a real probe, its source version is exactly what the current launch inputs would produce, and it is younger than 24h. Resolving that expected version reads the installed managed-runtime version without spawning or downloading, and returns "unknown" — always a miss — when the runtime is not installed. Startup capability discovery now records completion per config in a runtime-owned set, so a pass aborted by presence leaving `synced` and then re-armed no longer re-probes every agent config on each reconnect. A new `force` flag keeps every explicit probe unchanged: Settings refresh, post-authentication verification, onboarding's provider test, and provider setup verification. Model: claude-opus-5[1m]
github-actions
Bot
added
scope: cli
scope: shared
scope: components
scope: streams-rpc
labels
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66e56ed8cc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| params: z | ||
| .object({ | ||
| configId: AgentConfigIdSchema, | ||
| force: z.boolean().optional(), |
There was a problem hiding this comment.
force before sending it to older daemons
When a new renderer performs a Settings or onboarding refresh against a remote machine running the previous CLI, it sends force: true, but that CLI's LoroMachineAcpCapabilitiesRefreshRpcRequestSchema.params is strict and does not recognize this field, so the request is rejected instead of falling back to the old always-probe behavior. Gate this field with a versioned MachineMeta.protocolCapabilities capability and omit it when unsupported; remote daemon workflows are required to negotiate additions rather than infer support.
AGENTS.md reference: packages/shared/AGENTS.md:L71-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bf48415. Verified against the pre-change files at 4de83a57: both LoroMachineAcpCapabilitiesRefreshRpcRequestSchema.params and the shared MachineAcpCapabilitiesRefreshRequestSchema were .strict() and neither declared force.
Two corrections to the failure mode, both worse than "rejected":
- Machine RPC fails silently.
handleRawRequestdoesLoroStreamsRpcRequestSchema.safeParse, and on failure it logs a warning and returns without appending any response. The caller gets no error — only theACP_CAPABILITIES_REFRESH_CLIENT_BACKSTOP_MStimeout. - The local-control path is affected too.
LocalSessionControlRequestSchemais a discriminated union over the same strictMachineAcpCapabilitiesRefreshRequestSchema, so an older daemon answers HTTP 400invalid_request. That path matters as much as the remote one, since the desktop app and the CLI daemon upgrade independently and either can be newer.
Fix: new acpCapabilityRefreshCache capability at version 1 in packages/shared/src/machine-protocol-capabilities.ts, with its version constant and its check in the same binding per packages/shared/AGENTS.md. One key covers both facts deliberately — a daemon that never caches is exactly a daemon that rejects force, so splitting them could only create an unrepresentable state.
Negotiation lives at the two places where a request crosses a version boundary: create-workspace-runtime.ts's requestMachineAcpCapabilitiesRefresh (the single choke point both planes flow through, so renderer callers keep passing plain force: true) and apps/cli/src/commands/agent-config.ts. The CLI's in-process callers (post-auth verification, provider setup) are deliberately not negotiated — the message never leaves the build that created it.
One detail worth flagging, because it changed the implementation: I verified in zod 4.3.6 that .strict() rejects an unrecognized key even when its value is undefined. So negotiatedAcpCapabilitiesRefreshForce returns a spreadable {} rather than { force: undefined } — a "falsy force" spelling would have shipped this same bug in a form that reads as fixed.
Degradation is a no-op for every caller: such a daemon always probes, which is what a forced caller wanted, and an unforced caller gets the pre-change behavior.
Tests validate the payload a client actually emits against a previous-generation schema derived from the current one with .omit({ force: true }), so the reconstruction cannot drift from what shipped — packages/shared/tests/machine-protocol-capabilities.test.ts (local control, via the real safeParseLocalSessionControlRequest) and packages/loro-streams-rpc/tests/loro-streams-rpc.test.ts (RPC, via the real client and a fake stream). Both assert the key is absent from Object.keys, not merely falsy. Spec and Note updated; the Spec's earlier claim that an older machine "still probes" when sent force was wrong and is corrected.
| throw createAcpRefreshAbortError(); | ||
| } | ||
|
|
||
| if (message.force !== true) { |
There was a problem hiding this comment.
The existing lody agent config refresh caller in apps/cli/src/commands/agent-config.ts:577-583 still sends no force, so this new default cache path returns any matching entry up to 24 hours old without starting the agent. That makes the explicit refresh command unable to detect the external changes it is intended to refresh, such as a newly installed Bub binary or changed account entitlements; this caller should set force: true like the Settings refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bf48415. apps/cli/src/commands/agent-config.ts was the only request construction site I missed — a command that exists precisely to pick up changes Lody cannot see in the launch inputs would instead have accepted a stored entry up to 24 hours old.
It now sends force, routed through the same negotiation as the renderer, because the CLI binary can be newer than the daemon it dispatches to over local control.
I re-swept every construction site (grep -rn "'machine/acp-capabilities-refresh'" filtered to request literals); this is the complete set, now recorded as a table in the Note:
| Caller | Forces | Why |
|---|---|---|
create-workspace-runtime.ts startup pass |
no | wants the cache; this is the cost being removed |
use-agent-role-schema-reconciliation.ts |
no | reconciles against the current entry, whatever produced it |
machine-agent-settings.tsx Settings refresh |
yes | a person changed something outside the launch inputs |
providers-screen.tsx onboarding provider test |
yes | exists to prove the agent starts |
commands/agent-config.ts refresh-capabilities |
yes | this fix |
session-execution-service.ts post-authentication |
yes | new credentials change entitlements |
provider-setup-manager.ts verification |
yes | proves the runtime it just installed starts |
Stated limit: the command's own call site is not under test. Driving that Commander action would mean replacing getAuthContextOrThrow, withWorkspaceManager, listMachineMetasForWorkspace and dispatchLocalControl, and the assertions would then be about those mocks rather than behavior — which AGENTS.md asks us not to add. What it spreads is covered where it is real, in the negotiation tests; that force: true starts the agent despite a current entry is covered in apps/cli/tests/session-execution-service.test.ts. The gap is noted explicitly in the Note's verification section.
…mmand Both transports parse `machine/acp-capabilities-refresh` with a strict schema, so a daemon built before `force` exists does not ignore the field: Machine RPC fails `LoroStreamsRpcRequestSchema` and drops the request without a reply, leaving the caller with only a backstop timeout, and local control answers HTTP 400. The local path matters as much as the remote one, because the desktop app and the CLI daemon upgrade separately. `force` now travels only to a machine advertising `acpCapabilityRefreshCache`, negotiated at the renderer's single choke point for both planes and again in the CLI command, whose binary can outrun its daemon. The helper returns a spreadable object so an unsupported target gets no key at all: zod `.strict()` rejects an unrecognized key even when its value is `undefined`. Such a daemon has no cache to opt out of, so omission still gives a forced caller its probe. `lody agent config refresh-capabilities` now forces. It existed to pick up external changes and would otherwise have returned an entry up to 24 hours old without starting the agent. Model: claude-opus-5[1m]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf484151b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (nowMs - entry.fetchedAt > ttlMs) { | ||
| return { hit: false, reason: 'expired' }; |
There was a problem hiding this comment.
When an entry first crosses the 24-hour TTL, the real probe can return capabilities identical to the stored value, but MachineDocument.updateAcpCapabilities deliberately compares entries without fetchedAt and returns the old entry unchanged (apps/cli/src/lib/loro/doc.ts:3029-3035). The entry therefore remains expired, so every later unforced request—including agent-role reconciliation after each presence reconnect—starts another agent process indefinitely, recreating the recurring churn this change is intended to stop. A successful expiry-driven probe needs to advance the persisted freshness timestamp even when the capability payload is unchanged.
Useful? React with 👍 / 👎.
| if (input.cliType !== 'builtin' || !isManagedBuiltinAgentType(input.agentType)) { | ||
| return getAcpCapabilitySourceVersion(input); |
There was a problem hiding this comment.
For custom and registry configs, this newly trusted expected version is independent of input.env, even though fetchAcpCapabilities merges that environment into the spawned agent. If a user edits a token, endpoint, or another environment value that changes authentication or the advertised model catalog, the automatic refresh/reconciliation request still matches the old sourceVersion and returns stale capabilities for up to 24 hours instead of probing the updated config. Use a non-secret config revision/invalidation signal, or otherwise make these environment-changing edits miss the cache without persisting credential derivatives.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Problem, measured
machine/acp-capabilities-refreshstarted and killed a real ACP agent process on every request. On my idle machine, from~/.lody/logs/2026-09-16.log.1(02:11–10:29):capabilitySourceVersion(e.g.builtin-claude-acp:0.70.0+agent-sdk:0.3.258+claude-code:2.1.258).Which trigger it was
Decoding the machine's own Flock document (
flock_docs→:mf:<machineId>, read with@loro-dev/flock-wasm) settled it without instrumenting the app: the machine has exactly six liveagentConfigrows, and sorting their ids reproduces the observed request order exactly — that is the full config list walked byrunStartupAcpCapabilitiesRefresh.The agent-role reconciliation hook is excluded: the workspace's seven owned roles reference four distinct configs (claude, codex, deepseek, grok), so it can neither produce the observed
pi-acp/opencode/kimirequests nor omitcodex. It does explain the sporadic off-cadence bursts that do containcodex— those are now answered from the cache.Two structural facts turn one startup pass into a standing cycle:
scheduleAfterStartupNavigationCooldowncomputesmax(0, lastNavigationAt + cooldown - now), so on an app nobody is navigating the 30 s cooldown is 0 ms and a presence reconnect starts a pass immediately.startupAcpCapabilitiesRefreshCompletedonly latches when a pass reaches its end un-aborted, while the non-syncedbranch aborts the in-flight pass and.finallyre-arms it. Nothing recorded which configs had already answered.Named limit: the machine's logs cannot show why that boolean never latched over eight hours — both remaining explanations need presence to leave
syncedduring a ~20 s pass, and distinguishing them requires renderer instrumentation against a hosted presence room. The fix is therefore chosen to hold either way: with per-config completion recorded, a re-armed pass is a no-op regardless of how often or why it re-arms.Change
sourceVersionmatch at the currentcacheVersion,provenance: 'runtime', and age withinACP_CAPABILITY_REFRESH_CACHE_TTL_MS.decideAcpCapabilityRefreshCacheowns that decision and names each miss reason. A runtime override, custom command, or capability-affecting env change always misses.resolveExpectedAcpCapabilitySourceVersionreadsgetRuntimeStatus()— local, no download, no spawn — and returnsundefined(always a miss) when a managed runtime is not installed, rather than substituting the bundled target version. It still enqueues the managed-runtime update coordinator onupdateAvailable, which previously only happened via launches.runStartupAcpCapabilitiesRefreshtakes a caller-ownedrefreshedConfigKeysset; successes are recorded, failures stay retryable.forcekeeps every explicit probe real: Settings refresh, post-auth verification, onboarding's provider test, provider-setup verification. Absent on the wire means "cache is fine", so an older machine still probes.TTL rationale (24 h) is in the Note: the source version already covers every Lody-controlled input, real session creation rewrites the entry for free via
session/new, and Settings offers a forced refresh — so the TTL only bounds drift in agents nobody launches (an hour would cost ~144 probes/day here; a day costs ~6).STATIC_BUILTIN_ACP_CAPABILITIESwas evaluated and deliberately not used: it carries noavailableCommands/sessionFork/acknowledgedSteer/goalActions, returnsundefinedunder any runtime override, and its model lists are Lody constants (kimi/deepseekship empty ones). Reasons recorded in the Note.Docs
specs/acp-capability-refresh-cache.md(+.zh.md),Status: draft..agents/notes/implemented/bug-fix/2026-09-16-acp-capability-refresh-cache.md(+.zh.md).packages/components/src/providers/AGENTS.md.Verification
pnpm checkandpnpm run docs checkpass;pnpm formatapplied. Tests assert observable behavior — that no probe is started — rather than mock call tallies: shared cache-decision predicate,forceparity on both transports (local-control TS/CJS and machine RPC), machine-side hit/override-change/expiry/force/unreadable-entry, expected version equals what a launch stamps, and an aborted startup pass not re-probing what already answered.Not verified: the end-to-end effect on a running desktop build — reproducing the 300 s presence lease needs the hosted presence room, and the local composition has none. The expected steady state of ~6 probes/day instead of ~1700 is a projection from the cache predicate, not a measurement.
🤖 Generated with Claude Code