feat(api): consolidate interpretation-run retrieval, lookup, and stored-request adapters - #469
Conversation
…nt GET
Publish GET /v1/interpretation-runs/{idempotency_key}/request so operators
fetch the accepted create request without POST replay. scientific_authority
stays false. Naruon and LineageWeave are refused. NaruonLiveService stays
POST-only. Cancel extra-segment stays refused. ADR 0085.
…d CLI Publish tepp-interpretation-run-request get so operators retrieve the stored create request on spawned tepp-orchestrator-loopback TCP without POST replay. scientific_authority stays false. Empty stdin is admitted. Naruon and LineageWeave are refused. NaruonLiveService stays POST-only. Cancel extra- segment stays refused. ADR 0086.
GET /v1/interpretation-runs/by-run-id/{interpretation_run_id} returns the
metric-free hypothetical identity on OrchestratorLiveService. Dual identity
of GET-by-id (idempotency_key). Zero and ambiguous matches fail closed.
claim_status remains hypothetical; scientific_authority remains false.
NaruonLiveService stays POST-only. Does not re-open cancel lineages.
Publish tepp-interpretation-run-lookup so operators can mint
GET /v1/interpretation-runs/by-run-id/{interpretation_run_id} onto spawned
tepp-orchestrator-loopback TCP without scanning collection pages. Metric-free
identity only. Empty stdin admitted. Naruon and LineageWeave refused.
…ed run id
Publish GET /v1/interpretation-runs/by-run-id/{interpretation_run_id}/request
so operators who hold a 202 receipt can recover the stored create without a
second hop through client-key stored-request. Metric-free. Zero and
ambiguous matches fail closed. Naruon and LineageWeave refused.
seonghobae
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Coverage evidence remains incomplete
The repository requires 100% production line and branch coverage. The test plan omits coverage-gate evidence for the new parser, builder, and route.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn parse_http_response(bytes: &[u8]) -> Result<OrchestratorLiveResponse, OrchestratorLiveError> { | ||
| let text = std::str::from_utf8(bytes).map_err(|_| OrchestratorLiveError::InvalidWirePayload)?; | ||
| let (header_block, body) = text | ||
| .split_once("\r\n\r\n") | ||
| .ok_or(OrchestratorLiveError::InvalidWirePayload)?; | ||
| if header_block.len() > LIVE_HEADER_BYTE_LIMIT { | ||
| return Err(OrchestratorLiveError::LimitExceeded); | ||
| } | ||
| let mut lines = header_block.split("\r\n"); | ||
| let status_line = lines | ||
| .next() | ||
| .ok_or(OrchestratorLiveError::InvalidWirePayload)?; | ||
| let mut parts = status_line.split(' '); | ||
| if parts.next() != Some("HTTP/1.1") { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| let code = parts | ||
| .next() | ||
| .ok_or(OrchestratorLiveError::InvalidWirePayload)? | ||
| .parse::<u16>() | ||
| .map_err(|_| OrchestratorLiveError::InvalidWirePayload)?; | ||
| let reason_phrase = static_reason(code)?; | ||
| let mut content_length = None; | ||
| for (index, line) in lines.enumerate() { | ||
| if index >= LIVE_HEADER_COUNT_LIMIT { | ||
| return Err(OrchestratorLiveError::LimitExceeded); | ||
| } | ||
| let (name, value) = line | ||
| .split_once(':') | ||
| .ok_or(OrchestratorLiveError::InvalidWirePayload)?; | ||
| if name.eq_ignore_ascii_case("content-length") { | ||
| if content_length.is_some() { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| content_length = Some( | ||
| value | ||
| .trim() | ||
| .parse::<usize>() | ||
| .map_err(|_| OrchestratorLiveError::InvalidWirePayload)?, | ||
| ); | ||
| } | ||
| } | ||
| let declared = content_length.ok_or(OrchestratorLiveError::InvalidWirePayload)?; | ||
| if declared > DEFAULT_INTERPRETATION_BYTE_LIMIT { | ||
| return Err(OrchestratorLiveError::LimitExceeded); | ||
| } | ||
| if declared != body.len() { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| Ok(OrchestratorLiveResponse { | ||
| status_code: code, | ||
| reason_phrase, | ||
| body: body.to_owned(), | ||
| }) | ||
| } | ||
|
|
||
| fn static_reason(code: u16) -> Result<&'static str, OrchestratorLiveError> { | ||
| match code { | ||
| 200 => Ok("OK"), | ||
| 202 => Ok("Accepted"), | ||
| 400 => Ok("Bad Request"), | ||
| 403 => Ok("Forbidden"), | ||
| 413 => Ok("Payload Too Large"), | ||
| 422 => Ok("Unprocessable Entity"), | ||
| _ => Err(OrchestratorLiveError::InvalidWirePayload), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 CLI protocol logic is duplicated
Both CLIs duplicate response parsing, limits, timeouts, and status tables. Protocol fixes now require synchronized edits across several modules.
Was this helpful? React with 👍 or 👎 to provide feedback.
…edicated CLI Publish tepp-interpretation-run-lookup-request get so operators who hold a 202 receipt can recover the stored create onto spawned tepp-orchestrator-loopback TCP. Empty stdin admitted. Metric-free. Naruon and LineageWeave refused.
seonghobae
changed the title
| if is_interpretation_run_stored_request_path(path) { | ||
| return self.get_interpretation_run_stored_request(path, &headers, body); | ||
| } | ||
| if is_interpretation_run_lookup_stored_request_path(path) { | ||
| return self.get_interpretation_run_lookup_stored_request(path, &headers, body); | ||
| } | ||
| if is_interpretation_run_lookup_path(path) { | ||
| return self.lookup_interpretation_run_by_run_id(path, &headers, body); | ||
| } | ||
| return self.get_interpretation_run(path, &headers, body); |
There was a problem hiding this comment.
📝 Info: Route precedence avoids collisions
Reserved-segment rejection keeps client-key and server-ID routes distinct. Decoding after segmentation also prevents encoded slashes from changing the selected route.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut matches = self.accepted_runs.values().filter_map(|(request, accepted)| { | ||
| (accepted.interpretation_run_id() == interpretation_run_id).then_some(request) | ||
| }); | ||
| let stored = matches | ||
| .next() | ||
| .ok_or(OrchestratorLiveError::InvalidWirePayload)?; | ||
| if matches.next().is_some() { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); |
There was a problem hiding this comment.
📝 Info: Server-ID lookup remains linear
Both server-ID handlers scan the registry and continue for duplicate detection. Cost grows linearly, though no concrete production failure is established.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn oversized_lookup_stored_request_identity_preserves_limit_status() { | ||
| let oversized = "a".repeat(INTERPRETATION_RUN_LOOKUP_ID_MAX_LEN + 1); | ||
| let request = format!( | ||
| "GET /v1/interpretation-runs/by-run-id/{oversized}/request HTTP/1.1\r\nHost: 127.0.0.1\r\ncontent-type: application/json\r\ntepp-consumer: contextual-orchestrator\r\ntepp-contract-version: 1\r\ncontent-length: 0\r\n\r\n" | ||
| ); | ||
|
|
||
| let response = OrchestratorLiveService::new().handle_http_request(&request); | ||
| assert_eq!(response.status_code, 413); | ||
| assert!(response.body.contains("limit_exceeded")); |
There was a problem hiding this comment.
📝 Info: Oversized routes preserve limit errors
An oversized stored-request path misses its classifier, then reaches ordinary retrieval. That parser still returns LimitExceeded, preserving HTTP 413.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[test] | ||
| fn service_refuses_reserved_lookup_segment_before_acceptance() { | ||
| let request = InterpretationRunRequest::new( | ||
| INTERPRETATION_RUN_CONTRACT_VERSION, | ||
| "by-run-id", | ||
| "tenant-a", | ||
| "snapshot-a", | ||
| "2026-09-01T00:00:00Z", | ||
| OrchestrationMode::Direct, | ||
| 128, | ||
| vec!["span-a".into()], | ||
| false, | ||
| ) | ||
| .expect("wire-valid request reaches service identity policy"); | ||
|
|
||
| let response = OrchestratorLiveService::new().handle_http_request(&post_request(&request)); | ||
| assert_eq!(response.status_code, 400); | ||
| assert!(response.body.contains("invalid_wire_payload")); | ||
| } |
There was a problem hiding this comment.
📝 Info: Reserved identity reaches service admission
The constructor accepts by-run-id, but service admission rejects it before storage. Otherwise, its idempotency-key retrieval route would remain unreachable.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
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 |
| pub fn is_interpretation_run_lookup_path(path: &str) -> bool { | ||
| interpretation_run_lookup_path_id(path).is_ok() | ||
| } |
There was a problem hiding this comment.
🟡 Oversized identifiers return wrong status
Oversized lookup or stored-request identifiers make is_interpretation_run_lookup_path reject their routes. Dispatch then returns 400 instead of the promised 413.
Prompt for agents
Preserve route ownership when interpretation_run_lookup_path_id or interpretation_run_stored_request_path_id returns LimitExceeded, as the lookup stored-request predicate already does. Update both route predicates so structurally valid oversized identities still dispatch to their endpoint handlers and map LimitExceeded to HTTP 413. Add service-level tests for oversized /by-run-id/{id} and /{idempotency_key}/request paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let payload = InterpretationRunLookupStoredRequestPayload::new( | ||
| accepted.interpretation_run_id(), | ||
| stored.clone(), | ||
| )? | ||
| .to_json()?; |
There was a problem hiding this comment.
🔴 Stored-request GET returns incompatible JSON
Successful lookups wrap the stored request in InterpretationRunLookupStoredRequestPayload, although the endpoint contract returns the request directly. Direct HTTP clients cannot deserialize successful responses.
Prompt for agents
Restore the public GET /v1/interpretation-runs/by-run-id/{interpretation_run_id}/request response contract so its body is the stored InterpretationRunRequest directly, as specified by ADR 0097 and consumed by interpretation_run_lookup_stored_request_http_contract.rs. Rework render_interpretation_run_lookup_stored_request_cli_stdout accordingly. If response binding is still required, implement it without silently changing this existing HTTP representation, or explicitly version the endpoint and update every contract, client, test, ADR, and API document together.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head COMMENT on 72a7755bcc91b1107560c980ce817eca153126e4 (not APPROVE; author COMMENT is not an independent approval under ruleset 18156473). Predecessor Checks/reviews do not transfer. Do not un-draft. Do not duplicate interpretation-run lookup/stored-request surfaces.
Fail-closed read at this SHA:
- Metric-free identity lookup
GET /v1/interpretation-runs/by-run-id/{interpretation_run_id}and stored-request extra-segment.../by-run-id/{id}/requeststay onOrchestratorLiveService. ADR 0097 keepsscientific_authority=falseand refusestepp.scientific_acceptance.v1. - Dispatch order documented as collection →
{key}/request→by-run-id/{id}/request→ lookup by-run-id → GET-by-id, with reservedby-run-idrefused as a client key. - Naruon and LineageWeave are refused on this contextual-orchestrator-owned adapter.
NaruonLiveServicestays POST-only. Zero and ambiguous matches fail closed.
Do not treat consumer-only scoping as a tenant oracle. Export lookup stored-request had to be quarantined on #466 (ADR 0099) after a consumer-namespace search disclosed tenant_workspace_id/principal_id. If this lookup stored-request payload can serialize those identities without an authenticated tenant-and-principal binding, apply the same fail-closed quarantine rather than shipping unscoped stored-request-by-client-key disclosure. Unique interpretation-run retrieval/lookup/stored-request consolidation remains occupied here. Not merge-ready without two independent current-head APPROVEs plus exact-head Checks.
|
@opencode-agent repair current exact head RCA evidence from Rust Foundation CI run Finish the repository-owned repair by running |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head COMMENT on f81963d (draft). Predecessor seonghobae COMMENTED 5088185084 was on 72a7755 and does not transfer. Zero current-head APPROVE. Never self-approve. Do not un-draft.
Unique occupied: GET /v1/interpretation-runs/by-run-id/{interpretation_run_id}/request plus tepp-interpretation-run-lookup-request (ADR 0097/0098), stacked on lookup GET/CLI and stored-request GET/CLI. Naruon and LineageWeave stay refused. NaruonLiveService stays POST-only. Metric/scientific_acceptance keys stay fail-closed. Do not duplicate this surface. Do not un-quarantine export stored-request-by-client-key (ADR 0099 lives on #466).
f81963d retargets live_get_returns_stored_request_without_scientific_authority onto InterpretationRunLookupStoredRequestPayload::from_json and asserts interpretation_run_id() == "orch-run-1". That envelope is the right operator-visible HTTP shape (deny_unknown_fields, nested request to_json validation). CLI render_interpretation_run_lookup_stored_request_cli_stdout already fail-closes when the envelope id mismatches the invocation, then prints the inner request JSON.
Blocker on this exact head: crates/orchestrator_live/src/lib.rs re-exports the exchange/path/CLI helpers but does not pub use InterpretationRunLookupStoredRequestPayload. The integration test now imports that type from crate-public API, so the new commit will not compile until the payload is re-exported. Do not "fix" this by parsing the body as bare InterpretationRunRequest again — that re-opens cross-run substitution. Export the envelope type.
Do not weaken fail-closed. No Buyer language. Persistence remains GAP-003B. GAP-004 stay out.
1 participant
Consolidated Analysis Run landing vehicle
This Draft is the surviving contextual-orchestrator-facing Analysis Run application/adapter vehicle for the #453 → #454 → #467 → #468 → #469 → #470 lineage and subsequent current-source hardening. Strict descendants were fast-forward folded without force, preserving predecessor implementation/test commits and immutable PR/review/RED-GREEN records while reducing duplicate WIP.
Preserved surfaces
interpretation_run_idand its published lookup CLI;interpretation_run_id, eliminating an otherwise mandatory two-hop identity lookup;tepp-interpretation-run-lookup-request getforGET /v1/interpretation-runs/by-run-id/{interpretation_run_id}/request;NaruonLiveServicePOST-only behavior;claim_status=hypothetical,scientific_authority=false, and notepp.scientific_acceptance.v1exposure.DDD / queue authority
These routes and binaries are adapters inside the Analysis Run application context; they are not bounded contexts. Operation-specific ADR records in this lineage are implementation evidence pending repository-wide normalization under #437 and do not establish separate architecture authority. Further compatible interpretation-run retrieval/lookup/stored-request GET/CLI slices must fold into this vehicle or a coherent successor rather than creating another micro-PR.
Current exact head:
72a7755bcc91b1107560c980ce817eca153126e4.Merge bar
Every fast-forward or source repair invalidates predecessor-head hosted evidence as landing authority. This exact head must reacquire current Rust/documentation/security checks, resolve all valid current-head review conversations, and satisfy qualifying independent review under live ruleset
18156473. No stale evidence transfer, force push, self-approval, protection bypass, fail-open security change, or scientific-authority promotion.