feat: add persistent session goals - #803
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the persistent-session-goals feature is correctly built, persists and reloads end to end, reuses the existing hardened metadata write path, and ships with good coverage across all three packages. No surviving correctness or security findings.
What I verified (holds up)
- Persistence is safe.
internal/sessions/goal.goreusesstore.writeMetadata(temp file + fsync + atomicRenameWithRetry+ parent-dir fsync, mode0o600in a0o700session dir). No bespoke file writing, no partial-write window — the right call over hand-rolling a new format. - Concurrency is safe. All goal mutations run under
lockSession(in-process mutex + cross-process file lock) and re-read metadata under the lock before rewriting, so concurrent appends can't clobber goal state.go test ./internal/sessions -race -count=1is clean, includingTestGoalLifecyclePersistsInSessionMetadataandTestGoalBudgetPausesAtLimit. - Input validation is present.
ValidSessionIDgate, objectiveTrimSpacenon-empty,tokenBudget >= 0(tool caps at 1e9), negative usage rejected.CreateGoalrefuses to overwrite an existing goal (TestCreateGoalRefusesImplicitReplacement); reaching budget pauses the goal (GoalStatusBudgetLimited) before another autonomous turn rather than looping — good guard against runaway continuation. - No injection sink. Goal text is never shell-interpolated. The objective flows into the model system/continuation prompt (
goalSystemPrompt) — expected and user-authored for this feature — and the TUI renders it as plainrowSystemtranscript text (no control-sequence or command sink). - Feature works end to end.
CreateGoal → writeMetadata → readMetadatareload is exercised byTestGoalLifecyclePersistsInSessionMetadata; TUI wiring covered byTestGoalCommandCreatesPersistentGoalAndStartsRun,TestActiveGoalLaunchesContinuation,TestGoalBudgetStopsAutomaticContinuation, andTestCancelRunPausesActiveGoal.
Build / tests
gofmt -l, go build, and go vet clean on all three changed packages. All goal-related tests pass, including the internal/sessions suite under -race. The failures under internal/tools (TestScopedToolsAllowReadOnlyRootsWithoutWrite, TestRegistryAppliesSandboxBeforeToolExecution, TestRegistrySandboxGatesPathAliasKeys) and internal/tui (TestHandleAddDirCommand) reproduce identically on the clean base worktree — they assert OS-level sandbox write-blocking that isn't enforceable when the worktree lives under /private/tmp. Environment/sandbox artifacts, not PR-attributable.
Clean, well-scoped feature. Reusing writeMetadata/lockSession and pausing at the budget instead of looping are both the right design choices.
Merge is kevin's call per the program gate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds persistent session goals with lifecycle events, session-bound agent tools, ChangesGoal lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TUIModel
participant GoalTools
participant Agent
participant Store
TUIModel->>GoalTools: Register session-bound goal tools
TUIModel->>Agent: Start goal-aware run
Agent-->>TUIModel: Return response and usage events
TUIModel->>Store: Persist usage and updated status
Store-->>TUIModel: Return goal metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
5012-5033: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExclude loop iterations from goal registry/prompt injection.
fireDueLoopIfIdlesetsm.activeLoopID, thenfireLoopPromptcallsrunAgentWithOptionswith the defaultspecDraft == false, so loop agents currently getgoalRegistry()/NewGoalToolsandgoalSystemPrompt. Loop work is separate from goals, so this turn needs aactiveLoopID/loop-specific flag in the options path that skips the goal-only injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 5012 - 5033, Update the runAgentWithOptions options path so loop-triggered turns identified by m.activeLoopID or an equivalent loop-specific flag skip goalRegistry/NewGoalTools registration and goalSystemPrompt injection. Preserve the existing goal behavior for normal non-specDraft turns and ensure fireLoopPrompt passes the loop-specific state when invoking the agent.
🧹 Nitpick comments (2)
internal/sessions/goal.go (1)
157-245: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo race-detector test for the documented concurrent scenario.
AddGoalUsage's andPauseGoalIfActive's doc comments explicitly describe the case they're built for: usage accounting racing with a cancellation-triggered pause. There's no test that actually runs these concurrently under-raceto prove the per-session lock prevents lost updates/overwrites.As per coding guidelines, "run affected concurrent code under the race detector" for
**/*_test.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/goal.go` around lines 157 - 245, The concurrent behavior of AddGoalUsage and PauseGoalIfActive is not covered by a race-detector test. Add a test that invokes both methods concurrently for the same session, runs safely under -race, and verifies serialized final metadata without lost updates or overwrites, including the expected goal status and token usage.Source: Coding guidelines
internal/tui/goal_test.go (1)
1-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for
handleGoalCommand's action-string dispatch (pause/resume/edit/clear), including its guard paths.The current tests exercise
createend-to-end and exerciseresume/cancel/budgetbehavior only via direct calls tolaunchGoalContinuationIfReady/reconcileGoalAfterRun/cancelRun, bypassinghandleGoalCommand("pause"|"resume"|"edit"|"clear")itself. That leaves the actual command routing and its guard error messages (no-goal, run-in-progress, budget-exhausted) untested — exactly the kind of gap that hid the missingm.pendingguard on thecreatebranch (flagged ininternal/tui/goal.go).As per coding guidelines, "
**/*_test.go: Keep tests beside their source files, add regression tests for behavior changes, and run affected concurrent code under the race detector."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/goal_test.go` around lines 1 - 201, The goal tests cover helper methods but not handleGoalCommand action-string dispatch or its guard paths. Add regression tests in TestGoalCommandCreatesPersistentGoalAndStartsRun’s vicinity that invoke handleGoalCommand with pause, resume, edit, and clear, validating their state changes and user-facing errors for no goal, an active run, and an exhausted budget; also assert the create action refuses to start when m.pending is already true.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sessions/goal.go`:
- Around line 105-153: Add regression coverage for the EditGoal method: verify a
normal edit updates the objective, budget, and active status; verify a reduced
budget at or below existing TokensUsed restores GoalStatusBudgetLimited with the
expected reason; and verify negative token budgets and empty objectives are
rejected without mutating the goal.
In `@internal/tools/goal.go`:
- Around line 62-66: Update the token_budget schema in the goal tool to declare
the same upper bound enforced by Run’s intArg call: set Maximum to 1_000_000_000
alongside the existing Minimum. Keep the documented zero-as-unlimited behavior
and align the schema constraint with the token_budget validation.
In `@internal/tui/goal.go`:
- Around line 267-318: The reconcileGoalAfterRun flow performs unnecessary
synchronous session reloads for non-goal sessions. In internal/tui/goal.go lines
267-318, update reconcileGoalAfterRun to return before the activeSession
assignment and ReadEvents call when loaded.Goal is nil, while preserving those
reloads for goal sessions; internal/tui/model.go lines 2409-2409 requires no
direct change because it is only the call site.
- Around line 88-106: Update the "create" branch in the goal command handler to
reject creation while m.pending is true, using the same paused-run guard and
error behavior as the /goal edit path. Apply the check before
ensureActiveSession, CreateGoal, or launchPrompt, while preserving the existing
creation flow when no run is in progress.
---
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 5012-5033: Update the runAgentWithOptions options path so
loop-triggered turns identified by m.activeLoopID or an equivalent loop-specific
flag skip goalRegistry/NewGoalTools registration and goalSystemPrompt injection.
Preserve the existing goal behavior for normal non-specDraft turns and ensure
fireLoopPrompt passes the loop-specific state when invoking the agent.
---
Nitpick comments:
In `@internal/sessions/goal.go`:
- Around line 157-245: The concurrent behavior of AddGoalUsage and
PauseGoalIfActive is not covered by a race-detector test. Add a test that
invokes both methods concurrently for the same session, runs safely under -race,
and verifies serialized final metadata without lost updates or overwrites,
including the expected goal status and token usage.
In `@internal/tui/goal_test.go`:
- Around line 1-201: The goal tests cover helper methods but not
handleGoalCommand action-string dispatch or its guard paths. Add regression
tests in TestGoalCommandCreatesPersistentGoalAndStartsRun’s vicinity that invoke
handleGoalCommand with pause, resume, edit, and clear, validating their state
changes and user-facing errors for no goal, an active run, and an exhausted
budget; also assert the create action refuses to start when m.pending is already
true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c57406bb-cd55-4644-9df4-08b6c62c5cce
📒 Files selected for processing (12)
internal/sessions/goal.gointernal/sessions/goal_test.gointernal/sessions/store.gointernal/tools/goal.gointernal/tools/goal_test.gointernal/tui/btw.gointernal/tui/commands.gointernal/tui/goal.gointernal/tui/goal_test.gointernal/tui/model.gointernal/tui/session.gointernal/tui/view.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/goal_test.go`:
- Around line 129-131: Update the request validation in the relevant goal test
to inspect every element of request.Messages for the persistent goal marker,
rather than checking only the first message. Fail the test with the offending
message content whenever any message contains the goal instructions, while
preserving the existing empty-message handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dbbb9e0-62ec-4acc-8889-c7430e08ed0c
📒 Files selected for processing (5)
internal/tools/goal.gointernal/tools/goal_test.gointernal/tui/goal.gointernal/tui/goal_test.gointernal/tui/model.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tools/goal_test.go
- internal/tools/goal.go
- internal/tui/goal.go
- internal/tui/model.go
|
@coderabbitai The only actionable finding from the latest review is marked addressed in commit f4ca329, the CodeRabbit check is successful, and all CI checks are green. Please re-review the current head and clear the stale changes-requested verdict if no actionable concerns remain. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== PR metadata and current commit =='
gh pr view 803 --repo Gitlawb/zero --json number,headRefOid,baseRefOid,commits,reviews,statusCheckRollup \
--jq '{number, headRefOid, baseRefOid, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], reviews: [.reviews[] | {author: .author.login, state: .state, submittedAt: .submittedAt}], checks: [.statusCheckRollup[] | {name: .name, status: .status, conclusion: .conclusion}]}'
printf '%s\n' '== Changed files and patch statistics =='
git diff --stat origin/HEAD...HEAD 2>/dev/null || git diff --stat HEAD~1..HEAD
git diff --name-only origin/HEAD...HEAD 2>/dev/null || git diff --name-only HEAD~1..HEAD
printf '%s\n' '== Commit f4ca3296 and current-head delta =='
git show --no-ext-diff --format=fuller --stat f4ca3296
git diff --no-ext-diff --stat f4ca3296..HEAD
git diff --no-ext-diff --find-renames --unified=50 f4ca3296..HEAD
printf '%s\n' '== Goal implementation/test structure =='
fd -t f -a '^(goal|model|store|session|view|btw)\.go$' internal | sort | while read -r f; do
echo "--- $f"
ast-grep outline "$f" --items all --type function,method,struct,interface --view compact || true
done
printf '%s\n' '== CodeRabbit review comments/threads on PR =='
gh api --paginate 'repos/Gitlawb/zero/pulls/803/comments?per_page=100' \
--jq '.[] | select(.user.login == "coderabbitai" or .user.login == "coderabbit[bot]") | {id, path, line, original_line, commit_id, body, created_at, updated_at}'Length of output: 7175 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='9d5c82d441b72d8931e59af510b5631ef357d921'
files=(
internal/sessions/goal.go
internal/sessions/store.go
internal/tools/goal.go
internal/tui/goal.go
internal/tui/model.go
internal/tui/btw.go
internal/tui/commands.go
internal/tui/session.go
internal/tui/view.go
internal/sessions/goal_test.go
internal/tools/goal_test.go
internal/tui/goal_test.go
)
printf '%s\n' '== Supported AST outlines for changed Go files =='
for f in "${files[@]}"; do
echo "--- $f"
ast-grep outline "$f" --items all --view digest || true
done
printf '%s\n' '== Full PR patch =='
git diff --no-ext-diff --find-renames --unified=30 "${base}..HEAD" -- "${files[@]}"
printf '%s\n' '== Goal-related call sites and lifecycle integration =='
rg -n -C 4 --glob '*.go' \
'\b(CreateGoal|UpdateGoal|EditGoal|AddGoalUsage|PauseGoalIfActive|ClearGoal|NewGoalTools|launchGoalContinuationIfReady|reconcileGoalAfterRun|goalSystemPrompt|goalRegistry|handleGoalCommand)\b' \
internal
printf '%s\n' '== Pull-request review-thread state =='
gh api graphql -F owner='Gitlawb' -F name='zero' -F number=803 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
path
line
comments(first: 20) {
nodes { author { login } body url }
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] |
select([.comments.nodes[].author.login] | any(. == "coderabbitai" or . == "coderabbit[bot]")) |
{isResolved, isOutdated, path, line, comments: [.comments.nodes[] | {author: .author.login, body, url}]}'Length of output: 176727
I can’t clear the changes-requested verdict yet because one actionable thread remains:
The remaining performance thread is stale: |
|
@coderabbitai All actionable findings are now addressed on current head b44189d, your latest review generated no actionable comments, your check is successful, and the final stale performance thread has been resolved after your confirmation that the fix is present. Please clear the stale changes-requested verdict. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Gitlawb/zero"
pr="803"
printf '%s\n' '== PR head, reviews, and checks =='
gh pr view "$pr" --repo "$repo" \
--json headRefOid,reviews,statusCheckRollup \
--jq '{
headRefOid,
reviews: [.reviews[] | {author: .author.login, state, submittedAt}],
checks: [.statusCheckRollup[] | {name, status, conclusion}]
}'
printf '%s\n' '== CodeRabbit review thread status =='
gh api graphql -F owner='Gitlawb' -F name='zero' -F number="$pr" -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
author { login }
body
}
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select([.comments.nodes[].author.login] | any(. == "coderabbitai" or . == "coderabbit[bot]"))
| {
isResolved,
isOutdated,
path,
line,
comments: [.comments.nodes[] | {author: .author.login, body}]
}'
printf '%s\n' '== Relevant goal implementation and tests =='
ast-grep outline internal/sessions/goal.go --items all --view compact
ast-grep outline internal/sessions/goal_test.go --items all --view compact
rg -n -C 8 --glob '*.go' \
'func Test.*EditGoal|func \(store \*Store\) EditGoal|budget_limited|negative token|empty objective' \
internal/sessions/goal.go internal/sessions/goal_test.goLength of output: 20438
[approve] |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE (re-review at b44189dc) — the three commits since my first pass are all hardening, and they close the gaps well. My earlier approval was auto-dismissed by the push; re-approving after verifying the delta.
What changed since 7f87eb1e, and how it holds up
de8b4bd — goal run isolation (the substantive fix). runAgentWithOptions now gates goal machinery on goalAwareRun := !runOptions.specDraft && m.activeLoopID == "" (model.go:5013), so during an active /loop run the goal tools and goalSystemPrompt are no longer injected. This is the right isolation — a loop run shouldn't see or mutate goal state. I mutation-checked it: reverting the m.activeLoopID == "" clause makes TestLoopRunExcludesGoalToolsAndInstructions fail, and that test asserts a loop request exposes neither goal tools nor goal instructions. Real test for a real fix.
de8b4bd — create-goal guard. handleGoalCommand "create" now returns "A run is already in progress." when m.pending (goal.go:89), preventing a goal from being created mid-run. Covered by TestGoalCommandDoesNotCreateGoalDuringActiveRun.
Token-budget maximum. create_goal now declares Maximum: 1_000_000_000 on the budget param (tools/goal.go:50), enforcing at the schema level the cap that was previously only applied in code — closes the unbounded-budget edge. Covered by TestCreateGoalToolDeclaresTokenBudgetMaximum.
reconcileGoalAfterRun refactor. The nested if loaded.Goal != nil { … } became an early-return guard clause (if loaded.Goal == nil { return m }). Verified logic-equivalent — the usage-accounting, error-pause, and m.activeSession / ReadEvents tail all run under identical conditions.
Everything from my first review still holds
Persistence reuses the hardened writeMetadata (temp + fsync + atomic rename, 0o600); all mutations under lockSession; goal text has no shell/injection sink; the feature works end to end. No new concerns.
Build / tests
gofmt, go build, go vet clean on all three changed packages. All goal tests pass, including internal/sessions under -race, plus the four new tests (TestLoopRunExcludesGoalToolsAndInstructions, TestGoalCommandDoesNotCreateGoalDuringActiveRun, TestCreateGoalToolDeclaresTokenBudgetMaximum, TestEditGoalUpdatesStateAndRejectsInvalidInputWithoutMutation). The internal/tools / internal/tui sandbox-write test failures reproduce identically on the clean base worktree (/private/tmp artifacts) — not PR-attributable.
Clean feature, well-hardened by the follow-ups.
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Nice feature and the shape is right: one goal per session, explicit lifecycle states, restore on resume, footer status. The TUI and store work read well and the test coverage is decent for the single-step behaviours.
The thing I cannot approve yet is the automatic continuation, because it has no bound.
1. [blocker] The continuation chain is unbounded.
launchGoalContinuationIfReady (internal/tui/goal.go) gates on goal == nil || goal.Status != GoalStatusActive || m.pending || m.compactInFlight || m.exiting || m.provider == nil. Every one of those is a transient "is the app busy right now" check. There is no counter, no minimum interval, no repetition or no-progress guard, and the persisted Goal struct has no continuation count either. It is called at the end of every completed run, so run N finishing immediately starts run N+1 with no delay. In practice the only things that stop it are the model volunteering that it is done, a run error, a manual cancel, or the optional token budget.
This is the same hazard the repo already solved once: the loop feature in internal/tui/loop.go ships explicit rails for exactly this. I would want goals to carry the equivalent, at minimum a max consecutive continuations counter persisted on the goal, and ideally a no-progress guard so a model that keeps producing the same output cannot spin.
2. [major] The token budget is opt-in and fails open.
TokenBudget defaults to unlimited, and reconcileGoalAfterRun only calls AddGoalUsage when the summed usage is greater than zero. Usage events only exist when the provider reported them, so against a provider that does not emit usage the budget never advances and the one available brake silently never engages. A turn-count fallback would make it robust regardless of provider behaviour.
3. [major] The model can arm the loop itself, with no confirmation.
create_goal declares SideEffect: SideEffectNone, Permission: PermissionAllow, so it never prompts, the objective is unbounded in length, and creating an active goal is what starts the chain. Combined with 1, that means a single model tool call can put the session into an unbounded self-launching state the user never explicitly asked for. Given what a goal actually does, I do not think this is a no-side-effect control tool: types.go defines SideEffectNone as a tool that neither reads nor mutates state, and this mutates persistent session state and schedules billed work. At minimum create_goal should prompt, or goal creation should be user-initiated only.
4. [major] Resuming a session immediately starts a billed run.
handleResumeCommand returns empty text on success, and both call sites treat empty as auto-continue, so resuming a session whose goal is still active starts an agent run with no confirmation. active is a perfectly normal on-disk state after a crash or an idle exit, so this can surprise someone who just wanted to look at an old session. I would gate the post-resume continuation behind an explicit confirmation or an explicit /goal resume.
5. [major] Goals keep running inside BTW conversations.
The PR says goal execution stays separate from BTW, and the /goal command is indeed blocked there, but the hidden parent session's response still routes through the done handler that launches the continuation, so continuations keep firing on the main session while the user is inside a BTW side conversation. Either block it there too or drop the claim.
6. [minor] A couple of smaller ones. The next.activeLoopID == "" exclusion is dead code because activeLoopID is cleared earlier in the same handler, so loop-iteration tokens are charged to the goal budget. And splitGoalCommand matches only the first word against pause/resume/clear/edit and discards the rest, so a /goal clear ... style typo silently drops content.
Test gap: no test exercises more than one continuation, which is why none of the above shows up in CI. A test that asserts the chain stops after N would be the one to add alongside the fix.
Verification note: Smart App Control on my machine is currently blocking the unsigned go toolchain, so this is a source review; I did not run the suite locally. CI is green.
To be clear, none of this is a problem with the idea, and most of it is bounded work. The unbounded chain is the one I would insist on, because a self-launching agent that bills tokens needs a hard stop that does not depend on the model cooperating. Happy to re-review quickly.
Persist a provider-independent continuation limit, require approval for model-created goals, and keep resume, BTW, and loop runs from starting or charging unrelated goal work. Also reject trailing lifecycle arguments and bound persisted objective length.
c6f77b4
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/sessions/goal.go (1)
105-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContinuation-limit fallback duplicated across four mutators.
The
if ContinuationLimit <= 0 { ... = GoalMaxConsecutiveContinuations }pattern is repeated verbatim inUpdateGoal(Lines 105-110),EditGoal(Lines 160-163),ResetGoalContinuations(Lines 205-213), andReserveGoalContinuation(Lines 244-248), with slightly different surrounding logic each time. A future change to this safety cap (e.g. adjusting the default or adding a floor check) risks being applied inconsistently across the four call sites.Consider extracting a small helper, e.g.:
♻️ Suggested helper
// ensureContinuationLimit sets goal.ContinuationLimit to the default when unset // and reports whether it changed. func ensureContinuationLimit(goal *Goal) bool { if goal.ContinuationLimit > 0 { return false } goal.ContinuationLimit = GoalMaxConsecutiveContinuations return true }Then each call site replaces its inline
if ContinuationLimit <= 0 {...}block withensureContinuationLimit(session.Goal).Also applies to: 160-163, 205-213, 244-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/goal.go` around lines 105 - 110, The continuation-limit fallback is duplicated across multiple goal mutators and should be centralized. Add an ensureContinuationLimit helper near the goal mutation logic that updates unset or non-positive limits to GoalMaxConsecutiveContinuations and reports whether it changed, then replace the inline fallback blocks in UpdateGoal, EditGoal, ResetGoalContinuations, and ReserveGoalContinuation with calls to that helper while preserving each method’s surrounding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sessions/goal.go`:
- Around line 186-222: Add direct sessions-layer tests for
ResetGoalContinuations covering no goal, inactive goal, already-reset active
goal without persistence, and active goal reset with persistence. Follow the
existing EditGoal test patterns in goal_test.go, asserting returned metadata,
reset count/limit and updated timestamp where applicable, and verifying the
no-op path does not write metadata.
In `@internal/tui/btw.go`:
- Line 179: The BTW exit path must resume deferred goal continuations after
clearing goalContinuationsSuspended. In internal/tui/btw.go:179, invoke
launchGoalContinuationIfReady and batch its command with the existing return
commands; in internal/tui/btw_test.go:138-166, add a regression assertion that
leaving BTW after parent completion starts exactly one continuation.
---
Nitpick comments:
In `@internal/sessions/goal.go`:
- Around line 105-110: The continuation-limit fallback is duplicated across
multiple goal mutators and should be centralized. Add an ensureContinuationLimit
helper near the goal mutation logic that updates unset or non-positive limits to
GoalMaxConsecutiveContinuations and reports whether it changed, then replace the
inline fallback blocks in UpdateGoal, EditGoal, ResetGoalContinuations, and
ReserveGoalContinuation with calls to that helper while preserving each method’s
surrounding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13e12b06-5413-411f-8b60-ad66c71cb904
📒 Files selected for processing (13)
internal/sessions/goal.gointernal/sessions/goal_test.gointernal/sessions/store.gointernal/tools/goal.gointernal/tools/goal_test.gointernal/tools/types.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/goal.gointernal/tui/goal_test.gointernal/tui/model.gointernal/tui/session.gointernal/tui/session_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/tools/goal_test.go
- internal/sessions/store.go
- internal/tui/model.go
- internal/tools/goal.go
- internal/tui/goal.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at e4e4fa20c1b8, base ac50a5a840d2, confirmed against the live head before posting.
To be clear about what this does and does not do: it does not clear @Vasanthdev2004's outstanding request for changes, and it should not. He raised the blocker, the fix answers it, and whether it satisfies him is his call rather than mine. This approval records that I verified the fix independently.
The blocker is genuinely fixed. ReserveGoalContinuation (internal/sessions/goal.go:227) atomically reserves one automatic continuation under lockSession, incrementing a persisted ContinuationCount against a persisted ContinuationLimit defaulting to 20. When the allowance is exhausted it flips the goal to paused, records automatic continuation limit reached, appends an event, and the TUI surfaces "Goal paused after N automatic continuations. Review progress, then use /goal resume to continue." The reservation happens before the continuation prompt is built (internal/tui/goal.go:268), so no provider request can start unreserved.
The property that makes this a real bound rather than a decorative one is the reset path. ResetGoalContinuations is called from exactly one place, internal/tui/model.go:4758, inside the user-prompt submission path and guarded by m.activeLoopID == "". The model cannot reset its own allowance; only a human message does. A chain therefore stops after at most 20 consecutive runs regardless of what the model reports.
I mutation-checked both halves rather than reading them. Replacing the limit comparison with an unconditional reserve fails TestGoalContinuationLimitStopsWithoutProviderUsage and TestGoalContinuationChainStopsAtPersistedLimit. Removing the msg.goalAware gate in the completion handler fails TestLoopRunDoesNotConsumeGoalBudgetOrLaunchContinuation. The tests bind to the behaviour they claim to cover.
On the rest of @Vasanthdev2004's list. The token-budget concern is answered by a better mechanism than was asked for: the continuation counter does not depend on the provider reporting usage, so the brake engages against a provider that emits none, which is precisely what TestGoalContinuationLimitStopsWithoutProviderUsage exercises. create_goal now declares SideEffectWrite and PermissionPrompt with a reason naming the automatic follow-up runs, and the objective is capped at 4,000 runes by validateGoalObjective. The resume path no longer launches anything: the only four continuation triggers are BTW close, the two explicit /goal commands, and end of run. BTW is handled by goalContinuationsSuspended, covered by TestBTWHiddenParentDoesNotLaunchGoalContinuation. The dead-code point is resolved by capturing goalAware at run start and gating the completion handler on it rather than re-reading activeLoopID after it has been cleared.
One disagreement, with evidence. The outstanding CodeRabbit finding says loop iterations still receive goalRegistry/NewGoalTools and goalSystemPrompt at internal/tui/model.go:5012-5033. That does not hold on this head. fireDueLoopIfIdle sets m.activeLoopID = due.id at internal/tui/loop.go:353 before the run is dispatched, so the m.activeLoopID == "" clause in goalAwareRun at line 5022 evaluates false and neither the registry nor the prompt is injected. TestLoopRunExcludesGoalToolsAndInstructions asserts exactly this, and reverting the clause makes it fail. I would not hold the pull request for that comment.
Verification. On macOS (darwin/arm64), make fmt-check, go vet ./... and git diff --check are clean, and the full suite passes at 82 packages with no failures. go test -race -count=5 ./internal/sessions and -race -count=3 over the goal, loop and BTW tests in internal/tui are clean, which matters here because the reservation is a read-modify-write against shared session metadata.
Limitations. macOS only, and I did not drive a manual TUI session. The interactive flows rest on the test coverage and on reading the code, so if anything in the paused-and-resumed user experience reads awkwardly in practice, that is the gap in my review.
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed on e4e4fa2. All six are addressed, and the blocker is fixed the way I hoped rather than the cheap way. Clearing my stale block.
1, the blocker. ContinuationCount and ContinuationLimit are persisted on the goal, ReserveGoalContinuation reserves atomically and flips the goal to paused once the allowance is spent, and ResetGoalContinuations refills it only on explicit user input. That is the important detail: the bound is consecutive rather than lifetime, so a user who keeps engaging is never throttled, while a model left alone cannot spin. Nothing in the path depends on the model volunteering that it is done.
2. Answered by the same mechanism rather than by patching the budget: the counter advances regardless of whether the provider reports usage, so the brake no longer fails open on providers that emit no usage events. TestGoalContinuationLimitStopsWithoutProviderUsage is exactly the case I was worried about.
3. create_goal is now SideEffectWrite with PermissionPrompt, so the model can no longer arm a self-launching state silently.
4. Resuming an active goal now surfaces (run /goal resume to continue) instead of starting a billed run, so opening an old session is inspection rather than execution.
5. goalContinuationsSuspended gates the parent while a BTW conversation is open and continuations resume when it closes, so the separation the description claims is now real.
6. splitGoalCommand returns the remainder now. The activeLoopID == "" conditions are still there; I would rather leave them than churn the file, since I flagged them as minor and they are harmless either way.
Worth calling out that you added the test I asked for rather than only the fix: TestGoalContinuationChainStopsAtPersistedLimit drives the chain to the limit, which is precisely the coverage whose absence let all of this through CI the first time.
Verification, and its limits. I read the reservation path closely and ran the internal/sessions goal suite, which passes. I tried to mutation-check the bound by removing the count comparison, to confirm the tests fail without it, and could not: Smart App Control on this machine started refusing freshly built test binaries partway through and blocks that specific run. So the bound is verified by reading and by the tests existing and passing, not by watching them fail without it. Flagging that rather than implying more than I did.
Good iteration on this. The shape is right and the rails are now real.
3 participants
Summary
Verification
The advisory lint command reports the existing repository backlog; it reported no findings in the changed goal files.
Fixes #797
Summary by CodeRabbit
get_goal,create_goal,update_goal) and a/goalworkflow (view/create/update with pause/resume/clear/edit).