feat(analysis): bind simulation method-effect labels to an analysis-run profile - #415
feat(analysis): bind simulation method-effect labels to an analysis-run profile#415seonghobae wants to merge 2 commits into
Conversation
…un profile GAP-004 leftover / ADR 0057. Bind existing tepp_simulation::generate and refuse_unavailable_document to cutoff-safe method_effects_v1. Census of original/revision/translation/template_copy labels, not an estimator-side method model, not GPU, not MCMC, and not topic birth/split/merge.
|
Warning Review limit reachedNext included review available in 25 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: Team Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough
Changes메서드 효과 분석 실행 프로필
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to This change adds a versioned method-effects analysis artifact and execution result. Invalid digest metadata can pass validation, and tenant authorization is not visibly bound to the accepted run if the execution boundary is reachable by an untrusted caller. The PR is mergeable with explicit owner awareness and follow-up on digest validation and authorization binding. Sequence Diagram(s)sequenceDiagram
participant AnalysisRunRequest
participant execute_method_effects_run
participant tepp_simulation
participant MethodEffectsArtifact
participant AnalysisRunTerminalResult
AnalysisRunRequest->>execute_method_effects_run: 요청 및 접수 identity 전달
execute_method_effects_run->>tepp_simulation: cutoff-safe simulation manifest 생성
tepp_simulation-->>execute_method_effects_run: 사용 가능한 문서와 method effects 반환
execute_method_effects_run->>MethodEffectsArtifact: 문서 집계와 digest 생성
MethodEffectsArtifact-->>execute_method_effects_run: 검증된 artifact 반환
execute_method_effects_run->>AnalysisRunTerminalResult: 성공 상태와 artifact digest 연결
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 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 |
![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.
![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 != METHOD_EFFECTS_MODEL_CONTRACT_VERSION | ||
| || request.output_profile != METHOD_EFFECTS_OUTPUT_PROFILE | ||
| { |
There was a problem hiding this comment.
🟡 Equivalent cutoffs reject valid runs
A valid cutoff written with +00:00 fails execute_method_effects_run because it compares text instead of instants. Semantically matching requests cannot run.
Prompt for agents
In crates/analysis_engine/src/method_effects_artifact.rs, execute_method_effects_run compares request.knowledge_cutoff directly with KnowledgeCutoff::to_rfc3339(). AnalysisRunRequest validation and temporal_core accept equivalent RFC 3339 representations such as +00:00, while to_rfc3339 canonicalizes them to Z. Parse the request cutoff and compare the underlying instant to the supplied KnowledgeCutoff, preserving the existing profile and model checks. Add coverage for equivalent noncanonical RFC 3339 spellings.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn to_json(&self) -> Result<String, AnalysisEngineError> { | ||
| self.validate()?; | ||
| let payload = | ||
| serde_json::to_string(self).map_err(|_| AnalysisEngineError::SerializationFailure)?; | ||
| Ok(payload) | ||
| } |
There was a problem hiding this comment.
🔍 Serialization limit is one-sided
from_json enforces the artifact byte limit, while to_json only validates fields. Current fixed-size contents remain safely below that limit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn require_generation_budget(config: SimulationConfig) -> Result<(), AnalysisEngineError> { | ||
| let events = u64::from(config.event_count()); | ||
| let originals = events | ||
| .checked_mul(u64::from(config.documents_per_event())) | ||
| .ok_or(AnalysisEngineError::LimitExceeded)?; | ||
| let documents = originals | ||
| .checked_mul(4) | ||
| .ok_or(AnalysisEngineError::LimitExceeded)?; | ||
| let memberships = documents | ||
| .checked_mul(u64::from(config.membership_targets())) | ||
| .ok_or(AnalysisEngineError::LimitExceeded)?; | ||
| let relations = originals | ||
| .checked_mul(8) | ||
| .and_then(|value| { | ||
| events | ||
| .checked_mul(3) | ||
| .and_then(|events| value.checked_add(events)) | ||
| }) | ||
| .and_then(|value| value.checked_add(1)) | ||
| .ok_or(AnalysisEngineError::LimitExceeded)?; | ||
| let generated_rows = events | ||
| .checked_add(documents) | ||
| .and_then(|value| value.checked_add(memberships)) | ||
| .and_then(|value| value.checked_add(relations)) | ||
| .ok_or(AnalysisEngineError::LimitExceeded)?; | ||
| if generated_rows > METHOD_EFFECTS_GENERATED_ROW_LIMIT { | ||
| return Err(AnalysisEngineError::LimitExceeded); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
📝 Info: Budget uses a conservative ceiling
require_generation_budget substantially overcounts optional variants and relations. This can reject feasible runs, but it honors the documented pre-allocation ceiling.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn hash_document(hasher: &mut Sha256, document: &SimulatedDocument) { | ||
| hasher.update(document.document_id().as_bytes()); | ||
| hasher.update(document.event_id().as_bytes()); | ||
| hasher.update(document.document_time().to_rfc3339().as_bytes()); | ||
| hasher.update(document.available_time().to_rfc3339().as_bytes()); | ||
| hasher.update(document.method_effect().wire_name().as_bytes()); | ||
| if let Some(parent) = document.parent_document_id() { | ||
| hasher.update(parent.as_bytes()); | ||
| } | ||
| if let Some(observed) = document.observed_event_time() { | ||
| hasher.update(observed.to_rfc3339().as_bytes()); | ||
| } else { | ||
| hasher.update(b"missing"); | ||
| } | ||
| for membership in document.memberships() { | ||
| hasher.update(membership.group_id().as_bytes()); | ||
| hasher.update(membership.role_label().as_bytes()); | ||
| hasher.update(membership.weight_bps().to_le_bytes()); | ||
| } | ||
| } | ||
|
|
||
| /// Digest an ordered admitted-document population using canonical truth fields. | ||
| /// | ||
| #[must_use] | ||
| pub fn digest_documents(documents: &[&SimulatedDocument]) -> String { | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(b"tepp.simulated_documents.v1"); | ||
| hasher.update(documents.len().to_string().as_bytes()); | ||
| hasher.update([0]); | ||
| for document in documents { | ||
| hash_document(&mut hasher, document); | ||
| } | ||
| hex_encode(&hasher.finalize()) |
There was a problem hiding this comment.
📝 Info: Manifest digests remain compatible
hash_document preserves the previous field order and encoding. Existing full-manifest digests remain stable while admitted-document digests use a separate domain.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
GAP-004 leftover / ADR 0057. Bind existing
tepp_simulation::generateandrefuse_unavailable_documentto a cutoff-safemethod_effects_v1analysis-run profile (tepp.method_effects.v1).original,revision,translation,template_copy). Does not reimplement document generation.simulation_method_effect_labels_not_estimator_model.trsl_topic_lineage_v1,fitted_candidate_k_v1,pareto_candidate_k_v1,joint_posterior_draws_v1,composed_fitted_lineage_v1,case_deletion_refit_v1, andtopic_activity_v1.Not an estimator-side method model. Not GPU. Not MCMC. Not topic birth/split/merge. Not exhaustive case-deletion (#413 / ADR 0056). Not composed fitted-K+lineage (#412 / ADR 0055). Not Pareto (#409 / ADR 0053). Not joint Laplace (#408 / ADR 0052). Not implemented-main.
Distinct from live slices
Does not duplicate #413 (case-deletion refit), #412 (composed fitted lineage), #411 (export GET), #410 (export-authorize CLI), #409 (Pareto candidate-K), #408 (joint posterior Laplace), #407 (topic activity), #406 (wait CLI), #405 (interpreter/verifier), #404 (fitted candidate-K), #403 (retry-lineage CLI), #398 (membership-posterior ICC), #376 (ESEM/DSEM), #374 (Rubin), #372 (CWC), #351 (Leiden), or Driver p.16 std-family micro-PRs.
Verification
cargo test -p analysis_enginecargo clippy -p analysis_engine --all-targets -- -D warningspython3 scripts/validate_documentation.pyMerge gate
Two independent current-head APPROVEs required. Author/bot COMMENTED is not independent APPROVE. Exact-head Checks on this SHA only. Predecessor Checks do not transfer. Do not self-approve. Do not merge without two independent approvals.
Summary by CodeRabbit
새 기능
문서