diff --git a/contract/schema/SOURCE.md b/contract/schema/SOURCE.md index 39123babdc..713467f761 100644 --- a/contract/schema/SOURCE.md +++ b/contract/schema/SOURCE.md @@ -9,7 +9,7 @@ with this note until todo 44 removed its source; the copies here are the ones th | acknowledgement.json | 193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c | | completion-receipt.json | fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5 | | delivery-attempt.json | ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b | -| relationship.json | c8ebaf4559fa1ac6df26d98c4214938caf78bd8c61659bce026da6a3595a4b90 | +| relationship.json | 6a994cad6400a07d3e2faaf2161a16818ac7784be651cdcb2d6c9b9439988cbd | | verification-verdict.json | 0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9 | Contract bundle revision c37d332e2daba95c9ef47adf00a82bc9c6a539ab62538f0ad857561e470d2989. @@ -32,3 +32,14 @@ on a `ready_for_review` receipt. Old receipts without it still validate. The 18 were re-judged against the revised bytes with jsonschema 4.19.2 Draft7Validator and kept their verdicts; 18 additional recorded cases cover the item stated, absent, on an execution-only receipt and malformed. This changes the stored JSON contract, not the SQLite schema. + +The relationship copy now allows one more `generations[].reason`: `accepted_result_correction`, +the reason a coordinator gives `generation-open` when it corrects a result that was accepted and is +still current (the relay's verdict writer refuses a second ruling on an accepted head, so the +generation is opened by hand and its reason is what notes the route). The enum previously allowed +only `initial_assignment` and `needs_changes_revision`, so a relationship opened by that route +serialized a reason the shipped contract refused. Old relationships without it still validate; a +generation with no reason (a returning tenure) still validates as null. The 6 existing relationship +cases were re-judged against the revised bytes with jsonschema 4.19.2 Draft7Validator and kept their +verdicts; 6 additional recorded cases cover each reason the registry accepts, the empty reason, and +an unknown and a mistyped one. This changes the stored JSON contract, not the SQLite schema. diff --git a/contract/schema/relationship.json b/contract/schema/relationship.json index cc6452275a..aa6764fe3a 100644 --- a/contract/schema/relationship.json +++ b/contract/schema/relationship.json @@ -95,6 +95,7 @@ "enum": [ "initial_assignment", "needs_changes_revision", + "accepted_result_correction", null ] } diff --git a/docs/crw-run/dispatch-cases/recorded/0560-S27b.md b/docs/crw-run/dispatch-cases/recorded/0560-S27b.md index 606cc1fc62..5aaf0e067b 100644 --- a/docs/crw-run/dispatch-cases/recorded/0560-S27b.md +++ b/docs/crw-run/dispatch-cases/recorded/0560-S27b.md @@ -1 +1 @@ -| S27b | The child waits for the one run of each reviewer within the waiting budget before it emits: a Devin status `Analyzing your changes` is waited for up to the budget and `Full review skipped: trial expired and no credits remaining` is not; Codex is waited for while its eyes reaction is on the pull request or any row of its summary comment is not `Completed`, and each of its code review and security review must be `Completed` or skipped by the bot's notice (a thumbs-up counts for both when no row contradicts it), so a Security Review row `Completed` alone is not finished; past the budget a run still going is recorded as pending, not complete, and the wait is released only where the candidate's required independent review and its finding dispositions already exist; when neither reviewer shows any signal 30 minutes after the pull request became ready or was opened, the handoff says `review unavailable (no signal)` and the child goes on, and when only one is silent the same 30 minutes apply to it and the handoff says so; a thread that arrives after the receipt is a late finding: unless the parent has dispositioned it for that head (`--late-dispositions`) it returns the candidate before the acceptance, and after the acceptance no correction can reach the child and the candidate is held ([Late review threads](merge-readiness.md#late-review-threads)) | [The one run of each reviewer, awaited before the receipt](merge-readiness.md#the-one-run-of-each-reviewer-awaited-before-the-receipt) and [Recheck, integrate, and record](merge-readiness.md#recheck-integrate-and-record) and [a late thread the parent dispositions itself](merge-readiness.md#a-late-thread-the-parent-dispositions-itself) | Merge gate | changed with CRW-808; the unlimited wait became the waiting budget with a pending record | +| S27b | The child waits for the one run of each reviewer within the waiting budget before it emits: a Devin status `Analyzing your changes` is waited for up to the budget and `Full review skipped: trial expired and no credits remaining` is not; Codex is waited for while its eyes reaction is on the pull request or any row of its summary comment is not `Completed`, and each of its code review and security review must be `Completed` or skipped by the bot's notice (a thumbs-up counts for both when no row contradicts it), so a Security Review row `Completed` alone is not finished; past the budget a run still going is recorded as pending, not complete, and the wait is released only where the candidate's required independent review and its finding dispositions already exist; when neither reviewer shows any signal 30 minutes after the pull request became ready or was opened, the handoff says `review unavailable (no signal)` and the child goes on, and when only one is silent the same 30 minutes apply to it and the handoff says so; a thread that arrives after the receipt is a late finding: unless the parent has dispositioned it for that head (`--late-dispositions`) it returns the candidate before the acceptance, and after the acceptance the correction goes through the hand-opened generation [Late review threads](merge-readiness.md#late-review-threads) describes, while a node that landed is held ([Late review threads](merge-readiness.md#late-review-threads)) | [The one run of each reviewer, awaited before the receipt](merge-readiness.md#the-one-run-of-each-reviewer-awaited-before-the-receipt) and [Recheck, integrate, and record](merge-readiness.md#recheck-integrate-and-record) and [a late thread the parent dispositions itself](merge-readiness.md#a-late-thread-the-parent-dispositions-itself) | Merge gate | changed with CRW-808; the unlimited wait became the waiting budget with a pending record | diff --git a/docs/crw-run/dispatch-cases/recorded/0630-S27i.md b/docs/crw-run/dispatch-cases/recorded/0630-S27i.md index cc41042e66..284991481d 100644 --- a/docs/crw-run/dispatch-cases/recorded/0630-S27i.md +++ b/docs/crw-run/dispatch-cases/recorded/0630-S27i.md @@ -1 +1 @@ -| S27i | A red, P0, P1 or security late thread, a P2 or P3 whose effect is in a blocking class, or a late thread the coordinator cannot grade without reconstructing the child's reasoning, is the child's ordinary correction with the restoration block before the acceptance: the child fixes the finding or answers it with code evidence, pushes and reruns checks only if it changed something, and emits again, and the candidate does not merge meanwhile; after the acceptance of a current result no correction can reach the child, so such a thread, and a minor one the installed relay cannot record, is reported on the coordination record and the candidate is held, with no generation opened by hand around `dag-correct`; the review threads on N, and each reviewer's summary comment, are read once more before `dag-accept` so that a late thread surfaces while a route exists; after the merge a late thread is new work (a minor one for the backlog, a red, P0, P1 or security one a correction issue) | [Late review threads](merge-readiness.md#late-review-threads) and [Refresh the base yourself when only the base moved](merge-readiness.md#refresh-the-base-yourself-when-only-the-base-moved) | Merge gate | added 2026-10-03; basis below | +| S27i | A red, P0, P1 or security late thread, a P2 or P3 whose effect is in a blocking class, or a late thread the coordinator cannot grade without reconstructing the child's reasoning, is the child's ordinary correction with the restoration block before the acceptance: the child fixes the finding or answers it with code evidence, pushes and reruns checks only if it changed something, and emits again, and the candidate does not merge meanwhile; after the acceptance of a current result the correction goes through the generation opened by hand under a reason that states it, so such a thread returns to the child that way; a thread that route cannot reach, a node that landed and a minor one the installed relay cannot record included, is reported on the coordination record and the candidate is held; the review threads on N, and each reviewer's summary comment, are read once more before `dag-accept` so that a late thread surfaces while a route exists; after the merge a late thread is new work (a minor one for the backlog, a red, P0, P1 or security one a correction issue) | [Late review threads](merge-readiness.md#late-review-threads) and [Refresh the base yourself when only the base moved](merge-readiness.md#refresh-the-base-yourself-when-only-the-base-moved) | Merge gate | added 2026-10-03; basis below | diff --git a/docs/relay/README.md b/docs/relay/README.md index e49203e80b..1f36f2c8fc 100644 --- a/docs/relay/README.md +++ b/docs/relay/README.md @@ -841,7 +841,7 @@ When a step refuses, the refusal names what to do: | recorded `verified`, asked `unverified` or `aborted` | `disposition_conflict` | rule `needs_changes`, or stop the assignment by changing the relationship's status | | recorded `needs_changes` | `disposition_conflict` | the ruling opened a generation and is not withdrawn: rule the head of that generation when the child reports there | | recorded `unverified` or `aborted` | `disposition_conflict` | the ruling is final for the event: open a fresh execution generation (`generation-open`, then `generation-bind`) and rule what the child reports there | -| a plan accepted the verified event | `disposition_conflict` | read the node's stale reading: when its action is `correct`, `dag-correct --prepare` prints the instruction and the dispatch request id, the generation is opened by hand and `dag-correct` binds it; `revalidate` and `hold` have their own steps; when the accepted result is current, a base that moved after the acceptance is recorded by `dag-base-refresh` (a generation opened by hand that only merges the base, ruled `verified`; the relay proves from git that its head is the accepted head plus merges of the base), and for any other current result this build records no correction route, so report it and open no generation that `dag-correct` will refuse | +| a plan accepted the verified event | `disposition_conflict` | read the node's stale reading: when its action is `correct`, `dag-correct --prepare` prints the instruction and the dispatch request id, the generation is opened by hand and `dag-correct` binds it; `revalidate` and `hold` have their own steps; when the accepted result is current and a defect was found in it before it landed, the same steps correct it, with the generation opened under a reason that states the correction (`accepted_result_correction`, or the `needs_changes_revision` default), after which `dag-accept --supersedes` takes the corrected result ([the scheduler](dag-scheduler.md#correcting-a-result-that-was-accepted-and-is-still-current)); when only the base moved, `dag-base-refresh` records it (a generation opened by hand that only merges the base, ruled `verified`; the relay proves from git that its head is the accepted head plus merges of the base) | | the work is marked merged, or a turn landed it | `disposition_conflict` | a merged result is corrected by new work, not by a second ruling | | a merge turn is merging or of unknown effect | `disposition_conflict` | resolve the turn (`merge-turn-resolve` reads the branch), then rule again | | the event is not the head | `stale_generation` or `superseded_revision` | rule the head the assignment shows | @@ -886,7 +886,7 @@ The rule is fixed by the kind. `answer` and `stop` change nothing the child's at No refusal reason is added, and a refusal writes nothing. The command line is checked first: an unknown `--decision` or a missing required option ends with the parser's own exit 2 and a message on stderr before the writer runs, an unreadable `--note @path` is a host problem (exit 3, as for `verdict --criteria`), and `malformed_receipt` is what the writer answers to a direct call or to a blank note, turn or digest. `decision-show` reads the decisions of a relationship back, oldest first, each with the state of its delivery. -Limits that are part of the contract. The relay records the decision and its delivery; it does not read the child's thread, so `dispatched` says the message was accepted, not that the child acted. A decision that opened g+1 is recorded against a DAG plan node by `dag-correct` ([the third way to open a generation](dag-scheduler.md#three-ways-to-open-the-generation)), which refuses until the decision was dispatched into the turn the generation is bound to; until it is recorded, `dag-accept` finds no execution for g+1 and refuses `stale_generation`. A message sent to the child outside this route leaves a record only when the registered parent admits its turn ([Direct parent interventions](#direct-parent-interventions)). The crw-run skill's need-input procedure follows this route. After an answer or a stop, the relationship still reads as blocked (`dispositions-show`) until the child reports again. `registry`'s `generation-open --reason` still accepts only its own two reasons: `decision_reply` is written by this command alone. An installed relay older than this change has no `decision-reply` and answers the command as an unknown subcommand. +Limits that are part of the contract. The relay records the decision and its delivery; it does not read the child's thread, so `dispatched` says the message was accepted, not that the child acted. A decision that opened g+1 is recorded against a DAG plan node by `dag-correct` ([the third way to open a generation](dag-scheduler.md#three-ways-to-open-the-generation)), which refuses until the decision was dispatched into the turn the generation is bound to; until it is recorded, `dag-accept` finds no execution for g+1 and refuses `stale_generation`. A message sent to the child outside this route leaves a record only when the registered parent admits its turn ([Direct parent interventions](#direct-parent-interventions)). The crw-run skill's need-input procedure follows this route. After an answer or a stop, the relationship still reads as blocked (`dispositions-show`) until the child reports again. `registry`'s `generation-open --reason` still accepts only the reasons that command writes: `decision_reply` is written by this command alone. An installed relay older than this change has no `decision-reply` and answers the command as an unknown subcommand. ## The coordination summary diff --git a/docs/relay/dag-scheduler.md b/docs/relay/dag-scheduler.md index 27e13dcfab..65e60f0278 100644 --- a/docs/relay/dag-scheduler.md +++ b/docs/relay/dag-scheduler.md @@ -154,7 +154,7 @@ The reading says why a node is stale and, in the `action` of the stale object, w | `action` | When | What is done | What is never done | | --- | --- | --- | --- | | `revalidate` | only the node's criteria changed (cause `criteria_changed`) and everything it consumed is still current: no acceptance it consumed was replaced and the manifest rebuilt as it consumed it is complete and digests to the same name | the same output is ruled again under the plan's criteria and `dag-accept` records a revalidation of the same acceptance (E-11, E-21): the criteria registered for the relationship change, the parent rules the output `verified` under the new set, and accepts it again | a new generation or a new child | -| `correct` | everything else on a node whose relationship is active: its spec, an edge into it or an input it consumed changed, or a change of criteria that also moved an input | the output is reworked by the same child, as the next generation of the same relationship: `dag-correct --prepare`, then either the `needs_changes` ruling that carries its instruction or a generation opened by hand ([below](#two-ways-to-open-the-generation)), `dag-correct`, then `dag-accept --supersedes` for the reworked result | a child made again | +| `correct` | everything else on a node whose relationship is active: its spec, an edge into it or an input it consumed changed, or a change of criteria that also moved an input | the output is reworked by the same child, as the next generation of the same relationship: `dag-correct --prepare`, then either the `needs_changes` ruling that carries its instruction or a generation opened by hand ([below](#three-ways-to-open-the-generation)), `dag-correct`, then `dag-accept --supersedes` for the reworked result | a child made again | | `hold` | the node rests on a stale predecessor (whatever its own cause: cause `predecessor_stale`, or a replaced input, or a change of criteria on a node that does), or an input is not there (an incoming edge that is not satisfied now: a predecessor nobody has accepted yet, a decision that was rejected), or the plan holds the node (paused, or the whole plan paused), or its relationship is paused, or a correction generation of it is already open | nothing on this node: the detail names what to wait for | | | `redefine` | the relationship has ended (archived, superseded or cancelled), so there is no child to send a correction to | the parent decides whether to redefine the node: a new relationship registered with `supersedes` (contract 5, ID-4), which makes a new child | | @@ -172,9 +172,9 @@ What a node's siblings keep is whatever this page never touches. A revalidation The correction goes to the same child as the next generation of the same relationship, and the generation is opened in one of three ways. `dag-correct --prepare` prints the instruction line, the manifest digest and the dispatch request id for the first two. * **A ruling.** The relay's verdict writer takes another ruling on an event it already ruled `verified` in two cases ([Ruling an event that is already ruled](README.md#ruling-an-event-that-is-already-ruled)). The first is while a review is open, which the relay opens when the criteria registered for the relationship are no longer the set the head was ruled under (`delivery/ack.go`, re-review; the accepted event must still be the head of the current generation). The second is a `needs_changes` ruling before anything rests on the verified ruling: no acceptance of the event, no merged mark, no merge turn that is merging, of unknown effect or landed. Once the plan has accepted the event and no review is open, the writer refuses a second ruling with `disposition_conflict`, and the second way below is the route. The parent rules `needs_changes` with the instruction as the restoration finding, the writer opens the generation and sends the instruction, and `dag-correct` binds the manifest the finding names. A plan revision that changes the node's criteria digest, with the new set registered for the relationship, opens that review. -* **A generation opened by hand.** When the head is ruled `verified` under the criteria registered now, which is the case of a node made stale because an input it consumed was replaced and nothing else, the writer refuses a second ruling on the accepted head (`disposition_conflict`, naming this route) and opens no generation. The relay has a route for that, written in the coordinator's skill (`crw-run`, "A fresh execution generation"): the coordinator opens the generation with `generation-open --reason needs_changes_revision` under the dispatch request id `dag-correct --prepare` printed, sends the instruction line to the child in the message that dispatches the re-review, and binds that turn with `generation-bind`. Then `dag-correct --manifest-digest D` binds the generation to the manifest. Only a node whose stale reading says `correct` takes this route: `dag-correct --prepare` succeeds for any node, and a generation opened for a node whose route is `revalidate` or `hold` would be refused when it is recorded. +* **A generation opened by hand.** When the head is ruled `verified` under the criteria registered now, which is the case of a node made stale because an input it consumed was replaced and nothing else, the writer refuses a second ruling on the accepted head (`disposition_conflict`, naming this route) and opens no generation. The relay has a route for that, written in the coordinator's skill (`crw-run`, "A fresh execution generation"): the coordinator opens the generation with `generation-open --reason needs_changes_revision` under the dispatch request id `dag-correct --prepare` printed, sends the instruction line to the child in the message that dispatches the re-review, and binds that turn with `generation-bind`. Then `dag-correct --manifest-digest D` binds the generation to the manifest. A node whose stale reading says `correct` takes this route, and so does an accepted node that is still current when its generation's reason states a correction (below): `dag-correct --prepare` succeeds for any node, and a generation opened for a node whose route is `revalidate` or `hold` would be refused when it is recorded. -What the second way binds is bounded, so it cannot make a rerun or bind a manifest other than the one the generation was opened for: the node's accepted result must be stale (a result that is not accepted yet is corrected by a ruling, and an accepted result that is current has no recorded correction route in this build (a generation that only merged the base is recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) instead): the writer refuses a ruling on it and this route refuses it as not stale; a node that landed was refused before); the manifest must be named with `--manifest-digest`, be the one stored for this node at its current slice and criteria, and still rest on the inputs the node's edges are satisfied by now (it is judged against the store again, the file bytes aside, as release does when it records its own intent: a predecessor accepted again after the prepare leaves another input than the one the child was told to consume, and nothing is bound); the generation must have been opened under the dispatch request id derived from that manifest (`CorrectionRequestID`: the plan, the node, the manifest and the generation), so it was opened for this manifest and no other; it must be bound to a dispatch turn, because a child reports in a bound generation only; and it must be the generation right after the one the accepted result stands on, so a correction that is already open is not skipped by opening another beside it (opening one moved the relationship past the generation that correction is accepted on: the refusal is reported and no further generation is opened, as when the plan moved the node). Each refusal is `disposition_conflict` with its reason in the detail and writes nothing. What is recorded of how the instruction reached the child is that request id (in `managed_request_id` of the execution row, which a ruling leaves empty) and the turn the generation is bound to; the answer says `opened_by` (`ruling`, `generation_open` or `decision_reply`), `dispatch_request_id` and `dispatch_turn_id`, and a repeated `dag-correct` answers the same as a replay. The generation is not recorded for a node whose route, without that generation, is anything but a correction: a change of the criteria alone is ruled again and opens no generation, and what rests on a stale predecessor or on an input that is not there waits. +What the second way binds is bounded, so it cannot make a rerun or bind a manifest other than the one the generation was opened for: the node's accepted result must be stale, or accepted and still current with a reason that states a correction ([Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current); a result that is not accepted yet is corrected by a ruling, and a generation that only merged the base is recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) instead: the writer refuses a ruling on it and this route refuses it because it is not a correction; a node that landed was refused before); the manifest must be named with `--manifest-digest`, be the one stored for this node at its current slice and criteria, and still rest on the inputs the node's edges are satisfied by now (it is judged against the store again, the file bytes aside, as release does when it records its own intent: a predecessor accepted again after the prepare leaves another input than the one the child was told to consume, and nothing is bound); the generation must have been opened under the dispatch request id derived from that manifest (`CorrectionRequestID`: the plan, the node, the manifest and the generation), so it was opened for this manifest and no other; it must be bound to a dispatch turn, because a child reports in a bound generation only; and it must be the generation right after the one the accepted result stands on, so a correction that is already open is not skipped by opening another beside it (opening one moved the relationship past the generation that correction is accepted on: the refusal is reported and no further generation is opened, as when the plan moved the node). Each refusal is `disposition_conflict` with its reason in the detail and writes nothing. What is recorded of how the instruction reached the child is that request id (in `managed_request_id` of the execution row, which a ruling leaves empty) and the turn the generation is bound to; the answer says `opened_by` (`ruling`, `generation_open` or `decision_reply`), `dispatch_request_id` and `dispatch_turn_id`, and a repeated `dag-correct` answers the same as a replay. The generation is not recorded for a node whose route, without that generation, is anything but a correction: a change of the criteria alone is ruled again and opens no generation, and what rests on a stale predecessor or on an input that is not there waits. What the second way does not prove: the relay does not read the child's thread, so the request id and the bound turn are the coordinator's statement of how the instruction was delivered, where a ruling's restoration note is compared with the instruction line. The child checks the manifest file against the digest and sha256 the line names and answers `blocked_needs_input` on a mismatch. The child's first receipt in the generation carries no `--supersedes-revision` (the generation has no earlier revision to supersede). A generation opened by hand is also not recoverable by dag-correct if the plan moved the node between the opening and the recording: the recording is refused (the manifest was prepared for another version of the node), preparing again names the next generation, and recording that one is refused at the gap (`generation 3 of R follows generation 2, and the last recorded execution of N is 1`). Open no further generation: report the refusal and stop, since the same holds for a ruling that opened its generation before the plan moved. @@ -192,6 +192,23 @@ The row is a `correction` execution whose `managed_request_id` is the decision r What a decision does not do to a generation: `answer` and `stop` keep it, so there is nothing to record and `dag-correct` replays the generation already recorded. A result in g+1 is accepted as any result is (`dag-accept`, with `--supersedes` when the node already had an accepted result), and the node reads accepted and not stale, because its manifest carries the plan's slice and criteria digests. The generation must be recorded before anything else opens another one: a second advancing decision, or a ruling's generation, before g+1 is recorded leaves a gap that `dag-correct` refuses for good (generation g+2 follows a generation that is not recorded), and the way out is a redefinition. A paused plan, node or relationship is refused as for every correction, and the same call is recorded after the resume. +#### Correcting a result that was accepted and is still current + +A blocking defect can be found in a result that the plan already accepted and that is still current, before it lands: a bundle review that finds one in an accepted member is the case. Nothing above the node changed, so the reading gives it no stale object and no route, the relay's verdict writer takes no second ruling on the head it ruled `verified` (`disposition_conflict`), and `dag-accept` refuses a generation that is not recorded as an execution of the node. The result is corrected by the same child as the next generation of the same relationship, opened by hand, with the same steps as the stale case and one more: the reason the generation is opened under states the correction. + +1. `dag-correct --prepare` rebuilds the node's manifest as the store holds it now and prints the instruction line, the manifest digest and the dispatch request id. +2. `generation-open --relationship --dispatch-request-id --reason accepted_result_correction` (or `--reason needs_changes_revision`, the default; both state a correction) opens the next generation. The instruction line is sent to the child in the message that dispatches the re-review, and that turn is bound with `generation-bind`. +3. `dag-correct --manifest-digest D` records the generation as a `correction` execution of the node (`opened_by` `generation_open`), under every check the stale case has: the generation right after the one the acceptance stands on, the manifest stored for the node at its current slice and criteria, the dispatch request id derived from that manifest, a bound dispatch turn, and the inputs as they stand now. The node must not have landed, and a node whose result is not accepted yet keeps the ruling route. +4. The child reports in the generation, the parent rules it and takes the result with `dag-accept --supersedes `. Until then the previous acceptance stays active and the node still reads `done:accepted`; the edges that take its result read `blocked:stale_head` while the current generation is not recorded as an execution, so nothing downstream consumes the corrected generation before it is accepted. + + The accepted head is held back while the correction is open, not only after the child pushes. The merge gate reads the relationship's live execution generation against the generation its active acceptance stands on: while the live one is later, the node is **under correction** and its old head is not a merge candidate. The single-lane judgement (`dag-merge-judge`, `dag-merge-request`) is refused `disposition_conflict` naming the open generation and writes no row and no turn, and the bundle open, verify and land refuse a member the same way (see [Merge eligibility](#merge-eligibility)). Without this, a bundle or a lane turn taken between opening (or recording) the generation and the child's first push would carry and land exactly the head the correction is repairing, because until that push the pull request still shows the accepted head. The gate is about the correction being open, not about the head: `dag-accept --supersedes` of the new generation's result moves the acceptance to the stand generation and the node is judged again, a withdrawn generation puts the live generation back and the old head may ride again, and a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. + + A turn's repository is matched against the accepted node's repository and its forge repository (the owner/name recorded with the acceptance) by one rule. A forge slug is compared trimmed and case-insensitively. A local path is compared by the repository git names for it: the common git directory of the path, with symlinks resolved, so a checkout, its `.git` directory, a symlink to it, a linked worktree and that worktree's administrative directory are one repository, while a directory whose name ends in a space is still its own name. A path that git cannot name (it is not a repository here, git is missing, or it does not answer in time) is compared by its exact text, and nothing else. The lane gate and the in-flight guard of a correction apply the same rule, so a turn spelled any of these ways is held the same way. + +Recording is refused while the accepted head may already be on its way to the base: a merge turn of the node is `merging`, of unknown effect or `landed` (resolve it first, `merge-turn-resolve`), or a live bundle (opened or verified) carries the node's stand head as a member (close it first, `merge-train-close`). A bundle refusal is not a dead end: the coordinator closes the bundle and repeats step 3 with the same manifest digest. A turn that is `merging` or `unknown` is resolved first, and once it resolves to `returned` step 3 can be repeated. A turn that is already `landed` is refused, and `merge-turn-resolve` does not return it, so this route does not record the correction of that node; the parent reports it. In every case the generation opened in step 2 stays unrecorded and no other generation is opened. Preparing again and opening another generation would leave the unrecorded one behind, and the next correction would be refused for not following it. + +What notes the route is the generation's own `reason` column: no column, table, schema, refusal name, CLI flag or `specs.json` entry is added. A generation opened under a reason that states no correction (the assignment itself, a returning tenure, a reply) is refused by the same step, naming the reason it carries, and a generation opened for a base refresh is not bound here either: [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) records that one. A generation opened for this route and never sent is withdrawn like any other ([A generation opened by hand and never sent](#a-generation-opened-by-hand-and-never-sent)). + ### Input checks When all incoming edges of a candidate are satisfied, the artifacts it would consume are read again. These file checks apply to non-code `artifact_verified` inputs (a code-pinned input consumes the accepted head instead), and a frozen copy of the manifest is checked when the receipt records its reference: every file the predecessor's receipt declared must still @@ -568,7 +585,7 @@ A parent rules a node's report `verified`, finds before `dag-accept` that the ba `crw relay dag-generation-withdraw --plan P --node N --actor A --relationship R --generation G --reason TEXT [--expect-epoch E]` (answer `dag-generation-withdraw/1`) is the counterpart of `generation-open` for the plan's node. It closes the generation and puts the relationship back on the generation before it; the commands that follow are the ordinary ones. The answer carries `relationship_id`, `withdrawn_generation`, `restored_generation`, `dispatch_request_id`, `reason` and `replayed`. -**What the relay requires, all read in one transaction.** The caller holds the plan's epoch; the relationship named with `--relationship` (`assignment-show` prints it) is an execution of the node and the caller is its parent; the plan does not hold the node (paused, cancelled, archived); the relationship is active and not superseded; `G` is 2 or more and is the generation the relationship stands on (`stale_generation` otherwise); the generation's row is an anchor still pending, with no dispatch turn and no bind time, and carries a reason `generation-open` gives (`initial_assignment` or `needs_changes_revision`: the generation of a returning tenure has none, and a withdrawal does not undo a tenure); no event of any outcome belongs to it (a report, a revision request, a decision reply); no turn was admitted to it; no `needs_changes` ruling opened it; and no execution of a node was recorded on it (`dag-correct` and `dag-adopt` record one). A generation that fails one of these was sent, used or recorded, and the refusal is `disposition_conflict` naming what was found, exactly as `dag-accept` refuses it today. A poll the daemon stored for an older turn under the generation's number is not read: it says nothing about the generation. +**What the relay requires, all read in one transaction.** The caller holds the plan's epoch; the relationship named with `--relationship` (`assignment-show` prints it) is an execution of the node and the caller is its parent; the plan does not hold the node (paused, cancelled, archived); the relationship is active and not superseded; `G` is 2 or more and is the generation the relationship stands on (`stale_generation` otherwise); the generation's row is an anchor still pending, with no dispatch turn and no bind time, and carries a reason `generation-open` gives (`initial_assignment`, `needs_changes_revision` or `accepted_result_correction`: the generation of a returning tenure has none, and a withdrawal does not undo a tenure); no event of any outcome belongs to it (a report, a revision request, a decision reply); no turn was admitted to it; no `needs_changes` ruling opened it; and no execution of a node was recorded on it (`dag-correct` and `dag-adopt` record one). A generation that fails one of these was sent, used or recorded, and the refusal is `disposition_conflict` naming what was found, exactly as `dag-accept` refuses it today. A poll the daemon stored for an older turn under the generation's number is not read: it says nothing about the generation. **What it writes.** One row of `dag_generation_withdrawals` (appended, never changed: relationship, generation, plan, node, the dispatch request id it was opened under, the reason it was opened for, the generation the relationship stands on again, the coordinator's reason, who withdrew it, the epoch and the time), the relationship's `execution_generation`, and a `generation_withdrawn` journal row. The generation's own row stays. **A number is never reused**: the next generation, whoever opens it (`generation-open`, a ruling, a decision reply, a tenure), takes one more than the highest the relationship ever held, so after a withdrawn generation 2 the next is 3. A generation that was withdrawn is final: `generation-open` repeated under its request id, `generation-bind` and the anchor binders of delivery are refused `stale_generation` (the daemon's binder skips it), so it cannot come back. The generation a correction follows is the nearest one that was not withdrawn, in the relay's currency (the predecessor a child's first report declares) and in `dag-correct`. Nothing else is written; the command does not touch a delivery: a generation-open leaves only a `stale_generation` note on older deliveries that were not yet delivered, the delivery pass reads the current generation, so an older delivery that was queued then can become eligible again under the delivery policy as it stands. The same relationship and generation again are a replay, also after the node was adopted by another relationship: the relationship is named so that a repeated call can never withdraw the generation of another relationship that has the same number. @@ -590,7 +607,7 @@ supersedes the active one and keeps the history, so a rejection after an approva A correction goes back to the same child (contract 3.2): a `needs_changes` ruling opens the next generation of the same relationship and the existing verdict writer sends the correction message in the same transaction, so a manifest cannot be placed in that generation afterwards. The protocol therefore puts the manifest into the one text the relay itself sends. `dag-correct --prepare --manifest-request @file` rebuilds and verifies the node's input manifest as the store holds it now (the base it records is read from the named target branch as a release reads it: the request names the repository and the ref, and a `sha` it states is checked against the branch tip and refused as `merge_base_mismatch` when the branch reads another commit, with nothing stored; the recorded commit is always the one the branch reads), stores it, and prints an English -instruction line that names the manifest, a copy of its canonical bytes kept under the child's first artifact root (the file the child reads, since it cannot read the relay's store; see *The frozen copy*) and the sha256 of that file; the parent submits that line as the **restoration** finding of its `needs_changes` ruling. (When no ruling can open the generation, the coordinator opens it by hand under the dispatch request id the same step prints and carries the line in its own dispatching message: [Two ways to open the generation](#two-ways-to-open-the-generation).) After the ruling, `dag-correct` (without `--prepare`) binds the generation the ruling opened to the manifest that finding names +instruction line that names the manifest, a copy of its canonical bytes kept under the child's first artifact root (the file the child reads, since it cannot read the relay's store; see *The frozen copy*) and the sha256 of that file; the parent submits that line as the **restoration** finding of its `needs_changes` ruling. (When no ruling can open the generation, the coordinator opens it by hand under the dispatch request id the same step prints and carries the line in its own dispatching message: [Three ways to open the generation](#three-ways-to-open-the-generation). A result the plan already accepted and that is still current has no ruling to open it, and its generation is opened by hand under a reason that states the correction: [Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current).) After the ruling, `dag-correct` (without `--prepare`) binds the generation the ruling opened to the manifest that finding names (`dag_node_executions`, kind `correction`) once the manifest is stored for this node at its current slice and criteria and the restoration note carries, word for word, the instruction that manifest was prepared with (this generation, the path of the copy and its hash). No file is read when the generation is recorded: the child checks the copy against the hash in the line it was sent. The bound digest is derived from what the child was told: a digest in an ordinary finding does not bind, a restoration block that names two different manifests is refused, a ruling without the block leaves the previous manifest in force (`carried_over`), and a `--manifest-digest` that differs is refused, also when the generation is already bound. A node whose accepted head landed in every target is refused in both steps ([Handling a stale node](#handling-a-stale-node)). The previous acceptance stays active and reads `blocked:stale_head` until the parent accepts the corrected result with `--supersedes`; what consumed the old acceptance then reads stale ([Invalidation](#invalidation)). @@ -629,6 +646,8 @@ request (by the forge identity recorded with the acceptance) and the base branch A stale accepted result is not judged at all: a stale result never merges (contract 8.2, [Invalidation](#invalidation)). The call is refused `disposition_conflict` naming the stale reason, ahead of every rule above (an evicted head included), and writes no row of the history. The judgement is asked again inside the transaction that writes, so a plan revision that lands between the read of the pull request and the write is seen; `dag-merge-request` asks it once more in the transaction that creates the turn (after the criteria and the predecessors, so a criteria change that lands in that gap is `criteria_set_changed` and an integrated or code-pinned edge added in it, whose predecessor has not landed, is the unmet predecessor), and a revision that lands after the judgement has recorded an `eligible` row leaves that row in the history and creates no turn. A result that is current again (the plan repaired, the node accepted again) is judged as before. +A result that is **under correction** is not judged either, and this one is about the generation rather than the head: while the relationship's live execution generation is later than the generation its active acceptance stands on, a correction generation is open over the accepted result and has not been accepted over yet, so the head the acceptance stands on is the very result being repaired. The call is refused `disposition_conflict` naming the open generation and the generation the acceptance stands on, and writes no row of the history; `dag-merge-request` makes the same judgement, so it creates no turn. The refusal is made where the head would otherwise be judged, so a head the history already evicted keeps its eviction (that eviction is final and is restated, never lifted) and a pull request at another head is still stale_head; either way the head is not eligible and no turn is created. The reading is taken with the acceptance in one statement and asked again in the transaction that writes, so a generation opened while the pull request is being read is seen. It is the guard [Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current) rests on before the child has pushed: without it, a bundle or a lane turn taken between opening (or recording) the generation and that first push would carry and land the accepted head, which the pull request still shows. A correction accepted over with `dag-accept --supersedes` moves the acceptance to the stand generation and the node is judged as before; a withdrawn generation puts the live generation back and the old head may ride again; a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. The merge train's own gate (`trainStandRefusal`) refuses a bundle member the same way at open, verify and land, naming the open generation. + A required check is read the way merge-evidence and the merge lane's own check read it: of every run only its newest attempt counts, every run of a required name must be a `success` (a skipped or neutral run is not one, so a head the judgement lets in is not refused by the lane for the same check), a required (name, integration) pair needs its own run (a name that two integrations must answer needs both), and the opaque run identities are never ordered. Failures count only when every required check has finished, so one red job seen beside a running one never uses up the retry. A second, different failure after the retry round opened evicts. A failure is told from another by the run's identity, its attempt and the time the forge last changed it (a commit status has no attempt and a check run can be reset in place), and a failure of a run that a judgement saw not failing (running or passing) since it failed is a new failure whatever its identity; another check running again says nothing about this one. Everything is computed from the newest reading of each run (a listing that still carries a failed first attempt beside the passing second one judges as the second), a failure counts once a judgement recorded it (a red job beside a running one is not counted), and a reading of a run that is older than one already judged (an earlier attempt, or an earlier completion of the same attempt, arriving late) is not judged: it is `merge_candidate_moved` and the caller reads again. So is a reading of the pull request that was in flight while another judgement of the node was recorded: it is older than the history whatever it contains, and nothing is written. Times are compared as instants (the forge's spelling does not matter). A forge that returns an older state of a run after a newer one was judged cannot always be told from a run that was reset: a reading that goes backwards without a time and with no judgement between the two (a run read as running after its completion) can count as a rerun, so a head can be evicted one failure early. Two failures that carry the same identity and the same time and that no judgement saw apart read as one failure: the node is not eligible either way, but it can take more than one retry. A pull request whose list of required checks cannot be read is not judged (`merge_evidence_malformed`, nothing written): ignorance is not "none required". A draft, closed or merged pull request is `disposition_conflict` (a merged one once its reading is classified, [The forge reading of a merged pull request](#the-forge-reading-of-a-merged-pull-request)); evidence the relay could not read completely is the host's failure; both write nothing. A row of the history that no longer digests to what was recorded (B-13) is `revision_mismatch`, and the edges built on it read `blocked:evidence_mismatch`. @@ -918,7 +937,8 @@ Where the code reads the contract differently, or adds to it, and why: * An unknown edit region overlaps everything; a node that edits no repository (`non_pr`) never conflicts. * The reading adds the disposition and state `stale` with the reasons `stale:slice_changed`, `stale:criteria_changed` and `stale:edge:` (contract 3.1 `stale:`), and an artifact edge, or an integrated edge a landing satisfies, from a stale result reads `blocked:stale_predecessor`; the merge judgement refuses a stale result with the existing `disposition_conflict`. No refusal reason, outcome or zone statement is added. Contract 4.2 lists the landed commit among the consumed values: the integrated predicate takes the earliest positive observation of the containment run, so a manifest names a stable landing, and a landed commit that an observation of the same run names is read as unchanged ([Invalidation](#invalidation)). * The stale reading carries a route, `action` and `action_detail` (`revalidate`, `correct`, `hold`, `redefine`), which the contract leaves to the parent. Decision D-08 is read from contract 5: a correction is a new generation of the same relationship and a node redefinition a new relationship with `supersedes`; the scheduler never registers the second. No refusal reason, reading reason, outcome or zone statement is added: a revalidation of a node that is not stale for its criteria alone, and a correction of a node that landed, are refused with `disposition_conflict`. -* Contract 3.2 and E-19 send a correction to the same child through the existing path. That path opens a generation with a `needs_changes` ruling, and the relay's verdict writer takes a second ruling on a head it ruled `verified` only while a review is open or, for `needs_changes`, before the plan accepted it, and refuses it with `disposition_conflict` otherwise. For a stale accepted node whose criteria did not change the generation is opened by hand with the relay's `generation-open` and `generation-bind` (documented for the coordinator in `crw-run`), and `dag-correct` binds it to the prepared manifest under the conditions of [Two ways to open the generation](#two-ways-to-open-the-generation). No refusal reason, zone statement or table is added: the dispatch request id is kept in the existing nullable `managed_request_id` column of `dag_node_executions`. +* Contract 3.2 and E-19 send a correction to the same child through the existing path. That path opens a generation with a `needs_changes` ruling, and the relay's verdict writer takes a second ruling on a head it ruled `verified` only while a review is open or, for `needs_changes`, before the plan accepted it, and refuses it with `disposition_conflict` otherwise. For a stale accepted node whose criteria did not change the generation is opened by hand with the relay's `generation-open` and `generation-bind` (documented for the coordinator in `crw-run`), and `dag-correct` binds it to the prepared manifest under the conditions of [Three ways to open the generation](#three-ways-to-open-the-generation). No refusal reason, zone statement or table is added: the dispatch request id is kept in the existing nullable `managed_request_id` column of `dag_node_executions`. +* A result the plan accepted and that is still current, with a blocking defect found in it before it lands, is corrected the same way and not by a new route: the generation is opened by hand under a reason that states the correction (`accepted_result_correction`, or the `needs_changes_revision` default), and `dag-correct --manifest-digest` records it under the checks the stale case has ([Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current)). The reason the generation carries is what notes the route, so no column, table, schema, refusal reason, reading reason, CLI flag or `specs.json` entry is added; the acceptance stays active until `dag-accept --supersedes` takes the new result. The contract's 3.2 and E-19 speak of a stale result; this case is the contract's silence on a defect found after the acceptance, and the parent's answer to it is recorded here. * Contract 3.2 and E-10 read the pull request head against `dag_acceptances.head_sha`. A node whose acceptance stands on a recorded base refresh ([A base refresh of an accepted node](#a-base-refresh-of-an-accepted-node)) is read against the head that record names, because the lane's own check wants the head the newest generation's report names, which is that head; an unrecorded head, or a head no record can be made for, is E-10 as written. `dag_merge_checks.head_sha` holds the head the acceptance stood on when the check was made, which is the accepted head unless a record moved it. No refusal reason, outcome or zone statement is added. * The conflict sweep (CRW-410) and the merge-order constraint are not in the contract. Contract 7.2 says an observed conflict is the measured exception to a first in, first out merge order (D-16); the sweep, the ledger, the drift and the constraint are how this build measures that exception and says it. The constraint is advisory: the merge lane's own order and its checks are unchanged, and nothing refuses a merge for it. diff --git a/internal/relay/acceptance/stand.go b/internal/relay/acceptance/stand.go index a97c3d0597..e1240e53b2 100644 --- a/internal/relay/acceptance/stand.go +++ b/internal/relay/acceptance/stand.go @@ -96,6 +96,21 @@ func StandOf(ctx context.Context, q store.Querier, acceptanceID, relationshipID return stands[0], nil } +// Chain is every head an acceptance has stood on, newest first: the head of each valid base refresh +// recorded for it (Stands), then its own accepted head. It is the one definition of "the same result": +// a head in the chain is a head the plan still accepts for the node, and a head outside it was replaced. +func Chain(ctx context.Context, q store.Querier, acceptanceID, relationshipID string, generation int64, head string) ([]string, error) { + stands, err := Stands(ctx, q, acceptanceID, relationshipID, generation) + if err != nil { + return nil, err + } + out := make([]string, 0, len(stands)+1) + for _, st := range stands { + out = append(out, st.Head) + } + return append(out, head), nil +} + // ActiveForRelationship is the newest active acceptance of a relationship, the row the merge train // requires before it carries a member. found is false when the relationship has none. type Active struct { @@ -130,6 +145,78 @@ func ActiveForRelationship(ctx context.Context, q store.Querier, relationshipID return a, true, nil } +// ActiveWithLiveGeneration is ActiveForRelationship with the relationship's own live execution +// generation taken in the same statement, so the acceptance and the generation are one reading. The +// generation moves when a generation is opened, withdrawn or accepted over, and the acceptance moves +// when one is accepted: a correction gate that read the two apart could see neither move, and would let +// a node under correction be judged on the head its old acceptance stands on (CRW-906). +// +// The acceptance is read exactly as ActiveForRelationship reads it, and the generation comes from the +// relationship the acceptance names, in the same statement. A store whose relationships row is absent +// (a fixture that seeded only the acceptance, as some tests do) answers the acceptance's own +// generation, which is the reading that says no correction is open. found is false when the +// relationship has no active acceptance; the live generation is then read on its own, because no +// correction gate turns on it there (the caller's own "no acceptance" refusal wins), and a store that +// predates the zone table has no acceptance at all. +func ActiveWithLiveGeneration(ctx context.Context, q store.Querier, relationshipID string) (active Active, generation int64, found bool, err error) { + present, err := tableExists(ctx, q, "dag_acceptances") + if err != nil || !present { + return Active{}, 0, false, err + } + row := q.QueryRowContext(ctx, "SELECT a.acceptance_id, a.event_id, a.head_sha, a.revision_hash, a.execution_generation,"+ + " COALESCE(r.execution_generation, a.execution_generation)"+ + " FROM dag_acceptances a LEFT JOIN relationships r ON r.relationship_id = a.relationship_id"+ + " WHERE a.relationship_id = ? AND a.state = 'active' ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", relationshipID) + var head sql.NullString + if err := row.Scan(&active.AcceptanceID, &active.EventID, &head, &active.RevisionHash, &active.Generation, &generation); err != nil { + if errors.Is(err, sql.ErrNoRows) { + live, liveErr := liveGeneration(ctx, q, relationshipID) + return Active{}, live, false, liveErr + } + return Active{}, 0, false, err + } + active.HeadSHA = head.String + return active, generation, true, nil +} + +// liveGeneration is the generation a relationship stands on now, for the relationship that has no active +// acceptance. 0 means the store holds no such relationship. +func liveGeneration(ctx context.Context, q store.Querier, relationshipID string) (int64, error) { + var generation int64 + if err := q.QueryRowContext(ctx, "SELECT execution_generation FROM relationships WHERE relationship_id = ?", relationshipID).Scan(&generation); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return 0, nil + } + return 0, err + } + return generation, nil +} + +// UnderCorrection is the CRW-906 gate: whether the relationship's live execution generation is later +// than the generation its active acceptance stands on. A correction generation is then open (opened by +// hand, a ruling or a decision reply) and has not been accepted over yet, so the node is under +// correction and its old head is not a merge candidate: the defect the correction answers was found in +// the accepted result, and carrying that head to the base would land exactly what is being repaired. +// +// The stand generation counts a recorded base refresh (StandOf), so a generation that only carried the +// accepted head onto a moved base is not a correction: it moves the stand generation with it and the +// member may ride again. A withdrawn correction generation moves the live generation back to the +// generation the acceptance stands on, and the old head may ride again as well. +// +// under is false when the relationship has no active acceptance: that is the caller's own refusal, not +// this one, so the two are told apart. A store that predates the acceptance zone answers false. +func UnderCorrection(ctx context.Context, q store.Querier, relationshipID string) (under bool, live, stand int64, err error) { + active, live, found, err := ActiveWithLiveGeneration(ctx, q, relationshipID) + if err != nil || !found { + return false, live, 0, err + } + reading, err := StandOf(ctx, q, active.AcceptanceID, relationshipID, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) + if err != nil { + return false, live, 0, err + } + return live > reading.Generation, live, reading.Generation, nil +} + // RulingHead is the head a ruling fixed for an event: dag_verified_heads.head_sha for that event // (CRW-742), or the acceptance's own head when the event has no row, with source "acceptance". func RulingHead(ctx context.Context, q store.Querier, eventID, acceptanceHead string) (head, source string, err error) { diff --git a/internal/relay/acceptance/undercorrection_test.go b/internal/relay/acceptance/undercorrection_test.go new file mode 100644 index 0000000000..54aaa5312a --- /dev/null +++ b/internal/relay/acceptance/undercorrection_test.go @@ -0,0 +1,154 @@ +package acceptance + +import ( + "context" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// CRW-906 generation 2: the correction gate both the merge train and the single-lane judgement read. A +// relationship whose live execution generation is later than the generation its active acceptance stands +// on has a correction open over an accepted result that has not been accepted over yet, and the accepted +// head is not a merge candidate. These tests pin the reading on temporary stores, including the two +// cases that must NOT read as a correction: a recorded base refresh (which moves the stand generation +// with the live one) and a withdrawn generation (which moves the live generation back). + +// ucStore is a temporary store with one relationship and its acceptance, at the generations given. +func ucStore(t *testing.T, relationship string, acceptanceGeneration, liveGeneration int64) store.Querier { + t.Helper() + s := openStore(t) + ctx := context.Background() + if _, err := s.DB.ExecContext(ctx, "INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', 'task-parent', 'host-p', 'task-child', 'host-c', ?, '[]', '[\"task-parent\"]', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, liveGeneration); err != nil { + t.Fatal(err) + } + if _, err := s.DB.ExecContext(ctx, "INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-1','p','n','m',?,?,'ev-1','rev-1','c','verified','head-1','owner/repo',1,'bound','t','{}','task-parent',0,'2026-10-01T00:00:00Z','active')", + relationship, acceptanceGeneration); err != nil { + t.Fatal(err) + } + q := s.Querier(ctx) + return q +} + +// TestUnderCorrectionReadsTheLiveGenerationAgainstTheStand: the gate is exactly "the live generation is +// later than the generation the acceptance stands on". +func TestUnderCorrectionReadsTheLiveGenerationAgainstTheStand(t *testing.T) { + t.Run("the acceptance stands on the live generation", func(t *testing.T) { + q := ucStore(t, "rel-1", 1, 1) + under, live, stand, err := UnderCorrection(context.Background(), q, "rel-1") + if err != nil { + t.Fatal(err) + } + if under || live != 1 || stand != 1 { + t.Fatalf("UnderCorrection = under %v, live %d, stand %d; want false, 1, 1", under, live, stand) + } + }) + t.Run("a correction generation is open", func(t *testing.T) { + q := ucStore(t, "rel-1", 1, 2) + under, live, stand, err := UnderCorrection(context.Background(), q, "rel-1") + if err != nil { + t.Fatal(err) + } + if !under || live != 2 || stand != 1 { + t.Fatalf("UnderCorrection = under %v, live %d, stand %d; want true, 2, 1", under, live, stand) + } + }) + t.Run("the acceptance moved past the correction generation", func(t *testing.T) { + // dag-accept --supersedes of the corrected result: both moved, so nothing is under correction + q := ucStore(t, "rel-1", 2, 2) + under, _, _, err := UnderCorrection(context.Background(), q, "rel-1") + if err != nil { + t.Fatal(err) + } + if under { + t.Fatal("an accepted-over correction still reads under correction") + } + }) + t.Run("the relationship has no active acceptance", func(t *testing.T) { + s := openStore(t) + ctx := context.Background() + if _, err := s.DB.ExecContext(ctx, "INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES ('rel-none', 'ISS-1', 'active', 'task-parent', 'host-p', 'task-child', 'host-c', 2, '[]', '[\"task-parent\"]', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')"); err != nil { + t.Fatal(err) + } + q := s.Querier(ctx) + under, live, stand, err := UnderCorrection(ctx, q, "rel-none") + if err != nil { + t.Fatal(err) + } + // no acceptance is the caller's own refusal, not this one: the gate must not claim it + if under || stand != 0 || live != 2 { + t.Fatalf("UnderCorrection with no acceptance = under %v, live %d, stand %d; want false, 2, 0", under, live, stand) + } + }) +} + +// TestUnderCorrectionIsNotARecordedBaseRefresh: a generation that only carried the accepted head onto a +// moved base is not a correction. The refresh moves the stand generation to the live one, so the member +// may ride again on the refreshed head, exactly as dag-base-refresh intends. +func TestUnderCorrectionIsNotARecordedBaseRefresh(t *testing.T) { + q := ucStore(t, "rel-1", 1, 2) + ctx := context.Background() + head := "head-refreshed" + event, revision := "ev-refresh", "rev-refreshed" + id := RefreshDigest("acc-1", "rel-1", 2, event, revision, head, "owner/repo", "dev", "base-0", "{}", "[]") + if _, err := q.ExecContext(ctx, "INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?,?,1,?,?,?,?,?,?,?,?,?,?,?,?,?)", + id, "acc-1", "rel-1", int64(2), event, revision, head, "owner/repo", "dev", "base-0", "{}", "[]", "task-parent", int64(0), "2026-10-01T00:00:00Z"); err != nil { + t.Fatal(err) + } + under, live, stand, err := UnderCorrection(ctx, q, "rel-1") + if err != nil { + t.Fatal(err) + } + if under || live != 2 || stand != 2 { + t.Fatalf("UnderCorrection after a base refresh = under %v, live %d, stand %d; want false, 2, 2", under, live, stand) + } +} + +// TestActiveWithLiveGenerationReadsTheAcceptanceAsBefore: the reading is the acceptance +// ActiveForRelationship answered, with the live generation taken in the same statement, and a store +// whose relationships row is absent (a fixture that seeded only the acceptance) answers the +// acceptance's own generation rather than claiming a correction. +func TestActiveWithLiveGenerationReadsTheAcceptanceAsBefore(t *testing.T) { + q := ucStore(t, "rel-1", 1, 3) + active, live, found, err := ActiveWithLiveGeneration(context.Background(), q, "rel-1") + if err != nil || !found { + t.Fatalf("ActiveWithLiveGeneration = %v %v %v", active, found, err) + } + if active.AcceptanceID != "acc-1" || active.EventID != "ev-1" || active.HeadSHA != "head-1" || active.RevisionHash != "rev-1" || active.Generation != 1 { + t.Fatalf("the acceptance read = %+v", active) + } + if live != 3 { + t.Fatalf("live generation = %d, want 3", live) + } + plain, found, err := ActiveForRelationship(context.Background(), q, "rel-1") + if err != nil || !found { + t.Fatal(err) + } + if plain != active { + t.Fatalf("the acceptance read apart from the generation = %+v, want %+v", plain, active) + } + // an absent relationship row: the acceptance's own generation stands in, so no correction is claimed + s := openStore(t) + ctx := context.Background() + if _, err := s.DB.ExecContext(ctx, "INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-2','p','n','m','rel-orphan',4,'ev-2','rev-2','c','verified','head-2','owner/repo',1,'bound','t','{}','task-parent',0,'2026-10-01T00:00:00Z','active')"); err != nil { + t.Fatal(err) + } + oq := s.Querier(ctx) + _, live, found, err = ActiveWithLiveGeneration(ctx, oq, "rel-orphan") + if err != nil || !found { + t.Fatalf("an acceptance with no relationship row = %v %v", found, err) + } + if live != 4 { + t.Fatalf("live generation without a relationships row = %d, want the acceptance's own 4", live) + } + under, _, _, err := UnderCorrection(ctx, oq, "rel-orphan") + if err != nil || under { + t.Fatalf("UnderCorrection without a relationships row = %v %v, want false", under, err) + } +} diff --git a/internal/relay/commitid/commitid.go b/internal/relay/commitid/commitid.go new file mode 100644 index 0000000000..acc734b226 --- /dev/null +++ b/internal/relay/commitid/commitid.go @@ -0,0 +1,44 @@ +// Package commitid is the one definition of when two stored commit identities name the same commit. Go code +// and SQL both use it: Same is the Go definition, and the SQL function crw_same_commit(a, b) is registered +// with the sqlite driver here, so a query compares a head column through the same rule instead of trimming +// and lower-casing it by hand (the SQL trim only removes ASCII spaces, which is why a head stored with a +// no-break or ideographic space was missed). +package commitid + +import ( + "database/sql/driver" + "strings" + + "modernc.org/sqlite" +) + +// SQLFunction is the name of the scalar function a query calls to compare two commit identities. +const SQLFunction = "crw_same_commit" + +// Same reports whether a and b name the same commit: each is trimmed of surrounding Unicode whitespace and +// lower-cased, and an empty identity names no commit, so it is equal to nothing. +func Same(a, b string) bool { + a = strings.ToLower(strings.TrimSpace(a)) + b = strings.ToLower(strings.TrimSpace(b)) + return a != "" && a == b +} + +// Register installs the function on one driver instance. The store opens its databases through a driver it +// builds itself, and a function registered on another instance is not visible to those connections, so each +// place that builds a driver calls Register on it. +func Register(d *sqlite.Driver) error { + return d.RegisterDeterministicScalarFunction(SQLFunction, 2, func(_ *sqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + if Same(commitText(args[0]), commitText(args[1])) { + return int64(1), nil + } + return int64(0), nil + }) +} + +// commitText is the text of a SQL argument. Only TEXT names a commit: a BLOB, a number or NULL names none. +func commitText(v driver.Value) string { + if x, ok := v.(string); ok { + return x + } + return "" +} diff --git a/internal/relay/commitid/commitid_test.go b/internal/relay/commitid/commitid_test.go new file mode 100644 index 0000000000..aaf112d76e --- /dev/null +++ b/internal/relay/commitid/commitid_test.go @@ -0,0 +1,65 @@ +package commitid_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/commitid" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" + _ "github.com/thisisjun786/codex-relay-workflow/internal/testsupport" +) + +// Each row is a pair of commit identities and whether they name the same commit. Surrounding Unicode whitespace +// is removed and the case is folded before comparing; an empty identity names no commit. The SQL function must +// answer as Same does for every row, on a store connection. +var commitPairs = []struct { + a, b string + same bool +}{ + {"head-a", "head-a", true}, + {"HEAD-A", "head-a", true}, + {" head-a ", "head-a", true}, + {"\thead-a\n", "head-a", true}, + {"head-a\v", "head-a", true}, + {"\fhead-a\r", "head-a", true}, + {"\u00a0head-a", "head-a", true}, + {"head-a\u0085", "head-a", true}, + {"\u2003head-a", "head-a", true}, + {"head-a\u3000", "head-a", true}, + {"head-a", "head-b", false}, + {"", "", false}, + {" ", " ", false}, + {"\u3000", "\u3000", false}, + {"head-a", "", false}, +} + +func TestSameDefinesOneCommitIdentity(t *testing.T) { + for _, p := range commitPairs { + if got := commitid.Same(p.a, p.b); got != p.same { + t.Fatalf("Same(%q, %q) = %v, want %v", p.a, p.b, got, p.same) + } + } +} + +func TestTheStoreFunctionAnswersAsSameDoes(t *testing.T) { + ctx := context.Background() + s, err := store.Open(ctx, filepath.Join(t.TempDir(), "relay.sqlite3"), "") + if err != nil { + t.Fatal(err) + } + defer s.Close() + for _, p := range commitPairs { + var got int64 + if err := s.DB.QueryRowContext(ctx, "SELECT "+commitid.SQLFunction+"(?, ?)", p.a, p.b).Scan(&got); err != nil { + t.Fatalf("%s(%q, %q): %v", commitid.SQLFunction, p.a, p.b, err) + } + if (got == 1) != p.same { + t.Fatalf("%s(%q, %q) = %d, want same=%v", commitid.SQLFunction, p.a, p.b, got, p.same) + } + } + var null int64 + if err := s.DB.QueryRowContext(ctx, "SELECT "+commitid.SQLFunction+"(NULL, 'head-a')").Scan(&null); err != nil || null != 0 { + t.Fatalf("a NULL identity = %d, %v; want 0", null, err) + } +} diff --git a/internal/relay/commitid/commitid_text_test.go b/internal/relay/commitid/commitid_text_test.go new file mode 100644 index 0000000000..cbe664e37d --- /dev/null +++ b/internal/relay/commitid/commitid_text_test.go @@ -0,0 +1,39 @@ +package commitid_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/commitid" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" + _ "github.com/thisisjun786/codex-relay-workflow/internal/testsupport" +) + +// Only TEXT names a commit: a BLOB, a number or NULL names none, whatever its bytes or digits spell. +func TestOnlyTextNamesACommitInSQL(t *testing.T) { + ctx := context.Background() + s, err := store.Open(ctx, filepath.Join(t.TempDir(), "relay.sqlite3"), "") + if err != nil { + t.Fatal(err) + } + defer s.Close() + cases := []struct { + name string + a, b any + }{ + {"a BLOB of the head's bytes", []byte("head-a"), "head-a"}, + {"a BLOB on both sides", []byte("head-a"), []byte("head-a")}, + {"an integer against its digits", int64(1), "1"}, + {"NULL", nil, "head-a"}, + } + for _, c := range cases { + var got int64 + if err := s.DB.QueryRowContext(ctx, "SELECT "+commitid.SQLFunction+"(?, ?)", c.a, c.b).Scan(&got); err != nil { + t.Fatalf("%s: %v", c.name, err) + } + if got != 0 { + t.Fatalf("%s named a commit: %s(%v, %v) = %d, want 0", c.name, commitid.SQLFunction, c.a, c.b, got) + } + } +} diff --git a/internal/relay/dagsched/baserefresh.go b/internal/relay/dagsched/baserefresh.go index b67d581e07..cbb9afd3dc 100644 --- a/internal/relay/dagsched/baserefresh.go +++ b/internal/relay/dagsched/baserefresh.go @@ -57,12 +57,6 @@ func refreshDigest(acc, relationship string, generation int64, event, revision, return acceptance.RefreshDigest(acc, relationship, generation, event, revision, head, baseRepository, baseRef, baseTip, proofJSON, resolvedJSON) } -// validStands are the base refreshes recorded for an acceptance that digest to their ids, newest first. The table arrived after the first zone, so a store opened read-only that predates it has none; a row -// that does not digest to its id is ignored. The reading itself lives in internal/relay/acceptance. -func (s *Scheduler) validStands(ctx context.Context, q store.Querier, a Acceptance) ([]acceptanceStand, error) { - return acceptance.Stands(ctx, q, a.AcceptanceID, a.RelationshipID, a.ExecutionGeneration) -} - // standOf reads what an acceptance stands on now: the newest valid base refresh recorded for it, else its own. func (s *Scheduler) standOf(ctx context.Context, q store.Querier, a Acceptance) (acceptanceStand, error) { return acceptance.StandOf(ctx, q, a.AcceptanceID, a.RelationshipID, a.ExecutionGeneration, a.EventID, a.RevisionHash, a.HeadSHA) @@ -71,15 +65,7 @@ func (s *Scheduler) standOf(ctx context.Context, q store.Querier, a Acceptance) // stoodOn is every head the acceptance has stood on: its own and the head of each valid base refresh recorded for it, newest first. A merge turn of the node, or a check of its pull request, made for any // of them is the node's own (the newest is what it stands on now; the others are what it stood on before a record moved it). func (s *Scheduler) stoodOn(ctx context.Context, q store.Querier, a Acceptance) ([]string, error) { - stands, err := s.validStands(ctx, q, a) - if err != nil { - return nil, err - } - heads := make([]string, 0, len(stands)+1) - for _, st := range stands { - heads = append(heads, st.Head) - } - return append(heads, a.HeadSHA), nil + return acceptance.Chain(ctx, q, a.AcceptanceID, a.RelationshipID, a.ExecutionGeneration, a.HeadSHA) } // decodeRefreshProof reads back the proof and the resolved paths a record stores. diff --git a/internal/relay/dagsched/baserefresh_scenario_test.go b/internal/relay/dagsched/baserefresh_scenario_test.go index fc9c085ee3..1db9dc6133 100644 --- a/internal/relay/dagsched/baserefresh_scenario_test.go +++ b/internal/relay/dagsched/baserefresh_scenario_test.go @@ -156,8 +156,8 @@ func (s *refreshScenario) slotHeld() bool { } // The state CRW-430 is about, on the code without a record of the refresh: the head the parent accepted is contained in dev, the merged mark sits on generation 2, so the node reads is_ancestor true, -// integrated false and mark_present false, holds its slot, and the hand-opened generation is refused by dag-correct because the result is current. Nothing here is a defect to fix by itself: the -// relay does not integrate a node on a generation's mark until a base refresh is recorded. +// integrated false and mark_present false, holds its slot, and dag-correct binds nothing from the hand-opened generation. Nothing here is a defect to fix by itself: the relay does not integrate a node +// on a generation's mark until a base refresh is recorded. func TestWithoutARecordTheRefreshGenerationLeavesTheNodeUnintegrated(t *testing.T) { t.Parallel() s := newRefreshScenario(t) @@ -174,8 +174,25 @@ func TestWithoutARecordTheRefreshGenerationLeavesTheNodeUnintegrated(t *testing. if n := s.read("g").node("I"); n.State == StateIntegrated || n.Disposition == DispDone && n.Reason == DoneIntegrated { t.Fatalf("I = %+v, want it not integrated", n) } + // CRW-906 admits a generation opened by hand for a correction of a current accepted result, so this one is + // refused by what it is not: it was opened for a base refresh (its request id is not the one dag-correct + // --prepare prints) and no manifest is named for it. dag-base-refresh is what records it. _, err = s.sched.RecordCorrection(context.Background(), "g", "I", "parent", "") - if refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "is not stale") { - t.Fatalf("dag-correct = %v, want disposition_conflict naming that the result is not stale", err) + if refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "--manifest-digest") { + t.Fatalf("dag-correct = %v, want disposition_conflict asking for the manifest", err) + } + roots, err := relationshipRoots(context.Background(), s.s.Q(context.Background()), s.rid) + if err != nil || len(roots) == 0 { + t.Fatalf("the artifact roots of the child: %v %v", roots, err) + } + prepared, err := s.sched.PrepareCorrection(context.Background(), "g", "I", "parent", ManifestInput{Base: &BaseRef{Repository: s.target(), Ref: "dev"}, RuleVersion: s.request(false).RuleVersion}, VerifyOptions{ArtifactRoots: roots}) + if err != nil { + t.Fatalf("prepare I: %v", err) + } + if _, err := s.sched.RecordCorrection(context.Background(), "g", "I", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "not opened for this manifest") { + t.Fatalf("dag-correct with a prepared manifest = %v, want the generation not opened for it", err) + } + if n := s.count("SELECT COUNT(*) FROM dag_node_executions WHERE relationship_id = ? AND execution_generation = 2", s.rid); n != 0 { + t.Fatalf("dag-correct bound %d executions of the refresh generation", n) } } diff --git a/internal/relay/dagsched/correction.go b/internal/relay/dagsched/correction.go index 6b6c8944a9..4837a08305 100644 --- a/internal/relay/dagsched/correction.go +++ b/internal/relay/dagsched/correction.go @@ -258,7 +258,9 @@ func (s *Scheduler) RecordCorrection(ctx context.Context, plan, node, actor, sup if reason.String == delivery.DecisionReply { return s.recordDecisionOpened(txCtx, tx, plan, snap, n, rel, actor, suppliedDigest, &out) } - if reason.String != "needs_changes_revision" { + // the generation's own reason says how it was opened: a decision reply has its own binder below, and a correction was opened either by a needs_changes ruling or by hand. A correction of an accepted + // result that is still current has no ruling to open it (the writer refuses a second ruling on the accepted head), so it carries the reason the coordinator gave it (revalidation.go). + if !correctionOpenReason(reason.String) { return refuse(contract.RefusalDispositionConflict, "generation %d of %s was not opened by a needs_changes ruling or a decision reply (%q)", rel.Generation, rel.ID, reason.String) } // the ruling on the previous generation's head must have opened this generation diff --git a/internal/relay/dagsched/correction_accepted_test.go b/internal/relay/dagsched/correction_accepted_test.go new file mode 100644 index 0000000000..98ee06e23c --- /dev/null +++ b/internal/relay/dagsched/correction_accepted_test.go @@ -0,0 +1,436 @@ +package dagsched + +import ( + "context" + "database/sql" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" +) + +// CRW-906: the correction route of a result that was accepted and is still current. A bundle review found +// a blocking defect in an accepted member before it landed; the parent opened the next generation by hand +// and the child fixed it, but nothing could record that generation (the accepted result is not stale, so +// the hand-opened route refused it) and dag-accept refused the unrecorded generation, so the corrected +// head could not be accepted or merged. recordHandOpened now admits that generation when the reason it was +// opened under says a correction (needs_changes_revision, the generation-open default, or +// accepted_result_correction, which names the route) and every check it already had still holds: the +// generation right after the one the acceptance stands on, the manifest prepared for the node's current +// slice and criteria, opened under CorrectionRequestID, bound to a dispatch turn, and the inputs as they +// stand now. The acceptance stays active until dag-accept --supersedes takes the new result, the route is +// noted in the generation's own reason column, and no schema, refusal name, CLI flag or specs.json entry +// is added. + +// acOpenByHand is the coordinator's act with the relay's own commands, under a named reason: +// generation-open under the request id dag-correct --prepare printed, then generation-bind to the turn that +// carried the instruction line to the child. +func acOpenByHand(t *testing.T, k *releaseKit, relationship, request, reason string, generation int64, bind bool) { + t.Helper() + reg := ®istry.Registry{Store: k.s} + if _, err := reg.OpenGeneration(context.Background(), relationship, request, reason, sql.NullString{}); err != nil { + t.Fatalf("generation-open --reason %s: %v", reason, err) + } + if bind { + if _, err := reg.BindAnchor(context.Background(), relationship, generation, "turn-dispatch-"+relationship, "dispatch_receipt"); err != nil { + t.Fatalf("generation-bind: %v", err) + } + } +} + +// acExecution is the execution row of a node at one generation, as text, or "" when there is none: the +// kind, the manifest it is bound to and the request id it was opened under. +func acExecution(k *releaseKit, node string, generation int64) string { + k.t.Helper() + return rvRows(k, "SELECT kind, manifest_digest, COALESCE(managed_request_id, '') FROM dag_node_executions WHERE node_id = ? AND execution_generation = ?", node, generation) +} + +// acReason is the reason the generation row carries: what the record notes of the route without a schema +// change. +func acReason(k *releaseKit, relationship string, generation int64) string { + k.t.Helper() + var reason sql.NullString + if err := k.s.DB.QueryRow("SELECT reason FROM generations WHERE relationship_id = ? AND execution_generation = ?", relationship, generation).Scan(&reason); err != nil { + k.t.Fatal(err) + } + return reason.String +} + +// acPointer is the generation the relationship stands on. +func acPointer(k *releaseKit, relationship string) int64 { + k.t.Helper() + var generation int64 + if err := k.s.DB.QueryRow("SELECT execution_generation FROM relationships WHERE relationship_id = ?", relationship).Scan(&generation); err != nil { + k.t.Fatal(err) + } + return generation +} + +// Criterion c1 and the first case of c2: an accepted, current and not landed node whose generation was +// opened by hand for a correction is recorded as a correction (refused before this change), and the result +// the child reports in that generation replaces the acceptance through dag-accept --supersedes. The child +// is the same one, the acceptance stays active until then, and the siblings keep their records. +func TestAnAcceptedCurrentResultIsCorrectedByAHandOpenedGeneration(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + siblings := map[string]string{"R": rvRecords(k, "sr", "R"), "A": rvRecords(k, "sr", "A"), "C": rvRecords(k, "sr", "C")} + fleet := k.rvFleet() + + // the premise: B was accepted and nothing above it changed, so its result is current and the relay's + // verdict writer has no review to open a second ruling on its head with + if n := k.read("sr").node("B"); n.Disposition != DispDone || n.Reason != DoneAccepted { + t.Fatalf("B = %+v, want it accepted and current", n) + } + prepared := k.rvPrepare("sr", "B") + if prepared.DispatchRequestID != CorrectionRequestID("sr", "B", prepared.ManifestDigest, 2) { + t.Fatalf("prepare = %+v", prepared) + } + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + + res, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if err != nil || res.Replayed || res.Generation != 2 || res.RelationshipID != rid || res.OpenedBy != OpenedByGenerationOpen || + res.ManifestDigest != prepared.ManifestDigest || res.DispatchRequestID != prepared.DispatchRequestID || res.DispatchTurnID != "turn-dispatch-"+rid { + t.Fatalf("dag-correct for the accepted current result = %v %+v", err, res) + } + if got := acExecution(k, "B", 2); got != "correction|"+prepared.ManifestDigest+"|"+prepared.DispatchRequestID { + t.Fatalf("the execution row = %q", got) + } + if got := acReason(k, rid, 2); got != "needs_changes_revision" { + t.Fatalf("the generation's reason = %q, want the one it was opened under", got) + } + // the acceptance stays active until the corrected result replaces it + var active string + if err := k.s.DB.QueryRow("SELECT acceptance_id FROM dag_acceptances WHERE plan_id = 'sr' AND node_id = 'B' AND state = 'active'").Scan(&active); err != nil || active != accepted["B"].AcceptanceID { + t.Fatalf("the active acceptance = %q (%v), want %q", active, err, accepted["B"].AcceptanceID) + } + // recording again is a replay that says how the generation was opened + if again, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); err != nil || !again.Replayed || again.OpenedBy != OpenedByGenerationOpen { + t.Fatalf("replay = %v %+v", err, again) + } + + // the child reports in the bound generation and the parent accepts the corrected result in place of the first one + k.rvReportGeneration(rid, "B", 2, releaseCriteriaDigest()) + second, err := k.accept("sr", "B", AcceptInput{Supersedes: accepted["B"].AcceptanceID}) + if err != nil || second.SupersededID != accepted["B"].AcceptanceID || second.Generation != 2 || second.AcceptanceID == accepted["B"].AcceptanceID { + t.Fatalf("dag-accept --supersedes of the corrected result = %v %+v", err, second) + } + if n := k.read("sr").node("B"); n.Disposition != DispDone || n.Reason != DoneAccepted { + t.Fatalf("B after the correction was accepted = %+v", n) + } + // the correction went to the child B already had and touched no other node + if got := k.count("SELECT COUNT(DISTINCT relationship_id) FROM dag_node_executions WHERE plan_id = 'sr' AND node_id = 'B'"); got != 1 { + t.Fatalf("the correction made %d relationships for B", got) + } + if got := k.rvFleet(); !strings.HasPrefix(got, strings.Split(fleet, " generations=")[0]) { + t.Fatalf("the correction made a child:\nbefore %s\nafter %s", fleet, got) + } + for node, before := range siblings { + if now := rvRecords(k, "sr", node); now != before { + t.Fatalf("the records of the unrelated node %s changed:\nbefore %s\nafter %s", node, before, now) + } + } +} + +// Criterion c1: the second reason the route admits names it. A generation opened with +// accepted_result_correction is a generation-open reason like any other, dag-correct records it as a +// correction, and the generation's own reason column is what notes the route (no schema change). +func TestTheReasonThatNamesTheRouteIsOpenableAndRecorded(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "accepted_result_correction", 2, true) + res, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if err != nil || res.Replayed || res.OpenedBy != OpenedByGenerationOpen || res.ManifestDigest != prepared.ManifestDigest { + t.Fatalf("dag-correct under accepted_result_correction = %v %+v", err, res) + } + if got := acReason(k, rid, 2); got != "accepted_result_correction" { + t.Fatalf("the generation's reason = %q, want accepted_result_correction", got) + } + // the reason is one literal in two packages: dagsched declares the constant and the registry's closed + // list is what generation-open validates against, and the registry cannot import dagsched. This pins the + // two to each other, so a rename in one place cannot leave the other accepting a value nothing records. + for _, accepted := range []string{AcceptedResultCorrection, "needs_changes_revision", "initial_assignment"} { + if _, err := (®istry.Registry{Store: k.s}).OpenGeneration(context.Background(), "rel-none", "request-"+accepted, accepted, sql.NullString{}); refusalReason(err) == "unknown_generation" { + t.Fatalf("the registry refuses the reason %q that generation-open must accept", accepted) + } + } + + // a generation opened for this route and never sent is withdrawn like every other generation opened by + // hand: the withdrawal's own rule is about generation-open reasons, and this is one + k2, accepted2 := rvSettledSharedRoot(t) + rid2 := accepted2["C"].RelationshipID + acOpenByHand(t, k2, rid2, "correction-"+rid2, "accepted_result_correction", 2, false) + out, err := k2.sched.WithdrawGeneration(context.Background(), "sr", "C", "parent", WithdrawInput{Relationship: rid2, Generation: 2, Reason: "the correction is not needed"}) + if err != nil || out.Replayed || out.RestoredGeneration != 1 || out.Generation != 2 { + t.Fatalf("dag-generation-withdraw = %v %+v", err, out) + } + if got := acPointer(k2, rid2); got != 1 { + t.Fatalf("the relationship stands on generation %d after the withdrawal, want 1", got) + } +} + +// The second case of c2: every check the hand-opened route already had still refuses, on the same accepted +// current node, and a refusal writes no execution row. +func TestAHandOpenedGenerationOfAnAcceptedCurrentResultIsStillRefused(t *testing.T) { + t.Parallel() + bound := func(k *releaseKit, generation int64) string { return acExecution(k, "B", generation) } + t.Run("an open reason that states no correction", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "initial_assignment", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), `"initial_assignment"`) { + // the assignment itself is not a correction of anything: the recording refuses the generation by the reason it was opened under + t.Fatalf("the refusal does not name the reason the generation was opened under: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("a node that landed", func(t *testing.T) { + k, a := rvMerged(t) + acOpenByHand(t, k.releaseKit, a.Acceptance.RelationshipID, "correction-"+a.Acceptance.RelationshipID, "needs_changes_revision", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "g", "I", "parent", dig("a manifest of the landed node")); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "landed") { + t.Fatalf("the refusal does not say why: %v", err) + } + }) + t.Run("a generation that is not the one after the acceptance's", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + first := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, first.DispatchRequestID, "needs_changes_revision", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", first.ManifestDigest); err != nil { + t.Fatalf("the first correction: %v", err) + } + next := k.rvPrepare("sr", "B") + if !strings.Contains(next.Instruction, "generation 3") { + t.Fatalf("the second prepare = %+v", next) + } + acOpenByHand(t, k, rid, next.DispatchRequestID, "needs_changes_revision", 3, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", next.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("a second correction beside the open one = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "already open") { + t.Fatalf("the refusal does not say why: %v", err) + } else if got := bound(k, 3); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("a manifest prepared for another version of the node", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.invRevise("sr", "B", "sr-r2", invTitle("B, as the revision specifies it")) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "open no further generation") { + t.Fatalf("the refusal does not say what to do: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("a generation nobody bound", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, false) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "generation-bind") { + t.Fatalf("the refusal does not name the way on: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("no manifest named", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", ""); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "--manifest-digest") { + t.Fatalf("the refusal does not ask for the manifest: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) +} + +// The reason a generation was opened under is what states the correction, and the stale route keeps its own +// Criterion c1, the safety bound of the accepted-current route: a correction is not recorded over an +// Criterion c1, generation 2, d2: a live bundle is the other way the accepted head may already be on the +// base. The parent merges a verified bundle on the forge and records it with merge-train-land afterwards, +// so between those two the bundle's members still hold or wait in the lane while the forge already carries +// the head. Recording a correction in that window names a head the forge may already have merged. +func TestACorrectionIsNotRecordedOverALiveBundleCarryingTheHead(t *testing.T) { + t.Parallel() + for _, kind := range []string{"opened", "verified"} { + t.Run("a bundle that is "+kind, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + // a live bundle carries this node's accepted head as a member + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-" + kind + "', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-"+kind+"', 1, 'turn-1', 5, ?, 'head-b')", rid) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-"+kind+"', 1, ?, 'parent', '{}', 't')", kind) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a %s bundle carrying the head = %v, want disposition_conflict", kind, err) + } + if !strings.Contains(err.Error(), "trn-"+kind) || !strings.Contains(err.Error(), "merge-train-close") { + t.Fatalf("the refusal does not name the bundle and the way on: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + }) + } + t.Run("a bundle that landed with the head refuses the correction", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-done', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-done', 1, 'turn-1', 5, ?, 'head-b')", rid) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-done', 1, 'landed', 'parent', '{}', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a landed bundle that carries the head = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "trn-done") { + t.Fatalf("the refusal does not name the landed bundle: %v", err) + } + }) +} + +// Criterion c1, generation 2, d1: the guard resolves the merge turn by every identity a claim may carry. +// --relationship and --pr are both optional on merge-turn-request, and a successful merge-turn-check has +// already authorized the merge of H before the correction is recorded. A guard that read only the +// relationship would miss that turn and admit the record, and the later Land refusal cannot undo a merge +// the forge was already authorized to make. +func TestACorrectionIsNotRecordedOverATurnThatNamesNoRelationship(t *testing.T) { + t.Parallel() + for _, identity := range []string{"pull request", "head"} { + t.Run("a merging turn identified by its "+identity, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // the acceptance records the repository, the pull request and the head the turn may name + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b', pr_number = 5 WHERE relationship_id = ?", rid) + // the turn names no relationship: the guard must resolve it from the pull request or the head + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-pr-only', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', NULL, ?, 'head-b', 'merging', 1, 't', 't')", + prNumberFor(identity)) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn identified by its %s = %v, want disposition_conflict", identity, err) + } + if !strings.Contains(err.Error(), "on its way to the base") || !strings.Contains(err.Error(), "mtn-pr-only") { + t.Fatalf("the refusal does not name the turn and the reason: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + }) + } +} + +// prNumberFor is the pull request the turn records for the case named: the acceptance's, or NULL so the +// head is the only identity left. +func prNumberFor(identity string) any { + if identity == "pull request" { + return int64(5) + } + return nil +} + +// Criterion c1, the safety bound of the accepted-current route: a correction is not recorded over an +// accepted head a merge turn of the same relationship is already carrying to the base. The relay reads the +// forge, not the merge, so a turn that is merging or of unknown effect may already have landed the head +// outside the relay; naming that head as the one being repaired would leave the merge unrecorded and the +// correction unmergeable. The turn is resolved first. +func TestACorrectionIsNotRecordedOverAnAcceptedHeadAlreadyOnItsWayToTheBase(t *testing.T) { + t.Parallel() + for _, state := range []string{"merging", "unknown", "landed"} { + t.Run("a merge turn that is "+state, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-"+state+"', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', ?, ?, ?, 1, 't', 't')", rid, accepted["B"].HeadSHA, state) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a turn that is %s = %v, want disposition_conflict", state, err) + } + if !strings.Contains(err.Error(), "on its way to the base") || !strings.Contains(err.Error(), "mtn-"+state) { + t.Fatalf("the refusal does not name the turn and the reason: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + }) + } + t.Run("a turn that only holds the lane does not block the correction", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // a held turn cannot progress: the lane's own check and land refuse it under correction + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-held', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', ?, ?, 'holding', 1, 't', 't')", rid, accepted["B"].HeadSHA) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); err != nil { + t.Fatalf("a held turn blocked the correction: %v", err) + } + }) +} + +// The reason a generation was opened under is what states the correction, and the stale route keeps its own +// gate: a stale node whose route says correct is recorded under either reason (the new value is a correction +// reason like the default, not a route of its own), and one whose route is anything else is still refused +// whatever reason it carries. This pins that the change admits one case rather than loosening the gate. +func TestTheCorrectionReasonDoesNotLoosenTheStaleRouteGate(t *testing.T) { + t.Parallel() + t.Run("a stale node whose route is correct records under the new reason", func(t *testing.T) { + k, _, ridC, prepared := rvHandKit(t) + if got := rvAction(t, k.read("sr"), "C"); got != rvCorrect { + t.Fatalf("the route of C = %q, want %q: the premise of the stale correction", got, rvCorrect) + } + acOpenByHand(t, k, ridC, prepared.DispatchRequestID, "accepted_result_correction", 2, true) + res, err := k.sched.RecordCorrection(context.Background(), "sr", "C", "parent", prepared.ManifestDigest) + if err != nil || res.Replayed || res.OpenedBy != OpenedByGenerationOpen || res.ManifestDigest != prepared.ManifestDigest { + t.Fatalf("a stale correction under the new reason = %v %+v", err, res) + } + if got := acReason(k, ridC, 2); got != "accepted_result_correction" { + t.Fatalf("the generation's reason = %q", got) + } + }) + t.Run("a stale node whose route is revalidate is refused whatever reason it carries", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + ridA := accepted["A"].RelationshipID + // only A's criteria change: its route is revalidate, which opens no generation + k.rvReregister("sr", "A", "sr-r2", ridA, dig("A's new criteria"), nil) + if got := rvAction(t, k.read("sr"), "A"); got != rvRevalidate { + t.Fatalf("the route of A = %q, want %q: the premise", got, rvRevalidate) + } + prepared := k.rvPrepare("sr", "A") + acOpenByHand(t, k, ridA, prepared.DispatchRequestID, "accepted_result_correction", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "A", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("a revalidation recorded as a correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "revalidate") { + t.Fatalf("the refusal does not name the route: %v", err) + } else if got := acExecution(k, "A", 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) +} diff --git a/internal/relay/dagsched/correction_forge_identity_test.go b/internal/relay/dagsched/correction_forge_identity_test.go new file mode 100644 index 0000000000..560eaeace7 --- /dev/null +++ b/internal/relay/dagsched/correction_forge_identity_test.go @@ -0,0 +1,86 @@ +package dagsched + +import ( + "context" + "strings" + "testing" +) + +// CRW-906 generation 2, round 6 d3: an acceptance written before the forge rule keeps the target it was +// accepted against (a local checkout here) while dag_acceptance_forge holds the owner/name a merge turn is +// requested against. The in-flight guard must match the forge identity. A pull-request turn that names no +// relationship and is already merging is otherwise missed, and the correction would be recorded over a head +// the forge may already have merged. +func TestACorrectionIsNotRecordedOverALegacyAcceptanceTurnOfItsForgeRepository(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // the stored target is a local checkout; the forge identity is the owner/name the turn is requested against + k.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout', head_sha = 'head-b', pr_number = 5 WHERE relationship_id = ?", rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES ((SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?), 'owner/repo', 5)", rid) + // a merging turn of the forge repository for the pull request, naming neither a relationship nor the head + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)" + + " VALUES ('mtn-forge', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', NULL, 5, 'head-other', 'merging', 1, 't', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn of the legacy acceptance's forge repository = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "mtn-forge") { + t.Fatalf("the refusal does not name the turn: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } +} + +// CRW-906 final round, evaluation 6f79654d D2: a live bundle that carries an accepted head through another +// node is still the bundle whose merge may already be on the forge, so a correction of the accepted node is +// refused when the bundle's member holds that head, whichever relationship the member is. +func TestACorrectionIsNotRecordedOverALiveBundleCarryingTheHeadThroughAnotherNode(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-other', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + // the member is another relationship that carries the same head + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-other', 1, 'turn-other', 6, 'rel-other-node', 'head-b')") + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-other', 1, 'opened', 'parent', '{}', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a live bundle carrying the head through another node = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "trn-other") { + t.Fatalf("the refusal does not name the bundle: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } +} + +// CRW-906 final round, delta review ad054763: a legacy acceptance keeps its local target and has no forge row; +// a live bundle that carries its head through its own relationship is still refused, whatever repository the +// bundle's train names. +func TestACorrectionIsNotRecordedOverALiveBundleCarryingTheHeadThroughItsOwnLegacyNode(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-legacy', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-legacy', 1, 'turn-legacy', 5, ?, 'head-b')", rid) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-legacy', 1, 'opened', 'parent', '{}', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a live bundle carrying a legacy node's own head = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "trn-legacy") { + t.Fatalf("the refusal does not name the bundle: %v", err) + } +} diff --git a/internal/relay/dagsched/correction_head_spelling_test.go b/internal/relay/dagsched/correction_head_spelling_test.go new file mode 100644 index 0000000000..b64f648612 --- /dev/null +++ b/internal/relay/dagsched/correction_head_spelling_test.go @@ -0,0 +1,116 @@ +package dagsched + +import ( + "context" + "strings" + "testing" +) + +// headSpellingPads are the surrounding characters a stored head may carry. SameCommit trims all of them; the +// SQL that compares the head must agree with it, so a head stored with any of these is still the same commit. +var headSpellingPads = []struct{ name, pad string }{ + {"clean", ""}, {"no-break space", "\u00a0"}, {"ideographic space", "\u3000"}, {"em space", "\u2003"}, + {"next line", "\u0085"}, {"vertical tab", "\v"}, {"carriage return", "\r"}, +} + +// CRW-906 generation 2, round 7: the in-flight guard of a correction compares a stored merge-turn head with the +// accepted head through the one head definition (SameCommit). A turn that names no relationship is found by its +// head and the acceptance's forge repository alone, so a head stored with a no-break or ideographic space, which +// the SQL trim never removed, must still be found and refuse the correction. +func TestACorrectionIsNotRecordedOverAMergingTurnWhoseHeadIsStoredInAnotherSpelling(t *testing.T) { + t.Parallel() + for _, s := range headSpellingPads { + t.Run(s.name, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // the acceptance is matched by the repository the turn names: its forge identity, as the forge test does + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo' WHERE relationship_id = ?", rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + // the fixture's accepted node has no head of its own, so the test gives the active acceptance one + k.exec("UPDATE dag_acceptances SET head_sha = 'head-b' WHERE relationship_id = ? AND state = 'active'", rid) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-padded', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', NULL, ?, 'merging', 1, 't', 't')", s.pad+"head-b"+s.pad) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn stored with a %s head = %v, want disposition_conflict", s.name, err) + } + if !strings.Contains(err.Error(), "mtn-padded") { + t.Fatalf("the refusal does not name the turn: %v", err) + } + }) + } +} + +// The live-bundle guard matches a member by its train's repository and its head. The member here belongs to another +// accepted node, so the relationship clause cannot match it and only the padded head decides. +func TestACorrectionIsNotRecordedOverALiveBundleWhoseMemberHeadIsStoredInAnotherSpelling(t *testing.T) { + t.Parallel() + for _, s := range headSpellingPads { + t.Run(s.name, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + other := accepted["A"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-spelled', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-spelled', 1, 'turn-1', 5, ?, ?)", other, s.pad+"head-b"+s.pad) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-spelled', 1, 'opened', 'parent', '{}', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a live bundle whose member is stored with a %s head = %v, want disposition_conflict", s.name, err) + } + if !strings.Contains(err.Error(), "trn-spelled") { + t.Fatalf("the refusal does not name the bundle: %v", err) + } + }) + } +} + +// The landed bundle's excluded member: a member whose turn was withdrawn is still a member row, and the head it +// carries reached the base when the bundle landed, even though no integration mark names the member's node. +func TestACorrectionIsNotRecordedOverAnExcludedMemberOfALandedBundle(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + other := accepted["A"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-excluded', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-excluded', 1, 'turn-1', 5, ?, 'head-b')", other) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-excluded', 1, 'opened', 'parent', '{}', 't')") + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-excluded', 2, 'verified', 'parent', '{}', 't')") + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-excluded', 3, 'landed', 'parent', '{}', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over an excluded member of a landed bundle = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "trn-excluded") { + t.Fatalf("the refusal does not name the landed bundle: %v", err) + } +} + +// CRW-906 generation 2, round 11: a merge turn held under the acceptance's local checkout is the same node as its +// forge identity, so the in-flight guard finds it by either name. +func TestACorrectionIsNotRecordedOverAMergingTurnHeldUnderTheLocalPath(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES ((SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?), 'owner/repo', 5)", rid) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, candidate_head, state, tenure, requested_at, updated_at)" + + " VALUES ('mtn-local-path', 'tgt', '/synthetic/checkout', 'dev', 'P-TEST', 'parent', 'host', NULL, 'head-b', 'merging', 1, 't', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn held under the acceptance's local path = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "mtn-local") { + t.Fatalf("the refusal does not name the turn: %v", err) + } +} diff --git a/internal/relay/dagsched/correction_spelling_test.go b/internal/relay/dagsched/correction_spelling_test.go new file mode 100644 index 0000000000..48a40e2dcf --- /dev/null +++ b/internal/relay/dagsched/correction_spelling_test.go @@ -0,0 +1,76 @@ +package dagsched + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// CRW-906 generation 2, round 14: the in-flight guard matches a turn's repository by the lane's own reading of a +// repository (mergeturn.SameRepository), under every spelling the acceptance has. A turn spelled as the checkout's +// git directory names the same repository as the accepted checkout, so a merging turn of it holds the correction; +// a turn of an unrelated checkout with the same head does not. +func TestACorrectionIsNotRecordedOverAMergingTurnSpelledAsTheCheckoutsGitDirectory(t *testing.T) { + t.Parallel() + root := t.TempDir() + checkout := filepath.Join(root, "checkout") + csInit(t, checkout) + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = ?, head_sha = 'head-b', pr_number = 5 WHERE relationship_id = ?", checkout, rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-gitdir', 'tgt', ?, 'dev', 'P-TEST', 'parent', 'host', NULL, NULL, ' HEAD-B\n', 'merging', 1, 't', 't')", filepath.Join(checkout, ".git")) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn spelled as the checkout's git directory = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "mtn-gitdir") { + t.Fatalf("the refusal does not name the turn: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } +} + +func TestACorrectionIsNotHeldByAMergingTurnOfAnUnrelatedCheckout(t *testing.T) { + t.Parallel() + root := t.TempDir() + checkout := filepath.Join(root, "checkout") + unrelated := filepath.Join(root, "unrelated") + for _, dir := range []string{checkout, unrelated} { + csInit(t, dir) + } + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = ?, head_sha = 'head-b' WHERE relationship_id = ?", checkout, rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-unrelated', 'tgt', ?, 'dev', 'P-TEST', 'parent', 'host', NULL, NULL, 'head-b', 'merging', 1, 't', 't')", unrelated) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if err != nil && strings.Contains(err.Error(), "mtn-unrelated") { + t.Fatalf("a merging turn of an unrelated checkout held the correction: %v", err) + } +} + +// csInit makes a real git repository with one commit, so git names it as a repository. +func csInit(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"init", "-q", "-b", "dev"}, {"commit", "-q", "--allow-empty", "-m", "base"}} { + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=crw", "GIT_AUTHOR_EMAIL=crw@example.invalid", "GIT_COMMITTER_NAME=crw", "GIT_COMMITTER_EMAIL=crw@example.invalid") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } +} diff --git a/internal/relay/dagsched/mergejudge.go b/internal/relay/dagsched/mergejudge.go index 630de5efb0..cb718ace35 100644 --- a/internal/relay/dagsched/mergejudge.go +++ b/internal/relay/dagsched/mergejudge.go @@ -7,6 +7,7 @@ import ( "strconv" "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/acceptance" "github.com/thisisjun786/codex-relay-workflow/internal/relay/dag" "github.com/thisisjun786/codex-relay-workflow/internal/relay/mergeturn" "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" @@ -30,6 +31,14 @@ type JudgeResult struct { // Eligible is whether the judgement lets the pull request go to the merge lane. func (r JudgeResult) Eligible() bool { return r.Outcome == OutcomeEligible } +// refuseUnderCorrection is the CRW-906 refusal: the accepted result is being corrected in a later +// generation, so the head its acceptance stands on is not judged, not eligible and not carried to the +// merge lane. It is the existing disposition_conflict and it names the open generation, so a parent +// reading the refusal knows which generation to finish or withdraw; no refusal name is added. +func refuseUnderCorrection(node string, live, standGeneration int64) error { + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is under correction: generation %d is open over the acceptance, which stands on generation %d, so its head is not judged and does not go to the merge lane; accept the corrected result with dag-accept --supersedes, or withdraw the generation if it was never bound or sent", node, live, standGeneration) +} + const maxRounds = 2 // completedConclusions are the answers a finished check gives; anything else (empty, pending, queued, in_progress) has not finished. @@ -218,6 +227,17 @@ func (s *Scheduler) Judge(ctx context.Context, plan, node, actor string, in Judg case pr.HeadSHA != stand.Head: m.Outcome, m.Reason = OutcomeStaleHead, staleHeadReason(pr.HeadSHA, acc.HeadSHA, stand) default: + // CRW-906: this is the head the acceptance stands on, and the relationship's live generation + // is later than the generation it stands on, so the accepted result is under correction and + // the head the judgement is about is the result being repaired. Refused, and nothing is + // written: an eligible row here would let the old head ride to the base. A pull request at + // another head is stale_head above, as it was before this change, and a correction that is + // accepted over or withdrawn moves the stand generation and this gate stops firing. + if under, live, standGeneration, err := acceptance.UnderCorrection(txCtx, tx, still.RelationshipID); err != nil { + return err + } else if under { + return refuseUnderCorrection(node, live, standGeneration) + } landed, why, err := s.predecessorsLanded(txCtx, tx, plan, current, node) if err != nil { return err @@ -357,6 +377,13 @@ func (s *Scheduler) RequestMergeTurn(ctx context.Context, plan, node, actor stri if err != nil { return err } + // CRW-906: no turn is requested for a result whose correction generation opened while the + // judgement above was being made, so the lane never carries the head being repaired + if under, live, standGeneration, err := acceptance.UnderCorrection(txCtx, tx, acc.RelationshipID); err != nil { + return err + } else if under { + return refuseUnderCorrection(node, live, standGeneration) + } if landed, why, err := s.predecessorsLanded(txCtx, tx, plan, current, node); err != nil { return err } else if !landed { diff --git a/internal/relay/dagsched/mergejudge_undercorrection_test.go b/internal/relay/dagsched/mergejudge_undercorrection_test.go new file mode 100644 index 0000000000..7c964308da --- /dev/null +++ b/internal/relay/dagsched/mergejudge_undercorrection_test.go @@ -0,0 +1,163 @@ +package dagsched + +import ( + "context" + "database/sql" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" +) + +// CRW-906 generation 2: the merge gate of an accepted result that is being corrected. The issue's +// decision 2 promises that a node under correction is not merged on its old head. The bundle open and +// the single lane refuse a pull request head that differs from the accepted head, and that guard holds +// only once the child has pushed: between opening (or recording) the correction generation and that +// first push the pull request still shows the accepted head H, so the single-lane judgement must refuse +// by the generation alone. These tests are the single-lane half of that promise, on temporary stores. + +// ucRelationship is the relationship of the kit's accepted node I. +func ucRelationship(t *testing.T, k *judgeKit) string { + t.Helper() + var rid string + if err := k.s.DB.QueryRow("SELECT relationship_id FROM dag_acceptances WHERE plan_id = 'g' AND node_id = 'I' AND state = 'active'").Scan(&rid); err != nil { + t.Fatal(err) + } + return rid +} + +// ucOpenCorrection opens a correction generation of the relationship by hand, the shape generation-open +// leaves: the generations row and the relationship's live generation moved to it. +func ucOpenCorrection(t *testing.T, k *judgeKit, rid string) { + t.Helper() + if _, err := (®istry.Registry{Store: k.s}).OpenGeneration(context.Background(), rid, "correction-"+rid, "needs_changes_revision", sql.NullString{}); err != nil { + t.Fatalf("generation-open: %v", err) + } +} + +// ucJudge is one judgement of the kit's accepted node, with the refusal it is expected to answer. +func ucJudge(t *testing.T, k *judgeKit) (JudgeResult, error) { + t.Helper() + return k.sched.Judge(context.Background(), "g", "I", "parent", JudgeInput{}) +} + +// ucPointer is the generation the relationship stands on now. +func ucPointer(t *testing.T, k *judgeKit, rid string) int64 { + t.Helper() + var generation int64 + if err := k.s.DB.QueryRow("SELECT execution_generation FROM relationships WHERE relationship_id = ?", rid).Scan(&generation); err != nil { + t.Fatal(err) + } + return generation +} + +// Criterion c1 (generation 2): while the relationship's live generation is later than the generation its +// active acceptance stands on, the accepted head is not judged at all: the judgement is refused +// disposition_conflict, names the open generation, and writes no row. A generation that is recorded as +// the node's execution (dag-correct --manifest-digest) is the same state to this gate. +func TestTheSingleLaneRefusesAnAcceptedResultUnderCorrection(t *testing.T) { + t.Parallel() + for _, recorded := range []bool{false, true} { + name := "a correction generation that is open and bound" + if recorded { + name = "a correction generation recorded as the node's execution" + } + t.Run(name, func(t *testing.T) { + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + if recorded { + // what dag-correct --manifest-digest leaves: the execution row of the new generation + k.exec("INSERT INTO dag_node_executions (plan_id, node_id, relationship_id, execution_generation, manifest_digest, kind, managed_request_id)"+ + " VALUES ('g','I',?,2,?,'correction',?)", rid, dig("the corrected manifest"), "request-"+rid) + } + res, err := ucJudge(t, k) + if err == nil { + t.Fatalf("the judgement of an accepted result under correction was accepted: %+v", res) + } + if got := refusalReason(err); got != "disposition_conflict" { + t.Fatalf("reason = %s, want disposition_conflict (%v)", got, err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if n := k.count("SELECT COUNT(*) FROM dag_merge_checks"); n != 0 { + t.Fatalf("the refused judgement wrote %d row(s)", n) + } + if res.Eligible() { + t.Fatal("the refused judgement reads eligible") + } + }) + } +} + +// Criterion c1 (generation 2): the gate is about the correction being open, not about the head. A +// withdrawn generation puts the relationship back on the generation its acceptance stands on, and the +// old head is judged again exactly as before. +func TestTheSingleLaneJudgesAgainAfterTheCorrectionIsWithdrawn(t *testing.T) { + t.Parallel() + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + if _, err := ucJudge(t, k); err == nil { + t.Fatal("the judgement of an accepted result under correction was accepted") + } + if _, err := k.sched.WithdrawGeneration(context.Background(), "g", "I", "parent", + WithdrawInput{Relationship: rid, Generation: 2, Reason: "the correction is not needed"}); err != nil { + t.Fatalf("dag-generation-withdraw: %v", err) + } + if got := ucPointer(t, k, rid); got != 1 { + t.Fatalf("the relationship stands on generation %d after the withdrawal, want 1", got) + } + res, err := ucJudge(t, k) + if err != nil { + t.Fatalf("the judgement after the withdrawal: %v", err) + } + if !res.Eligible() { + t.Fatalf("the judgement after the withdrawal = %+v, want eligible", res) + } +} + +// Criterion c1 (generation 2): the accepted result is accepted over (the corrected head of the new +// generation replaces the acceptance), and the node is a candidate again. The gate reads the stand +// generation the acceptance moved to, so nothing about the correction is left behind. +func TestTheSingleLaneJudgesAgainAfterTheCorrectionIsAcceptedOver(t *testing.T) { + t.Parallel() + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + // dag-accept --supersedes of the corrected result: the acceptance moves to generation 2, the same + // generation the relationship now stands on, so nothing is under correction any more + k.exec("UPDATE dag_acceptances SET execution_generation = 2 WHERE relationship_id = ? AND state = 'active'", rid) + res, err := ucJudge(t, k) + if err != nil { + t.Fatalf("the judgement of the corrected result: %v", err) + } + if !res.Eligible() { + t.Fatalf("the judgement of the corrected result = %+v, want eligible", res) + } +} + +// Criterion c1 (generation 2): the single lane's other half, dag-merge-request, refuses too. It makes +// its own judgement, so no turn is requested and the lane never carries the head being repaired. +func TestRequestMergeTurnRefusesAnAcceptedResultUnderCorrection(t *testing.T) { + t.Parallel() + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + _, turn, err := k.sched.RequestMergeTurn(context.Background(), "g", "I", "parent", MergeRequestInput{Host: "host-a"}) + if err == nil { + t.Fatalf("a merge turn was requested for an accepted result under correction: %v", turn) + } + if got := refusalReason(err); got != "disposition_conflict" { + t.Fatalf("reason = %s, want disposition_conflict (%v)", got, err) + } + if !strings.Contains(err.Error(), "under correction") { + t.Fatalf("the refusal does not say why: %v", err) + } + if n := k.count("SELECT COUNT(*) FROM merge_turns"); n != 0 { + t.Fatalf("the refused request wrote %d merge turn(s)", n) + } +} diff --git a/internal/relay/dagsched/repository_identity_test.go b/internal/relay/dagsched/repository_identity_test.go new file mode 100644 index 0000000000..69f554816c --- /dev/null +++ b/internal/relay/dagsched/repository_identity_test.go @@ -0,0 +1,78 @@ +package dagsched + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// CRW-906 generation 2, round 16: the in-flight guard names a merging turn's repository by git's own identity of it +// (mergeturn.SameRepository), so every git spelling of the accepted repository holds the correction, a linked +// worktree included, and an unrelated repository does not. +func TestACorrectionIsHeldByEveryGitSpellingOfTheAcceptedRepository(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + main := filepath.Join(root, "main") + csInit(t, main) + linked := filepath.Join(root, "linked") + riGitRun(t, main, "worktree", "add", "-q", "--detach", linked, "HEAD") + link := filepath.Join(root, "link") + if err := os.Symlink(main, link); err != nil { + t.Fatal(err) + } + other := filepath.Join(root, "other") + csInit(t, other) + cases := []struct { + name, repository string + held bool + }{ + {"the main work tree", main, true}, + {"the main work tree with a trailing slash", main + "/", true}, + {"the main work tree through a dot segment", main + "/./", true}, + {"the .git directory", filepath.Join(main, ".git"), true}, + {"a symlink to the main work tree", link, true}, + {"a linked worktree", linked, true}, + {"a linked worktree with a trailing slash", linked + "/", true}, + {"the linked worktree's administrative directory", filepath.Join(main, ".git", "worktrees", "linked"), true}, + {"an unrelated repository with the same head", other, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = ?, head_sha = 'head-b', pr_number = 5 WHERE relationship_id = ?", main, rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-riwt', 'tgt', ?, 'dev', 'P-TEST', 'parent', 'host', NULL, NULL, 'head-b', 'merging', 1, 't', 't')", c.repository) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if c.held { + if refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "mtn-riwt") { + t.Fatalf("a merging turn spelled %q was not held: %v", c.repository, err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + return + } + if err != nil && strings.Contains(err.Error(), "mtn-riwt") { + t.Fatalf("a merging turn of an unrelated repository held the correction: %v", err) + } + }) + } +} + +func riGitRun(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=crw", "GIT_AUTHOR_EMAIL=crw@example.invalid", "GIT_COMMITTER_NAME=crw", "GIT_COMMITTER_EMAIL=crw@example.invalid") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } +} diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index 10c7311767..bfb4169b70 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -9,6 +9,7 @@ import ( "github.com/thisisjun786/codex-relay-workflow/internal/contract" "github.com/thisisjun786/codex-relay-workflow/internal/relay/dag" "github.com/thisisjun786/codex-relay-workflow/internal/relay/delivery" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/mergeturn" "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" ) @@ -51,6 +52,26 @@ const ( OpenedByDecisionReply = "decision_reply" ) +// AcceptedResultCorrection is the reason a coordinator gives generation-open when it corrects a result that was accepted and is still current (recordHandOpened): the relay's verdict writer has no review to +// open a second ruling on the accepted head with, so the generation is opened by hand and its own reason says why. needs_changes_revision, the reason generation-open writes by default, is admitted the same way; +// a generation opened for anything else (the assignment itself, a returning tenure, a reply) is not a correction of an accepted result. +const AcceptedResultCorrection = "accepted_result_correction" + +// correctionOpenReason is whether the reason a generation was opened with says a correction: the generation-open default, or the reason the coordinator gives when the accepted result is current. +func correctionOpenReason(reason string) bool { + return reason == "needs_changes_revision" || reason == AcceptedResultCorrection +} + +// correctionOpenedReason is the reason the relationship's current generation was opened under: generations.reason, which generation-open writes and a returning tenure leaves empty. Named for this route +// rather than for the column, because a sibling node of the same plan edits this package too and two top-level names that only differ by which file declares them do not conflict in git. +func (s *Scheduler) correctionOpenedReason(ctx context.Context, q store.Querier, rel relRow) (string, error) { + var reason sql.NullString + if _, err := queryOne(ctx, q, "SELECT reason FROM generations WHERE relationship_id = ? AND execution_generation = ?", []any{rel.ID, rel.Generation}, &reason); err != nil { + return "", err + } + return reason.String, nil +} + // CorrectionRequestID is the dispatch request id a correction generation opened by hand carries (generation-open --dispatch-request-id): derived from the plan, the node, the manifest and the generation, so // the generation row itself names the one manifest it was opened for and a generation opened under any other id is not bound to a manifest by dag-correct. func CorrectionRequestID(plan, node, manifestDigest string, generation int64) string { @@ -91,7 +112,8 @@ func (s *Scheduler) describeOpening(ctx context.Context, q store.Querier, plan, // an accepted head it ruled verified (disposition_conflict, naming this route) unless the criteria registered for the relationship changed (a node whose consumed input was replaced, its criteria untouched). The // route is bounded so that it cannot make a rerun or bind a manifest other than the one the generation was opened for: // -// - the node's accepted result is stale and its route, without this generation, is a correction: a result that is not accepted yet is corrected by a ruling, an accepted result that is current has no recorded correction route in this build, a change of the criteria alone is a revalidation and +// - the node's accepted result is stale and its route, without this generation, is a correction, or the result is accepted and still current and the generation's own reason states a correction +// (needs_changes_revision, the generation-open default, or accepted_result_correction): a result that is not accepted yet is corrected by a ruling, a change of the criteria alone is a revalidation and // opens no generation, and a node that landed was refused before this; // - the manifest is named (--manifest-digest) and is the one stored for this node at its current slice and criteria, as on the ruling route, and its inputs are still the ones the node's edges are satisfied // by now (VerifyManifest without the file bytes, as release checks its own intent): a predecessor accepted again after the prepare leaves another input than the one the child was told to consume; @@ -101,6 +123,11 @@ func (s *Scheduler) describeOpening(ctx context.Context, q store.Querier, plan, // What is recorded of how the instruction reached the child is that request id (dag_node_executions.managed_request_id, which a ruling leaves NULL) and the bound dispatch turn. It is the coordinator's // statement: the relay does not read the child's thread, so unlike the ruling route (which compares the restoration note with the instruction line) nothing here compares the dispatching message with the // line; the child checks the manifest file against the digest and hash the line names and answers blocked_needs_input on a mismatch. +// +// CRW-906: a result that was accepted and is still current (not stale) is corrected by hand as well, when a blocking defect is found in it before it lands and no ruling can open the generation: a bundle +// review that finds one in an accepted member is the case. The generation's own reason states the correction (needs_changes_revision, what generation-open writes by default, or accepted_result_correction, +// which names the route), the node must not have landed, and every check above is the one the stale route had. The acceptance stays active until the parent takes the new result with dag-accept --supersedes, +// and the reason of the generation row is what notes that the correction came through this route: nothing is added to the schema, no column and no refusal name is new. func (s *Scheduler) recordHandOpened(ctx context.Context, q store.Querier, plan string, snap dag.Snapshot, n dag.SnapNode, rel relRow, suppliedDigest string, out *CorrectionResult) error { before, err := store.LiveGenerationBefore(ctx, q, rel.ID, rel.Generation) if err != nil { @@ -130,8 +157,31 @@ func (s *Scheduler) recordHandOpened(ctx context.Context, q store.Querier, plan if err != nil { return err } + // CRW-906: an accepted result that is still current is corrected by hand as well, when the generation's own reason says a correction (needs_changes_revision or accepted_result_correction) and the node has + // not landed. Every check below is the one the stale route had. The reason of the generation row is what notes the route, so nothing is added to the schema and no refusal name is new. The caller refuses a + // generation whose reason states no correction before it reaches this binder (correction.go); the case below is this binder's own bound, so an accepted result is never recorded as corrected by a + // generation that was opened for something else. if st == nil && !ambiguousPrevious { - return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is not stale, so it is not corrected by a generation opened by hand either: a result that is not accepted yet is corrected by a ruling, and an accepted result that is current has no recorded correction route in this build. A generation that only merged the base into the branch is not a correction: when the generation was ruled verified, dag-base-refresh records it, after proving from git that its head is the accepted head plus merges of the base", notRuled, n.NodeID) + reason, err := s.correctionOpenedReason(ctx, q, rel) + if err != nil { + return err + } + switch { + case !hasAcceptance: + return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is not stale, so it is not corrected by a generation opened by hand either: a result that is not accepted yet is corrected by a ruling. A generation that only merged the base into the branch is not a correction: when the generation was ruled verified, dag-base-refresh records it, after proving from git that its head is the accepted head plus merges of the base", notRuled, n.NodeID) + case !correctionOpenReason(reason): + return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is current: a generation opened by hand corrects it only when the reason it was opened under states a correction, and generation %d reads %q. Open the generation with generation-open --reason needs_changes_revision or %s, or, when it only merged the base into the branch, record it with dag-base-refresh", notRuled, n.NodeID, rel.Generation, reason, AcceptedResultCorrection) + } + // CRW-906: a turn of this node that is merging or of unknown effect may already have carried the + // accepted head to the base on the forge, outside the relay. Recording a correction now would name + // a head that is on its way to landing as the one being repaired, and no later refusal can undo a + // merge the forge already made: the turn is resolved first (merge-turn-resolve, or merge-turn-unknown + // then merge-turn-resolve). A turn that only waits or holds the lane is not this case: the lane's own + // check and land refuse it under correction, so it cannot progress. This is the same conservative + // reading the verdict writer makes when a ruling rests on a merge turn (registry.RestsOn). + if err := s.refuseAcceptedHeadOnItsWayToTheBase(ctx, q, acc); err != nil { + return err + } } // the same route a stale node has without the generation: a change of the criteria alone is ruled again (a revalidation, no generation), and what rests on a stale predecessor or an input that is not // there waits; only a result that must be reworked is corrected @@ -425,6 +475,213 @@ func (s *Scheduler) refuseRevalidation(ctx context.Context, q store.Querier, pla return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is stale (%s) and ruling the same output again would not make it current; its route is %s: %s", n.NodeID, st.Reason(), action, detail) } +// refuseAcceptedHeadOnItsWayToTheBase is the guard CRW-906 adds to the accepted-current correction: a +// merge turn of the node's relationship that is merging or of unknown effect may already have carried the +// accepted head to the base on the forge. The relay reads the forge, not the merge itself, so the merge +// is a fact the relay may not have recorded; recording a correction over it would name a head that is on +// its way to landing as the one being repaired, and no later refusal can undo a merge the forge already +// made. The turn is resolved first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve). +// +// A turn that only waits or holds the lane is not this case: the lane's own check and land refuse it +// under correction (mergeturn.underCorrectionRefusal), so it cannot progress to the base while the +// generation is open. The reading is the same conservative one the verdict writer makes when a ruling +// rests on a merge turn (registry.RestsOn): a turn of the assignment that is merging, unknown or landed +// counts, whatever head it names. +// +// The turn is resolved by every identity a claim may carry, because --relationship and --pr are both +// optional on merge-turn-request: the relationship, the pull request, or the head the turn holds. The +// lane's own gate resolves the same three (mergeturn.underCorrectionRefusal), and a guard that read only +// the relationship would miss the turn a successful merge-turn-check had just authorized. +func (s *Scheduler) refuseAcceptedHeadOnItsWayToTheBase(ctx context.Context, q store.Querier, acc Acceptance) error { + heads, err := s.stoodOn(ctx, q, acc) + if err != nil { + return err + } + // The turn's repository is matched by mergeturn.SameRepository, the lane's one reading of a repository, + // against every spelling the acceptance has: the repository it was accepted against, and the forge + // repository recorded with it (dag_acceptance_forge). A legacy acceptance keeps whatever target it was + // accepted against, a local checkout included, so a PR-only or bare-head turn of it is found through the + // forge spelling, and a turn spelled as the checkout, its .git directory or a symlink to it is found + // through the path. + spellings, err := acceptanceRepositories(ctx, q, acc) + if err != nil { + return err + } + clauses := []string{"relationship_id = ?"} + args := []any{acc.RelationshipID} + for _, head := range heads { + if head == "" { + continue + } + clauses = append(clauses, "crw_same_commit(candidate_head, ?)") + args = append(args, head) + } + if acc.PRNumber > 0 { + clauses = append(clauses, "pr_number = ?") + args = append(args, acc.PRNumber) + } + rows, err := q.QueryContext(ctx, "SELECT turn_id, state, candidate_head, repository, relationship_id, pr_number FROM merge_turns"+ + " WHERE state IN ('merging','unknown','landed') AND ("+strings.Join(clauses, " OR ")+") ORDER BY requested_at, turn_id", args...) + if err != nil { + return err + } + type turnRow struct { + turn, state, repository string + head sql.NullString + relationship sql.NullString + pr sql.NullInt64 + } + var turns []turnRow + for rows.Next() { + var t turnRow + if err := rows.Scan(&t.turn, &t.state, &t.head, &t.repository, &t.relationship, &t.pr); err != nil { + rows.Close() + return err + } + turns = append(turns, t) + } + if err := rows.Err(); err != nil { + rows.Close() + return err + } + rows.Close() + for _, t := range turns { + byRelationship := t.relationship.Valid && t.relationship.String == acc.RelationshipID + bySelector := spelledAs(t.repository, spellings) && + ((acc.PRNumber > 0 && t.pr.Valid && t.pr.Int64 == acc.PRNumber) || headIsOneOf(t.head.String, heads)) + if !byRelationship && !bySelector { + continue + } + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: merge turn %s of the relationship is %s on head %s, and a correction cannot be recorded over a head the forge may already have merged. Resolve that turn first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve), then record this same generation again with the manifest digest dag-correct --prepare already printed: it is not yet recorded, and opening another generation would skip it", + acc.NodeID, short(t.turn), t.state, short(t.head.String)) + } + // A live bundle is the second way the head may already be on the base. The parent merges a verified + // bundle on the forge and records it with merge-train-land afterwards, so between those two the + // bundle's members still hold or wait in the lane while the forge already carries the head; the + // bundle's own state cannot be told from one that was never merged, and the relay reads no forge + // here. Recording a correction now would name a head the forge may already have merged, and the + // later train refusal cannot undo that. The bundle is closed first (merge-train-close), which is what + // a parent that found a defect in a bundle review does anyway. + if err := s.refuseLiveBundleCarrying(ctx, q, acc, heads); err != nil { + return err + } + return nil +} + +// refuseLiveBundleCarrying refuses the correction while an opened or verified bundle carries the node's +// stand head as a member: the bundle's merge may already be on the forge, and the relay records that +// merge only when the bundle lands. A bundle that landed, was done or was abandoned carries nothing +// live, so a parent that closed the bundle before correcting a member is not held back. +func (s *Scheduler) refuseLiveBundleCarrying(ctx context.Context, q store.Querier, acc Acceptance, heads []string) error { + present, err := tableExists(ctx, q, "merge_train_members") + if err != nil || !present { + return err + } + // A bundle carries a head through whichever node's member it is: two accepted nodes can name the same + // commit, so a member is matched by its head and by the repository its train names (under any spelling + // the acceptance has), or by its own relationship. + spellings, err := acceptanceRepositories(ctx, q, acc) + if err != nil { + return err + } + for _, head := range heads { + if head == "" { + continue + } + train, err := bundleMemberCarrying(ctx, q, acc, spellings, head, + "AND (SELECT kind FROM merge_train_events e WHERE e.train_id = m.train_id ORDER BY e.seq DESC LIMIT 1) IN ('opened','verified')") + if err != nil { + return err + } + if train != "" { + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: bundle %s is live and carries head %s as a member, so its merge may already be on the forge and a correction cannot be recorded over it. Close the bundle first (merge-train-close), then record this same generation again with the manifest digest dag-correct --prepare already printed: it is not yet recorded, and opening another generation would skip it", + acc.NodeID, short(train), short(head)) + } + // A landed bundle has merged the tree that holds every member head, excluded members included: a member + // whose turn was withdrawn is still a member row, and no integration mark names its node. Its head is on the + // base, so the correction is refused whatever the member's relationship. + landed, err := bundleMemberCarrying(ctx, q, acc, spellings, head, + "AND EXISTS (SELECT 1 FROM merge_train_events e WHERE e.train_id = m.train_id AND e.kind = 'landed')") + if err != nil { + return err + } + if landed != "" { + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s has already landed: bundle %s landed a tree that carries head %s as a member, so the head is on the base and a correction cannot be recorded over it. A node that landed is never run again: what changed above it is carried by a successor node of a new plan revision (contract 8.4, E-20)", + acc.NodeID, short(landed), short(head)) + } + } + return nil +} + +// bundleMemberCarrying is the first bundle, in member order, whose member carries head and that the event filter +// selects, and whose train names the acceptance's repository under one of its spellings or whose member is the +// acceptance's own relationship. "" means none. +func bundleMemberCarrying(ctx context.Context, q store.Querier, acc Acceptance, spellings []string, head, eventFilter string) (string, error) { + rows, err := q.QueryContext(ctx, "SELECT m.train_id, t.repository, m.relationship_id FROM merge_train_members m JOIN merge_trains t ON t.train_id = m.train_id"+ + " WHERE crw_same_commit(m.member_head, ?) "+eventFilter+" ORDER BY m.train_id, m.seq", head) + if err != nil { + return "", err + } + type member struct { + train, repository string + relationship sql.NullString + } + var members []member + for rows.Next() { + var m member + if err := rows.Scan(&m.train, &m.repository, &m.relationship); err != nil { + rows.Close() + return "", err + } + members = append(members, m) + } + if err := rows.Err(); err != nil { + rows.Close() + return "", err + } + rows.Close() + for _, m := range members { + if (m.relationship.Valid && m.relationship.String == acc.RelationshipID) || spelledAs(m.repository, spellings) { + return m.train, nil + } + } + return "", nil +} + +// acceptanceRepositories are every repository spelling of an acceptance: the one it was accepted against and the +// forge repository recorded with it. +func acceptanceRepositories(ctx context.Context, q store.Querier, acc Acceptance) ([]string, error) { + forge := "" + has, err := queryOne(ctx, q, "SELECT forge_repository FROM dag_acceptance_forge WHERE acceptance_id = ?", []any{acc.AcceptanceID}, &forge) + if err != nil { + return nil, err + } + if !has { + return []string{acc.Repository}, nil + } + return []string{acc.Repository, forge}, nil +} + +// spelledAs reports whether a stored repository names one of the spellings, by mergeturn.SameRepository. +func spelledAs(repository string, spellings []string) bool { + for _, s := range spellings { + if mergeturn.SameRepository(repository, s) { + return true + } + } + return false +} + +// headIsOneOf reports whether a stored head names one of the heads, by the one head definition. +func headIsOneOf(stored string, heads []string) bool { + for _, h := range heads { + if h != "" && mergeturn.SameCommit(stored, h) { + return true + } + } + return false +} + // refuseLanded is the guard of a correction (contract 8.4, E-20): a node whose accepted head landed in every target it lands on is never run again, whatever changed above it and whatever the plan now // says the node is: what landed is judged from the acceptance, so a revision that changes the kind of a landed node does not lift it. The change is carried by a successor node of a new plan revision, // which is released like any other node. diff --git a/internal/relay/dagsched/revalidation_test.go b/internal/relay/dagsched/revalidation_test.go index 652d87a849..944d82cf0a 100644 --- a/internal/relay/dagsched/revalidation_test.go +++ b/internal/relay/dagsched/revalidation_test.go @@ -865,7 +865,8 @@ func TestReworkWithUnchangedCriteriaGoesThroughAGenerationOpenedByHand(t *testin } // What dag-correct will not bind for a generation opened by hand: one opened for another manifest, one nobody bound to a turn, one without a named manifest, a manifest that is not the node's, and a -// node whose result is not stale (those are corrected by a ruling). Each leaves no execution row. +// node whose result is not stale (those are corrected by a ruling). Each leaves no execution row. A result that was accepted and is still current is corrected by hand under a reason that states a +// correction (CRW-906, correction_accepted_test.go); under any other reason it is refused as well, and this is the case below. func TestAGenerationOpenedByHandIsBoundOnlyToTheManifestItWasOpenedFor(t *testing.T) { t.Parallel() bound := func(k *releaseKit) int { @@ -1009,14 +1010,14 @@ func TestAGenerationOpenedByHandIsBoundOnlyToTheManifestItWasOpenedFor(t *testin t.Fatalf("the refusal does not name the route: %v", err) } }) - t.Run("a node whose accepted result is current has no hand-opened route", func(t *testing.T) { + t.Run("a node whose accepted result is current under a reason that states no correction", func(t *testing.T) { k, accepted, _, _ := rvHandKit(t) ridB := accepted["B"].RelationshipID prepared := k.rvPrepare("sr", "B") - rvOpenByHand(t, k, ridB, prepared.DispatchRequestID, true) + acOpenByHand(t, k, ridB, prepared.DispatchRequestID, "initial_assignment", 2, true) if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" || k.count("SELECT COUNT(*) FROM dag_node_executions WHERE node_id = 'B' AND execution_generation = 2") != 0 { t.Fatalf("correction = %v", err) - } else if !strings.Contains(err.Error(), "not stale") { + } else if !strings.Contains(err.Error(), "not opened by a needs_changes ruling or a decision reply") { t.Fatalf("the refusal does not say why: %v", err) } }) diff --git a/internal/relay/dagsched/withdraw.go b/internal/relay/dagsched/withdraw.go index e6db0998c5..d3f8eedae6 100644 --- a/internal/relay/dagsched/withdraw.go +++ b/internal/relay/dagsched/withdraw.go @@ -112,7 +112,7 @@ func (s *Scheduler) WithdrawGeneration(ctx context.Context, plan, node, actor st return refuse(contract.RefusalUnknownGeneration, "%s has no generation %d", rel.ID, in.Generation) } // generation-open gives the generation a reason; a returning tenure's generation has none, and undoing a tenure is not what a withdrawal does (it leaves the supersession and the linkage as they are) - if opened.String != "initial_assignment" && opened.String != "needs_changes_revision" { + if opened.String != "initial_assignment" && !correctionOpenReason(opened.String) { return refuse(contract.RefusalDispositionConflict, "generation %d of %s was not opened by generation-open (its reason is %q): a returning tenure or a reply opened it, and a withdrawal does not undo that", in.Generation, rel.ID, opened.String) } if anchor != "anchor_pending" || turn.Valid || bound.Valid { diff --git a/internal/relay/delivery/rulingchange.go b/internal/relay/delivery/rulingchange.go index 5484c314b2..ee8145e629 100644 --- a/internal/relay/delivery/rulingchange.go +++ b/internal/relay/delivery/rulingchange.go @@ -63,7 +63,7 @@ func builtOn(eventID, turn string, rest registry.Rest) error { prefix := fmt.Sprintf("%s is already ruled verified (verdict turn %s), and a ruling of needs_changes cannot replace it: ", strconv.Quote(eventID), strconv.Quote(turn)) switch rest.Kind { case registry.RestsAccepted: - return refuse(DispositionConflict, "%splan %s accepted it as acceptance %s (node %s), and an accepted result is no longer open to a second ruling. Read the node's stale reading: when its action is correct, dag-correct --prepare prints the instruction and the dispatch request id, the generation is opened by hand and dag-correct binds it; when the action is revalidate or hold, that action's own step applies; when the accepted result is current, a base that moved after the acceptance has its own route, dag-base-refresh: the child merges the base into the branch in a generation opened by hand and changes nothing else, the parent rules that generation verified and records it, and the relay proves from git that its head is the accepted head plus merges of the base (crw-run's 'A base refresh the child made after the acceptance'); for every other current result there is no recorded correction route in this build, so report it on the coordination record and do not open a generation dag-correct will refuse", + return refuse(DispositionConflict, "%splan %s accepted it as acceptance %s (node %s), and an accepted result is no longer open to a second ruling. Read the node's stale reading: when its action is correct, dag-correct --prepare prints the instruction and the dispatch request id, the generation is opened by hand and dag-correct binds it; when the action is revalidate or hold, that action's own step applies; when the accepted result is current and a defect was found in it before it landed, the same steps correct it: dag-correct --prepare prints the instruction and the dispatch request id, the generation is opened under a reason that states the correction (accepted_result_correction, or the needs_changes_revision default), the instruction is dispatched and that turn is bound, and dag-correct --manifest-digest binds it, after which dag-accept --supersedes takes the corrected result; when the accepted result is current, a base that moved after the acceptance has its own route, dag-base-refresh: the child merges the base into the branch in a generation opened by hand and changes nothing else, the parent rules that generation verified and records it, and the relay proves from git that its head is the accepted head plus merges of the base (crw-run's 'A base refresh the child made after the acceptance'). A generation opened under a reason that states no correction is refused, naming the reason it carries", prefix, rest.Plan, rest.ID, rest.Node) case registry.RestsMarkedMerged: return refuse(DispositionConflict, "%sthe work is marked merged (at %s), and a merged result is corrected by new work, not by a second ruling", prefix, rest.At) diff --git a/internal/relay/delivery/rulingchange_test.go b/internal/relay/delivery/rulingchange_test.go index 45511e905f..e814500e1b 100644 --- a/internal/relay/delivery/rulingchange_test.go +++ b/internal/relay/delivery/rulingchange_test.go @@ -237,10 +237,12 @@ func TestRC04_acceptance_and_a_merge_mark_close_the_change(t *testing.T) { event := h.managedVerified() h.rcAccept(event) detail := h.rcRefused(event, "needs_changes", "v2", rcRestoration(), nil, DispositionConflict) - rcMentions(t, detail, "acceptance-1", "plan-1", "node-1", "dag-correct --prepare", "stale", "no recorded correction route", "generation dag-correct will refuse") + rcMentions(t, detail, "acceptance-1", "plan-1", "node-1", "dag-correct --prepare", "stale", "accepted_result_correction", "dag-correct --manifest-digest", "dag-accept --supersedes") rcMentions(t, detail, "a base that moved after the acceptance", "dag-base-refresh", "merges of the base") - if strings.Contains(detail, "a base that moved after the acceptance is such a case") { - t.Fatalf("the refusal still files the moved base under the case that has no route: %s", detail) + // CRW-906: a defect found in a current accepted result has a recorded route, so the refusal must not + // tell the coordinator that no route exists for it + if strings.Contains(detail, "no recorded correction route") { + t.Fatalf("the refusal still says a current accepted result has no correction route: %s", detail) } }) t.Run("merged", func(t *testing.T) { diff --git a/internal/relay/mergeturn/check.go b/internal/relay/mergeturn/check.go index 8894e23dee..df9b0bce06 100644 --- a/internal/relay/mergeturn/check.go +++ b/internal/relay/mergeturn/check.go @@ -117,6 +117,16 @@ func (s *Service) Check(ctx context.Context, turn, actor, head, base string, che } else if guard != nil { refusal = guard } + // CRW-906: an accepted result whose correction generation is open is not carried to the base by + // the lane either. A turn can have been granted before the correction was opened, and this check + // is what moves it to merging, so the state is read here, inside the transaction that writes. + if refusal == nil { + if guard, e := underCorrectionRefusal(tx, s.Store.Querier(tx), row); e != nil { + return e + } else if guard != nil { + refusal = guard + } + } if refusal == nil { switch { case row.HolderTaskID != actor: diff --git a/internal/relay/mergeturn/head_identity_test.go b/internal/relay/mergeturn/head_identity_test.go new file mode 100644 index 0000000000..b7eb448eeb --- /dev/null +++ b/internal/relay/mergeturn/head_identity_test.go @@ -0,0 +1,121 @@ +package mergeturn + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/acceptance" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// refreshIDFor is the id acceptance.Stands accepts for a base refresh row: the digest of its fields. +func refreshIDFor(acceptanceID, relationship string, generation int64, event, revision, head, repository, ref, tip string) string { + return acceptance.RefreshDigest(acceptanceID, relationship, generation, event, revision, head, repository, ref, tip, "{}", "[]") +} + +// CRW-906 generation 2, round 8: a base refresh keeps a superseded head on its acceptance only when the relay +// validly recorded it. A refresh row whose id is not the digest of its fields does not keep the head. +func TestAnInvalidBaseRefreshDoesNotKeepASupersededHead(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-c", 2) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, 'acc-rel-c', 1, 'rel-c', 1, 'ev-r1', 'rev-r1', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:20.000000+00:00')", + "dbr-"+strings.Repeat("c", 60), fxRepo, fxBase, alpha.TaskID) + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-c'") + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-c2', 'plan-x', 'node-c', 'manifest-c2', 'rel-c', 2, 'ev-c2', 'rev-c2', 'crit-1', 'verified', 'head-b', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:21.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, 'acc-rel-c2', 1, 'rel-c', 2, 'ev-r2', 'rev-r2', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:22.000000+00:00')", + "dbr-"+strings.Repeat("d", 60), fxRepo, fxBase, alpha.TaskID) + turn := store.MergeTurnsRow{TurnID: "mtn-invalid-refresh", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-refreshed", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a superseded head kept by a refresh row whose id is not its digest was not refused: %+v", refusal) + } +} + +// CRW-906 generation 2, round 7: one head identity. Every spelling below names the same commit under +// SameCommit, so the lane gate must refuse a turn holding any of them when it holds the accepted head. +var headIdentitySpellings = []string{ + "HEAD-A", "head-a", " head-a", "\thead-a\n", "head-a\v", "\fhead-a\r", + "\u00a0head-a", "head-a\u0085", "\u2003head-a", "head-a\u3000", +} + +func TestTheLaneGateAgreesWithSameCommitForEverySpelling(t *testing.T) { + for _, head := range headIdentitySpellings { + t.Run(fmt.Sprintf("%q", head), func(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + // The accepted head is stored in the spelling under test (a merge turn or an acceptance is stored + // verbatim), and the turn holds the clean accepted head: SameCommit names them the same commit. + w.exec("UPDATE dag_acceptances SET head_sha = ? WHERE acceptance_id = ?", head, "acc-rel-lane") + turn := store.MergeTurnsRow{TurnID: "mtn-spelling", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if !SameCommit(head, "head-a") { + t.Fatalf("SameCommit does not name the accepted head for %q", head) + } + if refusal == nil { + t.Fatalf("the lane admitted a turn holding the accepted head stored as %q", head) + } + }) + } +} + +// The head column of a SQL comparison must go through the one head definition, never a hand-written trim +// or lower: the next query that trims a head is caught here instead of by an evaluation. +func TestNoSQLTrimsOrLowersAHeadOrShaColumn(t *testing.T) { + // the call must take a head or sha column itself, so a repository lower() on the same line is not a head + headCall := regexp.MustCompile(`(?i)\b(trim|lower)\((\w+\.)?\w*(head|sha)`) + for _, dir := range []string{".", "../dagsched"} { + files, err := filepath.Glob(filepath.Join(dir, "*.go")) + if err != nil { + t.Fatal(err) + } + for _, file := range files { + if strings.HasSuffix(file, "_test.go") { + continue + } + body, err := os.ReadFile(file) + if err != nil { + t.Fatal(err) + } + for n, line := range strings.Split(string(body), "\n") { + if headCall.MatchString(line) { + t.Errorf("%s:%d applies trim or lower to a head or sha; compare with crw_same_commit instead: %s", file, n+1, strings.TrimSpace(line)) + } + } + } + } +} + +// CRW-906 generation 2, round 11: an acceptance can record a local checkout and a forge identity; a turn held under +// the local path is the same node, so the lane gate matches it by either name. +func TestTheLaneGateMatchesALocalPathTurnWhenTheAcceptanceHasAForgeIdentity(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout' WHERE acceptance_id = 'acc-rel-lane'") + w.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES ('acc-rel-lane', ?, 1)", fxRepo) + turn := store.MergeTurnsRow{TurnID: "mtn-local-path", TargetKey: "tgt-x", Repository: "/synthetic/checkout", BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn held under the local path of an accepted head with a forge identity was not refused: %+v", refusal) + } +} diff --git a/internal/relay/mergeturn/outcome.go b/internal/relay/mergeturn/outcome.go index 9f4bc57796..cfaddea5da 100644 --- a/internal/relay/mergeturn/outcome.go +++ b/internal/relay/mergeturn/outcome.go @@ -88,6 +88,17 @@ func (s *Service) Land(ctx context.Context, turn, actor, landed, stated, evidenc } else if guard != nil { refusal = guard } + // CRW-906: a correction generation opened over the accepted result after the currency check (or + // before it) refuses the landing, on this transaction's snapshot: the merge already happened on + // the forge, but the relay records no landing of the head that is being repaired, and the turn + // stays merging for the holder to read again. + if refusal == nil { + if guard, e := underCorrectionRefusal(tx, s.Store.Querier(tx), r); e != nil { + return e + } else if guard != nil { + refusal = guard + } + } if refusal == nil { switch { case r.HolderTaskID != actor: diff --git a/internal/relay/mergeturn/repository_identity_test.go b/internal/relay/mergeturn/repository_identity_test.go new file mode 100644 index 0000000000..981bc9ce99 --- /dev/null +++ b/internal/relay/mergeturn/repository_identity_test.go @@ -0,0 +1,161 @@ +package mergeturn + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// CRW-906 generation 2, round 16: the repository identity is the common git directory git reports, so the tests +// drive real repositories made with git itself: a main work tree, a linked worktree, a directory whose name ends in +// a space, an unrelated repository and a directory that is not a repository at all. + +// riRepos are the spellings the identity tests name: each entry is one repository spelled several ways. +type riRepos struct { + main, linked, adminDir, link, spaced, other, plain string +} + +func riGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=crw", "GIT_AUTHOR_EMAIL=crw@example.invalid", + "GIT_COMMITTER_NAME=crw", "GIT_COMMITTER_EMAIL=crw@example.invalid") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v in %s: %v: %s", args, dir, err, out) + } +} + +func riInit(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + riGit(t, dir, "init", "-q", "-b", "dev") + riGit(t, dir, "commit", "-q", "--allow-empty", "-m", "base") +} + +func riSetup(t *testing.T) riRepos { + t.Helper() + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + var r riRepos + r.main = filepath.Join(root, "main") + riInit(t, r.main) + r.linked = filepath.Join(root, "linked") + riGit(t, r.main, "worktree", "add", "-q", "--detach", r.linked, "HEAD") + r.adminDir = filepath.Join(r.main, ".git", "worktrees", "linked") + r.link = filepath.Join(root, "link") + if err := os.Symlink(r.main, r.link); err != nil { + t.Fatal(err) + } + r.spaced = filepath.Join(root, "space dir ") + riInit(t, r.spaced) + r.other = filepath.Join(root, "other") + riInit(t, r.other) + r.plain = filepath.Join(root, "plain") + if err := os.MkdirAll(r.plain, 0o755); err != nil { + t.Fatal(err) + } + return r +} + +// TestSameRepositoryNamesTheRepositoryGitNamesForEverySpelling pins the identity itself. +func TestSameRepositoryNamesTheRepositoryGitNamesForEverySpelling(t *testing.T) { + r := riSetup(t) + same := []string{ + r.main, r.main + "/", r.main + "/.", r.main + "/.git", r.link, r.link + "/.", + r.linked, r.linked + "/", r.adminDir, filepath.Join(r.main, "..", "main"), + } + for _, spelling := range same { + if !SameRepository(r.main, spelling) || !SameRepository(spelling, r.main) { + t.Fatalf("%q does not name the repository %q that git names", spelling, r.main) + } + } + if !SameRepository(r.spaced, r.spaced+"/.") { + t.Fatal("a directory whose name ends in a space is not named by its own /. spelling") + } + for _, spelling := range []string{r.other, r.plain, filepath.Join(r.main, "missing"), strings.TrimSpace(r.spaced)} { + if SameRepository(r.main, spelling) || SameRepository(r.spaced, spelling) { + t.Fatalf("%q names a repository it does not reach", spelling) + } + } + if SameRepository(r.plain, r.plain+"/.") { + t.Fatal("a path that is not a repository is compared by something other than its exact text") + } + if !SameRepository(r.plain, r.plain) { + t.Fatal("a path that is not a repository is not equal to itself") + } + if !SameRepository(" Owner/Repo ", "owner/repo") || SameRepository(r.main, "owner/repo") || SameRepository("", "") { + t.Fatal("forge slugs or empty spellings are named wrongly") + } +} + +// TestTheLaneGateHoldsEveryGitSpellingOfTheRepository: the lane gate holds a head for a turn whose repository is any +// git spelling of the accepted repository, and for no other repository. +func TestTheLaneGateHoldsEveryGitSpellingOfTheRepository(t *testing.T) { + r := riSetup(t) + cases := []struct { + name, repository string + held bool + }{ + {"the main work tree", r.main, true}, + {"the main work tree with a trailing slash", r.main + "/", true}, + {"the main work tree through a dot segment", r.main + "/./", true}, + {"the .git directory", filepath.Join(r.main, ".git"), true}, + {"a symlink to the main work tree", r.link, true}, + {"a linked worktree", r.linked, true}, + {"a linked worktree with a trailing slash", r.linked + "/", true}, + {"the linked worktree's administrative directory", r.adminDir, true}, + {"an unrelated repository with the same head", r.other, false}, + {"the forge slug of a path-accepted node", fxRepo, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.exec("UPDATE dag_acceptances SET repository = ? WHERE acceptance_id = 'acc-rel-lane'", r.main) + turn := store.MergeTurnsRow{TurnID: "mtn-riwt", TargetKey: "tgt-x", Repository: c.repository, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if c.held != (refusal != nil) { + t.Fatalf("turn repository %q: held = %v, want %v", c.repository, refusal != nil, c.held) + } + if refusal != nil && refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("the refusal is %q, want disposition_conflict", refusal.Reason) + } + }) + } +} + +// TestTheLaneGateHoldsADirectoryWhoseNameEndsInASpace: the acceptance names the directory with a trailing space; its +// own spelling and its /. spelling hold, and the trimmed name, which is another directory, does not. +func TestTheLaneGateHoldsADirectoryWhoseNameEndsInASpace(t *testing.T) { + r := riSetup(t) + for _, c := range []struct { + repository string + held bool + }{{r.spaced, true}, {r.spaced + "/.", true}, {strings.TrimSpace(r.spaced), false}} { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.exec("UPDATE dag_acceptances SET repository = ? WHERE acceptance_id = 'acc-rel-lane'", r.spaced) + turn := store.MergeTurnsRow{TurnID: "mtn-space", TargetKey: "tgt-x", Repository: c.repository, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if c.held != (refusal != nil) { + t.Fatalf("turn repository %q: held = %v, want %v", c.repository, refusal != nil, c.held) + } + } +} diff --git a/internal/relay/mergeturn/target.go b/internal/relay/mergeturn/target.go index 448c5b8c2f..92bbbc18cb 100644 --- a/internal/relay/mergeturn/target.go +++ b/internal/relay/mergeturn/target.go @@ -12,6 +12,7 @@ import ( "time" "github.com/thisisjun786/codex-relay-workflow/internal/pyvalue" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/commitid" ) // Tip is the branch's current full object name, not a caller's restatement. @@ -54,9 +55,7 @@ func branch(base string) error { return nil } func SameCommit(a, b string) bool { - a = strings.ToLower(strings.TrimSpace(a)) - b = strings.ToLower(strings.TrimSpace(b)) - return a != "" && a == b + return commitid.Same(a, b) } // Reader is mergeturn.py's target_reader: where a target's base branch points now. @@ -96,10 +95,7 @@ func (r TargetReader) Tip(ctx context.Context, repository, base string) (Tip, er if err != nil || !info.IsDir() { return Tip{}, unreadable("repository %s is not a directory here", pyvalue.StrRepr(repository)) } - gitdir := filepath.Join(repository, ".git") - if _, err := os.Stat(gitdir); os.IsNotExist(err) { - gitdir = repository - } + gitdir := gitDirOf(repository) ref := "refs/heads/" + base git := r.Git if git == "" { @@ -187,6 +183,87 @@ func (r TargetReader) github(ctx context.Context, repository, base string) (Tip, } // excerpt is text[:EXCERPT], counted in characters as Python slices a str. +// gitDirOf is the git directory the lane reads a local repository through: its .git child when it exists, +// else the path itself (a bare repository, or a .git directory named directly). The repository is resolved by +// the kernel first, as the lane's own stat of it is, so a spelling with a symlink followed by a parent segment +// (link/../repo) names the same directory here as it does in the reader; filepath.Join alone would collapse the +// segment before the link was followed. +func gitDirOf(repository string) string { + if resolved, err := filepath.EvalSymlinks(repository); err == nil { + repository = resolved + } + gitdir := filepath.Join(repository, ".git") + if _, err := os.Stat(gitdir); os.IsNotExist(err) { + return repository + } + return gitdir +} + +// repositoryKeyTimeout bounds the one git call that names a local repository. A repository that does not answer +// in time is not named, and its spellings are then compared as text. +const repositoryKeyTimeout = 5 * time.Second + +// SameRepository reports whether two repository spellings name one repository. The two kinds of spelling are read +// differently, and neither is interpreted by the relay itself. +// +// A forge slug (owner/name) is a name: two slugs name the same repository when they are equal after trimming and +// lower-casing. A local path is a directory name, so its bytes are never trimmed or case-folded: a directory whose +// name ends in a space is a different name from the one without it. Two paths name the same repository when git +// says so: the identity of a path is the common git directory git reports for it (git rev-parse --git-common-dir, +// with symlinks resolved), so a main work tree, its .git directory, a linked worktree (which shares the refs and +// so is the same repository for a hold on a head), its administrative directory and a symlink to any of them all +// name one repository. When git cannot name either path (it is not a repository here, git is missing, or it does +// not answer in time), the two spellings are compared by their exact text, case-insensitively, and nothing else. +// A slug and a path never name the same repository, and an empty spelling names none. +func SameRepository(a, b string) bool { + aPath, bPath := strings.HasPrefix(a, "/"), strings.HasPrefix(b, "/") + if aPath != bPath { + return false + } + if !aPath { + a, b = strings.TrimSpace(a), strings.TrimSpace(b) + if a == "" || b == "" { + return false + } + return strings.EqualFold(a, b) + } + keyA, okA := repositoryKey(a) + keyB, okB := repositoryKey(b) + if okA && okB { + return keyA == keyB + } + return strings.EqualFold(a, b) +} + +// repositoryKey is the identity git gives a local repository spelling: its common git directory with symlinks +// resolved. The git call runs with the GIT_* environment scrubbed, as TargetReader.Tip scrubs it, so the answer is +// the repository's own and not one a caller's environment redirects. +func repositoryKey(path string) (string, bool) { + ctx, cancel := context.WithTimeout(context.Background(), repositoryKeyTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "git", "-C", path, "rev-parse", "--path-format=absolute", "--git-common-dir") + env := make([]string, 0) + for _, v := range os.Environ() { + if !strings.HasPrefix(v, "GIT_") { + env = append(env, v) + } + } + cmd.Env = append(env, "GIT_TERMINAL_PROMPT=0", "LC_ALL=C") + output, err := cmd.Output() + if err != nil { + return "", false + } + common := strings.TrimSuffix(string(output), "\n") + if !filepath.IsAbs(common) { + return "", false + } + resolved, err := filepath.EvalSymlinks(common) + if err != nil { + return "", false + } + return resolved, true +} + func excerpt(text string) string { runes := []rune(text) if len(runes) > 400 { diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index 08ae7f1e1b..cc6f8f87f3 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -247,24 +247,26 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] } // every member (the leader included) must stand on an active acceptance whose stand head is the // head its pull request shows and its turn holds (Blocking 1): in the single lane that tie is - // dag-accept, which the bundle path would otherwise never pass through - standFor := func(relationship, memberHead string) (acceptance.Active, string, string, string, error) { - active, found, err := acceptance.ActiveForRelationship(ctx, s.Store.Querier(ctx), relationship) + // dag-accept, which the bundle path would otherwise never pass through. The reading carries the + // correction gate too (CRW-906): a member whose accepted result is being corrected in an open + // generation is not a candidate this bundle may carry. + standFor := func(relationship, memberHead string) (acceptance.Active, string, string, string, bool, int64, int64, error) { + active, live, found, err := acceptance.ActiveWithLiveGeneration(ctx, s.Store.Querier(ctx), relationship) if err != nil { - return acceptance.Active{}, "", "", "", trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + return acceptance.Active{}, "", "", "", false, 0, 0, trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) } if !found { - return acceptance.Active{}, "", "", "", nil + return acceptance.Active{}, "", "", "", false, live, 0, nil } stand, err := acceptance.StandOf(ctx, s.Store.Querier(ctx), active.AcceptanceID, relationship, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) if err != nil { - return acceptance.Active{}, "", "", "", trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + return acceptance.Active{}, "", "", "", false, live, 0, trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) } rulingHead, rulingSource, err := acceptance.RulingHead(ctx, s.Store.Querier(ctx), active.EventID, active.HeadSHA) if err != nil { - return acceptance.Active{}, "", "", "", trainUnreadable("the ruling head of event %s was not read: %v", pyvalue.StrRepr(active.EventID), err) + return acceptance.Active{}, "", "", "", false, live, 0, trainUnreadable("the ruling head of event %s was not read: %v", pyvalue.StrRepr(active.EventID), err) } - return active, stand.Head, rulingHead, rulingSource, nil + return active, stand.Head, rulingHead, rulingSource, live > stand.Generation, live, stand.Generation, nil } for _, pr := range members { pull, err := forge.PullRequest(ctx, early.Repository, pr) @@ -284,11 +286,11 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] if !SameCommit(pull.HeadSHA, early.CandidateHead) { return nil, trainConflict("the leader's pull request %d reads head %s and its turn holds %s; a member's head is not refreshed by the bundle", pr, pyvalue.StrRepr(pull.HeadSHA), pyvalue.StrRepr(early.CandidateHead)) } - active, standHead, rulingHead, rulingSource, err := standFor(early.RelationshipID.String, early.CandidateHead) + active, standHead, rulingHead, rulingSource, under, live, standGeneration, err := standFor(early.RelationshipID.String, early.CandidateHead) if err != nil { return nil, err } - if err := trainStandRefusal(pr, early.RelationshipID.String, early.CandidateHead, active, standHead); err != nil { + if err := trainStandRefusal(pr, early.RelationshipID.String, early.CandidateHead, active, standHead, under, live, standGeneration); err != nil { return nil, err } expectations = append(expectations, TrainMemberExpectation{TurnID: turn, PRNumber: pr, RelationshipID: early.RelationshipID.String, AcceptedHead: early.CandidateHead, RuledEventID: active.EventID, RulingHead: rulingHead, RulingHeadSource: rulingSource}) @@ -307,11 +309,11 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] if !member.RelationshipID.Valid || member.RelationshipID.String == "" { return nil, trainConflict("pull request %d's turn %s records no relationship, so its landing cannot be recorded", pr, pyvalue.StrRepr(member.TurnID)) } - active, standHead, rulingHead, rulingSource, err := standFor(member.RelationshipID.String, member.CandidateHead) + active, standHead, rulingHead, rulingSource, under, live, standGeneration, err := standFor(member.RelationshipID.String, member.CandidateHead) if err != nil { return nil, err } - if err := trainStandRefusal(pr, member.RelationshipID.String, member.CandidateHead, active, standHead); err != nil { + if err := trainStandRefusal(pr, member.RelationshipID.String, member.CandidateHead, active, standHead, under, live, standGeneration); err != nil { return nil, err } expectations = append(expectations, TrainMemberExpectation{TurnID: member.TurnID, PRNumber: pr, RelationshipID: member.RelationshipID.String, AcceptedHead: member.CandidateHead, RuledEventID: active.EventID, RulingHead: rulingHead, RulingHeadSource: rulingSource}) @@ -340,6 +342,14 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] if e := s.trainMembershipRefusal(tx, actor, expectations); e != nil { return e } + // CRW-906: every member's correction state is read again on this transaction's snapshot. The + // reads above happen before it, and a generation opened in that gap would otherwise be recorded + // as an opened train: nothing is written for a member whose accepted result is under correction. + for _, m := range expectations { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { + return e + } + } // a train id names one opening, not the candidate: a turn that abandons a train and reopens // one for the same head gets a fresh id rather than colliding with the abandoned record for suffix := 2; ; suffix++ { @@ -454,14 +464,21 @@ func waitingTurnFor(turns []store.MergeTurnsRow, pr int64) (store.MergeTurnsRow, } // trainStandRefusal is Blocking 1's gate: the member must have an active acceptance whose stand head -// is the head its pull request shows and its turn holds. -func trainStandRefusal(pr int64, relationship, memberHead string, active acceptance.Active, standHead string) error { +// is the head its pull request shows and its turn holds, and no correction generation may be open over +// the accepted result (CRW-906). A defect found in an accepted result before it lands is corrected in a +// later generation; until that generation's result is accepted with dag-accept --supersedes, or the +// generation is withdrawn, the old head is not a candidate a bundle may carry or land. The refusal is +// the existing disposition_conflict and it names the open generation: no new refusal name, no column. +func trainStandRefusal(pr int64, relationship, memberHead string, active acceptance.Active, standHead string, underCorrection bool, liveGeneration, standGeneration int64) error { if active.AcceptanceID == "" { return trainConflict("pull request %d's relationship %s has no active acceptance, so the member is not a verified, accepted candidate this bundle may carry; run dag-accept on its head first", pr, pyvalue.StrRepr(relationship)) } if !SameCommit(standHead, memberHead) { return trainConflict("pull request %d stands on %s and its acceptance %s stands on %s, so the member's head is not the head the ruling covers", pr, pyvalue.StrRepr(memberHead), pyvalue.StrRepr(active.AcceptanceID), pyvalue.StrRepr(standHead)) } + if underCorrection { + return trainUnderCorrectionRefusal(pr, relationship, liveGeneration, standGeneration) + } return nil } @@ -723,6 +740,14 @@ func (s *Service) Verify(ctx context.Context, train, actor, bundlePR, head, run, if !found || (current != store.MergeTrainOpened && current != store.MergeTrainVerified) { return trainConflict("train %s changed while the bundle was being read, so it is no longer a train a verify may advance; call again", pyvalue.StrRepr(train)) } + // CRW-906: each member's correction state is read again on this transaction's snapshot. The + // expectations above were read before the run and the chain were proved, and a generation opened + // in that gap would otherwise be recorded as verified: nothing is written for such a member. + for _, m := range expected { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { + return e + } + } return store.RecordMergeTrainEvent(tx, s.Store, store.MergeTrainEventRow{TrainID: train, Seq: seq, Kind: store.MergeTrainVerified, Actor: actor, DetailJSON: pythonJSON(detail), RecordedAt: at}) }) if err != nil { @@ -738,7 +763,7 @@ func (s *Service) Verify(ctx context.Context, train, actor, bundlePR, head, run, func (s *Service) trainExpectations(ctx context.Context, members []store.MergeTrainMemberRow) ([]TrainMemberExpectation, error) { out := make([]TrainMemberExpectation, 0, len(members)) for _, m := range members { - active, found, err := acceptance.ActiveForRelationship(ctx, s.Store.Querier(ctx), m.RelationshipID) + active, live, found, err := acceptance.ActiveWithLiveGeneration(ctx, s.Store.Querier(ctx), m.RelationshipID) if err != nil { return nil, trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(m.RelationshipID), err) } @@ -749,7 +774,7 @@ func (s *Service) trainExpectations(ctx context.Context, members []store.MergeTr if err != nil { return nil, trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) } - if err := trainStandRefusal(m.PRNumber, m.RelationshipID, m.MemberHead, active, stand.Head); err != nil { + if err := trainStandRefusal(m.PRNumber, m.RelationshipID, m.MemberHead, active, stand.Head, live > stand.Generation, live, stand.Generation); err != nil { return nil, err } rulingHead, rulingSource, err := acceptance.RulingHead(ctx, s.Store.Querier(ctx), active.EventID, active.HeadSHA) @@ -944,6 +969,13 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed survivors := make([]store.MergeTrainMemberRow, 0, len(members)) turns := make(map[string]store.MergeTurnsRow, len(members)) excluded := make([]any, 0, len(members)) + // CRW-906: the members whose turn left the lane are excluded from the landing, but their code is + // still in the merge commit this bundle lands (every member head is checked as an ancestor of it + // above), so their correction state is read too. The CRW-897 carve-out keeps holding for what it + // is about (a revoked acceptance or a moved head of a member that left does not refuse the rest + // of the bundle); this is the separate question of whether the tree being landed still contains a + // result the plan is repairing. + leftMembers := make([]store.MergeTrainMemberRow, 0, len(members)) for _, m := range members { turn, e := s.Store.MergeTurn(tx, m.TurnID) if e != nil { @@ -951,6 +983,7 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed } if turn.State != Holding && turn.State != Waiting { excluded = append(excluded, map[string]any{"seq": m.Seq, "turnId": m.TurnID, "prNumber": m.PRNumber, "relationshipId": m.RelationshipID, "state": turn.State, "closeReason": value(turn.CloseReason)}) + leftMembers = append(leftMembers, m) continue } if !SameCommit(turn.CandidateHead, m.MemberHead) { @@ -963,6 +996,16 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed // or moved after open refuses the landing with nothing written (Blocking 1). A member that // left the lane is excluded above and is not re-read, so its parent revoking the acceptance // does not refuse the rest of the bundle. + for _, m := range survivors { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { + return e + } + } + for _, m := range leftMembers { + if e := trainExcludedMemberRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { + return e + } + } expectations, e := s.trainExpectations(tx, survivors) if e != nil { return e diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go new file mode 100644 index 0000000000..b92bfd080e --- /dev/null +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -0,0 +1,766 @@ +package mergeturn + +import ( + "context" + "database/sql" + "fmt" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// CRW-906 generation 2: the bundle half of the issue's decision 2. A node whose accepted result is being +// corrected is not merged on its old head. The bundle gate already refuses a member whose pull request +// head differs from the accepted head, and that guard holds only once the child has pushed: between +// opening (or recording) the correction generation and that first push the pull request still shows the +// accepted head H, so the train must refuse by the generation alone. These tests drive open, verify and +// land, each writing nothing. + +// ucCorrection opens a correction generation of a relationship by hand, the shape generation-open leaves: +// the generations row and the relationship's live execution generation moved to it. The turn fixture +// seeds an acceptance without a relationships row, so the row is written here too. +func (w *tr) ucCorrection(relationship string) { + w.t.Helper() + allowed := fmt.Sprintf("[%q]", trLeader) + w.exec("INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', ?, 'host-a', 'child-1', 'host-c', 2, '[]', ?, '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, trLeader, allowed) + w.exec("INSERT INTO generations (relationship_id, execution_generation, dispatch_request_id, anchor_state, dispatch_turn_id, reason, opened_at, bound_at)"+ + " VALUES (?, 2, ?, 'bound', 'turn-dispatch-2', 'needs_changes_revision', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, "correction-"+relationship) +} + +// TestTrainRefusesAMemberUnderCorrection: open, verify and land each refuse a member whose accepted +// result has an open correction generation, name it, and write nothing. +func TestTrainRefusesAMemberUnderCorrection(t *testing.T) { + t.Run("open refuses the member and writes no train", func(t *testing.T) { + w := newTr(t) + leader, members := w.threeMembers() + w.ucCorrection("rel-task-m2") + _, err := w.open(leader, trLeader, "base-0", members...) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member under correction: %v", err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if n := w.trainEvents(); n != 0 { + t.Fatalf("a refused open wrote %d event(s)", n) + } + }) + t.Run("verify refuses after the correction opened and writes no verified event", func(t *testing.T) { + w := newTr(t) + train := w.openedTrain() + w.pr(900, "head-bundle", TrainLaneLabel) + w.forge.runs["run-1"] = runFor("head-bundle") + w.ucCorrection("rel-task-m3") + if _, err := w.m.Verify(w.ctx, train, trLeader, "900", "head-bundle", "run-1", "/checkout", w.forge, w.proof); err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member under correction at verify: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'verified'"); n != 0 { + t.Fatalf("a refused verify wrote %d verified event(s)", n) + } + }) + t.Run("land refuses and lands no turn", func(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + w.ucCorrection("rel-task-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err := w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member under correction at land: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } + }) +} + +// TestTrainCarriesTheMemberAgainAfterTheCorrectionCloses: the gate is about the correction being open, +// not about the head. A withdrawn generation puts the relationship back on the generation the acceptance +// stands on, and the member rides again on the same head. +func TestTrainCarriesTheMemberAgainAfterTheCorrectionCloses(t *testing.T) { + w := newTr(t) + leader, members := w.threeMembers() + w.ucCorrection("rel-task-m2") + if _, err := w.open(leader, trLeader, "base-0", members...); err == nil { + t.Fatal("a member under correction was carried") + } + // the withdrawal puts the relationship back on the generation its acceptance stands on + w.exec("UPDATE relationships SET execution_generation = 1 WHERE relationship_id = 'rel-task-m2'") + answer, err := w.open(leader, trLeader, "base-0", members...) + if err != nil { + t.Fatalf("the bundle after the correction was withdrawn: %v", err) + } + if answer["train"] == nil { + t.Fatalf("the bundle after the correction was withdrawn = %v, want a train", answer) + } +} + +// ucLaneReport is the work report the lane's head comparison reads: the assignment the turn names has a +// report naming the head, so the check has something to compare the restated head with. +func (w *fx) ucLaneReport(relationship, head string) { + w.t.Helper() + w.exec("INSERT INTO work_reports (event_id, submission_no, relationship_id, execution_generation, revision_hash, repository, head_sha, cxc_status, cxc_reason, contract_version, summary, next_action, recorded_at)"+ + " VALUES (?, 1, ?, 1, ?, ?, ?, 'DONE', 'proved', 'v1', 'done', 'merge', '2023-11-14T22:13:20.000000+00:00')", + "ev-report-"+relationship, relationship, "rev-"+relationship, fxRepo, head) +} + +// ucLaneRelationship writes the relationship the turn names at the generation given, and (when the +// generation is 2) the correction generation opened over the accepted result. The fx fixture seeds no +// relationship row, so one is written for the turn the test holds. +func (w *fx) ucLaneRelationship(relationship string, generation int64) { + w.t.Helper() + w.exec("INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', ?, 'host-a', 'child-1', 'host-c', ?, '[]', ?, '2023-11-14T22:13:20.000000+00:00', '2023-11-14T22:13:20.000000+00:00')", + relationship, alpha.TaskID, generation, fmt.Sprintf("[%q]", alpha.TaskID)) + if generation > 1 { + w.exec("INSERT INTO generations (relationship_id, execution_generation, dispatch_request_id, anchor_state, dispatch_turn_id, reason, opened_at, bound_at)"+ + " VALUES (?, ?, ?, 'bound', 'turn-dispatch-2', 'needs_changes_revision', '2023-11-14T22:13:20.000000+00:00', '2023-11-14T22:13:20.000000+00:00')", + relationship, generation, "correction-"+relationship) + // the acceptance the correction is opened over: the gate compares the live generation with the + // generation this acceptance stands on + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES (?, 'plan-x', 'node-1', ?, ?, 1, ?, ?, 'crit-1', 'verified', 'head-a', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:20.000000+00:00', 'active')", + "acc-"+relationship, "manifest-"+relationship, relationship, "ev-"+relationship, "rev-"+relationship, fxRepo, alpha.TaskID) + } +} + +// ucLaneForge is the acceptance's forge identity, written with the acceptance: the mapping from a pull +// request back to the relationship whose result it carries. +func (w *fx) ucLaneForge(relationship string, pr int64) { + w.t.Helper() + w.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES (?, ?, ?)", "acc-"+relationship, fxRepo, pr) +} + +// CRW-906 generation 2: the lane's CLI lets a claim carry a forge pull request instead of a +// relationship, and that route must meet the same gate. The acceptance's forge identity is what maps +// the turn's pull request back to the relationship whose correction is open, so a PR-only turn is +// refused on the head the correction is repairing rather than exempted by the field it omits. +func TestTheLaneGateResolvesTheRelationshipFromThePullRequest(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.ucLaneForge("rel-lane", 7) + turn := store.MergeTurnsRow{TurnID: "mtn-pr-only", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", PRNumber: sql.NullInt64{Int64: 7, Valid: true}, State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a PR-only turn whose accepted result is under correction was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) + } + // a turn whose pull request has no accepted result, and whose head no acceptance stands on, is left + // to the lane's own rules + other := turn + other.PRNumber = sql.NullInt64{Int64: 99, Valid: true} + other.CandidateHead = "head-unaccepted" + if refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), other); err != nil || refusal != nil { + t.Fatalf("a turn whose pull request has no accepted result = %+v %v, want no refusal", refusal, err) + } +} + +// CRW-906 generation 2: --relationship and --pr are both optional on merge-turn-request, so a claim can +// carry neither and hold only the head. The accepted head of a node under correction is then the one +// identity left, and the acceptance that recorded it is what the head belongs to: the turn is refused on +// the head the correction is repairing rather than exempted by the two selectors it omits. +func TestTheLaneGateResolvesTheRelationshipFromTheHeadItHolds(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + turn := store.MergeTurnsRow{TurnID: "mtn-bare", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn naming neither selector, holding an accepted head under correction, was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) + } + // a head no active acceptance of that repository stands on is left to the lane's own rules + other := turn + other.CandidateHead = "head-unaccepted" + if refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), other); err != nil || refusal != nil { + t.Fatalf("a turn whose head no acceptance stands on = %+v %v, want no refusal", refusal, err) + } +} + +// ucHeldOn is a held, acknowledged turn on a head whose claim records a relationship. +func (w *fx) ucHeldOn(relationship, head string) string { + w.t.Helper() + options := ClaimOptions{Relationship: sql.NullString{String: relationship, Valid: true}} + turn := w.must(w.m.Request(w.ctx, fxRepo, fxBase, fxA, alpha.TaskID, alpha.HostID, head, true, options))["turnId"].(string) + w.answer(turn, alpha.TaskID) + return turn +} + +// CRW-906 generation 2, the lane's own half: a merge turn can have been granted before the correction was +// opened, so the check that moves it to merging and the land that records the merge read the correction +// state themselves, inside the transaction that writes. Both refuse disposition_conflict naming the open +// generation, the check writes no current row and the turn stays holding, and the land records no landing +// and leaves the turn merging. +func TestTheLaneRefusesATurnWhoseAcceptedResultIsUnderCorrection(t *testing.T) { + t.Run("the check refuses and the turn stays holding", func(t *testing.T) { + w := newFx(t) + turn := w.ucHeldOn("rel-lane", "head-a") + w.ucLaneReport("rel-lane", "head-a") + w.ucLaneRelationship("rel-lane", 2) + _, err := w.check(turn, "head-a", "base-0", "") + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("check under correction: %v", err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if got := w.must(w.m.Turn(w.ctx, turn))["state"]; got != "holding" { + t.Fatalf("the turn is %v after the refused check, want holding", got) + } + if n := w.ucCount("SELECT count(*) FROM merge_turn_checks WHERE result = 'current'"); n != 0 { + t.Fatalf("the refused check wrote %d current row(s)", n) + } + }) + t.Run("the land refuses and records no landing", func(t *testing.T) { + w := newFx(t) + turn := w.ucHeldOn("rel-lane", "head-a") + w.ucLaneReport("rel-lane", "head-a") + w.must(w.check(turn, "head-a", "base-0", "")) + w.merged("merge-1") + w.ucLaneRelationship("rel-lane", 2) + _, err := w.land(turn, "merge-1", "", "") + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("land under correction: %v", err) + } + if !strings.Contains(err.Error(), "under correction") { + t.Fatalf("the refusal does not say why: %v", err) + } + if got := w.must(w.m.Turn(w.ctx, turn))["state"]; got != "merging" { + t.Fatalf("the turn is %v after the refused land, want merging", got) + } + if n := w.ucCount("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("the refused land landed %d turn(s)", n) + } + }) + t.Run("a turn whose relationship is not under correction is unchanged", func(t *testing.T) { + w := newFx(t) + turn := w.ucHeldOn("rel-lane", "head-a") + // the relationship stands on the generation its acceptance stands on: no correction is open + w.ucLaneReport("rel-lane", "head-a") + w.ucLaneRelationship("rel-lane", 1) + if _, err := w.check(turn, "head-a", "base-0", ""); err != nil { + t.Fatalf("a turn with no correction open was refused: %v", err) + } + }) +} + +// CRW-906 generation 2: a recorded base refresh moves the head the acceptance stands on without changing +// the accepted head, so a lane turn that names neither selector and holds the refreshed head must still +// resolve to the relationship whose correction is open. The head it holds is the accepted head or the head +// of a refresh of it, and either belongs to the same acceptance. +func TestTheLaneGateResolvesARefreshedHead(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + // the acceptance stands on head-a; a base refresh moved it to head-refreshed + refresh := "dbr-" + strings.Repeat("a", 60) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, ?, 1, ?, 1, 'ev-refresh', 'rev-refresh', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:20.000000+00:00')", + refresh, "acc-rel-lane", "rel-lane", fxRepo, fxBase, alpha.TaskID) + turn := store.MergeTurnsRow{TurnID: "mtn-refreshed", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-refreshed", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn holding the refreshed head of an accepted result under correction was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) + } +} + +// CRW-906 generation 2: a member whose turn left the lane is excluded from the landing, but its code is +// CRW-906 generation 2, round 6 d2: a recorded base refresh outlives the acceptance it was recorded for, +// so a turn held for a refreshed head must still resolve once dag-accept --supersedes replaced the +// acceptance: otherwise the obsolete result merges after the correction was accepted over. +func TestTheLaneGateRefusesAReplacedHeadThatWasOnceRefreshed(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.ucLaneForge("rel-lane", 7) + // the acceptance stood on head-a, a base refresh moved it to head-refreshed, and then the whole + // acceptance was replaced by the corrected one + refresh := "dbr-" + strings.Repeat("b", 60) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, ?, 1, ?, 1, 'ev-refresh', 'rev-refresh', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:20.000000+00:00')", + refresh, "acc-rel-lane", "rel-lane", fxRepo, fxBase, alpha.TaskID) + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-lane'") + turn := store.MergeTurnsRow{TurnID: "mtn-refreshed-replaced", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-refreshed", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn holding a refreshed head whose acceptance was replaced was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, round 6 d3: an acceptance written before the forge rule keeps whatever target it +// was accepted against (a local checkout included) while dag_acceptance_forge holds the owner/name a merge +// turn is requested against. The lane gate must match the forge identity too, not the stored target. +func TestTheLaneGateMatchesATurnByItsForgeIdentityNotTheStoredTarget(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-local", 2) + w.ucLaneForge("rel-local", 7) + w.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout' WHERE relationship_id = 'rel-local'") + // a turn of the real forge repository, naming neither selector, already merging on the accepted head + turn := store.MergeTurnsRow{TurnID: "mtn-legacy", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Merging} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a legacy acceptance's forge turn was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-local") { + t.Fatalf("the refusal does not name the relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2: a member whose turn left the lane is excluded from the landing, but its code is +// still in the merge commit the bundle lands (every member head is checked as an ancestor of it), so the +// bundle must not record a landing of a tree that still carries a result the plan is repairing. The +// CRW-897 carve-out keeps holding for what it is about: a revoked acceptance or a moved head of a member +// that left does not refuse the rest of the bundle. +func TestTrainLandRefusesAnExcludedMemberUnderCorrection(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + rows, err := w.s.All(w.ctx, "SELECT turn_id FROM merge_train_members WHERE train_id = ? AND seq = 2", train) + if err != nil || len(rows) != 1 { + t.Fatalf("member 2: %v", err) + } + memberTurn := rows[0].Get("turn_id").(string) + // the member's parent withdraws its waiting turn, and the correction over its accepted result opens + if _, err := w.m.Withdraw(w.ctx, memberTurn, "task-m2"); err != nil { + t.Fatalf("withdrawing the member's turn: %v", err) + } + w.ucCorrection("rel-task-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err = w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land of a bundle still carrying an excluded member under correction: %v", err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } +} + +// CRW-906 generation 2, d2: acceptance uniqueness is per node and output, not per repository and pull +// request or per repository and head, so two accepted nodes can name the same commit. The gate must ask +// every matching relationship, not the newest one: a turn is refused when any of them is correcting the +// very head it holds. +func TestTheLaneGateAsksEveryMatchingAcceptance(t *testing.T) { + w := newFx(t) + // rel-a-live stands on its live generation and sorts first, so a lookup that took one arbitrary + // match would take it; rel-z-correcting has a correction open over the same head + w.ucLaneRelationship("rel-a-live", 1) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-a-live', 'plan-x', 'node-live', 'manifest-live', 'rel-a-live', 1, 'ev-live', 'rev-live', 'crit-1', 'verified', 'head-a', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:19.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + w.ucLaneRelationship("rel-z-correcting", 2) + turn := store.MergeTurnsRow{TurnID: "mtn-two", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a head two nodes accept, one of them under correction, was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-z-correcting") { + t.Fatalf("the refusal does not name the correcting relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was +// CRW-906 generation 2, d3: the recheck inside the write transaction asks about the head the bundle +// CRW-906 generation 2, d1: naming a relationship must not suppress the other identities. Acceptance +// CRW-906 generation 2: a bundle member is refused when the head it carries is the result another +// CRW-906 generation 2, round 6 d1: the landing transaction must apply the same identity-based gate the +// open and verify use. A correction of another relationship that accepts the same head, opened after the +// bundle was verified, is refused at land: otherwise the landing records the exact result being repaired. +func TestTrainLandRefusesAMemberWhoseHeadAnotherRelationshipIsRepairing(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + w.ucOtherCorrection("rel-other", "head-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err := w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land whose member's head another relationship is repairing: %v", err) + } + if !strings.Contains(err.Error(), "rel-other") { + t.Fatalf("the refusal does not name the repairing relationship: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } +} + +// CRW-906 generation 2: a bundle member is refused when the head it carries is the result another +// relationship is repairing, even though the member's own relationship is live. Acceptance uniqueness is +// per node and output, so two accepted nodes can name the same commit, and the bundle must apply the same +// rule the single lane does. +func TestTrainRefusesAMemberWhoseHeadAnotherRelationshipIsRepairing(t *testing.T) { + w := newTr(t) + train := w.openedTrain() + w.pr(900, "head-bundle", TrainLaneLabel) + w.forge.runs["run-1"] = runFor("head-bundle") + w.ucOtherCorrection("rel-other", "head-m2") + _, err := w.m.Verify(w.ctx, train, trLeader, "900", "head-bundle", "run-1", "/checkout", w.forge, w.proof) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member whose head another relationship is repairing: %v", err) + } + if !strings.Contains(err.Error(), "rel-other") { + t.Fatalf("the refusal does not name the repairing relationship: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'verified'"); n != 0 { + t.Fatalf("a refused verify wrote %d verified event(s)", n) + } +} + +// ucOtherCorrection writes another relationship's active acceptance on a head with a correction +// generation open over it: the shape the bundle gate must see through a member's own live relationship. +func (w *tr) ucOtherCorrection(relationship, head string) { + w.t.Helper() + allowed := fmt.Sprintf("[%q]", trLeader) + w.exec("INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', ?, 'host-a', 'child-x', 'host-c', 2, '[]', ?, '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, trLeader, allowed) + w.exec("INSERT INTO generations (relationship_id, execution_generation, dispatch_request_id, anchor_state, dispatch_turn_id, reason, opened_at, bound_at)"+ + " VALUES (?, 2, ?, 'bound', 'turn-dispatch-x', 'needs_changes_revision', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, "correction-"+relationship) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES (?, 'plan-x', 'node-x', 'manifest-x', ?, 1, 'ev-x', 'rev-x', 'crit-x', 'verified', ?, ?, 1, 'bound', 'turn-x', '{}', ?, 0, '2026-10-01T00:00:00Z', 'active')", + "acc-"+relationship, relationship, head, trRepo, trLeader) + w.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES (?, 'owner/repo', 1)", "acc-"+relationship) +} + +// CRW-906 generation 2: an acceptance recorded before the forge rule keeps whatever target it was +// accepted against — a local checkout included — while dag_acceptance_forge holds the owner/name a merge +// turn is actually requested against. The gate matches the forge identity, not the stored target. +func TestTheLaneGateMatchesTheForgeIdentityNotTheStoredTarget(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-local", 2) + w.ucLaneForge("rel-local", 7) + w.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout' WHERE relationship_id = 'rel-local'") + for _, turn := range []store.MergeTurnsRow{ + {TurnID: "mtn-by-pr", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", PRNumber: sql.NullInt64{Int64: 7, Valid: true}, State: Holding}, + {TurnID: "mtn-by-head", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding}, + } { + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("%s: a turn matched by the forge identity rather than the stored target was not refused: %+v", turn.TurnID, refusal) + } + if !strings.Contains(refusal.Detail, "rel-local") { + t.Fatalf("%s: the refusal does not name the relationship: %s", turn.TurnID, refusal.Detail) + } + } +} + +// CRW-906 generation 2: after dag-accept --supersedes the head an acceptance recorded belongs to no +// active acceptance any more, so a turn held for it would otherwise merge exactly the result the +// correction replaced. +func TestTheLaneGateRefusesAHeadAnAcceptanceRecordedAndThenReplaced(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.ucLaneForge("rel-lane", 7) + // the acceptance that recorded head-a was replaced: a later one stands on the corrected head now + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-lane'") + turn := store.MergeTurnsRow{TurnID: "mtn-replaced", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn holding a replaced head was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-lane") || !strings.Contains(refusal.Detail, "replaced") { + t.Fatalf("the refusal does not name the relationship and what happened: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, d1: naming a relationship must not suppress the other identities. Acceptance +// uniqueness is per node and output, so two nodes can accept the same commit: a turn that names the +// relationship which is NOT being corrected is still carrying a head another relationship is repairing, +// and the gate must read that one too rather than trusting the name it was given. +func TestTheLaneGateAsksTheOtherAcceptancesOfANamedRelationship(t *testing.T) { + w := newFx(t) + // rel-named is the relationship the turn names, and it is not under correction + w.ucLaneRelationship("rel-named", 1) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-named', 'plan-x', 'node-named', 'manifest-named', 'rel-named', 1, 'ev-named', 'rev-named', 'crit-1', 'verified', 'head-shared', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:19.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + // rel-correcting accepts the same head and has a correction open over it + w.ucLaneRelationship("rel-correcting", 2) + w.exec("UPDATE dag_acceptances SET head_sha = 'head-shared' WHERE relationship_id = 'rel-correcting'") + turn := store.MergeTurnsRow{TurnID: "mtn-named", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, RelationshipID: sql.NullString{String: "rel-named", Valid: true}, CandidateHead: "head-shared", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn naming a live relationship but holding a head another one is repairing was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-correcting") { + t.Fatalf("the refusal does not name the correcting relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, d3: the recheck inside the write transaction asks about the head the bundle +// carries, not only whether a correction is open. A correction opened AND accepted over inside the gap +// makes the live and stand generations equal again, so a guard that only asked UnderCorrection would let +// the obsolete bundle through with a verified event for a head the plan no longer accepts. +func TestTrainVerifyRefusesAMemberWhoseAcceptedResultMovedOn(t *testing.T) { + w := newTr(t) + train := w.openedTrain() + w.pr(900, "head-bundle", TrainLaneLabel) + w.forge.runs["run-1"] = runFor("head-bundle") + // the member's accepted result is corrected and accepted over while the run and the chain are being + // proved: the live and stand generations are equal again by the time the write transaction runs, so a + // guard that only asked whether a correction is open would let the obsolete bundle through. The + // mutation runs inside the proof, which is the gap the recheck exists for. + proof := &ucProver{trProof: w.proof, during: func() { + w.exec("UPDATE relationships SET execution_generation = 2 WHERE relationship_id = 'rel-task-m2'") + w.exec("UPDATE dag_acceptances SET execution_generation = 2, head_sha = 'head-m2-corrected' WHERE relationship_id = 'rel-task-m2' AND state = 'active'") + }} + _, err := w.m.Verify(w.ctx, train, trLeader, "900", "head-bundle", "run-1", "/checkout", w.forge, proof) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a verify whose member's accepted result moved on: %v", err) + } + if !strings.Contains(err.Error(), "head-m2-corrected") { + t.Fatalf("the refusal does not name the stand the acceptance moved to: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'verified'"); n != 0 { + t.Fatalf("a refused verify wrote %d verified event(s)", n) + } +} + +// CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was +// accepted over. Exclusion changes the accounting, not the bundle tree: the verified tree still carries the +// member head it was verified on, so once the acceptance stands on a corrected head the bundle no longer +// holds what the plan accepts for that member and must be rebuilt and verified. +func TestTrainLandRefusesAnExcludedMemberWhoseAcceptanceMovedOn(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + rows, err := w.s.All(w.ctx, "SELECT turn_id FROM merge_train_members WHERE train_id = ? AND seq = 2", train) + if err != nil || len(rows) != 1 { + t.Fatalf("member 2: %v", err) + } + memberTurn := rows[0].Get("turn_id").(string) + if _, err := w.m.Withdraw(w.ctx, memberTurn, "task-m2"); err != nil { + t.Fatalf("withdrawing the member's turn: %v", err) + } + // dag-accept --supersedes moved the acceptance onto the corrected result: the relationship and the + // acceptance stand on the same generation again, so no correction is open, and the head it stands on + // is the corrected one the verified bundle does not carry. + w.exec("UPDATE relationships SET execution_generation = 2 WHERE relationship_id = 'rel-task-m2'") + w.exec("UPDATE dag_acceptances SET execution_generation = 2, head_sha = 'head-m2-corrected' WHERE relationship_id = 'rel-task-m2' AND state = 'active'") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err = w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land of a bundle whose excluded member's acceptance moved on: %v", err) + } + if !strings.Contains(err.Error(), "head-m2-corrected") || !strings.Contains(err.Error(), "rebuild and verify") { + t.Fatalf("the refusal does not name the stand and the way on: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } +} + +// ucProver is the checkout stand-in with a seam: it runs during() inside the chain proof, which is the +// gap between the reads a verify makes and the transaction it writes in. That is where a member's +// accepted result can move without either read seeing it, and what the transaction recheck exists for. +type ucProver struct { + *trProof + during func() +} + +func (p *ucProver) Chain(ctx context.Context, checkout, head, base string, members []TrainMemberExpectation) (TrainChain, error) { + chain, err := p.trProof.Chain(ctx, checkout, head, base, members) + if err == nil && p.during != nil { + p.during() + } + return chain, err +} + +// ucCount is the number of rows a query answers, for the writes a refusal must not leave behind. +func (w *fx) ucCount(query string, args ...any) int64 { + w.t.Helper() + var n int64 + if err := w.s.DB.QueryRowContext(w.ctx, query, args...).Scan(&n); err != nil { + w.t.Fatal(err) + } + return n +} + +// CRW-906 final round, GLM P1: a head is one commit however it is spelled. merge-turn-request stores the +// caller's head verbatim, so a head-only turn holding the accepted head in upper case must still resolve +// to the relationship whose correction is open; comparing raw text would let it pass the lane gate. +func TestTheLaneGateMatchesAHeadRegardlessOfItsSpelling(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + turn := store.MergeTurnsRow{TurnID: "mtn-spelled", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: " HEAD-A ", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a head-only turn holding the accepted head in another spelling was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship: %s", refusal.Detail) + } +} + +// CRW-906 final round, evaluation 6f79654d D1: a head an accepted node replaced is not mergeable through another +// accepted node that still stands on it. The replaced-head check must run whenever the turn holds a head, not +// only when no relationship matched the head. +func TestTheLaneGateRefusesAReplacedHeadThatAnotherNodeStillAccepts(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-a", 2) + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-a'") + w.ucLaneRelationship("rel-b", 1) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-b', 'plan-x', 'node-b', 'manifest-b', 'rel-b', 1, 'ev-b', 'rev-b', 'crit-1', 'verified', 'head-a', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:19.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + turn := store.MergeTurnsRow{TurnID: "mtn-replaced-elsewhere", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a head another node accepts, after this node's correction replaced it, was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-a") { + t.Fatalf("the refusal does not name the replaced relationship: %s", refusal.Detail) + } +} + +// CRW-906 final round, delta review ad054763: a head the active acceptance of this relationship reaches only +// through its own base refresh is that relationship's current result, so a superseded acceptance's refresh +// row for the same head must not refuse a turn holding it. +func TestTheLaneGateKeepsAHeadThatTheActiveAcceptanceReachesByARefresh(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-c", 2) + refresh := "dbr-" + strings.Repeat("c", 60) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, 'acc-rel-c', 1, 'rel-c', 1, 'ev-r1', 'rev-r1', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:20.000000+00:00')", + refresh, fxRepo, fxBase, alpha.TaskID) + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-c'") + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-c2', 'plan-x', 'node-c', 'manifest-c2', 'rel-c', 2, 'ev-c2', 'rev-c2', 'crit-1', 'verified', 'head-b', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:21.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, 'acc-rel-c2', 1, 'rel-c', 2, 'ev-r2', 'rev-r2', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:22.000000+00:00')", + refreshIDFor("acc-rel-c2", "rel-c", 2, "ev-r2", "rev-r2", "head-refreshed", fxRepo, fxBase, "base-0"), fxRepo, fxBase, alpha.TaskID) + turn := store.MergeTurnsRow{TurnID: "mtn-refresh-current", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-refreshed", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal != nil { + t.Fatalf("a head the active acceptance reaches by its own refresh was refused: %s", refusal.Detail) + } +} + +// CRW-906 evaluation 7f7b39ca D1: a member that left the lane with no acceptance of its own (revoked) still carries +// its head in the bundle's tree, so another node's open correction over that head refuses the landing. The +// carve-out for a revoked acceptance does not cover another node's correction. +func TestTrainLandRefusesAnExcludedMemberWhoseHeadAnotherNodeIsRepairing(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + rows, err := w.s.All(w.ctx, "SELECT turn_id FROM merge_train_members WHERE train_id = ? AND seq = 2", train) + if err != nil || len(rows) != 1 { + t.Fatalf("member 2: %v", err) + } + memberTurn := rows[0].Get("turn_id").(string) + if _, err := w.m.Withdraw(w.ctx, memberTurn, "task-m2"); err != nil { + t.Fatalf("withdrawing the member's turn: %v", err) + } + w.exec("UPDATE dag_acceptances SET state = 'revoked' WHERE relationship_id = 'rel-task-m2'") + w.ucOtherCorrection("rel-other", "head-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err = w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land whose excluded member's head another node is repairing: %v", err) + } + if !strings.Contains(err.Error(), "rel-other") { + t.Fatalf("the refusal does not name the repairing relationship: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } +} + +// CRW-906 evaluation 596938c9 D1: a head padded with tab or newline is the same commit (SameCommit trims every +// whitespace). The correction gate must refuse it as the accepted head, whatever whitespace surrounds it. +func TestTheLaneGateMatchesAHeadPaddedWithTabsAndNewlines(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + turn := store.MergeTurnsRow{TurnID: "mtn-tabbed", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "\tHEAD-A\n", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a head padded with tabs and newlines was not refused: %+v", refusal) + } +} diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go new file mode 100644 index 0000000000..68b0c065ca --- /dev/null +++ b/internal/relay/mergeturn/undercorrection.go @@ -0,0 +1,328 @@ +package mergeturn + +import ( + "context" + "database/sql" + "errors" + "sort" + "strconv" + "strings" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/pyvalue" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/acceptance" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// underCorrectionRefusal is CRW-906's gate on the single lane's own operations. A defect found in an +// accepted result before it lands is corrected in a later generation of the same relationship, and +// until that generation's result is accepted over (dag-accept --supersedes) or the generation is +// withdrawn, the accepted head is the result being repaired: it must not be merged. +// +// The DAG judgement and the bundle gate refuse it already, but a merge turn can have been granted +// before the correction was opened, and the lane's own check and land are what actually carry the +// head to the base. They read the relationship's live execution generation against the generation its +// active acceptance stands on here, inside the transaction that writes, so a generation opened while +// the check was reading the forge is seen and nothing is written for it. +// +// A turn that names no relationship is not exempt: the CLI lets a claim carry a forge pull request +// instead, and a node under correction would otherwise be carried on the head the correction is +// repairing by the very route that omits the relationship. The acceptance's forge identity is the +// mapping from that pull request back to the relationship (dag_acceptance_forge, written with the +// acceptance), and both routes meet the same gate. +// +// A turn that names neither is not exempt either: --relationship and --pr are both optional on +// merge-turn-request, so the head alone is left, and a turn holding the accepted head of a node under +// correction would slip through the very route that omits both selectors. The active acceptance +// records its own repository and head (dag_acceptances.repository, head_sha), so a turn whose head is +// an accepted head of its repository resolves to that acceptance's relationship. A turn whose head no +// active acceptance of that repository stands on is left to the lane's own rules. +// +// The repository of a turn is matched by SameRepository, the lane's one reading of a repository, against +// every spelling the acceptance has (its repository and its forge repository): a local checkout, its .git +// directory, a symlink to it and the forge slug of the same repository name one repository, and a slug and a +// path never do. The head is matched by the one head definition (commitid.Same, in SQL crw_same_commit). +// +// Acceptance uniqueness is per node and output, not per repository and pull request or per repository +// and head, so more than one accepted node can name the same pull request or the same commit. Every +// matching relationship is asked, not the newest one: a turn is refused when ANY of them has a +// correction open, because the head it holds is then a result being repaired by at least one of them. +// +// The refusal is the existing disposition_conflict and it names the open generation: no new refusal +// name, no column, no schema change. +func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeTurnsRow) (*registry.CoordinationRefusal, error) { + held := strings.TrimSpace(r.CandidateHead) + candidates := []string{} + if r.RelationshipID.Valid && r.RelationshipID.String != "" { + candidates = append(candidates, r.RelationshipID.String) + } + // The turn's own selectors are not the only identities that matter. Acceptance uniqueness is per + // node and output, so two accepted nodes can name the same commit or the same pull request: a turn + // that names one relationship can still be carrying a head another relationship is repairing. The + // identities a claim may carry are resolved here whether or not the turn named a relationship, so a + // named relationship adds to the set rather than replacing it. + // + // The DAG zone arrives with the first write open (D-01), and the lane serves projects that have + // none: a store that predates the table has no accepted result for this turn, which is absence and + // not an error. + present, err := ucZoneTable(ctx, q, "dag_acceptances") + if err != nil { + return nil, err + } + if present { + if r.PRNumber.Valid { + named, err := ucAcceptancesNamed(ctx, q, r.Repository, + "SELECT f.acceptance_id, a.relationship_id, a.repository FROM dag_acceptance_forge f"+ + " JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id"+ + " WHERE a.state = 'active' AND f.pr_number = ?", r.PRNumber.Int64) + if err != nil { + return nil, err + } + candidates = append(candidates, ucRelationshipsOf(named)...) + } + if held != "" { + // The head the turn holds belongs to the acceptance that recorded it. A recorded base refresh + // moves the head the acceptance stands on without changing the accepted head + // (dag_base_refreshes carries the refreshed head), so both are read. + query, args, err := ucHeldQuery(ctx, q, "active", held) + if err != nil { + return nil, err + } + named, err := ucAcceptancesNamed(ctx, q, r.Repository, query, args...) + if err != nil { + return nil, err + } + candidates = append(candidates, ucRelationshipsOf(named)...) + } + } + seen := map[string]bool{} + unique := candidates[:0] + for _, relationship := range candidates { + if relationship == "" || seen[relationship] { + continue + } + seen[relationship] = true + unique = append(unique, relationship) + } + candidates = unique + // A head that some relationship accepted and then moved off is not the head the plan accepts for it + // any more. dag-accept --supersedes replaces the acceptance, so the old head no longer belongs to any + // ACTIVE acceptance and the active-only lookups above cannot see it: a turn that was held for the old + // head would otherwise proceed to merge exactly the result the correction replaced. The acceptance + // that recorded the head is read whatever its state, and a turn holding it is refused. + if held != "" { + if replaced, err := ucReplacedHead(ctx, q, r.Repository, held); err != nil { + return nil, err + } else if replaced != "" { + return coordination(r, contract.RefusalDispositionConflict, + "the head this turn holds is no longer the accepted result of its relationship "+pyvalue.StrRepr(replaced)+ + ": an acceptance of it was replaced by a later one (dag-accept --supersedes), so the result the plan accepts has moved and this turn would merge the result that was replaced. Request the turn for the head the acceptance stands on now", + held, r.HolderTaskID), nil + } + } + for _, relationship := range candidates { + under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) + if err != nil { + return nil, err + } + if !under { + continue + } + return coordination(r, contract.RefusalDispositionConflict, + "the accepted result of this turn's relationship "+pyvalue.StrRepr(relationship)+ + " is under correction: generation "+strconv.FormatInt(live, 10)+" is open over the acceptance, which stands on generation "+ + strconv.FormatInt(stand, 10)+", so the head this turn holds is the result being repaired and is not merged. "+ + "Accept the corrected result with dag-accept --supersedes, or withdraw the generation if it was never bound or sent, and request the turn again", + held, r.HolderTaskID), nil + } + return nil, nil +} + +// ucRelationshipsOf is the relationship of each named acceptance, in the order they were read. +func ucRelationshipsOf(named []ucNamed) []string { + out := make([]string, 0, len(named)) + for _, n := range named { + out = append(out, n.relationship) + } + return out +} + +// ucReplacedHead names a relationship whose acceptance recorded this head and was then replaced, so the +// head is no longer the result the plan accepts for it. "" means no acceptance ever recorded the head, or +// the relationship that recorded it still holds it actively. +// +// It reads the acceptance whatever its state: a superseded or revoked one is exactly the case, because +// the active-only lookups above cannot see it. Only a superseded acceptance counts as a replacement: a +// revoked acceptance is a parent's withdrawal, not a replacement. A head that the relationship still holds +// through its active acceptance, as the accepted head or through a base refresh of it, is not replaced. +func ucReplacedHead(ctx context.Context, q store.Querier, repository, head string) (string, error) { + if head == "" { + return "", nil + } + query, args, err := ucHeldQuery(ctx, q, "superseded", head) + if err != nil { + return "", err + } + named, err := ucAcceptancesNamed(ctx, q, repository, query, args...) + if err != nil { + return "", err + } + relationships := ucRelationshipsOf(named) + sort.Strings(relationships) + stood, err := ucValidStandAcceptances(ctx, q, head) + if err != nil { + return "", err + } + for _, relationship := range relationships { + held, err := ucRelationshipHoldsHead(ctx, q, relationship, head, stood) + if err != nil { + return "", err + } + if !held { + return relationship, nil + } + } + return "", nil +} + +// ucRelationshipHoldsHead reports whether an active acceptance of the relationship stands on head: its accepted +// head, or an acceptance a valid base refresh of the head belongs to (stood). +func ucRelationshipHoldsHead(ctx context.Context, q store.Querier, relationship, head string, stood []string) (bool, error) { + held := "crw_same_commit(a2.head_sha, ?)" + args := []any{relationship, head} + if len(stood) > 0 { + held += " OR a2.acceptance_id IN (" + strings.TrimSuffix(strings.Repeat("?,", len(stood)), ",") + ")" + for _, id := range stood { + args = append(args, id) + } + } + var one int + err := q.QueryRowContext(ctx, "SELECT 1 FROM dag_acceptances a2 WHERE a2.relationship_id = ? AND a2.state = 'active' AND ("+held+") LIMIT 1", args...).Scan(&one) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, err + } + return true, nil +} + +// ucZoneTable reports whether the store holds the named DAG zone table. The zone arrives with the first +// write open (D-01), so a store that predates it holds no acceptance and the lane's turn is left to the +// lane's own rules; that is absence and not an error, the way acceptance.Stands reads dag_base_refreshes. +func ucZoneTable(ctx context.Context, q store.Querier, table string) (bool, error) { + var name string + err := q.QueryRowContext(ctx, "SELECT name FROM sqlite_master WHERE type = 'table' AND name = ?", table).Scan(&name) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, err + } + return name == table, nil +} + +// trainUnderCorrectionRefusal is the bundle member's refusal: the member's relationship has a correction +// generation open over its accepted result, so the head the bundle would carry is the result being +// repaired. It is the existing disposition_conflict and it names the open generation. +func trainUnderCorrectionRefusal(pr int64, relationship string, liveGeneration, standGeneration int64) error { + return trainConflict("pull request %d's relationship %s is under correction: generation %d is open over the accepted result, which stands on generation %d, so the member is not a candidate a bundle may carry or land; accept the corrected result with dag-accept --supersedes, or withdraw the generation if it was never bound or sent", pr, pyvalue.StrRepr(relationship), liveGeneration, standGeneration) +} + +// ucStandingHead is the head an active acceptance stands on now (the newest valid base refresh's, else its own), +// and whether the member's head is one of the heads it has stood on. The chain is the one definition of the same +// result (acceptance.Chain): a member carrying a head of the chain carries the result the plan still accepts for +// its node, and a head outside it was replaced. +func ucStandingHead(ctx context.Context, q store.Querier, relationship string, active acceptance.Active, memberHead string) (string, bool, error) { + chain, err := acceptance.Chain(ctx, q, active.AcceptanceID, relationship, active.Generation, active.HeadSHA) + if err != nil { + return "", false, err + } + for _, head := range chain { + if SameCommit(head, memberHead) { + return chain[0], true, nil + } + } + return chain[0], false, nil +} + +// trainMemberCorrectionRefusal re-reads one member's correction state on the caller's querier and refuses +// the member when the head the bundle carries is not a head the plan still accepts for it (CRW-906). Open +// and verify read each member's acceptance before the transaction that writes, and a generation opened in +// that gap leaves the earlier reading stale; this is the same question asked again inside the write. +// +// The question is asked through the lane's own gate, with the member's own identities, because a member is +// not identified by its relationship alone: acceptance uniqueness is per node and output, so another +// accepted node can name the same pull request or the same commit, and a bundle member whose relationship +// is live can still carry a head another relationship is repairing. The gate resolves every matching +// acceptance and asks each one, which is the same rule the single lane applies. +func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, repository string, pr int64, relationship, memberHead string) error { + // A member whose own relationship still holds an active acceptance is also checked against the heads the plan + // accepts for it: a correction that was opened AND accepted over inside the gap moves the acceptance to the + // corrected head, which makes the live and stand generations equal again, so the generation comparison alone + // would let the obsolete bundle through. A base refresh of the same result is not a replacement: its head is in + // the acceptance's chain. + if active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship); err != nil { + return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + } else if found { + stand, inChain, err := ucStandingHead(ctx, q, relationship, active, memberHead) + if err != nil { + return trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + } + if stand != "" && !inChain { + return trainConflict("pull request %d's relationship %s stands on %s and the bundle carries %s for it, so the member's accepted result moved while the bundle was being read: rebuild and verify the bundle on the current result", + pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand), pyvalue.StrRepr(memberHead)) + } + } + refusal, err := underCorrectionRefusal(ctx, q, store.MergeTurnsRow{ + Repository: repository, + RelationshipID: sql.NullString{String: relationship, Valid: relationship != ""}, + PRNumber: sql.NullInt64{Int64: pr, Valid: pr > 0}, + CandidateHead: memberHead, + }) + if err != nil { + return trainUnreadable("the correction state of pull request %d was not read: %v", pr, err) + } + if refusal == nil { + return nil + } + return trainConflict("%s", refusal.Detail) +} + +// trainExcludedMemberRefusal is CRW-906's guard on a member whose turn left the lane, and it answers the +// question the excluded path must still ask: is the result the bundle carries for this member still the +// result the plan stands on? +// +// The CRW-897 carve-out keeps holding for what it is about: a member whose turn was returned or withdrawn +// is not recorded landed, and neither its pull request moving nor its acceptance being revoked refuses +// the rest of the bundle. This is a different question. A correction of the member's accepted result is +// accepted over by dag-accept --supersedes, which moves the acceptance to the corrected head; the old +// bundle's tree still carries the defective head it was verified on, so landing it would land exactly the +// result the correction repaired. Once the acceptance stands on a head outside its chain, the bundle no +// longer describes what the plan accepts for that member and must be rebuilt and verified. A base refresh +// moves the stand head inside the chain, so the bundle still holds the accepted result and may land. +func trainExcludedMemberRefusal(ctx context.Context, q store.Querier, repository string, pr int64, relationship, memberHead string) error { + active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship) + if err != nil { + return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + } + if !found { + // the carve-out: a revoked acceptance does not refuse the rest of the bundle. Another node's correction over + // the head the bundle still carries for this member is a different question, and it refuses the landing. + return trainMemberCorrectionRefusal(ctx, q, repository, pr, relationship, memberHead) + } + stand, inChain, err := ucStandingHead(ctx, q, relationship, active, memberHead) + if err != nil { + return trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + } + if stand != "" && !inChain { + return trainConflict("pull request %d's relationship %s now stands on %s and the bundle carries %s for it, so this bundle no longer holds the result the plan accepts for that member: rebuild and verify the bundle on the current result before landing it", + pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand), pyvalue.StrRepr(memberHead)) + } + // the same question the surviving members get: the excluded member's code is still in the merge + // commit, so a correction open over the head the bundle carries for it refuses the landing, whoever + // opened it. The carve-out above still lets a revoked acceptance or a moved pull request leave the + // rest of the bundle alone. + return trainMemberCorrectionRefusal(ctx, q, repository, pr, relationship, memberHead) +} diff --git a/internal/relay/mergeturn/undercorrection_identity_test.go b/internal/relay/mergeturn/undercorrection_identity_test.go new file mode 100644 index 0000000000..fca7338fa0 --- /dev/null +++ b/internal/relay/mergeturn/undercorrection_identity_test.go @@ -0,0 +1,185 @@ +package mergeturn + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// CRW-906 generation 2, round 14: a repository is one name however it is spelled. The lane reads a local +// checkout through its git directory, so SameRepository resolves a path the way the lane does, and the +// lane gate and the bundle gates match a turn's repository by it under every spelling an acceptance has. + +// TestSameRepositoryNamesOneCheckoutByItsLocalReading pins the one definition of a repository name. +func TestSameRepositoryNamesOneCheckoutByItsLocalReading(t *testing.T) { + root := t.TempDir() + checkout := filepath.Join(root, "checkout") + riInit(t, checkout) + other := filepath.Join(root, "other") + riInit(t, other) + link := filepath.Join(root, "link") + if err := os.Symlink(checkout, link); err != nil { + t.Fatal(err) + } + same := []string{ + checkout, checkout + "/", checkout + "/.", filepath.Join(checkout, "..", "checkout"), + filepath.Join(checkout, ".git"), link, link + "/", filepath.Join(link, ".git"), + } + for _, spelling := range same { + if !SameRepository(checkout, spelling) || !SameRepository(spelling, checkout) { + t.Fatalf("%q and the checkout %q name different repositories", spelling, checkout) + } + } + for _, spelling := range []string{other, filepath.Join(root, "missing")} { + if SameRepository(checkout, spelling) { + t.Fatalf("%q names the checkout %q", spelling, checkout) + } + } + if !SameRepository(" Owner/Repo ", "owner/repo") { + t.Fatal("two spellings of one forge slug do not name one repository") + } + if SameRepository(checkout, "owner/repo") || SameRepository("owner/repo", checkout) { + t.Fatal("a path and a forge slug name one repository") + } + if SameRepository("", "") || SameRepository(checkout, " ") { + t.Fatal("an empty spelling names a repository") + } +} + +// TestTheCorrectionHoldsEverySpellingOfTheAcceptedRepository: a head under an open correction is held for a turn +// whose repository is any spelling of the accepted repository, and for no other repository. +func TestTheCorrectionHoldsEverySpellingOfTheAcceptedRepository(t *testing.T) { + root := t.TempDir() + checkout := filepath.Join(root, "checkout") + riInit(t, checkout) + other := filepath.Join(root, "other") + riInit(t, other) + link := filepath.Join(root, "link") + if err := os.Symlink(checkout, link); err != nil { + t.Fatal(err) + } + cases := []struct { + name, repository string + held bool + }{ + {"the checkout", checkout, true}, + {"the checkout with a trailing slash", checkout + "/", true}, + {"the checkout through a dot segment", checkout + "/./", true}, + {"the checkout through a parent segment", filepath.Join(checkout, "..", "checkout"), true}, + {"the checkout's git directory", filepath.Join(checkout, ".git"), true}, + {"a symlink to the checkout", link, true}, + {"an unrelated checkout with the same head", other, false}, + {"the forge slug of a path-accepted node", fxRepo, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.exec("UPDATE dag_acceptances SET repository = ? WHERE acceptance_id = 'acc-rel-lane'", checkout) + turn := store.MergeTurnsRow{TurnID: "mtn-spelling", TargetKey: "tgt-x", Repository: c.repository, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if c.held != (refusal != nil) { + t.Fatalf("turn repository %q: held = %v, want %v (refusal %+v)", c.repository, refusal != nil, c.held, refusal) + } + if refusal != nil && refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("the refusal is %q, want disposition_conflict", refusal.Reason) + } + }) + } +} + +// TestTheCorrectionHoldsAForgeSlugSpelledInAnyCase: an acceptance that recorded a checkout and its forge identity +// is held for the forge slug under any case and padding, and the forge match names the same repository. +func TestTheCorrectionHoldsAForgeSlugSpelledInAnyCase(t *testing.T) { + for _, spelling := range []string{fxRepo, strings.ToUpper(fxRepo), " " + fxRepo + "\t"} { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout' WHERE acceptance_id = 'acc-rel-lane'") + w.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES ('acc-rel-lane', ?, 1)", fxRepo) + turn := store.MergeTurnsRow{TurnID: "mtn-forge", TargetKey: "tgt-x", Repository: spelling, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil { + t.Fatalf("the forge slug spelled %q was not held", spelling) + } + } +} + +// TestTrainLandCarriesAnExcludedMemberWhoseStandMovedByABaseRefresh: a member whose turn was withdrawn holds the +// accepted head H; the member then refreshes its base (dag-base-refresh H', same acceptance, no correction). The +// bundle that carries H is still the accepted result, so landing it records the landing. +func TestTrainLandCarriesAnExcludedMemberWhoseStandMovedByABaseRefresh(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + rows, err := w.s.All(w.ctx, "SELECT turn_id FROM merge_train_members WHERE train_id = ? AND seq = 2", train) + if err != nil || len(rows) != 1 { + t.Fatalf("member 2: %v", err) + } + memberTurn := rows[0].Get("turn_id").(string) + if _, err := w.m.Withdraw(w.ctx, memberTurn, "task-m2"); err != nil { + t.Fatalf("withdrawing the member's turn: %v", err) + } + acc, err := w.s.All(w.ctx, "SELECT acceptance_id, execution_generation, head_sha FROM dag_acceptances WHERE relationship_id = 'rel-task-m2' AND state = 'active'") + if err != nil || len(acc) != 1 { + t.Fatalf("the member's active acceptance: %v", err) + } + accID := acc[0].Get("acceptance_id").(string) + gen := acc[0].Get("execution_generation").(int64) + const refreshed = "head-m2-refreshed" + refresh := refreshIDFor(accID, "rel-task-m2", gen, "ev-m2-refresh", "rev-m2-refresh", refreshed, trRepo, trBase, "base-0") + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, ?, 1, 'rel-task-m2', ?, 'ev-m2-refresh', 'rev-m2-refresh', ?, ?, ?, 'base-0', '{}', '[]', 'task-m2', 0, '2023-11-14T22:13:20.000000+00:00')", + refresh, accID, gen, refreshed, trRepo, trBase) + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + answer, err := w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err != nil { + t.Fatalf("land with a member whose stand moved by a base refresh: %v", err) + } + if answer["state"] != "landed" { + t.Fatalf("state after land = %v, want landed", answer["state"]) + } +} + +// CRW-906 generation 2, round 15 (pre-merge evaluation b3056f57 D1): the reader and the identity resolve a spelling +// with a symlink followed by a parent segment to the same git directory. The lane's stat follows the link and then +// the parent segment, so link/../repo is other/repo; filepath.Join alone would have collapsed the segment first. +func TestTheLaneReadsASymlinkedSpellingWhereTheIdentityResolvesIt(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + riInit(t, filepath.Join(root, "r", "repo")) + riInit(t, filepath.Join(root, "r", "other", "repo")) + if err := os.MkdirAll(filepath.Join(root, "r", "other", "deep"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(root, "r", "other", "deep"), filepath.Join(root, "r", "link")); err != nil { + t.Fatal(err) + } + spelled := filepath.Join(root, "r", "link") + "/../repo" + want, err := filepath.EvalSymlinks(filepath.Join(root, "r", "other", "repo", ".git")) + if err != nil { + t.Fatal(err) + } + if got := gitDirOf(spelled); got != want { + t.Fatalf("the lane reads %q through %q, want %q", got, spelled, want) + } + if !SameRepository(spelled, filepath.Join(root, "r", "other", "repo")) { + t.Fatalf("%q does not name %q", spelled, filepath.Join(root, "r", "other", "repo")) + } + if SameRepository(spelled, filepath.Join(root, "r", "repo")) { + t.Fatalf("%q names the checkout it does not reach", spelled) + } +} diff --git a/internal/relay/mergeturn/undercorrection_match.go b/internal/relay/mergeturn/undercorrection_match.go new file mode 100644 index 0000000000..e6ed9cca2e --- /dev/null +++ b/internal/relay/mergeturn/undercorrection_match.go @@ -0,0 +1,98 @@ +package mergeturn + +import ( + "context" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// ucNamed is one acceptance a turn or a bundle member is held on: its relationship, and the acceptance that +// recorded it. +type ucNamed struct { + acceptanceID string + relationship string +} + +// ucAcceptancesNamed reads the acceptances query selects (acceptance_id, relationship_id, repository, one row +// per match) and keeps those whose repository names turnRepository under any spelling the acceptance has: its +// own repository and its forge repository. The repository is compared by SameRepository, the lane's one +// reading of a repository, so a local checkout, its .git directory, a symlink to it and the forge slug of the +// same repository all meet here. The rows are read in full before any further query runs on the same querier. +func ucAcceptancesNamed(ctx context.Context, q store.Querier, turnRepository, query string, args ...any) ([]ucNamed, error) { + rows, err := q.QueryContext(ctx, query, args...) + if err != nil { + return nil, err + } + type candidate struct{ id, relationship, repository string } + var candidates []candidate + for rows.Next() { + var c candidate + if err := rows.Scan(&c.id, &c.relationship, &c.repository); err != nil { + rows.Close() + return nil, err + } + candidates = append(candidates, c) + } + if err := rows.Err(); err != nil { + rows.Close() + return nil, err + } + rows.Close() + var out []ucNamed + for _, c := range candidates { + spellings, err := ucSpellings(ctx, q, c.id, c.repository) + if err != nil { + return nil, err + } + for _, s := range spellings { + if SameRepository(turnRepository, s) { + out = append(out, ucNamed{acceptanceID: c.id, relationship: c.relationship}) + break + } + } + } + return out, nil +} + +// ucSpellings are every repository spelling an acceptance has: the repository it was accepted against and the +// forge repository recorded with it (dag_acceptance_forge), when that table exists. +func ucSpellings(ctx context.Context, q store.Querier, acceptanceID, repository string) ([]string, error) { + out := []string{repository} + present, err := ucZoneTable(ctx, q, "dag_acceptance_forge") + if err != nil || !present { + return out, err + } + rows, err := q.QueryContext(ctx, "SELECT forge_repository FROM dag_acceptance_forge WHERE acceptance_id = ?", acceptanceID) + if err != nil { + return nil, err + } + defer rows.Close() + for rows.Next() { + var forge string + if err := rows.Scan(&forge); err != nil { + return nil, err + } + out = append(out, forge) + } + return out, rows.Err() +} + +// ucHeldQuery selects the acceptances of the given state that hold head: their accepted head, and each head a +// base refresh recorded for them, read as recorded. It +// selects acceptance_id, relationship_id and repository, and the head comparison is crw_same_commit. +func ucHeldQuery(ctx context.Context, q store.Querier, state, head string) (string, []any, error) { + query := "SELECT a.acceptance_id, a.relationship_id, a.repository FROM dag_acceptances a" + + " WHERE a.state = '" + state + "' AND crw_same_commit(a.head_sha, ?)" + args := []any{head} + refreshed, err := ucZoneTable(ctx, q, "dag_base_refreshes") + if err != nil { + return "", nil, err + } + if refreshed { + query += " UNION ALL SELECT a.acceptance_id, a.relationship_id, a.repository FROM dag_base_refreshes f" + + " JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + + " WHERE a.state = '" + state + "' AND crw_same_commit(f.head_sha, ?)" + args = append(args, head) + } + return query, args, nil +} diff --git a/internal/relay/mergeturn/undercorrection_standing.go b/internal/relay/mergeturn/undercorrection_standing.go new file mode 100644 index 0000000000..31fb84d05a --- /dev/null +++ b/internal/relay/mergeturn/undercorrection_standing.go @@ -0,0 +1,39 @@ +package mergeturn + +import ( + "context" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/acceptance" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// ucValidStandAcceptances are the active acceptances that stand on head through a base refresh the relay validly +// recorded. A refresh row keeps the head only when it names the acceptance's relationship and a generation no older +// than the acceptance's, and its id is the digest acceptance.Stands recomputes. A row that fails that check does not +// keep a head the acceptance no longer stands on, so the replaced-head refusal still applies to it. +func ucValidStandAcceptances(ctx context.Context, q store.Querier, head string) ([]string, error) { + present, err := ucZoneTable(ctx, q, "dag_base_refreshes") + if err != nil || !present { + return nil, err + } + rows, err := q.QueryContext(ctx, "SELECT f.refresh_id, f.acceptance_id, a.relationship_id, a.execution_generation, f.relationship_id, f.execution_generation, f.event_id, f.revision_hash, f.head_sha, f.base_repository, f.base_ref, f.base_tip_sha, f.proof_json, f.resolved_paths_json"+ + " FROM dag_base_refreshes f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id"+ + " WHERE a.state = 'active' AND crw_same_commit(f.head_sha, ?)", head) + if err != nil { + return nil, err + } + defer rows.Close() + var out []string + for rows.Next() { + var refreshID, acceptanceID, accRelationship, rowRelationship, event, revision, refreshedHead, baseRepo, baseRef, baseTip, proof, resolved string + var accGeneration, rowGeneration int64 + if err := rows.Scan(&refreshID, &acceptanceID, &accRelationship, &accGeneration, &rowRelationship, &rowGeneration, &event, &revision, &refreshedHead, &baseRepo, &baseRef, &baseTip, &proof, &resolved); err != nil { + return nil, err + } + if rowRelationship == accRelationship && rowGeneration >= accGeneration && + refreshID == acceptance.RefreshDigest(acceptanceID, rowRelationship, rowGeneration, event, revision, refreshedHead, baseRepo, baseRef, baseTip, proof, resolved) { + out = append(out, acceptanceID) + } + } + return out, rows.Err() +} diff --git a/internal/relay/registry/registry.go b/internal/relay/registry/registry.go index 4563e6e928..5df8d5ada7 100644 --- a/internal/relay/registry/registry.go +++ b/internal/relay/registry/registry.go @@ -23,7 +23,9 @@ const ( AnchorPending = "anchor_pending" ) -var reasons = []string{"initial_assignment", "needs_changes_revision"} +// reasons are the values generation-open accepts for --reason: the assignment itself, the correction of a stale result, and the correction of a result that is accepted and still current +// (dagsched's AcceptedResultCorrection, which recordHandOpened admits under the same checks). +var reasons = []string{"initial_assignment", "needs_changes_revision", "accepted_result_correction"} func isLive(status string) bool { return status == "active" || status == "paused" } diff --git a/internal/relay/store/location.go b/internal/relay/store/location.go index 9e9cc753cb..9bff6a731e 100644 --- a/internal/relay/store/location.go +++ b/internal/relay/store/location.go @@ -11,6 +11,7 @@ import ( "syscall" "time" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/commitid" "github.com/thisisjun786/codex-relay-workflow/internal/relay/store/ownership" "modernc.org/sqlite" ) @@ -23,6 +24,9 @@ func boundedDB(path, mode string, timeout time.Duration, pragmas ...string) (*sq // boundedURI is boundedDB with every SQLite URI parameter given (mode, immutable). func boundedURI(path string, params url.Values, timeout time.Duration, pragmas ...string) (*sql.DB, error) { d := &sqlite.Driver{} + if err := commitid.Register(d); err != nil { + return nil, err + } d.RegisterConnectionHook(func(conn sqlite.ExecQuerierContext, _ string) error { for _, pragma := range append([]string{fmt.Sprintf("PRAGMA busy_timeout=%d", timeout.Milliseconds())}, pragmas...) { if _, err := conn.ExecContext(context.Background(), pragma, []driver.NamedValue{}); err != nil { diff --git a/internal/relay/store/relationship_reason_schema_test.go b/internal/relay/store/relationship_reason_schema_test.go new file mode 100644 index 0000000000..2340122538 --- /dev/null +++ b/internal/relay/store/relationship_reason_schema_test.go @@ -0,0 +1,114 @@ +package store + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" +) + +// relationshipReasonCases are the cases the generations[].reason enum of the shipped relationship +// contract is judged on. CRW-906 records a result that was accepted and is still current as a +// correction, and the reason its generation carries is accepted_result_correction: the reason is +// what notes that route, so the record every schema-conforming consumer reads has to carry a value +// the contract allows. The three reasons the registry's generation-open accepts, and the empty +// reason a returning tenure leaves, are valid; anything else is refused. +func relationshipReasonCases(t *testing.T) []schemaCase { + t.Helper() + base := `{"relationshipId":"rel-0123456789abcdef","parent":{"taskId":"01parent-task","hostId":"host-a","cwd":null},` + + `"child":{"taskId":"01child-task","hostId":"host-a","cwd":null},"issueKey":"CRW-906","status":"active",` + + `"createdAt":"2026-01-01T00:00:00Z","executionGeneration":2,"generations":[` + + `{"executionGeneration":1,"dispatchRequestId":"dispatch-1","anchorState":"bound","dispatchTurnId":"turn-dispatch-1","openedAt":"2026-01-01T00:00:00Z","boundAt":"2026-01-01T00:00:00Z","reason":null},` + + `{"executionGeneration":2,"dispatchRequestId":"dispatch-2","anchorState":"bound","dispatchTurnId":"turn-dispatch-2","openedAt":"2026-01-01T00:00:00Z","boundAt":"2026-01-01T00:00:00Z","reason":"initial_assignment"}` + + `],"authorizedScope":{"artifactRoots":["/synthetic/work"],"allowedRecipients":["01parent-task"]}}` + // The reason of the generation the record's current pointer names is the one generation-open wrote. + reason := func(value any) func(map[string]any) { + return func(record map[string]any) { + generations := record["generations"].([]any) + generations[1].(map[string]any)["reason"] = value + } + } + cases := []schemaCase{} + for _, c := range []struct { + label string + value any + valid bool + }{ + {"correction of an accepted current result", "accepted_result_correction", true}, + {"correction of a stale result", "needs_changes_revision", true}, + {"the assignment itself", "initial_assignment", true}, + {"a returning tenure, which leaves the reason empty", nil, true}, + {"a reason the relay never writes", "ruled_again", false}, + {"a reason of the wrong type", 2, false}, + } { + cases = append(cases, schemaCase{Schema: "relationship", Label: c.label, Valid: c.valid, Instance: mutated(t, base, reason(c.value))}) + } + return cases +} + +func TestRelationshipReasonSchemaAcceptsEveryRecordedReason(t *testing.T) { + cases := relationshipReasonCases(t) + var fixture struct { + SchemaSHA string `json:"schemaSha256"` + Verdicts []draft7Verdict `json:"verdicts"` + } + readFixture(t, "relationship-reason-verdicts.json", &fixture) + dir := filepath.Join(repositoryRoot(t), "contract", "schema") + if problems := schemaPinProblems(dir, map[string]string{"relationship": fixture.SchemaSHA}, map[string]int{"relationship": len(cases)}); len(problems) != 0 { + t.Fatal(problems) + } + if len(fixture.Verdicts) != len(cases) { + t.Fatalf("%d recorded judgments, want %d", len(fixture.Verdicts), len(cases)) + } + for i, c := range cases { + verdict := fixture.Verdicts[i] + if verdict.Schema != c.Schema || verdict.Label != c.Label || verdict.Instance != normalInstance(t, c.Instance) || (len(verdict.Errors) == 0) != c.Valid { + t.Fatalf("%s: independent Draft 7 judgment does not match case: %+v", c.Label, verdict) + } + } + // The schema the judgments were made against is the shipped one, and it is the one the other + // verdicts of this schema were judged against: one contract, one pin. + if fixture.SchemaSHA != schemaPins["relationship"] { + t.Fatalf("the reason cases were judged against relationship.json %s, and the schema pins hold %s", fixture.SchemaSHA, schemaPins["relationship"]) + } +} + +// TestRelationshipReasonSchemaRefusesAReasonTheContractDoesNotKnow pins the shape of the guard: the +// case above is only evidence while the schema still refuses a reason outside its enum. +func TestRelationshipReasonSchemaRefusesAReasonTheContractDoesNotKnow(t *testing.T) { + var schema struct { + Properties struct { + Generations struct { + Items struct { + Properties struct { + Reason struct { + Enum []any `json:"enum"` + } `json:"reason"` + } `json:"properties"` + } `json:"items"` + } `json:"generations"` + } `json:"properties"` + } + raw, err := os.ReadFile(filepath.Join(repositoryRoot(t), "contract", "schema", "relationship.json")) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(raw, &schema); err != nil { + t.Fatal(err) + } + allowed := map[any]bool{} + for _, value := range schema.Properties.Generations.Items.Properties.Reason.Enum { + allowed[value] = true + } + if allowed["ruled_again"] { + t.Fatal("the contract now allows a reason the relay never writes; a case that judges one refused proves nothing") + } + for _, reason := range []string{"initial_assignment", "needs_changes_revision", "accepted_result_correction"} { + if !allowed[reason] { + t.Errorf("generation-open accepts %q and the record carries it, but the shipped relationship contract does not allow it: %v", reason, schema.Properties.Generations.Items.Properties.Reason.Enum) + } + } + if !allowed[nil] { + t.Error("a returning tenure leaves generations.reason empty, which the contract spells null") + } +} diff --git a/internal/relay/store/schema_conformance_test.go b/internal/relay/store/schema_conformance_test.go index e48e393610..80590c5069 100644 --- a/internal/relay/store/schema_conformance_test.go +++ b/internal/relay/store/schema_conformance_test.go @@ -368,7 +368,7 @@ func TestSchemaFiles_python_packaged_contract(t *testing.T) { // These digests follow the contract bundle (SOURCE.md). The Draft 7 verdicts keep their own // pins (schemaPins, schema_validator_test.go), which move only when the cases for a changed // schema are judged again, so a contract revision that edits a schema fails both. - expected := map[string]string{"acknowledgement.json": "193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c", "completion-receipt.json": "fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5", "delivery-attempt.json": "ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b", "relationship.json": "c8ebaf4559fa1ac6df26d98c4214938caf78bd8c61659bce026da6a3595a4b90", "verification-verdict.json": "0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9"} + expected := map[string]string{"acknowledgement.json": "193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c", "completion-receipt.json": "fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5", "delivery-attempt.json": "ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b", "relationship.json": "6a994cad6400a07d3e2faaf2161a16818ac7784be651cdcb2d6c9b9439988cbd", "verification-verdict.json": "0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9"} for name, want := range expected { data, err := os.ReadFile(filepath.Join(schema, name)) if err != nil { diff --git a/internal/relay/store/schema_validator_test.go b/internal/relay/store/schema_validator_test.go index a3a4ef3345..fc74ee5d3b 100644 --- a/internal/relay/store/schema_validator_test.go +++ b/internal/relay/store/schema_validator_test.go @@ -37,13 +37,15 @@ var recordSchemas = []string{"relationship", "completion-receipt", "delivery-att // were re-judged unchanged, with new attribution cases in runtime-attempt-verdicts.json.gz. // Completion-receipt gained the optional independentReview item the same way: its 18 original // cases were re-judged unchanged, with the new cases in independent-review-verdicts.json.gz. +// Relationship gained the correction reason CRW-906 records the accepted-current route under: its +// 6 original cases were re-judged unchanged, with the reason cases in relationship-reason-verdicts.json.gz. // Schemas hold only internal "#/definitions" references; a "$ref" to another file would escape // the pin, and that file would need a pin too. var schemaPins = map[string]string{ "acknowledgement": "193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c", "completion-receipt": "fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5", "delivery-attempt": "ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b", - "relationship": "c8ebaf4559fa1ac6df26d98c4214938caf78bd8c61659bce026da6a3595a4b90", + "relationship": "6a994cad6400a07d3e2faaf2161a16818ac7784be651cdcb2d6c9b9439988cbd", "verification-verdict": "0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9", } diff --git a/internal/relay/store/store.go b/internal/relay/store/store.go index f292f10fcd..8f0c949fdc 100644 --- a/internal/relay/store/store.go +++ b/internal/relay/store/store.go @@ -17,6 +17,7 @@ import ( "syscall" "time" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/commitid" "golang.org/x/sys/unix" "modernc.org/sqlite" ) @@ -135,6 +136,9 @@ func open(ctx context.Context, path, socketPath string, options OpenOptions) (_ return nil, err } d := &sqlite.Driver{} + if err := commitid.Register(d); err != nil { + return nil, err + } d.RegisterConnectionHook(func(conn sqlite.ExecQuerierContext, _ string) error { if options.OnConnect != nil { options.OnConnect() diff --git a/internal/relay/store/testdata/fixtures/relationship-reason-verdicts.json.gz b/internal/relay/store/testdata/fixtures/relationship-reason-verdicts.json.gz new file mode 100644 index 0000000000..ef83afbca6 Binary files /dev/null and b/internal/relay/store/testdata/fixtures/relationship-reason-verdicts.json.gz differ diff --git a/plugins/crw/.codex-plugin/plugin.json b/plugins/crw/.codex-plugin/plugin.json index e16d80a6d7..0a8d57a22a 100644 --- a/plugins/crw/.codex-plugin/plugin.json +++ b/plugins/crw/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "crw", - "version": "0.4.0+ac61b934f5c9", + "version": "0.4.0+f2a3e3855df6", "description": "Linear-driven workflow skills for Codex: define an initiative, plan projects and issues, run delivery, and check the result against its canonical criteria.", "author": { "name": "thisisjun786", diff --git a/plugins/crw/skills/crw-run/references/dispatch-verification.md b/plugins/crw/skills/crw-run/references/dispatch-verification.md index 3e8e30d68f..7463b64675 100644 --- a/plugins/crw/skills/crw-run/references/dispatch-verification.md +++ b/plugins/crw/skills/crw-run/references/dispatch-verification.md @@ -196,14 +196,14 @@ a case whose owner has moved or been reworded before relying on it. | S25d | A correction generation, a base-refresh generation and a separated publication step are not the first work-phase of new work, so they do not open `LOOP-DOCS-FIRST-01`'s docs-only cycle, and the CXC rule text is unchanged | [Default independent execution](../../crw-plan/references/integrations.md#default-independent-execution), the Loop bullet of the [Launch packet](task-packet.md#launch-packet) and the workflow bullet of the [Restoration block](task-packet.md#restoration-block) | Default settings | added 2026-10-03; decides the cases S25c left open, from S25c's goalplan record, and no child reading the line was observed, see below | | S26 | A task that has to stop its own tests, a child or the parent, stops them by the pid it recorded when it started them or by the process group it created for them, and never by pattern or name (`pkill -f`, `killall`); a long command records its pid from the start or runs under `timeout`, and a process the task did not start is reported with its pid and working directory, not stopped | The `Processes you start:` line of the [Launch packet](task-packet.md#launch-packet) and its line in [First full assignment required fields](task-packet.md#first-full-assignment-required-fields); for the parent, [Observe and verify](../SKILL.md#observe-and-verify) and [Check CI for this candidate](merge-readiness.md#check-ci-for-this-candidate) | Preserved limits | added 2026-10-03; the mechanics were measured on this host's own processes, no child or parent reading the wording has been observed, see below | | S27 | Devin Review and the Codex review are references and not merge gates: a candidate merges when every CI job succeeds on one and the same head, the coordinator verified it by its usual procedure (it read the diff and the code and reran the tests the criteria rest on) and the local gates pass; the merge waits for no Devin or Codex status or review, neither inheriting an earlier review by patch-id nor a substitute review comment is required, and a later head with no review of its own is normal | [The three gates](merge-readiness.md#the-three-gates) | Merge gate | added 2026-10-03; basis below | -| S27b | The child waits for the one run of each reviewer within the waiting budget before it emits: a Devin status `Analyzing your changes` is waited for up to the budget and `Full review skipped: trial expired and no credits remaining` is not; Codex is waited for while its eyes reaction is on the pull request or any row of its summary comment is not `Completed`, and each of its code review and security review must be `Completed` or skipped by the bot's notice (a thumbs-up counts for both when no row contradicts it), so a Security Review row `Completed` alone is not finished; past the budget a run still going is recorded as pending, not complete, and the wait is released only where the candidate's required independent review and its finding dispositions already exist; when neither reviewer shows any signal 30 minutes after the pull request became ready or was opened, the handoff says `review unavailable (no signal)` and the child goes on, and when only one is silent the same 30 minutes apply to it and the handoff says so; a thread that arrives after the receipt is a late finding: unless the parent has dispositioned it for that head (`--late-dispositions`) it returns the candidate before the acceptance, and after the acceptance no correction can reach the child and the candidate is held ([Late review threads](merge-readiness.md#late-review-threads)) | [The one run of each reviewer, awaited before the receipt](merge-readiness.md#the-one-run-of-each-reviewer-awaited-before-the-receipt) and [Recheck, integrate, and record](merge-readiness.md#recheck-integrate-and-record) and [a late thread the parent dispositions itself](merge-readiness.md#a-late-thread-the-parent-dispositions-itself) | Merge gate | changed with CRW-808; the unlimited wait became the waiting budget with a pending record | +| S27b | The child waits for the one run of each reviewer within the waiting budget before it emits: a Devin status `Analyzing your changes` is waited for up to the budget and `Full review skipped: trial expired and no credits remaining` is not; Codex is waited for while its eyes reaction is on the pull request or any row of its summary comment is not `Completed`, and each of its code review and security review must be `Completed` or skipped by the bot's notice (a thumbs-up counts for both when no row contradicts it), so a Security Review row `Completed` alone is not finished; past the budget a run still going is recorded as pending, not complete, and the wait is released only where the candidate's required independent review and its finding dispositions already exist; when neither reviewer shows any signal 30 minutes after the pull request became ready or was opened, the handoff says `review unavailable (no signal)` and the child goes on, and when only one is silent the same 30 minutes apply to it and the handoff says so; a thread that arrives after the receipt is a late finding: unless the parent has dispositioned it for that head (`--late-dispositions`) it returns the candidate before the acceptance, and after the acceptance the correction goes through the hand-opened generation [Late review threads](merge-readiness.md#late-review-threads) describes, while a node that landed is held ([Late review threads](merge-readiness.md#late-review-threads)) | [The one run of each reviewer, awaited before the receipt](merge-readiness.md#the-one-run-of-each-reviewer-awaited-before-the-receipt) and [Recheck, integrate, and record](merge-readiness.md#recheck-integrate-and-record) and [a late thread the parent dispositions itself](merge-readiness.md#a-late-thread-the-parent-dispositions-itself) | Merge gate | changed with CRW-808; the unlimited wait became the waiting budget with a pending record | | S27c | The Devin status is read by its description and never by its state: `Completed analysis in