feat: harden MRTR workflows and defer Tasks - #386
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change binds MRTR approval continuations to authenticated request context and exact operations. It adds explicit cancellation errors and audit status handling. Documentation defines the MRTR boundary and keeps Tasks unimplemented. Integration tests cover approval, replay, mismatch, cancellation, redaction, and audit behavior. ChangesMRTR confirmation and lifecycle handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MiftahHTTPServer
participant OperationPipeline
participant ApprovalStore
participant AuditLog
MCPClient->>MiftahHTTPServer: call confirmation-protected operation
MiftahHTTPServer->>OperationPipeline: process operation
OperationPipeline->>ApprovalStore: create request-bound continuation
OperationPipeline-->>MCPClient: return input_required and requestState
MCPClient->>MiftahHTTPServer: retry with inputResponses
MiftahHTTPServer->>ApprovalStore: validate and consume continuation
OperationPipeline->>AuditLog: record approval lifecycle
OperationPipeline-->>MCPClient: return operation result
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
area:mcp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/2026-08-11-mrtr-tasks-decision.md`:
- Line 54: Update the Tasks ownership requirements to include tasks/update
alongside tasks/get, tasks/result, and tasks/cancel, ensuring task lookup,
inputResponses updates, and cancellation enforce the same ownership check. Add a
corresponding ownership assertion in tests/mrtr-tasks-decision.test.ts.
In `@tests/mcp-v2-serving.test.ts`:
- Around line 63-85: Update waitForAuditEvent so malformed or partially written
JSONL lines are ignored during parsing rather than propagating SyntaxError.
Preserve valid parsed events and the existing ENOENT handling, allowing the next
polling iteration to discover the completed trailing line.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4dd8d3e9-e6bc-41b7-86fe-c10896715d2d
📒 Files selected for processing (17)
CHANGELOG.mddocs/config.mddocs/library-api.mddocs/plans/2026-08-11-mrtr-tasks-decision.mdsrc/approvals/approval-store.tssrc/audit/audit-types.tssrc/cli/exit-codes.tssrc/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/utils/errors.tstests/approval-continuation-store.test.tstests/cli-exit-codes.test.tstests/mcp-v2-serving.test.tstests/mcp-wrapper.test.tstests/mrtr-tasks-decision.test.tstests/package-contract.test.tstests/stateless-profile-context-runtime.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/2026-08-11-mrtr-tasks-decision.md`:
- Line 54: Remove tasks/result from the task-method list in
docs/plans/2026-08-11-mrtr-tasks-decision.md at line 54, retaining tasks/get,
tasks/update, and tasks/cancel. Update the matching assertion in
tests/mrtr-tasks-decision.test.ts at line 26 to expect the same current
contract, since tasks/get returns terminal results.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53dbe500-8722-4038-932f-65f51b8ba8cf
📒 Files selected for processing (3)
docs/plans/2026-08-11-mrtr-tasks-decision.mdtests/mcp-v2-serving.test.tstests/mrtr-tasks-decision.test.ts
coderabbitai
Bot
added
the
documentation
|
@coderabbitai review All three actionable threads are addressed on exact head 665b3a8, including the current SEP-2663 method contract. Please submit a fresh review decision for this head. |
|
|
|
@coderabbitai review Exact head d15c291 is green without reruns. All actionable review threads are addressed, including SEP-2663 methods and bounded fixture cleanup. Please submit a fresh review decision for this head. |
|
|
|
@coderabbitai review Please review exact head d15c291 and submit a fresh decision. All prior actionable findings are resolved, and exact-head CI passed every job without reruns. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/2026-08-11-mrtr-tasks-decision.md (1)
28-28: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCorrect the audit retention contract.
requestState,inputResponses, profile-context handles, and approval bearers remain excluded. Operationargumentsare redacted and stored whenaudit.includeArgumentsis enabled; otherwise they are omitted. State that MRTR operation records follow this policy on every round, and add contract coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-08-11-mrtr-tasks-decision.md` at line 28, Update the audit retention contract near the audit journal description to state that MRTR operation records apply the policy on every round: continue excluding requestState, inputResponses, profile-context handles, and approval bearers; redact and store operation arguments only when audit.includeArguments is enabled, omitting them otherwise. Add contract coverage verifying these retention rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/plans/2026-08-11-mrtr-tasks-decision.md`:
- Line 28: Update the audit retention contract near the audit journal
description to state that MRTR operation records apply the policy on every
round: continue excluding requestState, inputResponses, profile-context handles,
and approval bearers; redact and store operation arguments only when
audit.includeArguments is enabled, omitting them otherwise. Add contract
coverage verifying these retention rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1cf83772-c7d1-47e9-9b4b-9159db2115c5
📒 Files selected for processing (4)
docs/plans/2026-08-11-mrtr-tasks-decision.mdtests/mcp-v2-serving.test.tstests/mrtr-tasks-decision.test.tsvitest.config.ts
|
@coderabbitai review Exact head 14947d0 now passes the complete CI matrix without retries, and the MRTR audit-retention finding has executable coverage in both argument modes. Please submit a consolidated fresh review decision for this exact head. |
|
✅ Action performedReview finished.
|
Superseded by exact head 14947d0: all findings addressed; exact-head CodeRabbit check and full CI matrix passed.
1 participant
Summary
Acceptance evidence
miftah serve --transport http: one elicitation, one upstream mutation, andrequested/approved/consumedaudit actionscancelled/REQUEST_CANCELLED; the selected upstream receives cancellation and the request-scoped session closesValidation
npm test— 1,943 passed, 34 platform skipsnpm run test:packagenpm run buildnpm run typechecknpm run lintnpm run check:test-fixturenpm run check:packnpm audit --omit=dev— 0 vulnerabilitiesFixes #366
Fixes #384
Summary by CodeRabbit
Fixes #387