[hivecommons-hive]

Followers polling with since_seq = next_seq lost commands' terminal status: an ended line shares its started line's seq, and Read filtered lines by since_seq before folding, so the ended line was dropped and the record stayed unfinished forever.

Fix: Read now folds all lines first, then selects records with seq > since_seq. next_seq stops just before the first unfinished record in the page (clamped to since_seq), so it is re-delivered until terminal (idempotent by seq). Docs updated; tests cover the follow case and a mixed ended/unfinished/ended page.

Closes #17398

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign danathar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

[hivecommons-hive]

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

hive/awaiting-ci: CI pending — sweep will merge when green.

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

Review findings from ci-maintainer (folded here because this head already carries its hive review):

Style/maintainability review (head 082fe34, read src/crux/cmdlog/log.go, src/crux/api/cmdlog.go, and the NextSeq consumer src/crux/cli/cmdlog.go).

1. New NextSeq contract breaks the one caller that follows it: cmdlog list --all can duplicate records or loop forever (high, in-scope)

src/crux/cmdlog/log.go:167-172 now sets page.NextSeq = max(rec.Seq-1, sinceSeq) at the first unfinished record while leaving page.Commands and page.More untouched. src/crux/cli/cmdlog.go:59-69 follows with for { ...; if !*all || !page.More { break }; sinceSeq = page.NextSeq } and appends every page to records.

• If the unfinished record is at index i > 0 in a page with More == true, the next page starts at rec.Seq-1 and re-delivers records i..limit — duplicates in records with nothing deduplicating them.
• If the unfinished record is the first record of a page with More == true, NextSeq collapses to sinceSeq (the max clamp — which is actually a no-op, since rec.Seq > sinceSeq is already guaranteed by the filter at log.go:155) and the loop calls RunCmdlog with the same sinceSeq indefinitely.

Before this PR NextSeq always advanced, so the loop was total. A started line with no ended line is permanent after a runner crash (log.go:120-121), so any run with an abandoned command followed by more than --limit (default 100, cmdlog/cmdlog.go:46) later commands hangs crux cmdlog list --all. The PR body says "More keeps its meaning", but api/cmdlog.go:88-92 still documents More as "a following page exists now" while the following page now overlaps the current one — the two fields' documented semantics no longer compose.

Requested change: make a page never contain both delivered and to-be-redelivered records, so NextSeq is still the exclusive high-water mark of what was returned:

• in Read, when the first unfinished record is at index i > 0, truncate page.Commands = all[:i], set More = true, NextSeq = all[i-1].Seq;
• when i == 0, return the page as-is with NextSeq = sinceSeq and More = false (a further poll with the same since_seq can add nothing until the record ends), and drop the dead max(..., sinceSeq) clamp;
• guard the --all loop in cli/cmdlog.go:68 with if page.NextSeq <= sinceSeq { break } so it can never spin, and add a CLI test for --all over a run with a stuck record and > limit later commands.

TestReadNextSeqStopsBeforeFirstUnfinished (cmdlog_test.go:184-203) asserts len(page.Commands) == 3 && NextSeq == 1, i.e. it pins the overlapping-page behaviour; adjust it with the fix.

Review findings from reviewer-2 (folded here because this head already carries its hive review):

Read src/crux/cmdlog/log.go at 082fe34 and its caller src/crux/runs/service.go:278. The fix looks correct to me.

Why the old code lost the ended line: Read filtered ln.Seq <= sinceSeq before folding (old log.go:135), so for a follower polling with since_seq = <that command's seq> the ended line — which shares its started line's seq — was discarded and the record could never leave unfinished. Moving the filter to after the fold (log.go:155) means an ended line always meets its started record first; records at or below since_seq are then dropped as a unit. The NextSeq clamp at log.go:167-172 keeps the first unfinished record in range for the next poll, so re-delivery is idempotent by seq.

Both new tests fail against the previous head: TestReadFollowerSeesEndedLineOfUnfinishedCommand asserts NextSeq == 0 where the old code returned 1, and TestReadNextSeqStopsBeforeFirstUnfinished asserts 1 where the old code returned 3.

One design note, not a defect: because NextSeq now stops at the first unfinished record while More is still computed from the full tail (log.go:162-164), a follower whose page contains a long-running unfinished command at position k re-receives the same page and cannot see records beyond the page limit until that command ends. For sequential runs the unfinished record is the last one and this never bites; it only matters if a run records concurrent commands (the open map in Log allows more than one). Worth a sentence in the CommandPage doc if that is an accepted tradeoff.

Review findings from reviewer (folded here because this head already carries its hive review):

Read src/crux/cmdlog/log.go at 082fe34 and its callers (runs/service.go:278,628,677, agentstep/agentstep.go:295-342, cli/cmdlog.go:58-68). The core fix is correct: folding every line first and filtering by rec.Seq <= sinceSeq at log.go:155 is exactly what lets a follower who already saw the started line pick up its ended line, and both new tests fail against the old filter-before-fold order. Looks correct to me.

One verified edge worth a decision before merge, not a blocker:

crux cmdlog list --all can now spin forever if an unfinished record sits mid-page with more: true — low severity, needs a never-ended command followed by >limit later records in the same log.

• log.go:162-171: when the page overflows limit, More=true, and the first unfinished record is at index 0, NextSeq collapses to sinceSeq.
• cli/cmdlog.go:64-68: the --all loop breaks only on !page.More, then sets sinceSeq = page.NextSeq — so with more:true, next_seq==since_seq it re-requests the identical page indefinitely and appends duplicates to records.

In practice this is hard to reach: commands are sequential per run, Restart (runs/restart.go:46) creates a new run rather than appending, and startup reconciliation leaves the orphan as the last record, so More is false there. The only path I found to a mid-log orphan is an Ended append failure, which both callers swallow (agentstep.go:342, service.go:628 use _ =). Still, the server now emits a more:true / next_seq == since_seq pair that the protocol previously never produced, and the one in-tree follower trusts it. A one-line guard in the CLI loop (if page.NextSeq <= sinceSeq { break }) or a doc sentence on design/crux-work-services.md:300-301 ("pass next_seq as since_seq while more is true" — now true only until next_seq stops advancing) would close it.

Docs: the Go doc comments are updated; src/docs/design/crux-work-services.md:300-301 still describes the old "advance while more" contract without the re-delivery caveat. Info-level.

No security surface touched; scope matches #17398 exactly; changelog fragment present.

Evidence bundle `#17399082fe34` sha256:81903c18d4dac80101084faba650bcd2ce453a5cec530c81bd22107ef5c3dd82 — download from the dashboard or `hivectl review evidence #17399`.

[hivecommons-hive]

Intent alignment (head 082fe34): the diff does what #17398 decided — Read folds all lines then filters by seq > since_seq, and NextSeq is clamped to just before the first unfinished record (src/crux/cmdlog/log.go:152-172). The two new tests cover the issue's repro and a mixed page. Scope is tight. One in-scope consequence of the new NextSeq semantics is unhandled in the shipped follower:

crux cmdlog list --all can now loop forever or emit duplicate records (src/crux/cli/cmdlog.go:58-69). The loop is for { page := RunCmdlog(since); records = append(records, page.Commands...); if !page.More { break }; since = page.NextSeq }. With the clamp, a page with More == true whose first record is unfinished yields NextSeq == sinceSeq (log.go:169), so the identical request repeats indefinitely; an unfinished record mid-page yields NextSeq = seq-1, so the next page re-delivers it at index 0 — again spinning if that page is full, or duplicating the tail of the previous page in records otherwise. Permanently-unfinished records followed by ≥ page-size later records are reachable: Open (log.go:42-54) rebuilds next but not open, so a restarted runner (the case Open's comment anticipates) leaves its in-flight command's started line without an ended line forever while continuing to append. Before this PR NextSeq always advanced, so --all always terminated.

Requested fix (keep the Read change as-is, per the issue's decision): in remoteCmdlogList, stop when the page made no progress (page.NextSeq <= sinceSeq) and merge page.Commands into records keyed by Seq (later copy wins) so re-delivered records don't duplicate; add a CLI test with an unfinished record at the head of a full page.

Out of scope (pre-existing): src/docs/crux-integration-guide.md:227 shows "next_seq":2 for a page whose only record is seq 1; Read returns the last record's seq (1), not seq+1.

— hive: agent=adjudicator-3 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Review follow-ups (below review.backlog.min_severity: P2)

Non-blocking findings below the backlog filing line are listed here instead of as issues, one comment per PR, updated in place. Tick them off as they are fixed; set review.backlog.min_severity: P3 to file every finding.

• P3 low intent-alignment — Integration guide example shows next_seq one past the last record (src/docs/crux-integration-guide.md:227): The example response lists a single record with seq 1 and "next_seq":2, but Read sets NextSeq to the last record's seq (src/crux/cmdlog/log.go:166), i.e. 1. Pre-existing doc inaccuracy, not introduce…

[hivecommons-hive]

hive/ci-triage: build-contributor (linux/arm64, true, true, 75) on 082fe34 failed in src/Dockerfile.contributor step 15/37 — curl to https://go.dev/dl/go1.27.3.linux-arm64.tar.gz returned 404 (run 42631163201). That artifact resolves 200 now, so this was a transient release-CDN propagation failure, not this diff (which only touches src/crux/cmdlog/*, src/crux/api/cmdlog.go and the changelog fragment). Infrastructure flake — deferring; no rerun requested by the scanner lane.

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[github-actions]

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback