Repository navigation
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 26 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
1a5cedd to
9f6aff4
Compare
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
9f6aff4 to
4488553
Compare
685c01a to
c95c9c8
Compare
c95c9c8 to
44eefce
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
44eefce to
5035bed
Compare
769eda3 to
ddb6a5e
Compare
ddb6a5e to
e9819db
Compare
ad96e33 to
0c26950
Compare
|
The reviewed commit 0c26950 has one problem that should change before merge. src/cli/commands/proxy.ts:32 used to call the managed ensureDaemon, which replaced an incompatible or socket-only daemon. It now calls ensureDaemonInfo. Three cases now fail: a daemon from the previous release with no Not blocking, and fine to take or leave: the rule "may this caller start or retire the daemon" is now encoded four times (daemonLifecycle, retireIncompatibleContender, the sendRequest timeoutRecovery, and handleRequestTimeout's retireUnresponsive); daemonStopCommand in daemon-client-timeout.ts duplicates stopHint and the inline strings in daemon-client-lifecycle.ts; the attach test in daemon-client-explicit-lifecycle.test.ts only checks the timeoutRecovery value on a spy, and every quiesce outcome comes from a scripted /rpc stub rather than the #3764 handler, so one sendToDaemon attach test with a wedged daemon and one ensureDaemon test that replaces a real spawned daemon would close that gap; the quiesce timeout in daemon-client-ensure.ts can rethrow the attach-worded hint "start one with ensureDaemon()" and needs its own hint; and the PR body says 26 files while the diff has 29. Simplicity question: could daemonLifecycle, retireIncompatibleContender and timeoutRecovery/retireUnresponsive become one DaemonClientSettings field, CI has no result on this head. All 9 checks that did not pass were cancelled as superseded runs on the stacked branch, and the diff touches each of those lanes, so please rerun them. I read the code and did not run the tests or check:affected, so the "passed" in the PR body is unverified by me. No test drives ensureDaemon against the real #3764 daemon_quiesce handler, so the client and daemon pairing rests on contract constants only. There are no conflicts. Before merge, keep proxy.ts on ensureManagedDaemon (or document its new idle-only replacement in the CHANGELOG and remote-proxy.md) and rerun the cancelled CI lanes on this head. |
An attach client connects to the daemon registered in its state directory and never starts, replaces or kills one, including after a request times out; it fails with daemon-not-running or daemon-incompatible. ensureDaemon() starts a missing daemon and replaces an incompatible one only through the daemon's own quiesce answer, refusing with daemon-busy while it holds work and never replacing a daemon that predates the capability. managed stays the default and unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
0c26950 to
d892ab3
Compare
|
Thanks, the proxy regression was real. Fixed in d892ab3.
|
There was a problem hiding this comment.
4 issues found across 12 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/daemon-client/daemon-lifecycle-errors.ts">
<violation number="1" location="src/daemon-client/daemon-lifecycle-errors.ts:62">
P3: “Any agent-device command” is too broad: some commands return before creating a daemon client, and remote-mode commands do not start a local daemon. Specify a local command that uses the managed daemon lifecycle.</violation>
<violation number="2" location="src/daemon-client/daemon-lifecycle-errors.ts:92">
P2: The startup-busy hint drops the `agent-device daemon stop` recovery. If the daemon never finishes starting, retrying will not help, and the user gets no named way out. Keep the stop command in this hint too.</violation>
</file>
<file name="src/cli/commands/__tests__/proxy.test.ts">
<violation number="1" location="src/cli/commands/__tests__/proxy.test.ts:153">
P2: The rewritten success test no longer checks the transport settings the proxy sends. The old `deepEqual` pinned `daemonTransport` and `daemonServerMode` to `'http'`, and that coverage was dropped. Restore it by asserting `settings.transportPreference` and `settings.serverMode` equal `'http'`.</violation>
</file>
<file name="website/docs/docs/client-api.md">
<violation number="1" location="website/docs/docs/client-api.md:112">
P2: This census omits streaming resumable uploads and host human-control holds without a lease, so `ensureDaemon()` may end either during replacement. Name both exclusions here; otherwise the no-work wording promises more protection than the daemon provides.
(Based on your team's feedback about documenting daemon census exclusions.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| hint: census?.startingUp | ||
| ? 'The daemon is still starting up. Retry in a moment.' | ||
| : `Wait for its requests, sessions and leases to end and retry, or stop it deliberately with ${daemonStopCommand(stateDir)}.`, |
There was a problem hiding this comment.
P2: The startup-busy hint drops the agent-device daemon stop recovery. If the daemon never finishes starting, retrying will not help, and the user gets no named way out. Keep the stop command in this hint too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/daemon-client/daemon-lifecycle-errors.ts, line 92:
<comment>The startup-busy hint drops the `agent-device daemon stop` recovery. If the daemon never finishes starting, retrying will not help, and the user gets no named way out. Keep the stop command in this hint too.</comment>
<file context>
@@ -88,6 +89,8 @@ export function daemonBusyError(
daemon: daemonIdentityOf(info),
...census,
- hint: `Wait for its requests, sessions and leases to end and retry, or stop it deliberately with ${stopHint(stateDir)}.`,
+ hint: census?.startingUp
+ ? 'The daemon is still starting up. Retry in a moment.'
+ : `Wait for its requests, sessions and leases to end and retry, or stop it deliberately with ${daemonStopCommand(stateDir)}.`,
</file context>
| hint: census?.startingUp | |
| ? 'The daemon is still starting up. Retry in a moment.' | |
| : `Wait for its requests, sessions and leases to end and retry, or stop it deliberately with ${daemonStopCommand(stateDir)}.`, | |
| hint: census?.startingUp | |
| ? `The daemon is still starting up. Retry in a moment, or stop it deliberately with ${daemonStopCommand(stateDir)}.` | |
| : `Wait for its requests, sessions and leases to end and retry, or stop it deliberately with ${daemonStopCommand(stateDir)}.`, |
| const settings = mocks.ensureDaemon.mock.calls[0]![0]; | ||
| const settings = mocks.ensureManagedDaemon.mock.calls[0]![0]; | ||
| assert.equal(settings.paths.baseDir, resolveDaemonPaths(stateDir).baseDir); | ||
| assert.equal(settings.remoteBaseUrl, undefined); |
There was a problem hiding this comment.
P2: The rewritten success test no longer checks the transport settings the proxy sends. The old deepEqual pinned daemonTransport and daemonServerMode to 'http', and that coverage was dropped. Restore it by asserting settings.transportPreference and settings.serverMode equal 'http'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/cli/commands/__tests__/proxy.test.ts, line 153:
<comment>The rewritten success test no longer checks the transport settings the proxy sends. The old `deepEqual` pinned `daemonTransport` and `daemonServerMode` to `'http'`, and that coverage was dropped. Restore it by asserting `settings.transportPreference` and `settings.serverMode` equal `'http'`.</comment>
<file context>
@@ -155,22 +148,16 @@ test('proxy binds a loopback listener and reports the supplied client token', as
- });
+ const settings = mocks.ensureManagedDaemon.mock.calls[0]![0];
+ assert.equal(settings.paths.baseDir, resolveDaemonPaths(stateDir).baseDir);
+ assert.equal(settings.remoteBaseUrl, undefined);
});
</file context>
| assert.equal(settings.remoteBaseUrl, undefined); | |
| assert.equal(settings.remoteBaseUrl, undefined); | |
| assert.equal(settings.transportPreference, 'http'); | |
| assert.equal(settings.serverMode, 'http'); |
| const client = createAgentDeviceClient({ stateDir, daemonLifecycle: 'attach' }); | ||
| ``` | ||
|
|
||
| `ensureDaemon()` starts a daemon when none runs. It replaces a daemon that does not match the client only if that daemon holds no work: no request in flight, no session, no repair session and no live lease. Otherwise it fails and leaves the daemon alone. Two cases retire a daemon without that proof, because it cannot be asked: a registered daemon that answers nothing on any transport, and one that times out on the quiesce request and then fails repeated liveness probes. It returns `{ stateDir, daemon, started }`; `daemon` holds the pid, version and code identity, and no credentials. `onStart` runs when the call started a daemon: `'incompatible'` when it replaced a daemon that agreed to leave, with that daemon as `previous`, and `'missing'` otherwise. A `'missing'` start that also has a `previous` replaced a registered daemon that answered nothing. |
There was a problem hiding this comment.
P2: This census omits streaming resumable uploads and host human-control holds without a lease, so ensureDaemon() may end either during replacement. Name both exclusions here; otherwise the no-work wording promises more protection than the daemon provides.
(Based on your team's feedback about documenting daemon census exclusions.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At website/docs/docs/client-api.md, line 112:
<comment>This census omits streaming resumable uploads and host human-control holds without a lease, so `ensureDaemon()` may end either during replacement. Name both exclusions here; otherwise the no-work wording promises more protection than the daemon provides.
(Based on your team's feedback about documenting daemon census exclusions.) </comment>
<file context>
@@ -109,7 +109,7 @@ await ensureDaemon({
-ensureDaemon() starts a daemon when none runs. It replaces a daemon that does not match the client only if that daemon holds no work: no request in flight, no session, no repair session and no live lease. Otherwise it fails and leaves the daemon alone. Two cases retire a daemon without that proof, because it cannot be asked: a registered daemon that answers nothing on any transport, and one that times out on the quiesce request and then fails repeated liveness probes. It returns { stateDir, daemon, started }; daemon holds the pid, version and code identity, and no credentials. onStart runs when the call started a daemon: 'missing' when none was usable, 'incompatible' when it replaced one, with that daemon as previous.
+ensureDaemon() starts a daemon when none runs. It replaces a daemon that does not match the client only if that daemon holds no work: no request in flight, no session, no repair session and no live lease. Otherwise it fails and leaves the daemon alone. Two cases retire a daemon without that proof, because it cannot be asked: a registered daemon that answers nothing on any transport, and one that times out on the quiesce request and then fails repeated liveness probes. It returns { stateDir, daemon, started }; daemon holds the pid, version and code identity, and no credentials. onStart runs when the call started a daemon: 'incompatible' when it replaced a daemon that agreed to leave, with that daemon as previous, and 'missing' otherwise. A 'missing' start that also has a previous replaced a registered daemon that answered nothing.
These failures carry a typed error.details.reason:
</file context>
</details>
```suggestion
`ensureDaemon()` starts a daemon when none runs. It replaces a daemon that does not match the client only if that daemon holds no work: no request in flight, no session, no repair session and no live lease. A streaming resumable upload and a host human-control hold that names no lease are not counted by this census and may be ended by replacement. Otherwise it fails and leaves the daemon alone. Two cases retire a daemon without that proof, because it cannot be asked: a registered daemon that answers nothing on any transport, and one that times out on the quiesce request and then fails repeated liveness probes. It returns `{ stateDir, daemon, started }`; `daemon` holds the pid, version and code identity, and no credentials. `onStart` runs when the call started a daemon: `'incompatible'` when it replaced a daemon that agreed to leave, with that daemon as `previous`, and `'missing'` otherwise. A `'missing'` start that also has a `previous` replaced a registered daemon that answered nothing.
| reason: DAEMON_LIFECYCLE_REASONS.notRunning, | ||
| stateDir, | ||
| ...(registered ? { daemon: daemonIdentityOf(registered) } : {}), | ||
| hint: `Start one with any agent-device command, or with ensureDaemon({ stateDir: ${JSON.stringify(stateDir)} }) from the SDK, before attaching.${registered ? ` The registered daemon (pid ${registered.pid}) does not answer; ${daemonStopCommand(stateDir)} clears it.` : ''}`, |
There was a problem hiding this comment.
P3: “Any agent-device command” is too broad: some commands return before creating a daemon client, and remote-mode commands do not start a local daemon. Specify a local command that uses the managed daemon lifecycle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/daemon-client/daemon-lifecycle-errors.ts, line 62:
<comment>“Any agent-device command” is too broad: some commands return before creating a daemon client, and remote-mode commands do not start a local daemon. Specify a local command that uses the managed daemon lifecycle.</comment>
<file context>
@@ -58,7 +59,7 @@ export function daemonNotRunningError(stateDir: string, registered?: DaemonInfo)
stateDir,
...(registered ? { daemon: daemonIdentityOf(registered) } : {}),
- hint: `Start one with ensureDaemon({ stateDir: ${JSON.stringify(stateDir)} }) before attaching.${registered ? ` The registered daemon (pid ${registered.pid}) does not answer; ${stopHint(stateDir)} clears it.` : ''}`,
+ hint: `Start one with any agent-device command, or with ensureDaemon({ stateDir: ${JSON.stringify(stateDir)} }) from the SDK, before attaching.${registered ? ` The registered daemon (pid ${registered.pid}) does not answer; ${daemonStopCommand(stateDir)} clears it.` : ''}`,
});
}
</file context>
| hint: `Start one with any agent-device command, or with ensureDaemon({ stateDir: ${JSON.stringify(stateDir)} }) from the SDK, before attaching.${registered ? ` The registered daemon (pid ${registered.pid}) does not answer; ${daemonStopCommand(stateDir)} clears it.` : ''}`, | |
| hint: `Start one with a local command that uses the managed daemon lifecycle, or with ensureDaemon({ stateDir: ${JSON.stringify(stateDir)} }) from the SDK, before attaching.${registered ? ` The registered daemon (pid ${registered.pid}) does not answer; ${daemonStopCommand(stateDir)} clears it.` : ''}`, |
This branch has not been deployed
1 participant
Summary
Layer 2 of 3 for #3754 (umbrella #3747); based on #3764. The client half: who owns the daemon is now an explicit choice.
createAgentDeviceClient({ daemonLifecycle: 'attach' | 'managed' });managedis the default and behaves as before (additive in 0.21.x).attachconnects to the registered daemon and never starts, replaces or kills one, including after a timeout. A missing daemon fails withdetails.reason: 'daemon-not-running', an incompatible one with'daemon-incompatible'(both identities, hint namesagent-device daemon stop).ensureDaemon({ stateDir, onStart })starts a missing daemon and replaces an incompatible one only through the quiesce request from feat(daemon): quiesce request that retires an idle daemon atomically #3764. A busy daemon failsdaemon-busy, a daemon without the capability is never replaced, and a live daemon on another transport is quiesced, not killed. A daemon that times out on quiesce is force-retired only after repeated failed liveness probes.daemonBaseUrlconnection never starts or retires a local daemon in either mode;ensureDaemonreads no remote env.30 files incl. docs and changelog. The repo has no changeset tooling, so the
CHANGELOG.mdUnreleased entry is the changeset. Layer 3 adopts this in the CLI and MCP.Validation
pnpm check:affected --run --head d892ab3ed4: passed. New tests cover attach (missing, incompatible, unreachable, timeout),ensureDaemon(start, idle replace, busy, old daemon, transport, contention, unconfirmed retirement) and remote.🤖 Generated with Claude Code