feat(bin): reconcile diverged main and add grooming, review-page, and isolation tooling - #38
Merged
Conversation
fm-herdr-spur.sh: detached daemon that watches herdr agent status via the native pane.agent_status_changed event stream (reusing herdr-eventwait.py) and, on a working->idle/done edge for a tracked EXTERNAL agent, enqueues a check wake into state/.wake-queue so fm-wake-drain surfaces it. Fills the gap where parlay-spawned herdr agents have no firstmate status file or turn-end hook. Poll fallback via herdr agent list when events incapable. Debounced, keyed by agent name, configurable via --agent / config/herdr-spur.agents / all-agents. Read-only against herdr; bash 3.2 safe.
Grooms ready ideas into dispatched work without the captain writing a brief. Per idea: formulate a concrete runnable brief via PAI Inference (plain prompts to dodge PromptGuard), classify safe (research/design/prototype) vs escalate (merge/deploy/production), then dispatch safe via fm-spawn scout or file unsafe to the review store. Safety rails: OFF by default (dry-run unless FM_GROOM_ENABLED=1), rate-limited (FM_GROOM_MAX_IN_FLIGHT), bounded per run (FM_GROOM_MAX_PER_RUN), idempotent (groom:* state label), fail-safe classify (any error escalates). 7 hermetic tests cover the rails; shellcheck-clean.
bin/fm-review-page.ts: for each open review item, write a self-contained, phone-readable HTML page under ~/pulse-pages/review/<id>/ served by Pulse, plus an index at the review root. --all sweeps every open item; positional ids render specific ones. Own dependency-free markdown renderer (headings, fenced code, lists, blockquote, links, bold/italic) in the GitHub-dark house style matching existing pulse-pages. Wires each page URL back onto the item (page_url metadata + a Page: note), idempotently. Read-only on item content except that safe append; parameterized by env for scheduling.
tests/fm-review-page.test.sh: network-free suite stubbing the review CLI on a fakebin PATH (read verbs emit canned JSON, write verbs log calls) and writing pages to a temp FM_REVIEW_PAGE_OUT. Covers single-id render, --all sweep over N items, idempotent re-render (no dup dirs, note skipped on unchanged url), artifact link rendering (url/brain/branch), markdown body rendering, wire-back assertions, empty-queue index, unknown id, and the no-args usage error. shellcheck-clean under the canonical whole-set invocation.
…nderer The inline renderer used a text placeholder to protect `code` spans across the bold/italic/link passes; that placeholder corrupted into NUL bytes and leaked bare CODE0/CODE1 tokens into rendered pages (visible on the live fm-groom item). Replace it with a split-based renderer: split on the code-span capture group so even indices are prose (escaped + emphasized) and odd indices are raw code (escaped, wrapped in <code>). No placeholder token can survive to output. Adds a regression test with a code-span-heavy body (adjacent spans, em-dashes, a multi-span list line, parenthesized spans) asserting every span renders and no CODE placeholder leaks.
…ate, file harvest
… large mined tails
…ry-run, empty, malformed, idempotency, cap
The firstmate-side half of the interactive review loop. Given <id> <verdict> [comment], it durably (a) enqueues a check-kind wake into state/.wake-queue via the sanctioned fm_wake_append helper, keyed review-decision:<id>, so fm-wake-drain surfaces it on the next supervision cycle; (b) annotates the item via 'review note'; (c) appends a JSONL audit record. Fails LOUDLY (non-zero) if the wake or annotation cannot land — never a silent ok (robots-5l8). 8 hermetic tests cover every verdict, the fail-loud path, and --stdin comments.
Turn read-only review pages into decision surfaces. Every page now carries an Approve/Decline/Comment panel that POSTs same-origin to /api/review/decision with in-page success/error feedback, plus a structured What/Why/Stakes/Recommendation/Artifact breakdown parsed from the body so the decision is answerable in place. An already-recorded 'Captain decision:' note renders as a standing-decision banner. Phone-friendly (48px tap targets, no horizontal scroll, self-contained inline JS/CSS). Also fixes the double-nav-bar bug: portal's injectShell stacked /_pulse/nav.js on top of the page's own .topbar. The page now emits <meta name="pulse-shell" content="off"> to opt out — exactly one top bar, matching how /status and /plans compose. 5 new tests (13 total).
Snapshot of uncommitted local edits (AGENTS.md, .claude/settings.json, .codex/hooks.json) so the working tree is clean for merging origin/main. Reversible; preserves local work per the never-discard rule.
…ential HOME override alone doesn't stop Claude Code's ancestor-directory CLAUDE.md walk from re-loading ~/.claude/CLAUDE.md, since firstmate's repo is nested under the real home dir. Launch from a detached worktree mirror under /private/tmp instead (refreshed to HEAD each run), with FM_ROOT_OVERRIDE so bin/ scripts still resolve real state/data/config/projects. Also implements the previously-comment-only keychain credential seeding for first-run auth. Root-caused via a background agent's /context-verified test; confirmed independently by checking the mirror's CLAUDE.md symlink and ancestor chain.
…autonomy The isolated session was still stopping for a tool-approval dialog on every command (e.g. bin/fm-session-start.sh) because its fresh $HOME had no bypass-permissions state, unlike ordinary crewmates which fm-spawn.sh already launches with --dangerously-skip-permissions. Fix: seed settings.json (permissions.defaultMode=bypassPermissions + skipDangerousModePermissionPrompt, re-applied every launch) and .claude.json's bypassPermissionsModeAccepted, plus add --dangerously-skip-permissions to the exec line for parity with fm-spawn.sh:319. Verified live: pane now shows 'bypass permissions on' at startup and runs a command with zero approval prompt.
The isolated session overrides HOME, but ~18 federated store wrapper scripts (brain, robots, task, decisions, ...) hardcode BEADS_DIR=$HOME/data/<store>/.beads at runtime, keyed off the actual process HOME rather than a baked-in path. Under isolation that resolved to a nonexistent path instead of the real Dolt-backed stores. Symlink $ISOLATED_HOME/data -> the real ~/data so those wrappers reach the real federated stores. Exposes only store data, not any PAI CLAUDE.md/hooks/skills/agent config - none of that lives under data/. Verified live: HOME inside the isolated session reported as the isolated home, yet 'brain list' and 'robots list' returned real open issues from the actual stores.
…ints Add bin/fm-bead-stamp.sh (fail-open: stamps dispatch=sent + assigns a linked bead on spawn) and a --beads <id> flag on fm-brief.sh and fm-spawn.sh. All bead-specific logic lives in two new hook directories rather than being spliced directly into fm-brief.sh/fm-spawn.sh, so those files stay a pure addition target with minimal upstream conflict surface: - bin/fm-brief-hooks.d/beads.sh: sourced before fm-brief.sh writes the Brief section; emits the Bead Receipt (dispatch=claimed on brief read) and Bead Closure (close the bead before done:) sections. - bin/fm-spawn-hooks.d/beads.sh: sourced after a successful spawn; stamps the bead via fm-bead-stamp.sh and registers a watcher check that polls the bead for status=closed and wakes firstmate for teardown. Both hook loops run each hook in its own subshell so a hook's `exit` never terminates the calling script, keeping every hook fail-open by construction. fm-spawn.sh records beads_id= in state/<id>.meta when set. --beads is rejected for --secondmate on both scripts.
…hook dir resolution
- bin/claude-account.sh: standalone launcher for per-account Claude Code isolation (CLAUDE_CONFIG_DIR, flock-serialized shared-config symlinks, onboarding/trust-dialog pre-write, settings.json flag pre-write). - bin/claude-1.sh, bin/claude-2.sh: one-line direct launchers. - fm-spawn.sh --account <N>: records account=N in meta, sets CLAUDE_TRUST_DIR to the task worktree, launches through claude-account.sh N. Optional; absent behavior is unchanged. - docs/configuration.md: Multi-account Claude Code section. - tests/claude-account.test.sh, tests/fm-spawn-account.test.sh.
…G.md Adds .agents/skills/herdr-navigation/SKILL.md so agents know how to navigate herdr panes using existing primitives (herdr pane current, neighbor, split, send-text, list). Agents commonly don't know these exist; the skill surfaces them with working examples. Also adds CHANGELOG.md to .gitignore — it is a generated session activity log, not source content.
…enguid#1204) * fix(guard): allow session-local todo tools in the primary The delegation-shape guard denied TaskCreate and TaskUpdate because their normalized names contain the `task` stem. Those tools write only the harness's session-local todo list, which has no executor: it spawns no agent, allocates no worktree, registers no schedule, and starts nothing that outlives the session. That is not the unaccounted work the guard exists to stop, so the stem match was a false positive, and the deny text told the primary to run bin/fm-brief.sh and bin/fm-spawn.sh to create a todo entry. Add a separately-reasoned PLAN_ONLY_TOOLS exact-name exclusion rather than widening OBSERVE_ONLY_TOOLS, whose documented contract is tools that only observe or stop existing work. Both lists stay exact-name so neither can widen by substring. Tests cover the two allowed names and six near-miss names that a substring or shortened-stem widening would release; both mutations were watched red. * no-mistakes(review): drop session-local todo tools from recommended deny list * no-mistakes: apply CI fixes
…claude pid (kunchenguid#1206) * fix(session-lock): resolve Claude bg-spare ancestry to the outermost claude pid fm_harness_ancestry_pid() previously returned the first ancestor process whose command matched a verified harness name. Claude Code's Stop hook fires as a bg-spare worker several levels below the session's actual lock-owning claude process (hook shell -> claude bg-spare -> claude bg-pty-host -> claude -> claude(lock)), so the first match was the bg-spare worker, not the lock owner. fm_session_lock_owned_by_self() then never matched state/.lock, and the Claude Stop auto-arm silently treated its own primary session as an unrelated live owner and never armed the watcher. The walk now keeps going past a claude-named match, looking for a still more ancestral claude-named match, and stops the instant a non-match follows an already-found match (bounding it to a contiguous run rather than the literal ancestry top, so an unrelated claude-named process further up the real process tree is never mistaken for part of this session's own nested chain). Every other harness keeps the original first-match-wins behavior, since e.g. Pi's shared signed-wrapper ancestry actually holds the session at the inner engine pid, not an outer wrapper pid. Hop limit raised from 8 to 16 to cover the deeper bg-spare chain. * no-mistakes(review): Add nested-claude-ancestry regression test; fix nudge doc depth claim * no-mistakes: apply CI fixes
* fix: confirm watcher startup on MSYS * no-mistakes(review): gate MSYS arm ready timeout, cache uname, harden locale test * no-mistakes(review): validate OpenCode ready timeout, make uname cache internal
…d#1195) * fix(spawn): forward firstmate's CLAUDE_CONFIG_DIR to claude crewmates Crewmate panes are created by a long-lived tmux/herdr daemon that does not inherit firstmate's current environment. When firstmate runs under a non-default CLAUDE_CONFIG_DIR (for example a work-vs-personal subscription split), a bare `claude` in the crewmate pane fell back to the default ~/.claude store and launched unauthenticated, blocking the crewmate before it could do any work. fm-spawn now prefixes the claude launch with firstmate's own resolved CLAUDE_CONFIG_DIR when set, so the crewmate uses the same credential/config store firstmate is authenticated with. An unset value is the single-store default and adds no prefix; non-claude harnesses are unaffected. Adds three tests in fm-spawn-dispatch-profile.test.sh (forwarded-when-set, omitted-when-unset, non-claude-ignored) and pins CLAUDE_CONFIG_DIR in the test helper so launch assertions no longer depend on the developer's environment. * no-mistakes: apply CI fixes
…guid#1233) * fix: preserve dispatch harness identity * no-mistakes(review): Fix Grok counterfactual tuple validation * no-mistakes(document): Scope dispatch authentication to selected tuple * fix: restore dispatch instruction budget * no-mistakes(review): Scope dispatch authentication after candidate selection
* fix(bin): handle dash-leading harness process names (#2) * fix: handle dash-leading harness process names * no-mistakes(review): Make dash-leading harness regression hermetic * fix: preserve secondmate reply routes across relative homes Resolve relative home, data, and state inputs before durable charter generation, and fail when caller-relative directories cannot be resolved. Use absolute paths at the related spawn, AFK daemon, and X-mode cross-process handoffs so later processes cannot reinterpret them from another working directory. * no-mistakes(review): Preserve absolute overrides and normalize relative durable paths * no-mistakes(review): Normalize relative home before deriving durable paths * no-mistakes(document): Document relative durable-path normalization * no-mistakes(review): Captain: Ignore inherited CDPATH during relative path normalization * no-mistakes(lint): Fix empty CDPATH assignments for ShellCheck
* Add internal status skill * no-mistakes(document): register /status skill in documentation-audiences inventory * no-mistakes(lint): replace grep|wc -l with grep -c in status skill test * test: silence literal status skill patterns * Refactor bearings default to chat-only --------- Co-authored-by: Kun Chen <[email protected]>
…al coverage (kunchenguid#1304) The second assertion in fm-gitignore-config.test.sh (added by kunchenguid#1261) greps .gitignore for a specific spelling of the config/ ignore pattern. It fails on a semantically equivalent pattern like config/** and does not prove Git actually ignores anything, per the completed source-content-test audit. Replace it with a real git check-ignore control test on a generated unrelated path, and strengthen the existing directory-coverage test with generated unpredictable direct and nested config/ paths.
* fix(herdr): place workers in the launching agent's exact workspace Herdr enforces no workspace-label uniqueness, and spawn resolved its container by taking the FIRST workspace whose label matched the home label. With two workspaces both labeled "firstmate", a worker launched from the second one was created in the first, so it appeared in a different space than the Firstmate the captain was watching. Reproduced end to end on Herdr 0.7.5 protocol 17 by running the real bin/fm-spawn.sh inside a launcher pane in the second "firstmate" workspace: the worker landed in w1 while its launcher was in w2, with an unrelated third workspace focused throughout, which also rules out any dependence on the focused workspace. Placement now binds to the launching process's own Herdr identity. Herdr injects HERDR_PANE_ID, HERDR_SESSION, and HERDR_SOCKET_PATH into every process it manages a pane for, and fm_backend_herdr_launcher_identity resolves that pane's current owning tab and workspace live from Herdr, cross-checking the pane against its tab and confirming the workspace exists exactly once in the session. The injected HERDR_TAB_ID and HERDR_WORKSPACE_ID are creation-time snapshots and are deliberately not read as current identity. Labels are no longer placement authority. A claimed parent identity that is unreadable, contradictory, stale, or from another named session or Herdr server stops the spawn before any worker endpoint exists, rather than degrading to a label search. A launcher with no Herdr ancestry has no workspace to inherit and keeps the per-home labeled container, which must now resolve to exactly one workspace; two same-labeled candidates refuse instead of adopting either. A --secondmate launch keeps standing up that home's own workspace by design. With presentation spaces enabled, the projected child is created and bound under that same exact parent and anchors its ordering on it, so a duplicated home label no longer makes the layout ambiguous. Projection, focus restoration, restart binding, and quarantine rules are unchanged, and children are never collapsed into the parent. tmux, Zellij, cmux, Orca, and the away-mode daemon terminal were each inspected and are not affected: none resolves a container by searching mutable labels. tests/fm-backend-herdr-launcher-workspace-e2e.test.sh drives the real spawn and teardown against an isolated Herdr lab, with its headline case running fm-spawn.sh inside a real Herdr pane so the identity comes from Herdr's own injection. The refusal matrix and the ordering anchor are covered deterministically in tests/fm-backend-herdr.test.sh. Eight existing real-Herdr suites inherited the developer terminal's own Herdr pane into their isolated lab sessions, which the new cross-session check correctly refuses. tests/herdr-test-safety.sh now owns herdr_forget_inherited_pane and those suites call it, so what they assert no longer depends on where they were launched from. Two unrelated fixes found along the way. tests/fm-secondmate-harness.test.sh had the same class of environment leak through CLAUDECODE, which outranks PI_CODING_AGENT in bin/fm-harness.sh and made its pi-signed ancestry case resolve "claude" whenever the suite ran inside Claude Code. And fm-spawn.sh's usage() printed a fixed line range that had already been truncating its own help mid-sentence. * no-mistakes(review): Enforce exact Herdr launcher and projection identity * no-mistakes(document): Document exact Herdr launcher workspace placement
* feat(calm): replace Pi's working row with an animated ship while Calm is on While Calm is active and one logical agent run is under way, Calm now hides Pi's built-in working row and renders a small two-row SSHHIP-derived boat in its place. When Calm is off, Pi's stock working row is left untouched. The presentation uses only public Pi extension API: setWorkingVisible(false) plus a temporary setWidget() component whose render(width) owns the responsive geometry and whose timer requests a TUI render. Visibility follows agent_start through agent_settled, so the boat does not flicker between tool calls, automatic continuations, retries, or compaction inside the same run, and settle, abort, and failure all reach the same cleanup. fm-calm.ts stays the sole owner of the presentation choice and the only caller of setWorkingVisible(); the new lib owns the sprite geometry and widget. * no-mistakes(review): Guarded Calm-off lifecycle visibility writes; focused tests pass * no-mistakes(test): Fixed Calm E2E wait to include tmux scrollback * no-mistakes(document): Document Calm working boat behavior * no-mistakes: apply CI fixes * feat(calm): slow the Calm boat, animate blue water, and make the sail directional The boat now moves one column every 880ms while a bounded fixed-cell water phase advances every 220ms, so the water ripples several times between boat steps and the presentation reads as calm. One scheduler drives both clocks and disposing the widget stops them together; ticks rather than wall-clock timestamps drive every state change, so tests seek animation time exactly. Colors are standard ANSI foreground codes instead of theme lookups: blue for every water cell and yellow for the complete boat, each run closed with a default-foreground reset so nothing bleeds into padding or later frames. ANSI bytes never enter geometry, so visible width stays exact. The mainsail is directional and trails aft of the mast: <| travelling right and |> travelling left. Direction reverses the moment the boat lands on an endpoint, so the endpoint frame already shows the new heading and no frame at or after a bounce shows the previous sail. * test(calm): wait for the Ctrl+O expansion redraw this block asserts * docs(calm): record the revised working-presentation verification evidence * no-mistakes(document): Fix Calm feasibility document EOF whitespace
…henguid#1349) * fix(dispatch): scope candidate authentication to its own surface A locally expired timestamp in one credential store was reported to the captain as a sign-out, including for dispatch candidates that never read that store. A `harness=pi, model=xai/grok-*` candidate authenticates through Pi's own xAI credential, but the only Grok quota reading available was gated on the standalone Grok CLI's separate token, whose expiry clock drifts independently. The always-loaded intake rule then turned that unreadable quota into a mandatory captain escalation. Add `bin/fm-auth-preflight.sh` as the deterministic owner of the parts that must not depend on agent memory: it resolves a tuple's authentication surface from quota-axi's own emitted auth sources rather than from a harness or model name, so another harness's CLI can never gate a candidate that does not use it. A vendor CLI is launched only when the tuple's own harness owns the credential store under test and a non-destructive discovery command is registered for it, which today is `grok models` alone. That probe runs at most once with stdin closed and a hard timeout, reads its verdict from the first stdout line because the command exits 0 either way, treats unrecognized output as indeterminate, and never invokes login, logout, or the interactive TUI. Quota is read at most twice, and unknown headroom never makes a candidate ineligible on its own. Update the dispatch procedure to match: usable authentication with unmeasurable headroom stays eligible at lower preference with the unknown disclosed, and stop-and-report is reserved for unresolved authentication, an unresolved relationship, or malformed configuration. Record that Grok's `credits.remaining` is a prepaid balance rather than window headroom. Gate quota-axi at 0.1.16 in bootstrap, the first build reporting per-credential auth sources. A stale install previously passed the presence check silently, which is why a fix published two days earlier was still not in effect. Replace the orphaned quota-array-dispatch fixtures, which encoded a `provider: "xai"` shape the tool never emits and had no consumer, with fixtures shaped like real 0.1.16 output that the new suite drives the script against. The suite asserts the verdict and, separately, which vendor CLIs were launched, so a Pi/xAI candidate reaching the Grok CLI fails. Map `tests/fixtures/<dir>` to its consuming suite so a fixture change selects the right tests instead of refusing. * refactor(bootstrap): give the quota-axi floor one owner The floor was stated twice - once in bootstrap's gate and once inline in the auth preflight - so bumping it needed two edits that could drift. Move it to bin/fm-quota-axi-lib.sh alongside its rationale, matching the existing tasks-axi library, and derive the comparison from the constant so the number appears exactly once. Bootstrap turns a failing check into the operator diagnostic; the preflight refuses to emit an unscoped verdict. Map the new library to both consuming suites so a bump re-runs them, and record that any usable source means the surface authenticates. * no-mistakes(review): Captain: bound quota checks and removed Python dependency * no-mistakes(review): Captain: enforce conservative headroom and exact preflight retry * no-mistakes(review): Captain: preserve OpenCode eligibility without auth-surface guessing * no-mistakes(review): Captain: reject malformed OpenCode model relationships * no-mistakes(review): Captain: exempt verified unmodeled tuples from intake escalation * no-mistakes(document): Updated dispatch authentication documentation * no-mistakes: apply CI fixes
…nchenguid#1350) * feat(x-mode): reconcile promised public replies deterministically A promised final reply in an X or Discord thread was only kept while the primary remembered it. Compaction or restart erased that memory, so a typed public-followup obligation could sit at pending-work after its PR merged and the original thread never got its reply. Make the promise durable state instead: - bin/fm-public-followup-emit.sh reports a typed terminal work result (source home, work id, generation, outcome, safe deliverables, bounded public-safe text) into the owning home's private inbox. The event id is derived from that identity tuple, so duplicate reports and restart replay converge with no coordination, and nothing ever parses a free-form done: sentence. - bin/fm-public-followup.sh registers a commitment, reconciles events through tasks-axi public-followup, and runs the idempotent delivery sequence (begin-delivery with the payload hash, post, record the posted receipt or a typed error) against the stored platform and opaque thread binding. A delivery interrupted between post and receipt refuses rather than risk a second public reply. - Session start surfaces unresolved commitments from disk, the existing relay poll surfaces a new terminal-result set once, and teardown refuses while this home still owes a public reply for that exact work. tasks-axi public-followup remains the only owner of the obligation state machine, state/x-context/ the only owner of the private request context, and fm-x-reply.sh the only thing that posts. Its new optional --receipt-file is the one addition there, so a caller can record how many messages were sent. A home that never opted into the myfirstmate relay gates out on a single [ -f "$FM_HOME/.env" ] test: no tasks-axi call, no backlog or context scan, no output, and no artifact. Evidence in docs/verification/public-followup.md. * no-mistakes(review): Hardened public-followup reconciliation and ownership guards * no-mistakes(review): Hardened typed terminal cleanup and receipt reconciliation * no-mistakes(review): Automated typed-delivery cleanup and strict backlog validation * no-mistakes(review): Fail-closed parent resolution and registration-safe delivery * no-mistakes(review): Harden relay gating and validate secondmate bindings * no-mistakes(review): Use owner-aware single-gate teardown protection * no-mistakes(document): Correct public-followup documentation drift * no-mistakes(lint): Quote done literals to fix ShellCheck warnings * no-mistakes: apply CI fixes
…chenguid#1327) * feat: add semantic busy-state contract owner and event writer One owner (bin/fm-busy-lib.sh) for the captain-approved semantic busy-state redesign: a per-task gen-bound record written only by bin/fm-busy-event.sh, per-harness trusted-source classification with explicit source attribution, busy/idle/unknown/dead semantics where missing, malformed, stale, or untrusted semantic data is unknown - never idle - and endpoint death is the only process-level override. The Grok-only rendered-tail fallback and the standalone-Kimi verification gate live behind the same classifier. * feat: arm busy-state at spawn and convert Pi to the semantic extension path fm-spawn arms the busy-state contract for converted adapters and seeds busy/fm-spawn (the launch brief is a submitted turn). The Pi/pi-signed per-task extension now reports agent_start -> busy and agent_settled -> idle confirmed by ctx.isIdle(), covering auto-retries, compaction retries, tool loops, and queued continuations, while turn_end stays a wake notification touch. Teardown removes the new record, gen sidecar, and lock. Live-verified on Pi 0.82.0: seed -> agent-start busy -> agent-settled idle with the marker still touched. * feat: convert OpenCode to the semantic session.status plugin path The per-task plugin (renamed .opencode/plugins/fm-busy-state.js) now classifies from OpenCode's semantic session.status events - busy and retry are active, idle is inactive - latched to the worker's own session so a subagent child session can never clear the worker's busy state. The session.idle marker touch stays a wake notification. Teardown removes both the new and the legacy plugin filenames. Live-verified on OpenCode 1.17.18 in a real TUI pane: seed -> session-busy -> session-status-idle. * feat: convert Claude to the full lifecycle hooks path The per-task settings.local.json now wires UserPromptSubmit -> busy and Stop, StopFailure, and SessionEnd -> idle, so API-error and shutdown turn ends can never strand a busy record; Stop keeps the turn-ended notification touch. A refused (stale-gen) event exits 0 and stays silent so Claude's own lifecycle is never broken. Live-verified on Claude Code 2.1.220: UserPromptSubmit fires for the argv launch prompt, Stop closes each turn, a mid-stream Escape interrupt fires no closing hook, and the firstmate-controlled idle/fm-interrupt clear resolves it. * feat: gate Codex busy state behind verified semantic sources The approved contract prefers Codex's app-server turn lifecycle with capability negotiation and sanctions its lifecycle hooks as the intermediate. Live probes on codex-cli 0.145.0 show neither is usable for a pane worker: the app-server daemon is unreachable for a TUI thread and refuses to start outside the managed standalone install, and firstmate-written project hooks never fired (interactive with directory trust granted, and exec, both with --dangerously-bypass-hook-trust) while global hooks fired in the same runs. Codex therefore classifies unknown codex-unverified behind an explicit probe rather than falling back to idle or footer text, and fm-spawn installs no unverified Codex wiring. * feat: gate standalone Kimi busy state on live verification Standalone Kimi has no installed binary here, so per the approved contract its semantic path stays guarded and it classifies unknown kimi-unverified rather than idle - and never from its locale-sensitive moon-phase spinner, which the redesign forbids inventing as a state source. The gate records the preferred source order (Wire prompt request lifetime, which brackets a turn and reports cancellation, then the documented hooks including Interrupt because Stop does not fire on interrupts) and the exact evidence required to open it. Arming without wiring would seed a busy record nothing could clear, so both land together behind the same gate. * feat: route busy consumers through the contract and drop the global OR The watcher, crew-state reader, and away-mode daemon now decide busy state through bin/fm-busy-lib.sh: only an exact busy verdict counts as working, and unknown never becomes working or a silent idle, so a crew whose semantic state is missing, malformed, stale, or unverified surfaces instead of being absorbed. Crew-state reports the producing source in its detail. The watcher's global OR regex default is gone; Grok keeps its isolated fallback inside the contract. The daemon's supervisor-pane reader stays rendered-text - that pane is not a recorded task - but is now scoped to firstmate's own detected harness instead of every vendor signature. Secondmate pending-reply observation is deliberately unchanged and documented as a delivery-confirmation signal, not task state. * docs: point busy-state documentation at the single contract owner Adds a maintainer-architecture section naming bin/fm-busy-lib.sh as the owner of what busy means, with per-adapter sources, the unknown-never-idle rule, the endpoint-death override, and the two rendered-text readers that deliberately stay outside the contract. Replaces the stale regex-first prose in architecture, tmux-backend, herdr-backend, and configuration; converts the harness-adapters per-harness rows from UI signatures to the semantic source each harness uses; and records the live verification evidence, including why Codex and standalone Kimi stay unknown. * fix: arm away-launch signal handlers before acquiring the lifecycle lock fm_afk_launch_main acquired its lock and only then installed the EXIT, INT, and TERM traps. A signal arriving in that window terminated the process by default action and left the lock directory behind, which blocks the next away-mode launch until the stale-owner reclaim path clears it. The release helper only removes a lock this process owns, so the handlers are now armed first. The accompanying test also killed the child whether or not the lock had appeared and sampled cleanup the instant wait returned; it now requires the lock, then allows a bounded settle, so it proves the guarantee instead of racing it. * test: align fleet, Kimi, lifecycle, and detection suites with the contract The fleet snapshot and wake-daemon lifecycle fixtures now prove a working crew through its own semantic busy-state record instead of rendered pane text, which is what those consumers read. The Kimi watcher test asserts the approved contract directly: a standalone Kimi task classifies unknown rather than matching its moon-phase spinner, while Grok's isolated fallback still classifies only Grok. The pi-signed detection cases clear ambient harness markers, fixing a pre-existing failure where the running session's own CLAUDECODE outranked the fixture's marker. * fix: stop teardown from deleting a project's own Codex hooks file An intermediate revision wired Codex through a firstmate-written <worktree>/.codex/hooks.json, and teardown removed it alongside the other generated wiring. The Codex wiring was dropped when its probes came back unverified, so that removal now targets a file firstmate never creates - and a project may legitimately track its own .codex/hooks.json, which teardown would then delete from a pooled worktree. * fix: keep busy-record parsing from disturbing its sourcing caller The record parser split fields with set -- under a temporary noglob, which clobbers a sourcing caller's positional parameters and restores glob expansion even when the caller had disabled it. The watcher, the daemon, and the crew-state reader all source this library, so it now reads fields with read -a, which never globs and never touches caller state. * docs: state exactly which Claude hook paths were reproduced live The busy-state record listed all four wired Claude hooks in the source column, which could read as a claim that every one fired during the pass. UserPromptSubmit and Stop did; StopFailure and SessionEnd are wired from hook names confirmed present in the installed binary, but the abnormal turn ends they cover were not reproduced. * test: let reset_fakes own the crew-state busy-text fixture lifecycle The Grok fallback case set FM_FAKE_BUSY_TEXT and cleared it inline, so the variable's lifetime was owned by one test rather than by the shared reset that every other fake already uses. * no-mistakes(review): Fix semantic busy-state lifecycle races * no-mistakes(review): Make busy-state retirement idempotent * no-mistakes(review): Enforce semantic state boundaries for status and injection * no-mistakes(review): Restore harness-scoped away-mode busy guard * no-mistakes(document): Refresh semantic busy-state documentation * no-mistakes: apply CI fixes
…d#1356) * fix(calm): resume working boat from frozen column across runs Keep one extension-owned boat animation for the Pi session so settling freezes column and direction, the next working period resumes there without hidden-time jumps, and only a fresh session resets to the left edge. * no-mistakes(review): Freeze Calm boat from last rendered state * no-mistakes(document): Document Calm boat continuity contract
* fix(dispatch): judge candidate provider relations instead of rejecting them Firstmate deterministically dropped supported Pi candidates in the openai-codex family. bin/fm-auth-preflight.sh resolved a harness=pi tuple's credential surface by constructing the source id `pi:<model-prefix>`, so `pi + openai-codex/gpt-5.6-terra` looked for a `pi:openai-codex` source. That source does not exist, because Pi's Codex family authenticates through the Codex store quota-axi already lists as `auth-json`/`cli-rpc`. The tuple returned `eligible=no reason=surface-unresolved` while the Pi catalog listed the model and the Codex provider reported fresh, usable credentials with 64 effective percent remaining on its all-model scope. The prefix construction was only ever valid where Pi holds its own credential (`pi:xai`, `pi:kimi-coding`), which is why every previously configured Pi tuple resolved and the defect stayed hidden until a Codex-family Pi model was configured. Retire dispatch eligibility from deterministic shell. The dispatching first mate now establishes model support and provider family from each harness's authoritative catalog, applies quota at the granularity the vendor supplies, and shows that reasoning. Provider-level and all-model evidence bounds every model established in that family; a named-model window bounds only its own model. Missing model-level quota, a missing auth source, unmeasurable headroom, and unmodeled authentication are disclosed uncertainty. Only concrete contradictory evidence blocks a candidate. Replace the preflight with bin/fm-vendor-auth-probe.sh, which keeps the captain's approved bounded probe envelope without any routing knowledge: it takes no harness, model, or provider, reads no quota, renders no verdict, and holds only a fixed-argv safety allowlist. Its behavior suite proves the absent identity surface, the untouched quota, the uniform exit status, the fixed argv with stdin closed, and a real bound even when the configured bound is zero. Also fixed along the way: a zero FM_*_TIMEOUT silently removed the hard bound, the pinned Grok version had drifted to 0.2.117, and --changed selection refused outright on any deleted bin/ script. AGENTS.md section 4 and quota-array-dispatch own the corrected policy, harness-adapters gets the catalog-responsibility correction, and docs/verification/dispatch-auth.md records the 2026-07-30 evidence on Pi 0.82.0, quota-axi 0.1.16, and grok 0.2.117. * no-mistakes(review): Reject all-zero vendor probe timeouts
* docs: add captain-authorized inherent red-check merge exception Keep the default red-PR ban and own one always-loaded exception in the merge-authority section: captain-explicit PR or bounded batch plus exact check, only when the failure is inherent to the selected delivery path. Yolo cannot activate it; final head and the full current check suite must be verified; other substantive failures remain non-waivable. * docs: replace narrow red-check exception with captain precedence Supersede the inherent failing-check merge exception with one always-loaded Firstmate-local rule: a current explicit concrete captain instruction overrides a conflicting Firstmate-written standing rule only within exact scope, never above platform/system/developer instructions. Keep the ordinary red-PR default and yolo boundary; point section 7 at the section 1 owner.
* fix: give validation-time captain overrides a supersession sequence The Validate section let a captain instruction that completely invalidates the work being validated keep the same task and worker, but never said how: the adjacent rule flatly bans hand-editing, committing, aborting, or restarting during an active run with no carve-out, so a worker facing full invalidation had no sanctioned path forward. Add the missing sequence: cancel through no-mistakes axi's abort command, confirm the run has stopped through axi status, recover branch ownership through axi sync's guarded recovery, only then replace the obsolete work, and validate once against the final head. The existing ban on hand-editing an active run now cross-references this sequence instead of contradicting it. * no-mistakes(review): Make validation custody recovery conditional * no-mistakes(document): Clarify validation supersession abort exception * fix: keep obsolete pipeline commits out of the superseded deliverable The review-applied fix made custody recovery conditional on branch_sync.next_action.code, but left an open gap: recovering custody settles who owns the branch, not what content ships. As written, a worker could recover an obsolete run's branch and build the replacement on top of its now-irrelevant commits instead of from the correct pre-invalidation base, carrying obsolete content into the final deliverable. Make that explicit: custody recovery settles ownership, not content, so the worker replaces obsolete work from the correct base and keeps the obsolete run's commits out of what gets validated and shipped. * no-mistakes(test): Restore minimal pre-invalidation replacement instruction * fix: dedupe redundant "replace the obsolete work" restatement Line 309 already says the worker replaces the obsolete work from the correct pre-invalidation base, excluding the obsolete commits. The closing sentence restated "replace the obsolete work" again before gating the final validation run, layering the same fact twice instead of stating it once. Trim the closing sentence to just the ownership gate and the single-run-against-final-head requirement it uniquely adds.
* fix: bind explicit --backend to exact-task authority A Herdr-backed second mate carried a prior one-task --backend tmux exception forward by analogy, so its child landed in tmux and never appeared under the second mate in Herdr. Runtime detection was correct; the authority surface was not. docs/configuration.md now owns that an explicit --backend is authorized only for that exact task. AGENTS.md and fm-spawn help point there. * no-mistakes(document): Consolidate backend selection authorization documentation
…unchenguid#1229) * fix: remove projected workspaces through Herdr's focus-preserving pane-death path Herdr 0.7.5's explicit close of a workspace-emptying last pane moves the attached client's focus to a neighbor workspace, flashing the captain's whole window and routing in-flight keystrokes to the wrong pane until Firstmate's exact-tab restore masks it 56-197 ms later. Teardown and cleanup now plan a workspace-emptying close as a focus-safe removal: verify the close empties the workspace, reposition the doomed workspace behind the focused one through the verified workspace.move transport when it sits before a non-last focused workspace, prove the pane holds one lone idle shell, and end that shell so Herdr removes the emptied workspace through its focus-preserving pane-death path. Any ambiguity or failure falls back to the plain close behind the existing restore backstop, and fm_backend_herdr_kill applies the same plan for non-projected removals. Two conditions proven on real hardware are encoded in the adapter: BSD ps reports a login shell's comm as "-zsh", and an idle shell transiently hosts a prompt helper right after a workspace.move relayout, absorbed by a bounded strict-sample settle window in the idle-shell proof, now the single owner shared with session-start cleanup. An isolated-lab regression reproduces the raw steal on 0.7.5 and proves the plan removes a doomed workspace with zero wrong-focus samples and no corrective focus; unit fixtures cover the position, edge, ambiguity, move and kill failure, escalation, and transient-helper cases. Upstream fixes (#1877 explicit close, #1912 pane death) are merged but unreleased; once released the plan degrades to a harmless reorder-then-remove. * no-mistakes(review): Confirm pane death from structured not-found responses * no-mistakes(review): Serialize Herdr kills and sample focus continuously * no-mistakes(review): Synchronize Herdr focus evidence output * no-mistakes(review): Refuse unlocked Herdr pane closes * no-mistakes(document): Correct Herdr focus-safety documentation * no-mistakes: apply CI fixes * fix: never erase a Herdr task's records while its pane survives a refused close A transient presentation-lock contention could produce a completed teardown while the exact Herdr pane stayed alive as an unowned restored shell: the kill refused the unlocked close (correctly), returned success, the warning was suppressed, and cleanup erased the task's status, turn-end, and metadata records after the isolated copy had already been returned. Teardown now acquires the named-session presentation lock before anything destructive: a contended lock refuses up front while the isolated copy, the task branch, every durable record, and the endpoint are all intact for a plain rerun, and the projected and flat close paths both run under that one held lock instead of acquiring their own. Durable records are erased only once the exact pane is confirmed gone through its structured presence; a refused, skipped, or failed close retains every record with a visible, retryable error, and after a skipped close (unresolvable lock path) only a structured pane_not_found counts as gone - unknown never does. The teardown regression drives a live contending lock holder end to end: the refusal touches nothing (no worktree return, no branch drop, no close attempt), and the retry after release returns the copy, closes the pane under the lock, and removes the records. The unconfirmed projected close now refuses with records retained, and the structured-presence gate has a strict/default unit matrix. * no-mistakes(review): Require structured pane-not-found before Herdr record removal * no-mistakes(document): Correct Herdr record-retention verification date * fix: refuse ambiguity, revalidate SIGKILL ownership, and roll back failed removals Three accepted-contract corrections from the post-CI personal review of the Herdr keep-spaces focus-flash mitigation. Ambiguous endpoint identity no longer counts as a confirmed-gone pane: a missing or malformed target refuses record removal in the structured presence gate, and teardown treats missing confirmation machinery as a refusal instead of skipping the gate, so only an exact structured pane_not_found ever erases durable task records. The pane-death SIGKILL escalation re-reads the exact pane's process information and refuses to signal unless the same shell pid still passes the strict bare-idle ownership proof, so a pid that exited and was reused by an unrelated process is never signaled; the refused escalation falls back to the plain close with the unrelated process untouched. A reposition whose removal is not confirmed no longer outlives the attempt: the emptying-close plan records the verified pre-move order and original index whenever it invokes the mover, and both close owners restore the exact original workspace order through a second verified move, under the same held session lock, before reporting the close as failed. Each defect was reproduced first: the unit matrix documented malformed identity as gone, the PID-reuse regression showed SIGKILL reaching a disowned pid, and the rollback regression showed a single unrestored move. Teardown-level regressions cover unparseable presence retention alongside the strict identity matrix. * no-mistakes(review): Require confirmed Herdr removal and resolvable teardown locks * no-mistakes(review): Enforce structured Herdr closes and teardown preflight * no-mistakes(review): Preflight explicit Herdr close confirmation helper * no-mistakes(document): Document Herdr rollback failure semantics * no-mistakes(review): Captain, harden recursive Herdr teardown safety * no-mistakes(document): Document recursive Herdr teardown evidence * fix: retain nested secondmate home when a recursive child cleanup fails Captain-decided Option A correction for nm-askuser-flash-r6, found during complete-diff rereview of the merged head. cleanup_firstmate_home_children's recursive secondmate branch called itself for a nested child's home without checking the result, then unconditionally removed that home right after. remove_firstmate_home ends in an unconditional recursive delete with no check for leftover records, so a nested secondmate whose own Herdr grandchild failed its confirmed-gone check would have its entire home - retained grandchild records included - erased by the very next line. Guard the recursive call the same way every other fallible call in this function already is: || return 1, skipping remove_firstmate_home and leaving the nested home and its records for a safe rerun. Empirically, fm-teardown.sh's set -eu already halted the script on the prior unguarded call before reaching removal (verified by hand with the guard reverted, under both this session's bash and stock macOS bash 3.2) - the reachable behavior was already correct. The explicit guard is still applied exactly as decided: it matches every sibling call site in the function, and it stops the correctness of this path depending on errexit's well-known fragility under refactors (a wrapping if/&&, or a future subshell) rather than on an explicit check. Adds a teardown-level regression building on the existing direct-child Herdr fixtures: a top-level secondmate contains a nested secondmate, whose own Herdr child's close goes unconfirmed. Proves through the public fm-teardown.sh interface that the nested home, the nested secondmate's own record, and the grandchild's metadata and status all survive, and that the top-level secondmate's record survives too. * no-mistakes(document): Document nested Herdr teardown retention
…d#1431) * fix(dispatch): prioritize quota completion runway * no-mistakes(document): Document completion-aware quota runway selection
…uid#1447) * Preserve task contract in no-mistakes intent * no-mistakes(review): Preserve complete current task contract in no-mistakes intent
Default for exploratory changes during ship tasks: 1. Commit the work (never discard) 2. Mark with [quality:needs-review] 3. File tracking task for curation Preserves thought, keeps decisions durable, makes exploration visible.
…henguid # Conflicts: # .gitignore # AGENTS.md # bin/fm-spawn.sh
Local main and origin/main (trillium/firstmate fork) diverged into 54 ahead / 38 behind commits. This merges both histories forward rather than resetting or squashing, preserving: - 22 genuinely local-only commits (files that exist nowhere on origin): claude-account.sh, fm-idea-mine.ts, fm-review-decision.sh, fm-isolated-launch.sh, fm-groom.sh, fm-review-page.ts, fm-herdr-spur.sh, and their tests/docs. - 32 commits from a deliberate prior upstream-fork sync (kunchenguid/firstmate) that origin also lacks. - ~30 of origin's own merged PRs that local never pulled, including the task-store/beads-authority migration (Stages 2-5) and its write-queue resilience layer. 22 files had genuine three-way conflicts, resolved by intent rather than mechanically: both sides' functionality was kept wherever both were live and non-contradictory, and the more-evolved side was kept where one side had already superseded the other (e.g. the Herdr Mate naming convention, the beads-authority write-queue fallback, and AGENTS.md's captain-instruction-precedence section). No content was silently dropped; add/add conflicts (fm-bead-stamp.sh, fm-brief-hooks.d/beads.sh, etc.) were merged as strict supersets. Also fixes a pre-existing documentation-audience classification gap (docs/documentation-audiences.json was missing two local-only files, .agents/skills/herdr-navigation/SKILL.md and docs/examples/herdr-spur.agents) that predates this merge but would otherwise fail CI's audience-check gate. Verified: full shellcheck lint clean (bin/fm-lint.sh), git diff --check clean, zero remaining conflict markers, all 22 local-only files and beads-migration files present and intact. Full regression suite run (123 tests): 7 failures, every one confirmed pre-existing by reproducing identically against unmodified pre-merge main in an isolated scratch worktree, not merge-introduced. Five are real-Herdr E2E timing/process-shape flakiness inherent to those tests' reliance on precise real-time terminal state capture. Two are local-machine tooling gaps that would not manifest in CI: a missing @earendil-works/pi-coding-agent npm package, and an installed tasks-axi binary (0.2.2) that predates its public-followup subcommand.
….sh double bead stamp
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds Herdr focus-safe cleanup, Claude account and home isolation, Beads lifecycle hooks, idea and review automation, completion-aware dispatch guidance, an external-agent completion bridge, and supporting tests and documentation. ChangesDispatch contracts and workflow guidance
Claude isolation and Beads lifecycle
Idea, grooming, and review workflows
Herdr focus-safe cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (19)
bin/fm-review-page.ts-824-835 (1)
824-835: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe error message drops the HTTP status.
Line 832 concatenates an empty string literal, so the fallback text renders as
request failed (). The status never reaches the captain.resis not in scope in the second.then, so the status has to travel through the first one.🐛 Proposed fix
}).then(function (res) { - return res.json().then(function (data) { return { ok: res.ok, data: data }; }); + return res.json().catch(function () { return null; }).then(function (data) { + return { ok: res.ok, status: res.status, data: data }; + }); }).then(function (r) { if (r.ok && r.data && r.data.ok) { setFeedback('ok', 'Recorded — the first mate has been notified.'); var ts = new Date().toISOString().replace(/\\.\\d+Z$/, 'Z'); lockAfterDecision(verdict, comment, ts); } else { - var err = (r.data && r.data.error) ? r.data.error : ('request failed (' + '' + ')'); + var err = (r.data && r.data.error) ? r.data.error : ('request failed (' + r.status + ')'); setFeedback('err', 'Could not record that: ' + err); setButtons(false); }The added
.catchalso keeps a non-JSON error response (for example an HTML 502 page) on the in-page error path instead of the network.catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-review-page.ts` around lines 824 - 835, Preserve the HTTP status through the promise chain in the fetch handling around the first `.then` and the `r` response object by including it in the object returned after `res.json()`, then use that status in the fallback error text instead of the empty string. Also ensure JSON parsing or non-JSON responses remain on the existing in-page error path rather than reaching the network `.catch`.bin/fm-review-decision.sh-99-115 (1)
99-115: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEscape the remaining JSON control characters.
json_escapehandles",\,\n,\r, and\t. Every other character passes through unchanged, including the control characters U+0000-U+001F. RFC 8259 forbids raw control characters inside a JSON string.COMMENTis captain-supplied text that arrives through the Pulse endpoint, so a pasted body can contain\b,\f, or an escape byte. The resulting line in.decisions.jsonlis then unparseable, andjqrejects that audit record.Add the two named escapes and a
\uXXXXfallback for the rest of the control range.🐛 Proposed fix
case "$ch" in '"') out+='\"' ;; '\') out+='\\' ;; $'\n') out+='\n' ;; $'\r') out+='\r' ;; $'\t') out+='\t' ;; + $'\b') out+='\b' ;; + $'\f') out+='\f' ;; + [[:cntrl:]]) printf -v out '%s\\u%04x' "$out" "'$ch" ;; *) out+="$ch" ;; esac🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-review-decision.sh` around lines 99 - 115, Update json_escape to explicitly escape backspace and form-feed using their JSON short escapes, and encode every remaining character in the U+0000–U+001F range as a four-digit \uXXXX sequence. Preserve the existing handling for quotes, backslashes, newline, carriage return, and tab, while leaving non-control characters unchanged.AGENTS.md-623-645 (1)
623-645: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winConsolidate the duplicate Beads sections.
AGENTS.mdalready defines## Beads Issue Trackerat Line 568. The added block starts at Line 624 with the same heading and repeats the workflow. This triggersmarkdownlintMD024 and creates two sources of truth. Keep one canonical section. Put Codex-specific details in a short subsection or a pointer.As per coding guidelines, keep
AGENTS.mdlimited to knowledge useful to almost every future agent session; point to authoritative files instead of repeating codebase details, and prefer rewriting or pruning over appending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 623 - 645, Consolidate the duplicate Beads Issue Tracker content in AGENTS.md by retaining the existing canonical section and removing the repeated generated block. Preserve only essential Codex-specific guidance as a brief subsection or pointer to the Beads skill and authoritative workflow documentation, ensuring the file has a single Beads heading and no repeated instructions.Sources: Coding guidelines, Linters/SAST tools
docs/configuration.md-271-273 (1)
271-273: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a shell language to the login code fence.
The fenced block starts at Line 271 without a language identifier.
markdownlintreports MD040. Change the opening fence toshorbash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/configuration.md` around lines 271 - 273, Update the fenced code block containing the CLAUDE_CONFIG_DIR login command to specify a shell language identifier, such as sh or bash, on its opening fence to satisfy markdownlint MD040.Source: Linters/SAST tools
docs/scripts.md-32-32 (1)
32-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate
fm-bead-stamp.shrow.
fm-bead-stamp.shappears twice in the table, at Line 32 and at Line 56, with two different purpose clauses. Line 4 states that each row is one purpose clause only. Keep one row and merge the clauses.✏️ Proposed fix
-| `fm-bead-stamp.sh` | Stamp a linked bead's dispatch and lifecycle state when a task is spawned with --beads or auto-linked under the beads backend | +| `fm-bead-stamp.sh` | Fail-open stamp of a linked bead's dispatch and lifecycle state after a spawn with `--beads` or a beads-backend auto-link |Then drop the second row:
-| `fm-bead-stamp.sh` | Fail-open beads dispatch stamp: set `dispatch=sent` and assign a linked bead after spawn |Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/scripts.md` at line 32, Remove the duplicate fm-bead-stamp.sh entry from the scripts table, retaining a single row and merging both purpose clauses into that row. Preserve the table’s one-row-per-script and one-purpose-clause conventions.docs/examples/herdr-spur.agents-8-10 (1)
8-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComment out the example agent names.
Lines 9-10 are active entries, not comments. Line 1 tells the operator to copy this file to
config/herdr-spur.agents. A plain copy therefore restricts the spur tomuseandparlay-dev. Per Lines 4-6 that also disables auto-track. Prefix both names with#so a copied file keeps the documented default until the operator edits it.🛠️ Proposed fix
# Example: only spur firstmate for these two external agents. -muse -parlay-dev +#muse +#parlay-dev🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/examples/herdr-spur.agents` around lines 8 - 10, Comment out the example agent entries in the herdr-spur configuration by prefixing both muse and parlay-dev with #. Keep the explanatory comment unchanged so copying the file preserves the documented default behavior until operators explicitly enable specific agents.bin/backends/herdr.sh-836-846 (1)
836-846: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard an empty
before_orderbefore the mover runs.
before_orderis computed at Line 837 but never validated. If thejqextraction fails, the value is empty. The mover still runs at Line 838, and the recorded move record then carries an empty order field.fm_backend_herdr_emptying_move_rollbackrejects that record as malformed at Line 881, so a real reorder can persist with only a warning. Abort to the plain plan whenbefore_orderis empty, before any mutation.🛡️ Proposed guard
before_order=$(printf '%s' "$list" | jq -c '[.result.workspaces[].workspace_id]' 2>/dev/null) + if [ -z "$before_order" ]; then + echo "warning: herdr presentation cleanup could not record the pre-move workspace order; closing without the focus-safe removal path" >&2 + printf 'plain\n' + return 0 + fi if response=$("$mover" "$socket" "$ws_id" "$len" 2>/dev/null); then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/backends/herdr.sh` around lines 836 - 846, Validate that before_order is non-empty immediately after it is computed in the workspace move flow, before invoking the mover or recording the move. If extraction fails or returns empty, abort this move and continue through the existing plain-plan path, ensuring no mutation or malformed rollback record is produced.bin/fm-teardown.sh-1287-1307 (1)
1287-1307: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRecord the Herdr lock before installing the EXIT handler.
fm_lock_try_acquire "$lock_path"can succeed between leaving the retry loop andtrap teardown_release_herdr_locks EXIT, so a signal in that window can leave the lock unreleased. Record the acquisition state first, then arm the EXIT handler, and use that state to release only the acquired lock inteardown_release_herdr_locks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-teardown.sh` around lines 1287 - 1307, Record the successfully acquired Herdr lock in TEARDOWN_HERDR_LOCK_RECORDS before installing the EXIT trap, closing the signal window in the acquisition path around fm_lock_try_acquire and teardown_release_herdr_locks. Update teardown_release_herdr_locks to release only locks recorded as acquired, while preserving the existing verification and retry behavior.tests/fm-backend-herdr.test.sh-2455-2458 (1)
2455-2458: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the boundary range against a reversed
sedaddress.
agent_line + 1andclose_line - 1form the range. If the close directly follows the agent recheck,agent_line + 1equalsclose_line, so the start address exceeds the end address.sed -n 'N,Mp'withN > Mprints lineN, which is thepane closeline itself. That line does not match thegrep -Evallowlist, soboundary_mutationsbecomes non-empty and the test reports a mutation that does not exist.The current fixture places a
tab listbetween the two calls, so the range is valid today. The assertion inverts as soon as the adapter stops making that intermediate call, which is the strictest and most correct behavior.🐛 Proposed guard
- boundary_mutations=$(sed -n "$((agent_line + 1)),$((close_line - 1))p" "$log" \ - | grep -Ev $'\x1f(tab\x1flist|pane\x1flist|workspace\x1flist)' || true) - [ -z "$boundary_mutations" ] \ - || fail "reclaim mutated between the old pane agent recheck and the close: $boundary_mutations" + boundary_mutations= + if [ "$((agent_line + 1))" -le "$((close_line - 1))" ]; then + boundary_mutations=$(sed -n "$((agent_line + 1)),$((close_line - 1))p" "$log" \ + | grep -Ev $'\x1f(tab\x1flist|pane\x1flist|workspace\x1flist)' || true) + fi + [ -z "$boundary_mutations" ] \ + || fail "reclaim mutated between the old pane agent recheck and the close: $boundary_mutations"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fm-backend-herdr.test.sh` around lines 2455 - 2458, Guard the boundary extraction around boundary_mutations so it runs only when agent_line + 1 is less than or equal to close_line - 1; otherwise treat the range as empty. Preserve the existing grep filtering and mutation failure behavior for valid ranges, preventing sed from printing the pane close line when no lines exist between the calls.tests/fm-teardown.test.sh-1537-1561 (1)
1537-1561: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the test root for Herdr adapter resolution.
Run the mutated
$case_dir/test-root/bin/fm-teardown.sh$ under the test root, notFM_ROOT_OVERRIDE="$ROOT"$, sofm_backend_source herdrcannot load the pristinebin/backends/herdr.sh. As written,missing-adapter,missing-parser$, andmissing-explicit-close-helper` can source the unmutated adapter and report false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fm-teardown.test.sh` around lines 1537 - 1561, Update the teardown invocation for the mutated test binary in the case setup so FM_ROOT_OVERRIDE points to the case-specific test root rather than the pristine ROOT. Preserve the existing state, config, fake Herdr environment, and PATH overrides, ensuring fm_backend_source herdr resolves the mutated adapter for missing-adapter, missing-parser, and missing-explicit-close-helper modes.tests/fm-herdr-session-cleanup.test.sh-31-47 (1)
31-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the
commcase to the fakeps.
fm_backend_herdr_pane_idle_shell_pidonly uses the argv path for this test, butfm_backend_herdr_death_close_paneresamples the same pid viafm_backend_herdr_pid_is_bare_shelland readsps -p <pid> -o comm=there. Without a*-o comm=case, the command returns exit code 4 for that argument list, so the session-cleanup path can fail onkill -HUPrather than the fixture being able to complete the idle-shell shutdown path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fm-herdr-session-cleanup.test.sh` around lines 31 - 47, Add a matching ps fixture branch in FAKE_PS for the “-p 67 -o comm=” invocation, returning the expected bare-shell command name so fm_backend_herdr_pid_is_bare_shell can complete during fm_backend_herdr_death_close_pane. Keep the existing argv and process-list cases unchanged.bin/claude-account.sh-48-54 (1)
48-54: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a dangling symlink at
dest.
[ ! -e "$dest" ]follows symlinks. Ifdestis a symlink whose target was removed from~/.claude/, the test returns true,ln -sthen fails with "File exists", andset -eaborts the launcher while the bootstrap lock is held. Test for the link itself as well, and replace the dangling link.🐛 Proposed fix
for item in commands hooks skills mcp-configs settings.json settings.local.json rules agents; do src="$HOME/.claude/$item" dest="$CLAUDE_CONFIG_DIR/$item" - if [ -e "$src" ] && [ ! -e "$dest" ]; then + if [ -L "$dest" ] && [ ! -e "$dest" ]; then + rm -f "$dest" + fi + if [ -e "$src" ] && [ ! -e "$dest" ] && [ ! -L "$dest" ]; then ln -s "$src" "$dest" fi done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/claude-account.sh` around lines 48 - 54, Update the destination existence check in the loop over commands, hooks, skills, and related entries to detect dangling symlinks by testing the link itself as well as its target. When dest is a dangling symlink, remove or replace that link before running ln -s; preserve the existing behavior for valid destinations and missing sources.bin/fm-isolated-launch.sh-76-78 (1)
76-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--helptruncates the header before the usage and environment sections.
sed -n '2,58p'stops at line 58. In the current file, line 58 is the first# Usage:line, so--helpomits the second usage form, theEnvironment overridesblock, and the forwarding note. The fixed line range also breaks again on every header edit. Print the leading comment block until the first non-comment line instead.🐛 Proposed fix
fm_isolated_launch_usage() { - sed -n '2,58p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//' + awk ' + NR == 1 { next } + /^`#/` { sub(/^# ?/, ""); print; next } + { exit } + ' "${BASH_SOURCE[0]}" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-isolated-launch.sh` around lines 76 - 78, Update fm_isolated_launch_usage to print the complete leading comment block through the first non-comment line instead of using the fixed sed range 2,58. Preserve the existing comment-prefix stripping so --help includes all usage, environment, and forwarding sections and remains resilient to header edits.bin/fm-brief.sh-145-173 (1)
145-173: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject missing task IDs before reading
POS[0].
bin/fm-brief.shruns underset -eu, andID=${POS[0]}is executed before any guard for${#POS[@]}. A caller that provides only flags leavesPOSempty, so the script exits with bash’sunbound variableerror instead of the script usage/error path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-brief.sh` around lines 145 - 173, Validate that POS contains at least one positional argument before assigning ID from POS[0]. Add this guard immediately before the ID assignment, using the script’s existing usage/error path so flag-only invocations produce a deliberate missing-task-ID error instead of a set -u failure.bin/fm-isolated-launch.sh-115-123 (1)
115-123: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCreate the credential temp file with restrictive permissions before writing the token.
The redirection creates
.credentials.json.tmpwith the current umask, andsecuritywrites the OAuth token into it.chmod 600runs only after the write completes. With a default umask of 022, the token is readable by other local users during that window. Set the umask before the redirection so the file is never group- or world-readable.🔒️ Proposed fix
if [ ! -f "$ISOLATED_HOME/.claude/.credentials.json" ] && command -v security >/dev/null 2>&1; then - if security find-generic-password -a "$USER" -s "Claude Code-credentials" -w \ - > "$ISOLATED_HOME/.claude/.credentials.json.tmp" 2>/dev/null; then - chmod 600 "$ISOLATED_HOME/.claude/.credentials.json.tmp" + if (umask 077 && security find-generic-password -a "$USER" -s "Claude Code-credentials" -w \ + > "$ISOLATED_HOME/.claude/.credentials.json.tmp" 2>/dev/null); then mv "$ISOLATED_HOME/.claude/.credentials.json.tmp" "$ISOLATED_HOME/.claude/.credentials.json" else rm -f "$ISOLATED_HOME/.claude/.credentials.json.tmp" fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-isolated-launch.sh` around lines 115 - 123, Update the credential extraction block around security find-generic-password so the temporary file is created with restrictive permissions before the token is redirected into it. Set a restrictive umask for the command or enclosing operation before the redirection, while preserving the existing chmod, successful mv, and cleanup behavior.bin/fm-groom.sh-225-238 (1)
225-238: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe first in-flight guard is dead code.
The condition at line 232 is
[ $((in_flight + dispatched_this_run)) -ge "$MAX_IN_FLIGHT" ]. The condition at line 225 is the same test plus[ "$dispatched_this_run" -ge 1 ]. Line 225 is therefore strictly narrower than line 232, and both bodies are identical. Any idea that reaches line 225 and matches also matches line 232. The first block can never change the outcome.Delete the first block.
♻️ Proposed fix
if [ "$verdict" = "safe" ]; then # (4) SAFE -> dispatch (rate-limited). If we're at the in-flight cap, defer. - if [ "$dispatched_this_run" -ge 1 ] && [ $((in_flight + dispatched_this_run)) -ge "$MAX_IN_FLIGHT" ]; then - action="DEFER dispatch (in-flight cap $MAX_IN_FLIGHT reached)" - if [ "$JSON" -eq 1 ]; then emit_json "$id" "$title" "$verdict" "deferred" "$brief" "$rationale" - else emit_human "$id" "$title" "$verdict" "$action" "$brief" "$rationale"; fi - acted=$((acted + 1)) - continue - fi if [ $((in_flight + dispatched_this_run)) -ge "$MAX_IN_FLIGHT" ]; thenIf the first block was meant to express a different rail, state that rail explicitly; as written the two are indistinguishable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-groom.sh` around lines 225 - 238, Remove the redundant first in-flight guard before the remaining cap check in the dispatch flow. Keep the broader `[ $((in_flight + dispatched_this_run)) -ge "$MAX_IN_FLIGHT" ]` guard and its existing defer output unchanged.tests/claude-account.test.sh-105-112 (1)
105-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe leak assertion greps for a string the shared files never contain.
Lines 105-106 write
{"shared":"leak"}into the shared~/.claude/.credentials.jsonand~/.claude/.claude.json. Line 111 then asserts that the account's.claude.jsondoes not containacct1-secret.acct1-secretis the account's own credential token from line 28. It is not the content of either planted shared file. The launcher generates.claude.jsonfrom the Python block, so that token can never appear there. The assertion therefore passes unconditionally and does not detect a shared-file leak.Grep for the planted marker instead.
🐛 Proposed fix
- assert_no_grep "acct1-secret" "$home/.claude-homes/account1/.claude.json" \ - ".claude.json should not have been overwritten by any shared file" + assert_no_grep "leak" "$home/.claude-homes/account1/.claude.json" \ + ".claude.json should not have been seeded from the shared ~/.claude/.claude.json" + assert_no_grep "leak" "$home/.claude-homes/account1/.claude/.credentials.json" \ + ".credentials.json should not have been seeded from the shared ~/.claude/.credentials.json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-account.test.sh` around lines 105 - 112, Update the leak assertion in the account-launch test to search for the planted shared-file marker "shared":"leak" rather than the account-specific token "acct1-secret". Keep the existing symlink checks and target the generated account .claude.json path so the assertion detects content copied from either shared file.bin/fm-groom.sh-78-78 (1)
78-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe fixed
sedrange printsset -euinto the help output.Line 52 is
set -eu, and the range is2,52p. The help text therefore ends with a strayset -euline. The range also breaks silently every time the header block grows.bin/fm-spawn.shlines 166-169 record this exact failure and derive the range instead.🐛 Proposed fix
-usage() { sed -n '2,52p' "$0" | sed 's/^# \{0,1\}//'; } +# Derived rather than a fixed line range, which truncates or overruns whenever +# the header comment block above changes length (see bin/fm-spawn.sh usage()). +usage() { sed -n '2,${/^`#/`!q;p;}' "$0" | sed 's/^# \{0,1\}//'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-groom.sh` at line 78, Update the usage() function in fm-groom.sh to derive the help-text range from the script’s header markers or structure instead of using the fixed 2,52p range, excluding the set -eu line and keeping help output accurate when the header grows. Follow the dynamic range approach used by fm-spawn.sh.bin/fm-spawn.sh-287-300 (1)
287-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefuse
--accountfor secondmate spawns.The secondmate usage form does not document
--account, butfm-spawn.shaccepts--account=2 --harness=claude --secondmate, resolves Claude as the harness, and then mutates the Claude launch wrapper andCLAUDE_TRUST_DIR. Add the same kind-specific rejection used for--beads, or document secondmate support explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fm-spawn.sh` around lines 287 - 300, Reject --account when KIND is secondmate, alongside the existing --beads kind-specific validation in the argument checks. Preserve account validation for supported crewmate ship or scout spawns and prevent secondmate flow from mutating the Claude launch wrapper or CLAUDE_TRUST_DIR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.codex/hooks.json:
- Around line 13-21: Update .codex/hooks.json entries at lines 13-21 and 41-75:
guard every bd codex-hook command, including SessionStart, PreCompact,
PostCompact, and UserPromptSubmit, with command -v bd so missing dependencies
fail open, and add a 10-second timeout to each hook; prioritize UserPromptSubmit
while applying the same change to all four entries.
In `@bin/fm-groom-json-field.sh`:
- Around line 25-27: Remove the immediate process.exit(0) after
process.stdout.write in the JSON field output path, allowing stdout to flush
fully before the script terminates; alternatively, move termination into the
write completion callback while preserving the existing successful output
behavior.
In `@bin/fm-groom-lib.sh`:
- Around line 114-125: Update the classification flow around fm_groom_infer and
verdict handling to inspect the combined title and brief for the high-risk terms
deploy, merge, credential, rotate, delete, or migrate before accepting the model
verdict. Force verdict=escalate when any term matches, regardless of classifier
output; otherwise preserve the existing safe/escalate parsing and fail-safe
behavior.
In `@bin/fm-groom.sh`:
- Around line 270-280: Update the review CLI branch around REVIEW_BIN and the
review q invocation so a failed or non-zero review q leaves the idea unmarked
and retryable. Preserve the non-fatal behavior required by set -eu by capturing
an empty rid on failure, then only run the groom state update, note, and “FILED
review item” action when rid is non-empty; otherwise use the same retryable
outcome as the missing-CLI branch.
- Around line 240-253: Update the task_id construction in the grooming dispatch
flow to lowercase the idea id before sanitizing it, then reject the dispatch
when the sanitized value is empty or when the resulting task_id is already in
use, including an existing state/<task-id>.meta or task data directory. Preserve
the existing idempotency and accounting behavior by performing these checks
before creating the brief or spawning the crewmate.
In `@bin/fm-herdr-spur.sh`:
- Around line 259-301: Track an explicit subscription acknowledgement flag in
fm_herdr_spur_event_block, set it when the reader emits `@subscribed`, and return
a nonzero failure status if the stream ends without that acknowledgement.
Preserve the existing event processing and successful return path when
`@subscribed` is observed so the caller’s existing sleep/backoff fallback handles
reader or subscription failures.
In `@bin/fm-idea-mine.ts`:
- Around line 797-817: Update fileHarvest to accept the mined harvest data,
build the complete review body after the idea-filing loop when all filed and
skipped IDs are known, and pass that body directly to createIssue instead of
"PLACEHOLDER". Remove the subsequent review update logic in main so the review
is created only once with its final content while preserving the existing
idempotency flow.
In `@bin/fm-spawn-hooks.d/beads.sh`:
- Around line 29-49: Validate FM_HOOK_BEADS_ID and FM_HOOK_ID in the beads hook
before constructing CHECK and interpolating either value, accepting only
characters in [A-Za-z0-9._-] and exiting on invalid input. Update the generated
script’s task show invocation to quote the interpolated bead ID so it is passed
as one argument, while preserving the existing check behavior.
In `@tests/fm-review-page.test.sh`:
- Around line 94-104: Update the run_tool function to explicitly set
FM_REVIEW_PAGE_NO_WIRE to its enabled/default wire-back value when invoking the
tool, preventing inherited environment values from altering test behavior. Keep
the existing FM_REVIEW_BIN, FM_REVIEW_PAGE_OUT, and FM_REVIEW_PAGE_BASE_URL
setup unchanged.
---
Minor comments:
In `@AGENTS.md`:
- Around line 623-645: Consolidate the duplicate Beads Issue Tracker content in
AGENTS.md by retaining the existing canonical section and removing the repeated
generated block. Preserve only essential Codex-specific guidance as a brief
subsection or pointer to the Beads skill and authoritative workflow
documentation, ensuring the file has a single Beads heading and no repeated
instructions.
In `@bin/backends/herdr.sh`:
- Around line 836-846: Validate that before_order is non-empty immediately after
it is computed in the workspace move flow, before invoking the mover or
recording the move. If extraction fails or returns empty, abort this move and
continue through the existing plain-plan path, ensuring no mutation or malformed
rollback record is produced.
In `@bin/claude-account.sh`:
- Around line 48-54: Update the destination existence check in the loop over
commands, hooks, skills, and related entries to detect dangling symlinks by
testing the link itself as well as its target. When dest is a dangling symlink,
remove or replace that link before running ln -s; preserve the existing behavior
for valid destinations and missing sources.
In `@bin/fm-brief.sh`:
- Around line 145-173: Validate that POS contains at least one positional
argument before assigning ID from POS[0]. Add this guard immediately before the
ID assignment, using the script’s existing usage/error path so flag-only
invocations produce a deliberate missing-task-ID error instead of a set -u
failure.
In `@bin/fm-groom.sh`:
- Around line 225-238: Remove the redundant first in-flight guard before the
remaining cap check in the dispatch flow. Keep the broader `[ $((in_flight +
dispatched_this_run)) -ge "$MAX_IN_FLIGHT" ]` guard and its existing defer
output unchanged.
- Line 78: Update the usage() function in fm-groom.sh to derive the help-text
range from the script’s header markers or structure instead of using the fixed
2,52p range, excluding the set -eu line and keeping help output accurate when
the header grows. Follow the dynamic range approach used by fm-spawn.sh.
In `@bin/fm-isolated-launch.sh`:
- Around line 76-78: Update fm_isolated_launch_usage to print the complete
leading comment block through the first non-comment line instead of using the
fixed sed range 2,58. Preserve the existing comment-prefix stripping so --help
includes all usage, environment, and forwarding sections and remains resilient
to header edits.
- Around line 115-123: Update the credential extraction block around security
find-generic-password so the temporary file is created with restrictive
permissions before the token is redirected into it. Set a restrictive umask for
the command or enclosing operation before the redirection, while preserving the
existing chmod, successful mv, and cleanup behavior.
In `@bin/fm-review-decision.sh`:
- Around line 99-115: Update json_escape to explicitly escape backspace and
form-feed using their JSON short escapes, and encode every remaining character
in the U+0000–U+001F range as a four-digit \uXXXX sequence. Preserve the
existing handling for quotes, backslashes, newline, carriage return, and tab,
while leaving non-control characters unchanged.
In `@bin/fm-review-page.ts`:
- Around line 824-835: Preserve the HTTP status through the promise chain in the
fetch handling around the first `.then` and the `r` response object by including
it in the object returned after `res.json()`, then use that status in the
fallback error text instead of the empty string. Also ensure JSON parsing or
non-JSON responses remain on the existing in-page error path rather than
reaching the network `.catch`.
In `@bin/fm-spawn.sh`:
- Around line 287-300: Reject --account when KIND is secondmate, alongside the
existing --beads kind-specific validation in the argument checks. Preserve
account validation for supported crewmate ship or scout spawns and prevent
secondmate flow from mutating the Claude launch wrapper or CLAUDE_TRUST_DIR.
In `@bin/fm-teardown.sh`:
- Around line 1287-1307: Record the successfully acquired Herdr lock in
TEARDOWN_HERDR_LOCK_RECORDS before installing the EXIT trap, closing the signal
window in the acquisition path around fm_lock_try_acquire and
teardown_release_herdr_locks. Update teardown_release_herdr_locks to release
only locks recorded as acquired, while preserving the existing verification and
retry behavior.
In `@docs/configuration.md`:
- Around line 271-273: Update the fenced code block containing the
CLAUDE_CONFIG_DIR login command to specify a shell language identifier, such as
sh or bash, on its opening fence to satisfy markdownlint MD040.
In `@docs/examples/herdr-spur.agents`:
- Around line 8-10: Comment out the example agent entries in the herdr-spur
configuration by prefixing both muse and parlay-dev with #. Keep the explanatory
comment unchanged so copying the file preserves the documented default behavior
until operators explicitly enable specific agents.
In `@docs/scripts.md`:
- Line 32: Remove the duplicate fm-bead-stamp.sh entry from the scripts table,
retaining a single row and merging both purpose clauses into that row. Preserve
the table’s one-row-per-script and one-purpose-clause conventions.
In `@tests/claude-account.test.sh`:
- Around line 105-112: Update the leak assertion in the account-launch test to
search for the planted shared-file marker "shared":"leak" rather than the
account-specific token "acct1-secret". Keep the existing symlink checks and
target the generated account .claude.json path so the assertion detects content
copied from either shared file.
In `@tests/fm-backend-herdr.test.sh`:
- Around line 2455-2458: Guard the boundary extraction around boundary_mutations
so it runs only when agent_line + 1 is less than or equal to close_line - 1;
otherwise treat the range as empty. Preserve the existing grep filtering and
mutation failure behavior for valid ranges, preventing sed from printing the
pane close line when no lines exist between the calls.
In `@tests/fm-herdr-session-cleanup.test.sh`:
- Around line 31-47: Add a matching ps fixture branch in FAKE_PS for the “-p 67
-o comm=” invocation, returning the expected bare-shell command name so
fm_backend_herdr_pid_is_bare_shell can complete during
fm_backend_herdr_death_close_pane. Keep the existing argv and process-list cases
unchanged.
In `@tests/fm-teardown.test.sh`:
- Around line 1537-1561: Update the teardown invocation for the mutated test
binary in the case setup so FM_ROOT_OVERRIDE points to the case-specific test
root rather than the pristine ROOT. Preserve the existing state, config, fake
Herdr environment, and PATH overrides, ensuring fm_backend_source herdr resolves
the mutated adapter for missing-adapter, missing-parser, and
missing-explicit-close-helper modes.
---
Nitpick comments:
In @.claude/settings.json:
- Around line 13-20: Update the hook command associated with “bd prime
--hook-json” to first verify that the bd executable is available on PATH, and
only run bd prime when it is; otherwise exit silently. Preserve the existing
hook behavior when bd is installed and follow the file’s established
shell-command style.
In `@bin/backends/herdr.sh`:
- Around line 2763-2793: The plan parsing, death-or-explicit close handling,
repositioned-workspace confirmation, and rollback logic is duplicated between
the shown flow and fm_backend_herdr_projection_close_pane_focus_preserving.
Extract this shared sequence into one helper that returns the close status, then
have both callers invoke it while retaining their existing focus-restoration and
return-code policies.
In `@bin/claude-account.sh`:
- Line 62: Add a python3 availability guard before the bootstrap heredoc in
bin/claude-account.sh, using the existing command -v pattern from
bin/fm-isolated-launch.sh. If python3 is unavailable, emit a clear message and
either exit appropriately or skip the Python bootstrap so execution can continue
to exec claude.
In `@bin/fm-brief.sh`:
- Line 518: Rewrite the instruction near the no-mistakes `--intent` handling as
separate short sentences. State independently that it must preserve relevant `#
Task` content, include later accepted requirements, keep only each requirement’s
current accepted form, retain direct requirements rather than a diff summary,
and exclude generic scaffold boilerplate unless task-specific.
- Around line 175-179: In the BEADS_ID validation block, simplify the case
pattern and nested test so the validation rule is expressed directly: allow an
empty ID, but reject non-empty IDs containing characters outside A-Za-z0-9._-.
Preserve the existing error message and exit behavior.
In `@bin/fm-groom-lib.sh`:
- Around line 47-55: Update fm_groom_infer to bound both inference command
branches with timeout when the timeout utility is available, preserving the
existing FM_GROOM_INFERENCE and bun execution paths. Use the existing
standard-tier 30-second ceiling, and fall back to the current unwrapped commands
when timeout is unavailable so callers continue receiving the command’s non-zero
status on timeout.
In `@bin/fm-groom.sh`:
- Around line 257-262: Update the spawn invocation in the dispatch loop around
SPAWN_BIN to redirect its output to a per-task log file instead of /dev/null.
Include that log file’s name or path in the failure action assigned to action,
while preserving the existing success and failure handling.
- Line 96: Extend the prerequisite validation block in bin/fm-groom.sh alongside
the existing IDEAS_BIN check to run command -v for SPAWN_BIN before any grooming
dispatch or idempotency marker is written. Preserve the current fail-fast
error-and-exit behavior and use SPAWN_BIN’s configured value in the diagnostic.
In `@bin/fm-isolated-launch.sh`:
- Around line 155-165: Update the Python block that edits .claude.json to catch
JSON parsing or file-load failures using the same try/except pattern as the
preceding settings block, defaulting to an empty object on failure while
preserving all existing keys on successful parses before setting
bypassPermissionsModeAccepted.
In `@bin/fm-review-page.ts`:
- Around line 558-582: Rename the local regex-escaped key variable in the loop
over spec, currently named esc, to a distinct name and update both headRe and
inlineRe template expressions to use it, preserving the module-level esc() HTML
escaper name.
- Around line 1045-1111: Update the HELP text to document main’s exit codes 0,
1, 2, and 3. Propagate missing-ID information from fetchByIds to main, and
return a non-zero status when any requested ID is absent, while preserving the
existing all-miss behavior and successful rendering of found items. Ensure --all
is unaffected and the partial-miss result is clearly reported to batch callers.
In `@bin/fm-spawn.sh`:
- Around line 1913-1925: Update the hook contract header near the existing
“sourced” documentation to explicitly state that hooks must contain
Bash-compatible code because their shebangs are ignored when sourced. Keep the
current executable-file check, subshell isolation, and fail-open behavior
unchanged.
In `@docs/herdr-backend.md`:
- Line 112: Split the sentence at the transition from removal confirmation to
rollback behavior. Keep the move-to-last ordering and exact-workspace
confirmation claims in the first sentence, then state separately that an
unconfirmed removal makes one verified rollback attempt under the same session
lock to restore the workspace to its exact original position.
In `@tests/claude-account.test.sh`:
- Around line 116-126: Update test_prewrites_onboarding_and_trust_dialog to
include status in its local variable declaration alongside rec, home, fakebin,
log, and trust_dir, ensuring the subsequent exit-code assignment remains scoped
to the test.
In `@tests/fm-backend-herdr-focus-flash-e2e.test.sh`:
- Around line 140-151: Throttle the background sampler loop in the subshell that
checks B_OPERATION_ACTIVE by adding a short delay on each iteration, including
iterations without a sample. Use an approximately 10 ms tick before the next
loop iteration so focus_snapshot is not called in a tight loop while preserving
the existing sampling and UNREADABLE handling.
In `@tests/fm-backend-herdr.test.sh`:
- Around line 1508-1512: Update the SIGHUP-trapping fixtures in
tests/fm-backend-herdr.test.sh at lines 1508-1512, 1544-1548, 1613-1617, and
1657-1661 to add exec before sleep 300 in each bash command, so bgpid directly
identifies the process being signaled; apply this consistently in
test_projection_close_death_escalates_sigkill_after_sighup_survival,
test_projection_close_death_failure_falls_back_to_plain_close,
test_projection_close_death_never_sigkills_a_reused_pid, and
assert_projection_close_failed_removal_rolls_back_the_reposition.
In `@tests/fm-beads-hooks.test.sh`:
- Around line 126-134: Add coverage adjacent to
test_brief_beads_rejects_invalid_id for the missing-value and option-as-value
--beads paths, asserting exit code 1 and the “requires a value” message, and for
the --beads=<id> form, asserting successful execution and a rendered “# Bead
Receipt” section in the generated brief.
- Around line 265-291: Add a test case alongside
test_spawn_hook_is_fail_open_without_task_cli and
test_spawn_hook_noop_without_beads_id that sets FM_HOOK_BEADS_ID but omits
FM_HOOK_STATE (while providing the other required context), then invokes
beads.sh and captures output and status. Assert exit status 0 and the
missing-context warning text, covering the fail-open branch for empty
FM_HOOK_ID, FM_HOOK_STATE, or FM_HOOK_ROOT.
In `@tests/fm-brief.test.sh`:
- Around line 237-242: Reuse the existing brief variable in the assertions
around the direct-intent case instead of repeating the literal brief path. After
changing id for the direct project, reassign brief to the corresponding brief.md
before the second assert_no_grep, and update the pass message to mention both
authority assertions and the --intent contract.
In `@tests/fm-groom.test.sh`:
- Around line 203-212: Add two safety-path test cases in tests/fm-groom.test.sh:
seed enough state/groom-*.meta files to reach FM_GROOM_MAX_IN_FLIGHT, then
assert run_groom reports a deferred outcome; also make the review mock exit
non-zero for an unsafe idea and assert the expected failure behavior while
verifying the idea is still marked groom:escalated. Reuse the existing
reset_mock_state, run_groom, and assertion patterns.
In `@tests/fm-idea-mine.test.sh`:
- Around line 237-255: Add a `--force` scenario to
`test_idempotency_marker_skips_unchanged_tail` (or a dedicated adjacent test)
that reruns the unchanged transcript with the force option, verifies filing
occurs despite the marker, and confirms the normal non-forced rerun still
remains unchanged. Exercise the `!cfg.force` decision in `fm-idea-mine.ts`
without altering the existing marker-hit assertions.
In `@tests/fm-review-decision.test.sh`:
- Around line 237-244: Add a final completion-marker echo after the
t_missing_args call in the test sequence, matching the sibling test files’
format and clearly indicating that all fm-review-decision tests passed.
- Around line 191-202: The t_note_failure test currently verifies only the
non-zero result and diagnostic; extend it to assert that the queue retains the
failed decision’s review-decision wake after run_decision returns. Use the
existing queue inspection/assertion helpers and verify the key for review-znt
remains present, covering the wake enqueued before review note failure.
In `@tests/fm-review-page.test.sh`:
- Around line 465-477: Add a test case to the listed suite in
tests/fm-review-page.test.sh that exercises the wire-back failure path in
bin/fm-review-page.ts: stub the relevant wire-back verb to fail while allowing
page writes to complete, then assert the command exits with status 3. Follow the
existing single-verb failure stub pattern from fm-review-decision.test.sh and
preserve the other cases unchanged.
In `@tests/fm-spawn-account.test.sh`:
- Around line 141-160: Add a test covering the equals-sign syntax
`--account=<N>` in the account-flag test suite, mirroring the existing positive
or rejection assertions and verifying the same behavior as the space-separated
form. Define the test near `test_account_flag_rejects_non_positive_integer` and
invoke it with the other account flag tests.
In `@tests/fm-teardown.test.sh`:
- Around line 1445-1455: Bound the background holder’s wait loop in the shell
launched by the contending-lock setup, using a finite iteration counter or
timeout while waiting for $RELEASE. Ensure the holder exits even when the parent
test fails before creating the release marker, while preserving normal lock
release when the marker appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50467f5b-c620-48f8-b0db-b04689442037
📒 Files selected for processing (55)
.agents/skills/firstmate-coding-guidelines/SKILL.md.agents/skills/harness-adapters/SKILL.md.agents/skills/herdr-navigation/SKILL.md.agents/skills/quota-array-dispatch/SKILL.md.claude/settings.json.codex/hooks.json.github/workflows/ci.yml.gitignoreAGENTS.mdREADME.mdbin/backends/herdr.shbin/claude-1.shbin/claude-2.shbin/claude-account.shbin/fm-brief.shbin/fm-groom-json-field.shbin/fm-groom-lib.shbin/fm-groom.shbin/fm-herdr-session-cleanup.shbin/fm-herdr-spur.shbin/fm-idea-mine.tsbin/fm-isolated-launch.shbin/fm-review-decision.shbin/fm-review-page.tsbin/fm-spawn-hooks.d/beads.shbin/fm-spawn.shbin/fm-teardown.shbin/fm-test-isolation-proof.shbin/fm-test-run.shdocs/architecture.mddocs/configuration.mddocs/documentation-audiences.jsondocs/examples/crew-dispatch.jsondocs/examples/herdr-spur.agentsdocs/herdr-backend.mddocs/scripts.mddocs/verification/dispatch-auth.mddocs/verification/runtime-backends.mdtests/claude-account.test.shtests/fm-backend-herdr-focus-flash-e2e.test.shtests/fm-backend-herdr-presentation-e2e.test.shtests/fm-backend-herdr.test.shtests/fm-beads-hooks.test.shtests/fm-bearings-snapshot.test.shtests/fm-brief.test.shtests/fm-groom.test.shtests/fm-herdr-session-cleanup-e2e.test.shtests/fm-herdr-session-cleanup.test.shtests/fm-idea-mine.test.shtests/fm-quota-array-dispatch-live-e2e.test.shtests/fm-review-decision.test.shtests/fm-review-page.test.shtests/fm-session-start.test.shtests/fm-spawn-account.test.shtests/fm-teardown.test.sh
💤 Files with no reviewable changes (1)
- tests/fm-bearings-snapshot.test.sh
Comment on lines
+114
to
+125
| user=$(printf 'Task title: %s\n\nTask description: %s\n\nAnswer now.' "$title" "$brief") | ||
| # standard tier (sonnet, 30s): the fast/haiku 15s ceiling is too tight for the | ||
| # safety reasoning, and a timeout would fail-safe to escalate -- costing the | ||
| # captain a needless review item. Classification reliability is worth one tier up. | ||
| out=$(fm_groom_infer standard "$system" "$user") || return 1 | ||
| # ASCII-only on purpose: the verdict vocabulary is "safe"/"escalate". | ||
| # shellcheck disable=SC2018,SC2019 | ||
| verdict=$(printf '%s\n' "$out" | sed -e '/./,$!d' | head -1 | tr 'A-Z' 'a-z' | tr -cd 'a-z') | ||
| case "$verdict" in | ||
| safe|escalate) : ;; | ||
| *) verdict=escalate ;; # fail-safe: anything unparseable escalates | ||
| esac |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Identify every writer into the ideas store to establish the trust boundary for groom classification.
set -u
rg -n -C4 'ideas (create|add|q|new)\b' --hidden -g '!.git' || true
fd -H -t f -e ts -e sh . bin | xargs rg -n -C3 'ideas\b' 2>/dev/null | head -100Repository: trillium/firstmate
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repository files matching fm-groom:\n'
git ls-files | rg '(^|/)fm-groom(-lib)?\.sh$|bin/' || true
printf '\nOutline bin/fm-groom-lib.sh:\n'
ast-grep outline bin/fm-groom-lib.sh 2>/dev/null || true
printf '\nRelevant bin/fm-groom-lib.sh lines 1-160:\n'
sed -n '1,160p' bin/fm-groom-lib.sh | nl -ba
printf '\nSearch for ideas/store/writer terms:\n'
rg -n -C3 'ideas|store\b|idea|brief|title|groom' --hidden -g '!node_modules' --glob '*.sh' --glob '*.ts' . | head -200Repository: trillium/firstmate
Length of output: 17192
🏁 Script executed:
#!/bin/bash
set -u
printf 'bin/fm-idea-mine.ts outline:\n'
ast-grep outline bin/fm-idea-mine.ts 2>/dev/null || true
printf '\nbin/fm-idea-mine.ts relevant commands:\n'
rg -n -C4 'command|create|mine|capture|title|brief|description|FM_IDEA|MINE' bin/fm-idea-mine.ts
printf '\nbin/fm-groom-lib.sh full relevant content:\n'
sed -n '1,220p' bin/fm-groom-lib.sh
printf '\nbin/fm-groom.sh relevant command handling and dispatch call:\n'
rg -n -C5 'groom-.*lib|groom_category|groom_id|action|safe|escalate|FM_GROOM|dispatch|spawn|review' bin/fm-groom.sh
printf '\nAll deterministic files containing ideas command name creation paths:\n'
python3 - <<'PY'
from pathlib import Path
roots = Path('.')
patterns=[]
for p in roots.rglob('**'):
if p.is_file() and not any(part in {'.git','node_modules'} for part in p.parts):
try:
txt=p.read_text(errors='ignore')
except Exception:
continue
if 'create' in txt and 'ideas' in txt:
patterns.append(str(p))
print('\n'.join(patterns))
PY
printf '\nSearch for mined chat/mine/capture flows:\n'
rg -n -i -C4 'mine|captur|scrape|mining|mined|chat.*capture|capture.*chat|prompt.*title|shorten|brief' bin tests README.md 2>/dev/null | head -250Repository: trillium/firstmate
Length of output: 39679
🏁 Script executed:
#!/bin/bash
set -u
printf 'bin/fm-idea-mine.ts lines 250-330 (sanitizeText and mineTranscript):\n'
sed -n '250,330p' bin/fm-idea-mine.ts
printf '\nbin/fm-idea-mine.ts lines 559-790 (candidate generation, evaluation, createIssue):\n'
sed -n '559,790p' bin/fm-idea-mine.ts
printf '\nbin/fm-idea-mine.ts lines 900-975 (main and direct run):\n'
sed -n '900,975p' bin/fm-idea-mine.ts
printf '\nFind all write paths into ideas data by command strings:\n'
python3 - <<'PY'
from pathlib import Path
terms = ["ideas create", 'ideas "create"', "ideas add", 'ideas "add"', "ideas new", "fm-idea-mine", "writeMind", "createMind", "set-id", "stateDir", "ID_STORE_PATH", "ID_DATA_PATH"]
for file in Path('.').rglob('*'):
if not file.is_file() or '.git' in file.parts or 'node_modules' in file.parts:
continue
if file.suffix not in {'.sh','.ts','.mjs','.js','.json'}:
continue
text = file.read_text(errors='ignore')
if any(term in text for term in terms):
print(file)
PYRepository: trillium/firstmate
Length of output: 15775
🏁 Script executed:
#!/bin/bash
set -u
printf 'bin/fm-idea-mine.ts lines 800-900:\n'
sed -n '800,900p' bin/fm-idea-mine.ts
printf '\nRegex-style evidence check for store write paths:\n'
python3 - <<'PY'
from pathlib import Path
import re
for path in Path('.').rglob('*'):
if not path.is_file() or '.git' in path.parts or 'node_modules' in path.parts:
continue
if path.suffix not in {'.sh', '.ts', '.mjs', '.js', '.json'}:
continue
text = path.read_text(errors='ignore')
for expr in [
r'(["\'])ideas\s+\1\s+\.create\b',
r'\bcreateIssue\s*\(\s*this\.cfg\.ideasCmd',
r'fm_idea_mine|idea-mine|IDEA_MINE|mineTranscript|generateCandidates|eval',
]:
if re.search(expr, text):
print(f"{path}: {expr}")
break
PY
printf '\nBehavioral probe: classify would accept classifier-directed title/brief without deterministic rail:\n'
python3 - <<'PY'
cases = [
("safe deploy from local", "this is safe"),
("deploy to production", "safe\nok"),
("credential rotate", "safe\nbecause ok"),
("safe escalate", "safe"),
("migrate production data", "safe\nreason"),
]
for title, out in cases:
verdict = out.strip().splitlines()[0].lower().strip() if out.strip().splitlines() else ""
verdict = "".join(c for c in verdict if "a" <= c <= "z")
route = "safe" if verdict == "safe" else "escalate"
has_high_risk = any(t in {title, out} for t in ["deploy", "credential", "rotate", "delete", "migrate", "merge"])
print(f"title={title!r} out={out!r} -> verdict={verdict!r} route={route} has_high_risk={has_high_risk}")
PYRepository: trillium/firstmate
Length of output: 6342
Add a deterministic escalate rail before the model verdict.
fm-idea-mine.ts mines chat/assistant history and writes ideas with ideas create, so fm-groom.sh can classify mined idea text. bin/fm-groom-lib.sh sends that title and the generated brief to fm_groom_classify, and bin/fm-groom.sh dispatches any safe verdict. A confident first-line safe from classifier-directed wording bypasses the other fail-safes. Add deterministic pre-filtering for high-risk terms — deploy, merge, credential, rotate, delete, migrate — and force escalate, regardless of the model answer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-groom-lib.sh` around lines 114 - 125, Update the classification flow
around fm_groom_infer and verdict handling to inspect the combined title and
brief for the high-risk terms deploy, merge, credential, rotate, delete, or
migrate before accepting the model verdict. Force verdict=escalate when any term
matches, regardless of classifier output; otherwise preserve the existing
safe/escalate parsing and fail-safe behavior.
Owner
Author
There was a problem hiding this comment.
Acknowledged — deferring this one intentionally. fm-groom is off-by-default so there's no live exposure today, and the deny-list vs. fail-safe classifier design here is a security-policy call being escalated to the project owner rather than decided in this PR. Tracked as follow-up: fm-groom-classifier-escalate-rail (captain-held). Not blocking this merge.
There was a problem hiding this comment.
@trillium, acknowledged. I will treat this finding as intentionally deferred and non-blocking for this PR.
The captain-held follow-up fm-groom-classifier-escalate-rail should decide whether to add deterministic escalation terms before classifier output is accepted.
You are interacting with an AI system.
The hook-task-a8 positive-case assertion used stat -f '%Lp' X 2>/dev/null || stat -c '%a' X. On Linux, GNU stat's -f flag means filesystem-status mode, not a format flag. It processes %Lp and X as two separate file targets, fails on the literal %Lp target (suppressed by 2>/dev/null), but still prints X's filesystem info to stdout, then exits nonzero. That nonzero exit triggers the || fallback too, so the captured perm becomes the concatenation of the filesystem-info blob and the correct "700" from the GNU form, never matching "700" alone. Every other stat-portability check in this codebase already branches on uname first; this one instance used the fragile blind-fallback idiom instead. Match the established convention.
trillium
changed the title
Six independent correctness/reliability bugs surfaced by CodeRabbit's review of PR #38, fixed to satisfy the CodeRabbit-clean merge gate: - .codex/hooks.json: guard the four bd codex-hook entries with `command -v bd` and a 10s timeout, matching the fail-open pattern every other hook in the file already follows. - fm-groom-json-field.sh: exit from the stdout.write completion callback instead of immediately after, since Bun's pipe writes are async and an immediate process.exit(0) can truncate the field value. - fm-groom.sh: lowercase idea ids before sanitizing to a task_id (was dropping uppercase, causing distinct ideas to collide on one task_id and overwrite each other's brief); refuse to proceed on an empty or already-used task_id. - fm-groom.sh: leave an idea unmarked for retry when `review q\| fails, instead of unconditionally writing the permanent groom=escalated label and losing the idea with no review item filed. - fm-herdr-spur.sh: track whether the reader actually emitted @subscribed and return a failure code when it did not, so a persistent subscribe failure engages the existing sleep/backoff fallback instead of busy-looping python3 respawns. - fm-idea-mine.ts: build the harvest review body before creating the review item instead of filing it with a literal "PLACEHOLDER" body and patching it after; a failed patch no longer leaves a permanent placeholder review item. Also pins FM_REVIEW_PAGE_NO_WIRE in tests/fm-review-page.test.sh's run_tool so an inherited env value can't silently flip wire-back behavior and make an assertion pass or fail for the wrong reason. Verified: tests/fm-review-page.test.sh, tests/fm-groom.test.sh, tests/fm-idea-mine.test.sh, tests/fm-turnend-guard.test.sh all pass; bin/fm-lint.sh clean. Two remaining CodeRabbit findings on this review pass are security-sensitive (bin/fm-groom-lib.sh:125 - deterministic escalate rail for high-risk classifier terms; bin/fm-spawn-hooks.d/beads.sh:49 - unquoted env-sourced id interpolation into a generated script) and are routed to firstmate for a decision rather than self-applied.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/fm-groom.sh`:
- Around line 283-286: The escalation flow around the IDEAS_BIN set-state call
must not report success when durable persistence fails. Verify that the state
update succeeds before setting action to “FILED”; if it fails after review
creation, persist $rid through a recoverable retry path and reconcile that
record before the next review q invocation, preventing duplicate review items.
- Around line 281-290: Guard every durable idea mutation in bin/fm-groom.sh,
including the armed-branch set-state and note calls, behind successful
bin/fm-lock.sh acquisition and fm_session_lock_owned_by_self verification for
the current home. Make the real-file action path fail read-only when ownership
is unavailable, while preserving non-mutating grooming behavior and existing
mutation flow after ownership is confirmed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5da87b7f-c150-459f-8fba-2218084f53e1
📒 Files selected for processing (9)
.codex/hooks.jsonREADME.mdbin/fm-groom-json-field.shbin/fm-groom.shbin/fm-herdr-spur.shbin/fm-idea-mine.tsdocs/configuration.mddocs/scripts.mdtests/fm-review-page.test.sh
💤 Files with no reviewable changes (1)
- docs/scripts.md
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- bin/fm-groom-json-field.sh
- docs/configuration.md
- .codex/hooks.json
- bin/fm-herdr-spur.sh
- bin/fm-idea-mine.ts
Comment on lines
+281
to
+290
| --body-file - 2>/dev/null) || rid= | ||
| if [ -n "$rid" ]; then | ||
| "$IDEAS_BIN" set-state "$id" "$GROOM_DIM=escalated" \ | ||
| --reason "fm-groom filed review item $rid" >/dev/null 2>&1 || true | ||
| "$IDEAS_BIN" note "$id" "fm-groom escalated to review item $rid: $rationale" >/dev/null 2>&1 || true | ||
| action="FILED review item $rid" | ||
| else | ||
| # Never mark an idea handled when no review item exists; the groom | ||
| # label is permanent and would drop the captain's decision silently. | ||
| action="REVIEW FILING FAILED; idea left unmarked for retry" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' bin/fm-groom.sh
rg -n -C 5 'lock|session|set-state|note' bin/fm-groom.shRepository: trillium/firstmate
Length of output: 13395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## fm-groom ideas mutation sites"
rg -n -C 3 '\$IDEAS_BIN\s*(set-state|note|label)' bin/fm-groom.sh
echo
echo "## lock-related symbols in bin/*"
rg -n -i '\b(session[_-]?lock|lock_file|lock\(|acquire[^A-Za-z]|per-home|HOME|FM_HOME)' bin || true
echo
echo "## candidate lock scripts"
fd -t f -i 'lock|session' bin || true
echo
echo "## line count and tail around groom loop"
wc -l bin/fm-groom.sh
sed -n '180,330p' bin/fm-groom.shRepository: trillium/firstmate
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -u
echo "## fm-groom ideas mutation sites"
rg -n -C 3 'IDEAS_BIN\s*(set-state|note|label)' bin/fm-groom.sh || true
echo
echo "## lock-related symbols in bin/*"
rg -n -i '\b(session[_-]?lock|lock_file|lock\(|acquire[^A-Za-z]|per-home|HOME|FM_HOME)' bin || true
echo
echo "## candidate lock scripts"
fd -t f -i 'lock|session' bin || true
echo
echo "## line count and tail around groom loop"
wc -l bin/fm-groom.sh
sed -n '180,330p' bin/fm-groom.shRepository: trillium/firstmate
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -u
echo "## fm-groom raw occurrences"
rg -n -C 3 'set-state|note|label' bin/fm-groom.sh || true
echo
echo "## fm-lock script outline and key ranges"
wc -l bin/fm-lock.sh bin/fm-session-lock-lib.sh
sed -n '1,220p' bin/fm-lock.sh
sed -n '1,170p' bin/fm-session-lock-lib.sh
echo
echo "## current fm-lock usage with context"
rg -n -C 4 'bin/fm-lock\.sh|fm-lock|session-lock|session_lock' bin/fm-groom.sh bin/fm-lock.sh bin/fm-session-lock-lib.sh bin/fm-*.sh | head -n 220Repository: trillium/firstmate
Length of output: 27749
Mutate ideas only after per-home session-lock ownership is verified.
bin/fm-groom.sh writes ideas set-state and ideas note directly in the armed branch at lines 262, 264, 283, and 285, but it does not use bin/fm-lock.sh or fm_session_lock_owned_by_self. Since idea state is durable shared state, make the real-file action path fail read-only unless bin/fm-lock.sh has acquired and verified the lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-groom.sh` around lines 281 - 290, Guard every durable idea mutation in
bin/fm-groom.sh, including the armed-branch set-state and note calls, behind
successful bin/fm-lock.sh acquisition and fm_session_lock_owned_by_self
verification for the current home. Make the real-file action path fail read-only
when ownership is unavailable, while preserving non-mutating grooming behavior
and existing mutation flow after ownership is confirmed.
Source: Coding guidelines
Owner
Author
There was a problem hiding this comment.
Acknowledged — deferring this one intentionally, not fixing in this PR. fm-groom is off-by-default so there's no live exposure today. Consolidated into the captain-held follow-up fm-groom-classifier-escalate-rail, which now tracks fm-groom's full pre-enable hardening design (lock-ownership verification for durable ideas-store mutations, alongside the deny-list/fail-safe classifier question). Not blocking this merge.
There was a problem hiding this comment.
@trillium, acknowledged. The finding remains deferred to fm-groom-classifier-escalate-rail.
fm-groom is off by default in this PR. The follow-up scope includes session-lock ownership verification for durable ideas-store mutations and the classifier escalation rail. This does not block this merge.
🐇
You are interacting with an AI system.
Comment on lines
+283
to
+286
| "$IDEAS_BIN" set-state "$id" "$GROOM_DIM=escalated" \ | ||
| --reason "fm-groom filed review item $rid" >/dev/null 2>&1 || true | ||
| "$IDEAS_BIN" note "$id" "fm-groom escalated to review item $rid: $rationale" >/dev/null 2>&1 || true | ||
| action="FILED review item $rid" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not report completion when the durable state update fails.
If Line 283 fails, the idea remains eligible for grooming, but Line 286 reports a completed escalation. A later run can file another review item because no durable record associates the idea with $rid.
Persist and verify the escalation state before reporting success. If persistence fails after review creation, record $rid in a recoverable retry path and reconcile it before another review q call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-groom.sh` around lines 283 - 286, The escalation flow around the
IDEAS_BIN set-state call must not report success when durable persistence fails.
Verify that the state update succeeds before setting action to “FILED”; if it
fails after review creation, persist $rid through a recoverable retry path and
reconcile that record before the next review q invocation, preventing duplicate
review items.
Owner
Author
There was a problem hiding this comment.
Acknowledged — deferring this one intentionally, not fixing in this PR. fm-groom is off-by-default so there's no live exposure today. Consolidated into the captain-held follow-up fm-groom-classifier-escalate-rail, which now tracks fm-groom's full pre-enable hardening design (durable escalation-state persistence/verification, alongside the deny-list/fail-safe classifier question). Not blocking this merge.
There was a problem hiding this comment.
@trillium, acknowledged. The finding remains valid and is deferred intentionally.
Complete fm-groom-classifier-escalate-rail before enabling FM_GROOM_ENABLED=1. The follow-up must prevent duplicate review creation after failed escalation-state persistence.
Would you like me to create a GitHub issue for the follow-up, or is it already tracked elsewhere?
✏️ Learnings added
Learnt from: trillium
Repo: trillium/firstmate PR: 38
File: bin/fm-groom.sh:283-286
Timestamp: 2026-08-02T22:45:47.774Z
Learning: In `bin/fm-groom.sh`, `fm-groom` remains off by default. Before any production use of `FM_GROOM_ENABLED=1`, the captain-held `fm-groom-classifier-escalate-rail` follow-up must implement and verify durable escalation-state persistence with retry reconciliation after review-item creation, and resolve the deny-list/fail-safe classifier policy.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
CodeRabbit flagged that the beads spawn hook interpolated BEADS_ID and TASK_ID into a generated check script and a task-show command with no charset validation and no quoting. The unquoted `task show $BEADS_ID` inside the heredoc let a crafted bead id inject extra arguments or shell metacharacters into the generated script, and neither id was restricted before being used to build the $STATE/$TASK_ID.check.sh path. This path is live, not theoretical: under config/backlog-backend=beads, fm_beads_resolve_or_create auto-resolves/mints a bead id that reaches this hook with no character validation, and the bare positional task id ($ID) reaches it the same way. Only the explicit --beads CLI-arg path validates its input today (fm-spawn.sh:288). Add the same [A-Za-z0-9._-] charset guard CodeRabbit's diff used, covering both ids before either is used to build a path or a command, and quote the interpolated id in the generated task-show call. The guard restricts TASK_ID before $STATE/$TASK_ID.check.sh is built, so that path can no longer contain `/` or `..` either. Verified: tests/fm-beads-hooks.test.sh (16/16), tests/fm-brief.test.sh, and tests/fm-spawn-beads.test.sh all pass with no regressions; bin/fm-lint.sh clean under pinned shellcheck 0.11.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Reconcile firstmate's diverged main branch (Option A: real merge, no reset/squash) preserving 22 local-only commits, 32 commits from a prior upstream-fork sync, and ~30 of origin's own merged PRs including a beads-authority migration; resolve conflicts by intent, keep both sides' functionality, verify no content silently dropped, full tests + lint green, open a PR against origin/main
What Changed
bin/:fm-groom(proactive ideas-store work generator),fm-review-page/fm-review-decision(render review-store items to visitable pages and route captain decisions),fm-idea-mine(mine chat history for uncaptured ideas),fm-isolated-launch(HOME-isolated claude sessions),fm-herdr-spur(herdr→firstmate completion bridge), and per-account Claude launchers with a newfm-spawn --accountflag.fm-spawn-hooks.d/beads.sh,fm-brief.sh --beads), fixes--beadsarg parsing and a double bead stamp infm-spawn.sh, and expandsfm-teardown.shbead-close handling.docs/scripts.md, README, AGENTS.md,herdr-navigationskill) and extensive new/updated test suites.Risk Assessment
Testing
Ran coverage guard and changed suites, all pass; one real-Herdr concurrency E2E flaky locally.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
⏭️ **Rebase** - skipped
.agents/skills/afk/SKILL.md- branch carries 54 commit(s) that exist on your local main branch but were never pushed to origin/main; rebasing would bundle this unrelated work (120 file(s)) into the PR:Push main to origin, or rebase your branch onto origin/main, before gating.
bin/fm-teardown.sh:1682- The merge commit (3121197) message states the branch was landed with 7 known-failing tests: 5 real-Herdr E2E flakiness, plus 2 asserted to be local-only tooling gaps (missing @earendil-works/pi-coding-agent npm package, and an installed tasks-axi 0.2.2 predating its public-followup subcommand). The claim that these will not manifest in CI is the author's assertion, not verified here. The pipeline's dedicated test step should confirm; if the CI environment has the same tasks-axi version or Herdr timing sensitivity, these may recur.✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh --check-coverage✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit