Add head-keyed PR review and post-merge QA gates - #4
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@codex review |
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 PR adds an exact-head GitHub PR review ledger and watcher-backed review workflow, enforces conservative low/high-stakes merge gates with high-stakes attestations and a pre-merge head race check, and adds a post-merge browser QA or explicit non-browser verification gate before Ready for QA. Sequence diagram for exact-head PR review and mergesequenceDiagram
participant Agent
participant Review as fm-pr-review.sh
participant Snapshot as fm-pr-review-snapshot.sh
participant Ledger as Review ledger
participant Watcher as Existing watcher
participant Merge as fm-pr-merge.sh
participant GitHub
Agent->>Review: init task URL
Review->>Snapshot: collect complete review snapshot
Snapshot->>GitHub: read files, reviews, threads, checks
GitHub-->>Snapshot: head-stable snapshot
Snapshot-->>Review: exact head and review inputs
Review->>Ledger: create head generation and risk classification
Agent->>Review: arm
Review->>Watcher: register review-policy.check.sh
Watcher->>Review: poll checkpoint due
Agent->>Review: checkpoint URL
Review->>Snapshot: collect current snapshot
Snapshot->>GitHub: re-read review surfaces
GitHub-->>Snapshot: current head and checks
Snapshot-->>Review: checkpoint data
Review->>Ledger: record dispositions and checkpoint
Agent->>Review: merge task URL
Review->>Snapshot: final complete snapshot
Snapshot->>GitHub: verify current head
GitHub-->>Snapshot: reviewed head
Review->>Ledger: record merge decision and verified head
Review->>Merge: hand off expected reviewed head
Merge->>GitHub: merge with head verification
GitHub-->>Merge: merge result
State diagram for per-head review generationstateDiagram-v2
[*] --> Initialized: init
Initialized --> CheckpointDue: ten-minute checkpoint
CheckpointDue --> PendingRetry: pending reviewer or check
PendingRetry --> CheckpointDue: bounded retry
CheckpointDue --> ReviewCoverage: snapshot complete
ReviewCoverage --> NewGeneration: head changed
NewGeneration --> CheckpointDue: reset all evidence
ReviewCoverage --> HighStakesProofs: high-stakes risk
ReviewCoverage --> MergeReady: low-stakes gates pass
HighStakesProofs --> MergeReady: exact Fable 5.1 and independent review
MergeReady --> Merged: head reverified and guarded merge
Merged --> PostMergeQA: post-merge record required
PostMergeQA --> ReadyForQA: browser pass or non-browser N/A
PostMergeQA --> Blocked: failed smoke with owning bug
Blocked --> PostMergeQA: fresh verification
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e842fe12a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif grep -q "^no required checks reported on the '" "$TMP/checks.err"; then | ||
| printf '[]\n' > "$TMP/required.json" |
There was a problem hiding this comment.
When branch protection requires checks but none has reported yet, gh pr checks --required emits the matched no required checks reported error; the existing verified handling in bin/fm-pr-state.sh:113-121 explicitly treats this as readiness unconfirmed. Rewriting that condition to [] makes ready_check evaluate all([]) as true, so a PR can receive a merge decision before any required CI job starts. Preserve this state as pending or refuse the snapshot rather than treating it as no requirements.
AGENTS.md reference: AGENTS.md:L358-L359
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 84f75af. The collector now represents the GitHub unreported-required-check state as a pending required check, so readiness cannot treat it as an empty green set. The failing-first regression is in tests/fm-pr-review.test.sh.
| --slurpfile policy "$POLICY" ' | ||
| def pages($x): ($x[0] | add // []); | ||
| def external: select((.user.login // .author.login // "") != $author); | ||
| def check_pending: (.status != "COMPLETED"); |
There was a problem hiding this comment.
On repositories where a reviewer integration reports a classic commit status, statusCheckRollup contains a StatusContext with context and state rather than a check run with name and status (the distinction is already handled in bin/fm-pr-merge.sh:540-550). Here .status is null, so even a state:"SUCCESS" reviewer context always satisfies check_pending; if its context matches a configured marker, the ledger retries forever and readiness can never pass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 84f75af. Reviewer StatusContext entries now use their state, so SUCCESS, FAILURE, and ERROR are settled while pending states still retry. The live-collector regression covers a successful Sourcery status beside a pending Codex check.
| [ "$(jq -r '.generations[-1].merge_decision.decision // ""' "$LEDGER")" = merge ] \ | ||
| || die 'post-merge verification requires a recorded merge decision' | ||
| [ "$(jq -r '.generations[-1].merge_decision.verified_head // ""' "$LEDGER")" = "$HEAD" ] \ | ||
| || die 'post-merge verification head does not match the merge decision' |
There was a problem hiding this comment.
If the guarded merge fails after merge-decision is published—for example because of a last-second conflict, authentication failure, or reviewed-head race—the ledger still contains decision:"merge". These checks accept that intent as proof of landing, so post-merge can record an N/A result and ready-for-qa succeeds even though the PR remains unmerged. Bind this stage to a confirmed forge merge result or clear the decision when the merge command fails.
AGENTS.md reference: AGENTS.md:L363-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 84f75af. post-merge now reads the live pull request, requires merged=true with a valid merge commit SHA, records that forge SHA separately, and requires browser evidence to name the same merged SHA. The regression proves a recorded merge intent cannot authorize post-merge evidence while the pull request remains unmerged.
| ($evidence[0] + {recorded_at:$at,recorded_epoch:$epoch, | ||
| ready_for_qa:(if $evidence[0].applicability == "not-applicable" or $evidence[0].outcome == "passed" | ||
| then "allowed" else "blocked" end)}) as $record |
There was a problem hiding this comment.
After a browser verification fails and records an owning bug, another post-merge call for the same head can submit applicability:"not-applicable"; this unconditional assignment marks the new record allowed, and ready_for_qa_check consults only the last record. That lets the unchanged failed browser build pass the Ready for QA gate without the fresh passing verification required by .agents/skills/pr-review-policy/SKILL.md:82-85; reject an N/A record once that generation has browser evidence, or retain the failed block until a browser pass is recorded.
AGENTS.md reference: AGENTS.md:L365-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 84f75af. Once the current ledger generation contains browser evidence, post-merge refuses a later not-applicable record. The regression records a failed browser smoke with its owning bug and proves N/A cannot clear the block.
|
Review follow-up evidence for head 84f75af:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84f75af5c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| FORGE_MERGE_SHA=$(printf '%s\n' "$result" | jq -er ' | ||
| select(.merged == true) | ||
| | .merge_commit_sha | ||
| | select(type == "string" and test("^[0-9a-fA-F]{40}$")) | ||
| ') || die 'post-merge verification requires a forge-confirmed merge commit' |
There was a problem hiding this comment.
When merge-decision approves head A, the guarded merge refuses after a push to B, and B is subsequently merged externally, this query still accepts B's merge because it checks only .merged and .merge_commit_sha, never the response's .head.sha. Fresh evidence beyond the earlier unconfirmed-merge finding is this new API read: it proves that some head merged but does not prove it was A, so post-merge and Ready for QA can pass for content that never completed the recorded review generation. Require the forge-reported source head to equal the ledger head.
AGENTS.md reference: AGENTS.md:L365-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 53ff4e6. The forge merge read now requires the returned source head SHA to equal the reviewed ledger head before it accepts the merge commit SHA. The regression proves an externally merged replacement head cannot satisfy the earlier reviewed generation.
| (if $g.merge_decision.decision != "hold" then empty else "pull request is held: " + ($g.merge_decision.reason // "no reason recorded") end), | ||
| (if any($g.checkpoints[]; .head == $head and .at_epoch >= $g.not_before_epoch) then empty else "ten-minute review checkpoint has not completed on this head" end), | ||
| (if ($g.checkpoints | length) > 0 and ($g.checkpoints[-1].pending_reviews | length) == 0 then empty else "an explicitly pending review still needs bounded-backoff retry" end), | ||
| (if ($g.checkpoints | length) > 0 and all($g.checkpoints[-1].checks[]; .status == "COMPLETED" and (.conclusion == "pass" or .conclusion == "skipping")) then empty else "a required check is not green on this head" end), |
There was a problem hiding this comment.
When the captain explicitly authorizes one red check and the caller uses fm-pr-review.sh merge ... --allow-red <check>, this unconditional all-green test exits before those arguments reach fm-pr-merge.sh. Because the review wrapper is mandatory for GitHub merges, the documented single-check waiver is therefore unusable; readiness must exempt only the named check while continuing to require every other check to be green.
AGENTS.md reference: AGENTS.md:L359-L363
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 53ff4e6. The review wrapper now applies the one exact --allow-red check name during readiness, records that name in the merge decision, and still forwards the original arguments to the guarded merge parser. The regression proves lint can be waived while a second red unit check still blocks.
| SENSITIVE=$(jq -r ' | ||
| .[].filename | ||
| | select(test( | ||
| "(^|/)(auth|permissions?|secrets?|credentials?|migrations?|schema|payments?|billing|money)(/|\\.|$)"; |
There was a problem hiding this comment.
For small changes to common paths such as src/authentication.ts or src/authorization.go, the boundary immediately after auth prevents this regex from matching, so the classifier returns low and skips the exact-model and independent-review requirements. These are explicitly security-sensitive changes, so the matcher must recognize authentication and authorization names rather than requiring auth to be a complete path component or basename.
AGENTS.md reference: AGENTS.md:L358-L360
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 53ff4e6. The sensitive-path matcher now recognizes authentication and authorization names, including the alternate authorisation spelling, while retaining the existing auth component match. Failing-first cases cover src/authentication.ts and src/authorization.go.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53ff4e6a68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SENSITIVE=$(jq -r ' | ||
| .[].filename | ||
| | select(test( | ||
| "(^|/)(auth(entication|ori[sz]ation)?|permissions?|secrets?|credentials?|migrations?|schema|payments?|billing|money)(/|\\.|$)"; |
There was a problem hiding this comment.
A small migration under the conventional db/migrate/... path does not match migrations?, so it is classified as low stakes; for example, a one-file db/migrate/20260921_add_users.rb change returns level:"low". This contradicts .agents/skills/pr-review-policy/SKILL.md:36-40, which classifies schema/data migrations as high stakes, and lets such production data changes skip the exact-model and independent-review gates. Match the conventional migrate component as well.
AGENTS.md reference: AGENTS.md:L348-L349
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9e144fd. The sensitive-path matcher now recognizes migrate, migration, and migrations path components. A failing-first regression covers db/migrate/20260921_add_users.rb and requires a high-stakes result.
| jq -e --arg head "$head" --arg url "$URL" --arg task "$task" \ | ||
| --arg forge_merge_sha "$forge_merge_sha" ' |
There was a problem hiding this comment.
When an evidence file contains multiple JSON documents—for example, a generator appends a corrected record—jq -e emits one result per document and returns success when the last result is true, even if the first record is invalid. The subsequent --slurpfile evidence logic records $evidence[0], so an invalid first record followed by a valid one can be stored as ready_for_qa:"allowed", bypassing required reasons or browser safety fields. Require exactly one JSON object and validate the same element that is recorded.
AGENTS.md reference: AGENTS.md:L365-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9e144fd. post-merge now normalizes the input to exactly one JSON object in a private staged file, validates that staged object, and records the same staged object. The regression proves an invalid first document followed by a valid second document is rejected without adding a ledger record.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e144fd808
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| PR_REPO=$FM_PR_REPO | ||
| PR_NUMBER=$FM_PR_NUMBER | ||
| FM_PR_REVIEW_EXPECTED_HEAD=${FM_PR_REVIEW_EXPECTED_HEAD:-} | ||
| if [ -n "$FM_PR_REVIEW_EXPECTED_HEAD" ]; then |
There was a problem hiding this comment.
When a remaining direct caller invokes this script for GitHub with the normal unset environment, this conditional skips the only ledger handoff check, and the pre-existing authority/green-check path can merge without a review generation, checkpoint, dispositions, or high-stakes attestations. This remains reachable because bin/fm-branch-prompt.sh:103 and .agents/skills/bearings/SKILL.md:142 still direct callers to fm-pr-merge.sh; require a valid expected head for GitHub while preserving direct GitLab use, or update every route to the mandatory wrapper.
AGENTS.md reference: AGENTS.md:L363-L363
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 7e1d79a. Direct GitHub merges now refuse unless FM_PR_REVIEW_EXPECTED_HEAD carries a valid reviewed-head handoff from bin/fm-pr-review.sh merge; direct GitLab merges remain supported. Remaining GitHub routes and documentation now point to the review wrapper, with regression coverage for the refusal and GitLab exception.
| "(^|/)(auth(entication|ori[sz]ation)?|permissions?|secrets?|credentials?|migrat(e|ions?)|schema|payments?|billing|money)(/|\\.|$)"; | ||
| "i") or | ||
| test("(^|/)(deploy|production|infra|terraform|kubernetes|\\.github/workflows)(/|$)"; "i") or | ||
| test("(^|/)(fm-(spawn|teardown|control|watch|session|afk|merge|lease|recover)|backends?)([./-]|$)"; "i")) |
There was a problem hiding this comment.
For a small change to bin/fm-pr-review.sh or bin/fm-pr-merge.sh, this matcher misses the actual fm-pr-* basenames; a one-file, three-line fixture for either is classified low. Changes to these mandatory GitHub review and merge guards can therefore skip the exact-model and independent-review requirements, so include the concrete PR guard names and their policy surface in the high-stakes classification.
AGENTS.md reference: AGENTS.md:L363-L363
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 7e1d79a. The classifier now treats fm-pr-merge, fm-pr-review, fm-pr-review-snapshot, fm-pr-risk, and firstmate-review-policy.json as high-stakes surfaces. The focused test covers each concrete path.
| end | ||
| else false | ||
| end | ||
| ' "$file" >/dev/null || die 'post-merge evidence is incomplete, unsafe, or does not match the reviewed head' |
There was a problem hiding this comment.
When an evidence producer replaces the source file between the normalization read and this validation read, the replacement can pass validation while the earlier staged object is what lines 559-563 record and mark allowed. Fresh evidence beyond the earlier multi-document fix is that the new staging path still validates "$file" instead of "$normalized"; validate the immutable staged object so the checked and recorded evidence cannot diverge.
AGENTS.md reference: AGENTS.md:L365-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 7e1d79a. Post-merge validation now reads the immutable normalized staging file, which is also the object recorded in the ledger. A fake-jq source-swap regression proves that replacing the source after normalization cannot substitute different validation evidence.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e1d79ab6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "(^|/)(auth(entication|ori[sz]ation)?|permissions?|secrets?|credentials?|migrat(e|ions?)|schema|payments?|billing|money)(/|\\.|$)"; | ||
| "i") or | ||
| test("(^|/)(deploy|production|infra|terraform|kubernetes|\\.github/workflows)(/|$)"; "i") or |
There was a problem hiding this comment.
For small changes to conventional paths such as openapi.yaml or k8s/deployment.yaml, none of these patterns match, so the bounded-change branch classifies them as low. The review policy explicitly excludes public-interface and production-infrastructure changes from low stakes, meaning these changes incorrectly skip the exact-model and independent-review requirements; extend the sensitive matcher to cover these conventional surfaces.
AGENTS.md reference: AGENTS.md:L348-L349
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bf91fbb. The high-stakes matcher now covers conventional public API surfaces (api, OpenAPI, Swagger, and proto) and production infrastructure paths including k8s, Kubernetes, Helm, charts, deployments, Terraform, CloudFormation, Pulumi, and Ansible. Focused fixtures include openapi.yaml and k8s/deployment.yaml.
| --slurpfile rollup "$TMP/rollup.json" \ | ||
| --slurpfile policy "$POLICY" ' | ||
| def pages($x): ($x[0] | add // []); | ||
| def external: select((.user.login // .author.login // "") != $author); |
There was a problem hiding this comment.
When the PR author differs from the authenticated maintainer posting the required final-disposition evidence—for example, on an external contributor's PR—this predicate treats that maintainer's new disposition post as reviewer input. The mandatory fresh snapshot taken by fm-pr-review.sh merge then discovers an undispositioned item and rejects the documented post-bind-merge sequence; identify and exclude the authenticated posting actor in addition to the PR author.
AGENTS.md reference: AGENTS.md:L363-L363
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bf91fbb. Snapshot collection now reads the authenticated GitHub actor and excludes both that actor and the PR author across top-level comments, submitted reviews, and inline-thread content. The external-contributor fixture proves a maintainer final-disposition post and inline reply do not become reviewer input.
| def posted_url: | ||
| text and (startswith($url + "#") or startswith($url + "/files") or startswith("https://linear.app/")); |
There was a problem hiding this comment.
For a passing browser verification, values such as https://linear.app/ or the PR URL followed by an arbitrary fragment satisfy posted_url, after which the record is marked ready_for_qa:"allowed". Those URLs do not prove that the required screenshot and verification evidence were posted anywhere, so an incomplete evidence handoff can clear the Ready for QA gate; require a concrete Linear issue URL or recognized GitHub comment/files anchor.
AGENTS.md reference: AGENTS.md:L365-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bf91fbb. Posted evidence now requires a concrete PR issue comment, discussion, review, anchored files diff, or Linear issue URL. Regressions reject the Linear root, arbitrary PR fragments, and bare files URLs while accepting a concrete Linear issue.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf91fbb5a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def reviewer_login: . != "" and . != $author and . != $actor; | ||
| def external: select((.user.login // .author.login // "") | reviewer_login); |
There was a problem hiding this comment.
On an external contributor's PR, the authenticated maintainer may also be a genuine reviewer; excluding $actor here removes all of that maintainer's top-level comments, submitted reviews, and inline feedback, so the ledger can merge without dispositions for those findings. Fresh evidence beyond the earlier final-disposition fix is that the exclusion is actor-wide across every review surface rather than limited to the bound final-disposition post.
AGENTS.md reference: AGENTS.md:L348-L349
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6ad6d93. This corrects the actor-wide exclusion introduced in bf91fbb. The snapshot now retains every genuine maintainer comment, submitted review, and inline reply; checkpoint application excludes only the authenticated actor's exact URL already bound as the ledger final-disposition evidence. The regression proves a real maintainer finding remains disposition-required while only the exact final post is omitted.
| [ "$#" -ge 2 ] || die 'merge requires task id and pull-request URL' | ||
| TASK=$1; parse_url "$2"; shift 2 | ||
| fm_pr_task_id_valid "$TASK" || die 'invalid task id' |
There was a problem hiding this comment.
When this command is given task B for a ledger initialized for task A, it validates only that B is syntactically valid and then passes B to fm-pr-merge.sh; the downstream hold, away-grant, metadata, and authority checks therefore run against B rather than the task whose review ledger authorized the PR. A mistaken or stale task ID can consequently bypass A's unresolved human hold and attach the merge outcome to the wrong task, so require .task == $TASK before recording the merge decision.
AGENTS.md reference: AGENTS.md:L358-L358
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6ad6d93. The merge wrapper now requires its task ID to equal the task recorded in the PR ledger before it records a merge decision or calls the guarded merge command. A focused regression proves a mismatched task cannot create a merge decision.
| test("(^|/)\\.github/workflows(/|$)"; "i") or | ||
| test("(^|/)(fm-pr-(merge|review|review-snapshot|risk))([./-]|$)"; "i") or | ||
| test("(^|/)firstmate-review-policy[.]json$"; "i") or | ||
| test("(^|/)(fm-(spawn|teardown|control|watch|session|afk|merge|lease|recover)|backends?)([./-]|$)"; "i")) |
There was a problem hiding this comment.
A small change to bin/fm-captain-hold.sh does not match this lifecycle list and is therefore classified as low stakes, even though that script controls whether unresolved human decisions continue to block merges. Such a change can alter the merge-authorization boundary while skipping the configured exact-model and independent-review requirements; include the captain-hold surface in the sensitive matcher.
AGENTS.md reference: AGENTS.md:L358-L360
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6ad6d93. bin/fm-captain-hold.sh is now part of the high-stakes lifecycle matcher, with a concrete classifier fixture alongside the PR review and merge guard fixtures.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ad6d93791
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| test("(^|/)(api|openapi|swagger|proto)([./-]|$)"; "i") or | ||
| test("(^|/)(deploy(ment)?s?|production|infra(structure)?|terraform|kubernetes|k8s|helm|charts?|cloudformation|pulumi|ansible)([./-]|$)"; "i") or | ||
| test("(^|/)\\.github/workflows(/|$)"; "i") or | ||
| test("(^|/)(fm-pr-(merge|review|review-snapshot|risk))([./-]|$)"; "i") or |
There was a problem hiding this comment.
A small change to bin/fm-pr-lib.sh does not match this expression and is classified as low stakes, even though both fm-pr-review.sh:66 and fm-pr-merge.sh:123 source that library for canonical PR identities, task IDs, and reviewed-head validation. Fresh evidence beyond the earlier guard-name fix is this shared dependency omitted from the enumerated guard surfaces; changes to it can therefore weaken both mandatory merge entrypoints while skipping the exact-model and independent-review gates. Include fm-pr-lib.sh in the sensitive matcher.
AGENTS.md reference: AGENTS.md:L348-L348
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cdc3b6c. The shared bin/fm-pr-lib.sh identity and validation dependency is now explicitly classified high stakes, with a focused classifier fixture.
| jq --arg kind "$KIND" --arg subject "$SUBJECT" --arg evidence "$EVIDENCE" --arg head "$HEAD" --arg at "$(now_iso)" ' | ||
| .generations[-1].attestations += [({kind:$kind,head:$head,evidence:$evidence,at:$at} | ||
| + if $kind == "no-mistakes" then {model:$subject} else {actor:$subject} end)] | ||
| | .generations[-1].merge_decision = (if .generations[-1].merge_decision.decision == "merge" then null else .generations[-1].merge_decision end)' "$LEDGER" > "$WORK" |
There was a problem hiding this comment.
When final-disposition is recorded before the high-stakes proofs, a later attest call appends the no-mistakes or independent-review evidence but clears only merge_decision; ready_check also omits attestations from the final-disposition digest. Consequently, a late exact-model attestation or a no-findings independent review can make the generation merge-ready while the bound final post predates that evidence. Clear final_disposition when attestations change, or include attestations in its state digest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cdc3b6c. Every attestation change now clears final_disposition, and the bound state digest now includes normalized attestation evidence. The regression records final disposition before the exact-model proof, verifies the late proof invalidates it, and requires a fresh final post before readiness.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdc3b6c32e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| test("(^|/)(fm-pr-(lib|merge|review|review-snapshot|risk))([./-]|$)"; "i") or | ||
| test("(^|/)firstmate-review-policy[.]json$"; "i") or | ||
| test("(^|/)(fm-(spawn|teardown|control|watch|session|afk|merge|lease|recover|captain-hold)|backends?)([./-]|$)"; "i")) |
There was a problem hiding this comment.
A small change to .agents/skills/pr-review-policy/SKILL.md is classified as low stakes because this matcher covers the policy JSON and scripts but not the skill that defines the mandatory review, merge, and post-merge procedure. Such a change can weaken the exact-model, independent-review, or evidence gates while skipping those same high-stakes checks; include the review-policy skill in the sensitive surfaces.
AGENTS.md reference: AGENTS.md:L348-L348
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in a3c0a0e: the review-policy skill is now a high-stakes surface, with regression coverage.
| [ "$#" -eq 3 ] || die 'final-disposition requires URL, head, and posted evidence' | ||
| parse_url "$1"; HEAD=$2; EVIDENCE=$3 | ||
| [ -n "$EVIDENCE" ] || die 'posted disposition evidence must not be empty' |
There was a problem hiding this comment.
When final-disposition receives a typo, a deleted post URL, or any arbitrary nonempty string, the fresh merge snapshot filters nothing and the stored digest can still match, so readiness permits the merge without the required final disposition ever being present on the PR. Require a concrete URL on this PR and have the fresh snapshot prove that the matching post by the collector exists before treating the disposition as bound.
AGENTS.md reference: AGENTS.md:L348-L348
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in a3c0a0e: final-disposition evidence must be a concrete post URL on the same pull request, starts unobserved, and becomes merge-ready only when a fresh snapshot finds that exact URL authored by the collector. Regression coverage includes invalid URLs and deleted or missing posts.
| def bug_url: | ||
| text and (startswith("https://linear.app/") or test("^https://github[.]com/[^/]+/[^/]+/issues/[0-9]+($|#)")); |
There was a problem hiding this comment.
When a failed browser verification supplies bug.url as https://linear.app/ or any unrelated Linear path and bug.action as created or reopened, this predicate accepts and records it as the owning bug. The Ready for QA refusal then names a non-actionable URL even though the required bug was never evidenced; tighten this separate bug_url predicate to require a concrete Linear issue URL. This remains after the earlier posted_url fix because that validation does not constrain bug.url.
AGENTS.md reference: AGENTS.md:L365-L365
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in a3c0a0e: failed-smoke bug evidence now accepts only concrete Linear issue or GitHub issue URLs, with regressions for the Linear root and unrelated paths.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3c0a0e251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --argjson epoch "$epoch" --argjson not_before "$not_before" --slurpfile risk "$risk" ' | ||
| .current_head as $old_head | ||
| | .generations[-1].merge_decision as $prior_decision | ||
| | .task = (if $task == "" then .task else $task end) |
There was a problem hiding this comment.
When init is rerun for an existing PR after its head changes, this assignment silently replaces task A with the caller-supplied task B. Fresh evidence beyond the earlier merge-argument fix is that its equality check trusts this mutable field, so a stale task ID can make the wrapper and downstream metadata, authority, and captain-hold checks run against B while bypassing A's task-level hold and recording the outcome on the wrong task. Reject an init task mismatch instead of rewriting .task.
AGENTS.md reference: AGENTS.md:L358-L360
Useful? React with 👍 / 👎.
| url:$url, | ||
| head:$head, | ||
| collector_actor:$actor, | ||
| files:(pages($files) | map({filename,status,additions,deletions})), |
There was a problem hiding this comment.
For a renamed sensitive file, GitHub supplies previous_filename, but this normalization discards it and the risk classifier examines only the new name. A small rename such as src/authentication.ts to src/helpers.ts is therefore classified low stakes even though it removes or relocates an authentication surface, allowing the change to skip the exact-model and independent-review gates. Preserve and classify both paths for renamed entries.
AGENTS.md reference: AGENTS.md:L348-L349
Useful? React with 👍 / 👎.
| SENSITIVE=$(jq -r ' | ||
| .[].filename | ||
| | select(test( | ||
| "(^|/)(auth(entication|ori[sz]ation)?|permissions?|secrets?|credentials?|migrat(e|ions?)|schema|payments?|billing|money)(/|\\.|$)"; |
There was a problem hiding this comment.
For conventional filenames such as src/payment-service.ts, src/secret-store.go, or src/permissions_helper.rb, the matcher requires /, ., or end-of-string immediately after the sensitive stem, so these changes are classified low stakes. Fresh evidence beyond the earlier authentication-name fix is that hyphenated and underscored money, secret, and permission surfaces still bypass the high-stakes review gates; accept conventional - and _ separators here as the neighboring API and infrastructure patterns do.
AGENTS.md reference: AGENTS.md:L348-L349
Useful? React with 👍 / 👎.
| "i") or | ||
| test("(^|/)(api|openapi|swagger|proto)([./-]|$)"; "i") or | ||
| test("(^|/)(deploy(ment)?s?|production|infra(structure)?|terraform|kubernetes|k8s|helm|charts?|cloudformation|pulumi|ansible)([./-]|$)"; "i") or | ||
| test("(^|/)\\.github/workflows(/|$)"; "i") or |
There was a problem hiding this comment.
For a small .github/CODEOWNERS change, none of the sensitive-path predicates match, so the classifier returns low stakes even though the file controls review ownership and can alter required code-owner approval coverage. This lets a permissions-sensitive review-policy change skip the configured exact-model and independent-review requirements; include conventional CODEOWNERS locations in the high-stakes matcher.
AGENTS.md reference: AGENTS.md:L348-L349
Useful? React with 👍 / 👎.
* 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
…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
…'s Git common directory (#8) * 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 * fix(spawn): bind worker pool allocations to clone custody * no-mistakes(ci): Updated the verified CI Treehouse pin from v2.0.1 to v2.3.0 with official platform checksums. The Herdr failures were caused by v2.0.1 lacking the required `--root` capability. Verified installer download/checksum/version, `--root` support, lint, clone-custody regression, and dispatch-resolve regression. The portable failure was an unrelated transient broken-pipe race in unchanged code and passed locally * no-mistakes(ci): Fixed the flaky broken-pipe failure in bin/fm-quota-axi-lib.sh by replacing the private process-substitution lookup with a direct case mapping. This preserves all provider mappings while preventing an early consumer exit from closing the producer pipe and leaking `printf: write error: Broken pipe` to stderr. Verified with tests/fm-dispatch-resolve.test.sh, bin/fm-lint.sh, and git diff --check; all passed * no-mistakes(review): Move pool root outside homes; drop fork fixtures * no-mistakes(document): Point architecture doc at real Treehouse custody regression * no-mistakes(ci): ci-1 (Behavior portable serial 8): tests/fm-tangle-guard.test.sh still expected the old `treehouse get --root '<root>'` command, but this PR sends `treehouse --root '<root>' get` (bin/fm-spawn.sh:4049; `--root` is a global Treehouse flag, so both orders are valid). Updated the test to expect the new order; no production code changed. The failure reproduced locally before the fix and the script exits 0 after it. No other test, doc or script uses the old order. ci-2 (PR must be raised via no-mistakes): attestation failure because the pipeline's required `test` step is skipped (the Test agent timed out and the re-run was declined). Not caused by the code; the outer pipeline must re-run and complete the test step --------- Co-authored-by: Firstmate Crew <crew@firstmate.local>
…mary landing (#26) * 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 * fix(spawn): bind worker pool allocations to clone custody * no-mistakes(ci): Updated the verified CI Treehouse pin from v2.0.1 to v2.3.0 with official platform checksums. The Herdr failures were caused by v2.0.1 lacking the required `--root` capability. Verified installer download/checksum/version, `--root` support, lint, clone-custody regression, and dispatch-resolve regression. The portable failure was an unrelated transient broken-pipe race in unchanged code and passed locally * no-mistakes(ci): Fixed the flaky broken-pipe failure in bin/fm-quota-axi-lib.sh by replacing the private process-substitution lookup with a direct case mapping. This preserves all provider mappings while preventing an early consumer exit from closing the producer pipe and leaking `printf: write error: Broken pipe` to stderr. Verified with tests/fm-dispatch-resolve.test.sh, bin/fm-lint.sh, and git diff --check; all passed * feat(secondmate): seed local-only projects as bound child clones with primary-owned landing A local-only project has no forge, so a secondmate home could not hold one at all: bin/fm-home-seed.sh refused it and the routing prose sent that work back to the primary. Seed it instead as an independent local clone of the primary's own clone, pinned to its current default-branch commit, with no origin, no publication remote, no borrowed object storage and no no-mistakes initialization, recorded by a durable versioned binding inside the existing seed transaction. Fleet sync keeps skipping it and the whole-home remote route still refuses it. Custody splits along the same line the design drew. The child keeps its task, branch, worktree and endpoint; the landing stays with the primary that seeded the copy. bin/fm-local-handoff.sh offer publishes an immutable head-pinned offer carrying the commit as a git bundle, and the existing guarded entrypoint bin/fm-merge-local.sh consumes it as a pinned delegated input under its own per-task control lock, incarnation recheck and captain-hold check, rather than gaining a second acceptance system. No worker record is read, written or invented for the child. The primary alone fast-forwards its local default branch, then publishes a landing receipt into the child home. Only that receipt opens ordinary teardown, and bin/fm-teardown.sh re-proves the receipt's commit is still contained in the primary's default branch before accepting it; a child-local merge or a branch pushed anywhere is not that proof. Receipt recovery after a landing whose acknowledgement failed is idempotent and never merges. Missing or stale identities, dirty or diverged work, a changed head, a changed route, a damaged record and an interrupted transaction all refuse and preserve the work. tests/fm-local-handoff.test.sh drives the real scripts against isolated temporary homes over ten cases covering the bound seed, the two seed refusals that remain, the child's inability to land its own clone, offer pinning and republication, the guarded delegated landing and its receipt, the unpinned and stale approval refusals, idempotent receipt recovery, the teardown gate and the fail-closed record parsing, plus a held landing row blocking the landing. The obsolete refusal case in tests/fm-secondmate-safety.test.sh is removed with the behavior it asserted; the unchanged whole-home remote refusal stays covered by tests/fm-remote-secondmate-lifecycle-e2e.test.sh. Test inventory entries are additive only. * fix(secondmate): pin local-only landings to a parent-owned approval record The review found that an approval released by the captain could be inherited by any later child head, that a receipt could be satisfied by a substituted clone, that a refused landing left an imported ref behind, and that an absent worktree skipped the receipt gate entirely. Add one durable record, fm-local-landing.v1, written only by the new bin/fm-local-handoff.sh request subcommand while the captain's row is still held, and require the delegated landing to match that record's pinned offer, head, and identity. The landing guard now also refuses an unreadable hold status, a record already marked landed, and a project that has left local-only custody, and deletes its private import ref on every refusal path. The receipt proof derives the containment repository from the child's own parent route and project binding and additionally requires the parent's own landed record, so a receipt naming another clone proves nothing. Cleanup of a bound local-only task now faces that gate even when its worktree is already gone. * fix(secondmate): make a published local-only landing pin immutable A request could publish its landing record after the captain's row had already been released, so an answer given for one head was inherited by another. The pin is now published create-only, and the whole check, publication, and re-read of the row runs under the landing's existing per-landing control lock, which bin/fm-merge-local.sh and bin/fm-captain-hold.sh already take. A record that exists is reported rather than replaced: the identical identity repeats it, a different head refuses, and a landed record refuses outright. A row released outside that lock withdraws this call's own record byte for byte. Each approval therefore owns its own landing row; a moved head needs a new row rather than a re-pin. * test(secondmate): prove the answer waits on the pin's own lock The case that covered a captain's answer overlapping a landing pin in flight asserted only that the answer had not completed after a fixed three-second window. That assertion passes whenever the answer has simply not finished yet, so on a host where an uncontended release already costs more than three seconds it would have passed with the serialization removed entirely. Replace it with positive evidence. The fixture wrapper that freezes a publication now records the publishing process's pid, and the case asserts that the landing's own control lock is held by that process, or an ancestor of it, while the answer is running. The absence window stays as independent corroboration but is now scaled to a baseline the case measures on this host with the same command on its own row, and the boundary at the release instant plus the row's state after the answer completes are checked too. The frozen wrapper also ends with the case that installed it, so a case that fails inside its own window no longer leaves a publication spinning behind it. With the request's lock acquisition removed from bin/fm-local-handoff.sh the case now fails at that assertion rather than at a timer. * no-mistakes(review): Close landing rows after receipts; align routing and receipt checks * no-mistakes(review): Keep receipt recovery idempotent after landing row archival * no-mistakes(review): Refuse receipt recovery before writing when landing row missing * no-mistakes(review): Gate every recovery write on a present, unheld landing row * no-mistakes(review): Drop import refs on every exit; fail broken landing fixtures * no-mistakes(document): Align seeding docs with bound local-only secondmate clones * no-mistakes(ci): ci-2 (Behavior portable serial 8), fixed. tests/fm-gotmp.test.sh failed with "teardown exited non-zero with a valid tasktmp". Invariant: a test that runs the real bin/fm-teardown.sh from a fake bin folder must provide every library teardown loads. This PR made teardown load bin/fm-local-handoff-lib.sh, but the test's two fake bin folders (make_fake_root and the inline copy near line 170; the third case reuses make_fake_root) never got it, so teardown exited at startup. I reproduced this locally. No other test in tests/ links teardown into a fake folder, and the library's own dependencies (fm-secondmate-parent-lib.sh, fm-secondmate-registry-lib.sh) were already linked. Fix: link fm-local-handoff-lib.sh in both folders, with a comment matching the file's style. No production code changed. Verified: bash tests/fm-gotmp.test.sh passes all 3 cases and shellcheck is clean. ci-1 (Behavior portable serial 2), not caused by this PR. tests/fm-remote-secondmate-lifecycle-e2e.test.sh printed ALL TESTS PASSED, then exited 1 only because its cleanup rm -rf hit "Directory not empty" while a leftover background process was still writing. This PR doesn't touch that test or the watcher/remote code it runs. The same cleanup failure hit unrelated branch fm/fm-opencode-2-adapter (run 36213627355), so the test was already flaky. A local run on this loaded host (load average about 8.5) also failed: it hit the watcher's 30-second relaunch time limit, then the same cleanup failure. Making it reliable means finding which leftover process keeps writing, which is separate work outside this change. ci-3 (PR must be raised via no-mistakes), not caused by the code. The attestation check failed because the pipeline's test step had status=skipped, which depends on the pipeline run's state * Guard bound local-only landing by offered head and call identity * no-mistakes(review): Accept defer-then-release pins and tolerate deleted task branches * no-mistakes(review): Accept legacy date-only answered stamps for pinned landings * no-mistakes(document): Sync hold-stamp and teardown branch docs with fixes
1 participant
Summary
This is a non-browser change.
Tracking: https://linear.app/testawerawr/issue/TES-79
Validation
Summary by Sourcery
Enforce head-keyed GitHub pull-request review and post-merge QA gates while preserving safe, auditable merge and Ready for QA workflows.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: