Update docs#302
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConsolidates and reorganizes repository documentation and QA: adds a CONTRIBUTING hub and contributor-facing docs, repurposes AGENTS.md for AI assistants, splits engine implementation docs, adds docs-quality TypeScript validation scripts plus a CI docs job, and migrates legacy design/style content into new authoritative locations. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Scripts as Docs Validation Scripts
participant Repo as Repository
PR->>GH: push / open (trigger)
GH->>Repo: checkout code
GH->>Scripts: run markdownlint + check-doc-*.ts
Scripts->>Repo: read docs + Pascal sources (headings, anchors, symbols, lengths, duplicates)
Scripts-->>GH: emit reports & exit code
GH-->>PR: display status (pass / fail)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
documentation
![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)
docs/architecture.md (1)
10-10:⚠️ Potential issue | 🟡 MinorTighten wording to avoid implying
TGocciaValue-backed VM registers.“Operates directly on
TGocciaValue” conflicts with the documented tagged-register execution model later in this file. Consider rephrasing to emphasize shared runtime semantics/values at boundaries, not register storage.Based on learnings: “the bytecode VM register file uses a tagged
TGocciaRegistertype, notTGocciaValuedirectly.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` at line 10, The wording wrongly implies VM registers store TGocciaValue directly; update the sentence to state that the bytecode VM shares runtime semantics/values at API/boundaries using TGocciaValue but that the register file itself uses a tagged TGocciaRegister type (not TGocciaValue directly), aligning it with the tagged-register execution model described later—mention TGocciaValue for boundary/shared semantics and TGocciaRegister for register storage to remove the ambiguity.
🧹 Nitpick comments (1)
docs/contributing/code-style.md (1)
195-196: Consider clarifying iteration guidance to avoid mixed signals.This page recommends indexed loops for array-returning map APIs, but later recommends
for..inover dictionary key/value collections. Suggest a short clarification that these are context-specific patterns (dynamic arrays vs keyed collections) to prevent over-generalization.Also applies to: 317-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/code-style.md` around lines 195 - 196, Clarify the iteration guidance to state that the recommendation to use indexed for loops applies specifically to APIs that return dynamic arrays (e.g., Keys, Values, ToArray) from custom maps that implement the TDictionary-style core API (Add, AddOrSetValue, TryGetValue, ContainsKey, Remove, Clear), whereas the later recommendation to prefer for..in applies to keyed collection enumerations — add a short sentence distinguishing "array-returning map methods (use indexed for)" from "enumerable/keyed collections (for..in)" so readers don't over-generalize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/bytecode-vm.md`:
- Line 163: Reword the sentence that currently says “The current VM uses
TGocciaValue directly” to explicitly differentiate the shared runtime object
model (TGocciaValue) from the VM's internal register representation
(TGocciaRegister); state that the register file stores tagged TGocciaRegister
entries and that hot scalar kinds are kept unboxed in registers until crossing
object/runtime boundaries. Update the same clarification at the second
occurrence as well so both mentions accurately describe TGocciaValue as the
runtime object representation and TGocciaRegister as the VM's internal, possibly
unboxed, register representation.
- Around line 154-158: The opcode-range description is incorrect: update the
prose to reflect the project's three-tier model — core 0..127, non-core generic
128..166, and semantic/helper 167..255 — and mention that semantic opcodes start
at OP_SEM_FIRST (value 167) as defined in units/Goccia.Bytecode.pas so
contributors are not misled; replace the single "semantic 128–255" paragraph
with this corrected split and a short note referencing OP_SEM_FIRST for readers
who want the authoritative constant.
In `@docs/decision-log.md`:
- Around line 7-9: The decision-log template and entries use brittle relative PR
links (`../../pull/N`); update the template line containing "`../../pull/N`" and
all decision entries (lines referencing that same pattern) to use absolute
GitHub PR URLs or repo-root absolute paths (for example replace `../../pull/N`
with `https://github.com/frostney/GocciaScript/pull/N` or
`/frostney/GocciaScript/pull/N`) so the PR links resolve correctly from rendered
docs; ensure you update every occurrence of the `../../pull/` pattern in this
file.
In `@docs/interpreter.md`:
- Around line 24-29: Update the TGocciaEvaluationContext record in the docs so
it matches the actual declaration: include the missing fields CurrentFilePath,
CoverageEnabled and DisposalTracker alongside the existing Scope, OnError and
LoadModule entries (i.e., mirror the exact field names and order used in the
TGocciaEvaluationContext declaration in the codebase) so the documentation
snippet reflects the current units/Goccia.Evaluator.Context.pas definition.
In `@docs/language.md`:
- Line 157: Update the markdown link so its fragment matches the actual heading
text: replace the reference `[Decorators](`#decorators`)` with the fragment that
matches the target heading `### Decorators (Stage 3)` (e.g., use
`#decorators-stage-3`) so the link correctly points to the `Decorators (Stage
3)` section.
- Around line 62-76: Rename the first level-2 heading "TC39 Proposals" to a
distinct title (e.g., "TC39 Proposals — Overview" or "TC39 Proposals (Summary)")
to avoid duplicate headings with the detailed section also titled "TC39
Proposals"; update the table-introducing heading that currently reads "TC39
Proposals" so fragment links and MD024 checks resolve correctly while leaving
the later detailed section heading unchanged.
In `@docs/value-system.md`:
- Around line 714-715: The documentation incorrectly states that TGocciaValue
inherits from TInterfacedObject; update the sentence to reflect the actual
declaration TGocciaValue = class(TGCManagedObject) so the memory-management
rationale is accurate, and mention that values are stored as class references
(TGocciaValue) rather than interface references to explain why switching to
interfaces would be a large refactor and risk circular references between
objects and their prototypes in the evaluator.
---
Outside diff comments:
In `@docs/architecture.md`:
- Line 10: The wording wrongly implies VM registers store TGocciaValue directly;
update the sentence to state that the bytecode VM shares runtime
semantics/values at API/boundaries using TGocciaValue but that the register file
itself uses a tagged TGocciaRegister type (not TGocciaValue directly), aligning
it with the tagged-register execution model described later—mention TGocciaValue
for boundary/shared semantics and TGocciaRegister for register storage to remove
the ambiguity.
---
Nitpick comments:
In `@docs/contributing/code-style.md`:
- Around line 195-196: Clarify the iteration guidance to state that the
recommendation to use indexed for loops applies specifically to APIs that return
dynamic arrays (e.g., Keys, Values, ToArray) from custom maps that implement the
TDictionary-style core API (Add, AddOrSetValue, TryGetValue, ContainsKey,
Remove, Clear), whereas the later recommendation to prefer for..in applies to
keyed collection enumerations — add a short sentence distinguishing
"array-returning map methods (use indexed for)" from "enumerable/keyed
collections (for..in)" so readers don't over-generalize.
🪄 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: 1135e8ee-ee37-4db2-8942-008b59ab5063
📒 Files selected for processing (20)
AGENTS.mdCONTRIBUTING.mdREADME.mddocs/adding-built-in-types.mddocs/architecture.mddocs/build-system.mddocs/built-ins.mddocs/bytecode-vm.mddocs/code-style.mddocs/contributing/code-style.mddocs/contributing/tooling.mddocs/contributing/workflow.mddocs/core-patterns.mddocs/decision-log.mddocs/design-decisions.mddocs/interpreter.mddocs/language.mddocs/testing.mddocs/tutorial.mddocs/value-system.md
💤 Files with no reviewable changes (2)
- docs/design-decisions.md
- docs/code-style.md
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 191 improved · 🔴 26 regressed · 147 unchanged · avg +1.6% arraybuffer.js — Interp: 🟢 8, 🔴 1, 5 unch. · avg +1.5% · Bytecode: 🔴 7, 7 unch. · avg -1.9%
arrays.js — Interp: 🟢 12, 🔴 1, 6 unch. · avg +1.5% · Bytecode: 🟢 2, 🔴 7, 10 unch. · avg -1.0%
async-await.js — Interp: 🟢 1, 🔴 3, 2 unch. · avg -1.0% · Bytecode: 🟢 2, 🔴 2, 2 unch. · avg +0.2%
base64.js — Interp: 🟢 4, 6 unch. · avg +1.1% · Bytecode: 🟢 2, 🔴 3, 5 unch. · avg -0.6%
classes.js — Interp: 🟢 14, 🔴 2, 15 unch. · avg +1.2% · Bytecode: 🟢 1, 🔴 4, 26 unch. · avg -0.8%
closures.js — Interp: 🟢 8, 🔴 1, 2 unch. · avg +1.0% · Bytecode: 🔴 2, 9 unch. · avg +0.2%
collections.js — Interp: 🟢 7, 5 unch. · avg +1.9% · Bytecode: 🔴 10, 2 unch. · avg -3.1%
destructuring.js — Interp: 🟢 16, 🔴 1, 5 unch. · avg +2.4% · Bytecode: 🟢 6, 🔴 1, 15 unch. · avg +1.6%
fibonacci.js — Interp: 🟢 2, 🔴 1, 5 unch. · avg +0.4% · Bytecode: 🟢 2, 🔴 4, 2 unch. · avg -0.4%
float16array.js — Interp: 🟢 19, 🔴 2, 11 unch. · avg +1.7% · Bytecode: 🟢 10, 🔴 6, 16 unch. · avg +1.0%
for-of.js — Interp: 🟢 6, 1 unch. · avg +4.5% · Bytecode: 🟢 4, 3 unch. · avg +5.5%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 24, 18 unch. · avg +2.3% · Bytecode: 🟢 11, 🔴 7, 24 unch. · avg +0.7%
json.js — Interp: 🟢 10, 10 unch. · avg +1.9% · Bytecode: 🟢 3, 🔴 4, 13 unch. · avg +0.1%
jsx.jsx — Interp: 🟢 11, 10 unch. · avg +2.0% · Bytecode: 🔴 13, 8 unch. · avg -1.7%
modules.js — Interp: 🟢 2, 🔴 3, 4 unch. · avg -0.8% · Bytecode: 🔴 7, 2 unch. · avg -1.0%
numbers.js — Interp: 🟢 5, 6 unch. · avg +2.1% · Bytecode: 🔴 8, 3 unch. · avg -2.8%
objects.js — Interp: 🟢 7 · avg +4.5% · Bytecode: 🟢 1, 🔴 5, 1 unch. · avg -2.5%
promises.js — Interp: 🟢 7, 🔴 1, 4 unch. · avg +1.4% · Bytecode: 🔴 9, 3 unch. · avg -2.6%
regexp.js — Interp: 🟢 8, 3 unch. · avg +2.8% · Bytecode: 🔴 5, 6 unch. · avg -2.9%
strings.js — Interp: 🟢 9, 🔴 1, 9 unch. · avg +1.6% · Bytecode: 🔴 10, 9 unch. · avg -2.3%
typed-arrays.js — Interp: 🟢 9, 🔴 1, 12 unch. · avg +1.5% · Bytecode: 🔴 16, 6 unch. · avg -2.8%
uint8array-encoding.js — Interp: 🟢 2, 🔴 8, 8 unch. · avg -1.4% · Bytecode: 🟢 1, 🔴 13, 4 unch. · avg -2.7%
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. |
Add comprehensive documentation and automated doc validation: introduce new docs (built-ins-temporal, built-ins-binary-data, garbage-collector, language-tables, testing-api) and update many existing docs (built-ins, README, CONTRIBUTING, build-system, adding-built-in-types, etc.). Add markdownlint configuration (.markdownlint-cli2.jsonc) and new doc-check scripts (check-doc-symbols.ts, check-doc-duplication.ts, check-doc-length.ts, check-doc-links.ts). Integrate a new docs job into .github/workflows/pr.yml to run markdownlint and these checks on PRs. Also refine repository templates and doc formatting (issue & PR templates, markdown fence types, executive summary rules) to enforce documentation standards and improve discoverability.
- bytecode-vm.md: Correct opcode tiers to three-tier model (core 0-127, non-core 128-166, semantic 167-255) matching OP_SEM_FIRST in source. Clarify TGocciaRegister vs TGocciaValue distinction in register file. - decision-log.md: Replace brittle relative PR links (../../pull/N) with absolute GitHub URLs. - interpreter.md: Add missing TGocciaEvaluationContext fields (CurrentFilePath, CoverageEnabled, DisposalTracker). - garbage-collector.md: Fix incorrect TInterfacedObject inheritance claim; TGocciaValue inherits from TGCManagedObject. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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)
docs/value-system.md (1)
513-518:⚠️ Potential issue | 🟠 MajorThe native callback snippet no longer matches the implementation.
The codebase uses
TGocciaNativeFunctionCallback(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue of object, not aTGocciaValueArray-basedTGocciaNativeFunctionValueCallback. Anyone copying this snippet for a built-in will end up with the wrong callback type and call surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/value-system.md` around lines 513 - 518, The docs show an outdated native callback signature using TGocciaNativeFunctionValueCallback with TGocciaValueArray, but the codebase uses TGocciaNativeFunctionCallback(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue of object; update the snippet to match the real callback type and parameter names (use TGocciaNativeFunctionCallback and TGocciaArgumentsCollection and the const AThisValue: TGocciaValue parameter) so examples and built-ins use the correct call surface.
🧹 Nitpick comments (1)
scripts/check-doc-symbols.ts (1)
185-206: Walk the docs tree recursively.
collectMdFiles()only sees markdown files at the top level ofdocs/anddocs/contributing/, so any futuredocs/**/foo.mdpage will skip symbol validation entirely. A recursive walk plus the existing skip rules would make this checker follow the documentation tree instead of today's exact layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-doc-symbols.ts` around lines 185 - 206, collectMdFiles currently only collects markdown at the immediate MD_ROOT_FILES and one-level MD_DIRS; update it to recursively walk directories so nested docs (e.g., docs/**/foo.md) are included. Replace the existing loops in collectMdFiles with a helper recursive function (e.g., walkDir or collectFromDir) that uses readdirSync(..., { withFileTypes: true }) to traverse directories, filter for files ending with ".md", apply the existing skip rules used elsewhere, and push matching full paths into the files array; ensure collectMdFiles still seeds with MD_ROOT_FILES and MD_DIRS but calls the recursive walker for directories and returns the accumulated files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/code-style.md`:
- Around line 43-46: Add a short exception note to the Units section clarifying
that not all Pascal units use the Goccia.<Category>.<Name>.pas pattern:
shared/utility infrastructure units (e.g., BaseMap.pas, HashMap.pas)
intentionally live outside the Goccia.* namespace and follow a separate naming
convention tracked in issue `#104`; update the Units table text and mirror the
same exception wording in the other affected section (around the Classes/Units
guidance) so contributors know to follow the alternate convention for those
cross-cutting utilities.
In `@docs/contributing/tooling.md`:
- Around line 36-38: The documentation overstates the formatter's behavior:
update the "Function naming" bullet to match the actual implementation in
./format.pas and the FixFuncNames routine by specifying that it only uppercases
the first character of routine names (does not enforce internal PascalCase,
remove underscores, or re-camelize existing names); reword to say "capitalizes
the first character of function, procedure, constructor, and destructor names"
and note that it renames references within the same file but does not perform
further normalization or underscore removal.
In `@docs/interpreter.md`:
- Line 42: The docs incorrectly state that OnError is stored on TGocciaScope and
propagated to child scopes; update the text to say that OnError is on
TGocciaEvaluationContext (not TGocciaScope) and that TGocciaScope.CreateChild
only inherits ThisValue, so scopes do not propagate the error handler—adjust the
sentences at the referenced locations (including the repeated claim around lines
93-94) to reflect that error handling lives on TGocciaEvaluationContext and not
on TGocciaScope/CreateChild.
In `@docs/language.md`:
- Line 118: Update the documentation text in docs/language.md that claims tagged
templates set the cooked segment to `undefined` on malformed escape sequences;
the lexer currently throws for both tagged and untagged templates. Remove or
replace the clause referencing "Tagged templates support the TC39 Template
Literal Revision (ES2018): malformed escape sequences ... set the cooked segment
to `undefined`" and instead state that malformed template escapes are currently
rejected (lexer throws) for both tagged and untagged templates until the
deferred cooked=`undefined` behavior is implemented; ensure references to
`tagged templates`, `String.raw`, and examples like `\u{`/`\xG1` are updated
accordingly.
- Line 355: The example uses Exclude(Engine.Preprocessors, ppJSX) which won't
compile because Exclude requires a var set parameter and Engine.Preprocessors is
a property; update the documentation example to show assigning the modified set
back to the property (e.g., subtract ppJSX from Engine.Preprocessors and assign
the result) or, if disabling all preprocessors is acceptable, assign an empty
set to Engine.Preprocessors; reference Engine.Preprocessors, Exclude and ppJSX
in the explanation and replace the non-compiling call with the assignment-based
form.
In `@docs/value-system.md`:
- Around line 25-26: Diagram currently shows TGocciaNativeFunctionValue
inheriting from TGocciaValue; change the arrows so TGocciaNativeFunctionValue
inherits from TGocciaFunctionBase (i.e., place native built-ins under the
function branch) and ensure TGocciaError remains directly under TGocciaValue;
update the corresponding repeated section (the other instance referencing
TGocciaNativeFunctionValue) to match this corrected hierarchy so built-ins
appear as a subtype of TGocciaFunctionBase rather than a separate branch.
In `@README.md`:
- Around line 216-218: Replace the sentence that claims the bytecode VM
"executes directly on TGocciaValue" with wording that clarifies the VM uses a
register file of tagged TGocciaRegister values and only materializes
TGocciaValue at the runtime/object boundary; specifically update the README text
referencing the bytecode backend so it states the bytecode VM operates on
TGocciaRegister in its register file and converts to TGocciaValue only when
interacting with runtime/object APIs.
---
Outside diff comments:
In `@docs/value-system.md`:
- Around line 513-518: The docs show an outdated native callback signature using
TGocciaNativeFunctionValueCallback with TGocciaValueArray, but the codebase uses
TGocciaNativeFunctionCallback(const AArgs: TGocciaArgumentsCollection; const
AThisValue: TGocciaValue): TGocciaValue of object; update the snippet to match
the real callback type and parameter names (use TGocciaNativeFunctionCallback
and TGocciaArgumentsCollection and the const AThisValue: TGocciaValue parameter)
so examples and built-ins use the correct call surface.
---
Nitpick comments:
In `@scripts/check-doc-symbols.ts`:
- Around line 185-206: collectMdFiles currently only collects markdown at the
immediate MD_ROOT_FILES and one-level MD_DIRS; update it to recursively walk
directories so nested docs (e.g., docs/**/foo.md) are included. Replace the
existing loops in collectMdFiles with a helper recursive function (e.g., walkDir
or collectFromDir) that uses readdirSync(..., { withFileTypes: true }) to
traverse directories, filter for files ending with ".md", apply the existing
skip rules used elsewhere, and push matching full paths into the files array;
ensure collectMdFiles still seeds with MD_ROOT_FILES and MD_DIRS but calls the
recursive walker for directories and returns the accumulated files.
🪄 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: 233814dd-faba-46d7-91f4-627ff928b11b
📒 Files selected for processing (30)
.github/ISSUE_TEMPLATE/default.md.github/pull_request_template.md.github/workflows/pr.yml.markdownlint-cli2.jsoncCONTRIBUTING.mdREADME.mddocs/adding-built-in-types.mddocs/build-system.mddocs/built-ins-binary-data.mddocs/built-ins-temporal.mddocs/built-ins.mddocs/bytecode-vm.mddocs/contributing/code-style.mddocs/contributing/tooling.mddocs/contributing/workflow.mddocs/core-patterns.mddocs/decision-log.mddocs/embedding.mddocs/garbage-collector.mddocs/interpreter.mddocs/language-tables.mddocs/language.mddocs/spikes/fpc-hashmap-performance.mddocs/testing-api.mddocs/testing.mddocs/value-system.mdscripts/check-doc-duplication.tsscripts/check-doc-length.tsscripts/check-doc-links.tsscripts/check-doc-symbols.ts
✅ Files skipped from review due to trivial changes (12)
- .github/ISSUE_TEMPLATE/default.md
- .github/pull_request_template.md
- docs/spikes/fpc-hashmap-performance.md
- .markdownlint-cli2.jsonc
- docs/language-tables.md
- docs/adding-built-in-types.md
- CONTRIBUTING.md
- docs/decision-log.md
- docs/built-ins-binary-data.md
- docs/testing-api.md
- docs/bytecode-vm.md
- scripts/check-doc-duplication.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/build-system.md
- docs/core-patterns.md
- docs/testing.md
👮 Files not reviewed due to content moderation or server errors (8)
- .github/workflows/pr.yml
- docs/embedding.md
- scripts/check-doc-length.ts
- docs/contributing/workflow.md
- scripts/check-doc-links.ts
- docs/built-ins-temporal.md
- docs/garbage-collector.md
- docs/built-ins.md
- code-style.md: Note legacy utility units that predate Goccia.* naming - language.md: Fix non-compiling Exclude() example for preprocessors - value-system.md: Correct type hierarchy diagram (TGocciaFunctionBase intermediate class, TGocciaNativeFunctionValue placement) - README.md: Clarify bytecode VM uses TGocciaRegister, not TGocciaValue Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- code-style.md: Correct wording — shared infrastructure units (BaseMap, HashMap, etc.) are intentionally outside Goccia.* namespace, not legacy units that haven't been renamed - value-system.md: Update native callback signature to match actual TGocciaNativeFunctionCallback with TGocciaArgumentsCollection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/language.md (1)
118-118: Improve inline-code readability in the template-literal bullet.The semantics are correct, but this line is hard to scan due to tight punctuation around inline code spans. A small markdown cleanup will improve readability.
✏️ Suggested wording/formatting cleanup
-- Template literals with interpolation and tagged templates (``tag`...```,`String.raw`). Tagged templates support the TC39 Template Literal Revision (ES2018): malformed escape sequences (e.g.,`\u{`,`\xG1`) set the cooked segment to`undefined` while preserving the raw source text; untagged templates still throw `SyntaxError` for invalid escapes. +- Template literals with interpolation and tagged templates (e.g., ``tag`...` ``, `String.raw`). +- Tagged templates support the TC39 Template Literal Revision (ES2018): malformed escape sequences (e.g., `\u{`, `\xG1`) set the cooked segment to `undefined` while preserving the raw source text; untagged templates still throw `SyntaxError` for invalid escapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/language.md` at line 118, The "Template literals with interpolation and tagged templates" bullet is hard to scan due to tight punctuation around inline code; split it into two clearer bullets and add spacing/inline-code fences around examples like tag`...`, `String.raw`, and escape examples (`\u{`, `\xG1`) so they render cleanly. Specifically, update the bullet that begins "Template literals with interpolation and tagged templates" to first list the features (e.g., tag`...`, `String.raw`) and then a separate sentence or bullet that explains tagged-template semantics per TC39 Template Literal Revision (ES2018) about cooked becoming `undefined` for malformed escapes while preserving raw text and that untagged templates still throw `SyntaxError`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/code-style.md`:
- Around line 31-37: The compiler-directive `{H+}` is missing the `$` and should
be `{$H+}`; update the snippet so all directives use the same `{$...}` syntax
(specifically replace `{H+}` with `{$H+}`) in the block containing `{$mode
delphi}`, `{$overflowchecks on}`, `{$rangechecks on}`, and `{$modeswitch
advancedrecords}` to restore consistency and correct Pascal directive syntax.
In `@docs/value-system.md`:
- Around line 515-518: The documentation currently references the wrong callback
type name (TGocciaNativeFunctionValueCallback); update the doc to use the real
type name TGocciaNativeFunctionCallback (as defined in the native function unit)
so contributors can find the symbol when grepping; search for occurrences of
TGocciaNativeFunctionValueCallback in docs/value-system.md and replace them with
TGocciaNativeFunctionCallback and verify the surrounding description matches the
signature in TGocciaNativeFunctionCallback from the native function
implementation.
- Line 223: The docs are inconsistent about special-number representation;
update the narrative so all sections (the paragraph referencing
IsNaN/IsInfinity/IsNegativeZero and the section describing tagged special values
including FValue) agree that special values are represented via tagged
value-level encodings that may store 0.0 internally, and that correctness must
rely on the provided accessors IsNaN, IsInfinity, and IsNegativeZero rather than
raw floating-point comparisons or assuming Math.* behavior; change the sentence
claiming direct delegation to Math primitives to state that those accessors
delegate to platform-safe checks (e.g., Math.IsNaN/IsInfinite or endian-neutral
Int64 sign-bit logic) and add a clarifying note that FValue == 0.0 is a known
storage artifact handled by the accessors.
---
Nitpick comments:
In `@docs/language.md`:
- Line 118: The "Template literals with interpolation and tagged templates"
bullet is hard to scan due to tight punctuation around inline code; split it
into two clearer bullets and add spacing/inline-code fences around examples like
tag`...`, `String.raw`, and escape examples (`\u{`, `\xG1`) so they render
cleanly. Specifically, update the bullet that begins "Template literals with
interpolation and tagged templates" to first list the features (e.g., tag`...`,
`String.raw`) and then a separate sentence or bullet that explains
tagged-template semantics per TC39 Template Literal Revision (ES2018) about
cooked becoming `undefined` for malformed escapes while preserving raw text and
that untagged templates still throw `SyntaxError`.
🪄 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: 98c7b9f3-2267-443c-8be6-4263ff8c85f3
📒 Files selected for processing (4)
README.mddocs/contributing/code-style.mddocs/language.mddocs/value-system.md
✅ Files skipped from review due to trivial changes (1)
- README.md
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/value-system.md (1)
223-223:⚠️ Potential issue | 🟡 MinorNumber model docs remain internally contradictory (flagged in previous review).
Line 223 states accessors delegate to
Math.IsNaN/Math.IsInfinitewithout mentioning storage artifacts, while lines 620-629 describe theFValue = 0.0pitfall. This creates the same contributor confusion flagged in the previous review: someone reading line 223 won't realize rawValuecomparisons are unsafe.The previous review's suggested consistency rewrite still applies: update line 223 to state that accessors delegate to platform-safe checks and note that
FValue == 0.0is a storage artifact handled by the accessors.Based on learnings: In frostney/GocciaScript tests/docs context, special values can store 0.0 internally and correctness should rely on IsNaN/value-level helpers rather than raw floating-point checks.
Also applies to: 620-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/value-system.md` at line 223, Update the docs so the description of the special-value accessors clarifies they perform platform-safe/value-level checks (not naive raw storage comparisons): change the sentence about IsNaN, IsInfinity and IsNegativeZero to say they delegate to platform-safe checks (e.g., Math.IsNaN/Math.IsInfinite or an endian-neutral sign-bit check) and explicitly note that FValue may store 0.0 as a storage artifact so callers must use the accessors rather than testing Value/FValue == 0.0; apply the same clarification language to the section around the FValue = 0.0 pitfall (lines ~620-629) and reference the IsNaN/IsInfinity/IsNegativeZero helpers where appropriate.docs/contributing/code-style.md (1)
31-37:⚠️ Potential issue | 🟡 MinorFix the compiler-directive syntax inconsistency on line 33.
Line 33 shows
{H+}without the$prefix, while all other directives in this block use the{$...}format. For consistency in this authoritative style guide, change it to{$H+}.📝 Proposed fix
{$mode delphi} // Delphi-compatible mode -{H+} // Long strings by default +{$H+} // Long strings by default {$overflowchecks on} // Runtime overflow checking🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/code-style.md` around lines 31 - 37, Replace the inconsistent Pascal compiler directive "{H+}" with the correct "$-prefixed" form "{$H+}" in the code block so all directives use the "{$...}" syntax; update the directive inside the fenced Pascal block (the line currently showing {H+}) to "{$H+}" to match other lines like "{$mode delphi}" and "{$overflowchecks on}".
🧹 Nitpick comments (1)
docs/contributing/code-style.md (1)
58-58: Consider simplifying "very small scopes" to "small scopes".The intensifier "very" can be omitted without losing clarity: "Short single-letter names in small scopes (e.g. loop
I,J) remain acceptable."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/code-style.md` at line 58, Update the phrasing in the guideline sentence that reads "Short single-letter names in very small scopes (e.g. loop `I`, `J`) remain acceptable." to remove "very" so it reads "Short single-letter names in small scopes (e.g. loop `I`, `J`) remain acceptable."; ensure the surrounding sentence about avoiding snake_case/mixed_Case and using descriptive PascalCase names (e.g., `PrimaryScope`, `FallbackScope`) is unchanged and that the examples `I`, `J` remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/contributing/code-style.md`:
- Around line 31-37: Replace the inconsistent Pascal compiler directive "{H+}"
with the correct "$-prefixed" form "{$H+}" in the code block so all directives
use the "{$...}" syntax; update the directive inside the fenced Pascal block
(the line currently showing {H+}) to "{$H+}" to match other lines like "{$mode
delphi}" and "{$overflowchecks on}".
In `@docs/value-system.md`:
- Line 223: Update the docs so the description of the special-value accessors
clarifies they perform platform-safe/value-level checks (not naive raw storage
comparisons): change the sentence about IsNaN, IsInfinity and IsNegativeZero to
say they delegate to platform-safe checks (e.g., Math.IsNaN/Math.IsInfinite or
an endian-neutral sign-bit check) and explicitly note that FValue may store 0.0
as a storage artifact so callers must use the accessors rather than testing
Value/FValue == 0.0; apply the same clarification language to the section around
the FValue = 0.0 pitfall (lines ~620-629) and reference the
IsNaN/IsInfinity/IsNegativeZero helpers where appropriate.
---
Nitpick comments:
In `@docs/contributing/code-style.md`:
- Line 58: Update the phrasing in the guideline sentence that reads "Short
single-letter names in very small scopes (e.g. loop `I`, `J`) remain
acceptable." to remove "very" so it reads "Short single-letter names in small
scopes (e.g. loop `I`, `J`) remain acceptable."; ensure the surrounding sentence
about avoiding snake_case/mixed_Case and using descriptive PascalCase names
(e.g., `PrimaryScope`, `FallbackScope`) is unchanged and that the examples `I`,
`J` remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23a3bd05-51b4-431b-aa99-92c18e2985f8
📒 Files selected for processing (2)
docs/contributing/code-style.mddocs/value-system.md
Remove outdated "dedicated enum" wording from design rationale — the actual implementation uses a single Double with IEEE 754 property accessors (IsNaN, IsInfinity, IsNegativeZero). Align the rationale section with the Numbers section above. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@docs/value-system.md`:
- Line 661: Update the phrasing in the descriptor-merging sentence that
references the ECMAScript wording: locate the sentence mentioning "ECMAScript
spec behavior" near the `Object.defineProperty` example and replace it with a
clearer phrase such as "ECMAScript specification behavior" (or simply
"ECMAScript behavior") so the line reads smoothly while preserving the example
about `Object.defineProperty(obj, "x", { enumerable: false })` and the
explanation that unspecified attributes are preserved.
🪄 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: f1ef1221-a515-43b8-b228-94097581b46e
📒 Files selected for processing (1)
docs/value-system.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/value-system.md (1)
96-110:⚠️ Potential issue | 🟡 MinorRemove
TGocciaErrorfrom the type hierarchy diagram.
TGocciaErroris still rendered inside theTGocciaValuehierarchy diagram, but it is not aTGocciaValuesubtype (it extends PascalException). Please remove this class block from the hierarchy diagram to avoid reintroducing the same confusion.Based on learnings: In GocciaScript (
docs/value-system.md),TGocciaErrorextends Pascal'sException, notTGocciaValue, and should not appear in theTGocciaValuetype hierarchy diagram.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/value-system.md` around lines 96 - 110, Remove the TGocciaError class block from the TGocciaValue hierarchy diagram in docs/value-system.md because TGocciaError is not a TGocciaValue subtype (it extends Pascal Exception); locate the diagram entries (the class blocks such as TGocciaNativeFunctionValue, TGocciaArrayBufferValue, TGocciaSharedArrayBufferValue, TGocciaTypedArrayValue and TGocciaError) and delete only the TGocciaError block so the diagram reflects that TGocciaValue's subtypes remain while TGocciaError is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/value-system.md`:
- Around line 96-110: Remove the TGocciaError class block from the TGocciaValue
hierarchy diagram in docs/value-system.md because TGocciaError is not a
TGocciaValue subtype (it extends Pascal Exception); locate the diagram entries
(the class blocks such as TGocciaNativeFunctionValue, TGocciaArrayBufferValue,
TGocciaSharedArrayBufferValue, TGocciaTypedArrayValue and TGocciaError) and
delete only the TGocciaError block so the diagram reflects that TGocciaValue's
subtypes remain while TGocciaError is omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7c2cc9c-de8f-48fe-9bc2-46ece8af5066
📒 Files selected for processing (1)
docs/value-system.md
TGocciaError extends Exception, not TGocciaValue — it doesn't belong in the TGocciaValue subtype diagram. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/value-system.md (1)
680-685:⚠️ Potential issue | 🟡 MinorError model wording appears internally inconsistent.
Line 680+ describes runtime errors as
TGocciaErrorinstances, but Line 393 says throw helpers buildTGocciaObjectValueand raiseTGocciaThrowValue. Please clarify this section so contributors don’t infer the wrong runtime error type path.Based on learnings: In GocciaScript (
docs/value-system.md),TGocciaErrorextends Pascal’sException, notTGocciaValue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/value-system.md` around lines 680 - 685, The documentation is internally inconsistent about runtime error types: update the text to clearly state that TGocciaError is a Pascal Exception subclass (extends Exception) and not a TGocciaValue, and then explicitly distinguish that the throw helpers (referenced as TGocciaObjectValue and TGocciaThrowValue) construct a TGocciaObjectValue payload and raise it via TGocciaThrowValue at runtime; revise the paragraphs around TGocciaError, TGocciaObjectValue, TGocciaThrowValue, and TGocciaValue to reflect this separation (TGocciaError = Exception subclass; throw helpers = build TGocciaObjectValue and raise TGocciaThrowValue) so readers cannot conflate TGocciaError with TGocciaValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/value-system.md`:
- Around line 625-626: The documentation's example for IsActualZero conflicts
with the warning about negative zero; update the doc text around the
IsActualZero example to state that the shown predicate `(Value = 0) and not
IsNaN and not IsInfinite` (as implemented in Goccia.Evaluator.Arithmetic.pas
IsActualZero) is intentionally used for exponentiation where `-0` is treated as
zero, and add a note that in other contexts you should include an explicit
IsNegativeZero guard or use the NumericRank behavior in
Goccia.Values.ArrayValue.pas to distinguish `-0`; alternatively, replace the
example with the guarded form if you prefer the universal rule—be sure to
reference IsActualZero and NumericRank in the updated wording.
---
Outside diff comments:
In `@docs/value-system.md`:
- Around line 680-685: The documentation is internally inconsistent about
runtime error types: update the text to clearly state that TGocciaError is a
Pascal Exception subclass (extends Exception) and not a TGocciaValue, and then
explicitly distinguish that the throw helpers (referenced as TGocciaObjectValue
and TGocciaThrowValue) construct a TGocciaObjectValue payload and raise it via
TGocciaThrowValue at runtime; revise the paragraphs around TGocciaError,
TGocciaObjectValue, TGocciaThrowValue, and TGocciaValue to reflect this
separation (TGocciaError = Exception subclass; throw helpers = build
TGocciaObjectValue and raise TGocciaThrowValue) so readers cannot conflate
TGocciaError with TGocciaValue.
🪄 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: 9ca14cbd-7c7d-4b5e-bb1d-13ab24ec67df
📒 Files selected for processing (1)
docs/value-system.md
IsActualZero intentionally treats -0 as zero (correct for exponentiation). Note that contexts needing -0 distinction should use IsNegativeZero. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comprehensive docs: docs/errors.md (error types, display, JSON envelope, Error.cause, embedding notes) and docs/goals.md (project goals, sandboxing and embedding rationale). Update README table to link the new pages and reference errors.md. Propagate errors.md cross-references into embedding.md, interpreter.md, language.md, language-tables.md, and value-system.md. Consolidate and expand testing guidance: remove duplicate testing guidance from testing-api.md and enrich testing.md with testing layers, CLI behavior/CI checks, cross-platform newline rules, and test metadata examples. Minor wording/formatting tweaks across docs to reflect the new documentation structure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/value-system.md (1)
682-687:⚠️ Potential issue | 🟠 MajorAlign the Error Values section with the object-based JS error model.
This section currently says constructors create
TGocciaErrorinstances, but earlier in this same doc (Line 393) you document runtime throws viaTGocciaObjectValue+TGocciaThrowValue. That mismatch can mislead contributors implementing error paths.🛠️ Proposed doc fix
-`TGocciaError` carries JavaScript error information: - -- `Name` — Error type (`Error`, `TypeError`, `ReferenceError`, `RangeError`) -- `Message` — Error description - -Error constructors are registered as globals, creating `TGocciaError` instances that can be `throw`n and `catch`ed. +JavaScript errors are represented as `TGocciaObjectValue` instances (with `name` and `message`) and propagated via `TGocciaThrowValue`. + +- `name` — Error type (`Error`, `TypeError`, `ReferenceError`, `RangeError`) +- `message` — Error description + +Error constructors are registered as globals and produce these JS error objects, which are then `throw`n/`catch`ed through the runtime throw-value flow.Based on learnings: In frostney/GocciaScript (docs/value-system.md),
TGocciaErrorextends Pascal'sException, notTGocciaValue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/value-system.md` around lines 682 - 687, The doc currently claims error constructors create TGocciaError instances, but this conflicts with earlier documentation stating runtime throws use TGocciaObjectValue + TGocciaThrowValue and that TGocciaError extends Pascal's Exception rather than TGocciaValue; update the "Error Values" section to align with the object-based JS error model by describing that JS errors are represented as TGocciaObjectValue objects (with fields Name and Message) and that throwing is implemented via TGocciaThrowValue carrying that object, and note that TGocciaError is a Pascal Exception subtype (not a TGocciaValue) to avoid confusion between TGocciaError, TGocciaObjectValue, TGocciaThrowValue, TGocciaValue and Exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/errors.md`:
- Around line 15-16: Update the statement that "All error types inherit from
`Error` and work with `instanceof`" to scope it to JavaScript-visible
constructors only and add an explicit note that the CLI-only `TimeoutError` is
not exposed to JS; mention `Error`, `instanceof`, and `TimeoutError` so
reviewers can find and adjust the sentence and add the clarification about the
CLI-only nature of `TimeoutError`.
- Around line 146-147: Update the documentation wording to avoid saying "all
error constructors accept an optional second argument" — clarify that most
built‑in errors (Error, TypeError, RangeError, ReferenceError, SyntaxError,
URIError) accept an options object with a cause as their second parameter, but
AggregateError uses the signature new AggregateError(errors, message, options)
(options is third); also remove SuppressedError from the list that supports
cause and note that SuppressedError's constructor is new SuppressedError(error,
suppressed, message) and does not accept an options/cause argument.
In `@docs/testing.md`:
- Around line 231-239: Update the example TestRunner commands so they run the
full suite with ASI enabled: change the interpreted invocation
"./build/TestRunner tests" to "./build/TestRunner tests --asi" and the bytecode
example "./build/TestRunner tests --mode=bytecode" to "./build/TestRunner tests
--mode=bytecode --asi" (or explicitly note the commands are for non-ASI subsets)
to ensure tests under tests/language/asi/* are included; adjust the surrounding
text to state both modes must pass with --asi present.
---
Outside diff comments:
In `@docs/value-system.md`:
- Around line 682-687: The doc currently claims error constructors create
TGocciaError instances, but this conflicts with earlier documentation stating
runtime throws use TGocciaObjectValue + TGocciaThrowValue and that TGocciaError
extends Pascal's Exception rather than TGocciaValue; update the "Error Values"
section to align with the object-based JS error model by describing that JS
errors are represented as TGocciaObjectValue objects (with fields Name and
Message) and that throwing is implemented via TGocciaThrowValue carrying that
object, and note that TGocciaError is a Pascal Exception subtype (not a
TGocciaValue) to avoid confusion between TGocciaError, TGocciaObjectValue,
TGocciaThrowValue, TGocciaValue and Exception.
🪄 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: 46125e62-a4da-43ae-b337-7b960c78e94c
📒 Files selected for processing (10)
README.mddocs/embedding.mddocs/errors.mddocs/goals.mddocs/interpreter.mddocs/language-tables.mddocs/language.mddocs/testing-api.mddocs/testing.mddocs/value-system.md
✅ Files skipped from review due to trivial changes (4)
- docs/language-tables.md
- docs/embedding.md
- docs/goals.md
- docs/testing-api.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
…inements - errors.md: Clarify TimeoutError is CLI-only, not a JS constructor. Fix Error.cause argument position documentation — second for most error types, third for AggregateError, fourth for SuppressedError. - testing.md: Add --asi flag to Run All Tests examples (CI uses it). Fix Pascal unit tests labeled as tertiary consistently. - goals.md: Inline roadmap items, remove honest assessment heading, remove high-stakes security wording. - language-tables.md: Trim runtime extensions to spec/proposal reference. - value-system.md: Fix IsActualZero context, ECMAScript spec wording, remove TGocciaError from hierarchy diagram. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Describe JavaScript errors as `TGocciaObjectValue` instances - Explain `TGocciaThrowValue` vs internal `TGocciaError` exceptions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/testing.md (1)
497-500: Keep the “when to write tests” guidance consistent with the 3-layer modelLine 497–500 currently presents a JS-vs-Pascal split, which omits the documented CLI behavior layer and can misdirect contributors for CLI-facing behavior.
♻️ Suggested update
-- **JavaScript tests** (preferred) — For any behavior observable from script code. This is almost everything. -- **Pascal unit tests** — Only for internal implementation details not reachable from script code (e.g., the test framework's own pass/fail recording logic). +- **JavaScript tests** (primary, preferred) — For behavior observable from script code. This is most feature work. +- **CLI behavior tests** (secondary) — For command-surface behavior (`ScriptLoader`, `TestRunner`, `BenchmarkRunner`) such as flags, output envelopes/formats, and error presentation. +- **Pascal unit tests** (tertiary) — For internal implementation details not reachable through documented user-facing script/CLI entry points (e.g., the test framework's own pass/fail recording logic).Based on learnings, this PR’s testing docs intentionally use a three-layer priority (JavaScript → CLI → Pascal).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing.md` around lines 497 - 500, Update the "When to write Pascal unit tests vs JavaScript tests" guidance to follow the three-layer priority model (JavaScript → CLI → Pascal) instead of a binary JS vs Pascal split: state that JavaScript tests are preferred for behaviors observable from script code, that CLI-level tests should cover CLI-facing behavior and integrations exposed via the command-line, and that Pascal unit tests are reserved only for internal implementation details not reachable from script or CLI (e.g., test framework internals); adjust the bullet list to explicitly include the CLI layer and reorder/examples accordingly so contributors know to prefer JS tests, then CLI tests, then Pascal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing.md`:
- Around line 409-410: Update the sentence "Pascal unit tests are the tertiary
testing layer for low-level value system internals." to a broader description
that explicitly includes other internal concerns (e.g., function/scope behavior,
test framework internals, and memory/type conversion edge cases), so the
paragraph reflects the wider coverage shown in the table and avoids
underspecifying the role of Pascal tests.
---
Nitpick comments:
In `@docs/testing.md`:
- Around line 497-500: Update the "When to write Pascal unit tests vs JavaScript
tests" guidance to follow the three-layer priority model (JavaScript → CLI →
Pascal) instead of a binary JS vs Pascal split: state that JavaScript tests are
preferred for behaviors observable from script code, that CLI-level tests should
cover CLI-facing behavior and integrations exposed via the command-line, and
that Pascal unit tests are reserved only for internal implementation details not
reachable from script or CLI (e.g., test framework internals); adjust the bullet
list to explicitly include the CLI layer and reorder/examples accordingly so
contributors know to prefer JS tests, then CLI tests, then Pascal.
🪄 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: bb97e606-67ed-4cb4-bebd-c42e9255cb80
📒 Files selected for processing (3)
docs/errors.mddocs/testing.mddocs/value-system.md
- testing.md: Broaden Pascal test description from "value system internals" to include function/scope internals and test-framework state behavior - value-system.md: Fix Error Values section to accurately describe the three-class model (TGocciaObjectValue for JS errors, TGocciaThrowValue for throw propagation, TGocciaError for Pascal parser exceptions) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/testing.md (1)
391-406: Consider marking the CLI checklist table as non-exhaustive to reduce doc drift.This table is detailed and helpful, but it can become stale as
pr.ymlevolves. Adding a short “non-exhaustive; workflow is source of truth” note would keep maintenance lighter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing.md` around lines 391 - 406, Update the CLI checklist table in docs/testing.md to include a short one-line note that the table is non-exhaustive and that the pr.yml workflow is the source of truth; edit the block containing the table (the lines listing "TestRunner JSON output", "ScriptLoader JSON output", etc.) to append a sentence such as "This checklist is non-exhaustive — see the pr.yml workflow for the authoritative test steps" so readers know to consult pr.yml for the canonical behavior and to reduce doc drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/testing.md`:
- Around line 391-406: Update the CLI checklist table in docs/testing.md to
include a short one-line note that the table is non-exhaustive and that the
pr.yml workflow is the source of truth; edit the block containing the table (the
lines listing "TestRunner JSON output", "ScriptLoader JSON output", etc.) to
append a sentence such as "This checklist is non-exhaustive — see the pr.yml
workflow for the authoritative test steps" so readers know to consult pr.yml for
the canonical behavior and to reduce doc drift.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@docs/testing.md`:
- Line 361: The sentence currently reads "lexer → parser → evaluator" and is
interpreter-specific; update the wording to be mode-agnostic by replacing that
phrase with something inclusive of bytecode/VM execution (for example "lexer →
parser → interpreter/VM execution" or "lexer → parser → evaluator/VM") so the
primary testing pipeline covers both interpreter and bytecode modes; edit the
occurrence of the phrase "lexer → parser → evaluator" in docs/testing.md
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Changed "lexer → parser → evaluator" to "lexer → parser → interpreter/VM execution" since tests run in both modes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/testing.md (1)
475-487:⚠️ Potential issue | 🟠 MajorOutdated NaN implementation description.
Lines 486-487 describe the OLD enum-backed design (
FSpecialValue = nsvNaNflag withFValueset to 0) that was already removed. Based on learnings,TGocciaNumberLiteralValuenow stores a singleDoublepayload (FValue) where special values are represented as standard IEEE 754 bit patterns, not via a separate enum field.The guidance to use
Value.ToNumberLiteral.IsNaNinstead ofMath.IsNaNis still correct, but the explanation is wrong.📝 Corrected explanation
### NaN Checks in Pascal Tests When testing for NaN, use the `IsNaN` property on `TGocciaNumberLiteralValue` rather than `Math.IsNaN()`: ```pascal // Correct — uses the internal special value flag Expect<Boolean>(Value.ToNumberLiteral.IsNaN).ToBe(True); -// WRONG — NaN values store 0.0 internally, so Math.IsNaN returns False -IsNaN(Value.ToNumberLiteral.Value) // always False! +// WRONG — Direct double access bypasses the value-level accessor +Math.IsNaN(Value.ToNumberLiteral.Value) // may not work as expected-This is because GocciaScript represents NaN via a
FSpecialValue = nsvNaNflag withFValueset to 0 (avoiding floating-point NaN propagation issues). TheIsNaNproperty checks the flag;Math.IsNaNchecks the stored double.
+TGocciaNumberLiteralValuestores special values (NaN, Infinity, -Infinity, -0) as standard IEEE 754 bit patterns inFValue. TheIsNaNproperty accessor correctly detects the NaN bit pattern; always use the value-level accessors for correctness.</details> Based on learnings: "TGocciaNumberLiteralValue stores a single Double payload (FValue); special values (NaN, Infinity, -Infinity, -0) are represented as standard IEEE 754 bit patterns and are detected via property accessors (IsNaN, IsInfinity, IsNegativeZero) — not via a separate enum field or 0.0 storage artifact. The old enum-backed design (where special values stored FValue = 0.0) was removed; always use the value-level accessors for correctness." <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/testing.mdaround lines 475 - 487, The docs incorrectly describe an old
enum-backed NaN representation; update the paragraph around
TGocciaNumberLiteralValue so it no longer claims FSpecialValue/nsvNaN or that
FValue is set to 0.0. Replace with a concise statement that
TGocciaNumberLiteralValue stores a single Double in FValue using standard
IEEE‑754 bit patterns for NaN/Infinity/-0 and that the
IsNaN/IsInfinity/IsNegativeZero property accessors must be used (rather than
calling Math.IsNaN on FValue) to detect special values correctly.</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Outside diff comments: In `@docs/testing.md`: - Around line 475-487: The docs incorrectly describe an old enum-backed NaN representation; update the paragraph around TGocciaNumberLiteralValue so it no longer claims FSpecialValue/nsvNaN or that FValue is set to 0.0. Replace with a concise statement that TGocciaNumberLiteralValue stores a single Double in FValue using standard IEEE‑754 bit patterns for NaN/Infinity/-0 and that the IsNaN/IsInfinity/IsNegativeZero property accessors must be used (rather than calling Math.IsNaN on FValue) to detect special values correctly.
Clarify project docs and workflow: mark decision-log entries as immutable and require adding new entries for architectural/design decisions; update CONTRIBUTING and the workflow guide to reference the decision log. Standardize number representation documentation—numbers now use a single IEEE‑754 Double and callers should use IsNaN/IsInfinity/IsNegativeZero property accessors (replace previous enum-based guidance). Update tests and adding-built-in-types docs to prefer property accessors, and add/reorganize recent entries in docs/decision-log.md to reflect the current history.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/decision-log.md (1)
16-16: Tighten redundant phrasing in the immutability note.“at that point in time” is wordy; “at that point” reads cleaner without changing meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decision-log.md` at line 16, Replace the phrase "at that point in time" with the tighter wording "at that point" in the immutable entry sentence inside decision-log.md (the sentence starting "Each entry records what was decided..."). Keep the rest of the sentence unchanged and preserve the note about adding a new entry and updating links.CONTRIBUTING.md (1)
31-31: Minor wording cleanup for readability.“at that point in time” can be shortened to “at that point” with no loss of meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 31, Update the phrasing in the CONTRIBUTING.md bullet titled "Decision log entries are immutable" by replacing "at that point in time" with the shorter "at that point" while keeping the rest of the sentence intact and preserving the reference to docs/decision-log.md; ensure the header text "Decision log entries are immutable" and the instructions about adding a new entry and updating links remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/workflow.md`:
- Line 55: Update the sentence in the "Add or update tests" bullet so Pascal
test guidance explicitly includes scope-chain changes: when referring to Pascal
units under `units/*.Test.pas`, append "scope chain" to the list (e.g., "when
you touch AST, evaluator, value types, or scope chain") so native Pascal tests
are triggered for scope-chain behavior changes.
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 31: Update the phrasing in the CONTRIBUTING.md bullet titled "Decision
log entries are immutable" by replacing "at that point in time" with the shorter
"at that point" while keeping the rest of the sentence intact and preserving the
reference to docs/decision-log.md; ensure the header text "Decision log entries
are immutable" and the instructions about adding a new entry and updating links
remain unchanged.
In `@docs/decision-log.md`:
- Line 16: Replace the phrase "at that point in time" with the tighter wording
"at that point" in the immutable entry sentence inside decision-log.md (the
sentence starting "Each entry records what was decided..."). Keep the rest of
the sentence unchanged and preserve the note about adding a new entry and
updating links.
🪄 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: 40e70a80-aacd-442b-9a54-1ddfcc2cfa65
📒 Files selected for processing (6)
CONTRIBUTING.mddocs/adding-built-in-types.mddocs/contributing/workflow.mddocs/decision-log.mddocs/testing.mddocs/value-system.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/adding-built-in-types.md
- Replace `MD_FALLOW_*` env vars with `DOC_DUP_*` - Ignore URLs during tokenization and fuzzy paragraph checks - Update scanner banner to `doc-duplication`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check-doc-duplication.ts`:
- Around line 547-560: The linkText for suggestions is built using
relative(root, canon.file) which produces wrong links when pasted into duplicate
files in nested dirs; update the construction to compute a per-duplicate
relative path from the duplicate's directory to the canonical file (e.g. use
path.relative(dirname(duplicate.file), canon.file) for each entry in
suggestions. Locate the suggestions push where variables sorted, canon,
duplicates, and linkText are used and replace the single relPath based on root
with a per-duplicate relPath (or store a per-duplicate replacement string) so
each duplicates[] entry contains the correct relative link; apply the same
change to the similar block around the 563-577 region.
- Around line 121-123: The duplication scanner is treating vendored assistant
docs as repo-owned because ".agents" isn't ignored; update the IGNORE_DIRS Set
(symbol IGNORE_DIRS in scripts/check-doc-duplication.ts) to include ".agents"
(or the vendored subtree name) so the walker skips vendored docs like
.agents/skills/resolve-reviews/SKILL.md; leave EXTENSIONS unchanged.
- Around line 704-717: The corpus currently concatenates tokens from every file
into one continuous stream which allows suffix-array matches to span file
boundaries; after the call to tokenise(...) that pushes a file's tokens into
corpus (within the loop that also uses buildHeadingIndex, tokenise,
extractParagraphs), append a synthetic boundary token object (e.g., a unique
word like "__doc_boundary_<unique>" with a loc pointing to the file and a line
just past the last line) to corpus so the suffix-array cannot match across
files; ensure you create the boundary after tokenise(...) and before calling
extractParagraphs(...) (use the existing file and lines variables to build a
unique marker and location).
🪄 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: bfc7b0ee-950e-494b-bc34-a3ecf287232e
📒 Files selected for processing (1)
scripts/check-doc-duplication.ts
…tches, optimize sort - Add .agents to IGNORE_DIRS so vendored skill docs are excluded - Insert unique sentinel words between files to prevent suffix-array matches from spanning file boundaries - Cap suffix comparison depth at 64 words (12× speedup: 2:24 → 12s) - Rename MD_FALLOW env vars to DOC_DUP for clarity - Strip URLs before tokenisation to reduce false similarity - Fix architecture.md overview duplication with bytecode-vm.md - Add decision log workflow step to contributing guide - Add 5 missing decision log entries (VM unification, ASI, profiling, Goccia.build, source maps) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
No description provided.