Skip to content

feat(agent): consume the out-of-band hermes_confab_notice response extension - #764

Merged
Kyzcreig merged 1 commit into
mainfrom
feat/confab-notice-consumer
Sep 20, 2026
Merged

Kyzcreig merged 1 commit into
mainfrom
feat/confab-notice-consumer

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

What

Hermes-side consumer for the out-of-band confab-catch notice defined in
claude-bpx docs/SPEC-confab-marker-out-of-band.md v1.

A bridge that detects and strips self-fabricated scaffold text from a model reply
must still tell the user — that signal is load-bearing for triage. Today it is
delivered in band, appended to assistant content, which mutates conversation
history and gets replayed upstream on any full-history recovery. This PR moves the
notice to a versioned response-envelope extension: a status event for the current
turn, and presentation-only metadata on the assistant row for historical triage.

Consumer-first is mandatory per the spec — this lands before any producer emits
the field. With today's bridge (no extension field) behavior is unchanged, and
that is pinned by a test.

Transport contract

{"hermes_confab_notice": {"version": 1, "kind": "scaffold_confab_removed",
                          "request_id": "3b264082", "scope": "visible",
                          "grammar": "inbound"}}

Non-stream: top-level on the completion. Stream: on the final usage chunk
(choices: []).

Changes

File What
agent/confab_notice.py New. The single validation gate. Fails closed on unknown version, wrong kind, bad scope, non-string/empty/oversized request_id, bad grammar. Returns a fresh dict of only the five contract keys, so a provider cannot smuggle extra fields into display_metadata.
agent/chat_completion_helpers.py Stream path scans chunks for the extension, accepts at most one per response (a second is logged and dropped), forwards it on the synthetic completion. build_assistant_message stamps display_kind='confab_notice' + versioned display_metadata.
agent/transports/chat_completions.py Same extraction/validation for non-stream completions, preserved in NormalizedResponse.provider_data.
agent/transports/types.py NormalizedResponse.confab_notice accessor, alongside the existing codex_* / reasoning_* ones.
agent/conversation_loop.py Emits the status line once per notice via _emit_status (CLI + TUI + gateway). A request_id ledger stops a retry/fallback re-normalizing the same response from double-emitting.
hermes_cli/cli_agent_setup_mixin.py, apps/desktop/src/types/hermes.ts Render the new display_kind in the CLI resume recap and the desktop type union.

Assistant content is never touched — no suffix, no prefix.

Replay exclusion

Both presentation fields are already stripped from every outgoing copy in
conversation_loop's api_msg builder. The spec's own line citation for that
strip moved twice, so the new contract test pins it by symbol — an AST walk
for api_msg.pop("display_kind"/"display_metadata") inside the function that also
calls _clone_message_for_send — plus a wire-level e2e asserting the notice never
appears in outgoing messages[].

Per spec §consumer step 4 the flush is not modified; the existing
display_metadata pass-through is asserted instead, so a future refactor that
drops it fails loudly.

Verification

tests/agent/test_confab_notice.py              42 passed
tests/agent/test_confab_notice_e2e.py           8 passed  (stream + non-stream)
tests/agent/test_confab_notice_replay_strip.py  5 passed
tests/tui_gateway/test_confab_notice_render.py  5 passed

The e2e suite drives the real agent loop against an in-process mock provider
returning the v1 object in both wire shapes, and asserts on the captured
outgoing request bytes
, not on internal state.

Mutation proofs (the tests are not vacuous):

  1. Delete the two api_msg.pop lines → both AST pins and both wire e2e tests go
    red (5 failed, 8 passed). Restored.
  2. Delete "display_metadata" from the flush row dict → the persistence pin and
    both persistence e2e tests go red (3 failed, 6 passed). Restored.

Neighbor suites, all green:

tests/agent/transports + sidecar     18 files, 420 passed
streaming + display                   9 files, 127 passed
CLI resume + gateway history          6 files, 244 passed

Not in this PR

The bpx producer. It is a separate card, blocked on this one — the bridge must not
emit the field until this consumer is deployed.

…tension

A bridge that catches and strips self-fabricated scaffold text from a model
reply must still tell the user — that signal is load-bearing for triage. It is
currently delivered IN BAND, appended to assistant content, which mutates
conversation history and is replayed upstream on any full-history recovery.
Move the consumer side to a versioned response-envelope extension, per
claude-bpx docs/SPEC-confab-marker-out-of-band.md v1.

Consumer-first is mandatory: this lands before any producer emits the field.
With today's bridge (no extension field) behavior is unchanged.

- agent/confab_notice.py: the single validation gate. Fails closed on unknown
  version, wrong kind, bad scope, non-string request_id, oversized labels.
  Returns a fresh dict of only the five contract keys, so a provider cannot
  smuggle extra fields into display_metadata.
- chat_completion_helpers._call_chat_completions: scan chunks for the
  extension (contract puts it on the final usage chunk), accept at most one
  per response, forward it on the synthetic completion.
- ChatCompletionsTransport.normalize_response: same extraction for non-stream
  completions, preserved in NormalizedResponse.provider_data.
- conversation_loop: emit CONFAB_NOTICE_TEXT once per notice (request_id
  ledger stops a retry/fallback double-emit) via _emit_status, which reaches
  CLI, TUI and gateway.
- build_assistant_message: stamp display_kind='confab_notice' plus versioned
  display_metadata on the assistant row. Content is never touched.
- CLI resume recap + desktop display_kind union render the new tag.

Replay exclusion is an existing invariant, not a promise. Pinned by symbol
(AST walk of api_msg.pop) rather than line number, since the spec's own line
citation moved twice.

Verified:
- tests/agent/test_confab_notice.py 42 passed
- tests/agent/test_confab_notice_e2e.py 8 passed (stream + non-stream, real
  agent loop against an in-process mock provider)
- tests/agent/test_confab_notice_replay_strip.py 5 passed
- tests/tui_gateway/test_confab_notice_render.py 5 passed
- mutation proof 1: delete the two api_msg.pop lines -> 2 AST pins + both wire
  e2e tests go red (5 failed, 8 passed); restored.
- mutation proof 2: delete "display_metadata" from the flush row dict ->
  persistence pin + both persistence e2e tests go red (3 failed, 6 passed);
  restored.
- neighbor suites green: tests/agent/transports (18 files, 420 passed),
  streaming + display (9 files, 127 passed), CLI resume/gateway history
  (6 files, 244 passed).
@Kyzcreig
Kyzcreig added this pull request to the merge queue Sep 20, 2026
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

FleetReview

Confidence: 3/5

Findings

  • P1 agent/chat_completion_helpers.py:5225 — Duplicate Accepted
  • P1 agent/conversation_loop.py:7670 — Alert Suppression
  • P1 hermes_cli/cli_agent_setup_mixin.py:801 — False Notice
  • P1 apps/desktop/src/types/hermes.ts:586 — Desktop clients never surface the confabulation notice
  • P1 tests/agent/test_confab_notice.py:201 — Provenance Untested
  • P2 tests/agent/test_confab_notice_e2e.py:243 — Missing Marker
  • P2 tests/agent/test_confab_notice_e2e.py:269 — Incomplete Validation
  • P2 tests/agent/test_confab_notice_replay_strip.py:145 — Self-Testing
  • P1 tests/agent/test_confab_notice.py:137 — Streaming Untested
  • P2 tests/agent/test_confab_notice_e2e.py:188 — Mode Coverage
  • P3 agent/confab_notice.py:18 — Module docstring's fail-closed list claims duplicate-notice behaviour the code does not implement
  • P2 tests/agent/test_confab_notice_e2e.py:119 — Fixture never calls server_close(), and server/tempdir setup sits outside the try/finally
  • P1 agent/chat_completion_helpers.py:2488 — Persisted notices are invisible in reloaded TUI and desktop sessions

FleetReview provenance · models: B=gpt-5.6-sol, C=claude-code-opus-5, F=gpt-5.6-sol, G=grok-4.6 · cost: $35.76 · duration: 1h 12m 29s · rounds: 2 · files examined: 11

Merged via the queue into main with commit a2a1a5c Sep 20, 2026
95 of 97 checks passed
@Kyzcreig
Kyzcreig deleted the feat/confab-notice-consumer branch September 20, 2026 20:46
@Kyzcreig
Kyzcreig restored the feat/confab-notice-consumer branch September 21, 2026 10:32
Kyzcreig added a commit that referenced this pull request Sep 24, 2026
Re-port #779 onto current main without reverting #764 validation or #787 usage accounting. Carry fixed kind-specific labels and metadata-only history events; share the dropped-call retry budget and strip ephemeral correction pairs at finalization.

Verified: 160 confab/CLI/gateway tests, 42 dropped-call and usage tests, 33 finalizer/status tests; git diff --check.
Kyzcreig added a commit that referenced this pull request Sep 24, 2026
Re-port #779 onto current main without reverting #764 validation or #787 usage accounting. Carry fixed kind-specific labels and metadata-only history events; share the dropped-call retry budget and strip ephemeral correction pairs at finalization.

Verified: 160 confab/CLI/gateway tests, 42 dropped-call and usage tests, 33 finalizer/status tests; git diff --check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant