Skip to content

Control IPC versioned hello + consent hardening + one execution domain - #430

Merged
alexeyzimarev merged 27 commits into
mainfrom
ipc-hello-consent-hardening
Aug 2, 2026
Merged

alexeyzimarev merged 27 commits into
mainfrom
ipc-hello-consent-hardening

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

Slice-2 pre-work of the desktop supervisor umbrella (AI-1622), first of the two merge-ordered PRs (AI-1648; AI-1649 follows). Spec, review-hardened across two flows (6 + 11 rounds to clean): docs/superpowers/specs/2026-08-01-slice2-prework-control-ipc-design.md.

What this delivers

1. Versioned hello on the control socket — optional one-shot Hello = 15 → HelloReply = 75 (protocol_version, daemon_version, daemon_name, capabilities: ["consent/1"]). Capability strings are the discovery surface for the future desktop app; the list is assembled next to the routing (LocalControlCapabilities) so a capability can't be advertised without its handler. Down-level discovery is hello-then-EOF (an old daemon's codec can't decode byte 15). Forward compat pinned: unmapped JSON members skipped, omitted capabilities handled null-safe.

2. Consent hardening — the prompt_no_ui instant deny gains a subscriber grace (min(5s, timeout), burned from one monotonic TimeProvider deadline — total wall time never exceeds the policy timeout; zero-budget arrivals settle as prompt_timeout). Broker gains a generational subscriber-arrival waiter (arrival wins ties; 1→0 re-arm only on real removals). External cancellation now aborts without fabricating a consent decision or a log record.

3. One execution domain for server launch/stop commands — the SignalR receive pump no longer awaits launch/stop execution; both command formats execute in arrival order on the processor's single serial lane (typed SubmitUnsequenced → Committed|Coalesced|Refused|DroppedUnknownTarget, every predicate under one lock). Instance-counted active-launch tracking keeps a dequeued, consent-parked launch a valid stop target; admissibility includes PID-record survivors so the server's retry-until-gone physical stop keeps working; launch-aware per-(target, payload) stop coalescing with an edge-triggered depth alarm; shutdown settles accepted sequenced items via synthesized terminal answers and discards un-seq'd items (daemon teardown supersedes them). Nothing is ever refused for its command format — the shipped server mixes formats by design.

An earlier daemon-lifetime "format latch" design was implemented, then reverted mid-branch after review verified against kcap-server that the server legitimately mixes formats (the latch would have broken launches and silently discarded stops after the first review flow); the commit history deliberately preserves that correction trail.

Testing

Full unit suite baseline-exact vs main (5084 tests; all failures byte-identical to main's known ~/.codex-area baseline). New coverage: FrameCodec/hello wire pins, real-socket hello tests, the grace/deadline matrix on FakeTimeProvider, broker waiter state machine, and the one-domain matrix (cross-format ordering both directions, instance-count pins, transition-lock pin, coalescing/saturation/alarm-hysteresis pins, shutdown settlement incl. done-task exactly-once, teardown-reap + next-boot handoff seam). Every parkable test wait is bounded — a regression fails fast instead of hanging the suite.

Refs: AI-1648 (this PR), AI-1622 (umbrella), AI-1649 (next: supervision IPC, rebases on this).

🤖 Generated with Claude Code

alexeyzimarev and others added 24 commits August 1, 2026 18:04
…vision surface

Slice-2 pre-work of the desktop supervisor umbrella: versioned hello frame,
subscriber grace for the no-UI consent deny, pump-only receive-loop unparking,
and the StatusSubscribe/DaemonStatus supervision surface. Two PRs, one spec.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pre-hello daemon's codec throws on frame byte 15 and drops the connection
without a reply, so the app detects a down-level daemon by the absent
HelloReply, not by an Error frame.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…subscribe

Review findings on the subscriber-arrival waiter state machine:
- Unsubscribe re-armed _subscriberArrival whenever the map was left empty,
  including a 0->0 no-op remove (unknown/duplicate id). That orphans any
  waiter holding the pre-existing generation's source until its full wait
  budget expires. Gate the re-arm on TryRemove actually succeeding.
- Document why PromptAsync's CancelAfter still runs on the system clock
  (the TimeProvider parameter is threaded but not yet wired) so a future
  fake-time test can't silently pass while the real timeout never fires.
- Explain why _subscriberArrival needs RunContinuationsAsynchronously
  (Subscribe holds _deliveryGate when it completes the source).
- Rename the LaunchConsentIpcTests polling helper off the production API
  name it shadowed (WaitForSubscriberAsync -> SpinUntilSubscribedAsync).

Added a regression test pinning the 0->0 no-op case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Threads the TimeProvider injected in Task 3 end-to-end: the gate anchors one
monotonic deadline at prompt-path entry, derives every wait (grace,
PromptAsync timeout) from Remaining() computed immediately before waiting,
and lets external CancellationToken firing propagate uncaught (no fabricated
decision, no decision-log record). The broker's PromptAsync timeout now
runs on the same injected clock (WaitAsync(timeout, time, ct) replacing the
CancelAfter linked-CTS), with cancellation claiming the entry like a timeout
but always rethrowing instead of ever returning a resolver's verdict.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The latch's premise was false: the shipped server mixes sequenced and
un-sequenced commands by design (sequenced rides only the review-flow
settlement lane; ordinary launches and every stop are un-sequenced), so a
daemon-lifetime latch would break normal launches and silently discard
stops after the first review flow. All server-origin launch/stop execution
now routes through the one existing serial lane in arrival order; nothing
is refused; queue bounds come from the server's capacity-gated dispatch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Handler classification for every unparked handler, synchronous enqueue
contract on SubmitUnsequenced (both cross-format directions pinned),
per-agent stop coalescing as the count bound, processor publication rule
with the bounded transition residual, shutdown-only token provenance, and
lane fault isolation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lane start gate drains the one inline legacy item before any lane
execution (single-assignment processor, per-boot epoch); launch-aware stop
coalescing; stop admission bounded to known targets (drop-at-enqueue is
observably identical to the eventual no-op); bool commit-or-refuse
SubmitUnsequenced; pinned lane shutdown order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lets; settle accepted sequenced items on shutdown

Typed SubmitUnsequenced with all predicates under the processor lock,
active-launch tracking so an executing consent-parked launch stays an
admissible stop target, transition lock with placeholder-TCS reservation,
corrected payload-class bound formula, shutdown settlement of accepted
sequenced items, and restoration of the processor-null and internal-bypass
bullets a prior edit script accidentally deleted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ration; honest bound formula

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stop-queue cap

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bmission outcomes

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…+ edge-triggered alarm

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng proof; alarm hysteresis

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t residual

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reverts task 5's daemon-lifetime `mixed_command_formats` latch, whose premise was
false: the shipped server mixes command formats permanently by design — the
sequenced tuple rides only the review-flow settlement lane, while ordinary
launches and every stop are un-sequenced. The latch would have bricked every
dashboard launch and silently discarded every stop after the first review flow.

Nothing is refused for its format any more. All server-origin launch/stop
EXECUTION — sequenced and un-sequenced alike — runs in arrival order on the one
existing serial lane, so the pump's serialization is relocated rather than
changed and cross-format ordering holds by construction:

- SequencedCommandProcessor gains a typed SubmitUnsequenced(UnsequencedItem) ->
  Committed | Coalesced | Refused | DroppedUnknownTarget, with every predicate
  and mutation in ONE critical section before it returns: stop admissibility
  against the injected target probe union the in-flight-launch set,
  instance-counted active-launch tracking retired by ONE terminal-finalization
  path, launch-aware pending-stop key clearing, (agent, payload-class)
  coalescing with identity-guarded retirement at dequeue, and an edge-triggered
  queued-stop alarm with 128/60s hysteresis plus current/high-water metrics.
- The sequenced mutations live in SubmitLocked's ACCEPT branch only; duplicate
  replays and every rejection class mutate nothing.
- No handler awaits execution (launch or stop); a null processor keeps the
  shipped inline await, reserving an inline slot under the one lock that also
  guards publication, which the lane awaits before its first item.
- Shutdown closes the lane before teardown, synthesizes terminal answers for
  accepted queued sequenced items (completing their done-tasks exactly once),
  discards queued un-sequenced ones, retires every active token and returns the
  queued-stop counter to zero.

Internal reaping and local-socket stops keep bypassing the lane deliberately.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l in production

Both corrections ground the spec in verified code reality found during the
rework: the registry-independent physical stop targets prior-incarnation
survivors that exist only in PID records, and the processor is built in
the orchestrator constructor.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…docs

Review round on the one-execution-domain change:

- Every `Submit*ForTest` call in the orchestrator-level pins is now wrapped in
  WaitBoundedAsync. Those tests park the lane and release it later in the same
  test, so a regression re-adding an execution await inside a handler would block
  before the release ran — and with no [Timeout] on them that is a suite hang
  rather than a named failure.
- Spec §3.3 no longer claims the queued-stop depth counters are exported metrics:
  they are carried in the alarm message and exposed as accessors for a future
  status surface. DaemonStatusReport is deliberately untouched (wire change).
- Spec §3.3 also gains the half of the PID-record admission correction its
  ratifying commit left out: admissible stop targets are the registry, durable
  PID records and active launch instances, because the stop path reaps a prior
  incarnation's survivor by record for an id this incarnation never registered —
  a real action, not the no-op that justifies dropping.
- The plan no longer asserts the retracted cross-format latch outside its
  SUPERSEDED note: the goal line, the latch trip-condition constraint, the
  "no queue/worker for legacy commands" constraint and Task 6's CLAUDE.md
  instruction all describe the one serial execution domain instead.
- StopAcceptingForShutdown's two stacked <summary> blocks are split, with the
  lane shutdown-order paragraph moved onto DisposeAsync.
- AgentPidRecordStore.Exists documents its failure asymmetry: a filesystem fault
  answers false and drops that one survivor stop, safe only because the server's
  retry-until-gone lane re-sends.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ect index

Documents AI-1648's versioned hello frame pair + LocalControlCapabilities
discovery, the consent subscriber-grace/TimeProvider deadline discipline, and
the one-execution-domain routing for server launch/stop commands, pointing to
the slice-2 pre-work spec.
@linear-code

linear-code Bot commented Aug 2, 2026

Copy link
Copy Markdown

AI-1648

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Control IPC Hello/HelloReply, consent prompt hardening, and one command execution lane

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add optional Hello/HelloReply handshake on the local control socket for version + capability
 discovery.
• Harden consent prompting with subscriber grace, monotonic deadline budgeting, and
 cancellation-safe teardown.
• Execute server launch/stop commands on one serial processor lane to preserve cross-format arrival
 order.
Diagram

graph TD
A["Local client"] --> B["Core LocalIpc"] --> C["LocalControlServer"] --> D["HelloReply + caps"]
G["AgentOrchestrator"] --> H["SequencedCommandProcessor"]
G --> E["LaunchConsentGate"] --> F["LaunchConsentBroker"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add a second queue/worker for unsequenced commands
  • ➕ Keeps the existing sequenced lane narrowly scoped to review-flow settlement traffic
  • ➕ Potentially reduces coupling between legacy unsequenced traffic and sequenced machinery
  • ➖ Breaks the primary guarantee: cross-format arrival-order execution
  • ➖ Introduces a second execution domain (more races, harder shutdown semantics)
2. Command-format latch (first format dictates allowed subsequent commands)
  • ➕ Simplifies admission rules by forbidding format mixing within a daemon epoch
  • ➖ Incorrect for the shipped server, which mixes formats by design; would drop valid commands
  • ➖ Unsequenced stops have no reply surface, so latch-based drops are silent and dangerous
3. Keep prompt_no_ui instant-deny for prompt policies
  • ➕ Simpler state machine and fewer timing edge cases
  • ➖ Preserves the reconnect/startup race where a UI subscriber is milliseconds away
  • ➖ Makes prompt behavior sensitive to scheduling jitter rather than policy timeout

Recommendation: The PR’s approach is the best fit: capability-based Hello provides an extensible discovery surface for desktop supervision, and the consent grace + monotonic deadline discipline closes a real race without exceeding policy timeouts. Committing unsequenced commands onto the existing processor lane is the simplest way to guarantee cross-format arrival order while avoiding a second concurrency domain; the reviewed-and-reverted format latch design is rightly avoided given server behavior.

Files changed (27) +4149 / -169

Enhancement (8) +88 / -1
FrameCodec.csTreat Hello/HelloReply as text frames in codec allowlists +2/-0

Treat Hello/HelloReply as text frames in codec allowlists

• Adds Hello and HelloReply to the codec’s text-frame encode/decode switches so their JSON payloads flow through the existing Text machinery.

src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs

FrameType.csAdd Hello=15 and HelloReply=75 frame type bytes +2/-0

Add Hello=15 and HelloReply=75 frame type bytes

• Extends the append-only frame-type enum with the new hello handshake frame values for control IPC discovery.

src/Capacitor.Cli.Core/LocalIpc/FrameType.cs

HelloIpc.csIntroduce hello DTOs and snake_case source-generated JSON context +22/-0

Introduce hello DTOs and snake_case source-generated JSON context

• Adds ClientHelloDto and HelloReplyDto plus a source-gen JsonSerializerContext using snake_case; reply capabilities are nullable to support older daemons omitting the field.

src/Capacitor.Cli.Core/LocalIpc/HelloIpc.cs

LocalFrame.csAdd LocalFrame.HelloJson helper for hello payload frames +4/-0

Add LocalFrame.HelloJson helper for hello payload frames

• Adds a dedicated constructor helper for Hello/HelloReply JSON payload frames, mirroring the existing ConsentJson pattern.

src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs

DaemonRunner.csRegister TimeProvider and inject it into LaunchConsentGate +4/-0

Register TimeProvider and inject it into LaunchConsentGate

• Adds TimeProvider.System to DI and wires it into LaunchConsentGate so all deadline/grace timing is monotonic and testable with FakeTimeProvider.

src/Capacitor.Cli.Daemon/DaemonRunner.cs

AgentPidRecordStore.csAdd Exists(agentId) probe used by stop-target admission +20/-0

Add Exists(agentId) probe used by stop-target admission

• Introduces a cheap, non-parsing existence check for PID records to support stop admissibility inside the processor lock; fail-closed on filesystem errors with debug logging.

src/Capacitor.Cli.Daemon/Services/AgentPidRecordStore.cs

LocalControlCapabilities.csDefine advertised control-socket capability list +13/-0

Define advertised control-socket capability list

• Adds LocalControlCapabilities.Current (currently ["consent/1"]) with an invariant that capabilities are only advertised when routed by LocalControlServer.

src/Capacitor.Cli.Daemon/Services/LocalControlCapabilities.cs

LocalControlServer.csRoute Hello and reply with daemon version, name, and capabilities +21/-1

Route Hello and reply with daemon version, name, and capabilities

• Adds Hello handling to the first-frame router; parses client hello payload as diagnostics-only (malformed JSON tolerated) and replies with HelloReplyDto serialized via HelloIpcJsonContext.

src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs

Bug fix (2) +111 / -26
LaunchConsentBroker.csAdd generational subscriber-arrival waiter and cancel-safe prompt behavior +78/-22

Add generational subscriber-arrival waiter and cancel-safe prompt behavior

• Implements WaitForSubscriberAsync using a generational 0→1 arrival TaskCompletionSource (re-armed only on real 1→0 transitions). Updates PromptAsync to use TimeProvider-based waits and to rethrow external cancellation after claim-or-defer cleanup (no fabricated verdict/decision-log entry).

src/Capacitor.Cli.Daemon/Services/LaunchConsentBroker.cs

LaunchConsentGate.csAdd subscriber grace and monotonic deadline discipline to prompt policy +33/-4

Add subscriber grace and monotonic deadline discipline to prompt policy

• Adds a bounded grace wait (min(5s, timeout)) before prompt_no_ui denial and re-budgets prompt time from one monotonic anchor so total wall time never exceeds policy timeout. Ensures external cancellation aborts without producing a decision/log record.

src/Capacitor.Cli.Daemon/Services/LaunchConsentGate.cs

Refactor (2) +860 / -114
AgentOrchestrator.csDetach pump from execution and commit unsequenced commands onto the processor lane +329/-34

Detach pump from execution and commit unsequenced commands onto the processor lane

• Changes server command handling so launch/stop execution is no longer awaited on the SignalR receive pump; unsequenced launch/stop are committed to the processor’s single serial lane. Adds a publication/transition barrier (test seam), stop-target admission that includes PID-record survivors, and updated handler wiring to preserve intended bypasses for internal reaping/local stops.

src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs

SequencedCommandProcessor.csUnify sequenced + unsequenced execution and add stop admission/coalescing/shutdown rules +531/-80

Unify sequenced + unsequenced execution and add stop admission/coalescing/shutdown rules

• Adds SubmitUnsequenced to commit unsequenced launch/stop onto the same serial lane as sequenced items, with active-launch instance tracking (for consent-parked launches), stop admission + coalescing by (target,payload), and a queued-stop depth alarm with hysteresis. Implements shutdown behavior that refuses new submissions, settles accepted sequenced items with synthesized terminal answers, and discards unsequenced items (daemon teardown supersedes them).

src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs

Tests (10) +2154 / -28
AgentOrchestratorVendorTests.csExtend orchestrator test harness for TimeProvider, shutdown lifetime, and deferred publication +13/-5

Extend orchestrator test harness for TimeProvider, shutdown lifetime, and deferred publication

• Updates the shared harness to pass TimeProvider into LaunchConsentGate and to optionally supply a cancellable host lifetime and deferred processor publication for one-execution-domain tests.

test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs

HealBarrierReportTests.csAdjust sequenced server double for detached execution and successful registration flows +18/-1

Adjust sequenced server double for detached execution and successful registration flows

• Updates commentary/guards for the detached execution model and extends the SeqCaptureServerConnection test double to no-op AgentRegisteredAsync to avoid hub invocation during successful launches.

test/Capacitor.Cli.Tests.Unit/Daemon/HealBarrierReportTests.cs

LaunchConsentBrokerTests.csAdd FakeTimeProvider-driven tests for subscriber arrival waiting and time-aware prompts +176/-6

Add FakeTimeProvider-driven tests for subscriber arrival waiting and time-aware prompts

• Updates PromptAsync call sites to pass TimeProvider and adds coverage for WaitForSubscriberAsync’s generational behavior and tie-breaking semantics using FakeTimeProvider.

test/Capacitor.Cli.Tests.Unit/Daemon/LaunchConsentBrokerTests.cs

LaunchConsentGateTests.csAdd FakeTimeProvider tests for grace wait and monotonic deadline budgeting +184/-4

Add FakeTimeProvider tests for grace wait and monotonic deadline budgeting

• Refactors gate construction to inject TimeProvider and adds deterministic tests proving grace is bounded, prompt budget is remaining-from-anchor, and total prompt wall time never exceeds the policy timeout.

test/Capacitor.Cli.Tests.Unit/Daemon/LaunchConsentGateTests.cs

LaunchConsentIpcTests.csUpdate consent IPC socket harness for TimeProvider gate injection +13/-12

Update consent IPC socket harness for TimeProvider gate injection

• Constructs LaunchConsentGate with TimeProvider.System in the real-socket consent IPC harness and renames the subscribe spin helper for clarity while keeping determinism.

test/Capacitor.Cli.Tests.Unit/Daemon/LaunchConsentIpcTests.cs

LocalControlHelloTests.csAdd end-to-end Unix-domain socket tests for Hello/HelloReply +213/-0

Add end-to-end Unix-domain socket tests for Hello/HelloReply

• Adds real-socket tests validating Hello routing and HelloReply content for client-info, empty, and malformed payloads, and confirms List routing still returns AgentList.

test/Capacitor.Cli.Tests.Unit/Daemon/LocalControlHelloTests.cs

OneExecutionDomainProcessorTests.csAdd processor-level tests for SubmitUnsequenced ordering, admission, coalescing, alarms, and shutdown +778/-0

Add processor-level tests for SubmitUnsequenced ordering, admission, coalescing, alarms, and shutdown

• Introduces a comprehensive suite pinning cross-format execution ordering, stop admissibility, coalescing/key lifecycle, queued-stop alarm hysteresis, fault containment, and shutdown settlement behavior on the processor lane.

test/Capacitor.Cli.Tests.Unit/Daemon/OneExecutionDomainProcessorTests.cs

OneExecutionDomainTests.csAdd orchestrator-level tests for one execution domain and cancellation/shutdown interactions +649/-0

Add orchestrator-level tests for one execution domain and cancellation/shutdown interactions

• Adds orchestrator integration tests covering handler routing onto the single lane, pump unparking while launches are consent-parked, publication barrier behavior (test seam), and cancellation behavior during prompting.

test/Capacitor.Cli.Tests.Unit/Daemon/OneExecutionDomainTests.cs

SequencedCommandProcessorTests.csAdd regression test proving SubmitAsync accepts next items without awaiting execution +25/-0

Add regression test proving SubmitAsync accepts next items without awaiting execution

• Adds a test that blocks an item’s execution but still submits/accepts the next seq item in order, validating the detached-execution assumption required by the unparked pump.

test/Capacitor.Cli.Tests.Unit/Daemon/SequencedCommandProcessorTests.cs

FrameCodecHelloTests.csAdd unit tests pinning Hello wire bytes and JSON forward/back compat +85/-0

Add unit tests pinning Hello wire bytes and JSON forward/back compat

• Adds codec tests for Hello/HelloReply round-tripping, exact snake_case JSON shapes, unknown-field tolerance, omitted capabilities null behavior, and stable frame byte values (15/75).

test/Capacitor.Cli.Tests.Unit/FrameCodecHelloTests.cs

Documentation (3) +934 / -0
CLAUDE.mdUpdate contributor notes for slice-2 control IPC and supervision pre-work +34/-0

Update contributor notes for slice-2 control IPC and supervision pre-work

• Expands repo guidance to reflect the new control-IPC hello, consent hardening, and one-execution-domain work and associated review expectations.

CLAUDE.md

2026-08-01-ai1648-ipc-hello-consent-hardening.mdAdd implementation plan for hello + consent hardening + one execution domain +250/-0

Add implementation plan for hello + consent hardening + one execution domain

• Adds a task-by-task implementation plan aligned to the spec, including protocol constraints, timing discipline requirements, and test expectations.

docs/superpowers/plans/2026-08-01-ai1648-ipc-hello-consent-hardening.md

2026-08-01-slice2-prework-control-ipc-design.mdAdd approved spec for control IPC hello, consent hardening, and execution lane +650/-0

Add approved spec for control IPC hello, consent hardening, and execution lane

• Introduces the authoritative design spec describing the versioned Hello handshake, consent race hardening, and the single execution domain for launch/stop handling (merge-ordered PR 1/PR 2).

docs/superpowers/specs/2026-08-01-slice2-prework-control-ipc-design.md

Other (2) +2 / -0
Directory.Packages.propsAdd Microsoft.Extensions.TimeProvider.Testing package version +1/-0

Add Microsoft.Extensions.TimeProvider.Testing package version

• Pins the central package version for FakeTimeProvider-based tests used by consent timing and execution-lane alarms.

Directory.Packages.props

Capacitor.Cli.Tests.Unit.csprojReference Microsoft.Extensions.TimeProvider.Testing for unit tests +1/-0

Reference Microsoft.Extensions.TimeProvider.Testing for unit tests

• Adds the TimeProvider testing package to enable deterministic FakeTimeProvider-driven tests.

test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb415ac25c

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// never-published one in a test, or a stuck inline item) still drains instead of hanging here.
await _laneShutdown.CancelAsync();
try { await _laneTask; } catch { /* best-effort */ }
_laneShutdown.Dispose();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Make processor disposal idempotent

DaemonRunner explicitly calls orchestrator.DisposeAsync() and then disposes the host, whose service provider invokes AgentOrchestrator.DisposeAsync() again for the registered singleton. That second invocation calls SequencedCommandProcessor.DisposeAsync() again, but this line disposed _laneShutdown during the first invocation, so the subsequent _laneShutdown.CancelAsync() throws ObjectDisposedException. Consequently, every normal daemon shutdown can fault during host cleanup; retain the cancellation source or otherwise guard repeated disposal.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6b90897.

@qodo-code-review

qodo-code-review Bot commented Aug 2, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. SequencedCommandProcessor comments too verbose ✗ Dismissed 📘 Rule violation ⚙ Maintainability
Description
Newly added comments in SequencedCommandProcessor are extremely long and embed detailed
spec/behavior narratives, reducing maintainability and making the code harder to scan. This violates
the requirement to keep comments concise and prefer self-explanatory code.
Code

src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs[R46-49]

+/// <para>Spec §3.3 (ONE execution domain) widens this from "the sequenced lane" to "the daemon's single
+/// server-command execution lane". <see cref="SubmitUnsequenced"/> commits un-sequenced launches and
+/// stops onto the SAME serial lane — no seq, no identity cache, no acks — so cross-format arrival order
+/// holds by construction. The shipped server mixes formats permanently BY DESIGN (§1.9: the sequenced
Evidence
PR Compliance ID 7 requires concise comments. The cited regions add multi-paragraph, highly detailed
comments (including spec section references and exhaustive behavioral descriptions) that could
largely be replaced with clearer factoring/naming plus a brief pointer to the spec.

CLAUDE.md: Keep code comments concise; prefer self-explanatory code
src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs[46-61]
src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs[122-140]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SequencedCommandProcessor.cs` introduces very large comment blocks (multi-paragraph explanations with spec references and detailed behavioral narration). This makes the file harder to maintain and review, and conflicts with the guideline to keep comments concise and let code structure/naming carry intent.

## Issue Context
The PR already includes an extensive design/spec document; detailed rationale and full behavioral descriptions belong there. In code, keep only short, high-signal comments that explain *why* something is non-obvious (or link to the spec), and rely on naming/factoring for the rest.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs[46-61]
- src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs[122-140]
- src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs[240-246]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Remaining() called twice ✓ Resolved 🐞 Bug ≡ Correctness
Description
In LaunchConsentGate.DecideAsync, wait is computed with `grace < Remaining() ? grace :
Remaining()`, which evaluates Remaining() twice; if time advances between calls, the code can select
a stale grace value that exceeds the true remaining budget, contradicting the stated
single-deadline guarantee.
Code

src/Capacitor.Cli.Daemon/Services/LaunchConsentGate.cs[R56-58]

+        var grace = TimeSpan.FromSeconds(Math.Min(5, policy.PromptTimeoutSeconds));
+        var wait  = grace < Remaining() ? grace : Remaining(); // computed immediately before waiting
+        if (!await prompter.WaitForSubscriberAsync(wait, time, ct))
Evidence
The PR introduces Remaining() as the monotonic time-budget source, but wait is derived from a
conditional that invokes Remaining() twice, making the chosen wait duration potentially stale
relative to the deadline.

src/Capacitor.Cli.Daemon/Services/LaunchConsentGate.cs[40-68]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LaunchConsentGate.DecideAsync` computes `wait` using two calls to `Remaining()`. Because time can advance between these calls, the chosen `wait` can be slightly larger than the true remaining time budget, undermining the intended "one monotonic deadline" discipline.

### Issue Context
This is in the new consent hardening path that introduces a subscriber grace period and derives all waits from a single `TimeProvider` timestamp.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/LaunchConsentGate.cs[48-61]

### Suggested change
Compute `remaining = Remaining()` once immediately before deriving `wait`, then clamp:
- `var remaining = Remaining();`
- `var wait = grace < remaining ? grace : remaining;`
(or equivalent `TimeSpan.FromTicks(Math.Min(grace.Ticks, remaining.Ticks))`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. Mutable capabilities list shared ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
LocalControlCapabilities.Current is a mutable List<string> instance that is passed directly into
HelloReply; this exposes a shared mutable global that can be accidentally modified within the daemon
assembly and can race with JSON serialization if mutated concurrently.
Code

src/Capacitor.Cli.Daemon/Services/LocalControlCapabilities.cs[R11-13]

+internal static class LocalControlCapabilities {
+    public static readonly List<string> Current = ["consent/1"];
+}
Evidence
The PR introduces a shared List<string> for advertised capabilities and directly uses it as the
HelloReplyDto Capabilities payload.

src/Capacitor.Cli.Daemon/Services/LocalControlCapabilities.cs[1-13]
src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs[90-103]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LocalControlCapabilities.Current` is a `static readonly List<string>`, which is still mutable. Because `LocalControlServer` returns this same list instance in every `HelloReply`, any accidental mutation within the assembly can change advertised capabilities at runtime and can potentially race with serialization.

### Issue Context
This list is described as an invariant-backed discovery surface; keeping it immutable helps ensure the advertised list cannot drift from the routing switch.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/LocalControlCapabilities.cs[11-13]
- src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs[90-103]

### Suggested change
Prefer an immutable representation and/or a defensive copy, e.g.:
- Make `Current` an `IReadOnlyList<string>` backed by an array (`string[]`) or `ImmutableArray<string>`.
- When constructing `HelloReplyDto`, pass a new list copy (`new List<string>(LocalControlCapabilities.Current)`) if the DTO must remain `List<string>` on the wire.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread src/Capacitor.Cli.Daemon/Services/SequencedCommandProcessor.cs
Comment thread src/Capacitor.Cli.Daemon/Services/LaunchConsentGate.cs
Comment thread src/Capacitor.Cli.Daemon/Services/LocalControlCapabilities.cs
…capability list

Bot-review findings on the PR: a second DisposeAsync (DI container after the
orchestrator's explicit dispose) would CancelAsync a disposed CTS and throw;
the grace-wait expression read the monotonic clock twice; the capability
list was a mutable shared List handed straight to the reply.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alexeyzimarev and others added 2 commits August 2, 2026 18:41
A UTC step must not stretch or shrink the 60s window between emitted
alarms; compare TimeProvider timestamps instead of wall-clock instants.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…isposal

A launch already past consent keeps running to registration after the
shutdown token fires; settling the lane after the child enumeration let
that late-registered child survive graceful shutdown. Drain first — the
supersession semantics are unchanged because the lane was closed to new
work before anything else. Also: null (not timestamp zero) as the
never-emitted alarm sentinel.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit d222a72 into main Aug 2, 2026
6 checks passed
@alexeyzimarev
alexeyzimarev deleted the ipc-hello-consent-hardening branch August 2, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant