Skip to content

feat(strings_compiler): accept multiple positional input files (#681)#706

Merged
jsavin merged 2 commits into
developfrom
worktree-strings-compiler-681-multi-input
Jun 5, 2026
Merged

feat(strings_compiler): accept multiple positional input files (#681)#706
jsavin merged 2 commits into
developfrom
worktree-strings-compiler-681-multi-input

Conversation

@jsavin

@jsavin jsavin commented Jun 5, 2026

Copy link
Copy Markdown
Owner

Closes #681.

Summary

strings_compiler previously rejected a second positional argument, forcing both frontier-cli/Makefile and tests/Makefile to work around it with cat *.yaml | compiler ... -. The pipe-via-cat workaround compiled correctly (the runtime accumulates tables across begin_table calls in one stream) but parse errors lost their source filename and reported "stdin" with a line number that was the offset into the concatenated stream.

This PR teaches the compiler to argv-loop multi-input, reverts both Makefiles to direct invocation, and tightens the libyaml-parser error path to prepend source name + line/col so multi-file runs can pin parse errors to the right file.

What changed

  • tools/strings_compiler/strings_compiler_main.c — Collect positional inputs into a heap array (initial cap 4, grows by doubling). Loop after argparse, one strings_load_yaml_stream per file. strings_begin/finish_document still brackets the whole multi-file load so cross-file dedup works unchanged. Zero-input case still reads stdin (backward compat). Every exit path frees the inputs array.
  • tools/strings_compiler/strings_yaml_loader.c — The libyaml-parser-level error branch now formats as <source_name>:<line>:<col>: libyaml parser error: <problem>. The structural-error branches already prepended source_name; this brings the parser branch in line.
  • frontier-cli/Makefile + tests/Makefile — Reverted from cat $(STRINGS_INPUTS) | $(STRINGS_COMPILER) ... - to direct $(STRINGS_COMPILER) ... $(STRINGS_INPUTS). Workaround explanation comments removed.
  • tests/strings_compiler_multi_input_test.sh (new) — Test 1 (positive): invoke with both real corpus inputs, assert exit 0 + manifest contains both. Test 2 (negative): sandbox a bad-syntax YAML as the second positional, assert the error message names the file (not "stdin"). Wired into tests/Makefile test-integration.

/gate review

Verdict: PASS (0 P0, 0 P1, 2 P2; 1 fixed inline, 1 deferred as separate-issue FYI).

Reviewer Findings Status
bar-raiser 1 P2 (dead defensive branch in stdin fallback) + 1 P2-FYI (pre-existing tests/Makefile chain short-circuits on Python runner failure; my test inherits this but works directly) P2 #1 fixed in eb75ec042. P2 FYI deferred — separate concern, can file as a follow-up issue if desired
security 0 findings ship-ready
concurrency skipped (no triggers) n/a
swiftui excluded n/a

Test plan

  • ./tests/strings_compiler_multi_input_test.sh — 2/2 pass
  • make -C tools/strings_compiler builds clean (-Wall -Wextra -std=c99)
  • make -C frontier-cli clean && make -C frontier-cli rebuilds from scratch via new direct multi-input invocation
  • ./tools/run_headless_tests.sh — 580/580 pass
  • cd tests && make test-integration — 2186 pass, 20 baseline failures preserved
  • stdin backward-compat: cat *.yaml | strings_compiler still works (verified directly)

jsavin and others added 2 commits June 4, 2026 19:50
The previous main.c rejected a second positional argument with
"unexpected argument", forcing both Makefiles to work around this
by piping concatenated YAML to stdin via `cat *.yaml | compiler -`.
The workaround compiled correctly because strings_compiler_runtime
accumulates tables across multiple begin_table calls in one stream,
but parse errors lost their source filename and reported "stdin"
with a line number that was the offset into the concatenated stream.

What changed
============

tools/strings_compiler/strings_compiler_main.c
  - Collect positional inputs into a small heap-allocated array
    during arg parsing (grows by doubling, starts at cap=4).
  - Loop after argparse: open + strings_load_yaml_stream per file,
    closing each before the next opens. strings_begin/finish_document
    still brackets the whole multi-file load so cross-file dedup and
    accumulation work exactly as before.
  - Backward compat: zero positional inputs still reads stdin once
    as "stdin" so existing `cat | compiler` invocations continue
    to work (and any other tooling that piped to stdin).
  - Free the inputs array on every exit path.

tools/strings_compiler/strings_yaml_loader.c
  - The libyaml-parser-level error (line 60 pre-fix) ignored
    source_name; it just reported "libyaml parser error: <problem>".
    For multi-input runs this meant the operator could not tell
    which file failed to parse. Updated to format as
    "<source_name>:<line>:<col>: libyaml parser error: <problem>",
    using libyaml's problem_mark (line/col are 0-based; bumped to
    1-based for human-style output). Structural-error branches
    (lines 70+) already prepended source_name; this just brings
    the libyaml-parser branch in line.

frontier-cli/Makefile + tests/Makefile
  - Both reverted from `cat $(STRINGS_INPUTS) | $(STRINGS_COMPILER)
    ... -` to direct invocation `$(STRINGS_COMPILER) ...
    $(STRINGS_INPUTS)`.
  - Workaround explanation comments removed.

tests/strings_compiler_multi_input_test.sh (new)
  - Test 1 (positive): invoke compiler with both real corpus inputs
    (idsystemtablescripts.yaml + langerrorlist.yaml) as separate
    positional args, assert exit 0 and that the manifest contains
    table names from BOTH files. Pre-fix: rejected with "unexpected
    argument". Post-fix: pass.
  - Test 2 (negative): sandbox a deliberately-broken second file,
    assert the parse error names the file (not "stdin"). Pre-fix:
    error reported "libyaml parser error" with no filename. Post-fix:
    error reports "<bad.yaml>:<line>:<col>: libyaml parser error".
  - Wired into tests/Makefile test-integration alongside the other
    shell-based tests.

Closes #681.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…#681)

Addresses bar-raiser P2 from the /gate review of this PR.

The `if (input_cap == 0)` guard inside the zero-positional stdin
fallback was always true. input_cap only grows in the realloc block
that also bumps input_count, so input_count == 0 implies input_cap
== 0 and inputs == NULL. The implicit else (reuse already-allocated
buffer of cap >= 4) was unreachable.

Replaced with a direct malloc and added an invariant comment so the
next reader doesn't have to re-derive the same conclusion.

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

jsavin commented Jun 5, 2026

Copy link
Copy Markdown
Owner Author

/gate Review @ eb75ec042

Verdict: PASS

Tests

  • unit: 580/580 pass
  • integration: 2186 pass, 20 baseline failures preserved (matches develop)
  • multi-input test: 2/2 pass

Reviewers

  • bar-raiser: ran (always) — ship-ready, 1 P2 + 1 P2-FYI
  • security: ran (always) — clean, no findings, "ship it"
  • concurrency: skipped (auto: no triggers matched)
  • swiftui: skipped (excluded)

Findings

# Sev Finding Status
1 P2 strings_compiler_main.c:92 dead defensive branch (input_cap == 0 guard always true) Fixed eb75ec042 — replaced with direct malloc + invariant comment
2 P2 FYI Pre-existing: tests/Makefile chains shell tests after tools/run_integration_tests.sh; the runner's non-zero exit on baseline failures short-circuits the rest. Affects this PR's new test the same way; works correctly when invoked directly. Out of scope for #681 — can file as a separate Makefile-chain follow-up if desired

Security praise (from review)

  • Realloc pattern is textbook-correct (uses resized temporary, preserves original on failure)
  • fclose correctly gated on input != stdin inside the loop body
  • Free-on-every-exit-path conscientiously applied at all 5 early returns

Bar-raiser praise

  • Comment quality in main.c explains the "why" (issue context, history)
  • Test 2's bad YAML genuinely triggers the libyaml-parser branch (the actual fix target)
  • Realloc pattern: textbook-correct via resized temporary
  • Backward compat preserved for stdin pipe callers
  • Error attribution fix uses conventional file:line:col: format

Branch is ready to merge.

@jsavin jsavin merged commit 2b6aad7 into develop Jun 5, 2026
@jsavin jsavin deleted the worktree-strings-compiler-681-multi-input branch June 5, 2026 05:58
@jsavin

jsavin commented Jun 5, 2026

Copy link
Copy Markdown
Owner Author

/auto Summary

What went well: Mechanical refactor with TDD-first (failing test before implementation, then green). Bar-raiser caught a real dead branch in the stdin fallback that I'd written defensively without thinking through the invariant. Fixing it inline kept the diff tight. Security review came back clean with positive observations on the realloc pattern.

What could improve: The pre-existing tests/Makefile chain issue (Python runner's non-zero exit short-circuits the shell tests appended after it was visible during this work. My new test inherits the bug — it doesn't actually run via today because the prior YAML-suite baseline failure blocks it. Worth a separate follow-up to convert the chain to non-short-circuiting form.

Recommendations: File a quick follow-up for the short-circuit issue. Pre-approved memory rule for worktree triggers the destructive-command hook reminder even though it's carved out; the hook output makes that explicit, so no change needed.

🤖 Generated with Claude Code /auto
EOM
)

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.

strings_compiler: accept multiple positional input files

1 participant