test(qualification): close greenfield Phase 0 - #391
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds qualification workflows for frontend builds, chat batching, OpenClaw parity, SQLite outbox behavior, resource budgets, shutdown, and WebSocket protocols. It also adds Effect-based lifecycle management, coordinated server shutdown, exact dependency pins, build-plugin ordering, and architecture documentation updates. ChangesQualification and runtime changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mira-2026
changed the title
mira-2026
added
type: tests
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
qualification/parity/parityInventory.test.ts-98-103 (1)
98-103: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGive this parity test a timeout above the probe budget.
loadLegacyBackendRouteIdentitieswaits onBun.spawnSyncwith a 5000 ms subprocess timeout, while Bun tests also default to 5000 ms. Add an explicit test timeout larger than 5000 ms so probe failures can surface instead of being masked by a test timeout.♻️ Proposed change
- test("accounts for every executable backend route and documented row exactly once", async () => { + test("accounts for every executable backend route and documented row exactly once", async () => {).toHaveLength(0); - }); + }, 20_000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/parity/parityInventory.test.ts` around lines 98 - 103, Set an explicit timeout greater than 5000 ms on the test named “accounts for every executable backend route and documented row exactly once,” ensuring the loadLegacyBackendRouteIdentities probe can reach its timeout and report failures without the test runner timing out first.qualification/openclaw/sourceAudit.ts-1016-1020 (1)
1016-1020: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBind
promptChars: 4000to a source marker.
promptCharsis emitted by the reviewer, but no marker in the installed OpenClaw artifacts ties it to the source. Add the OpenClaw constant or literal that sets the bounded task prompt length to the relevant task protocol/handler assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/openclaw/sourceAudit.ts` around lines 1016 - 1020, Update the OpenClaw source-audit assertion around promptVisibility to include the source marker for the constant or literal that sets the bounded task prompt length to 4000. Bind promptChars to the relevant task protocol or handler assertion, preserving the existing visibility flags and value.qualification/budgets/resourceBudgetPolicy.test.ts-217-220 (1)
217-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest
unitCollectedseparately.This case changes
cgroupRemoved, notunitCollected. It duplicates the cgroup cleanup condition and does not verify thatassessResourceBudgetEvidencerejects an uncollected unit.Proposed fix
[ - "unit was not collected", + "cgroup was not removed", (candidate) => { candidate.cgroupRemoved = false; }, ], + [ + "unit was not collected", + (candidate) => { + candidate.unitCollected = false; + }, + ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/budgets/resourceBudgetPolicy.test.ts` around lines 217 - 220, Update the test case around the “unit was not collected” candidate mutation so it sets unitCollected to false instead of cgroupRemoved. Keep cgroup cleanup coverage in its separate test and ensure this case verifies that assessResourceBudgetEvidence rejects an uncollected unit.qualification/budgets/resourceBudgetOrchestration.ts-406-447 (1)
406-447: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the launcher exit code before reading the unit report.
readResultruns at Line 418, inside the scope. The launcher exit-code check runs at Line 432, after the scope closes.If the launcher fails, the unit writes no report.
readResultthen fails first with"Resource-budget unit did not write a report". The diagnostic block at Lines 433-439, which carries the launcher stderr and stdout, never executes. The operator sees a missing-report error instead of the actual launch failure.Move the exit-code check into the scope, directly after
runBoundedProcess.🐛 Proposed fix to surface the launcher failure first
const completed = yield* Effect.scoped( Effect.gen(function* () { yield* transientUnitResource(command); const limits = resourceBudgetPolicy.scenarios[scenarioId].limits; const launcher = yield* runBoundedProcess( command.argv, command.environment, resourceBudgetPolicy.launcherOutputMaxBytes, limits.outerDeadlineMs, "run-transient-unit", scenarioId ); + if (launcher.exitCode !== 0) { + const diagnostic = [launcher.stderr.trim(), launcher.stdout.trim()] + .filter((value) => value.length > 0) + .join("\n") + .slice(0, 16 * 1024); + return yield* Effect.fail( + new ResourceBudgetOrchestrationError({ + cause: diagnostic, + operation: "transient-unit-exit", + scenarioId, + }) + ); + } const report = yield* readResult(command); return { launcher, report }; }) ); const [unitCollected, cgroupRemoved] = yield* Effect.all( [unitIsCollected(command), cgroupIsRemoved(cgroupPath)] as const, { concurrency: "unbounded" } ); - const evidence: ResourceBudgetScenarioEvidence = { + return { cgroupRemoved, launcherExitCode: completed.launcher.exitCode, report: completed.report, unitCollected, - }; - if (completed.launcher.exitCode !== 0) { - const diagnostic = [ - completed.launcher.stderr.trim(), - completed.launcher.stdout.trim(), - ] - .filter((value) => value.length > 0) - .join("\n") - .slice(0, 16 * 1024); - return yield* Effect.fail( - new ResourceBudgetOrchestrationError({ - cause: diagnostic, - operation: "transient-unit-exit", - scenarioId, - }) - ); - } - return evidence; + } satisfies ResourceBudgetScenarioEvidence;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/budgets/resourceBudgetOrchestration.ts` around lines 406 - 447, Move the launcher exit-code validation into the Effect.scoped generator immediately after runBoundedProcess and before readResult. Preserve the existing diagnostic construction and ResourceBudgetOrchestrationError with operation "transient-unit-exit", and return the completed launcher/report only when the launcher succeeds.qualification/outbox/sqliteOutboxQualification.test.ts-74-86 (1)
74-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe sentinel error is swallowed by its own
catch, and the competing transaction leaks.If
competingWriter.run("BEGIN IMMEDIATE")succeeds, the code throws"Competing writer unexpectedly acquired WAL lock". The adjacentcatchreceives that sameError.classifyQualificationSqliteErrorthen returnsundefinedbecause the value has noSQLITE_code. The assertion at Line 99 still fails, but the reported cause becomes "received undefined" instead of the explicit sentinel message.On that path
competingWriteralso stays inside an openBEGIN IMMEDIATEtransaction. Thefinallyblock rolls back onlywriter.Track the acquisition outside the
catchand roll back both connections.🐛 Proposed fix for the sentinel and the leaked transaction
const contention = yield* Effect.sync(() => { writer.run("BEGIN IMMEDIATE"); + let competingWriterAcquired = false; try { competingWriter.run("BEGIN IMMEDIATE"); - throw new Error( - "Competing writer unexpectedly acquired WAL lock" - ); + competingWriterAcquired = true; + return undefined; } catch (error) { return classifyQualificationSqliteError(error); } finally { + if (competingWriterAcquired) competingWriter.run("ROLLBACK"); writer.run("ROLLBACK"); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/outbox/sqliteOutboxQualification.test.ts` around lines 74 - 86, Update the contention logic in the Effect.sync block to track whether competingWriter.run("BEGIN IMMEDIATE") succeeds, only classify errors from the competing transaction attempt, and preserve the explicit sentinel error for unexpected lock acquisition. In the finally block, roll back writer and also roll back competingWriter when it successfully began a transaction.qualification/budgets/resourceBudgetUnit.ts-430-439 (1)
430-439: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe reported limits use unchecked casts on
finalCgroupand a hardcodedoomGroup.Two problems exist in the
limitsblock.First,
cpuQuotaMicros,memoryHighBytes,memoryMaxBytes,memorySwapMaxBytes, andpidsMaxare typed asnumber | "max". The guard at Lines 365-378 validatedinitialCgroup, notfinalCgroup. Theas numbercasts therefore suppress the union without any runtime check. If a final read returns"max",writeReportserializes the string.parseResourceBudgetUnitReportinqualification/budgets/resourceBudgetPolicy.tsthen rejects the report with a Valibot error, so the orchestrator reports a parse failure instead of a cgroup-policy failure.Second,
oomGroup: trueis a literal.assertObservedLimitschecks!observed.oomGroupat Line 298 ofqualification/budgets/resourceBudgetPolicy.ts, and the schema usesv.literal(true). Both checks are therefore tautological for the final state.Validate the final limits in the unit, and carry the observed
oomGroupvalue.🐛 Proposed fix to validate the final cgroup limits
+ if ( + finalCgroup.cpuQuotaMicros === "max" || + finalCgroup.memoryHighBytes === "max" || + finalCgroup.memoryMaxBytes === "max" || + finalCgroup.memorySwapMaxBytes === "max" || + finalCgroup.pidsMax === "max" || + !finalCgroup.oomGroup + ) { + return yield* Effect.fail( + new ResourceBudgetUnitError({ operation: "verify-final-cgroup-policy" }) + ); + } const report: ResourceBudgetUnitReport = { @@ limits: { cpuPeriodMicros: finalCgroup.cpuPeriodMicros, - cpuQuotaMicros: finalCgroup.cpuQuotaMicros as number, - memoryHighBytes: finalCgroup.memoryHighBytes as number, - memoryMaxBytes: finalCgroup.memoryMaxBytes as number, - memorySwapMaxBytes: finalCgroup.memorySwapMaxBytes as number, - oomGroup: true, - pidsMax: finalCgroup.pidsMax as number, + cpuQuotaMicros: finalCgroup.cpuQuotaMicros, + memoryHighBytes: finalCgroup.memoryHighBytes, + memoryMaxBytes: finalCgroup.memoryMaxBytes, + memorySwapMaxBytes: finalCgroup.memorySwapMaxBytes, + oomGroup: finalCgroup.oomGroup, + pidsMax: finalCgroup.pidsMax, },The explicit
=== "max"comparisons narrow each union, so the casts become unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/budgets/resourceBudgetUnit.ts` around lines 430 - 439, Update the limits construction around finalCgroup to validate the final values rather than relying on casts: explicitly reject or handle "max" for cpuQuotaMicros, memoryHighBytes, memoryMaxBytes, memorySwapMaxBytes, and pidsMax so the reported fields remain numeric. Replace the hardcoded oomGroup: true with the observed finalCgroup.oomGroup value, preserving schema and assertObservedLimits validation.qualification/build/frontendBuildQualification.test.ts-12-12 (1)
12-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert production hashed assets are present
The
.every(...)assertion passes on an empty filtered array, so this check does not prove that any.css/.jsoutput is hash-busted. Assertproduction.outputPathscontains at least one production asset before checking thehashedAssetPattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/build/frontendBuildQualification.test.ts` at line 12, Update the production asset assertion using hashedAssetPattern to first verify that production.outputPaths contains at least one matching CSS or JavaScript asset, then apply the existing hash-pattern validation so an empty filtered array cannot pass.qualification/build/frontendBuildQualification.test.ts-70-81 (1)
70-81: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the React Compiler assertion depend on the unminified output.
buildQualificationFrontendruns production withminify: true, and the compiler imports the cache function locally before calling it. Minification can rename that local binding, souseMemoCacheis not a stable production-bundle marker. Assert the compiler ran against the unminified development bundle, or assert a stable specifier such as the compiler-runtime import string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/build/frontendBuildQualification.test.ts` around lines 70 - 81, Update the React Compiler assertion in the frontend build qualification test to inspect unminified development output or a stable compiler-runtime import specifier instead of requiring the production bundle to contain the local name “useMemoCache”. Keep the stylesheet assertion unchanged and ensure the test still verifies that the compiler ran.qualification/chat/chatBatchingModel.ts-123-127 (1)
123-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompute the p95 rank with integer arithmetic.
Math.ceil(sorted.length * 0.95)is affected by binary floating-point representation.0.95is not exact, so20 * 0.95evaluates to19.000000000000004andMath.ceilreturns20. The function then returns the maximum value instead of the 95th percentile. The same drift occurs for other lengths that are multiples of 20.
p95CommitDelayMsis published in the evidence artifact byqualification/chat/runChatBatchingQualification.ts, so the reported value is overstated for those trace lengths.🐛 Proposed fix
function percentile95(values: readonly number[]): number { if (values.length === 0) return 0; const sorted = values.toSorted((left, right) => left - right); - return sorted[Math.ceil(sorted.length * 0.95) - 1] ?? 0; + return sorted[Math.ceil((sorted.length * 95) / 100) - 1] ?? 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/chat/chatBatchingModel.ts` around lines 123 - 127, Update percentile95 to calculate the 95th-percentile rank using integer arithmetic rather than multiplying by the floating-point literal 0.95; preserve the existing empty-input fallback, sorted ordering, and zero-based indexing while ensuring lengths divisible by 20 select the 19th-ranked value instead of the maximum.qualification/chat/chatBatchingQualification.ts-68-77 (1)
68-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
thinkingguard cannot fail; assert the stream instead.
fixtureEventat line 37 already filters oncandidate.kind === kindand throws when no event matches. Line 75 therefore tests a condition that is always false, and the error message at line 76 does not describe what is actually verified.
thinkingis bound to the firstagent-deltaincompleted-tool-run, not to an event whosestreamis"thinking". The reviewed fixture happens to order the thinking delta first. If a future reviewed fixture reorders that scenario,thinkingsilently becomes an assistant-stream template andpayloadBytesis computed from the wrong template, while this guard still passes.Select
thinkingby stream and assert it, matching howassistantis selected.🐛 Proposed fix
const throttleMs = fixture.streamingPolicy.deltaThrottleMs; - const thinking = fixtureEvent(fixture, "completed-tool-run", "agent-delta"); - const assistant = fixture.syntheticScenarios + const agentDeltaForStream = (stream: "assistant" | "thinking") => + fixture.syntheticScenarios .flatMap(({ events }) => events) .find( (event): event is Extract<SyntheticChatEvent, { kind: "agent-delta" }> => - event.kind === "agent-delta" && event.stream === "assistant" + event.kind === "agent-delta" && event.stream === stream ); - if (thinking.kind !== "agent-delta" || assistant === undefined) { + const thinking = agentDeltaForStream("thinking"); + const assistant = agentDeltaForStream("assistant"); + if (thinking === undefined || assistant === undefined) { throw new Error("Reviewed chat fixture lacks both coalesced agent streams"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/chat/chatBatchingQualification.ts` around lines 68 - 77, Update the thinking event selection in the fixture validation to require both kind "agent-delta" and stream "thinking", rather than relying on fixtureEvent’s first matching event. Preserve the assistant selection, and change the guard/error to assert that the thinking and assistant stream-specific events are both present before computing payloadBytes.qualification/shutdown/shutdownService.ts-166-168 (1)
166-168: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe process-group evidence is not established by the code. The qualification reports and documents a detached process group, but
grandchildProcessResourceinqualification/shutdown/shutdownServiceResources.tsspawns withdetached: false, andstopGrandchildsignals a single pid. One decision resolves both sites: either create a real process group and signal it, or stop claiming one.
qualification/shutdown/shutdownService.ts#L166-L168: either establish group leadership before reportingprocessGroupId, or rename the field to reflect that it carries the service pid.docs/architecture/greenfield-rewrite/progress.md#L618-L625: replace "ends the detached process group" with wording that matches the single-child SIGTERM-then-SIGKILL reaping that the code performs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/shutdown/shutdownService.ts` around lines 166 - 168, Align the reported process-group evidence with the implemented single-process behavior: in qualification/shutdown/shutdownService.ts at lines 166-168, rename processGroupId to represent the service pid unless group leadership is explicitly established; in docs/architecture/greenfield-rewrite/progress.md at lines 618-625, replace the claim that a detached process group is ended with wording describing single-child SIGTERM-then-SIGKILL reaping.qualification/shutdown/shutdownServiceResources.ts-430-465 (1)
430-465: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA failed handshake leaks the open WebSocket.
failremoves the listeners and resumes with a failure. It never closessocket. The socket stays inCONNECTINGorOPENon thegateway-non-text-frame,gateway-transport-error, andgateway-protocol-errorpaths.gatewaySocketResourceacquires throughopenGatewaySocket, so a failed acquisition produces no resource and registers no finalizer. The socket then survives until the process exits.The interrupt canceler at Line 469 already performs the correct close. Reuse it in
fail.🐛 Proposed fix to close the socket on handshake failure
+ const closeSocket = () => { + if ( + socket.readyState === WebSocket.CONNECTING || + socket.readyState === WebSocket.OPEN + ) { + socket.close(1000, "qualification handshake failed"); + } + }; const fail = (operation: string, cause?: unknown) => { if (settled) return; settled = true; removeListeners(); + closeSocket(); resume( Effect.fail( new ShutdownQualificationResourceError({ cause, operation }) ) ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/shutdown/shutdownServiceResources.ts` around lines 430 - 465, Update the handshake failure handler fail in gatewaySocketResource to close the WebSocket before resuming with failure. Reuse the existing interrupt canceler’s socket-close behavior rather than duplicating a different cleanup path, while preserving listener removal and ShutdownQualificationResourceError reporting.qualification/shutdown/shutdownGrandchild.ts-1-3 (1)
1-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the grandchild alive with an explicit handle.
Effect.neverkeeps the fiber scheduled, but this uses rawEffect.runPromiseinstead of a platform runtime that supports SIGTERM. If the runtime handle is not guaranteed after Effect 4 refactor, replace this withEffect.sleepfor a long duration or an explicit interval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/shutdown/shutdownGrandchild.ts` around lines 1 - 3, Update the grandchild’s top-level Effect execution around Effect.runPromise so its long-running lifecycle is retained through an explicit runtime handle that supports SIGTERM. If that handle is unavailable after the Effect 4 refactor, replace Effect.never with a sufficiently long Effect.sleep or an explicit interval while preserving the process’s alive behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qualification/budgets/runSafeChildCancellationEvidence.ts`:
- Around line 5-10: Update the validation around
interruptedShutdownQualification to fail when
report.processGroupMembersWhileReady.length is zero, ensuring the evidence
confirms a child process existed before interruption. Preserve the existing
post-interruption and stopped-status checks in the same error path.
In `@qualification/outbox/sqliteOutboxProtocol.ts`:
- Line 4: Align countSchema with the maximum count produced by runDrainCommand:
either reduce the drain loop’s permitted total to the protocol bound or increase
validation to cover the full possible drain result. Ensure successful drain
statuses remain accepted by readStatus.
In `@qualification/shutdown/completeShutdownQualification.test.ts`:
- Around line 60-61: Set explicit Bun timeout values on the tests invoking
completeShutdownQualification and interruptedShutdownQualification, using a
duration greater than each qualification’s total operation budget and
process-spawning overhead. Update both test declarations while preserving their
existing assertions and qualification behavior.
In `@qualification/shutdown/completeShutdownQualification.ts`:
- Around line 254-273: Update
qualification/shutdown/completeShutdownQualification.ts:254-273 in fetchResponse
to acquire the Response body within the enclosing scope and register
response.body?.cancel() as its release finalizer, ensuring callers at 523, 534,
and 566 cannot leak connections. Also update
qualification/shutdown/completeShutdownQualification.ts:334-356 to explicitly
cancel the reader when opening-event validation fails, because the
Effect.acquireRelease cleanup does not run when acquisition fails.
- Around line 159-181: Update the early-exit branch in stopServiceProcess so an
already-exited child still invokes killProcessGroup(child.pid) before
completing. Preserve the existing graceful shutdown and timeout flow for running
children, and ensure the detached process group is reaped in both exitCode and
signalCode cases.
In `@qualification/shutdown/shutdownIdleHttpConnection.ts`:
- Around line 90-105: Update the response handling in the idle HTTP
qualification to parse the headers after headerEnd, extract Content-Length, and
wait until headerEnd + 4 + contentLength bytes are available before settling.
Keep the existing status validation, then pause the socket and resume only after
the complete readiness response body has been consumed.
In `@src/app/server.ts`:
- Around line 150-162: Update the shutdownListener error path in the stopPromise
callback to specifically handle ApplicationListenerStopTimeoutError before
disposing the application runtime: mark readiness unavailable and record that
listener shutdown did not complete so the process supervisor can escalate
termination. Preserve primaryErrorAfterCleanup for other errors and do not
dispose before recording this state.
In `@src/server/platform/runtime/applicationRuntime.ts`:
- Around line 112-155: Ensure gracefulShutdownTimeoutMs represents one total
shutdown budget rather than applying independently to the graceful wait,
boundedForceListenerStop, and awaitGracefulListenerSettlement; derive each later
timeout from the remaining deadline while preserving the existing timeout errors
and shutdown sequence. Update the ApplicationListenerShutdownOptions
documentation to state the single-budget semantics.
---
Minor comments:
In `@qualification/budgets/resourceBudgetOrchestration.ts`:
- Around line 406-447: Move the launcher exit-code validation into the
Effect.scoped generator immediately after runBoundedProcess and before
readResult. Preserve the existing diagnostic construction and
ResourceBudgetOrchestrationError with operation "transient-unit-exit", and
return the completed launcher/report only when the launcher succeeds.
In `@qualification/budgets/resourceBudgetPolicy.test.ts`:
- Around line 217-220: Update the test case around the “unit was not collected”
candidate mutation so it sets unitCollected to false instead of cgroupRemoved.
Keep cgroup cleanup coverage in its separate test and ensure this case verifies
that assessResourceBudgetEvidence rejects an uncollected unit.
In `@qualification/budgets/resourceBudgetUnit.ts`:
- Around line 430-439: Update the limits construction around finalCgroup to
validate the final values rather than relying on casts: explicitly reject or
handle "max" for cpuQuotaMicros, memoryHighBytes, memoryMaxBytes,
memorySwapMaxBytes, and pidsMax so the reported fields remain numeric. Replace
the hardcoded oomGroup: true with the observed finalCgroup.oomGroup value,
preserving schema and assertObservedLimits validation.
In `@qualification/build/frontendBuildQualification.test.ts`:
- Line 12: Update the production asset assertion using hashedAssetPattern to
first verify that production.outputPaths contains at least one matching CSS or
JavaScript asset, then apply the existing hash-pattern validation so an empty
filtered array cannot pass.
- Around line 70-81: Update the React Compiler assertion in the frontend build
qualification test to inspect unminified development output or a stable
compiler-runtime import specifier instead of requiring the production bundle to
contain the local name “useMemoCache”. Keep the stylesheet assertion unchanged
and ensure the test still verifies that the compiler ran.
In `@qualification/chat/chatBatchingModel.ts`:
- Around line 123-127: Update percentile95 to calculate the 95th-percentile rank
using integer arithmetic rather than multiplying by the floating-point literal
0.95; preserve the existing empty-input fallback, sorted ordering, and
zero-based indexing while ensuring lengths divisible by 20 select the
19th-ranked value instead of the maximum.
In `@qualification/chat/chatBatchingQualification.ts`:
- Around line 68-77: Update the thinking event selection in the fixture
validation to require both kind "agent-delta" and stream "thinking", rather than
relying on fixtureEvent’s first matching event. Preserve the assistant
selection, and change the guard/error to assert that the thinking and assistant
stream-specific events are both present before computing payloadBytes.
In `@qualification/openclaw/sourceAudit.ts`:
- Around line 1016-1020: Update the OpenClaw source-audit assertion around
promptVisibility to include the source marker for the constant or literal that
sets the bounded task prompt length to 4000. Bind promptChars to the relevant
task protocol or handler assertion, preserving the existing visibility flags and
value.
In `@qualification/outbox/sqliteOutboxQualification.test.ts`:
- Around line 74-86: Update the contention logic in the Effect.sync block to
track whether competingWriter.run("BEGIN IMMEDIATE") succeeds, only classify
errors from the competing transaction attempt, and preserve the explicit
sentinel error for unexpected lock acquisition. In the finally block, roll back
writer and also roll back competingWriter when it successfully began a
transaction.
In `@qualification/parity/parityInventory.test.ts`:
- Around line 98-103: Set an explicit timeout greater than 5000 ms on the test
named “accounts for every executable backend route and documented row exactly
once,” ensuring the loadLegacyBackendRouteIdentities probe can reach its timeout
and report failures without the test runner timing out first.
In `@qualification/shutdown/shutdownGrandchild.ts`:
- Around line 1-3: Update the grandchild’s top-level Effect execution around
Effect.runPromise so its long-running lifecycle is retained through an explicit
runtime handle that supports SIGTERM. If that handle is unavailable after the
Effect 4 refactor, replace Effect.never with a sufficiently long Effect.sleep or
an explicit interval while preserving the process’s alive behavior.
In `@qualification/shutdown/shutdownService.ts`:
- Around line 166-168: Align the reported process-group evidence with the
implemented single-process behavior: in
qualification/shutdown/shutdownService.ts at lines 166-168, rename
processGroupId to represent the service pid unless group leadership is
explicitly established; in docs/architecture/greenfield-rewrite/progress.md at
lines 618-625, replace the claim that a detached process group is ended with
wording describing single-child SIGTERM-then-SIGKILL reaping.
In `@qualification/shutdown/shutdownServiceResources.ts`:
- Around line 430-465: Update the handshake failure handler fail in
gatewaySocketResource to close the WebSocket before resuming with failure. Reuse
the existing interrupt canceler’s socket-close behavior rather than duplicating
a different cleanup path, while preserving listener removal and
ShutdownQualificationResourceError reporting.
---
Nitpick comments:
In `@docs/architecture/greenfield-rewrite/progress.md`:
- Around line 631-642: Update the final scenario label in the table to
“Child-process cancel” so it matches the corresponding row in the other
architecture document, without changing its measurements or table structure.
In `@docs/architecture/greenfield-rewrite/runtime-and-delivery.md`:
- Around line 90-101: Remove the duplicated resource matrix table from the
runtime-and-delivery document and replace it with a reference link to the
authoritative measurements in the progress record. Preserve the surrounding
statement about passing resource checks and direct readers to the existing
resource matrix section in progress.md.
In `@package.json`:
- Around line 59-61: The manifest lacks a direct pin for `@tanstack/db`. In
package.json lines 59-61, add the `@tanstack/db` version pin at 0.6.17 or an
equivalent Bun override alongside the existing TanStack pins; in
qualification/browser/queryCollectionAdapter.test.ts lines 203-210, keep the
version assertion and update its expected value whenever the manifest pin
changes.
In `@qualification/browser/queryCollectionAdapter.test.ts`:
- Around line 231-256: Update readInstalledVersions() to resolve each
package.json through the adapter test’s module-resolution context instead of
constructing URLs from ../../node_modules. Use the resolver to locate the
installed copy under the test scope’s `@tanstack` packages, then read and validate
its version as before.
In `@qualification/budgets/resourceBudgetOrchestration.ts`:
- Around line 79-85: Update requiredExecutable and its callers in
resourceBudgetQualification to return failures through the declared Effect error
channel instead of throwing plain Error values. Convert missing or non-absolute
executable results into ResourceBudgetOrchestrationError, preserving the
existing ResourceBudgetOrchestrationDeadlineError behavior and allowing callers
to handle the failure with Effect.catchTag.
In `@qualification/budgets/resourceBudgetPolicy.ts`:
- Around line 6-18: Update resourceBudgetUnitNamePattern to derive its scenario
alternation from resourceBudgetScenarioIds instead of repeating literal ids,
while preserving the existing resource-budget unit-name format and matching
behavior. Ensure the pattern is initialized after resourceBudgetScenarioIds is
declared and remains compatible with createResourceBudgetUnitName and
assertResourceBudgetUnitName.
In `@qualification/build/frontendBuildQualification.ts`:
- Around line 26-28: Update qualificationFrontendEntrypoint to resolve the
fixture path relative to import.meta.dir rather than process.cwd(), while
preserving the existing qualification/build/fixtures/frontend/index.html
location.
In `@qualification/build/runFrontendBuildQualification.ts`:
- Around line 33-49: Update the metrics validation in the build qualification
flow after readFile to parse the JSON content and compare the parsed
formatVersion value to 1, replacing the formatted substring check in the
existing metrics.includes condition. Preserve the existing error message and all
other validation checks.
In `@qualification/chat/chatBatchingModel.ts`:
- Line 225: Update maximumCommitDelayMs in simulateChatBatching to compute the
maximum commit delay with reduce instead of spreading commitDelays into
Math.max, while preserving the existing zero fallback for empty or non-positive
values and supporting arbitrarily large traces.
In `@qualification/chat/chatBatchingQualification.ts`:
- Around line 139-151: In the rejection-reason construction, define separate
named constants for the visual-delay and crash-window bounds using the
corresponding metrics fields maximumAdditionalVisualDelayMs and
maximumCrashWindowMs, then use each constant for its matching reason condition.
Keep the existing scheduled-transaction bound and reason unchanged.
In `@qualification/openclaw/reviewedFixtures.ts`:
- Around line 209-229: Refactor the existence check around stat(outputDirectory)
to use an explicit existence flag rather than throwing and identifying the
sentinel via error.message. Set the flag when stat succeeds, handle only ENOENT
as the absent-directory case, and throw the “OpenClaw audit output directory
already exists” error after the try/catch when the flag indicates the directory
exists.
In `@qualification/openclaw/sourceAudit.test.ts`:
- Around line 429-450: Extend the source-audit tests around
parseSourceAuditCliArguments to cover the --output= argument mode, and add a
companion case that calls writeOpenClawAuditCandidate twice with the same output
directory, asserting the second call rejects with the existing-directory
refusal. Preserve the current coverage and use the existing audit fixture/setup
symbols.
- Around line 361-367: Update the test around loadReviewedOpenClawFixtures to
assert its rejected promise directly with Bun’s rejection matcher, checking that
the error message contains “hash mismatch for chat.json”; remove the try/catch
and self-thrown sentinel so a successful resolution cannot be mistaken for the
expected failure.
In `@qualification/outbox/sqliteOutboxQualification.ts`:
- Around line 213-241: Update claimAndTerminateChild to capture the child’s
required signal value within the Effect.scoped block and return that value with
status instead of returning the released QualificationChildProcess handle.
Adjust the call site and report field to read the captured signal directly,
preserving the existing post-kill behavior.
In `@qualification/parity/parityFixtureCandidate.ts`:
- Around line 39-59: Extract the shared greenfield projection and sorting logic
from buildGreenfieldContractFixtureCandidate and
assertGreenfieldRegistryMatchesReviewed into one exported helper, and have both
callers use it so procedures and rawHttp remain byte-identical. Move the related
ProcedureContractCandidate, RawHttpContractCandidate, ProcedureContractIdentity,
and RawHttpContractIdentity types into the shared module, preserving the
existing method compatibility between the union and string declarations.
In `@qualification/parity/parityInventory.test.ts`:
- Around line 31-40: Tighten the countByPhase parameter type to use the exported
EndpointTarget and frontend route target fixture types from
parityInventorySchemas.ts instead of an anonymous object shape. Preserve the
existing reviewed-removal filtering and phase counting while ensuring schema
changes produce compile-time failures.
In `@qualification/parity/parityInventorySchemas.ts`:
- Around line 191-194: Export a named constant for the reviewed endpoint row
count from the schema module, and replace the hard-coded 157 in the parity
schema validation and parityInventory test with that shared constant. Update the
relevant imports so both the validation message/check and test expectations use
the single exported value.
In `@qualification/parity/reviewedParityInventory.ts`:
- Around line 37-39: Update canonicalJson to serialize objects with
deterministically sorted keys, so parity comparisons depend on values rather
than insertion order. Reuse or move the existing compareStrings helper as
needed, ensuring it is declared before canonicalJson or otherwise available when
serialization occurs; preserve the current formatted JSON output and trailing
newline.
In `@qualification/parity/sourceParityInventory.test.ts`:
- Around line 22-44: Generalize withModifiedRouter to accept the relative source
path to mutate instead of always using paritySourcePaths.router. Update its
temporary-file mutation logic and callers so rejection tests can modify the
navigation, route-module, and preload source files, then add rejection coverage
for the corresponding extractNavigationEntries, extractRouteModules, and
extractPreloadEntries guards while preserving existing router tests.
In `@qualification/resources/sseMemoryScenario.ts`:
- Around line 174-177: Optionally move scope creation into the per-round flow
surrounding consumer creation, so each round uses and closes its own Effect
scope before the next begins. Update the cleanup logic to close that round’s
scope and remove the now-redundant closeConsumers call, while preserving
consumer cleanup and memory assertions.
In `@qualification/shutdown/shutdownProtocol.ts`:
- Around line 134-157: Update createGatewayConnectRequest to include the
validated nonce in the connect request’s authentication parameters, and extend
gatewayConnectParametersSchema to accept and validate the same nonce field.
Ensure the generated fixture request echoes the challenge so the handshake is
bound to the original nonce.
In `@qualification/shutdown/shutdownService.ts`:
- Around line 305-311: Update the top-level catch around parseCommand and
runService to bind the thrown error, then include its diagnostic details when
writing the failure to process.stderr; preserve the existing failure message and
exitCode assignment while retaining the tagged error and stack information.
In `@qualification/shutdown/shutdownServiceResources.test.ts`:
- Around line 93-126: The existing memoization test does not exercise SSE
controller teardown because it mocks Bun.serve. Add a separate test that starts
applicationServerResource with a real listener, opens /api/events, verifies an
SSE connection is registered, calls close(), and asserts sseConnectionCount
returns to zero; keep the existing memoization assertions unchanged.
In `@qualification/websocket/nativeWebSocketQualification.test.ts`:
- Around line 366-393: Reduce or remove the fixed 1200-millisecond sleep in the
test around observeNativeWebSocket. Since the effect has already completed and
attempts is updated only by the injected WebSocket factory, verify the
no-reconnect behavior immediately or use a shorter wait sufficient to observe
the released socket state while preserving the attempts assertion.
In `@qualification/websocket/nativeWebSocketQualification.ts`:
- Around line 314-333: Document the port-reuse race in
closedLoopbackWebSocketUrl: add a comment explaining that listener.stop(true)
runs before the caller connects, allowing concurrent tests or the OS to reuse
the ephemeral port and invalidate refusal expectations. Do not change the
existing acquire/use/release behavior.
In `@qualification/websocket/rawWebSocketProtocol.ts`:
- Around line 142-145: Update createUpgradeResponse at the createHash("sha1")
call with an inline static-analysis suppression that references RFC 6455 §4.2.2
and explains SHA-1 is required for the WebSocket Sec-WebSocket-Accept protocol
value. Keep the hashing implementation unchanged.
- Around line 5-8: Export maximumFixtureOutboundBytes from
rawWebSocketProtocol.ts and update nativeWebSocketQualification.test.ts to
import and use that constant in the evidence.sentBytes assertion instead of
duplicating the 128 * 1024 literal, keeping the test synchronized with the
fixture limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0843f828-0ab0-46c8-a09c-c281e7b3c872
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockand included by**/*docs/generated/packages-and-runtime.mdis excluded by!**/generated/**and included by**/*
📒 Files selected for processing (84)
bunfig.tomldocs/api/endpoints.mddocs/architecture/greenfield-rewrite.mddocs/architecture/greenfield-rewrite/application-architecture.mddocs/architecture/greenfield-rewrite/implementation-plan.mddocs/architecture/greenfield-rewrite/progress.mddocs/architecture/greenfield-rewrite/runtime-and-delivery.mdpackage.jsonqualification/browser/queryCollectionAdapter.test.tsqualification/browser/queryCollectionAdapter.tsqualification/budgets/resourceBudgetCommand.tsqualification/budgets/resourceBudgetOrchestration.test.tsqualification/budgets/resourceBudgetOrchestration.tsqualification/budgets/resourceBudgetPolicy.test.tsqualification/budgets/resourceBudgetPolicy.tsqualification/budgets/resourceBudgetUnit.tsqualification/budgets/runResourceBudgetEvidence.tsqualification/budgets/runSafeChildCancellationEvidence.tsqualification/build/fixtures/frontend/index.htmlqualification/build/fixtures/frontend/src/LazyPanel.tsxqualification/build/fixtures/frontend/src/QualificationApp.tsxqualification/build/fixtures/frontend/src/index.cssqualification/build/fixtures/frontend/src/main.tsxqualification/build/frontendBuildQualification.test.tsqualification/build/frontendBuildQualification.tsqualification/build/runFrontendBuildQualification.tsqualification/chat/chatBatching.test.tsqualification/chat/chatBatchingModel.tsqualification/chat/chatBatchingQualification.tsqualification/chat/runChatBatchingQualification.tsqualification/openclaw/fixtures/2026.7.2-beta.7/agents.jsonqualification/openclaw/fixtures/2026.7.2-beta.7/chat.jsonqualification/openclaw/fixtures/2026.7.2-beta.7/cron.jsonqualification/openclaw/fixtures/2026.7.2-beta.7/gateway.jsonqualification/openclaw/fixtures/2026.7.2-beta.7/manifest.jsonqualification/openclaw/fixtures/2026.7.2-beta.7/sessions.jsonqualification/openclaw/fixtures/2026.7.2-beta.7/tasks.jsonqualification/openclaw/reviewedFixtures.tsqualification/openclaw/runSourceAudit.tsqualification/openclaw/sourceAudit.test.tsqualification/openclaw/sourceAudit.tsqualification/openclaw/sourceAuditSchemas.tsqualification/outbox/runSqliteOutboxEvidence.tsqualification/outbox/sqliteOutboxChild.tsqualification/outbox/sqliteOutboxProtocol.tsqualification/outbox/sqliteOutboxQualification.test.tsqualification/outbox/sqliteOutboxQualification.tsqualification/outbox/sqliteOutboxStore.tsqualification/parity/fixtures/frontend-routes.jsonqualification/parity/fixtures/greenfield-contracts.jsonqualification/parity/fixtures/legacy-endpoints.jsonqualification/parity/legacyBackendRouteInventory.tsqualification/parity/parityFixtureCandidate.tsqualification/parity/parityInventory.test.tsqualification/parity/parityInventorySchemas.tsqualification/parity/reviewedParityInventory.tsqualification/parity/sourceParityInventory.test.tsqualification/parity/sourceParityInventory.tsqualification/resources/pausedTlsSseClient.test.tsqualification/resources/pausedTlsSseClient.tsqualification/resources/sseMemoryScenario.tsqualification/shutdown/completeShutdownQualification.test.tsqualification/shutdown/completeShutdownQualification.tsqualification/shutdown/runCompleteShutdownEvidence.tsqualification/shutdown/shutdownDatabase.tsqualification/shutdown/shutdownGrandchild.tsqualification/shutdown/shutdownIdleHttpConnection.tsqualification/shutdown/shutdownProtocol.tsqualification/shutdown/shutdownService.tsqualification/shutdown/shutdownServiceResources.test.tsqualification/shutdown/shutdownServiceResources.tsqualification/test/asyncCleanupStack.test.tsqualification/test/asyncCleanupStack.tsqualification/websocket/nativeWebSocketQualification.test.tsqualification/websocket/nativeWebSocketQualification.tsqualification/websocket/rawWebSocketFixture.tsqualification/websocket/rawWebSocketProtocol.tsscripts/frontendBuild.tsscripts/qualification/legacyBackendRouteProbe.tssrc/app/server.tssrc/server/platform/runtime/applicationRuntime.test.tssrc/server/platform/runtime/applicationRuntime.tssrc/server/test/support/requestContext.tssrc/server/test/system/serverShutdown.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze JavaScript and TypeScript
🧰 Additional context used
🪛 ast-grep (0.45.0)
qualification/websocket/rawWebSocketProtocol.ts
[warning] 142-142: Do not use weak hash functions (MD5/SHA1)
Context: createHash("sha1")
Note: [CWE-328] Use of Weak Hash.
(insecure-hash-typescript)
[warning] 142-142: Avoid SHA1 security protocol
Context: createHash("sha1")
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm (SHA-1).
(avoid-crypto-sha1-typescript)
qualification/openclaw/sourceAudit.ts
[warning] 279-279: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(const ${name} = ([^;]+);, "u")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 LanguageTool
docs/architecture/greenfield-rewrite/progress.md
[style] ~610-~610: Using “real” as an adverb is considered informal. Consider using “really” or “very”.
Context: ...er/writer and writer/writer behavior, real busy/locked classification, no-gap/no-d...
(REAL_REALLY)
docs/architecture/greenfield-rewrite.md
[style] ~5-~5: Consider using “incomplete” to avoid wordiness.
Context: ...ed, > hardening, and cutover phases are not complete. The rewrite is built beside the curren...
(NOT_ABLE_PREMIUM)
docs/architecture/greenfield-rewrite/implementation-plan.md
[style] ~102-~102: Using “real” as an adverb is considered informal. Consider using “really” or “very”.
Context: ...ox without gaps or duplicates, classify real busy/locked outcomes, and recover an ...
(REAL_REALLY)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/server.ts (1)
148-167: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA failed stop is memoized, so a later
stop(true)cannot escalate.Line 150 memoizes
stopPromiseon the first call, including whenshutdownListenerrejects. A supervisor that first callsstop()and then escalates withstop(true)abortsforceStopControllerat Line 149, but the memoized rejected promise is returned without re-enteringshutdownListener. The abort signal is never observed, no forced listener stop runs, anddispose()never runs.Either clear the memo on rejection so an escalation can retry, or document that a listener-stop failure is terminal and require the supervisor to kill the process.
🐛 Proposed fix to allow one escalation retry
stop(force = false) { if (force) forceStopController.abort(); stopPromise ??= (async () => { try { await options.applicationRuntime.shutdownListener({ forceSignal: forceStopController.signal, gracefulShutdownTimeoutMs, stop: (forceListener) => server.stop(forceListener), }); } catch (error) { // A listener-stop rejection cannot prove that no request can still enter // the runtime. Withdraw readiness and preserve process services for the // supervisor's terminal containment instead of disposing them underneath // a potentially live listener. options.readiness.markUnavailable(); + // Release the memo so a forced escalation can retry the listener stop. + stopPromise = undefined; throw error; } await options.applicationRuntime.dispose(); })(); return stopPromise; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/server.ts` around lines 148 - 167, Update the stop method’s stopPromise lifecycle so a shutdownListener rejection does not permanently memoize the failed attempt: clear or reset stopPromise in the rejection path before propagating the error, allowing a later stop(true) call to re-enter shutdownListener with the already-aborted forceStopController signal and then dispose successfully. Preserve promise memoization for successful shutdown and concurrent callers.
🧹 Nitpick comments (4)
qualification/shutdown/shutdownServiceResources.ts (1)
368-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the challenge nonce into one constant.
Line 375 and Line 391 repeat the literal
"shutdown-qualification-nonce". If one site changes, the handler rejects every connect request and closes with 1008. The failure looks like a client defect, not a fixture defect. Bind the value once and use it at both sites.♻️ Proposed refactor
+const gatewayChallengeNonce = "shutdown-qualification-nonce"; + export function gatewayFixtureResource(): Effect.Effect<- if ( - request.params.nonce !== - "shutdown-qualification-nonce" - ) { + if (request.params.nonce !== gatewayChallengeNonce) { throw new Error( "Gateway connect request did not echo its challenge" ); }payload: { - nonce: "shutdown-qualification-nonce", + nonce: gatewayChallengeNonce, ts: 1_786_000_000_000, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/shutdown/shutdownServiceResources.ts` around lines 368 - 397, Define a single constant for the shutdown qualification nonce in the surrounding setup, then replace both the request validation comparison and the challenge payload’s duplicated "shutdown-qualification-nonce" literal with that constant. Keep the existing connect handling and close behavior unchanged.qualification/files/boundedFile.ts (2)
125-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original failure cause.
The
catchat Line 125 discards the caught value. Every failure then surfaces as the sameinvalidMessagewith no cause. A qualification failure is undiagnosable: an allowed-root violation, anEACCES, and a hook rejection are indistinguishable. Attach the original error ascause.String(error)does not rendercause, so the redaction assertions inqualification/files/boundedFile.test.ts(Lines 145, 167, 189) still hold.♻️ Proposed refactor
-function invalidFileState(message: string): Error { - return new Error(message); +function invalidFileState(message: string, cause?: unknown): Error { + return cause === undefined ? new Error(message) : new Error(message, { cause }); }let result: Buffer | undefined; - let failed = false; + let failed = false; + let failureCause: unknown; try { // ... result = buffer.subarray(0, bytesRead); - } catch { - failed = true; + } catch (error) { + failed = true; + failureCause = error; } if (pathFile) { try { await pathFile.close(); - } catch { - failed = true; + } catch (error) { + failed = true; + failureCause ??= error; } } if (file) { try { await file.close(); - } catch { - failed = true; + } catch (error) { + failed = true; + failureCause ??= error; } } - if (failed || !result) throw invalidFileState(invalidMessage); + if (failed || !result) throw invalidFileState(invalidMessage, failureCause); return result; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/files/boundedFile.ts` around lines 125 - 143, Preserve the caught qualification error in the main try/catch around bounded-file validation instead of discarding it. Pass that original error as the cause when constructing invalidFileState(invalidMessage), while keeping cleanup failures and existing redaction behavior unchanged.
83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the procfs dependency, or the reader fails closed on every non-Linux host.
Line 83 and Line 113 resolve
/proc/self/fd/${fd}. That path exists only on Linux. On macOS therealpathcall rejects, the catch at Line 125 records the failure, and every call throwsinvalidMessage. A developer then sees a containment violation instead of an unsupported platform. State the Linux requirement in the JSDoc, or detectprocess.platformand throw a distinct error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/files/boundedFile.ts` around lines 83 - 86, Document the Linux-only procfs requirement in the JSDoc for the bounded-file reader and its use of /proc/self/fd via realpath, so unsupported platforms are clearly identified rather than reported as containment violations. Anchor the documentation to the reader function containing the descriptorPath resolution and the corresponding resolution near the second descriptor check; do not alter containment behavior.qualification/build/frontendBuildQualification.test.ts (1)
113-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection reason for each HTML sample.
The loop only asserts that some
Erroris thrown. One overly broad rule insideassertSelfHostedFrontendHtmlwould satisfy all 14 samples. The test would then keep passing after a specific CSP rule regresses. Pair each sample with its expected message fragment.♻️ Proposed refactor
- for (const html of [ - '<script type="module" src="/assets/app.js">inline()</script>', - '<script type="module" src="/assets/app.js"></script><script>inline()</script >', + for (const [html, expected] of [ + [ + '<script type="module" src="/assets/app.js">inline()</script>', + "inline script", + ], + [ + '<script type="module" src="/assets/app.js"></script><script>inline()</script >', + "inline script", + ], // ... remaining samples paired with their expected message fragment - ]) { + ] as const) { await writeFile(indexPath, html, "utf8"); let rejected = false; try { await assertSelfHostedFrontendHtml(indexPath); } catch (error) { rejected = true; expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain(expected); } expect(rejected).toBeTrue(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qualification/build/frontendBuildQualification.test.ts` around lines 113 - 138, Update the HTML samples in the test around assertSelfHostedFrontendHtml to pair each input with its expected rejection-message fragment, then assert the caught Error message contains that fragment for every case. Preserve the existing rejection assertion while ensuring each sample validates the specific rule it is intended to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qualification/openclaw/sourceAudit.test.ts`:
- Around line 365-367: Await both rejection assertions in
qualification/openclaw/sourceAudit.test.ts at lines 365-367 and 425-427: the
assertion around loadReviewedOpenClawFixtures must await the "hash mismatch for
chat.json" rejection, and the assertion around writeOpenClawAuditCandidate must
await the "output directory already exists" rejection.
---
Outside diff comments:
In `@src/app/server.ts`:
- Around line 148-167: Update the stop method’s stopPromise lifecycle so a
shutdownListener rejection does not permanently memoize the failed attempt:
clear or reset stopPromise in the rejection path before propagating the error,
allowing a later stop(true) call to re-enter shutdownListener with the
already-aborted forceStopController signal and then dispose successfully.
Preserve promise memoization for successful shutdown and concurrent callers.
---
Nitpick comments:
In `@qualification/build/frontendBuildQualification.test.ts`:
- Around line 113-138: Update the HTML samples in the test around
assertSelfHostedFrontendHtml to pair each input with its expected
rejection-message fragment, then assert the caught Error message contains that
fragment for every case. Preserve the existing rejection assertion while
ensuring each sample validates the specific rule it is intended to exercise.
In `@qualification/files/boundedFile.ts`:
- Around line 125-143: Preserve the caught qualification error in the main
try/catch around bounded-file validation instead of discarding it. Pass that
original error as the cause when constructing invalidFileState(invalidMessage),
while keeping cleanup failures and existing redaction behavior unchanged.
- Around line 83-86: Document the Linux-only procfs requirement in the JSDoc for
the bounded-file reader and its use of /proc/self/fd via realpath, so
unsupported platforms are clearly identified rather than reported as containment
violations. Anchor the documentation to the reader function containing the
descriptorPath resolution and the corresponding resolution near the second
descriptor check; do not alter containment behavior.
In `@qualification/shutdown/shutdownServiceResources.ts`:
- Around line 368-397: Define a single constant for the shutdown qualification
nonce in the surrounding setup, then replace both the request validation
comparison and the challenge payload’s duplicated "shutdown-qualification-nonce"
literal with that constant. Keep the existing connect handling and close
behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35dfca3d-04da-4c85-9712-0a879a630a08
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockand included by**/*docs/generated/packages-and-runtime.mdis excluded by!**/generated/**and included by**/*
📒 Files selected for processing (51)
docs/architecture/greenfield-rewrite.mddocs/architecture/greenfield-rewrite/application-architecture.mddocs/architecture/greenfield-rewrite/implementation-plan.mddocs/architecture/greenfield-rewrite/progress.mddocs/architecture/greenfield-rewrite/runtime-and-delivery.mdpackage.jsonqualification/browser/queryCollectionAdapter.test.tsqualification/budgets/resourceBudgetOrchestration.tsqualification/budgets/resourceBudgetPolicy.test.tsqualification/budgets/resourceBudgetPolicy.tsqualification/budgets/resourceBudgetUnit.tsqualification/budgets/runSafeChildCancellationEvidence.tsqualification/build/frontendBuildQualification.test.tsqualification/build/frontendBuildQualification.tsqualification/build/runFrontendBuildQualification.tsqualification/chat/chatBatchingModel.tsqualification/chat/chatBatchingQualification.tsqualification/files/boundedFile.test.tsqualification/files/boundedFile.tsqualification/openclaw/fixtures/2026.7.2-beta.7/manifest.jsonqualification/openclaw/reviewedFixtures.tsqualification/openclaw/sourceAudit.test.tsqualification/openclaw/sourceAudit.tsqualification/openclaw/sourceAuditSchemas.tsqualification/outbox/sqliteOutboxChild.tsqualification/outbox/sqliteOutboxProtocol.tsqualification/outbox/sqliteOutboxQualification.test.tsqualification/outbox/sqliteOutboxQualification.tsqualification/parity/legacyBackendRouteInventory.tsqualification/parity/parityFixtureCandidate.tsqualification/parity/parityInventory.test.tsqualification/parity/parityInventorySchemas.tsqualification/parity/reviewedParityInventory.tsqualification/parity/sourceParityInventory.test.tsqualification/parity/sourceParityInventory.tsqualification/resources/sseMemoryScenario.tsqualification/shutdown/completeShutdownQualification.test.tsqualification/shutdown/completeShutdownQualification.tsqualification/shutdown/shutdownGrandchild.tsqualification/shutdown/shutdownIdleHttpConnection.tsqualification/shutdown/shutdownProtocol.tsqualification/shutdown/shutdownService.tsqualification/shutdown/shutdownServiceResources.test.tsqualification/shutdown/shutdownServiceResources.tsqualification/websocket/nativeWebSocketQualification.test.tsqualification/websocket/nativeWebSocketQualification.tsqualification/websocket/rawWebSocketProtocol.tssrc/app/server.tssrc/server/platform/runtime/applicationRuntime.test.tssrc/server/platform/runtime/applicationRuntime.tssrc/server/test/system/serverShutdown.test.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- package.json
- qualification/outbox/sqliteOutboxChild.ts
- qualification/parity/legacyBackendRouteInventory.ts
- docs/architecture/greenfield-rewrite/implementation-plan.md
- qualification/openclaw/reviewedFixtures.ts
- qualification/outbox/sqliteOutboxProtocol.ts
- qualification/shutdown/shutdownProtocol.ts
- qualification/budgets/runSafeChildCancellationEvidence.ts
- qualification/websocket/nativeWebSocketQualification.ts
- qualification/parity/parityInventorySchemas.ts
- qualification/budgets/resourceBudgetUnit.ts
- qualification/shutdown/shutdownService.ts
- qualification/openclaw/fixtures/2026.7.2-beta.7/manifest.json
- qualification/outbox/sqliteOutboxQualification.ts
- qualification/openclaw/sourceAuditSchemas.ts
- qualification/parity/parityInventory.test.ts
- src/server/platform/runtime/applicationRuntime.ts
- qualification/budgets/resourceBudgetPolicy.ts
- qualification/build/runFrontendBuildQualification.ts
- qualification/parity/reviewedParityInventory.ts
- docs/architecture/greenfield-rewrite.md
- docs/architecture/greenfield-rewrite/application-architecture.md
- qualification/build/frontendBuildQualification.ts
- qualification/websocket/nativeWebSocketQualification.test.ts
- docs/architecture/greenfield-rewrite/progress.md
- qualification/chat/chatBatchingModel.ts
- qualification/openclaw/sourceAudit.ts
- src/server/test/system/serverShutdown.test.ts
- qualification/chat/chatBatchingQualification.ts
- qualification/outbox/sqliteOutboxQualification.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze JavaScript and TypeScript
🧰 Additional context used
🪛 ast-grep (0.45.0)
qualification/websocket/rawWebSocketProtocol.ts
[warning] 144-144: Do not use weak hash functions (MD5/SHA1)
Context: createHash("sha1")
Note: [CWE-328] Use of Weak Hash.
(insecure-hash-typescript)
[warning] 144-144: Avoid SHA1 security protocol
Context: createHash("sha1")
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm (SHA-1).
(avoid-crypto-sha1-typescript)
🪛 GitHub Check: CodeQL
qualification/files/boundedFile.ts
[failure] 109-112: Potential file system race condition
The file may have changed since it was checked.
🔇 Additional comments (30)
qualification/websocket/rawWebSocketProtocol.ts (8)
1-12: 🎯 Functional CorrectnessVerify the fixture budget invariant.
asBoundedPayloadlimits payload bytes, but frame headers add 2, 4, or 10 bytes. Confirm thatoversizedQualificationMessageBytesexceeds the application message limit while the encoded frame remains withinmaximumRawWebSocketFixtureOutboundBytes. Otherwise,oversized-textcan throw during fixture construction or produce an over-budget frame.Also applies to: 46-55, 326-343
13-44: 🗄️ Data Integrity & IntegrationVerify the exported protocol contracts against their consumers.
Confirm that every
RawWebSocketScenariomember has a matchingcreateScenarioBytesbranch. Confirm that all result-interface fields have compatible producers and consumers in the WebSocket qualification tests.
57-112: 🎯 Functional CorrectnessVerify RFC 6455 control-frame validation.
Confirm that
encodeServerFramerejects reserved opcodes, non-final control frames, and control payloads larger than 125 bytes. Confirm thatencodeServerCloseFramerejects reserved close codes and enforces the 123-byte UTF-8 reason limit.
142-158: LGTM!
114-141: 🎯 Functional CorrectnessVerify upgrade parsing at the HTTP/WebSocket boundary.
Confirm that header names and token values use the required case-insensitive matching. Reject malformed or ambiguous duplicate headers. Confirm that the parser enforces the handshake-size limit and preserves every byte after
\r\n\r\n.Also applies to: 161-193
195-255: 🩺 Stability & AvailabilityVerify safe 64-bit length handling.
Confirm that lengths with the high bit set are rejected. Confirm that values above the configured budget are rejected before allocation or slicing. Do not convert an untrusted 64-bit length to an unsafe
numberbefore these checks. Also confirm that incomplete frame suffixes are retained without dropping bytes.
257-286: 🎯 Functional CorrectnessVerify fragmented UTF-8 evidence at the byte level.
Confirm that concatenating the fragment bytes reproduces the complete UTF-8 sequence. Confirm that the first frame uses the text opcode, continuation frames use opcode
0x00, and only the final frame setsFIN.
335-337: 🎯 Functional CorrectnessConfirm that
silentmay send a ping.
createScenarioBytes("silent")emits a WebSocket ping. This is not wire-level silence and can trigger a pong or reset an idle timer. If the qualification requires no inbound bytes, return an empty buffer or use a separate control-frame scenario.docs/architecture/greenfield-rewrite/runtime-and-delivery.md (2)
136-141: LGTM!
91-94: 📐 Maintainability & Code QualityNo changes needed.
qualification/budgets/resourceBudgetPolicy.test.ts (1)
217-227: LGTM!qualification/build/frontendBuildQualification.test.ts (1)
39-62: LGTM!qualification/files/boundedFile.test.ts (1)
1-275: LGTM!qualification/openclaw/sourceAudit.test.ts (1)
144-147: LGTM!Also applies to: 432-442
qualification/shutdown/shutdownServiceResources.test.ts (1)
5-8: LGTM!Also applies to: 17-24, 141-179
qualification/shutdown/shutdownServiceResources.ts (1)
438-456: LGTM!Also applies to: 486-489
src/app/server.ts (1)
9-9: LGTM!Also applies to: 70-70
qualification/shutdown/shutdownGrandchild.ts (1)
3-6: 🩺 Stability & AvailabilityNo change needed for
Effect.sleep(oneDayMs).Effect 4 beta still accepts bare millisecond numbers in
DurationInput.qualification/shutdown/completeShutdownQualification.test.ts (2)
167-167: The explicit 60_000 ms and 30_000 ms per-test timeouts resolve the earlier finding about the 5000 ms Bun default for these process-spawning qualifications.Also applies to: 181-181
43-48: 🩺 Stability & AvailabilityConfirm TestClock handoff behavior before changing this test.
Deferred.succeed(cancelStarted, undefined)can complete beforecancelShutdownStreamBeforeDeadlineregisters its 25 ms timeout. If this Effect version advances time only for already-suspended fibers,TestClock.adjust(25)can run before the child fiber is scheduled, leaving no timeout to fire and causingFiber.jointo wait. MoveDeferred.succeedafter the cancellation effect is started if needed.qualification/shutdown/completeShutdownQualification.ts (2)
189-191: The already-exited branch now callskillProcessGroup(child.pid), which resolves the earlier finding about the surviving detached grandchild.
105-128: LGTM!Also applies to: 292-306, 370-398
qualification/shutdown/shutdownIdleHttpConnection.ts (1)
92-127: The handler now parsesContent-Lengthand resumes only afterheaderEnd + 4 + contentLengthbytes arrive, which resolves the earlier finding about measuring a connection with an unconsumed in-flight response.qualification/budgets/resourceBudgetOrchestration.ts (1)
79-99: LGTM!Also applies to: 433-445, 454-459, 477-490
qualification/parity/parityFixtureCandidate.ts (1)
17-32: LGTM!Also applies to: 34-65, 77-91
qualification/parity/sourceParityInventory.test.ts (1)
22-45: LGTM!Also applies to: 152-236
qualification/parity/sourceParityInventory.ts (1)
61-65: LGTM!Also applies to: 236-311, 322-323, 517-524
src/server/platform/runtime/applicationRuntime.test.ts (1)
166-209: LGTM!qualification/resources/sseMemoryScenario.ts (1)
184-208: 🩺 Stability & AvailabilityNo change needed.
Effect4Scope.make("parallel")andScope.provide(roundScope)(consumerResource)match the declared API.qualification/browser/queryCollectionAdapter.test.ts (1)
240-244: 📐 Maintainability & Code QualityPackage metadata exports are present.
All four pinned TanStack packages expose
./package.jsonas an explicit export entry, soBun.resolveSync(${packageName}/package.json, import.meta.dir)should not fail due to missing subpath exports.
2 participants
Summary
2026.7.2-beta.7protocol/source auditArchitecture and delivery boundaries
17d6843606d76620cb55d31424d7fb0aed51c367is qualification evidence, not a new repository-wide runtime pin.f1a66709a07ba45f853d18070de6ff92ae956183.Verification
bun install --frozen-lockfilebun run typecheck:qualificationbun run typecheck:serverbun run test:server: 836 tests, 3340 assertions, 0 failuresbun run test:server:docs: 13 tests, 291 assertions, 0 failuresbun run test:server:toolingbun run docs:checkbun run db:checkbun run lintbun run format:checkbun run build:frontendbun run test:frontend:coverage: 705 tests, 4493 assertions, 0 failuresbun run build:backendbun run test:backend:coverage: 738 tests, 4769 assertions, 0 failures