chore: rewriter unit tests + drop misleading noqa on import inbox - #2440
Merged
HongmingWang-Rabbit merged 1 commit intoMay 1, 2026
Merged
Conversation
Two small follow-ups to the PR #2433 → #2436 → #2439 incident chain. 1) `import inbox # noqa: F401` in workspace/a2a_mcp_server.py was misleading — `inbox` IS used (at the bridge wiring inside main()). F401 means "imported but unused", which would mask a real future F401 if the usage is removed. Drop the noqa, keep the explanatory block comment about the rewriter's `import X` → `import mr.X as X` expansion (and the `import X as Y` → `import mr.X as X as Y` trap the comment exists to prevent re-introducing). 2) scripts/test_build_runtime_package.py — 17 unit tests covering `rewrite_imports()` and `build_import_rewriter()` in scripts/build_runtime_package.py. Until now the function had zero coverage despite the entire wheel build depending on it. Tests pin: bare-import aliasing, dotted-import preservation, indented imports, from-imports (simple + dotted + multi-symbol + block), the `import X as Y` rejection added in PR #2436 (with comment- stripping + indented + comma-not-alias edge cases), allowlist anchoring (`a2a` ≠ `a2a_tools`), and end-to-end reproduction of the PR #2433 failing pattern + the #2436 fix pattern. 3) Wire scripts/test_*.py into CI by adding a second discover pass to test-ops-scripts.yml. Top-level scripts/ tests live alongside their target file (parallels the scripts/ops/ test layout); the existing scripts/ops/ pass keeps running because scripts/ops/ has no __init__.py so a single discover from scripts/ root doesn't recurse. Two passes is simpler than retrofitting namespace packages. Path filter widened from `scripts/ops/**` to `scripts/**` so PRs touching the build script trigger the new tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
requested a review
from hongmingwang-moleculeai
as a code owner
May 1, 2026 03:46
HongmingWang-Rabbit
enabled auto-merge
May 1, 2026 03:46
HongmingWang-Rabbit
deleted the
chore/wheel-rewriter-tests-and-noqa-cleanup
branch
May 1, 2026 03:52
2 tasks
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
Serialized merge by gitea-merge-queue after current-main, genuine approvals, and required CI checks were green.
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
The status-pagination bug (RCA, #2440-family): merge/verify status readers fetched only the FIRST page of a commit's statuses. On high-churn PRs Gitea caps the combined GET /commits/{sha}/status `statuses` array at the default page size (~30) and pushes older-but-still-current required-context rows past it. A reader of that truncated view records the required context as ABSENT (missing) even though its current SUCCESS row exists — wrongly blocking, or mis-reading the gate. Confirmed on #2448/#2426/#2438/#2331/#2259/#2055/#2032 (reviewers had to manually paginate to verify gates this whole session). Live proof on PR #2331 head: combined /status returns 30 rows; exhaustive /statuses returns 50 rows across 20 distinct contexts. Two verify-by-state readers consumed that capped combined view for required-context decisions and are fixed here to page the dedicated /commits/{sha}/statuses list to EXHAUSTION (until a short/empty page), then collapse to newest-row-per-context: - prod-auto-deploy.py (wait-ci gate): replaced the single combined /status fetch with fetch_all_statuses() (paginated). A required context past page 1 no longer reads "missing" forever and times out a legitimate prod deploy. latest_status_for_context now selects newest-by-id so the oldest-first /statuses ordering can't let a stale run shadow the current one. - audit-force-merge.sh: replaced the single combined /status fetch with a page loop over /commits/{sha}/statuses, accumulating all rows before the newest-wins CHECK_STATE collapse. A required SUCCESS past the cap no longer reads "missing" and emits a false-positive incident.force_merge. gitea-merge-queue.py already paginates /statuses to exhaustion (get_combined_status + api_paginated) — left unchanged; it is the reference behavior this change brings the other two readers in line with. STRENGTHENING ONLY — fail-closed preserved, NO fail-open path introduced: - prod-auto-deploy: a genuinely-absent required context appears on NO page, so ci_context_state() still returns "missing", context_is_satisfied() rejects it, and the gate never greens (times out). Any page that errors or is not a list raises (fetch_all_statuses/_api_json_list) — a partial list never passes as complete. - audit-force-merge: any non-200 page or non-array body aborts with exit 1; an absent required context has no CHECK_STATE entry so `${...:-missing}` keeps it not-green and the audit still fires. Tests (mutation-resistant): added regressions that (a) place a required SUCCESS on page 2+ behind a full page of churn and assert the reader FINDS it, and (b) make a required context genuinely absent on all pages and assert the reader STILL fail-closes (missing/never-satisfied → blocks/times out). Mocks the paginated HTTP responses. Also locks newest-wins collapse, short-page stop, full-page continue, and page-error propagation. Refs: status-pagination RCA, #2440-family. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two small follow-ups to the PR #2433 → #2436 → #2439 incident chain. Pure polish — neither change affects runtime behavior.
1. Drop misleading `# noqa: F401`
`import inbox # noqa: F401` in `workspace/a2a_mcp_server.py` was incorrect — `inbox` is used at the bridge wiring inside `main()`. F401 means "imported but unused"; suppressing it on a used import would mask a future genuine F401 if the usage is ever removed. Drop the noqa, keep the explanatory comment about the rewriter expansion (the actual reason the import was lifted to module scope in PR #2436).
2. Unit tests for `rewrite_imports()`
Until now the wheel-build rewriter had zero test coverage despite the entire wheel build depending on it. `scripts/test_build_runtime_package.py` adds 17 tests covering:
3. CI wiring
`test-ops-scripts.yml` gets a second `unittest discover` pass: one from `scripts/` (top-level tests like the new one) and one from `scripts/ops/` (existing). Path filter widens from `scripts/ops/` to `scripts/` so PRs touching the build script trigger the new tests. Two passes is simpler than retrofitting namespace packages.
Test plan
🤖 Generated with Claude Code