fix(duration): reject junk between duration tokens instead of skipping it - #343
Open
devYRPauli wants to merge 3 commits into
Open
fix(duration): reject junk between duration tokens instead of skipping it#343devYRPauli wants to merge 3 commits into
devYRPauli wants to merge 3 commits into
Conversation
…g it
The multi-unit branch of parseDuration scans with a global regex and only
checks that the final match ends at the end of the string. exec() skips
any text that does not match, so junk before or between the unit tokens
is silently dropped and a wrong duration is returned.
parseDuration('10gibberish5s', fallback) -> 5000
parseDuration('abc5s', fallback) -> 5000
parseDuration('1h!30m', fallback) -> 5400000
Require each match to start where the previous one ended, so the tokens
have to cover the whole string contiguously. Malformed input now returns
the caller's fallback, which is what the trailing-junk case already did.
parseDuration backs the --browser-timeout, --browser-input-timeout,
--browser-recheck-delay and related CLI options, so a mistyped value
silently produced a different timeout than the user asked for.
Adds tests/duration.test.ts; the file had no test coverage.
clawsweeper
Bot
added
rating: 🧂 unranked krab
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 1, 2026, 7:21 PM ET / 23:21 UTC. ClawSweeper reviewWhat this changesThe branch requires multi-unit duration tokens to cover the whole input and makes malformed Merge readinessThis PR remains necessary: current Priority: P2 Review scores
Verification
How this fits togetherOracle’s browser CLI accepts duration strings for timeouts and retry delays, converts them to milliseconds, and passes the resulting configuration to browser-session execution. This PR hardens that parsing boundary so malformed optional flags cannot be partially interpreted without operator feedback. flowchart LR
A[Browser CLI duration flags] --> B[Duration parser]
B --> C[Contiguous token validation]
C --> D[Fallback and warning]
D --> E[Browser configuration]
E --> F[Browser session runtime]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the narrow validation and warning change so valid browser durations remain unchanged while malformed values consistently use their documented defaults with an operator-visible diagnostic. Do we have a high-confidence way to reproduce the issue? Yes. Current Is this the best way to solve the issue? Yes. The contiguous-match guard fixes the parser at its source, and the browser configuration helper retains existing defaults while making rejected operator input visible instead of silently changing runtime timing. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 68b8c51b0ee0. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (19 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
The contiguity guard makes a malformed value fall back instead of being partially parsed, but buildBrowserConfig passed a real default as the fallback, so a bad --browser-timeout silently became the default with no signal to the operator. Route the browser duration options through a helper that parses with a NaN fallback, warns naming the option, the rejected value and the fallback in effect, then returns the fallback.
clawsweeper
Bot
added
proof: sufficient
The helper serves eleven --browser-* duration options but only --browser-timeout was exercised. Add a zero-fallback option, a second non-zero-default option, and a combined case asserting each malformed option produces its own correctly named warning and its own fallback.
clawsweeper
Bot
added
rating: 🦞 diamond lobster
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The multi-unit branch of
parseDuration(src/duration.ts) scans with a global regex:exec()on a global regex skips forward past anything that does not match, and the only structural check afterwards is that the last match ends at the end of the string. Junk before or between the unit tokens is therefore silently dropped rather than rejected:10gibberish5s5000abc5s50001h!30m54000005s!!The trailing-junk case is already rejected, which is what makes the other three look like an oversight rather than intent:
5s!!falls back, butabc5squietly returns 5 seconds.parseDurationbacks--browser-timeout,--browser-input-timeout,--browser-attachment-timeout,--browser-recheck-delay,--browser-recheck-timeoutand--browser-reuse-wait(src/cli/browserConfig.ts). A typo in one of those silently produces a different timeout than the user asked for, with no warning.Fix
Require each match to begin where the previous one ended, so the tokens must cover the string contiguously:
Malformed input now returns the caller's fallback, matching what the trailing-junk case already did.
Verification
Behaviour before and after:
1h30m540000054000002h15m30s813000081300001 h 30 m54000005400000500ms/30s/2h10gibberish5s5000abc5s50001h!30m54000001h30,zzz,5s!!Every valid input is unaffected; only the three silently-wrong cases change.
src/duration.tshad no test file, so this addstests/duration.test.tscovering bare numbers, single units, multi-unit, whitespace, the fallback cases and the three above. Confirmed the new junk-rejection test fails against the unpatched module (AssertionError: expected 5000 to be 42) and passes with the fix.tests/duration.test.ts: 4 passedbrowserConfig,browserDefaults,dryRun,dryRun.coverage,sessionRunner,searchPersist,followup,mcp/consult): 138 passedoxlint: clean;oxfmt --check: clean;tsc --noEmit: no errors in the changed filesFollow-up: surface rejected browser durations
Fair point on the review. The parser change is what makes a malformed value fall back rather than be partially parsed, but
buildBrowserConfigpassed a real default as the fallback, so--browser-timeout 1h!30mquietly became the default with nothing shown to the operator.The eleven
--browser-*duration options now go through one helper that parses with aNaNfallback, warns naming the option, the rejected value and the fallback actually in effect, then returns that fallback. Deliberately still a warning and not a hard error: erroring would be a bigger behaviour change than the problem calls for, and these are optional tuning flags. The wording andchalk.yellow+Warning:prefix followsrc/cli/bundleWarnings.ts, which is the existing convention for non-fatal CLI input problems.src/duration.tsis untouched.Real-behavior proof
Real CLI runs, valid value then malformed value.
--dry-run summarybecause a live browser run needs a signed-in Chrome session, but this still goes through the real entrypoint and the realbuildBrowserConfig(), stopping just before Chrome launches:The valid run prints no warning. Calling the same production config builder directly makes the resulting values explicit:
Checks:
Two tests added to
tests/cli/browserConfig.test.ts: a valid explicit duration asserting no warning is logged, and a malformed one asserting both the fallback value and the exact warning text.Wider test coverage for the shared helper
Good point that only
--browser-timeoutwas exercised while the helper backs eleven options. Added coverage for the shapes that could actually differ rather than duplicating one test eleven times:--browser-recheck-delay, whose fallback is0(the case most likely to be mishandled, since0is falsy)--browser-auto-reattach-timeout, a second option with a real non-zero default--browser-input-timeout,--browser-reuse-waitand--browser-auto-reattach-intervalmalformed together, asserting each resulting config field independently plus all three distinct warning strings andtoHaveBeenCalledTimes(3), so a copy-pasted option name in one call site could not pass unnoticedWhile in there I also audited all eleven call sites against their flag names and default constants. All eleven are correct, so no source change was needed.