Skip to content

feat(web): confirm cleanup of pre-existing files in the controlling tab - #549

Open
testikun wants to merge 1 commit into
openpi-dev:mainfrom
testikun:codex/issue-343-web-confirmation
Open

testikun wants to merge 1 commit into
openpi-dev:mainfrom
testikun:codex/issue-343-web-confirmation

Conversation

@testikun

Copy link
Copy Markdown
Collaborator

Problem

The cleanup guard calls Pi confirmation for deletion of pre-existing workspace files, but the Web runtime binds extensions in print mode without an interactive UI. Its no-op response blocks the command while telling the agent to obtain a confirmation that the browser cannot provide. Refs #343.

Value

The initiating Web tab can inspect the exact protected paths and explicitly approve or deny that one cleanup request. Missing UI, expiry, cancellation and shutdown remain fail-closed and no browser-specific permission policy is persisted.

Approach

  • Preserve the guard as the deletion policy owner and the TUI ctx.ui.confirm path. A scoped same-process registry transports only its print-mode confirmation to the active Web runtime; a future real Pi UIContext remains authoritative. No half UIContext is installed because Pi 0.85.1 would mark every extension as having UI and incorrectly enable custom TUI components.
  • Bind a bounded one-minute pending record to canonical workspace, Session, command, turn epoch and random request id. The Host binds the original prompt admission to a per-tab controller id; only that authenticated controller can read the exact paths and answer. SSE carries only a payload-free invalidation event. A terminal ledger distinguishes approved, denied, expired, stale and already-settled receipts.
  • Abort, turn settlement, Session replacement and Host disposal invalidate pending requests. Missing controller transport gives the model an explicit unavailable block, distinct from a real denial. Browser refresh recovers the same tab identity from session storage, but ordinary chat cannot answer a native request.
  • Ablation removed tool-call id from the projection: the unique request id and bound turn plus exact paths are sufficient. Reused the existing snapshot refresh trigger so streaming SSE does not issue a pending GET per token.

Validation

  • bun run check: passed on Node 24 from checkout base f6b49ae59605b1276b8267f2886d22c03f01533c.
  • bun run test: Node 1663 passed, 1 platform skip; Vitest 220 passed.
  • Guard, confirmation state machine, runtime and Host targeted tests: 94/94 passed. Host tests cover cross-controller denial, stale workspace, exact path privacy and duplicate answers; state-machine tests cover expiration, abort and capacity.
  • Real Chrome suite: 34/35 passed in final run. The unchanged touch-viewport navigation test timed out after page-level Escape; it passed isolated (1/1), and PR feat(web): browse archived and terminal Session history #547 already fixes that focus race. This PR does not duplicate that change. The new confirmation and refresh case passed in both full runs and isolated after the final UI optimization. Earlier full run passed 35/35.
  • Isolated pi list showed one local OpenPI source at this checkout. The browser fixture uses an authenticated real Host and mocked pending records, not a live model/provider deletion. No real workspace files were deleted.

Exact cleanup confirmation

Impact

  • User-visible: exact-path deletion approval/denial in the controlling Web tab; unavailable and denial have distinct model feedback.
  • Model-visible context/tools: no new tool; only the cleanup guard block reason changes when confirmation is unavailable.
  • Runtime/lifecycle: bounded in-memory pending/terminal state, controller-bound admission and fail-closed cleanup; TUI behavior unchanged.
  • Persisted config/data: none; the tab stores only its transient controller id for refresh, not an approval or permission decision.
  • Compatibility/risk: scoped to the existing cleanup guard and Web Host, not a generic Pi select/input/custom bridge or a Trust mutation. The overlapping TUI display PR fix(tui): show cleanup approval wait instead of running Bash #545 is separate; generated Web assets and README may require rebase after other open Web PRs merge. Refs feat(web): expose Pi Trust state and native approval flows #343; does not close the broader issue.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查提交:c9828a489203ccfd270114a5853160a7049b6bc2。

需要修改后再合并。

具体问题

P2 — 新标签页可继承 controllerId,标签页排他性不成立 · web/ui/src/protocol/client.ts:27

Bundled actual WebClient from exact head and ran in ego-browser space 13. A same-origin window.open child inherited sessionStorage and instantiated the identical controllerId (tab-repro.json). Host authorizes pending/read and answers solely by this id, so it cannot enforce README's other-tabs-cannot-answer promise.

P2 — 批准响应丢失后会错误宣称未批准删除 · web/ui/src/app/App.tsx:109

answerConfirmation catches all request errors and renders confirmationStale text 'No deletion was approved'. Server settles approval before HTTP receipt; a response loss or timeout can leave deletion approved despite this message. Repeated requests return already-settled/409, which does not recover the original decision.

验证范围

Full production/test diff reviewed; 94 Node tests and 15 protocol UI tests pass. Actual browser identity-inheritance reproduction succeeded with bundled exact-head client. Manager expiry/abort/scope tests pass.

限制与后续

No live Pi cleanup execution or full browser confirmation flow. Response-loss finding is source-level distributed-outcome analysis, not injected network test. Added E2E test stubs pending/answer/snapshot routes and does not prove native guard binding.

This branch has not been deployed

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

Labels

area:github GitHub workflows, templates, ownership, or tests documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants