From 5fcfdd0e4b491ecf90ba7d0081ab73d66a047404 Mon Sep 17 00:00:00 2001 From: spud <92900806+jamespud@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:29:08 +0800 Subject: [PATCH 1/4] fix(report): validate authoritative citations and persist dissent The report generator accepted any non-empty citation list, so a fabricated evidence/claim ID passed, and the model authored the authoritative decision itself. Minority positions were re-derived at read time from vote rows, which meant the persisted report, HTTP, A2A and export could disagree, and a legacy resolution with no stored record was indistinguishable from a confirmed "no dissent". - Reject empty, unknown and duplicate report citations; require the citation set to be non-empty when authoritative material exists. The report model no longer supplies the decision: `FinalReportData` drops the field and the rendered heading comes from `resolution.FinalDecision`. - Persist structured dissent on the resolution (`Resolution.Dissent`, migration `magi_s30_resolution_dissent.sql`). The column is nullable so the tri-state survives a round trip: NULL = never recorded (legacy/uncommitted), `[]` = recorded none, `[...]` = recorded minority. HTTP, A2A, export and the frontend project the same `dissent_state`. - Bind dissent to the frozen round's ballots at resolve time (position-aligned agent attribution) and embed the deterministic section in the canonical persisted report so every read path returns the same bytes. - Make `ArtifactRemap.RemapText` idempotent: a namespaced ID contains its bare form as a suffix, so the previous implementation doubled the prefix when a report echoed one of the namespaced IDs it was given. Boundary: this protects the structured report (decision binding, citation membership, dissent truthfulness). It does not semantically verify free-text claims about authorization, sending or verification; that six-dimension state contract is tracked by Epic #7, so REPORT-01 is not fully closed here. --- backend/adapter/a2a_repository.go | 17 +- ...act_generation_orchestration_mysql_test.go | 45 +++- backend/adapter/model.go | 10 +- backend/adapter/repository.go | 38 +++- backend/adapter/resolution_dissent_test.go | 55 +++++ backend/application/a2a/projector.go | 42 +++- backend/application/a2a/projector_test.go | 52 +++++ backend/bootstrap/migration_parity_test.go | 26 +++ backend/bootstrap/mysql_migration_s30_test.go | 207 ++++++++++++++++++ backend/domain/entity/idmap.go | 41 +++- backend/domain/entity/idmap_test.go | 21 ++ backend/domain/entity/report.go | 6 +- backend/domain/entity/resolution.go | 55 ++++- .../orchestration/dissent_binding_test.go | 180 +++++++++++++++ .../domain/orchestration/integration_test.go | 6 +- backend/domain/orchestration/orchestrator.go | 14 ++ .../domain/orchestration/orchestrator_test.go | 79 ++++++- backend/domain/orchestration/steps.go | 29 ++- backend/domain/prompt/defaults.go | 3 +- backend/domain/service/commander.go | 79 +++++-- backend/domain/service/commander_test.go | 20 +- .../service/commander_truthfulness_test.go | 97 ++++++++ backend/domain/service/dissent.go | 25 ++- backend/domain/service/dissent_test.go | 46 +++- backend/domain/service/report.go | 9 +- backend/domain/service/report_test.go | 10 +- backend/server/dto/dissent_state_test.go | 75 +++++++ backend/server/dto/dto.go | 56 +++-- backend/server/handler/decision.go | 19 +- backend/server/handler/export.go | 17 +- .../magi_s30_resolution_dissent.sql | 12 + frontend/src/api/client.ts | 3 + .../components/workspace/ConsensusPanel.tsx | 10 + frontend/src/stores/caseStore.ts | 1 + frontend/src/types/case.ts | 3 + 35 files changed, 1258 insertions(+), 150 deletions(-) create mode 100644 backend/adapter/resolution_dissent_test.go create mode 100644 backend/bootstrap/mysql_migration_s30_test.go create mode 100644 backend/domain/orchestration/dissent_binding_test.go create mode 100644 backend/domain/service/commander_truthfulness_test.go create mode 100644 backend/server/dto/dissent_state_test.go create mode 100644 docker/atlas/migrations/magi_s30_resolution_dissent.sql diff --git a/backend/adapter/a2a_repository.go b/backend/adapter/a2a_repository.go index 06219cdc..eb0206fe 100644 --- a/backend/adapter/a2a_repository.go +++ b/backend/adapter/a2a_repository.go @@ -270,13 +270,7 @@ func loadConversationResolutions(tx *gorm.DB, messages []*entity.ConversationMes out := make(map[string]*entity.Resolution, len(models)) for i := range models { model := &models[i] - out[model.CaseID] = &entity.Resolution{ - ID: model.ID, CaseID: model.CaseID, ExecutionGeneration: model.ExecutionGeneration, - Consensus: fromJSON[entity.ConsensusResult](model.ConsensusJSON), - FinalDecision: entity.VoteDecision(model.FinalDecision), FinalReport: model.FinalReport, - KeyEvidenceIDs: fromJSON[[]string](model.KeyEvidenceIDsJSON), KeyClaimIDs: fromJSON[[]string](model.KeyClaimIDsJSON), - VoteIDs: fromJSON[[]string](model.VoteIDsJSON), Evaluation: fromJSON[*entity.Evaluation](model.EvaluationJSON), CreatedAt: model.CreatedAt, - } + out[model.CaseID] = resolutionFromModel(model) } return out, nil } @@ -724,15 +718,6 @@ func loadTaskRecord(db *gorm.DB, binding A2ASubmissionModel, includeArtifacts bo return record, nil } -func resolutionFromModel(m *ResolutionModel) *entity.Resolution { - return &entity.Resolution{ - ID: m.ID, CaseID: m.CaseID, Consensus: fromJSON[entity.ConsensusResult](m.ConsensusJSON), - FinalDecision: entity.VoteDecision(m.FinalDecision), FinalReport: m.FinalReport, - KeyEvidenceIDs: fromJSON[[]string](m.KeyEvidenceIDsJSON), KeyClaimIDs: fromJSON[[]string](m.KeyClaimIDsJSON), - VoteIDs: fromJSON[[]string](m.VoteIDsJSON), Evaluation: fromJSON[*entity.Evaluation](m.EvaluationJSON), CreatedAt: m.CreatedAt, - } -} - func voteFromModel(m *VoteModel) *entity.Vote { return &entity.Vote{ ID: m.ID, CaseID: m.CaseID, AgentRunID: m.AgentRunID, Round: m.Round, Decision: entity.VoteDecision(m.Decision), diff --git a/backend/adapter/artifact_generation_orchestration_mysql_test.go b/backend/adapter/artifact_generation_orchestration_mysql_test.go index 9389b8d0..da6cec7f 100644 --- a/backend/adapter/artifact_generation_orchestration_mysql_test.go +++ b/backend/adapter/artifact_generation_orchestration_mysql_test.go @@ -2,6 +2,7 @@ package magi_test import ( "context" + "encoding/json" "errors" "fmt" "strings" @@ -35,9 +36,22 @@ type t3ScriptedModel struct { calls int } -func (m *t3ScriptedModel) Generate(context.Context, []*schema.Message, ...model.Option) (*schema.Message, error) { +func (m *t3ScriptedModel) Generate(_ context.Context, in []*schema.Message, _ ...model.Option) (*schema.Message, error) { m.mu.Lock() defer m.mu.Unlock() + var prompt strings.Builder + for _, msg := range in { + if msg != nil { + prompt.WriteString(msg.Content) + prompt.WriteString("\n") + } + } + // The report call cites the first authoritative ID in the prompt. The + // strict citation validator (issue #13) rejects the old canned "EV-001" + // because the frozen set holds namespaced IDs the script cannot know. + if strings.Contains(prompt.String(), "FinalReportData") { + return schema.AssistantMessage(t3CitingReportJSON(prompt.String()), nil), nil + } if m.calls >= len(m.responses) { return nil, fmt.Errorf("t3 scripted model exhausted after %d calls", m.calls) } @@ -46,6 +60,34 @@ func (m *t3ScriptedModel) Generate(context.Context, []*schema.Message, ...model. return msg, nil } +func t3CitingReportJSON(prompt string) string { + body := map[string]any{"summary": "decision report summary", "key_reasons": []string{"r1"}, "risks": []string{}, "next_steps": []string{}} + if id := t3FirstListedID(prompt, "Available evidence IDs: ["); id != "" { + body["key_evidence_ids"] = []string{id} + } else if id := t3FirstListedID(prompt, "Available claim IDs: ["); id != "" { + body["key_claim_ids"] = []string{id} + } + raw, _ := json.Marshal(body) + return string(raw) +} + +func t3FirstListedID(prompt, marker string) string { + i := strings.Index(prompt, marker) + if i < 0 { + return "" + } + rest := prompt[i+len(marker):] + j := strings.IndexByte(rest, ']') + if j < 0 { + return "" + } + fields := strings.Fields(rest[:j]) + if len(fields) == 0 { + return "" + } + return fields[0] +} + func (m *t3ScriptedModel) Stream(context.Context, []*schema.Message, ...model.Option) (*schema.StreamReader[*schema.Message], error) { return nil, fmt.Errorf("stream not implemented") } @@ -106,7 +148,6 @@ func t3Commander(t *testing.T) *service.Commander { t.Helper() script := &t3ScriptedModel{responses: []*schema.Message{ schema.AssistantMessage(`{"canonical_question":"compute"}`, nil), - schema.AssistantMessage(`{"decision":"approve","summary":"decision report summary","key_reasons":["r1"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), }} cmd, err := service.NewCommander( service.CommanderConfig{Model: entity.ModelRef{ModelID: 1}, Persona: "commander"}, diff --git a/backend/adapter/model.go b/backend/adapter/model.go index b951428f..cf4cb9fe 100644 --- a/backend/adapter/model.go +++ b/backend/adapter/model.go @@ -187,9 +187,13 @@ type ResolutionModel struct { FinalReport string `gorm:"type:text"` KeyEvidenceIDsJSON string `gorm:"type:text"` KeyClaimIDsJSON string `gorm:"type:text"` - VoteIDsJSON string `gorm:"type:text"` - EvaluationJSON string `gorm:"type:text"` - CreatedAt time.Time + // DissentJSON is nullable on purpose: NULL means the minority record was + // never persisted (legacy/uncommitted), while "[]" is an explicit recorded + // "no dissent". The distinction is part of the REPORT-01 contract. + DissentJSON *string `gorm:"type:text"` + VoteIDsJSON string `gorm:"type:text"` + EvaluationJSON string `gorm:"type:text"` + CreatedAt time.Time } func (ResolutionModel) TableName() string { return "resolution" } diff --git a/backend/adapter/repository.go b/backend/adapter/repository.go index 4510e1c6..e0337603 100644 --- a/backend/adapter/repository.go +++ b/backend/adapter/repository.go @@ -33,6 +33,32 @@ func fromJSON[T any](s string) T { return v } +// encodeDissent persists the tri-state minority record. A nil slice is stored +// as SQL NULL ("not recorded"); a non-nil slice is stored as its JSON form +// ("[]" or "[...]") so an explicit "no dissent" survives a round trip. +func encodeDissent(d []entity.Dissent) *string { + if d == nil { + return nil + } + b, err := json.Marshal(d) + if err != nil { + return nil + } + s := string(b) + return &s +} + +// decodeDissent restores the tri-state. NULL (legacy), an empty string and JSON +// null all mean "not recorded"; "[]" is an explicit recorded-none; "[...]" is a +// recorded minority. json.Unmarshal yields a non-nil empty slice for "[]", so +// the nil-ness of the returned slice preserves the distinction. +func decodeDissent(raw *string) []entity.Dissent { + if raw == nil || *raw == "" || *raw == "null" { + return nil + } + return fromJSON[[]entity.Dissent](*raw) +} + // --- aggregate --- type magiRepository struct { @@ -1022,7 +1048,7 @@ func resolutionModel(res *entity.Resolution) ResolutionModel { return ResolutionModel{ ID: res.ID, CaseID: res.CaseID, ExecutionGeneration: res.ExecutionGeneration, ConsensusJSON: toJSON(res.Consensus), FinalDecision: string(res.FinalDecision), FinalReport: res.FinalReport, KeyEvidenceIDsJSON: toJSON(res.KeyEvidenceIDs), KeyClaimIDsJSON: toJSON(res.KeyClaimIDs), - VoteIDsJSON: toJSON(res.VoteIDs), EvaluationJSON: toJSON(res.Evaluation), CreatedAt: res.CreatedAt, + DissentJSON: encodeDissent(res.Dissent), VoteIDsJSON: toJSON(res.VoteIDs), EvaluationJSON: toJSON(res.Evaluation), CreatedAt: res.CreatedAt, } } func (r *resolutionRepo) Get(ctx context.Context, caseID string) (*entity.Resolution, error) { @@ -1030,12 +1056,20 @@ func (r *resolutionRepo) Get(ctx context.Context, caseID string) (*entity.Resolu if err := r.db.WithContext(ctx).First(&m, "case_id = ?", caseID).Error; err != nil { return nil, err } + return resolutionFromModel(&m), nil +} + +// resolutionFromModel maps a persisted resolution row to its domain entity. +// Generic (non-authoritative) reads must never invent fields a legacy row did +// not carry: Dissent stays nil when dissent_json is empty. +func resolutionFromModel(m *ResolutionModel) *entity.Resolution { return &entity.Resolution{ ID: m.ID, CaseID: m.CaseID, ExecutionGeneration: m.ExecutionGeneration, Consensus: fromJSON[entity.ConsensusResult](m.ConsensusJSON), FinalDecision: entity.VoteDecision(m.FinalDecision), FinalReport: m.FinalReport, KeyEvidenceIDs: fromJSON[[]string](m.KeyEvidenceIDsJSON), KeyClaimIDs: fromJSON[[]string](m.KeyClaimIDsJSON), + Dissent: decodeDissent(m.DissentJSON), VoteIDs: fromJSON[[]string](m.VoteIDsJSON), Evaluation: fromJSON[*entity.Evaluation](m.EvaluationJSON), CreatedAt: m.CreatedAt, - }, nil + } } // --- EventRepository --- diff --git a/backend/adapter/resolution_dissent_test.go b/backend/adapter/resolution_dissent_test.go new file mode 100644 index 00000000..3ad692f7 --- /dev/null +++ b/backend/adapter/resolution_dissent_test.go @@ -0,0 +1,55 @@ +package magi_test + +import ( + "context" + "testing" + + magi "github.com/jamespud/magi/backend/adapter" + "github.com/jamespud/magi/backend/domain/entity" +) + +// The dissent tri-state must survive a persistence round trip: NULL (legacy, +// not recorded), an explicit non-nil empty slice (recorded none) and a +// non-empty slice (recorded minority) are three distinct facts. +func TestResolutionRepo_DissentTriStateRoundTrip(t *testing.T) { + db := openConversationDB(t) + repo := magi.NewRepository(db) + ctx := context.Background() + + // Simulate a legacy row written before the dissent column existed. The + // column is nullable, so an unset value is SQL NULL. + legacy := &entity.Resolution{ID: "res-legacy", CaseID: "c-legacy", FinalDecision: entity.VoteDecisionApprove, FinalReport: "# legacy"} + if err := repo.ResolutionRepo().Create(ctx, legacy); err != nil { + t.Fatalf("create legacy: %v", err) + } + if err := db.Exec("UPDATE resolution SET dissent_json = NULL WHERE case_id = ?", "c-legacy").Error; err != nil { + t.Fatalf("null out dissent: %v", err) + } + + none := &entity.Resolution{ID: "res-none", CaseID: "c-none", FinalDecision: entity.VoteDecisionApprove, FinalReport: "# none", Dissent: []entity.Dissent{}} + if err := repo.ResolutionRepo().Create(ctx, none); err != nil { + t.Fatalf("create none: %v", err) + } + present := &entity.Resolution{ID: "res-present", CaseID: "c-present", FinalDecision: entity.VoteDecisionApprove, FinalReport: "# present", + Dissent: []entity.Dissent{{AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, Reasoning: "risk", + Conditions: []entity.DecisionCondition{{Statement: "rollback", MustHold: true}}}}} + if err := repo.ResolutionRepo().Create(ctx, present); err != nil { + t.Fatalf("create present: %v", err) + } + + for caseID, want := range map[string]entity.DissentState{ + "c-legacy": entity.DissentUnknown, "c-none": entity.DissentNone, "c-present": entity.DissentPresent, + } { + got, err := repo.ResolutionRepo().Get(ctx, caseID) + if err != nil || got == nil { + t.Fatalf("get %s: %+v err=%v", caseID, got, err) + } + if got.DissentState() != want { + t.Fatalf("%s state = %s, want %s (dissent=%+v)", caseID, got.DissentState(), want, got.Dissent) + } + } + got, _ := repo.ResolutionRepo().Get(ctx, "c-present") + if len(got.Dissent) != 1 || got.Dissent[0].Reasoning != "risk" || len(got.Dissent[0].Conditions) != 1 { + t.Fatalf("present dissent content lost: %+v", got.Dissent) + } +} diff --git a/backend/application/a2a/projector.go b/backend/application/a2a/projector.go index d20dfb2d..bda37fbc 100644 --- a/backend/application/a2a/projector.go +++ b/backend/application/a2a/projector.go @@ -9,7 +9,6 @@ import ( a2a "github.com/a2aproject/a2a-go/v2/a2a" "github.com/jamespud/magi/backend/application/redact" "github.com/jamespud/magi/backend/domain/entity" - "github.com/jamespud/magi/backend/domain/service" ) // JobNone marks "no durable job row" inside a StatusGroup. The adapter turns it @@ -361,7 +360,8 @@ func (p *TaskProjector) artifactParts(rec *TaskRecord) (*a2a.Part, *a2a.Part) { md := &a2a.Part{Content: a2a.Text(summary), Filename: "decision-report.md", MediaType: "text/markdown"} result := &a2a.Part{Content: a2a.Data{Value: decisionResultJSON{ SchemaVersion: "1", CaseID: rec.Case.ID, ContextID: rec.Submission.ContextID, - Outcome: outcome, Dissent: []dissentJSON{}, EvidenceRefs: []string{}, ClaimRefs: []string{}, + Outcome: outcome, DissentState: string(entity.DissentUnknown), + Dissent: nil, EvidenceRefs: []string{}, ClaimRefs: []string{}, }}, Filename: "decision-result.json", MediaType: "application/json"} return md, result } @@ -377,17 +377,30 @@ func (p *TaskProjector) decisionResult(rec *TaskRecord) decisionResultJSON { decision := string(res.FinalDecision) confidence := decisionConfidence(res) consensus := &consensusJSON{Outcome: string(res.Consensus.Outcome), Round: res.Consensus.Round} - dissent := make([]dissentJSON, 0) - for _, d := range service.ExtractDissent(res.FinalDecision, resolutionVotes(res), nil) { - dissent = append(dissent, dissentJSON{ - AgentCode: string(d.AgentCode), Decision: string(d.Decision), - Reasoning: p.redactor.String(d.Reasoning), EvidenceRefs: d.EvidenceIDs, ClaimRefs: d.ClaimIDs, - }) + // Dissent is read from the authoritative record persisted with the + // resolution, not re-derived from the vote list (issue #13). Its nil-ness + // encodes the tri-state: nil -> not recorded, [] -> recorded none. + dissentState := res.DissentState() + var dissent []dissentJSON + if res.Dissent != nil { + dissent = make([]dissentJSON, 0, len(res.Dissent)) + for _, d := range res.Dissent { + conds := make([]string, 0, len(d.Conditions)) + for _, c := range d.Conditions { + conds = append(conds, p.redactor.String(c.Statement)) + } + dissent = append(dissent, dissentJSON{ + AgentCode: string(d.AgentCode), Decision: string(d.Decision), + Reasoning: p.redactor.String(d.Reasoning), EvidenceRefs: d.EvidenceIDs, ClaimRefs: d.ClaimIDs, + Conditions: conds, + }) + } } return decisionResultJSON{ SchemaVersion: "1", CaseID: rec.Case.ID, ContextID: rec.Submission.ContextID, Outcome: "resolved", Decision: &decision, Consensus: consensus, Confidence: &confidence, - Dissent: dissent, EvidenceRefs: sortedCopy(res.KeyEvidenceIDs), ClaimRefs: sortedCopy(res.KeyClaimIDs), + DissentState: string(dissentState), Dissent: dissent, + EvidenceRefs: sortedCopy(res.KeyEvidenceIDs), ClaimRefs: sortedCopy(res.KeyClaimIDs), } } @@ -439,9 +452,13 @@ type decisionResultJSON struct { Decision *string `json:"decision"` Consensus *consensusJSON `json:"consensus,omitempty"` Confidence *float64 `json:"confidence"` - Dissent []dissentJSON `json:"dissent"` - EvidenceRefs []string `json:"evidenceRefs"` - ClaimRefs []string `json:"claimRefs"` + // DissentState is UNKNOWN (never recorded), NONE (recorded no dissent) or + // PRESENT. Dissent is null for UNKNOWN, [] for NONE and the minority list + // for PRESENT (issue #13). + DissentState string `json:"dissentState"` + Dissent []dissentJSON `json:"dissent"` + EvidenceRefs []string `json:"evidenceRefs"` + ClaimRefs []string `json:"claimRefs"` } type consensusJSON struct { @@ -455,4 +472,5 @@ type dissentJSON struct { Reasoning string `json:"reasoning,omitempty"` EvidenceRefs []string `json:"evidenceRefs,omitempty"` ClaimRefs []string `json:"claimRefs,omitempty"` + Conditions []string `json:"conditions,omitempty"` } diff --git a/backend/application/a2a/projector_test.go b/backend/application/a2a/projector_test.go index 8d6faa4a..4fc8b709 100644 --- a/backend/application/a2a/projector_test.go +++ b/backend/application/a2a/projector_test.go @@ -182,6 +182,13 @@ func resolvedRecord() *TaskRecord { }, FinalDecision: entity.VoteDecisionApprove, FinalReport: "# Final report", KeyEvidenceIDs: []string{"EV-2", "EV-1"}, KeyClaimIDs: []string{"CL-2", "CL-1"}, + // New-format resolutions persist the authoritative dissent record; + // the projector reads it instead of re-deriving from the votes. + Dissent: []entity.Dissent{{ + AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, + Reasoning: "too risky", EvidenceIDs: []string{"EV-1"}, + Conditions: []entity.DecisionCondition{{Statement: "require rollback plan", MustHold: true}}, + }}, }, Evidence: []*entity.EvidenceRecord{{ID: "EV-1"}, {ID: "EV-2"}}, Claims: []*entity.Claim{{ID: "CL-1"}, {ID: "CL-2"}}, @@ -245,6 +252,51 @@ func TestTaskProjector_ResolvedArtifactsAreDeterministic(t *testing.T) { if len(result.Dissent) != 1 || result.Dissent[0].Decision != "reject" { t.Fatalf("dissent = %+v", result.Dissent) } + if len(result.Dissent[0].Conditions) != 1 || result.Dissent[0].Conditions[0] != "require rollback plan" { + t.Fatalf("dissent conditions = %+v", result.Dissent[0].Conditions) + } +} + +// A resolution's dissent must project the tri-state distinctly: null for a +// legacy "not recorded", [] for an explicit "recorded none", and a populated +// array for a recorded minority. The projector must not fabricate dissent by +// re-deriving it from the vote rows (issue #13). +func TestTaskProjector_DissentTriState(t *testing.T) { + cases := []struct { + name string + dissent []entity.Dissent + wantState string + wantNull bool + }{ + {"legacy unknown", nil, "UNKNOWN", true}, + {"recorded none", []entity.Dissent{}, "NONE", false}, + {"recorded present", []entity.Dissent{{AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, Reasoning: "risk"}}, "PRESENT", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rec := resolvedRecord() + rec.Resolution.Dissent = tc.dissent + proj := NewTaskProjector(redact.New("sk-secret")) + artifacts := proj.Project(rec, 1, true).Artifacts + jsonText, _ := json.Marshal(artifacts[1].Parts[0].Data()) + var probe struct { + DissentState string `json:"dissentState"` + Dissent json.RawMessage `json:"dissent"` + } + if err := json.Unmarshal(jsonText, &probe); err != nil { + t.Fatal(err) + } + if probe.DissentState != tc.wantState { + t.Fatalf("dissentState = %q, want %q", probe.DissentState, tc.wantState) + } + if tc.wantNull && string(probe.Dissent) != "null" { + t.Fatalf("legacy dissent JSON = %s, want null", probe.Dissent) + } + if !tc.wantNull && string(probe.Dissent) == "null" { + t.Fatalf("recorded dissent JSON = null, want an array") + } + }) + } } func TestTaskProjector_ArtifactsRespectAcceptedOutputModes(t *testing.T) { diff --git a/backend/bootstrap/migration_parity_test.go b/backend/bootstrap/migration_parity_test.go index 4e3a1a38..33d634e2 100644 --- a/backend/bootstrap/migration_parity_test.go +++ b/backend/bootstrap/migration_parity_test.go @@ -272,3 +272,29 @@ func TestS21MigrationMatchesModelWidths(t *testing.T) { } } } + +const s30MigrationPath = "../../docker/atlas/migrations/magi_s30_resolution_dissent.sql" + +// Issue #13 (CAS-8): the authoritative minority record is persisted on the +// resolution. The Atlas migration and the GORM model must agree, or an offline +// upgrade would silently drop structured dissent. +func TestS30ResolutionDissentMigrationMatchesModel(t *testing.T) { + raw, err := os.ReadFile(s30MigrationPath) + if err != nil { + t.Fatalf("read s30 migration: %v", err) + } + sql := string(raw) + if regexp.MustCompile(`(?i)ALTER\s+TABLE\s+resolution[^;]*ADD\s+COLUMN\s+dissent_json\s+TEXT\s+NULL`).FindString(sql) == "" { + t.Fatalf("S30 must add a nullable dissent_json TEXT column to resolution:\n%s", sql) + } + field, ok := reflect.TypeOf(magi.ResolutionModel{}).FieldByName("DissentJSON") + if !ok { + t.Fatal("ResolutionModel is missing DissentJSON") + } + if tag := field.Tag.Get("gorm"); !strings.Contains(tag, "type:text") { + t.Fatalf("ResolutionModel.DissentJSON tag = %q, want type:text", tag) + } + if field.Type.Kind() != reflect.Ptr || field.Type.Elem().Kind() != reflect.String { + t.Fatalf("ResolutionModel.DissentJSON must be *string so NULL stays distinct from '[]': %s", field.Type) + } +} diff --git a/backend/bootstrap/mysql_migration_s30_test.go b/backend/bootstrap/mysql_migration_s30_test.go new file mode 100644 index 00000000..3f9c6318 --- /dev/null +++ b/backend/bootstrap/mysql_migration_s30_test.go @@ -0,0 +1,207 @@ +package bootstrap + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "gorm.io/gorm" + + magi "github.com/jamespud/magi/backend/adapter" + "github.com/jamespud/magi/backend/domain/entity" + "github.com/jamespud/magi/backend/domain/port" +) + +// s29ResolutionColumns are the resolution columns published before S30. The +// upgrade path must add dissent_json without touching them. +const s29ResolutionColumns = `(id, case_id, execution_generation, consensus_json, final_decision, + final_report, key_evidence_ids_json, key_claim_ids_json, vote_ids_json, evaluation_json)` + +// rollResolutionBackToS29 drops dissent_json, reproducing a database that was +// migrated up to S29 only. +func rollResolutionBackToS29(t *testing.T, db *gorm.DB) { + t.Helper() + if err := db.Exec("ALTER TABLE resolution DROP COLUMN dissent_json").Error; err != nil { + t.Fatalf("roll back resolution to S29 shape: %v", err) + } +} + +// assertS30DissentColumn checks the shape AND the tri-state contract of the +// column: TEXT, nullable, no default, so a legacy row reads as "not recorded" +// instead of an implicit "no dissent". +func assertS30DissentColumn(t *testing.T, db *gorm.DB) writerColumn { + t.Helper() + var row writerColumn + if err := db.Raw(`SELECT COLUMN_NAME AS column_name, DATA_TYPE AS data_type, COLUMN_TYPE AS column_type, + IS_NULLABLE AS is_nullable, COLUMN_DEFAULT AS column_default, + CHARACTER_MAXIMUM_LENGTH AS character_maximum_length + FROM information_schema.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'resolution' AND COLUMN_NAME = 'dissent_json'`). + Scan(&row).Error; err != nil { + t.Fatalf("read resolution.dissent_json: %v", err) + } + if row.Name == "" { + t.Fatal("resolution.dissent_json is missing") + } + if row.Type != "text" || row.Nullable != "YES" || row.Default != nil { + t.Fatalf("resolution.dissent_json shape = %+v, want nullable TEXT with no default", row) + } + return row +} + +// TestMySQLMigration_S30DissentTriState proves the S29 -> S30 upgrade preserves +// historical resolutions and that NULL, "[]" and "[...]" stay distinguishable +// through a real MySQL round trip. +func TestMySQLMigration_S30DissentTriState(t *testing.T) { + dsn := newMySQLSchema(t) + db := provideDBForTest(t, dsn) + repo := magi.NewRepository(db) + ctx := context.Background() + + rollResolutionBackToS29(t, db) + // A resolution written before S30 carries no minority record at all. + if err := db.Exec(`INSERT INTO resolution `+s29ResolutionColumns+` + VALUES (?,?,?,?,?,?,?,?,?,?)`, + "res-legacy", "legacy-case", 0, "{}", "approve", "# legacy report", "[]", "[]", "[]", "null").Error; err != nil { + t.Fatalf("seed legacy resolution: %v", err) + } + + applyMySQLScript(t, db, s30MigrationPath) + assertS30DissentColumn(t, db) + + legacy, err := repo.ResolutionRepo().Get(ctx, "legacy-case") + if err != nil || legacy == nil { + t.Fatalf("legacy resolution lost: %+v err=%v", legacy, err) + } + if legacy.FinalReport != "# legacy report" { + t.Fatalf("legacy report changed by migration: %q", legacy.FinalReport) + } + if legacy.Dissent != nil || legacy.DissentState() != entity.DissentUnknown { + t.Fatalf("legacy resolution must read as UNKNOWN, got dissent=%+v state=%s", legacy.Dissent, legacy.DissentState()) + } + + // An explicit "recorded no dissent" is a non-nil empty slice. + none := &entity.Resolution{ID: "res-none", CaseID: "case-none", FinalDecision: entity.VoteDecisionApprove, + FinalReport: "# none", Dissent: []entity.Dissent{}} + if err := repo.ResolutionRepo().Create(ctx, none); err != nil { + t.Fatalf("write recorded-none: %v", err) + } + gotNone, err := repo.ResolutionRepo().Get(ctx, "case-none") + if err != nil || gotNone == nil { + t.Fatalf("read recorded-none: %+v err=%v", gotNone, err) + } + if gotNone.Dissent == nil || gotNone.DissentState() != entity.DissentNone { + t.Fatalf("recorded-none collapsed: dissent=%+v state=%s", gotNone.Dissent, gotNone.DissentState()) + } + + // A recorded minority survives with its conditions. + present := &entity.Resolution{ID: "res-present", CaseID: "case-present", FinalDecision: entity.VoteDecisionApprove, + FinalReport: "# present", + Dissent: []entity.Dissent{{ + AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, + Reasoning: "unproven worst case", EvidenceIDs: []string{"EV-1"}, + Conditions: []entity.DecisionCondition{{Statement: "require rollback plan", MustHold: true}}, + }}} + if err := repo.ResolutionRepo().Create(ctx, present); err != nil { + t.Fatalf("write recorded-minority: %v", err) + } + gotPresent, err := repo.ResolutionRepo().Get(ctx, "case-present") + if err != nil || gotPresent == nil || gotPresent.DissentState() != entity.DissentPresent { + t.Fatalf("recorded-minority round trip: %+v err=%v", gotPresent, err) + } + if len(gotPresent.Dissent) != 1 || gotPresent.Dissent[0].AgentCode != entity.MagiCodeCasper || + gotPresent.Dissent[0].Reasoning != "unproven worst case" || len(gotPresent.Dissent[0].Conditions) != 1 { + t.Fatalf("recorded-minority content lost: %+v", gotPresent.Dissent) + } + + // A restart must not reinterpret any of the three states. + provideDBForTest(t, dsn) + for caseID, want := range map[string]entity.DissentState{ + "legacy-case": entity.DissentUnknown, "case-none": entity.DissentNone, "case-present": entity.DissentPresent, + } { + res, err := repo.ResolutionRepo().Get(ctx, caseID) + if err != nil || res == nil || res.DissentState() != want { + t.Fatalf("after restart %s: state=%v want=%v err=%v", caseID, res.DissentState(), want, err) + } + } +} + +// TestMySQLMigration_S30FreshInitMatchesMigration proves a brand-new database +// (AutoMigrate) and an upgraded one (S30 script) agree on the column contract. +func TestMySQLMigration_S30FreshInitMatchesMigration(t *testing.T) { + fresh := provideDBForTest(t, newMySQLSchema(t)) + migrated := provideDBForTest(t, newMySQLSchema(t)) + rollResolutionBackToS29(t, migrated) + applyMySQLScript(t, migrated, s30MigrationPath) + + freshCol := assertS30DissentColumn(t, fresh) + migratedCol := assertS30DissentColumn(t, migrated) + if freshCol.FullType != migratedCol.FullType || freshCol.Nullable != migratedCol.Nullable { + t.Fatalf("fresh %+v != migrated %+v", freshCol, migratedCol) + } +} + +// TestMySQLDecisionWriter_TerminalFailureLeavesNoResolution proves the terminal +// transaction never leaves a partial authoritative Resolution (and therefore no +// half-committed FinalReport or Dissent) when the commit fails. +func TestMySQLDecisionWriter_TerminalFailureLeavesNoResolution(t *testing.T) { + _, db := t6SchemaFixture(t) + repo := magi.NewDecisionWriterRepository(db) + ctx := context.Background() + + c := &entity.DecisionCase{ID: uuid.NewString(), Status: entity.CaseStatusDraft} + if err := repo.CaseRepo().Create(ctx, c); err != nil { + t.Fatalf("create case: %v", err) + } + jobs := magi.NewDecisionJobRepository(db) + job, admitted, err := jobs.Admit(ctx, c.ID, 3, 0) + if err != nil || !admitted { + t.Fatalf("admit: job=%+v admitted=%v err=%v", job, admitted, err) + } + claimed, ok, err := jobs.Claim(ctx, job.ID, "worker-s30", uuid.NewString(), time.Now().Add(time.Minute)) + if err != nil || !ok || claimed == nil { + t.Fatalf("claim: job=%+v ok=%v err=%v", claimed, ok, err) + } + owner := &entity.ExecutionContext{CaseID: c.ID, JobID: job.ID, WorkerID: "worker-s30", ExecutionGeneration: claimed.ExecutionGeneration} + owned := repo.(port.OwnedCaseCommitter) + + statusEvent := entity.NewEvent(c.ID, "", nil, entity.EventCaseStatusChanged, nil) + statusEvent.ExecutionGeneration = owner.ExecutionGeneration + if ok, err := owned.CommitStatusTransitionOwned(ctx, owner, []entity.CaseStatus{entity.CaseStatusDraft}, entity.CaseStatusResolving, &statusEvent); err != nil || !ok { + t.Fatalf("advance to resolving: ok=%v err=%v", ok, err) + } + + // The resolutions cites an evidence row that does not exist in this + // generation, so the in-transaction reference check rejects the commit + // after the case row is locked. + res := &entity.Resolution{ + ID: uuid.NewString(), CaseID: c.ID, ExecutionGeneration: owner.ExecutionGeneration, + FinalDecision: entity.VoteDecisionApprove, FinalReport: "# half-committed report", + KeyEvidenceIDs: []string{"does-not-exist-EV-001"}, + Dissent: []entity.Dissent{{ + AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, Reasoning: "risk", + }}, + } + doneEvent := entity.NewEvent(c.ID, "", nil, entity.EventCaseCompleted, nil) + doneEvent.ExecutionGeneration = owner.ExecutionGeneration + committed, err := owned.CommitTerminalOwned(ctx, owner, entity.CaseStatusResolving, entity.CaseStatusResolved, res, &doneEvent) + if err == nil || committed { + t.Fatalf("terminal commit with an unknown citation must fail: committed=%v err=%v", committed, err) + } + + if got, _ := repo.ResolutionRepo().Get(ctx, c.ID); got != nil { + t.Fatalf("partial resolution persisted: %+v", got) + } + var count int64 + if err := db.Table("resolution").Where("case_id = ?", c.ID).Count(&count).Error; err != nil { + t.Fatal(err) + } + if count != 0 { + t.Fatalf("resolution rows for failed commit = %d, want 0", count) + } + fresh, _ := repo.CaseRepo().Get(ctx, c.ID) + if fresh == nil || fresh.Status != entity.CaseStatusResolving { + t.Fatalf("case status must stay RESOLVING after rollback, got %+v", fresh) + } +} diff --git a/backend/domain/entity/idmap.go b/backend/domain/entity/idmap.go index 65c6e2a2..6bf3dfd1 100644 --- a/backend/domain/entity/idmap.go +++ b/backend/domain/entity/idmap.go @@ -1,6 +1,9 @@ package entity -import "strings" +import ( + "strconv" + "strings" +) // ArtifactRemap maps in-memory ledger IDs (EV-001/CL-001) to the namespaced // IDs persisted for one dispatch round ("-"). It is the single @@ -66,20 +69,44 @@ func (m *ArtifactRemap) RemapList(ids []string) []string { // RemapText replaces every known in-memory ID inside a text blob with its // persisted form. Used for the final report body, whose citations are // free-form text rather than structured fields. +// +// It is idempotent: a namespaced ID contains its bare in-memory form as a +// suffix ("--...-EV-001" contains "EV-001"), so a naive replace +// would rewrite an already-remapped citation into a doubled prefix. Every +// persisted value is shielded first, the bare forms are rewritten, and the +// shields are restored. func (m *ArtifactRemap) RemapText(s string) string { if m == nil || s == "" { return s } - for old, new := range m.evidence { - if old != "" { - s = strings.ReplaceAll(s, old, new) + type shield struct{ token, value string } + var shields []shield + guard := func(new string) { + if new == "" || !strings.Contains(s, new) { + return } + token := "\x00" + strconv.Itoa(len(shields)) + "\x00" + s = strings.ReplaceAll(s, new, token) + shields = append(shields, shield{token: token, value: new}) } - for old, new := range m.claims { - if old != "" { - s = strings.ReplaceAll(s, old, new) + for _, new := range m.evidence { + guard(new) + } + for _, new := range m.claims { + guard(new) + } + replace := func(pairs map[string]string) { + for old, new := range pairs { + if old != "" { + s = strings.ReplaceAll(s, old, new) + } } } + replace(m.evidence) + replace(m.claims) + for _, sh := range shields { + s = strings.ReplaceAll(s, sh.token, sh.value) + } return s } diff --git a/backend/domain/entity/idmap_test.go b/backend/domain/entity/idmap_test.go index 190c6d9e..ae052118 100644 --- a/backend/domain/entity/idmap_test.go +++ b/backend/domain/entity/idmap_test.go @@ -21,6 +21,27 @@ func TestArtifactRemap_RemapListAndText(t *testing.T) { } } +// A namespaced ID contains its bare form as a suffix, so RemapText must be +// idempotent: re-running it (or seeing an already-remapped citation alongside a +// bare one) must not produce a doubled prefix. +func TestArtifactRemap_RemapTextIsIdempotent(t *testing.T) { + m := NewArtifactRemap() + m.AddEvidence("EV-001", "case-abc-melchior-r1-investigate-EV-001") + namespaced := "case-abc-melchior-r1-investigate-EV-001" + oncemapped := m.RemapText(namespaced) + if oncemapped != namespaced { + t.Fatalf("already-namespaced text was rewritten: %s", oncemapped) + } + mixed := m.RemapText("see " + namespaced + " and EV-001") + want := "see " + namespaced + " and " + namespaced + if mixed != want { + t.Fatalf("mixed RemapText:\n got %s\nwant %s", mixed, want) + } + if twice := m.RemapText(mixed); twice != mixed { + t.Fatalf("second RemapText changed text: %s", twice) + } +} + func TestArtifactRemap_Merge(t *testing.T) { a := NewArtifactRemap() a.AddEvidence("EV-001", "p1-EV-001") diff --git a/backend/domain/entity/report.go b/backend/domain/entity/report.go index 6fd2a53d..fa58b395 100644 --- a/backend/domain/entity/report.go +++ b/backend/domain/entity/report.go @@ -1,9 +1,11 @@ package entity // FinalReportData is the Commander's structured, schema-validated output for -// the final decision report (design §9). RenderReport turns it into markdown. +// the final decision report (design §9). It carries only natural-language +// explanation plus citations: the authoritative Decision and the minority +// positions are bound deterministically from the resolution, never authored by +// the report model (invariant REPORT-01, issue #13). type FinalReportData struct { - Decision string `json:"decision"` Summary string `json:"summary"` KeyReasons []string `json:"key_reasons"` Risks []string `json:"risks"` diff --git a/backend/domain/entity/resolution.go b/backend/domain/entity/resolution.go index f6a1d1a2..d8f701f8 100644 --- a/backend/domain/entity/resolution.go +++ b/backend/domain/entity/resolution.go @@ -12,9 +12,58 @@ type Resolution struct { FinalReport string KeyEvidenceIDs []string KeyClaimIDs []string - VoteIDs []string - Evaluation *Evaluation - CreatedAt time.Time + // Dissent is the authoritative structured minority record derived from the + // frozen round's ballots, persisted with the resolution so the report, + // HTTP/A2A and export projections cannot independently re-invent (or hide) + // minority positions (invariant REPORT-01, issue #13). + // + // Its nil-ness is the tri-state (see DissentState): nil means the record was + // never persisted (legacy), an empty non-nil slice means an explicit + // "no dissent", and a non-empty slice means minority positions exist. + Dissent []Dissent + VoteIDs []string + Evaluation *Evaluation + CreatedAt time.Time +} + +// DissentState classifies whether minority positions were recorded for a +// resolution. "No record" is not the same fact as "recorded that nobody +// dissented": a truthfulness contract must never present the former as the +// latter (invariant REPORT-01, issue #13). +type DissentState string + +const ( + // DissentUnknown means the resolution predates structured dissent + // persistence (or its record was never committed). Whether anyone dissented + // cannot be proven from the stored data. + DissentUnknown DissentState = "UNKNOWN" + // DissentNone means the resolution explicitly recorded that no participant + // dissented. + DissentNone DissentState = "NONE" + // DissentPresent means one or more minority positions were recorded. + DissentPresent DissentState = "PRESENT" +) + +// DissentStateOf classifies a dissent record by the nil-ness of its slice: nil +// means "never persisted", an empty non-nil slice means "explicitly no +// dissent", and a non-empty slice means "minority positions recorded". +func DissentStateOf(d []Dissent) DissentState { + switch { + case d == nil: + return DissentUnknown + case len(d) == 0: + return DissentNone + default: + return DissentPresent + } +} + +// DissentState reports the persisted minority state of the resolution. +func (r *Resolution) DissentState() DissentState { + if r == nil { + return DissentUnknown + } + return DissentStateOf(r.Dissent) } // Dissent is a structured minority position recorded for auditability. diff --git a/backend/domain/orchestration/dissent_binding_test.go b/backend/domain/orchestration/dissent_binding_test.go new file mode 100644 index 00000000..6154c283 --- /dev/null +++ b/backend/domain/orchestration/dissent_binding_test.go @@ -0,0 +1,180 @@ +package orchestration_test + +import ( + "context" + "strings" + "testing" + + magiapp "github.com/jamespud/magi/backend/adapter" + "github.com/jamespud/magi/backend/domain/consensus" + "github.com/jamespud/magi/backend/domain/debate" + "github.com/jamespud/magi/backend/domain/entity" + "github.com/jamespud/magi/backend/domain/orchestration" + "github.com/jamespud/magi/backend/domain/port" + "gorm.io/driver/sqlite" + "gorm.io/gorm" +) + +// dissentCase builds an orchestrator backed by a real sqlite repository so the +// persisted resolution and the in-memory result can be compared. +func dissentCase(t *testing.T, votes map[string][]*entity.Vote) (orchestration.OrchestratorDeps, port.Repository, *gorm.DB, *mockMagiRuntime) { + t.Helper() + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + if err := db.AutoMigrate(magiapp.AllModels()...); err != nil { + t.Fatalf("automigrate: %v", err) + } + repo := magiapp.NewRepository(db) + mrt := newMockMagiRuntime() + mrt.votes = votes + deps := orchestration.OrchestratorDeps{ + AgentLoop: mrt, + Consensus: consensus.NewConsensusEngine(), + Debate: debate.NewDebateEngine(nil), + Commander: newCommander(t), + CaseRepo: repo.CaseRepo(), + Repo: repo, + EventPub: magiapp.NewEventPublisherAdapter(repo.EventRepo()), + Configs: []*entity.MagiConfig{magiCfg("melchior"), magiCfg("balthasar"), magiCfg("casper")}, + Policy: consensus.DefaultConsensusPolicy(), + } + return deps, repo, db, mrt +} + +func dissentReject() *entity.Vote { + return &entity.Vote{ + Decision: entity.VoteDecisionReject, + Confidence: 70, + ReasoningSummary: "unproven worst case", + EvidenceIDs: []string{"EV-001"}, + Conditions: []entity.DecisionCondition{{Statement: "require rollback plan", MustHold: true}}, + } +} + +// Issue #13 / CAS-8: the minority position must be bound to the resolution at +// solve time and persisted, so the report, HTTP and export all read one +// authoritative record instead of re-deriving (or dropping) dissent. +func TestOrchestrate_PersistsAuthoritativeDissent(t *testing.T) { + deps, repo, _, _ := dissentCase(t, map[string][]*entity.Vote{ + "melchior": {approve()}, + "balthasar": {approve()}, + "casper": {dissentReject()}, + }) + orch := orchestration.NewOrchestrator(deps) + ctx := context.Background() + case_ := &entity.DecisionCase{ID: "c-dissent", Question: "compute", MaxDebateRounds: 1} + if err := repo.CaseRepo().Create(ctx, case_); err != nil { + t.Fatalf("create case: %v", err) + } + + res, err := orch.Orchestrate(ctx, case_) + if err != nil { + t.Fatalf("orchestrate: %v", err) + } + if res == nil || res.Consensus.Outcome != entity.ConsensusMajorityApprovalDissent { + t.Fatalf("expected majority approval with dissent, got %+v", res) + } + if res.FinalDecision != entity.VoteDecisionApprove { + t.Fatalf("final decision = %s", res.FinalDecision) + } + if len(res.Dissent) != 1 { + t.Fatalf("want exactly one persisted dissenter, got %+v", res.Dissent) + } + d := res.Dissent[0] + if d.AgentCode != entity.MagiCodeCasper || d.Decision != entity.VoteDecisionReject { + t.Fatalf("dissent attribution: %+v", d) + } + if d.Reasoning != "unproven worst case" || len(d.Conditions) != 1 { + t.Fatalf("dissent content lost: %+v", d) + } + if !strings.Contains(res.FinalReport, "--- Dissent ---") || !strings.Contains(res.FinalReport, "casper dissented with reject") { + t.Fatalf("canonical report must embed the minority position:\n%s", res.FinalReport) + } + + // The persisted resolution is the single source: reload and compare. + got, err := repo.ResolutionRepo().Get(ctx, "c-dissent") + if err != nil || got == nil { + t.Fatalf("reload resolution: %+v err=%v", got, err) + } + if len(got.Dissent) != 1 || got.Dissent[0].AgentCode != entity.MagiCodeCasper || got.Dissent[0].Reasoning != "unproven worst case" { + t.Fatalf("persisted dissent = %+v", got.Dissent) + } + if got.FinalReport != res.FinalReport { + t.Fatalf("persisted report diverged from the returned report") + } + // Dissent evidence/claim references must resolve to persisted artifacts. + evSet := map[string]bool{} + evs, _ := repo.EvidenceRepo().ListByCase(ctx, "c-dissent") + for _, e := range evs { + evSet[e.ID] = true + } + for _, id := range got.Dissent[0].EvidenceIDs { + if !evSet[id] { + t.Fatalf("dissent cites unknown evidence %q", id) + } + } +} + +func TestOrchestrate_UnanimousHasNoDissent(t *testing.T) { + deps, repo, _, _ := dissentCase(t, map[string][]*entity.Vote{ + "melchior": {approve()}, + "balthasar": {approve()}, + "casper": {approve()}, + }) + orch := orchestration.NewOrchestrator(deps) + ctx := context.Background() + case_ := &entity.DecisionCase{ID: "c-unanimous", Question: "compute", MaxDebateRounds: 1} + if err := repo.CaseRepo().Create(ctx, case_); err != nil { + t.Fatalf("create case: %v", err) + } + + res, err := orch.Orchestrate(ctx, case_) + if err != nil { + t.Fatalf("orchestrate: %v", err) + } + if len(res.Dissent) != 0 { + t.Fatalf("a unanimous round must not fabricate dissent: %+v", res.Dissent) + } + if strings.Contains(res.FinalReport, "--- Dissent ---") { + t.Fatalf("unanimous report must not carry a dissent section:\n%s", res.FinalReport) + } +} + +// The frozen citation set handed to the report model must be the current +// execution generation's namespaced artifacts, not an arbitrary string set: +// every cited id carries this case's prefix and the run phase it was produced +// under, and the report only cites ids from that same set. +func TestOrchestrate_ReportCitationsAreGenerationScoped(t *testing.T) { + deps, repo, _, _ := dissentCase(t, map[string][]*entity.Vote{ + "melchior": {approve()}, + "balthasar": {approve()}, + "casper": {approve()}, + }) + orch := orchestration.NewOrchestrator(deps) + ctx := context.Background() + case_ := &entity.DecisionCase{ID: "c-cite", Question: "compute", MaxDebateRounds: 1} + if err := repo.CaseRepo().Create(ctx, case_); err != nil { + t.Fatalf("create case: %v", err) + } + res, err := orch.Orchestrate(ctx, case_) + if err != nil { + t.Fatalf("orchestrate: %v", err) + } + if len(res.KeyEvidenceIDs) == 0 || len(res.KeyClaimIDs) == 0 { + t.Fatalf("expected durable citations, got evidence=%v claims=%v", res.KeyEvidenceIDs, res.KeyClaimIDs) + } + allowed := map[string]bool{} + for _, id := range append(append([]string{}, res.KeyEvidenceIDs...), res.KeyClaimIDs...) { + if !strings.HasPrefix(id, "c-cite-") { + t.Fatalf("citation %q is not namespaced to this case/generation", id) + } + allowed[id] = true + } + for _, id := range findIDs(res.FinalReport) { + if !allowed[id] { + t.Fatalf("report cites %q which is not in the current generation's frozen set", id) + } + } +} diff --git a/backend/domain/orchestration/integration_test.go b/backend/domain/orchestration/integration_test.go index 55f308b4..2b84b522 100644 --- a/backend/domain/orchestration/integration_test.go +++ b/backend/domain/orchestration/integration_test.go @@ -5,7 +5,6 @@ import ( "strings" "testing" - "github.com/cloudwego/eino/schema" magiapp "github.com/jamespud/magi/backend/adapter" "github.com/jamespud/magi/backend/domain/consensus" "github.com/jamespud/magi/backend/domain/debate" @@ -46,10 +45,7 @@ func TestIntegration_OrchestratorWithDB(t *testing.T) { gen := validation.NewReflectSchemaGenerator() val := validation.NewJSONSchemaValidator() - cmdModel := &scriptedChatModel{responses: []*schema.Message{ - schema.AssistantMessage(`{"canonical_question":"compute"}`, nil), - schema.AssistantMessage(`{"decision":"approve","summary":"decision report from db test","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), - }} + cmdModel := &citingCommanderModel{} cmd, err := service.NewCommander(service.CommanderConfig{Model: entity.ModelRef{ModelID: 1}, Persona: "commander"}, &stubModelPort{m: cmdModel}, gen, val) if err != nil { t.Fatalf("commander: %v", err) diff --git a/backend/domain/orchestration/orchestrator.go b/backend/domain/orchestration/orchestrator.go index f4d7a5d4..b4c5edbf 100644 --- a/backend/domain/orchestration/orchestrator.go +++ b/backend/domain/orchestration/orchestrator.go @@ -532,6 +532,20 @@ func (o *Orchestrator) collectBallots(results []*runtime.LoopResult) ([]*entity. return votes, absences } +// extractDissent binds the current round's minority ballots to their agents. +// Votes and results stay index-aligned with the config slice, so an agent code +// is read from the same position that produced the ballot. A nil vote slot is a +// missing ballot, never a dissent. +func (o *Orchestrator) extractDissent(st *State) []entity.Dissent { + codes := make([]entity.MagiCode, len(st.Votes)) + for i := range st.Votes { + if st.Votes[i] != nil { + codes[i] = codeOf(o.configAt(i)) + } + } + return service.ExtractDissent(st.Resolution.FinalDecision, st.Votes, codes) +} + // absencesMsg renders the non-votes for a case-failure message. func absencesMsg(absences []entity.AgentAbsence) string { parts := make([]string, 0, len(absences)) diff --git a/backend/domain/orchestration/orchestrator_test.go b/backend/domain/orchestration/orchestrator_test.go index 4161d1dc..d979c678 100644 --- a/backend/domain/orchestration/orchestrator_test.go +++ b/backend/domain/orchestration/orchestrator_test.go @@ -58,6 +58,80 @@ func (s *scriptedChatModel) WithTools(tools []*schema.ToolInfo) (model.ToolCalli return s, nil } +// citingCommanderModel answers the normalize call with a canned task and the +// report call with a report that cites the first authoritative ID offered in +// the prompt. The strict citation validator (issue #13) requires the report to +// reference the frozen set, whose namespaced IDs a scripted model cannot know +// ahead of time. +type citingCommanderModel struct { + mu sync.Mutex + calls int +} + +var _ model.ToolCallingChatModel = (*citingCommanderModel)(nil) + +func (m *citingCommanderModel) Generate(ctx context.Context, in []*schema.Message, opts ...model.Option) (*schema.Message, error) { + m.mu.Lock() + m.calls++ + m.mu.Unlock() + var text strings.Builder + for _, msg := range in { + if msg != nil { + text.WriteString(msg.Content) + text.WriteString("\n") + } + } + if strings.Contains(text.String(), "FinalReportData") { + return schema.AssistantMessage(citingReportJSON(text.String()), nil), nil + } + return schema.AssistantMessage(`{"canonical_question":"compute"}`, nil), nil +} + +func (m *citingCommanderModel) Stream(ctx context.Context, in []*schema.Message, opts ...model.Option) (*schema.StreamReader[*schema.Message], error) { + return nil, fmt.Errorf("not implemented") +} + +func (m *citingCommanderModel) WithTools(tools []*schema.ToolInfo) (model.ToolCallingChatModel, error) { + return m, nil +} + +// citingReportJSON emits a valid FinalReportData, citing the first evidence ID +// in the prompt's authoritative list when one is offered. +func citingReportJSON(prompt string) string { + body := map[string]any{ + "summary": "decision report summary", + "key_reasons": []string{"r1"}, + "risks": []string{}, + "next_steps": []string{}, + } + if id := firstListedID(prompt, "Available evidence IDs: ["); id != "" { + body["key_evidence_ids"] = []string{id} + } else if id := firstListedID(prompt, "Available claim IDs: ["); id != "" { + body["key_claim_ids"] = []string{id} + } + raw, _ := json.Marshal(body) + return string(raw) +} + +// firstListedID returns the first whitespace-separated token inside the list +// that follows marker, or "" when the marker is absent or the list is empty. +func firstListedID(prompt, marker string) string { + i := strings.Index(prompt, marker) + if i < 0 { + return "" + } + rest := prompt[i+len(marker):] + j := strings.IndexByte(rest, ']') + if j < 0 { + return "" + } + fields := strings.Fields(rest[:j]) + if len(fields) == 0 { + return "" + } + return fields[0] +} + type stubModelPort struct{ m model.ToolCallingChatModel } func (s *stubModelPort) Build(ctx context.Context, ref entity.ModelRef) (model.ToolCallingChatModel, error) { @@ -155,10 +229,7 @@ func newCommanderWith(t *testing.T, cycle bool) *service.Commander { t.Helper() gen := validation.NewReflectSchemaGenerator() val := validation.NewJSONSchemaValidator() - cm := &scriptedChatModel{responses: []*schema.Message{ - schema.AssistantMessage(`{"canonical_question":"compute"}`, nil), - schema.AssistantMessage(`{"decision":"approve","summary":"decision report summary","key_reasons":["r1"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), - }, cycle: cycle} + cm := &citingCommanderModel{} cmd, err := service.NewCommander(service.CommanderConfig{Model: entity.ModelRef{ModelID: 1}, Persona: "commander"}, &stubModelPort{m: cm}, gen, val) if err != nil { t.Fatalf("commander: %v", err) diff --git a/backend/domain/orchestration/steps.go b/backend/domain/orchestration/steps.go index d5248fe5..6c9294ec 100644 --- a/backend/domain/orchestration/steps.go +++ b/backend/domain/orchestration/steps.go @@ -262,19 +262,42 @@ func (o *Orchestrator) stepResolve(ctx context.Context, case_ *entity.DecisionCa res.CaseID = case_.ID res.ExecutionGeneration = case_.ExecutionGeneration res.CreatedAt = time.Now() + // Bind the frozen round's minority positions now, from the same ballots the + // consensus was computed over. Storing them on the resolution means the + // report, HTTP/A2A and export read one authoritative record instead of + // re-deriving dissent from mutable vote joins (issue #13). st.Resolution = res + res.Dissent = o.extractDissent(st) return entity.CaseStatusGeneratingReport, false, nil } func (o *Orchestrator) stepGenerateReport(ctx context.Context, case_ *entity.DecisionCase, st *State) (entity.CaseStatus, bool, error) { + // Provenance of the frozen citation set handed to the report model: + // st.Resolution.KeyEvidenceIDs/KeyClaimIDs were built in stepResolve from + // THIS round's ledgers and rewritten through the round's ArtifactRemap, so + // every ID carries the current execution generation's run prefix + // ("--g-r--"). They are the same durable + // rows the terminal committer re-verifies (generation-scoped, all present) + // before it publishes the resolution, so the report can never be published + // citing a different generation's artifact. report, err := o.commander.GenerateReport(ctx, case_, st.Resolution, st.Votes, st.Resolution.KeyEvidenceIDs, st.Resolution.KeyClaimIDs) if err != nil { return "", false, fmt.Errorf("report generation: %w", err) } - // The report body cites IDs as free text; rewrite them to the persisted - // namespaced forms so citations resolve in the DB. - st.Resolution.FinalReport = mergeRemaps(st.AllRemaps).RemapText(report) + // The report is validated against the frozen namespaced ID set before it is + // returned, so its structured citations already resolve in the DB. RemapText + // still rewrites any bare IDs that appear in the model's prose; it is + // idempotent so an already-namespaced citation is not doubled into + // "...-EV-001" twice. + report = mergeRemaps(st.AllRemaps).RemapText(report) + // Persist the deterministic dissent section inside the canonical report so + // every read path (HTTP, A2A, export) returns the same bytes and none of + // them can silently drop minority positions by skipping a re-derivation. + if section := service.RenderDissent(st.Resolution.Dissent); section != "" { + report += "\n\n--- Dissent ---\n" + section + } + st.Resolution.FinalReport = report o.publish(ctx, case_, entity.EventResolutionCreated, map[string]any{"final_decision": string(st.Resolution.FinalDecision)}) return entity.CaseStatusSavingMemory, false, nil } diff --git a/backend/domain/prompt/defaults.go b/backend/domain/prompt/defaults.go index 618f8320..c4926977 100644 --- a/backend/domain/prompt/defaults.go +++ b/backend/domain/prompt/defaults.go @@ -35,7 +35,6 @@ Constraints: {{CONSTRAINTS}}`, KeyCommanderReport: `{{PERSONA}} Generate a FinalReportData JSON for this decision. You MUST include ALL fields: -- decision: the final decision (approve/reject/conditional_approve) - summary: a one-paragraph summary of the decision - key_reasons: main reasoning points (array of strings) - risks: risks or dissenting points (array of strings) @@ -46,7 +45,7 @@ Generate a FinalReportData JSON for this decision. You MUST include ALL fields: Question: {{QUESTION}}. Consensus: {{CONSENSUS}}. Votes: {{VOTES}}. Available evidence IDs: {{EVIDENCE_IDS}} Available claim IDs: {{CLAIM_IDS}} -When evidence or claim IDs are available you MUST cite at least one of them in key_evidence_ids/key_claim_ids.`, +When evidence or claim IDs are available you MUST cite at least one of them in key_evidence_ids/key_claim_ids, using ONLY the IDs listed above. Do not state the final decision yourself; the deterministic resolver supplies it.`, KeyAgentWorkflowTools: `Workflow: gather evidence via tool calls; limit yourself to AT MOST 3 tool calls. Once you have gathered enough evidence, STOP calling tools and output an EvidenceSummary JSON (no tool calls) citing real EV-IDs; after the gate passes, output a Vote JSON. Do not keep searching past 3 calls -- converge to a decision. You may also submit claims incrementally during the gather phase: diff --git a/backend/domain/service/commander.go b/backend/domain/service/commander.go index a71c7983..109daf4c 100644 --- a/backend/domain/service/commander.go +++ b/backend/domain/service/commander.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "sort" "github.com/cloudwego/eino/schema" @@ -92,6 +93,17 @@ func (c *Commander) Normalize(ctx context.Context, case_ *entity.DecisionCase) ( return nil, fmt.Errorf("commander: normalize failed after retries") } +// GenerateReport produces the validated final report body. +// +// Boundary (issue #13): this enforces the STRUCTURED report contract — +// the authoritative decision is bound from the resolution (never model-authored) +// and every cited ID must belong to the frozen, current-generation artifact set +// passed in by the caller (validateReportCitations). It does NOT semantically +// verify free-text claims about authorization, sending or verification inside +// Summary/KeyReasons/Risks/NextSteps: those authority and execution states must +// eventually be rendered from a deterministic template under the six-dimension +// state contract, which is owned by Epic #7, not this fix. Until then this +// function must not be described as a complete REPORT-01 implementation. func (c *Commander) GenerateReport(ctx context.Context, case_ *entity.DecisionCase, resolution *entity.Resolution, votes []*entity.Vote, evidenceIDs, claimIDs []string) (string, error) { cm, err := c.model.Build(ctx, c.cfg.Model) if err != nil { @@ -117,8 +129,8 @@ func (c *Commander) GenerateReport(ctx context.Context, case_ *entity.DecisionCa msgs := []*schema.Message{schema.SystemMessage(prompt)} if attempt > 0 { msgs = append(msgs, schema.UserMessage( - "Your previous response did not pass validation or did not cite any available evidence/claim ID. "+ - "Re-output the FinalReportData JSON now, citing at least one of the provided IDs in key_evidence_ids/key_claim_ids.")) + "Your previous response did not pass validation, cited no available ID, or cited an ID that is not in the provided set. "+ + "Re-output the FinalReportData JSON now, citing at least one of the provided IDs in key_evidence_ids/key_claim_ids and citing only IDs from that set.")) } msgs = append(msgs, schema.UserMessage("Output the FinalReportData JSON now.")) resp, err := cm.Generate(ctx, msgs) @@ -127,27 +139,68 @@ func (c *Commander) GenerateReport(ctx context.Context, case_ *entity.DecisionCa continue } data, vr := c.reportVal.ValidateAndUnmarshal([]byte(resp.Content)) - if vr != nil && vr.Valid && reportCites(data, evidenceIDs, claimIDs) { - return RenderReport(data), nil - } - if vr != nil { + if vr != nil && vr.Valid { + if err := validateReportCitations(data, evidenceIDs, claimIDs); err == nil { + return RenderReport(resolution.FinalDecision, data), nil + } else { + log.Printf("commander: report attempt %d: citation rejected: %v", attempt+1, err) + } + } else if vr != nil { log.Printf("commander: report attempt %d: validation failed with %d violations", attempt+1, len(vr.Violations)) for i, v := range vr.Violations { log.Printf(" violation %d: [%s] %s field=%s", i+1, v.Code, v.Message, v.Field) } - } else if data != nil && !reportCites(data, evidenceIDs, claimIDs) { - log.Printf("commander: report attempt %d: no evidence/claim citation", attempt+1) } } return "", fmt.Errorf("commander: report generation failed after retries") } -func reportCites(data *entity.FinalReportData, evidenceIDs, claimIDs []string) bool { +// validateReportCitations enforces that every ID the report cites belongs to +// the frozen authoritative set handed to the model, that duplicates are not +// laundered into a longer citation list, and that a report cannot silently omit +// citations when authoritative material exists. Empty/unknown/duplicate +// references are rejected so the caller retries instead of persisting a report +// that claims evidence MAGI cannot prove (invariants REPORT-01, EVID-02). +func validateReportCitations(data *entity.FinalReportData, evidenceIDs, claimIDs []string) error { if data == nil { - return false + return fmt.Errorf("report data is nil") } - if len(evidenceIDs)+len(claimIDs) == 0 { - return true + allowed := make(map[string]struct{}, len(evidenceIDs)+len(claimIDs)) + for _, id := range evidenceIDs { + allowed[id] = struct{}{} + } + for _, id := range claimIDs { + allowed[id] = struct{}{} + } + cited := append(append([]string{}, data.KeyEvidenceIDs...), data.KeyClaimIDs...) + for _, id := range cited { + if id == "" { + return fmt.Errorf("report cites an empty artifact id") + } + if _, ok := allowed[id]; !ok { + return fmt.Errorf("report cites %q which is not in the authoritative set %v", id, sortedKeys(allowed)) + } + } + for name, ids := range map[string][]string{"evidence": data.KeyEvidenceIDs, "claim": data.KeyClaimIDs} { + seen := make(map[string]struct{}, len(ids)) + for _, id := range ids { + if _, dup := seen[id]; dup { + return fmt.Errorf("report cites duplicate %s id %q", name, id) + } + seen[id] = struct{}{} + } + } + if len(allowed) > 0 && len(cited) == 0 { + return fmt.Errorf("report cites no authoritative artifact while %d are available", len(allowed)) + } + return nil +} + +func sortedKeys(m map[string]struct{}) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) } - return len(data.KeyEvidenceIDs)+len(data.KeyClaimIDs) > 0 + sort.Strings(out) + return out } diff --git a/backend/domain/service/commander_test.go b/backend/domain/service/commander_test.go index 304c8f47..e771e4eb 100644 --- a/backend/domain/service/commander_test.go +++ b/backend/domain/service/commander_test.go @@ -52,10 +52,10 @@ func newReportCommander(t *testing.T, m model.ToolCallingChatModel) *service.Com func TestGenerateReport_ValidJSONRenders(t *testing.T) { m := &reportScriptedModel{responses: []*schema.Message{ - schema.AssistantMessage(`{"decision":"approve","summary":"proceed","key_reasons":["r1"],"risks":[],"next_steps":["s1"]}`, nil), + schema.AssistantMessage(`{"summary":"proceed","key_reasons":["r1"],"risks":[],"next_steps":["s1"]}`, nil), }} cmd := newReportCommander(t, m) - out, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{}, nil, nil, nil) + out, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, nil, nil) if err != nil { t.Fatalf("report: %v", err) } @@ -73,7 +73,7 @@ func TestGenerateReport_InvalidJSONRetriesAndFails(t *testing.T) { schema.AssistantMessage("{invalid", nil), }} cmd := newReportCommander(t, m) - _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{}, nil, nil, nil) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, nil, nil) if err == nil { t.Fatal("expected error after 3 invalid attempts") } @@ -81,11 +81,11 @@ func TestGenerateReport_InvalidJSONRetriesAndFails(t *testing.T) { func TestGenerateReport_RequiresEvidenceCitation(t *testing.T) { m := &reportScriptedModel{responses: []*schema.Message{ - schema.AssistantMessage(`{"decision":"approve","summary":"no citation","key_reasons":["r1"],"risks":[],"next_steps":["s1"]}`, nil), - schema.AssistantMessage(`{"decision":"approve","summary":"cited","key_reasons":["r1"],"risks":[],"next_steps":["s1"],"key_evidence_ids":["EV-001"]}`, nil), + schema.AssistantMessage(`{"summary":"no citation","key_reasons":["r1"],"risks":[],"next_steps":["s1"]}`, nil), + schema.AssistantMessage(`{"summary":"cited","key_reasons":["r1"],"risks":[],"next_steps":["s1"],"key_evidence_ids":["EV-001"]}`, nil), }} cmd := newReportCommander(t, m) - out, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{}, nil, []string{"EV-001"}, nil) + out, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, nil) if err != nil { t.Fatalf("report: %v", err) } @@ -99,12 +99,12 @@ func TestGenerateReport_RequiresEvidenceCitation(t *testing.T) { func TestGenerateReport_FailsWhenCitationNeverProvided(t *testing.T) { m := &reportScriptedModel{responses: []*schema.Message{ - schema.AssistantMessage(`{"decision":"approve","summary":"a","key_reasons":["r"],"risks":[],"next_steps":["s"]}`, nil), - schema.AssistantMessage(`{"decision":"approve","summary":"b","key_reasons":["r"],"risks":[],"next_steps":["s"]}`, nil), - schema.AssistantMessage(`{"decision":"approve","summary":"c","key_reasons":["r"],"risks":[],"next_steps":["s"]}`, nil), + schema.AssistantMessage(`{"summary":"a","key_reasons":["r"],"risks":[],"next_steps":["s"]}`, nil), + schema.AssistantMessage(`{"summary":"b","key_reasons":["r"],"risks":[],"next_steps":["s"]}`, nil), + schema.AssistantMessage(`{"summary":"c","key_reasons":["r"],"risks":[],"next_steps":["s"]}`, nil), }} cmd := newReportCommander(t, m) - _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{}, nil, []string{"EV-001"}, nil) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, nil) if err == nil { t.Fatal("expected error when report never cites available evidence") } diff --git a/backend/domain/service/commander_truthfulness_test.go b/backend/domain/service/commander_truthfulness_test.go new file mode 100644 index 00000000..1d993b02 --- /dev/null +++ b/backend/domain/service/commander_truthfulness_test.go @@ -0,0 +1,97 @@ +package service_test + +import ( + "context" + "strings" + "testing" + + "github.com/cloudwego/eino/schema" + + "github.com/jamespud/magi/backend/domain/entity" +) + +// Issue #13 / CAS-8: the report generator must not be able to cite an artifact +// that is not part of the frozen authoritative set handed to it. The old +// reportCites helper accepted any non-empty list, so a fabricated ID passed. +func TestGenerateReport_RejectsFabricatedCitation(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"summary":"forged","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-999"]}`, nil), + schema.AssistantMessage(`{"summary":"forged","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-999"]}`, nil), + schema.AssistantMessage(`{"summary":"forged","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-999"]}`, nil), + }} + cmd := newReportCommander(t, m) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, nil) + if err == nil { + t.Fatal("report citing an ID outside the authoritative set must be rejected") + } +} + +func TestGenerateReport_RetriesAfterFabricatedCitation(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"summary":"forged","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-999"]}`, nil), + schema.AssistantMessage(`{"summary":"honest","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), + }} + cmd := newReportCommander(t, m) + out, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, nil) + if err != nil { + t.Fatalf("report: %v", err) + } + if m.calls != 2 { + t.Fatalf("expected one retry after a fabricated citation, calls=%d", m.calls) + } + if strings.Contains(out, "EV-999") { + t.Fatalf("fabricated citation leaked into the report:\n%s", out) + } +} + +func TestGenerateReport_RejectsDuplicateCitation(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"summary":"dup","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001","EV-001"]}`, nil), + schema.AssistantMessage(`{"summary":"dup","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001","EV-001"]}`, nil), + schema.AssistantMessage(`{"summary":"dup","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001","EV-001"]}`, nil), + }} + cmd := newReportCommander(t, m) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, nil) + if err == nil { + t.Fatal("a duplicate citation must not be accepted as a distinct reference") + } +} + +// The authoritative decision is bound by the deterministic resolution, never by +// the report model. A model that cannot see the decision still produces the +// correct heading. +func TestGenerateReport_BindsAuthoritativeDecision(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), + }} + cmd := newReportCommander(t, m) + out, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionReject}, nil, []string{"EV-001"}, nil) + if err != nil { + t.Fatalf("report: %v", err) + } + if !strings.Contains(out, "## Decision\nreject") { + t.Fatalf("report must render the authoritative decision, got:\n%s", out) + } +} + +// The report model must not be able to author the authoritative decision. The +// structured contract no longer has a decision field, so a model that tries to +// supply one fails schema validation instead of silently overriding the +// resolver. +func TestGenerateReport_RejectsModelSuppliedDecision(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"decision":"reject","summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), + schema.AssistantMessage(`{"decision":"reject","summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), + schema.AssistantMessage(`{"decision":"reject","summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"]}`, nil), + }} + cmd := newReportCommander(t, m) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, nil) + if err == nil { + t.Fatal("a report that supplies its own decision must be rejected") + } +} diff --git a/backend/domain/service/dissent.go b/backend/domain/service/dissent.go index fa030aef..8a21d9f1 100644 --- a/backend/domain/service/dissent.go +++ b/backend/domain/service/dissent.go @@ -3,19 +3,32 @@ package service import "github.com/jamespud/magi/backend/domain/entity" // ExtractDissent returns the structured minority positions for a final -// decision. Abstentions are not dissent; a deadlock/insufficient-quorum has no -// majority to dissent from, so an empty list is returned. -func ExtractDissent(final entity.VoteDecision, votes []*entity.Vote, codeByRun map[string]entity.MagiCode) []entity.Dissent { +// decision. codes is index-aligned with votes: a caller that holds the ballots +// next to the agent configs (the orchestrator does) passes the code that +// produced each ballot, so attribution never depends on a persisted run id or +// on a join that a legacy row may lack. An abstention is not dissent, and a +// missing ballot slot (nil) is skipped rather than fabricated. +// +// The nil-ness of the result is part of the contract: a complete round with no +// dissent returns a non-nil empty slice ("recorded none"), while an +// undecidable round (no final decision or no ballots) returns nil ("not +// recorded"). Callers persist the distinction so a legacy "no record" is never +// presented as "no dissent". +func ExtractDissent(final entity.VoteDecision, votes []*entity.Vote, codes []entity.MagiCode) []entity.Dissent { if final == "" || len(votes) == 0 { return nil } - var out []entity.Dissent - for _, v := range votes { + out := []entity.Dissent{} + for i, v := range votes { if v == nil || v.Decision == final || v.Decision == entity.VoteDecisionAbstain { continue } + var code entity.MagiCode + if i < len(codes) { + code = codes[i] + } out = append(out, entity.Dissent{ - AgentCode: codeByRun[v.AgentRunID], + AgentCode: code, Decision: v.Decision, Reasoning: v.ReasoningSummary, EvidenceIDs: v.EvidenceIDs, diff --git a/backend/domain/service/dissent_test.go b/backend/domain/service/dissent_test.go index f83f96fa..3022cc4f 100644 --- a/backend/domain/service/dissent_test.go +++ b/backend/domain/service/dissent_test.go @@ -13,7 +13,7 @@ func TestExtractDissent_ReturnsMinorityPositions(t *testing.T) { {AgentRunID: "r-bal", Decision: entity.VoteDecisionReject, ReasoningSummary: "too risky", EvidenceIDs: []string{"EV-2"}, Conditions: []entity.DecisionCondition{{Statement: "must rollback", MustHold: true}}}, {AgentRunID: "r-cas", Decision: entity.VoteDecisionAbstain, ReasoningSummary: "unsure"}, } - codes := map[string]entity.MagiCode{"r-mel": "melchior", "r-bal": "balthasar", "r-cas": "casper"} + codes := []entity.MagiCode{"melchior", "balthasar", "casper"} out := service.ExtractDissent(entity.VoteDecisionApprove, votes, codes) if len(out) != 1 { t.Fatalf("want 1 dissenter, got %d", len(out)) @@ -27,19 +27,51 @@ func TestExtractDissent_ReturnsMinorityPositions(t *testing.T) { } } +// A complete unanimous round records an explicit "no dissent": the result is a +// non-nil empty slice so persistence can distinguish it from "never recorded". func TestExtractDissent_EmptyForUnanimous(t *testing.T) { votes := []*entity.Vote{{AgentRunID: "r1", Decision: entity.VoteDecisionApprove}} - if out := service.ExtractDissent(entity.VoteDecisionApprove, votes, map[string]entity.MagiCode{"r1": "melchior"}); len(out) != 0 { + out := service.ExtractDissent(entity.VoteDecisionApprove, votes, []entity.MagiCode{"melchior"}) + if len(out) != 0 { t.Fatalf("want no dissent, got %+v", out) } + if out == nil { + t.Fatal("a complete unanimous round must record NONE (non-nil empty), not UNKNOWN (nil)") + } + if entity.DissentStateOf(out) != entity.DissentNone { + t.Fatalf("state = %s, want NONE", entity.DissentStateOf(out)) + } +} + +// An undecidable round (no authoritative decision) has no minority record at +// all: nil, which callers persist as UNKNOWN. +func TestExtractDissent_UnknownWhenNoFinalDecision(t *testing.T) { + votes := []*entity.Vote{{Decision: entity.VoteDecisionReject}} + if out := service.ExtractDissent("", votes, []entity.MagiCode{"casper"}); out != nil { + t.Fatalf("no final decision must yield an unrecorded (nil) dissent, got %+v", out) + } +} + +func TestExtractDissent_SkipsAbstainAndMissingSlot(t *testing.T) { + votes := []*entity.Vote{ + nil, // a participant that produced no ballot is not a dissenter + {Decision: entity.VoteDecisionAbstain}, + {Decision: entity.VoteDecisionApprove}, + } + if out := service.ExtractDissent(entity.VoteDecisionApprove, votes, []entity.MagiCode{"melchior", "balthasar", "casper"}); len(out) != 0 { + t.Fatalf("abstain and missing ballots are not dissent: %+v", out) + } } -func TestExtractDissent_SkipsAbstain(t *testing.T) { +// Attribution follows ballot position, not a persisted run id, so a repo-less +// run (where AgentRunID is empty on every ballot) still names the right agent. +func TestExtractDissent_AttributionByPosition(t *testing.T) { votes := []*entity.Vote{ - {AgentRunID: "r1", Decision: entity.VoteDecisionAbstain}, - {AgentRunID: "r2", Decision: entity.VoteDecisionApprove}, + {Decision: entity.VoteDecisionApprove}, + {Decision: entity.VoteDecisionReject, ReasoningSummary: "risk"}, } - if out := service.ExtractDissent(entity.VoteDecisionApprove, votes, nil); len(out) != 0 { - t.Fatalf("abstain is not dissent: %+v", out) + out := service.ExtractDissent(entity.VoteDecisionApprove, votes, []entity.MagiCode{"melchior", "balthasar"}) + if len(out) != 1 || out[0].AgentCode != "balthasar" { + t.Fatalf("want balthasar dissent, got %+v", out) } } diff --git a/backend/domain/service/report.go b/backend/domain/service/report.go index d7466eb6..addc179a 100644 --- a/backend/domain/service/report.go +++ b/backend/domain/service/report.go @@ -68,14 +68,17 @@ func RenderDissent(dissent []entity.Dissent) string { } // RenderReport renders a validated FinalReportData into a markdown report. -// Empty list sections are omitted. A nil input returns "". -func RenderReport(d *entity.FinalReportData) string { +// The Decision heading is taken from the authoritative resolution, not from the +// model output, so a report can never assert a decision other than the one the +// deterministic consensus produced (invariant REPORT-01). Empty list sections +// are omitted. A nil input returns "". +func RenderReport(decision entity.VoteDecision, d *entity.FinalReportData) string { if d == nil { return "" } var b strings.Builder b.WriteString("# Decision Report\n\n") - fmt.Fprintf(&b, "## Decision\n%s\n\n", d.Decision) + fmt.Fprintf(&b, "## Decision\n%s\n\n", decision) fmt.Fprintf(&b, "## Summary\n%s\n", d.Summary) if len(d.KeyReasons) > 0 { b.WriteString("\n## Key Reasons\n") diff --git a/backend/domain/service/report_test.go b/backend/domain/service/report_test.go index 57823943..4302a52c 100644 --- a/backend/domain/service/report_test.go +++ b/backend/domain/service/report_test.go @@ -10,7 +10,6 @@ import ( func TestRenderReport_FullFields(t *testing.T) { d := &entity.FinalReportData{ - Decision: "approve", Summary: "the team should proceed", KeyReasons: []string{"r1", "r2"}, Risks: []string{"risk1"}, @@ -18,7 +17,7 @@ func TestRenderReport_FullFields(t *testing.T) { KeyEvidenceIDs: []string{"EV-001", "EV-002"}, KeyClaimIDs: []string{"CL-001"}, } - out := service.RenderReport(d) + out := service.RenderReport(entity.VoteDecisionApprove, d) for _, want := range []string{"# Decision Report", "approve", "the team should proceed", "- r1", "- r2", "- risk1", "- step1", "## Key Reasons", "## Risks", "## Next Steps", "## Key Evidence", "EV-001", "EV-002", "## Key Claims", "CL-001"} { if !strings.Contains(out, want) { t.Errorf("render missing %q, got:\n%s", want, out) @@ -28,10 +27,9 @@ func TestRenderReport_FullFields(t *testing.T) { func TestRenderReport_EmptySectionsOmitted(t *testing.T) { d := &entity.FinalReportData{ - Decision: "reject", - Summary: "no", + Summary: "no", } - out := service.RenderReport(d) + out := service.RenderReport(entity.VoteDecisionReject, d) if !strings.Contains(out, "reject") || !strings.Contains(out, "no") { t.Fatalf("missing decision/summary: %s", out) } @@ -43,7 +41,7 @@ func TestRenderReport_EmptySectionsOmitted(t *testing.T) { } func TestRenderReport_NilSafe(t *testing.T) { - if got := service.RenderReport(nil); got != "" { + if got := service.RenderReport(entity.VoteDecisionApprove, nil); got != "" { t.Fatalf("nil should render empty string, got %q", got) } } diff --git a/backend/server/dto/dissent_state_test.go b/backend/server/dto/dissent_state_test.go new file mode 100644 index 00000000..b729eeb0 --- /dev/null +++ b/backend/server/dto/dissent_state_test.go @@ -0,0 +1,75 @@ +package dto + +import ( + "encoding/json" + "testing" + + "github.com/jamespud/magi/backend/domain/entity" +) + +// The three dissent states must be distinguishable in every JSON projection. +// "Not recorded" (legacy) must never serialize the same way as a confirmed +// "no dissent" (issue #13). +func TestDissentTriState_HTTPAndExportProjections(t *testing.T) { + c := &entity.DecisionCase{ID: "c1", Question: "q"} + + present := []entity.Dissent{{ + AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, + Reasoning: "risk", Conditions: []entity.DecisionCondition{{Statement: "rollback", MustHold: true}}, + }} + cases := []struct { + name string + dissent []entity.Dissent + wantState entity.DissentState + wantJSON string // the marshaled `dissent` member + }{ + {"legacy unknown", nil, entity.DissentUnknown, "null"}, + {"recorded none", []entity.Dissent{}, entity.DissentNone, "[]"}, + {"recorded present", present, entity.DissentPresent, "["}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res := &entity.Resolution{ID: "res", CaseID: "c1", FinalDecision: entity.VoteDecisionApprove, + FinalReport: "# report", Dissent: tc.dissent} + + httpResp := FromCaseWithDissent(c, res, res.Dissent) + if httpResp.DissentState != string(tc.wantState) { + t.Fatalf("HTTP dissent_state = %q, want %q", httpResp.DissentState, tc.wantState) + } + assertDissentJSON(t, "HTTP", httpResp, tc.wantJSON) + + export := FromResolutionExport(res) + if export.DissentState != string(tc.wantState) { + t.Fatalf("export dissent_state = %q, want %q", export.DissentState, tc.wantState) + } + assertDissentJSON(t, "export", export, tc.wantJSON) + }) + } +} + +// assertDissentJSON checks the raw JSON `dissent` member so null, [] and a +// populated array stay distinct on the wire. +func assertDissentJSON(t *testing.T, label string, v any, prefix string) { + t.Helper() + raw, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + var probe struct { + Dissent json.RawMessage `json:"dissent"` + } + if err := json.Unmarshal(raw, &probe); err != nil { + t.Fatal(err) + } + got := string(probe.Dissent) + if prefix == "[" { + if len(got) == 0 || got[0] != '[' || got == "[]" { + t.Fatalf("%s dissent JSON = %s, want a populated array", label, got) + } + return + } + if got != prefix { + t.Fatalf("%s dissent JSON = %s, want %s", label, got, prefix) + } +} diff --git a/backend/server/dto/dto.go b/backend/server/dto/dto.go index a4583fba..c7584466 100644 --- a/backend/server/dto/dto.go +++ b/backend/server/dto/dto.go @@ -19,21 +19,26 @@ type CreateCaseRequest struct { } type CaseResponse struct { - ID string `json:"id"` - Question string `json:"question"` - Background string `json:"background"` - Constraints []ConstraintDTO `json:"constraints"` - ParentCaseID string `json:"parent_case_id,omitempty"` - Status string `json:"status"` - Consensus *ConsensusDTO `json:"consensus,omitempty"` - Dissent []DissentDTO `json:"dissent,omitempty"` - FinalDecision string `json:"final_decision,omitempty"` - Confidence float64 `json:"confidence"` - Round int `json:"round"` - Pinned bool `json:"pinned"` - Archived bool `json:"archived"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + ID string `json:"id"` + Question string `json:"question"` + Background string `json:"background"` + Constraints []ConstraintDTO `json:"constraints"` + ParentCaseID string `json:"parent_case_id,omitempty"` + Status string `json:"status"` + Consensus *ConsensusDTO `json:"consensus,omitempty"` + // DissentState is UNKNOWN (legacy: never recorded), NONE (recorded that no + // one dissented) or PRESENT. Dissent is null for UNKNOWN, [] for NONE and + // the minority list for PRESENT, so consumers cannot mistake "not recorded" + // for "no dissent". + DissentState string `json:"dissent_state"` + Dissent []DissentDTO `json:"dissent"` + FinalDecision string `json:"final_decision,omitempty"` + Confidence float64 `json:"confidence"` + Round int `json:"round"` + Pinned bool `json:"pinned"` + Archived bool `json:"archived"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` } // UpdateCaseRequest carries optional flag updates for PATCH /cases/:id. @@ -205,7 +210,9 @@ func fromCase(c *entity.DecisionCase, resolution *entity.Resolution, dissent []e CreatedAt: c.CreatedAt.Format(time.RFC3339), UpdatedAt: c.UpdatedAt.Format(time.RFC3339), } - if len(dissent) > 0 { + resp.DissentState = string(entity.DissentStateOf(dissent)) + if dissent != nil { + resp.Dissent = make([]DissentDTO, 0, len(dissent)) for _, d := range dissent { resp.Dissent = append(resp.Dissent, FromDissent(d)) } @@ -1100,7 +1107,11 @@ type ResolutionExport struct { KeyClaimIDs []string `json:"key_claim_ids"` VoteIDs []string `json:"vote_ids"` Conditions []string `json:"conditions,omitempty"` - CreatedAt string `json:"created_at"` + // DissentState/Dissent mirror CaseResponse: null = not recorded, [] = + // recorded none, [...] = recorded minority. + DissentState string `json:"dissent_state"` + Dissent []DissentDTO `json:"dissent"` + CreatedAt string `json:"created_at"` } // FromResolutionExport maps a resolution to its export DTO (nil-safe). @@ -1112,13 +1123,20 @@ func FromResolutionExport(r *entity.Resolution) *ResolutionExport { for _, cd := range r.Consensus.Conditions { cond = append(cond, cd.Statement) } - return &ResolutionExport{ + out := &ResolutionExport{ ID: r.ID, CaseID: r.CaseID, Outcome: string(r.Consensus.Outcome), Round: r.Consensus.Round, Detail: r.Consensus.Detail, FinalDecision: string(r.FinalDecision), FinalReport: r.FinalReport, KeyEvidenceIDs: r.KeyEvidenceIDs, KeyClaimIDs: r.KeyClaimIDs, VoteIDs: r.VoteIDs, - Conditions: cond, CreatedAt: r.CreatedAt.Format(time.RFC3339), + Conditions: cond, DissentState: string(r.DissentState()), CreatedAt: r.CreatedAt.Format(time.RFC3339), } + if r.Dissent != nil { + out.Dissent = make([]DissentDTO, 0, len(r.Dissent)) + for _, d := range r.Dissent { + out.Dissent = append(out.Dissent, FromDissent(d)) + } + } + return out } // MemoryExport is the full memory projection list for a user. diff --git a/backend/server/handler/decision.go b/backend/server/handler/decision.go index 2143aa38..7cf95326 100644 --- a/backend/server/handler/decision.go +++ b/backend/server/handler/decision.go @@ -12,7 +12,6 @@ import ( "github.com/jamespud/magi/backend/application/decision" "github.com/jamespud/magi/backend/application/metrics" "github.com/jamespud/magi/backend/domain/entity" - domainservice "github.com/jamespud/magi/backend/domain/service" "github.com/jamespud/magi/backend/server/dto" ) @@ -25,17 +24,15 @@ type DecisionHandler struct { metrics *metrics.Registry } +// dissent returns the authoritative minority record persisted with the +// resolution. It never re-derives dissent from the vote tables: a legacy +// resolution without a persisted record reports no dissent rather than a +// reconstructed, unprovable one (issue #13). func (h *DecisionHandler) dissent(ctx context.Context, id string, res *entity.Resolution) []entity.Dissent { if res == nil { return nil } - votes, _ := h.svc.Votes(ctx, id) - runs, _ := h.svc.AgentRuns(ctx, id) - codes := make(map[string]entity.MagiCode, len(runs)) - for _, r := range runs { - codes[r.ID] = r.MagiCode - } - return domainservice.ExtractDissent(res.FinalDecision, votes, codes) + return res.Dissent } func NewDecisionHandler(svc *decision.Service, regs ...*metrics.Registry) *DecisionHandler { @@ -225,11 +222,9 @@ func (h *DecisionHandler) Report(ctx context.Context, c *app.RequestContext) { Forbidden(c) return } - res, _ := h.svc.Resolution(ctx, id) + // The persisted FinalReport already contains the deterministic dissent + // section, so HTTP returns the canonical bytes verbatim. report := h.svc.Report(ctx, id) - if ds := h.dissent(ctx, id, res); len(ds) > 0 { - report += "\n\n--- Dissent ---\n" + domainservice.RenderDissent(ds) - } c.JSON(consts.StatusOK, dto.DecisionReport{Report: report}) } diff --git a/backend/server/handler/export.go b/backend/server/handler/export.go index 1e265e58..b24f39f0 100644 --- a/backend/server/handler/export.go +++ b/backend/server/handler/export.go @@ -13,7 +13,6 @@ import ( "github.com/jamespud/magi/backend/application/memory" "github.com/jamespud/magi/backend/domain/entity" "github.com/jamespud/magi/backend/domain/port" - domainservice "github.com/jamespud/magi/backend/domain/service" "github.com/jamespud/magi/backend/server/dto" ) @@ -63,10 +62,8 @@ func (h *ExportHandler) Case(ctx context.Context, c *app.RequestContext) { if h.mem != nil { memory, _ = h.mem.Get(ctx, id) } + // The persisted report already carries the deterministic dissent section. report := h.dec.Report(ctx, id) - if ds := h.dissent(ctx, id, res); len(ds) > 0 { - report += "\n\n--- Dissent ---\n" + domainservice.RenderDissent(ds) - } c.JSON(consts.StatusOK, dto.CaseExport{ Case: dto.FromCaseWithDissent(case_, res, h.dissent(ctx, id, res)), Resolution: dto.FromResolutionExport(res), @@ -126,17 +123,11 @@ func (h *ExportHandler) Evaluation(ctx context.Context, c *app.RequestContext) { }) } -// dissent is a package-level helper to avoid importing the domain dissent -// renderer here twice; it reuses the same extraction the decision handler uses. +// dissent returns the authoritative minority record persisted with the +// resolution; it is never re-derived from the vote tables (issue #13). func (h *ExportHandler) dissent(ctx context.Context, id string, res *entity.Resolution) []entity.Dissent { if res == nil { return nil } - votes, _ := h.dec.Votes(ctx, id) - runs, _ := h.dec.AgentRuns(ctx, id) - codes := make(map[string]entity.MagiCode, len(runs)) - for _, r := range runs { - codes[r.ID] = r.MagiCode - } - return domainservice.ExtractDissent(res.FinalDecision, votes, codes) + return res.Dissent } diff --git a/docker/atlas/migrations/magi_s30_resolution_dissent.sql b/docker/atlas/migrations/magi_s30_resolution_dissent.sql new file mode 100644 index 00000000..36b41755 --- /dev/null +++ b/docker/atlas/migrations/magi_s30_resolution_dissent.sql @@ -0,0 +1,12 @@ +-- Issue #13: persist the authoritative structured minority record alongside the +-- resolution so the report, HTTP/A2A and export projections all read one +-- source instead of re-deriving dissent from the vote tables. +-- +-- The column is nullable on purpose. NULL means the minority record was never +-- persisted (a resolution written before this migration), which is a different +-- fact from an explicit "no dissent". New resolutions always write their record +-- as JSON: "[]" (confirmed no dissent) or "[...]" (minority positions present). +-- Reading a legacy row therefore yields "not recorded", never a fabricated +-- consensus that nobody dissented. +ALTER TABLE resolution + ADD COLUMN dissent_json TEXT NULL; diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index c6c40252..339e5acf 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -15,6 +15,9 @@ interface ApiCaseResponse { parent_case_id?: string; status: string; consensus: ApiConsensus | null; + // Tri-state: UNKNOWN = legacy resolution with no persisted minority record, + // NONE = explicitly recorded that nobody dissented, PRESENT = dissent below. + dissent_state?: 'UNKNOWN' | 'NONE' | 'PRESENT'; dissent?: ApiDissent[]; final_decision: string; confidence: number; diff --git a/frontend/src/components/workspace/ConsensusPanel.tsx b/frontend/src/components/workspace/ConsensusPanel.tsx index f8e9d6fd..dc01c764 100644 --- a/frontend/src/components/workspace/ConsensusPanel.tsx +++ b/frontend/src/components/workspace/ConsensusPanel.tsx @@ -11,6 +11,7 @@ export default function ConsensusPanel() { const status = useCaseStore((s) => s.case?.status ?? 'DRAFT'); const agents = useAgentStore((s) => s.agents); const dissent = useCaseStore((s) => s.case?.dissent ?? []); + const dissentState = useCaseStore((s) => s.case?.dissentState); const renderVoteIcon = (stance: string | undefined) => { const s = normalizeStance(stance); @@ -139,6 +140,15 @@ export default function ConsensusPanel() { })} + {dissentState === 'UNKNOWN' && dissent.length === 0 && ( +
+

Dissent

+

+ Not recorded: this resolution predates structured dissent persistence, so minority positions cannot be confirmed. +

+
+ )} + {dissent.length > 0 && (

Dissent

diff --git a/frontend/src/stores/caseStore.ts b/frontend/src/stores/caseStore.ts index addaa92c..f4ad1ad4 100644 --- a/frontend/src/stores/caseStore.ts +++ b/frontend/src/stores/caseStore.ts @@ -32,6 +32,7 @@ function mapToCase(c: ApiCaseResponse): Case { needReflection: c.consensus.need_reflection, } : null, confidence: c.confidence, + dissentState: c.dissent_state, dissent: (c.dissent || []).map((d) => ({ agentCode: d.agent_code, decision: d.decision, diff --git a/frontend/src/types/case.ts b/frontend/src/types/case.ts index af743d51..f41cffc3 100644 --- a/frontend/src/types/case.ts +++ b/frontend/src/types/case.ts @@ -69,6 +69,9 @@ export interface Case { status: CaseStatus; round: number; consensus: ConsensusState | null; + // UNKNOWN means the minority record was never persisted (legacy); it must not + // be presented as a confirmed "no dissent". + dissentState?: 'UNKNOWN' | 'NONE' | 'PRESENT'; dissent?: Dissent[]; confidence: number; finalDecision: string; From 6266d42a2f6bd5664ecc66a88f4906ee89af49eb Mon Sep 17 00:00:00 2001 From: spud <92900806+jamespud@users.noreply.github.com> Date: Wed, 14 Oct 2026 17:06:30 +0800 Subject: [PATCH 2/4] fix(report): reject cross-type citations and fail closed on corrupt dissent Addresses the review findings on the report truthfulness change. F31-01: a non-NULL dissent_json that was not a well-formed JSON array was decoded through the lenient generic helper, so corruption was reported as UNKNOWN and a stored minority record silently vanished from every projection. SQL NULL is now the only legal "not recorded" value; any other value must strictly decode to a JSON array or the resolution read fails with an error. The write path mirrors this: an (unreachable) encode failure stores a sentinel that the read rejects rather than SQL NULL. F31-02: evidence and claim citations were validated against one merged set, so a claim id cited as evidence was accepted. Each namespace is now validated against its own authoritative set, and an id may not appear in both lists. F31-03: RemapText shielded already-persisted ids with a placeholder token that could collide with control characters in the report, and rewrote bare ids with unbounded ReplaceAll, so "EV-001" could partially rewrite "EV-0012". It is now a single left-to-right pass: already-persisted ids match first and are copied verbatim, bare ids match longest-first on token boundaries, and no sentinel tokens are used. --- backend/adapter/a2a_repository.go | 12 +- backend/adapter/repository.go | 51 +++++--- backend/adapter/resolution_dissent_test.go | 27 ++++ backend/domain/entity/idmap.go | 115 ++++++++++++++---- backend/domain/entity/idmap_test.go | 40 ++++++ backend/domain/service/commander.go | 62 ++++++---- .../service/commander_truthfulness_test.go | 31 +++++ 7 files changed, 271 insertions(+), 67 deletions(-) diff --git a/backend/adapter/a2a_repository.go b/backend/adapter/a2a_repository.go index eb0206fe..3103b5db 100644 --- a/backend/adapter/a2a_repository.go +++ b/backend/adapter/a2a_repository.go @@ -270,7 +270,11 @@ func loadConversationResolutions(tx *gorm.DB, messages []*entity.ConversationMes out := make(map[string]*entity.Resolution, len(models)) for i := range models { model := &models[i] - out[model.CaseID] = resolutionFromModel(model) + resolution, err := resolutionFromModel(model) + if err != nil { + return nil, err + } + out[model.CaseID] = resolution } return out, nil } @@ -681,7 +685,11 @@ func loadTaskRecord(db *gorm.DB, binding A2ASubmissionModel, includeArtifacts bo if includeArtifacts { var resolution ResolutionModel if err := db.Where("case_id = ?", binding.TaskID).First(&resolution).Error; err == nil { - record.Resolution = resolutionFromModel(&resolution) + resolutionEntity, rerr := resolutionFromModel(&resolution) + if rerr != nil { + return nil, rerr + } + record.Resolution = resolutionEntity } else if !errors.Is(err, gorm.ErrRecordNotFound) { return nil, err } diff --git a/backend/adapter/repository.go b/backend/adapter/repository.go index e0337603..ab3a57af 100644 --- a/backend/adapter/repository.go +++ b/backend/adapter/repository.go @@ -36,27 +36,44 @@ func fromJSON[T any](s string) T { // encodeDissent persists the tri-state minority record. A nil slice is stored // as SQL NULL ("not recorded"); a non-nil slice is stored as its JSON form // ("[]" or "[...]") so an explicit "no dissent" survives a round trip. +// +// The struct is marshal-only (strings/slices/bools), so encoding cannot fail in +// practice. If it ever did, writing SQL NULL would silently turn a real +// minority record into "not recorded"; the sentinel below instead makes the +// read fail closed. func encodeDissent(d []entity.Dissent) *string { if d == nil { return nil } b, err := json.Marshal(d) if err != nil { - return nil + bad := "!dissent-encode-error" + return &bad } s := string(b) return &s } -// decodeDissent restores the tri-state. NULL (legacy), an empty string and JSON -// null all mean "not recorded"; "[]" is an explicit recorded-none; "[...]" is a -// recorded minority. json.Unmarshal yields a non-nil empty slice for "[]", so -// the nil-ness of the returned slice preserves the distinction. -func decodeDissent(raw *string) []entity.Dissent { - if raw == nil || *raw == "" || *raw == "null" { - return nil +// decodeDissent restores the tri-state. SQL NULL is the only legal "not +// recorded" representation. Every other value must be a well-formed JSON array: +// "[]" is an explicit recorded-none and "[...]" is a recorded minority. +// +// A malformed or non-array value is reported as an error instead of being +// downgraded to "not recorded": silently returning nil would drop a stored +// minority record from the HTTP/A2A/export projections and hide the corruption. +func decodeDissent(raw *string) ([]entity.Dissent, error) { + if raw == nil { + return nil, nil + } + trimmed := strings.TrimSpace(*raw) + var out []entity.Dissent + if err := json.Unmarshal([]byte(trimmed), &out); err != nil { + return nil, fmt.Errorf("resolution.dissent_json is corrupt: %w", err) } - return fromJSON[[]entity.Dissent](*raw) + if out == nil { + return nil, fmt.Errorf(`resolution.dissent_json must be a JSON array; use SQL NULL for "not recorded", got %q`, trimmed) + } + return out, nil } // --- aggregate --- @@ -1056,20 +1073,24 @@ func (r *resolutionRepo) Get(ctx context.Context, caseID string) (*entity.Resolu if err := r.db.WithContext(ctx).First(&m, "case_id = ?", caseID).Error; err != nil { return nil, err } - return resolutionFromModel(&m), nil + return resolutionFromModel(&m) } // resolutionFromModel maps a persisted resolution row to its domain entity. -// Generic (non-authoritative) reads must never invent fields a legacy row did -// not carry: Dissent stays nil when dissent_json is empty. -func resolutionFromModel(m *ResolutionModel) *entity.Resolution { +// A row whose dissent column is SQL NULL loads as UNKNOWN (never recorded); a +// corrupt non-NULL value is an error, not a silent UNKNOWN. +func resolutionFromModel(m *ResolutionModel) (*entity.Resolution, error) { + dissent, err := decodeDissent(m.DissentJSON) + if err != nil { + return nil, fmt.Errorf("resolution %s: %w", m.CaseID, err) + } return &entity.Resolution{ ID: m.ID, CaseID: m.CaseID, ExecutionGeneration: m.ExecutionGeneration, Consensus: fromJSON[entity.ConsensusResult](m.ConsensusJSON), FinalDecision: entity.VoteDecision(m.FinalDecision), FinalReport: m.FinalReport, KeyEvidenceIDs: fromJSON[[]string](m.KeyEvidenceIDsJSON), KeyClaimIDs: fromJSON[[]string](m.KeyClaimIDsJSON), - Dissent: decodeDissent(m.DissentJSON), + Dissent: dissent, VoteIDs: fromJSON[[]string](m.VoteIDsJSON), Evaluation: fromJSON[*entity.Evaluation](m.EvaluationJSON), CreatedAt: m.CreatedAt, - } + }, nil } // --- EventRepository --- diff --git a/backend/adapter/resolution_dissent_test.go b/backend/adapter/resolution_dissent_test.go index 3ad692f7..8172e8a6 100644 --- a/backend/adapter/resolution_dissent_test.go +++ b/backend/adapter/resolution_dissent_test.go @@ -53,3 +53,30 @@ func TestResolutionRepo_DissentTriStateRoundTrip(t *testing.T) { t.Fatalf("present dissent content lost: %+v", got.Dissent) } } + +// A non-NULL dissent_json that is not a well-formed JSON array is corruption, +// not "not recorded". It must fail closed instead of being downgraded to +// UNKNOWN, otherwise a stored minority record would silently vanish from the +// HTTP/A2A/export projections. +func TestResolutionRepo_CorruptDissentRecordFailsClosed(t *testing.T) { + db := openConversationDB(t) + repo := magi.NewRepository(db) + ctx := context.Background() + res := &entity.Resolution{ID: "res-corrupt", CaseID: "c-corrupt", FinalDecision: entity.VoteDecisionApprove, FinalReport: "# r", + Dissent: []entity.Dissent{{AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, Reasoning: "risk"}}} + if err := repo.ResolutionRepo().Create(ctx, res); err != nil { + t.Fatalf("create: %v", err) + } + for _, bad := range []string{"{not json", "null", "", `{"agent_code":"casper"}`} { + if err := db.Exec("UPDATE resolution SET dissent_json = ? WHERE case_id = ?", bad, "c-corrupt").Error; err != nil { + t.Fatalf("corrupt row: %v", err) + } + got, err := repo.ResolutionRepo().Get(ctx, "c-corrupt") + if err == nil { + t.Fatalf("dissent_json=%q must fail closed, got %+v (state=%s)", bad, got, got.DissentState()) + } + if got != nil { + t.Fatalf("dissent_json=%q must not return a resolution, got %+v", bad, got) + } + } +} diff --git a/backend/domain/entity/idmap.go b/backend/domain/entity/idmap.go index 6bf3dfd1..75ec20ec 100644 --- a/backend/domain/entity/idmap.go +++ b/backend/domain/entity/idmap.go @@ -1,7 +1,7 @@ package entity import ( - "strconv" + "sort" "strings" ) @@ -70,44 +70,105 @@ func (m *ArtifactRemap) RemapList(ids []string) []string { // persisted form. Used for the final report body, whose citations are // free-form text rather than structured fields. // -// It is idempotent: a namespaced ID contains its bare in-memory form as a -// suffix ("--...-EV-001" contains "EV-001"), so a naive replace -// would rewrite an already-remapped citation into a doubled prefix. Every -// persisted value is shielded first, the bare forms are rewritten, and the -// shields are restored. +// It is a single left-to-right pass, which makes it idempotent and safe: +// +// - Already-persisted (namespaced) values are matched first and copied +// verbatim, so a namespaced ID that contains its bare form as a suffix is +// never rewritten into a doubled prefix. +// - Bare IDs are replaced longest-match-first and only on token boundaries, +// so "EV-001" can neither partially rewrite "EV-0012" nor rewrite the +// "EV-001" inside an unrelated "...-EV-001" identifier. +// - Matching does not use sentinel/placeholder tokens, so control characters +// or other text in the report can never collide with the rewriting. func (m *ArtifactRemap) RemapText(s string) string { if m == nil || s == "" { return s } - type shield struct{ token, value string } - var shields []shield - guard := func(new string) { - if new == "" || !strings.Contains(s, new) { + namespaced := make(map[byte][]string, len(m.evidence)+len(m.claims)) + bare := make(map[byte][]remapPair, len(m.evidence)+len(m.claims)) + add := func(old, new string) { + if old == "" || new == "" || old == new { return } - token := "\x00" + strconv.Itoa(len(shields)) + "\x00" - s = strings.ReplaceAll(s, new, token) - shields = append(shields, shield{token: token, value: new}) + namespaced[new[0]] = append(namespaced[new[0]], new) + bare[old[0]] = append(bare[old[0]], remapPair{old: old, new: new}) } - for _, new := range m.evidence { - guard(new) + for old, new := range m.evidence { + add(old, new) } - for _, new := range m.claims { - guard(new) + for old, new := range m.claims { + add(old, new) } - replace := func(pairs map[string]string) { - for old, new := range pairs { - if old != "" { - s = strings.ReplaceAll(s, old, new) - } + for b := range namespaced { + sort.Slice(namespaced[b], func(i, j int) bool { return len(namespaced[b][i]) > len(namespaced[b][j]) }) + } + for b := range bare { + sort.Slice(bare[b], func(i, j int) bool { return len(bare[b][i].old) > len(bare[b][j].old) }) + } + + out := make([]byte, 0, len(s)) + i := 0 + for i < len(s) { + if new, ok := matchNamespaced(s, i, namespaced[s[i]]); ok { + out = append(out, new...) + i += len(new) + continue + } + if pair, ok := matchBare(s, i, bare[s[i]]); ok { + out = append(out, pair.new...) + i += len(pair.old) + continue } + out = append(out, s[i]) + i++ } - replace(m.evidence) - replace(m.claims) - for _, sh := range shields { - s = strings.ReplaceAll(s, sh.token, sh.value) + return string(out) +} + +type remapPair struct{ old, new string } + +// matchNamespaced returns the longest already-persisted id that starts at i on +// a token boundary. +func matchNamespaced(s string, i int, candidates []string) (string, bool) { + for _, cand := range candidates { + if strings.HasPrefix(s[i:], cand) && idTokenBoundary(s, i, len(cand)) { + return cand, true + } + } + return "", false +} + +// matchBare returns the longest bare id that starts at i on a token boundary. +func matchBare(s string, i int, candidates []remapPair) (remapPair, bool) { + for _, cand := range candidates { + if strings.HasPrefix(s[i:], cand.old) && idTokenBoundary(s, i, len(cand.old)) { + return cand, true + } + } + return remapPair{}, false +} + +// idTokenBoundary reports whether the substring s[i:i+n] is delimited by +// characters that cannot be part of an artifact id. +func idTokenBoundary(s string, i, n int) bool { + if i > 0 && isIDByte(s[i-1]) { + return false + } + if j := i + n; j < len(s) && isIDByte(s[j]) { + return false + } + return true +} + +func isIDByte(b byte) bool { + switch { + case b >= '0' && b <= '9', b >= 'a' && b <= 'z', b >= 'A' && b <= 'Z': + return true + case b == '-' || b == '_': + return true + default: + return false } - return s } // Merge folds other's mappings into m. diff --git a/backend/domain/entity/idmap_test.go b/backend/domain/entity/idmap_test.go index ae052118..2cd79f64 100644 --- a/backend/domain/entity/idmap_test.go +++ b/backend/domain/entity/idmap_test.go @@ -72,3 +72,43 @@ func TestCodeOfRun(t *testing.T) { } } } + +// The shield tokens used during rewriting must not collide with characters +// that already appear in the report body: a model can legitimately return +// control characters as JSON escapes, and rewriting them would corrupt text. +func TestArtifactRemap_RemapTextDoesNotRewriteUnrelatedControlChars(t *testing.T) { + m := NewArtifactRemap() + m.AddEvidence("EV-001", "case-r1-EV-001") + in := "\x000\x00 case-r1-EV-001" + if got := m.RemapText(in); got != in { + t.Fatalf("control characters were rewritten:\n got %q\nwant %q", got, in) + } +} + +// Overlapping bare IDs must be replaced longest-first and with token +// boundaries: "EV-001" must never partially rewrite "EV-0012". +func TestArtifactRemap_RemapTextHandlesOverlappingIDs(t *testing.T) { + m := NewArtifactRemap() + m.AddEvidence("EV-001", "p-EV-001") + m.AddEvidence("EV-0012", "p-EV-0012") + in := "see EV-0012 then EV-001 and EV-001" + want := "see p-EV-0012 then p-EV-001 and p-EV-001" + if got := m.RemapText(in); got != want { + t.Fatalf("overlapping rewrite:\n got %s\nwant %s", got, want) + } + if twice := m.RemapText(m.RemapText(in)); twice != want { + t.Fatalf("not idempotent:\n got %s\nwant %s", twice, want) + } +} + +// A bare ID embedded in a longer identifier (e.g. an unknown namespaced id) +// must be left alone rather than partially rewritten. +func TestArtifactRemap_RemapTextRespectsTokenBoundaries(t *testing.T) { + m := NewArtifactRemap() + m.AddEvidence("EV-001", "p-EV-001") + in := "other-scope-EV-001 and EV-001" + want := "other-scope-EV-001 and p-EV-001" + if got := m.RemapText(in); got != want { + t.Fatalf("boundary rewrite:\n got %s\nwant %s", got, want) + } +} diff --git a/backend/domain/service/commander.go b/backend/domain/service/commander.go index 109daf4c..fbb76d34 100644 --- a/backend/domain/service/commander.go +++ b/backend/domain/service/commander.go @@ -156,42 +156,58 @@ func (c *Commander) GenerateReport(ctx context.Context, case_ *entity.DecisionCa } // validateReportCitations enforces that every ID the report cites belongs to -// the frozen authoritative set handed to the model, that duplicates are not -// laundered into a longer citation list, and that a report cannot silently omit -// citations when authoritative material exists. Empty/unknown/duplicate -// references are rejected so the caller retries instead of persisting a report -// that claims evidence MAGI cannot prove (invariants REPORT-01, EVID-02). +// the frozen authoritative set handed to the model. Evidence and claim ids are +// separate namespaces: a claim id cited as evidence (or vice versa) is +// rejected, as are duplicates within one list or the same id appearing in both +// lists. Empty/unknown/duplicate references are rejected so the caller retries +// instead of persisting a report that claims evidence MAGI cannot prove +// (invariants REPORT-01, EVID-02). func validateReportCitations(data *entity.FinalReportData, evidenceIDs, claimIDs []string) error { if data == nil { return fmt.Errorf("report data is nil") } - allowed := make(map[string]struct{}, len(evidenceIDs)+len(claimIDs)) + allowedEvidence := make(map[string]struct{}, len(evidenceIDs)) for _, id := range evidenceIDs { - allowed[id] = struct{}{} + allowedEvidence[id] = struct{}{} } + allowedClaims := make(map[string]struct{}, len(claimIDs)) for _, id := range claimIDs { - allowed[id] = struct{}{} + allowedClaims[id] = struct{}{} } - cited := append(append([]string{}, data.KeyEvidenceIDs...), data.KeyClaimIDs...) - for _, id := range cited { + if err := checkCitationList("evidence", data.KeyEvidenceIDs, allowedEvidence); err != nil { + return err + } + if err := checkCitationList("claim", data.KeyClaimIDs, allowedClaims); err != nil { + return err + } + seen := make(map[string]struct{}, len(data.KeyEvidenceIDs)+len(data.KeyClaimIDs)) + for _, id := range append(append([]string{}, data.KeyEvidenceIDs...), data.KeyClaimIDs...) { + if _, dup := seen[id]; dup { + return fmt.Errorf("report cites %q in more than one citation list", id) + } + seen[id] = struct{}{} + } + if available := len(allowedEvidence) + len(allowedClaims); available > 0 && len(seen) == 0 { + return fmt.Errorf("report cites no authoritative artifact while %d are available", available) + } + return nil +} + +// checkCitationList validates one citation namespace: no empty id, no id +// outside the authorized set, and no duplicate inside the list. +func checkCitationList(kind string, ids []string, allowed map[string]struct{}) error { + seen := make(map[string]struct{}, len(ids)) + for _, id := range ids { if id == "" { - return fmt.Errorf("report cites an empty artifact id") + return fmt.Errorf("report cites an empty %s id", kind) } if _, ok := allowed[id]; !ok { - return fmt.Errorf("report cites %q which is not in the authoritative set %v", id, sortedKeys(allowed)) + return fmt.Errorf("report cites %q which is not an authorized %s id %v", id, kind, sortedKeys(allowed)) } - } - for name, ids := range map[string][]string{"evidence": data.KeyEvidenceIDs, "claim": data.KeyClaimIDs} { - seen := make(map[string]struct{}, len(ids)) - for _, id := range ids { - if _, dup := seen[id]; dup { - return fmt.Errorf("report cites duplicate %s id %q", name, id) - } - seen[id] = struct{}{} + if _, dup := seen[id]; dup { + return fmt.Errorf("report cites duplicate %s id %q", kind, id) } - } - if len(allowed) > 0 && len(cited) == 0 { - return fmt.Errorf("report cites no authoritative artifact while %d are available", len(allowed)) + seen[id] = struct{}{} } return nil } diff --git a/backend/domain/service/commander_truthfulness_test.go b/backend/domain/service/commander_truthfulness_test.go index 1d993b02..647ef8cd 100644 --- a/backend/domain/service/commander_truthfulness_test.go +++ b/backend/domain/service/commander_truthfulness_test.go @@ -95,3 +95,34 @@ func TestGenerateReport_RejectsModelSuppliedDecision(t *testing.T) { t.Fatal("a report that supplies its own decision must be rejected") } } + +// Evidence and claim citations are different namespaces: a claim id must not +// be accepted as an evidence citation (and vice versa). +func TestGenerateReport_RejectsCrossTypeCitation(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["CL-001"]}`, nil), + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["CL-001"]}`, nil), + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["CL-001"]}`, nil), + }} + cmd := newReportCommander(t, m) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, []string{"CL-001"}) + if err == nil { + t.Fatal("a claim id cited as evidence must be rejected") + } +} + +// The same id must not be cited in both the evidence and the claim list. +func TestGenerateReport_RejectsIdInBothCitationLists(t *testing.T) { + m := &reportScriptedModel{responses: []*schema.Message{ + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"],"key_claim_ids":["EV-001"]}`, nil), + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"],"key_claim_ids":["EV-001"]}`, nil), + schema.AssistantMessage(`{"summary":"s","key_reasons":["r"],"risks":[],"next_steps":[],"key_evidence_ids":["EV-001"],"key_claim_ids":["EV-001"]}`, nil), + }} + cmd := newReportCommander(t, m) + _, err := cmd.GenerateReport(context.Background(), &entity.DecisionCase{Question: "q"}, + &entity.Resolution{FinalDecision: entity.VoteDecisionApprove}, nil, []string{"EV-001"}, []string{"EV-001"}) + if err == nil { + t.Fatal("an id cited in both lists must be rejected") + } +} From c6651d8adee4fdc98bcf148d27db7fc364fb14d9 Mon Sep 17 00:00:00 2001 From: spud <92900806+jamespud@users.noreply.github.com> Date: Wed, 14 Oct 2026 18:34:47 +0800 Subject: [PATCH 3/4] fix(report): validate persisted dissent elements F31-04: a well-formed JSON array can still hold elements that are not valid dissent records. `[null]` and `[{}]` decoded into a non-empty slice and were reported as PRESENT while carrying no persona or decision, so the tri-state guard stopped at the array level. Add entity.ValidateDissent and apply it on the read path, so a malformed or unattributable element makes the resolution read fail closed instead of surfacing as a real minority position. The orchestration write path validates the same way before persisting, so a record that would be rejected on read can never be committed. --- backend/adapter/repository.go | 5 +++ backend/adapter/resolution_dissent_test.go | 29 +++++++++++++++++ backend/domain/entity/resolution.go | 22 ++++++++++++- backend/domain/entity/resolution_test.go | 33 ++++++++++++++++++++ backend/domain/orchestration/orchestrator.go | 12 +++++-- backend/domain/orchestration/steps.go | 6 +++- 6 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 backend/domain/entity/resolution_test.go diff --git a/backend/adapter/repository.go b/backend/adapter/repository.go index ab3a57af..1ccfa595 100644 --- a/backend/adapter/repository.go +++ b/backend/adapter/repository.go @@ -73,6 +73,11 @@ func decodeDissent(raw *string) ([]entity.Dissent, error) { if out == nil { return nil, fmt.Errorf(`resolution.dissent_json must be a JSON array; use SQL NULL for "not recorded", got %q`, trimmed) } + for i, d := range out { + if err := entity.ValidateDissent(d); err != nil { + return nil, fmt.Errorf("resolution.dissent_json element %d is invalid: %w", i, err) + } + } return out, nil } diff --git a/backend/adapter/resolution_dissent_test.go b/backend/adapter/resolution_dissent_test.go index 8172e8a6..83d73f29 100644 --- a/backend/adapter/resolution_dissent_test.go +++ b/backend/adapter/resolution_dissent_test.go @@ -80,3 +80,32 @@ func TestResolutionRepo_CorruptDissentRecordFailsClosed(t *testing.T) { } } } + +// A well-formed JSON array can still contain elements that are not valid +// dissent records. Those must fail closed instead of being reported as PRESENT. +func TestResolutionRepo_InvalidDissentElementsFailClosed(t *testing.T) { + db := openConversationDB(t) + repo := magi.NewRepository(db) + ctx := context.Background() + res := &entity.Resolution{ID: "res-elem", CaseID: "c-elem", FinalDecision: entity.VoteDecisionApprove, FinalReport: "# r", + Dissent: []entity.Dissent{{AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, Reasoning: "risk"}}} + if err := repo.ResolutionRepo().Create(ctx, res); err != nil { + t.Fatalf("create: %v", err) + } + for _, bad := range []string{ + `[null]`, + `[{}]`, + `[{"agent_code":"nobody","decision":"reject"}]`, + `[{"agent_code":"casper","decision":"maybe"}]`, + `[{"decision":"reject"}]`, + `[{"agent_code":"casper"}]`, + } { + if err := db.Exec("UPDATE resolution SET dissent_json = ? WHERE case_id = ?", bad, "c-elem").Error; err != nil { + t.Fatalf("corrupt row: %v", err) + } + got, err := repo.ResolutionRepo().Get(ctx, "c-elem") + if err == nil { + t.Fatalf("dissent_json=%s must fail closed, got %+v (state=%s)", bad, got, got.DissentState()) + } + } +} diff --git a/backend/domain/entity/resolution.go b/backend/domain/entity/resolution.go index d8f701f8..7b47460b 100644 --- a/backend/domain/entity/resolution.go +++ b/backend/domain/entity/resolution.go @@ -1,6 +1,9 @@ package entity -import "time" +import ( + "fmt" + "time" +) // Resolution is the final outcome of a DecisionCase. type Resolution struct { @@ -66,6 +69,23 @@ func (r *Resolution) DissentState() DissentState { return DissentStateOf(r.Dissent) } +// ValidateDissent rejects a dissent record that cannot be trusted as a +// structured minority position: a voting persona must be identified and the +// decision must be a defined vote value. A well-formed JSON array can still +// contain `null` or `{}` elements, which must never be reported as a real +// minority position. +func ValidateDissent(d Dissent) error { + switch d.AgentCode { + case MagiCodeMelchior, MagiCodeBalthasar, MagiCodeCasper: + default: + return fmt.Errorf("dissent agent code %q is not a voting persona", d.AgentCode) + } + if !IsValidVoteDecision(d.Decision) { + return fmt.Errorf("dissent decision %q is not a valid vote decision", d.Decision) + } + return nil +} + // Dissent is a structured minority position recorded for auditability. type Dissent struct { AgentCode MagiCode diff --git a/backend/domain/entity/resolution_test.go b/backend/domain/entity/resolution_test.go new file mode 100644 index 00000000..2575001b --- /dev/null +++ b/backend/domain/entity/resolution_test.go @@ -0,0 +1,33 @@ +package entity_test + +import ( + "testing" + + "github.com/jamespud/magi/backend/domain/entity" +) + +func TestValidateDissent(t *testing.T) { + valid := []entity.Dissent{ + {AgentCode: entity.MagiCodeMelchior, Decision: entity.VoteDecisionReject}, + {AgentCode: entity.MagiCodeBalthasar, Decision: entity.VoteDecisionApprove}, + {AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionConditionalApprove}, + } + for i, d := range valid { + if err := entity.ValidateDissent(d); err != nil { + t.Fatalf("valid[%d] rejected: %v", i, err) + } + } + invalid := []entity.Dissent{ + {}, // zero value from `[null]` / `[{}]` + {AgentCode: entity.MagiCodeCasper}, // no decision + {Decision: entity.VoteDecisionReject}, // no persona + {AgentCode: "nobody", Decision: entity.VoteDecisionReject}, // unknown persona + {AgentCode: entity.MagiCodeCommander, Decision: entity.VoteDecisionReject}, // commander does not vote + {AgentCode: entity.MagiCodeCasper, Decision: "maybe"}, // undefined decision + } + for i, d := range invalid { + if err := entity.ValidateDissent(d); err == nil { + t.Fatalf("invalid[%d] accepted: %+v", i, d) + } + } +} diff --git a/backend/domain/orchestration/orchestrator.go b/backend/domain/orchestration/orchestrator.go index b4c5edbf..03e8ac93 100644 --- a/backend/domain/orchestration/orchestrator.go +++ b/backend/domain/orchestration/orchestrator.go @@ -536,14 +536,22 @@ func (o *Orchestrator) collectBallots(results []*runtime.LoopResult) ([]*entity. // Votes and results stay index-aligned with the config slice, so an agent code // is read from the same position that produced the ballot. A nil vote slot is a // missing ballot, never a dissent. -func (o *Orchestrator) extractDissent(st *State) []entity.Dissent { +func (o *Orchestrator) extractDissent(st *State) ([]entity.Dissent, error) { codes := make([]entity.MagiCode, len(st.Votes)) for i := range st.Votes { if st.Votes[i] != nil { codes[i] = codeOf(o.configAt(i)) } } - return service.ExtractDissent(st.Resolution.FinalDecision, st.Votes, codes) + dissent := service.ExtractDissent(st.Resolution.FinalDecision, st.Votes, codes) + // Never persist a record the read path would reject: an unattributable or + // malformed minority position is a case failure, not a silent drop. + for i, d := range dissent { + if err := entity.ValidateDissent(d); err != nil { + return nil, fmt.Errorf("dissent %d: %w", i, err) + } + } + return dissent, nil } // absencesMsg renders the non-votes for a case-failure message. diff --git a/backend/domain/orchestration/steps.go b/backend/domain/orchestration/steps.go index 6c9294ec..b567d462 100644 --- a/backend/domain/orchestration/steps.go +++ b/backend/domain/orchestration/steps.go @@ -267,7 +267,11 @@ func (o *Orchestrator) stepResolve(ctx context.Context, case_ *entity.DecisionCa // report, HTTP/A2A and export read one authoritative record instead of // re-deriving dissent from mutable vote joins (issue #13). st.Resolution = res - res.Dissent = o.extractDissent(st) + dissent, err := o.extractDissent(st) + if err != nil { + return "", false, fmt.Errorf("resolve dissent: %w", err) + } + res.Dissent = dissent return entity.CaseStatusGeneratingReport, false, nil } From 0ccb9a87ddf1aabb9557eaa01c0927e7cd25d135 Mon Sep 17 00:00:00 2001 From: spud <92900806+jamespud@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:03:24 +0800 Subject: [PATCH 4/4] fix(report): reject abstention as persisted dissent F31-05: ValidateDissent reused IsValidVoteDecision, so a stored `{"agent_code":"casper","decision":"abstain"}` was reported as PRESENT even though an abstention is not a minority position and ExtractDissent never emits one. Abstention is now rejected as a dissent element; the legacy conditional_approve value stays accepted because the current single-candidate VoteDecision still defines it (the stricter Rev4 three-value rule applies to the new protocol's SealedBallot, not to legacy Resolution reads). Also give entity.Dissent explicit snake_case json tags. It had none, so the persisted dissent_json used Go's default PascalCase and the existing element-level tests' snake_case payloads silently decoded to zero values -- passing without reaching the intended validation branch. The tests now assert the failure reason, and a new test locks the stored encoding. --- backend/adapter/resolution_dissent_test.go | 67 +++++++++++++++++----- backend/domain/entity/resolution.go | 24 +++++--- backend/domain/entity/resolution_test.go | 1 + 3 files changed, 70 insertions(+), 22 deletions(-) diff --git a/backend/adapter/resolution_dissent_test.go b/backend/adapter/resolution_dissent_test.go index 83d73f29..743712ba 100644 --- a/backend/adapter/resolution_dissent_test.go +++ b/backend/adapter/resolution_dissent_test.go @@ -2,6 +2,7 @@ package magi_test import ( "context" + "strings" "testing" magi "github.com/jamespud/magi/backend/adapter" @@ -82,7 +83,9 @@ func TestResolutionRepo_CorruptDissentRecordFailsClosed(t *testing.T) { } // A well-formed JSON array can still contain elements that are not valid -// dissent records. Those must fail closed instead of being reported as PRESENT. +// dissent records. Those must fail closed, and for a specific reason: the JSON +// keys below match the persisted encoding (entity.Dissent json tags), so each +// case really reaches its validation branch. func TestResolutionRepo_InvalidDissentElementsFailClosed(t *testing.T) { db := openConversationDB(t) repo := magi.NewRepository(db) @@ -92,20 +95,54 @@ func TestResolutionRepo_InvalidDissentElementsFailClosed(t *testing.T) { if err := repo.ResolutionRepo().Create(ctx, res); err != nil { t.Fatalf("create: %v", err) } - for _, bad := range []string{ - `[null]`, - `[{}]`, - `[{"agent_code":"nobody","decision":"reject"}]`, - `[{"agent_code":"casper","decision":"maybe"}]`, - `[{"decision":"reject"}]`, - `[{"agent_code":"casper"}]`, - } { - if err := db.Exec("UPDATE resolution SET dissent_json = ? WHERE case_id = ?", bad, "c-elem").Error; err != nil { - t.Fatalf("corrupt row: %v", err) - } - got, err := repo.ResolutionRepo().Get(ctx, "c-elem") - if err == nil { - t.Fatalf("dissent_json=%s must fail closed, got %+v (state=%s)", bad, got, got.DissentState()) + cases := []struct{ name, json, want string }{ + {"null element", `[null]`, "not a voting persona"}, + {"empty object", `[{}]`, "not a voting persona"}, + {"unknown persona", `[{"agent_code":"nobody","decision":"reject"}]`, "not a voting persona"}, + {"commander does not vote", `[{"agent_code":"commander","decision":"reject"}]`, "not a voting persona"}, + {"undefined decision", `[{"agent_code":"casper","decision":"maybe"}]`, "not a valid vote decision"}, + {"missing decision", `[{"agent_code":"casper"}]`, "not a valid vote decision"}, + {"abstention is not dissent", `[{"agent_code":"casper","decision":"abstain"}]`, "abstain"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if err := db.Exec("UPDATE resolution SET dissent_json = ? WHERE case_id = ?", tc.json, "c-elem").Error; err != nil { + t.Fatalf("corrupt row: %v", err) + } + got, err := repo.ResolutionRepo().Get(ctx, "c-elem") + if err == nil { + t.Fatalf("dissent_json=%s must fail closed, got %+v (state=%s)", tc.json, got, got.DissentState()) + } + if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("dissent_json=%s error %q does not mention %q (wrong branch)", tc.json, err, tc.want) + } + }) + } +} + +// Lock the persisted encoding: resolution.dissent_json must use the explicit +// snake_case keys (matching the HTTP/A2A projections), not Go's default +// PascalCase, so hand-written and generated records agree. +func TestResolutionRepo_DissentJSONUsesSnakeCaseKeys(t *testing.T) { + db := openConversationDB(t) + repo := magi.NewRepository(db) + ctx := context.Background() + res := &entity.Resolution{ID: "res-keys", CaseID: "c-keys", FinalDecision: entity.VoteDecisionApprove, FinalReport: "# r", + Dissent: []entity.Dissent{{AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionReject, Reasoning: "risk", + EvidenceIDs: []string{"EV-1"}, Conditions: []entity.DecisionCondition{{Statement: "rollback", MustHold: true}}}}} + if err := repo.ResolutionRepo().Create(ctx, res); err != nil { + t.Fatalf("create: %v", err) + } + var stored string + if err := db.Raw("SELECT dissent_json FROM resolution WHERE case_id = ?", "c-keys").Scan(&stored).Error; err != nil { + t.Fatalf("read raw: %v", err) + } + for _, want := range []string{`"agent_code":"casper"`, `"decision":"reject"`, `"evidence_ids":["EV-1"]`, `"conditions"`} { + if !strings.Contains(stored, want) { + t.Fatalf("stored dissent_json missing %s: %s", want, stored) } } + if strings.Contains(stored, "AgentCode") || strings.Contains(stored, "Decision") { + t.Fatalf("stored dissent_json uses PascalCase keys: %s", stored) + } } diff --git a/backend/domain/entity/resolution.go b/backend/domain/entity/resolution.go index 7b47460b..4a0a9669 100644 --- a/backend/domain/entity/resolution.go +++ b/backend/domain/entity/resolution.go @@ -83,17 +83,27 @@ func ValidateDissent(d Dissent) error { if !IsValidVoteDecision(d.Decision) { return fmt.Errorf("dissent decision %q is not a valid vote decision", d.Decision) } + // A valid vote value is not automatically a valid minority position: an + // abstention is not dissent (ExtractDissent never emits one). The legacy + // conditional_approve value stays acceptable here because the current + // single-candidate VoteDecision still defines it. + if d.Decision == VoteDecisionAbstain { + return fmt.Errorf("dissent decision %q is an abstention, which is not a minority position", d.Decision) + } return nil } -// Dissent is a structured minority position recorded for auditability. +// Dissent is a structured minority position recorded for auditability. It is +// persisted in resolution.dissent_json, so these tags define the stored +// encoding: explicit snake_case rather than Go's default PascalCase, matching +// the rest of the schema and the HTTP/A2A projections. type Dissent struct { - AgentCode MagiCode - Decision VoteDecision - Reasoning string - EvidenceIDs []string - ClaimIDs []string - Conditions []DecisionCondition + AgentCode MagiCode `json:"agent_code"` + Decision VoteDecision `json:"decision"` + Reasoning string `json:"reasoning,omitempty"` + EvidenceIDs []string `json:"evidence_ids,omitempty"` + ClaimIDs []string `json:"claim_ids,omitempty"` + Conditions []DecisionCondition `json:"conditions,omitempty"` } // ConsensusResult is the deterministic consensus outcome. diff --git a/backend/domain/entity/resolution_test.go b/backend/domain/entity/resolution_test.go index 2575001b..db552ffb 100644 --- a/backend/domain/entity/resolution_test.go +++ b/backend/domain/entity/resolution_test.go @@ -24,6 +24,7 @@ func TestValidateDissent(t *testing.T) { {AgentCode: "nobody", Decision: entity.VoteDecisionReject}, // unknown persona {AgentCode: entity.MagiCodeCommander, Decision: entity.VoteDecisionReject}, // commander does not vote {AgentCode: entity.MagiCodeCasper, Decision: "maybe"}, // undefined decision + {AgentCode: entity.MagiCodeCasper, Decision: entity.VoteDecisionAbstain}, // abstention is not dissent } for i, d := range invalid { if err := entity.ValidateDissent(d); err == nil {