Skip to content

feat(codegen): propagate JSON Schema deprecated:true as // Deprecated: doc comment#135

Draft
bokelley wants to merge 5 commits into
mainfrom
claude/issue-134-codegen-deprecated-comment-v2
Draft

feat(codegen): propagate JSON Schema deprecated:true as // Deprecated: doc comment#135
bokelley wants to merge 5 commits into
mainfrom
claude/issue-134-codegen-deprecated-comment-v2

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Refs #134

Modifies adcp/schemas/generate.py so that JSON Schema properties with deprecated: true produce a preceding standalone // Deprecated: <description> comment on the generated Go struct field. The preceding-line form is required by Go's documentation comment convention (go.dev/doc/comment §Deprecated) for gopls, go doc, and IDE hover to recognise the deprecation — a trailing inline comment (the existing approach for description text) is not recognised.

No changes to types_gen.go: the current 3.0 bundle has no deprecated: true properties. The generated output will update automatically when the AdCP 3.1 schema bundle lands (download.sh + generate.py re-run, see adcontextprotocol/adcp#4904).

What changed

adcp/schemas/generate.py (8 lines changed in schema_to_struct):

  • When prop.get('deprecated', False) is true, emit // Deprecated: <description> on a line preceding the field declaration, and suppress the duplicate trailing inline description comment.
  • Strip a redundant leading "Deprecated: " prefix (case-insensitive) from description before using it as the message, to prevent // Deprecated: Deprecated: ... if a schema author sets both deprecated: true and starts the description with "Deprecated: " (the existing axe_include_segment pattern in the 3.0 bundle, lines 2004–2005 of types_gen.go, illustrates this authoring pattern).
  • Fallback message when no description is present: "No replacement specified.".

adcp/schemas/test_generate.py (new, 156 lines): Python unittest for schema_to_struct. 7 test cases: basic deprecated field, fallback message, non-deprecated unchanged, no-description, mixed struct regression, double-prefix strip, case-insensitive strip.

adcp/generator_test.go (new, 26 lines): Go test that shells out to python3 -m unittest discover so the Python tests run when cd adcp && go test ./... is executed. Python is already a build-time dependency of this module (CI invokes generate.py and lint.py).

What was tested

  • python3 -m unittest discover -s adcp/schemas -p 'test_*.py' -v → 7/7 pass
  • cd adcp && go test ./... → all pass (including TestGeneratorDeprecatedPropagation)
  • go build ./... (root module) → clean
  • go vet ./... (root module) → clean
  • cd adcp/schemas && python3 lint.py → no drift (no schema in 3.0 bundle has deprecated: true, so types_gen.go is unchanged)

⚠️ Pre-existing CI gap (adcp/ module, not introduced by this PR): The adcp/ sub-module (adcp/go.mod) has no dedicated CI step in .github/workflows/ci.yml. All 6+ existing test files in adcp/ (e.g. testcontroller_test.go, addtool_test.go, seller_test.go) run only via cd adcp && go test ./... locally. A follow-up CI step analogous to the existing Test context agent / Test bench module steps is needed:

- name: Test adcp module
  working-directory: adcp
  run: go test -race -count=1 ./...

Pre-PR review

  • code-reviewer: approved — re.sub double-prefix fix is correct; CI gap is pre-existing infrastructure debt for all adcp/ tests, not a logic issue; nit (doc comment clarity) addressed
  • ad-tech-protocol-expert: approved — double-prefix blocker resolved; JSON Schema deprecated → Go // Deprecated: preceding-line mapping is correct per go.dev/doc/comment; back-compat clean on 3.0 bundle
  • security-reviewer: approved — no TEE/pinhole surface; safe_comment() sanitization already covers the new deprecated_msg path; no new injection surface

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_01TXU3btrUh956CDjk3rm8sL


Generated by Claude Code

claude added 3 commits May 21, 2026 16:49
…: doc comment

For struct fields where the source JSON Schema property declares
`deprecated: true`, generate.py now emits a preceding standalone
`// Deprecated: <description>` line rather than a trailing inline comment.
The preceding-line form is required by Go's documentation comment convention
(go.dev/doc/comment) for the deprecation to be recognised by gopls and go doc.

Also adds adcp/schemas/test_generate.py (Python unittest) with 5 test cases
covering deprecated, non-deprecated, and mixed-struct scenarios, and
adcp/generator_test.go which shells out to run those Python tests under
`go test ./...` so CI coverage does not require a separate workflow step.

No types_gen.go changes: no current schema in the 3.0 bundle has
`deprecated: true`. The generated output will update automatically when
the AdCP 3.1 schema bundle lands and `download.sh` + `generate.py` are
re-run (adcontextprotocol/adcp#4904).

Refs #134

https://claude.ai/code/session_01TXU3btrUh956CDjk3rm8sL
…recated: doc comment

Strip redundant leading "Deprecated: " prefix from description before
using it as the // Deprecated: message, preventing double-prefix output
("// Deprecated: Deprecated: ...") when schema authors set both
deprecated:true and start the description with "Deprecated: ".
Stripping is case-insensitive. Two new test cases cover the
already-prefixed and lowercase-prefix variants.

Refs #134

https://claude.ai/code/session_01TXU3btrUh956CDjk3rm8sL
…recated: doc comment

Clarify generator_test.go doc comment: the adcp/ module has no CI step
(pre-existing gap). Replace misleading "CI does not require a separate
workflow step" with an accurate note and follow-up pointer.

Refs #134

https://claude.ai/code/session_01TXU3btrUh956CDjk3rm8sL
@bokelley
Copy link
Copy Markdown
Contributor Author

Fold suggestion from triage of #151: two small fixes naturally belong in this diff before merge rather than in a separate follow-up PR.

Item 1 — generate.py path resolution (generate.py:396-397): replace root = Path.cwd().resolve() with root = SCRIPT_DIR (add SCRIPT_DIR = Path(__file__).resolve().parent at module top, matching what lint.py already does). Also fix bare relative paths in _will_generate_set's os.path.exists(rel) calls (~lines 503/506) and update the stale cwd reference in _reset_will_generate_cache's docstring. Security note: the SCRIPT_DIR-anchored bound is strictly tighter than the cwd-anchored bound for the directory-traversal guard in resolve_ref_schema.

Item 2 — TypeError in catch tuples (generate.py:404, 507, 516, 738): add TypeError to all four catch sites — a JSON Pointer walk into a scalar raises TypeError: string indices must be integers, which currently produces a traceback rather than a clean drift error.

Both changes are mechanical (no logic impact on the deprecated-annotation feature) and each is ~1–3 lines. Details in #151.


Triaged by Claude Code. Session: https://claude.ai/code/cse_01Y3MmzgEj2mxK4G7rN5NQyk


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

Fold candidate note from #180: that issue requests union-helper regression tests — broken UNION_SCHEMA_TYPES pointers fail coverage/generation instead of false-greening, shared helper pointers must have equivalent minItems semantics, and generated helpers reject null/empty-array wire shapes where minItems: 1. All of these would land in adcp/schemas/test_generate.py and adcp/generator_test.go, the two files this PR creates as net-new. Folding them here before merge avoids a conflict on both files and keeps generator test coverage in one pass. Scope is orthogonal to the deprecated-comment feature — treat as optional pre-merge opportunity.


Generated by Claude Code

claude added 2 commits May 26, 2026 17:46
…gn discovery pattern

- Merge TestDeprecatedField from test_generate.py into generate_test.py so all
  schema generator tests live in one file with a consistent *_test.py naming
  convention
- Remove test_generate.py (now superseded)
- Update generator_test.go to use *_test.py discovery pattern, matching the
  CI "Test schema generator" step exactly — previously test_generate.py was
  invisible to CI and generate_test.py (from #183) was invisible to the Go test

https://claude.ai/code/session_018Gmdum6TsUTwpA6SXHjLLv
@bokelley
Copy link
Copy Markdown
Contributor Author

The fold candidate from #180 is no longer needed — @bokelley landed the union helper tests directly in PR #183 (adcp/schemas/generate_test.py) which merged today.

However, the merge surfaced a test discovery gap: test_generate.py used the test_*.py naming convention (discoverable by the Go test in generator_test.go) while generate_test.py from #183 uses *_test.py (discoverable by the CI "Test schema generator" step). The two patterns are complementary and each file was invisible to the other runner.

Fixed in the latest push:

  • Merged TestDeprecatedField from test_generate.py into generate_test.py — all generator tests now live in one file.
  • Deleted test_generate.py.
  • Updated generator_test.go to use -p '*_test.py', matching the CI step exactly.

All 13 tests (7 deprecated-comment + 6 union-helper) pass under both python3 -m unittest discover -p '*_test.py' and go test ./....

Also merged the latest main (includes #179 and #183) into this branch — no conflicts.


Generated by Claude Code


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

Acknowledged. The test consolidation is clean: a single generate_test.py with a consistent *_test.py discovery pattern aligns Go and CI runner behavior, and the fold candidate from #180 is properly resolved via #183. No action needed from triage — PR is in good shape when you're ready to mark it ready for review.


_Generated by Claude Code


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants