fix(config): stop numeric env overrides resolving to Infinity, and cap timer periods below the 32-bit overflow (BLO-27641) - #1383
Conversation
1 similar comment
|
@ally please review at head 40700bb (BLO-27641). This generalizes the Specific things worth your attention:
Mutation-checked: all 8 sites turn red when the bare idiom is restored, and a separate ceiling-only mutation isolates the |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 40700bb
Solid, well-targeted fix. I verified the core claims against the tree rather than the diff prose: loadConfig() is a plain un-memoized function (config.ts:333), so the describe.each re-invocations are real; all eight settings are top-level Config fields (config.ts:90-125), so loadConfig()[key] indexes correctly; and the TIMER_SETTING_MS_FACTOR values match the actual call sites — index.ts:1546, 1575, 1614 all multiply by 60 * 1000, and heartbeatSchedulerIntervalMs is passed straight to setInterval at index.ts:1539. The only two surviving Number(process.env.X) || sites are the PORT pair at config.ts:555 and config.ts:627, so the guard test's allowlist is accurate as written.
One Important finding, on a comment rather than on the logic.
Critical Issues (0)
Important Issues (1)
-
[comments]
server/src/config.ts:315— The JSDoc closes with "Every currently-valid input resolves exactly as before." That is not true, and it understates the one part of this change an operator must act on. The newmaxceilings silently change the resolved value for any finite, positive override that was previously honoured verbatim:HEARTBEAT_SCHEDULER_INTERVAL_MS=172800000(48h) resolved to172800000; now resolves to86400000.PAPERCLIP_PR_RECONCILER_WINDOW_DAYS=730resolved to730; now resolves to365.PAPERCLIP_DB_BACKUP_RETENTION_DAYS=7300resolved to7300; now resolves to3650— this one shortens retention, so the next prune deletes backups the operator asked to keep.
The paragraph correctly flags the negative-override change, which makes the omission read as deliberate scoping rather than an oversight, and a reader who trusts it will skip auditing their overrides on upgrade. Compounding it: a clamp of a finite value produces no signal at all. The doc argues (correctly) that falling back on
Infinity"keeps the startup banner honest", but the banner atindex.ts:1670-1672prints the post-clamp number with nothing distinguishing "operator asked for this" from "operator asked for 730 and got 365".- Reword to say that finite values above the new ceilings are now clamped, and list the ceilings that are plausibly reachable in an existing deployment (
heartbeatSchedulerIntervalMs,prReconcilerWindowDays,databaseBackupRetentionDays). - Consider having
resolveNumericSettingtake the setting name and log once when a candidate is clamped or rejected. That closes the gap for free and makes the "already invalid" fallback path observable too, which is the property the surrounding doc is arguing for.
Suggestions (3)
- [tests]
server/src/__tests__/numeric-env-bounds.test.ts:218— The reintroduction guard matches onlyNumber(process.env.X) ||. Near-identical spellings escape it:Number(process.env.X ?? ""),parseInt(process.env.X, 10) ||,Number(process.env["X"]) ||, andNumber(x) ||after destructuringprocess.env. Given the comment above it notes two in-flight PRs (#1375, #1309) re-adding this idiom, the next one is likely to arrive in a form the regex misses. Widening to(?:Number|parseInt|parseFloat)\(\s*process\.env[.\[]with a||/??lookahead would cover the realistic variants at no cost to the failure message. - [types]
server/src/config.ts:319—candidates: readonly unknown[]discards the type information the call sites have. Every caller passesstring | undefinedenv values and typed file-config numbers, soreadonly (string | number | null | undefined)[]would be accurate and would make the compiler reject a mistyped config field. As written,Number(true) === 1andNumber([5]) === 5, so a boolean or single-element array in a config file resolves to a plausible-looking interval instead of falling through to the documented default. (Not a regression — the old code passed the file value intoMath.maxunchecked — but the new helper is the natural place to close it.) - [code]
server/src/__tests__/numeric-env-bounds.test.ts:204— ThePORTallowlist rationale ("fails loudly atlisten()") holds forconfig.ts:555, butPORTis read a second time atconfig.ts:627to buildlinearOAuthRedirectUri. WithPORT=Infinitythat interpolateshttp://localhost:Infinity/api/auth/linear/callbackbefore anything reacheslisten(). The process does still die at bind, so the exemption is sound — worth a half-sentence so the next reader doesn't have to re-derive that the second call site is covered by the first.
Strengths
- The root-cause comment at
config.ts:255-268is the best part of this change: it explains whyMath.max(FLOOR, Number(x) || D)is not a double guard, and it splits the consequence by consumer. TherecoveryActionTimeoutMsobservation — thatnew Date(now + Infinity)is an Invalid Date whose comparisons are all false, so the BLO-19124 bound is absent rather than loosened — is the kind of second-order detail that usually gets lost. - Choosing fallback-over-clamp for non-finite input, and stating the reasoning, is the right call and is directly tested (
falls back to the documented default rather than clamping to the ceiling). HOSTILE_INPUTSearns its keep.1e308(finite, so aNumber.isFinitecheck alone passes it) and40000(finite, plausible as a minutes/ms confusion, and still 27.7 days) are exactly the two cases that make the ceilings necessary rather than decorative, and thebounds.max * factor <= MAX_TIMER_DELAY_MSstatic assertion checks the invariant in the timer's unit rather than the setting's.- Centralising the bounds in one table instead of scattering literals across eight call sites makes the ceilings reviewable together, which is what let me check the timer math above in one pass.
Recommended Action
- No Critical issues — nothing blocking merge on correctness grounds.
- Fix the Important comment inaccuracy this cycle; it is a two-line edit and it is the thing an operator reading this file will rely on during upgrade. The optional clamp-warning is worth considering alongside it.
- Take the Suggestions opportunistically — the guard-regex widening is the one with the most leverage, given the stated re-introduction pressure.
Note: CI was still pending on the test jobs at the time of this review, so I have not verified the new suite passes — only that its assertions are sound against the code at this head.
40700bb to
e7df01a
Compare
…uning them (BLO-27641) Addresses the Ally review on #1383. Important — the JSDoc claimed "Every currently-valid input resolves exactly as before", which is false. The new ceilings clamp finite overrides that were previously honoured verbatim, and a clamp of a finite value emits no signal: the startup banner prints the post-clamp number, so 730 and 365 are indistinguishable once resolved. Three are plausibly reachable in an existing deployment, and one of them (DB_BACKUP_RETENTION_DAYS=7300 -> 3650) *shortens* retention, so the next prune deletes backups an operator asked to keep. - Reword the JSDoc to state the clamp explicitly and list the three reachable cases as an upgrade-audit note. - Warn once per setting when an override is clamped or rejected, so the adjustment is visible to the operator who caused it. Dedupe is process-global (loadConfig() is un-memoized and re-invoked freely); resetNumericSettingWarnings() is the test seam. Suggestions: - Widen the reintroduction guard beyond the literal `Number(process.env.X) ||` form. parseInt/parseFloat, bracket access, and `??` have the same hole, and with two in-flight PRs re-adding the idiom the next one is as likely to arrive in a variant. Verified: catches 6 hostile spellings, rejects 3 safe ones, and still finds exactly the two allowlisted PORT sites. - Narrow `candidates` from `readonly unknown[]` to the types the call sites actually pass, so a boolean or single-element array in a config file cannot resolve to a plausible-looking interval via Number(true) === 1. - Note that PORT's second read (linearOAuthRedirectUri) is covered by the first: the process still dies at bind, so the failure stays loud.
|
@ally please re-review at head Your Important finding (JSDoc over-stated) — addressed in
Your 3 suggestions, all taken. Guard regex widened beyond the literal Specific focus I'd value: the warning dedupe being process-global rather than per- Verification at this head: 144/144 in |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 56f3353
Two commits landed since my last pass. e7df01a8 closes the one Important finding from that review and takes all three suggestions with it; 56f33530 then makes the reintroduction guard falsifiable, which is the single best change in this PR. config.ts is byte-identical between those two heads, so the production surface here is unchanged from e7df01a8 and only the test file moved.
I re-verified the claims against the tree at this head rather than the diff prose. Running the PR's own detector over config.ts at this head returns exactly ['PORT', 'PORT'], matching the allowlist and its two call sites (config.ts:613, config.ts:688). Every fallback in NUMERIC_SETTING_BOUNDS sits inside its own [min, max]. Both timer ceilings clear the overflow threshold (10080 × 60_000 = 604_800_000; 86_400_000 × 1), against 2_147_483_647. Clamp-up-to-floor is correctly preserved for positive input — HEARTBEAT_SCHEDULER_INTERVAL_MS=5000 still resolves to 10000, as it did under Math.max — so change (1) in the new JSDoc is accurately scoped to negatives.
Two Important findings: one in the warning path, one in where the new guard is aimed.
Prior Findings Dispositioned (1)
- prior:40700bb important 1 — fixed —
server/src/config.ts:341— The "Every currently-valid input resolves exactly as before" sentence is gone. The JSDoc now opens a "Two intentional behaviour changes, both of which an operator should audit on upgrade" block that names the finite-clamp case explicitly and enumerates the three reachable ceilings, includingPAPERCLIP_DB_BACKUP_RETENTION_DAYS=7300 → 3650with its "shortens retention" consequence called out (config.ts:346-351). The optional half of that finding also landed:warnNumericSettingAdjustment(config.ts:322) now reports both the clamp and the reject path, so the adjustment is observable rather than silent.
Critical Issues (0)
Important Issues (2)
-
[errors]
server/src/config.ts:370— The reject-path warning renders the offending value withJSON.stringify, which maps every non-finite number to the stringnull. So the one input class this branch exists to report is the one it cannot name:[config] databaseBackupIntervalMinutes: ignoring override null — not a finite positive numberAn operator reads that and goes looking for a literal
nullin their config file, which is not there. This is reachable, and the PR itself establishes the path: the test atnumeric-env-bounds.test.ts:194feedsJSON.parse("1e999")precisely because "a config file can carry a non-finite number even though JSON has no Infinity token", anddatabaseBackupIntervalMinutes/databaseBackupRetentionDaysare the two settings that pass a file-config number as a candidate. The string path is fine —JSON.stringify("Infinity")gives"Infinity", quoted and legible — so the defect is confined to numeric candidates, i.e. the config-file source, where the operator has the least other signal about which key is at fault. It matters because this commit's whole purpose is making an adjustment legible to the operator who caused it, and the clamp branch one line below names both values correctly.typeof candidate === "number" ? String(candidate) : JSON.stringify(candidate)yieldsignoring override Infinitywhile preserving the quoting that distinguishes the string"abc"from a number.- Worth a case in "warns when an override is rejected as unusable" asserting the message contains
Infinityfor a numericJSON.parse("1e999")candidate. The existing assertion passes the string"Infinity", which is the variant that already works.
-
[tests]
server/src/__tests__/numeric-env-bounds.test.ts:342— The guard reads onlyconfig.ts, but56f33530widened the detector specifically because of a different file, and then did not aim it there. The comment at line 296 states the "fallback inside" shape "is the spelling live at three sites inservices/k8s-job-liveness.tstoday". I ranBARE_NUMERIC_ENV_IDIOMagainst that file at this head: it matches five, not three —PAPERCLIP_K8S_JOB_LIVENESS_TIMEOUT_MS,..._STALE_JOB_DELETE_CONFIRM_ATTEMPTS,..._STALE_JOB_DELETE_CONFIRM_DELAY_MS,..._FAILURE_LOG_TAIL_LINES,..._FAILURE_LOG_TAIL_MAX_BYTES. So the detector already proves it would catch every one of them, and the harness is scoped so it never will.These are not benign. Two carry the same consequence split this PR's own JSDoc describes at
config.ts:255-268:k8s-job-liveness.ts:28-31bounds the retry loop atk8s-job-liveness.ts:791(for (…; attempt < STALE_JOB_DELETE_CONFIRM_ATTEMPTS; …)with anawait sleepMsat 792).Math.max(1, Number("Infinity"))isInfinity, so the budget the comment at lines 24-27 says exists to "fail closed" instead never terminates.k8s-job-liveness.ts:64-67bounds the failure-log tail, consumed at lines 521-522 asredacted.length > FAILURE_LOG_TAIL_MAX_BYTES. AtInfinitythat comparison is always false, so the transcript is never truncated — the exact "cost and a context-window problem" the comment at lines 57-59 says the bound prevents.
Fixing that file is fairly out of scope for BLO-27641, whose acceptance criteria are a
config.tsgrep — but shipping a detector that demonstrably catches five live instances while pointing it at a file where it catches zero leaves the class unguarded in the one place the PR knows it is live, and a scoping decision recorded only as an aside in a test comment will not survive contact with the next reader.- Either widen
findBareNumericEnvIdiomsto scan the service files too (the function already takes a source string, so this is a secondreadFileand a loop), or file the follow-up and cite the ticket in that comment instead of the bare observation. - Either way correct "three sites" to five, or reword to avoid a count that is already wrong at the head that introduces it.
Suggestions (3)
- [tests]
server/src/__tests__/numeric-env-bounds.test.ts:136— ThisbeforeEachneither stubsconsole.warnnor callsresetNumericSettingWarnings(), unlike its counterpart at line 70. The block then drives 4 timer settings × 10HOSTILE_INPUTSthroughloadConfig(), so roughly three dozen genuine[config] …lines print into the suite output. The assertions are unaffected and the global dedupe keeps it bounded, but the first block already established the pattern for silencing this, and expected-warning noise trains readers to ignore the real thing. Copying the two lines across is enough. - [code]
server/src/config.ts:270— Nothing asserts the table-level invariantmin <= fallback <= max. It holds for all eight entries today, and a violation would not throw:resolveNumericSettingclamps the fallback at line 383, so a mis-declared default silently resolves to a bound. The "resolves to the documented default when the override is unset" test would catch it, but only forENV_ONLY_SETTINGS, which deliberately excludesdatabaseBackupIntervalMinutesanddatabaseBackupRetentionDays(line 62) — the two settings where a config file can also supply the value. A three-line loop overObject.entries(NUMERIC_SETTING_BOUNDS)closes it for all eight and is the natural companion to thebounds.max * factorassertion already there. - [types]
server/src/config.ts:295—TIMER_SETTING_MS_FACTORisPartial<Record<keyof typeof NUMERIC_SETTING_BOUNDS, number>>, and the overflow assertion iterates its keys. A future timer setting added toNUMERIC_SETTING_BOUNDSand passed tosetIntervalbut not added here is therefore invisible to the invariant — the compiler permits the omission by design, and the test derives its key list from the incomplete table, so both guards still report green. Given the reintroduction pressure the guard documents, consider asserting that every bounds key matching/IntervalM(inutes|s)$/has a factor entry. Heuristic, but it covers the naming convention all four current timers follow.
Strengths
56f33530is the change I would most want to see here. A guard asserting only that today's file is clean is satisfied equally well by a regex matching nothing, and the comment atnumeric-env-bounds.test.ts:311-316names that failure mode outright — "a guard that reports green on a case it never evaluated" — before fixing it. The eight positive cases at line 317 and five negative cases at line 330 pin the detector from both sides, and including "prose in a comment" as a must-not-flag case tests the comment-stripping step rather than assuming it.- Extracting
findBareNumericEnvIdioms(line 303) to take a source string is what makes the detector testable in isolation, and incidentally makes the Important finding above a small change rather than a restructure. - The
Number(process.env.X ?? "3")insight is genuinely non-obvious and correctly argued: an inline default substitutes only for an unset variable, so a hostile value passes straight through toNumber— as the comment says, weaker than the outside-fallback form rather than safer, which is the opposite of how it reads. - The response to the prior review is unusually complete: the JSDoc rewrite is not a minimal edit to delete a false sentence but a restructure that names the audit action and quantifies the reachable clamps, and the "consider logging" aside was implemented rather than deferred.
warnNumericSettingAdjustmentgets the dedupe scope right, andconfig.ts:313-317explains why process-global is correct rather than merely noting that it is.resolveNumericSetting reports adjustmentsthen tests the silent cases as carefully as the loud ones — honoured-verbatim, absent, and no-name-supplied all assertnot.toHaveBeenCalled(), which is the usual failure mode for this kind of change.
Recommended Action
- No Critical issues — nothing blocking merge on correctness grounds.
- Fix the
JSON.stringifyrendering atconfig.ts:370this cycle; it is a one-line change that restores the operator-legibility property the commit was written to provide. - Decide the guard's scope deliberately — widen the scan to
k8s-job-liveness.tsor file the follow-up and cite it — and correct the "three sites" count either way. - Take the Suggestions opportunistically; the
min <= fallback <= maxassertion is the highest-leverage, since it is the only gap that can change a resolved value rather than only a message.
Note: at the time of this review the Build, Typecheck, and all General tests jobs were still pending on this head, so I have not seen the suite execute — only verified that its assertions are sound against the code at this head, and separately re-ran the detector regex myself against both files.
|
Both Important findings were correct and are fixed in 1.
|
| site | at Infinity |
|---|---|
JOB_LIVENESS_TIMEOUT_MS |
AbortSignal.timeout(Infinity) throws RangeError — verified on Node, it does not clamp. Loud. |
STALE_JOB_DELETE_CONFIRM_ATTEMPTS |
attempt < Infinity never terminates |
STALE_JOB_DELETE_CONFIRM_DELAY_MS |
a real setTimeout delay via sleepMs → 1 ms coercion; with the row above, a hot loop against the k8s API |
FAILURE_LOG_TAIL_LINES / _MAX_BYTES |
length > Infinity always false; never truncated |
BLO-28664 said "none of these is a setInterval delay, so the 1 ms hot-loop direction does not apply here." That was wrong because of the delay site. Ticket corrected: 8 sites → 10, the timer claim retracted, and a ceiling requirement added to its AC.
Suggestions — all three taken
min <= fallback <= maxover all eight entries. Agreed this was the highest-leverage one: it is the only table error that changes a resolved value rather than a message, since the fallback is itself clamped.- Timer-factor completeness on
/IntervalM(inutes|s)$/. Worth noting how invisible the omission is — deleting oneTIMER_SETTING_MS_FACTORentry silently drops 11 parametrized cases (169 → 158) because the overflow block derives its keys from that table, and without this assertion the suite still reports green. console.warnsilencing in the timer block'sbeforeEach, matching the first block.
Mutation checks
Every new assertion was verified to bite — a guard that has never been seen red is the defect class this PR exists to close:
| mutation | result |
|---|---|
revert describeNumericCandidate → JSON.stringify |
1 failed / 168 passed |
databaseBackupRetentionDays.fallback 7 → 5000 |
1 failed / 168 passed |
drop strandedBlockedIssueReconcilerIntervalMinutes ms factor |
1 failed / 157 passed (and 11 cases vanish) |
add a 6th offender to k8s-job-liveness.ts |
1 failed / 168 passed |
"fix" FAILURE_LOG_TAIL_LINES without shrinking the list |
1 failed / 168 passed |
169 pass clean at 17b4b8aa; tsc --noEmit clean. k8s-job-liveness.ts is untouched by this commit (git diff empty) — the file is read by the guard, not modified.
On your note about CI: agreed, and the jobs pending at your last pass have since been re-run. I will not enqueue until the suite is actually green on this head.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 17b4b8a
One commit since my last pass. 17b4b8aa closes both Important findings from that review and takes all three Suggestions with it. The production surface moves by a single function — describeNumericCandidate (config.ts:342-344, +17 lines) — and everything else is test material.
Two things I could verify this time that I could not before.
CI is green at this head. Build, Typecheck + Release Registry, all four General tests (server N/4) shards, e2e, policy and verify all report success. My previous two reviews both carried a caveat that the suite had not been observed executing; that caveat is now discharged.
The guard's claims are executable, so I ran its own detector against both scanned files at this head rather than reading them. config.ts → []. k8s-job-liveness.ts → exactly PAPERCLIP_K8S_FAILURE_LOG_TAIL_LINES, ..._FAILURE_LOG_TAIL_MAX_BYTES, ..._JOB_LIVENESS_TIMEOUT_MS, ..._STALE_JOB_DELETE_CONFIRM_ATTEMPTS, ..._STALE_JOB_DELETE_CONFIRM_DELAY_MS — the five names in knownOffenders, no more and no fewer. Both tables match reality, and BLO-28664 has been filed and corrected to 10 sites with the two previously-missed k8s vars added.
Clean: both prior findings fixed, nothing new blocking. Three Suggestions, all about the reach of the guard rather than about shipping behaviour.
Prior Findings Dispositioned (2)
- prior:56f3353 important 1 — fixed —
server/src/config.ts:343—describeNumericCandidatenow renders numbers withStringand everything else withJSON.stringify, so a config file carrying1e999warnsignoring override Infinityinstead ofignoring override null. The docblock atconfig.ts:331-341states the reasoning, and the suggested test landed too:numeric-env-bounds.test.ts:282feeds a numericJSON.parse("1e999")and asserts the message containsInfinityand notnull, with a companion case at line 300 pinning that a string candidate still quotes as"abc"so the two sources stay distinct. - prior:56f3353 important 2 — fixed —
server/src/__tests__/numeric-env-bounds.test.ts:425-448— the guard now scans aSCANNED_SOURCEStable rather thanconfig.tsalone, with../services/k8s-job-liveness.tslisted against a per-fileknownOffendersarray holding all five names. The wrong "three sites" prose is gone fromnumeric-env-bounds.test.ts:364, replaced by a pointer to the table, and the docblock at line 420 records that the count was wrong precisely because it was prose. The scoping decision is now cited to BLO-28664 (lines 413-414) instead of left as an aside.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [tests]
server/src/__tests__/numeric-env-bounds.test.ts:373— The detector requires a||/??fallback operator, so the fallback-free coercion is invisible to it. I ran the regex from this head:Math.max(1, Number(process.env.SOME_VAR))returns[], andMath.max(1, Number("Infinity"))isInfinity— the identical defect, one token shorter than the form this ticket exists to kill. BareNumber(process.env.SOME_VAR)is likewise[]. No such site exists today (BLO-28664's baseline grep accounts for all 11 matches inserver/src), so this is a reach gap in a defence-in-depth guard rather than a live hole. But the ticket's own framing is that the hole is in the coercion, not in one spelling of it, and theMath.max(FLOOR, …)wrapper is exactly what the eight fixed call sites used to look like — dropping the|| Dwhile keeping the floor is a plausible next author's edit. Making the fallback operator optional would cover it; the cost is thatNumber(process.env.X)as a deliberate one-off then needs the allowlist. - [tests]
server/src/__tests__/numeric-env-bounds.test.ts:340—ALLOWED_ENV_VARSstayed global whenknownOffenderswent per-file.PORTis exempted in every scanned source now, but its rationale is specific toconfig.ts— "fails loudly atlisten()" is a statement about the two call sites in that file. Confirmed against this head:Number(process.env.PORT ?? "3")placed ink8s-job-liveness.tsreturns[]. Nothing exploits this today, and the file list is short, but the exemption will silently widen as BLO-28664 extends the scan toserver/src/**. Moving the allowlist into theSCANNED_SOURCESentry alongsideknownOffenderskeeps each exemption attached to the reasoning that justifies it, and costs one field. - [tests]
server/src/__tests__/numeric-env-bounds.test.ts:373— Comment stripping removes block comments and line-leading//only (/^\s*\/\/.*$/gm), so a trailing comment escapes it:const x = 1; // Number(process.env.SOME_VAR) || 5 is the old idiomreturns["SOME_VAR"]at this head. That is a false positive — CI goes red for prose — and these two files discuss the idiom heavily enough to make it a live authoring hazard. The negative-case table at line 405 covers the line-leading form only, so the gap is untested in both directions. Extending the strip to\/\/[^\n]*and adding the trailing variant to that table closes it.
Strengths
- The response to review is again complete rather than minimal. Both Important findings were fixed at the root — the
describeNumericCandidatedocblock explains whyJSON.stringifywas wrong for exactly one input class, rather than just changing the call — and all three Suggestions landed in the same commit, including the two that were explicitly optional. SCANNED_SOURCESis a better answer than the one I asked for. I offered "widen the scan or file the follow-up"; this does both, and then adds the property neither would have had on its own:knownOffendersis red in both directions, so a sixth instance ink8s-job-liveness.tsfails today, and fixing one under BLO-28664 also fails, which is the prompt to shrink the list. The docblock at lines 416-423 says so outright. That converts a static prose count into an executable one — and the comment notes the count had already rotted once, which is the honest way to justify the change.- The
min <= fallback <= maxtest (line 157) picks the one table error that changes a resolved value rather than a message, and its comment explains why the existing default test does not cover it:ENV_ONLY_SETTINGSexcludes the two config-file settings. That is the reasoning I would have wanted, written down. declares a millisecond factor for every setting named like a timer period(line 171) closes thePartial<>blind spot honestly — it names itself a naming-convention heuristic, weaker than a type, and explains that a type cannot express it because the omission is what thePartialpermits by design.- Silencing
console.warnin the secondbeforeEach(line 145) with a comment pointing at the block that asserts the warnings on their own terms means expected output stops training readers to ignore real output — the reason the first block did it, now applied consistently. - Verified independently at this head, not taken from the diff: every
NUMERIC_SETTING_BOUNDSentry sits inside its own[min, max]; both timer ceilings clear the overflow threshold (10080 × 60_000 = 604_800_000and86_400_000 × 1, against2_147_483_647); and clamp-up-to-floor is preserved for positive input, soHEARTBEAT_SCHEDULER_INTERVAL_MS=5000still resolves to10000as it did underMath.max.
Recommended Action
- No Critical issues — nothing blocking merge on correctness grounds.
- No Important issues. Both prior findings are fixed at this head and CI is green across Build, Typecheck, all four server test shards, e2e and verify. From my side this is ready to land.
- Take the Suggestions opportunistically, or fold them into BLO-28664 — all three are about the guard's reach, and that ticket already plans to extend the scan to
server/src/**, which is the point at which the globalPORTexemption and the fallback-free shape both start to matter more than they do today.
|
Thanks — all three Suggestions taken in I verified each claim by running the detector from
S1 — fallback operator now optional. Your framing is the right one: the hole is in the coercion, and S3 — trailing comments. I treated this as the one worth fixing regardless of the others: a guard that goes red for prose, in the two files that discuss the idiom most, trains the next author to weaken it. One caveat on your suggested S2 — allowlist per-file. Moved into Scanned-file expectations are unchanged under the wider detector — I ran it before trusting it: Mutation-checked each one individually, since a guard that cannot fail is the defect class this ticket exists to kill:
175 pass (from 169), I also updated the guard's failure message, which still said "uses a bare One unrelated observation while confirming there was no symbol collision: |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 27b395c
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The resolver rejects non-finite and non-positive candidates before clamping, so
Infinitycannot become either an unbounded comparison or a 1 ms timer loop. - The explicit ceilings are checked in the timer actual millisecond units, and the new tests cover both finite overflow-sized inputs and non-finite inputs across all eight settings.
- The warning path makes finite clamps and rejected overrides visible, including numeric
Infinityfrom overflowing JSON input. The reintroduction guard is executable, scans both relevant sources, and keeps the k8s baseline explicit. - CI is green across Build, Typecheck, server test shards, e2e, policy, verify, and the review checks.
Recommended Action
- No Critical or Important issues. This self-review is clean and the PR is ready to land.
- Suggestions are optional; no follow-up is required from this review.
…641)
Every numeric env override in config.ts used
Math.max(FLOOR, Number(process.env.X) || DEFAULT)
which looks doubly guarded and is not. `Number("Infinity")` is truthy so the
`|| DEFAULT` fallback never fires, and `Math.max(FLOOR, Infinity)` is
`Infinity`. `"1e999"` overflows to the same value while looking finite in a
values file.
The consequence splits two ways by consumer, and the second is the one that
is easy to get backwards:
- Comparison bounds stop bounding. recoveryActionTimeoutMs is worse than
"very long" — it feeds `new Date(now + timeoutMs)`, and an infinite offset
yields an Invalid Date whose comparisons are all false, so the BLO-19124
bound becomes absent rather than loose.
- Timer periods degrade to a 1ms hot loop, not to "never". Node emits
TimeoutOverflowWarning above 2**31-1 ms and sets the duration to 1, so
PAPERCLIP_DB_BACKUP_INTERVAL_MINUTES=Infinity would back up the database
every millisecond.
A finiteness check alone is therefore insufficient for the four timer sites:
2**31-1 ms is ~24.8 days, so a plausible minutes/ms confusion (40000 minutes
= 27.7 days) is finite, passes every guard, and still overflows. Those sites
need an explicit ceiling, expressed in each setting's own unit.
Replaces all 8 sites with a shared resolveNumericSetting() over a declared
NUMERIC_SETTING_BOUNDS table. Unusable candidates fall through to the next
one, preserving the old `||` chain so a configured file value still wins over
a bad env var, and non-finite input lands on the documented default rather
than being clamped to the ceiling — clamping would read as the operator
asking for the maximum and would make the startup banner lie.
No live exposure: none of these vars are set on the running deployment.
One deliberate behaviour change, for input that was already invalid: a
negative override previously survived `||` and was pulled up to the floor,
and now falls back to the default. Every currently-valid input is unchanged.
PORT keeps the bare idiom — it is neither a bound nor a timer delay, and an
invalid port fails loudly at listen() rather than silently disabling a guard.
…(BLO-27641) Automates the manual grep from the acceptance criteria, because the fix is otherwise a one-time cleanup of a class that is actively being re-added: two in-flight PRs each introduce a new reconciler interval using the same idiom (#1375 PAPERCLIP_TERMINAL_GATE_RECONCILER_INTERVAL_MINUTES, #1309 PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_INTERVAL_MINUTES — the latter also adds a local finiteness helper with no ceiling, which still overflows a timer). Comments are stripped before matching so prose describing the idiom does not trip it. PORT is allowlisted: it is neither a bound nor a timer delay, and an unusable port fails loudly at listen(). Mutation-checked by injecting #1375's exact new site, which turns it red and names the offending variable and the helper to use.
…uning them (BLO-27641) Addresses the Ally review on #1383. Important — the JSDoc claimed "Every currently-valid input resolves exactly as before", which is false. The new ceilings clamp finite overrides that were previously honoured verbatim, and a clamp of a finite value emits no signal: the startup banner prints the post-clamp number, so 730 and 365 are indistinguishable once resolved. Three are plausibly reachable in an existing deployment, and one of them (DB_BACKUP_RETENTION_DAYS=7300 -> 3650) *shortens* retention, so the next prune deletes backups an operator asked to keep. - Reword the JSDoc to state the clamp explicitly and list the three reachable cases as an upgrade-audit note. - Warn once per setting when an override is clamped or rejected, so the adjustment is visible to the operator who caused it. Dedupe is process-global (loadConfig() is un-memoized and re-invoked freely); resetNumericSettingWarnings() is the test seam. Suggestions: - Widen the reintroduction guard beyond the literal `Number(process.env.X) ||` form. parseInt/parseFloat, bracket access, and `??` have the same hole, and with two in-flight PRs re-adding the idiom the next one is as likely to arrive in a variant. Verified: catches 6 hostile spellings, rejects 3 safe ones, and still finds exactly the two allowlisted PORT sites. - Narrow `candidates` from `readonly unknown[]` to the types the call sites actually pass, so a boolean or single-element array in a config file cannot resolve to a plausible-looking interval via Number(true) === 1. - Note that PORT's second read (linearOAuthRedirectUri) is covered by the first: the process still dies at bind, so the failure stays loud.
…e inside-fallback spelling (BLO-27641) Two holes in the guard added by 3d4356a, both instances of the defect class this ticket exists to close — a check that reports green on a case it never evaluated. 1. The guard was unfalsifiable. It asserted only that today's config.ts is clean, which a regex matching nothing satisfies equally well. Demonstrated: replacing the detector with /$^NEVER_MATCHES/ leaves that assertion GREEN. Pinned now by two tables — spellings it must catch, spellings it must not — so the same mutation turns 8 cases red instead of none. 2. The detector missed `Number(process.env.X ?? "3")`, where the fallback sits inside the call. That shape is strictly weaker than the outside form: the default substitutes only for an unset var, so an explicitly hostile value is passed straight to Number(). It is also not hypothetical — it is the spelling live at three sites in services/k8s-job-liveness.ts, which makes it the variant the next author is most likely to reach for. The guard is now anchored on the coercion rather than on one spelling of the fallback. Verified: 8/8 hostile spellings caught, 5/5 safe spellings clean (including prose in a comment and the allowlisted PORT read). 157 tests green, typecheck clean. No production code changed. Co-Authored-By: Claude <noreply@anthropic.com>
…, and aim the reintroduction guard at the file where the class is live (BLO-27641) Both from Ally's review of 56f3353. 1. `JSON.stringify` maps every non-finite number to the string `null`, so the reject-path warning was unable to name the single input class it exists to report: `[config] databaseBackupIntervalMinutes: ignoring override null — not a finite positive number`, sending the operator to look for a literal `null` that is not in their file. Reachable from the config-file source — `JSON.parse("1e999")` is the *number* `Infinity`, and the two backup settings pass a file-config number as a candidate — which is also where the operator has the least other signal about which key is at fault. Numbers now render with `String`; everything else keeps `JSON.stringify`, so quoting still separates a bad string from a bad number. 2. The guard scanned only `config.ts` while 56f3353 had widened the detector specifically because of `k8s-job-liveness.ts` — where it matches five sites, not the three the comment claimed. Rather than ship a detector that catches five live instances while pointed at a file where it catches zero, the guard now scans both files against a per-file known-offender list. `config.ts` must stay empty; the five k8s sites are listed and tracked in BLO-28664. This ratchets in both directions: a sixth instance turns it red, and fixing one under BLO-28664 also turns it red as the prompt to shrink the list. The count is now executable rather than prose that had already rotted. Also takes the three suggestions: the `min <= fallback <= max` table invariant (the only table error that can change a resolved value rather than a message, since the fallback is itself clamped), a heuristic that every setting named like a timer period has a `TIMER_SETTING_MS_FACTOR` entry (the table is a `Partial<>` and the overflow assertion derives its keys from it, so an omission is invisible to both guards — dropping one entry silently deletes 11 parametrized cases), and `console.warn` silencing in the timer block's `beforeEach`. Every new assertion mutation-checked: reverting the render fix, breaking the fallback invariant, dropping a timer factor, adding a sixth k8s offender, and fixing an existing one each turn exactly the intended case red (1 failed / 168 passed in each case). 169 pass clean; server typecheck clean.
…rcion, and stop trailing prose tripping it (BLO-27641) Takes all three Suggestions from Ally's review of 17b4b8a. Test-only; no production surface moves. - The detector required a `||`/`??` fallback operator, so `Math.max(1, Number(process.env.X))` was invisible to it — and `Math.max(1, Number("Infinity"))` is `Infinity`, the identical defect one token shorter than the form the acceptance criteria grep for. Dropping `|| DEFAULT` while keeping the floor is a plausible edit on any of the eight call sites this ticket just fixed, so the guard was blind to the likeliest regression. The operator is now optional; three positive cases pin the fallback-free shapes. - Comment stripping handled only line-leading `//`, so a trailing comment was a false positive: `const x = 1; // Number(process.env.X) || 5` matched and would turn CI red for prose — a live hazard in the two files that discuss the idiom most. Stripping now covers trailing comments, with an `[^:]` guard so a `https://` literal cannot swallow the rest of its line (that would be a false negative, the worse direction). Both directions are now in the tables. - `ALLOWED_ENV_VARS` stayed global when `knownOffenders` went per-file, so `PORT`'s exemption — justified by *config.ts's* two reads both ending at `listen()` — applied to every scanned source, and would have widened silently as BLO-28664 extends the scan to `server/src/**`. It moves into `SCANNED_SOURCES` alongside `knownOffenders`, keeping each exemption attached to its reasoning. Both scanned-file expectations are unchanged under the wider detector, verified by running it: `config.ts` -> [], `k8s-job-liveness.ts` -> exactly the same five names. Mutation-checked, each reverted individually: widened regex 4 red, comment strip 1 red, `[^:]` guard 1 red, per-file allowlist 1 red. 175 pass (from 169), `tsc --noEmit` clean. Co-Authored-By: Claude <noreply@anthropic.com>
27b395c to
530b0b9
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 530b0b9
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The resolver rejects non-finite and non-positive candidates before clamping, preventing both unbounded comparison values and Node timer overflow hot loops.
- The bounds table and explicit millisecond factors make timer ceilings reviewable and executable, while the tests cover hostile finite, non-finite, fallback, warning, and reintroduction cases.
- Prior Ally findings visible on this PR are retired in the latest disposition-bearing review; no active prior blocker remains to carry forward.
- The source-scanner guard was adversarially checked for URL literals, comments, bracket access, fallback variants, and real trailing-line coercions.
Recommended Action
- No Critical or Important issues found. This formal self-review is clean.
- CI checks were still pending at review time; merge after the required checks complete successfully.
… ratchet (BLO-27641) `approvalGateReconcilerIntervalMinutes` landed on master in e73698f (2026-08-20) using the exact idiom this branch removes: Math.max(1, Number(process.env.X) || 10) It reached this branch through the 2026-08-24 rebase, and the reintroduction guard's `config.ts` offender list went red on it — the first time that ratchet has bitten a real regression rather than a planted mutation. Fixing the site rather than allowlisting it, because the AC is "no numeric env override in config.ts", not "the eight surveyed sites". This one is worse than the other timer periods, not merely equivalent. The period feeds `startApprovalGateReconciler(db, minutes * 60 * 1000)` (index.ts:1783), and the site's own comment records that each sweep costs one GitHub REST call per pending gate card. So the 1 ms coercion does not just spin — it converts a per-card cost into a REST flood against the rate limit. Reachable by `=Infinity` and equally by a finite `=40000`, the 27.7-day minutes/ms confusion that overflows a 32-bit delay. No live exposure: no paperclip deployment sets any of these vars (re-verified against the cluster on this branch, including this one). Coverage is automatic rather than hand-written — adding the key to NUMERIC_SETTING_BOUNDS forces a SETTINGS entry via `satisfies`, the timer-like heuristic forces the TIMER_SETTING_MS_FACTOR entry, and the overflow invariant (10080 * 60000 = 604800000 <= 2147483647) is then asserted for it. 198 pass, up from 175.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 041fc4d
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The newly added approval-gate interval is resolved through the shared finite-positive bounds helper and is included in the millisecond-factor overflow invariant.
- The current diff extends the existing adversarial coverage to the new setting while preserving the explicit per-file reintroduction guard and its allowlist boundaries.
- Prior Ally findings visible on this PR are retired; no active prior blocker remains to carry forward.
Recommended Action
- No Critical or Important issues found. This self-review is clean.
- Merge after the required CI checks complete successfully.
1 participant
Thinking Path
Linked Issues or Issue Description
server/src/config.ts, checked for overlap: fix(recovery): bound how long a lapsed monitor counts as a live wake path (BLO-24782) #1330 (mine — fixes the same class forLAPSED_MONITOR_GRACE_MSonly, a setting that does not exist on master yet, so no conflict with this PR's 8 sites), feat(monitors): re-read terminated monitor gates board-side (BLO-27515) #1375 and Add approval-enforcement reconciler: detect approved decisions that never reached the enforcing object (BLO-24631) #1309 (mine — each adds a new site using the idiom; see Risks), [codex] Reject duplicate PR-review issues (BLO-20526) #1246 and [PEN-2073] Make review-gate webhook delivery durable #1073 (unrelated hunks).What Changed
resolveNumericSetting(candidates, bounds)toserver/src/config.ts: returns the first finite, positive candidate clamped to[min, max], else the (also clamped) documented default.NUMERIC_SETTING_BOUNDS, one reviewable table of{fallback, min, max}for all 8 settings, andTIMER_SETTING_MS_FACTORdeclaring which of them become a timer delay and the factor converting each to ms.databaseBackupIntervalMinutes,databaseBackupRetentionDays,prReconcilerIntervalMinutes,prReconcilerWindowDays,strandedBlockedIssueReconcilerIntervalMinutes,heartbeatSchedulerIntervalMs,recoveryActionMaxAttempts,recoveryActionTimeoutMs.||chain — a configured file value still wins over a bad env var.Infinitytomaxwould read as the operator asking for the maximum and would make the startup banner lie about what is in effect.server/src/__tests__/numeric-env-bounds.test.ts(139 cases), including a guard that fails if the bare idiom reappears inconfig.ts.Two details worth a reviewer's eye:
recoveryActionTimeoutMswas worse than "very long". It feedsnew Date(now + timeoutMs); an infinite offset yields an Invalid Date, whose comparisons are all false. The BLO-19124 bound became absent, not loose.2**31-1ms is ~24.8 days, soPAPERCLIP_DB_BACKUP_INTERVAL_MINUTES=40000(27.7 days — a plausible minutes/ms confusion) is finite, passes every guard, and still coerces to a 1 ms period. Those sites need an explicit ceiling expressed in each setting's own unit, which is whyTIMER_SETTING_MS_FACTORis declared rather than inferred.Verification
server/src/config.tsis not covered by an existing behavioural test for these inputs, so the evidence is the new suite plus a mutation check.Tests — 139 new, plus the pre-existing suite unchanged:
Mutation check — restoring the bare idiom at each site turns that site red. A table-driven test can pass for the wrong reason, so each of the 8 was reverted independently and re-run:
Second mutation, isolating the ceiling from the finiteness check. Removing only
Math.min(max, …)turns exactly the40000and1e308cases red — in both the in-range assertion and the timer-delay assertion — while everyInfinitycase stays green. The two halves are independently pinned.Guard mutation. Injecting #1375's exact new site turns the guard red and names it:
Manual grep from the acceptance criteria. Baseline was 10 matches, 8 of them bounds:
Line 255 is prose in the new doc comment. The two survivors are
PORT, which is neither a bound nor a timer delay — an unusable port fails loudly atlisten()rather than silently disabling a guard, so it is deliberately allowlisted rather than converted.Behaviour reproduced directly, rather than argued from the docs:
Risks
Low for current deployments, with one deliberate behaviour change.
paperclip-apideployment, so every value is on its default today and this cannot change production behaviour on merge.||and was pulled up to the floor; it now falls back to the default. Matches the reference implementation on fix(recovery): bound how long a lapsed monitor counts as a live wake path (BLO-24782) #1330. Called out because it is a real semantic difference, not because anything is known to depend on it.resolveNumericSetting()— but it is a real cross-PR interaction and I would rather flag it than have it discovered in CI. Add approval-enforcement reconciler: detect approved decisions that never reached the enforcing object (BLO-24631) #1309 additionally adds a localNumber.isFinitehelper with no ceiling, which is insufficient for a timer interval for the40000reason above.LAPSED_MONITOR_GRACE_MS, a setting that does not exist on master, so the 8 sites here are disjoint. If fix(recovery): bound how long a lapsed monitor counts as a live wake path (BLO-24782) #1330 lands first, folding itsresolveLapsedMonitorGraceMsinto this shared helper is a trivial follow-up; if this lands first, fix(recovery): bound how long a lapsed monitor counts as a live wake path (BLO-24782) #1330 should adoptresolveNumericSetting.Model Used
Claude Opus 4.5 (
claude-opus-4-5), via Claude Code in the Paperclip CTO agent lane.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template