Keep ACP tool results readable and local JSON frames intact - #277
Conversation
deepfates
left a comment
There was a problem hiding this comment.
Independent runtime/behavior review of 77d6e9b: no blocking findings.
Inspected capture omission, failure status, JSON rendering, UTF-8 bounds, listener/connecting socket framing, and the Dwell consumer. Independently ran mix test test/acp_local_test.exs test/acp_imp_acp_test.exs: 62 tests, 0 failures. Includes real sockets, permitted large bidirectional frames, fragmented UTF-8, consecutive frames, oversize refusal, and result-state distinctions. The configured frame cap remains enforced; socket buffers scale with that existing cap.
The ownership split is appropriate: Imp makes capture omission truthful and bounds ACP presentation; the retaining host supplies recovery coordinates. Dwell's operator recovery was separately exercised against this commit. This review does not establish Haven's display or historical card repair, and does not authorize release/deployment. CI was still running when inspected.
|
Validation at 77d6e9b:
Dwell candidate 1d34ce4 was exercised with |
Closes #276.
A large successful tool result became literal
nilin ACP because the observer capture limit omitted its output. Smaller structured results were Elixir inspect dumps. ACP now distinguishes omission from realnull/empty output, renders JSON and readable errors, and explicitly marks previews cut at 32,768 UTF-8 bytes. Call identity and completed/failed status remain intact; capture limits are unchanged.The real-socket test exposed a separate prerequisite: TCP line mode split permitted JSON frames at the default driver buffer (9,216 bytes locally). Align listener/client buffers with the existing configured frame bound, and check the actual frame size after removing its newline. This preserves the frame cap while allowing large valid frames through.
Validation: the new result-view regression failed original (nil/inspect), then passed; the large bidirectional socket regression timed out original, then passed. Relevant ACP/local/capture suites passed 75 tests before the two new socket tests; local suite now passes 7 tests including fragmented UTF-8, back-to-back lines and oversized refusal. Full
mix checkpassed: 59 doctests, 9 properties, 3,601 tests, 0 failures (13 skipped, 221 excluded). No providers, accounts, dependencies, releases or runtime settings changed.Dwell's complete native result → bounded preview and Haven's final text projection are separate owning changes (GroveResearch/dwell#195, GroveResearch/haven#101). Release Imp first, then update Dwell to that released version. This does not reconstruct historical omitted cards or add full-result retrieval UI.