Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a GitHub Dispatch job-polling scheduler worker that continuously fetches dispatch jobs from a cloud license service, enqueues corresponding DAG runs with webhook-derived runtime parameters, tracks job state on disk, and reports completion status back to the cloud. Supporting changes add cloud client methods, license credential structures, webhook parameter formatting, session message persistence for UI actions, and frontend navigation handling for UI-action messages. ChangesGitHub Dispatch Feature
OpenAI Codex Tool-Call ID Fix
Sequence DiagramsequenceDiagram
participant Scheduler
participant CloudClient
participant GitHubDispatchWorker
participant DAGStore
participant Queue
participant Tracker as Tracker Store
participant DAGRunStore
Scheduler->>GitHubDispatchWorker: Start()
Note over GitHubDispatchWorker: pullLoop & reportLoop concurrent
GitHubDispatchWorker->>CloudClient: PullGitHubDispatch()
alt Job Available
CloudClient-->>GitHubDispatchWorker: GitHubDispatchJob
GitHubDispatchWorker->>DAGStore: Fetch DAG config
DAGStore-->>GitHubDispatchWorker: DAG definition
GitHubDispatchWorker->>Queue: EnqueueWebhookRun<br/>(with runtime params)
GitHubDispatchWorker->>Tracker: Upsert(JobID, pending_accept)
GitHubDispatchWorker->>CloudClient: AcceptGitHubDispatch()
Tracker->>Tracker: Update phase: accepted
else No Job
CloudClient-->>GitHubDispatchWorker: nil
Note over GitHubDispatchWorker: Sleep (idleDelay)
end
par Report Loop
GitHubDispatchWorker->>Tracker: List()
Tracker-->>GitHubDispatchWorker: [TrackedJob]
GitHubDispatchWorker->>DAGRunStore: GetAttempt(dagRunID)
alt DAG Complete
DAGRunStore-->>GitHubDispatchWorker: Terminal Status
GitHubDispatchWorker->>CloudClient: FinishGitHubDispatch<br/>(with status)
GitHubDispatchWorker->>Tracker: Delete(JobID)
else DAG Active
Note over GitHubDispatchWorker: Skip, check again
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/license/client.go (1)
145-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat empty 2xx dispatch responses as errors, not no-ops.
doJSONOptional()currently collapses204 No Contentand a 200 response with an empty body into the samefalse, nil, which letsPullGitHubDispatch()treat malformed API responses as an empty queue. Preserve the 204/no-content distinction and fail when a 2xx dispatch response is missingid.🔧 Suggested fix
func (c *CloudClient) doJSONOptional(ctx context.Context, method, path string, reqBody, respBody any) (bool, error) { @@ if resp.StatusCode == http.StatusNoContent { return false, nil } @@ - if respBody == nil || len(respData) == 0 { - return false, nil + if respBody == nil { + return true, nil + } + if len(respData) == 0 { + return true, nil } if err := json.Unmarshal(respData, respBody); err != nil { return false, fmt.Errorf("failed to unmarshal response: %w", err) } return true, nil } func (c *CloudClient) PullGitHubDispatch(ctx context.Context, req PullGitHubDispatchRequest) (*GitHubDispatchJob, error) { var resp GitHubDispatchJob ok, err := c.doJSONOptional(ctx, http.MethodPost, "/api/v1/github/dispatch/pull", req, &resp) if err != nil { return nil, err } - if !ok || resp.ID == "" { - return nil, nil + if !ok { + return nil, nil + } + if resp.ID == "" { + return nil, fmt.Errorf("github dispatch response missing id") } return &resp, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/license/client.go` around lines 145 - 259, doJSONOptional currently treats an empty 2xx response the same as 204 No Content and lets callers (like PullGitHubDispatch) silently treat malformed responses as no-ops; change doJSONOptional so only an explicit http.StatusNoContent (204) returns (false, nil), but any other 2xx with an empty body returns (false, error) (e.g. fmt.Errorf("empty response body for %s", path) or a CloudError) and still unmarshal non-empty bodies as before, and update PullGitHubDispatch to return an error when doJSONOptional succeeds but resp.ID == "" (rather than returning nil,nil) so missing ID is treated as a failed/malformed response.
🧹 Nitpick comments (1)
internal/persis/filegithubdispatch/store.go (1)
138-141: ⚡ Quick winConsider syncing the tracker directory after rename for full crash durability.
At Line 138, the file data is synced, but the directory entry update is not. A sudden crash can still lose
tracked.jsonmetadata even after successfulRename.💡 Suggested durability hardening
- if err := os.Rename(tmpName, filepath.Join(s.dir, trackerFile)); err != nil { + finalPath := filepath.Join(s.dir, trackerFile) + if err := os.Rename(tmpName, finalPath); err != nil { return fmt.Errorf("rename tracker temp file: %w", err) } + dir, err := os.Open(s.dir) + if err != nil { + return fmt.Errorf("open tracker dir for sync: %w", err) + } + defer dir.Close() + if err := dir.Sync(); err != nil { + return fmt.Errorf("sync tracker dir: %w", err) + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/filegithubdispatch/store.go` around lines 138 - 141, The rename succeeds but the containing directory is not fsynced, risking loss of tracked.json metadata on a crash; after the os.Rename(tmpName, filepath.Join(s.dir, trackerFile)) call in the function that uses s.dir, open the directory (os.Open(s.dir)), call Sync() on that *os.File, handle and return any error (e.g., wrap with "sync tracker dir after rename"), and Close the directory file; ensure this directory-sync step occurs only after the rename and before returning nil to provide full crash durability for trackerFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/license/client.go`:
- Around line 157-163: AcceptGitHubDispatch and FinishGitHubDispatch interpolate
jobID directly into the request path, allowing '/' or '?' in jobID to alter the
URL; call url.PathEscape(jobID) before concatenating into the path and use the
escaped value when invoking doJSONAllowNoContent (e.g., replace
"/api/v1/github/dispatch/"+jobID+"/accept" with
"/api/v1/github/dispatch/"+escapedJobID+"/accept" and similarly for the finish
path).
In `@internal/license/manager.go`:
- Around line 80-106: CloudMachineCredentials currently can return persisted
credentials from m.state even when the Manager is a zero-value or running in a
non-heartbeat/cloud mode; to fix this, early-return nil when m is nil or m.state
is nil and also validate the manager's active source/mode before exposing creds:
add a guard at the top of CloudMachineCredentials that checks m != nil, m.state
!= nil, and that the manager's source/mode (e.g. a method or field such as
m.Source(), m.Mode(), or m.activeSource) indicates the heartbeat/cloud
credential source is active — if not, return nil, nil; keep the existing
Claims() and ActivationData() checks (claims.ID, activation.ServerID,
activation.HeartbeatSecret) afterward.
In `@internal/service/scheduler/enqueue_webhook.go`:
- Around line 36-42: The idempotency check currently treats any non-nil error
from dagRunStore.FindAttempt as "not found"; change it to explicitly handle the
not-found case by using errors.Is(err, exec.ErrDAGRunIDNotFound) and only
proceed to create a new attempt when that specific error occurs, while returning
other errors immediately; keep the existing behavior of logging and returning
nil when FindAttempt returns nil (attempt exists). Locate the call to
dagRunStore.FindAttempt(ctx, dagRun) and replace the broad err==nil/else flow
with explicit branching: if err==nil -> logger.Info(...) and return nil; else if
errors.Is(err, exec.ErrDAGRunIDNotFound) -> continue to create attempt; else ->
return the error. Ensure you import/use errors and reference
exec.ErrDAGRunIDNotFound exactly as in scheduler.go.
In `@internal/service/scheduler/github_dispatch_test.go`:
- Around line 120-121: The assertions in github_dispatch_test.go check unquoted
substrings against status.Params; update the expected fragments to include
quotes so they match the runtime-serialized params (e.g., change checks against
GITHUB_EVENT_NAME=pull_request and GITHUB_PR_NUMBER=42 to match
GITHUB_EVENT_NAME="pull_request" and GITHUB_PR_NUMBER="42"). Locate the
assertions that reference status.Params (the two assert.Contains calls) and
replace the expected substrings with the quoted forms to ensure the test
compares against the actual serialized param values.
---
Outside diff comments:
In `@internal/license/client.go`:
- Around line 145-259: doJSONOptional currently treats an empty 2xx response the
same as 204 No Content and lets callers (like PullGitHubDispatch) silently treat
malformed responses as no-ops; change doJSONOptional so only an explicit
http.StatusNoContent (204) returns (false, nil), but any other 2xx with an empty
body returns (false, error) (e.g. fmt.Errorf("empty response body for %s", path)
or a CloudError) and still unmarshal non-empty bodies as before, and update
PullGitHubDispatch to return an error when doJSONOptional succeeds but resp.ID
== "" (rather than returning nil,nil) so missing ID is treated as a
failed/malformed response.
---
Nitpick comments:
In `@internal/persis/filegithubdispatch/store.go`:
- Around line 138-141: The rename succeeds but the containing directory is not
fsynced, risking loss of tracked.json metadata on a crash; after the
os.Rename(tmpName, filepath.Join(s.dir, trackerFile)) call in the function that
uses s.dir, open the directory (os.Open(s.dir)), call Sync() on that *os.File,
handle and return any error (e.g., wrap with "sync tracker dir after rename"),
and Close the directory file; ensure this directory-sync step occurs only after
the rename and before returning nil to provide full crash durability for
trackerFile.
🪄 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: 213e443d-1d6d-4150-86fc-a0d678966a82
📒 Files selected for processing (21)
internal/agent/session.gointernal/agent/session_test.gointernal/cmd/context.gointernal/core/webhook_params.gointernal/core/webhook_params_test.gointernal/license/activation.gointernal/license/client.gointernal/license/client_test.gointernal/license/manager.gointernal/license/manager_test.gointernal/llm/providers/openaicodex/codex.gointernal/llm/providers/openaicodex/codex_test.gointernal/persis/filegithubdispatch/store.gointernal/persis/filegithubdispatch/store_test.gointernal/service/frontend/api/v1/webhooks.gointernal/service/scheduler/enqueue_webhook.gointernal/service/scheduler/github_dispatch.gointernal/service/scheduler/github_dispatch_test.gointernal/service/scheduler/scheduler.goui/src/features/agent/hooks/__tests__/useAgentChat.test.tsxui/src/features/agent/hooks/useAgentChat.ts
Collaborator
Author
|
Follow-up commit |
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.
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests