fix(ogar-vocab): repair EnterEffect placement after #13 (main doesn't compile)#15
Conversation
PR #13 introduced two placement bugs that left main not compiling. Bug 1 (~L302-332): EnterEffect was inserted between ActionDef's derive attributes and its struct body. The attrs (#[derive(Default, ...)] + #[cfg_attr serde] + #[non_exhaustive]) attached to EnterEffect (which ALSO has its own derives -> E0119 conflicting impls x6), and ActionDef was left with no attrs (-> ActionDef: Default not satisfied). Fix: relocate the EnterEffect doc/attrs/struct/impl block to AFTER ActionDef's closing brace, so ActionDef's original 3 attrs are again adjacent to it. EnterEffect keeps only its own 2 derives. Bug 2 (L1093-1114): the two new #[test] functions landed inside impl Default for Language { fn default()... } -> '#[test] may only be used on a free function' + E0407 'method ... is not a member of trait'. Fix: move both tests into the existing #[cfg(test)] mod tests { ... } block, just before its closing `}`. impl Default for Language closes cleanly after fn default(). Both fixes are mechanical text moves with no semantic changes. Global brace balance verified; EnterEffect/ActionDef attr adjacency verified; tests confirmed inside mod tests (before impl Default for Language). https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e64cf4fc33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// values (beyond string-encoded `to_value`) is a tracked follow-up. | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| #[derive(Debug, Clone, PartialEq, Eq, Default)] | ||
| pub struct EnterEffect { |
There was a problem hiding this comment.
Because EnterEffect is a public vocabulary struct and the module docs promise every public struct/enum is forward-compatible via #[non_exhaustive], leaving this type exhaustive lets downstream crates construct it with a literal and makes adding the planned typed value fields a semver-breaking change. Please put #[non_exhaustive] back on EnterEffect after moving it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the latest push. #[non_exhaustive] restored on EnterEffect, ordered after the derives to match ActionDef's convention in this crate. Tests still compile — #[non_exhaustive] only restricts external-crate construction; the in-crate assert_eq!(e, EnterEffect { ... }) literal is permitted within ogar-vocab itself. This makes the planned typed-value tightening of to_value (and any other future fields) a non-breaking change for downstream consumers.
…#15) EnterEffect is a public vocabulary struct; the module convention is #[non_exhaustive] for forward-compat (mirrors ActionDef and the rest of the vocab). Without it, downstream crates can construct EnterEffect with a struct literal, and adding the planned typed-value field later (the follow-up noted in the doc comment) would be a semver-breaking change. Tests are unaffected — #[non_exhaustive] only restricts external crates; the in-crate test that uses a struct literal still compiles. Piggybacked on the placement fix branch (#15) so the EnterEffect repair lands as one coherent unit. https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
…change PR #13 changed ActionDef.on_enter from Option<String> to Option<EnterEffect> but didn't propagate to ogar-emitter, leaving two cascade breakages: - emit_action_def (L678-680) was doing Triple::new(..., e.clone()) on what is now &EnterEffect, not &String -> compile error. - the action_def_emits_rubicon_statem_carriers test (L1307+L1311) built a String fixture and asserted against a free-form string object -> two further compile errors. Structural fix matching the §6 TTL (vocab/ogar.ttl from #13): ogar:onEnter rdfs:range ogar:EnterEffect, with ogar:enterField + ogar:enterToValue as the leaf properties. Emitter now produces: (<def_id>, ogar:onEnter, <node_id>) (<node_id>, ogar:enterField, e.field) (<node_id>, ogar:enterToValue, e.to_value) where <node_id> = <def_id>::on_enter (stable per def). Test fixture rebuilt to use EnterEffect::transition('state', 'sale') and assertions updated to verify the link triple + the two leaf triples. Closes the cascade from #13; #15 now ships ogar-vocab AND ogar-emitter as one coherent EnterEffect repair. https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
|
Pushed
Took the structural shape rather than the simpler concatenated-string variant since it matches the TTL #13 already promotes the predicate to — consumers can reassemble the EnterEffect structurally without re-parsing. Same call as your proposal. #15 now ships |
…ed -> Implemented Closes the loop on the ADR doc by recording the bardioc-side Rubicon Phase 1 -> 4 implementation receipts handed off after PR #21 merged. Six ADRs moved from 'Pinned' to 'Pinned + Implemented': ADR-001 (State=ActionState lifecycle) — bardioc 9412c68 (6/6) ADR-002 (split ActionDef/ActionInvocation) — bardioc 9412c68 (6/6) ADR-005 (typed EnterEffect) — bardioc 9412c68 + OGAR #15 ADR-007 (canonical state_machine signatures) — ractor_actors @ 38a71a4 (7/7) ADR-008 (commit_event sibling) — lance-graph #467 + bardioc 8c74c18 (13/13) ADR-009 (temporal two-axis engine) — lance-graph #468 + bardioc b055bfc (20/20) Plus the §14 acceptance gate (referenced by ADR-018 + multiple SUBSTRATE- ENDGAME / TRANSCODING sections) — bardioc Phase 4 43b272a (25/25). One ADR moved to 'Pinned (half-implemented)': ADR-010 (knowable_from meet-point) — consumer side live (lance-graph #468); producer side stubbed (OGAR #18 ogar-adapter-surrealql awaiting lance-bind Sprint-5b + rust-version bump). Closed-loop end-to-end documented in the addendum: ActionDef -> on_event -> evaluate_guard (StateGuard + Depends) -> Goto(Committed) -> on_commit -> LanceMembraneWriter -> commit_event(row) -> version bump + LanceVersionWatcher fan-out -> next Pending evaluates against post-commit deinterlace. Per-ADR receipts table + remaining-Pinned table with each unlock condition. Cumulative status table summarising 6 Pinned+Implemented / 1 half / 14 Pinned awaiting (the expected distribution: most ADRs are architectural decisions whose implementation lands incrementally as the ecosystem matures). Index table at top updated to reflect new statuses for receipt-bearing ADRs. Other ADR statuses unchanged. Bardioc-side anchor in MIGRATION_SPINE.md §2 + CROSS_SESSION_COORDINATION.md carries the parallel record; the docs are mutually-anchored. Pure docs; no code changes. https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
1 participant
Fix:
maindoesn't compile after #13PR #13 introduced two placement bugs in
crates/ogar-vocab/src/lib.rs. Both mechanical, both my mistake; the other session diagnosed them precisely. This PR is the green-verified repair.Bug 1 — struct region (~L302–332)
EnterEffectwas inserted betweenActionDef's derive attributes and its struct body:Fix: relocate the
EnterEffectdoc + attrs + struct +implblock to afterActionDef's closing brace.ActionDefregains its 3 attrs adjacent to it;EnterEffectkeeps only its own 2 derives.Bug 2 — test region (L1093–1114)
The two new
#[test]functions landed insideimpl Default for Language { fn default()… }, triggering#[test] may only be used on a free function+ E0407method … is not a member of trait.Fix: move both tests into the existing
#[cfg(test)] mod tests { … }block, before its closing brace.impl Default for Languagecloses cleanly afterfn default().Verification (in-script before push)
{=}after the moves.impl Default for Languageblock: regex-extracted, no#[test]inside.ActionDefandEnterEffectderive attrs each verified adjacent to their respective struct keyword.mod testsandimpl Default for Language(i.e. inside the tests mod).No semantic changes — pure text relocation.
Why this slipped past CI on #13
PR #13 merged without
cargo test --workspacerunning on the merge candidate (the new tests would have caught Bug 2 trivially; Bug 1 fails any cargo build). Worth adding a required check oncrates/**PRs.https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY