Repository navigation
fix(report): validate authoritative citations and persist dissent - #31
Conversation
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.
…issent 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.
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.
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.
jamespud
left a comment
There was a problem hiding this comment.
Code review — head 0ccb9a87ddf1aabb9557eaa01c0927e7cd25d135
Conclusion: no outstanding blocking findings identified within the scoped report-truthfulness changes. This is a COMMENT review, not an independent GitHub approval: the connected account (jamespud) is also the PR author.
- F31-01..05 were rechecked against the final code and regression tests.
dissent_jsonpreserves SQL NULL vs[]vs valid nonempty arrays; malformed/non-array/invalid Dissent elements fail closed on reads, and the normal write path validates them before persistence.abstainis rejected as Dissent; the legacy single-candidateconditional_approveremains compatible. - Evidence IDs and Claim IDs are validated independently against the frozen sets and checked for duplication; authoritative
FinalDecisioncomes from the Resolution, not model-supplied JSON.RemapTextuses one-pass, token-delimited replacement without sentinel collisions. - The latest CI run completed successfully with 6/6 jobs, including MySQL migration compatibility and MySQL decision-writer cutover. CI success does not imply the separately documented live-Sandbox tests ran.
Scope / merge note: The PR intentionally does not validate free-text claims that an action has already been authorized, sent, or verified. This is a remaining aspect of the original #13 acceptance text and the Rev4 REPORT-01 contract. Keep that deficiency explicitly tracked under Epic #7 and avoid closing #13 as fully satisfied unless its outstanding acceptance criterion is explicitly split/updated. The S30 schema change and new Dissent JSON encoding are also part of the release/migration notes.
Recommendation: Ready for an independent maintainer review and merge decision. No merge, status or issue changes were made by this review.
问题
报告生成存在三处真实性缺陷(issue #13 / REPORT-01):
reportCites只要求引用列表非空,模型编造一个形状合法但不在冻结集合里的EV-/CL-ID 也能通过,并可能被持久化。FinalDecision,可以写出与确定性裁决不一致的结论。改动
validateReportCitations。空 ID、未知 ID、重复 ID 一律拒绝并触发重试(最多 3 次后失败);当存在权威材料时要求至少一条引用。引用集合来自stepResolve用本轮 ledger 经ArtifactRemap生成的命名空间 ID,并在终结提交时由verifyResolutionReferences按case_id + execution_generation复核。FinalReportData移除decision字段(schema 收紧,模型再填即校验失败);报告标题改由resolution.FinalDecision渲染。Resolution.Dissent+ 迁移magi_s30_resolution_dissent.sql(列dissent_json TEXT NULL)。在stepResolve按“票据与 config 位置对齐”绑定少数意见,并把确定性 Dissent 段写入规范持久化报告。CaseResponse、导出ResolutionExport、A2AdecisionResult与前端统一读取持久化记录,并新增dissent_state/dissentState。ArtifactRemap.RemapText幂等修复:命名空间 ID 里含裸 ID 作为后缀,原实现二次替换会生成…-EV-001双重前缀(模型回显 prompt 中命名空间 ID 时就会命中)。NULL / [] / 非空数组的领域含义
dissent_json可空,三态在 DB / 领域 / JSON 之间保持一致,不得塌缩:dissent_statedissentNULLDissent == nilUNKNOWNnull'[]'NONE[]'[…]'PRESENT[…]历史 Resolution 因此读为
UNKNOWN,不会被当成“确认无异议”。验证(本地,作者在本 commit
5fcfdd0上执行)RED→GREEN 具名测试:
backend/domain/service/commander_truthfulness_test.go:编造引用、重复引用、模型自填 decision 全部拒绝;重试后正确引用通过;裁决取自 resolution。backend/domain/orchestration/dissent_binding_test.go:2:1 场景持久化唯一少数意见(含条件)、报告内嵌 Dissent 段、重载一致;一致同意不造 dissent;报告引用均带本 case/代次前缀。backend/adapter/resolution_dissent_test.go、backend/server/dto/dissent_state_test.go、backend/application/a2a/projector_test.go、backend/domain/entity/idmap_test.go:三态往返、HTTP/导出/A2A JSON 三态、RemapText 幂等。backend/bootstrap/mysql_migration_s30_test.go+backend/bootstrap/migration_parity_test.go。命令与结果:
go test ./...、go test -race ./...、go vet ./...、gofmt -l .:全部通过 / 无输出。npm run lint(0 error,1 条Dataset.tsx既有 warning)、npx tsc -b、npm test(188 passed)。MAGI_TEST_MYSQL_DSN=… go test -p 1 ./bootstrap/ ./adapter/ ./domain/runtime/→ PASS=645 FAIL=0 SKIP=4。覆盖:S29→S30 升级保留历史行、全新初始化与逐版本迁移列形状一致、NULL/[]/非空三态读写、终态事务失败不产生部分 Resolution(无 FinalReport/Dissent 残留)。TestCodeRunnerAdapter_RealSandbox_RunsPython、…_TimeoutKillsInfiniteLoop、…_BlockedPatternRejected、…_DisallowedLanguageRejected—— 均需真实代码沙箱,与本次 MySQL/报告改动无关。未覆盖边界(重要)
本 PR 只保护结构化真实性:权威裁决绑定、引用必须属于当前有效代次的冻结工件集合、Dissent 持久化与跨渠道一致。它没有对自由文本中“已授权 / 已发送 / 已验证”等权限与执行状态陈述做语义校验。该六维状态与真实报告契约归属 Epic #7,需独立实施与追踪。
因此本 PR 不应被视为 REPORT-01 完整通过;关闭 #13 前需确认其范围已被本 PR 满足,且剩余能力在 Epic #7 下有独立条目承接。
兼容性
dissent_json可空、无默认值;旧行不迁移、不补值,读取为UNKNOWN。A2A 新增conditions/dissentState为可选字段,向后兼容。审阅重点
validateReportCitations与 Generation Fencing、终态事务之间是否存在 TOCTOU 或跨代引用漏洞。ArtifactRemap.RemapText幂等实现对部分重叠、相似前缀、恶意构造 ID 的处理。Related to #13:本 PR 只交付结构化引用与持久化 Dissent;#13 的自由文本真实性验收仍未完成,保留 Open 并由 Epic #7 继续追踪。
审阅修订(第二轮,由首轮审阅 findings 驱动)
修订提交:
6266d42(首轮审阅对象为5fcfdd0)。dissent_json非 NULL 但非合法 JSON 数组时,此前会被宽松解码成nil(UNKNOWN),使已存少数意见在所有投影中静默消失。现在 SQL NULL 是唯一合法“未记录”;其他值必须严格解码为 JSON 数组,否则读取返回明确错误;写入侧同样 fail-closed。RED→GREEN:
TestResolutionRepo_CorruptDissentRecordFailsClosed({not json/null/""/ JSON 对象)。RED→GREEN:
TestGenerateReport_RejectsCrossTypeCitation、TestGenerateReport_RejectsIdInBothCitationLists。RemapText的占位符可能与正文控制字符冲突,且ReplaceAll会部分重写前缀重叠的 ID。现在改为单次左到右扫描:已命名空间化的 ID 优先原样匹配,裸 ID 按最长匹配 + token 边界替换,不再使用占位符。RED→GREEN:
TestArtifactRemap_RemapTextDoesNotRewriteUnrelatedControlChars、TestArtifactRemap_RemapTextHandlesOverlappingIDs、TestArtifactRemap_RemapTextRespectsTokenBoundaries。修订后在
6266d42上本地复跑:go test ./...、go test -race ./...、go vet ./...、gofmt -l .全部通过。CI 结果以本 PR 的 checks 为准。审阅修订(第三轮,F31-04)
修订提交:
c6651d8(第二轮审阅对象为6266d42)。[null]/[{}]仍会形成非空切片并被报为PRESENT,却没有任何人格与结论。新增entity.ValidateDissent,在读取路径逐元素校验(必须是有投票权的人格 + 合法Decision);写入路径在持久化前用同一校验,避免写出读取器会拒绝的记录。RED→GREEN:
TestResolutionRepo_InvalidDissentElementsFailClosed([null]、[{}]、未知人格、非法决策、缺字段)、TestValidateDissent。本地在
c6651d8上复跑:go test ./...、go test -race ./...、go vet ./...、gofmt -l .全部通过。CI 结果以本 PR 的 checks 为准。审阅修订(F31-05)
修订提交:
0ccb9a8(第三轮审阅对象为c6651d8)。ValidateDissent复用了IsValidVoteDecision,因此持久化的{"agent_code":"casper","decision":"abstain"}会被当作PRESENT,但弃权不是少数意见,ExtractDissent也不会产生它。现在显式拒绝abstain;旧协议的conditional_approve仍被接受(严格的 Rev4 三值规则适用于新协议的SealedBallot,不回溯限制历史 Resolution 读取)。entity.Dissent原本没有 json tag,持久化用 PascalCase,导致既有测试里的 snake_case 载荷被静默解码成零值——测试通过但没有走到目标分支。现为Dissent增加显式 snake_case tag,测试改为断言失败原因,并新增测试锁定存储编码。RED→GREEN:
TestResolutionRepo_InvalidDissentElementsFailClosed(7 个子用例,含 abstain 与「未真正命中分支」的对照)、TestValidateDissent、TestResolutionRepo_DissentJSONUsesSnakeCaseKeys。本地在
0ccb9a8上复跑:go test ./...、go test -race ./...、go vet ./...、gofmt -l .全部通过。CI 结果以本 PR 的 checks 为准。