Skip to content

feat(secrets): relay destination-owned webhook requests - #583

Closed
lukemaj wants to merge 6 commits into
feat/379-cross-machine-parityfrom
feat/385-remote-secret-channel
Closed

lukemaj wants to merge 6 commits into
feat/379-cross-machine-parityfrom
feat/385-remote-secret-channel

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What: PR #583 preserves privately saved destination-owned webhook secrets through timeout, transient validation and cleanup recovery.
Why: Losing cancellation and destructive/parked recovery could discard or retain protected copies incorrectly on idle B.
So what: Parent freezes this exact component for both fresh whole reviews, completes CI/client/cross-machine acceptance and controls integration composition and all merges.

Refs #385, #379. Approved design and mandatory R1-R5 amendment. Canonical Issue handoff retains active acceptance/native edges.

A private human answer entered through A becomes an immutable protected B value. The agent receives a destination-qualified non-bearer request handle and can create one signed webhook on idle B with no B agent. The negotiated typed route and existing authority owners enforce the destination, original run/project/family/generation and verified human actor.

Binding Value
Head 28338d0f9f355657cf0fa2d66a909e0b986cac90
Tree 27c9b2ef83c554b305787083b2e5cdc7a1ef1907
Frozen source base de50f47a47a87310649e058da0467badc1d48938
Base tree a32295120382ba5d8a8c2f3e052476ad2b07cae9
Checkpoint checkpoint/583-before-r1-cbd72 = cbd72f9cdb8066e0c4c6e73b41a031b681c5cde6
Full diff SHA-256, de50..HEAD ba590c658fd898df3e215a3de2b5d149a05ed6d9571724cc809d6737626ae7c6
Incremental diff SHA-256, cbd72..HEAD 969a2d0d686cc911c0e0c620df9e353852dcfbd22c6f26266aa2d46310ddff53
Final proof index SHA-256 827262e63e8233cccbe230c56c3c6ce23b0e77aaf9a03f5cd4ca4e2566039c88

Review finding dispositions

Finding Disposition and executed evidence
Sol F1, timeout/save winner Repaired only in requestSecret cancellation: normal timeout rereads the committed card before retirement; a winning save returns its handle and consumes once. Aborted/ended calls still retire even after a winning save because no caller remains. TestClock controls the final pending poll and timeout; native A private WebSocket answer commits before timeout. Ordinary timeout and aborted-call controls pass.
Sol F2 + Opus F6, terminal cleanup/availability The existing loop retries a failed cleanup batch before parking; failed remove also wakes this owner. Only filesystem cleanup leaves are deferred and logged per row, retaining terminal row/attempt/file ownership. Healthy rows continue. Startup schema, SQL transition, decoding and authority errors are outside that boundary. The ORIGINAL running B owner retries after a transient remove fault clears, with no reconstruction, manual drain, new request or unrelated wake. Separate startup reconstruction builds the protected service despite one bad deletion, cleans the other row, denies consumed material through the fresh owner and keeps native B HTTP available. A startup SQL-transition fault still fails construction.
Opus F2, transient secret-origin frames Added secret-origin to BOTH the ask finalizer and startup sweep. Actual success, TestClock timeout, cancellation, abandoned-frame startup and 33 consecutive cancelled asks pass; no unanswered-frame budget is leaked. Startup uses a copy of an actual in-flight frame to model process loss, not an OS kill.
Opus F3, destructive transient validation eligible retires/deletes only capability_denied or local expiry; typed unavailability propagates without changing saved/reserved state or protected bytes. Actual held control replies produce the real B ask timeout and orchestration_error; complete row/consumer/attempt and file SHA remain unchanged, then the same original attempt and consumer identity succeed once. Existing definitive run/grant denial races still retire. No admission result is mocked and SQL corruption is not the transient discriminator.
Opus F1 + Sol proof gap, active hook failure The cbd72 case is strengthened, not inherited as approved. Actual hook SQL failure logs a sanitized warning, delivers the real parent question, leaves the complete obligation unchanged with pending=0/no ACK, denies stale consumption and preserves the protected-file hash. Normal source recovery is reconstructed/started WHILE the SQL fault still exists; its existing retry timer is observed. After fault removal the same automatic recovery loop verifies the exact B ACK without a manual drain. This is direct actual reactor handling plus startup/recovery proof, not proof of live client registration or an installed harness.
Opus F4, global network semaphore Deferred per explicit parent disposition. No lock/concurrency redesign or measurement claim.
Opus F5, history wakes/scans Deferred per explicit parent disposition. No history pruning, new table or performance harness; only necessary failed-cleanup wake/retry behavior changes.
Optional card ID/guidance/span/placement/status observations No edits per parent. The question reactor's original pending/question logic and assertions remain unchanged.
Both old reviews' incomplete full-file coverage No approval inferred. Parent owns fresh Sol MEDIUM and Opus 5.5 MEDIUM whole-candidate reviews of all 36 files, current CI, actual clients/cross-machine acceptance, controlled composition and all merges.

Exact-head proof

174/174 focused tests pass, ten full files, no skipped tests, including both unchanged catch-up files 34/34 and all 33 native secret cases. Five direct cache-free scoped compilers and all scoped lint/format/exports/fork/registry/whitespace checks pass. All twelve observations bind exact committed head/tree/base, argv/cwd/start/end/exit, clean before/after state, complete tracked inputs and installed runtime hashes. Only generated Vitest results.json changes; raw final logs contain no fixture values and captured commands have exited.

Final immutable-production red is 10 failed/23 passed; repair green is 33/33 with identical argv/test/fixture bytes and pinned Node. The idle red never reaches deletion completion and hits the unchanged test safety timeout; green awaits the actual removal milestone after driving the production timer. The compiler supervisor's exit 143 is retained separately; three unfinished scopes then passed independently without source changes.

Snapshot 20/10/2026, 16:09:36 Europe/Warsaw, exact head 28338d0: successful Changes, Fork Stack Model, Native fingerprint diff, Label PR size, Collect PR targets, Lint, Label PR 583, Build and Typecheck, Test Server 1, Release Smoke. Pending: Test, Test Web, Test Server 2, Test Server 3, Test Server 4, Test Server 5, Test Server 6, Test Server 7, Test Server 8. Existing preview/Rust/mobile-native jobs are skipped in this workflow. No complete CI or integration gate is claimed. Current CI evidence: Fork Stack Model, Lint, Build and Typecheck, Test.

Limits and next owner

Native proof uses real isolated SQLite, HTTP/OAuth, authenticated private A WebSocket ingress, PeerAuthority and ServerSecretStore. Fixture catalog/provider entries are not an actual installed harness or second machine. Abort is an injected call interruption at the actual service; frame startup is a captured in-flight row restored as an abandoned row; storage/SQL interruption uses deterministic faults and service reconstruction, not an OS process kill. Startup availability proves construction of the protected layer used by the server and B native HTTP availability, not a full production server launch. Transient classification is observed at the actual B validator; the dedicated control transport keeps its existing external error mapping.

B scheduled_tasks.webhook_secret is separately authorized protected storage. Cards, results, receipts, transcripts and named forbidden DB/WAL owners exclude values/raw refs; no false whole-B-database absence claim is made. One-element remote fields map to one webhook value; broader fields remain a #379/#386 catalog obligation. Local fields/files/ref and unknown-ref semantics are unchanged and affected positives pass. No real human secret/account/grant/live userdata, live provider, browser/device/devserver, persistent automation, deployment, global tooling/shared runtime write or version bump occurred.

The source branch stays on exact de50 while integration independently advanced to 02343e5 through parent-owned #582. No fetch/merge/rebase/retarget or source absorption occurred. #386 completion/result/group regions and #531 pending/question ownership are untouched. The component is one open PR, not acceptance or merge clearance. #369/#531, #384/#555 and parent #379/#386 native edges remain open for final delivery.

Current canonical private handoff and both response/review packets: /home/luke/.local/state/chromeria-379-proof/385-implementation/r1-repair/report.md, sol-review-packet.md and opus55-review-packet.md. Exact bindings/raw logs: r1-repair/verification-index.json. All earlier c9/cbd indexes/manifests/logs remain unchanged; 72 retained log/manifest hashes and three initial identities match. The root historical index is intentionally preserved; the current index is under r1-repair. Workspace/branch/checkpoint remain for parent review and eventual parent-owned retirement.

Elon record

Requirements and who asked: User-approved #379/#385 R1-R5 and parent-accepted Sol F1/F2 plus Opus F1/F2/F3/F6 require private A capture, B one-use ownership, safe cancellation, durable retirement and recoverable acceptance.
Deleted: Optional unmeasured semaphore/history redesign, new tables/frameworks/locks, generic raw refs/paths, synthetic B agents, saved replacement and unrelated topology/version/deployment work.
Bottleneck: Faithful native lifecycle proof where timeouts, unavailable validation and cleanup meet durable destination state.
Checked myself: Both complete reports/addendum, real owner/caller/state paths, immutable-production red versus identical-test green, all 174 final-head cases, five compilers, full scoped static checks, preserved old proof hashes and actual current CI/Git state.

Implemented by GPT-6.1 Sol, MEDIUM effort, Codex harness.

@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 20, 2026
@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Initial-head CI has a second failure in the existing delegated-question catch-up test.
Why: On 10cd075, Orchestrator.control-reads.test.ts:835 expected two notification messages and observed one; the cause is not yet established.
So what: The #385 author has the exact failure and will reproduce and diagnose it before final readiness, alongside the already identified unused-export lint repair.

Failed job: https://github.com/toolboxmd/chromeria/actions/runs/39422982279/job/118628440145 . Test: wakes the parent once when its delegated task asks a question; shard summary: 1 failed, 1140 passed, 21 skipped. Parent read the actual failed log, the assertion's source context, and the complete component Orchestrator delta, which only forwards optional secret destination metadata. This does not establish whether the failure is inherited, timing-dependent or introduced. No flake classification, expectation relaxation or clearance is claimed.

Requirements and who asked: The user requires green current CI and complete component proof before integration.
Deleted: Treating a green focused retry as a cause diagnosis, and changing test counts or timing to hide the failure.
Bottleneck: Determining why the persisted catch-up notice is absent at this assertion.
Checked myself: Exact failed job output and source assertion, plus the component's Orchestrator diff.

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 5.0 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.8 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.9 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 5.0 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.8 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 21.2 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: unavailable · PR result: 28338d0 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj
lukemaj marked this pull request as ready for review October 20, 2026 12:59
@lukemaj
lukemaj marked this pull request as draft October 20, 2026 13:04
@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Sol requests changes on reviewed c9e93 for two lifecycle defects; this is not complete whole-file clearance.
Why: A timeout cancellation can retire a saved winner, and transient terminal-copy deletion can park without retry.
So what: Parent is freezing the newer proof-only checkpoint and completing the review frontier before author repair; no component clearance is granted.

Parent adjudication: source inspection confirms F1 through closeCard/answered-card no-op/relay retirement and F2 through terminal deletion plus active-only deadline parking. Both require deterministic reproductions and bounded owner fixes. The explicit active-obligation failure/retry proof gap is retained; a newer test-only checkpoint is not yet assessed. Sol examined every changed hunk but reports incomplete requested whole-file coverage, which must be completed in the next exact-candidate round. The worktree moved during review to cbd72; no c9 result transfers to that head. Full report follows unchanged.

PR #583 independent code review, round 1

Verdict: REQUEST_CHANGES. Two source-supported lifecycle defects are described below. This report is not a completed all-lines whole-file clearance: every changed diff was examined, but the full-file coverage requested for all 36 files was not completed. The coverage ledger explicitly distinguishes complete file reads from scoped reads; the parent must not count this as a completed clean Sol whole-candidate round.

Reviewer: GPT-6.1 Sol, MEDIUM, Codex. Independent of the implementation writer. Date: 2026-10-20, Europe/Warsaw. No test, install, provider, dev server, browser, device or live-data operation was performed. Findings below are source-derived mechanisms, not executed reproductions.

Exact candidate and read boundary

  • PR: feat(secrets): relay destination-owned webhook requests #583
  • Read-only worktree: /home/luke/.t3/worktrees/chromeria/feat-385-remote-secret-channel
  • Branch: feat/385-remote-secret-channel
  • HEAD at entry and last source/diff identity check: c9e93fbaf996c4b5443aa94ad2d2edc0786e8da3
  • Tree at entry and last source/diff identity check: 8140a5eed7ae6d272943832180092795515959cb
  • Base: de50f47a47a87310649e058da0467badc1d48938, tree a32295120382ba5d8a8c2f3e052476ad2b07cae9
  • Full committed diff: 36 files, +3135/-65. git diff de50f47 HEAD | sha256sum returned a3b4f00347e89b649cfb68b235173ca46fcd72e628297227535580cd35113504; repeated against explicit base/head with the same result.
  • Initial status was already dirty: M apps/server/src/peer/PeerAuthority.remoteSecrets.workflow.test.ts. The last source/diff identity check still named that file and git diff --numstat reported 150 added lines. Initial dirty bytes were not hashed, so byte-level equality of that user-owned change is not claimed. Candidate source inspection used committed git show/git diff objects, not the modified test contents.
  • Final verification detected an externally moving worktree: HEAD cbd72f9cdb8066e0c4c6e73b41a031b681c5cde6, tree 2a1b36d00fdc34f8204eed7a6ac9c4bf147f40c1, clean status. No further source inspection or mutation was performed after that detection. This report reviews only the assigned c9e93 candidate, and cannot clear or establish the test coverage of the new commit. The claims about missing committed proof refer exclusively to c9e93, not any later test additions.
  • Exclusive plain mkdir of the assigned sol directory succeeded, after creating the named parent. Report and proof-binding summary are the only filesystem writes by this review. No source, index, ref, checkout, worktree topology or forge mutation occurred. Required T3 PR-panel linking was performed and verified; it is app metadata, not a GitHub review/status/comment.

Required fixes

F1. P2: a save that wins just before timeout is nevertheless retired

Location: apps/server/src/mcp/OrchestratorMcpService.ts:2717, particularly the unconditional retirement following record("cancelled"); supporting owners are Orchestrator.ts:9045 and RemoteSecretRelay.ts:225-235.

Reachable trigger: the source wait has read a pending card and entered its 500 ms sleep (OrchestratorMcpService.ts:165,2755). The authenticated human answer then successfully saves on B and records secretStatus: "saved" on A, before the tool's timeout fires but before its next projection read. This uses the normal private-client answer path, without a forged actor, changed grant or failed storage.

Mechanism: timeout returns Option.none, so line 2765 calls closeCard. The orchestrator intentionally preserves an already answered card: its unchanged branch keeps an existing non-pending secret request, so record("cancelled") succeeds as a no-op on the saved card. The new Effect.andThen nevertheless calls relay.retire, which sets the exact obligation's pending=1. The fallback projection read at lines 2768-2779 correctly sees the winning saved answer and selects status="saved", but line 2787 then calls relay.handle, which rejects every obligation with pending !== 0.

Effect: the client successfully saved and continues displaying a saved card, while the agent receives capability_denied rather than its saved handle; the retirement drain also deletes B's accepted material. The same key cannot recover the answer because the obligation has been retired. This contradicts the function's existing reconciliation of a saved answer winning the timeout boundary and unnecessarily discards valid human input.

Correction required: base retirement on the committed outcome of the card transition, so a saved winner is not retired by a losing timeout cancellation. Preserve a coherent saved-result or cancelled-result outcome through the public request/answer path. Prove the boundary with deterministic milestones around the pending read, committed human save and timeout, rather than a wall-clock sleep. No change to the approved dedicated-channel architecture is needed.

Source evidence: closeCard and saved reconciliation, answered-card no-op, durable retirement and handle denial.

F2. P2: failed terminal-copy cleanup can park instead of retrying

Location: apps/server/src/secrets/RemoteSecretRequests.ts:418-433; supporting caller OrchestratorMcpService.ts:2417 and cleanup at RemoteSecretRequests.ts:396-398.

Reachable trigger: a consumer commits its task, consumed CAS and confirmed receipt successfully, but removal of the redundant protected request blob fails temporarily. The same filesystem fault also affects the background reconciliation awakened by the commit. Let the fault clear after that reconciliation fails, with no remaining pending/saving/saved/reserved requests and no subsequent automation-change notification. A can be offline after receiving the confirmation, so no later source retirement call is required to occur.

Mechanism: finalization logs and leaves the terminal row/blob for reconciliation. Background reconciliation reaches the terminal row and fails in remove. The catch logs that reconciliation remains pending and sleeps two seconds. However, the next iteration first calculates a deadline using only pending/saving/saved/reserved rows; consumed/declined/retired rows with outstanding deletion are excluded. With no active rows, it waits on PubSub.take(changes) before running reconcile again. The two-second error delay therefore does not actually retry the failed deletion. The unchanged local secret sweep only recognizes secret-request-* names, not these secret-remote-* blobs.

Effect: a redundant private copy can remain for the rest of B's process lifetime, including beyond its expiry, until an unrelated notification, source recovery call or restart happens. This is a cleanup/lifetime failure, not evidence that the secret is exposed to A or that consumption can happen twice.

Correction required: keep failed cleanup eligible for an actual retry before the owner parks. Retain the existing owner/recovery mechanism; no new cleanup framework is required. Prove that removal succeeds after a transient terminal-copy deletion fault clears, with no restart, new request, manual reconcile/drain or unrelated event needed to trigger it.

The committed outer-rollback test at PeerAuthority.remoteSecrets.workflow.test.ts:559-649 explicitly keeps cleanup failing and then calls rebuildSecretOwner; that demonstrates restart recovery but does not falsify this running-process retry failure.

Source evidence: terminal cleanup and wait-before-reconcile loop, post-acceptance finalization, local sweep name filter.

Explicit proof gap for the CI repair

The supplied requirement asks for failure-retained-obligation/retry proof for the repaired hook if not already covered. That combined case is absent from the committed 19-case native file read in full:

  • The returned-handle/run-exit case writes the run event and explicitly invokes drainAutomationDisables; it does not require the reactor to wake and drain automatically.
  • The unreachable-unlink case verifies pending-without-ACK, reconstructs the source, restores transport and explicitly drains. It proves retained delivery uncertainty and exact successor-generation isolation.
  • The unchanged control-reads/RemoteDelegatedTask files, their red runs and the base-reactor ablation establish the catch-up regression and its repair when no active secret obligation exists. They do not establish an active obligation surviving a reconciliation SQL failure while ordinary question processing continues, followed by an automatic successful retirement retry.

Source inspection supports the bounded guard/error-boundary repair: an absent/inactive owner exits early; the new catch contains reconciliation failure; the reactor still publishes the wake; delivery errors return true to the two-second source recovery loop and do not write an ACK. This is useful evidence, but it does not replace the specifically required failure-and-retry observation. The parent should obtain that focused proof on the next exact candidate. This is a verification gap, not an independently reproduced code defect.

Prior design finding dispositions

Prior finding Disposition in the inspected paths
R1 exact retirement Durable source obligations precede card creation; exact credential/origin/generation retirement and ACK verification exist; unlink retains the original recovery credential. Source run/card/link reconciliation and destination revocation are wired. F1 and F2 remain lifecycle defects; automatic hook-failure retry proof remains unverified.
R2 stable intent and actor Create compares canonical safe input and immutable verified origin; reserve binds the complete operation, namespace, project, family and origin. Human answer preparation derives the session from client ingress and B checks its actor independently. remoteSecretIntent strips the value before prepare's digest and record-id inputs. No raw value was found on those identifier paths.
R3 immutable save fencing The saving/attempt SQL transition precedes immutable protected creation; publication is fenced and retries compare the original bytes. Reserve serializes with control, and take is fenced by the SQL state/identity. The native cases exercise publication interruption, startup reconstruction and grant/source-exit overlap. Running-process terminal cleanup retry is F2.
R4 receipt-first recovery A's withContext tries the dedicated receipt route before new source checks. Recovery requires the retained input/envelope, original session/generation and confirmed resource evidence and cannot execute a missing operation. The outer receipt transaction encloses task, automation acceptance and consumed CAS; finalization follows acceptance. Native lost-reply and rollback tests distinguish confirmation from a missing operation.
R5 parity and privacy oracles One scalar/one-field map is explicit; broader remote fields are rejected before prompting and remain #379/#386 scope. B refuses generic peer request_secret. A raw DB/WAL and forbidden logical owners are separated from authorized B webhook storage. Local files/ref/fields paths and local-ref unknown semantics remain separate. No full client/catalog/cross-machine acceptance is inferred.

No independent prior implementation approval exists. The module-private privacy-note repair removes the unused export while web and mobile retain real helper consumers. The causal diagnostic contains the decisive no such table: peer_environment_links error; the repaired reactor's original question-processing code is unchanged in the candidate diff.

Coverage ledger

All rows refer to committed candidate objects. Full means every line was read. Scoped means the complete changed diff plus listed surrounding regions/consumers was inspected; it is explicitly not the required full-file read. This ledger covers all 36 paths and exposes the unfinished scope rather than treating a diff read as full-file coverage.

Changed file Coverage
apps/mobile/src/features/threads/SecretRequestCard.tsx Full, 177 lines
apps/server/src/mcp/McpHttpServer.ts Scoped: authentication/credential/enroll/accept middleware, dedicated routes and route registration
apps/server/src/mcp/McpRequestReceipts.ts Full, 449 lines
apps/server/src/mcp/McpRouting.ts Full, 419 lines
apps/server/src/mcp/OrchestratorMcpService.ts Scoped: schedule identity/replay/recheck/create, remote update refusal and complete requestSecret implementation
apps/server/src/mcp/toolkits/orchestrator/tools.ts Scoped: declarations/dependencies, request_secret, scheduler descriptions and toolkit registration; a combined full read truncated
apps/server/src/orchestration-v2/DelegatedTaskQuestionReactor.ts Full, 181 lines
apps/server/src/orchestration-v2/Orchestrator.ts Scoped: complete dispatchSecretRequestRecord and its transition invariants
apps/server/src/peer/PeerAuthority.remoteSecrets.workflow.test.ts Full, committed 1216 lines, recovered in 1-360 / 361-760 / 761-1216 chunks; dirty additions excluded
apps/server/src/peer/PeerAuthority.ts Scoped: source validation, prepare and identifier inputs, new origin/client methods and frames, deadline, historical receipt and grant replacement
apps/server/src/peer/PeerAuthority.workflow.fixture.ts Scoped: complete changed diff, secret-owner/fault construction, native RPC entry and credential renewal, production-kernel/owner assembly
apps/server/src/peer/PeerForwarding.ts Scoped: full changed diff, original envelope retention and consumer identity path
apps/server/src/peer/PeerLinks.ts Full, 656 lines
apps/server/src/peer/PeerMcpClient.ts Scoped: full changed diff, retirement delivery/ACK, full recovery loop/startup and dedicated receipt/control implementations
apps/server/src/peer/PeerToolRouting.test.ts Scoped: full changed diff and replaced generic-route contract
apps/server/src/scheduledTasks/DestinationAutomationAuthority.ts Scoped: session invalidation hook and its callers from grant changes
apps/server/src/scheduledTasks/ScheduledTaskService.ts Scoped: upsert preflight/capture, outer accept and protected webhook branch, final notification; some large combined output truncated
apps/server/src/scheduledTasks/mutationReceipts.ts Full, 345 lines
apps/server/src/secrets/RemoteSecretRelay.ts Full, 240 lines
apps/server/src/secrets/RemoteSecretRequests.ts Full, 508 lines
apps/server/src/secrets/SecretRequestIngress.ts Full, 12 lines
apps/server/src/secrets/SecretRequests.ts Full, 446 lines, assembled from bounded chunks
apps/server/src/secrets/remoteSecretPersistence.ts Full, 61 lines
apps/server/src/server.ts Scoped: runtime/reactor dependencies, routes and complete changed layer assembly
apps/server/src/testUtils/shardWeights.json Scoped: full diff and new test weight
apps/server/src/ws.ts Scoped: full diff and authenticated session passed to shared ingress
apps/web/src/components/chat/SecretRequestCard.tsx Full, 194 lines
docs/fork-features.md Scoped: complete new remote-secret registry entry and file ownership inventory
docs/map.md Full, 200 lines
packages/client-runtime/src/secretRequest.ts Full, 144 lines
packages/contracts/src/index.ts Full, 94 lines
packages/contracts/src/orchestrationV2.ts Scoped: full diff, card metadata and internal record-command schema
packages/contracts/src/orchestratorMcp.ts Scoped: full diff, complete request_secret input/result and schedule/update contracts relevant to the consumer
packages/contracts/src/remoteSecret.ts Full, 101 lines
packages/contracts/src/rpc.ts Scoped: full diff and scope-authorized shared secret-answer group
packages/contracts/src/scheduledTask.ts Full, 356 lines

Additional reachable consumers inspected: ServerSecretStore immutable publication and cleanup; requestIdentity namespace/generation; PeerLinkEvents subscription and wake semantics; McpToolAccess routing/access wrappers; ScheduledTaskChecks schedule wrapper and DefinitionExtension preparation; server runtime/reactor construction. These additional reads are scoped except requestIdentity and PeerLinkEvents, which were read fully. No source-file full-read claim is inferred from merely hashing it.

Inputs and method

Read canonical operations SKILL; code-review index, review-method and impact-analysis; verification, orchestration, bounded-delegation and test-design; Elon index and algorithm; Great Library README. Read committed worktree AGENTS and contributing guidance, root glossary and relevant Effect-service guidance. Some combined output was truncated, and recovered portions are reflected in the coverage ledger; no complete review-method gate is inferred from an unrecovered truncated source read.

The complete original design report was read at /home/luke/.local/state/chromeria-379-proof/385-cf541-design/report.md. The original independent design review, parent adjudication and author handoff were supplied in full in the assignment and used as requirements, not candidate approval. Actual #385 body and the amendment's acceptance preamble were also obtained read-only with gh; the repeated embedded design review in the latter response truncated and its already supplied complete text remained the input. Relevant #379 semantics came from the supplied approved design/adjudication rather than a fresh full #379 retrieval. The private author report and ownership record were inspected; private preferences are not reproduced.

Elon record:

Requirements and who asked: User-requested independent exact-candidate review against approved #379/#385 and mandatory R1-R5, with no source or runtime mutation.
Deleted: No alternative architecture, extra remote multi-field parity requirement, synthetic destination agent, invented security quota or new cleanup framework.
Bottleneck: Correct card outcome versus retirement and whether pending cleanup truly receives a retry; complete requested full-file coverage also remains outstanding.
Checked myself: Immutable committed diff/hash, complete new owner/relay/protocol and test files, reachable card/timeout/recovery/transaction owners, all twelve proof-record bindings and raw log hashes.

Reused proof and its limits

verification-index.json SHA-256 is cc209177402f9a961b88d143671004cc8aece89c8b0bfda96fd1347403a8e92a, matching the supplied parent binding. Parsed the complete index and all twelve referenced verified-*.json records. Independently verified each record's base/head/tree before/after, zero exit, recorded clean status, each of the 36 changed source hashes before/after against committed Git bytes, and each raw log hash against the index. Result: all twelve match. Machine-readable result: proof-binding-check.json beside this report.

Read the raw final test summary: ten files passed, 160 tests passed. This is reused execution evidence, not tests run by this reviewer. The committed native test file's assertions and fault seams were read in full; its cases use actual private WS ingress, HTTP peer paths, isolated SQLite and protected files, with fixture provider/catalog boundaries. Save/SQL faults and service reconstruction are not an OS process kill or installed-provider/client/cross-machine proof.

The twelve records cover focused tests, five scoped compilers, changed lint/format, supported exports, registry/fork and whitespace checks. Compiler success and hash identity do not prove F1/F2 absent. Did not read all raw log text or rehash the complete 261433-entry installed-runtime manifests. The index's explicit generated Vitest results.json difference is preserved as a limit; no all-runtime-byte-identity claim is made. Prior d269 failure and base-reactor ablation are historical causal evidence, not proof for the current head. The diagnostic's actual missing-table error and final 160/160 summary were checked directly.

Unverified: complete requested full-file review coverage; deterministic reproductions of F1/F2; active-obligation reactor-error/automatic-retry proof; full CI completion/current live forge state; real client screenshots, installed harness and cross-machine acceptance; broader #379/#386 catalog parity and cumulative integration gate. No merge, deployment, outcome closure, independent-review success status or Opus-review clearance is claimed. Parent retains publication, status, adjudication and all topology/delivery actions.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Opus requests changes on c9e93, confirming Sol lifecycle findings and identifying transient-frame leakage and destructive handling of unavailability.
Why: Temporary transport/storage failures must preserve private state and remain recoverable; timeout cleanup must respect the saved winner.
So what: Parent accepts four concrete source defects and bounded cleanup retry/startup recovery, releases deterministic author repair, and requires both new whole-candidate reviews afterward.

Parent adjudication: Sol-F1 timeout race and Sol-F2 parked terminal-copy retry accepted; Opus-F2 missing secret-origin cleanup in both ask finalizer/startup sweep accepted; Opus-F3 transient validation error retiring/deleting saved state accepted. Opus-F6 startup cleanup failure preventing service construction is accepted within the same cleanup recovery owner: isolate failed cleanup without hiding schema/initialization or authority errors, retain pending material and observable retry. Opus-F1 proof gap remains unaccepted for c9e; the newer cbd72 test is retained for separate assessment. Opus-F4 global semaphore latency and F5 unbounded history/wake overhead are source-supported performance concerns but unmeasured; defer broad concurrency/history redesign in this repair, carry them into subsequent review and acceptance. Other optional wording/guidance/placement/span observations remain optional with no requested source change. Neither reviewer completed the requested every-line large-file coverage, so no whole-file clearance is claimed. Future exact-candidate reviews must close the necessary coverage honestly.

The follow-up addendum arrived on the same delegated task after a queued candidate-freeze notice. It is supplementary source evidence only, not a new independent review round or additional approval; all claims remain bound to c9e. Current cbd72 production is identical to c9e, and its 150-line test-only addition transfers no verdict. Complete report including addendum follows unchanged.

PR #583 whole-candidate review, round 1 (Opus 5.5 MEDIUM)

Verdict: REQUEST_CHANGES for exact candidate c9e93fbaf996c4b5443aa94ad2d2edc0786e8da3.
The architecture matches the approved design and R1-R5 amendments, and I found no secret-exposure or authority-bypass path. Two small reachable defects (F2, F3) and one parent-required proof gap (F1) block approval of this exact head. F4-F6 are recommended, not blocking.

Reviewer: Claude Opus 5.5 (claude-opus-5-5), MEDIUM, Claude Code harness inside T3 Code. Date 2026-10-20, Europe/Warsaw. I did not author the implementation. This review is independent of the concurrent Sol review; I did not read or wait for its verdict.

Identity and workspace state

Item Value How checked
Candidate HEAD / tree c9e93fbaf996c4b5443aa94ad2d2edc0786e8da3 / 8140a5eed7ae6d272943832180092795515959cb git rev-parse HEAD HEAD^{tree} at review start
Base de50f47a47a87310649e058da0467badc1d48938 git diff de50f47a..c9e93
Diff 36 files, +3135/-65, SHA-256 a3b4f00347e89b649cfb68b235173ca46fcd72e628297227535580cd35113504 git diff de50f47a c9e93 | sha256sum; matches the brief
Scratch /home/luke/.local/state/chromeria-379-proof/583-c9e93-review/opus55 plain mkdir succeeded before any write

The worktree was not frozen. At the start, git status showed an uncommitted 150-line change to apps/server/src/peer/PeerAuthority.remoteSecrets.workflow.test.ts (mtime 15:07:46). During the review, HEAD moved to cbd72f9cdb8066e0c4c6e73b41a031b681c5cde6 (tree 2a1b36d00fdc34f8204eed7a6ac9c4bf147f40c1, committed 15:09:08, "test(secrets): retain failed retirement for recovery"). git diff --stat c9e93 cbd72f9 shows that commit adds 150 lines to that one test file and changes nothing else. Another writer is active in this workspace. I made no writes there. All non-test source I read through the working tree is therefore byte-identical to c9e93. I read the test file only through git show c9e93:.... This verdict binds c9e93 only. cbd72f9 is a new candidate and needs its own review.

Findings

F1 (required proof, parent-specified): the c9e93 error boundary with an active obligation is untested at this head

  • Change: c9e93 adds Effect.catchCause around reconcileObligations in DelegatedTaskQuestionReactor.ts:54-58. It also adds early returns in remoteSecretPersistence.ts:39-45 and PeerMcpClient.ts:549-552.
  • What the committed tests cover: only the uninitialized path. The unchanged control-reads and RemoteDelegatedTask files never create remote_secret_obligations, so they exercise the guard. No committed test makes reconcileObligations fail while an active obligation exists, and then shows that (a) question delivery still happens, (b) the obligation keeps pending=0 with no ACK, and (c) a later drain retries it.
  • Source reading of that path: it is plausible. The hook still publishes automationChanged. The drain calls reconcileObligations again, and a failure there makes deliverSecretRetirements resolve true, which retries every 2 s (PeerMcpClient.ts:613-616, 664-667). This is inference, not execution.
  • Required: the parent explicitly required failure-retained-obligation/retry proof. It does not exist at c9e93. The test that appeared in cbd72f9 claims to be that proof. It belongs to a different head and is unreviewed here.

F2 (required, small): secret-origin control frames are never deleted

  • Location: PeerAuthority.ts:741-751 (the ensuring delete in ask) and PeerAuthority.ts:448 (the startup sweep). Both list validate, capture-automation, capture-legacy-automation, request-actor and request-ancestry. The new secret-origin frame (PeerAuthority.ts:150, sent from secretOrigin, new code around line 3604) is in neither list.
  • Trigger: every B-side control create (RemoteSecretRequests.ts:189) and every reserve (:284) calls authority.secretOrigin(record.id). On B that is an ask over A's session.
  • Effect:
    • Each peer_control_frames row holding the full envelope JSON is retained forever. The startup sweep never removes it either.
    • An unanswered frame, for example after the 25 s timeout, also counts toward the per-session budget of 32 unanswered frames (:707) until A processes it. The transient-frame contract that every other ask type follows is broken.
  • Fix: add 'secret-origin' to both lists.

F3 (required, small): B destroys a saved or reserved value on transient validation failure

  • Location: RemoteSecretRequests.ts:154-163 (eligible). checkInteractiveRecord(...).pipe(Effect.tapError(retire + remove)) retires the row and deletes the protected blob on any failure.
  • Which failures count: this includes orchestration_error "unavailable" from ask. That error comes from the 25 s wait timeout (PeerAuthority.ts:726-734), the 32-frame budget being full, or a SQL error.
  • Inconsistency: the A-side drain makes the distinction correctly. It marks an obligation pending only on error.code === "capability_denied" (PeerMcpClient.ts:563-571). The approved state machine retires on "source liveness/revocation denial".
  • Trigger: eligible runs in answer, status and in reserve, which runs inside schedule_task acceptance. A slow A or a long-poll reconnect gap at that moment is enough.
  • Effect: the user's entered value is deleted irreversibly. The agent gets capability_denied and must request a new secret, although the source never denied anything.
  • Fix: retire only when the code is capability_denied; otherwise fail with unavailable and leave the state unchanged.

F4 (recommended): one global semaphore held across network round trips

  • Location: RemoteSecretRequests.ts:126. lock wraps control, reserve, retire and reconcile (:269, :300, :375, :411).
  • What runs under the lock: control and reserve make up to three B-to-A asks: secretOrigin, secretClient, and eligible's validate. Each can wait up to 25 s.
  • Effect:
    • One slow or offline source blocks every other link's remote-secret create, answer and reserve on B, plus the reconcile loop.
    • Reserve runs inside the scheduler's task-locked acceptance, so it can stall up to about 75 s per queued call.
    • This is not a deadlock: no source handler re-enters B's owner. It is still head-of-line blocking across peers.
  • Smaller shape: do the live checks outside the lock and keep the lock (or per-request CAS) around state transitions and blob publication only.

F5 (recommended, performance): retirement wakes on every run end and B rescans its full history each time

  • Wake frequency: DelegatedTaskQuestionReactor.ts:46-61 publishes automationChanged on every run.updated whose status is not active (including queued and waiting), and on every archive, on every server. It does this whether or not any obligation exists. Before this change, that PubSub fired only on fork plans, link changes and auth changes.
  • Cost per wake on A: the drain runs deliverSecretRetirements. nextAutomationDisableDeadline now also runs the three CREATE TABLE IF NOT EXISTS statements each time (PeerAuthority.ts:3474-3477).
  • Cost per wake on B: the subscriber runs the full reconcile under the global lock (RemoteSecretRequests.ts:413-438):
    • an UPDATE with an auth_sessions subquery;
    • SELECT * over all remote_secret_requests rows, which are never pruned;
    • a re-read plus a store.remove (an unlink returning NotFound) for every terminal row;
    • a readDirectory of the secrets directory.
  • Effect: cost grows with lifetime request count and the wake is frequent. That conflicts with "Performance without compromise".
  • Smaller shape: publish only when an obligation row changed, and skip terminal rows whose blob has already been removed (or prune them).

F6 (recommended): startup reconcile failure fails B's server layer

  • Location: RemoteSecretRequests.ts:412. yield* reconcile runs inside layer construction (server.ts uses Layer.provideMerge(RemoteSecretRequests.layer ...)).
  • Trigger: any typed failure fails construction. Examples are a store.remove failing with something other than NotFound (EACCES, EPERM, EBUSY; see ServerSecretStore.ts:300-313), a decodeConsumer failure on one row, or an unreadable secrets directory.
  • Effect: the server cannot start. At runtime the same single bad row aborts the whole loop, so every other row's recovery waits behind it, with a retry every 2 s.
  • Fix: isolate per-row failures, and log and continue at startup.

Optional observations (not blocking)

  • The status control variant (RemoteSecretRequests.ts:222-226, remoteSecret.ts union) has no caller.
  • The card text shows the raw B projectId (client-runtime/src/secretRequest.ts:14-17), which means little on A's client.
  • Agent guidance in provider-core/src/server/orchestrationInstructions.ts:18 still describes only local secretRef; the tool description was updated.
  • Hosting secret retirement in DelegatedTaskQuestionReactor puts an unrelated owner in a question reactor. That placement produced the d269 CI regression.
  • Pending obligations whose B row no longer exists retry every 2 s indefinitely. This follows the existing automation-disable pattern and is not a new pattern.
  • ws.ts lost the orchestration_v2.thread_id span annotation on secretsAnswerRequest. SecretRequests.answer still annotates it internally.

R1-R5 and acceptance disposition (source-read)

  • R1 retirement.
    • The obligation is inserted before the card exists (RemoteSecretRelay.ts:179-185, before record("pending")).
    • Run end, archive and card cancellation go through the reactor's reconcileObligations. Card close and answer races go through relay.retire.
    • Unlink and replacement go through invalidateLink plus retaining the retired-link credential. Local expiry and link expiry are handled in reconcileObligations. B-side revocation is handled in invalidateSessions through invalidateAutomationSessions.
    • The exact ACK identity is compared before persisting (PeerMcpClient.ts:605-608). The B retire route binds to the original credential hash, origin and generation, and reconciles the consumer receipt first (RemoteSecretRequests.ts:357-375).
    • Satisfied, subject to F1, F3 and F5.
  • R2 identity.
    • Create replay compares input_digest and origin_json (:196-203). A later run is denied.
    • Reserve binds namespace, origin, project and family. Take compares the full RequestIdentity.
    • Answer requires secretClient (B asks A for request-actor). prepareSecretAnswer requires an operate-scoped non-MCP client session derived from the WS session (SecretRequestIngress.ts).
    • The value is excluded from every digest and record id through remoteSecretIntent. Satisfied.
    • Cleared: another peer forging A's environment in an envelope. enroll binds each session to its durable environment and generation (PeerAuthority.ts:686-697), so owned() is effectively bound to the session.
  • R3 fencing.
    • A durable saving attempt precedes store.create, which uses an exclusive hard link. saved is published only through a CAS on the winning attempt id. There is no saved-value replacement.
    • The attempt id is fixed per obligation, so a lost reply retries the same attempt. The temp-file sweep runs under the same lock as create.
    • Satisfied, with the F4 caveat.
  • R4 receipt-first.
    • McpRouting.withContext runs relay.recover before any authority work. The B receipt route has no authority header, so it triggers no accept or ask. It binds session, generation, namespace, project, the original envelope and the original record id (:447-505).
    • Without a receipt, PeerForwarding re-validates the original envelope's record live and reuses it. Reserve runs outside SQL. Take, the receipt, the task and automation share the outer transaction. Finalize runs after MutationReceipts.accept returns.
    • I verified that no ownedSqlMutation or sqlMutation wraps schedule_task; only IssueLinks.ts uses it.
    • Satisfied.
  • R5 bounds.

Coverage ledger (all 36 files)

Read in full as diffs (git diff de50f47a c9e93 -- <file>), with surrounding consumers as noted:

  • Contracts: index.ts, orchestrationV2.ts, orchestratorMcp.ts, rpc.ts, scheduledTask.ts. remoteSecret.ts was read as the whole file.
  • New server files, each read whole at c9e93: RemoteSecretRequests.ts, RemoteSecretRelay.ts, remoteSecretPersistence.ts, SecretRequestIngress.ts.
  • Changed server files:
    • SecretRequests.ts, ws.ts, server.ts, PeerLinks.ts, Orchestrator.ts, McpRequestReceipts.ts, tools.ts, PeerToolRouting.test.ts, shardWeights.json.
    • DelegatedTaskQuestionReactor.ts, read whole at head.
    • PeerAuthority.ts, plus ask, enroll, validate, accept, checkPayload, readInteractive and checkInteractiveRecord in context.
    • PeerMcpClient.ts, plus the recovery loop and start in context.
    • McpHttpServer.ts, plus the auth middleware in context.
    • McpRouting.ts, plus invokeRouted in context.
    • OrchestratorMcpService.ts, PeerForwarding.ts, DestinationAutomationAuthority.ts.
    • ScheduledTaskService.ts, plus the upsert accept site in context.
    • mutationReceipts.ts, plus accept in context.
    • PeerAuthority.workflow.fixture.ts.
    • PeerAuthority.remoteSecrets.workflow.test.ts, all 1216 lines through git show c9e93:, in three ranges.
  • Clients: web and mobile SecretRequestCard.tsx, client-runtime/secretRequest.ts.
  • Docs: docs/fork-features.md, docs/map.md.
  • Consumers also read: ServerSecretStore.create/remove, PeerLinkEvents.ts, EventSink wake, ProjectionStore active-run statuses, RunExecutionService waiting semantics, McpRequestReceipts.sqlMutation/ownedSqlMutation, provider-core orchestration instructions, and the docs/user secret coverage (none exists).
  • Required inputs read:
    • operations SKILL.md, code-review index, review-method, impact-analysis, verification, test-design, orchestration, bounded-delegation;
    • Great Library README; worktree GLOSSARY.md, CONTRIBUTING.md, docs/internals/effect-services.md;
    • the design report (full text as supplied in the brief, with head and tail checked against 385-cf541-design/report.md);
    • the feat(peer): request destination secrets and create signed webhooks without a destination agent #385 body and amendment comment 6289829273 (read-only gh; the brief embeds 6289669134);
    • the author handoff (full text as supplied in the brief);
    • verified-focused-tests.log (tail shows Test Files 10 passed (10), Tests 160 passed (160); its one "Remote secret reconciliation remains pending" warning is the deliberate cleanup fault);
    • the top-level keys of verification-index.json.
  • Not read in full:
    • verification-index.json's records and the per-check runtime manifests. The parent already verified their hashes; I relied on its stated check and did not rehash 261k entries.
    • 385-implementation/report.md as a file, separately from the brief's copy.
    • owned-inventory.json.

Proof classes and limits

  • Executed: read-only git, gh and file reads only, plus scratch writes (report.md, full.diff).
  • Not done: no tests, install, server, browser, device, provider, live DB, GitHub write or nested agent.
  • Findings are source-read evidence. F2 is definite from the source lists. F3's trigger requires an ask timeout or other unavailability, which I did not execute. F4 to F6 are source-read reasoning; I made no performance measurements.
  • I reused the author's 160/160 focused-test result, its logs and the parent's hash verification as evidence. I did not reproduce them.
  • No CI clearance, client or cross-machine acceptance, or merge readiness is claimed. These remain with the parent.

Elon record

Requirements and who asked: Parent-requested independent whole-candidate review of #583 against the approved #385 design, R1-R5 and the parent's c9e93 proof requirement.
Deleted: Re-running proof, nested lenses, waiting for Sol, and any new interface or design proposal.
Bottleneck: Where live source validation meets destination state transitions: the eligible/lock/ask paths and transient mailbox frames.
Checked myself: Every changed file at c9e93, the cited consumer sources above, the committed test bodies, the raw focused-test summary, and the workspace HEAD movement to cbd72f9.

Addendum: candidate freeze notice and Sol lead evaluation

The assigned candidate remains c9e93fbaf996c4b5443aa94ad2d2edc0786e8da3. I do not review or approve cbd72f9cdb8066e0c4c6e73b41a031b681c5cde6.

Moving workspace and reads not bound to Git objects (disclosure)

At this addendum the workspace HEAD is cbd72f9, with a clean status. In my first pass I read the following non-test sources through the moving working tree rather than through git show c9e93: objects:

  • Orchestrator.ts: the isBlockingRun and delegatedTaskTerminalStatus region.
  • PeerAuthority.ts: validate, ask, enroll, readInteractive, checkInteractiveRecord, and the 448/700-753 regions.
  • McpHttpServer.ts, around lines 286-360.
  • PeerMcpClient.ts, around lines 433-467 and 655-740.
  • McpRequestReceipts.ts, around lines 325-449.
  • ScheduledTaskService.ts, around lines 1300-1330.
  • mutationReceipts.ts, around lines 262-300.
  • ServerSecretStore.ts.
  • ProjectionStore.ts, ThreadManagementService.ts, EventSink.ts and PeerLinkEvents.ts, around the cited lines.

Two facts bind those bytes to c9e93:

  1. Throughout my reads, the only working-tree difference from HEAD was the single test file (git status at entry and before the commit).
  2. git diff --name-only c9e93 cbd72f9 names only apps/server/src/peer/PeerAuthority.remoteSecrets.workflow.test.ts.

So those non-test reads are byte-equal to c9e93. This rests on those two Git observations; I did not re-read each file through git show. I read the test file only through git show c9e93:. Everything in this addendum was read through git show c9e93: or the explicit de50..c9e93 diff.

Sol F1 (timeout closeCard retires an already-saved winner): CONFIRMED by source

At c9e93:OrchestratorMcpService.ts:2717-2787 the sequence is:

  1. closeCard is record("cancelled") followed by an unconditional relay.retire.
  2. If the human save commits on A during the final poll sleep, timeoutOption yields None, so line 2765 runs closeCard.
  3. record("cancelled") is a no-op on the saved card. The Orchestrator's unchanged branch (c9e93:Orchestrator.ts:9040-9044) keeps the existing non-pending item.
  4. relay.retire still sets the obligation to pending=1.
  5. The fallback projection read selects saved. relay.handle (RemoteSecretRelay.ts:232-236) then denies because pending !== 0.
  6. The next drain retires B's row and deletes the blob.

The agent gets capability_denied while the card shows saved, and the value is lost. The same key cannot recover, because relay.create denies an obligation with pending !== 0.

The trigger is the normal private answer landing in the last poll interval before the timeout. Required fix. Retire only when the card transition actually committed cancelled, or when the reread status is not saved. The interrupt path (onExit failure) is a separate case: there, retiring is acceptable because no caller remains to receive the handle.

Sol F2 (failed terminal-copy cleanup parks without retry): CONFIRMED by source, with one qualification

At c9e93:RemoteSecretRequests.ts:413-438:

  • A failed reconcile, for example a store.remove error on a terminal row's blob (:396-398), is caught, then sleeps 2 s.
  • The next iteration's deadline query covers only pending|saving|saved|reserved rows.
  • With no active rows, the loop blocks on PubSub.take(changes) and never retries the failed removal.
  • finalize failure (OrchestratorMcpService.ts after accept) only logs.

Qualification. My F5 shows that any non-active run.updated on that server also publishes automationChanged, so a busy B would eventually retry on an unrelated wake. The defect is fully reachable on an idle B, which is exactly the #385 target topology. There the redundant protected copy survives until a restart, an unrelated link or auth change, or a new request.

Required fix, small. Include terminal rows that still have a blob in the retry decision, or keep retrying while the last reconcile failed. This finding and my F6 share an owner: one bad row should neither park nor abort the loop or startup.

Updated verdict

REQUEST_CHANGES on c9e93.

  • Required: F1 (parent proof gap at c9e93), F2 (secret-origin frames), F3 (transient-error retirement), Sol-F1 (timeout retire of a saved winner), Sol-F2 (cleanup park).
  • Recommended: F4-F6.

Overlap with Sol: both reports independently identify the c9e93 hook-retry proof gap. Neither Sol finding duplicates my F2-F6. Every finding is source-read evidence only; I executed no reproduction.

Coverage remains as stated in the main ledger. All 36 files were covered through diffs, and the new owner and test files were read whole at c9e93. Several large modified files (for example PeerAuthority.ts, PeerMcpClient.ts, OrchestratorMcpService.ts and ScheduledTaskService.ts) were read as their full diff plus relevant consumer regions, not every line of the file. That is a limit on any "every line of every file" claim.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #599, which replays this PR's six commits onto main with the required lifecycle findings re-proven, passed whole reviews by GPT-6.1 Sol and Grok 4.7 on its exact head, and merged as 6fe0b0e.

@lukemaj lukemaj closed this Oct 20, 2026
@lukemaj
lukemaj deleted the feat/385-remote-secret-channel branch October 20, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant