Conversation
✅ Deploy Preview for taucad-staging canceled.
|
API Preview 🐙Status: ❌ Deployment Failed |
|
Important Review skippedToo many files! This PR contains 2036 files, which is 1986 over the limit of 50. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (24)
📒 Files selected for processing (2612)
You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds GeoSpec package (authoring API, runner, mesh/STEP loaders, native analyzers, CLI), wires runGeoSpecTests RPC and tool/schema changes, updates VM imports resolution, refactors testing to delegate to GeoSpec/Tau, and adds api-extractor bundling and filesystem mount support. ChangesGeoSpec end-to-end feature and integrations
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
@claude review |
|
Claude Code is working… I'll analyze this and get back to you. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/chat/src/schemas/message.schema.test.ts (1)
225-246:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the parsed
tool-test_modelpayload, not just parse success.Both additions stop at
result.success, so they still pass if the schema dropsrawInput, stripsfiles, or rewritestestNamePattern. Please inspect the parsed tool part and assert the resultinginput/rawInputshape the same way the neighboringtool-read_filecases do.As per coding guidelines,
Every it block must assert something a consumer can observeandAssert data structure shape with toEqual/toMatchObject, not just toBeDefined() or toBeGreaterThan(0).Also applies to: 392-406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/chat/src/schemas/message.schema.test.ts` around lines 225 - 246, Update the test so it not only checks result.success but also inspects the parsed message payload from uiMessagesSchema: after safeParse and expect(result.success).toBe(true), extract the parsed tool message (e.g., result.data[1] or the part matching type `tool-${toolName.testModel}` / MyMessagePart) and assert its input/rawInput shape with toEqual/toMatchObject (mirroring the neighboring tool-read_file assertions) to ensure rawInput is preserved and input/files/testNamePattern are correct; apply the same change to the analogous test block referenced (lines 392-406) so both tests validate observable parsed structure rather than only parse success.
🟡 Minor comments (10)
packages/vm/src/esbuild-core.ts-366-371 (1)
366-371:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWildcard pattern priority comparison may be incorrect.
The condition
prefix.length > best.pattern.indexOf('*')compares the current prefix length against the position of*in the best pattern, rather than comparing prefix lengths directly. This doesn't correctly implement "most specific match wins" semantics.For Node.js package imports, the most specific pattern should win (longest matching prefix). The current logic could select a less specific pattern in edge cases.
🐛 Proposed fix
- if (!best || prefix.length > best.pattern.indexOf('*') || pattern.length > best.pattern.length) { + const bestPrefixLength = best ? best.pattern.indexOf('*') : -1; + if (!best || prefix.length > bestPrefixLength) { best = { target, pattern, wildcard }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vm/src/esbuild-core.ts` around lines 366 - 371, The wildcard selection logic compares prefix.length to best.pattern.indexOf('*') which is unclear and fragile; instead compute and compare prefix lengths directly: when setting best (the object assigned to best currently containing target, pattern, wildcard) include the numeric bestPrefixLength = pattern.indexOf('*') (or store prefix.length) and then change the comparison to prefix.length > best.bestPrefixLength (or prefix.length > bestPrefixLength) || pattern.length > best.pattern.length so the "most specific (longest prefix) wins" rule is implemented reliably; update the assignments and the if condition around best, pattern, prefix, suffix, wildcard accordingly.packages/geospec/src/index.test.ts-152-198 (1)
152-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
as unknown as GeometrySubjectfrom test fixtures
packages/geospec/src/index.test.tsends themodelLoaderfixtures with} as unknown as GeometrySubjectin two places, which bypasses theGeometrySubjectcontract and can hide fixture drift. Replace these casts with a helper/fixture typed to satisfyGeometrySubjectat compile time (no type-erasing assertions).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/index.test.ts` around lines 152 - 198, The test currently masks type mismatches by returning the modelLoader fixture with a type assertion "as unknown as GeometrySubject"; remove that cast and instead construct a properly typed fixture object that satisfies the GeometrySubject interface at compile time (either by creating a helper function like buildGeometrySubjectFixture(...) or a typed constant fixture and returning it from modelLoader). Update the modelLoader implementation in packages/geospec/src/index.test.ts to return that helper/fixture (matching GeometrySubject fields such as kind, provenance, capabilities, diagnostics, mesh, and the mesh.stats methods) so the compiler enforces the contract rather than relying on a type-erasing assertion.packages/geospec/README.md-25-27 (1)
25-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the bounding-box axis order in the first example.
makeBaseBox(40, 20, 8)describes a 40×20×8 box, but the example assertsy: 0.008andz: 0.02. That swaps Y/Z and gives readers a failing reference snippet.Proposed fix
- expectGeo(model).toHaveBoundingBox({ size: { x: 0.04, y: 0.008, z: 0.02 }, tolerance: 0.001 }); + expectGeo(model).toHaveBoundingBox({ size: { x: 0.04, y: 0.02, z: 0.008 }, tolerance: 0.001 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/README.md` around lines 25 - 27, The bounding-box axis order in the README example is wrong: change the toHaveBoundingBox assertion so its size matches makeBaseBox(40, 20, 8) by swapping the Y/Z values; update the expectGeo(...).toHaveBoundingBox call (reference symbols: expectGeo, toHaveBoundingBox, makeBaseBox) to use size: { x: 0.04, y: 0.02, z: 0.008 } while leaving the tolerances unchanged.packages/geospec/README.md-119-121 (1)
119-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComplete this parameter example before publishing it.
defaultParamsis used here but never declared or imported in the snippet, so the copy-paste example does not run as written.Proposed fix
import { describe, expectGeo, it } from 'geospec'; import { loadModel, parameterGroups } from 'geospec/model'; import mainParams from '`#params/main.ts.json`' with { type: 'json' }; -const groups = parameterGroups(mainParams, { defaults: defaultParams }); +const groups = parameterGroups(mainParams);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/README.md` around lines 119 - 121, The README example uses an undeclared symbol defaultParams causing the snippet to fail; update the example to either declare or import defaultParams and show its structure, e.g., add a brief defaultParams declaration or an import before calling parameterGroups so the example is runnable: reference the existing mainParams import and the parameterGroups(...) call and ensure defaultParams is defined (or explicitly passed as undefined/null) so the snippet compiles and demonstrates the intended usage.packages/geospec/src/mesh/load-mesh.ts-113-125 (1)
113-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t default arbitrary filenames to GLB.
Any string/path that is not
.gltfcurrently falls through to'glb', somodel.stepormodel.objbypasses the typedUNSUPPORTED_MESH_FORMATbranch and later comes back as a genericMESH_LOAD_FAILED. That makes the public API much harder to diagnose for unsupported inputs.Also applies to: 250-261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/mesh/load-mesh.ts` around lines 113 - 125, inferFormat currently defaults any unknown filename to 'glb', which hides unsupported extensions; update inferFormat in packages/geospec/src/mesh/load-mesh.ts (and the similar logic at the other occurrence) to only return known formats ('mesh-buffer', 'gltf', 'glb') when the source clearly matches, and otherwise return undefined (or a dedicated unsupported sentinel) so the caller can hit the typed UNSUPPORTED_MESH_FORMAT branch; use the existing symbols inferFormat, LoadMeshOptions, isMeshBufferSource, and MeshFileFormat to locate and adjust the logic (do not silently treat unknown extensions like ".obj" or ".step" as 'glb').packages/geospec/src/mesh/distance.test.ts-39-43 (1)
39-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid regex-based assertions here.
The negative
/simpler|simplify/icheck is brittle and doesn't assert a stable consumer-visible contract. Prefer exact text or principled substring assertions on the specific guidance you expect. As per coding guidelines, "Avoid regex-based assertions in tests bound for upstream PRs — they are hacky and indefensible in review, prefer principled assertions on parsed structures".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/mesh/distance.test.ts` around lines 39 - 43, The test uses a brittle regex negative assertion on suggestion; replace it with principled substring assertions: compute suggestion and normalizedSuggestion as before (result.diagnostics[0]?.suggestion ?? '' and suggestion.toLowerCase()), then remove expect(suggestion).not.toMatch(/simpler|simplify/i) and instead add explicit negative assertions like expect(normalizedSuggestion).not.toContain('simpler') and expect(normalizedSuggestion).not.toContain('simplify'), keeping the existing positive contains checks for 'native geospec opencascade c++ metrics analyzer' and 'test precision budget' to ensure a stable, consumer-visible contract.packages/geospec/native/opencascade/geospec_opencascade_single.provenance.json-3-4 (1)
3-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRegenerate this provenance file; it no longer matches the checked-in build config.
This metadata says
noLTO/lto: falseand even records the YAML config asgeospec_opencascade_single.js, while the actual config in this PR isgeospec.single.ymland its link flags include-flto. That breaks auditability because the provenance no longer describes the artifact you're shipping.Also applies to: 41-41, 51-52, 87-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/native/opencascade/geospec_opencascade_single.provenance.json` around lines 3 - 4, The provenance JSON (geospec_opencascade_single.provenance.json) is stale — regenerate it so the metadata matches the actual build config (geospec.single.yml) and flags; specifically update lto to true, include the -flto link flag in the recorded link flags, correct the referenced YAML name from geospec_opencascade_single.js to geospec.single.yml, and refresh buildId/timestamp to match a new authoritative build run; run the same provenance generation step you use in CI to produce a consistent, auditable file.packages/geospec/src/model/load-model.test.ts-55-70 (1)
55-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the thrown message here, not just the diagnostics payload.
This only checks
nameanddiagnostics, so a regression inGeoSpecModelLoadError.messagewould still pass unnoticed. Add a message assertion in this test, or switch it toawait expect(promise).rejects.toThrow(...)alongside the typed check.As per coding guidelines, "Assert both message and type in error assertions. Use
await expect(p).rejects.toThrow('msg')for async errors."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/model/load-model.test.ts` around lines 55 - 70, The test for loadModel should also assert the error message; update the assertion around loadModel({ source: new Uint8Array([1,2,3]), format: 'step' }) so it checks GeoSpecModelLoadError.message in addition to name and diagnostics — either convert the expectation to await expect(promise).rejects.toThrow('expected message') and retain the typed diagnostics check, or include a message field in the rejects.toMatchObject call to match the exact error message emitted by GeoSpecModelLoadError; ensure you reference the same loadModel call and the GeoSpecModelLoadError diagnostics structure when adding the message assertion.packages/testing/src/tau.ts-399-414 (1)
399-414:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
totalconsistent in the empty-entry fallback.This branch returns one failure but reports
total: 0, so any caller computing summary counts or pass rates will get inconsistent results when no GeoSpec files are found.Suggested fix
passes: [], passed: 0, - total: 0, + total: 1, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/testing/src/tau.ts` around lines 399 - 414, The empty-entry fallback returns one failure (id 'missing_geospec_file') but sets total: 0; update the returned object so total accurately reflects the number of checks (e.g., set total: failures.length or total: 1) while leaving passed: 0 unchanged; locate the branch that checks options.entryPaths.length === 0 in tau.ts and change the total field in the returned object to derive from the failures/passes arrays rather than a hard-coded 0.packages/testing/README.md-18-23 (1)
18-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the example self-contained.
defaultParamsis used here but never declared or imported, so the snippet cannot be pasted as-is. Please add the missing definition/import or inline a minimal defaults object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/testing/README.md` around lines 18 - 23, The example uses defaultParams but never defines or imports it; update the snippet around the parameterGroups(...) call to provide a minimal defaults object or import it so the example is self-contained — either add a local const defaultParams = { /* minimal defaults used by parameterGroups */ } above the groups declaration or import defaultParams from its module and ensure the names match the existing parameterGroups(mainParams, { defaults: defaultParams }) usage so the snippet can be copy-pasted and run.
🧹 Nitpick comments (13)
packages/vm/src/node-module-execution.ts (1)
23-29: ⚡ Quick winAdd
@publictag to exported function JSDoc.The
executeCodeInNodefunction is exported and has JSDoc documentation, but is missing the@publictag required for public API functions inpackages/.📝 Proposed fix
/** * Execute bundled code in Node.js without statically importing Node built-ins. * * Browser/client bundlers still parse this file, so Node imports are intentionally * hidden behind an opaque dynamic importer and the caller keeps the runtime guard. + * + * `@param` code - The bundled JavaScript code to execute. + * `@returns` The module exports and the file URL used for execution. + * `@public` */ export async function executeCodeInNode(code: string): Promise<{ value: unknown; entryUrl: string }> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vm/src/node-module-execution.ts` around lines 23 - 29, The JSDoc for the exported function executeCodeInNode is missing the required `@public` tag; update the JSDoc block immediately above the executeCodeInNode declaration to include an `@public` tag (e.g., add a line with "`@public`") so the function is marked as part of the public API; ensure the tag is placed inside the existing /** ... */ comment and follows the project's JSDoc style.libs/api-extractor/src/extract-geospec-types.ts (2)
163-163: 💤 Low valueConsole output in CLI script.
The coding guidelines require removing
console.logstatements. For CLI scripts, consider using a configurable logger or thestdoutparameter pattern used elsewhere (e.g., inpackages/geospec/src/cli.ts). However, since this is a build-time CLI tool (not runtime code), this is acceptable if intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/api-extractor/src/extract-geospec-types.ts` at line 163, Remove the direct console.log call in extract-geospec-types.ts and replace it with a configurable output mechanism: accept or thread a stdout/write callback (or use the existing CLI logger pattern from packages/geospec/src/cli.ts) and write the message via that interface instead of console.log; specifically change the line that logs using outputPath to use the injected stdout/logger so tests and linting don't rely on console.* usage.
122-156: ⚡ Quick winAdd JSDoc for exported public function.
buildGeoSpecTypeBundleis exported and used by consumers, but lacks JSDoc documentation. As per coding guidelines forlibs/packages, public JSDoc is required.📝 Proposed JSDoc
+/** + * Build a bundled GeoSpec type declaration package for Monaco and editor integration. + * + * Emits TypeScript declarations from the GeoSpec source, rewrites internal `#` aliases + * to relative imports, and packages the result as a JSON-serializable structure. + * + * `@returns` A record mapping package name to its generated types, files, and package.json. + * `@public` + */ export function buildGeoSpecTypeBundle(): Record<string, GeneratedPackageTypes> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/api-extractor/src/extract-geospec-types.ts` around lines 122 - 156, Add a public JSDoc block above the exported function buildGeoSpecTypeBundle describing its purpose (builds and returns a bundle of generated GeoSpec TypeScript declaration files), the returned type (Record<string, GeneratedPackageTypes>), any important side-effects (creates and removes a temporary directory), and the errors it may throw (e.g., when declaration emit fails or index.d.ts is missing); include tags `@returns` and `@throws` and briefly mention key behavior around createProgram/createProgram.emit and file collection (temporaryDirectory, rewriteInternalAliases, collectDeclarationFiles) so consumers understand usage and failure modes.packages/geospec/src/cli.ts (1)
84-130: 💤 Low valuePotential ReDoS from user-provided glob patterns.
The
globPatternToRegExpfunction constructs a regex from user-provided--patternCLI arguments. While the conversion escapes literal characters, deeply nested brace alternations (e.g.,{a,{b,{c,d}}}) or repeated wildcards could create complex regex patterns.For a CLI tool where the user controls input and would only harm themselves, this is low risk. However, if this function is ever used with untrusted input (e.g., from config files or network), consider adding:
- Pattern length limits
- Nesting depth limits for brace expansions
- A timeout wrapper around the regex test
Based on learnings: Guard against ReDoS by validating user-provided or dynamic regex patterns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/cli.ts` around lines 84 - 130, The globPatternToRegExp function currently builds RegExp from user input and can produce catastrophic patterns; update globPatternToRegExp (and any callers like normalizeCliPath usage) to validate input before conversion by enforcing a maximum pattern length, a maximum brace-nesting depth (detect nested '{' '}' pairs while scanning) and a limit on number of alternates in brace expansions, and reject or truncate patterns that exceed those thresholds; also document/raise a clear error when escapeRegExp(...) inputs or generated source would exceed limits, and consider wrapping downstream RegExp.test usage with a timeout or using a safe match function for untrusted inputs.libs/api-extractor/src/authoring-types.ts (1)
13-15: ⚡ Quick winUse a const function expression for the parser helper.
Line 13-15 uses a function declaration, which conflicts with this repo’s TS style rule.
Suggested change
-function parseAuthoringTypesMap(raw: string): AuthoringTypesMap { - return JSON.parse(raw) as AuthoringTypesMap; -} +const parseAuthoringTypesMap = (raw: string): AuthoringTypesMap => + JSON.parse(raw) as AuthoringTypesMap;As per coding guidelines:
**/*.{ts,tsx,js,jsx}: Use const declarations over function declarations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/api-extractor/src/authoring-types.ts` around lines 13 - 15, The parser helper parseAuthoringTypesMap is declared as a function declaration which violates the repo style that prefers const function expressions; change the declaration to a const arrow/function expression (e.g., const parseAuthoringTypesMap = (raw: string): AuthoringTypesMap => JSON.parse(raw) as AuthoringTypesMap) so the symbol parseAuthoringTypesMap remains the same and typing with AuthoringTypesMap is preserved.libs/chat/src/rpc/handlers/handle-run-geospec-tests.test.ts (1)
27-42: ⚡ Quick winUse
mock<RpcGeoSpecClient>()here instead of a hand-rolled stub.This literal works, but it drifts from the repo’s partial-mock convention and makes this test harder to keep aligned with the rest of the suite.
Suggested change
-import { describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; +import { mock } from 'vitest-mock-extended'; ... - const geospec: RpcGeoSpecClient = { - runTests: vi.fn().mockResolvedValue({ - success: true, - failures: [], - passes: [ - { - id: 'main.geospec.ts:main dimensions > width', - requirement: 'main dimensions > width', - targetFile: 'main.geospec.ts', - }, - ], - passed: 1, - total: 1, - }), - }; + const geospec = mock<RpcGeoSpecClient>(); + geospec.runTests.mockResolvedValue({ + success: true, + failures: [], + passes: [ + { + id: 'main.geospec.ts:main dimensions > width', + requirement: 'main dimensions > width', + targetFile: 'main.geospec.ts', + }, + ], + passed: 1, + total: 1, + });As per coding guidelines, "Use
mock<T>()fromvitest-mock-extendedfor partial mock objects. Never useas unknown asfor mocking."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/chat/src/rpc/handlers/handle-run-geospec-tests.test.ts` around lines 27 - 42, Replace the hand-rolled stub for the GeoSpec client with the repo-standard partial mock: create the geospec variable using mock<RpcGeoSpecClient>() (from vitest-mock-extended) instead of the inline object, then set its runTests to resolve the same result via geospec.runTests.mockResolvedValue({...}); update references in the test (handle-run-geospec-tests.test.ts) to use this mocked RpcGeoSpecClient instance so the test follows the project's mock<T>() convention.packages/testing/src/prompt-examples.test.ts (1)
40-45: ⚡ Quick winReplace these regex assertions with exact string checks.
These cases are simple string-shape checks, so the regexes add brittleness without extra signal.
startsWith/endsWithfor the fence and a small list of exact forbidden phrases withnot.toContainwould keep the tests aligned with the repo’s upstream-review guidance.As per coding guidelines,
Avoid regex-based assertions in tests bound for upstream PRs — they are hacky and indefensible in review, prefer principled assertions on parsed structures.Also applies to: 129-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/testing/src/prompt-examples.test.ts` around lines 40 - 45, Update the test "should produce a fenced TypeScript GeoSpec code block" to avoid regex assertions: use rendered.startsWith("```ts") and rendered.endsWith("```") instead of toMatch, and replace the loose contains checks with explicit assertions (e.g., expect(rendered).toContain("loadModel({ file: 'main.scad' })") and add expect(rendered).not.toContain(...) for any forbidden phrases). Make the same change for the similar assertions referenced around lines 129-131; locate these via the test name and the helper function renderCanonicalExample('scad') to apply the replacements.libs/chat/src/schemas/tools/test-model.tool.schema.ts (1)
77-79: ⚡ Quick winExport the input side of the schema.
TestModelInputis consumer-facing, so it should come fromz.input<typeof testModelInputSchema>rather thanz.infer. Otherwise a future.default()on one of these filters will make the call-site type stricter than the actual accepted wire input.Proposed fix
export const testModelInputSchema = geoSpecRunFilterInputSchema; /** `@public` */ -export type TestModelInput = z.infer<typeof testModelInputSchema>; +export type TestModelInput = z.input<typeof testModelInputSchema>;As per coding guidelines, "Use
z.input(notz.infer) for consumer-facing Zod schema types — preserves.default()optionality".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/chat/src/schemas/tools/test-model.tool.schema.ts` around lines 77 - 79, TestModelInput is currently derived with z.infer which reflects the parsed output and will incorrectly tighten types if defaults are added; change the consumer-facing type to use z.input<typeof testModelInputSchema> instead of z.infer to preserve optionality/defaults. Update the TestModelInput type alias (referencing testModelInputSchema and TestModelInput) to use z.input so callers receive the correct wire-input shape.packages/geospec/src/mesh/load-mesh.test.ts (1)
34-40: ⚡ Quick winAvoid the regex assertion for
contentHash.This test can assert the same contract without
toMatch(...), which the repo explicitly avoids for upstream test cases.Proposed fix
expect(result.subject.kind).toBe('geometry-subject'); expect(result.subject.provenance.source.path).toBe('/fixtures/square.glb'); expect(result.subject.provenance.parameters).toEqual({ width: 10 }); - expect(result.subject.provenance.contentHash).toMatch(/^sha256:/); + expect(result.subject.provenance.contentHash.startsWith('sha256:')).toBe(true); + expect(result.subject.provenance.contentHash.slice('sha256:'.length)).toHaveLength(64); expect(result.subject.mesh.stats.triangleCount).toBe(2);As per coding guidelines, "Avoid regex-based assertions in tests bound for upstream PRs — they are hacky and indefensible in review, prefer principled assertions on parsed structures".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/mesh/load-mesh.test.ts` around lines 34 - 40, The test should avoid a regex assertion on result.subject.provenance.contentHash; instead assert the structured properties: verify contentHash is a non-empty string, that it begins with the literal prefix "sha256:", and that the substring after that prefix is non-empty (or valid hex) to ensure a principled contract. Update the assertion around result.subject.provenance.contentHash accordingly (replace the toMatch(...) check with explicit checks on type, starts-with "sha256:" and non-empty suffix).packages/testing/src/schemas.ts (1)
78-96: ⚡ Quick winUse
z.inputfor these exported schema aliases.
TestFileEntryandTestFileare consumer-facing types, so keeping them onz.inferwill drop.default()optionality as soon as defaults are introduced. Switching the updated aliases toz.inputkeeps the public contract aligned with the schema definition.Proposed fix
-export type TestFileEntry = z.infer<typeof testFileEntrySchema>; +export type TestFileEntry = z.input<typeof testFileEntrySchema>; @@ -export type TestFile = z.infer<typeof testFileSchema>; +export type TestFile = z.input<typeof testFileSchema>;As per coding guidelines, "Use
z.input(notz.infer) for consumer-facing Zod schema types — preserves.default()optionality."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/testing/src/schemas.ts` around lines 78 - 96, Change the exported consumer-facing TypeScript aliases to use z.input instead of z.infer so defaulted/optional fields on the Zod schemas are preserved: replace the type alias for TestFileEntry (currently using z.infer<typeof testFileEntrySchema>) with z.input<typeof testFileEntrySchema>, and do the same for TestFile (replace z.infer<typeof testFileSchema> with z.input<typeof testFileSchema>), leaving the underlying schemas testFileEntrySchema and testFileSchema unchanged.packages/geospec/src/mesh/watertight.ts (1)
122-126: ⚡ Quick winNon-indexed TRIANGLES primitives are silently excluded from watertight analysis.
Line 124 skips primitives without an index accessor, but
connected-components.tshandles non-indexed geometry by generating implicit indices (lines 56-64 in that file). This inconsistency means non-indexed meshes will be analyzed for connected components but not for watertightness.Consider aligning the behavior by generating implicit indices when
indexAccessoris null, similar toreadTriangleVertexIndices.♻️ Suggested approach
const posAccessor = primitive.getAttribute('POSITION'); const indexAccessor = primitive.getIndices(); - if (!posAccessor || !indexAccessor) { + if (!posAccessor) { continue; } const vCount = posAccessor.getCount(); - const indexCount = indexAccessor.getCount(); + const indexCount = indexAccessor ? indexAccessor.getCount() : vCount; const name = resolvedMeshName && resolvedMeshName.length > 0 ? resolvedMeshName : `${fallbackName}#${primOrdinal}`; primOrdinal += 1; const start = allTriangles.length; for (let i = 0; i < vCount; i++) { allPositions.push(posAccessor.getElement(i, [0, 0, 0])); } for (let i = 0; i < indexCount; i += 3) { - allTriangles.push([ - indexAccessor.getScalar(i) + vertexOffset, - indexAccessor.getScalar(i + 1) + vertexOffset, - indexAccessor.getScalar(i + 2) + vertexOffset, - ]); + const i0 = indexAccessor ? indexAccessor.getScalar(i) : i; + const i1 = indexAccessor ? indexAccessor.getScalar(i + 1) : i + 1; + const i2 = indexAccessor ? indexAccessor.getScalar(i + 2) : i + 2; + allTriangles.push([i0 + vertexOffset, i1 + vertexOffset, i2 + vertexOffset]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/mesh/watertight.ts` around lines 122 - 126, The watertight check currently skips primitives when primitive.getIndices() is null, causing non-indexed TRIANGLES to be ignored; update the logic in the function using primitive.getAttribute('POSITION') and primitive.getIndices() to generate implicit indices (0..vertexCount-1) when indexAccessor is null, mirroring readTriangleVertexIndices and the implicit-index behavior in connected-components.ts, then pass those generated indices into the existing triangle processing so non-indexed geometry is included in the watertight analysis.packages/testing/src/tau.test.ts (1)
196-210: ⚡ Quick winUse a typed
GeometrySubjectfixture instead of double-casting.Both mocks bypass the GeoSpec contract with
as unknown as GeometrySubject, and the objects are already out of sync with the canonical type (capabilitiesshape,source.format,provenance.unit/loader,mesh.format, andanalyseConnectedComponentsresult fields). That means these tests can keep passing while the real public contract has drifted underneath them. Please switch to a shared fully-typed fixture factory ormock<GeometrySubject>()so the test fails when the contract changes. As per coding guidelines,Use mock<T>() from vitest-mock-extended for partial mock objects. Never use as unknown as for mocking.Also applies to: 248-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/testing/src/tau.test.ts` around lines 196 - 210, Replace the double-cast test fixtures that assign to the subject variable with a properly typed mock: stop using "as unknown as GeometrySubject" and create the fixture using mock<GeometrySubject>() (from vitest-mock-extended) or a shared typed fixture factory so the object adheres to the GeoSpec contract (capabilities shape, provenance.source.format/unit/loader, mesh.format, and analyseConnectedComponents return fields). Update the two places where this pattern appears (the subject declaration and the later similar block) to construct and partially override the mock's properties rather than force-casting, ensuring the tests will fail if the GeometrySubject type changes.packages/geospec/src/model/types.ts (1)
25-27: ⚡ Quick winNarrow
export()toGeoSpecModelFormat.
GeoSpecRuntimeClient.export()currently accepts any string, while the loader API only exposesGeoSpecModelFormat. Keeping this wider weakens the public contract for custom runtime adapters and lets unsupported formats slip through compile time. Typing this parameter asGeoSpecModelFormatkeeps the runtime interface aligned with whatloadModel()can actually request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/geospec/src/model/types.ts` around lines 25 - 27, The export() function currently types its format parameter as a plain string, allowing unsupported values; change the parameter type to GeoSpecModelFormat so GeoSpecRuntimeClient.export(format: GeoSpecModelFormat, input: unknown) matches the loader API and prevents unsupported formats at compile time—update the export signature in types.ts and any related declarations/usages to use GeoSpecModelFormat.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/geospec/native/opencascade/geospec_opencascade_single.build-manifest.json`:
- Line 6: The manifest incorrectly sets validation_passed to true despite
binding_report.failed being 2 with compile_error failures; update the
manifest-generation logic (the code that writes validation_passed and
symbols.pass) so validation_passed is true only when symbols.pass is true AND
binding_report.failed == 0 (or when all binding_report failures are explicitly
filtered/whitelisted); alternatively, if those compile_error entries are
expected and safe, add them to the allowed-failures filter used by the writer so
those specific binding_report.compile_error entries are excluded from the failed
count before computing validation_passed/symbols.pass; ensure the fields
validation_passed and symbols.pass are derived from the filtered binding report
rather than being hard-coded to true.
In `@packages/geospec/native/opencascade/wrappers/geospec-mesh-metrics.cpp`:
- Around line 310-317: The code currently computes perDirectionLimit with
integer division which over-allocates for odd samples; replace that with an
explicit split that honors odd counts: compute int dirA = (samples + 1) / 2 and
int dirB = samples / 2, use distances.reserve(static_cast<size_t>(samples)),
then call geospecAppendDistances(distances, actual, actualTriangleCount, dirA,
expectedBvh) and geospecAppendDistances(distances, expected,
expectedTriangleCount, dirB, actualBvh). Update the perDirectionLimit variable
usage around GeoSpecTriangleBvh expectedBvh/actualBvh and the two
geospecAppendDistances calls to use dirA/dirB so the native path produces
exactly `samples` distances.
In `@packages/geospec/native/opencascade/wrappers/geospec-step-reader.cpp`:
- Around line 89-97: The GeoSpecStepReadResult copy constructor currently
performs move-like ownership transfer via const_cast (mutating meshTrianglesPtr_
and meshTriangleCount_), which violates the Rule of Five; replace this with
explicit move semantics by deleting the copy constructor and copy assignment,
implementing a proper move constructor and move assignment that transfer
meshTrianglesPtr_ and meshTriangleCount_ (setting the source's members to
nullptr/0 after move), and ensure evidenceJson_ is moved or copied
appropriately; alternatively, implement deep-copy semantics for
meshTrianglesPtr_ or change meshTrianglesPtr_ to std::unique_ptr so ownership is
clear—also update any Emscripten bindings to reflect that the type is
non-copyable if you choose move-only semantics.
In `@packages/geospec/package.json`:
- Around line 135-145: The package.json added a new peer dependency "replicad":
">=0.23.0" but the pnpm lockfile wasn't updated causing CI to fail; either
regenerate and commit the updated lockfile by running a fresh install (e.g., run
pnpm install to update pnpm-lock.yaml) so the new specifier is reflected, or if
"replicad" is not actually required remove the peer dependency entry from
package.json; reference the package.json peerDependencies block (the "replicad"
specifier) and ensure the pnpm-lock.yaml is updated and committed.
In `@packages/geospec/src/mesh/distance.ts`:
- Around line 302-323: perDirectionLimit currently uses
Math.floor(sampleLimit/2) which causes odd sampleLimit values to produce twice
the requested samples; instead split the budget explicitly so actual+expected
sample counts equal sampleLimit (e.g. left = Math.ceil(sampleLimit/2), right =
Math.floor(sampleLimit/2)) and use those two limits when calling samplePoints
(replace perDirectionLimit with
perDirectionLimitActual/perDirectionLimitExpected used by actualSamples and
expectedSamples), update estimatedTrianglePairs to use actualSamples.length and
expectedSamples.length (as you already do) and ensure
failBudget/requestedSamples still reports the original sampleLimit; this
guarantees stats.samples matches the caller's sampleLimit while preserving
existing logic in distanceToTriangles and summarizeDistances.
In `@packages/geospec/src/mesh/native.ts`:
- Around line 64-68: Validate that the declared triangleCount matches the actual
triangles buffer length before any _malloc or HEAP writes: in allocateFloat64
(and the related allocation helpers used around the triangle handling code paths
referenced by lines 78-109), check that triangles.length (or values.length) is
at least triangleCount * 3 (or the expected element count) and throw or return
an error if it doesn't; perform this check before calling module._malloc or
module.HEAP*.set so we never write/read past the allocated WASM heap. Ensure the
validation references the same parameter names (triangleCount, triangles, and
the allocation helpers like allocateFloat64) so callers fail fast on mismatches.
In `@packages/geospec/src/model/errors.ts`:
- Around line 8-21: The GeoSpecModelLoadError constructor currently stores the
caller-owned diagnostics array by reference causing mutation after throw to
change error.diagnostics; snapshot and (optionally) freeze the diagnostics
before assigning to this.diagnostics in GeoSpecModelLoadError (e.g., create a
shallow copy like [...diagnostics] or diagnostics.slice(), and Object.freeze if
immutability is desired) so the stored diagnostics and the error.message remain
stable.
In `@packages/geospec/src/model/load-model.ts`:
- Around line 65-67: The call to the caller-provided runtime factory
(options.runtime()) in resolveRuntime can reject and currently bubbles raw;
catch any rejection around the await of options.runtime() and rethrow a
GeoSpecModelLoadError (or wrap the original error as its cause/message) so
loadModel's error contract is preserved; update the branch in resolveRuntime
that returns { runtime: await options.runtime(), ownsRuntime: false } to perform
a try/catch, convert failures into a GeoSpecModelLoadError with contextual text,
and include the original error for debugging.
In `@packages/geospec/src/runner/collector.ts`:
- Around line 1123-1141: The async describe/it bodies use shared mutable state
(suite and __GEOSPEC_ACTIVE_TEST__) and snapshot pending once, so resumed async
work can run under the wrong suite/test and late-registered promises aren’t
waited for; fix trackPending by capturing the current suite and currentTest into
local constants inside the function and use those captured values when reporting
errors/finalizing so callbacks run with the correct context (referencing
trackPending, pending, suite, and __GEOSPEC_ACTIVE_TEST__); also change
waitForCompletion to not snapshot pending once—loop awaiting Promise.allSettled
on newly added entries until no new pending items remain (i.e., repeatedly
drain/await pending batches) so promises registered after awaits are also
awaited.
In `@packages/geospec/src/runner/run-geospec-module.ts`:
- Around line 171-177: The code currently stores per-run state (collector from
createCollector and loaders accessed via globalWithModelLoader using
geospecModelLoaderGlobalKey and geospecStepLoaderGlobalKey) on process-global
globalThis, causing races between concurrent runGeoSpecModule invocations;
instead make the state VM-local by passing the collector, modelLoader and
stepLoader through the VM/module registry or by generating a unique run token
and keying the state by that token (e.g., attach to the VM context or module
registry accessible inside runGeoSpecModule rather than globalThis), update
usages in runGeoSpecModule and any helper functions that read those globals to
accept the injected per-run bindings (or look them up by token), and remove
reads/writes to geospecModelLoaderGlobalKey/geospecStepLoaderGlobalKey on
globalThis so finally blocks cannot clear another run's state.
In `@packages/testing/src/parameters.ts`:
- Around line 82-90: The parseParameterEntry function currently treats any
string as raw JSON which causes JSON.parse to choke on filesystem paths and
misses validating group shapes; update parseParameterEntry (and the analogous
JSON-parsing block around lines 118-123) to first detect and reject
filesystem-like strings (e.g., strings that start with '.', '/', or contain path
separators/backslashes) with a clear error instructing the caller to read the
file first, then JSON.parse the string and validate that parsed is a record with
a string activeGroup and that parsed.groups is a record whose each group value
is itself a record containing a "values" record; throw a targeted Error message
when the group or values shape is invalid.
In `@packages/testing/src/prompt-examples.ts`:
- Around line 180-203: The example uses parameterGroups(mainParams, { defaults:
defaultParams }) but never defines or imports defaultParams; add a declaration
or import named defaultParams before it is used (e.g., const defaultParams = {
base: { width: <number> }, ... } or import { defaultParams } from '…') and
ensure the object has the expected shape accessed later (contains base.width so
group.values.base.width resolves) so the example produces valid, runnable test
code.
In `@packages/testing/src/tau.ts`:
- Around line 289-362: The code currently uses a single shared global renderer
key (tauGeoSpecRendererGlobalKey / globalThis.__TAUCAD_GEOSPEC_RENDER__) which
allows concurrent runs to clobber each other; change to per-invocation scoping
by passing a renderer reference through the call stack instead of
reading/writing the global: update renderTauModel and analyzeTauModel to accept
an explicit renderer option (e.g., options.renderer) and prefer that over
globalThis, and modify runTauGeoSpecTests (and the code paths around the other
occurrence at the later block) to provide a renderer function via options or a
unique token/closure so each invocation binds its own renderer instance; ensure
loadModel callers use the provided renderer option and remove reliance on the
shared tauGeoSpecRendererGlobalKey.
---
Outside diff comments:
In `@libs/chat/src/schemas/message.schema.test.ts`:
- Around line 225-246: Update the test so it not only checks result.success but
also inspects the parsed message payload from uiMessagesSchema: after safeParse
and expect(result.success).toBe(true), extract the parsed tool message (e.g.,
result.data[1] or the part matching type `tool-${toolName.testModel}` /
MyMessagePart) and assert its input/rawInput shape with toEqual/toMatchObject
(mirroring the neighboring tool-read_file assertions) to ensure rawInput is
preserved and input/files/testNamePattern are correct; apply the same change to
the analogous test block referenced (lines 392-406) so both tests validate
observable parsed structure rather than only parse success.
---
Minor comments:
In
`@packages/geospec/native/opencascade/geospec_opencascade_single.provenance.json`:
- Around line 3-4: The provenance JSON
(geospec_opencascade_single.provenance.json) is stale — regenerate it so the
metadata matches the actual build config (geospec.single.yml) and flags;
specifically update lto to true, include the -flto link flag in the recorded
link flags, correct the referenced YAML name from geospec_opencascade_single.js
to geospec.single.yml, and refresh buildId/timestamp to match a new
authoritative build run; run the same provenance generation step you use in CI
to produce a consistent, auditable file.
In `@packages/geospec/README.md`:
- Around line 25-27: The bounding-box axis order in the README example is wrong:
change the toHaveBoundingBox assertion so its size matches makeBaseBox(40, 20,
8) by swapping the Y/Z values; update the expectGeo(...).toHaveBoundingBox call
(reference symbols: expectGeo, toHaveBoundingBox, makeBaseBox) to use size: { x:
0.04, y: 0.02, z: 0.008 } while leaving the tolerances unchanged.
- Around line 119-121: The README example uses an undeclared symbol
defaultParams causing the snippet to fail; update the example to either declare
or import defaultParams and show its structure, e.g., add a brief defaultParams
declaration or an import before calling parameterGroups so the example is
runnable: reference the existing mainParams import and the parameterGroups(...)
call and ensure defaultParams is defined (or explicitly passed as
undefined/null) so the snippet compiles and demonstrates the intended usage.
In `@packages/geospec/src/index.test.ts`:
- Around line 152-198: The test currently masks type mismatches by returning the
modelLoader fixture with a type assertion "as unknown as GeometrySubject";
remove that cast and instead construct a properly typed fixture object that
satisfies the GeometrySubject interface at compile time (either by creating a
helper function like buildGeometrySubjectFixture(...) or a typed constant
fixture and returning it from modelLoader). Update the modelLoader
implementation in packages/geospec/src/index.test.ts to return that
helper/fixture (matching GeometrySubject fields such as kind, provenance,
capabilities, diagnostics, mesh, and the mesh.stats methods) so the compiler
enforces the contract rather than relying on a type-erasing assertion.
In `@packages/geospec/src/mesh/distance.test.ts`:
- Around line 39-43: The test uses a brittle regex negative assertion on
suggestion; replace it with principled substring assertions: compute suggestion
and normalizedSuggestion as before (result.diagnostics[0]?.suggestion ?? '' and
suggestion.toLowerCase()), then remove
expect(suggestion).not.toMatch(/simpler|simplify/i) and instead add explicit
negative assertions like expect(normalizedSuggestion).not.toContain('simpler')
and expect(normalizedSuggestion).not.toContain('simplify'), keeping the existing
positive contains checks for 'native geospec opencascade c++ metrics analyzer'
and 'test precision budget' to ensure a stable, consumer-visible contract.
In `@packages/geospec/src/mesh/load-mesh.ts`:
- Around line 113-125: inferFormat currently defaults any unknown filename to
'glb', which hides unsupported extensions; update inferFormat in
packages/geospec/src/mesh/load-mesh.ts (and the similar logic at the other
occurrence) to only return known formats ('mesh-buffer', 'gltf', 'glb') when the
source clearly matches, and otherwise return undefined (or a dedicated
unsupported sentinel) so the caller can hit the typed UNSUPPORTED_MESH_FORMAT
branch; use the existing symbols inferFormat, LoadMeshOptions,
isMeshBufferSource, and MeshFileFormat to locate and adjust the logic (do not
silently treat unknown extensions like ".obj" or ".step" as 'glb').
In `@packages/geospec/src/model/load-model.test.ts`:
- Around line 55-70: The test for loadModel should also assert the error
message; update the assertion around loadModel({ source: new
Uint8Array([1,2,3]), format: 'step' }) so it checks
GeoSpecModelLoadError.message in addition to name and diagnostics — either
convert the expectation to await expect(promise).rejects.toThrow('expected
message') and retain the typed diagnostics check, or include a message field in
the rejects.toMatchObject call to match the exact error message emitted by
GeoSpecModelLoadError; ensure you reference the same loadModel call and the
GeoSpecModelLoadError diagnostics structure when adding the message assertion.
In `@packages/testing/README.md`:
- Around line 18-23: The example uses defaultParams but never defines or imports
it; update the snippet around the parameterGroups(...) call to provide a minimal
defaults object or import it so the example is self-contained — either add a
local const defaultParams = { /* minimal defaults used by parameterGroups */ }
above the groups declaration or import defaultParams from its module and ensure
the names match the existing parameterGroups(mainParams, { defaults:
defaultParams }) usage so the snippet can be copy-pasted and run.
In `@packages/testing/src/tau.ts`:
- Around line 399-414: The empty-entry fallback returns one failure (id
'missing_geospec_file') but sets total: 0; update the returned object so total
accurately reflects the number of checks (e.g., set total: failures.length or
total: 1) while leaving passed: 0 unchanged; locate the branch that checks
options.entryPaths.length === 0 in tau.ts and change the total field in the
returned object to derive from the failures/passes arrays rather than a
hard-coded 0.
In `@packages/vm/src/esbuild-core.ts`:
- Around line 366-371: The wildcard selection logic compares prefix.length to
best.pattern.indexOf('*') which is unclear and fragile; instead compute and
compare prefix lengths directly: when setting best (the object assigned to best
currently containing target, pattern, wildcard) include the numeric
bestPrefixLength = pattern.indexOf('*') (or store prefix.length) and then change
the comparison to prefix.length > best.bestPrefixLength (or prefix.length >
bestPrefixLength) || pattern.length > best.pattern.length so the "most specific
(longest prefix) wins" rule is implemented reliably; update the assignments and
the if condition around best, pattern, prefix, suffix, wildcard accordingly.
---
Nitpick comments:
In `@libs/api-extractor/src/authoring-types.ts`:
- Around line 13-15: The parser helper parseAuthoringTypesMap is declared as a
function declaration which violates the repo style that prefers const function
expressions; change the declaration to a const arrow/function expression (e.g.,
const parseAuthoringTypesMap = (raw: string): AuthoringTypesMap =>
JSON.parse(raw) as AuthoringTypesMap) so the symbol parseAuthoringTypesMap
remains the same and typing with AuthoringTypesMap is preserved.
In `@libs/api-extractor/src/extract-geospec-types.ts`:
- Line 163: Remove the direct console.log call in extract-geospec-types.ts and
replace it with a configurable output mechanism: accept or thread a stdout/write
callback (or use the existing CLI logger pattern from
packages/geospec/src/cli.ts) and write the message via that interface instead of
console.log; specifically change the line that logs using outputPath to use the
injected stdout/logger so tests and linting don't rely on console.* usage.
- Around line 122-156: Add a public JSDoc block above the exported function
buildGeoSpecTypeBundle describing its purpose (builds and returns a bundle of
generated GeoSpec TypeScript declaration files), the returned type
(Record<string, GeneratedPackageTypes>), any important side-effects (creates and
removes a temporary directory), and the errors it may throw (e.g., when
declaration emit fails or index.d.ts is missing); include tags `@returns` and
`@throws` and briefly mention key behavior around createProgram/createProgram.emit
and file collection (temporaryDirectory, rewriteInternalAliases,
collectDeclarationFiles) so consumers understand usage and failure modes.
In `@libs/chat/src/rpc/handlers/handle-run-geospec-tests.test.ts`:
- Around line 27-42: Replace the hand-rolled stub for the GeoSpec client with
the repo-standard partial mock: create the geospec variable using
mock<RpcGeoSpecClient>() (from vitest-mock-extended) instead of the inline
object, then set its runTests to resolve the same result via
geospec.runTests.mockResolvedValue({...}); update references in the test
(handle-run-geospec-tests.test.ts) to use this mocked RpcGeoSpecClient instance
so the test follows the project's mock<T>() convention.
In `@libs/chat/src/schemas/tools/test-model.tool.schema.ts`:
- Around line 77-79: TestModelInput is currently derived with z.infer which
reflects the parsed output and will incorrectly tighten types if defaults are
added; change the consumer-facing type to use z.input<typeof
testModelInputSchema> instead of z.infer to preserve optionality/defaults.
Update the TestModelInput type alias (referencing testModelInputSchema and
TestModelInput) to use z.input so callers receive the correct wire-input shape.
In `@packages/geospec/src/cli.ts`:
- Around line 84-130: The globPatternToRegExp function currently builds RegExp
from user input and can produce catastrophic patterns; update
globPatternToRegExp (and any callers like normalizeCliPath usage) to validate
input before conversion by enforcing a maximum pattern length, a maximum
brace-nesting depth (detect nested '{' '}' pairs while scanning) and a limit on
number of alternates in brace expansions, and reject or truncate patterns that
exceed those thresholds; also document/raise a clear error when
escapeRegExp(...) inputs or generated source would exceed limits, and consider
wrapping downstream RegExp.test usage with a timeout or using a safe match
function for untrusted inputs.
In `@packages/geospec/src/mesh/load-mesh.test.ts`:
- Around line 34-40: The test should avoid a regex assertion on
result.subject.provenance.contentHash; instead assert the structured properties:
verify contentHash is a non-empty string, that it begins with the literal prefix
"sha256:", and that the substring after that prefix is non-empty (or valid hex)
to ensure a principled contract. Update the assertion around
result.subject.provenance.contentHash accordingly (replace the toMatch(...)
check with explicit checks on type, starts-with "sha256:" and non-empty suffix).
In `@packages/geospec/src/mesh/watertight.ts`:
- Around line 122-126: The watertight check currently skips primitives when
primitive.getIndices() is null, causing non-indexed TRIANGLES to be ignored;
update the logic in the function using primitive.getAttribute('POSITION') and
primitive.getIndices() to generate implicit indices (0..vertexCount-1) when
indexAccessor is null, mirroring readTriangleVertexIndices and the
implicit-index behavior in connected-components.ts, then pass those generated
indices into the existing triangle processing so non-indexed geometry is
included in the watertight analysis.
In `@packages/geospec/src/model/types.ts`:
- Around line 25-27: The export() function currently types its format parameter
as a plain string, allowing unsupported values; change the parameter type to
GeoSpecModelFormat so GeoSpecRuntimeClient.export(format: GeoSpecModelFormat,
input: unknown) matches the loader API and prevents unsupported formats at
compile time—update the export signature in types.ts and any related
declarations/usages to use GeoSpecModelFormat.
In `@packages/testing/src/prompt-examples.test.ts`:
- Around line 40-45: Update the test "should produce a fenced TypeScript GeoSpec
code block" to avoid regex assertions: use rendered.startsWith("```ts") and
rendered.endsWith("```") instead of toMatch, and replace the loose contains
checks with explicit assertions (e.g., expect(rendered).toContain("loadModel({
file: 'main.scad' })") and add expect(rendered).not.toContain(...) for any
forbidden phrases). Make the same change for the similar assertions referenced
around lines 129-131; locate these via the test name and the helper function
renderCanonicalExample('scad') to apply the replacements.
In `@packages/testing/src/schemas.ts`:
- Around line 78-96: Change the exported consumer-facing TypeScript aliases to
use z.input instead of z.infer so defaulted/optional fields on the Zod schemas
are preserved: replace the type alias for TestFileEntry (currently using
z.infer<typeof testFileEntrySchema>) with z.input<typeof testFileEntrySchema>,
and do the same for TestFile (replace z.infer<typeof testFileSchema> with
z.input<typeof testFileSchema>), leaving the underlying schemas
testFileEntrySchema and testFileSchema unchanged.
In `@packages/testing/src/tau.test.ts`:
- Around line 196-210: Replace the double-cast test fixtures that assign to the
subject variable with a properly typed mock: stop using "as unknown as
GeometrySubject" and create the fixture using mock<GeometrySubject>() (from
vitest-mock-extended) or a shared typed fixture factory so the object adheres to
the GeoSpec contract (capabilities shape, provenance.source.format/unit/loader,
mesh.format, and analyseConnectedComponents return fields). Update the two
places where this pattern appears (the subject declaration and the later similar
block) to construct and partially override the mock's properties rather than
force-casting, ensuring the tests will fail if the GeometrySubject type changes.
In `@packages/vm/src/node-module-execution.ts`:
- Around line 23-29: The JSDoc for the exported function executeCodeInNode is
missing the required `@public` tag; update the JSDoc block immediately above the
executeCodeInNode declaration to include an `@public` tag (e.g., add a line with
"`@public`") so the function is marked as part of the public API; ensure the tag
is placed inside the existing /** ... */ comment and follows the project's JSDoc
style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
Author
|
@claude review |
|
Claude Code is working… I'll analyze this and get back to you. |
Contributor
Author
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Contributor
Author
|
@claude review |
This comment was marked as resolved.
This comment was marked as resolved.
Two worker-lifecycle edge cases could leave runTests promises unresolved: an unsolicited 'closed' message (no close() in flight) detached the worker without failing pending runs, and if the worker never emitted 'initialized'/'error' the init promise never settled. Terminate and fail in-flight runs on unexpected close, and add a bounded initialization timeout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the toBeDefined() gate plus manual throw and per-field checks with a single toMatchObject over the diagnostic shape (using expect.closeTo for the approximate volume), per the review's test-quality guidance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace regex message matching with exact-substring checks; the repo test guidelines discourage regex assertions in upstream PR tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The claude-code workflow checks out the untrusted PR head and runs PR-authored tooling (pnpm/nx/npm), so a persisted GITHUB_TOKEN in .git/config could be scraped by a malicious PR build script. Set persist-credentials: false on both code-running checkouts as defense-in-depth; the action authenticates its own git/API operations via its github_token input, so pushes are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Anthropic AIMessages carry each tool call twice: as a native tool_use content block (with the args in `input`) and in the normalized `tool_calls` array. estimateRawComponents folded the tool_use block into `structuredTokens` and then added `call.args` again, roughly doubling the `tool_call_args` component for the primary provider. The over-estimate is conservative (it only makes compaction fire early, never overflows) but skews the budget breakdown and trigger point. Skip tool_use blocks in estimateContent when the message has a tool_calls array, so the args are counted once via tool_calls. Keying the skip on tool_calls being present keeps every provider shape counted exactly once and cannot flip to an under-count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
stepOccurrenceEntities inner-joins STEP products against mesh components by exact name. A product whose exported mesh component was renamed or suffixed produced no match and returned []. Because occurrenceEntities preferred STEP entities whenever any product matched, that unmatched mesh occurrence was dropped from every selector result and became silently unselectable. occurrenceEntities now unions the STEP-labelled matches with the mesh occurrences no product claimed, so every component stays selectable. The mesh occurrence list is also computed once instead of per product. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
solidNodeBySolidIndex kept a single { nodeIndex, meshIndex } per solid, so
when two glTF nodes referenced the same instanced solid the last write won
and the earlier node lost its body binding. That node received no
tauComponentId extras in applyTopologyExtras and became unselectable in the
topology payload.
Accumulate a binding per referencing node and set the shared body component
id on all of them; the representative node/mesh still drives the component's
geometry refs. Renamed the map to solidNodesBySolidIndex to reflect the
one-to-many relationship.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GeoSpec suites should use model defaults when no parameters are supplied and pass variant values directly to loadModel. Removing the generated package import map keeps suites portable across browser and headless runtimes.
Revision reconstruction requires invertible create and delete operations. Persist full before/after content with tool results while trimming only the LLM-bound copy.
Restore state is derived from persisted file snapshots and keyed by stable user-message IDs. Persist the revision slice through the project machine so full-document writes cannot clobber it.
Render one shared revision card in chat history and the revisions pane, with restore, discard, current-state, fork, and return-to-latest controls.
Filesystem-driven model updates synchronously trigger Monaco's normal change callback. Preserve path provenance across that callback so restore stays clean without dropping genuine queued editor writes.
Restores use writeFiles, which bypassed writer-pool invalidation and exact-path events. Delegate every batch entry to writeFile so runtime watchers, mount backends, origins, queues, and locks retain canonical behavior.
Sol, Terra, and Luna each passed Tau's live tool-call smoke test. Keep GPT-5.5 available outside the recommended list as a fallback.
Publications need to observe invite/notification send volume and cap suppressions to act on abuse. Add publication.invite_emails{outcome} and publication.invite_emails.suppressions{reason} counters, plural-suffixed per the registry's counter naming rule.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An authenticated owner could trigger unbounded emails from Tau's domain via repeated invites or publishes; the revoke/re-invite path bypassed the only existing guard, a spam and sender-reputation risk. Enforce a 200/owner/day cap at the sendPublicationInviteNotifications chokepoint, reusing the renamed PublicationRateLimiterService (shared INCRBY Lua). Over-cap and limiter-unavailable both fail closed, so grants and publishes still succeed; sent and suppressed emails are recorded via the new counters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The thumbnail v3->v4 review exposed that a filesystem-first architecture was violated because the philosophy lived only as one vision-policy bullet, not as enforceable rules. Codify it (research doc: filesystem-first-policy-alignment, PR1-PR8): - project-manifest-policy (new): tau.json contract - placement, Zod source of truth, schemaVersion migrations, loop guards, untrusted input. - filesystem-authority-policy (new): single-authority invariant (one provider per storage root, mounts as routing from persistent config); absorbs the stale ZenFS-era provider rules with an old-number map. - filesystem-policy: retire ZenFS mechanics, trim 599->498 lines. - storage-policy: Rule 0 store-selection boundary (what belongs in IDB). - worker-policy: worker-per-feature topology. - runtime-architecture-policy: multi-client cache-parity + placement. - vision-policy: "Hosts are peers" principle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rules 13a/13b moved to filesystem-authority-policy (Rules 10/11) when the Backend & Provider rules were extracted from filesystem-policy. Update the code comments that cite them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Enforce storage-policy Rule 0: project-scoped state belongs in the project filesystem, not IndexedDB. Flags indexedDB.* factory calls and globalThis/window/self.indexedDB access outside an allowlist (object-store provider, handle-store, filesystem backends, tests). Report-only with a rule-option allowlist; repo-wide blast radius is zero. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
no-handrolled-fanout.js returns unwrapped.members.some(...), inferred as any because TSESTree.TypeNode resolves loosely in JSDoc-typed .js files - the ESTree-AST false-positive class the override already documents. Matches the three sibling no-unsafe-* rules already disabled for this glob. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
GeoSpec and
test_modelchange how agents validate CAD output end-to-end; API/chat RPC and dev-server lifecycle changes affect local dev and reconnect behavior, though CI adds integration coverage for API storage/DB.Overview
Adds
geospecas a Vitest-style geometry testing stack (describe/it/expectGeo,loadModel, STEP/BRep matchers, CLI filters/JSON) and routes agent verification throughtest_modelover chat RPC instead of the removededit_tests/test.jsonflow.Runtime & analysis: headless paths now pass
defineRuntime({ kernels, bundlers })intoinProcessTransport; geometry analysis tests align bounding-box expectations with Z-up raw GLB from the runtime.API & chat: registers
PublicationsModule, hardens dev Vite/Nest lifecycle (serializedapp.init, SSR-loaded unhandled-rejection handler), adds chat RPC protocol version checks on join, improves RPC output validation and abort/shutdown handling, and expands.env.example/CI with S3, view-cookie, and a Postgres + MinIO API test job.Tooling & policy: agent skills/docs shift to ESM-only package templates, geometry naming for new kernels, new
create-reference/create-skillskills, and broad learned-rules updates; oxlint ignores generated GeoSpec OpenCascade natives.Reviewed by Cursor Bugbot for commit a02c4e9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes & Removals