feat(mobile): honest state across sessions, security, and reviews (w4a) - #5324
Merged
Conversation
Add a beforeRemove discard confirm for a non-empty new-session prompt. Skip the confirm on a successful Start or remote spawn, and re-arm it after a failed spawn. Stay and toast when the stored draft cannot be cleared, so a discarded prompt can never resurface.
Persist the dismiss reason, comment, last error, and retry class per scope and finding. Restore the draft on remount and refresh the retry card on the mounted dashboard and finding-detail screens after the sheet closes. Clear the draft only after an authoritative accept.
Carry actionRequired through ReviewConfigData and render a distinct banner above the enable switch. The three user-facing strings live in @kilocode/app-shared; web re-exports them instead of keeping a copy.
Lead with conclusion, findings, council decision, and gate before metadata. Flatten council findings and paginate in memory, appending pages so earlier findings stay visible.
resetCodeReviewForRetry now updates only rows still in a retryable status and returns the row count. A zero-row update surfaces as a conflict instead of a successful retrigger.
Mount a mobile-scoped agentProfiles router and render a read-only profile row with capability counts. Resolve personal default over org default. Block Start until the profile query settles and keep Start enabled on a failed query, submitting with no profile id.
setEnabled throws PRECONDITION_FAILED when the effective repo set is empty. The mobile switch is disabled with an explanation and the repo picker stays reachable while disabled. startAnalysis always sends forceSandbox. A loading or failed repo query is not read as empty.
Classify connectivity as online, offline, or unknown and treat unknown as not-online. Start the banner store unknown, pause React Query on offline and unknown, and clear account/permission cache on change. An unknown state with empty cache shows a skeleton, not Connect or $0.
The fulfilled-result predicate used an unassignable shape. Narrow it to SuccessResult so the parallel-retrigger test typechecks.
Write invitation, audit, and outbox row in one transaction so the mutation never sends mail. Drain via cron with the analytics-outbox pattern. Resend resets the same row, fenced on the invitation still being valid, and refuses a revoked or expired invite. Expose emailStatus on the list contract and keep copy honest (Invite created, not sent).
Replace the hardcoded autoCommit:true with a two-way choice that defaults to Leave changes. Leave maps to autoCommit:false and Commit and push to autoCommit:true. The api.send payload omits autoCommit so the worker inherits the prepare value. The choice is part of the intent fingerprint so a changed choice is a fresh intent.
Add config_revision to agent_configs and admitted_config_revision to security_analysis_queue. getConfig returns the revision and save takes an expectedRevision; a stale revision yields CONFLICT. The backlog enqueue and the CAS write share one transaction so an enqueue throw rolls back the config. Migrate the web and mobile save consumers.
Persist safe-retry and reconcile-first mutation rows under outbox:<userId> so a crash mid-flight reuses the stored operationKey instead of minting a duplicate session. Only safe-retry session registration auto-replays; reconcile-first security sync surfaces a retry card and never auto-POSTs. The sync wire always uses the row's own key, and the dashboard filters reconcile cards to its scope.
The S14 CAS change made securityAgent.getConfig return a discriminated union on hasConfig/configRevision. The mobile optimistic updates and the settings screens' dirty-state refs spread patch objects over that union, which no longer type-checks. Add a FlattenedSecurityAgentConfig mapped type and use it for the spreads; fix the onError key cast and the two test files' invocationCallOrder assertions.
S13 added emailStatus to InvitedOrganizationMemberSchema; the storybook mock data for invited members was missing it.
Fix four lint errors in the security settings overview: extract the nested loading/error/copy ternary into a helper, use optional chaining for the empty check, and drop the redundant data-null guards.
The continue path minted a fresh in-memory operationKey and never wrote a safe-retry row, so a kill mid-continue could create a second session. The sync path did not gate on the outbox load, so a crash row's stored reconcile key could be overwritten by a fresh key. Gate both on whenLoaded and reuse getStoredOperationKey before minting.
use-security-agent-mutations imports the outbox (P1-E-40c), which pulls @sentr/react-native (Flow) transitively; the findings test loaded react-native's index.js and failed the transform. Mock the outbox so the pure-logic test never loads it.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (5 snapshots, latest commit 38c1f94)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 38c1f94)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (50 files)
Fix these issues in Kilo Cloud Previous review (commit 2ace7c8)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit 2728351)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryAn unbounded remove→refetch loop in the mobile query cache's permission-denied handler and an invite-email outbox claim that ignores invitation validity are the highest-risk issues in this otherwise well-tested honest-state change. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (128 files)
Fix these issues in Kilo Cloud Previous review (commit 7c0b995)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryAn unbounded remove→refetch loop in the mobile query cache's permission-denied handler and an invite-email outbox claim that ignores invitation validity are the highest-risk issues in this otherwise well-tested honest-state change. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (128 files)
Fix these issues in Kilo Cloud Previous review (commit 5082220)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryAn unbounded remove→refetch loop in the mobile query cache's permission-denied handler and an invite-email outbox claim that ignores invitation validity are the highest-risk issues in this otherwise well-tested honest-state change. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (128 files)
Reviewed by grok-4.6 · Input: 111.4K · Output: 10K · Cached: 248.2K Review guidance: REVIEW.md from base branch |
Remove the unused isProfileError parameter and the always-true copyable field, and fix require-await lint errors in the outbox test mock.
OutboxTaxonomy is only used by OutboxRow.taxonomy inside the module. The export tripped knip's check:unused gate in CI.
Removing a still-observed query inside queryCache.onError rebuilds it on the next render and refetches, which fails FORBIDDEN again and loops for as long as the error screen stays mounted. Only drop queries nothing is observing.
Legacy review rows can carry a manual_config without agentConfig. Reading gate_threshold through the missing inner object crashed the detail screen instead of showing the error surfaces this PR adds.
The empty-state Sync findings control fired the reconcile-first mutation with only onSuccess, so a retryable failure left the reconcile card invisible until the next focus refresh. Match the other sync entry points with onSettled.
The claim loop selected pending rows without checking the linked invitation, so an accepted or expired invite was still emailed. The resend reset matched a sending row, so a mid-send resend could email the invitee twice. Fence the claim on invitation validity and the reset on a non-sending status.
iscekic
changed the title
pandemicsyn
approved these changes
Aug 18, 2026
pandemicsyn
left a comment
pandemicsyn
left a comment
Contributor
There was a problem hiding this comment.
lgtm, pair of minor ones inline.
… and refuse on a failed outbox read Two review findings on PR #5324. Security config: the CAS save committed the enabled config and the queued backlog rows, then wrote `auto_analysis_enabled_at` after the commit. A worker that claimed a queued row in that gap saw a null boundary and skipped the finding, and a failed post-commit write left the backlog with no boundary at all. The boundary write now runs inside the same transaction, so both helpers take an optional `tx`. Mobile outbox: a failed `listOutboxRows` read returned an empty list, which the session creators read as "no stored row" and used to mint a fresh operation key, so an accepted-but-unreadable row could produce a duplicate Cloud Agent session. The read now returns null, `whenLoaded` reports the failure and re-reads on the next attempt, and all three producers refuse the mutation instead of minting a key.
Review finding on the previous commit: `markLoaded()` ran outside the generation fence, so a load superseded by an identity change released the waiters against stale rows and a submit could mint a duplicate session key. `runLoad` now owns the marking: a superseded load applies nothing and marks nothing, and the load that superseded it releases the waiters.
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.
Summary
The mobile app now reports state honestly, and Security Agent and Code Reviewer saves can no longer silently overwrite each other.
$0balance or a "Connect" prompt as if they were truth.A session that fails to start now surfaces a classified terminal error instead of an endless loading skeleton. The class is derived from the session manager's formatted message text (
not-found,permission,transient, orunknown), and only the transient class offers Retry; every terminal error offers Copy and Back. The session detail route adds a permanent NOT_FOUND/UNAUTHORIZED state and retries a transient NOT_FOUND only on the spawned route at a flat 1s cadence.Files
apps/mobile/src/components/agents/session-terminal-error.ts— new; classifies a terminal error from the message text and resolves the error a session must surface, with Copy always offered.apps/mobile/src/components/agents/session-detail-content.tsx— renders the terminal error with Copy, Back to sessions, and Retry only for the transient class.apps/mobile/src/app/(app)/agent-chat/[session-id].tsx— renders a permanent NOT_FOUND/UNAUTHORIZED state (no Retry) with Copy and Back, and retries a transient NOT_FOUND on thespawned=1route only.apps/mobile/src/components/agents/mobile-session-manager.ts— addsreadFetchSessionErrorCode,isCloudPrepareRetryableError, andfetchSessionWithNotFoundRetryat a flat 1s cadence.Leaving the new-session screen with a non-empty prompt now asks to discard the draft on every exit path. A single React Navigation
beforeRemovelistener covers header back, Android back, and the iOS swipe gesture, and the caller's discard runs before the captured navigation action is replayed. A committed remote spawn that settles without navigating re-arms the guard so an abandon after a failed spawn still confirms.Files
apps/mobile/src/app/(app)/agent-chat/use-new-session-discard-guard.ts— new; registers thebeforeRemovelistener and the Discard/Keep editing alert.apps/mobile/src/app/(app)/agent-chat/new.tsx— wires the guard, the durable draft, the discard bypass on a successful Start/spawn, and the discard handler.apps/mobile/src/lib/use-new-session-share-remote.ts— threadsonSpawnAdmitted/onSpawnFailedcallbacks.apps/mobile/src/components/agents/use-remote-spawn-dispatch.ts— firesonSpawnAdmittedat admission andonSpawnFailedwhen a spawn settles without navigating.The new-session screen now shows the effective environment profile and a Leave changes / Commit and push choice. The effective profile resolves from a new read-only
agentProfilesrouter (listfor personal,listCombinedfor org), and the create body sendsautoCommitplus an optionalprofileId. Start is blocked only while the profile is still loading; a failed profile never blocks Start.Files
apps/mobile/src/components/agents/use-effective-agent-profile.ts— new; resolves the effective default profile fromlistorlistCombined.apps/mobile/src/components/agents/new-session-configure-form.tsx— renders the read-only profile row and the commit segmented control.apps/mobile/src/lib/new-session-submit.ts— addsresolveNewSessionStartDisabled, which blocks Start only while the profile is loading.packages/trpc/src/agent-profiles-mobile.ts— new; read-onlylist/listCombined/getrouter.packages/trpc/src/mobile.ts— mountsagentProfilesMobileRouterin place of the full web router.A persisted mutation outbox now survives relaunch so a crash mid-session-create reuses the same operation key instead of minting a duplicate session. Rows are account-scoped under
outbox:<userId>with two taxonomies:safe-retry(idempotent under itsoperationKey) andreconcile-first(must reconcile before re-submit). The session creators write asafe-retryrow beforeprepareSessionand reuse the stored key on relaunch.Files
apps/mobile/src/lib/persist/mutation-outbox.ts— new; write/load/remove/list over the encrypted KV store, epoch-fenced and serialized.apps/mobile/src/lib/persist/use-mutation-outbox.ts— new; exposesgetStoredOperationKey,writeSafeRetry,writeReconcileFirst,needsReconcile, andwhenLoaded.apps/mobile/src/components/agents/use-new-session-creator.ts— sendsautoCommitand an optionalprofileId, writes asafe-retryrow before the mutate, and reuses the stored key.apps/mobile/src/components/agents/use-continue-cloud-create.ts— writes asafe-retryrow before the mutate and reuses the stored key on the continue path.The home screen now shows a retryable "Couldn't load active sessions" error instead of the first-use promo when the active-sessions poll fails before any data loads.
Files
apps/mobile/src/components/home/home-screen.tsx— renders theactiveIsErrorstate as a retryable error.Security Agent config saves are now compare-and-set on a
config_revisioncolumn, so two tabs saving at once can no longer silently overwrite each other. A nullexpectedRevisionis a first insert; a non-null value updates only while the stored revision matches, otherwise the save conflicts with a clear message. Enabling now refuses an empty effective repository set, and the CAS write, the activation boundary, and the include-existing enqueue all commit in one transaction.Files
packages/db/src/schema.ts— addsagent_configs.config_revision(integer, default 1,>= 1check) andsecurity_analysis_queue.admitted_config_revision(nullable integer).apps/web/src/lib/security-agent/db/security-config.ts— addswriteSecurityAgentConfigWithRevisionandsaveSecurityAgentConfigWithRevision, which commit the CAS write, boundary timestamp, and enqueues in one transaction.apps/web/src/lib/security-agent/router/shared-handlers.ts— threadsexpectedRevisionthroughsaveConfig, returnsconfigRevisionfromgetConfig, and refusessetEnabledwith an empty effective repo set.apps/web/src/lib/security-agent/core/schemas.ts— addsexpectedRevisiontoSaveSecurityConfigInputSchema.apps/web/src/lib/security-agent/db/security-analysis.ts— accepts an optionaltxandadmittedConfigRevisiononenqueueBacklogFindings.apps/web/src/components/security-agent/SecurityAgentContext.tsx— sendsexpectedRevisionand addsrefetchConfigOnConflictError.apps/mobile/src/lib/security-agent.ts— adds theFlattenedSecurityAgentConfighelper type.apps/mobile/src/components/security-agent/analysis-settings-screen.tsx— switches its config ref to the flattened type.apps/mobile/src/components/security-agent/automation-settings-screen.tsx— switches its config ref to the flattened type.apps/mobile/src/components/security-agent/repository-settings-screen.tsx— switches its config ref to the flattened type and opts out of the disabled-state redirect.apps/mobile/src/components/security-agent/settings-overview-screen.tsx— disables the enable switch without an effective repo set and offers a repo-picker CTA.apps/mobile/src/lib/hooks/use-settings-back-guard.ts— addsskipRedirectso the repositories screen stays reachable while disabled.apps/mobile/src/lib/hooks/use-security-agent-mutations.ts— sendsexpectedRevisionfrom the cached config and writes areconcile-firstrow before a sync POST.A failed finding dismissal now keeps its reason and comment and surfaces a retry card instead of losing the work. The dismiss draft persists per finding with
lastErrorandretryable, and the dashboard and finding detail render one retry card per failed dismissal.Files
apps/mobile/src/lib/hooks/use-security-dismiss-draft.ts— new; persists a per-finding dismiss draft and lists failed dismissals per scope.apps/mobile/src/components/security-agent/dismiss-finding-screen.tsx— restores the draft, records the intent before submit, and writes the failure on error.apps/mobile/src/components/security-agent/finding-detail-screen.tsx— renders the retry card for a failed dismissal.apps/mobile/src/components/security-agent/dashboard-screen.tsx— renders retry cards for failed dismissals and reconcile cards for crashed syncs.apps/mobile/src/components/security-agent/security-command-retry-card.tsx— new; Retry/Discard card for a failed command.apps/mobile/src/lib/persist/drafts.ts— addssecurityDismissDraftKey.Security analysis admission and rendering are tightened so a stale result is never read as current. The analysis start always sends
forceSandbox, and the analysis panel gates triage and sandbox evidence on a terminal state so a stale result left by a failed retry does not read as "Analyzed".Files
apps/mobile/src/lib/hooks/use-security-findings.ts— sendsforceSandbox: trueonstartAnalysis.apps/mobile/src/components/security-agent/finding-analysis-panel.tsx— gates triage and sandbox evidence on a terminal analysis state.Connectivity is now classified three ways —
online,offline, andunknown— so a not-yet-settled NetInfo boot is never read as live data. The offline banner startsunknownand delays 5s, the connection lifecycle resumes only on a real offline→online recovery, and paused cold-launch queries stay loading instead of showing$0or a Connect prompt. The query client also stops retrying permanent error codes and drops permission-denied queries from the persisted cache.Files
apps/mobile/src/lib/connectivity-online.ts— three-way classification; a null reachability is never online.apps/mobile/src/lib/offline-banner-state.ts— startsunknownand delays the offline banner 5s.apps/mobile/src/lib/user-web-connection-lifecycle.ts— resumes only on a real offline→online recovery.apps/mobile/src/components/profile-credits-card.tsx— treats "no balance data yet" as loading, not$0.apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx— usesisPendingso a paused cold launch stays loading.apps/mobile/src/lib/code-reviewer-status.ts— keeps "no data yet" loading.apps/mobile/src/lib/persist/cache-persistence-mount.tsx— setsshouldDehydrateMutation: () => false.apps/mobile/src/lib/query-client.ts— addsPERMANENT_CODES(no retry) and drops permission-denied queries from the cache.The Code Reviewer overview now shows an action-required banner when it is disabled for a fixable reason, and the detail screen leads with the outcome. The action-required state and its copy move to a shared package so web and mobile read one source, and the retrigger reset becomes a status compare-and-set so a concurrent retrigger cannot dispatch twice.
Files
packages/app-shared/src/code-reviews/action-required.ts— new; shared state type, six reasons, and title/description/recovery copy.packages/app-shared/src/code-reviews/index.ts— new; exports the module.packages/app-shared/package.json— adds the./code-reviewsexport.apps/web/src/lib/code-reviews/action-required-shared.ts— re-exports the shared state and keeps the web-only copy fields.apps/mobile/src/components/code-reviewer/platform-overview-screen.tsx— renders the action-required banner.apps/mobile/src/lib/code-reviewer-config.ts— addsactionRequiredtoReviewConfigData.apps/mobile/src/lib/hooks/use-code-reviewer.ts— drops the stale org-only comment.apps/mobile/src/components/code-reviewer/review-detail-helpers.ts— new; flattens council findings and labels decisions/votes.apps/mobile/src/components/code-reviewer/review-detail-screen.tsx— renders an outcome-first detail.apps/mobile/src/components/code-reviewer/review-detail-sections.tsx— new; council, gate, and finding sections.apps/web/src/lib/code-reviews/db/code-reviews.ts— makesresetCodeReviewForRetrya status CAS returning a count.apps/web/src/routers/code-reviews/code-reviews-router.ts— throwsCONFLICTwhen the reset matches zero rows.Organization invite emails now go through a durable outbox instead of being sent inline, so the invite mutation returns immediately and a failed email is resendable. The outbox table holds one row per invitation with
pending/sending/delivered/failedstates and claim-fenced delivery marks, and a cron route drains it every minute. The member list surfaces the email status and offers Resend for failed invites.Files
packages/db/src/external-side-effect-outbox.ts— new; enqueue/claim/mark/reclaim/reset helpers, fenced on the claim token.packages/db/package.json— exports the module.apps/web/src/lib/organizations/dispatch-invite-email-outbox.ts— new; drains the outbox in one cron pass.apps/web/src/app/api/cron/dispatch-invite-email-outbox/route.ts— new; authenticated cron route.apps/web/vercel.json— schedules the cron every minute.apps/web/src/routers/organizations/organization-members-router.ts— enqueues instead of sending inline, marks the row failed ondeleteInvite, and addsresendInvite.apps/web/src/lib/organizations/organizations.ts— joinsemailStatusinto the member list and accepts an optionaltxon the role/remove/invite helpers.apps/web/src/lib/organizations/organization-types.ts— addsemailStatus.apps/web/src/lib/user/index.ts— deletes pending/sending outbox rows insoftDeleteUser.apps/web/src/components/organizations/OrganizationMembersCard.tsx— renders the email-status badge and a Resend button.apps/web/src/components/organizations/members/InviteMemberDialog.tsx— says "Invite created".apps/web/src/components/organizations/members/invited-email-status.ts— new; maps the status to a badge label.apps/mobile/src/components/organization/invite-member-sheet.tsx— says "Invite created".apps/mobile/src/components/organization/invited-member-row-state.ts— new; label, resend eligibility, and action options.apps/mobile/src/components/organization/invited-member-row.tsx— renders the status and offers Resend.apps/storybook/src/mockData/organizations.ts— records theemailStatusfield.ENVIRONMENT.md— records the cron path.apps/mobile/.gitignore— ignores the generatedsentry.options.json.Tests: 46 files changed (19 new, 27 modified) across mobile mounted/unit tests, web db/router/component tests, and the shared action-required test.
Generated: 5 files — 2 migrations (
0217_fresh_boom_boom.sql,0218_violet_whizzer.sql), 2 snapshots, and the migration journal.Verification
Visual Changes
The mobile screens now render honest state. After states from the iOS E2E run:
Reviewer Notes
pnpm drizzle generate; do not hand-edit them.Human steps
No human step is needed. Migrations auto-apply on deploy (
NODE_ENV=production pnpm run drizzle migrate); the cron path is read fromvercel.jsonon deploy; no new secret (reusesCRON_SECRET).