[zuey-cloud-harness]

Closes #28.

batch on POST /api/admin/recompute went straight through parseIntSafe, which only rejected NaN and negatives. Two things followed:

• batch=0 passed, became LIMIT 0, and an empty batch made isComplete = batch.length < batchSize evaluate 0 < 0 = false. With all=true the loop then re-read the same offset for all 50 permitted iterations and answered "Paused after 50 batches. Call again with resume=true" while having processed nothing.
• There was no upper bound either: batch=100000 was accepted, which defeats the per-request guard that keeps the Worker under its CPU limit.

This adds apps/web/app/lib/admin/batch-limit.ts with parseBatchSize(raw, limits): anything absent, blank, non-finite or below 1 falls back to 100, anything above 500 is clamped to 500, and fractions are floored because .limit() needs an integer. It uses Number rather than parseInt, so "1e9" is read as a magnitude and clamped instead of being truncated to 1.

The route now calls parseBatchSize; parseIntSafe stays for offset only, and its doc comment says so.

Tests live in apps/web/app/lib/admin/batch-limit.test.ts (six cases covering fallback, zero and negatives, in-range values, fractions, the cap, and a caller-supplied bound). The parser itself was also checked against 21 inputs with Node's type stripping; all passed.

No behaviour change for any caller already sending a sensible batch.

[zuey-cloud-harness]

Claimed and implemented by AKAdmin on 2026-10-31 (Asia/Saigon). Branch fix/akadmin-admin-recompute-batch-clamp @ d2e7c44. The akadmin-wip label on #28 marks it as taken until this merges.

[clark-cant]

Review — PR #169 fix(admin): clamp the recompute batch size to 1..500

Summary: Replaces the unvalidated parseIntSafe call on ?batch= with a new parseBatchSize() helper (positive integers only, cap 500, fallback 100, floors fractions, Number-based so 1e9 clamps instead of truncating to 1), and adds a six-case unit test. parseIntSafe stays for offset only, with a comment saying so.

Risk level: Low.

Mandatory gates

• Duplicate / prior implementation: clear — gh pr list --search "batch clamp recompute" returns only this PR; nothing merged or open implements the clamp.
• Project standards: docs found (CLAUDE.md) — 45 and 51 new lines are well under the 200-LOC limit; apps/web/app/lib/admin/ is a domain anchor, not a dumping-ground dir; the test path is picked up by the root vitest.config.ts include apps/web/app/**/*.test.ts; the ~ alias resolves in both tsconfig and vitest.
• Strategic necessity: clear value — closes the batch=0 spin in #28 and adds the upper bound the endpoint never had. Side benefit for #29: ./scripts/run-recompute.sh 0 forwards its argument unvalidated, so the server-side clamp is now the thing that makes that call safe.

Verified at d2e7c44

• Behaviour reproduced independently: re-ran parseBatchSize through Node type stripping over 24 inputs — null/undefined/""/" "/"abc"/"NaN"/"Infinity"/"-0"/"0"/"0.0"/"-25" → 100, "1" → 1, " 250 " → 250, "500" → 500, "12.9" → 12, "501"/"100000"/"1e9" → 500, and the caller-supplied {max: 50, fallback: 10} bound works. All match the tests in the PR, so the PR-body claim of 21 inputs is credible.
• Single call site: grep -rn recomputeBatch apps/web returns only this route plus the definition, so no other path can hand batchSize straight to .limit().
• isComplete = batch.length < batchSize (recompute-all-skills.ts:177) is now safe from the route because batchSize >= 1 is guaranteed, so an empty batch completes instead of re-reading the same offset.
• CI web-contracts-cli COMPLETED SUCCESS at 2026-10-31T12:11:33Z; MERGEABLE / CLEAN.

Findings

Critical: none.

Important: none.

Suggestion — batch=0 becomes 100, which is neither option in #28. The issue asked "clamp to 1, or return 400"; the PR picks a third answer: treat 0 as unusable and fall back. That is defensible (0 is a missing/garbage value, not a request for one row), but it is currently implicit — the endpoint header (api.admin.recompute.ts:1-13) still advertises "custom batch size" with no bounds, so a caller sending batch=0 sees 100 processed and no signal. One header line documenting the accepted range would close it; a sentence in #28 recording the decision would stop the next reader re-litigating it (#29's docs gap is the same family).

Suggestion — the cap bounds one batch, not one request. With all=true the route still runs up to MAX_BATCHES_PER_REQUEST = 50, and recomputeBatch issues one db.update per skill, so 500 × 50 is up to ~25,000 D1 round-trips in a single request against the Workers subrequest budget — which is the resource MAX_BATCHES_PER_REQUEST exists to protect. Not a regression (the value was previously unbounded), and real catalogs rarely fill a batch, but if the clamp is meant to be the CPU guard, the product is the number that matters: either align max with the 100 default, or add a per-request row budget.

Suggestion — make the invariant local to the library. recomputeBatch is exported and its completion test is still batch.length < batchSize, so any future non-HTTP caller passing 0 re-creates the #28 loop. if (batchSize < 1) batchSize = 1 (or treating an empty batch as complete) at the top of recomputeBatch would put the guard where the loop lives instead of relying on the route being the only caller.

Verdict: Comment — minor suggestions only, safe to merge as-is. Not merged here: reviewDecision is empty and the akadmin-wip label says the AKAdmin flow owns this branch, so the merge stays with the owner/that flow. Once approved it is a one-click merge — CLEAN state, green web-contracts-cli, closes #28.

Posted by github-maintain (ck:review-pr profile) at 2026-10-31T19:05Z