fix(p9): watcher folds gh exit into state; +abandon, +cleanup, --background alias#3
Conversation
…ground alias
Closes five bugs surfaced by a fresh-session test against this very
primitive — proving the dogfood loop works as a feedback channel.
B1 (doc/code drift): AGENTS.md reflexive rule references
`p9 watch <pr> --background`, but the engine only had `--dry-run`. Every
session following the rule hit `error: unrecognized arguments: --background`.
Fix: add `--background` and `--block` as aliases for the (now-default)
foreground behavior.
B2 (architectural — silent state drop): `cmd_watch` spawned
`gh pr checks --watch` detached and returned immediately. When the gh
subprocess exited, *nothing* wrote a state transition. State.jsonl stayed
at WATCHING forever even after CI completed — a direct violation of the
"never silently drop state" cardinal rule.
Fix: foreground-by-default. `p9 watch` blocks on the gh subprocess, then
folds the exit code into a state event:
- exit 0 → WATCHING → GREEN
- exit non-0 → WATCHING → RED_UNCLASSIFIED (caller runs
`p9 heal --classify` next)
The agent invokes `p9 watch` via `run_in_background`, so the bg-task
notification fires when the *whole* watch+fold completes — which is what
the cardinal protocol actually wants. The bg-wrapper is the agent's
harness; `p9 watch` no longer second-guesses with its own detach. Old
behavior is preserved via the new `--detach` flag for fire-and-forget
callers.
B3 (no terminal subcommand for orphans): when a PR is closed via
`gh pr close` (or merged outside `p9 auto-merge`), the local state row was
orphaned. With `max_concurrent_prs=1`, that orphan would deadlock all
future watches.
Fix:
- `p9 abandon <pr> [--reason TEXT]` — explicit terminal transition
(idempotent on already-terminal states; error on unknown PR).
- `p9 cleanup` — polls `gh pr view` for every open row; PRs that GitHub
reports as MERGED or CLOSED get a terminal ABANDONED event (with
reason). PRs still OPEN are left alone. Failed gh queries are
reported but not abandoned (no false-positive drain).
State-machine table extended: ABANDONED now reachable from PUSHED,
GREEN, and MERGE_READY (already had WATCHING/HEALING/RED_*). All
terminal states remain absorbing.
B5 (`load_policy` crashes on str): the function signature claimed
`Path | None` but the body called `.exists()` directly without coercion.
A test caller `p9.load_policy('/some/path')` raised AttributeError.
Fix: accept `Path | str | None`; coerce with `Path(path)` at the
boundary.
Tests: 25 new under tests/test_p9_pr_e.py — watch fold (green/red/dry/
detach/--background/--block aliases), abandon (open/unknown/idempotent/
from-green), cleanup (no-op/merged/closed/open/gh-failure), state-
machine transitions (parametrized over all non-terminals → ABANDONED).
Total now 108 passing (~15s).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
Closes five bugs surfaced by a fresh-session test against this very primitive — proving the dogfood loop works as a feedback channel. The fresh agent followed AGENTS.md's reflexive rule, hit `unrecognized arguments: --background`, and from there exposed cascading state-machine drift.
Bugs closed
(B4 — no `p9` shim in `$PATH` — is a separate UX PR, queued.)
Fixes
B2 — architectural fix (the real one)
`p9 watch` is now foreground-by-default:
The agent invokes `p9 watch` via `run_in_background`, so the bg-task notification fires when the whole watch+fold completes — which is what the cardinal protocol actually wants. The bg-wrapper is the agent's harness; `p9 watch` no longer second-guesses with its own detach.
Old fire-and-forget behavior preserved via new `--detach` flag for callers that genuinely want it.
B1 — flag aliases
`--background` and `--block` added as aliases for the (now-default) foreground behavior. Historic AGENTS.md guidance keeps working without surprise.
B3 — terminal subcommands
State-machine table extended: `ABANDONED` now reachable from PUSHED, GREEN, MERGE_READY (already had WATCHING/HEALING/RED_*). All terminal states remain absorbing.
B5 — `load_policy` accepts `str`
Coerce with `Path(path)` at the boundary.
Tests
25 new tests under `tests/test_p9_pr_e.py`:
Total: 108 passing (was 83). ~15s.
🤖 Generated with Claude Code