refactor(todo): remove priority field from todo system #1223
Conversation
Remove the priority field from the todo system to simplify the interface. The priority field served no meaningful purpose in the current implementation and was not displayed in the UI panel or used in continuation logic. Changes: - Remove TodoPriority enum and priority field from todo-schemas.ts - Update todo-write.ts and todo-read.ts to remove priority from statistics and sorting (now sorts by status then alphabetically) - Update todoFormatter.ts to remove priority labels and sorting - Remove priority normalization from TextToolCallParser.ts - Update all test files to remove priority references - Update documentation in docs/todo-system.md Backward compatibility is maintained - Zod strips unknown fields, so existing todo files with priority will load without errors. Closes #1165
github-actions
bot
added
the
maintainer:e2e:ok
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
LLxprt PR Review – PR #1223Issue AlignmentReady. This PR directly addresses issue #1165 by removing the
Side Effects
Code Quality
Tests and CoverageCoverage Impact: Increase (tests updated to reflect new behavior) Tests are present and substantive:
VerdictReady. This PR is well-executed with comprehensive changes, proper test coverage, and clear documentation updates. The backward compatibility concern is mitigated by Zod's behavior. One minor note: verify the completed icon change from |
![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
🤖 Fix all issues with AI agents
In `@docs/todo-system.md`:
- Line 46: The backticked placeholder "` Todo list updated`" contains a leading
space which violates MD038; update the markdown in docs/todo-system.md to remove
the leading space (change to "`Todo list updated`") or convert the placeholder
into normal prose, ensuring any reference to the todo_write tool calls still
reads correctly.
In `@packages/cli/src/services/todo-continuation/todoContinuationService.ts`:
- Around line 536-550: The alphabetical sort in findBestActiveTodo can throw
when todo.content is undefined; update the pendingTodos.sort call to compare
content in a null-safe way (e.g., use a default empty string or other fallback
for a.content and b.content before calling localeCompare) so malformed or
user-edited todos don't crash the continuation; ensure the comparator references
todo.content fallback consistently and keeps the same sort semantics for normal
entries.
In `@packages/core/src/todo/todoFormatter.ts`:
- Around line 21-25: The STATUS_ICONS map currently sets completed: '' which
causes the formatter to still emit a space prefix even when the icon is empty;
update the formatting logic that uses STATUS_ICONS (look for the function that
builds the line/label for a Todo using STATUS_ICONS and Todo['status']) to only
prepend the icon plus a trailing space when the icon string is non-empty (i.e.,
conditionally add `${icon} ` or nothing), ensuring alignment is preserved for
other statuses while avoiding a stray leading space for completed items.
In `@packages/core/src/tools/todo-read.test.ts`:
- Around line 159-161: The test currently uses a vacuous assertion
expect(result.llmContent).toContain('') which always passes; replace it with an
explicit check for the completed-task marker or completed task line used by the
todo renderer. Inspect how completed items are rendered in the codebase (the
todo formatter/parser used by the test) to find the exact symbol or text for
completed tasks, then change the assertion to something like
expect(result.llmContent).toContain('<completed-marker>') or use a regex/line
match (e.g., expect(result.llmContent).toMatch(/completed task text or marker/))
referencing result.llmContent in this test so the completed state is actually
validated instead of asserting an empty string.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
docs/todo-system.mdintegration-tests/todo-continuation.e2e.test.jspackages/cli/src/integration-tests/todo-continuation.integration.test.tspackages/cli/src/services/todo-continuation/todoContinuationService.spec.tspackages/cli/src/services/todo-continuation/todoContinuationService.tspackages/cli/src/ui/components/TodoPanel.responsive.test.tsxpackages/cli/src/ui/components/TodoPanel.semantic.test.tsxpackages/cli/src/ui/components/messages/ToolGroupMessage.test.tsxpackages/cli/src/ui/hooks/useGeminiStream.integration.test.tsxpackages/cli/src/ui/hooks/useTodoContinuation.spec.tspackages/cli/src/zed-integration/schema.tspackages/cli/src/zed-integration/zedIntegration.tspackages/core/src/core/client.test.tspackages/core/src/core/client.tspackages/core/src/core/subagent.test.tspackages/core/src/parsers/TextToolCallParser.tspackages/core/src/services/todo-reminder-service.tspackages/core/src/todo/todoFormatter.tspackages/core/src/tools/ToolFormatter.test.tspackages/core/src/tools/todo-read.test.tspackages/core/src/tools/todo-read.tspackages/core/src/tools/todo-schemas.test.tspackages/core/src/tools/todo-schemas.tspackages/core/src/tools/todo-store.test.tspackages/core/src/tools/todo-write.test.tspackages/core/src/tools/todo-write.tspackages/core/src/utils/parameterCoercion.test.ts
💤 Files with no reviewable changes (15)
- packages/core/src/core/client.ts
- packages/core/src/tools/todo-schemas.ts
- packages/core/src/tools/todo-schemas.test.ts
- packages/cli/src/ui/components/TodoPanel.responsive.test.tsx
- packages/core/src/core/client.test.ts
- packages/cli/src/zed-integration/zedIntegration.ts
- packages/cli/src/ui/components/TodoPanel.semantic.test.tsx
- packages/core/src/services/todo-reminder-service.ts
- packages/core/src/parsers/TextToolCallParser.ts
- packages/cli/src/zed-integration/schema.ts
- packages/core/src/core/subagent.test.ts
- packages/core/src/tools/todo-write.test.ts
- packages/core/src/tools/todo-store.test.ts
- packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx
- packages/cli/src/ui/hooks/useGeminiStream.integration.test.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-14T08:00:48.375Z
Learnt from: sogoiii
Repo: vybestack/llxprt-code PR: 1124
File: packages/core/src/providers/ProviderManager.ts:869-884
Timestamp: 2026-01-14T08:00:48.375Z
Learning: In ProviderManager.getAvailableModels() in packages/core/src/providers/ProviderManager.ts, the supportedToolFormats field on IModel/HydratedModel is not actively used after the tool capabilities filtering feature was removed. It can be set to an empty array when constructing models from the registry fallback.
Applied to files:
packages/core/src/tools/ToolFormatter.test.ts
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `node scripts/start.js --profile-load synthetic --prompt "write me a haiku"` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
integration-tests/todo-continuation.e2e.test.js
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run typecheck` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
integration-tests/todo-continuation.e2e.test.js
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run lint` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
integration-tests/todo-continuation.e2e.test.js
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run test` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
integration-tests/todo-continuation.e2e.test.js
🧬 Code graph analysis (4)
packages/core/src/tools/todo-read.test.ts (1)
packages/core/src/tools/todo-store.ts (1)
TodoStore(13-74)
integration-tests/todo-continuation.e2e.test.js (1)
integration-tests/test-helper.ts (3)
TestRig(243-1324)printDebugInfo(78-104)validateModelOutput(107-151)
packages/cli/src/integration-tests/todo-continuation.integration.test.ts (1)
packages/core/src/tools/todo-schemas.ts (1)
Todo(42-42)
packages/cli/src/ui/hooks/useTodoContinuation.spec.ts (1)
packages/core/src/tools/todo-schemas.ts (1)
Todo(42-42)
🪛 markdownlint-cli2 (0.18.1)
docs/todo-system.md
46-46: Spaces inside code span elements
(MD038, no-space-in-code)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (19)
packages/cli/src/integration-tests/todo-continuation.integration.test.ts (1)
642-644: LGTM — malformed todo case still validates the schema guard.
Keeping the missingcontentcase is sufficient for negative-path coverage after removingpriority.packages/cli/src/ui/hooks/useTodoContinuation.spec.ts (1)
519-531: Good malformed-data coverage.
The fixture now includes a pending todo missingcontent, which is a useful edge-case regression check for the continuation path.packages/core/src/utils/parameterCoercion.test.ts (1)
228-247: LGTM: payload updated for new Todo shape.
Test data now matches the priority-free schema, which keeps coercion coverage accurate.packages/cli/src/services/todo-continuation/todoContinuationService.spec.ts (1)
362-413: Tests align with alphabetical selection behavior.
Nice update to verify the first pending todo is chosen by content order when noin_progressitems exist.integration-tests/todo-continuation.e2e.test.js (3)
31-35: LGTM: basic flow updated for priority-free todos.
The scenario now reflects the new todo shape without priority metadata.
163-165: LGTM: auto-resume setup aligns with new input shape.
The updated creation prompt keeps the test consistent with the simplified todo schema.
394-450: Good coverage for alphabetical selection.
The multi-active scenario now correctly asserts the alphabetically first task is chosen.docs/todo-system.md (1)
9-10: Docs align with priority removal.Concise and consistent with the updated output semantics.
packages/core/src/tools/ToolFormatter.test.ts (1)
47-47: Schema expectations updated correctly.Required fields now match the priority-free Todo schema.
packages/core/src/tools/todo-read.ts (3)
25-25: Updated description is accurate.Reflects the simplified Todo shape.
104-107: Alphabetical selection for in-progress tasks looks good.
118-121: Alphabetical selection for pending tasks looks good.packages/core/src/tools/todo-write.ts (3)
96-96: Required fields updated correctly.
251-254: Next-action selection by content is consistent with new behavior.
265-268: Pending-task selection by content is consistent with new behavior.packages/core/src/tools/todo-read.test.ts (4)
19-43: Fixture updates match the new semantics.
70-72: Formatting assertions updated correctly.
85-91: Status ordering expectations look correct.
98-115: Alphabetical ordering test is clear and aligned.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| - The Todo panel is hidden immediately—no restart required. | ||
| - `todo_write` tool calls render the full structured todo list inline (status icons, priorities, subtasks, recent tool calls) instead of the `✦ Todo list updated` placeholder. | ||
| - `todo_write` tool calls render the full structured todo list inline (status icons, subtasks, recent tool calls) instead of the ` Todo list updated` placeholder. |
There was a problem hiding this comment.
Fix code span with leading space (MD038).
The backticked placeholder has a leading space, which violates markdownlint’s no-space-in-code rule and can render inconsistently. Prefer removing the space or describing it in prose.
📝 Suggested doc fix
-- `todo_write` tool calls render the full structured todo list inline (status icons, subtasks, recent tool calls) instead of the ` Todo list updated` placeholder.
+- `todo_write` tool calls render the full structured todo list inline (status icons, subtasks, recent tool calls) instead of the `Todo list updated` placeholder.📝 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.
| - `todo_write` tool calls render the full structured todo list inline (status icons, subtasks, recent tool calls) instead of the ` Todo list updated` placeholder. | |
| - `todo_write` tool calls render the full structured todo list inline (status icons, subtasks, recent tool calls) instead of the `Todo list updated` placeholder. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
46-46: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
In `@docs/todo-system.md` at line 46, The backticked placeholder "` Todo list
updated`" contains a leading space which violates MD038; update the markdown in
docs/todo-system.md to remove the leading space (change to "`Todo list
updated`") or convert the placeholder into normal prose, ensuring any reference
to the todo_write tool calls still reads correctly.
| private findBestActiveTodo(todos: readonly Todo[]): Todo | undefined { | ||
| // Priority 1: Find in_progress todos (should be max 1) | ||
| // First: Find in_progress todos (should be max 1) | ||
| const inProgressTodos = todos.filter( | ||
| (todo) => todo.status === 'in_progress', | ||
| ); | ||
| if (inProgressTodos.length > 0) { | ||
| return inProgressTodos[0]; | ||
| } | ||
|
|
||
| // Priority 2: Find pending todos, prioritize by priority | ||
| // Second: Find pending todos, sort alphabetically by content | ||
| const pendingTodos = todos.filter((todo) => todo.status === 'pending'); | ||
| if (pendingTodos.length > 0) { | ||
| // Sort by priority: high > medium > low | ||
| const priorityOrder: Record<string, number> = { | ||
| high: 3, | ||
| medium: 2, | ||
| low: 1, | ||
| }; | ||
| pendingTodos.sort((a, b) => { | ||
| const aPriority = priorityOrder[a.priority || 'medium'] || 2; | ||
| const bPriority = priorityOrder[b.priority || 'medium'] || 2; | ||
| return bPriority - aPriority; // Descending order (high to low) | ||
| }); | ||
| // Sort alphabetically by content | ||
| pendingTodos.sort((a, b) => a.content.localeCompare(b.content)); | ||
| return pendingTodos[0]; |
There was a problem hiding this comment.
Guard alphabetical sort against missing content.
localeCompare will throw if content is undefined; malformed or user-edited todo files could crash continuation. Consider a null-safe fallback when sorting.
🛠️ Proposed fix
- pendingTodos.sort((a, b) => a.content.localeCompare(b.content));
+ pendingTodos.sort(
+ (a, b) => (a.content ?? '').localeCompare(b.content ?? ''),
+ );📝 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.
| private findBestActiveTodo(todos: readonly Todo[]): Todo | undefined { | |
| // Priority 1: Find in_progress todos (should be max 1) | |
| // First: Find in_progress todos (should be max 1) | |
| const inProgressTodos = todos.filter( | |
| (todo) => todo.status === 'in_progress', | |
| ); | |
| if (inProgressTodos.length > 0) { | |
| return inProgressTodos[0]; | |
| } | |
| // Priority 2: Find pending todos, prioritize by priority | |
| // Second: Find pending todos, sort alphabetically by content | |
| const pendingTodos = todos.filter((todo) => todo.status === 'pending'); | |
| if (pendingTodos.length > 0) { | |
| // Sort by priority: high > medium > low | |
| const priorityOrder: Record<string, number> = { | |
| high: 3, | |
| medium: 2, | |
| low: 1, | |
| }; | |
| pendingTodos.sort((a, b) => { | |
| const aPriority = priorityOrder[a.priority || 'medium'] || 2; | |
| const bPriority = priorityOrder[b.priority || 'medium'] || 2; | |
| return bPriority - aPriority; // Descending order (high to low) | |
| }); | |
| // Sort alphabetically by content | |
| pendingTodos.sort((a, b) => a.content.localeCompare(b.content)); | |
| return pendingTodos[0]; | |
| private findBestActiveTodo(todos: readonly Todo[]): Todo | undefined { | |
| // First: Find in_progress todos (should be max 1) | |
| const inProgressTodos = todos.filter( | |
| (todo) => todo.status === 'in_progress', | |
| ); | |
| if (inProgressTodos.length > 0) { | |
| return inProgressTodos[0]; | |
| } | |
| // Second: Find pending todos, sort alphabetically by content | |
| const pendingTodos = todos.filter((todo) => todo.status === 'pending'); | |
| if (pendingTodos.length > 0) { | |
| // Sort alphabetically by content | |
| pendingTodos.sort( | |
| (a, b) => (a.content ?? '').localeCompare(b.content ?? ''), | |
| ); | |
| return pendingTodos[0]; |
🤖 Prompt for AI Agents
In `@packages/cli/src/services/todo-continuation/todoContinuationService.ts`
around lines 536 - 550, The alphabetical sort in findBestActiveTodo can throw
when todo.content is undefined; update the pendingTodos.sort call to compare
content in a null-safe way (e.g., use a default empty string or other fallback
for a.content and b.content before calling localeCompare) so malformed or
user-edited todos don't crash the continuation; ensure the comparator references
todo.content fallback consistently and keeps the same sort semantics for normal
entries.
| const STATUS_ICONS: Record<Todo['status'], string> = { | ||
| in_progress: '→', | ||
| pending: '○', | ||
| completed: '✔', | ||
| }; | ||
|
|
||
| const PRIORITY_LABELS: Record<Todo['priority'], string> = { | ||
| high: 'HIGH', | ||
| medium: 'MEDIUM', | ||
| low: 'LOW', | ||
| completed: '', | ||
| }; |
There was a problem hiding this comment.
Avoid leading space when completed icon is empty.
With completed: '', the formatter now emits a leading space before content. A conditional prefix keeps output aligned without stray whitespace.
🛠️ Proposed fix
- return `${marker} ${todo.content}${currentSuffix}`;
+ const prefix = marker ? `${marker} ` : '';
+ return `${prefix}${todo.content}${currentSuffix}`;Also applies to: 139-143
🤖 Prompt for AI Agents
In `@packages/core/src/todo/todoFormatter.ts` around lines 21 - 25, The
STATUS_ICONS map currently sets completed: '' which causes the formatter to
still emit a space prefix even when the icon is empty; update the formatting
logic that uses STATUS_ICONS (look for the function that builds the line/label
for a Todo using STATUS_ICONS and Todo['status']) to only prepend the icon plus
a trailing space when the icon string is non-empty (i.e., conditionally add
`${icon} ` or nothing), ensuring alignment is preserved for other statuses while
avoiding a stray leading space for completed items.
| expect(result.llmContent).toContain('→'); // in_progress | ||
| expect(result.llmContent).toContain('○'); // pending | ||
| expect(result.llmContent).toContain(''); // completed |
There was a problem hiding this comment.
Avoid vacuous assertion for completed status.
toContain('') always passes and doesn’t validate output. Assert the completed line or marker explicitly.
✅ Suggested test fix
- expect(result.llmContent).toContain(''); // completed
+ expect(result.llmContent).toContain(' Completed task'); // completed (no icon)📝 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.
| expect(result.llmContent).toContain('→'); // in_progress | |
| expect(result.llmContent).toContain('○'); // pending | |
| expect(result.llmContent).toContain(''); // completed | |
| expect(result.llmContent).toContain('→'); // in_progress | |
| expect(result.llmContent).toContain('○'); // pending | |
| expect(result.llmContent).toContain(' Completed task'); // completed (no icon) |
🤖 Prompt for AI Agents
In `@packages/core/src/tools/todo-read.test.ts` around lines 159 - 161, The test
currently uses a vacuous assertion expect(result.llmContent).toContain('') which
always passes; replace it with an explicit check for the completed-task marker
or completed task line used by the todo renderer. Inspect how completed items
are rendered in the codebase (the todo formatter/parser used by the test) to
find the exact symbol or text for completed tasks, then change the assertion to
something like expect(result.llmContent).toContain('<completed-marker>') or use
a regex/line match (e.g., expect(result.llmContent).toMatch(/completed task text
or marker/)) referencing result.llmContent in this test so the completed state
is actually validated instead of asserting an empty string.
|
@CodeRabbit explain the alphabetical sorting? You mean it changes the numeric task order and sorts them alphabetically or? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5947 🏁 Script executed: Length of output: 3844 🏁 Script executed: Length of output: 16490
Here's how it works now: Two-tier sorting:
Looking at the code in const orderTodos = (todos: Todo[]): Todo[] =>
[...todos].sort((a, b) => {
const statusDiff = STATUS_ORDER[a.status] - STATUS_ORDER[b.status];
if (statusDiff !== 0) {
return statusDiff;
}
// Secondary sort: alphabetical by content
return a.content.localeCompare(b.content);
});Example: If you have these todos:
They'll be displayed as:
Previously: Within the same status, todos were ordered by priority (high → medium → low) This same alphabetical logic is used when selecting which task to continue in |
|
@CodeRabbit -- expalin so before they showed in like numeric order and which as being done -- or at least I thought they did -- so if it adds 5 items to a todo they are now sorted alphabetically? with only the in progress at the top? |
|
Tip For best results, initiate chat on the files or code changes.
Before this PR:
After this PR:
Concrete Example:If you add 5 items to a todo: Display order will be: Key point: All The status-based grouping is preserved—you'll never see a pending task above an in_progress task! |
Remove alphabetical content sorting from todo ordering - todos now maintain the order the LLM added them within each status group (in_progress first, then pending, then completed). This ensures when an LLM creates a task list with a specific order, the first pending task becomes the next task to work on, not whichever one happens to be first alphabetically.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/tools/todo-write.ts`:
- Around line 248-263: determineNextAction currently preserves the input todos
order when selecting the first in-progress or pending task, but the PR expects
alphabetical ordering within each status group; update the selection logic in
determineNextAction to sort the filtered arrays (inProgressTasks and
pendingTasks) by task.content (case-insensitive) before taking the first
element, ensuring the same alphabetical tie-breaker used elsewhere (e.g.,
formatter/continuation) so next-task choice is consistent across components.
♻️ Duplicate comments (1)
packages/core/src/todo/todoFormatter.ts (1)
139-142: Avoid leading space when completed icon is empty.
Completed items still render with a leading space because the marker is''and the formatter always prefixes"${marker} ". This was flagged earlier; consider the same conditional-prefix fix.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/todo/todoFormatter.tspackages/core/src/tools/todo-read.tspackages/core/src/tools/todo-write.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tools/todo-read.ts
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
🔇 Additional comments (2)
packages/core/src/tools/todo-write.ts (1)
96-96: Required fields update looks correct.This aligns with removing priority from the schema.
packages/core/src/todo/todoFormatter.ts (1)
210-212: Confirm the intended secondary ordering within each status group.
The comparator now only uses status, so ordering within a status group follows original input order. The PR objectives mention alphabetical secondary ordering, while commit notes mention preserving LLM order. Please confirm the intended behavior and align docs/tests accordingly. If alphabetical is still desired, a secondary comparator will be needed.Optional change if alphabetical ordering is intended
-const orderTodos = (todos: Todo[]): Todo[] => - // Sort by status only, preserving original array order within each status group - [...todos].sort((a, b) => STATUS_ORDER[a.status] - STATUS_ORDER[b.status]); +const orderTodos = (todos: Todo[]): Todo[] => + [...todos].sort((a, b) => { + const statusDiff = STATUS_ORDER[a.status] - STATUS_ORDER[b.status]; + if (statusDiff !== 0) return statusDiff; + return a.content.localeCompare(b.content); + });
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Check if any tasks are in progress - return first one (preserves LLM order) | ||
| const inProgressTasks = todos.filter((t) => t.status === 'in_progress'); | ||
| if (inProgressTasks.length > 0) { | ||
| // Continue with highest priority in-progress task | ||
| const task = inProgressTasks.sort((a, b) => { | ||
| const priorityOrder = { high: 0, medium: 1, low: 2 }; | ||
| return priorityOrder[a.priority] - priorityOrder[b.priority]; | ||
| })[0]; | ||
| const task = inProgressTasks[0]; | ||
| return { | ||
| type: 'continue', | ||
| taskId: task.id, | ||
| taskContent: task.content, | ||
| }; | ||
| } | ||
|
|
||
| // Check if any tasks are pending | ||
| // Check if any tasks are pending - return first one (preserves LLM order) | ||
| const pendingTasks = todos.filter((t) => t.status === 'pending'); | ||
| if (pendingTasks.length > 0) { | ||
| // Start with highest priority pending task | ||
| const task = pendingTasks.sort((a, b) => { | ||
| const priorityOrder = { high: 0, medium: 1, low: 2 }; | ||
| return priorityOrder[a.priority] - priorityOrder[b.priority]; | ||
| })[0]; | ||
| const task = pendingTasks[0]; | ||
| return { |
There was a problem hiding this comment.
Align next-action ordering with the system-wide policy.
determineNextAction now preserves list order, but the PR objectives/summary describe alphabetical ordering within status groups. If other components (e.g., todo continuation or formatter) are using alphabetical ordering, this creates inconsistent “next task” selection. Please confirm the intended behavior and align all paths accordingly.
🛠️ If alphabetical ordering is intended, one possible fix
- const inProgressTasks = todos.filter((t) => t.status === 'in_progress');
+ const inProgressTasks = todos.filter((t) => t.status === 'in_progress');
if (inProgressTasks.length > 0) {
- const task = inProgressTasks[0];
+ const task = inProgressTasks.sort((a, b) =>
+ a.content.localeCompare(b.content),
+ )[0];
return {
type: 'continue',
taskId: task.id,
taskContent: task.content,
};
}
const pendingTasks = todos.filter((t) => t.status === 'pending');
if (pendingTasks.length > 0) {
- const task = pendingTasks[0];
+ const task = pendingTasks.sort((a, b) =>
+ a.content.localeCompare(b.content),
+ )[0];
return {
type: 'start',
taskId: task.id,
taskContent: task.content,
};
}📝 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.
| // Check if any tasks are in progress - return first one (preserves LLM order) | |
| const inProgressTasks = todos.filter((t) => t.status === 'in_progress'); | |
| if (inProgressTasks.length > 0) { | |
| // Continue with highest priority in-progress task | |
| const task = inProgressTasks.sort((a, b) => { | |
| const priorityOrder = { high: 0, medium: 1, low: 2 }; | |
| return priorityOrder[a.priority] - priorityOrder[b.priority]; | |
| })[0]; | |
| const task = inProgressTasks[0]; | |
| return { | |
| type: 'continue', | |
| taskId: task.id, | |
| taskContent: task.content, | |
| }; | |
| } | |
| // Check if any tasks are pending | |
| // Check if any tasks are pending - return first one (preserves LLM order) | |
| const pendingTasks = todos.filter((t) => t.status === 'pending'); | |
| if (pendingTasks.length > 0) { | |
| // Start with highest priority pending task | |
| const task = pendingTasks.sort((a, b) => { | |
| const priorityOrder = { high: 0, medium: 1, low: 2 }; | |
| return priorityOrder[a.priority] - priorityOrder[b.priority]; | |
| })[0]; | |
| const task = pendingTasks[0]; | |
| return { | |
| // Check if any tasks are in progress - return first one (preserves LLM order) | |
| const inProgressTasks = todos.filter((t) => t.status === 'in_progress'); | |
| if (inProgressTasks.length > 0) { | |
| const task = inProgressTasks.sort((a, b) => | |
| a.content.localeCompare(b.content), | |
| )[0]; | |
| return { | |
| type: 'continue', | |
| taskId: task.id, | |
| taskContent: task.content, | |
| }; | |
| } | |
| // Check if any tasks are pending - return first one (preserves LLM order) | |
| const pendingTasks = todos.filter((t) => t.status === 'pending'); | |
| if (pendingTasks.length > 0) { | |
| const task = pendingTasks.sort((a, b) => | |
| a.content.localeCompare(b.content), | |
| )[0]; | |
| return { |
🤖 Prompt for AI Agents
In `@packages/core/src/tools/todo-write.ts` around lines 248 - 263,
determineNextAction currently preserves the input todos order when selecting the
first in-progress or pending task, but the PR expects alphabetical ordering
within each status group; update the selection logic in determineNextAction to
sort the filtered arrays (inProgressTasks and pendingTasks) by task.content
(case-insensitive) before taking the first element, ensuring the same
alphabetical tie-breaker used elsewhere (e.g., formatter/continuation) so
next-task choice is consistent across components.
Windows CI Flaky Test NoteThe Windows test (windows-latest, 24.x) continues to fail with exit code 1 despite all tests passing: ``` This appears to be a known flaky infrastructure issue unrelated to the code changes in this PR. All actual tests pass successfully on all platforms (Linux, macOS, Windows). The Windows job has been re-run multiple times with the same result. Please consider manually merging or re-running the workflow. |
TLDR
Remove the
priorityfield from the todo system to simplify the interface. The priority field served no meaningful purpose - it was not displayed in the UI panel, not used in continuation logic, and only affected secondary sort order in the text formatter.Key Changes:
TodoPriorityenum andpriorityfield from schemasDive Deeper
Why remove priority?
The issue (#1165) identified that priorities serve no purpose for todo_write/todo_read:
[HIGH],[MEDIUM],[LOW]) were displayed in the formatter but provided no actionable valueBackward Compatibility
Zod's default parsing behavior strips unknown keys, so existing todo files with priority fields will load without errors - the field will simply be ignored. No migration is needed.
Sorting Behavior
After this change, todos are sorted by:
in_progress>pending>completedThis maintains deterministic ordering while simplifying the interface.
Files Modified
Core Implementation:
packages/core/src/tools/todo-schemas.ts- Removed TodoPriority enum and priority fieldpackages/core/src/tools/todo-write.ts- Removed priority from schema and statisticspackages/core/src/tools/todo-read.ts- Removed priority from description and statisticspackages/core/src/todo/todoFormatter.ts- Removed priority labels and sortingpackages/core/src/parsers/TextToolCallParser.ts- Removed priority normalizationpackages/core/src/services/todo-reminder-service.ts- Removed priority referencepackages/core/src/core/client.ts- Removed priority from todo normalizationCLI Integration:
packages/cli/src/services/todo-continuation/todoContinuationService.ts- Updated commentspackages/cli/src/zed-integration/schema.ts- Removed priority from Zed ACP schemapackages/cli/src/zed-integration/zedIntegration.ts- Removed priority assignmentTests (27 files):
Documentation:
docs/todo-system.md- Removed priority from parameter documentationReviewer Test Plan
Pull and build the branch:
```bash
git checkout issue1165
npm install
npm run build
```
Run the test suite:
```bash
npm run test
npm run lint
npm run typecheck
```
Manual verification:
```bash
node scripts/start.js
In the chat, try creating a todo list:
"Create a todo list with 3 tasks for building a web app"
Verify the todos display correctly without priority badges
```
Check backward compatibility - If you have existing todo files in `~/.llxprt/todos/`, they should still load correctly
Testing Matrix
Linked issues / bugs
Closes #1165