fix(analyze): ACP-only path, tape, dedupe ChatModel callbacks#31
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55bb3dd419
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sc := bufio.NewScanner(f) | ||
| for sc.Scan() { |
There was a problem hiding this comment.
scanMaxID uses bufio.Scanner with the default 64 KiB token size, so reopening .tape.jsonl will fail with bufio.Scanner: token too long once any single tape line exceeds that size. This is likely with large tool outputs, and after that OpenJSONL fails and workspace analyze cannot start for that workspace until the tape is manually edited.
Useful? React with 👍 / 👎.
| if msg != nil { | ||
| h.write(Message(messageToMap(msg), h.baseMeta())) | ||
| } | ||
| runData := map[string]any{"status": "ok"} |
There was a problem hiding this comment.
In OnEndWithStreamOutput, stream read errors only log and break the receive loop, but the handler still emits a run event with "status": "ok". When ACP streaming fails mid-response (transport/process errors), tape records the run as successful, which makes debugging and run auditing inaccurate.
Useful? React with 👍 / 👎.
- Drop eino filesystem middleware so ToolsNode never runs; ACP executes tools - acpToolCallingModel reports IsCallbacksEnabled to skip callbackInjectedModel - Wire tape JSONL + SlogEinoHandler on analyze; remove local backend adapter - go mod tidy (drop unused eino-ext/local backend) Made-with: Cursor
…abysit) - scanMaxID: increase bufio.Scanner buffer to 10MB to handle large tool outputs that exceed the default 64KB token limit - OnEndWithStreamOutput: track stream read errors and emit run event with "error" status instead of always reporting "ok"
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae3d198e64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return "", errors.Errorf("create filesystem middleware: %w", err) | ||
| return "", errors.Errorf("open tape store: %w", err) | ||
| } | ||
| defer func() { _ = tapeStore.Close() }() |
There was a problem hiding this comment.
When ACP emits streamed callbacks, pkg/tape/eino_handler.go drains and writes the stream from a goroutine, but this deferred close runs as soon as the runner iterator finishes. There is no synchronization with those goroutines, so the final assistant message/run event can race with Close() and be dropped with an append error, leaving .tape.jsonl incomplete for normal streamed model responses.
Useful? React with 👍 / 👎.
1 participant
Made-with: Cursor