feat(membership): refuse episode membership outside the episode - #146
Conversation
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthrough
Changes에피소드 멤버십 검증
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The recovery calculation can overflow for extremely large valid inputs, potentially causing a panic or incorrect recovery rate. The PR is otherwise mergeable with explicit owner awareness or follow-up to use a size-safe counter. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Current head |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head validation update (5a05b8f): fixed the quality contract to derive the Rust crate count from scripts/check_workspace_contract.py instead of hard-coding 10. Local evidence: 89 quality tests passed; coverage 100% (991/991 statements, 442/442 branches); workspace, docstring, documentation, and diff checks passed. Please review and rerun Checks against this exact head; merge remains subject to the repository's two independent approvals and protected rules. |
|
Current-head review fix (ecaa8bc): Added the missing APA 7 Allen (1983) reference to the same standards-and-literature register as its in-text citation. Local evidence: 89 quality tests, 100% statement/branch coverage (991/991, 442/442), workspace/docstring/documentation contracts, and diff check pass. Please re-review this exact head. |
|
@opencode-agent @cwl-noema-review Review-only request for exact current head |
|
Exact-head failure RCA for ecaa8bc: Strix produced Vulnerabilities 0, then failed because the local Caido guest bootstrap could not connect to 127.0.0.1:48080. This is a central Strix runtime-infrastructure failure, not a source vulnerability. The narrow fail-closed classifier repair is in ContextualWisdomLab/.github#1181; do not treat this old failed run as a clean security verdict until the central fix is merged and this exact head is rerun. |
|
Queued @cwl-noema-review and @opencode-agent for PR #146 at head |
|
Root-cause owner update: central PR #1181 was closed as superseded, and canonical owner PR #1153 now carries the fix. Its current exact head is ; the Strix failure on this path was a real Medium diagnostic-disclosure finding, now repaired by allowlisting safe HTTP methods and redacting untrusted methods. Do not rerun unchanged TEPP Strix until the central fix is merged into protected main; then revalidate this PR's exact HEAD. |
|
Root-cause owner update: central |
|
Current exact-head triage: |
|
Current-head review refresh for ecaa8bc:
|
|
Rebased current head 5419b0f onto origin/main. The changelog conflict retains both feature and current-main entries; inherited documentation trailing whitespace was removed. Local merge-tree, git diff --cached --check, and cargo fmt --all -- --check pass. Exact-head hosted checks and required independent approvals remain required before protected merge; the prior Strix infrastructure failure is tracked separately from TEPP source. |
|
Current-head disposition for the Devin finding on 5419b0f: the full APA 7 Allen (1983) reference is already present in docs/research/standards-and-literature.md in the exact current tree, matching the episode-membership doctoring. The finding is stale and needs no source patch. Please refresh independent review for 5419b0f only; no approval or merge bypass is being issued. |
|
Current-head review request: exact head |
Exact-head queue dispositionExact head |
|
Current-head maintenance update for exact head 58a5eae against protected main e65cd66. Normal restack completed; no force push or bypass was used. Removed tracked generated .codegraph/codegraph.db; .codegraph/.gitignore remains the only tracked CodeGraph artifact. Preserved the episode-membership contract: inverted windows fail closed, equal and contained boundaries pass, and recovery is compared with known truth. Exact merge-result local verification passed: workspace contract, Rust documentation contract, documentation validation, 140 Python quality tests, cargo fmt, workspace tests, clippy, release build, cargo doc, cargo deny advisories bans licenses sources, Rust line coverage 4927/4927 100%, and Rust branch coverage 1850/1850 100% with the repository-approved sqlx_live.rs exclusion. Secret-scan findings are unchanged pre-existing test fixtures and credential-shaped literals outside this change. Hosted Checks and independent review must be refreshed for this exact head; no approval or merge bypass is being issued. Decision: WAIT_AND_REMEDIATE. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # ARCHITECTURE.md # CHANGELOG.md # Cargo.toml # README.md # crates/episode_membership/src/window.rs # docs/TRACEABILITY.md # docs/adr/0003-relational-event-multiple-membership.md # docs/adr/0004-shared-multilingual-latent-space.md # docs/adr/0012-temporal-relational-shared-latent-topic-measurement.md # docs/adr/README.md # docs/research/standards-and-literature.md # docs/validation/temporal-event-foundation.md # scripts/check_workspace_contract.py # tests/quality/test_check_docstrings.py
| "interpretation_gateway", | ||
| "model_selection", | ||
| "checkpoint_authority", | ||
| "episode_membership", |
There was a problem hiding this comment.
🔴 Duplicate crate entry fails the contract tests
EXPECTED_CRATES lists episode_membership twice. test_live_repository_is_documented asserts the inventory has no duplicates and that its length equals the count of on-disk crate roots. Both assertions fail, turning the contract gate red.
| "episode_membership", | |
| "compute_backend", |
Was this helpful? React with 👍 or 👎 to provide feedback.
| "crates/interpretation_gateway", | ||
| "crates/model_selection", | ||
| "crates/checkpoint_authority", | ||
| "crates/episode_membership", |
There was a problem hiding this comment.
🟡 episode_membership listed twice in workspace manifest
crates/episode_membership is added to members while the same path already exists a couple lines below, and the same duplication is repeated in default-members. The crate is registered twice in both lists.
| "crates/episode_membership", | |
| "crates/compute_backend", |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let matches = truth | ||
| .iter() | ||
| .zip(decided) | ||
| .filter(|(truth_flag, decided_flag)| truth_flag == decided_flag) | ||
| .count(); | ||
| Ok(matches as f64 / truth.len() as f64) | ||
| let matches = count_matching_decisions(truth.iter().copied().zip(decided.iter().copied())); | ||
| Ok(recovery_rate_from_tally(matches, truth.len())) |
There was a problem hiding this comment.
🔴 Duplicated body in recovery-rate function breaks the build
identity_recovery_rate computes and returns the rate inline, then a leftover second copy of the computation follows that return expression. The inline Ok(...) has no semicolon before the following let, so the crate fails to compile.
| let matches = truth | |
| .iter() | |
| .zip(decided) | |
| .filter(|(truth_flag, decided_flag)| truth_flag == decided_flag) | |
| .count(); | |
| Ok(matches as f64 / truth.len() as f64) | |
| let matches = count_matching_decisions(truth.iter().copied().zip(decided.iter().copied())); | |
| Ok(recovery_rate_from_tally(matches, truth.len())) | |
| let matches = count_matching_decisions(truth.iter().copied().zip(decided.iter().copied())); | |
| Ok(recovery_rate_from_tally(matches, truth.len())) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| The workspace bounded crates compile independently but intentionally expose no | ||
| The workspace bounded crates compile independently; Task 1 includes the | ||
| The twelve bounded crates compile independently but intentionally expose no | ||
| The eleven bounded crates compile independently; Task 1 includes the | ||
| implemented `encrypted_mapping` crate with AES-256-GCM sealing and | ||
| purpose-bound opening, while the remaining domain behavior begins in Task 2 | ||
| with immutable evidence identifiers and source records. | ||
| The eleven bounded crates compile independently. `derived_sensitivity` inherits | ||
| The workspace bounded crates compile independently. `derived_sensitivity` inherits | ||
| source Restricted/Internal classes onto topic, factor, and relation artifacts | ||
| and fails closed on unknown kinds; derivation and blanket PII masking are not | ||
| declassification. Other crates still begin domain behavior in Task 2 with | ||
| immutable evidence identifiers and source records. | ||
| The workspace bounded crates compile independently but intentionally expose no |
There was a problem hiding this comment.
📝 Info: Dangling duplicated README paragraphs
README.md gains several truncated, duplicated sentence fragments (e.g. "The workspace bounded crates compile independently but intentionally expose no") from the same merge that duplicated the crate registrations. Documentation cleanup, not a code bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # CHANGELOG.md # README.md # docs/adr/0012-temporal-relational-shared-latent-topic-measurement.md
# Conflicts: # CHANGELOG.md
| let cases = [ | ||
| (10, 20, 10, 18, true), | ||
| (30, 40, 32, 40, true), | ||
| (50, 60, 50, 60, true), | ||
| (70, 80, 69, 75, false), | ||
| (90, 100, 95, 101, false), | ||
| (110, 120, 112, 118, true), | ||
| ]; | ||
| let mut truth = Vec::with_capacity(cases.len()); | ||
| let mut recovered = Vec::with_capacity(cases.len()); | ||
| for (episode_start, episode_end, member_start, member_end, expected) in cases { | ||
| let episode = EventWindow::new(episode_start, episode_end).expect("episode"); | ||
| let membership = EventWindow::new(member_start, member_end).expect("membership"); | ||
| assert!(episode.start() <= episode.end()); | ||
| truth.push(expected); | ||
| recovered.push(refuse_membership_outside_episode(membership, episode).is_ok()); | ||
| } | ||
| let collapsed = vec![true; truth.len()]; | ||
| let recovered_rate = identity_recovery_rate(&truth, &recovered).expect("recovered"); | ||
| let collapsed_rate = identity_recovery_rate(&truth, &collapsed).expect("collapsed"); | ||
| let expected = { | ||
| let mut matches = 0_u32; | ||
| for (truth_flag, decided_flag) in truth.iter().zip(recovered.iter()) { | ||
| if truth_flag == decided_flag { | ||
| matches += 1; | ||
| } | ||
| } | ||
| f64::from(matches) / f64::from(u32::try_from(truth.len()).expect("len")) | ||
| }; | ||
| assert!((recovered_rate - expected).abs() < f64::EPSILON); | ||
| assert!((recovered_rate - 1.0).abs() < f64::EPSILON); | ||
| assert!((collapsed_rate - (2.0 / 3.0)).abs() < f64::EPSILON); |
There was a problem hiding this comment.
📝 Info: Recovery-rate test values are consistent
The rewritten test derives recovered from refuse_membership_outside_episode and expects collapsed_rate of 2/3. The six cases split 4 contained / 2 escaping, so the all-true collapse matches 4/6 = 2/3, consistent with equal-bound containment.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
A document may belong to an episode only while that episode is active in event time (ADR 0003). Membership cannot start before or end after the episode interval. Recovery is the computed share of containment flags that match known truth versus accepting every membership.
Complementary to #118 (`subevent_containment`: child event versus parent event). This crate owns episode-membership containment.
Local gates:
Does not allocate migration `0008`. Does not recreate `stopword_deletion` (#145), `psychometric_core` multilevel (#144), or other in-flight crates.
Keep this PR draft. Preferred merge remains #46 only when exact-head required Checks pass and a qualifying independent (non-Cursor/CodeRabbit) APPROVE exists. Do not empty-commit.
Summary by CodeRabbit
새로운 기능
문서
테스트