fix(compression): fence WebUI with durable revisions - #6554
Conversation
8c0e78f to
bb58255
Compare
nesquena-hermes
left a comment
There was a problem hiding this comment.
Changes requested: keep the WebUI compatible with the pre-fence Agent contract
The current head unconditionally includes conversation_history_revision in all three AIAgent.run_conversation(...) paths:
api/streaming.py:9117(ordinary turn)api/streaming.py:9607-9609(first credential self-heal retry)api/streaming.py:10854-10856(second self-heal path)
The currently deployed Agent's strict method signature (run_agent.py:6804-6814, Agent revision e6b977933e3f980a81ec5211fa2131ce8876b9cf) does not accept that keyword and has no **kwargs. The call therefore raises before the Agent body runs, including when the revision value is None. A sandboxed exact-head probe reproduced a generic apperror containing unexpected keyword argument 'conversation_history_revision', with zero entries into the Agent method. This is a WebUI-head regression: base does not pass the keyword.
The paired Agent PR is still open, and pairing does not make a WebUI-first or independently updated installation safe. The adjacent moa_config handling already demonstrates the required rolling-compatibility pattern.
Required fix
- Centralize a capability check for the live
run_conversationcallable. Addconversation_history_revisiononly when the signature explicitly supports it or accepts**kwargs, and use that same rule at all three invocation sites. - For a pre-fence Agent, either omit the keyword and preserve the existing turn behavior, or detect an enforced minimum Agent version before accepting the turn and return an explicit actionable upgrade error. Do not let an unexpected-keyword
TypeErrorconsume the turn. - Add a strict legacy-Agent fake with the deployed parameter list and no
**kwargs. Cover ordinary turns and both self-heal paths, with bothrevision=Noneand a real revision.
The submitted focused files are otherwise green through the enforced sandbox: 18 passed. The WebUI-side snapshot/profile-scope work reads coherently, but the cross-repo atomic revision contract still needs paired acceptance coverage for same-ID api_content mutation and late writes around Agent compression publication.
bb58255 to
8986913
Compare
|
Addressed in 8986913. The WebUI now capability-gates |
|
@nesquena-hermes CI is now fully green on the compatibility fix. Re-review requested when available. |
🔬 Gate certification — RED ⛔Certified head: The prior maintainer CHANGES_REQUESTED (unconditional What I ran (isolated worktree, exact head)
⛔ Blocker (reproduced) — returned-result stale path is unclassified
So on the normal (returned-result) stale outcome the user gets a bare Precise fix-spec (small)
Non-blocking polish (do not gate on these)
Recommendation to the next agentFix #1+#2 then re-gate. This is a small, mechanical addition (classify one result code + one test) but it needs the contributor's cross-repo contract knowledge of exactly how the Agent delivers the stale outcome — bouncing to @ruizanthony. Everything else (maintainer compat blocker, atomicity, profile isolation, digest fence, full suite) is green at this head. No merge/tag/deploy performed by this lane. Full gate: Codex-reproduce + Opus 4.8 + full pytest suite (matched-control attribution) + digest mutation-sensitivity + deployed-strict-Agent compat probe. Head |
Gate fix applied — head
|
nesquena-hermes
left a comment
There was a problem hiding this comment.
Changes requested: classify stale results from both credential self-heal retries
The new commit correctly classifies a structured stale result from the ordinary run_conversation() attempt, and the previously approved capability gate remains intact at all three Agent call sites. One reachable part of the same contract is still unhandled: when either credential self-heal retry returns the structured stale result, WebUI never re-adjudicates _heal_result.
Reproduced at exact head 564d4dcad531
I adapted the PR's existing two-mode credential-self-heal regression so that the rebuilt Agent returns the same contract this commit now handles on the ordinary path:
{
"error": "compression_snapshot_stale",
"compression_snapshot_stale": True,
"partial": True,
"failed": False,
"messages": [..., {"role": "assistant", "content": "partial stale"}],
}The enforced CLEAN-scan + bubblewrap gate ran both schedules. Both failed because no stale apperror was emitted:
FAILED ...test_auth_self_heal_refreshes_revision_after_first_agent_persists_user[result]
FAILED ...test_auth_self_heal_refreshes_revision_after_first_agent_persists_user[exception]
assert apperrors and apperrors[-1]["type"] == "compression_snapshot_stale"
E assert ([])
2 failed
The code trace matches the runtime result:
- Returned-auth path,
api/streaming.py:9660-9668:_heal_resultis judged only by new assistant/token state. A stale partial result can satisfy_heal_ok, enter the success merge path, and later emit success instead of the stale terminal event. - Raised-auth path,
api/streaming.py:10910-10954: any normal_heal_resultis persisted and the function returns before error emission. - Only the primary result is passed to
_classify_provider_error(..., result=result)atapi/streaming.py:9518-9523.
The submitted focused file is otherwise green through the same sandbox (19 passed), and the previous compatibility/state-db matrix remains green (35 passed, up from 34). This is a narrow incomplete-consumer defect, not a walk-back of the earlier compatibility fix.
Required fix
- Route every
run_conversation()return through one common stale-result adjudication path. At minimum, classify_heal_resultbefore_heal_okin the returned-auth retry and before persistence/return in the raised-auth retry. - On an exact stale marker, preserve partial assistant output once, clear/materialize pending-turn ownership, emit exactly one sanitized
apperrorwith typecompression_snapshot_stale, emit nodone, and do not attempt a third Agent run. - Extend
test_auth_self_heal_refreshes_revision_after_first_agent_persists_user(or add an equivalent two-mode regression) so both first-attempt auth-failure forms return stale from attempt two. Assert two calls, the refreshed revision, one staleapperror, nodone, and retained partial/error persistence. - Exercise the accepted error-string-only and boolean-flag-only forms independently, or narrow production acceptance to the exact paired-Agent result contract.
No merge, branch write, release, production action, or screenshot was performed.
Self-heal stale-result gate fixed — head
|
nesquena-hermes
left a comment
There was a problem hiding this comment.
Changes requested: keep stale-retry partials scoped to the current turn
Thanks for fixing the two credential self-heal consumers. At 93d2832265883dab2f1074aac2724d4a396d6240, both retry paths now classify a returned stale snapshot before success/persistence, emit one compression_snapshot_stale apperror, emit no done, and stop after two Agent runs.
One deterministic transcript-corruption case remains in the new partial-retention path:
api/streaming.py::_append_result_partial_on_error()scans all ofresult["messages"]in reverse and takes the latest non-error assistant row.- If the stale retry returns
partial=Truewith history through the newly persisted current user but no assistant row for that current turn, the scan selects the previous turn's assistant answer. - Both retry error consumers then append that historical answer as a new
_partialfor the current turn.
I reproduced this in both self-heal entry modes (the first Agent call returning a 401 result and raising a 401 exception). The sandboxed characterization passed only while asserting the unsafe behavior: one replayed prior answer _partial, one stale apperror, no done, and exactly two Agent runs. The submitted changed-file suite itself remains green: 23 passed under the mandatory no-network sandbox.
Required fix
- Make result-partial extraction current-turn-owned. Give
_append_result_partial_on_error()the exact pre-call context (and currentmsg_text) and inspect only result rows added for the current call. For a self-heal result, use that retry's freshly loaded_heal_context_messagesas the baseline. - If the current-turn slice has no non-error assistant output, append no result-derived
_partial. The independent stream-buffer snapshot can still retain output that was actually streamed. - Keep
_partial_marker_already_present()deduplication. - Add a production-composed regression for both self-heal entry modes where retry result has
partial=True, contains history through the current user, and contains no current-turn assistant row. Assert that no prior answer is replayed, exactly one staleapperroris emitted, nodoneis emitted, exactly two Agent runs occur, and pending/active ownership is cleared.
This is a current-head changes request. The previously requested stale-result adjudication is resolved; the blocker is the new current-turn ownership defect introduced by the partial-retention helper.
Current-turn partial ownership fixed — head
|
25f503c to
96fd952
Compare
|
Rebased onto current |
|
CI shard-4 compatibility failure fixed at head |
7a3c54c to
144e376
Compare
|
Final rebase after upstream advanced: exact head |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Changes requested: non-prefix stale results can still replay the prior turn
Thanks for scoping result-derived partials to a pre-call baseline. The submitted append-prefix cases now pass for both credential self-heal entry modes, and the refreshed retry baselines plus deduplication are present at exact head 144e376ba24d82f75ce5e073620e2dae71197459.
One deterministic transcript-attribution defect remains in the same current-turn ownership path.
api/streaming.py::_append_result_partial_on_error() uses only len(messages) >= len(pre_call_context) before slicing messages[len(pre_call_context):]. It does not verify that the returned messages actually have pre_call_context as a prefix. When a supported compacted/replayed result replaces rather than appends to the baseline, a historical assistant row can shift into that numeric suffix. Because the refreshed baseline already ends with the current user, the fallback current-user search is skipped, and the historical answer is appended as this turn's _partial.
This result shape is part of the code's existing contract: the same module already notes that compacted/replayed results are not always append-only, and _assistant_reply_added_after_current_turn() handles it with _messages_have_prefix(...) plus _find_current_user_turn(...).
I reproduced the residual through the mandatory CLEAN threat scan and no-network bubblewrap gate: a non-prefix compacted/replayed stale result with no assistant after the current user appended the historical prior answer once as _partial. Dedup correctly suppressed a second append. The contributor's focused tests remain green, including both new append-prefix no-replay cases.
Required fix
- In
_append_result_partial_on_error(), use the established boundary rule:- if
_messages_have_prefix(messages, pre_call_context), inspect only the appended suffix; - otherwise locate the current user with
_find_current_user_turn(messages, msg_text), returnNoneif no current-user anchor exists, and inspect only rows after that anchor.
- if
- Preserve the refreshed per-self-heal baselines and
_partial_marker_already_present()deduplication. - Extend the production-composed self-heal regression with a non-prefix compacted/replayed stale result containing historical assistant rows before the last current-user row and no assistant after it, crossed with both first-entry modes. Assert zero partial replay, one
compression_snapshot_staleapperror, nodone, two Agent calls, and cleared ownership.
Targeted sandbox evidence at this exact head: the focused contributor/tip-spy gate passed 11 tests, and the changed/neighbor file slice passed 57 tests. This is still-red partial rework on the existing current-turn ownership blocker, not a walk-back of the earlier stale-result adjudication fixes.
No merge, branch write, release, production action, or screenshot was performed.
|
Reviewer defect addressed (non-prefix partial replay in Defect: the error-path partial recovery still sliced Fix: before assigning, the result is verified with the existing Regression coverage: new Tests: New head: @nesquena-hermes could you please re-review? |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Changes requested: fail closed when the current user turn is absent
Thanks for adding the prefix check and the non-prefix compacted regression. The exact numeric-suffix case from the prior review is fixed at da209652ee80, but the same historical-partial attribution blocker remains reachable through the helper reused by the new fallback.
Reproduced at exact head da209652ee80
In the non-prefix branch, _append_result_partial_on_error() calls _find_current_user_turn(messages, msg_text) and treats only None as “no current-user anchor.” That helper does not return None when the requested current turn is absent. It falls back to the last arbitrary user row:
fallback = idx
...
return fallbackI ran a production-helper characterization through the mandatory CLEAN threat scan and no-network bubblewrap sandbox. The result had partial=True, an unrelated historical user/assistant pair, no row matching the actual current prompt, and a mismatching pre-call baseline. The safety invariant failed:
assert appended is None
E AssertionError: assert {
'_partial': True,
'content': 'historical answer',
'role': 'assistant',
...
} is None
1 failed
So the new comment says “fail closed when no current-user anchor exists,” but the called helper converts that no-match state into an unrelated historical anchor. The following historical assistant is then appended and persisted as this turn's _partial.
The submitted regression is useful and passes for both first-attempt modes. It contains a matching final new webui turn, so it cannot exercise this no-match branch.
Required fix
- In
_append_result_partial_on_error(), use a safety-specific current-turn lookup that returnsNoneunless it positively matches the actual current user text. Do not reuse_find_current_user_turn()'s arbitrary-user fallback in this persistence path. - Keep workspace-prefix normalization, but require a strong/exact normalized match here. If the current turn cannot be proven, retain no result-derived partial. The independent stream buffer remains the safe source for output actually observed during this turn.
- Add a production-composed regression, crossed with both credential self-heal entry modes, where the non-prefix stale result contains an unrelated historical user followed by a historical assistant and contains no current-prompt row. Assert zero
_partialreplay, one sanitizedcompression_snapshot_staleapperror, nodone, two Agent calls, and cleared ownership. - Preserve the new prefix-vs-non-prefix split, refreshed retry baselines, and final partial deduplication.
Targeted sandbox evidence at this head: the contributor's four-file regression slice is green (59 passed), and the exact new non-prefix cases are green (2 passed). The blocker is the uncovered no-anchor branch above, not a walk-back of those fixes.
No merge, branch write, release, screenshot, server, or production action was performed.
da20965 to
29229bf
Compare
|
Corrections de la dernière revue publiées sur le head exact
Merci de re-review ce SHA exact. |
|
Exact review target updated to |
0e2bad0 to
0f7c4ce
Compare
|
Superseding the immediately previous review request: the tree is unchanged, but the final commit was republished with the repository-required verified author/committer identity. Exact WebUI review target: |
🔬 Gate certification — RED ⛔Certified contributor head: What I ran
Reproduced blockers
What is closedThe prior revision-capability, stale-result classification, no-current-anchor, repeated-prompt ownership, owner-context settlement, profile scope, and revision/digest parity findings are closed at this head. The mutation bite and latest Agent companion parity probe specifically defend those repairs. RecommendationDo not merge this head. @ruizanthony, fix the two bounded streaming defects above and re-request certification on the new exact head. A fresh full gate is required because this is an L-sized persistence/streaming path, but the next focused reproduction can be narrow: both production-shaped self-heal lanes must recover without |
a2c75ea to
a399327
Compare
|
| Filename | Overview |
|---|---|
| api/models.py | Adds typed state.db snapshots, durable active-row revision calculation, profile-scoped reads, and revision-preserving reconciliation. |
| api/streaming.py | Centralizes Agent invocation compatibility, propagates refreshed revisions, classifies stale results, and bounds partial recovery to the current turn. |
| tests/test_compression_snapshot_revision.py | Adds extensive coverage for revision construction, Agent compatibility, retries, stale settlement, repeated prompts, and exact-once partial recovery. |
| tests/test_issue5121_provider_auth_terminal_error.py | Extends terminal provider-error tests to cover successful and stale credential-recovery results. |
| tests/test_state_db_read_backstop.py | Extends state.db backstop tests for durable ordering and snapshot-return compatibility. |
Sequence Diagram
sequenceDiagram
participant DB as state.db
participant WebUI as WebUI streaming worker
participant Agent as Hermes Agent
DB->>WebUI: messages + durable revision
WebUI->>WebUI: reconcile and sanitize context
WebUI->>Agent: conversation history + compatible revision kwarg
Agent-->>WebUI: result or stale-snapshot marker
WebUI->>WebUI: settle only current-turn-owned output
WebUI-->>DB: persist terminal transcript
Reviews (7): Last reviewed commit: "Merge branch 'master' into fix/compressi..." | Re-trigger Greptile
🔬 Gate certification — RED ⛔Certified contributor head: Verdict: SHIP ONLY WITH FIXES. Two current-turn settlement defects remain on the exact paired-Agent non-prefix result shape. What I ran
Reproduced blockers
These are reachable cross-layer states, not advisor-only hypotheticals: the paired Agent reanchors its current-turn index after compaction but returns ordinary/stale result dictionaries with Required repair
RecommendationDo not merge this head. @ruizanthony, repair the paired-Agent non-prefix authority handoff above and request a fresh exact-head gate. The ordinary revision fence, profile isolation, capability gate, hosted matrix, and focused suites are otherwise in good shape. No merge, tag, deploy, close, or contributor-branch push was performed by this lane. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gate: turn-boundary fallback can select historical same-text output
Thanks for the substantive response. I re-gated exact head cdfa1b5e2e6e0a800fad29b1ecb77a79d2410ffa against the two non-prefix Agent-boundary findings on a0bbda98b4c4c16748315a9f7ba8ab2048af7d80.
The original token-only false rejection and partial drop are improved, but the replacement introduces one deterministic historical-row authority flaw.
Must fix
api/streaming.py::_find_active_turn_checkpoint_index() treats one resolved Agent index as two different projection domains. For non-prefix results it tests:
current_turn_user_idx - len(previous_context), thencurrent_turn_user_idx.
It accepts the first user row whose normalized text matches. With a repeated prompt, both positions can be matching user rows. The shifted historical row wins, so _self_heal_result_succeeded() can accept historical assistant prose as the successful retry, and _append_result_partial_on_error() can persist that historical prose as the current turn's partial.
Use exactly one declared index domain. If the Agent index addresses result["messages"], validate only that exact index. If a shifted projection is genuinely supported, carry an explicit projection-origin/base-length discriminator from the Agent rather than probing both positions.
Required regressions
Add a repeated-prompt case where both candidate positions are same-text users, historical assistant output follows only the shifted row, and current output follows only the exact row. Prove the historical row is never accepted or persisted. Compose it through both production self-heal entries, returned auth failure and raised 401, and assert exact-once settlement, persistence, done, and apperror behavior.
The new terminal=[returned, raised] direct-helper parameterization does not execute either production self-heal arm; the parameter changes only assistant text.
Verification
Threat scan CLEAN. Mandatory Layer-3 sandbox runs completed at this exact head:
tests/test_compression_snapshot_revision.py: 44 passed- adjacent authority/self-heal slice: 11 passed, 48 deselected
Those green tests omit the dual-matching-user collision above. No bare PR-code execution, merge, branch write, release, screenshot, server, or production action was performed.
3d08764 to
18342ad
Compare
|
Rebuilt the PR as one focused commit on current The rebuild preserves the five-file durable revision/profile/capability contract and repairs the remaining turn-authority and exact-once gaps: paired-Agent non-prefix boundaries are consumed as one coherent index/turn domain and validated against normalized current-user text; current-turn partial selection stops at the next user; repeated prompts cannot select historical output; live-buffer/result snapshots consolidate into one Local evidence on the exact tree:
The two inline P1 findings have been answered with the exact fix SHA. Hosted checks are now running against that SHA. |
18342ad to
c8bd54f
Compare
🔬 Gate certification — GREEN ✅ (durable revision fencing converged; legacy Agent lane remains safe)Certified contributor head: Verdict: exact-head Authoritative gate
The shared 9 failures + 2 errors are the established sandbox/env baseline, byte-for-byte identical on candidate and pinned control. Installed-runtime compatibility
Prior RED closure
Non-blocking follow-upCoordinate exact digest/count semantics when Agent PR #72806 lands. That future runtime boundary cannot be exercised by today's installed Agent, but its absence does not regress the current product lane. RecommendationGate-pass / Tier 1. Safe for release-agent current-head validation and normal merge/release mechanics. Preserve @ruizanthony attribution. This gate does not merge, tag, deploy, close, or write the contributor branch. Valid only for exact contributor head |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Independent release-manager re-gate at exact head c8bd54fc — GREEN, autonomous-ship (crown-jewel backend, no screenshot surface). Clears the stale CHANGES_REQUESTED (the gate-cert + this re-gate both post-date it).
- Codex regression gate: SAFE TO SHIP, and it reproduced the load-bearing backward-compat: the installed
AIAgent.run_conversationlacksconversation_history_revision, soapi/streaming.py:8295-8350omits it (retainingpersist_user_timestamp); a full strict-legacy send completed withdone— noapperror, noTypeError— and persisted the answer. Stale snapshots settle once as terminal errors (no retry loop); partial output stays with the authoritative current turn; compression rotation preserves the parent snapshot + continuation transcript. The future paired-Agent fence is dormant (kwarg omitted) until the companion Agent PR ships, and is covered by capable-Agent contract tests. - Full serial suite (my box): 14,914 passed. The 52 non-passing (16 fail + 36 error) are pre-existing same-box environment artifacts (
test_mcp_servermcp-SDK version mismatch +test_issue1699model-cache-fingerprint flake) — proven by a same-box clean-master control reproducing the identical set; this PR touches neither file and has zero compression/streaming/models/state failures. - Consistent with the 2026-08-21 02:33 gate-cert (Codex SAFE + senior GREEN + 81/81 focused + zero PR-owned residuals in its own sandbox baseline).
Approving to clear the stale review state. Thanks @ruizanthony.
…sions (#6554, @ruizanthony) (#7194) Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
|
Shipped in exp-v0.52.257 🎉 — WebUI now fences the transcript with a durable revision so cancelled/interrupted compression can't lose or misattribute work. Live on the experimental channel. The paired-Agent fence stays dormant on current Agent builds (WebUI omits the new revision kwarg via signature detection) and activates once the companion Agent build ships — existing installs are unaffected. Thanks @ruizanthony! |
Summary
Capture WebUI history and its durable transcript revision atomically, then pass the same identity to compatible Hermes Agent versions for compression freshness checks.
This is the WebUI half of the durable fence: projection differences are valid; true concurrent SQLite mutations must block stale publication.
Changes
api_contentdigest;conversation_history_revisionfor strict legacy Agents and pass it to explicit/variadic compatible Agents;Exact pairing
320789ae596a3963d726d90f6c7f3bc86f7f2d6d;0f7c4cef3f38aab591ae6fa6fcfdb76a869c669a;1bdac5d4547914d70c134f06af0435568a35159c;cb11a7e25579638c9f67e8501dd151f581c4c942.Validation
git diff --check: passed;The complete guarantee requires both PRs. Rolling WebUI-first installations remain functional through the capability gate but do not gain the durable fence until the Agent side is upgraded.