[oschema generator] service-block method after a section comment inherits the next method's description (off-by-one) #153

New issue

Closed

opened 2026-06-14 17:49:12 +00:00 by sameh-farouk · 2 comments

sameh-farouk commented

2026-06-14 17:49:12 +00:00

Member

The openrpc_server! / oschema->OpenRPC generator mis-assigns method descriptions by one when a service-block method is declared immediately after a section comment: the method inherits the next method's description.

Evidence (hero_planner): 90_rpc.oschema is correct — ping() -> bool # liveness check; returns true if the server is responsive and system_health() -> str # returns "ok"; .... But the regenerated openrpc_main.json documents ping with system_health's text ('returns "ok"...') and system_counts with ping's text ('liveness check...'). The SDK client doc-comments are fine — only the OpenRPC JSON (which drives the API Docs pane) is shifted. The shift is localized to the system block where ping was inserted after a comment line.

Fix at the generator (do NOT hand-edit the generated json): the oschema parser should not let a leading section comment cause a description off-by-one. Interim workaround: add a blank line / reorder so a method isn't directly after a section comment, then a clean regen re-aligns (verify ping shows 'liveness check…' in /api-docs-pane).

Cross-cutting: any migrated service with a commented service block can hit this — worth checking hero_collab during its migration. Found via hero_planner.

The `openrpc_server!` / oschema->OpenRPC generator **mis-assigns method descriptions by one** when a `service`-block method is declared immediately after a section comment: the method inherits the *next* method's description. **Evidence (hero_planner):** `90_rpc.oschema` is correct — `ping() -> bool # liveness check; returns true if the server is responsive` and `system_health() -> str # returns "ok"; ...`. But the regenerated `openrpc_main.json` documents `ping` with system_health's text ('returns "ok"...') and `system_counts` with ping's text ('liveness check...'). The SDK client doc-comments are fine — only the OpenRPC JSON (which drives the API Docs pane) is shifted. The shift is localized to the system block where `ping` was inserted after a comment line. **Fix at the generator** (do NOT hand-edit the generated json): the oschema parser should not let a leading section comment cause a description off-by-one. **Interim workaround:** add a blank line / reorder so a method isn't directly after a section comment, then a clean regen re-aligns (verify `ping` shows 'liveness check…' in `/api-docs-pane`). **Cross-cutting:** any migrated service with a commented service block can hit this — worth checking hero_collab during its migration. Found via hero_planner.

sameh-farouk commented

2026-06-14 20:06:49 +00:00

Author

Member

Triage (verified on-box) — difficulty: S, breaking: no

Root cause located. parse_rpc_method (crates/oschema/src/oschema/parser.rs:703/716/723) calls skip_comments() after parsing the return type / error clause / @sse but before collect_trailing_comment() (line 727). skip_comments pushes every contiguous comment — including the method's own same-line trailing # description — into pending_comments as a leading comment, so the next method claims it via take_leading_comments(). Classic off-by-one. The struct/field and union parsers already guard against exactly this (parser.rs:961 / :1197).

Fix (~3–9 lines, 1 file, no API change): collect the trailing comment into method.comments before each post-clause skip_comments(), mirroring the field/union parsers. Do not hand-edit the generated JSON — the parser fix regenerates correct output on the next build.

Verified on-box: applied → oschema lib 269 + integ 59, macros openrpc_server_test 7 + openrpc_from_oschema_test 8 all pass; the shift disappears (e.g. planner ping stops showing system_health's text).

Not breaking: rebuilt hero_planner_server against the fixed generator — compiles; regenerated openrpc_main.json changed only the description field (14 methods corrected), method names/params/results/components byte-identical. SDK docs derive from summary (= method name), not description, so SDKs regenerate identical. Each consumer picks it up on its next hero_lib bump; no coordinated cut needed.

Reach: ~50 repos have .oschema service blocks; 7 carry the literal trigger shape today (hero_planner, hero_proc, hero_osis, hero_ledger, hero_sync_webdav, zinit_archive, mycelium_portal_archived).

Residual (separate, cosmetic): a leading section-banner comment (e.g. ── system ──) still attaches to the first method after it — that's normal leading-comment behavior, not this off-by-one. Optional follow-up via markers.rs::is_marker_only_comment.

## Triage (verified on-box) — difficulty: **S**, breaking: **no** **Root cause located.** `parse_rpc_method` (`crates/oschema/src/oschema/parser.rs:703/716/723`) calls `skip_comments()` after parsing the return type / error clause / `@sse` but **before** `collect_trailing_comment()` (line 727). `skip_comments` pushes every contiguous comment — including the method's own same-line trailing `# description` — into `pending_comments` as a *leading* comment, so the next method claims it via `take_leading_comments()`. Classic off-by-one. The struct/field and union parsers already guard against exactly this (`parser.rs:961` / `:1197`). **Fix (~3–9 lines, 1 file, no API change):** collect the trailing comment into `method.comments` *before* each post-clause `skip_comments()`, mirroring the field/union parsers. Do **not** hand-edit the generated JSON — the parser fix regenerates correct output on the next build. **Verified on-box:** applied → oschema lib 269 + integ 59, macros `openrpc_server_test` 7 + `openrpc_from_oschema_test` 8 all pass; the shift disappears (e.g. planner `ping` stops showing `system_health`'s text). **Not breaking:** rebuilt `hero_planner_server` against the fixed generator — compiles; regenerated `openrpc_main.json` changed **only the `description` field** (14 methods corrected), method names/params/results/components **byte-identical**. SDK docs derive from `summary` (= method name), not `description`, so SDKs regenerate identical. Each consumer picks it up on its next `hero_lib` bump; no coordinated cut needed. **Reach:** ~50 repos have `.oschema` service blocks; 7 carry the literal trigger shape today (hero_planner, hero_proc, hero_osis, hero_ledger, hero_sync_webdav, zinit_archive, mycelium_portal_archived). **Residual (separate, cosmetic):** a leading section-banner comment (e.g. `── system ──`) still attaches to the first method after it — that's normal leading-comment behavior, not this off-by-one. Optional follow-up via `markers.rs::is_marker_only_comment`.

sameh-farouk referenced this issue from a commit

2026-06-14 20:42:10 +00:00

fix(oschema): keep a method/enum trailing comment on its own node (#153)

sameh-farouk referenced this issue from a commit

2026-06-14 20:50:51 +00:00

fix(oschema): keep a method/enum trailing comment on its own node (#153)

sameh-farouk closed this issue

2026-06-14 20:50:51 +00:00

sameh-farouk commented

2026-06-14 20:51:18 +00:00

Author

Member

Fixed on development (squash-merged).

Root cause: skip_comments (in parse_rpc_method, parse_union_type, and the bare-@sse path of parse_sse_annotation) converted a method/enum same-line trailing comment into a leading comment for the next node — shifting every description by one.

Fix: added skip_comments_preserving_trailing() and used it at every clause/variant boundary so the owning node collects its own trailing comment via collect_trailing_comment (mirrors parse_field). The logic delta is ~1 helper + swapping ~7 boundary skip-calls + removing one now-redundant collect; the remaining ~111 of 121 added lines are regression tests. No parsing-structure/AST change.

Verified: RED→GREEN with 2 regression tests (return/error/void/@sse methods + typed enums); herolib_oschema lib 270/0, integration 59/0. A/B regen of planner's real schema (pristine parser vs fixed parser, same hero_lib otherwise) changed only description fields — 28 corrected; method names/params/results/components byte-identical (e.g. ping now reads its own "liveness check…" instead of system_health's text).

Residual (separate, pre-existing cosmetic): the decorative ── section ── banner still prefixes the first method of each section. Tracked as a follow-up via markers.rs::is_marker_only_comment — not part of this off-by-one.

Fixed on `development` (squash-merged). **Root cause:** `skip_comments` (in `parse_rpc_method`, `parse_union_type`, and the bare-`@sse` path of `parse_sse_annotation`) converted a method/enum same-line *trailing* comment into a *leading* comment for the next node — shifting every description by one. **Fix:** added `skip_comments_preserving_trailing()` and used it at every clause/variant boundary so the owning node collects its own trailing comment via `collect_trailing_comment` (mirrors `parse_field`). The logic delta is ~1 helper + swapping ~7 boundary skip-calls + removing one now-redundant collect; the remaining ~111 of 121 added lines are regression tests. No parsing-structure/AST change. **Verified:** RED→GREEN with 2 regression tests (return/error/void/`@sse` methods + typed enums); `herolib_oschema` lib 270/0, integration 59/0. A/B regen of planner's real schema (pristine parser vs fixed parser, same hero_lib otherwise) changed **only `description` fields — 28 corrected; method names/params/results/components byte-identical** (e.g. `ping` now reads its own "liveness check…" instead of `system_health`'s text). **Residual (separate, pre-existing cosmetic):** the decorative `── section ──` banner still prefixes the first method of each section. Tracked as a follow-up via `markers.rs::is_marker_only_comment` — not part of this off-by-one.

sameh-farouk referenced this issue

2026-06-14 20:53:15 +00:00

[oschema/macros] pre-existing test failures on development: 4 base36 doctests (unresolved herolib_sid) + 7 openrpc_rhai_test (ErrorFunctionNotFound) #154