fix: M1-M11 medium-severity issues from prior code review - #53
Merged
Conversation
- chat_subcommand_pin.go: use m.session.SetPinnedMessages - chat_subcommand_simple.go: use m.session.SetGLMThinkingEnabled - chat_subcommand_status.go: use m.session.CostValue().Summary() - session.go: add SetPinnedMessages accessor
- chat.go: use PermSvc().Autonomy()/SetAutonomy, SetContainerExecutor, SetContainerRequired - permissions_center.go: use PermSvc().SetAutonomy - welcome_gate.go, statusbar.go: use PermSvc().Autonomy() - hud_panel.go: use MemorySvc().Yaad() - chat_status.go: use ContextWindowCachedValue, Persistence().SetContextWindowCached - chat_subcommand_branches.go: use Persistence().DAG() - chat_commands_session.go: use Persistence().DAG() - session.go: add SetContainerExecutor accessor - audit_test.go: regex now matches m.session.X; cmdHardFailThreshold=0
- statusbar.go: use m.session.CostValue() - chat_subcommand_cost.go, chat_subcommand_usage.go: use CostValue() - chat_commands_session.go, chat_commands_util.go: use CostValue() - chat.go: use ContextWindowCachedValue() - permissions_center.go: use ModeValue() (safe fallback for test struct literal) - chat_model_test.go, statusbar_test.go: use accessors - session_sync.go, snapshot_cmd.go: avoid audit regex false positives in text - session.go: add SetContextWindowCached, ModeValue, SetMode accessors
…e().DAG() - AddUser: read Memory from s.MemorySvc().Memory() (was s.Memory) - AddUser, AddUserWithImage, AddAssistant, ForkConversation, SwitchBranch, ListBranches, ConvoHead: read DAG from s.Persistence().DAG() (was s.ConvoDAG) - Aligns with the H6 sub-service decomposition: the legacy ConvoDAG and Memory fields on Session are aliases set by NewSessionWithClient; new code goes through the sub-service getters to keep the god-object-decomposition contract consistent.
- FewShot() -> FewShotStore() (typo fix in comment) - PlanState, Trajectory, LintLoop, TestLoop, FileMentions, Files, Snapshots, Tracer, RateLimiter: comments now indicate these remain on the legacy Session struct (no sub-service accessor exists); the previous deprecation text pointed to non-existent methods that callers could not find.
NewPermissionService no longer initializes mode to PermissionModeDefault;
the zero value ("") is preserved so IsZero() can correctly report a
freshly constructed service. IsZero() now checks for the empty string
instead of PermissionModeDefault. This is the right semantics: callers
that want a default mode should call SetMode explicitly, and tests can
rely on IsZero() being true for a fresh service.
The previous implementation checked for primary-name duplicates (`r.primary[name]`) but not alias collisions. Two subcommands that each register a different primary but share an alias would silently overwrite `r.aliasOf[alias]`. Now the alias check mirrors the primary-name check: registration is rejected if any of the subcommand's aliases is already taken. The existing silent-no-op pattern is preserved (no error return) so init() callers don't have to change.
The previous Handle passed the post-name args slice directly to handleSessionCommand, which expects parts[0] to be the command name. This broke /recover <id>, /resume <id>, and /tag <label>: the trailing arg landed at parts[0] instead of parts[1], and the `len(parts) >= 2` check failed, hitting the usage error path. The fix reconstructs the full parts slice (name + args) before calling handleSessionCommand, matching the contract the simple.go delegatingCommand handlers use. The parts-building logic is extracted into a buildSessionParts helper for testability. New tests (TestBuildSessionParts_*, TestSessionSubcommand_RecoverIDReachesPartsIndex1, TestResolveSessionName_PicksLongestMatch) assert the fix and the dispatcher contract.
The previous for-select loop had two problems: (1) after the first \<-w.done fired, the closed channel kept firing and the loop re-called AcquireLock, busy-spinning until the timer.C fired. (2) Both <-w.done and <-timer.C become ready at timeout, and Go's select picks randomly — so the function could miss the timeout and loop indefinitely. Fix: replace the for-loop with a one-shot select. On the first signal (w.done or timer), try AcquireLock exactly once. If it succeeds, return nil; if the timer fired, return a timeout error. The race-loser no longer re-registers — it returns an error. TestWaitForLock_MultipleWaiters_OnlyOneAcquires updated to assert the new semantics: the race-loser gets an error rather than re-registering and waiting for a second signal.
…ection The previous implementation called logDroppedMessage while holding mb.mu (write lock). The slog.Warn call may acquire its own internal lock (the slog handler's mutex); if the handler is slow (file I/O, network sink), this serializes all bus operations for the duration of the log write. Fix: snapshot the (from, to, topic) of each drop into a local slice while still holding the lock, then iterate the slice after the lock is released and call logDroppedMessage on each entry. The error return path is also collected into a local variable and returned after the lock is released. No behavior change for the caller; only the order of operations differs (logs now happen after the bus is unlocked).
The previous 100 was a magic number inlined in the `n%100 != 0` check. Extracted as a package-level const `dropLogEveryN = 100` with a doc comment explaining the rationale (balancing observability against log volume under sustained pressure).
- TestWaitForLock_OwnerMismatchOnRelease renamed to TestReleaseLock_NonOwnerReturnsError; the original name claimed it was about waiters not being woken, but the body only checked the error return. - New TestWaitForLock_ReleaseByNonOwnerDoesNotWakeWaiter registers a real waiter, attempts a ReleaseLock from a non-owner, and asserts the waiter times out (rather than being woken by the failed ReleaseLock). This is the test the original name promised but never delivered.
… channel The previous TestStats_NotAffectedByWaiters launched a goroutine that called WaitForResponse and never joined it; the goroutine ran for 100ms after the test exited. Fixed by: - Adding a done channel that the goroutine closes on return. - Adding a 1-second join timeout via select on the done channel. - The test now fails loudly if the goroutine does not return within 1s (which would indicate a future regression).
…safe
The M2 refactor changed these methods to call
s.Persistence().DAG() and s.MemorySvc().Memory() directly. That
panics on &engine.Session{} struct literals (used by cmd tests)
where s.persist and s.memory are nil.
This commit wraps each call site in a nil-check on the parent
service (s.Persistence() / s.MemorySvc()) so that struct-literal
sessions still work the same way as before — the methods
become no-ops when their backing sub-service is nil, which
matches the previous behavior on s.ConvoDAG == nil and
s.Memory == nil.
Patel230
added a commit
that referenced
this pull request
Jun 18, 2026
* fix(cmd): M1 part 1 - pinSubcommand, GLM thinking, status use accessors
- chat_subcommand_pin.go: use m.session.SetPinnedMessages
- chat_subcommand_simple.go: use m.session.SetGLMThinkingEnabled
- chat_subcommand_status.go: use m.session.CostValue().Summary()
- session.go: add SetPinnedMessages accessor
* fix(cmd): M1 part 2 - Autonomy, ContainerExecutor, ConvoDAG accessors
- chat.go: use PermSvc().Autonomy()/SetAutonomy, SetContainerExecutor, SetContainerRequired
- permissions_center.go: use PermSvc().SetAutonomy
- welcome_gate.go, statusbar.go: use PermSvc().Autonomy()
- hud_panel.go: use MemorySvc().Yaad()
- chat_status.go: use ContextWindowCachedValue, Persistence().SetContextWindowCached
- chat_subcommand_branches.go: use Persistence().DAG()
- chat_commands_session.go: use Persistence().DAG()
- session.go: add SetContainerExecutor accessor
- audit_test.go: regex now matches m.session.X; cmdHardFailThreshold=0
* fix(cmd): M1 part 3 - remaining cmd/ Cost/Mode/ContextWindow accessors
- statusbar.go: use m.session.CostValue()
- chat_subcommand_cost.go, chat_subcommand_usage.go: use CostValue()
- chat_commands_session.go, chat_commands_util.go: use CostValue()
- chat.go: use ContextWindowCachedValue()
- permissions_center.go: use ModeValue() (safe fallback for test struct literal)
- chat_model_test.go, statusbar_test.go: use accessors
- session_sync.go, snapshot_cmd.go: avoid audit regex false positives in text
- session.go: add SetContextWindowCached, ModeValue, SetMode accessors
* fix(engine): M2 - AddUser/AddAssistant use MemorySvc() and Persistence().DAG()
- AddUser: read Memory from s.MemorySvc().Memory() (was s.Memory)
- AddUser, AddUserWithImage, AddAssistant, ForkConversation, SwitchBranch,
ListBranches, ConvoHead: read DAG from s.Persistence().DAG() (was s.ConvoDAG)
- Aligns with the H6 sub-service decomposition: the legacy ConvoDAG and
Memory fields on Session are aliases set by NewSessionWithClient; new
code goes through the sub-service getters to keep the
god-object-decomposition contract consistent.
* fix(engine): M3 - deprecation comments point at real accessors
- FewShot() -> FewShotStore() (typo fix in comment)
- PlanState, Trajectory, LintLoop, TestLoop, FileMentions, Files,
Snapshots, Tracer, RateLimiter: comments now indicate these
remain on the legacy Session struct (no sub-service accessor
exists); the previous deprecation text pointed to non-existent
methods that callers could not find.
* fix(engine): M4 - PermissionService constructor no longer pre-sets mode
NewPermissionService no longer initializes mode to PermissionModeDefault;
the zero value ("") is preserved so IsZero() can correctly report a
freshly constructed service. IsZero() now checks for the empty string
instead of PermissionModeDefault. This is the right semantics: callers
that want a default mode should call SetMode explicitly, and tests can
rely on IsZero() being true for a fresh service.
* fix(cmd): M5 - SubcommandRegistry.Register detects alias collision
The previous implementation checked for primary-name duplicates
(`r.primary[name]`) but not alias collisions. Two subcommands
that each register a different primary but share an alias would
silently overwrite `r.aliasOf[alias]`. Now the alias check
mirrors the primary-name check: registration is rejected if any
of the subcommand's aliases is already taken. The existing
silent-no-op pattern is preserved (no error return) so init()
callers don't have to change.
* fix(cmd): M6 - sessionSubcommand.Handle prepends command name to args
The previous Handle passed the post-name args slice directly to
handleSessionCommand, which expects parts[0] to be the command
name. This broke /recover <id>, /resume <id>, and /tag <label>:
the trailing arg landed at parts[0] instead of parts[1], and the
`len(parts) >= 2` check failed, hitting the usage error path.
The fix reconstructs the full parts slice (name + args) before
calling handleSessionCommand, matching the contract the
simple.go delegatingCommand handlers use. The parts-building
logic is extracted into a buildSessionParts helper for testability.
New tests (TestBuildSessionParts_*, TestSessionSubcommand_RecoverIDReachesPartsIndex1,
TestResolveSessionName_PicksLongestMatch) assert the fix and the
dispatcher contract.
* fix(multiagent): M7 - WaitForLock no longer busy-spins
The previous for-select loop had two problems: (1) after the first
\<-w.done fired, the closed channel kept firing and the loop
re-called AcquireLock, busy-spinning until the timer.C fired.
(2) Both <-w.done and <-timer.C become ready at timeout, and Go's
select picks randomly — so the function could miss the timeout
and loop indefinitely.
Fix: replace the for-loop with a one-shot select. On the first
signal (w.done or timer), try AcquireLock exactly once. If it
succeeds, return nil; if the timer fired, return a timeout error.
The race-loser no longer re-registers — it returns an error.
TestWaitForLock_MultipleWaiters_OnlyOneAcquires updated to assert
the new semantics: the race-loser gets an error rather than
re-registering and waiting for a second signal.
* fix(multiagent): M8 - logDroppedMessage moved out of mb.mu critical section
The previous implementation called logDroppedMessage while holding
mb.mu (write lock). The slog.Warn call may acquire its own internal
lock (the slog handler's mutex); if the handler is slow (file I/O,
network sink), this serializes all bus operations for the duration
of the log write.
Fix: snapshot the (from, to, topic) of each drop into a local
slice while still holding the lock, then iterate the slice after
the lock is released and call logDroppedMessage on each entry.
The error return path is also collected into a local variable
and returned after the lock is released.
No behavior change for the caller; only the order of operations
differs (logs now happen after the bus is unlocked).
* fix(multiagent): M9 - extract dropLogEveryN as a named constant
The previous 100 was a magic number inlined in the
`n%100 != 0` check. Extracted as a package-level const
`dropLogEveryN = 100` with a doc comment explaining the
rationale (balancing observability against log volume under
sustained pressure).
* test(multiagent): M10 - rename test + add real waiter-assertion test
- TestWaitForLock_OwnerMismatchOnRelease renamed to
TestReleaseLock_NonOwnerReturnsError; the original name claimed
it was about waiters not being woken, but the body only checked
the error return.
- New TestWaitForLock_ReleaseByNonOwnerDoesNotWakeWaiter registers
a real waiter, attempts a ReleaseLock from a non-owner, and
asserts the waiter times out (rather than being woken by the
failed ReleaseLock). This is the test the original name
promised but never delivered.
* test(multiagent): M11 - join the WaitForResponse goroutine via a done channel
The previous TestStats_NotAffectedByWaiters launched a goroutine
that called WaitForResponse and never joined it; the goroutine
ran for 100ms after the test exited. Fixed by:
- Adding a done channel that the goroutine closes on return.
- Adding a 1-second join timeout via select on the done channel.
- The test now fails loudly if the goroutine does not return
within 1s (which would indicate a future regression).
* fix(engine): M2 - make AddUser/AddAssistant/ForkConversation/etc nil-safe
The M2 refactor changed these methods to call
s.Persistence().DAG() and s.MemorySvc().Memory() directly. That
panics on &engine.Session{} struct literals (used by cmd tests)
where s.persist and s.memory are nil.
This commit wraps each call site in a nil-check on the parent
service (s.Persistence() / s.MemorySvc()) so that struct-literal
sessions still work the same way as before — the methods
become no-ops when their backing sub-service is nil, which
matches the previous behavior on s.ConvoDAG == nil and
s.Memory == nil.
Patel230
added a commit
that referenced
this pull request
Aug 15, 2026
- external/yaad -> fbc0466 (PRs #53-58: incremental HNSW, graph restore, backup rotation, backup scheduler, refcounted process lock) - go.mod pseudo-version bumped to match the gitlink (parity guard) - YaadBridge starts the hourly yaad backup scheduler once per process on the first ready bridge (~/.yaad/data/backups, keep 7, 30d)
Patel230
added a commit
that referenced
this pull request
Aug 15, 2026
- external/yaad -> fbc0466 (PRs #53-58: incremental HNSW, graph restore, backup rotation, backup scheduler, refcounted process lock) - go.mod pseudo-version bumped to match the gitlink (parity guard) - YaadBridge starts the hourly yaad backup scheduler once per process on the first ready bridge (~/.yaad/data/backups, keep 7, 30d)
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.
Summary
Addresses the 11 medium-severity issues from the prior code review (saved at /tmp/hawk-review.md).
cmd/sites + audit-test regex expansion + cmdHardFailThreshold→0Test plan
go test -race -count=1 -shuffle=on ./...passes (except the sandbox-blocked ones below)gofmtcleango vet ./...clean/recover <id>receives the idSandbox / environment notes
The following packages fail under the sandbox due to
mkdir .../.git/hooksor~/.hawk/backups/operation-not-permitted errors; these are not regressions from this PR and pass on a normal dev box:internal/context/repomapinternal/engine/gitinternal/engine/planninginternal/engine/projectinternal/engine/workflowinternal/intelligence/repomapinternal/multiagent/parallelinternal/rulesinternal/snapshotinternal/toolMade with Cursor