feat: add task attempt lifecycle protocol#41
Merged
glittercowboy merged 1 commit intomainfrom Apr 30, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe protocol is extended to support attempt-scoped task execution and structured lifecycle diagnostics. New message types, deadline configuration, and attempt metadata are introduced across task-related messages and a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages_test.go`:
- Around line 1838-1926: Add tests to messages_test.go that exercise
ParseEnvelope/Envelope.DecodePayload for negative and compatibility cases: (1)
add a ParseEnvelope test for MsgTypeTaskLifecycle that ensures decoding succeeds
for the new wire shape (use TaskLifecycle and
Envelope.DecodePayload/ParseEnvelope), (2) add invalid-payload tests that feed
JSON where fields have wrong types (e.g., numeric PID as string or ObservedAt as
number) and assert DecodePayload/ParseEnvelope returns an error, (3) add a test
that includes unknown extra fields in the TaskLifecycle/Task JSON and asserts
decoding tolerates them, and (4) add a compatibility test that decodes a legacy
Task JSON without AttemptID/DeadlineProfile and verifies Task unmarshals with
zero/nil AttemptID and a nil/zero DeadlineProfile (use Task and TaskDeadlines
validation), ensuring all new tests reference the existing symbols
TaskLifecycle, Task, Envelope.DecodePayload, ParseEnvelope, and TaskDeadlines so
they fail/validate the protocol change.
In `@messages.go`:
- Around line 253-257: Field DeadlineProfile in messages.go is currently a
value-typed TaskDeadlines which cannot distinguish "omitted" vs "present but
empty" when marshaling JSON (it will serialize as {}), so change the type to a
pointer (*TaskDeadlines) on the struct that contains DeadlineProfile (or
implement custom JSON marshal/unmarshal for that field) so omitempty will
actually omit absent values; update any construction sites that populate
DeadlineProfile to use &TaskDeadlines or nil accordingly and adjust consumers to
handle a nil DeadlineProfile.
In `@PROTOCOL.md`:
- Around line 247-250: The protocol docs currently state that attempt metadata
is echoed beyond taskLifecycle but the message tables for stream, taskStarted,
taskComplete, taskError, taskCancelled, permissionRequest, and question still
show the old shapes; update PROTOCOL.md to include the new attempt fields
(attemptId, attemptNumber, leaseExpiresAt, deadlineProfile, turnKind) on each of
those task-adjacent frame descriptions and message tables (also apply the same
fix at the other occurrence around lines 297-300), and ensure the prose
clarifies that these fields are present on every echoed frame so it matches the
Go types and tests before bumping consumers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eb9c4c1-616c-4111-9733-c92e25a56b54
📒 Files selected for processing (4)
PROTOCOL.mdenvelope.gomessages.gomessages_test.go
Comment on lines
+1838
to
+1926
| func TestTaskLifecycleRoundTrip(t *testing.T) { | ||
| observedAt := time.Date(2026, 4, 30, 12, 30, 55, 0, time.UTC) | ||
| deadlineAt := observedAt.Add(90 * time.Second) | ||
| msg := &TaskLifecycle{ | ||
| Type: MsgTypeTaskLifecycle, | ||
| TaskID: "task-1", | ||
| AttemptID: "attempt-1", | ||
| AttemptNumber: 1, | ||
| SessionID: "session-1", | ||
| ChannelID: "channel-1", | ||
| Phase: TaskLifecyclePhasePromptWritten, | ||
| Status: TaskAttemptStatusPromptWritten, | ||
| ObservedAt: observedAt, | ||
| DeadlineAt: &deadlineAt, | ||
| PID: 12345, | ||
| Provider: "claude-cli", | ||
| Model: "claude-sonnet-4-6", | ||
| RequestID: "request-1", | ||
| Traceparent: "00-b1a91ab1f89d5141c7280de8bd272d73-1111111111111111-01", | ||
| } | ||
|
|
||
| raw, err := json.Marshal(msg) | ||
| if err != nil { | ||
| t.Fatalf("marshal: %v", err) | ||
| } | ||
|
|
||
| env := Envelope{Type: MsgTypeTaskLifecycle, Payload: raw} | ||
| decoded, err := env.DecodePayload() | ||
| if err != nil { | ||
| t.Fatalf("decode: %v", err) | ||
| } | ||
| got, ok := decoded.(*TaskLifecycle) | ||
| if !ok { | ||
| t.Fatalf("decoded type = %T", decoded) | ||
| } | ||
| if got.AttemptID != "attempt-1" || got.Phase != TaskLifecyclePhasePromptWritten { | ||
| t.Fatalf("lifecycle fields missing: %#v", got) | ||
| } | ||
| } | ||
|
|
||
| func TestTaskAttemptFieldsRoundTrip(t *testing.T) { | ||
| task := Task{ | ||
| Type: MsgTypeTask, | ||
| TaskID: "task-1", | ||
| SessionID: "session-1", | ||
| ChannelID: "channel-1", | ||
| Prompt: "hello", | ||
| Model: "claude-sonnet-4-6", | ||
| Effort: "max", | ||
| PermissionMode: "acceptEdits", | ||
| CWD: "/tmp/project", | ||
| AttemptID: "attempt-1", | ||
| AttemptNumber: 2, | ||
| LeaseExpiresAt: "2026-04-30T12:32:55Z", | ||
| TurnKind: TurnKindUser, | ||
| DeadlineProfile: TaskDeadlines{ | ||
| ProcessStartMs: 15000, | ||
| PromptWriteMs: 5000, | ||
| FirstEventMs: 90000, | ||
| FirstVisibleEventMs: 120000, | ||
| StreamIdleMs: 120000, | ||
| ToolIdleMs: 300000, | ||
| UserInputMs: 600000, | ||
| CleanupTermMs: 2000, | ||
| }, | ||
| PlanCapability: &PlanCapability{ | ||
| ID: "cap-1", | ||
| AttemptID: "attempt-1", | ||
| Token: "opaque", | ||
| APIBaseURL: "https://app.gsd.build", | ||
| ExpiresAt: "2026-04-30T12:45:55Z", | ||
| }, | ||
| } | ||
|
|
||
| raw, err := json.Marshal(task) | ||
| if err != nil { | ||
| t.Fatalf("marshal: %v", err) | ||
| } | ||
| var got Task | ||
| if err := json.Unmarshal(raw, &got); err != nil { | ||
| t.Fatalf("unmarshal: %v", err) | ||
| } | ||
| if got.AttemptID != "attempt-1" || got.DeadlineProfile.FirstEventMs != 90000 { | ||
| t.Fatalf("attempt/deadline fields missing: %#v", got) | ||
| } | ||
| if got.PlanCapability == nil || got.PlanCapability.AttemptID != "attempt-1" { | ||
| t.Fatalf("plan capability attempt missing: %#v", got.PlanCapability) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add compatibility and invalid-payload coverage for the new wire shapes.
These tests only cover the happy path. For this protocol change, we still need ParseEnvelope coverage for taskLifecycle, invalid field-type cases, unknown-field tolerance, and a compatibility case where legacy task payloads omit the new attempt/deadline fields.
As per coding guidelines: "Tests must verify parsing, serialization, unknown/invalid payload handling, and protocol compatibility behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@messages_test.go` around lines 1838 - 1926, Add tests to messages_test.go
that exercise ParseEnvelope/Envelope.DecodePayload for negative and
compatibility cases: (1) add a ParseEnvelope test for MsgTypeTaskLifecycle that
ensures decoding succeeds for the new wire shape (use TaskLifecycle and
Envelope.DecodePayload/ParseEnvelope), (2) add invalid-payload tests that feed
JSON where fields have wrong types (e.g., numeric PID as string or ObservedAt as
number) and assert DecodePayload/ParseEnvelope returns an error, (3) add a test
that includes unknown extra fields in the TaskLifecycle/Task JSON and asserts
decoding tolerates them, and (4) add a compatibility test that decodes a legacy
Task JSON without AttemptID/DeadlineProfile and verifies Task unmarshals with
zero/nil AttemptID and a nil/zero DeadlineProfile (use Task and TaskDeadlines
validation), ensuring all new tests reference the existing symbols
TaskLifecycle, Task, Envelope.DecodePayload, ParseEnvelope, and TaskDeadlines so
they fail/validate the protocol change.
Comment on lines
+253
to
+257
| AttemptID string `json:"attemptId,omitempty"` | ||
| AttemptNumber int `json:"attemptNumber,omitempty"` | ||
| LeaseExpiresAt string `json:"leaseExpiresAt,omitempty"` | ||
| DeadlineProfile TaskDeadlines `json:"deadlineProfile,omitempty"` | ||
| TurnKind TurnKind `json:"turnKind,omitempty"` |
There was a problem hiding this comment.
Preserve omission for deadlineProfile.
deadlineProfile is documented as optional, but this value-typed field can't preserve “omitted” versus “present but empty” across JSON round-trips. That makes legacy task payloads easy to re-emit in a different shape. Prefer a pointer here (or equivalent custom marshal logic).
Suggested change
- DeadlineProfile TaskDeadlines `json:"deadlineProfile,omitempty"`
+ DeadlineProfile *TaskDeadlines `json:"deadlineProfile,omitempty"`In Go's `encoding/json`, does `omitempty` omit a zero-value non-pointer struct field, or will it still serialize as `{}`?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@messages.go` around lines 253 - 257, Field DeadlineProfile in messages.go is
currently a value-typed TaskDeadlines which cannot distinguish "omitted" vs
"present but empty" when marshaling JSON (it will serialize as {}), so change
the type to a pointer (*TaskDeadlines) on the struct that contains
DeadlineProfile (or implement custom JSON marshal/unmarshal for that field) so
omitempty will actually omit absent values; update any construction sites that
populate DeadlineProfile to use &TaskDeadlines or nil accordingly and adjust
consumers to handle a nil DeadlineProfile.
Comment on lines
+247
to
+250
| The relay owns task attempts. A dispatched `task` includes the active | ||
| `attemptId`, `attemptNumber`, `leaseExpiresAt`, `deadlineProfile`, and | ||
| `turnKind`. Daemons echo attempt metadata on task-adjacent frames so the relay | ||
| can associate runtime events with the active attempt. |
There was a problem hiding this comment.
Document the new attempt fields on the existing task-adjacent frames too.
This section says attempt metadata is echoed beyond taskLifecycle, but the message tables for stream, taskStarted, taskComplete, taskError, taskCancelled, permissionRequest, and question still describe the pre-attempt shapes. That leaves the authoritative spec behind the Go types.
Based on learnings: "Treat message shape changes as cross-repo work: update PROTOCOL.md, Go types, and tests here first before bumping consumers".
Also applies to: 297-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PROTOCOL.md` around lines 247 - 250, The protocol docs currently state that
attempt metadata is echoed beyond taskLifecycle but the message tables for
stream, taskStarted, taskComplete, taskError, taskCancelled, permissionRequest,
and question still show the old shapes; update PROTOCOL.md to include the new
attempt fields (attemptId, attemptNumber, leaseExpiresAt, deadlineProfile,
turnKind) on each of those task-adjacent frame descriptions and message tables
(also apply the same fix at the other occurrence around lines 297-300), and
ensure the prose clarifies that these fields are present on every echoed frame
so it matches the Go types and tests before bumping consumers.
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 by CodeRabbit