fix(engine): respect ctx cancel on stream retry, surface self-review revert failure#32
Merged
Merged
Conversation
ai_passage.md was a 53-line, ~1000-word essay on the history and ethics of AI in general — entirely unrelated to the hawk project, no README/AGENTS.md/CHANGELOG.md reference to it. It looks like LLM-generated filler committed in '99261ca Fix CI formatting and toolchain hygiene' to satisfy a 'must have an essay' requirement that no longer applies. Untrack and delete.
…revert failure
Three related fixes in the hawk agent loop:
1. **stream.go Stream retry no longer blocks ctx cancellation.**
Previously: `time.Sleep(time.Duration(streamAttempt+1) * time.Second)` — a
user-initiated cancel during a reasoning-only retry's backoff was ignored for
up to 3s. Now uses a select with ctx.Done() so cancel is observed immediately,
the in-flight stream is closed, and the loop exits with a structured error event.
2. **stream_tool_exec.go Self-review-before-apply now returns a hard error
when the revert itself fails.** Previously: if the LLM said 'this diff is bad'
and we tried to revert (os.Remove or os.WriteFile) and that syscall failed, the
code only logged s.log.Warn and proceeded with the rejected diff still on disk.
Now: capture the revert error, log it at Error level, and surface a hard tool
error ('Self-review rejected the change AND the revert failed: <err>. Manual
intervention required.') so the LLM can't continue building on top of code it
just flagged as broken.
3. **internal/tool/tool.go now exports ReadOnlyTools + IsReadOnly() so the
safe-concurrent allowlist has a single source of truth.** The map was previously
duplicated in stream.go:716 and stream_tool_exec.go:29 with identical content
— drift waiting to happen. Both call sites now go through tool.IsReadOnly(name),
which canonicalises aliases (read/file_read, ls, etc.) before lookup. A new
test (TestIsReadOnly, TestReadOnlyToolsSetContainsExpectedNames) locks the
contract so a future removal of e.g. 'Read' from the allowlist fails CI in
internal/tool/ rather than silently breaking concurrency classification in
internal/engine/.
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.
Three related fixes in the hawk agent loop.
1. Stream retry no longer blocks ctx cancellation
hawk/internal/engine/stream.go:479Previously:
time.Sleep(time.Duration(streamAttempt+1) * time.Second)— a user-initiated cancel during a reasoning-only retry's backoff was ignored for up to 3s.Now uses a
selectwithctx.Done()so cancel is observed immediately, the in-flight stream is closed viaresult.Close(), and the loop exits with a structured error event.2. Self-review-before-apply now returns a hard error when revert fails
hawk/internal/engine/stream_tool_exec.go:208-230Previously: if the LLM said 'this diff is bad' and we tried to revert (
os.Removeoros.WriteFile) and that syscall failed, the code only loggeds.log.Warnand proceeded with the rejected diff still on disk.Now: capture the revert error, log it at Error level, and surface a hard tool error ("Self-review rejected the change AND the revert failed: ${err}. Manual intervention required.") so the LLM can't continue building on top of code it just flagged as broken.
3.
safeConcurrentallowlist consolidated intotool.ReadOnlyToolshawk/internal/tool/tool.goand the two call sites.Previously:
stream.go:716andstream_tool_exec.go:29both hardcoded the same map{Read, Grep, Glob, LS, WebSearch, WebFetch, ToolSearch}— drift waiting to happen.Now:
internal/tool/tool.goexportsReadOnlyTools(the set) andIsReadOnly(name)(canonicalising lookup). Both call sites go through it. A newTestIsReadOnly+TestReadOnlyToolsSetContainsExpectedNamestest locks the contract so a future removal of e.g.Readfrom the allowlist fails CI ininternal/tool/rather than silently breaking concurrency classification ininternal/engine/.Verification
go build ./...✓go test ./internal/tool/ -count=1✓go test ./internal/engine/ -count=1 -short✓gofumpt -l internal/tool/ internal/engine/clean