fix(plugins): complete BQAA privacy and shutdown hardening

[caohy1988]

Summary

Current main contains the baseline BigQuery Agent Analytics hardening, but it does not include the later privacy, shutdown, and resource-lifecycle fixes validated during google#6360. This PR ports that complete reviewed implementation and regression corpus onto the fork's latest synchronized main.

The result is a BQAA pipeline that fails closed around formatter/parser output, prevents encoded or adversarial values from republishing sensitive content, performs bounded linear sanitization, and reports shutdown success only after all owned work and clients are actually drained or closed.

Area	Outcome
Privacy	Formatter and parser boundaries accept only supported native shapes, normalize nested output, redact sensitive mappings, diagnostic text, signed URIs, and raw prompt text before inline/GCS storage, and use constant non-payload logs on failure. Encoded, concatenated, malformed, and oversized JSON-like layers fail closed.
Readiness	Existing BigQuery fields must match the desired type and mode recursively; incompatible tables fail startup instead of accepting rows that will never write.
Shutdown	Concurrent callers coalesce on an honest completion rendezvous; cancellation, remote-loop failure, stale loops, task-factory rejection, dead retained processors, and Python 3.10 worker cancellation preserve retryable ownership rather than reporting false success.
Resources	BigQuery and GCS clients plus executors are closed off-loop under the shutdown budget, including pre-supplied-client, orphan-client, invalidated-publication, detached dead-writer, and construction-failure paths.
Concurrency/performance	Queue accounting is O(1), collision markers remain unique, BOM/whitespace scanning is linear, JSON nesting is bounded consistently across Python versions, and remote drains are created and finalized on their owning event loop.

Review follow-ups

Commit 40f8a362 addressed the six P1s requested in review 4731058331: count-only write-error logs, recursive schema type/mode validation, pre-storage content redaction, model-span cleanup on short-circuit, host-cancellation propagation, and atomic dead-writer replacement.

Commit 83ee8673 addresses review 4731753868 and its P3 follow-ups:

• Python 3.10 shutdown compatibility uses a shielded gather(..., return_exceptions=True) acknowledgement owner. This avoids the unavailable Task.cancelling() API while still distinguishing the worker's expected cancellation from host cancellation; a getattr fallback alone would have swallowed host cancellation on 3.10.
• error_message and AGENT/INVOCATION tracebacks now pass through one bounded diagnostic privacy boundary. Structured/encoded credentials, sensitive key fragments, temp: values, Authorization/Basic/Bearer headers, signed-query material, and Unicode/hex-escaped sensitive keys are redacted or fail closed; safe prose remains exact and content loss sets is_truncated.
• A live processor already marked _shutdown now returns a distinct lifecycle-aborted outcome to both the setup owner and coalesced waiters, without arming setup backoff or tearing down the retained writer. The row owner records exactly one shutdown_race loss.
• Detached and newly-created write transports have structured, bounded ownership on replacement, constructor/start failure, invalidated publication, cancellation, and success. No fire-and-forget close task or unbounded invalidated close remains.
• External file URIs redact sensitive query and fragment values; URI userinfo and non-string values fail closed.
• function_response, executable_code, and code_execution_result parts now emit bounded, JSON-native, sanitized summaries and content_parts instead of empty placeholders.
• Bracket-led prose was fixed differently from the review's prefix suggestion: bounded ordinary content such as [INFO], Markdown links, {not json}, and safe Windows-path diagnostics is preserved intact inline and in GCS, while malformed or encoded credential-shaped input still fails closed. The stricter generic attributes sanitizer is unchanged.

Commit 171db081 addresses review 4736064808 as one sanitizer-class fix:

• [REDACTED] is idempotent only as a complete value; suffix mutations including SECRET, ]SECRET, and /SECRET are wholly redacted.
• A bounded linear canonicalizer recognizes nested Unicode, hex, percent, and double-percent encodings for detection only. Safe Windows paths, JSON decoder errors, and unrelated percent-encoded prose remain byte-identical.
• Authorization and Proxy-Authorization header values redact completely across Bearer, Basic, Digest, Negotiate, AWS4, and other schemes. Standalone Bearer and semantically valid/padded Basic tokens redact across whitespace without corrupting ordinary “bearer of bad news” or “basic test” prose.
• Exact-boundary signature variants (sig, AWS, Google, and X-Goog forms) now share the mapping, diagnostic, bracket-content, signed-URI, and structured-part privacy policy; hyphen/underscore forms are equivalent.
• External URI path components use a conservative sensitive-key/next-value policy in addition to query/fragment handling, and percent-encoded key separators cannot bypass redaction.
• Structured part dictionary keys are sanitized with collision-safe allocation, so key-side signed URLs cannot leak or overwrite another field.
• Invalidated fresh writer transports close in a finally even when cancellation interrupts their processor shutdown.

The earlier claim that test_get_loop_state_uses_single_lookup is vacuous was also rechecked and remains non-reproducible: reverting to membership-plus-index triggers its mutating __contains__ hook and raises KeyError.

Upstream synchronization

The fork's main and this PR are synchronized with google/adk-python main at f1a0a148. The upstream BQAA changes merged without conflicts, including 6e43800f, which makes flush() wait for dequeued in-flight writes via Queue.join(), plus deterministic write-synchronization tests and use of the platform thread helper in multi-threaded tests. The combined plugin suite passes with both the upstream synchronization changes and this PR's shutdown/privacy hardening.

Testing plan

• python -W error::RuntimeWarning -m pytest -q tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py
  • 434 passed, 6 skipped
  • zero never-awaited-coroutine warnings
• Focused latest-review regressions: 19 passed
• CPython 3.10.16:
  • changed module compiles successfully
  • standalone worker-cancel and host-cancel harness verifies the shielded-gather distinction without Task.cancelling()
  • the GitHub Python 3.10 matrix remains the authoritative full dependency-suite run
• pyink --check on the two changed files
• isort --check-only on the two changed files
• ruff check on the two changed files
• git diff --check
• Focused mypy: 35 pre-existing diagnostics, unchanged from the previous PR head; zero diagnostics introduced by this follow-up

As an independent regression check, the original 404-test corpus produced 57 failures plus one teardown error when run against unmodified latest upstream main; the expanded 434-test suite passes with this branch. GitHub's Python 3.14 job also exposed and now covers the decoder's runtime-specific acceptance of extreme JSON nesting.

Related: google#6356
Related: google#6360

[caohy1988]

Full review of fix/bqaa-main-hardening (d350fec vs main @ be5828f)

Verdict: no blocking issues found. The full 2,453-line source diff was read line by line, checked against all 8 recorded pitfalls from the google#6360 review rounds, the test diff (67 new tests) was inspected, and the complete CI matrix is green at the head commit. Two minor, non-blocking observations below.

Scope check: clean

The diff touches exactly the two files the PR describes (plugin + its test file, +3,794/−553). Every change maps to the stated intent — privacy fail-closed boundaries, honest shutdown, resource lifecycle, and the Python 3.14 JSON-nesting bound. No unrelated drive-by changes.

Critical pass: what was verified

• All 8 prior pitfalls from the fix(plugins): close BQAA fail-open privacy, GCS concurrency, and startup-loss gaps (#6356) google/adk-python#6360 rounds are correctly ported: concatenated-JSON raw_decode handling, ValueError fail-closed (int digit limits), duplicate-key reserialization via object_pairs_hook, namedtuple-as-mapping rebuild, plain-list emission for tuple subclasses, ConcurrentFuture-based cross-loop setup coalescing, asyncio.shield(asyncio.wrap_future(...)) on both the setup and shutdown waiter paths, and loop-bound writer creation kept out of shared setup.
• Shutdown honesty contract holds. Traced the ownership/waiter loop: a waiter that sees _ShutdownIncompleteError retries ownership as a loop (bounded — each caller does at most one owned teardown before raising to its own caller), the completion future is resolved only on true completion, retained_remote_drains correctly forces failure, and the finally block does the flag reset and future swap in one guarded transition. Lock ordering (_loop_states_guard then _drop_counts_guard) is consistent at every site — no deadlock path.
• One subtle spot chased to ground: in _sanitize_json_blob, stripped[:end] looks like it could hit an undefined end when json.loads succeeds on the whole string — but that branch is unreachable (it requires a non-empty suffix, which only exists on the raw_decode path where end is bound). Correct, though it took real tracing to prove.
• The fix(ci) commit is sound: the fixed _MAX_JSON_NESTING_DEPTH = 1000 preflight makes deep-nesting behavior deterministic across CPython ≤3.13 (RecursionError) and 3.14 (iterative decoder), preserving fail-closed either way.

Findings (both informational, neither blocks merge)

1. _get_loop_state reverted a documented lookup pattern. Old main used .get() with a comment explaining that membership-test-then-index could KeyError against a concurrent shutdown; this branch reverts to if loop in dict: return dict[loop]. Analysis says it is actually safe now: deleting a live loop's entry requires that loop to run its own drain task first, and the generation check guards the re-entry path — so the KeyError window no longer exists. But the safety is implicit (no awaits in that window, drains run on the owning loop) where it used to be explicit. A one-line .get() restores the robustness for free; worth a follow-up, not worth a new CI cycle alone.

2. _normalize_json_native placeholder keys can collide with a real key (dict branch of the normalizer). A non-string key becomes [UNSUPPORTED_KEY_1]; a genuine string key literally named [UNSUPPORTED_KEY_1] in the same mapping would silently overwrite it (the full sanitizer has [KEY_COLLISION_n] handling for exactly this; the normalizer does not). The loss is still flagged via replaced=True, so the row is marked truncated — impact is cosmetic telemetry loss on a pathological input. Fine to leave.

Tests and CI

• The test diff adds 67 tests mirroring each hardening fix, including negative paths. The one time.sleep(0.2) is a deliberate slow-close simulation coordinated by threading.Events, not a flaky timing assertion. No real-network calls or shared-mutable-state issues spotted.
• CI is fully green at d350fec: unit tests on Python 3.10–3.14, mypy on 3.10–3.13, A2A on all five versions, and lint. PR state is MERGEABLE / CLEAN.

[caohy1988]

Addressed both informational findings in 9607f2ee.

_get_loop_state reverted a documented lookup pattern.

Restored a single .get() lookup and added a deterministic regression mapping that deletes the entry during the old __contains__ path. The old split lookup raises KeyError; the new lookup returns the existing state without a race window.

_normalize_json_native placeholder keys can collide with a real key.

Normalized dictionaries now preserve the first writer under the plain key and reallocate [KEY_COLLISION_n] markers until unique. This covers both input orderings, genuine marker-like keys, sensitive-value insertion, and the sanitization-budget sentinel without changing redaction or budget accounting.

Verification after the combined fix: 407 passed / 6 skipped with RuntimeWarning treated as an error; pyink, isort, and git diff --check clean; focused mypy unchanged at the 35-diagnostic baseline.

[caohy1988]

Code Review — BQAA hardening port

Reviewed with 4 parallel specialist passes (security, concurrency, testing/maintainability, adversarial red-team) against fork/main...pr-11-review; top findings manually verified against the code.

Scope Check: CLEAN — the diff delivers exactly what the description states (2 files, +3897/−552). No scope creep.

High-confidence findings (multi-source confirmed)

1. [P1] bigquery_agent_analytics_plugin.py:2297 — full row payloads dumped to logs on non-retryable write errors.

logger.error("Row content causing error: %s", rows)

Prompts, user messages, tool args — unredacted — land in application logs precisely when content is weird enough to fail server-side validation. Directly contradicts the "constant non-payload logs on failure" claim. Fix: delete it or log len(rows) only.

2. [P1] bigquery_agent_analytics_plugin.py:4001-4013 — _schema_fields_match compares field NAMES only.

existing_by_name = {f.name: f for f in existing}
...
if existing_field is None:
    new_fields.append(desired_field)
elif (desired_field.field_type == "RECORD" and existing_field.field_type == "RECORD" ...):

A pre-existing table with content as STRING instead of JSON (the common "table from an older deployment" case) passes readiness, _started=True, and every AppendRows then dies as non_retryable for the life of the process — exactly the silent-write-death scenario the fail-closed readiness claim targets. Fix: compare field_type and mode; treat mismatch as unready.

P1 — remaining (each verified or high-confidence)

• [P1] Content/prompt path gets no credential redaction. The mandatory final pass (_recursive_smart_truncate, :5487) runs on attributes only; content/content_parts get length truncation only (:2679-2683, :2787-2788). A user message containing {"access_token": "..."} or service-account JSON is stored verbatim — and _SENSITIVE_KEYS (:415-422) omits private_key, token, secret, authorization. GCS-offloaded text also uploads raw before any sanitizer sees it. Biggest privacy gap in the PR.
• [P1] before_model short-circuit leaks the llm_request span. Push at :6078, pop only in after_model_callback (:6166). When a guardrail/cache callback returns an LlmResponse, base_llm_flow._call_llm_with_tracing short-circuits (yield response; return) — after_model_callback never runs — and every subsequent row in the invocation gets a wrong span_id/parent_span_id. on_agent_error_callback is kind-guarded (:6363); this path isn't.
• [P1] after_run_callback → flush() → unbounded queue.join() (:2018). Each failing batch costs up to 30s × (retries+1) + backoff, serialized — a BQ outage parks the agent's own run completion for minutes (PluginManager only timeboxes close()). Also: retry after a client-side timeout on the _default stream re-sends a batch that may have committed server-side → silent duplicate rows.
• [P1] :2400-2404 — external cancellation landing on the worker-ack await is swallowed as the worker's own ack; shutdown then reports success. Fix: re-raise if current_task().cancelling() > 0.
• [P1] :3652-3654 — after an incomplete shutdown, the retained state (dead worker, _shutdown=True) is handed back by _get_loop_state; _started flips True and rows are appended to a queue nothing drains — silently lost, uncounted. Fix: treat a state with a dead processor as absent and rebuild.
• [P1] :4445 — await state.write_client.transport.close() has no timeout. A hung gRPC close blocks the shutdown owner forever; coalesced waiters at :4278 (shield(wrap_future(waiter)), no timeout) hang with it. Same pattern at :3720, :4918. Fix: asyncio.wait_for(..., timeout=t).
• [P1] :4939-4945 — _teardown_aborted_setup awaits executor.shutdown(wait=True) unbounded while the _setup_future rendezvous is still held; a wedged constructor job wedges all future setup waiters forever.
• [P1] (py3.10-specific) :2201-2205 — worker suppresses cancellation in its error-recovery sleep and returns normally; pre-3.11 wait_for then returns instead of raising, so the shutdown drain is skipped and queued rows vanish uncounted. requires-python >= 3.10 makes this live.

P2 — notable

• Fail-closed JSON heuristic guts legitimate telemetry (:769, :814-822): any string starting with [/{ that doesn't parse becomes [UNPARSEABLE_JSON_BLOB]. [INFO] server started-style tool output is the common case in production; at volume this silently destroys the args/result analytics the table exists for.
• error_message=str(error) written unsanitized/unbounded (:6215, :6332, :6380, :6430) — API exceptions routinely embed request bodies/tokens; bypasses every redaction pass. Same for error_traceback (:6366-6373, :6415-6422).
• Prose-embedded credential JSON bypasses blob redaction (PoC-verified): strings not starting with {/[ return uninspected (:769-770); note: {"access_token":"SECRET"} persists verbatim.
• Coalesced shutdown waiter swallows any non-_ShutdownIncompleteError and returns success (:4286-4288).
• Shutdown "budget" is ~4× claimed — each of 4 sequential phases gets the full t (:4304, :4380, :4467, :4491).
• Duplicated claim-fold accounting (:3494-3517 vs :4334-4354, ~20 lines verbatim) and duplicated key-normalization with two inconsistent sentinel dialects (:580 vs :1135-1141) — extract shared helpers before they drift.
• BatchProcessor.close() keeps its own inline drain (:2457-2471) that pops the sentinel without decrementing _sentinel_count, skewing the O(1) accounting used everywhere else. Call _drain_queue_and_count("shutdown_timeout") there too.
• Vacuous test: test_get_loop_state_uses_single_lookup (test:12883) installs a __contains__ hook the source never calls (it uses .get()) — passes with the source change reverted. Delete or fix.
• Flaky timing assert: assert elapsed < 0.15 (test:12399) will fail on loaded CI runners; the thread-identity assert is the real invariant.
• Coverage gaps: executor-retention branch on cancelled setup (:4792) and the done-but-not-cancelled worker arm (:2388-2390) have no tests.
• __getstate__ doesn't scrub _credentials (:4557-4578) — pickling the plugin serializes live OAuth refresh tokens/private keys.
• DDL identifier interpolation (:4155, :3310-3318) — view_prefix/dataset_id/table_id go into CREATE OR REPLACE VIEW raw; trusted-config-only, but a backtick breaks out. Validate ^[A-Za-z0-9_]+$ at construction.
• Partial-commit misaccounting (:2293-2298): AppendRows with row-level errors can commit valid rows, yet the whole batch is counted as non_retryable dropped — the loss signal over-reports while the timeout-retry path under-reports duplicates.
• atexit.register per loop-state, never unregistered (:3725) — unbounded accumulation under loop churn.
• Parser-None window drops escape accounting (:5406-5408) — no _count_local_drop, unlike every sibling early-return.
• on_run_error_callback never flushes (:6388-6441) — error-terminated invocations lose their final queued rows in short-lived processes.
• The 6 test skips are pre-existing optional-extra guards, but the net effect is zero coverage of the TRANSFER_A2A/MCP classification branches in the reported run — consider a CI job with the extras installed.

Verified as holding (specialists probed and could not break): double/unicode-encoded credential layers, temp: keys, StrEnum/namedtuple redaction, BOM/whitespace linearity, fail-closed formatter sentinels, shutdown coalescing/generation guards, O(1) queue accounting, loop-affinity of remote drains, orphan-client close on cancelled setup.

Verdict

Request changes. The architecture is sound and most hardening claims hold under adversarial probing, but four findings undercut the PR's headline guarantees: the row-payload log (#1), name-only schema readiness (#2), the unredacted content path, and the span leak. Those plus the cancellation-swallow and dead-processor-handback (both small and mechanical) should land in this PR; the rest can be follow-ups.

[caohy1988]

Addressed the six requested P1s in 40f8a362.

1. Row-payload logging: removed the full rows log from the non-retryable path. It now records only the dropped-row count; a regression puts a secret in the rejected row and proves it never reaches caplog.
2. Schema readiness: _schema_fields_match now rejects same-name fields whose type or mode differs, recursively for nested records. Type/mode mismatches leave startup failed and do not call update_table.
3. Content privacy: expanded the sensitive-key set with private_key, token, secret, and authorization. Raw role/system/prompt/part text is sanitized before truncation, inline storage, preview generation, or GCS upload. Regressions cover parser output, the serialized BigQuery row, and the exact GCS upload argument; formatter/redaction sentinels remain intact.
4. Short-circuit span leak: kept ADK's intentional “no after_model on before_model short-circuit” contract. BQAA now closes only an expected llm_request span at the synthesized non-partial event boundary, with a defensive cleanup before the next model request. Partial streaming events retain their live span, and normal/error pop sites are kind-guarded.
5. Cancellation during worker acknowledgement: the timeout branch distinguishes the worker's own cancellation from host cancellation using the owning task's cancellation count and re-raises the latter. The deterministic regression cancels the owner precisely during the acknowledgement await, then proves a retry accounts the retained row.
6. Dead processor handback: _get_loop_state no longer returns a processor whose worker is absent/done. It atomically detaches and folds the old state, accounts queued rows in O(1), publishes a fresh live writer, and performs bounded best-effort transport cleanup. A concurrent caller receives the same replacement, and the new worker is proven to drain a row.

Verification at 40f8a362:

• 418 passed / 6 skipped with RuntimeWarning promoted to error
• zero never-awaited-coroutine warnings
• pyink, isort, Ruff, and git diff --check clean
• focused strict mypy remains at the branch's existing 35 diagnostics; no new diagnostic is introduced by these changes
• branch is 0 commits behind the latest fetched upstream main

I left the other timeout-budget, Python 3.10 recovery, error-payload sanitization, retry/idempotency, and maintainability findings for follow-ups, matching the review verdict. One review item was not changed: test_get_loop_state_uses_single_lookup is not vacuous. Reverting the source to membership-plus-index invokes its mutating __contains__, deletes the entry, and raises KeyError; the current .get() implementation is what makes it pass.

[caohy1988]

Follow-up Review — commit 40f8a36

Re-reviewed with two verification passes (privacy+schema, concurrency+span), including empirical checks on CPython 3.10.16 / 3.11.13 / 3.12.9 / 3.13.5 and mutation-testing of the new tests.

Verdict: 4 of 6 fixes verified solid. Fix 5 is broken on Python 3.10. Fix 3 left a previously-flagged column raw.

Fix-by-fix

#	Fix	Status
1	Row payload log removed	✅ Verified — count-only log at :2298
2	Schema type/mode compatibility	✅ Verified — raises before any update, fails setup closed (_started never set, bounded backoff retry), mode can't be None, nested path tracking works. New tests are mutation-proof.
3	Content credential redaction	⚠️ Partial — every text path sanitized (inline, GCS upload, preview, offload-failure fallback, role strings, system instruction); new tests genuinely guard it. Two gaps below.
4	Span leak on short-circuit	✅ Verified — all six flows walked (normal, streaming, later-plugin short-circuit, own-plugin, model error, sequential calls); the kind-guarded pop can only take the correct span. Tests fail if the pop is removed. (Minor correction to the PR response: the defensive pop is in before_model_callback, not after_model_callback — placement is correct.)
5	External cancellation preserved	❌ Broken on 3.10 — see below. Correct on 3.11+ (verified: no false-positive re-raise on the normal timeout path).
6	Dead processor replacement	✅ Verified — pop is single-winner under guards, no double-counting, the "still shutting down" RuntimeError path drops and counts the event, detached close fully bounded.

❌ [P1] Fix 5 crashes on Python 3.10

bigquery_agent_analytics_plugin.py:2418:

current_task = asyncio.current_task()
if current_task is not None and current_task.cancelling():

Task.cancelling() was added in 3.11; the repo declares requires-python = ">=3.10" and CI tests 3.10. Verified on 3.10.16: hasattr(asyncio.Task, "cancelling") is False, no compat shim.

Normal timeout path on 3.10: wait_for cancels the worker → TimeoutError → await worker raises CancelledError → handler calls .cancelling() → AttributeError, escaping shutdown()/close(), skipping _drain_queue_and_count, and replacing the very CancelledError the fix was meant to preserve. The pre-existing test_shutdown_timeout_counts_lost_rows should also fail on 3.10 — expect the 3.10 matrix job to go red.

One-line fix:

if current_task is not None and getattr(current_task, "cancelling", lambda: 0)():

⚠️ [P1] error_message column still stored raw

Flagged in the first review, untouched by this diff. :5654 writes "error_message": event_data.error_message straight into the row from str(error) (:6369, :6486, :6534, :6584) — bypassing both the parser and the attributes sanitize pass. google.auth exceptions routinely quote tokens/headers. error_traceback gets partial coverage via the blob sanitizer, but prose passes through. Fix: run both through _recursive_smart_truncate and cap to max_content_length before row assembly.

⚠️ [P2] Fix 3 quietly extends destroy-on-bracket semantics to ALL content

_sanitize_raw_text means any user message/prompt whose first non-whitespace char is [, {, or " and isn't valid JSON becomes [UNPARSEABLE_JSON_BLOB] — in the row, the summary, AND the GCS object. Verified: [INFO] please summarize this, [link](https://x.com), {not json} are all wholesale destroyed. This was a P2 in the first review when it only affected attributes; it now covers 100% of content. Fail-closed is defensible, but it should be a conscious, documented trade-off — right now no test pins it as intended for content. Consider a truncated-prefix sentinel (first 200 chars + [UNPARSEABLE_JSON_BLOB]) so analytics keep signal.

New P3s (follow-up material)

• :3735 — the "still shutting down" RuntimeError routes through generic setup-failure handling, arming exponential backoff and misclassifying later drops as setup_unavailable instead of shutdown_race. Self-limiting.
• :3771 — if fresh-state construction raises after the detached-close task is created, the task is never retrieved → "Task was destroyed but it is pending" noise at loop teardown.
• :3840 (pre-existing) — the invalidated-publication branch still does an unbounded await transport.close(); the new bounded helper is right there to copy.
• :2636 — file_data.file_uri stored raw; signed URLs carry X-Goog-Signature into content_parts (structurally outside the JSON sanitizer's reach).
• :2726-2731 — function_response/executable_code/code_execution_result parts fall through _parse_content_object unhandled; all-None rows in content_parts (completeness gap, not privacy).

Bottom line

Fixes 1, 2, 4, 6 are merge-quality — well-implemented and mutation-tested. Blocking: the 3.10 cancelling() crash (one line) and the raw error_message column (small). The bracket-prose destruction needs an explicit decision, not necessarily code. The rest is follow-up.

[caohy1988]

Addressed the follow-up review at review 4731753868 and pushed the fixes as 83ee8673.

Finding	Resolution
Python 3.10 Task.cancelling() crash	Fixed differently. The worker acknowledgement is now shield(gather(worker, return_exceptions=True)). This is available on 3.10, turns only the worker's expected CancelledError into data, and still raises when the shutdown owner is cancelled. The suggested getattr(..., lambda: 0) avoids AttributeError but would still swallow host cancellation on 3.10.
Raw error_message / prose traceback credentials	Fixed. Both surfaces now cross one bounded, fail-closed diagnostic sanitizer before formatting/parsing. It handles structured and encoded JSON, underscore/hyphen key fragments, temp: keys, Authorization/Basic/Bearer headers, signed-query values, and Unicode/hex-escaped sensitive keys. Safe prose is preserved exactly, and any redaction/truncation sets is_truncated.
Bracket-led content destruction	Fixed with an explicit content policy. Bounded, escape-free ordinary content such as [INFO] please..., Markdown links, and {not json} is preserved byte-for-byte both inline and before GCS upload. Malformed credential JSON, escaped sensitive keys, backslashes, or recognized credential markers remain fail-closed. The generic attributes sanitizer keeps its stricter contract.
Live writer already shutting down	Fixed. A distinct lifecycle-aborted exception wakes the setup owner and coalesced waiters as aborted, without setup backoff or shared-resource teardown; the row owner counts one shutdown_race.
Detached close task retrieval	Fixed. The claimant now owns and awaits bounded old-transport cleanup in finally across success, failure, and cancellation. Fresh publication still occurs before old cleanup so concurrent callers share the replacement.
Invalidated publication close	Fixed. It uses the same bounded transport helper. Fresh clients are also bounded-closed if BatchProcessor construction/start fails.
Raw signed file URI	Fixed. Sensitive query/fragment values are redacted; userinfo and non-string/None URI values fail closed while safe location and parameters remain usable.
Missing structured Part variants	Fixed. function_response, executable_code, and code_execution_result now produce bounded, JSON-native, sanitized summaries and content_parts, with no arbitrary repr/str fallback.

Two issues found during the final independent pass were also closed before push: Authorization header redaction no longer produces an extra ], and prose-embedded access\\u005ftoken cannot bypass the diagnostic boundary.

Verification at 83ee8673:

• BQAA suite: 427 passed / 6 skipped with RuntimeWarning promoted to an error; no never-awaited warning
• latest-review focused class: 13 passed
• exact CPython 3.10.16 module compile plus standalone normal-worker-cancel/host-cancel semantic harness: passed
• pyink, isort, Ruff, and git diff --check: clean
• focused mypy: 35 existing diagnostics, unchanged from the previous PR head; no new diagnostic from this patch

GitHub CI is now fully green from 83ee8673: pre-commit, mypy 3.10–3.13, unit tests 3.10–3.14, and A2A tests 3.10–3.14 all passed. In particular, the previously failing Python 3.10 unit job passed in 4m12s.

[caohy1988]

Round-3 Review — commit 83ee867

Verified with two passes (adversarial privacy/sanitizer, concurrency/lifecycle), including empirical repros on CPython 3.10.16 / 3.12.9 / 3.13.7 and live re-runs of the new tests against the v3 source.

Headline: all claimed fixes work. No P1s. The concurrency/lifecycle half is clean. The new _sanitize_sensitive_text has 5 P2-grade evasion/false-positive classes worth one more iteration.

Claimed fixes — verification results

Fix	Status
Python 3.10 cancellation	✅ Verified — shield(gather(task, return_exceptions=True)) is correct and version-portable. Empirically repro'd on 3.10/3.12/3.13: normal timeout drains and counts; host cancel propagates; no unretrieved-exception warnings. New tests run live against v3 and genuinely guard this.
Raw error_message/tracebacks	✅ Fixed — all four producers route through the new _log_event boundary (:5833), unconditional on event type, fail-closed on sanitizer exception, single row writer. But see FP finding #2 below.
Bracket-led prose	✅ Fixed — [INFO], markdown links, {not json} preserved byte-identical, inline and in GCS. Fail-closed side holds (escaped keys, temp:, valid credential JSON still destroyed). Genuine pinning test.
Lifecycle accounting	✅ Verified — _LoopStateAdmissionAbortedError provably never touches _startup_error/backoff; owner+waiter both get "aborted"; retry after drain succeeds.
Bounded transport cleanup	✅ Verified — detached close now in a finally with structured ownership; every fresh-construction failure/cancellation interleaving closes the client. The pre-existing unbounded close is gone.
Signed URI redaction	✅ Mostly — userinfo/scheme-less/duplicate/percent-encoded keys/fragment/gs:// all held under attack. Residual gaps below.
Structured-part handling	✅ Fixed — function_response/executable_code/code_execution_result captured with structural + per-string sanitization; depth/budget can't be bypassed.

New P2s — all inside the new text sanitizer (empirically confirmed)

1. access_token=[REDACTED]SECRET passes verbatim (:541-546, conf 9). The [REDACTED] idempotency carve-out matches as a prefix of the value, so anything upstream that pre-redacts a prefix becomes an exfil channel. Fix: only treat [REDACTED] as safe when it's the entire value, e.g. \[REDACTED\](?![^\s,;&}\]]).
2. Backslash+marker nukes entire innocuous error messages (:561-565, :494, conf 9). Any message with a literal \ and any marker substring → [REDACTED_SENSITIVE_TEXT]. Empirically destroyed: FileNotFoundError: 'C:\Users\secret\config.json', json.decoder.JSONDecodeError: Invalid \escape, and worst — CPython's standard SyntaxError: ... can't decode \x5c in position 0 fails closed with no credential marker at all (\x5c alone is treated as "introduced escape"). Windows-path and JSON-decode errors are the most common error_message content in production — this blanks exactly the rows the fix was meant to protect. No test pins this. Fix: only fail closed when a decoded marker is actually found, not on the mere presence of \x5c.
3. Authorization: Digest SECRET redacts only the scheme word (:465-468, conf 8) — the scheme word is consumed as the credential; the payload survives. Affects Digest, Negotiate, AWS4-HMAC-SHA256 Credential=... in 401/403 error text.
4. URI path component never inspected (:2763-2807, conf 8) — https://h/download/TOKEN/file.zip survives verbatim; only query/fragment are sanitized. Token-in-path links are a real credential class.
5. Urlencoded access_token%3DSECRET evades (:459-463, conf 9) — signed-URL material often appears double-encoded in HTTP error text.

New P3s

• Prose-restore gate is weaker than the sanitizer: [log] BEARER\tX, bearer\nX, sig=X, Basic dXNlcg== restored verbatim (:2748).
• sig=, X-Amz-Signature, X-Amz-Credential, GoogleAccessId not in the key list; bearer\n evades [ \t]+ (:469).
• function_response dict keys never text-sanitized — {url_with_sig: metadata} maps leak key-side (:2827).
• Prose corruption: "the bearer of bad news" → "the bearer [REDACTED] bad news" (:469).
• Host-cancel during the shield lands inside the TimeoutError handler, so the sibling CancelledError handler is bypassed — rows are counted later via the retry/fold paths instead of immediately (:2567-2577, conf 9; loss still eventually observable, just delayed).
• Narrow leak: host-cancel during the invalidated-publication batch_processor.shutdown() skips the fresh transport close (:4184-4193, conf 7).
• model_dump() runs before any size budget — multi-MB tool responses serialize unbounded on the event loop (:2789, conf 6).
• test_normal_timeout_retrieves_worker_cancel_and_drains_queue is only meaningful on 3.10 — nearly vacuous on ≥3.11 (the old code passed it there too). Its guard value depends on CI keeping the 3.10 job.
• Untested attack surface: the [REDACTED]-prefix carve-out, Digest schemes, %3D, path credentials, the backslash-FP class.

Held under attack (verified, don't re-litigate): ReDoS (all three regexes linear, timed to 350k chars); prose-restore fall-through keeps the sentinel on valid JSON; URI userinfo/scheme-less/encoded-key/fragment handling; part-model depth/budget; error-producer coverage; all concurrency fixes listed above.

Bottom line

This is close. The structural work — cancellation, lifecycle, schema, spans, redaction plumbing — is done and well-tested. What remains is one more iteration on _sanitize_sensitive_text's pattern table: fix the [REDACTED]-prefix hole and the backslash false-positive (those two are worth blocking on; the first is an exfil channel, the second blanks common production error rows), and consider the path-component and %3D gaps. Everything else is follow-up material.

[caohy1988]

Addressed round-3 review 4736064808 and pushed as 171db081.

Review item	Resolution
[REDACTED]SECRET suffix exfiltration	Fixed. The sentinel is idempotent only as the complete value; suffix variants including extra ] and / are consumed and redacted.
Backslash false positives	Fixed with a bounded, linear, detection-only canonicalizer for nested \\uXXXX, \\xHH, and %HH escapes. Safe Windows paths and JSON decode diagnostics remain byte-identical; encoded credential constructs still fail closed. The plain JSONDecodeError: Invalid \\escape example already passed before this patch, while the reproduced Windows-path/\\x5c cases are now covered.
Digest and multi-word authorization schemes	Fixed. Full Authorization and Proxy-Authorization values are redacted independently of scheme; Bearer and Basic matching also handles whitespace variants without blanking prose such as “bearer of bad news” or “a basic test”. Basic credentials are recognized with bounded strict Base64 validation.
URI path-component credentials	Fixed conservatively. Sensitive marker segments and their following value segment are redacted, while unrelated paths remain intact. Query and fragment handling retain their existing fail-closed behavior.
%3D and nested percent encoding	Fixed. Bounded recursive canonicalization exposes single- and double-encoded credential separators before matching.
Signature/key variants	Fixed at delimiter-aware boundaries for sig, AWS, Google signed-URL, API-key, and proxy-authorization names. This applies to free text and structured parser output; benign substrings such as design and signal are preserved.
Structured part keys	Hardened. Keys are sanitized, sensitive-key values are redacted, and rewritten-key collisions use the existing unique collision-marker allocation.
Invalidated publication transport cleanup	Tightened as a narrow follow-up: transport close now runs from a nested finally even if processor shutdown is cancelled.

The remaining review notes about immediate host-cancellation accounting, pre-budget model_dump(), and the long-term value of the explicit Python 3.10 regression remain documented P3 follow-ups, consistent with the review’s scope; none violates the current shutdown or privacy contracts.

Verification at 171db081:

• Full plugin suite: 433 passed / 6 skipped, with runtime warnings treated as errors.
• Focused round-3 regression class: 19 passed.
• Python 3.10 compile check and strict Base64 compatibility probes passed.
• pyink, isort, Ruff, and git diff --check are clean.
• Focused mypy remains at the same 35 upstream diagnostics; this patch adds none.

GitHub Actions is green across pre-commit, mypy on Python 3.10–3.13, unit tests on Python 3.10–3.14, and A2A v0.3 tests on Python 3.10–3.14. The PR is mergeable at this head.

[caohy1988]

Round-4 Review — commit 171db08 — LGTM ✅

Verified with two passes (adversarial sanitizer battery + test/regression verification), including a full suite run in an isolated worktree: 439/439 pass in 29.6s, matching the claimed local results. Every round-3 PoC was re-run against the real module.

Round-3 P2s — all closed or deliberately scoped

#	Finding	Status
1	[REDACTED]SECRET exfil	✅ Closed — exact-match exemption only; all suffix variants redact
2	Backslash false-positives	✅ Closed — Windows paths / Invalid \escape / \x5c SyntaxError byte-identical; access\u005ftoken, %3D, %253D, nested %255F still fail closed. Canonicalizer linear (1M chars / 193ms, no ReDoS)
3	Digest/multi-scheme auth	✅ Closed — full redaction to end-of-line incl. AWS4-HMAC-SHA256 Credential=...
4	URI path credentials	✅ Closed for labeled segments (incl. encoded/hyphenated labels); unlabeled high-entropy path tokens accepted as out-of-pattern scope
5	bearer\n / Basic auth	✅ Closed for padded/colon Basic and \n/\t bearers; prose FPs correctly preserved

All 6 new tests verified genuine — each was traced against the previous commit and fails on the old code. The invalidated-publication finally change is correct in all three control flows (success / Exception / CancelledError) with no silent-teardown path. Scope: clean, every hunk maps to a round-3 finding.

Two documented trade-offs to note (non-blocking, follow-up material)

1. Corrupt/truncated Basic tokens pass verbatim (:548-551) — Basic dXNlcjpwYXNzXJ9 fails base64 validation and is emitted unchanged, but the credential is recoverable (user:pass). Suggested follow-up: redact on validation failure when the blob is credential-length.
2. Path-segment redaction over-fires on benign endpoints (:2848-2861) — /oauth/token/refresh → /oauth/%5BREDACTED%5D/%5BREDACTED%5D. Availability trade-off, fail-closed direction; consider keeping the label segment and redacting only the successor in a follow-up.

Remaining P3s for the backlog: base64url charset gap, credential-as-query-key, [REDACTED] SECRET2 suffix exemption, "bearer bonds" prose FP, obs-fold continuation lines, redaction-not-flagged in truncation telemetry, exact-match key policy on part models, stale comment at :444-446.

Verdict

LGTM — merge. Three rounds of structural findings (privacy boundaries, schema compatibility, span hygiene, cancellation correctness across 3.10–3.14, lifecycle accounting, resource ownership) are all verified fixed with genuine regression tests. The sanitizer converged from an exfil-channel + mass-FP state to a tight pattern table with only edge-semantics trade-offs left, both documented above. Nothing outstanding blocks the PR's fail-closed, honest-shutdown, bounded-sanitization guarantees.

[caohy1988]

Upstream synchronization complete.

• Fast-forwarded caohy1988/main from be5828f3 to google/adk-python main at f1a0a148 (21 commits).
• Merged that upstream head into this PR without conflicts as aa5e46be; the PR head is 970498e5 after a no-content synchronization commit used to refresh CI against the updated base.
• Incorporated upstream's BQAA in-flight-write fix (6e43800f): flush() now waits on Queue.join(), and the tests use deterministic flush synchronization plus the platform thread helper.
• Combined local BQAA suite: 434 passed / 6 skipped with runtime warnings treated as errors.
• pyink, isort, Ruff, and git diff --check are clean; focused mypy remains at the same 35 existing diagnostics.
• Fresh CI against the synchronized f1a0a148 base is green: pre-commit, mypy on Python 3.10–3.13, unit tests on Python 3.10–3.14, and A2A v0.3 tests on Python 3.10–3.14.

The first post-merge run compared against the fork's old be5828f3 base and therefore reported three newly-upstream mypy diagnostics as PR regressions. Fast-forwarding the fork base and retriggering the run removed that baseline mismatch; no BQAA code change was needed.