feat: add machine protocol v1#33
Conversation
zywkloo
left a comment
There was a problem hiding this comment.
Overall, this is an exceptionally rigorous update. The bash scripting for JSON construction, NUL-delimited parsing, and worktree discovery is extremely solid. The fallback logic for detached worktrees and the usage of git_info_exclude_path perfectly handle edge cases with linked worktrees.
I've reviewed the diff closely and have a few skeptical observations and suggestions, though most are non-blocking or just things to be aware of:
1. scripts/wtcraft (line 427)
The json_escape function:
This is a very clever implementation! Stripping control characters using tr -d '\000-\007\013\016-\037' while leaving the JSON-valid ones (like \b, \t, \n) to be explicitly escaped later is a great way to handle binary garbage in bash strings. Also, correctly doing the backslash replacement s="${s//\\/\\\\}" before adding your own backslashes for newline/tabs ensures they don't get double-escaped. Brilliant.
2. scripts/wtcraft (line 1082)
Git Diff quoting in cmd_check:
When git diff --name-only outputs files with spaces or non-ASCII characters, Git wraps them in double quotes and escapes them by default (unless core.quotePath is false). Since you are iterating line-by-line using read -r line (without -z), a file like "my file.txt" will be stored in changed_files with literal quotes. When this hits json_escape(), it gets double-escaped in the JSON (e.g., "\"my file.txt\"").
Suggestion: Consider using git diff -z --name-only and read -r -d '' for robust path handling in the future. (This is an existing flaw, not introduced by this PR, but worth noting for the machine protocol).
3. scripts/wtcraft (line 315)
Fixing info/exclude path resolution:
Changing ${repo_root}/.git/info/exclude to git_info_exclude_path is a great catch. Linked worktrees have .git as a plain text file, not a directory, so mkdir -p .git/info would fail in wtcraft init --local inside a linked worktree. Using --git-path correctly resolves to the common git dir.
4. scripts/wtcraft (line 915)
Stderr redirection in verify --json:
Redirecting child stdout to stderr (cd "$wt_path" && bash -lc "$cmd") >&2 when running in --json mode is very smart. It ensures the JSON payload on stdout isn't corrupted by rogue echos from the verification scripts.
5. scripts/wtcraft (line 384)
list_worktrees and detached HEADs:
Using awk with %c and 0 to produce NUL bytes is fantastic. One small note: if a worktree is checked out to a detached commit, git worktree list --porcelain does NOT output a branch line. Your awk script correctly falls through and assigns an empty string "" to the branch, which then smoothly fails the exact branch match in resolve_worktree_path and falls back to basename matching. This works perfectly.
No blockers. The architecture decisions in the PR description (selecting Rust for the future core but keeping Bash updated for parity) makes total sense. Great work.
- Remove hardcoded VERSION; use read_cli_version() from PR #33 - Add --yes flag to migrate for non-interactive usage - Detect non-interactive stdin and skip prompt gracefully - Add dedicated usage_doctor and usage_migrate help text - Add E2E tests for version, doctor, migrate --yes, and help Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summary
wtcraft --versionandcapabilities --json--repotargeting and stablecheck --json/verify --jsonoutputinit --localin linked worktrees and makenewbase-branch resolution tolerant oforigin/HEAD,main,master,develop, and the current branchTesting
bash tests/run_all.shgit diff --check./scripts/wtcraft verify feat/machine-protocol-v1