Repository navigation
[ISSUE #8255] Expose a bounded normalized request view before governance - #8378
Conversation
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8378 adds a bounded, generation-aware normalized AI request view (issue #8255) so the prompt, token-limiter and proxy plugins share one immutable structural interpretation per request body version. Core additions: AiNormalizedRequestView (immutable value), AiRequestViewHolder (per-exchange slot, idempotent ensurePublished, generation counter, stale-consumer tracking), AiRequestViewOperations (publish/inspect/wireSnapshot + Jackson/SHA-256 parsing) and a Markdown spec. The prompt plugin publishes before rewriting, the proxy publishes before decoding, and the token limiter records an inspection; republish after a body mutation increments the generation and flags earlier consumers as stale. 8 new test files provide broad unit coverage.
Evidence
- Full diff read via resource_diff offset 0 through ~79796 bytes (has_more=false); all 16 files reviewed: AiNormalizedRequestView.java, AiRequestViewHolder.java, AiRequestViewOperations.java, AiRequestViewParser.java, AI_NORMALIZED_REQUEST_VIEW.md, pom.xml edits, AiPromptPlugin.java (+11/-1), AiProxyPlugin.java (+4), AiTokenLimiterPlugin.java (+5) and 8 test files.
- Core contracts verified in head version: AiRequestViewHolder.ensurePublished short-circuits if (current!=null && current.published) and otherwise publishes with generation = first+1; republish via parseBody increments holder.generation; recordInspection increments staleConsumerCount when view.generation != holder.generation; wireSnapshot returns the published view without flagging inspection.
- CI evidence (context_resources) shows 9 checks all status 'queued', conclusion null, complete=true but passed=false and status_count=0; the PASS criterion 'passing CI' cannot be confirmed. Minor: AiNormalizedRequestView.digest documented non-null-once-published but bodyToDigest only runs inside buildRequestView, so digest can be null when parseBody yields empty.
Recommended next steps
- Wait for the 9 CI checks to complete and confirm a passing run before merge (currently all queued, passed=false).
- Clarify the AiNormalizedRequestView.digest contract: document it as nullable or skip publishing when parseBody yields empty.
- Optionally add an integration test exercising prompt-rewrite -> token-limiter -> proxy republish to lock in the stale-consumer detection behavior end-to-end.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 9d4ea60a-3cfe-4f03-907b-f033cdd6e84f · Commit: 905b9923003bf993e17461d30522cce2cf5e5e5c
Review verdict: INCONCLUSIVE
Full diff coverage was achieved and no concrete HIGH/CRITICAL code defect was identified; the design (idempotent per-version publish, generation tracking, stale-consumer detection) is coherent and well tested. However, PASS requires passing CI and the review evidence shows all 9 CI checks are still queued with no conclusions (passed=false, status_count=0), so the passing-CI criterion cannot be verified. Verdict is therefore INCONCLUSIVE pending a completed CI run; no blocking findings are recorded.
Publication: COMMENT (INCONCLUSIVE_REVIEW)
Review details
Reviewed the full diff (16 files, ~79.8 KB) and verified the core view contracts directly. Design is sound: ensurePublished is idempotent per body-version and short-circuits when already published; republishing after a mutation correctly increments the generation, and recordInspection flags earlier consumers as stale (staleConsumerCount), which directly addresses the cross-plugin disagreement described in #8255. The prompt/proxy/token-limiter wiring is minimal and consistent. Tests cover parsing, generation transitions, idempotency, stale detection and each consumer. No blocking code defect found. One contract nit: AiNormalizedRequestView.digest is documented non-null-once-published but can be null when parseBody returns empty (non-AI payloads), because bodyToDigest only runs inside buildRequestView. Recommend tightening the doc or skipping publish on empty parse. I am holding the PASS verdict because all 9 CI checks are still queued with no conclusions (passed=false); please confirm a successful CI run before merging.
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8378 implements a bounded, immutable, generation-aware normalized request view (AiNormalizedRequestView) published once per body version via a per-exchange AiRequestViewHolder, eliminating divergent per-plugin parsing of the same AI request. It adds the view abstraction plus tests and wires ensurePublished/snapshotAndRecordInspection into the AI prompt, proxy, and token-limiter plugins. Full diff reviewed; CI green; no blocking defect.
Evidence
- Full versioned diff consumed to has_more=false (79,796 chars, complete_diff=true): 12 files, +~1.7k/0 deletions — 7 new (AiNormalizedRequestView, AiRequestViewHolder, AiRequestViewOperations, AiRequestViewParser, tests, design doc) plus 4 production wiring edits in ai-prompt, ai-proxy, ai-token-limiter plugins.
- AiRequestViewHolder.ensurePublished is synchronized, one-based generation, only stores fingerprint (FNV-1a) never body, and bounds inspections via AiNormalizedRequestView.MAX_INSPECTIONS=8; AiNormalizedRequestView record bounds messages to MAX_MESSAGES=32 and MAX_MESSAGE_CHARS=512.
- CI context: complete=true, passed=true, 64 checks including e2e / e2e-case (jdbc, rocketmq http+websocket+zookeeper, http, grpc, websocket) all success.
Recommended next steps
- Optionally harden AiRequestViewParser to return an unmodifiable messages list so the shared BoundedMessage overview cannot be mutated by a consumer.
- Confirm downstream consumers (token limiter, any safety/observability stage) read the view through snapshotAndRecordInspection so staleness is surfaced end-to-end.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: f76a4d3c-1e78-4d13-a1fc-ae80c0ea3cb1 · Commit: 905b9923003bf993e17461d30522cce2cf5e5e5c
Review verdict: PASS
Full diff coverage achieved (all pages read, has_more=false, complete_diff=true). CI is complete and passing (64 checks, passed=true). The implementation correctly delivers a bounded, immutable, generation-aware request view: the holder retains only a fingerprint (no body retention, limiting memory exposure), synchronizes all access, increments the generation only on a distinct body via FNV-1a fingerprint, and distinguishes WIRE vs EFFECTIVE views. The plugin wiring is consistent — prompt publishes WIRE then EFFECTIVE only on real mutation, proxy publishes authoritative decode body while returning the current view on fingerprint match, and the token limiter records the inspected generation for staleness detection. Inspection logging and message overview are bounded (MAX_INSPECTIONS=8, MAX_MESSAGES=32, MAX_MESSAGE_CHARS=512). The only residual risk (hash collision -> missed regeneration) is documented and bounded, not a concrete blocking defect. No HIGH/CRITICAL findings; recommend approval with a non-blocking hardening suggestion to defensively unmodifiable the parser's messages list.
Publication: APPROVE (EXPLICIT_PASS)
Review details
The change is well-scoped and correct. The holder stores only a 64-bit FNV-1a fingerprint (never the body), uses a synchronized lock, increments a one-based generation only on a genuinely new body version, and bounds both the message overview (MAX_MESSAGES=32, MAX_MESSAGE_CHARS=512) and the consumer inspection log (MAX_INSPECTIONS=8). Prompt publishes a WIRE view then an EFFECTIVE view only when it actually mutates the body; the proxy publishes before decode (returning the current view when the fingerprint already matches, so it does not create a spurious generation); the token limiter records the generation it inspected, making stale interpretations observable. The only acknowledged risk is a FNV-1a collision causing a missed regeneration, which is documented and bounded. All 64 CI checks passed. Recommendation: approve; consider a follow-up to defensively copy/unwrap the messages list returned by the parser for stronger immutability guarantees.
Fixes #8255.
Problem
The AI plugin chain has no shared interpretation of the request it governs: the prompt plugin rewrites the body through one parser, the proxy decodes it through another, and no stage can state which version of the request it acted on — a token estimate, a safety decision and the final decode can silently disagree about the same request, and nothing distinguishes a deliberate passthrough from an unnoticed mutation. This delivers the bounded normalized request view of #8255: one immutable, generation-aware structural interpretation published once per body version, consumed by every governance stage that needs metadata instead of re-parsing the body.
Approach
AiNormalizedRequestView(shenyu-common) is the contract: protocol/operation/model hints, a bounded message overview (role, content kind, bounded text prefix), tool presence, and an explicit unsupported flag with a reason — unsupported inspection stays visible, never silently empty. Bounds are part of the contract: at most 32 message entries, 512 characters of textual content per message, structured content is reported by kind and never flattened into text, and at most two views plus a bounded inspection log are retained per exchange. The request body itself is never retained, only a 64-bit content fingerprint per publication, and no request content leaves the view contract — it is never placed inAgentRequestContext, trace or audit events, and the state lives and dies with the exchange attribute (no global map, no cleanup path).Publication is idempotent per body version (
AiRequestViewOperations): the first publication is theWIREview; a publication with different content creates the next generation as anEFFECTIVEview naming the mutator. Consumers read throughsnapshotAndRecordInspection, which records the generation they saw; a consumer whose generation is no longer current is reported as superseded, so a misordered or stale consumer cannot claim a successful inspection of the effective request, andwireSnapshotkeeps the original representation reachable after mutations.The wiring keeps the production surface minimal and behavior neutral: the prompt plugin publishes the wire view it already holds and, only when decoration really changes the body, the effective generation; the token limiter records the generation it acts on (inspection only — budget estimation plugs into the same seam later); the AI proxy publishes the body it is about to decode, so an unwired upstream mutation is detected at the terminal stage while a republished identical body is a no-op. The authoritative decode in
AiProxyEngine.prepareis untouched — the view is an interpretation, never a replacement for protocol decoding. PII and sensitive-word remain documented full-content consumers (scanning a bounded view would weaken detection). The coverage matrix and source-to-consumer table live inshenyu-plugin-ai/AI_NORMALIZED_REQUEST_VIEW.md.Testing
All tests are deterministic and sleep-free. They cover the bounded overview (per-message truncation, message-count cap, structured content not flattened), role and tool-call preservation, malformed/empty/non-matching bodies staying visible as unsupported views, idempotent republication, generation bumps only on real mutations, superseded versus current inspections, the bounded inspection log, exchange isolation, split-buffer bodies yielding one view, the wire view surviving effective generations, and proxy-only behavior staying unchanged.
shenyu-common666 tests,shenyu-plugin-ai-common56 (15 new),shenyu-plugin-ai-prompt20 (4 new),shenyu-plugin-ai-token-limiter55 (2 new),shenyu-plugin-ai-proxy463 (3 new) — all green; Checkstyle clean on all five modules.