Skip to content

perf(server): drop transient native events from provider logs - #240

Open
leoisadev1 wants to merge 1 commit into
mainfrom
perf/native-event-log-filter
Open

perf(server): drop transient native events from provider logs#240
leoisadev1 wants to merge 1 commit into
mainfrom
perf/native-event-log-filter

Conversation

@leoisadev1

Copy link
Copy Markdown
Member

What Changed

Provider event logs no longer persist streaming chunks that cannot change thread state: Codex item/realtime deltas, Claude content-block deltas, ACP agent_message_chunk/agent_thought_chunk, and OpenCode text/reasoning part updates. Writes push into the pending buffer instead of copying it. ACP request diagnostics stay on; full protocol logging is opt-in and applies the same chunk filter.

Why

Native streams currently write every delta into thread-scoped ndjson files. Canonical events already drop content.delta and similar types. Native events did not, so the files grew with records nobody reads. Remaining pingdotgg#8187 logging work, kept off #230 (narrow projection queries) and off #203 (async log drains).

EventNdjsonLogger.ts is shared with OPEN #203. This PR only changes persist filters and pending-buffer mutation. It does not rewrite drain/close.

Out of scope here: OpenCode adapter text merge, native telemetry sampling, GitVcsDriver Trace2 decode, RepositoryIdentityResolver root cache (already local).

UI Changes

None.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I linked the accepted plugin or provider proposal in Why, or this PR does not add one
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Verification

  • vp test run apps/server/src/provider/Layers/EventNdjsonLogger.test.ts apps/server/src/provider/acp/AcpNativeLogging.test.ts — 22 passed
  • vp lint on the five changed files — clean
  • vp run --filter akeru-bot typecheck — suggestions only, no errors

Grok 4.6 High via Grok Build.

Streaming provider chunks were filling thread-scoped ndjson files even though they cannot change thread state. Filter those native events before serialization, push into the pending buffer instead of copying it, and keep ACP protocol logging opt-in with the same chunk filter.

Async drain/close for EventNdjsonLogger stays on OPEN #203.

Grok 4.6 High via Grok Build.
@vercel

vercel Bot commented Sep 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
akeru-bot-landing Building Building Preview Sep 10, 2026 6:14pm UTC

Request Review

@github-actions github-actions Bot added type:provider Agent provider contribution. size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Sep 10, 2026
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

Greptile Summary

This change reduces persisted provider log volume and makes ACP protocol diagnostics conditional. One non-blocking operational gap remains: Grok and Cursor do not expose or forward the setting required to enable full ACP protocol logging, so operators cannot turn on the documented diagnostics in production.

Confidence Score: 4/5

Safe to merge from a correctness and security perspective, but production ACP protocol diagnostics remain unavailable to operators until configuration is wired through the Grok and Cursor adapters.

The executed check confirmed one limited operational configuration gap. No runtime failure, data-integrity issue, or security issue was established.

Files Needing Attention: apps/server/src/provider/acp/AcpNativeLogging.ts; apps/server/src/provider/Layers/GrokAdapter.ts; apps/server/src/provider/Layers/CursorAdapter.ts; packages/contracts/src/settings.ts

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced proofs for two posted P2 findings and linked them to the corresponding review comments.
  • General contract validation completed; status is confirmed, and execution evidence shows the test run of AcpNativeLogging.test.ts exited 0 with 7 passing tests and the verbose protocol reachability script exited 0 and reported CONFIRMED UNREACHABLE, with the authored source and observed output uploaded for review.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P2 Grok and Cursor cannot enable documented ACP full protocol logging

    • Bug
      • AcpNativeLogging only creates protocolLogging when verboseProtocolLogging is true, but the Grok and Cursor production adapters call the logger factory without that property. Neither provider's production settings schema nor settings patch accepts the property, so an operator cannot configure the required true value.
    • Cause
      • The logger factory has an unplumbed optional verboseProtocolLogging input: it is exercised only by direct unit-test construction, not passed through the production adapter/driver configuration pipeline.
    • Fix
      • Add verboseProtocolLogging as an explicit Grok and Cursor provider configuration setting (including update/patch schemas as appropriate), then pass it from each production adapter's effective settings into makeAcpNativeLoggers. Add integration coverage that enables the setting and observes protocolLogging supplied to the ACP runtime.

    T-Rex Ran code and verified through T-Rex

Fix all with Greploop Fix All in Claude Code

Reviews (1): Last reviewed commit: "perf(server): drop transient native even..." | Re-trigger Greptile

payload: formatRequestLogPayload(event),
}),
...(input.nativeEventLogger
...(input.nativeEventLogger && input.verboseProtocolLogging

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Protocol opt-in is unreachable

Full ACP protocol logging now requires verboseProtocolLogging, but the Grok and Cursor production adapters do not pass that option and their provider settings do not expose it. Operators therefore cannot enable the documented diagnostics outside direct test construction. Wire the setting through provider configuration and adapter construction. This is a non-blocking operational concern, but it leaves incident diagnostics unavailable when they are needed.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Artifacts

Evidence from the check

  • Authored TypeScript check reads the production ACP logging, Grok/Cursor adapter and driver, and settings schema sources and asserts that the required option is gated but never supplied or configurable; the takeaway is that the check directly covers the claimed integration path.

Command output from the check

  • Executed `pnpm exec vp test run apps/server/src/provider/acp/AcpNativeLogging.test.ts` from the repository root and captured 7 passing tests; the takeaway is that unit tests validate explicit test-only opt-in but do not establish production plumbing.

Command output from the check

  • Executed the authored reachability check from the repository root and captured its zero-exit assertion results; the takeaway is that Grok and Cursor production construction and configuration cannot set the gate's required option.

View artifacts

T-Rex Ran code and verified through T-Rex

Fix in Claude Code

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

Labels

size:L type:provider Agent provider contribution. vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant