Repository navigation
Auto-install Claude Code plugin during setup - #1
Merged
Merged
Conversation
`kapacitor setup` step 4 now writes the plugin registration directly into Claude Code's settings.json instead of printing a /plugin install command for the user to copy-paste. Users choose the scope (user-wide, project-only, or skip for manual install). Supports --plugin-scope flag for non-interactive mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
Apr 19, 2026
Findings #1–4 from qodo-code-review bot on PR #22: 1. Wrong auth base URL — CreateAuthenticatedClientAsync() defaulted its auth-discovery baseUrl to AppConfig/KAPACITOR_URL/localhost, which can diverge from the daemon's --server-url. Pass _baseUrl explicitly in all three phase handlers. 2. Cache TTL breaks long evals — the 15-min hard TTL is shorter than worst-case eval runtime (13 questions × 5 min each). Switch to 30-min sliding expiration keyed off last Get(), so only abandoned entries age out. 3. Abandoned contexts never reaped — expiry was only checked inside Get(), so a server crash between Prepare and Finalize leaked TraceJson until daemon restart. Add a 5-min sweep Timer; make the cache IDisposable so DI shutdown cleans it up. 4. Finalize exception leaks cache — _cache.Remove() was only on the success path; a throw from FinalizeAsync left the entry behind. Move the removal into a finally block. Finding #5 (Cancel handler doesn't stop in-flight work) is deferred to PR 3, which wires user-facing cancellation end-to-end; the PR description already calls this out as a known gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
Apr 19, 2026
) * [DEV-1463] Add per-question eval command + result DTOs * [DEV-1463] Add EvalContextCache for per-run daemon state * [DEV-1463] Thread taxonomy through EvalContext for per-phase access * [DEV-1463] Rewrite daemon handlers for per-question eval dispatch * [DEV-1463] Remove obsolete RunEvalCommand from CLI * [DEV-1463] Address Qodo review findings on eval context cache Findings #1–4 from qodo-code-review bot on PR #22: 1. Wrong auth base URL — CreateAuthenticatedClientAsync() defaulted its auth-discovery baseUrl to AppConfig/KAPACITOR_URL/localhost, which can diverge from the daemon's --server-url. Pass _baseUrl explicitly in all three phase handlers. 2. Cache TTL breaks long evals — the 15-min hard TTL is shorter than worst-case eval runtime (13 questions × 5 min each). Switch to 30-min sliding expiration keyed off last Get(), so only abandoned entries age out. 3. Abandoned contexts never reaped — expiry was only checked inside Get(), so a server crash between Prepare and Finalize leaked TraceJson until daemon restart. Add a 5-min sweep Timer; make the cache IDisposable so DI shutdown cleans it up. 4. Finalize exception leaks cache — _cache.Remove() was only on the success path; a throw from FinalizeAsync left the entry behind. Move the removal into a finally block. Finding #5 (Cancel handler doesn't stop in-flight work) is deferred to PR 3, which wires user-facing cancellation end-to-end; the PR description already calls this out as a known gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
Apr 20, 2026
Findings #1–3 from qodo-code-review bot on PR #23: 1. Empty CSV treated as unset — Parse("") used to return null, conflating "flag absent" with "flag present, empty value". Resolver then fell through to the full-catalog path instead of the zero- selection error. Parse now returns null only when csv itself is null; "" and ",,," now produce an empty array, flow into Resolve, and HandleEval exits 2 with "selection resolved to zero questions". 2. Missing flag value misparsed — `kapacitor eval --questions --skip safety <sid>` silently treated "--skip" as the value of --questions, producing a confusing "unknown token" error from the resolver. Guard in Program.cs now detects values starting with "--" and exits 2 with a "requires a value" message before dispatch. 3. Duplicate question IDs can crash — Resolve previously used ToDictionary(q => q.Id) which throws ArgumentException on duplicate keys, crashing the CLI if a misbehaving server returns a malformed catalog. Replaced with a TryAdd loop that surfaces a controlled error ("catalog contains duplicate id '...'") instead. Added tests for all three: Parse's null/empty/commas/whitespace behavior, Resolve's empty-selection and duplicate-ID paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 of 3 tasks
alexeyzimarev
added a commit
that referenced
this pull request
Apr 20, 2026
* [DEV-1463] Add CLI eval question selection resolver * [DEV-1463] Add --questions / --skip / --list-questions to kapacitor eval * [DEV-1463] Address Qodo review findings on CLI eval flags Findings #1–3 from qodo-code-review bot on PR #23: 1. Empty CSV treated as unset — Parse("") used to return null, conflating "flag absent" with "flag present, empty value". Resolver then fell through to the full-catalog path instead of the zero- selection error. Parse now returns null only when csv itself is null; "" and ",,," now produce an empty array, flow into Resolve, and HandleEval exits 2 with "selection resolved to zero questions". 2. Missing flag value misparsed — `kapacitor eval --questions --skip safety <sid>` silently treated "--skip" as the value of --questions, producing a confusing "unknown token" error from the resolver. Guard in Program.cs now detects values starting with "--" and exits 2 with a "requires a value" message before dispatch. 3. Duplicate question IDs can crash — Resolve previously used ToDictionary(q => q.Id) which throws ArgumentException on duplicate keys, crashing the CLI if a misbehaving server returns a malformed catalog. Replaced with a TryAdd loop that surfaces a controlled error ("catalog contains duplicate id '...'") instead. Added tests for all three: Parse's null/empty/commas/whitespace behavior, Resolve's empty-selection and duplicate-ID paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
May 8, 2026
#1: StartForegroundAsync's finally block deleted agent.pid unconditionally, which orphaned a concurrent legitimate daemon's PID file in this race: 1. Process A's daemon-A exits cleanly. Process A enters finally. 2. Process B's `kapacitor agent start` reads the PID file, sees PID-A; IsOurDaemon returns false (PID-A's process is gone), guard passes. 3. Process B spawns daemon-B, writes PID-B. 4. Process A's finally deletes the PID file — orphaning daemon-B. Now we re-read agent.pid in finally and only delete if it still matches the process we spawned. Belt-and-braces against PID race losses. #2: StatusCommand had its own PID-file parser that did ReadAllText().Trim() + int.TryParse(...) — which fails on the multi-line PID|StartTicks format AgentCommands.WritePidFile produces. Without the foreground guard this only triggered for `-d` daemons; this PR makes foreground daemons hit it too. Parse the first non-empty line instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 of 4 tasks
alexeyzimarev
added a commit
that referenced
this pull request
May 8, 2026
… is alive (#51) * [AI-78] Refuse foreground `kapacitor agent start` when another daemon is alive The detached path (`-d`/`--detach`) has guarded against duplicate launches via the PID file since day one. The foreground path didn't, so a second `kapacitor agent start` (run by mistake, by an automation, by a parallel terminal) would happily spawn a fresh kapacitor-daemon process. That fresh process calls DaemonConnect with empty live_agents (orchestrator not yet wired → GetLiveAgentIds returns []), the server's Register silently replaces the active daemon's slot, ReconcileDaemon([]) flips every running hosted agent to Failed, and the displaced real daemon keeps its WebSocket open without ever noticing. Mirror the detached path's PID-file check into StartForegroundAsync, write the PID file when the foreground daemon launches (so subsequent starts in any mode see it), and clean it up on exit. IsOurDaemon's StartTime check already handles the recycled-PID case if the parent dies hard and leaves a stale file. Server-side belt: kurrent-io/Kurrent.Capacitor PR 590 also tightens the ReconcileDaemon guard to ignore empty live_agents, so the cascade becomes impossible regardless of what produced the second DaemonConnect. Either fix alone closes the bug; both together make it double-safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [AI-78] Address Qodo review: PID-file race + status parser #1: StartForegroundAsync's finally block deleted agent.pid unconditionally, which orphaned a concurrent legitimate daemon's PID file in this race: 1. Process A's daemon-A exits cleanly. Process A enters finally. 2. Process B's `kapacitor agent start` reads the PID file, sees PID-A; IsOurDaemon returns false (PID-A's process is gone), guard passes. 3. Process B spawns daemon-B, writes PID-B. 4. Process A's finally deletes the PID file — orphaning daemon-B. Now we re-read agent.pid in finally and only delete if it still matches the process we spawned. Belt-and-braces against PID race losses. #2: StatusCommand had its own PID-file parser that did ReadAllText().Trim() + int.TryParse(...) — which fails on the multi-line PID|StartTicks format AgentCommands.WritePidFile produces. Without the foreground guard this only triggered for `-d` daemons; this PR makes foreground daemons hit it too. Parse the first non-empty line instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [AI-78] Add OS-level exclusive lock around daemon start Previous PID-file guard had a TOCTOU window: two concurrent `kapacitor agent start` invocations could both observe no live daemon, both spawn daemons, and only one's PID would end up in the file. The loser's daemon would then race-write DaemonConnect with empty live_agents and trip the cascade we're meant to prevent. Add agent.start.lock opened with FileShare.None (POSIX flock(LOCK_EX), Windows native sharing constraint). Foreground holds the lock for the daemon's entire lifetime; detached holds it just for the check + spawn + WritePidFile window before the parent exits. Two concurrent starts now serialize at the OS level: the second's TryAcquireStartLock returns null (IOException from FileShare.None) and it refuses with "Another `kapacitor agent start` is already in progress …" The lock is per-open-handle so even SIGKILL on the parent releases it cleanly. Refactored StartForegroundAsync into two parts: the lock-acquiring guard wrapper and the original spawn body (now SpawnForegroundAsync) to keep the lock-acquire/release scope easy to read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
May 8, 2026
#1: Reliability — TickAsync now total. Both recovery actions (ReRegisterAsync, ForceReconnectAsync) ultimately call into SignalR (InvokeAsync / StopAsync) and can throw on transient transport state (invalid hub state, mid-reconnect cancellation). With the previous code those exceptions escaped TickAsync and faulted the unobserved RunDaemonHeartbeatLoopAsync background Task — silently disabling the daemon's liveness probing forever. Each recovery call now runs through a guarded helper (SafeReRegisterAsync / SafeForceReconnectAsync). A failed re-register escalates to forced reconnect; a failed forced reconnect logs and lets the next 15 s tick retry. Belt-and-suspenders try/catch in RunDaemonHeartbeatLoopAsync as defence-in-depth so a future change accidentally rethrowing from TickAsync still doesn't kill the loop. Two new tests pin the contract: TickAsync must not rethrow when ForceReconnect fails, and must not rethrow when both ReRegister AND ForceReconnect fail. #2: Maintainability — remove ServerConnection.SendHeartbeatAsync. Its last caller was the pre-AI-566 fire-and-forget heartbeat loop, which this branch replaced with the round-trip Ping path. The server-side DaemonHeartbeat hub method stays as a wire-compat alias for older deployed daemons, but on the daemon side the local method is genuinely dead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 of 3 tasks
alexeyzimarev
added a commit
that referenced
this pull request
May 8, 2026
* [AI-566] Daemon round-trip Ping replaces fire-and-forget heartbeat Pairs with the server-side DaemonPing hub method + EvalRunOrchestrator fast-fail on slot replacement (server PR). The current SendAsync heartbeat is one-way, so when DaemonRegistry.Register silently displaces this connection's slot (the staging incident), the daemon never notices — it keeps pumping heartbeats the server drops and the orchestrator's in-flight calls hang for the full per-question timeout. DaemonHeartbeatLoop runs every 15s (under the server's default 30s ClientTimeoutInterval) with a 10s ping deadline. Ping returning false → ReRegisterAsync (slot was displaced under us). Ping throwing/timing out → ForceReconnectAsync (transport is hung; stop the hub so OnClosed → ConnectWithRetryAsync builds a fresh conn and re-registers). Outer cancel exits cleanly so process shutdown doesn't trigger a reconnect storm. The loop sits behind a small IDaemonHeartbeatPort interface so the unit tests can exercise the four classify cases without spinning up a real SignalR HubConnection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [AI-566] Address Qodo review: harden heartbeat tick, drop dead method #1: Reliability — TickAsync now total. Both recovery actions (ReRegisterAsync, ForceReconnectAsync) ultimately call into SignalR (InvokeAsync / StopAsync) and can throw on transient transport state (invalid hub state, mid-reconnect cancellation). With the previous code those exceptions escaped TickAsync and faulted the unobserved RunDaemonHeartbeatLoopAsync background Task — silently disabling the daemon's liveness probing forever. Each recovery call now runs through a guarded helper (SafeReRegisterAsync / SafeForceReconnectAsync). A failed re-register escalates to forced reconnect; a failed forced reconnect logs and lets the next 15 s tick retry. Belt-and-suspenders try/catch in RunDaemonHeartbeatLoopAsync as defence-in-depth so a future change accidentally rethrowing from TickAsync still doesn't kill the loop. Two new tests pin the contract: TickAsync must not rethrow when ForceReconnect fails, and must not rethrow when both ReRegister AND ForceReconnect fail. #2: Maintainability — remove ServerConnection.SendHeartbeatAsync. Its last caller was the pre-AI-566 fire-and-forget heartbeat loop, which this branch replaced with the round-trip Ping path. The server-side DaemonHeartbeat hub method stays as a wire-compat alias for older deployed daemons, but on the daemon side the local method is genuinely dead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 14, 2026
4 of 5 tasks
alexeyzimarev
added a commit
that referenced
this pull request
Jun 26, 2026
…ol cap, README - CreateClientWithinBudgetAsync: the abandoned (timed-out) client-factory task is now observed for ALL terminal states — a late fault (likely during the outage this guards) no longer surfaces as an UnobservedTaskException; the client is disposed only on RanToCompletion. - HookSpool cap: count UTF-8 bytes (not chars) so the 1 MB cap holds for non-ASCII payloads (FileInfo.Length is byte-measured; char counts under-counted). Adds a regression test. - README: document durable lifecycle delivery (failed SessionStart/SessionEnd hooks spool to ~/.config/kcap/spool and replay on the next hook; reaped after 30 days) per CLAUDE.md. (qodo finding #1 'repo budget not enforced' was already resolved by the prior merge-fix commit.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jun 27, 2026
…ations #2: replace the ledger's parent line-count key with a SHA-256 content fingerprint over the parent transcript + children, so a same-line-count mutation (tool part completing, in-place edit, changed/added child) invalidates the skip and re-imports. Fingerprint computed at classify, carried on SourceMeta, recorded after session-end. Document #1 (server returns 200 on swallowed per-event write failure) and #3 (subagent lifecycle hooks return OK on write failure) as known limitations with server-repo follow-ups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jun 27, 2026
…20) (#192) * Surface OpenCode in `kcap status` hooks line AI-921 wired OpenCode end-to-end (installer, plugin, hook dispatcher, import-less live ingest) but left it off the `kcap status` Hooks line and the `kcap status --help` text — the same vendor-surface gap that hit Gemini and Kiro before (PR #169). The vendor fully works; it just wasn't reported, so nothing failed and the miss was invisible to build/tests. - StatusCommand.BuildHooksStatusLine: add `opencode` param + `OpenCode ✓/✗` entry (canonical order, last — like Pi it ships a live-ingest plugin file, not shell hooks), detected via OpenCodeExtensionInstaller.IsInstalled(OpenCodePaths.KcapPlugin()). - help-status.txt: Hooks line now reads "Pi / OpenCode live-ingest extensions". - StatusCommandHooksTests: cover OpenCode in both BuildHooksStatusLine tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * README: list OpenCode in the setup-wizard detection paragraph The `## CLI commands` setup section enumerated detected agents only through Pi, omitting SST OpenCode — the quick-start paragraph already lists it, and CLAUDE.md requires both stay in sync. Mirrors the existing phrasing and notes that, like Pi, OpenCode has no shell hooks so the wizard installs a live-ingest plugin. Flagged by PR review on #178. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Design spec: kcap import --opencode (historical OpenCode import from SQLite) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revise OpenCode import spec per Codex design review - Fix subagent lifecycle order (children before parent session-end) - Resolve subagent watermark via Gemini import precedent (startLine 0 + idempotency) - Soften byte-match claim to final-state/normalizer-compatible; document watermark caveat - Pin part ordering (time_created,id) with empirical validation note - Move SQLite dependency to CLI project (keep daemon/Core AOT-clean) - Use /hooks/set-title for native title; ms epoch conversion - Add edge-cases section; accept shared send-failure behavior (idempotency) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revise OpenCode import spec per Codex 2nd-pass review (rev3) - Replace line-number resume with binary New/AlreadyLoaded classification (live snapshot vs import final-state line spaces are incompatible) - Add importable-line predicate (Pi IsImportRelevantLine analog) for MinLines - Fix synthesis query: order by message chronology, not lexical message_id; LEFT JOIN so empty messages aren't dropped - Strengthen part-ordering pre-merge verification requirements - Document OpenCode-specific send-failure consequence (summary/model recompute) - Make parent/child lifecycle sequencing explicit; deterministic child order - Fix Architecture/Core-vs-CLI contradiction; expand edge cases (no-canonical-event messages, grandchildren, mixed live/historical) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Implementation plan: kcap import --opencode (TDD, 10 tasks) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revise OpenCode import plan per Codex plan review - Add opt-in strict transcript sender (failOnError) — OpenCode aborts before session-end/subagent-stop on batch failure (binary policy has no resume) - Make IsImportRelevantLine role-aware + hidden-aware to match server normalizer - Subagent start/stop use the real temp transcript path; stronger ordering test (agent_id, agent_type, vendor, full POST order) - Order-sensitive assertions (string.Join+IsEqualTo) instead of IsEquivalentTo - Harden AOT gate: explicit RIDs + pipefail, no masked publish failures - Add WAL-writer read test; null-dir / zero-message / timestamp-magnitude cases - Cleaner fixture JSON via JsonObject - Reconcile spec (strict sender, role-aware predicate) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revise plan+spec per Codex 2nd plan review: completeness-gated repair Blocker fix: binary AlreadyLoaded couldn't repair a partial multi-batch import (server HWM advances on transcript ingest, not session-end). Replace with completeness-gated classification: - New / AlreadyLoaded(ended) / Partial-repair(watermark, not ended) / TooShort - repair replays full transcript with line numbers offset above HWM (lineNumberOffset param added to SendTranscriptBatches); dedup by canonical prt_ id - strict sender's role clarified: keeps session not-ended on failure so re-run repairs - per-subsession gating via ?agentId= (skip if SubagentCompleted, else repair) - new Task 0: confirm server contract (ended signal, dedup-by-id, HWM filter) + fallback Also from review: align IsImportRelevantLine with server normalizer (Length>0, assistant id requirement, tool fields null-checked); AOT gate exit 1 not break; WAL test asserts sidecars + settles Cache=Private; timestamp seconds test; tighten subagent test (child transcript between start/stop); repair integration test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revise plan+spec per Codex 3rd plan review: CLI-side import ledger Codex confirmed no data-loss blocker remains, but the current server exposes no ended signal — so the completeness-gated design would run in always-replay mode (re-send every session each run). Per decision, add a client-side import ledger as the completeness signal instead: - OpenCodeImportLedger (Task 4b): per-machine, per-server record of fully-imported sessions (keyed by server URL + reconstructed line count), AOT-safe source-gen JSON - classification: ledger hit -> AlreadyLoaded (skip); else New / Partial-repair - ledger written only after session-end succeeds; strict sender keeps partials unrecorded - drop the speculative server ended-signal plumbing (ServerState); Task 0 now confirms only dedup-by-id + HWM filter - children: no per-child gate (complete parent skipped wholesale via ledger) - tests isolated via fixture LedgerPath; add ledger round-trip, second-run-skip, and batch2-failure-then-rerun-repairs (WireMock scenario, 150 lines) tests Also from review: HasField -> string-kind (server Str parity); checked offset arithmetic for overflow; fix stale no-watermark-left comment; Task 0 reframed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Task 0 confirmed: kcap-server repair contract verified against source Verified against kurrent-io/kcap-server@main: - dedup by canonical EventId (HashGuid of prt_/message id), line-number-independent - HWM filter drops line_number <= currentHwm before normalization - last-line returns last_line_number only (no ended field -> ledger is required); reads max lineNumber over last 50 events backward (under-report caveat noted) - HWM + dedup keyed sessionId|agentId; last-line accepts ?agentId= Repair design inherits the live watcher reconnect-resend idempotency profile. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * build: add Microsoft.Data.Sqlite to CLI for OpenCode import Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: OpenCodeDb read-only reader, line reconstruction, importable predicate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: OpenCode import ledger (client-side completeness record) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: OpenCodeImportSource discovery + ledger-gated classify + import + subagents Includes opt-in failOnError/lineNumberOffset on SessionImporter.SendTranscriptBatches (defaulted; peers unchanged). Parent + child import with strict send + repair-above-HWM. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: wire --opencode import filter + register OpenCodeImportSource Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: OpenCode import integration tests (lifecycle, subagents, repair, ledger) Also fix IL2026/IL3050 in OpenCodeDb: cast to JsonNode so Add(JsonNode?) is chosen over the AOT-unsafe generic Add<T> (per CLAUDE.md). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: document kcap import --opencode (help + README) Corrects the README claim that OpenCode capture is live-only / has no import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * harden OpenCode import per code review: discovery guard, cancellation, overflow - DiscoverAsync: guard db open/query so a corrupt/schema-drifted opencode.db warns and skips OpenCode instead of crashing the whole import run (other vendors) - QuerySessions: skip malformed/null rows instead of aborting the scan - propagate OperationCanceledException out of the import catches (was swallowed as Failed) - checked() on repair line-number offsets (parent + child) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: ledger keyed by content fingerprint; document server-side limitations #2: replace the ledger's parent line-count key with a SHA-256 content fingerprint over the parent transcript + children, so a same-line-count mutation (tool part completing, in-place edit, changed/added child) invalidates the skip and re-imports. Fingerprint computed at classify, carried on SourceMeta, recorded after session-end. Document #1 (server returns 200 on swallowed per-event write failure) and #3 (subagent lifecycle hooks return OK on write failure) as known limitations with server-repo follow-ups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * review: fix NUL byte in source, propagate cancellation in helpers, cite AI-1023 Codex final pre-merge review (no blockers): - replace the literal NUL fingerprint separator with a backslash-u0000 escape so the .cs file is text (rg/tools no longer treat it as binary); runtime unchanged - rethrow OperationCanceledException in the watermark-probe and PostHookAsync catches so cancellation is not masked as ProbeError/hook-failure - name AI-1023 in the spec server-side limitation notes Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 3, 2026
…ailure (review round 2) Addresses follow-up review on the proactive-refresh change: - Legacy fallback (Qodo #2): RefreshIfExpiringAsync read only the per-profile token file, so a pre-upgrade install with only tokens.json got no proactive refresh. Extract LoadWithLegacyFallbackAsync (shared with LoadAsync) and use it, so the legacy token is refreshed — and migrated into the per-profile store when the refresh persists under the lock. - Lock contention (Qodo #3): a 15s cross-process-lock acquisition timeout returned null, which the daemon reported as a refresh Failure (misleading "run kcap login" warning + backoff) even though no endpoint call was made. Add an onLockContended callback (proactive path only; reactive GetValidTokensAsync is unaffected — default null) and a ProactiveRefreshOutcome.Contended that the loop logs at Debug with no warning and no backoff (contention is transient). Qodo #1 (profile-switch miswrite) was already resolved in 783b65b. Adds a TokenRefreshLoop test for the Contended path; legacy fallback is covered via the shared LoadWithLegacyFallbackAsync (LoadAsync's existing fallback tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
alexeyzimarev
pushed a commit
that referenced
this pull request
Jul 4, 2026
* feat(daemon): proactively refresh auth tokens ahead of expiry Auth tokens were refreshed only lazily (on the next call after the access token had already expired), so after an idle period the next hook hit a 401 and forced a `kcap login`. The daemon now runs a low-frequency loop that refreshes the active profile's token *ahead* of expiry, keeping a WorkOS sliding-inactivity session alive for as long as the daemon runs. - TokenStore.RefreshIfExpiringAsync refreshes within a window via the existing cross-process lock (rotation-safe re-read under the lock); no-op for the None provider and when no tokens are stored. Resolves the active profile once and threads it through the read + lock. - TokenRefreshLoop rate-limits attempts to at most one per interval, so a failing refresh (dead/rotated token) or a short-lived token that keeps re-entering the window can't hammer the refresh endpoint every tick. - Wired into AgentOrchestrator alongside the existing heartbeat loops. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(auth): persist refreshed token under the locked profile; skip double-refresh after a peer Addresses two review findings on the proactive-refresh change: 1. Profile-switch miswrite (high): RefreshWorkOSAsync/RefreshGitHubAsync persisted via the active-profile-resolving SaveAsync(StoredTokens) overload, so a `kcap profile switch` while a refresh was in flight could write the locked profile's rotated token into a *different* profile's file — without that profile's lock, corrupting its credentials and leaving the original stale. The refresh delegates now return without persisting; RefreshWithCrossProcessLockAsync persists via SaveAsync(profile, refreshed) under the same profile it locked. Fixes both the proactive and the pre-existing reactive (GetValidTokensAsync) path. 2. Double-rotation after a peer refresh (medium): the under-lock re-read only suppressed a refresh when the token was no longer "due". For a short-lived token (or JwtExpiry's now+5min parse fallback), a token a peer had just refreshed was still inside the proactive window, so the proactive caller rotated it again. Now, if the re-read token changed from the one we read and is still valid, we return it without re-refreshing. The reactive path is unaffected (its predicate is IsExpired, already false for a valid token). Exposes RefreshWithCrossProcessLockAsync as internal for unit testing; adds CrossProcessRefreshTests covering persist-under-locked-profile, peer-refresh suppression, and reactive-path preservation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(auth): legacy-token fallback + distinguish lock contention from failure (review round 2) Addresses follow-up review on the proactive-refresh change: - Legacy fallback (Qodo #2): RefreshIfExpiringAsync read only the per-profile token file, so a pre-upgrade install with only tokens.json got no proactive refresh. Extract LoadWithLegacyFallbackAsync (shared with LoadAsync) and use it, so the legacy token is refreshed — and migrated into the per-profile store when the refresh persists under the lock. - Lock contention (Qodo #3): a 15s cross-process-lock acquisition timeout returned null, which the daemon reported as a refresh Failure (misleading "run kcap login" warning + backoff) even though no endpoint call was made. Add an onLockContended callback (proactive path only; reactive GetValidTokensAsync is unaffected — default null) and a ProactiveRefreshOutcome.Contended that the loop logs at Debug with no warning and no backoff (contention is transient). Qodo #1 (profile-switch miswrite) was already resolved in 783b65b. Adds a TokenRefreshLoop test for the Contended path; legacy fallback is covered via the shared LoadWithLegacyFallbackAsync (LoadAsync's existing fallback tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
Jul 7, 2026
…fault - McpFlowsServer: resolving requester_machine_id no longer aborts the flow. MachineId.Get() can throw on first-run create (unwritable config dir), and the field is optional on the wire, so degrade to null (server falls back to the mirror) instead of failing start_review_flow. (Qodo #295 #2) - README: same-host review flows now run read-only in the requester's live checkout (borrow), not a mirrored worktree — update the start_flow mode description to match the new default. (Qodo #295 #1) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
Jul 7, 2026
…nc (#295) * feat(cli): stamp requester_machine_id on the flow request — activates borrow-cwd [AI-1207] The server's borrow-cwd resolver (kcap-server AI-1207 Phase B) picks the read-only borrow path over a mirrored worktree only when it can prove the reviewer would run on the SAME host as the requester — by matching the request's requester_machine_id against each connected daemon's registration id. Until now the CLI never sent that id, so RequesterMachineId was always null server-side and every flow fell back to the (fragile) mirror. Stamp MachineId.Get() — the same stable machine id the daemon reports at registration (ServerConnection) — onto every start_review_flow request. This is the last piece that activates borrow end to end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(daemon): remove dead mirror-sync now that the server borrows the cwd [AI-1207 Phase C] With borrow activated, the reviewer runs read-only directly in the requester's checkout, so the server (kcap-server AI-1207 Phase B) no longer invokes RefreshAgentWorktree or sends syncFromRepoRoot. The daemon-side mirror machinery is now dead code — remove it in full: - AgentOrchestrator: HandleRefreshAgentWorktree, TrySyncWorktreeAtLaunchAsync, TryReSyncWorktreeForRoundAsync, IsAllowedSyncSourceAsync, the launch/round call sites, DaemonManagedWorktreeExcludes, RoundResyncTimeout, the three Log* mirror partials, and AgentInstance.SyncSourceRepoRoot. - WorktreeManager: SyncFromSourceAsync + its now-orphaned IsUnderExcluded helper and LogSyncCompleted. - ServerConnection: the RefreshAgentWorktree handler property + hub On<> registration. - Models: RefreshAgentWorktreeCommand/Result records + JsonSerializable registrations, and LaunchAgentCommand.SyncFromRepoRoot. The borrow path (BorrowAuthorizer, ProbeBorrowSource, WorkLocation, the origin check GetOriginRemoteAsync) is untouched. Deleted the per-round-resync test file and the SyncFromSource / SyncFromRepoRoot tests; the frozen OldLaunchAgentCommand wire snapshot keeps its historical field. Daemon unit suite: 2620 passed / 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address Qodo review: degrade MachineId failure + fix README borrow default - McpFlowsServer: resolving requester_machine_id no longer aborts the flow. MachineId.Get() can throw on first-run create (unwritable config dir), and the field is optional on the wire, so degrade to null (server falls back to the mirror) instead of failing start_review_flow. (Qodo #295 #2) - README: same-host review flows now run read-only in the requester's live checkout (borrow), not a mirrored worktree — update the start_flow mode description to match the new default. (Qodo #295 #1) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 17, 2026
…byte checkpoint, heartbeat-aware idle ceiling, delivery-boundary re-checks Addresses findings #2, #3, #6, and the watcher-side half of #8/#1 from kcap-cli PR #324 review: - #2: CursorRewriteGuard.VerifyFullPrefix existed but was never called from the poll loop; wire it in on a periodic cadence (CursorFullPrefixVerifyEveryNPolls). Also add VerifyNotShrunk — the guard's zone checks were previously gated entirely behind "did the file grow", so a shrink or in-place same-length rewrite slipped through undetected. - #3: checkpoint the Cursor byte cursor using the SAME capped snapshot length ReadNewCompleteLinesAsync sampled (NewTranscriptLines.SnapshotByteLength), not a fresh FileInfo.Length re-sample racing a concurrent append. Advance the checkpoint only as far as the server's acked LINE count actually covers (ByteOffsetForAckedLines), not the full capped range, so a partially-disposed D3 batch never has its unacked tail checkpointed as delivered. - #6: the Cursor idle clock (ShouldEndOnIdle) is now the later of transcript activity and the hook heartbeat mtime (ResolveCursorIdleClock), and child (subagent) watchers are idle-ceiling eligible without requiring ThresholdReached, which they never set. - #8 (watcher half): re-check the quarantine/barrier markers immediately before SendTranscriptBatchAcked, not only at the top of the poll. - #1 (backfill half): CursorTranscriptBackfill re-checks quarantine/barrier immediately before the POST, closing the same race window on that path. DrainNewLines is now internal (was private) so the guard wiring is directly regression-testable without a live SignalR server — every path exercised trips before ever touching the HubConnection argument. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 17, 2026
…ical import skips quarantined sessions (incl. correlated children) Addresses the replay half of finding #1 and finding #7 from kcap-cli PR #324 review: - #1 (replay half): LifecycleSpoolDrain's transcript poster now parses the batch's vendor/session_id and drops (permanently discards) a Cursor batch whose session is quarantined, and treats a pending side-effect barrier as transient (retry later). This is the only delivery-time check the shutdown-spool-replay path has — a batch queued before a runtime rewrite-guard trip could otherwise still be replayed later. - #7: CursorImportSource.ClassifyAsync now skips (ProbeError) any session whose quarantine IDENTITY is marked — resolved via the already-computed subagentLinks map so a correlated child is filtered under its PARENT's quarantine marker (CursorRewriteGuard is always keyed on the family/parent id for a spawned child watcher), not its own id. Previously `kcap import` had no awareness of the marker at all and could feed the exact corrupted line-number source the guard exists to shut off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 17, 2026
… it actually sends Three related TOCTOU-closing fixes to the D0 rewrite guard, all stemming from the same theme: the guard's hash must be bound to the exact bytes actually processed/sent, and its baseline must be real from the start. 1. Bind-to-sent-bytes (finding #1): ReadNewCompleteLinesAsync now optionally (captureRawBytes: true, wired for vendor=="cursor") captures the raw byte buffer of the SAME capped read that decodes the batch's lines (NewTranscriptLines.SnapshotBytes). DrainNewLines's guard block now hashes directly from that snapshot instead of reopening the file separately to record the new-range/prior-zone hashes — closing the window where a rewrite landing between the decode read and the old reopen produced a hash for bytes the batch never actually came from. The post-ack checkpoint's trailing hash is now derived from the same already-verified snapshot instead of reopening the file after the RPC returns (a rewrite in flight during the ack could otherwise be blessed as the new baseline). 2. Reconnect rewind atomicity (finding #2): a reconnect discovering the server is behind the client now rewinds state.CursorByteOffset and the guard's checkpoint ATOMICALLY with the line cursor, via the extracted ApplyReconnectRewindAsync (testable without a live SignalR reconnect). The true byte offset of the rewound line is resolved by scanning the transcript (ResolveByteOffsetForLineAsync) rather than leaving the byte checkpoint at the later, too-far-ahead offset (which left the replayed line gap's new-range verification starting past the bytes it actually occupies). 3. Real full-prefix baseline (finding #3): the periodic full-prefix re-hash now seeds on the REAL first poll (not just lazily on the guard's own first VerifyFullPrefix call, which previously never happened before poll N) — a same-length rewrite of an already-checkpointed middle region landing in polls 1..N-1 now has a real baseline to be caught against at poll N, instead of poll N seeding the already-rewritten file as if it were the original. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 21, 2026
…(Qodo) - Security (Qodo #3): AcpReviewFlowMcp allowlist now resolves via KcapMcpRegistry.TryResolveReviewFlowAllowlist — the authoritative read-only reviewer policy (ReviewFlowAutoApprovableServers) the orchestrator already enforces for Codex — so a write server like kcap-memory can never reach an auto-approving reviewer. - Observability (Qodo #2): an unknown/flow-starting/non-auto-approvable entry now fails the launch fast with the offending name instead of being silently dropped. Build takes the validated canonical ids. - Maintainability (Qodo #1): trimmed verbose spec-prose comments to concise intent (rationale lives in the design spec). Tests updated: split recursion/dedup into a dedup test + a parameterized fail-fast test (kcap-flows/kcap-memory/kcap-workitems/unknown). 67 tests pass; AOT clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 21, 2026
…P + owned-worktree-gated auto-approve (#337) * [AI-1407] ACP unattended reviewer foundation: flow-result MCP over ACP + owned-worktree-gated auto-approve Wires the vendor-neutral plumbing so a per-vendor child can flip SupportsUnattended and become an unattended review-flow reviewer over ACP. Flips nothing itself — every real vendor stays SupportsUnattended:false, so these paths are unreachable in production (Cursor pinned byte-for-byte). - AcpReviewFlowMcp.Build: the AcpMcpServerSpec analogue of ClaudeLauncher's PTY BuildReviewFlowMcpConfig — kcap-flow-result (KCAP_URL + KCAP_FLOW_AGENT_ID) plus the flow's MCP allowlist resolved via KcapMcpRegistry (flow-starting servers stripped, unknown skipped, deduped by canonical id to match ClaudeLauncher's JsonObject keying). - AcpHostedAgentRuntimeFactory: ValidateAndBuildReviewFlowMcp runs as the first statement of StartAsync, BEFORE the connectionSource spawns, and fails closed for a review-flow launch that isn't unattended-capable, isn't an owned worktree, has no ACP mcpServers support, or can't build a deliverable result channel (nonblank url/path/agent id). BuildProcessStartInfo gains a matching owned-worktree refusal as defense-in-depth. - AcpInteractionBridge: autoApproveUnattended selects the least-privilege allow option by Kind (nonblank + unique OptionId) without routing to a human, and declines elicitations; fails closed otherwise. Audit log pins agentId + kind + untrusted tool title, never a path. Owned-worktree is a launch precondition, NOT filesystem confinement — real containment is a per-vendor live-verification gate for each reviewer child. 62 new/covered unit tests; AOT-clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * AI-1407: enforce read-only reviewer allowlist policy + trim comments (Qodo) - Security (Qodo #3): AcpReviewFlowMcp allowlist now resolves via KcapMcpRegistry.TryResolveReviewFlowAllowlist — the authoritative read-only reviewer policy (ReviewFlowAutoApprovableServers) the orchestrator already enforces for Codex — so a write server like kcap-memory can never reach an auto-approving reviewer. - Observability (Qodo #2): an unknown/flow-starting/non-auto-approvable entry now fails the launch fast with the offending name instead of being silently dropped. Build takes the validated canonical ids. - Maintainability (Qodo #1): trimmed verbose spec-prose comments to concise intent (rationale lives in the design spec). Tests updated: split recursion/dedup into a dedup test + a parameterized fail-fast test (kcap-flows/kcap-memory/kcap-workitems/unknown). 67 tests pass; AOT clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * AI-1407: treat reserved kcap-flow-result allowlist id as a no-op (codex review) The result channel is always injected by AcpReviewFlowMcp.Build and is not a KcapMcpRegistry entry, but the server's DynamicFlowPolicy legitimately lists kcap-flow-result in McpAllowlist. Strip it (case-insensitive) before the strict read-only validation so it is a redundant no-op, not a fail-fast rejection — while still rejecting unknown/flow-starting/write-server entries. Single-sourced as AcpReviewFlowMcp.ResultChannelId. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * AI-1407: normalize reserved kcap-flow-result no-op in TryResolveReviewFlowAllowlist (codex review r2) Move the reserved result-channel normalization from a daemon-side pre-filter into KcapMcpRegistry.TryResolveReviewFlowAllowlist so the ACP reviewer path and the Codex orchestrator path share ONE consistent contract: kcap-flow-result (always launcher-injected, not a registry entry, legitimately listed by the server's dynamic-flow policy) is a satisfied no-op, never re-emitted, never a rejection. Single-sourced as KcapMcpRegistry.ReservedResultChannelId. Core test added; ACP end-to-end no-op test retained. 9+31+37+67 tests pass; AOT clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Jul 28, 2026
…e as a swap Codex review round 1 on this PR. Three findings, all real. P1 #1 -- the retry budget was right for registration and wrong on the launch path: three 10s waits plus backoff is ~30.75s of a thread-pool thread held before a launch can even be rejected, multiplied by concurrent launches. Registration keeps the three attempts (its result is cached for the daemon's lifetime, so a miss there is durable and destructive); the launch path now uses a single attempt. It does not need the retry: a miss there is no longer misclassified, produces a retryable rejection, and the caller can try again. P1 #2 -- the same bug I fixed at registration was still live at LAUNCH. With a version advertised at registration and the launch probe timing out, the swap arm fired and told the operator the CLI had changed and to restart -- when nothing had changed and restarting merely repeats the transient failure. A failed probe is now its own arm, classified BEFORE the swap comparison, with a retry remedy. Still fails closed; only the diagnosis and the remedy differ. P2 #3 -- the swap arm hid the out-of-range diagnosis. A CLI genuinely replaced with an out-of-range version was told only to restart the daemon, and restarting re-advertises the same out-of-range version, so the remedy could never work. The range is now checked before the swap arm, so the operator is told the thing they can act on. Final arm order: vendor -> connection -> policy -> probe-failed -> range -> swap. Mutation-verified rather than asserted: - remove the probe-failed arm -> 3 tests fail - move the swap arm before the range arm -> 1 test fails 12 tests, check-linear-ids clean.
realtonyyoung
added a commit
that referenced
this pull request
Jul 28, 2026
…ewer (#385) * fix: a transient version-probe timeout must not disable the reviewer Surfaced by three cross-driver conformance attempts that each failed differently. One defect explains all of them. ProbeCliVersion shelled out to `<cli> --version` with a 3s timeout, no retry, returning null on failure -- and that null was computed once at daemon registration and advertised as ExpectedCliVersion for the daemon's whole lifetime. `claude` is a Node CLI whose cold start routinely exceeds 3s on a loaded host. One transient failure then produced either: - the vendor dropping out of the unattended set entirely (reviewer_vendor_unavailable), because the server cannot range-check a null; - or EVERY launch being rejected, because the launch-time arm demanded exact equality between a freshly probed version and the advertised null. The second is worse than it sounds: it survives the remedy the error text gives. Restarting the daemon on a still-loaded host simply re-poisons the advertisement, which is exactly what happened between two of the attempts. Three changes: 1. The probe retries (3 attempts, 250ms/500ms backoff) with a 10s budget. A cold Node start under load is not an error condition. 2. The equality arm no longer treats "null advertised" as a mismatch. It exists to catch a CLI SWAP between advertisement and launch; null-vs-value is not a swap. A null or empty advertised version now falls through to the range check, which is the real gate -- proven by a test that an out-of-range CLI is still rejected when the advertisement is null. 3. The rejection NAMES the failing arm. Four distinguishable conditions collapsed into one message that reported the certification revision -- which matches on every one of these paths -- and told the operator to update a CLI that was correct and in range. This was already recommended when the same message was hit from a different arm (empty AllowedCliRanges), but that fix shipped server-side only and the daemon message never changed. Each arm now states the cause AND the remedy that applies to it, and a test asserts no rejection mentions the revision. The arm logic is extracted to an internal EvaluateReviewerCertification so it is testable at all -- it was previously a single inline boolean expression. 9 new tests, 29/29 in the neighbouring daemon suite, check-linear-ids clean. * fix: split the probe budget, and stop misreading a failed launch probe as a swap Codex review round 1 on this PR. Three findings, all real. P1 #1 -- the retry budget was right for registration and wrong on the launch path: three 10s waits plus backoff is ~30.75s of a thread-pool thread held before a launch can even be rejected, multiplied by concurrent launches. Registration keeps the three attempts (its result is cached for the daemon's lifetime, so a miss there is durable and destructive); the launch path now uses a single attempt. It does not need the retry: a miss there is no longer misclassified, produces a retryable rejection, and the caller can try again. P1 #2 -- the same bug I fixed at registration was still live at LAUNCH. With a version advertised at registration and the launch probe timing out, the swap arm fired and told the operator the CLI had changed and to restart -- when nothing had changed and restarting merely repeats the transient failure. A failed probe is now its own arm, classified BEFORE the swap comparison, with a retry remedy. Still fails closed; only the diagnosis and the remedy differ. P2 #3 -- the swap arm hid the out-of-range diagnosis. A CLI genuinely replaced with an out-of-range version was told only to restart the daemon, and restarting re-advertises the same out-of-range version, so the remedy could never work. The range is now checked before the swap arm, so the operator is told the thing they can act on. Final arm order: vendor -> connection -> policy -> probe-failed -> range -> swap. Mutation-verified rather than asserted: - remove the probe-failed arm -> 3 tests fail - move the swap arm before the range arm -> 1 test fails 12 tests, check-linear-ids clean. * fix: notify the caller before the self-heal, not after it Codex review round 2. My round-1 claim that the launch path was bounded to one 10s attempt was FALSE, and the reviewer was right to check the wiring rather than take it. The rejection branch called ComputeUnattendedVendorCapabilities BEFORE LaunchFailedAsync -- and that recompute re-runs the REGISTRATION probe, three 10s attempts plus 750ms of backoff. So a failed launch probe cost ~10s in the single-attempt probe, entered the new transient arm, and then blocked ~30.75s more before the caller heard anything. Splitting the probe budget did nothing while this sat in front of the notification. LaunchFailedAsync now goes first. The self-heal (recompute + re-advertise) still runs -- a certification mismatch usually does mean the advertisement is stale -- but off the response path and contained, so a throw there cannot surface as a second, different failure for a launch that has already been rejected. Not unit-tested, and saying so rather than implying otherwise: this is call-site wiring inside a large launch method, and the arm tests deliberately cover only EvaluateReviewerCertification. The reviewer made the same point. Verified by reading the ordering, not by a test. * fix: single-flight the background capability refresh Codex review round 3. My round-2 fire-and-forget REINTRODUCED the defect this PR removes, and the reviewer described the interleaving exactly: refresh A starts on a loaded host and spends ~30s timing out refresh B starts later, probes successfully, publishes, re-registers A completes LAST, overwrites the valid snapshot with its failed-probe null, and re-registers that poisoned advertisement Atomic reference assignment prevents a torn pointer, not stale completion order. I had reasoned about the former and asserted the latter was fine. SingleFlightRefresh serialises publication and coalesces: a request arriving mid-flight sets a rerun flag rather than starting its own pass, so a burst of rejections collapses to at most ONE extra pass, and that pass starts AFTER the request that asked for it -- so the last write is always the newest computation. The rerun flag is cleared BEFORE the work, not after, or a request arriving during a pass would be swallowed. Extracted as its own type for the same reason the certification arms were: the coordination is the risky part and it was untestable inline. 5 tests, including the reviewer's exact scenario (slow-failing overlapping fast-success) and a burst collapsing to one rerun. Mutation-verified: remove the gate so passes can run concurrently and ALL 5 fail. 12 certification-arm tests still green. * fix: Trigger must schedule, not run the delegate inline Codex review round 4. A DISCARDED task is not an asynchronous boundary -- `_ = RequestAsync(...)` still runs the method's synchronous prefix on the caller's stack, up to its first incomplete await. The refresh delegate computes capabilities SYNCHRONOUSLY (it shells out to probe CLI versions) before awaiting anything, and SemaphoreSlim.WaitAsync(0) completes synchronously when the gate is free -- so nothing yielded, and the launch path still ate the whole probe budget. My round-3 claim that the work was off the response path was wrong for the second time in this PR, in the same way: I asserted a boundary existed instead of checking where the first real await was. RequestAsync becomes a non-async void Trigger that wins the gate with the synchronous Wait(0) and hands the pass to Task.Run. The caller cannot inherit the work. That also removes the API asymmetry I raised last round rather than papering over it with a doc comment: there is no task to await, so a future caller cannot be misled by one whose meaning depends on whether they won the gate. An internal Current property exists purely so tests can await quiescence. New test: a delegate that BLOCKS SYNCHRONOUSLY must not delay Trigger. Mutation-verified -- replace Task.Run with an inline call and it fails. 6 single-flight tests, 12 certification-arm tests, check-linear-ids clean. * fix: serialize daemon registration so a stale snapshot cannot land last Codex review round 5. I argued this was a harmless duplicate registration and that reasoning was wrong -- checking the DTO settled it. DaemonConnectAsync reads _config.UnattendedVendorCapabilities INLINE at DTO construction, which happens AFTER an `await MergeRepoPathsAsync()` yield, and nothing serialized two concurrent registrations. So: heartbeat (slot displaced) constructs its DTO with the OLD capabilities the certification self-heal assigns the NEW capabilities and sends its own if the heartbeat's frame is processed last, the server ends up advertising the STALE set while the daemon's local config says otherwise That silently undoes the self-heal. It matters more now than it did before, because this change makes the self-heal the thing that restores a missing advertisement -- so its reliability is load-bearing rather than incidental. DaemonConnect construction AND invocation now run under a registration lock. Held across the invoke deliberately: releasing after construction would let a DTO built from fresher config overtake an in-flight older one, which is the same bug with a smaller window. The race predates this PR. I am fixing it here anyway because this PR is what makes the self-heal load-bearing, and because the fix is contained to one class rather than the cross-subsystem change I assumed it would be when I declined it last round. NOT unit-tested, stated plainly: exercising it needs a hub fake that can pause a registration mid-invoke, and there is no ServerConnection harness in this suite. Verified by reading the ordering. If a harness is wanted, it should be its own change rather than grown inside this one. 6 single-flight tests, 12 certification-arm tests, check-linear-ids clean. * fix: keep the launch-lane probe on its original short budget Qodo finding 4 caught a regression I introduced. The launch-time probe runs on the SEQUENCED COMMAND LANE -- a single serial consumer -- so every later launch and stop queues behind it. Reducing it to one attempt was right, but I also raised the per-attempt timeout from 3s to 10s for BOTH call sites, so the lane stall went from 3s to 10s. The change meant to shorten it tripled it. Registration keeps 3 x 10s (cached for the daemon's lifetime, so a miss there is durable). The launch path is back to a single 3s attempt: a miss there is now correctly classified as transient and retryable, so it never needed the bigger budget. Qodo's 61.5s figure assumed the capability recompute still ran inline on the launch path; it moved to a scheduled single-flight pass earlier in this PR, so the second probe no longer holds the lane at all. Findings 1-3 (comment length): trimmed the probe-budget, ExpectedCliVersion and test-class comments to the non-obvious invariant. Incident history belongs in the PR, which is where it now lives. 12 certification-arm tests, 6 single-flight tests, check-linear-ids clean.
realtonyyoung
added a commit
that referenced
this pull request
Jul 29, 2026
Qodo #1, partially taken. The comments explain why each assertion is shaped the way it is — which is load-bearing here, since three of this PR's review findings were tests that passed vacuously and the rationale is what stops the next reader simplifying them back. That stays, and it matches the surrounding code (SingleFlightRefresh, McpConfigShape, KcapMcpRegistry all document intent at length). What was genuinely historical rather than explanatory is gone: which review round found what, what my earlier attempts did, and past-tense accounts of defects that no longer exist. Rewritten as present-tense reasons. 42/42.
realtonyyoung
added a commit
that referenced
this pull request
Jul 29, 2026
…jection (#388) * test(mcp): pin the vendor-capable flows schema across every driver projection Reviewer choice is meant to be a property of the request, not of whichever harness is driving. Nothing enforced that: registration is FOUR mechanisms, not one. Six harnesses (Cursor, Copilot, Gemini, Kiro, OpenCode, Antigravity) converge on one JSON writer and differ only by a shape; Codex writes TOML through a separate engine with its own ownership ledger; Claude Code loads a hand-maintained static kcap/.mcp.json; and Pi gets a hard-coded server list inside an embedded TypeScript bridge. The existing per-harness tests each assert Contains("kcap-flows") in isolation — that a server by that name was written, not that it resolves to the same executable and therefore the same schema. A harness whose registration drifts to a different command leaves a caller believing it named a reviewer when it sent nothing. Adds a conformance suite covering: - vendor is an optional string on both START tools, and on neither of the six follow-up tools (the applied vendor is pinned at start; a vendor there would be ignored or an incoherent mid-run switch); - the vendor/model DESCRIPTIONS carry what the schema cannot — that omitting vendor takes the server default, that the token is canonical lowercase, that there is no silent fallback, and that model requires vendor. This is the only mechanism by which a driver LLM learns to pass the parameter, so a correct schema with a silent description produces exactly the failure the contract exists to prevent; - all nine driver projections resolve to the same `kcap mcp flows`, driven through the real writers rather than asserted against the descriptor; - Pi's bridge still lists flows in its literal — it discovers tools at runtime, so dropping it there is silent; - the two independent copies of the server list (KcapMcpServers vs KcapMcpRegistry) agree, and every canonical server resolves as an allowlist entry. Nothing kept these in sync. Also pins the hand-written harness table against VendorSelection.KnownVendorFlags (now internal), so a tenth installable target fails here instead of quietly being uncovered — no enumeration of supported harnesses exists in production code, the list is spread across four separate string arrays. Mutation-tested: dropping vendor from start_flow, drifting KcapMcpRegistry's args, removing flows from Pi's literal, adding a tenth vendor flag, and changing the canonical flows args each fail their assertion. The last one is instructive — all seven generated arms fail while the two static hand-maintained files pass, which is exactly the generated-vs-static drift this is for. Scratch dirs live under the assembly output, not the system temp root: on macOS /var is a symlink and CodexConfigToml's path guard rejects any symlinked component, so a temp-rooted Codex registration silently returns Failed. (That is also why the pre-existing CodexConfigTomlTests fail locally on macOS.) 26/26. Full Tests.Unit: 42 failures, all pre-existing on macOS and none in this suite. * fix(test): drive the real installers, not a reconstructed projection table Codex review round 1, and the P1 defeated the suite's central claim. The table hard-coded KcapMcpServers.ForCursor and the expected McpConfigShape, while the real choices are wired independently in SetupCommand and each PluginCommand installer. So changing a real arm to omit kcap-flows, use the wrong subset, or use the wrong shape left every projection test green — the test kept invoking its own correct reconstruction. The Codex arm had the same hole, calling RegisterKcapMcpServers directly rather than the install path. Each arm now runs PluginCommand.HandleAsync against a FakeUserHome, seeding the same installed-but-stale state its own PluginCommand*Tests use so `--if-installed` takes the refresh branch. Codex is flagged BareInstall — it installs unconditionally and needs a planted plugin root. Proof it now bites: dropping kcap-flows from the production ForCursor subset fails six arms, where before it failed none. Also from round 1: - argv comparisons were UNORDERED, so ["flows","mcp"] passed while launching nothing. Ordered in both the projection assertion and the two-list check. - the drift check only ran canonical -> registry, so a registry-only server stayed allowlistable-but-never-registered with everything green — one of the exact failure modes the suite claims to prevent. Now compares both name sets (KcapMcpRegistry.AllIds is new) and every server's args. - `model` was pinned by prose only, so retyping it to boolean or promoting it into Required — which would break every caller relying on the vendor's default model — still passed. Type and non-requiredness now pinned, mirroring vendor. The coverage tripwire also stops matching on a display name split on whitespace (which could match by accident) and matches on the flag each arm actually drives. Every fix mutation-tested: production dropping flows, a registry-only server, reversed argv order, and model promoted to Required each fail. 27/27. Full Tests.Unit unchanged at 42 pre-existing macOS failures, none mine. * fix: one definition of each harness's MCP projection, and real path containment Codex review round 2. P1 — the SetupCommand route was outside the gate. `kcap setup` builds its own six Register*Mcp delegates, duplicating the (subset, shape, marker) tuple that PluginCommand also spells out. Mutating SetupCommand.RegisterCopilotMcp to drop flows or use a divergent shape left every installer-driven arm green: a user could get a different tool surface depending on whether they ran `kcap plugin install` or `kcap setup`. Fixed structurally rather than by testing both routes. The tuple now lives once, in HarnessMcpProjections, and both call sites consume it — there is no longer a second definition to diverge. Dropping flows from the shared Copilot projection now fails 2 tests; giving it the wrong shape fails 1. P1 — path containment was broken in three of seven arms, and the test could therefore read and rewrite a developer's REAL harness config, or pass against a pre-existing entry. The Gemini arm cleared GEMINI_HOME, a name GeminiPaths does not read (it honours GEMINI_CLI_HOME); the Codex arm cleared nothing while CodexPaths still gives ambient CODEX_HOME precedence; OpenCode cleared OPENCODE_CONFIG_DIR but left its XDG_CONFIG_HOME fallback live. Every known override is now cleared for every arm — a per-arm list is exactly what was wrong, so there is no per-arm list. Codex also now passes --skip-codex-network-access; a schema test has no business rewriting profile network config. P2 — the status assertion was satisfied by the fallback. FormatStatusResponse catches formatter exceptions and returns the raw JSON body, so "contains claude" passed even if formatting failed entirely. Now asserts the rendered labels and that no raw JSON survives. Mutation-testing that fix found something else: the audit rendering is TRIPLICATED across FormatRoundResponse, FormatStatusResponse and FormatPolledRoundResult, and my first mutant hit the wrong copy. The polled path is the one an agent reads on nearly every flow, and it had no coverage at all — added, and both formatter mutants now fail. 34/34. Full Tests.Unit unchanged at 42 pre-existing macOS failures, none mine. * fix: route removal and ownership through the projection too Codex review round 3. I added HarnessMcpProjection.Unregister and then left it unused — the six PluginCommand remove paths still hard-coded shape and marker, and Kiro's "is the MCP half already installed?" probe constructed `new McpMarker("kiro")` directly. So the single-source claim was only half true: changing a projection made new installs write under one ownership tuple while uninstall looked under the old one (stranding owned entries kcap could no longer see) and Kiro's refresh read an existing install as absent. All six removals now go through the projection, and OwnsAnything moves the probe there for the same reason the marker name is derived rather than passed: a probe reading a different tuple than the writer is the same bug in a third place. `new McpMarker(` no longer appears in PluginCommand at all. Pinned by a per-harness register -> probe -> unregister round-trip asserting the config is left with no kcap entries. Mutation-tested by making Unregister use a different marker name: all six fail. 41/41. Full Tests.Unit failure set byte-identical to the 42-item pre-existing macOS baseline. * test(mcp): make deleting a bundled-config arm fail the coverage tripwire Qodo #3. The two bundled static configs were covered by an [Arguments] test the tripwire could not see, and both reduce to `--codex` / `--claude` there — so deleting the Codex-plugin arm left `--codex` green while one of two INDEPENDENT Codex registration mechanisms went untested. The bundled configs are now a list the coverage assertion can read, compared against what is actually shipped in kcap/. A third bundled config, or a deleted arm, fails. Mutation-tested by removing the .codex-mcp.json entry. 42/42. * docs(test): drop the review narrative from the conformance comments Qodo #1, partially taken. The comments explain why each assertion is shaped the way it is — which is load-bearing here, since three of this PR's review findings were tests that passed vacuously and the rationale is what stops the next reader simplifying them back. That stays, and it matches the surrounding code (SingleFlightRefresh, McpConfigShape, KcapMcpRegistry all document intent at length). What was genuinely historical rather than explanatory is gone: which review round found what, what my earlier attempts did, and past-tense accounts of defects that no longer exist. Rewritten as present-tense reasons. 42/42.
This was referenced Aug 7, 2026
realtonyyoung
added a commit
that referenced
this pull request
Aug 19, 2026
…reason (qodo) - FailEnvelopeSourcedLaunchAsync now SetAgentStatus(Failed) BEFORE CleanupAgentAsync disposes the runtime — disposal ends the read loop, whose FinalizeAgentRunAsync would otherwise classify by exit code and emit a Completed/Failed status that races and could MASK the coded launch failure. A pre-set Failed makes that classification a no-op (the same guard the launch-verdict path uses). qodo #3 (finalizer overwrites claim failure). - The rejected-claim reason is now vendor-neutral (no 'codex app-server:' prefix in shared orchestration code). qodo #1 (vendor/driver separation). - qodo #2 (first-turn leak) was already addressed in the prior commit (tear down on a first-turn dispatch failure). Tests 4/4 + 8 forwarding regression still green.
realtonyyoung
added a commit
that referenced
this pull request
Aug 19, 2026
…nce (#612) * Codex app-server: orchestrator deferred-first-turn source-claim sequence The daemon driver that wires the hosted-Codex source-claim handshake into the launch lifecycle. Dormant until the activation slice sets deferFirstTurn on the factory (the production factory is unchanged; the fake test runtime exercises the whole sequence). - Factor StartAcpForwardingAsync's post-bind body (liveness/TOCTOU guards, local binding registration, forwarder build+run) into a shared StartForwarderAfterBind, so the source-claim path reuses it WITHOUT re-binding (the claim supersedes AcpSessionStarted). The daemon forwarder ignores the claim's AcceptedSeq — resume is server-driven via the AcpBatchAck, exactly as before. - StartEnvelopeSourcedSessionAsync (fired fire-and-forget after RegisterAgentAsync for a runtime whose RequiresSourceClaimBeforeFirstTurn is true): AcpSessionSourceClaim → (Rejected/method-not-found: coded launch failure + teardown | Bound: start forwarder without re-binding → BeginFirstTurnAsync → confirm). Every step gated on the agent's AcpCts so a finalize aborts cleanly; contains all its own faults. - FailEnvelopeSourcedLaunchAsync = CleanupAgentAsync (single-flight: disposes the runtime, releases the slot, unregisters) + LaunchFailedAsync — the proven outer-catch composition, never FinalizeAgentRunAsync (which classifies a still-live process by exit code). - ConfirmSessionLaunchLoopAsync: background confirm, retrying only transient failures on a paced cadence for the agent's lifetime and NEVER tearing it down (Confirmed/AlreadyConfirmed done; Superseded/NotFound stop; a lost confirm leaves the row provisional for the server's own recovery to settle). - HandleLaunchAgent branches on RequiresSourceClaimBeforeFirstTurn; Cursor keeps the single-phase bind-then-forward path. Tests: AgentOrchestratorSourceClaimTests (3 — happy path claims-then-forwards-without-rebind- then-first-turns-then-confirms, Rejected teardown, method-not-found teardown), asserting through the deterministic AcpCallOrder/signal seams. The 8 existing forwarding tests still green (the factoring is behaviour-preserving). CaptureServerConnection/FakeAcpRuntime/ SpyAcpHostedAgentRuntimeFactory gain the source-claim/confirm/deferred-first-turn doubles. * Codex app-server: contain FailEnvelopeSourcedLaunchAsync teardown faults (Copilot review) It is awaited from the fire-and-forget source-claim task outside any try, so a teardown fault (a cleanup or a LaunchFailed hub error) would escape as an unobserved task exception. Wrap its body in try/catch + log; document the no-throw invariant. * Codex app-server: tear down on a first-turn dispatch failure (Kiro review) A BeginFirstTurnAsync failure after a successful claim previously only logged, leaving a zombie agent holding a slot while the server's Rule-2 expiry closed the provisional row. Tear the agent down instead (symmetric with the claim-failure path, immediate slot release); the provisional row is still closed by Rule 2. New test: first-turn failure → LaunchFailed + unregister + dispose, no confirm. * Codex app-server: mark agent Failed before teardown + vendor-neutral reason (qodo) - FailEnvelopeSourcedLaunchAsync now SetAgentStatus(Failed) BEFORE CleanupAgentAsync disposes the runtime — disposal ends the read loop, whose FinalizeAgentRunAsync would otherwise classify by exit code and emit a Completed/Failed status that races and could MASK the coded launch failure. A pre-set Failed makes that classification a no-op (the same guard the launch-verdict path uses). qodo #3 (finalizer overwrites claim failure). - The rejected-claim reason is now vendor-neutral (no 'codex app-server:' prefix in shared orchestration code). qodo #1 (vendor/driver separation). - qodo #2 (first-turn leak) was already addressed in the prior commit (tear down on a first-turn dispatch failure). Tests 4/4 + 8 forwarding regression still green.
realtonyyoung
added a commit
that referenced
this pull request
Aug 19, 2026
- Fix (qodo #4): the coordination-notices capability was injected into `body`, which the transient-POST-failure path also spools — so a replay would make the server mark notices delivered that the replay never renders. Inject into a separate POST-only `postBody`; the spool now keeps the capability-free `body`. New test pins that a spooled session-start body carries no coordination_notices while the live POST still advertises it. - Docs (qodo #1, #5): add a README SessionStart coordination-notices bullet (opt-out + capability-gating) next to the sibling injections, and list disable_coordination_notices in `kcap config set` usage. - Comments (qodo #2): trim narration in ProfileConfig / ClaudeHookCommand / CoordinationNoticesEmitter, keeping the non-obvious rationale. - qodo #3 (move emitter under Harness/Claude/): declined — every sibling SessionStart-fragment emitter lives at src/Capacitor.Cli/ root. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 19, 2026
…lane (#609) * [AI-2095] Render SessionStart coordination-notices terminal-delivery lane Client half of the kcap-server U3 coordination-notices lane (server shipped first, capability-gated and inert until this lands). On a live Claude/generic SessionStart the CLI now: - advertises the `coordination_notices: "v1"` capability on the POST body, only on the live path (injected after the ordering-guard/backlog return, so a spooled body never carries it; `kcap import` uses the vendor routes with origin=historical and never reaches here); - renders a returned `coordination_notices: [{text}]` list into the SessionStart additionalContext envelope, next to the team-memory index, via a new CoordinationNoticesEmitter (modelled on SessionGuidelinesEmitter); - honours a new `disable_coordination_notices` profile opt-out that mirrors `disable_memory_index` (Profile field, ConfigCommand set arm, help-config), read from the effective profile so it holds for KCAP_URL users too; when set, the capability is not sent at all (notices stay in the bell / Slack). Additive and fail-open throughout: any parse/build error leaves the hook unaffected. AOT-safe (JsonNode + the existing source-gen Profile context; no reflection). Unit + integration tests cover capability send/omit, render, opt-out suppression and malformed-field fail-open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [AI-2095] Address qodo review: capability-free replay spool + docs - Fix (qodo #4): the coordination-notices capability was injected into `body`, which the transient-POST-failure path also spools — so a replay would make the server mark notices delivered that the replay never renders. Inject into a separate POST-only `postBody`; the spool now keeps the capability-free `body`. New test pins that a spooled session-start body carries no coordination_notices while the live POST still advertises it. - Docs (qodo #1, #5): add a README SessionStart coordination-notices bullet (opt-out + capability-gating) next to the sibling injections, and list disable_coordination_notices in `kcap config set` usage. - Comments (qodo #2): trim narration in ProfileConfig / ClaudeHookCommand / CoordinationNoticesEmitter, keeping the non-obvious rationale. - qodo #3 (move emitter under Harness/Claude/): declined — every sibling SessionStart-fragment emitter lives at src/Capacitor.Cli/ root. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [AI-2095] Drop redundant null-forgiving in CoordinationNoticesEmitter (kiro nit) string.IsNullOrWhiteSpace is [NotNullWhen(false)], so after the guard the compiler already flows `text` as non-null — the `!` was redundant. Still builds warning-free without it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
…ness, catalog delegate - Offer ledger updates now serialize across processes via ConfigFileLock and write through a per-process-unique temp file, so a concurrent hook/setup/ command can no longer overwrite another's change (notably lose a dismissal). (qodo #4) - Load normalizes a null `vendors` member to an empty dictionary, honoring the corrupt-to-empty contract so management commands can't null-deref. (qodo #5) - Claude/Codex wired-checks moved into their own installers (ClaudePluginInstaller.IsPluginEnabled / CodexHooksInstaller.ReferencesKcapHook) and read via a new Core SharedFileText (FileShare.ReadWrite), so probing never blocks an agent writing its own settings.json/hooks.json on Windows. (qodo #2) - Wired-check folded into a per-entry delegate on HarnessCatalog (the single Core registration site); HarnessIntegrationProbe is now thin generic dispatch, no per-vendor switch in shared code. (qodo #1) - `harness` added to Program.cs offline-command allowlist, so `kcap harness list|dismiss|reset` works with no server configured. (qodo #7) - Trimmed predicate comments that restated the code. (qodo #3) - Added null-vendors ledger test. (qodo #6 — "setup stamping fails compilation" — is a false positive: `detected` is CodingAgentsStep.DetectedAgents whose members are bool, and CI's build passed.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
…osed throttle, honest dismiss, snapshot-consistent wiring - Offer-ledger Update takes a lock timeout; the hook/best-effort StampOffered path uses a short 1s wait so a contended ledger can't stall a SessionStart hook past a host's ~5s budget (management commands keep the 5s wait). (Codex P1 #1) - SharedFileText opens with FileShare.Delete too, so a concurrent read can't make the ledger's atomic File.Move-overwrite fail with a sharing violation on Windows (which would silently drop a dismissal). (Codex P1 #2) - TryClaimCheck now fails CLOSED: if the throttle stamp can't be written the same failure blocks the ledger stamp, so returning true would nudge on every hook — suppress instead. (Codex P1 #3) - Update returns whether the change persisted; `kcap harness dismiss|reset` surface a false as exit 1 instead of falsely reporting success (a lost dismissal would otherwise silently revive the nudge). (Codex P1 #4) - Kiro/Pi/OpenCode wired-probes now thread the injected KiroHome/PiAgentDir/ OpenCodeConfigDir overrides (new optional params on the *Paths helpers) so detection and wiring consume the same AgentDetectionInputs snapshot, per the Core API contract. (Codex P2 #5) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
…ck, pure snapshot wiring - On lock-acquire failure Update no longer does a lockless read-modify-write (which could recreate the lost-dismissal race); it returns false without mutating. Hook callers ignore it (best-effort); dismiss/reset already report false as exit 1. (Codex R2 P1 #1) - Hook-path stamping uses a zero-wait (non-blocking) lock try, so a SessionStart hook spends none of its exit-budget safety reserve waiting on the mutex; on contention it simply skips stamping. (Codex R2 P1 #2) - Kiro/Pi/OpenCode wired-probes resolve paths via new pure helpers (KcapAgentJsonPure / KcapExtensionPure / KcapPluginPure) built from the existing *Pure roots, so a null injected override means "unset → home default" and never re-reads ambient env — honoring AgentDetectionInputs' null-as-unset contract and keeping detection and wiring on the same snapshot. Reverted the round-1 non-pure override params (superseded). Added a Kiro override-threading test. (Codex R2 P2 #3) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
- StampOffered takes a lockTimeout; the hook path passes TimeSpan.Zero while setup omits it and gets the normal serialized 5s wait, so a momentary concurrent writer no longer makes setup silently skip stamping the offered vendors (which would drop the 7-day floor). (Codex R3 P2 #1) - Copilot/Gemini/Antigravity wired-probes now use pure path helpers (KcapHooksJsonPure / SettingsJsonPure / GlobalHooksJsonPure) built from the existing *Pure roots, so every override the snapshot carries is resolved without an ambient env re-read. Claude/Codex necessarily read CLAUDE_CONFIG_DIR/CODEX_HOME from ambient because those roots aren't part of AgentDetectionInputs (detection is PATH-only for them) — documented as the sole, production-coincident exception on HarnessCatalog. (Codex R3 P2 #2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
* Spec: new-harness detection and setup nudges (#625) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Spec: address Copilot spec-review round 1 findings (#625) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Spec: address Copilot spec-review round 2 findings (#625) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Spec: address Kiro spec-review round 1 findings (#625) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Spec: post-signoff consistency pass, mark reviewed (#625) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * New-harness detection and setup nudges (surfaces 1–2) (#625) Detects, after initial setup, that a supported coding harness is installed but kcap is not wired into it, and prompts to set it up. Implements the spec's PR 1 (independently shippable client surfaces); surfaces 3–4 (daemon/ingest inventory → server notification, and the server-side machine-went-quiet backstop) follow in PRs 2–3. Core substrate: - HarnessCatalog: single Core table binding vendor id + label + install flag + detection selector; the App's AgentVendors re-derives from it, and a conformance test pins it against VendorSelection.KnownVendorFlags. - HarnessIntegrationProbe.IsWired: "is kcap wired into vendor X?", the single source of truth now shared by the kcap status Hooks line (Claude's enabled-plugin and Codex's hooks-reference checks moved to Core; status delegates to them, behavior-preserving). - HarnessOfferLedger/Store: per-machine harness-offers-v1.json (atomic write, corrupt→empty) plus the shared 6h evaluation throttle stamp. - HarnessNudge.Nudgeable: pure predicate (detected ∧ !wired ∧ !declined ∧ past the 7-day re-offer floor). Surfaces: - 1: HarnessNudgeEmitter fragment wired into all 9 hook commands' SessionStart additionalContext, next to the work-items nudge. - 2: HarnessSetupNotice — exit-time interactive stderr notice (TTY-gated, human-facing commands only), mirroring the update-available notice and sharing surface 1's throttle. - kcap status: passive "installed but kcap not configured" line (ledger- independent — always tells the truth). - kcap harness list|dismiss|reset: inspect/silence/re-enable the nudges; all bypass the throttle. - kcap setup stamps offered harnesses so it never re-nudges a vendor just seen at setup; never writes/overwrites a dismissal. - Profile.DisableHarnessNudge (config key disable_harness_nudge) opts out surfaces 1–2 entirely. Tests: catalog conformance, predicate per-vendor, ledger/store (incl. dismissal-preservation), IsWired, emitter (fragment/notice/throttle/fold/ opt-out/exception→null). README + help + config help updated. Closes #625 AI-2118 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address qodo review: locking, shared reads, null-vendors, offline harness, catalog delegate - Offer ledger updates now serialize across processes via ConfigFileLock and write through a per-process-unique temp file, so a concurrent hook/setup/ command can no longer overwrite another's change (notably lose a dismissal). (qodo #4) - Load normalizes a null `vendors` member to an empty dictionary, honoring the corrupt-to-empty contract so management commands can't null-deref. (qodo #5) - Claude/Codex wired-checks moved into their own installers (ClaudePluginInstaller.IsPluginEnabled / CodexHooksInstaller.ReferencesKcapHook) and read via a new Core SharedFileText (FileShare.ReadWrite), so probing never blocks an agent writing its own settings.json/hooks.json on Windows. (qodo #2) - Wired-check folded into a per-entry delegate on HarnessCatalog (the single Core registration site); HarnessIntegrationProbe is now thin generic dispatch, no per-vendor switch in shared code. (qodo #1) - `harness` added to Program.cs offline-command allowlist, so `kcap harness list|dismiss|reset` works with no server configured. (qodo #7) - Trimmed predicate comments that restated the code. (qodo #3) - Added null-vendors ledger test. (qodo #6 — "setup stamping fails compilation" — is a false positive: `detected` is CodingAgentsStep.DetectedAgents whose members are bool, and CI's build passed.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address Codex review: hook-path lock timeout, delete-sharing, fail-closed throttle, honest dismiss, snapshot-consistent wiring - Offer-ledger Update takes a lock timeout; the hook/best-effort StampOffered path uses a short 1s wait so a contended ledger can't stall a SessionStart hook past a host's ~5s budget (management commands keep the 5s wait). (Codex P1 #1) - SharedFileText opens with FileShare.Delete too, so a concurrent read can't make the ledger's atomic File.Move-overwrite fail with a sharing violation on Windows (which would silently drop a dismissal). (Codex P1 #2) - TryClaimCheck now fails CLOSED: if the throttle stamp can't be written the same failure blocks the ledger stamp, so returning true would nudge on every hook — suppress instead. (Codex P1 #3) - Update returns whether the change persisted; `kcap harness dismiss|reset` surface a false as exit 1 instead of falsely reporting success (a lost dismissal would otherwise silently revive the nudge). (Codex P1 #4) - Kiro/Pi/OpenCode wired-probes now thread the injected KiroHome/PiAgentDir/ OpenCodeConfigDir overrides (new optional params on the *Paths helpers) so detection and wiring consume the same AgentDetectionInputs snapshot, per the Core API contract. (Codex P2 #5) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address Codex review round 2: no lockless fallback, zero-wait hook lock, pure snapshot wiring - On lock-acquire failure Update no longer does a lockless read-modify-write (which could recreate the lost-dismissal race); it returns false without mutating. Hook callers ignore it (best-effort); dismiss/reset already report false as exit 1. (Codex R2 P1 #1) - Hook-path stamping uses a zero-wait (non-blocking) lock try, so a SessionStart hook spends none of its exit-budget safety reserve waiting on the mutex; on contention it simply skips stamping. (Codex R2 P1 #2) - Kiro/Pi/OpenCode wired-probes resolve paths via new pure helpers (KcapAgentJsonPure / KcapExtensionPure / KcapPluginPure) built from the existing *Pure roots, so a null injected override means "unset → home default" and never re-reads ambient env — honoring AgentDetectionInputs' null-as-unset contract and keeping detection and wiring on the same snapshot. Reverted the round-1 non-pure override params (superseded). Added a Kiro override-threading test. (Codex R2 P2 #3) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address Codex review round 3: setup stamping wait, full snapshot purity - StampOffered takes a lockTimeout; the hook path passes TimeSpan.Zero while setup omits it and gets the normal serialized 5s wait, so a momentary concurrent writer no longer makes setup silently skip stamping the offered vendors (which would drop the 7-day floor). (Codex R3 P2 #1) - Copilot/Gemini/Antigravity wired-probes now use pure path helpers (KcapHooksJsonPure / SettingsJsonPure / GlobalHooksJsonPure) built from the existing *Pure roots, so every override the snapshot carries is resolved without an ambient env re-read. Claude/Codex necessarily read CLAUDE_CONFIG_DIR/CODEX_HOME from ambient because those roots aren't part of AgentDetectionInputs (detection is PATH-only for them) — documented as the sole, production-coincident exception on HarnessCatalog. (Codex R3 P2 #2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Fix Antigravity pure hooks path: GUI config root, not data root (#626) GlobalHooksJsonPure built from RootPure (<gemini>/antigravity, the data root), but production GlobalHooksJson resolves through GuiConfigRoot (<gemini>/config), so a normally-wired Antigravity install was probed at the wrong path and reported unwired — spurious nudges while `kcap status` said configured. Add GuiConfigRootPure (<gemini>/config) and build the pure hooks path from it, mirroring production. Add HarnessWiredPathParityTests pinning every pure wiring path to its production layout so a wrong root/segment fails a test. (Codex R4 P1) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
…Claude spool path - RefreshHarnessInventoryIfStale now runs check-evaluate-publish under one lock (single-flight): overlapping report calls can't each probe, and a slower probe can't overwrite a newer one and reset the TTL to stale content. Eval is a few dir/PATH stats + a small JSON read, so holding the gate across it is cheap; a probe failure no longer advances the timestamp (retries next report). (Codex P2 #1) - Claude's degraded arm spools the session-start body via NormalizeForSpool and bypasses HandleCore's stamp, so a replayed session-start never carried the inventory. Stamp session-start bodies in NormalizeForSpool too, so the hook-ingest carrier works even for auth/URL-outage replays. (Codex P2 #2) AI-2118
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
…bose docs - RefreshHarnessInventoryIfStale now advances the timestamp on failure too (and logs at Debug), so a persistently-failing environment backs off to the 6h TTL instead of re-probing on every 60s send. (qodo #2) - Trimmed the HarnessInventory / SessionStartInventory / status-report-cache doc comments to the load-bearing "why" per the repo's comment guideline. (qodo #1) AI-2118
realtonyyoung
added a commit
that referenced
this pull request
Aug 20, 2026
* Surface 3 (client): report harness inventory to the server
Attaches a per-machine coding-agent inventory to the two client→server carriers
so the server can raise the "installed but not configured" notification and (PR 3)
the machine-went-quiet backstop. This is PR 2 of the new-harness-detection spec;
PR 3 (kcap-server) consumes it.
- Core: HarnessInventory { machine_id, vendors: { <id>: { detected, wired } },
declined[] } + a pure Evaluate (shared by both carriers), registered in
CapacitorJsonContext. machine_id is MachineId.Get() — the same id the daemon
sends on DaemonConnect — so the server correlates a machine's connect record,
status-report inventory, and hook-ingest inventory.
- Daemon: additive nullable field on DaemonStatusReport; AgentOrchestrator caches
the inventory and recomputes on a 6h in-memory cadence (never claims the on-disk
nudge throttle stamp, so surfaces 1–2 aren't starved). BuildStatusReport stays
pure (reads the cache); the send path refreshes.
- Hook ingest: SessionStartInventory.Stamp serializes the same HarnessInventory
(via the same context → byte-identical wire shape) onto each vendor's SessionStart
body; wired into all 9 hook commands (session-start only for Claude/Cursor).
- Additive/down-level-safe: an old server ignores the field; an old client never
sends it.
Tests: Core Evaluate (mapping + declined); daemon status report carries the
inventory; hook session-start body carries it (integration, CI-gated). AOT clean.
AI-2118
* Address Codex review on #632: single-flight inventory refresh, stamp Claude spool path
- RefreshHarnessInventoryIfStale now runs check-evaluate-publish under one lock
(single-flight): overlapping report calls can't each probe, and a slower probe
can't overwrite a newer one and reset the TTL to stale content. Eval is a few
dir/PATH stats + a small JSON read, so holding the gate across it is cheap; a
probe failure no longer advances the timestamp (retries next report). (Codex P2 #1)
- Claude's degraded arm spools the session-start body via NormalizeForSpool and
bypasses HandleCore's stamp, so a replayed session-start never carried the
inventory. Stamp session-start bodies in NormalizeForSpool too, so the hook-ingest
carrier works even for auth/URL-outage replays. (Codex P2 #2)
AI-2118
* Address qodo on #632: TTL backoff on inventory-eval failure, trim verbose docs
- RefreshHarnessInventoryIfStale now advances the timestamp on failure too (and logs
at Debug), so a persistently-failing environment backs off to the 6h TTL instead of
re-probing on every 60s send. (qodo #2)
- Trimmed the HarnessInventory / SessionStartInventory / status-report-cache doc
comments to the load-bearing "why" per the repo's comment guideline. (qodo #1)
AI-2118
realtonyyoung
added a commit
that referenced
this pull request
Aug 22, 2026
qodo #1 (High) on PR #648: both new transcript reads (the watcher's one-shot prefix scan, and ImportCommand's path-based evidence overload) used File.ReadLines, which opens FileShare.Read — mandatory-exclusive on Windows, so it can deny the write handle the agent itself still holds on that same transcript mid-flush. Added WatchCommand.ReadLinesShared: a line-yielding sibling of the existing ReadAllTextShared/ReadAllTextSharedAsync helpers (same FileShare.ReadWrite, same rationale, doc-commented in place already), streamed rather than materialized so a scan that attributes early doesn't pay for the whole file. Both call sites now use it; ImportCommand reuses it directly since it's in the same project/namespace.
realtonyyoung
added a commit
that referenced
this pull request
Aug 22, 2026
…utside a repo (#648) * feat: evidence scanner deriving a git root from tool-input paths * feat: watcher evidence-based repo detection for outside-repo sessions * fix: genericize evidence scanner on completeness + deliver read fallback via final-drain batch Spec review findings on the watcher-integration work: - Scanner latched Done the moment FindRoot found a .git dir, before checking whether the root resolved to a usable owner/repo. A no-remote local repo edited first permanently blocked a later real GitHub repo from attributing. RepoEvidenceScanner is now generic over TRepo with an injected async resolver + completeness predicate; a root is only latched once resolved AND complete, and each distinct root is resolved at most once (cached). - The read-fallback promotion for a clean session end (StopWatcher, no parent-exit) was applied after the final drain's batch had already been sent, so a correctly-computed fallback was never transmitted. Promotion now happens inside DrainNewLines's isFinalDrain branch, before repoToSend is computed, so it rides the same live-hub batch that goes out on every exit that runs a final drain. * feat: import applies evidence-based repo detection to outside-repo transcripts * test: guard slot-B fallback delivery on the final drain Part A whole-branch review, item 1: the final-drain promote-and-deliver path (Finding 1's fix) was verified only by inspection. Extract it into WatchCommand.ApplyEvidenceScanAsync — a small seam over WatchState + the generic scanner, no HubConnection needed — and unit-test with a fake scanner that a read-only line's fallback lands on state.Repository when isFinalDrain is true, and stays untouched when it's false. * fix: catch File.ReadLines' eager path validation in the import evidence scan Part A whole-branch review, item 2: File.ReadLines(session.FilePath) was passed as a call argument to TryBuildEvidenceRepositoryNodeAsync, i.e. evaluated outside that helper's own try/catch. File.ReadLines validates its path argument eagerly (confirmed: an empty path throws ArgumentException synchronously, before the lazy file read), so an invalid path would mark an otherwise-importable session Errored instead of importing without evidence. Add a path-based overload that reads inside its own try and delegates to the existing line-based one; the call site now passes session.FilePath directly instead of pre-evaluating File.ReadLines. * fix: make the evidence scanner OS-independent; read top-level content too Windows CI + qodo review on PR #648: - qodo #3 / Windows CI failure: SafeDirectory used Path.GetDirectoryName, which rewrites/misreads the OTHER OS's separator style (a Unix path run through Windows' Path becomes garbage, and vice versa) instead of the transcript-producing OS's own convention. Replaced with a lexical last-separator split. ExtractClaudePaths' path.StartsWith('/') gate likewise dropped every Windows-absolute path; replaced with IsLexicallyAbsolute, mirroring RepoAttributionMatcher.IsLexicallyAbsolute from the server repo (reimplemented locally, no dependency taken) — Unix-rooted, Windows drive-rooted, or UNC, purely lexical either way. - qodo #2: a tool_use block can sit at an assistant event's top-level content, not only nested under message.content; ExtractClaudePaths now checks both. * fix: read transcripts with FileShare.ReadWrite, not File.ReadLines qodo #1 (High) on PR #648: both new transcript reads (the watcher's one-shot prefix scan, and ImportCommand's path-based evidence overload) used File.ReadLines, which opens FileShare.Read — mandatory-exclusive on Windows, so it can deny the write handle the agent itself still holds on that same transcript mid-flush. Added WatchCommand.ReadLinesShared: a line-yielding sibling of the existing ReadAllTextShared/ReadAllTextSharedAsync helpers (same FileShare.ReadWrite, same rationale, doc-commented in place already), streamed rather than materialized so a scan that attributes early doesn't pay for the whole file. Both call sites now use it; ImportCommand reuses it directly since it's in the same project/namespace.
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.
Summary
kapacitor setupstep 4 now writes plugin registration directly into Claude Code'ssettings.jsoninstead of printing a/plugin installcommand for the user to copy-paste--plugin-scope user|project|skipfor--no-promptmodeTest plan
InstallPlugin(new file, preserve existing, update path, create dirs, malformed JSON)kapacitor setupwith option 1 (user-wide) — verify~/.claude/settings.jsonupdatedkapacitor setupwith option 2 (project) — verify.claude/settings.local.jsoncreatedkapacitor setupwith option 3 (skip) — verify fallback message shownkapacitor setup --no-prompt --plugin-scope user— verify non-interactive mode🤖 Generated with Claude Code