Skip to content

fix(mobile): localize the Security Agent enable error in every locale - #9443

Merged
iscekic merged 5 commits into
mainfrom
drain/localize-security-agent-enable-error
Nov 1, 2026
Merged

iscekic merged 5 commits into
mainfrom
drain/localize-security-agent-enable-error

Conversation

@iscekic

@iscekic iscekic commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — on the Android emulator against the local stack at this head, with the app in Portuguese (Brazil) and a fixture account whose GitHub connection lacks the Dependabot permission, the enable switch now refuses with app-catalog copy; the merge base bundle on the same device, account and language still showed the server's English sentence with the raw permission key.

Change

Enabling the Security Agent is refused by the server for two reasons, and both carried only an English sentence, which the mobile write path toasted as it arrived: every locale showed untranslated copy, and the first also exposed the raw vulnerability_alerts key.

Server:

  • The enable refusal for a missing Dependabot (vulnerability alerts) permission now carries the same stable precondition code the manual-sync path already uses (shared-handlers.ts setEnabled), so a client can map it. The English message stays for the web surface.
  • The enable refusal for an empty effective repository scope carries the sync path's no-repositories code, so the device can point at the repository picker instead of a generic sentence.
  • The config compare-and-set refusal gets its own code (SECURITY_CONFIG_CONFLICT_CODE), attached in db/security-config.ts for both the first-insert race and the stale-revision update.

Mobile:

  • securityConfigMutationMessage in security-agent-ledger.ts resolves a marked refusal to catalog copy: the missing permission reads the same re-authorize sentence the scope screen shows, the empty scope reads the repository picker's "select at least one repository" line, a stale config reads the new conflict line, and anything unmarked reads the existing localized generic sentence instead of server prose.
  • Both config-write onErrors in use-security-agent-mutations.ts (the enable toggle and the settings save) use it.

Copy: the two refusal cases reuse keys that are already translated in every catalog — securityAgent.scopeEntry.reauthorizeDescription for the permission and securityAgent.repositories.selectAtLeastOne for the empty scope — so the toast cannot drift from the screens that own the wording. The one new key, securityAgent.mutations.configConflict, is added to en.json and translated in all 86 other catalogs in this change, so no translation slice is needed.

Verification

  • Web: npx jest src/lib/security-agent/router/shared-handlers.test.ts src/lib/security-agent/db/security-config.test.ts — 111 tests pass, including checks that the permission refusal carries security_sync_missing_permission, that the empty-scope refusal carries security_sync_no_repositories on both the personal and organization routers, that a disable still goes through without the permission, and that the stale-revision conflict carries security_config_conflict.
  • Mobile: npx vitest run --shard=3/8 in apps/mobile — 180 files, 3413 tests pass at this head.
  • Catalogs: node tools/i18n/check-catalogs.mjs reports 87 languages matching en.json, node tools/i18n/check-plurals.mjs --source-only passes, and a strict duplicate-key parse of all 87 catalogs is clean (the merge with main resolved one conflicting block in et.json, keeping main's Estonian register for the sibling sentences and this branch's new key).
  • Android end to end on this VM at this head: local stack (nextjs on 3000, Metro on 8081), the emulator dev client, app language Portuguese (Brazil), and a fixture account with a GitHub connection whose stored permissions omit vulnerability alerts — so the switch stays reachable while every enable is refused.
    • Permission refusal — before (merge-base JS bundle, same device, account and language): the toast read GitHub App does not have vulnerability_alerts permission. After (this head): O agente de segurança precisa de permissão para ler os alertas do Dependabot. Reautorize o app do GitHub para conceder essa permissão. The switch returns to off in both captures.
    • Empty scope — at this head, with the stored repository selection no longer resolving in the installation: the toast reads Selecione pelo menos um repositório.
    • HTTP shape confirmed on the same stack: the enable mutation answers 412 PRECONDITION_FAILED with data.upstreamCode = security_sync_missing_permission (and security_sync_no_repositories once the permission is present but the scope is empty) while error.message keeps the English sentence for web.
  • Design gate: 4 cells, 0 errors, 0 warnings. The merge-base run for the same link reports the same 0 errors, 0 warnings.

Production prerequisites

None.

Proxy answers

The owner's proxy gave these answers in the owner's place.

  1. Question: Enable toast copy: Which catalog copy should the enable toggle show when the GitHub App lacks the Dependabot permission?
    Answer: Reuse the scope screen reauthorize copy (Recommended)
    Confidence: 0.80 · Precedent: Speed and least work: 'do the least amount of work possible, with no drop in quality'. The brief asks for wording consistent with the scope screen, and this reuses the exact sentence that is already translated in every catalog.
  2. Question: Config save toast: How far should the same mapping go for the config-save toast (the file's other raw-server-text onError)?
    Answer: Save too: stable conflict code + new key (Recommended)
    Confidence: 0.65 · Precedent: Fix the class: 'if anything repeats the same mistake over and over again, fix the class'. i18n is important to the owner. A real conflict should tell the user to review and save again, not show a misleading generic message.

Enabling the agent without the Dependabot permission was refused with an
English sentence and no stable code, so a device could only show that
sentence. The refusal now carries the same precondition code the manual-sync
path uses; the English message stays for the web surface.

The config compare-and-set refusal (settings save and the enable toggle) gets
its own code for the same reason: another session or device wrote first, and a
device needs to say so in the reader's language.
The enable toggle and the settings save toasted the server's sentence as it
arrived, so every locale showed English plus the raw permission key. Both now
resolve a marked refusal to catalog copy: a missing Dependabot permission
reads the same re-authorize sentence the scope screen shows, a stale config
reads the new conflict line, and anything else reads the existing localized
generic sentence instead of server prose.

The conflict copy lands in all 87 catalogs with the English key.
@iscekic iscekic added drain Work item from the findings drain explorer:android Finding surfaced by the Android explorer labels Nov 1, 2026
@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Verified on the Android emulator against the local stack, with the app language set to Portuguese (Brazil) and a fixture account whose GitHub connection lacks the Dependabot permission.

  • Before this change (merge-base bundle): turning the Security Agent switch on showed GitHub App does not have vulnerability_alerts permission — English copy with the raw permission key on an otherwise Portuguese screen.
  • After this change: the same tap shows O agente de segurança precisa de permissão para ler os alertas do Dependabot. Reautorize o app do GitHub para conceder essa permissão. — the app's own re-authorize sentence, the same wording the scope screen uses, with no permission key.

Design gate for /security-agent/personal/settings: 4 cells, 0 errors, 0 warnings; the merge-base run of the same link is also 0 errors, 0 warnings.

Before: English server sentence with the raw permission key on the Portuguese settings screen

After: Portuguese re-authorize sentence in the toast on the same settings screen

Design gate contact sheet for the Security Agent settings screen across the standard cells

The lint rule for the mobile app requires a switch over a possibly undefined
value to name every case; two equality checks read the same and say the same.
@kilo-code-bot

kilo-code-bot Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change marks the empty-scope enable refusal with the existing security_sync_no_repositories code, renames the shared marker helper to securityAgentPreconditionError, and maps the code onto the already-translated repository-picker copy, all of which matches the surrounding precondition pattern and reads consistently with the handler guards.

Assumptions: [INFERENCE] security_sync_no_repositories is emitted from the setEnabled handler only in the input.isEnabled branch (apps/web/src/lib/security-agent/router/shared-handlers.ts:1271), so the manual-sync toast keeps its own securityAgent.mutations.syncNoRepositories key and the two mappings do not cross. The key securityAgent.repositories.selectAtLeastOne is present with a non-empty value in all 87 catalogs and is the string asserted by the new mobile test.

Files Reviewed (5 incremental files)
  • apps/web/src/lib/security-agent/router/shared-handlers.ts
  • apps/web/src/lib/security-agent/router/shared-handlers.test.ts
  • apps/mobile/src/lib/security-agent-ledger.ts
  • apps/mobile/src/lib/hooks/use-security-agent-mutations.test.ts
  • packages/app-shared/src/security-agent/sync-preconditions.ts
Previous Review Summary (commit c548b87)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit c548b87)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The change adds stable wire codes (security_sync_missing_permission, security_config_conflict) to the Security Agent config refusals and maps them onto catalog copy in the mobile config-write toasts; the code, tests, and all 87 catalogs are consistent with the existing sync-precondition pattern.

Assumptions: [INFERENCE] The security_config_conflict code reaching mobile end to end relies on the shared tRPC error formatter (packages/web-shared/src/lib/trpc/transport.ts:103), which is the same path the already-shipped sync codes use; only the missing-permission case was exercised end to end in the PR. [INFERENCE] Unmarked actionable refusals on the settings save (for example assertGitHubAutomationCanBeEnabled's "Agent access connections support Slack and Cloud Agent only") now read the localized generic terminal sentence instead of the server's specific English reason; this matches the PR's stated fallback design, so it is noted rather than raised as a finding.

Files Reviewed (97 files)
  • apps/web/src/lib/security-agent/db/security-config.ts
  • apps/web/src/lib/security-agent/db/security-config.test.ts
  • apps/web/src/lib/security-agent/router/shared-handlers.ts
  • apps/web/src/lib/security-agent/router/shared-handlers.test.ts
  • apps/mobile/src/lib/hooks/use-security-agent-mutations.ts
  • apps/mobile/src/lib/hooks/use-security-agent-mutations.test.ts
  • apps/mobile/src/lib/security-agent-ledger.ts
  • packages/app-shared/src/security-agent/config-conflict.ts
  • packages/app-shared/src/security-agent/index.ts
  • apps/mobile/src/i18n/locales/*.json (87 catalogs; key present at securityAgent.mutations.configConflict in every one and all parse as valid JSON)

Reviewed by deepseek-flash-latest · Input: 56.4K · Output: 9.9K · Cached: 757.4K

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added the merge:human Substantive functional risk requires human review. label Nov 1, 2026

@eshurakov eshurakov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot - Minor: the sibling enable refusal still loses its actionable text. setEnabled also throws PRECONDITION_FAILED for an empty effective repo scope ("Select at least one repository before enabling Security Agent.", apps/web/src/lib/security-agent/router/shared-handlers.ts:1271) with no stable code, so securityConfigMutationMessage falls through to the generic copy. Before this change the toast carried that sentence; after it, an unmarked PRECONDITION_FAILED classifies as terminal, so the user sees securityAgent.mutations.actionBlocked ("The problem needs fixing before you can retry.") with no hint to pick repositories. Consider giving that refusal its own marker, or reusing the existing noRepositories precondition code mapped to catalog copy.

The empty-scope refusal on enable carried no stable code, so the mobile
config-write toast fell back to the generic terminal sentence instead of
naming the fix. Mark it with the sync no-repositories code and read the
repository picker's own sentence in the config-write toast.
@iscekic iscekic added merge:blocked:insufficient-e2e Verification does not cover the change merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:human Substantive functional risk requires human review. and removed merge:human Substantive functional risk requires human review. merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head labels Nov 1, 2026
@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Fixed the minor remark in this review: the empty-scope enable refusal no longer falls through to the generic terminal sentence.

  • setEnabled now throws the existing security_sync_no_repositories code for the empty effective scope (shared-handlers.ts), so the client can name the fix.
  • configMutationCopyKeyFor maps that code to securityAgent.repositories.selectAtLeastOne, the sentence the repository picker itself shows, so the toast says "Select at least one repository." instead of the generic copy. It is already translated in all 87 catalogs.
  • Tests: shared-handlers.test.ts asserts the code on both the personal and organization routers (111 tests pass with security-config.test.ts); use-security-agent-mutations.test.ts asserts the picker sentence and that the server sentence is not shown (43 pass).
  • Android end to end: the same settings screen and the same tap showed the generic sentence before this commit and Selecione pelo menos um repositório. after it.

The remark's alternative, reusing the code but keeping the sync wording, was not taken because that sentence ends "then sync again", which does not fit an enable refusal. The other observation in the summary stays as designed: a refusal the server does not mark still reads the localized generic sentence, never the server's English prose.

@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Proof of the empty-scope enable refusal on the Android emulator against this VM's local stack, with the app in Portuguese (Brazil) and a fixture account whose stored repository selection no longer resolves in the GitHub installation.

Same screen, same tap of the "Agente de segurança" switch:

  • Before this commit: the toast read the generic terminal sentence Não foi possível concluir esta ação. É preciso corrigir o problema antes de tentar novamente.
  • After this commit: the toast reads Selecione pelo menos um repositório. — the repository picker's own sentence, no English server prose.

Both captures came from a uiautomator dump taken in the same command as the screenshot, and the server logged POST /api/trpc/securityAgent.setEnabled 412 for each tap. The design gate for the same link reports 4 cells, 0 errors, 0 warnings at this head, and the merge-base run of the same link reports the same.

Portuguese settings screen with the generic terminal toast before this change

Portuguese settings screen with the select-a-repository toast after this change

Design gate contact sheet for the Security Agent settings screen

@iscekic iscekic added merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. and removed merge:blocked:insufficient-e2e Verification does not cover the change merge:human Substantive functional risk requires human review. labels Nov 1, 2026
@iscekic
iscekic added this pull request to the merge queue Nov 1, 2026
@iscekic
iscekic removed this pull request from the merge queue due to a manual request Nov 1, 2026
…ty-agent-enable-error

# Conflicts:
#	apps/mobile/src/i18n/locales/et.json
@iscekic iscekic removed the merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. label Nov 1, 2026
@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Verified at this head on the Android emulator against the local stack, with the app language set to Portuguese (Brazil) and a fixture account whose GitHub connection has no Dependabot (vulnerability alerts) permission.

  • Before, merge-base bundle on the same device, account and language: turning the Security Agent switch on showed the server's English sentence with the raw permission key in the toast.
  • After, this head: the same tap shows the Portuguese re-authorize sentence the scope screen uses, and the switch returns to off.
  • After, this head, with the stored repository selection no longer resolving in the installation: the same switch shows the repository picker's "select at least one repository" sentence.
  • Design gate contact sheet for the Security Agent settings screen at this head: 4 cells, 0 errors, 0 warnings.

Merge-base bundle: English sentence in the toast

This head: Portuguese permission sentence in the toast

This head: Portuguese empty-scope sentence in the toast

Design gate contact sheet

@iscekic iscekic added the merge:human Substantive functional risk requires human review. label Nov 1, 2026
@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: The red mobile-test (3/8) and test checks on this head were a timing flake in an existing suite, not a defect in this change. Both were re-run and pass on this head (mobile-test (3/8) 2026-11-01T12:18Z, test 2026-11-01T12:19Z), and every check on the head is now green.

Cause: apps/mobile/src/lib/hooks/use-security-agent-mutations.offline.mounted.test.tsx snapshots the sync error during render (holder.syncError = sync.error instanceof Error ? sync.error.message : null) and asserts on it right after the act scope. When React Query's observer notification lands after that scope flushes, the snapshot still holds null, which is exactly the observed failure (expected null to be 'Could not complete this action…', with securityAgent.triggerSync already called once and the pending flag already false). Neither that file nor the manual-sync path it exercises is touched by this branch.

Evidence on this VM at head 8e9dfd8: the file passes 10 runs out of 10 on its own, and npx vitest run --shard=3/8 passes in 5 consecutive full-shard runs.

@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Correction to the previous comment's count: the full-shard evidence at head 8e9dfd8 is 3 consecutive npx vitest run --shard=3/8 passes (180 files, 3413 tests each), not 5. Two earlier local shard runs on this VM failed to resolve the unbuilt @kilocode/harness-sdk workspace package, so they say nothing either way about this suite and are not counted. The single-file evidence stands: the mounted file passes 10 runs out of 10 alone.

@iscekic
iscekic added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 5e63f9a Nov 1, 2026
104 of 106 checks passed
@iscekic
iscekic deleted the drain/localize-security-agent-enable-error branch November 1, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain explorer:android Finding surfaced by the Android explorer merge:human Substantive functional risk requires human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants