Repository navigation
Fix agent service heartbeat JSON body - #4374
Conversation
The hosted agent registration client declared JSON but sent an empty request body, so strict control-plane parsing rejected every heartbeat and deployment health checks correctly failed. Send the minimal JSON object required by the existing endpoint contract and pin that behavior in the lifecycle test. Constraint: The API heartbeat endpoint intentionally requires a JSON object. Rejected: Relax the API parser | weakens a shared boundary and hides malformed clients. Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep heartbeat requests aligned with the strict control-plane schema. Tested: Focused registration suite, framework typecheck, formatting, and lint.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ ApprovedFixes the agent service heartbeat to send the required JSON object to the strict control-plane endpoint, resolving registration failures. Wire contract is locked in the hosted registration lifecycle test. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Important Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
kwakayama
left a comment
There was a problem hiding this comment.
Review: 92/100 — correct, minimal fix with a regression test that pins the wire contract
Strengths
- Root cause is real and precisely diagnosed:
createHeaders()setsContent-Type: application/jsonon every request, butsendHeartbeatRequestsent no body, so a strict control-plane parser would reject the heartbeat.body: JSON.stringify({})makes the request self-consistent with its own declared content type. - Fix is applied at the right boundary (the client), not by loosening server-side/API parsing — matches the PR description's stated intent and is the correct fix for a shared contract.
- Test change is a good regression guard: it asserts
calls[1].init.body === "{}"on the heartbeat call specifically (not the registration call), so it will actually catch a future regression rather than just re-asserting the currently-passing behavior. - Change is tightly scoped (+6/-0 across 2 files, 1 commit) — no unrelated churn, no API surface change.
- PR description documents red/green verification, typecheck, fmt, lint, and the full pre-push suite, which is more rigor than this diff size would normally require.
Minor/optional (not blocking)
mergeable_stateis currentlyblocked— worth confirming that's just outstanding required reviews/CI (which were still queued at review time) and not an actual merge conflict.- Very small nit, non-blocking:
body: JSON.stringify({})is a magic literal repeated implicitly against the registration path'sJSON.stringify(buildRegistrationRequest(input)). A named constant (e.g.EMPTY_JSON_BODY = "{}") shared between the heartbeat call and its test assertion would make the "this must literally be{}" contract self-documenting, but this is style preference, not a defect.
This is a textbook small bugfix: clear root cause, minimal diff, and a test that would have failed before the fix and passes after. No functional, security, or design concerns.
Generated by Claude Code
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe heartbeat POST request now sends an explicit empty JSON object. The heartbeat test verifies that the request body is ChangesHeartbeat payload
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to The heartbeat now sends the required JSON body while preserving the existing endpoint, authentication, and retry behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
kwakayama
left a comment
There was a problem hiding this comment.
No actionable findings.
The heartbeat now sends the required JSON object at src/agent/service/registration.ts:449 while preserving its existing POST method, authentication headers, endpoint, and abort behavior (src/agent/service/registration.ts:446-451). The regression test pins the exact wire body to "{}" at src/agent/service/registration.test.ts:291-295.
Rubric: correctness 40/40, tests 20/20, reliability/security 15/15, maintainability 15/15, scope/docs 10/10.
Review-Gate:
Reviewer: Codex
Reviewed-SHA: 17f34e6
Score: 100/100
Actionable-Findings: 0
Verdict: APPROVE
Summary
Why
The generic hosted agent replicas register successfully, then every heartbeat fails because the framework declares JSON without sending a body. The API deployment health gate correctly detects both registrations as stale.
Verification
deno task test:file src/agent/service/registration.test.ts(27 steps)deno task typecheckdeno fmt --checkdeno lintThis fixes the client at the owning boundary. It does not relax the API parser or deployment health gate.
Summary by CodeRabbit
Bug Fixes
Tests