Import Universal v1.0.0#1111
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to lay groundwork for cross-platform (Windows/macOS/Linux) support in gstack by adding new setup/build tooling, path/binary discovery utilities, and extensive migration guidance/docs.
Changes:
- Added cross-platform utilities (
lib/paths.ts,lib/binary-locator.ts) and updatedmake-pdf/designcode to use them. - Introduced new setup and build entrypoints (
setup.ts,setup.bat,scripts/build-binaries.*) and rewiredpackage.jsonbuild pipeline. - Added/expanded cross-platform documentation, guides, and example update files; added new lib-focused tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.ts | New Node-based setup script to build and register skills across agents/OSes. |
| setup.bat | New Windows batch setup alternative for users without Bash/WSL. |
| setup | Updates existing Bash setup script header with cross-platform guidance. |
| scripts/build-binaries.ts | New (but currently broken) TS build helper for platform-aware binary builds/aliases. |
| scripts/build-binaries.js | New JS build helper invoked by package.json to compile binaries and create aliases. |
| package.json | Updates build to call the new build-binaries script; adds build:binaries. |
| make-pdf/test/e2e/combined-gate.test.ts | Updates gate test to await async copyPasteGate. |
| make-pdf/src/setup.ts | Uses new path utilities and awaits async resolvePdftotext. |
| make-pdf/src/pdftotext.ts | Replaces macOS-centric lookup with cross-platform binary locator; makes APIs async. |
| lib/paths.ts | New cross-platform path helpers for home/config/temp and path operations. |
| lib/binary-locator.ts | New cross-platform executable discovery helper for Windows/macOS/Linux. |
| lib/tests/paths.test.ts | Adds unit tests for lib/paths.ts (currently mismatched to implementation). |
| lib/tests/binary-locator.test.ts | Adds unit tests for lib/binary-locator.ts (currently mismatched to API). |
| lib/tests/integration.test.ts | Adds integration-style checks for cross-platform migration (currently brittle/mismatched). |
| design/src/cli.ts | Replaces hardcoded paths with path utilities (but introduces separator issues). |
| browse/src/server.ts | Adds documentation comment for future claude binary discovery logic. |
| browse/src/cli.ts | Adds platform/arch constants and additional node-server bundle resolution logic. |
| README.md | Expands installation/prerequisite documentation for macOS/Linux/Windows. |
| EXAMPLE_MAKE_PDF_UPDATE.ts | Adds example showing how to migrate make-pdf to the new utilities. |
| EXAMPLE_BROWSE_UPDATE.ts | Adds example showing cross-platform binary execution patterns. |
| CROSS_PLATFORM_SUMMARY.md | Adds project status summary and phased roadmap for cross-platform work. |
| CROSS_PLATFORM_QUICK_REFERENCE.md | Adds user/dev quick reference for cross-platform patterns. |
| CROSS_PLATFORM_MIGRATION.md | Adds step-by-step migration guide and checklist for contributors. |
| CROSS_PLATFORM_CONTRIBUTING.md | Adds cross-platform coding guidelines and pitfalls to avoid. |
| CROSS_PLATFORM_ANALYSIS.md | Adds detailed report of current platform blockers and remediation plan. |
| CONTRIBUTING.md | Adds prerequisite documentation and points to README install section. |
| ARCHITECTURE.md | Adds Claude Code interface/platform support notes and points to tests for details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe("Cross-platform path handling", () => { | ||
| test("lib/paths.ts exports all required functions", () => { | ||
| const file = path.join(__dirname, "../paths.ts"); | ||
| const content = fs.readFileSync(file, "utf-8"); | ||
|
|
||
| const requiredFunctions = [ | ||
| "getHomeDir", | ||
| "getConfigDir", | ||
| "getTempDir", | ||
| "expandHome", | ||
| "normalizePath", | ||
| "getPathSeparator", | ||
| "isPathWithin", | ||
| "getClaudeSkillsDir", | ||
| "slugify", | ||
| ]; | ||
|
|
||
| for (const fn of requiredFunctions) { | ||
| expect(content).toContain(`export ${fn}`); | ||
| } | ||
| }); | ||
|
|
||
| test("lib/binary-locator.ts exports all required functions", () => { | ||
| const file = path.join(__dirname, "../binary-locator.ts"); | ||
| const content = fs.readFileSync(file, "utf-8"); | ||
|
|
||
| const requiredFunctions = [ | ||
| "findBinary", | ||
| "findBinaryOrThrow", | ||
| "getSearchPaths", | ||
| "getExecutableNames", | ||
| "isExecutable", | ||
| "findBinaries", | ||
| ]; | ||
|
|
||
| for (const fn of requiredFunctions) { | ||
| expect(content).toContain(`export ${fn}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
These “integration tests” do string matching that doesn’t reflect the actual TypeScript exports (e.g., expecting export ${fn} instead of export function ${fn}), and they expect isExecutable to be exported even though it’s private in lib/binary-locator.ts. As written, this suite will fail for reasons unrelated to runtime behavior. Prefer importing modules and asserting behavior, or at least align the string expectations with the real source code.
| "build": "bun run gen:skill-docs --host all && node scripts/build-binaries.js && (rm -f .*.bun-build || true)", | ||
| "build:binaries": "node scripts/build-binaries.js", |
There was a problem hiding this comment.
package.json now calls node scripts/build-binaries.js, but with "type": "module" that script must be ESM (or have a .cjs extension). As-is it uses require, so bun run build will fail immediately. Fix by converting the script to ESM or renaming it to .cjs and updating this script entry.
| "build": "bun run gen:skill-docs --host all && node scripts/build-binaries.js && (rm -f .*.bun-build || true)", | |
| "build:binaries": "node scripts/build-binaries.js", | |
| "build": "bun run gen:skill-docs --host all && node scripts/build-binaries.cjs && (rm -f .*.bun-build || true)", | |
| "build:binaries": "node scripts/build-binaries.cjs", |
| #!/usr/bin/env node | ||
| /** | ||
| * gstack setup — cross-platform installation script | ||
| * | ||
| * Builds browser/design/pdf binaries and registers skills with Claude Code and other agents | ||
| * Works on Windows, macOS, and Linux | ||
| * | ||
| * Usage: | ||
| * node setup.ts [--host claude|codex|kiro|factory|opencode|auto] | ||
| * ./setup-node.mjs [--host claude|codex|factory|opencode|auto] | ||
| */ |
There was a problem hiding this comment.
The header/usage says node setup.ts, but this repo is "type": "module" and Node will not execute a TypeScript .ts file by default. As written, #!/usr/bin/env node will fail on most systems. Consider shipping a .js/.mjs version (or using #!/usr/bin/env bun / documenting bun setup.ts / node --experimental-strip-types setup.ts), and make the README/usage consistent with the actual supported invocation.
| const configDir = getConfigDir(); | ||
| console.log("gstack design — AI-powered UI mockup generation\n"); | ||
| console.log("Commands:"); | ||
| for (const [name, info] of COMMANDS) { | ||
| console.log(` ${name.padEnd(12)} ${info.description}`); | ||
| console.log(` ${"".padEnd(12)} ${info.usage}`); | ||
| } | ||
| console.log("\nAuth: ~/.gstack/openai.json or OPENAI_API_KEY env var"); | ||
| console.log(`\nAuth: ${configDir}/openai.json or OPENAI_API_KEY env var`); | ||
| console.log("Setup: $D setup"); |
There was a problem hiding this comment.
Several paths are constructed via string concatenation with / (e.g., ${configDir}/openai.json). On Windows this will produce mixed separators and can break consumers or confuse users. Prefer path.join(configDir, 'openai.json') (and similarly for temp outputs) so paths are always valid for the current OS.
| @@ -120,7 +123,7 @@ async function main(): Promise<void> { | |||
| await generate({ | |||
| brief: flags.brief as string, | |||
| briefFile: flags["brief-file"] as string, | |||
| output: (flags.output as string) || "/tmp/gstack-mockup.png", | |||
| output: (flags.output as string) || `${getTempDir()}/gstack-mockup.png`, | |||
| check: !!flags.check, | |||
| retry: flags.retry ? parseInt(flags.retry as string) : 0, | |||
There was a problem hiding this comment.
Temp output defaults use ${getTempDir()}/... in several places. Since getTempDir() may return a path with backslashes on Windows, appending '/...' yields mixed separators (and can produce invalid paths). Use path.join(getTempDir(), 'gstack-mockup.png') (and for directories, path.join(getTempDir(), 'gstack-variants')).
| function prerequisitesAvailable(): { ok: true } | { ok: false; reason: string } { | ||
| if (!fs.existsSync(PDF_BIN)) return { ok: false, reason: `make-pdf binary missing (${PDF_BIN}). Run bun run build.` }; | ||
| if (!fs.existsSync(BROWSE_BIN)) return { ok: false, reason: `browse binary missing (${BROWSE_BIN}).` }; | ||
| if (!fs.existsSync(FIXTURE)) return { ok: false, reason: `fixture missing (${FIXTURE}).` }; | ||
| if (!fs.existsSync(EXPECTED)) return { ok: false, reason: `expected.txt missing (${EXPECTED}).` }; | ||
| try { resolvePdftotext(); } catch (err: any) { return { ok: false, reason: err.message }; } | ||
| // Note: pdftotext check is now async, so we skip it here | ||
| // It will be checked when the test actually runs | ||
| return { ok: true }; | ||
| } | ||
|
|
||
| describe("combined-features copy-paste gate", () => { | ||
| const avail = prerequisitesAvailable(); | ||
|
|
||
| test.skipIf(!avail.ok)("fixture PDF extracts cleanly through pdftotext", () => { | ||
| test.skipIf(!avail.ok)("fixture PDF extracts cleanly through pdftotext", async () => { | ||
| if (!avail.ok) return; // satisfies the type checker |
There was a problem hiding this comment.
prerequisitesAvailable() no longer checks for pdftotext, so on machines/CI where Poppler isn’t installed the test will now fail instead of being skipped (the file header explicitly says it should be “Skipped cleanly otherwise”). Consider adding a synchronous availability check (e.g., spawnSync('pdftotext', ['-v']) / which/where) or moving prerequisites detection into an async beforeAll and using test.skipIf based on that result.
| { | ||
| name: 'browse', | ||
| source: 'browse/src/cli.ts', | ||
| distDir: 'browse/dist', | ||
| baseNames: ['browse', 'find-browse-cli'], | ||
| }, | ||
| { | ||
| name: 'find-browse', | ||
| source: 'browse/src/find-browse.ts', | ||
| distDir: 'browse/dist', | ||
| baseNames: ['find-browse'], | ||
| }, | ||
| { | ||
| name: 'design', | ||
| source: 'design/src/cli.ts', | ||
| distDir: 'design/dist', | ||
| baseNames: ['design'], | ||
| }, | ||
| { | ||
| name: 'pdf', | ||
| source: 'make-pdf/src/cli.ts', | ||
| distDir: 'make-pdf/dist', | ||
| baseNames: ['pdf', 'make-pdf-cli'], | ||
| }, | ||
| { | ||
| name: 'gstack-global-discover', | ||
| source: 'bin/gstack-global-discover.ts', | ||
| distDir: 'bin', | ||
| baseNames: ['gstack-global-discover'], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
BINARIES has an extra duplicated block starting immediately after the closing ]; (line 73). This makes the file syntactically invalid TypeScript/JavaScript and will prevent it from running at all. Remove the stray duplicated objects / brackets so BINARIES is declared exactly once.
| { | |
| name: 'browse', | |
| source: 'browse/src/cli.ts', | |
| distDir: 'browse/dist', | |
| baseNames: ['browse', 'find-browse-cli'], | |
| }, | |
| { | |
| name: 'find-browse', | |
| source: 'browse/src/find-browse.ts', | |
| distDir: 'browse/dist', | |
| baseNames: ['find-browse'], | |
| }, | |
| { | |
| name: 'design', | |
| source: 'design/src/cli.ts', | |
| distDir: 'design/dist', | |
| baseNames: ['design'], | |
| }, | |
| { | |
| name: 'pdf', | |
| source: 'make-pdf/src/cli.ts', | |
| distDir: 'make-pdf/dist', | |
| baseNames: ['pdf', 'make-pdf-cli'], | |
| }, | |
| { | |
| name: 'gstack-global-discover', | |
| source: 'bin/gstack-global-discover.ts', | |
| distDir: 'bin', | |
| baseNames: ['gstack-global-discover'], | |
| }, | |
| ]; |
| const path = require('path'); | ||
| const { execSync } = require('child_process'); | ||
|
|
||
| const ROOT = __dirname.replace('/scripts', ''); |
There was a problem hiding this comment.
const ROOT = __dirname.replace('/scripts', '') is not portable: on Windows __dirname uses backslashes, so this replacement won’t match and ROOT will still include \scripts, breaking all relative paths. Prefer path.dirname(__dirname) (as the .js version already does) or path.resolve(__dirname, '..').
| const ROOT = __dirname.replace('/scripts', ''); | |
| const ROOT = path.dirname(__dirname); |
| if (process.platform === 'darwin') { | ||
| paths.push('/usr/local/bin'); | ||
| paths.push('/opt/homebrew/bin'); // Apple Silicon Macs | ||
| paths.push('/usr/local/opt/*/bin'); // Homebrew versioned packages |
There was a problem hiding this comment.
getSearchPaths() includes /usr/local/opt/*/bin, but this literal * is never expanded by path.join()/fs.stat(), so it can never match an actual directory and just adds noise to the search list. Either remove it, or implement real glob expansion (e.g., enumerate /usr/local/opt and append each <pkg>/bin that exists).
| paths.push('/usr/local/opt/*/bin'); // Homebrew versioned packages |
| #!/usr/bin/env node | ||
| /** | ||
| * gstack setup — cross-platform installation script | ||
| * | ||
| * Builds browser/design/pdf binaries and registers skills with Claude Code and other agents | ||
| * Works on Windows, macOS, and Linux | ||
| * | ||
| * Usage: | ||
| * node setup.ts [--host claude|codex|kiro|factory|opencode|auto] | ||
| * ./setup-node.mjs [--host claude|codex|factory|opencode|auto] | ||
| */ |
There was a problem hiding this comment.
PR description emphasizes “documentation additions and updates”, but this PR also introduces substantial executable/setup/build/test changes (new setup.ts, setup.bat, build pipeline refactor, new libs/tests). If the intent is broader than documentation, consider updating the PR description/title to reflect that, or splitting code changes into a separate PR to reduce review/rollout risk.
This pull request adds comprehensive documentation to support and guide the cross-platform development and migration of the gstack repository. The main focus is on identifying current macOS-centric limitations, outlining a remediation roadmap, and providing actionable guidelines for contributors to ensure Windows, macOS, and Linux compatibility.
Key documentation additions and updates:
Cross-platform architecture and compatibility
ARCHITECTURE.mddescribing Claude Code version/platform support and referencing integration testing for cross-platform implementation details.CROSS_PLATFORM_ANALYSIS.md) that identifies blockers (e.g., hardcoded Unix paths, Bash-only scripts, Mach-O binaries), provides a phased remediation roadmap, and specifies success criteria for universal platform support.Contributor guidance and onboarding
CONTRIBUTING.mdwith a prerequisites section, listing required tools and platform-specific setup notes for new contributors.CROSS_PLATFORM_CONTRIBUTING.md, a comprehensive guide for writing cross-platform code and scripts. It covers best practices, common pitfalls, platform detection, import patterns, file type guidelines, and a checklist for contributors to ensure compatibility.