feat(compact): reactive compaction with direct input support - #726
Conversation
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Completes five compaction features from #30: previous_usage token estimation fast-path, configurable summary_prefix, hiding compaction items from input_items, persisting compaction responses, and an explicit POST /v1/responses/compact endpoint.
Overall: Solid implementation with good integration tests covering both the rehydrated and direct-input paths. The summary prefix plumbing is clean and consistent across both translation paths. A few issues below around blocking in async context, stale docstrings, and missing unit test coverage for the new endpoint.
| Severity | Count |
|---|---|
| Large | 3 |
| Medium | 3 |
Findings without inline placement
[Large] Stale docstrings — The struct-level doc comment on CompactFilter (~line 96 of mod.rs) says compaction "only applies to multi-turn requests where openai_responses_rehydrate has loaded stored conversation history". The generated docs page (docs/filters/openai_responses_compact.md, Configuration Notes section) says the same. Both are now incorrect — this PR adds a direct-input compaction path that runs without rehydration. Update both to match the module-level doc comment (lines 14-20), which was correctly updated.
[Large] Missing unit tests for explicit compact endpoint — The new pure/mostly-pure functions parse_compact_request_body, extract_stored_messages, is_explicit_compact_request, and ensure_compactable_state have no unit tests. Per project convention each testable function should have coverage. Suggested cases:
parse_compact_request_body: empty body, invalid JSON, missingresponse_id, valid request with optional fieldsextract_stored_messages: empty array, non-array messages, valid arrayis_explicit_compact_request: POST to correct path, GET to correct path, POST to wrong pathensure_compactable_state: noResponsesState, rehydrated state, non-rehydrated with compaction config, non-rehydrated without compaction config
43ac0b3 to
b0046dd
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (Aug 18 commits)
Previous review had 6 findings. The new commits addressed 4 fully (async conversion of fetch_response/build_and_persist_compaction, _body rename, response_id emptiness validation, tiktoken overhead restoration). One partially addressed (unit tests added for parse_compact_request_body and extract_stored_messages, but is_explicit_compact_request and ensure_compactable_state still lack coverage). One unaddressed (stale docstrings).
One new finding below from the rewritten explicit compact endpoint.
| Severity | Count |
|---|---|
| Medium | 1 |
aslakknutsen
left a comment
There was a problem hiding this comment.
Outside the PR diff, but
ai/apis/src/openai/responses/store/filter.rs
Line 464 in bc1e5bd
"ResponseStoreFilter::should_init_store_for_request() returns false for POST /v1/responses/compact because the path is not is_responses_create and the classifier does not set has_previous_response_id for sub-resource routes. The store Arc is therefore never registered into ResponseStoreRegistry before CompactFilter::resolve_store_and_tenant() runs. On a freshly built pipeline (or after reload), the first explicit compact call fails with "response store not available" even when the backend is healthy. Extend should_init_store_for_request (or equivalent) to initialize the store for POST /v1/responses/compact, mirroring how rehydrate paths are handled."
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review (re-review)
The Aug 20 commits fixed the response_object finding from the previous re-review (now includes model, created_at, and output).
One new finding from the direct-input compaction path.
| Severity | Count |
|---|---|
| Medium | 1 |
9d6b735 to
3a48a2c
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (Aug 24 commits)
The two new commits fix the summarization scope (exclude current input from summarization text in the rehydrated path), add is_compactable unit tests, add an explicit compact endpoint integration test, fix the stale docstrings on CompactFilter and the docs page, and add store initialization for the compact endpoint.
Previous findings addressed:
- Stale docstrings: fixed (struct doc and docs page updated)
- Missing
is_compactable/ensure_compactable_statetests: fixed (extractedis_compactableand added 5 tests)
One new finding below.
| Severity | Count |
|---|---|
| Medium | 1 |
|
@crstrn13 please rebase |
b9908e8 to
e849dfe
Compare
|
Missing Signed-off-by: e849dfe. All commits require sign-off (via |
e849dfe to
d2de464
Compare
…roxy#30) Signed-off-by: Alexander Cristurean <acristur@redhat.com>
f6dc948 to
359559a
Compare
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
…-issue-30 Signed-off-by: Alexander Cristurean <acristur@redhat.com> # Conflicts: # apis/src/openai/responses/compact/mod.rs # tests/integration/tests/suite/examples/compact.rs
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Rephrase the module-level Scope doc so a line no longer begins with "- ", which clippy's doc_lazy_continuation lint treated as a markdown list item with unindented continuations. No behavior change. Signed-off-by: Alexander Cristurean <acristur@redhat.com>
dba98b1 to
95925a4
Compare
…-issue-30 Signed-off-by: Alexander Cristurean <acristur@redhat.com> # Conflicts: # apis/src/openai/responses/compact/mod.rs
- build_usage: use saturating_add for total_tokens to avoid overflow - extract_compaction_config: reject present-but-non-array context_management with an error; keep absent/null as no-op. Extract per-entry parsing into parse_compaction_entry helper to stay within the line limit. - build_and_persist_compaction: document the fail-open pass-through arm (usage.output_tokens is 0; caller detects the no-op via the absent compaction item). Adds unit tests for null-as-absent and non-array-error cases. Signed-off-by: Alexander Cristurean <acristur@redhat.com>
The module-level # Scope section only described reactive multi-turn compaction and omitted POST /v1/responses/compact, which summarizes any previously stored response regardless of rehydration. Bring it in line with the CompactFilter struct doc, which already describes both the reactive and explicit scenarios. Signed-off-by: Alexander Cristurean <acristur@redhat.com>
CI's nightly rustfmt reflows doc-comment prose to the configured comment_width (120); the hand-wrapped bullets from the previous commit tripped `fmt --all --check`. Reflow to match. Signed-off-by: Alexander Cristurean <acristur@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
last one on_failure: open returns raw input messages inside CompactResource.output. Those lack required output fields such as type, id, and status, so typed OpenAI clients may reject the response.
…ersistence. Signed-off-by: Alexander Cristurean <acristur@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
final:
Persisted response.compaction objects have no status, but rehydration requires status == "completed". Referencing the returned ID therefore fails as status 'unknown', despite the new code claiming follow-up continuity.
Accept response.compaction records during rehydration and add an actual follow-up test.
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
Thanks for your patience!
…-issue-30 # Conflicts: # apis/src/openai/responses/store/filter.rs
Summary
Completes the remaining work items from #30:
previous_usagefor token estimation —should_compact()checks the storedusage.total_tokensfrom the rehydrated response before falling back to local tiktoken countingsummary_prefixconfig option (default:[Previous conversation summary]) carried through to compaction items and both translation pathsinput_itemsAPI —normalize_input_items()filters out{"type": "compaction"}items so clients never see internal statePOST /v1/responses/{id}/compacttriggers compaction on a previously stored responseBehavior details
compact_threshold(e.g. belowMIN_COMPACT_THRESHOLD) returns400 invalid_request_errorbefore the compactability gate, for both streaming and non-streaming requests, and for direct-input requestsshould_compact()summarizes the entire rehydrated history including the current turn, so the summarizer has complete context. The current turn is not lost or duplicated:replace_messages()preserves it verbatim after the compaction item via a tail splitTest plan
cargo test -p praxis-ai-apis— unit tests for all work itemscargo test -p praxis-ai-filters— no filter regressionscargo test -p praxis-tests-integration— example integration tests (mock backends), incl.compact.rstest_openai_responses_vllm.py) — invalid-threshold rejection and over-threshold rehydrated-history compaction, against live vLLM + OGXmake lint,rustdoc, unicode-safety, DCO/signoff all green