Readme.md update - added Qodo highlights. - #20
Closed
JAIVIGNESH2002 wants to merge 3 commits into
Closed
JAIVIGNESH2002 wants to merge 3 commits into
JAIVIGNESH2002 wants to merge 3 commits into
Conversation
PR Summary by QodoBound run-store lifecycles and document Qodo review highlights
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Inspection bypasses active limit
|
Comment on lines
+431
to
+435
| if (record.status === "running" && now - record.updatedAtMs > runningTimeoutMs) { | ||
| record.status = "completed"; | ||
| record.outcome = "interrupted"; | ||
| record.message = | ||
| "Upgrade verification was interrupted after the run stopped reporting progress."; |
There was a problem hiding this comment.
1. Timeout result is not terminal 📘 Rule violation ☼ Reliability
Stale-run pruning marks an upgrade as completed/interrupted, but the still-pending completeUpgradeRun workflow does not re-check that terminal state after its awaits and can overwrite it with a late verification or repair result. This makes timeout handling unreliable and can present a timed-out run as verified, blocked, or repair-failed instead of interrupted.
Agent Prompt
## Issue description
A verification or repair promise may resolve after stale-run pruning has marked its upgrade run completed/interrupted, allowing the in-flight async completion path to overwrite the terminal timeout result.
## Issue Context
Treat timeout/interruption as a terminal state. The current completion flow reads and checks the record before awaiting verification, while status polling can prune the same record during that await; later result assignment and persistence occur without confirming that the run has not timed out. Re-check the current stored record after asynchronous verification returns, or use a generation/cancellation guard so late work cannot commit results. Add coverage for a promise that resolves after pruning, rather than only a never-resolving promise.
## Fix Focus Areas
- src/lib/upgrade-run-store.ts[275-345]
- src/lib/upgrade-run-store.ts[430-440]
- src/lib/upgrade-run-store.test.ts[84-108]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+201
to
+205
| if (record.status === "running" && now - record.updatedAtMs > runningTimeoutMs) { | ||
| record.status = "completed"; | ||
| record.updatedAtMs = now; | ||
| record.baseline = interruptedWorkspaceBaseline( | ||
| "Baseline verification was interrupted after the run stopped reporting progress." |
There was a problem hiding this comment.
2. Baseline timeout can be overwritten 📘 Rule violation ☼ Reliability
pruneBaselineRuns marks a stale baseline run completed/interrupted but leaves its asynchronous verification active, allowing completeBaselineRun to overwrite the same record with a late result and persist it again. Consequently, a timed-out run can later appear healthy or failed, be resurrected after eviction, and continue consuming capacity, so consumers cannot rely on the timeout outcome as terminal.
Agent Prompt
## Issue description
A baseline verification can resolve after timeout pruning marks its run `completed`/`interrupted`, allowing the detached completion path to overwrite that terminal result and persist or reinsert the run.
## Issue Context
Timeout pruning and `completeBaselineRun` share the same mutable record, while verification continues asynchronously after request handling. Make interruption terminal by checking the current stored status after verification resolves or by introducing cancellation/generation state, and add a regression test that resolves verification after the interrupted timeout result has been observed.
## Fix Focus Areas
- src/lib/baseline-run-store.ts[134-162]
- src/lib/baseline-run-store.ts[200-209]
- src/lib/baseline-run-store.test.ts[107-123]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+38
to
+41
| const DEFAULT_RUN_RETENTION_MS = 60 * 60 * 1000; | ||
| const DEFAULT_MAX_RUNS = 100; | ||
| const DEFAULT_RUNNING_RUN_TIMEOUT_MS = 30 * 60 * 1000; | ||
| const DEFAULT_MAX_ACTIVE_RUNS = 25; |
There was a problem hiding this comment.
3. Run retention is unrelated 📘 Rule violation ⚙ Maintainability
The PR is documented as a README-only Qodo-highlights update, but it also adds production run retention, timeout, and concurrency-limit behavior. This is a separate logical concern that should be moved to a dedicated PR with its own description and verification.
Agent Prompt
## Issue description
The change set combines the declared README Qodo-highlights update with unrelated production run-store retention and concurrency behavior.
## Issue Context
Keep this PR focused on documentation, or update the change organization by moving the run-store implementation and tests to a separately described and verified PR.
## Fix Focus Areas
- src/lib/baseline-run-store.ts[38-41]
- src/lib/upgrade-run-store.ts[103-106]
- src/lib/run-store-retention.ts[1-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return fallback; | ||
| } | ||
|
|
||
| return Number(rawValue); |
There was a problem hiding this comment.
4. Oversized values disable limits 🐞 Bug ☼ Reliability
readPositiveIntegerEnv accepts arbitrary-length digit strings and returns Number(rawValue) without requiring a positive finite safe integer, so oversized values can become Infinity or lose precision. Used for retention, timeout, maximum stored runs, or maximum active runs, such values make the relevant comparisons false and silently disable the controls in both run stores instead of falling back to a safe configuration.
Agent Prompt
## Issue description
Very large digit-only environment values pass validation but can convert to `Infinity` or unsafe integers, silently disabling resource-retention, timeout, capacity, and concurrency limits.
## Issue Context
The shared parser feeds baseline and upgrade run-store retention, timeout, maximum-size, and active-run comparisons. Parse the value once, then fall back unless the result is a positive finite safe integer; optionally impose setting-specific upper bounds.
## Fix Focus Areas
- src/lib/run-store-retention.ts[1-9]
- src/lib/baseline-run-store.ts[188-220]
- src/lib/upgrade-run-store.ts[418-452]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+54
to
+56
| if ( | ||
| activeBaselineRunCount() >= | ||
| readPositiveIntegerEnv("UPGRADEPILOT_MAX_ACTIVE_RUNS", DEFAULT_MAX_ACTIVE_RUNS) |
There was a problem hiding this comment.
5. Inspection bypasses active limit 🐞 Bug ☼ Reliability
startBaselineRun checks the active-run count before awaiting repository inspection but does not reserve a slot or create a running record until afterward. Concurrent baseline requests can therefore all observe available capacity, fan out unbounded GitHub inspections, and potentially insert enough running records to exceed UPGRADEPILOT_MAX_ACTIVE_RUNS before later verification rejects requests.
Agent Prompt
## Issue description
The baseline active-run cap does not cover asynchronous repository inspection because concurrent starts can all pass the active-run check before any slot is reserved. This allows requests to fan out GitHub API work and potentially exceed `UPGRADEPILOT_MAX_ACTIVE_RUNS`.
## Issue Context
The POST route independently awaits `startBaselineRun` for each request, and each invocation reaches the `inspectRepository` await before a running record is stored. Admission and reservation must occur synchronously before the first await, with the reservation released or transitioned when inspection fails or the repository is unsupported.
## Fix Focus Areas
- src/lib/baseline-run-store.ts[52-99]
- src/app/api/repositories/baseline/runs/route.ts[5-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added Qodo highlights to readme.md,