Skip to content

feat(menu): Phase C - window-event callback bridge (#682)#683

Merged
jsavin merged 4 commits into
developfrom
worktree-menu-port-phase-c-bridge
May 31, 2026
Merged

feat(menu): Phase C - window-event callback bridge (#682)#683
jsavin merged 4 commits into
developfrom
worktree-menu-port-phase-c-bridge

Conversation

@jsavin

@jsavin jsavin commented May 31, 2026

Copy link
Copy Markdown
Owner

Summary

Phase C of docs/MENU_PORT_PLAN.md: the C-side bridge that fires idopenwindowscript / idclosewindowscript when the REPL "frontmost window" changes, so the existing UserTalk windowTypes framework at system.verbs.builtins.Frontier.tools.windowTypes/ receives window-focus-change events at REPL boot.

  • frontier-cli/window_registry.c + window_registry.h: window_registry_init() creates system.temp.windowTypes.windows.repl with type="ReplWindow". on_frontmost_changed(old, new) fires idclosewindowscript / idopenwindowscript via getsystemtablescript -> langdeparsestring(bs, ASCII-DQ) -> parsedialogstring -> langrunstringnoerror.
  • frontier-cli/repl.c: boot step 3.3 calls window_registry_init() then on_frontmost_changed(nil, WINDOW_BRIDGE_REPL_PATH) after install_repl_menubar().
  • databases/Virgin.root + usertalk_scripts/.../ReplWindow/openWindow.ut: installs Frontier.tools.data.windowTypes.ReplWindow so findWindowType("ReplWindow") returns true.
  • tests/test_window_bridge.c: 5 behavioral tests (open fires, close fires, nil+nil noop, same-path no double-fire, injection attempt safe). Uses C-side langcompiletext+hashtableassign stubs - no script.newScriptObject dependency on full Frontier.root.

Escaping correction (issue #682): original spec said use chclosecurlyquote (0xD3). The script templates use straight ASCII " as string delimiters. langdeparsestring(bs, 0xD3) does NOT escape ASCII " (0x22) - it only escapes the Mac curly-quote (0xD3). Fixed to langdeparsestring(bs_path, '"'). Without this fix, test 5 (injection attempt) fails because embedded " breaks out of the string literal.

Phase D pre-work signal: window.attributes.getOne("type", adr) is NOT implemented in headless. The windowTypes framework's callbacks/openWindow.ut gracefully returns true when this fails (skips the if-body). Full ReplWindow menu composition via the framework is blocked until Phase D implements window.attributes.*.

Test plan

  • tests/test_window_bridge.c - 5 behavioral tests, all GREEN
  • Unit suite: 491 -> 492 tests (1 new suite, 5 new tests), 492/492 pass
  • Integration suite: html/tcp failures are pre-existing (same on develop base, confirmed before this PR); repl_sessions and repl_commands are cleaner on this branch than on develop
  • CLI builds clean: make -C frontier-cli
  • tools/verify_virgin_root_sync.sh confirms ReplWindow/openWindow.ut syncs with Virgin.root

🤖 Generated with Claude Code

jsavin and others added 2 commits May 31, 2026 13:37
Wires the kernel-side bridge that fires idopenwindowscript /
idclosewindowscript from headless C so the existing UserTalk
windowTypes framework at system.verbs.builtins.Frontier.tools.windowTypes/
receives window-focus-change events at REPL boot.

Key implementation details:
- frontier-cli/window_registry.c: window_registry_init() creates
  system.temp.windowTypes.windows.repl with type="ReplWindow",
  title="REPL". on_frontmost_changed(old, new) fires the close/open
  scripts using: getsystemtablescript -> langdeparsestring(bs, ASCII-DQ)
  -> parsedialogstring -> langrunstringnoerror.
- Escaping correction (issue #682): templates use straight ASCII double-
  quote as string delimiters. Must pass ASCII-DQ (not chclosecurlyquote
  0xD3) to langdeparsestring or embedded quotes in paths go unescaped.
- frontier-cli/repl.c: wires window_registry_init() + initial
  on_frontmost_changed(nil, WINDOW_BRIDGE_REPL_PATH) into boot
  sequence after install_repl_menubar() (step 3.3).
- databases/Virgin.root + usertalk_scripts/.../ReplWindow/openWindow.ut:
  installs Frontier.tools.data.windowTypes.ReplWindow placeholder
  so findWindowType("ReplWindow") returns true.
- tests/test_window_bridge.c: 5 behavioral tests using C-side
  langcompiletext+hashtableassign stubs (avoids script.newScriptObject
  dependency on full Frontier.root). All 5 pass.

Test counts: unit 491->492 (1 new suite, 5 new tests), all pass.
Integration: pre-existing html/tcp failures unchanged (verified same
failures on develop base before this PR).

Phase D pre-work signal: window.attributes.getOne("type", adr) is NOT
implemented in headless. The framework gracefully returns true when it
fails. Full ReplWindow menu composition is blocked until Phase D.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…har truncation

The 411-char inline UserTalk script in window_registry_init() was silently
truncated to 255 bytes by cstr_to_bigstring (bigstring = Str255 = 256-byte
Pascal string), cutting mid-token and causing a syntax error at REPL startup.

Split into 5 separate langrunstringnoerror calls, each under 110 chars.

Fixes PR #683 Bug 1.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two items from /gate round-1 review:

P1 (bar-raiser): fix test comment wording — escaping is backslash, not
"doubled". `langdeparsestring(bs, '"')` inserts a backslash before each
`"` and `\`, producing `\"` (langscan.c:228). The previous comment
described SQL/CSV-style doubled-quote escaping which is inaccurate.

P2 (bar-raiser): compact Virgin.root from 26,663,785 to 10,970,203 bytes
(15.7 MB reclaimed). The +3.6 MB growth from the single 8-line ReplWindow
openWindow.ut addition was allocation churn / free-list slack from the
protocol install session — the same class of bloat that PR #581 cleaned
up (99 MB -> 11 MB). Ran db.compactDatabase against the post-install
state. ReplWindow entry verified intact in compacted output via
`defined(@Frontier.tools.data.windowTypes.ReplWindow.openWindow)` = true.

Tests: 492/492 unit pass, integration baseline maintained.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jsavin

jsavin commented May 31, 2026

Copy link
Copy Markdown
Owner Author

/gate Round 1 — PR #683 (Phase C)

Verdict: PASS WITH NOTES (1 P1 from bar-raiser; 0 from security/concurrency)

Reviewers

  • bar-raiser (always): PASS WITH NOTES — 1 P1, 4 P2
  • security (always): PASS — 0 P0/P1, 4 P2 (all forward-flag for Phase E)
  • concurrency (auto: matched path frontier-cli/**): PASS — 0 findings

Tests

  • Unit: 492/492 (cached at 73df1ef)
  • Integration: 2155 pass / 207 skip / 18 fail (18 pre-existing html./tcp. baseline)

Live test (per task #15 standing gate)

PASSED after fix-round. Bug 1 (boot syntax error) was a 411-byte UserTalk script truncated to 255 bytes by cstr_to_bigstring; fixed in 73df1ef3a by splitting into 5 statements. Bug 2 (sleepfor "too many parameters") was traced to pre-existing thrv_sleepfor arity-check issue in Frontier.tools.thread.script.ut; database-state dependent, reproducible only on non-Virgin Frontier.root. NOT a Phase C regression. Filed as issue #684.


P1 (addressed in round-2 commit 7c1994c4c)

# Finding File:line Flagged by Status
1 Test comment says escaping "doubles" the quote, but langdeparsestring actually backslash-escapes (Common/source/langscan.c:228). Misleading for future debugging tests/test_window_bridge.c:340-342 bar-raiser FIXED in 7c1994c — comment now describes backslash escaping accurately

P2 disposition

# Finding Status
BR P2 #2 Virgin.root +3.6 MB growth for one 8-line .ut (allocation churn) FIXED in 7c1994c — ran db.compactDatabase: 26.7 MB → 11.0 MB (12 MB smaller than pre-PR baseline). ReplWindow entry verified intact post-compact
BR P2 #3, Sec P2-2 cstr_to_bigstring silently truncates at 255 bytes FILED as issue #685
BR P2 #4 Same-window test doesn't assert close marker FILED as issue #686 (Phase E hardening)
BR P2 #5 strcmp-based path comparison doesn't normalize FILED as issue #686
Sec P2-1 langdeparsestring overflow return value ignored FILED as issue #686
Sec P2-3 flscriptrunning save/restore deviates from legacy FILED as issue #686
Sec P2-4 User-controlled path interpolated into log unscrubbed FILED as issue #686

Surprises worth surfacing

  1. Escaping contract: chclosecurlyquote vs ASCII ". The original Phase C plan in MENU_PORT_PLAN.md specified langdeparsestring(bs, chclosecurlyquote) (0xD3). The agent caught this was wrong — the YAML templates use ASCII " (0x22) post curly-to-ASCII normalization (see PR feat(menu): Phase B1 - getsystemtablescript headless support (list 139) #680's resources/strings/idsystemtablescripts.yaml header lines 16-17). Passing 0xD3 would have left ASCII " unescaped → fully exploitable injection vector. Agent corrected to '"' and documented in code + docs.

  2. thread.callScript already wired: confirmed via frontier-cli/headless_thread_verbs.c:683. Task docs: clarify headless uses full sanitized Frontier.root #25 RESOLVED at the framework layer — Phase B2 (C-side async dispatch) is deferrable indefinitely because runFileMenuScript.ut already uses thread.callScript for menu-item dispatch.

  3. Virgin.root growth recurrence: PR db: add db.compactDatabase + slim Virgin.root from 99 MB to 11 MB #581 compacted from 99 MB → 11 MB; Phase A added negligibly; Phase C grew it +3.6 MB for a single 8-line file. ODB free-list slack during install sessions appears to be a recurring failure mode. Worth thinking about whether the compact should be part of the install protocol itself.

Stats

  • Files reviewed: 9
  • Round-1 findings: 0 P0, 1 P1 (fixed round 2), 8 P2 (1 fixed + 7 filed across 3 issues)
  • Reviewers ran: 3 of 4 configured (swiftui excluded)

🤖 Generated with Claude Code

Bar-raiser P3 round-2 nit: the comment block above this TEST_ASSERT was
fixed in 7c1994c but the assertion failure message itself still said
'double the embedded quote'. Same mental model the P1 corrected.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jsavin

jsavin commented May 31, 2026

Copy link
Copy Markdown
Owner Author

/gate Round 2 — PR #683 (Phase C)

Verdict: PASS (0 P0/P1, 1 P3 nit found-and-fixed in same round)

Reviewers (round 2)

  • bar-raiser: PASS — verified both round-1 fixes byte-correct; found 1 P3 (assertion-message text drift) which was fixed in 4d372e1f9
  • security: not re-run (round-1 had 0 P0/P1; round-2 diff is binary compaction + 1-line comment + 1-line test message; no new security surface)
  • concurrency: not re-run (round-1 zero findings; round-2 introduces no concurrency surface)

Tests

  • Unit: 492/492 (cached at 4d372e1)
  • Integration: 2155 pass / 207 skip / 18 fail (baseline maintained)

Round-1 fix disposition

ID Item Status
P1 test_window_bridge.c comment "doubled" → "backslash-escaped" FIXED in 7c1994c
P2 #2 Virgin.root +3.6 MB bloat FIXED in 7c1994c (compacted 26.7 MB → 11.0 MB, below pre-PR baseline)
P2 #3 cstr_to_bigstring silent truncation FILED #685
P2 #4 Same-window test missing close assert FILED #686
P2 #5 strcmp path comparison normalization FILED #686

Round-2 fix disposition

ID Item Status
P3 nit test_window_bridge.c:354 assertion message also said "double the embedded quote" FIXED in 4d372e1 (15-sec edit)

Convergence

Ready to merge.

🤖 Generated with Claude Code

@jsavin jsavin merged commit 3bedd58 into develop May 31, 2026
@jsavin jsavin deleted the worktree-menu-port-phase-c-bridge branch May 31, 2026 23:34
jsavin added a commit that referenced this pull request Jun 5, 2026
…685) (#708)

* feat(window_registry): detect cstr_to_bigstring 255-byte truncation (#685)

Pre-#685: cstr_to_bigstring() was a static, void-returning helper that
silently truncated C strings longer than 255 bytes. This masked PR
#683 bug 1 -- a 411-byte UserTalk script got cut mid-token at boot
and produced "syntax error at line 1". The fix at the time split the
script into smaller pieces, but the helper itself remained a silent
truncator, ready to bite the next caller.

Two complementary detections, per the issue's "pick one or combine"
guidance:

1. Runtime: cstr_to_bigstring() now returns bool. true on a complete
   copy, false when the input exceeded 255 bytes and was clamped to
   fit. Both production callers check the return:
   - fire_window_script() bails with a log_warn() if the window_path
     was truncated. Phase C always passes WINDOW_BRIDGE_REPL_PATH (a
     short compile-time constant) so this is dormant today, but
     Phase E will populate paths from user-derived editor-window ODB
     addresses where the guard becomes load-bearing.
   - window_registry_init() bails if any stmts[i] was truncated. The
     compile-time macro can't apply here because stmts[] is an array
     of (const char *) rather than array-of-arrays, so sizeof(stmts[i])
     would give pointer size. The runtime check is equivalent.

2. Compile-time: new CSTR_TO_BIGSTRING_LIT(literal, bs) macro in
   window_registry.h. _Static_assert(sizeof(literal) <= 256) catches
   over-length string-literal inputs at build time, with zero runtime
   cost. This is the form to use for any future direct-literal
   callsite. It would have failed the build on the 411-byte literal
   that caused PR #683 bug 1, surfacing the bug before boot.

Helper was promoted from static to a header-declared function so
tests can link against it directly. The header is the natural home
since cstr_to_bigstring's only callers are in window_registry.c and
its scope is "Pascal bigstring construction for the window-event
bridge"; broader reuse should wait for an actual second caller.

Test coverage: 5 new unit tests in tests/cstr_to_bigstring_tests.c
covering empty input, short input, exactly-at-limit (255 bytes),
one-byte-over (256 bytes), and the PR #683 bug 1 shape (411 bytes).
Wired into RUN_BUILDABLE. Unit suite now reports 585 tests (was 580).

Closes #685.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(window_registry): invoke CSTR_TO_BIGSTRING_LIT macro at production callsite (#685)

Addresses bar-raiser P1 #1 from /gate.

The original commit defined CSTR_TO_BIGSTRING_LIT but never invoked it.
The macro's documented compile-time guarantee was aspirational only; the
production code used the runtime cstr_to_bigstring() helper. The test
file's comment claimed the macro was "exercised by the production code"
which was false.

Fix per bar-raiser Option B: restructure stmts[] from an array of
(const char *) to a fixed-width char[256] array-of-arrays. This makes
sizeof(stmts[i]) return 256 at compile time, so CSTR_TO_BIGSTRING_LIT
can apply its _Static_assert(sizeof(literal) <= 256) per element. The
runtime cstr_to_bigstring check inside the loop is dropped because the
compile-time assert is strictly stronger.

Memory cost: ~6 * 256 = 1536 bytes static const data on a one-time CLI
init path. Negligible.

Sanity check (manual): adding a 260-byte literal to stmts[] now fails
the build with "_Static_assert failed; expression evaluates to '261 <=
256'" - the exact PR #683 bug 1 detection the issue asked for. This was
verified by compiling a stub program with a 260-char literal passed to
the macro; the compiler emitted the expected static-assert error.

Also addresses bar-raiser P2 (C11 requirement on the macro):
- Updated the macro's doc comment to spell out the C11 requirement and
  cross-reference the project's -std=c17 / -std=c99 split.
- Updated tests/cstr_to_bigstring_tests.c file comment to honestly
  describe where the macro is exercised (stmts[] table) instead of the
  prior false claim about the init path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
jsavin added a commit that referenced this pull request Jun 5, 2026
…an (#707) (#713)

Closes #707.

Pre-#707: copyctopstring() in Common/source/strings.c had two silent
failures that produced a buffer overflow on >255-byte input:

  1. No length clamp. `short len = strlen(src); memmove(bs+1, src, len);`
     -- a >255-byte source overflowed the 256-byte bigstring buffer.

  2. setstringlength() truncated len to a single byte. A short input
     of 300 became length byte 44, leaving the bigstring internally
     inconsistent: claimed 44 bytes, actually held 300.

The vulnerability was discovered during PR #685's gate review when
bar-raiser flagged the duplication between PR #685's safer local
helper and the canonical copyctopstring(). 338 callsites across
Common/ and portable/ + tests use copyctopstring(); most pass short
literal error strings or short paths, but a few accept dynamic data
(filesystem paths, error buffers, version strings).

Fix
===

Common/source/strings.c + Common/headers/strings.h:
  - copyctopstring() now returns boolean (true on complete copy,
    false on truncation). Existing callers ignore the return,
    getting the same observable behavior MINUS the buffer overflow.
    New safety-aware callers can opt in by checking the return.
  - Payload is clamped to 255 bytes BEFORE memmove. The length byte
    and the actual payload count now always agree.
  - `short len` -> `size_t srclen` so the comparison `srclen <= 255`
    doesn't sign-flip on very long inputs (defensive).

portable/runtime_stubs.h + runtime_stubs_string.c:
  - Stub declaration and definition updated to match the canonical
    signature. (These files aren't linked into the live build today
    but kept in sync to avoid future surprises.)

frontier-cli/window_registry.{c,h}:
  - The window-registry-local cstr_to_bigstring() (added in PR #685)
    is deleted; window_registry uses the shared copyctopstring() now.
  - The CSTR_TO_BIGSTRING_LIT macro is replaced by an inline
    _Static_assert(sizeof(stmts[0]) <= 256) next to the copyctopstring
    call inside window_registry_init's stmts[] loop, preserving the
    compile-time guarantee. The fire_window_script's runtime check
    + log_warn + early-return on truncation is preserved.

tests:
  - New tests/copyctopstring_tests.c: 6 behavioral tests using a
    guarded-bigstring (pad bytes before/after the buffer with sentinel
    values) to detect any OOB write. Covers empty, short, exactly-at-
    limit (255), one-byte-over (256), PR #683 bug 1 shape (411 bytes),
    and a length-byte-agrees-with-payload invariant test. Pre-fix the
    one-over and 411-byte tests would FAIL the OOB guard; post-fix
    all 6 pass.
  - tests/cstr_to_bigstring_tests.c deleted; its coverage is now in
    copyctopstring_tests against the shared helper.
  - tests/Makefile: copyctopstring_tests added to RUN_BUILDABLE;
    cstr_to_bigstring_tests target removed.

Test plan
=========

  $ make -C frontier-cli clean && make -C frontier-cli
  ... builds clean (no signature-change warnings from the 338 callsites)

  $ ./tools/run_headless_tests.sh
  ... 586/586 pass (was 585; -5 cstr_to_bigstring + 6 copyctopstring)

  $ cd tests && make test-integration
  ... 2186 pass, 21 baseline failures preserved (no regression)

  $ ./tests/copyctopstring_tests
  === copyctopstring buffer-overflow detection (#707) ===
  PASS x6
  [test_report] copyctopstring_tests: 6/6 tests passed

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant