feat: derive chat owner from token, drop /chats path#32
Conversation
The chat owner on `POST /api/experimental/chats` is always the `coder-token` holder; the API has no owner override. The action previously framed `coder-username` as picking "the Coder user the chat runs as", which is incorrect. What changed: - Add CoderClient.getAuthenticatedUser() backed by GET /api/v2/users/me. - resolveCoderUsername falls back to users/me when no input is set and the trust gate returns no-signal (schedule, workflow_dispatch with no sender or actor, custom repository_dispatch chains). The gate still refuses untrusted triggers and does not fall through to users/me. - Warn when the resolved acting user (from coder-username or github-user-id) differs from the token owner. Suppressed for auto-resolved sources. - Rewrite README "Security model" to make clear the trust gate protects the acting user (org pick, reuse label), not the chat owner. The chat owner is the token holder. - generateChatUrl and buildDeploymentChatsUrl now build /agents and /agents/<id> paths, matching the real Coder frontend routes (site/src/pages/AgentsPage/utils/navigation.ts). - Update inline docs on coder-username and github-user-id inputs and outputs to stop calling them "the chat runs as". Closes CODAGT-437 Closes CODAGT-394 Closes CODAGT-438 🤖 Authored by Coder Agents.
…-github-user-id The `acting-` prefix is the unambiguous signal that these inputs select the acting user (org pick, per-user reuse label), not the chat owner. The chat owner is the `coder-token` holder and is not overridable. What changed: - Rename input `coder-username` -> `acting-coder-username` in action.yaml. - Rename input `github-user-id` -> `acting-github-user-id` in action.yaml. - Rename output `coder-username` -> `acting-coder-username` in action.yaml. - Update getInput and setOutput calls; update OUTPUT_MAP entry. - Update error message references, core.info lines, IdentitySource literal values, and failure-comment bodies. - README inputs table, outputs table, identity resolution narrative, troubleshooting table, security model, and the doc-check recipe yaml example all renamed. - Test names and string-contains assertions on the input keys updated. - New schemas.test.ts regex assertion pins the mutex error message shape after the rename. TS-internal field names (coderUsername, githubUserID) and the chat label key (coder-agents-chat-action-user) are unchanged. 🤖 Authored by Coder Agents.
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The identity-model correction is well-reasoned: the API has no owner override, the users/me fallback is safely gated (hostile triggers throw before reaching it), the URL fix matches the real frontend routes, and the divergence warning adds observability without blocking. 271 tests pass with good positive and negative assertions. Pariston: "I tried to build a case against this change and could not."
The central issue is that the PR's own error messages and one README paragraph re-teach the exact mental model the PR exists to correct. Three user-facing strings still say "the user the chat should run as" when the whole point is that these inputs don't control who the chat runs as. Similarly, buildDeploymentChatsUrl and its surrounding names were updated in implementation but not in identity.
1 P2, 4 P3, 1 P4, 3 Nit. The P2 is the error-message/doc language contradicting the corrected ownership model. The P3s are: stale test fixture URL, function/field naming mismatch, README "runs under" language, and an untested catch path. The remaining findings are minor.
src/schemas.test.ts:331
P3 [DEREM-1] Stale /chats/ URL fixture not migrated to /agents/. Every other test file migrated chatUrl fixtures from /chats/ to /agents/. This one was missed. Change to "https://coder.test/agents/990e8400-e29b-41d4-a716-446655440000". (Netero)
Verified: grep -c 'chatUrl.*chats' src/schemas.test.ts returns 1; all other test files show 0 remaining /chats/ in URL fixtures.
🤖
src/action.ts:1067
Nit [DEREM-7] The PR added Resolved acting Coder user: '${coderUsername}' (source: ${identitySource}) at line 1054. This line (Coder username: ${coderUsername}) repeats the same value without the "acting" qualifier or the source. Should have been removed as part of the rename. (Gon)
🤖
🤖 This review was automatically generated with Coder Agents.
| throw new Error( | ||
| "Refusing to auto-resolve a GitHub identity: " + | ||
| `${trust.reason}. ` + | ||
| "Set the `coder-username` input to a Coder username, or set " + | ||
| "`github-user-id` to the GitHub numeric user id of the user " + | ||
| "the chat should run as.", | ||
| `Failed to resolve the \`coder-token\` owner via GET /api/v2/users/me: ${describeError(err)}. ` + | ||
| "Set the `acting-coder-username` input to a Coder username, or set " + | ||
| "`acting-github-user-id` to the GitHub numeric user id of the user the " + | ||
| "chat should run as.", |
There was a problem hiding this comment.
P4 [DEREM-6] The users/me fallback throw doesn't attach { cause: err }, losing the structured CoderAPIError for classifyError. The explicit-input error paths (e.g. line 653) use new ActionFailureError(... { cause: err }), preserving the cause chain. The auto-resolve throws (sender at 735, actor at 758/771) and this new users/me throw use plain new Error(...) without cause.
classifyErrorsees a plainError, not the originalCoderAPIError, so it always classifies asapi_errorregardless of the upstream status code. The message is descriptive enough for humans, but the cause chain is invisible to programmatic consumers. (Hisoka)
Pre-existing pattern; fixing only here would create an inconsistency. Noting for a future cleanup pass. (Hisoka P4, Chopper Note)
🤖
There was a problem hiding this comment.
Leaving as-is. The sender and actor sibling throws (lines 728-730, 753-755, 766-768) use the same plain new Error(...) pattern; fixing only the users/me site would create an inconsistency without addressing the underlying class of failure. A separate cleanup pass should switch all four to ActionFailureError(..., { cause: err }) together.
🤖 Posted using
/amend-reviewskill via Coder Agents.
Addresses the round-1 review findings on PR #32. The PR's own error messages and one README paragraph still carried the pre-rename framing ("the user the chat should run as"), and the deployment-list URL identity ("chats") was not renamed when the URL migrated to `/agents`. What changed: - Rewrite three error-message strings to match the README's "acting user (for org pick and the per-user reuse label)" framing (DEREM-2: action.ts trust-gate refusal, action.ts users/me failure, comment.ts user_ambiguous). - Rename `buildDeploymentChatsUrl` -> `buildDeploymentAgentsUrl`, `chatsUrl` field on `FailureCommentContext` -> `agentsUrl`, "View chats in the Coder deployment" link text -> "View agents", and the surrounding comment (DEREM-3). Update the call site in action.ts and the comment.test.ts tests. - Rewrite the README quickstart paragraph to stop using "runs under" language for the acting user (DEREM-4). - Add a divergence catch-path test: when `getAuthenticatedUser` rejects, action.run completes, a `core.warning` is emitted naming the fetch failure, and createChat is still reached (DEREM-5). - Add a `core.info` for the `no-signal` trust-gate verdict so an operator debugging identity resolution can see the gate ran and deferred (DEREM-8). - Fix the stale doc comment on `resolveOrganizationID` that claimed the username lookup happens here (DEREM-9). The lookup now always happens in `resolveCoderUsername`. - Migrate the missed `/chats/` URL fixture in schemas.test.ts:331 to `/agents/` (DEREM-1). - Drop the duplicate `Coder username: ...` info log; the new `Resolved acting Coder user: ... (source: ...)` line above already covers it (DEREM-7). DEREM-6 (P4) declined: the `users/me` throw uses the same plain `new Error(...)` shape as the sender/actor sibling throws on purpose. Fixing only this site would create an inconsistency; the cleanup is best landed as a separate pass across all four throws. 🤖 Authored by Coder Agents.
|
In reply to DEREM-1 (P3, Updated to In reply to DEREM-7 (Nit, Removed the duplicate
|
|
/coder-agents-review |
There was a problem hiding this comment.
All 8 R1 fixes verified by the full panel. Error messages now match the corrected ownership model. The buildDeploymentAgentsUrl rename is complete. The warnOnTokenOwnerDivergence catch path is tested. The README no longer says "runs under." DEREM-6 (P4, { cause: err } missing on users/me throw) remains acknowledged; the pre-existing pattern across four sibling throw sites makes a single-site fix create asymmetry.
The users/me fallback is safely gated: untrusted triggers throw before reaching it, and the test suite pins this with negative assertions on mockGetAuthenticatedUser. The divergence warning is correctly scoped to explicit inputs. 272 tests pass. Pariston: "I tried to build a case against this change and could not."
1 new Nit below. 1 Nit dropped (scope creep: pre-existing function name).
🤖 This review was automatically generated with Coder Agents.
| | { kind: "untrusted"; reason: string } | ||
| | { kind: "no-signal" }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Nit [DEREM-10] IdentitySource doc comment describes one consumer (the divergence check), not the type itself. The type is also returned from resolveCoderUsername (line 637), logged in runInner (line 1067), and threaded through warnOnTokenOwnerDivergence (line 846). The comment should describe what the values represent and where they originate, not just one reader. (Gon)
🤖
1 participant
The chat owner on
POST /api/experimental/chatsis fixed bycoder-token; the API has no owner override. The action previously framed its identity inputs as picking "the Coder user the chat runs as", and built chat URLs against the wrong route. This drops both, renames the identity inputs to make their scope unambiguous, and rewrites the README threat model so the trust gate is documented for what it actually protects (the acting user used for org pick and the per-user reuse label), not the chat owner.What changed
coder-username->acting-coder-usernamegithub-user-id->acting-github-user-idcoder-username->acting-coder-usernameacting-prefix is the unambiguous signal that these select the acting user, not the chat owner. The action is unreleased; no compatibility shims.CoderClient.getAuthenticatedUser()backed byGET /api/v2/users/me.resolveCoderUsernamefalls back tousers/mewhen no input is set and the trust gate returnsno-signal(schedule,workflow_dispatchwith no sender or actor, customrepository_dispatchchains). The gate still refuses untrusted triggers and does not fall through tousers/me.acting-coder-usernameoracting-github-user-id) differs from the token owner. Suppressed for auto-resolved sources and for theusers/mefallback itself.secrets.*rule for fork PRs is the primary attacker-controlled-prompts mitigation, with the trust gate as defense in depth.generateChatUrlandbuildDeploymentChatsUrlnow build/agents/<id>and/agentspaths, matching the real Coder frontend routes (coder/codersite/src/router.tsx:715-764,site/src/pages/AgentsPage/utils/navigation.ts:6).New tests pin:
users/mefallback for schedule, partialworkflow_dispatch, andrepository_dispatch;users/medoes not fire when the trust gate refuses; explicit input precedence; divergence warning fires and is suppressed correctly;users/meis not called on auto-resolved paths;getAuthenticatedUserclient behavior. The schema mutex error-message shape is pinned to the new key names. Existing URL assertions updated for the/agentspath; all other existing tests pass without changes.Closes CODAGT-437
Closes CODAGT-394
Closes CODAGT-438
🤖 Authored by Coder Agents.