feat(mcp): in-chat approvals for Cedar denials - #436
Conversation
When a temper.* call inside execute returns a structured authorization_denied with a decision id, and the MCP client declared the elicitation capability at initialize, the stdio server now sends an elicitation/create request so the human at the client resolves the pending decision inline (approve narrow/broad, deny, or leave pending). Approvals and denials are resolved against the Temper server with the MCP's configured operator credential; the tool result is annotated so the model can retry the action itself. Decline, cancel, timeout, or a malformed answer leaves the decision pending and the result unchanged. The stdio loop is restructured into reader/writer tasks with a correlated pending-request map so the server can send JSON-RPC requests to the client mid tools/call; the sequential dispatch queue guarantees at most one elicitation in flight per session. The initialize handler now negotiates the protocol revision (2025-06-18 supported) instead of always answering 2024-11-05. Disable with TEMPER_MCP_ELICIT_APPROVALS=0; elicitation timeout defaults to 120s (TEMPER_MCP_ELICIT_TIMEOUT_SECS). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Runs the full MCP loop against a scripted fake client and a mock Temper backend that answers every action with a structured Cedar denial: approve (elicitation emitted, approve endpoint hit with the operator bearer and a narrow PolicyScopeMatrix, result annotated for retry), human deny (deny endpoint hit), decline (decision left pending, no resolution call), and a client without the elicitation capability (denial passes through untouched, no elicitation sent). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the client stream ends while an elicitation is awaiting its answer, the reader now clears the pending server-to-client request map so the requester returns Closed at once — the decision is left pending and the session finalizes promptly instead of waiting out the 120s timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sis.rs (readability ratchet) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
QA (Rei). Do not merge. Draft stays draft. P1 — wrong decision approved. Fix is a structured P2 — version negotiation. ADR-0173 SD3 says elicitation only from 2025-06-18. P2 — capability gate. P2 — OTS. P3. Keep draft until P1 is gone and there is a live Claude Code elicitation test. I will not merge this. |
…P_APPROVER_KEY) On the credential-bound edge, the agent's scoped credential makes the denied call; a distinct operator credential must post the approval or ARN-389's self-approval guard rejects it. Read TEMPER_MCP_APPROVER_KEY and use it only for resolve; fall back to api_key when unset. Proven live: agent claude-code denied -> human accepted inline -> resolved as operator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
QA (Rei) on ARN-389 key split is the right shape ( P1 parser and the two gates did not move:
New: zero tests for the split. Keep draft until parser + |
rita-aga
changed the title
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if: >- | ||
| github.event.issue.pull_request != null && | ||
| (startsWith(github.event.comment.body, 'DECISION:') || startsWith(github.event.comment.body, 'RESOLVE:')) |
There was a problem hiding this comment.
Any PR commenter can submit DECISION: or RESOLVE: because this guard checks only the comment prefix, not the commenter’s repository role. A DECISION: is then persisted as an “Owner ruling,” while the review workflow accepts every commenter’s RESOLVE: lines and marks matching findings resolved before validation. A fork contributor can therefore forge governance decisions or clear blocking findings and cause the protected review check to pass.
How this was verified: The comment author is never checked against repository permissions before the trusted workflow edits the PR or marks review findings resolved.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/sdlc-decision-intake.yml
Line: 26-28
Comment:
**Commenters Can Forge Decisions**
Any PR commenter can submit `DECISION:` or `RESOLVE:` because this guard checks only the comment prefix, not the commenter’s repository role. A `DECISION:` is then persisted as an “Owner ruling,” while the review workflow accepts every commenter’s `RESOLVE:` lines and marks matching findings resolved before validation. A fork contributor can therefore forge governance decisions or clear blocking findings and cause the protected review check to pass.
**How this was verified:** The comment author is never checked against repository permissions before the trusted workflow edits the PR or marks review findings resolved.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| body=subprocess.run(["gh","api",f"repos/{os.environ.get('GITHUB_REPOSITORY','')}/issues/{n}/comments", | ||
| "--paginate","-q",".[].body"],capture_output=True,text=True).stdout | ||
| import base64 | ||
| rec=None | ||
| def _consider(txt): | ||
| global rec | ||
| try: | ||
| cand=json.loads(base64.b64decode(txt).decode()) | ||
| except Exception: | ||
| return | ||
| if cand.get("commit")==head: rec=cand | ||
| for m in re.finditer(r"<!--\s*sdlc-review-record-b64\s*([A-Za-z0-9+/=\s]+?)\s*-->", body): | ||
| _consider(m.group(1).strip()) |
There was a problem hiding this comment.
The review and proof gates trust unsigned base64 records from every PR comment, checking only the self-declared commit before passing the data to validators. A contributor can post a fabricated current-head record with passing fields and drive the trusted review or proof check on their commit without evidence produced by the authorized review or verification process. Require an authenticated record source, signer, or trusted comment author rather than treating arbitrary comment bodies as gate evidence.
How this was verified: Both privileged workflows discard comment authors, decode matching records from all comments, and use the decoded record to determine the check conclusion on the contributor SHA.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/sdlc-review.yml
Line: 80-92
Comment:
**Unsigned Records Pass Gates**
The review and proof gates trust unsigned base64 records from every PR comment, checking only the self-declared commit before passing the data to validators. A contributor can post a fabricated current-head record with passing fields and drive the trusted `review` or `proof` check on their commit without evidence produced by the authorized review or verification process. Require an authenticated record source, signer, or trusted comment author rather than treating arbitrary comment bodies as gate evidence.
**How this was verified:** Both privileged workflows discard comment authors, decode matching records from all comments, and use the decoded record to determine the check conclusion on the contributor SHA.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| \x20 await temper.create(tenant, entity_type, fields) -> create entity\n\ | ||
| \x20 await temper.action(tenant, entity_type, entity_id, action_name, body) -> invoke action\n\ | ||
| \x20 await temper.patch(tenant, entity_type, entity_id, fields) -> update fields\n\ | ||
| \x20 await temper.put_file_text(tenant, file_id, content, content_type) -> PUT File $value; UTF-8 text up to 1 MiB, application/json | text/plain | text/markdown\n\ |
There was a problem hiding this comment.
The execute-tool documentation advertises temper.put_file_text(tenant, file_id, content, content_type), but dispatch forwards all arguments to an implementation that requires exactly three arguments and interprets the first as file_id. Agents following the advertised API will pass four arguments and receive an argument-count error, preventing the new evidence-upload operation from working. Align the exposed signature and implementation, including how the tenant is selected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-mcp/src/protocol.rs
Line: 235
Comment:
**Upload Signature Does Not Match**
The execute-tool documentation advertises `temper.put_file_text(tenant, file_id, content, content_type)`, but dispatch forwards all arguments to an implementation that requires exactly three arguments and interprets the first as `file_id`. Agents following the advertised API will pass four arguments and receive an argument-count error, preventing the new evidence-upload operation from working. Align the exposed signature and implementation, including how the tenant is selected.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| pub(crate) async fn run_loop<R, W>(mut ctx: RuntimeContext, reader: R, writer: W) -> Result<()> | ||
| where | ||
| F: Fn(Vec<&str>) -> TemperCallMetadata, | ||
| R: AsyncBufRead + Unpin + Send + 'static, | ||
| W: AsyncWrite + Unpin + Send + 'static, | ||
| { | ||
| let mut out = Vec::new(); | ||
| let mut cursor = 0usize; | ||
|
|
||
| while let Some(found) = code[cursor..].find(needle) { | ||
| let method_start = cursor + found + needle.len(); | ||
| let mut open = method_start; | ||
| while open < code.len() | ||
| && code | ||
| .as_bytes() | ||
| .get(open) | ||
| .is_some_and(|b| b.is_ascii_whitespace()) | ||
| { | ||
| open += 1; | ||
| } | ||
| if code.as_bytes().get(open) != Some(&b'(') { | ||
| cursor = method_start; | ||
| continue; | ||
| } | ||
|
|
||
| let Some(close) = find_matching_paren(code, open) else { | ||
| break; | ||
| }; | ||
| let args = split_top_level_args(&code[open + 1..close]); | ||
| out.push(mapper(args)); | ||
| cursor = close + 1; | ||
| } | ||
|
|
||
| out | ||
| } | ||
|
|
||
| fn find_matching_paren(input: &str, open_idx: usize) -> Option<usize> { | ||
| let mut depth = 0i32; | ||
| let mut in_quote: Option<char> = None; | ||
| let mut escaped = false; | ||
|
|
||
| for (offset, ch) in input[open_idx..].char_indices() { | ||
| let idx = open_idx + offset; | ||
| if let Some(quote) = in_quote { | ||
| if escaped { | ||
| escaped = false; | ||
| continue; | ||
| } | ||
| if ch == '\\' { | ||
| escaped = true; | ||
| continue; | ||
| } | ||
| if ch == quote { | ||
| in_quote = None; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| match ch { | ||
| '\'' | '"' => in_quote = Some(ch), | ||
| '(' => depth += 1, | ||
| ')' => { | ||
| depth -= 1; | ||
| if depth == 0 { | ||
| return Some(idx); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
|
||
| fn split_top_level_args(input: &str) -> Vec<&str> { | ||
| let mut parts = Vec::new(); | ||
| let mut start = 0usize; | ||
| let mut depth_paren = 0i32; | ||
| let mut depth_brace = 0i32; | ||
| let mut depth_bracket = 0i32; | ||
| let mut in_quote: Option<char> = None; | ||
| let mut escaped = false; | ||
|
|
||
| for (idx, ch) in input.char_indices() { | ||
| if let Some(quote) = in_quote { | ||
| if escaped { | ||
| escaped = false; | ||
| continue; | ||
| } | ||
| if ch == '\\' { | ||
| escaped = true; | ||
| continue; | ||
| } | ||
| if ch == quote { | ||
| in_quote = None; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| match ch { | ||
| '\'' | '"' => in_quote = Some(ch), | ||
| '(' => depth_paren += 1, | ||
| ')' => depth_paren -= 1, | ||
| '{' => depth_brace += 1, | ||
| '}' => depth_brace -= 1, | ||
| '[' => depth_bracket += 1, | ||
| ']' => depth_bracket -= 1, | ||
| ',' if depth_paren == 0 && depth_brace == 0 && depth_bracket == 0 => { | ||
| parts.push(input[start..idx].trim()); | ||
| start = idx + 1; | ||
| } | ||
| _ => {} | ||
| } | ||
| let (out_tx, out_rx) = mpsc::unbounded_channel::<Value>(); | ||
| let mut writer_task = tokio::spawn(write_outbound(out_rx, writer)); | ||
| let pending = PendingClientRequests::default(); | ||
| ctx.requester = Some(ClientRequester::new(out_tx.clone(), pending.clone())); | ||
| let (in_tx, mut in_rx) = mpsc::channel::<Value>(MAX_PENDING_CLIENT_MESSAGES); | ||
| let reader_task = tokio::spawn(read_inbound(reader, in_tx, pending, out_tx.clone())); | ||
|
|
||
| let mut writer_result = None; | ||
| let dispatch_result = tokio::select! { | ||
| result = &mut writer_task => { | ||
| writer_result = Some(result); | ||
| Ok(()) | ||
| } | ||
| result = dispatch_client_messages(&mut ctx, &mut in_rx, &out_tx) => result, | ||
| }; | ||
| // A failed output must not wait for another input frame to finish shutdown. | ||
| let abort_reader = writer_result.is_some() || dispatch_result.is_err(); | ||
| if abort_reader { | ||
| reader_task.abort(); | ||
| } | ||
| let reader_result = reader_task.await; | ||
| ctx.requester = None; | ||
| drop(out_tx); | ||
| let writer_result = match writer_result { | ||
| Some(result) => result, | ||
| None => writer_task.await, | ||
| }; | ||
| ctx.finalize_trajectory().await; | ||
|
|
||
| if start <= input.len() { | ||
| let tail = input[start..].trim(); | ||
| if !tail.is_empty() { | ||
| parts.push(tail); | ||
| } | ||
| match reader_result { | ||
| Err(error) if abort_reader && error.is_cancelled() => {} | ||
| result => result??, | ||
| } | ||
| parts | ||
| writer_result??; | ||
| dispatch_result | ||
| } |
There was a problem hiding this comment.
The expanded transport implementation leaves runtime.rs at 764 lines. This violates the repository directive that files over 500 lines must be split into directory modules. The requirement must be satisfied before merging, such as by extracting the transport loop and its helpers into a submodule.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-mcp/src/runtime.rs
Line: 618-658
Comment:
**Runtime File Exceeds Limit**
The expanded transport implementation leaves `runtime.rs` at 764 lines. This violates the repository directive that files over 500 lines must be split into directory modules. The requirement must be satisfied before merging, such as by extracting the transport loop and its helpers into a submodule.
**Context Used:** CLAUDE.md ([source](https://github.com/nerdsane/temper/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Rita has paused the broader recording/MCP/merge-history repair pending a cleanup decision. These changes are preserved, not approved for wholesale merge as part of the fork-token correction. The separately authorized narrow fix is arni-labs/stack#19 and #461 (ARN-498), followed by Nick's ORIGINAL #411 and #412. Do not merge this broader candidate to unblock those PRs. The original approval work in #436 must be distinguished from this task's added changes before any later cleanup or merge. |
rita-aga
changed the title
2 participants
Enable in-chat approval prompts when a Temper MCP call receives a Cedar denial. The existing implementation negotiates MCP elicitation support, leaves declined or unanswered decisions pending, and supports a separate approver credential. See docs/adrs/0173-mcp-elicitation-approvals.md.
The unrequested ARN-461 gate, file-upload, and transport additions made by the coordinating Codex task have been removed at Rita’s request. Commit9597895d restores the exact tree at ab265ce, before those additions; the original approval implementation is preserved. This PR is separate from the minimum token fix in #461 and is not being advanced by that task.
The removal was verified by exact Git tree equality. Earlier tests/reviews of the added repair do not represent verification of this restored proposal.
Original implementation: Claude Code. Removal of coordinating-task additions: Codex (Astra), Codex desktop.