Post-public-release review remediation (ASH-210..213)#12
Merged
Conversation
Found in the post-public-release code review. HIGH: ashd panicked on every shutdown when LSP is disabled (the default). cmd/ashd/main.go defers and signal-handler-calls langCache.Close(), but langCache stays nil unless [lsp].enabled — Cache.Close() then dereferenced the nil receiver via c.mu.Lock(). The live ashd.log held 98 such panic traces; the crash skipped the deferred led.Close()/WAL checkpoint. Fix: make Cache.Close() nil-receiver-safe, matching Broker.Close()'s style. Bundled hardening (all small, behavior-preserving): - internal/lsp/lsp.go: close stdin/stdout/stderr pipes when cmd.Start() fails in Broker.start() — they leaked on that path. - cmd/ashd/main.go: create the UDS socket under a 0o177 umask so it is never briefly world-connectable between net.Listen and Chmod. - internal/ledger/ledger.go: chmod ledger.db to 0600 — it holds file paths and result fragments and was created 0644. - Fix two stale code comments: the misplaced `ash hook` doc comment in cmd/ash/main.go, and the jail-policy-swap comment in cmd/ashd/main.go (it is mutex-guarded, not a lock-free swap). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Found in the post-public-release security review; verified exploitable. The git verb appended request-controlled rev/pathspec args (range, ref, rev, author, since, until, pathspec) into the git argv unsanitized. With [git].backend = "shellout", git read a value like ash git --op log --range '--output=/path' as an option, not a revision — `git log --output=FILE` writes attacker-chosen files anywhere the daemon user can write. log/show are read-side verbs (exposed over MCP as ash_git), so a prompt-injected tool call could drop a .git/hooks script or overwrite ~/.bashrc. Fix: ParseArgs — the single choke point for both the shellout and go-git backends — now rejects any of those seven args whose value begins with "-". A legitimate ref, revision range, date, author, or pathspec never starts with "-", so no real call is affected. Adds a regression test covering the --output= injection across all seven args. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Found in the post-public-release performance review. - internal/ledger/tokens.go: Counter.Count called enc.Encode(s,nil,nil), which routes to the special-token-scanning encode path and does a redundant second full []rune(s) conversion. ash responses never carry <|endoftext|>-style tokens, so EncodeOrdinary yields identical counts on the cheaper path. Count runs 3-4x per request on the synchronous response path. - cmd/ashd/main.go: argsBlob re-decoded the whole request via proto.DecodeRequest just to read Args, while the decoded req was already in scope. Take *proto.Request directly; one fewer full msgpack unmarshal per call. - internal/jail/relpath.go: PrettyPath copied and re-sorted the prefix list every call despite PathPrefixes() being longest-first by contract. Iterate directly; drop the now-unused sort import. PERF#9 (emitter Flush time.Now dedup) was intentionally skipped — lastFlush and firstFlushAt are semantically distinct timestamps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Found in the post-public-release docs review. Mechanical accuracy fixes
— the repo is now public, so stale docs are read by strangers.
- README §Benchmarks: numbers were stale (19 cases / -54.8% / 2026-05-10).
Updated to bench/baseline.md: 21 cases, -63.8%, 2026-05-18, 41,767 vs
115,410 tok.
- Delete docs/install.md — the orphaned original ASH-47/70 *planning*
doc, zero inbound links, contradicted by the live docs/adoption/install.md.
- docs/configuration.md: dropped the false "git --op status returns
not_implemented because go-git is stubbed" comment.
- internal/verbs/hook/hook.go: nudgeTail pointed contributors at
docs/session-notes/, a directory retired per CLAUDE.md — the most-seen
agent-facing string in the repo. Now points at the session feedback
ritual. Regenerated docs/vocab/inventory.{md,json}; hook tests assert
against the nudgeTail const directly now, not a magic substring.
- internal/verbs/initverb/template.md: dropped the retired "per-ship
session notes" reference.
- README §How we're building: corrected "no CLAUDE.md is propagated"
(ash init does) and "every read-side verb" over MCP (read- and
write-side; fixed the ash_lang_def example to ash_edit).
- README §Roadmap: build shipped — moved from Upcoming to Live; added
git blame to the live git ops.
- CLAUDE.md: not_implemented IS returned by git blame under the
shellout backend — corrected the error-code note.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediation of the verified findings from the post-public-release code review (docs / security / performance / correctness). Four commits, one per ticket.
ASH-210 — daemon shutdown panic + file-perm hardening
ashdpanicked on every shutdown when[lsp]is disabled (the default) —langCache.Close()on a nil receiver. The liveashd.logheld 98 panic traces; the crash skipped the deferredled.Close()/ WAL checkpoint. Fixed by makingCache.Close()nil-receiver-safe.cmd.Start()failure (fd leak).0o177umask (no world-connectable window).chmodledger.dbto0600(it holds paths + result fragments).ASH-211 — git argument injection (security, verified exploitable)
Request-controlled rev/pathspec args (
range/ref/rev/author/since/until/pathspec) reached the git argv unsanitized. With[git].backend="shellout",--range '--output=/path'madegit log --output=FILEwrite attacker-chosen files — andlog/showare read-side verbs exposed over MCP.ParseArgs(the single choke point for both backends) now rejects any of those args beginning with-. Regression test added.ASH-212 — hot-path perf
EncodeOrdinaryinstead ofEncode— drops the special-token scan + a redundant full[]runepass, on a function called 3-4× per request.argsBlobno longer re-decodes the request (one fewer full msgpack unmarshal per call).PrettyPathdrops a redundant per-call sort.ASH-213 — docs accuracy sweep
README benchmark numbers refreshed (21 cases / −63.8% / 2026-05-18); orphaned
docs/install.mddeleted; hook deny message no longer points at the retireddocs/session-notes/;not_implemented/lang/build/ MCP-surface claims corrected across README, CLAUDE.md, configuration.md. Vocab inventory regenerated.Verification
go vet, full test suite (54 pkgs),vocab-check,schema-checkall pass locally.Not in this PR
Pre-existing repo-wide gofmt drift (~70 files) — filed as ASH-220 with a CI-gate proposal; kept separate so the 70-file formatting diff doesn't bury these changes. Larger findings filed as ASH-214..219.
🤖 Generated with Claude Code