feat(generate)!: emit self-contained @forst/gen TS client package - #158
Merged
Conversation
…kage Replace the dual generated/ and client/ layout with one configurable package under .forst/client by default. Emit ESM JavaScript and declaration files with per-package subpath exports, an inlined invoke transport including streaming, tagged invoke failures, per-call options, a testing subpath with scoped overrides, optional Effect mode, atomic byte-stable writes, watch regeneration, node_modules linking, and provider omission reporting. Cut examples and docs over to the new import surface and require a postinstall generate lifecycle script for ephemeral output. BREAKING CHANGE: forst generate no longer writes sibling generated/ and client/ directories or depends on @forst/client. Adopters must import from @forst/gen or a configured packageName via subpaths, add a postinstall script that runs forst generate, and delete any leftover generated/ or client/ trees themselves. Forst never removes those directories.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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:
WalkthroughThis PR replaces legacy TypeScript output with a self-contained ChangesGenerated client platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 63
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forst/cmd/forst/generate_test.go (1)
604-618: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate core module write failures to
generate.
TestGenerateCommand_writeClientModuleLogsErrormakesdist/core/main.jswrite fail, butrunGenerateOnceonly logs and continues. The package./mainexport points todist/pkg/main.js, anddist/pkg/main.jsre-exports../core/main.js; a missing core file makes the package resolve at import time. Return an error for failed core outputs so the command does not ship an incomplete package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_test.go` around lines 604 - 618, Update runGenerateOnce to propagate errors from writing core module outputs, including dist/core/main.js, instead of only logging and continuing. Ensure generateCommand receives and returns that error so failed core generation prevents shipping an incomplete package, while preserving existing handling for unrelated outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/interop/node/effect.mdx`:
- Around line 128-139: Update the code snippet’s imports to include the
ComparePassword function used by the Effect.runPromise example, alongside the
existing Bcrypt import, so the snippet has no unresolved identifiers.
In `@docs/interop/node/generate-types.mdx`:
- Line 75: Update the sentence in the documentation to hyphenate the compound
modifier as “provider-gated functions,” preserving the rest of the wording and
capitalization.
In `@examples/in/rfc/node-interop/remix-serve/app/routes/_index.tsx`:
- Around line 15-16: Update the FORST_BASE_URL assignment to treat an empty
string as unset by using the same fallback behavior as mergeForstSidecarEnv,
while preserving any non-empty configured URL.
- Line 11: Add JSDoc directly above the TodoRow type alias documenting that it
widens ListTodos.encoded fields to string, represents tab-separated data, and
has no runtime validation. Keep the existing TodoRow shape unchanged.
In `@examples/in/tictactoe/tests/tictactoe-game.simulation.test.ts`:
- Line 170: Preserve the existing board-state assertions in the tictactoe
simulation test and retain the state.winner assertion as an additional contract
check. Ensure the test explicitly verifies that the first three board cells
contain "X" before or alongside validating winner is "X".
In `@forst/cmd/forst/dev_server_typescript_test.go`:
- Around line 188-194: Rename the test function currently named
TestTypeScriptGenerator_GenerateTypesForFunctions_conflictingSignatures_returnsError
to precisely describe successful cross-package type generation, reflecting that
conflicting signatures now merge without error. Keep the test implementation and
assertions unchanged.
In `@forst/cmd/forst/generate_acceptance_test.go`:
- Around line 359-426: Split the independent scenarios into named t.Run
subtests, preferably using table-driven cases:
forst/cmd/forst/generate_acceptance_test.go:359-426 should separate default
name, adopter rename, spaced directory, and configured packageName; :493-573
should separate duplicate function name, conflicting Config type, and identical
Address merge; :576-642 should separate near-reserved names, rejected testing
package, and testingSubpath override. Ensure each subtest owns its setup and
failures identify the scenario.
- Around line 729-731: Update the .gitignore validation in the acceptance test
to check only for the directory pattern ".forst/". Remove the redundant
strings.Contains check for ".forst" while preserving the existing failure
behavior when the directory pattern is absent.
- Around line 218-233: Update the tsConfig Include construction in the
acceptance-test generation flow to derive the client declaration glob from
defaultClientDistDir relative to projectRoot, matching the shared logic in
writeTSConfig. Remove the hardcoded .forst/client/dist path while preserving the
existing app-smoke.ts and shim entries.
In `@forst/cmd/forst/generate_cross_file_test.go`:
- Around line 42-48: Update the merged-type assertion in the test around the
types string variable s to check for the precise declaration form of type R,
rather than any standalone capital “R”. Keep the existing negative assertion
preventing “export function GetX” signatures unchanged.
In `@forst/cmd/forst/generate_effect_test.go`:
- Around line 8-26: Update TestEffectVersionAtLeast to wrap each table entry in
a named t.Run subtest, using a descriptive case name derived from the version
and floor values. Move the assertion inside the subtest and replace t.Fatalf
with t.Errorf so all table cases execute and failures identify the specific
input.
In `@forst/cmd/forst/generate_effect.go`:
- Around line 93-108: Replace parseSemverPrefix with complete SemVer validation
and parsing, rejecting prerelease values, malformed numeric components, and
versions with extra components instead of coercing or ignoring them. Update
effectVersionAtLeast to compare only successfully parsed stable three-component
versions, preserving correct boundary behavior for 3.16.99, 3.17.0-beta.1, and
3.17.0.
In `@forst/cmd/forst/generate_guard.go`:
- Around line 21-27: Update the package-manifest check in the guard-generation
flow to walk all ancestor package.json files rather than stopping at the result
of findNearestPackageJSON. Evaluate packageJSONRunsForstGenerate for each
manifest and return as soon as one contains the forst generate script; otherwise
continue to the filesystem root so nearer manifests without the script do not
end the search.
In `@forst/cmd/forst/generate_link_test.go`:
- Around line 27-44: Make link-related tests portable across symlinks, Windows
junctions, and copy fallbacks. In forst/cmd/forst/generate_link_test.go:27-44,
update the generated-client assertion to verify the resolved path matches outDir
without requiring os.ModeSymlink. At lines 126-151 and 284-301, replace direct
os.Symlink fixture creation with the platform-aware test helper used for link
setup.
In `@forst/cmd/forst/generate_link.go`:
- Around line 180-182: Update the documentation comment for
assertLinkReplaceable to refer to the parameter as isSymlinkTarget instead of
foreignIsDir, keeping the described symlink-target behavior unchanged.
- Around line 360-376: Update copyFileContents to close the destination file
explicitly after io.Copy and return any out.Close error, while preserving the
existing copy error handling and source cleanup.
- Around line 110-115: Lower the “node_modules link ready” log in
linkGeneratedClient from Info to Debug so unchanged links do not produce noisy
user-facing output; keep the existing fields and message intact.
In `@forst/cmd/forst/generate_phase1_test.go`:
- Line 176: Update the root export handling around exports["."] to use a
comma-ok type assertion instead of panicking. When the assertion fails because
the root export is absent or has the wrong type, fail the test while including
the JSON payload in the failure message; preserve the existing map-based
processing for valid root exports.
- Around line 19-25: Centralize the client output default across all three
tests: in forst/cmd/forst/generate_phase1_test.go:19-25, derive
defaultClientOutDir from ftconfig.EffectiveGenerateConfig(nil, "").OutDir and
retain one test pinning .forst/client; in
forst/cmd/forst/generate_acceptance_test.go:228-232 and
forst/cmd/forst/generate_tsc_test.go:163-167, construct the **/*.d.ts include
pattern from defaultClientDistDir(projectRoot) relative to projectRoot.
In `@forst/cmd/forst/generate_phase2_test.go`:
- Line 41: Replace unchecked JSON type assertions in generate_phase2_test.go at
lines 41, 92, 97, 114-116, and 127 with comma-ok checks, failing with the
decoded JSON j or offending entry value as specified; update
generate_phase1_test.go line 176 similarly for exports["."].(map[string]any).
Preserve the existing assertions’ behavior on valid payloads while reporting
malformed shapes through test failures instead of panics.
- Line 160: Update stale TypeScript path references in the failure messages for
forst/cmd/forst/generate_phase2_test.go lines 160, 128, and 163 to the emitted
JavaScript paths dist/pkg/main.js, pkg/core.js, and dist/main.js respectively.
Update forst/cmd/forst/generate_test.go lines 784-786, 808-810, 836-841, and
879-885 so all referenced module paths use .js instead of .ts, preserving the
existing assertions and message structure.
In `@forst/cmd/forst/generate_phase3_test.go`:
- Around line 267-280: Update the test cleanup around generateCommand to wait
for the watch goroutine by draining done with a timeout before
temporary-directory cleanup proceeds. Preserve the existing stop-hook cleanup
and ensure the wait cannot block indefinitely, including failure paths that call
t.Fatal.
In `@forst/cmd/forst/generate_phase4_test.go`:
- Around line 108-116: The test name
TestGenerate_taggedErrorPropsCannotOverwriteTag claims runtime protection, but
the body only checks emitted source ordering and can match comments. Rename it
to describe the emission-order assertion, or extend the existing Node smoke test
to construct an error with a _tag prop and verify the generated tag remains
unchanged.
- Line 241: Update the smoke-test script paths in the relevant phase-4 tests
around the existing filepath.Join calls so the .mjs files are created outside
dist, using the project root or a separate temporary directory. Adjust the
import or execution paths accordingly while preserving the tests’ behavior and
preventing generated-tree assertions from seeing the scripts.
In `@forst/cmd/forst/generate_phase5_test.go`:
- Around line 173-193: Rename TestGenerate_acceptance_scopedPackageOverride to
precisely describe that it verifies input pass-through, preserving its existing
assertions. Add a separate acceptance test covering a partial packages.main
override that omits another function, and assert invocation of the omitted
function throws InvokeRejected.
- Around line 407-431: Refactor
TestGenerate_acceptance_testdataExampleRunsWithoutHTTP to read
scoped-override.mjs from testdata/testing and delegate execution to
runPhase5NodeAcceptance, removing the duplicated Node lookup, temporary
directory, generation, script writing, and command execution setup. Preserve the
test’s existing acceptance-script behavior while reusing the helper.
- Around line 91-93: Guard the ifaceStart and fnStart results in the test around
the strings.Index calls before slicing got. If either marker is absent (-1),
fail the test with a clear assertion instead of constructing
got[ifaceStart:fnStart], matching the defensive behavior in the equivalent
transformer test.
In `@forst/cmd/forst/generate_phase6_test.go`:
- Around line 251-348: Extract the repeated Promise-mode setup into a
generatePromiseProject helper near generateEffectProject, using t.Helper(),
writing the minimal Forst file and link-never configuration, running
generateCommand, and returning defaultClientDistDir. Update
TestGenerate_effectMode_coreModuleIsByteIdenticalToPromiseMode,
TestGenerate_effectMode_transportAndErrorsAreByteIdenticalToPromiseMode, and
TestGenerate_effectMode_onlyPkgModulesDifferFromPromiseMode to use the helper
and its returned directory.
- Around line 84-86: Remove the empty strings.Contains conditional in the Echo
declaration test; retain the existing assertion that follows to validate the
declaration returns an Effect.
- Around line 377-383: Update
TestGenerate_effectMode_coreModulesContainNoEffectImport to assert that
core/main.js does not contain the generated effect import form, matching the
import-specific check used by TestGenerate_promiseMode_emitsNoEffectImport,
rather than rejecting every occurrence of the substring “effect”.
In `@forst/cmd/forst/generate_test.go`:
- Around line 575-585: The test TestGenerateCommand_mkdirSrcFails should be
renamed to describe the dist directory creation failure, and its assertion
should require the exact error message for the directory the generator creates
first. Remove the OR-based acceptance of either “dist/core directory” or
“dist/pkg directory” while preserving the existing failure setup.
In `@forst/cmd/forst/generate_tsc_examples_test.go`:
- Around line 131-145: Handle the error returned by filepath.WalkDir in the
generate TypeScript examples test instead of assigning it to _. Propagate or
fail the test with that walk error before asserting on combined, while
preserving the existing file-reading and aggregation behavior in the WalkDir
callback.
In `@forst/cmd/forst/generate_tsc_test.go`:
- Around line 19-21: Replace the duplicated os.MkdirAll calls in the test setup
with the existing ensureNodeModulesDir helper from the same package, including
the additional occurrence noted in the comment. Preserve fatal error handling
for any helper failure.
- Around line 83-89: Update the core module read/assertion in the test so any
os.ReadFile error for corePath immediately fails the test, rather than skipping
validation. Keep the existing invoke-helper content checks for successful reads
and report the read error with corePath.
In `@forst/cmd/forst/generate_watch.go`:
- Around line 42-56: Update the signal-handling branch in watchGenerate to
ensure signal.Notify registration is stopped when the function returns,
including early error returns from watchPackageRootFn. Apply function-scope
cleanup for sigCh while preserving the existing stop-channel closure and custom
generateWatchStopHook path.
- Around line 27-71: Add a co-located generate_watch_test.go covering
watchGenerate’s debounced regeneration callback and the runGenerateOnce failure
path that logs “generate failed during watch; keeping previous output.” Reuse
existing test seams and hooks such as watchPackageRootFn, generateWatchStopHook,
and runGenerateOnce, and keep the existing generate_watch.go behavior unchanged.
In `@forst/cmd/forst/generate.go`:
- Around line 317-356: Update the pkgJS and pkgDTS writeGeneratedFile error
branches in the client output loop to return the encountered error instead of
logging and continuing, matching the existing error propagation behavior for
other writes. Ensure failed package generation prevents successful completion
and avoids retaining the package in activePackages.
In `@forst/cmd/forst/testdata/effect/consumer.ts`:
- Around line 33-42: Update the mocked ForstTransport.client.invokeFunction
implementation in the transportMock fixture to declare the fourth options
parameter after the optional arguments parameter, preserving the existing return
behavior. Use the appropriate InvokeCallOptions type or inferred signature so
this fixture validates the generated transport argument order and
interruption-signal contract.
In `@forst/cmd/forst/testdata/effect/runtime.mjs`:
- Around line 58-71: Extend the runtime fixture with a transport-rejection case
by configuring or mocking invokeFunction to reject, then run the resulting
Effect and assert the failure value has the InvokeFailure tag. Place this
alongside the existing unimplemented, interruption, and value-handler cases,
preserving their assertions and using the existing runtime test utilities.
In `@forst/internal/codegen/layout/layout.go`:
- Around line 38-44: Replace the exported mutable ReservedDotForstEntries slice
with an unexported backing collection and add an IsReservedDotForstEntry lookup
function. Update all callers and tests, including layout_test.go, to use the
function rather than accessing or reconstructing the list, while preserving the
existing reserved-name membership behavior.
In `@forst/internal/ftconfig/generate_test.go`:
- Around line 268-283: Add coverage in the generate configuration tests for the
Effect-enabled path: verify ReservedSubpaths returns both the effect and testing
owners when Generate.Effect is true, and verify Validate rejects a configuration
combining Effect true with TestingSubpath set to "effect".
- Around line 237-257: Split the always-link assertion out of
TestShouldLink_autoLinksOnlyWhenEphemeral into its own precisely named test, or
isolate it in a clearly named t.Run subtest. Keep the existing auto/ephemeral
and auto/non-ephemeral scenarios under the auto behavior test, and ensure each
scenario reports an accurate name on failure.
In `@forst/internal/ftconfig/generate.go`:
- Around line 101-114: Update Validate to reject configurations where
TestingSubpath resolves to "effect" while Effect is enabled, before
ReservedSubpaths can overwrite the testing reservation. Preserve existing
validation behavior and report the conflicting testingSubpath/effect combination
as invalid.
In `@forst/internal/transformer/ts/emit_effect_test.go`:
- Around line 22-36: Add a sibling test for EmitIndexEffectDTS that invokes it
with representative modules and asserts the output includes ForstClientLive,
layerForstClient, makeForstClientRuntime, and the type-only
ForstInvokeClientConfig import from "./transport.js".
In `@forst/internal/transformer/ts/emit_effect.go`:
- Around line 303-327: Make the generated Effect index declarations
self-contained by updating EmitIndexEffectDTS in
forst/internal/transformer/ts/emit_effect.go:303-327 to import
ForstInvokeClientConfig as a type from "./transport.js", matching
EmitEffectSupportDTS. Add
TestEmitIndexEffectDTS_importsReferencedTransportConfigType in
forst/internal/transformer/ts/emit_effect_test.go:22-36 to verify every
referenced type is imported. Add a second tsc invocation in
forst/cmd/forst/generate_effect_e2e_test.go:47-58 without skipLibCheck, scoped
to .forst/client/dist/**/*.d.ts, so generated declaration errors are detected.
- Around line 351-366: Update generated layerForstTest to honor
overrides.transport by creating and using a ForstTransport layer from the
supplied transport; preserve package overrides and use each service’s
DefaultWithoutDependencies when no package handler override exists. Keep the
declared transport field in ForstTestOverrides consistent with this runtime
behavior, and add coverage proving a transport-only override reaches the
supplied invokeFunction.
- Around line 184-199: Extend service-class validation to detect when pkg is a
valid TypeScript identifier and equals ServiceClassName(pkg), rejecting
generation before DTS/ESM emission. Preserve existing validation for other
package and class-name cases, and ensure the failure occurs before emitting
conflicting declarations.
- Line 360: Update the emission around mockPackageLayer to access packages using
bracket notation with a quoted package key instead of packages.%s. Apply the
same quoted bracket-key form to the corresponding DTS output so package names
containing spaces, digits, @, or other non-identifier characters remain valid.
In `@forst/internal/transformer/ts/emit_esm_test.go`:
- Around line 390-397: Rename TestEmitCoreESM_identicalWithoutRuntimeModeArg to
TestEmitCoreESM_isByteStableForSameModuleEmit so the test name precisely
reflects its byte-stability assertion for identical ModuleEmit inputs; leave the
test implementation unchanged.
In `@forst/internal/transformer/ts/errors.go`:
- Around line 174-181: Remove the redundant i == 0 conditional in the
InvokeFailure union generation loop and assign the " | " prefix directly before
writing each ErrorCatalog entry.
In `@forst/internal/transformer/ts/output.go`:
- Around line 91-104: The generated module naming is inconsistent when
PackageName differs from SourceFileStem. In
forst/internal/transformer/ts/output.go lines 91-104, use SourceFileStem first
with PackageName as fallback for the shared package/core module identifier; in
forst/internal/transformer/ts/client.go lines 135-140, derive the public-module
import using that same rule; in forst/internal/transformer/ts/client_gen_test.go
lines 44-64, add coverage for PackageName "main" and SourceFileStem "api",
asserting ../core/api.js and ./pkg/api.js.
In `@forst/internal/transformer/ts/reserved_test.go`:
- Around line 66-78: Update TestServiceClassName_pascalCase to use an ordered
slice of named test cases instead of a map, then wrap each case assertion in
t.Run using its case name. Preserve all existing inputs, expected outputs, and
failure validation.
In `@forst/internal/transformer/ts/reserved.go`:
- Around line 36-48: Update the reserved-package collision handling in the loop
over names to include the fetched reason value in the returned fmt.Errorf
message, and remove the unused `_ = reason` discard. Preserve the existing
collision details while making the error identify why the subpath is reserved.
In `@forst/internal/transformer/ts/runtime_test.go`:
- Around line 23-30: Refactor TestClientRuntime_String into a named table-driven
test containing each ClientRuntime value and its expected string, then iterate
over the cases with t.Run using the case names and assert the result within each
subtest.
In `@forst/internal/transformer/ts/testing_emit_test.go`:
- Around line 65-72: Rename
TestEmitTestingESM_handlersTypeNameCapitalizesPackage to precisely describe
handlersTypeName capitalization, then convert it to a table-driven test with
t.Run subtests for each input. Retain the bcrypt and main expectations and add
coverage for the empty-string input, asserting the behavior implemented by
handlersTypeName.
In `@forst/internal/transformer/ts/testing_emit.go`:
- Around line 195-197: Update the name filtering condition in EmitTestingDTS to
reject only the exact global names Promise and Async, not names that merely use
those prefixes. Preserve the existing punctuation filtering and ensure
user-defined names such as AsyncJobRequest and PromiseSettings remain eligible
for the ./types.js import.
- Around line 247-262: Update mergeOverrides so inherited
parentTransport.invokeFunction and invokeStream methods are not copied into the
merged transport; keep transport overrides local to the current scope and let
createScopeTransport use its existing parentTransport fallback after checking
inner package handlers. Add a nested-scope test covering an outer transport
override with an inner packages.main.Echo override, verifying the inner handler
wins.
- Around line 25-31: Extend the reserved-identifier validation used by
ValidateReservedSubpaths to reject all names exported by dist/testing.js:
AsyncLocalStorageCtor, scopeStorage, fallbackScope, getActiveScope,
mergePackageMaps, mergeOverrides, createScopeTransport, withForstTestScope,
InvokeRejected, and setActiveTestTransportResolver. Ensure the import generation
loop in testing_emit.go cannot emit package imports that collide with these
testing-module identifiers.
- Around line 203-224: Add a one-time warning at the start of withForstTestScope
whenever scopeStorage is unavailable and the shared fallbackScope path is used.
Track whether the warning has already been emitted so repeated calls do not log
again, while preserving the existing fallback scope behavior.
In `@forst/internal/transformer/ts/transport_test.go`:
- Around line 161-172: Update the import validation loop in the transport test
so brace imports are not automatically exempted; directly validate every
import’s module specifier and allow only ./errors.js, while preserving support
for its multi-line import form. Ensure imports such as `import { Effect } from
"effect";` are rejected independently of the banned-substring checks.
In `@forst/internal/transformer/ts/transport.go`:
- Around line 402-431: Add bounded backoff between retry attempts in
invokeFunction after incrementing attempt and before the next loop iteration.
Apply the delay only when shouldRetryInvokeFailure(err) permits another retry,
and cap the delay at a defined maximum while increasing it across attempts to
avoid back-to-back requests during outages.
- Around line 712-718: Update the invokeStreamBody async generator to track
whether the reader reached normal completion, and in its finally block call
reader.cancel() when it did not. Keep releasing the reader lock afterward,
including safe handling of cancel and release errors, so early break, return, or
thrown consumer errors close the response body.
In `@packages/sidecar/README.md`:
- Line 267: Update the remaining legacy client/output references: in
packages/sidecar/README.md lines 267-267, describe generation output as
.forst/client or the configured output path; in examples/in/tictactoe/README.md
lines 21-23, replace client/types with `@forst/gen`; in
examples/in/rfc/node-interop/remix-serve/README.md lines 23-26, remove
references to the `@forst/client` dependency from app/lib/forst-client.ts; and at
lines 43-43 remove `@forst/client` from standalone dependency instructions.
---
Outside diff comments:
In `@forst/cmd/forst/generate_test.go`:
- Around line 604-618: Update runGenerateOnce to propagate errors from writing
core module outputs, including dist/core/main.js, instead of only logging and
continuing. Ensure generateCommand receives and returns that error so failed
core generation prevents shipping an incomplete package, while preserving
existing handling for unrelated outputs.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 8f57acc0-52b5-4a41-b61d-7b68ce3e8d47
📒 Files selected for processing (95)
.gitignoreCHANGELOG.mdROADMAP.mdTaskfile.ymldocs/docs.jsondocs/index.mdxdocs/interop/node.mdxdocs/interop/node/call-forst.mdxdocs/interop/node/effect.mdxdocs/interop/node/generate-types.mdxdocs/interop/node/testing.mdxdocs/language/overview.mdxdocs/quickstart.mdxdocs/resources/llms.mdxdocs/resources/roadmap.mdxdocs/snippets/catalog-order.mdxdocs/workflow/cli.mdxdocs/workflow/dev-server.mdxexamples/README.mdexamples/in/forst-generate-ts-examples.jsonexamples/in/rfc/node-interop/remix-serve/.gitignoreexamples/in/rfc/node-interop/remix-serve/README.mdexamples/in/rfc/node-interop/remix-serve/app/routes/_index.tsxexamples/in/rfc/node-interop/remix-serve/e2e/client.test.tsexamples/in/rfc/node-interop/remix-serve/vite.config.tsexamples/in/tictactoe/README.mdexamples/in/tictactoe/ftconfig.jsonexamples/in/tictactoe/tests/tictactoe-game.simulation.test.tsforst/cmd/forst/dev_server_typescript_test.goforst/cmd/forst/generate.goforst/cmd/forst/generate_acceptance_test.goforst/cmd/forst/generate_cross_file_test.goforst/cmd/forst/generate_effect.goforst/cmd/forst/generate_effect_e2e_test.goforst/cmd/forst/generate_effect_test.goforst/cmd/forst/generate_guard.goforst/cmd/forst/generate_guard_test.goforst/cmd/forst/generate_link.goforst/cmd/forst/generate_link_test.goforst/cmd/forst/generate_omission.goforst/cmd/forst/generate_omission_test.goforst/cmd/forst/generate_phase1_test.goforst/cmd/forst/generate_phase2_test.goforst/cmd/forst/generate_phase3_test.goforst/cmd/forst/generate_phase4_test.goforst/cmd/forst/generate_phase5_test.goforst/cmd/forst/generate_phase6_test.goforst/cmd/forst/generate_test.goforst/cmd/forst/generate_tsc_examples_test.goforst/cmd/forst/generate_tsc_test.goforst/cmd/forst/generate_watch.goforst/cmd/forst/generate_write.goforst/cmd/forst/generate_write_test.goforst/cmd/forst/testdata/effect/consumer.tsforst/cmd/forst/testdata/effect/runtime.mjsforst/cmd/forst/testdata/testing/scoped-override.mjsforst/internal/codegen/layout/layout.goforst/internal/codegen/layout/layout_test.goforst/internal/ftconfig/ftconfig.goforst/internal/ftconfig/generate.goforst/internal/ftconfig/generate_test.goforst/internal/transformer/ts/client.goforst/internal/transformer/ts/client_gen_test.goforst/internal/transformer/ts/direct_invoke_exports.goforst/internal/transformer/ts/direct_invoke_exports_test.goforst/internal/transformer/ts/emit_effect.goforst/internal/transformer/ts/emit_effect_test.goforst/internal/transformer/ts/emit_esm.goforst/internal/transformer/ts/emit_esm_test.goforst/internal/transformer/ts/errors.goforst/internal/transformer/ts/errors_test.goforst/internal/transformer/ts/function.goforst/internal/transformer/ts/function_test.goforst/internal/transformer/ts/merge.goforst/internal/transformer/ts/merge_test.goforst/internal/transformer/ts/output.goforst/internal/transformer/ts/output_test.goforst/internal/transformer/ts/providers_export.goforst/internal/transformer/ts/providers_export_test.goforst/internal/transformer/ts/reserved.goforst/internal/transformer/ts/reserved_test.goforst/internal/transformer/ts/runtime.goforst/internal/transformer/ts/runtime_test.goforst/internal/transformer/ts/testdata/ndjson/stream.ndjsonforst/internal/transformer/ts/testing_emit.goforst/internal/transformer/ts/testing_emit_test.goforst/internal/transformer/ts/transformer.goforst/internal/transformer/ts/transformer_test.goforst/internal/transformer/ts/transport.goforst/internal/transformer/ts/transport_test.gopackages/sidecar/README.mdpackages/sidecar/src/config-merge.test.tspackages/sidecar/src/config-merge.tspackages/sidecar/src/types.tsscripts/remix-serve-standalone-e2e.sh
💤 Files with no reviewable changes (3)
- examples/in/rfc/node-interop/remix-serve/.gitignore
- .gitignore
- scripts/remix-serve-standalone-e2e.sh
Comment on lines
+128
to
+139
| ```typescript | ||
| import { Effect, Layer } from "effect"; | ||
| import { Bcrypt } from "@forst/gen/bcrypt"; | ||
|
|
||
| const BcryptTest = Layer.mock(Bcrypt, { | ||
| ComparePassword: () => Effect.succeed({ valid: true }), | ||
| }); | ||
|
|
||
| await Effect.runPromise( | ||
| ComparePassword(input).pipe(Effect.provide(BcryptTest)) | ||
| ); | ||
| ``` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the missing ComparePassword import.
The snippet imports only Bcrypt but calls ComparePassword(input) at line 137. Copying this block produces an unresolved identifier. The other snippets on this page (lines 102 and 148) import the function explicitly.
📝 Proposed fix
import { Effect, Layer } from "effect";
import { Bcrypt } from "`@forst/gen/bcrypt`";
+import { ComparePassword } from "`@forst/gen/bcrypt`";📝 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
| ```typescript | |
| import { Effect, Layer } from "effect"; | |
| import { Bcrypt } from "@forst/gen/bcrypt"; | |
| const BcryptTest = Layer.mock(Bcrypt, { | |
| ComparePassword: () => Effect.succeed({ valid: true }), | |
| }); | |
| await Effect.runPromise( | |
| ComparePassword(input).pipe(Effect.provide(BcryptTest)) | |
| ); | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interop/node/effect.mdx` around lines 128 - 139, Update the code
snippet’s imports to include the ComparePassword function used by the
Effect.runPromise example, alongside the existing Bcrypt import, so the snippet
has no unresolved identifiers.
|
|
||
| <CatalogOrder /> | ||
|
|
||
| Only **exported** functions (capitalized names) appear in the client. Provider gated functions are omitted. See [Caveats](#caveats). |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate "provider-gated".
"Provider gated" is a compound modifier before a noun. Use "provider-gated". call-forst.mdx line 307 already writes "Provider-dependent functions" with a hyphen.
✏️ Proposed fix
-Only **exported** functions (capitalized names) appear in the client. Provider gated functions are omitted. See [Caveats](`#caveats`).
+Only **exported** functions (capitalized names) appear in the client. Provider-gated functions are omitted. See [Caveats](`#caveats`).-### Provider gated exports
+### Provider-gated exportsAlso applies to: 123-123
🧰 Tools
🪛 LanguageTool
[grammar] ~75-~75: Use a hyphen to join words.
Context: ...ed names) appear in the client. Provider gated functions are omitted. See [Caveat...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interop/node/generate-types.mdx` at line 75, Update the sentence in the
documentation to hyphenate the compound modifier as “provider-gated functions,”
preserving the rest of the wording and capitalization.
Source: Linters/SAST tools
Comment on lines
+247
to
+262
| function createScopeTransport(scope, parentTransport) { | ||
| return { | ||
| async invokeFunction(packageName, functionName, args = [], options) { | ||
| if (typeof scope.transport?.invokeFunction === "function") { | ||
| return scope.transport.invokeFunction( | ||
| packageName, | ||
| functionName, | ||
| args, | ||
| options | ||
| ); | ||
| } | ||
| const handler = scope.packages?.[packageName]?.[functionName]; | ||
| if (typeof handler === "function") { | ||
| const result = await handler(...args, options); | ||
| return { success: true, result }; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Inherited parent transport suppresses inner package handlers.
mergeOverrides copies parentTransport.invokeFunction and parentTransport.invokeStream into merged.transport (lines 239-243). createScopeTransport then receives that merged object as scope. The check at line 250 sees the inherited parent function and returns early, so the inner scope's package handler at line 258 is never reached.
Reproduction: an outer scope sets transport.invokeFunction, and an inner scope sets packages.main.Echo. The inner handler is ignored. This contradicts the documented rule at lines 314-317, "the innermost override wins".
The parentTransport argument already provides the fall-through to the outer transport at lines 263-270. So mergeOverrides must not copy transport methods from the parent.
Add a nested-scope test that mixes an outer transport override with an inner package override.
🐛 Proposed fix to keep the transport override scope-local
function mergeOverrides(parent, child) {
- const parentTransport = parent?.transport ?? {};
const childTransport = child?.transport ?? {};
return {
packages: mergePackageMaps(parent?.packages, child?.packages),
transport: {
- invokeFunction:
- childTransport.invokeFunction ?? parentTransport.invokeFunction,
- invokeStream: childTransport.invokeStream ?? parentTransport.invokeStream,
+ invokeFunction: childTransport.invokeFunction,
+ invokeStream: childTransport.invokeStream,
},
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/testing_emit.go` around lines 247 - 262, Update
mergeOverrides so inherited parentTransport.invokeFunction and invokeStream
methods are not copied into the merged transport; keep transport overrides local
to the current scope and let createScopeTransport use its existing
parentTransport fallback after checking inner package handlers. Add a
nested-scope test covering an outer transport override with an inner
packages.main.Echo override, verifying the inner handler wins.
Comment on lines
+161
to
+172
| for _, line := range strings.Split(src, "\n") { | ||
| trimmed := strings.TrimSpace(line) | ||
| if !strings.HasPrefix(trimmed, "import ") { | ||
| continue | ||
| } | ||
| if !strings.Contains(trimmed, `from "./errors.js"`) && !strings.HasPrefix(trimmed, "import {") { | ||
| // Multi-line import from errors.js is OK; any other module is not. | ||
| if strings.Contains(trimmed, " from ") && !strings.Contains(trimmed, `"./errors.js"`) { | ||
| t.Fatalf("transport may only import ./errors.js, got:\n%s", line) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The import scan lets any brace import through.
The outer condition requires both !strings.Contains(trimmed, "from \"./errors.js\"") and !strings.HasPrefix(trimmed, "import {"). Every brace import therefore skips the inner check. A line such as import { Effect } from "effect"; passes this loop. Only the banned-substring list above catches it today. Invert the check so the loop asserts the module specifier directly.
💚 Proposed fix
for _, line := range strings.Split(src, "\n") {
trimmed := strings.TrimSpace(line)
- if !strings.HasPrefix(trimmed, "import ") {
+ if !strings.HasPrefix(trimmed, "import ") || !strings.Contains(trimmed, " from ") {
continue
}
- if !strings.Contains(trimmed, `from "./errors.js"`) && !strings.HasPrefix(trimmed, "import {") {
- // Multi-line import from errors.js is OK; any other module is not.
- if strings.Contains(trimmed, " from ") && !strings.Contains(trimmed, `"./errors.js"`) {
- t.Fatalf("transport may only import ./errors.js, got:\n%s", line)
- }
+ if !strings.Contains(trimmed, `"./errors.js"`) {
+ t.Fatalf("transport may only import ./errors.js, got:\n%s", line)
}
}📝 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
| for _, line := range strings.Split(src, "\n") { | |
| trimmed := strings.TrimSpace(line) | |
| if !strings.HasPrefix(trimmed, "import ") { | |
| continue | |
| } | |
| if !strings.Contains(trimmed, `from "./errors.js"`) && !strings.HasPrefix(trimmed, "import {") { | |
| // Multi-line import from errors.js is OK; any other module is not. | |
| if strings.Contains(trimmed, " from ") && !strings.Contains(trimmed, `"./errors.js"`) { | |
| t.Fatalf("transport may only import ./errors.js, got:\n%s", line) | |
| } | |
| } | |
| } | |
| for _, line := range strings.Split(src, "\n") { | |
| trimmed := strings.TrimSpace(line) | |
| if !strings.HasPrefix(trimmed, "import ") || !strings.Contains(trimmed, " from ") { | |
| continue | |
| } | |
| if !strings.Contains(trimmed, `"./errors.js"`) { | |
| t.Fatalf("transport may only import ./errors.js, got:\n%s", line) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/transport_test.go` around lines 161 - 172,
Update the import validation loop in the transport test so brace imports are not
automatically exempted; directly validate every import’s module specifier and
allow only ./errors.js, while preserving support for its multi-line import form.
Ensure imports such as `import { Effect } from "effect";` are rejected
independently of the banned-substring checks.
Comment on lines
+712
to
+718
| } finally { | ||
| try { | ||
| reader.releaseLock(); | ||
| } catch { | ||
| // ignore release errors after cancel | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The generated stream reader leaks the response body when the consumer exits early.
invokeStreamBody is an async generator. If a caller does break or return inside for await, the generator runs the finally block at line 712. That block only calls reader.releaseLock(). It never calls reader.cancel(), so the underlying HTTP response body stays open and the connection is not released. The same applies when the caller throws. Cancel the reader when the stream did not reach normal completion.
🔒 Proposed fix: cancel the reader on early exit
try {
while (true) {
+ completed = false;Track completion and cancel in finally:
let rowIndex = 0;
+ let completed = false;- buffer += decoder.decode();
+ buffer += decoder.decode();
+ completed = true; } finally {
+ if (!completed) {
+ try {
+ await reader.cancel();
+ } catch {
+ // ignore cancel errors
+ }
+ }
try {
reader.releaseLock();
} catch {
// ignore release errors after cancel
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/transport.go` around lines 712 - 718, Update
the invokeStreamBody async generator to track whether the reader reached normal
completion, and in its finally block call reader.cancel() when it did not. Keep
releasing the reader lock afterward, including safe handling of cancel and
release errors, so early break, return, or thrown consumer errors close the
response body.
| **`generateTypes()`** runs `forst generate` with the same effective project root as `forst dev -root` (`rootDir` / `forstDir`). If **`configPath`** is set, `-config` is passed so include/exclude matches `forst dev`. Output goes under `generated/`—no HTTP server required. In monorepos, pair with **connect** mode: one task runs `forst dev`, another runs codegen. | ||
|
|
||
| **`watchGenerate`:** when `true`, runs `forst generate` after debounced hot-reload restarts when `.ft` files change (**spawn** mode only; same roots as `generateTypes`). | ||
| **`watchGenerate`:** when `true`, runs `forst generate` after debounced hot-reload restarts when `.ft` files change (**spawn** mode only; same roots as `generateTypes`). Defaults to `true` in `development` mode. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update all remaining legacy client and output references.
The documentation still points users to generated/, client/types, or @forst/client, while this PR requires .forst/client and @forst/gen.
packages/sidecar/README.md#L267-L267: Replace thegenerated/output description at Line 265 with.forst/clientor the configured output path.examples/in/tictactoe/README.md#L21-L23: Replace theclient/typesimport described at Line 46 with@forst/gen.examples/in/rfc/node-interop/remix-serve/README.md#L23-L26: Update theapp/lib/forst-client.tsdescription so it does not name the removed@forst/clientdependency.examples/in/rfc/node-interop/remix-serve/README.md#L43-L43: Remove@forst/clientfrom the standalone dependency instructions at Line 46.
📍 Affects 3 files
packages/sidecar/README.md#L267-L267(this comment)examples/in/tictactoe/README.md#L21-L23examples/in/rfc/node-interop/remix-serve/README.md#L23-L26examples/in/rfc/node-interop/remix-serve/README.md#L43-L43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sidecar/README.md` at line 267, Update the remaining legacy
client/output references: in packages/sidecar/README.md lines 267-267, describe
generation output as .forst/client or the configured output path; in
examples/in/tictactoe/README.md lines 21-23, replace client/types with
`@forst/gen`; in examples/in/rfc/node-interop/remix-serve/README.md lines 23-26,
remove references to the `@forst/client` dependency from app/lib/forst-client.ts;
and at lines 43-43 remove `@forst/client` from standalone dependency instructions.
Comment on lines
+729
to
+731
| } else if !strings.Contains(string(ignore), ".forst/") && !strings.Contains(string(ignore), ".forst") { | ||
| t.Fatal("repo .gitignore must cover .forst/") | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the redundant .gitignore condition.
strings.Contains(ignore, ".forst/") is subsumed by strings.Contains(ignore, ".forst"). The combined condition therefore only tests for the substring .forst, which also matches unrelated entries such as .forstcache. Check for the directory pattern only.
🐛 Proposed fix
- } else if !strings.Contains(string(ignore), ".forst/") && !strings.Contains(string(ignore), ".forst") {
+ } else if !strings.Contains(string(ignore), ".forst/") {
t.Fatal("repo .gitignore must cover .forst/")
}📝 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
| } else if !strings.Contains(string(ignore), ".forst/") && !strings.Contains(string(ignore), ".forst") { | |
| t.Fatal("repo .gitignore must cover .forst/") | |
| } | |
| } else if !strings.Contains(string(ignore), ".forst/") { | |
| t.Fatal("repo .gitignore must cover .forst/") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_acceptance_test.go` around lines 729 - 731, Update
the .gitignore validation in the acceptance test to check only for the directory
pattern ".forst/". Remove the redundant strings.Contains check for ".forst"
while preserving the existing failure behavior when the directory pattern is
absent.
Comment on lines
+19
to
+25
| func defaultClientOutDir(boundary string) string { | ||
| return filepath.Join(boundary, ".forst", "client") | ||
| } | ||
|
|
||
| func defaultClientDistDir(boundary string) string { | ||
| return filepath.Join(defaultClientOutDir(boundary), "dist") | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The default output path .forst/client is hardcoded in three places. ftconfig.EffectiveGenerateConfig owns this default. Each duplicate breaks independently if the default changes, and the tsconfig duplicates then silently include zero declaration files, so tsc reports success on an empty program.
forst/cmd/forst/generate_phase1_test.go#L19-L25: derivedefaultClientOutDirfromftconfig.EffectiveGenerateConfig(nil, "").OutDirand keep one test that pins the literal.forst/client.forst/cmd/forst/generate_acceptance_test.go#L228-L232: build the**/*.d.tsinclude pattern fromdefaultClientDistDir(projectRoot)relative toprojectRoot.forst/cmd/forst/generate_tsc_test.go#L163-L167: build the**/*.d.tsinclude pattern fromdefaultClientDistDir(projectRoot)relative toprojectRoot.
📍 Affects 3 files
forst/cmd/forst/generate_phase1_test.go#L19-L25(this comment)forst/cmd/forst/generate_acceptance_test.go#L228-L232forst/cmd/forst/generate_tsc_test.go#L163-L167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_phase1_test.go` around lines 19 - 25, Centralize the
client output default across all three tests: in
forst/cmd/forst/generate_phase1_test.go:19-25, derive defaultClientOutDir from
ftconfig.EffectiveGenerateConfig(nil, "").OutDir and retain one test pinning
.forst/client; in forst/cmd/forst/generate_acceptance_test.go:228-232 and
forst/cmd/forst/generate_tsc_test.go:163-167, construct the **/*.d.ts include
pattern from defaultClientDistDir(projectRoot) relative to projectRoot.
Comment on lines
+33
to
+42
| const transportMock = Layer.mock(ForstTransport, { | ||
| client: { | ||
| invokeFunction: async <T>(_p: string, _f: string, _a?: unknown[]) => | ||
| ({ | ||
| success: true as const, | ||
| result: { echo: "ok", timestamp: 1 } as T, | ||
| }), | ||
| async *invokeStream() {}, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the options parameter in the mocked invokeFunction.
The mock declares three parameters. The generated Effect service calls core.<fn>(input, withTransport(client, options, signal)), so the real transport receives a fourth options argument that carries the interruption signal. TypeScript accepts the narrower implementation signature, so this fixture compiles without checking that argument.
This fixture is the type-level guard for the transport contract. Add the options parameter so a change to InvokeCallOptions or to the argument order fails the build here.
Note that forst/cmd/forst/testdata/effect/runtime.mjs Line 28 already relies on the fourth parameter.
♻️ Proposed change
const transportMock = Layer.mock(ForstTransport, {
client: {
- invokeFunction: async <T>(_p: string, _f: string, _a?: unknown[]) =>
+ invokeFunction: async <T>(
+ _p: string,
+ _f: string,
+ _a?: unknown[],
+ _o?: import("`@forst/gen/transport`").InvokeCallOptions
+ ) =>
({
success: true as const,
result: { echo: "ok", timestamp: 1 } as T,
}),
async *invokeStream() {},
},
});📝 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
| const transportMock = Layer.mock(ForstTransport, { | |
| client: { | |
| invokeFunction: async <T>(_p: string, _f: string, _a?: unknown[]) => | |
| ({ | |
| success: true as const, | |
| result: { echo: "ok", timestamp: 1 } as T, | |
| }), | |
| async *invokeStream() {}, | |
| }, | |
| }); | |
| const transportMock = Layer.mock(ForstTransport, { | |
| client: { | |
| invokeFunction: async <T>( | |
| _p: string, | |
| _f: string, | |
| _a?: unknown[], | |
| _o?: import("`@forst/gen/transport`").InvokeCallOptions | |
| ) => | |
| ({ | |
| success: true as const, | |
| result: { echo: "ok", timestamp: 1 } as T, | |
| }), | |
| async *invokeStream() {}, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/testdata/effect/consumer.ts` around lines 33 - 42, Update the
mocked ForstTransport.client.invokeFunction implementation in the transportMock
fixture to declare the fourth options parameter after the optional arguments
parameter, preserving the existing return behavior. Use the appropriate
InvokeCallOptions type or inferred signature so this fixture validates the
generated transport argument order and interruption-signal contract.
Comment on lines
+184
to
+199
| b.WriteString(fmt.Sprintf("export declare const %s: (client: ForstInvokeClient) => {\n", pkg)) | ||
| for _, fn := range m.Functions { | ||
| params := emitParamListDTS(fn.Parameters, true) | ||
| b.WriteString(fmt.Sprintf(" %s: (%s) => Promise<%s>;\n", fn.Name, params, fn.ReturnType)) | ||
| if fn.StreamingRowType != "" { | ||
| b.WriteString(fmt.Sprintf( | ||
| " %sStream: (%s) => AsyncGenerator<StreamingResult & { data?: %s }, void, undefined>;\n", | ||
| fn.Name, params, fn.StreamingRowType, | ||
| )) | ||
| } | ||
| } | ||
| b.WriteString("};\n\n") | ||
|
|
||
| // Ambient Tag form avoids TS1005 from type args inside Effect.Service() value args. | ||
| // Runtime JS still emits Effect.Service with accessors and dependencies. | ||
| b.WriteString(fmt.Sprintf("export declare class %s extends Context.Tag(%q)<\n", className, tag)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect ServiceClassName and any package-name validation that would prevent pkg == ServiceClassName(pkg).
set -euo pipefail
rg -nP --type=go -C6 'func ServiceClassName\s*\(' forst/internal/transformer/ts
rg -nP --type=go -C4 'ServiceClassName\(' forst/internal/transformer/ts forst/cmd/forstRepository: forst-lang/forst
Length of output: 9510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reserved.go relevant sections =="
sed -n '1,140p' forst/internal/transformer/ts/reserved.go | cat -n
echo
echo "== reserved_test.go collision tests around ServiceClassName and ValidateServiceClassNames =="
sed -n '1,140p' forst/internal/transformer/ts/reserved_test.go | cat -n
echo
echo "== emit_effect.go relevant ESM/DTS declarations =="
sed -n '88,225p' forst/internal/transformer/ts/emit_effect.go | cat -nRepository: forst-lang/forst
Length of output: 14994
Reject package names that collide with their service class.
ServiceClassName("main") returns Main, so a package named main generates both export const Main/export declare const Main and export class Main/export declare class Main. That is a duplicate identifier in the same module. Extend the service-class validation to reject pkg being a valid TypeScript identifier equal to ServiceClassName(pkg), or fail generation before emitting the DTS/ESM.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/emit_effect.go` around lines 184 - 199, Extend
service-class validation to detect when pkg is a valid TypeScript identifier and
equals ServiceClassName(pkg), rejecting generation before DTS/ESM emission.
Preserve existing validation for other package and class-name cases, and ensure
the failure occurs before emitting conflicting declarations.
Comment on lines
+303
to
+327
| func EmitIndexEffectDTS(packages []string) string { | ||
| pkgs := sortDedupeStrings(packages) | ||
| var b strings.Builder | ||
| b.WriteString("\n// --- Effect mode root layers ---\n") | ||
| b.WriteString(`import { Layer, ManagedRuntime } from "effect";` + "\n") | ||
| serviceTypes := make([]string, 0, len(pkgs)) | ||
| for _, pkg := range pkgs { | ||
| className := ServiceClassName(pkg) | ||
| b.WriteString(fmt.Sprintf("import { %s } from \"./pkg/%s.js\";\n", className, pkg)) | ||
| serviceTypes = append(serviceTypes, className) | ||
| } | ||
| b.WriteString("\n") | ||
| req := "never" | ||
| if len(serviceTypes) > 0 { | ||
| req = strings.Join(serviceTypes, " | ") | ||
| } | ||
| b.WriteString(fmt.Sprintf("export declare const ForstClientLive: Layer.Layer<%s>;\n\n", req)) | ||
| b.WriteString("export declare const layerForstClient: (\n") | ||
| b.WriteString(" config?: ForstInvokeClientConfig\n") | ||
| b.WriteString(fmt.Sprintf(") => Layer.Layer<%s>;\n\n", req)) | ||
| b.WriteString("export declare const makeForstClientRuntime: (\n") | ||
| b.WriteString(" config?: ForstInvokeClientConfig\n") | ||
| b.WriteString(fmt.Sprintf(") => ManagedRuntime.ManagedRuntime<%s, never>;\n", req)) | ||
| return b.String() | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The Effect index declaration fragment is not self-contained, and no test can detect it. EmitIndexEffectDTS references ForstInvokeClientConfig but imports only Layer and ManagedRuntime. The unit tests skip that function, and the end-to-end tsc run sets skipLibCheck: true, which suppresses errors inside generated .d.ts files.
forst/internal/transformer/ts/emit_effect.go#L303-L327: emitimport type { ForstInvokeClientConfig } from "./transport.js";in the fragment, matchingEmitEffectSupportDTSat Line 68.forst/internal/transformer/ts/emit_effect_test.go#L22-L36: addTestEmitIndexEffectDTS_importsReferencedTransportConfigTypethat asserts the fragment imports every type name it references.forst/cmd/forst/generate_effect_e2e_test.go#L47-L58: add a secondtscrun withoutskipLibCheckthat includes only.forst/client/dist/**/*.d.ts, so errors inside the generated declarations fail the test.
📍 Affects 3 files
forst/internal/transformer/ts/emit_effect.go#L303-L327(this comment)forst/internal/transformer/ts/emit_effect_test.go#L22-L36forst/cmd/forst/generate_effect_e2e_test.go#L47-L58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/emit_effect.go` around lines 303 - 327, Make
the generated Effect index declarations self-contained by updating
EmitIndexEffectDTS in forst/internal/transformer/ts/emit_effect.go:303-327 to
import ForstInvokeClientConfig as a type from "./transport.js", matching
EmitEffectSupportDTS. Add
TestEmitIndexEffectDTS_importsReferencedTransportConfigType in
forst/internal/transformer/ts/emit_effect_test.go:22-36 to verify every
referenced type is imported. Add a second tsc invocation in
forst/cmd/forst/generate_effect_e2e_test.go:47-58 without skipLibCheck, scoped
to .forst/client/dist/**/*.d.ts, so generated declaration errors are detected.
Move dist writes, client package assembly, and SSR module generation out of generate.go into generate_dist.go, generate_client_package.go, and generate_ssr.go. Move ModuleEmitFromOutput into the TypeScript transformer. Rename generate_phase*_test.go files to feature-based names. Replace leftover phase-number comments and test messages with permanent product language. docs(interop): document Promise and Effect modes side by side Rewrite the Node interop docs around VerifyPassword and the auth package subpath. Add Promise and Effect CodeGroups on call-forst, effect, generate-types, and testing pages so both runtimes read as one story. chore(examples): rename multi-package-dev bcrypt package to auth Rename bcrypt.ft to auth.ft and refresh golden Go output and README wording so the example matches the updated documentation.
Rewrite pages around developer goals and centralize generated client setup. Clarify Node workflows, Effect usage, testing, error handling, narrowing, providers, and union type support. Add persistent readability guidance for future documentation.
Explain unions and intersections through practical error handling, value alternatives, and shared field requirements. Add theoretical context and clearer guidance on supported behavior and current limits.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
forst/internal/transformer/ts/output.go (1)
91-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
GeneratePackageModule.
generate_dist.gowrites package modules withEmitPackageESMandEmitPackageDTS;GeneratePackageModulehas no production callers. Remove the method unless a caller is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/internal/transformer/ts/output.go` around lines 91 - 106, Remove the unused TypeScriptOutput.GeneratePackageModule method and its associated implementation, since package modules are generated through EmitPackageESM and EmitPackageDTS. Do not add replacement callers or alter the existing emission flow.docs/interop/node/testing.mdx (1)
109-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse
InvokeRejectedfrom@forst/gen/testing.
@forst/genexports invoke errors from its root, but the testing subpath only re-exportsInvokeRejected;InvokeTimedOutis not available there. Import the error from@forst/genas in the replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/interop/node/testing.mdx` around lines 109 - 124, Update the testing example’s invoke error import to use InvokeRejected from `@forst/gen/testing`, while retaining the existing root import for InvokeTimedOut from `@forst/gen`. Keep the withForstTestScope transport behavior and error construction unchanged.
♻️ Duplicate comments (7)
forst/cmd/forst/generate_effect_mode_test.go (3)
84-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the empty conditional.
The
ifbody contains only a comment. The statement performs no assertion and no side effect. Linters report this as an empty branch. Lines 87-94 already verify that theEchodeclaration returns anEffect.♻️ Proposed fix
- if strings.Contains(got, "): Promise<") && strings.Contains(got, "export declare const Echo") { - // namespace factory may still mention Promise; named export must be Effect - } echoIdx := strings.Index(got, "export declare const Echo:")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_effect_mode_test.go` around lines 84 - 86, Remove the empty if conditional checking Promise and the Echo export from the test, leaving the existing assertions around the Echo declaration unchanged.
377-383: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the import, not the bare word.
The test name states that core modules contain no
effectimport. The assertion rejects any occurrence of the substringeffect. A generated identifier or comment such asSideEffectoreffectivethen fails the test for the wrong reason.Match the import form, as
TestGenerate_promiseMode_emitsNoEffectImportat line 367 already does.♻️ Proposed fix
core := mustRead(t, filepath.Join(dist, "core", "main.js")) - if strings.Contains(core, "effect") { - t.Fatalf("core must not mention effect:\n%s", core) + if strings.Contains(core, `from "effect"`) || strings.Contains(core, "from 'effect'") { + t.Fatalf("core must not import effect:\n%s", core) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_effect_mode_test.go` around lines 377 - 383, Update TestGenerate_effectMode_coreModulesContainNoEffectImport to assert that core does not contain an effect import, matching the import-pattern check used by TestGenerate_promiseMode_emitsNoEffectImport, rather than rejecting every occurrence of the substring “effect”.
251-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the Promise-mode project setup.
TestGenerate_effectMode_coreModuleIsByteIdenticalToPromiseModeat lines 252-259,TestGenerate_effectMode_transportAndErrorsAreByteIdenticalToPromiseModeat lines 277-284, andTestGenerate_effectMode_onlyPkgModulesDifferFromPromiseModeat lines 299-306 repeat the same setup. A helper next togenerateEffectProjectremoves the duplication and keeps both setups symmetric.♻️ Proposed helper
func generatePromiseProject(t *testing.T, dir string) string { t.Helper() writeMainFt(t, dir, generateTestMinimalValidForst) if err := os.WriteFile(filepath.Join(dir, "ftconfig.json"), []byte(`{"generate":{"link":"never"}}`), 0644); err != nil { t.Fatal(err) } if err := generateCommand([]string{dir}); err != nil { t.Fatalf("promise generate: %v", err) } return defaultClientDistDir(dir) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_effect_mode_test.go` around lines 251 - 348, Extract the repeated Promise-mode setup from the three tests into a helper next to generateEffectProject, named generatePromiseProject. Have it mark itself as a helper, create the minimal project and ftconfig.json, run generateCommand with the existing failure message, and return defaultClientDistDir(dir); replace each duplicated setup with a call to this helper while preserving the existing comparisons.forst/cmd/forst/generate_testing_hooks_test.go (3)
91-93: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the index results before slicing.
strings.Indexreturns -1 when the marker is absent. Line 93 then panics with a slice bounds error instead of reporting the missing fragment.💚 Proposed fix to add the guard
ifaceStart := strings.Index(got, "export interface ForstTestOverrides") fnStart := strings.Index(got, "export declare function withForstTestScope") + if ifaceStart < 0 || fnStart < 0 || fnStart < ifaceStart { + t.Fatalf("missing ForstTestOverrides or withForstTestScope declarations:\n%s", got) + } body := got[ifaceStart:fnStart]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_testing_hooks_test.go` around lines 91 - 93, Validate the ifaceStart and fnStart results from strings.Index before slicing got in the test that checks generated TypeScript. If either marker is missing, fail the test with a clear assertion describing the absent fragment; only construct body after both indices are valid.
173-193: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThe package-override test duplicates the function-override test.
TestGenerate_acceptance_scopedPackageOverrideoverrides one function underpackages.main, exactly asTestGenerate_acceptance_scopedFunctionOverridedoes at lines 151-171. The only new behavior is the input pass-through. No test covers a partial package map that omits a function.Rename this test to describe the input pass-through. Add a separate test that omits a function from
packages.mainand asserts the omitted function throwsInvokeRejected.As per coding guidelines: "Ensure presence of precise, reproducing unit or integration tests (preferably unit tests) with precise names describing exactly what's under test".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_testing_hooks_test.go` around lines 173 - 193, Rename TestGenerate_acceptance_scopedPackageOverride to identify the input pass-through behavior it uniquely verifies. Add a separate acceptance test for a partial packages.main override that omits one function, invokes that omitted function, and asserts the failure is InvokeRejected; keep the existing override assertion focused on the overridden function’s result.Source: Coding guidelines
407-431: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReuse
runPhase5NodeAcceptanceinstead of duplicating it.Lines 408-430 repeat every step of
runPhase5NodeAcceptanceat lines 433-454: thenodelookup, the temp directory,writeMainFt,generateCommand, the dist path, the script write, and thenodeexecution. The only difference is the script source.♻️ Proposed fix to remove the duplicated setup
func TestGenerate_acceptance_testdataExampleRunsWithoutHTTP(t *testing.T) { - if _, err := exec.LookPath("node"); err != nil { - t.Skip("node not found") - } - dir := t.TempDir() - writeMainFt(t, dir, generateTestMinimalValidForst) - if err := generateCommand([]string{dir}); err != nil { - t.Fatalf("generateCommand: %v", err) - } - dist := defaultClientDistDir(dir) exampleSrc, err := os.ReadFile(filepath.Join("testdata", "testing", "scoped-override.mjs")) if err != nil { t.Fatal(err) } - script := filepath.Join(dist, "testdata-example.mjs") - if err := os.WriteFile(script, exampleSrc, 0644); err != nil { - t.Fatal(err) - } - cmd := exec.Command("node", script) - cmd.Dir = dist - out, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("testdata example failed: %v\n%s", err, out) - } + runPhase5NodeAcceptance(t, "testdata-example.mjs", string(exampleSrc)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_testing_hooks_test.go` around lines 407 - 431, Refactor TestGenerate_acceptance_testdataExampleRunsWithoutHTTP to reuse runPhase5NodeAcceptance for the shared Node acceptance setup and execution. Pass the existing scoped-override.mjs source through the helper’s script-source parameter or equivalent, removing the duplicated node lookup, temp directory, generation, script writing, and command execution while preserving the test’s behavior.docs/interop/node/effect.mdx (1)
133-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the missing
VerifyPasswordimport.The snippet imports
Authonly, then callsVerifyPassword(input)at line 142. Copying this block produces an unresolved identifier. ThelayerForstTestsnippet at lines 150-165 imports the function explicitly.📝 Proposed fix
import { Effect, Layer } from "effect"; -import { Auth } from "`@forst/gen/auth`"; +import { Auth, VerifyPassword } from "`@forst/gen/auth`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/interop/node/effect.mdx` around lines 133 - 144, Update the TypeScript snippet imports to include the VerifyPassword symbol used by the Effect.runPromise call, while preserving the existing Auth import and AuthTest layer setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/installation.mdx`:
- Around line 52-67: Clarify the installation workflow around the postinstall
script and `@forst/cli`: explicitly state whether production installs must keep
`@forst/cli` in dependencies, or whether it may remain a devDependency when client
generation runs during the build. Keep the dependency declaration and generation
commands consistent with the chosen deployment behavior.
In `@docs/interop/node.mdx`:
- Around line 49-50: Update the interoperability documentation around the `forst
generate` description to state the supported Node.js version range first, then
separately list Express, Remix, and plain Node as supported environments.
Explicitly document the required ESM and web/HTTP APIs for plain Node users.
In `@docs/interop/node/call-javascript.mdx`:
- Around line 163-164: Update the setup documentation sentence near “Forst
waits” to hyphenate the compound modifier as “app-ready signal,” leaving the
surrounding wording unchanged.
- Around line 285-287: Update the prose in the bootstrap mode description to
hyphenate the compound modifier as “Forst-owned” before “script,” leaving the
surrounding documentation unchanged.
In `@docs/interop/node/generate-types.mdx`:
- Around line 29-32: Remove the duplicated `package auth` to `@forst/gen/auth`
row from the package-import mapping table, leaving only one entry for that
mapping.
In `@docs/interop/node/testing.mdx`:
- Around line 65-69: Update the “Connection” row in the testing choices table to
hyphenate “low-level” when describing the invoke client, leaving the rest of the
table unchanged.
In `@docs/language/ensure-and-narrowing.mdx`:
- Around line 120-124: Update the prose in the “Map lookups use Result” section
to hyphenate “Go-style” when it modifies “comma assignment,” while preserving
the surrounding explanation and code syntax unchanged.
In `@docs/language/errors-and-result.mdx`:
- Around line 190-191: Update the “Ensure and narrowing” link in
errors-and-result.mdx to use the renamed “Current limits” section anchor
(`#current-limits`), preserving the existing link target path.
In `@docs/language/providers.mdx`:
- Line 45: Update the links at docs/language/providers.mdx lines 45-45 and 98-98
to use the existing “Let Forst infer the return type” text and
`#let-forst-infer-the-return-type` anchor, replacing the dangling target in both
locations.
In `@docs/language/type-guards.mdx`:
- Around line 15-17: Rework the introductions in docs/language/type-guards.mdx
lines 15-17 and docs/language/union-and-intersection-types.mdx lines 10-14 so
each page leads with its first practical example. In type-guards.mdx, move or
simplify the predicate and refinement-types explanation until after the reusable
guard workflow; in union-and-intersection-types.mdx, move or simplify the
set-theory explanation until after the named error-union example, defining
technical terms in plain language before use.
- Line 168: Update the sentence near “Continue with” to hyphenate the compound
adjective as “built-in constraints,” leaving the rest of the text unchanged.
In `@docs/language/union-and-intersection-types.mdx`:
- Line 129: Update the sentence containing “end to end path” to use the compound
adjective “end-to-end path,” preserving the rest of the wording.
In
`@examples/out/rfc/node-interop/multi-package-dev/main_forst_invoke_server.gen.go`:
- Line 22: Update DecodeArgsFromJSONQualified to validate that the decoded
argument array length matches fn.Parameters before indexing forstInvokeArgs[0],
returning the established decode/arity error on mismatch. Apply the same
behavior to the related invoke codec and invoke server tests, and regenerate the
affected golden output.
In `@forst/cmd/forst/generate_client_package.go`:
- Around line 171-173: Replace strconv.Quote in jsonString with encoding/json
string marshaling so user-configured values are emitted as valid JSON strings,
including invalid UTF-8 handling. Add the encoding/json import and preserve the
existing jsonString interface and callers.
In `@forst/cmd/forst/generate_dist_layout_test.go`:
- Around line 335-346: Update TestGenerate_neverWritesLegacyDirectories to
inspect every generate*.go source file in the package, including generate.go,
generate_dist.go, generate_client_package.go, and generate_ssr.go, when checking
for banned output path segments. Preserve the existing failure behavior while
ensuring newly moved output-path code cannot bypass the guard.
In `@forst/cmd/forst/generate_dist.go`:
- Around line 97-137: Update the module-generation loop in the function
containing activePackages and writeGeneratedFile calls to return the first write
error instead of logging it and continuing. Only add each package to
activePackages after all core and package files have been written successfully,
so failed packages are pruned and downstream index/package exports are not
emitted for missing files.
In `@forst/cmd/forst/generate_self_contained_test.go`:
- Around line 126-129: Restrict the node: import exemption in the generated-file
validation loop to cases where path identifies the generated testing module;
continue rejecting node: imports from core and package modules. Add a focused
test covering both an allowed testing-module import and a rejected
non-testing-module import.
In `@forst/cmd/forst/generate_ssr.go`:
- Around line 22-25: Update GenerateConfig.Validate to reject generate.ssrModule
values whose cleaned path escapes the configured boundary root, including
leading ".." traversal segments. Ensure writeSSRModule uses only the validated
SSR path when constructing modulePath and creating directories, preserving
writes within boundaryRoot.
In `@forst/cmd/forst/generate_subpaths_test.go`:
- Around line 96-105: Update the export assertion around exports["./types"] to
fail when the entry is absent, then validate its resolved path. Preserve the
existing rejection of compiler-owned /types/types.d.ts and the requirement that
the path contains /pkg/types.d.ts.
In `@forst/cmd/forst/generate_testing_hooks_test.go`:
- Around line 433-454: Rename the helper runPhase5NodeAcceptance to
runNodeAcceptance, and update all call sites in the file, including the seven
tests identified in the review, so no references to the phase-labeled name
remain.
---
Outside diff comments:
In `@docs/interop/node/testing.mdx`:
- Around line 109-124: Update the testing example’s invoke error import to use
InvokeRejected from `@forst/gen/testing`, while retaining the existing root import
for InvokeTimedOut from `@forst/gen`. Keep the withForstTestScope transport
behavior and error construction unchanged.
In `@forst/internal/transformer/ts/output.go`:
- Around line 91-106: Remove the unused TypeScriptOutput.GeneratePackageModule
method and its associated implementation, since package modules are generated
through EmitPackageESM and EmitPackageDTS. Do not add replacement callers or
alter the existing emission flow.
---
Duplicate comments:
In `@docs/interop/node/effect.mdx`:
- Around line 133-144: Update the TypeScript snippet imports to include the
VerifyPassword symbol used by the Effect.runPromise call, while preserving the
existing Auth import and AuthTest layer setup.
In `@forst/cmd/forst/generate_effect_mode_test.go`:
- Around line 84-86: Remove the empty if conditional checking Promise and the
Echo export from the test, leaving the existing assertions around the Echo
declaration unchanged.
- Around line 377-383: Update
TestGenerate_effectMode_coreModulesContainNoEffectImport to assert that core
does not contain an effect import, matching the import-pattern check used by
TestGenerate_promiseMode_emitsNoEffectImport, rather than rejecting every
occurrence of the substring “effect”.
- Around line 251-348: Extract the repeated Promise-mode setup from the three
tests into a helper next to generateEffectProject, named generatePromiseProject.
Have it mark itself as a helper, create the minimal project and ftconfig.json,
run generateCommand with the existing failure message, and return
defaultClientDistDir(dir); replace each duplicated setup with a call to this
helper while preserving the existing comparisons.
In `@forst/cmd/forst/generate_testing_hooks_test.go`:
- Around line 91-93: Validate the ifaceStart and fnStart results from
strings.Index before slicing got in the test that checks generated TypeScript.
If either marker is missing, fail the test with a clear assertion describing the
absent fragment; only construct body after both indices are valid.
- Around line 173-193: Rename TestGenerate_acceptance_scopedPackageOverride to
identify the input pass-through behavior it uniquely verifies. Add a separate
acceptance test for a partial packages.main override that omits one function,
invokes that omitted function, and asserts the failure is InvokeRejected; keep
the existing override assertion focused on the overridden function’s result.
- Around line 407-431: Refactor
TestGenerate_acceptance_testdataExampleRunsWithoutHTTP to reuse
runPhase5NodeAcceptance for the shared Node acceptance setup and execution. Pass
the existing scoped-override.mjs source through the helper’s script-source
parameter or equivalent, removing the duplicated node lookup, temp directory,
generation, script writing, and command execution while preserving the test’s
behavior.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 9fb00543-d5c6-43a6-b55d-a9b6dbbaac9a
📒 Files selected for processing (46)
.cursor/rules/docs-readability.mdcCHANGELOG.mdTaskfile.ymldocs/docs.jsondocs/installation.mdxdocs/interop/go.mdxdocs/interop/node.mdxdocs/interop/node/call-forst.mdxdocs/interop/node/call-javascript.mdxdocs/interop/node/effect.mdxdocs/interop/node/generate-types.mdxdocs/interop/node/testing.mdxdocs/language/ensure-and-narrowing.mdxdocs/language/errors-and-result.mdxdocs/language/providers.mdxdocs/language/shapes-and-constraints.mdxdocs/language/type-guards.mdxdocs/language/union-and-intersection-types.mdxdocs/resources/roadmap.mdxdocs/snippets/node-client-setup-note.mdxdocs/workflow/cli.mdxdocs/workflow/dev-server.mdxexamples/in/rfc/node-interop/multi-package-dev/README.mdexamples/in/rfc/node-interop/multi-package-dev/auth.ftexamples/out/rfc/node-interop/multi-package-dev/auth/auth.gen.goexamples/out/rfc/node-interop/multi-package-dev/main_forst_invoke_server.gen.goforst/cmd/forst/dev_server_typescript_test.goforst/cmd/forst/generate.goforst/cmd/forst/generate_client_package.goforst/cmd/forst/generate_dist.goforst/cmd/forst/generate_dist_layout_test.goforst/cmd/forst/generate_effect_mode_test.goforst/cmd/forst/generate_errors_options_test.goforst/cmd/forst/generate_self_contained_test.goforst/cmd/forst/generate_ssr.goforst/cmd/forst/generate_subpaths_test.goforst/cmd/forst/generate_test.goforst/cmd/forst/generate_testing_hooks_test.goforst/cmd/forst/testdata/testing/scoped-override.mjsforst/internal/compiler/compile_pipeline_invoke_test.goforst/internal/transformer/ts/emit_esm.goforst/internal/transformer/ts/emit_esm_test.goforst/internal/transformer/ts/output.goforst/internal/transformer/ts/output_test.goforst/internal/transformer/ts/transformer_test.goforst/internal/transformer/ts/transport.go
Comment on lines
+52
to
+67
| npm install @forst/cli | ||
| ``` | ||
|
|
||
| ```json | ||
| { | ||
| "scripts": { | ||
| "postinstall": "forst generate ." | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ```bash | ||
| npx forst generate . | ||
| ``` | ||
|
|
||
| The `postinstall` line is required. The client lives under `.forst/client` (gitignored) and is linked into `node_modules`. A fresh `npm ci` removes that link. Without `postinstall`, imports of `@forst/gen` fail until someone runs generate by hand. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clarify the CLI dependency scope for postinstall.
The earlier npm example installs @forst/cli as a dev dependency, while this workflow installs it as a regular dependency. The postinstall script needs the CLI when it creates .forst/client. State whether production installs must keep the CLI in dependencies, or use -D and generate the client during the build.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/installation.mdx` around lines 52 - 67, Clarify the installation
workflow around the postinstall script and `@forst/cli`: explicitly state whether
production installs must keep `@forst/cli` in dependencies, or whether it may
remain a devDependency when client generation runs during the build. Keep the
dependency declaration and generation commands consistent with the chosen
deployment behavior.
Comment on lines
+49
to
50
| `forst generate` emits a linked package at `@forst/gen`. The invoke wire protocol is JSON over HTTP. Any Node runtime works — Express, Remix, plain `node`. | ||
| </Info> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching docs/interop/node.mdx:"
git ls-files | grep -F 'docs/interop/node.mdx' || true
echo
echo "Relevant docs/interop/node.mdx lines:"
if [ -f docs/interop/node.mdx ]; then
nl -ba docs/interop/node.mdx | sed -n '1,140p'
fi
echo
echo "Search for generated package/Node version/runtime compatibility statements:"
rg -n "forst generate|`@forst/gen`|Node runtime|node version|Express|Remix|plain node|web APIs|ESM|fetch" docs package.json README.md . -g '!node_modules' -g '!dist' -g '!build' | head -200 || trueRepository: forst-lang/forst
Length of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant docs/interop/node.mdx lines:"
if [ -f docs/interop/node.mdx ]; then
awk '{printf "%8d %s\n", NR, $0}' docs/interop/node.mdx | sed -n '1,160p'
fi
echo
echo "Search for generated package/Node version/runtime compatibility statements:"
rg -n "forst generate|`@forst/gen`|Node runtime|node version|Express|Remix|plain node|web APIs|ESM|fetch|fetch API|Node js" docs package.json README.md . -g '!node_modules' -g '!dist' -g '!build' | head -200 || trueRepository: forst-lang/forst
Length of output: 32937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find generated `@forst/gen` client package files:"
fd -a 'package\.json$' .forst/client packages examples 2>/dev/null | sed 's#^\./##' | head -100 || true
echo
echo "package.json candidates mentioning Node engine/modules/type:"
for f in $(fd 'package\.json$' .forst/client packages examples 2>/dev/null | head -50); do
echo "## $f"
sed -n '1,220p' "$f" | python3 - <<'PY'
import sys, json
x=json.load(sys.stdin)
for key in ("name","version","type","engines","exports","main","module","files"):
if key in x:
print(f"{key}: {x[key]}")
PY
done
echo
echo "Generated client outline/client files containing fetch/http/ESM syntax:"
if [ -d .forst/client ]; then
fd -e 'js|ts|d\.ts|json' .forst/client | sed -n '1,120p' || true
rg -n "createInvokeClient|fetch|fetchWithRetry|POST /invoke|`@forst/gen`|export type|export interface|from " .forst/client | sed -n '1,240p' || true
fi
echo
echo "Check Go generate package writer for emit options relevant to Node compatibility:"
rg -n "generate|outDir|packageName|type .*json|module:|commonjs|imports|fetch|http|node" forst cmd packages -g '*.go' -g '*.ts' -g '*.js' | head -300 || trueRepository: forst-lang/forst
Length of output: 1489
Separate Node version support from framework support.
Any Node runtime works — Express, Remix, plain node mixes Node version compatibility with framework support. State the supported Node range first, then list Express, Remix, and plain Node as supported environments. Specify the required ESM and web/HTTP APIs so plain Node users know what they must provide.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interop/node.mdx` around lines 49 - 50, Update the interoperability
documentation around the `forst generate` description to state the supported
Node.js version range first, then separately list Express, Remix, and plain Node
as supported environments. Explicitly document the required ESM and web/HTTP
APIs for plain Node users.
Comment on lines
+163
to
+164
| The app may need time to finish setup before it can accept calls. Forst waits | ||
| for an app ready signal before connecting. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
"app ready signal" modifies "signal". Write "app-ready signal".
✏️ Proposed fix
The app may need time to finish setup before it can accept calls. Forst waits
-for an app ready signal before connecting.
+for an app-ready signal before connecting.As per coding guidelines: "Use hyphens only when grammar or a technical name requires them."
📝 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
| The app may need time to finish setup before it can accept calls. Forst waits | |
| for an app ready signal before connecting. | |
| The app may need time to finish setup before it can accept calls. Forst waits | |
| for an app-ready signal before connecting. |
🧰 Tools
🪛 LanguageTool
[grammar] ~164-~164: Use a hyphen to join words.
Context: ...can accept calls. Forst waits for an app ready signal before connecting. **Defau...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interop/node/call-javascript.mdx` around lines 163 - 164, Update the
setup documentation sentence near “Forst waits” to hyphenate the compound
modifier as “app-ready signal,” leaving the surrounding wording unchanged.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+285
to
+287
| Bootstrap mode starts a small Forst owned script. It does not run one of your | ||
| TypeScript files as the process entry. Your modules load only when an allowed | ||
| call needs them. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate "Forst-owned".
"Forst owned script" uses a compound modifier before the noun.
✏️ Proposed fix
-Bootstrap mode starts a small Forst owned script. It does not run one of your
+Bootstrap mode starts a small Forst-owned script. It does not run one of your
TypeScript files as the process entry. Your modules load only when an allowed
call needs them.As per coding guidelines: "Use hyphens only when grammar or a technical name requires them."
📝 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
| Bootstrap mode starts a small Forst owned script. It does not run one of your | |
| TypeScript files as the process entry. Your modules load only when an allowed | |
| call needs them. | |
| Bootstrap mode starts a small Forst-owned script. It does not run one of your | |
| TypeScript files as the process entry. Your modules load only when an allowed | |
| call needs them. |
🧰 Tools
🪛 LanguageTool
[grammar] ~285-~285: Use a hyphen to join words.
Context: ...ode Bootstrap mode starts a small Forst owned script. It does not run one of you...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/interop/node/call-javascript.mdx` around lines 285 - 287, Update the
prose in the bootstrap mode description to hyphenate the compound modifier as
“Forst-owned” before “script,” leaving the surrounding documentation unchanged.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+126
to
+129
| // node: builtins are allowed for the testing module (AsyncLocalStorage). | ||
| if strings.HasPrefix(spec, "node:") { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the node: exception to the testing module.
The comment limits this exception to the generated testing module. The condition accepts node: imports from every file under dist/.
A core or package module can gain a Node-only dependency without this test failing. Allow this prefix only when path identifies the generated testing subpath.
As per coding guidelines, “Ensure presence of precise, reproducing unit or integration tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_self_contained_test.go` around lines 126 - 129,
Restrict the node: import exemption in the generated-file validation loop to
cases where path identifies the generated testing module; continue rejecting
node: imports from core and package modules. Add a focused test covering both an
allowed testing-module import and a rejected non-testing-module import.
Source: Coding guidelines
Comment on lines
+22
to
+25
| modulePath := filepath.Join(boundaryRoot, filepath.Clean(ssrRelPath)) | ||
| if err := generateIO.MkdirAll(filepath.Dir(modulePath), 0755); err != nil { | ||
| return fmt.Errorf("failed to create SSR module directory: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -C6 'SSRModule' forst/internal/ftconfig
ast-grep run --pattern 'func ($_ GenerateConfig) Validate() error { $$$ }' --lang go forst/internal/ftconfigRepository: forst-lang/forst
Length of output: 5374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg 'forst/cmd/forst/generate_ssr\.go|forst/internal/ftconfig/generate\.go|forst/internal/ftconfig/generate_test\.go|forst/internal/ftconfig/ftconfig\.go'
echo
echo "== generate_ssr.go outline and relevant lines =="
ast-grep outline forst/cmd/forst/generate_ssr.go || true
wc -l forst/cmd/forst/generate_ssr.go
sed -n '1,140p' forst/cmd/forst/generate_ssr.go
echo
echo "== validation-related search in ftconfig =="
rg -n -C 4 'SSRModule|Validate\(|Valid' forst/internal/ftconfig/*.go
echo
echo "== go filepath edge-case semantics =="
tmp="$(mktemp -d)"
cat > "$tmp/filepath_probe.go" <<'EOF'
package main
import (
"fmt"
"path/filepath"
"runtime"
"strings"
)
func main() {
sep := string(filepath.Separator)
cases := []string{"../../src/invoke.ts", "../foo", "/abs", "src/ssr.ts", strings.Repeat("../", 4) + "src/ssr.ts"}
for _, ssrRelPath := range cases {
boundaryRoot := "/project"
modulePath := filepath.Join(boundaryRoot, filepath.Clean(ssrRelPath))
rel, err := filepath.Rel(boundaryRoot, modulePath)
outside := err != nil || rel == ".." || strings.HasPrefix(rel, ".."+sep)
fmt.Printf("input=%q modulePath=%q rel=%v err=%v outside=%v\n", ssrRelPath, modulePath, rel, err, outside)
}
fmt.Println("os", runtime.GOOS)
fmt.Println("filepath base", filepath.Base("/x"))
}
EOF
cd "$tmp"
go run filepath_probe.goRepository: forst-lang/forst
Length of output: 12121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect generate validation implementation and tests =="
sed -n '1,190p' forst/internal/ftconfig/generate.go
sed -n '144,163p' forst/internal/ftconfig/generate_test.go
echo
echo "== use/write sites for writeSSRModule and GenerateConfig.Validate =="
rg -n -C 4 'writeSSRModule|GenerateConfig\(\)|Default\(\)|Validate\(\)' forst/cmd forst/internal --glob '*.go'Repository: forst-lang/forst
Length of output: 45632
Reject escaping generate.ssrModule paths before writing.
GenerateConfig.Validate checks generate.outDir, but generate.ssrModule is written through writeSSRModule without escaping validation. filepath.Clean("../../src/invoke.ts") leaves a leading .., so writeSSRModule writes outside boundaryRoot and creates files under the parent directory. Add the containment check to GenerateConfig.Validate and keep SSR writing on the validated path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_ssr.go` around lines 22 - 25, Update
GenerateConfig.Validate to reject generate.ssrModule values whose cleaned path
escapes the configured boundary root, including leading ".." traversal segments.
Ensure writeSSRModule uses only the validated SSR path when constructing
modulePath and creating directories, preserving writes within boundaryRoot.
Comment on lines
+96
to
+105
| if entry, ok := exports["./types"]; ok { | ||
| m := entry.(map[string]any) | ||
| typesPath, _ := m["types"].(string) | ||
| if strings.HasSuffix(typesPath, "/types.d.ts") && !strings.Contains(typesPath, "/pkg/") { | ||
| t.Fatalf("./types must resolve to pkg module, not compiler types file: %#v", entry) | ||
| } | ||
| if !strings.Contains(typesPath, "/pkg/types.d.ts") { | ||
| t.Fatalf("./types should point at dist/pkg/types.d.ts, got %#v", entry) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require the valid ./types package export.
This test passes when exports["./types"] is absent. It then cannot verify that a user package named types remains importable.
Fail when the entry is absent before checking that it resolves to dist/pkg/types.d.ts. Keep the check that rejects the compiler-owned declaration path.
As per coding guidelines, “Ensure presence of precise, reproducing unit or integration tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_subpaths_test.go` around lines 96 - 105, Update the
export assertion around exports["./types"] to fail when the entry is absent,
then validate its resolved path. Preserve the existing rejection of
compiler-owned /types/types.d.ts and the requirement that the path contains
/pkg/types.d.ts.
Source: Coding guidelines
Comment on lines
+433
to
+454
| func runPhase5NodeAcceptance(t *testing.T, name, body string) { | ||
| t.Helper() | ||
| if _, err := exec.LookPath("node"); err != nil { | ||
| t.Skip("node not found") | ||
| } | ||
| dir := t.TempDir() | ||
| writeMainFt(t, dir, generateTestMinimalValidForst) | ||
| if err := generateCommand([]string{dir}); err != nil { | ||
| t.Fatalf("generateCommand: %v", err) | ||
| } | ||
| dist := defaultClientDistDir(dir) | ||
| script := filepath.Join(dist, name) | ||
| if err := os.WriteFile(script, []byte(body), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| cmd := exec.Command("node", script) | ||
| cmd.Dir = dist | ||
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("%s failed: %v\n%s", name, err, out) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename the helper to drop the phase label.
The file and the tests no longer use the "phase 5" label. The helper name runPhase5NodeAcceptance still carries it. Rename it to runNodeAcceptance and update the call sites at lines 152, 174, 196, 224, 275, 310, and 347.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_testing_hooks_test.go` around lines 433 - 454,
Rename the helper runPhase5NodeAcceptance to runNodeAcceptance, and update all
call sites in the file, including the seven tests identified in the review, so
no references to the phase-labeled name remain.
Propagate core and package module write failures instead of logging and continuing. Parse Effect semver strictly so prerelease and malformed versions do not satisfy the floor check. Walk all ancestor package.json files when checking for a forst generate lifecycle script. Lower unchanged link-ready logging to debug and return copy destination close errors. fix(node-interop): align multi-package-dev golden with auth package Update node interop compile golden expectations from bcrypt to auth after the example package rename. test(generate): isolate acceptance and effect-mode scenarios Split monolithic acceptance tests into named t.Run subtests. Add clientDistIncludeGlob, generatePromiseProject, and stricter assertions for cross-file types, semver cases, link fixtures, and Effect e2e tsc without skipLibCheck. fix(ftconfig): reject effect mode with testingSubpath effect Validate rejects generate.effect together with testingSubpath set to effect. Add layout IsReservedDotForstEntry lookup and include reserved subpath reason in collision errors. Use bracket package keys in Effect testing emit. docs: fix interop snippets and sidecar output path Import VerifyPassword in the Effect Layer.mock doc example. Document .forst/client as generateTypes output in the sidecar README. Treat empty FORST_BASE_URL as unset in the remix-serve route and tighten tictactoe simulation winner assertions.
writeFileAtomic now defers temp file removal so a failed WriteFile does not leave .tmp-* files in the output directory. test(generate): close ACCEPTANCE gaps and add edge-case coverage Add TestGenerate_tsc_supportedModuleResolutions for bundler, nodenext, and preserve moduleResolution without tsconfig paths, asserting the generated package.json has no typesVersions map. Add consolidated TestGenerate_acceptance_effectCompatibility covering promise-mode catchTag typing, byte-identical core modules across runtimes, effect tsc and runtime fixtures, and the effect version floor. Cover node_modules link refusal for regular files and corrupt ownership markers, semver parsing edge cases, and a promise-mode InvokeFailure catchTag fixture. fix(transformer/ts): reject case-variant reserved subpaths ValidateReservedSubpaths compares case-insensitively so Forst packages such as Testing cannot collide with the ./testing export on case-insensitive filesystems. Improve the collision error when symbol-only package names map to an empty Effect service class.
Expect InvokeHttpFailure from @forst/gen when a duplicate move fails, using responseText instead of sidecar DevServerHttpFailure fields. Assert GameState.status is x_won after X wins the top row, matching the engine type rather than a nonexistent winner field.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forst/cmd/forst/generate_tsc_test.go (1)
143-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated tsconfig writer.
writeTSConfigWithModuleResolutionandwriteTSConfigwrite the sameapp-smoke.tscontent and the sameIncludelist. Onlymodule,moduleResolution, and the output file name differ. ExpresswriteTSConfigin terms of the new helper and rename the output file, or add a file-name parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/cmd/forst/generate_tsc_test.go` around lines 143 - 211, Remove the duplicated configuration-building logic by making writeTSConfig delegate to writeTSConfigWithModuleResolution, or by extending the helper with an output filename parameter. Preserve the shared app-smoke.ts content and Include entries, while ensuring the default writeTSConfig path still uses ESNext/bundler settings and writes tsconfig.json rather than the module-resolution-specific filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forst/cmd/forst/generate_acceptance_test.go`:
- Around line 938-949: Update the promise catchTag fixture at
forst/cmd/forst/generate_acceptance_test.go:938-949 and the Effect consumer
fixture at forst/cmd/forst/generate_acceptance_test.go:1047-1058 to construct
their tsconfig through the existing tsConfig structure, using
clientDistIncludeGlob(dir) for the declaration include path instead of
hardcoding .forst/client/dist/**/*.d.ts; follow the existing pattern near line
230.
In `@forst/cmd/forst/generate_effect.go`:
- Around line 109-115: Reject SemVer components with leading zeroes in the
parsing loop of generate_effect.go by invalidating any component longer than one
character whose first character is ‘0’; retain acceptance of the single
component “0”. Update the leading_zero test case in
forst/cmd/forst/generate_effect_test.go:21 to expect false.
In `@forst/cmd/forst/generate_guard.go`:
- Around line 54-74: Update anyAncestorRunsForstGenerate so errors returned by
packageJSONRunsForstGenerate for an individual manifest are ignored, allowing
the loop to continue checking parent directories. Preserve successful true
results and the existing termination behavior when no ancestor declares the
script.
In `@forst/internal/transformer/ts/reserved_test.go`:
- Around line 105-111: Update the table-driven loops in the tests around
ServiceClassName, including
TestServiceClassName_symbolOnlyNameProducesEmptyString and the additional loop
near the referenced lines, to wrap each input in t.Run(name, func(t *testing.T)
{ ... }). Use the input as the subtest name and call t.Fatalf on the subtest
handle so failures identify the specific case without stopping other inputs.
In `@forst/internal/transformer/ts/reserved.go`:
- Around line 41-46: The collision error construction in reserved-key validation
must use remediation text specific to the conflicting key: for the built-in
“types” key, do not mention a test double or suggest changing
generate.testingSubpath, while preserving the existing guidance for configurable
testing-subpath collisions. Add coverage asserting the key-specific “types”
diagnostic.
---
Outside diff comments:
In `@forst/cmd/forst/generate_tsc_test.go`:
- Around line 143-211: Remove the duplicated configuration-building logic by
making writeTSConfig delegate to writeTSConfigWithModuleResolution, or by
extending the helper with an output filename parameter. Preserve the shared
app-smoke.ts content and Include entries, while ensuring the default
writeTSConfig path still uses ESNext/bundler settings and writes tsconfig.json
rather than the module-resolution-specific filename.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: c365b65c-b4a9-412d-9740-5ea3543cd9f9
📒 Files selected for processing (34)
docs/interop/node/effect.mdxexamples/in/rfc/node-interop/remix-serve/app/routes/_index.tsxexamples/in/tictactoe/tests/tictactoe-game.simulation.test.tsforst/cmd/forst/dev_server_typescript_test.goforst/cmd/forst/generate_acceptance_test.goforst/cmd/forst/generate_cross_file_test.goforst/cmd/forst/generate_dist.goforst/cmd/forst/generate_effect.goforst/cmd/forst/generate_effect_e2e_test.goforst/cmd/forst/generate_effect_mode_test.goforst/cmd/forst/generate_effect_test.goforst/cmd/forst/generate_guard.goforst/cmd/forst/generate_link.goforst/cmd/forst/generate_link_test.goforst/cmd/forst/generate_self_contained_test.goforst/cmd/forst/generate_test.goforst/cmd/forst/generate_testing_hooks_test.goforst/cmd/forst/generate_tsc_examples_test.goforst/cmd/forst/generate_tsc_test.goforst/cmd/forst/generate_write.goforst/cmd/forst/generate_write_test.goforst/cmd/forst/node_interop_goldens_test.goforst/cmd/forst/testdata/effect/promise-catchtag.tsforst/internal/codegen/layout/layout.goforst/internal/codegen/layout/layout_test.goforst/internal/ftconfig/generate.goforst/internal/ftconfig/generate_test.goforst/internal/transformer/ts/emit_effect.goforst/internal/transformer/ts/emit_effect_test.goforst/internal/transformer/ts/emit_esm_test.goforst/internal/transformer/ts/errors.goforst/internal/transformer/ts/reserved.goforst/internal/transformer/ts/reserved_test.gopackages/sidecar/README.md
Comment on lines
+938
to
+949
| cfg := `{ | ||
| "compilerOptions": { | ||
| "target": "ES2022", | ||
| "module": "ESNext", | ||
| "moduleResolution": "bundler", | ||
| "strict": true, | ||
| "noEmit": true, | ||
| "skipLibCheck": true, | ||
| "types": [] | ||
| }, | ||
| "include": ["promise-catchtag.ts", ".forst/client/dist/**/*.d.ts"] | ||
| }` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded .forst/client/dist/**/*.d.ts in two inline tsconfig fixtures. Both fixtures embed the declaration glob as a string literal. Line 230 of the same file already derives it with clientDistIncludeGlob(projectRoot). If the default outDir changes, tsc includes zero declaration files and both fixtures pass without checking the generated types.
forst/cmd/forst/generate_acceptance_test.go#L938-L949: build the promise catchTag tsconfig from thetsConfigstruct andclientDistIncludeGlob(dir).forst/cmd/forst/generate_acceptance_test.go#L1047-L1058: build the Effect consumer tsconfig the same way.
📍 Affects 1 file
forst/cmd/forst/generate_acceptance_test.go#L938-L949(this comment)forst/cmd/forst/generate_acceptance_test.go#L1047-L1058
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_acceptance_test.go` around lines 938 - 949, Update
the promise catchTag fixture at
forst/cmd/forst/generate_acceptance_test.go:938-949 and the Effect consumer
fixture at forst/cmd/forst/generate_acceptance_test.go:1047-1058 to construct
their tsconfig through the existing tsConfig structure, using
clientDistIncludeGlob(dir) for the declaration include path instead of
hardcoding .forst/client/dist/**/*.d.ts; follow the existing pattern near line
230.
Comment on lines
+109
to
+115
| var out [3]int | ||
| for i := 0; i < 3; i++ { | ||
| n, err := strconv.Atoi(parts[i]) | ||
| if err != nil || parts[i] == "" || n < 0 { | ||
| return [3]int{}, false | ||
| } | ||
| out[i] = n |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject leading-zero SemVer components.
Strict SemVer does not permit a numeric component such as 017. The current parser accepts it, and the test preserves that invalid behavior.
forst/cmd/forst/generate_effect.go#L109-L115: reject components longer than one character when their first character is0.forst/cmd/forst/generate_effect_test.go#L21-L21: change theleading_zeroexpected result tofalse.
Proposed fix
for i := 0; i < 3; i++ {
+ if len(parts[i]) > 1 && parts[i][0] == '0' {
+ return [3]int{}, false
+ }
n, err := strconv.Atoi(parts[i])- {"leading_zero", "3.017.0", "3.17.0", true},
+ {"leading_zero", "3.017.0", "3.17.0", false},📍 Affects 2 files
forst/cmd/forst/generate_effect.go#L109-L115(this comment)forst/cmd/forst/generate_effect_test.go#L21-L21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_effect.go` around lines 109 - 115, Reject SemVer
components with leading zeroes in the parsing loop of generate_effect.go by
invalidating any component longer than one character whose first character is
‘0’; retain acceptance of the single component “0”. Update the leading_zero test
case in forst/cmd/forst/generate_effect_test.go:21 to expect false.
Comment on lines
+54
to
+74
| func anyAncestorRunsForstGenerate(start string) (bool, error) { | ||
| dir := filepath.Clean(start) | ||
| for { | ||
| candidate := filepath.Join(dir, "package.json") | ||
| if st, err := os.Stat(candidate); err == nil && !st.IsDir() { | ||
| has, checkErr := packageJSONRunsForstGenerate(candidate) | ||
| if checkErr != nil { | ||
| return false, checkErr | ||
| } | ||
| if has { | ||
| return true, nil | ||
| } | ||
| } | ||
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| break | ||
| } | ||
| dir = parent | ||
| } | ||
| return false, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not abort the ancestor walk on one unreadable manifest.
anyAncestorRunsForstGenerate returns the first packageJSONRunsForstGenerate error. One malformed or unreadable package.json in a nearer directory then stops the walk. The root manifest that declares the postinstall script is never inspected, and warnMissingLifecycleScript prints the warning incorrectly. Skip the failing manifest and continue upward.
🐛 Proposed fix
if st, err := os.Stat(candidate); err == nil && !st.IsDir() {
- has, checkErr := packageJSONRunsForstGenerate(candidate)
- if checkErr != nil {
- return false, checkErr
- }
- if has {
+ has, checkErr := packageJSONRunsForstGenerate(candidate)
+ if checkErr == nil && has {
return true, nil
}
}📝 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
| func anyAncestorRunsForstGenerate(start string) (bool, error) { | |
| dir := filepath.Clean(start) | |
| for { | |
| candidate := filepath.Join(dir, "package.json") | |
| if st, err := os.Stat(candidate); err == nil && !st.IsDir() { | |
| has, checkErr := packageJSONRunsForstGenerate(candidate) | |
| if checkErr != nil { | |
| return false, checkErr | |
| } | |
| if has { | |
| return true, nil | |
| } | |
| } | |
| parent := filepath.Dir(dir) | |
| if parent == dir { | |
| break | |
| } | |
| dir = parent | |
| } | |
| return false, nil | |
| } | |
| func anyAncestorRunsForstGenerate(start string) (bool, error) { | |
| dir := filepath.Clean(start) | |
| for { | |
| candidate := filepath.Join(dir, "package.json") | |
| if st, err := os.Stat(candidate); err == nil && !st.IsDir() { | |
| has, checkErr := packageJSONRunsForstGenerate(candidate) | |
| if checkErr == nil && has { | |
| return true, nil | |
| } | |
| } | |
| parent := filepath.Dir(dir) | |
| if parent == dir { | |
| break | |
| } | |
| dir = parent | |
| } | |
| return false, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/cmd/forst/generate_guard.go` around lines 54 - 74, Update
anyAncestorRunsForstGenerate so errors returned by packageJSONRunsForstGenerate
for an individual manifest are ignored, allowing the loop to continue checking
parent directories. Preserve successful true results and the existing
termination behavior when no ancestor declares the script.
Comment on lines
+105
to
+111
| func TestServiceClassName_symbolOnlyNameProducesEmptyString(t *testing.T) { | ||
| for _, name := range []string{"___", "...", "---"} { | ||
| if got := ServiceClassName(name); got != "" { | ||
| t.Fatalf("ServiceClassName(%q) = %q, want empty", name, got) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Name each input with t.Run.
Both loops iterate several inputs in one test function. The first t.Fatalf stops the remaining inputs, and the report does not name the case. Wrap each iteration in t.Run(name, ...).
As per coding guidelines: "Implement table-driven tests for multiple inputs and name subtests with t.Run("case", ...)."
Also applies to: 126-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/reserved_test.go` around lines 105 - 111,
Update the table-driven loops in the tests around ServiceClassName, including
TestServiceClassName_symbolOnlyNameProducesEmptyString and the additional loop
near the referenced lines, to wrap each input in t.Run(name, func(t *testing.T)
{ ... }). Use the input as the subtest name and call t.Fatalf on the subtest
handle so failures identify the specific case without stopping other inputs.
Source: Coding guidelines
Comment on lines
+41
to
+46
| return fmt.Errorf( | ||
| "generate: Forst package %q collides with the reserved client subpath \"./%s\" (%s)\n"+ | ||
| " the generated package exports a test double at <packageName>/%s\n"+ | ||
| " rename the Forst package, or set generate.testingSubpath to a different key", | ||
| pkg, key, reason, key, | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make remediation text specific to the reserved key.
For the built-in types key, Line 43 incorrectly says that the package exports a test double. Line 44 suggests changing generate.testingSubpath, but that cannot resolve a collision with types. Emit key-specific remediation text, and add coverage for the types diagnostic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/reserved.go` around lines 41 - 46, The
collision error construction in reserved-key validation must use remediation
text specific to the conflicting key: for the built-in “types” key, do not
mention a test double or suggest changing generate.testingSubpath, while
preserving the existing guidance for configurable testing-subpath collisions.
Add coverage asserting the key-specific “types” diagnostic.
Remove the Vite alias that mapped @forst/gen to .forst/client. That bypassed the package exports map and broke subpath imports such as @forst/gen/main during the Remix SSR build. Keep ssr.noExternal so the generated client still bundles for server rendering. docs(generate): document bundler wiring for @forst/gen Add guidance for Vite, Remix, and webpack adopters: rely on the node_modules link from forst generate, avoid aliasing @forst/gen to the output directory, and use ssr.noExternal for SSR frameworks. Point to the remix-serve example and its CI standalone e2e task.
Document one .forst tree per ftconfig boundary, node_modules link placement, ownership conflicts between boundaries, unique package names, committed mode, and package-manager guidance.
When a parent-owned node host dies during startup, waitForHostMarkerReady previously polled until hostReadyTimeoutSeconds elapsed even though the outcome was already known. EnsureHostProcessRunning now watches process exit alongside the ready-marker poll and returns immediately on crash. Ready-wait failures include the process exit reason and a stderr tail from a small ring buffer on managedProcess, so bind and spawn errors are visible in CI logs instead of a silent 30s timeout. Remove the duplicate reload reattach wrapper test and add coverage for the fast-fail path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forst/nodert/host_lifecycle.go`:
- Around line 204-217: Update hostReadyWaitError in
forst/nodert/host_lifecycle.go (lines 204-217) to preserve exitErr for errors.Is
while retaining ready-file and stderr diagnostics, wrapping the exit error
rather than formatting it only as text. Update the nonzero-exit tests in
forst/nodert/host_lifecycle_test.go (lines 92-117) to use table-driven t.Run
subtests covering clean early exit and ErrNodeRuntimeDied, and assert errors.Is
directly instead of relying on substring matching.
- Around line 168-173: Synchronize the `managed.wait()` call with both
`forwardChildOutput` goroutines so `Cmd.Wait` runs only after stdout and stderr
forwarding has completed. Update the lifecycle flow around `exitCh`,
`waitForHostMarkerReady`, and `managed.stderrTail` to drain both child pipes
before waiting, while preserving the existing startup-error reporting behavior.
In `@forst/nodert/process.go`:
- Around line 43-82: Introduce a private interface for the ring’s required write
and tail-read operations, and make managedProcess depend on that interface
instead of concrete outputRing state. Keep outputRing as the private
implementation, update its method set to satisfy the interface, and adjust
managedProcess references including stderrTail to use the interface.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 9a30049e-cf94-478e-8096-137a8e25991c
📒 Files selected for processing (6)
forst/internal/devserver/reload_node_reattach_test.goforst/nodert/host.goforst/nodert/host_integration_test.goforst/nodert/host_lifecycle.goforst/nodert/host_lifecycle_test.goforst/nodert/process.go
💤 Files with no reviewable changes (1)
- forst/internal/devserver/reload_node_reattach_test.go
Replace the unsynchronized sync.WaitGroup with an inFlightN counter protected by the existing mu and cond. BeginDrain now waits on the condition variable until in-flight work completes, and TrackInFlight increments and decrements under the same lock. This removes a data race where Add and Wait could run concurrently during drain, which failed go test -race and was reachable from invoke middleware during reload.
Introduce a private stderrRingBuffer interface for stderr capture and tail reads, and store that on managedProcess instead of a concrete outputRing pointer. Keep outputRing as the implementation with Write and tail, and route stderrTail through the interface so the process layer is decoupled from the ring type.
Wire watchGenerate (default true) into native forst dev and runtime hot reload so @forst/gen stays in step with .ft edits. Dev /types now reads .forst/client/dist/types.d.ts instead of the legacy inline types generator. Add forst generate --list for JSON export discovery. Warn when pnpm isolated node_modules (.pnpm) is detected. Use real Windows directory junctions via mklink /J instead of symlinks. Generated invoke transport merges config and per-call HTTP headers, validates X-Forst-Contract-Version on responses, and retries with exponential backoff and jitter. Invoke server sets the contract version header on JSON responses. Document custom headers, Node SSR runtime boundaries, known limitations, and CLI --list. Rename generate_acceptance_test.go to generate_client_test.go and add an esbuild bundler integration test when esbuild is on PATH.
Align generated Effect-mode API with Effect ecosystem conventions. ForstClientLayer and ForstTestLayer pair with ForstClientLive and makeForstClientRuntime. Update emitter, tests, fixtures, docs, and generated README copy. BREAKING CHANGE: layerForstClient is now ForstClientLayer and layerForstTest is now ForstTestLayer. Regenerate the client after upgrading when generate.effect is enabled.
Capture the watchGenerate stop channel in a local variable before starting the WatchPackageRoot goroutine so stopWatchGenerate cannot race with the watcher under go test -race. Give the tictactoe example its own generate.packageName (@forst/tictactoe) so CI e2e no longer fails when multipackage-dev claims the default node_modules/@forst/gen slot after watchGenerate on dev was wired in 469c9ef. Update tictactoe imports, README, and Taskfile text to match. Silence errcheck on printGenerateSummary Fprintf calls and copyFileContents deferred Close handlers.
Emit startForstTestServer in Promise mode and the ForstTestServer / ForstTestServerLayer / makeForstTestServer trio in Effect mode. They lazy-import @forst/cli/invoke, wire the default client to the live base URL, and surface harness failures as ForstTestServerFailed outside InvokeFailure. The generated package always declares an optional @forst/cli peer so install stays zero-cost until those helpers are used. fix(invokeserver): record bound port in invoke.ready After StartAsync, write .forst/invoke.ready from BoundAddr so FORST_INVOKE_PORT=0 no longer publishes http://127.0.0.1:0.
Expose startForstInvokeServer so tests and globalSetup can attach to or spawn a Node→Forst invoke server, wait on /health, and stop cleanly. Keeps process handling out of generated clients and separate from @forst/node-runtime.
Overlay caller env on process env for both attach URL lookup and spawn so FORST_BASE_URL and related vars stay consistent. test(cli): cover invoke e2e and require exports Add an opt-in embedded-invoke spawn test gated by FORST_CLI_INVOKE_E2E, declare require conditions on package exports, and run the e2e from ci:e2e. test(tictactoe): use startForstTestServer for simulation Replace hand-rolled forst dev lifecycle with the generated testing helper and flat @forst/tictactoe/main imports.
Run build:cli before bun test so dist/invoke.js exists when the export test runs. Track invoke outputs in build:cli generates. ci: bump arduino/setup-task to v3 Update CI workflows and the shared Go/Bun setup action to avoid Node 20 deprecation warnings on GitHub runners.
Merged
This was referenced Aug 9, 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.
generated/andclient/layout with one configurable package under.forst/clientby default.node_moduleslinking, and provider omission reporting.postinstallgenerate lifecycle script for ephemeral output.BREAKING CHANGE:
forst generateno longer writes siblinggenerated/andclient/directories or depends on@forst/client. Adopters must import from@forst/genor a configuredpackageNamevia subpaths, add apostinstallscript that runsforst generate, and delete any leftovergenerated/orclient/trees themselves. Forst never removes those directories.Summary by CodeRabbit
New Features
.forst/client, available through@forst/gen.Documentation
Bug Fixes