Fixed in 8b5ba1a — see the reply to mafredri's DEREM-2. Both handlers now share a helper that uses httpapi.Is404Error and returns 500 (with Detail) for non-404 errors.

Acknowledged but intentionally not changing in this PR. The double-Authorize pattern is what watchChatDesktop has shipped with for a while; introducing a new multi-action authz primitive (or open-coding the underlying rbac.Authorizer.Authorize call) here would diverge from the surrounding handlers and add API surface for what is, in the worst case, a single extra WARN log line per allowed-via-one-action request. With the new shared authorizeChatWorkspaceExec helper, if/when the broader codebase grows a "require any of N actions" helper we can swap it in one place. Happy to file a follow-up if you'd like.

Description fixed in an earlier revision and explained in the reply to mafredri's DEREM-3.

P2 [DEREM-2] GetWorkspaceByID returns 400 "Chat workspace not found" for all errors, including database timeouts, connection failures, and context cancellations. Only sql.ErrNoRows and dbauthz denials are "not found" situations; infrastructure failures are server errors.

The same file already has the correct pattern at lines 248, 2627, 3200, and 4841:

if httpapi.Is404Error(err) {
    httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
        Message: "Chat workspace not found.",
    })
    return
}
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
    Message: "Internal error fetching chat workspace.",
    Detail:  err.Error(),
})
return

Consequence: a database outage surfaces as "workspace not found" with no log entry and no 5xx for monitoring dashboards. An operator investigating sees a 400 and chases a phantom deleted workspace instead of paging the DBA.

The sibling watchChatDesktop at line 1766 has the identical gap. Since the stated goal is "stay in lock-step," both should get the fix. (Chopper P2, Ryosuke P2, Knov P3, Leorio P3, Mafuuu P3, Meruem P3, Zoro P4)

🤖

Good catch — fixed in 8b5ba1a. Both watchChatGit and watchChatDesktop now share an authorizeChatWorkspaceExec helper that distinguishes 404-class errors (httpapi.Is404Error) from other failures, returning 400 and 500 respectively (with the Detail field for operators on the 500 path). The shared helper also addresses DEREM-5 — now there's only one block to keep correct.

P2 [DEREM-3] The PR description states: "Adds real-coderdtest RBAC coverage for both /stream/git and /stream/desktop (OwnerWithoutSSH_Forbidden) plus a sanity case that confirms the check doesn't over-tighten (OwnerWithSSH_PassesAuthz)." None of these exist in the diff.

grep -rn 'OwnerWithoutSSH_Forbidden\|OwnerWithSSH_PassesAuthz' coderd/ returns nothing. The actual test added is TestWatchChatGitAuthz, a single function with no subtests. No desktop authz test exists (the only desktop test is TestWatchChatDesktop/NoWorkspace which tests a 400, not RBAC). No positive authorization test exists.

A reviewer reading the description and not the diff will believe both handlers have symmetric RBAC regression coverage and that an over-tightening sanity check exists. Neither is true. Fix the description to match what was delivered, or deliver what was promised. (Mafu-san P1, Kite P3, Leorio P3)

🤖

Fixed — the description was already updated earlier in the review cycle to describe the actual diff (one regression test on /stream/git, no desktop subtest, no positive sanity case). The trim happened after a reviewer asked to keep the test surface minimal; I should have updated the description before they saw the original wording. Apologies for the noise.

P3 [DEREM-1] No positive integration test verifying that a user with SSH/ApplicationConnect access can actually use the git-watch endpoint through real RBAC. The mock tests (BidirectionalProxyWorks, etc.) use mockAuthorizer which unconditionally permits, so they cannot distinguish correct policy logic from incorrect (e.g., && vs ||, or a wrong action constant). A positive integration test with a real RBAC stack would catch regressions where the authz check is accidentally over-restrictive. (Netero, Kite P3, Mafu-san P2)

🤖

Won't add in this PR. The previous revision did include OwnerWithSSH_PassesAuthz for exactly this reason, but the reviewer who shipped this fix asked to drop it as out-of-scope test surface for a 2-line authz patch. The argument is fair on both sides — happy to add it back if you want; otherwise file a follow-up that covers both stream handlers' positive RBAC paths together.

P3 [DEREM-4] All three mock subtests that reach GetWorkspaceByID (DisconnectedAgentRejected, BidirectionalProxyWorks, ClientDisconnectTearsDown) configure it to succeed. No subtest exercises GetWorkspaceByID returning an error. A workspace hard-deleted after the chat was bound would hit this path.

A subtest mocking GetWorkspaceByID returning sql.ErrNoRows and asserting 400 would cover the new branch. If the P2 above is fixed (splitting 404 from 500), a second subtest asserting 500 for non-404 errors prevents regression on the error classification. (Bisky P3, Chopper P3)

🤖

Added in 8b5ba1a as TestWatchChatGit/WorkspaceLookupErrors with NotFound (mocks sql.ErrNoRows → 400) and InternalError (mocks a generic error → 500) subtests, sharing a runWatchChatGitWorkspaceLookupTest helper. Both branches of the new helper are now covered.

P3 [DEREM-5] Lines 1596-1614 and 1764-1775 are character-for-character identical: fetch workspace by ID, check ApplicationConnect || SSH, return 403. The comment "Mirrors watchChatDesktop" documents the coupling rather than eliminating it. If the policy changes (add a third action, tighten to SSH-only, add logging), one handler will be updated and the other will drift.

With only two call sites in the same file, the blast radius is small. A shared helper like requireChatWorkspaceExec(rw, r, workspaceID) (database.Workspace, bool) would enforce lock-step by construction. The API surface is experimental and actively growing (a third stream handler would need to replicate the same block). (Meruem P3, Ryosuke P3, Zoro P3)

🤖

Done in 8b5ba1a. Extracted authorizeChatWorkspaceExec (coderd/exp_chats.go) and refactored both handlers to call it; the only per-handler difference is the "no workspace" message string, which is passed in to preserve existing wording. Now there's one block to keep correct, and the next stream handler can opt into the gate with a single line.

Nit [DEREM-6] Em-dash in comment: // user — the server refuses self-demotion. Project convention prohibits em-dashes. Use a comma or rewrite. (Mafu-san)

🤖

Fixed in 8b5ba1a. The comment is now // user, since the server refuses self-demotion.. Verified no other em-dashes in this PR's added lines.

Nit [DEREM-7] admin should be adminClient per file convention. Every other test with newChatClientWithDatabase uses adminClient (lines 425, 457). (Gon)

🤖

Fixed in 8b5ba1a — renamed admin to adminClient throughout the test and updated the surrounding comments to match.