Repository navigation
feat(server): server-verified approvals on ledger rows - #73
chrisohearn merged 11 commits into
Conversation
ledger_upsert accepts approval: { source: { threadId, itemId, questionId? } }.
The server records it only when the source is a message Chris typed in a T3
client or his answer to a question dialog there, and stores quotes taken from
that item. Dialog answers now record who answered (owner client, MCP sign-in or
agent), and a queued message rewritten by an agent or MCP client is restamped
as theirs. approval: null revokes; ledger_query filters by approved.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
approval is optional on WorkLedgerItem so clients can read an environment on an older build. A guest's message is covered by the refusal tests, the questionId refusal names the dialog's question ids, and the ADR records where the owner-only WebSocket boundary is enforced. The tool-list budget grows by the new approval input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ❌ FailedThis 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Substitute code review (Claude Opus 5.5), done in place of the hosted Codex code review, which is blocked by a usage limit. Head 4ce6759. Verdict: merge after fixes (M1, M2, M3). A separate security review is still running. Focused tests pass: 273 in WorkLedger, core, ThreadLaunchService, runtimeLayer and migrations; 34 files in AgentContextBudget, ForkMigrations and src/mcp; 429 in web settings. Server typecheck is clean. MediumM1. A withdrawn or still-queued message verifies as approval (
M2. A blank dialog answer is recorded as approval ( M3. The agent-edit restamp to
Low
Confirmed correct
Test gaps
🤖 Generated with Claude Code |
|
Substitute security review (Claude Opus 5.5), done in place of the hosted Codex security review, which is blocked by a usage limit. Head 4ce6759. Verdict: merge after fixes: the P1s, and P2-1 and P2-2. I found no remote way for a non-owner to fake owner provenance, and the edit restamp works. But approvals can be laundered. Jarvis acts on an approval with no re-checking, so the P1s must be fixed first. P1P1-1. An approval isn't tied to its row, can be reused, and comes back after a revoke.
P1-2. A message quote has no context, and
P1-3. "Owner client" means any non-MCP WebSocket session with operate scope.
P2
P3
Clean
🤖 Generated with Claude Code |
Review fixes for server-verified approvals. Only Chris's answer to a question dialog can approve, and only when the question, as it read when he answered, names the row as "Ledger key: <key>". Messages are no longer sources, and the queued-edit restamp is reverted, so queue behaviour is unchanged. - Owner answers come only from paired sessions; sessions from `t3 auth session issue` (stored as bot) get no actor. The answering session's short id and label are recorded on the answer and the approval. - Answers snapshot each chosen option's label and description, or the typed text, at answer time. An option is chosen only by its exact value, or by its label when it has no value. Blank, prefilled and unmatched answers are refused. - An approval binds to a hash of the row's key, title, nextAction, owner kind and owner name. Changing any of them, revoking, replacing or rolling back the run withdraws it. Withdrawn sources stay refused for that row, and one answer backs one row. - Dialogs must be completed in a run that was not rolled back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An agent that changes a row's title, next action or owner now gets a warning that Chris's approval was withdrawn, so it asks again instead of leaving Jarvis waiting. Adds the quote case where one option's value is another's label. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… into feat/ledger-verified-approvals
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Substitute security re-review (Claude Opus 5.5) of the dialogs-only design at b5e521c (f545d17 is an empty commit). Verdict: merge after fixes: P0-1, P1-1 and the P1-2 ADR rewrite, ideally with the P2 key and links fixes. A temporary probe test confirmed every "accepted" attack below against the real P0P0-1. Hidden markdown or invisible characters put an action in the quote that Chris never saw.
P1P1-1. Reopening a done row replays the approval.
P1-2. The ADR undersells the host-level risk.
Rule wording: "Act only on an action the quoted question itself spells out, with its object and amount, and that the quoted answer accepts. References such as 'the linked PR', 'the next action', 'as above' or 'this row' approve nothing. If the question and the row disagree, do nothing and ask Chris." P2
P3
Clean
🤖 Generated with Claude Code |
Security re-review fixes for server-verified approvals.
- The quoted question must name the row on a line of its own
("Ledger key: <key>", nothing else on the line) and name no other key. It is
refused if it holds content the dialog does not show: link reference
definitions, link titles, images, raw HTML, character references, or
invisible, format, private-use or control characters. Questions are capped at
600 characters, and the quoted answer gets the same hidden-content check.
- Answers older than 24 hours are refused.
- An approval is used once: marking the row done or cancelled retires it, and a
retired answer can never approve anything again (revocations are recorded by
source, not per row). Done or cancelled rows cannot be approved.
- The row hash also covers owner thread, blocker, links and approvalNote.
- Deleting the thread that holds the answer withdraws the approval.
- ADR 0015 states that full-access agents on the host run as Chris and can
forge approvals, and adopts the review's rule wording.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… into feat/ledger-verified-approvals
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Substitute security re-review, round 3 (Claude Opus 5.5), at 1a89dcb (30d8803 is an empty commit). Verdict: merge after fixing the rest of P0-1. Everything else is fixed: P1-1 single-use, the P1-2 ADR, the P2 hash, one key per question, revocations by source, the 24-hour limit, the exact key match, and the P3 thread deletion. Probes were re-run against the real P0-1, remainder. The regex blocklist (
Fix: stop matching markdown with regexes. Either:
Also cap the question at about 12 lines, and refuse blank-rendering characters (U+2800, U+3164, U+FFA0, U+115F, U+1160, and any space other than U+0020). P2: 🤖 Generated with Claude Code |
Round-3 security review: a regex blocklist missed markdown that renders invisibly (reference definitions in blockquotes or list items, escaped or split labels, titles after parenthesised URLs, backslash and U+2800 padding). - The quoted question and option labels and descriptions refuse [ ] \ < & $ outright instead of matching markdown, and the question is capped at 12 lines as well as 600 characters. - Blank-rendering characters are refused: U+2800, U+3164, U+FFA0, U+115F, U+1160, any space other than U+0020, and line or paragraph separators, along with format, private-use, unassigned, surrogate and control characters. - Chris's typed answers get only the invisible-character check, so "R&D" and "a < b" pass. Prefilled text stays refused. - The key line strips only matched emphasis pairs, so "r3/x_" no longer names "r3/x". Each probe case is a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… into feat/ledger-verified-approvals
6540171
into
integration/mcp-events-20261109
Jarvis acts only on Chris's approval. Until now, T3 agents relayed that approval as text, so Jarvis re-checked thread history on every wake and never acted. With this PR, a ledger row can carry an approval the server has verified: Chris's answer to a question dialog that names the row, quoted as it read when he answered it, and used once.
API
ledger_upserttakesapproval: { source: { threadId, itemId, questionId? } }.approval: nullrevokes the approval.approvalkeeps it only while the row's bound fields are unchanged.approval: { approvedBy: "user", sourceThreadId, sourceItemId, sourceRunId, questionId, quotedQuestion, quotedAnswer, answeringClient: { sessionId, clientLabel }, rowHash, verifiedAt, recordedBy }.approvalNote, which "grants no approval".ledger_queryfilters byapproved.ledger_historyrecords each grant and withdrawal.portfolio_statusshowsitem.approval.approvalis optional onWorkLedgerItem, so clients can still read an environment on an older build.Verification
A dialog source is accepted only when all of the following hold:
questionAnswer.answeredByisowner_clientandansweringClientis present.CommandActorcontext; nothing on the command can claim it.bot.t3 auth session issuesessions, thet3CLI's own sessions and MCP tokens arebotor rejected.agentormcp_client.completed, and its run exists and was not rolled back.questionTextByIdsnapshot), names the row on a line of its own asLedger key: <key>, with the key running to the end of the line and no other key anywhere.r3/foodoes not matchr3/foo-2,r3/foo/bar,r3/foo extra,r3/foo, r3/bar,r3/foo.orr3/foo_, or a key named mid-sentence. Only matched emphasis pairs around the label or key are stripped.[ ] \ < & $outright, instead of parsing markdown, so no link, reference, HTML, entity, escape or math can hide text.quotesByIdsnapshot, never from the live dialog.valueif it has one, otherwise its trimmed label. I used this rather than(value ?? label) === valuebecause the client sends the trimmed label for value-less options, and an option with a value must never be chosen by its label.Withdrawal
An approval is withdrawn when any of the following happens:
rowHash.approval: null.Triggers in the projection transaction handle rollback and deletion.
Each withdrawal writes history and records the source in
work_ledger_revoked_approvals, keyed by source, so that answer can never approve anything again.Trust boundary
These checks stop callers that only have MCP access. They don't stop a full-access agent on the host: it runs as Chris, so it can write the database directly or answer through a
t3 auth pairing createsession. The recorded label is chosen by the client, so only the short session id identifies a session.Chris accepted this limit ("Accept it, document it"). ADR 0015 says so plainly, and device-signed approvals verified off the host are filed as #75.
Review findings
rowHash, unique source, per-row revoked sourcesbotsessions get no actor; session id and label recorded; ADR states the pairing-link limitcompleted, run not rolled back, rollback trigger, pre-build answers refusedForks only reference parent items, and merge-back creates a context transfer, so no path copies an answered dialog under new ids.
Storage
Fork migration 012
WorkLedgerApprovaladds:work_ledger.approval_json;work_ledger_revoked_approvals, keyed by source;The integration tip, after #71, ends at 011.
Tests
WorkLedger.test.ts:r3/task-2,r3/task/sub,r3/task extra,r3/task, r3/other,r3/task.), a key named mid-sentence, two keys, a missing key.$\phantom{}math;questionId.approvedfilters work.approvalNotechange (the probe's "$50,000" note) withdraws the approval with a warning and history, and re-attaching that answer is refused.userInputAnswerQuotes.test.ts: option by label and by exact value; the label of an option that has a value; typed text; blank; prefilled; typing not allowed; a value two options share; a value that is another option's label; multi-select.CommandActor.test.ts: cloud-connect desktop and mobile, the desktop app and browser sessions answer as the owner; abotsession gets no actor whatever subject it claims.core.test.ts: the realt3_pending_requestanswer path stampsagentandmcp_client.runtimeLayer.test.ts: an owner answer persistsansweredBy,answeringClientandquotesById.answeredBy: owner_client,answeringClient: {"sessionId":"31a81e08", ...}andquotesById. The CLI's own sessions arebot.WorkLedgerpassed every check and stopped at the row binding ("does not name this row as "Ledger key: r3/e2e"").Ledger key: r3/e2eline written into the snapshot copy, it was accepted with the right quotes.vp run check:changedon head14cbf56231(merged with the latest integration tip): format, lint, knip and typecheck pass. 105 of 107 test files passed in the full run.OrchestratorMcpService(inside upstream migration 55's setup) and anAcpAdapterV2permission test.agent-context-increaselabel is applied.mcp-tool-usecase covers the ledger tools, and this worktree has no model API keys.UI
Settings → Work ledger shows the quote on an approved row. It is read-only, like the rest of the panel. The screenshots below predate the dialog-only rule; the line itself is unchanged.
Done by Claude Opus 5.5 in Claude Code (T3 Code).
🤖 Generated with Claude Code