py/aio.api.bazel: Code review notes

[Copilot]

Deep second-pass code-quality review of py/aio.api.bazel, following the initial packaging cleanup PR. Adds a single new file — no source changes.

py/aio.api.bazel/REVIEW.md

Structured findings across 12 categories with GitHub permalinks, effort/risk ratings, and 10 prioritised follow-up PRs. Top correctness issues identified:

• cwd silently dropped in ABazelRun.run — parameter accepted, never forwarded to subproc_run
• bazel_startup_options omitted from bazel run (present in query_command, absent in run)
• _dump always emits exit_code=0 — worker protocol reports success even on handler exceptions (# TODO: add error handling left unresolved)
• Non-persistent worker is a silent no-op — exits 0 without processing anything (# TODO: implement one-shot op)
• Async cancellation not propagated — subprocess.run in a thread-pool executor orphans Bazel processes on task cancel; asyncio.create_subprocess_exec would fix this natively
• Empty query output returns [""] not [] — "".split("\n") behaviour unguarded
• Zero logging — no subprocess invocations, return codes, or Bazel stderr captured in the logger

Additional findings cover: ABazel/ABazelCommand hierarchy that earns no abstraction, four empty concrete classes in bazel.py, type-annotation gaps (list vs list[str], untyped *args/**kwargs, unjustified # type:ignore), test cases that mirror broken implementation (no assertion that cwd or startup options are forwarded), copy-paste docstring on ABazelEnv.run, and a one-sentence README.

Original prompt

Deep code-quality review of py/aio.api.bazel

Deliverable: a single PR that adds one new markdown file containing a structured code-quality / code-smell report for py/aio.api.bazel. No code changes — report only.

Where to put the file

py/aio.api.bazel/REVIEW.md

What the report must cover

This is a deeper second-pass review that follows the in-flight initial cleanup PR (packaging metadata fixes including name = aio_api_bazel → aio.api.bazel and the bogus url = https://bazel.com/..., lower dep bounds, mechanical Py3.12+ typing/syntax modernisation). Explicitly skip anything already covered by that initial pass.

Investigate and categorise findings under at least these headings (omit a heading if you genuinely find nothing, but be thorough across every module and test file):

1. Architectural / API-surface smells — class hierarchies that don't earn their abstraction, leaky abstractions, public API that should be private, modules doing too much / too little.
2. Bazel subprocess correctness — shell=True, unsanitised arguments, missing cwd discipline, output capture/streaming correctness, exit-code handling, signal/cancel propagation, environment leakage.
3. Bazel worker lifecycle — startup/shutdown ordering, zombie processes, stdout/stderr framing for the worker protocol, partial-message handling, deadlocks on shutdown.
4. Async correctness — missing await, accidental coroutine objects, unbounded concurrency, sync I/O on the event loop, blocking subprocess calls inside async functions, async-for-async's-sake.
5. Query / output parsing — bazel query / cquery / aquery output parsing edge cases, malformed-output handling, escaping/quoting bugs, locale assumptions.
6. Caching / memoisation — @cached_property / functools.cache usage, cache-key bugs, caches that don't actually cache or grow unboundedly.
7. Error handling — bare except:, swallowed exceptions, raise … from None hiding root causes, missing context in re-raises.
8. Logging / observability — print calls, inconsistent log levels, f-string-formatted logs vs %s lazy formatting, swallowed bazel stderr.
9. Type-annotation correctness (beyond mechanical 3.12 syntax) — Any-soup, missing return annotations on public functions, structural types that should be Protocols, # type: ignore without justification.
10. Testing smells — over-mocking of subprocess/asyncio internals, tests mirroring implementation 1:1, missing edge-case coverage (bazel exit codes, malformed output, signals), assert_called_once_with overuse, slow/flaky tests.
11. Dead / duplicated / commented-out code — commented-out lines, obvious copy-paste, unused private helpers, files imported by nothing.
12. Documentation — missing/stale docstrings on public API, README.rst accuracy.

Format of the report

Use this structure (markdown):

# `aio.api.bazel` — code review

_Generated <date>. Follow-up to the initial packaging cleanup pass._

## Summary

<one-paragraph high-level take>

## Findings

### 1. Architectural / API-surface smells

#### 1.1 <short title>
- **Where**: `path/to/file.py:L10-L42` (full GitHub permalink to the commit SHA you reviewed)
- **What**: <concise description>
- **Why it matters**: <impact>
- **Suggested fix**: <concrete, actionable>
- **Effort**: S / M / L
- **Risk**: low / medium / high

(etc. for each heading)

## Recommended follow-up PRs

Prioritised list of self-contained follow-up PRs, each with bundled finding ids, effort and risk. Aim for 5–10.

Rules

• Every finding must include a GitHub permalink with line anchors (#L10-L42) pointing at the commit SHA reviewed.
• Be concrete, not vague.
• Group genuine nits under "Minor nits" subsections rather than separate entries.
• Do not modify any source files — diff is exactly one new markdown file.
• PR title: py/aio.api.bazel: Code review notes.
• PR description: one-paragraph summary plus a link to the new file.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: @copilot Accepted Confirmation: Confirm agent session

User: @copilot Accepted Confirmation: Confirm agent session

User: @copilot Accepted Confirmation: Confirm agent session

Assistant: All three are now dispatched.

data:
    - title: '[WIP] py/`envoy.distribution.release`: Packaging cleanups'
      repository: envoyproxy/toolshed
      url: https://github.com/copilot/tasks/pull/PR_kwDOF71pj87aD823?session_id=f7d17044-04b1-4c95-baef-fd32d71cfa25
      state: queued
    - title: '[WIP] py/`envoy.docs.sphinx_runner`: Packaging cleanups'
      repository: envoyproxy/toolshed
      url: https://github.com/copilot/tasks/pull/PR_kwDOF71pj87aD88x?session_id=30e6fa00-c126-4a80-aca5-c3b0757e3647
      state: queued
    - title: '[WIP] py/`envoy.ci.report`: P...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

[netlify]

✅ Deploy Preview for nifty-bassi-e26446 ready!

Name	Link
🔨 Latest commit	18e2ab0
🔍 Latest deploy log	https://app.netlify.com/projects/nifty-bassi-e26446/deploys/6a011b811b368c0008cf241d
😎 Deploy Preview	https://deploy-preview-4425--nifty-bassi-e26446.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.