refactor(agents): move conditional workflows into skills - #6
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideThis refactor substantially shrinks AGENTS.md by retaining always-loaded safety and supervision boundaries while moving conditional intake, delivery, and backlog procedures into trigger-based internal skills; dependent skills, scripts, docs, and tests now reference those policy owners, with focused delegation-guard scenarios passing and semantic skill-trigger loading remaining untested. Sequence diagram for task intake and delivery skill loadingsequenceDiagram
actor Captain
participant Firstmate
participant TaskIntake as task-intake
participant Spawn as fm-spawn.sh
participant Worker
participant TaskDelivery as task-delivery
participant Backlog as backlog-management
Captain->>Firstmate: New project request
Firstmate->>TaskIntake: Load for classification and dispatch
TaskIntake->>Firstmate: Resolve project, task kind, mode, profile
Firstmate->>Spawn: Create brief and spawn with explicit mode and yolo
Spawn->>Worker: Start isolated task
Worker-->>Firstmate: Validation or delivery milestone
Firstmate->>TaskDelivery: Load for validation, landing, or teardown
TaskDelivery->>Worker: Apply delivery gate
TaskDelivery->>Backlog: Re-evaluate queue after completion or teardown
Flow diagram for guarded delegation routingflowchart TD
TOOL[Firstmate tool request]
GUARD[fm-subagent-pretool-check]
CLASSIFY[Load task-intake\nand classify work]
DISPATCH[fm-brief.sh then fm-spawn.sh]
ALLOW[Allow ordinary, observe-only, plan-only, or MCP tool]
DENY[Deny harness-native delegation]
TOOL --> GUARD
GUARD -->|delegation-shaped| DENY
DENY --> CLASSIFY
CLASSIFY --> DISPATCH
GUARD -->|non-delegation| ALLOW
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Sep 24, 2026
twilwa
added a commit
that referenced
this pull request
Sep 25, 2026
* docs: audit AGENTS.md size and ownership * docs: slim always-loaded Firstmate contract * no-mistakes(review): drop audit doc, dedupe skill triggers, fix stale pointers * no-mistakes(review): fix yolo brief split, state guard, and stale pointers * no-mistakes(review): restore backstop wake duty, dedupe trigger, repoint pointers * no-mistakes(document): Repoint stale brief guidance comment
twilwa
added a commit
that referenced
this pull request
Sep 25, 2026
…merge handoff (#16) * feat(bin): pin resolver model and persist dispatch decision receipts (#1) * Fix dispatch resolver model and receipts * no-mistakes(review): Drop model-drift branch, harden receipt lock and brief join * no-mistakes(review): Scope receipt recording to clear, report failed joins, measure latency * no-mistakes(review): Narrow dispatch clause and concurrency test, shrink lock budget * no-mistakes(review): Accept --project on the join, assert drop-or-append concurrency * no-mistakes(review): Split lock budgets by path, drop receipt size bound * no-mistakes(review): Record brief_path as spelled, drop abs_path normalization * no-mistakes(review): Pin model in contract, bound receipt latency, record reason * no-mistakes(review): Report dropped resolution receipts, project profile agreement, drop dispatch_id * no-mistakes(review): Enforce append-only cmp, complete join example, govern latency bound * no-mistakes(review): Keep no-rules exit 0 without jq, dedupe error default * no-mistakes(review): Refuse symlinked receipts path, drop dead no_rules jq argument * no-mistakes(document): Document receipt identity, symlink refusal, jq exit narrowing * fix(bin): refuse unknown flags and stray --key arguments in fm-send (#3) * fix(bin): refuse an unrecognised fm-send flag instead of sending it as text fm-send's option loop ended in an unconditional `*) break ;;`, so any token it did not recognise - including one obviously shaped as a flag - fell out of the loop and became the positional message body. A steer invoked with a flag that does not exist was durably written into a live worker's steering inbox as the literal flag string while fm-send exited 0, so the worker was mis-steered and the caller got a success code and no diagnostic. The accepted set is now an allowlist rather than a pattern. --key is a real, supported flag parsed after this loop and must keep falling through it untouched, so a blanket "starts with -- and matched no case arm, therefore refuse" rule would have broken it. A bare -- ends flag parsing, which is how a message whose text starts with -- is sent. That separator is threaded to the two --key dispatch points so text after it is text everywhere rather than being re-parsed as a flag. A single-dash word was never a flag here and still needs no separator. The refusal exits before anything is marked, recorded, rung, or typed, the same discipline the header already applies to an empty message. * no-mistakes(review): drop -- end-of-flags separator, keep pure flag allowlist * no-mistakes(document): document fm-send's flag allowlist and leading-`--` message limit * docs(bin): drop the flag-allowlist commentary from fm-send's source The header block in bin/fm-send.sh is that script's documented contract. Recording the no-end-of-flags-separator limitation there amends that contract and turns a deliberate, narrow behaviour change into a documented guarantee the project would then owe. The rationale comment above the option loop goes for the same reason: the limitation describes a decision, which belongs in the pull request, not in the source, where it reads as a promise. Removes only those thirteen comment lines. The refusal itself is unchanged: the option loop remains a pure allowlist, --key still falls through to its own plane untouched, there is no end-of-flags handling, the usage line is unmodified, and the tests are untouched. * fix(bin): refuse trailing arguments after fm-send's --key The option loop breaks at --key without consuming what follows it, and the key path reads only the key itself, so every remaining argument was discarded in silence while the key was still delivered and the command still exited 0. `fm-send.sh lane --key Enter --not-a-real-flag` sent Enter and reported success. That is the same silent-delivery shape the unknown-flag refusal in this change exists to remove, so the key path contradicted the contract on that one path. The same ordering bypassed the --fire-and-forget incompatibility: FIRE_AND_FORGET_ID is only set when the flag precedes --key, so `--key Enter --fire-and-forget x` passed both existing guards. The key path now refuses any trailing argument before delivering the key, naming the offending token in the wording already used for an unknown flag in flag position, and names --fire-and-forget specifically so that incompatibility holds on either ordering. Adds regression coverage for both orderings and for a trailing plain word; both new tests fail before this commit and pass after it. * Add head-keyed PR review and post-merge QA gates (#4) * Add head-keyed PR review policy ledger * Add post-merge browser QA gate * Fix PR review and post-merge gates * Close remaining PR review gate gaps * Harden migration risk and QA evidence parsing * Close PR review guard bypasses * Tighten review evidence boundaries * Bind final review authorization * Invalidate stale review dispositions * Harden review evidence validation * feat(bin): record captain decision deferrals as dated answers (#2) * Add keyed decision defer mode * no-mistakes(review): Fix defer date identity, hold age, parent channel, reporting * no-mistakes(review): Derive board defer from the option's until alone * no-mistakes(review): Show the defer date on the board card * Fix deferred decision lifecycle edges * no-mistakes(review): Drop fabricated defer hold reason fallback * no-mistakes(document): Correct stale captain-defer docs for the recorded answer path * Fix defer intake failure edges * Require future dates for decision defers * no-mistakes(review): Narrow UTC day parsing; fix elapsed-defer recovery guidance * no-mistakes(review): Refuse duplicate board option values; fix defer recovery wording * Stabilize chat defer hold assertion * Keep chat defer date stable across midnight * Refactor defer validation for bounded lint * fix(bin): route ask-user gates back to firstmate as needs-decision (#5) * fix(brief): forbid validation auto-accept * no-mistakes(review): restore fleet-wide --yes ban, add ask-user routing sentence * no-mistakes(ci): Fixed a flaky test that failed the "Behavior portable serial 4" shard. Failure: tests/fm-pi-branch-extension.test.sh -> test_captain_outcome_processing_turn_is_sequence_keyed_and_re_presented, with "Error: supervision branch prompt settled but produced no durable outcome for its claimed wake rows" (thrown at .pi/extensions/fm-branch-supervision.ts:1548). Nothing in this PR's diff (the --yes DoD line, the harness-adapters sentence, three brief assertions) touches that extension or test; the other two check runs on the same head commit (99a0187) passed. It is a pre-existing race that surfaces on a slow/loaded runner. Root cause: in fm-branch-supervision.ts a wake builds the branch session (ensureBranch), then runs several awaited subprocesses (flushMirror, actingAsOwner, scopeForUnreadWake, writeEligibleRowsSnapshot, away-posture read-back) and only then snapshots reportRevisionBeforePrompt immediately before session.prompt(...); after the prompt settles it requires that revision to have advanced. The test synchronized on the wrong point: `settle(() => __fmSessions.length === 2, "replacement branch session")`. Session creation precedes that snapshot, so when the extension's pre-prompt work is slower than the test's report append, report2's durable append lands before the snapshot and the wake rejects its own settled prompt as outcome-less. The routine wake earlier in the same test already waits on __fmPrompts.length === 1 and is unaffected. Fix (tests/fm-pi-branch-extension.test.sh:1377, 9 insertions / 1 deletion): wait for the wake prompt as well as the replacement session, matching the routine wake's own idiom, with a comment naming why the built session is not the synchronization point. No production code changed; no new machinery. Verification: reproduced the exact CI error deterministically by temporarily injecting a delay ahead of reportRevisionBeforePrompt (delays 100/200/300/400/500/700 ms all failed with the identical message); that injection was reverted (git status shows only the test file modified). With the fix the test passes under injected delays of 100, 400 and 1500 ms. Full file run: exit 0, 45 tests passing. 24 parallel runs of the target test: 24/24 pass. shellcheck -x on the changed file is clean, and this PR's own tests (tests/fm-brief.test.sh, tests/fm-ask-user-authority.test.sh) still pass. The change is left uncommitted in the worktree, since prior rounds' commits on this branch were made by the executor rather than this phase * refactor(agents): move conditional workflows into skills (#6) * docs: audit AGENTS.md size and ownership * docs: slim always-loaded Firstmate contract * no-mistakes(review): drop audit doc, dedupe skill triggers, fix stale pointers * no-mistakes(review): fix yolo brief split, state guard, and stale pointers * no-mistakes(review): restore backstop wake duty, dedupe trigger, repoint pointers * no-mistakes(document): Repoint stale brief guidance comment * docs: cover omitted conditional skill load triggers * fix: bind resolver requests to immutable brief snapshots * fix(bin): bound session-start cleanup, defer summary publication, and avoid jq argv overflow (#10) * fix: bound startup reconciliation and large fleet input * no-mistakes(review): Drop redundant contribution-input EXIT trap in fleet snapshot * no-mistakes(test): Widen cleanup deadline test budget to avoid load flakes * no-mistakes(document): Document startup summary deferral and herdr cleanup deadline * no-mistakes(ci): Lint 1 failed because ShellCheck SC2329 ("function never invoked") fired at tests/fm-herdr-session-cleanup.test.sh:356. That line is a subshell copy of fixture_workspaces that replaces the file's main version. The fake herdr command calls fixture_workspaces indirectly when it answers `workspace list` and `api snapshot`, and ShellCheck can't see that call. The fix is one comment line above the replacement: `# shellcheck disable=SC2329 # invoked indirectly by the fake herdr workspace list.` The same file already does this for its other indirectly-called replacements (lines 43 and 49), as do tests/fm-daemon.test.sh and tests/fm-bootstrap.test.sh. No behavior changed. Checked locally: `bin/fm-lint.sh tests/fm-herdr-session-cleanup.test.sh` passes with pinned ShellCheck 0.11.0 and full extended analysis, and `bash tests/fm-herdr-session-cleanup.test.sh` passes every test, including the journal-read-count, deadline, lock and identity tests. The change is not committed * fix: reclaim cleanup locks after hard timeout * no-mistakes(review): Use shared fm_lock receipts lock; synthesize ledger fixtures (cherry picked from commit 5118fbce1f5ba294d74ec0862913a5c4bce7129d) * no-mistakes(document): Document cleanup lock reclaim and receipt state path (cherry picked from commit 53740853205c45ae4c8b835656224a60708998d6) * no-mistakes(review): Skip torn receipt lines, clear lock record, list --defer-until * no-mistakes(review): Start each receipt append on its own line * no-mistakes(document): Document torn receipt-line handling in dispatch receipts * no-mistakes(document): Mark dispatch receipt cost figures historical, pending remeasurement * no-mistakes(ci): ci-2 (Lint 2), caused by this PR, fixed. Invariant: a function only ever called by a trap must carry `# shellcheck disable=SC2329`, or the full-analysis lint fails. This PR added `reap_zombie_owner` in tests/fm-herdr-session-cleanup.test.sh, called only by `trap reap_zombie_owner EXIT`, without that directive. A local run of `bin/fm-lint.sh --partition 2of2` with the pinned ShellCheck 0.11.0 exited 1 with that single SC2329 finding (line 454). In CI the job was stopped (exit 143) at about 10.5 minutes, before it printed the finding; main's partition 2 took 441 s. Fix: added the directive, worded like the file's existing ones (lines 43, 49, 365). No other sites: that was the only partition-2 finding, and partition 1 passed in CI. Verified: `shellcheck --norc --external-sources -- tests/fm-herdr-session-cleanup.test.sh` exits 0. Not rerun: the full 24-minute partition after the fix, and the test itself (Test stays skipped). The fix is uncommitted in the worktree. ci-1 (Behavior portable serial 3), not caused by this PR, flaky, no change. The only failure is tests/fm-watch-checkpoint.test.sh, "watch lock pid survived quiet checkpoint timeout". bin/fm-watch.sh takes its singleton lock at line 2327 but only sets up its cleanup-on-exit trap at 2456; a timeout in between leaves .watch.lock/pid behind. Reproduced locally: `timeout 0.6`–`1.0` leaves the pid file, 0.2/0.4/1.5/2 s do not. fm-watch.sh, fm-watch-checkpoint.sh and the test are unchanged from base 040b337. The only changed file the watcher uses (fm-captain-hold.sh) runs at wake time, not during startup. The same code passed on main. Closing the gap means changing upstream watcher code, beyond this carry-forward; worth fixing separately. ci-3 (PR must be raised via no-mistakes), not caused by the code, no change. It fails with "Required no-mistakes pipeline steps are not completed: test (status=skipped)", which is expected because the user intent keeps Test skipped. ci-4 (Review changed files (advisory)), external, no change. It fails with "No OpenRouter API key configured": a missing repository secret, not a code defect * fix: make reviewed-head merge handoff opt-in * no-mistakes(review): Keep collector inline feedback; refuse held direct merges * no-mistakes(review): Attribute ledger merge checks; name configured high-stakes model
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AGENTS.md audit
The stage-1 audit inventoried every paragraph and bullet group in the 85,533-byte baseline.
The shipped file is 33,003 bytes, a reduction of 52,530 bytes (61.4%).
Safety boundaries remain always loaded; conditional procedures moved only where section 13 declares an explicit trigger.
docs/configuration.mdand producing script headers/helpbin/fm-session-start.sh,bootstrap-diagnosticstask-intake,harness-adapters,quota-array-dispatchstuck-crewmate-recovery,secondmate-provisioning, section 8project-management,secondmate-provisioning,stowtask-intake,task-delivery,pr-review-policybearings,fmx-respondbacklog-managementtask-intake,bin/fm-brief.shupdatefirstmatefmx-respondfirstmate-coding-guidelinesNew triggered skills
task-intaketask-deliverybacklog-managementIntent
can you open a lane on reviewing firstmate AGENTS.md, pruning irrelevant information, and seeing if there's anything
that only applies under certain circumstances and should be transposed into a skill that loads only during the appropriate
conditions? we wanna cut down AGENTs.md in size
What Changed
AGENTS.mdby retaining always-loaded operating rules and replacing conditional lifecycle details with targeted skill triggers and authoritative documentation links.Risk Assessment
✅ Low: The change cleanly relocates conditional operating procedures into triggered skills, preserves the necessary always-loaded safety boundaries, and introduces no substantiated behavioral defect or unnecessary component.
Testing
No separate baseline command was supplied; I ran the focused delegation-guard behavior test twice, captured reviewer-visible CLI evidence, confirmed denial routing, non-delegation allowances, escape-hatch boundaries, malformed-input behavior, and a clean worktree. All live-exercisable scenarios passed; this change has no UI surface requiring screenshots.
tests/fm-subagent-pretool-check.test.sh; artifact “Focused delegation-guard behavior transcript”tests/fm-subagent-pretool-check.test.sh; artifact “Focused delegation-guard behavior transcript”tests/fm-subagent-pretool-check.test.sh; artifact “Focused delegation-guard behavior transcript”Evidence: Focused delegation-guard behavior transcript
Source: Focused delegation-guard behavior transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
tests/fm-subagent-pretool-check.test.sh; artifact “Focused delegation-guard behavior transcript”tests/fm-subagent-pretool-check.test.sh; artifact “Focused delegation-guard behavior transcript”tests/fm-subagent-pretool-check.test.sh; artifact “Focused delegation-guard behavior transcript”rtk bash tests/fm-subagent-pretool-check.test.shrtk bash tests/fm-subagent-pretool-check.test.sh 2>&1 | tee ~/.no-mistakes/evidence/01M35F1EMBC79W79MFA3FJZ1D0/subagent-pretool-check.txtrtk git status --shortto confirm targeted testing left no transient worktree changes✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by Sourcery
Move conditional task lifecycle and backlog procedures out of AGENTS.md into targeted agent-only skills while preserving concise always-loaded safety boundaries.
New Features:
Enhancements:
Documentation:
Tests: