Skip to content

fix(messages): keep UI-only notices out of the model payload - #899

Open
addyCooks wants to merge 1 commit into
Nano-Collective:mainfrom
addyCooks:fix/display-only-notices
Open

fix(messages): keep UI-only notices out of the model payload#899
addyCooks wants to merge 1 commit into
Nano-Collective:mainfrom
addyCooks:fix/display-only-notices

Conversation

@addyCooks

Copy link
Copy Markdown
Contributor

Description

Cancellation notices, inline error banners, the non-interactive tool-approval notice, and the
VS Code replies to built-in slash commands (/help, /copy, /model, unrecognized commands)
were pushed into conversation history as assistant messages. They sit in the same Message[]
that becomes the provider payload, so the next request handed the model harness-authored
markdown as its own past output and because sessions persist Message[] verbatim, a resumed
session replayed it into context too. The built-in command replies never involved the model at
all, so both halves of that exchange are marked, not just the reply.

Mark those notices displayOnly and filter them in convertToModelMessages, the single point
where history becomes the provider payload. They still render in chat and still replay with
session history.

Deliberately left alone:

  • truncateToolResult's elision marker (... [Output truncated: N characters total …] ...) harness-authored, but it lives inside a tool result, the correct channel for telling the
    model its output was elided. Removing it would make truncation silent.
  • Harness user messages (auto-nudges, auto-diagnostics, [Active file: …]) and synthetic
    tool results (cancellation placeholders, unknown-tool errors). These are the legitimate way
    a harness addresses a model, not text passed off as the model's own output.
  • ACP's cleanedContent and prepareStep were audited and are already purely subtractive they strip tool-call syntax and empty messages, never add text.

Closes #893.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging

Assignment: #893 currently has no assignee — worth asking a maintainer to assign it before
merge.

Logging: no new logging. The change adds no new failure path; the three notice sites keep
their existing logger.error calls, and the converter filter cannot fail.

Breaking changes: none. displayOnly is an optional field on Message; every existing
message and every persisted session omits it and behaves exactly as before.

Cancellation notices, inline error banners, the non-interactive
tool-approval notice, and the VS Code replies to built-in slash commands
(/help, /copy, /model, unrecognized commands) were pushed into
conversation history as assistant messages. They sit in the same
Message[] that becomes the provider payload, so the next request handed
the model harness-authored markdown as its own past output — and because
sessions persist Message[] verbatim, a resumed session replayed it into
context too. The built-in command replies never involved the model at
all, so both halves of that exchange are marked, not just the reply.

Mark those notices displayOnly and filter them in convertToModelMessages,
the single point where history becomes the provider payload. They still
render in chat and still replay with session history.

Closes Nano-Collective#893.
@addyCooks

Copy link
Copy Markdown
Contributor Author

@will-lamerton
Heads up!
main's been red since yesterday, unrelated to any one PR's contents.

chat-panel.js reads globalThis.NanocoderMentionUtils, which media/mention-utils.js defines. The webview loads both, but chat-panel-harness.ts only ran chat-panel.js, so createPanel() throws on the destructure and takes out all 28 chat-panel tests. The harness didn't get updated when the mention helpers were split into their own script in #842.

One-line fix in the harness PR is up: #900.
Suggest merging that first, since anything branched off current main shows the same 28 failures.

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.

[Bug] OpenAI Shim Injects Synthetic Protocol Text

1 participant