feat(groups): I11 — NEGOTIATION style, the trade form - #641
Conversation
EDDI had win/lose decision forms and no trade form. A negotiation
drafts a compromise with an explicit concession ledger for human
sign-off; the typed structure is the anti-sycophancy mechanism.
- Phase types PROPOSAL + BARGAIN; skipIf=AGREEMENT_REACHED, a single
enum condition (deliberately not an expression language) that skips
the arbitration once a typed AGREEMENT exists.
- NegotiationState on GroupConversation: proposals (OPEN/SUPERSEDED,
acceptedBy, acceptance entry indices) + the concession ledger.
- BARGAIN turns are a typed JSON move with three-tier parsing; an
unreadable turn is prose with no state effect. A concession that
names nothing in return is not recorded. A new proposal supersedes
the mover''s own open one; proposers sign their own terms.
- The open proposals + ledger are appended to every negotiation turn -
the record the outcome will quote.
- Unanimous acceptance ends the bargaining repeats early (the I2
outcome plumbing) and records DecisionRecord{AGREEMENT,
method=negotiation} whose tally.signedAcceptances maps signatories
to their signed acceptance entries - the co-signatures, no new
crypto. Failed bargaining runs the arbitration, whose conclusion
becomes DecisionRecord{VERDICT, method=arbitration}.
- Preset NEGOTIATION: Positions & Interests (parallel, blind) ->
Opening Proposals -> Bargaining (repeats=maxRounds) -> Arbitration
(conditional, own template) -> Synthesis.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds the ChangesNegotiation protocol and state
Negotiation state machine
Phase execution integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agents
participant PhaseExecutionEngine
participant NegotiationEngine
participant GroupConversation
participant Moderator
Agents->>PhaseExecutionEngine: Submit proposals and bargain turns
PhaseExecutionEngine->>NegotiationEngine: Apply negotiation repeat
NegotiationEngine->>GroupConversation: Persist proposals, acceptances, and concessions
NegotiationEngine->>PhaseExecutionEngine: Provide negotiation state
NegotiationEngine->>GroupConversation: Record unanimous agreement
Moderator->>NegotiationEngine: Provide arbitration synthesis when needed
NegotiationEngine->>GroupConversation: Record verdict
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java (1)
302-325: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the rendered ledger size.
truncatecaps each individual field atMAX_QUOTED_TERMS_CHARS, but the number of rendered concessions and open proposals is unbounded. The ledger is appended to every PROPOSAL, BARGAIN, and SYNTHESIS turn, so the prompt grows with each round. With the defaultmaxTurnsof 50 this adds token cost on every turn and can crowd out the transcript.Render the most recent N concessions and state the count of the omitted lines. The full ledger stays in
NegotiationState, so no record is lost.♻️ Proposed refactor
+ /** The ledger is quoted every turn; render the recent tail, keep the full record in state. */ + static final int MAX_RENDERED_CONCESSIONS = 20; + sb.append("Concession ledger:\n"); if (state.getConcessions().isEmpty()) { sb.append("- (empty)\n"); } else { - for (Concession c : state.getConcessions()) { + List<Concession> all = state.getConcessions(); + int from = Math.max(0, all.size() - MAX_RENDERED_CONCESSIONS); + if (from > 0) { + sb.append("- (").append(from).append(" earlier concession(s) omitted)\n"); + } + for (Concession c : all.subList(from, all.size())) { sb.append("- ").append(c.byAgentId()).append(" gave up \"").append(truncate(c.gaveUp())) .append("\" in return for \"").append(truncate(c.inReturnFor())).append("\"\n"); } }🤖 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 `@src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java` around lines 302 - 325, Bound the rendered negotiation ledger in the code building the StringBuilder output: render only the most recent N concessions from state.getConcessions(), and append a clear count of omitted concession lines when older entries are skipped. Keep the complete concession history in NegotiationState and preserve the existing empty-ledger output; apply the same bounded-rendering approach to open proposals if required by the ledger size limit.
🤖 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 `@src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java`:
- Around line 107-112: Update CURRENT_SCHEMA_VERSION from 3 to 4 and add the
corresponding 3→4 document migration in the existing schema migration mechanism.
Ensure the migration preserves the complete negotiation transcript, including
proposals, acceptances, and the concession ledger, while advancing the persisted
document version to 4.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java`:
- Around line 160-184: Update NegotiationEngine.applyMove in
src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java:160-184
to define handling when a turn contains both accept and proposal, either
withdrawing the mover’s signature from every other OPEN proposal before
addProposal or ignoring accept with a WARN; ensure stale signatures and open
offers cannot remain active. Update
src/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java:261-293
to assert the intended state of p2 after a2 signs p3, and add coverage for a
turn that both accepts and counter-proposes.
- Around line 228-234: Update the moderator exclusion filter in the participants
stream to compare moderatorAgentId against each id null-safely using the already
imported Objects utility, preserving exclusion of the moderator and allowing
null agentId values without throwing.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java`:
- Around line 38-40: Extend NegotiationEngineTest with a GroupMember whose
memberType() is MemberType.GROUP, include it in the agreement participants, and
assert that unanimity does not require this participant’s signature. Keep the
existing moderator coverage intact.
- Around line 131-144: Extend apply_badMovesAreInert to first supersede proposal
p1, then attempt to accept p1, while preserving the existing unknown-proposal
and unparseable-turn cases. Assert that the superseded acceptance leaves
proposals and concessions unchanged, covering the non-open branch in
NegotiationEngine.applyMove.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java`:
- Around line 302-325: Bound the rendered negotiation ledger in the code
building the StringBuilder output: render only the most recent N concessions
from state.getConcessions(), and append a clear count of omitted concession
lines when older entries are skipped. Keep the complete concession history in
NegotiationState and preserve the existing empty-ledger output; apply the same
bounded-rendering approach to open proposals if required by the ledger size
limit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a88574a0-9c6e-4728-9857-c3016c425c30
📒 Files selected for processing (14)
docs/changelog.mddocs/group-conversations.mdsrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/configs/groups/model/DiscussionStylePresets.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/rest/RestAgentGroupStore.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.javasrc/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.javasrc/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.javasrc/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.javasrc/test/java/ai/labs/eddi/configs/groups/model/DiscussionStylePresetsTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java
- Putting new terms on the table now withdraws the mover's signatures from every other open proposal, and signing someone else's terms supersedes the signer's own open offer - a proposal can no longer reach unanimity on a signature its signatory moved away from. A turn carrying both accept and proposal resolves for the proposal (WARN). - CURRENT_SCHEMA_VERSION 3->4: negotiationState is resume-critical; an older pod re-saving a paused v4 doc would drop the table (identity hop, no migration entry needed). - NegotiationState getters return unmodifiable views; mutation goes through addProposal/replaceProposal/addConcession. - Unused phase param dropped from applyRepeat; moderator filter null-safe; 4 CodeQL log-injection sites sanitized.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java (1)
250-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the last-entry selection rule in
recordArbitration.The test passes a single SYNTHESIS entry.
recordArbitrationselects the last non-blank SYNTHESIS entry throughreduce((first, second) -> second). With one entry, this assertion also passes for afindFirstimplementation, so the selection rule is untested. A blank-content entry is also untested.Pass a list with a blank entry, an earlier entry, and the intended final entry.
💚 Proposed addition
void arbitration_recordsVerdictOnce() { var gc = gc(); + var earlier = new TranscriptEntry("mod", "Moderator", "Preliminary reading.", 3, + "Arbitration", TranscriptEntryType.SYNTHESIS, Instant.now(), null, null); + var blank = new TranscriptEntry("mod", "Moderator", " ", 3, + "Arbitration", TranscriptEntryType.SYNTHESIS, Instant.now(), null, null); var arbitrationEntry = new TranscriptEntry("mod", "Moderator", "Split 55/45; support shared.", 3, "Arbitration", TranscriptEntryType.SYNTHESIS, Instant.now(), null, null); - NegotiationEngine.recordArbitration(gc, List.of(arbitrationEntry), "Arbitration"); + NegotiationEngine.recordArbitration(gc, List.of(earlier, arbitrationEntry, blank), "Arbitration");🤖 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 `@src/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java` around lines 250 - 259, Update arbitration_recordsVerdictOnce to pass three SYNTHESIS entries: a blank-content entry, an earlier substantive entry, and the intended final entry. Keep the assertions focused on the final entry’s outcome so the test verifies recordArbitration selects the last non-blank entry rather than the first.src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java (2)
174-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider withdrawing prior signatures when an agent accepts a different proposal.
The accept-only path supersedes the signer's own OPEN proposal. It does not withdraw the signer's signature from other OPEN proposals. An agent that accepts
p1and later acceptsp2remains a signatory on both. Two proposals can then satisfy unanimity at the same time, and list order decides the winner incheckAndRecordAgreement.
addProposalalready applies the stricter rule for counter-proposals. If one live position per agent is the intended invariant, apply the same withdrawal here. If multiple simultaneous acceptances are intended, state that in the Javadoc.🤖 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 `@src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java` around lines 174 - 199, Update the accept-only handling in NegotiationEngine so accepting a proposal withdraws the agent’s signature from every other OPEN proposal, not just superseding the agent’s own offer. Preserve the accepted proposal’s signature and existing supersession behavior, and apply the same one-live-position invariant used by addProposal before checkAndRecordAgreement evaluates unanimity.
351-359: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the rendered concession ledger.
truncatebounds each value, but not the block. Open proposals are bounded by participant count, because one agent holds at most one OPEN proposal. The concession ledger is append-only across every repeat and every round, and this method renders every entry into every PROPOSAL, BARGAIN, and SYNTHESIS turn. A long negotiation therefore grows each prompt without limit, which raises token cost and can exceed the model context.Render the most recent N entries and note the count of omitted lines.
♻️ Proposed bound
+ /** The ledger is append-only; only the most recent lines are quoted back. */ + static final int MAX_RENDERED_CONCESSIONS = 25; +sb.append("Concession ledger:\n"); if (state.getConcessions().isEmpty()) { sb.append("- (empty)\n"); } else { - for (Concession c : state.getConcessions()) { + List<Concession> ledger = state.getConcessions(); + int from = Math.max(0, ledger.size() - MAX_RENDERED_CONCESSIONS); + if (from > 0) { + sb.append("- (").append(from).append(" earlier concession(s) omitted)\n"); + } + for (Concession c : ledger.subList(from, ledger.size())) { sb.append("- ").append(c.byAgentId()).append(" gave up \"").append(truncate(c.gaveUp())) .append("\" in return for \"").append(truncate(c.inReturnFor())).append("\"\n"); } }🤖 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 `@src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java` around lines 351 - 359, Bound the concession ledger rendering in the block that iterates over state.getConcessions() by emitting only the most recent N entries, while preserving newest-to-oldest or existing order as appropriate. Add a note indicating how many older lines were omitted, and keep the existing per-value truncate behavior for rendered entries.
🤖 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.
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.java`:
- Around line 174-199: Update the accept-only handling in NegotiationEngine so
accepting a proposal withdraws the agent’s signature from every other OPEN
proposal, not just superseding the agent’s own offer. Preserve the accepted
proposal’s signature and existing supersession behavior, and apply the same
one-live-position invariant used by addProposal before checkAndRecordAgreement
evaluates unanimity.
- Around line 351-359: Bound the concession ledger rendering in the block that
iterates over state.getConcessions() by emitting only the most recent N entries,
while preserving newest-to-oldest or existing order as appropriate. Add a note
indicating how many older lines were omitted, and keep the existing per-value
truncate behavior for rendered entries.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java`:
- Around line 250-259: Update arbitration_recordsVerdictOnce to pass three
SYNTHESIS entries: a blank-content entry, an earlier substantive entry, and the
intended final entry. Keep the assertions focused on the final entry’s outcome
so the test verifies recordArbitration selects the last non-blank entry rather
than the first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 58d06b48-9e5c-4fa4-a18b-2eb08051a45a
📒 Files selected for processing (5)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/NegotiationEngine.javasrc/test/java/ai/labs/eddi/engine/internal/groups/NegotiationEngineTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java
- docs/changelog.md
- src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java
continueDiscussion cleared every other round-scoped conclusion but not the persisted negotiation state - round 2 ran against round 1's proposals, and one fresh acceptance could reach unanimous agreement on signatures cast for a different question.
3 participants
Summary
Implements I11 — NEGOTIATION style from
planning/group-collaboration-improvements-plan.md(first Wave 3 item in theplanning/group-collaboration-NEXT.mdqueue, after I17 #637, I14 #638, I8 #639, I6 #640). EDDI had win/lose decision forms (verdicts, votes) and no trade form: a negotiation is a process for surfacing trade-offs, and its output is a drafted compromise with an explicit concession ledger for human sign-off. The typed structure is precisely what stops sycophantic instant-agreement.Design (per the plan, decisions honored)
PROPOSAL+BARGAIN;skipIf: "AGREEMENT_REACHED"— a single enum condition, deliberately NOT an expression language: a phase is skipped for a reason the engine can prove against the typed decision. The arbitration phase is its only user.NegotiationStateonGroupConversation: proposals{id, byAgentId, round, terms (String v1), status OPEN|SUPERSEDED, acceptedBy, acceptanceEntryIndices}+ concessions{byAgentId, round, gaveUp, inReturnFor, refProposalId}. Persisted with the document, so a pause/resume keeps the table as it stood.{"accept": "<proposalId>"|null, "proposal": {"terms": "..."}|null, "concessions": [{"gaveUp": "...", "inReturnFor": "..."}]}+ free-text reasoning. Three-tier parse mirroringVoteTallyEngine's discipline (strict JSON → embedded → give up,FAIL_ON_TRAILING_TOKENS); an unparseable turn is prose with no state effect (WARN) — never a guessed acceptance. A concession that names nothing in return is not recorded — the rule is the structure, and the baked-in template says so ("Every concession must name what you received in return. The ledger below is the record — it will be quoted in the outcome."). A new proposal supersedes the mover's own open one (one live offer per agent); the proposer signs their own terms implicitly.NegotiationEngine.appendStateIfRelevantat the two input-build sites rather than templated — the state lives on the conversation, whichbuildPhaseInputdeliberately does not see.PhaseOutcome.endRepeatsplumbing convergence uses, andDecisionRecord{AGREEMENT, method="negotiation"}carriestally.signedAcceptances— each signatory mapped to the transcript index of their (already signed) acceptance entry. The signed entries ARE the co-signatures; no new crypto.decision_reachedfires.NEGOTIATION: ① Positions & Interests (ALL, PARALLEL, NONE — interests enable integrative trades) ② Opening Proposals (SEQUENTIAL, FULL) ③ Bargaining (SEQUENTIAL, FULL, repeats=maxRounds) ④ Arbitration (MODERATOR,skipIf=AGREEMENT_REACHED, its ownTEMPLATE_ARBITRATION— the default synthesis template asks for a balanced summary; an arbitrator decides) ⑤ Synthesis. An arbitration that runs records its conclusion asDecisionRecord{VERDICT, method="arbitration"}, never overwriting an existing decision.TranscriptEntryType.PROPOSAL/BARGAINalready existed from Wave 0's F4 peer-visibility matrix — peer-visible, as open bargaining must be).Tests (12 new in
NegotiationEngineTest+ preset shape + enum pins; 1694 tests green acrossengine.internal+configs.groups; checkstyle clean)Parse tiers incl. JSON-followed-by-reasoning; concession-must-name-return; implicit self-signature with the authoring entry index asserted; supersession; unknown/superseded acceptances and unparseable turns are inert (state compared before/after); ledger accumulation with round + refProposal attribution; unanimous acceptance with the signed entry indices asserted exactly; partial acceptance ≠ agreement (and the moderator's signature is not required); arbitration records once and never overwrites; ledger rendering into the turn + its no-op paths; preset shape (5 phases, PARALLEL+NONE positions, repeats=maxRounds bargaining, conditional arbitration with its own template); and the plan's scripted 3-round bargain converging in round 3 (propose → counter+concede → sign) as living documentation of the protocol.
Summary by CodeRabbit
New Features
NEGOTIATIONdiscussion style with structured proposals, bargaining, concessions, agreement detection, arbitration, and final synthesis.Documentation
Tests