test(todo-sync): match required priority fallback#2832
test(todo-sync): match required priority fallback#2832code-yeongyu merged 2 commits intocode-yeongyu:devfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@code-yeongyu Small separate fix for the current red base-branch CI. Root cause was that syncTaskToTodo() always injected a medium priority even though TodoInfo.priority is optional and the existing tests expect missing/invalid task priorities to stay undefined. I reproduced the same failure on clean upstream/dev before opening this, so this is intentionally split from #2829. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 2/5
- High-risk regression: in
src/tools/task/todo-sync.ts, removing thepriorityfallback conflicts with Opencode’s non-null requirement and can trigger aNOT NULL constraint failedSQLite error during task sync. - This is concrete and high-confidence user impact (severity 9/10, confidence 10/10), so merge risk is elevated rather than a minor follow-up fix.
- Pay close attention to
src/tools/task/todo-sync.ts- ensurepriorityis always populated with a non-null string before persistence/synchronization.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/task/todo-sync.ts">
<violation number="1" location="src/tools/task/todo-sync.ts:68">
P0: Custom agent: **Opencode Compatibility**
Opencode strictly requires a non-null string for `priority`. Removing this fallback will crash task synchronization with a `NOT NULL constraint failed` SQLite exception whenever a task lacks priority metadata.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@code-yeongyu Quick correction on #2832: cubic found a valid issue in my first version. After re-checking against the OpenCode SDK types, Todo.priority is treated as required, so dropping the fallback was the wrong direction. I pushed 46c6e1d to restore the medium fallback and changed the three failing tests instead. Fresh verification: bun test src/tools/task/todo-sync.test.ts src/hooks/todo-continuation-enforcer --bail, bunx tsc --noEmit, bun run build. |
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 3/5
- There is a concrete regression risk:
src/tools/task/todo-sync.test.tsnow enforces a default"medium"priority when missing, which conflicts with the PR goal to preserve missing priority values. - Given the issue’s medium severity (6/10) and high confidence (9/10), this is more than a minor mismatch and could change user-visible sync behavior if merged as-is.
- Pay close attention to
src/tools/task/todo-sync.test.ts- test expectations currently appear misaligned with the intended missing-priority behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/task/todo-sync.test.ts">
<violation number="1" location="src/tools/task/todo-sync.test.ts:30">
P2: Tests were changed to require a `"medium"` default for missing priority, contradicting the PR’s stated goal to preserve missing priority.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| content: "Fix bug", | ||
| status: "pending", | ||
| priority: undefined, | ||
| priority: "medium", |
There was a problem hiding this comment.
P2: Tests were changed to require a "medium" default for missing priority, contradicting the PR’s stated goal to preserve missing priority.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tools/task/todo-sync.test.ts, line 30:
<comment>Tests were changed to require a `"medium"` default for missing priority, contradicting the PR’s stated goal to preserve missing priority.</comment>
<file context>
@@ -27,7 +27,7 @@ describe("syncTaskToTodo", () => {
content: "Fix bug",
status: "pending",
- priority: undefined,
+ priority: "medium",
});
});
</file context>
There was a problem hiding this comment.
cubic found a real mismatch here, but it was in the PR narrative rather than in the current runtime behavior. After the previous correction, the product code intentionally keeps priority: extractPriority(task.metadata) ?? "medium" because @opencode-ai/sdk exposes Todo.priority as a required string and current OmO readers assume a non-null priority. The tests now match that current contract. I updated the PR title/body to reflect the actual scope (test(todo-sync): match required priority fallback) so it no longer claims that missing priorities should stay undefined. Fresh verification on 46c6e1dc: bun test src/tools/task/todo-sync.test.ts src/hooks/todo-continuation-enforcer --bail (100 pass), bun test src/hooks/atlas --bail (91 pass), bunx tsc --noEmit, bun run build. @cubic-dev-ai please re-review.
There was a problem hiding this comment.
@RaviTharuma I have started the AI code review. It will take a few minutes to complete.
|
@code-yeongyu cubic's review point is fixed on the current PR head
So the todo-sync change itself is clean on the current head. The remaining red GitHub Could you re-run the failed CI job once from the maintainer side? If it reproduces after a clean re-run, I'll split that Atlas runner-specific failure into a separate follow-up immediately. |
RaviTharuma
changed the title
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: This PR only updates test expectations to match existing runtime behavior, ensuring CI passes without modifying application logic.
2 participants
Summary
"medium"fallback insyncTaskToTodo()when task metadata has no valid prioritytodo-syncexpectations so tests match the real OpenCode/OmO contract#2829Verification
bun test src/tools/task/todo-sync.test.ts src/hooks/todo-continuation-enforcer --bailbun test src/hooks/atlas --bailbunx tsc --noEmitbun run buildContext
This stays intentionally separate from
#2829. The original failingtodo-synctest was reproducible on cleanupstream/dev, but the first attempt on this PR went in the wrong direction by removing the runtime priority fallback. The current PR head restores the required fallback and aligns the tests with the actual runtime contract instead.