feat(api): consolidate interpretation-run create and collection adapters - #436
feat(api): consolidate interpretation-run create and collection adapters#436seonghobae wants to merge 3 commits into
Conversation
GAP-003A unique slice: tepp-interpretation-runs create mints typed contextual-orchestrator POST /v1/interpretation-runs onto spawned tepp-orchestrator-loopback TCP. Metric-free hypothetical JSON only. Naruon and LineageWeave are refused. Persistence remains GAP-003B. ADR 0064 unique vs main.
GET /v1/interpretation-runs lists accepted hypothetical runs on tepp-orchestrator-loopback so operators do not guess idempotency keys. Rows stay metric-free with claim_status=hypothetical. Naruon and LineageWeave are refused. Empty body; idempotency-key is refused.
GAP-003A unique slice stacked on collection GET: tepp-interpretation-runs list mints typed contextual-orchestrator GET /v1/interpretation-runs onto spawned tepp-orchestrator-loopback TCP. Metric-free hypothetical identities only. Empty stdin admitted. Naruon and LineageWeave refused. ADR 0070.
|
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
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.
🔍 Duplicated transports already diverge
Create and list duplicate flag parsing, TCP setup, framing, and response parsing. Their size bounds already differ, making future contract fixes easy to apply inconsistently.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🔍 Rust verification remains external
The environment lacks cargo, so the new tests could not run locally. Current-head CI must provide the repository’s required verification evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
| InterpretationRunCollectionItem::new( | ||
| accepted.interpretation_run_id(), | ||
| accepted.idempotency_key(), | ||
| accepted.orchestration_mode(), | ||
| accepted.claim_status(), | ||
| accepted.scientific_authority(), | ||
| ) |
There was a problem hiding this comment.
🟡 Long run keys disable listing
After POST accepts a key over 128 bytes, InterpretationRunCollectionItem::new rejects every list request. One accepted run makes the collection unavailable.
Prompt for agents
InterpretationRunRequest and InterpretationRunAccepted admit idempotency keys up to the overall JSON size, while InterpretationRunCollectionItem rejects keys longer than INTERPRETATION_RUN_COLLECTION_CURSOR_MAX_LEN. The list handler converts every accepted run, so one previously accepted long key makes all collection GETs return 413. Align creation-time idempotency-key validation with the collection cursor bound, or redesign cursor encoding so every accepted run is listable without treating its raw key as a bounded cursor. Add a POST-then-GET regression test using a key longer than 128 bytes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let end = (start + limit).min(items.len()); | ||
| let next_cursor = if end < items.len() { | ||
| Some(items[end - 1].idempotency_key.clone()) |
There was a problem hiding this comment.
🔴 Zero page limits crash callers
With items and a zero limit, page_interpretation_run_collection_items subtracts one from zero. The public helper panics instead of returning a page.
Prompt for agents
The public page_interpretation_run_collection_items helper accepts an arbitrary usize limit but assumes it is nonzero when deriving next_cursor. A zero limit with a nonempty collection underflows at items[end - 1], and very large limits can also overflow start + limit. Make the helper validate the limit or change its API to return Result, and use overflow-safe end calculation. Update direct helper tests for zero and extreme limits while preserving validated service behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| .unwrap_or(usize::MAX); | ||
| let mut service = OrchestratorLiveService::bind(bind_addr)?; | ||
| println!("{}", service.local_addr()?); | ||
| (0..request_limit).for_each(|_| drop(service.serve_one())); |
There was a problem hiding this comment.
🟡 Server failures exit successfully
drop(service.serve_one()) discards accept and response-write errors. A bounded server counts failed attempts and exits successfully without serving the requested traffic.
| (0..request_limit).for_each(|_| drop(service.serve_one())); | |
| for _ in 0..request_limit { | |
| service.serve_one()?; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn run_create(args: &[String]) -> Result<(), OrchestratorLiveError> { | ||
| let body = read_interpretation_run_cli_stdin(io::stdin().is_terminal(), io::stdin())?; | ||
| let invocation = InterpretationRunCliInvocation::from_args(args, body)?; | ||
| let response = execute_interpretation_run_cli(&invocation)?; | ||
| let stdout = render_interpretation_run_cli_stdout(&invocation, &response)?; | ||
| println!("{stdout}"); | ||
| if (200..300).contains(&response.status_code) { | ||
| Ok(()) | ||
| } else { | ||
| Err(OrchestratorLiveError::InvalidWirePayload) | ||
| } | ||
| } | ||
|
|
||
| fn run_list(args: &[String]) -> Result<(), OrchestratorLiveError> { | ||
| let body = | ||
| read_interpretation_run_collection_cli_stdin(io::stdin().is_terminal(), io::stdin())?; | ||
| let invocation = InterpretationRunCollectionCliInvocation::from_args(args, body)?; | ||
| let response = execute_interpretation_run_collection_cli(&invocation)?; | ||
| let stdout = render_interpretation_run_collection_cli_stdout(&invocation, &response)?; | ||
| println!("{stdout}"); | ||
| Ok(()) |
There was a problem hiding this comment.
🔍 CLI failures expose inconsistent output
Create prints server error bodies before failing, while list suppresses every non-200 body. Operators receive different diagnostics from one consolidated command.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let items = self | ||
| .accepted_runs | ||
| .values() | ||
| .map(|(_, accepted)| { | ||
| InterpretationRunCollectionItem::new( | ||
| accepted.interpretation_run_id(), | ||
| accepted.idempotency_key(), | ||
| accepted.orchestration_mode(), | ||
| accepted.claim_status(), | ||
| accepted.scientific_authority(), | ||
| ) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| let (page, next_cursor) = | ||
| page_interpretation_run_collection_items(items, cursor.as_deref(), limit); | ||
| let collection = InterpretationRunCollection::new(page, next_cursor)?; |
There was a problem hiding this comment.
📝 Info: Cursor ordering is internally stable
Accepted entries are immutable and keys are unique. Sorting by key makes the exclusive cursor stable for the in-memory registry, including later insertions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn new( | ||
| items: Vec<InterpretationRunCollectionItem>, | ||
| next_cursor: Option<String>, | ||
| ) -> Result<Self, OrchestratorLiveError> { | ||
| if items.len() > INTERPRETATION_RUN_COLLECTION_MAX_LIMIT { | ||
| return Err(OrchestratorLiveError::LimitExceeded); | ||
| } | ||
| for item in &items { | ||
| item.validate()?; | ||
| } | ||
| if let Some(cursor) = next_cursor.as_deref() { | ||
| parse_interpretation_run_collection_page_cursor(Some(cursor))?; | ||
| } | ||
| Ok(Self { | ||
| contract_version: INTERPRETATION_RUN_CONTRACT_VERSION, | ||
| items, | ||
| next_cursor, | ||
| }) |
There was a problem hiding this comment.
🔍 Public pages omit pagination invariants
InterpretationRunCollection::new admits unordered rows and unrelated next cursors. Only the CLI renderer enforces those invariants for exported collection pages.
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); | ||
| } | ||
| parse_interpretation_run_collection_page_cursor(page_cursor)?; | ||
| parse_interpretation_run_collection_page_limit(page_limit)?; | ||
| let mut headers = vec![ | ||
| ("content-type".into(), "application/json".into()), | ||
| ( | ||
| "tepp-consumer".into(), | ||
| CONTEXTUAL_ORCHESTRATOR_CONSUMER_CODE.into(), | ||
| ), | ||
| ("tepp-contract-version".into(), "1".into()), | ||
| ]; | ||
| if let Some(cursor) = page_cursor { | ||
| headers.push(("tepp-page-cursor".into(), cursor.to_owned())); | ||
| } | ||
| if let Some(limit) = page_limit { | ||
| headers.push(("tepp-page-limit".into(), limit.to_owned())); | ||
| } | ||
| Ok(InterpretationRunCollectionHttpExchange { | ||
| method: "GET", | ||
| target_url: format!("{origin}{INTERPRETATION_RUN_PATH}"), |
There was a problem hiding this comment.
🔍 Exchange builder admits malformed origins
The exported builder accepts empty authorities and origins containing paths. CLI rendering later rejects many, but direct callers receive malformed target URLs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut body = String::new(); | ||
| stdin | ||
| .read_to_string(&mut body) | ||
| .map_err(|_| OrchestratorLiveError::InvalidWirePayload)?; | ||
| Ok(body) |
There was a problem hiding this comment.
🟨 Unbounded streams enable memory exhaustion
read_to_string and read_to_end buffer untrusted streams without limits. Oversized stdin or loopback responses can exhaust the create process's memory.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
the
enhancement
ChatGPT Codex Connector
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head COMMENT on 460503b (draft). Unique occupied: GET /v1/interpretation-runs plus tepp-interpretation-runs (ADR 0069/0070). Collection rows stay metric-free identities with claim_status=hypothetical and scientific_authority=false. FORBIDDEN_COLLECTION_KEYS includes rmse/scientific_acceptance/tenant_workspace_id/causal_score/findings/evidence_text. Naruon and LineageWeave refused. NaruonLiveService stays POST-only.
Do not duplicate this collection surface. Do not un-draft. Predecessor Devin COMMENTED on this SHA is not independent APPROVE. Zero current-head APPROVE. Never self-approve. Persistence remains GAP-003B. Do not weaken fail-closed. No Buyer language.
1 participant
Consolidated interpretation-run adapter vehicle
This PR now folds predecessor #425 together with the already-folded #433 collection GET into one contextual-orchestrator-facing Analysis Run / interpretation-run application-adapter vehicle. Its current head contains #425 -> #433 -> collection CLI ancestry, so retargeting to protected
mainpreserves all three increments while removing another open micro-PR. #425/#433 remain immutable review/history evidence; retrieval vehicles #439/#454 retain the same ancestry.Preserved create behavior from #425:
tepp-orchestrator-loopback, publishedtepp-interpretation-runs create,InterpretationRunRequeststdin, contextual-orchestrator-only consumer, typed POST exchange, metric-free hypothetical stdout withclaim_status=hypotheticalandscientific_authority=false, loopback/credential/consumer/metric fail-closed boundaries, and no direct model-provider/scientific authority.Preserved collection behavior:
GET /v1/interpretation-runsplus publishedlistCLI, bounded exclusive cursor pagination, metric-free hypothetical identities, extra-path/idempotency-header refusal, and naruon/LineageWeave refusal.This is one Analysis Run application/adapter landing vehicle, not a new bounded context. Per-operation ADR 0064/0069/0070 identifiers are implementation lineage pending #437 normalization, not independent architecture authority. Future compatible interpretation-run transport operations should fold into this vehicle or a coherent successor rather than mint 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.