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. |
…ination # Conflicts: # friction-log.md
|
Landing refresh at Fresh local Fleet race tests pass. Full remote CI is running. Final independent code-head review was |
…ination # Conflicts: # friction-log.md
…ination # Conflicts: # FOLLOWUPS.md
|
Current integration head No fourth panel round. Final independent reviews cover9a243af, not this current head; exact-head freshness and the recorded Windows branch-case/status timestamp/lock-diagnostic residuals require an explicit governed judgment. This is evidence/disposition bookkeeping, not acceptance or merge authority. Final CI is running; no live installation or session/custody changes. |
…ination # Conflicts: # cmd/fleet/internal/verbs/verbs.go
|
Final integration head Documented validation now passes on the combined tree: Fleet race suite, vet, zero lint issues, Claude reference suite (364 scenarios), Codex reference suite (367 scenarios), and full CI plus native Mac/Windows seats. The reference suite initially failed only because it required the old "already running" watcher diagnostic. Commit977cf74 strengthens that assertion to rc1 plus "watcher lock unavailable" and absence of the old claim; no runtime policy changed. Three original review cycles remain capped. Existing FOLLOWUPS.md still records the Windows branch-spelling, invalid request-field timestamp and busy-store diagnostic residuals for an explicit judgment. No independent final-head panel completion is claimed, and fixture tests do not prove live harness wiring. No live hook or ownership changes. |
|
Governed landing decision: BLOCKED, Gate run
The expanded write subscriptions are verified addressed. Busy-store exit classification and redundant conditions are nonblocking. Old-head reviews are not represented as current-head completion. Full CI, native seats checks, Fleet race/vet/lint, and 364 Claude / 367 Codex fixtures passed, but those do not remove the two correctness defects. No merge, override, grant mint or automatic review-loop extension. The two original repair rounds are exhausted under AGENTS.md; landing pauses for an explicitly bounded repair decision. The existing grant tier/cycle budget is not the issue. |
…ws never read complete Operator-authorized repair of the two blockers from the same-run judgment at 977cf74. (1) CmdRequest keyed ownership and activity by the caller's spelling; on a case-insensitive filesystem two spellings keyed two assignments for one branch. canonicalBranch resolves the spelling from git's own ref list (exact, else unique case-insensitive match). (2) strictDispatchRows accepted a request-bound row with no worker, lead or timestamp, so damaged evidence could still report complete:true; such a row is now an error for status and refuses new requests. One test each. Suite 364/364, race tests, lint 0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Claude finished @itsHabib's task in 5m 21s —— View job Re-review — PR #288: Fleet retry-safe task requests and truthful statusReviewed operator-authorized repair at Tasks:
Scope of this commitOne commit since the judgment head: Verification of blocker 1 — canonical branch spelling
branch = canonicalBranch(cwd(), branch)
wanted := fleet.Rec{"request_id": id, "repo": rid, "change": branch, ...}The implementation runs
This is the correct precedence. The call happens before the
Creates a repo with branch
This covers the primary concern: hooks and request records now key on the same spelling while the branch exists. Design note — cross-case replay after branch deletion (not a new defect)
The fix does not cover cross-case replay after deletion (e.g., stored as Verification of blocker 2 — strict validation of request-bound rows
Added to if fleet.S(row, "request_id") != "" && (fleet.S(row, "worker") == "" || fleet.S(row, "for") == "" || fleet.F(row, "at") <= 0) {
return nil, fmt.Errorf("assignment evidence damaged: %s", entry.Name())
}
When
Creates a valid request, then for each of
Each iteration overwrites the previous broken file with a freshly derived broken record from the original, so iterations are independent. All three damage cases are proved. ✓ No new blocking defectsThe two fixes are focused and minimal. Both address exactly the stated blockers, introduce no new logic paths beyond the repair, and are covered by targeted tests. The redundant identity conditions in the |
Kept verbatim under its own heading so #288 resolves the same-file add by taking this union. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 376f3b9a4d
ℹ️ 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".
| // Ownership and activity are keyed by the branch as git spells it, never by the | ||
| // caller's spelling: on a case-insensitive filesystem two spellings would | ||
| // otherwise key two assignments for one branch. | ||
| branch = canonicalBranch(cwd(), branch) |
There was a problem hiding this comment.
When the initial request uses a case variant (for example nav-fix for local branch Nav-Fix), this stores Nav-Fix; after that branch is deleted, the identical retry canonicalizes back to the caller's nav-fix, so validateReplay rejects it as different work. This breaks the documented full-ID retry behavior after branch deletion. Fresh evidence beyond the earlier replay finding is that the new canonicalization still derives replay identity from mutable refs before consulting the retained row; replay comparison should preserve or use the stored branch spelling.
Useful? React with 👍 / 👎.
| } | ||
| // A request-bound row carries who it went to and when; without either it is | ||
| // damaged evidence, and damaged evidence must not read as a complete status. | ||
| if fleet.S(row, "request_id") != "" && (fleet.S(row, "worker") == "" || fleet.S(row, "for") == "" || fleet.F(row, "at") <= 0) { |
There was a problem hiding this comment.
When valid JSON contains a non-string request_id (or an invalid string outside the accepted ID grammar), this guard does not validate it: fleet.S returns empty for a non-string value, bypassing the request-specific checks, and CmdStatus subsequently omits the row while returning complete: true. Fresh evidence beyond the prior damaged-row finding is that the new guard uses the successfully decoded string itself to decide whether the row is request-bound; inspect field presence/type and validate the stored ID before that decision.
Useful? React with 👍 / 👎.
…ng; non-string request_id is damaged evidence Codex at 376f3b9. sameRequest compares the change case-insensitively, since a retry after the branch is gone can only offer the caller's spelling of a branch the store keyed by git's. strictDispatchRows treats a present request_id that is not a string in the ID grammar as damaged rather than silently non-request. One test each. Its P1 (Codex post-tool matcher only ^Bash$) is stale at this head: role.go already registers the write tools. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Claude finished @itsHabib's task in 3m 27s —— View job Re-review — PR #288: Fleet retry-safe task requests and truthful statusReviewed operator-authorized follow-up at Tasks:
Scope of this commitOne commit since the judgment head: Verification of Codex P2 #1 — replay survives branch deletion after canonicalization
func sameRequest(a, b fleet.Rec) bool {
for _, key := range []string{"request_id", "repo", "worker", "for", "brief", "relationship"} {
if fleet.S(a, key) != fleet.S(b, key) {
return false
}
}
// The stored change is git's spelling; a retry after the branch is gone can
// only offer the caller's, so the same branch spelled differently by case is
// the same work, as it would be on the filesystem that keyed it.
return strings.EqualFold(fleet.S(a, "change"), fleet.S(b, "change"))
}The fix is correct. The scenario is:
The fix doesn't weaken conflict detection: a retry with a genuinely different branch (different case-insensitive name) returns
Creates branch Design note — cross-case replay correctness on case-sensitive filesystems. The fix is narrow and correct: it allows case-insensitive match on a field that git itself is case-insensitive about (on any filesystem where the branch could exist). On a case-sensitive Linux filesystem with two distinct branches Verification of Codex P2 #2 — non-string request_id is damaged evidence
if id, present := row["request_id"]; present {
ids, ok := id.(string)
if !ok || !requestID.MatchString(ids) || fleet.S(row, "worker") == "" || fleet.S(row, "for") == "" || fleet.F(row, "at") <= 0 {
return nil, fmt.Errorf("assignment evidence damaged: %s", entry.Name())
}
}The previous guard used The
Creates a valid request (for branch Verification of Codex P1 (post-tool matcher) — already resolved
specs := [][2]string{..., {"PreToolUse", "^(Bash|Edit|Write|MultiEdit|NotebookEdit|apply_patch)$"}, {"PostToolUse", "^(Bash|Edit|Write|MultiEdit|NotebookEdit|apply_patch)$"}, ...}Codex PostToolUse matcher covers No new blocking defectsThe two commits since the judgment head ( |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
Review P1: it documented request/status verbs that do not exist on main, plus two transient lines. The section returns with #288's own README. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…solves, the caller's when it does not Judge counterexample at bd4ea8d: sameRequest folded case, so an ID recorded for Nav-Fix replayed against a distinct nav-fix on a case-sensitive filesystem. The row now also stores the caller's spelling (requested). canonicalBranch reports whether the name resolved to a ref; a replay compares change exactly when it did, and the caller's original spelling exactly when it did not (the deleted-branch retry). Regression covers the counterexample and both retry cases. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Claude finished @itsHabib's task in 2m 57s —— View job Re-review — PR #288: Fleet retry-safe task requests and truthful statusReviewed latest commit at HEAD Tasks:
Scope of this commitOne commit since the prior reviewed head: Verification of the judge's blocker — exact replay identity
The fix introduces a
This is correct and matches what the judge required. The old
The Verification of TestReplayNeverMatchesADistinctRefByCase
The test exercises Three cases:
One observation (not a blocker)Legacy rows without Rows created by commits before VerdictNo blocking defects. The fix closes the judge's counterexample exactly: replay identity is strict equality when the ref resolves (preventing cross-case reuse of an ID between distinct branches) and strict caller-spelling equality when it does not (preserving the deleted-branch retry guarantee). The test exercises all three discriminating cases on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e0a07aa60
ℹ️ 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".
| if name == cand { | ||
| return cand, true | ||
| } | ||
| if name != "" && strings.EqualFold(name, cand) { |
There was a problem hiding this comment.
On a case-sensitive filesystem, if only Nav-Fix exists and the caller requests nav-fix, Git does not resolve that spelling, but this unconditional EqualFold match rewrites it to Nav-Fix; resolveDispatchTarget then succeeds and records an assignment for the wrong branch instead of rejecting the unresolved caller input. Only apply case-insensitive canonicalization where Git/filesystem semantics actually make the spellings equivalent (the existing fleet.BranchSpelling helper already makes that platform distinction).
Useful? React with 👍 / 👎.
| if id, present := row["request_id"]; present { | ||
| ids, ok := id.(string) | ||
| if !ok || !requestID.MatchString(ids) || fleet.S(row, "worker") == "" || fleet.S(row, "for") == "" || fleet.F(row, "at") <= 0 { | ||
| return nil, fmt.Errorf("assignment evidence damaged: %s", entry.Name()) |
There was a problem hiding this comment.
Fresh evidence after the earlier damaged-row finding is that this new guard still accepts request rows whose required brief or requested field is missing or non-string. Such a row makes fleet status report complete: true with an empty assignment brief, and deleting requested also makes the documented identical replay fail after branch deletion because sameRequest compares the missing value against the caller spelling; validate these immutable request fields before admitting the row.
Useful? React with 👍 / 👎.
…ask-coordination increment #288's section kept under its own heading now that request/status are on main; its two transient lines (a personal path 'during this build', 'This PR') dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* docs(fleet): README for cmd/fleet The one cmd/ directory without one. What the binary is, the four faces, the store, install/shadow/switch, ownership rows, the verb groups, testing and what is deliberately absent. Facts checked against main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(fleet): fold #288's task-coordination section into the README Kept verbatim under its own heading so #288 resolves the same-file add by taking this union. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(fleet): say what it is for — leads, seats, location as identity Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(fleet): drop the task-coordination section until #288 is on main Review P1: it documented request/status verbs that do not exist on main, plus two transient lines. The section returns with #288's own README. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(fleet): handoff is a control verb; the watcher writes its own board files Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(fleet): repair the sentence the transient-line drop cut Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(fleet): last pull-request phrasing out of the README Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.