fix+test: freeze pending nego.open + M3.3 adversarial e2e (#37)#68
Conversation
Same-initiator redelivery must not escalate goal/mandate behind a stale pending gate (RT-PEND-1); remove preserveProgress overwrite path.
Cover the six ROADMAP defenses plus nego.open redelivery across pending/live/signed/closed/open_rejected, and point the checklist at it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe session state machine now treats pending ChangesAdversarial negotiation handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdversarialTest
participant Relay
participant handleInbox
participant SessionStore
AdversarialTest->>Relay: POST adversarial envelope
Relay->>handleInbox: dispatch accepted envelope
handleInbox->>SessionStore: process replay or nego.open
SessionStore-->>handleInbox: reject or preserve session state
handleInbox-->>AdversarialTest: return result and status
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| const { thread } = await openSessionToSigned(alice, bob); | ||
| await redeliverNegoOpen(alice, bob, thread, { | ||
| ...OPEN_PAYLOAD, | ||
| goal: HIJACK_GOAL, |
There was a problem hiding this comment.
[suggestion] The nego.open redelivery matrix only overrides goal with HIJACK_GOAL. The real pending-mutation threat (shown in nego-open-pending-mutation.probe.test.ts) also escalates acceptance, budget.max_turns, and especially mandate (e.g. emptying human_required). Because the freeze is all-or-nothing, goal-only checks are a reasonable proxy, but the suite currently claims “terms frozen” while only asserting goal invariance end-to-end. A regression that somehow reapplied mandate/acceptance while leaving goal alone would still pass every e2e case here.
Suggestion: In at least the pending (and ideally live) redelivery case, redeliver a full escalated payload matching the probe, then assert freeze of goal/mandate/acceptance/budget via handleSessionStatus and/or the session_open pending item fields—not only unwrapGoal(status.goal).
| await assertStatusAndGoal(bob, thread, "live", OPEN_PAYLOAD.goal); | ||
| }, 45000); | ||
|
|
||
| it("signed: status + terms unchanged (light msg/report/sign path)", async () => { |
There was a problem hiding this comment.
[suggestion] The highest-value pending scenario stops after redelivery + status/goal checks. It never re-runs human approve after the hijacked open and asserts the resulting live session still carries first-open terms. That approve path is exactly where the pre-fix store mutation became harmful (handleApproveOpen promotes the store record, not the redelivered wire payload). The protocol probe covers this; the e2e matrix does not, so “full stack” coverage of the security fix is incomplete despite looking thorough.
Suggestion: After redelivering the escalated nego.open on pending, call handleHumanApprove with the original pendingId / approval code and assert post-approve status live with original goal/mandate (and that no second session_open pending was created).
| expect(opened.ok).toBe(true); | ||
| if (!opened.ok) return; | ||
|
|
||
| const first = await bobMachine.handleIncomingEnvelope({ |
There was a problem hiding this comment.
[nit] With createLinkedMachines, aliceMachine.handleOpen already delivers nego.open into Bob’s machine. The subsequent first handleIncomingEnvelope is therefore a same-terms redelivery, not the initial open. The test still correctly proves escalated terms are ignored, but the narrative (“first” vs “second”) and setup are slightly misleading. Also, pending-queue assertions only check goal (lines 80/125) while the store checks are thorough—pre-fix ensureRecipientOpenPending already returned the existing pending without rewriting fields, so pending-only checks would not have caught the original bug.
Suggestion: Drop the redundant first delivery (or pause linked delivery for the initial open), and optionally assert pending mandate/acceptance/budget for documentation symmetry with the store checks. Keep store + approve assertions as the primary regression signal.
| } | ||
|
|
||
| const redelivered = await bobMachine.handleIncomingOpen({ | ||
| thread: opened.thread, |
There was a problem hiding this comment.
[nit] The new unit test only mutates goal via handleIncomingOpen and asserts the return shape { ok: true, status: "pending" }. That is useful for the early-return contract, but it largely duplicates the probe’s weaker axis and does not lock the freeze for mandate/acceptance/budget at the unit layer (those are probe-only).
Suggestion: Either extend this test to assert full term equality (matching the probe’s store expectations) or keep it narrowly as a return-shape test and rely on the probe for field-level freeze—worth a one-line comment so future edits do not treat this as full N1/pending coverage.
Assert full goal/mandate/acceptance/budget freeze in e2e and unit tests, approve-after-hijack on pending, and drop redundant probe first delivery.
1 participant
Summary
nego.openterms while a session ispendingso same-initiator redelivery cannot escalate goal/mandate behind a stale human gate (RT-PEND-1).nego.openredelivery across pending/live/signed/closed/open_rejected viahandleInbox/ relay POST.open_expiredcredited to protocol unit tests).Closes #37
Type of Change
Test plan
pnpm --filter @agentpair/protocol exec vitest run src/session/state-machine.test.ts src/session/nego-open-pending-mutation.probe.test.ts(132 passed)pnpm --filter agentpair exec vitest run src/e2e/adversarial.test.ts src/e2e/happy-path.test.ts(11 passed)Notes
open_expired,dual-server.syncInboxes→handleInbox, hostinbox-hygienerouting_mismatchcase; SPEC N1 wording → M3.5.Summary by CodeRabbit
Bug Fixes
Tests