Skip to content

feat: remediate all 10 Phase 0-3 completeness audit findings - #584

Merged
aram-devdocs merged 9 commits into
mainfrom
alpha-001-phase0-3-completeness-remediation
Mar 20, 2026
Merged

feat: remediate all 10 Phase 0-3 completeness audit findings#584
aram-devdocs merged 9 commits into
mainfrom
alpha-001-phase0-3-completeness-remediation

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

Comprehensive remediation of all 10 findings from the ALPHA-001 Phase 0-3 completeness audit.

  • Finding 1 (CRITICAL): Zero distribution secrets → Created docs/SECRETS_SETUP.md documenting all required GitHub secrets
  • Finding 2 (CRITICAL): C language missing Flappy Bird → Full C implementation mirroring C++ version (10 source files)
  • Finding 3 (HIGH): Missing publish CI jobs → Added publish-luarocks and publish-go to release workflow
  • Finding 4 (HIGH): No SECRETS_SETUP.md → Created comprehensive documentation
  • Finding 5 (MEDIUM): C++ SDK hand-written → Created codegen/gen_cpp.py generating goud.g.hpp from schema
  • Finding 6 (MEDIUM): Missing sandbox/feature_lab examples → Added for Go, Kotlin, Lua, Swift, C++ (10 new examples)
  • Finding 7 (MEDIUM): Open issues UiManager labels need absolute positioning support #553, feat(sdk): Add coordinate origin option (center vs top-left) for DrawQuad/DrawSprite #554 → Both implemented (UiManager absolute positioning + CoordinateOrigin)
  • Finding 8 (LOW): Schema lists only 3 SDKs → Updated to list all 9
  • Finding 9 (MEDIUM): Version skew → Aligned schema to version.txt (0.0.832)
  • Finding 10 (MEDIUM): C SDK no test suite → Created 4 test files with CMake

Test plan

  • cargo build — 502 FFI functions, clean
  • cargo test — 576 passed, 0 failed
  • cargo clippy -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • Schema/mapping JSON validation — valid, no duplicate keys
  • C Flappy Bird compiles via CMake
  • C SDK tests pass via CTest (4/4)
  • CI/CD pipeline passes
  • SDK regeneration via ./codegen.sh (post-merge)

🤖 Generated with Claude Code

- Fix version skew (schema 0.0.833 → 0.0.832 to match version.txt)
- Fix schema description to list all 9 SDK languages
- Create docs/SECRETS_SETUP.md for release pipeline secrets
- Implement C Flappy Bird example (full parity with C++ version)
- Create C SDK test suite (4 test files with CMake)
- Add publish-luarocks and publish-go jobs to release workflow
- Implement #553: UiManager absolute positioning (SetNodePosition, SetNodeVisible FFI)
- Implement #554: CoordinateOrigin option for DrawQuad/DrawSprite (Center/TopLeft)
- Create codegen/gen_cpp.py for C++ SDK generation from schema
- Add sandbox + feature_lab examples for Go, Kotlin, Lua, Swift, C++
- Add --sdk c support to dev.sh

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @aram-devdocs's task in 4m 44s —— View job


PR Review: feat: remediate all 10 Phase 0-3 completeness audit findings

Review checklist:

  • Gather context (AGENTS.md, PR description)
  • Phase 1: Context & layer identification
  • Phase 2: Architecture validation
  • Phase 3: Anti-pattern scan (FFI safety, unsafe blocks)
  • Phase 4: Quality assessment
  • Phase 5: Security review (FFI/unsafe changes present)
  • Phase 6: Scope verification
  • Phase 7: Final verdict

Verdict: ⚠️ CHANGES REQUESTED (non-blocking — can merge with accepted warnings)

The Rust engine changes are solid and well-tested. The main concern is SDK parity: the PR explicitly defers Python and C++ SDK updates to post-merge, but per the repo's own CLAUDE.md/FFI rules, these must be updated atomically with the FFI changes.


BLOCKERS

None that prevent merge.


WARNINGS (should fix before or immediately after merge)

# Severity File Issue
1 HIGH sdks/python/goud_engine/generated/_ffi.py Missing ctypes declarations for 4 new FFI functions: goud_ui_set_node_position, goud_ui_set_node_visible, goud_renderer_set_coordinate_origin, goud_renderer_get_coordinate_origin. Per .agents/rules/ffi-patterns.md: "Manually update sdks/python/goud_engine/generated/_ffi.py for ctypes declarations". C# is auto-generated by csbindgen on cargo build, but Python is manual.
2 MEDIUM sdks/python/goud_engine/ + sdks/csharp/ Python wrapper classes and C# high-level wrappers (e.g. UiManager, Renderer) don't expose the new absolute positioning or CoordinateOrigin capabilities to SDK users.
3 MEDIUM goud_engine/src/ffi/ui/widget.rs:294, :322 goud_ui_set_node_position and goud_ui_set_node_visible directly mutate nodes via get_node_mut() without calling manager.mark_layout_dirty(). Changing a node's position or visibility should invalidate the layout so the next update() call re-flows the tree. Calling set_node_visible(false) won't affect flex layout until dirty is set (flex already filters by node.visible() at manager/layout.rs:101).
4 LOW codegen/gen_cpp.py File is 806 lines, exceeding the repo's 500-line flag threshold. Fix this →
5 LOW examples/c/flappy_bird/src/main.c:49 ASSET_BASE path is hardcoded as "../../../csharp/flappy_goud/assets/sprites/", relying on CWD being examples/c/flappy_bird/. This will silently fail with blank textures if run from any other directory. Needs a comment warning or env var override.

POSITIVE CALLOUTS

FFI safety is clean:

  • Every unsafe block in widget.rs, immediate.rs, and lifecycle.rs has a // SAFETY: comment. ✅
  • All new pointer parameters (mgr, style, text_ptr) have null checks before dereference. ✅
  • CoordinateOrigin enum is #[repr(C)] and exported correctly. ✅

Coordinate origin implementation (Finding #554):

  • Thread-local per-context storage is the right pattern for immediate-mode state.
  • CoordinateOrigin::adjust() is correct: TopLeft → Center offset by (width/2, height/2).
  • All 4 unit tests cover default, from_u32, adjust, and repr values. ✅

Absolute positioning (Finding #553):

  • set_position() on UiNode correctly switches position_mode to Absolute.
  • Layout system correctly overrides computed rect when position_mode == Absolute (manager/layout.rs:48-51). ✅
  • Tests cover set + read-back for both happy path and null-manager guard. ✅

C SDK test suite (Finding #10):

  • Test structure is idiomatic for C unit tests (CTest + CMake helper function).
  • test_constants.c uses float tolerance 1e-6f rather than exact equality. ✅
  • Tests compile against the real libgoud_engine.so (not mocked). ✅

Release workflow additions (Findings #3):

  • publish-luarocks and publish-go both have correct needs guards.
  • Both are included in the release-summary table. ✅
  • Go proxy warming (go list -m) is a safe no-op if the tag isn't published yet. ✅

C++ codegen (Finding #5):

  • goud.g.hpp is correctly marked AUTO-GENERATED with schema version header.
  • Generator reads from the canonical schema, not hand-written. ✅

Detail: Warning #3 — Missing mark_layout_dirty() calls

// goud_engine/src/ffi/ui/widget.rs:288–295
pub unsafe extern "C" fn goud_ui_set_node_position(
    mgr: *mut UiManager, node_id: u64, x: f32, y: f32,
) -> i32 {
    // ...
    let manager = unsafe { &mut *mgr };
    let id = unpack_node_id(node_id);
    let Some(node) = manager.get_node_mut(id) else { return ERR_NODE_NOT_FOUND; };
    node.set_position(x, y);   // ← layout_dirty not set; next update() skips recompute
    0
}

The UiManager::update()recompute_layout_if_needed() short-circuits when layout_dirty == false. Calling this FFI setter after the initial layout pass will silently do nothing until something else marks the manager dirty.

Note: this is consistent with other existing FFI setters (goud_ui_set_style, goud_ui_set_widget) which also don't call mark_layout_dirty, so this is a pre-existing pattern. But because set_node_position and set_node_visible directly control layout behavior (absolute positioning, flex filtering), they are higher-risk than style setters.

Suggested fix: add manager.mark_layout_dirty() after the node.set_*(...) call in both new functions. Or expose a mark_layout_dirty method as a public FFI entrypoint so callers can batch mutations.


Detail: Warning #1 — Python _ffi.py not updated

Per sdks/CLAUDE.md:

NEVER add an FFI function without updating ALL SDKs (C#, Python, TypeScript)

And per goud_engine/src/ffi/CLAUDE.md:

  1. Manually update sdks/python/goud_engine/generated/_ffi.py with ctypes signatures

The following four functions need ctypes entries in _ffi.py:

  • goud_renderer_set_coordinate_origin(context_id, origin: u32) -> bool
  • goud_renderer_get_coordinate_origin(context_id) -> u32
  • goud_ui_set_node_position(mgr, node_id: u64, x: f32, y: f32) -> i32
  • goud_ui_set_node_visible(mgr, node_id: u64, visible: bool) -> i32

The PR labels "SDK regeneration via ./codegen.sh (post-merge)" as an unchecked item — but ./codegen.sh doesn't update _ffi.py (that's a manual step). Fix this →

Convert bold secret names to level-4 headings to satisfy
the no-emphasis-as-heading rule.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
…gen failure

The setCoordinateOrigin/getCoordinateOrigin methods were defined twice
in the schema — once with u32 types and once with CoordinateOrigin enum
types. The TS codegen generated both, causing TS2393 (duplicate function)
and TS2304 (missing CoordinateOrigin type) errors. Keep the u32-typed
copy consistent with all other FFI-boundary methods.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
- Add ctypes declarations for 4 new FFI functions to Python _ffi.py
  (goud_renderer_set/get_coordinate_origin, goud_ui_set_node_position/visible)
- Add mark_layout_dirty() calls after set_position and set_visible in FFI
  to ensure layout recomputation on next update()
- Make UiManager::mark_layout_dirty() public for FFI access

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
Remove redundant blank lines in test section to stay within the
Rust file line limit enforced by CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
aram-devdocs and others added 2 commits March 20, 2026 09:57
Run ./codegen.sh to propagate setCoordinateOrigin, getCoordinateOrigin,
setNodePosition, and setNodeVisible to all generated SDK surfaces:
C#, Python, TypeScript, Go, Kotlin, Swift, Lua, C++, and JNI bindings.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
Add setCoordinateOrigin/getCoordinateOrigin to GoudGame and
setNodePosition/setNodeVisible to UiManager in the WASM web target
to satisfy IGoudGame and IUiManager interface contracts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
Add setCoordinateOrigin/getCoordinateOrigin stubs to GoudGame and
setNodePosition/setNodeVisible stubs to UiManager in the web SDK
codegen so the generated classes satisfy their interface contracts.
Fixes TS SDK Wasm Build and Codegen Drift Check CI failures.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Mar 20, 2026
@aram-devdocs
aram-devdocs merged commit 3801800 into main Mar 20, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant