Fleet: retry-safe task requests and truthful status - #288
Conversation
Co-Authored-By: Codex <noreply@openai.com>
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 4m 17s —— View job Review — PR #288: Fleet retry-safe task requests and truthful statusReviewed head: SummaryThe implementation is correct and well-bounded. The design is honest about what it does and doesn't do (no delivery, acceptance, lease transfer, or hook changes). Tests are thorough — the cross-process subprocess test in particular ( FindingsP2 — Dead code: second
|
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. |
|
Reviewer: claude |
There was a problem hiding this comment.
🟡 Changes recommended
fleet dispatch is now blocked by any unrelated malformed dispatch row due to global strict validation in replaceableDispatch, which is an avoidable operational regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements the first increment of “natural coordination” for Fleet by adding a retry-safe, repo-scoped request identity (immutable replay) and a truthful, read-only status view that separates queued work from merely observed tool activity.
Changes:
- Add
fleet request/fleet_requestto record immutable request-bound assignments and refuse conflicting replays. - Add
fleet status/fleet_statusto provide a non-migrating, read-only status view with explicit evidence gaps and activity vs acceptance separation. - Protect request-bound dispatch rows from legacy mutation paths and record hook-owned post-tool write activity into the existing session record.
File summaries
| File | Description |
|---|---|
| WORK.md | Work plan/intent and stop conditions for this increment. |
| friction-log.md | Records the operational lesson and the implemented boundary (“assignment ≠ acceptance”). |
| cmd/fleet/testdata/test.sh | Updates MCP tool-count expectations for the new tools. |
| cmd/fleet/README.md | Documents the new request/status interface, limitations, and verification steps. |
| cmd/fleet/internal/verbs/work.go | Adds dispatch-store serialization and guards preventing legacy mutation of request-bound assignments. |
| cmd/fleet/internal/verbs/verbs.go | Wires new CLI entrypoints for request and status. |
| cmd/fleet/internal/verbs/status.go | Implements read-only request-bound status rendering and JSON output. |
| cmd/fleet/internal/verbs/request.go | Implements request ID validation, immutable replay, conflict refusal, and strict dispatch-row reading. |
| cmd/fleet/internal/verbs/request_test.go | Adds concurrency, replay/conflict, mutation protection, and read-only/status gap tests. |
| cmd/fleet/internal/mcp/task_test.go | Verifies MCP status does not initialize/migrate state and preserves parseable JSON-RPC error packets. |
| cmd/fleet/internal/mcp/mcp.go | Adds fleet_request/fleet_status MCP tools and routes status via JSON-only output capture. |
| cmd/fleet/internal/fleet/task_activity_test.go | Tests post-tool write evidence attribution/provenance rules. |
| cmd/fleet/internal/fleet/hook.go | Records last_write evidence on PostToolUse via hook-owned session record updates. |
Review details
Suppressed comments (1)
cmd/fleet/internal/verbs/work.go:254
- The request-bound guard is duplicated: the pre-scan loop already refuses if any request-bound assignment exists for this branch, so the second
if request_id != ""inside the unlink loop is unreachable.
Removing the dead check reduces complexity and ensures there’s only one refusal path to keep consistent.
if fleet.S(r, "request_id") != "" {
return refuse("fleet undispatch: request-bound assignment is retained for replay safety; inspect `fleet status`")
}
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func replaceableDispatch(rid, branch, rel string) (fleet.Rec, error) { | ||
| if _, err := strictDispatchRows(); err != nil { | ||
| return nil, err | ||
| } | ||
| existing := fleet.ReadJSON(dispatchFile(rid, branch, rel)) | ||
| if fleet.S(existing, "request_id") != "" { | ||
| return nil, refuse("fleet dispatch: this assignment is request-bound; inspect `fleet status` rather than replacing it") | ||
| } | ||
| return existing, nil | ||
| } |
| if fleet.S(r, "request_id") != "" { | ||
| return refuse("fleet reassign: request-bound assignment cannot be changed by an unbound reassign; inspect `fleet status`") | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eeb1498e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| func postWriteEvidence(ev Event, sid string) Rec { | ||
| tool := S(ev, "tool_name") | ||
| cmd := S(M(ev, "tool_input"), "command") | ||
| if !IsWrite(tool, cmd) { |
There was a problem hiding this comment.
For Codex installations generated by fleet role, cmd/fleet/internal/verbs/role.go:140 registers PostToolUse only for ^Bash$. Consequently this new evidence path is never invoked for ordinary Edit/Write/apply_patch activity, so a Codex worker can modify the assigned branch while fleet status remains Queued. Broaden the installed post-tool matcher to include the write tools handled here.
Useful? React with 👍 / 👎.
| if !requestID.MatchString(id) || worker == "" || lead == "" || strings.TrimSpace(brief) == "" { | ||
| return refuse("fleet request: requires --id (1-96 letters/digits/._-), --worker, --for and --brief") | ||
| } | ||
| rid, branch, head, err := resolveDispatchTarget("request", change) |
There was a problem hiding this comment.
When an identical request is retried after its branch has been deleted or its local/remote ref is otherwise unavailable, resolveDispatchTarget refuses before the existing request_id is inspected. Since request rows are intentionally retained and the README promises same-repo/same-ID replay, a caller recovering from a lost response can no longer retrieve the recorded assignment once repository state changes; look up the stored ID first and apply branch/head preconditions only to new requests.
Useful? React with 👍 / 👎.
| if fleet.S(r, "repo") == rid && fleet.S(r, "change") == branch && fleet.S(r, "request_id") != "" { | ||
| return refuse("fleet: request-bound assignments require a correlated lifecycle action; inspect `fleet status`") |
There was a problem hiding this comment.
If a request-bound implementation row exists and a legacy verify row is subsequently added for the same branch, fleet undispatch <branch> --as verify is refused because this preliminary scan rejects any request row on the branch without considering rel. This leaves the unrelated legacy row impossible to retire through its targeted command; limit this guard to request rows selected by the requested relationship.
Useful? React with 👍 / 👎.
|
Fix round 1 is pushed at 363fd07 (initial review head: 6eeb149). Consolidated the completed Codex, Claude and Copilot findings before editing:
Validation: Fleet race tests, focused lint/vet, both harness regression suites, work-contract validation and diff checks pass. Initial-head full-module CI passed; this head awaits CI and panel re-review. MCP still runs verbs serially; scoped ReadOnly is documented, not advertised as concurrency-safe. Scope remains the first assignment/observation increment: no delivery, semantic acceptance, actual stop/replacement or live installation. The current request interface is implementation-only; caller-selected relationships remain follow-on work before a general-purpose adapter. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 5m 53s —— View job Re-review — PR #288: Fleet retry-safe task requests and truthful statusReviewed fix round 1 at Tasks:
Verification of prior findingsAll findings from the initial review and the Codex/Copilot round are addressed. Claude P2 (dead code — duplicate request_id guard): Confirmed resolved. The second mutation loop in both Codex P1 (PostToolUse only subscribed to Codex P2 (replay fails after branch deleted): Confirmed resolved. Codex P2 (relationship-selective undispatch): Confirmed resolved. The pre-scan in Copilot P1 (global dispatch DoS from unrelated malformed row): Confirmed resolved. New findingsP2 —
|
There was a problem hiding this comment.
🟡 Changes recommended
Hook subscription and legacy mutation paths have correctness/usability issues (missed write tools and overly strict dispatch-store scanning) that can lead to incomplete activity observation or blocked maintenance actions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cmd/fleet/internal/verbs/work.go:239
undispatchalso usesstrictDispatchRows(), so any unrelated corrupted dispatch row prevents retiring an otherwise valid legacy row. Since the protection needed here is specifically “don’t delete a request-bound implementation assignment”, this can be narrowed to validate that one row while keeping legacy cleanup usable.
rows, err := strictDispatchRows()
if err != nil {
return err
}
for _, r := range rows {
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Lite
| rows, err := strictDispatchRows() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, r := range rows { | ||
| if fleet.S(r, "repo") == rid && fleet.S(r, "change") == branch && fleet.S(r, "request_id") != "" { | ||
| return refuse("fleet: request-bound assignments require a correlated lifecycle action; inspect `fleet status`") | ||
| } | ||
| } | ||
| n := 0 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 363fd07a4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| data["hooks"] = hooks | ||
| } | ||
| specs := [][2]string{{"SessionStart", ""}, {"UserPromptSubmit", ""}, {"PreToolUse", "^(Bash|Edit|Write)$"}, {"PostToolUse", "^Bash$"}, {"Stop", ""}, {"SessionEnd", ""}} | ||
| specs := [][2]string{{"SessionStart", ""}, {"UserPromptSubmit", ""}, {"PreToolUse", "^(Bash|Edit|Write|NotebookEdit|apply_patch)$"}, {"PostToolUse", "^(Bash|Edit|Write|NotebookEdit|apply_patch)$"}, {"Stop", ""}, {"SessionEnd", ""}} |
There was a problem hiding this comment.
When Codex uses apply_patch, this matcher now invokes both hooks, but IsWrite still recognizes only Edit, Write, MultiEdit, and NotebookEdit as file tools. Consequently PreToolUse skips the lease check and PostToolUse records no activity, allowing a patch on a branch held by another session while the assignment remains Queued. Fresh evidence after the earlier subscription finding is that the subscription was added without updating the classifier; align FileTools with this matcher.
Useful? React with 👍 / 👎.
| case fleet.S(fleet.M(rec, "last_write"), "key") == key && fleet.F(fleet.M(rec, "last_write"), "at") >= fleet.F(d, "at"): | ||
| row["activity_at"] = fleet.M(rec, "last_write")["at"] | ||
| taskState(row, "Activity observed", "Acceptance and completion remain unconfirmed", "Read the worker's result and current checks") |
There was a problem hiding this comment.
When one live worker has requests on branches A and B, a qualifying write on A initially makes A Activity observed, but the next write on B replaces the session's single last_write; this predicate then reports A as Queued and suggests delivering its brief again even though A's post-dispatch activity was already observed. Retain the latest write per branch/request rather than consulting only the worker's globally latest write.
Useful? React with 👍 / 👎.
|
Reviewer: claude |
|
Final allowed fix round: Consolidated all three completed reviews of 363fd07:
Fleet race tests, focused lint/vet, both full harness regression suites, diff checks and work-contract validation pass. CI on this head is pending. This is the final requested panel cycle under the repository's initial-plus-two-fix-round cap; any residual findings will be reported explicitly rather than starting another loop. No merge or installation performed. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 4m 19s —— View job Re-review — PR #288: Fleet retry-safe task requests and truthful statusReviewed final fix round at Tasks:
Verification of prior findingsAll round-2 findings are addressed. Claude P2 (apply_patch classifier bypass — evidence never recorded): Resolved by proof, not by adding Copilot P1 / Codex round-2 (MultiEdit missing from write subscriptions): Confirmed resolved. Codex P2 / Copilot round-2 (global strict dispatch validation blocked unrelated maintenance): Confirmed resolved. Codex P2 round-2 (writes on another branch erased prior observation): Confirmed resolved. New findingsObservation — Redundant identity conditions in
|
|
Reviewer: claude |
There was a problem hiding this comment.
🟡 Changes recommended
New dispatch-store locking paths can surface fleet.ErrKeyBusy as a generic error instead of a user-actionable refusal, which is a user-visible behavior regression that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
cmd/fleet/internal/verbs/request.go:72
- If the dispatch-store lock times out,
fleet.KeyLock("dispatch", ...)returnsfleet.ErrKeyBusy. Right now that bubbles up as a generic error (exit code 4) even thoughErrKeyBusyis meant to be a refusal with next-action guidance. Handlefleet.ErrKeyBusyexplicitly so a busy store reads as an expected concurrency refusal, not an internal error.
cmd/fleet/internal/verbs/work.go:183
fleet.KeyLock("dispatch", ...)can returnfleet.ErrKeyBusy, but this wrapper currently bubbles it up as a generic error (exit code 4) instead of a refusal. Since this is a user-visible concurrency condition, map it to arefuse(...)message (similar to other verbs likefleet take).
func CmdReassign(change, forRole string) error {
return fleet.KeyLock("dispatch", func() error { return cmdReassign(change, forRole) })
}
cmd/fleet/internal/verbs/work.go:225
fleet.KeyLock("dispatch", ...)can returnfleet.ErrKeyBusy, butcmdUndispatchcurrently propagates it as a generic error (exit code 4) instead of a refusal. TranslateErrKeyBusyinto arefuse(...)so callers get a consistent user-actionable refusal.
func cmdUndispatch(change, rel string) error {
return fleet.KeyLock("dispatch", func() error { return undispatch(change, rel) })
}
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
| func CmdDispatch(change, rel, forRole, due, slot, brief, by, replyTo string, take bool) error { | ||
| return fleet.KeyLock("dispatch", func() error { | ||
| return cmdDispatch(change, rel, forRole, due, slot, brief, by, replyTo, take) | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a243af21b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| wanted := fleet.Rec{"request_id": id, "repo": rid, "change": branch, | ||
| "worker": worker, "for": lead, "brief": strings.TrimSpace(brief), "relationship": "implementation"} |
There was a problem hiding this comment.
On Windows, Git can resolve a differently-cased spelling such as Task to the existing branch task (the existing fleet.BranchSpelling helper handles this case), but this record preserves the caller's spelling. The request then checks leases and status under repo:<id>:Task, while hooks derive repo:<id>:task from HEAD, so it can miss an existing holder and never match the worker's activity. Store the branch's canonical ref spelling before constructing the request record and key.
Useful? React with 👍 / 👎.
| row := fleet.ReadJSON(filepath.Join(dispatchDir(), entry.Name())) | ||
| if row == nil || fleet.S(row, "repo") == "" || fleet.S(row, "change") == "" || fleet.S(row, "relationship") == "" { | ||
| return nil, fmt.Errorf("assignment evidence unreadable: %s", entry.Name()) |
There was a problem hiding this comment.
A valid JSON row carrying request_id passes this check even when request-specific evidence such as worker or numeric at is missing or malformed. In particular, requestStatus reads a missing/invalid at as zero, so any old matching last_write from a live worker is reported as Activity observed and the packet remains complete: true despite the damaged assignment evidence. Validate the required request fields and types before returning the row.
Useful? React with 👍 / 👎.
|
Final panel consolidated on code head Claude: no blocking defects, one cosmetic redundancy. Copilot: busy dispatch lock surfaces generic error instead of refusal. Codex: Windows branch spelling needs canonicalization; request-specific fields/types need validation to prevent damaged timestamp evidence from admitting old activity. These findings are recorded with impact and proposed deferral rationale, not dismissed or accepted by me. Code-head CI (full check, fuzz, hygiene) and both local harness suites/race/lint checks passed. No merge, gate judgment, installation or live worker control performed. This first increment is built and reviewed but retains the recorded P2s for judgment. Broader natural delivery/acceptance/questions/stop/replacement work remains in progress. |
Summary
A supervisor retrying a queued Fleet dispatch can overwrite its timestamp or brief, while branch activity can be mistaken for accepted work. Add a repo-scoped request identity that retains the original assignment on replay, refuses conflicting payloads, and exposes a non-migrating status view that explicitly separates queued work from observed tool activity.
This is the first implementation increment of the natural-coordination design, not the completed agent-team interface. It does not launch or message workers, infer semantic acceptance, stop processes, transfer leases, or install hooks.
What this adds
fleet requestand MCPfleet_requestrecord immutable request ID/worker fields in the existing dispatch store. Cross-process locking serializes replay and protects request rows against legacy dispatch/reassign/undispatch mutation.fleet statusand MCPfleet_statusgive read-only local request observations. Queued, activity observed and evidence needing inspection remain distinct; no false Started/Stopped/Done claims.Request records remain retained for replay safety until correlated lifecycle operations are implemented. Request is effectful and can perform existing lazy migration; status does not. Assignment remains accountability, not an execution reservation or a replacement for the branch guard. Source, current limitations and follow-on adapter proof are documented in cmd/fleet/README.md and WORK.md. Broader Dossier task remains open.
Validation
go test -race ./cmd/fleet/...go vet ./...andgolangci-lint run ./...(final touched-package lint repeated after changes)cmd/fleet/testdata/run-suite.sh: all scenarios passedgit diff --check9a243af21bd7f72f94e5c5af0e7bad0e4d60b1fd; local full-suite request was cost-guarded, so targeted checks were usedReview focus: immutable request replay across processes, all legacy writers of dispatch rows, read-only CLI/MCP behavior, and the boundary between activity and acceptance. No merge requested by this implementation.
Final review disposition
All three reviewers completed on code head
9a243af21bd7f72f94e5c5af0e7bad0e4d60b1fd.The later documentation-only commit records residuals in FOLLOWUPS.md: Windows
branch spelling, request-metadata validation, busy-lock diagnostics and a cosmetic
redundancy. These remain for judgment after the two-fix-round cap; they are not
dismissed or accepted. No fourth panel request, merge or installation.