Skip to content

feat: Handle Oracle cross-payload binds#11492

Open
rvql wants to merge 1 commit intomainfrom
oracle-separated-bind
Open

feat: Handle Oracle cross-payload binds#11492
rvql wants to merge 1 commit intomainfrom
oracle-separated-bind

Conversation

@rvql
Copy link
Contributor

@rvql rvql commented Mar 20, 2026

Cache Oracle request info across frames so custom SQL extraction can resolve bind values when SQL and binds arrive separately. Keep the local enterprise-utils stubs aligned with the parser changes.

This PR is for:

  • Agent

@rvql rvql requested a review from kylewanginchina March 20, 2026 11:02
@rvql
Copy link
Contributor Author

rvql commented Mar 20, 2026

@claude review this pr according to description.

@claude
Copy link
Contributor

claude bot commented Mar 20, 2026

Claude encountered an error —— View job


- Event type: PR general comment triggered by `@claude` on an open PR. - Request type: code review only; no implementation requested. - Key context: PR #11492 (`oracle-separated-bind` -> `main`) updates `agent/src/flow_generator/protocol_logs/sql/oracle.rs` and keeps `agent/crates/enterprise-utils/src/lib.rs` aligned. - Review focus: whether request/bind state is cached correctly across frames so custom SQL extraction can resolve bind values when SQL and binds are separated. - Plan: check repo instructions, inspect the diff against `origin/main`, read the touched files for surrounding context, then post review findings. I can inspect code and diffs locally; I do not currently plan to run a full test suite unless needed for a specific concern.

Tasks

  • Check repository instructions and changed files context
  • Review the Oracle parser diff against origin/main
  • Assess correctness against the PR description
  • Update this comment with review feedback

Status

Starting the review now.

@rvql rvql force-pushed the oracle-separated-bind branch from 5f360db to dd68cbc Compare March 20, 2026 13:11
@rvql
Copy link
Contributor Author

rvql commented Mar 20, 2026

@claude review this pr according to description

@claude
Copy link
Contributor

claude bot commented Mar 20, 2026

Claude finished @rvql's task in 17m 33s —— View job


- Event type: PR general comment on an open PR. - Request type: code review only. - Scope reviewed: `agent/src/flow_generator/protocol_logs/sql/oracle.rs` and the matching `enterprise-utils` stub updates. - Focus: whether caching request info across frames correctly supports custom SQL extraction when Oracle SQL text and bind values arrive separately.

Tasks

  • Read repository instructions and touched file context
  • Inspect the PR diff against origin/main
  • Assess correctness against the PR description
  • Update the PR comment with review feedback

Review

I found 2 correctness issues.

  1. Split requests now underreport RRT when custom policies are enabled

    • agent/src/flow_generator/protocol_logs/sql/oracle.rs:447-473
    • When a SQL-bearing request frame is cached and skipped at oracle.rs:447-452, the request does not reach perf_stats() until a later bind-only frame is parsed. perf_stats() timestamps the request from the current param.time, so the request start moves from the original SQL frame to the later bind frame. That makes Oracle latency depend on whether custom-field policies are configured, and it underreports RRT for exactly the cross-payload case this PR is trying to handle.
    • Fix this →
  2. The placeholder heuristic can suppress valid requests indefinitely

    • agent/src/flow_generator/protocol_logs/sql/oracle.rs:305-335, agent/src/flow_generator/protocol_logs/sql/oracle.rs:440-452
    • sql_max_bind_placeholder() treats any :1 / :x1 substring as a bind placeholder. That includes quoted literals and comments, so a statement like select ':1' from dual will be classified as “waiting for binds”. The new path then caches the request and continues without emitting a log. If no real bind section ever appears, that request disappears entirely. At minimum this needs either parser-backed bind metadata or a safe fallback that emits the cached request unchanged when a bind frame is not actually confirmed.
    • Fix this →

Summary

The overall direction makes sense, but in the current form it changes request timing semantics and can drop some valid Oracle requests entirely under custom-policy configurations.

Cache Oracle request info across frames so custom SQL extraction can resolve bind values when SQL and binds arrive separately. Keep the local enterprise-utils stubs aligned with the parser changes.
@rvql rvql force-pushed the oracle-separated-bind branch from 1991870 to 07a9f95 Compare March 24, 2026 06:56
@rvql
Copy link
Contributor Author

rvql commented Mar 24, 2026

@claude review it

@claude
Copy link
Contributor

claude bot commented Mar 24, 2026

- Event type: PR general comment on an open PR. - Request type: code review only (`@claude review it`), so I should inspect the PR changes and report review feedback without implementing changes. - Key context: PR #11492 (`oracle-separated-bind` -> `main`) updates `agent/src/flow_generator/protocol_logs/sql/oracle.rs` plus the matching `agent/crates/enterprise-utils/src/lib.rs` stub. - Review focus: whether Oracle request state is cached correctly across frames so custom SQL extraction can resolve binds when SQL text and bind values arrive separately. - Plan: read repo instructions from `CLAUDE.md`, inspect `git diff origin/main...HEAD`, read the touched files in context, assess correctness and regressions, then update this comment with findings. I may run targeted git inspection commands; I do not plan to modify code unless explicitly asked.

Tasks

  • Review repository instructions and changed-file context
  • Inspect the Oracle parser diff against origin/main
  • Assess correctness against the PR description
  • Update this comment with review feedback

Status

Starting the review.

View job run

@claude
Copy link
Contributor

claude bot commented Mar 24, 2026

Claude finished @rvql's task in 27m 35s —— View job


- Event type: PR general comment on an open PR. - Request type: code review only; the trigger comment says `@claude review it`. - Key context: review PR #11492 (`oracle-separated-bind` -> `main`) against the description about caching Oracle request info across frames so custom SQL extraction can resolve separated bind values. - Files in scope: `agent/src/flow_generator/protocol_logs/sql/oracle.rs` and `agent/crates/enterprise-utils/src/lib.rs`. - Main tasks: read repository instructions, inspect the diff against `origin/main`, validate behavior for split SQL/bind frames, and post concise review feedback with file references. - Plan completed: read the touched code and surrounding parser/perf/session code, then update this comment with findings. I did not implement changes.

Tasks

  • Review repository instructions and changed files
  • Analyze the Oracle parser diff against origin/main
  • Assess correctness against the PR description
  • Update this comment with review feedback

Review

I found 1 correctness issue.

  1. Cached SQL-only requests can be dropped silently at parser reset / flow end
    • agent/src/flow_generator/protocol_logs/sql/oracle.rs:303-309, agent/src/flow_generator/protocol_logs/sql/oracle.rs:527-550
    • The new path stores a SQL-bearing request in self.cached_info and returns continue when the statement looks like an upsert waiting for bind values. That cache is only flushed in two situations: when a later response arrives (oracle.rs:526-529) or when a later request does not match the bind-only pattern (oracle.rs:530-539). There is no reset() override to clear/emit cached_info, even though the parser is reset after every parse call in agent/src/flow_generator/perf/mod.rs:324-332. If the SQL-only frame is the last Oracle payload seen for that flow, or protocol inference/parser state is reset before the follow-up frame is parsed, the cached request never reaches L7ParseResult, so both the log and any perf accounting recorded for it are stranded.
    • This is especially easy to hit on truncated captures or partial eBPF streams, which are exactly the cases where split Oracle requests already occur.
    • Fix this →

Summary

The cross-frame bind merge logic looks directionally right, and the placeholder detection is much safer than before because it now tokenizes only the supported upsert regions. The remaining problem is lifecycle-related: once a request is hidden behind cached_info, there needs to be a guaranteed flush path when no follow-up frame arrives.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant