feat(human): add confirmation presenter contract - #93
ben-ranford wants to merge 5 commits into
Conversation
ben-ranford
added
area:runtime
Codex Review SummaryThis 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. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical confirmation replay and action-mismatch issues, plus presenter error handling, block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an opt-in, renderer-neutral human confirmation presenter contract with redacted views, registry-backed resolution, cancellation, tests, documentation, and refreshed inventories.
Changes:
- Adds confirmation presenter and resolution flow.
- Adds grant cancellation and lifecycle tests.
- Updates security documentation and generated inventories.
Review findings:
- Critical (3 votes): Cancellation must retain a revocation tombstone to prevent replay.
- Critical (1 vote): Reject confirmation grants whose action ID differs from the invoked action.
- Moderate (1 vote): Preserve presenter errors instead of reporting every failure as cancellation.
- Nit (3 votes): Add the required Unreleased changelog entry.
File summaries
| File | Summary |
|---|---|
scripts/rigor/generated/public-api.txt |
Updated exported API inventory. |
scripts/rigor/generated/dependency-inventory.json |
Updated dependency inventory. |
runtime/human/confirmation.go |
Implements the presenter contract and confirmation flow. |
runtime/human/confirmation_test.go |
Tests confirmation behavior and redaction. |
docs/security.md |
Documents confirmation security and modal focus integration. |
action/action.go |
Adds confirmation cancellation support. |
Review details
Suppressed comments (1)
runtime/human/confirmation.go:74
- An error from
PresentConfirmationis discarded and reported asCONFIRMATION_INVALID: confirmation cancelled, even when the host failed rather than the person canceling. Callers cannot distinguish a renderer/context failure from an explicit cancel to decide whether to retry; handleerrseparately and return an appropriate failure result while still revoking the grant.
if err != nil || decision != ConfirmationConfirmed {
f.Registry.CancelConfirmation(grant)
return confirmationRejected(call, action.ConfirmationInvalid, "confirmation cancelled")
- Files reviewed: 4/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 280f7009f5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c2aebc02
ℹ️ 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".
|
ff6fdb8 to
a15ec4c
Compare
Summary
Closes #63
action.Registry; cancellation revokes the exact issued grant without invoking its action.focus.ModalLifecycle.Validation
go test ./action ./runtime/human -count=1go test -race ./action ./runtime/human -count=1make generated-refreshmake ci(exit 0)Release Notes
Scope firewall: this stacked PR depends on #53 only for the documented modal focus lifecycle. It adds no transport or controller framework and does not change authorization, root dependencies, Go floor, existing wire behavior, or canonical encodings.