Repository navigation
Approvals: one way to ask, worker buttons, a Summary page and one compact chat card - #1311
Conversation
… template, with no scripts or network
…st that fails on drift (P1)
…lumn and a status line names the reader; Summary is a sandboxed page (P4-P7)
…ion why and the Summary page
…new card and chat UI, add a Playwright check for the compact card and status line
…t card keeps D-343's grey outline and Open label (D-286)
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
…le, a relative needs_you summary is found from the worker's folder, the chat line says Updated · ready again, list rows keep their titles, tidy banner and help copy
…g it; name a missing Summary page where the worker meant it
…its questions, a relay of an old worker question waits a full round, a re-asked card drops a stale Tidied banner
# Conflicts: # packages/runpane-py/src/runpane/generated_contract.py
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0dde0db4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const choices: NotifyChoice[] = onLeft | ||
| ? [{ ...recommended, label: capitalized(a) }, { label: capitalized(right) }] | ||
| : [{ label: capitalized(a) }, { ...recommended, label: capitalized(right) }]; | ||
| return { title: `${plain.trim()}?`, choices }; |
There was a problem hiding this comment.
When a paragraph uses a parenthetical recommendation, this return discards everything after the first question mark. For example, Release approval.\nDeploy to production (recommended) or wait? Only deploy after the database backup is verified. becomes a card whose context is only “Deploy to production or wait?”, with the deployment option preselected. The safety condition survives only behind “Show as sent” and is also absent from the generated Summary. Preserve the trailing text in the visible card, or refuse to tidy inputs containing text this pattern cannot account for.
Useful? React with 👍 / 👎.
| const head = /<head(\s[^>]*)?>/iu.exec(html); | ||
| if (head) return `${html.slice(0, head.index + head[0].length)}${meta}${html.slice(head.index + head[0].length)}`; |
There was a problem hiding this comment.
For a worker-supplied Summary beginning with <!-- <head> -->, this regex matches the tag inside the comment and inserts the only CSP meta element inside that comment. The resulting document therefore has no effective Summary policy. Both viewers disable JavaScript, but that does not block image requests; the desktop renderer's inherited policy also explicitly allows HTTPS images. Consequently, opening such a Summary can contact arbitrary remote endpoints despite the promised network isolation. Insert the policy before untrusted markup in a guaranteed valid head, rather than locating it with this regex.
Useful? React with 👍 / 👎.
Agents in Pane now ask through one set of rules. A worker's card gets real buttons, every card gets a subtitle and an HTML Summary, and a Session chat shows one compact approval card at a time. After you send, a status line says which agent is reading your reply.
Visual Overview
PR artifact: companion page (Grain)
Plan: the approvals-system cover sheet r2, approved as D-348 (design reference: mock-ups v3). Companion PR in Agent Farm: greenfield-inc/agent-farm#211.
Labels used below. P1–P8 are the plan's packages and J1–J11 its QA journeys. D-n and Q-n are numbered decisions and questions from the plan's decision log; Q-n also prefixes an approval's title (for example "Q-42: …"). The dock is the strip above a Session chat's message box where approval cards show. Autopilot is Pane's always-on agent above all Sessions; it can read a reply when nobody else can.
Why this change exists
Approvals were hard to answer. Take Q-42, a real approval used as the example throughout this PR: a worker agent sent two questions, their recommendations and two absolute file paths as one 640-character
--question. Pane showed it as one paragraph with no buttons and no subtitle, and its Summary only restated the card. In a Session chat, two full cards stacked above the composer. When the orchestrator used Claude's own question card (AskUserQuestion), the question showed twice in that chat: Claude's card in the conversation and a copy as an approval card in the dock. After you sent a reply, the line said "Reading your reply…" without saying who was reading it, and a "last replied" strip followed you onto unrelated approvals. The rules that tell agents how to ask were copied by hand into five places, and they disagreed.The outcome the plan asks for: one rule for which card to use, cards Pane checks and can tidy, a Summary you can read, one compact card in the chat, and a clear line saying who has your reply. Non-goals: no database migration (new fields live in the approval's existing JSON), no change to who reads a reply (D-258), no model-written Summary, and no redesign of the phone layout.
Concepts and approach
runpane notify,runpane report --state blockedand the MCPnotifytool post. It has a title, a context (now shown as the subtitle), choices or several questions (each with its own choices), and links.needs_youblock holds what it needs from the person (questions, options with(recommended), files and a top-levelwhy). Passed as--summary-file, it becomes the card.The approach puts one gate (
admitCard) where every way of asking meets, builds the Summary in one place (notify:summary-page), and shares the reply-state wording between the desktop and the phone (shared/approvalStatus.ts).Behavior and diff walkthrough
P3 · Worker cards with buttons, and one card check
main/src/services/notify/workerContext.tsnow turns two to fourneeds_youquestions that all have options into sub-questions, each with its own buttons. Each question'swhyshows under it, and the top-levelwhybecomes the subtitle. Before this, they were listed as text. The same applies to aneeds_youblock sent asrunpane notify --ui ask --context. A card that can't take buttons there (anotherui, or an--update) lists the questions in its subtitle instead.cardCheck.tsholds the contract andcardAdmit.tsruns it onrunpane notify(and so the MCP tool, which shells out to the CLI), on--relays, on--update's new title or questions, and onrunpane report --state blocked, before the report is recorded:ERR_NOTIFY_TITLE, with a next stepERR_NOTIFY_RECOMMENDEDWarning: …line each (warningsin--json)Both CLIs (npm and Python) print the warnings, and their result schemas accept the field. Info and secret cards, and Claude's own bridged questions, aren't checked.
P8 · Tidying old paragraph cards
cardTidy.tsruns before the check, only on a card with no buttons whose title breaks the contract. It applies fixed rules, never a model: the first sentence becomes the title;1) … 2) …become questions;Recommended: yes|noafter a yes-or-no question andA or B (recommended)become options;Files:lists and absolute paths become links. It only applies when every question splits into options. Otherwise the card stays as sent, and the check refuses it. The original title and context are kept on the item asasSent, so the card reads "Tidied by Pane" and Show as sent shows them exactly. Q-42's real text becomes this:cardTidy.test.tschecks that no word appears that wasn't sent, apart from the "No" a yes-or-no question offers.P4 · The Summary is one HTML page
A new daemon command,
notify:summary-page <id> [{theme}], returns the asker's own page or Pane's template. The asker's page is an.htmlpassed as a worker's--summary-file, orsummaryinneeds_you. Pane's template (summaryPage.ts) holds the title and state, who asked, the subtitle, each option with its consequence (the recommended one starred), where it stands (step and checks from the worker's status file) and references with real paths. Either way, the HTML gets a Content-Security-Policy that allows no scripts and no network. The desktop shows it in an<iframe sandbox="">, read throughmachines:readso another machine's approval works the same way. The phone shows it in a WebView with JavaScript off and navigation refused. If the asker's page is gone, the template shows instead and says so. The page is read or built each time it's opened; no copy is saved.P5, P6 · One compact card in the chat
SessionApprovalCards.tsxnow shows one card. Its header has the Q-n, who asks, how many questions and the age. The title and subtitle (two lines each) span the card. The bottom row has ‹ n of m ›, dots, "N references · Summary" and Open. The pager follows the column's open item, and ‹ › move the column too when it's open. The card open on the right keeps D-343's grey outline and Open label from #1302. The dock now hides a question the chat already shows as Claude's own card: an approval copied ("bridged") from the same chat panel, marked byreplyIsAnswer,optionsOnlyoragentPrompt. Approvals and the phone keep that copy, and whichever place answers first (the chat or the copy) settles the question; the other closes. It also leaves out approvals whose reply is being read, until they come back ready again.In the column (
ApprovalColumn.tsx), the first tab is now Decide. It shows the subtitle, each question'swhy, the "Tidied by Pane" banner, reference chips and a "Read the Summary" link. Recommended picks start selected. The reply box grows with what you type up to half the column, then scrolls (it was fixed at two rows).P7 · After Send
shared/approvalStatus.ts, which names the reader for every kind of reader. The phone uses the same module.Phone (native)
Rows and the approval screen show the subtitle. Readers are named through
approvalStatus. The groups are Waiting on you, With the agent and Done today, the last counted from local midnight. Each question shows itswhy, and recommended picks are pre-selected. The tidied banner has Show as sent, and a Summary screen callsnotify:summary-page. The Summary button shows only for this machine's own approvals.P1 · One rules text
main/src/services/approvalsRules.tsholds the which-card rule, the card contract and the reply rule. The generated pane-orchestrator skill (all four copies), the bundledpane-orchestrationskill (betweenapprovals-rulesmarkers), each Session's AGENTS.md and thepanehome skill (now version 4) carry it word for word. The bundledorchestrate-sessionsskill gainsreferences/relay.md, covering clarify handling and "a check, not a pipe".approvalsRules.test.tsfails if any copy drifts, or if any of these texts (or the notify guidance and the repo agent-context block incontract.json) tells an agent to use AskUserQuestion. The generated contract files, this repo's AGENTS.md block,docs/PANE_NOTIFY.md,docs/SESSIONS.mdanddocs/CHAT_TABS.mdare updated to match.Assumptions made while building
runpane notifygets its warnings with the answer, because the call returns then.reportand--returnAfterMscalls get them at once.Validation and limitations
Run on
3d0dde0db, which merges origin/main up to #1303 and #1300 into this branch:pnpm check:fast: typecheck, lint, renderer test types, frontend, native and glasses unit tests pass. Markdown links weren't checked locally (no lychee); CI's Markdown Links job passes.skillCacheManager.test.tslauncher tests fail because this Mac has a globalrunpaneon PATH.remoteTerminalInput.test.ts("Unexpected fixture request: HEAD /") failed 1 of 3 runs alone. It is a flake in a file this PR doesn't touch, and CI retries it once.needs_yousent as--contextlost its questions. It now gets buttons, or a written list when the card can't take buttons. Tested innotify.test.ts.waitingat once on every call. This made a tight resume loop for clients that passreturnAfterMs, such as Cursor. Each relay call now waits a full round. Tested innotifyQueue.lifecycle.test.ts.summaryPage.test.tsexpected POSIX paths and failed on Windows CI; it now resolves them per platform.PANE_DIR,HOMEand browser profile):ERR_NOTIFY_TITLEand a next step.whyand option details.runpane notifycall, together with the card check'swarningsfor that card.CI result).Not verified in the real app:
tests/approvals-system.spec.tscovers them in CI's Functional Smoke.Follow-ups from the review, not fixed here:
cardTidy's "A or B (recommended)" split takes as many words before "or" as follow it, so "ship the new version or wait" gives the buttons Version and Wait.Validation level: full, with the real-app gaps listed above. Review was a fresh-agent correctness review; no human review.