fix: complete security, concurrency, architecture, and test hardening (rebased on flux) - #309
Merged
Merged
Conversation
Security: - validatePathAllowed now fails closed without a ToolContext; tests attach a permissive test context. - Delete the dead, bypassable BoundaryChecker (no production callers). - Wrap MCP/remote tool output as untrusted external content. - Bound HTTP clients in stt/media; safewrite uses crypto/rand + O_EXCL; hooks URL validation now actually rejects loopback; plugin index reads are capped. Concurrency: - jobs: snapshot cancel status under lock; guard nil Done. - git context, spec tools, watcher: bounded exec timeouts. - planning prompt: context-aware prompt + ctx timeout. - watcher: bounded fireChange workers. - filewatcher/cron: idempotent Stop (no double-close panic). - event bus RunWaterfall: snapshot handlers under lock. - AutoCommit errors logged; AssertWritable returns an error; SessionPreparations load/wait honor a context. Architecture: - Move IsSensitivePath/ResolvePath into internal/pathsafe; drop config->tool. - Consolidate byte-unsafe truncate copies onto textutil (rune-safe). - Delete dead types.ChatClient and the dead markdown_renderer.
- Fix a real bug in spec extractDescription: the requirement body excludes the header, so descriptions were always empty and every ADDED/MODIFIED requirement failed SHALL/MUST validation. - Add spec tests (parse/validate/apply/DAG/config) lifting coverage from 2.4% to 24.3%, plus a fuzz target and benchmarks. - Make ContextDecay clock injectable; rewrite the timing-flaky decay tests deterministically. - Un-skip TestParallelExecution, TestIntegration_FullSessionFlow, and the two config-apply tests; remove the blanket CI -skip. - Golden test restores rootCmd globals; add make update-golden. - Add testutil.Eventually/Never. - CI: per-package coverage floors, FuzzParseDeltaSpec target, version fixture aligned to 0.0.1.
applyRename built a regexp with regexp.MustCompile from an unescaped requirement name; a name containing invalid UTF-8 (or a bad pattern) panicked the whole process. Use regexp.Compile and fall back to leaving the content unchanged, and ReplaceAllLiteralString so $$ in the new name is not treated as a group reference. Found by the new FuzzParseDeltaSpec target.
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.
Re-applies the hardening work from the closed #307 on top of the new
main(flux migration #308). Four commits:Security
validatePathAllowedfails closed without aToolContext; tests attach a permissive context.BoundaryChecker.safewriteusescrypto/rand+O_EXCL; hook URL validation rejects loopback; plugin index reads capped.Concurrency
cmd.Waitrace fixed;lastAutoCommitHashmutex; daemon review subprocess tracking.Doneguard.Stopfor filewatcher/cron;RunWaterfallsnapshots handlers under lock.AutoCommiterrors logged;AssertWritablereturns an error;SessionPreparationshonors a context.Architecture
IsSensitivePath/ResolvePathtointernal/pathsafe; drop theconfig → tooledge.truncatecopies onto rune-safetextutil.types.ChatClientandcmd/markdown_renderer.go.Tests / CI
spec.extractDescriptionalways returned empty, so every ADDED/MODIFIED requirement failed SHALL/MUST validation.spec.applyRenamepanicked on a requirement name containing invalid UTF-8 (regexp.MustCompileon an unescaped name).FuzzParseDeltaSpec+ benchmarks.ContextDecayclock; un-skipped flaky/provider tests; removed the blanket CI-skip.make update-golden;testutil.Eventually/Never.fmt-check.Verified locally:
go vet,golangci-lint(0 issues), fullgo test ./...,-raceon changed packages,make boundaries, coverage 68.1%, per-package floors pass.