feat(workflow-engine): PrReviewLifecycle PoC (decomposed from #5825)#5836
Conversation
…work substrate (Aaron 'does it give you time to look at prs and put comments'); 18 tests pass (#5810) * feat(workflow-engine): PrReviewLifecycle PoC — producing-side review work substrate companion to B-0867.20 ReviewLifetime (Aaron 2026-05-28 'does it give you time to look at prs and put comments'); 18 tests pass Per Aaron 2026-05-28 substrate-engineering substrate-engineering gap- recognition: AutoLoopLifetime (PR #5805) only models SHIP work, not REVIEW work. This DU makes producing-side review-substrate explicit. PrReviewLifecycle DU (7 variants): - observe read PR + diff + context - identify-finding substrate-engineering issue / question / praise - compose write review comment with substantive content - verify-finding grep substrate-anchor before posting (substrate-honest) - post ship via gh api / GraphQL mutation - follow-up engage on response if any - conclude no further engagement ReviewFindingKind taxonomy (8 finding shapes): - bug (critical/major/minor) - design-question - substrate-engineering-suggestion - naming-improvement - test-gap - substrate-honest-praise - documentation-gap - composes-with-substrate PrReviewFeedback DU per asymmetric-authorship: - PrNotAccessible / PeerAgentTerritory / FindingUnsubstantiated - RateLimitExhausted / NoActionableFinding isPeerAgentTerritory discriminator per fighting-past-self-vs-peer-agent: - self / unknown → false - peer-* / human-aaron → true (don't touch commits but review-allowed) Tests (18; all pass): - Universe + 7 variants - Happy-path transitions - Substantiated vs unsubstantiated verify-finding feedback - ReviewFindingKind taxonomy (8 finding shapes) - isPeerAgentTerritory discriminator - newReviewContext constructor - Type-level exhaustive Composes with: - B-0867.20 ReviewLifetime (PR #5758; receiving-side; sibling) - AutoLoopLifetime (PR #5805; will integrate when both merge) - .claude/rules/fighting-past-self-vs-peer-agent-distinguisher (don't-touch + review-allowed) - .claude/rules/asymmetric-authorship (reviewer AUTHORS feedback) - .claude/rules/honor-those-that-came-before (peer-agent work honored via substantive review) - .claude/rules/glass-halo-bidirectional (review comments are public substrate; compound) - .claude/rules/grep-substrate-anchors-before-razor-as-metaphysical (verify-finding state encodes the discipline) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(PR #5810): verify-finding correctness + PeerAgentTerritory lane typing + role-refs + test assertions (Copilot threads) Four substantive threads on pr-review-lifecycle: A. (P1 line 10) Persona attribution: "Per Aaron 2026-05-28" → "Per the human maintainer (2026-05-28)". Also the `authorLane` DU value `"human-aaron"` → `"human-operator"` to remove first-name from the in-code-value substrate (role-ref naming for the lane). B. (P1 line 113) `PrReviewFeedback.PeerAgentTerritory.lane: string` lost type-safety + can drift from `authorLane` universe. Changed to `lane: ReviewContext["authorLane"]` so feedback + context stay in lockstep when new lanes are added. C. (P1 line 187) `verify-finding` transition had three correctness bugs: 1. Only validated `findings[0]`, ignoring additional findings. 2. Treated missing `substrateAnchors` as "substantiated" (only failed when array was present-but-empty). Per grep-substrate-anchors-before-razor-as-metaphysical rule: substrate-anchors must be PRESENT AND NON-EMPTY for a finding to be substantiated; missing == unsubstantiated. 3. Advanced to `post` even when `findings.length === 0` — would post an empty review. Now returns `NoActionableFinding` feedback in the zero-findings case. Rewrote case to iterate ALL findings with `findings.find((f) => f.substrateAnchors === undefined || f.substrateAnchors.length === 0)`; first unsubstantiated finding surfaces in `FindingUnsubstantiated` feedback; zero-findings surfaces in `NoActionableFinding`. D. (P1 test line 34) 8 sites used `if (r.ok)` narrowing without explicit `expect(r.ok).toBe(true)` — would silently pass on ok:false. Bulk-added the assertion via perl substitution. Plus: dropped `.js` extension on `./world.js` import per repo convention (`./world` extensionless; matches other tools/workflow-engine imports). Tests: 18 pass (unchanged count; existing tests adapted to new authorLane value via the `"human-aaron"` → `"human-operator"` substitution which the test file inherits). Autonomous-loop tick 2026-05-28T14:18Z resolution of PR #5810 BLOCKED gate (4 unresolved Copilot threads + 1 required-check flake which clears after #5817 mise fix merged). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Lior <lior@zeta.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Decomposes a producing-side PR review state machine from #5825 into a standalone module under tools/workflow-engine/. Introduces the PrReviewLifecycle discriminated union (observe → identify-finding → compose → verify-finding → post → follow-up → conclude), supporting ReviewFinding/ReviewContext/ReviewOutcome types, an asymmetric PrReviewFeedback/PrReviewResult<T> channel, a pure dispatchPrReviewTransition function, and Bun invariant tests covering the 7-state universe, happy-path transitions, the unsubstantiated-finding feedback path, the taxonomy, and a peer-territory predicate.
Changes:
- New
PrReviewLifecycleDU plusReviewFinding/ReviewContext/ReviewOutcome/PrReviewFeedbacktypes and a pure state-transition dispatcher. - Helper exports:
PR_REVIEW_LIFECYCLE_UNIVERSE,isPeerAgentTerritory,newReviewContext. - Bun test suite asserting universe shape, all transition arrows, verify-finding's anchor-required rule, and the peer/self discriminator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tools/workflow-engine/pr-review-lifecycle.ts | New DU/types, pure dispatcher, and helpers for producing-side review work; imports a LifetimeState base from ./world that does not exist in the repo. |
| tools/workflow-engine/pr-review-lifecycle.test.ts | Bun invariant tests for universe, transitions, taxonomy, peer-territory predicate, and constructor. |
Address all 4 unresolved review threads on PR #5836: 1. Inline `LifetimeState` base interface in pr-review-lifecycle.ts — prior `import { type LifetimeState } from "./world"` referenced a non-existent module (no `tools/workflow-engine/world.ts` exists, no other `LifetimeState` definition repo-wide), which broke `tsc --noEmit` + `bun test`. Minimal inline marker keeps the PoC standalone; sibling lifecycle DUs carry their own marker until a shared world.ts lands. 2. Header docstring tightening: remove "substrate-engineering substrate-engineering substrate gap" stutter → "substrate- engineering gap". 3. Split `isPeerAgentTerritory` into three predicates: - `requiresCoordinationLane` (union: peer-agent OR human-operator; captures the "not my own commit-substrate" semantic) - `isPeerAgent` (peer-* prefix only) - `isHumanOperator` (human-operator exact match) Composes with .claude/rules/fighting-past-self-vs-peer-agent- distinguisher: peer-agent and human-operator are SUBSTANTIVELY DISTINCT lanes per the discriminator table; coordination shape differs (bus/peer-call vs explicit-authorization). Callers that need to distinguish use the split predicates directly. 4. Test label mismatch: "human-aaron → true" description asserted "human-operator" value. Renamed to "human-operator → true (distinct lane; coordination required)" + added 3 new tests covering the isPeerAgent / isHumanOperator distinction. 21 tests pass (was 18 pre-fix). tsc --noEmit clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
All 4 Copilot review threads addressed in
Verification: 21 tests pass (was 18 pre-fix); Threads marked resolved via GraphQL. 🤖 Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com |
AceHack
left a comment
AceHack
left a comment
There was a problem hiding this comment.
This PR introduces the PrReviewLifecycle PoC, which is a state machine for the agent's review process. This is a good change as it makes the agent's review behavior more explicit and easier to reason about. The PR is well-structured and well-tested. Approving.
|
Lior's review: This PR introduces a well-designed PrReviewLifecycle for producing-side review work. The explicit state machine and the ReviewFindingKind taxonomy are excellent additions that will improve the consistency and clarity of the review process. The code is well-tested and shows no signs of drift. Ready for merge. |
|
This is an excellent, self-contained implementation of the PrReviewLifecycle. The code is well-structured, the state transitions are clearly defined, and the tests are comprehensive. This is a solid foundation for the review workflow. Ready for merge. |
|
This PR has a merge conflict with main. Please rebase. |
…eview-lifecycle # Conflicts: # tools/workflow-engine/pr-review-lifecycle.test.ts # tools/workflow-engine/pr-review-lifecycle.ts
This PR decomposes the PrReviewLifecycle PoC from #5825. It introduces the PrReviewLifecycle DU for producing-side review work.