Skip to content

fix(compaction): only treat a single-line result as a success marker - #1645

Open
qwist1233-cpu wants to merge 2 commits into
Nano-Collective:mainfrom
qwist1233-cpu:fix/1633-anchor-success-patterns
Open

qwist1233-cpu wants to merge 2 commits into
Nano-Collective:mainfrom
qwist1233-cpu:fix/1633-anchor-success-patterns

Conversation

@qwist1233-cpu

Copy link
Copy Markdown

What

compressToolResultDefault collapsed a tool result into Tool: X\nResult: success whenever the body matched /completed successfully/i or /no errors/i anywhere, while success/ok/done were anchored. A log such as

Build completed successfully
failures: 3

lost its failures: 3 line after /compact.

How

isSuccess now trims the content, returns false for anything multi-line, and anchors the two phrase patterns to the whole text (/^.{0,80}completed successfully\.?$/i, /^no errors\.?$/i). A single-line Build completed successfully is still a success marker; a multi-line log falls through to the existing key-info / first-line path.

Verified

  • npx ava source/utils/message-compression.spec.ts: 31 passed (two new tests: multi-line log with failures is not collapsed; single-line "completed successfully" still is).
  • Negative check: with the source change stashed and the new tests kept, compressMessages keeps a log that mentions success but also reports failures fails, so the test exercises the bug.
  • biome check on both files clean, tsc --noEmit clean.
  • Changeset added (patch).

Closes #1633

compressToolResultDefault replaced a tool result with 'Result: success'
when it matched /completed successfully/ or /no errors/ anywhere, so a
multi-line log that also reported failures lost those lines. Anchor the
patterns to the whole (trimmed) content and bail out on multi-line text.

Closes Nano-Collective#1633
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 nit

@qwist1233-cpu — a few things worth a look, none blocking.

The PR anchors the success patterns in isSuccess to a trimmed, single-line input and trims multi-line content out, so a log like Build completed successfully\nfailures: 3 is no longer collapsed into Result: success. The fix correctly resolves issue #1633 and is covered by two regression tests, a meaningful changeset, and unchanged callers. The one finding is a deliberate narrowing beyond the bug: the phrase is now anchored to the end of a single-line input, so a single-line result like "Operation completed successfully in 5s" (phrase in the middle, trailing details) also falls through to the key-info path rather than collapsing to Result: success.

⚪ nit · design · source/utils/message-compression.ts:381

The new pattern /^.{0,80}completed successfully\.?$/i anchors the phrase to the end of the trimmed single-line content. The reported bug only required rejecting multi-line input; this also makes single-line content like "Operation completed successfully in 5s" fall through to the key-info path instead of collapsing to Result: success. That is arguably more correct (the body is not exclusively the success marker), and the PR description acknowledges the change explicitly, so it is reasonable — flagging only so a reviewer notices the scope is slightly wider than "multiline is out".


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Oct 8, 2026
A single-line result whose success phrase is not at the end, such as
"Operation completed successfully in 5s", now takes the key-info path.
It is not dropped: the whole line is kept as the result, which is more
than "Result: success" carried.

Signed-off-by: qwist1233-cpu <qwist1233@gmail.com>
@qwist1233-cpu

Copy link
Copy Markdown
Author

On the nit: the wider scope is deliberate, and I checked that nothing is lost by it. "Operation completed successfully in 5s" now compresses to

Tool: execute_bash
Result: Operation completed successfully in 5s

so the key-info path keeps the whole line rather than replacing it with Result: success — strictly more information for one extra token or two. Pushed a test that pins exactly that, so the behaviour is documented instead of incidental.

I kept the end anchor rather than allowing trailing text, because "Build completed successfully, but 3 tests failed" is a single line too, and matching the phrase mid-line would collapse it to Result: success — the same class of bug as #1633.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Default compaction treats any mention of success as the whole tool result

1 participant