feat(api): consolidate interpretation-run stored-request GET and CLI - #454
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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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 new CLI leaves response parsing, rendering, size-limit, transport, header, and flag branches untested. Repository rules require 100% production line and branch coverage.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let idempotency_key = decode_path_segment(encoded_id)?; | ||
| require_nonempty(&idempotency_key)?; | ||
| if idempotency_key.contains('/') || idempotency_key.contains('\0') { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| if idempotency_key.len() > INTERPRETATION_RUN_RETRIEVAL_ID_MAX_LEN { | ||
| return Err(OrchestratorLiveError::LimitExceeded); |
There was a problem hiding this comment.
🟡 Accepted request keys become unretrievable
interpretation_run_stored_request_path_id rejects slashes, controls, and keys over 128 bytes, but POST validation accepts them. Those accepted requests cannot be retrieved.
Prompt for agents
Align interpretation-run POST identity validation with every retrieval path. InterpretationRunRequest::validate currently accepts slash-containing, control-containing, and arbitrarily long idempotency keys, while interpretation_run_stored_request_path_id and its exchange reject those identities. Either reject unsupported identities before storing a POST or define one reversible bounded path encoding accepted consistently by POST, collection, GET-by-id, stored-request GET, and both CLIs. Add integration tests that create each boundary identity and then retrieve its stored request.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if is_interpretation_run_stored_request_path(path) { | ||
| return self.get_interpretation_run_stored_request(path, &headers, body); |
There was a problem hiding this comment.
🟡 Oversized paths return the wrong status
is_interpretation_run_stored_request_path discards every parse error, so an oversized stored-request path reaches the wrong parser. The service returns 400 instead of 413.
Prompt for agents
Preserve stored-request path parser errors during dispatch. Route based on the endpoint shape without converting interpretation_run_stored_request_path_id errors into false, then let get_interpretation_run_stored_request return LimitExceeded for an oversized identity. Keep unrelated extra-segment paths on the invalid-route path, and add a service-level test asserting HTTP 413 for a stored-request identity over the documented limit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| require_nonempty(origin)?; | ||
| if !origin.starts_with("https://") || origin.ends_with('/') { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| let rest = origin | ||
| .strip_prefix("https://") | ||
| .ok_or(OrchestratorLiveError::InvalidWirePayload)?; | ||
| if rest.contains('@') || rest.contains('?') || rest.contains('#') || rest.contains('\\') { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); |
There was a problem hiding this comment.
🟡 Origins with paths break CLI requests
contextual_orchestrator_interpretation_run_stored_request_exchange accepts an origin containing a path, then appends the endpoint beneath it. CLI composition rejects the resulting route, so the invocation cannot execute.
Prompt for agents
Validate origin as an HTTPS origin rather than a string prefix. Reject any path, query, fragment, userinfo, empty authority, or malformed authority before constructing target_url, while retaining valid host and port forms. Add tests showing that an origin such as https://tepp.example.test/base fails during invocation or exchange validation rather than producing a route rejected later by loopback_http1_from_interpretation_run_stored_request_exchange.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let bytes = read_bounded(&mut stream, MAXIMUM_HTTP_RESPONSE_BYTES)?; | ||
| parse_http_response(&bytes) |
There was a problem hiding this comment.
🔍 Response framing assumes connection close
execute_interpretation_run_stored_request_cli reads until EOF instead of stopping at Content-Length. A future keep-alive listener would turn complete responses into timeouts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn refuse_metrics_on_json(value: &serde_json::Value) -> Result<(), OrchestratorLiveError> { | ||
| match value { | ||
| serde_json::Value::Object(object) => { | ||
| if FORBIDDEN_STORED_REQUEST_KEYS | ||
| .iter() | ||
| .any(|key| object.contains_key(*key)) | ||
| { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| for nested in object.values() { | ||
| refuse_metrics_on_json(nested)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| serde_json::Value::Array(items) => { | ||
| for nested in items { | ||
| refuse_metrics_on_json(nested)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| _ => Ok(()), | ||
| } |
There was a problem hiding this comment.
📝 Info: Metric filter preserves valid requests
The recursive forbidden-key check cannot reject accepted content. InterpretationRunRequest has fixed fields, and user-provided evidence identifiers remain string values rather than object keys.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Superseded by fold into #467
Strict ancestry was verified before closure: #454 exact head
c911fbcd6e13046358cc7f3692775ed4b008daddis the merge base of #467, and #467 was one commit ahead with zero commits behind. #467 was retargeted to this PR's former base, so the complete stored-request GET/CLI implementation, tests, RED/GREEN lineage and review history remain in the surviving Analysis Run landing vehicle. This PR is closed only to reduce duplicate WIP; its discussion remains immutable evidence.Consolidated landing vehicle history
This PR previously folded predecessor #453 into one Analysis Run / contextual-orchestrator stored-request application-adapter vehicle. The head contains #453 as its direct ancestor and preserves
/v1/interpretation-runs/{idempotency_key}/request, hostile identity/header/credential refusals, empty-body and cancel-route refusal,scientific_authority=false, and the publishedtepp-interpretation-run-request getCLI with fail-closed bind/origin/consumer/input behavior.This is an application/adapter lineage record, not independent bounded-context or per-route ADR authority. Current landing work continues on #467 under queue-authority policy.