chore(dx): align local typecheck workflow#884
Conversation
There was a problem hiding this comment.
LGTM. Follow-ups noted below. The mypy warn_unreachable + strict_equality_for_none flip is the right shape — load-bearing dead branches caught at compile time beats defensive is None in every getter.
Things I checked
capabilities.py:215-226— removedif self._caps.adcp is None: return False. Safe.adcpis required (non-Optional) onGetAdcpCapabilitiesResponsepertypes/generated_poc/protocol/get_adcp_capabilities_response.py:1363.warn_unreachablewas right.proposal_dispatch.pynewmaybe_validate_refine_proposal_refs(51 LoC). Skipsaction="finalize"entries (maybe_intercept_finalizehandles those); usesexpected_account_id=ctx.account.idso cross-tenant probes collapse to the samePROPOSAL_NOT_FOUNDas missing-record.repr()-formattedproposal_idin the message bounds log-injection from a buyer-controlled id. Wired intohandler.get_productsbefore thehas_refine_supportcheck. Right shape.PROPOSAL_NOT_FOUNDrecovery downgradeterminal→correctable. Aligned atproposal_lifecycle.py:114,proposal_dispatch.py:208, and the new helper. Cross-tenant probe and missing-record both return identicalNonefrom the in-memory store and the PG store — no principal-enumeration discriminator introduced.- Server-side decisioning-error shim refactor (
server/serve.py:2371,server/translate.py:117,server/a2a_server.py:50).DecisioningAdcpError = Nonereplaced with_DECISIONING_ADCP_ERROR_TYPES: tuple[...].isinstance(x, ())returns False — preserved semantics.protocols/mcp.py:42does the same forHTTPStatusError. Clean. scripts/check_type_ignore_contract.py(46 LoC). Walks COMMENT tokens only, so docstring mentions of "type: ignore" don't trip it.comment.startswith("type: ignore")catches both bare andtype: ignore[code]forms — intentional and matches the no-suppression contract.- Public Protocol widening:
BuyerAgentRegistry.resolve_by_credentialacceptsCredential(now includesHttpSigCredential). Framework dispatch atdecisioning/handler.pywalls HttpSig off toresolve_by_agent_urlso the bearer-resolution path is never invoked with an HttpSig credential at runtime — no signature-verification bypass.security-reviewer: no High, no Brian-blocker. adopter-type-ignore-contracthook + CI step. Two checks look duplicate but aren't:mypy --strictenforces correctness, the script enforces no suppressions on top. Complementary.
Follow-ups (non-blocking — file as issues)
recovery="terminal"still intry_reserve_consumptionforPROPOSAL_NOT_FOUND.decisioning/proposal_store.py:562-564(in-memory) anddecisioning/pg/proposal_store.py:691-693(PG) still emitterminalfor the same error code that's nowcorrectableon the pre-check path. Theenforce_proposal_expiryprecheck atproposal_dispatch.pyshadows these on the happy path, but a record evicted between expiry-check and lock-acquisition surfaces the staleterminalrecovery. Bring both store sites in line with the new posture so the wire contract is uniform._credential_keydoesn't handleHttpSigCredential.decisioning/registry_cache.py:152-163dispatches onApiKeyCredential/OAuthCredentialand falls intounknown:HttpSigCredentialfor the third arm. Unreachable today —handler.pynever callsresolve_by_credentialwith HttpSig — but the Protocol now publicly advertises the broader signature without a corresponding cache key. Add an explicitHttpSigCredential → f"http_sig:{keyid}:{agent_url}"branch so the next caller refactor can't quietly create a cache-collision bug.- PR title vs release-please. This SDK uses release-please with
changelog-sectionslimited tofeat|fix|perf|revert|deps.e63b384a fix(storyboards): align examples with 3.1 runneris the wire-visible recovery change — if this PR squash-merges underchore(dx), the fix gets buried with no version bump or changelog entry. Either preserve the per-commit history at merge or retitle to surface thefix:semantic. pre-commitis only in[dependency-groups].dev(PEP 735), not[project.optional-dependencies].dev. A contributor following the legacypip install -e ".[dev]"path (still a documented target in the Makefile asinstall-dev) will not get pre-commit. Either move it into the project extras so both paths work, or change theinstall-devhelp string to flag that pre-commit is uv-only.
Minor nits (non-blocking)
webhooks.py:1074-1089: boundary fail-open softening.validate_webhook_destination_urlpreviously mapped non-str/non-AnyUrlinputs tourl_text = None→reason="missing_url". Now they hitstr(...)and fall through to scheme/SSRF checks, which still reject — but the error category becomesinvalid_schemeinstead of the more accuratemissing_url. Type-boundary fail-closed beats trusting downstream guards here.scripts/check_type_ignore_contract.py:startswith("type: ignore")over-matches. Catches a straytype: ignoredtypo as if it were a real ignore. Tighten tocomment == "type: ignore" or comment.startswith("type: ignore[").scripts/check_type_ignore_contract.py: unhandledtokenize.TokenizeError. A future syntax error intests/type_checks/crashes the CI step instead of pointing at the bad file.pyproject.toml:126:mypy==1.20.2pin is unexplained. Neighboring comment at:317-323explains why mypy is duplicated across[project.optional-dependencies]and[dependency-groups]but not why this exact patch. One-line rationale would help the next reader who tries to bump it.CONTRIBUTING.md:make bootstrapadvertises one step but does two. The commit-msg hook install is invisible to a contributor debugging a missing hook. One trailing parenthetical — "(installs pre-commit + commit-msg hooks)" — fixes it.tests/test_proposal_lifecycle_e2e.py: per-testfrom adcp.types import .... Three new tests do the import inside the function body; move to the module-level import block for consistency with the rest of the file.
Three commits, three conventional-commit flavors, one PR title — worth a passing thought before merge.
…ping # Conflicts: # examples/seller_agent.py # src/adcp/decisioning/proposal_lifecycle.py # tests/test_proposal_lifecycle_e2e.py
There was a problem hiding this comment.
Two real things to address before approve. The DX/bootstrap work is clean and the new refine guard's cross-tenant scoping is right.
Things I checked
make bootstrap+make typecheck-all+scripts/check_type_ignore_contract.pylock down the adopter type-check fixture contract. The script walkstests/type_checks/withtokenizeand fails on any# type: ignore. Right shape, and pre-commit + CI both wire it.maybe_validate_refine_proposal_refsatsrc/adcp/decisioning/proposal_dispatch.py:340-388threadsexpected_account_id=ctx.account.idintostore.get. Cross-tenant probes collapse toPROPOSAL_NOT_FOUNDper the principal-enumeration contract atproposal_lifecycle.py:68-74. Verified in bothInMemoryProposalStore.getandPgProposalStore.get— single dict-lookup / single SELECT, no second-hop timing side-channel.security-reviewer: not a ship-blocker.- Mypy guard removals:
capabilities.py:218-222(_caps.adcpnon-Optional in schema),server/responses.py:85(preceding branch raises),signing/brand_jwks.py:436(_innernon-None at:423),validation/oneof_hints.py(callers filter to dicts upstream) — all correctly dead branches. - Tuple-of-types refactor in
protocols/mcp.py,server/a2a_server.py:50-53,server/serve.py:2371-2375,server/translate.py:119-122is semantically equivalent.isinstance(exc, ())returns False;cast(Any, exc)intranslate.py:144is needed because mypy can't narrow through a variable-typed tuple. - Pinning
mypy==1.20.2in both[dev]extras and[tool.uv] devis the right call — anything looser reproduces the CI/local error-count split the comment atpyproject.toml:321was already worried about.
Issues
-
_part_data_dictregression insrc/adcp/server/a2a_server.py:139-142(and mirrored insrc/adcp/protocols/a2a.py:39-44).part.dataisgoogle.protobuf.Value, notStruct— see_make_data_part:166-170which builds it viaParseDict(data, Value()).MessageToDicton aValuereturns whatever JSON kind it holds:None, bool, float, str, list, or dict. The pre-PRisinstance(value, dict)guard caught non-struct kinds and returned None so the parser fell through to TextPart parsing. Post-PR, a peer sendingPart(data=Value(string_value=...))returns astrfrom a function annotated-> dict[str, Any] | None. Caller ata2a_server.py:424doesdata.get("skill")and raisesAttributeError._parse_requestis called at:269, before thetryat:289, so the exception escapes the structuredadcp_errorenvelope. Happy path is unchanged; adversarial path is now a 500. Restore the guard, or change the signature toAnyand add isinstance at the callsites.code-reviewerflagged this as a Blocker — downgrading because it's not a happy-path crash, but it should still be fixed before ship. -
Neither new test actually exercises
maybe_validate_refine_proposal_refs.tests/test_proposal_lifecycle_e2e.py:280-305usesaction: \"finalize\"— the new guard explicitly skips finalize (proposal_dispatch.py:362-364). The error comes from the pre-existingmaybe_intercept_finalize.tests/test_proposal_lifecycle_e2e.py:374-400callscreate_media_buy, but the new guard is wired intoget_productsonly (handler.py:1804). The error comes fromproposal_lifecycle.enforce_proposal_expiry. The pre-existingtest_refine_unknown_proposal_is_correctable_not_found:198does land on the new code path, but the PR doesn't make that explicit. Add a direct test withaction=\"include\"oraction=\"omit\"and an unknownproposal_idagainstget_products, asserting the manager'srefinemethod was not called.
Follow-ups (non-blocking — file as issues)
BuyerAgentRegistry.resolve_by_credentialwidened fromApiKeyCredential | OAuthCredentialtoCredentialacrossregistry.py:219-220,registry_cache.py:334/532/634,pg/buyer_agent_registry.py:266.BuyerAgentRegistryis a Protocol surface adopters implement. PEP 544 contravariance accepts broader impls, but adopter registries that kept the narrower annotation will now fail Liskov against the published Protocol under strict mypy. Pre-1.0bump-minor-pre-major: trueinrelease-please-config.json:7absorbs the semver impact, but a CHANGELOG line orBREAKING CHANGE:footer would give adopters the breadcrumb.ad-tech-protocol-expert: sole real adopter-breaking change in the diff.refine[]atget_products_request.py:172-178hasmin_length=1and nomax_length. The new guard does one sequentialawait store.get(...)perproposal-scoped entry. Per_size_limit.py:44(10 MB cap), an attacker can pack ~170k entries per request → ~170k sequential DB roundtrips. Fix belongs at the schema layer, not in this PR.webhook_supervisor.py:324andwebhook_supervisor_pg.py:213widenedsender: WebhookSender→WebhookSender | None, but both constructors raiseValueErroron None at the next few lines. Type lies in the other direction — adopters reading the signature will think None is legal. Drop the runtime check or keep the narrower type.BuyerAgent.status: BuyerAgentStatus | stratregistry.py:158is a runtime no-op (BuyerAgentStatusis aLiteral[...], runtime isstr) but worth a one-line CHANGELOG note for adopters that do exhaustive narrowing.
Minor nits (non-blocking)
handler.py:1336._resolve_account(ref: AccountReference | None, ...)→ref: object | None. Internal method, low impact, butAccountReference | Nonewas more honest — if mypy was the friction, acastat the offending callsite would have kept the public type lie out of the signature.- Commit-set-vs-diff mismatch. Title is
chore(dx): align local typecheck workflow. The diff also adds a new decisioning guard (maybe_validate_refine_proposal_refs), widens a Protocol method, and adds acorrectablePROPOSAL_NOT_FOUND test surface. A separatefix(decisioning):commit would have landed those in the right CHANGELOG bucket instead of getting swept underchore(dx). Notable, given the type-pin in this same PR comments specifically about CI/local drift.
LGTM after (1) is fixed and (2) is either re-tested or the existing coverage is called out explicitly. Once the parsing path is robust again and the new guard has a test that actually targets it, this is safe to merge.
There was a problem hiding this comment.
Clean DX cleanup. The warn_unreachable premise pays off — most of the removals are provably dead, and the type-widening sites are paired with runtime guards.
Things I checked
warn_unreachableremovals are sound at every site.GetAdcpCapabilitiesResponse.adcpis required persrc/adcp/types/generated_poc/protocol/get_adcp_capabilities_response.py:1363, so thesupports_v3short-circuit atcapabilities.py:221-222was dead._normalize_capabilities_adcp_versionalready raises atresponses.py:79-84so the redundantis not Noneat line 85 is dead.validation/oneof_hints.pypre-filters at line 228 so the per-helper guards were dead. The_X_TYPES: tuple[...] = ()swap pattern acrossmcp.py,a2a_server.py,serve.py,translate.pyis functionally identical —isinstance(exc, ())returnsFalseper Python docs.maybe_validate_refine_proposal_refs(src/adcp/decisioning/proposal_dispatch.py:340-388) — same cross-tenant collapse asenforce_proposal_expiry,expected_account_id=ctx.account.idcomes from the authenticated principal, error envelope (PROPOSAL_NOT_FOUND/correctable/refine[i].proposal_id) matchesmaybe_intercept_finalize. Skippingaction == "finalize"is correct because finalize is owned by the dedicated path. The refine action enum atget_products_request.py:77-80confirmsinclude/omit/more_like_this/finalizeare the only proposal-scope values — no draft-creating action that would legitimately reference an unborn id.- A2A scalar
datafallback atprotocols/a2a.py:38-43andserver/a2a_server.py:138-143is aligned;test_part_data_dict_ignores_scalar_value_payloads(tests/test_protocols.py:102) andtest_execute_skips_scalar_datapart_and_uses_text_fallback(tests/test_a2a_server.py:247) cover both layers. - Mypy pin. Both
pyproject.tomlsites aremypy==1.20.2. Consistent. - Conventional-commit signal.
chore(dx)is right for the PR scope; the new validator and A2A fix arrived via merge from main with their ownfix(storyboards)/fix(a2a)prefixes. No breaking public-API surface — widenings only. scripts/check_type_ignore_contract.py.tokenize.tokenize(handle.readline)on a binary handle honors encoding declarations and BOMs; non-tokenizable files fail loud; mypy's# type: ignorerecognition is ASCII-whitespace only so unicode bypasses don't help either side.
Follow-ups (non-blocking — file as issues)
-
HttpSigCredentialcollides to one cache/rate-limit bucket in_credential_key.src/adcp/decisioning/registry_cache.py:152-163. TheCredentialwidening makesHttpSigCredentialreachable on theresolve_by_credentialProtocol and the_CachingRegistry/_RateLimitedRegistry/_AuditingRegistrywrappers, but_credential_keystill only branches onApiKeyCredential/OAuthCredentialand falls back tounknown:HttpSigCredentialfor everything else. Framework dispatch athandler.py:565keepsHttpSigCredentialoff this path today, so it's not exploitable as-shipped — but the static guard mypy used to give us is gone. Either add an explicitHttpSigCredentialbranch (f\"http_sig:{kid}:{agent_url}\") or convert theunknown:fallback to aTypeErrorso future variants fail closed. -
validate_webhook_destination_urlmislabels non-string inputs asinvalid_schemeinstead ofmissing_url.src/adcp/webhooks.py:1073-1080. The newelse: url_text = str(url)letsNonestringify to\"None\", which passes the truthy check and tripsurlsplitdownstream. No SSRF bypass — the resolver still runs — but a public registration-time validator loses error fidelity for adopters reaching it throughfrom adcp.webhooks import .... Restore the explicitelse: url_text = None, or tighten withisinstance(url, (str, AnyUrl)). -
BuyerAgent.status: BuyerAgentStatus | strcollapses tostr.src/adcp/decisioning/registry.py:158. Adopters pattern-matching the Literal exhaustively lose mypy coverage. Dispatch athandler.py:611+defaults toPERMISSION_DENIEDon unknown values so runtime behavior is right, but the widening at the public type could be narrower — raw-string adopter return + framework-sidecastwould preserve the Literal at the boundary. -
WebhookSender | Noneconstructor widening.src/adcp/webhook_supervisor.py:324,webhook_supervisor_pg.py:213. Trades a type-check error for a runtimeValueError. Either keepWebhookSenderand remove the runtime guard, or document the call path that warranted the widening. -
AccountReference | None→object | None.src/adcp/decisioning/handler.py:1336. Internal helper, runtime cast already existed, but the docstring still references:class:\AccountReference`` and is now inconsistent with the annotation.
Minor nits (non-blocking)
list(...)no-op inmaybe_validate_refine_proposal_refs.src/adcp/decisioning/proposal_dispatch.py:357— Pydantic exposesrefineaslist | None; the wrap clones for no reason.enforce_proposal_expiryiterates lazily._build_domain_index/AuthorizationContext.__init__widened tolist[Any].src/adcp/adagents.py:1313, 1936— the publicAuthorizationContextconstructor loses self-documenting precision adopters were reading off the signature. Keeplist[Any]on the underscore-prefixed helper if the cleanup is load-bearing, but the public constructor could stay atlist[dict[str, Any]].brand_jwks.py:436is concurrency-neutral, not safer. The new directawait self._inner(kid)is fine under sequential reasoning, but a racingforce_refresh()settingself._inner = Nonebetween line 433's await and line 436's read would AttributeError. The pre-PRelse Noneguard was equally racy (TOCTOU), so this isn't a regression — but if you're already in there, an explicit re-check after the cascade refresh would close the window.
LGTM. Follow-ups noted below.
Summary
make bootstrapsetup path for uv-managed dev dependencies and hooksmake typecheck-allso source mypy, adopter type fixtures, and the no-ignore contract run togethertests/type_checks/fixtures do not use real# type: ignoresuppressionswarn_unreachableandstrict_equality_for_none, with small type-boundary cleanups needed to make those checks passWhy
The repo already had strong mypy settings, but local docs, Make targets, pre-commit, and CI were teaching slightly different workflows. This makes the contributor path and the adopter type-check contract explicit and executable.
The follow-up type cleanup turns the useful stricter mypy probe into an enforced project setting instead of a one-off advisory command.
The 3.1 storyboard runner now exercises fixed-price product paths, viewability totals, and correctable recovery for unknown proposal IDs. The reference examples and proposal guards now match that contract while keeping cross-tenant proposal lookups collapsed into
PROPOSAL_NOT_FOUND.Validation
make ci-localADCP_RUNNER_BIN=adcp ADCP_PORT=3102 STORYBOARD_RESULT_PATH=.context/storyboard-result-adcp-3.1-rerun.json SELLER_LOG_PATH=.context/reference-seller-adcp-3.1-rerun.log scripts/ci/run_storyboard_reference_seller.shADCP_SDK_VERSION=adcp-3.0 ADCP_PORT=3103 STORYBOARD_RESULT_PATH=.context/storyboard-result-adcp-3.0-rerun.json SELLER_LOG_PATH=.context/reference-seller-adcp-3.0-rerun.log scripts/ci/run_storyboard_reference_seller.shadcp storyboard run http://127.0.0.1:3107/mcp media_buy_seller/proposal_finalize --json --allow-httpruffand pytest checks while iterating on the stricter mypy cleanupuv run --extra dev pre-commit run adopter-type-checks --all-filesuv run --extra dev pre-commit run adopter-type-ignore-contract --all-filesuv run --extra dev pre-commit run check-yaml --all-filesuv run --extra dev black --check scripts/check_type_ignore_contract.pygit diff --check