Apply per-file config across all CLI apps#373
Merged
Conversation
- Discover nearest goccia.json/json5/toml for each input file - Apply ASI, compat-var, and max-memory from file config unless overridden by CLI - Add coverage for loader, bundler, test runner, and config lookup
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPer-file config discovery/application was added to CLI apps; parser/compile APIs now accept explicit ASI/compat-var booleans sourced from the Engine instance. CI removed large inline shell CLI tests and now runs Bun-based TypeScript CLI test scripts. Changes
Sequence DiagramsequenceDiagram
participant CLI as "CLI App"
participant Discover as "DiscoverFileConfig"
participant Config as "goccia.*"
participant Apply as "ApplyFileConfigToEngine"
participant Engine as "Engine Instance"
participant Parser as "Parser"
CLI->>Discover: DiscoverFileConfig(entryFilename)
Discover->>Config: Search upward for goccia.json/json5/toml
Config-->>Discover: Return TConfigEntryArray (or empty)
CLI->>Apply: ApplyFileConfigToEngine(Engine, CLIFlags, FileConfig)
Apply->>Engine: Set ASIEnabled / VarEnabled / MaxMemory (respecting CLI > File > Default)
CLI->>Parser: ParseSource(..., asi=Engine.ASIEnabled, var=Engine.VarEnabled, ...)
Parser-->>Engine: Use provided flags to configure parsing behavior
Parser-->>CLI: Return parsed program or errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
new feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr.yml:
- Around line 1201-1204: The CI step temporarily disables errexit with "set +e"
then runs "./build/GocciaScriptLoader ... | grep -q 'SyntaxError'"; capture the
pipeline's exit status into a variable (e.g., status=$?) immediately after that
command returns, then re-enable "set -e" and assert/test the captured status
(e.g., test $status -eq 0) so the failure is not ignored; locate the block using
the commands "set +e", "./build/GocciaScriptLoader", "grep -q 'SyntaxError'",
and "set -e" to apply this change.
In `@source/app/Goccia.CLI.Application.pas`:
- Around line 452-460: The per-file max-memory logic is sticky because
AEngineOptions.MaxMemory.Present stays true after the global config is applied
and missing per-file entries leave GC.MaxBytes unchanged; update the branch in
the code around ApplyMaxMemory/AEngineOptions.MaxMemory.Present so you only
apply the global/CLI max-memory when the value was provided via the CLI (i.e.
distinguish CLI-origin vs. already-applied top-level config), otherwise consult
the per-file entry via FindConfigEntry/TryStrToInt64 and set
TGarbageCollector.Instance.MaxBytes to that parsed MemoryLimit; if no per-file
override is present explicitly, explicitly reset GC.MaxBytes to the default
(e.g. 0 or call the GC reset API) instead of leaving the previous value in
place.
- Around line 440-450: The current logic treats AEngineOptions.ASI.Present and
CompatVar.Present as "came from CLI", but Execute() already applied file config
into FAllOptions so Present can be true for config-derived values; update the
branches that set AEngine.ASIEnabled and AEngine.VarEnabled to only honor the
CLI override when the option actually originated from the command line (e.g.,
check an origin/source flag such as an IsFromCLI/Origin field on
AEngineOptions.ASI and .CompatVar) and otherwise fall back to reading the
per-file config via FindConfigEntry('asi', ValueStr) and
FindConfigEntry('compat-var', ValueStr) as currently done.
In `@source/app/GocciaBundler.dpr`:
- Around line 200-215: EngineOptions.ASI.Present and
EngineOptions.CompatVar.Present are being read after ApplyConfigFile mutated
them, so snapshot the original CLI-origin flags before any config application
and use those snapshots when resolving per-file EffectiveASI/EffectiveVar; e.g.,
in TGocciaCLIApplication.Execute capture booleans like CLIOverrideASI and
CLIOverrideCompatVar from EngineOptions.ASI.Present /
EngineOptions.CompatVar.Present before calling ApplyConfigFile, then either pass
those CLI-only booleans into CompileSource (and replace checks of
EngineOptions.ASI.Present / EngineOptions.CompatVar.Present with the new
CLIOverrideXXX variables) or reference the snapshots where EffectiveASI and
EffectiveVar are computed so per-file goccia.* config can override unless the
original CLI flag was truly supplied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74bada72-ea48-4a35-95cc-b5f7bd58cc31
📒 Files selected for processing (8)
.github/workflows/pr.ymlsource/app/Goccia.CLI.Application.passource/app/GocciaBenchmarkRunner.dprsource/app/GocciaBundler.dprsource/app/GocciaScriptLoader.dprsource/app/GocciaTestRunner.dprsource/shared/CLI.ConfigFile.Test.passource/shared/CLI.ConfigFile.pas
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 25 improved · 🔴 342 regressed · 19 unchanged · avg -7.0% arraybuffer.js — Interp: 🔴 12, 2 unch. · avg -8.6% · Bytecode: 🔴 3, 11 unch. · avg -0.2%
arrays.js — Interp: 🔴 18, 1 unch. · avg -10.9% · Bytecode: 🟢 2, 🔴 1, 16 unch. · avg +0.1%
async-await.js — Interp: 🔴 5, 1 unch. · avg -10.5% · Bytecode: 🔴 3, 3 unch. · avg -1.6%
base64.js — Interp: 🔴 9, 1 unch. · avg -11.6% · Bytecode: 🔴 2, 8 unch. · avg +0.3%
classes.js — Interp: 🔴 30, 1 unch. · avg -7.9% · Bytecode: 🟢 11, 20 unch. · avg +1.5%
closures.js — Interp: 🔴 11 · avg -12.0% · Bytecode: 🟢 1, 10 unch. · avg +0.0%
collections.js — Interp: 🟢 3, 🔴 9 · avg -6.0% · Bytecode: 🔴 6, 6 unch. · avg -1.4%
csv.js — Interp: 🔴 13 · avg -8.8% · Bytecode: 🟢 4, 9 unch. · avg +1.7%
destructuring.js — Interp: 🔴 22 · avg -8.6% · Bytecode: 🟢 2, 🔴 3, 17 unch. · avg -0.9%
fibonacci.js — Interp: 🔴 8 · avg -10.2% · Bytecode: 🟢 2, 🔴 2, 4 unch. · avg -0.6%
float16array.js — Interp: 🟢 4, 🔴 25, 3 unch. · avg -3.6% · Bytecode: 🔴 5, 27 unch. · avg -0.9%
for-of.js — Interp: 🔴 7 · avg -7.7% · Bytecode: 🟢 2, 5 unch. · avg +2.5%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 41, 1 unch. · avg -8.0% · Bytecode: 🟢 25, 🔴 2, 15 unch. · avg +2.6%
json.js — Interp: 🔴 20 · avg -10.0% · Bytecode: 🟢 2, 🔴 6, 12 unch. · avg -1.0%
jsx.jsx — Interp: 🔴 20, 1 unch. · avg -8.2% · Bytecode: 🔴 4, 17 unch. · avg -1.5%
modules.js — Interp: 🔴 9 · avg -8.4% · Bytecode: 🔴 3, 6 unch. · avg -1.1%
numbers.js — Interp: 🔴 11 · avg -10.3% · Bytecode: 🟢 2, 🔴 2, 7 unch. · avg -0.4%
objects.js — Interp: 🔴 7 · avg -6.7% · Bytecode: 🟢 1, 🔴 1, 5 unch. · avg +0.1%
promises.js — Interp: 🔴 11, 1 unch. · avg -5.9% · Bytecode: 12 unch. · avg -0.4%
regexp.js — Interp: 🔴 11 · avg -9.2% · Bytecode: 🟢 1, 10 unch. · avg +0.0%
strings.js — Interp: 🔴 17, 2 unch. · avg -15.2% · Bytecode: 🟢 3, 🔴 5, 11 unch. · avg -8.5%
tsv.js — Interp: 🔴 9 · avg -11.0% · Bytecode: 🟢 8, 1 unch. · avg +5.4%
typed-arrays.js — Interp: 🟢 8, 🔴 12, 2 unch. · avg -1.8% · Bytecode: 🟢 4, 🔴 8, 10 unch. · avg -0.2%
uint8array-encoding.js — Interp: 🟢 10, 🔴 5, 3 unch. · avg +20.0% · Bytecode: 🟢 12, 🔴 1, 5 unch. · avg +37.0%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
- Add Bun-based scripts for CLI, config, and app-specific coverage - Simplify CI and PR workflows by replacing long inline shell blocks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-cli-config.ts`:
- Around line 372-393: The test currently only inspects stdout for "asi" but
doesn't verify the process succeeded; for both Bun.spawnSync invocations
(benchInterp and benchBc) check that their exitCode === 0 before asserting
stdout.includes("asi") — if exitCode is non-zero, throw a descriptive Error
indicating the benchmark runner failed (include stderr text) so CI fails fast;
apply the same guard to the other BenchRunner blocks referenced later (the block
that creates benchInterp/benchBc equivalents around the second occurrence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c374e7d3-a774-47e0-a04c-638abdf216e4
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/pr.ymlscripts/test-cli-apps.tsscripts/test-cli-config.tsscripts/test-cli.ts
- Move parser error display and numeric separator tests out of the general CLI suite - Add CI and PR workflow steps for the new parser-focused check
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-cli-parser.ts`:
- Around line 18-24: Replace the single-quoted echo with printf so the loader
receives a real multiline source (e.g. use printf 'const x = 1\nconst y = x +' |
${LOADER} 2>&1) so the caret/multiline rendering is exercised; also strengthen
the subsequent assertions around res/out by verifying the process exit code and
asserting concrete line and column values parsed from the JSON error output
(update the JSON assertions that currently only check presence of line/column to
compare expected numeric values, and add similar line/column checks to the
bytecode test plus an explicit exit-code check) so regressions in reported error
coordinates will fail the test.
- Around line 32-82: The JSON error-path assertions are too lax: replace
undefined checks with numeric-type checks and add exit-code validation;
specifically, in the top SyntaxError block change line/column checks to assert
typeof json.error?.line === "number" and typeof json.error?.column === "number";
in the bytecode SyntaxError block (the block using LOADER with
"--mode=bytecode") add the same typeof checks for line/column and assert the
spawn result status is non-zero (proc.status !== 0); and in the numeric
separator loop (the cases iterating over `cases`) assert proc.status !== 0 and
that typeof json.error?.line === "number" and typeof json.error?.column ===
"number" for each failing literal so regressions in error coordinates and exit
codes are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0110fc94-86e6-41f4-a18c-d66f038e41a4
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/pr.ymlscripts/test-cli-parser.tsscripts/test-cli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test-cli.ts
- Add dedicated lexer test for numeric separator rejection - Keep parser test focused on error display - Update CI and PR workflows to run both scripts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/test-cli-lexer.ts (1)
27-35: Also assert a failing exit status for these lexer rejections.This is the right layer for lex-time numeric-separator failures, but the loop currently accepts any
{ ok:false }payload even if the loader exits 0. That weakens the CLI contract check a bit.Bun.spawnSync()exposessuccess/exitCode, so you can fail fast before parsing JSON. (bun.sh) Based on learnings: Invalid numeric separator placement is rejected at lex time and must be verified externally via ScriptLoader/CLI invocation.♻️ Proposed tweak
for (const [literal, desc] of cases) { const proc = Bun.spawnSync([LOADER, "--output=json"], { stdin: new TextEncoder().encode(`${literal};\n`), stdout: "pipe", stderr: "pipe", }); + if (proc.success) { + throw new Error( + `Numeric separator "${literal}" (${desc}) should fail, but exited 0: ${proc.stdout.toString()}` + ); + } const json = JSON.parse(proc.stdout.toString()); if (json.ok !== false || json.error?.type !== "SyntaxError") { throw new Error(`Numeric separator "${literal}" (${desc}) should be SyntaxError, got ok=${json.ok} type=${json.error?.type}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-cli-lexer.ts` around lines 27 - 35, Check the child process exit status before parsing the JSON: after calling Bun.spawnSync (proc) in the loop in scripts/test-cli-lexer.ts, assert that proc.success is false (or proc.exitCode !== 0) and throw/fail fast if the loader exited successfully; only then parse proc.stdout to JSON and validate json.ok === false and json.error?.type === "SyntaxError". Use the existing Bun.spawnSync return (proc) to perform this early exit check so lexer rejections are verified by the CLI exit status as well as the JSON payload..github/workflows/ci.yml (1)
689-702: Consider centralizing this Bun CLI test fan-out.This exact five-step list now exists here and again in
.github/workflows/pr.ymlLines 227-240. Keeping it in two places makes PR/CI coverage easy to drift the next time a script is added or renamed. A small composite action or reusable workflow would keep both workflows aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 689 - 702, The five identical CI steps ("Check CLI flags across all apps", "Check lexer numeric separator rejection", "Check parser error display", "Check config file loading and per-file config", "Check app-specific CLI features") that run bun scripts (scripts/test-cli.ts, scripts/test-cli-lexer.ts, scripts/test-cli-parser.ts, scripts/test-cli-config.ts, scripts/test-cli-apps.ts) are duplicated in .github/workflows/ci.yml and .github/workflows/pr.yml; extract them into a single reusable workflow or composite action (e.g., .github/workflows/cli-tests.yml or .github/actions/cli-tests/) that runs those five bun commands, then replace the inlined steps in both ci.yml and pr.yml with a single call to that reusable workflow/action (using workflow_call or uses:) so both workflows invoke the same centralized step set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 689-702: The five identical CI steps ("Check CLI flags across all
apps", "Check lexer numeric separator rejection", "Check parser error display",
"Check config file loading and per-file config", "Check app-specific CLI
features") that run bun scripts (scripts/test-cli.ts, scripts/test-cli-lexer.ts,
scripts/test-cli-parser.ts, scripts/test-cli-config.ts,
scripts/test-cli-apps.ts) are duplicated in .github/workflows/ci.yml and
.github/workflows/pr.yml; extract them into a single reusable workflow or
composite action (e.g., .github/workflows/cli-tests.yml or
.github/actions/cli-tests/) that runs those five bun commands, then replace the
inlined steps in both ci.yml and pr.yml with a single call to that reusable
workflow/action (using workflow_call or uses:) so both workflows invoke the same
centralized step set.
In `@scripts/test-cli-lexer.ts`:
- Around line 27-35: Check the child process exit status before parsing the
JSON: after calling Bun.spawnSync (proc) in the loop in
scripts/test-cli-lexer.ts, assert that proc.success is false (or proc.exitCode
!== 0) and throw/fail fast if the loader exited successfully; only then parse
proc.stdout to JSON and validate json.ok === false and json.error?.type ===
"SyntaxError". Use the existing Bun.spawnSync return (proc) to perform this
early exit check so lexer rejections are verified by the CLI exit status as well
as the JSON payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4cb6f59-4118-4d08-9452-63ff5a5d7d93
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/pr.ymlscripts/test-cli-lexer.tsscripts/test-cli-parser.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/test-cli-parser.ts
- test-cli-parser.ts: Use printf for proper multiline input, add exit code checks, strengthen line/column assertions to check numeric type instead of undefined - test-cli-lexer.ts: Assert non-zero exit code before parsing JSON - test-cli-config.ts: Assert BenchmarkRunner exit code before matching stdout Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
goccia.json/json5/tomlfor the file or directory being processed.asi,compat-var, andmax-memory, while keeping CLI flags as the highest-priority override.GocciaScriptLoader,GocciaTestRunner,GocciaBundler, andGocciaBenchmarkRunnerto use the effective engine settings consistently.FindConfigEntryhelpers and unit coverage for config lookup behavior.Testing
source/shared/CLI.ConfigFile.Test.pas: added tests forFindConfigEntryreturning matches, missing keys, and duplicate-key precedence..github/workflows/pr.yml: added integration checks for per-file config discovery inGocciaScriptLoader,GocciaTestRunner,GocciaBundler, and bytecode mode.