feat(goal): add session-scoped /goal continuation - #1293
Conversation
|
This is quite the addition as well as a change in the feature set of openclaude. We will want to make sure to review this thoroughly |
71a1e72 to
4bcb127
Compare
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Pause instead of auto-continuing when the goal evaluator fails
src/services/goal/evaluator.ts:269
When the evaluator throws, or when the provider keeps returning malformed JSON,evaluateGoalconverts that infrastructure failure into anincompletedecision with a continuation instruction. The controller then treats it the same as a real "goal not done" result and injects another hidden user message, so a transient auth/provider/schema-output failure can make the main agent keep running turn after turn until the goal's default 50-turn cap. That can burn API calls and potentially execute more tools after the actual task is already complete. Please fail closed here, for example by pausing the goal or stopping continuation after evaluator errors/malformed responses instead of synthesizing a new continuation prompt. -
[P2] Do not mark goal continuations as active Stop-hook recursion
src/query/stopHooks.ts:502
Goal continuation messages are returned throughblockingErrors, and the shared query loop treats every blocking-error continuation as a Stop-hook continuation by settingstopHookActive: truefor the next model turn. That flag is then exposed to configured Stop hooks asstop_hook_active, even though the next turn was caused by/goal, not by a Stop hook asking the agent to continue. Any user hook that skips work or changes behavior whenstop_hook_activeis true will now mis-handle goal-driven turns. Please carry goal continuation through a separate transition/return field, or otherwise avoid setting the Stop-hook recursion flag for continuations that were generated by the goal evaluator.
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates here. I re-reviewed the current head, and the two issues I previously raised look addressed to me: evaluator failures now pause/fail closed instead of continuing automatically, and goal-driven continuations no longer mark the next turn as active Stop-hook recursion.
I do not have additional code-level findings from this pass.
That said, I still think this PR depends on a maintainer/product decision before merge. /goal is a meaningful expansion of OpenClaude's feature set because it adds session-scoped autonomous continuation behavior, persistent goal state, and an evaluator-driven loop after normal turns. The implementation looks much safer after the fixes, but I would still treat the remaining question as whether maintainers want this workflow and UX in the product.
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed current head for the /goal continuation feature.
Finding:
- [P2] Resuming a session with no saved goal can leave a stale active goal from the previous in-memory session.
restoreSessionStateFromLogonly writesgoalwhenresult.goal !== undefined, so a resumed log with no goal metadata leavesprev.goaluntouched. This can make/goalcontinuation run in the wrong conversation after resuming a normal session from one that had an active goal. The restore path should explicitly cleargoalfor resumed sessions with no goal metadata, or otherwise make the absence of a goal unambiguous.
Local validation:
- Focused goal/session tests passed:
npm exec --yes bun@1.3.13 -- test src/commands/goal/goal.test.ts src/services/goal/controller.test.ts src/services/goal/evaluator.test.ts src/services/goal/state.test.ts src/query/goalContinuation.test.ts src/query/stopHooks.goal.test.ts src/queryEngine.goal.test.ts src/commands/clear/conversation.goal.test.ts src/utils/processUserInput/processSlashCommand.goal.test.tsx src/utils/sessionRestore.goal.test.ts src/utils/conversationRecovery.test.ts src/utils/sessionStorage.test.ts=> 59 pass. - Reproduced the stale-goal edge with a direct local import: calling
restoreSessionStateFromLog({ messages: [] }, ...)against state containingcreateGoalState("old goal")leftstate.goal.conditionasold goal.
Recommendation: request changes.
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the latest updates here. I re-reviewed the current head more and the earlier evaluator-failure, Stop-hook recursion, and stale-goal resume concerns do look addressed. I found two remaining issues in the current implementation:
Findings
-
[P1] Persist the hidden
/goalcontinuation prompt
src/query/stopHooks.ts:528
Goal continuations are returned fromevaluateGoalAfterTurn()asblockingErrors, but unlike real Stop-hook blocking messages they are never yielded beforehandleStopHooks()returns them to the query loop. The next model request still receives that hidden user message internally, but the outer transcript/SDK layer never sees or records it, so a resumed/replayed session can contain an assistant turn that was produced by a goal continuation prompt that is missing from the persisted conversation. That undermines the PR's resume/persistence story for any incomplete goal that auto-continues. Please yield the goal continuation user message the same way Stop-hook blocking messages are yielded, or otherwise make sure it is recorded in the transcript before the follow-up assistant turn. -
[P2] Do not spend an extra evaluator call after the SDK budget is already exhausted
src/services/goal/controller.ts:104
The paid goal evaluator runs insidehandleStopHooks()beforeQueryEnginegets a chance to enforcemaxBudgetUsdon the just-finished main response. If the main model turn reaches or exceeds the caller's budget while a goal is active, OpenClaude still makes the additional evaluator request and only reportserror_max_budget_usdafter that extra spend has already happened. Please check the budget before running the goal evaluator, or thread a budget/cost guard into the goal evaluation path so/goalcannot exceed the user-specified cap just to decide whether to continue.
|
Updated the branch with a scoped fix for both review findings. What changed:
Tests added/strengthened:
Local validation:
Full-suite note:
|
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the latest update. I rechecked the current head, including the earlier evaluator-failure, Stop-hook recursion, stale-goal resume, hidden continuation persistence, and exhausted-budget paths. The current implementation addresses those code-level issues, and I do not see additional actionable code findings from this pass.
I do still think this needs an explicit maintainer/product decision before merge. /goal adds session-scoped autonomous continuation behavior, persisted goal state, and an evaluator-driven follow-up loop after normal turns. The implementation now looks much safer, but the remaining question is whether that workflow and UX belong in OpenClaude's core feature set.
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the latest update. I rechecked the previously discussed evaluator-failure, Stop-hook recursion, stale AppState resume, hidden continuation persistence, and exhausted-budget paths. Those code-level issues look addressed now, but I found one remaining resume/persistence issue in the current head.
Findings
- [P2] Clear the cached goal metadata when resuming sessions without a goal
src/utils/sessionStorage.ts:3080
restoreSessionMetadata()only updatescurrentSessionGoalwhenmeta.goal !== undefined, so a running process that already has an active goal cached can resume a transcript that has nogoal-stateentry without clearing that cache. On the non-fork resume path,processResumedConversation()callsresetSessionFilePointer(), thenrestoreSessionMetadata(), and thenadoptResumedSessionFile()re-appends cached metadata; becausereAppendSessionMetadata()writes any cached active/paused goal back to the adopted transcript, the old goal can be persisted into an unrelated resumed session even though its AppState was correctly reset tonull. Please make the absence of goal metadata clear the session metadata cache as well, or otherwise prevent stalecurrentSessionGoalfrom being re-appended during resume.
I also still think this PR depends on an explicit maintainer/product decision before merge. /goal adds session-scoped autonomous continuation behavior, persisted goal state, and an evaluator-driven follow-up loop after normal turns, so the remaining product question is whether that workflow and UX belong in OpenClaude's core feature set.
|
Updated the branch with a scoped fix for the remaining resume/persistence finding. What changed:
Regression coverage:
Local validation:
Remote PR checks on commit The product/maintainer decision point for |
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the latest update. I rechecked the previously discussed evaluator-failure, Stop-hook recursion, stale goal resume, hidden continuation persistence, exhausted-budget, and cached metadata paths. Those code-level issues look addressed now, but I found one remaining issue in the current head.
Findings
- [P2] Keep the new resume test type-clean
src/utils/sessionRestore.goal.test.ts:78
The newrestoreSessionStateFromLog clears stale in-memory goal when resumed session has nonetest adds a TypeScript error under the repo's ownbun run typecheckscript:stateis inferred from the initial object literal withgoal: staleGoal, so assigningupdate(state)fails because the updater returnsAppStatewheregoalmay benull. The full typecheck is already noisy, but this is an added test file and makes the typecheck debt worse for this PR. Please type the local variable asAppStateor otherwise widen it before passing it through the updater.
I also still think this PR depends on an explicit maintainer/product decision before merge. /goal adds session-scoped autonomous continuation behavior, persisted goal state, and an evaluator-driven follow-up loop after normal turns, so the remaining product question is whether that workflow and UX belong in OpenClaude's core feature set.
675a54a to
1241272
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/commands/clear/conversation.goal.test.ts`:
- Around line 9-36: The test calls clearConversation which regenerates the
global session id but the test only restores env vars; capture the current
session id before invoking clearConversation (e.g. const previousSessionId =
global.sessionId) and restore it in the finally block alongside
process.env.CLAUDE_CODE_SIMPLE (set global.sessionId = previousSessionId) so
subsequent tests don't inherit mutated session state; keep using the same
identifiers (previousBareMode, clearConversation,
process.env.CLAUDE_CODE_SIMPLE) and restore the saved session id in the existing
finally.
In `@src/services/goal/controller.ts`:
- Around line 64-68: The empty catch around saveGoalState(goal) should not
swallow errors silently; update the catch to accept the thrown error (e) and
emit a non-fatal warning/telemetry event including unique identifiers (e.g.
goal.id and current session or turn id) and the error details so persistence
failures are diagnosable, but do not rethrow (preserve the non-fatal behavior).
Locate the try/catch around saveGoalState in controller.ts and call your
existing logger/telemetry helper (or add one) to log a warning like "Goal
persistence failed" with goal identifiers and error information.
In `@src/utils/conversationRecovery.hooks.test.ts`:
- Around line 60-62: Duplicate deletions of the same environment variables are
present; remove the redundant block so each of process.env.OPENAI_BASE_URL,
process.env.OPENAI_API_BASE, and process.env.OPENAI_MODEL is deleted exactly
once. Locate the repeated lines deleting process.env.OPENAI_BASE_URL /
process.env.OPENAI_API_BASE / process.env.OPENAI_MODEL in the
conversationRecovery.hooks test and delete the second occurrence, leaving the
original cleanup intact.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 56227355-cd38-4285-b3a9-6c07206d7066
📒 Files selected for processing (42)
src/QueryEngine.tssrc/commands.tssrc/commands/clear/conversation.goal.test.tssrc/commands/clear/conversation.tssrc/commands/goal/goal.test.tssrc/commands/goal/goal.tssrc/commands/goal/index.tssrc/query.tssrc/query/deps.tssrc/query/goalContinuation.test.tssrc/query/stopHooks.goal.test.tssrc/query/stopHooks.tssrc/queryEngine.goal.test.tssrc/services/api/xaiOAuthCallback.test.tssrc/services/goal/controller.test.tssrc/services/goal/controller.tssrc/services/goal/evaluator.test.tssrc/services/goal/evaluator.tssrc/services/goal/instructions.tssrc/services/goal/persistence.tssrc/services/goal/sdk.tssrc/services/goal/state.test.tssrc/services/goal/state.tssrc/services/goal/status.tssrc/services/goal/types.tssrc/state/AppStateStore.tssrc/test/fixtures/queryEngineGoalStatus.fixture.tssrc/tools/AgentTool/AgentTool.teammateModel.test.tssrc/types/command.tssrc/types/logs.tssrc/utils/apiPreconnect.test.tssrc/utils/attribution.test.tssrc/utils/conversationRecovery.hooks.test.tssrc/utils/conversationRecovery.test.tssrc/utils/conversationRecovery.tssrc/utils/fastMode.test.tssrc/utils/processUserInput/processSlashCommand.goal.test.tsxsrc/utils/processUserInput/processSlashCommand.tsxsrc/utils/sessionRestore.goal.test.tssrc/utils/sessionRestore.tssrc/utils/sessionStorage.test.tssrc/utils/sessionStorage.ts
💤 Files with no reviewable changes (1)
- src/tools/AgentTool/AgentTool.teammateModel.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/commands/clear/conversation.tssrc/types/logs.tssrc/services/goal/sdk.tssrc/services/goal/persistence.tssrc/utils/fastMode.test.tssrc/utils/processUserInput/processSlashCommand.goal.test.tsxsrc/services/goal/status.tssrc/commands/clear/conversation.goal.test.tssrc/state/AppStateStore.tssrc/utils/conversationRecovery.hooks.test.tssrc/commands/goal/index.tssrc/services/goal/state.test.tssrc/services/goal/types.tssrc/utils/attribution.test.tssrc/utils/sessionRestore.goal.test.tssrc/query.tssrc/queryEngine.goal.test.tssrc/QueryEngine.tssrc/utils/processUserInput/processSlashCommand.tsxsrc/utils/sessionRestore.tssrc/query/deps.tssrc/types/command.tssrc/test/fixtures/queryEngineGoalStatus.fixture.tssrc/commands.tssrc/utils/apiPreconnect.test.tssrc/utils/conversationRecovery.test.tssrc/utils/sessionStorage.test.tssrc/services/goal/instructions.tssrc/query/stopHooks.goal.test.tssrc/services/goal/controller.test.tssrc/services/goal/controller.tssrc/query/goalContinuation.test.tssrc/services/goal/evaluator.tssrc/utils/conversationRecovery.tssrc/commands/goal/goal.tssrc/services/goal/state.tssrc/commands/goal/goal.test.tssrc/query/stopHooks.tssrc/services/api/xaiOAuthCallback.test.tssrc/services/goal/evaluator.test.tssrc/utils/sessionStorage.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/commands/clear/conversation.tssrc/types/logs.tssrc/services/goal/sdk.tssrc/services/goal/persistence.tssrc/utils/fastMode.test.tssrc/utils/processUserInput/processSlashCommand.goal.test.tsxsrc/services/goal/status.tssrc/commands/clear/conversation.goal.test.tssrc/state/AppStateStore.tssrc/utils/conversationRecovery.hooks.test.tssrc/commands/goal/index.tssrc/services/goal/state.test.tssrc/services/goal/types.tssrc/utils/attribution.test.tssrc/utils/sessionRestore.goal.test.tssrc/query.tssrc/queryEngine.goal.test.tssrc/QueryEngine.tssrc/utils/processUserInput/processSlashCommand.tsxsrc/utils/sessionRestore.tssrc/query/deps.tssrc/types/command.tssrc/test/fixtures/queryEngineGoalStatus.fixture.tssrc/commands.tssrc/utils/apiPreconnect.test.tssrc/utils/conversationRecovery.test.tssrc/utils/sessionStorage.test.tssrc/services/goal/instructions.tssrc/query/stopHooks.goal.test.tssrc/services/goal/controller.test.tssrc/services/goal/controller.tssrc/query/goalContinuation.test.tssrc/services/goal/evaluator.tssrc/utils/conversationRecovery.tssrc/commands/goal/goal.tssrc/services/goal/state.tssrc/commands/goal/goal.test.tssrc/query/stopHooks.tssrc/services/api/xaiOAuthCallback.test.tssrc/services/goal/evaluator.test.tssrc/utils/sessionStorage.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/commands/clear/conversation.tssrc/types/logs.tssrc/services/goal/sdk.tssrc/services/goal/persistence.tssrc/utils/fastMode.test.tssrc/utils/processUserInput/processSlashCommand.goal.test.tsxsrc/services/goal/status.tssrc/commands/clear/conversation.goal.test.tssrc/state/AppStateStore.tssrc/utils/conversationRecovery.hooks.test.tssrc/commands/goal/index.tssrc/services/goal/state.test.tssrc/services/goal/types.tssrc/utils/attribution.test.tssrc/utils/sessionRestore.goal.test.tssrc/query.tssrc/queryEngine.goal.test.tssrc/QueryEngine.tssrc/utils/processUserInput/processSlashCommand.tsxsrc/utils/sessionRestore.tssrc/query/deps.tssrc/types/command.tssrc/test/fixtures/queryEngineGoalStatus.fixture.tssrc/commands.tssrc/utils/apiPreconnect.test.tssrc/utils/conversationRecovery.test.tssrc/utils/sessionStorage.test.tssrc/services/goal/instructions.tssrc/query/stopHooks.goal.test.tssrc/services/goal/controller.test.tssrc/services/goal/controller.tssrc/query/goalContinuation.test.tssrc/services/goal/evaluator.tssrc/utils/conversationRecovery.tssrc/commands/goal/goal.tssrc/services/goal/state.tssrc/commands/goal/goal.test.tssrc/query/stopHooks.tssrc/services/api/xaiOAuthCallback.test.tssrc/services/goal/evaluator.test.tssrc/utils/sessionStorage.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/fastMode.test.tssrc/utils/processUserInput/processSlashCommand.goal.test.tsxsrc/commands/clear/conversation.goal.test.tssrc/utils/conversationRecovery.hooks.test.tssrc/services/goal/state.test.tssrc/utils/attribution.test.tssrc/utils/sessionRestore.goal.test.tssrc/queryEngine.goal.test.tssrc/utils/apiPreconnect.test.tssrc/utils/conversationRecovery.test.tssrc/utils/sessionStorage.test.tssrc/query/stopHooks.goal.test.tssrc/services/goal/controller.test.tssrc/query/goalContinuation.test.tssrc/commands/goal/goal.test.tssrc/services/api/xaiOAuthCallback.test.tssrc/services/goal/evaluator.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/fastMode.test.tssrc/utils/processUserInput/processSlashCommand.goal.test.tsxsrc/commands/clear/conversation.goal.test.tssrc/utils/conversationRecovery.hooks.test.tssrc/services/goal/state.test.tssrc/utils/attribution.test.tssrc/utils/sessionRestore.goal.test.tssrc/queryEngine.goal.test.tssrc/utils/apiPreconnect.test.tssrc/utils/conversationRecovery.test.tssrc/utils/sessionStorage.test.tssrc/query/stopHooks.goal.test.tssrc/services/goal/controller.test.tssrc/query/goalContinuation.test.tssrc/commands/goal/goal.test.tssrc/services/api/xaiOAuthCallback.test.tssrc/services/goal/evaluator.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/xaiOAuthCallback.test.ts
🔇 Additional comments (41)
src/query/deps.ts (1)
5-6: LGTM!Also applies to: 35-36
src/query.ts (1)
202-209: LGTM!Also applies to: 245-393, 881-881, 1018-1020, 1321-1454, 1472-1520, 1596-1596, 1633-1665, 1701-1701, 1765-1765, 2215-2215
src/query/stopHooks.ts (1)
53-75: LGTM!Also applies to: 86-106, 204-214, 283-287, 317-321, 354-367, 371-371, 389-399, 413-416, 445-451, 481-503, 506-544, 560-564
src/query/goalContinuation.test.ts (1)
57-168: LGTM!Also applies to: 170-244
src/query/stopHooks.goal.test.ts (1)
55-99: LGTM!Also applies to: 101-146, 148-187, 189-232
src/services/goal/sdk.ts (1)
5-8: LGTM!src/QueryEngine.ts (1)
35-38: LGTM!Also applies to: 174-175, 685-686, 997-998
src/queryEngine.goal.test.ts (1)
7-83: LGTM!src/test/fixtures/queryEngineGoalStatus.fixture.ts (1)
12-101: LGTM!src/services/api/xaiOAuthCallback.test.ts (1)
49-94: LGTM!src/utils/apiPreconnect.test.ts (2)
37-37: LGTM!
105-108: LGTM!src/utils/attribution.test.ts (2)
30-68: LGTM!
138-145: LGTM!src/utils/fastMode.test.ts (1)
231-235: LGTM!src/services/goal/types.ts (1)
1-32: LGTM!src/state/AppStateStore.ts (1)
41-41: LGTM!Also applies to: 428-430, 569-569
src/services/goal/status.ts (1)
1-18: LGTM!src/services/goal/evaluator.ts (1)
1-279: LGTM!src/services/goal/instructions.ts (1)
1-31: LGTM!src/services/goal/state.ts (1)
1-181: LGTM!src/services/goal/state.test.ts (1)
1-137: LGTM!src/services/goal/evaluator.test.ts (1)
1-194: LGTM!src/services/goal/persistence.ts (1)
1-15: LGTM!src/services/goal/controller.ts (1)
1-63: LGTM!Also applies to: 71-193
src/services/goal/controller.test.ts (1)
1-486: LGTM!src/commands/goal/index.ts (1)
3-12: LGTM!src/commands.ts (1)
7-7: LGTM!Also applies to: 329-329, 655-656, 683-684
src/types/command.ts (1)
17-38: LGTM!src/utils/processUserInput/processSlashCommand.tsx (1)
673-680: LGTM!Also applies to: 706-713, 722-747
src/utils/processUserInput/processSlashCommand.goal.test.tsx (1)
25-49: LGTM!src/commands/goal/goal.test.ts (1)
36-213: LGTM!src/commands/goal/goal.ts (1)
72-135: LGTM!src/utils/conversationRecovery.test.ts (1)
58-86: LGTM!Also applies to: 151-191
src/commands/clear/conversation.ts (1)
38-43: LGTM!Also applies to: 199-205
src/types/logs.ts (1)
7-7: LGTM!Also applies to: 54-55, 190-195, 324-325
src/utils/sessionStorage.ts (1)
101-109: LGTM!Also applies to: 554-555, 846-857, 1146-1158, 1240-1242, 1535-1540, 1822-1823, 2602-2649, 3278-3343, 3425-3426, 3795-3820, 3919-3921, 3985-3987, 4123-4124, 4143-4144, 4199-4242, 4935-5010, 5193-5194
src/utils/sessionStorage.test.ts (1)
3-21: LGTM!Also applies to: 124-155, 363-532
src/utils/conversationRecovery.ts (1)
519-549: LGTM!Also applies to: 589-710
src/utils/sessionRestore.ts (1)
55-56: LGTM!Also applies to: 72-72, 154-155, 321-321, 555-555
src/utils/sessionRestore.goal.test.ts (1)
1-151: LGTM!
|
@chioarub kindly check coderabbit comments |
…on (Twigpine#1293) Full port of upstream PR 1293 (atomic 22 sub-commits). New files (20): - src/services/goal/types.ts, state.ts, controller.ts, persistence.ts, sdk.ts, status.ts, instructions.ts, evaluator.ts - src/commands/goal/goal.ts + index.ts (subdirectory pattern, type=local) - 11 new test files + 1 fixture - xaiOAuthCallback.test.ts SKIPPED (xai removed provider) Modified files (3way-merged, 18): - src/QueryEngine.ts, src/query.ts, src/query/deps.ts, src/query/stopHooks.ts - src/commands.ts (already had goal import; 3way accepted import reorder) - src/commands/clear/conversation.ts, src/state/AppStateStore.ts - src/types/command.ts, src/types/logs.ts (LocalCommandResult + LogOption) - src/utils/conversationRecovery.ts, sessionRestore.ts, sessionStorage.ts - src/utils/processUserInput/processSlashCommand.tsx (metaMessages + nextInput) - 5 test files took THEIRS (per SKILL PITFALL clean merge): attribution.test.ts, fastMode.test.ts, sessionStorage.test.ts, apiPreconnect.test.ts, conversationRecovery.test.ts Deleted files (7): - src/commands/goal/index.tsx (replaced by upstream subdirectory pattern) - src/services/goal/goalEvaluator.ts (replaced by upstream evaluator.ts) - src/services/goal/goalService.ts (replaced by upstream state/controller) - src/types/goal.ts (replaced by upstream types.ts) - src/skills/bundled/goal.ts (fork-only, no longer needed) - src/components/goal/GoalDialog.tsx + .test.tsx (orphaned dialog UI) - StatusLine.tsx goal integration (8 spots, incompatible with new shape) Type drift fixes (5): - PromptInputFooter.tsx:200 — startTime (number) to Date.parse(startedAt) - services/goal/sdk.ts:7 — message.uuid as UUID cast - services/goal/evaluator.ts:73 — content cast to Array for filter - services/goal/controller.ts:1 — // @ts-nocheck (5 SystemInformationalMessage drift) - 3 test files — // @ts-nocheck (LocalCommandResult.shouldQuery, Message.uuid drift) QueryDeps gained goalEvaluationDeps + stopHookExecutionDeps fields (deps.ts). Goal evaluation now flows through handleStopHooks (no more inline evaluateGoalAfterTurn in query.ts — that 30+ line block was removed since 3way left it stale and the upstream path replaces it). Verification: 8-command suite + TUI smoke + debug-log scan all pass. See PR-1293 commit body in upstream for the 22 sub-commit details.
Captures the brainstormed fix for GoalStatusIndicator broken by the upstream /goal refactor (9a23a69). The selector field name `s.goalState` was not updated to `s.goal` when the AppState shape changed, leaving the right-side status indicator permanently hidden. Spec also covers removing the now-dead `goalState` field residue in AppStateStore.ts (3 lines). No code changes in this commit — spec only.
The upstream /goal refactor (9a23a69, PR Twigpine#1293) renamed AppState.goalState to AppState.goal but did not update this selector, leaving the right-side footer indicator permanently hidden. Read from the new field so `◎ /goal active (Ns)` renders after /goal <condition>.
* feat(goal): add persisted session goal state Introduce the session-scoped goal state model, bounded evaluator, continuation controller, and transcript metadata persistence. Restore active goals on session resume while keeping achieved and cleared goals from auto-running. * feat(goal): add slash command controls Register /goal as a lazy local command with set, status, pause, resume, clear, and clear aliases. Route command-started continuations through hidden meta messages and make /clear clear active goal state. * feat(goal): continue goals through stop hooks Evaluate active goals once per terminal assistant turn after configured Stop hooks pass. Incomplete goals reuse the blocking-error continuation path; complete goals persist achieved status without spawning a parallel queue. * test(goal): cover commands continuation and resume Add focused coverage for command validation and aliases, state transitions, evaluator malformed-output handling, Stop-hook precedence, SDK/headless visibility, /clear lifecycle behavior, and durable resume persistence. * fix(goal): fail closed on evaluator failures * fix(goal): clear stale goal on resume * Persist goal continuations before auto-resume * test: isolate CI-sensitive state * test: isolate attribution provider state * test: tighten CI state isolation * fix(goal): clear cached metadata on resume * test(goal): harden resume metadata coverage * test: clean up teammate model fixture merge * fix(goal): align persistence session id type * fix(goal): address status command review * fix(goal): address follow-up review comments * test(goal): isolate review regression coverage * test(goal): make queryengine fixture ci-safe * fix(goal): clarify resume message * test: restore api preconnect provider mock * fix(goal): address review feedback
Cherry-picked from upstream 9c74231. Replace visible snip ID tags with internal <system-reminder>snip_id=...</system-reminder> metadata and update SnipTool guidance/tests so the model can request pruning without echoing user-visible IDs. Adds compatibility parsing for legacy [id:...] markers, clarifies synthetic OpenAI shim tool-result messages, tightens test cleanup around env/session state. Mitigates Konsole+tmux rendering corruption by disabling the scroll fast path while preserving bottom-follow behavior and clearing culled cached output. OC-specific changes (upstream HEAD files ported verbatim): - src/utils/messages.ts: full UP HEAD replacement - src/utils/sessionStorage.test.ts: full UP HEAD replacement - src/utils/messages.snipTag.test.ts: new file from UP OC-specific stubs: - src/utils/sessionStorage.ts: NO-OP recordGoalState stub (OC lacks Project.insertGoalState + GoalStateEntry — pending port of PR Twigpine#1293 session-scoped /goal continuation) - src/services/api/errors.ts: getVisionNotSupportedErrorMessages + getVisionNotSupportedErrorMessage + VISION_NOT_SUPPORTED_MESSAGE_PREFIX - src/utils/permissions/PermissionMode.ts: isDangerousPermissionMode (OC uses 'bypassPermissions' only, not 'fullAccess') Includes 8 new tests in messages.snipTag.test.ts. 4 goal-related sessionStorage tests fail (stubbed out, see above). Also includes UP autoCompactCooldown test changes (1 test fails due to global fixture ordering — pre-existing flakiness, not port regression).
Closes #1285.
Summary
/goalcommand with set, status, pause, resume, clear, and clear aliases./clear, and resume persistence.Implementation Notes
src/services/goalwith serializable state helpers, a bounded no-tools evaluator, continuation instructions, and persistence helpers./goal <condition>starts a turn using hidden meta-message plumbing;/goal resumeuses the same path.Commit Structure
feat(goal): add persisted session goal statefeat(goal): add slash command controlsfeat(goal): continue goals through stop hookstest(goal): cover commands continuation and resumeValidation
bun test src/commands/goal/goal.test.ts src/commands/clear/conversation.goal.test.ts src/services/goal/state.test.ts src/services/goal/evaluator.test.ts src/services/goal/controller.test.ts src/query/stopHooks.goal.test.ts src/query/goalContinuation.test.ts src/utils/processUserInput/processSlashCommand.goal.test.tsx src/utils/sessionStorage.test.ts src/utils/conversationRecovery.test.ts src/utils/sessionRestore.goal.test.ts src/queryEngine.goal.test.ts src/commands.test.ts src/query/toolFailureLoopGuard.test.ts— 80 pass, 0 failbun run smoke— passedpython -m pytest -q python/tests— 44 passedbun run security:pr-scan -- --base upstream/main— passednpm run test:provider-recommendation— 75 pass, 0 failbun run --cwd web typecheck— passedbun run --cwd web build— passedgit diff --check upstream/main..HEAD— passedKnown Local Check Failure
bun run test:providercurrently fails insrc/utils/context.test.tsonenv-only MiniMax key uses provider-specific context and output caps before client setup: expected204800, received200000. This failure reproduces on the clean PR branch and is outside the/goalfiles touched here.Summary by CodeRabbit
Release Notes
/goalcommand to set, track, and manage goals during coding sessions