Skip to content

fix: wake pi crewmates after each turn - #12

Merged
kunchenguid merged 2 commits into
mainfrom
fix/pi-turn-end
Jun 13, 2026
Merged

kunchenguid merged 2 commits into
mainfrom
fix/pi-turn-end

Conversation

@kunchenguid

Copy link
Copy Markdown
Owner

Intent

Fix firstmate's pi crewmate turn-end hook. fm-spawn's generated pi extension listened on pi's 'agent_end' event, which (per pi 0.79.1 type docs) fires only once when the whole agent run exits - so the turn-end backstop never fired at per-turn boundaries for pi crewmates, leaving firstmate reliant on crewmate status appends alone and blind to an idle crewmate that stopped without writing status. Change the hook to 'turn_end', which fires after each turn the agent finishes, matching how the claude/codex/opencode adapters behave. One-line change to the pi-ext template in bin/fm-spawn.sh plus an explanatory comment; no other behavior change.

What Changed

  • Updated the pi harness extension generated by bin/fm-spawn.sh to listen for turn_end instead of agent_end, so firstmate receives a turn-ended signal after each completed pi turn rather than only when the whole agent run exits.
  • Added an inline comment explaining why the pi hook must use turn_end for per-turn idle detection.
  • Documented the pi adapter requirement in AGENTS.md so future harness changes preserve the same watcher behavior.

Risk Assessment

✅ Low: The branch is a narrow one-line event-name correction plus explanatory generated-code comments, with no broader control-flow or API surface changes.

Testing

Baseline bootstrap could not run due missing local firstmate config state, but the focused end-to-end spawn path was exercised: fm-spawn.sh generated and launched a pi extension using turn_end; live pi execution stopped at the trust prompt, which I did not accept because it would change user-level trust outside the worktree boundary, so I supplemented it with a mocked extension event simulation proving the generated hook would touch the turn-ended sentinel. All focused checks passed and temporary worktree test scaffolding was removed.

Evidence: Generated pi extension evidence

The emitted extension registers pi.on("turn_end", ...) and touches state/pi-hook-e2e.turn-ended; it contains no pi.on("agent_end", ...) registration.

// Firstmate turn-end signal; written by fm-spawn.
// Use "turn_end" (fires after each turn the agent finishes), not "agent_end"
// (fires once, only when the whole run exits): the watcher needs a signal at
// every turn boundary so an idle crewmate is surfaced, not just at shutdown.
import { execFile } from "node:child_process";
export default function (pi: any) {
  pi.on("turn_end", () => execFile("touch", ["/Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KV18PMDX505TN87957JNNXTT/state/pi-hook-e2e.turn-ended"]));
}
Evidence: Pi spawn pane transcript

Shows the real spawn command launching pi -e /Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KV18PMDX505TN87957JNNXTT/state/pi-hook-e2e.pi-ext.ts ... before pi displayed its project trust prompt.

treehouse get
01KV18PMDX505TN87957JNNXTT HEAD
❯ treehouse get
🌳 Setting up worktree...
🌳 Entered worktree at ~/.treehouse/01KV18PMDX505TN87957JNNXTT-b8697d/1/01KV18PMDX505TN87957JNNXTT. Type 'exit' to return.
01KV18PMDX505TN87957JNNXTT HEAD
❯ pi -e /Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KV18PMDX505TN87957JNNXTT/state/pi-hook-e2e.pi-ext.ts "$(cat /Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KV18PMDX505TN87957JNNXTT/data/pi-hook-e2e/brief.md)"
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

 Trust project folder?
 /Users/kunchen/.treehouse/01KV18PMDX505TN87957JNNXTT-b8697d/1/01KV18PMDX505TN87957JNNXTT

 This allows pi to load .pi settings and resources, install missing project packages, and execute project extensions.

 → Trust
   Trust parent folder (/Users/kunchen/.treehouse/01KV18PMDX505TN87957JNNXTT-b8697d/1)
   Trust (this session only)
   Do not trust
   Do not trust (this session only)

 ↑↓ navigate  enter select  escape/ctrl+c cancel

───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Evidence: Generated extension event simulation

registered_event=turn_end execFile_command=touch execFile_arg=/Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KV18PMDX505TN87957JNNXTT/state/pi-hook-e2e.turn-ended

registered_event=turn_end
execFile_command=touch
execFile_arg=/Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KV18PMDX505TN87957JNNXTT/state/pi-hook-e2e.turn-ended

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • bin/fm-bootstrap.sh - attempted baseline bootstrap; it failed because this isolated worktree lacks config/crew-harness parent state, so validation continued without creating persistent config.
  • Created temporary data/pi-hook-e2e/brief.md and state/, then ran bin/fm-spawn.sh pi-hook-e2e . pi --scout to exercise the real pi spawn path.
  • Captured state/pi-hook-e2e.pi-ext.ts and the tmux pane transcript into /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T/no-mistakes-evidence/01KV18PMDX505TN87957JNNXTT.
  • node -e '...' asserted the generated pi extension contains pi.on("turn_end"...) and does not register pi.on("agent_end"...).
  • node -e '...' asserted the captured spawn transcript launched pi -e .../state/pi-hook-e2e.pi-ext.ts.
  • node <<'NODE' ... NODE loaded the generated extension text with a mocked pi event emitter and verified triggering the registered callback calls touch on state/pi-hook-e2e.turn-ended.
  • bin/fm-teardown.sh pi-hook-e2e tore down the spawned scout task; git status --short confirmed no temporary worktree scaffolding remained.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@kunchenguid
kunchenguid merged commit d9ec890 into main Jun 13, 2026
3 checks passed
@kunchenguid
kunchenguid deleted the fix/pi-turn-end branch June 13, 2026 20:12
vipentti pushed a commit to vipentti/firstmate that referenced this pull request Aug 5, 2026
* fix(spawn): fire pi turn-end on turn_end, not agent_end

* no-mistakes(document): Document pi turn-end event
@kyokosawada

Copy link
Copy Markdown

Shipped. The middleman is built and running on main: it greets with its protocol and herdr version, streams the fleet and a watched pane over /updates, and accepts send / sendImage / press, all tailnet-only. Closing as delivered.

stoneevenson-biz referenced this pull request in stoneevenson-biz/firstmate Sep 1, 2026
…n it

Quarterdeck reject, second of the same class, and the instruction was the right
one: stop patching the pattern. Three wrong-merge defects came out of this
parser and all three were one shape - a value read from a reference that did not
name it:

  1. the number was matched out of any `*/pull/<digits>`, so a gitlab.com url
     lent its 23 to whichever repository the remotes resolved;
  2. the number survived a url whose repository lost precedence to
     --remote/--repo, merging PR 23 of a repository the url never named;
  3. the slug was read at the FIRST `/pull/` and the number at the LAST, so
     `.../pull/12?next=kunchenguid/pull/99` merged PR 99 while every cross-check saw a
     repository agreeing with itself.

Each was fixed where it was found, and the next arrived through the next door.
A guard against merging the wrong PR had twice resolved the wrong PR itself.

So the matching is gone. `fm_merge_target_parse_pr_url` takes the url apart in
the order a url is defined - fragment off first, then query, then scheme, then
host matched EXACTLY against github.com, then a path of exactly four segments
`<owner>/<repo>/pull/<digits>` - and returns BOTH the repository and the number
from that one parse. They cannot disagree because there is nothing left to
disagree. `fm_merge_target_from_pr_url` and `fm_merge_target_pr_number` are now
thin halves of it rather than two parsers with their own opinions.

The accepted shape is exactly `http(s)://github.com/<owner>/<repo>/pull/<digits>`
with an optional query and fragment naming no second pull request. Four things
are REFUSED rather than repaired: a foreign host; a second `/pull/<n>` in the
query; a second `/pull/<n>` in the fragment; anything trailing in the path
(`/files`, `/commits/abc`, `/12/files/pull/77`). Trailing segments are no longer
trimmed - trimming is how a url that says one thing came to mean another.
Refusing a url a human could have meant costs one trimmed paste; accepting one
costs a merge.

Each rejection is gated as its OWN case with its own name - 5.2 foreign host,
5.3 query, 5.4 fragment, 5.5 trailing path, 5.6 malformed path, 5.7 end to end -
precisely so that no single tweak can silently re-open one: a change that
reopens the fragment hole fails the fragment case by name. That is the point of
splitting them rather than looping over one list.

Behaviour change worth stating: `.../pull/12/files` used to merge PR 12 and now
refuses. That was a deliberate call - it is the same trick as
`/pull/12/files/pull/77` wearing an innocent path, and this parser does not
decide which part of a url the caller meant.

gates/verify.sh: green:41 red:2 - exactly gate-l2-loop-audit-level and
m1-hook-registered, both pre-existing declarations this branch does not touch.
tests/run-all.sh: 64 ran, 2 skipped, 0 failed. shellcheck and bash -n clean.
gate-t1-merge-target-resolution re-frozen, still non-vacuous.
@bingb0t5

Copy link
Copy Markdown

Rebased onto current main; no-mistakes attestation parked until Codex reset 2026-09-15 08:24 ICT.

1 similar comment
@bingb0t5

Copy link
Copy Markdown

Rebased onto current main; no-mistakes attestation parked until Codex reset 2026-09-15 08:24 ICT.

@marano

marano commented Sep 17, 2026

Copy link
Copy Markdown

CI note

One check on this PR, Behavior portable serial 1, was cancelled by the provider without reporting a verdict - not a job failure, and no result to inherit. That cancellation was deliberately not treated as a pass.

The check was re-run and reached a terminal pass (24m10s) on workflow run 35216615138 against head c7e5201d, with every sibling job already green: serial 2-5, parallel 1-2, Lint, Repo invariants, Stock macOS Bash lane, Herdr tests, coverage guard, and the no-mistakes attestation.

The green state on this PR is that rerun's real verdict, not the cancelled attempt. Recorded here so the check history does not read as an unexplained cancellation.

NewAiCoder added a commit to NewAiCoder/firstmate that referenced this pull request Sep 26, 2026
…st starvation (kunchenguid#12)

* fix(fm-lint): bound each file's shellcheck by time and memory

A single pathological file could starve the host: on 2026-09-05 a Fedora-host
fm-lint run sat 21 minutes in disk sleep at 2.7 GB resident on
bin/fm-teardown.sh, getting an unrelated production build killed by the
memory guard four times.

fm-lint.sh now runs shellcheck one file at a time per shard, each under
a wall-clock timeout (FM_LINT_FILE_TIMEOUT, default 120s) and a
ulimit -v memory ceiling (FM_LINT_FILE_MEM_KB, default 1048576 KiB). A
file that hits either bound is reported by name as a lint failure and
the shard continues to its next file instead of hanging or growing
without limit.

Reproduction: shellcheck 0.11.0 --version confirmed pinned; a bounded
ulimit -v 1000000 timeout 60 run against bin/fm-teardown.sh alone
finished in 7 seconds at 549 MB resident, well short of the 21 minute,
2.7 GB incident. That is consistent with the incident being a
resource-starvation interaction under concurrent shard load rather than
a single-file shellcheck defect, so the fix is the general per-file
bound rather than a targeted disable.

Adds test_per_file_bound_reports_and_continues, which stubs shellcheck
to hang past the timeout on one fixture and asserts the run reports
that file by name and still lints the rest. Documents both env vars in
docs/configuration.md.

* no-mistakes(review): Re-arm per-file lint deadline alarm to close SECONDS race

* no-mistakes(review): Recover true exit code when alarm races clean shellcheck exit

* fix(fm-lint): fall back off --external-sources and derive the memory ceiling from host memory

The 6 GiB flat default from the previous commit made canonical lint
permanently red: --external-sources makes ShellCheck recursively analyze
every sourced file, so 27 real files in this repo (measured directly with
systemd-run + /usr/bin/time -v) legitimately exceed even a generous ceiling,
4 of them never stabilizing at all. A single file that hits either bound now
retries once without --external-sources - the same file, no sourced-file
traversal - before being reported as a failure; only a fallback that also
hits a bound is a real failure. docs/fm-lint-external-sources-fallback.md
tracks which tracked files are currently known to need it and why.

The default memory ceiling is also no longer one flat number: it targets
6291456 KiB (6 GiB) but is capped by 70% of this host's own MemTotal divided
by the concurrent shard count (FM_LINT_JOBS), matching the slice_memory_max_pct
convention already used for whole-crewmate memory limits. A flat ceiling times
multiple concurrent shards could authorize more memory than a small CI runner
actually has - exactly how a false-positive on one file becomes collateral
damage on someone else's build. An explicit FM_LINT_FILE_MEM_KB still
overrides the computed default untouched.

Verified against the real pinned ShellCheck (0.11.0), not simulated:
- bin/fm-teardown.sh (one of the two files named in the original incident)
  times out under --external-sources, falls back, and finishes cleanly with
  its own real findings.
- bin/fm-backlog-handoff.sh (the largest measured legitimate peak, ~3.9 GiB)
  passes cleanly with no fallback under the true default (JOBS=2, no
  override) on this host.
- All 30 fm-lint tests pass, including a new real-shellcheck-vs-real-file
  regression test and a deterministic (uname-stubbed) test proving the
  memory-ceiling probe fails closed to timeout-only, once per shard process,
  without itself being misreported as a lint failure.

* no-mistakes(test): Register new fm-lint doc in documentation-audiences inventory

* no-mistakes(ci): Root cause: the PR's new per-file shellcheck fallback (drops --external-sources on a timeout/memory-bound hit) is inherently guaranteed to emit SC1091 ("not following" sourced files, even with `# shellcheck source=` annotations) and SC2329 ("function never invoked") false positives whenever a file relies on cross-file analysis - which is exactly the case for the two files CI's fallback hit (bin/fm-teardown.sh, tests/fm-pending-reply.test.sh). Reproduced the exact CI failure locally with `shellcheck --norc -- bin/fm-teardown.sh` / `tests/fm-pending-reply.test.sh`. Also found one genuine pre-existing defect the fallback exposed for the first time (a truly unused `for i in $(seq 1 100)` loop variable) and one genuine cross-file-only variable (`FM_LOCK_LOG_PREFIX`, consumed by the sourced fm-lock-lib.sh) that only fires without --external-sources. Fix: - bin/fm-lint.sh: the fallback shellcheck invocation now passes `--exclude=SC1091,SC2329` (documented why in a code comment) - these codes are structural artifacts of dropping --external-sources, not real defects, and excluding them at the flag level avoids needing per-line disables on every source line in every file the fallback ever reaches. SC2034 stays fully enforced. - bin/fm-teardown.sh: added a scoped `# shellcheck disable=SC2034` on `FM_LOCK_LOG_PREFIX=teardown` (only visible to ShellCheck with --external-sources, consumed by fm-lock-lib.sh). - tests/fm-pending-reply.test.sh: fixed the genuinely unused loop variable (`for i` → `for _`) and removed it from the function's now-unnecessary `local` declaration. - docs/fm-lint-external-sources-fallback.md: corrected the doc's inaccurate "measured clean" claim to reflect the actual SC1091/SC2329 exclusion behavior. Verified: reproduced the exact CI scenario locally (`FM_LINT_FILE_TIMEOUT=5 FM_LINT_JOBS=1 bin/fm-lint.sh bin/fm-teardown.sh tests/fm-pending-reply.test.sh` now exits 0, forcing the same fallback path CI hit), ran shellcheck directly against both files with the new exclude flags (clean), ran bin/fm-lint.sh's own test suite (all 29 tests pass), and ran tests/fm-pending-reply.test.sh's full suite (all pass, including the edited test)

* no-mistakes(ci): Fixed the Lint CI failure: bin/fm-lint.sh:332 built `fallback_args=(--exclude=SC1091,SC2329)` as a single array element, which ShellCheck's own SC2054 rule flags (looks like a comma-separated list crammed into one bash array slot). Root cause fix, not a suppression: split it into two array elements `(--exclude=SC1091 --exclude=SC2329)`, since ShellCheck accepts repeated --exclude flags (verified locally, rc=0). Confirmed shellcheck --norc on fm-lint.sh is now clean, reran the exact CI fallback repro (FM_LINT_FILE_TIMEOUT=5 FM_LINT_JOBS=1 bin/fm-lint.sh bin/fm-teardown.sh tests/fm-pending-reply.test.sh) which exits 0 with no SC2054 warning, and ran fm-lint's own 29-test suite plus fm-pending-reply's 37-test suite — all pass. No behavior change to the SC1091/SC2329 exclusion intent from the prior commit

---------

Co-authored-by: NewAiCoder <170579485+NewAiCoder@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants