feat: send coder-organization through to the chats API#23
Conversation
|
/coder-agents-review 🤖 Posted 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.
This is a first-pass review only, covering mechanical findings from the Netero scan. The full review panel has not yet reviewed this PR; the panel will review after these findings are addressed.
The org-resolution wiring is clean and well-tested for the happy paths. The ChatActionError plumbing, schema additions, and doc updates are solid. Three findings, one structural (P2), two gaps (P3).
P3 [DEREM-2] RealCoderClient.getCoderUserByUsername and RealCoderClient.getOrganizationByName have zero direct test coverage in coder-client.test.ts. Both methods are new in this PR (lines 93-109 of coder-client.ts). They contain input validation, endpoint construction with encodeURIComponent, and Zod schema parsing. The action tests exercise MockCoderClient only, which stubs all behavior. The existing coder-client.test.ts describes getCoderUserByGitHubId with five cases but has no describe block for either new method. (Netero)
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Further review is blocked until the author responds or pushes fixes for the open findings from round 1.
Unaddressed findings:
- DEREM-1 (P2,
src/action.ts:290):resolveOrganizationIDruns unconditionally, blocking theexistingChatIdpath with a spuriousorg_not_foundfailure. Move the call into thecreateChatbranch. - DEREM-3 (P3,
src/action.ts:254):getOrganizationByName404 is not wrapped intoChatActionError(org_not_found). Workflows branching onchat-error-kindcannot distinguish a typo incoder-organizationfrom an unrelated API failure.
The full review panel will run once these are addressed or the author responds.
🤖 This review was automatically generated with Coder Agents.
|
In reply to DEREM-2 (P3, Added direct
|
mafredri
force-pushed
the
s2-identity-sEwMTa
branch
from
5868d6b to
4e94f0e
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 1 findings (DEREM-1, DEREM-3) verified as fixed. DEREM-2 (test coverage) also addressed with 8 new tests. The org-resolution wiring is clean, the ChatActionError plumbing is solid, and the test suite covers the four S3 paths plus the existing-chat skip. Melody walked 9 paired structures (interface/impl/mock, schema/request, output map/yaml) and found no gaps.
Severity count: 3 P3, 4 Nit, 2 Note.
The strongest panel finding is DEREM-5 (P3): getCoderUserByUsername 404 inside resolveOrganizationID propagates as a raw CoderAPIError, the same class of bug DEREM-3 addressed for getOrganizationByName. Six reviewers flagged it independently. DEREM-6 (P3) is structural: multi-org users get a non-deterministic org choice with no warning. DEREM-7 (P3) is about debuggability: ChatActionError drops the original error chain.
Process nits: commit a98706d subject "address round 1 review" does not name the fix. The PR title's "S3" prefix is project-internal jargon; strip it from the permanent log line. (Leorio)
"Eight org-resolution tests, eight client tests, the existing-chat regression. All four S3 paths covered plus the skip path. Real gems." (Luffy, Bisky)
🤖 This review was automatically generated with Coder Agents.
| // workflow consumers can branch on machine-readable failures (org | ||
| // not found, spend exceeded, timeout, etc.). Later slices reuse the | ||
| // same plumbing. | ||
| core.setOutput("chat-error-kind", error.errorKind); |
There was a problem hiding this comment.
Note [DEREM-4] (Netero, R1) Error outputs bypass OUTPUT_MAP. The ChatActionError catch block writes chat-error-kind and chat-error-message via direct core.setOutput calls, while the success path uses the data-driven OUTPUT_MAP (which includes entries for these same outputs). The two mechanisms are mutually exclusive today, so there is no bug. If someone renames these outputs in OUTPUT_MAP, the catch block drifts silently. Low risk with only two names duplicated. Do not re-raise.
🤖
There was a problem hiding this comment.
Acknowledged per the reviewer's "Do not re-raise" instruction. The ChatActionError catch in index.ts writes chat-error-kind and chat-error-message via direct core.setOutput calls; the success path uses OUTPUT_MAP. The two paths are mutually exclusive at runtime (success and failure cannot both fire in one invocation), so there is no observable bug. Adding an indirection through OUTPUT_MAP for a 2-name failure path would couple the catch to the success-output schema for no current benefit; if we ever rename either output, the YAML and the catch will both need updating either way. Leaving as a known structural duplication. (No code change in this round.)
🤖 Posted using
/amend-reviewskill via Coder Agents.
mafredri
force-pushed
the
s2-identity-sEwMTa
branch
from
011205a to
fe4d632
Compare
|
/coder-agents-review 🤖 Posted by Coder Agents. |
There was a problem hiding this comment.
All R1 and R3 findings verified as fixed. Netero clean. 10-reviewer panel found 3 P3, 2 Nit. Prior fixes were thorough: error wrapping is symmetric (org and user 404s both wrapped with Error.cause), multi-org warning emitted, catch deduplication clean, JSDoc complete, mock defaults aligned with production.
Severity count: 3 P3, 2 Nit.
The three P3s are test quality and documentation issues, not production defects. The feature itself is solid. Approving because the remaining findings do not affect correctness or safety of the shipped behavior.
"TDD commit sequence is visible and genuine. Red tests precede the implementation commit. Corrections generalized, not just patched. Each correction was applied to the class of bug, not just the instance." (Mafu-san)
🤖 This review was automatically generated with Coder Agents.
mafredri
force-pushed
the
s2-identity-sEwMTa
branch
from
6f313fe to
cc6b864
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All R4 findings verified as fixed. Netero caught two P2 stale-description issues that slipped through prior rounds. The 5-reviewer panel confirmed and added one test bug (P3).
Severity count: 2 P2, 1 P3, 4 Nit.
The two P2s are the headline: action.yaml:39 and README.md:122 still say coder-organization is "Reserved; not yet wired," which directly contradicts the Organization resolution section this PR adds. A workflow author reading those descriptions will believe the input is inert. These are documentation-only fixes with no code change needed.
DEREM-22 (P3) is a test correctness bug: two .rejects.toThrow() calls lack await, so the error-type assertions are fire-and-forget.
Approving because the production code and error handling are solid across 5 rounds. The remaining findings are documentation and test hygiene.
PS. Leorio's R3 nit about the PR title's "S3" prefix still applies. Strip it before merge so the permanent log reads feat(create-agent-chat-action): wire coder-organization through to chats API.
action.yaml:39
P2 [DEREM-19] Description says "Reserved; not yet wired through to chat creation, the action emits a warning if set." The input is now fully functional: resolveOrganizationID looks it up by name, returns the UUID, and passes it to createChat. The README's Organization resolution section (lines 27-41) correctly describes the behavior. This description is the primary contract for GitHub Actions consumers.
A workflow author reading this will believe the input is inert and skip setting it, even for multi-org deployments where pinning the org is recommended. (Netero P2)
Fix: update to match the README body, e.g. "Coder organization name. Looked up by name to resolve the organization UUID for chat creation. Recommended when the user belongs to more than one organization."
🤖
README.md:122
P2 [DEREM-20] Inputs table row for coder-organization says "Reserved; not yet wired through to chat creation, the action emits a warning if set." This contradicts the Organization resolution section at lines 27-41 of the same file. The contradiction within the same document is worse than a standalone stale description because a reader who checks both will not know which to trust. (Netero P2)
🤖
🤖 This review was automatically generated with Coder Agents.
mafredri
changed the title
|
In reply to DEREM-19 (P2, Updated In reply to DEREM-20 (P2, Updated the inputs table row to mirror the
|
mafredri
force-pushed
the
s2-identity-sEwMTa
branch
from
cc6b864 to
7641630
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 24 findings from rounds 1-5 are closed. Netero clean. 4-reviewer panel clean. No new findings.
6 rounds, 24 findings (3 P2, 8 P3, 1 P4, 8 Nit, 4 Note), 22 author-fixed, 1 acknowledged, 1 dropped. Every P2 and P3 was addressed with code changes and regression tests. The production code, error handling, documentation, and test suite are solid.
124 tests pass. CI green. Ready to merge.
PS. Strip "S3" from the PR title before merge.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 24 prior findings closed. Netero clean. 3-reviewer panel: Mafuuu and Kite found no issues. Pariston raised org_not_found conflating two conditions (named-org-missing vs. no-memberships) as a design observation; downgraded to Note since the human-readable messages distinguish the cases and adding a third kind later is backward-compatible.
7 rounds, 25 findings total (3 P2, 8 P3, 1 P4, 8 Nit, 5 Note). 22 author-fixed, 1 acknowledged, 3 dropped. Every P2 and P3 was addressed. 124 tests pass. CI green.
🤖 This review was automatically generated with Coder Agents.
mafredri
force-pushed
the
s2-identity-sEwMTa
branch
from
ef5d184 to
1751b10
Compare
|
/coder-agents-review |
|
/coder-agents-review |
|
/coder-agents-review |
|
/coder-agents-review |
There was a problem hiding this comment.
All prior findings remain closed. Netero clean. 3-reviewer panel found 1 P3, 1 Nit.
DEREM-27 (P3): the README's service-account example claims to support fork PRs ("including fork PRs from community contributors") but uses on: pull_request with secrets.*, which GitHub does not provide to fork PR runs. A copy-paste user discovers the mismatch at runtime. The caveat paragraph after the example explains this correctly, but the structure misleads: claim, then complete yaml that doesn't work for the claim, then caveat. Fix: remove the fork-PR parenthetical from the lead-in and let the caveat carry that responsibility.
DEREM-26 (Nit): resolveCoderUsername wraps errors without Error.cause, inconsistent with resolveOrganizationID's pattern and the JSDoc's "preserve the upstream error" promise. Diagnostic info IS preserved via describeError(err) string embedding; the structured cause chain is a nice-to-have.
Approving because the P3 is a documentation precision issue, not a production defect. The action itself behaves correctly.
README.md:46
P3 [DEREM-27] The lead-in says "including fork PRs from community contributors" but the example uses on: pull_request with ${{ secrets.CODER_URL }} and ${{ secrets.CODER_TOKEN }}. On fork PRs, pull_request events do not receive secrets.*. The inputs will be empty strings and Zod validation will reject them.
The caveat paragraph after the example explains this correctly, but the structure is: claim (fork PRs work) then complete yaml (that doesn't work for fork PRs) then caveat (explaining why). A user who copies the yaml for the stated purpose discovers the mismatch at runtime. (Pariston P3)
Simplest fix: remove the "(including fork PRs from community contributors)" parenthetical from the lead-in. Let the caveat carry that responsibility alone.
🤖
🤖 This review was automatically generated with Coder Agents.
| return coderUser.username; | ||
| return { username: coderUser.username, user: coderUser }; | ||
| } catch (err) { | ||
| throw new Error( |
There was a problem hiding this comment.
Nit [DEREM-26] resolveCoderUsername wraps errors without Error.cause (here, line 520, line 529), while resolveOrganizationID uses { cause: err } on every wrap. The JSDoc at line 405 promises "preserve the upstream error" but these throw sites use new Error(message) without { cause: err }. describeError(err) inlines the message text, so diagnostic info is not lost in practice; the structured cause chain is the gap. (Mafuuu P3, downgraded to Nit)
🤖
mafredri
force-pushed
the
s2-identity-sEwMTa
branch
2 times, most recently
from
ee0f5a3 to
1283f5a
Compare
…ats API The action accepted `coder-organization` as input but never sent it, so multi-org Coder deployments fell back to whatever default the platform picked. Closes [#8](#8) by johnstcn. What changed: - `src/coder-client.ts`: add `getOrganizationByName` and `getCoderUserByUsername` to `CoderClient`, with `RealCoderClient` impls against `/api/v2/organizations/{name}` and `/api/v2/users/{name}`. Add `CoderOrganizationSchema`. Make `organization_id` required on `CreateChatRequestSchema`. - `src/action.ts`: add `resolveOrganizationID(coderUsername, resolvedUser?)`. Resolution order: `coder-organization` input looked up by name, falling back to the resolved user's `organization_ids[0]`. The call lives only on the create-chat branch so the `existing-chat-id` follow-up flow does not pay the extra API call or hit a spurious `org_not_found` failure. Both 404 paths (named org missing, username missing) wrap into `ChatActionError("org_not_found")` and `ChatActionError("user_not_found")` so workflows can branch on `chat-error-kind`; the original `CoderAPIError` is attached via `Error.cause`. When the resolved user has multiple org memberships and `coder-organization` is unset, the action emits `core.warning` with the chosen org id and recommends pinning the choice (the API does not order `organization_ids`). Adds `ChatActionError` carrying a machine-readable `errorKind` and forwarding `ErrorOptions` to `super`. Adds `handleFatalError(error)` so the outer catch in `index.ts` is unit-testable. - `src/index.ts`: outer catch delegates to `handleFatalError`, which maps `ChatActionError` to the `chat-error-kind` and `chat-error-message` action outputs, then falls through to the shared `Error` logging body. - `action.yaml`, `README.md`: describe the wired behavior. Document the resolution order, the `org_not_found` and `user_not_found` triggers, the multi-org warning, and the `Error.cause` chain. - Tests: cover the four resolution paths, the existing-chat skip path, both 404 wraps, the multi-org warning, the single-org no-warn case, the `cause` chain, direct `RealCoderClient` coverage for the two new methods, and `handleFatalError` branches. Closes CODAGT-285.
mafredri
changed the title
1 participant
Wires the
coder-organizationinput through to the chats API. The action accepted the input but never sent it, so multi-org Coder deployments fell back to whatever default the platform picked. Closes coder/create-agent-chat-action#8 by johnstcn.What changed
src/coder-client.ts: addgetOrganizationByNameandgetCoderUserByUsernametoCoderClient, withRealCoderClientimpls against/api/v2/organizations/{name}and/api/v2/users/{name}. AddCoderOrganizationSchema. Makeorganization_idrequired onCreateChatRequestSchema.src/action.ts: addresolveOrganizationID(coderUsername, resolvedUser?). Resolution order:coder-organizationinput looked up by name, falling back to the resolved user'sorganization_ids[0]. The call lives only on the create-chat branch so theexisting-chat-idfollow-up flow does not pay the extra API call or hit a spuriousorg_not_foundfailure. Both 404 paths (named org missing, username missing) are wrapped intoChatActionError("org_not_found")andChatActionError("user_not_found")so workflows can branch onchat-error-kind; the originalCoderAPIErroris attached viaError.cause. When the resolved user has multiple org memberships andcoder-organizationis unset, the action emitscore.warningwith the chosen org id and recommends pinning the choice (the API does not orderorganization_ids).resolveCoderUsernamereturns{ username, user? }so the GitHub-id-path user object is reused without a redundant API call. AddsChatActionErrorcarrying a machine-readableerrorKindand forwardingErrorOptionstosuper.src/index.ts: outer catch mapsChatActionErrorto thechat-error-kindandchat-error-messageaction outputs, then falls through to the sharedErrorlogging body.src/action.test.ts,src/coder-client.test.ts,src/test-helpers.ts: tests for the four resolution paths plus the existing-chat skip path, both 404 wraps (org / user), the multi-org warning, the single-org no-warn case, thecausechain, and directRealCoderClientcoverage for the two new methods.createMockInputsno longer defaultscoderOrganization, matching production where the input is optional with no default.README.md: new "Organization resolution" section documenting the precedence, bothorg_not_foundtriggers, theuser_not_foundtrigger, the multi-org warning, and theError.causechain.dist/index.jsrebuilt with bun 1.3.13.Closes CODAGT-285
🤖 Authored by Coder Agents.