Repository navigation
docs(herdr-delegation): wait only for lane completion - #146
Conversation
|
@codex review |
Codex Review Summary: ❌ FailedThis 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| When the wait returns done, read the reply with `herdr agent read`. A reply without `LANE-READY` | ||
| or `LANE-BLOCKED` means the turn ended while work continues (for example the recipient is | ||
| waiting for CI in the background): treat it as incomplete and wait again from | ||
| `--until working`. Otherwise, for an implementation, check the PR state and report to the user. |
There was a problem hiding this comment.
P1 — The rule "a reply without LANE-READY or LANE-BLOCKED means … incomplete; wait again from --until working" applies to every recipient, but only implementation lanes emit those markers (L50-51). A plan-review recipient (L97-105) finishes with a findings list and no marker. Under this wording, the parent treats a finished review as incomplete and re-runs herdr agent wait <name> --until working. That call has no --timeout, and the reviewer stays idle, so the parent hangs with no limit and never reads or reports the review. Limit the marker check to implementation lanes, e.g. "For an implementation, a reply without …".
| When the wait returns done, read the reply with `herdr agent read`. A reply without `LANE-READY` | ||
| or `LANE-BLOCKED` means the turn ended while work continues (for example the recipient is | ||
| waiting for CI in the background): treat it as incomplete and wait again from | ||
| `--until working`. Otherwise, for an implementation, check the PR state and report to the user. |
There was a problem hiding this comment.
P2 — When the parent waits again from --until working, nothing limits that wait. The 30-minute --timeout only covers the --until done step. Two cases hang forever without telling the user: (a) the lane ends its turn without a marker by mistake and never resumes; (b) the background work finishes and the lane goes working→done before the parent starts the new --until working wait. Add a --timeout to the --until working re-wait too, then check with herdr agent get and report to the user, as the done-timeout path does.
Review summaryI reviewed this PR read-only and found 2 problems (1 blocking).
The |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| `--until working --timeout 1800000`, then `--until done --timeout 1800000`. A review recipient | ||
| carries no marker, so read its findings instead. Otherwise check the PR state and report to the | ||
| user. Treat `LANE-BLOCKED` and its reason as incomplete work. When any wait returns on | ||
| `--timeout`, check `herdr agent get <name>`: if it is blocked, wait again; otherwise report the |
There was a problem hiding this comment.
P2: The new timeout step tells the parent to run herdr agent get <name>, but no pattern in agents/hooks/rules/allowed_commands.json matches it. The herdr allowlist has agent list, agent read, agent wait, workspace get and others, but not agent get. The step exists so the parent can check state on its own while the user approves in the lane pane. As written, every timeout raises a permission prompt or a denial instead. Either use an allowlisted command (for example herdr agent list, which already reports agent state) or add an ^herdr agent get [^ -][^ ]*$ pattern with a test in tests/hooks/test_allowed_command.py.
|
Review summary (read-only) The PR only changes docs: the parent now waits with Findings:
No P0 or P1 issues. 🤖 Generated with Claude Code |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| `--until working --timeout 1800000`, then `--until done --timeout 1800000`. A review recipient | ||
| carries no marker, so read its findings instead. Otherwise check the PR state and report to the | ||
| user. Treat `LANE-BLOCKED` and its reason as incomplete work. When any wait returns on | ||
| `--timeout`, check the recipient's state with `herdr agent list`: if it is blocked, wait again; otherwise report the |
There was a problem hiding this comment.
P2 — On timeout, only blocked leads to waiting again; anything else, including a recipient that is still working, means "report the state to the user". So any implementation lane that runs past 30 minutes (implementation + verification + CI is common) makes the parent stop watching and report a lane that is still in progress. After that, no background wait is left to notice when it finishes. Consider waiting again while the state is working (or blocked) and reporting only for an unexpected or idle state.
| When the wait returns done, read the reply with `herdr agent read`. For an implementation, a reply | ||
| without `LANE-READY` or `LANE-BLOCKED` means the turn ended while work continues (for example the | ||
| recipient is waiting for CI in the background): treat it as incomplete and wait again with | ||
| `--until working --timeout 1800000`, then `--until done --timeout 1800000`. A review recipient |
There was a problem hiding this comment.
P2 — A done reply without a marker always gets re-waited with --until working --timeout 1800000. That assumes the recipient will start working again on its own (for example from a background CI notification). Two cases break this:
- The turn simply ended: the recipient asked a question, failed, or forgot the marker. It never becomes
working, so the parent sits idle for 30 minutes before the timeout path reports it. The old flow treated this as incomplete right away. - The background job wakes the recipient and it finishes (working → done) between the parent's
agent readand the new wait starting.--until workingthen misses that work, and on timeout the state isdone, so the parent reports it as unfinished even though the lane may have printedLANE-READY.
Consider checking herdr agent list / re-reading the reply first, or treating a marker-less reply as something to look at right away rather than always re-waiting.
Review summaryNo blocking findings. I left two P2 inline comments on the new wait rules in
Checks:
|
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| `LANE-BLOCKED` may mean the turn ended while work continues (for example the recipient is waiting | ||
| for CI in the background): check `herdr agent list`. If the recipient is working, or blocked, | ||
| wait again with `herdr agent wait <name> --until done --timeout 1800000`; if it is already done | ||
| or idle, the turn really ended, so report the reply to the user at once. Otherwise check the PR |
There was a problem hiding this comment.
P1: The recheck for a done reply with no marker almost never sees the lane still running, so the case this PR is meant to fix still ends in an early report.
The parent runs herdr agent list right after --until done returned done, so the list will nearly always show done too. Take the example the text itself gives: a lane whose turn ended while it waits for CI in the background. That lane sits in done until the background task finishes and wakes it, which can be minutes later. By the rule here ("if it is already done or idle, the turn really ended, so report the reply to the user at once"), the parent reports the unfinished work straight away and stops waiting. Re-waiting with --until done would not help either, because it returns at once on a lane that already shows done, which is the stale-done problem the earlier paragraph describes.
The PR description says the parent should restart the wait from herdr agent wait <name> --until working when no marker is present, and the doc does not do that. Suggest: when the reply has no LANE-READY / LANE-BLOCKED, wait --until working --timeout <bound>, then --until done --timeout 1800000. Report to the user only if the --until working wait times out.
| state and report to the user. Treat `LANE-BLOCKED` and its reason as incomplete work. When a wait | ||
| returns on `--timeout`, check `herdr agent list` the same way: wait again while the recipient is | ||
| working or blocked, re-read the reply if it is done, and report any other state to the user. This |
There was a problem hiding this comment.
P2: The timeout loop has no limit and never reports. When a wait returns on --timeout and the lane is working or blocked, the parent waits again with no cap and no message to the user.
Example: the user is away and misses Herdr's toast or sound for an approval prompt. The lane stays blocked, and the parent restarts a 30-minute wait over and over without telling anyone. A lane stuck working (a hung command, or a CI poll that never ends) has the same problem. Before this PR, --until blocked at least brought this back to the parent.
Suggest reporting the blocked or long-running state to the user after the first timeout, or after N timeouts, while waiting continues.
Review summary
Checked and fine: 🤖 Generated with Claude Code |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| marker, so read its findings. For an implementation, a reply without `LANE-READY` or | ||
| `LANE-BLOCKED` means the turn ended while work continues (for example the recipient is waiting | ||
| for CI in the background): treat it as incomplete and wait again with | ||
| `--until working --timeout 1800000`, then `--until done --timeout 1800000`. If the | ||
| `--until working` wait times out, the turn really ended: report the reply to the user. Otherwise |
There was a problem hiding this comment.
P2: The "no marker → --until working → timeout means the turn really ended" rule can report a stale reply as final.
- Race: the recipient is waiting for CI in the background, so it shows
done. It can resume (becomeworking) and finish (back todonewithLANE-READY) between the parent'sherdr agent readand its--until workingcall. That wait never seesworking, times out after 30 minutes, and the parent reports the old marker-less reply as the final result. - Slow CI: if the background CI wait takes more than 30 minutes,
--until workingalso times out and the parent concludes the turn ended, while the lane is still waiting to resume.
Suggested fix: when the --until working wait times out, re-read the reply with herdr agent read (or check herdr agent list) and look for a marker before reporting the turn as ended, the same way the --until done timeout path re-reads.
|
Review summary (read-only)
Checked and fine:
No P0/P1 findings. 🤖 Generated with Claude Code |
Wait with --until done --timeout 1800000 instead of also waiting for blocked, so approvals are handled by the user in the lane's pane and not relayed by the parent. Treat a done reply without LANE-READY/LANE-BLOCKED as incomplete, and check `herdr agent get` on timeout. branches.md now refers to herdr-delegation for the handling. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…nd the re-wait Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…gent list Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
bca10cd to
fd002af
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| for CI in the background): treat it as incomplete and wait again with | ||
| `--until working --timeout 1800000`, then `--until done --timeout 1800000`. If the | ||
| `--until working` wait times out, the turn really ended: report the reply to the user. Otherwise | ||
| check the PR state and report to the user. Treat `LANE-BLOCKED` and its reason as incomplete |
There was a problem hiding this comment.
[P2] 「Otherwise check the PR state and report to the user.」が直前の「If the --until working wait times out ...」に続いているため，「--until working が timeout せずに返ったら PR 状態を確認して報告する」と読めます．その読み方だと，lane がまた working に戻った時点で親が報告してしまい，--until done の再待機と，その後に返信を読み直してマーカーを確認する手順が飛ばされます．本来の意図はおそらく「返信に LANE-READY があれば PR 状態を確認して報告する」と，「再待機の --until done が返ったら返信を読み直してマーカー確認からやり直す」の 2 点です．分岐ごとに文を分けて，条件を明示するのがよいと思います．
| `--until working --timeout 1800000`, then `--until done --timeout 1800000`. If the | ||
| `--until working` wait times out, the turn really ended: report the reply to the user. Otherwise |
There was a problem hiding this comment.
[P3] 再待機の --until working --timeout 1800000 には競合があります．親が herdr agent read で返信を読んでいる間に，lane が working に戻ってそのまま done まで進むことがあります（例: バックグラウンドの CI がすぐ終わった場合）．この場合 --until working は 30 分待って timeout し，手順どおりなら「the turn really ended」として古い返信（マーカーなし）を報告してしまいます．timeout 時には herdr agent read で返信を読み直し，前回から変わっていないか（新しく LANE-READY / LANE-BLOCKED が出ていないか）を確かめてから報告するよう書いておくと安全です．
レビュー結果ブロッキングな指摘はありません（P0: 0，P1: 0）．
確認したこと:
補足: PR 本文には timeout 時に 🤖 Generated with Claude Code |
A lane that waits for CI in the background ends its turn and shows done in Herdr, which the parent cannot tell from completion. Lanes now run gh run watch in the foreground until CI finishes, so done means the final report. Remove the marker based re-wait rules. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| When the wait returns done, read the reply with `herdr agent read`. For an implementation, check | ||
| for `LANE-READY`, check the PR state, and report to the user. Treat `LANE-BLOCKED` and its reason | ||
| as incomplete work. When the wait returns on `--timeout`, check `herdr agent list`: keep waiting |
There was a problem hiding this comment.
P2: The PR description says that if the wait returns done but the reply has neither LANE-READY nor LANE-BLOCKED, the parent should treat the lane as unfinished and wait again starting from --until working. That rule isn't written here or in branches.md. With the new design, a lane that ends its turn early (for example, it backgrounds gh run watch despite delivery.md, or stops to ask a question) shows up as done with no marker. The parent then just reads and reports, and doesn't resume waiting. Either add the stated re-wait rule or update the PR description to match.
There was a problem hiding this comment.
The PR description was rewritten for option B before this commit's review: it no longer describes a marker re-wait, and it states that a lane that ends its turn early is read and reported by the parent. The text and the description now agree, so no re-wait rule is added. Recorded in the PR body.
| as incomplete work. When the wait returns on `--timeout`, check `herdr agent list`: keep waiting | ||
| while the recipient is `working` or `blocked`; report any other state to the user. This works for |
There was a problem hiding this comment.
P3: In the timeout branch, any state other than working/blocked falls under "report any other state to the user", and that includes done. If the lane finishes between the 30-minute timeout and the herdr agent list check, the parent will report a bare state and skip the done path (read the reply, check LANE-READY, check the PR). Consider saying "if done, continue as for a done return". (Also, the PR description says herdr agent get but the text uses herdr agent list. Worth making them agree.)
Review summaryFound no blocking issues (no P0/P1). The change is documentation only. Under the new wait flow ( Inline findings:
🤖 Generated with Claude Code |
…nds done Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review summaryNo blocking findings. I found no correctness, security, or test-coverage problems worth raising. What I checked:
🤖 Generated with Claude Code |
Closes #144
変更（案 B）
lane が CI を裏で待つ間ターンを終えると Herdr では
doneに見え，親は本当の完了と区別できない．返答の目印による待ち直しはレビューのたびに新しい穴が見つかったため，原因側を直した．delivery.mdの Wait for CI：lane はgh run watch <run-id> --exit-statusを前面で実行し（Bash タイムアウトは最大の 600000 ms），終わらなければ再実行して，CI が終わるまでターンを終えない．対話中のセッションは従来どおり裏で待つherdr-delegation/SKILL.md：親の待機は--until working→--until done --timeout 1800000（30 分）．doneは lane のターンの終わり（最終報告）とみなす．blockedは待たず，承認待ちを親は中継しない（Herdr の音・トースト・サイドバーと provider の通知 hook に任せる）--timeoutで返ったらherdr agent listで状態を見て，working/blockedなら待ち続け，doneなら done と同じ手順，それ以外は利用者に報告する（herdr agent getは許可リストにないためlistを使う）branches.mdは重複をやめherdr-delegationを参照する目印（
LANE-READY/LANE-BLOCKED）による待ち直しの規則は置かない．lane が規則に反して早く終えた場合は，親が返答を読んで報告する．検証
uv run pytest通過．dprint checkは main 上の同ファイルでも同じ指摘が出る既存の差分codex-reviewは Codex の利用上限で失敗（外部要因）🤖 Generated with Claude Code