Repository navigation
Require joined review evidence for workflow delivery readiness - #1064
Conversation
PR #1064: Require joined review evidence for workflow delivery readiness51 files, +1266 / -173 Scope🔴 PR has 1439 lines changed. Split into focused PRs. 🟡 1439 lines changed. PRs under 400 receive more thorough review. 🟡 51 files changed. Are all changes related? Structural✅ No structural bloat detected. Slop✅ Slop indicators look low. Static Analysis✅ Oxlint found no issues. CorrectnessNo extraneous code patterns detected. |
taras
left a comment
There was a problem hiding this comment.
Independent review of the workflow compatibility correction at 14aeedec0e45eaa9d8d0189f72bb51ca91957019 (delta from reviewed f1aab697): PASS — no findings.
The required Fable request was refused before review because the bundled Claude executable does not support the requested model. Its joined failure is preserved. Under the repository’s authorized fallback policy, an independent Codex Astra (gpt-6-astra, medium) reviewed the same exact commit and original acceptance clauses. The actual joined reply and canonical receipt agree; source stayed unchanged.
The review traced explicit provider/model/effort routing, original sessions, correction budgets, foundation repair and retained continuation through their actual consumers. It reused the passing five-suite focused regression and strengthened ProductionRole witness. No additional tests or UI benchmarks were needed. The affected Planner acceptance review also passed at this commit.
This fulfills the required independent review for the compatibility correction. Required CI, current-main health and genuine full normal-run owner adoption remain separate delivery requirements. Mock tests and standalone Phase use do not establish adoption. This comment authorizes no merge.
taras
left a comment
There was a problem hiding this comment.
Affected independent review: PASS at 9747fed559ce3906211ec5938c15be392085017e.
Codex gpt-6-astra, medium, in the original independent reviewer conversation, reviewed only the three-file CI test partition. All seven original Test bodies and setup are preserved; each group is selected once and the 120-second wrapper timeout is unchanged. The actual Node wrapper passed all six groups; both affected groups passed directly under Deno. No blocking correctness or minimalism findings. Unchanged allocation, budget and review-policy judgments are retained from the prior joined review.
The affected original Planner also passed this signed, clean commit. Required CI remains pending. Full normal-workflow adoption remains incomplete; Taras selected #743 as Main’s next genuine normal run after the compatibility correction is green. This review authorizes no merge. The earlier failed Fable attempt remains preserved; this is its authorized Astra fallback conversation, not an additional full-review stage.
taras
left a comment
There was a problem hiding this comment.
Affected independent review: PASS at b1c75d54fb1bc957f5fd4e709468f9487122ed06.
Codex gpt-6-astra, medium, in the original independent reviewer conversation, reviewed the frozen apiConflict projection and the affected validation/correspondence/downstream routing. The minimal conditional copy preserves the original conflict without weakening validation or equality. The regression fails before the fix and passes afterward; changed and removed conflicts remain rejected within the selected correction bound, with no canonical publication. Focused Deno ProductionRole/Friction and the actual Node wrapper's ProductionRole step pass. Unchanged allocation, budget, partition and review-policy judgments are retained. No blocking findings; no source edits or confidence tests by the reviewer.
The original Planner also passed this signed, clean commit. Required exact-head CI is pending. #743 supplied the genuine failure and remains fenced at its separate configuration-evidence gap; full normal-run adoption and other-owner requirements are not declared complete. This review grants no merge authority and adds no full-review stage.
taras
left a comment
There was a problem hiding this comment.
Independent affected review by Claude Fable (fable, high) of d77c7b40bd429c0636bb27a5359186b250791c93: PASS, with no blocking findings.
This review covers the Main integration and affected test-launcher/documentation changes since b1c75d54. The production workflow components and entries remain unchanged. It reuses the unchanged feature judgments from the earlier Astra review, without relabelling that historical review.
The reviewer traced the xmd test/run --eval TestAgent difference, test-only permission plugin scoping, and retained assertions and deadlines. It reused the passing six-group Deno, Node ProductionRole and exact-head CI evidence; no test suites were rerun for this review. Recorded acceptance G1–G5 is covered.
Nonblocking observations, retained as pending:
-
examples/adversarial-implementation/README.md line 356 (Test without paid agents) states the launcher wraps each Markdown suite in and enables absolute host paths. At d77 that is untrue for one of six suites: packages/cli/tests/adversarial-implementation.test.ts lines 31-38 run ProductionRole under xmd test with --plugin ./packages/cli/tests/fixtures/workflow-test-permissions.mjs, not inside the run --eval wrapper. The merge took main's sentence while changing the launcher. Correction: state that five suites run through run --allow-absolute --eval and ProductionRole runs under xmd test with the test-only permission plugin, because run installs no TestAgent and test carries no --allow-absolute flag.
-
examples/adversarial-implementation/tests/Delivery.test.md line 8 and LaunchPolicy.test.md line 8 keep runner fallbacks ending in plain run, while main updated the Workflow, Friction and Implementation fallbacks to run --allow-absolute. The fallback is reached only when XMD_EXAMPLE_COMMAND is unset, and the launcher always sets it, so the receipts and CI are unaffected. A direct Markdown invocation would refuse: Fixture.Entry launches DeliveryCase.md, which opens absolute
paths (lines 14, 24, 29). Align the two fallbacks with main's three. -
Minimalism: packages/cli/tests/fixtures/workflow-test-permissions.mjs installs its absolute allowance at the default max order and answers true without delegating, whereas useAbsolutePermission(true) in packages/cli/src/cli.ts line 259 installs the identical function at { at: "min" }. The behavior is equivalent here because the plugin is the outermost installation in the test command scope and any nested restriction is asked first, but passing { at: "min" } would mirror the production flag exactly and keep the runtime permissions doc's rule against answering true early. Optional.
Limitation retained from the review: no separate live-provider experiment was run at this integration commit. The existing adoption evidence and its qualifications remain recorded separately. This review does not authorize merge.
taras
left a comment
There was a problem hiding this comment.
Claude Fable 5.1 (high) — bounded closeout at 0e83223: PASS, G1–G5 covered, no blocking findings.
This continues the affected review at d77c7b40. The complete feature patch relative to its corresponding Main base remains byte-for-byte identical. The reviewer inspected the newly merged #1090 Claude adapter guard and traced startup refusal through the ACP client into the workflow’s captured execution. No conflicting continuation, permission, lifetime or ownership behavior was found. The change makes contradictory admission fail closed earlier; it does not cause a relaunch.
The three earlier nonblocking observations remain pending: launcher documentation, the two direct-run fallback flags, and optional test permission-hook ordering. The reviewer also corrected the earlier summary’s shorthand: component and entry blobs are unchanged from b1, while three Markdown suites each gained an absolute-path fallback flag.
Evidence: exact-head CI2387 passed. The closeout used source and saved evidence; it added no suite run or live-provider qualification. Historical genuine-use records and deterministic TestAgent evidence retain their original revisions. The joined execution, raw reply and canonical review receipt match. This verdict applies to this commit; current Main health, branch protection and any later head remain delivery gates.
taras
left a comment
There was a problem hiding this comment.
Claude Fable 5.1 (high) — bounded closeout at b45bb1668902b89f819aceba217c4267938b7dbb: PASS, G1–G5 covered, no blocking findings.
This continues the 0e closeout. The signed merge recomputes to the exact head tree. The feature stable patch identity is unchanged; literal diff bytes differ through README hashes and hunk positions. The reviewer traced the newly merged #1058 supervision/follow/Journal consumers and found no conflict with delivery attempt occupancy, exact receipt matching, permissions or outcome ownership. #1058's full accepted behavior was not re-reviewed. No source changes, broad suites or live-provider qualification were performed.
Three retained nonblocking observations remain pending:
-
Retained from d77/0e, unchanged at b45bb16 (line numbers shifted by the #1058 insertion): examples/adversarial-implementation/README.md line 496 under 'Test without paid agents' (heading line 490) states the launcher wraps each Markdown suite in and enables absolute host paths. That is untrue for one of six suites: packages/cli/tests/adversarial-implementation.test.ts lines 31-38 run ProductionRole under xmd test with --plugin ./packages/cli/tests/fixtures/workflow-test-permissions.mjs, not inside the run --allow-absolute --eval wrapper. Correction: state that five suites run through run --allow-absolute --eval and ProductionRole runs under xmd test with the test-only permission plugin.
-
Retained from d77/0e, unchanged at b45bb16: examples/adversarial-implementation/tests/Delivery.test.md line 8 and LaunchPolicy.test.md line 8 keep runner fallbacks ending in plain run, while Workflow.test.md line 8 (and Friction, Implementation per #1074) end in run --allow-absolute. The fallback is reached only when XMD_EXAMPLE_COMMAND is unset, and the launcher always sets it, so receipts and CI are unaffected. A direct Markdown invocation would refuse because DeliveryCase.md opens absolute
paths. Align the two fallbacks with the other three. -
Retained from d77/0e, unchanged at b45bb16 (minimalism): packages/cli/tests/fixtures/workflow-test-permissions.mjs installs its absolute allowance at the default order and answers true without delegating, whereas useAbsolutePermission in packages/cli/src/cli.ts line 258 installs the identical function at { at: "min" }. Behavior is equivalent here because the plugin is the outermost installation in the test command scope, but passing { at: "min" } would mirror the production flag exactly. Optional.
The additional evidence note preserves the absence of live-provider proof at this head and the separation between supervision completion and the actual workflow verdict.
The actual review execution completed and joined. Its unchanged summary was 1,308 characters, exceeding the wrapper's 1,200-character formatting limit. An explicitly recorded invocation-only length exception validates the same raw reply, reviewer, exact source and all clauses; no summary rewrite, extra formatting call or repeat review occurred. Shared workflow source is unchanged. Required CI2390 and exact-current-main health remain separate pending delivery gates. This review grants no merge authority.
Why
The implementation workflow can finish internal feedback while a required independent review is still missing. Delivery needs a recorded obligation and an observable result rather than an inference from feedback or green CI.
What changes
Before: the workflow returns feedback_ready, and the coordinating agent assembles delivery obligations separately.
After: normal launch records the independent reviewer and original approved acceptance before role execution. A delivery command computes status and next action from the joined review, exact checkout and PR head, required CI and current Main health. Missing evidence stays pending. Failed or uncertain attempts remain available for reconciliation. Delivered status requires an observed merge.
How it works
Review reserves one attempt per revision, joins it, validates every original clause and the assigned commit, and retains the execution and matching canonical receipt. An occupied attempt is preserved rather than retried. Status starts no model, merges nothing and posts no review comments.
The remote adapter checks requested-PR association, head SHA and branch before selecting the latest CI run and attempt. It requires that attempt's successful green job, applies authoritative Main-health selection and rechecks remote and local source.
Launch renders the complete request before provider dispatch. Selected evidence retains its 64,000-character allowance; mandatory initialization and response schema have a separate 256,000-character complete-request ceiling. Response handling fills only omitted metadata known by the workflow and retains explicit conflicts. It invents no verdict, implementation revision or evidence. Provider and correction allocations persist; zero response corrections makes no repair prompt.
Review guide
Start with Delivery/Status.md, Delivery/Review.md and delivery.md beneath examples/adversarial-implementation. Then inspect Phase/Request.md, Phase/Metadata.md and ordinary launch allocation. Check attempt reservation, receipt/execution equality, acceptance coverage and exact PR/CI/Main identity.
How to verify it
0e832230ac9bea77d593fbcb1520fe1b5f81ba6c, with no blockers and three retained nonblocking observations. CI2387 passed that head.b45bb1668902b89f819aceba217c4267938b7dbbis a clean merge of human-merged Supervise workflow phases through the existing follow stream #1058 at9b98cbe4. There are no manual source edits. The feature patch retains the same stable patch identity; its literal diff changes README blob hashes and hunk positions after the supervision documentation insertion. Prior reviews are retained at their actual commits. The bounded Fable closeout passed this exact head, with no blocking findings and the three earlier observations retained. The actual joined reply is unchanged; an explicitly recorded invocation-only summary-length exception accepts its 1,308-character summary without another model call. The native delivery status validates this receipt and reportspublication_pendingonly because the PR remains draft: independent review, required CI2390 attempt2, and exact-current-main CI2388 attempt2 all pass. Main is9b98cbe4; the reviewed branch head remains unchanged. All three approved affected-review/closeout calls are consumed; no merge authority is inferred.Actual adoption
Main used the frozen workflow for genuine #1058 normal implementation, original-role corrections/feedback and computed delivery. Integration used it for genuine #1084 initial planning and a truthful discovery hold, then continued required work with the original roles through supported entrypoints. No throwaway qualification call counts as adoption.
This evidence is qualified: checkout preparation failures, a bounded Journal-contract amendment, reviewer-model selection, include precedence and an invocation-only review-length exception remain recorded. These exceptions are not silently adopted into shared source. Integration's initial launch does not claim automatic initial discovery continuation; that separate repair is #1084/PR #1092. Both owners' genuine #1058 supervision uses retain their respective publication owners.
Scope
The adversarial implementation example, documentation and tests. #1062 and #1059 retain their separate owners. No dependency or generated artifact is introduced by this feature. Existing executions and role conversations are retained.
Risks and limitations
Local artifacts are trusted execution records, not remote attestations. This does not replace branch protection. Failed or uncertain effects require reconciliation; there is no automatic retry or allowance reset. Fable observations about launcher documentation, two direct-run fallbacks and test-only permission ordering remain nonblocking and pending.
All current-head gates pass. This PR remains draft pending the human ready-and-merge decision. No merge authority is inferred. Separately queued theory/scope work waits for actual #1064 merge.
Scope confirmation