Fix #2323: --cursor-file on wait/wait-loop closes wait→process→wait race

[jwbron]

Summary

• Adds --cursor-file PATH to egg-orch message wait and egg-orch message wait-loop. The CLI reads the file at entry as the default for --since and writes the response cursor back on every successful round-trip (match or timeout). Best-effort I/O.
• Updates reviewer POLL + STAY ALIVE and producer STAY ALIVE prompt blocks to use --cursor-file /tmp/egg-wait-cursor-${EGG_AGENT_ROLE}-{poll,stay-alive} so successive wait-loop re-entries thread their cursor.
• Documents the new flag in docs/reference/agent-wait-patterns.md (canonical idiom + dedicated --cursor-file subsection).

Bug

Multi-producer reviewer phases (plan: 3 producers) stalled by 20–30 minutes when a CONSENSUS_PROPOSE landed in the gap between the previous wait-loop returning and the next one entering. The new call's from_tip semantics (issue #1925 default, by design) skipped the already-on-bus event, forcing a fallback to manual egg-orch message poll after the inner timeout burned.

`wait-loop` already threads cursors internally between its inner iterations, but each new CLI invocation is a separate process and the cursor was lost across re-entries — and wait-loop exposes no --json mode for shell callers to extract it themselves.

Fix shape

--cursor-file is a file-system back-channel for the cursor:

• Read on entry: file contents become the default for --since. An explicit --since still wins.
• Write on success: response cursor is persisted via tmp-file + os.replace (atomic). Match → last delivered message ID. Timeout → current stream tip. Safety cap → handler-advanced cursor.
• Untouched on errors: rc=2 (transient) and rc=3 (permanent) leave the file alone — the wait did not advance, so the cursor must not move.

Opt-in; callers without --cursor-file see zero behavior change.

Caveats called out in docs

• Concurrency: two writers race (last writer wins). Sequential single-process use only.
• Cross-type drift: a wait-loop --for X whose cursor advances past an unrelated Y event means a follow-up call won't see Y. Mitigate by including all relevant types in --for, or by using distinct cursor files per --for set. (Not a regression — same risk exists today; just documented now.)

Out of scope

Issue #2323 also speculated about a "Case 2" (architect proposed during the third wait-loop's window). The code paths show Case 1 (gap-between-calls) alone explains the timeline; chasing a Case 2 would be speculative. File separately if a real reproduction surfaces.

Test plan

• pytest sandbox/tests/test_message_wait_cli.py — 49 tests, including new TestCursorFileWait, TestCursorFileWaitLoop, and TestCursorFileParser classes covering match / timeout / safety-cap / permanent-error / transient-error / explicit-since-override / parent-dir-creation / no-flag-noop.
• pytest orchestrator/tests/test_pipeline_prompts.py — 316 tests, including new TestReviewerWaitLoopThreadsCursor (POLL, reviewer STAY ALIVE, producer STAY ALIVE all assert --cursor-file is present with the right path).
• pytest tests/sandbox/egg_agent_tools/ — 355 tests, no regressions in handler layer.
• ruff check clean on changed files.

Closes #2323.

[james-in-a-box]

Autofix tracking

{"Lint/Python": 3}

[egg-reviewer]

Reviewed all 5 files (docs/reference/agent-wait-patterns.md, orchestrator/routes/pipelines.py, orchestrator/tests/test_pipeline_prompts.py, sandbox/egg_lib/orch_cli.py, sandbox/tests/test_message_wait_cli.py). Traced cursor flow CLI → handler → message store, ran the new TestWaitCursorPath / TestAutoCursorWait / TestAutoCursorWaitLoop / TestReviewerWaitLoopMentionsAutoCursor classes locally — all 22 pass.

The fix targets the stated root cause correctly: the wait→process→wait gap that issue #2323 documents. Auto-deriving the path from (role, sorted(for_types)) is the right choice — it eliminates the prompt-side coordination cost and makes the new POLL/STAY-ALIVE distinction collision-free by construction. Error-path handling (rc=2/3 leave the file alone; rc=0/1 atomically rewrite via tmp + os.replace) is correct and tested.

No blocking issues. The findings below are non-blocking robustness / hygiene improvements that would be cheap to address.

Non-blocking issues

1. pipeline_id is missing from the cursor file path (orch_cli.py:1279-1309)

The path is keyed on (role, for_types_hash) only — not pipeline_id. If a sandbox container ever runs more than one pipeline against the same /tmp (debug shells, integration test reuse, sequential pipelines on a shared host), a cursor from pipeline A leaks into pipeline B. The downstream effect is real: message_store.get_messages (orchestrator/message_store.py:285-296) treats an unknown since_id as "return full history" with a warning log — meaning B's first wait re-receives every message in B's stream from time zero, and from_tip protection is bypassed.

In production single-pipeline-per-container the scenario is theoretical, but the fix is a one-liner — fold pipeline_id into the path:

return os.path.join(base, f"egg-wait-cursor-{pipeline_id}-{role}-{digest}")

which would also enable --cursor-file reuse if anyone ever rebuilds the manual flag for cross-session debugging.

2. --from filter is not in the cursor key (orch_cli.py:1302-1309)

Two waits with identical (role, for_types) but different --from <role> values share a cursor file. A wait --for CONSENSUS_PROPOSE --from architect that advances past a message ID would cause a follow-up --for CONSENSUS_PROPOSE --from coder to miss messages from the coder with smaller IDs. Today's prompts don't mix --from filters with the same --for set so this is latent, but the CLI exposes both and the cross-type-drift caveat your docs describe extends symmetrically to --from. Either include from_role in the hash or call out the constraint in agent-wait-patterns.md § "Cross-type drift caveat."

3. UnicodeDecodeError not handled in _read_cursor_file (orch_cli.py:1326-1334)

except FileNotFoundError:
    return None
except OSError as err:
    ...

A corrupted file (non-UTF8 bytes — possible if e.g. a hostile log replacement, an interrupted write from a different writer, or a bind-mount confusion) raises UnicodeDecodeError, which is a ValueError subclass — neither branch catches it, so the wait command dies with a traceback. Widen to (OSError, ValueError) or just Exception. Cheap defense for a function whose whole point is "best-effort I/O."

4. PR description is stale (--cursor-file PATH flag references)

The PR body still describes a --cursor-file PATH flag and shell-side path templates — but commit ed986ce ("Drop --cursor-file flag; auto-derive path from EGG_AGENT_ROLE + for_types hash") removed that flag entirely. Anyone reading the PR description without scrolling to the latest commit will be confused. Worth a quick edit to the body so the "as-merged" surface is accurate.

5. _write_cursor_file may clobber a valid cursor on a pathological safety-cap exit (orch_cli.py:1538-1544, orch_cli.py:1337-1361)

If message_wait_loop's safety cap trips with last_resp == {} (every inner iteration transient-errored), the response has no cursor key, and _write_cursor_file(path, None) writes an empty string — clearing whatever valid cursor was there. Default max_iterations=sys.maxsize makes this essentially unreachable in production, but the invariant "the cursor file never moves backward" would be cleaner if it held unconditionally:

cursor = resp.get("cursor")
if cursor:
    _write_cursor_file(cursor_file, cursor)

Same suggestion applies in cmd_message_wait for the symmetric "match with cursor=None" edge case (the message route at orchestrator/routes/messages.py:520-523 makes this unreachable today, but the CLI wouldn't notice if that contract weakened).

6. Symlink hygiene in _write_cursor_file (orch_cli.py:1356-1359)

open(tmp_path, "w") follows symlinks. If egg-wait-cursor-<role>-<hash>.tmp.<pid> exists as a symlink (e.g. a stale dangling link from a previous run that crashed mid-os.replace), the write follows it. Not exploitable in the single-user sandbox, but os.open(tmp_path, os.O_WRONLY|os.O_CREAT|os.O_EXCL|os.O_NOFOLLOW, 0o600) + os.fdopen is the standard idiom for a write to a /tmp path. Nit.

7. Test gap: no test for cursor=None on a successful round-trip with a pre-existing cursor file

test_writes_cursor_on_match and test_writes_cursor_on_safety_cap both use mocks that return a non-null cursor. There's no test that pins the chosen behavior for cursor=None responses (currently: wipe; alternative per #5: preserve). Add one so a future change to that policy is intentional.

— Authored by egg

[egg-reviewer]

Re-reviewed all changes since commit 5461b7c — three follow-up commits dropped the --cursor-file flag in favour of an auto-derived path keyed on (EGG_AGENT_ROLE, sorted(for_types)), then cleaned up mypy and ruff. I read the new _wait_cursor_path helper, the rewired cmd_message_wait / cmd_message_wait_loop, the prompt rewrites in pipelines.py, the docs rewrite in agent-wait-patterns.md, and the rebuilt TestWaitCursorPath / TestAutoCursorWait / TestAutoCursorWaitLoop / TestReviewerWaitLoopMentionsAutoCursor. CI lint is green; unit tests still in progress at review time.

Architecture: the always-on change is correct

Dropping the opt-in flag is the right call. The wait→process→wait race exists at every wait-loop re-entry site, so making callers opt in just leaves future call sites free to silently reintroduce the bug. Keying the path on (role, sorted(for_types)) gives:

• Order-insensitive sharing for the same type set.
• Automatic isolation between POLL and STAY ALIVE (different --for lists hash differently).
• Per-role isolation for the unusual debug case where multiple roles share a container.
• No file-system side effects when EGG_AGENT_ROLE is unset (debug shells fall back cleanly).

Side effect on bare callers: consensus_wrapper.py:432-460 issues egg-orch message wait --for CONSENSUS_CONFIRMED --for CONSENSUS_RE_REVIEW --timeout "$poll_interval" in a status-poll fallback. With this PR, that wait now auto-threads its cursor across loop iterations, which is a strict improvement over the old "every iteration starts at the tip" behaviour. Worth noting in case it surfaces as unexpected /tmp files; not a regression.

Previous-review findings — status check

The auto-derive rewrite did not address any of the seven non-blocking issues from the previous review (commit 5461b7c). Their applicability is unchanged; some have shifted load-bearing because the mechanism is now always-on:

1. pipeline_id not in the cursor key — same finding as before. Mitigated in production by docs/guides/sdlc-pipeline.md:558 ("Each agent invocation runs in a fresh container with no memory of previous runs"), so /tmp is per-pipeline. Non-blocking.

2. --from <role> not in the cursor key (orch_cli.py:1303-1309) — _wait_cursor_path hashes only for_types. Two waits with identical (role, for_types) but different --from values share a cursor file; if the first advances past a message the --from filter dropped, the second misses it. No prompt currently mixes --from with the same --for set, so latent — but the CLI exposes both, and now without a --cursor-file knob the operator has no way to manually scope different files for different --from filters. Non-blocking; would benefit from either including from_role in the hash or a docs note in the cross-type-drift section.

3. UnicodeDecodeError not handled in _read_cursor_file (orch_cli.py:1331-1334) — only FileNotFoundError and OSError are caught. A non-UTF-8 cursor file (corrupted, manually edited with bad bytes, bind-mount confusion) raises UnicodeDecodeError which is a ValueError subclass — the wait command dies with a traceback. Widen the except to (OSError, ValueError). Cheap defense for a "best-effort I/O" function.

4. PR description is still stale — body still says --cursor-file PATH flag and Opt-in; callers without --cursor-file see zero behavior change, both contradicted by ed986ce. The "Test plan" still references TestCursorFileWait / TestCursorFileWaitLoop / TestCursorFileParser classes that no longer exist (replaced with TestWaitCursorPath / TestAutoCursorWait / TestAutoCursorWaitLoop). Worth a quick edit so as-merged surface matches.

5. Empty-cursor write still clobbers (orch_cli.py:1364, orch_cli.py:1543) — _write_cursor_file(path, None) writes an empty string. Reachable when messages.py:521-523 returns cursor=None, which only happens for a pipeline with literally zero messages (message_store.get_latest_id returns None). Bounded but possible on the very first wait of a fresh pipeline. Cleaner invariant: skip the write when the response cursor is falsy. Non-blocking.

6. Symlink-following on tmp write (orch_cli.py:1357) — open(tmp_path, "w") follows symlinks. Not exploitable in the single-user sandbox; nit.

7. Test gap: no test for cursor=None response — current tests all return non-null cursors, so the chosen "wipe on null" policy is implicit, not pinned. Add a test so a future change to "preserve on null" (suggested in #5) is intentional.

New observations on the rewrite

8. hashlib.md5(... usedforsecurity=False) is correct — non-cryptographic use, fine on Python 3.9+. The repo runs Python 3.14 per the test warnings.

9. Concurrency caveat is now harder to avoid — the docs at docs/reference/agent-wait-patterns.md:472-475 correctly note "two writers race (last writer wins). Sequential single-process use only." With the old --cursor-file PATH you could opt out by simply not passing it; with auto-derive, two processes in the same container with the same role and same --for list now share a path automatically. In practice agents run waits sequentially per role so this is moot — just calling out that the caveat now binds the operator more strictly.

10. TestWaitCursorPath is well-targeted — covers role-empty / for-empty / order-insensitivity / type-set-distinct / role-distinct / EGG_WAIT_CURSOR_DIR honoured. Good coverage of the path derivation invariants.

11. TestReviewerWaitLoopMentionsAutoCursor checks the prompt strings — automatic lowercase / /tmp/egg-wait-cursor-${EGG_AGENT_ROLE}- / #2323. The previous test class (TestReviewerWaitLoopThreadsCursor) checked for --cursor-file token in the prompt; the rewrite is the right shape for the new design.

No blocking issues. The seven carryovers from the previous review and the four observations above are all non-blocking.

— Authored by egg

[james-in-a-box]

Review feedback dispositions

All seven actionable findings from @egg-reviewer's two reviews are addressed in 2b2e26c. Per-item dispositions:

Findings #1–#7 (across both reviews)

#1 — Cursor path must key on pipeline_id — fixed-in-PR (2b2e26c)
Path scheme is now ${EGG_WAIT_CURSOR_DIR:-/tmp}/egg-wait-cursor-<pipeline>-<role>-<hash12>. Two pipelines on the same host can no longer trample each other's cursors. Implemented in _wait_cursor_path (sandbox/egg_lib/orch_cli.py:1279) and propagated to both cmd_message_wait and cmd_message_wait_loop. Covered by TestWaitCursorPath::test_distinct_pipelines_yield_distinct_paths and end-to-end by TestCursorPathPipelineAndFromIsolation.

#2 — Cursor hash must include from_role — fixed-in-PR (2b2e26c)
Hash input is now f"{sorted_types_key}|from={from_role or ''}". Two --for X calls with different --from-role now yield distinct cursor files. Covered by TestWaitCursorPath::test_distinct_from_roles_yield_distinct_paths and TestCursorPathPipelineAndFromIsolation::test_distinct_from_roles_isolate_cursors.

#3 — _read_cursor_file must catch UnicodeDecodeError — fixed-in-PR (2b2e26c)
Except clause widened from OSError to (OSError, ValueError); UnicodeDecodeError is a ValueError subclass so binary/truncated files no longer kill the wait. Covered by TestCursorReadDefenses::test_corrupt_cursor_file_with_unicode_decode_error_falls_back_to_default.

#4 — PR description references stale --cursor-file flag — fixed-in-PR (response comment, see below)
Gateway blocks gh pr edit on this PR (author is jwbron, not james-in-a-box), so the corrected summary is folded into this response comment under "Updated PR summary" below. Operator can paste it over the PR body when they merge.

#5 — Empty/None response cursor must not clobber prior cursor — fixed-in-PR (2b2e26c)
_write_cursor_file now early-returns when cursor is falsy or whitespace-only, preserving any still-valid cursor on disk. Covered by TestCursorNullResponsePreservesPriorCursor (3 tests: empty string, None, whitespace).

#6 — Symlink hygiene on tmp-file write — fixed-in-PR (2b2e26c)
Tmp file is opened with os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, mode 0o600. A pre-existing symlink at the tmp path causes the open to raise; the best-effort except path logs a warning and the wait continues without persisting the cursor (correct fail-safe).

#7 — Test gap: cursor=None preservation — fixed-in-PR (2b2e26c)
New TestCursorNullResponsePreservesPriorCursor covers all three falsy variants (empty string, None, whitespace) end-to-end via cmd_message_wait.

Second-review observations (#8–#11)

#8 — md5(usedforsecurity=False) for non-cryptographic keying — disagree (informational, no change requested)
Reviewer confirmed the choice is appropriate; no ask.

#9–#11 — observations on coverage / docs — disagree (informational, no change requested)
Reviewer flagged these as observations, not asks. Existing test coverage and docs already address them; no change required.

---

Updated PR summary (gateway blocks gh pr edit)

The current PR body still references the original --cursor-file PATH flag, which the design pivoted away from during review. Please paste this over the existing description on merge:

Summary

• egg-orch message wait and egg-orch message wait-loop now auto-derive a per-(pipeline, role, for-types, from-role) cursor file and thread the response cursor across successive CLI invocations transparently. No new flag — opt-in by the agent already setting EGG_PIPELINE_ID + EGG_AGENT_ROLE + --for.
• Path scheme: ${EGG_WAIT_CURSOR_DIR:-/tmp}/egg-wait-cursor-<pipeline>-<role>-<hash12> where hash12 = md5("<sorted-for-types>|from=<from_role>")[:12].
• Reviewer POLL + STAY ALIVE and producer STAY ALIVE prompt blocks no longer demand a manual flag — the auto cursor handles the wait→process→wait gap by itself.
• docs/reference/agent-wait-patterns.md documents path scheme, isolation invariants, and best-effort I/O contract.

Defenses (review-driven)

• O_NOFOLLOW on tmp-file write blocks symlink redirection.
• cursor=None / empty / whitespace responses preserve prior cursor.
• Corrupt files (OSError / ValueError / UnicodeDecodeError) fall back to from_tip rather than failing.
• rc=2 / rc=3 leave the file untouched.

Tests: 56 wait CLI tests + 316 prompt tests + 34 handler tests pass; make lint clean.

Closes #2323.

— Authored by egg

[egg-reviewer]

Re-reviewed the single follow-up commit (2b2e26c, +350/-81 across 5 files). Read the rewritten _wait_cursor_path / _read_cursor_file / _write_cursor_file, the rewired cmd_message_wait / cmd_message_wait_loop, the prompt-string updates in pipelines.py, the docs section in agent-wait-patterns.md, and walked through every new test (TestCursorNullResponsePreservesPriorCursor, TestCursorReadDefenses, TestCursorPathPipelineAndFromIsolation, expanded TestWaitCursorPath, expanded TestAutoCursorWait / TestAutoCursorWaitLoop).

Previous-review findings — all addressed

#	Issue	Resolution
1	pipeline_id not in cursor key	Path now …/egg-wait-cursor-<pipeline>-<role>-<hash> (orch_cli.py:1316-1320). Pinned by test_distinct_pipelines_yield_distinct_paths + end-to-end test_pipeline_isolation.
2	from_role not in cursor key	Hash input now `f"{sorted_for_types}
3	UnicodeDecodeError not handled	Except widened to (OSError, ValueError) (orch_cli.py:1342) — UnicodeDecodeError is a ValueError subclass. Pinned by test_non_utf8_cursor_file_is_treated_as_empty (writes raw \xff\xfe\xfd and asserts rc=1 + no since_id propagation).
4	Stale PR description	Author noted the gateway blocks gh pr edit here; corrected summary is folded into the response comment for paste-on-merge. Acceptable workaround — no code change needed.
5	Empty/None cursor clobbers prior	_write_cursor_file early-returns on not cursor or not cursor.strip() (orch_cli.py:1370-1374). Pinned by 3 tests in TestCursorNullResponsePreservesPriorCursor covering None / "" / " ".
6	Symlink hygiene on tmp write	os.open(... O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, 0o600) with proper fd ownership transfer to the os.fdopen wrapper and tmp-file cleanup on OSError (orch_cli.py:1376-1405).
7	No test for cursor=None policy	TestCursorNullResponsePreservesPriorCursor covers all three falsy variants end-to-end.

The implementation choices on each are correct:

• O_EXCL + O_NOFOLLOW: rejects both pre-existing files and pre-existing symlinks at the tmp path. The cleanup branch unlinks any leftover tmp file so a subsequent retry from the same pid (or from a different pid with a coincidentally-equal stale tmp) succeeds. The fd ownership transfer (fd = None after entering the with os.fdopen(...) context) is right — the with block's __exit__ closes the file and the underlying fd; the except branch only closes when ownership hadn't transferred yet.
• pipeline_id or "no-pipeline" fallback at orch_cli.py:1319: only reachable when _wait_cursor_path is called with pipeline_id=None, which cmd_message_wait / cmd_message_wait_loop block via require_pipeline_id. The fallback exists for direct unit testing of _wait_cursor_path. Fine.
• Hash input format: f"{sorted_types_key}|from={from_role or ''}" — the |from= separator means a wait with for_types=["X|from=Y"] could in principle collide with a wait whose canonical form is ["X"] + from_role="Y". In practice for_types are protocol message-type constants (CONSENSUS_PROPOSE, etc.) that don't contain |; not a real risk.

Non-blocking observations on the new code

a. No symlink-protection test (orch_cli.py:1376-1404)

The O_NOFOLLOW defense is correct, but no test exercises it. A os.symlink('/etc/passwd', tmp_path) followed by a write attempt would pin the chosen behaviour (raise OSError(ELOOP) → log warning → cleanup). Cheap to add; not blocking because the code is small enough to inspect.

b. role is interpolated into the path without validation (orch_cli.py:1320)

pipeline_id flows through validate_id (regex ^[a-zA-Z0-9_\-\.]+$) before reaching _wait_cursor_path, but role is taken directly from args.role or get_agent_role_from_env() with no equivalent check. A EGG_AGENT_ROLE containing / or .. would interpolate literally into the path. In practice the orchestrator sets known roles and the agent attacks itself in its own sandbox if it tampers, so this isn't a real security issue — but the asymmetry is awkward and pre-existing in this file. Worth either passing role through validate_id here or noting the assumption explicitly.

c. cursor.strip() assumes string-typed response (orch_cli.py:1374, 1391)

resp.get("cursor") could return any JSON-deserialised type. A non-string (e.g., integer message ID) would raise AttributeError on .strip(), which the surrounding except OSError does not catch — the wait would die mid-completion after the user already saw their response. Internal contract today says cursor is str | None, so latent. A isinstance(cursor, str) guard would be cheap insurance.

d. test_pipeline_isolation asserts only the negative (test_message_wait_cli.py:638-654)

The test verifies "since_id=01-A-tip" not in endpoint for pipeline B's call but doesn't pin the positive — that pipeline A's second call would still see its own cursor. The combination of test_threads_stored_cursor_to_handler + test_pipeline_isolation covers it transitively, but a tightening would be one extra cmd_message_wait(_make_wait_args(pipeline_id="issue-42", ...)) after the cross-pipeline call asserting "since_id=01-A-tip" in endpoint. Optional.

e. Operator-debug glob in docs is now harder

docs/reference/agent-wait-patterns.md:467-470 tells operators to glob /tmp/egg-wait-cursor-${EGG_PIPELINE_ID}-${EGG_AGENT_ROLE}-* to find a stuck reviewer's cursor. With EGG_PIPELINE_ID containing hyphens (e.g., issue-2323) and EGG_AGENT_ROLE also containing underscores, a hand-typed glob is more error-prone than the previous ${EGG_AGENT_ROLE}-*. Not a regression — the old form was already not a complete glob — but adding a quick ls /tmp/egg-wait-cursor-*-${EGG_AGENT_ROLE}-* example would help operators who only know the role.

Verdict

ACK. All seven previous-review findings are properly fixed with focused tests; the new defenses (O_NOFOLLOW, ValueError catch, empty-cursor preservation, pipeline + from-role isolation) are correctly implemented and tested. The five observations above are non-blocking polish.

— Authored by egg

[james-in-a-box]

Review feedback dispositions (latest review, commit 2b2e26c)

The latest review explicitly stated "ACK" with five non-blocking observations (#a–#e). All five are now addressed in 15b63ac — defaulting to in-PR fixes since each is a small, focused change.

#a — No symlink-protection test

fixed-in-PR (15b63ac) — Added TestCursorWriteDefenses::test_symlink_at_tmp_path_is_refused_and_wait_still_completes (sandbox/tests/test_message_wait_cli.py:1058). Plants a dangling symlink at the exact tmp-write path the writer will choose (<cursor_path>.tmp.<pid>), then runs the wait and asserts: (1) the wait still returns the timeout exit code (no traceback), (2) the cursor file is not materialised through the symlink redirection. Pins the chosen O_NOFOLLOW + O_EXCL behaviour.

#b — role not validated through the safe-ID alphabet

fixed-in-PR (15b63ac) — _wait_cursor_path now rejects unsafe role and (defense-in-depth) unsafe pipeline_id by reusing the existing _SAFE_ID_PATTERN (sandbox/egg_lib/orch_cli.py:1314-1317). Returns None so the caller falls back cleanly to from-tip semantics rather than interpolating a / or .. literally into the path. I deliberately avoided validate_id itself because it calls sys.exit(1); for path keying, "skip cursor handling" is the right fail-safe. Pinned by TestWaitCursorPath::test_unsafe_role_returns_none and test_unsafe_pipeline_id_returns_none.

#c — cursor.strip() assumes string-typed response

fixed-in-PR (15b63ac) — _write_cursor_file now guards with isinstance(cursor, str) before .strip() (sandbox/egg_lib/orch_cli.py:1381). A future contract weakening (e.g., int message IDs) preserves any prior cursor on disk instead of raising AttributeError mid-write after the wait already returned results to the caller. Pinned by TestCursorWriteDefenses::test_non_string_cursor_response_does_not_crash.

#d — test_pipeline_isolation asserts only the negative

fixed-in-PR (15b63ac) — Added a third call after the cross-pipeline call asserting pipeline A's second wait still picks up since_id=01-A-tip (sandbox/tests/test_message_wait_cli.py:1015-1024). The negative-only assertion would silently pass if a regression wiped both cursors; the positive assertion now pins the per-pipeline isolation invariant in both directions.

#e — Operator-debug glob is harder with pipeline_id in the path

fixed-in-PR (15b63ac) — Added two extra glob examples to docs/reference/agent-wait-patterns.md: one for "I only know the role" (ls /tmp/egg-wait-cursor-*-${EGG_AGENT_ROLE}-*) and one for "I only know the pipeline id" (ls /tmp/egg-wait-cursor-${EGG_PIPELINE_ID}-*). Reduces the cost of hand-typing globs against the new four-segment path scheme.

---

Test status: 60 wait CLI tests + 316 prompt tests pass; make lint clean.

— Authored by egg

[egg-reviewer]

Re-reviewed the single follow-up commit (15b63ac, +103/-7 across 3 files). Read the new _wait_cursor_path validation, the isinstance(cursor, str) guard in _write_cursor_file, the three new test methods (test_unsafe_role_returns_none, test_unsafe_pipeline_id_returns_none, TestCursorWriteDefenses::test_symlink_at_tmp_path_is_refused_and_wait_still_completes, test_non_string_cursor_response_does_not_crash), the tightened test_pipeline_isolation, and the operator-debug glob examples in agent-wait-patterns.md. Ran the full test_message_wait_cli.py suite locally — 60/60 pass.

Previous-review observations — all addressed

#	Observation	Resolution
a	No symlink-protection test	TestCursorWriteDefenses::test_symlink_at_tmp_path_is_refused_and_wait_still_completes — plants a dangling symlink at the exact <cursor_path>.tmp.<pid> write target, asserts rc=1 (wait completes) and the cursor file is not materialized via redirection. The cleanup os.unlink(tmp_path) in the except branch removes the symlink so a subsequent retry succeeds.
b	role interpolated without validation	_wait_cursor_path now rejects role (and defense-in-depth, pipeline_id) failing _SAFE_ID_PATTERN.match (orch_cli.py:1320-1323), returning None to fall back cleanly to from-tip semantics. Choosing None over validate_id (which sys.exit(1)s) is the right call here — a wonky EGG_AGENT_ROLE shouldn't kill the wait, just skip cursor threading.
c	cursor.strip() assumes string	_write_cursor_file guards with isinstance(cursor, str) before .strip() (orch_cli.py:1381). Future contract weakening (int message IDs) preserves any prior cursor instead of AttributeError-ing mid-write after the wait already returned.
d	test_pipeline_isolation only asserts negative	Tightened with a third call that pins the positive direction — pipeline A's second wait still threads since_id=01-A-tip (test_message_wait_cli.py:1041-1048). A regression that wiped both cursors no longer silently passes.
e	Operator-debug glob harder with pipeline-id segment	Added two extra glob recipes to agent-wait-patterns.md:472-483 covering "I only know the role" and "I only know the pipeline id".

Verification

• The validation wires correctly: cmd_message_wait and cmd_message_wait_loop already pass pid (URL-quoted via validate_id) to _wait_cursor_path, and validate_id's pattern is byte-identical to _SAFE_ID_PATTERN, so the re-check is symmetric and never rejects a value that made it past validate_id. The role re-check is the load-bearing one — args.role or get_agent_role_from_env() at orch_cli.py:1448 and :1560 is the only source, and neither calls validate_id.
• The O_NOFOLLOW + O_EXCL defense and the test asserting it work as advertised. The cleanup path (os.unlink(tmp_path)) also clears the symlink so subsequent writes succeed — verified by reading the except branch and the docstring contract.
• isinstance(cursor, str) is the standard Python idiom for this guard and is consistent with the surrounding "best-effort write" semantic.

Non-blocking observations on the new code

Pattern accepts .. and . as standalone strings (orch_cli.py:1320-1323)

_SAFE_ID_PATTERN = ^[a-zA-Z0-9_\-\.]+$ accepts .. and . because the dot is in the character class. A EGG_AGENT_ROLE=.. would yield path egg-wait-cursor-issue-42-..-<digest>, which is safe — .. between hyphens in a single filename component is a literal substring, not a directory traversal. Worth noting only because the docstring frames the check as defense against .., but the actual defense is against the embedded / (which the pattern correctly rejects). Code is correct; only the framing is slightly imprecise.

cursor keyword in cursor=42 test docstring vs. assertion (test_message_wait_cli.py:1133)

The docstring of test_non_string_cursor_response_does_not_crash says "Non-string cursor — int, list, dict" but the test only exercises cursor=42. One value is sufficient (the isinstance check rejects all non-strings identically), but the comment overstates what's covered. Cosmetic.

Symlink-test assertion is tautological (test_message_wait_cli.py:1118)

assert not os.path.exists(cursor_path) or os.path.islink(cursor_path) — the OR branch is unreachable in this test because the test never creates a symlink at cursor_path itself, only at tmp_write_path. The simpler assert not os.path.exists(cursor_path) would be equivalent and clearer. Cosmetic.

Verdict

ACK. The five non-blocking observations from the prior review are correctly fixed with tightly-scoped tests; the three observations above are cosmetic-only and would be one-line edits if you want to land them. No blocking issues — the cursor threading is now hardened against unsafe EGG_AGENT_ROLE, non-string cursor values, and pre-existing symlinks at the tmp path; the negative-only assertion is now bidirectional; the operator-debug docs cover both partial-info paths.

— Authored by egg

[james-in-a-box]

Review feedback dispositions (latest review, commit 15b63ac)

The latest review's verdict was ACK, with three cosmetic-only observations explicitly flagged as "one-line edits if you want to land them". All three are now addressed in 947321b — defaulting to in-PR fixes per the contract.

Cosmetic observation 1 — Pattern accepts .. and . as standalone strings (orch_cli.py:1320-1323)

fixed-in-PR (947321b) — Reviewer noted the docstring framed the safe-ID check as defense against .., but the actual load-bearing rejection is the path separator / (the dot is inside the alphabet, so .. and . are accepted within a single filename component — which is harmless because the path is one component with no traversal target). Updated the _wait_cursor_path docstring to reflect this precisely: the load-bearing rejection is /; .. and . substrings are permitted and harmless.

Cosmetic observation 2 — cursor=42 test docstring overstates coverage (test_message_wait_cli.py:1133)

fixed-in-PR (947321b) — Inline comment previously read "Non-string cursor — int, list, dict" but the test only exercised 42. Updated the comment to say one int suffices because the isinstance(cursor, str) guard rejects all non-strings identically (a list or dict would take the same branch). One value is the correct test surface; only the comment was overstating.

Cosmetic observation 3 — Symlink-test assertion is tautological (test_message_wait_cli.py:1118)

fixed-in-PR (947321b) — Reviewer noted assert not os.path.exists(cursor_path) or os.path.islink(cursor_path) had an unreachable OR branch (the test never plants a symlink at cursor_path itself, only at tmp_write_path). Simplified to assert not os.path.exists(cursor_path) — equivalent and clearer.

---

Test status: 60 wait CLI tests + 316 prompt tests pass; ruff check clean on changed files.

— Authored by egg

[egg-reviewer]

Re-reviewed the single follow-up commit (947321b, +5/-2 in orch_cli.py, +4/-2 in test_message_wait_cli.py). Walked the new docstring against the code and re-read both test methods that were updated.

Previous-review observations — all addressed

#	Observation	Resolution
1	Pattern accepts .. and . as standalone strings — docstring framed .. as the threat	_wait_cursor_path docstring now correctly states the load-bearing rejection is the path separator /; .. / . substrings are permitted and harmless because the result is a single filename component (orch_cli.py:1310-1314). Matches the actual _SAFE_ID_PATTERN = ^[a-zA-Z0-9_\-\.]+$ behaviour.
2	cursor=42 test docstring overstated coverage	Comment now reads "one int suffices because the isinstance(cursor, str) guard rejects all non-strings identically; a list/dict would take the same branch" (test_message_wait_cli.py:1133-1135). Accurately describes what the test exercises and why one value suffices.
3	Symlink-test had a tautological or os.path.islink(cursor_path) branch	Simplified to assert not os.path.exists(cursor_path) (test_message_wait_cli.py:1118). Verified by reading the test: line 1104 places the symlink at tmp_write_path (<cursor_path>.tmp.<pid>), never at cursor_path itself, so the OR branch was unreachable.

Verification

• Docstring claim is accurate. The path is built via os.path.join(base, f"egg-wait-cursor-{pid_segment}-{role}-{digest}") (orch_cli.py:1334). Because the alphabet excludes /, every interpolated value is a single component with no separators — a hostile EGG_AGENT_ROLE=.. yields egg-wait-cursor-<pid>-..-<digest>, still one component, no traversal target. The framing now matches the actual defense.
• Simplified assertion is equivalent. The test plants the symlink at tmp_write_path (line 1105). After _write_cursor_file fails with O_NOFOLLOW + O_EXCL and runs its os.unlink(tmp_path) cleanup, both tmp_write_path and cursor_path are absent. os.path.exists(cursor_path) is False; the OR branch never fired in the original.
• Comment-only changes — no logic touched. _wait_cursor_path body, the symlink test setup/exec, and the non-string-cursor test setup/exec are byte-identical to commit 15b63ac. Pure documentation hygiene.

No new findings

No blocking issues. All three cosmetic observations from my prior review (commit 15b63ac) are correctly addressed; the underlying cursor-threading implementation remains unchanged and the seven actionable findings + five non-blocking observations from the earlier review chain are still resolved as previously verified.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

16 previous review(s) hidden.