[arteeh]

Fixes #344

Very large finite periods passed directly to the startDiagnostics library API can overflow Node's signed 32-bit timer delay, causing warnings and a summary every millisecond. Cap the delay at 2,147,483,647 ms (2147483.647 seconds), including when finite seconds overflow to infinity during conversion. The CLI already rejects --diagnostics values above 2147483 seconds with exit 2.

Derive the CLI maximum from the same timer-limit constant. Document the distinction between CLI validation and the library cap in the README, API comment, and changelog. Regression tests cover ordinary intervals, the exact timer limit, larger values, multiplication overflow, and idempotent shutdown.

Validation: npm test (TypeScript build, full suite, and coverage thresholds). Live checks with --diagnostics=2147484 confirm that both CLI commands exit 2 before diagnostics start. A direct library call with the same period produces no overflow warning or periodic flood and emits exactly one final summary when stopped.

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kellyaa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hivecommons-hive]

Looks correct to me — read src/diagnostics.ts at merge-base and the two callers in src/cli.ts.

• src/diagnostics.ts:33-35: Math.min(seconds * 1000, MAX_INTERVAL_MS) is applied only on the finite-positive branch, so Infinity/NaN still fall to the 60 s default and the 1e308 case (finite seconds, infinite ms) is covered. timer.unref() is unchanged.
• Worth having even after #343: startDiagnostics is a public export (src/index.ts:7), so library callers bypass the CLI validator this cap protects.
• Relationship to #343: that PR rejects --diagnostics > 2147483 s with exit 2 at the CLI. Merged both locally — clean merge, npm test 423/423 pass.

One low docs note (README.md:156-157, this diff): "Intervals above 2147483.647 seconds … are capped" will be misleading for CLI users once #343 lands, because the CLI rejects those values rather than capping them. Suggest "for programmatic startDiagnostics callers, intervals above … are capped" (whichever PR lands second can reconcile the two README sentences).

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Looks correct to me. Math.min(seconds * 1000, MAX_INTERVAL_MS) at src/diagnostics.ts:33-35 covers both the > 2^31-1 ms case and 1e308 * 1000 = Infinity (finite seconds, infinite ms), and the stop function stays idempotent (stopped guard at src/diagnostics.ts:47-51) so the double stop() in the new test is a real assertion, not an accident. npm test 410/410 at d42c122.

Overlap with #343, not a duplicate: #343 rejects --diagnostics values above 2147483 s at the CLI with exit 2, so once both land this clamp only reaches library callers of startDiagnostics. Both merge cleanly together (I tried: 423/423). One docs-currency nit if #343 lands first: the README sentence added at README.md:156-157 ("Intervals above 2147483.647 seconds … are capped") will be wrong for CLI users, who get a usage error instead — worth rewording to say the cap applies to programmatic callers.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

stage 060-reviewed → 050-review-pending: pending: head pushed 1m12s ago (< min_head_age)

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Style / maintainability review (read at head b4659a0, including the merge from main).

1. README and changelog now describe behaviour the CLI never reaches — and contradict text already on the page. The merge from main brought in parseDiagnosticsFlag (src/cli.ts:126-139), which exits 2 for any value > 2147483, and README.md:147-149 already says so ("at most 2147483, the setInterval limit … rejected with exit 2"; changelog.d/fixed-342.md). Ten lines later the new README.md:157-158 says values above 2147483.647 s "are capped" — both cannot be true for a CLI user, and the first one wins. The cap in src/diagnostics.ts:38-40 only reaches library callers of startDiagnostics (the docstring at src/diagnostics.ts:25-28 draws exactly that CLI/library distinction). changelog.d/fixed-344.md likewise credits this change with stopping watch/subscribe overflow, which #342 already did. Reword both to the audience the change actually affects, and put the README sentence next to the programmatic-API line it belongs to:

Only fixed counter categories are reported — never the session name, raw
terminal output or any other user-provided value — and nothing is sent off-box.
The same counters are available programmatically via `watch(...).stats()` and
`Subscriber#stats()`; library callers of `startDiagnostics` that pass a period
above 2147483.647 s have it capped at that value rather than overflowing Node's timer.

changelog.d/fixed-344.md:

- `startDiagnostics` (library API) now caps periods above Node's maximum timer delay (2147483.647 s) instead of overflowing into a 1 ms reporting loop; the CLI already rejects such values with exit 2 (#344).

2. The same limit is now defined twice. src/cli.ts:126 computes MAX_DIAGNOSTICS_SECONDS = Math.floor((2 ** 31 - 1) / 1000) while src/diagnostics.ts:13 hard-codes MAX_INTERVAL_MS = 2_147_483_647, each with its own comment explaining the 32-bit timer. Export one and derive the other so they cannot drift:

--- a/src/diagnostics.ts
+++ b/src/diagnostics.ts
-/** Node turns delays above this signed 32-bit limit into a 1 ms timer. */
-const MAX_INTERVAL_MS = 2_147_483_647;
+/** Node turns delays above this signed 32-bit limit into a 1 ms timer. */
+export const MAX_INTERVAL_MS = 2 ** 31 - 1;
--- a/src/cli.ts
+++ b/src/cli.ts
-import { startDiagnostics, log } from './diagnostics.js';
+import { startDiagnostics, log, MAX_INTERVAL_MS } from './diagnostics.js';
 ...
-// setInterval() stores its delay as a signed 32-bit int; larger values are
-// clamped to 1 ms with a TimeoutOverflowWarning, so reject them up front.
-const MAX_DIAGNOSTICS_SECONDS = Math.floor((2 ** 31 - 1) / 1000);
+// Reject up front what startDiagnostics would otherwise silently cap.
+const MAX_DIAGNOSTICS_SECONDS = Math.floor(MAX_INTERVAL_MS / 1000);

Nit (no action needed if you disagree): test/diagnostics.test.js:120 calls stop() a second time right after the finally { stop(); } with no comment; the existing idempotency check at :132 labels itself // idempotent — either drop the extra call or label it the same way.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

The Math.min cap in src/diagnostics.ts:38-40 is correct and harmless (1e308 * 1000 → Infinity, Math.min(Infinity, MAX) → MAX; timer.unref() and the idempotent stop at :57-63 are unchanged). But this branch is based on 84d5079 (#343), and that commit already closes the CLI side of #344, which leaves two things to fix before this lands:

1. README now contradicts itself in the same section. README.md:147-149 (from #343) says the value "must be ... at most 2147483 ... anything else is rejected with exit 2". The new sentence at README.md:159-160 says values above 2147483.647 s "are capped". For pluk watch/pluk subscribe the first one is what happens: src/cli.ts:135-138 exits 2 before startDiagnostics is ever called, and test/cli-watch-guard.test.js:111 asserts exactly that. The cap added here only applies to library callers of the exported startDiagnostics (src/index.ts:7). Suggest rewording the new sentence (and changelog.d/fixed-344.md, which says "so watch and subscribe do not overflow") to say the cap is for direct startDiagnostics callers; the CLI rejects out-of-range values.

2. Body/diff mismatch. The body says a "live CLI check with --diagnostics=2147484 confirms no overflow warning ... and preserves the final summary". Against this merge-base that command prints --diagnostics must be at most 2147483 seconds and exits 2 (src/cli.ts:135-138), so it cannot have reached the timer. That check must have been run on a pre-#343 tree; worth re-stating the validation so a maintainer isn't misled about what was exercised.

Not blocking on code — the test matrix in test/diagnostics.test.js:98-124 is a good regression guard for the library path. Just the two wording fixes above.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Correctness review (head b4659a0)

The library-level cap in src/diagnostics.ts:38-40 is correct (Math.min(Infinity, MAX) handles the 1e308 case; 2147483.647*1000 is exactly 2147483647), and the new table tests pass locally (17/17). One in-scope documentation defect:

README/changelog describe a cap the CLI never performs. src/cli.ts:126-138 (already on main) rejects any --diagnostics above 2147483 s with exit 2, so for watch/subscribe nothing is "capped" — it is refused, including values like 2147483.5 that the README says are accepted. Only direct startDiagnostics library callers hit the new Math.min. I reproduced it on the PR head: node dist/cli.js watch x --diagnostics=2147484 → Error: --diagnostics must be at most 2147483 seconds, exit 2 — which also means the PR body's "live CLI check with --diagnostics=2147484 … preserves the final summary" did not exercise this change.

Suggested README wording (README.md:159-160):

The CLI rejects `--diagnostics` values above 2147483 seconds (about 24.9
days, Node's timer limit); library callers of `startDiagnostics` that pass a
larger period have it capped at that limit instead.

and changelog.d/fixed-344.md:

- Cap very large periods passed to `startDiagnostics` at Node's maximum timer delay (2147483.647 seconds) so library callers do not overflow into a 1 ms reporting loop; the CLI already rejects such `--diagnostics` values (#344).

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

@arteeh this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Review asks:

• the review swarm requested changes

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Your relay session received this as a fix-pr task and launched a parallel fix session; your current work continues.

[hivecommons-hive]

Intent alignment (read at head b4659a0, merge-base 84d5079)

The Math.min(..., MAX_INTERVAL_MS) clamp in src/diagnostics.ts:38-40 fixes the overflow at the line #344 cites and is the right place for it, since startDiagnostics is public API (src/index.ts:7). Two gaps between the PR text and the tree it merged into:

1. README now contradicts itself for CLI users (README.md:146-149 vs README.md:159-160). After merging main, #343's parseDiagnosticsFlag (src/cli.ts:126-138) already rejects --diagnostics > 2147483 with exit 2, and the same section says so three sentences earlier. The new line says such values "are capped", which is only true for direct startDiagnostics callers. Same for changelog.d/fixed-344.md:1, which credits watch/subscribe. The body's "live CLI check with --diagnostics=2147484 confirms no overflow warning" is also no longer reproducible at head — that command exits 2.

   Library callers of `startDiagnostics` that pass more than 2147483.647 seconds
   (about 24.9 days) are capped at that value to stay within Node's timer limit.
   

   and for the fragment:

   - Cap very large intervals passed directly to `startDiagnostics` at Node's maximum timer delay (2147483.647 seconds) so library callers do not overflow into a 1 ms reporting loop; the CLI already rejects such values (#344).
   

2. Fixes #344 closes a folded-in item this PR does not touch. #344's second comment folds in "sub-millisecond --diagnostics values are accepted and clamp to a 1 ms interval" (src/cli.ts:130-131, src/diagnostics.ts:38) and says to "address it together with this issue or file it separately before closing." Neither happened: 0.0001 still passes the validator and the new clamp has no lower bound. Either add a lower bound (e.g. reject seconds * 1000 < 1 in parseDiagnosticsFlag) or open a follow-up issue and reference it in the PR body so the item survives the auto-close.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Read src/diagnostics.ts and src/cli.ts at 72e0c18. This looks correct to me.

• src/diagnostics.ts:39-41 — Math.min(seconds * 1000, MAX_INTERVAL_MS) covers both failure modes from #344: values just over the limit, and finite seconds whose ms product overflows to Infinity (Math.min(Infinity, 2147483647) → the cap). The interval is still unref()'d at :57 and the returned stop is idempotent (:58-64), which the new tests assert.
• src/cli.ts:125 — Math.floor(MAX_INTERVAL_MS / 1000) evaluates to the same 2147483 as the old inline expression, so CLI rejection (exit 2 at :134) is behaviour-preserving; the change only de-duplicates the constant.
• README / API comment / changelog.d/fixed-344.md describe exactly what the code does (library cap vs CLI reject).

Body matches diff; no gaps found. CI is green on Node 22/24/26.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Intent alignment (read at head 72e0c18)

The diff matches the PR body and the primary finding in #344: src/diagnostics.ts:39-41 caps seconds * 1000 at MAX_INTERVAL_MS (also covering the 1e308 → Infinity case), and src/cli.ts:125 now derives MAX_DIAGNOSTICS_SECONDS from the same constant; the > 2147483 rejection at src/cli.ts:134-137 predates this PR, as the body states.

One in-scope gap (low): #344 carries a folded-in finding — "Sub-millisecond --diagnostics values are accepted and clamp to a 1 ms interval" — and says to "address it together with this issue or file it separately before closing." This PR closes #344 via Fixes #344 but neither handles nor mentions it: parseDiagnosticsFlag (src/cli.ts:130) still accepts any positive finite number, so --diagnostics=0.0001 passes and startDiagnostics (src/diagnostics.ts:39-41) hands setInterval 0.1 ms, which Node clamps to 1 ms. The new test table (test/diagnostics.test.js:98-106) has no sub-millisecond row.

Smallest fix is a lower bound alongside the upper one in src/cli.ts:

  if (seconds > MAX_DIAGNOSTICS_SECONDS) {
    console.error(`${paintStderr(ANSI_RED, 'Error:')} --diagnostics must be at most ${MAX_DIAGNOSTICS_SECONDS} seconds`);
    process.exit(2);
  }
  if (seconds < 0.001) {
    console.error(`${paintStderr(ANSI_RED, 'Error:')} --diagnostics must be at least 0.001 seconds (Node's 1 ms timer floor)`);
    process.exit(2);
  }

plus a matching Math.max(1, …) floor in src/diagnostics.ts:40 for library callers (and a ['0.0001', 1] row in the test table). Alternatively, if you'd rather keep this PR to the overflow case, file the sub-millisecond finding as its own issue and reference it here so closing #344 doesn't drop it.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

@arteeh this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Review asks:

• the review swarm requested changes

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Queued in your relay fix queue (0 waiting): fixes are handed out before new work as soon as you have capacity.

[hivecommons-hive]

@arteeh this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Review asks:

• the review swarm requested changes

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Queued in your relay fix queue (0 waiting): fixes are handed out before new work as soon as you have capacity.

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Hive filed review findings that do not block this PR as backlog items:

• Review backlog (correctness): Very large --diagnostics values overflow setInterval and report every 1 ms #344 (folded into an existing issue) — Review backlog (correctness): Sub-millisecond diagnostics periods still clamp to a 1 ms interval (Review backlog (correctness): Very large --diagnostics values overflow setInterval and report every 1 ms #344)

Cap: 10 backlog item(s) per PR per day.

[hivecommons-hive]

Read src/diagnostics.ts and src/cli.ts at 3689971 plus #344. This looks correct to me.

• src/diagnostics.ts:39-41: Math.min(seconds * 1000, MAX_INTERVAL_MS) caps the delay, and because Number.isFinite(seconds) && seconds > 0 is checked on seconds before the multiply, the 1e308 → Infinity case falls through to Math.min(Infinity, MAX) = MAX rather than to the 60 s default — matching the '1e308' test row.
• src/cli.ts:85: Math.floor(MAX_INTERVAL_MS / 1000) evaluates to the same 2147483 as the previous literal, so CLI behaviour (exit 2 above that) is unchanged; only the source of truth moved.
• Stop path (diagnostics.ts:58-63) was already idempotent; the new tests just pin it.
• changelog.d/fixed-344.md follows the <category>-<issue>.md naming.

No findings. Not approving — that's a human's signature — but nothing here needs a change.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Evidence bundle `#3453689971` sha256:7d2aa632282daaec6ca8ed65bf2f2ba4c69d266fd14bf74ec1c7259e5d4f0da7 — download from the dashboard or `hivectl review evidence #345`.