Skip to content

fix: status judges symlinks sync key, reject TOML-breaking chars, keep exec bits for non-ASCII paths - #54

Merged
min0625 merged 1 commit into
mainfrom
fix/review-sync-keys-toml-safety
Jul 11, 2026
Merged

min0625 merged 1 commit into
mainfrom
fix/review-sync-keys-toml-safety

Conversation

@min0625

@min0625 min0625 commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Three fixes from a full-project review (all reproduced end-to-end before fixing, re-verified after):

1. status omitted the symlinks sync key (spec §4.5)

checkSync (apply / lock --check) compares five sync keys; depStatus compared only four. Changing a dep's symlinks policy without re-locking made status report ok (exit 0) while apply refused with "out of sync" (exit 2) — a CI gate built on status would miss the drift.

Fix: checkSync's field list is extracted into syncFields, the single source of truth now shared by status, so the two can never disagree on what "in sync" means.

2. TOML-breaking characters were never rejected

git allows " in tag names, and the surgical graft.toml editor emits values verbatim inside a TOML basic string. So graft add repo@'v1"x' happily wrote:

version = "v1"x"

…after which every graft command fails with a parse error. version had no character validation at all; repo only rejected whitespace; ValidatePath missed ".

Fix: a shared validateTomlSafe rejects ", \, and control characters in repo, version, subdir, and dir with exit 2 — at load and before graft add writes anything. Spec §7 gains a "write-back (TOML) safety" paragraph (both languages).

3. Exec bits silently dropped for non-ASCII paths

ExecBits parsed ls-tree output with git's default core.quotePath, which octal-escapes non-ASCII paths (腳本.sh → "\350\205\263..."), so the exec bit was recorded under a name that never matches the real file and fell out of the hash. Now runs with -c core.quotePath=off.

⚠️ Hash change: trees containing non-ASCII executable files hash differently after this fix (.graft-execbits content + exec byte). Pre-1.0 breaking change; affected deps need a re-lock. Worth a release-notes line.

Ratchet

docs/requirements.md gains REQ-STATUS-SYMLINKS-SYNC, REQ-TOML-SAFE, and REQ-EXECBIT-QUOTEPATH, each with covering tests (status_test.go, manifest_test.go via TOML literal-string smuggling, add_test.go with a real quoted tag, repocache_test.go). Also refreshes CLAUDE.md's release status (beta.6) and command list.

🤖 Generated with Claude Code

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@codecov

codecov Bot commented Jul 11, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.47368% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/gittest/gittest.go 55.55% 2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

…p exec bits for non-ASCII paths

Three fixes from a full-project review:

- status omitted the symlinks policy from its sync-key comparison, so it
  reported ok while apply/lock --check refused with "out of sync" (spec
  §4.5 lists symlinks). checkSync's field list is now extracted into
  syncFields, the single source of truth shared with status, so the two
  can never disagree again.

- Nothing rejected a double quote, backslash, or control character in
  repo/version/subdir/dir values, yet the surgical graft.toml editor
  emits them verbatim inside a TOML basic string — `graft add
  repo@'v1"x'` (a tag name git allows) wrote a manifest that no longer
  parses, wedging every subsequent command. A new validateTomlSafe check
  rejects these with exit 2 at load and before graft add writes anything.

- ExecBits ran ls-tree with git's default core.quotePath, which
  octal-escapes non-ASCII paths, so such a file's exec bit was recorded
  under a name that never matches and silently dropped from the hash.
  Now runs with core.quotePath=off. Note: this changes the content hash
  of trees containing non-ASCII executable files (pre-1.0 breaking
  change; affected deps need a re-lock).

Spec §7 gains a "write-back (TOML) safety" paragraph in both languages;
requirements.md gains REQ-STATUS-SYMLINKS-SYNC, REQ-TOML-SAFE, and
REQ-EXECBIT-QUOTEPATH with covering tests. Also refreshes CLAUDE.md's
release status (beta.6) and command list.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@min0625
min0625 force-pushed the fix/review-sync-keys-toml-safety branch from 81c57d4 to be0bd70 Compare July 11, 2026 07:52
@min0625
min0625 merged commit 0269e05 into main Jul 11, 2026
9 checks passed
@min0625
min0625 deleted the fix/review-sync-keys-toml-safety branch July 11, 2026 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant