feat(api): serve scientific acceptance only on loopback status GET - #359
feat(api): serve scientific acceptance only on loopback status GET#359seonghobae wants to merge 2 commits into
Conversation
GAP-003A third slice for issue #166. POST /v1/analysis-runs stays a metric-free receipt. GET /v1/analysis-runs/{run_id} keeps accepted and running statuses metric-free, and may return tepp.scientific_acceptance.v1 only on a succeeded status with profile scientific_acceptance_v1. Receipt metric keys, GET bodies, failed-plus-artifact emission, all-zero digests, and digest mismatch fail closed. This does not duplicate the analysis_engine library bind or the terminal-result DTO wire.
|
Warning Review limit reachedNext included review available in 8 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 selected for processing (13)
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}
| #[cfg(test)] | ||
| pub(crate) fn record_loopback_status( |
There was a problem hiding this comment.
🔴 Production runs never reach terminal status
record_loopback_status is compiled only for tests, leaving production runs permanently accepted. Successful runs can never return the new scientific acceptance response.
Prompt for agents
Expose a production status-update integration for AnalysisRunLiveService instead of compiling record_loopback_status only in tests. Preserve request/receipt binding and artifact validation, and connect the service to the worker or execution boundary that supplies running and terminal states. Add a non-test integration test that creates a run, records a succeeded result through the production API, and retrieves it over GET.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let terminal_value = status_value | ||
| .get_mut("terminal_result") | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| let terminal_object = terminal_value | ||
| .as_object_mut() | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| terminal_object.insert("scientific_acceptance".to_owned(), artifact); | ||
| serde_json::to_string(&status_value).map_err(|_| ApiError::InvalidWirePayload) |
There was a problem hiding this comment.
🔴 Successful responses reject their own DTO
inject_scientific_acceptance_http adds an unsupported terminal field. Consumers using AnalysisRunStatus::from_json reject every scientific acceptance response as invalid.
Prompt for agents
Make the succeeded HTTP response conform to an exported, parseable wire contract. Either extend/version the terminal-result DTO with a validated scientific_acceptance field or define and export a distinct status-response DTO, then use that type for serialization and parsing. Add a round-trip test that feeds the exact GET response body into the public response parser.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let artifact = parse_scientific_acceptance_http_artifact(artifact_json)?; | ||
| let mut status_value: serde_json::Value = | ||
| serde_json::from_str(status_json).map_err(|_| ApiError::InvalidWirePayload)?; | ||
| let terminal_value = status_value | ||
| .get_mut("terminal_result") | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| let terminal_object = terminal_value | ||
| .as_object_mut() | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| terminal_object.insert("scientific_acceptance".to_owned(), artifact); | ||
| serde_json::to_string(&status_value).map_err(|_| ApiError::InvalidWirePayload) |
There was a problem hiding this comment.
🔴 Returned artifacts lose digest identity
inject_scientific_acceptance_http reserializes the artifact after hashing its original bytes. Whitespace or key-order changes prevent clients from reproducing result_sha256.
Prompt for agents
Define one canonical scientific-acceptance serialization and calculate result_sha256 over exactly those canonical bytes. Ensure the nested object returned by GET canonicalizes to the same bytes, or return a byte-preserving artifact representation. Add tests using alternate key order and whitespace that verify the artifact reconstructed from the HTTP response hashes to terminal_result.result_sha256.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn parse_scientific_acceptance_http_artifact( | ||
| artifact_json: &str, | ||
| ) -> Result<serde_json::Value, ApiError> { | ||
| require_nonempty(artifact_json)?; | ||
| let value: serde_json::Value = | ||
| serde_json::from_str(artifact_json).map_err(|_| ApiError::InvalidWirePayload)?; | ||
| let object = value.as_object().ok_or(ApiError::InvalidWirePayload)?; | ||
| let schema = object | ||
| .get("schema_version") | ||
| .and_then(serde_json::Value::as_str) | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| let profile = object | ||
| .get("output_profile") | ||
| .and_then(serde_json::Value::as_str) | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| let binding = object | ||
| .get("binding_sha256") | ||
| .and_then(serde_json::Value::as_str) | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| if schema != SCIENTIFIC_ACCEPTANCE_HTTP_SCHEMA || profile != SCIENTIFIC_ACCEPTANCE_HTTP_PROFILE | ||
| { | ||
| return Err(ApiError::InvalidWirePayload); | ||
| } | ||
| require_nonzero_canonical_sha256(binding)?; | ||
| Ok(value) | ||
| } |
There was a problem hiding this comment.
🔍 Scientific artifact lacks structural bounds
parse_scientific_acceptance_http_artifact accepts arbitrary extra fields and nesting without an artifact-specific limit. Add a strict DTO and bounded shape before this becomes production-facing.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Hour-20 exact-head review request. Current head GAP-003A GET-by-id. ADR 0027. Do not duplicate GET status. Do not self-approve. Do not --admin merge. Checks/reviews are not a reason to weaken fail-closed gates. |
|
Hour-21 exact-head review request. Current head @opencode-agent review |
|
Hour-22 exact-head review request. GAP-003A loopback HTTP status GET. Current head @opencode-agent review |
1 participant
Consolidation decision — closed after the terminal scientific-acceptance artifact was retired
This PR is not merged. Exact head
e5601809dc4b50e4ae9b4441da4a1b44e2ee0eaecontains a useful generic Analysis Run status GET, but its terminal success contract is defined specifically aroundtepp.scientific_acceptance.v1, whose #356/#358 authority chain has been retired.Preserve and fold the generic status behavior into the coherent Analysis Run application/adapter vehicle under #166/#435: run identity lookup, accepted/running metric-free responses, failed terminal behavior, consumer isolation, digest binding, loopback-only serving and fail-closed malformed requests. Rebuild the succeeded terminal payload around versioned Validation Evidence produced by its owning Rust validation path; status transport must not decide Scientific Claim Promotion.
Branch, tests, reviews and doctoring remain available as fold lineage.