Repository navigation
Retry a failed pull request or issue reply - #78
maxime-pharmania wants to merge 1 commit into
Conversation
9c4d5b0 to
8db8a15
Compare
![proval-0e6d07[bot]](https://avatars.githubusercontent.com/in/4828992?s=60&v=4){kind=small}
|
|
||
| if (activity.type === "pr_reply" || activity.type === "issue_reply") { | ||
| const targetCommentId = activity.targetCommentId; | ||
| if (targetCommentId == null) { |
There was a problem hiding this comment.
⚫️ [PROBLEM] Reply-target validation runs after provider setup, so doomed retries can 500 instead of 400
⚫ The null-comment guard at apps/api/src/api/activity/activity.service.ts:668 runs only after createGitProvider (line 619) and the rest of the provider/sender setup complete. For GitHub repositories, createGitProvider performs a network round-trip (octokit.request("GET /repositories/:repository_id") in repository.service.ts:372-376) and throws if the installation row is gone — so retrying a comment-less reply (any pre-0047 reply row with NULL target_comment_id) can fail during setup with an unmapped error and surface as a 500 instead of the 400 this PR adds to RETRY_CLIENT_ERRORS (activity.controller.ts:111). Even in the happy path, every doomed retry burns a full provider setup plus a GitHub API call just to be rejected.
Consider moving the check to right after the RETRY_TYPES validation (before repository/provider/workspace/sender setup):
if (activity.type === "pr_reply" || activity.type === "issue_reply") {
if (activity.targetCommentId == null) {
throw new Error("This activity has no comment to reply to");
}
}There was a problem hiding this comment.
Fixed in ca923cd. The check now runs right after the RETRY_TYPES guard, before createGitProvider, and it resolves the value once so the reply dispatch keys on it.
const replyTargetCommentId =
activity.type === "pr_reply" || activity.type === "issue_reply"
? requireReplyTargetComment(activity)
: null;The dispatch block became if (replyTargetCommentId != null), so the refusal has a single path and the later pr_review / issue_open flow is untouched.
|
Thanks for wiring reply-target persistence through this PR end to end — schema, all three webhook providers, retry, client gating, and demo fixtures move together, which makes the feature feel cohesive rather than bolted on. a. Summary This PR persists b. Good points
Main Issues
Additional Notes
|
8db8a15 to
ca923cd
Compare
|
All three points are addressed in ca923cd, and the two code ones are also on the fork main as 4c12124. Validation ordering. The null-comment guard now runs right after the Test spy. Snapshot chain. You were right that it looked hand-edited. The 0047 snapshot had been generated on top of an 0046 that only exists on the fork, so its The branch takes |
| ) | ||
| WHERE type IN ('pr_reply', 'issue_reply') | ||
| AND target_comment_id IS NULL |
There was a problem hiding this comment.
⚫️ [PROBLEM] Legacy recovery over-counts no-op rows and rescans them on every boot
The outer WHERE here matches any pr_reply/issue_reply row with a NULL target_comment_id and valid logs — including rows whose SET subquery resolves to NULL (inline-review replies and truncated logs), which activity-retry.test.ts:207-231 proves stay NULL forever. SQLite counts those no-op matches in changes(), so result.changes (line 271) and the startup log at apps/api/src/index.ts:53-55 print a false "Recovered N legacy reply comment link(s)" on every restart, and each boot re-runs the json_each scan over those permanently unrecoverable rows' full logs before the server accepts traffic.
Add an EXISTS guard mirroring the subquery's conditions so only rows with an extractable commentId are updated and counted:
WHERE type IN ('pr_reply', 'issue_reply')
AND target_comment_id IS NULL
AND json_valid(logs)
AND EXISTS (
SELECT 1 FROM json_each(activity.logs) AS entry
WHERE json_extract(entry.value, '$.type') = 'tool-call'
AND json_extract(entry.value, '$.toolName') IN ('get_pull_request_comment', 'get_issue_comment')
AND json_valid(json_extract(entry.value, '$.message'))
AND json_extract(json_extract(entry.value, '$.message'), '$.commentId') IS NOT NULL
)There was a problem hiding this comment.
Confirmed and fixed in 1c30c07, with the EXISTS guard you proposed so the update only touches rows whose subquery can actually fill the column. The test now pins the count instead of a lower bound: one recoverable row returns 1, and the pair of unrecoverable rows (inline review reply, truncated log entry) returns 0 where it used to count 2.
| "autoincrement": false, | ||
| "default": 600 | ||
| }, | ||
| "stream": { |
There was a problem hiding this comment.
⚫️ [PROBLEM] Snapshot contains a model_provider.stream column that exists nowhere else
This stream column (integer, notNull, default true) exists nowhere else in the repository: schema.ts has no stream field and no migration SQL in packages/db/src/migration/*.sql ever adds it — the only match in the repo is this snapshot entry. The snapshot's prevId at line 5 (10333aa2-...) also matches no snapshot id in the repo (0045's id is d54cf2e8-...), so this snapshot was generated on top of a 0046 that was dropped from the branch.
Impact: the snapshot is the baseline for the next drizzle-kit generate run, which will diff schema.ts against it and emit ALTER TABLE model_provider DROP COLUMN stream. No real database has that column, so applying the generated migration fails ("no such column") and blocks the startup migration (apps/api/src/index.ts:32). Fix: regenerate 0047_snapshot.json from the current schema.ts so it equals 0045 plus exactly the two new activity columns — i.e. remove this entry and repoint prevId to d54cf2e8-40e0-4bb1-a75f-a8efbc571842.
There was a problem hiding this comment.
This one was already fixed before this review landed. Commit ca923cd dropped that snapshot, and the branch now carries 0046_fearless_vermin generated on this base, so its prevId is 0045's id and it records the schema the branch really has, without stream. bun run --filter api generate answers No schema changes, nothing to migrate on the current head, so the DROP COLUMN you describe cannot be emitted.
|
Thanks for this PR — extending retry to OverviewSummaryThis PR adds Good points
Main Issues
Additional Notes
|
ca923cd to
1c30c07
Compare
|
Round 2 handled in 1c30c07. Recovery over-count. Valid, and the only finding still live against the head. The outer WHERE matched no-op rows and SQLite counts them, so the boot line could claim a recovery for an inline review reply or a truncated log entry, and rescanned them on every restart. The EXISTS guard is in, with the same conditions as the subquery, and the test asserts the exact count on both sides. Snapshot and the journal gap. Both belong to the previous head. ca923cd deleted the fork-generated snapshot and regenerated on this base as GitLab double expression. Hoisted, |
| END | ||
| FROM json_each(activity.logs) AS entry | ||
| WHERE json_extract(entry.value, '$.type') = 'tool-call' | ||
| AND json_extract(entry.value, '$.toolName') IN ('get_pull_request_comment', 'get_issue_comment') |
There was a problem hiding this comment.
⚫️ [PROBLEM] EXISTS guard and SET subquery disagree, so mixed logs stay NULL forever
The new EXISTS guard (activity.service.ts lines 278-286) only requires that some tool-call entry has an extractable commentId, but the SET subquery above still selects via ORDER BY entry.key LIMIT 1 with no non-null predicate in its WHERE (lines 270-271). When the first matching entry has an unextractable commentId (truncated/invalid message, or valid JSON without a commentId key) and a later entry has a valid one, EXISTS passes but the CASE expression evaluates to NULL, so target_comment_id stays NULL forever and that legacy row can never be retried — even though a recoverable commentId exists in its log.
Because SQLite counts every WHERE-matched row in result.changes, that same row is also counted as "recovered" by the startup log at apps/api/src/index.ts:54 while remaining unrecovered, so it is re-scanned on every boot — the same over-count/rescan class the EXISTS guard was meant to eliminate.
Add the identical non-null predicate to the SET subquery so both select the same row:
SET target_comment_id = (
SELECT CASE
WHEN json_valid(json_extract(entry.value, '$.message'))
THEN json_extract(json_extract(entry.value, '$.message'), '$.commentId')
END
FROM json_each(activity.logs) AS entry
WHERE json_extract(entry.value, '$.type') = 'tool-call'
AND json_extract(entry.value, '$.toolName') IN ('get_pull_request_comment', 'get_issue_comment')
AND CASE
WHEN json_valid(json_extract(entry.value, '$.message'))
THEN json_extract(json_extract(entry.value, '$.message'), '$.commentId')
END IS NOT NULL
ORDER BY entry.key
LIMIT 1
)There was a problem hiding this comment.
Right, and fixed in 317144c. Both queries now carry the same non-null predicate, so the update reads the entry the guard accepted instead of the first fetch of the log, and a mixed log is recovered instead of being written as NULL while counted as recovered. activity-retry.test.ts gains that exact case, a truncated first fetch followed by a readable second one, asserted at 1 change and comment id 77.
|
Thanks for the follow-up — every point from the last round is addressed: the reply-target guard now fails fast before provider setup, the phantom OverviewThis push renames migration 0047→0046 with a repaired snapshot chain, moves the missing-comment guard to the top of Good points
Main Issues
Additional Notes
|
| InstanceSettingUpdateInput, | ||
| UserResponse, | ||
| } from "@proval/types"; | ||
| import { resolveUserPromptMaxLength } from "@proval/types"; |
There was a problem hiding this comment.
⚫️ [PROBLEM] Unused resolveUserPromptMaxLength import fails lint in auth.service.ts
⚫️ [PROBLEM] This import is dead code and fails lint. resolveUserPromptMaxLength is imported here but never used in this file — the actual limit resolution at getMe goes through currentUserPromptMaxLength() from ../../util/user-prompt-limit.js (auth.service.ts:77). Since the repo's eslint config sets @typescript-eslint/no-unused-vars to "error" (packages/config/eslint/base.js:18), bun run lint on apps/api fails on this line. It also misleads readers into thinking auth.service resolves the limit itself. Remove the resolveUserPromptMaxLength import and keep only the currentUserPromptMaxLength import.
import type {
AuthCredentialInput,
AuthMeResponse,
InstanceSettingResponse,
InstanceSettingUpdateInput,
UserResponse,
} from "@proval/types";
import db from "../../db/index.js";
import { currentUserPromptMaxLength } from "../../util/user-prompt-limit.js";
| try { | ||
| selectResultQueue = [[instanceSetting], [{ value: 0 }]]; | ||
| const me = await authService.getMe(null); | ||
| expect(me.userPromptMaxLength).toBe(9000); |
There was a problem hiding this comment.
⚫️ [PROBLEM] Env-override test uses 9000, the same as the default, so it can't catch regressions
⚫️ [PROBLEM] This override test can't actually fail on a regression. It sets PROVAL_USER_PROMPT_MAX_LENGTH to "9000" and expects 9000, but 9000 is exactly the default USER_PROMPT_MAX_LENGTH (packages/types/src/user-prompt.ts:1). If getMe ignored the env var and always returned the constant, this test would still pass — the same is true of the other two tests in the file (both expect the default). Unlike repository.service.test.ts, which uses a distinctive "50", none of these tests can distinguish env-driven resolution from a hardcoded return. Use a non-default value, e.g. set the env to "1234" and expect 1234.
process.env.PROVAL_USER_PROMPT_MAX_LENGTH = "1234";
try {
selectResultQueue = [[instanceSetting], [{ value: 0 }]];
const me = await authService.getMe(null);
expect(me.userPromptMaxLength).toBe(1234);
} finally {
restoreUserPromptMaxLengthEnv(previousMaxLength);
}
|
Thanks for putting this together — this PR packs three well-scoped features: a per-provider streaming toggle ( Main Issues
Good points
Additional Notes
|
A failed pr_reply or issue_reply never offered Retry because the retry path only accepted pr_review and issue_open, and because the activity row did not keep the comment that started the reply. The webhook handlers now store targetCommentId and targetInlineReviewId at start, retry resolves the target comment before any provider setup and replays runPullRequestReply or runIssueReply with it, and the review page shows the button for both reply types. The boot repair reads the comment id back from the agent log of a reply created before this change, so an old failed reply becomes retryable too.
1c30c07 to
317144c
Compare
|
Last open finding from that round is fixed in 317144c, and the added test case covers the mixed log it pointed at. Nothing else in that round was still live. On the 16:07 review, it read commit 4f997b9, the first head of this branch before it was rebased onto main. That old diff carried #71 and #73 along with this work, which is where the streaming toggle, the The one historical note worth keeping is the guard ordering, and that was already fixed in this branch before the review was posted. |
|
|
||
| await db.update(activityTable).set({ status: "failed" }).where(eq(activityTable.status, "started")); | ||
|
|
||
| const recoveredReplyTargetCount = recoverLegacyReplyTargetComment(); |
There was a problem hiding this comment.
⚫️ [PROBLEM] Unwrapped legacy backfill at boot can hard-crash dev against a pre-0046 database
⚫️ [PROBLEM] recoverLegacyReplyTargetComment() is an optional legacy backfill, but at apps/api/src/index.ts:52 it runs raw SQL referencing target_comment_id with no error handling, immediately before Bun.serve. In dev the boot path never migrates — migrate() only runs under NODE_ENV === "production" (apps/api/src/index.ts:26), and the dev workflow relies on the separate manual drizzle-kit migrate script (apps/api/package.json). Starting the API against a local.db created before this PR therefore throws an uncaught no such column: target_comment_id and the process exits before the servers start, even though the backfill is strictly optional. Production is safe because migrate() runs first and hard-fails on its own, but any dev or stale-DB boot — or a transient SQLite error such as a locked database — takes the whole API down. This also breaks the pattern set a few lines above, where clearWorkspaceRoot is wrapped in try/catch with logError (lines 17–22) precisely so a nonessential boot step cannot block serving. Wrap the recovery the same way.
try {
const recoveredReplyTargetCount = recoverLegacyReplyTargetComment();
if (recoveredReplyTargetCount > 0) {
log(pc.bgGreen(pc.bold(` Recovered ${recoveredReplyTargetCount} legacy reply comment link(s) `)));
}
} catch (error) {
logError("Failed to recover legacy reply comment links", error);
}
|
Thanks for the cleanup pass — rebasing onto main so this branch is a single commit over Main Issues
Good points
Additional Notes
|
|
I don't think wrapping this in a try/catch is necessarily the right fix. If the database schema is outdated, other parts of the application may also fail when accessing the missing columns. Silently skipping the recovery would allow the server to start in a potentially inconsistent state. Failing fast on an unmigrated database seems preferable unless the project explicitly supports running against outdated schemas. I would consider this a non-blocking observation rather than a bug in this PR. The expected migration workflow should be clarified by the maintainers before changing the startup behavior. |
|
Thanks for using Proval and even making many contributions. I made a addtional updates to resolve migration conflict and some small code change, but I can't push to pharmania/proval, it's rejected with 403. maybe because it's from I pushed my change to this repository so you can see the updates. check branch If you let me push to your forked repository, I'll push it and merge it Changes
Please let me know your opinion and push my commit to your fork |
|
@seoes Maybe you can re-push his fork to a new branch in the main repo. |
4 participants
A failed reply never offered Retry. The retry endpoint only accepted
pr_reviewandissue_open, and the activity row did not keep the comment the reply answered, so a reply had nothing to replay.target_comment_idandtarget_inline_review_id(migration0046), filled by the GitHub, GitLab and Forgejo webhook handlers when they start apr_replyorissue_replyrunPullRequestReplywith the stored inline review id, orrunIssueReplyactivity-retry.test.tscovers both reply types, the issue open path, the missing comment case and the log recoveryTests.
bun run --filter api test58 pass,bun run --filter client test9 pass,bun run typecheck0 errors. A deployment needsbun run migratefor the two new columns.Migration number. It takes
0046, the next number onmain. #71 also carries a0046, so the one that lands second takes0047.