Skip to content

fix(electron): use the shared error-category table instead of a second one - #779

Merged
sanchitmonga22 merged 1 commit into
RunanywhereAI:mainfrom
ayaangazali:fix/electron-shared-error-category
Sep 11, 2026
Merged

sanchitmonga22 merged 1 commit into
RunanywhereAI:mainfrom
ayaangazali:fix/electron-shared-error-category

Conversation

@ayaangazali

@ayaangazali ayaangazali commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Description

What is wrong

bindings/proto-ts/src/convenience/errors_category.ts opens by naming its own scope:

Canonical ErrorCode / rac_result_tErrorCategory range table for every TypeScript SDK (Web, React Native, Electron).
[...] Do not invent a second table in an SDK.

bindings/electron/src/errors.ts:70 invents a second table. Web and React Native both import { categoryForCode } from '@runanywhere/proto-ts/convenience/errors_category'; Electron declares its own, described as a port of the commons range table.

The same file already argues against this one level up, about the enums:

ErrorCode, ErrorCategory, and ErrorSeverity are the GENERATED proto enums from idl/errors.proto, re-exported rather than re-declared [...] A local subset would silently collapse the 115 commons codes it did not list, which is exactly the bug this file used to have.

The category table then does the collapsing instead.

The divergence

Evaluating both tables over codes 0..1100, they disagree on 560 of them:

codes shared table Electron
330-349 AUTH INTERNAL
350-369 IO (archive/extraction) INTERNAL
370-379 VALIDATION (calibration) INTERNAL
400-499 COMPONENT (module/service) INTERNAL
500-599 CONFIGURATION (platform/adapter) INTERNAL
600-699 COMPONENT (backend/runtime) INTERNAL
1-99, 390-399, 1000+ UNSPECIFIED INTERNAL

So an Electron consumer branching on e.category sees INTERNAL where the identical code gives AUTH or CONFIGURATION on Web and React Native.

This is not the commons table being wrong, and the shared file says so explicitly:

C++ commons [...] currently only maps |100|–|329| and falls through to INTERNAL — that is a known under-mapping, deliberately deferred [...] Do NOT "fix" this TS table down to match commons.

Electron's copy is that deferred under-mapping, re-introduced in the SDK layer.

It is reachable: the local table is the fallback for when the wire supplies no category, and proto3 leaves an unset enum at 0, which fromSerializedSdkError treats as absent.

What this changes

Import and re-export the shared categoryForCode, delete the local copy. Six insertions, twenty-eight deletions, no new code. SDKException's two call sites are untouched.

Verification

Ran the codegen (generate_ts.sh, generate_ts_convenience.py, generate_defaults_pool.py, generate_streams.sh), built proto-ts, then built and tested the Electron package:

electron build: rc=0, 0 TypeScript errors
npm test:  tests 241  pass 241  fail 0

Both existing assertions in test/unit/errors.test.ts still pass unchanged, including categoryForCode maps ranges like the canonical table. That is not luck: all eight codes it pins (0, 100, 110, 130, 182, 259, 380, 804) fall in bands where the two tables already agreed, which is exactly why the divergence went unnoticed.

The behaviour change, from the built package:

code 340 -> AUTH           (was INTERNAL)
code 355 -> IO             (was INTERNAL)
code 372 -> VALIDATION     (was INTERNAL)
code 450 -> COMPONENT      (was INTERNAL)
code 520 -> CONFIGURATION  (was INTERNAL)
code 640 -> COMPONENT      (was INTERNAL)

Summary by CodeRabbit

  • Refactor
    • Standardized error categorization to use the shared implementation, ensuring consistent fallback behavior across the application.
  • Documentation
    • Clarified which error-categorization behavior serves as the authoritative fallback when no more specific classification is available.

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Refactoring

Testing

  • Lint passes locally — bindings/electron has no lint script; npm run build (tsc, 0 errors) is the equivalent gate and passed
  • Added/updated tests for changes — none added on purpose. test/unit/errors.test.ts already pins this function and passes unchanged; per AGENTS.md I do not add tests unless asked

Ran locally in bindings/electron, after the four IDL codegen steps and a proto-ts build:

npm run build   ->  rc=0, 0 TypeScript errors
npm test        ->  tests 241   pass 241   fail 0

Platform-Specific Testing

None of the template's platform sections apply: this changes bindings/electron, which the matrix does not cover (Electron is also missing from the Labels list below).

Labels

None of the listed labels matches. This is bindings/electron, and there is no Electron SDK label in the repo today (ios-sdk, kotlin-sdk, web-sdk, flutter-sdk, rn-sdk, core). Closest by blast radius is none of them, since no other binding is touched. Happy to relabel if you have a preference.

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed) — not needed; the shared table already documents itself as canonical for Electron

Screenshots

Not applicable, no UI surface.

Copilot AI lite review requested due to automatic review settings August 24, 2026 06:09

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ad8c153f-00ab-4279-b9b3-105c3565a37c

📥 Commits

Reviewing files that changed from the base of the PR and between 488cf27 and ecc6149.

📒 Files selected for processing (1)
  • bindings/electron/src/errors.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The Electron error bindings now re-export categoryForCode from the canonical proto convenience module. The local error-code mapping was removed, and the documentation now identifies the shared implementation as authoritative.

Changes

Error category mapping

Layer / File(s) Summary
Canonical category export
bindings/electron/src/errors.ts
The module imports and re-exports the canonical categoryForCode, removes the local mapping, and updates the related documentation.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to ecc61

The shared error-category mapping is now used consistently by Electron, with no unresolved merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: Electron now uses the shared error-category table instead of a duplicate local table.
Description check ✅ Passed The description follows the repository template and provides a clear problem statement, scope, rationale, testing results, platform-testing applicability, label limitations, and checklist status. It a…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ayaangazali

Copy link
Copy Markdown
Contributor Author

This one is not mine, so per the rule I am replying with evidence rather than pushing a fix.

The failing step is "Swift distribution repo (runanywhere-swift) is cut at this release", i.e. scripts/validation/gates/check_swift_dist_repo_sync.sh:

[FAIL] runanywhere-swift has no 0.20.26 tag (latest: 0.20.25)
       v0.20.26 is published here, so 'from: "0.20.26"' is broken for every
       SwiftPM consumer of https://github.com/RunanywhereAI/runanywhere-swift.git

It fails identically on a clean checkout of the default branch with none of my changes:

$ git checkout --detach origin/main      # e6826fb2f
$ bash scripts/validation/gates/check_swift_dist_repo_sync.sh
[FAIL] runanywhere-swift has no 0.20.26 tag (latest: 0.20.25)
...
[FAIL] runanywhere-swift distribution sync: 1 problem(s)
rc=1

The timing is what decides it, not the diff. chore: release 0.20.26 (#774) landed at 2026-08-24 01:49 UTC. PR runs on either side of that:

PR centralization started result
#775 2026-08-24 00:13 UTC pass (predates the release)
#779 2026-08-24 06:09 UTC fail (postdates it)

pull_request runs resolve against the merge with main, so #779 is simply the first of my PRs to run CI after 0.20.26 existed. The others will show the same on their next run.

The repo already documents this as expected. release.yml:36:

scripts/validation/gates/check_swift_dist_repo_sync.sh fails every PR once v1.2.3 exists here until that repo carries the 1.2.3 tag. That gate is deliberately NOT in this workflow: it runs on the tag push, before the split repo can legitimately be cut.

and sync-versions.sh:616 prints the same warning at bump time.

For completeness, my diff is a single TypeScript file, bindings/electron/src/errors.ts (+6/-28). It touches no version, no manifest, no Swift file, and nothing the gate reads.

Clearing it is the maintainer step the gate prints: cut the 0.20.26 tag on runanywhere-swift. Nothing in this PR can or should change it. Happy to rebase once that lands if you want a green run on the record.

@ayaangazali

Copy link
Copy Markdown
Contributor Author

Also restructured the description onto .github/pull_request_template.md, which CodeRabbit was right to flag. I had been writing prose-only bodies and skipping the template's sections; that is my miss, not a tooling nit.

Filled in truthfully rather than ticking boxes:

  • bindings/electron has no lint script, so I could not check "Lint passes locally". npm run build (tsc, 0 errors) is the equivalent gate and is recorded instead.
  • No tests added on purpose. test/unit/errors.test.ts already covers categoryForCode and passes unchanged, and AGENTS.md says not to add tests unless asked.
  • Every platform-testing section and every label in the template is left unticked because none of them covers Electron. The matrix has Swift, Kotlin, Flutter, React Native and Web; the label list has those five plus Commons. bindings/electron ships from this repo, has its own electron-unit job in pr-build.yml and its own packages in release.yml, but a contributor touching it has no row to tick and no label to pick. The repo's label set matches (ios-sdk, kotlin-sdk, web-sdk, flutter-sdk, rn-sdk, core, no Electron one).

Not something to fix inside this PR, and I am not going to open an issue for it unprompted. Flagging it here since it is the direct cause of two of the unticked sections above, and say the word if you would like a separate docs PR adding the Electron rows.

@ayaangazali
ayaangazali force-pushed the fix/electron-shared-error-category branch from dae49b8 to 854971b Compare August 24, 2026 18:41
@ayaangazali

Copy link
Copy Markdown
Contributor Author

Update: this clears itself now, and I have rebased to trigger it.

When I looked at this the first time, check_swift_dist_repo_sync.sh was failing on main too, because v0.20.26 existed here while runanywhere-swift was still tagged 0.20.25. That has since been resolved upstream: main is now 0.20.27 (0b95af9ae), and the dist repo carries the matching tag.

$ git ls-remote --tags .../runanywhere-swift.git | tail -3
0.20.24
0.20.25
0.20.27

$ git checkout --detach origin/main      # 0b95af9ae, core/VERSION 0.20.27
$ bash scripts/validation/gates/check_swift_dist_repo_sync.sh
[OK] runanywhere-swift is released at 0.20.27, matching monorepo v0.20.27
[OK] runanywhere-swift distribution sync: 0.20.27
rc=0

So the red on this PR was a stale result: that centralization job started at 06:09 UTC, when 0.20.26 was current and the dist tag had not been cut. Nothing about the run had anything to do with this diff.

Rebased onto 0b95af9ae and pushed as 854971b8a. The diff is byte-identical, still one file, +6/-28. The fresh run should show centralization green; I will confirm once it lands.

@ayaangazali

Copy link
Copy Markdown
Contributor Author

Still reproduces on today's main (c11f78ef4), and it is worse than the description says. I framed this as removing a duplicate table. The two tables have already diverged, so this is a live mis-categorisation rather than a tidy-up.

I parsed the range rules out of both functions in source order and compared them across codes 1..999. The local copy has 9 rules, the shared one has 21, and they disagree on 350 codes:

330-349   electron INTERNAL   shared AUTH
350-369   electron INTERNAL   shared IO
370-379   electron INTERNAL   shared VALIDATION
400-499   electron INTERNAL   shared COMPONENT
500-599   electron INTERNAL   shared CONFIGURATION
600-699   electron INTERNAL   shared COMPONENT

Those are not hypothetical ranges. Cross-referencing idl/errors.proto, 28 real error codes land in them:

330 ERROR_CODE_KEYCHAIN_ERROR          electron INTERNAL, should be AUTH
351 ERROR_CODE_CHECKSUM_MISMATCH       electron INTERNAL, should be IO
370 ERROR_CODE_CALIBRATION_FAILED      electron INTERNAL, should be VALIDATION
400 ERROR_CODE_MODULE_NOT_FOUND        electron INTERNAL, should be COMPONENT
500 ERROR_CODE_ADAPTER_NOT_SET         electron INTERNAL, should be CONFIGURATION
600 ERROR_CODE_BACKEND_NOT_FOUND       electron INTERNAL, should be COMPONENT

So an Electron caller branching on category sees a keychain failure and a missing backend both reported as INTERNAL, while every other binding categorises them correctly through the shared table. That is exactly the drift a second copy invites, and it has already happened.

Branch merges clean into current main, CI green, and the file is not superseded (still +6/-28 against main). Nothing pushed, since a rebase here would only move the PR across the v0.20.31 release boundary and pick up the unrelated centralization dist-repo red.

…d one

errors.ts declared its own categoryForCode, a port of the narrower commons
range table. proto-ts already exports the canonical one and names Electron as a
consumer: "Canonical ErrorCode / rac_result_t -> ErrorCategory range table for
every TypeScript SDK (Web, React Native, Electron) ... Do not invent a second
table in an SDK."

The two disagree on 560 codes in 0..1100. Electron collapsed the auth (330-349),
archive (350-369), calibration (370-379), module (400-499), platform (500-599)
and backend (600-699) bands to INTERNAL, so the same error carried a different
category on Electron than on Web and React Native.

Import the shared table and re-export it. This is the same rule the file already
applies one level up to the enums themselves: re-export, never re-declare.
@ayaangazali
ayaangazali force-pushed the fix/electron-shared-error-category branch from 854971b to ecc6149 Compare September 2, 2026 19:18
@ayaangazali

Copy link
Copy Markdown
Contributor Author

Still reproduces on 45e91276e (0.20.36), and the canonical file answers the "is this just deduplication?" question itself.

bindings/proto-ts/src/convenience/errors_category.ts opens with:

Canonical ErrorCode / rac_result_tErrorCategory range table for every TypeScript SDK (Web, React Native, Electron). ... Do not invent a second table in an SDK.

This table is the richer 18-range mapping historically shared byte-for-byte by Web and React Native. It deliberately covers bands beyond |329| ... so those defined proto codes keep meaningful categories instead of collapsing to INTERNAL.

C++ commons ... currently only maps |100|–|329| and falls through to INTERNAL — that is a known under-mapping, deliberately deferred. Do NOT "fix" this TS table down to match commons.

Electron's errors.ts is that second table, and it is the narrower one: it ports the |100|–|329| commons ranges verbatim, so everything above |329| falls through to INTERNAL.

That is not cosmetic, because those codes exist. Comparing the two tables band by band, Electron mis-categorizes every defined code from 330 up:

330 KEYCHAIN_ERROR          Electron INTERNAL   Web/RN AUTH
333 SECURE_STORAGE_FAILED   Electron INTERNAL   Web/RN AUTH
351 CHECKSUM_MISMATCH       Electron INTERNAL   Web/RN IO
370 CALIBRATION_FAILED      Electron INTERNAL   Web/RN VALIDATION
400 MODULE_NOT_FOUND        Electron INTERNAL   Web/RN COMPONENT

So the same failure reported through Electron and through Web gets a different category, and Electron's is the useless one. Anything routing or filtering on category (retry policy, user-facing messaging, telemetry grouping) sees a keychain failure as an internal error.

Worth noting what this does not change: the local table is fallback-only, consulted when the wire supplies no category. Commons does supply one on the paths that produce it, and those are unaffected. This matters for the paths where Electron constructs an SDKError locally from a bare code.

Merges clean, CI green.

@sanchitmonga22

Copy link
Copy Markdown
Contributor

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@sanchitmonga22

Copy link
Copy Markdown
Contributor

Replying to this comment

That would be great, thanks for flagging it -- the label set really doesn't have an Electron entry anywhere yet (only ios-sdk/kotlin-sdk/web-sdk/flutter-sdk/rn-sdk/core exist today). A separate docs PR adding Electron to the template's platform matrix and the label list would be welcome.

Reviewed with help from Claude Code and Codex.

@sanchitmonga22

Copy link
Copy Markdown
Contributor

Replying to this comment

That makes sense -- thanks for laying out the evidence. We read both range tables side by side and confirm the same 28-code divergence across 330-699; this is a real behavior fix, not cosmetic dedup, and it's why we're merging on that basis. Treating this as addressed.

Reviewed with help from Claude Code and Codex.

@sanchitmonga22

Copy link
Copy Markdown
Contributor

Replying to this comment

Confirmed on our end too -- bindings/proto-ts/src/convenience/errors_category.ts's own header names Electron as a consumer and calls commons' narrower mapping a known, deliberately deferred gap, not something to copy backwards. Merging now. Treating this as addressed.

Reviewed with help from Claude Code and Codex.

@sanchitmonga22 sanchitmonga22 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.

Thanks a lot for this, @ayaangazali! Electron's fallback categories for 28 real error codes (keychain -> AUTH, checksum -> IO, module/backend -> COMPONENT, adapter -> CONFIGURATION, and more) now match Web and React Native instead of collapsing to INTERNAL, and the duplicate table is gone.

Checked: CodeRabbit reviewed the latest commit · CI green · built and linted locally merged into main (electron, TS-only change) · two independent code reviews.

Follow-ups, not blocking: #880 (Electron's two native error paths, and the other SDKs, still disagree with each other and with this table for codes 330-699) -- you're welcome to pick it up if you're interested.

Merging now -- really appreciate the contribution, and thanks for the patient, well-evidenced follow-ups while this waited for a look!

Reviewed with help from Claude Code and Codex.

@sanchitmonga22
sanchitmonga22 merged commit e00dec7 into RunanywhereAI:main Sep 11, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants