Add night-ops-directive and coderabbit-pr-gate skills

[trillium]

Clarifies the yolo reversibility test for routine gates (including PR merges), adds a fork-first-push rule for repos whose default branch isn't captain-writable, and registers two new agent-only skills:

• night-ops-directive — the standing autonomous-dispatch directive: delegator discipline during unattended work, firstmate's own wake cadence (the watcher heartbeat, since ScheduleWakeup/CronCreate are blocked for the primary session), human-only task triage via bd transfer, and fork-first pushes.
• coderabbit-pr-gate — reacting to CodeRabbit's actionable review comments beyond a bare pass/fail check, and treating its rate limits as a paused external wait rather than a failure.

Authored directly by firstmate per explicit captain override of the crew-live delegation coordination rule (this touches only firstmate's own shared tracked material, not a project).

Summary by CodeRabbit

• New Features
  • Added isolated and multi-account Claude Code launching with separate credentials and configuration.
  • Added Beads task tracking integration across session lifecycle, spawning, briefs, and completion monitoring.
  • Added interactive review pages with approve, decline, and comment workflows.
  • Added idea mining from recent transcripts and notes.
  • Added external agent completion notifications and review decision recording.
• Documentation
  • Documented isolated launches, account configuration, Beads workflows, and external agent monitoring.
• Tests
  • Added coverage for account isolation, task hooks, spawning options, and review decisions.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request adds Claude account isolation, Beads lifecycle integration, herdr monitoring, transcript idea mining, interactive review pages, review decision persistence, JSON grooming support, and related documentation and tests.

Changes

Operational policy and repository guidance

Layer / File(s)	Summary
Agent policy and repository guidance .agents/..., AGENTS.md, .gitignore	The repository documents delegation, CodeRabbit review handling, fork-based pull requests, reversible autonomous work, and new ignored paths.

Claude account isolation

Layer / File(s)	Summary
Isolated Claude bootstrap bin/claude-account.sh, bin/claude-1.sh, bin/claude-2.sh	Account launchers validate credentials, isolate account state, share non-sensitive configuration, initialize trust state, and execute Claude.
Claude launch wiring and documentation bin/fm-spawn.sh, README.md, docs/configuration.md, docs/scripts.md	The repository documents isolated and multi-account Claude launches and records account metadata during spawning.
Claude launcher validation tests/claude-account.test.sh, tests/fm-spawn-account.test.sh	Tests cover credential isolation, symlink setup, state initialization, settings behavior, argument validation, and launch construction.

Beads and herdr lifecycle integration

Layer / File(s)	Summary
Beads lifecycle configuration .codex/hooks.json, .claude/settings.json, AGENTS.md	Codex and Claude hooks invoke Beads context commands. Repository guidance defines Beads tracking and Codex setup.
Beads spawn stamping and checks bin/fm-brief.sh, bin/fm-spawn-hooks.d/beads.sh, docs/scripts.md, docs/examples/herdr-spur.agents	Brief and spawn flows validate Beads identifiers, execute hooks, stamp dispatched beads, and register close-status checks.
Herdr status monitoring bin/fm-herdr-spur.sh	The herdr spur supports configured watch sets, event subscriptions, polling fallback, transition debouncing, self-detach, and wake enqueueing.
Beads integration tests tests/fm-beads-hooks.test.sh	Tests cover Beads stamping, brief and spawn flags, hook registration, close detection, fail-open behavior, and syntax validation.

Transcript idea mining

Layer / File(s)	Summary
Idea mining pipeline bin/fm-idea-mine.ts	The Bun CLI mines bounded transcript data, generates and evaluates candidates, removes duplicates, ranks results, supports dry runs, and files ideas with idempotency markers.

Interactive review workflow

Layer / File(s)	Summary
Review page rendering bin/fm-review-page.ts	The CLI fetches review items, renders self-contained item and index pages, extracts context and artifacts, supports decisions, and wires page URLs back to review metadata.
Decision persistence bin/fm-review-decision.sh	The decision command validates verdicts, appends a wake, annotates the review item, and records a JSONL audit entry.
Review decision tests tests/fm-review-decision.test.sh	Hermetic tests cover verdict handling, comments, stdin input, validation errors, annotation failures, wake records, and audit records.

Shell utility portability

Layer / File(s)	Summary
Portable JSON helper bin/fm-groom-json-field.sh	The grooming helper extracts string fields from JSON objects or arrays and handles malformed input without shell quoting issues.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Captain
  participant ReviewPage
  participant PulseAPI
  participant ReviewCLI
  Captain->>ReviewPage: select verdict and optional comment
  ReviewPage->>PulseAPI: POST review decision
  PulseAPI->>ReviewCLI: annotate review item
  ReviewCLI-->>PulseAPI: decision result
  PulseAPI-->>ReviewPage: return decision status
  ReviewPage-->>Captain: display recorded decision

Loading

Suggested reviewers: kunchenguid

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 56.73% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the two primary skills added by the pull request.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fm/night-ops-directive-skill

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 19

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (12)

.gitignore-7-7 (1)

7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Scope the generated-file ignore rules to the repository root.

• .fm-isolated-home is only used by ISOLATED_HOME="${FM_ISOLATED_HOME:-$FM_ROOT/.fm-isolated-home}", so ignore it at the root with /.fm-isolated-home.
• CHANGELOG.md also matches untracked changelogs anywhere in the tree. Use /CHANGELOG.md if only the repository-root changelog is intended to be ignored.

🤖 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 @.gitignore at line 7, Update the .gitignore entries for .fm-isolated-home
and CHANGELOG.md to use leading slashes, restricting both generated-file ignore
rules to the repository root.

bin/fm-review-page.ts-824-835 (1)

824-835: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Forward the HTTP status so the failure message is not "request failed ()".

Line 832 concatenates an empty string where a status belongs, so a failure without a JSON error field shows request failed (). The root cause is line 825: the handler forwards only ok and data, so res.status is unavailable. Forward the status and use it.

🐛 Proposed fix

         }).then(function (res) {
-          return res.json().then(function (data) { return { ok: res.ok, data: data }; });
+          return res.json().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);
           }

🤖 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, Update the response mapping in
the promise chain before the failure handler to forward the HTTP status
alongside ok and data, then use that status in the fallback error message within
the r failure branch instead of concatenating an empty string.

bin/fm-review-decision.sh-96-114 (1)

96-114: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Escape the remaining control characters, or correct the comment.

json_escape covers ", \, \n, \r, and \t. Every other C0 control character passes through raw, which makes the emitted .decisions.jsonl line invalid JSON. The comment at lines 97-98 claims control-character coverage. A pasted captain comment can carry such a byte, and one bad line breaks any reader of the audit log.

🐛 Proposed fix

       $'\n') out+='\n' ;;
       $'\r') out+='\r' ;;
       $'\t') out+='\t' ;;
+      [$'\x01'-$'\x1f'])
+        out+="$(printf '\\u%04x' "'$ch'")" ;;
       *) out+="$ch" ;;

🤖 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 96 - 114, Update json_escape to
escape every remaining C0 control character required by JSON, not just quotes,
backslashes, newline, carriage return, and tab; use JSON-compatible \u00XX
escapes for control bytes below 0x20 while preserving existing handling. Ensure
the audit output remains valid JSONL and keep the comment accurate about the
implemented control-character coverage.

bin/fm-herdr-spur.sh-259-300 (1)

259-300: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Keep native push coverage up to date during FM_HERDR_SPUR_EVENT_BUDGET.

herdr-eventwait.py subscriptions are built from argv[3:], while the bash wrapper sends only the panes captured at the start of fm_herdr_spur_event_block. If a watched agent starts a new pane during the event-wait block, the current native push subscription can miss its transition until the block returns. Use a shorter default budget or periodically reconcile while the native wait is active.

🤖 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-herdr-spur.sh` around lines 259 - 300, Keep native push subscriptions
current during fm_herdr_spur_event_block instead of passing only the initial
pane_ids to herdr-eventwait.py for the entire event budget. Use a shorter
default FM_HERDR_SPUR_EVENT_BUDGET or add periodic snapshot reconciliation so
panes created for watched agents during the wait are subscribed before their
transitions are missed.

.codex/hooks.json-12-21 (1)

12-21: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Add an existence guard and explicit timeout for the new bd codex-hook hooks.

The new SessionStart, PreCompact, PostCompact, and UserPromptSubmit hooks call bd codex-hook <Name> directly. If bd is unavailable, these hooks fail before the hooks context succeeds, unlike the other command hooks that guard before running. Add command -v bd >/dev/null 2>&1 || exit 0 to each new hook and add an explicit "timeout"; keep UserPromptSubmit short because it runs on every prompt.

🤖 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 @.codex/hooks.json around lines 12 - 21, Update each new bd codex-hook entry
for SessionStart, PreCompact, PostCompact, and UserPromptSubmit to first guard
with command -v bd >/dev/null 2>&1 || exit 0, then invoke the hook. Add an
explicit timeout to every entry, using a shorter timeout for UserPromptSubmit
than the lifecycle hooks.

bin/fm-idea-mine.ts-747-758 (1)

747-758: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

createIssue id extraction is fragile.

The function returns the last whitespace-separated token of stdout. If the CLI does not support --silent, or prints a trailing period or a hint line, the returned value is an arbitrary word rather than an id. fileHarvest then records that value as the filed idea id, and main passes it to review update. The function also returns "" for empty stdout without raising an error, so the caller cannot tell success from silent failure.

Validate the extracted id against the expected id shape and raise on a mismatch.

🛡️ Proposed id validation

-  return (res.stdout || "").trim().split(/\s+/).pop() || "";
+  const id = (res.stdout || "").trim().split(/\s+/).pop() || "";
+  if (!/^[A-Za-z0-9][\w.-]*$/.test(id)) {
+    throw new Error(`${cmd} create printed no usable id: ${(res.stdout || "").slice(0, 200)}`);
+  }
+  return id;

🤖 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-idea-mine.ts` around lines 747 - 758, Update createIssue to validate
the extracted stdout value against the expected issue-id format before returning
it. Raise an error when stdout is empty or the extracted value does not match
that format, rather than returning an arbitrary token or an empty string;
preserve the existing command failure handling.

bin/fm-idea-mine.ts-331-336 (1)

331-336: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

The token budget is not enforced when one message exceeds it.

The loop stops at tail.length > 1. If the last message alone exceeds cfg.tokenBudget, the code sends it in full. A single large pasted block or file dump then breaks the documented read-side bound and can overflow the inference context, which makes generateCandidates fail and returns exit 1.

Truncate the surviving message to the budget.

🛡️ Proposed truncation of the last surviving message

   while (tail.length > 1 && approxTokens(charCount) > cfg.tokenBudget) {
     const dropped = tail.shift();
     if (dropped) charCount -= dropped.text.length;
   }
+  // A single oversized message still has to respect the budget.
+  if (tail.length === 1 && approxTokens(charCount) > cfg.tokenBudget) {
+    const maxChars = cfg.tokenBudget * 4;
+    tail = [{ role: tail[0].role, text: `${tail[0].text.slice(-maxChars)}` }];
+    charCount = tail[0].text.length;
+  }

🤖 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-idea-mine.ts` around lines 331 - 336, Update the tail-trimming logic
around tail, charCount, and approxTokens so the token budget is enforced even
when one message remains and exceeds cfg.tokenBudget. After dropping older
messages, truncate the surviving message’s text to fit the remaining budget
before generateCandidates consumes it, preserving the existing behavior when the
message is already within budget.

tests/fm-idea-mine.test.sh-184-185 (1)

184-185: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Cover every store write in negative-path tests.

The fake store logs create, update, and note, but these checks do not reject all three verbs. A dry run could emit note and pass. Malformed or empty history could emit update or note and pass. Use one assertion for \[(create|update|note)\] in all three cases.

Proposed test fix

-  if grep -qE '\[create\]|\[update\]' "$log"; then
+  if grep -qE '\[(create|update|note)\]' "$log"; then

-  if grep -qE '\[create\]' "$log"; then
+  if grep -qE '\[(create|update|note)\]' "$log"; then

Also applies to: 212-213, 230-231

🤖 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-idea-mine.test.sh` around lines 184 - 185, Update the dry-run,
malformed-history, and empty-history assertions in the test to reject every
fake-store write verb by matching create, update, or note with a single combined
grep pattern. Apply the same \[(create|update|note)\] check to all three cases,
preserving their existing failure messages and log output.

.agents/skills/herdr-navigation/SKILL.md-34-35 (1)

34-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the split example match its description.

The comment says that a specific command runs in the new pane, but the command only supplies --cwd. Change the comment to describe the current-directory split, or add the supported command argument.

🤖 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/skills/herdr-navigation/SKILL.md around lines 34 - 35, Update the
comment above the herdr pane split invocation to accurately describe the
current-directory split, or add a supported command argument so the new pane
runs a specific command. Keep the example and its description consistent.

docs/configuration.md-175-179 (1)

175-179: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the isolated-launch documentation to match the implementation.

The text says that first launch always requires a new login and that no authentication state is copied. bin/fm-isolated-launch.sh now attempts to seed .credentials.json from the macOS Keychain and falls back to login only when extraction fails.

Document the Keychain-seeding behavior. Also document the detached FM_ISOLATED_CWD worktree and its requirement to remain outside the real home tree.

🤖 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 175 - 179, Update the isolated-launch
documentation around bin/fm-isolated-launch.sh to describe its macOS
Keychain-based .credentials.json seeding and clarify that login is only required
when extraction fails. Also document the detached FM_ISOLATED_CWD worktree and
state that it must remain outside the real home directory tree.

bin/fm-groom.sh-124-132 (1)

124-132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Emit valid JSON for every --json record.

fm_groom_json_escape escapes backslashes, quotes, newlines, and tabs. It does not escape carriage returns, backspace, form feed, or other JSON control characters. A title or model response containing these characters can produce invalid JSON.

Use a JSON encoder or escape the complete control-character range.

🤖 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 124 - 132, Update fm_groom_json_escape, used by
emit_json, to produce valid JSON for every input by escaping the complete JSON
control-character range, including carriage returns, backspace, and form feed,
while preserving the existing handling of backslashes, quotes, newlines, and
tabs. Prefer an available JSON encoder if the script already supports one;
otherwise extend the escaping logic without changing emit_json’s field
structure.

bin/fm-brief.sh-92-95 (1)

92-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject a missing or option-like --beads value.

When --beads is last, ${1-} becomes empty and the command silently creates a brief without the Beads link. When another option follows, that option can be consumed as the bead ID.

Require a value before shifting. Reject values beginning with --.

🤖 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 92 - 95, Update the --beads handling branch to
validate that a following argument exists and is not option-like before
consuming it. Reject missing or --prefixed values with the script’s established
argument-error behavior, and only then shift and assign BEADS_ID.

🧹 Nitpick comments (13)

.agents/skills/coderabbit-pr-gate/SKILL.md (1)

46-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Define the CodeRabbit stall bound.

“Reasonable number of backoff cycles” is not measurable. Different workers can wait or escalate at different times. Specify a maximum elapsed time or watcher-cycle count, and define the action after that bound.

Based on the PR objective, rate limits must remain paused external waits without creating an indefinite wait.

🤖 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/skills/coderabbit-pr-gate/SKILL.md around lines 46 - 48, Update the
CodeRabbit review-waiting guidance near the stalled-review rule to define a
measurable maximum watcher-cycle count or elapsed-time bound, accounting for
paused external waits caused by rate limits. After that bound is reached without
a review, stop waiting and report the stalled review to the captain as evidence
rather than blocking indefinitely or merging silently.

bin/fm-review-page.ts (4)

1026-1043: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document exit code 3 in HELP.

main returns 3 when a wire-back fails but pages were written (line 1108). The header block documents the cron use at lines 54-57, so a scheduler author needs to know that 3 means partial success rather than a hard failure. Add an EXIT section to HELP.

🤖 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 1026 - 1043, Update the HELP text in the
`HELP` constant to add an EXIT section documenting that exit code 3 means pages
were written but the store wire-back failed, indicating partial success rather
than a complete failure.

---

851-878: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Render Stakes once per page.

extractStakes (line 853) and extractContextFields (line 855) both match a ## Stakes section. For a body that carries one, the page shows the same prose twice: in the amber callout at line 921 and again as the Stakes field inside the context block. Suppress the callout when the context block already carries a Stakes field.

♻️ Proposed change

-  const stakes = extractStakes(item.description)
   const artifacts = extractArtifacts(item.description)
   const contextFields = extractContextFields(item.description)
+  const hasStakesField = contextFields.some((f) => f.label === "Stakes")
+  const stakes = hasStakesField ? "" : extractStakes(item.description)

🤖 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 851 - 878, Update the rendering flow
around extractStakes and extractContextFields so the amber callout is omitted
when contextFields already contains a Stakes field, while preserving the callout
for stakes content not represented in the context block.

---

1009-1022: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Validate the item id before it becomes a path segment and a URL segment.

item.id comes straight from the store payload into join(OUT_ROOT, item.id) and into ${BASE_URL}/${item.id}/. An id containing / or .. writes outside the review root and yields a wrong public URL. A single shape check keeps the output root closed and costs nothing.

🛡️ Proposed guard

+/** Store ids are slug-shaped; reject anything that could escape OUT_ROOT. */
+const ID_RE = /^[A-Za-z0-9._-]+$/
+
 /** Write one item's page and return its public URL. Idempotent (overwrite). */
 function writeItemPage(item: ReviewItem): string {
+  if (!ID_RE.test(item.id)) {
+    throw new Error(`refusing to write page for unsafe item id: ${item.id}`)
+  }
   const dir = join(OUT_ROOT, item.id)

🤖 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 1009 - 1022, Validate item.id in
writeItemPage before using it as a filesystem or URL segment, accepting only the
expected single-segment identifier shape and rejecting values containing path
separators or traversal components. Preserve the existing page-writing behavior
for valid IDs and fail before mkdirSync or writeFileSync for invalid ones.

---

558-585: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename the local esc to remove the shadow of the HTML-escape helper.

Line 561 shadows the module-level esc HTML escaper for the whole for (const key of keys) block. Any future call to the HTML escaper inside that block hits a temporal dead zone or silently uses the regex-escaped key. Rename it to make the two roles distinct.

♻️ Proposed rename

     for (const key of keys) {
-      const esc = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
+      const keyRe = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
       // (1) Heading form: `## What\n...` up to the next heading, a blank-line
       // paragraph break, or end of body. Stopping at a blank line keeps the last
       // heading section from swallowing a following inline-label line (e.g. a
       // trailing `ARTIFACT: ...`).
       const headRe = new RegExp(
-        `(?:^|\\n)#{1,6}\\s*${esc}\\b[:\\s]*\\n([\\s\\S]*?)(?=\\n#{1,6}\\s|\\n\\s*\\n|$)`,
+        `(?:^|\\n)#{1,6}\\s*${keyRe}\\b[:\\s]*\\n([\\s\\S]*?)(?=\\n#{1,6}\\s|\\n\\s*\\n|$)`,
         "i",
       )
@@
-      const inlineRe = new RegExp(`(?:^|\\n)\\s*${esc}\\s*:\\s*([^\\n]+)`, "i")
+      const inlineRe = new RegExp(`(?:^|\\n)\\s*${keyRe}\\s*:\\s*([^\\n]+)`, "i")

🤖 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 558 - 585, Rename the local regex-escaped
key variable `esc` inside the `for (const key of keys)` loop to a distinct name,
and update both `headRe` and `inlineRe` template expressions to use it,
preserving the module-level HTML escape helper name.

tests/fm-review-decision.test.sh (3)

68-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Drop the dead env prefix on printf.

Lines 70-72 export three variables to printf, which does not read them. Only the right side of the pipe needs them. Removing the prefix makes the stdin branch match the non-stdin branch.

♻️ Proposed cleanup

     OUTPUT=$(
-      FM_HOME="$home" \
-      FM_REVIEW_BIN="$fakebin/review" \
-      FM_REVIEW_DECISIONS_LOG="$home/decisions.jsonl" \
-        printf '%s' "$stdin_data" | \
+      printf '%s' "$stdin_data" | \
       FM_HOME="$home" \
       FM_REVIEW_BIN="$fakebin/review" \
       FM_REVIEW_DECISIONS_LOG="$home/decisions.jsonl" \
         bash "$TOOL" "${args[@]}" 2>&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 `@tests/fm-review-decision.test.sh` around lines 68 - 79, Remove the FM_HOME,
FM_REVIEW_BIN, and FM_REVIEW_DECISIONS_LOG environment-variable prefix from the
printf command in the stdin branch; retain those variables on the bash "$TOOL"
pipeline command so behavior matches the non-stdin path.

---

222-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for the best-effort audit-log failure.

The script treats an audit-log append failure as warn-only and still exits 0 (lines 158-160). No case covers it. Point FM_REVIEW_DECISIONS_LOG at an unwritable path, then assert exit 0, the WARNING: line, and that the wake and note still landed. That pins the asymmetry between the load-bearing steps and the best-effort step.

🤖 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-review-decision.test.sh` around lines 222 - 244, Extend the test
suite with a best-effort audit-log failure case, using the existing run_decision
and assertion helpers. Set FM_REVIEW_DECISIONS_LOG to an unwritable path, then
verify exit code 0, a WARNING: message, and successful wake and note effects;
register the new test with the other test calls.

---

191-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the wake-queue state after the annotation failure.

Case (f) checks the exit code and the diagnostic, but not the queue. That is why the ordering behavior in bin/fm-review-decision.sh (lines 118-145) is invisible today: the wake is already enqueued when review note fails. Add the assertion so the intended contract is pinned once the two steps are reordered.

💚 Proposed assertion

   run_decision "$home" "$fakebin" "$log" review-znt approve
   [ "$RC" -ne 0 ] || fail "note-failure: expected non-zero exit on store failure, got 0"
   assert_contains "$OUTPUT" "failed to annotate review item review-znt" "note-failure: loud diagnostic"
+  # A reported failure must not leave firstmate an actionable wake for a
+  # decision the captain was told did not record.
+  assert_absent "$(wake_queue "$home")" "note-failure: no wake left behind"

🤖 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-review-decision.test.sh` around lines 191 - 202, Update
t_note_failure in the test to assert the wake-queue state after run_decision
reports the annotation failure. Verify that no wake entry was enqueued when
storing the review note fails, while preserving the existing non-zero exit and
diagnostic assertions.

bin/fm-idea-mine.ts (4)

99-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive the default transcript directory instead of hardcoding one machine's path.

defaultTranscriptDir embeds -Users-trilliumsmith-code-firstmate. On any other machine or checkout path the default resolves to a directory that does not exist. mineTranscript then returns no messages and the run exits with a benign no-op, so the failure is silent.

Derive the encoded project directory from the repository root, and keep FM_IDEA_MINE_TRANSCRIPT_DIR as the override.

♻️ Proposed derivation from the script location

 function defaultTranscriptDir(): string {
-  return join(homedir(), ".claude", "projects", "-Users-trilliumsmith-code-firstmate");
+  // Claude encodes a project path by replacing every "/" with "-".
+  const repoRoot = resolve(fileURLToPath(import.meta.url), "..", "..");
+  const encoded = repoRoot.replace(/\//g, "-");
+  return join(homedir(), ".claude", "projects", encoded);
 }

🤖 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-idea-mine.ts` around lines 99 - 101, Update defaultTranscriptDir to
derive the encoded project directory from the repository root based on the
script location instead of hardcoding a user-specific path, while preserving the
FM_IDEA_MINE_TRANSCRIPT_DIR override used by mineTranscript.

---

470-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The hardcoded 180000 ms outer timeout overrides FM_IDEA_MINE_INFER_TIMEOUT_MS.

FM_IDEA_MINE_INFER_TIMEOUT_MS is documented as an operator control, but spawnSync always uses 180000 ms. If an operator raises the inner timeout above that value, spawnSync kills the child first and the run reports a generic inference failure. Derive the outer bound from the configured value plus a margin.

♻️ Proposed derivation of the outer timeout

 interface Config {
   inferCmd: string[];
+  inferTimeoutMs: number;

-  const res = spawnSync(cmd, [...base, system, user], {
-    encoding: "utf8",
-    timeout: 180000,
-    maxBuffer: 32 * 1024 * 1024,
-  });
+  const res = spawnSync(cmd, [...base, system, user], {
+    encoding: "utf8",
+    // Outer wall-clock bound must exceed the inner inference timeout.
+    timeout: cfg.inferTimeoutMs + 30000,
+    maxBuffer: 32 * 1024 * 1024,
+  });

Set inferTimeoutMs in loadConfig from envInt("FM_IDEA_MINE_INFER_TIMEOUT_MS", 150000) and reuse it in defaultInferCmd.

🤖 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-idea-mine.ts` around lines 470 - 491, Update the configuration flow so
loadConfig derives and stores inferTimeoutMs from FM_IDEA_MINE_INFER_TIMEOUT_MS
with a 150000 ms default, and ensure defaultInferCmd reuses that value. In
runInference, replace the hardcoded spawnSync timeout with the configured
inferTimeoutMs plus an appropriate margin, preserving the existing inference
error handling.

---

181-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the new FM_IDEA_MINE_* env schema in docs/configuration.md.

loadConfig introduces eleven new environment variables. The file header is the only place that describes them. The coding guidelines make docs/configuration.md the single owner of configuration schemas, so add the FM_IDEA_MINE_* entries and the marker file location under <fm-home>/state there.

As per coding guidelines: "Keep docs/configuration.md as the single owner of top-level operational-home layout and configuration schemas".

🤖 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-idea-mine.ts` around lines 181 - 198, Update docs/configuration.md,
the designated owner of configuration schemas, to document all eleven
FM_IDEA_MINE_* environment variables introduced by loadConfig, including their
defaults or behavior, and document the marker file location under
<fm-home>/state. Keep the schema and operational-home layout documentation
centralized there.

Source: Coding guidelines

---

948-950: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove or correct the stale bd comment.

This comment claims bd exposes update -d, but the file never calls bd. The update at line 923 targets cfg.reviewCmd, which defaults to review. Reword the note to name the review CLI, or delete it.

🤖 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-idea-mine.ts` around lines 948 - 950, Correct the stale comment near
the cfg.reviewCmd update to reference the review CLI rather than bd, reflecting
that the command defaults to review and no bd invocation exists; alternatively,
remove the comment.

tests/fm-groom.test.sh (1)

193-210: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

Add failure-path coverage for the grooming state machine.

These tests cover only successful spawn and successful review filing. Add mock failures for ideas label list, ideas set-state, fm-spawn, and review q. Assert that failed actions remain retryable and cannot create duplicate work.

🤖 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-groom.test.sh` around lines 193 - 210, Extend the grooming
state-machine tests around the existing armed safe and unsafe cases to mock
failures from ideas label list, ideas set-state, fm-spawn, and review q. For
each failed action, assert the command reports failure without applying the
completed-state label, preserves retryability, and does not create duplicate
spawn or review work on retry.

🤖 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 @.agents/skills/coderabbit-pr-gate/SKILL.md:
- Around line 33-34: Revise the unresolved actionable CodeRabbit finding rule so
the `yolo` exception applies only to a standing `yolo` posture for routine,
reversible merges within accepted task criteria. Preserve the requirement for
captain escalation on security-sensitive actions, preventing this exception from
bypassing that authority boundary.

In @.agents/skills/night-ops-directive/SKILL.md:
- Around line 22-26: Update the direct-edit exception in the Firstmate
delegation policy to require an empty fleet and the firstmate-coding-guidelines
skill for ship tasks before editing shared tracked material. Preserve delegation
as the default, including for policy, skill, hook, and launcher files, and allow
direct edits only when these conditions are satisfied and the change is bounded
and low risk.

In `@AGENTS.md`:
- Around line 508-586: Replace both duplicated Beads sections in AGENTS.md with
one concise, scoped pointer stating that applicable project work uses Beads and
directing agents to the Beads skill or authoritative documentation for workflow
details. Remove the duplicated commands, lifecycle protocol, architecture
explanation, and profile policy so AGENTS.md is not a second owner of Beads
guidance and only one Beads heading remains.

In `@bin/claude-account.sh`:
- Around line 102-133: The settings update in bin/claude-account.sh, around the
SETTINGS handling block, must run only when the launch arguments include
--dangerously-skip-permissions; leave settings unchanged for normal launches
while preserving the existing symlink protection. Update
tests/claude-account.test.sh around the existing launcher coverage to assert a
non-flagged launch preserves the setting and add a separate
--dangerously-skip-permissions case verifying the setting is applied.
- Around line 62-86: Update the Python trust-handling block in claude-account.sh
to preserve onboarding pre-acceptance while only creating
projects.<path>.hasTrustDialogAccepted when CLAUDE_TRUST_DIR is explicitly
provided. Avoid using the default current working directory for trust
initialization, and add coverage for launching without CLAUDE_TRUST_DIR that
verifies no project trust entry is written.

In `@bin/fm-bead-stamp.sh`:
- Around line 34-42: Update the stamping flow around task set-state and task
assign so claiming the bead and marking dispatch=sent are handled as one atomic
operation, or persist a durable pending/error state that retries reconcile
before reporting success. Do not leave either metadata update partially applied
while exiting 0; ensure the script’s final status distinguishes a completed
stamp from an incomplete update.

In `@bin/fm-groom-json-field.sh`:
- Around line 24-27: Remove the unconditional process.exit(0) after the field
write in the JSON field extraction flow. Let the process exit naturally after
process.stdout.write completes, while preserving the existing object/key
validation and output behavior.

In `@bin/fm-groom.sh`:
- Around line 240-246: Update task ID generation near task_id in the enabled
dispatch path to preserve uniqueness for distinct idea IDs; do not rely solely
on deleting non-alphanumeric characters. Use a collision-free encoding or append
a deterministic hash derived from the complete id, while retaining the groom-
prefix and ensuring generated paths remain safe.
- Around line 198-203: Update the idea-record loading flow around IDEAS_BIN show
so failed or malformed responses stop processing instead of falling back to the
ID. Require a successful, valid JSON record, and extract both title and
description through fm-groom-json-field.sh using the appropriate FM_KEY values;
remove the regex-based title parsing and ID-based fallback values before
formulation or dispatch continues.
- Around line 115-119: Make the groom state machine fail closed and recoverable:
update fm_groom_already_done so label-list failures are treated as an
indeterminate/error state, not as “not processed.” In the dispatch, filing, and
review handlers, use explicit transitional and failure states; write terminal
labels only after the external action succeeds and its durable state update is
confirmed. Ensure failed state writes prevent further actions, failed spawns
remain retryable rather than terminal, and failed reviews cannot produce
groom:escalated without a valid review result.
- Around line 151-159: Update the ready-idea query in bin/fm-groom.sh to capture
the exit status of "$IDEAS_BIN" ready --json instead of masking failures with
the [] fallback. When the command fails, exit non-zero and preserve the
documented unreadable-store failure behavior; only parse IDs and handle an empty
backlog after a successful query.

In `@bin/fm-herdr-spur.sh`:
- Around line 336-350: Update the main loop around fm_herdr_spur_event_block to
measure each block invocation’s elapsed time and treat a successful return below
the normal budget as an immediate failure. For either a non-zero status or a
fast successful return, run fm_herdr_spur_reconcile "$watch_set" and sleep for
FM_HERDR_SPUR_POLL_INTERVAL before retrying; preserve normal blocking behavior
without the fallback delay when the full budget elapses.

In `@bin/fm-idea-mine.ts`:
- Around line 813-816: Update fileHarvest to compose the review body with
reviewDescription(result, filed.filedIdeas, mined) after filing ideas, then
create the review item once with that body instead of PLACEHOLDER. Pass mined at
the fileHarvest call site, and remove the later review update block in main so
the marker is written only after creation succeeds.
- Around line 366-390: The listExistingIdeas flow must distinguish a failed
ideas read from a successfully empty store, so fileHarvest skips filing or
aborts when the command is missing, exits non-zero, or returns invalid JSON.
Update the CLI invocation to match repository usage by using the supported ready
--json, show --json, and set-state operations instead of list --all --json,
while preserving normal filing only when existing-idea data was read
successfully.

In `@bin/fm-isolated-launch.sh`:
- Around line 115-123: Update the credential extraction block around the
security command to set umask 077, create a private unpredictable temporary file
with mktemp, and write credentials to that file instead of the predictable .tmp
path. Require both chmod and mv to succeed before completing the atomic rename,
and clean up the temporary file on any failure.
- Around line 73-74: Update the path override handling near ISOLATED_HOME and
ISOLATED_CWD to resolve both configured paths before setup and reject overrides
that equal or reside within REAL_HOME, including FM_ISOLATED_HOME set to
REAL_HOME. Preserve the existing defaults when overrides are absent, and fail
immediately with a clear error for unsafe values before creating or using the
isolated environment.
- Around line 106-108: Update the isolated-home data setup around the data
symlink creation so it does not expose the entire REAL_HOME/data tree. Link only
the required store directories using explicit per-store paths, and validate that
any existing isolated data path is an approved directory or symlink target
before using it; preserve the existing setup behavior for valid required stores.

In `@bin/fm-review-decision.sh`:
- Around line 118-145: Move the STORE ANNOTATION block using NOTE, NOTE_ERR, and
the review note command before the DURABLE WAKE block invoking fm_wake_append,
so annotation succeeds before any wake is queued. Preserve the existing failure
handling and payload construction, then update the ordering comments near the
script’s section descriptions to state that annotation is the first durable
write and wake enqueue follows it.

In `@bin/fm-review-page.ts`:
- Around line 142-158: Update runReview to treat any terminated child as a
failure by checking res.signal first, then rejecting cases where res.status is
not a number, before retaining the existing non-zero status check. Include the
review command and available termination details in the thrown errors so
fetchAllOpen cannot process incomplete output.

---

Minor comments:
In @.agents/skills/herdr-navigation/SKILL.md:
- Around line 34-35: Update the comment above the herdr pane split invocation to
accurately describe the current-directory split, or add a supported command
argument so the new pane runs a specific command. Keep the example and its
description consistent.

In @.codex/hooks.json:
- Around line 12-21: Update each new bd codex-hook entry for SessionStart,
PreCompact, PostCompact, and UserPromptSubmit to first guard with command -v bd
>/dev/null 2>&1 || exit 0, then invoke the hook. Add an explicit timeout to
every entry, using a shorter timeout for UserPromptSubmit than the lifecycle
hooks.

In @.gitignore:
- Line 7: Update the .gitignore entries for .fm-isolated-home and CHANGELOG.md
to use leading slashes, restricting both generated-file ignore rules to the
repository root.

In `@bin/fm-brief.sh`:
- Around line 92-95: Update the --beads handling branch to validate that a
following argument exists and is not option-like before consuming it. Reject
missing or --prefixed values with the script’s established argument-error
behavior, and only then shift and assign BEADS_ID.

In `@bin/fm-groom.sh`:
- Around line 124-132: Update fm_groom_json_escape, used by emit_json, to
produce valid JSON for every input by escaping the complete JSON
control-character range, including carriage returns, backspace, and form feed,
while preserving the existing handling of backslashes, quotes, newlines, and
tabs. Prefer an available JSON encoder if the script already supports one;
otherwise extend the escaping logic without changing emit_json’s field
structure.

In `@bin/fm-herdr-spur.sh`:
- Around line 259-300: Keep native push subscriptions current during
fm_herdr_spur_event_block instead of passing only the initial pane_ids to
herdr-eventwait.py for the entire event budget. Use a shorter default
FM_HERDR_SPUR_EVENT_BUDGET or add periodic snapshot reconciliation so panes
created for watched agents during the wait are subscribed before their
transitions are missed.

In `@bin/fm-idea-mine.ts`:
- Around line 747-758: Update createIssue to validate the extracted stdout value
against the expected issue-id format before returning it. Raise an error when
stdout is empty or the extracted value does not match that format, rather than
returning an arbitrary token or an empty string; preserve the existing command
failure handling.
- Around line 331-336: Update the tail-trimming logic around tail, charCount,
and approxTokens so the token budget is enforced even when one message remains
and exceeds cfg.tokenBudget. After dropping older messages, truncate the
surviving message’s text to fit the remaining budget before generateCandidates
consumes it, preserving the existing behavior when the message is already within
budget.

In `@bin/fm-review-decision.sh`:
- Around line 96-114: Update json_escape to escape every remaining C0 control
character required by JSON, not just quotes, backslashes, newline, carriage
return, and tab; use JSON-compatible \u00XX escapes for control bytes below 0x20
while preserving existing handling. Ensure the audit output remains valid JSONL
and keep the comment accurate about the implemented control-character coverage.

In `@bin/fm-review-page.ts`:
- Around line 824-835: Update the response mapping in the promise chain before
the failure handler to forward the HTTP status alongside ok and data, then use
that status in the fallback error message within the r failure branch instead of
concatenating an empty string.

In `@docs/configuration.md`:
- Around line 175-179: Update the isolated-launch documentation around
bin/fm-isolated-launch.sh to describe its macOS Keychain-based .credentials.json
seeding and clarify that login is only required when extraction fails. Also
document the detached FM_ISOLATED_CWD worktree and state that it must remain
outside the real home directory tree.

In `@tests/fm-idea-mine.test.sh`:
- Around line 184-185: Update the dry-run, malformed-history, and empty-history
assertions in the test to reject every fake-store write verb by matching create,
update, or note with a single combined grep pattern. Apply the same
\[(create|update|note)\] check to all three cases, preserving their existing
failure messages and log output.

---

Nitpick comments:
In @.agents/skills/coderabbit-pr-gate/SKILL.md:
- Around line 46-48: Update the CodeRabbit review-waiting guidance near the
stalled-review rule to define a measurable maximum watcher-cycle count or
elapsed-time bound, accounting for paused external waits caused by rate limits.
After that bound is reached without a review, stop waiting and report the
stalled review to the captain as evidence rather than blocking indefinitely or
merging silently.

In `@bin/fm-idea-mine.ts`:
- Around line 99-101: Update defaultTranscriptDir to derive the encoded project
directory from the repository root based on the script location instead of
hardcoding a user-specific path, while preserving the
FM_IDEA_MINE_TRANSCRIPT_DIR override used by mineTranscript.
- Around line 470-491: Update the configuration flow so loadConfig derives and
stores inferTimeoutMs from FM_IDEA_MINE_INFER_TIMEOUT_MS with a 150000 ms
default, and ensure defaultInferCmd reuses that value. In runInference, replace
the hardcoded spawnSync timeout with the configured inferTimeoutMs plus an
appropriate margin, preserving the existing inference error handling.
- Around line 181-198: Update docs/configuration.md, the designated owner of
configuration schemas, to document all eleven FM_IDEA_MINE_* environment
variables introduced by loadConfig, including their defaults or behavior, and
document the marker file location under <fm-home>/state. Keep the schema and
operational-home layout documentation centralized there.
- Around line 948-950: Correct the stale comment near the cfg.reviewCmd update
to reference the review CLI rather than bd, reflecting that the command defaults
to review and no bd invocation exists; alternatively, remove the comment.

In `@bin/fm-review-page.ts`:
- Around line 1026-1043: Update the HELP text in the `HELP` constant to add an
EXIT section documenting that exit code 3 means pages were written but the store
wire-back failed, indicating partial success rather than a complete failure.
- Around line 851-878: Update the rendering flow around extractStakes and
extractContextFields so the amber callout is omitted when contextFields already
contains a Stakes field, while preserving the callout for stakes content not
represented in the context block.
- Around line 1009-1022: Validate item.id in writeItemPage before using it as a
filesystem or URL segment, accepting only the expected single-segment identifier
shape and rejecting values containing path separators or traversal components.
Preserve the existing page-writing behavior for valid IDs and fail before
mkdirSync or writeFileSync for invalid ones.
- Around line 558-585: Rename the local regex-escaped key variable `esc` inside
the `for (const key of keys)` loop to a distinct name, and update both `headRe`
and `inlineRe` template expressions to use it, preserving the module-level HTML
escape helper name.

In `@tests/fm-groom.test.sh`:
- Around line 193-210: Extend the grooming state-machine tests around the
existing armed safe and unsafe cases to mock failures from ideas label list,
ideas set-state, fm-spawn, and review q. For each failed action, assert the
command reports failure without applying the completed-state label, preserves
retryability, and does not create duplicate spawn or review work on retry.

In `@tests/fm-review-decision.test.sh`:
- Around line 68-79: Remove the FM_HOME, FM_REVIEW_BIN, and
FM_REVIEW_DECISIONS_LOG environment-variable prefix from the printf command in
the stdin branch; retain those variables on the bash "$TOOL" pipeline command so
behavior matches the non-stdin path.
- Around line 222-244: Extend the test suite with a best-effort audit-log
failure case, using the existing run_decision and assertion helpers. Set
FM_REVIEW_DECISIONS_LOG to an unwritable path, then verify exit code 0, a
WARNING: message, and successful wake and note effects; register the new test
with the other test calls.
- Around line 191-202: Update t_note_failure in the test to assert the
wake-queue state after run_decision reports the annotation failure. Verify that
no wake entry was enqueued when storing the review note fails, while preserving
the existing non-zero exit and diagnostic assertions.

🪄 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: 4ad5dac7-9b79-459b-b107-73f70f14e86f

📥 Commits

Reviewing files that changed from the base of the PR and between 85ea6c0 and c9e8dd9.

📒 Files selected for processing (35)

• .agents/skills/coderabbit-pr-gate/SKILL.md
• .agents/skills/herdr-navigation/SKILL.md
• .agents/skills/night-ops-directive/SKILL.md
• .claude/settings.json
• .codex/hooks.json
• .gitignore
• AGENTS.md
• README.md
• bin/claude-1.sh
• bin/claude-2.sh
• bin/claude-account.sh
• bin/fm-bead-stamp.sh
• bin/fm-brief-hooks.d/beads.sh
• bin/fm-brief.sh
• bin/fm-groom-json-field.sh
• bin/fm-groom-lib.sh
• bin/fm-groom.sh
• bin/fm-herdr-spur.sh
• bin/fm-idea-mine.ts
• bin/fm-isolated-launch.sh
• bin/fm-review-decision.sh
• bin/fm-review-page.ts
• bin/fm-spawn-hooks.d/beads.sh
• bin/fm-spawn.sh
• bin/fm-watch.sh
• docs/configuration.md
• docs/examples/herdr-spur.agents
• docs/scripts.md
• tests/claude-account.test.sh
• tests/fm-beads-hooks.test.sh
• tests/fm-groom.test.sh
• tests/fm-idea-mine.test.sh
• tests/fm-review-decision.test.sh
• tests/fm-review-page.test.sh
• tests/fm-spawn-account.test.sh

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Constrain the yolo exception to the documented authority boundary.

Lines 29-32 require captain escalation for security-sensitive actions. Line 34 also permits a yolo-authorized merge past an unresolved actionable finding without repeating the routine, reversible, and in-scope conditions. This can bypass the security-sensitive escalation rule.

Limit the exception to a standing yolo posture for a routine, reversible merge within accepted task criteria.

Based on the PR objective, reversibility is the yolo qualification for routine gates, including PR merges.

Proposed policy change

-4. Do not merge past an unresolved actionable CodeRabbit finding without either fixing it or
-   getting an explicit captain or `yolo`-authorized decision to proceed anyway.
+4. Do not merge past an unresolved actionable CodeRabbit finding without either fixing it,
+   getting explicit captain approval, or using a standing `yolo` posture that qualifies the
+   merge as routine, reversible, and within accepted task criteria. `yolo` does not override
+   captain escalation for security-sensitive findings.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	4. Do not merge past an unresolved actionable CodeRabbit finding without either fixing it or
	getting an explicit captain or `yolo`-authorized decision to proceed anyway.
	4. Do not merge past an unresolved actionable CodeRabbit finding without either fixing it,
	getting explicit captain approval, or using a standing `yolo` posture that qualifies the
	merge as routine, reversible, and within accepted task criteria. `yolo` does not override
	captain escalation for security-sensitive findings.

🧰 Tools

🪛 SkillSpector (2.4.4)

[info] 30: [EA3] Scope Creep: Skill's behavior or capabilities extend beyond its stated purpose. Scope creep allows an agent to perform actions unrelated to its documented functionality, increasing the attack surface.

Remediation: Limit the skill's scope to its documented purpose. Remove instructions that enable the agent to perform actions outside its stated functionality.

(Excessive Agency (EA3))

🤖 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/skills/coderabbit-pr-gate/SKILL.md around lines 33 - 34, Revise the
unresolved actionable CodeRabbit finding rule so the `yolo` exception applies
only to a standing `yolo` posture for routine, reversible merges within accepted
task criteria. Preserve the requirement for captain escalation on
security-sensitive actions, preventing this exception from bypassing that
authority boundary.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Restrict direct edits to shared firstmate material.

The exception allows firstmate to edit shared tracked material whenever a direct edit is faster. It does not require an empty fleet or firstmate-coding-guidelines. It can include policy, skill, hook, and launcher files that enforce unattended-session safety. Add the fleet-empty guard, require the guidance skill for ship tasks, and delegate otherwise.

Proposed wording

-  If a bounded, low-risk edit to firstmate's own shared tracked material
-  is genuinely faster to author directly than to brief, treat that as the
-  exception, not the norm.
+  Direct edits to firstmate's shared tracked material are allowed only when
+  the fleet is empty. For a ship task that touches this material, load
+  `firstmate-coding-guidelines` before editing; otherwise delegate the change
+  to a crewmate.

As per coding guidelines, direct edits require an empty fleet, and ship tasks require firstmate-coding-guidelines before editing.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- Firstmate stays a delegator for the whole session, including overnight: read state, decide
	routing, write briefs, spawn crewmates, supervise, and report; do not personally hand-edit
	project files, grind through binary or database debugging, or `git diff` a project clone to
	do a crewmate's job. If a bounded, low-risk edit to firstmate's own shared tracked material
	is genuinely faster to author directly than to brief, treat that as the exception, not the norm.
	- Firstmate stays a delegator for the whole session, including overnight: read state, decide
	routing, write briefs, spawn crewmates, supervise, and report; do not personally hand-edit
	project files, grind through binary or database debugging, or `git diff` a project clone to
	do a crewmate's job. Direct edits to firstmate's shared tracked material are allowed only when
	the fleet is empty. For a ship task that touches this material, load
	`firstmate-coding-guidelines` before editing; otherwise delegate the change
	to a crewmate.

🧰 Tools

🪛 SkillSpector (2.4.4)

[error] 14: [RA1] Self-Modification: Skill modifies its own code, configuration, or behavior at runtime. Self-modification enables an agent to escalate privileges, disable safety constraints, or install persistent backdoors.

Remediation: Prevent the skill from modifying its own code, SKILL.md, or configuration files. Treat skill files as read-only at runtime.

(Rogue Agent (RA1))

🤖 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/skills/night-ops-directive/SKILL.md around lines 22 - 26, Update the
direct-edit exception in the Firstmate delegation policy to require an empty
fleet and the firstmate-coding-guidelines skill for ship tasks before editing
shared tracked material. Preserve delegation as the default, including for
policy, skill, hook, and launcher files, and allow direct edits only when these
conditions are satisfied and the change is bounded and low risk.

Source: Coding guidelines

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Replace the duplicated Beads blocks with one scoped pointer.

Lines 508-586 add two ## Beads Issue Tracker sections. This triggers MD024. These blocks also duplicate command and lifecycle policy in an always-loaded file that already defines firstmate task tracking and completion behavior.

Keep one short instruction that scopes Beads to applicable project work and points to the Beads skill or its authoritative documentation. Do not make AGENTS.md a second owner of Beads workflow details.

As per coding guidelines, “Keep AGENTS.md for knowledge useful to almost every future agent session in this project; do not repeat what the codebase already shows” and “prefer rewriting or pruning over appending.”

🧰 Tools

🪛 markdownlint-cli2 (0.23.1)

[warning] 565-565: Multiple headings with the same content

(MD024, no-duplicate-heading)

🤖 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 508 - 586, Replace both duplicated Beads sections in
AGENTS.md with one concise, scoped pointer stating that applicable project work
uses Beads and directing agents to the Beads skill or authoritative
documentation for workflow details. Remove the duplicated commands, lifecycle
protocol, architecture explanation, and profile policy so AGENTS.md is not a
second owner of Beads guidance and only one Beads heading remains.

Sources: Coding guidelines, Linters/SAST tools

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In Claude Code, does setting projects.<path>.hasTrustDialogAccepted in .claude.json bypass the workspace trust dialog for that path?

💡 Result:

Yes, manually setting hasTrustDialogAccepted to true within the project entry in your ~/.claude.json file can effectively bypass the workspace trust dialog for that specific directory [1][2][3]. In Claude Code, the ~/.claude.json file stores per-project state, including trust settings [4][5]. The structure for these settings is defined under a projects key, where each project path serves as a key containing its configuration, including the hasTrustDialogAccepted boolean [6][3][7]. If you are experiencing persistent prompts despite accepting the dialog, this manual configuration in ~/.claude.json is a recognized method to resolve the issue [1][2][3]. You should ensure the path is correctly formatted for your operating system (e.g., using double backslashes on Windows if necessary) [7] and placed under the projects object as follows [3][7]: { "projects": { "/path/to/your/project": { "hasTrustDialogAccepted": true } } } Please note that while this is a functional workaround, there have been documented bugs where the trust dialog acceptance failed to persist automatically to this file [1][3]. If the setting is correctly applied in the file but the prompt continues to appear, it may indicate a specific environment issue or a regression in how the file is read [1][3].

Citations:

• 1: [BUG] Trust dialog acceptance not persisted to .claude.json anthropics/claude-code#36403
• 2: [BUG] Trust prompt appears every session when running Claude Code from home directory (~) anthropics/claude-code#18942
• 3: [BUG] hasTrustDialogAccepted never persisted to ~/.claude.json despite repeated interactive sessions anthropics/claude-code#71562
• 4: https://code.claude.com/docs/en/settings
• 5: https://code.claude.com/docs/en/configuration
• 6: https://gist.github.com/Frisch12/03dea0f3920079723bacc7eae645e41a
• 7: [BUG] Claude on native Windows repeatedly asks to trust folder anthropics/claude-code#3366

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate claude-account and wrappers =="
fd -a 'claude-(account|1|2)\.sh$|\.envrc|AGENTS\.md|README\.md' . | sed 's#^\./##'

echo "== bin files size/contents =="
for f in bin/claude-account.sh bin/claude-1.sh bin/claude-2.sh; do
  if [ -f "$f" ]; then
    echo "--- $f ($(wc -l < "$f") lines) ---"
    sed -n '1,140p' "$f" | cat -n
  fi
done

echo "== search for hasTrustDialogAccepted / CLAUDE_TRUST_DIR =="
rg -n "hasTrustDialogAccepted|CLAUDE_TRUST_DIR|trust_dir|claude-(1|2|account)" .

Repository: trillium/firstmate

Length of output: 11542

---

Do not pre-accept trust for the launcher directory by default.

bin/claude-1.sh and bin/claude-2.sh invoke claude-account.sh without CLAUDE_TRUST_DIR, so line 62 pre-trusts the current working directory before Claude evaluates it. Keep onboarding pre-acceptance, but only write projects.<path>.hasTrustDialogAccepted when an explicit trusted directory is supplied. Add a test that launches without CLAUDE_TRUST_DIR and checks that no project trust entry is created.

🤖 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 62 - 86, Update the Python trust-handling
block in claude-account.sh to preserve onboarding pre-acceptance while only
creating projects.<path>.hasTrustDialogAccepted when CLAUDE_TRUST_DIR is
explicitly provided. Avoid using the default current working directory for trust
initialization, and add coverage for launching without CLAUDE_TRUST_DIR that
verifies no project trust entry is written.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file sizes =="
wc -l bin/claude-account.sh tests/claude-account.test.sh 2>/dev/null || true

echo "== relevant launcher source =="
sed -n '1,180p' bin/claude-account.sh

echo "== relevant test source =="
sed -n '130,200p' tests/claude-account.test.sh

echo "== dangerous mode occurrences =="
rg -n "dangerously|skipDangerousModePermissionPrompt|CLAUDE_CONFIG_DIR|DANGEROUS|dangerous" bin tests -S

Repository: trillium/firstmate

Length of output: 15053

---

Gate dangerous-mode consent on the dangerous-mode flag.

bin/claude-account.sh#L102-L133 writes skipDangerousModePermissionPrompt even for normal Claude launches before exec claude "$@"; update tests/claude-account.test.sh#L161-L179 to cover a non-flagged launch preserving the setting and add a separate --dangerously-skip-permissions case for the launcher.

📍 Affects 2 files

• bin/claude-account.sh#L102-L133 (this comment)
• tests/claude-account.test.sh#L161-L179

🤖 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 102 - 133, The settings update in
bin/claude-account.sh, around the SETTINGS handling block, must run only when
the launch arguments include --dangerously-skip-permissions; leave settings
unchanged for normal launches while preserving the existing symlink protection.
Update tests/claude-account.test.sh around the existing launcher coverage to
assert a non-flagged launch preserves the setting and add a separate
--dangerously-skip-permissions case verifying the setting is applied.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Enforce the isolation invariant for path overrides.

FM_ISOLATED_CWD can resolve inside REAL_HOME, where Claude can rediscover the operator's global .claude/CLAUDE.md. FM_ISOLATED_HOME can also be set to REAL_HOME, which removes configuration isolation.

Resolve both paths before setup. Reject unsafe overrides instead of relying on the default values.

Also applies to: 189-196

🤖 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 73 - 74, Update the path override
handling near ISOLATED_HOME and ISOLATED_CWD to resolve both configured paths
before setup and reject overrides that equal or reside within REAL_HOME,
including FM_ISOLATED_HOME set to REAL_HOME. Preserve the existing defaults when
overrides are absent, and fail immediately with a clear error for unsafe values
before creating or using the isolated environment.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Restrict the data exposed to the isolated Claude session.

The symlink exposes the entire $REAL_HOME/data tree. The child is launched with --dangerously-skip-permissions, so a prompt injection or tool mistake can read or modify unrelated operational data.

Link only the required store directories, or provide explicit per-store paths. Validate any existing data path before using it.

🤖 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 106 - 108, Update the isolated-home
data setup around the data symlink creation so it does not expose the entire
REAL_HOME/data tree. Link only the required store directories using explicit
per-store paths, and validate that any existing isolated data path is an
approved directory or symlink target before using it; preserve the existing
setup behavior for valid required stores.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Create the credential file with private permissions from the start.

Shell redirection creates the predictable .tmp file before chmod 600 runs. A permissive umask can expose the OAuth token during the write, and another process can race the predictable path.

Set umask 077 and use a private mktemp file followed by an atomic rename. Check both chmod and mv.

🤖 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 the security command to set umask 077, create a private
unpredictable temporary file with mktemp, and write credentials to that file
instead of the predictable .tmp path. Require both chmod and mv to succeed
before completing the atomic rename, and clean up the temporary file on any
failure.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Annotate the item before enqueuing the wake.

The wake lands first (line 127). If review note then fails (line 141), the script exits 1 and the endpoint returns non-2xx, but the check: review-decision:<id> wake is already durable in the queue. Two consequences follow:

• Firstmate drains that wake and acts on a decision that the captain was told did not record.
• The panel re-enables its buttons on a non-2xx response (bin/fm-review-page.ts, line 834). Each retry appends another wake with the same key, so firstmate can act on the same decision several times.

Swap the order. The store annotation becomes the first durable write, and the wake is only enqueued once the decision is visible on the item. A failure then leaves no queued action that contradicts the reported error. Update the ordering description at lines 14-25 and 27-33 to match.

Based on learnings: "Report outcomes faithfully; if work failed, state it plainly with the evidence" — the current order reports failure after a side effect already landed.

🛡️ Proposed reorder

 # ---------------------------------------------------------------------------
-# (a) DURABLE WAKE - load-bearing, fail loud on any failure.
-# The payload is a compact, human-readable line firstmate reads directly off the
-# drained wake: verdict first, then the comment if present.
-# ---------------------------------------------------------------------------
-WAKE_PAYLOAD="captain decided ${VERDICT} on ${ID}"
-if [ -n "${COMMENT//[[:space:]]/}" ]; then
-  WAKE_PAYLOAD="${WAKE_PAYLOAD} - ${COMMENT}"
-fi
-if ! fm_wake_append check "review-decision:${ID}" "$WAKE_PAYLOAD"; then
-  die "failed to enqueue wake for ${ID} (queue: ${FM_WAKE_QUEUE})"
-fi
-
-# ---------------------------------------------------------------------------
-# (b) STORE ANNOTATION - load-bearing, fail loud. Makes the decision visible in
+# (a) STORE ANNOTATION - load-bearing, fail loud, and FIRST. Nothing is queued
+# for firstmate until the decision is visible on the item, so a failure here
+# never leaves a wake that contradicts the error the captain sees. Visible in
 # `review show <id>` so the item carries its own decision trail.
 # ---------------------------------------------------------------------------
 NOTE="Captain decision: ${VERDICT}"
@@
 rm -f "$NOTE_ERR"
+
+# ---------------------------------------------------------------------------
+# (b) DURABLE WAKE - load-bearing, fail loud on any failure.
+# The payload is a compact, human-readable line firstmate reads directly off the
+# drained wake: verdict first, then the comment if present.
+# ---------------------------------------------------------------------------
+WAKE_PAYLOAD="captain decided ${VERDICT} on ${ID}"
+if [ -n "${COMMENT//[[:space:]]/}" ]; then
+  WAKE_PAYLOAD="${WAKE_PAYLOAD} - ${COMMENT}"
+fi
+if ! fm_wake_append check "review-decision:${ID}" "$WAKE_PAYLOAD"; then
+  die "failed to enqueue wake for ${ID} (queue: ${FM_WAKE_QUEUE}); the item was annotated"
+fi

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# ---------------------------------------------------------------------------
	# (a) DURABLE WAKE - load-bearing, fail loud on any failure.
	# The payload is a compact, human-readable line firstmate reads directly off the
	# drained wake: verdict first, then the comment if present.
	# ---------------------------------------------------------------------------
	WAKE_PAYLOAD="captain decided ${VERDICT} on ${ID}"
	if [ -n "${COMMENT//[[:space:]]/}" ]; then
	WAKE_PAYLOAD="${WAKE_PAYLOAD} - ${COMMENT}"
	fi
	if ! fm_wake_append check "review-decision:${ID}" "$WAKE_PAYLOAD"; then
	die "failed to enqueue wake for ${ID} (queue: ${FM_WAKE_QUEUE})"
	fi
	# ---------------------------------------------------------------------------
	# (b) STORE ANNOTATION - load-bearing, fail loud. Makes the decision visible in
	# `review show <id>` so the item carries its own decision trail.
	# ---------------------------------------------------------------------------
	NOTE="Captain decision: ${VERDICT}"
	if [ -n "${COMMENT//[[:space:]]/}" ]; then
	NOTE="${NOTE} - ${COMMENT}"
	fi
	NOTE="${NOTE} @ ${TS}"
	NOTE_ERR="$(mktemp "${TMPDIR:-/tmp}/fm-review-decision-note.XXXXXX")"
	if ! BEADS_ACTOR="$DECISION_ACTOR" "$REVIEW_BIN" note "$ID" "$NOTE" >/dev/null 2>"$NOTE_ERR"; then
	err="$(cat "$NOTE_ERR" 2>/dev/null || true)"; rm -f "$NOTE_ERR"
	die "failed to annotate review item ${ID}: ${err}"
	fi
	rm -f "$NOTE_ERR"
	# ---------------------------------------------------------------------------
	# (a) STORE ANNOTATION - load-bearing, fail loud, and FIRST. Nothing is queued
	# for firstmate until the decision is visible on the item, so a failure here
	# never leaves a wake that contradicts the error the captain sees. Visible in
	# `review show <id>` so the item carries its own decision trail.
	# ---------------------------------------------------------------------------
	NOTE="Captain decision: ${VERDICT}"
	if [ -n "${COMMENT//[[:space:]]/}" ]; then
	NOTE="${NOTE} - ${COMMENT}"
	fi
	NOTE="${NOTE} @ ${TS}"
	NOTE_ERR="$(mktemp "${TMPDIR:-/tmp}/fm-review-decision-note.XXXXXX")"
	if ! BEADS_ACTOR="$DECISION_ACTOR" "$REVIEW_BIN" note "$ID" "$NOTE" >/dev/null 2>"$NOTE_ERR"; then
	err="$(cat "$NOTE_ERR" 2>/dev/null || true)"; rm -f "$NOTE_ERR"
	die "failed to annotate review item ${ID}: ${err}"
	fi
	rm -f "$NOTE_ERR"
	# ---------------------------------------------------------------------------
	# (b) DURABLE WAKE - load-bearing, fail loud on any failure.
	# The payload is a compact, human-readable line firstmate reads directly off the
	# drained wake: verdict first, then the comment if present.
	# ---------------------------------------------------------------------------
	WAKE_PAYLOAD="captain decided ${VERDICT} on ${ID}"
	if [ -n "${COMMENT//[[:space:]]/}" ]; then
	WAKE_PAYLOAD="${WAKE_PAYLOAD} - ${COMMENT}"
	fi
	if ! fm_wake_append check "review-decision:${ID}" "$WAKE_PAYLOAD"; then
	die "failed to enqueue wake for ${ID} (queue: ${FM_WAKE_QUEUE}); the item was annotated"
	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-review-decision.sh` around lines 118 - 145, Move the STORE ANNOTATION
block using NOTE, NOTE_ERR, and the review note command before the DURABLE WAKE
block invoking fm_wake_append, so annotation succeeds before any wake is queued.
Preserve the existing failure handling and payload construction, then update the
ordering comments near the script’s section descriptions to state that
annotation is the first durable write and wake enqueue follows it.

Source: Learnings

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Bun node:child_process spawnSync timeout returns status null signal SIGTERM

💡 Result:

When a process spawned via Bun.spawnSync (or the Node.js compatibility layer child_process.spawnSync) hits a timeout, it is intentionally terminated by the runtime using a killSignal (which defaults to SIGTERM) [1][2][3]. The observed behavior—where status returns null and signal is SIGTERM—is expected and consistent with how Node.js represents a process that did not exit normally but was instead terminated by a signal [4][5]. Key points regarding this behavior: 1. Status vs. Signal: In Node.js and Bun's compatibility layer, when a process terminates due to a signal, the status (or exitCode) property is set to null to indicate that the process did not have a standard exit code [5]. The signal property is then populated with the signal name (e.g., 'SIGTERM') that caused the termination [4]. 2. Timeout Mechanism: When the timeout option is configured, Bun enforces this limit by sending the specified killSignal to the child process [1][6]. If the process is unresponsive to SIGTERM, it will remain in a state where it has been signaled, which the parent process waits for [7]. 3. Previous Issues: Earlier versions of Bun had bugs where signal was incorrectly reported as null in spawnSync [4], or where timeout behaviors varied on specific platforms like Windows [8]. Recent updates have improved compatibility to match Node.js behavior, ensuring that signal is correctly reported as the terminating signal [4][5]. If you are seeing status: null and signal: 'SIGTERM', your code is correctly identifying that the process was killed due to a timeout [1][2]. You can customize the signal used by providing the killSignal option (e.g., 'SIGKILL') in your spawnSync configuration if a stronger termination signal is required [1][9].

Citations:

• 1: https://bun.sh/reference/bun/spawnSync
• 2: https://bun.sh/docs/runtime/child-process
• 3: https://bun.sh/reference/bun/Spawn/SpawnSyncOptions/timeout
• 4: child_process.spawnSync does not provide signal oven-sh/bun#15111
• 5: node:child_process: match Node's spawnSync result shape when the spawn fails oven-sh/bun#32878
• 6: https://bun.com/docs/runtime/child-process
• 7: https://bun.sh/reference/node/child_process/spawnSync
• 8: Bun.spawnSync({ timeout }) returns exitCode: null with empty stdout/stderr for a real ssh.exe child on Windows — not reproduced with a synthetic closed-port case oven-sh/bun#33932
• 9: https://bun.com/reference/bun/Spawn/SpawnSyncOptions/killSignal

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '=== locate file ===\n'
git ls-files | rg '(^|/)bin/fm-review-page\.ts$|(^|/)AGENTS\.md$|(^|/)README\.md$|(^|/)bin/package\.json$|(^|/)bun\.lockb$|(^|/)package\.json$|(^|/)bun\.lock$' || true

printf '\n=== fm-review-page outline ===\n'
if [ -f bin/fm-review-page.ts ]; then
  wc -l bin/fm-review-page.ts
  ast-grep outline bin/fm-review-page.ts || true
  printf '\n=== relevant fm-review-page sections ===\n'
  sed -n '1,220p' bin/fm-review-page.ts | nl -ba
fi

printf '\n=== search runReview/parseJsonItems/index.html writes ===\n'
rg -n "function runReview|runReview\s*\(|parseJsonItems|index\.html|REVIEW_BIN|CMD_TIMEOUT_MS|parse" bin/fm-review-page.ts || true

Repository: trillium/firstmate

Length of output: 2963

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '=== relevant fm-review-page sections ===\n'
sed -n '132,185p' bin/fm-review-page.ts
printf '\n=== parsing/index write sections ===\n'
sed -n '930,1032p' bin/fm-review-page.ts
printf '\n=== main sections ===\n'
sed -n '1040,1110p' bin/fm-review-page.ts

printf '\n=== parseJsonItems behavioral probe ===\n'
python3 - <<'PY'
from pathlib import Path
import re

p = Path("bin/fm-review-page.ts")
text = p.read_text()
m = re.search(r'function\s+parseJsonItems\s*\([^)]*\)\s*:\s*[^{]+\{(?P<body>.*?)\n}', text, re.S)
if m:
    print(m.group(0))
    body = m.group("body")
    for k in ("JSON.parse", ".map(", ".filter(", "return"):
        print("contains", k, "=>", k in body)
else:
    print("parseJsonItems function not found")
PY

printf '\n=== spawnSync/timeout/guard static invariant check ===\n'
python3 - <<'PY'
from pathlib import Path
import re

text = Path("bin/fm-review-page.ts").read_text()
checks = {
    "has timeout option": bool(re.search(r'runReview\(\{\s*timeout:\s*CMD_TIMEOUT_MS\s*,?\s*', text, re.S)),
    "has stdio ignore/pipe/pipe": 'stdio: ["ignore", "pipe", "pipe"]' in text,
    "has res.error guard": re.search(r'if\s*\(\s*res\.error\s*\)', text) is not None,
    "has signal guard": re.search(r'if\s*\(\s*res\.signal\s*\)', text) is not None,
    "has non-number status guard": re.search(r'typeof\s+res\.status\s*!\s*===\s*["\']number["\']|\bstatus\s*===\s*null', text) is not None,
    "has res.status integer nonzero guard": bool(re.search(r'typeof\s+res\.status\s*===\s*["\']number["\']\s*&&\s*res\.status\s*!==\s*0', text)),
}
for k, v in checks.items():
    print(f"{k}: {v}")
PY

Repository: trillium/firstmate

Length of output: 8600

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '=== fetchAllOpen/fetchByIds sections ===\n'
sed -n '215,245p' bin/fm-review-page.ts

printf '\n=== runReview call sites ===\n'
rg -n "runBenchmark\(|runReview\(|review \\.\\.\\." bin/fm-review-page.ts || true
rg -n "function run.*Benchmark|function runReview" bin/fm-review-page.ts || true

Repository: trillium/firstmate

Length of output: 1889

---

🌐 Web query:

Bun node:child_process spawnSync missing binary error ENOENT status null

💡 Result:

The ENOENT error with status null in Bun's child_process.spawnSync is a documented behavior indicating that the child process failed to start (e.g., the binary was not found) [1][2]. Recent updates to Bun have specifically aligned this behavior with Node.js to ensure consistency [1][2]. When spawnSync fails to start a process due to an ENOENT (or EACCES) error, it returns a result object with a standardized shape [1][2]: - pid: 0 - status: null - signal: null - output: null - stdout: undefined - stderr: undefined - error: [The error object containing code: 'ENOENT', syscall, and path] Key points regarding this error: 1. Behavioral Alignment: Earlier versions of Bun sometimes deviated from Node's expected return shape (e.g., returning undefined instead of null for status), leading to compatibility issues [1][2]. Current versions (post-fix PRs #31768 and #32878) now return the expected Node-compatible result [1][2]. 2. Troubleshooting Causes: If you encounter this error, it typically means the specified binary cannot be located or accessed [1][3]. Common causes include: - Invalid or missing PATH: Bun may not be resolving the binary correctly if the environment is restricted or if the PATH is not set as expected [4]. - Working Directory Issues: If an invalid or inaccessible cwd is provided, Bun may report an ENOENT error on the executable itself, even if the executable exists [3]. - Sandbox Restrictions: When running inside restricted environments (like macOS Seatbelt sandboxes), process.env can sometimes be empty, preventing path resolution [4]. Providing an explicit env object with PATH can often resolve this [4]. 3. Verification: You can use Bun.which(binary) to verify if the executable is discoverable by Bun before attempting to spawn it [5]. If you are using an older version of Bun, updating is recommended to ensure you have the improved spawn failure handling [2][6].

Citations:

• 1: child_process: match Node's spawnSync result shape when spawn fails oven-sh/bun#31768
• 2: node:child_process: match Node's spawnSync result shape when the spawn fails oven-sh/bun#32878
• 3: Bun.spawn throws incorrect ENOENT error when cwd does not exist oven-sh/bun#24012
• 4: process.env is empty (0 keys) when bun runs inside macOS Seatbelt sandbox oven-sh/bun#27802
• 5: https://bun.sh/reference/bun/spawnSync
• 6: child_process: match node's spawn-failure event contract oven-sh/bun#33304

---

Treat a timeout kill and missing exit status as failures.

res.status is null and res.signal is present when the child is terminated by the timeout, but the current guard ignores this case. On --all, fetchAllOpen() then uses truncated or empty stdout, parseJsonItems() returns [], and writeIndexPage() overwrites index.html with the empty queue message. Split the guard into res.signal, typeof res.status !== "number", then the non-zero check.

🤖 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 142 - 158, Update runReview to treat any
terminated child as a failure by checking res.signal first, then rejecting cases
where res.status is not a number, before retaining the existing non-zero status
check. Include the review command and available termination details in the
thrown errors so fetchAllOpen cannot process incomplete output.

Source: Linters/SAST tools

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

README.md (1)

142-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the isolated-home authentication guidance.

bin/fm-isolated-launch.sh attempts to seed the isolated .credentials.json from the macOS Keychain. A new login is required only when that extraction is unavailable or fails.

• README.md#L142-L142: State that login is conditional on credential seeding failure.
• docs/configuration.md#L200-L204: State the same conditional login behavior.

🤖 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 `@README.md` at line 142, Update the isolated-home authentication guidance to
state that a new login is required only when credential seeding from the macOS
Keychain is unavailable or fails; apply this wording at README.md lines 142-142
and docs/configuration.md lines 200-204.

.claude/settings.json (1)

12-20: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Run the Beads hook from the project root and add a timeout.

Claude runs hooks with a 600s command timeout and uses the process current directory for relative paths. bd prime --hook-json can detect stale Linear data and pull when LINEAR_API_KEY is set, so the session can block on startup. Nest this hook under the Firstmate startup matcher and run it from $CLAUDE_PROJECT_DIR.

Proposed fix

-            "command": "bd prime --hook-json"
+            "command": "cd \"$CLAUDE_PROJECT_DIR\" && exec bd prime --hook-json",
+            "timeout": 30

🤖 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 @.claude/settings.json around lines 12 - 20, Update the hook configuration
containing the empty matcher so the Beads command is nested under the Firstmate
startup matcher, executes from $CLAUDE_PROJECT_DIR, and has an explicit short
timeout to prevent startup blocking during stale-data synchronization.

bin/fm-brief.sh (1)

164-190: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Base fork routing on repository permission data.

fork_repo_for_origin only parses the owner-like path component and skips the branch. Any non-Trillium owner emits the fork rule even if direct pushes work, while Trillium-owned origins do not emit it even if they are read-only. A non-GitHub URL ending in /trillium/<repo> can also avoid the fork rule.

Require an authoritative permission check or repository-policy input before emitting FORK_FIRST; keep parsed owner values as display data only.

🤖 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 164 - 190, Update fork_repo_for_origin so it
emits a repository name only after an authoritative permission or
repository-policy check confirms the origin requires forking; do not infer
pushability from the parsed owner or URL shape. Use the existing
policy/permission source, treating owner and repository parsing only as display
metadata, and preserve the empty successful result when permission data is
unavailable or direct pushes are allowed.

🤖 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.

Outside diff comments:
In @.claude/settings.json:
- Around line 12-20: Update the hook configuration containing the empty matcher
so the Beads command is nested under the Firstmate startup matcher, executes
from $CLAUDE_PROJECT_DIR, and has an explicit short timeout to prevent startup
blocking during stale-data synchronization.

In `@bin/fm-brief.sh`:
- Around line 164-190: Update fork_repo_for_origin so it emits a repository name
only after an authoritative permission or repository-policy check confirms the
origin requires forking; do not infer pushability from the parsed owner or URL
shape. Use the existing policy/permission source, treating owner and repository
parsing only as display metadata, and preserve the empty successful result when
permission data is unavailable or direct pushes are allowed.

In `@README.md`:
- Line 142: Update the isolated-home authentication guidance to state that a new
login is required only when credential seeding from the macOS Keychain is
unavailable or fails; apply this wording at README.md lines 142-142 and
docs/configuration.md lines 200-204.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6235b3f8-e6ad-4e3d-b13a-90147a8fb0fe

📥 Commits

Reviewing files that changed from the base of the PR and between c9e8dd9 and 3e33d78.

📒 Files selected for processing (8)

• .claude/settings.json
• .gitignore
• AGENTS.md
• README.md
• bin/fm-brief.sh
• bin/fm-spawn.sh
• docs/configuration.md
• docs/scripts.md

🚧 Files skipped from review as they are similar to previous changes (3)

• docs/scripts.md
• .gitignore
• AGENTS.md

[trillium]

Superseded by PR #22 which reships this content clean through no-mistakes (night-ops-directive + coderabbit-pr-gate skills, yolo-reversibility clarification; the general fork-first line was dropped as already covered by fork-first-brief-08). Closing.