fix(smart-forms): allow visibility placeholders

[yevhen-porechnyi]

What & why

pushSmartForm rejected page configs that used a pure {{viewModelKey}} placeholder for visibility, although pong-server resolves that field before the page reaches the CDU renderer.

This change accepts pure visibility placeholders for forms, sections, and rendered items in header, modalHeader, and content, including nested and contentLoop items. Malformed, embedded, and multiple placeholders remain invalid. The unsupported section footer slot is deliberately not broadened.

The Smart Form skill and CDU protocol reference now distinguish server-side visibility binding from client-side reactive visibility.

Type of change

• Bug fix
• New MCP tool / capability
• Skill behaviour
• Docs
• Chore / refactor

Checklist

• make build && make vet && make test pass locally
• No tool was added or renamed
• Skill frontmatter is unchanged; make discovery produces no diff
• Reference docs remain under plugins/simulator/docs/
• No tokens or .env committed; TLS behaviour is unchanged
• Added an entry under CHANGELOG.md → Unreleased

Notes for reviewers

Verified against pong-server template rendering: form and section visibility are injected explicitly, while rendered item properties are injected recursively. Placeholder values must resolve to visible, disabled, or hidden.

[gh-corezoid]

AI Review

Relaxes pushSmartForm validation to accept pure {{viewModelKey}} placeholders for visibility on forms, sections, and rendered items in header, modalHeader, and content slots; footer is intentionally kept to resolved values only. Includes Go unit tests, CHANGELOG entry, CDU-protocol docs update, and skill documentation.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (develop)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	⚠️ warning (see below)
S1 — No manual edits to public/	✅ pass
S2 — API path parameter names	⏭ skip (no internal/tools/ changes)
S3 — New tools have eval scenarios	⏭ skip (no new tools)
S4 — Discovery artifacts committed	✅ pass
S5 — All six manifest files version-synced	⏭ skip (no manifest files changed)
S6 — README / ARCHITECTURE updated for new tools	⏭ skip (no new tools)

U5 detail: go build ./... and go test ./... both clean on the PR branch (all 14 packages); base branch (develop) is equally clean — no regressions introduced.

S4 detail: Ran go run ./cmd/gendiscovery on the PR branch; public/ produced zero diff — the cduschema change does not affect discovered output.

Issues found

[warning — U6] isViewModelPlaceholder silently accepts {{ key }} with internal whitespace

key := value[2 : len(value)-2]
return strings.TrimSpace(key) != "" && !strings.ContainsAny(key, "{}")

strings.TrimSpace is used only to test emptiness, not to normalise the key. A value like "{{ myKey }}" (spaces inside braces) passes validation because TrimSpace(" myKey ") != "" and the key contains no {}. If pong-server's template engine does not strip whitespace from placeholder identifiers, {{ myKey }} will pass our validator but silently fail to resolve server-side (yielding an unresolved string instead of visible|disabled|hidden, which the CDU renderer may treat as invalid).

Suggested fix — tighten the guard to reject keys that contain any whitespace:

return key == strings.TrimSpace(key) && key != "" && !strings.ContainsAny(key, "{}")

None of the test cases cover this variant, so the current test suite cannot catch the regression if pong-server behaviour ever diverges.

[warning — U6] PR description claims contentLoop item coverage; no test confirms it

The PR body states the change covers "nested and contentLoop items." However, contentLoop is not in the validated slot list (["header", "content", "modalHeader"]) and has no dedicated test case in schema_test.go. If contentLoop is a distinct section slot (per SKILL.md: "contentLoop | section array expansion | one template → N rows"), its items are skipped by the validator entirely — both before and after this PR (pre-existing gap, not introduced here).

This is low-risk for the bug being fixed, but the phrasing in the PR description overstates coverage. Consider either:

• adding a contentLoop test that documents the current behaviour (items not validated), or
• adding "contentLoop" to the validated slot list with allowVisibilityPlaceholder: true if pong-server does resolve it.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[gh-corezoid]

AI Review

Re-review after synchronize: relaxes pushSmartForm validation to accept pure {{viewModelKey}} placeholders for visibility on forms, sections, and rendered items in header, modalHeader, and content; spaced-key placeholders ({{ key }}) are now correctly rejected; footer intentionally left to resolved values only.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (develop)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	⚠️ warning (see below)
S1 — No manual edits to public/	✅ pass
S2 — API path parameter names	⏭ skip (no internal/tools/ changes)
S3 — New tools have eval scenarios	⏭ skip (no new tools)
S4 — Discovery artifacts committed	✅ pass
S5 — All six manifest files version-synced	⏭ skip (no manifest files changed)
S6 — README / ARCHITECTURE updated for new tools	⏭ skip (no new tools)

U5 detail: go build ./... and go test ./... both clean on the PR branch (all 14 packages, including 3 new test functions in cduschema); develop base is equally clean — no regressions introduced.

S4 detail: internal/cduschema/validate.go change does not affect discovery output — public/ correctly untouched.

Issues found

[warning — U6] contentLoop-expanded item visibility: coverage is implicit, not explicit

The PR description and both docs files state that the fix "applies to … items expanded from contentLoop." Architecturally this is correct — contentLoop is a section-level expansion directive, not a separate slot, and the items it expands come from section.content, which is already in the validated slot list with allowVisibilityPlaceholder: true. So content template items with a visibility placeholder do pass the validator.

However, no test exercises a section that uses both contentLoop and a visibility placeholder in its content template items. This leaves a documentation gap: a reader of schema_test.go cannot confirm from the test suite alone that contentLoop-expanded items are covered. The risk is low (the coverage path is real), but a one-line test documenting the behavior would close the gap and guard against future refactors that could accidentally break the implicit path.

---

Fixed since last review ✅

• [U6] isViewModelPlaceholder accepted {{ key }} with internal whitespace. Resolved by commit 8d072c37 (fix(smart-forms): reject spaced visibility placeholders). The guard now uses key == strings.TrimSpace(key) — values like "{{ myKey }}" are correctly rejected. A dedicated test case ("placeholder with surrounding whitespace") confirms the behaviour.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[gh-corezoid]

AI Review

Re-review after synchronize: relaxes pushSmartForm validation to accept pure {{viewModelKey}} placeholders for visibility on forms, sections, and rendered items in header, modalHeader, and content; footer intentionally left to resolved values only; contentLoop-expanded item coverage is now exercised by an explicit test.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (develop)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	✅ pass
S1 — No manual edits to public/	✅ pass
S2 — API path parameter names	⏭ skip (no internal/tools/ changes)
S3 — New tools have eval scenarios	⏭ skip (no new tools)
S4 — Discovery artifacts committed	✅ pass
S5 — All six manifest files version-synced	⏭ skip (no manifest files changed)
S6 — README / ARCHITECTURE updated for new tools	⏭ skip (no new tools)

U5 detail: go build ./... and go test ./... both clean on the PR branch (all 14 packages, including 5 new test functions in cduschema); develop base is equally clean — no regressions introduced by this PR.

S4 detail: internal/cduschema/ changes do not affect discovery output — public/ correctly untouched. Confirmed by the PR author's own make discovery check and the absence of public/ in the diff.

Issues found

No issues found.

---

Fixed since last review ✅

• [U6] contentLoop-expanded item visibility: coverage was implicit, not explicit. Resolved by commit 701eb1f (test(smart-forms): cover contentLoop visibility placeholders). The new TestValidatePageConfig_ContentLoopVisibilityPlaceholder test exercises a section with a contentLoop directive and a {{fieldVisibility}} placeholder on a content item, confirming that the content-slot path with allowVisibilityPlaceholder: true is reachable and correct. The previously documented coverage gap is now closed.

---

This review was generated automatically. A human maintainer should still make the merge decision.