feat(research): add reversible duplicate review manifest - #12
feat(research): add reversible duplicate review manifest#12seonghobae wants to merge 27 commits into
Conversation
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4324218125
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let after_canonical_keys = candidate | ||
| .item_keys | ||
| .iter() | ||
| .map(|item_key| (item_key.clone(), decision.retained_item_key.clone())) | ||
| .collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
When records share more than one identity—for example, two items with both the same DOI and title—the report contains overlapping DOI and title candidates, but each decision is validated independently. A steward can therefore retain A for the DOI group and B for the title group, producing simultaneous mappings B -> A and A -> B; downstream canonical resolution then depends on which operation is applied or queried rather than yielding one canonical record. Require consistent retained keys across overlapping groups or collapse candidates into connected components before emitting mappings.
Useful? React with 👍 / 👎.
| let item_revisions = report | ||
| .snapshot_items | ||
| .iter() | ||
| .map(|item| (item.item_key.as_str(), item.item_version)) | ||
| .collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
When the public classify_snapshot entry point receives repeated Zotero keys, it can produce a candidate containing the same key more than once, and this collect silently keeps only the last version. A subsequently approved manifest then emits that surviving version for every occurrence while claiming to preserve the exact source revisions, corrupting the review evidence instead of failing closed. Validate that snapshot_items contains unique, nonblank keys before building this map, as the golden-set path already does.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| /// Snapshot-bound duplicate decisions verified by the caller's governance boundary. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] | ||
| pub struct ReviewedDuplicateMergeSet { | ||
| /// Opaque steward review receipt. | ||
| pub review_id: String, | ||
| /// Opaque governance authority receipt; contains no person identity. | ||
| pub authority_receipt: String, |
There was a problem hiding this comment.
This introduces new public domain concepts—including the reviewed duplicate set, authority receipt, canonical-key operation, and the Research Intake-to-Governance verification handoff—but neither docs/UBIQUITOUS_LANGUAGE.md nor docs/CONTEXT_MAP.md defines that language or relationship. Update those DDD artifacts so the newly documented workflow does not diverge from the repository's canonical domain model.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
…search/zotero-duplicate-review-manifest
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ef6504df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if canonical_choices | ||
| .insert(item_key, decision.retained_item_key.as_str()) | ||
| .is_some_and(|retained_key| retained_key != decision.retained_item_key) | ||
| { | ||
| return Err(DuplicateReviewError::InvalidReview); |
There was a problem hiding this comment.
When duplicate identities form a chain such as DOI {A,B}, title {B,C}, and DOI {C,D}, no review can pass this check: the first overlap forces the middle group to retain B, while the second forces it to retain C. Fresh evidence beyond the earlier conflicting-choice finding is that the new per-item consistency check rejects every possible decision set for this valid classifier output, even though the documented Reviewed Duplicate Merge Set requires one canonical item across the whole overlapping cluster. Collapse connected candidates into a component before review, or permit every group to select a component-level retained key.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| .map(|item| item.item_key.as_str()) | ||
| .collect::<BTreeSet<_>>() | ||
| .len() | ||
| != report.snapshot_items.len() |
There was a problem hiding this comment.
When the public classify_snapshot entry point receives one uniquely blank Zotero key alongside another item sharing its DOI or title, this cardinality check passes and the manifest can emit source_items and canonical maps containing "" as a source or retained identity. Fresh evidence beyond the earlier duplicate-key finding is that the added validation checks uniqueness only and never checks item_key.trim().is_empty(), so the supposedly stable source identity remains unusable for downstream resolution and provenance. Reject blank snapshot keys before verifying or materializing the review.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fb7bfa5dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| item_revisions | ||
| .get(item_key.as_str()) | ||
| .map(|item_version| SnapshotItemRevision { | ||
| item_key: item_key.clone(), | ||
| item_version: *item_version, |
There was a problem hiding this comment.
The new key-only validation still permits a caller to alter a public ClassificationReport.snapshot_items entry's version while retaining the original reviewed digest; every check passes and this line emits the altered version under the authority receipt for the original snapshot. Since the tests already treat mutated reports as untrusted inputs, bind the reviewed set to the complete item-key/version coordinates, as the golden-set contract does, before materializing these source revisions.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| let source_items = candidate | ||
| .item_keys | ||
| .iter() |
There was a problem hiding this comment.
The new component-level retained-key acceptance allows the transitive {A,B}, {B,C}, {C,D} case to choose A, but the operation for {C,D} still builds source_items only from candidate.item_keys; it therefore names A as the retained record without preserving A's source revision in that operation. This makes the supposedly exact operation evidence incomplete for independent validation or replay, so build each operation from the connected component (or collapse the component into one operation).
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e77a2d232c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .ok_or(DuplicateReviewError::UnknownCandidate)?; | ||
| // ponytail: quadratic component expansion is enough for a steward-sized review; | ||
| // replace with union-find only if measured duplicate sets become large. | ||
| let mut component_keys = candidate.item_keys.iter().collect::<BTreeSet<_>>(); |
There was a problem hiding this comment.
When a reviewed {A,B} candidate shares a snapshot with an unrelated item C, replacing the public candidate's B key with C after review passes every check because C exists in item_revisions; this line then emits C -> A under the unchanged verified receipt while silently omitting B. Although the new detached-key test rejects missing, substitution with another valid snapshot key still bypasses it because candidate membership is absent from ReviewedDuplicateMergeSet and is never rederived from the reviewed digest. Bind the exact candidate/component membership into the verified review or recompute and validate it from immutable snapshot evidence before materializing operations.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Non-force two-parent restack onto current classification-audit parent. Preserve the duplicate-review manifest delta while inheriting the repaired Foundation CI contract through the parent chain. Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Current-stack correction (2026-09-05): exact base |
# Conflicts: # docs/PRD.md # docs/TRD.md
|
Fresh owner-stack reconciliation (2026-09-06): the named base branch Required repair: read/adopt the complete #11 successor delta, including the new #10 source-scope admission lineage and #11 derived-audit/provenance repairs, then normally non-force merge/restack it into this existing branch while preserving every #12 duplicate-review manifest/test/DDD delta. Resolve overlaps causally; do not drop either side, force-push, destructive-rebase, transfer predecessor GREEN, or mark review threads resolved from source adoption alone. Re-run Rust 1.98 locked workspace tests, strict fmt/Clippy/rustdoc, release/CI contract/diff and the owned coverage gate on one unchanged successor head before advancing descendants #13+. No source/base/head mutation is performed by this coordination comment; it leaves the current writer free to complete the normal restack. |
Reuse shared report admission; exact candidate membership remains independently bound. Remove subsumed key checks and move governance after all local decisions. Require v2 proposal identity on receipt and manifest, with no legacy default or approval issuance.
Receipt/source comparison remains before structural admission, and both remain before independent governance. Preserve existing exact error contracts without weakening newly added source and decision checks.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head read-only review found one additional acceptance defect independent of the stale-parent restack. This is a review finding, not approval or GREEN.
| { | ||
| return Err(DuplicateReviewError::SnapshotMismatch); | ||
| } | ||
| if !verify_review(reviewed) { |
There was a problem hiding this comment.
P1: complete local validation before invoking the governance verifier. At this exact head verify_review(reviewed) runs here, but blank/duplicate report.snapshot_items are rejected only below (lines 636–648), and later candidate/decision structural checks also occur after this call. A malformed untrusted report can therefore invoke the external authority boundary and receive UnverifiedApproval precedence before ConceptWeave has finished its own fail-closed validation. This is inconsistent with the repository’s existing local-check-before-authority contract and can spend/trigger verifier work on inputs ConceptWeave already knows are invalid.
RED: use a counting verifier and prove blank/duplicate snapshot identity plus each existing invalid candidate/decision structural case invoke it 0 times; a fully valid reviewed set invokes it exactly 1 time. Minimal GREEN: perform every deterministic local report/review/candidate/component check before verify_review; after adopting current parent #11 6dff8c2..., also run its shared validate_classification_report(report) (or an error-mapped equivalent) before the verifier so the parent’s forged-audit/provenance repair cannot be bypassed in this duplicate-owner path. Do not change authority semantics or turn local validation into approval.
1 participant
Current duplicate source-scope checkpoint — 2026-09-06
Exact head
3d2c2529af3c57c10d7e31e806b8e39dbf524efenormally preserves old #12a4a7c2d56fc592ef1c7abf64ca6875b0fe10c5eeand parent #116dff8c2ee42cfeb7bf8688c1f7e95989b61be266. Base remainsautoresearch/zotero-reclassification-sep04; OPEN Draft. Runtime repairfc0465e, regression additions0c825d9, test-only Clippy repair5fff9d0.Exact candidate membership was already independently bound by this owner, and remains so. RED
4656d6binstead reproduced three source-scope bypasses: invalid inventory accepted, changed standalone evidence reaching governance, and legacy receipt missing complete-scope binding. Required v2proposal_digestnow binds current proposals, retained metadata and pending sources in both reviewed set and output manifest. Recomputing it does not reuse independent approval. Receipt comparison preserves SnapshotMismatch precedence; shared report/audit validation and all local component/decision checks run before external governance. No duplicate semantics, connected-component evidence or rollback mapping was discarded.Exact-head locked tests87/17unfiltered suites including2doctests; strictClippy, warnings-denied rustdoc, release build, format, CI contract and diff checks passed. Independent source review reran12duplicate tests without actionable findings; this is not GitHub approval. Unchanged pinned coverage passed153/153functions1294/1294normalizedregions220/220normalizedbranches. Raw1649/1658lines2516/2536regions200/220branches are not100%. No added exclusion or dependency.
PRD/TRD/DDD/ProposedADR0006/Gap/CHANGELOG record the breaking receipt and source boundaries. Restored-report, worksheet and write consumers still require forward integration; no empty defaults, full-text downcast, capture rewrite or automatic approval transfer. Fresh visual evidence remains the preceding native Zotero screenshot+AX3719items observation, not a new reclassification or screenshot claim. No actual decision, approval, mutation, protected merge or release is claimed. Earlier checkpoints below are historical.
Latest bounded-read integration checkpoint
Exact head
a4a7c2d56fc592ef1c7abf64ca6875b0fe10c5eenormally merges parent1dc032598b41a35d52c09d8690c871e07365d7e3while retaining previous childee2c494a3136a4bfa520c29f1c938b621e1ecb9c. Base remainsautoresearch/zotero-reclassification-sep04. The #9 whole-snapshot elapsed-time repair and its RED/GREEN evidence are inherited without reverse-merging later features or discarding predecessor deltas.This exact head passes Rust 1.98.0 locked workspace tests=68 suites=17, including doctests and excluding filtered subprocess duplicates, strict all-target Clippy, warnings-denied rustdoc, formatting, the existing CI contract and diff checks. Log:
/private/tmp/conceptweave-deadline-pr12-20260906.log. Intermediate coverage is not inferred from owner/final-endpoint coverage. No new dependency, actual paper read/decision, Zotero mutation or authority issuer was used.Draft and protected prerequisites remain. Local tests are not hosted GREEN, independent approval, merged/released source or evidence for another head. Earlier checkpoints below are retained history, not the current head.
Current source-integrity note — 2026-09-05
autoresearch/zotero-duplicate-review-manifest@3bed88707b347a6db52ea9722f5a1c8f00933813.autoresearch/zotero-reclassification-sep04@082710e8c9a0e37ddae528e305cd75ab3574d0bb.e7d4e59f1b55b5954c5f8436527bc96e7ef2fb13through ordinary merge ancestry. The source-snapshot digest binds complete captured raw provider JSON and the actual typed classifier inputs; source evidence and derived proposals retain separate identities.GoldenSetApproval.proposal_digestis required and binds the complete proposal records used for evaluation. The current proposal digest is checked before the caller-owned governance verifier. Do not backfill old receipts: regenerate evidence and obtain a new approval bound to the reviewed evidence.Earlier heads, runtime snapshots, campaign counts, and verification statements below are historical notes, not current acceptance evidence.
Historical PR notes — original text retained
Outcome
Adds a Foundation-dependent, local-only reversible duplicate-review manifest for Issue #8. Reviewed duplicate decisions bind to an opaque authority receipt plus exact library/rule/raw-snapshot identity and produce deterministic before/after/rollback canonical-key mappings while preserving every Zotero source record. This branch performs no Zotero mutation or deletion.
Current exact stack — 2026-09-05
autoresearch/zotero-reclassification-sep04@cb365fbf175ae6588aa6176cf9e99ae4d85fcdca;5ce1a186e0235843abc6e39ebedbb9a4d5501320;5ce1a18....Review -> test -> repair lineage
The current lineage enforces the reviewed duplicate-manifest invariants at the source boundary:
reviewed.snapshot_itemsmust exactly equalreport.snapshot_items, binding item keys and exact item revisions to the verified review rather than re-reading mutable report coordinates after approval;reviewed.duplicate_candidatesmust exactly equalreport.duplicate_candidates, so a caller cannot substitute another valid snapshot key into a reviewed candidate while retaining the old authority receipt;Previously returned review threads remain intentionally unresolved where they require execution evidence. A restack does not convert source inspection or predecessor evidence into GREEN.
Existing manifest boundary
The manifest binds decisions to review/authority receipts, exact library version, rule revision, raw snapshot digest, exact snapshot item coordinates and exact duplicate-candidate membership; requires one decision per candidate; rejects unknown/repeated decisions; retains source revisions; emits deterministic before/after/rollback canonical-key mappings; and marks source records preserved.
Reviewed Duplicate Merge Setis one steward decision set over a connected duplicate component. Governance & Publication remains the authority boundary. Candidate duplicate evidence does not become semantic truth merely because deterministic mappings can be generated.Issue #8 AC4 remains partial until real duplicate groups receive actual steward decisions through the governance boundary.
Dependency boundary
Keep Draft behind #11 -> #10 -> #9 -> Foundation. #13 and later descendants must retain the current #12 ancestry. Downstream write-plan or transport work does not back-prove this manifest. Do not perform Zotero write-back, promote candidate classification to semantic authority, resolve valid review findings before their required verification, or transfer predecessor/cancelled evidence.
Merge gate
Require one unchanged exact head with terminal Product/security/SAST/dependency/review evidence and zero valid unresolved findings. No self-approval, routine bypass, force-push, destructive rebase, mutable foreign dependency, or gate weakening.
Refs #8.