feat(api): consolidate interpretation-run retrieval GET and CLI - #439
Conversation
GAP-003A unique slice stacked on collection GET: loopback
GET /v1/interpretation-runs/{idempotency_key} returns one accepted
metric-free hypothetical identity without POST replay. Naruon and
LineageWeave refused. ADR 0071.
GAP-003A unique slice stacked on GET-by-id HTTP: published
tepp-interpretation-run-get get mints typed contextual-orchestrator
GET /v1/interpretation-runs/{idempotency_key} onto spawned
tepp-orchestrator-loopback TCP. Naruon and LineageWeave refused.
ADR 0072.
|
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn read_bounded( | ||
| reader: &mut impl Read, | ||
| maximum_bytes: usize, | ||
| ) -> Result<Vec<u8>, OrchestratorLiveError> { | ||
| let mut bytes = Vec::new(); | ||
| reader | ||
| .take((maximum_bytes + 1) as u64) | ||
| .read_to_end(&mut bytes) | ||
| .map_err(|error| map_io_error(&error))?; | ||
| if bytes.len() > maximum_bytes { | ||
| return Err(OrchestratorLiveError::LimitExceeded); | ||
| } | ||
| Ok(bytes) |
There was a problem hiding this comment.
🔍 Response parsing depends on connection closure
read_bounded waits for EOF after receiving the complete response. A keep-alive HTTP/1.1 listener can therefore turn a valid retrieval into a timeout.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
changed the title
There was a problem hiding this comment.
🔍 Current-head verification remains external
The environment lacks cargo, so the new Rust tests could not run locally. Repository rules require current-head focused and complete verification before merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
| headers: &HashMap<String, String>, | ||
| body: &str, | ||
| ) -> Result<OrchestratorLiveResponse, OrchestratorLiveError> { | ||
| let idempotency_key = interpretation_run_retrieval_path_id(path)?; |
There was a problem hiding this comment.
🟡 Long accepted keys cannot be retrieved
When InterpretationRunRequest::validate accepts a key over 128 bytes, get_interpretation_run cannot return that accepted run. Both new retrieval clients fail.
Prompt for agents
Reconcile idempotency-key validation across interpretation-run creation and retrieval. InterpretationRunRequest currently accepts keys longer than INTERPRETATION_RUN_RETRIEVAL_ID_MAX_LEN, stores them in OrchestratorLiveService::accepted_runs, and returns 202. GET-by-id later rejects the same key in interpretation_run_retrieval_path_id, and the retrieval CLI rejects it while composing the request. Define one shared key limit and enforce it before POST acceptance, or extend retrieval safely to every key POST can accept. Add a contract test that creates a boundary-length key and retrieves it, plus a test for the first rejected length.
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); | ||
| } | ||
| if host_implies_table_access(rest) { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); |
There was a problem hiding this comment.
🟡 Path-bearing origins break retrieval
When origin contains a path, validation appends the endpoint after that path. loopback_http1_from_interpretation_run_retrieval_exchange then rejects every composed request.
Prompt for agents
Validate origin as an actual HTTPS origin with a nonempty authority and no path, rather than checking only the scheme, trailing slash, and selected characters. contextual_orchestrator_interpretation_run_retrieval_exchange currently accepts values such as https://tepp.example.test/base, appends /v1/interpretation-runs/{id} to that path, and produces an exchange that the loopback renderer rejects. Keep InterpretationRunRetrievalCliInvocation::validate and the exchange constructor consistent, and add tests for path-bearing and malformed-authority origins.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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)?; |
There was a problem hiding this comment.
🔍 Response status parsing is asymmetric
parse_http_response ignores trailing status-line tokens and the received reason phrase. The bundled listener remains compatible, but stricter framing may be required for other responders.
Was this helpful? React with 👍 or 👎 to provide feedback.
| | Interpretation-run GET-by-id doctoring | [`docs/research/interpretation-run-retrieval-http.md`](docs/research/interpretation-run-retrieval-http.md) | | ||
| | Interpretation-run retrieval CLI doctoring | [`docs/research/interpretation-run-retrieval-cli.md`](docs/research/interpretation-run-retrieval-cli.md) | |
There was a problem hiding this comment.
🔍 Duplicated documentation map diverges
The canonical map repeats its heading and table later, but the new retrieval links appear only in the first copy. Readers can encounter inconsistent indexes.
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 only (not APPROVE) on 95dbb195943fb17ca79fc316796de738abcfc6de. Devin COMMENTED is not independent APPROVE.
Unique remains tepp-interpretation-run-get + ADR 0072 (GET /v1/interpretation-runs/{idempotency_key}). Metric-free retrieval; Naruon and LineageWeave refused; NaruonLiveService stays POST-only. Path/origin fail closed. Do not duplicate interpretation-run retrieval GET/CLI. Lookup/stored-request consolidation is #469 @ 72a7755b — do not copy that surface here. Do not treat consumer-only scoping as a tenant oracle. Persistence remains GAP-003B.
Still draft. Do not un-draft. Zero exact-head APPROVEs. Do not merge without two independent current-head APPROVEs under ruleset 18156473.
1 participant
Consolidated landing vehicle
This PR folds predecessor #438 into one contextual-orchestrator-facing Analysis Run / interpretation-run retrieval application-adapter vehicle. The current head contains #438 as its direct ancestor, so retargeting to #438's former base preserves GET-by-id implementation/tests while eliminating one open micro-PR. #438 remains immutable review/history evidence; stored-request vehicle #454 also retains this ancestry.
Preserved GET behavior from #438:
GET /v1/interpretation-runs/{idempotency_key}, contextual-orchestrator-only consumer boundary, empty body, identity/pagination/extra-segment hostility refusals, metric-free hypothetical identity,scientific_authority=false, and naruon/LineageWeave refusal.CLI behavior on this head: published
tepp-interpretation-run-get get, empty-stdin admission, public-bind/localhost/http-origin/unpublished-consumer/pagination/credential refusals, typed exchange, and the same metric-free non-scientific-authority output.This is one Analysis Run application/adapter landing vehicle, not a bounded context. ADR 0071/0072 are implementation lineage pending #437 normalization. Merge only after fresh exact-head required workflows, resolved conversations, and qualifying independent approval under live ruleset 18156473. No predecessor-head evidence transfer or bypass.