fix(provider-acp): declare manual compaction support for omp - #2293
Conversation
|
Live production evidence that the bridge compaction path works end-to-end for acp-omp — gathered today (2026-08-22) on a real thread, independent of this PR's tests. A local auto-compaction plugin (context-compact) queues Sequence numbers on the thread's event log: 19648 (prior turn/completed), 19649–19652 (the Two conclusions:
So the declaration flip in this PR doesn't enable an untested path — it un-gates the direct RPC onto a path production already exercises successfully.
|
Rework addendum (reply to review of #2293)Finding 1 (blocking — obsolete server.ts hunk): Addressed by the rebase. The branch now sits on current main ( Finding 2 (major — false
Detection is scoped narrowly to the compaction turn and never inspects ordinary agent traffic; the message strings are matched verbatim from the report. Covered by three new tests (two bridge-level against the fake ACP agent — which gained a
Finding 3 (minor — bridge behavior untested): The two bridge-level tests close this gap. Finding 4 (minor — unrelated reformatting): Dropped during the rebase, but the repo-pinned oxfmt 0.64.0 re-introduces both hunks on main's current copy of the file (main's own blob fails Finding 5/6 (ok): No wire/daemon/CLI changes; the Gates:
|
f8620a8 to
64dce8d
Compare
Rebase + SDK-version fix (CI failure root-caused)The last push failed CI with Done, plus a refresh onto current
Gates:
@SawyerHood — re-review when you get a chance; the two rework commits are unchanged in content, only rebased, plus the mechanical version bump. |
Hardened: compaction classification no longer pins exact proseFollow-up to the review thread: the no-op detection matched omp's failure strings verbatim, so an upstream reword of the
Two new bridge tests cover the drift cases (reworded no-op prose → skip; failure sentence preceded by other streamed text → failed), each mutation-verified: reverting no-op matching alone fails the first, reverting to the old prefix-only check fails the second with a false Gates: bridge 293/293 (17 files), typecheck, lint, oxfmt, |
|
Refreshed onto current main — main took Gates: bridge 304/304 (17 files — includes the 11 tests main added), typecheck, lint, oxfmt, |
|
Refreshed onto current main (SDK 0.4.27 → branch claims 0.4.28). This merge also adopts main's two newer conventions where they collided with this branch's regions: the comment-free style from #2624 (the classifier's rationale lives in the PR description) and main's independently-landed |
960e37a to
9a026a4
Compare
CI status on head
|
| Run | Failing test | Budget |
|---|---|---|
| 33185878477 | bb-plugin-provider-pi catalog.test.ts › "re-reads model scope after the catalog child restarts" |
5000ms |
| 33186437294 | @bb/host-workspace › "does not overflow the call stack merging a large subdirectory" (hook) |
10000ms |
| 33187231762 | pi catalog.test.ts (same test) |
5000ms |
The diff is exonerated
Running the exact CI shard at full turbo parallelism on a pristine origin/main checkout reproduces the identical failure set:
pnpm exec turbo run test --continue \
--filter='!@bb/server' --filter='!@bb/app' --filter='!@bb/integration-tests' --force
→ @bb/host-workspace "Hook timed out in 10000ms" (2), bb-plugin-provider-pi (catalog child restart), bb-plugin-tasks, @bb/plugin-build — same set as on this branch. Every one of these tests passes when its package runs alone (verified locally: bridge 305/305, provider-acp 76/76, the pi catalog test in isolation ×2, server 2072/2073 with the known env failure). Main's and #2626's green packages-job logs contain zero discovery-timeout lines; this branch's slightly longer bridge suite appears to be what tips the marginal scheduling over on the CI runner.
Filed separately with the deterministic main-branch repro and fix options: #2653.
Ask
Fork PRs can't re-run their own checks (Must have admin rights to Repository). If a maintainer re-runs the packages job — or #2653 lands a budget/concurrency fix — this should go green. All local gates on this head pass (typecheck, lint, oxfmt, version guard, full suites above).
|
cc @SawyerHood — evidence above that the four red |
|
Refreshed against main: absorbed the #2701 SDK bump via merge + re-bump to |
The provider-acp bridge already serves manual compaction by sending the agent's own /compact command as a session/prompt maintenance turn, but the acp-omp declaration kept the tier default supportsManualCompaction: false, so compactThreadContext answered 409 before the bridge was ever consulted. Override the capability per agent, matching acp-opencode. The public compaction suite gains the acp-omp dispatch case (200 with the standalone builtin /compact turn submitted for the acp provider), and the first-party capability table tracks the flip.
omp resolves every consumed builtin slash command with end_turn, including a /compact that printed "Compaction failed: ..." as an ordinary agent message, so an end_turn compaction prompt is only a shrunk context when the agent did not spend the turn reporting a failure (get-bb#2290 review). While the compaction maintenance prompt is in flight the bridge now keeps the streamed agent message text. On end_turn it classifies the completed text: the two known no-op messages ("Nothing to compact (session too small)", "Already compacted" — the same strings pi prints, get-bb#1721) settle the compaction as skipped, which the translator reports as a compaction-skipped warning plus a clean turn boundary with no thread/compacted; any other "Compaction failed:" text fails the turn with the agent's reason. Detection only runs for the compaction prompt, never for ordinary agent traffic.
The compaction-honesty change alters dist/provider-bridge-acp.js, so the published package content differs from 0.4.22; check-npm-version-guard requires a new version before CI passes.
…act prose omp's failure phrasing is TUI presentation code, not a contract (asked for a structured signal in can1357/oh-my-pi#9786). Match the failure opener and the no-op reasons case-insensitively anywhere in the streamed message: a reworded reason stays a skip, a failure sentence preceded by other text still fails the turn, and no reword can report a compacted context that never compacted.
03dde13 to
eece92f
Compare
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request now. I will check security, code quality, performance, architecture, and the main user path. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| agentMessage: string, | ||
| ): Record<string, unknown> { | ||
| const text = agentMessage.trim(); | ||
| if (!COMPACTION_FAILURE_PATTERN.test(text)) { |
There was a problem hiding this comment.
🚨 slopcop/review — The OMP text rule changes compaction results for every ACP agent.
compactionOutcomeForEndTurn also runs for OpenCode and custom ACP providers. Successful text that mentions "compaction failed" now produces a failed turn.
An unsupported failure phrase can still produce a false thread/compacted event. Put this rule behind an OMP dialect hook.
Keep the generic end_turn result unchanged. Add a regression test for generic ACP message text.
| parsed.data.update, | ||
| ); | ||
| if (chunk.success) { | ||
| session.compactionAgentMessage += |
There was a problem hiding this comment.
🚨 slopcop/review — The bridge keeps all compaction message text without a size limit.
Each message chunk makes this string larger. finishCompaction does not clear the string.
A verbose ACP agent can keep a large string for the session. The repeated copies and final scans also increase CPU use.
Use a fixed-size diagnostic buffer or a small classifier state. Clear the stored text after compaction.
Add a test for the size limit and the cleanup.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
This pull request lets users request manual context compaction for OMP. It also checks OMP messages before it reports compaction success.
I found two problems.
- Major: The OMP text rule runs for every ACP agent. OpenCode and custom ACP providers can now receive incorrect compaction results.
- Medium: The bridge keeps all compaction message text without a size limit. The text also remains after compaction completes.
The first issue needs an OMP dialect hook. Generic ACP agents must keep the existing end_turn behavior.
The second issue needs a size limit and cleanup in finishCompaction.
I found no authentication, permission, command, path, network, dependency, or secret problem.
The Pi provider has similar warning output. However, Pi receives a structured error field, so a shared text parser would not help.
Local checks passed:
- The ACP bridge passed 305 tests.
- The ACP provider plugin passed 81 tests.
- The two server files passed 7 tests.
- Type checks passed for the bridge, provider plugin, and server.
- All GitHub checks passed.
I could not run a real OMP browser test because this machine does not have the omp command. The server route and fake bridge tests passed.
I left this review as a comment. I did not approve the pull request or request changes.
## Human comments ## What was wrong The ACP bridge classified every successful manual-compaction response by looking for OMP-specific phrases such as `compaction failed`. That policy leaked across dialects, so a generic ACP or OpenCode response describing an earlier failed attempt could be reported as a failed compaction even when the agent's final result was successful. ## What changed - Move end-turn compaction classification into the ACP dialect contract. - Apply the existing failure/no-op prose handling only to OMP. - Let generic ACP and OpenCode keep the completed result reported by their successful end turn. - Add regressions for generic ACP and OpenCode while preserving OMP failed and skipped outcomes. - No wire contract or host-daemon protocol change is required. ## How you verified - `pnpm exec turbo run test typecheck --filter=@bb/provider-bridge-acp --force` (307 tests passed) - `pnpm exec turbo run test --filter=@bb/server --force -- "test/public/public-thread-compaction.test.ts"` (4 tests passed) - `pnpm exec turbo run build:types --filter=@get-bb/plugin-sdk --force` - `pnpm exec turbo run test --filter=@bb/plugin-api-map --force` (75 tests passed; public SDK inventory unchanged) - `pnpm exec oxfmt --check packages/provider-bridge-acp/src/bridge/bridge.ts packages/provider-bridge-acp/src/bridge/bridge.test.ts packages/provider-bridge-acp/src/dialect.ts` - `git diff --check origin/main...HEAD` Follow-up to #2293. > AGENT GENERATED
|
Follow-up for the two review findings on this merged commit: #3013. Status: the Major finding (compaction text rule applying to all ACP agents) was already fixed on main by #2957; #3013 closes the Medium finding — compaction prose accumulation is now gated to the omp dialect, clamped at 64 KiB head-preserved, and cleared in |
2 participants
What was wrong
The provider-acp bridge already serves manual compaction — bb's compact
affordance sends a standalone builtin
/compactmention, which the bridgeruns as the agent's own
/compactcommand viasession/promptand reportsthrough the
acp/compaction/*envelopes — but theacp-ompproviderdeclaration still carried the ACP tier default
supportsManualCompaction: false.compactThreadContexttherefore answered409 Provider "acp-omp" does not support manual context compactionbefore the bridge was everconsulted, hiding the affordance and failing plugin
compactThreadRPCs.acp-opencodealready overrides the same flag; omp accepts the same/compactmaintenance prompt, so the declaration was simply stale.What changed
plugins/provider-acp/server.ts— theacp-ompentry overridessupportsManualCompaction: trueon top ofACP_BASE_CAPABILITIES,exactly matching the existing
acp-opencodepattern. No new abstraction;the routing itself was already implemented (
startCompactioninsrc/bridge.ts). Cursor, Grok Build, and Hermes Agent stayfalse.apps/server/test/public/public-thread-compaction.test.ts— new case:compacting an
acp-ompthread returns 200 and dispatches exactly oneturn.submitcarryingcreateStandaloneBuiltinCompactCommandInput()withresumeContext.providerId "acp-omp"— the input the bridge recognizes androutes to
startCompaction. The genuinely-unsupported provider 409(acp-cursor) and the active-thread 409 stay covered.
apps/server/test/services/plugins/first-party-provider-plugins.test.ts—the acp-omp expectation-table entry tracks the flipped capability.
How you verified
compactThreadContext→providerRegistry.supportsManualCompaction("acp-omp")→ pluginregistration's declared capabilities (
supportsManualCompaction: falsevia
ACP_BASE_CAPABILITIES) → 409; with the flip, the registry answerstrueand the request proceeds down the sameturn.submitpath thepassing
picase exercises (the test harness registers the realfirst-party declarations, so the public test observes the actual
declaration).
pnpm exec turbo run test --filter=@bb/server— 1923 tests, 1922 passing; the single failure
(
test/app/install-machine-script.test.ts) reproduces identically on apristine
maincheckout atfff3ae8(environmental, pre-existing).Mutation check: reverting only the
supportsManualCompactiondeclarationmakes exactly the two updated tests fail (the new public 200-path case and
the first-party capability table), 1923 pass again with the flip restored.
pnpm exec turbo run typecheck lint --filter=@bb/server --filter=bb-plugin-provider-acpgreen;
pnpm exec oxfmt --checkgreen on all three touched files./compactrouting was already covered byplugins/provider-acp/src/bridge/bridge.test.ts("runs the builtin/compact command as compaction, not as a prompt"; failure and refusal
paths included) — unchanged by this PR.
Fixes #2290