Add Opencode hooks to nitro agent - #10366
Open
michaelstaib wants to merge 83 commits into
Open
michaelstaib wants to merge 83 commits into
michaelstaib wants to merge 83 commits into
Conversation
The first-prompt announcement and idle-push-per-transition gate no longer reserve reserved sentinel ids on the mail delivery ledger (session_deliveries stays real-mail-only). Both are now atomically armed/claimed via two new agent_sessions columns and IAgentSessionRegistry methods, so a Nitro-pushed chat message can skip rearming without touching the ledger at all.
Nitro's push side is expected (hc-10-5n6.2) to prepend a reserved text prefix to a delivered message instead of tagging it with metadata; the generated shim now detects and strips that exact prefix from output.parts (opencode delivers the model-bound message there, not input.parts) and sets nitroPushed on the hook payload. Adds a generated-JavaScript regression, executed under Node, guarding the output.parts requirement.
… command group
Wires the opencode hook handler landed in hc-10-nds.1 into a process
entry point: OpencodeHookExecutor mirrors ClaudeHookExecutor and
CodexHookExecutor's fail-open envelope (stdin/stdout JSON, 10s entry
timeout, NITRO_HOOK_SUPPRESS kill switch, neutral {} on every failure
except a schema mismatch, which reports to stderr with exit 1), and
the hidden `nitro agent hook opencode` command group adds the
session-created, chat-message, session-idle, and session-deleted
leaf commands the generated shim invokes.
Adds the opencode-server branch to ActorWakeDispatcher and PingSessionExecutor: wake pushes the mail digest as a real prompt (prefixed with the reserved nitro push marker so the shim recognizes and skips it) via OpencodeServerClient.PushMessageAsync, guarded by the session registry's idle-push claim so at most one push fires per idle transition and mail during a nitro-pushed turn waits for the next one; when there is nothing left to deliver, a health-only ping via PushAsync's sibling verifies the endpoint instead of a blind ok. Registers IOpencodeServerClient additively in ServiceCollectionExtensions. Existing claude-peer and codex-thread branches are untouched. Ref: hc-10-5n6.2
Move the opencode idle-push claim so it only spends the session's one-shot flag once the gate reservation is already held, instead of before it: a busy gate or a dropped capacity slot no longer burns the claim for nothing. Rearm the claim whenever an attempt did not actually deliver (a terminal failure, an access-denied offer, or a health-only ping that found no digest left to push), so a transport failure or a benign mail-already-read race no longer strands the session waiting on a human prompt. PingSessionExecutor now tags a health-only ping's outcome with a dedicated detail constant so the dispatcher can tell it apart from a delivered push. Name the opencode switch arm explicitly instead of falling through a wildcard. Covers: a busy session gate never spends the idle-push claim, an outright transport failure rearms it, and a health-only ping (no mail left to push) rearms it too.
…ation DispatchTargetAsync could strand an OpencodeServer session's one-shot idle-push claim when the dispatch was cancelled between a successful ClaimIdlePushAsync and the outcome-based rearm (a lost lease renewal or caller shutdown mid-transport). Track the claim with idlePushClaimed/ idlePushSettled flags and hand it back from the inner finally whenever a claimed attempt never reached the existing outcome-based decision.
SessionDeliveryLedger's reserving INSERT is already conditioned on the session row existing, so the FK-violation catch in OpencodeHookHandler.ReserveAsync (and the SessionRemovedDuringDeliveryException it fed) could never fire; a deleted session already reserves nothing via the normal empty-result path. Also documents that dryRun has no effect in this handler (opencode has no session-file side channel to skip). Findings F1 and F6 from hc-10-nds.1's review comment.
…stract Adds back ISessionDeliveryLedger.ReserveAsync(harness, sessionId, ...) as an additive overload with its original byte-identical SQL, and reverts ClaudeHookHandler/CodexHookHandler and their test doubles to call it, so the opencode host-aware overload no longer touches the shipped Claude/Codex paths beyond the additive abstraction. Also makes IAgentSessionRegistry's Arm/Claim announcement and idle-push members abstract instead of default no-ops, and implements them on the fake registry and every test decorator, so an implementation that does not support the markers fails to compile rather than silently doing nothing. Findings F4 and F5 from hc-10-nds.1's review comment.
Seeds a raw v12-shaped agent_sessions table, predating the announcement_pending and idle_push_armed columns, and asserts InitializeAsync adds both defaulted to 0 without losing the existing row. Finding F2 from hc-10-nds.1's review comment.
Adds a chat-message-pushed.json fixture with nitroPushed: true and a test asserting OpencodeHookExecutor deserializes it onto OpencodeHookPayload.NitroPushed. Finding F7 from hc-10-nds.1's review comment.
OpencodeHookHandler.HandleSessionIdleAsync no longer calls ClaimIdlePushAsync or reserves a gate-channel delivery: the dispatcher already claims the flag after its own gate reservation and rearms it on every non-delivering path, so an idle event that beats the daemon no longer spends the one-shot claim with nothing pushed. The event now only touches presence/heartbeat, since the shim discards its response either way. OpencodeHookOutcome.IdleDelivery is removed as dead plumbing along with it. Per the hc-10-5n6.2 planner ruling (option A).
Four minors from the hc-10-5n6.2 review cycle 3: - Split the two ActorWakeDispatcherTests exceeding the 5-Assert limit into separate push/suppress and offer/deliver tests. - Classify 'idle-not-armed' as a transient offer reason in MailWakeDaemonRetryPolicy.IsTransientOffer, matching busy/ capacity-dropped/access-denied. - Deduplicate PingSessionExecutor.ExecuteOpencodeServerAsync against ExecuteAsync via a shared helper, keeping every branch's existing behavior byte-identical. - Persist the opencode health-only ping detail only when it actually changed from the row's own last recorded state, instead of rewriting it on every idle poll.
ActorWakeDispatcher minted a fresh pingAttemptId for every dispatch but never stamped it onto agent_sessions.last_ping_attempt, so the executor's later WritePingResultAsync call (conditioned on that column) always matched zero rows: no ping result was ever durably recorded via the wake path, for any harness. Stamp the attempt through IAgentSessionRegistry.TryClaimPingCooldownAsync right before invoking the executor, mirroring the retired manual ping command's own sequence, so the write actually lands.
Adds a regression proving ActorWakeDispatcher, wired to the real PingSessionExecutor, durably stamps last_ping_attempt and writes last_ping_result on the agent_sessions row - the gap the wake-path fix closes.
Drops the persist-only-when-changed gate in PingSessionExecutor: the prior TryClaimPingCooldownAsync claim already nulls last_ping_result and last_ping_detail before every attempt, so skipping the write for a repeated health-only outcome left the row NULL on alternate wakes instead of restoring it. Every health-only ping now writes back like any other attempt. Also corrects the ActorWakeDispatcher comment describing TryClaimPingCooldownAsync's false-return condition to match its actual predicate (row missing or last_ping_at ahead of now).
…r the main file im3 closed the pre-enable lost-update window for -wal growth only. An ordinary write (the common case: every store connection opens without pooling and checkpoints on close) still moved the main file's mtime during that same window with no reconciliation to catch it, losing the write outright. Capture the main file's LastWriteTimeUtc/Length next to the -wal baseline and re-read both after enabling raising events, emitting one DataChangedEvent if either changed.
A plain `opencode` TUI never binds an HTTP server - it reaches its own server inside a Worker over postMessage RPC - and the plugin's serverUrl getter then falls back to a hardcoded http://localhost:4096 placeholder that is syntactically valid but proves nothing about what, if anything, is listening there. Nitro registered it as a healthy opencode-server endpoint anyway, which is worse than a dead port: 4096 is also opencode's own `opencode serve` default, so a push could land in an unrelated process's session. The shim now reports whether one of the flags that actually make opencode bind (--port / --hostname / --mdns) was present in its own process.argv - opencode exposes no other process-identity probe. The handler only trusts serverUrl, arming the idle-push gate, when that flag was reported; otherwise the session registers as endpoint_kind='none' so the dispatcher records "no-endpoint" honestly instead of spending pushes. The chat.message announcement still arms either way, since it rides the hook response, never HTTP.
…via argv opencode's plugins run inside a Bun Worker whose process.argv never carries the parent process's --port/--hostname/--mdns flags, so the argv mirror in the shim always read false and any push aimed at a TUI-with-explicit-port session was wrongly demoted to endpoint_kind 'none' (see the NEEDS-PLANNER comment on hc-10-w61.1). Detect the bind in the plugin's own realm instead: opencode's serverUrl getter returns the same URL object on every read once a server actually bound, and a fresh placeholder URL on every read otherwise. Reading it twice and comparing by reference proves the bind without argv or a process-identity probe. Also stop arming the idle-push gate on a genuine chat message for a session whose endpoint was never trusted (endpoint_kind='none'): the dispatcher can never spend a push into it, so arming it there was spending nothing but still lying about the session's state.
StatusOpencodeHooksCommand reported only whether the plugin file was missing, current, or outdated - never whether a session's registered endpoint could actually take a push, so a session with 6/6 failing pings still looked fine. It now lists every opencode session this instance knows about next to what is actually known about its push path: the registered endpoint, a reachability derived from endpoint_kind and the last recorded ping (never a fresh probe, and never phrased as proof the agent did anything - Ok only means the async push route accepted the run), and the raw last-ping result/detail. An unreachable session (no trusted endpoint, or a failing one) gets the same remedy note install now states upfront: start opencode with an explicit --port, --hostname, or --mdns flag. A missing agent workspace (hooks can be installed and checked before `agent init` runs) yields an empty session list rather than an error.
…'s failed ping
hc-10-w61.4 follow-up: the previous commit derived Reachability=Unreachable
(and thus the "Pushes will not arrive" remedy) from any non-ok last ping
result, including endpoint-gone/error/timeout on a session whose endpoint
was actually a proven, bound opencode-server (see hc-10-w61.1). A single
recorded ping failure is not proof the endpoint is unreachable, and per
alex's binding comment on this ticket, status must report only what is
actually known and never synthesize a verdict.
The --port/--hostname/--mdns remedy now prints only when
endpoint_kind='none' - the one case Nitro can say for certain that a push
has nowhere to go. Reachability for a registered endpoint now reflects only
what the last recorded ping actually says: 'ok' -> reachable at last ping,
'endpoint-gone' -> unreachable at last ping (the client's own recorded
verdict), everything else (never pinged, timeout, error, capacity-dropped,
unsupported) -> unknown. The raw last-ping result/detail is unchanged.
Adds a regression test seeding a bound opencode-server session with a
failed last ping, proven to fail against the prior commit on the
DoesNotContain("Pushes will not arrive") assertion.
…opencode status
hc-10-sv7, cleanup follow-ups from hc-10-w61.4's cycle-2 review:
- endpoint-gone is recorded for a non-2xx answer (401/404/500) as well as a
genuine connection failure, so status no longer labels that case
"unreachable at last ping"; it now names the recorded result
("endpoint gone at last ping") without asserting reachability either way.
- refreshed the status command's stale help description, which still only
described the plugin-file check.
- registered the opencode install/status result records with the JSON
source-gen context (missing entirely, so --output json crashed for both
commands) and added tests for the JSON sessions and endpointNote fields.
- documented in agent-hooks.md that opencode pushes need a bound server
(--port / --hostname / --mdns), matching what install and status already
print.
…oard ResolveForInit always hands init the nearest board above the current directory, so a folder under an already-initialized parent could never get its own board and even a bare leftover .nitro/agents directory hijacked a fresh init. Add init-only --database-path <path>, naming a .nitro directory to create the board at directly (<path>/agents, standard fallback layout), bypassing ResolveForInit entirely so no flag is needed afterward. Rejects a value whose last segment is not .nitro, cannot combine with --migrate, and never offers the migrate hint since the board was placed on purpose.
InitAgentCommand's fresh-init and upgrade branches read and wrote the task ID prefix through ITaskStore's config methods, which connect to the nearest board above the current directory rather than the --database-path directory the command had just resolved. A value that was not itself the current directory (a subdirectory, or an absolute path elsewhere) silently operated on the wrong board or failed outright with "No agent workspace found.". Read and write the prefix against the resolved workspace directory directly instead.
…ck-independent hc-10-xd8 closed the main-file startup window by comparing mtime and length against a pre-enable baseline, but that compare is only as good as the file system's timestamp granularity: on ext4 without multigrain timestamps, two writes inside one jiffy can compare mtime-equal, and SQLite's common case is an in-place rewrite that leaves length unchanged too, so a real write can go undetected. Read the SQLite main database header's file change counter (offset 24, incremented on every write transaction) instead, falling back to mtime/length for a file that is absent, shorter than the header, or not a SQLite database. Also add a startup-silence guard: xd8's review found that SettleAsync's drain step let a watcher that emits unconditionally on start still pass 14/14, with nothing asserting silence when nothing was written.
The agent-hooks.md status/install sentence claimed both commands "call this out", but status only names the remedy for a session with no endpoint registered; a session with an endpoint never gets that line. Fix the docs to say what each command actually does instead of widening status's output. The endpoint-gone status test asserted a "404 Not Found" last-ping detail; the opencode transport (OpencodeServerClient / PingSessionExecutor.MapOpencodeResult) never records any detail for EndpointGone, so drive the test with what production actually produces: no detail at all. Rename the template probe test to reflect that it now discriminates both directions (fresh-URL vs. stable-URL fake) and drop the dead stableServerUrl:false branch its single-direction predecessor left in BuildServerBoundDriverScript, since only the discrimination test now covers that side.
…bounce cycle The prior fix widened the tail wait based on observed notifications, which could only ever observe more events for an assertion expecting zero -- a strict weakening, not a fix (see review on hc-10-dfk). Replace the notification seam with an internal OnDebounceTick hook fired at the top of OnTick, before any publish. The test counts debounce cycles and asserts published events never exceed them (1 + extraEvents <= tickCount) after the original fixed wait, instead of asserting zero further events outright. A real coalescing bug (a publish without a matching cycle) still fails; a notification split by OS delivery jitter across two legitimate debounce cycles still passes, since every publish is tied to a counted cycle.
…tion The tick-count assertion landed in edeb875a70 could not fail for a real coalescing bug: every publish in SqliteDbWatcher happens inside OnTick, so published-events <= ticks held by construction for any defect that keeps publishing from the tick path. Restore the OnNotificationObserved seam alongside OnDebounceTick and snapshot the notification count at the first act-phase debounce cycle, so the tail assertion compares extra publishes against notifications that arrived after that first cycle instead of against tick count. A legitimate split delivery (a late notification produces its own cycle and event) still passes; a publish with no notification to account for it now fails, proven against an injected regression that leaks mainDatabaseChanged and re-arms the timer periodically.
…BurstOfWrites Per planner ruling on hc-10-dfk comment 108: when no act-phase debounce cycle is ever observed, notificationsAtFirstTick stays at its -1 sentinel. Math.Max(notificationsAtFirstTick, 0) silently collapsed that into a legitimate 0, which subtracts to the raw notification count and makes the tail assertion accept any number of extra events -- a state the test cannot fail, which is exactly what cycles 1 and 2 rejected. Assert the sentinel never survives to the comparison instead, with a message naming the missing boundary. Also read notificationsAtFirstTick with Volatile.Read at its one use site: it is written by Interlocked.CompareExchange on the timer thread, and its sibling counter was already read that way.
hc-10-949 deleted NotifierTests.cs but left its fake behind; git grep FakeActorWakeDispatcher now matches only its own declaration.
session-idle is a heartbeat-only touch since hc-10-o8h; it delivers nothing itself regardless of which event shape raised it, and any push happens out of band through the idle wake dispatcher.
The enable-gap regression test had drifted onto the same counters-1-to-2, same-length shape as the mtime/length-unchanged case, discriminating nothing the latter did not already cover. Land a length-changing SQLite write in the enable gap instead, so the test covers the gap and the length signal together, distinct from the equal-mtime-and-length case and the equal-counter replacement case.
… comment The enable-gap test's comment called the MainFileReplaced sibling 'above' when it sits below, and called the ChangeCounterAdvances sibling 'below', which happened to be right but was one reorder away from rotting the same way. Both fully-qualified test names already identify their target, so the directional words added nothing but a way to go wrong again; drop them instead of just swapping the one word.
# Conflicts: # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Notify/ActorWakeDispatcherTests.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Tui/Runtime/SqliteDbWatcherTests.cs
# Conflicts: # src/Nitro/CommandLine/src/CommandLine/Services/Results/JsonSourceGenerationContext.cs # src/Nitro/CommandLine/src/CommandLine/Services/Workspace/AgentDatabase.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Hook/ClaudeHookHandlerTests.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Hook/SessionDeliveryLedgerTests.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Notify/ActorWakeDispatcherTests.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Notify/NotifierTests.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Notify/PingSessionExecutorTests.cs # src/Nitro/CommandLine/test/CommandLine.Tests/Services/Workspace/AgentDatabaseTests.cs
…bering merge The merge (24a4826) renumbered this branch's schema versions against main's, so the same stamped user_version means a different schema depending on which build wrote it. Verified against real databases built from the pre-merge branch tip (c19225a, stamped 13), main's tip (fb6f5ca, stamped 12), and a fresh init: InitializeAsync's unconditional CREATE TABLE IF NOT EXISTS / column-guarded ALTER design already upgrades every provenance to v14 with the full schema union, unchanged row counts, and working tasks/mail/memory/agent commands, so no fix to AgentDatabase.cs is needed. Adds a regression test that seeds a database exactly the way the pre-merge branch build stamped a real v13 workspace (opencode harness, endpoint credential, and the announcement/idle-push columns already present, but no takeover ledger tables, since those only reached the unified schema from main) and asserts the upgrade adds the ledger tables without losing existing rows. Confirmed the test discriminates: removing the unconditional TakeoverLedgerSchema.Create step reproduces the failure (three tests fail with "no such table: agent_takeovers"/"agent_takeover_items").
StatusOpencodeHooksCommand.cs built clean on net10.0, the local convention's target, but the workspace also multi-targets net11.0, where this analyzer runs as a build error. The standing test convention (--framework net10.0) never built that target, so the lambda that could be a method group never surfaced.
cspell flagged mdns in agent-hooks.md even inside backticks. The word belongs in the repo's custom dictionary, not cspell.json's inline words array; added in alphabetical order alongside the existing entries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.