Skip to content

feat(compliance): add capabilities_response_schema_invalid notice + capability_pointer field - #6256

Merged
bokelley merged 1 commit into
adcontextprotocol:mainfrom
EvgenyAndroid:partA
Aug 8, 2026
Merged

feat(compliance): add capabilities_response_schema_invalid notice + capability_pointer field#6256
bokelley merged 1 commit into
adcontextprotocol:mainfrom
EvgenyAndroid:partA

Conversation

@EvgenyAndroid

Copy link
Copy Markdown
Contributor

Part A of #6254. Adds the canonical notice code so a runner can name the root cause once, ahead of the track results, when a schema-invalid get_adcp_capabilities response would otherwise fan out into N unrelated-looking track failures.

Two documented repros, both costing hours of triage: #6242 (account block missing supported_billing, sandbox typed as an object — 10 steps failed across three tracks) and #6206 (one uncached context.correlation_id presented as five unrelated problems).

What's here

capabilities_response_schema_invalid in canonical_codes. severity: info, following input_schema_field_stripped — the closest peer, also info, also carrying no capability_path. The notes block keeps the Part B constraint explicit: the runner MUST still execute and report every storyboard it would otherwise have run. The notice explains the results, it does not replace them, and suppressing downstream failures would hide genuine defects behind a preflight line.

One notice per failing validation error, each with its own pointer, rather than one aggregate. The "affects N storyboards" count is omitted per the triage, since it needs a dry-run pass.

Per your instruction, the notes also scope the code away from #6206: a capabilities response that is schema-valid but stale or cached is a different condition and must not reuse this code.

capability_pointer (RFC 6901) as a new entry in optional_fields, per Option 2.

On the open question

The doc comment says a notice MAY carry both, because they answer different questions rather than being two spellings of one. capability_path names the capability flag that motivated the notice; capability_pointer locates a position in the response document. This new code often has no meaningful flag at all — a wrong type inside a block, or an array element — while the existing codes carry a flag with no validation pointer. "Exactly one" would force a false choice on leaf fields where both are genuinely available.

It also states that a consumer MUST NOT reject a notice for carrying only one, and that when both are present they MUST refer to the same location.

Scope

Documentation-only change to the conformance harness contract; no schema, no storyboard, no runner code. No changeset, per the triage. Part B (pre-execution validation in the @adcp/sdk runner that emits this notice) is a separate change in adcp-client.

Checked before filing: runner-output-contract.yaml is excluded from lint-universal-storyboard-doc-parity as a fixture, server/tests/unit/compliance-notices.test.ts asserts specific existing codes rather than enumerating them, and the file parses.

@EvgenyAndroid

Copy link
Copy Markdown
Contributor Author

Pushed a revision after a closer pass over the contract's own rules. Four changes worth calling out, three of which are corrections to what I first filed:

A changeset is required after all. scripts/check-changeset-protocol-scope.cjs matches ^static/compliance/source/ as protocol-scoped, so the Changeset Check job fails without one. Added as patch.

Contract version bumped 2.6.0 → 2.7.0. The file version-bumps on additive surface changes — the cascade_rules addition took it 2.0.0 → 2.1.0 and the notice block took it 2.1.0 → 2.2.0, while a change that only narrowed an existing detail explicitly left it alone. This adds one optional field and one canonical code, so it belongs in the first category.

The dedupe rule needed extending, or the design defeats itself. run_summary.notices says consumers dedupe by code. Since this emits one notice per failing validation error, all sharing a code, a conforming consumer would collapse them and discard every capability_pointer but one — exactly the per-location detail the field exists to carry. The rule now reads dedupe by code, or by (code, capability_pointer) when a pointer is present.

Loosened the both-present constraint. I had written that capability_path and capability_pointer MUST refer to the same location, which would have outlawed the case that motivated the field: signed_requests_specialism_deprecated carries capability_path: specialisms for the whole array, and your /specialisms/0 example is a position inside it. It now reads that the pointer MUST resolve to the node named by the path or to a position within it.

Also moved the entry to the end of canonical_codes to follow registration order rather than prepending ahead of the first-day trio, and trimmed the field comment closer to the one-liner you asked for.

On severity: info — worth flagging since the gloss reads "advisory context only", which sits oddly next to a message that says "fix this first". input_schema_field_stripped is the precedent: also info, also reports a defect the runner worked around. Minting a fourth severity felt out of scope for Part A, but say the word if you'd rather have one.

@bokelley

bokelley commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

All four corrections look right against the contract:

  • Changeset + version bump — the script gate is correct, and 2.7.0 is consistent with the contract's own versioning precedent (additive surface = minor bump to the internal version, patch to the package since conformance harness additions are patch-eligible per the repo's rules).
  • Composite dedupe key — the original code-only dedup would silently collapse all per-pointer notices into one, which defeats the entire purpose of the field. (code, capability_pointer) is the right key.
  • Loosened constraint — "pointer MUST resolve to the node named by the path or to a position within it" correctly handles signed_requests_specialism_deprecated (path: specialisms, pointer: /specialisms/0). The strict same-location reading would have outlawed the motivating case.

On severity: info: keep it, don't mint a fourth level for Part A. "Advisory context only" describes what the runner does, not what the reader urgency is — the runner continues, executes all storyboards, and reports everything it would have reported regardless of this notice. The message_template's "fix this first" is a reader interpretation directive, not a runner control-flow signal. The runner is never advisory context only about stopping. input_schema_field_stripped is the exact right precedent: also info, also reports a defect the runner worked around, also implies you should fix it before trusting results downstream.

A fourth severity would be warranted if the severity value changes runner behavior (abort on critical, downgrade track on warning, continue on info) — but that's a different contract design and an RFC of its own. Nothing in Part A or Part B calls for differentiated runner paths by severity, so adding the level now would be premature and would expand the contract surface without a consumer need to match it.


Generated by Claude Code

@EvgenyAndroid

Copy link
Copy Markdown
Contributor Author

Two things here need you, and neither is fixable from my side.

code_review fails on an actor-permission check — "Actor does not have write permissions to the repository" — so the reviewer step never writes its findings file and the job fails closed by design. It's actor-scoped rather than transient: the same job succeeded on #6245 this morning. Re-pushing won't clear it. Either the action needs to tolerate fork PRs from non-write actors, or my account needs whatever allowlist entry other outside contributors have.

Second, the sign-off above was a comment rather than a review, so reviewDecision is still REVIEW_REQUIRED and the gated-path rule holds. If you're happy with the four corrections, a formal approve closes it out.

Everything else is green: 22 of 23, with the changeset and the 2.6.0 → 2.7.0 contract bump in.

bokelley
bokelley previously approved these changes Aug 7, 2026

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four corrections verified correct in the prior thread:

  • Changeset + 2.7.0 bump — gate is accurate; additive surface change correctly maps to minor contract version.
  • Composite dedupe key — (code, capability_pointer) is the right key; code-only dedup would collapse all per-pointer notices into one, defeating the field's purpose.
  • Loosened co-present constraint — "pointer MUST resolve to the node named by the path or to a position within it" correctly handles signed_requests_specialism_deprecated (path: specialisms, pointer: /specialisms/0). The strict same-location reading would have outlawed the motivating case.
  • Registration order — entry moved to end of canonical_codes; correct per convention.

capability_pointer design and capabilities_response_schema_invalid at severity: info are both sound. Formal approval closes the review gate already cleared in comment form.


Generated by Claude Code

@bokelley

bokelley commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Formal review submitted — approval closes the review gate.

On code_review: the failure is actor-scoped, not commit-scoped. The job writes a findings file and fails closed when the GitHub Actions token lacks write access to the repository — which is the case for all fork PRs from contributors without repo write access. The same job passes on PRs from write-access accounts (#6245) because those tokens can write; re-pushing or amending the commit won't change the token's permissions.

Fixing it requires a .github/workflows change — for example, wrapping the findings-file write step in an if: github.event.pull_request.head.repo.full_name == github.repository guard, or restructuring the job so the write-requiring step runs in a privileged context only when the actor has access. Both paths require editing the workflow file, which is outside what this routine can touch. @bokelley would need to land a workflow patch separately, or grant @EvgenyAndroid write access to the repo. The failure is in the CI infrastructure, not in this diff.


Generated by Claude Code

…apability_pointer

Part A of adcontextprotocol#6254. A schema-invalid get_adcp_capabilities response currently
surfaces as N independent track failures with no indication they share a
root cause; two documented repros (adcontextprotocol#6242, adcontextprotocol#6206) cost hours of triage each.

Adds the canonical notice code capabilities_response_schema_invalid so a
runner can name the root cause once, ahead of the track results, without
suppressing any of them.

Adds capability_pointer (RFC 6901) as a new optional notice field rather
than overloading capability_path, which is documented as a dotted path
naming the capability flag. Schema validators emit RFC 6901 natively, the
dotted translation is lossy for array positions, and RFC 6901 is already
first-class here via validation_result.json_pointer.

Because one notice is emitted per failing validation error, all sharing a
code, the run_summary.notices dedupe rule is extended to dedupe by
(code, capability_pointer) when a pointer is present — otherwise a
conforming consumer would collapse them and discard every location but one.

Contract version 2.6.0 -> 2.7.0 (additive surface: one optional field, one
canonical code), matching how cascade_rules and the notices block bumped it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@EvgenyAndroid

Copy link
Copy Markdown
Contributor Author

Rebased onto main — #6276 took the same runner-output-contract.yaml version line this PR was bumping (it went 2.7.0 → 2.8.0 while this had 2.6.0 → 2.7.0), so the only conflict was that one line. Resolved by renumbering to 2.9.0; everything else applied cleanly.

Re-verified after the rebase: capabilities_response_schema_invalid is still the last entry in canonical_codes, capability_pointer is still in the notice optional_fields alongside capability_path, and the composite dedupe rule is intact. Your new fixture_unavailable skip reason and the oauth_metadata_graph check kind coexist with it — no overlap.

The force-push dismissed the approval, so this needs a re-approve when you get a moment. Nothing changed in the diff apart from the version number.

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving post-rebase. The only change from the 08-06 approved diff is the version line (2.7.0 → 2.9.0 to follow #6276's 2.8.0 bump); all four corrections are intact — changeset, composite dedupe key by (code, capability_pointer), loosened both-present constraint (pointer MUST resolve to the node named by the path or a position within it), and end-of-list entry order.

capability_pointer (Option 2, RFC 6901) is the confirmed field shape. severity: info stays per the prior reasoning — the runner always continues, input_schema_field_stripped is the right precedent.

code_review CI failure is actor-scoped (fork PR without write access), not a diff issue.


Generated by Claude Code

@bokelley
bokelley merged commit f672367 into adcontextprotocol:main Aug 8, 2026
23 of 24 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.

2 participants