feat: surface the Fischer increment on TurnContext - #8
Conversation
WebhookHandler now parses state.timeControl (present on every webhook turn) and exposes the per-turn Fischer increment as TurnContext.incrementMillis (Long ms), null for controls without one (unlimited/sudden-death/per-move) or an absent field. Defensive parsing never turns a valid turn into a 400. Bots doing Fischer time management no longer configure the increment out of band. Closes #7 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2026.1.3
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesTurn time control context
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookPayload
participant WebhookHandler
participant TurnContext
participant Strategy
WebhookPayload->>WebhookHandler: Provide state.timeControl.Fischer.incrementSeconds
WebhookHandler->>WebhookHandler: Validate and convert seconds to milliseconds
WebhookHandler->>TurnContext: Construct Clock with incrementMillis
TurnContext->>Strategy: Expose Clock timing data
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/lv/id/jc/dicechess/runtime/WebhookHandler.java`:
- Around line 162-168: Update the incrementSeconds handling in WebhookHandler to
parse the value defensively, treating null, non-primitive, non-numeric, and
out-of-range inputs as absent while leaving incrementMillis null. If conversion
succeeds, scale seconds to milliseconds with overflow-safe arithmetic before
assigning the result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c921157a-c1e7-43ea-9aaf-dcf9b9ff0f2e
📒 Files selected for processing (4)
README.mdsrc/main/java/lv/id/jc/dicechess/runtime/TurnContext.javasrc/main/java/lv/id/jc/dicechess/runtime/WebhookHandler.javasrc/test/java/lv/id/jc/dicechess/runtime/WebhookHandlerTest.java
…ment parse Review feedback on #8: - Group remainingMillis/opponentRemainingMillis/incrementMillis into a nested TurnContext.Clock (a null clock == an untimed game), so the clock is one cohesive value and an impossible state — one side's clock present without the other — is unrepresentable. Best done now, before 0.2.0 ships and consumers pin to the flat fields. - Parse the Fischer increment defensively (numeric, non-negative, within int range): a null/non-numeric/out-of-range incrementSeconds now degrades to a null increment instead of turning an otherwise-valid turn into a 400. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed both review points in 1. Defensive increment parse (CodeRabbit, 2. Group the clock fields into a record (@rabestro). Done —
The pre-merge Linked Issues warning (version bump + consumer updates) is intentional — those are tracked as the follow-ups in the PR description: cut a 0.2.0 release, then switch |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI, Code Quality (Qodana), Javadoc, latest release, Java 25, and MIT — the library-appropriate set (distinct from the bot repos' Play/Leaderboard badges). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/lv/id/jc/dicechess/runtime/WebhookHandler.java`:
- Around line 257-262: Update the increment conversion in the webhook handling
flow around incrementSeconds so getAsLong() is caught as a NumberFormatException
and returns null for malformed values. Preserve the existing numeric range check
and millisecond conversion for valid values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 749da3d9-d1f1-4705-b997-644f3fa96ebd
📒 Files selected for processing (4)
README.mdsrc/main/java/lv/id/jc/dicechess/runtime/TurnContext.javasrc/main/java/lv/id/jc/dicechess/runtime/WebhookHandler.javasrc/test/java/lv/id/jc/dicechess/runtime/WebhookHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
CodeRabbit follow-up on #8: getAsLong() documents a NumberFormatException for untrusted values. The isNumber() guard already routes to the non-throwing longValue() path in current Gson, but wrapping the conversion keeps TurnContext.Clock's 'no input can throw' contract true against Gson's documented API rather than its internals — a malformed increment stays a null increment, never a 400. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
1 participant
What
WebhookHandlernow parsesstate.timeControlfrom the webhook envelope and surfaces the per-turn Fischer increment as a newTurnContext.incrementMillisfield (Long, milliseconds). It isnullfor any control without an increment (unlimited, sudden-death, per-move) or when the field is absent — the parsing is defensive, so a surprising shape never turns a valid turn into a 400.Why
play-api already sends the full
timeControlon every webhook turn (PublicGameState.timeControl), but the library dropped it — so a bot doing Fischer time management had to configure the increment out of band (e.g.dicechess-bot-gcp'sLADDER_INCREMENT_MSenv var). Now the increment rides the wire and every webhook bot gets correct Fischer budgeting for free, on whatever control the server pairs.Wire shape
timeControlis a single-key object (Circe's derived enum codec); onlyFischercarries an increment:{"Fischer":{"initialSeconds":300,"incrementSeconds":3}}->incrementMillis = 3000{"Unlimited":{}},{"SuddenDeath":{...}},{"PerMove":{...}}, or absent ->nullTests
Three new
WebhookHandlerTestcases (Fischer -> ms, non-Fischer -> null, absent -> null).mvn testgreen: 32 tests, 0 failures.Follow-ups (not in this PR)
dicechess-bot-gcpthen drops itsLADDER_INCREMENT_MSworkaround and readsctx.incrementMillis().Closes #7