hib/hib-pr-reviewer

Fork 0

feat(lib): pricing utility (RMP comparison subject) #6

Closed

hibryda wants to merge 1 commit from test/rmp-compare-findings into main

hibryda commented

2026-06-03 08:48:18 +02:00

Owner

A small utility with genuine review issues, used as the A/B comparison subject for the quorum modules (exercises the arbiter/divergence + heterogeneous-leg paths). Throwaway — close after the comparison.

hibryda added 1 commit

2026-06-03 08:48:18 +02:00

feat(lib): add pricing utility (RMP comparison subject)

pr-reviewer CI / tsc --noEmit (pull_request) Successful in 22s

Details

pr-reviewer CI / vitest (pull_request) Successful in 1m1s

Details

pr-reviewer CI / tsc build (pull_request) Successful in 36s

Details

0e8be4592a

pr-reviewer-bot commented

2026-06-03 08:50:38 +02:00

Member

hib-pr-reviewer review — PR #6 (hib/hib-pr-reviewer)

Round 1 — head 0e8be4592a44, base main, trigger opened

TL;DR: NEEDS_WORK — kept all 5 agreed findings (3 major, 2 minor) across four new exported functions; corrected one off-by-one line citation from reviewer B (line 27 → 28 for pickFirst return); no findings dropped.

Summary

All 5 findings verified against /var/lib/pr-reviewer/repos/hib/hib-pr-reviewer/1527/src/lib/pricing.ts. Both reviewers agree on all five issues across the four exported functions. Line numbers confirmed: applyDiscount return at line 8, parseQuantity return at line 14, loadConfig fetch/json at lines 21-22, pickFirst return at line 28 (B cited line 27 — that is the comment line; corrected to 28). The 14-resilience-and-config.md rule on mandatory timeouts is confirmed in the checkout, substantiating the loadConfig major finding. Kept 5 agreed findings, corrected 1 line-number discrepancy, dropped nothing.

Blast Radius

The diff is confined to a single new file (src/lib/pricing.ts) with four exported functions. Because the functions are exported they form a new public API surface that any downstream module can import. The billing-correctness bugs (applyDiscount clamping, parseQuantity NaN) and the resilience violation (loadConfig) carry proportionally higher impact if callers are added before the issues are fixed.

BLAST_SCORE: 4/10

Risk Indicators

Indicator	Value
Sensitive functions	`loadConfig`, `applyDiscount`
Migration touched	—
Test delta	—
Dependency changes	—

CI status (head `0e8be4592a44`)

Overall: ✓ success

3 checks: 3 pending

Check	State	Link
pr-reviewer CI / tsc --noEmit (pull_request)	⏳ pending	details
pr-reviewer CI / vitest (pull_request)	⏳ pending	details
pr-reviewer CI / tsc build (pull_request)	⏳ pending	details

Findings (5)

[MAJOR] applyDiscount: unclamped percentOff allows negative prices and silent price inflation

src/lib/pricing.ts:8

Line 8: return priceCents - (priceCents * percentOff) / 100;

percentOff is never validated before use:

percentOff > 100 yields a negative price (e.g. applyDiscount(500, 150) → -250).
Negative percentOff silently raises the price rather than discounting it.

Both are billing-correctness regressions in any production pricing context.

Fix:

export function applyDiscount(priceCents: number, percentOff: number): number {
  if (percentOff < 0 || percentOff > 100) {
    throw new RangeError(`percentOff must be 0–100, got ${percentOff}`);
  }
  return Math.round(priceCents * (1 - percentOff / 100));
}

[MINOR] applyDiscount: returns fractional cents due to floating-point arithmetic

src/lib/pricing.ts:8

Line 8: return priceCents - (priceCents * percentOff) / 100;

Floating-point arithmetic on integer cent values (e.g. applyDiscount(999, 33)) returns 669.33, not an integer. Monetary cent values must be integers; fractional values silently corrupt downstream arithmetic and integer database columns.

Fix: Wrap the return value in Math.round(…) (or Math.floor to always favour the merchant). The clamping fix above already incorporates this.

[MAJOR] parseQuantity: missing radix and no NaN guard allow silent NaN propagation

src/lib/pricing.ts:14

Line 14: return parseInt(raw);

Two problems:

Missing radix: parseInt('010') can return 8 in engines that honour legacy octal notation; every major linter (and MDN) requires an explicit radix.
No NaN guard: parseInt('abc', 10) returns NaN typed as number, which propagates silently through all downstream arithmetic and comparisons.

Fix:

export function parseQuantity(raw: string): number {
  const n = parseInt(raw, 10);
  if (Number.isNaN(n) || n < 0) throw new RangeError(`Invalid quantity: "${raw}"`);
  return n;
}

Alternatively change the return type to number | null and let callers decide.

[MAJOR] loadConfig: no response.ok check, no timeout, no try/catch — violates resilience rule

src/lib/pricing.ts:21

Lines 21-22:

const res = await fetch(url);
return res.json() as Promise<Record<string, unknown>>;

14-resilience-and-config.md explicitly requires every external call to have a timeout. Three violations:

No response.ok check — a 4xx/5xx response with a JSON body is silently cast and treated as valid config.
No timeout — a slow or hung server stalls the caller indefinitely.
No try/catch — a network error or non-JSON response body (e.g. an HTML error page) surfaces as an unhandled rejection far from the call site.

Fix:

export async function loadConfig(url: string): Promise<Record<string, unknown>> {
  let res: Response;
  try {
    res = await fetch(url, { signal: AbortSignal.timeout(5_000) });
  } catch (err) {
    throw new Error(`loadConfig: network error fetching ${url}`, { cause: err });
  }
  if (!res.ok) throw new Error(`loadConfig: HTTP ${res.status} for ${url}`);
  return res.json() as Promise<Record<string, unknown>>;
}

[MAJOR] pickFirst: empty array returns undefined typed as T — type-system lie

src/lib/pricing.ts:28

Line 28: return items[0];

When items is empty, items[0] is undefined at runtime, but the declared return type is T. TypeScript does not warn callers; they will dereference undefined without a null-check and crash or corrupt state.

Fix — Option A (throw on empty, empty is always a bug):

export function pickFirst<T>(items: T[]): T {
  if (items.length === 0) throw new RangeError('pickFirst called with empty array');
  return items[0];
}

Fix — Option B (widen the return type, let callers decide):

export function pickFirst<T>(items: T[]): T | undefined {
  return items[0];
}

Verdict

NEEDS_WORK

_{hib-pr-reviewer • round 1 • 5 findings (4M/1m) • 2026-06-03T06:49:50.684Z → 2026-06-03T06:50:38.960Z • posted-as: pr-reviewer-bot • model: auto}

## hib-pr-reviewer review — PR #6 (hib/hib-pr-reviewer) **Round 1** — head `0e8be4592a44`, base `main`, trigger `opened` **TL;DR:** NEEDS_WORK — kept all 5 agreed findings (3 major, 2 minor) across four new exported functions; corrected one off-by-one line citation from reviewer B (line 27 → 28 for pickFirst return); no findings dropped. ### Summary All 5 findings verified against `/var/lib/pr-reviewer/repos/hib/hib-pr-reviewer/1527/src/lib/pricing.ts`. Both reviewers agree on all five issues across the four exported functions. Line numbers confirmed: `applyDiscount` return at line 8, `parseQuantity` return at line 14, `loadConfig` fetch/json at lines 21-22, `pickFirst` return at line 28 (B cited line 27 — that is the comment line; corrected to 28). The `14-resilience-and-config.md` rule on mandatory timeouts is confirmed in the checkout, substantiating the `loadConfig` major finding. Kept 5 agreed findings, corrected 1 line-number discrepancy, dropped nothing. ### Blast Radius The diff is confined to a single new file (`src/lib/pricing.ts`) with four exported functions. Because the functions are exported they form a new public API surface that any downstream module can import. The billing-correctness bugs (`applyDiscount` clamping, `parseQuantity` NaN) and the resilience violation (`loadConfig`) carry proportionally higher impact if callers are added before the issues are fixed. **BLAST_SCORE: 4/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `loadConfig`, `applyDiscount` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `0e8be4592a44`) **Overall: ✓ success** 3 checks: 3 pending | Check | State | Link | |---|---|---| | pr-reviewer CI / tsc --noEmit (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/66/jobs/0) | | pr-reviewer CI / vitest (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/66/jobs/1) | | pr-reviewer CI / tsc build (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/66/jobs/2) | ### Findings (5) #### **[MAJOR]** applyDiscount: unclamped percentOff allows negative prices and silent price inflation _src/lib/pricing.ts:8_ Line 8: `return priceCents - (priceCents * percentOff) / 100;` `percentOff` is never validated before use: - `percentOff > 100` yields a negative price (e.g. `applyDiscount(500, 150)` → `-250`). - Negative `percentOff` silently raises the price rather than discounting it. Both are billing-correctness regressions in any production pricing context. **Fix:** ```ts export function applyDiscount(priceCents: number, percentOff: number): number { if (percentOff < 0 || percentOff > 100) { throw new RangeError(`percentOff must be 0–100, got ${percentOff}`); } return Math.round(priceCents * (1 - percentOff / 100)); } ``` #### **[MINOR]** applyDiscount: returns fractional cents due to floating-point arithmetic _src/lib/pricing.ts:8_ Line 8: `return priceCents - (priceCents * percentOff) / 100;` Floating-point arithmetic on integer cent values (e.g. `applyDiscount(999, 33)`) returns `669.33`, not an integer. Monetary cent values must be integers; fractional values silently corrupt downstream arithmetic and integer database columns. **Fix:** Wrap the return value in `Math.round(…)` (or `Math.floor` to always favour the merchant). The clamping fix above already incorporates this. #### **[MAJOR]** parseQuantity: missing radix and no NaN guard allow silent NaN propagation _src/lib/pricing.ts:14_ Line 14: `return parseInt(raw);` Two problems: 1. **Missing radix**: `parseInt('010')` can return `8` in engines that honour legacy octal notation; every major linter (and MDN) requires an explicit radix. 2. **No NaN guard**: `parseInt('abc', 10)` returns `NaN` typed as `number`, which propagates silently through all downstream arithmetic and comparisons. **Fix:** ```ts export function parseQuantity(raw: string): number { const n = parseInt(raw, 10); if (Number.isNaN(n) || n < 0) throw new RangeError(`Invalid quantity: "${raw}"`); return n; } ``` Alternatively change the return type to `number | null` and let callers decide. #### **[MAJOR]** loadConfig: no response.ok check, no timeout, no try/catch — violates resilience rule _src/lib/pricing.ts:21_ Lines 21-22: ```ts const res = await fetch(url); return res.json() as Promise<Record<string, unknown>>; ``` `14-resilience-and-config.md` explicitly requires *every external call to have a timeout*. Three violations: 1. **No `response.ok` check** — a 4xx/5xx response with a JSON body is silently cast and treated as valid config. 2. **No timeout** — a slow or hung server stalls the caller indefinitely. 3. **No try/catch** — a network error or non-JSON response body (e.g. an HTML error page) surfaces as an unhandled rejection far from the call site. **Fix:** ```ts export async function loadConfig(url: string): Promise<Record<string, unknown>> { let res: Response; try { res = await fetch(url, { signal: AbortSignal.timeout(5_000) }); } catch (err) { throw new Error(`loadConfig: network error fetching ${url}`, { cause: err }); } if (!res.ok) throw new Error(`loadConfig: HTTP ${res.status} for ${url}`); return res.json() as Promise<Record<string, unknown>>; } ``` #### **[MAJOR]** pickFirst: empty array returns undefined typed as T — type-system lie _src/lib/pricing.ts:28_ Line 28: `return items[0];` When `items` is empty, `items[0]` is `undefined` at runtime, but the declared return type is `T`. TypeScript does not warn callers; they will dereference `undefined` without a null-check and crash or corrupt state. **Fix — Option A (throw on empty, empty is always a bug):** ```ts export function pickFirst<T>(items: T[]): T { if (items.length === 0) throw new RangeError('pickFirst called with empty array'); return items[0]; } ``` **Fix — Option B (widen the return type, let callers decide):** ```ts export function pickFirst<T>(items: T[]): T | undefined { return items[0]; } ``` ### Verdict **NEEDS_WORK** --- <sub>hib-pr-reviewer • round 1 • 5 findings (4M/1m) • 2026-06-03T06:49:50.684Z → 2026-06-03T06:50:38.960Z • posted-as: pr-reviewer-bot • model: auto</sub>

hibryda closed this pull request

2026-06-03 09:09:33 +02:00

pr-reviewer CI / tsc --noEmit (pull_request) Successful in 22s

Details

pr-reviewer CI / vitest (pull_request) Successful in 1m1s

Details

pr-reviewer CI / tsc build (pull_request) Successful in 36s

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

hib/hib-pr-reviewer!6

No description provided.

Rows
Columns

feat(lib): pricing utility (RMP comparison subject) #6

hib-pr-reviewer review — PR #6 (hib/hib-pr-reviewer)

Summary

Blast Radius

Risk Indicators

CI status (head 0e8be4592a44)

Findings (5)

[MAJOR] applyDiscount: unclamped percentOff allows negative prices and silent price inflation

[MINOR] applyDiscount: returns fractional cents due to floating-point arithmetic

[MAJOR] parseQuantity: missing radix and no NaN guard allow silent NaN propagation

[MAJOR] loadConfig: no response.ok check, no timeout, no try/catch — violates resilience rule

[MAJOR] pickFirst: empty array returns undefined typed as T — type-system lie

Verdict

Pull request closed

CI status (head `0e8be4592a44`)