feat: redesign action input and output schema#18
Conversation
Failing tests for the v0 input/output contract before implementation: - ActionInputsSchema accepts the new shape (renames github-issue-url to github-url, adds wait, wait-timeout-seconds, idempotency-label-key). - Wait enum is none|complete with default none. - wait-timeout-seconds defaults to 600, accepts positive integers, and rejects 0, negative, and non-integer values. - ActionInputsSchema rejects setting both github-user-id and coder-username. - ActionInputsSchema rejects the old github-issue-url field. - ActionOutputsSchema includes chat-status, chat-title, workspace-id, pull-request-*, additions, deletions, changed-files, head-branch, base-branch, chat-error-kind, chat-error-message. - CoderChatSchema parses diff_status, last_error and the other v0 chat fields cherry-picked from the discarded PR.
…r v0 Locks the v0 action schema before later slices wire the new inputs to behavior. Renames github-issue-url to github-url, makes the identity inputs optional (S2 will auto-resolve), adds wait, wait-timeout-seconds, and idempotency-label-key for slices that own them. Adds the chat-status, chat-title, workspace-id, pull-request-*, additions, deletions, changed-files, head-branch, base-branch, chat-error-kind, and chat-error-message outputs cherry-picked from the discarded PR #1. CoderChatSchema now parses diff_status, last_error, and the other v0 chat fields so subsequent slices can read them. No behavior changes beyond the github-url rename. The chat creation path now renders the new outputs from the chat object that createChat already returns; the existing-chat-id path keeps the original four outputs because S1 must not introduce extra API calls.
|
/coder-agents-review |
The CI workflow used 'bun-version: latest', which produced a dist/ that differed from one bundled with bun 1.2.15 locally. Pin bun to 1.3.13 (current latest at the time of writing) so the foundation slice does not force every downstream slice to fight version drift, and regenerate dist/index.js with bun 1.3.13 so check_unstaged.sh passes on CI.
|
/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 schema redesign is well-structured: Zod schemas serve as both validation and type generation, the refine() mutual-exclusion rule is clean, and the cherry-picked outputs from the discarded PR are integrated without leftover cruft. The buildOutputs extractor keeps index.ts focused on I/O.
First-pass review (Netero only): 2 P2, 2 P3, 3 Nit, 1 Note. These are mechanical findings. The full review panel has not yet reviewed this PR; the panel will review after these findings are addressed.
The two P2s are contract violations: the github-url input promises PR URL support that the regex rejects at runtime, and the wait input accepts complete but silently does nothing. Both create false contracts that will bite workflow authors.
"Shipping a documented contract the code cannot honor is a runtime failure for any PR-triggered workflow."
src/action.test.ts:213
P3 [DEREM-3] The "creates new chat successfully" test calls buildOutputs indirectly but only asserts coderUsername, chatCreated, and chatUrl. The mapping from CoderChat.status, CoderChat.title, CoderChat.workspace_id, and CoderChat.diff_status.* to the corresponding ActionOutputs fields is unchecked. (Netero)
A field swap (e.g.,
chatStatus: chat.title) would pass all existing tests. The mock chat hasdiff_status: null, so the diff-field mapping path is never exercised.
A dedicated buildOutputs unit test with a mock chat containing non-null diff_status would catch mapping errors.
🤖
src/action.ts:180
P3 [DEREM-4] The existing-chat path returns a bare 4-field object (coderUsername, chatId, chatUrl, chatCreated), skipping buildOutputs. A workflow step reading chat-status or workspace-id after sending a follow-up message gets empty outputs, while the create path populates them. (Netero)
The existing-chat path could call
getChat(chatId)to fetch the full object and then callbuildOutputs, keeping the output surface consistent.
🤖
src/action.ts:23
Nit [DEREM-5] Method name parseGithubIssueURL, return field githubIssueNumber, and error messages all reference "issue" exclusively, despite the input being renamed to github-url to cover both issues and PRs. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
| headBranch: z.string().optional(), | ||
| baseBranch: z.string().optional(), | ||
| // Failure-path outputs, populated by S5 when the chat errors. | ||
| chatErrorKind: z.string().optional(), |
There was a problem hiding this comment.
Note [DEREM-8] chatErrorKind and chatErrorMessage output fields are declared but never populated by any code path. The catch block in index.ts calls core.setFailed() and exits without setting these outputs. Per PR description, S5 adds the failure path. (Netero)
🤖
There was a problem hiding this comment.
Disagreed with making this a code change in S1. chat-error-kind and chat-error-message are intentionally declared in the schema and action.yaml so S5 (the failure-path slice) can populate them without amending action.yaml again. The PR description and src/schemas.ts already note that this slice does not populate them. The implementation plan locks the chat-error-kind enum during S5 specifically. Without these declarations in S1, every later slice that surfaces an error would need to widen the schema, which is exactly the layering the v0 implementation plan is designed to avoid.
🤖 Posted using
/amend-reviewskill via Coder Agents.
DEREM-1 / DEREM-5: `parseGithubURL` now accepts both `/issues/` and `/pull/` URLs to honor the action.yaml description. The method is renamed from `parseGithubIssueURL` to `parseGithubURL` and the error messages no longer say "issue URL". DEREM-2: `warnUnwiredInputs` emits a workflow warning when the user explicitly opts in to `wait: complete` or `idempotency-label-key`, which the schema accepts but no slice has wired yet. The warning points at the slice that owns each input so the discard is discoverable rather than silent. DEREM-3: `buildOutputs` now has dedicated tests covering both the `diff_status: null` shape and a fully populated diff, plus an edge case for the empty `pull_request_title`. The create-path test also asserts the new mapped fields so a property swap can no longer pass. DEREM-4: the existing-chat-id path now calls `getChat` after the follow-up message and routes through `buildOutputs`, so workflows reading `chat-status`, `workspace-id`, etc. after a follow-up get the same output surface as the create path. The extra API call is the cost of the contract action.yaml documents. DEREM-6: replace the 13 conditional `core.setOutput` blocks in index.ts with a single typed loop over an OUTPUT_MAP table so the property/output-name relationship lives in one place. DEREM-7: add an inline comment above the `||` fallback for `pullRequestTitle` explaining why the divergence from the `??` style elsewhere is intentional (the Zod default is the empty string). DEREM-8: no code change. The `chat-error-kind` and `chat-error-message` outputs are intentionally declared in S1 so S5 can populate them without amending action.yaml again. Already called out in schemas.ts and the PR description.
DEREM-4 introduced a getChat call after createChatMessage so the existing-chat-id path produces the same rich outputs as the create path. Without a try/catch, a transient getChat failure would fail the whole step even though the follow-up message was sent successfully. This is a regression compared to the pre-fix behavior. Wrap the getChat call so a failure logs a warning, the step still succeeds, and the action returns the four base outputs. Add a regression test.
|
In reply to DEREM-3 (P3, Added a dedicated In reply to DEREM-4 (P3, The existing-chat-id path now calls In reply to DEREM-5 (Nit, Renamed the method to
|
|
/coder-agents-review |
There was a problem hiding this comment.
Good progress. All 7 addressed findings from R1 are verified: the regex expansion, warnUnwiredInputs, dedicated buildOutputs tests, the getChat + buildOutputs path for existing chats, the method rename, the OUTPUT_MAP loop, and the intentional || comment all land cleanly.
First-pass review (Netero only, round 2): 1 P2, 1 P3, 1 Nit. These are mechanical findings from Netero. The full review panel has not yet reviewed this PR; the panel will review after these findings are addressed.
The P2 is a sibling of the R1 warnUnwiredInputs fix: the same "schema accepts, runtime crashes" pattern applies to the both-identity-inputs-unset case.
"A workflow author who reads the docs and omits both gets a cryptic crash instead of the discoverable warning that
waitandidempotencyLabelKeyalready receive."
src/action.ts:166
P2 [DEREM-9] Schema permits both githubUserID and coderUsername unset (the refine only rejects when both are set), and action.yaml documents this as valid ("both may be unset to auto-resolve from the workflow context"). But run() falls into the else branch here and calls getCoderUserByGitHubId(undefined), which throws "GitHub user ID cannot be undefined" with a 400 status. (Netero)
Re-raise of the DEREM-2 root cause. The
warnUnwiredInputspattern already handleswait: completeandidempotencyLabelKey. The same pattern applies here: guard the both-unset case with a clear warning or error naming both inputs and the S2 auto-resolve plan.
🤖
🤖 This review was automatically generated with Coder Agents.
DEREM-9: `run()` now throws a clear error when neither `github-user-id` nor `coder-username` is set, naming both inputs and pointing at S2 for the auto-resolve plan. Previously the lookup crashed inside `getCoderUserByGitHubId(undefined)` with the misleading message "GitHub user ID cannot be undefined". This is the same shape as the round 1 `warnUnwiredInputs` fix: the schema accepts a contract that S1 cannot honor, so the runtime must fail loudly. Added a regression test. DEREM-10: extracted `OUTPUT_MAP` and `setActionOutputs` from `src/index.ts` to a new `src/outputs.ts` module so they are importable without triggering `main()`. Added `src/outputs.test.ts` with edge-case coverage: numeric `0` emits "0" rather than being skipped, boolean `false` emits "false", optional `undefined` is skipped, required `undefined` falls through to the empty string defensively, and the full-output emission path covers all 18 entries. DEREM-11: replaced the user-facing log strings that said "GitHub issue number", "Commenting on issue", and "Skipping comment on issue" with the neutral "GitHub item number", "Commenting on", and "Skipping comment" so workflows triggered on PR URLs do not see log lines that misrepresent the input. Kept the field name `githubIssueNumber` and the method `commentOnIssue` because they match GitHub's REST API naming, where issues and PRs share a single endpoint.
|
In reply to DEREM-9 (P2,
|
…comments Drop references to the slice numbering, the implementation plan, and the discarded source PR from comments and runtime strings. Trim verbose doc comments around schemas, the output map, the warn-unwired-inputs helper, the both-identity-inputs-unset error, and the getChat fallback note. No behavior change. All exported APIs, output names, warning intents, and label keys stay byte-stable; only the surrounding prose tightens.
|
/coder-agents-review |
There was a problem hiding this comment.
All 10 findings from R1 and R2 are addressed. The fixes generalized well: DEREM-10 led to extracting outputs.ts as a testable module, DEREM-4 added graceful degradation on the existing-chat path, and the warnUnwiredInputs pattern from DEREM-2 now covers the both-identity-inputs-unset case. The buildOutputs test suite (three code paths, diff/no-diff/empty-title) and the setActionOutputs edge-case coverage are genuine.
DEREM-8 (contested, Note): panel closed 9/12 accept. The schema-first forward declaration is justified; the consequence (empty optional outputs) is harmless. The documentation concern about descriptions reading as implemented is tracked in DEREM-20.
Panel round (14 reviewers): 9 P3, 4 Nit, 2 Note. No P2s. The strongest theme is documentation-code contract misalignment: coder-organization promises defaulting that doesn't exist (3 independent reviewers), wait: complete description reads as implemented, and the Zod error message sends users in a circle with the runtime error. The code is solid; the contract surface needs tightening.
Process note: commit c5b9a3d claims "No behavior change" while shortening user-facing error/warning strings and removing a test assertion. The fix is precise language: "No functional change; user-facing strings shortened to remove internal plan references."
"Two error messages from the same codebase send the user in a circle. The first one promises auto-resolve; the second one says auto-resolve doesn't exist." (Leorio)
src/action.ts:55
Nit [DEREM-21] JSDoc at line 55 ("Comment on GitHub issue with chat link") and catch-block error at line 94 ("Failed to comment on issue: ${error}") still say "issue" after the PR URL rename. The method name commentOnIssue is fine (matches GitHub REST API), but the doc comment and error message are user-visible strings. (Gon, Chopper, Leorio)
🤖
src/action.ts:63
Nit [DEREM-22] The comment body "Agent chat created: ${chatUrl}" is posted for both new chats and existing-chat follow-ups. A user sees "chat created" when a follow-up message was sent. The existingComment matcher also keys on "Agent chat created:", so a follow-up overwrites the creation comment with identical text. (Chopper)
Consider
"Agent chat: ${chatUrl}"or parameterizing the verb.
🤖
🤖 This review was automatically generated with Coder Agents.
- Make `pull_request_title` nullable in `ChatDiffStatusSchema` so a null from the API does not reject the entire chat object. - Add missing `await` on `expect(action.run()).rejects.toThrow(...)` and `commentOnIssue(...)\.resolves.toBeUndefined()` so assertion failures actually fail the test rather than surfacing as unhandled rejections after the test has already passed. - Add `coder-organization` to `warnUnwiredInputs` so a workflow that sets it sees the same discoverable warning as `wait` and `idempotency-label-key`. Update action.yaml and README descriptions for those three inputs to call out that they are reserved and not yet wired. - Simplify the Zod refine error to "Cannot set both github-user-id and coder-username; choose one." so it does not contradict the runtime error that says exactly one must be set. - Wire `chat.last_error` to the `chatErrorMessage` output as a passthrough; the data path was already in place and was discarded at the last step. `chatErrorKind` stays unwired. - Gate `additions`, `deletions`, and `changed-files` on a meaningful PR indicator (`url` or `pr_number`) so a chat with `diff_status` but no PR yet does not emit a misleading truthy "0" while the string PR fields are unset. - Drop the unconditional "Comment posted successfully" log; it was emitted even when `commentOnIssue` swallowed an internal error, and the existing-chat path did not have it, making the two paths inconsistent. - Rename the issue comment body from "Agent chat created: ..." to "Agent chat: ..." so a follow-up message does not claim a chat was created. The comment matcher updates in lockstep. - Update `commentOnIssue` JSDoc and the catch-block error string to drop the "issue" wording now that the action also handles PR URLs. - Extract `DEFAULT_WAIT_TIMEOUT_SECONDS` so the 600 default is named in one place. - Add a structural test that `OUTPUT_MAP` covers every key of `ActionOutputsSchema` so a new schema field cannot silently fail to emit. - Add `buildOutputs` cases for zero-numerics with a PR (emits 0), diff_status without a PR indicator (skips the numerics), and `last_error` passthrough. - Replace the duplicated chat fixtures in schemas.test.ts with imports of `mockChat` and `mockChatWithDiff` from test-helpers.\n - Consolidate the `buildOutputs` nullish-handling commentary into one block at the top of the function.
|
In reply to DEREM-21 (Nit, Updated the JSDoc on In reply to DEREM-22 (Nit, Renamed the comment body from "Agent chat created: ${chatUrl}" to "Agent chat: ${chatUrl}". The matcher now keys on "Agent chat:" so the verb does not lie on follow-ups. Tests updated. Commit 2cf137b.
|
|
/coder-agents-review |
There was a problem hiding this comment.
All 15 R3 findings are addressed in commit 2cf137b. Verified by all 6 reviewers. The fixes are thorough: hasPR gating resolves the numeric asymmetry, the structural guard test catches OUTPUT_MAP drift, chatErrorMessage is wired as a passthrough, and the documentation now uses a consistent "Reserved; not yet wired" pattern for unimplemented features.
Panel round (6 reviewers): 2 P2, 2 P3, 3 Nit. Both P2s are contract-code divergences the R3 fixes missed at specific locations.
Pariston: "I tried to build a case against this and could not. The problem is correctly understood. The solution is proportional."
src/action.test.ts:210
P3 [DEREM-31] "creates new chat successfully" (line 210), "creates chat using direct coder-username" (line 421), and "creates chat with workspace-id" (line 523) run with commentOnIssue: true but never mock octokit.rest.issues.listComments. Each call reaches commentOnIssue, throws TypeError: Right side of assignment cannot be destructured, and the catch block eats it. (Bisky)
The tests pass because the production catch block swallows the TypeError, not because commenting works. If that catch block were tightened to rethrow non-API errors, all three tests break for the wrong reason. Fix: set
commentOnIssue: falsein these tests since they don't test commenting.
🤖
🤖 This review was automatically generated with Coder Agents.
- Drop the auto-resolve claim from `github-user-id`'s description in action.yaml so it stops contradicting the runtime, matching the README copy fixed in the previous round. - Mark `chat-error-kind` as "Reserved; not yet populated" in action.yaml and README so the documentation does not enumerate error kinds the code never produces. `chat-error-message` is populated and stays as is. - Add `.nullable()` to `last_model_config_id` and `archived` in CoderChatSchema, matching the other nullable UUID/boolean fields, so a `null` from the API does not reject the entire chat object after a successful create or message send. - Tighten `hasPR` in buildOutputs to gate purely on `pr_number`. A branch URL with no PR (`url` set, `pr_number` null) no longer emits `additions`, `deletions`, `changed-files` under the `pull-request-*` cluster. New test covers the branch-only case. - Switch `pull_request_title` to `.nullable().optional()` and consume it via `?? undefined` so all nullable string fields share one pattern. Test renamed from empty-string to null coverage. - Set `commentOnIssue: false` in the three create-path tests that do not exercise the comment path. They were previously hitting `commentOnIssue` which then failed silently because the `octokit.rest.issues.listComments` mock was unset; the production catch swallowed the TypeError. - Spy on `core.error` in the GitHub API failure test so deletion of the error log would actually break the assertion. - Default `createMockInputs`'s `coderOrganization` to undefined so the test suite no longer triggers the warnUnwiredInputs warning on every run.
|
In reply to DEREM-31 (P3, Set
|
|
/coder-agents-review |
There was a problem hiding this comment.
34 findings across 5 rounds, all resolved. 4 P2s (contract violations), 15 P3s, 8 Nits, 3 Notes, 1 contested (DEREM-8, panel closed 9/12). Every finding was addressed in the immediately following commit with no regressions.
The code is solid. The schema redesign cleanly separates declaration (Zod schemas, action.yaml) from mapping (buildOutputs, OUTPUT_MAP) from emission (setActionOutputs). The structural guard test (OUTPUT_MAP covers every schema key) eliminates the class-of-bug that manual synchronization creates. The hasPR gate, graceful getChat fallback, and warnUnwiredInputs pattern are well-designed and tested.
Strengths worth noting: the author generalized from corrections (DEREM-10 led to extracting outputs.ts; DEREM-2 established warnUnwiredInputs that was reused for 3 more inputs), pushed back with evidence when findings were wrong (DEREM-8, DEREM-11 field name), and fixed an additional unawaited assertion (line 204) beyond what DEREM-15 asked for.
88 tests pass. CI green. No open findings.
🤖 This review was automatically generated with Coder Agents.
…dempotency-key The original name was a mouthful and mixed the implementation detail (today's chat label) with the contract (an opaque deduplication key). `idempotency-key` is shorter and maps cleanly to the future `Idempotency-Key` HTTP header path; the label-key implementation can swap to the header without another rename. The schema property follows: `idempotencyLabelKey` to `idempotencyKey`. The chat label value is unchanged; only the input name and property name move.
The `additions`, `deletions`, and `changed-files` outputs lacked the namespace prefix that every other diff-derived output already carries (`pull-request-*`, `head-branch`, `base-branch`). Rename to `diff-additions`, `diff-deletions`, `diff-changed-files` so the outputs cluster cleanly when sorted alphabetically and make their provenance obvious in workflow YAML. Only the YAML output names change; the `ActionOutputs` property shape stays camelCase (`additions`, `deletions`, `changedFiles`) since those are internal mapping keys. `OUTPUT_MAP` rewires the name column for the three entries.
|
/coder-agents-review |
There was a problem hiding this comment.
Post-approval renames verified clean. Two mechanical changes: idempotency-label-key to idempotency-key (generalizing before any consumers exist) and prefixing diff outputs with diff- (namespace clarity matching the pull-request-* cluster). Both are pre-merge naming decisions with zero consumers.
Netero: all 8 sections clean, 88 tests pass. Grepped for residual old names, zero matches. Panel (3 reviewers): no findings. Pariston verified both renames are defensible (generalize before publish, rename before consumers), the OUTPUT_MAP bridge handles the internal/external name split by design, and the structural guard test catches any drift.
34 findings across 6 rounds, all resolved. No open findings.
🤖 This review was automatically generated with Coder Agents.
mafredri
changed the title
2 participants
Redesigns the action's input/output schema and how outputs are populated. Renames
github-issue-urltogithub-urland accepts both issue and pull request URLs. Adds chat status, title, workspace, PR metadata, and error outputs that the action populates from the chat object the API already returns.What changed
action.yaml:github-urlaccepts issue or PR URLs;github-user-idandcoder-usernameare now optional with mutual-exclusion enforced by the schema; new optional inputswait,wait-timeout-seconds,idempotency-label-key; new outputschat-status,chat-title,workspace-id,pull-request-*,additions,deletions,changed-files,head-branch,base-branch,chat-error-kind,chat-error-message.src/schemas.ts:ActionInputsSchemaenforces mutual exclusion via.refine();waitis anone|completeenum (defaultnone);wait-timeout-secondscoerces to a positive integer (default 600).ActionOutputsSchemadeclares the new output surface.src/coder-client.ts:CoderChatSchemaparsesdiff_status,last_error,parent_chat_id,root_chat_id,last_model_config_id,archived. NewChatDiffStatusSchemacovers PR/branch metadata.src/action.ts:parseGithubURLaccepts both/issues/and/pull/URLs;warnUnwiredInputswarns when the user opts in to inputs whose runtime is not yet wired;buildOutputsrenders the new outputs from the chat object; both create and existing-chat-id paths return the full output surface, with a graceful fallback to minimal outputs if the post-messagegetChatfetch fails so a transient API error does not fail a step that already sent its message;run()throws a clear error when neither identity input is set.src/outputs.ts(new):OUTPUT_MAPandsetActionOutputsextracted fromindex.tsso they are unit-testable without triggeringmain().src/outputs.test.tscovers numeric0, booleanfalse, optionalundefined, requiredundefined, and the full-output emission path.src/action.test.ts,src/schemas.test.ts,src/test-helpers.ts: tests forparseGithubURL(issue and PR URLs),warnUnwiredInputs,buildOutputs(no-diff, populated diff, empty title), thegetChatfailure fallback, and the both-identity-inputs-unset guard..github/workflows/ci.yaml: pinbun-version: 1.3.13sodist/stays reproducible across runs.action.yamlexactly.dist/index.jsrebuilt.Closes CODAGT-283
🤖 Authored by Coder Agents.