Repository navigation
feat: render live Working Plan revisions in TUI and Web - #309
Conversation
ghfind-review
Bot
added
the
review: high
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
ghfind-review
Bot
added
area: architecture
![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: acba71309c
ℹ️ 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".
| if (data.interaction) renderSystemOneStatus({...systemOneStatus, system_one_backend: data.interaction.system_one_backend, | ||
| decision_assist: data.decision_assist || decisionAssistState}); | ||
| renderInteraction(data.interaction); | ||
| renderWorkingPlan(data.working_plan); |
There was a problem hiding this comment.
Avoid passing an absent field to renderWorkingPlan here. The /api/chat response does not include working_plan (api_chat returns only reply/session/profile/interaction/context data), so every submitControl call clears the currently displayed plan. This also affects the Accept action: the request can create the Working Plan, but the response immediately hides it instead of showing the accepted plan. Include the current snapshot in the response or update the panel only when the field is present.
Useful? React with 👍 / 👎.
|
|
||
| function renderWorkingPlan(value) { | ||
| if (value && workingPlan && value.plan_id === workingPlan.plan_id && value.revision < workingPlan.revision) return; | ||
| if (value && workingPlan && value.updated_at && workingPlan.updated_at && value.updated_at < workingPlan.updated_at) return; |
There was a problem hiding this comment.
Only apply this stale-timestamp check when value.plan_id === workingPlan.plan_id. workingPlan is global across restored sessions, so switching from a recently updated session to an older saved session whose plan has an earlier updated_at causes this return to retain and display the previous session's plan even though the fetched plan has a different ID.
Useful? React with 👍 / 👎.
| if event.kind.startswith("working_plan."): | ||
| return {"event": "working_plan", "session_id": event.session_id, "working_plan": payload} |
There was a problem hiding this comment.
Do not treat every working_plan.* payload as a renderable snapshot. TaskManager.adopt_legacy_plan still emits working_plan.migrated with only plan_id, complexity, and task_ids; during a complex legacy Plan/TaskList turn this branch forwards that metadata to both clients, which replace the current panel with a plan lacking revision, status, timestamps, and task objects until a later task snapshot arrives. Emit working_plan_snapshot() for that event or restrict this projection to event kinds whose payload has the full snapshot shape.
Useful? React with 👍 / 👎.
| events = self.store.list_events( | ||
| "working_plan.updated", limit=20, user_id=self.user_id, | ||
| workspace_id=self.workspace_id, session_id=self.session_id, | ||
| ) | ||
| for event in events: |
There was a problem hiding this comment.
Include working_plan.cancelled when restoring the current revision's rationale. A structural edit that cancels the final unfinished task persists its reason on a working_plan.cancelled event, while this lookup searches only working_plan.updated; consequently the immediate callback has the reason only because edit_plan patches it in memory, and the next working_plan_snapshot() returns an empty reason and removes the rationale from the live TUI/Web panel.
Useful? React with 👍 / 👎.
| plan_task = next((task for task in self.tasks if task["id"] in plan_ids), None) | ||
| if plan_task is not None: | ||
| self._event(plan_task, "working_plan.cancelled", {"plan_id": plan.id}) | ||
| self._event(plan_task, "working_plan.cancelled", self.working_plan_snapshot()) |
There was a problem hiding this comment.
Update self.working_plan before building this cancellation snapshot. set_working_plan_status changes only the database row, so working_plan_snapshot() still reads the in-memory object's status == "active"; clearing an active plan therefore emits a working_plan.cancelled event whose embedded snapshot claims the plan is active while all its tasks are cancelled, producing an internally contradictory live update.
Useful? React with 👍 / 👎.
| const links = task.superseded_by?.length | ||
| ? ` · superseded by ${task.superseded_by.map(id => (workingPlan.tasks || []).find(other => other.id === id)?.description || id).join(', ')}` : ''; | ||
| const blocker = task.status === 'blocked' && task.reason ? ` · ${task.reason}` : ''; | ||
| item.textContent = `${task.description} · ${task.status}${links}${blocker}`; |
There was a problem hiding this comment.
Translate the succeeded status to the user-facing verified label here, as the TUI does. The Web panel currently renders the raw text succeeded, so the two newly added plan views disagree and the Web UI does not communicate the evidence-backed verification state promised by this change.
Useful? React with 👍 / 👎.
| tasks.append({ | ||
| "id": task_id, | ||
| "description": str(task.get("description", "")), | ||
| "status": str(task.get("status", "pending")), | ||
| "supersedes": list(checkpoint.get("supersedes", [])) if isinstance(checkpoint, dict) else [], | ||
| "superseded_by": list(checkpoint.get("superseded_by", [])) if isinstance(checkpoint, dict) else [], | ||
| "reason": str(block.get("reason", "")) if isinstance(block, dict) else "", |
There was a problem hiding this comment.
Bound or truncate the projected task text before placing it in runtime/interface events. Plan task descriptions, blocker reasons, revision reasons, and task counts have no corresponding size limits, but canonical runtime events reject payloads above 128 KB and the TUI JSONL boundary replaces lines above 64 KB with event_too_large; for example, several allowed 12 KB task descriptions are enough for the TUI to lose the entire tasks/plan update. The new snapshot should enforce a total budget rather than copying every full description into each event.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
1 participant
Closes #256
Root cause
Working Plan revisions existed durably in task state, but the TUI and Web boundaries exposed only flattened task lists. Plan rationale, ordering, supersession, and a coherent revision snapshot were therefore unavailable to the views, and status events did not update the current plan display.
Approach
Expose one bounded, redacted Working Plan projection from
TaskManager, recoverable directly from scoped SQLite state. Forward it through existing task/runtime events and session restoration. Both interfaces render ordered revisions, rationale, cancellation/supersession, blockers, and task progress; only task statussucceededis presented as verified. Kept the existing API entry points and task-list compatibility shape.Alternatives
Adding a separate plan UI API or deriving the view from model/provider text would duplicate state or expose protocol details. The existing task store and typed runtime events already own this information, so the projection uses those boundaries.
Validation
make test— 1,025 passed, 13 skipped; browser acceptance included.make check,make lint,make docs-check— passed.go test -count=1 ./...— passed.git verify-commit.Risks and follow-ups
The plan view intentionally omits execution evidence and private tool arguments. Platform release matrix and soak tests remain release-gate work. No new dependency or API endpoint was added.
Depends on the V3 plan, completion, event, and task-history foundations already merged to
main; not stacked on an open PR.