Repository navigation
fix(native): retain Objective-C classes whose adopted-protocol list exceeds its count limit - #8958
SahinurDEV wants to merge 1 commit into
Conversation
…xceeds its count limit Fixes morluto#8945
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 [Medium impact] The change affects static Objective-C metadata results for binaries with oversized adopted-protocol lists. The behavior fix appears sound, but the new helper must follow the repository’s required error shape before merging. Findings
Fix with agent prompt### Issue 1
src/native/AppleObjcDispatchFacets.ts:286-293
`readAdoptedProtocols` reports a failed read through an `unreadable` boolean and a shared error list. This violates the repository directive to model expected failures with the tagged error algebra and `Result`.
Return `Result<string[], ...>` with a tagged error, then let `readClass` record the reason and mark the row partial. This repository requirement must be satisfied before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryRetains Objective-C class and metaclass rows when an adopted-protocol list cannot be read.
No security issue was established. Supersession could not be checked remotely because GitHub access was unavailable. No tests were run during this review. SahinurDEV explicitly excludes category protocol-list recovery from this PR’s scope. Reviews (1) · Last reviewed commit: "fix(native): retain Objective-C classes ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ): { protocols: string[]; unreadable: boolean } => { | ||
| try { | ||
| return { protocols: list(), unreadable: false }; | ||
| } catch (cause: unknown) { | ||
| failures.push( | ||
| `Protocol list of ${owner}: ${cause instanceof Error ? cause.message : String(cause)}`, | ||
| ); | ||
| return { protocols: [], unreadable: true }; |
There was a problem hiding this comment.
readAdoptedProtocols reports a failed read through an unreadable boolean and a shared error list. This violates the repository directive to model expected failures with the tagged error algebra and Result.
Return Result<string[], ...> with a tagged error, then let readClass record the reason and mark the row partial. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/native/AppleObjcDispatchFacets.ts
Line: 286-293
Comment:
**Failure uses two separate signals**
`readAdoptedProtocols` reports a failed read through an `unreadable` boolean and a shared error list. This violates the repository directive to model expected failures with the tagged error algebra and `Result`.
Return `Result<string[], ...>` with a tagged error, then let `readClass` record the reason and mark the row partial. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/morluto/rea/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Fixes #8945.
inspect-native-dispatch-metadatadropped a class'sobjc_classesrow (and never visited its metaclass) when the class's adopted-protocol list declared more than 20000 entries. The count guard in the protocol reader throws before the class row is appended, so the outer class-list catch only recorded the reason. Method implementations and ivars survived because they are appended earlier.After this change the class and its metaclass are retained, the class row is marked
partialwithprotocol_list_unreadable, itsprotocolsis empty, and the class coverage stayspartialwith the original limit reason.Problem and expected behavior
Change and scope
readClassnow reads the adopted-protocol list through a smallreadAdoptedProtocolshelper. When the list read throws, the helper recordsProtocol list of <class>: <reason>in the class failures and returns an empty list. The class row then getsprotocol_list_unreadablein its decode reasons, the same waymax_records_reachedandsuperclass_binding_unresolvedare reported.Not included: the 20000 limit is unchanged, category protocol lists (
decodeObjcCategories) keep their current behavior, and no new schema fields are added (decodereasonis already a free-form string).Contribution scope: an ordinary fix inside the existing static Mach-O reader; maintainer scope decision not applicable.
Contract and boundary impact
decodeObjcDispatchFacets(src/native/AppleObjcDispatchFacets.ts), the class row producer forinspect_native_dispatch_metadata.partial.docs/public/product-catalog.json), package, or installation impact: none.Expected effects:
Evidence and regression coverage
Tests added or updated:
src/native/AppleDispatchMetadata.test.ts, newObjective-C adopted protocol-list count limitblock, built on the same fixture changes as the issue's reproduction (root class, count at0x700):partial/protocol_list_unreadable, metaclassdecoded), the three class properties, the ivar and theperformAction:implementation, and coverage ispartialwithProtocol list of Fixture: Protocol list exceeds 20000 entries.protocol_list_unreadablereason.Base reproduction or other evidence: executed. With the source change stashed, the new count-20001 test fails (no class rows); with it applied, it passes.
User-visible CLI/MCP output (if applicable): for the issue's oversized fixture,
objc_classesis now[{name: "Fixture", is_meta_class: false, decode: partial}, {name: "Fixture", is_meta_class: true, decode: decoded}]andobjc_class_method_ivar_metadatacoverage ispartial,examined: 2,decoded: 1(derived from the unit test, which calls the same decoder the CLI uses).Remaining proof gaps: not run through the compiled CLI or on macOS; no real-world binaries with this layout were tested.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.
Validation performed
npm run test:focused -- src/native/AppleDispatchMetadata.test.ts src/native/AppleDispatchMetadata.failure.test.ts src/native/AppleObjcClassCoverage.test.ts src/native/AppleObjcClassImportBindings.test.ts— 4 files, 132 tests passednpx vitest run src/native— 53 files, 1163 passed, 1 skippednpm run typecheck— passednpm run lint— passed (no new errors);npx oxlinton the changed source keeps the existingmax-lines-per-function/complexitywarnings onreadClass(the try/catch lives in a separate helper)npx oxfmt --checkon the changed files — passednpm run verify:module-boundaries— passedCompatibility, safety, and release
Author readiness
Human owner: @SahinurDEV
Before merge
type(scope): outcome.