skit: SyntaxKit codegen CLI for v0.0.5#156
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: skitrun POC (PR #156)
OverviewSolid 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 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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.0.5 #156 +/- ##
=========================================
Coverage ? 77.18%
=========================================
Files ? 147
Lines ? 4974
Branches ? 0
=========================================
Hits ? 3839
Misses ? 1135
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review —
|
| Core logic | ✅ Correct and well-structured |
| Blocking async | |
| Atomic writes | |
| Linux portability | |
| 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).
Code Review — PR #156: Research + POC:
|
| 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) | |
| Security | ✅ low risk; add call-convention comment |
| Test coverage | |
| Naming / conventions | _-prefix skip rule — reconsider before locking in |
| Timeout on spawned process |
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
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. OverviewThe PR delivers a working Design CorrectnessWrap-and-spawn is the right shape for v1. The key insight — that the host process only needs to assemble flags, not parse ASTs — keeps 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 Naming. The Code Quality
The walk-up-to-root logic is correct, but the loop invariant is subtle. An explicit comment explaining that // dir.deletingLastPathComponent() returns itself at the filesystem root
if parent.path == dir.path { return nil }
The inline Python that string-matches the exact library product block is fragile. Three separate scripts duplicate the same Shell scripts — repeated staging boilerplate
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 Potential Bugs / Issues1. CryptoKit on Linux (blocking for step 7)
This needs to be resolved before step 7. The cleanest option is adding 2. 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 3. Walk-up false positives for Step 5 results note this: an unrelated 4. Import-line diagnostic offset Step 5 results: 5. No timeout on the spawned The design doc (§7) calls for a 60s default with 6. Bundle includes SwiftSyntax twice (45 MB total) Step 4 results identify this: SwiftSyntax is linked statically into PerformanceThe measured numbers are good:
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 SecurityNo sandboxing for v1. The design doc acknowledges this and the Tuist comparison shows
Test CoverageNo automated tests for
The bash POC scripts serve as manual integration tests today and are clearly written, which is helpful. Folder Layout ConventionsThe SummaryThe POC is solid and the design is well-reasoned. The key things to address before merging (or continuing to v1):
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 |
Code Review — PR #156:
|
Code Review: PR #156 —
|
| 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
Code Review:
|
Code Review — PR #156: skitrun POCThis is a well-researched POC. The overall design (pure DSL input → wrap in Correctness Issues1. In folder mode with N inputs, 2. If the user writes 3. let escapedPath = originalPath
.replacingOccurrences(of: "\\", with: "\\\\")
.replacingOccurrences(of: "\"", with: "\\\"")A path containing a newline (unusual but legal on most filesystems) would break the guard !originalPath.contains("\n") else {
throw CLIError(message: "input path contains a newline: \(originalPath)")
}4.
For a POC this is fine. For productization, converting the inner Minor Issues5. let __skitrun_root = Group {A user DSL file that defines 6. The loop terminates when 7. The comment in 8. On Apple platforms, Design Questions (per the PR's ask)On naming ( My lean is On the skip rule ( The On This is the biggest user-visible hole. Any conditional codegen requires a workaround (computed vars in a helper, or Test CoverageThere are no tests for the new code. For a POC that's fine, but API Changes in ExamplesThe examples show a cluster of breaking API changes that aren't explicitly called out in the PR description:
These are the right moves for Summary: The POC ladder is complete and the research deliverables are solid. The concurrency issue (#4) and the |
Code Review — PR #156:
|
leogdion
changed the title
Code Review:
|
| 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 | |
VS Code config (skitrun reference) |
|
| Cache key completeness |
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
Code Review — PR #156:
|
Code Review: PR #156 — skit: SyntaxKit Codegen CLI for v0.0.5SummaryThis PR unifies the previous
Net new Swift: ~1,000 lines across Code Quality and Best Practices ✅Strengths:
Issues:
Potential Bugs 🐛Medium severity:
Low severity:
Performance Considerations ⚡
Security Considerations 🔒
Test Coverage
|
Code Review: PR #156 —
|
Code Review — PR #156: skit CLI for v0.0.5OverviewThis is a substantial new feature: a unified Strengths
Concerns1. { "name": "Debug skitrun", "target": "skitrun" },
{ "name": "Release skitrun", "target": "skitrun" }The PR description says 2. No automated tests for The test plan is manual smoke tests.
These don't require spawning 3. Minor:
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. private func isLibDir(_ path: String) -> Bool {
...
return fm.fileExists(atPath: "\(path)/\(dylibFilename(forLibrary: "SyntaxKit"))")
}A directory that contains only the dylib (no 5.
let env = ProcessInfo.processInfo.environment
.filter { $0.key.hasPrefix("SKIT_") || $0.key.hasPrefix("SYNTAXKIT_") }
Minor notes
SummaryThe core pipeline design is solid, the Linux compatibility work is careful, and the user-facing experience (toolchain stamp, |
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>
Code Review:
|
| # | Item | File |
|---|---|---|
| 1 | group.next()! — add assertion + comment |
Task+Timeout.swift |
| 2 | libPath env-var — add absolute-path validation + warning |
Subprocess.Configuration+Swift.swift |
| 3 | -- before wrappedPath in swift args |
Subprocess.Configuration+Swift.swift |
| 4 | #sourceLocation — escape \n/\r in path |
WrappedSource.swift |
| 5 | Reconcile timeout docs with Task+Timeout.swift |
Docs/skit.md |
| 6 | Linux release script or remove Linux claim | Scripts/, README.md |
Items 1–4 are correctness/security fixes; 5–6 are doc/claim alignment. Everything else is polish or hardening that can land as follow-up.
🤖 Generated with Claude Code
- Relocate writeOutput to the FileManager extension beside collectInputs,
since the directory-create + write are filesystem operations.
- Replace the success/failure switch with a functional fold:
Result.flatMap maps the render outcome into the write outcome, and the
write itself is captured via Result { ... }.mapError(.unexpected).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code ReviewThis is a substantial PR — ~3400 lines, 67 files — that lands a well-thought-out CLI on top of a previously pure code-generation library. The architecture is clean and the design doc is unusually thorough. Notes below focus on real issues rather than nits. OverviewThe wrap-and-spawn pipeline is a sensible approach for script-mode DSL execution. The dependency injection of the Issues Worth Addressing Before Merge1. let escapedPath =
originalPath
.replacingOccurrences(of: "\\", with: "\\\\")
.replacingOccurrences(of: "\"", with: "\\\"")This handles 2. let first = try await group.next()!This is always safe in the current code (two tasks are always added before this call), but force-unwrapping an 3. No unit tests for the core engine The only new test in this PR is the integration regression test for the subprocess timeout ( 4.
Suggestions (Non-blocking)5. Comment density in CLAUDE.md calls for no comments unless the why is non-obvious. Much of the inline commentary in // Load the input source. Anything past this point keys off these bytes.
let inputURL = URL(fileURLWithPath: inputPath).standardizedFileURLThe code already reads clearly without the comment. A pass to remove explanatory prose would tighten these files. 6. The script patches old_block = ' .library(\n name: "SyntaxKit",\n targets: ["SyntaxKit"]\n ),'If SwiftFormat or a routine edit changes the indentation, the script will silently no-op (it prints "already has type: .dynamic") or exit with an error. Consider using 7. Swift 6.0 removed from CI matrix The PR removes Swift 6.0 from the Ubuntu matrix and bumps 8. try? fileManager().removeItem(at: staging)
if !fileManager().fileExists(atPath: final.path) {
throw error
}The What's Working Well
🤖 Generated with Claude Code |
…uire - Remove .swift-version pin (unblocks swiftly toolchain resolution) - Move CLI presentation helpers into Skit.Run+Render.swift to satisfy the 300-line file_length cap; add guarded `import Subprocess` back - Drop assign-only `destination` from DirectoryRender.FileOutcome - Use `try #require(...)` form in SkitSubprocessTimeoutTests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: skit CLI for v0.0.5OverviewThis PR replaces the original single-purpose Architecture & Code QualityStrengths
Issues Found1. No unit tests for
|
…pers - FileOutcome.result: Result<Void, RunError> -> (any Error)? (nil == success) - Merge all SyntaxKit FileManager extensions into FileManager+Execution.swift (isLibDir, libStamp, collectInputs, writeOutput); drop the inline extension from Runner+Directory.swift (also clears its file_types_order warnings) - runSwift backend returns SwiftRunOutcome; move .completed wrapping out of Runner - collectInputs throws a dedicated CollectInputsError (.cliError / .resourceValuesFailure), mapped to RunError.unexpected by renderDirectory Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review — PR #156: skit CLI for v0.0.5This is a substantial, well-documented PR. The design rationale in Architecture / Structural ConcernsExecution infrastructure lives in the wrong target.
Testing GapsThis is the most significant concern for a v0.0.5 release.
Build Script Concerns
The Python string search is whitespace-sensitive. Linux flow is not shipped. Correctness and Edge CasesCache key collision → silent wrong output.
Minor / StyleStale
What's Working Well
SummaryThe core design is solid. The main blockers before merge are:
The build script fragility and the stale 🤖 Generated with Claude Code |
Move hardcoded literals into named type-local constants (matching the existing private static let convention), giving the CLI surface, swiftc flag set, and cache/path/env conventions a single source of truth. - skit: command/option/flag names, swift executable name, SKIT_LIB_DIR key, shared "skit: " stderr prefix, --version flag, release script path, and the swiftc flag set (-I/-L/-lSyntaxKit/-Xcc/-Xlinker/-rpath/etc). - Execution: cache subpaths, outputs/output.swift names, staging prefix, SKIT_/SYNTAXKIT_ env prefixes, XDG_CACHE_HOME key, SyntaxKit product name, swift extension, lib/lib-skit dirs, %016x hash format, temp-dir prefix, wrapped-input filename, and dylib prefix/extensions. Platform-only constants are #if-gated so neither is flagged unused. CLI option names and all emitted strings are byte-for-byte unchanged. Also adds Docs/string-literal-audit.md cataloguing the broader audit. swift build + swift test (398 tests) + ./Scripts/lint.sh all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: skit CLI for v0.0.5Overall this is a well-designed PR. The SDK/CLI split ( High Priority1. The Python fragment does a literal string match to flip the library product to old = ' .library(\n name: "SyntaxKit",\n targets: ["SyntaxKit"]\n ),'Any whitespace or formatting change to 2. Test coverage for the core The only new test I see is 3. Dead
do {
resolved = try RunInput.resolve(input: input, output: output)
} catch .invalidInput(let message) {
throw ValidationError(message)
} catch {
// RunInput.resolve only throws .invalidInput; defensive.
throw ExitCode(1)
}If 4.
Medium Priority5.
6. Exit codes are not documented in the CLI help text
7. Docs/skit.md describes
The actual implementation in 8. Synchronous I/O in async context In Low Priority / Nits9. No Linux equivalent of The script exits immediately on non-Darwin. "Linux uses a parallel flow" is mentioned but the script doesn't exist. A minimal 10. The 4 KiB cap is fine, but if a future Swift toolchain emits more than 4 KiB on 11. is readable but non-standard. Tools that parse diagnostic output (editors, CI formatters) expect 12. Noted in docs, acceptable tradeoff. The cache lives in the user's home directory so a local attacker is already that user. The collision probability at realistic usage is negligible. No change needed, just confirming the tradeoff is intentional. SummaryThe architecture is well-thought-out, the Swift 6 typed-throws integration is correct, and the toolchain stamp + cache design are good engineering. The main asks before merge:
Everything else is polish and can be tracked as follow-up. |
Centralize the run command's failure handling and isolate the Subprocess dependency behind an SDK-level protocol so the command logic is platform-clean. - Add Skit.Run.CommandError: a self-describing typed error (diagnostic + terminalError) so run() maps every failure to its stderr + ExitCode/ ValidationError in one place; the render chain is now throws(CommandError) end-to-end. - Move the render-session setup into a SyntaxKit `Runner` initializer (Runner+Session.swift) that resolves the lib dir, gates the toolchain check, builds the cache, and assembles the Runner, throwing a typed Runner.SetupError. swiftVersion + the spawn closure are injected so SyntaxKit stays Subprocess-free. - Add a SwiftBackend protocol in SyntaxKit (capture-version + spawn-swift) with a SubprocessBackend conformance in skit, held in an optional that's non-nil only when canImport(Subprocess). run() guards it (nil -> unsupportedPlatform) and no longer branches on #if. - Ungate RunError/RunInput/DirectoryRender (Subprocess-free; the ungated Runner already depended on them) and the Skit.Run+Render extension; #if now survives only where Subprocess types are genuinely used. Behavior (stderr text + exit codes) is unchanged. swift build, swift test (398 tests), and ./Scripts/lint.sh all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review — PR #156:
|
| Component | Cases worth testing |
|---|---|
OutputCache |
key() determinism; concurrent store() (verify no duplicate entries); lookup() miss; schema-version invalidation |
WrappedSource |
Import hoisting (leading, mixed, no imports); #sourceLocation line numbers; empty input; imports-only input |
ContentHasher |
Determinism across invocations; different inputs produce different keys |
ToolchainCheckResult |
.match, .mismatch, .stampMissing (no file), .stampMissing (nil swiftVersion) |
Bundle+ResolveLibPath |
Adjacent layout hit; Homebrew layout hit; all candidates fail |
Runner.processFile |
Cache hit short-circuits spawn; non-zero exit → renderFailed; stderr path rewrite |
None of these require a live swift process — they just need lightweight fakes for the run: closure and FileManager.
Minor Nits
-
Skit+Parse.swift:51—print(json)appends\n, which is fine for human output, but programmatic consumers piping JSON will get a trailing newline after what may already be a compact single-line JSON object.FileHandle.standardOutput.write(data)avoids theData → String → printround-trip and is more explicit. -
CI matrix — Swift 6.0 removal (
SyntaxKit.yml:68) — Dropping a Swift version from CI is a supported-versions change. It would be worth a line in the changelog / README so consumers know the floor moved from 6.0 → 6.1. -
ContentHasheraccess level —internalwhile the rest ofExecution/ispackage. Not wrong (it's an implementation detail), but worth confirming it's intentional givenOutputCache(package) calls it. -
Skit.Run.execute()—libCandidatesorder (Skit+Run.swift:131):[self.libPath, envLibPath]— explicit--libflag wins over the env var, which is the correct precedence. A comment noting the order might prevent a future "why does--liboverride the env var?" question.
Summary
The architecture is solid and the PR description does an excellent job explaining the design trade-offs. The two things most worth addressing before merge are the missing unit tests for the Execution layer and the ToolchainCheckResult stderr side-effect in library code. Everything else is polish.
🤖 Generated with Claude Code
| /// `throw` the case that describes *what* failed; `run()` catches these in | ||
| /// one place and converts each to its stderr diagnostic + process exit, so | ||
| /// the steps stay free of presentation/exit logic. | ||
| internal enum CommandError: Error { |
There was a problem hiding this comment.
Does this need to be inside Skit.Run?
| /// stdout/stderr presentation and file IO, and translate `RunError`/batch | ||
| /// failures into the typed `CommandError` that `run()` maps to a process | ||
| /// exit. Success-path output is still written here. | ||
| internal func render(using runner: Runner, input: String, output: String?) |
There was a problem hiding this comment.
does this use anything inside the Run instance?
| /// `result` is `nil` when the rendered output was written to its mirrored | ||
| /// destination, and carries the error when the input could not be rendered | ||
| /// or its output could not be written (typically a `RunError`). | ||
| package struct FileOutcome: Sendable { |
There was a problem hiding this comment.
Do we need DirectoryRender or just use an Array?
| /// change in these inputs produces a fresh key and forces a recompile. | ||
| /// See `ContentHasher` for the choice of FNV-1a over a cryptographic hash. | ||
| package func key(forInput source: String, libPath: String) -> String { | ||
| var hasher = ContentHasher() |
There was a problem hiding this comment.
Is there an existing protocol or could we refactor into a protocol?
| var isDirectory: ObjCBool = false | ||
| guard FileManager.default.fileExists(atPath: input, isDirectory: &isDirectory) else { | ||
| throw RunError.invalidInput("input does not exist: \(input)") | ||
| } |
There was a problem hiding this comment.
create an extension method on FileManager which returns an enumerated or struct for isDirectory and/or doesExist and refactor all fileExists to use it
|
|
||
| // Phase 3: write successes and capture a per-input outcome for each. | ||
| let outcomes = renderResults.map { | ||
| FileManager.default.writeOutput(for: $0, inputBase: inputURL, outputBase: outputURL) |
There was a problem hiding this comment.
replace all uses of FileManager.default with using an instance property or an argument
| /// Why a render session couldn't be brought up. Decoupled from any caller: | ||
| /// the initializer reports *what* failed; the caller (CLI, build plugin, | ||
| /// in-process driver) decides how to present it and which exit code to use. | ||
| package enum SetupError: Error { |
There was a problem hiding this comment.
Does this need to be inside Runner?
CI: - Remove Android API level 36 from the build matrix (33/34/35 pass; 36 fails). - Drop the `Duration` requirement in the timeout watchdog so the SyntaxKit library compiles on its minimum deployment targets. `Task.timeout` now takes `seconds: Int` and uses `Task.sleep(nanoseconds:)`, fixing the iOS 13 / tvOS 13 / watchOS 6 builds that failed on `'Duration' is only available in iOS 16 / tvOS 16 / watchOS 9`. - Move iOS and visionOS simulators to osVersion 26.5 so all four Apple device platforms run on 26.5; drop the stale 26.4 pin comment. Promote the Execution engine surface (Runner, RunInput, SwiftBackend, and related types) from `package` to `public`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: skit CLI for SyntaxKit v0.0.5OverviewThis PR unifies a stdin-to-JSON parser and a POC DSL renderer into a single skit CLI with run (default) and parse subcommands. The design is solid: clean separation between the SDK-shaped Runner and the CLI surface in Skit, typed throws throughout, injected backend for testability, and thoughtful caching/timeout infrastructure. Architecture Concern: CLI infrastructure in the library targetThe Sources/SyntaxKit/Execution/ directory lives in the SyntaxKit library target, not the skit executable. Runner, OutputCache, WrappedSource, ToolchainCheckResult, and friends are all marked public, meaning subprocess management, output caching, and CLI-specific utilities become part of the public API of what is otherwise a lightweight DSL library. Compounding this, OutputCache.swift uses Suggestion: Move Sources/SyntaxKit/Execution/ into the skit target or a new SyntaxKitCLI library target. At minimum, Bug: Side effect in value type initializer
Potential overflow in timeout conversionIn Task+Timeout.swift: try await Task<Never, Never>.sleep(nanoseconds: UInt64(seconds) * 1_000_000_000)validate() in Skit+Run.swift checks timeoutSeconds >= 0 but has no upper bound. Passing --timeout 18446744074 overflows the UInt64 multiplication silently. Adding Test coverage gapsThe single integration test (SkitSubprocessTimeoutTests) is excellent for its target regression, but several new components have no coverage:
Build script fragilityScripts/build-skit-release.sh patches Package.swift in-place via string matching. The Python guard (sys.exit()) fires if the expected block is not found, and the Minor observations
SummaryThe core design is well-executed and the code is clean. Three things worth addressing before landing:
The deferred items (#157 auto-rebuild, #158 if-in-Group crash) are well-documented and reasonable to defer. Generated with Claude Code |
skit run was non-functional: the `swift` interpreter's JIT can't load the dynamic libSyntaxKit symbols. Switch the render backend to compile the wrapped DSL with `swiftc` to a temp executable and run it (Subprocess.Configuration+Swift). Also addresses code-review findings and follow-ups: - Guard --timeout against the UInt64 nanosecond overflow in validate(). - Make ToolchainCheckResult.init pure (no stderr). Record the outcome as a new Runner.ToolchainVerification, carry it on RunError.renderFailed, and have skit surface a one-line hint only on a render failure — keeping the SDK silent. - ContentHasher: fix 32-bit truncation in finalize() (%016x consumed a 32-bit int, halving the 64-bit digest) by formatting via String(_:radix:). - Replace the in-place Package.swift patch in build-skit-release.sh with a SYNTAXKIT_DYNAMIC_LIB env-var switch read by the manifest. - Introduce a pluggable ContentHashing protocol; OutputCache derives keys through an injectable hasher factory defaulting to ContentHasher (FNV-1a). Add unit tests for ContentHasher, WrappedSource, ToolchainCheckResult, OutputCache (incl. injected-hasher), and Runner render-failure toolchain plumbing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: skit — SyntaxKit codegen CLI for v0.0.5OverviewThis PR lands Strengths
IssuesPotential Bug —
|
…ome.result Extract domain-agnostic filesystem operations into generic FileManager helpers returning clean Swift types instead of Foundation idioms: - FileManager+FileSystem.swift: PathKind enum (replaces the fileExists + ObjCBool dance), FileFingerprint struct (size + mtime), FileEnumerationError, and pathKind/fingerprint/regularFiles(under:)/writeData methods. The recursive walk and path sort live here so deterministic ordering is guaranteed in one place. - URL+Reroot.swift: rerooted(from:onto:), the input->output path mirroring pulled out of writeOutput. FileManager+Execution.swift becomes thin domain wrappers: isLibDir/libStamp over pathKind/fingerprint, collectInputs over regularFiles (mapping FileEnumerationError -> CollectInputsError) plus the SyntaxKit .swift/_-prefix input convention, and writeOutput keeping only Result-folding + rerooted + writeData. Adopt pathKind at the other fileExists sites (RunInput.resolve, OutputCache.store). Tighten DirectoryRender.FileOutcome.result from (any Error)? to RunError?: the only producer (the SDK) funnels everything through RenderTaskResult.writeOutput, which is throws(RunError). The skit consumer drops its now-redundant `as? RunError` downcast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: skit CLI for v0.0.5Overall: This is a well-crafted PR. The architecture is clean, the wrap-and-spawn pipeline is well-reasoned, and the execution layer has solid unit test coverage. The findings below are mostly low-severity hardening opportunities; nothing blocks merge. Architecture & Design ✅
Issues FoundMedium
Several variable uses are unquoted, which will silently break if any path segment contains a space: # Examples from the script
cp $BUILD_DIR/skit $OUTPUT_DIR/ # line ~56
cp -r $BUILD_DIR/*.swiftmodule $LIB_DIR/ # line ~61All path variables (
BUILD_DIR=$(ls -d .build/*-apple-macosx*/release | head -1)If a future build system or Xcode version produces multiple matching directories, this silently uses the wrong one. Consider: mapfile -t dirs < <(ls -d .build/*-apple-macosx*/release 2>/dev/null)
[[ ${#dirs[@]} -eq 1 ]] || { echo "Expected exactly one build dir, got: ${dirs[*]}"; exit 1; }
BUILD_DIR="${dirs[0]}"Low
// Note: symlinks are followed; callers with untrusted input should resolve
// the root URL first to confirm it stays within the expected boundary.
After // executableURL is this binary's own path — trusted, no prefix check needed.
let source = String(data: inputData, encoding: .utf8) ?? ""Invalid UTF-8 on stdin silently produces an empty parse result with no diagnostic. A one-line stderr message before the fallback would save confusion: if let source = String(data: inputData, encoding: .utf8) {
// ...
} else {
fputs("skit parse: warning: stdin is not valid UTF-8; treating as empty\n", stderr)
}Informational / Non-blockingTest coverage gaps in batch/collection path The execution unit tests cover
These aren't blockers, but they'd increase confidence in the batch mode that most users will hit first. CI matrix changes worth confirming Two matrix shrinks in
Summary
The wrap-and-spawn pipeline, cache design, toolchain stamp, and timeout watchdog are all solid. The shell script hardening is the most actionable item before merge. |
- Replace OutputCache's concrete ProcessInfo dep with an EnvironmentProvider protocol so FakeEnvironment (struct) replaces the FakeProcessInfo subclass that broke on swift-corelibs-foundation (final class, environment on an extension). Clears the OutputCacheTests failure on Linux/Windows/Android. - Add swift-system as a conditional dep + canImport(System)/SystemPackage shim so the skit target compiles on Linux/Windows. - Hoist Runner.SetupError -> RunnerSetupError, Runner.ToolchainVerification -> ToolchainVerification, Skit.Run.CommandError -> RunCommandError, DirectoryRender.FileOutcome -> FileOutcome. - Replace DirectoryRender with [FileOutcome] + a failureCount extension. - Lift render/renderSingle/renderBatch out of extension Skit.Run into enum Render (none used self). - Move SKIT_LIB_DIR lookup to ProcessInfo.skitLibPath. - Inject FileManager via @sendable () -> FileManager on Runner; thread through Runner+Session and Bundle.resolveLibPath. - Delete Docs/skit-internals.md + Docs/string-literal-audit.md. - Unify Scripts/build-skit-{debug,release}.sh into Scripts/build-skit.sh (release default; --debug flag). - Lint cleanups: split FileManager+FileSystem.swift into PathKind / FileFingerprint / FileEnumerationError; extract RenderTaskResult to its own file; reorder type contents (cases/properties/init/methods); fix conditional-returns-on-newline + multiline-arguments-brackets; add doc comment to ContentHasher.update(data:). Lint goes from 21 violations to 5 (the remaining 5 are all pre-existing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review:
|
| Priority | Item | File |
|---|---|---|
| Bug | Force-unwrap in task group | Task+Timeout.swift:54 |
| Bug | FileManager factory called 3× | Runner+Session.swift |
| Architecture | Execution/ in library target |
Sources/SyntaxKit/Execution/ |
| Correctness | FNV-1a silent collision risk | ContentHasher.swift |
| Quality | Misleading Linux error message | build-skit.sh |
| Quality | Unconditional warning suppression | Subprocess.Configuration+Swift.swift |
| Quality | Cache schema version lifecycle | OutputCache.swift |
| Tests | Argument validation tests missing | Skit+Run.swift |
| Tests | Special-char path escaping untested | WrappedSourceTests.swift |
| Tests | Integration test flake risk | SkitSubprocessTimeoutTests.swift |
| CI | Swift 6.0 drop unexplained | .github/workflows/SyntaxKit.yml |
The force-unwrap and triple fileManager() invocations are the two items I'd fix before merge. The rest are improvements but not blockers.
🤖 Generated with Claude Code
regularFiles relied solely on .skipsHiddenFiles, which keys off the dot-prefix on Unix but the FILE_ATTRIBUTE_HIDDEN attribute on Windows, so dot-prefixed entries leaked through there. Filter dot-prefixed path components explicitly for consistent hidden-file semantics on every platform. The two RunnerRenderFailure render tests drive real temp-file IO that is unreliable on WASI (and rendering can't spawn swift there anyway), so disable them on WASI via a new Platform.isWASI test-trait helper rather than #if. Also clear adjacent lint findings surfaced along the way: drop the group.next()! force-unwrap in Task.timeout for a for-await loop guarded by assertionFailure, and bind operands separately in the determinism tests to avoid identical_operands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…exity Add RunCommandError(renderFileError:) and RunCommandError(renderDirectoryError:input:) initializers that translate the RunError thrown by Runner.renderFile / renderDirectory onto the CLI's exit policy, mirroring the existing init(_:RunnerSetupError). Render.renderSingle/renderBatch now use a single total catch each instead of an inline switch / multiple pattern-matched catches, and the per-outcome batch diagnostics move into a reportOutcomeDiagnostics helper. Clears both Render.swift cyclomatic_complexity violations; behavior (error->exit mapping and stderr output) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: skit CLI for v0.0.5Overall assessment: This is a well-designed, production-quality PR. The architecture is clean, documentation is thorough, and the key engineering decisions (content-addressed caching, atomic writes, timeout watchdog, toolchain stamp) are all well-reasoned and correctly implemented. Comments below are mostly refinements and gap-fills rather than blockers. Architecture & DesignStrengths:
Issues & Suggestions🔴 Medium — Cache eviction not implemented
Suggestion: Add a 🔴 Medium —
|
| Category | Rating |
|---|---|
| Architecture | Excellent |
| Error handling | Excellent |
| Concurrency safety | Excellent |
| Test coverage | Good (gaps noted above) |
| Documentation | Excellent |
| Security | No concerns |
| Build scripts | Good |
The deferred items (#157 auto-rebuild, #158 if-in-Group crash) are correctly scoped out. The medium-priority items above are worth addressing before or shortly after merge, but none are blockers for v0.0.5. Good work.
🤖 Generated with Claude Code
The public Runner render API no longer reads user inputs or writes user outputs. `renderFile(input:)` becomes `render(source:originalPath:)` and `renderDirectory(inputDir:outputDir:)` becomes `render(sources: [RenderInput]) -> [FileOutcome]`, where FileOutcome now carries the rendered `stdout`. The internal temp-wrapper write that swiftc compiles stays in the SDK (purity is at the API boundary, not zero IO). The reusable filesystem primitives stay as public SDK utilities the CLI composes — `FileManager.collectInputs`, `URL.rerooted`, `FileManager.writeData` are now public; the output-mirroring orchestration (`FileManager.writeOutput`, `RenderTaskResult.writeOutput`/`OutputDestination`) is deleted. skit's Render/Render+Batch now own walking, reading, and writing: collect inputs → read into RenderInput → render in-memory → mirror each stdout via rerooting, folding read/write failures into per-file outcomes so one bad input doesn't abort the batch. RunnerRenderFailureTests now drives in-memory source (no temp input file); adds RunnerBatchRenderTests covering empty/all-success/isolated-failure batch semantics with a stub backend. Verified end-to-end: single-file render to stdout, directory render mirrors a nested tree (2/2 succeeded, exit 0), and a bad input is diagnosed + skipped while peers are written (1/2 succeeded, exit 1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
originalPath is only a diagnostic label (#sourceLocation + stderr path-rewriting), so default it to nil and resolve the no-path case to a synthetic "source.swift" label in processFile. Lets a developer render a bare string of SyntaxKit DSL without inventing a path. RunnerRenderFailureTests now exercises the no-originalPath call. Follow-up #164 tracks revisiting the synthetic-label choice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review — PR #156:
|
1 participant
Summary
This branch lands
skit, a SyntaxKit CLI for config-driven Swift codegen. Two verbs:runis the default subcommand, soskit Input.swiftworks without a verb.The PR unifies the previous
skit(stdin → JSON parser, ~50 lines) and the POCskitrun(DSL renderer, ~700 lines) into one binary with ArgumentParser subcommands. Net replacement: one CLI, one product, declarative arg parsing, productized docs.What's in
skitCLI withrun+parsesubcommands (runis the default). Built onswift-argument-parser.run: parse the input with SwiftSyntax, hoistimports, wrap the rest inGroup { … }, spawn script-modeswift, capture stdout, splice the output.#sourceLocationremaps body diagnostics back to the input file.Helpers/directories auto-discovered up-tree from the input, compiled once intolibSyntaxKitHelpers.{dylib,so}and cached by content hash.--no-helpers/--helpers <dir>overrides.(input, helpers, libSyntaxKit stamp, swift version, sorted SKIT_*/SYNTAXKIT_* env)hash. Warm hit ≈ 0.14s, no swift spawn.--no-cacheto skip.lib/swift-version.txtat build time;skit runcompares to localswift --versionon startup and refuses to spawnswifton mismatch with a clear error and the rebuild command.--no-toolchain-checkbypass.swift. SIGTERM → 5s grace → SIGKILL. Exit code 124.--timeout 0disables. Implementation usesDispatchSemaphore.wait(timeout:)to avoid LinuxFoundation.Process.waitUntilExit()hangs.Scripts/build-skit-release.shproduces.build/skit-release/{skit, lib/}. Portable:cp -rit anywhere, runs zero-config.swift:6.0-jammy/aarch64.swift-cryptoreplaces CryptoKit; the Mach-Oinstall_namerewrite is skipped. Concurrent pipe drain viaDispatchGroupto avoid the 64KB pipe-buffer deadlock.Examples/Completed/*/dsl.swiftfiles render end-to-end throughskit. The 11th (enum_generator) was a full demo project shape rather than single-file DSL; moved toExamples/Demos/enum_generator/where it doesn't masquerade as a completed example.Docs/skit.mdis the deep dive (architecture, caches, toolchain stamp, timeout, sharp edges, deferred items).Sources/skit/README.mdis the per-target quick reference.What's deferred
Tracked as follow-up issues, not blockers:
skitbundles 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/elseinsideGroup { … }builder triggers Swift type-checker crash #158 —if-in-GroupSwift type-checker crash. Independent SyntaxKit bug (notskit's); the workaround is documented inDocs/skit.md(hoist the conditional into a helper that uses plain Swiftif/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 inDocs/skit.mdunder "What's deferred".Sharp edges documented for users
@mainand 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@availableetc. in the output should use the DSL's.attribute(…)method..comment { Line(.doc, …) }.Test plan
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