refactor: remove unsafe overload casts in parallel probe#383
refactor: remove unsafe overload casts in parallel probe#383
Conversation
parallel-probe used forced casts to distinguish overload inputs and to normalize probe failures. Replace those with explicit type guards and error normalization so the module keeps the same behavior without leaning on unsafe assertions.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!isGetTopCandidatesParams(accountManagerOrParams)) { | ||
| throw new TypeError("getTopCandidates requires accountManager"); | ||
| } |
There was a problem hiding this comment.
isGetTopCandidatesParams validates four fields — accountManager, modelFamily, model, and maxCandidates — but the thrown message always says "getTopCandidates requires accountManager". a caller that passes { accountManager: validMgr, modelFamily: 123, model: null, maxCandidates: 2 } hits this throw because modelFamily is not a string, but the error blames accountManager. this is a new throw path introduced by this PR, and the message is wrong in those cases.
| if (!isGetTopCandidatesParams(accountManagerOrParams)) { | |
| throw new TypeError("getTopCandidates requires accountManager"); | |
| } | |
| if (!isGetTopCandidatesParams(accountManagerOrParams)) { | |
| throw new TypeError("getTopCandidates: invalid named params (check accountManager, modelFamily, model, maxCandidates)"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/parallel-probe.ts
Line: 95-97
Comment:
**misleading error message when non-accountManager field is invalid**
`isGetTopCandidatesParams` validates four fields — `accountManager`, `modelFamily`, `model`, and `maxCandidates` — but the thrown message always says `"getTopCandidates requires accountManager"`. a caller that passes `{ accountManager: validMgr, modelFamily: 123, model: null, maxCandidates: 2 }` hits this throw because `modelFamily` is not a string, but the error blames `accountManager`. this is a new throw path introduced by this PR, and the message is wrong in those cases.
```suggestion
if (!isGetTopCandidatesParams(accountManagerOrParams)) {
throw new TypeError("getTopCandidates: invalid named params (check accountManager, modelFamily, model, maxCandidates)");
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #387, which rebuilds the full open PR stack onto one reviewed integration branch. |
|
Closing in favor of #387. |
1 participant
Problem
parallel-proberelied on unsafe forced casts to distinguish its overload inputs and to normalize probe failures.That keeps behavior working, but it weakens the type guarantees around a hot path that is already small enough to model explicitly.
Fix
Replace the forced casts with explicit type guards and a small error-normalization helper.
Changes
lib/parallel-probe.tsisAccountManager(...)isGetTopCandidatesParams(...)toProbeError(...)getTopCandidates(...)as Errortest/parallel-probe.test.tsErrorprobe failures being normalizedValidation
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this PR replaces unsafe forced casts (
as GetTopCandidatesParams,as AccountManager,as Error) inlib/parallel-probe.tswith explicit runtime type guards (isAccountManager,isGetTopCandidatesParams) and an error-normalization helper (toProbeError). two new test cases cover non-Errorthrow normalization in single-candidate probing and the named-params overload path. no windows filesystem or token safety concerns are introduced; no concurrency issues are added — the parallel probe path is structurally unchanged.isGetTopCandidatesParamsfailure emits\"requires accountManager\"even whenmodelFamilyormaxCandidatesis the bad field — misleading for callersisAccountManagerguard in the positional-argselsebranch has no vitest test (named-params path is covered; positional path is not)Confidence Score: 4/5
safe to merge with minor fixes recommended — the core type-narrowing logic is correct and tests pass
the refactor is sound and improves type safety; the two gaps (misleading error message in the named-params failure path, missing positional-args TypeError test) are low-risk but worth addressing before merge. no concurrency, token safety, or windows filesystem concerns introduced.
lib/parallel-probe.ts lines 95-97 (error message) and 104-106 (missing test coverage)
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant getTopCandidates participant isGetTopCandidatesParams participant isAccountManager participant AccountManager Caller->>getTopCandidates: named params call getTopCandidates->>isGetTopCandidatesParams: validate params object isGetTopCandidatesParams->>isAccountManager: validate accountManager field isAccountManager-->>isGetTopCandidatesParams: bool isGetTopCandidatesParams-->>getTopCandidates: bool alt invalid params (misleading error message) getTopCandidates-->>Caller: TypeError("requires accountManager") else valid getTopCandidates->>AccountManager: getAccountsSnapshot() AccountManager-->>getTopCandidates: ManagedAccount[] getTopCandidates-->>Caller: top-N ManagedAccount[] end Caller->>getTopCandidates: positional args call getTopCandidates->>isAccountManager: validate first arg isAccountManager-->>getTopCandidates: bool alt invalid (no test coverage for this path) getTopCandidates-->>Caller: TypeError("requires accountManager") else valid getTopCandidates->>AccountManager: getAccountsSnapshot() AccountManager-->>getTopCandidates: ManagedAccount[] getTopCandidates-->>Caller: top-N ManagedAccount[] endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor: remove unsafe overload casts i..." | Re-trigger Greptile