Skip to content

feat(honcho): add per-turn observation opt-out phrases - #85040

Open
seradin wants to merge 2 commits into
NousResearch:mainfrom
seradin:seradin/honcho-observation-optout
Open

seradin wants to merge 2 commits into
NousResearch:mainfrom
seradin:seradin/honcho-observation-optout

Conversation

@seradin

@seradin seradin commented Aug 13, 2026

Copy link
Copy Markdown

Summary

  • add provider-local observationOptOutPhrases configuration
  • preserve opted-out messages in session history/search while sending them with Honcho configuration.reasoning.enabled=false
  • fail closed if the SDK cannot construct the per-message opt-out configuration
  • preserve existing behavior for ordinary turns and leave saveMessages semantics unchanged

Why

A user can explicitly mark a fact as conversation-only, yet Honcho's peer representation can currently absorb it and recall it in a new conversation. Session routing remains correct; the leak occurs in cross-session representation derivation. This adds a narrow, operator-configured boundary without disabling useful observation globally.

Verification

  • python -m pytest tests/honcho_plugin/ -q — 316 passed
  • ruff check plugins/memory/honcho tests/honcho_plugin — clean
  • git diff origin/main...HEAD --check — clean

The default phrase list is empty, so this is behavior-neutral until explicitly configured.

@seradin

seradin commented Aug 13, 2026

Copy link
Copy Markdown
Author

Maintainer review requested. This is opt-in (default empty) and fixes a live-reproduced cross-session memory bleed: messages explicitly marked “this conversation only” were promoted into Honcho peer representation and recalled in a new session.

Validated locally: 316 passed in tests/honcho_plugin, ruff clean, normal wire shape unchanged, matched user+assistant messages persist with reasoning.enabled=false, and SDK-unavailable construction fails closed without advancing the sync cursor. An AOP deployment has already staged the inert config; the behavior activates only after this core change lands.

@seradin

seradin commented Aug 13, 2026

Copy link
Copy Markdown
Author

Correction to my validation note: a late adversarial review exercised the real pinned honcho-ai==2.2.0 Peer.message() path and found a blocking dict-vs-model mismatch that the MagicMock tests missed. The current commit passes a MessageConfiguration model where the SDK requires a plain dict and would leave opt-out batches unsynced. The PR is still unmerged, so there is no deployed runtime impact. I am updating the branch with a real-SDK regression test; please do not merge the current SHA.

@seradin
seradin force-pushed the seradin/honcho-observation-optout branch from ea5b337 to ee9e42b Compare August 13, 2026 05:22
@seradin

seradin commented Aug 13, 2026

Copy link
Copy Markdown
Author

Updated head ee9e42b9e fixes the blocking SDK mismatch. _message_configuration_for now returns the plain dict required by pinned honcho-ai==2.2.0; a new regression test calls the real installed honcho.Peer.message() offline and verifies MessageCreateParams.configuration.reasoning.enabled is False. Full tests/honcho_plugin: 317 passed; ruff clean. The previous broken SHA was never merged or deployed.

@seradin
seradin force-pushed the seradin/honcho-observation-optout branch from ee9e42b to a265d0d Compare August 13, 2026 06:41
@pulsead-kyle

Copy link
Copy Markdown

Rebased onto latest origin/main.

  • Old base: 08606fc23 → New base: 89a84e1ae (feat: profiles.list/profiles.create ws RPC + plugin session-navigation doors, feat: profiles.list/profiles.create ws RPC + plugin session-navigation doors #85093)
  • Old head: ee9e42b9e → New head: a265d0dd8
  • Checked upstream main for overlapping honcho opt-out work: no commits touched plugins/memory/honcho or tests/honcho_plugin between old base and new base — no duplicate implementation found, single commit rebased cleanly with zero conflicts.
  • tests/honcho_plugin: 317 passed, 1 unrelated deprecation warning (discord/audioop).
  • ruff check plugins/memory/honcho tests/honcho_plugin: all checks passed.
  • Real honcho-ai SDK smoke test (installed honcho-ai==2.2.0, not a mock): confirmed Peer.message(content, configuration={'reasoning': {'enabled': False}}) builds MessageCreateParams(configuration=MessageConfiguration(reasoning=ReasoningConfiguration(enabled=False))) as expected, and Peer.message(content) with no configuration still returns configuration=None — matches this PR's _message_configuration_for contract exactly, no dict/SDK-type regression.
  • Force-pushed with --force-with-lease after confirming remote head matched the expected prior SHA ee9e42b9e.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

feat(honcho): add per-turn observation opt-out phrases

  1. plugins/memory/honcho/__init__.py_matches_observation_opt_out matches against the raw user_content, while the chunks that get stored use sanitize_context(...). If sanitization ever rewrites a phrase boundary (or the phrase itself), the flag and the stored content disagree. Matching against the same sanitized string used for storage would give a single source of truth.

  2. plugins/memory/honcho/client.py_coerce_string_list silently drops non-list values, so a config typo (e.g. observationOptOutPhrases: "off the record" — a string instead of an array) silently disables the feature with zero signal. Fail-closed is right, but a logger.warning when an explicit-but-invalid value is discarded would save operators a confusing debugging session.

  3. plugins/memory/honcho/session.py — the fail-closed contract for a rejected dict shape is good (batch stays unsynced, retried), but the only signal is a debug log on every failed flush. A repeated-failure warning (after N consecutive failures) would surface a silently stuck opt-out queue in production.

  4. The no_observe flag is derived from the user message alone and applied to both user and assistant chunks — documented and intentional, but worth confirming the asymmetry is acceptable: a user phrase marks the assistant's reply unobserved too, while a sensitive assistant reply without a user phrase stays observed.

The casefold-vs-lower decision and the real-SDK regression test (dict, not MessageConfiguration instance) are both well handled.

@alt-glitch alt-glitch added type/feature New feature or request comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 16, 2026
@seradin
seradin force-pushed the seradin/honcho-observation-optout branch from a265d0d to f661493 Compare August 16, 2026 14:52
@seradin

seradin commented Aug 16, 2026

Copy link
Copy Markdown
Author

Rebased onto latest origin/main and addressed the automated review.

Rebase (a265d0dd8f661493cb, new base 7095e23eb)

Not a trivial replay — main had since changed the same sync_turn body in two ways, and taking my side wholesale would have reverted both:

  1. An empty-content guard (if clean_user_content: / if clean_assistant_content:) so interrupted or tool-only turns never write empty-string messages.
  2. A switch from _flush_session(session) to save(session), fixing a bug where the direct flush bypassed writeFrequency batching and flushed every turn regardless of config.

Both are preserved; no_observe is now threaded through the guarded loops and the turn still routes through save(). A new test pins this so the batching fix cannot be silently undone again.

Review point 1 — match the sanitized string, not the raw input

Agreed, and the risk runs in the direction you flagged plus one more. sanitize_context strips injected context blocks and system notes, so raw matching could trip the opt-out on a phrase the user never wrote that merely appeared inside an injected <memory-context> block — hiding a turn nobody asked to hide — while the stored payload no longer contains that phrase at all. _matches_observation_opt_out now reads clean_user_content, making one string the source of truth for both the decision and the payload.

Review point 2 — silent disable on a config typo

Fixed. _coerce_string_list now logs a warning when an explicit non-list value is discarded (observationOptOutPhrases: "off the record" as a bare string), naming the received type and pointing at the array form. Fail-closed behavior is unchanged; it is just no longer silent.

Review points 3 and 4

3 (repeated-failure warning on a stuck flush queue) is a real gap, but it belongs to the general flush path rather than this feature, so I would rather not widen this PR's surface for it. Happy to follow up separately.

4 — confirming the asymmetry is intentional: the user is the one who asks for a turn to be off the record, and the assistant reply is part of that same turn, so both chunks are flagged. The inverse case (a sensitive assistant reply with no user phrase) is a different feature and would need its own signal.

Validation

  • tests/honcho_plugin: 358 passed on the new base (4 new tests).
  • ruff check plugins/memory/honcho tests/honcho_plugin: clean.
  • Mutation-checked both new behavioral tests rather than trusting green: reverting to raw matching fails test_opt_out_matches_sanitized_text_not_injected_context, and restoring the direct _flush_session call fails test_opt_out_turn_routes_through_save_so_write_frequency_is_honored.
  • Force-pushed with --force-with-lease after confirming the remote head still matched a265d0dd8.

Formatting note: ruff format --check reports the three touched files as unformatted, but they are equally unformatted on main (20 files repo-wide) and no workflow enforces ruff format, so I left that alone rather than bury this diff in unrelated reformatting.

@seradin
seradin force-pushed the seradin/honcho-observation-optout branch from f661493 to 7003fac Compare August 23, 2026 08:44
@seradin

seradin commented Aug 23, 2026

Copy link
Copy Markdown
Author

Review follow-up pushed after rebasing onto current origin/main.

Review-point status

  1. Match the stored/sanitized user text — implemented and covered in both directions: injected context containing an opt-out phrase does not trigger, while a phrase actually written by the user still does.
  2. Invalid config should not fail silently — implementation already emitted an actionable warning for explicit non-list values. Commit 7003facca6 now pins that contract with caplog regressions:
    • root-level bare string: exactly one warning, including expected type, actual str, and ignored/off guidance;
    • host-level invalid string: same warning and fail-closed [], with no fallback to root;
    • valid list and explicit null: no invalid-type warning.
  3. Repeated flush failure visibility — no change added here. On re-review, the generic path already logs every _flush_session failure at ERROR; async writes additionally log WARNING before the retry and ERROR after a failed retry. This PR does not alter that lifecycle, so an N-consecutive-failure counter would be a separate rate-limited observability enhancement rather than a correctness prerequisite for this opt-out feature.
  4. Apply the user's opt-out to both sides of the turn — retained intentionally and covered: user and assistant chunks receive the same no_observe flag for the opted-out turn.

Verification after rebase

  • Opt-out/config/real-SDK focused tests: 28 passed
  • Full tests/honcho_plugin: 358 passed
  • ruff check: passed
  • compileall: passed
  • git diff --check: passed

PR HEAD: 7003facca61b3e20306cfa06f59848541e7d6d5f. The feature patch is range-diff identical across the rebase; the only additional patch is the test-only warning regression.

@alt-glitch alt-glitch added area/memory Memory subsystem: store, providers, sync, background reviews sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 23, 2026
@seradin

seradin commented Sep 3, 2026

Copy link
Copy Markdown
Author

Maintainer ping: this PR is mergeable and locally verified, but all GitHub Actions runs are still waiting for repository approval (action_required; 0 jobs started).

Could a maintainer please approve and run the workflows when convenient?

This addresses a cross-session privacy/correctness gap: an operator-configured conversation-only phrase can remain searchable in session history while being excluded from Honcho representation reasoning. The default remains behavior-neutral.

pulsead-kyle and others added 2 commits September 16, 2026 11:46
Per-turn Honcho observation opt-out via config.observationOptOutPhrases
(Unicode-casefold substring match on user content, so non-ASCII case
folds like German STRASSE/straße match correctly while Korean and other
scripts without case keep working as before). Matched turns are still
written/searchable but flagged no_observe=True, reusing existing
HonchoSession.add_message/_message_configuration_for wire mechanism.
Provider-local only, no core agent/ changes.

Fail-closed on the opt-out contract: if a no_observe message's
MessageConfiguration/ReasoningConfiguration cannot be imported/
constructed, _message_configuration_for now raises instead of quietly
returning None. _flush_session's outer try/except catches this, sends
no batch, returns False, and leaves the message unsynced for retry —
never dropped, never sent as fully observed.

observationOptOutPhrases host precedence fails closed: host presence
(even an explicit non-list, null, or empty list) overrides root entirely
rather than falling back to root phrases on an invalid host value, so a
malformed host block can never unexpectedly activate root opt-out phrases.

Tests: 316 passed in tests/honcho_plugin/ (added regression coverage
for the fail-closed path, Unicode casefold matching, and host-vs-root
opt-out phrase precedence).
@seradin
seradin force-pushed the seradin/honcho-observation-optout branch from 7003fac to c91ce1e Compare September 16, 2026 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants