fix: ACP bridge finish-text dedup gaps + agents table column exhaustion - #372
Merged
Conversation
…r prompt migrations no-ops
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- acp-bridge finish-text dedup:
_AssistantTextRelay.already_emittednow matches finish text as a trailing suffix instead of requiring full equality, soFinishAction/FinishObservationtext is blanked correctly whenACPAgentretries a turn in place after a transient connection error. The blanking now also covers theObservationEventhalf of the finish pair, not just theActionEventhalf. - Agents table column-limit fix:
000010_add_trigger_prompts_to_agents.sqland000011_add_description_write_trigger.sqlare turned into no-ops by removing theirADD COLUMNstatements. BecauseRunMigrationsFSre-runs every migration on every boot and000019_drop_agent_trigger_prompts.sqlimmediately drops the same columns, Postgres's non-reclaimable dropped-column slots were leaking four columns per boot until the 1600-column ceiling was hit. - Tests and docs: Added coverage for observation blanking, trailing-duplicate blanking after a retry, and negative cases; README updated to describe the retry behavior.
One non-blocking note: the PR title and body only describe the migration fix, but the runner.py changes are a second, independent behavioral fix. Worth calling out in the merge commit or a quick update to the PR description so the changelog captures both.
Kimi K2 (free via Pullfrog for OSS) | 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Two unrelated fixes, bundled here because both surfaced while following up on the #370 review:
apps/acp-bridge— the assistant-text dedup added in feat(acp-bridge): stream assistant text in position instead of one end-of-turn block #370 was incomplete and not retry-safe.services/api/migrations— a live dev database hit Postgres's hard 1600-column-per-table limit on theagentstable, caused by a migration pattern that silently leaked a "ghost" column on every single application boot.1. ACP bridge: fix incomplete finish-text dedup
#370 streams assistant narration as it arrives and blanks the turn's closing
FinishAction.messagewhen it's an exact duplicate of what already streamed, to avoid showing/persisting the same text twice. Two gaps in that logic:a) Only half the duplicate was blanked. The SDK's
_finalize_successful_turnalways closes a turn with two events built from the same joined text: theActionEvent(FinishAction.message) and, immediately after, anObservationEvent(FinishObservation, same text viacontent). The original fix only rewrote theActionEvent.services/ai-agentpersists every event unconditionally, so theObservationEventkept storing the full duplicate text server-side — nothing rendered it (the frontend already skipsObservationEventfortool_name == "finish"), but the "avoid persisting the same text twice" goal wasn't actually met._event_payloadnow blanks both via a shared_blank_duplicate_finish_texthelper.b) The dedup check didn't survive an ACP-level retry.
ACPAgent.step()can retry a turn in place on a transient connection error. Its_reset_client_for_turn()clears the SDK's own accumulated text for the new attempt but re-wires the sameon_tokencallback — so this bridge's relay buffer (_AssistantTextRelay._emitted) keeps growing across every attempt while the SDK's eventualFinishActionmessage reflects only the attempt that succeeded. The old exact-equality check would then fail to match, showing the surviving attempt's text a second time on top of whatever had already streamed from the failed attempt.already_emitted()now does a trailing-suffix match instead of full equality — provably correct here since retries run strictly sequentially, never interleaved, so the successful attempt's text is always the tail of everything the relay has seen.Also corrected a code comment that cited a
_raise_masking_errorSDK function — it doesn't exist anywhere in the pinnedopenhands-sdk; the SDK's actual masking-failure behavior is the opposite (fails open, not closed).Verification: 4 new tests (paired
ObservationEventblanking, observation left alone on mismatch, retry-suffix dedup, guard against false-positive substring matches). Full suite: 38 passed.ruff check/ruff format --checkclean.2. Migrations: stop leaking ghost columns on
agentsservices/api/internal/platform/database.RunMigrationsFSreplays every.sqlfile inservices/api/migrations/on every application boot — there's noschema_migrationstracking table, so idempotency is load-bearing for every file.000010_add_trigger_prompts_to_agents.sql/000011_add_description_write_trigger.sql(ALTER TABLE agents ADD COLUMN IF NOT EXISTS ...) and000019_drop_agent_trigger_prompts.sql(ALTER TABLE agents DROP COLUMN IF EXISTS ..., unconditional) both ran on every boot, and 000019 always undid 000010/000011's adds in the same run. Postgres never reclaims a dropped column's slot inpg_attribute—DROP COLUMNonly marks itattisdropped; theattnumstays consumed until the table is physically rebuilt. So every boot created 4 fresh columns only to have them immediately ghosted again, permanently burning 4 ofagents's hard 1600-column ceiling per boot:Confirmed directly against the affected dev database:
agentswas at 1599/1600 attribute slots, of which only 25 were live columns — 1574 were dropped ghosts. No other table in the database showed this pattern (checked all of them);agentswas the only one where an add-migration and a drop-migration for the same columns both replay on every boot.Fix:
000010/000011no longer run theirADD COLUMNstatements — since000019always undid them in the same boot anyway, the final schema is unchanged; this just stops the pointless churn.000011's still-neededCHECKconstraint extension is untouched, and000019's drops are left in place (now a permanent no-op, but still correct for a database restored from a backup taken before this fix).Verification: replayed the entire 32-file migration sequence against the actual affected
paca-dev-postgres-1container, in order — completes with zero errors, and the ghost-column count onagentsdid not grow.Known follow-up, not done here:
agentsis left at 1599/1600 slots (only 25 live) — exactly one more real column can ever be added to it before this recurs. Reclaiming the 1574 wasted slots requires physically rebuilding the table (new table, copied data, every FK that referencesagents.idre-pointed) — out of scope for this fix, which only stops the leak from getting worse.Type of Change
Checklist
apps/acp-bridge/README.md's "Assistant text arrives in position" section now describes the trailing-duplicate/retry behavior.