TypeScript SDK for Exosphere #417
Conversation
- Introduced a new TypeScript SDK for ExosphereHost, including core functionalities for state management, graph execution, and node handling. - Added essential files: package.json, README.md, and TypeScript configuration. - Implemented logging capabilities and error handling through custom signals. - Included comprehensive tests to ensure functionality and reliability. - Created a .gitignore file to exclude unnecessary files from version control.
…idation, and add utility functions - Refactored endpoint construction in the Runtime class to use singular 'state' instead of 'states'. - Introduced utility functions for schema validation: isZodObjectSchema and isZodStringSchema. - Updated schema generation logic to use generateFlatSchema for inputs and outputs. - Added Content-Type header to fetch requests in signals and state management. - Included new utils.ts file for schema-related functions and updated tests accordingly.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a new TypeScript SDK for ExosphereHost, including core runtime, state manager, node base class, models, utilities, logger, signals, package config, tests, and CI workflow. Adds a single entrypoint export, comprehensive documentation, versioning scripts, and Vitest setup with extensive unit and integration-like tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as SDK Consumer
participant RT as Runtime
participant SM as StateManager API
participant Node as BaseNode subclass
participant Sec as Secrets Endpoint
App->>RT: new Runtime(namespace, name, nodes, options)
App->>RT: start()
RT->>SM: POST /v{ver}/{ns}/register {nodes}
SM-->>RT: 200 OK
loop Enqueue cycle
RT->>SM: POST /v{ver}/{ns}/{name}/enqueue {batchSize}
SM-->>RT: 200 OK [{state...}]
RT->>RT: Queue states
end
par Workers
RT->>RT: Dequeue state
RT->>Node: new Node()
alt Node needs secrets
RT->>Sec: GET /v{ver}/{ns}/secrets?node=...
Sec-->>RT: 200 OK {secrets}
end
RT->>Node: _execute(inputs, secrets)
alt Normal result
RT->>SM: POST /.../executed {result}
SM-->>RT: 200 OK
else PruneSignal
RT->>SM: POST /.../prune {data}
SM-->>RT: 200 OK
else ReQueueAfterSignal
RT->>SM: POST /.../re-enqueue-after {enqueue_after}
SM-->>RT: 200 OK
else Error
RT->>SM: POST /.../errored {error}
SM-->>RT: 200 OK
end
end
sequenceDiagram
autonumber
actor Dev as SDK Consumer
participant SM as StateManager API
Dev->>SM: PUT /v{ver}/{ns}/graph/{graph} {nodes, secrets, retry, store_config}
SM-->>Dev: 202 Accepted {validation: PENDING, id}
loop Poll until terminal
Dev->>SM: GET /v{ver}/{ns}/graph/{graph}
SM-->>Dev: {validation: PENDING|VALID|INVALID}
end
alt VALID
Dev->>SM: POST /v{ver}/{ns}/graph/{graph}/trigger {inputs, store, start_delay}
SM-->>Dev: 200 OK {state_id}
else INVALID or Timeout
Dev-->>Dev: Throw error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
- Updated the getSecrets method to include 'Content-Type: application/json' in the fetch request headers, ensuring proper content handling for API interactions.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive TypeScript SDK for Exosphere, mirroring the functionality of the existing Python SDK. The implementation is well-structured, covering the runtime, state manager, node definitions, and utility functions. The inclusion of a full test suite is excellent and ensures good coverage. I've identified several areas for improvement, primarily related to correctness in examples, code clarity, and dependency management. Key suggestions include correcting the usage examples in the README, refining Zod models for better maintainability, improving type safety in the BaseNode class, and adjusting the package dependencies. Overall, this is a solid foundation for the TypeScript SDK.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 73
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
typescript-sdk/tests/README.md (1)
78-86: Harden env var handling; add .env.example and per-test isolation.Document .env usage, warn against committing secrets, and show Vitest helpers to isolate process.env.
Apply this diff:
## Environment Variables Tests set up required environment variables: @@ - `EXOSPHERE_API_KEY` -These are automatically configured in test setup and cleaned up between tests. +These are automatically configured in test setup and cleaned up between tests. + +Recommended: +- Provide `typescript-sdk/.env.example` (no real secrets). +- Load via `dotenv` in `vitest.setup.ts`, or inject in tests with helpers. +- Snapshot/restore env per test: +```ts +const OLD_ENV = { ...process.env } +beforeEach(() => { process.env = { ...OLD_ENV } }) +afterAll(() => { process.env = OLD_ENV }) +``` +- In CI, set secrets via secure variables; never commit real keys.I can generate
vitest.config.ts,vitest.setup.ts, and.env.examplealigned with the SDK—say the word.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
typescript-sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
typescript-sdk/.gitignore(1 hunks)typescript-sdk/README.md(1 hunks)typescript-sdk/exospherehost/index.ts(1 hunks)typescript-sdk/exospherehost/logger.ts(1 hunks)typescript-sdk/exospherehost/models.ts(1 hunks)typescript-sdk/exospherehost/node/BaseNode.ts(1 hunks)typescript-sdk/exospherehost/node/index.ts(1 hunks)typescript-sdk/exospherehost/runtime.ts(1 hunks)typescript-sdk/exospherehost/signals.ts(1 hunks)typescript-sdk/exospherehost/stateManager.ts(1 hunks)typescript-sdk/exospherehost/types.ts(1 hunks)typescript-sdk/exospherehost/utils.ts(1 hunks)typescript-sdk/package.json(1 hunks)typescript-sdk/tests/README.md(1 hunks)typescript-sdk/tests/test_base_node.test.ts(1 hunks)typescript-sdk/tests/test_base_node_abstract.test.ts(1 hunks)typescript-sdk/tests/test_base_node_comprehensive.test.ts(1 hunks)typescript-sdk/tests/test_coverage_additions.test.ts(1 hunks)typescript-sdk/tests/test_integration.test.ts(1 hunks)typescript-sdk/tests/test_models_and_statemanager_new.test.ts(1 hunks)typescript-sdk/tests/test_package_init.test.ts(1 hunks)typescript-sdk/tests/test_runtime_comprehensive.test.ts(1 hunks)typescript-sdk/tests/test_runtime_edge_cases.test.ts(1 hunks)typescript-sdk/tests/test_runtime_validation.test.ts(1 hunks)typescript-sdk/tests/test_signals_and_runtime_functions.test.ts(1 hunks)typescript-sdk/tests/test_statemanager_comprehensive.test.ts(1 hunks)typescript-sdk/tests/test_version.test.ts(1 hunks)typescript-sdk/tsconfig.json(1 hunks)typescript-sdk/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
typescript-sdk/tests/test_package_init.test.ts (1)
python-sdk/tests/test_package_init.py (1)
SampleNode(120-131)
typescript-sdk/tests/test_coverage_additions.test.ts (3)
typescript-sdk/exospherehost/stateManager.ts (1)
StateManager(10-134)typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-353)typescript-sdk/exospherehost/signals.ts (2)
PruneSignal(1-20)ReQueueAfterSignal(22-44)
typescript-sdk/tests/test_runtime_edge_cases.test.ts (2)
python-sdk/tests/test_runtime_edge_cases.py (2)
MockTestNodeWithNonStringFields(24-36)MockTestNodeWithoutSecrets(39-50)typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-353)
typescript-sdk/exospherehost/models.ts (1)
python-sdk/exospherehost/models.py (1)
RetryStrategyEnum(78-89)
typescript-sdk/tests/test_signals_and_runtime_functions.test.ts (2)
typescript-sdk/exospherehost/signals.ts (2)
PruneSignal(1-20)ReQueueAfterSignal(22-44)typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-353)
typescript-sdk/tests/test_base_node.test.ts (1)
python-sdk/tests/test_base_node.py (1)
EchoNode(6-17)
typescript-sdk/tests/test_runtime_validation.test.ts (1)
typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-353)
typescript-sdk/exospherehost/runtime.ts (3)
typescript-sdk/exospherehost/logger.ts (2)
logger(70-70)error(63-67)typescript-sdk/exospherehost/utils.ts (3)
generateFlatSchema(27-50)isZodObjectSchema(5-14)isZodStringSchema(16-25)typescript-sdk/exospherehost/signals.ts (2)
PruneSignal(1-20)ReQueueAfterSignal(22-44)
typescript-sdk/tests/test_statemanager_comprehensive.test.ts (1)
typescript-sdk/exospherehost/stateManager.ts (1)
StateManager(10-134)
typescript-sdk/exospherehost/stateManager.ts (1)
typescript-sdk/exospherehost/models.ts (6)
GraphNodeModel(18-65)GraphNodeModel(67-67)RetryPolicyModel(83-89)RetryPolicyModel(91-91)StoreConfigModel(94-157)StoreConfigModel(159-159)
typescript-sdk/tests/test_base_node_abstract.test.ts (1)
typescript-sdk/exospherehost/utils.ts (1)
isZodObjectSchema(5-14)
typescript-sdk/tests/test_integration.test.ts (3)
python-sdk/tests/test_integration.py (3)
IntegrationTestNode(42-59)MultiOutputNode(62-74)ErrorProneNode(77-90)typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-353)typescript-sdk/exospherehost/stateManager.ts (1)
StateManager(10-134)
typescript-sdk/tests/test_runtime_comprehensive.test.ts (1)
typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-353)
typescript-sdk/tests/test_models_and_statemanager_new.test.ts (1)
typescript-sdk/exospherehost/models.ts (6)
GraphNodeModel(18-65)GraphNodeModel(67-67)StoreConfigModel(94-157)StoreConfigModel(159-159)RetryPolicyModel(83-89)RetryPolicyModel(91-91)
typescript-sdk/tests/test_base_node_comprehensive.test.ts (1)
typescript-sdk/exospherehost/utils.ts (2)
isZodObjectSchema(5-14)isZodStringSchema(16-25)
🔇 Additional comments (21)
typescript-sdk/tests/README.md (2)
7-14: Confirmed — BaseNode test files exist and match the README. Found under typescript-sdk/tests: test_base_node.test.ts, test_base_node_abstract.test.ts, test_base_node_comprehensive.test.ts; each contains describe/it test blocks.
17-25: Incorrect — no casing mismatch found.
README liststest_statemanager_comprehensive.test.tsand that file exists at typescript-sdk/tests/test_statemanager_comprehensive.test.ts; imports reference ../exospherehost/stateManager.js (typescript-sdk/exospherehost/stateManager.ts) consistently.Likely an incorrect or invalid review comment.
typescript-sdk/vitest.config.ts (3)
3-15: Baseline config looks good.Globals + node env + V8 coverage with text/json/html reporters is a solid starting point.
1-3: No action required — no TS path aliases detected.
typescript-sdk/tsconfig.json contains no compilerOptions.paths, so adding vite-tsconfig-paths to vitest config is unnecessary.
8-13: Coverage config: enableall, add thresholds, and confirm glob scope
- Verification inconclusive: the provided script showed 1 TS file matched exospherehost/**/*.ts and then failed to list non-included files (comm: /dev/fd/63). Confirm whether other SDK folders (models/, runtime/, stateManager/, node/, etc.) contain TS sources that should be covered; if so apply the diff below.
coverage: { provider: 'v8', reporter: ['text', 'json', 'html'], - include: ['exospherehost/**/*.ts'], - exclude: ['exospherehost/**/*.d.ts', 'tests/**/*'] + include: ['exospherehost/**/*.ts'], + exclude: [ + 'exospherehost/**/*.d.ts', + 'tests/**/*', + '**/*.test.ts', + '**/*.spec.ts', + '**/vitest.config.*', + 'dist/**' + ], + all: true, + thresholds: { statements: 80, branches: 70, functions: 80, lines: 80 }, + reportsDirectory: 'coverage' }typescript-sdk/exospherehost/node/index.ts (1)
1-1: LGTMRe-export is correct and matches the public surface expectations.
typescript-sdk/tests/test_base_node.test.ts (1)
5-21: LGTMCovers the core _execute path and state capture on the instance.
Also applies to: 23-34
typescript-sdk/package.json (1)
14-17: Make zod a peer dependency (avoid duplicate instances) and keep as devDep for building testsAligns with ecosystem practice and avoids schema identity mismatches for users defining nodes.
"dependencies": { - "zod": "^3.23.8", "zod-to-json-schema": "^3.23.3" }, "devDependencies": { + "zod": "^3.23.8", "@types/node": "^20.14.11", "@vitest/coverage-v8": "^1.6.0", "typescript": "^5.6.3", "vitest": "^1.6.0" }, "peerDependencies": { - "@types/node": ">=20.0.0" + "@types/node": ">=20.0.0", + "zod": "^3.23.8" }Also applies to: 18-23, 24-26
typescript-sdk/README.md (3)
23-36: Add requirednamespacein GraphNode examplesThe schema requires
namespace; example would fail parsing otherwise.const nodes: GraphNode[] = [ { node_name: 'Start', identifier: 'start-node', + namespace: 'my-namespace', inputs: {}, next_nodes: ['end-node'] }, { node_name: 'End', identifier: 'end-node', + namespace: 'my-namespace', inputs: {}, next_nodes: [] } ];
45-46: Pass only inputs totrigger
triggerexpectsRecord<string,string>inputs, not the whole TriggerState.-await sm.trigger('sample-graph', trigger); +await sm.trigger('sample-graph', trigger.inputs);
54-57: Include the Secrets generic when extending BaseNode for clarityImproves readability and avoids relying on defaults.
-class ExampleNode extends BaseNode<typeof ExampleNode.Inputs, typeof ExampleNode.Outputs> { +class ExampleNode extends BaseNode< + typeof ExampleNode.Inputs, + typeof ExampleNode.Outputs, + typeof ExampleNode.Secrets +> {typescript-sdk/tests/test_runtime_comprehensive.test.ts (1)
80-83: Null output turns into [null] payloadRuntime wraps non-arrays in an array; null becomes [null]. Confirm contract: if “no outputs” is valid, normalize to [] before notifyExecuted; otherwise enforce schema and throw. Align test with the decided behavior.
typescript-sdk/tests/test_base_node_comprehensive.test.ts (2)
183-192: Strengthen null-output assertion.Also assert Outputs schema isn’t parsed when result is null (post‑fix when BaseNode adds null to return type).
- expect(result).toBeNull(); + expect(result).toBeNull(); + // ensure no array wrapping or schema parsing occurred + expect(Array.isArray(result)).toBe(false);
2-2: ESM import extension sanity-check — confirm .js resolution in teststypesript-sdk/tests/test_base_node_comprehensive.test.ts imports ../exospherehost/node/BaseNode.js. I inspected tsconfig(s): one is module: ES2020 + moduleResolution: node; another is module: esnext + moduleResolution: bundler — neither uses "NodeNext". Confirm Vitest/Vite (the test runner) resolves .js imports at test time, or change the import (omit .js) or use a tsconfig with moduleResolution: "NodeNext" and appropriate ESM runtime config (package.json "type":"module" or test-runner ESM settings).
typescript-sdk/exospherehost/node/BaseNode.ts (2)
11-15: Constructor guard LGTM.
17-35: Include null in return types; remove unsafeas anycast.Aligns with prior feedback; improves type safety for nodes legitimately returning no outputs.
- async _execute(inputsRaw: unknown, secretsRaw: unknown): Promise<z.infer<O> | z.infer<O>[]> { + async _execute(inputsRaw: unknown, secretsRaw: unknown): Promise<z.infer<O> | z.infer<O>[] | null> { @@ - if (result === null) { - return null as any; - } + if (result === null) { + return null; + } @@ - abstract execute(): Promise<z.infer<O> | z.infer<O>[]>; + abstract execute(): Promise<z.infer<O> | z.infer<O>[] | null>;typescript-sdk/exospherehost/models.ts (2)
19-23: Prefertrim().min(1)over transform+refine chain.Cleaner and equivalent validation.
- node_name: z.string() - .min(1, 'Node name cannot be empty') - .transform((val: string) => val.trim()) - .refine((val: string) => val.length > 0, 'Node name cannot be empty') + node_name: z.string().trim().min(1, 'Node name cannot be empty') @@ - identifier: z.string() - .min(1, 'Node identifier cannot be empty') - .transform((val: string) => val.trim()) - .refine((val: string) => val.length > 0, 'Node identifier cannot be empty') + identifier: z.string().trim().min(1, 'Node identifier cannot be empty')Also applies to: 25-31
124-157: Remove redundantString(value)cast; prefersuperRefinefor errors.- default_values: z.record(z.string()) - .transform((values: Record<string, string>) => { + default_values: z.record(z.string()) + .transform((values: Record<string, string>) => { const errors: string[] = []; const keySet = new Set<string>(); const normalizedDict: Record<string, string> = {}; @@ - keySet.add(trimmedKey); - normalizedDict[trimmedKey] = String(value); + keySet.add(trimmedKey); + normalizedDict[trimmedKey] = value; } @@ return normalizedDict; })Optional: replace the throw-based error collection with
.superRefineprior to transform for better issue reporting.typescript-sdk/exospherehost/runtime.ts (1)
151-154: Log response body, not[object Response], on register failure.Same as prior feedback.
- if (!res.ok) { - logger.error('Runtime', `Failed to register nodes: ${res}`); - throw new Error(`Failed to register nodes: ${res}`); - } + if (!res.ok) { + const errorText = await res.text(); + logger.error('Runtime', `Failed to register nodes: ${errorText}`); + throw new Error(`Failed to register nodes: ${errorText}`); + }typescript-sdk/exospherehost/stateManager.ts (2)
40-48: Use default parameters in signature instead of in-body undefined checks.Cleaner and makes intent explicit.
- async trigger( - graphName: string, - inputs?: Record<string, string>, - store?: Record<string, string>, - startDelay: number = 0 - ): Promise<unknown> { - if (inputs === undefined) inputs = {}; - if (store === undefined) store = {}; + async trigger( + graphName: string, + inputs: Record<string, string> = {}, + store: Record<string, string> = {}, + startDelay: number = 0 + ): Promise<unknown> {
92-103: Remove Pythonicmodel_dumpbranches; Zod outputs are plain objects.Unnecessary checks add noise.
- const body: any = { - secrets, - nodes: graphNodes.map(node => typeof node === 'object' && 'model_dump' in node ? (node as any).model_dump() : node) - }; + const body: any = { + secrets, + nodes: graphNodes + }; @@ - if (retryPolicy !== undefined) { - body.retry_policy = typeof retryPolicy === 'object' && 'model_dump' in retryPolicy ? (retryPolicy as any).model_dump() : retryPolicy; - } - if (storeConfig !== undefined) { - body.store_config = typeof storeConfig === 'object' && 'model_dump' in storeConfig ? (storeConfig as any).model_dump() : storeConfig; - } + if (retryPolicy !== undefined) body.retry_policy = retryPolicy; + if (storeConfig !== undefined) body.store_config = storeConfig;
| export * from './types.js'; | ||
| export * from './models.js'; | ||
| export * from './stateManager.js'; | ||
| export * from './node/index.js'; | ||
| export * from './runtime.js'; | ||
| export * from './signals.js'; | ||
| export * from './logger.js'; | ||
| export * from './utils.js'; |
There was a problem hiding this comment.
🧹 Nitpick
LGTM for public barrel.
Re-exports look coherent across types, models, runtime, signals, and utils.
Ensure package.json “exports” and “types” map to built ESM/CJS (e.g., dist/index.js + dist/index.d.ts) so consumers can import { Runtime } from '@exosphere/sdk' without pathing to files.
🤖 Prompt for AI Agents
In typescript-sdk/exospherehost/index.ts lines 1-8, the barrel exports are
correct but package.json currently may not point consumers to the built ESM/CJS
artifacts; update package.json so the package "exports" and type entry point map
to the compiled outputs (e.g., point "module" or appropriate export conditions
to dist/index.js for ESM, "main" or commonjs export to dist/index.cjs or
dist/index.js for CJS, and "types" to dist/index.d.ts), ensure the build step
emits those files (adjust tsconfig to emitDeclarationOnly or declaration: true
and set outDir to dist), and add proper export conditions in "exports" for "./"
and "./package.json" so consumers can import { Runtime } from '@exosphere/sdk'
without deep paths.
| describe('test_version_in_package_init', () => { | ||
| it('should expose version in package', () => { | ||
| // Test that version is properly exposed in package | ||
| expect(true).toBe(true); // Placeholder test | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Rename suite and assert public API surface.
TS has no “package init”; check the public entry exports version.
Apply this diff:
-describe('test_version_in_package_init', () => {
- it('should expose version in package', () => {
- // Test that version is properly exposed in package
- expect(true).toBe(true); // Placeholder test
- });
-});
+describe('version: public API exposure', () => {
+ it('should expose `version` on the SDK surface', () => {
+ expect(Object.prototype.hasOwnProperty.call(sdk, 'version')).toBe(true);
+ });
+});📝 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.
| describe('test_version_in_package_init', () => { | |
| it('should expose version in package', () => { | |
| // Test that version is properly exposed in package | |
| expect(true).toBe(true); // Placeholder test | |
| }); | |
| }); | |
| describe('version: public API exposure', () => { | |
| it('should expose `version` on the SDK surface', () => { | |
| expect(Object.prototype.hasOwnProperty.call(sdk, 'version')).toBe(true); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_version.test.ts around lines 27-32, the test suite
name and assertion are placeholders; rename the suite to something like "version
export" and replace the placeholder assertion with a concrete check of the
public API: import the package.json version (from the repo root) and import the
SDK's public export (the package entry point) that should expose "version", then
assert that the exported version exists, is a string, and equals the
package.json version (e.g. expect(exported.version).toBeDefined(); expect(typeof
exported.version).toBe('string');
expect(exported.version).toBe(packageJson.version)).
| "target": "ES2020", | ||
| "module": "ES2020", | ||
| "moduleResolution": "node", | ||
| "declaration": true, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adopt ESM-friendly resolution to match .js extension imports in TS
Current "moduleResolution":"node" + ESM can mis-typecheck imports like './Foo.js' to TS sources. Prefer Bundler (for Vitest/Vite) or NodeNext.
"compilerOptions": {
- "target": "ES2020",
- "module": "ES2020",
- "moduleResolution": "node",
+ "target": "ES2022",
+ "module": "ES2022",
+ "moduleResolution": "Bundler",
+ "verbatimModuleSyntax": true,
"declaration": true,
"rootDir": "exospherehost",
"outDir": "dist",
"strict": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
- "skipLibCheck": true
+ "skipLibCheck": true,
+ "sourceMap": true,
+ "declarationMap": true
},Also applies to: 8-12
🤖 Prompt for AI Agents
In typescript-sdk/tsconfig.json around lines 3-6 (and similarly lines 8-12), the
current "moduleResolution": "node" can mis-typecheck ESM-style imports that
include .js extensions; change moduleResolution to an ESM-friendly option such
as "bundler" (recommended for Vite/Vitest) or "nodenext" to correctly resolve
'./Foo.js' imports, and update the same setting in the other block mentioned
(lines 8-12) so both compiler configurations use the ESM-aware resolver.
| "include": ["exospherehost/**/*"] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Consider including tests in tsconfig for type-check only
If you want tests type-checked (no emit), add a separate tsconfig or a script using tsc --noEmit over tests too. Keeps runtime tsconfig clean.
🤖 Prompt for AI Agents
In typescript-sdk/tsconfig.json around lines 14-15 the project tsconfig
currently includes only "exospherehost/**/*" and the reviewer suggests
type-checking tests separately; create a new tsconfig for tests (e.g.,
tsconfig.tests.json) that extends ./tsconfig.json, set "noEmit": true and
include your test folders (e.g., "tests/**/*" or "src/**/*.spec.ts"), and add an
npm script (e.g., "typecheck:tests" running tsc -p tsconfig.tests.json or use
tsc --noEmit against test files) so tests are type-checked without changing the
runtime tsconfig.
| export default defineConfig({ | ||
| test: { |
There was a problem hiding this comment.
Set an explicit Vitest project root (monorepo safety).
When run from the repo root, Vitest may resolve globs relative to CWD, skewing test/coverage paths. Pin root to the config’s directory.
Apply this diff:
+import { fileURLToPath } from 'node:url';
import { defineConfig } from 'vitest/config';
export default defineConfig({
+ root: fileURLToPath(new URL('.', import.meta.url)),
test: {📝 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.
| export default defineConfig({ | |
| test: { | |
| import { fileURLToPath } from 'node:url'; | |
| import { defineConfig } from 'vitest/config'; | |
| export default defineConfig({ | |
| root: fileURLToPath(new URL('.', import.meta.url)), | |
| test: { |
🤖 Prompt for AI Agents
In typescript-sdk/vitest.config.ts around lines 3-4, the Vitest config needs an
explicit root pinned to the config file’s directory so globs and coverage
resolve correctly; add a root property set to the config directory (e.g., root:
path.resolve(__dirname)) and ensure you import path (or, if using ESM, compute
__dirname via fileURLToPath/dirname) so Vitest always resolves relative to the
config file rather than the repo CWD.
| test: { | ||
| globals: true, | ||
| environment: 'node', | ||
| include: ['tests/**/*.test.ts'], |
There was a problem hiding this comment.
🧹 Nitpick
*Widen test globs to catch .spec.ts as well.
Prevents missed tests if contributors use the common .spec.ts suffix.
Apply this diff:
- include: ['tests/**/*.test.ts'],
+ include: ['tests/**/*.{test,spec}.ts'],📝 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.
| include: ['tests/**/*.test.ts'], | |
| include: ['tests/**/*.{test,spec}.ts'], |
🤖 Prompt for AI Agents
In typescript-sdk/vitest.config.ts around line 7, the include glob currently
only matches tests/**/*.test.ts and will miss files using the common .spec.ts
suffix; update the include entry to also match .spec.ts (e.g., change the
pattern to tests/**/*.test.ts and tests/**/*.spec.ts or use a combined glob like
tests/**/*.{test,spec}.ts) so both .test.ts and .spec.ts files are discovered by
Vitest.
| export class Logger { | ||
| private static instance: Logger; | ||
| private level: LogLevel; | ||
| private isDisabled: boolean; | ||
|
|
||
| private constructor() { | ||
| this.level = this.getLogLevelFromEnv(); | ||
| this.isDisabled = process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING === 'true'; | ||
| } | ||
|
|
||
| public static getInstance(): Logger { | ||
| if (!Logger.instance) { | ||
| Logger.instance = new Logger(); | ||
| } | ||
| return Logger.instance; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Expose runtime controls for tests and dynamic configs
Provide setLevel/setDisabled to avoid relying on process.env at import-time only.
private level: LogLevel;
private isDisabled: boolean;
@@
}
+ public setLevel(level: LogLevel) { this.level = level; }
+ public setDisabled(disabled: boolean) { this.isDisabled = disabled; }📝 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.
| export class Logger { | |
| private static instance: Logger; | |
| private level: LogLevel; | |
| private isDisabled: boolean; | |
| private constructor() { | |
| this.level = this.getLogLevelFromEnv(); | |
| this.isDisabled = process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING === 'true'; | |
| } | |
| public static getInstance(): Logger { | |
| if (!Logger.instance) { | |
| Logger.instance = new Logger(); | |
| } | |
| return Logger.instance; | |
| } | |
| export class Logger { | |
| private static instance: Logger; | |
| private level: LogLevel; | |
| private isDisabled: boolean; | |
| private constructor() { | |
| this.level = this.getLogLevelFromEnv(); | |
| this.isDisabled = process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING === 'true'; | |
| } | |
| public setLevel(level: LogLevel) { this.level = level; } | |
| public setDisabled(disabled: boolean) { this.isDisabled = disabled; } | |
| public static getInstance(): Logger { | |
| if (!Logger.instance) { | |
| Logger.instance = new Logger(); | |
| } | |
| return Logger.instance; | |
| } | |
| } |
🤖 Prompt for AI Agents
In typescript-sdk/exospherehost/logger.ts around lines 8 to 23, the Logger
currently reads log level and disabled flag only at construction from
process.env, preventing tests and runtime code from adjusting them; add public
setLevel(level: LogLevel) and setDisabled(disabled: boolean) methods that update
the instance's level and isDisabled fields at runtime, keep the constructor
behavior of initializing from env, and ensure getInstance continues to return
the single Logger so callers (including tests) can call setLevel/setDisabled to
alter behavior without modifying environment variables.
| private constructor() { | ||
| this.level = this.getLogLevelFromEnv(); | ||
| this.isDisabled = process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING === 'true'; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Boolean env parsing is too strict
Support common true/false forms (true/1/yes/on).
Apply this diff:
- this.isDisabled = process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING === 'true';
+ const v = (process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING || '').toString().toLowerCase();
+ this.isDisabled = ['true','1','yes','on'].includes(v);📝 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.
| private constructor() { | |
| this.level = this.getLogLevelFromEnv(); | |
| this.isDisabled = process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING === 'true'; | |
| } | |
| private constructor() { | |
| this.level = this.getLogLevelFromEnv(); | |
| const v = (process.env.EXOSPHERE_DISABLE_DEFAULT_LOGGING || '').toString().toLowerCase(); | |
| this.isDisabled = ['true','1','yes','on'].includes(v); | |
| } |
| private formatMessage(level: string, name: string, message: string): string { | ||
| const timestamp = new Date().toISOString().replace('T', ' ').substring(0, 19); | ||
| return `${timestamp} | ${level} | ${name} | ${message}`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Preserve milliseconds in timestamps
Current substring drops ms. Keep full ISO timestamp for better tracing.
- const timestamp = new Date().toISOString().replace('T', ' ').substring(0, 19);
- return `${timestamp} | ${level} | ${name} | ${message}`;
+ const timestamp = new Date().toISOString(); // includes milliseconds, UTC
+ return `${timestamp} | ${level} | ${name} | ${message}`;📝 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.
| private formatMessage(level: string, name: string, message: string): string { | |
| const timestamp = new Date().toISOString().replace('T', ' ').substring(0, 19); | |
| return `${timestamp} | ${level} | ${name} | ${message}`; | |
| } | |
| private formatMessage(level: string, name: string, message: string): string { | |
| const timestamp = new Date().toISOString(); // includes milliseconds, UTC | |
| return `${timestamp} | ${level} | ${name} | ${message}`; | |
| } |
🤖 Prompt for AI Agents
In typescript-sdk/exospherehost/logger.ts around lines 40 to 43, the timestamp
generation currently truncates milliseconds via substring(0,19); remove the
substring call so the full ISO timestamp (including milliseconds) is
preserved—e.g., use new Date().toISOString().replace('T', ' ') (or otherwise
avoid slicing) and return the formatted string as before.
| public debug(name: string, message: string): void { | ||
| if (this.shouldLog(LogLevel.DEBUG)) { | ||
| console.debug(this.formatMessage('DEBUG', name, message)); | ||
| } | ||
| } | ||
|
|
||
| public info(name: string, message: string): void { | ||
| if (this.shouldLog(LogLevel.INFO)) { | ||
| console.info(this.formatMessage('INFO', name, message)); | ||
| } | ||
| } | ||
|
|
||
| public warn(name: string, message: string): void { | ||
| if (this.shouldLog(LogLevel.WARN)) { | ||
| console.warn(this.formatMessage('WARN', name, message)); | ||
| } | ||
| } | ||
|
|
||
| public error(name: string, message: string): void { | ||
| if (this.shouldLog(LogLevel.ERROR)) { | ||
| console.error(this.formatMessage('ERROR', name, message)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Allow non-string payloads and Error objects
Broaden message type and include stack traces automatically.
Apply this diff:
- public debug(name: string, message: string): void {
+ private toText(payload: unknown): string {
+ if (payload instanceof Error) return payload.stack || payload.message;
+ if (typeof payload === 'string') return payload;
+ try { return JSON.stringify(payload); } catch { return String(payload); }
+ }
+
+ public debug(name: string, message: unknown): void {
if (this.shouldLog(LogLevel.DEBUG)) {
- console.debug(this.formatMessage('DEBUG', name, message));
+ console.debug(this.formatMessage('DEBUG', name, this.toText(message)));
}
}
- public info(name: string, message: string): void {
+ public info(name: string, message: unknown): void {
if (this.shouldLog(LogLevel.INFO)) {
- console.info(this.formatMessage('INFO', name, message));
+ console.info(this.formatMessage('INFO', name, this.toText(message)));
}
}
- public warn(name: string, message: string): void {
+ public warn(name: string, message: unknown): void {
if (this.shouldLog(LogLevel.WARN)) {
- console.warn(this.formatMessage('WARN', name, message));
+ console.warn(this.formatMessage('WARN', name, this.toText(message)));
}
}
- public error(name: string, message: string): void {
+ public error(name: string, message: unknown): void {
if (this.shouldLog(LogLevel.ERROR)) {
- console.error(this.formatMessage('ERROR', name, message));
+ console.error(this.formatMessage('ERROR', name, this.toText(message)));
}
}📝 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.
| public debug(name: string, message: string): void { | |
| if (this.shouldLog(LogLevel.DEBUG)) { | |
| console.debug(this.formatMessage('DEBUG', name, message)); | |
| } | |
| } | |
| public info(name: string, message: string): void { | |
| if (this.shouldLog(LogLevel.INFO)) { | |
| console.info(this.formatMessage('INFO', name, message)); | |
| } | |
| } | |
| public warn(name: string, message: string): void { | |
| if (this.shouldLog(LogLevel.WARN)) { | |
| console.warn(this.formatMessage('WARN', name, message)); | |
| } | |
| } | |
| public error(name: string, message: string): void { | |
| if (this.shouldLog(LogLevel.ERROR)) { | |
| console.error(this.formatMessage('ERROR', name, message)); | |
| } | |
| } | |
| private toText(payload: unknown): string { | |
| if (payload instanceof Error) return payload.stack || payload.message; | |
| if (typeof payload === 'string') return payload; | |
| try { return JSON.stringify(payload); } catch { return String(payload); } | |
| } | |
| public debug(name: string, message: unknown): void { | |
| if (this.shouldLog(LogLevel.DEBUG)) { | |
| console.debug(this.formatMessage('DEBUG', name, this.toText(message))); | |
| } | |
| } | |
| public info(name: string, message: unknown): void { | |
| if (this.shouldLog(LogLevel.INFO)) { | |
| console.info(this.formatMessage('INFO', name, this.toText(message))); | |
| } | |
| } | |
| public warn(name: string, message: unknown): void { | |
| if (this.shouldLog(LogLevel.WARN)) { | |
| console.warn(this.formatMessage('WARN', name, this.toText(message))); | |
| } | |
| } | |
| public error(name: string, message: unknown): void { | |
| if (this.shouldLog(LogLevel.ERROR)) { | |
| console.error(this.formatMessage('ERROR', name, this.toText(message))); | |
| } | |
| } |
🤖 Prompt for AI Agents
In typescript-sdk/exospherehost/logger.ts around lines 45 to 67, the log methods
only accept string messages and therefore drop non-string payloads and Error
stack traces; change the public debug/info/warn/error signatures to accept a
broader type (e.g., unknown | Error), then normalize the payload before passing
to formatMessage: if the value is an Error, use its stack (fallback to message),
if it's a non-string object, JSON.stringify it (with safe/circular handling or a
replacer), and otherwise convert toString(); update formatMessage to accept a
string already-normalized message and ensure the Error stack gets logged
automatically.
| .transform((nodes: string[]) => nodes.map((node: string) => node.trim())) | ||
| .refine((nodes: string[]) => { | ||
| const errors: string[] = []; | ||
| const identifiers = new Set<string>(); | ||
|
|
||
| for (const node of nodes) { | ||
| if (node === '') { | ||
| errors.push('Next node identifier cannot be empty'); | ||
| continue; | ||
| } | ||
| if (identifiers.has(node)) { | ||
| errors.push(`Next node identifier ${node} is not unique`); | ||
| continue; | ||
| } | ||
| identifiers.add(node); | ||
| } | ||
|
|
||
| if (errors.length > 0) { | ||
| throw new Error(errors.join('\n')); | ||
| } | ||
| return nodes; | ||
| }) | ||
| .optional() |
There was a problem hiding this comment.
Use superRefine for per-item errors; avoid throwing inside refine.
Throwing breaks Zod’s issue reporting. Return issues via context instead.
- next_nodes: z.array(z.string())
- .transform((nodes: string[]) => nodes.map((node: string) => node.trim()))
- .refine((nodes: string[]) => {
- const errors: string[] = [];
- const identifiers = new Set<string>();
-
- for (const node of nodes) {
- if (node === '') {
- errors.push('Next node identifier cannot be empty');
- continue;
- }
- if (identifiers.has(node)) {
- errors.push(`Next node identifier ${node} is not unique`);
- continue;
- }
- identifiers.add(node);
- }
-
- if (errors.length > 0) {
- throw new Error(errors.join('\n'));
- }
- return nodes;
- })
+ next_nodes: z.array(z.string())
+ .transform((nodes: string[]) => nodes.map((node: string) => node.trim()))
+ .superRefine((nodes, ctx) => {
+ const seen = new Set<string>();
+ nodes.forEach((node, idx) => {
+ if (node === '') {
+ ctx.addIssue({ code: z.ZodIssueCode.custom, message: 'Next node identifier cannot be empty', path: [idx] });
+ } else if (seen.has(node)) {
+ ctx.addIssue({ code: z.ZodIssueCode.custom, message: `Next node identifier '${node}' is not unique`, path: [idx] });
+ } else {
+ seen.add(node);
+ }
+ });
+ })Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In typescript-sdk/exospherehost/models.ts around lines 33 to 55, the current
.refine implementation throws an Error for per-item validation which breaks
Zod's issue reporting; replace .refine with .superRefine((nodes, ctx) => { ...
}) and inside iterate trimmed nodes, for each empty or duplicate node call
ctx.addIssue(...) with appropriate message and path (index) instead of pushing
to an errors array or throwing, then return nothing (Zod collects issues); keep
the .transform and .optional chaining intact.
| const count = parseInt(this.inputs.count); | ||
| return Array.from({ length: count }, (_, i) => ({ numbers: i.toString() })); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
parseInt without radix
Add radix to avoid locale-dependent parsing.
Apply this diff:
- const count = parseInt(this.inputs.count);
+ const count = parseInt(this.inputs.count, 10);📝 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.
| const count = parseInt(this.inputs.count); | |
| return Array.from({ length: count }, (_, i) => ({ numbers: i.toString() })); | |
| } | |
| const count = parseInt(this.inputs.count, 10); | |
| return Array.from({ length: count }, (_, i) => ({ numbers: i.toString() })); | |
| } |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_runtime_comprehensive.test.ts around lines 41 to
43, parseInt is called without a radix which can lead to locale-dependent
parsing; update the call to include an explicit radix (e.g.,
parseInt(this.inputs.count, 10)) so the input is always parsed as base 10 and
ensure any other parseInt uses in the file also include the radix.
| const runtime = new Runtime('test_namespace', 'test_runtime', [MockTestNode]); | ||
|
|
||
| await expect((runtime as any).register()).rejects.toThrow('Failed to register nodes'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Assertion on error message is brittle
Use regex to match the prefix so runtime can include details (status/text).
Apply this diff:
- await expect((runtime as any).register()).rejects.toThrow('Failed to register nodes');
+ await expect((runtime as any).register()).rejects.toThrow(/Failed to register nodes/);📝 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.
| const runtime = new Runtime('test_namespace', 'test_runtime', [MockTestNode]); | |
| await expect((runtime as any).register()).rejects.toThrow('Failed to register nodes'); | |
| }); | |
| const runtime = new Runtime('test_namespace', 'test_runtime', [MockTestNode]); | |
| await expect((runtime as any).register()).rejects.toThrow(/Failed to register nodes/); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_runtime_comprehensive.test.ts around lines 201 to
204, the test currently asserts the exact error message 'Failed to register
nodes' which is brittle; update the expectation to use a regex that matches the
error prefix (e.g., /^Failed to register nodes/) so the test will accept
additional details like status or text appended to the message; change the
assertion to expect(...).rejects.toThrow(/regex/) accordingly.
| const workerPromise = (runtime as any).worker(1); | ||
|
|
||
| // Wait a bit for processing | ||
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
|
|
||
| // Verify secrets were fetched | ||
| expect((runtime as any).getSecrets).toHaveBeenCalledWith('test_state_1'); | ||
|
|
||
| // Verify execution was notified | ||
| expect((runtime as any).notifyExecuted).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Infinite worker loop in tests can leak async tasks
worker() is an endless loop. Prefer driving a single iteration via a helper or mock stateQueue.get() to resolve once and then return a never-resolving promise, and ensure the test runner doesn’t await the worker. Alternatively, introduce an abort signal into Runtime for testability.
Example change (within the test) to constrain to one iteration:
- const workerPromise = (runtime as any).worker(1);
+ const originalGet = (runtime as any).stateQueue.get.bind((runtime as any).stateQueue);
+ const getSpy = vi.spyOn((runtime as any).stateQueue, 'get')
+ .mockImplementationOnce(originalGet)
+ .mockImplementation(() => new Promise(() => {})); // park forever after one item
+ (runtime as any).worker(1); // don't awaitAlso applies to: 296-301, 319-324, 342-347
| vi.spyOn(runtime as any, 'register').mockResolvedValue({ status: 'registered' }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Unused spies
Spying on register() here has no effect. Remove to reduce noise.
- vi.spyOn(runtime as any, 'register').mockResolvedValue({ status: 'registered' });Also applies to: 310-311, 333-334
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_runtime_comprehensive.test.ts around lines 287-288
(and similarly at 310-311 and 333-334), the vi.spyOn(runtime as any,
'register').mockResolvedValue(...) calls are unused and add noise; remove those
spy/mock lines from those locations so the tests no longer create unnecessary
spies, ensuring any required behavior is covered by the existing test setup or
by targeted spies elsewhere.
| it('should raise error for duplicate node names', () => { | ||
| class GoodNode1 extends BaseNode { | ||
| static Inputs = z.object({ name: z.string() }); | ||
| static Outputs = z.object({ message: z.string() }); | ||
| static Secrets = z.object({ api_key: z.string() }); | ||
| async execute() { return { message: 'ok' }; } | ||
| } | ||
|
|
||
| class GoodNode2 extends BaseNode { | ||
| static Inputs = z.object({ name: z.string() }); | ||
| static Outputs = z.object({ message: z.string() }); | ||
| static Secrets = z.object({ api_key: z.string() }); | ||
| async execute() { return { message: 'ok' }; } | ||
| } | ||
|
|
||
| // Use the same name for both classes | ||
| Object.defineProperty(GoodNode2, 'name', { value: 'GoodNode1' }); | ||
|
|
||
| expect(() => { | ||
| new Runtime('ns', 'rt', [GoodNode1, GoodNode2]); | ||
| }).toThrow(); | ||
| }); |
There was a problem hiding this comment.
Fix duplicate-class-name test — you can’t reliably overwrite name on classes.
Overwriting Function.prototype.name is non-writable in many runtimes; this can throw. Use named class expressions with the same internal name instead.
- class GoodNode1 extends BaseNode {
+ const GoodNode1 = class GoodNodeDup extends BaseNode {
static Inputs = z.object({ name: z.string() });
static Outputs = z.object({ message: z.string() });
static Secrets = z.object({ api_key: z.string() });
async execute() { return { message: 'ok' }; }
- }
+ };
- class GoodNode2 extends BaseNode {
+ const GoodNode2 = class GoodNodeDup extends BaseNode {
static Inputs = z.object({ name: z.string() });
static Outputs = z.object({ message: z.string() });
static Secrets = z.object({ api_key: z.string() });
async execute() { return { message: 'ok' }; }
- }
-
- // Use the same name for both classes
- Object.defineProperty(GoodNode2, 'name', { value: 'GoodNode1' });
+ };
expect(() => {
new Runtime('ns', 'rt', [GoodNode1, GoodNode2]);
}).toThrow();📝 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.
| it('should raise error for duplicate node names', () => { | |
| class GoodNode1 extends BaseNode { | |
| static Inputs = z.object({ name: z.string() }); | |
| static Outputs = z.object({ message: z.string() }); | |
| static Secrets = z.object({ api_key: z.string() }); | |
| async execute() { return { message: 'ok' }; } | |
| } | |
| class GoodNode2 extends BaseNode { | |
| static Inputs = z.object({ name: z.string() }); | |
| static Outputs = z.object({ message: z.string() }); | |
| static Secrets = z.object({ api_key: z.string() }); | |
| async execute() { return { message: 'ok' }; } | |
| } | |
| // Use the same name for both classes | |
| Object.defineProperty(GoodNode2, 'name', { value: 'GoodNode1' }); | |
| expect(() => { | |
| new Runtime('ns', 'rt', [GoodNode1, GoodNode2]); | |
| }).toThrow(); | |
| }); | |
| it('should raise error for duplicate node names', () => { | |
| const GoodNode1 = class GoodNodeDup extends BaseNode { | |
| static Inputs = z.object({ name: z.string() }); | |
| static Outputs = z.object({ message: z.string() }); | |
| static Secrets = z.object({ api_key: z.string() }); | |
| async execute() { return { message: 'ok' }; } | |
| }; | |
| const GoodNode2 = class GoodNodeDup extends BaseNode { | |
| static Inputs = z.object({ name: z.string() }); | |
| static Outputs = z.object({ message: z.string() }); | |
| static Secrets = z.object({ api_key: z.string() }); | |
| async execute() { return { message: 'ok' }; } | |
| }; | |
| expect(() => { | |
| new Runtime('ns', 'rt', [GoodNode1, GoodNode2]); | |
| }).toThrow(); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_runtime_validation.test.ts around lines 126-147,
the test attempts to overwrite a class's name via Object.defineProperty which is
non-writable in many runtimes and can throw; replace that hack by creating
GoodNode2 as a named class expression that uses the same internal class name as
GoodNode1 (e.g., const GoodNode2 = class GoodNode1 extends BaseNode { ... }) so
both classes share the same internal name without touching
Function.prototype.name, keeping the exported variable name GoodNode2 and
leaving the rest of the test unchanged.
…larity and functionality - Added Zod dependency to both devDependencies and peerDependencies in package.json for schema validation. - Expanded README.md to provide a comprehensive overview of the ExosphereHost TypeScript SDK, including installation instructions, key features, and usage examples. - Improved error handling in the Runtime class by logging detailed error messages when node registration fails.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
typescript-sdk/README.md(1 hunks)typescript-sdk/exospherehost/runtime.ts(1 hunks)typescript-sdk/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
typescript-sdk/exospherehost/runtime.ts (3)
typescript-sdk/exospherehost/logger.ts (2)
logger(70-70)error(63-67)typescript-sdk/exospherehost/utils.ts (3)
generateFlatSchema(27-50)isZodObjectSchema(5-14)isZodStringSchema(16-25)typescript-sdk/exospherehost/signals.ts (2)
PruneSignal(1-20)ReQueueAfterSignal(22-44)
🪛 markdownlint-cli2 (0.18.1)
typescript-sdk/README.md
391-391: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
509-509: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (14)
typescript-sdk/package.json (3)
6-7: Ensure dist entry points exist before publish (build via lifecycle).Add “prepare”/“prepublishOnly” (above) so dist/index.{js,d.ts} exists for consumers.
#!/bin/bash # Verify manifest and artifacts (no build/run). set -euo pipefail sed -n '1,160p' typescript-sdk/package.json echo "---- dist contents ----" if [ -d typescript-sdk/dist ]; then ls -la typescript-sdk/dist; else echo "MISSING: typescript-sdk/dist"; fi echo "---- aggregator ----" if [ -f typescript-sdk/exospherehost/index.ts ]; then sed -n '1,160p' typescript-sdk/exospherehost/index.ts; else echo "MISSING: exospherehost/index.ts"; fi
1-27: Harden package for publish/consumption: exports map, files, license, engines, sideEffects, prepare/prepublish, optional @types/node peer, typecheck scriptPrevents deep imports, ensures build artifacts ship, and documents runtime requirements. Also marks @types/node peer as optional.
{ "name": "exospherehost", "version": "0.1.0", "description": "Official TypeScript SDK for ExosphereHost", "type": "module", "main": "dist/index.js", "types": "dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + } + }, + "files": [ + "dist", + "README.md", + "LICENSE" + ], + "license": "MIT", + "sideEffects": false, + "engines": { + "node": ">=18.17" + }, "scripts": { "build": "tsc", + "typecheck": "tsc -p tsconfig.json --noEmit", + "prepare": "npm run build", + "prepublishOnly": "npm run typecheck && npm run build && npm run test:run", "test": "vitest", "test:run": "vitest run", "test:coverage": "vitest run --coverage" }, "dependencies": { "zod-to-json-schema": "^3.23.3" }, "devDependencies": { "@types/node": "^20.14.11", "@vitest/coverage-v8": "^1.6.0", "typescript": "^5.6.3", "vitest": "^1.6.0", "zod": "^3.23.8" }, "peerDependencies": { "@types/node": ">=20.0.0", "zod": "^3.23.8" + }, + "peerDependenciesMeta": { + "@types/node": { + "optional": true + } } }
2-2: Package name "exospherehost" appears available on npm (verified)npm registry returned HTTP 404 for https://registry.npmjs.org/exospherehost, indicating the name is currently unregistered. File: typescript-sdk/package.json contains "name": "exospherehost".
typescript-sdk/exospherehost/runtime.ts (8)
151-155: Good fix: log response body on registration failuresUsing res.text() improves observability.
51-56: Stabilize node identifier against bundler name-manglingPrefer explicit static NodeName when present; fall back to .name.
-type NodeCtor = (new () => BaseNode<any, any, any>) & { +type NodeCtor = (new () => BaseNode<any, any, any>) & { Inputs: ZodObject<any>; Outputs: ZodObject<any>; Secrets: ZodObject<any>; - name: string; + name: string; + NodeName?: string; };
69-89: Build mapping with canonical namesUse NodeName??name for nodeMapping/nodeNames.
) { @@ - this.stateQueue = new AsyncQueue<StateItem>(2 * this.batchSize); - this.nodeMapping = Object.fromEntries(nodes.map(n => [n.name, n])); - this.nodeNames = nodes.map(n => n.name); + this.stateQueue = new AsyncQueue<StateItem>(2 * this.batchSize); + const getName = (n: NodeCtor) => n.NodeName ?? n.name; + this.nodeMapping = Object.fromEntries(nodes.map(n => [getName(n), n])); + this.nodeNames = nodes.map(getName); @@ logger.debug('Runtime', `Initialized runtime with namespace: ${this.namespace}, name: ${this.name}, nodes: ${this.nodeNames.join(', ')}`);
320-325: Don’t send [null]; treat null/undefined as no outputsPrevents API receiving [null].
- const outArray = Array.isArray(outputs) ? outputs : [outputs]; + const outArray = + outputs == null ? [] : (Array.isArray(outputs) ? outputs : [outputs]); await this.notifyExecuted(state.state_id, outArray);
345-352: “Started workers” is unreachable; move log before awaitPromise.all never resolves under normal operation.
- const poller = this.enqueue(); - const workers = Array.from({ length: this.workers }, (_, idx) => this.worker(idx)); - await Promise.all([poller, ...workers]); - logger.info('Runtime', `Started workers: ${this.workers}`); + const poller = this.enqueue(); + const workers = Array.from({ length: this.workers }, (_, idx) => this.worker(idx)); + logger.info('Runtime', `Starting workers: ${this.workers}`); + await Promise.all([poller, ...workers]); // runs until shutdown
128-141: Register using canonical names (NodeName??name)Ensures registration/enqueue/dispatch use a consistent identifier.
- const nodeNames = this.nodes.map(node => `${this.namespace}/${node.name}`); + const nodeNames = this.nodes.map(node => `${this.namespace}/${(node.NodeName ?? node.name)}`); @@ - nodes: this.nodes.map(node => ({ - name: node.name, + nodes: this.nodes.map(node => ({ + name: node.NodeName ?? node.name, inputs_schema: generateFlatSchema(node.Inputs, 'Inputs'), outputs_schema: generateFlatSchema(node.Outputs, 'Outputs'), secrets: Object.keys((node.Secrets as ZodObject<any>).shape) }))
23-49: AsyncQueue capacity is unused; enforce backpressureWithout enforcing capacity, enqueue can grow unbounded under slow consumers.
class AsyncQueue<T> { private items: T[] = []; - private resolvers: ((value: T) => void)[] = []; + private resolvers: ((value: T) => void)[] = []; + private pendingPuts: (() => void)[] = []; constructor(private capacity: number) {} @@ - async put(item: T) { - if (this.resolvers.length) { + async put(item: T) { + if (this.items.length >= this.capacity && this.resolvers.length === 0) { + await new Promise<void>(resolve => this.pendingPuts.push(resolve)); + } + if (this.resolvers.length) { const resolve = this.resolvers.shift()!; resolve(item); } else { this.items.push(item); } } @@ async get(): Promise<T> { if (this.items.length) { - return this.items.shift()!; + const it = this.items.shift()!; + const release = this.pendingPuts.shift(); + if (release) release(); + return it; } return new Promise<T>(resolve => { this.resolvers.push(resolve); }); } }
246-290: Duplicate-name check should use canonical nameAvoid false negatives if NodeName differs.
- const names = this.nodes.map(n => n.name); + const names = this.nodes.map(n => n.NodeName ?? n.name);typescript-sdk/README.md (3)
21-23: Document Node.js requirement and env-var defaults in InstallationMake runtime expectations explicit (Node 18+ for global fetch).
## Installation ```bash npm install exospherehost
+Requirements:
+- Node.js >= 18.17 (built-in fetch) or provide a fetch polyfill.
+Optional environment variables (can also be passed via options):
+- EXOSPHERE_STATE_MANAGER_URI
+- EXOSPHERE_API_KEY--- `63-76`: **runtime.start() is long-running; note for CLI usage** Avoid accidental indefinite awaits in scripts. ```diff -await runtime.start(); +// Long-running: consider not awaiting in short-lived scripts +await runtime.start();
37-61: Show explicit generics when extending BaseNodeImproves clarity of Input/Output/Secrets types.
-class SampleNode extends BaseNode { +class SampleNode extends BaseNode< + typeof SampleNode.Inputs, + typeof SampleNode.Outputs, + typeof SampleNode.Secrets +> {
|
|
||
| private needSecrets(node: typeof BaseNode) { | ||
| return Object.keys((node.Secrets as ZodObject<any>).shape).length > 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Type tightening: accept constructor type
Optional, but aligns with usage in needSecrets/new.
- private needSecrets(node: typeof BaseNode) {
+ private needSecrets(node: NodeCtor) {
return Object.keys((node.Secrets as ZodObject<any>).shape).length > 0;
}📝 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.
| private needSecrets(node: typeof BaseNode) { | |
| return Object.keys((node.Secrets as ZodObject<any>).shape).length > 0; | |
| } | |
| private needSecrets(node: NodeCtor) { | |
| return Object.keys((node.Secrets as ZodObject<any>).shape).length > 0; | |
| } |
| # ExosphereHost TypeScript SDK | ||
|
|
||
| [](https://badge.fury.io/js/exospherehost) | ||
| [](https://nodejs.org/) | ||
| [](https://www.typescriptlang.org/) | ||
| [](https://opensource.org/licenses/MIT) |
There was a problem hiding this comment.
🧹 Nitpick
Optional: document stable NodeName for bundlers
Matches runtime change to prefer static NodeName over class name.
class GreetingNode extends BaseNode {
- static name = 'greeting';
+ static name = 'greeting';
+ // Optional: stable identifier if minifiers mangle names
+ static NodeName = 'greeting';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In typescript-sdk/README.md around lines 1–6, add a short section documenting
the new runtime behavior that prefers a static NodeName over using the class
name so bundlers can reliably identify node types; state that Node
implementations should export or declare a stable static NodeName property (and
show the intended usage pattern and example value), explain how bundlers should
reference that static property instead of relying on class.name, and add a brief
compatibility note about older code that relied on class names and how to
migrate.
| // Create a single trigger state | ||
| const triggerState = { | ||
| identifier: "user-login", | ||
| inputs: { | ||
| "user_id": "12345", | ||
| "session_token": "abc123def456", | ||
| "timestamp": "2024-01-15T10:30:00Z" | ||
| } | ||
| }; | ||
|
|
||
| // Trigger the graph | ||
| const result = await stateManager.trigger( | ||
| "my-graph", | ||
| { | ||
| "user_id": "12345", | ||
| "session_token": "abc123def456" | ||
| }, | ||
| { | ||
| "cursor": "0" // persisted across nodes | ||
| } | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick
Remove unused triggerState example or hook it up
Currently defined then unused; can confuse users.
-// Create a single trigger state
-const triggerState = {
- identifier: "user-login",
- inputs: {
- "user_id": "12345",
- "session_token": "abc123def456",
- "timestamp": "2024-01-15T10:30:00Z"
- }
-};
-
// Trigger the graph
const result = await stateManager.trigger(
"my-graph",
{
"user_id": "12345",
"session_token": "abc123def456"
},
{
"cursor": "0" // persisted across nodes
}
);📝 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.
| // Create a single trigger state | |
| const triggerState = { | |
| identifier: "user-login", | |
| inputs: { | |
| "user_id": "12345", | |
| "session_token": "abc123def456", | |
| "timestamp": "2024-01-15T10:30:00Z" | |
| } | |
| }; | |
| // Trigger the graph | |
| const result = await stateManager.trigger( | |
| "my-graph", | |
| { | |
| "user_id": "12345", | |
| "session_token": "abc123def456" | |
| }, | |
| { | |
| "cursor": "0" // persisted across nodes | |
| } | |
| ); | |
| ``` | |
| // Trigger the graph | |
| const result = await stateManager.trigger( | |
| "my-graph", | |
| { | |
| "user_id": "12345", | |
| "session_token": "abc123def456" | |
| }, | |
| { | |
| "cursor": "0" // persisted across nodes | |
| } | |
| ); |
🤖 Prompt for AI Agents
In typescript-sdk/README.md around lines 407 to 428, the example defines a const
triggerState that is never used which can confuse readers; either remove the
unused triggerState block entirely, or update the example to pass its values
into the trigger call (e.g., use triggerState.inputs or merge triggerState into
the second argument) so the variable is consumed and the example demonstrates a
real flow.
- Introduced a new workflow to automate the build, test, and publish process for the TypeScript SDK on npm. - Configured jobs for testing, publishing, and creating GitHub releases, including steps for dependency installation, TypeScript compilation, and test coverage reporting. - Enhanced package.json with prepublish and prepack scripts for better version management. - Updated .gitignore to exclude additional build artifacts and logs. - Added version management scripts for easier version bumping (beta, patch, minor, major). - Adjusted test expectations to handle null results appropriately.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish-typescript-sdk.yml (1)
160-185: Optional: Prefer maintained release action and attach artifacts.Consider
softprops/action-gh-release@v1and attach SBOM/vuln report for traceability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/publish-typescript-sdk.yml(1 hunks)typescript-sdk/.gitignore(1 hunks)typescript-sdk/exospherehost/node/BaseNode.ts(1 hunks)typescript-sdk/package.json(1 hunks)typescript-sdk/scripts/version.bat(1 hunks)typescript-sdk/scripts/version.js(1 hunks)typescript-sdk/tests/test_base_node_comprehensive.test.ts(1 hunks)typescript-sdk/tests/test_runtime_comprehensive.test.ts(1 hunks)typescript-sdk/tests/test_signals_and_runtime_functions.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
typescript-sdk/tests/test_base_node_comprehensive.test.ts (1)
typescript-sdk/exospherehost/utils.ts (2)
isZodObjectSchema(5-14)isZodStringSchema(16-25)
typescript-sdk/tests/test_signals_and_runtime_functions.test.ts (2)
typescript-sdk/exospherehost/signals.ts (2)
PruneSignal(1-20)ReQueueAfterSignal(22-44)typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-357)
typescript-sdk/tests/test_runtime_comprehensive.test.ts (1)
typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-357)
🪛 YAMLlint (1.37.1)
.github/workflows/publish-typescript-sdk.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
🔇 Additional comments (20)
typescript-sdk/.gitignore (2)
1-5: Deduplicate and normalize directory ignoresYou already include the normalized entries with trailing slashes. Remove the redundant/unnecessary variants to avoid drift.
-node_modules -Dist -dist
6-15: Add a few standard ignores (env, caches, editor, more logs)These are common for TS SDKs and prevent accidental commits of secrets and local noise.
# build cache / type info *.tsbuildinfo # coverage / vitest coverage/ .vitest/ .vite/ # logs & OS junk npm-debug.log* yarn-error.log* .DS_Store +*.log +pnpm-debug.log* + +# env files (keep .env.example committed if present) +.env +.env.local +.env.*.local + +# editor/caches +.eslintcache +.cache/ +.idea/ +.vscode/ + +# nyc/istanbul output +.nyc_output/ + +# Windows +Thumbs.dbtypescript-sdk/tests/test_signals_and_runtime_functions.test.ts (3)
28-37: Add per-test mock reset in TestPruneSignal.Ensure isolation; clear fetch mocks before each test in this suite.
describe('TestPruneSignal', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); it('should initialize with data', () => {
82-91: Add per-test mock reset in TestReQueueAfterSignal.Same isolation need as other suites.
describe('TestReQueueAfterSignal', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); it('should initialize with delay', () => {
82-91: Also assert the interpolated delay value is present.Improve the message assertion by checking the numeric delay.
it('should initialize with delay', () => { const delayMs = 30000; const signal = new ReQueueAfterSignal(delayMs); expect(signal.delayMs).toBe(delayMs); expect(signal.message).toContain('ReQueueAfter signal received with delay'); + expect(signal.message).toContain(`${delayMs}ms`); expect(signal.message).toContain('Do not catch this Exception'); });typescript-sdk/exospherehost/node/BaseNode.ts (2)
17-34: Type contract: allow null instead of returning {}.Returning
{}for null results is misleading and skirts type safety. Includenullin the return types and propagate it.- async _execute(inputsRaw: unknown, secretsRaw: unknown): Promise<z.infer<O> | z.infer<O>[]> { + async _execute(inputsRaw: unknown, secretsRaw: unknown): Promise<z.infer<O> | z.infer<O>[] | null> { @@ - if (result === null) { - return {} as z.infer<O>; - } + if (result === null) { + return null; + } return outputsSchema.parse(result); } @@ - abstract execute(): Promise<z.infer<O> | z.infer<O>[]>; + abstract execute(): Promise<z.infer<O> | z.infer<O>[] | null>;Follow-ups:
- Update tests expecting
{}for “no output” to expectnull.- In runtime, handle
nullby skipping notify or sending an empty outputs array (confirm expected API behavior).
Run repo-wide checks to find affected call sites.#!/bin/bash rg -nP --type=ts '\._execute\(' -C2
17-24: Guard against reentrancy on the same instance.Concurrent
_executecalls can race onthis.inputs/this.secrets. Serialize executions on an instance.export abstract class BaseNode<I extends ZodObject<any> = ZodObject<any>, O extends ZodObject<any> = ZodObject<any>, S extends ZodObject<any> = ZodObject<any>> { @@ protected secrets!: z.infer<S>; + private _executing = false; @@ - async _execute(inputsRaw: unknown, secretsRaw: unknown): Promise<z.infer<O> | z.infer<O>[]> { + async _execute(inputsRaw: unknown, secretsRaw: unknown): Promise<z.infer<O> | z.infer<O>[] | null> { + if (this._executing) { + throw new Error('BaseNode instances are not re-entrant; create a new instance per execution.'); + } + this._executing = true; const ctor = this.constructor as typeof BaseNode; const inputs = (ctor.Inputs as I).parse(inputsRaw); const secrets = (ctor.Secrets as S).parse(secretsRaw); this.inputs = inputs; this.secrets = secrets; - const result = await this.execute(); - const outputsSchema = ctor.Outputs as O; - if (Array.isArray(result)) { - return result.map(r => outputsSchema.parse(r)); - } - if (result === null) { - return {} as z.infer<O>; - } - return outputsSchema.parse(result); + try { + const result = await this.execute(); + const outputsSchema = ctor.Outputs as O; + if (Array.isArray(result)) { + return result.map(r => outputsSchema.parse(r)); + } + if (result === null) { + return null; + } + return outputsSchema.parse(result); + } finally { + this._executing = false; + } }Also applies to: 31-34
typescript-sdk/tests/test_runtime_comprehensive.test.ts (5)
6-8: Centralize global.fetch mocking and reset lifecycle.Avoid per-file assignment; move to tests/setup.ts and reset between tests to prevent interference.
-// Mock fetch globally -global.fetch = vi.fn(); +// tests/setup.ts +import { vi } from 'vitest'; +global.fetch = vi.fn();Then add in suites:
beforeEach(() => { vi.clearAllMocks(); });
41-42: Specify radix for parseInt.Make parsing deterministic.
- const count = parseInt(this.inputs.count); + const count = parseInt(this.inputs.count, 10);
201-204: Brittle error assertion; use regex.The error includes server text; match the prefix.
- await expect((runtime as any).register()).rejects.toThrow('Failed to register nodes'); + await expect((runtime as any).register()).rejects.toThrow(/Failed to register nodes/);
267-273: Worker is an infinite loop; constrain to one iteration in tests.Don’t await the loop and park after one state to avoid async leaks.
- // Start worker and let it process one item - const workerPromise = (runtime as any).worker(1); + // Drive a single iteration, then park subsequent gets + const originalGet = (runtime as any).stateQueue.get.bind((runtime as any).stateQueue); + vi.spyOn((runtime as any).stateQueue, 'get') + .mockImplementationOnce(originalGet) + .mockImplementation(() => new Promise(() => {})); // park forever + (runtime as any).worker(1); // do not awaitApply similarly to the other worker tests in this file.
Also applies to: 296-301, 319-324, 342-347
287-288: Remove unused register() spies.These spies aren’t asserted; drop them to reduce noise.
- vi.spyOn(runtime as any, 'register').mockResolvedValue({ status: 'registered' });Also applies to: 310-311, 333-334
typescript-sdk/tests/test_base_node_comprehensive.test.ts (2)
44-46: Specify radix for parseInt.Ensure deterministic parsing.
- const count = parseInt(this.inputs.items); + const count = parseInt(this.inputs.items, 10);
344-351: Concurrent _execute on the same instance races internal state.Create a new instance per concurrent run (or add non-reentrancy guard in BaseNode as suggested).
- const node = new ValidNode(); - const inputs = { name: 'test', count: '1' }; - const secrets = { api_key: 'key', token: 'token' }; - const promises = Array.from({ length: 5 }, () => node._execute(inputs, secrets)); + const inputs = { name: 'test', count: '1' }; + const secrets = { api_key: 'key', token: 'token' }; + const promises = Array.from({ length: 5 }, () => new ValidNode()._execute(inputs, secrets));typescript-sdk/package.json (6)
64-65: Raise engines floor to Node 18.17+ (stable fetch/Web APIs)18.0 has rough edges; 18.17+ is a safer baseline for global fetch/streams.
"engines": { - "node": ">=18.0.0" + "node": ">=18.17.0" },
6-7: Ensure dist entry points exist before publish (and aggregator builds)package.json points to dist/index.{js,d.ts}; verify they are produced by tsc and included in the tarball. Add/keep "prepare" to build on git installs.
#!/bin/bash set -euo pipefail test -f dist/index.js && echo "OK: dist/index.js" || (echo "MISSING: dist/index.js" && exit 1) test -f dist/index.d.ts && echo "OK: dist/index.d.ts" || (echo "MISSING: dist/index.d.ts" && exit 1) npm pack --dry-run | sed -n '1,200p'
26-36: Peer dep setup looks good; mark @types/node as optionalKeeps flexibility for consumers without ambient Node types. Keeping zod in devDependencies alongside peerDependencies is fine.
"peerDependencies": { "@types/node": ">=20.0.0", "zod": "^3.23.8" }, + "peerDependenciesMeta": { + "@types/node": { + "optional": true + } + },
8-22: Build on git installs and add a typecheck script"prepare" builds for git-based installs; explicit "typecheck" helps CI.
"scripts": { "build": "tsc", + "typecheck": "tsc -p tsconfig.json --noEmit", "test": "vitest", "test:run": "vitest run", "test:coverage": "vitest run --coverage", - "prepublishOnly": "npm run build && npm run test:run", + "prepublishOnly": "npm run typecheck && npm run build && npm run test:run", "prepack": "npm run build", + "prepare": "npm run build", "clean": "rm -rf dist", "lint": "echo 'No linting configured'", "lint:fix": "echo 'No linting configured'",
6-7: Add an exports map and mark the package as side‑effect freeLocks public surface, blocks deep imports, and improves tree‑shaking.
"type": "module", "main": "dist/index.js", "types": "dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + } + }, + "sideEffects": false,
23-25: Move zod-to-json-schema to devDependenciesNo runtime imports/usages found in the repo; typescript-sdk/package.json currently lists "zod-to-json-schema" under dependencies. If it's only used for build/test, move it to devDependencies to avoid adding it to consumers' installs — confirm there are no dynamic/runtime imports or runtime JSON‑Schema generation first.
| publish: | ||
| runs-on: ubuntu-latest | ||
| needs: test | ||
| defaults: | ||
| run: | ||
| working-directory: typescript-sdk | ||
| if: github.repository == 'exospherehost/exospherehost' | ||
|
|
||
| steps: |
There was a problem hiding this comment.
Prevent double publishing; gate publish job to push events only.
Currently runs on release events too (and release job also publishes), risking duplicate publish failures.
publish:
@@
- if: github.repository == 'exospherehost/exospherehost'
+ if: github.event_name == 'push' && github.repository == 'exospherehost/exospherehost'📝 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.
| publish: | |
| runs-on: ubuntu-latest | |
| needs: test | |
| defaults: | |
| run: | |
| working-directory: typescript-sdk | |
| if: github.repository == 'exospherehost/exospherehost' | |
| steps: | |
| publish: | |
| runs-on: ubuntu-latest | |
| needs: test | |
| defaults: | |
| run: | |
| working-directory: typescript-sdk | |
| if: github.event_name == 'push' && github.repository == 'exospherehost/exospherehost' | |
| steps: |
🤖 Prompt for AI Agents
In .github/workflows/publish-typescript-sdk.yml around lines 66 to 74, the
publish job is currently gated only by the repository name and thus runs on
release events as well, causing duplicate publish attempts; change the job-level
if condition to only run on push events for this repository (e.g., require
github.event_name == 'push' in addition to the repository check) so the publish
job is skipped for release events and only executes on push events to
exospherehost/exospherehost.
| - name: Check version for beta indicator | ||
| run: | | ||
| VERSION=$(node -p "require('./package.json').version") | ||
| if [[ "$VERSION" == *"b"* ]]; then | ||
| echo "Version $VERSION contains beta indicator - publishing to npm with beta tag" | ||
| echo "NPM_TAG=beta" >> $GITHUB_ENV | ||
| else | ||
| echo "Version $VERSION does not contain beta indicator - publishing to npm with latest tag" | ||
| echo "NPM_TAG=latest" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
Fix beta tag detection; matching any “b” is unsafe.
Use prerelease detection consistent with semver -beta.N (after version.js refactor).
- - name: Check version for beta indicator
+ - name: Determine npm tag from semver prerelease
run: |
- VERSION=$(node -p "require('./package.json').version")
- if [[ "$VERSION" == *"b"* ]]; then
- echo "Version $VERSION contains beta indicator - publishing to npm with beta tag"
- echo "NPM_TAG=beta" >> $GITHUB_ENV
- else
- echo "Version $VERSION does not contain beta indicator - publishing to npm with latest tag"
- echo "NPM_TAG=latest" >> $GITHUB_ENV
- fi
+ VERSION=$(node -p "require('./package.json').version")
+ if [[ "$VERSION" == *"-beta."* ]]; then
+ echo "NPM_TAG=beta" >> "$GITHUB_ENV"
+ elif [[ "$VERSION" == *"-"* ]]; then
+ # Other prereleases (alpha/rc) -> publish under 'next'
+ echo "NPM_TAG=next" >> "$GITHUB_ENV"
+ else
+ echo "NPM_TAG=latest" >> "$GITHUB_ENV"
+ fi🤖 Prompt for AI Agents
.github/workflows/publish-typescript-sdk.yml around lines 92-101: the current
check looks for any "b" in the version which is too permissive; replace it with
a prerelease-aware check that matches semver beta identifiers (e.g. -beta or
-beta.N). Update the script to set VERSION=$(node -p
"require('./package.json').version") and use a bash regex such as if [[
"$VERSION" =~ -beta([[:digit:]]+)?($|\\.) ]]; then ... to detect "-beta" or
"-beta.N" (and set NPM_TAG=beta), otherwise set NPM_TAG=latest.
| - name: Publish to npm with latest tag | ||
| run: npm publish --tag latest --access public | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid republishing on release event.
Publishing already happens in publish on push; remove duplicate publish here to prevent 409 errors on identical version.
- - name: Publish to npm with latest tag
- run: npm publish --tag latest --access public
- env:
- NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
+ # Publishing happens in the 'publish' job on push. Skip duplicate publish here.📝 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.
| - name: Publish to npm with latest tag | |
| run: npm publish --tag latest --access public | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| # Publishing happens in the 'publish' job on push. Skip duplicate publish here. |
🤖 Prompt for AI Agents
.github/workflows/publish-typescript-sdk.yml around lines 154 to 158: the
workflow contains a duplicate "Publish to npm with latest tag" job step that
re-publishes on release events and can cause 409 errors for identical versions;
remove this npm publish step (the name/run/env block) from these lines so
publishing only occurs in the existing publish-on-push job, or alternatively
gate it behind a conditional that skips publishing when the version/tag already
exists.
| @@ -0,0 +1,70 @@ | |||
| { | |||
| "name": "exospherehost", | |||
There was a problem hiding this comment.
🧹 Nitpick
🧩 Analysis chain
Consider a scoped package name before first publish
Prefer "@exospherehost/sdk" (or similar) to avoid collisions and to convey ownership. Verify "exospherehost" availability on npm before committing to this unscoped name.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking npm availability for 'exospherehost'..."
npm view exospherehost name version description || echo "Looks available (no npm metadata found)."
echo "If publishing under a scope, consider: @exospherehost/sdk"Length of output: 558
Consider using a scoped package name; 'exospherehost' appears available on npm (checked 2025-09-22)
npm view returned 404, so the unscoped name looks free; prefer @exospherehost/sdk to avoid future collisions.
File: typescript-sdk/package.json (name)
🤖 Prompt for AI Agents
In typescript-sdk/package.json around line 2, the package name is currently
unscoped ("exospherehost"); change it to a scoped name (e.g.
"@exospherehost/sdk") by updating the "name" field to the chosen scope, ensure
the JSON remains valid, update any internal references or README/docs that
mention the old name, and verify npm publish settings (access/public) and
CI/publish workflows are updated to publish the scoped package.
| beta Create a beta version (e.g., 0.1.0b1, 0.1.0b2) | ||
| patch Create a patch version (e.g., 0.1.0 → 0.1.1) | ||
| minor Create a minor version (e.g., 0.1.0 → 0.2.0) | ||
| major Create a major version (e.g., 0.1.0 → 1.0.0) | ||
|
|
||
| Examples: | ||
| node scripts/version.js beta # 0.1.0 → 0.1.0b1 | ||
| node scripts/version.js patch # 0.1.0b1 → 0.1.0 | ||
| node scripts/version.js minor # 0.1.0 → 0.2.0 | ||
| node scripts/version.js major # 0.1.0 → 1.0.0 | ||
|
|
||
| Current version: ${getCurrentVersion()} |
There was a problem hiding this comment.
🧹 Nitpick
Update help text to reflect valid prerelease format.
Docs/examples should show -beta.N not bN.
- beta Create a beta version (e.g., 0.1.0b1, 0.1.0b2)
+ beta Create a beta version (e.g., 0.1.0-beta.1, 0.1.0-beta.2)
@@
- node scripts/version.js beta # 0.1.0 → 0.1.0b1
- node scripts/version.js patch # 0.1.0b1 → 0.1.0
+ node scripts/version.js beta # 0.1.0 → 0.1.0-beta.1
+ node scripts/version.js patch # 0.1.0-beta.1 → 0.1.0📝 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.
| beta Create a beta version (e.g., 0.1.0b1, 0.1.0b2) | |
| patch Create a patch version (e.g., 0.1.0 → 0.1.1) | |
| minor Create a minor version (e.g., 0.1.0 → 0.2.0) | |
| major Create a major version (e.g., 0.1.0 → 1.0.0) | |
| Examples: | |
| node scripts/version.js beta # 0.1.0 → 0.1.0b1 | |
| node scripts/version.js patch # 0.1.0b1 → 0.1.0 | |
| node scripts/version.js minor # 0.1.0 → 0.2.0 | |
| node scripts/version.js major # 0.1.0 → 1.0.0 | |
| Current version: ${getCurrentVersion()} | |
| beta Create a beta version (e.g., 0.1.0-beta.1, 0.1.0-beta.2) | |
| patch Create a patch version (e.g., 0.1.0 → 0.1.1) | |
| minor Create a minor version (e.g., 0.1.0 → 0.2.0) | |
| major Create a major version (e.g., 0.1.0 → 1.0.0) | |
| Examples: | |
| node scripts/version.js beta # 0.1.0 → 0.1.0-beta.1 | |
| node scripts/version.js patch # 0.1.0-beta.1 → 0.1.0 | |
| node scripts/version.js minor # 0.1.0 → 0.2.0 | |
| node scripts/version.js major # 0.1.0 → 1.0.0 | |
| Current version: ${getCurrentVersion()} |
🤖 Prompt for AI Agents
In typescript-sdk/scripts/version.js around lines 116 to 127, the help text and
examples currently show prerelease versions using the `bN` format (e.g.,
0.1.0b1) but should use the `-beta.N` format; update the descriptive lines and
all example strings to use `-beta.N` (e.g., 0.1.0 → 0.1.0-beta.1 and
0.1.0-beta.1 → 0.1.0) and adjust the shorthand example invocation comment
accordingly so the docs and examples consistently reflect the valid prerelease
format.
| it('should handle node with null output', async () => { | ||
| const node = new NodeWithNoneOutput(); | ||
| const inputs = { name: 'test' }; | ||
| const secrets = { api_key: 'test_key' }; | ||
|
|
||
| const result = await node._execute(inputs, secrets); | ||
|
|
||
| expect(result).toEqual({}); | ||
| expect((node as any).inputs).toEqual(inputs); | ||
| expect((node as any).secrets).toEqual(secrets); |
There was a problem hiding this comment.
🧹 Nitpick
Note on “null output” expectations.
Tests assert {} when execute() returns null. If BaseNode is updated to return null (recommended), adjust these tests to expect null.
Also applies to: 430-436
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_base_node_comprehensive.test.ts around lines
182-191 (and also update the similar assertions at lines 430-436), the test
currently expects {} when a node returns null; change the assertions to expect
null instead: update the result expectation from expect(result).toEqual({}) to
expect(result).toBeNull() (or expect(result).toEqual(null)), and keep the input
and secret checks unchanged so the test reflects BaseNode returning null.
| // Mock fetch globally | ||
| global.fetch = vi.fn(); |
There was a problem hiding this comment.
🧹 Nitpick
Centralize global.fetch mocking and reset per test.
Move the global fetch mock to a shared test setup (e.g., tests/setup.ts) and ensure beforeEach resets to avoid cross-file bleed.
Example:
-// Mock fetch globally
-global.fetch = vi.fn();
+// tests/setup.ts
+import { vi } from 'vitest';
+global.fetch = vi.fn();
+// vitest.config.ts
+test: { setupFiles: ['tests/setup.ts'] }And add per-suite:
beforeEach(() => { vi.clearAllMocks(); });🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around lines
7-8, the global.fetch mock is declared directly in this test file causing
cross-file bleed; move the global.fetch = vi.fn() into a shared setup file
(e.g., tests/setup.ts) that is registered in the test runner config, remove the
direct mock from this test, and ensure each suite calls beforeEach(() => {
vi.clearAllMocks(); }) (or add a global beforeEach in tests/setup.ts) so mocks
are reset between tests.
| it('should handle send failure', async () => { | ||
| (global.fetch as any).mockResolvedValueOnce({ | ||
| ok: false | ||
| }); | ||
|
|
||
| const data = { reason: 'test_prune' }; | ||
| const signal = new PruneSignal(data); | ||
|
|
||
| await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow('Failed to send prune signal'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Make failure assertion resilient to appended details.
The thrown message includes the endpoint; match by regex.
- await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow('Failed to send prune signal');
+ await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow(/Failed to send prune signal/);📝 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.
| it('should handle send failure', async () => { | |
| (global.fetch as any).mockResolvedValueOnce({ | |
| ok: false | |
| }); | |
| const data = { reason: 'test_prune' }; | |
| const signal = new PruneSignal(data); | |
| await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow('Failed to send prune signal'); | |
| }); | |
| it('should handle send failure', async () => { | |
| (global.fetch as any).mockResolvedValueOnce({ | |
| ok: false | |
| }); | |
| const data = { reason: 'test_prune' }; | |
| const signal = new PruneSignal(data); | |
| await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow(/Failed to send prune signal/); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around lines
70 to 79, the test asserts the exact thrown message 'Failed to send prune
signal' but the implementation appends details like the endpoint, so update the
expectation to match the error using a regex (e.g. /Failed to send prune
signal/) instead of exact string equality; change the
expect(...).rejects.toThrow call to use a regex matcher that will pass whether
or not extra context is appended.
| it('should handle send failure', async () => { | ||
| (global.fetch as any).mockResolvedValueOnce({ | ||
| ok: false | ||
| }); | ||
|
|
||
| const delayMs = 30000; | ||
| const signal = new ReQueueAfterSignal(delayMs); | ||
|
|
||
| await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow('Failed to send requeue after signal'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Make failure assertion resilient to appended details.
Same reason as prune failure path.
- await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow('Failed to send requeue after signal');
+ await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow(/Failed to send requeue after signal/);📝 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.
| it('should handle send failure', async () => { | |
| (global.fetch as any).mockResolvedValueOnce({ | |
| ok: false | |
| }); | |
| const delayMs = 30000; | |
| const signal = new ReQueueAfterSignal(delayMs); | |
| await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow('Failed to send requeue after signal'); | |
| }); | |
| it('should handle send failure', async () => { | |
| (global.fetch as any).mockResolvedValueOnce({ | |
| ok: false | |
| }); | |
| const delayMs = 30000; | |
| const signal = new ReQueueAfterSignal(delayMs); | |
| await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow(/Failed to send requeue after signal/); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around lines
137 to 146, the test asserts the exact error message on send failure which can
break if additional details are appended; change the assertion to be resilient
by expecting a partial match (e.g., use toThrow with a RegExp or
toThrowErrorMatchingInlineSnapshot that matches /Failed to send requeue after
signal/) so the test passes even when extra context is added to the error
message.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
typescript-sdk/exospherehost/types.ts(1 hunks)typescript-sdk/tests/test_models_and_statemanager_new.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
typescript-sdk/tests/test_models_and_statemanager_new.test.ts (1)
typescript-sdk/exospherehost/models.ts (6)
GraphNodeModel(18-65)GraphNodeModel(67-67)StoreConfigModel(94-157)StoreConfigModel(159-159)RetryPolicyModel(83-89)RetryPolicyModel(91-91)
🔇 Additional comments (3)
typescript-sdk/exospherehost/types.ts (1)
6-12: Past mismatch resolved: next_nodes is optional.This aligns the interface with the model schema. Good catch.
typescript-sdk/tests/test_models_and_statemanager_new.test.ts (2)
25-36: Avoidnullfor optional fields to keep failures deterministic.Passing
nullto optional fields (next_nodes,unites) triggers type errors that can mask the intended validation. Omit these keys or use empty values as appropriate.Apply this diff:
@@ it('should validate node name cannot be empty', () => { expect(() => { GraphNodeModel.parse({ node_name: ' ', namespace: 'ns', identifier: 'id1', inputs: {}, - next_nodes: null, - unites: null + // omit optional fields to isolate the node_name failure + // next_nodes omitted + // unites omitted }); }).toThrow('Node name cannot be empty'); }); @@ GraphNodeModel.parse({ node_name: 'n', namespace: 'ns', identifier: 'id1', inputs: {}, - next_nodes: ['', 'id2'], - unites: null + next_nodes: ['', 'id2'] + // unites omitted }); }).toThrow('cannot be empty'); @@ GraphNodeModel.parse({ node_name: 'n', namespace: 'ns', identifier: 'id1', inputs: {}, - next_nodes: ['dup', 'dup'], - unites: null + next_nodes: ['dup', 'dup'] + // unites omitted }); }).toThrow('not unique'); @@ GraphNodeModel.parse({ node_name: 'n', namespace: 'ns', identifier: 'id1', inputs: {}, - next_nodes: null, + // next_nodes omitted unites: { identifier: ' ' } }); }).toThrow('Unites identifier cannot be empty');Also applies to: 58-60, 71-73, 84-86
25-36: Make Zod error assertions robust with safeParse + issues inspection.String-matching thrown messages is brittle. Prefer
safeParseand assert onerror.issues(message + path).Example refactors:
@@ - it('should validate node name cannot be empty', () => { - expect(() => { - GraphNodeModel.parse({ - node_name: ' ', - namespace: 'ns', - identifier: 'id1', - inputs: {}, - // optional fields omitted per previous comment - }); - }).toThrow('Node name cannot be empty'); - }); + it('should validate node name cannot be empty', () => { + const result = GraphNodeModel.safeParse({ + node_name: ' ', + namespace: 'ns', + identifier: 'id1', + inputs: {} + }); + expect(result.success).toBe(false); + expect(result.error!.issues).toEqual( + expect.arrayContaining([ + expect.objectContaining({ message: 'Node name cannot be empty', path: ['node_name'] }) + ]) + ); + }); @@ - it('should validate identifier is not reserved word', () => { - expect(() => { - GraphNodeModel.parse({ - node_name: 'n', - namespace: 'ns', - identifier: 'store', - inputs: {}, - next_nodes: [], - unites: undefined - }); - }).toThrow('reserved word'); - }); + it('should validate identifier is not reserved word', () => { + const result = GraphNodeModel.safeParse({ + node_name: 'n', + namespace: 'ns', + identifier: 'store', + inputs: {}, + next_nodes: [], + unites: undefined + }); + expect(result.success).toBe(false); + expect(result.error!.issues).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: "Node identifier cannot be reserved word 'store'", + path: ['identifier'] + }) + ]) + ); + }); @@ - it('should validate duplicated keys', () => { - expect(() => { - StoreConfigModel.parse({ - required_keys: ['a', 'a'] - }); - }).toThrow('duplicated'); - }); + it('should validate duplicated keys', () => { + const result = StoreConfigModel.safeParse({ required_keys: ['a', 'a'] }); + expect(result.success).toBe(false); + expect(result.error!.issues.some(i => /duplicated/.test(i.message) && i.path[0] === 'required_keys')).toBe(true); + });If you prefer to keep
toThrow, at least use a regex that matches the actual message, e.g.toThrow(/Node identifier .* reserved word 'store'/).Also applies to: 38-49, 51-62, 64-75, 77-88, 102-107, 110-116, 118-124, 126-132, 134-141
| export interface TriggerState { | ||
| identifier: string; | ||
| inputs: Record<string, string>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Unify inputs typing across SDK; prefer a JSONValue.
GraphNode.inputs allows unknown while TriggerState.inputs forces string. This divergence leaks into the public API and complicates reuse. Recommend a single JSON-serializable value type used in both.
+export type JSONValue =
+ | string
+ | number
+ | boolean
+ | null
+ | { [key: string]: JSONValue }
+ | JSONValue[];
+
export interface TriggerState {
identifier: string;
- inputs: Record<string, string>;
+ inputs: Record<string, JSONValue>;
}
@@
export interface GraphNode {
node_name: string;
identifier: string;
- inputs: Record<string, unknown>;
+ inputs: Record<string, JSONValue>;
next_nodes?: string[];
namespace?: string;
}Please confirm other modules don’t rely on TriggerState.inputs being strictly string.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== typescript-sdk/exospherehost/types.ts (first 200 lines) ==="
sed -n '1,200p' typescript-sdk/exospherehost/types.ts || true
echo
echo "=== rg: usages of 'TriggerState' (TypeScript files, context) ==="
rg -n --type=ts -C2 '\bTriggerState\b' || true
echo
echo "=== rg: usages of 'GraphNode' (TypeScript files, context) ==="
rg -n --type=ts -C2 '\bGraphNode\b' || true
echo
echo "=== rg: occurrences of 'Record<string, string>' (TypeScript files, context) ==="
rg -n --type=ts -C2 'Record<\s*string\s*,\s*string\s*>' || true
echo
echo "=== rg: occurrences of 'inputs' property (TypeScript files, context) ==="
rg -n --type=ts -C2 '\binputs\b' || true
echo
echo "=== rg: imports that reference exospherehost/types (TS files, context) ==="
rg -n --type=ts -C2 'exospherehost/types' || trueLength of output: 56690
Do not change TriggerState.inputs to JSONValue without a coordinated refactor — callers expect strings.
- Evidence: callers use Record<string,string> (typescript-sdk/exospherehost/stateManager.ts — trigger signature; typescript-sdk/exospherehost/runtime.ts — node state, getSecrets, and runtime pass Record<string,string>; typescript-sdk/exospherehost/models.ts — z.record(z.string()) / default_values transform; many tests under typescript-sdk/tests construct string-valued inputs).
- Action: either (A) adopt JSONValue and update all callsites, Zod schemas, models, runtime and tests, or (B) keep TriggerState.inputs as Record<string,string> and only unify types where safe.
🤖 Prompt for AI Agents
In typescript-sdk/exospherehost/types.ts around lines 1 to 4, do not change
TriggerState.inputs to a JSONValue; keep it explicitly typed as Record<string,
string> to match existing callers, Zod schemas, models and tests. If a broader
change to JSONValue is desired, coordinate a full refactor: update all
callsites, Zod schemas, models, runtime functions, and tests together —
otherwise preserve Record<string,string> and add a short inline comment
explaining why it must remain string-valued to prevent accidental mass
replacement.
| it('should validate duplicated keys', () => { | ||
| expect(() => { | ||
| StoreConfigModel.parse({ | ||
| required_keys: ['a', 'a'] | ||
| }); | ||
| }).toThrow('duplicated'); |
There was a problem hiding this comment.
🧹 Nitpick
Deduplicate negative-path store-config tests via test.each.
Consolidates repeated structure and makes adding new cases trivial.
Example:
describe('StoreConfigModel invalid cases', () => {
it.each([
[{ required_keys: ['a', 'a'] }, /duplicated/],
[{ required_keys: ['a.'] }, /cannot contain '\.'/],
[{ required_keys: [' '] }, /cannot be empty/],
[{ default_values: { 'k.k': 'v' } }, /cannot contain '\.'/],
[{ default_values: { '': 'v' } }, /cannot be empty/],
])('rejects %j', (payload, msg) => {
const res = StoreConfigModel.safeParse(payload as any);
expect(res.success).toBe(false);
expect(res.error!.issues.some(i => msg.test(i.message))).toBe(true);
});
});Also applies to: 110-116, 118-124, 126-132, 134-141
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_models_and_statemanager_new.test.ts around lines
102 to 107 (and similarly for blocks at 110-116, 118-124, 126-132, 134-141),
multiple near-identical negative-path StoreConfigModel tests should be
consolidated into a single parameterized test using test.each; replace the
repeated it(...) blocks with a describe block that uses it.each([...cases])
where each case is [payload, expectedMessageRegex], call
StoreConfigModel.safeParse(payload as any), assert res.success is false, and
assert res.error!.issues.some(i => expectedMessageRegex.test(i.message)) to
verify the correct validation error; this deduplicates tests and makes adding
new invalid cases trivial.
| describe('RetryPolicyModel defaults', () => { | ||
| it('should have correct defaults', () => { | ||
| const pol = RetryPolicyModel.parse({}); | ||
| expect(pol.max_retries).toBe(3); | ||
| expect(pol.backoff_factor).toBe(2000); | ||
| expect(pol.strategy).toBe(RetryStrategyEnum.EXPONENTIAL); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Cover all defaults: add exponent and max_delay expectations.
This documents the full default contract of RetryPolicyModel.
it('should have correct defaults', () => {
const pol = RetryPolicyModel.parse({});
expect(pol.max_retries).toBe(3);
expect(pol.backoff_factor).toBe(2000);
expect(pol.strategy).toBe(RetryStrategyEnum.EXPONENTIAL);
+ expect(pol.exponent).toBe(2);
+ expect(pol.max_delay).toBeUndefined();
});📝 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.
| describe('RetryPolicyModel defaults', () => { | |
| it('should have correct defaults', () => { | |
| const pol = RetryPolicyModel.parse({}); | |
| expect(pol.max_retries).toBe(3); | |
| expect(pol.backoff_factor).toBe(2000); | |
| expect(pol.strategy).toBe(RetryStrategyEnum.EXPONENTIAL); | |
| }); | |
| describe('RetryPolicyModel defaults', () => { | |
| it('should have correct defaults', () => { | |
| const pol = RetryPolicyModel.parse({}); | |
| expect(pol.max_retries).toBe(3); | |
| expect(pol.backoff_factor).toBe(2000); | |
| expect(pol.strategy).toBe(RetryStrategyEnum.EXPONENTIAL); | |
| expect(pol.exponent).toBe(2); | |
| expect(pol.max_delay).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_models_and_statemanager_new.test.ts around lines
143-149, the test checks only max_retries, backoff_factor, and strategy; add
assertions for exponent and max_delay to fully cover the model defaults — e.g.
after creating pol = RetryPolicyModel.parse({}) add
expect(pol.exponent).toBe(<default exponent value>) and
expect(pol.max_delay).toBe(<default max_delay value>) using the actual default
values defined on RetryPolicyModel (or import the constants) so the test
documents the complete default contract.
| it('should include store config in upsert', async () => { | ||
| // This test would require mocking the StateManager and its HTTP calls | ||
| // For now, we'll test the model parsing which is the core functionality | ||
| const storeCfg = StoreConfigModel.parse({ | ||
| required_keys: ['k1'], | ||
| default_values: { 'k2': 'v' } | ||
| }); | ||
|
|
||
| expect(storeCfg.required_keys).toEqual(['k1']); | ||
| expect(storeCfg.default_values).toEqual({ 'k2': 'v' }); | ||
| }); | ||
|
|
||
| it('should handle store in trigger', () => { | ||
| // This test would require mocking the StateManager and its HTTP calls | ||
| // For now, we'll test that the models can be parsed correctly | ||
| const storeConfig = StoreConfigModel.parse({ | ||
| required_keys: ['cursor'], | ||
| default_values: { 'cursor': '0' } | ||
| }); | ||
|
|
||
| expect(storeConfig.required_keys).toEqual(['cursor']); | ||
| expect(storeConfig.default_values).toEqual({ 'cursor': '0' }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Remove unnecessary async from non-async test.
No await is used; avoid async to prevent accidental swallowed rejections.
- it('should include store config in upsert', async () => {
+ it('should include store config in upsert', () => {📝 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.
| it('should include store config in upsert', async () => { | |
| // This test would require mocking the StateManager and its HTTP calls | |
| // For now, we'll test the model parsing which is the core functionality | |
| const storeCfg = StoreConfigModel.parse({ | |
| required_keys: ['k1'], | |
| default_values: { 'k2': 'v' } | |
| }); | |
| expect(storeCfg.required_keys).toEqual(['k1']); | |
| expect(storeCfg.default_values).toEqual({ 'k2': 'v' }); | |
| }); | |
| it('should handle store in trigger', () => { | |
| // This test would require mocking the StateManager and its HTTP calls | |
| // For now, we'll test that the models can be parsed correctly | |
| const storeConfig = StoreConfigModel.parse({ | |
| required_keys: ['cursor'], | |
| default_values: { 'cursor': '0' } | |
| }); | |
| expect(storeConfig.required_keys).toEqual(['cursor']); | |
| expect(storeConfig.default_values).toEqual({ 'cursor': '0' }); | |
| }); | |
| it('should include store config in upsert', () => { | |
| // This test would require mocking the StateManager and its HTTP calls | |
| // For now, we'll test the model parsing which is the core functionality | |
| const storeCfg = StoreConfigModel.parse({ | |
| required_keys: ['k1'], | |
| default_values: { 'k2': 'v' } | |
| }); | |
| expect(storeCfg.required_keys).toEqual(['k1']); | |
| expect(storeCfg.default_values).toEqual({ 'k2': 'v' }); | |
| }); | |
| it('should handle store in trigger', () => { | |
| // This test would require mocking the StateManager and its HTTP calls | |
| // For now, we'll test that the models can be parsed correctly | |
| const storeConfig = StoreConfigModel.parse({ | |
| required_keys: ['cursor'], | |
| default_values: { 'cursor': '0' } | |
| }); | |
| expect(storeConfig.required_keys).toEqual(['cursor']); | |
| expect(storeConfig.default_values).toEqual({ 'cursor': '0' }); | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_models_and_statemanager_new.test.ts around lines
153 to 175 the first test function is declared async but contains no await;
remove the unnecessary async keyword from the it('should include store config in
upsert', ...) declaration so the test is a regular synchronous function, then
run the test suite to ensure no behavioral changes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Downgraded the SDK version from 0.1.0 to 0.0.1 in package.json and package-lock.json for proper versioning. - Added license information and Node.js engine requirements in package-lock.json. - Updated Vitest configuration to exclude pending tests. - Refactored version script to use ES module syntax for better compatibility. - Introduced a new integration test file for comprehensive testing of runtime and state manager functionalities.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
typescript-sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/publish-typescript-sdk.yml(1 hunks)typescript-sdk/exospherehost/types.ts(1 hunks)typescript-sdk/package.json(1 hunks)typescript-sdk/scripts/version.js(1 hunks)typescript-sdk/tests/pending_test_integration.test.ts(1 hunks)typescript-sdk/tests/test_signals_and_runtime_functions.test.ts(1 hunks)typescript-sdk/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
typescript-sdk/tests/test_signals_and_runtime_functions.test.ts (2)
typescript-sdk/exospherehost/signals.ts (2)
PruneSignal(1-20)ReQueueAfterSignal(22-44)typescript-sdk/exospherehost/runtime.ts (1)
Runtime(58-357)
typescript-sdk/tests/pending_test_integration.test.ts (1)
python-sdk/tests/test_integration.py (3)
IntegrationTestNode(42-59)MultiOutputNode(62-74)ErrorProneNode(77-90)
🪛 YAMLlint (1.37.1)
.github/workflows/publish-typescript-sdk.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
🔇 Additional comments (26)
typescript-sdk/package.json (7)
31-36: zod placement looks correctPeer + dev for local builds is appropriate. Ensure consumers install a compatible zod.
2-2: Prefer a scoped package nameUse a scope to avoid collisions and signal ownership, e.g. "@exospherehost/sdk".
- "name": "exospherehost", + "name": "@exospherehost/sdk",
15-16: Make clean cross‑platformUse rimraf and declare it.
- "clean": "rm -rf dist", + "clean": "rimraf dist","devDependencies": { "@types/node": "^20.14.11", "@vitest/coverage-v8": "^1.6.0", + "rimraf": "^5.0.5", "typescript": "^5.6.3", "vitest": "^1.6.0", "zod": "^3.23.8" },Also applies to: 26-32
33-36: Mark @types/node peer as optionalReduces friction for consumers without ambient Node types.
"peerDependencies": { "@types/node": ">=20.0.0", "zod": "^3.23.8" }, + "peerDependenciesMeta": { + "@types/node": { + "optional": true + } + },
6-7: Add exports map and mark package side‑effect freePrevents deep imports and improves tree‑shaking.
"main": "dist/index.js", "types": "dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + } + }, + "sideEffects": false,
63-65: Pin engines to Node 18.17+Ensures stable, up‑to‑date WHATWG fetch/undici behavior.
"engines": { - "node": ">=18.0.0" + "node": ">=18.17" },
8-22: Add typecheck and prepare scriptsType safety in CI and auto‑builds when publishing from git.
"scripts": { "build": "tsc", + "typecheck": "tsc -p tsconfig.json --noEmit", "test": "vitest", @@ "prepack": "npm run build", + "prepare": "npm run build",typescript-sdk/exospherehost/types.ts (1)
6-12: Types look good; confirm JSON‑serializable inputs for GraphNodeIf inputs must be JSON‑safe, consider a JSONValue alias; keep TriggerState.inputs as string as callers rely on it.
Also applies to: 14-20
typescript-sdk/scripts/version.js (3)
102-104: Avoid hardcoding branch in guidanceUse a generic hint.
- console.log('3. Push to trigger publishing:'); - console.log(' git push origin main'); + console.log('3. Push to trigger publishing (adjust branch as needed):'); + console.log(' git push origin <branch>');
119-129: Fix help text to valid prerelease formatShow -beta.N examples.
- beta Create a beta version (e.g., 0.1.0b1, 0.1.0b2) + beta Create a beta version (e.g., 0.1.0-beta.1, 0.1.0-beta.2) @@ - node scripts/version.js beta # 0.1.0 → 0.1.0b1 - node scripts/version.js patch # 0.1.0b1 → 0.1.0 + node scripts/version.js beta # 0.1.0 → 0.1.0-beta.1 + node scripts/version.js patch # 0.1.0-beta.1 → 0.1.0
28-110: Blocker: invalid prerelease scheme (b1) will fail npm publish; switch to semver-beta.Nwith strict parsingCurrent split/contains logic emits non‑semver like 1.0.0b1. Replace with a semver‑aware flow.
function updateVersion(type) { const currentVersion = getCurrentVersion(); console.log(`Current version: ${currentVersion}`); - - let newVersion; - - switch (type) { - case 'beta': - // If current version already has beta, increment beta number - if (currentVersion.includes('b')) { - const [baseVersion, betaNum] = currentVersion.split('b'); - newVersion = `${baseVersion}b${parseInt(betaNum) + 1}`; - } else { - // Create first beta version - newVersion = `${currentVersion}b1`; - } - break; - - case 'patch': - if (currentVersion.includes('b')) { - // Remove beta suffix for stable release - newVersion = currentVersion.split('b')[0]; - } else { - // Increment patch version - const [major, minor, patch] = currentVersion.split('.').map(Number); - newVersion = `${major}.${minor}.${patch + 1}`; - } - break; - - case 'minor': - if (currentVersion.includes('b')) { - // Remove beta suffix and increment minor - const [major, minor] = currentVersion.split('b')[0].split('.').map(Number); - newVersion = `${major}.${minor + 1}.0`; - } else { - // Increment minor version - const [major, minor] = currentVersion.split('.').map(Number); - newVersion = `${major}.${minor + 1}.0`; - } - break; - - case 'major': - if (currentVersion.includes('b')) { - // Remove beta suffix and increment major - const major = parseInt(currentVersion.split('b')[0].split('.')[0]) + 1; - newVersion = `${major}.0.0`; - } else { - // Increment major version - const major = parseInt(currentVersion.split('.')[0]) + 1; - newVersion = `${major}.0.0`; - } - break; - - default: - console.error(`Unknown version type: ${type}`); - console.error('Valid types: beta, patch, minor, major'); - process.exit(1); - } - - console.log(`New version: ${newVersion}`); - - // Update package.json - packageJson.version = newVersion; + + const SEMVER_RE = /^(\d+)\.(\d+)\.(\d+)(?:-beta\.(\d+))?$/; + const m = currentVersion.match(SEMVER_RE); + if (!m) { + console.error(`package.json version '${currentVersion}' is not supported. Expected X.Y.Z or X.Y.Z-beta.N`); + process.exit(1); + } + let major = +m[1], minor = +m[2], patch = +m[3]; + let beta = m[4] ? +m[4] : null; + + switch (type) { + case 'beta': + beta = beta != null ? beta + 1 : 1; + break; + case 'patch': + if (beta != null) { + beta = null; // finalize prerelease + } else { + patch += 1; + } + break; + case 'minor': + minor += 1; patch = 0; beta = null; + break; + case 'major': + major += 1; minor = 0; patch = 0; beta = null; + break; + default: + console.error(`Unknown version type: ${type}`); + console.error('Valid types: beta, patch, minor, major'); + process.exit(1); + } + + const newVersion = beta != null + ? `${major}.${minor}.${patch}-beta.${beta}` + : `${major}.${minor}.${patch}`; + console.log(`New version: ${newVersion}`); + + packageJson.version = newVersion; fs.writeFileSync(packageJsonPath, JSON.stringify(packageJson, null, 2) + '\n');.github/workflows/publish-typescript-sdk.yml (3)
65-73: Gate publish job to push events to avoid double publish on releasePrevents duplicate npm publishes.
- if: github.repository == 'exospherehost/exospherehost' + if: github.event_name == 'push' && github.repository == 'exospherehost/exospherehost'
91-101: Semver‑aware npm tag detectionMatch -beta prereleases; avoid any “b”.
- - name: Check version for beta indicator + - name: Determine npm tag from semver prerelease run: | VERSION=$(node -p "require('./package.json').version") - if [[ "$VERSION" == *"b"* ]]; then - echo "Version $VERSION contains beta indicator - publishing to npm with beta tag" - echo "NPM_TAG=beta" >> $GITHUB_ENV - else - echo "Version $VERSION does not contain beta indicator - publishing to npm with latest tag" - echo "NPM_TAG=latest" >> $GITHUB_ENV - fi + if [[ "$VERSION" == *"-beta."* ]]; then + echo "NPM_TAG=beta" >> "$GITHUB_ENV" + elif [[ "$VERSION" == *"-"* ]]; then + echo "NPM_TAG=next" >> "$GITHUB_ENV" + else + echo "NPM_TAG=latest" >> "$GITHUB_ENV" + fi
153-157: Remove duplicate publish in release jobPublishing already happens in publish job.
- - name: Publish to npm with latest tag - run: npm publish --tag latest --access public - env: - NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} + # Skip npm publish here; already handled in the 'publish' job on push.typescript-sdk/vitest.config.ts (2)
1-4: Pin Vitest root (monorepo safety)Prevents CWD surprises when run from repo root.
-import { defineConfig } from 'vitest/config'; +import { fileURLToPath } from 'node:url'; +import { defineConfig } from 'vitest/config'; + +const root = fileURLToPath(new URL('.', import.meta.url)); -export default defineConfig({ +export default defineConfig({ + root,
7-7: *Discover .spec.ts tooCovers common test suffix.
- include: ['tests/**/*.test.ts'], + include: ['tests/**/*.{test,spec}.ts'],typescript-sdk/tests/pending_test_integration.test.ts (2)
118-120: Remove incorrect nodeMapping overridesnodeMapping is keyed by node class name; overriding with state_id is misleading and unnecessary.
- (runtime as any).nodeMapping['test_state_1'] = IntegrationTestNode; @@ - (runtime as any).nodeMapping[state.state_id] = MultiOutputNode; @@ - (runtime as any).nodeMapping['test_state_1'] = ErrorProneNode; @@ - (runtime as any).nodeMapping[state.state_id] = IntegrationTestNode; @@ - states.forEach(state => { - (runtime as any).nodeMapping[state.state_id] = IntegrationTestNode; - }); + // No overrides needed; runtime already maps by node classAlso applies to: 210-211, 243-244, 297-298, 401-405
128-131: Tests start endless workers; make the loop abortable or bound to one iterationAs written, worker() never resolves and can hang CI.
Suggested approaches:
- Add an AbortSignal param to Runtime.worker and pass AbortController.signal in tests; abort after first assertion.
- Or mock the poll loop (e.g., spy on get/enqueue to return once, then throw/return sentinel) and await worker exit.
- Prefer vi.useFakeTimers to drive any polling timers deterministically, combined with an exit condition.
Also applies to: 218-221, 251-254, 304-308, 416-420
typescript-sdk/tests/test_signals_and_runtime_functions.test.ts (8)
28-31: Good: per-test mock reset present.Clearing mocks in this suite prevents cross-test leakage.
233-251: LGTM:needSecretscoverage is precise.Good positive/negative assertions for presence of secrets.
496-574: Good: notifyExecuted/notifyErrored behavior tested including failure paths.Covers headers, body shape, and non-throwing failure handling.
72-81: Make failure assertion resilient to appended details.Implementation appends the endpoint to the error; prefer regex match.
- await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow('Failed to send prune signal'); + await expect(signal.send('http://test-endpoint/prune', 'test-api-key')).rejects.toThrow(/Failed to send prune signal/);
84-85: Reset mocks in ReQueueAfterSignal suite.Add per-test
vi.clearAllMocks()like other suites.describe('TestReQueueAfterSignal', () => { + beforeEach(() => { + vi.clearAllMocks(); + });
139-148: Make failure assertion resilient to appended details.Align with prune failure path; use regex.
- await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow('Failed to send requeue after signal'); + await expect(signal.send('http://test-endpoint/requeue', 'test-api-key')).rejects.toThrow(/Failed to send requeue after signal/);
89-92: Strengthen message assertions to validate format and delay interpolation.Also assert the delay appears with "ms" and the full guidance phrase.
- expect(signal.message).toContain('ReQueueAfter signal received with delay'); - expect(signal.message).toContain('Do not catch this Exception'); + expect(signal.message.startsWith('ReQueueAfter signal received with delay')).toBe(true); + expect(signal.message).toContain(`${delayMs}ms`); + expect(signal.message).toContain('Do not catch this Exception, let it bubble up to Runtime for handling at StateManager');
7-8: Centralize global.fetch mock in a shared test setup and clear mocks between tests.
- Remove the inline mock from typescript-sdk/tests/test_signals_and_runtime_functions.test.ts (lines 7–8) and move it to tests/setup.ts; register that file via vitest config's setupFiles.
- tests/setup.ts:
import { beforeEach, afterEach, vi } from 'vitest'; (globalThis as any).fetch = vi.fn(); beforeEach(() => vi.clearAllMocks()); afterEach(() => vi.clearAllMocks());
- vitest.config.ts (test): { setupFiles: ['tests/setup.ts'], environment: 'node' }
- Verify no other inline mocks remain: rg -n "global\.(fetch|this\.fetch)\s*=\s*vi\.fn\(" -g '!/node_modules/'
-// Mock fetch globally -global.fetch = vi.fn();
| const graphNodes = [{ | ||
| node_name: 'IntegrationTestNode', | ||
| namespace: 'test_namespace', | ||
| identifier: 'IntegrationTestNode', | ||
| inputs: { type: 'test' }, | ||
| next_nodes: null, | ||
| unites: null | ||
| }]; | ||
| const secrets = { api_key: 'test_key', database_url: 'db://test' }; |
There was a problem hiding this comment.
Fix graph node payload: avoid nulls for optional fields
Use undefined/omit instead of null to match schema; drop unknown fields.
- const graphNodes = [{
+ const graphNodes = [{
node_name: 'IntegrationTestNode',
namespace: 'test_namespace',
identifier: 'IntegrationTestNode',
- inputs: { type: 'test' },
- next_nodes: null,
- unites: null
+ inputs: { type: 'test' }
}];📝 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.
| const graphNodes = [{ | |
| node_name: 'IntegrationTestNode', | |
| namespace: 'test_namespace', | |
| identifier: 'IntegrationTestNode', | |
| inputs: { type: 'test' }, | |
| next_nodes: null, | |
| unites: null | |
| }]; | |
| const secrets = { api_key: 'test_key', database_url: 'db://test' }; | |
| const graphNodes = [{ | |
| node_name: 'IntegrationTestNode', | |
| namespace: 'test_namespace', | |
| identifier: 'IntegrationTestNode', | |
| inputs: { type: 'test' } | |
| }]; | |
| const secrets = { api_key: 'test_key', database_url: 'db://test' }; |
🤖 Prompt for AI Agents
In typescript-sdk/tests/pending_test_integration.test.ts around lines 163 to
171, the test graph node uses null for optional fields and includes fields not
in the schema; update the payload to omit optional fields (remove next_nodes and
unites or set them to undefined) rather than using null, and remove any unknown
fields (e.g., drop identifier if it isn't part of the schema) so the object
matches the expected schema shape.
| @@ -0,0 +1,575 @@ | |||
| import { describe, it, expect, beforeEach, vi } from 'vitest'; | |||
There was a problem hiding this comment.
🧹 Nitpick
Import afterEach to support env teardown.
You add per-suite cleanup later; import afterEach here.
-import { describe, it, expect, beforeEach, vi } from 'vitest';
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';📝 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.
| import { describe, it, expect, beforeEach, vi } from 'vitest'; | |
| import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around line
1, the test file imports Vitest helpers but omits afterEach which is needed for
environment teardown; update the import statement to include afterEach (i.e.,
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest') so the
teardown code can run.
| it('should send with minutes', async () => { | ||
| (global.fetch as any).mockResolvedValueOnce({ | ||
| ok: true | ||
| }); | ||
|
|
||
| const delayMs = 150000; // 2.5 minutes | ||
| const signal = new ReQueueAfterSignal(delayMs); | ||
|
|
||
| await signal.send('http://test-endpoint/requeue', 'test-api-key'); | ||
|
|
||
| expect(global.fetch).toHaveBeenCalledWith('http://test-endpoint/requeue', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'x-api-key': 'test-api-key', | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ enqueue_after: delayMs }) | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Deduplicate “send successfully” tests via parametrization.
Combine these two nearly identical tests using test.each to cut duplication and simplify maintenance.
Example:
it.each([
{ delayMs: 45000 },
{ delayMs: 150000 }, // 2.5 minutes
])('should send successfully with delay $delayMs', async ({ delayMs }) => {
(global.fetch as any).mockResolvedValueOnce({ ok: true });
const signal = new ReQueueAfterSignal(delayMs);
await signal.send('http://test-endpoint/requeue', 'test-api-key');
expect(global.fetch).toHaveBeenCalledWith('http://test-endpoint/requeue', {
method: 'POST',
headers: { 'x-api-key': 'test-api-key', 'Content-Type': 'application/json' },
body: JSON.stringify({ enqueue_after: delayMs }),
});
});🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around lines
119 to 137, two nearly identical "send successfully" tests should be
deduplicated by replacing them with a single parameterized test using test.each;
create an array of test cases with the different delayMs values (e.g., 45000 and
150000), call (global.fetch as any).mockResolvedValueOnce({ ok: true }) inside
the test, instantiate ReQueueAfterSignal with the current delayMs, await
signal.send(...), and assert global.fetch was called with the same URL, method,
headers and body JSON.stringify({ enqueue_after: delayMs }); remove the
duplicate individual tests and keep the single test.each variant with a
descriptive name including the delay parameter.
| describe('TestRuntimeSignalHandling', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| process.env.EXOSPHERE_STATE_MANAGER_URI = 'http://test-state-manager'; | ||
| process.env.EXOSPHERE_API_KEY = 'test-key'; | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Teardown env vars to avoid bleed across suites.
Remove test-specific ENV after each test in this suite.
beforeEach(() => {
vi.clearAllMocks();
process.env.EXOSPHERE_STATE_MANAGER_URI = 'http://test-state-manager';
process.env.EXOSPHERE_API_KEY = 'test-key';
});
+ afterEach(() => {
+ delete process.env.EXOSPHERE_STATE_MANAGER_URI;
+ delete process.env.EXOSPHERE_API_KEY;
+ });📝 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.
| describe('TestRuntimeSignalHandling', () => { | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| process.env.EXOSPHERE_STATE_MANAGER_URI = 'http://test-state-manager'; | |
| process.env.EXOSPHERE_API_KEY = 'test-key'; | |
| }); | |
| describe('TestRuntimeSignalHandling', () => { | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| process.env.EXOSPHERE_STATE_MANAGER_URI = 'http://test-state-manager'; | |
| process.env.EXOSPHERE_API_KEY = 'test-key'; | |
| }); | |
| afterEach(() => { | |
| delete process.env.EXOSPHERE_STATE_MANAGER_URI; | |
| delete process.env.EXOSPHERE_API_KEY; | |
| }); |
🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around lines
157-163, the beforeEach sets test-specific environment variables but there is no
teardown, risking bleed across suites; add an afterEach that restores or deletes
process.env.EXOSPHERE_STATE_MANAGER_URI and process.env.EXOSPHERE_API_KEY
(preferably capture originals in beforeEach and restore them, or at minimum
delete the two keys in afterEach) so tests do not leak environment state.
| it('should convert delay correctly to milliseconds', async () => { | ||
| const testCases = [ | ||
| { delayMs: 1000, expected: 1000 }, | ||
| { delayMs: 60000, expected: 60000 }, | ||
| { delayMs: 3600000, expected: 3600000 }, | ||
| { delayMs: 86400000, expected: 86400000 }, | ||
| { delayMs: 30500, expected: 30500 } // 30.5 seconds | ||
| ]; | ||
|
|
||
| for (const { delayMs, expected } of testCases) { | ||
| (global.fetch as any).mockResolvedValueOnce({ ok: true }); | ||
|
|
||
| const signal = new ReQueueAfterSignal(delayMs); | ||
| await signal.send('http://test-endpoint', 'test-key'); | ||
|
|
||
| expect(global.fetch).toHaveBeenCalledWith('http://test-endpoint', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'x-api-key': 'test-key', | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ enqueue_after: expected }) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Stabilize per-iteration assertions in the conversion loop.
Clear mocks at the start of each iteration (since you re-stub mockResolvedValueOnce) to ensure each expectation validates only the current call.
- for (const { delayMs, expected } of testCases) {
- (global.fetch as any).mockResolvedValueOnce({ ok: true });
+ for (const { delayMs, expected } of testCases) {
+ vi.clearAllMocks();
+ (global.fetch as any).mockResolvedValueOnce({ ok: true });Alternatively, assert nth call order instead of clearing:
const callIndex = i + 1;
expect(global.fetch).toHaveBeenNthCalledWith(callIndex, 'http://test-endpoint', { /* ... */ });🤖 Prompt for AI Agents
In typescript-sdk/tests/test_signals_and_runtime_functions.test.ts around lines
449 to 472, the loop re-stubs global.fetch with mockResolvedValueOnce but does
not clear or assert call index, which can make expectations flaky; either
clear/reset the mock at the start of each iteration (e.g., reset or mockClear)
so each expect checks only the current call, or compute the expected call index
and use toHaveBeenNthCalledWith for the specific iteration; update the test to
perform one of these two approaches so each assertion deterministically verifies
the intended fetch invocation.
No description provided.