feat(console): complete goal stack reconciliation - #95
Conversation
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughWalkthroughThe PR extends Goal Stack plan normalization with escalation and degraded-stream handling, materializes plans as advisory programs, updates responsive Goal Stack and sidebar behavior, and expands unit, route, and E2E coverage. ChangesGoal Stack execution and presentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlanEvents
participant normalizePlanPoll
participant GoalStackView
participant PlanTaskNode
PlanEvents->>normalizePlanPoll: task transitions and escalation details
normalizePlanPoll->>GoalStackView: normalized snapshot
GoalStackView->>PlanTaskNode: task status and escalation fields
PlanTaskNode-->>GoalStackView: escalation and changed badges
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59221757a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const foldEvents = inspected | ||
| ? events.filter((event) => event.graphVersion > inspected.streamCursor) | ||
| : events; |
There was a problem hiding this comment.
When what_changed fails, the route deliberately retains the old cursor, but the fresh inspect snapshot can still carry a newer watermark. On the first successful poll after recovery, this filter drops every replayed event at or below that inspect watermark, including the previously unavailable interval. The snapshot preserves current task status, but event-only projection data such as changed paths and escalation receipts is permanently lost; filter against the last successfully consumed event cursor when recovery is in progress instead.
Useful? React with 👍 / 👎.
| progressFraction: task.progressFraction ?? prior.progressFraction, | ||
| progressNote: task.progressNote ?? prior.progressNote, | ||
| assignedHead: task.assignedHead ?? prior.assignedHead, | ||
| escalation: task.escalation ?? prior.escalation, |
There was a problem hiding this comment.
When a later authoritative inspect response clears an optional field, this merge restores the previous value instead. For example, a task that is reassigned or resumes after escalation can return no assigned_head or escalation, yet the canvas continues to show its former target and escalation receipt; similarly, a reset task continues to show its old progress. Preserve historical event data separately, but let explicit fresh task state replace these current-state fields.
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| const routedId = surfaceIdForPath(pathname); | ||
| if (!routedId || routedId === activeSurfaceId) return; | ||
| void host.activateSurface(routedId); | ||
| }, [activeSurfaceId, host, pathname]); | ||
| if (routedId) void host.activateSurface(routedId); | ||
| }, [host, pathname]); |
There was a problem hiding this comment.
Selecting a routed surface from MainToolbar still calls only host.activateSurface, while this effect no longer reruns after activeSurfaceId changes. As a result, choosing Workspace, Chat, Documents, Filing, or Cards from the toolbar can render that surface while the URL remains on the prior route; refreshing or sharing the URL then opens a different surface. Route toolbar selections through router.push (or retain reconciliation when the active surface changes).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR improves the Console Goal Stack’s reconciliation and promotion pipeline by making plan polling more resilient (especially under degraded event streams), projecting additional task lifecycle detail (escalations, assigned heads, changed paths), and ensuring “Save as program” produces an explicitly advisory, non-side-effecting programmable graph.
Changes:
- Extend plan/task projection to support durable escalations, assigned heads, more complete changed-path ingestion, and better event-to-task targeting/canonicalization.
- Make plan polling cursor behavior robust when
what_changedis degraded, while preserving event history and task change history across inspect snapshots. - Update Console Goal Stack UI/UX (responsive resizable panes, additional task badges/inspector details) and harden tests (unit + route + e2e) around these behaviors.
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/theorem-acp/src/plan-state.ts | Adds escalated status and escalation/assigned-head projection; improves event normalization, cursor handling, and history merging across snapshots. |
| packages/theorem-acp/src/plan-program.ts | Makes promoted programs advisory (authority: advisory), non-side-effecting, and records side-effecting affordances in metadata only. |
| apps/console/src/views/goal-stack/ToolPalette.tsx | Adds a stable data-plan-capability attribute to support reliable e2e DnD selection. |
| apps/console/src/views/goal-stack/PromotionDialog.tsx | Resets dialog state via keyed remount and surfaces an advisory warning for side-effecting source affordances. |
| apps/console/src/views/goal-stack/ProgressEdge.tsx | Treats escalated edges as warn-color and renders the running overlay consistently (incl. 0 progress). |
| apps/console/src/views/goal-stack/ProgressEdge.test.tsx | Adds coverage for “running with 0 progress” rendering behavior. |
| apps/console/src/views/goal-stack/PlanTaskNode.tsx | Tightens droppable eligibility and adds badges for approval requirement, escalation target, and changed-path presence. |
| apps/console/src/views/goal-stack/PlanTaskNode.test.tsx | Adds coverage for rendering escalation target details on nodes. |
| apps/console/src/views/goal-stack/plan-layout.ts | Adds escalated as a projected edge state. |
| apps/console/src/views/goal-stack/NodeInspector.tsx | Shows assigned head + escalation detail; improves revert/attachment gating and adds “replan subtree” affordance. |
| apps/console/src/views/goal-stack/GoalStackView.tsx | Introduces resizable responsive panes and ensures pins load synchronously when switching plans. |
| apps/console/src/motion/motion.css | Scales edge animation timing with motion variables. |
| apps/console/src/lib/plan-route.test.ts | Adds route-level tests for refusal mapping, advisory promotion behavior, and degraded-cursor semantics. |
| apps/console/src/lib/goal-stack-contract.test.ts | Adds contract-level tests for event folding, degraded cursor behavior, escalation projection, and advisory promotion metadata. |
| apps/console/src/components/shell/Sidebar.tsx | Improves collapse behavior with compact mode and async write-through handling. |
| apps/console/src/components/shell/IntuiShell.tsx | Simplifies route-driven surface activation behavior. |
| apps/console/src/app/api/harness/plan/route.ts | Ensures cursor stability under degraded events, always refreshes runs rail, demotes side-effecting affordances to metadata, and expands refusal envelope parsing. |
| apps/console/e2e/workspace-goal-stack.spec.ts | Makes surface navigation assertions more robust; stabilizes Goal Stack open+DnD flows across viewports and adds new UI assertions/screenshots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
apps/console/src/views/goal-stack/NodeInspector.tsx (1)
173-208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep subtree replanning limited to blocked tasks.
Lines 189-196 are still inside
taskRevertEligible, which means failed and superseded tasks retainreplan_subtree. Remove that button from this fragment and leave the blocked-only branch.Proposed fix
- <button - type="button" - disabled={busy} - onClick={() => mutate('replan_subtree', { taskId: task.id })} - className="h-ij-control rounded-ij-arc bg-ij-accent px-3 text-ij-ink-bright" - > - Replan subtree - </button>🤖 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 `@apps/console/src/views/goal-stack/NodeInspector.tsx` around lines 173 - 208, Remove the “Replan subtree” button from the taskRevertEligible fragment, leaving only the revert controls there. Keep subtree replanning available exclusively through the existing task.status === 'blocked' branch.
🧹 Nitpick comments (1)
apps/console/src/app/api/harness/plan/route.ts (1)
35-50: 🚀 Performance & Scalability | 🔵 TrivialRuns rail
progressquery now fires unconditionally on every poll tick.The
runsRailquery const runsRail = await callHarnessMcp('plan', { action: 'query', plan_id: planId, query: 'progress' }); is no longer gated behindincludeManifestthe way the capability manifest fetch is (that one is gated and only re-fetched once per session viamanifestLoadedRef). Since the Goal Stack view polls this route roughly every 1.5s per open plan session, this adds one more recurring upstream MCP round trip per poll tick, per open session, indefinitely. If this endpoint becomes a hot path with several concurrently open plans, consider throttling/caching the progress query similarly to how the manifest fetch is gated.🤖 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 `@apps/console/src/app/api/harness/plan/route.ts` around lines 35 - 50, The plan polling route unconditionally calls the runsRail progress query on every poll. Update the runsRail flow around callHarnessMcp and normalizeRunsFromProgress to gate or cache progress retrieval, using the existing manifestLoadedRef/session-fetch pattern or an equivalent throttle so repeated polls do not issue an upstream request indefinitely while preserving the current runsRail response 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 `@apps/console/e2e/workspace-goal-stack.spec.ts`:
- Around line 45-53: Update openGoalPlan so its fast path verifies that the
currently rendered plan matches the requested planId before skipping the open
flow. Read the visible plan’s ID or another plan-specific invariant, and only
reuse the existing surface when it matches; otherwise fill the Plan id input and
click Open, then retain the final task visibility assertion.
In `@apps/console/src/components/shell/Sidebar.tsx`:
- Around line 200-206: Update toggleCollapse in Sidebar so it returns
immediately when compact mode is active, matching the footer action’s disabled
behavior. Ensure Cmd/Ctrl+B does not call setCollapseOverride or persist the
collapsed update while compact, while preserving the existing toggle behavior in
non-compact mode and updating the callback dependencies as needed.
In `@apps/console/src/views/goal-stack/GoalStackView.tsx`:
- Around line 65-68: Update openPlan and the other plan-switching paths to clear
all plan-bound state—snapshot, selected task, nodes, and capabilities—before
changing the active plan via setPlanId. Then load pins for the new plan and
preserve the empty-map behavior when no plan is selected, preventing stale
interactions during polling.
In `@packages/theorem-acp/src/plan-state.ts`:
- Around line 239-254: Update the snapshot merge flow around
mergeSnapshotHistory so stale inspected snapshots do not overwrite
previous.status or previous.claimHolder when inspected.streamCursor is less than
previous.streamCursor. Preserve the inspected snapshot’s status and claimHolder
when its cursor is equal to or newer than previous.streamCursor, while leaving
event folding and cursor calculation unchanged.
---
Outside diff comments:
In `@apps/console/src/views/goal-stack/NodeInspector.tsx`:
- Around line 173-208: Remove the “Replan subtree” button from the
taskRevertEligible fragment, leaving only the revert controls there. Keep
subtree replanning available exclusively through the existing task.status ===
'blocked' branch.
---
Nitpick comments:
In `@apps/console/src/app/api/harness/plan/route.ts`:
- Around line 35-50: The plan polling route unconditionally calls the runsRail
progress query on every poll. Update the runsRail flow around callHarnessMcp and
normalizeRunsFromProgress to gate or cache progress retrieval, using the
existing manifestLoadedRef/session-fetch pattern or an equivalent throttle so
repeated polls do not issue an upstream request indefinitely while preserving
the current runsRail response 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95d9caa4-c4ae-44b2-9f98-2daf41ad0808
⛔ Files ignored due to path filters (4)
apps/console/e2e/workspace-goal-stack.spec.ts-snapshots/goal-stack-v2-1024-dark-darwin.pngis excluded by!**/*.pngapps/console/e2e/workspace-goal-stack.spec.ts-snapshots/goal-stack-v2-1440-dark-darwin.pngis excluded by!**/*.pngapps/console/e2e/workspace-goal-stack.spec.ts-snapshots/goal-stack-v2-390-dark-darwin.pngis excluded by!**/*.pngapps/console/e2e/workspace-goal-stack.spec.ts-snapshots/workspace-substrate-v2-1440-dark-darwin.pngis excluded by!**/*.png
📒 Files selected for processing (18)
apps/console/e2e/workspace-goal-stack.spec.tsapps/console/src/app/api/harness/plan/route.tsapps/console/src/components/shell/IntuiShell.tsxapps/console/src/components/shell/Sidebar.tsxapps/console/src/lib/goal-stack-contract.test.tsapps/console/src/lib/plan-route.test.tsapps/console/src/motion/motion.cssapps/console/src/views/goal-stack/GoalStackView.tsxapps/console/src/views/goal-stack/NodeInspector.tsxapps/console/src/views/goal-stack/PlanTaskNode.test.tsxapps/console/src/views/goal-stack/PlanTaskNode.tsxapps/console/src/views/goal-stack/ProgressEdge.test.tsxapps/console/src/views/goal-stack/ProgressEdge.tsxapps/console/src/views/goal-stack/PromotionDialog.tsxapps/console/src/views/goal-stack/ToolPalette.tsxapps/console/src/views/goal-stack/plan-layout.tspackages/theorem-acp/src/plan-program.tspackages/theorem-acp/src/plan-state.ts
| async function openGoalPlan(page: Page, planId: string) { | ||
| const activeSurface = await page.locator('[data-shell]').getAttribute('data-active-surface'); | ||
| if (activeSurface !== 'console-goals') await openSurface(page, 'console-goals'); | ||
| const firstTask = page.locator('[data-plan-task]').first(); | ||
| if (!await firstTask.isVisible().catch(() => false)) { | ||
| await page.locator('input[aria-label="Plan id"]').fill(planId); | ||
| await page.getByRole('button', { name: 'Open' }).click(); | ||
| } | ||
| await expect(firstTask).toBeVisible(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the fast path to the requested plan.
When any task is visible, firstTask.isVisible() is true and planId is ignored. A different or stale plan can therefore remain open while the test proceeds. Compare the rendered plan ID, or another plan-specific invariant, before skipping the open flow.
🤖 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 `@apps/console/e2e/workspace-goal-stack.spec.ts` around lines 45 - 53, Update
openGoalPlan so its fast path verifies that the currently rendered plan matches
the requested planId before skipping the open flow. Read the visible plan’s ID
or another plan-specific invariant, and only reuse the existing surface when it
matches; otherwise fill the Plan id input and click Open, then retain the final
task visibility assertion.
| const toggleCollapse = useCallback(() => { | ||
| if (!landmarksRegion) return; | ||
| const next = !collapsed; | ||
| setCollapsed(next); | ||
| void host.emit({ kind: 'update', id: landmarksRegion.object.id, patch: { collapsed: next } }); | ||
| setCollapseOverride(next); | ||
| void host.emit({ kind: 'update', id: landmarksRegion.object.id, patch: { collapsed: next } }) | ||
| .finally(() => setCollapseOverride(null)); | ||
| }, [collapsed, host, landmarksRegion]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore the collapse shortcut in compact mode.
The footer disables this action when compact, but Cmd/Ctrl+B still persists collapsed. Returning to a wider layout can therefore leave the sidebar unexpectedly collapsed.
Proposed fix
const toggleCollapse = useCallback(() => {
- if (!landmarksRegion) return;
+ if (compact || !landmarksRegion) return;
const next = !collapsed;
setCollapseOverride(next);
void host.emit({ kind: 'update', id: landmarksRegion.object.id, patch: { collapsed: next } })
.finally(() => setCollapseOverride(null));
-}, [collapsed, host, landmarksRegion]);
+}, [collapsed, compact, host, landmarksRegion]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const toggleCollapse = useCallback(() => { | |
| if (!landmarksRegion) return; | |
| const next = !collapsed; | |
| setCollapsed(next); | |
| void host.emit({ kind: 'update', id: landmarksRegion.object.id, patch: { collapsed: next } }); | |
| setCollapseOverride(next); | |
| void host.emit({ kind: 'update', id: landmarksRegion.object.id, patch: { collapsed: next } }) | |
| .finally(() => setCollapseOverride(null)); | |
| }, [collapsed, host, landmarksRegion]); | |
| const toggleCollapse = useCallback(() => { | |
| if (compact || !landmarksRegion) return; | |
| const next = !collapsed; | |
| setCollapseOverride(next); | |
| void host.emit({ kind: 'update', id: landmarksRegion.object.id, patch: { collapsed: next } }) | |
| .finally(() => setCollapseOverride(null)); | |
| }, [collapsed, compact, host, landmarksRegion]); |
🤖 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 `@apps/console/src/components/shell/Sidebar.tsx` around lines 200 - 206, Update
toggleCollapse in Sidebar so it returns immediately when compact mode is active,
matching the footer action’s disabled behavior. Ensure Cmd/Ctrl+B does not call
setCollapseOverride or persist the collapsed update while compact, while
preserving the existing toggle behavior in non-compact mode and updating the
callback dependencies as needed.
| const openPlan = useCallback((nextPlanId: string) => { | ||
| setPlanId(nextPlanId); | ||
| setPins(nextPlanId ? loadPins(nextPlanId) : new Map()); | ||
| }, []); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset plan-bound state before changing the active plan.
planId changes immediately, but the prior snapshot, selected task, nodes, and capabilities remain interactive until the new poll resolves. An action in that window posts the old task ID to the new plan.
Proposed fix
const openPlan = useCallback((nextPlanId: string) => {
+ setSnapshot(null);
+ setCapabilities([]);
+ setNodes([]);
+ setEdges([]);
+ setSelectedTaskId(null);
+ setPromotionOpen(false);
setPlanId(nextPlanId);
setPins(nextPlanId ? loadPins(nextPlanId) : new Map());
}, []);Also applies to: 189-194, 211-214
🤖 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 `@apps/console/src/views/goal-stack/GoalStackView.tsx` around lines 65 - 68,
Update openPlan and the other plan-switching paths to clear all plan-bound
state—snapshot, selected task, nodes, and capabilities—before changing the
active plan via setPlanId. Then load pins for the new plan and preserve the
empty-map behavior when no plan is selected, preventing stale interactions
during polling.
| const eventStreamDegraded = record(payload.degraded)?.events === true; | ||
| const base = inspected ? mergeSnapshotHistory(inspected, previous) : previous; | ||
| if (!base) throw new Error('Plan stream response did not contain a usable snapshot.'); | ||
| const foldEvents = inspected | ||
| ? events.filter((event) => event.graphVersion > inspected.streamCursor) | ||
| : events; | ||
| const snapshot = foldEvents.reduce(applyPlanEvent, base); | ||
|
|
||
| const explicitCursor = number(payload.cursor); | ||
| const cursor = eventStreamDegraded | ||
| ? Math.max(previous?.streamCursor ?? 0, explicitCursor ?? previous?.streamCursor ?? 0) | ||
| : Math.max( | ||
| snapshot.streamCursor, | ||
| explicitCursor ?? 0, | ||
| ...events.map((event) => event.graphVersion), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'plan-state\.ts$' . || true
echo "== file stats =="
stat -c '%n %s' packages/theorem-acp/src/plan-state.ts || true
wc -l packages/theorem-acp/src/plan-state.ts
echo "== relevant outlines =="
ast-grep outline packages/theorem-acp/src/plan-state.ts --view expanded || true
echo "== relevant snippets =="
sed -n '200,275p' packages/theorem-acp/src/plan-state.ts
echo "-----"
sed -n '430,505p' packages/theorem-acp/src/plan-state.ts
echo "== search mergeSnapshotHistory and related usages =="
rg -n "mergeSnapshotHistory|foldEvents|streamCursor|applyPlanEvent|what_changed|degraded|status|claimHolder" packages/theorem-acp/src/plan-state.tsRepository: Travis-Gilbert/CommonPlace
Length of output: 13016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests mentioning normalizePlanPoll / mergeSnapshotHistory / stale cursor / what_changed =="
rg -n "normalizePlanPoll|mergeSnapshotHistory|what_changed|streamCursor|status|claimHolder|stale" packages/theorem-acp -g '*.ts' -g '*.js' || true
echo "== test files around plan-state =="
fd -a '.*test.*|.*spec.*' packages/theorem-acp || true
echo "== inspect tests for plan-state =="
rg -n "describe|it\\(|test\\(|normalizePlanPoll|streamCursor|status|claimHolder|degraded" packages/theorem-acp --glob '*.test.ts' --glob '*.spec.ts' --glob '*.test.js' --glob '*.spec.js' || trueRepository: Travis-Gilbert/CommonPlace
Length of output: 7031
Guard stale status/claimHolder when merging snapshots across polls.
When inspected.streamCursor < previous.streamCursor, mergeSnapshotHistory still overwrites status and claimHolder from the fresher payload while skipping previously folded events, so task state can regress after an advanced cursor has already folded a later event. Use previous for these fields when the inspected cursor is stale; preserve status/claimHolder authoritative when cursors are equal or newer.
🤖 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 `@packages/theorem-acp/src/plan-state.ts` around lines 239 - 254, Update the
snapshot merge flow around mergeSnapshotHistory so stale inspected snapshots do
not overwrite previous.status or previous.claimHolder when
inspected.streamCursor is less than previous.streamCursor. Preserve the
inspected snapshot’s status and claimHolder when its cursor is equal to or newer
than previous.streamCursor, while leaving event folding and cursor calculation
unchanged.
Narrow harness boot MCP error access, clear set-state-in-effect lint, brace broken Playwright evaluate callbacks, and loosen jotai facade typing so console build and gates can pass on main. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary by CodeRabbit