Skip to content

brief: demand an independent review, not a command a crew cannot run - #54

Merged
webjema merged 1 commit into
mainfrom
fm/brief-review-fallback-r7
Aug 10, 2026
Merged

webjema merged 1 commit into
mainfrom
fm/brief-review-fallback-r7

Conversation

@webjema

@webjema webjema commented Aug 10, 2026

Copy link
Copy Markdown
Owner

brief: demand an independent review, not a command that may not exist

The scaffold every crewmate receives told it to Run /code-review and Run /verify.
A crewmate on the claude harness can run neither.
The gate therefore failed in the flattering direction: nothing stopped a crew from reporting a review it never obtained, and it surfaced twice only because the crew volunteered it (2026-08-03 prioritized-metrics, 2026-08-09 optiroq PR kunchenguid#942).

Everything below was established by execution on this box, not by reading a frontmatter field.

Why naming a command is unsafe even where it runs

code-review is two different commands sharing one name, and which one a crew gets is environment-dependent:

built-in code-review marketplace plugin code-review
reviews the working diff an already-open pull request (allowed-tools are all gh pr *)
disable-model-invocation true - a crew cannot invoke it false - a crew can

The plugin is not enabled on this box (~/.claude/settings.json enabledPlugins is {"provider@claude-provider-plugin": false}; ~/.claude.json enabledPlugins is null - a marketplace clone on disk is not an installed plugin).
But precedence is determinable, and it goes the wrong way: resolver VNy in the claude binary replaces a same-named entry when the existing one is gated, the new one is ungated, and the new one is type === "prompt", and jFe() returns "on" unconditionally for source === "plugin".
So on a box with that plugin enabled, the plugin wins.
The word /code-review in the scaffold would then silently mean "review the PR" - at a step where no PR exists yet, so it reviews nothing and returns clean.

This is the reason harness-agnostic wording is right rather than merely convenient: a named command is not just unavailable here, it is ambiguous everywhere.

The current scaffold text is correct on ZERO verified harnesses

harness installed here /code-review evidence
claude yes, 2.1.220 exists, GATED Skill(code-review) -> cannot be used with Skill tool due to disable-model-invocation. Control: Skill(firstmate-coding-guidelines) and Skill(harness-adapters) both succeeded, so the refusal is specific
opencode yes, 1.18.4 absent entirely grepped the whole bundle for code-review: zero hits; no ~/.config/opencode/{command,skills}
codex no /<skill> is rejected as "Unrecognized command"; the sigil is $<skill> harness-adapters, verified 2026-06-11
pi no no verified form harness-adapters
grok no user-level skills via /<skill>; a Claude Code built-in is not one harness-adapters

/verify is gated identically - the brief's premise died

The task brief said "/verify and project skills are ordinary model-invocable skills and are unaffected - only the built-in /code-review is gated. Do not change the /verify step."
Executed: Skill(verify) returns the identical refusal.
Both are Claude Code built-ins (built-in set in the binary: verify, pr, commit, code-review, simplify, go).
Escalated rather than silently widening scope.
Firstmate ruled that both steps get the same wording, from the quality direction's "root cause, never symptom": /verify is the same defect as /code-review, not a separate one, so it moves with it.
That call was not put to the captain, and it is firstmate's, not theirs - recorded here so it stays open to challenge rather than reading as settled authority.

The gate is "not unless the USER typed it this turn", not "never"

Read out of the binary, not inferred:

if (e.disableModelInvocation && !userTypedThisTurn) return {reason:"disable_model_invocation", ...}

No user setting rescues it: skillOverrides can only force a skill off, never past user-invocable-only.

Option (c) - firstmate types /code-review into the crew's composer via bin/fm-send.sh - is real and is rejected.
It works on the claude harness only, it adds a supervision round-trip to every crew's review, and it still cannot disambiguate the two same-named commands above.
Recorded as considered-and-rejected in data/learnings.md so nobody re-proposes it.

What the scaffold now demands

The outcome, never a command. Both modes, both steps:

  • obtain an INDEPENDENT review of the diff by whatever mechanism the harness actually provides (a review subagent handed the diff works everywhere and is always acceptable);
  • it must be a fresh reader over the diff, not the crew re-reading its own work;
  • report it as reviewed by: <mechanism> - <what it found> on the status line the crew already writes.

The disclosure is the load-bearing half, and it is deliberately two-part.
A mechanism alone is unfalsifiable - "reviewed by: review subagent" is textually identical whether or not the review ran, and the brief supplies that answer two lines earlier.
The outcome half is checkable: firstmate reads the same diff independently, so no findings on a diff with obvious defects is the tell that no review ran.
AGENTS.md's "Review and ship" now says a report missing that clause is sent back rather than reviewed on top of.

The gate is built once by review_gate() and interpolated into both delivery-mode DODs; only the line the crew reports it on differs. Two copies of one contract drift the moment only one is edited.

data/learnings.md 2026-08-03 - corrected in place

Verdict: right but incomplete, and both imprecisions are load-bearing. It is gitignored, so the correction is not in this diff.

  • Its mechanism claim (disable-model-invocation, user-typed only) is CONFIRMED by execution.
  • It said "only the built-in /code-review is gated". Wrong: /verify is gated identically.
  • It never mentioned the name collision or that precedence is environment-dependent - the finding that makes naming a command unsafe even where it runs.

A wrong learning is laundered into every future brief, which is why this correction was in scope.

Sites changed

  • bin/fm-brief.sh - both DOD steps 2 and 3, both delivery modes; the header's per-mode contract and a rationale block explaining why no command is named (the header owns that contract); the new shared review_gate().
  • AGENTS.md - the judgment layer (section 5) and "Review and ship" (section 6): the crew's own review is now described by outcome, and firstmate is told to bounce a report that carries no reviewed by: clause and to judge the outcome half against the diff it reads anyway.
  • bin/fm-promote.sh - a scout's brief is never regenerated on promotion, so the promote send-template had to carry the gate itself. Without it, a promoted crew could not satisfy the new AGENTS.md rule and would be bounced forever.
  • bin/fm-hooks-install.sh - header comment named the un-invokable command.
  • .agents/skills/harness-adapters/SKILL.md - the claude and grok rows used /code-review as their skill-invocation example, which is exactly the belief this PR is correcting. Both now carry the verified caveat.
  • CONTRIBUTING.md - reversed an earlier "leave". It stated as fact that this repo's quality gate includes /code-review, which is false for an agent contributor. Trimmed to a genuine one-line cross-reference rather than a second copy of the contract.
  • docs/proposals/context-management.md - reversed an earlier "leave". The doc is Status: draft for sign-off, i.e. live and about to be acted on, so a false statement in it would be built on.
  • tests/fm-brief.test.sh - two new tests.

Sites deliberately left

  • local-only step 3 carries no disclosure requirement. The PR-mode step says "state in the PR body how you exercised it"; local-only has no PR body, so there is nowhere to state it. The asymmetry is deliberate and only affects the end-to-end exercise. The load-bearing review disclosure is present in both modes.
  • No historical document was rewritten. Only docs/proposals/context-management.md was touched, and only because it is live.

Verification

  • Both new tests fail before the change and pass after (git stash push bin/fm-brief.sh -> not ok - PR DOD lost the self-review step).
  • test_review_gate_is_obtainable_and_disclosed runs over both heredocs and asserts the emitted scaffold carries the new instruction, the two-part disclosure, and assert_no_grep 'code-review' / assert_no_grep '/verify' - so the old text cannot come back on either branch.
  • test_review_gate_is_identical_in_both_modes pins the review_gate() extraction: the local-only gate must stay a line-subset of the PR gate, so the two DODs cannot drift.
  • bin/fm-lint.sh clean; bin/fm-test.sh (the single owner CI runs) green.
  • Both mode briefs scaffolded with the real script and read by eye.
  • No consumer breakage: bin/fm-classify-lib.sh:62 is verb-anchored, and tests/fm-prepush-review.test.sh matches by substring.

The scaffold every crewmate receives told it to run /code-review and
/verify. A crewmate on the claude harness can run neither: both are
Claude Code built-ins carrying disable-model-invocation, and the gate
is `if (disableModelInvocation && !userTypedThisTurn) refuse` - a crew
never has a user-typed turn. Verified by execution (Skill(code-review)
and Skill(verify) both refused; Skill(firstmate-coding-guidelines) and
Skill(harness-adapters) succeeded in the same session, so the refusal
is specific). The gate failed in the flattering direction: nothing
stopped a crew from reporting a review it never obtained.

Naming a command is unsafe even where one runs. `code-review` is two
different commands sharing a name: the built-in reviews the working
diff and is gated, while the official marketplace plugin reviews an
already-open pull request and is not. The plugin is not enabled here,
but the binary's resolver replaces a gated entry with an ungated
prompt-type one, so where it IS enabled it wins - and at review-ready
time no PR exists yet, so it would review nothing and return clean.

Across every harness checked the old text was wrong: absent entirely
from opencode 1.18.4, rejected by codex (its sigil is $<skill>), and
unverifiable on pi. It was correct on zero verified harnesses.

The scaffold now demands the outcome instead of a command: an
independent review of the diff by whatever mechanism the harness
provides, reported as `reviewed by: <mechanism> - <what it found>`.
The outcome half is what makes it checkable - firstmate reads the
same diff independently, so "no findings" on a diff with obvious
defects is the tell that no review ran. A mechanism alone is
unfalsifiable. The gate is built once by review_gate() and
interpolated into both delivery modes, so the two cannot drift.

Carried through every stale restatement: AGENTS.md's judgment layer
and "Review and ship", the fm-brief.sh header that owns the per-mode
contract, fm-hooks-install.sh's comment, CONTRIBUTING.md, the live
context-management proposal, and the harness-adapters skill, whose
claude and grok rows used the un-invokable command as their example.
fm-promote.sh's send-template gains the gate too: a scout's brief is
never regenerated on promotion, so a promoted crew could otherwise
never satisfy the new rule.
@webjema
webjema merged commit 8503816 into main Aug 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant