fix(goal,hooks): correct timeout units and permission aggregation - #40
Merged
Conversation
Goal: - judge timeout: DEFAULT_JUDGE_TIMEOUT was 30_000 (ms) used as "30000 seconds" in Effect.timeout — effectively 8.3h with no timeout. Changed to DEFAULT_JUDGE_TIMEOUT_SECONDS = 30. - pause message: removed misleading reference to non-existent auxiliary.goalJudge config (code never reads it; judge always uses provider.defaultModel). Hooks: - agent handler timeout: entry.timeout was used raw (seconds as ms). Added missing * 1000 to match all other handler types. - permissionDecision: was last-write-wins (later hook's allow could overwrite earlier hook's deny). Changed to most-restrictive- wins (deny > ask > allow), matching Claude Code semantics. - mcp handler: removed dead re-entry guard (inHook was always false from runEntry call site; guard never fired). Found by cross-review (FABLE5 model). Verified against current code.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Five fixes found by cross-review (FABLE5 model), verified against current code, applied.
Goal module (2 fixes)
G1: Judge timeout unit mismatch —
DEFAULT_JUDGE_TIMEOUTwas 30_000 (ms) used asEffect.timeout("30000 seconds")= 8.3hBefore:
DEFAULT_JUDGE_TIMEOUT = 30_000→Effect.timeout(${opts.timeout} seconds)→"30000 seconds"= effectively no timeout. A hanging provider would stall the goal loop for hours.After:
DEFAULT_JUDGE_TIMEOUT_SECONDS = 30→Effect.timeout("30 seconds"). Unit is now consistent.G2: Pause message referenced non-existent config
Before:
goal.ts:391told users to "配置 auxiliary.goalJudge" — but the code never reads this config (judge always usesprovider.defaultModel()). Users following the hint would see no effect.After: Message now says "检查模型配置或换用更可靠的模型" (generic guidance).
Hooks module (3 fixes)
H1: Agent handler timeout missing
* 1000Before:
settings.ts:1248—entry.timeout ?? DEFAULT_AGENT_TIMEOUT_MS— the ONLY handler that didn't multiply seconds by 1000. A user configuringtimeout: 120(intending 120 seconds) got 120ms.After:
entry.timeout ? entry.timeout * 1000 : DEFAULT_AGENT_TIMEOUT_MS— matches all other handler types (command/mcp/http/prompt).H2: permissionDecision was last-write-wins (security semantics)
Before:
settings.ts:1528— each hook'spermissionDecisionoverwrote the previous. A later hook'sallowcould override an earlier hook'sdeny.After: Most-restrictive-wins (deny > ask > allow). Once any hook says
deny, it staysdeny. Matches Claude Code permission semantics.H3: MCP re-entry guard was dead code
Before:
settings.ts:1053—if (inHook)guard, butrunEntryalways passedfalseforinHook(line 1480). The guard never fired.After: Guard removed; parameter renamed to
_inHook(matching all other handlers).Findings NOT fixed (assessed as not actionable)
hook/extensions/hot-reload.tswhich watches settings filesVerification
bun run typecheck: 0 errorsbun test test/goal/: 56 pass, 0 failNote
The previous dev CI run (PR #39) had 1 test failure in
attention.test.ts(TUI sound notification mock) — pre-existing, unrelated to goal-loop. 3079/3104 tests passed including all 56 goal tests.