Batch file reviews with structured Gemini output, polish UI, and swap syntax highlighter - #77
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![codra-app-personal[bot]](https://avatars.githubusercontent.com/in/3347243?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
4 findings not posted - mostly over the max_comments cap (10) (4). Per-file detail below.
| // Clears a label once it reaches a terminal state, so a retry of the job starts from the primary. | ||
| async clear(label: string): Promise<void> { | ||
| const progress = await this.load(); | ||
| progress.delete(label); |
There was a problem hiding this comment.
The clear method modifies the in-memory progress map by deleting a label, but it does not trigger a persistence operation to KV. As a result, the changes made by clear are not durable. When a new job invocation loads the chain progress from KV, it will retrieve the old state, meaning the 'cleared' label will still be present. This defeats the purpose of clearing the label, as a job retry for that label will not start from the primary model as intended, but rather from the previously recorded index. This is a logical bug that leads to incorrect resumption behavior for job retries.
| progress.delete(label); | |
| progress.delete(label); | |
| this.dirty = true; | |
| return this.flush(); |
|
|
||
| reviews = await getFileReviewsForJobs(env, [job.id]); | ||
| } else { | ||
| } else if (reviews.length < files.length) { |
There was a problem hiding this comment.
The else if (reviews.length < files.length) condition at line 55 is unreachable. The preceding if (missingFiles.length > 0) block handles cases where files are not reviewed. If missingFiles.length is 0, it implies that every file path in files has a corresponding entry in reviews. This means reviews.length must be greater than or equal to files.length. Consequently, the condition reviews.length < files.length cannot be true if missingFiles.length is 0. This renders the await updateJobStep and await enqueueJobPhase calls within this else if block unreachable, potentially preventing jobs from being correctly re-enqueued for review under certain edge cases where reviews.length < files.length for reasons not captured by missingFiles (e.g., duplicate file paths in files array).
| } else if (reviews.length < files.length) { | |
| Consider removing the `else if` block as it appears to be unreachable, or re-evaluating the conditions to ensure all cases where a job needs to bounce back to review are covered logically. |
| export function langForPath(path: string): Lang { | ||
| const ext = path.split('.').pop()?.toLowerCase() ?? ''; | ||
| return EXT_LANG[ext] ?? 'plain'; | ||
| return normalizeLang(ext) ?? EXTRA_EXT[ext]; |
There was a problem hiding this comment.
The previous EXT_LANG mapping explicitly supported .mdx and .mdc files by mapping them to md for highlighting. With the new langForPath implementation, normalizeLang(ext) from sugar-high does not recognize mdx or mdc extensions. Furthermore, these extensions are not included in the EXTRA_EXT fallback. Consequently, files with these extensions will no longer receive syntax highlighting and will be rendered as plain text, which is a functional regression.
| return normalizeLang(ext) ?? EXTRA_EXT[ext]; | |
| const EXTRA_EXT: Record<string, LanguageName> = { | |
| // ... existing mappings ... | |
| mdx: 'markdown', mdc: 'markdown', | |
| }; |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
3 findings not posted - mostly unconfirmed against the diff (1), 2 other reasons. Per-file detail below.
| out.push(full); | ||
| } | ||
| } | ||
| if (!lang || text.length === 0 || text.length > 1000) return text; |
There was a problem hiding this comment.
The previous implementation included specific logic in highlightMarkdownLine to highlight structural elements in Markdown files such as headings, blockquotes, and list markers using distinct CSS classes (tok-md-head, tok-com, tok-kw, tok-num). The new sugar-high based solution replaces this custom logic. While sugar-high might provide generic markdown highlighting, it is unlikely to preserve the specific visual distinctions for these structural elements that the custom regex-based parser provided, leading to a functional regression in how markdown diffs are presented.
| // Extensions sugar-high's own alias table doesn't cover, mapped to the closest supported grammar. | ||
| const EXTRA_EXT: Record<string, LanguageName> = { | ||
| mjs: 'javascript', cjs: 'javascript', mts: 'typescript', cts: 'typescript', | ||
| vue: 'html', svelte: 'html', svg: 'html', |
There was a problem hiding this comment.
The previous EXT_LANG mapping (deleted lines 13-23) assigned .vue and .svelte files to JavaScript ('js') for highlighting. The new EXTRA_EXT map now assigns them to HTML ('html'). While these files contain HTML, they also frequently include significant JavaScript/TypeScript and CSS blocks. Treating the entire file as HTML will result in a loss of syntax highlighting for the script and style sections, which were previously highlighted as JavaScript, degrading the readability of these common component file types in the diff viewer.
| // Clears a label once it reaches a terminal state, so a retry of the job starts from the primary. | ||
| async clear(label: string): Promise<void> { | ||
| const progress = await this.load(); | ||
| progress.delete(label); |
There was a problem hiding this comment.
The clear method updates the in-memory progress map by deleting a specific label. However, it neither sets the this.dirty flag to true nor calls this.flush(). As a result, the deletion is not persisted to the KV store. If the server process terminates after clear is called but before any other operation (like advance or noteTimeout) triggers a flush, the cleared progress will reappear on the next load, causing the job to potentially re-process work that was intended to be marked as complete.
| progress.delete(label); | |
| async clear(label: string): Promise<void> { | |
| const progress = await this.load(); | |
| if (progress.has(label)) { | |
| progress.delete(label); | |
| this.dirty = true; | |
| return this.flush(); | |
| } | |
| } |
| // "no model was attempted" instead of just re-running the last one. | ||
| // The MINIMUM across members: a file that has not yet been tried against model k must not have k | ||
| // skipped just because a bin-mate already ruled it out. | ||
| const recorded = await Promise.all(progressLabels.map((key) => ctx.chainProgress.startIndexFor(key))); |
There was a problem hiding this comment.
The Promise.all call on line 66 will reject immediately if any individual call to ctx.chainProgress.startIndexFor(key) rejects. This could lead to the runModelChain function terminating prematurely, potentially with an unhandled rejection, before any model is even attempted. It's often more robust to handle individual promise rejections within a Promise.allSettled or to wrap the mapping function with a try/catch to log errors and default to a safe value (e.g., 0) for a specific key, allowing the model chain to proceed with potentially incomplete progress information rather than failing entirely.
| const recorded = await Promise.all(progressLabels.map((key) => ctx.chainProgress.startIndexFor(key))); | |
| const results = await Promise.allSettled(progressLabels.map((key) => ctx.chainProgress.startIndexFor(key))); | |
| const recorded = results.map(r => r.status === 'fulfilled' ? r.value : 0); // Log rejections here if needed. |
| // Only when there is somewhere left to go: at the end of the chain the memo would pin every | ||
| // future attempt to the last entry, and the file should get a clean walk instead. | ||
| if (attemptedFailedThrough > 0 && attemptedFailedThrough < wholeChain.length) { | ||
| for (const key of progressLabels) await ctx.chainProgress.advance(key, attemptedFailedThrough); |
There was a problem hiding this comment.
The for...of loop with await on line 237 will stop at the first ctx.chainProgress.advance call that rejects. This means that if an error occurs while persisting progress for one key, the subsequent progressLabels will not have their progress updated. This can lead to an inconsistent state where some labels are advanced while others are not, affecting the accuracy of chain resumption in subsequent retries. Furthermore, this error will mask the original RetryableModelError that was being prepared, propagating a potentially less informative error.
| for (const key of progressLabels) await ctx.chainProgress.advance(key, attemptedFailedThrough); | |
| await Promise.allSettled(progressLabels.map((key) => ctx.chainProgress.advance(key, attemptedFailedThrough))); |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
1 finding not posted - mostly not grounded in a quoted line (1). Per-file detail below.
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| // Explicitly fail fast for timeouts so they don't loop endlessly, aligning with | ||
| // isTransientModelFailure which prevents timeouts from being retried. | ||
| // Our own markers win over the heuristics below: messages interpolate file paths, so a path like `core/timeout.ts` would otherwise decide retry behaviour by coincidence. | ||
| if (lower.includes('retrying later') || lower.includes('all configured review models failed')) { |
There was a problem hiding this comment.
The new early return for retrying later or all configured review models failed takes precedence over the subsequent isTimeoutMessage check. This means if a message contains both a retryable phrase (e.g., 'retrying later') AND a timeout indicator, it will be marked as retryable, contradicting the explicit comment to 'fail fast for timeouts so they don't loop endlessly'. This could lead to infinite retry loops for certain timeout messages.
| if (lower.includes('retrying later') || lower.includes('all configured review models failed')) { | |
| The order of checks should prioritize the `isTimeoutMessage` to ensure that timeouts are always treated as non-retryable, regardless of other phrases in the error message. Alternatively, `isTimeoutMessage` should be incorporated into the initial `if` statement with an `&&` condition to ensure that the error is only retryable if it does NOT contain a timeout message. |
| * Builds the collapsed directory tree the diff viewer's file list renders. Pure: single-child | ||
| * directory chains are compressed into one "a/b/c" node, and directories sort before files. | ||
| */ | ||
| /** Builds the collapsed directory tree the diff viewer's file list renders. */ |
There was a problem hiding this comment.
The JSDoc comment was significantly shortened, removing critical information about the file-tree module's behavior. Specifically, the details that 'single-child directory chains are compressed into one "a/b/c" node, and directories sort before files' are important invariants of this module. Removing them makes the code's behavior less clear to future maintainers, potentially leading to incorrect assumptions or bugs if this logic is ever modified without full context.
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
2 findings not posted - mostly not grounded in a quoted line (2). Per-file detail below.
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
5 findings not posted - mostly not grounded in a quoted line (4), 1 other reason. Per-file detail below.
| } | ||
|
|
||
| // Clears a label once it reaches a terminal state, so a retry of the job starts from the primary. | ||
| async clear(label: string): Promise<void> { |
There was a problem hiding this comment.
The clear method deletes an entry from the in-memory progress map, but never sets this.dirty = true nor calls this.flush(). As a result, clearing a label only affects the local instance and is never persisted to KV, meaning subsequent reads or concurrent runs will still see the old value.
| async clear(label: string): Promise<void> { | |
| async clear(label: string): Promise<void> { | |
| const progress = await this.load(); | |
| if (progress.delete(label)) { | |
| this.dirty = true; | |
| return this.flush(); | |
| } | |
| } |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
4 findings not posted - mostly not grounded in a quoted line (4). Per-file detail below.
| } | ||
|
|
||
| // Clears a label once it reaches a terminal state, so a retry of the job starts from the primary. | ||
| async clear(label: string): Promise<void> { |
There was a problem hiding this comment.
In ModelChainProgressStore.clear(label), the method loads the progress map and deletes the label, but it neither sets this.dirty = true nor calls this.flush(). As a result, clearing a label only mutates the in-memory cache and never persists the deletion to KV storage.
| async clear(label: string): Promise<void> { | |
| async clear(label: string): Promise<void> { | |
| const progress = await this.load(); | |
| if (progress.delete(label)) { | |
| this.dirty = true; | |
| return this.flush(); | |
| } | |
| } |
| // Only when there is somewhere left to go: at the end of the chain the memo would pin every | ||
| // future attempt to the last entry, and the file should get a clean walk instead. | ||
| if (attemptedFailedThrough > 0 && attemptedFailedThrough < wholeChain.length) { | ||
| for (const key of progressLabels) await ctx.chainProgress.advance(key, attemptedFailedThrough); |
There was a problem hiding this comment.
Using await inside a standard for...of loop over progressLabels executes sequentially, which is fine, but if progressLabels contains multiple items, throwing the error immediately after in a synchronous manner without proper error handling or parallel execution considerations could hide issues. More importantly, calling await inside a loop without Promise.all can cause unnecessary serial execution. Wait, looking closely at for (const key of progressLabels) await ctx.chainProgress.advance(key, attemptedFailedThrough);, each iteration is awaited sequentially. However, if any advance fails, subsequent keys are skipped and the error is thrown. This may be intended, but using Promise.all(progressLabels.map(...)) as done elsewhere on lines 66 and 167 is the established pattern in this file and avoids serial latency.
| for (const key of progressLabels) await ctx.chainProgress.advance(key, attemptedFailedThrough); | |
| await Promise.all(progressLabels.map((key) => ctx.chainProgress.advance(key, attemptedFailedThrough))); |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| transient_error_count | ||
| ) | ||
| VALUES ($1::uuid, $2, 'failed', $3, $4, $5, $6, NULL, NULL, NULL, $7, NULL, NULL, NULL, NULL, $8, 1) | ||
| VALUES ($1::uuid, $2, 'failed', $3, $4, $5, $6, NULL, NULL, NULL, $7, NULL, NULL, NULL, NULL, $8, $9::int) |
There was a problem hiding this comment.
The query references $9::int for transient_error_count in both the INSERT and UPDATE clauses, but the parameter array passed to queryTransaction only provides 8 parameters ($1 through $8), resulting in an unsupplied parameter error at runtime.
| VALUES ($1::uuid, $2, 'failed', $3, $4, $5, $6, NULL, NULL, NULL, $7, NULL, NULL, NULL, NULL, $8, $9::int) | |
| VALUES ($1::uuid, $2, 'failed', $3, $4, $5, $6, NULL, NULL, NULL, $7, NULL, NULL, NULL, NULL, $8, $8::int) |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
6 findings not posted - mostly unconfirmed against the diff (3), 2 other reasons. Per-file detail below.
| // Clears a label once it reaches a terminal state, so a retry of the job starts from the primary. | ||
| async clear(label: string): Promise<void> { | ||
| const progress = await this.load(); | ||
| progress.delete(label); |
There was a problem hiding this comment.
The 'clear' method removes a label from the 'progress' Map but does not call 'this.flush()' or 'this.writeOnce()'. As a result, the removal is never persisted to KV storage, meaning a subsequent job retry will still read the stale state from KV.
| progress.delete(label); | |
| async clear(label: string): Promise<void> { | |
| const progress = await this.load(); | |
| progress.delete(label); | |
| this.dirty = true; | |
| await this.flush(); | |
| } |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc91f7c4d
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
5 findings not posted - mostly not grounded in a quoted line (4), 1 other reason. Per-file detail below.
| // Clears a label once it reaches a terminal state, so a retry of the job starts from the primary. | ||
| async clear(label: string): Promise<void> { | ||
| const progress = await this.load(); | ||
| progress.delete(label); |
There was a problem hiding this comment.
The 'clear' method removes an entry from the 'progress' Map but fails to set the 'dirty' flag or trigger 'this.flush()'. Consequently, the change is never persisted to the KV store, rendering the 'clear' operation ineffective at stopping retries from the previously cached progress index.
| progress.delete(label); | |
| progress.delete(label); | |
| this.dirty = true; | |
| return this.flush(); |
|
|
||
| if (!entries?.length) { | ||
| // No usable `files`, but a findings array is present: the flat shape, not a lost response. | ||
| if (Array.isArray(root.findings)) return { shape: 'flat', data: parseRawPayload(raw) }; |
There was a problem hiding this comment.
The function parseRawPayload(raw) is already defined in the imported local scope and is documented as a single-file parser. Calling parseRawPayload(raw) here with the same raw input that was previously passed to extractJson (which uses regex/substring logic) and then processed by jsonrepair is redundant and potentially problematic if parseRawPayload expects a clean string rather than one potentially containing remnants of the batch-response wrapper. If the goal is to handle the flat shape, it should likely be parsed from the already extracted/repaired parsedJson or repaired string instead of re-parsing the original raw payload.
| if (Array.isArray(root.findings)) return { shape: 'flat', data: parseRawPayload(raw) }; | |
| Instead of passing raw, pass the structured data already parsed: data: parseRawPayload(JSON.stringify(root)) |
1 participant
Summary
sugar-high, removing ~200 lines of hand-rolled tokenization while keeping the existing theme-aware color tokens.Details
bin-runner,pack,json-batch, andmodel-chain-progress/model-review-chainmodules batch file reviews and track chain state;model-output,finding-gates, and prompts updated to support structured (schema-validated) Gemini output.file-reviews-bulkbulk upsert path,003_grounding.sqlmigration.gemini-schemamodel helper.highlight.tsxnow delegates tokenization tosugar-high/core, with CSS variables (--sh-*) replacing the old.tok-*classes.batch-flow,batch-grouping,bulk-upsert,chain-progress-store,chain-resume,gemini-schema,output-batch,pack,prompts-batch-review,batch-routing.Closes #43
Part of #40
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist: