docs: update wait race docs for auto cursor threading (#2323) [doc-updater] - #2348
Conversation
The send→wait race section in agent-wait-patterns.md and the orchestrator-cli.md table still described manual --since anchoring as the recommended zero-drop approach. With #2323, cursor threading is automatic when EGG_AGENT_ROLE is set, so the manual pattern is now an edge-case fallback for shell scripts using wait --json directly. Authored-by: egg
There was a problem hiding this comment.
Review summary
Documentation-only PR. The technical claims line up with the implementation introduced in 6e0ddf4 / PR #2326:
_wait_cursor_pathinsandbox/egg_lib/orch_cli.py:1279returnsNonewhenEGG_AGENT_ROLEis unset, falling back to legacy from-tip behavior — matches the "automatic whenEGG_AGENT_ROLEis set" qualifier inorchestrator-cli.md.- Forward reference "see 'Auto cursor threading' below" lands on the existing §"Auto cursor threading (issue #2323)" added by the same upstream commit.
- Issue #2323 referenced correctly throughout.
No blocking issues. Two non-blocking observations:
1. Intro paragraph and example contradict each other (agent-wait-patterns.md:355-365)
The new intro says:
For shell scripts that use
wait --jsondirectly and need explicit cursor control, the manual anchor pattern remains available:
But the example below it uses egg-orch message wait-loop, not wait --json:
# manual zero-drop pattern — for shell scripts using wait --json directly
anchor=$(egg-orch message poll --limit 1 --json | jq -r '.messages[0].id // empty')
egg-orch consensus confirmed
egg-orch message wait-loop \
--for CONSENSUS_CONFIRMED --for CONSENSUS_RE_REVIEW --for OVERSEER_ALERT \
${anchor:+--since "$anchor"}The wait --json cursor-threading flow is already documented separately in §"Cursor threading across waits (issue #1995)" further down, so this example reads as the "manual --since with wait-loop" fallback. Suggest dropping the wait --json qualifier from both the intro paragraph and the inline comment, e.g. "For callers that need explicit cursor control" — the example then matches the framing.
2. Conditional vs. unconditional phrasing of the race-closure claim (agent-wait-patterns.md:348)
The bolded claim reads:
This race is closed automatically by the auto cursor threading introduced in #2323
This is true only when EGG_AGENT_ROLE is set. The orchestrator-cli.md table entries correctly carry the qualifier ("automatic when EGG_AGENT_ROLE is set"); §"Auto cursor threading" later in the file also spells it out. But a reader scanning the §"Race: send → wait" subsection in isolation gets an unconditional impression. In practice this is fine (production agents always have EGG_AGENT_ROLE), but it's an asymmetry between the two docs the PR is updating. Consider matching the qualifier from orchestrator-cli.md, or reinforcing the "standard wait-loop callers" caveat in the same sentence.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add EGG_AGENT_ROLE qualifier to the bolded race-closure claim so the §Race: send → wait subsection matches the qualifier already carried in orchestrator-cli.md and §Auto cursor threading. - Reframe the manual-anchor fallback intro and example comment from "shell scripts using wait --json directly" to "callers that need explicit cursor control" — the example uses wait-loop, not wait --json, so the new framing matches the code shown. Authored-by: egg
|
Thanks for the review. Both non-blocking observations addressed in deb73d20. Per-item disposition:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Actions": 1} |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
This is a docs-only update (with a minor wget→curl swap in lint.yml) that simplifies the documented wait pattern by noting that cursor threading is now automatic — which actually reduces prompt-level rigidity for agents. No pre-fetching, no structured-output-for-humans, no rigid procedures, no API-call concerns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary
Both prior non-blocking observations are properly addressed in deb73d20, and the additional fc72165 is a sensible CI fix.
Prior feedback verification
- #1 Intro paragraph vs. example mismatch (
agent-wait-patterns.md:356-361): Fixed. The intro now reads "For callers that need explicit cursor control (e.g. shell scripts running outside an agent role, or composing custom flows)…" and the inline comment matches: "manual zero-drop pattern — for callers that need explicit cursor control". Framing now matches thewait-loop-based example. - #2 Conditional phrasing of the race-closure claim (
agent-wait-patterns.md:348-350): Fixed. The bolded claim now reads "For standardwait-loopcallers withEGG_AGENT_ROLEset, this race is closed automatically by the auto cursor threading introduced in #2323" — qualifier carried inline, consistent withorchestrator-cli.md:45-46and §"Auto cursor threading" at line 437.
New change: wget→curl --retry for actionlint install (.github/workflows/lint.yml:106-107)
curl -fsSL --retry 3 --retry-delay 5 -o actionlint.tar.gz is correct:
-fexits non-zero on HTTP ≥400 (ensures retry kicks in for retriable HTTP errors).--retry 3 --retry-delay 5retries on the standard transient classes (timeouts, 408, 429, 5xx).-Lis included — important because GitHub releases 302-redirect to the S3 bucket, and curl does not follow redirects by default (whereas the priorwgetdid).-sSkeeps the silent-with-error-on-failure behaviour the priorwget -qhad.
No blocking issues.
Non-blocking observation
hadolint install in the same workflow still uses bare wget (lint.yml:84): The actionlint install was changed because of a transient HTTP error, but the hadolint install one job above downloads from the same GitHub releases CDN and is equally susceptible. Worth a follow-up to keep the two jobs symmetric. Not flagging as blocking — the PR's modification is scoped to the actions job, not the docker job, and hadolint hasn't observed the failure in the wild yet.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mirrors the actionlint install change from fc72165 — both binaries download from the same GitHub releases CDN and are equally susceptible to the transient HTTP errors that prompted the actionlint switch. Keeps the two install steps symmetric.
|
Thanks for the re-review. The non-blocking observation is addressed in 6fc0b46. Per-item disposition:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary
No agent-mode design concerns.
Delta since my last review (fc72165) is one commit (6fc0b46) addressing the non-blocking hadolint observation: wget → curl -fsSL --retry 3 --retry-delay 5 for symmetry with the actionlint install one job above. Same retry semantics, same redirect-following (-L), same silent-with-error-on-failure behaviour. CI-only change with no agent-mode design surface.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary
Delta since my last review (fc72165) is a single commit (6fc0b46) that addresses the non-blocking hadolint observation from that review. Change is one line: .github/workflows/lint.yml:84 swaps wget -qO for curl -fsSL --retry 3 --retry-delay 5 -o.
Prior feedback verification
hadolintinstall still uses barewget(lint.yml:84): Fixed. The hadolint install now mirrors the actionlint install atlint.yml:106exactly:- Both use
curl -fsSL --retry 3 --retry-delay 5 -o <dest> <url>. - Same flag rationale carries over:
-fexits non-zero on HTTP ≥400 (so--retrykicks in on retriable HTTP error classes);-Lfollows the GitHub releases → S3 302 redirect that curl wouldn't otherwise follow (whereas the priorwgetdid);-sSkeeps the silent-with-error-on-failure behaviourwget -qhad. - The structural difference between the two installs (hadolint downloads a single binary directly to
/usr/local/bin/hadolint; actionlint downloads a tarball and extracts) is correctly preserved. curlis preinstalled onubuntu-latestrunners, so no new install dependency.
- Both use
No blocking issues. No new observations.
— Authored by egg
|
egg review completed. View run logs 10 previous review(s) hidden. |
Update documentation to reflect changes from 6e0ddf4:
docs/reference/agent-wait-patterns.md: The "Race: send → wait" subsectiondescribed manual
--sinceanchoring as the way to get zero-drop semantics.With BRC reviewer wait-loop misses proposals between calls — review of multi-producer phase stalls 30+ min #2323, cursor threading across
wait-loopre-entries is automatic whenEGG_AGENT_ROLEis set. Updated the section to note this, and repositionedthe manual
--sincepattern as an edge-case fallback for shell scripts usingwait --jsondirectly.docs/reference/orchestrator-cli.md: Updated themessage waitandmessage wait-looptable entries to replace "pass--since <id>forzero-drop send→wait semantics" with a note that cursor threading is automatic
(issue BRC reviewer wait-loop misses proposals between calls — review of multi-producer phase stalls 30+ min #2323).
Triggered by: 6e0ddf4 (Fix #2323: --cursor-file on wait/wait-loop closes wait→process→wait race)
Authored-by: egg