feat(bin): surface unexpected new phases in requested operations - #3355
zachlandes wants to merge 6 commits into
Conversation
* A captain-requested operation that unexpectedly grew a new multi-minute phase surfaced nothing at all. The mechanical classifier decides on the leading status verb alone, and that growth arrives on a working: line it is deliberately built to absorb, so the captain waited without learning the operation he asked for had grown materially longer. * Put the rule in the branch verdict criteria rather than the classifier because every predicate it needs is absent from the status stream: whether the captain explicitly asked for the operation, whether the phase is unexpected, and whether an estimate already went out. Only the branch has the request context and the outcome history to judge those, and keying it off the classifier would have meant new persistence. * Kept it strictly additive. The unconditional explicit-request rule and the failure, credential, and security escalations are untouched, and the new rule states that it qualifies none of them. * Held it to surface-once per phase so a repeated unchanged estimate stays quiet, with re-escalation only on a material change, a failure, or a decision. Claude-Session: https://claude.ai/code/session_01ANFgLsgWqDuMCzuG383qmU
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes(review): Restore authorized ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - an explicit captain-requested operation that unexpectedly gains a new multi-minute phase - name the condition and the revised rough duration in the summary. | ||
| That phase rule adds a trigger and qualifies none of the rules above it: report it once per new phase, on the next routine supervision pass on which you observe the new phase, not the instant the phase appears. |
There was a problem hiding this comment.
When an active worker reports an unexpected multi-minute phase in a working: status line, the watcher classifies the update as routine, marks it seen, and queues no wake; unread-status presentation also excludes the line. The branch therefore never observes the phase, so the required captain-facing outcome and revised duration are not emitted.
Knowledge Base Used:
… `working [key=new-phase-...]` updates, routing them through the existing classifier, and allowing exact non-escalation cases to be durably recorded without rendering. Preserved unconditional explicit-request, failure, credential, and security escalations. Added behavioral coverage for routing, correlation tokens, silence, persistence, and prompt rules. Relevant branch, extension, brief, classifier, typecheck, ShellCheck, coverage, and diff checks pass. The known pre-existing fm-watch-triage process-event fixture failure remains unchanged
zachlandes
changed the title
|
Speaking as Kun's firstmate: first look on HEAD Attestation: MATCH ( VISION (files:
CI: SUCCESS Cannot auto-merge new-default. MERGEABLE/CLEAN, MATCH, CI+NM green, safe review — captain-decision hold (flag for Firstmate; I cannot message Firstmate). Not waiting-on-author. Not 14d stale (opened 2026-08-30). Help this PR; no competing PR. |
|
Closing this: we are delivering this behavior in our own local configuration layer instead of upstream, so it no longer needs to land here. Thanks. |
2 participants
Intent
Add ONE narrow rule to firstmate's existing supervision outcome-classification owner so the supervisor surfaces a captain-relevant outcome exactly when an explicit captain-requested operation unexpectedly gains a new multi-minute phase, and stays quiet otherwise.
The exact rule, in the captain's words: when an explicit captain-requested operation unexpectedly gains a NEW multi-minute phase, surface ONE immediate outcome naming the condition and the revised rough duration. Do NOT surface expected waits, background work, ordinary progress, or unchanged/repeated estimates. Surface AGAIN only when the estimate changes materially, the operation fails, or a captain decision is needed. This is surface-once per new phase: an unchanged repeat of the same estimate must not re-surface.
Preserve, do not weaken: (1) the unconditional explicit-request rule in bin/fm-branch-prompt.sh ("Report verdict captain for any outcome that directly answers an explicit captain request ... This rule is unconditional") - the new rule is ADDITIVE to it, never a qualification of it; (2) every failure, credential, and security escalation - none may become conditional on the new rule.
Owner and boundary: the rule had to go to whichever EXISTING owner is correct - bin/fm-branch-prompt.sh (the Pi supervision branch's verdict rules) or bin/fm-classify-lib.sh (the shared captain-relevant-status classifier) - NOT Calm, NOT a new notification channel, NOT any new persistence or control plane. Confirming the owner by reproducing current behavior before editing was a required part of the task.
What was reproduced, and the resulting decision: running the mechanical classifier against the scenario surfaces nothing. status_is_captain_relevant returns false for a "working:" line unconditionally (its verb case returns before any free-text match), so "working: schema migration needs a full table rewrite first, roughly 20 more minutes" classifies identically to plain "working: rewriting the table". bin/fm-branch-prompt.sh was chosen as the owner because every predicate the rule needs is absent from the status stream the shell classifier reads: whether the captain explicitly asked for the operation (no status or meta record carries that fact at all), whether the phase is unexpected, and whether an estimate already went out. Surface-once additionally needs memory of what was already reported, which the branch already has (its persistent conversation plus the durable outcome store) and which the shell classifier could only get by adding new persistence - explicitly out of scope. docs/pi-supervision-branch.md line 63 independently confirms that owner: "The branch prompt owns the verdict criteria, including its unconditional explicit-request rule."
Implementation decisions a reviewer reading only the diff would not know: the new trigger is deliberately placed AFTER the unconditional rule and after every standing escalation so it reads as one more entry rather than a filter over them; it states in its own words that it "adds a trigger and qualifies none of the rules above it"; and it explicitly re-affirms that the failure, credential, and security rules still fire on their own terms. The documentation change patches the existing one-line cross-reference at docs/pi-supervision-branch.md:63 instead of adding a new paragraph, following this repo's one-owner rule (the firstmate-coding-guidelines skill). bin/fm-branch-prompt.sh carries a prefix-stability contract requiring the generated prompt to stay a pure function of tracked files; this addition is static text and the existing byte-stability test still passes.
Regression coverage was required in BOTH directions and is colocated in the existing tests/fm-branch-supervision.test.sh, which already owns the verdict-rule assertions: the one-time escalation naming the condition plus the revised rough duration; the non-escalation cases (expected waits, background work, ordinary progress, unchanged/repeated estimate); the re-surface cases (materially changed estimate, failure, needed decision); and the preserved cases (the unconditional explicit-request rule intact and contiguous, plus the failure, credential, and security escalations). It also asserts ordering so a future edit that turns the new rule into a qualification of the unconditional one fails. The interface under assertion is the prompt generator's stdout - the intentional generated prompt artifact delivered to the agent, an owned text contract, not implementation source bytes used as a proxy for unrelated code. The assertions were mutation-tested in four directions (rule removed, unconditional rule qualified, silence cases dropped, escalation preservation dropped) and each mutation fails the test, so the coverage is not vacuous.
Scope discipline set by the captain: keep this ENTIRELY separate from a different, issue-only semantic-dedup effort. Do not touch Calm presentation, do not add a new notification channel, and do not build any new mechanism beyond the one rule and its tests.
Known pre-existing issue, deliberately NOT fixed here: tests/fm-watch-triage.test.sh has one failing process-event fixture test ("the fixture captured no process-event result"). It was proved pre-existing by running that suite at base commit d71f4b9, which fails the identical test with identical counts (58 ok, 1 not ok), and nothing in the watcher or that suite references any file this change touches. Hand-fixing it inside this run was explicitly ruled out as an unrelated edit; it is tracked as a separate task.
Delivery: this is an UPSTREAM change to kunchenguid/firstmate, shipped as a fork PR from zachlandes/firstmate that AWAITS the upstream maintainer for merge.
What Changed
Risk Assessment
✅ Low: The change is narrowly confined to the authorized generated-prompt contract, documentation, and corresponding interface assertions, with prior out-of-scope mechanisms fully reverted.
Testing
Confirmed the target matches the user-authorized tree and excluded files remain unchanged; after direct test invocation encountered non-executable scripts, both focused suites passed when run through Bash. Manual classifier reproduction confirmed the shell classifier cannot distinguish the scenario, while the generated supervision prompt visibly carries the additive next-pass, surface-once rule and preserves standing escalations.
Evidence: Generated new-phase supervision contract and classifier owner check
Source: Generated new-phase supervision contract and classifier owner check
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
bin/fm-branch-prompt.sh:74- The required behavior remains unreachable for the reproducedworking:scenario. Although this hunk tells the branch to escalate,bin/fm-watch.sh:1429-1504classifies aworking:update from a provably active crew as benign, advances its seen marker, and never queues a wake, so the branch never receives the new-phase information. This contradicts the required criterion to “surface ONE immediate outcome” when the phase appears. Please reconcile the scope by authorizing an existing wake-boundary change that delivers this event to the branch, or revise the required behavior; the prompt-only test does not exercise this failing sequence.🔧 Fix: Report new phases on next supervision pass
1 error still open:
.pi/extensions/fm-branch-supervision.ts:675- The model-visibleverdictdescription still exhaustively lists the old captain cases and says “use routine otherwise.” On the next supervision pass for a requested migration that unexpectedly adds a 20-minute rewrite, the new prompt requirescaptainwhile this tool instruction requiresroutine, leaving the required outcome silently suppressible. Please authorize synchronizing this competing instruction—preferably by making it defer to the branch prompt—despite the requested prompt/docs/tests-only scope.🔧 Fix: Defer report verdicts to branch prompt
✅ Re-checked - no issues remain.
bin/fm-branch-prompt.sh:75- The target reverses the user's later recorded decision: report on the “next routine supervision pass … not the instant the phase appears,” and do not modifybin/fm-classify-lib.sh. This line instead promises immediate delivery, whilebin/fm-classify-lib.sh:145-149adds the expressly excluded classifier route. Restore the authorized next-pass implementation or obtain explicit approval for the new mechanism..pi/extensions/fm-branch-supervision.ts:692- The validation now acceptssilent=truefor every routine task report. For example,{task:"task-9", verdict:"routine", summary:"automatic recovery completed", silent:true}succeeds and line 651 hides it, although it is neither a no-change heartbeat nor a new-phase silence case. This silently suppresses ordinary outcomes without error; enforce the permitted cases at the report boundary or revert this broadening.bin/fm-brief.sh:269- The only added producer instruction is in the secondmate-only charter and requires the secondmate to know that an operation was explicitly captain-requested. A marked request tells it only that MAIN routed the work, not whether the captain originated it; ordinary ship/scout workers receive no reserved-key instruction at all. Thus a captain-requested migration delegated to either path can still emit ordinaryworking:and be absorbed. Keep captain-provenance judgment at the branch and base any delivery signal only on producer-visible facts across supported worker paths, or retain the authorized next-pass semantics.🔧 Fix: Restore authorized next-pass supervision semantics
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
git diff --check d71f4b9cf1e6a8c647867d9a92c67ab0a6bb460f..584067524275d24f9cf584cb0da77d6194f931abDirect execution of both test scripts initially returned permission denied; reran explicitly with Bash.bash tests/fm-branch-supervision.test.shbash tests/fm-pi-branch-extension.test.shGeneratedbin/fm-branch-prompt.shstdout, extracted its model-visible verdict contract, and independently verified the required escalation, next-pass, silence, re-surface, and preserved unconditional-rule clauses.git status --shortconfirmed testing left no transient worktree files.✅ No issues found.
git diff --exit-code fdafe7c -- .pi/extensions/fm-branch-supervision.ts bin/fm-branch-prompt.sh docs/pi-supervision-branch.md tests/fm-branch-supervision.test.sh tests/fm-pi-branch-extension.test.shgit diff --name-only d71f4b9cf1e6a8c647867d9a92c67ab0a6bb460f -- bin/fm-classify-lib.sh bin/fm-brief.sh tests/fm-brief.test.sh tests/fm-classify-corr-token.test.shbash tests/fm-branch-supervision.test.shbash tests/fm-pi-branch-extension.test.shSourcedbin/fm-classify-lib.shand confirmed both the new-phase and ordinaryworking:scenarios remain non-captain-relevant.Generatedbin/fm-branch-prompt.shoutput and captured its complete verdict section as reviewer-visible evidence.git status --shortafter testing confirmed no transient worktree artifacts remained.✅ **Document** - passed
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.