docs(bdd): annotate UC-019 SERVICE_UNAVAILABLE wire-code rationale#1656
docs(bdd): annotate UC-019 SERVICE_UNAVAILABLE wire-code rationale#1656mkostromin-sigma wants to merge 3 commits into
Conversation
Rebase noteRebased onto current Comment-only |
4c3f7ba to
dce49c9
Compare
|
Rebased onto current |
KonstantinMirin
left a comment
There was a problem hiding this comment.
Review — PR #1656
Overview — The factual claim these annotations add is correct: INTERNAL_ERROR is internal-only (src/core/exceptions.py INTERNAL_CODES, and ERROR_CODE_MAPPING["INTERNAL_ERROR"] = "SERVICE_UNAVAILABLE") and does collapse to SERVICE_UNAVAILABLE on the wire, and AdCP 3.1.1 is the correct pinned version (docs/adcp-spec-version.md). The headline for the fixes is that the annotation reads as an authoritative, spec-verified coverage note but isn't grounded like one: it decorates scenarios that never execute, it cites a derived internal mapping instead of the spec enum, and it repeats itself twice inside each Scenario Outline.
Should fix
Annotation asserts wire coverage the BDD scenarios don't actually provide.
The three BR-RULE-294 rehydration scenarios this PR annotates (@T-UC-019-partition-targeting-rehydration, @T-UC-019-inv-294-3, @T-UC-019-boundary-targeting-overlay) are dormant: their distinctive steps — And package "pkg-001" persisted package_config has …, And response.errors[] should include an entry with code "SERVICE_UNAVAILABLE", And that errors[] entry field selector should be … — have no definitions under tests/bdd/steps/, so pytest-bdd raises StepDefinitionNotFoundError and tests/bdd/conftest.py converts each to xfail before any assertion runs. The only thing that actually grades the SERVICE_UNAVAILABLE-on-the-wire behavior is the model-level unit test tests/unit/test_get_media_buys.py. So the diff pins a wire fact with a comment that presents as verified while no live scenario exercises it across the four transports. Wire the BR-RULE-294 steps through dispatch_request and assert via ctx["result"].assert_wire_error("SERVICE_UNAVAILABLE", …); until they're live, mark the annotation explicitly as unit-graded / ungraded-on-wire rather than leaving it reading as spec-verified coverage.
Rationale grounded in a derived internal artifact instead of the spec authority.
The purpose of this comment is to be the authoritative citation for why the wire code is SERVICE_UNAVAILABLE, but it grounds that in src/core/exceptions.py INTERNAL_CODES — a salesagent-internal mapping that is downstream of the spec, not the source of truth. The authority is the pinned AdCP 3.1.1 error-code enum (dist/schemas/…/enums/error-code.json: SERVICE_UNAVAILABLE present, INTERNAL_ERROR absent). The two sibling correction annotations in this same file already model the right form:
# existing (L394 / L521):
# CORRECTED to AdCP 3.1 enums/media-buy-status.json @ v3.1-04f59d2d5: …
# this PR (x5):
# corrected: INTERNAL_ERROR is internal-only (src/core/exceptions.py INTERNAL_CODES)
# and collapses to SERVICE_UNAVAILABLE on the wire — AdCP 3.1.1
Cite the enum (enums/error-code.json @ 3.1.1) as the reason INTERNAL_ERROR is off-wire and SERVICE_UNAVAILABLE is the collapse target; keep the src/core/exceptions.py pointer as the implementation of that spec fact, not its justification. Matching the sibling CORRECTED to … form also resolves the casing/format drift.
Same rationale block placed twice inside each Scenario Outline.
For both outlines the identical two-line block sits once above the Scenario Outline keyword and again above that outline's Examples: block (L935-936 + L946-947 for the partition outline; L1212-1213 + L1222-1223 for the boundary outline). The standalone INV-3 scenario correctly carries one copy. Two of the five insertions are pure within-outline redundancy that will drift the moment the rationale is refined at one placement and not the other. Keep one copy per scenario — the one above the Scenario Outline keyword — and drop the copy above each Examples: block.
Nice to have
- The pre-existing
# @source repo=adcp ref=v3.1-04f59d2d5 commit=04f59d2d5 …anchor sits two lines below the refreshed rationale on these same scenarios and still points at a bare commit anchor rather than the pinned3.1.1. Since this PR is re-grounding exactly these scenarios, refreshing the adjacent anchor in the same pass keeps the citation self-consistent. Optional and pre-existing.
Notes / prior-review follow-ups
- No prior maintainer review points to re-raise — the only prior activity is @mkostromin-sigma's rebase notes (2026-07-16). Nothing was left unaddressed.
- The wire-collapse fact and the
AdCP 3.1.1version label are both correct and verified againstsrc/core/exceptions.pyanddocs/adcp-spec-version.md; the fixes above are about how the claim is grounded and placed, not whether it's true.
Record why rehydration TypeError expects SERVICE_UNAVAILABLE (INTERNAL_ERROR is internal-only) so regeneration merges keep the rationale from prebid#1649 review.
Cite error-code.json as authority, drop redundant Examples copies, and mark INV-3 as unit-graded until BDD errors[] steps are wired.
dce49c9 to
fe7fcd5
Compare
Security Audit / pip-audit fail on mcp 1.27.2 (GHSA-vj7q-gjh5-988w).
|
Addressed all four review threads in
|
Summary
SERVICE_UNAVAILABLEis the wire code (INTERNAL_ERRORis internal-only / absent from AdCP 3.1.1enums/error-code.json).tests/bdd/features/BR-UC-019-query-media-buys.feature, grounded in the enum;INTERNAL_CODESnoted as implementation.errors[]steps are wired.mcp>=1.28.1for CVE-2026-59950 / GHSA-vj7q-gjh5-988w (pip-audit/ Security Audit). Canonical dep landing also in fix(ci): pre-start E2E stack, free BDD disk, harden pinact/IPR #1669.Closes #1655.
Test plan
.featurefile