Refactor CLIs onto shared application framework#299
Merged
Conversation
- Add shared CLI application, option, parser, and help units - Update ScriptLoader, REPL, BenchmarkRunner, and TestRunner to use them - Document the new CLI lifecycle and architecture
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduce a CLI framework (application base, CLI subclass, option model, parser, help generator, engine setup) and migrate REPL, ScriptLoader, TestRunner, and BenchmarkRunner from manual ParamStr/globals to TGocciaApplication/TGocciaCLIApplication-driven tools and option-driven lifecycles. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant App as TGocciaCLIApplication
participant Builder as BuildAllOptions
participant Parser as ParseCommandLine
participant Options as TGocciaOptionArray
participant Instrument as Coverage/Profiler
participant Engine as CreateEngine/CreateBytecodeBackend
participant Runner as ExecuteWithPaths
Caller->>App: RunApplication(AppClass, Name)
activate App
App->>App: Create / Configure()
App->>Builder: BuildAllOptions()
Builder-->>Options: TGocciaOptionArray
App->>Parser: ParseCommandLine(Options)
activate Parser
Parser->>Options: Apply parsed values
Parser-->>App: Return positional Paths
deactivate Parser
alt Help requested
App->>App: GenerateHelpText(Name, UsageLine, Options)
App->>Caller: Print help & exit
else Normal run
App->>App: Validate()
App->>Instrument: InitializeCoverageIfEnabled(CoverageOptions)
App->>Instrument: InitializeProfilerIfEnabled(ProfilerOptions)
App->>Engine: CreateEngine(...) / CreateBytecodeBackend(...)
App->>Runner: ExecuteWithPaths(Paths)
Runner-->>App: Results
App->>Instrument: ShutdownProfilerIfEnabled(ProfilerOptions)
App->>Instrument: ShutdownCoverageIfEnabled(CoverageOptions)
end
App-->>Caller: Exit code
deactivate App
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TestRunner.dpr (1)
655-669:⚠️ Potential issue | 🔴 CriticalKeep
AllTestResultsrooted until after the final property writes.The temp roots are removed before the last
AssignProperty(...)calls. Those allocations can trigger GC, which leavesAllTestResults/AllFailedTestscollectible while this function is still mutating them.As per coding guidelines, "Use AddTempRoot/RemoveTempRoot to protect values held only by Pascal code (not in any GocciaScript scope)".🛡️ Suggested fix
- if Assigned(GC) then - begin - GC.RemoveTempRoot(AllTestResults); - GC.RemoveTempRoot(AllFailedTests); - end; - AllTestResults.AssignProperty('totalTests', TGocciaNumberLiteralValue.Create(AFiles.Count * 1.0)); AllTestResults.AssignProperty('passed', TGocciaNumberLiteralValue.Create(PassedCount)); AllTestResults.AssignProperty('failed', TGocciaNumberLiteralValue.Create(FailedCount)); AllTestResults.AssignProperty('skipped', TGocciaNumberLiteralValue.Create(SkippedCount)); AllTestResults.AssignProperty('totalRunTests', TGocciaNumberLiteralValue.Create(TotalRunCount)); AllTestResults.AssignProperty('duration', TGocciaNumberLiteralValue.Create(TotalDuration)); AllTestResults.AssignProperty('assertions', TGocciaNumberLiteralValue.Create(TotalAssertions)); Result.TestResult := AllTestResults; + + if Assigned(GC) then + begin + GC.RemoveTempRoot(AllTestResults); + GC.RemoveTempRoot(AllFailedTests); + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 655 - 669, The temp roots for AllTestResults and AllFailedTests are removed too early (GC.RemoveTempRoot is called before the final AssignProperty calls), risking collection during property writes; move the GC.RemoveTempRoot(AllTestResults) and GC.RemoveTempRoot(AllFailedTests) calls to after all AssignProperty(...) invocations and after Result.TestResult := AllTestResults so the values remain rooted through the final mutations (ensure the existing AddTempRoot calls still surround this region).
🧹 Nitpick comments (2)
units/Goccia.CLI.Help.pas (1)
43-49: Remove the fixed 32-group ceiling.This helper writes into a fixed
array[0..31], so the 33rd help group will raise a range-check error. Sizing the backing array toLength(AOptions)keeps the function bounded without baking in an arbitrary crash threshold.♻️ Suggested refactor
const - MAX_GROUPS = 32; COLUMN_GAP = 2; var Buffer: TStringBuffer; - Groups: array[0..MAX_GROUPS - 1] of TGroupEntry; + Groups: array of TGroupEntry; GroupCount: Integer; MaxWidth, I, J, GroupIndex: Integer; FormattedName, Header, Padding: string; Option: TGocciaOptionBase; begin GroupCount := 0; MaxWidth := 0; + SetLength(Groups, Length(AOptions));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.CLI.Help.pas` around lines 43 - 49, The code currently declares a fixed-size Groups array (Groups: array[0..MAX_GROUPS - 1] of TGroupEntry) which will raise a range-check error for >32 help groups; change Groups to a dynamic array (e.g., Groups: array of TGroupEntry or TArray<TGroupEntry>) and call SetLength(Groups, Length(AOptions)) (and initialize GroupCount := Length(AOptions) or maintain GroupCount based on Length(Groups)) before populating it; also remove or ignore the MAX_GROUPS constant and ensure any loops use Length(Groups) rather than the fixed bound to avoid out-of-range accesses while keeping Buffer: TStringBuffer and GroupCount references consistent.units/Goccia.CLI.Parser.Test.pas (1)
42-66: Add directParseCommandLinecoverage.This suite validates option objects and help rendering, but it never calls the parser itself. Regressions in
--opt=value,--opt value, short flags, positional-path capture, and unknown-option handling would currently slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.CLI.Parser.Test.pas` around lines 42 - 66, Add direct parser tests by extending TCLIOptionsTests.SetupTests to register new test cases that call ParseCommandLine; specifically add tests like TestParseOptionEquals, TestParseOptionSeparated, TestShortFlagParsing, TestPositionalPathCapture, and TestUnknownOptionHandling which invoke ParseCommandLine with crafted argv arrays and assert expected Option.Present/Value, positional captures, and proper error raising (TGocciaParseError) for unknown options; locate the registration area in TCLIOptionsTests.SetupTests and add Test('...', TestParseOptionEquals) etc., and implement the corresponding test methods to exercise ParseCommandLine, covering --opt=value, --opt value, -f short flags, positional paths, and unknown-option behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 111-112: Update the CLI pipeline doc to include the Configure step
(placed before ParseCommandLine) so it matches TGocciaCLIApplication behavior:
change the pipeline line to "Options → Configure → ParseCommandLine → Validate →
InitializeSingletons → ExecuteWithPaths → AfterExecute → ShutdownSingletons" and
ensure the documentation notes that Configure is where option sets are
registered (as TGocciaCLIApplication does).
In `@docs/architecture.md`:
- Around line 77-84: The tool-mapping table in the documentation is not
separated by blank lines and triggers MD058; add a single blank line before the
table markdown block and a single blank line after it so the table is isolated
(the block containing the header row and the rows for REPL, ScriptLoader,
TestRunner, BenchmarkRunner must be surrounded by empty lines).
In `@docs/code-style.md`:
- Around line 271-276: The docs currently state enum prefixes and space-form
behavior as universal rules; update the wording to describe them as
defaults/opt-ins instead: explain that TGocciaEnumOption<T> derives CLI values
by stripping a prefix but the prefix length is configurable (not limited to 2
characters) and that AcceptsSpaceForm enables the `--opt value` form (it is not
automatic for all repeatable options), and add a specific note that some options
(e.g., BenchmarkRunner’s `--format`) explicitly disable the space form;
reference TGocciaEnumOption<T>, AcceptsSpaceForm, and BenchmarkRunner `--format`
in the prose so readers know where exceptions apply.
In `@REPL.dpr`:
- Around line 88-96: ParseCommandLine returns Positionals but the REPL ignores
them; add validation after Positionals := ParseCommandLine(AllOptions) to reject
any unexpected positional arguments by checking Positionals.Count and, if >0,
print a clear error and Exit (same pattern used for Help.Present). Update both
places where Positionals is used (the startup block around IsBytecodeMode and
the other occurrence around lines 270-271) to surface the error and stop startup
instead of silently dropping arguments.
In `@units/Goccia.CLI.Application.pas`:
- Around line 184-190: The current InitializeSingletons only calls
InitializeCoverageIfEnabled when FCoverageOptions.Enabled.Present is true, which
skips initialization when only coverage-format or coverage-output are provided;
update TGocciaCLIApplication.InitializeSingletons to call
InitializeCoverageIfEnabled whenever FCoverageOptions is assigned and any of the
coverage-related flags/options are present (e.g.,
FCoverageOptions.Enabled.Present OR FCoverageOptions.Format.Present OR
FCoverageOptions.Output.Present), ensuring
InitializeCoverageIfEnabled(FCoverageOptions) runs for --coverage-format and
--coverage-output as well as --coverage.
In `@units/Goccia.CLI.Options.pas`:
- Around line 305-308: TGocciaRepeatableOption.FormatForHelp currently always
returns '--' + LongName + ' <value>' which misdocuments options when
AcceptsSpaceForm is false; update FormatForHelp to check the instance's
AcceptsSpaceForm flag and render the help string accordingly (use '--' +
LongName + ' <value>' when AcceptsSpaceForm = True, and use '--' + LongName +
'=<value>' when AcceptsSpaceForm = False) so callers like FGlobalFiles (which
sets AcceptsSpaceForm := False) produce correct help text for repeatable
options.
In `@units/Goccia.CLI.Parser.pas`:
- Around line 92-100: In the space-form branch for repeatable options, avoid
consuming a following token that is itself an option flag (e.g.
"--mode=bytecode"); before Inc(I) / Value := ParamStr(I) check that ParamStr(I)
exists and does not start with '-' (or otherwise look like an option), and if it
does look like an option throw TGocciaParseError.CreateFmt('--%s requires a
value', [Name]) instead of accepting it; update the logic around
TGocciaRepeatableOption, Count, I, ParamStr and the code path that eventually
calls Apply so option-shaped tokens are rejected as values.
---
Outside diff comments:
In `@TestRunner.dpr`:
- Around line 655-669: The temp roots for AllTestResults and AllFailedTests are
removed too early (GC.RemoveTempRoot is called before the final AssignProperty
calls), risking collection during property writes; move the
GC.RemoveTempRoot(AllTestResults) and GC.RemoveTempRoot(AllFailedTests) calls to
after all AssignProperty(...) invocations and after Result.TestResult :=
AllTestResults so the values remain rooted through the final mutations (ensure
the existing AddTempRoot calls still surround this region).
---
Nitpick comments:
In `@units/Goccia.CLI.Help.pas`:
- Around line 43-49: The code currently declares a fixed-size Groups array
(Groups: array[0..MAX_GROUPS - 1] of TGroupEntry) which will raise a range-check
error for >32 help groups; change Groups to a dynamic array (e.g., Groups: array
of TGroupEntry or TArray<TGroupEntry>) and call SetLength(Groups,
Length(AOptions)) (and initialize GroupCount := Length(AOptions) or maintain
GroupCount based on Length(Groups)) before populating it; also remove or ignore
the MAX_GROUPS constant and ensure any loops use Length(Groups) rather than the
fixed bound to avoid out-of-range accesses while keeping Buffer: TStringBuffer
and GroupCount references consistent.
In `@units/Goccia.CLI.Parser.Test.pas`:
- Around line 42-66: Add direct parser tests by extending
TCLIOptionsTests.SetupTests to register new test cases that call
ParseCommandLine; specifically add tests like TestParseOptionEquals,
TestParseOptionSeparated, TestShortFlagParsing, TestPositionalPathCapture, and
TestUnknownOptionHandling which invoke ParseCommandLine with crafted argv arrays
and assert expected Option.Present/Value, positional captures, and proper error
raising (TGocciaParseError) for unknown options; locate the registration area in
TCLIOptionsTests.SetupTests and add Test('...', TestParseOptionEquals) etc., and
implement the corresponding test methods to exercise ParseCommandLine, covering
--opt=value, --opt value, -f short flags, positional paths, and unknown-option
behavior.
🪄 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: eb23f5bf-9dd4-4fd6-bf0e-62ef1ab599bf
📒 Files selected for processing (15)
AGENTS.mdBenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprdocs/architecture.mddocs/code-style.mddocs/embedding.mdunits/Goccia.Application.pasunits/Goccia.CLI.Application.pasunits/Goccia.CLI.EngineSetup.pasunits/Goccia.CLI.Help.pasunits/Goccia.CLI.Options.pasunits/Goccia.CLI.Parser.Test.pasunits/Goccia.CLI.Parser.pas
Comment on lines
+305
to
+308
| function TGocciaRepeatableOption.FormatForHelp: string; | ||
| begin | ||
| Result := '--' + LongName + ' <value>'; | ||
| end; |
Contributor
There was a problem hiding this comment.
Make help text reflect AcceptsSpaceForm.
FormatForHelp always renders repeatable options as --name <value>, but some callers disable the space form. For example, ScriptLoader.dpr sets FGlobalFiles.AcceptsSpaceForm := False, so the generated help currently documents a syntax the parser rejects.
✏️ Suggested fix
function TGocciaRepeatableOption.FormatForHelp: string;
begin
- Result := '--' + LongName + ' <value>';
+ if FAcceptsSpaceForm then
+ Result := '--' + LongName + ' <value>'
+ else
+ Result := '--' + LongName + '=<value>';
end;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function TGocciaRepeatableOption.FormatForHelp: string; | |
| begin | |
| Result := '--' + LongName + ' <value>'; | |
| end; | |
| function TGocciaRepeatableOption.FormatForHelp: string; | |
| begin | |
| if FAcceptsSpaceForm then | |
| Result := '--' + LongName + ' <value>' | |
| else | |
| Result := '--' + LongName + '=<value>'; | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.CLI.Options.pas` around lines 305 - 308,
TGocciaRepeatableOption.FormatForHelp currently always returns '--' + LongName +
' <value>' which misdocuments options when AcceptsSpaceForm is false; update
FormatForHelp to check the instance's AcceptsSpaceForm flag and render the help
string accordingly (use '--' + LongName + ' <value>' when AcceptsSpaceForm =
True, and use '--' + LongName + '=<value>' when AcceptsSpaceForm = False) so
callers like FGlobalFiles (which sets AcceptsSpaceForm := False) produce correct
help text for repeatable options.
Owner
Author
There was a problem hiding this comment.
AcceptsSpaceForm was removed entirely in a previous commit. All repeatable options now accept space-form, all other value options require =. Help text is accurate.
Contributor
There was a problem hiding this comment.
@frostney, understood — thanks for the clarification! Since AcceptsSpaceForm has been removed entirely, all repeatable options consistently accept space-form and the --name <value> help text is accurate. My comment no longer applies.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: units/Goccia.CLI.Options.pas:303-306
Timestamp: 2026-04-14T06:46:05.407Z
Learning: In frostney/GocciaScript (`units/Goccia.CLI.Options.pas`), `TGocciaRepeatableOption` does NOT have an `AcceptsSpaceForm` property — it was removed. All repeatable options accept space-form syntax (`--name value`), and all other value options require `=` syntax (`--name=value`). The `FormatForHelp` output of `--name <value>` for repeatable options is intentionally correct. Do not flag this as mismatched help text in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 272
File: units/Goccia.Values.TextDecoderValue.pas:175-214
Timestamp: 2026-04-11T17:28:36.950Z
Learning: In frostney/GocciaScript, `TGocciaArgumentValidator` only validates argument *counts* (RequireExactly, RequireAtLeast, RequireAtMost). It does NOT provide per-value type inspection (e.g., is-undefined, is-string, is-object). Therefore, in `InitializeNativeFromArguments` overrides (and similar constructor init paths) where all arguments are optional and correctness depends on value/type checks rather than count checks, manual checks (e.g., `OptionsArg is TGocciaUndefinedLiteralValue`, `OptionsArg is TGocciaObjectValue`) are correct and should not be replaced or flagged as needing `TGocciaArgumentValidator`. Do not re-flag this pattern in `units/Goccia.Values.*.pas` constructor/init methods when the validation is inherently about value kinds, not argument counts.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T07:58:33.798Z
Learning: Applies to **/*.pas : Use named type aliases for generic list specializations (e.g., TGocciaValueList = TObjectList<TGocciaValue>)
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:0-0
Timestamp: 2026-03-08T04:50:29.540Z
Learning: In format.pas (GocciaScript formatter), ParseParamNames previously misidentified parenthesized arguments inside one-liner function bodies as parameter names, causing the `A` prefix to be prepended to them. The fix (f5b2f23) makes ParseParamNames ignore parentheses appearing after the first semicolon. Be cautious when reviewing one-liner procedure/function bodies in .pas files for incorrectly prefixed identifiers — this was the root cause of the AGC_ANY_CONVERT_EXTERN → AGC_ANY_CONVERT_EXTERN typos in souffle/Souffle.Wasm.Emitter.pas.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : Format-defined newline semantics (e.g., TOML multiline strings, YAML folding) must be implemented explicitly and not rely on `LineEnding`/`sLineBreak`; add regression coverage with explicit `#13#10` / `\r\n` inputs
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : `./format.pas` auto-fixes uses clause ordering, PascalCase function names, and parameter `A` prefix naming
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:0-0
Timestamp: 2026-03-08T04:50:21.036Z
Learning: In souffle/Souffle.Wasm.Emitter.pas (FreePascal), a code formatter has a known bug where it incorrectly prepends an `A` prefix to opcode constant names inside single-line method bodies (e.g. `begin EmitByte(OP_FOO); end`), turning `OP_FOO` into `AOP_FOO`. This affects all one-liner EmitXxx methods. When reviewing this file, check that no `AOP_*` identifiers appear; they are always formatter-introduced typos and should be `OP_*`.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : Use `Goccia.Constants.*` split constant units instead of hardcoded string literals for property names, type names, error names, constructor names, and symbol names
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.694Z
Learning: Applies to **/*.pas : Use split constant units for runtime constants instead of hardcoded string literals: `Goccia.Constants.PropertyNames` for property names, `Goccia.Constants.TypeNames` for type names, `Goccia.Constants.ErrorNames` for error names, `Goccia.Constants.ConstructorNames` for constructor names, and `Goccia.Constants.SymbolNames` for symbol names.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : `type`/`interface` declarations and `import type`/`export type` must produce `TGocciaEmptyStatement` (no-op at runtime). Access modifiers (`public`, `protected`, `private`, `readonly`, `override`, `abstract`) in class bodies must be consumed and discarded.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : Use `TObjectList<T>` over `TList<T>` when `T` is a class; define named type aliases for generic specializations used across multiple units (e.g., `TGocciaValueList = TObjectList<TGocciaValue>`)
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.Coverage.pas:225-249
Timestamp: 2026-04-07T20:24:40.351Z
Learning: In `units/Goccia.Coverage.pas` `BuildExecutableLineFlags`: `interface Foo {` opening lines are intentionally marked as executable (`AFlags[I] := True`) because they produce `TGocciaEmptyStatement` which the engine does evaluate; interface body lines are suppressed via `TypeBodyDepth`. Single-line `type` aliases (e.g. `type Point = { x: number };`) are also correctly counted as executable for the same reason. The only known gap is multi-line `type` aliases spanning several lines — those continuation lines are not currently suppressed, but this is considered rare in practice and acceptable for now. Do not flag the `interface` opening-line executable marking or single-line `type` handling as bugs.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to **/*.pas : All Pascal units must include `{$I Goccia.inc}` compiler directive.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:140-156
Timestamp: 2026-04-12T21:19:18.811Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr, TestRunner.dpr, BenchmarkRunner.dpr): suppressing status/progress WriteLn messages in JSON output mode is intentionally handled per-tool for now. The long-term goal is to unify the stdout/JSON-output suppression pattern across all CLI tools. In ScriptLoader, status lines must be gated on `not GJsonOutput` because `--output=json` routes the result document directly to stdout. TestRunner and BenchmarkRunner write JSON to a file, so their current unconditional WriteLn to stdout is safe but will need to align with the unified pattern in a future refactor.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:08:41.070Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas
Maintainer preference: FBlueprintBridgeCache is intentionally shared between UnwrapToGocciaValue (which uses BlueprintToClassValue) and Construct (which uses ConvertBlueprintToClassValue); this pre‑existing pattern on main is acceptable and should not be re‑flagged in future reviews unless a functional bug is demonstrated.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:46.617Z
Learning: GocciaScript’s Promise statics are intentionally not species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics. This applies to units/Goccia.Runtime.Operations.pas and related Promise implementations; ensure consistency across related Pas files in the frostney/GocciaScript repository.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:7514-7544
Timestamp: 2026-03-11T08:22:20.482Z
Learning: When bridging a native namespace object to a TSouffleRecord (e.g., performance) in units/Goccia.Runtime.Operations.pas, always set Result.Delegate := ARuntime.VM.RecordDelegate after TSouffleRecord.Create(...). This ensures Object.prototype methods (toString/hasOwnProperty/valueOf/toLocaleString) work in bytecode mode and NativeRecordToString honors Symbol.toStringTag. Apply this pattern to BuildPerformanceRecord and any future bridged records.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/HashMap.pas:266-309
Timestamp: 2026-03-17T22:17:37.410Z
Learning: Do not flag enumeration in THashMap<TKey, TValue> as concurrent-modification unsafe. The no-mutation contract is maintained during enumeration: GetEnumerator shares the internal FSlots array (no per-call allocation) and only mutation occurs via destructor or Clear, not during active iteration. This pattern should be recognized across similar Pascal units/pascal collection implementations; when reviewing code, ensure no concurrent-modification warnings are raised for shared internal structures during enumeration if the no-mutation contract is preserved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/OrderedMap.pas:0-0
Timestamp: 2026-03-17T22:17:55.588Z
Learning: Enforce the constraint on TOrderedMap<TKey, TValue> in units/OrderedMap.pas: TKey must be an unmanaged, fixed-size value type (no strings, dynamic arrays, or records with managed/pointer fields). HashKey should use fast paths for SizeOf(TKey)=8 (QWord) and SizeOf(TKey)=4 (Cardinal); KeysEqual should compare via direct PQWord/PCardinal for those sizes. The raw-byte DJB2/CompareMem fallback is dead code for current uses. Document this constraint in a unit-header comment as the enforcement mechanism. Do NOT re-flag the byte-wise hash/equality in future reviews as long as all TOrderedMap specializations use pointer-sized unmanaged keys.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:00.984Z
Learning: Do not re-introduce TScopeMap. In frostney/GocciaScript, TScopeMap<TValue> was removed due to performance regressions from its linear-scan Add-as-upsert behavior. Reviews should flag any use of TScopeMap in Units/ScopeMap.pas or elsewhere and ensure bindings use TOrderedStringMap<TLexicalBinding> instead. If reviewing similar Pascal files, prefer avoiding upsert-like add semantics that overwrite existing entries, as they correlated with bytecode performance drops. Ensure changes adhere to the established replacement: TOrderedStringMap<TLexicalBinding>.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:4349-4353
Timestamp: 2026-03-23T12:38:54.412Z
Learning: In Pascal source under units/, when decoding JSON string escape sequences that represent Unicode code points and then converting them to UTF-8, prefer using the existing helper (implemented via CodePointToUTF8) that correctly combines UTF-16 surrogate pairs (\uD800–\uDBFF + \uDC00–\uDFFF) into a single Unicode code point before UTF-8 encoding. Avoid ad-hoc conversions like Char(StrToInt(...)) for code-point→UTF-8 conversion logic; use the helper instead to ensure surrogate pairs are handled correctly.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:6337-6344
Timestamp: 2026-03-23T15:57:55.802Z
Learning: In Pascal unit files under units/, any iterator result record objects produced by native iterators (e.g., in routines analogous to NativeMapIteratorNext / NativeSetIteratorNext) must: (1) set `Rec.Delegate := FVM.RecordDelegate`; and (2) set properties using `PROP_VALUE` / `PROP_DONE` constants from `Goccia.Constants.PropertyNames` rather than hard-coded string literals. Apply the same pattern to any future native iterator result objects so they interoperate correctly in bytecode mode (e.g., for `Object.prototype` method behavior) and keep property-name usage consistent via the shared constants.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 136
File: docs/adding-built-in-types.md:105-133
Timestamp: 2026-03-27T14:09:25.371Z
Learning: In frostney/GocciaScript Pascal code, do not flag or suggest refactoring the early-exit singleton initialization guard `if Assigned(FShared) then Exit` when it appears at the top of `InitializePrototype`. The project documentation treats this early-exit form as the canonical/shared-prototype singleton initialization template (i.e., avoid recommending an `if not Assigned(FShared) then ...` wrapping alternative).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 189
File: units/Goccia.JSON.pas:499-526
Timestamp: 2026-04-05T17:33:32.709Z
Learning: In frostney/GocciaScript, when using `TStringBuffer` from the `StringBuffer` unit, treat it as an advanced record (stack-allocated) with automatic cleanup by the compiler. For `TStringBuffer` locals, do not require or flag missing `SB.Free` calls in `try/finally` blocks—no destructor/free call is needed or appropriate. Only look for `Free` when the variable is actually a heap-allocated class instance.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 189
File: units/Goccia.JSON.pas:535-597
Timestamp: 2026-04-05T17:33:35.381Z
Learning: When reviewing Free Pascal code in this repo, treat `TStringBuffer` (from the `StringBuffer` unit) as an advanced record (value type), meaning instances are stack-allocated. Do not report or require `SB.Free` / `SB.FreeAndNil` (or any `Free` call) for variables typed as `TStringBuffer`, and do not treat an empty or no-op `finally` block after `TStringBuffer` usage as an issue. Only expect heap-management calls for actual `class`/reference types; calling `Free` on an advanced record is incorrect.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.AST.Expressions.pas:1339-1346
Timestamp: 2026-04-07T21:00:57.707Z
Learning: When reviewing GocciaScript coverage/branch tracking, do not require call-sites to explicitly pre-register binary (two-arm) branches before recording hits. In units/Goccia.Coverage.pas, TGocciaFileCoverage.RecordBranchHit automatically ensures the opposite arm exists via EnsureBranchExists(ALine, AColumn, 1 - ABranchIndex), establishing a zero-hit baseline for the paired arm whenever either arm is first recorded. Therefore, call-sites such as TGocciaConditionalExpression.Evaluate (Goccia.AST.Expressions.pas), EvaluateIf/EvaluateBinary/EvaluateSwitch (Goccia.Evaluator.pas), and OP_JUMP_IF_* handlers (Goccia.VM.pas) should not be flagged for missing a separate RegisterBranch call for binary branches—pairing is handled inside RecordBranchHit.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.VM.pas:3547-3561
Timestamp: 2026-04-07T21:01:16.129Z
Learning: When reviewing calls that record branch coverage via `TGocciaFileCoverage.RecordBranchHit(ALine, AColumn, ABranchIndex)` for binary branches (branch indices 0 and 1), do not require an explicit prior `RegisterBranch(...)` at the call site. `RecordBranchHit` internally calls `EnsureBranchExists(ALine, AColumn, 1 - ABranchIndex)` for binary branches, which guarantees both arms exist in the branch list (the untaken arm is pre-registered with zero hits). Therefore VM/evaluator/AST branch recording sites in `Goccia.VM.pas`, `Goccia.Evaluator.pas`, and `Goccia.AST.Expressions.pas` should not be flagged for missing explicit pre-registration before `RecordBranchHit(...)`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 273
File: units/Goccia.Values.URLValue.pas:465-470
Timestamp: 2026-04-11T17:42:27.330Z
Learning: In this codebase, when implementing or updating a `TGCManagedObject.MarkReferences` override for a held reference, treat `MarkReferences` as the act of marking *and* tracing children. The expected pattern is: after a nil check, call the held reference’s `MarkReferences` implementation directly (e.g., `TGocciaSomeValue(FRef).MarkReferences`) rather than invoking any non-existent separate `Mark(obj)`/`Mark()` helper. Do not suggest “call `Mark(obj)`”; this project’s marking flow starts by setting `FGCMark := FCurrentMark` and then tracing child references from within `MarkReferences` (consistent with `TGocciaMapValue`, `TGocciaSetValue`, `TGocciaArrayValue`, etc.).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 271
File: units/Goccia.Builtins.GlobalObject.pas:104-129
Timestamp: 2026-04-11T19:43:06.627Z
Learning: In frostney/GocciaScript, `TGarbageCollector.AddTempRoot`/`RemoveTempRoot` are set-membership (not reference-counted). In built-in method implementations, ONLY call `AddTempRoot`/`RemoveTempRoot` if the value was freshly boxed by `ToObject` (i.e., the argument was not already a `TGocciaObjectValue`). Do not unconditionally add/remove temp roots for values passed in as method arguments, since a single `RemoveTempRoot(Obj)` will remove the root for all holders and can cause use-after-free on the next GC cycle. Pattern: `if Assigned(TGarbageCollector.Instance) and not (AArgs.GetElement(0) is TGocciaObjectValue) then AddTempRoot(Obj); try ... finally if Assigned(TGarbageCollector.Instance) and not (AArgs.GetElement(0) is TGocciaObjectValue) then RemoveTempRoot(Obj); end;`
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1071-1111
Timestamp: 2026-04-12T07:39:20.695Z
Learning: Do not free the result of `TGocciaLexer.ScanTokens` (e.g., in `ParseInterpolationExpression` and any other caller). `ScanTokens` returns a reference to the lexer's internal `FTokens: TObjectList<TGocciaToken>` created with `OwnsObjects=True` and freed by `TGocciaLexer`’s destructor. Adding `Tokens.Free` (or otherwise freeing the returned list/object) will cause double-free/access violations; token lifetime is managed entirely by the `TGocciaLexer` instance.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 60 improved · 🔴 84 regressed · 220 unchanged · avg -0.3% arraybuffer.js — Interp: 🔴 2, 12 unch. · avg -0.3% · Bytecode: 🟢 1, 🔴 10, 3 unch. · avg -1.4%
arrays.js — Interp: 🟢 1, 🔴 6, 12 unch. · avg -0.3% · Bytecode: 🟢 3, 🔴 6, 10 unch. · avg -0.2%
async-await.js — Interp: 🟢 1, 🔴 3, 2 unch. · avg -0.5% · Bytecode: 🔴 4, 2 unch. · avg -1.7%
base64.js — Interp: 🟢 2, 🔴 2, 6 unch. · avg +0.3% · Bytecode: 🟢 1, 🔴 7, 2 unch. · avg -1.4%
classes.js — Interp: 🟢 7, 🔴 1, 23 unch. · avg +0.4% · Bytecode: 🟢 2, 29 unch. · avg +1.8%
closures.js — Interp: 🟢 2, 🔴 3, 6 unch. · avg -0.9% · Bytecode: 🟢 2, 9 unch. · avg +1.2%
collections.js — Interp: 🟢 6, 🔴 2, 4 unch. · avg +0.6% · Bytecode: 🟢 6, 🔴 2, 4 unch. · avg +1.2%
destructuring.js — Interp: 🟢 5, 17 unch. · avg +0.4% · Bytecode: 🟢 11, 🔴 3, 8 unch. · avg +2.4%
fibonacci.js — Interp: 🔴 3, 5 unch. · avg -1.4% · Bytecode: 🟢 4, 🔴 2, 2 unch. · avg +0.7%
float16array.js — Interp: 🟢 11, 🔴 5, 16 unch. · avg +0.3% · Bytecode: 🟢 16, 🔴 8, 8 unch. · avg +1.3%
for-of.js — Interp: 🟢 1, 🔴 1, 5 unch. · avg -0.0% · Bytecode: 🟢 4, 🔴 1, 2 unch. · avg -0.8%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 2, 🔴 8, 32 unch. · avg -0.6% · Bytecode: 🟢 18, 🔴 3, 21 unch. · avg +2.1%
json.js — Interp: 🟢 3, 🔴 2, 15 unch. · avg +0.1% · Bytecode: 🟢 14, 6 unch. · avg +4.2%
jsx.jsx — Interp: 🟢 1, 🔴 13, 7 unch. · avg -1.4% · Bytecode: 🟢 4, 🔴 3, 14 unch. · avg +0.8%
modules.js — Interp: 🟢 1, 🔴 6, 2 unch. · avg -1.5% · Bytecode: 🟢 2, 🔴 6, 1 unch. · avg -1.1%
numbers.js — Interp: 🔴 2, 9 unch. · avg -0.6% · Bytecode: 🟢 5, 🔴 2, 4 unch. · avg +1.4%
objects.js — Interp: 🔴 4, 3 unch. · avg -1.5% · Bytecode: 🟢 3, 🔴 1, 3 unch. · avg +1.8%
promises.js — Interp: 🟢 3, 🔴 1, 8 unch. · avg +0.0% · Bytecode: 🟢 8, 🔴 1, 3 unch. · avg +1.9%
regexp.js — Interp: 🟢 1, 🔴 2, 8 unch. · avg -0.5% · Bytecode: 🟢 6, 🔴 2, 3 unch. · avg +2.4%
strings.js — Interp: 🔴 3, 16 unch. · avg -0.8% · Bytecode: 🟢 12, 🔴 2, 5 unch. · avg +3.5%
typed-arrays.js — Interp: 🟢 7, 🔴 8, 7 unch. · avg -0.8% · Bytecode: 🟢 9, 🔴 6, 7 unch. · avg -0.4%
uint8array-encoding.js — Interp: 🟢 6, 🔴 7, 5 unch. · avg -0.2% · Bytecode: 🟢 11, 🔴 3, 4 unch. · avg +3.3%
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. |
- Move ScriptLoader, TestRunner, and BenchmarkRunner onto `TGocciaApplication` - Let CLI subclasses supply their own usage line and simplify option handling - Keep garbage collector lifecycle outside the shared app wrapper
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 `@TestRunner.dpr`:
- Around line 133-140: EscapeJSONString currently only replaces \, ", newline,
carriage return and tab, but must also escape all control characters
U+0000..U+001F (e.g., `#0`, `#8` backspace, `#12` form feed) into their JSON-safe
forms; update EscapeJSONString to iterate over the input string and build the
result character-by-character: first handle backslash and double-quote as
existing, map known single-character escapes (\b, \f, \n, \r, \t) to their short
sequences, and for any character with Ord <= 31 emit a \u00XX hex escape,
leaving other characters unchanged; ensure the function still returns correct
UTF-8/Unicode behavior for non-control characters.
🪄 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: 4c652374-958a-4591-8100-c57728748878
📒 Files selected for processing (5)
BenchmarkRunner.dprScriptLoader.dprTestRunner.dprunits/Goccia.Application.pasunits/Goccia.CLI.Application.pas
✅ Files skipped from review due to trivial changes (2)
- units/Goccia.Application.pas
- ScriptLoader.dpr
🚧 Files skipped from review as they are similar to previous changes (1)
- BenchmarkRunner.dpr
Comment on lines
+133
to
+140
| function EscapeJSONString(const S: string): string; | ||
| begin | ||
| Result := StringReplace(S, '\', '\\', [rfReplaceAll]); | ||
| Result := StringReplace(Result, '"', '\"', [rfReplaceAll]); | ||
| Result := StringReplace(Result, #10, '\n', [rfReplaceAll]); | ||
| Result := StringReplace(Result, #13, '\r', [rfReplaceAll]); | ||
| Result := StringReplace(Result, #9, '\t', [rfReplaceAll]); | ||
| end; |
Contributor
There was a problem hiding this comment.
JSON escaping is incomplete for control characters.
EscapeJSONString escapes \, ", \n, \r, and \t, but JSON requires all control characters (U+0000–U+001F) to be escaped. Characters like backspace (#8), form feed (#12), or NUL (#0) in test output would produce invalid JSON.
🩹 Proposed fix to escape all control characters
function EscapeJSONString(const S: string): string;
+var
+ I: Integer;
+ C: Char;
+ SB: string;
begin
- Result := StringReplace(S, '\', '\\', [rfReplaceAll]);
- Result := StringReplace(Result, '"', '\"', [rfReplaceAll]);
- Result := StringReplace(Result, `#10`, '\n', [rfReplaceAll]);
- Result := StringReplace(Result, `#13`, '\r', [rfReplaceAll]);
- Result := StringReplace(Result, `#9`, '\t', [rfReplaceAll]);
+ SB := '';
+ for I := 1 to Length(S) do
+ begin
+ C := S[I];
+ case C of
+ '\': SB := SB + '\\';
+ '"': SB := SB + '\"';
+ `#8`: SB := SB + '\b';
+ `#9`: SB := SB + '\t';
+ `#10`: SB := SB + '\n';
+ `#12`: SB := SB + '\f';
+ `#13`: SB := SB + '\r';
+ `#0`..#7, `#11`, `#14`..#31:
+ SB := SB + Format('\u%.4x', [Ord(C)]);
+ else
+ SB := SB + C;
+ end;
+ end;
+ Result := SB;
end;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function EscapeJSONString(const S: string): string; | |
| begin | |
| Result := StringReplace(S, '\', '\\', [rfReplaceAll]); | |
| Result := StringReplace(Result, '"', '\"', [rfReplaceAll]); | |
| Result := StringReplace(Result, #10, '\n', [rfReplaceAll]); | |
| Result := StringReplace(Result, #13, '\r', [rfReplaceAll]); | |
| Result := StringReplace(Result, #9, '\t', [rfReplaceAll]); | |
| end; | |
| function EscapeJSONString(const S: string): string; | |
| var | |
| I: Integer; | |
| C: Char; | |
| SB: string; | |
| begin | |
| SB := ''; | |
| for I := 1 to Length(S) do | |
| begin | |
| C := S[I]; | |
| case C of | |
| '\': SB := SB + '\\'; | |
| '"': SB := SB + '\"'; | |
| `#8`: SB := SB + '\b'; | |
| `#9`: SB := SB + '\t'; | |
| `#10`: SB := SB + '\n'; | |
| `#12`: SB := SB + '\f'; | |
| `#13`: SB := SB + '\r'; | |
| `#0`..#7, `#11`, `#14`..#31: | |
| SB := SB + Format('\u%.4x', [Ord(C)]); | |
| else | |
| SB := SB + C; | |
| end; | |
| end; | |
| Result := SB; | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TestRunner.dpr` around lines 133 - 140, EscapeJSONString currently only
replaces \, ", newline, carriage return and tab, but must also escape all
control characters U+0000..U+001F (e.g., `#0`, `#8` backspace, `#12` form feed) into
their JSON-safe forms; update EscapeJSONString to iterate over the input string
and build the result character-by-character: first handle backslash and
double-quote as existing, map known single-character escapes (\b, \f, \n, \r,
\t) to their short sequences, and for any character with Ord <= 31 emit a \u00XX
hex escape, leaving other characters unchanged; ensure the function still
returns correct UTF-8/Unicode behavior for non-control characters.
Owner
Author
There was a problem hiding this comment.
Pre-existing issue not introduced by this PR — EscapeJSONString exists in 5 copies across the codebase. Tracked for consolidation separately.
Contributor
There was a problem hiding this comment.
@frostney, understood — thanks for the context. I'll note this for future reviews and won't re-flag the incomplete control-character escaping in the individual tool copies until the consolidation lands.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:10.919Z
Learning: In frostney/GocciaScript, `EscapeJSONString` (escaping `\`, `"`, `\n`, `\r`, `\t` only) is intentionally duplicated across ~5 CLI tool files (e.g., TestRunner.dpr, BenchmarkRunner.dpr). The incomplete control-character coverage (missing U+0000–U+001F beyond the five named characters) is a known pre-existing issue tracked for fix as part of a future consolidation into a shared helper. Do NOT re-flag missing control-character escaping in individual per-tool copies of `EscapeJSONString` until that consolidation PR lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:4349-4353
Timestamp: 2026-03-23T12:38:57.366Z
Learning: Repo frostney/GocciaScript — units/Goccia.Runtime.Operations.pas: JSON string escape decoding now uses CodePointToUTF8 and combines UTF‑16 surrogate pairs (\uD800–\uDBFF with \uDC00–\uDFFF) into a single Unicode code point before UTF‑8 encoding. Prefer this helper over Char(StrToInt(...)) for any future code‑point to UTF‑8 conversions in Pascal units.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: TestRunner.dpr:237-240
Timestamp: 2026-04-07T20:24:01.235Z
Learning: Repo: frostney/GocciaScript — TestRunner.dpr / Goccia.Coverage
The synthetic `runTests({...})` call is appended to `Source` before lexing in both `RunGocciaScriptBytecode` and `RunGocciaScriptInterpreted`. `Lexer.SourceLines` therefore includes this line, and `CountExecutableLines` counts it as +1 executable. Because the engine always executes it, coverage also records +1 hit. The net effect on the percentage is self-consistent (+1/+1) and does NOT skew coverage ratios. `PrintCoverageDetail` reads the original file from disk, so the synthetic line never appears in the detail output. Do not flag this as a coverage accuracy bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:140-156
Timestamp: 2026-04-12T21:19:18.811Z
Learning: For GocciaScript CLI tools, suppress human-readable status/progress WriteLn output when running in JSON output mode. In ScriptLoader (ScriptLoader.dpr), gate any status/progress WriteLn lines on `not GJsonOutput`, because `--output=json` streams the result document directly to stdout (mixing status text would corrupt JSON). In TestRunner and BenchmarkRunner (TestRunner.dpr, BenchmarkRunner.dpr), JSON is written to a file, so current unconditional stdout WriteLn calls are safe, but these tools should be refactored later to follow the same stdout/JSON suppression pattern used by ScriptLoader.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: docs/testing.md:774-776
Timestamp: 2026-04-12T15:44:39.321Z
Learning: In frostney/GocciaScript TestRunner.dpr, both interpreted (`RunGocciaScriptInterpreted` via `TGocciaEngine`) and bytecode (`RunGocciaScriptBytecode` via `TGocciaBytecodeBackend`) paths use the same globals set `[ggTestAssertions, ggFFI]`. `TGocciaBytecodeBackend.RegisterBuiltIns` accepts the same `TGocciaGlobalBuiltins` type. Documentation stating "creates a fresh `TGocciaEngine` with `[ggTestAssertions, ggFFI]`" is intentionally concise and accurate for both modes — do not flag it as incomplete for omitting the bytecode backend distinction.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:649-650
Timestamp: 2026-04-12T21:08:07.595Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr, units/Goccia.Engine.pas): `--source-map` is valid in interpreted mode when JSX preprocessing is active. Source maps are produced by the JSX pre-pass inside `ParseSource`/`TGocciaEngine`, not solely by the bytecode pipeline. `ExecuteInterpreted` needs to surface the engine's source map (via a public `SourceMap` property on `TGocciaEngine`) and pass it to `WriteSourceMapIfEnabled` — do not reject `--source-map` in interpreted mode.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : `JSON.rawJSON(text)` must create a frozen, null-prototype `TGocciaRawJSONValue` with `rawJSON` property; during `JSON.stringify`, check for `TGocciaRawJSONValue` and emit raw text verbatim
Learnt from: frostney
Repo: frostney/GocciaScript PR: 267
File: units/Goccia.Lexer.pas:504-533
Timestamp: 2026-04-11T09:43:56.558Z
Learning: In frostney/GocciaScript (units/Goccia.Lexer.pas), the lexer intentionally throws on malformed escape sequences (\u{, \xG1, etc.) regardless of whether the template literal is tagged or untagged. Supporting cooked=undefined for tagged templates (per TC39 proposal-template-literal-revision / ES2018 Template Literal Revision) requires per-segment escape validity tracking and deferred error handling — this is explicitly deferred to a future PR. The raw string (RawSB) is always correctly captured for valid escapes. Do NOT re-flag missing cooked=undefined / malformed-escape tolerance in ScanTemplate or ProcessEscapeSequence as a bug in the current PR.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : Format-defined newline semantics (e.g., TOML multiline strings, YAML folding) must be implemented explicitly and not rely on `LineEnding`/`sLineBreak`; add regression coverage with explicit `#13#10` / `\r\n` inputs
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1010-1023
Timestamp: 2026-04-12T09:44:48.683Z
Learning: In frostney/GocciaScript (units/Goccia.Parser.pas, SplitTemplateAtBoundaries and the original ParseTaggedTemplate), the brace-counting used to find the closing `}` of a template interpolation expression (`${...}`) is NOT lexically aware — it does not account for `{`/`}` inside string literals, comments, or nested templates within the expression. This is a known pre-existing limitation that predates this codebase area; it was present in the original `ParseTaggedTemplate` on `main` and is carried forward. Fixing it properly requires lexer-time segmentation (embedding boundary markers during `ScanTemplate` while escape/string context is available). Do NOT re-flag the naive brace counter in SplitTemplateAtBoundaries or ParseTaggedTemplate as a regression or new bug — it is intentionally deferred to a follow-up. The limitation exists because `ScanTemplate` stores the entire template (including `${...}` expressions) as a single flat token with no expression-level lexical analysis.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to units/**/*.Test.pas : Update `units/*.Test.pas` native tests to reflect changes in behavior when modifying AST logic, scope chain, evaluator, or value types
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.Test.pas : When modifying AST logic, scope chain, evaluator, or value types, update the native Pascal tests in `units/*.Test.pas` to reflect any changes in behaviour (e.g. new parameters, changed return semantics). Both JavaScript tests and native Pascal tests must pass.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to units/**/*.Test.pas : Native Pascal unit tests in `units/*.Test.pas` must be updated whenever AST logic, scope chain, evaluator, or value types are modified. Both JavaScript tests and native Pascal tests must pass.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Build and run native Pascal test suite `./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; done` when modifying AST logic, scope chain, evaluator, or value types
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Built-ins are registered by the engine via `TGocciaGlobalBuiltins` flags. The TestRunner adds `ggTestAssertions` for the test framework. The BenchmarkRunner adds `ggBenchmark` for the benchmark framework.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to **/Goccia.Engine.pas : Built-ins are registered by the engine via `TGocciaGlobalBuiltins` flags in `DefaultGlobals`. The TestRunner adds `ggTestAssertions` for `describe`/`test`/`expect`. The BenchmarkRunner adds `ggBenchmark` for `bench()` with setup/teardown.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:38.347Z
Learning: In frostney/GocciaScript, shared/utility units (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, garbage collector, test assertion units) that are used across multiple separate parts of the project or can be extracted as standalone infrastructure should NOT use the `Goccia.*` naming prefix. A separate naming convention (to be defined in a follow-up issue) applies to these units. Do not flag the absence of a `Goccia.*` prefix for such shared/utility units in future reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:38.347Z
Learning: In frostney/GocciaScript, shared/utility units (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, garbage collector, test assertion units) that are used across multiple separate parts of the project or can be extracted as standalone infrastructure should NOT use the `Goccia.*` naming prefix. A separate naming convention (to be defined — tracked in issue `#104`) applies to these units. Do not flag the absence of a `Goccia.*` prefix for such shared/utility units in future reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:08:41.070Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas
Maintainer preference: FBlueprintBridgeCache is intentionally shared between UnwrapToGocciaValue (which uses BlueprintToClassValue) and Construct (which uses ConvertBlueprintToClassValue); this pre‑existing pattern on main is acceptable and should not be re‑flagged in future reviews unless a functional bug is demonstrated.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:57.064Z
Learning: Repo: frostney/GocciaScript PR: 58
Context: units/Goccia.Runtime.Operations.pas (BridgePromiseStatic, NativePromiseStaticResolve, NativePromiseStaticReject) and interpreter Promise statics in Goccia.Builtins.GlobalPromise.pas
Learning: GocciaScript’s Promise statics are intentionally NOT species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Owner
Author
There was a problem hiding this comment.
Please create an issue for this
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@frostney, I've created the issue for consolidating EscapeJSONString into a shared helper and fixing the incomplete control-character escaping. The link will be in the output above once the script runs. It covers both the consolidation work and the full U+0000–U+001F fix.
🏁 Script executed:
gh issue create \
--repo frostney/GocciaScript \
--title "Consolidate EscapeJSONString into shared helper and fix incomplete control-character escaping" \
--body "## Summary
\`EscapeJSONString\` is currently duplicated across approximately 5 CLI tool source files (e.g., \`TestRunner.dpr\`, \`BenchmarkRunner.dpr\`, and others). Each copy only escapes \`\\\`, \`\"\`, \`\\n\`, \`\\r\`, and \`\\t\`, leaving other C0 control characters (U+0000–U+001F, e.g., NUL \`#0\`, backspace \`#8\`, form feed \`#12\`) unescaped, which produces invalid JSON if those characters appear in test output or error messages.
## Work Items
1. **Consolidate** all per-tool copies of \`EscapeJSONString\` into a single shared helper (e.g., in a \`Goccia.JSON.Utils\` or similar unit).
2. **Fix** the shared helper to escape all control characters U+0000–U+001F:
- Known short escapes: \`\\b\` (`#8`), \`\\t\` (`#9`), \`\\n\` (`#10`), \`\\f\` (`#12`), \`\\r\` (`#13`)
- Remaining C0 characters: emit \`\\u00XX\` hex escapes
3. **Update** all call sites (TestRunner, BenchmarkRunner, ScriptLoader, REPL, etc.) to use the shared helper.
## References
- Flagged in PR `#299`: https://github.com/frostney/GocciaScript/pull/299#discussion_r3075918260
- Requested by `@frostney`"
Length of output: 75
@frostney, unfortunately the sandbox environment doesn't have GitHub authentication available, so I'm unable to create the issue automatically. Here are the details you can use to open it directly:
Title: Consolidate EscapeJSONString into shared helper and fix incomplete control-character escaping
Body:
## Summary
`EscapeJSONString` is currently duplicated across approximately 5 CLI tool source files
(e.g., `TestRunner.dpr`, `BenchmarkRunner.dpr`, and others). Each copy only escapes
`\`, `"`, `\n`, `\r`, and `\t`, leaving other C0 control characters (U+0000–U+001F,
e.g., NUL `#0`, backspace `#8`, form feed `#12`) unescaped, which produces invalid JSON
if those characters appear in test output or error messages.
## Work Items
1. **Consolidate** all per-tool copies of `EscapeJSONString` into a single shared helper
(e.g., in a `Goccia.JSON.Utils` or similar unit).
2. **Fix** the shared helper to escape all control characters U+0000–U+001F:
- Known short escapes: `\b` (`#8`), `\t` (`#9`), `\n` (`#10`), `\f` (`#12`), `\r` (`#13`)
- Remaining C0 characters: emit `\u00XX` hex escapes
3. **Update** all call sites (TestRunner, BenchmarkRunner, ScriptLoader, REPL, etc.)
to use the shared helper.
## References
- Flagged in PR `#299`: https://github.com/frostney/GocciaScript/pull/299#discussion_r3075918260
- Requested by `@frostney`You can create it at: https://github.com/frostney/GocciaScript/issues/new
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:10.919Z
Learning: In frostney/GocciaScript CLI tool sources, the duplicated `EscapeJSONString` helper intentionally escapes only `\`, `"`, `\n`, `\r`, and `\t` (leaving other C0 control characters U+0000–U+001F unescaped). During code review, do not flag missing additional control-character escaping in these per-tool `EscapeJSONString` copies while the shared-helper consolidation/fix PR is not yet landed; only re-check once the implementation has been consolidated into the shared helper and the tracking fix is in place.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:4349-4353
Timestamp: 2026-03-23T12:38:57.366Z
Learning: Repo frostney/GocciaScript — units/Goccia.Runtime.Operations.pas: JSON string escape decoding now uses CodePointToUTF8 and combines UTF‑16 surrogate pairs (\uD800–\uDBFF with \uDC00–\uDFFF) into a single Unicode code point before UTF‑8 encoding. Prefer this helper over Char(StrToInt(...)) for any future code‑point to UTF‑8 conversions in Pascal units.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: TestRunner.dpr:237-240
Timestamp: 2026-04-07T20:24:01.235Z
Learning: Repo: frostney/GocciaScript — TestRunner.dpr / Goccia.Coverage
The synthetic `runTests({...})` call is appended to `Source` before lexing in both `RunGocciaScriptBytecode` and `RunGocciaScriptInterpreted`. `Lexer.SourceLines` therefore includes this line, and `CountExecutableLines` counts it as +1 executable. Because the engine always executes it, coverage also records +1 hit. The net effect on the percentage is self-consistent (+1/+1) and does NOT skew coverage ratios. `PrintCoverageDetail` reads the original file from disk, so the synthetic line never appears in the detail output. Do not flag this as a coverage accuracy bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:140-156
Timestamp: 2026-04-12T21:19:18.811Z
Learning: For GocciaScript CLI tools, suppress human-readable status/progress WriteLn output when running in JSON output mode. In ScriptLoader (ScriptLoader.dpr), gate any status/progress WriteLn lines on `not GJsonOutput`, because `--output=json` streams the result document directly to stdout (mixing status text would corrupt JSON). In TestRunner and BenchmarkRunner (TestRunner.dpr, BenchmarkRunner.dpr), JSON is written to a file, so current unconditional stdout WriteLn calls are safe, but these tools should be refactored later to follow the same stdout/JSON suppression pattern used by ScriptLoader.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: docs/testing.md:774-776
Timestamp: 2026-04-12T15:44:39.321Z
Learning: In frostney/GocciaScript TestRunner.dpr, both interpreted (`RunGocciaScriptInterpreted` via `TGocciaEngine`) and bytecode (`RunGocciaScriptBytecode` via `TGocciaBytecodeBackend`) paths use the same globals set `[ggTestAssertions, ggFFI]`. `TGocciaBytecodeBackend.RegisterBuiltIns` accepts the same `TGocciaGlobalBuiltins` type. Documentation stating "creates a fresh `TGocciaEngine` with `[ggTestAssertions, ggFFI]`" is intentionally concise and accurate for both modes — do not flag it as incomplete for omitting the bytecode backend distinction.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:649-650
Timestamp: 2026-04-12T21:08:07.595Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr, units/Goccia.Engine.pas): `--source-map` is valid in interpreted mode when JSX preprocessing is active. Source maps are produced by the JSX pre-pass inside `ParseSource`/`TGocciaEngine`, not solely by the bytecode pipeline. `ExecuteInterpreted` needs to surface the engine's source map (via a public `SourceMap` property on `TGocciaEngine`) and pass it to `WriteSourceMapIfEnabled` — do not reject `--source-map` in interpreted mode.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to **/*.pas : `JSON.rawJSON(text)` must create a frozen, null-prototype `TGocciaRawJSONValue` with `rawJSON` property; during `JSON.stringify`, check for `TGocciaRawJSONValue` and emit raw text verbatim
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:57.064Z
Learning: Repo: frostney/GocciaScript PR: 58
Context: units/Goccia.Runtime.Operations.pas (BridgePromiseStatic, NativePromiseStaticResolve, NativePromiseStaticReject) and interpreter Promise statics in Goccia.Builtins.GlobalPromise.pas
Learning: GocciaScript’s Promise statics are intentionally NOT species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 271
File: units/Goccia.Builtins.GlobalObject.pas:104-129
Timestamp: 2026-04-11T19:43:06.627Z
Learning: In frostney/GocciaScript (units/Goccia.Builtins.GlobalObject.pas and all other built-in units), `TGarbageCollector.AddTempRoot`/`RemoveTempRoot` use `THashMap<TGCManagedObject, Boolean>` which is **set-membership based, NOT reference-counted**. A single `RemoveTempRoot(Obj)` removes the root for ALL holders of that object, including callers higher up the stack (e.g., a benchmark runner that already called `AddTempRoot(SetupResult)`).
Therefore, `AddTempRoot`/`RemoveTempRoot` guards in built-in method implementations MUST be conditioned on whether the value was freshly boxed by `ToObject` (i.e., was not already a `TGocciaObjectValue`). The correct pattern is:
```pascal
if Assigned(TGarbageCollector.Instance) and
not (AArgs.GetElement(0) is TGocciaObjectValue) then
TGarbageCollector.Instance.AddTempRoot(Obj);
try
// ... work ...
finally
if Assigned(TGarbageCollector.Instance) and
not (AArgs.GetElement(0) is TGocciaObjectValue) then
TGarbageCollector.Instance.RemoveTempRoot(Obj);
end;
```
Never unconditionally call `AddTempRoot`/`RemoveTempRoot` on a value that was passed in as an argument — doing so can strip a temp root that an external caller (e.g., a benchmark/test runner) already holds on the same object, causing use-after-free on the next GC cycle.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/OrderedStringMap.pas:118-156
Timestamp: 2026-03-17T22:18:27.332Z
Learning: In frostney/GocciaScript (units/OrderedStringMap.pas), TOrderedStringMap.Remove is currently only called during bridge-cache teardown (inside Destroy) and never in tight loops between Grow triggers. Tombstone accumulation is therefore not a live performance concern for current usage. A hardening measure — tracking FDeletedCount and triggering Compact/Rehash when tombstones dominate — is deferred to a follow-up issue. Do not re-flag tombstone growth in OrderedStringMap.pas as a blocking issue for this PR or until the follow-up lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1010-1023
Timestamp: 2026-04-12T09:44:48.683Z
Learning: In frostney/GocciaScript (units/Goccia.Parser.pas, SplitTemplateAtBoundaries and the original ParseTaggedTemplate), the brace-counting used to find the closing `}` of a template interpolation expression (`${...}`) is NOT lexically aware — it does not account for `{`/`}` inside string literals, comments, or nested templates within the expression. This is a known pre-existing limitation that predates this codebase area; it was present in the original `ParseTaggedTemplate` on `main` and is carried forward. Fixing it properly requires lexer-time segmentation (embedding boundary markers during `ScanTemplate` while escape/string context is available). Do NOT re-flag the naive brace counter in SplitTemplateAtBoundaries or ParseTaggedTemplate as a regression or new bug — it is intentionally deferred to a follow-up. The limitation exists because `ScanTemplate` stores the entire template (including `${...}` expressions) as a single flat token with no expression-level lexical analysis.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 267
File: units/Goccia.Compiler.Expressions.pas:1826-1851
Timestamp: 2026-04-11T09:43:49.758Z
Learning: In frostney/GocciaScript (units/Goccia.Compiler.Expressions.pas), `CompileTaggedTemplate` intentionally builds fresh cooked/raw arrays on every call site execution rather than caching a single frozen template object per call site. The `.raw` property is correctly attached as non-enumerable/non-writable/non-configurable via `EmitDefineNonEnumerableProperty` (which emits `Object.defineProperty(cookedArray, 'raw', {value: rawArray})`). Template object identity/caching (reusing the same frozen object across invocations of the same call site, as required by ES2026 §13.2.8.3) is a known deferred optimization tracked for a follow-up PR that adds `FTemplateObject: TGocciaValue` caching to `TGocciaTaggedTemplateExpression`. Do NOT re-flag the per-call allocation as a blocking correctness issue — most tag functions only read from the template object and do not rely on same-site reference equality.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 267
File: units/Goccia.Evaluator.pas:2927-2946
Timestamp: 2026-04-11T09:43:55.698Z
Learning: In frostney/GocciaScript (units/Goccia.Evaluator.pas, EvaluateTaggedTemplate), template object identity — i.e. GetTemplateObject returning the same frozen template object for repeated evaluations of the same source location (AST node / call site) — is a known ES2026 §13.2.8.3 conformance gap, deferred to a follow-up PR. The fix is to cache the built template object on the TGocciaTaggedTemplateExpression AST node (a FTemplateObject field) and reuse it on subsequent calls instead of rebuilding RawArray and CookedArray each time. Do NOT re-flag the per-call rebuild as a blocking issue until the caching follow-up PR lands.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T22:35:35.657Z
Learning: Applies to units/**/*.Test.pas : Update `units/*.Test.pas` native tests to reflect changes in behavior when modifying AST logic, scope chain, evaluator, or value types
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.Test.pas : When modifying AST logic, scope chain, evaluator, or value types, update the native Pascal tests in `units/*.Test.pas` to reflect any changes in behaviour (e.g. new parameters, changed return semantics). Both JavaScript tests and native Pascal tests must pass.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: docs/benchmarks.md:121-123
Timestamp: 2026-04-12T15:44:31.303Z
Learning: In frostney/GocciaScript BenchmarkRunner.dpr, both interpreted and bytecode execution paths use the same `[ggBenchmark]` globals set. `TGocciaEngine` is constructed with `[ggBenchmark]` for interpreted mode, and `TGocciaBytecodeBackend.RegisterBuiltIns([ggBenchmark])` is called for bytecode mode. Documentation referring to `[ggBenchmark]` as the globals for the benchmark runtime is accurate for both modes. Do not flag the doc step as mode-inaccurate on the basis of different backend types.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to units/**/*.Test.pas : Native Pascal unit tests in `units/*.Test.pas` must be updated whenever AST logic, scope chain, evaluator, or value types are modified. Both JavaScript tests and native Pascal tests must pass.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: ScriptLoader.dpr:724-737
Timestamp: 2026-04-14T06:46:14.795Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `--emit` with `--output=<file>` and multiple input paths or a directory input silently overwrites the same output file for each compiled module. This is pre-existing behavior preserved across the CLI refactor in PR `#299` and should NOT be re-flagged as a regression. A follow-up guard (raising TGocciaParseError when FEmit.Present and FOutputPath.Present and APaths.Count > 1 or the single path is a directory) was proposed but deferred.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 139
File: units/Goccia.ScriptLoader.JSON.Test.pas:1-113
Timestamp: 2026-03-30T13:02:33.857Z
Learning: In frostney/GocciaScript, CI workflows (`.github/workflows/ci.yml` and `.github/workflows/pr.yml`) use glob patterns (`build/Goccia.*.Test*` for artifact upload, `Goccia.*.Test` / `Goccia.*.Test.exe` for execution) to discover and run native Pascal test binaries automatically. Do not flag missing entries in a hard-coded test list — new `units/*.Test.pas` test programs are picked up automatically via these globs as of commit b63911d (PR `#139`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:38.347Z
Learning: In frostney/GocciaScript, shared/utility units (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, garbage collector, test assertion units) that are used across multiple separate parts of the project or can be extracted as standalone infrastructure should NOT use the `Goccia.*` naming prefix. A separate naming convention (to be defined in a follow-up issue) applies to these units. Do not flag the absence of a `Goccia.*` prefix for such shared/utility units in future reviews.
- Move engine and backend creation into TGocciaCLIApplication - Simplify repeatable option parsing and help formatting - Update docs for value-option and repeatable-option syntax
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
TestRunner.dpr (2)
561-575:⚠️ Potential issue | 🟠 MajorKeep the aggregate result rooted until after the final writes.
AllTestResults/AllFailedTestsare unrooted before the lastAssignProperty(...)calls and beforeResult.TestResultis published. Those boxed-number allocations can trigger GC while the aggregate object is still being built.🩹 Minimal fix
- if Assigned(GC) then - begin - GC.RemoveTempRoot(AllTestResults); - GC.RemoveTempRoot(AllFailedTests); - end; - - AllTestResults.AssignProperty('totalTests', TGocciaNumberLiteralValue.Create(AFiles.Count * 1.0)); - AllTestResults.AssignProperty('passed', TGocciaNumberLiteralValue.Create(PassedCount)); - AllTestResults.AssignProperty('failed', TGocciaNumberLiteralValue.Create(FailedCount)); - AllTestResults.AssignProperty('skipped', TGocciaNumberLiteralValue.Create(SkippedCount)); - AllTestResults.AssignProperty('totalRunTests', TGocciaNumberLiteralValue.Create(TotalRunCount)); - AllTestResults.AssignProperty('duration', TGocciaNumberLiteralValue.Create(TotalDuration)); - AllTestResults.AssignProperty('assertions', TGocciaNumberLiteralValue.Create(TotalAssertions)); - - Result.TestResult := AllTestResults; + try + AllTestResults.AssignProperty('totalTests', TGocciaNumberLiteralValue.Create(AFiles.Count * 1.0)); + AllTestResults.AssignProperty('passed', TGocciaNumberLiteralValue.Create(PassedCount)); + AllTestResults.AssignProperty('failed', TGocciaNumberLiteralValue.Create(FailedCount)); + AllTestResults.AssignProperty('skipped', TGocciaNumberLiteralValue.Create(SkippedCount)); + AllTestResults.AssignProperty('totalRunTests', TGocciaNumberLiteralValue.Create(TotalRunCount)); + AllTestResults.AssignProperty('duration', TGocciaNumberLiteralValue.Create(TotalDuration)); + AllTestResults.AssignProperty('assertions', TGocciaNumberLiteralValue.Create(TotalAssertions)); + + Result.TestResult := AllTestResults; + finally + if Assigned(GC) then + begin + GC.RemoveTempRoot(AllTestResults); + GC.RemoveTempRoot(AllFailedTests); + end; + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 561 - 575, The code unroots AllTestResults and AllFailedTests (calls to GC.RemoveTempRoot) before finishing AssignProperty calls and before publishing Result.TestResult, risking GC of boxed-number temporaries; to fix, delay removing the temp roots (i.e. move the GC.RemoveTempRoot(AllTestResults) and GC.RemoveTempRoot(AllFailedTests) calls) until after all AssignProperty(...) calls and after setting Result.TestResult, so the aggregate object remains rooted while building and publishing it.
307-323:⚠️ Potential issue | 🟠 Major
--silentstill leaks test-console output in bytecode mode.The interpreted path silences
BuiltinConsole, but this path only suppresses parser warnings.--mode=bytecode --silentwill still print test script output to stdout.🩹 Minimal fix
var Source: TStringList; SourceText: string; JSXResult: TGocciaJSXTransformResult; SourceMap: TGocciaSourceMap; Lexer: TGocciaLexer; Tokens: TObjectList<TGocciaToken>; Parser: TGocciaParser; Warning: TGocciaParserWarning; ProgramNode: TGocciaProgram; Module: TGocciaBytecodeModule; Backend: TGocciaBytecodeBackend; + SilentCon: TGocciaTestConsole; ScriptResult: TGocciaObjectValue; ResultValue: TGocciaValue; OrigLine, OrigCol, I: Integer; LexStart, LexEnd, ParseEnd, CompileEnd, ExecEnd: Int64; begin ScriptResult := CreateDefaultScriptResult; + SilentCon := nil; ... Backend := CreateBytecodeBackend(AFileName); try + if FSilent.Present and Assigned(Backend.Bootstrap) and + Assigned(Backend.Bootstrap.BuiltinConsole) then + begin + SilentCon := TGocciaTestConsole.Create; + SilentCon.Silence(Backend.Bootstrap.BuiltinConsole.BuiltinObject); + end; LexStart := GetNanoseconds; Lexer := TGocciaLexer.Create(SourceText, AFileName); try Tokens := Lexer.ScanTokens; LexEnd := GetNanoseconds; @@ finally + SilentCon.Free; Backend.Free; end;Also applies to: 352-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 307 - 323, The bytecode execution path still writes test-console output because the bytecode backend/module is not configured to silence the BuiltinConsole; update the bytecode branch (references: Backend, TGocciaBytecodeBackend, Module, TGocciaBytecodeModule) to apply the same suppression used by the interpreted path: when the --silent flag is set, replace or disable the BuiltinConsole used by the backend/module (e.g. set its Enabled/Active flag to false or install a noop/null console) before compiling/executing bytecode so stdout from test scripts is suppressed; mirror this change in the other bytecode-related block noted (lines 352-383 equivalent) to ensure both places honor --silent.
♻️ Duplicate comments (1)
TestRunner.dpr (1)
132-139:⚠️ Potential issue | 🟡 MinorEscape all JSON control characters.
This still leaves
#0..#31values like backspace and form feed unescaped, so certain failure messages will generate invalid JSON.🩹 Minimal fix
function EscapeJSONString(const S: string): string; +var + I: Integer; + C: Char; + SB: string; begin - Result := StringReplace(S, '\', '\\', [rfReplaceAll]); - Result := StringReplace(Result, '"', '\"', [rfReplaceAll]); - Result := StringReplace(Result, `#10`, '\n', [rfReplaceAll]); - Result := StringReplace(Result, `#13`, '\r', [rfReplaceAll]); - Result := StringReplace(Result, `#9`, '\t', [rfReplaceAll]); + SB := ''; + for I := 1 to Length(S) do + begin + C := S[I]; + case C of + '\': SB := SB + '\\'; + '"': SB := SB + '\"'; + `#8`: SB := SB + '\b'; + `#9`: SB := SB + '\t'; + `#10`: SB := SB + '\n'; + `#12`: SB := SB + '\f'; + `#13`: SB := SB + '\r'; + `#0`..#7, `#11`, `#14`..#31: + SB := SB + Format('\u%.4x', [Ord(C)]); + else + SB := SB + C; + end; + end; + Result := SB; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 132 - 139, The EscapeJSONString function currently only replaces a few characters and misses control characters in the range `#0`..#31 which can break JSON; update EscapeJSONString to also escape all control characters: explicitly map known ones (e.g. `#8` -> \b, `#12` -> \f) and for any remaining codepoints < 32 emit a \u00XX hex escape, or implement a single pass that builds Result char-by-char and emits either the known escape sequences or a \u00HH escape for chars < `#32`; keep the existing handling for backslash and double-quote and ensure the function name EscapeJSONString is updated in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ScriptLoader.dpr`:
- Around line 724-737: Add a validation that rejects using --emit together with
a single output file when there are multiple input paths or the single input is
a directory: in the same validation area that checks FOutputPath and APaths, add
a check that if FEmit.Present and FOutputPath.Present and ((APaths.Count > 1) or
((APaths.Count = 1) and DirectoryExists(APaths[0]))) then raise
TGocciaParseError with a clear message like '--emit with --output=<file>
supports a single input file or stdin.' This references FEmit, FOutputPath and
APaths and prevents RunSource from overwriting the same output for multi-input
runs.
In `@units/Goccia.CLI.Application.pas`:
- Around line 177-188: CreateEngine and CreateBytecodeBackend start the global
execution timeout (StartExecutionTimeout) inside the factory, which can be set
before the caller's try/finally that calls ClearExecutionTimeout; move the
StartExecutionTimeout call out of TGocciaCLIApplication.CreateEngine and the
analogous CreateBytecodeBackend so that factories only configure the
engine/backend (ASIEnabled, ConfigureModuleResolver, etc.), and instead invoke
StartExecutionTimeout immediately before the corresponding execution site (e.g.,
just before Engine.Execute or Backend.RunModule) inside the same try/finally
that calls ClearExecutionTimeout; keep references to StartExecutionTimeout and
ClearExecutionTimeout and ensure the timeout is started and cleared within the
same scope to avoid leaving a stale global timeout on parse/compile errors.
---
Outside diff comments:
In `@TestRunner.dpr`:
- Around line 561-575: The code unroots AllTestResults and AllFailedTests (calls
to GC.RemoveTempRoot) before finishing AssignProperty calls and before
publishing Result.TestResult, risking GC of boxed-number temporaries; to fix,
delay removing the temp roots (i.e. move the GC.RemoveTempRoot(AllTestResults)
and GC.RemoveTempRoot(AllFailedTests) calls) until after all AssignProperty(...)
calls and after setting Result.TestResult, so the aggregate object remains
rooted while building and publishing it.
- Around line 307-323: The bytecode execution path still writes test-console
output because the bytecode backend/module is not configured to silence the
BuiltinConsole; update the bytecode branch (references: Backend,
TGocciaBytecodeBackend, Module, TGocciaBytecodeModule) to apply the same
suppression used by the interpreted path: when the --silent flag is set, replace
or disable the BuiltinConsole used by the backend/module (e.g. set its
Enabled/Active flag to false or install a noop/null console) before
compiling/executing bytecode so stdout from test scripts is suppressed; mirror
this change in the other bytecode-related block noted (lines 352-383 equivalent)
to ensure both places honor --silent.
---
Duplicate comments:
In `@TestRunner.dpr`:
- Around line 132-139: The EscapeJSONString function currently only replaces a
few characters and misses control characters in the range `#0`..#31 which can
break JSON; update EscapeJSONString to also escape all control characters:
explicitly map known ones (e.g. `#8` -> \b, `#12` -> \f) and for any remaining
codepoints < 32 emit a \u00XX hex escape, or implement a single pass that builds
Result char-by-char and emits either the known escape sequences or a \u00HH
escape for chars < `#32`; keep the existing handling for backslash and
double-quote and ensure the function name EscapeJSONString is updated in place.
🪄 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: ce5dff9a-11b1-42c6-a092-f0b3d040fa39
📒 Files selected for processing (9)
BenchmarkRunner.dprScriptLoader.dprTestRunner.dprdocs/code-style.mdunits/Goccia.CLI.Application.pasunits/Goccia.CLI.EngineSetup.pasunits/Goccia.CLI.Help.pasunits/Goccia.CLI.Options.pasunits/Goccia.CLI.Parser.pas
✅ Files skipped from review due to trivial changes (2)
- docs/code-style.md
- units/Goccia.CLI.Options.pas
🚧 Files skipped from review as they are similar to previous changes (3)
- units/Goccia.CLI.Parser.pas
- units/Goccia.CLI.EngineSetup.pas
- units/Goccia.CLI.Help.pas
- Fix ScriptLoader/TestRunner exit code: RunApplication returning 0 was overwriting ExitCode set internally by per-file error handlers - Move StartExecutionTimeout from engine factory to execution sites to avoid stale timeouts during setup - Reject option-shaped tokens as repeatable values (--alias --mode) - Reject unexpected positional args in REPL - Add Matches() method to TGocciaEnumOption<T> for cleaner comparisons - Fix docs: CLI pipeline includes Configure, markdown table formatting, AcceptsSpaceForm removed from docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rary-1 # Conflicts: # docs/architecture.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TestRunner.dpr (1)
307-452:⚠️ Potential issue | 🟠 MajorHandle
EGocciaBytecodeThrowexplicitly in bytecode mode.
Backend.RunModule()can raiseEGocciaBytecodeThrow, but this method only formatsTGocciaThrowValue. In bytecode mode, uncaught JS throws therefore fall through to the genericExceptionbranch and get reported as a plain fatal error instead of a proper throw detail.ScriptLoader.dprandBenchmarkRunner.dpralready special-case this path.Suggested fix
except on E: Exception do begin if E is TGocciaError then begin WriteLn(TGocciaError(E).GetDetailedMessage(IsColorTerminal)); MarkLoadError(ScriptResult, AFileName, TGocciaError(E).GetDetailedMessage); end else if E is TGocciaThrowValue then begin WriteLn(FormatThrowDetail(TGocciaThrowValue(E).Value, AFileName, Source, IsColorTerminal)); MarkLoadError(ScriptResult, AFileName, FormatThrowDetail(TGocciaThrowValue(E).Value, AFileName, Source, False)); end + else if E is EGocciaBytecodeThrow then + begin + WriteLn(FormatThrowDetail(EGocciaBytecodeThrow(E).ThrownValue, + AFileName, Source, IsColorTerminal)); + MarkLoadError(ScriptResult, AFileName, + FormatThrowDetail(EGocciaBytecodeThrow(E).ThrownValue, + AFileName, Source, False)); + end else begin WriteLn('Fatal error: ', E.Message); MarkLoadError(ScriptResult, AFileName, E.Message); end; Result := MakeEmptyTestResult(ScriptResult); end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 307 - 452, The bytecode runner needs to explicitly handle EGocciaBytecodeThrow thrown by Backend.RunModule; inside TTestRunnerApp.RunGocciaScriptBytecode's exception block add an else-if branch for EGocciaBytecodeThrow (similar to the TGocciaThrowValue handling in ScriptLoader.dpr/BenchmarkRunner.dpr) that writes the formatted throw detail (use FormatThrowDetail(EGocciaBytecodeThrow(E).Value, AFileName, Source, IsColorTerminal)) and calls MarkLoadError(ScriptResult, AFileName, FormatThrowDetail(..., AFileName, Source, False)); ensure you reference EGocciaBytecodeThrow, FormatThrowDetail, ScriptResult, AFileName and Source so uncaught JS throws in bytecode mode are reported like other throw values.
🧹 Nitpick comments (1)
units/Goccia.CLI.Parser.Test.pas (1)
42-65: Add parser-level cases for the new CLI branches.These tests stop at
Option.ApplyandGenerateHelpText; none of them exerciseParseCommandLineitself. That leaves the parser-only rules from this refactor untested, especially repeatable--name valuehandling and rejection of option-shaped/misplaced positional arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.CLI.Parser.Test.pas` around lines 42 - 65, Tests only exercise Option.Apply and GenerateHelpText but not the parser entrypoint; add unit tests that call ParseCommandLine directly to cover parser-only branches such as repeatable option accumulation and rejection of option-shaped positional args. Specifically, add cases that invoke ParseCommandLine with argv arrays containing repeated "--name value" pairs and assert the resulting RepeatableOption contains all values, and add cases with positional arguments that look like options (e.g., "-x" or "--foo=bar" in positional position) and assert ParseCommandLine returns an error or raises the expected TGocciaParseError; reference ParseCommandLine, RepeatableOption, Option.Apply and GenerateHelpText when locating where to add these tests. Ensure assertions verify both successful accumulation and proper parse-time rejection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.CLI.Options.pas`:
- Around line 226-229: TGocciaFlagOption.Apply currently accepts any AValue
(including non-empty values) and silently treats flags like `--silent=false` as
present; update TGocciaFlagOption.Apply to reject non-empty AValue and fail
fast: if AValue is not empty, raise a clear command-line/parsing exception
(e.g., EArgumentException or the project’s command-line error type) with a
descriptive message mentioning the flag name, otherwise set FPresent := True.
This change affects TGocciaFlagOption.Apply to ensure ParseCommandLine cannot
pass `--flag=value` through silently.
- Around line 379-384: The variable ArdinalValue in TGocciaEnumOption<T>.Matches
is not initialized before calling Move, so when SizeOf(T) < SizeOf(Integer)
leftover bytes produce garbage; to fix, initialize ArdinalValue to 0 (e.g.,
ArdinalValue := 0) immediately before the Move(AValue, ArdinalValue, SizeOf(T))
call so the remaining bytes are deterministic, then keep the existing comparison
Result := FPresent and (FOrdinal = ArdinalValue).
---
Outside diff comments:
In `@TestRunner.dpr`:
- Around line 307-452: The bytecode runner needs to explicitly handle
EGocciaBytecodeThrow thrown by Backend.RunModule; inside
TTestRunnerApp.RunGocciaScriptBytecode's exception block add an else-if branch
for EGocciaBytecodeThrow (similar to the TGocciaThrowValue handling in
ScriptLoader.dpr/BenchmarkRunner.dpr) that writes the formatted throw detail
(use FormatThrowDetail(EGocciaBytecodeThrow(E).Value, AFileName, Source,
IsColorTerminal)) and calls MarkLoadError(ScriptResult, AFileName,
FormatThrowDetail(..., AFileName, Source, False)); ensure you reference
EGocciaBytecodeThrow, FormatThrowDetail, ScriptResult, AFileName and Source so
uncaught JS throws in bytecode mode are reported like other throw values.
---
Nitpick comments:
In `@units/Goccia.CLI.Parser.Test.pas`:
- Around line 42-65: Tests only exercise Option.Apply and GenerateHelpText but
not the parser entrypoint; add unit tests that call ParseCommandLine directly to
cover parser-only branches such as repeatable option accumulation and rejection
of option-shaped positional args. Specifically, add cases that invoke
ParseCommandLine with argv arrays containing repeated "--name value" pairs and
assert the resulting RepeatableOption contains all values, and add cases with
positional arguments that look like options (e.g., "-x" or "--foo=bar" in
positional position) and assert ParseCommandLine returns an error or raises the
expected TGocciaParseError; reference ParseCommandLine, RepeatableOption,
Option.Apply and GenerateHelpText when locating where to add these tests. Ensure
assertions verify both successful accumulation and proper parse-time rejection
behavior.
🪄 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: 38d1e016-40df-46fe-bd82-fda7e9a41f2a
📒 Files selected for processing (11)
AGENTS.mdBenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprdocs/architecture.mddocs/code-style.mdunits/Goccia.CLI.Application.pasunits/Goccia.CLI.Options.pasunits/Goccia.CLI.Parser.Test.pasunits/Goccia.CLI.Parser.pas
✅ Files skipped from review due to trivial changes (2)
- docs/architecture.md
- docs/code-style.md
🚧 Files skipped from review as they are similar to previous changes (4)
- AGENTS.md
- units/Goccia.CLI.Parser.pas
- REPL.dpr
- units/Goccia.CLI.Application.pas
Comment on lines
+226
to
+229
| procedure TGocciaFlagOption.Apply(const AValue: string); | ||
| begin | ||
| FPresent := True; | ||
| end; |
Contributor
There was a problem hiding this comment.
Reject values passed to flag options.
ParseCommandLine forwards --flag=value straight into Apply, and this implementation silently enables the flag. Inputs like --silent=false or --coverage=0 therefore do the opposite of what the user asked instead of failing fast.
Suggested fix
procedure TGocciaFlagOption.Apply(const AValue: string);
begin
+ if AValue <> '' then
+ raise TGocciaParseError.CreateFmt(
+ '--%s does not take a value', [LongName]);
FPresent := True;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.CLI.Options.pas` around lines 226 - 229, TGocciaFlagOption.Apply
currently accepts any AValue (including non-empty values) and silently treats
flags like `--silent=false` as present; update TGocciaFlagOption.Apply to reject
non-empty AValue and fail fast: if AValue is not empty, raise a clear
command-line/parsing exception (e.g., EArgumentException or the project’s
command-line error type) with a descriptive message mentioning the flag name,
otherwise set FPresent := True. This change affects TGocciaFlagOption.Apply to
ensure ParseCommandLine cannot pass `--flag=value` through silently.
The OrdinalValue local was not zero-initialized before Move copied SizeOf(T) = 1 byte into it. The remaining 3 bytes held stack garbage, causing FOrdinal = OrdinalValue to fail on x64 Linux where the garbage was non-zero. This made Matches(emBytecode) return False, writing "mode":"interpreted" into the JSON even when --mode=bytecode was set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rary-1 # Conflicts: # AGENTS.md # docs/code-style.md
Add required blank lines between bold paragraph headers and the list items that follow them in architecture.md and embedding.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 14, 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
ScriptLoader,REPL,TestRunner, andBenchmarkRunnerto use shared CLI/application base classes instead of bespoke command-line parsing and lifecycle code.Testing
./build.pas testrunner && ./build/TestRunner tests./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; done