generator validation requires manual `sid` field, contradicting Rust struct emitter (which auto-injects it) #85

New issue

Closed

opened 2026-05-20 07:53:43 +00:00 by timur · 1 comment

timur commented

2026-05-20 07:53:43 +00:00

Owner

Problem

crates/generator/src/generator.rs::validate_root_object rejects any root object that does not explicitly declare a sid field in the schema:

// crates/generator/src/generator.rs:170-177
let has_sid = obj.fields.iter().any(|f| f.name == "sid");
if !has_sid {
    return Err(GeneratorError::ValidationError(format!(
        "Root object '{}' must have a 'sid' field",
        name
    )));
}

But the Rust struct emitter (crates/generator/src/rust/rust_struct.rs:368-389) auto-injects sid: SmartId, created_at: u64, and updated_at: u64 for every root object — and silently skips any schema-declared field with those names:

if obj.is_root_object {
    output.push_str("    pub sid: SmartId,\n");
    output.push_str("    pub created_at: u64,\n");
    output.push_str("    pub updated_at: u64,\n");
}

for field in &obj.fields {
    if obj.is_root_object
        && (field.name == "sid" || field.name == "created_at" || field.name == "updated_at")
    {
        continue;
    }

The parser (crates/oschema/src/parser.rs:655-661) detects root-object-ness from either a sid field or a [rootobject] comment marker — so the [rootobject] marker should be sufficient.

Three pieces of code in disagreement:

Component	Behavior
Parser	`[rootobject]` marker → root object; `sid` field also accepted as marker
Generator validator	`sid` field REQUIRED — fails otherwise
Rust struct emitter	Always auto-injects `sid`/`created_at`/`updated_at`; ignores any schema declarations

The validator is the odd one out. Result: authors are forced to write a misleading sid: str line (which the emitter ignores) just to get past validation. That line then turns into stale guidance in hero_skills#275 (the skill says root objects MUST have a sid: str field — which is only true because of this validator, not because the generated code needs it).

Fix

Drop the must have a 'sid' field check. Root-object-ness is determined by the [rootobject] marker. The sid (and created_at, updated_at) fields are an implementation detail of code generation, not the schema.

Keep:

must have at least one user-defined field (still useful).

Drop:

must have a 'sid' field.

Acceptance

A schema like the following compiles cleanly:

# A thing tracked by the catalog [rootobject]
Thing = {
    name: str @index
    description: str
}

Generated types_generated.rs still has pub sid: SmartId, pub created_at: u64, pub updated_at: u64 on Thing (auto-injected, unchanged).
Existing schemas that do declare sid: str still build (no regression).

Refs

Downstream of: hero_skills#275 — once this lands, that skill change can drop the "MUST have a sid: str field" rule honestly.
Discovered while implementing hero_service#1 (catalog schema collapse blocked by this validator).

## Problem `crates/generator/src/generator.rs::validate_root_object` rejects any root object that does not explicitly declare a `sid` field in the schema: ```rust // crates/generator/src/generator.rs:170-177 let has_sid = obj.fields.iter().any(|f| f.name == "sid"); if !has_sid { return Err(GeneratorError::ValidationError(format!( "Root object '{}' must have a 'sid' field", name ))); } ``` But the Rust struct emitter ([`crates/generator/src/rust/rust_struct.rs:368-389`](https://forge.ourworld.tf/lhumina_code/hero_rpc/src/branch/development/crates/generator/src/rust/rust_struct.rs#L368)) **auto-injects** `sid: SmartId`, `created_at: u64`, and `updated_at: u64` for every root object — and **silently skips** any schema-declared field with those names: ```rust if obj.is_root_object { output.push_str(" pub sid: SmartId,\n"); output.push_str(" pub created_at: u64,\n"); output.push_str(" pub updated_at: u64,\n"); } for field in &obj.fields { if obj.is_root_object && (field.name == "sid" || field.name == "created_at" || field.name == "updated_at") { continue; } ``` The parser ([`crates/oschema/src/parser.rs:655-661`](https://forge.ourworld.tf/lhumina_code/hero_rpc/src/branch/development/crates/oschema/src/parser.rs#L655)) detects root-object-ness from **either** a `sid` field **or** a `[rootobject]` comment marker — so the `[rootobject]` marker should be sufficient. Three pieces of code in disagreement: | Component | Behavior | |---|---| | Parser | `[rootobject]` marker → root object; `sid` field also accepted as marker | | Generator validator | `sid` field REQUIRED — fails otherwise | | Rust struct emitter | Always auto-injects `sid`/`created_at`/`updated_at`; ignores any schema declarations | The validator is the odd one out. Result: authors are forced to write a misleading `sid: str` line (which the emitter ignores) just to get past validation. That line then turns into stale guidance in [hero_skills#275](https://forge.ourworld.tf/lhumina_code/hero_skills/issues/275) (the skill says root objects MUST have a `sid: str` field — which is only true because of *this* validator, not because the generated code needs it). ## Fix Drop the `must have a 'sid' field` check. Root-object-ness is determined by the `[rootobject]` marker. The `sid` (and `created_at`, `updated_at`) fields are an implementation detail of code generation, not the schema. Keep: - `must have at least one user-defined field` (still useful). Drop: - `must have a 'sid' field`. ## Acceptance - A schema like the following compiles cleanly: ```oschema # A thing tracked by the catalog [rootobject] Thing = { name: str @index description: str } ``` - Generated `types_generated.rs` still has `pub sid: SmartId`, `pub created_at: u64`, `pub updated_at: u64` on `Thing` (auto-injected, unchanged). - Existing schemas that *do* declare `sid: str` still build (no regression). ## Refs - Downstream of: [hero_skills#275](https://forge.ourworld.tf/lhumina_code/hero_skills/issues/275) — once this lands, that skill change can drop the "MUST have a `sid: str` field" rule honestly. - Discovered while implementing [hero_service#1](https://forge.ourworld.tf/lhumina_code/hero_service/issues/1) (catalog schema collapse blocked by this validator).

timur referenced this issue from a commit

2026-05-20 08:01:29 +00:00

fix(generator): drop redundant `sid`-field requirement on root objects (#85)

timur commented

2026-05-20 08:09:58 +00:00

Author

Owner

Branch pushed: issue-85-rootobj-sid-validation. Diff is one file:

crates/generator/src/generator.rs — drop the must have a 'sid' field check in validate_root_object; replace the test that pinned that wrong behavior with one that pins the new correct behavior.

All 125 generator tests pass. Used by hero_service#1 — that branch’s Cargo.toml temporarily pins hero_rpc to this branch.

Branch pushed: `issue-85-rootobj-sid-validation`. Diff is one file: - `crates/generator/src/generator.rs` — drop the `must have a 'sid' field` check in `validate_root_object`; replace the test that pinned that wrong behavior with one that pins the new correct behavior. All 125 generator tests pass. Used by [hero_service#1](https://forge.ourworld.tf/lhumina_code/hero_service/issues/1) — that branch’s Cargo.toml temporarily pins hero_rpc to this branch.

timur referenced this issue from lhumina_code/hero_service

2026-05-20 08:09:59 +00:00

catalog domain is over-modelled — collapse ServiceDefinition to name + description + interfaces #1

timur referenced this issue from a pull request that will close it,

2026-05-20 08:16:44 +00:00

Drop redundant 'sid' field requirement in root object validation (#85) #87

timur closed this issue

2026-05-20 08:16:47 +00:00

timur referenced this issue from a commit

2026-05-20 08:16:49 +00:00

Drop redundant 'sid' field requirement in root object validation (#85) (#87)

timur referenced this issue from lhumina_code/hero_skills