Handle OpenCode question prompts in Discord flow - #66
Conversation
RoundTable02
left a comment
RoundTable02
left a comment
There was a problem hiding this comment.
Thanks for taking over #60 and preserving the original commit. I independently reproduced #46: main has no question.asked callback path, while this PR does deliver the event to the execution layer. However, the current implementation still fails several valid OpenCode question flows, so I do not think it is safe to merge yet.
Verification performed:
main: 171/171 existing tests passed; the added issue regression test failed becauseonQuestionAskeddoes not exist.- PR #60: 178/178 existing tests and build passed, but an adversarial multi-question test showed that it immediately sends only
[["A"]], dropping the second question. - PR #66: 184/184 existing tests and build passed, and the basic issue regression test passed. Four adversarial tests failed: it submitted
[["A"], []], accepted a request owned by another session, serialized a message select withmin_values: 0, and threwInteractionNotRepliedon an OpenCode failure before the interaction was acknowledged.
The inline comments below cover the blocking findings. The OpenCode 1.3.9 contract also enables custom answers by default, so the Discord flow needs a modal or a mapped next-message reply path rather than options-only handling.
Security note: I found no new dependency, secret, SSRF, or unauthenticated authorization issue in this patch. The session/request ownership issue below is an integrity boundary problem because /question lists pending requests across all sessions on the same OpenCode server.
Recommendation: keep #60 superseded, address these findings in #66, and add regression tests that validate serialized Discord component payloads and error/timeout paths.
|
I will make changes later in the weekend, thanks for the review |
RoundTable02
left a comment
RoundTable02
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous review. I re-ran the full suite and the adversarial regression tests against 8d3ac28.
Confirmed fixed:
- Discord message selects now serialize with
min_values: 1. - Incomplete manual submission is rejected instead of posting an empty answer array.
- Answer/select/modal/submit paths acknowledge interactions before calling OpenCode.
- Answer paths now bind the request to the current OpenCode session.
Verification:
- Existing suite: 188/188 passed.
- TypeScript build: passed.
- Follow-up adversarial suite: 3 passed, 4 failed.
Four blocking issues remain. The inline comments cover the exact cases: OpenCode's omitted custom field is interpreted incorrectly, explicit custom input can create a six-button Discord row, requests with more than five questions lose controls, and reject still bypasses the request/session ownership check.
No new dependency or secret changes were introduced by the follow-up commit. I recommend keeping this PR in CHANGES_REQUESTED until these cases are covered by regression tests and fixed.
- custom detection: treat custom as enabled unless explicitly disabled (question.custom !== false instead of === true) - 6-button row: when 5 options + custom would exceed limit, use select menu instead; add custom button in its own row for select menu questions - pagination: support 5+ questions by paginating maxRows-1 questions per page with Prev/Next nav in the bottom row - reject ownership: add findRequestForSession check before rejecting
RoundTable02
left a comment
RoundTable02
left a comment
There was a problem hiding this comment.
Thanks for addressing the four findings from the previous review. I re-ran the full suite and fresh adversarial regressions against 745d68a.
Confirmed fixed:
- Omitted
customis now treated as enabled. - Five options plus Custom no longer creates a six-button row.
- Six one-row questions are reachable through pagination.
- Reject now enforces request/session ownership.
Verification:
- Existing suite: 188/188 passed.
- TypeScript build: passed.
- Fresh adversarial suite: 4 passed, 2 failed.
- The Reject unit test fails when run in isolation, despite passing as part of the full suite.
Two blocking hang regressions remain. Pagination is based on a fixed question count even though some questions consume two component rows, which makes questions disappear between pages. Separately, the message text is not paginated and can exceed Discord's 2,000-character content limit; the resulting fallback has no answer controls. The inline comments include exact reproductions and fixes.
I also found two correctness/coverage gaps: OpenCode permits more than 25 options but this implementation silently drops the overflow, and the Reject test passes only because mock state leaks from the previous test.
Security update: the cross-session Reject integrity issue is fixed. No dependency or lockfile changes, new secrets, or new authentication bypasses were introduced by this follow-up. The current production audit findings are pre-existing and not caused by this PR.
Recommendation: keep this PR in CHANGES_REQUESTED until the variable-row pagination, complete-message length budget, option overflow behavior, and independent regression tests are addressed.
…ation - Pagination now counts by actual row consumption (select+custom=2 rows) - buildQuestionText only renders the current page's questions - Options >25 force enable custom input so overflow is reachable - Reject test now explicitly mocks listQuestions (no state leak)
2 participants
Copy of #60