feat(analysis): bind location-membership refusals to an analysis-run profile - #430
Conversation
…profile GAP-004 leftover / ADR 0066. Bind existing location_membership refusals (refuse_location_as_entity_identity, refuse_location_as_language_channel) to cutoff-safe location_membership_v1. identity_recovery_rate stays library-side. Distinct from membership-posterior ICC (#398), copied-text (#427), citation-edge (#426), and corpus-background (#422). Not GPU, not MCMC, and not topic birth/split/merge.
![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.
📝 WalkthroughWalkthrough
Changes위치 멤버십 분석 실행
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The new analysis-run profile can count documents newer than the declared knowledge cutoff, accepts unbounded document inputs, and does not bind each document to the stated tenant, snapshot, and cutoff. This could produce historically invalid or resource-heavy results and, for insufficiently constrained callers, mislabeled artifacts; merge should wait for these safeguards or explicit owner acceptance. Sequence Diagram(s)sequenceDiagram
participant Client
participant execute_location_membership_run
participant location_membership
participant LocationMembershipArtifact
Client->>execute_location_membership_run: 실행 요청과 문서 전달
execute_location_membership_run->>location_membership: 문서 위치 멤버십 검증
location_membership-->>execute_location_membership_run: 검증 결과 반환
execute_location_membership_run->>LocationMembershipArtifact: 거부 집계와 실행 메타데이터 기록
LocationMembershipArtifact-->>execute_location_membership_run: SHA-256 digest 반환
execute_location_membership_run-->>Client: 아티팩트와 terminal result 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 56.52% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 3 files. (7 skipped: 7 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
![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: 8dc7f87fd7
ℹ️ 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".
| pub struct LocationMembershipDocument { | ||
| document_id: String, | ||
| kind: LocationKind, |
There was a problem hiding this comment.
When a caller supplies evidence that became available after the requested cutoff, it is still counted because LocationMembershipDocument carries only an ID and kind, while the executor merely compares the request cutoff with a caller-supplied cutoff value. The public constructor therefore cannot substantiate the claimed cutoff admission; include validated availability metadata or a cutoff-bound snapshot proof and reject/exclude late evidence before producing the artifact.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| for document in documents { | ||
| if !seen.insert(document.document_id()) { |
There was a problem hiding this comment.
When an untrusted request supplies a very large document slice, this loop performs unbounded work and grows the duplicate-detection BTreeSet without the MAX_EVIDENCE_UNITS guard used by the existing analysis path; the fixed-size output artifact does not constrain this input cost. Reject slices above an explicit execution bound before entering the loop to prevent CPU and memory exhaustion.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| pub struct LocationMembershipDocument { | ||
| document_id: String, | ||
| kind: LocationKind, |
There was a problem hiding this comment.
When one document has two active location assignments or belongs to multiple membership roles, this singular kind cannot represent the required structure, and submitting multiple rows for the document is rejected later as duplicate evidence. Representing the document once therefore undercounts its assignments, while representing each assignment fails the run; consume explicit membership assignments or a membership network that preserves group, weight, and validity information.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
![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.
| if request.knowledge_cutoff != knowledge_cutoff.to_rfc3339() | ||
| || request.model_contract_version != LOCATION_MEMBERSHIP_MODEL_CONTRACT_VERSION | ||
| || request.output_profile != LOCATION_MEMBERSHIP_OUTPUT_PROFILE | ||
| { | ||
| return Err(AnalysisEngineError::InvalidEvidence); | ||
| } |
There was a problem hiding this comment.
🟡 Valid cutoff spellings reject execution
An accepted RFC 3339 cutoff using an offset or redundant fractional zeros fails execute_location_membership_run. Equivalent cutoff instants cannot run this profile.
| if request.knowledge_cutoff != knowledge_cutoff.to_rfc3339() | |
| || request.model_contract_version != LOCATION_MEMBERSHIP_MODEL_CONTRACT_VERSION | |
| || request.output_profile != LOCATION_MEMBERSHIP_OUTPUT_PROFILE | |
| { | |
| return Err(AnalysisEngineError::InvalidEvidence); | |
| } | |
| let request_cutoff = KnowledgeCutoff::parse_rfc3339(&request.knowledge_cutoff) | |
| .map_err(|_| AnalysisEngineError::InvalidEvidence)?; | |
| if request_cutoff.instant() != knowledge_cutoff.instant() | |
| || request.model_contract_version != LOCATION_MEMBERSHIP_MODEL_CONTRACT_VERSION | |
| || request.output_profile != LOCATION_MEMBERSHIP_OUTPUT_PROFILE | |
| { | |
| return Err(AnalysisEngineError::InvalidEvidence); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| || self.document_count < 2 | ||
| || self.location_count == 0 |
There was a problem hiding this comment.
🟡 Oversized artifacts pass validation
from_json accepts consistent counts above the 100,000-document execution limit. Consumers can accept artifacts that this profile cannot produce.
| || self.document_count < 2 | |
| || self.location_count == 0 | |
| || self.document_count < 2 | |
| || self.document_count > MAX_EVIDENCE_UNITS as u64 | |
| || self.location_count == 0 |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let _ = refuse_location_as_entity_identity(document.kind()); | ||
| let _ = refuse_location_as_language_channel(document.kind()); | ||
| refused_as_entity_identity_count += 1; | ||
| refused_as_language_channel_count += 1; |
There was a problem hiding this comment.
🔍 Refusal results do not bind counts
Both refusal results are discarded before counters advance. A changed refusal contract can silently produce false successful-refusal counts instead of failing closed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| uuid.workspace = true | ||
|
|
||
| [dev-dependencies] | ||
| location_membership = { path = "../location_membership", version = "0.2.0" } |
There was a problem hiding this comment.
🔍 Dependency declared twice
location_membership appears in both dependency sections with identical settings. Tests already inherit production dependencies, so the duplicate can drift during maintenance.
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 digest-bound location-membership refusal profile with cutoff-safe availability and a 100k-document bound, and tests cover many fail-closed paths. However, three concrete issues remain: (1) the executor compares the request cutoff as a raw string against the canonical RFC3339 form, rejecting valid equivalent spellings such as offsets or redundant fractional zeros; (2) the refusal function results are discarded, so a changed refusal contract can silently produce false successful-refusal counts instead of failing closed; (3) artifact validation does not enforce the MAX_EVIDENCE_UNITS upper bound, allowing consumers to accept artifacts this profile cannot produce. These are confirmed behavioral defects that should be fixed before merge.
Reviewed changed lines
crates/analysis_engine/src/location_membership_artifact.rs:212 (RIGHT): The executor comparesrequest.knowledge_cutoff(a String) directly toknowledge_cutoff.to_rfc3339(). This rejects any valid RFC3339 spelling that is not byte-identical to the canonical form, e.g.2026-08-01T00:00:00+00:00or2026-08-01T00:00:00.000Z, even though they represent the same instant. The cutoff should be parsed and compared by instant.crates/analysis_engine/src/location_membership_artifact.rs:235 (RIGHT): The results ofrefuse_location_as_entity_identityandrefuse_location_as_language_channelare discarded withlet _ =. If either function returns an error (e.g., because the location cannot be refused under the current contract), the code still increments the refusal counters and emits a successful artifact. This is fail-open and can produce false successful-refusal counts.crates/analysis_engine/src/location_membership_artifact.rs:160 (RIGHT):LocationMembershipArtifact::validatechecksdocument_count < 2but does not enforce an upper bound. A tampered or malformed artifact withdocument_countaboveMAX_EVIDENCE_UNITSpasses validation, even though the executor can never produce such an artifact. Consumers could accept oversized artifacts.crates/analysis_engine/Cargo.toml:28 (RIGHT):location_membershipis declared in both[dependencies]and[dev-dependencies]with identical settings. Tests already inherit production dependencies, so the duplicate is redundant and can drift during maintenance.
Adversarial validation
crates/analysis_engine/src/location_membership_artifact.rs:212 (RIGHT)confirmed: A request with a valid RFC3339 cutoff that is not byte-identical to the canonical form (e.g., with an offset or redundant fractional zeros) is rejected. — Source trace:if request.knowledge_cutoff != knowledge_cutoff.to_rfc3339()performs a direct string equality check. The two strings differ, so the condition is true and the function returnsInvalidEvidence.crates/analysis_engine/src/location_membership_artifact.rs:235 (RIGHT)confirmed: Ifrefuse_location_as_entity_identityorrefuse_location_as_language_channelreturns an error, the executor still increments the refusal counters and emits a successful artifact. — Source trace: theLocationKind::Locationarm calls both refusal functions withlet _ =and then increments both refusal counters without checking the results. The counters are later used to build the artifact, which passes validation because the refusal counts equallocation_count.crates/analysis_engine/src/location_membership_artifact.rs:160 (RIGHT)confirmed: An artifact withdocument_countgreater thanMAX_EVIDENCE_UNITSpasses validation. — Source trace:validate()checksself.document_count < 2but has noself.document_count > MAX_EVIDENCE_UNITScondition. The artifact is accepted.- Residual risk: The cutoff string comparison and refusal-result discard are confirmed defects. The artifact upper-bound gap is also confirmed. These should be fixed and covered by tests before merge.
Findings
-
[high] crates/analysis_engine/src/location_membership_artifact.rs:235 (RIGHT): Refusal function results are discarded; a changed refusal contract can silently produce false successful-refusal counts instead of failing closed. Check the results and propagate errors before incrementing counters.
-
[medium] crates/analysis_engine/src/location_membership_artifact.rs:212 (RIGHT): The request cutoff is compared as a raw string to the canonical RFC3339 form, rejecting valid equivalent spellings (offsets, redundant fractional zeros). Parse the request cutoff and compare instants.
-
[medium] crates/analysis_engine/src/location_membership_artifact.rs:160 (RIGHT): Artifact validation does not enforce the MAX_EVIDENCE_UNITS upper bound, allowing consumers to accept artifacts this profile cannot produce. Add
document_count > MAX_EVIDENCE_UNITS as u64to the validation. -
[low] crates/analysis_engine/Cargo.toml:28 (RIGHT):
location_membershipis declared in both[dependencies]and[dev-dependencies]with identical settings. Remove the duplicate dev-dependency entry. -
Result: REQUEST_CHANGES
-
Head SHA:
bafef7ea37c699c629985724c4080d50108a720e -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
1 participant
Consolidation status
fold_into_landing_vehicle— this profile is stacked directly on provisional Validation / Analysis Run landing vehicle #416 (feat/copy-identity-analysis-run-gap-004). The retarget is queue consolidation, not a request to land #430 independently.Current base:
#416@e0e44805acf3a5ec833e83baa37cc72c80544514. Current exact head:8bbe557bb63f632c4edddb0d3c3bdd96fe54f9f3. GitHub reports the stack non-mergeable because #416 and this branch both modify shared Analysis Run integration/registry surfaces (Cargo.toml,lib.rs,Cargo.lock, CHANGELOG/DOCUMENTATION/TRACEABILITY/ADR index). Preserve #416's copy-identity + inferred-status repairs and this branch's location-membership implementation/tests; do not overwrite either side, force-push, or merge this child independently.Current repair
Fresh consolidation review found the same RFC 3339 textual-equality defect already repaired in copy-identity and inferred-status:
execute_location_membership_runcomparedrequest.knowledge_cutofftext directly with the executor cutoff's canonical string, rejecting semantically identical instants with different offsets.3b6049d63064cd8783fa0b2f3b589b2020fe32ebadds a focused contract requiring2026-08-01T09:00:00+09:00to be admitted against executor cutoff2026-08-01T00:00:00Z.8bbe557bb63f632c4edddb0d3c3bdd96fe54f9f3parses the request cutoff asKnowledgeCutoffand comparesinstant()values, while malformed or genuinely different cutoffs still fail closed through the existing contract.Earlier RED
a4b57c80b6295b276b6180657647ecdfc2f76b1d/ GREEN5b5791b51705404eb2a650d4bc3278a1bc5e3fa8retain compact impossible-census refusal and the canonical output byte bound.Unique profile evidence to preserve
location_membership_v1/tepp.location_membership.v1bindslocation_membership::refuse_location_as_entity_identityandrefuse_location_as_language_channelto cutoff-safe Analysis Run execution. Mixed location/entity/language-channel corpora emit a digest-bound census;identity_recovery_ratestays library-side; inspect output remains metric-free;MAX_EVIDENCE_UNITSapplies to execution and artifact validation; empty/single-class/no-location, duplicate identity, profile/snapshot/cutoff mismatch and hostile inputs fail closed.DDD / landing rule
Location membership remains source-domain vocabulary; Analysis Run owns request admission, cutoff-safe evidence composition, digest-bound terminal projection and claim refusal. This profile does not create a bounded context. ADR 0066 remains implementation lineage pending repository-wide normalization under #437.
Next source action is the real shared-file fold: construct one #416 successor head containing copy-identity, inferred-status, and location-membership source/tests/doctoring, resolve shared Cargo/lib/docs/lock surfaces, then reacquire exact-head Rust/documentation/security/SAST and qualifying review under the live organization ruleset. Predecessor checks and reviews do not transfer.