feat(cloud): add operation ledgers and event contract - #5093
Merged
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryIncremental review of 1 new PR-authored commit ( Overview
Issue Details (click to expand)WARNING
SUGGESTION
Resolved in this range
Files Reviewed (1 PR-authored commit)
|
Contributor
Author
|
(bot) Kilobot review not detected after two retriggers, proceeding further |
Contributor
Author
|
(bot) Rejected the carried organization key warning because the hook owns one mounted mutation surface, and a remount starts a new user interaction. The key remains stable across retryable errors while that owner remains mounted; no new evidence changes the approved plan contract. |
jeanduplessis
approved these changes
Aug 17, 2026
Net -3080 lines with no functional change. - Collapsed duplicated ledger helpers: one bounded canonical-result merge, one non-terminal row update, one outbox validation. - Deleted a third dedupe layer in the cloud-agent SDK and a verbatim copy of the mobile spawn classifier. - Collapsed repeated admit/settle/reconcile blocks into one helper per router. - Merged the two identical security worker clients into one shared client. - Deleted the unwired analytics capture module, the speculative route-lifecycle publisher, and the mobile analytics re-export hop. - Reused existing helpers: cron auth, terminal-status predicate, organization role list, and the shared owner/repo schema. - Cut doc comments that restated the code and removed duplicated test coverage.
- Deleted `setOperationProviderRef`: dead, and the only ledger write path with no row lock and no terminal-status check. - Deleted `resolvePrDistinctId`: `ctx.user` already holds the email, so the per-mutation user query is gone. - The role-change repair path now audits the role as reconciled instead of claiming a `from` role it never observed. - A scheduled security sync no longer reads the operation ledger; only a manual trigger can hold a row. - The PR and organization routers import the shared settled-event constants instead of hardcoding the names. - Finding dismissal has its own in-progress copy instead of the sync wording. - Recorded the screens that mobile tracking stops reporting. - Added a development-only warning naming redacted analytics keys.
- One `runAfterResponse` in `apps/web/src/lib/after-response.ts`. It keeps the hardened error handling: a synchronous `after()` throw and a rejected work promise are both reported instead of becoming unhandled rejections. - One `sha256Hex` in `services/cloud-agent-next/src/utils/sha256.ts`. `canonicalJson` stays duplicated on purpose. The two copies serialize `undefined`, sparse arrays, `Date`, `NaN`, and `Infinity` differently, and the output is a dedupe identity, so merging them would rotate every in-flight operation key for the ledger's retention window.
`UQ_organization_memberships_org_user` already makes the role update and the membership delete idempotent, and the gateway-state revoke is idempotent too. The ledger only added a 120-second lease that rejected a legitimate second attempt, so it is gone along with the `operationKey` input, the admission dispatcher, the repair and replay paths, and the mobile key hoisting. Removed `organization_write_settled`. It was unconsumed and outbox-only, so keeping it would have needed a new capture path. Net -2327 lines. Authorization, owner authority, every error message, the audit rows, and the post-removal cleanup are unchanged. Three tests that only the deleted ledger suite covered moved into the router test. Accepted trade-off: a duplicate tap now writes a second audit row.
The field list lived twice, once in the router and once per mobile hook. Drift would have rotated every in-flight operation key and rejected same-key retries for the ledger's 30-day retention window. `packages/app-shared/src/pr-review/intent-fingerprint.ts` now owns the field list and builds the fingerprint; the router and the three mobile hooks import it. The router keeps the `owner/repo#number` prefix and the hashing, because `node:crypto` is not platform-agnostic. Output is byte-identical over 44 inputs across the four intents, covering absent optionals, reversed key-insertion order, unicode, and quoting. Both old copies agreed on every input. Pinned-literal tests now guard both sides.
`security_agent_commands` already holds the terminal state the UI polls, and the dismissal is idempotent against the GitHub alert state, so retry safety needs only a key on the command row. `operation_key` plus two partial unique indexes, one per owner column, now enforce it. The owner must be in the identity because a key is unique per actor only; a single composite index cannot work, since the unused owner column is NULL and Postgres treats NULLs as distinct. The worker loses its ledger reads, its settle calls, its outbox insertion, and the queue-send claim fence: net -485 lines. A keyed enqueue creates or finds the command row and sends no queue message when the row already exists. The web side settles its own ledger row when it observes a terminal command, so only the ledger helpers still insert outbox rows. Deploy `apps/web` first, then `services/security-sync`.
Security, share, and session hooks imported `useHoistedOperationKey` from `lib/pr-review/merge/pr-operation-ledger.ts`, which is the wrong dependency direction. The intent-agnostic pieces now live in `lib/operation-key.ts`: the hook, both server markers, and one shared `isOperationInProgress` predicate that replaces the two near-identical copies. The PR module keeps its PR-only messages, surface copy, and error mapping. Each surface still supplies its own in-progress copy, so the PR, security sync, and dismissal strings stay distinct.
The PR added `eslint-disable max-lines` to two files instead of splitting them. Both disables are gone. - `use-continue-session.ts` gives up its cloud-agent leg to `use-continue-cloud-create.ts`: 291 and 103 lines, was 369. - `use-security-findings.ts` gives up the remediation write hooks to `use-security-remediation.ts`: 203 and 186 lines, was 379. The post-success containment moved verbatim: the key still rotates before the UI work, each step is contained on its own, and only a non-retryable `prepareSession` error ends the intent.
Four conflicts, all in apps/mobile. Both sides' intent survives in each. - `use-pr-review-mutations.ts`, `use-pr-merge-mutations.ts`: kept the ledger `mutationFn` and main's accessibility announcements. Failures now announce through `announcingToast` carrying the mapped ledger copy. - `use-new-session-creator.ts`: kept main's `onCreated` draft clear inside the post-success containment, and contained the analytics and cache-invalidation calls together so neither can skip it. - `use-new-session-creator.test.ts` was added on both sides. Combined into one harness with all 20 tests, 10 per side. `use-pr-review-mutations.test.ts` needed main's `react-native` mock, since the production file now reaches `announce.ts` and the pure vitest project cannot parse Flow source.
A definitively-lost `createSessionWithInitialAdmission` RPC left the operation key stuck: the ownership row existed, both metadata reads stayed absent, and every same-key retry threw `creation_in_progress` forever, so the client never reached a terminal outcome. Branch (c) now settles the row `failed` with `create_rpc_abandoned` once it outlives `SESSION_CREATE_ABANDON_AFTER_SECONDS` (900s, 7.5x the create lease). Below that age the behaviour is unchanged. The double-execution guard is intact: nothing is deleted, no fresh create is allocated from absent state, and the settle only flips the row terminal. A later same-key retry replays the terminal outcome, and the user's next attempt carries a new key, new session IDs, and a new Durable Object. `BAD_REQUEST session_creation_failed` already classifies as terminal on mobile, so the client rotates its key instead of retrying. The test fixture now dates rows relative to now. Its hard-coded date made every row 11 days old against the real clock, which would have put the existing branch-(c) tests into the abandonment path.
…_ref This PR added two migrations. Neither has been applied to production, so they are now one: `0216_familiar_boomer`. `IDX_operation_ledgers_provider_ref` is now `WHERE provider_ref IS NOT NULL`. The column is NULL for every non-security domain, so the partial form is smaller and equally fast for the join it serves. Anyone who already ran `0216_secret_alex_wilder` or `0217_public_kylun` locally must reset their local database.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the 17 files changed since the previous review: all 7 prior findings (security-sync same-key re-enqueue, ambiguous outbox event identity, PII in dispatch logs, uncontained Files Reviewed (17 files)
No memory leaks introduced (no new listeners, intervals, or unbounded structures). Previous Review Summary (commit 4cdebb1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4cdebb1)Status: 7 Issues Found | Recommendation: Address before merge Executive SummaryThe security-sync same-key dedupe branch acknowledges retries of already-terminally-failed commands with 202 Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (106 files)
Note: existing-comment reconciliation was unavailable during this run (GitHub returned 403 on comment-list endpoints), so duplicates against pre-existing human comments could not be excluded. Reviewed by kimi-k3 · Input: 157K · Output: 11.7K · Cached: 524.2K Review guidance: REVIEW.md from base branch |
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
2 participants
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.
User: The app now keeps cloud and remote session creation tied to one user intent, so safe retries do not create duplicate sessions.
Product manager: PR actions, security commands, and organization writes now deduplicate safe retries; terminal outcomes use a durable analytics outbox; mobile tracks only settled visible leaf routes.
Maintainer: Postgres operation ledgers provide admission, replay, lease takeover, reconciliation, terminal settlement, and atomic outbox insertion. The session-ingest Durable Object keeps remote create deduplication. Mobile passes stable intent keys and contains post-success UI failures.
Changes:
Human steps: None. The normal deployment migration process applies the generated PostgreSQL migration.
Visual Changes: N/A
E2E: bot-e2e — final iOS verification passed for cloud duplicate replay, analytics outbox delivery, organization role replay, cloud takeover reconciliation, settled Home tracking, and KiloClaw route exclusion.
Verification:
pnpm test:db, root typecheck, root lint, changed-file formatting, shared analytics tests, cloud SDK tests, worker-utils tests, Cloud Agent Next tests, and cumulative implementation review passed. CodeQL remains red because repository code scanning is disabled; the same CodeQL jobs fail onmainwith HTTP 403 during SARIF upload.