fix(mobile,web): PR & reviewer safety audit batch (W1-A) - #4696
Merged
Conversation
…ct save patch Resolve the cross-slice type collision introduced when P0-B-13a added the field-merge PATCH endpoints: separate CodeReviewFieldMergePatch (org-only council/manually-added fields) from CodeReviewConfigPatch, which feeds the strict mobile saveReviewConfig mutation. Align RepositoryModelOverrideInput's thinkingEffort with its authoritative zod schema (.nullable().optional()) so the PATCH handlers can assign the parsed input without a type error. Extract pure, directly-testable helpers alongside the fix: - applyMergeSuccessEffects() from PrMergeSheet's post-merge branch - named cancel/retrigger/createManualReview mutationFns from use-code-reviews Green: typecheck, lint, format:check, check:unused; app-shared/mobile/web router unit tests pass.
The barrier refactor extracted use-code-reviews mutationFns and reworked the merge sheet, dropping test coverage the slice acceptance criteria require: - P1-B-12a: re-add hook-level wiring assertions for useCancelReview / useRetriggerReview / useCreateManualReview — onError calls toast.error(error.message) with the server's data.error and does NOT invalidate; onSuccess invalidates 2x (list+detail) for cancel/retrigger, 1x (list) for create. Kept the direct mutationFn throw/resolve tests. - P0-B-08: re-add a PrMergeSheet performSubmit test proving haptic + refetch + dismiss fire on clean and partial success (partial also writes the banner store), and none fire when the mutation rejects (merged:false). This also re-activates the src/components/pr-review/**/*.test.tsx vitest include. Test-only; no runtime change. Verified by inverting the gates (broken wiring fails the new assertions). Green: typecheck, lint, format:check, check:unused, mobile affected tests.
… data
P0-D-09: mergePullRequest no longer trusts client headRef/isCrossRepo to
decide which branch to delete. The server fetches the PR via octokit.pulls.get
and derives the authoritative head ref name, same-repo identity (by numeric
repo id), and head sha. The post-merge branch delete is fenced on:
merged && deleteBranch && sameRepo && headRef present && fetchedHeadSha ===
expectedHeadSha, and deletes only heads/<derived-ref>. A spoofed headRef can
no longer delete an arbitrary same-repo ref; cross-repo and sha-mismatch abort
the delete. The { merged, sha, branchDeleted, branchDeleteError? } result shape
is unchanged (P0-B-08 mobile gating preserved).
Backward-compat: headRef/isCrossRepo made .optional() (schema stays .strict());
already-shipped clients that send them are accepted and the fields ignored.
New mobile buildMergeInput stops sending them; isCrossRepo still drives the
local delete-branch toggle.
Tests: 5 new github-pr-review-router cases (spoofed headRef ignored, cross-repo
deletes nothing, sha-mismatch aborts, legacy fields accepted, new wire omits
them); existing merge tests updated to mock pulls.get. Green: typecheck, root
lint/format, web jest (19), mobile merge tests (59).
P1-F-46b: the review-submit affordance was only rendered by the Files-tab floating action bar and only when the pending-comment queue was non-empty, so Overview had no submit path and a clean PR (0 queued comments) could never be approved even though the submit screen already supports a clean approve. - Files: ungate the 'Finish review' button so it always renders (clean approve reachable); the numeric count badge only shows when the queue is non-empty. - Overview: add a 'Submit review' header-right action (tab === 'overview' only) that pushes the same review-submit route. Discussion tab is left unchanged. - Both affordances are >=44pt with accessibility labels. Tests: new reachability tests assert the submit affordance is present and navigates to the review-submit route on both Overview and Files with 0 and >0 pending items; inversion-checked (re-gating fails them). Green: typecheck, lint, format:check, 254 pr-review/lib tests.
P1-F-47a: the auto-fix review-comment webhook admitted fix requests with two local regexes (/@Kilo\\b/i + /\\b(fix|patch)\\b/i). The mention regex rejected the product-advertised '@kilocode-bot fix it' footer command (the \\b fails inside 'kilocode'), so the exact command the inline-comment footer tells users to use never triggered Auto Fix. - Add shared pure parser packages/app-shared/src/code-review/mention-command.ts exporting parseFixCommand(text): boolean, broadened to /@Kilo[\\w-]*/i so it admits @Kilo, @kilocode, and @kilocode-bot while still requiring a fix/patch keyword; exported from the code-review barrel. - The webhook processor consumes parseFixCommand instead of its local regexes (same reject path/log preserved). - Drift guard: default-prompt-template.json is unchanged; a new apps/web test reads the inlineCommentFooter literal and asserts parseFixCommand admits the advertised command, so footer/parser divergence fails the build. (Placed in apps/web, not app-shared, because the shared package cannot import the apps/web template JSON.) Tests: shared parser unit tests (advertised + shorthand + negatives); webhook processor delegation/admit-reject tests; drift-guard. Inversion-checked (narrowing the parser fails the advertised + drift-guard tests). Green: typecheck, root lint/format, app-shared (201), web suites (48, incl. generate-prompt unchanged).
P0-C-14: REVIEW_THREADS_QUERY and REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY
selected count/reactors/viewerHasReacted under reactions.nodes (the Reaction
type, which has none of those fields), so GitHub rejected the whole document
and review threads never loaded. Switch both queries to reactionGroups (one
group per ReactionContent) and realign GraphQlReactionNode / GraphQlCommentNode
and normalizeReactions to read reactors.totalCount.
The output DTO { content, count, viewerHasReacted } is byte-for-byte unchanged
(same order, no new filtering), so mappers.ts and the mobile reactions row are
unaffected. Schema-validity of all raw docs is proven in Wave 4 (P0-H-14).
Tests: new normalize-reactions.test.ts pins the DTO invariant against a
synthetic reactionGroups payload (count from reactors.totalCount, null->0,
order/one-per-content, viewerHasReacted passthrough); inversion-checked.
Green: typecheck, root lint/format, web github-pr-review (90).
P0-B-13b: useSaveReviewConfig sent a full-document saveReviewConfig built from
the cached ReviewConfigData, which erased council / manuallyAddedRepositories /
councilEnabledRepositoryIds — fields the mobile client never loads. Route the
save through the field-merge patchReviewConfig endpoints (P0-B-13a) with only
the edited fields, so unlisted fields are preserved server-side.
- Personal/org patch payload is { platform, ...editedFields } (+ organizationId
for org); personal selectedRepositoryIds/repositoryModelOverrides still
narrowed to numeric ids and only sent when the patch carries them (no empty-
array synthesis). GitLab autoConfigureWebhooks:true sent only when the patch
includes selectedRepositoryIds (matches server webhook-sync gating).
- chainSave FIFO, throw-on-!success, GitLab webhook warning, onMutate/onError/
onSettled all preserved.
- Removed the now-unused buildSaveConfigInput helper (+ its shared tests and the
mobile re-export); check:unused clean.
Tests: mobile unit test asserts the partial-patch shape (only edited fields,
not a full doc) + onError toast (inversion-checked); new web integration cases
in both patchReviewConfig suites seed council/manuallyAdded/councilEnabled and
drive a mobile-shaped patch through the real procedure, asserting they survive.
Green: typecheck, root lint/format, mobile hook test (10), app-shared (194),
web routers (72), check:unused.
Contributor
Author
|
(bot) On-device E2E status The user-facing device flows in this batch are deferred to manual verification (pre-agreed), because the local repro gate cannot reach PR Review — this environment has no GitHub-connected local account, so a real PR with review threads / a mergeable state cannot be opened on device. Each deferred flow is covered by an in-repo unit/integration gate:
The remaining slices are web-only (P1-D-32, P0-H-14, P0-C-14 server, P0-D-09 server, P1-F-47a) or require a hard-to-force |
![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 Executive SummarySince the previous review, the branch added an internal code-review-status callback route with structured terminal-reason classification, an AI-gateway client-disconnect vs. upstream-disconnect distinction (with Vercel request-id correlation), and an auto-routing sticky-accuracy tolerance band, plus a derived-from-runtime-array fix for the terminal-reason zod schema drift bug; all changed logic is consistent with its accompanying regression tests and no new issues were found. Files Reviewed (10 files)
Previous Review Summaries (4 snapshots, latest commit 3b5d1b8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3b5d1b8)Status: No Issues Found | Recommendation: Merge Executive SummarySince the previous review, the branch added a mobile-consumable field-merge PATCH endpoint (personal + org) for review-agent config and wired the web Files Reviewed (5 files)
Previous review (commit 9c30973)Status: No Issues Found | Recommendation: Merge Executive SummarySince the previous fix commit, the branch added a PR-conversation-comments feature, a merge-sheet UI refactor, and a council required-labels UI gate — all consistent with server-side caps and covered by regression tests; no new issues found. Files Reviewed (13 files)
Previous review (commit ac6a762)Status: No Issues Found | Recommendation: Merge Executive SummaryBoth previously flagged issues (inverted success check in Files Reviewed (4 files)
Previous review (commit ca3586d)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryAn inverted success check in Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (48 files)
Reviewed by claude-sonnet-5 · Input: 42 · Output: 11.1K · Cached: 972.8K Review guidance: REVIEW.md from base branch |
- use-code-reviews.ts: only throw when createManualReviewJob explicitly
returns {success: false}; the real success payload has no success field
so the previous check treated every successful creation as a failure.
- mention-command.ts: tighten MENTION_PATTERN so it matches @Kilo,
@kilocode, and @kilocode-bot but rejects unrelated @kilo-prefixed
handles such as @kilocorp and @kilogram.
- Update unit tests for both fixes.
Contributor
Author
|
(bot) Fixed both review findings in this worktree:
Both affected unit tests were updated and pass; mobile lint/typecheck and app-shared typecheck also pass. Pushed as ac6a762. |
Contributor
|
Got it — no action needed from me here, just noting the fix commit ac6a762 addressing both review findings (success-check inversion and MENTION_PATTERN over-match). |
Conflicts in apps/web PR-review code resolved to the shipped main behavior: - github-pr-review-router.ts: both sides migrated reactions->reactionGroups; keep main's version (GraphQlReactionGroup type, zero-count groups filtered, new CONVERSATION_COMMENTS_QUERY). - review-thread-comments.test.ts: keep main's live-shape reactionGroups fixture. - normalize-reactions.test.ts: re-pin the DTO invariant to the shipped zero-count-filtering contract (mobile renders a fixed 8-pill row and hides zero counts, so the consumer is unaffected). Auto-merge integration repair in apps/mobile: - pr-merge-sheet.test.tsx: pass new required sheetTitle/eyebrow props, stub PrFormSheetHeader (expo-router chain) and MergeSheetFormBody, drive the confirm handler via onConfirm, extend the react-native mock with Keyboard and useWindowDimensions.
The main merge added a tenth PR-Review document; enumerate it in PR_REVIEW_GRAPHQL_DOCUMENTS so the schema-validity test keeps its auto-coverage invariant, and bump the export-count guard 9 -> 10.
The merge-resolution verifier hit three reusable setup blockers: missing USER_GITHUB_APP_TOKEN_* keys in the worktree env, empty git-token-service .dev.vars token keys, and the iOS paste / Safari open prompts. Record symptom, cause, and fix for the next run.
Prettier and oxfmt disagree on line layout; the repository format gate uses oxfmt. Reformat the three conflict-resolved files so the root format-check passes. No semantic change; 137 web tests + typecheck green.
…ile-audit-w1-pr-safety Both sides appended describe blocks at the end of organization-code-reviews-router.test.ts; kept both (patchReviewConfig + skip bot pull requests). Semantic integration fix: #4765 added skip_bot_pull_requests to the code-reviewer config. The field-merge PATCH handlers (org + personal) neither read nor wrote it, and the jsonb preserve-list covers only the two feature flags — so any mobile PATCH save would silently reset a user's permit-bots setting to the default. Pass the stored value through in both handlers and pin preservation with skip_bot_pull_requests:false seeds in both patch test suites.
Contributor
Author
|
(bot) @kilo-code-bot please review the latest head d74ec5b — the branch was updated after the previous review was cancelled by a superseding push. |
…-audit-w1-pr-safety WORKFLOW_LEARNINGS.md: both sides appended Orchestrator entries; kept all three (PR-review E2E env traps + EXITCODE false-trigger + Kilobot no-findings reading). #4788's code-review error-code changes touch only the review-status route, disjoint from this PR's surface — behavior-neutral resolution, no reruns per the base-advance rule.
jeanduplessis
approved these changes
Jul 27, 2026
iscekic
enabled auto-merge (squash)
July 27, 2026 18:09
Conflicts: apps/mobile/vitest.config.ts (main split the mobile vitest run into pure + mounted projects) — kept main's project split and moved this branch's src/components/pr-review/**/*.test.tsx include into vitest.pure.config.ts; apps/mobile/.kilo/WORKFLOW_LEARNINGS.md — both sides appended sections, kept both.
…afety Conflict in apps/mobile/src/lib/hooks/use-code-reviews.ts: W1-B swapped `toast` for `announcingToast` inside the in-onSuccess domain-failure branch that this branch deletes (P1-B-12a moves domain failures into a throwing mutationFn so onError handles them). Kept the extracted mutationFn; the surviving onError handlers use announcingToast from main. The two hooks now import announcing-toast, which reaches react-native, so retargeted the sonner-native mock in use-code-reviews.test.ts and use-code-reviewer.test.ts at '@/lib/a11y/announcing-toast', matching use-session-mutations.test.ts on main.
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.
Mobile Audit W1-A — PR & reviewer safety
Fixes a batch of 10 audited defects across the PR-Review / Code-Reviewer surfaces (mobile + web + shared). Each slice ships with a regression test; the non-device defects each have a baseline-demonstrating test that fails on the pre-fix code.
What changed, in plain language
@kilocode-bot fix itcommand advertised in the UI now actually works — one shared parser recognizes it, with a guard test that fails if the advertised text and the parser ever drift apart.Slices
P0 (correctness / security)
mergedresult; a non-merge reply (e.g. 405 "not mergeable") surfaces as a retryable inline error instead of a false success.reactionGroups; the normalized reaction DTO invariant is pinned (one entry per content,countfromreactors.totalCount, source order; zero-count groups filtered, matching the shipped main behavior — the mobile reactions row renders a fixed 8-pill set and hides zero counts, so the filter is invisible to the app).headRef/isCrossRepoand remains backward compatible with old clients.@octokit/graphql-schema) and validates all 10 raw PR-Review documents (enumerated from a module export, so new docs are auto-covered) with a deliberately-broken teeth-guard.P1
{success:false}responses now throw a typed error carrying the server's reason, soonError/toast.errorruns and success haptics/navigation do not fire on failure.getGitLabStatus(org and personal). Rotation is admin-gated (owner/billing_manager for org, self for personal), returns the new secret once, and re-syncs Kilo-managed webhooks with the new secret so the integration keeps working. No mass/forced rotation.@kilocode/app-shared, consumed by the webhook processor, so the UI-advertised@kilocode-bot fix itcommand is admitted. A drift-guard test parses the literal template footer through the parser.Validation
pnpm typecheck,pnpm lint,pnpm format:check— all green.check:unusedclean.@kilocode/app-shared: 194 tests pass.E2E
Non-goals
Only these 10 slices. Excluded: mobile display / mass rotation of the GitLab secret,
use-code-reviewer.ts(already correct), PR-merge server re-architecture, and every other audit catalog item outside this batch.