Skip to content

feat(web): add ACP permission cards to project chat - #2200

Closed
simple-agent-manager[bot] wants to merge 8 commits into
mainfrom
sam/execute-task-using-skill-e285me
Closed

simple-agent-manager[bot] wants to merge 8 commits into
mainfrom
sam/execute-task-using-skill-e285me

Conversation

@simple-agent-manager

@simple-agent-manager simple-agent-manager Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Project chat now reads durable ACP permission snapshots from Cloudflare and renders permission cards beside their originating tool call, with an unanchored live-tail fallback. Session creators can fetch transient no-store detail and submit an exact option ID through the existing Cloudflare answer route; other viewers see generic waiting state only.

The UI distinguishes pending, answered, delivery-confirmed, delivery-unconfirmed, expired, cancelled, and interrupted states. It also handles refresh/reconnect, deadline expiry, revoked access, two-tab conflict recovery, and lost-receipt retry with the original idempotency key. The orphan callback-based PermissionDialog was removed from packages/acp-client.

Contract reconciliation from current main:

  • Snapshot: GET /api/projects/:projectId/sessions/:sessionId/interactions
  • Creator detail: GET /api/projects/:projectId/sessions/:sessionId/interactions/:interactionId
  • Creator answer: POST /api/projects/:projectId/sessions/:sessionId/interactions/:interactionId/answer
  • Decrypted detail remains Record<string, unknown> on main. The browser accepts only exact AcpInteractionOptionSchema options and never infers malformed choices.
  • optionId is the selected choice authority. The browser sends SHA-256 of that exact ID as answerHash; runtime task 01M3RF53QVR8ZWK446ZSZAFWB6 must preserve that contract.
  • No VM, Worker route, shared schema, auth/token custody, form, URL elicitation, prompt-answer, or direct browser-to-VM path changed.

Validation

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • Additional validation run (if applicable)
  • If this PR changes candidate selection for a sweep/cron/alarm loop (WHERE clause, status set, join, or equivalent), expected candidate volume and worst-case per-candidate cost are stated in the summary or validation notes (see .claude/rules/47-control-loop-io-budget.md)

Scoped validation:

  • pnpm --filter @simple-agent-manager/web test -- project-message-view.test.tsx project-chat.test.tsx useAcpPermissionInteractions.test.tsx acp-permission-card.test.tsx — 148 passed
  • pnpm --filter @simple-agent-manager/web test -- acp-permission-card.test.tsx — 24 passed after the status/retry clearance follow-up
  • pnpm --filter @simple-agent-manager/web exec playwright test tests/playwright/acp-permission-chat-audit.spec.ts --project='iPhone SE (375x667)' — 12 passed
  • pnpm --filter @simple-agent-manager/web typecheck — passed
  • pnpm --filter @simple-agent-manager/web lint — passed with 3 pre-existing warnings
  • pnpm --filter @simple-agent-manager/web build — passed
  • pnpm --filter @simple-agent-manager/acp-client typecheck, lint, and build — passed before the final web-only hardening commit
  • pnpm quality:file-sizes, pnpm quality:type-boundaries, and git diff --check — passed
  • pnpm quality:gitleaks:current could not complete because scanner output was withheld by workspace policy.
  • CI run 36701147517 attempt 2 — passed on review-fix head 88da9a3022b6e351ef887534a428a75682983b16; the retry cleared an unrelated tool-rail pointer-event flake from attempt 1.
  • CI run 36704715011 — passed on final head e13a166b5b14860b99922211927dc1e0941087a4, including Playwright Visual Tests.
  • Candidate-selection item is N/A: this PR adds no sweep, cron, alarm, query candidate selection, or per-candidate loop.

Staging Verification (REQUIRED for all code changes — merge-blocking)

  • Staging deployment green — Deploy Staging workflow triggered manually and passed for this branch
  • Live app verified via Playwright — logged into app.sammy.party (staging) using test credentials and actively tested the application
  • Existing workflows confirmed working — navigated dashboard, projects, and settings; confirmed no regressions in core flows (pages load, data displays, navigation works, no new console errors)
  • New feature/fix verified on staging — the specific changes in this PR work correctly on the live staging environment (describe what was tested below)
  • Infrastructure verification completed — N/A: no infrastructure paths changed
  • Mobile and desktop verification notes added for UI changes

Staging Verification Evidence

Staging is intentionally deferred under Raphaël's scoped authorization for delegated pieces. The coordinator will arrange integrated staging with runtime bridge task 01M3RF53QVR8ZWK446ZSZAFWB6 before shipment. This draft did not deploy, mutate shared staging, or activate flags.

UI Compliance Checklist (Required for UI changes)

  • Mobile-first layout verified
  • Accessibility checks completed
  • Shared UI components used or exception documented
  • Playwright visual audit run locally — mock data scenarios that push the changed UI surface (normal, long text, empty, many items, error, special chars) tested at mobile (375x667) and desktop (1280x800); no horizontal overflow; screenshots in .codex/tmp/playwright-screenshots/
  • Desktop and mobile screenshots for every changed UI surface are posted in a PR comment and linked below
  • Agent/human reviewed the posted screenshots for quality control and found no visual issues, or fixed/documented every issue found

UI Screenshot Evidence

Surface: Project chat ACP permission cards

  • Desktop evidence: Playwright capture in feat(web): add ACP permission cards to project chat #2200 (comment)
  • Mobile evidence: Playwright captures in feat(web): add ACP permission cards to project chat #2200 (comment)
  • Mock/stress data used: owner and noncreator roles; anchored and unanchored requests; 30+ lifecycle rows; long title, description, URL-like text, special characters, long option labels; detail error and revoked-access states; lost receipt; two-tab conflict.
  • Screenshot quality review: the updated 320px, 375px, retry-state, and desktop captures were inspected by the implementation agent using the UI specialist rubric. Option, status-copy, and retry-control clearance; wrapping; overflow; readability; and desktop/mobile responsiveness passed.

End-to-End Verification (Required for multi-component changes)

  • Data flow traced from user input to final outcome with code path citations (see .claude/rules/10-e2e-verification.md)
  • Capability test exercises the complete happy path across system boundaries
  • All spec/doc assumptions about existing behavior verified against code (not just "read the code")
  • If any gap exists between automated test coverage and full E2E, manual verification steps documented below

Data Flow Trace

  1. useAcpPermissionInteractions reads the existing Cloudflare snapshot through listAcpInteractions, refreshes on realtime attention changes, recovers missed events on a bounded configurable cadence, and keeps the fast delivery-state cadence while the server pending bucket is nonempty.
  2. useAcpPermissionPlacement associates toolCallId with production chat rows; requests with no resolvable anchor go to the Virtuoso footer.
  3. AcpPermissionCard gates secure content on exact creator status, pending state, and deadline.
  4. useAcpPermissionCard fetches no-store detail, validates exact options, hashes the clicked exact ID, creates the answer key, and calls answerAcpInteraction.
  5. The existing Worker answer route remains the sole browser authority; refreshed snapshots render the canonical outcome.

Untested Gaps

The production chat capability is covered through mocked HTTP boundaries and real production components. A real ACP runtime fixture and integrated live staging remain for coordinator verification with runtime task 01M3RF53QVR8ZWK446ZSZAFWB6; no runtime bridge exists in this branch by scope.

Post-Mortem (Required for bug fix PRs)

Parent review found missed mounted-chat requests, a pre-hash choice race, and mobile action overlap in the initial feature slice.

What broke

New permissions could remain invisible after an empty snapshot; two deferred-digest clicks could submit different choices; jump-to-latest covered the mobile Allow target.

Root cause

Refresh depended on reconnect/pending state, the choice guard started after async hashing, and permission controls did not reserve the floating control footprint.

Class of bug

Realtime invalidation, async interaction atomicity, and responsive hit-target collision.

Why it was not caught

The original tests lacked empty-to-pending realtime transitions, deferred digest concurrency, and measured mobile hit-target geometry.

Process fix included in this PR

Added real component contract tests, deterministic deferred promises, bounded request-count assertions, and Playwright rectangle plus center-point hit tests at 320px, 375px, and desktop.

Post-mortem file

N/A.

Specialist Review Evidence (Required for agent-authored PRs)

  • All local reviewers completed and findings addressed before merge
  • If any reviewer did NOT complete: N/A — all requested reviewers completed
Reviewer Status Outcome
ui-ux-specialist PASS Reviewed updated 320px, 375px, and desktop captures; measured hit targets and all rubric categories passed
security-auditor PASS Creator-only transient detail, stale-request cleanup, idempotent retries, and Cloudflare-only answer routing passed
test-engineer PASS 148-test focused suite covers realtime refresh, bounded recovery, deferred SHA guards, retries, and authorization revocation
constitution-validator PASS Configurable recovery cadence and retry budget pass Principle XI re-review
task-completion-validator PASS Final A-F audit found no implementation or evidence gaps
env-validator PASS Optional VITE_ACP_PERMISSION_RECOVERY_POLL_MS and VITE_ACP_PERMISSION_QUERY_RETRY_COUNT are consistently typed, documented, and resolved; no secret/deployment mapping applies

CodeRabbit Review Evidence (Required for agent-authored PRs)

  • CodeRabbit requested after local review, staging if applicable, and CI gates passed
  • Waited about 15 minutes, or up to about 45 minutes in total while a review CodeRabbit had started was still in progress
  • Either CodeRabbit reviewed and no CodeRabbit feedback is unresolved, or it did not review and the observed outcome is recorded below

CodeRabbit Notes

Deferred. Per the explicit handoff constraint, this PR must remain draft and must not be marked ready. CodeRabbit is requested only after the other gates and staging are complete and the PR is no longer draft; the coordinator owns that step.

Exceptions (If any)

  • Scope: Integrated staging verification and CodeRabbit request.
  • Rationale: Raphaël explicitly authorized scoped staging deferral for delegated pieces and required a draft-only handoff. Runtime task 01M3RF53QVR8ZWK446ZSZAFWB6 supplies the real ACP fixture.
  • Expiration: Before this PR is marked ready or merged.

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

N/A: this slice reuses internal contracts from merged foundation PRs #2182/#2187 and approved v2 idea 01M3P2E0JJNQRXX020P65ZRKEJ; it adds no external API.

Codebase Impact Analysis

  • apps/web: API client, permission snapshot hook, transient secure-detail/answer state machine, real chat placement/rendering, focused tests, and Playwright visual fixtures.
  • packages/acp-client: removes the unused callback-based PermissionDialog and export.
  • packages/ui: unchanged.
  • apps/api, packages/shared, and packages/vm-agent: unchanged; existing contracts are consumed as-is.

Documentation & Specs

Task contract and acceptance evidence are recorded in tasks/active/2026-09-30-acp-permission-chat-ui.md. No public docs changed because global activation and runtime delivery remain outside this slice.

Constitution & Risk Check

Checked Principles IV and XI plus UI, security, request-budget, and file-size rules. Polling uses a typed/documented environment override and stops in hidden tabs. Sensitive detail stays in transient component state behind creator/pending/deadline gates. Options preserve exact backend IDs/order with no preselection. The browser has no VM response path. Focused extraction keeps production functions below 50 lines and SessionMessageView.tsx below 500 lines.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • coderabbit-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: raphaeltm/simple-agent-manager/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: b6f7399c-4ab8-4d02-8ce9-45b43e4c5466

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Playwright UI evidence

All captures use the production project-chat components with mocked production HTTP boundaries. The fixture stresses long content, special characters, anchored and unanchored requests, 30+ lifecycle rows, creator/noncreator access, detail failure, and revoked access. The implementation agent and UI specialist reviewed every capture for header clearance, wrapping, clipping, horizontal overflow, readability, and responsive behavior; no blocking visual issues remain.

State Desktop 1280×800 Mobile 375×667
Creator with exact options and lifecycle history Creator desktop Creator mobile
Noncreator generic waiting state Noncreator desktop Noncreator mobile
Secure-detail fetch error Detail error desktop Detail error mobile
Secure-detail access revoked Revoked desktop Revoked mobile

Evidence commit: cb59b206

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Parent review of 1abff4b: changes requested before integration.

  1. High — new requests can remain invisible in an already-open chat. useAcpPermissionInteractions.ts:25-27 disables polling when the initial snapshot has no pending rows. Its only refresh effect watches reconnect; neither incoming attention events nor session updates invalidate this query. The project attention reducer only patches session attention. A connected tab that initially returns pending: [] therefore misses a later permission request until focus/reconnect/remount. Add event-driven invalidation plus a bounded recovery strategy as appropriate. Test empty → newly pending without disconnecting, changing focus, or remounting.
  2. Medium — choice acquisition is not atomic. useAcpPermissionCard.ts:218-225 awaits SHA-256 before acquiring submission state. Two clicks before the digest resolves both see no submission and can send different options with distinct answer keys; whichever request reaches Cloudflare first wins. Acquire a synchronous in-memory lock before hashing, preserve one answer key, re-check eligibility after async work, and test with a deliberately deferred digest plus two different option clicks. Also guard duplicate retry clicks.
  3. Medium — floating jump control overlaps the mobile permission action. I personally inspected all eight posted screenshots. In owner-mobile-375x667 the bottom-right jump-to-latest control overlaps the Allow option's right side. Reserve clearance or relocate/suppress the jump control while it would cover a permission action, and verify hit targets at 375px and 320px with Playwright. Do not merely assert document overflow.

Security positives: browser answers use the Cloudflare route; exact option IDs are preserved and validated; secure detail is fetched outside persistent query state; noncreators receive generic cards. These do not resolve the issues above.

CI is green at the reviewed head. This review was source tracing plus inspection of posted Playwright captures, not a fresh test execution. Staging, integrated runtime validation, and final parent re-review remain outstanding. Keep the PR draft and unmerged.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Parent review fixes — ready for draft-only handoff

Fixed all findings from the parent review at e687505bd1a6a8887fb6ef84978043a6fb6eea2f.

  • Realtime attention changes now refresh the mounted permission snapshot. A configurable 30s recovery cadence repairs missed events without a polling storm, while the existing 2s cadence continues through answered delivery state.
  • Option choice locks synchronously before SHA-256 and creates one answer key. Post-hash mount, identity, creator eligibility, state, and deadline checks prevent stale submission; digest failures unlock safely; retries retain the original body and reject concurrent clicks.
  • Snapshot 401/403 now fails closed, removing the card immediately and unmounting its transient secure detail.
  • Permission actions reserve clearance from jump-to-latest on mobile. Playwright verifies every exact option's bounding box and center-point hit ownership at 320px, 375px, and 1280px.

Updated visual evidence

320×667 375×667 1280×800
Owner 320px Owner 375px Owner desktop

Evidence commit: 92ed27cc3

Validation

  • Focused Vitest: 4 files, 148 tests passed.
  • Broader message-view regression set: 7 files, 143 tests passed before the additional review regressions; all added tests are included in the 148-test focused run.
  • Playwright ACP permission audit: 12 tests passed, covering 320px, 375px, and desktop plus post-load authorization revocation.
  • Web typecheck, lint, production build, file-size audit, type-boundary audit, formatter ratchet, and git diff --check: passed. Lint retains three unrelated pre-existing warnings.
  • UI/UX, security, test-engineering, and constitution specialist re-reviews: passed.

The PR remains draft and unmerged. I did not deploy or mutate staging, mark the PR ready, trigger final CodeRabbit review, touch runtime task 01M3RF53QVR8ZWK446ZSZAFWB6, or enable production flags. Parent retains integrated staging and final review.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Coordinator re-review of e687505:

  • Source and behavioral regression review resolves the original missed-new-request and competing-choice findings. Realtime attention reaches the mounted query, bounded recovery covers missed events, answered items keep delivery polling, and choice/retry locks precede async work with identity/deadline checks.
  • Observed snapshot 401/403 now removes cards and unmounts their transient secure detail; reviewed hook and browser regressions.
  • I personally inspected the updated 320px, 375px, and desktop Playwright captures at evidence commit 92ed27c. The original overlap with permission option actions is resolved, and the tests now assert action rectangles and center-point ownership.
  • One adjacent visual correction remains requested from the active UI agent: at 320px the floating jump control still overlaps the Answer saved status sentence. Extend the clearance strategy to status text/retry controls and update the capture.

This pass reviewed source, tests, and posted images; I did not rerun the suites locally. Full Test CI is still in progress. Final head review and integrated staging remain required; keep draft/unmerged.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Final review-fix handoff

  • Exact head: 88da9a3022b6e351ef887534a428a75682983b16
  • CI: run 36701147517, attempt 2 passed. Attempt 1 had one unrelated flaky pointer-event timeout in session-tool-rail-audit.spec.ts; the failed-job retry passed.
  • Focused regressions: 148 tests passed, covering empty → pending, a second request after settlement, bounded recovery/no storm, delivery refresh, synchronous choice/retry locking, deferred digest failure and stale-state guards, and fail-closed 401/403 behavior.
  • Local ACP Playwright audit: 12/12 passed at 320px, 375px, and desktop, including measured non-overlapping 44px hit targets and post-load authorization revocation.
  • Updated screenshots and visual review: feat(web): add ACP permission cards to project chat #2200 (comment)
  • UI/UX, security, test, constitution, environment, and task-completion reviews: PASS.

The PR remains open and draft. No staging deployment or mutation, production flag change, ready-for-review transition, merge, force push, API/VM change, or runtime-task branch change was performed.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Parent re-review visual follow-up

  • Head: e13a166b5b14860b99922211927dc1e0941087a4
  • Reserved the jump-to-latest footprint for permission status, error, and submission rows on mobile.
  • Kept retry compact as Retry answer while preserving the exact selected option in its accessible name.
  • Added deterministic text-range and control-rectangle overlap assertions at 375px and 320px.
  • ACP Playwright audit: 12/12 passed. Focused card tests: 24/24 passed. Web typecheck and lint passed (3 pre-existing warnings).

I inspected all five updated captures. The saved/delivery sentence and retry control remain clear of jump-to-latest at 320px and 375px, without excessive retry-label wrapping; desktop spacing is unchanged.

320px saved status 320px retry state
320px saved status 320px retry state
375px saved status 375px retry state
375px saved status 375px retry state

Desktop permission cards

UI review: visual hierarchy 5/5, interaction clarity 5/5, mobile usability 4/5, accessibility 5/5, system consistency 5/5.

The PR remains draft. No staging deployment/mutation, production flag change, ready transition, merge, API/VM change, force push, or runtime-task branch change was performed.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Final CI is green on e13a166b5b14860b99922211927dc1e0941087a4: run 36704715011 passed, including Playwright Visual Tests. The PR remains open and draft for parent handoff.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Coordinator review of e13a166: remaining mobile clearance finding resolved.

I personally inspected all five updated Playwright captures at evidence commit b153245 (320px/375px saved status and retry state, plus desktop). Status text and retry controls now clear the floating jump control; exact option labels remain intact and desktop spacing is preserved. The source change is scoped to permission content clearance and the compact visible retry label, with the exact option retained in its accessible name. No additional blocking source or visual finding in this pass.

Full final-head Test CI is still running. Integrated VM/Instant permission roundtrip, staging evidence, and final release gates remain required. This is source/screenshot review, not a local rerun or merge approval. Keep draft until those gates are complete.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Final ACP clearance evidence

Exact head: fca210a90e6ee60428929d3b7c4a6c11dafc5666

The implementation at e13a166b5b14860b99922211927dc1e0941087a4 reserves the mobile jump-control footprint for permission status, error, and retry rows, while keeping the visible retry action compact (Retry answer) and retaining the exact option in its accessible name. The follow-up test commit adds center-point hit ownership for status and retry content, covers the detail retry, and captures the status and uncertain/retry state beside the floating control at 320px, 375px, and desktop.

Viewport Saved status Uncertain/retry
320×667 320px saved status 320px retry
375×667 375px saved status 375px retry
1280×800 Desktop saved status Desktop retry

Parent review: I personally inspected all six captures. Status text, uncertain copy, and retry controls remain readable and hit-testable beside jump-to-latest; option actions retain their prior rectangle and center-point checks. No horizontal overflow, clipping, or excessive retry-label wrapping was found.

Validation on final working tree:

  • ACP real-component Playwright audit: 12/12 passed.
  • ACP card unit tests: 24/24 passed.
  • Web lint and typecheck passed; lint retained three unrelated existing warnings.
  • File-size, type-boundary, and diff checks passed.

UI rubric: visual hierarchy 5/5; interaction clarity 5/5; mobile usability 4/5; accessibility 5/5; system consistency 5/5.

The PR remains draft and unmerged. No staging deployment/mutation, ready transition, production flag change, API/VM change, or force push was performed.

@sonarqubecloud

Copy link
Copy Markdown

@simple-agent-manager simple-agent-manager Bot mentioned this pull request Sep 30, 2026
18 of 19 tasks
@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Final exact-head verification is complete for fca210a90e6ee60428929d3b7c4a6c11dafc5666.

  • CI run 36708750560, attempt 2: success
  • Focused ACP Playwright audit: 12/12 passed, including 320px, 375px, and desktop status/retry geometry and hit ownership
  • ACP permission card unit tests: 24/24 passed
  • Web typecheck, lint, file-size checks, and git diff --check: passed (lint retained three existing unrelated warnings)
  • The first monorepo test attempt had one unrelated 5 ms API timeout-stage timing failure; its isolated test passed locally and the failed-job rerun passed in full.

The PR remains draft. No merge, readiness change, deployment, staging mutation, or production flag change was performed.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Superseded by merged #2202 and #2206. The late test-only fca210a clearance assertions were preserved in C1 with the reviewed positioning adjustment, and C1 production deploy36783707657 succeeded. Closing this source PR without merging it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant