diff --git a/CHANGELOG.md b/CHANGELOG.md index 12bde790..a1e0d448 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security +- **Session lock TOCTOU eliminated**: `AcquireLock`'s stat → stale-if->5min → remove → O_EXCL dance could delete a live lock on misjudged staleness and let two instances open the same session. Mutual exclusion now uses an OS advisory lock (`gofrs/flock`, promoted to a direct dependency); a crashed holder's lock is reclaimed instantly because the kernel drops the flock at process death. The lock file keeps PID/timestamps purely as diagnostics. +- **Hardened atomic writes for state files**: global settings, checkpoint file contents and restores, handovers, and named checkpoints now go through `internal/safewrite` (same 0600 mode as before, plus fsync+rename atomicity and symlink refusal at the destination). +- **Dead shell-injection surface removed**: `AssumptionTracker.VerifyCommandSucceeds` ran caller-supplied strings through `sh -c`, bypassing the permission stack; it had zero callers and is deleted. `SelfHealer.RunScript` no longer shell-evaluates the script path (double evaluation) and invokes it directly via `/bin/sh`. + +### Fixed +- **Engine subprocesses are bounded and observable**: experiment-loop rollback, auto-commit git calls, and post-edit syntax validators (`go vet`, `python3`, `node`, `npx tsc`) ran on `context.Background()` with ignored errors; they are now time-bounded and log failures instead of discarding them. +- **Memory and config failures no longer silently dropped**: stream-loop memory persists (assistant learnings, skills, conversation summaries, insights) and the self-improve lesson store log failures via slog, corrupt lesson stores are reported, and the config panel surfaces failed `provider` setting saves instead of ignoring them. + +### Changed +- **Makefile lint pin matches CI**: `make lint`/`lint-fix`/`setup` install `golangci-lint@v2.1.0` (was `@latest`), the same version CI enforces. +- **Docs truth and housekeeping**: SECURITY.md/CONTRIBUTING.md now describe the actual Go toolchain (golangci-lint, go vet, govulncheck) instead of the polyglot template's ruff/mypy/pip-audit/pnpm-lock language, CONTRIBUTING documents `make setup`/`boundaries`/`test-10x`/`smoke`, and the planning docs (`SPEC_DRIVEN_PLAN.md`, `SPEC_DRIVEN_PHASE2_PLAN.md`, `internal/engine/REFACTOR_PLAN.md`) moved to `docs/plans/`. + ## [0.2.0] — 2026-07-13 ### Changed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e9c68510..5c1db48e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -29,12 +29,21 @@ for the full list. The most common targets: | `make test` | Run unit tests | | `make test-race` | Run unit tests with the race detector | | `make cover` | Generate a coverage report | -| `make lint` | Run the linter (`golangci-lint` / `ruff`) | +| `make lint` | Run `golangci-lint` (pinned to CI's version) | | `make fmt` | Format source files | -| `make vet` | Run `go vet` / `mypy` | -| `make security` | Run `govulncheck` / `pip-audit` | +| `make vet` | Run `go vet` | +| `make security` | Run `govulncheck` | | `make ci` | Run everything CI runs (the gate before pushing) | +Repo-specific dev targets: + +| Target | What it does | +| ------------------ | ------------------------------------------------------------------- | +| `make setup` | Set up local development environment (go.work + external repos). | +| `make boundaries` | Alias for all boundary guards (matches `make boundaries` in engine repos). | +| `make test-10x` | Run tests 10 times to surface flakes. | +| `make smoke` | Quick build + doctor + ecosystem verification. | + ## Commit message convention We use [Conventional Commits](https://www.conventionalcommits.org/). This diff --git a/Makefile b/Makefile index 71b44515..7484abb1 100644 --- a/Makefile +++ b/Makefile @@ -26,6 +26,8 @@ LDFLAGS := -s -w \ # --------------------------------------------------------------------------- GOBIN_DIR := $(shell go env GOPATH)/bin GOLANGCI := $(GOBIN_DIR)/golangci-lint +# Keep in sync with the pin in .github/workflows/ci.yml (lint job). +GOLANGCI_VERSION := v2.1.0 GOFUMPT := $(GOBIN_DIR)/gofumpt GOIMPORTS := $(GOBIN_DIR)/goimports GOVULNCHECK := $(GOBIN_DIR)/govulncheck @@ -134,11 +136,11 @@ submodule-release-parity: ## Verify every go.mod ecosystem version resolves to i bash ./scripts/check-submodule-release-parity.sh lint: ## Run golangci-lint. - @command -v $(GOLANGCI) >/dev/null 2>&1 || (echo "install: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" && exit 1) + @command -v $(GOLANGCI) >/dev/null 2>&1 || (echo "install: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_VERSION)" && exit 1) $(GOLANGCI) run ./... --timeout=5m lint-fix: ## Run golangci-lint with --fix. - @command -v $(GOLANGCI) >/dev/null 2>&1 || (echo "install: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" && exit 1) + @command -v $(GOLANGCI) >/dev/null 2>&1 || (echo "install: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_VERSION)" && exit 1) $(GOLANGCI) run ./... --fix --timeout=5m security: ## Run govulncheck. @@ -228,7 +230,7 @@ setup: ## Set up local development environment (go.work + external repos). @echo "=== Installing development tools ===" @command -v $(GOFUMPT) >/dev/null 2>&1 || go install mvdan.cc/gofumpt@latest || echo " ⚠ Could not install gofumpt" @command -v $(GOIMPORTS) >/dev/null 2>&1 || go install golang.org/x/tools/cmd/goimports@latest || echo " ⚠ Could not install goimports" - @command -v $(GOLANGCI) >/dev/null 2>&1 || go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest || echo " ⚠ Could not install golangci-lint" + @command -v $(GOLANGCI) >/dev/null 2>&1 || go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_VERSION) || echo " ⚠ Could not install golangci-lint" @command -v $(GOVULNCHECK) >/dev/null 2>&1 || go install golang.org/x/vuln/cmd/govulncheck@latest || echo " ⚠ Could not install govulncheck" @command -v lefthook >/dev/null 2>&1 || go install github.com/evilmartians/lefthook@latest || echo " ⚠ Could not install lefthook" @echo "✓ All tools installed" diff --git a/SECURITY.md b/SECURITY.md index 220b1cbe..47ebe5fd 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -46,11 +46,12 @@ We follow [coordinated vulnerability disclosure](https://en.wikipedia.org/wiki/C - **Dependency monitoring:** vulnerable dependencies are detected by `govulncheck`, which runs on every CI build (see "Vulnerability scanning"). -- **Static analysis:** `golangci-lint` / `ruff` / `mypy` enforced in CI. -- **Vulnerability scanning:** `govulncheck` (Go) / `pip-audit` (Python) run - on every CI build. -- **Lockfiles:** `go.sum` / `pnpm-lock.yaml` / `pyproject.toml` are pinned - and committed. +- **Static analysis:** `golangci-lint` (including `gosec` rules) and `go vet` + are enforced in CI. +- **Vulnerability scanning:** `govulncheck` runs on every CI build. +- **Dependency pinning:** `go.sum` is pinned and committed; ecosystem + submodules are pinned to exact Gitlinks and verified by + `make submodule-release-parity`. - **Reproducible builds:** release artefacts ship with SHA-256 checksums via goreleaser. - **No secrets in source:** API keys are configuration, not constants. Pre- diff --git a/cmd/chat_config_panel.go b/cmd/chat_config_panel.go index cfe1b107..64d608e2 100644 --- a/cmd/chat_config_panel.go +++ b/cmd/chat_config_panel.go @@ -965,12 +965,20 @@ func (m chatModel) selectConfigModelFromOptions(opts []configModelOption) (chatM return m.closeConfigPanel(), nil } m.session.SetModel(modelID) + // Same precedence as before (manual pick > gateway > provider), but a + // failed provider persist is surfaced instead of silently dropped. + provider := "" if gw := strings.TrimSpace(m.configModelProvider); gw != "" { - _ = hawkconfig.SetGlobalSetting("provider", gw) + provider = gw } else if gw := strings.TrimSpace(selected.GatewayID); gw != "" { - _ = hawkconfig.SetGlobalSetting("provider", gw) + provider = gw } else if prov := strings.TrimSpace(selected.ProviderID); prov != "" { - _ = hawkconfig.SetGlobalSetting("provider", prov) + provider = prov + } + if provider != "" { + if err := hawkconfig.SetGlobalSetting("provider", provider); err != nil { + m.messages = append(m.messages, displayMsg{role: "error", content: "model set, but saving provider failed: " + err.Error()}) + } } m.applyModelThinkingPref(selected) m.syncSessionSelection() diff --git a/SPEC_DRIVEN_PHASE2_PLAN.md b/docs/plans/SPEC_DRIVEN_PHASE2_PLAN.md similarity index 100% rename from SPEC_DRIVEN_PHASE2_PLAN.md rename to docs/plans/SPEC_DRIVEN_PHASE2_PLAN.md diff --git a/SPEC_DRIVEN_PLAN.md b/docs/plans/SPEC_DRIVEN_PLAN.md similarity index 100% rename from SPEC_DRIVEN_PLAN.md rename to docs/plans/SPEC_DRIVEN_PLAN.md diff --git a/internal/engine/REFACTOR_PLAN.md b/docs/plans/engine-refactor-plan.md similarity index 100% rename from internal/engine/REFACTOR_PLAN.md rename to docs/plans/engine-refactor-plan.md diff --git a/external/eyrie b/external/eyrie index 7ec579ab..ede66717 160000 --- a/external/eyrie +++ b/external/eyrie @@ -1 +1 @@ -Subproject commit 7ec579abfaa96ebc5f726e528c93f2d74e90414f +Subproject commit ede6671749b14807990ee7614cac2eb5192aa482 diff --git a/external/hawk-core-contracts b/external/hawk-core-contracts index 8f52f8f4..16ebcfd5 160000 --- a/external/hawk-core-contracts +++ b/external/hawk-core-contracts @@ -1 +1 @@ -Subproject commit 8f52f8f4504385344269137f2178d2fc85e583be +Subproject commit 16ebcfd5ad6e298c9cace718af8e958d2e3e299f diff --git a/external/hawk-mcpkit b/external/hawk-mcpkit index 85ac53f3..4a5ea251 160000 --- a/external/hawk-mcpkit +++ b/external/hawk-mcpkit @@ -1 +1 @@ -Subproject commit 85ac53f3ec847c607bc419ff7e72b08a8c73b6df +Subproject commit 4a5ea251cd7a87b935ea4ff75b467df1fdbf443c diff --git a/external/inspect b/external/inspect index 2a40bbb1..b5b65842 160000 --- a/external/inspect +++ b/external/inspect @@ -1 +1 @@ -Subproject commit 2a40bbb1727cc937b6f0514cc832f89af887af20 +Subproject commit b5b658421c8b798d03448a4309e516e5e1aeba18 diff --git a/external/sight b/external/sight index 84c96edf..481d3d1d 160000 --- a/external/sight +++ b/external/sight @@ -1 +1 @@ -Subproject commit 84c96edfc589e4ac6383ae11fb0ab91f73be80f0 +Subproject commit 481d3d1d57053417ecff1a96cbdcddd86dcd2790 diff --git a/external/tok b/external/tok index 5355fcab..643b6675 160000 --- a/external/tok +++ b/external/tok @@ -1 +1 @@ -Subproject commit 5355fcab2ef6cecbc79522d11e4003fe3d8a95b7 +Subproject commit 643b6675ebc75b32e448fbebcd3caba92b6d7583 diff --git a/external/trace b/external/trace index a31e98c7..59b437bb 160000 --- a/external/trace +++ b/external/trace @@ -1 +1 @@ -Subproject commit a31e98c7d75b661daf5714f3d2422d72dab06735 +Subproject commit 59b437bbe8dc62f1d601cd9f124ad9aad58b4b97 diff --git a/external/yaad b/external/yaad index ab5e6490..42bdda93 160000 --- a/external/yaad +++ b/external/yaad @@ -1 +1 @@ -Subproject commit ab5e6490c052919e9e293612c15201af0cf70c7b +Subproject commit 42bdda93995b0c53cf77cf94d59bf61556485fdd diff --git a/go.mod b/go.mod index 54e11fdd..eb89e6fe 100644 --- a/go.mod +++ b/go.mod @@ -11,17 +11,18 @@ require ( charm.land/bubbles/v2 v2.1.1 charm.land/bubbletea/v2 v2.0.8 charm.land/lipgloss/v2 v2.0.5 - github.com/GrayCodeAI/eyrie v0.2.3-0.20260813100729-7ec579abfaa9 - github.com/GrayCodeAI/hawk-core-contracts v0.1.12 - github.com/GrayCodeAI/inspect v0.0.0-20260813092651-2a40bbb1727c - github.com/GrayCodeAI/sight v0.0.0-20260726091804-84c96edfc589 - github.com/GrayCodeAI/tok v0.1.5-0.20260815024238-5355fcab2ef6 - github.com/GrayCodeAI/yaad v0.2.1-0.20260815153959-ab5e6490c052 + github.com/GrayCodeAI/eyrie v0.2.3-0.20260816034245-ede6671749b1 + github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e + github.com/GrayCodeAI/inspect v0.0.0-20260816034902-b5b658421c8b + github.com/GrayCodeAI/sight v0.0.0-20260816034858-481d3d1d5705 + github.com/GrayCodeAI/tok v0.1.5-0.20260816034249-643b6675ebc7 + github.com/GrayCodeAI/yaad v0.2.1-0.20260816034238-42bdda93995b github.com/alecthomas/chroma/v2 v2.26.1 github.com/bwmarrin/discordgo v0.28.1 github.com/charmbracelet/x/ansi v0.11.7 github.com/chromedp/chromedp v0.16.0 github.com/fsnotify/fsnotify v1.10.1 + github.com/gofrs/flock v0.13.0 github.com/google/uuid v1.6.0 github.com/mattn/go-runewidth v0.0.27 github.com/smacker/go-tree-sitter v0.0.0-20240827094217-dd81d9e9be82 @@ -42,7 +43,7 @@ require ( ) require ( - github.com/GrayCodeAI/hawk-mcpkit v0.1.6-0.20260729083555-85ac53f3ec84 // indirect + github.com/GrayCodeAI/hawk-mcpkit v0.1.6-0.20260816034242-4a5ea251cd7a // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/chromedp/cdproto v0.0.0-20260714215040-dc233986426f // indirect github.com/chromedp/sysutil v1.1.0 // indirect @@ -56,7 +57,6 @@ require ( github.com/gobwas/httphead v0.1.0 // indirect github.com/gobwas/pool v0.2.1 // indirect github.com/gobwas/ws v1.4.0 // indirect - github.com/gofrs/flock v0.13.0 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/ogen-go/ogen v1.23.0 // indirect github.com/oklog/ulid/v2 v2.1.2 // indirect @@ -157,7 +157,7 @@ require ( require ( github.com/BurntSushi/toml v1.6.0 - github.com/GrayCodeAI/trace v0.1.4-0.20260812042155-a31e98c7d75b + github.com/GrayCodeAI/trace v0.1.4-0.20260816034253-59b437bbe8dc github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect diff --git a/go.sum b/go.sum index b68f7585..413748bf 100644 --- a/go.sum +++ b/go.sum @@ -16,22 +16,22 @@ github.com/BobuSumisu/aho-corasick v1.0.3 h1:uuf+JHwU9CHP2Vx+wAy6jcksJThhJS9ehR8 github.com/BobuSumisu/aho-corasick v1.0.3/go.mod h1:hm4jLcvZKI2vRF2WDU1N4p/jpWtpOzp3nLmi9AzX/XE= github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk= github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= -github.com/GrayCodeAI/eyrie v0.2.3-0.20260813100729-7ec579abfaa9 h1:PmA9hUIiFWs66V4UNfB/NE/uw4LxHcmTS+055SeYGes= -github.com/GrayCodeAI/eyrie v0.2.3-0.20260813100729-7ec579abfaa9/go.mod h1:AW/UPuj+EWxMibiD+/Cy0TWd6RmTvth+KeGXSxU3t6I= -github.com/GrayCodeAI/hawk-core-contracts v0.1.12 h1:percfsd771JLmO9gMkrQtENEPBA9ZN3dG1Nc1moN3ZQ= -github.com/GrayCodeAI/hawk-core-contracts v0.1.12/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= -github.com/GrayCodeAI/hawk-mcpkit v0.1.6-0.20260729083555-85ac53f3ec84 h1:HzoXUYNNyt88IccaPBxSvOQ/5PZzJOcSHbvBjX3l2mQ= -github.com/GrayCodeAI/hawk-mcpkit v0.1.6-0.20260729083555-85ac53f3ec84/go.mod h1:C32HPDRqiDETbVbMIbOTvguek6KImpLCffJjet7sqck= -github.com/GrayCodeAI/inspect v0.0.0-20260813092651-2a40bbb1727c h1:IVGlQTaYScCL4wsEzCGPxgBP4s9QIXhtZTX6aeHUqMw= -github.com/GrayCodeAI/inspect v0.0.0-20260813092651-2a40bbb1727c/go.mod h1:kSyO5gWDrBYKcYXHG4JNkJ/2yWAayAVPee8PIC24bX0= -github.com/GrayCodeAI/sight v0.0.0-20260726091804-84c96edfc589 h1:gMXVRCqqdth6ziqFYnw8nXXbrD7LDtlgbGpryJZ0r1Q= -github.com/GrayCodeAI/sight v0.0.0-20260726091804-84c96edfc589/go.mod h1:kSCQwmYLH/ek9xU81ITyUgeeoDJjigAC0dGYyAMmKmc= -github.com/GrayCodeAI/tok v0.1.5-0.20260815024238-5355fcab2ef6 h1:AsN1F0LbKtPeJNqW4WWJXUomtKJJExEYZf9QEdqNbxs= -github.com/GrayCodeAI/tok v0.1.5-0.20260815024238-5355fcab2ef6/go.mod h1:zHM1Ei/uHq2793uVY5mEtli4o9znAwtfdzPgSCrTjyQ= -github.com/GrayCodeAI/trace v0.1.4-0.20260812042155-a31e98c7d75b h1:xqdfzKQ+wzy7B0aTr14Y7CXply41M7z2LoG/qjO0cYE= -github.com/GrayCodeAI/trace v0.1.4-0.20260812042155-a31e98c7d75b/go.mod h1:RPt/KV4f2DKezxzqAPe960z3vVmO5eJUGB/ZDlV/sOI= -github.com/GrayCodeAI/yaad v0.2.1-0.20260815153959-ab5e6490c052 h1:0PWsscUHI4CgnlJhaijuQK0OQcATn8TMSQlAfiSlN+c= -github.com/GrayCodeAI/yaad v0.2.1-0.20260815153959-ab5e6490c052/go.mod h1:5qaVC0sdXT38zTH54JJZnUJmKOfCAKoVcrNaoIj4bXw= +github.com/GrayCodeAI/eyrie v0.2.3-0.20260816034245-ede6671749b1 h1:Zx11XUnTo6k6KaBQDIvNqB2gWYkuoFQmk/Hcj+Boi+w= +github.com/GrayCodeAI/eyrie v0.2.3-0.20260816034245-ede6671749b1/go.mod h1:UMHSsVERLnVntJDE1U4YwkJd4VXOrIj6ZyRQYV4Vrj0= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e h1:cG9bLB3rWmMVU/7GwDxUIrEFbGHz7OTD5s9I4K+udr0= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= +github.com/GrayCodeAI/hawk-mcpkit v0.1.6-0.20260816034242-4a5ea251cd7a h1:kvZ2IKMOEs3yxU0j+rEAF3rtlJ+f97/01Tlv3RmodA4= +github.com/GrayCodeAI/hawk-mcpkit v0.1.6-0.20260816034242-4a5ea251cd7a/go.mod h1:3JAUitpEyUL08KNOUP+p58zcxTuFIMF9/co/669Ncmo= +github.com/GrayCodeAI/inspect v0.0.0-20260816034902-b5b658421c8b h1:TTVybhl/x8fzsjWlv9sH02eOcjF4dd2EGYaC3aSBuik= +github.com/GrayCodeAI/inspect v0.0.0-20260816034902-b5b658421c8b/go.mod h1:ipnOyNHbY1I6H5BlZY4RBDmFBpRnuQbshwawTqzHS/8= +github.com/GrayCodeAI/sight v0.0.0-20260816034858-481d3d1d5705 h1:q463jd+dPzRmLsBWbA1CUfZHS+5VujpxTHOMXvdS2uI= +github.com/GrayCodeAI/sight v0.0.0-20260816034858-481d3d1d5705/go.mod h1:0D2fhnfizzjywVOx/QPIdMduOtv3nZt1xrC3SBYmaF8= +github.com/GrayCodeAI/tok v0.1.5-0.20260816034249-643b6675ebc7 h1:agvfUOO5eVzCyI/mCYFr5Di8bB4OQSgEhrKgMyViTSA= +github.com/GrayCodeAI/tok v0.1.5-0.20260816034249-643b6675ebc7/go.mod h1:zHM1Ei/uHq2793uVY5mEtli4o9znAwtfdzPgSCrTjyQ= +github.com/GrayCodeAI/trace v0.1.4-0.20260816034253-59b437bbe8dc h1:b83/X8ETGFfu8Dn976v9ZLZJy6O1pVpInnqKc7i/TjU= +github.com/GrayCodeAI/trace v0.1.4-0.20260816034253-59b437bbe8dc/go.mod h1:3IYIRSxM+ggLJmbzFM8undLSI0VaB7hFVkXzxRsyZaA= +github.com/GrayCodeAI/yaad v0.2.1-0.20260816034238-42bdda93995b h1:ERRJu8E87qSA02j/9UM/3HqUNKSCYIQEnDWlQXHmix4= +github.com/GrayCodeAI/yaad v0.2.1-0.20260816034238-42bdda93995b/go.mod h1:QPnkRii/n5BrEVgr5w/D1BKukRhW/jviKhTqJxcBLNQ= github.com/MakeNowJust/heredoc v1.0.0 h1:cXCdzVdstXyiTqTvfqk9SDHpKNjxuom+DOlyEeQ4pzQ= github.com/MakeNowJust/heredoc v1.0.0/go.mod h1:mG5amYoWBHf8vpLOuehzbGGw0EHxpZZ6lCpQ4fNJ8LE= github.com/Masterminds/semver/v3 v3.5.0 h1:kQceYJfbupGfZOKZQg0kou0DgAKhzDg2NZPAwZ/2OOE= diff --git a/internal/config/settings.go b/internal/config/settings.go index 91ee0147..79e9c43b 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -15,6 +15,7 @@ import ( "github.com/GrayCodeAI/hawk/internal/provider/gateway" "github.com/GrayCodeAI/hawk/internal/provider/routing" + "github.com/GrayCodeAI/hawk/internal/safewrite" "github.com/GrayCodeAI/hawk/internal/storage" "github.com/GrayCodeAI/hawk/internal/types" @@ -401,8 +402,9 @@ func SaveGlobal(s Settings) error { if err != nil { return err } - // 0600: per-user config; keep it unreadable to other local users. - if err := os.WriteFile(globalSettingsPath(), data, 0o600); err != nil { + // safewrite keeps the previous 0600 mode (per-user config, unreadable to + // other local users) while making the write atomic and symlink-resistant. + if err := safewrite.WriteFile(globalSettingsPath(), data); err != nil { return err } // Invalidate the in-process byte cache so subsequent loads within the diff --git a/internal/engine/agent/aliases.go b/internal/engine/agent/aliases.go index 0602af5b..3de01654 100644 --- a/internal/engine/agent/aliases.go +++ b/internal/engine/agent/aliases.go @@ -1,3 +1,3 @@ // Package agent is the namespace for sub-agent orchestration types. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package agent diff --git a/internal/engine/agent_reexports.go b/internal/engine/agent_reexports.go index 41caf109..9ab95d95 100644 --- a/internal/engine/agent_reexports.go +++ b/internal/engine/agent_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the agent sub-package so that existing // callers of engine.SubAgentMode, engine.NewSubAgentBudget, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import ( diff --git a/internal/engine/assumptions.go b/internal/engine/assumptions.go index e5b2c6e9..1c1f7bda 100644 --- a/internal/engine/assumptions.go +++ b/internal/engine/assumptions.go @@ -1,10 +1,8 @@ package engine import ( - "context" "fmt" "os" - "os/exec" "strings" "sync" @@ -60,21 +58,8 @@ func (at *AssumptionTracker) VerifyFileExists(text, path string) { at.Assumptions = append(at.Assumptions, a) } -// VerifyCommandSucceeds checks if a command-based assumption holds. -func (at *AssumptionTracker) VerifyCommandSucceeds(text, cmd string) { - at.mu.Lock() - defer at.mu.Unlock() - a := Assumption{Text: text} - out, err := exec.CommandContext(context.Background(), "sh", "-c", cmd).CombinedOutput() // #nosec G204 -- intentional assumption-check command boundary - if err == nil { - a.Status = AssumptionConfirmed - a.Proof = "command succeeded" - } else { - a.Status = AssumptionFailed - a.Proof = strings.TrimSpace(string(out)) - } - at.Assumptions = append(at.Assumptions, a) -} +// VerifyCommandSucceeds was removed: it ran caller-supplied strings through +// `sh -c`, bypassing the permission/safety stack, and had no callers. // Failed returns all assumptions that were proven wrong. func (at *AssumptionTracker) Failed() []Assumption { diff --git a/internal/engine/auto_commit.go b/internal/engine/auto_commit.go index 422bf091..1f813ccb 100644 --- a/internal/engine/auto_commit.go +++ b/internal/engine/auto_commit.go @@ -3,11 +3,18 @@ package engine import ( "context" "fmt" + "log/slog" "os/exec" "strings" "time" ) +// autoCommitTimeout bounds each git operation run by the auto-committer. +// These operations run after an edit completes, detached from any request +// context (the caller has none to give), so a hung git invocation — e.g. on +// a slow network filesystem — cannot block the session forever. +const autoCommitTimeout = 2 * time.Minute + // AutoCommitter automatically commits changes after every successful edit. // Never lose work — every change is a git commit you can undo. type AutoCommitter struct { @@ -25,16 +32,25 @@ func (ac *AutoCommitter) CommitIfChanged(description string) error { if !ac.Enabled { return nil } + ctx, cancel := context.WithTimeout(context.Background(), autoCommitTimeout) + defer cancel() + // Check if there are changes - cmd := exec.CommandContext(context.Background(), "git", "status", "--porcelain") + cmd := exec.CommandContext(ctx, "git", "status", "--porcelain") cmd.Dir = ac.RepoDir out, err := cmd.Output() - if err != nil || len(strings.TrimSpace(string(out))) == 0 { + if err != nil { + // Distinguish "git status failed" from "nothing to commit" so the + // failure is not silently swallowed as a no-op. + slog.Warn("auto-commit: git status failed; skipping commit", "dir", ac.RepoDir, "error", err) + return nil + } + if len(strings.TrimSpace(string(out))) == 0 { return nil // no changes } // Stage all changes - stage := exec.CommandContext(context.Background(), "git", "add", "-A") + stage := exec.CommandContext(ctx, "git", "add", "-A") stage.Dir = ac.RepoDir if err := stage.Run(); err != nil { return err @@ -44,14 +60,17 @@ func (ac *AutoCommitter) CommitIfChanged(description string) error { msg := ac.generateMessage(description) // Commit - commit := exec.CommandContext(context.Background(), "git", "commit", "-m", msg, "--no-verify") // #nosec G204 -- git subcommand invocation with fixed subcommand and internally-derived args + commit := exec.CommandContext(ctx, "git", "commit", "-m", msg, "--no-verify") // #nosec G204 -- git subcommand invocation with fixed subcommand and internally-derived args commit.Dir = ac.RepoDir return commit.Run() } // Undo reverts the last auto-commit. func (ac *AutoCommitter) Undo() error { - cmd := exec.CommandContext(context.Background(), "git", "reset", "--soft", "HEAD~1") + ctx, cancel := context.WithTimeout(context.Background(), autoCommitTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "git", "reset", "--soft", "HEAD~1") cmd.Dir = ac.RepoDir return cmd.Run() } diff --git a/internal/engine/code/aliases.go b/internal/engine/code/aliases.go index 8f20a0bb..38ab7934 100644 --- a/internal/engine/code/aliases.go +++ b/internal/engine/code/aliases.go @@ -1,3 +1,3 @@ // Package code provides code-aware features: context extraction, -// lenses, actions, and explainer. See ../REFACTOR_PLAN.md. +// lenses, actions, and explainer. See ../../docs/plans/engine-refactor-plan.md. package code diff --git a/internal/engine/compact/aliases.go b/internal/engine/compact/aliases.go index 25f50c3e..0d149aac 100644 --- a/internal/engine/compact/aliases.go +++ b/internal/engine/compact/aliases.go @@ -1,5 +1,5 @@ // Package compact provides compaction strategies, types, and helpers -// for context-window management. See ../REFACTOR_PLAN.md. +// for context-window management. See ../../docs/plans/engine-refactor-plan.md. package compact // Result is the outcome of a compaction pass. diff --git a/internal/engine/compact_reexports.go b/internal/engine/compact_reexports.go index 716e818e..a11ec4ee 100644 --- a/internal/engine/compact_reexports.go +++ b/internal/engine/compact_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the compact sub-package so that existing // callers of engine.* keep compiling during the Stage 2 migration. -// See REFACTOR_PLAN.md. +// See docs/plans/engine-refactor-plan.md. package engine import ( diff --git a/internal/engine/compression/aliases.go b/internal/engine/compression/aliases.go index ddfc20d6..86de74da 100644 --- a/internal/engine/compression/aliases.go +++ b/internal/engine/compression/aliases.go @@ -1,5 +1,5 @@ // Package session is the Stage-1 namespace for session-lifecycle types in -// package engine. See ../REFACTOR_PLAN.md. +// package engine. See ../../docs/plans/engine-refactor-plan.md. package compression // Compressor is a shorter name for SessionCompressor. diff --git a/internal/engine/cost/aliases.go b/internal/engine/cost/aliases.go index 8170cc06..ca0f62bb 100644 --- a/internal/engine/cost/aliases.go +++ b/internal/engine/cost/aliases.go @@ -1,3 +1,3 @@ // Package cost provides cost tracking, optimisation, and display -// for the hawk engine. See ../REFACTOR_PLAN.md. +// for the hawk engine. See ../../docs/plans/engine-refactor-plan.md. package cost diff --git a/internal/engine/ctxmgr/aliases.go b/internal/engine/ctxmgr/aliases.go index 07cac927..bf243b7d 100644 --- a/internal/engine/ctxmgr/aliases.go +++ b/internal/engine/ctxmgr/aliases.go @@ -1,5 +1,5 @@ // Package ctxmgr is the namespace for context budget, decay, packing, -// providers, visualisation, and read-only context. See ../REFACTOR_PLAN.md. +// providers, visualisation, and read-only context. See ../../docs/plans/engine-refactor-plan.md. // // Named "ctxmgr" (not "context") to avoid shadowing the stdlib context package. package ctxmgr diff --git a/internal/engine/diff/aliases.go b/internal/engine/diff/aliases.go index 0c2c471c..1887d8c8 100644 --- a/internal/engine/diff/aliases.go +++ b/internal/engine/diff/aliases.go @@ -1,5 +1,5 @@ // Package diff is the Stage-1 namespace for diff sandbox, staging, preview, -// summariser, test selector, and 3-way merge. See ../REFACTOR_PLAN.md. +// summariser, test selector, and 3-way merge. See ../../docs/plans/engine-refactor-plan.md. package diff type ( diff --git a/internal/engine/experiment_loop.go b/internal/engine/experiment_loop.go index 005b1df5..651bb69e 100644 --- a/internal/engine/experiment_loop.go +++ b/internal/engine/experiment_loop.go @@ -3,6 +3,7 @@ package engine import ( "context" "fmt" + "log/slog" "os" "os/exec" "path/filepath" @@ -12,6 +13,12 @@ import ( "github.com/GrayCodeAI/hawk/internal/ui/icons" ) +// gitRollbackTimeout bounds rollback git operations in the experiment loop. +// Rollback deliberately uses a detached context: reverting an experiment must +// complete even when the surrounding request context has already been +// canceled, but it must not hang forever on a stuck git invocation. +const gitRollbackTimeout = 2 * time.Minute + // ExperimentResult holds the outcome of a single autonomous experiment. type ExperimentResult struct { ID int @@ -59,7 +66,7 @@ func (el *ExperimentLoop) Run(ctx context.Context, modifyFn func(ctx context.Con } // Snapshot current state - snapshot, err := el.snapshot() + snapshot, err := el.snapshot(ctx) if err != nil { return fmt.Errorf("snapshot failed: %w", err) } @@ -117,28 +124,44 @@ func (el *ExperimentLoop) validate(ctx context.Context) (bool, string) { } // snapshot captures git state for rollback. -func (el *ExperimentLoop) snapshot() (string, error) { - cmd := exec.CommandContext(context.Background(), "git", "stash", "create") +func (el *ExperimentLoop) snapshot(ctx context.Context) (string, error) { + cmd := exec.CommandContext(ctx, "git", "stash", "create") cmd.Dir = el.WorkDir out, err := cmd.Output() if err != nil { // No changes to stash — use HEAD - cmd = exec.CommandContext(context.Background(), "git", "rev-parse", "HEAD") + cmd = exec.CommandContext(ctx, "git", "rev-parse", "HEAD") cmd.Dir = el.WorkDir - out, _ = cmd.Output() + out, err = cmd.Output() + if err != nil { + // Fall back to the empty-ref rollback path, but make the failure + // visible instead of silently discarding it. + slog.Warn("experiment loop: git rev-parse HEAD failed; falling back to checkout rollback", "dir", el.WorkDir, "error", err) + } } return strings.TrimSpace(string(out)), nil } -// restore reverts to a snapshot. +// restore reverts to a snapshot. It uses a detached, time-bounded context +// (gitRollbackTimeout) rather than the request context because a rollback +// must still complete when the request has just been canceled. func (el *ExperimentLoop) restore(ref string) { + ctx, cancel := context.WithTimeout(context.Background(), gitRollbackTimeout) + defer cancel() + if ref == "" { - _ = exec.CommandContext(context.Background(), "git", "checkout", "--", ".").Run() + // NOTE: no WorkDir here (pre-existing behaviour): rolls back the + // process CWD when no snapshot ref was captured. + if err := exec.CommandContext(ctx, "git", "checkout", "--", ".").Run(); err != nil { + slog.Warn("experiment loop: rollback (checkout) failed", "error", err) + } return } - cmd := exec.CommandContext(context.Background(), "git", "checkout", "--", ".") + cmd := exec.CommandContext(ctx, "git", "checkout", "--", ".") cmd.Dir = el.WorkDir - _ = cmd.Run() + if err := cmd.Run(); err != nil { + slog.Warn("experiment loop: rollback (checkout) failed", "dir", el.WorkDir, "error", err) + } } // Summary returns a formatted summary of all experiments. diff --git a/internal/engine/git/aliases.go b/internal/engine/git/aliases.go index 1154ced9..0664b070 100644 --- a/internal/engine/git/aliases.go +++ b/internal/engine/git/aliases.go @@ -1,5 +1,5 @@ // Package git provides git-context enrichment and remote-forge integration. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package git // Context wraps a local git repo and exposes file/commit/blame queries. diff --git a/internal/engine/intelligence/aliases.go b/internal/engine/intelligence/aliases.go index dda8a1c6..b8b86029 100644 --- a/internal/engine/intelligence/aliases.go +++ b/internal/engine/intelligence/aliases.go @@ -1,3 +1,3 @@ // Package intelligence is the Stage-1 namespace for intent classification, capabilities, language support, tool selection. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package intelligence diff --git a/internal/engine/intelligence_reexports.go b/internal/engine/intelligence_reexports.go index e59571fc..3137743c 100644 --- a/internal/engine/intelligence_reexports.go +++ b/internal/engine/intelligence_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the intelligence sub-package so that existing // callers of engine.Intent, engine.NewIntentClassifier, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import "github.com/GrayCodeAI/hawk/internal/engine/intelligence" diff --git a/internal/engine/lifecycle/aliases.go b/internal/engine/lifecycle/aliases.go index 24765be4..fbef65d5 100644 --- a/internal/engine/lifecycle/aliases.go +++ b/internal/engine/lifecycle/aliases.go @@ -1,6 +1,6 @@ // Package lifecycle is the Stage-1 namespace for session lifecycle, limits, // timeouts, and sleep-time operations. After Stage 2 the implementation lives -// here and the engine root re-exports the public API. See ../REFACTOR_PLAN.md. +// here and the engine root re-exports the public API. See ../../docs/plans/engine-refactor-plan.md. // // Note: engine.go (the Engine type itself) stays in the root engine package // as the coordinator — it is NOT re-exported here. This cluster covers the diff --git a/internal/engine/lifecycle_reexports.go b/internal/engine/lifecycle_reexports.go index cf9af405..076b4d89 100644 --- a/internal/engine/lifecycle_reexports.go +++ b/internal/engine/lifecycle_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the lifecycle sub-package so that existing // callers of engine.SessionLifecycle, engine.NewLimitTracker, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import "github.com/GrayCodeAI/hawk/internal/engine/lifecycle" diff --git a/internal/engine/observability/aliases.go b/internal/engine/observability/aliases.go index afdb51dc..75a2410f 100644 --- a/internal/engine/observability/aliases.go +++ b/internal/engine/observability/aliases.go @@ -1,3 +1,3 @@ // Package observability is the Stage-1 namespace for profiling, debug recording, structured logging, feedback. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package observability diff --git a/internal/engine/observability_reexports.go b/internal/engine/observability_reexports.go index f81f98cf..b4706e4d 100644 --- a/internal/engine/observability_reexports.go +++ b/internal/engine/observability_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the observability sub-package so that existing // callers of engine.Profiler, engine.NewProfiler, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import "github.com/GrayCodeAI/hawk/internal/engine/observability" diff --git a/internal/engine/planning/aliases.go b/internal/engine/planning/aliases.go index e94c2935..a07151eb 100644 --- a/internal/engine/planning/aliases.go +++ b/internal/engine/planning/aliases.go @@ -1,3 +1,3 @@ // Package planning is the Stage-1 namespace for task planning, decomposition, goals, and suggested tasks. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package planning diff --git a/internal/engine/planning_reexports.go b/internal/engine/planning_reexports.go index 48040da5..2ca365b8 100644 --- a/internal/engine/planning_reexports.go +++ b/internal/engine/planning_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the planning sub-package so that existing // callers of engine.ExecutionPlan, engine.NewExecutionPlanner, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import "github.com/GrayCodeAI/hawk/internal/engine/planning" diff --git a/internal/engine/project/aliases.go b/internal/engine/project/aliases.go index aa6517b1..93617d3b 100644 --- a/internal/engine/project/aliases.go +++ b/internal/engine/project/aliases.go @@ -1,3 +1,3 @@ // Package project is the Stage-1 namespace for project analysis, snapshots, impact analysis, dep updates, migrations, releases. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package project diff --git a/internal/engine/project_reexports.go b/internal/engine/project_reexports.go index b7c71675..a658788b 100644 --- a/internal/engine/project_reexports.go +++ b/internal/engine/project_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the project sub-package so that existing // callers of engine.ProjectAnalysis, engine.NewProjectAnalyzer, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import "github.com/GrayCodeAI/hawk/internal/engine/project" diff --git a/internal/engine/review/aliases.go b/internal/engine/review/aliases.go index c65225c8..bd08c1eb 100644 --- a/internal/engine/review/aliases.go +++ b/internal/engine/review/aliases.go @@ -1,5 +1,5 @@ // Package review is the Stage-1 namespace for self-review / critique / quality -// scoring types in package engine. See ../REFACTOR_PLAN.md. +// scoring types in package engine. See ../../docs/plans/engine-refactor-plan.md. package review // Bot is the rule-driven review bot for diffs. diff --git a/internal/engine/scaffold/aliases.go b/internal/engine/scaffold/aliases.go index 120b02b9..0f2eb7fb 100644 --- a/internal/engine/scaffold/aliases.go +++ b/internal/engine/scaffold/aliases.go @@ -1,3 +1,3 @@ // Package scaffold is the Stage-1 namespace for scaffolding, recipes, -// patterns, skills, and few-shot types. See ../REFACTOR_PLAN.md. +// patterns, skills, and few-shot types. See ../../docs/plans/engine-refactor-plan.md. package scaffold diff --git a/internal/engine/self_heal.go b/internal/engine/self_heal.go index 0a48c21f..1ebd6d37 100644 --- a/internal/engine/self_heal.go +++ b/internal/engine/self_heal.go @@ -428,7 +428,12 @@ func (sh *SelfHealer) RunScript(ctx context.Context, path string) (stdout, stder ctx, cancel := context.WithTimeout(ctx, sh.Timeout) defer cancel() - cmd := exec.CommandContext(ctx, "sh", "-c", path) // #nosec G204 -- intentional self-heal script execution boundary + // Pass the path as a direct argument instead of `sh -c `: the + // shell-eval form re-parses the path, so a path containing shell + // metacharacters would be executed as shell code (double evaluation). + // /bin/sh treats a leading `#!` shebang line as a comment, so script + // semantics are unchanged. + cmd := exec.CommandContext(ctx, "/bin/sh", path) var outBuf, errBuf bytes.Buffer cmd.Stdout = &outBuf cmd.Stderr = &errBuf diff --git a/internal/engine/self_improve.go b/internal/engine/self_improve.go index edeedb3c..8372715b 100644 --- a/internal/engine/self_improve.go +++ b/internal/engine/self_improve.go @@ -3,6 +3,7 @@ package engine import ( "encoding/json" "fmt" + "log/slog" "os" "path/filepath" "sync" @@ -64,7 +65,11 @@ func (si *SelfImprover) Learn(what, why, lesson, category string) { if len(si.Entries) > maxSelfImproveEntries { si.Entries = si.Entries[len(si.Entries)-maxSelfImproveEntries:] } - si.save() + // Learn is best-effort by contract (nil-safe, fire-and-forget), but a + // failed persist must not be silent — the lesson is otherwise lost. + if err := si.save(); err != nil { + slog.Warn("self-improve: persisting lesson failed", "error", err) + } } // Lessons returns all lessons, optionally filtered by category. @@ -97,7 +102,9 @@ func (si *SelfImprover) Clear() { return } si.Entries = nil - si.save() + if err := si.save(); err != nil { + slog.Warn("self-improve: clearing lesson store failed", "error", err) + } } // ForPrompt formats recent lessons as context for the system prompt. @@ -124,15 +131,27 @@ func (si *SelfImprover) ForPrompt(maxEntries int) string { func (si *SelfImprover) load() { data, err := os.ReadFile(si.Path) if err != nil { - return + return // no store yet — first run + } + if err := json.Unmarshal(data, &si.Entries); err != nil { + // A corrupt store should not crash the session; log and start fresh. + slog.Warn("self-improve: lesson store is corrupt; starting empty", "path", si.Path, "error", err) + si.Entries = nil } - _ = json.Unmarshal(data, &si.Entries) } -func (si *SelfImprover) save() { - _ = os.MkdirAll(filepath.Dir(si.Path), 0o750) - data, _ := json.MarshalIndent(si.Entries, "", " ") - _ = os.WriteFile(si.Path, data, 0o600) +func (si *SelfImprover) save() error { + if err := os.MkdirAll(filepath.Dir(si.Path), 0o750); err != nil { + return fmt.Errorf("self-improve: create state dir: %w", err) + } + data, err := json.MarshalIndent(si.Entries, "", " ") + if err != nil { + return fmt.Errorf("self-improve: marshal lessons: %w", err) + } + if err := os.WriteFile(si.Path, data, 0o600); err != nil { + return fmt.Errorf("self-improve: write lesson store: %w", err) + } + return nil } // LearnPrompt generates a prompt that asks a model to extract a lesson from a diff --git a/internal/engine/stage2_move.sh b/internal/engine/stage2_move.sh index 29bf2de9..3afaae30 100644 --- a/internal/engine/stage2_move.sh +++ b/internal/engine/stage2_move.sh @@ -75,7 +75,7 @@ REEXPORT="engine/${SUBPKG}_reexports.go" { echo "// This file re-exports symbols from the $SUBPKG sub-package so that existing" echo "// callers of engine.* keep compiling during Stage 2 migration." - echo "// See REFACTOR_PLAN.md." + echo "// See docs/plans/engine-refactor-plan.md." echo "package engine" echo "" echo "import \"github.com/GrayCodeAI/hawk/internal/engine/$SUBPKG\"" diff --git a/internal/engine/stream.go b/internal/engine/stream.go index cf7f655c..5ddc72c2 100644 --- a/internal/engine/stream.go +++ b/internal/engine/stream.go @@ -685,7 +685,9 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { // extension when the backend becomes context-aware. if s.MemorySvc().Memory() != nil && shouldRemember(textContent.String()) { go func(content string) { - _ = s.MemorySvc().Memory().Remember(content, "assistant_learning") + if err := s.MemorySvc().Memory().Remember(content, "assistant_learning"); err != nil { + slog.Warn("background assistant_learning remember failed", "error", err) + } }(textContent.String()) } } @@ -757,7 +759,9 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { return } content, _ := json.Marshal(skill) - _ = s.MemorySvc().Yaad().Remember(string(content), "skill") + if err := s.MemorySvc().Yaad().Remember(string(content), "skill"); err != nil { + slog.Warn("background skill remember failed", "error", err) + } }() } emit(StreamEvent{Type: "done"}) @@ -925,11 +929,15 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { } if userMsg != "" && assistantMsg != "" { condensed := fmt.Sprintf("Q: %s\nA: %s", truncate(userMsg, 200), truncate(assistantMsg, 300)) - _ = s.MemorySvc().Memory().Remember(condensed, "conversation") + if err := s.MemorySvc().Memory().Remember(condensed, "conversation"); err != nil { + slog.Warn("conversation remember failed", "error", err) + } } // Also save insights if the response has learning signals if assistantMsg != "" && shouldRemember(assistantMsg) { - _ = s.MemorySvc().Memory().Remember(truncate(assistantMsg, 500), "insight") + if err := s.MemorySvc().Memory().Remember(truncate(assistantMsg, 500), "insight"); err != nil { + slog.Warn("insight remember failed", "error", err) + } } } } diff --git a/internal/engine/streaming/aliases.go b/internal/engine/streaming/aliases.go index c4621dfd..38f46ae3 100644 --- a/internal/engine/streaming/aliases.go +++ b/internal/engine/streaming/aliases.go @@ -1,4 +1,4 @@ // Package streaming is the Stage-1 namespace for response caching, // formatting, stream optimisation, thinking protocol, and steering. -// See ../REFACTOR_PLAN.md. +// See ../../docs/plans/engine-refactor-plan.md. package streaming diff --git a/internal/engine/streaming_reexports.go b/internal/engine/streaming_reexports.go index 174198e4..eb8cbf45 100644 --- a/internal/engine/streaming_reexports.go +++ b/internal/engine/streaming_reexports.go @@ -1,6 +1,6 @@ // This file re-exports symbols from the streaming sub-package so that existing // callers of engine.ResponseCache, engine.NewResponseCache, etc. keep compiling -// during the Stage 2 migration. See REFACTOR_PLAN.md. +// during the Stage 2 migration. See docs/plans/engine-refactor-plan.md. package engine import "github.com/GrayCodeAI/hawk/internal/engine/streaming" diff --git a/internal/engine/token/aliases.go b/internal/engine/token/aliases.go index 0154c6c5..cc79c96b 100644 --- a/internal/engine/token/aliases.go +++ b/internal/engine/token/aliases.go @@ -1,5 +1,5 @@ // Package token is the Stage-1 namespace for token-related types and -// functions in package engine. See ../REFACTOR_PLAN.md. +// functions in package engine. See ../../docs/plans/engine-refactor-plan.md. // // New code in hawk should import this package instead of reaching into // engine for token symbols. Implementation will move here in Stage 2. diff --git a/internal/engine/validate.go b/internal/engine/validate.go index aa3b02a5..7a547b3b 100644 --- a/internal/engine/validate.go +++ b/internal/engine/validate.go @@ -9,6 +9,7 @@ import ( "regexp" "strconv" "strings" + "time" ) // ValidationResult holds the outcome of validating a file. @@ -28,6 +29,13 @@ type ValidationError struct { // MaxAutoFixRetries is the maximum number of times to retry auto-fixing a file. const MaxAutoFixRetries = 3 +// validatorTimeout bounds each external syntax-validator invocation +// (go vet, python3, node, npx tsc). Validators run as part of post-edit +// checks without a request context; the timeout keeps a misbehaving +// toolchain (e.g. npx resolving packages, a wedged go build cache) from +// hanging the session forever. +const validatorTimeout = 5 * time.Minute + // ValidateFile determines the language from the file extension and runs // the appropriate syntax checker. func ValidateFile(path string) *ValidationResult { @@ -81,8 +89,11 @@ func languageValidator(ext string) func(path string) *ValidationResult { // validateGo runs go vet on a Go file and parses the output. func validateGo(path string) *ValidationResult { + ctx, cancel := context.WithTimeout(context.Background(), validatorTimeout) + defer cancel() + dir := filepath.Dir(path) - cmd := exec.CommandContext(context.Background(), "go", "vet", "./...") + cmd := exec.CommandContext(ctx, "go", "vet", "./...") cmd.Dir = dir output, err := cmd.CombinedOutput() @@ -108,7 +119,10 @@ func validateGo(path string) *ValidationResult { // validatePython runs py_compile on a Python file. func validatePython(path string) *ValidationResult { - cmd := exec.CommandContext(context.Background(), "python3", "-c", // #nosec G204 -- debugger/interpreter invocation with file path or expression from tool params + ctx, cancel := context.WithTimeout(context.Background(), validatorTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "python3", "-c", // #nosec G204 -- debugger/interpreter invocation with file path or expression from tool params fmt.Sprintf("import py_compile; py_compile.compile('%s', doraise=True)", path)) output, err := cmd.CombinedOutput() @@ -133,7 +147,10 @@ func validatePython(path string) *ValidationResult { // validateJS runs node --check on a JavaScript file. func validateJS(path string) *ValidationResult { - cmd := exec.CommandContext(context.Background(), "node", "--check", path) // #nosec G204 -- fixed Node executable + ctx, cancel := context.WithTimeout(context.Background(), validatorTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "node", "--check", path) // #nosec G204 -- fixed Node executable output, err := cmd.CombinedOutput() if err == nil { @@ -158,8 +175,11 @@ func validateJS(path string) *ValidationResult { // validateTS performs a basic syntax check for TypeScript files. // Uses npx tsc --noEmit if available, otherwise falls back to node --check. func validateTS(path string) *ValidationResult { + ctx, cancel := context.WithTimeout(context.Background(), validatorTimeout) + defer cancel() + // Try tsc first - cmd := exec.CommandContext(context.Background(), "npx", "tsc", "--noEmit", "--allowJs", path) // #nosec G204 -- fixed TypeScript compiler executable + cmd := exec.CommandContext(ctx, "npx", "tsc", "--noEmit", "--allowJs", path) // #nosec G204 -- fixed TypeScript compiler executable output, err := cmd.CombinedOutput() if err == nil { return &ValidationResult{Valid: true} diff --git a/internal/engine/validation/aliases.go b/internal/engine/validation/aliases.go index de310c8a..582acb58 100644 --- a/internal/engine/validation/aliases.go +++ b/internal/engine/validation/aliases.go @@ -1,3 +1,3 @@ // Package validation is the Stage-1 namespace for generated-code validation, -// schema validation, test loops, and lint loops. See ../REFACTOR_PLAN.md. +// schema validation, test loops, and lint loops. See ../../docs/plans/engine-refactor-plan.md. package validation diff --git a/internal/engine/workflow/aliases.go b/internal/engine/workflow/aliases.go index dce198f9..f80c9ed0 100644 --- a/internal/engine/workflow/aliases.go +++ b/internal/engine/workflow/aliases.go @@ -1,5 +1,5 @@ // Package workflow is the Stage-1 namespace for workflow + workspace + -// trajectory types in package engine. See ../REFACTOR_PLAN.md. +// trajectory types in package engine. See ../../docs/plans/engine-refactor-plan.md. package workflow import "context" diff --git a/internal/session/autosave.go b/internal/session/autosave.go index e8d5b6fa..fe98c4e3 100644 --- a/internal/session/autosave.go +++ b/internal/session/autosave.go @@ -3,11 +3,15 @@ package session import ( "bufio" "encoding/json" + "fmt" + "log/slog" "os" "path/filepath" "sync" "time" "unicode/utf8" + + "github.com/gofrs/flock" ) // AutoSaver periodically saves sessions and tracks session metadata. @@ -81,12 +85,27 @@ type SessionMeta struct { TokenCount int `json:"token_count,omitempty"` } +// lockStaleAfter is the age at which a lock file's diagnostics timestamp is +// considered stale. It is diagnostics-only: ownership is decided by flock(2), +// so staleness never causes a live lock to be removed. +const lockStaleAfter = 5 * time.Minute + // LockFile prevents double-opening a session. +// +// Mutual exclusion is an OS-level advisory lock (flock via gofrs/flock) on the +// lock file, so a lock held by a live process can never be deleted by another +// instance that misjudges it as stale — the old stat→remove→O_EXCL dance had +// exactly that TOCTOU race. The file's contents (PID + timestamps) are purely +// diagnostic; if the holder crashes, the kernel releases the flock and the +// next AcquireLock simply rewrites the file. type LockFile struct { path string + fl *flock.Flock } -// AcquireLock creates a lock file for the session. Returns error if already locked. +// AcquireLock takes the session lock. Returns a *SessionLockedError if +// another live instance holds it. A leftover lock file from a crashed +// process is reclaimed automatically because its flock died with the process. func AcquireLock(sessionID string) (*LockFile, error) { if err := ValidateID(sessionID); err != nil { return nil, err @@ -94,37 +113,55 @@ func AcquireLock(sessionID string) (*LockFile, error) { dir := sessionsDir() path := filepath.Join(dir, sessionID+".lock") - // Check if lock exists and is stale (>5 min old) - if info, err := os.Stat(path); err == nil { - if time.Since(info.ModTime()) > 5*time.Minute { - _ = os.Remove(path) // stale lock - } else { - return nil, &SessionLockedError{ID: sessionID} - } - } - - f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) // #nosec G304 -- path built from sessionsDir()+session ID, internal lock file + fl := flock.New(path) + ok, err := fl.TryLock() if err != nil { + return nil, fmt.Errorf("session %s: acquire lock: %w", sessionID, err) + } + if !ok { + // Diagnostics: report whether the holder's heartbeat looks abandoned. + // This never removes the lock — ownership is the kernel's call. + if info, statErr := os.Stat(path); statErr == nil && time.Since(info.ModTime()) > lockStaleAfter { + slog.Warn("session lock is held but its heartbeat is stale; holder may be hung (not removing: flock owns exclusion)", "session", sessionID, "lock_age", time.Since(info.ModTime()).Round(time.Second)) + } return nil, &SessionLockedError{ID: sessionID} } - _, _ = f.Write([]byte(time.Now().Format(time.RFC3339))) - _ = f.Close() - return &LockFile{path: path}, nil + writeLockDiagnostics(path, time.Now()) + + return &LockFile{path: path, fl: fl}, nil +} + +// writeLockDiagnostics records PID and timestamps in the lock file. Best +// effort: lock correctness never depends on these contents. +func writeLockDiagnostics(path string, acquiredAt time.Time) { + content := fmt.Sprintf("pid=%d acquired=%s\n", os.Getpid(), acquiredAt.Format(time.RFC3339)) + _ = os.WriteFile(path, []byte(content), 0o600) // #nosec G304 -- path built from sessionsDir()+session ID, internal lock file } -// Release removes the lock file. +// Release drops the lock. The diagnostics file is intentionally kept, with a +// released marker: unlinking right after unlock has a classic flock race +// where a concurrent opener holds the old inode while a later acquirer +// creates a fresh file at the same path — both would then "hold" a lock. +// Presence of the file means nothing; only flock ownership does. func (l *LockFile) Release() { - if l != nil && l.path != "" { - _ = os.Remove(l.path) + if l == nil || l.path == "" { + return + } + if l.fl != nil { + _ = l.fl.Unlock() } + content := fmt.Sprintf("pid=%d released=%s\n", os.Getpid(), time.Now().Format(time.RFC3339)) + _ = os.WriteFile(l.path, []byte(content), 0o600) // #nosec G304 -- path built from sessionsDir()+session ID, internal lock file } -// Refresh updates the lock file timestamp to prevent it from going stale. +// Refresh updates the lock file's heartbeat (PID + timestamp) so stale-lock +// diagnostics can tell an active holder from an abandoned one. func (l *LockFile) Refresh() { - if l != nil && l.path != "" { - _ = os.Chtimes(l.path, time.Now(), time.Now()) + if l == nil || l.path == "" { + return } + writeLockDiagnostics(l.path, time.Now()) } // SessionLockedError indicates a session is already open. diff --git a/internal/session/autosave_test.go b/internal/session/autosave_test.go index 9799b4aa..321b7fcd 100644 --- a/internal/session/autosave_test.go +++ b/internal/session/autosave_test.go @@ -4,7 +4,9 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" + "sync" "testing" "time" "unicode/utf8" @@ -284,6 +286,92 @@ func TestSessionLockedError(t *testing.T) { } } +// TestAcquireLock_ConcurrentOnlyOneWins fires N concurrent acquirers at the +// same session and asserts exactly one holds the lock while all of them have +// attempted — the mutual-exclusion property the stat→remove→O_EXCL design +// could not guarantee. +func TestAcquireLock_ConcurrentOnlyOneWins(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + _ = os.MkdirAll(setTestSessionsDir(t, dir), 0o755) + + const goroutines = 8 + var ( + mu sync.Mutex + held int + attempt sync.WaitGroup + release = make(chan struct{}) + done sync.WaitGroup + badErrTy int + ) + for i := 0; i < goroutines; i++ { + attempt.Add(1) + done.Add(1) + go func() { + defer done.Done() + lock, err := AcquireLock("concurrent-session") + // Signal the attempt BEFORE blocking on the release gate so + // attempt.Wait() cannot wait on a goroutine that is itself + // waiting for attempt.Wait() to proceed. + attempt.Done() + if err != nil { + var lockErr *SessionLockedError + if !errors.As(err, &lockErr) { + mu.Lock() + badErrTy++ + mu.Unlock() + } + return + } + mu.Lock() + held++ + mu.Unlock() + <-release // hold until every goroutine has attempted + lock.Release() + }() + } + attempt.Wait() + close(release) + done.Wait() + + if badErrTy != 0 { + t.Errorf("%d losers returned a non-SessionLockedError error", badErrTy) + } + if held != 1 { + t.Fatalf("expected exactly 1 concurrent lock holder, got %d", held) + } + + // After the holder releases, the lock must be acquirable again. + lock, err := AcquireLock("concurrent-session") + if err != nil { + t.Fatalf("re-acquire after release: %v", err) + } + lock.Release() +} + +// TestAcquireLock_CrashReclaim verifies a leftover lock file with no live +// flock holder (the post-crash state) is reclaimed without waiting out the +// stale window — the kernel dropped the flock when the holder died. +func TestAcquireLock_CrashReclaim(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + sessDir := setTestSessionsDir(t, dir) + _ = os.MkdirAll(sessDir, 0o755) + + // Simulate a crashed holder: lock file exists, fresh mtime, but no + // process holds the flock on it. + lockPath := filepath.Join(sessDir, "crashed-session.lock") + if err := os.WriteFile(lockPath, []byte("pid=999999 acquired=just-now\n"), 0o600); err != nil { + t.Fatal(err) + } + + lock, err := AcquireLock("crashed-session") + if err != nil { + t.Fatalf("should reclaim a lock whose holder died, got: %v", err) + } + lock.Release() +} + func TestAddTag(t *testing.T) { sess := &Session{Name: ""} AddTag(sess, "important") diff --git a/internal/session/checkpoint.go b/internal/session/checkpoint.go index 3d7f0a54..f9e748b4 100644 --- a/internal/session/checkpoint.go +++ b/internal/session/checkpoint.go @@ -454,7 +454,7 @@ func (cm *CheckpointManager) saveFileContents(id string, files []string) error { "content": string(content), } data, _ := json.Marshal(entry) - if err := os.WriteFile(filepath.Join(cpDir, safeName+".json"), data, 0o600); err != nil { + if err := safewrite.WriteFile(filepath.Join(cpDir, safeName+".json"), data); err != nil { return err } } @@ -492,7 +492,10 @@ func (cm *CheckpointManager) restoreFileContents(id string, filesState map[strin if err := os.MkdirAll(filepath.Dir(filePath), 0o750); err != nil { continue } - if err := os.WriteFile(filePath, []byte(content), 0o600); err != nil { + // safewrite keeps the previous 0600 mode and refuses to restore + // through a symlinked destination (an attacker-planted symlink at a + // restore path would otherwise receive checkpoint contents). + if err := safewrite.WriteFile(filePath, []byte(content)); err != nil { return fmt.Errorf("restore file %s: %w", filePath, err) } } diff --git a/internal/session/handover.go b/internal/session/handover.go index f7fa309a..04bae4d2 100644 --- a/internal/session/handover.go +++ b/internal/session/handover.go @@ -7,6 +7,8 @@ import ( "strings" "sync" "time" + + "github.com/GrayCodeAI/hawk/internal/safewrite" ) // Handover represents a session transfer between models, machines, or team members. @@ -363,7 +365,9 @@ func SaveHandover(handover *Handover, path string) error { if err != nil { return fmt.Errorf("marshal handover: %w", err) } - if err := os.WriteFile(path, data, 0o600); err != nil { + // safewrite keeps the previous 0600 mode while making the write atomic + // (temp + rename) and symlink-resistant. + if err := safewrite.WriteFile(path, data); err != nil { return fmt.Errorf("write handover: %w", err) } return nil diff --git a/internal/session/named_checkpoint.go b/internal/session/named_checkpoint.go index 3f99bb94..5c189501 100644 --- a/internal/session/named_checkpoint.go +++ b/internal/session/named_checkpoint.go @@ -8,6 +8,8 @@ import ( "sort" "strings" "time" + + "github.com/GrayCodeAI/hawk/internal/safewrite" ) // ───────────────────────────────────────────────────────────────────────────── @@ -95,14 +97,11 @@ func SaveNamedCheckpoint(name string, s *Session) (*NamedCheckpoint, error) { } target := namedCheckpointPath(name) - tmp := target + ".tmp" - if err := os.WriteFile(tmp, data, 0o600); err != nil { + // safewrite replaces the hand-rolled tmp+rename dance with the hardened + // atomic writer (same 0600 mode, plus symlink refusal and fsync). + if err := safewrite.WriteFile(target, data); err != nil { return nil, fmt.Errorf("write checkpoint: %w", err) } - if err := os.Rename(tmp, target); err != nil { - _ = os.Remove(tmp) - return nil, fmt.Errorf("atomic rename checkpoint: %w", err) - } return cp, nil } diff --git a/testdata/golden/help_root.txt b/testdata/golden/help_root.txt index 55a82abb..fb4e1da3 100644 --- a/testdata/golden/help_root.txt +++ b/testdata/golden/help_root.txt @@ -43,7 +43,6 @@ Available Commands: daemon Manage the hawk background server doctor Run local diagnostics ecosystem Show eyrie, yaad, and tok integration status - entire Entire CLI eval Evaluate model performance on coding benchmarks exec Execute a single command non-interactively features List and manage feature flags @@ -81,6 +80,7 @@ Available Commands: stats Show usage statistics and cost analytics taste Manage taste profile (learned coding style preferences) tools List built-in tools + trace Git-native session capture for AI coding agents trust Manage folder trust for project automation update Check for hawk updates verify Run local self-verification (security log, governance policy)