feat(analysis): bind membership-target refusals to an analysis-run profile - #434
feat(analysis): bind membership-target refusals to an analysis-run profile#434seonghobae wants to merge 3 commits into
Conversation
…ofile GAP-004 leftover / ADR 0069. Bind existing MembershipTargetKind and refuse_collapsed_target to cutoff-safe membership_target_v1. Language, episode, template, department, and opportunity-pool targets are not entities; identity_recovery_rate stays library-side.
|
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 |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Operator-visible GAP-004 leftover / ADR 0069. Binds existing membership_target::MembershipTargetKind + refuse_collapsed_target to membership_target_v1 (tepp.membership_target.v1). Inference language_episode_template_department_opportunity_pool_are_not_entities. Distinct from #430 location-membership, #398 membership-posterior ICC, #427 copied-text, #416 copy-identity. identity_recovery_rate stays library-side. Two independent current-head APPROVEs required. Author/bot COMMENTED is not APPROVE. Exact-head Checks on c0fbaab only. Do not self-approve.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| document_id: String, | ||
| kind: MembershipTargetKind, | ||
| } |
There was a problem hiding this comment.
🔴 Historical censuses admit future evidence
Any MembershipTargetDocument is counted without an availability timestamp or cutoff check. Future-available documents can contaminate historical results.
Prompt for agents
Add availability provenance to MembershipTargetDocument and enforce cutoff eligibility inside execute_membership_target_run before counting. The executor must not trust an unbound KnowledgeCutoff argument as proof that every supplied document was available. Bind each document to an AvailableTime or a validated snapshot type, exclude or fail on documents later than the request cutoff, and add tests containing both eligible and future-available documents.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if !seen.insert(document.document_id()) { | ||
| return Err(AnalysisEngineError::DuplicateEvidence); | ||
| } |
There was a problem hiding this comment.
🟡 Multiple memberships fail as duplicates
When one document has several memberships, seen.insert rejects its second target as duplicate. Valid multiple-membership analyses fail.
Prompt for agents
Represent multiple target assignments for one document without treating them as duplicate evidence. In execute_membership_target_run, distinguish duplicate rows from distinct target kinds for the same document, likely by keying duplicate detection on (document_id, kind) or by modeling one document with a set of kinds. Define whether document_count counts unique documents or assignments, update per-kind and refusal invariants accordingly, and test one document assigned simultaneously to language, template, opportunity pool, episode, entity, and project targets.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut seen = std::collections::BTreeSet::new(); | ||
| let mut language_count = 0_u64; | ||
| let mut episode_count = 0_u64; | ||
| let mut template_count = 0_u64; | ||
| let mut department_count = 0_u64; | ||
| let mut opportunity_pool_count = 0_u64; | ||
| let mut entity_count = 0_u64; | ||
| let mut project_count = 0_u64; | ||
| let mut refused_as_entity_count = 0_u64; | ||
| let mut refused_as_project_count = 0_u64; | ||
| for document in documents { |
There was a problem hiding this comment.
🟡 Membership runs bypass corpus limits
execute_membership_target_run scans every supplied document without enforcing the engine’s 100,000-unit bound. Oversized runs consume unbounded time and memory.
Prompt for agents
Enforce the analysis engine's MAX_EVIDENCE_UNITS bound before allocating the duplicate-detection set or scanning documents in execute_membership_target_run. Return AnalysisEngineError::LimitExceeded for oversized input and add a boundary test for MAX_EVIDENCE_UNITS + 1.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let summary = AnalysisResultSummary::new( | ||
| "membership_target", | ||
| document_count, | ||
| 4, | ||
| MEMBERSHIP_TARGET_INFERENCE_STATUS, | ||
| )?; |
There was a problem hiding this comment.
🔍 Statistic count lacks defined meaning
The summary reports four statistics without mapping them to artifact fields. Define the count’s semantics so consumers can interpret it consistently.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR adds a membership-target analysis-run profile, but it does not address the three blocking issues raised in prior review threads: (1) MembershipTargetDocument carries no availability timestamp, so the executor counts future-available documents as historical evidence; (2) duplicate detection keys on document_id alone, so a single document with multiple membership kinds is rejected as DuplicateEvidence; (3) execute_membership_target_run never enforces MAX_EVIDENCE_UNITS, allowing unbounded scans. These are confirmed by source inspection and the existing tests. The artifact validation and refusal logic are otherwise sound, but the executor's core invariants are incomplete.
Reviewed changed lines
crates/analysis_engine/src/membership_target_artifact.rs:29 (RIGHT): MembershipTargetDocument contains only document_id and kind. There is no availability timestamp or cutoff-eligibility field. The executor accepts a KnowledgeCutoff argument but never checks per-document availability, so any document supplied is counted regardless of whether it was available at the cutoff. This violates the 'cutoff-safe' claim and the ADR 0022 contract.crates/analysis_engine/src/membership_target_artifact.rs:224 (RIGHT): The duplicate-detection setseenis keyed ondocument.document_id()only. If one document has multiple membership kinds (e.g., language and entity), the second row is rejected as DuplicateEvidence. The ADR and changelog claim multiple-membership aggregation, but the implementation collapses them. The testempty_single_class_and_duplicate_identities_fail_closedexplicitly expects DuplicateEvidence for the same id with different kinds, confirming the behavior.crates/analysis_engine/src/membership_target_artifact.rs:221 (RIGHT): The executor iterates overdocumentswithout any length check against MAX_EVIDENCE_UNITS (100,000). The engine's global bound is defined in lib.rs but not enforced here. An oversized input will allocate a BTreeSet of unbounded size and scan all rows, causing unbounded time and memory. The prior review thread flagged this and the code does not address it.crates/analysis_engine/src/membership_target_artifact.rs:319 (RIGHT): The summary uses a hardcoded statistic count of 4, but the artifact exposes 11 count fields (document_count, language_count, episode_count, template_count, department_count, opportunity_pool_count, entity_count, project_count, refused_as_entity_count, refused_as_project_count, plus inference_status). The meaning of '4' is undefined and inconsistent with the artifact schema. This is a maintainability/contract issue.
Adversarial validation
crates/analysis_engine/src/membership_target_artifact.rs:29 (RIGHT)confirmed: A document with no availability timestamp is counted even if it was not available at the knowledge cutoff. — Source: MembershipTargetDocument has only document_id and kind; execute_membership_target_run takes knowledge_cutoff but never compares it to any per-document time. The loop counts every document unconditionally.crates/analysis_engine/src/membership_target_artifact.rs:224 (RIGHT)confirmed: A single document with two different membership kinds is rejected as duplicate. — The integration testempty_single_class_and_duplicate_identities_fail_closedconstructs exactly this case and asserts Err(DuplicateEvidence). The code path is confirmed.crates/analysis_engine/src/membership_target_artifact.rs:221 (RIGHT)confirmed: An input larger than MAX_EVIDENCE_UNITS is not rejected and causes unbounded work. — Source: noif documents.len() > MAX_EVIDENCE_UNITScheck exists in execute_membership_target_run. The constant is defined in lib.rs but unused here. The loop runs unconditionally.- Residual risk: The three blocking issues are confirmed by source inspection. The executor cannot be trusted to produce cutoff-safe, multi-membership, bounded results until they are fixed.
Findings
-
[high] crates/analysis_engine/src/membership_target_artifact.rs:29 (RIGHT): MembershipTargetDocument lacks availability provenance. The executor counts every supplied document without checking whether it was available at the knowledge cutoff, allowing future-available documents to contaminate historical results. Add an AvailableTime or validated snapshot timestamp to each document and enforce cutoff eligibility before counting.
-
[high] crates/analysis_engine/src/membership_target_artifact.rs:224 (RIGHT): Duplicate detection keys on document_id alone, so a single document with multiple membership kinds is rejected as DuplicateEvidence. Key on (document_id, kind) or model one document with a set of kinds, and define whether document_count counts unique documents or assignments.
-
[high] crates/analysis_engine/src/membership_target_artifact.rs:221 (RIGHT): execute_membership_target_run does not enforce MAX_EVIDENCE_UNITS. Oversized inputs cause unbounded time and memory. Add a length check before allocating the duplicate-detection set or scanning documents, returning AnalysisEngineError::LimitExceeded.
-
[low] crates/analysis_engine/src/membership_target_artifact.rs:319 (RIGHT): The summary statistic count of 4 is hardcoded and does not correspond to the artifact's 11 count fields. Define the semantics of the statistic count or derive it from the artifact schema to avoid consumer confusion.
-
Result: REQUEST_CHANGES
-
Head SHA:
c0fbaabd8c95e69407c3b9e50f8d1846bd949598 -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
|
Hour-35 exact-head review request. Current head @opencode-agent review |
|
Hour-36 re-verify: membership-target head c0fbaab. noema CHANGES_REQUESTED is not independent APPROVE; do not weaken fail-closed to satisfy it. Distinct from this hour's #458 outcome-order (AvailableTime + MAX_EVIDENCE_UNITS already on that slice). Zero independent APPROVEs. Exact-head Checks on c0fbaab only. Do not self-approve. @opencode-agent review. |
Preserve #416 copy-identity, inferred-status, location-membership, episode-membership, and subevent-containment source/tests with #434 membership-target profile evidence. Shared Cargo/lib/lock/docs surfaces are unioned; neither side is discarded. Per-profile ADR numbers remain implementation lineage pending #437. # Conflicts: # CHANGELOG.md # DOCUMENTATION.md # crates/analysis_engine/Cargo.toml # crates/analysis_engine/src/lib.rs # docs/TRACEABILITY.md # docs/adr/README.md
|
|
1 participant
Consolidation status
fold_into_landing_vehicle— this PR binds one existingmembership_targetrefusal family into an Analysis Run profile. That is implementation evidence inside the Validation / Analysis Run boundary, not an independently shippable bounded context. Preserve its typed target distinctions, digest-bound census, execution/artifact bounds, metric-free inspect boundary, duplicate/profile/snapshot/cutoff refusals, and focused tests, but do not merge it independently while queue-authority recovery is active. Fold these mechanics with compatible refusal profiles into the coherent Validation / Analysis Run landing vehicle selected under #435. ADR 0069 is implementation evidence pending #437 normalization, not per-rule architecture authority.Summary
GAP-004 leftover / ADR 0069. Bind existing
membership_target::MembershipTargetKindandrefuse_collapsed_targetto a cutoff-safemembership_target_v1analysis-run profile (tepp.membership_target.v1).language_episode_template_department_opportunity_pool_are_not_entities.identity_recovery_ratestays library-side; inspect payloads stay metric-free (scientific_acceptancenever appears).MAX_EVIDENCE_UNITSnow applies both to execution input and to deserialized/serialized artifact counts, so a compact forged JSON artifact cannot claim an impossible corpus size.Not GPU. Not MCMC. Not topic birth/split/merge. Not implemented-main.
RED -> GREEN repair evidence
d3fa7276d4e0f0382d2c65e5404438ef26bdf67a: requires an internally consistent compact artifact withdocument_count > MAX_EVIDENCE_UNITSto fail both serialization validation and deserialization validation, and requires an execution corpus ofMAX_EVIDENCE_UNITS + 1documents to failLimitExceededbefore census work.a976c99255073c5fe08b4d850833e068fca11110: imports the shared bound into this profile, rejects oversized execution slices before allocation/census work, and enforces the same bound inMembershipTargetArtifact::validate().Verification
Fresh exact-head hosted verification is required on
a976c99255073c5fe08b4d850833e068fca11110; queued/pending/predecessor evidence is non-passing. Folded landing-vehicle verification must preserve these tests.Merge gate
Do not merge this micro-profile independently during queue recovery. Preserve its unique tests/evidence in the folded landing vehicle, then satisfy that exact head's active ruleset, required workflows, review-thread resolution, and current review policy.