Switch skit-aist plan to ArgumentParser analyze subcommand on skit

[leogdion]

Summary

Reworks the skit-aist plan (#106) based on review feedback:

• Drop ConfigKeyKit and swift-configuration in favor of ArgumentParser, which skit already uses. Removes the unreferenced Sources/ConfigKeyKit/ module (13 files, never registered in Package.swift, headers copied from another project) and the 13,333-line vendored swift-configuration doc under .claude/.
• No new executable: the analyzer becomes a skit analyze subcommand alongside run and parse (Skit.Analyze: AsyncParsableCommand in Sources/skit/Skit+Analyze.swift). Domain logic lives in the new AiSTKit target; the OpenAPI-generated Claude client lives in ClaudeKit.
• AnalyzerConfiguration becomes a plain Sendable value type assembled from parsed arguments.
• Updates Docs/skit-analyze-plan.md, IMPLEMENTATION_STATUS.md, Scripts/issue-config.json, and Scripts/ISSUE_CREATION_SUMMARY.md to match.

Corresponding GitHub issue updates (already applied)

• Update Package.swift with Dependencies and Targets #108, Implement AnalyzerConfiguration and AnalyzerError #110, Implement skit analyze Subcommand (ArgumentParser) #111, Create Example Usage Documentation #123 edited to match the new design
• Create ConfigKeyKit Target Structure #109 (ConfigKeyKit target) closed as not planned
• Implement AnalyzerConfiguration and AnalyzerError #110 now depends directly on Update Package.swift with Dependencies and Targets #108

Convergence loop (added in second commit)

The iterate/compare/fix-until-match loop is now in core scope as plan Section 9, split as Part 2 of the orchestration: #120 stays the single-shot pipeline (Part 1), and new issue #168 (depends on #120) tracks the loop — build the generated library, render dsl.swift against it, diff vs expected.swift, re-prompt with the errors/diff until converged or --max-iterations is hit.

🤖 Generated with Claude Code

[coderabbitai]

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: ee2acf88-e077-4c7a-8e8a-d4e82b8dd910

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 skit-analyze-argumentparser

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Code Review — PR #167: Switch skit-aist plan to ArgumentParser analyze subcommand

Overview

This is a planning/documentation-only PR — no Swift implementation yet. It reworks the design for skit analyze by:

• Deleting 13 orphaned ConfigKeyKit source files (never registered in Package.swift) and 13,333 lines of vendored swift-configuration docs
• Replacing the planned ConfigKeyKit configuration framework with ArgumentParser (which skit already uses)
• Changing the target shape from a new executable (skit-aist) to a skit analyze subcommand
• Updating Docs/skit-analyze-plan.md, IMPLEMENTATION_STATUS.md, Scripts/issue-config.json, and Scripts/ISSUE_CREATION_SUMMARY.md to match

The direction is correct. The cleanup alone justifies merging.

---

Strengths

• Repo hygiene win: Removing ~14,000 lines of orphaned/vendored content makes the repo significantly cleaner.
• Consistent CLI conventions: Skit.Analyze as an AsyncParsableCommand extension mirrors the existing Skit.Parse/Skit.Run pattern exactly — right file name (Skit+Analyze.swift), same access levels, same dependency on CommandConfiguration.
• Plain value type config: AnalyzerConfiguration: Sendable with no framework dependency is the correct approach. Assembling it from parsed arguments in run() and handing it to SyntaxKitAnalyzer is clean separation of concerns.
• package access modifier on OpenAPI-generated types is correct — all targets share the same Swift package, so package gives AiSTKit access without leaking to consumers.

---

Issues & Suggestions

1. Test-mode flags are missing from the Analyze struct definition

The plan's Section 8 (test mode) documents flags like --test, --test-stop-on-fail, --test-filter, and --test-cases, but none of these appear in the Skit.Analyze ArgumentParser struct shown in Section 2A. That struct only declares inputFolder, syntaxKitPath, outputFolder, apiKey, model, and verbose.

These test-mode flags also conflict with the positional argument requirement — validate() would fail when --test is the only argument because inputFolder/syntaxKitPath/outputFolder are required @Arguments. The plan should either:

• Add the test-mode @Flag/@Option declarations to the struct (with conditional validation in validate()), or
• Move test mode into a separate skit test-analyze subcommand.

2. outputFolder is not validated in validate()

The current validate() implementation checks that inputFolder and syntaxKitPath exist, but skips outputFolder. Since the analyzer writes to this path, it should either assert the directory exists or create it. At minimum the plan should document the intended behavior.

3. Unofficial OpenAPI spec is a build-time risk

The plan uses laszukdawid/anthropic-openapi-spec (an unofficial, community-maintained spec). This introduces a dependency that could become stale, go unmaintained, or diverge from the actual Claude API. Worth noting in the plan:

• Pin to a specific commit/tag in the curl command, not main
• Add a note to re-verify against the official Anthropic API reference before implementation

4. Default model ID may need updating

The plan uses "claude-opus-4-6" as the default model. The current Opus model is claude-opus-4-8. Hardcoding a model ID in a planning doc that will be implemented later risks the default being stale on day one. Consider documenting it as claude-opus-4-8 or noting explicitly that the implementer should confirm the latest available model ID at implementation time.

5. API key must not appear in verbose output

AnalyzerConfiguration holds apiKey: String and verbose: Bool. The plan doesn't say anything about preventing the API key from appearing in debug/verbose logs. This should be called out explicitly in the configuration design notes — e.g., AnalyzerConfiguration should conform to a custom CustomStringConvertible (or CustomDebugStringConvertible) that redacts apiKey.

6. Open scope gap in #120 should be tracked

The PR description calls out: "#120 still specifies a single-shot pipeline; the iterate/compare/fix-until-match loop remains under 'Future Enhancements' and needs to be moved into core scope." This is left unresolved with no follow-up action tracked in IMPLEMENTATION_STATUS.md. Suggest adding a note to Phase 5 in IMPLEMENTATION_STATUS.md marking this gap, or updating issue #120 before this PR merges.

---

Minor

• IMPLEMENTATION_STATUS.md says "Issues Completed: 0/17" but also notes Create ConfigKeyKit Target Structure #109 was closed; consider 0/17 (18 created, 1 closed as not planned) for clarity.
• The Scripts/issue-config.json label additions are a clean fix — no issues there.

---

Summary

The design change is sound and the cleanup is unambiguously good. The two items most worth resolving before implementation begins are the test-mode flag gap (structural inconsistency in the plan that will create real work when implementing) and the unofficial OpenAPI spec pinning (build stability). The others are documentation improvements that can be addressed inline as the relevant issues are implemented.

🤖 Generated with Claude Code

[claude]

Code review posted below (see full review in the comment thread).

[claude]

Overview

This PR is a planning and cleanup change with no new Swift implementation. It reworks the skit-aist design by removing 13 unreferenced ConfigKeyKit source files and a 13,333-line vendored swift-configuration doc (never wired into Package.swift), switching the architecture to a skit analyze subcommand using ArgumentParser (instead of a new skit-aist executable), and updating planning docs.

Strengths

• Right call on ArgumentParser: reusing the existing dependency is a clear win — fewer dependencies, consistent CLI patterns, and ArgumentParser's validate() + ValidationError are the idiomatic approach for pre-flight errors.
• Dead code removal: ConfigKeyKit was never in Package.swift, so deletion is pure cleanup with zero breakage risk.
• Subcommand UX: skit analyze is more conventional and discoverable than a standalone skit-aist binary.
• Idiomatic plan snippets: the Skit.Analyze: AsyncParsableCommand pattern mirrors how Skit.Run and Skit.Parse are written.

Issues

1. validate() missing output folder parent check — The plan's validate() checks inputFolder and syntaxKitPath but skips outputFolder. If the parent doesn't exist, SyntaxKitAnalyzer.run() fails with a low-level filesystem error after API calls have been made. Validate the parent directory exists when implementing (the output folder itself can be created by the tool).

2. Hardcoded model string — The default 'claude-opus-4-6' will end up in source code. Extract it to a named constant (e.g. AnalyzerConfiguration.defaultModel) so it's updated in one place when model IDs change.

3. Issue Implement SyntaxKitAnalyzer Orchestration #120 scope not yet updated — Implement SyntaxKitAnalyzer Orchestration #120's body still specifies a single-shot pipeline while this PR adds Implement Convergence Loop (iterate until rendered output matches expected.swift) #168 for the convergence loop. Edit Implement SyntaxKitAnalyzer Orchestration #120 before merging so the two issues don't contradict each other for future readers.

4. Base branch is claude-prompt, not main — This PR targets PR Claude prompt #106 (currently draft) and should not merge before it does. Consider noting this or adding a blocking label.

Nits

• JSON formatting changes in issue-config.json (arrays reformatted to multiline) add diff noise that makes meaningful spec-text changes harder to spot.
• ConfigKeyKit/Command.swift was missing a trailing newline — pre-existing, not introduced here.

Summary

The architectural pivot is sound and the cleanup is warranted. Action items before implementation: (1) add output-folder parent validation in validate(); (2) extract the model string to a constant; (3) edit issue #120 to reflect the single-shot/convergence split; (4) merge only after #106 is out of draft and merged.

Generated with Claude Code

[claude]

Code review for PR 167 (already merged) - see inline analysis below.

Overview: Planning-only PR, no production Swift code yet. Drops ConfigKeyKit/swift-configuration in favor of ArgumentParser, removes skit-aist executable, adds convergence loop as Part 2 (issue 168).

STRENGTHS: (1) Removing dead ConfigKeyKit module and vendored docs is clean. (2) Plain Sendable AnalyzerConfiguration is simpler than framework-dependent config. (3) Skit.Analyze follows the same pattern as Skit.Run/Skit.Parse. (4) accessModifier package in openapi-generator-config.yaml is correct for cross-module visibility. (5) LibraryVerifying protocol boundary mirrors how Runner works today.

ISSUES TO ADDRESS BEFORE IMPLEMENTATION:

Issue 1 - validate() missing output folder parent check: The proposed validate() checks inputFolder and syntaxKitPath but not the parent of outputFolder. A missing parent will surface as a cryptic LibraryWriter error. Issue 111 should add a FileManager check on URL(fileURLWithPath: outputFolder).deletingLastPathComponent() and throw ValidationError if it does not exist.

Issue 2 - Test-mode flags vs Skit.Analyze definition mismatch: Section 8 shows skit analyze --test, --test-stop-on-fail, --test-filter, --test-cases but none appear in the Skit.Analyze struct in Section 2. If test mode is deferred, the plan should say so explicitly so the implementor of issue 111 knows the scope.

Issue 3 - ClaudeKit.swift wrapper vs ClaudeKit target name collision: Sources/AiSTKit/ClaudeKit.swift is the wrapper around the OpenAPI-generated client, but ClaudeKit is also the Swift target name. Rename the wrapper file and type to ClaudeAPIClient to avoid ambiguity.

Issue 4 - Unofficial OpenAPI spec maintenance risk: laszukdawid/anthropic-openapi-spec is community-maintained and may lag behind the real API. Add a strategy for keeping it current (pin to a commit hash, periodic update task). The hardcoded default model claude-opus-4-6 has the same staleness risk.

SECURITY: API key handling is sound. Issue 111 should note: never log the key, even in verbose mode.

MINOR: Scripts/issue-config.json is missing a trailing newline. Issue 168 spec should clarify whether diff normalization means running SwiftFormat or simpler whitespace collapsing.

SUMMARY: Correct simplification. ArgumentParser subcommand is the right call. Follow-up items for implementation: output-folder-parent validation in issue 111, clarify test-mode flag scope, rename the ClaudeKit.swift wrapper.