feat(api): enumerate interpretation runs via loopback collection GET - #433
Conversation
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.
![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 needs follow-up
Repository rules require 100% production line and branch coverage. The submitted verification omits fresh coverage-gate evidence for the new public branches.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🔍 Bodyless GET requires content type
refuse_common_live_headers requires content-type: application/json for an empty GET. The documented exchange includes it, but generic HTTP clients often omit it.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.idempotency_key.len() > INTERPRETATION_RUN_COLLECTION_CURSOR_MAX_LEN { | ||
| return Err(OrchestratorLiveError::LimitExceeded); |
There was a problem hiding this comment.
🔴 Long keys disable run listing
A POST accepts idempotency keys above 128 bytes, but InterpretationRunCollectionItem::validate rejects that stored run during every GET. One such run makes the entire collection return 413.
Prompt for agents
Align creation and collection validation for idempotency keys. InterpretationRunRequest::validate in crates/orchestrator_live/src/request.rs currently accepts keys up to the overall payload/header limits, while InterpretationRunCollectionItem::validate rejects keys longer than 128 bytes. Consequently, an accepted POST can poison every collection GET. Enforce a shared key limit before storing runs, or make collection pagination support every key accepted by POST, and add an integration test that creates a key above 128 bytes before listing.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let next_cursor = if end < items.len() { | ||
| Some(items[end - 1].idempotency_key.clone()) |
There was a problem hiding this comment.
🟡 Zero page limit panics
With nonempty input and a zero limit, page_interpretation_run_collection_items subtracts one from zero. The exported helper panics instead of returning a page.
Prompt for agents
Make page_interpretation_run_collection_items safe for every value its public usize limit accepts. The service validates limits first, but external crate callers can pass zero or values that overflow start + limit. Define invalid-limit behavior, preferably by returning Result with the same parser errors, or otherwise guarantee non-panicking arithmetic and indexing. Add direct tests for zero and usize::MAX.
Was this helpful? React with 👍 or 👎 to provide feedback.
| items.sort_by(|left, right| left.idempotency_key.cmp(&right.idempotency_key)); | ||
| let start = cursor.map_or(0, |cursor| { | ||
| items | ||
| .iter() | ||
| .position(|item| item.idempotency_key.as_str() > cursor) | ||
| .unwrap_or(items.len()) | ||
| }); |
There was a problem hiding this comment.
📝 Info: Exclusive cursor tolerates missing keys
page_interpretation_run_collection_items resumes at the first greater key. Deleted or synthetic cursors therefore preserve lexical progress without requiring an exact match.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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.
🟨 Origin validation permits embedded paths
An origin containing a path passes contextual_orchestrator_interpretation_run_collection_exchange. The resulting request targets an unintended route instead of the configured host root.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f94a77316f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.idempotency_key.len() > INTERPRETATION_RUN_COLLECTION_CURSOR_MAX_LEN { | ||
| return Err(OrchestratorLiveError::LimitExceeded); |
There was a problem hiding this comment.
A POST currently accepts any nonempty idempotency key within the overall 64 KiB request limit, but this new collection row rejects keys longer than 128 bytes. After a client successfully creates a run with a matching 129-byte header/body key, every collection GET encounters that stored run here and returns 413 instead of a page. Enforce the same bound at the create boundary or support all previously accepted keys in collection rows.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| 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.
When the exported helper is called with a nonempty item list and limit == 0, end is zero while end < items.len() is true, so end - 1 underflows and the function panics while indexing. The HTTP parser rejects zero, but this helper is publicly re-exported and its signature and documentation impose no nonzero precondition; make the invalid limit unrepresentable or return an error before indexing.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
| accepted.orchestration_mode(), | ||
| accepted.claim_status(), | ||
| accepted.scientific_authority(), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
When the service has accumulated many accepted runs, every paginated GET clones every accepted row into a Vec and then sorts the entire registry before returning at most 64 items. Because POST places no bound on the registry size, request-time memory and CPU remain unbounded despite the page limit, and repeatedly listing a large registry can exhaust the loopback process. Store rows in cursor order or range over only enough entries to produce the requested page.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| if rest.contains('@') || rest.contains('?') || rest.contains('#') || rest.contains('\\') { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| if host_implies_table_access(rest) { |
There was a problem hiding this comment.
For an input such as https://:443, rest is nonempty, contains none of the rejected characters or table-access substrings, and the builder returns https://:443/v1/interpretation-runs as a successful exchange even though the URL has no host. Consumers then fail later while parsing or dispatching an exchange that this boundary declared valid; parse the origin as a URL and require a valid host/authority before returning it.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| require_nonempty(raw)?; | ||
| if raw.contains('/') || raw.contains('\0') { | ||
| return Err(OrchestratorLiveError::InvalidWirePayload); | ||
| } | ||
| if raw.len() > INTERPRETATION_RUN_COLLECTION_CURSOR_MAX_LEN { |
There was a problem hiding this comment.
A cursor such as idem-a\r\nAuthorization: Bearer x passes this validation because only slash and NUL are rejected, and the exchange builder then places it verbatim in the public headers vector. A direct HTTP/1.1 serializer can turn that value into an injected credential header, while a validating HTTP client will reject an exchange that the builder reported as valid. Reject all HTTP control characters before constructing the typed exchange.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
|
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 |
1 participant
Folded into #436
Closed as
superseded_by_fold, not discarded. #436's head contains this PR as its direct ancestor and has been retargeted to this PR's former base, so the interpretation-run collection GET implementation/tests and this review history remain intact while queue WIP is reduced. Retrieval vehicles #439/#454 also retain this ancestry.Canonical collection landing vehicle: #436 (
feat(api): consolidate interpretation-run collection GET and CLI).Do not reopen unless a surviving folded head demonstrably loses unique behavior or evidence.