Repository navigation
test: migrate ACP approval fixtures to tool-authoritative replies - #769
AlexanderZ-Band merged 8 commits into
Conversation
There was a problem hiding this comment.
Review: one confirmed Logical Bugs issue and three nits (A:2, B:1; C/D/E:0). All four fixed in 2bdefc5. The outside-hunk finding at tests/e2e/baseline/smoke/adapters/test_copilot_acp.py:403 is fixed by removing the obsolete Band-tools-off explanation. The three inline threads have replies and are resolved. No findings declined. Focused post-fix Logical Bugs pass found no new defects. Full local gates passed: Ruff, format, Pyrefly, 28 targeted tests and 7879 nonlive tests (170 reported skips), reruns disabled. One review cycle.
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Second full my-code-review pass: zero new findings across A-F; prior resolved findings remain fixed. Scope matches INT-1757: reachable tool-authoritative replies in hosted Copilot and Cursor approval fixtures, preserving native approval/lifecycle/delivery contracts. Independent logical skeptic found no defects; original approval repro and 28 ACP/MCP regressions pass. Edge-case reviewer passed 78 focused tests with reruns disabled. No changes required; one cycle.
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Plumbing cleanup reviewed across A-F: zero new findings. One Cursor fixture construction path replaces duplicate factory forwarding. Structural matching and SendMessageInput JSON validation normalize only the pinned completion frame directly, without a synthetic MCP envelope. Native/manual permission handling, active room/session, registration, offered one-shot selection, cancellation and inherited logging remain intact. Independent logical check found no defects. Local gates: Ruff/format/Pyrefly, 128 focused tests, full nonlive 7880 passed with 170 reported skips, reruns disabled. New-revision CI and first-attempt Ubuntu/Windows live acceptance are pending.
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Fresh my-code-review pass on this head: zero findings across Style & Hygiene, Design & Reuse, Runtime Safety & Observability, Test Quality, Edge-Case Flow Analysis, and independently verified Logical Bugs. The three prior inline findings remain fixed and resolved. Scope matches INT-1757 and its plan, with the documented correction for the pinned Cursor display-only permission frame. Real ACP/MCP regressions passed 29/29 with no skips or reruns; exact-head required CI and first-attempt Ubuntu/Windows live acceptance are green. No additional changes required.
…d-fixture-INT-1757 Main's CursorACPAdapter now pre-approves registered Band tools during an active turn, which supersedes this branch's test-only CursorApprovalAdapter. Drop it, cursor_approval_adapter and their fixture tests, and let the Cursor approval cells build the product adapter as main does. ALLOW_ONCE_KIND now comes from main's permissions module, and FakeACPAgent keeps the general offered-options check, typed through AllowedOutcome. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N9yodkF7egXLBeXULEb7PM
…ted-fixture-INT-1757
amit-gazal-band
left a comment
There was a problem hiding this comment.
LGTM. After the main merge (#750 pre-approves Band tools in production), the net diff is just the Copilot hosted-fixture cleanup and the scripted ACP peer's selected-option approval fix, plus regressions. ACP + Cursor adapter tests (615 passed) and ruff pass locally on 3575914; CI green. Non-blocking: keep an eye on nightly for the previously documented hosted Copilot band_send_message loop now that inject_band_tools=False is gone; will_ask_permission(options=...) silently ignores allow_option_id.
…ted-fixture-INT-1757
will_ask_permission took both allow_option_id and options, and silently dropped allow_option_id whenever options was given. Keep options as the only argument and build the default allow/reject menu with allow_or_reject(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyJ4C6gG4U83aLNhFzQ3i8
|
Replying to the two non-blocking notes in @amit-gazal-band's approval:
Confirmed and fixed in 9280a9e.
Agreed. It's worth watching, because the main merge brought about 1.4k lines of ACP changes after this PR's recorded live acceptance. I re-ran the hosted cells live against the merged head ( |
Hosted Copilot and Cursor approval fixtures disabled Band MCP tools while requiring room replies. Native ACP text is thought telemetry, so these turns could not deliver the required reply. Keep completion tools injected and preserve the native-command approval assertions.
Cursor approval scenarios use a test-only resolver that selects an offered one-shot permission only for a registered, schema-valid send-message call in the active room/session. Every other permission delegates to the existing manual resolver. The pinned Cursor CLI permission formatter omits rawInput and sends a decorated MCP title plus fenced JSON arguments. Adapt only its exact completion-reply frame before the inherited handler; do not interpret other display content as tool input. Production approval policy and turn judgment remain unchanged.
Evidence and validation:
Final-revision CI passed, including Ubuntu and Windows Python 3.11-3.13, lint/type checks, packaging, conformance/collection, and executable documentation. Python core coverage passed as well.
Implements INT-1757. Focused validation does not certify the full nightly baseline; parent acceptance remains INT-1563.
Review follow-up (
2bdefc5d): fix the scripted ACP peer to record approval from the selected offered option rather than a legacy ID substring. Four real ACP/MCP regressions cover custom one-shot/permanent grants, rejection, and cancellation; three fail against the previous helper and all four pass after the repair. Reuse the shared server constant and ACP content builders, and remove the remaining obsolete hosted-tools explanation. The permission fixture policy is unchanged. Local validation on this commit: 28 targeted tests and 7,879 non-live tests passed (170 reported skips), reruns disabled; Ruff, formatting, and Pyrefly passed. The review-follow-up CI remains RED: Windows Python 3.11 timed out running the existing native PowerShell write probe after 10 seconds. That shell subprocess test and command generation are unchanged by the cleanup; its underlying timeout cause is unresolved. The earlier live acceptance evidence remains tied to its recorded revision.Plumbing cleanup (
0fe714e0): consolidate the Cursor test factory into one construction path and declare the exact pinned frame with structural matching. Parse its reply through SendMessageInput and pass canonical title/direct arguments to the inherited handler, removing the synthetic MCP envelope. Invalid display frames remain manual; an added nested-invocation case guards that boundary. Six-angle review and independent logical check found no new defects. Local gates pass: Ruff/format/Pyrefly, 128 focused tests and 7,880 nonlive tests (170 reported skips), reruns disabled. Fresh first-attempt Ubuntu/Windows acceptance passed on this exact revision: 9/9 required cells on each OS, exact inventory compared with the previous accepted scorecards, no missing/skipped required cells, and both backend jobs plus merged scorecard gate successful. Cleanup CI passed on this exact revision, including Ubuntu/Windows Python 3.11–3.13, all extra lanes, lint/type checks, packaging and executable documentation. Coverage and all required PR checks also passed. The weekly-report job is schedule-only and skipped on this PR run. No previous run was retried; the earlier failures remain recorded.Auto-merge is enabled. GitHub requires one independent approving review; the authenticated author cannot approve this PR, so it remains open pending that approval before merge-queue admission.