Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions .agents/skills/nathan-coding-loop/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
---
name: nathan-coding-loop
description: >-
Agent-only procedure for mandatory second-mate coordination of tracked code changes.
Load before routing, starting, or coordinating work that will change tracked code, and keep it loaded through the independent review loop.
Requires a PR-owning implementation worker and a separate read-only reviewer to alternate fixes and complete-diff review until the current PR head has no material findings, without granting merge authority.
user-invocable: false
metadata:
internal: true
---

# Second-mate code review

Load this skill before routing, starting, or coordinating work that will change tracked code.
This skill is the single procedure owner for the mandatory second-mate developer-reviewer loop.
The selected delivery path still owns implementation, validation, push, and PR mechanics, and `AGENTS.md` still owns merge authority.

## Scope and routing gate

This loop applies when authorized work will change tracked executable source, scripts, behavioral configuration, workflows, or tests.
Documentation changed as part of such work stays in the same loop.
A prose-only task remains under the ordinary task lifecycle unless it also changes executable behavior.

The main firstmate routes the complete work item to a fitting second mate rather than directly commissioning the implementation or review.
Use an existing scope-compatible second mate when one is available.
If none is available, load `secondmate-provisioning` and establish an appropriate persistent second mate before work starts.
A missing viable second-mate route is a blocker to escalate, not permission for the main firstmate to coordinate the loop itself.

The task must have a viable GitHub PR path using its already selected `no-mistakes` or `direct-PR` delivery mode.
A selected `local-only` mode, unavailable forge, missing push permission, or branch that cannot update its task PR is a blocker to escalate before implementation proceeds.
Do not silently downgrade, bypass the review, or invent a parallel delivery path.

## Roles and independence

The second mate is the coordinator and must not implement or review the change itself.
It commissions at least these two separate workers:

- One implementation worker owns the task branch and the selected PR delivery path.
- One independent reviewer owns read-only review of the PR and must not edit code, commit, push, run a fix agent, or operate the implementation worker's validation pipeline.

The implementation worker cannot review or approve its own work.
The reviewer must be a different agent in a different isolated task context and must not have authored any part of the task branch.
Keep the same implementation worker and reviewer through every round when they remain viable, because continuity makes disagreements and regressions explicit.
If either worker must be replaced, preserve the task records and require the replacement reviewer to inspect the complete current diff from the beginning.

Use the normal brief, spawn, dispatch-profile, supervision, steering, recovery, and cleanup contracts for both workers.
Scaffold the implementation as a ship using the selected PR mode.
Scaffold the reviewer as a scout, then make its task-specific instructions retain the no-code rule, the full-diff protocol below, the GitHub comment requirement, and the iterative wait for later heads.
Do not weaken either generated scaffold's safety sections.

## Establish the one task PR

Before the implementation worker opens anything, inspect the task's durable records and query GitHub with `gh-axi` for an existing open PR for its branch.
If the task already has a PR, give that full URL to the implementation worker and require updates to that PR.
If it does not, the implementation worker creates exactly one PR through the selected delivery path.
Never open a second PR to represent a fix round or review round.
If multiple plausible task PRs exist or the worker cannot update the existing PR, stop and escalate the ambiguity.

For `no-mistakes`, the implementation worker owns the complete gate interaction and reports once the PR checks are green.
For `direct-PR`, the implementation worker pushes and opens or updates the PR with `gh-axi` under the normal direct path.
After a PR exists, register it through the normal PR-check path so the task record follows the current forge head.
The reviewer never starts until the PR URL and exact current head are known.

## Independent complete-diff review

For each review round, the reviewer must independently obtain the current PR head and inspect the complete branch diff against the PR's authoritative base.
Use `gh-axi pr view`, `gh-axi pr diff --full`, and repository reads as needed rather than relying on the implementation worker's summary or only the commits since the prior round.
If using `bin/fm-review-diff.sh`, treat its local-branch fallback warning as insufficient for a conclusive review and escalate unless the PR head can be independently resolved.
Record the exact reviewed head SHA in the review report.
Re-read the PR head after the review, and restart the complete-diff review if it changed during inspection.

Review the actual changed behavior, tests, documentation, and integration boundaries.
Verify consistency with the repository's existing lifecycle, delivery-path, and merge-authority contracts, including their authoritative scripts, skills, and tests rather than prose recollection.
Inspect every affected supported harness and runtime integration surface required by `firstmate-coding-guidelines`.
Mark an axis not applicable only after inspecting the integration surface and record the reason in the review report.

A material finding is a concrete defect in accepted intent, correctness, safety, security, lifecycle behavior, delivery-path behavior, merge-authority preservation, compatibility, tests, or required documentation.
For every material finding, post an evidence-backed line-specific GitHub review comment through `gh-axi` against the reviewed head.
Consult current `gh-axi api --help` before posting and use the GitHub pull-request review-comment endpoint with the PR repository, pull number, current commit id, changed-file path, diff line, side, and body supplied explicitly.
The comment body states the observed evidence, consequence, and required correction.
Never post a summary-only issue in place of the required line-specific comment.
If a material concern cannot be anchored to a relevant changed line, report that limitation to the second mate instead of fabricating a line location, keep the review round open, and escalate before completion.

Do not post nits, praise, acknowledgements, or placeholder comments.
A clean review produces no GitHub comment merely to prove the review happened.
The reviewer reports `no material findings` to the second mate with the PR URL, reviewed head SHA, inspected surfaces, and validation evidence in its scout report and completion status.
The reviewer does not submit a GitHub approval, because reviewer satisfaction is evidence for the coordinator and never merge authority.

## Alternate fixes and review

When the reviewer posts material findings, the second mate sends the complete finding set to the same implementation worker.
The implementation worker resolves the findings on the existing task branch, updates the existing PR through the same selected delivery path, and returns the new exact head only after that path's required validation is complete.
For `no-mistakes`, branch custody and every fix or rerun remain governed by the installed no-mistakes procedure and the task lifecycle in `AGENTS.md`; neither the second mate nor reviewer edits around an active run.

The same reviewer then fetches the new head and performs another complete-diff review against the authoritative base.
Reviewing only the latest fix commit, only previously commented lines, or only the developer's response is not sufficient.
Repeat developer fixes followed by a fresh complete-diff review until the reviewer explicitly reports `no material findings` for the unchanged current PR head.
A reviewer dispute or unavailable line target goes to the second mate for resolution or escalation and never becomes a reviewer code edit.

## Completion and authority

The second mate may report the PR ready to the main firstmate only when all of these are true:

- The implementation worker's selected PR delivery path is complete for the current head.
- The reviewer explicitly reported `no material findings` for that same head.
- Every material comment from earlier rounds is addressed by the current diff or explicitly resolved without bypassing accepted intent.
- The PR remains unmerged.

Report the full PR URL, current head, selected path result, review-round count, and the reviewer's clean conclusion.
Do not translate reviewer satisfaction into approval to merge.
The captain's explicit approval or the project's standing `yolo` posture remains the only routine merge authority, and destructive, irreversible, security-sensitive, and red-check boundaries remain unchanged.
Neither the second mate nor either worker merges the PR as part of this loop.
22 changes: 15 additions & 7 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ config/calm Pi Calm presentation preference; LOCAL, gitignored, and not inhe
config/startup-memory-budget primary-authoritative per-home startup-memory budget; LOCAL, gitignored, materialized as 7,500 estimated tokens by locked primary bootstrap and inherited into secondmate homes; see docs/configuration.md "Startup memory budget"
config/stow-pass-horizon optional presence flag opting this home in to /stow's default-off pass-count decay horizon; LOCAL, gitignored, and not inherited; see docs/configuration.md "Stow pass horizon"
config/herdr-presentation-spaces optional "off" opt-out from, or "on" opt-in to, Herdr's default-on disposable single-task visual projection, which is unconfigured-default-on only at or above a Herdr version floor; LOCAL, gitignored; inherited by secondmate homes; see docs/herdr-backend.md "Presentation spaces"
config/herdr-completed-task-views optional "on" opt-in to bounded static completed-task tabs during successful Herdr cleanup; LOCAL, gitignored, default off, and not inherited; see docs/herdr-backend.md "Completed task views"
config/trace-context optional presence flag enabling default-off native W3C trace-context propagation to spawned agents; LOCAL, gitignored; inherited by secondmate homes; see docs/configuration.md "Trace context propagation" and docs/trace-context.md
config/cmux-socket-password optional cmux control-socket password; LOCAL, gitignored; read fresh on every cmux CLI call and passed through without ever overriding an operator's own ambient CMUX_SOCKET_PASSWORD when absent (docs/cmux-backend.md "Setup")
config/wedge-alarm optional away-mode wedge-alarm active-alert directives; LOCAL, gitignored; absent means auto (macOS Notification Center when available); see docs/wedge-alarm.md
Expand All @@ -98,6 +99,7 @@ state/ runtime records and signals; gitignored
<id>.inbox/ durable steering inbox: sequenced firstmate instruction records the worker acknowledges by moving them into its handled/ subdirectory; written by fm-send, re-rung and escalated by the watcher, removed by teardown (bin/fm-task-inbox-lib.sh)
<id>.meta task metadata; each producer script's header owns its exact fields and mutation contract, with docs/configuration.md routing operator-facing backend and trace-context details
<id>.herdr-presentation quarantinable attempt and restart-binding journal for Herdr's optional visual projection; never task or endpoint authority; see docs/herdr-backend.md "Presentation spaces"
completed-task-views/ opt-in bounded sanitized summaries and exact Herdr view identities retained after ordinary task records retire; see docs/herdr-backend.md "Completed task views"
<id>.check.sh authenticated slow poll; the watcher dispatches validated PR data and the byte-identified Relay shim through trusted repository scripts, runs registered custom checks from hash-validated private snapshots, and rejects every other state check without execution
<id>.check-trust private content binding created by fm-check-register.sh for an intentional custom check
<id>.pr-poll private validated data sidecar for the byte-static PR merge poll
Expand Down Expand Up @@ -241,7 +243,7 @@ Project creation never authorizes an unmentioned remote, and project removal nev

Load `secondmate-provisioning` before creating, seeding, validating, launching, handing backlog to, recovering, pushing inherited local material into, or retiring a secondmate home, and before editing `data/secondmates.md`.
Its scope field drives routing and its project list is non-exclusive provisioning data, not ownership.
Keep `local-only` work in the main home.
Keep `local-only` work in the main home, subject to section 7's mandatory PR path for tracked code.

A secondmate is idle by default and acts only on work routed by the main firstmate.
It reconciles its own work under way after restart, then waits silently; an empty queue never authorizes a survey, audit, or self-directed improvement sweep.
Expand Down Expand Up @@ -271,10 +273,13 @@ Resolve the project independently for every request.
An explicit project wins, a clear follow-up inherits its referent, and otherwise match the request against the registry, work under way, and project code or README.
Proceed on one confident match while naming the project in plain language; ask one concise question when multiple or no projects plausibly match.

Before routing or starting any task that will change tracked code, load `nathan-coding-loop`.
A second mate must coordinate separate implementation and independent review workers through one task PR until the reviewer explicitly reports no material findings on its current head; a missing viable second-mate or PR path is a blocker to escalate, and reviewer satisfaction never grants merge authority.

Route by the nature of the work against each registered secondmate scope, not by a non-exclusive clone list.
Keep `local-only` work in the main home.
Keep `local-only` work in the main home when it is eligible to proceed outside the tracked-code PR requirement.
Send in-scope work to the fitting secondmate unless it is blocked or the captain explicitly redirects it; do not read the secondmate's chat because marked routed replies return through its status or referenced document.
If no secondmate scope fits, use the main home or discuss creating an appropriate persistent secondmate.
If no secondmate scope fits, use the main home for work outside the mandatory tracked-code loop or discuss creating an appropriate persistent secondmate.
For one-off or infrequent operational work, start with the simplest direct end-to-end path.
Do not build wrappers, control planes, policy layers, custom verifiers, or automation unless the direct path exposes a concrete blocker or repeated need that justifies the added machinery.

Expand Down Expand Up @@ -319,9 +324,10 @@ Supervise all live work under section 8.
### Selected delivery path and merge authority

The selected delivery path owns its own rigor.
When no-mistakes is selected, no-mistakes alone owns review, fixes, tests, documentation, push, PR, and CI; otherwise follow the faster path without adding an independent reviewer.
Never hold work outside no-mistakes for a manual clean verdict, stack serial manual reviews, or infer authority for one from security, architecture, or risk alone.
A separate review or audit is allowed only when the captain explicitly requests that deliverable or the authorized task is a knowledge-only review; one named question remains scoped to that question.
For tracked code, the independent loop owned by `nathan-coding-loop` complements that path and is the only additional review layer: no-mistakes retains ownership of its automated review, fixes, tests, documentation, push, PR, and CI, while the faster path retains its ordinary delivery mechanics.
Outside that mandatory loop, follow the selected path without adding an independent reviewer.
Never hold work for another manual clean verdict, stack additional serial reviews, or infer authority for one from security, architecture, or risk alone.
A separate review or audit beyond the mandatory loop is allowed only when the captain explicitly requests that deliverable or the authorized task is a knowledge-only review; one named question remains scoped to that question.
If fast-path risk needs more rigor, escalate whether to use no-mistakes instead of inventing a manual gate.
The path's worker, automated gates, and captain approval remain authoritative:

Expand Down Expand Up @@ -365,14 +371,15 @@ The worker reports the PR when CI first becomes green rather than waiting for me

For PR-based ship tasks, the ready signal depends on mode: `no-mistakes` reports `done: PR <url> checks green` after CI is green, while `direct-PR` reports `done: PR <url>` after opening the PR.
Run `bin/fm-pr-check.sh <id> <PR url>` - it records `pr=` and the forge's `pr_head=` when available in the task's meta and arms the watcher's merge poll.
Tell the captain the PR's full URL, always the complete `https://...` link rather than a bare `#number`, a concise outcome summary, and the no-mistakes risk level when applicable.
After any mandatory `nathan-coding-loop` loop is clean for that exact current head, tell the captain the PR's full URL, always the complete `https://...` link rather than a bare `#number`, a concise outcome summary, and the no-mistakes risk level when applicable.
A captain instruction to merge is explicit authority; `yolo` is the only standing routine merge authority.
For any custom `state/<id>.check.sh` you write yourself, keep it an ordinary single-link mode-`0700` file, print one line only when firstmate should wake, print nothing otherwise, finish before `FM_CHECK_TIMEOUT`, then bind its current bytes with `bin/fm-check-register.sh <id>` before the watcher may execute it.

Tear down a ship task only after landing is confirmed.
A teardown refusal for uncommitted or unlanded work is a stop-and-investigate result, never an obstacle to bypass.
Never force teardown without explicit discard authority.
After successful teardown, record completion, retain only the configured recent Done history, and re-evaluate queued work whose blockers and time gates have cleared.
An explicitly enabled Herdr completed-task view is presentation only; `docs/herdr-backend.md` owns its bounded summary, exact dismissal, and unsupported-backend fallback contract.

A secondmate is persistent and an empty queue is healthy.
Retire one only on an explicit captain or main-firstmate decision, after loading `secondmate-provisioning`; its home must contain no work under way, and forced discard still requires explicit captain authority.
Expand Down Expand Up @@ -543,6 +550,7 @@ These skills are not captain-invocable; load them only at their precise triggers
Cloning or registering a project is add intake and uses the same trigger.
- `stuck-crewmate-recovery` - load when the session-start digest reports an ordinary direct report's endpoint dead or its metadata has no window, or after a stale wake, looping pane, repeated confusion, an answered-by-brief question, an unresponsive crewmate, or a failed steer.
- `secondmate-provisioning` - load before creating, seeding, validating, launching, handing backlog to, recovering, pushing inherited local material into, or retiring a secondmate home, and before editing `data/secondmates.md`.
- `nathan-coding-loop` - load before routing, starting, or coordinating any task that will change tracked code, and keep it loaded through its mandatory second-mate developer-reviewer loop.
- `captain-hold-lifecycle` - load before treating an investigation or visual review as complete, before ending a visual review that exposed a captain decision, when recording or routing the captain's answer, and on any `RECORD DIVERGENCE` line from the wake drain.
- `process-event-sources` - load before arming a long-polling source, before registering a deterministic condition->action watch (do X as soon as Y is true), and on any `procevent <adapter> <source-id> <sequence>` check wake.
Never run a registered source's blocking command yourself in a conversational turn.
Expand Down
Loading