docs: rule that a wait is a notification, never a tight poll loop (#2253)
Agents routinely spend turns tight-polling for a condition that was already going to notify them — ~80 consecutive no-op turns waiting out one local:gate run on #2250, with a completion notifier for that very process already armed. The convention existed only in a per-user memory note, which is invisible to anyone else and cannot be relied on mid-session. - AGENTS.md gains a "Waiting on long-running work" section: arm a notifier and stop; one notification is a backgrounded command or a single `until` loop, many is a Monitor. It states the one genuine exception — state the harness cannot observe — and says where the poll belongs in that case, inside one backgrounded loop rather than spread across turns. - pre-push-gate points at it where the gate is actually run. - pr-flow §7 points at it where the Copilot review is actually waited on, with the loop written out. The count-not-existence trap is called out: on round two the first round's review is still there, so `-ge 1` returns immediately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017nLLLTGc37hQiC8s3mNyhq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
edab149d5b
commit
27bf55e0e5
2 changed files with 105 additions and 64 deletions
|
|
@ -240,7 +240,23 @@ gh api graphql -f query='
|
|||
```
|
||||
|
||||
Poll for the review with a `startswith` match — the review login carries a
|
||||
`[bot]` suffix.
|
||||
`[bot]` suffix. **Put that poll in one backgrounded loop that exits when the
|
||||
round lands, and wait for its notification** rather than re-fetching once per
|
||||
turn; a review is remote state the harness cannot observe, which is exactly the
|
||||
exception described in [Waiting on long-running
|
||||
work](../../../AGENTS.md#waiting-on-long-running-work) — and exactly where the
|
||||
poll belongs when one is needed.
|
||||
|
||||
```sh
|
||||
until [ "$(gh api --paginate repos/modelcontextprotocol/inspector/pulls/<N>/reviews \
|
||||
--jq '[.[]|select(.user.login|startswith("copilot-pull-request-reviewer"))]|length' \
|
||||
| awk '{s+=$1} END{print s+0}')" -ge "$EXPECTED" ]; do sleep 20; done
|
||||
```
|
||||
|
||||
`EXPECTED` is the review **count** you are waiting to reach, not `1` — on round
|
||||
two the first round's review is still there, so an existence check returns
|
||||
immediately. Give the inline comments a further ~60s after the body lands; they
|
||||
arrive late (see step 8).
|
||||
|
||||
## 8. Respond to the review
|
||||
|
||||
|
|
|
|||
|
|
@ -37,7 +37,7 @@ not.
|
|||
|
||||
⚠️ **There is no `npm run ci`.** The gate was renamed to `local:gate` (#2146)
|
||||
precisely because `npm ci` is a built-in that clean-installs from the lockfile
|
||||
and does *not* run this script. `npm run ci` now fails with npm's missing-script
|
||||
and does _not_ run this script. `npm run ci` now fails with npm's missing-script
|
||||
error.
|
||||
|
||||
## Verify by exit code, not by grepping output
|
||||
|
|
@ -50,7 +50,14 @@ npm run local:gate; echo "EXIT=$?"
|
|||
```
|
||||
|
||||
⚠️ If you run it as a background task, the harness's "exit code 0" notification
|
||||
describes the *wrapper*, not the gate — read the `EXIT=` line.
|
||||
describes the _wrapper_, not the gate — read the `EXIT=` line.
|
||||
|
||||
**Background it and then wait for that notification** — do not spend turns
|
||||
watching it. The gate takes several minutes, and re-running `tail` or an
|
||||
`echo ok` once per turn until it lands tells you nothing the completion
|
||||
notification would not have; see [Waiting on long-running
|
||||
work](../../../AGENTS.md#waiting-on-long-running-work). Waiting out one run this
|
||||
way cost ~80 consecutive no-op turns on #2250.
|
||||
|
||||
## Diagnosing a failing stage
|
||||
|
||||
|
|
@ -105,7 +112,7 @@ a whitelist, so a module placed outside it falls out of the gate silently.
|
|||
### `smoke:web*`
|
||||
|
||||
⚠️ **An orphaned prod web server from a previous run fakes a rejection.** It
|
||||
answers the readiness probe with *its* token, and the deep link comes back
|
||||
answers the readiness probe with _its_ token, and the deep link comes back
|
||||
`data-deeplink="rejected"` with no error. Assert the port is free before
|
||||
blaming the change.
|
||||
|
||||
|
|
@ -118,7 +125,7 @@ Vite's `fs.allow`. Do a real `npm install` in the worktree.
|
|||
|
||||
⚠️ Two concurrent `npm run local:gate` runs starve each other — ~326 tests time
|
||||
out at 5s. Run one at a time. (A `pgrep -f "npm run local:gate"` wait loop
|
||||
matches *itself* and never exits.)
|
||||
matches _itself_ and never exits.)
|
||||
|
||||
## Local-only steps
|
||||
|
||||
|
|
@ -128,7 +135,7 @@ Two stages have no GitHub CI counterpart, each deliberately:
|
|||
Firefox. Trialled as a CI job and removed (#2086): across a dozen runs it never
|
||||
disagreed with Chromium, and `playwright install --with-deps` carries a real
|
||||
flake surface. Kept in front of a human about to push instead.
|
||||
- **`smoke:tui`** — needs a real TTY. It *is* invoked in CI via `npm run smoke`
|
||||
- **`smoke:tui`** — needs a real TTY. It _is_ invoked in CI via `npm run smoke`
|
||||
and self-skips there on `process.env.CI`, so it needs no guarding.
|
||||
|
||||
A guard (`scripts/lib/workflow-gate.mjs`, run by `npm run test:scripts`) fails
|
||||
|
|
|
|||
Loading…
Reference in a new issue