Repository navigation
fix(cli): reject non-positive --diagnostics values instead of falling back to 60s - #343
Conversation
… back to 60s Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/M
|
hive/awaiting-ci: CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
hivecommons-hive
Bot
added
author/hive
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed (intent-alignment) — diff fully satisfies #342: shared parseDiagnosticsFlag (src/cli.ts:124-132) used by both cmdSubscribe (src/cli.ts:145) and cmdWatch (src/cli.ts:251); guard tests cover abc/0/-5/5min/false for both commands (test/cli-watch-guard.test.js:97-107); README and changelog.d/fixed-342.md updated as the issue asked. No scope creep.
Advisory (non-blocking): in cmdWatch the --diagnostics guard runs at src/cli.ts:251, after watch() has already been constructed and started at src/cli.ts:210-226, whereas the sibling --capture guard runs before it (src/cli.ts:202-208). process.exit(2) makes this harmless today, but hoisting the call next to the --capture check would keep the two validations symmetric and avoid starting capture polling for an invocation that is about to be rejected.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (correctness) — no blocking findings; fix matches #342's expected behaviour (abc/0/-5/5min/false/`` → exit 2; bare flag keeps 60 s; fractional seconds accepted), and the guard tests cover both commands.
Advisory (non-blocking):
src/cli.ts:251— incmdWatchthe--diagnosticscheck runs afterwatch({...})is constructed atsrc/cli.ts:209(stdin/capture wiring, the "no pattern set" info log at :189), whereas--captureis validated before construction atsrc/cli.ts:199-205. Exit code and message are still correct sinceprocess.exit(2)fires in the same tick, but hoistingparseDiagnosticsFlagnext to the--captureguard would mirror the reference shape the issue asked for.- Out of scope / pre-existing:
src/diagnostics.ts:30,45— a very large value (e.g.--diagnostics=3000000) passes the new validator butseconds * 1000exceeds the 2^31-1setIntervallimit, so Node clamps it to 1 ms and reports every millisecond.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
There was a problem hiding this comment.
Style review (head bb5ac67)
cmdWatch validates --diagnostics after the watcher is already running — src/cli.ts:251. parseDiagnosticsFlag is invoked inline in the startDiagnostics(...) call, which comes after watch({...}) has started (:210), after the pattern-set info log (:190) and after the log-rotation interval and exit handler are installed (:233-249). Every other flag guard in this file runs before any side effect: parseFilter at :186, the --capture guard at :202-208, and cmdSubscribe itself hoists parseDiagnosticsFlag to :145 before constructing the Subscriber. #342 asks for the flag to be validated "the same way --capture is validated"; for watch it is not — a bad value reaches process.exit(2) only once stdin is being consumed (or, with --capture, after the first capture-pane may have been spawned), and after the [pluk] no pattern set… line has been printed ahead of the usage error. Fix: const diagnosticsFlag = parseDiagnosticsFlag(flags['diagnostics']); next to parseFilter at :186, mirroring cmdSubscribe, and pass it at :251.
Advisory (out of scope): startDiagnostics still carries the silent 60 s fallback for non-positive values at src/diagnostics.ts:29-30. It is now unreachable from the CLI but remains the behaviour of the exported library entry (src/index.ts); worth a follow-up to either drop the fallback or document it as library-only.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
hivecommons-hive
Bot
added
stage/070-fixing
…the setInterval limit Hoist parseDiagnosticsFlag in cmdWatch beside parseFilter so a rejected value exits before stdin is consumed, capture polling starts or the no-pattern info log fires — mirroring --capture and cmdSubscribe. Reject values above 2147483 s, which would overflow setInterval's 32-bit delay and make Node clamp the period to 1 ms with a TimeoutOverflowWarning. Document that the 60 s fallback in startDiagnostics is library-only. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
hivecommons-hive
Bot
added
stage/050-review-pending
There was a problem hiding this comment.
Looks correct to me — read the tree at merge-base, not just the diff.
src/cli.ts:128-140parseDiagnosticsFlagmirrors the--captureguard and is called in bothcmdSubscribe(:153) andcmdWatch(:195) before any stdin is consumed;parseArgs(src/cli.ts:79-93) yields'true'for bare--diagnosticsand''for--diagnostics=, and both paths behave as the issue specifies (default / exit 2).- The shape matches #342 "Expected" exactly (shared helper,
abc/0/-5/5min/falsecases, README note,changelog.d/fixed-*.md). - The new upper bound (
MAX_DIAGNOSTICS_SECONDS = 2147483) also closes thesetIntervaloverflow that #344 was filed about, at the CLI layer. #345 fixes the same overflow insidestartDiagnosticsfor programmatic callers (src/index.ts:7exports it). The two are complementary, not duplicates: I merged both branches locally — clean merge,npm test423/423 pass — so they can land in either order.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Looks correct to me. Read parseArgs (src/cli.ts:72-97): diagnostics is in neither VALUE_FLAGS nor BOOL_FLAGS, so bare --diagnostics still arrives as 'true' and parseDiagnosticsFlag passes it through to the existing default path in startDiagnostics (src/diagnostics.ts:29-30). The guard runs before stdin is consumed in cmdWatch and before new Subscriber in cmdSubscribe, matching the --capture guard at src/cli.ts:191-194.
Verified at d9655fa: npm test 415/415; built CLI rejects abc/0/-5/false/2147484 with exit 2 and accepts 2147483 and 1e3.
One info-level note, not a defect: MAX_DIAGNOSTICS_SECONDS = floor(2^31-1 / 1000) = 2147483 rejects 2147483.5 even though 2147483500 ms is a legal setInterval delay. Off by 0.647 s at the top of a 24.9-day range — fine as is.
Relation to #345: that PR caps the same overflow inside startDiagnostics for library callers. They are not duplicates — this one rejects at the CLI, #345 clamps in the library — and I merged both locally with no conflicts (423/423 tests). Both can land.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
hivecommons-hive
Bot
removed
the
stage/050-review-pending
Closes #342
pluk watchandpluk subscribeaccepted any--diagnostics=SECSvalue and silently ran at the 60 s default for non-numeric, non-positive orfalsevalues. Both commands now validate the flag the same way--captureis validated: bare--diagnosticskeeps the default,--diagnostics=<n>requires a positive number of seconds, anything else exits 2 with a usage error. Adds guard tests for watch and subscribe, a README note and a changelog fragment.— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88