skit: SyntaxKit codegen CLI for v0.0.5 by leogdion · Pull Request #156 · brightdigit/SyntaxKit

leogdion · 2026-05-12T10:06:26Z

Summary

This branch lands skit, a SyntaxKit CLI for config-driven Swift codegen. Two verbs:

skit run Input.swift            # render a SyntaxKit DSL file → Swift source
skit run InputDir/ -o OutDir/   # folder mode
skit parse < Input.swift        # parse Swift → JSON syntax tree

run is the default subcommand, so skit Input.swift works without a verb.

The PR unifies the previous skit (stdin → JSON parser, ~50 lines) and the POC skitrun (DSL renderer, ~700 lines) into one binary with ArgumentParser subcommands. Net replacement: one CLI, one product, declarative arg parsing, productized docs.

What's in

Unified skit CLI with run + parse subcommands (run is the default). Built on swift-argument-parser.
Wrap-and-spawn pipeline for run: parse the input with SwiftSyntax, hoist imports, wrap the rest in Group { … }, spawn script-mode swift, capture stdout, splice the output. #sourceLocation remaps body diagnostics back to the input file.
Helpers compilation cache — Helpers/ directories auto-discovered up-tree from the input, compiled once into libSyntaxKitHelpers.{dylib,so} and cached by content hash. --no-helpers / --helpers <dir> overrides.
Output cache — fully-rendered output cached by (input, helpers, libSyntaxKit stamp, swift version, sorted SKIT_*/SYNTAXKIT_* env) hash. Warm hit ≈ 0.14s, no swift spawn. --no-cache to skip.
Toolchain stamp + detect — bundle records lib/swift-version.txt at build time; skit run compares to local swift --version on startup and refuses to spawn swift on mismatch with a clear error and the rebuild command. --no-toolchain-check bypass.
Timeout watchdog — default 60s per-input timeout on the spawned swift. SIGTERM → 5s grace → SIGKILL. Exit code 124. --timeout 0 disables. Implementation uses DispatchSemaphore.wait(timeout:) to avoid Linux Foundation.Process.waitUntilExit() hangs.
Self-contained release bundle — Scripts/build-skit-release.sh produces .build/skit-release/{skit, lib/}. Portable: cp -r it anywhere, runs zero-config.
Linux verified — swift:6.0-jammy/aarch64. swift-crypto replaces CryptoKit; the Mach-O install_name rewrite is skipped. Concurrent pipe drain via DispatchGroup to avoid the 64KB pipe-buffer deadlock.
Examples — 10 of 11 Examples/Completed/*/dsl.swift files render end-to-end through skit. The 11th (enum_generator) was a full demo project shape rather than single-file DSL; moved to Examples/Demos/enum_generator/ where it doesn't masquerade as a completed example.
Docs — Docs/skit.md is the deep dive (architecture, caches, toolchain stamp, timeout, sharp edges, deferred items). Sources/skit/README.md is the per-target quick reference.

What's deferred

Tracked as follow-up issues, not blockers:

Hybrid bundle: rebuild SyntaxKit from sources on toolchain mismatch #157 — Auto-rebuild on toolchain mismatch. Today users get a clear error and a rebuild command; tomorrow skit bundles SyntaxKit sources and rebuilds transparently per-Swift-version when the stamp doesn't match. The stamp-and-detect path in this PR is the foundation.
if/else inside Group { … } builder triggers Swift type-checker crash #158 — if-in-Group Swift type-checker crash. Independent SyntaxKit bug (not skit's); the workaround is documented in Docs/skit.md (hoist the conditional into a helper that uses plain Swift if/else).

Explicitly out of scope for v0.0.5: multi-file output from a single input, sandboxing the spawned swift, an HTTP/web-server form. All discussed in Docs/skit.md under "What's deferred".

Sharp edges documented for users

@main and other top-level decl attributes don't work on DSL expressions (the wrapper places them inside a result builder where attributes can't bind to function calls). Users wanting @available etc. in the output should use the DSL's .attribute(…) method.
Comments in the input file don't carry through to the rendered output — they're for the input's author only. Emit output comments via .comment { Line(.doc, …) }.

Test plan

# Build the self-contained bundle.
Scripts/build-skit-release.sh

# Quick smoke test.
echo 'Struct("Hello") { Variable(.let, name: "name", type: "String") }' > /tmp/h.swift
.build/skit-release/skit /tmp/h.swift

# Sweep all 10 working examples.
for ex in card_game conditionals for_loops macro_tutorial attributes \
          concurrency blackjack errors_async protocols swiftui; do
  .build/skit-release/skit "Examples/Completed/$ex/dsl.swift" --no-cache >/dev/null
  echo "$ex: $?"
done

# Stdin → JSON.
echo 'struct Foo { let x: Int }' | .build/skit-release/skit parse

# Help text.
.build/skit-release/skit --help
.build/skit-release/skit run --help

Reviewer focus

Docs/skit.md — the design + as-shipped explainer. Best read first.
Sources/skit/Skit.swift — CLI surface via ArgumentParser.
Sources/skit/Runner.swift — the wrap-and-spawn pipeline + toolchain check.
Scripts/build-skit-release.sh — bundle layout and how the dylib + swiftmodules get staged.

The deferred items (#157, #158) and the documented sharp edges (@main, comments) are the natural discussion points; everything else should be a straight read.

🤖 Generated with Claude Code

coderabbitai · 2026-05-12T10:06:35Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a7ff9b76-7d47-496a-bef9-67ec42f9bd93

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch research/swift-manifest-codegen

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude · 2026-05-12T10:08:53Z

Code Review: skitrun POC (PR #156)

Reviewing as draft/research — focusing on correctness bugs worth fixing before step 4, design issues worth locking in now, and POC-aware feedback. Not flagging things the PR explicitly defers (timeout, caching, Linux smoke test).

Overview

Solid research output. The Tuist analysis is thorough, the design doc is well-reasoned, and the three POC steps de-risk the approach incrementally. The wrap+spawn architecture is the right call — it keeps the host dead-simple and avoids an AST round-trip. The #sourceLocation trick for diagnostic fidelity is a nice find and worth preserving.

The main concerns below are one real bug that can deadlock and one Swift concurrency misuse that can exhaust the thread pool under load. Both are fixable before step 4.

Bug: Pipe deadlock in `runSwift` (high severity)

Sources/skitrun/Main.swift — runSwift function (~line 1255)

try process.run()
process.waitUntilExit()

let stdoutData = stdoutPipe.fileHandleForReading.readDataToEndOfFile()
let stderrData = stderrPipe.fileHandleForReading.readDataToEndOfFile()

This is the classic Foundation.Process + Pipe deadlock. Pipe has a fixed kernel buffer (~64 KB on macOS). If the spawned swift process writes enough to stdout or stderr to fill the buffer before exiting, it will block waiting for the parent to drain — but the parent is blocked in waitUntilExit() and never drains. Deadlock.

For a compiler error on a large input or a large codegen output this is very reachable.

Fix: Read from both pipes concurrently with process execution:

let stdoutData = DispatchSemaphore(value: 0)
var stdout = Data()
var stderr = Data()
let stdoutQueue = DispatchQueue(label: "skitrun.stdout")
let stderrQueue = DispatchQueue(label: "skitrun.stderr")

stdoutPipe.fileHandleForReading.readabilityHandler = { fh in
    stdout.append(fh.availableData)
}
stderrPipe.fileHandleForReading.readabilityHandler = { fh in
    stderr.append(fh.availableData)
}

try process.run()
process.waitUntilExit()

stdoutPipe.fileHandleForReading.readabilityHandler = nil
stderrPipe.fileHandleForReading.readabilityHandler = nil
// drain any remaining bytes
stdout.append(stdoutPipe.fileHandleForReading.readDataToEndOfFile())
stderr.append(stderrPipe.fileHandleForReading.readDataToEndOfFile())

Or just kick each readDataToEndOfFile() onto a background Thread/DispatchQueue before calling waitUntilExit().

Bug: Blocking thread pool in `withTaskGroup` (medium severity)

Sources/skitrun/Main.swift — runDirectory / runOne (~line 963)

runOne calls processFile which calls runSwift which calls process.waitUntilExit() — a blocking, non-returning call. Inside a Swift concurrency TaskGroup, all tasks share the cooperative thread pool. Blocking a pool thread with a syscall starves other tasks and, with many files, can exhaust the pool entirely (Swift 6 strict concurrency mode will warn about this).

The cap (activeProcessorCount) helps but doesn't eliminate the problem — the Swift runtime may not schedule enough OS threads to compensate.

Fix: Bridge the synchronous spawn to async with withCheckedThrowingContinuation + a detached Thread, or use DispatchQueue.global().async with a continuation. Alternatively, keep withTaskGroup but run each processFile on a DispatchQueue worker via a continuation so the cooperative pool isn't blocked:

func runSwiftAsync(wrappedPath: String, libPath: String) async throws -> ProcessResult {
    try await withCheckedThrowingContinuation { continuation in
        DispatchQueue.global().async {
            do {
                let result = try runSwift(wrappedPath: wrappedPath, libPath: libPath)
                continuation.resume(returning: result)
            } catch {
                continuation.resume(throwing: error)
            }
        }
    }
}

Design: `_`-prefix skip applies only to filenames, not parent directories

Sources/skitrun/Main.swift — collectInputs (~line 1044)

guard !url.lastPathComponent.hasPrefix("_") else { continue }

dir/_Helpers/Foo.swift would NOT be skipped — only files whose own name starts with _. If the helpers convention (design §4) intends _-prefixed directories as private scopes, the skip rule needs to check all path components, not just the last one.

Suggest: check every path component between inputDir and the file, or document that _ applies to filenames only (and name shared helpers files _Shared.swift, not directories).

Fragile: path separator `+1` in relative path computation

Sources/skitrun/Main.swift — runDirectory (~line 980)

let relative = outcome.input.path.dropFirst(inputURL.path.count + 1)

+1 assumes the separator is exactly one character after the prefix, and that inputURL.path doesn't end with /. If inputURL.path ends with a slash (e.g. when the user passes ./dir/), the +1 eats the first character of the relative path.

Use URL-native relative path stripping instead:

let relative = outcome.input.path.dropFirst(
    inputURL.path.hasSuffix("/") ? inputURL.path.count : inputURL.path.count + 1
)

Or better, use URL(fileURLWithPath: outcome.input.path, relativeTo: inputURL).relativePath.

Missing: tests for `wrap(source:originalPath:)`

wrap is internal and is the only pure-function piece of the whole pipeline. It has no tests. This is the most testable thing in the codebase — it takes a String, returns a String, no I/O. A handful of unit tests (empty input, import-only input, body-only input, mixed imports + body, non-ASCII path) would catch regressions when the wrapping format changes.

The function's correctness is load-bearing for diagnostic fidelity (the #sourceLocation offset) — worth covering before it gets more complex.

Minor: `ProcessResult` type asymmetry

private struct ProcessResult {
  let exitCode: Int32
  let stdout: Data     // binary-safe
  let stderr: String   // decoded at capture site
}

Treating stdout as Data and stderr as String makes sense given usage (stdout is written verbatim, stderr is string-processed for path rewriting), but it's easy to trip over. A comment on each field stating why would help the next reader.

Minor: `-framework SyntaxKit` / `-F` flags missing from `runSwift`

The design doc (§3) shows both -lSyntaxKit and -framework SyntaxKit / -F flags; the implemented runSwift only passes -I/-L/-lSyntaxKit. This is likely intentional for the POC (dylib-only, no framework bundle), but it diverges from the design doc. Worth a note so step 4's bundled-binary work doesn't miss the framework-search-path case.

Minor: default `--lib` is POC-only, correct to flag early

The hardcoded /tmp/syntaxkit-poc/lib default will confuse anyone who runs skitrun without first running poc-step1.sh. Since this is a draft, it's fine — but step 4's ResourceLocator should make --lib optional (bundle-relative default) before any wider exposure.

Shell script

Docs/research/poc-step1.sh is well-structured: set -euo pipefail, cleanup trap on EXIT, Python3 for the Package.swift mutation. One nit: the python3 - heredoc uses string matching on Package.swift formatting — if SwiftFormat ever reflows the library product block, the script silently exits non-zero via sys.exit(...), which is fine, but a confirming echo after the patch would make failures easier to diagnose in CI.

Design questions (pre-step-4 bikeshed)

Per the PR's ask:

Naming: Weak preference for syntaxkit run as a subcommand on the existing skit binary rather than a new binary. The use cases don't overlap today, but both are "tooling that operates on SyntaxKit files" — a single entry point keeps installation simpler. That said, skit is currently stdin-driven JSON and that's a very different UX from a codegen runner, so the separate-binary argument is real.
_-prefix convention: Fine as a filename convention. The Tuist parallel (Helpers/ directory, not _Helpers/) is probably the cleaner split since it's directory-based rather than name-based, and avoids the question of whether the skip propagates through subdirectories. But _-prefix is simpler to implement and document.
Folder layout: Mirroring input paths into the output tree is the right default. The one gap is what happens when two input files produce the same output path (e.g. a/Foo.swift and b/Foo.swift both map to out/Foo.swift if the input root is chosen wrongly). Folder mode preserves relative paths so this isn't a problem given correct usage, but a clear error when destination paths collide would be good.

Summary: One deadlock bug, one thread pool issue, and one fragile path join to fix before step 4. Everything else is POC-appropriate. The design is sound.

codecov · 2026-05-12T10:12:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (v0.0.5@508bcca). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             v0.0.5     #156   +/-   ##
=========================================
  Coverage          ?   78.04%           
=========================================
  Files             ?      127           
  Lines             ?     4491           
  Branches          ?        0           
=========================================
  Hits              ?     3505           
  Misses            ?      986           
  Partials          ?        0

Flag	Coverage Δ
spm	`77.82% <ø> (?)`
swift-6.1	`77.97% <ø> (?)`
swift-6.2	`77.84% <ø> (?)`
swift-6.3	`77.84% <ø> (?)`
ubuntu	`78.02% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

claude · 2026-05-12T10:19:28Z

Code Review — `skitrun` POC (#156)

This is a well-scoped research PR with a clean design. The "wrap into Group { … } and spawn swift" approach is the right call — it keeps the host process small and avoids any AST serialization round-trip. Notes below are ordered by impact.

Bugs / Correctness

1. Blocking waitUntilExit() inside Swift concurrency (withTaskGroup)

runSwift calls process.waitUntilExit(), which blocks the calling OS thread. Since runOne is called from withTaskGroup tasks, this runs on Swift's cooperative thread pool — blocking those threads starves other tasks and can cause deadlocks under high concurrency.

Fix options:

Wrap the Process lifecycle in withCheckedThrowingContinuation + NotificationCenter/terminationHandler:

process.terminationHandler = { p in continuation.resume(returning: p.terminationStatus) }
try process.run()
let exitCode = await withCheckedContinuation { continuation in ... }

Or mark runSwift / processFile as nonisolated and dispatch to a global DispatchQueue via Task { await Task.detached(priority: .userInitiated) { ... }.value }.

For a POC this is low-risk at small batch sizes, but at activeProcessorCount concurrency the thread pool can still be depleted.

2. Non-atomic output write in single-file mode (Sources/skitrun/Main.swift, runSingleFile)

try result.stdout.write(to: URL(fileURLWithPath: outputPath))

Data.write(to:) is not atomic by default. A crash mid-write leaves a partial file. Use:

try result.stdout.write(to: URL(fileURLWithPath: outputPath), options: .atomic)

The folder-mode path (processResult.stdout.write(to: destination)) has the same issue.

Platform Portability

3. isLibDir is macOS-only

return fm.fileExists(atPath: "\(path)/libSyntaxKit.dylib")

.dylib is a macOS extension. On Linux the file would be libSyntaxKit.so. The poc-step1.sh script also exits early with "This reproducer is macOS-only." — fine for now, but isLibDir will silently reject a valid Linux lib dir when step 7 lands.

Suggestion: check for both extensions, or accept any file matching libSyntaxKit.*:

let candidates = ["libSyntaxKit.dylib", "libSyntaxKit.so"]
return candidates.contains { fm.fileExists(atPath: "\(path)/\($0)") }

Design / Edge Cases

4. wrap: imports are only hoisted when they appear before the first non-import statement

From the current logic, a user who writes:

Struct("Foo") { ... }
import MyHelpers    // after a DSL statement

will get import MyHelpers left in the body inside Group { … }, which Swift will reject (import declarations must appear at the top level). The failure message from swift will be cryptic.

Worth adding a runtime check in wrap: if any ImportDeclSyntax is found after firstBodyByte, emit a warning to stderr pointing the user at the hoisting rule.

5. Reserved variable name collision

The generated wrapper uses __skitrun_root. If the user happens to define a variable with that name in their DSL file, the compiler will emit a confusing duplicate-symbol error. A UUID suffix (__skitrun_\(UUID().uuidString.replacingOccurrences(of: "-", with: "_"))) would make collisions impossible, at the cost of reproducibility in diagnostics.

6. No timeout on spawned swift process

The design doc (§7) specifies 60 s default + SIGTERM → SIGKILL. The implementation has no timeout at all. Not blocking for a POC, but worth a // TODO: add --timeout (see codegen-cli-design.md §7) comment so it doesn't get forgotten.

Minor / Style

7. --help writes to stderr

case "-h", "--help":
    FileHandle.standardError.write(Data(helpText.utf8))
    exit(0)

Convention for most Unix tools is --help → stdout (exit 0), --help on error → stderr (exit 2). Writing help to stderr makes it harder to pipe or page (skitrun --help | less won't work). Easy fix: use FileHandle.standardOutput.

8. @main on enum SkitRun

Valid Swift, and deliberately chosen to prevent accidental instantiation — that's a reasonable use. Worth a one-line comment explaining the choice (// enum prevents instantiation; @main entry point) so the next reader doesn't wonder.

9. poc-step1.sh mutates Package.swift in-place

The trap cleanup EXIT + backup/restore is solid. The one gap: if the script is killed with SIGKILL (not SIGTERM/EXIT), the backup isn't restored. A .bak file left at $PACKAGE_BACKUP makes recovery easy to document: add a note in the script header that $PACKAGE_BACKUP can be used to restore manually.

Design Questions (from PR's "what I'd value review on")

Naming: skit run as a subcommand on the existing skit binary makes more sense than a separate skitrun binary if the two executables share any infrastructure (flags, lib-path resolution, etc.). Right now they don't — skitrun doesn't depend on SyntaxKit at all, while skit is a stdin-driven parser. The use cases are orthogonal, so a separate syntaxkit binary or a skit run subcommand both work. My vote is skit run to reduce the binary surface area, but the cases for a standalone syntaxkit are reasonable.

_-prefix skip rule: Clear, precedented (Swift itself uses _ for private-by-convention). Lock it in — no objection.

Folder layout: The Helpers/ adjacent-to-input sketch is reasonable. The only risk is ambiguity when a folder contains both Helpers/ and _-prefixed files — document the precedence (helpers directory is not walked, _-prefix files inside a walked directory are skipped).

Summary


Core logic	✅ Correct and well-structured
Blocking async	⚠️ Should be addressed before prod
Atomic writes	⚠️ Easy fix, do it now
Linux portability	⚠️ Needed for step 7
Import hoisting edge case	📝 Worth a warning
Timeout	📝 Add TODO comment
Help to stderr	📝 Minor

Good research PR — the design is sound and the POC ladder gives a clear path forward. The blocking waitUntilExit and non-atomic writes are the two items worth fixing before step 4 (bundled release).

claude · 2026-05-12T12:36:23Z

Code Review — PR #156: Research + POC: `skitrun` codegen CLI

This is a draft/POC, so the review focuses on design soundness and the issues worth resolving before the code graduates to a real target, rather than nit-level style polish.

Overview

The PR delivers a working three-step pipeline: parse a pure-DSL input file with SwiftParser, splice it into a Group { … } + print(…generateCode()) wrapper, and spawn swift against a pre-staged libSyntaxKit.dylib. The helpers caching and folder-mode concurrency work end-to-end. The design is coherent and the POC status is clearly communicated — this is genuinely useful exploratory work.

Design Feedback

Wrap shape is correct. The import-hoisting + #sourceLocation remapping is the right approach. Diagnostic paths pointing at the input file is a first-class concern and it's handled from the start — good.

skitrun not depending on SyntaxKit is an important property worth preserving. The host is just a compiler driver; any coupling to SyntaxKit's type system would make version pinning painful. Keep it.

Naming. Agree with the author's weak preference for a separate syntaxkit binary over a run subcommand on the existing skit. The stdin-driven JSON-parser shape of skit is completely different from a compiler-driver shape, and sharing a binary would mean shipping SyntaxKit as a hard dependency of the CLI host — contradicting the "host is dependency-free" property above.

_-prefix as the private-helpers skip rule is non-standard Swift. Swift convention for "skip this" in other contexts is . (dotfiles) or out-of-tree placement, not underscore-prefix. Before locking in step 4–5, consider making it configurable via a .skitrunignore file or defaulting to the .-prefix rule that most tools already honour. The current approach will silently eat any file a user name-prefixes with _ for unrelated reasons (e.g. _generated_output.swift placed in the helpers directory by another tool).

Security Concerns

Process argument injection — low risk but worth noting.
Main.swift passes paths to the swift / swiftc invocations as [String] array elements (not shell-interpolated strings), which is safe. The risk would appear if a future change switches to Process's launchPath = "/bin/sh" + arguments = ["-c", "swift \(path)"] form — add a comment to runSwift and compileHelpers making the safe calling convention explicit so it isn't accidentally regressed.

Library path from user-controlled source.
--lib accepts an arbitrary directory and the dylib from that path is loaded into the spawned child, not the host. The risk is bounded to the child process, which already runs arbitrary user Swift code. Not a blocker, but worth documenting.

Cache directory permissions.
Helpers.swift uses ~/Library/Caches/com.brightdigit.SyntaxKit/helpers/<hash>/ without explicitly setting permissions. FileManager.createDirectory defaults to 0755 which is fine for a single-user developer tool, but the intention should be explicit.

Platform Support

~/Library/Caches/ is macOS-only.
Helpers.swift hard-codes FileManager.default.urls(for: .cachesDirectory, ...). On Linux this resolves to ~/.cache/ via Foundation emulation but the behaviour isn't guaranteed across Swift toolchain versions and the module directory may use .dylib extensions vs .so. Since POC step 7 (Linux smoke test) is pending, at minimum add a #if os(macOS) guard with a fatalError("Linux not yet supported") so CI doesn't silently produce broken artifacts.

.dylib extension is hard-coded in resolveLibPath. The validation check libSyntaxKit.dylib will fail on Linux where the artifact is libSyntaxKit.so. Abstract this behind a dynamicLibraryExtension computed property.

Error Handling

CLIArgs.parse exits on error via fputs + exit(1) rather than throwing. This is pragmatic for a CLI entry point but makes parse untestable in isolation. Consider keeping the throwing form internally and having main() catch and print — the pattern try CLIArgs.parse(...) catch { fputs(...); exit(1) } composes better with future tests.

Helpers build-race handling (concurrent invocations write to tmp.<pid>.<uuid>/ then atomic-rename) is a solid approach. One gap: if two processes hash to the same cache key but one crashes mid-compile, the partial temp directory is leaked. Add a cleanup on compileHelpers failure.

Performance

Concurrency cap at ProcessInfo.activeProcessorCount is the right instinct but can overwhelm CI machines where activeProcessorCount is reported as the container vCPU count while disk I/O is the actual bottleneck. Consider capping at min(ProcessInfo.activeProcessorCount, 8) as a safe upper bound.

dylib mtime as cache-key proxy for version is fragile — a touch or a cp -p that preserves mtime but changes content would produce a stale cache hit. Use the dylib's SHA-256 hash instead of mtime (the file is small; hashing it is fast).

Test Coverage

This is the main gap for graduation from POC. The shell scripts in Docs/research/ cover integration-level smoke tests but the following units are testable today and would catch regressions during refactor:

wrap(source:originalPath:) output shape — given a two-line input, assert the generated wrapper has the correct #sourceLocation line numbers and the imports are hoisted.
CLIArgs.parse — assert --lib, -o, --no-helpers, directory-input-requires-output, help text.
helpersCacheKey() — assert that changing a helper source byte changes the hash.
discoverHelpersDir(near:) — assert walk-up terminates correctly at the filesystem root.

Adding these under Tests/SyntaxKitTests/Unit/SkitRun/ would align with the existing test layout.

Minor Issues

internal access level on SkitRun and most helpers — correct for an executable target; no change needed, just confirming intent.
No timeout on spawned swift — acknowledged in PR. Before step 4 ships, add a 60-second SIGTERM → 5s → SIGKILL sequence (or use Task.sleep + process.terminate()). A runaway script will otherwise silently hang the caller.
Folder mode output tree creation is done per-file. If two concurrent tasks try to createDirectory for the same intermediate subdirectory the second will get an "already exists" error — ensure withIntermediateDirectories: true is set (standard FileManager call handles this, just confirm).
poc-step1.sh mutates Package.swift in-place with a sed-based flip and a trap revert. This is fragile under Ctrl-C before the trap fires and under parallel swift build invocations. For a reproducible benchmark script, copy Package.swift and restore from the copy rather than relying on a reverse-sed.

Summary

Area	Status
Core design (wrap + spawn)	✅ sound
Helpers caching	✅ solid; dylib mtime → SHA-256 recommended
Folder-mode concurrency	✅ works; cap at `min(cpu, 8)` recommended
Platform (Linux)	⚠️ macOS-only; needs guard before step 4
Security	✅ low risk; add call-convention comment
Test coverage	⚠️ no unit tests yet; four units are ready to add
Naming / conventions	⚠️ `_`-prefix skip rule — reconsider before locking in
Timeout on spawned process	⚠️ acknowledged gap; block step 4 on this

The three POC steps that are done are genuinely solid. The above are the things I'd want resolved before step 4 (release bundle) ships, since they become harder to change once the CLI has users.

🤖 Generated with Claude Code

claude · 2026-05-12T12:49:59Z

Code Review — PR #156 (Research + POC: skitrun codegen CLI)

This is a well-documented research PR with solid POC implementation. The design decisions are clearly reasoned and the trade-offs are explicitly called out. Below is a detailed review across the areas you flagged and the things I noticed independently.

Overview

The PR delivers a working skitrun POC CLI that wraps pure-DSL SyntaxKit input files, splices them into a Group { … } wrapper, and spawns swift to produce rendered Swift source. Steps 1–6 of the ladder are complete. The research docs (Tuist comparison, step results) are unusually thorough and will be a valuable reference.

Design Correctness

Wrap-and-spawn is the right shape for v1. The key insight — that the host process only needs to assemble flags, not parse ASTs — keeps skitrun tiny and keeps SyntaxKit itself out of the CLI's dependency graph. The #sourceLocation trick for diagnostic remapping is elegant and avoids manual stderr arithmetic.

The departure from Tuist's token-delimited stdout is justified. SyntaxKit's output is Swift source, not a JSON descriptor that the host needs to reinterpret, so there's no need for the MANIFEST_START/END envelope. The design doc makes this explicit.

Naming. The skitrun POC name is fine for a branch. For the final shape, I'd lean toward a run subcommand on the existing skit binary rather than a new top-level binary. Rationale: skit already handles Swift parsing tasks, users install one binary, and discoverability is better (skit --help surfaces run). The concern about overlapping use cases is real but manageable via subcommand help text.

Code Quality

Helpers.swift — discoverHelpersDir(near:)

The walk-up-to-root logic is correct, but the loop invariant is subtle. An explicit comment explaining that parent.path == dir.path detects the filesystem root would help future readers:

// dir.deletingLastPathComponent() returns itself at the filesystem root
if parent.path == dir.path { return nil }

Package.swift modification in shell scripts

The inline Python that string-matches the exact library product block is fragile. Three separate scripts duplicate the same old/new block verbatim. If Package.swift is reformatted (SwiftFormat 602 will do this), all three scripts break silently at the if old not in src: sys.exit(...) guard. Suggest extracting this into a single shared Scripts/make-dynamic.py invoked from all three scripts. Better yet, leave a // MARK: dynamic-library-toggle comment in Package.swift and key the Python off the comment anchor rather than the exact whitespace.

Shell scripts — repeated staging boilerplate

poc-step1.sh, poc-step4-release.sh, poc-step5.sh, and poc-step6.sh all repeat the same 30-line pattern: backup Package.swift, flip library type, build, stage to /tmp/…. For a POC this is fine, but if any of these scripts graduate to CI they should be refactored.

OutputCache.swift — libStamp proxy

The step 6 results note this: size + mtime is a coarse proxy that a deterministic rebuild can defeat. The correct long-term fix (embed a version constant in the bundle) is the right call. Worth leaving a // TODO: in the code to prevent future maintainers from treating the proxy as intentionally sufficient.

Potential Bugs / Issues

1. CryptoKit on Linux (blocking for step 7)

Helpers.swift imports CryptoKit, which is macOS-only. The step 6 results explicitly flag this:

"CryptoKit is macOS-only; Linux will need swift-crypto or a small fallback hash impl behind a #if canImport(CryptoKit) shim."

This needs to be resolved before step 7. The cleanest option is adding swift-crypto as a dependency and using it unconditionally rather than the shim approach, since swift-crypto is already in the SwiftSyntax/SyntaxKit ecosystem.

2. if-in-Group type-checker crash (#155)

Already filed separately. Worth noting for reviewers: this is a blocker for users wanting conditional codegen, which is a common pattern. The underlying issue (variadic any CodeBlock overloads + buildEither overload resolution) should be prioritized before skitrun ships publicly, since the error message ("failed to produce diagnostic for expression") will be deeply confusing to users.

3. Walk-up false positives for Helpers/

Step 5 results note this: an unrelated Helpers/ directory higher in the tree will be picked up silently. The --helpers <dir> escape hatch exists, but the failure mode is confusing (unexpected helpers compiled into the invocation). Consider requiring a sentinel file (Helpers/.syntaxkit) or at minimum logging which helpers directory was discovered at a verbose log level.

4. Import-line diagnostic offset

Step 5 results: #sourceLocation wraps the body but not the hoisted imports, so errors on user import lines are off by one. Low priority but worth a // FIXME: comment in the wrapper generation code.

5. No timeout on the spawned swift process

The design doc (§7) calls for a 60s default with SIGTERM → 5s grace → SIGKILL. This is not yet implemented. For a POC it's fine, but this should land before any public release — an infinite-looping codegen script will block a build step indefinitely.

6. Bundle includes SwiftSyntax twice (45 MB total)

Step 4 results identify this: SwiftSyntax is linked statically into skitrun (for input parsing) and dynamically via libSyntaxKit.dylib. The fix (make skitrun share a dynamic SwiftSyntax with the SyntaxKit dylib) is non-trivial but important for the "feels small" download goal. Could be mitigated in the near term by making skitrun's input parsing use the same dynamic SwiftSyntax rather than its own static copy.

Performance

The measured numbers are good:

Cold: ~720ms (dominated by dylib loading, acceptable for a generator)
Warm: ~110ms
Output cache hit: ~140ms (no swift spawn)

The parallelism in folder mode is working (3 files in 1.41s vs. 3×0.72s = 2.16s serial). The observation that per-file compile work isn't deduplicated across a batch is correct — the right long-term fix (a single long-lived swift process servicing all inputs) is architecturally complex and appropriately deferred to post-v1.

Security

No sandboxing for v1. The design doc acknowledges this and the Tuist comparison shows sandbox-exec is the right reference implementation. For v1 (user's own repo), this is acceptable — users who run skitrun Input.swift have the same trust model as running swift Input.swift directly. The concern rises if/when this runs in CI against third-party contributions.

Package.swift is modified at build time in scripts. The trap-based backup/restore is correct but fragile under power loss or hard kill. The cleanup() trap is registered before the modification, so ordinary exits (including signals caught by bash) are handled correctly. This is fine for a POC.

Test Coverage

No automated tests for skitrun itself. For a POC this is expected. When this graduates from research status, the following should be added:

Unit tests for discoverHelpersDir(near:) — particularly the walk-up-to-root edge case and the directory vs. file input handling.
Unit tests for the wrapper generation (#sourceLocation injection, import hoisting).
Integration tests using the existing swift test harness: spawn skitrun against known inputs and assert stdout matches expected output.
Tests for OutputCache cache key stability (same input → same key, mutated input → different key).

The bash POC scripts serve as manual integration tests today and are clearly written, which is helpful.

Folder Layout Conventions

The _-prefix skip rule is clean and consistent. The Helpers/ adjacency convention is intuitive. Both are worth locking in for steps 4–5. One bikeshed worth raising: the helpers module name SyntaxKitHelpers is hardcoded. If users want multiple helper modules or a project-specific name, this becomes a limitation. For v1 hardcoding is fine — just ensure it's documented as a known constraint.

Summary

The POC is solid and the design is well-reasoned. The key things to address before merging (or continuing to v1):

Priority	Item
Blocking (step 7)	CryptoKit → swift-crypto for Linux
High	Process timeout (60s + SIGTERM/SIGKILL)
High	`if`-in-`Group` crash (#155)
Medium	Walk-up helpers false-positive mitigation
Medium	Bundle size (duplicate SwiftSyntax)
Low	Shell script Package.swift manipulation fragility
Low	Import-line diagnostic offset fix

The design doc, step results, and POC shell scripts are all worth keeping as permanent research artifacts — they'll be the canonical reference when the feature is revisited.

🤖 Generated with Claude Code

claude · 2026-05-12T12:52:24Z

Code Review — PR #156: `skitrun` POC

This is a research/POC PR (explicitly not for merge as-is), so the review focuses on issues worth resolving before the design graduates to production code, plus a few things that are already well-done.

Overview

The PR introduces a skitrun CLI executable that wraps pure-DSL SyntaxKit input files in a Group { … } closure, spawns swift, and captures the rendered output. Three Swift source files are added (Main.swift, Helpers.swift, OutputCache.swift), a new executable target is wired up in Package.swift, and twelve research/results docs are added under Docs/research/.

The design property of not linking SyntaxKit into the host binary is smart — it keeps the CLI small and avoids the messy version-pinning problem that Tuist has with ProjectDescription.framework.

Code Quality & Correctness

captureSwiftVersion() / libStamp() are called twice per invocation.
Both helpersCacheKey() and outputCacheKey() call these independently. Each captureSwiftVersion() call spawns a swift --version subprocess; libStamp() does two attributesOfItem syscalls. Neither result changes during a single CLI run — memoize them (e.g., pass as String? parameters, or lazily compute once at startup).

No process timeout in runSwift() / compileHelpers().
process.waitUntilExit() blocks forever. The design doc already flags this as a known gap and suggests 60s / SIGTERM / 5s grace / SIGKILL. Worth adding before step 4 ships, because a hung swift invocation will hang the CLI with no user-visible feedback.

isLibDir() only checks for libSyntaxKit.dylib.
A partial installation (dylib present, .swiftmodule files or _SwiftSyntaxCShims-include/ missing) passes the check and fails later with a confusing missing required module '_SwiftSyntaxCShims' error. Adding a check for the C-shims directory would produce a much cleaner error:

private func isLibDir(_ path: String) -> Bool {
  let fm = FileManager.default
  var isDir: ObjCBool = false
  guard fm.fileExists(atPath: path, isDirectory: &isDir), isDir.boolValue else { return false }
  return fm.fileExists(atPath: "\(path)/libSyntaxKit.dylib")
    && fm.fileExists(atPath: "\(path)/_SwiftSyntaxCShims-include")
}

libStamp() uses mtime, which is fragile.
touching the dylib, restoring from a backup at the same mtime, or crossing a filesystem boundary can produce false cache hits or false misses. A SHA-256 of the first 1 KB of the dylib header (or even just the file size as a secondary check) would be more stable. The current approach is acceptable for a POC but worth hardening before the cache is used in CI.

__skitrun_root variable name in the generated wrapper.
Double-underscore names are reserved by the Swift compiler for internal use (__ prefixed names are generated for various compiler internals). A collision is unlikely in a function-level context, but _syntaxKitRoot or _skitrunOutput would be less risky and equally private.

wrap() doesn't strip a leading blank line from body.
If the input file has a blank line before the first non-import statement (common), the wrapper contains:

let __skitrun_root = Group {
#sourceLocation(file: "...", line: 3)

Struct("Person") { … }

The blank line is inside the Group closure but before the #sourceLocation, which is fine. However, any error on that blank line maps to line 3 of the original file (correct), but the blank line itself is at the wrong wrapper offset. Low priority, but worth testing explicitly.

runDirectory always prints "N/M succeeded" to stderr, even on full success.
For CI and piped usages this is noise. Consider only printing when failed > 0, or adding a --verbose flag that enables the summary in the success case.

Performance

Folder mode concurrency cap: ProcessInfo.activeProcessorCount.
Each swift spawn blocks a thread waiting on dylib loads (I/O-bound), not CPU. A cap of 2× or 3× activeProcessorCount might be more efficient, or letting the task group drain naturally without a manual semaphore (since each spawn blocks a thread, the Swift cooperative pool won't help here — you'd want actual OS threads or a hard cap via an AsyncSemaphore). The current sentinel-loop approach is correct and safe; the concurrency level is just a tuning knob to revisit.

Design

_-prefix skip rule vs. a _helpers or .skitignore convention.
The single underscore convention is clear for simple projects. Before locking it in at step 4, worth considering whether users will want to exclude test fixture files or generated files that happen to be in the same directory without renaming them. A .skitignore file or --exclude flag would be more ergonomic at scale, but is premature for v1.

helpersModuleName = "SyntaxKitHelpers" is hardcoded.
All projects share the same module name. If users ever have multiple helpers directories (e.g., per-subdirectory helpers), they'll collide at import time. Since the cache key isolates the compiled artefacts, there's no functional issue today, but the hardcoded name will become a constraint. At minimum, document this limitation.

Single-file output writes processResult.stdout with write(to:) (line ~1929) without atomicity — the output directory case uses FileManager directly. The single-file case (runSingleFile) uses result.stdout.write(to: URL(fileURLWithPath: outputPath)) which is also non-atomic. For consistency with the cache's staging + rename pattern, the output write should also be atomic (write to a temp file, rename). Concurrent skitrun invocations writing the same output path would race otherwise.

What's well-done

No SyntaxKit link in the host. This keeps cold-start at 0.72s instead of adding another framework to load.
#sourceLocation for diagnostic remapping — zero manual arithmetic, and it just works for the body range.
Atomic cache writes using staging + rename and the "loser drops their copy" semantics are solid for concurrent invocations.
resolveLibPath error message is genuinely helpful — it lists exactly what was searched and how to fix it.
The POC ladder table in the PR description makes it very easy to track what's production-ready vs. still WIP.
Research docs are thorough — the Tuist pipeline analysis in particular is a good reference for explaining design decisions.

Open issues worth filing before step 4 ships

Process timeout on runSwift / compileHelpers.
Atomic output write in runSingleFile and the folder-mode success path.
captureSwiftVersion() / libStamp() called twice — easy perf win.
isLibDir() should also verify the C-shims include directory.

The if-in-Group compiler crash (#155) is already filed and is independent of this PR.

🤖 Generated with Claude Code

claude · 2026-05-12T14:03:48Z

Code Review: PR #156 — `skitrun` POC CLI

Reviewed as a draft/research POC, not a merge-readiness review. Feedback is oriented toward the design questions raised in the PR description and the issues that should be addressed before step 4+ hardening begins.

Overview

This PR documents the Tuist manifest pipeline research and delivers a working skitrun POC that wraps pure-DSL .swift inputs into a Group { … } closure and spawns swift with a bundled SyntaxKit dylib. Steps 1–3 (hand-driven timing, CLI subcommand, folder mode + partial-failure semantics) are all implemented and appear to work end-to-end on macOS and Linux.

The documentation (tuist-manifest-pipeline.md, codegen-cli-design.md, and the three POC results files) is well-written and thorough. The key design decisions are well-motivated.

Design Correctness

The "pure-DSL input, wrap into Group { … }, spawn swift" shape is sound. Rejecting Tuist's Output(...) wrapper is the right call — it eliminates a deserialization layer that would add complexity without benefit, since the generated source is the output.

One concern: the Group { … } wrapping assumes the entire user input is a single CodeBlock-building expression sequence. If users write multi-file outputs via a single input script, the wrapper shape breaks. The PR already calls this out-of-scope, but it should be documented explicitly in the design doc as a constraint, not just an exclusion.

On naming: Strongly agree with separating skitrun from the existing skit binary. The use cases don't overlap — skit parses Swift→JSON, skitrun evaluates DSL→generated Swift. Merging them into a run subcommand would conflate two very different mental models. A standalone syntaxkit binary (or skit run as a clearly-scoped subcommand on a renamed top-level CLI) both seem reasonable; the important thing is the split.

Folder layout: The _-prefix skip rule is clear and conventional (mirrors Swift's _ protocol prefix for internal use). The Helpers/ adjacency convention is easy to discover. Both are fine to lock in at step 4.

Code Quality & Bugs

`Main.swift`

1. Process start failure leaves semaphore un-signaled (potential deadlock)

In the spawn function, terminationHandler is registered before process.run(). If process.run() throws, the handler is never called and the DispatchSemaphore.wait() below it blocks forever:

process.terminationHandler = { _ in semaphore.signal() }
try process.run()           // ← throws: handler never fires
// …
semaphore.wait()            // ← deadlocks

Fix: wrap process.run() in a do/catch and signal the semaphore in the catch block (or use a defer).

2. #sourceLocation path escaping is incomplete

The path is escaped for backslashes and quotes, but #sourceLocation(file:) also fails on paths containing \n or null bytes (unusual but possible on Linux). Use Foundation.URL.path percent-encoding, or simply reject input paths containing those characters with a clear error.

3. Import-line diagnostic offset

Hoisted imports aren't wrapped in their own #sourceLocation directive, so diagnostics in imports report the wrong line numbers. Low-severity for a POC, but easy to fix before users encounter it.

`Helpers.swift`

4. libStamp uses mtime + file size as a cache key proxy

This is a known weak proxy: deterministic rebuilds (same content, forced rebuild) produce the same mtime+size and won't invalidate the cache. The design doc acknowledges hashing the dylib bytes defeats cache efficiency, but the current proxy is silently incorrect in that case. Consider using the dylib's modification time truncated to second precision (which swiftc updates on any actual recompile) and document the known limitation explicitly in a comment.

5. Helpers directory walk-up has no depth limit

The walk-up from input path to find Helpers/ will traverse all the way to / if no helpers directory exists, stat-ing every ancestor. On a deep project tree this is harmless but slow. Cap the walk at a reasonable depth (e.g., 10 levels) with a diagnostic if the limit is hit.

6. swiftc invocation doesn't sanitize PATH

The helpers compiler spawns swiftc via resolved path, but the flags passed (e.g., -module-name) come from caller-controlled input (the helpers directory name). If the directory name contains shell metacharacters and the invocation path ever goes through a shell, this would be a command injection vector. Verify that Process arguments are passed directly (no shell interpolation) — if so, this is a non-issue; just worth confirming in a comment.

`OutputCache.swift`

7. Cache key omits output path

The SHA-256 key covers input source, helpers key, swift version, libStamp, and env vars — but not the output directory path. Two invocations writing to different -o directories with identical inputs will share a cache entry and the second will read the first's output (correct content, wrong path for path-dependent generated code). If the generated output ever embeds the output path (e.g., for #sourceLocation in the output), this will silently emit wrong paths. Recommend including the output path in the cache key.

Performance

Binary footprint: SwiftSyntax statically linked into skitrun (~17 MB) while also shipped as a dylib in lib/. For a developer tool this is acceptable, but it's worth noting in the release docs that users don't need to ship skitrun with their generated artifacts — only the generated .swift files.
Helpers cold-start (~2.96s): Unavoidable for first run; the cache makes subsequent runs fast. The step 5 design should document the warm/cold split prominently in the UX (e.g., a --verbose timing line).
Concurrent folder mode: The ProcessInfo.activeProcessorCount cap is sensible. Consider making it overridable via --jobs for constrained CI environments.

Security

No significant security concerns for a local developer tool. The lack of sandboxing is called out in the PR and is consistent with Tuist's own approach for v1.
The --lib flag and $SKITRUN_LIB_DIR env var accept arbitrary paths; document that pointing them at untrusted directories is equivalent to arbitrary code execution (standard disclaimer for plugin/dylib loaders).

Test Coverage

There are no automated Swift tests for skitrun — all coverage is via the shell script reproducers in Docs/research/. This is appropriate for a POC, but before step 4 the following should have unit tests:

wrapInput() (import hoisting + Group { } wrapping)
OutputCache key generation and atomic write/hit logic
Helpers directory walk-up and _-prefix filtering

The #155 bug (if-in-Group crashes the type checker) should have a regression test once fixed — it's exactly the kind of issue that would silently re-appear.

Summary

The POC is well-executed and the three open design questions (shape, naming, folder conventions) all have defensible answers. The main pre-step-4 blockers are:

Priority	Issue
Must fix	`Process` start failure → semaphore deadlock (#1 above)
Should fix	Output cache key missing output path (#7)
Should fix	Helpers walk-up depth limit (#5)
Nice to have	Import-line diagnostic offset (#3)
Document	`libStamp` weak-proxy caveat (#4), dylib loader trust boundary

Great research foundation — the Tuist pipeline doc in particular is a useful reference regardless of where the final CLI design lands.

🤖 Generated with Claude Code

claude · 2026-05-12T16:49:10Z

Code Review: `skitrun` POC CLI (PR #156)

This is a well-executed research spike. The POC ladder approach (retire one risk per step, measure, document) is exactly the right structure for this kind of architectural exploration. Notes below are calibrated to the draft/POC status and the explicit scope decisions called out in the PR description.

Overview

skitrun wraps a user's pure-DSL .swift file into a Group { … } closure, spawns swift with the bundled SyntaxKit dylib on the search path, and captures the generated code. Single-file and folder (concurrent) modes both work. The design deliberately avoids depending on SyntaxKit in the host process — the host only needs SwiftSyntax/SwiftParser for import-hoisting and swift-crypto for cache keying.

Strengths

Architecture is clean and minimal. The host does exactly three things: wrap, spawn, cache. No AST round-trip, no JSON envelope. The departure from Tuist's manifest framing is well-justified.
Cross-platform handling is thorough. The Process.waitUntilExit() hang on Linux, the pipe-buffer deadlock, the @rpath install_name Mach-O-only flag, and the CryptoKit → swift-crypto swap are all handled correctly and clearly commented.
Concurrency design is sound. withTaskGroup + activeProcessorCount cap + Sendable-clean FileOutcome is the right shape for bounded fork-join parallelism in Swift 6.
Partial-failure semantics in folder mode match what a user would expect from a build tool (all successes written, failures reported, non-zero exit).
#sourceLocation remapping is the right mechanism for diagnostic attribution. Good that it's validated end-to-end in step 2.
Documentation quality is high. The POC step results files read like proper engineering journals — timing measurements, observed vs. expected, follow-up actions. These are worth keeping even after the CLI ships.

Issues & Suggestions

1. Process timeout is a v1 blocker, not just future work

The PR description marks timeouts as "out of scope for v1," but I'd push back mildly: a hanging swift child (e.g., a DSL file that triggers an infinite type-checker loop — not hypothetical given issue #155) will silently hang skitrun forever with no feedback to the user. In folder mode, the withTaskGroup slot stays occupied, stalling all subsequent files.

A minimal timeout (e.g., 60s with a logged warning) using Task.sleep + task.cancel() or a DispatchWorkItem-based approach would make the tool usable in CI without a watchdog wrapper. Worth considering before step 4 (bundled-binary release) ships.

2. Helpers walk-up can silently pick the wrong directory

The current discovery walks up from the input file until it finds a Helpers/ directory. If the user's repo has an unrelated Helpers/ in a parent directory (common in many projects), skitrun will silently compile and link it. The PR notes this as a "false positive" risk in step 5 results, but the fix should land before users try it.

Suggested heuristic: only accept a Helpers/ that also contains at least one .swift file with import SyntaxKit at the top, or require the presence of a sentinel file (e.g., .skithelpers). Either approach is cheap to add and avoids confusing failures.

3. `@unchecked Sendable` on `PipeDataBox` — document the invariant more precisely

The comment says the box is needed because "raw local vars can't be Sendable." That's true, but the safety argument is incomplete: what makes this actually safe is that the box is only written from one DispatchGroup-leaf closure at a time (stdout reader vs. stderr reader), and the DispatchGroup.wait() provides the happens-before edge before the data is read. Worth adding that to the comment so future readers don't have to re-derive it.

4. `String(decoding: stderrData, as: UTF8.self)` silently replaces invalid bytes

String(decoding:as:) uses the Unicode replacement character (U+FFFD) for non-UTF-8 byte sequences rather than failing. In practice Swift's compiler output is always UTF-8, so this is low risk — but if a child process emits binary or latin-1 output on stderr, the diagnostic rewriting pass will produce garbled messages. Consider String(bytes: stderrData, encoding: .utf8) ?? "<non-UTF-8 stderr>" to make the degraded case explicit.

5. No automated tests for `skitrun`

The POC validation is entirely in shell repros + manual measurement. Before step 4 (release bundle), a small test suite would help:

Unit test wrap() with a known input → assert expected wrapper structure (imports hoisted, body preserved, #sourceLocation present at correct line).
Integration test: end-to-end processFile() with a trivial DSL input in a CI-available environment (macOS runner).

The wrap logic is pure and has no side effects, so unit testing it is straightforward without needing a live swift binary.

6. Cache eviction policy is absent

OutputCache writes entries and reads them but never evicts. Over time (many input files, multiple toolchain upgrades) the cache will accumulate stale entries. Cache keys include the toolchain version hash, so stale entries from old toolchains won't produce wrong output — but they will consume disk space indefinitely.

For v1, even a simple LRU eviction by mtime (drop entries not accessed in N days on cache write) or a --clean-cache flag would be sufficient.

7. Minor: `.build-linux/` in `.gitignore` is fine, but document why it's separate

The .gitignore addition is correct, but without context a future contributor might wonder why the Linux build output lives outside .build/. Worth a one-line comment in the gitignore or a note in the README.

Design Questions (per the PR's explicit asks)

Naming — skit run vs standalone binary

The PR's weak preference is a separate syntaxkit binary. I'd agree: skit today is stdin-driven Swift-to-JSON (a different enough use case that sharing a binary would confuse the UX). A separate syntaxkit run subcommand on a new top-level binary feels cleaner. The skitrun POC name is clearly a placeholder, so no strong objection to either direction — just worth deciding before step 4 locks the binary name into a release artifact.

Folder layout — _-prefix skip rule

The _-prefix convention is clear and matches existing Swift ecosystem conventions (e.g., SwiftPM's _ prefix for internal modules). No objection. Make sure it's documented in the README (it may already be — didn't verify).

Wrap shape — Group { … } vs Output(...)

The Tuist-style Output(...) wrapper alternative was correctly rejected. The Group { … } wrap is natural for a @resultBuilder DSL and doesn't require the user to know anything about the CLI's execution model.

Summary

The design is sound and the POC execution is solid. The three things I'd prioritize before marking step 4 as done:

A process timeout (even a rough one).
A Helpers/ walk-up guard heuristic.
A unit test for wrap().

Everything else is polish that can land incrementally. The Linux compatibility work in step 7 is particularly well-done.

🤖 Reviewed with Claude Code

claude · 2026-05-12T17:58:43Z

Code Review — PR #156: skitrun POC

This is a well-researched POC. The overall design (pure DSL input → wrap in Group { … } → spawn swift) is clean, the documentation is thorough and honest about gaps, and the Linux porting work (pipe drain order, waitUntilExit bug, install_name conditional) is correctly handled throughout. Observations below, ordered by severity.

Correctness Issues

1. captureSwiftVersion() spawns a subprocess on every outputCacheKey() call

In folder mode with N inputs, processFile is called N times, each computing a fresh outputCacheKey, each spawning swift --version. That's N extra subprocesses for something constant per invocation. Easy fix: compute it once at startup and thread it through as a parameter (or a lazy let at module scope using nonisolated(unsafe) let swiftVersionCache: String? = captureSwiftVersion()).

2. wrap() injects import SyntaxKit unconditionally — duplicate if user already imports it

If the user writes import SyntaxKit in their DSL file (to satisfy an IDE or linter), it'll be hoisted into hoisted[] and the wrapper preamble will inject a second one. Swift tolerates duplicate imports, but it produces a warning the user didn't write. Fix: filter "SyntaxKit" from the hoisted list before constructing the wrapper string, or only inject import SyntaxKit when it's not already in hoisted.

3. #sourceLocation path escaping is incomplete

let escapedPath = originalPath
  .replacingOccurrences(of: "\\", with: "\\\\")
  .replacingOccurrences(of: "\"", with: "\\\"")

A path containing a newline (unusual but legal on most filesystems) would break the #sourceLocation directive and produce a confusing compile error attributed to the wrapper. Worth adding a guard before the wrap:

guard !originalPath.contains("\n") else {
    throw CLIError(message: "input path contains a newline: \(originalPath)")
}

4. runOne blocks cooperative thread pool threads on semaphore

runDirectory uses withTaskGroup to schedule up to maxConcurrent runOne tasks. Each runOne calls processFile → runSwift, which blocks a cooperative thread pool thread on DispatchSemaphore.wait(). If all maxConcurrent tasks simultaneously reach the wait, every worker thread in the pool is blocked — Swift's cooperative scheduler can't preempt them. The concurrent pipe-drain further adds two DispatchQueue.global() async items per file.

For a POC this is fine. For productization, converting the inner Process launch to async via withCheckedContinuation (wrapping the semaphore signal) would let the structured concurrency scheduler actually breathe.

Minor Issues

5. __skitrun_root variable name collides with user-defined names (low risk)

let __skitrun_root = Group {

A user DSL file that defines let __skitrun_root = ... would shadow this. Using something like __skitrun_v1_output__ or using a UUID-derived suffix would eliminate the risk entirely.

6. discoverHelpersDir walks to filesystem root without bound

The loop terminates when parent.path == dir.path, which is the root. A deeply nested input in a project that has no Helpers/ directory will walk every ancestor all the way to /. Consider capping at a git root (git rev-parse --show-toplevel) or a fixed depth (e.g., 10 levels) and documenting the convention.

7. PipeDataBox uses @unchecked Sendable — subtle

The comment in poc-step7-results.md explains the reasoning well, but the class itself has no note about the invariant that makes it safe (only one writer, reads happen only after group.wait()). Adding a brief inline comment would help the next reader who considers adding a second mutation point.

8. swift-crypto for just SHA-256

On Apple platforms, CryptoKit.SHA256 is available in the SDK with no package dependency. swift-crypto is the right choice for Linux portability, but the step-7 results already call out the boringssl binary size hit. For the productized release, consider using #if canImport(CryptoKit) / #else to dispatch to the native framework on Apple and swift-crypto only on Linux. This keeps the macOS binary lean without breaking Linux.

Design Questions (per the PR's ask)

On naming (skit run subcommand vs separate binary)

My lean is skit run — both skit and skitrun are in the SyntaxKit DSL domain, and consolidating under one binary reduces the surface area users have to discover. The only argument for a separate binary is if the dylib/lib-path requirements make installation meaningfully different from the existing skit use case. If they do share users, a subcommand also makes skit --help self-documenting.

On the skip rule (_-prefix) and Helpers/ discovery

The _-prefix convention is consistent with Swift's own convention for "internal to this module" and is predictable. The Helpers/ upward-walk is powerful but could surprise users working in monorepos. Worth documenting that the search stops at the first Helpers/ found, not the nearest git root (users may expect git-root semantics). Consider explicitly calling this out in the README under a "How helpers are discovered" section.

On if-in-Group crash (issue #155)

This is the biggest user-visible hole. Any conditional codegen requires a workaround (computed vars in a helper, or Switch). Worth linking the issue from the Sources/skitrun/README.md "What won't work" section so users hit the issue before hitting the compiler's unhelpful diagnostic.

Test Coverage

There are no tests for the new code. For a POC that's fine, but wrap() in particular has enough branching (import hoisting, empty-body edge case, #sourceLocation injection, path escaping) that even a small table-driven @Test would catch regressions when the signature inevitably evolves. Same for CLIArgs.parse — the arg parsing is hand-rolled and has enough edge cases (flag order, missing values, unknown flags) to benefit from a few targeted tests.

API Changes in Examples

The examples show a cluster of breaking API changes that aren't explicitly called out in the PR description:

Variable(.var, ..., defaultValue:) → equals: rename
ComputedProperty now requires explicit type: label
Parameter → ParameterExp rename
Call("print", "string") → Call("print") { ParameterExp(unlabeled: Literal.string(...)) } (more explicit but more verbose)
Infix("lhs", "op", rhs) → Infix("op", lhs:..., rhs:...) (operator moved to first position, arguments labeled)
Bare let structExample = Group {...}; print(...) → top-level expression (required for skitrun input shape)

These are the right moves for skitrun input compatibility, but they'll all be breaking for existing SyntaxKit users if merged to main. Worth a dedicated changelog entry or a migration note when this leaves draft.

Summary: The POC ladder is complete and the research deliverables are solid. The concurrency issue (#4) and the captureSwiftVersion O(N) cost (#1) are the two things I'd fix before productizing. Everything else is polish or documentation.

claude · 2026-05-12T18:59:11Z

Code Review — PR #156: `skitrun` POC CLI

This is a well-documented research PR with a solid design foundation. The POC ladder is clearly articulated, the research docs are thorough, and the 7-step build-up is a good way to de-risk the approach incrementally. The notes below are forward-looking for when this moves toward merge rather than blocking the research phase.

Architecture & Design

Overall shape is sound. The "wrap into Group { … }, spawn swift, capture stdout" model is simpler than Tuist's token-delimited payload and maps cleanly to SyntaxKit's DSL semantics. The decision to not depend on SyntaxKit in the host binary keeps the host small and avoids the version-pinning pain Tuist hits with ProjectDescription.

swift-crypto is now a package-level dependency. Adding it in Package.swift dependencies: pulls it in for all consumers of SyntaxKit who resolve the package graph, even those who never use skitrun. Before finalising the package structure, consider either scoping it to the target only (it already is in targets:, but SwiftPM still resolves the top-level dependencies for all clients) or gating it with #if canImport(CryptoKit) on Apple platforms so CryptoKit is used instead — the Crypto wrapper exists exactly for this cross-platform case, but the package-level dependency is still added on Apple platforms. Worth a comment explaining why Crypto is used even on macOS.

Code Quality

Main.swift at 819 lines is too big. The file contains argument parsing, process spawning, directory enumeration, wrapping logic, and the @main entry point. The conventions in CLAUDE.md ask for focused, single-responsibility files. The wrapping logic (wrap()), argument parser (CLIArgs), and the spawn/timeout code could each live in their own file the way Helpers.swift and OutputCache.swift are split out.

PipeDataBox: @unchecked Sendable is a workaround that passes Swift 6 strict-concurrency checking without actually being safe — two threads write to .value without synchronisation. It works here because writes happen in DispatchGroup async blocks and .value is read only after group.wait(), but the compiler can't verify that. A (sendableData: Data) -> Void callback closure passed into each DispatchQueue.global().async block — or using withCheckedThrowingContinuation on Pipe — would be both safe and checkable.

captureSwiftVersion() is called up to three times per invocation (toolchain check, helpers cache key, output cache key). The result should be memoized at startup into a let on CLIArgs or passed as a parameter.

Indentation inconsistency in runDirectory (for await outcome in group body):

group.addTask {
runOne(                        // ← missing indentation
  next, libPath: libPath, ...
)
}

processResult.stdout.write(to: destination) is not atomic in folder mode. Data.write(to:) without .atomic option can leave a half-written file if the process is interrupted mid-write. Use .write(to:, options: .atomic).

Bug Risks

wrap() only hoists leading imports. If a user writes:

import SyntaxKit
Struct("Person") { ... }
import Foundation     // ← NOT hoisted

the second import stays in the body and the spawned swift fails with error: imports are not allowed after top-level code. This is a real footgun for incremental file editing. The fix is either a second pass to hoist all imports regardless of position, or a clear error message that explains the constraint when non-leading imports are detected.

#sourceLocation path injection. The path escaping in wrap():

originalPath
  .replacingOccurrences(of: "\\", with: "\\\\")
  .replacingOccurrences(of: "\"", with: "\\\"")

...does not handle newlines. A path containing \n#sourceLocation() would close the source-location directive early and potentially inject compiler directives. Paths with newlines are unusual but allowed by POSIX. Consider replacing \n, \r, and NUL bytes as well, or rejecting input paths that contain them at startup.

helpersExcludePath only skips a Helpers/ dir directly under inputDir, but discoverHelpersDir can find one anywhere up the tree. In folder mode, if the helpers dir lives at a parent level (e.g. project/Helpers/ and inputDir = project/inputs/), collectInputs won't exclude it — helpers sources won't be enumerated as inputs anyway (they're not under inputDir), but the asymmetry is confusing and could bite edge cases (e.g. inputDir = project/ where Helpers/ is directly under project/). Worth a comment or an explicit test.

Missing timeout for compileHelpers. DispatchSemaphore.wait() in compileHelpers has no deadline. A hung swiftc blocks the caller indefinitely. The same 60-second watchdog pattern used for swift script-mode should apply here.

Performance

maxConcurrent = activeProcessorCount is likely too high for this workload. Spawning N swift interpreter processes in parallel is memory-intensive (each cold-start loads a 9 MB dylib plus all swiftmodule files into a fresh process). On a machine with 10 cores that's potentially 10 simultaneous 720 ms cold-starts each loading ~100 MB of shared libs. A default of 2–4 would be safer; the design doc could expose --concurrency <n> alongside --timeout so users can tune.

libStamp() uses mtime + size for cache invalidation. mtime is not reliable after git checkout, tar extraction, or Homebrew bottle installs, all of which can set arbitrary timestamps. A SHA-256 of the first N bytes of libSyntaxKit.dylib (or of its LC_UUID if on macOS) would be more robust. The current approach will cause spurious cache misses and unnecessary recompiles after common packaging operations.

Test Coverage

The PR adds ~1,200 lines of production code with zero automated tests. Before leaving research phase, the following pure-logic functions would benefit from unit tests under Tests/SyntaxKitTests/Unit/ (no process spawning required):

wrap(source:originalPath:) — import hoisting, body passthrough, #sourceLocation placement, edge cases (empty file, imports-only file, leading comment)
CLIArgs.parse(_:) — flag combinator correctness, missing-value errors, unknown-flag errors, directory vs. file mode detection
discoverHelpersDir(near:) — walks up correctly, stops at filesystem root, finds adjacent dir
collectHelperSources(in:) — _-prefix skip, non-.swift skip
outputCacheKey determinism — same inputs → same key, changed input → different key

Minor / Bikeshed

The PR description mentions __syntaxkit_root but the code uses __skitrun_root. The description should be updated or vice versa.
helpText is a private let at file scope; it could move inside CLIArgs to reduce Main.swift's global surface.
The README.md under Sources/skitrun/ is good for discoverability, but CLAUDE.md asks to "prefer editing existing files" and "not create documentation files unless explicitly requested". Given this is a POC, the README is justified — just noting it for the production version.

Design Questions (as raised in the PR)

Naming. Agreed that skitrun is a placeholder. The strongest argument for a run subcommand on skit (rather than a separate binary) is that it reduces binary count in a Homebrew formula. The strongest argument against is that skit today is stdin/stdout-driven JSON parsing — a completely different use case — and conflating them produces a confusing --help. A separate syntaxkit or skit-run binary seems cleaner; the skit name could even be retired if syntaxkit subsumes it.

Folder skip convention. _-prefix is a recognisable Swift convention (private test helpers, _Internal targets). It's a good default. Would also suggest honouring a .skitrunignore or .gitignore-style file in a later step for finer control without renaming files.

Good research — the cold-start numbers (720ms cold / 110ms warm) are well inside acceptable latency for a codegen CLI, the design doc is thorough, and the identified blockers (#155 if-in-Group crash, toolchain ABI fragility) are called out honestly. The implementation is POC-quality as advertised; the notes above are mostly things to address before the done column on the POC ladder.

claude · 2026-05-12T19:30:18Z

Code Review: `skit` CLI for v0.0.5

TL;DR: This is a well-thought-out, clearly documented MVP for the skit CLI. The two-layer caching strategy, import hoisting, and timeout watchdog are all solid engineering choices. The main gaps are test coverage, missing CI integration, and undocumented DSL API changes visible in the examples.

Overview

The PR unifies the old stub skit (parse/JSON) and the POC skitrun (DSL renderer) into a single binary with two ArgumentParser subcommands (run and parse). The run path parses DSL input, wraps it in Group { }, hoists import declarations, spawns swift, and caches the result. Very clean architecture for a v0.0.5 milestone.

Strengths

Two-layer content-addressed cache (helpers + output) mirrors what Tuist does; the atomic-rename write strategy in both Helpers.swift and OutputCache.swift handles concurrent skit invocations correctly.
Timeout watchdog (SIGTERM → 5s grace → SIGKILL via DispatchSemaphore) is the right fix for the Linux Foundation.Process.waitUntilExit() hang; matches POSIX convention with exit code 124.
Toolchain stamp (swift-version.txt burned in at build time, checked at startup) prevents silent dylib ABI mismatches and provides a clean error + rebuild command.
Docs/skit.md is reference-quality — architecture, caches, sharp edges, and deferred items are all clearly explained. Great first-read for reviewers.
build-skit-release.sh is smart: temp-flips Package.swift to dynamic, rewrites Mach-O install_name, stamps toolchain, produces a cp-able bundle.

Issues & Suggestions

🔴 No test coverage for the new `skit` target

Runner.swift is 611 lines covering subprocess spawning, #sourceLocation injection, pipe draining, timeout, path rewriting, and folder-mode parallelism — none of it has unit or integration tests. At minimum:

wrap(): test import hoisting order and #sourceLocation injection with a few fixture inputs.
OutputCache / Helpers cache key generation: verify determinism and that a single-byte change in input busts the key.
Timeout watchdog: a mock Process that never terminates should trigger SIGTERM → SIGKILL.

Even a single integration test that runs skit Examples/Completed/card_game/dsl.swift --no-cache and asserts non-empty stdout would catch regressions.

🔴 No CI for `skit` / release bundle

There are no GitHub Actions changes visible in this diff. The release script and Linux path both need coverage:

# Suggested job addition in CI
- name: Build skit release bundle
  run: Scripts/build-skit-release.sh
- name: Smoke test skit
  run: .build/skit-release/skit Examples/Completed/card_game/dsl.swift

🟡 DSL API breaking changes undocumented

The example diffs show widespread API changes that aren't mentioned in the PR description:

defaultValue: → equals:
Parameter() → ParameterExp()
.let() → Pattern.let()
print() removed (now uses implicit Group output)

These are breaking changes for anyone on the current main. They need a migration note in CHANGELOG.md or at minimum a PR description section. Otherwise users upgrading from v0.0.4 will have silent build failures.

🟡 `.vscode/launch.json` references `skitrun` (deleted target)

{
    "name": "Debug skitrun",
    "target": "skitrun",   // ← this target no longer exists
    ...
}

The PR description says skitrun is unified into skit, but the launch config still references it. This will show an error for VS Code Swift extension users.

🟡 Environment variables missing from output cache key

OutputCache.swift only includes SKIT_* and SYNTAXKIT_* env vars in the cache key. DYLD_LIBRARY_PATH, DYLD_FRAMEWORK_PATH, and SWIFT_EXEC can all affect the swift subprocess output but are excluded. Consider including them or at minimum documenting the limitation.

// Current:
let envSnapshot = ProcessInfo.processInfo.environment
    .filter { $0.key.hasPrefix("SKIT_") || $0.key.hasPrefix("SYNTAXKIT_") }

// Suggestion: also include DYLD_* and SWIFT_EXEC

🟡 `swift-crypto` version constraint is loose

Package.swift specifies from: "3.0.0" but Package.resolved pins 3.15.1. Tighten the lower bound to avoid pulling in a version known to be incompatible:

.package(url: "...", from: "3.15.0")

🟢 Minor: `kill()` return value unchecked

In Runner.swift, the SIGKILL fallback after grace period doesn't check the kill() return value. On a process that already exited (ESRCH), this is harmless, but a brief comment explaining the intentional ignore would help future readers.

🟢 Minor: Root `README.md` not updated

After this PR lands, skit is a major deliverable but nowhere in the root README. Add a brief "Usage" section pointing to Docs/skit.md and the release script so it's discoverable from GitHub.

Summary

Area	Status
Architecture & caching	✅ Solid
Toolchain stamp + mismatch detection	✅ Well done
Timeout watchdog	✅ Correct
Documentation (`Docs/skit.md`)	✅ Excellent
Test coverage	❌ None for new code
CI integration	❌ Not added
DSL breaking changes documented	⚠️ Missing
VS Code config (`skitrun` reference)	⚠️ Stale
Cache key completeness	⚠️ Minor gap

The deferred items (#157 auto-rebuild, #158 if-in-Group crash) are reasonable and well-tracked. The two blocking items before merge are test coverage and documenting the DSL API changes.

🤖 Generated with Claude Code

claude · 2026-05-12T20:13:42Z

Code Review — PR #156: `skit` CLI for SyntaxKit codegen

Overview

This PR unifies skit (stdin→JSON parser) and the POC skitrun (DSL renderer) into a single binary with two ArgumentParser subcommands. The architecture is sound: wrap-and-spawn pipeline, two-layer content-addressed cache, toolchain stamp guard, and a Linux-verified DispatchSemaphore workaround for Foundation.Process bugs. The doc coverage is excellent.

Potential Bugs / Correctness

captureSwiftVersion() is called on every cold-cache path — multiple times

outputCacheKey calls captureSwiftVersion(), helpersCacheKey calls it independently, and the toolchain check calls it a third time at startup. Each call spawns a subprocess. For a single file these are serial and add meaningful latency on a cold run. More importantly, if swift --version output changes between invocations (e.g., a TOOLCHAINS= override mid-session) the helpers cache key and the output cache key would disagree. Memoize once per process:

private let _swiftVersion: String? = captureSwiftVersion()

PipeDataBox is @unchecked Sendable with unsynchronized mutation

private final class PipeDataBox: @unchecked Sendable {
  var value = Data()
}

value is written from DispatchQueue.global() and read on the calling thread after group.wait(). The group.wait() provides a happens-before edge so the read is safe in practice, but Swift's concurrency checker can't see through DispatchGroup barriers — hence the @unchecked. This is a known-safe pattern (Dispatch memory model), but fragile to refactor. Consider adding a brief comment explaining the ordering guarantee, or use nonisolated(unsafe) on the property (Swift 5.10+) to be more explicit.

Stale .vscode/launch.json entries reference skitrun

The two new launch configurations target skitrun:

"target": "skitrun",
"preLaunchTask": "swift: Build Debug skitrun"

Package.swift has no skitrun target — that product was removed in this PR. These will fail silently or break the VS Code Swift extension build task. Should reference skit.

wrap() body-offset slicing uses byte index into a String

let start = source.utf8.index(source.utf8.startIndex, offsetBy: firstBodyByte.utf8Offset)
body = String(source[start...])

firstBodyByte.utf8Offset is a byte offset in the UTF-8 view, but source[start...] indexes into the Character view via String.Index. utf8.index(offsetBy:) returns a valid String.Index for well-formed UTF-8 (which Swift source always is), so this is correct today. Still worth a comment since this isn't idiomatic Swift string slicing and a reviewer could easily mis-read it as a bug.

Missing Test Coverage

The new Runner.swift, Helpers.swift, ContentHasher.swift, and OutputCache.swift have no automated tests. The test plan is entirely manual. At minimum, unit tests for the following would be valuable and are easy to write:

wrap() — verify import hoisting, #sourceLocation line numbers, empty-body case
ContentHasher — determinism, avalanche behaviour on small changes
outputCacheKey — different inputs produce different keys
discoverHelpersDir — walk-up finds the right directory
toolchainCheck — match/mismatch/missing-stamp branches

These don't require spawning swift and could live in Tests/SyntaxKitTests/Unit/.

Performance

Helper sources read twice — helpersCacheKey reads each source file for hashing; compileHelpers then hands the paths to swiftc which reads them again. This is minor (helpers are small) but the source data is already in memory after hashing. Passing [URL: Data] through would avoid the second stat+read.

captureSwiftVersion() during output cache key computation — see the bug note above. On a cold miss for a single-file run the call graph is: processFile → outputCacheKey → captureSwiftVersion (subprocess). Combined with the startup toolchain check, a cold run spawns swift --version at least twice. Caching the result is straightforward.

Code Quality

build-skit-release.sh — Package.swift mutation via Python string matching

old = '    .library(\n      name: "SyntaxKit",\n      targets: ["SyntaxKit"]\n    ),'
new = '    .library(\n      name: "SyntaxKit",\n      type: .dynamic,\n      targets: ["SyntaxKit"]\n    ),'
if old not in src:
    sys.exit("Package.swift: expected SyntaxKit library product block not found")

This is fragile — any formatting change to Package.swift silently breaks the build script. A sed-based replacement or a dedicated Package.swift variant (e.g., Package.release.swift) would be more maintainable. The trap cleanup EXIT + backup approach is good, but the detection would miss reformatted declarations.

libStamp uses mtime + size as dylib identity

return "\(size)/\(Int(mtime))"

This can produce false positives (same size/mtime across rebuilds on fast machines) or false negatives (genuinely different builds with the same mtime due to timestamp truncation). A SHA-256 of the dylib contents would be definitive. This is acknowledged in comments but worth calling out since stale cache hits would silently produce wrong output.

helpersExcludePath only excludes a Helpers/ directly under inputDir

let candidate = inputDir.appendingPathComponent("Helpers").standardizedFileURL

If a project has inputs/sub/Helpers/ it will be enumerated as input. The docs state helpers are searched "up-tree" so this is intentional, but the exclusion logic only covers one level. A user who puts helpers inside the input tree at a non-root level will get confusing compile errors.

Minor Nits

runDirectory returns Int32 (exit code) rather than throwing — inconsistent with runSingleFile which throws. A consistent error-handling strategy would make the caller in Run.run() cleaner.
"skit: \(outcomes.count - failed)/\(outcomes.count) succeeded\n" is always printed to stderr even on a fully-successful single-file run… actually this line is only reached from runDirectory, so it's fine. But note the trailing \n is redundant inside Data(…utf8) — the final newline in the string already provides it.
Example errors_async/dsl.swift now has a multi-line comment block explaining the TupleAssignment workaround. Per project conventions, comments should be brief (one line max explaining the WHY). Consider trimming to: // TupleAssignment is internal; emit equivalent single bindings.

Summary

The design is solid and the implementation handles the hard parts well (pipe-drain deadlock, timeout, atomic cache writes, toolchain stamp). The main gaps before merging:

Fix stale skitrun VS Code launch targets
Add unit tests for wrap(), ContentHasher, and cache-key functions
Memoize captureSwiftVersion() to avoid redundant subprocess spawns
Harden build-skit-release.sh dylib-stamp logic or at minimum document the mtime fragility

🤖 Generated with Claude Code

claude · 2026-05-12T22:03:02Z

Code Review: PR #156 — skit: SyntaxKit Codegen CLI for v0.0.5

Summary

This PR unifies the previous skit stub (stdin→JSON parser, ~50 lines) and a prototype skitrun renderer into a single skit binary with two ArgumentParser subcommands:

skit run <input.swift> — wraps a SyntaxKit DSL file in Group { … }, spawns the Swift script-mode interpreter, captures the rendered output. Supports folder mode, output caching, helpers compilation pipeline, toolchain-version stamping, and a timeout watchdog.
skit parse (stdin) — the original JSON syntax-tree emitter.

Net new Swift: ~1,000 lines across Runner.swift, Helpers.swift, OutputCache.swift, ContentHasher.swift, plus the expanded Skit.swift. The build script produces a self-contained portable bundle. Ten of eleven Examples/Completed/*/dsl.swift files are migrated.

Code Quality and Best Practices ✅

Strengths:

Runner.swift is well-structured with clear MARK sections and tight function boundaries. The pipeline separation (resolve → wrap → spawn → drain → cache) reads cleanly.
ContentHasher correctly uses FNV-1a (deterministic across processes, unlike stdlib Hasher) and the tradeoff vs SHA-256 is explicitly justified.
PipeDataBox with @unchecked Sendable is scoped narrowly to a single class with clear ownership semantics — a pragmatic Swift 6 concurrency solution.
Atomic cache writes via tmp.<pid>.<uuid> staging + rename are correct and consistent across both buildHelpers and storeCachedOutput.
compileHelpers drain-before-wait ordering correctly avoids the Linux 64 KB pipe-buffer deadlock.
ArgumentParser integration is clean; validate() is used appropriately for --timeout.

Issues:

captureSwiftVersion() called 3× per invocation. It's called in toolchainCheck, helpersCacheKey, and outputCacheKey — three subprocess spawns before the user's input is touched. Cache the result at startup and thread it through as a parameter.
libStamp uses mtime+size instead of content hash. This produces false-positive cache hits when files are byte-for-byte identical but have different timestamps (e.g. after git checkout). Worth a comment acknowledging the intentional tradeoff vs the helper-source keying (which hashes content).
import SyntaxKit unconditionally prepended in wrap() even when the input already contains it. Swift tolerates duplicate imports but this is untidy. A one-line filter on hoisted to exclude an existing import SyntaxKit before prepending would be cleaner.
runSingleFile writes output non-atomically. result.stdout.write(to: URL(fileURLWithPath: outputPath)) writes directly to the destination — inconsistent with the staged cache writes. A mid-write kill leaves a partially-written file. Use the same staging + rename pattern.
discoverHelpersDir has no depth limit or filesystem boundary. A Helpers/ directory in a parent folder high in the tree (e.g., ~/Helpers/) can be silently picked up. A sentinel file (.skitroot) or a SKIT_HELPERS_BOUNDARY env var would address this.
HelpersOptions and CLIError ownership is unclear. Both are defined in Runner.swift while CompiledHelpers lives in Helpers.swift. A small Types.swift or Shared.swift would clarify the module structure.

Potential Bugs 🐛

Medium severity:

Import-hoisting breaks on leading comments. wrap() stops hoisting at the first non-ImportDeclSyntax node. A top-level comment before the first import (// Generated by skit\nimport SyntaxKit\n...) will cause all subsequent imports to remain inside Group { … }, producing a compile error. The loop needs to skip leading comment trivia.
Folder mode partial-failure is silent to callers. When some files fail in batch mode, the exit code is 1 but there's no indication of which files produced stale outputs. Callers checking only the exit code can't detect stale output files.
exit(result.exitCode) in runSingleFile bypasses ArgumentParser's normal termination. Prefer throw ExitCode(result.exitCode) for consistency and to allow deferred cleanup.

Low severity:

captureSwiftVersion() doesn't drain stderr. If swift --version emits to stderr (unusual but possible in non-standard toolchain setups), the pipe could fill and block the process. Redirect stderr to /dev/null.
Stale skitrun entries in .vscode/launch.json. If skitrun is no longer a Package.swift target after this unification, these VS Code launch configurations will produce extension errors for every developer opening the workspace. They should be removed.

Performance Considerations ⚡

3× swift --version spawns per invocation (see above). Cache the result.
Cache key computation in folder mode calls captureSwiftVersion() and libStamp() per file — N files means 2N+ subprocesses just for cache key computation. Hoist both to a single call in the caller and pass as parameters.
ContentHasher iterates bytes in pure Swift. FNV-1a over large helper sources in pure Swift is slower than CryptoKit.SHA256. Acceptable for v1 since helpers are hashed once per unique content, but worth noting.

Security Considerations 🔒

Arbitrary code execution is the design — skit run compiles and runs user-provided Swift code. The "threat model: you ran your own code" framing is correct for a developer CLI.
FNV-1a is not collision-resistant against adversarial inputs. For the current single-user local-cache scope this is fine, but worth a one-line comment in ContentHasher.swift documenting the threat-model assumption.
SKIT_LIB_DIR env var controls dylib path — an attacker who controls the environment can redirect the dylib load. This is standard for CLI tools that load dylibs via env paths and is acceptable for a developer tool, but worth a sentence in the security documentation.
#sourceLocation path injection. wrap() correctly escapes backslashes and double-quotes in escapedPath. However, a newline in originalPath (unusual but possible) would break the directive. A newline strip or rejection would be defensive.
build-skit-release.sh mutates Package.swift in-place with trap cleanup EXIT. A SIGKILL or power loss mid-build leaves the modified Package.swift on disk. Consider using git stash or a named backup (Package.swift.bak) for more obvious recovery.

Test Coverage ⚠️

This is the weakest area of the PR.

Zero new unit tests for core pipeline. wrap(), discoverHelpersDir(), collectHelperSources(), toolchainCheck(), outputCacheKey(), and ContentHasher have no automated test coverage. These are straightforward to unit-test with the Swift Testing framework already used in this repo.
ContentHasher is trivially testable: a few known-input/known-output cases verify the FNV-1a constants and hex encoding.
wrap() is the most important function to cover: needs tests for no imports, only imports, comment-before-import, import SyntaxKit already present, and mixed cases.
The 10-example smoke test is manual. Consider adding at least one CI step that builds the bundle and runs a minimal DSL file end-to-end.

Actionable Suggestions

P1 — Fix before merge:

Remove stale skitrun entries from .vscode/launch.json if skitrun is no longer a package target.
Cache swift --version at startup and thread the result through helpersCacheKey, outputCacheKey, and toolchainCheck.
Fix import-hoisting for inputs with leading comments in wrap() — test with a file that has a comment before import SyntaxKit.
Use atomic (staged) writes for single-file output — replace result.stdout.write(to:) with the staging + rename pattern used in the cache.

P2 — Should do, not blocking:

Add unit tests for wrap(), ContentHasher, and discoverHelpersDir() using Swift Testing. Even 3–5 test cases per function dramatically improves confidence.
Add a comment in libStamp acknowledging the intentional mtime+size vs content-hash tradeoff.
Add a depth limit or sentinel to discoverHelpersDir() to prevent accidentally picking up unrelated Helpers/ directories.
Prefer throw ExitCode(...) over exit(...) in runSingleFile for ArgumentParser consistency.

P3 — Nice to have:

Add a CI step that runs at least one example through the release bundle end-to-end.
Consolidate CLIError and HelpersOptions into a Types.swift or Shared.swift.
Consider whether the Linux cache root should use the binary name skit rather than syntaxkit for XDG clarity.

Overall: The architecture is well-considered, the documentation is unusually thorough, and the core pipeline is correct. The two main concerns are the absence of unit tests for non-trivial pipeline logic (especially wrap()) and the repeated swift --version subprocess spawns. Neither is a correctness blocker for v0.0.5, but both should be addressed before the next release.

🤖 Generated with Claude Code

claude · 2026-05-12T22:32:50Z

Code Review: PR #156 — `skit`: SyntaxKit codegen CLI for v0.0.5

Overview

This PR lands skit, a unified CLI that replaces two prior proof-of-concept tools (skit stdin parser + skitrun renderer) into one ArgumentParser-based binary. The implementation is a wrap-and-spawn pipeline with two cache layers and a toolchain compatibility guard.

✅ What's working well

Wrap-and-spawn pipeline (Runner.swift:wrap())
The #sourceLocation remap approach is the right call for keeping diagnostics coherent. Import hoisting via SwiftSyntax's ImportDeclSyntax is more robust than regex/string-split approaches. Path escaping for #sourceLocation (backslash + quote only) is appropriate for macOS/Linux.

Atomic helpers cache (Helpers.swift:buildHelpers())
The staging-directory → atomic moveItem pattern mirrors what SwiftPM and Tuist do for the same reason: safe under concurrent skit processes. The "if rename fails and destination already exists, keep theirs" logic is correct.

Toolchain stamp + detect
Exact-string comparison after whitespace trim is correct given that even patch-level Swift versions can produce incompatible swiftmodules. The error message includes the rebuild command, which is excellent UX.

Timeout watchdog
Using DispatchSemaphore.wait(timeout:) to avoid the Linux Foundation.Process.waitUntilExit() hang is the right workaround. The SIGTERM → 5s grace → SIGKILL escalation is standard.

Concurrent folder mode
The task-group pool pattern (maxConcurrent = activeProcessorCount, dynamic task refilling as completions come in) is correct Swift concurrency. Writing all successes even when some inputs fail matches the documented "Tuist-analog batch semantics".

Linux portability
#if os(Linux) guards for install_name, dylib vs .so, and the CryptoKit → swift-crypto substitution are all properly conditioned.

Documentation
Docs/skit.md is excellent — it covers the design, caches, sharp edges, and deferred items with enough depth to be a real decision log. The Tuist research doc (Docs/research/tuist-manifest-pipeline.md) is a thorough reference.

⚠️ Issues worth discussing

Unbounded upward traversal in discoverHelpersDir

while true {
    let candidate = dir.appendingPathComponent("Helpers")
    ...
    let parent = dir.deletingLastPathComponent()
    if parent.path == dir.path { return nil }
    dir = parent
}

This walks to the filesystem root. An input inside /Users/alice/work/proj-a/ could accidentally pick up /Users/alice/Helpers/ or /Users/Helpers/. Consider stopping at a project boundary heuristic (e.g., a Package.swift or .git directory in an ancestor) to prevent silent cross-project contamination.

stale skitrun launch targets in .vscode/launch.json
The PR adds Debug skitrun and Release skitrun launch configurations, but skitrun is the POC binary being replaced by this PR. These entries reference a target that presumably no longer exists in Package.swift. They'll produce confusing errors in VS Code.

maxConcurrent = activeProcessorCount for Swift spawns
Each swift invocation is a heavy JIT process. On a 32-core machine this spawns 32 Swift interpreters simultaneously, which will saturate RAM well before CPU. A conservative cap (e.g., min(activeProcessorCount, 8)) or a user-configurable --jobs flag would be more robust for large batches.

No unit/integration tests for the CLI
The test plan is entirely manual (Scripts/build-skit-release.sh + smoke invocations). The Runner.swift internals — wrap(), toolchainCheck(), cache key computation — are all pure/deterministic functions that are straightforward to unit-test without spawning swift. At minimum, wrap() should have tests covering import hoisting, #sourceLocation line numbers, and files with no imports. This is a gap that will make future refactors risky.

OutputCache.swift / processFile race on output writing
try result.stdout.write(to: URL(fileURLWithPath: outputPath)) in runSingleFile is not atomic. If two skit invocations target the same output file simultaneously, the result is partial/corrupt output. Consider writing to a temp file and renaming, consistent with the helpers cache strategy.

stderr from helpers compilation
In compileHelpers, the process's standardOutput is set to FileHandle.nullDevice, but standardError is set to a Pipe (visible from the partial diff). Confirm that the captured stderr is surfaced to the user on compile failure, not silently dropped. From the code visible, it appears the pipe is read and re-emitted — just flagging for explicit verification.

🔍 Minor observations

Skit.swift validate() checks timeoutSeconds >= 0 — correct, since 0 is documented as "disable the watchdog."
output cache key includes SKIT_*/SYNTAXKIT_* env vars but not PATH. A different swift on PATH could produce different output for the same input. Whether this is a real concern depends on whether users might have multiple toolchains installed — the toolchain stamp mitigates this, but once the stamp check passes, PATH differences could still cause stale cache hits.
The enum_generator move from Examples/Completed/ to Examples/Demos/ is correct and well-motivated.
Package.swift exclude: ["README.md"] for the skit target is the right fix to prevent SPM from complaining about the per-target readme.

🧪 Test coverage

The existing integration test migrations (PlusAssign → Infix, Parameter(isUnnamed:) → Parameter(unlabeled:), ParameterExp(name:value:String) → ParameterExp(name:value:VariableExp)) are all correct. The library test suite passing is necessary but not sufficient coverage for the new CLI surface area.

Verdict

The architecture is sound and the implementation is production-quality in its core pipeline. Three items should be addressed before merge: (1) skitrun VS Code launch targets are stale, (2) discoverHelpersDir should bound upward traversal at a project boundary, and (3) the absence of unit tests for wrap() / toolchainCheck() is a maintenance risk worth tracking (even if not blocking).

claude · 2026-05-12T22:48:25Z

Code Review — PR #156: skit CLI for v0.0.5

Overview

This is a substantial new feature: a unified skit CLI that replaces the previous stdin-parser-only binary with a two-verb tool (run + parse). The architecture is well thought-out, the doc is thorough, and the sharp edges are honestly documented. Notes below focus on actionable concerns.

Strengths

Clean separation of concerns — Runner.swift (pipeline), Skit.swift (ArgumentParser surface), Helpers.swift (dylib compile + cache), OutputCache.swift, ContentHasher.swift. Each file has one job.
FNV-1a with proper rationale — ContentHasher correctly explains why a non-cryptographic, deterministic hash is used (stdlib Hasher is process-seeded and can't be used as a disk key). The comment is useful.
Atomic cache writes — staging dir + rename(2) (via FileManager.moveItem) handles concurrent skit processes correctly. This is exactly the right pattern.
DispatchSemaphore-based wait + concurrent pipe drain — documents and solves the real Linux Foundation.Process.waitUntilExit() hang and 64KB pipe-deadlock problem. This is not obvious and the inline explanation earns its place.
#sourceLocation remapping — surfacing diagnostics pointing at the original input path rather than the temp wrapper is a user-experience win that takes real care to get right.
Toolchain stamp — catching swiftmodule version mismatches with a clear error + rebuild command is far better than letting users hit the cryptic compiler diagnostic. Good foundation for Hybrid bundle: rebuild SyntaxKit from sources on toolchain mismatch #157.
Examples migration is thorough — all deprecated APIs (Parameter(isUnnamed:), Infix(_:_:) builder closure, PlusAssign) updated to new signatures.

Concerns

1. .vscode/launch.json adds stale skitrun targets

{ "name": "Debug skitrun",   "target": "skitrun" },
{ "name": "Release skitrun", "target": "skitrun" }

The PR description says skitrun is replaced by the unified skit. If the skitrun product/target is being removed, these VS Code launch configs reference a target that no longer exists and will fail to build. Either remove them (if skitrun is gone) or update them to skit.

2. No automated tests for skit itself

The test plan is manual smoke tests. Runner.swift, the cache layer, the toolchain-check logic, and helpers discovery are all untested by the Swift Testing suite. Given the complexity (process spawning, caching, timeout watchdog), at least the deterministic parts deserve unit coverage:

ContentHasher hashing stability
outputCacheKey determinism for the same inputs
toolchainCheck returns the right enum arm for match/mismatch/missing
discoverHelpersDir walk-up logic (can be tested with a temp directory)

These don't require spawning swift and would catch regressions in cache invalidation logic.

3. Minor: ContentHasher collision probability comment is slightly off

~10⁻⁹ collision probability at 10⁶ cache entries

FNV-64's birthday-attack collision probability at N entries is approximately N²/2⁶⁵. At N = 10⁶: (10⁶)²/2⁶⁵ ≈ 2.7×10⁻⁸, not 10⁻⁹. Still negligibly small for this use case, but the comment is off by an order of magnitude.

4. isLibDir() only validates dylib presence

private func isLibDir(_ path: String) -> Bool {
  ...
  return fm.fileExists(atPath: "\(path)/\(dylibFilename(forLibrary: "SyntaxKit"))")
}

A directory that contains only the dylib (no .swiftmodule) would pass this check but fail at spawn time with a cryptic module-not-found error. Consider also checking for a SyntaxKit.swiftmodule (or the Modules/ directory) so the path validation catches partially-formed bundles early.

5. SKIT_LIB_DIR env var leaks into output cache key via env-var scan

outputCacheKey scans SKIT_* env vars:

let env = ProcessInfo.processInfo.environment
  .filter { $0.key.hasPrefix("SKIT_") || $0.key.hasPrefix("SYNTAXKIT_") }

SKIT_LIB_DIR matches this filter. Changing the lib path (e.g., between a debug and release bundle) will bust the output cache, which is arguably correct behavior — but it means a user who sets SKIT_LIB_DIR to an absolute path will never get a cache hit on a different machine, even with identical content. Worth documenting or filtering SKIT_LIB_DIR out of the key explicitly.

Minor notes

Run.validate() checks timeoutSeconds >= 0 — correct, 0 is the "disable watchdog" sentinel.
The resolveHelpers → runSingleFile / runDirectory pattern resolves helpers once and passes CompiledHelpers down — good, avoids redundant compilations in folder mode.
helpersModuleName = "SyntaxKitHelpers" hard-coded — acceptable for v1, but worth an issue tracking user-configurable names for multi-project setups.
Docs are unusually good for a v1 feature: the Docs/skit.md design doc + Sources/skit/README.md quick-ref split is a nice pattern.

Summary

The core pipeline design is solid, the Linux compatibility work is careful, and the user-facing experience (toolchain stamp, #sourceLocation, clear cache flags) is thoughtful. Main asks before merging: (1) resolve the stale skitrun VS Code configs, and (2) add even minimal unit tests for the deterministic parts (hash stability, toolchain-check enum, directory discovery). The isLibDir validation gap and the SKIT_LIB_DIR cache-key nuance are worth fixing or at least documenting in skit.md.

Phase 1 documents how Tuist evaluates user manifests via xcrun swift + token-delimited JSON over stdout, with citations into tuist/tuist and swiftlang/swift-package-manager. Phase 2 sketches the SyntaxKit equivalent: pure-DSL input files wrapped into a Group { … } closure before spawning swift, with a 7-step POC ladder that retires cold-start cost first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Ran the hand-driven wrap+spawn flow against a temporarily-dynamic SyntaxKit dylib. Pure-DSL input spliced into a Group {…} wrapper compiles cleanly under xcrun swift with -I/-L/-l + an -Xcc include for SwiftSyntax's C shims. Cold start 0.72s, warm 0.11s. Dylib weight 25MB debug. Hoisted imports work; `if`-in-Group hits a type-checker crash that's a separate SyntaxKit bug to file. Design doc updated with the -Xcc requirement, the rpath flag, the bundled-binary layout's new C-shims include directory, and the POC findings in §7. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

One-command reproduction: flips Package.swift to a dynamic SyntaxKit library, builds, stages dylib + swiftmodules + _SwiftSyntaxCShims headers into /tmp/syntaxkit-poc/, writes a pure-DSL input + hand-rolled wrapper, and runs one cold + three warm timings. Restores Package.swift on exit via trap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Measured SyntaxKit dylib under -c release with strip -x: 25 MB debug → 18 MB release → 9.3 MB stripped. Warm execution timings identical to debug. Distribution sizing is comfortable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

New executable target `skitrun` (POC name) wraps the hand-driven step-1 flow in real code: parse input with SwiftSyntax, hoist top-level imports, emit a Group { … } wrapper fenced in #sourceLocation directives, spawn `swift` via Foundation.Process, pipe stdout through, forward stderr with literal-path fix-up. #sourceLocation does the heavy lifting for diagnostic fidelity — a NonexistentType in InputError.swift:4 now reports as InputError.swift:4:37 from the spawned swift, with no manual stderr arithmetic. Verified default-stdout, -o file output, hoisted Foundation import, and error path rewriting all work end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

skitrun InputDir/ -o OutDir/ walks **/*.swift (skipping `_`-prefixed files), runs the per-file wrap+spawn over withTaskGroup capped at activeProcessorCount, and writes successes into mirrored paths under OutDir/. Failures don't abort the batch: successes are still written and the CLI exits non-zero with a `skitrun: N/M succeeded` summary. Verified happy path (3 files, 1.41s wall vs 0.72s cold baseline), skip rule (deliberately-invalid `_HelperShouldBeSkipped.swift` is not visited), partial failure (3/4 succeeded with exit 1), and single-file regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

skitrun now resolves its lib/ directory automatically: --lib flag, then $SKITRUN_LIB_DIR, then <binary-dir>/lib/, then <binary-dir>/../lib/skitrun/ (Homebrew layout). No more /tmp/syntaxkit-poc/lib fallback. Clear diagnostic when no lib is found, naming all four search paths. poc-step4-release.sh builds a portable bundle under .build/skitrun-release/: release-config + stripped dylib (9.3 MB), modules, C-shims headers, and the binary itself (17 MB — SwiftSyntax statically linked, follow-up to deduplicate). Tested copying the bundle to unrelated directories, both same-dir and Homebrew layouts work zero-config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

skitrun now walks up from the input looking for a Helpers/ directory. On hit, helpers compile via swiftc into libSyntaxKitHelpers.dylib under ~/Library/Caches/com.brightdigit.SyntaxKit/helpers/<sha256>/, keyed on helper-source hashes + swift --version + libSyntaxKit.dylib stamp + cache schema version. Compile lands in a tmp.<pid>.<uuid>/ staging dir then atomic-renames into the cache path so concurrent invocations are safe. Inputs then `import SyntaxKitHelpers` and call into the compiled module; runSwift splices -I/-L/-lSyntaxKitHelpers -Xlinker -rpath onto the spawn. Two new flags: --helpers <dir> overrides discovery, --no-helpers skips it entirely. Folder mode's enumerator now also yields directories so it can skipDescendants() on a Helpers/ directly under the input root — without it the helpers would be reprocessed as inputs. Verified end-to-end via Docs/research/poc-step5.sh: cold compile 2.96s, warm cache hit 0.54s (matches step-1 warm baseline), folder mode 2/2 with Helpers/ excluded, --no-helpers errors with `no such module 'SyntaxKitHelpers'` as expected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

skitrun now hashes (input bytes + helpers cache key + swift --version + libSyntaxKit stamp + sorted SKITRUN_*/SYNTAXKIT_* env vars) into a SHA-256 key under ~/Library/Caches/com.brightdigit.SyntaxKit/outputs/. On hit, processFile short-circuits — no temp wrapper, no swift spawn, just return the cached bytes. On miss the normal compile path runs and stores the rendered output. Only exit-0 runs are cached so failures always re-spawn with fresh diagnostics. Atomic write through a tmp.<pid>.<uuid>/ staging dir + rename mirrors the helpers cache; concurrent writers race safely, the loser drops their copy when the destination already exists. --no-cache disables the lookup wholesale (debugging, after manual cache deletion). Threads through runSingleFile + runDirectory so folder mode can opt out batch-wide. Helpers.swift's syntaxKitCacheRoot/captureSwiftVersion/libStamp are now internal so OutputCache.swift can reuse them without duplication. Verified via Docs/research/poc-step6.sh: cold 0.55s → warm 0.14s (4× faster, no swift spawn), --no-cache 0.27s, post-mutation miss 0.41s then 0.14s on the new key, two cache entries persist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Adds a §7 bullet recording that a long-lived HTTP server variant is on the table after the 7-step CLI ladder finishes — warm `swift` reuse across requests, shared helpers/output caches across tenants, but new isolation/request-shape questions. Captured here so it doesn't get lost between now and step 7. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Verifies the 7-step skitrun POC ladder works on Linux. Tests via Docker (swift:6.0-jammy/aarch64): cold 0.73s, warm cache hit 0.26s, --no-cache 0.30s. Rendered output matches macOS exactly. The real find of this step is a Foundation.Process bug: on Linux, `waitUntilExit()` blocks indefinitely on already-exited children even when EOF on the pipe proves the child has terminated. Reproduced deterministically in `captureSwiftVersion`, where the parent reads all 76 bytes of `swift --version` output and the subsequent wait never returns. Workaround applied to all three call sites (captureSwiftVersion, compileHelpers, runSwift): let semaphore = DispatchSemaphore(value: 0) process.terminationHandler = { _ in semaphore.signal() } try process.run() // ... drain pipes ... semaphore.wait() runSwift additionally drains both stdout + stderr pipes concurrently via DispatchGroup + a PipeDataBox class (boxing for Swift 6 strict concurrency without `@unchecked Sendable` on local vars). Sequential reads would deadlock when either pipe (~64 KB) fills before the child exits. Other Linux-portability changes: * Switch `import CryptoKit` to `import Crypto` in Helpers.swift + OutputCache.swift; add swift-crypto 3.0.0 as a skitrun dep so the same SHA256 API works on both platforms. * dylibFilename(forLibrary:) returns libX.dylib on Apple / libX.so on Linux. Threaded through isLibDir, libStamp, the helpers cache hit check, and `swiftc -emit-library -o ...`. * `-Xlinker -install_name @rpath/...` is Mach-O specific. Wrapped in #if !os(Linux). `-Xlinker -rpath` works on both and is what actually locates the dylib at runtime. Demo script `Docs/research/poc-step7.sh` self-reruns inside the swift container when invoked from the host; uses .build-linux/ as a separate build path so the host's .build/ stays untouched (gitignored). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Lives at Sources/skitrun/README.md (per user's "may be temp" note — keep it local to the target, not the repo front door). Covers usage, the portable bundle, input shape, helpers, caches, the flag table, platform notes (including the Linux waitUntilExit gotcha), and the open scope decisions from codegen-cli-design.md §7. Cross-links into Docs/research/ so the design + per-step results are one click away. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- poc-step4-release.sh: explicitly build the SyntaxKit product. skitrun doesn't depend on SyntaxKit (it spawns swift on user input that does), so --product skitrun alone never produced libSyntaxKit.dylib and the bundle ended up with whatever swiftmodule happened to be cached — which surfaced as a 6.3 vs 6.3.2 module-version mismatch after a toolchain bump. - Examples/Completed/blackjack/dsl.swift: update to the current DSL API (ComputedProperty now requires type:, Init's builder takes ParameterExp, VariableDecl → Variable), escape \(…) interpolations so the literal appears in the generated code, and drop the let/print wrapper so the file is a top-level CodeBlock expression compatible with skitrun. - .vscode/launch.json: add Debug/Release launch configs for skitrun. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Brings 6 more examples up to a state where skitrun renders them end-to-end (joining blackjack from the previous commit). 7/11 examples now work; the remaining 4 (concurrency, errors_async, macro_tutorial, enum_generator) need deeper rewrites and are left for follow-up. Common patterns applied: - Drop `let <name> = …; print(<name>.generateCode())` wrappers — skitrun wraps the input in `Group { … }` itself and rejects top-level let/print. - `ComputedProperty(_:)` → `ComputedProperty(_:type:)` (type is now required). - `Parameter(name:value:)` inside `Init { … }` → `ParameterExp(name:value:)`. - `Call("fn", "literal")` → `Call("fn") { ParameterExp(unlabeled: Literal.string("literal")) }`. - `Infix("a", "op", v)` → `Infix("op", lhs: VariableExp("a"), rhs: <expr>)`. - `Function("f", parameters: […])` → trailing parameter-builder closure form. - `.access("public")` → `.access(.public)` (now takes AccessModifier enum). - `Literal("\"…\"")` → `Literal.string(…)`. - `VariableDecl(.let, …)` → `Variable(.let, …)` (rename). - `Variable(…, defaultValue:)` → `Variable(…, equals:)`. - `.let("x")` pattern shorthand → `Pattern.let("x")`. - `.reference("weak")` → `.reference(.weak)` (now takes CaptureReferenceType). - `.property(name:)` → `.property(_:)` (label removed). - Escape `\(…)` interpolations so the literal appears in the *generated* code rather than being evaluated against non-existent DSL-scope names. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Per codegen-cli-design.md §7. The spawned `swift` process is the only unknown-runtime piece in skitrun (helpers compile and version capture are bounded and cached); a hung user script previously had no upper bound, so the CLI could sit forever. - New flag: `--timeout <seconds>` on `runSwift`. Default 60. Pass 0 to disable. - On expiry: `process.terminate()` (SIGTERM), 5s grace, then `kill(pid, SIGKILL)`. Exit code 124 (matches POSIX `timeout(1)`), with a `skitrun: timed out after Xs` prefix on stderr. - Folder mode propagates the per-input timeout to each parallel worker. - Drive-by: bad CLI args now exit cleanly with the usage text instead of the Swift runtime's "Fatal error: ..." trace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Swift swiftmodules aren't reliably compatible across compiler versions (observed today: a 6.3-built SyntaxKit.swiftmodule was rejected by a 6.3.2 swift). When the bundled module won't load, the spawned `swift` emits a cryptic "module compiled with Swift X cannot be imported by Y" diagnostic that doesn't point at the actual fix. - poc-step4-release.sh now writes lib/swift-version.txt at bundle time, recording the build toolchain's `swift --version`. - skitrun reads the stamp on startup and compares to a local `swift --version` capture (existing captureSwiftVersion helper). Strict exact-string match — patch-level drift broke today. - On mismatch: exit 2 with a stderr message that names both versions and points at the rebuild script. New --no-toolchain-check flag bypasses the check (debugging / forward-compat experiments). - On missing stamp (older bundles): one-line warn and continue, so existing prebuilt bundles keep working through the transition. Auto-rebuild on mismatch is the natural follow-up; tracked as #157. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Brings concurrency, errors_async, and macro_tutorial up to a state where skitrun renders them end-to-end (joining the 7 already updated). 10/11 examples now work; enum_generator remains broken — it's a fully programmatic Swift program (struct definitions, JSON loading, demo runner) rather than a single DSL expression, so it needs structural restructuring into the Helpers/+dsl pattern. Left for follow-up. Notable per-example fixes: - concurrency: rewritten Guard/Throw/Function shapes; Dictionary values are external `Init` expressions of an `Item` type that Literal.dictionary's typed cases can't represent, so the inventory is emitted via the raw VariableExp escape hatch. - errors_async: dropped the TupleAssignment line — that type became internal in the current API — and substituted two single Variable bindings (same observable behaviour for the catch block). - macro_tutorial: collapsed 11 per-example `let` bindings + a final `print` into a single top-level Group (skitrun's input shape). Some sub-examples used `Init { Parameter(...) }` to *declare* initializers; the current public DSL only has Init-as-expression, so those are emitted as raw Swift via VariableExp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Examples/Completed/ holds single-file rendered-DSL examples that skitrun can take as input. enum_generator never fit that shape — it's a full demo project (Package.swift, main.swift, before/after dirs, INTEGRATION_GUIDE.md, JSON config). Move it under a new Examples/Demos/ bucket so the Completed/ contract stays clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@argument

`skit` is now a single CLI with two verbs: skit run <input> # was `skitrun` — render DSL → Swift skit parse # was `skit` — stdin Swift → JSON Default subcommand is `run`, so `skit Input.swift` works without a verb. CLI parsing moves to swift-argument-parser, replacing the hand-rolled CLIArgs parser. The declarative @Option/@Flag/@argument surface produces better-formatted help for free and validates inputs at parse time. - Sources/skitrun/ folded into Sources/skit/ (Main.swift → Runner.swift, Helpers + OutputCache unchanged in substance). - skitrun product/target removed from Package.swift; skit gains SwiftSyntax / SwiftParser / Crypto / ArgumentParser deps. - Env vars: SKITRUN_LIB_DIR → SKIT_LIB_DIR; cache-key env prefix SKITRUN_* → SKIT_*. Bundle dir: .build/skitrun-release/ → .build/skit-release/. Homebrew fallback path: lib/skitrun/ → lib/skit/. - Release script moves to Scripts/build-skit-release.sh — promoted from Docs/research/poc-step4-release.sh to reflect that it's now a shipping build script, not a POC step. - Wrapper internal name: __skitrun_root → __skit_root (only visible in spawned-swift error messages on wrapper-line failures). All 10 working Examples/Completed/*/dsl.swift still render through `skit run` (and the default-subcommand form). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Drop the "research POC for issue #154" framing. Replace `skitrun` with `skit run`, env var prefixes with SKIT_*, and the release-script path with Scripts/build-skit-release.sh. The "Open scope decisions" section is gone — items either shipped (timeout) or have follow-up issues (#157 auto-rebuild, #158 if-in-Group), and the rest are explicitly out-of-scope. Point at Docs/skit.md for the deeper dive (forthcoming). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The chronological poc-step{1..7}-results.md log and the codegen-cli-design.md sketch were the trail by which `skit` reached its current shape — useful at review time, noise for ongoing maintenance. Their substance is now folded into a single forward-looking explainer at Docs/skit.md (the architecture, caches, toolchain stamp, timeout, sharp edges, deferred items, references to #157/#158). Docs/research/tuist-manifest-pipeline.md stays — it's reference material for the manifest-pipeline pattern, not a step log. Git history retains the deleted files for anyone wanting to trace how a particular decision came about. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The two cache keys (helpers cache, output cache) only need a stable, deterministic, cross-platform hash for content addressing — they aren't security-critical and there's no adversary trying to forge collisions. Drop swift-crypto's SHA-256 in favour of a ~10-line pure-Swift FNV-1a 64-bit hasher. - 64-bit output gives ~10⁻⁹ collision probability at 10⁶ cache entries (well past anything we'll see). - Cache keys go from 64-char hex to 16-char hex. - Drops the swift-crypto dep entirely. On macOS the binary barely changes (~4KB) since swift-crypto used CryptoKit there anyway; on Linux we no longer statically link boringssl, which was the dep's real cost. - New file: Sources/skit/ContentHasher.swift. Same streaming API (update/finalize) so the two call sites are nearly identical to the SHA-256 version. All 10 working examples re-rendered with fresh keys after cache clear; output identical. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

claude · 2026-05-13T12:57:57Z

Code Review — PR #156: `skit` CLI for v0.0.5

Overview

This PR unifies two previous CLI tools (skit stdin→JSON parser and skitrun DSL renderer) into a single skit binary with run and parse subcommands. The design borrows the manifest-pipeline pattern from Tuist (well-documented in Docs/research/). It's a substantial, well-thought-out feature with solid attention to cross-platform correctness.

Code Quality & Style

Strengths:

Runner.swift, Helpers.swift, and OutputCache.swift are cleanly decomposed with clear responsibilities. The MARK sections help readers navigate the 611-line Runner.swift.
The pipe-drain pattern (DispatchGroup + concurrent reads before semaphore.wait()) is the correct fix for the Linux 64KB pipe-buffer deadlock — this is a real and well-documented Foundation bug and the workaround is sound.
ContentHasher (FNV-1a) is correctly justified: deterministic across processes (unlike stdlib Hasher), no adversary, 64 bits is enough for the collision budget at realistic cache sizes. The inline math is a nice touch.
Atomic staging-then-rename for both helpers and output caches handles concurrent skit invocations correctly, including the "loser drops theirs" path.
PipeDataBox as @unchecked Sendable is the pragmatic choice here — the boxes are written-once before the group.wait() barrier so the unsafety is contained and well-commented.

Issues / Suggestions:

libStamp uses mtime, not content hash (Helpers.swift:2201–2204). An mtime-based stamp breaks in two common scenarios: (a) touch libSyntaxKit.dylib (legitimate re-touch without rebuild would needlessly invalidate), and (b) cross-timezone copy or filesystem with coarse mtime resolution. Since the dylib is already on disk, a small SHA-256 or even a CRC over the first N bytes would be more reliable. At minimum, document the known limitation.
captureSwiftVersion() is called multiple times — once in helpersCacheKey, once in outputCacheKey, and once in toolchainCheck. Each call spawns a swift --version process. Consider caching the result in a module-level once pattern or passing it through. The redundant spawns add ~50–100ms per cold invocation.
wrap() trims and re-joins imports (Runner.swift:2906), but importDecl.description includes trivia (leading newlines, inline comments). Trimming and re-joining with \n throws away multi-line import comments and could re-order trivia. Consider using importDecl.trimmedDescription instead, or at minimum test what happens with a leading // MARK: before the imports.
PipeDataBox is mutable shared state (Runner.swift:3055). The @unchecked Sendable is fine given the usage pattern, but the class has an unguarded mutable var value. A let with an initializer that takes Data (or marking it nonisolated(unsafe)) would be marginally safer. Low priority.
build-skit-release.sh uses a fragile hardcoded Python string match (Scripts/build-skit-release.sh:1815–1820). The script fails silently if Package.swift formatting drifts. Consider a sed or swift package approach, or at least add a diff check of the restored file in the cleanup trap to catch partial failures.
.vscode/launch.json adds skitrun targets that no longer exist as a product. These are stale after the unification — should be removed to avoid confusing future contributors.

Potential Bugs

Timeout + pipe drain ordering (Runner.swift:3023–3040): After SIGKILL, the code calls group.wait() to drain the pipes. If the child is killed before it writes anything, pipes drain immediately — correct. But if the child has filled the stdout pipe buffer before being killed, readDataToEndOfFile() on stdout will still block until EOF. On Linux, a killed process closes its pipe ends promptly, so EOF arrives quickly. On macOS this is also fine. Looks correct, but worth a comment explaining why this is safe post-kill.
wrap() body could be empty when input has only imports (Runner.swift:2913–2920). If source is nothing but import SyntaxKit, body is "" and the wrapped output is:
```
import SyntaxKit
let __skit_root = Group {
#sourceLocation(file: "...", line: 1)

#sourceLocation()
}
print(__skit_root.generateCode())
```
This compiles fine and emits an empty string — correct behavior, but the user gets no warning. A stderr notice would be friendlier.
runDirectory accumulates all FileOutcome results in memory (Runner.swift:2687). For very large input directories this could be significant (each outcome holds Data of the rendered Swift). Consider streaming writes during the task group instead of collecting first.
storeCachedOutput staging directory uses the same parent as cacheRoot (OutputCache.swift:2314). If createDirectory for the parent succeeds but moveItem fails (e.g. cross-device), the staging directory leaks inside <cache>/outputs/. The try? fm.removeItem(at: staging) in the catch handles most cases, but a cross-device moveItem may not fail atomically. Low risk in practice since caches are typically on the same volume.

Performance

Helpers compile without -O (Helpers.swift:2107–2131). The swiftc invocation has no optimization flag, defaulting to -Onone. For complex helper modules with many generic SyntaxKit expressions, this can make the compile take 2–4× longer. Consider adding -O since helpers are cached and compile-once cost is amortized.
Folder mode spawns one swift per input up to core count — well-matched to the CPU-bound nature of swift script interpretation. Good choice.
Output cache key calls captureSwiftVersion() even on cache hit — the key is computed before the cache lookup check, so a cache hit still spawns swift --version. Reordering to check the cache first with a cheaper key, then refine with the version on miss, would save the subprocess on warm hits.

Security

No shell injection surface: Process arguments are passed as arrays, not shell strings. Safe.
Temp file in FileManager.default.temporaryDirectory (Runner.swift:2851): UUID-named, cleaned up via defer. Fine.
#sourceLocation path injection: escapedPath escapes backslashes and quotes (Runner.swift:2928–2931). The escaping looks correct for the generated Swift string literal context.
Cache directory is user-owned (~/Library/Caches/… on macOS): No privilege escalation concerns.
The PR docs correctly note that the spawned swift is unsandboxed — this is the right call for a developer tool, and the threat model ("you ran your own code") is the correct framing. The deferred sandboxing item (Hybrid bundle: rebuild SyntaxKit from sources on toolchain mismatch #157 mentions auto-rebuild; sandboxing is noted as out-of-scope) should be kept in mind if skit ever becomes a server-side component.

Test Coverage

No unit tests added for Runner.swift, Helpers.swift, OutputCache.swift, or ContentHasher.swift. The PR's test plan is entirely manual (Scripts/build-skit-release.sh + shell sweep). Some targeted unit tests would improve confidence:
- wrap() is pure and easily unit-testable — covering the import-hoist logic, #sourceLocation insertion, and the empty-body case would be high-value.
- ContentHasher.finalize() is deterministic and could have a golden-output test.
- discoverHelpersDir() could be tested with a temp directory tree.
The 10/11 example coverage is good as integration evidence, but it's not automated in CI. Consider wiring the example sweep into the CI workflow (even as a non-blocking job initially).

Examples

The example rewrites are clean and consistent. The key patterns:

Parameter → ParameterExp for call-site usage ✓
Removing top-level let bindings and print(…) in favor of DSL-only expressions ✓
Call("print", "…") → Call("print") { ParameterExp(unlabeled: Literal.string("…")) } ✓
Access("public") → .access(.public) ✓

One oddity in Examples/Completed/errors_async/dsl.swift: the comment mentions TupleAssignment is internal, but the workaround uses two separate Variable bindings which changes the semantics (the original was a simultaneous destructuring). Worth tracking as a DSL capability gap even if the current workaround is functionally equivalent for this example.

Summary

This is solid foundational work. The design decisions (wrap-and-spawn, two-layer cache, toolchain stamp) are well-reasoned and the Linux workarounds are correct. The main action items before merge:

Should fix:

Remove stale skitrun VS Code launch targets
Cache captureSwiftVersion() across call sites (performance + correctness)

Worth addressing:

libStamp mtime fragility — document or replace with content hash
Add wrap() unit tests (it's pure, testable, and is the core transformation)
Add -O to the helpers swiftc invocation

Deferred / acceptable as-is:

Empty-body warning (friendliness, not correctness)
Streaming writes in folder mode (memory, only relevant at large scale)

🤖 Generated with Claude Code

@main

Run 25759444849 broke skit's build on every non-macOS-host target. Foundation.Process is unavailable on watchOS/tvOS and on Ubuntu's wasm + wasm-embedded toolchains, and POSIX kill/SIGKILL aren't in scope on Windows. The previous skit implementation worked around Linux Foundation.Process quirks with a DispatchSemaphore + DispatchGroup pipe-drain ladder and a manual kill(pid, SIGKILL) timeout watchdog — none of which port. This commit replaces all three Foundation.Process call sites (captureSwiftVersion, compileHelpers, runSwift) with swift-subprocess (v0.4.0). The timeout watchdog becomes a withThrowingTaskGroup race against Task.sleep + cancelAll; swift-subprocess's teardown sequence handles SIGTERM → grace → SIGKILL on POSIX and the WM_CLOSE/CTRL_C_EVENT/TerminateProcess equivalent on Windows. ~70 lines of platform workaround dropped. swift-subprocess requires Swift 6.1, so swift-tools-version goes 6.0 → 6.1 (and CLAUDE.md follows). skit itself is gated by `#if canImport(Subprocess)`; on platforms where the Subprocess module isn't built (iOS, watchOS, tvOS, visionOS, Android, WASI) the new SkitStub.swift provides a caseless-enum @main that exits 1 with a clear message. The target still links — just with no usable subcommands. A regression test in Tests/SyntaxKitTests/Integration/ covers swift-subprocess #256 (stream-read hang on cancellation when grandchildren inherit pipe fds). skit's runSwift hits this scenario because swift forks the frontend + linker as grandchildren. Test passes locally in ~1s, bounded at 15s. The iOS/visionOS simulator failures in the same CI run were a separate cause: the May-13 workflow sync bumped osVersion to 26.5, but the macos-26 runner image still only ships OS 26.4 simulators, so download-platform: true couldn't fetch what the matrix asked for. Revert osVersion 26.5 → 26.4 on the iOS/watchOS/tvOS/visionOS rows; Xcode pins unchanged. Tracked for re-bump in #160. Scripts/build-skit-debug.sh is a debug-mode counterpart to the existing release-bundle script, for fast local end-to-end iteration on the DSL transformation (~10s vs. 5-15 min). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

claude · 2026-05-13T13:54:01Z

Code Review — PR #156: skit CLI for v0.0.5

Overview

This PR unifies the old skit (stdin → JSON parser) and the POC skitrun (DSL renderer) into a single binary with two ArgumentParser subcommands: run (default) and parse. The core design — parse the input with SwiftSyntax, hoist imports, wrap in Group { }, spawn swift, capture stdout — is pragmatic and well-suited to the problem. The documentation in Docs/skit.md is thorough and honest about limitations.

Issues

1. Dead launch configurations in `.vscode/launch.json`

The PR adds two new VS Code launch configurations targeting skitrun, but skitrun is the POC target being replaced by this PR. Package.swift has no skitrun product. These will silently fail to build in VS Code:

{ "name": "Debug skitrun",   "target": "skitrun", ... },
{ "name": "Release skitrun", "target": "skitrun", ... },

These should either be removed or updated to reference skit.

2. No unit tests for `wrap()`

wrap() in Runner.swift is the most critical logic in the tool — it parses the input, hoists imports, injects #sourceLocation, and produces the wrapped source. It's a pure (String, String) -> String function and is directly testable without spawning any subprocess. Currently it has zero tests.

Suggested test cases:

Input with no imports → preamble contains only import SyntaxKit
Input with one import → it gets hoisted
Import followed by a non-import statement → import is hoisted, body starts at the non-import
Import after a non-import → stays in body (correct; compiler will catch it)
Path with a double-quote in it → escapedPath is correctly escaped
Empty input → generates a valid wrapper with an empty body

3. `captureSwiftVersion()` is called up to 3 times per run

In a single skit run invocation, captureSwiftVersion() (which spawns a swift --version subprocess) can be called from:

toolchainCheck() (startup)
helpersCacheKey() (helpers compilation)
outputCacheKey() (output cache lookup)

Each call spawns a new process. The result is constant for the lifetime of the process; caching it in a global Task or actor would eliminate the extra spawns.

// Simple fix — top-level async-once capture:
private let swiftVersionTask: Task<String?, Never> = Task {
    await captureSwiftVersion()
}

4. Build scripts patch `Package.swift` by exact string match

Both build-skit-release.sh and build-skit-debug.sh use Python to find-and-replace a hard-coded whitespace-sensitive block in Package.swift to flip the SyntaxKit library to .dynamic. If the formatting of that block ever changes (reformatter, SwiftFormat, manual edit), the script will exit with "expected SyntaxKit library product block not found" and be confusing to debug.

A more robust approach would be to keep a separate Package-dynamic.swift or accept --configuration dynamic style flag, or at minimum check for the dynamic block first (the debug script already does this, but the release script doesn't).

5. `swift-tools-version` bump may surprise existing users

Bumping swift-tools-version from 6.0 to 6.1 means anyone building this package with Swift 6.0 (Xcode 16.0–16.2) will now get a hard error. CLAUDE.md was updated to reflect the new requirements (Swift 6.1+ / Xcode 16.3+), which is good. Worth calling out explicitly in the PR description or a migration note for downstream consumers.

Observations / Suggestions

libStamp uses mtime + size (not content hash). This is the same heuristic make uses and works for typical development workflows, but is theoretically defeatable (e.g., build systems that preserve mtimes). Not a blocker; documenting the tradeoff in Docs/skit.md would be good.

Orphaned tmp.<pid>.<uuid> directories on crash. The atomic-rename cache pattern is solid, but if the process is killed between createDirectory(staging) and moveItem(staging → cacheRoot), a tmp.* directory is left in the cache. These are benign but accumulate. A cache GC pass or a startup sweep of old tmp dirs would keep things tidy over time.

canImport(Subprocess) gating. Gating all skit run functionality behind #if canImport(Subprocess) with SkitStub.swift as the fallback is clean and correctly handles platforms where swift-subprocess isn't available.

Timeout via Task.sleep + group.cancelAll(). Replacing the old DispatchSemaphore approach with Swift structured concurrency is a clear improvement. The regression test in SkitSubprocessTimeoutTests.swift directly targets the known swift-subprocess #256 hang scenario — good to see a test for it.

for_loops/dsl.swift — where clause dropped. The For with a where clause was rewritten as For + If. This loses the semantic, but it's documented as a current API limitation. Fine for now if there's a tracked issue.

Summary

Area	Finding
Correctness	Dead `skitrun` launch configs; `wrap()` has no tests
Performance	`captureSwiftVersion()` called 3× per run
Robustness	Build-script Package.swift patching is brittle
Security	No new concerns; arbitrary-code-execution is documented and intentional
Test coverage	Only subprocess timeout is tested; `wrap()`, `ContentHasher`, cache logic are untested

The architecture is sound and well-documented. The items above are fixable without structural changes — the wrap() test gap is the most important one to close before shipping.

🤖 Generated with Claude Code

claude · 2026-05-13T14:45:39Z

Code Review: `skit` CLI for SyntaxKit v0.0.5

Overall this is a well-designed, ambitious PR that ships a real developer tool with thoughtful caching, timeout handling, and cross-platform support. The documentation is excellent. Below are issues ranging from actionable blockers to suggestions.

🔴 Issues Worth Addressing Before Merge

1. dropFirst path slicing can panic (Runner.swift)

let relative = outcome.input.path.dropFirst(inputURL.path.count + 1)

If a symlink or relative/absolute path normalization mismatch causes outcome.input.path to not be prefixed by inputURL.path, the +1 silently produces garbage or a fatal precondition failure on the substring. Replace with:

let prefix = inputURL.path + "/"
guard outcome.input.path.hasPrefix(prefix) else {
    throw CLIError("internal: input path outside expected directory")
}
let relative = outcome.input.path.dropFirst(prefix.count)

2. #sourceLocation path escaping is incomplete (Runner.swift wrap())

The path escaping handles \ and " but not newlines or carriage returns:

.replacingOccurrences(of: "\\", with: "\\\\")
.replacingOccurrences(of: "\"", with: "\\\"")
// missing: \n, \r, \t

A file path containing a newline (rare but legal on macOS/Linux) would produce a syntactically invalid #sourceLocation directive, causing the spawned swift to emit confusing errors. Add .replacingOccurrences(of: "\n", with: "\\n") etc.

3. Build script Package.swift patching is fragile (Scripts/build-skit-release.sh)

The python3 heredoc does an exact-string replacement that will break if:

Indentation in Package.swift changes (tabs vs spaces, extra blank line)
The block is reordered
SwiftFormat reformats the manifest

Consider using a sed pattern anchored on name: \"SyntaxKit\" (more resilient) or checking the existing type: .dynamic before patching and exiting cleanly when already patched (the "already has type: .dynamic" branch exists but the failure path will abort a CI re-run). At minimum, pin a comment in Package.swift above the product block saying "do not reformat — build-skit-release.sh matches this exactly".

🟡 Improvements Worth Considering

4. No tests for skit in the PR

The diff adds ~2600 lines of new code with zero new test files. Given the complexity of the wrap-and-spawn pipeline, the specific areas most likely to regress are:

wrap() — #sourceLocation line offset arithmetic
Folder-mode path stripping (the dropFirst above)
Timeout signal sequencing (SIGTERM → SIGKILL)
Cache key stability (env var sorting, FNV hash)

Even a handful of unit tests for wrap() and the cache key functions would meaningfully reduce review risk. Happy to see these tracked in a follow-up issue if not in this PR.

5. Redundant swift --version subprocess spawns

captureSwiftVersion() is called in at least three places:

toolchainCheck() at startup
outputCacheKey()
helpersCacheKey()

Each spawns a swift subprocess. In the common single-file case this is 3 extra process launches per invocation. Cache the result once per process lifetime (a simple let swiftVersion: String captured on first call via a lazy actor or nonisolated(unsafe) var + once).

6. Stale staging directories accumulate in cache

Both OutputCache and Helpers write via tmp.<pid>.<uuid>/ → moveItem() (correct for atomicity). But if the process crashes between createDirectory and moveItem, the orphaned tmp.*/ directory is never cleaned. On a dev machine that runs skit hundreds of times with occasional SIGKILL (Xcode, ctrl-C), this accumulates. A sweep on cache-miss or a max-age prune on startup would prevent this.

7. Error output inconsistency (Skit.swift)

Some error paths write to stderr before throwing:

FileHandle.standardError.write(Data("\(error)\n".utf8))
throw ExitCode(2)

Others throw ValidationError(...) which ArgumentParser formats. The UX is inconsistent — some errors emit twice (once by the explicit write, once by ArgumentParser). Audit and pick one pattern.

8. Folder-mode exit code semantics

When processing a directory, skit exits 0 if any files succeed. This is intentional (documented as "Tuist-analog batch semantics") but surprising — most Unix tools exit non-zero if any input fails. Consider adding a --strict flag that exits non-zero on any failure. At minimum, document this in --help.

🟢 Positive Observations

FNV-1a over SHA for cache keys — correct call, CryptoKit/swift-crypto overhead is real for short inputs.
Concurrent pipe drain via DispatchGroup — correctly avoids the 64 KB Linux pipe-buffer deadlock.
DispatchSemaphore.wait(timeout:) for process reaping — good workaround for the Foundation.Process.waitUntilExit() hang on Linux.
UUID-named temp dirs — avoids predictable temp path collisions.
Docs are excellent — Docs/skit.md is unusually thorough; the sharp-edges section is genuinely useful.
Linux verified — swift-crypto substitution and the install_name skip are the right calls.

Minor

argumentsCopy in the subprocess-launch closure — either document why the defensive copy is needed (thread safety? mutation?) or remove it.
Error messages are inconsistently prefixed: some say "skit: …", others don't. Standardise on "skit: " for errors emitted by skit itself (mirrors swift: , swiftc: ).
The --no-toolchain-check flag bypasses a safety rail; document in --help that swiftmodules are not reliably ABI-stable across compiler versions so mismatches silently produce wrong output, not just crashes.

Summary: The dropFirst panic and the build-script fragility are the two things I'd fix before merging. The missing tests are a gap but acceptable if tracked as a follow-up. Everything else is polish.

🤖 Generated with Claude Code

claude · 2026-05-13T15:23:00Z

Code Review: PR #156 — `skit` unified CLI

This is a well-structured PR that cleanly unifies the previous skit/skitrun split. The design decisions (FNV-1a for cache keys, atomic staging + rename, concurrent pipe drain, DispatchSemaphore watchdog) are all sound and match the explained rationale. Below is focused feedback on items worth addressing before merge.

Bugs / Correctness

.vscode/launch.json — leftover skitrun entries
The PR adds two new launch configurations targeting skitrun (debug and release), but the stated goal is to unify skitrun into skit and remove it as a separate product. If skitrun no longer exists as a Package.swift target, these entries will fail silently (VS Code ignores unknown targets) or confuse future contributors. Remove them.

SkitStub.swift — @main conflict risk
The stub uses @main as a fallback for !canImport(Subprocess) platforms. Verify that the conditional compilation and product wiring ensure exactly one @main is active per compilation unit — if a build configuration edge case activates both Skit.swift and SkitStub.swift, you'll get a compile error. A unit test or CI job targeting the stub path would catch this.

Test Coverage

This is the most significant gap. The PR ships one integration test (timeout regression) but the following critical paths have no tests at all:

wrap() — the core transformation that hoists imports and inserts #sourceLocation. A wrong split here produces broken output silently. Table-driven unit tests covering: file with no imports, file with only imports, mixed, import lookalikes (// import Foo), multi-line function bodies.
ContentHasher — trivial to unit-test, and confirming determinism across invocations matters for the cache correctness story.
OutputCache hit/miss — at minimum, a test that writes then reads a cached entry and confirms a cache miss on mutated input.
discoverHelpersDir — the walk-up logic is easy to exercise with a temp directory tree.

The timeout test itself is good — the bounded wall-time assertion directly encodes the regression condition from swift-subprocess #256.

Design / Architecture

Swift 6.1+ is now a hard requirement
Package.swift bumps swift-tools-version to 6.1, dropping Swift 6.0 users. CLAUDE.md and CI are updated consistently, which is good housekeeping. Worth calling out explicitly in the PR description / release notes so downstream users know the minimum toolchain jumped.

FNV-1a for cache keys
Non-cryptographic hash for a local developer cache is the right call — fast, no platform dependencies (avoids the CryptoKit/Linux split that would otherwise require swift-crypto), deterministic across processes. The collision probability is acceptable for this use case. This is a good choice, but adding a brief inline comment explaining why FNV-1a over SHA (speed + no CryptoKit dependency) would help future contributors who might instinctively reach for SHA.

--timeout 0 disables the watchdog entirely
A stuck swift process with --timeout 0 will hang skit indefinitely. This is documented in Docs/skit.md, but consider printing a warning to stderr when --timeout 0 is passed so it's visible in interactive use.

Folder mode concurrency ceiling
runDirectory caps concurrency at ProcessInfo.processInfo.processorCount. This is reasonable for CPU-bound work, but spawning a swift subprocess per file is I/O + memory bound, not CPU bound. Consider a lower ceiling (e.g. max(2, processorCount / 2)) or making it configurable via --jobs. At high file counts, processorCount simultaneous swift spawns could exhaust memory or file descriptors.

Minor / Nits

SkitStub.swift error message — exits with code 1 with a message about unsupported platforms. Add the rebuild command (same style as the toolchain-mismatch error in Runner.swift) so users know exactly what to do.
Examples/Demos/enum_generator/ rename — clean move, no issues.
CI fail-fast: false — good addition; a single flaky WASM matrix cell won't abort the whole run.
osVersion: "26.4.1" pin — the inline comment explaining the Xcode 26.5 simulator gap is exactly the right kind of comment to leave.

Summary

The implementation is solid. The wrap-and-spawn pipeline, caching strategy, toolchain stamp, and timeout watchdog are all well-reasoned. The two items worth addressing before merge are:

Remove the stale skitrun VS Code launch entries.
Add unit tests for wrap() (import hoisting + #sourceLocation) — this is the highest-value coverage gap given how central it is to correctness.

Everything else is either already documented as deferred (#157, #158) or a minor improvement.

🤖 Generated with Claude Code

leogdion · 2026-05-13T18:50:18Z

-    For(VariableExp("number"), in: VariableExp("numbers"), where: {
-        try Infix("==") {
-            try Infix("%") {
-                VariableExp("number")
-                Literal.integer(2)
+    For(VariableExp("number"), in: VariableExp("numbers"), then: {
+        If(VariableExp("number % 2 == 0"), then: {
+            Call("print") {
+                ParameterExp(unlabeled: "\"Even number: \\(number)\"")
            }
-            Literal.integer(0)
-        }
-    }, then: {
-        Call("print") {
-            ParameterExp(unlabeled: "\"Even number: \\(number)\"")
-        }
+        })
    })


this is change in functionationality

leogdion · 2026-05-13T18:50:28Z

+    // single-variable bindings instead — same observable behaviour for the
+    // catch block below.
+    Variable(.let, name: "fetchedData") { VariableExp("data") }
+    Variable(.let, name: "fetchedPosts") { VariableExp("posts") }


The original was correct

- for_loops/dsl.swift: restore `For(_, in:, where:, then:)` with where clause filtering; the earlier rewrite to an inner-body `If` was a semantic change (iterate-all-then-branch vs filter-at-iteration). - errors_async/dsl.swift: restore the original `TupleAssignment(...).async().throwing()` line emitting `let (fetchedData, fetchedPosts) = try await (data, posts)`. - Promote `TupleAssignment` (struct, syntax property, init, `.async()`/`.throwing()`/`.asyncSet()`) to `public` so the example compiles via skit, and switch its `import SwiftSyntax` to `public import SwiftSyntax`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…files [skip ci] Drop the file-level `#if canImport(Subprocess)` around the entire Skit type and the separate SkitStub `@main`. The single `@main Skit` now covers all platforms; only the body of `Run.run()` is guarded, so `skit parse` works without a Subprocess backend. Each subcommand lives in its own extension file (Skit+Run.swift, Skit+Parse.swift), and Run's options shed their verbose `discussion:` text in favor of ArgumentParser's default `--help` rendering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…p ci] Documents the `skit run` pipeline so a reader can trace it end-to-end from the source files alone: top-of-Runner.swift lifecycle map, numbered phases in Skit.Run.run(), docstrings on every previously undocumented orchestrator, and inline step labels inside processFile, runDirectory, runSwift, wrap, buildHelpers, compileHelpers, outputCacheKey, and storeCachedOutput. No behaviour changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

leogdion changed the base branch from main to v0.0.5 May 12, 2026 10:06

leogdion mentioned this pull request May 12, 2026

Hybrid bundle: rebuild SyntaxKit from sources on toolchain mismatch #157

Open

leogdion mentioned this pull request May 12, 2026

if/else inside Group { … } builder triggers Swift type-checker crash #158

Open

leogdion changed the title ~~Research + POC: skitrun codegen CLI driven by SyntaxKit (#154)~~ skit: SyntaxKit codegen CLI for v0.0.5 May 12, 2026

leogdion marked this pull request as ready for review May 12, 2026 19:28

leogdion and others added 9 commits May 13, 2026 08:48

leogdion and others added 13 commits May 13, 2026 08:48

leogdion force-pushed the research/swift-manifest-codegen branch from fe0ed83 to e3e243f Compare May 13, 2026 12:48

leogdion mentioned this pull request May 13, 2026

CI: bump device-platform simulator OS back to 26.5 once macos-26 runner image refreshes #160

Open

Fixing CI

6758822

Update OS version to 26.5 in SyntaxKit.yml

fbcc8fa

leogdion commented May 13, 2026

View reviewed changes

leogdion and others added 3 commits May 13, 2026 14:58

Conversation

leogdion commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's in

What's deferred

Sharp edges documented for users

Test plan

Reviewer focus

Uh oh!

coderabbitai Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

claude Bot commented May 12, 2026

Code Review: skitrun POC (PR #156)

Overview

Bug: Pipe deadlock in runSwift (high severity)

Bug: Blocking thread pool in withTaskGroup (medium severity)

Design: _-prefix skip applies only to filenames, not parent directories

Fragile: path separator +1 in relative path computation

Missing: tests for wrap(source:originalPath:)

Minor: ProcessResult type asymmetry

Minor: -framework SyntaxKit / -F flags missing from runSwift

Minor: default --lib is POC-only, correct to flag early

Shell script

Design questions (pre-step-4 bikeshed)

Uh oh!

codecov Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude Bot commented May 12, 2026

Code Review — skitrun POC (#156)

Bugs / Correctness

Platform Portability

Design / Edge Cases

Minor / Style

Design Questions (from PR's "what I'd value review on")

Summary

Uh oh!

claude Bot commented May 12, 2026

Code Review — PR #156: Research + POC: skitrun codegen CLI

Overview

Design Feedback

Security Concerns

Platform Support

Error Handling

Performance

Test Coverage

Minor Issues

Summary

Uh oh!

claude Bot commented May 12, 2026

Code Review — PR #156 (Research + POC: skitrun codegen CLI)

Overview

Design Correctness

Code Quality

Potential Bugs / Issues

Performance

Security

Test Coverage

Folder Layout Conventions

Summary

Uh oh!

claude Bot commented May 12, 2026

Code Review — PR #156: skitrun POC

Overview

Code Quality & Correctness

Performance

Design

What's well-done

Open issues worth filing before step 4 ships

Uh oh!

claude Bot commented May 12, 2026

Code Review: PR #156 — skitrun POC CLI

Overview

Design Correctness

Code Quality & Bugs

Main.swift

leogdion commented May 12, 2026 •

edited

Loading

coderabbitai Bot commented May 12, 2026 •

edited

Loading

Bug: Pipe deadlock in `runSwift` (high severity)

Bug: Blocking thread pool in `withTaskGroup` (medium severity)

Design: `_`-prefix skip applies only to filenames, not parent directories

Fragile: path separator `+1` in relative path computation

Missing: tests for `wrap(source:originalPath:)`

Minor: `ProcessResult` type asymmetry

Minor: `-framework SyntaxKit` / `-F` flags missing from `runSwift`

Minor: default `--lib` is POC-only, correct to flag early

codecov Bot commented May 12, 2026 •

edited

Loading

Code Review — `skitrun` POC (#156)

Code Review — PR #156: Research + POC: `skitrun` codegen CLI

Code Review — PR #156: `skitrun` POC

Code Review: PR #156 — `skitrun` POC CLI

`Main.swift`

`Helpers.swift`

`OutputCache.swift`

Code Review: `skitrun` POC CLI (PR #156)

3. `@unchecked Sendable` on `PipeDataBox` — document the invariant more precisely

4. `String(decoding: stderrData, as: UTF8.self)` silently replaces invalid bytes

5. No automated tests for `skitrun`

7. Minor: `.build-linux/` in `.gitignore` is fine, but document why it's separate

Code Review — PR #156: `skitrun` POC CLI

Code Review: `skit` CLI for v0.0.5

🔴 No test coverage for the new `skit` target

🔴 No CI for `skit` / release bundle

🟡 `.vscode/launch.json` references `skitrun` (deleted target)

🟡 `swift-crypto` version constraint is loose

🟢 Minor: `kill()` return value unchecked

🟢 Minor: Root `README.md` not updated

Code Review — PR #156: `skit` CLI for SyntaxKit codegen