Repository navigation
Conversation
Port the preserved b5d103c1/97f0cd4a/d216d39c contribution onto upstream 9.0.0 f360d3a. Python and test hunks match the previously reviewed source exactly; retain historical Flow review attempts at their original ranges and add the Unreleased entry. This contribution does not include or depend on PR gmickel#546. Validation: 4061 tests, 0 failures, 0 errors, 7 skips; 345 focused tests; Ruff, manifest, mirror, anchors and scoped Git checks pass.
Official Copilot completion review returned SHIP on f360d3a..ae80c9c. Native session 5bb1c8da-f36a-45ee-82de-10cb68c32974 records claude-opus-5.5, high, and CLI 1.0.98; the reviewer read the complete code/test diff in chunks and verified R1-R4 plus G1/G2/G4. Historical implementation SHIP remains bound to 627efd0..97f0cd4a; source/test hunks are unchanged by the upstream port. The pre-existing optional triage-judge P3 remains out of scope.
There was a problem hiding this comment.
🟡 Changes recommended
Two new tests assume POSIX prompt delivery and raise StopIteration on Windows.
1 open issue
What changed in this PR
Updates Flow-Next’s Copilot review backend to honor selected reasoning effort and provide shell-less reviewers with pinned diffs.
Changes:
- Forward reasoning effort across model families while preserving fallback-floor behavior.
- Materialize pinned diffs before primary and fan-out review dispatch.
- Update regression tests, documentation, and review records.
| File | Description |
|---|---|
plugins/flow-next/tests/test_review_convergence_cap.py |
Updates effort-attribution assertions. |
plugins/flow-next/tests/test_model_resolution.py |
Tests diff delivery, fan-out identity, and dispatch failures. |
plugins/flow-next/tests/test_backend_spec.py |
Tests reasoning-effort arguments and precedence. |
plugins/flow-next/skills/flow-next-impl-review/workflow-cli.md |
Documents Copilot diff access. |
plugins/flow-next/scripts/flowctl.py |
Implements effort forwarding and pinned-diff delivery. |
plugins/flow-next/scripts/flowctl_tracker/MANIFEST.json |
Refreshes the source checksum. |
plugins/flow-next/docs/flowctl.md |
Documents the diff-delivery contract. |
plugins/flow-next/codex/skills/flow-next-impl-review/workflow-cli.md |
Synchronizes generated workflow guidance. |
plugins/flow-next/codex/docs/flow-next/flowctl.md |
Synchronizes generated runtime documentation. |
CHANGELOG.md |
Records the user-facing correction. |
.flow/tasks/fn-304-honor-copilot-reasoning-effort-for.1.md |
Preserves implementation and validation evidence. |
.flow/tasks/fn-304-honor-copilot-reasoning-effort-for.1.json |
Records completed task state. |
.flow/specs/fn-304-honor-copilot-reasoning-effort-for.md |
Defines effort and diff-transport requirements. |
.flow/specs/fn-304-honor-copilot-reasoning-effort-for.json |
Preserves review history and completion status. |
Review effort: Balanced
Fix the two hosted Windows StopIteration errors by explicitly exercising POSIX argv and Windows stdin transport. Preserve byte-equal pinned diff, range, receipt identity and tool-denial assertions; initialize hooks so the implementation dispatch test also runs independently. No runtime behavior changes. Validation: all 64 model-resolution tests and Ruff pass; native implementer provenance verified.
Address correctness:1 from official review e85b332f: remove the prior pinned diff before every POSIX and Windows subtest so the exists-before-CLI assertion proves fresh materialization independently. Only two setup lines change; all 64 model-resolution tests pass and native implementer provenance is verified.
Merge upstream without rewriting the published branch. Use backend-neutral names for the existing shared guarded diff writer as requested in gmickel#529, and verify that an optional Copilot pass resumes with the primary pinned artifact intact.
|
I published
Validation: focused suites passed (64 model-resolution tests and 26 managed-review tests); the full suite passed before the final comment/task-note-only edits (4,073 tests, 0 failures, 0 errors, 7 skipped). Ruff, Codex mirror, schema/manifest checks, anchor check, shell syntax, and GitHub CI passed on the final PR head: the aggregate, unit tests and functional smokes on all three OSes, Python compatibility, and the Windows interpreter stub. Main CI run · Docs CI run. Independent implementation review used |
|
Thanks for this. The diff delivery is in #589, which fixes several backlog issues for 9.5.1. Copilot primary reviews and fan-out draws now get the reviewed diff by path through one shared helper, as Claude reviews do, and Cursor gets it too, since ask mode blocks shell where there's no OS sandbox. You're credited in the changelog and as co-author. I didn't take the reasoning-effort change. It changes what Copilot sends for Claude-family models, and I want to see that behaviour on its own, with evidence that the flag is accepted, before it ships. If you'd like to pursue it, a small PR with just that change would be easier to judge. Closing this in favour of #589. |
Why
Honor the selected Copilot review effort for Claude and OpenAI models, and give shell-less reviewers the exact pinned patch.
What changes for a user or operator
Claude review selections such as copilot:claude-opus-5.5:high now send --reasoning-effort high. Implementation, plan and completion reviews, including each implementation fan-out draw, receive the full pinned diff at a readable path.
Scope
14 files changed; +555/-97 lines; 3 generated, 2 mechanical.
1. Make High effort and patch coverage real
The adapter previously dropped Claude-family effort. Shell-less review sessions also lacked a readable pinned patch.
2. Forward effort and materialize pinned diffs before dispatch
Send the resolved effort on non-floor Copilot runs. Reuse the existing guarded diff writer before managed or CLI execution, with per-receipt/range/axis identity and failure before dispatch.
plugins/flow-next/scripts/flowctl.py: Copilot argv, truthful effort attribution and readable diff delivery; retain Claude's transport. [R1, R2, R3, R4]1 generated file
3. Verify argv, fallback receipts and diff transport offline
Tests assert the CLI option/value, precedence, floor behavior, byte-equal pinned diffs, fan-out identity and failure before managed/CLI execution. Both affected diff-boundary tests now run POSIX argv and Windows stdin subtests on every host; the implementation dispatch test also initializes its hooks independently.
plugins/flow-next/tests/test_backend_spec.py: Claude Opus High, Haiku Low, OpenAI and explicit-spec precedence. [R1, R3]plugins/flow-next/tests/test_model_resolution.py: Floor effort omission, exact diff delivery and fail-closed transport. [R2, R3, R4]plugins/flow-next/tests/test_review_convergence_cap.py: Record the effort that actually reached the CLI. [R2, R3]4. Document the operator contract and retain review provenance
Explain readable patch delivery under existing tool denials. Keep historical attempts at their original ranges and record the fresh completion verdict.
plugins/flow-next/docs/flowctl.md: Copilot pinned-diff contract. [R4]plugins/flow-next/skills/flow-next-impl-review/workflow-cli.md: Reviewer file access while shell/write tools remain denied. [R4]CHANGELOG.md: Unreleased user-facing correction. [R1, R4].flow/specs/fn-304-honor-copilot-reasoning-effort-for.md: R1-R4 scope and the historical patch-coverage defect. [R1, R2, R3, R4].flow/tasks/fn-304-honor-copilot-reasoning-effort-for.1.md: Preserved implementation, tests and review evidence. [R1, R2, R3, R4]Rest of diff: 2 mechanical files; 2 generated files
Coverage: R1 → groups 1, 2, 3, 4; R2 → groups 1, 2, 3, 4; R3 → groups 1, 2, 3, 4; R4 → groups 1, 2, 3, 4
Blast radius
Copilot review argv, effort attribution, diff transport and their tests/docs. Existing low/medium/high/xhigh values, configuration precedence, session handling and read-only restrictions remain supported. The auto floor omits the effort option; it does not attribute an effort that was not selected.
Verification
Tradeoffs
Ported onto upstream 9.0.0 (f360d3a) without changing runtime source hunks from historical fan-out 5388114c (627efd0..97f0cd4a). The older diff-less SHIP is retained as history and excluded from accepted evidence. A fresh completion SHIP covers f360d3a..ae80c9c. The later test-only commits have scoped SHIP reviews on 02467aa..958ea8b and 958ea8b..88a06d0, including independent diff creation per transport. There is no UI QA surface.
This is independent of #546. Both touch flowctl.py, test_model_resolution.py and runtime docs, but no changed function overlaps. A combined merge preserves both behaviors and merges code/tests/docs automatically; only CHANGELOG and the generated manifest need mechanical reconciliation if both are integrated. Neither contribution includes the other's fix.
The earlier corrected 8.8.0 implementation was used for Opus 5.5 High reviews during the completed XPosed delivery (https://github.com/jcarosi/xposed/pull/2). Current 9.0.0 compatibility is verified here with deterministic tests and a live official completion review; XPosed was not changed or retested.
Open items
The pre-existing optional Copilot triage judge still has its older effort handling (P3); it is outside the review dispatch fixed here. CLI compatibility was exercised with Copilot 1.0.98. Maintainer-owned website follow-up is separate. Global Flow validation has the same 23 legacy heading errors as upstream; this spec validates cleanly. Gordon retains the upstream merge decision.