Skip to content

feat(api): canonical-label enum, options namespace, 0.3.0 bump - #19

Merged
jstjoe merged 3 commits into
mainfrom
jstjoe/api-reference-docs
May 13, 2026
Merged

jstjoe merged 3 commits into
mainfrom
jstjoe/api-reference-docs

Conversation

@jstjoe

@jstjoe jstjoe commented May 13, 2026

Copy link
Copy Markdown
Owner

API surface improvements driven by new-consumer DX:

  • categories is now a closed CanonicalLabel enum mirroring opf_eval.taxonomy.CANONICAL_LABELS exactly. Clients (and codegen) see the 15 valid values in the spec, and bad values fail at the pydantic layer with a structured 422 instead of a custom 400.
  • Detector-specific options moved off the top level into a options.<detector> namespace. Today only options.opf is defined (carrying decode_mode), with extra="forbid" so typos like decod_mode surface as 422. New detector options can be added additively without re-shaping the top-level request.
  • schema_version dropped from every response. The API is pre-1.0; info.version is now the single source of truth and bumps to 0.3.0 for this breaking shape change. /v1/ prefix stays reserved for the eventual 1.0 cutover.
  • Routes: removed the dead _validate_categories helper (pydantic handles it) and the unused decode_mode no-op branch. Added a _ERROR_422_EXAMPLE with concrete bad_category and unknown_opf_option payloads so generated docs show what a validation failure looks like.
  • Guides + spec regenerated; tests updated (422 for bad enum input
    • new test for extra=forbid on OpfOptions).

Caveat: per-request decode_mode is still advisory — OPFDetector sets decode mode at construction time from OPF_DECODE_MODE. The options namespace reserves the shape for future per-call wiring.

API surface improvements driven by new-consumer DX:

- `categories` is now a closed `CanonicalLabel` enum mirroring
  `opf_eval.taxonomy.CANONICAL_LABELS` exactly. Clients (and codegen)
  see the 15 valid values in the spec, and bad values fail at the
  pydantic layer with a structured 422 instead of a custom 400.
- Detector-specific options moved off the top level into a
  `options.<detector>` namespace. Today only `options.opf` is defined
  (carrying `decode_mode`), with `extra="forbid"` so typos like
  `decod_mode` surface as 422. New detector options can be added
  additively without re-shaping the top-level request.
- `schema_version` dropped from every response. The API is pre-1.0;
  `info.version` is now the single source of truth and bumps to
  `0.3.0` for this breaking shape change. `/v1/` prefix stays
  reserved for the eventual 1.0 cutover.
- Routes: removed the dead `_validate_categories` helper (pydantic
  handles it) and the unused `decode_mode` no-op branch. Added a
  `_ERROR_422_EXAMPLE` with concrete `bad_category` and
  `unknown_opf_option` payloads so generated docs show what a
  validation failure looks like.
- Guides + spec regenerated; tests updated (422 for bad enum input
  + new test for `extra=forbid` on `OpfOptions`).

Caveat: per-request `decode_mode` is still advisory — `OPFDetector`
sets decode mode at construction time from `OPF_DECODE_MODE`. The
options namespace reserves the shape for future per-call wiring.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jstjoe

jstjoe commented May 13, 2026

Copy link
Copy Markdown
Owner Author

Hey @claude could you review please?

@claude

claude Bot commented May 13, 2026

Copy link
Copy Markdown

Claude finished @jstjoe's task in 1m 36s —— View job


PR Review

  • Gather context and read changed files
  • Review schemas.py
  • Review routes.py
  • Review main.py
  • Review tests
  • Review OpenAPI spec / docs changes
  • Post final review

Overall this is a clean, well-motivated set of changes. The shift to a CanonicalLabel enum with an import-time assertion against the taxonomy, and the options.<detector> namespace with extra="forbid", are both good API design. A few things worth looking at before merge:


Bug: stale route description

api/src/opf_api/routes.py:167

"OPF-only: pass `decode_mode` to override the default Viterbi decoding."

decode_mode has moved to options.opf.decode_mode but this description string wasn't updated. It'll show verbatim in generated docs and be misleading to consumers. Should read something like "OPF-only: pass \options.opf.decode_mode` to override the default Viterbi decoding."` Fix this →


Subtle behavior change: categories=[]

api/src/opf_api/routes.py:150

if body.categories:
    allow = set(body.categories)
    spans = [s for s in spans if s["label"] in allow]

The old _validate_categories returned None only when categories is None; passing an empty list would return an empty set, filtering all spans to nothing. The new if body.categories: is falsy for [], so categories=[] now silently returns all spans instead of none.

This is almost certainly the friendlier behavior (an empty filter list as "no filter" makes more intuitive sense for callers), but it's an unintentional semantic change rather than a deliberate one, and there's no test covering it. Worth either adding a test asserting the current behavior, or a categories is not None guard with a comment explaining the empty-list choice. Fix this →


DetectorOptions silently accepts unknown top-level keys

api/src/opf_api/schemas.py:68

DetectorOptions has no model_config (so extra defaults to "ignore"). A client sending {"options": {"gliner": {...}, "typo_detector": "foo"}} gets no validation error. This is intentional per the docstring ("other keys are accepted but ignored"), but it has a future-proofing tradeoff: when you add a second detector's options field (e.g. gliner: GlinerOptions | None), a client already sending options.gliner would silently pass through until you add the field — but a client with a typo of the new field would also silently pass through. The current design is fine for a pre-1.0 API; just flagging it as a conscious choice vs. extra="forbid" on DetectorOptions too.


Minor: missing happy-path test for valid options.opf

The test suite now covers the typo-rejection case (decod_mode) but has no test sending a valid options.opf.decode_mode value and asserting it's accepted with 200. Since decode_mode is currently advisory, this won't catch a regression in the field-accepted path. Low-priority but easy to add. Fix this →


Nit: DetectorInfo.categories typing inconsistency

api/src/opf_api/schemas.py:318

categories: list[str] = Field(...)

Request fields use list[CanonicalLabel] but this response field remains list[str]. Since these come from detector_categories() in the registry rather than from user input, the str type is technically correct and there's no pydantic validation issue. Just a minor consistency note — not a blocker.


What's good

  • The import-time assert in schemas.py:43 is exactly the right way to enforce taxonomy alignment — fails loudly at startup if the enum drifts.
  • OpfOptions with extra="forbid" and the _ERROR_422_EXAMPLE block are solid DX work.
  • Removing schema_version from every response (and app.state.schema_version) simplifies the contract meaningfully; info.version as single source of truth is cleaner.
  • Test for extra_forbidden is well-structured with the explicit type and loc assertion.
  • The SanitizeRequest inheriting from DetectRequest means the new validation applies to both endpoints without duplication — good.

Summary: Two things worth addressing before merge — the stale decode_mode description in the route doc string, and a test/comment to nail down the categories=[] edge-case intent. The rest are observations or nits.

- routes.py: stale `decode_mode` mention in `/v1/detect` description now
  points at `options.opf.decode_mode`. The string is rendered verbatim
  in Scalar/Swagger/ReDoc and would have misled consumers.
- schemas.py: `DetectorInfo.categories` typed as `list[CanonicalLabel]`
  to match the request side. Source is `detector_categories()` which
  already returns canonical labels, so this is a pure spec-tightening.
- tests: add a happy-path test sending `options.opf.decode_mode` and
  asserting 200, alongside the existing `extra_forbidden` typo case.

DetectorOptions intentionally keeps `extra="ignore"` so clients can
carry forward-compat blobs across detector swaps. The tradeoff
(typo on a future top-level options key passes silently until the
field is added) is acceptable pre-1.0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jstjoe

jstjoe commented May 13, 2026

Copy link
Copy Markdown
Owner Author

Hey @claude re-review?

@claude

claude Bot commented May 13, 2026

Copy link
Copy Markdown

Claude finished @jstjoe's task in 59s —— View job


Re-review

  • Check routes.py — stale description fix
  • Check routes.py — categories=[] behavior
  • Check schemas.py — DetectorOptions + DetectorInfo consistency
  • Check tests — happy-path for valid options.opf

The fix commit (9314d2d) addresses three of the four items from the first review. Here's the updated status:


Resolved since first review

Stale route description ✅ — routes.py:167 now reads options.opf.decode_mode. Fixed.

Happy-path test for options.opf ✅ — test_detect_valid_opf_options_accepted (test_routes.py:181) sends a valid decode_mode and asserts 200. Covers the field-accepted path.

DetectorInfo.categories typing ✅ — schemas.py:318 now list[CanonicalLabel], consistent with the request side. The commit message notes that detector_categories() already returns canonical labels, so this is a pure spec-tightening with no runtime change.

DetectorOptions extra=ignore ✅ — The fix commit's message explicitly calls out the tradeoff ("acceptable pre-1.0"). This is documented intent, not an oversight.


Still open

categories=[] behaviorroutes.py:150 still uses if body.categories:, meaning an explicit empty list is treated identically to None (no filtering). The existing comment on lines 151–153 explains the coercion mechanics but not the empty-list intent. No test covers it. This isn't wrong, but it's the one remaining undocumented semantic choice from the original review.

The fix is low-effort: either add a one-liner test asserting that categories=[] returns all spans (documents the current behavior as intentional), or change the guard to if body.categories is not None: and add a test for the zero-result case if "empty list = no spans" is actually what you want. Fix this →


Summary: Everything actionable from the first review is done except the categories=[] edge case. That's a minor documentation/test gap — call it a pre-merge nit rather than a blocker. The core API shape (enum, options namespace, 422 examples) is solid.

Previously `if body.categories:` collapsed `None` and `[]` to the same
"no filter" behavior. Strict semantic is clearer and composes better:

- `null` / field omitted → keep every category the detector produces.
- `[]` → deliberate "match nothing" filter, returns zero spans.

Switch the guard to `is not None` and document the contract on the
field description. Two new tests pin both branches.

Reasoning for strict: clients building `categories` dynamically (e.g.
`[c for c in wanted if c in known]`) expect narrowing to win. Silently
widening an accidentally-empty filter back to "everything" is the more
surprising default. Pre-1.0; reverse is easy if this turns out wrong.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jstjoe
jstjoe merged commit d76a5e9 into main May 13, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant