feat(api): consolidate export retrieval GET and CLI - #417
Conversation
GAP-003A unique slice: AnalysisRunLiveService mints a metric-free
export_id on naruon POST /v1/exports and serves GET /v1/exports/{id}.
NaruonLiveService stays POST-only. Persistence remains GAP-003B.
ADR 0053 is already live on #409 (Pareto candidate-K vs main). Rename this stack's decision identity rather than collide.
GAP-003A unique slice: tepp-export-get mints typed naruon
GET /v1/exports/{export_id} onto spawned tepp-loopback TCP.
LineageWeave is refused. NaruonLiveService stays POST-only.
Persistence remains GAP-003B. ADR 0055 on the #411 lineage.
|
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: Pro Plus 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}
| if encoded.is_empty() || encoded.contains('/') { | ||
| return Err(ApiError::InvalidWirePayload); | ||
| } |
There was a problem hiding this comment.
🟡 Query-bearing export paths pass validation
A target containing ? or # passes loopback_http1_from_export_retrieval_exchange. The function returns a request the loopback service rejects instead of failing immediately.
| if encoded.is_empty() || encoded.contains('/') { | |
| return Err(ApiError::InvalidWirePayload); | |
| } | |
| if encoded.is_empty() | |
| || encoded.contains('/') | |
| || encoded.contains('?') | |
| || encoded.contains('#') | |
| { | |
| return Err(ApiError::InvalidWirePayload); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn main() -> ExitCode { | ||
| match run() { | ||
| Ok(()) => ExitCode::SUCCESS, | ||
| Err(_) => ExitCode::FAILURE, | ||
| } | ||
| } | ||
|
|
||
| fn run() -> Result<(), ApiError> { | ||
| let args: Vec<String> = std::env::args().skip(1).collect(); | ||
| let body = read_export_retrieval_cli_stdin(io::stdin().is_terminal(), io::stdin())?; | ||
| let invocation = ExportRetrievalCliInvocation::from_args(&args, body)?; | ||
| let response = execute_export_retrieval_cli(&invocation)?; | ||
| let stdout = render_export_retrieval_cli_stdout(&invocation, &response)?; | ||
| println!("{stdout}"); | ||
| if (200..300).contains(&response.status_code) { | ||
| Ok(()) | ||
| } else { | ||
| Err(ApiError::InvalidWirePayload) | ||
| } |
There was a problem hiding this comment.
🔍 Binary entry point lacks coverage
Tests call library helpers but never execute tepp-export-get. The mandatory 100% coverage gate still needs evidence for output and exit-status behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let status_line = lines.next().ok_or(ApiError::InvalidWirePayload)?; | ||
| let mut parts = status_line.split(' '); | ||
| if parts.next() != Some("HTTP/1.1") { | ||
| return Err(ApiError::InvalidWirePayload); | ||
| } | ||
| let code = parts | ||
| .next() | ||
| .ok_or(ApiError::InvalidWirePayload)? | ||
| .parse::<u16>() | ||
| .map_err(|_| ApiError::InvalidWirePayload)?; | ||
| let reason_phrase = match code { | ||
| 200 => "OK", | ||
| 202 => "Accepted", | ||
| 400 => "Bad Request", | ||
| 403 => "Forbidden", | ||
| 413 => "Payload Too Large", | ||
| 422 => "Unprocessable Entity", | ||
| _ => return Err(ApiError::InvalidWirePayload), |
There was a problem hiding this comment.
🔍 Status-line validation remains permissive
parse_http_response ignores missing, incorrect, or extra reason text. Current server output is canonical, but the advertised framing validation is incomplete.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut body = String::new(); | ||
| stdin | ||
| .read_to_string(&mut body) | ||
| .map_err(|_| ApiError::InvalidWirePayload)?; | ||
| Ok(body) |
There was a problem hiding this comment.
🟨 Unbounded stdin enables resource exhaustion
Redirected input is fully buffered before rejection. A large or endless stream can exhaust memory or occupy the CLI indefinitely.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut bytes = Vec::new(); | ||
| stream | ||
| .read_to_end(&mut bytes) | ||
| .map_err(|error| map_io_error(&error))?; | ||
| parse_http_response(&bytes) |
There was a problem hiding this comment.
🟨 Unbounded responses enable resource exhaustion
Responses are buffered to EOF before size validation. A continuously sending loopback peer can consume unbounded memory and evade the per-read timeout.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (name, value) in &exchange.headers { | ||
| if name.eq_ignore_ascii_case("host") || name.eq_ignore_ascii_case("content-length") { | ||
| continue; | ||
| } | ||
| write!(request, "{name}: {value}\r\n").map_err(|_| ApiError::InvalidWirePayload)?; |
There was a problem hiding this comment.
🟨 Exchange headers allow request injection
Arbitrary header names and values are rendered without control-character validation. A crafted NaruonHttpExchange can inject headers or alter HTTP request framing.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut request = String::new(); | ||
| write!( | ||
| request, | ||
| "{} {path} HTTP/1.1\r\nHost: {host}\r\n", | ||
| exchange.method | ||
| ) |
There was a problem hiding this comment.
🟨 Target paths allow request injection
The mutable target path enters the request line without control-character validation. Crafted CRLF bytes can inject headers or an additional HTTP request.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
changed the title
There was a problem hiding this comment.
🔍 Schema drift lacks coverage
No test loads the published schema or compares it with ExportRetrieval. Executable validation and external consumers can diverge unnoticed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.export_id.len() > EXPORT_RETRIEVAL_ID_MAX_LEN | ||
| || self.artifact_id.len() > EXPORT_RETRIEVAL_ID_MAX_LEN | ||
| || self.idempotency_key.len() > EXPORT_RETRIEVAL_ID_MAX_LEN | ||
| { | ||
| return Err(ApiError::LimitExceeded); |
There was a problem hiding this comment.
🟡 Unicode receipts violate their schema
Non-ASCII identities under 128 characters can exceed the byte checks in validate. Schema-valid receipts then fail the published Rust contract.
Prompt for agents
Align ExportRetrieval::validate with schemas/export_retrieval_v1.json. JSON Schema maxLength counts characters, but the Rust implementation currently counts UTF-8 bytes. Either use one character-based limit consistently and update byte-oriented documentation, or explicitly constrain the wire contract to an ASCII identifier alphabet in both Rust and the schema. Add multibyte identity contract tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| require_nonempty(export_id)?; | ||
| if export_id.len() > EXPORT_RETRIEVAL_ID_MAX_LEN { | ||
| return Err(ApiError::LimitExceeded); | ||
| } | ||
| let encoded_id = encode_path_segment(export_id); | ||
| let target_path = format!("{NARUON_EXPORT_PATH}/{encoded_id}"); | ||
| let target_url = compose_https_target(origin, &target_path)?; |
There was a problem hiding this comment.
🟡 Accepted IDs produce unusable requests
An ID containing / passes naruon_export_retrieval_exchange, but the server rejects its encoded path. The returned exchange can never retrieve that ID.
| require_nonempty(export_id)?; | |
| if export_id.len() > EXPORT_RETRIEVAL_ID_MAX_LEN { | |
| return Err(ApiError::LimitExceeded); | |
| } | |
| let encoded_id = encode_path_segment(export_id); | |
| let target_path = format!("{NARUON_EXPORT_PATH}/{encoded_id}"); | |
| let target_url = compose_https_target(origin, &target_path)?; | |
| require_nonempty(export_id)?; | |
| if export_id.len() > EXPORT_RETRIEVAL_ID_MAX_LEN { | |
| return Err(ApiError::LimitExceeded); | |
| } | |
| if export_id.contains('/') { | |
| return Err(ApiError::InvalidWirePayload); | |
| } | |
| let encoded_id = encode_path_segment(export_id); | |
| let target_path = format!("{NARUON_EXPORT_PATH}/{encoded_id}"); | |
| let target_url = compose_https_target(origin, &target_path)?; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn main() -> ExitCode { | ||
| match run() { | ||
| Ok(()) => ExitCode::SUCCESS, | ||
| Err(_) => ExitCode::FAILURE, | ||
| } |
There was a problem hiding this comment.
🔍 CLI failures are silent
main discards every error and emits only a failure status. Operators cannot distinguish bad arguments, connection failures, or rejected exports.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let consumer = require_headers(headers, self.bound_addr, false)?; | ||
| if consumer != NARUON_CONSUMER_CODE { | ||
| return Err(ApiError::InvalidWirePayload); | ||
| } | ||
| refuse_metrics_on_export_retrieval_payload(body)?; | ||
| let replay_key = self | ||
| .exports_by_id | ||
| .get(&export_id) | ||
| .cloned() | ||
| .ok_or(ApiError::InvalidWirePayload)?; | ||
| let stored = self | ||
| .authorized_exports | ||
| .get(&replay_key) | ||
| .ok_or(ApiError::InvalidWirePayload)?; |
There was a problem hiding this comment.
🟥 Export receipts lack caller authorization
Any local caller claiming naruon can retrieve a known export ID. read_export never verifies the authorizing tenant or principal.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Consolidated export-retrieval landing vehicle
This PR folds predecessor #411 into one naruon-facing Analysis Run / export retrieval application-adapter vehicle. The current head contains #411 as its direct ancestor, so retargeting to protected
mainpreserves the retrieval GET implementation/tests while eliminating one open micro-PR. #411 remains immutable review/history evidence; export collection/stored-request/idempotency-lookup vehicles #444/#459/#466 also retain #411 ancestry.Preserved GET behavior from #411:
POST /v1/exportsmints metric-freeexport_idandGET /v1/exports/{export_id}retrieves the purpose-bound authorization identity; naruon-only ownership, LineageWeave refusal, and exclusion of RMSE/bias/coverage/SE-gate/scientific-acceptance/terminal result/tenant/principal/source-text fields remain intact.CLI behavior on this head: published
tepp-export-get get, typed retrieval exchange, empty-stdin requirement, loopback/localhost/non-HTTPS/unpublished-consumer/LineageWeave/credential/nonempty-body refusals, noidempotency-keyrequest header, and metric-free stdout.This is one Analysis Run/export application-adapter landing vehicle, not a bounded context. ADR 0054/0055 are implementation lineage pending #437 normalization. Further compatible export retrieval mechanics should fold here or into a coherent successor rather than creating one-route PRs.
Merge only after fresh exact-head required workflows, resolved conversations, and qualifying independent approval under live ruleset 18156473. No predecessor-head evidence transfer, self-approval, or bypass.