Make automation chat accountable by default - #2790
Conversation
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. |
![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: 7fc7448d22
ℹ️ 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 (dto.RequestProposal && LooksLikeChecklistBootstrapRequest(dto.Content)) | ||
| if (LooksLikeChecklistBootstrapRequest(actionAttemptContent)) |
There was a problem hiding this comment.
When a board-scoped user merely asks about or pastes a checklist, such as Can you review this checklist?\n- [ ] Ship release, this unconditional fast path bypasses both turnRequestsAction and the provider and immediately persists a checklist-bootstrap proposal. This makes conversational checklist content create unwanted Review items; require an actual action request before entering the bootstrap path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triage: valid P2 conversational-classification edge case. It can create an unwanted Pending Review proposal, but cannot mutate a board without the separate approval and Apply steps. No realistic direct HIGH path from this diff; leave as a #2004 acceptance residual rather than expand this bounded fix round.
There was a problem hiding this comment.
Correction to my earlier disposition: confirmed core ADR-0069 correctness defect and fixed in cdfd28c. Checklist bootstrap now rejects a leading conversational question unless the turn independently requests action; regression covers the reported checklist question and proves no proposal is created.
| var hasProposalIntent = llmResult.IsActionable || turnRequestsAction; | ||
| var shouldAttemptProposal = !llmResult.IsDegraded && hasProposalIntent; |
There was a problem hiding this comment.
When a user asks an informational Taskdeck question such as How do I create a card called 'Release'?, the local classifier marks it as card.create, and the extractor turns the quoted text into a valid create-card instruction. Because turnRequestsAction now drives hasProposalIntent even when the provider returned ordinary non-actionable prose, this branch creates a proposal for a question that should remain conversational; suppress command-like matches in informational questions before attempting proposal creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triage: valid P2 classifier false-positive. The result is a reviewable proposal only; Review, Approve, and Apply still gate any board change. This does not meet the HIGH bar and is retained as a #2004 acceptance residual.
There was a problem hiding this comment.
Correction to my earlier disposition: confirmed core ADR-0069 correctness defect and fixed in cdfd28c. Informational Taskdeck questions now classify as conversational before action extraction; focused classifier regression covers the reported trigger.
| else if (turnRequestsAction) | ||
| { | ||
| messageType = "action-no-proposal"; | ||
| assistantContent = AppendNoProposalActionNotice(assistantContent); |
There was a problem hiding this comment.
When the local classifier misses a natural-language action but the LLM invokes a propose_* tool that returns an error payload—for example, because a label or target column is invalid—the orchestrator can finish non-degraded with tool calls but no ProposalId. Since turnRequestsAction is false, this branch persists ordinary text without the server-derived notice that no proposal was created, restoring the silent-action ambiguity this change is intended to eliminate; treat a proposal-tool attempt with no receipt as an action-no-proposal outcome while excluding read-only tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triage: valid P2 outcome-label gap when a provider chooses a write tool after the local classifier misses intent. The failed tool creates no proposal and no board mutation, so this is non-blocking under the bounded review rule; retain for #2004 acceptance follow-up.
There was a problem hiding this comment.
Correction to my earlier disposition: confirmed accountable-outcome defect and fixed in cdfd28c. A logged propose_* tool attempt without a receipt now persists action-no-proposal. Focused regression reproduces a classifier miss plus failed propose_create_card executor.
| content, | ||
| requestProposal: requestProposal.value, | ||
| }) | ||
| await chatApi.sendMessage(sessionId, { content }) |
There was a problem hiding this comment.
When the continuation POST succeeds and creates a proposal but the subsequent session GET fails, the returned ChatMessage is discarded and the selected session still ends with the old action-needs-board message. After sendingMessage resets, the continuation button becomes active again, so another click resends the retained instruction and can create a duplicate proposal; apply the successful POST response locally or otherwise mark the recovery action consumed before attempting the refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triage: valid P2 recovery edge case: a successful POST followed by a failed refresh can allow a second explicit continuation and duplicate Pending Review proposals. It does not create or change a board and remains behind Review, Approve, and Apply. Retain as a #2004 acceptance residual; no HIGH fix batch.
There was a problem hiding this comment.
Correction to my earlier disposition: confirmed duplicate-proposal path and fixed in cdfd28c. The successful continuation response is applied locally before refresh, so a failed GET cannot leave the old recovery action active. The composable regression proves a second continuation is not sent.
| <template v-else-if="eligibleBoards.length === 0"> | ||
| <p class="td-board-recovery__copy"> | ||
| There are no active boards you can edit. Create a board or ask an owner for edit access, then reload boards. |
There was a problem hiding this comment.
When the initial board-list request fails, loadBoardOptions leaves eligibleBoards empty after showing only a toast, so this branch then tells the user that no editable boards exist and directs them to create one or request access. That is false for a transient API failure and provides no inline retry path; retain and render a distinct board-load error state instead of falling through to the genuine empty state.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triage: valid P2 presentation/retry gap after the editable-board list fails. It neither selects a board nor resends an instruction, so it is non-blocking. Retain for #2004 acceptance follow-up.
There was a problem hiding this comment.
Triage remains P2: the board-list request failure falls through to a false empty-state explanation, but it does not violate the action receipt, board-binding, or stale-session correctness contracts. It is intentionally outside this one high-priority repair batch.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent full-stack review at 1434972d23b01629f6087aec4c9b277fc42cb3b5 against 0cebd938d79f045a20ce99bff495b986c25cf267: no CRITICAL or HIGH findings.
Confirmed the ADR-0069 seams reviewed here: owned-session board binding with CAS/idempotency, one persisted clarification round and explicit continuation, session-selection guards for late frontend responses, truthful persisted streaming/no-proposal outcomes, receipt preservation, and actual existing-card grounding before proposal creation. A distinct auth/CAS review also found no blocker.
The five connector P2 findings were triaged on-thread as #2004 acceptance residuals. They can affect proposal classification, duplicate review proposals after a failed refresh, or board-picker messaging, but do not bypass Review -> Approve -> Apply or directly mutate a board.
Validation in this review: supplied exact diff and head/base confirmed; git diff --check passed. No focused tests were necessary because no fix was warranted. Full backend/frontend, browser/mock, and live-provider runs remain outside this review lease and are not claimed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1434972d23
ℹ️ 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".
| for (let index = messages.length - 1; index >= 0; index--) { | ||
| const message = messages[index]! | ||
| if (normalizeChatRole(message.role) !== 'Assistant') continue | ||
| if (message.messageType !== 'action-needs-board') return null |
There was a problem hiding this comment.
When an unbound actionable turn receives a provider clarification, ChatService keeps the message type as clarification while appending text that says “Select a writable board below” (ChatService.cs:576-577). This predicate returns null for every assistant type except action-needs-board, so ChatMessageList receives no pending message ID and renders no picker. The user is therefore directed to a control that does not exist; either include these clarification outcomes in board recovery or use notice text that does not promise an inline picker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cdfd28c. The recovery selector now recognizes an unbound clarification that contains the server-derived inline-picker instruction, preserving the clarification type while making the promised picker available. Focused composable regression passes.
| if (isDisposed || selectionGeneration !== sessionSelectionGeneration) return | ||
| toast.error(getErrorDisplay(e, 'Failed to load chat session').message) |
There was a problem hiding this comment.
When session A is displayed and loading session B fails, requestedSessionId remains B while selectedSession remains A. A subsequent send from the still-visible A session succeeds server-side, but the guard at lines 318–320 skips refreshing the history and clearing the draft because the IDs differ, making the message appear unsent and easy to submit twice; board-binding responses are similarly ignored. Restore requestedSessionId to the displayed session or expose a selection error state in this catch.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cdfd28c. When loading B fails while A remains displayed, requestedSessionId is restored to A so A's successful send can clear its draft and refresh its transcript. Focused composable regression passes.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Correction to my earlier review: reproducing the connector triggers showed six core accountable-chat correctness defects, so I fixed them in cdfd28c31.
The batch keeps ordinary Taskdeck questions conversational, makes failed propose_* attempts persist action-no-proposal, consumes a successful continuation before a refresh can fail, renders the picker promised by an unbound actionable clarification, restores the visible session after a failed session switch, and orders reload transcripts by causal creation time. It also fixes the old-head hosted API failure: the API DTO had exposed an unordered EF Include collection, while the test correctly required the persisted outcome as the final message.
Focused validation: API reload 1/1; Application 4/4; chat composable 29/29; frontend typecheck; git diff --check. The board-list false-empty connector report remains P2 and is intentionally not expanded into this batch. Required CI for cdfd28c31 is pending; no full suite, browser, or live-provider run is claimed here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdfd28c31f
ℹ️ 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".
| toolChatMessages, | ||
| Attribution: BuildAttribution(session, userId), | ||
| SystemPrompt: ToolCallingSystemPrompt.Prompt); | ||
| SystemPrompt: ToolCallingSystemPrompt.Prompt + clarificationPrompt); |
There was a problem hiding this comment.
When tool calling is enabled for a board-bound session and an ambiguous actionable request makes the provider follow this new prompt by returning clarifying questions without a tool call, the reuse path stamps that prose IsActionable from the original request. The later clarification predicate only inspects prose when IsActionable is false, so the service skips the allowed clarification round and immediately attempts a guessed proposal instead of waiting for the user's answer; preserve the provider's clarification outcome or detect the questions before overriding actionability.
Useful? React with 👍 / 👎.
| if (normalizeChatRole(userMessage.role) === 'User') { | ||
| return { | ||
| messageId: message.id, | ||
| instruction: userMessage.content, |
There was a problem hiding this comment.
When an unbound actionable turn receives a clarification and the user answers it before linking a board, the resulting action-needs-board message is paired here only with that short answer, not the original instruction plus answer. After binding, Continue resends only the answer; because the intervening board-needed message means the backend no longer sees a pending clarification, the default mock/local fallback treats input such as Backlog as non-actionable and creates no proposal. Preserve the reconstructed clarification context for this recovery path.
Useful? React with 👍 / 👎.
| if (sessionIndex >= 0) sessions.value.splice(sessionIndex, 1, bound) | ||
|
|
||
| if (requestedSessionId === sessionId && selectedSession.value?.id === sessionId) { | ||
| selectedSession.value = bound |
There was a problem hiding this comment.
When a user clicks Link board and sends from the composer before the bind request returns, the operations can overlap because neither control disables the other. If the send and its refresh finish before the slower bind response, assigning the bind response wholesale restores the older session snapshot loaded at the start of binding, hiding the new assistant response and potentially exposing the previous continuation control again. Merge only the binding state, refresh after binding, or mutually disable binding and sending.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| const pending = pendingBoardRecovery.value | ||
| const session = selectedSession.value | ||
| if (!pending || pending.messageId !== messageId || !session?.boardId) return | ||
| await sendMessageToSession(pending.instruction) |
There was a problem hiding this comment.
When the recovery card is visible, a user can type a separate compose draft and then click Continue retained instruction. This passes pending.instruction to the generic sender, whose success path unconditionally clears messageContent, silently discarding the unrelated unsent draft. Clear the composer only when its current contents were the message actually submitted.
Useful? React with 👍 / 👎.
|
Parked at cdfd28c under the one-review/one-repair ceiling. Confirmed HIGH #2795: a failed proposal tool followed by provider timeout can fall back to ordinary text without the required no-proposal receipt; coordinator probes failed both message-type and receipt-content assertions. No further repair round or release approval is inferred. #2004 remains incomplete. |
|
Current-head Windows API failure is tracked separately as #2798: unchanged CaptureApiTests Processing-state assertion races the live triage worker. The deterministic test-isolation repair is a separate test-only lane. #2790 remains parked for the confirmed receipt defect #2795; no extra chat repair is authorized by this diagnosis. |
1 participant
Parked: remaining HIGH blocker #2795
The review ceiling has been reached. A failed write-tool attempt followed by degraded provider fallback can still return ordinary text without a no-proposal receipt. Direct synthetic tests reproduce the defect at cdfd28c. The implementation below is not accepted as complete; #2004 remains open. A further repair pass requires explicit re-scoping.
Accountable chat now turns actionable instructions into reviewable proposals by default while keeping every board mutation behind Review → Approve → Apply. An unbound session keeps the original instruction, lets the user link an existing writable board inline, and continues only after an explicit click; ordinary questions remain conversational.
Behavior
POST /api/llm/chat/sessions/{id}/boardto bind the existing owned session. Missing and foreign sessions share the same 404 response; same-board binding is idempotent and rechecks write access plus archive state; a different board returns 409.action-needs-boardandaction-no-proposaloutcomes, including honest terminal suffixes for buffered and streamed failures. Board-creation requests direct the user to Boards instead of showing an invalid picker.Authorization and state cases
Verification
dotnet test backend/Taskdeck.sln -c Release -m:1— exercised all projects: 8,992 passed, 5 skipped, 1 failed because an existing live-provider-stub assertion still expected genericstatus; no production failure.dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --no-restoreafter correcting that assertion — 2,862 passed, 4 skipped, 0 failed. Together these runs cover the corrected solution-equivalent total of 8,993 passed and 5 skipped; this is not represented as one green full-solution invocation.cardIdare the actual persisted card ID, and whose requested title appears in the Review diff while the card title and description remain unchanged.npm run lint— exit 0 with 8 pre-existing warnings.npm run typecheck— exit 0.npm run build— exit 0.npx vitest run --maxWorkers=2— 6,144 passed, 3 skipped, 5 unrelated due-date failures when the run crossed midnight BST while UTC remained on the prior date. Immediate default-TZ rerun reproduced those five; rerunning the three affected files withTZ=UTCpassed 102/102. Changed chat files passed 83/83 after the final base merge.5021, frontend4181, one worker, and a fresh worktree-local SQLite database: SC-005 passed 1/1 in 13.6s. It verified same-session binding, no automatic resend, explicit continuation, proposal visibility in Review, and no card before Apply. A full-page Review screenshot and trace were retained locally.Not verified
No database migration or new provider/budget path is introduced. Existing source, proposal, and history contracts remain additive-compatible.
Refs #2004