feat(platform): establish greenfield process boundaries - #392
Conversation
mira-2026
changed the title
mira-2026
added
type: security
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds AST-based source-boundary checks, isolated TypeScript projects, registered configuration metadata, generated configuration documentation, structured logging with request correlation, and validated tRPC error policies. CI now runs the new checks. ChangesGreenfield foundation hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (25)
scripts/sourceBoundaries/sourceDiscovery.test.ts (1)
146-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test asserts the same condition as the previous one.
The test name states that executable source hidden in an unknown root directory is rejected. The assertion only checks the
toolsdirectory violation, which the test at lines 127-144 already covers. The filetools/evil.tsis never referenced in any expectation, so the test passes whether or not discovery descends intotools.Add an assertion that states the intended behavior for the nested file.
💚 Suggested assertion
expect( violations.some( (violation) => violation.importer === "tools" && violation.message.includes("exact reviewed project layout") ) ).toBe(true); + expect( + violations.some( + (violation) => violation.importer === "tools/evil.ts" + ) + ).toBe(false);Set the expected value to match the intended discovery behavior. If discovery is meant to descend into unknown roots, assert
trueand the expected message instead.🤖 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 `@scripts/sourceBoundaries/sourceDiscovery.test.ts` around lines 146 - 167, Strengthen the test around checkSourceBoundaries so it explicitly asserts the expected violation for the nested tools/evil.ts source, not only the tools root-directory violation. Add an expectation that identifies the nested file and its intended message, using true if unknown roots should be discovered or false if they should remain excluded.scripts/sourceBoundaries/policy.test.ts (1)
571-585: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the allowlist contents, not only the size.
Line 572 pins the count to 18. If a developer removes one entry and adds another, the count still passes and the ratchet does not hold. If a developer legitimately changes the count, the failure message reports only two numbers.
An inline snapshot of the sorted keys catches both cases and shows the exact delta on failure.
🤖 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 `@scripts/sourceBoundaries/policy.test.ts` around lines 571 - 585, Update the test “freezes the exact legacy script coexistence allowlist” to assert an inline snapshot of the sorted keys in legacyScriptImportAllowlist, rather than relying only on its size. Retain the existing validation assertions and ensure the snapshot exposes exact added or removed entries and any legitimate count changes.scripts/checkSourceBoundaries.ts (1)
39-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounded concurrency for the per-file scan.
The loop reads and parses every discovered file strictly in sequence. Each iteration awaits file I/O, and later each import awaits one or more
lstat/realpathcalls. On a full repository this dominates the check runtime, andscripts/sourceBoundaries/policy.test.tsalready reserves 30 seconds for one full scan.A bounded-parallel map over
discovery.fileskeeps the output deterministic, because violations are sorted at the end.🤖 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 `@scripts/checkSourceBoundaries.ts` around lines 39 - 46, Replace the sequential per-file processing loop in the source scan with bounded-concurrency mapping over discovery.files, including validateSourceFile, file reading, parseSourceAnalysis, and downstream import checks. Preserve each file’s importer association and existing violation collection behavior, then retain the final sorting so output remains deterministic.scripts/sourceBoundaries/importTargetValidation.ts (1)
22-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared component walk.
Both functions run the same algorithm: split the repository-relative target,
lstateach component, reject symbolic links, require directories for intermediate components and a regular file for the last one, then comparerealpathagainst the repository real path. Only the messages differ.Two behaviors have drifted:
- Line 29-37 catches every
lstaterror and reports "missing or unreadable". Line 117-118 rethrows anything that is notENOENT. Pick one policy for both callers.- Line 56 and line 141 call
realpath(lexicalProjectRoot)on every validated import. The value is constant for one run, so it can be resolved once and passed in.A single helper that takes the target, the importer context, and a message set removes the duplication and the drift.
Also applies to: 109-148
🤖 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 `@scripts/sourceBoundaries/importTargetValidation.ts` around lines 22 - 65, The duplicated component-walk logic in the two validation functions should be extracted into one shared helper that accepts the target, importer context, message set, and precomputed repository real path. Make both callers use the same lstat error policy, symbolic-link and file/directory checks, and realpath containment validation, while preserving their distinct messages; resolve realpath(projectRoot) once per run and pass it to the helper.scripts/sourceBoundaries/boundaryConfiguration.test.ts (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
temporaryProjectis copied into four boundary test files. The four files define byte-identical fixture helpers because no shared test-support module exists for the source-boundary suite. A change to the fixture layout, for example adding a new required root directory, needs four edits, and a partial edit produces tests that scan different project shapes.
scripts/sourceBoundaries/boundaryConfiguration.test.ts#L8-L14: movetemporaryProjectinto a new shared module, for examplescripts/sourceBoundaries/testSupport.ts, and import it here.scripts/sourceBoundaries/checkerIntegration.test.ts#L8-L14: delete the localtemporaryProjectand import the shared helper.scripts/sourceBoundaries/importTargetValidation.test.ts#L8-L14: delete the localtemporaryProjectand import the shared helper.scripts/sourceBoundaries/sourceDiscovery.test.ts#L8-L14: delete the localtemporaryProjectand import the shared helper.Confirm that the new module path satisfies the boundary policy for the
scriptsrole before you add it.🤖 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 `@scripts/sourceBoundaries/boundaryConfiguration.test.ts` around lines 8 - 14, Centralize the byte-identical temporaryProject fixture in scripts/sourceBoundaries/testSupport.ts after confirming that module path complies with the scripts boundary policy. Remove the local helper and import the shared temporaryProject in scripts/sourceBoundaries/boundaryConfiguration.test.ts:8-14, checkerIntegration.test.ts:8-14, importTargetValidation.test.ts:8-14, and sourceDiscovery.test.ts:8-14.scripts/sourceBoundaries/importTargetValidation.test.ts (1)
233-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe legacy-allowlist test depends on a frozen allowlist entry.
The test only produces a violation if
legacyScriptImportAllowliststill contains the key forscripts/buildBackend.tsandbackend/src/services/releases/runtime.ts. If that entry is removed,validateLegacyAllowlistTargetnever runs and the assertion fails with an unclear message.Add an explicit precondition so the failure names the cause:
♻️ Suggested precondition
+ expect( + legacyScriptImportAllowlist.has( + legacyScriptImportKey("scripts/buildBackend.ts", { + kind: "import", + line: 1, + specifier: "../backend/src/services/releases/runtime.ts", + }) + ) + ).toBe(true); + const violations = await checkSourceBoundaries(projectRoot);Verify the exact key shape before you apply this, because
legacyScriptImportKeyreturnsstring | undefined.🤖 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 `@scripts/sourceBoundaries/importTargetValidation.test.ts` around lines 233 - 272, Add an explicit precondition in the test before invoking checkSourceBoundaries that computes the key with legacyScriptImportKey and verifies it is defined and present in legacyScriptImportAllowlist. Use the exact key shape returned by legacyScriptImportKey, then retain the existing symbolic-link violation assertion..oxlintrc.json (1)
279-279: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlign the browser global restriction with the contracts/shared form.
Line 279 uses the string form. The string form reports only bare identifier references. The contracts/shared override at lines 228-242 uses the object form with
checkGlobalObject: true, so it also reportsglobalThis.processandwindow.Bun. Browser source can therefore reachglobalThis.processwithout a lint error. Use the object form here as well for equal enforcement.♻️ Proposed change
- "no-restricted-globals": ["error", "Bun", "Buffer", "process"], + "no-restricted-globals": [ + "error", + { + "checkGlobalObject": true, + "globals": ["Bun", "Buffer", "Deno", "process"] + } + ],🤖 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 @.oxlintrc.json at line 279, Update the no-restricted-globals configuration in the browser override to use the object form with checkGlobalObject enabled, while retaining Bun, Buffer, and process as restricted globals. Match the existing contracts/shared override configuration so member access through globalThis or window is also reported.scripts/sourceBoundaries/runtimeOwnerAnalysis.ts (1)
92-112: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the runtime owner names into a module-level set.
Lines 93 and 107 build a new array on every call.
isRuntimeEnvironmentOwnerruns for many nodes per file throughisRuntimeAuthorityOwnerand the recursive owner checks, so the allocation repeats for each visited node. A sharedReadonlySetalso matches the existing style ofruntimeGlobalRootNames.♻️ Proposed refactor
+const runtimeEnvironmentOwnerNames: ReadonlySet<string> = new Set([ + "Bun", + "Deno", + "process", +]); + export function isRuntimeEnvironmentOwner( node: unknown, runtimeIdentifierReferences: RuntimeIdentifierReferences, staticStringValues: StaticStringValues ): boolean { if (!isRecord(node)) return false; if ( - ["Bun", "Deno", "process"].includes(identifierName(node) ?? "") && + runtimeEnvironmentOwnerNames.has(identifierName(node) ?? "") && runtimeIdentifierReferences.has(node) ) { return true; } @@ if ( - !["Bun", "Deno", "process"].includes( - memberPropertyName(node, staticStringValues) ?? "" - ) + !runtimeEnvironmentOwnerNames.has( + memberPropertyName(node, staticStringValues) ?? "" + ) ) { return 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 `@scripts/sourceBoundaries/runtimeOwnerAnalysis.ts` around lines 92 - 112, Define a module-level ReadonlySet containing the runtime owner names Bun, Deno, and process, then update isRuntimeEnvironmentOwner to reuse it for both identifier and member-property checks instead of constructing arrays on each call. Preserve the existing matching behavior and integrate with the nearby runtimeGlobalRootNames style.scripts/sourceBoundaries/importGraph.test.ts (1)
495-509: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the parse-failure path.
No test exercises invalid source text.
parseSourceAnalysisthrows for a null Babel result, and Babel throws a syntax error for unparsable input. The boundary checker runs over every discovered file, so a parse failure aborts the whole check. A test that asserts the rejection keeps that behavior explicit.💚 Proposed test
test("finds triple-slash directives that can restore ambient authority", async () => { @@ }); + + test("rejects source text that cannot be parsed", async () => { + expect( + parseSourceAnalysis("const = ;", "src/shared/invalid.ts") + ).rejects.toThrow(); + }); });🤖 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 `@scripts/sourceBoundaries/importGraph.test.ts` around lines 495 - 509, Add a test near the existing parseSourceAnalysis tests that supplies invalid TypeScript/JavaScript source and asserts the returned promise rejects with the expected parse error. Cover the null Babel-result failure path if it is separately reachable, while preserving parseSourceAnalysis’s rejection behavior so boundary checking remains explicit.src/server/platform/configuration/configurationRegistry.test.ts (1)
12-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert set equality between the name list and the registry.
The test pins the literal contents of
applicationConfigurationEnvironmentNamesand pins the registry length to 13. It never compares the two collections. A registry entry whoseenvironmentNameis absent from the const list still passes while the counts match. Add the direct comparison.💚 Proposed addition
expect(applicationConfigurationRegistry).toHaveLength(13); + expect( + applicationConfigurationRegistry + .map((entry) => entry.environmentName) + .toSorted() + ).toEqual([...applicationConfigurationEnvironmentNames].toSorted());🤖 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/server/platform/configuration/configurationRegistry.test.ts` around lines 12 - 37, Add a direct set-equality assertion in the test “accounts for every accepted environment name exactly once” comparing applicationConfigurationEnvironmentNames with the registry’s environmentName values, while preserving the existing uniqueness checks.scripts/sourceBoundaries/lintConfiguration.test.ts (1)
110-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelax the exact output equality assertion.
Line 119 requires the combined stdout and stderr to be exactly
"\n". Any unrelated oxlint notice, such as a configuration deprecation warning on stderr, then fails the test while the boundary rules still behave correctly. Assert the exit code and the absence of the specific rule identifiers instead.♻️ Proposed assertion
- expect(testResult).toEqual({ exitCode: 0, output: "\n" }); + expect(testResult.exitCode).toBe(0); + for (const rule of [ + "'memo' import from 'react' is restricted", + "import is restricted", + "no-implied-eval", + "no-console", + ]) { + expect(testResult.output).not.toContain(rule); + }🤖 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 `@scripts/sourceBoundaries/lintConfiguration.test.ts` around lines 110 - 119, Update the assertion in the runOxlint test to keep verifying exitCode 0 while replacing exact output equality with checks that output does not contain the specific boundary-rule identifiers. Allow unrelated oxlint notices or warnings without failing the test.src/server/platform/configuration/webConfiguration.test.ts (1)
348-377: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a keyring case with the wrong key size.
The keyring cases cover unknown keys, an unresolvable
activeKeyId, duplicate ids, duplicate key material, and nine keys. No case supplies a key whose decoded length is not 32 bytes. The registry documents an AES-256 constraint, so the key-size rule is security relevant and deserves direct coverage.💚 Proposed addition
const keyringCases = [ serializedKeyring({ extra: true }), serializedKeyring({ activeKeyId: "missing" }), + serializedKeyring({ + keys: [ + { id: "primary", keyBase64: Buffer.alloc(16, 1).toString("base64") }, + ], + }),🤖 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/server/platform/configuration/webConfiguration.test.ts` around lines 348 - 377, Add a case to the keyringCases array using serializedKeyring with a key whose decoded keyBase64 length is not 32 bytes, then keep it within the existing expectConfigurationError loop for MIRA_DASHBOARD_TOTP_KEYRING. Use the existing encodedKey helper or an equivalent fixture to represent the invalid AES-256 key size.tsconfig.contracts.json (1)
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord why
src/sharedmust stay environment-neutral.
lib: ["ESNext"]withtypes: []removesTextEncoder,fetch, and every Node global from this project.src/shared/**/*.tsis included, so this configuration is the constraint that forcessrc/shared/encoding.tsto count UTF-8 bytes manually instead of usingTextEncoder. Any future reintroduction of a DOM or Node global insrc/sharedbreaks this project rather than the calling code.Add a short comment here so the constraint survives future edits.
Separately, the
excludelist names only.tspatterns whiletsconfig.browser.jsonnames both.tsand.tsx. Theincludelist here is.tsonly, so the asymmetry is currently harmless. Align the two lists ifsrc/sharedever gains.tsxfiles.🤖 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 `@tsconfig.contracts.json` around lines 3 - 13, Add a short comment near the compilerOptions/include configuration documenting that src/shared must remain environment-neutral because lib ["ESNext"] and types [] exclude DOM and Node globals, including the manual UTF-8 byte counting constraint. Do not change the current exclude patterns unless src/shared gains .tsx files; if that occurs, align the contract and browser exclude lists.src/shared/encoding.ts (1)
6-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the two UTF-8 byte-length implementations.
The manual loop is correct. For every input it returns the same count as
TextEncoder, including lone surrogates, because a lone surrogate encodes as the three-byte U+FFFD replacement. The new tests atsrc/shared/encoding.test.tslines 9 and 10 confirm that.
src/server/domains/security/mfa/totpSecretCipher.tsline 102 still measures the keyring bound withtextEncoder.encode(serializedKeyRing).byteLength. Two byte-length methods now coexist. If the motivation for this change is to avoid the per-call allocation, apply it to the keyring bound too.#!/bin/bash # Description: Find remaining TextEncoder byte-length measurements that could use utf8ByteLength. rg -nP --type=ts 'encode\([^)]*\)\.byteLength'🤖 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/shared/encoding.ts` around lines 6 - 23, Replace the TextEncoder-based byte-length calculation in the keyring-bound logic of totpSecretCipher with the shared utf8ByteLength function from encoding.ts. Preserve the existing serializedKeyRing value and bound behavior while consolidating all applicable measurements onto the manual implementation.src/shared/configuration/applicationConfigurationRegistry.ts (1)
326-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude the environment name in the registry error.
The thrown message does not identify which lookup failed. The environment name is a registered constant, so including it leaks no secret and speeds up diagnosis.
♻️ Proposed change
- throw new Error("Application configuration registry is incomplete"); + throw new Error( + `Application configuration registry is missing ${environmentName}` + );🤖 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/shared/configuration/applicationConfigurationRegistry.ts` around lines 326 - 336, Update configurationMetadata so the error thrown when no registry entry matches environmentName includes the requested environment name, while preserving the existing error condition and return behavior for successful lookups.src/server/platform/configuration/webConfiguration.ts (1)
244-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFreeze the returned origin list.
trustedProxyAddressesreturnsObject.freeze(...)at line 213.webAuthnOriginsreturns the mutable array produced bysplit. The current caller passes it to a factory that produces a frozen copy, so no defect exists today. Freezing here keeps the two list parsers consistent and removes the dependency on factory behavior.♻️ Proposed change
- return values; + return Object.freeze(values);🤖 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/server/platform/configuration/webConfiguration.ts` around lines 244 - 267, Update webAuthnOrigins to return an Object.freeze-wrapped values array after validation, matching the immutable result of trustedProxyAddresses. Preserve the existing parsing and validation behavior while ensuring callers cannot mutate the returned origin list.src/server/trpc/procedureErrorPolicy.ts (1)
21-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe policy duplicates every contract
errorsarray exactly.
assertProcedureExpectedErrorPolicyrequires strict equality betweencontract.errorsandpolicy[contract.name]. The policy therefore carries no information that the contracts do not already carry, and every contract change needs a matching edit here. The JSDoc states this is intentional as a server-owned allowlist.If the intent is drift detection only, the duplication is fine. If the intent is a second, independent source of truth, add a comment that states why the two lists must be authored separately. Either way, a reader benefits from the rationale.
🤖 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/server/trpc/procedureErrorPolicy.ts` around lines 21 - 177, Add a concise comment adjacent to procedureExpectedErrorPolicy explaining why each policy errors array intentionally duplicates the corresponding contract.errors list and must be authored independently as a server-owned allowlist. Clarify whether the purpose is independent policy enforcement rather than drift detection, without changing the policy entries.src/contracts/contractRegistry.ts (2)
14-21: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDuplicate procedure names are not detected on the documentation path.
assertProcedureContractErrorsvalidates error codes only. Duplicatenamevalues are detected byassertProcedureExpectedErrorPolicyinsrc/server/trpc/procedureErrorPolicy.ts.scripts/documentation/artifacts.tsimportsprocedureContractswithout importingprocedureErrorPolicy.ts, so a duplicate name would produce duplicate documentation rows instead of a build failure.Consider adding a uniqueness check next to the existing assertion.
♻️ Proposed check
assertProcedureContractErrors(registeredProcedureContracts); +if ( + new Set(registeredProcedureContracts.map(({ name }) => name)).size !== + registeredProcedureContracts.length +) { + throw new TypeError("Procedure contract names are not unique"); +}🤖 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/contracts/contractRegistry.ts` around lines 14 - 21, Add a uniqueness assertion next to assertProcedureContractErrors for registeredProcedureContracts so duplicate procedure name values fail during documentation artifact generation. Reuse the existing procedure-contract validation behavior or shared symbol from assertProcedureExpectedErrorPolicy where appropriate, and ensure unique names continue through the existing registration flow unchanged.
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Object.freezehere is shallow.
Object.freeze(registeredProcedureContracts)protects the array shape only. EachProcedureContractobject and eacherrorsarray stay mutable at runtime. The exported value is therefore not fully immutable, although TypeScript blocks writes through the declared types.If runtime immutability is a goal for this export, freeze the entries as well.
♻️ Proposed hardening
assertProcedureContractErrors(registeredProcedureContracts); -export const procedureContracts = Object.freeze(registeredProcedureContracts); +for (const contract of registeredProcedureContracts) { + Object.freeze(contract.errors); + Object.freeze(contract); +} +export const procedureContracts = Object.freeze(registeredProcedureContracts);🤖 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/contracts/contractRegistry.ts` around lines 14 - 22, Make the exported procedureContracts deeply immutable by freezing each ProcedureContract entry, including its errors array where applicable, before freezing the outer registeredProcedureContracts array. Update the registration flow around registeredProcedureContracts and preserve assertProcedureContractErrors before applying the freezes.scripts/documentation/configurationMarkdown.test.ts (1)
24-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that covers Markdown cell escaping.
markdownTableCellescapes backslashes and pipe characters and collapses line breaks. No test exercises that path. A regression in the escape order would corrupt the generated table without failing any test. Add one entry whosedescriptioncontains|, a backslash, and a newline, then assert the rendered row.♻️ Proposed test addition
+ test("escapes Markdown table control characters", () => { + const documentation = renderConfiguration([ + { + ...completeEntry, + description: "pipe | and \\ backslash\nsecond line", + }, + ]); + + expect(documentation).toContain( + String.raw`pipe \| and \\ backslash second line` + ); + });🤖 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 `@scripts/documentation/configurationMarkdown.test.ts` around lines 24 - 103, Add a test case in the “application configuration Markdown” suite using an entry whose description contains a pipe, backslash, and newline. Render it with renderConfiguration and assert the generated Markdown row contains the correctly escaped pipe and backslash with the newline collapsed, covering markdownTableCell’s escaping behavior and order.src/server/trpc/procedureErrorPolicy.test.ts (1)
135-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the positive case for subscription errors.
This test proves the wrapper internalizes an undeclared error during iteration. No test proves the wrapper preserves a declared error during iteration. If
enforceAsyncIterableErrorsever over-sanitized, legitimate streaming failures would becomeINTERNAL_SERVER_ERRORand no test would fail.Add a case on a registered subscription path whose policy declares the thrown code, for example
events.streamwithTOO_MANY_REQUESTS.💚 Proposed test addition
+ test("preserves declared errors while a subscription is iterated", async () => { + const testRouter = router({ + events: router({ + stream: publicProcedure.subscription(async function* () { + await Promise.resolve(); + yield "started"; + throw new TRPCError({ + code: "TOO_MANY_REQUESTS", + message: "Declared streaming failure", + }); + }), + }), + }); + const caller = testRouter.createCaller(await createTestRequestContext()); + const stream = await caller.events.stream(); + const iterator = stream[Symbol.asyncIterator](); + + expect(await iterator.next()).toEqual({ done: false, value: "started" }); + const failure = await captureFailure(() => iterator.next()); + expect((failure as TRPCError).code).toBe("TOO_MANY_REQUESTS"); + });🤖 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/server/trpc/procedureErrorPolicy.test.ts` around lines 135 - 156, Add a positive subscription-iteration test alongside the existing runtimeIdentity case, using a registered policy path such as events.stream that declares TOO_MANY_REQUESTS. Make the async generator yield once and then throw TRPCError with that code, iterate twice, and assert the captured failure remains TRPCError with TOO_MANY_REQUESTS rather than being converted to INTERNAL_SERVER_ERROR.src/app/trpcHttpHandler.ts (1)
245-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn
dispatchTrpcHttpRequestdirectly.The wrapper repeats the full signature and only forwards the arguments. Rename the inner function to
handleTrpcHttpRequestand return it, so one signature stays authoritative.🤖 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/trpcHttpHandler.ts` around lines 245 - 252, Update the surrounding factory to rename the inner wrapper function to handleTrpcHttpRequest and return dispatchTrpcHttpRequest directly, removing the redundant repeated signature and argument-forwarding wrapper while preserving the existing request handling behavior.src/server/platform/observability/structuredLogger.ts (1)
301-320: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the
TextEncoderinstance out of the serialization path.
serializeRecordruns for every log record and allocates a newTextEncoderon each call, and twice on the bounded path. Create one module-level encoder.♻️ Proposed refactor
+const structuredLogEncoder = new TextEncoder(); + function serializeRecord( record: StructuredLogRecord, limits: StructuredLogLimits ): string { const serialized = `${JSON.stringify(record)}\n`; if ( - new TextEncoder().encode(serialized).byteLength <= limits.maximumSerializedBytes + structuredLogEncoder.encode(serialized).byteLength <= + limits.maximumSerializedBytes ) { return serialized; } const boundedRecord: StructuredLogRecord = { ...record, fields: { truncated: true }, }; const bounded = `${JSON.stringify(boundedRecord)}\n`; - if (new TextEncoder().encode(bounded).byteLength <= limits.maximumSerializedBytes) { + if ( + structuredLogEncoder.encode(bounded).byteLength <= + limits.maximumSerializedBytes + ) { return bounded; } throw new RangeError("Structured log envelope exceeds its byte budget"); }🤖 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/server/platform/observability/structuredLogger.ts` around lines 301 - 320, Hoist a single TextEncoder instance to module scope and reuse it in serializeRecord for both serialized and bounded byte-length checks, removing the per-call allocations while preserving the existing size-limit behavior.src/server/test/system/serverFoundation.test.ts (2)
249-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWait for log quiescence before asserting the exact record count.
The loop stops at the first emitted line. A second record that arrives later is not observed, so
expect(records).toHaveLength(1)can pass even when the server also emits a response-created or failed record. Add a short settle delay after the first line, then assert. The same pattern applies at Lines 329-336.♻️ Proposed change
for (let attempt = 0; attempt < 100 && logLines.length === 0; attempt += 1) { await Bun.sleep(5); } + await Bun.sleep(50);🤖 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/server/test/system/serverFoundation.test.ts` around lines 249 - 267, After the existing wait in the cancellation test, add a short settle delay so any additional server log records are emitted before parsing logLines and asserting the exact count. Apply the same quiescence wait to the analogous test block around the second referenced assertion, while preserving the existing record expectations.
157-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated capturing structured-logger fixture across tests. Six call sites repeat the same identity block and line-collecting sink. The shared root cause is the absence of a capturing counterpart to
createTestStructuredLoggerinsrc/server/test/support/requestContext.ts.
src/server/test/system/serverFoundation.test.ts#L157-L170: replace this block, and the identical blocks at Lines 214-227, 272-285, and 353-366, with the shared fixture.src/app/trpcHttpHandler.test.ts#L46-L59: replace this block, and the identical block at Lines 121-134, with the shared fixture.🤖 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/server/test/system/serverFoundation.test.ts` around lines 157 - 170, The duplicated structured-logger setup should be centralized in a capturing counterpart to createTestStructuredLogger within requestContext.ts. Add and reuse that shared fixture at src/server/test/system/serverFoundation.test.ts lines 157-170, 214-227, 272-285, and 353-366, and src/app/trpcHttpHandler.test.ts lines 46-59 and 121-134, preserving access to the captured log lines at each call site.
🤖 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 `@docs/architecture/greenfield-rewrite/progress.md`:
- Around line 10-18: Update the Phase 0 evidence entry in the progress table so
its stated count matches the listed evidence: either change “eight mandatory
spikes” to “nine,” or explicitly distinguish capped resource evidence from the
eight named spikes.
In `@scripts/sourceBoundaries/boundaryConfiguration.ts`:
- Around line 13-15: Update isRecord to reject arrays in addition to null and
non-object values, so only plain object-shaped records pass validation. Preserve
the existing type-predicate behavior for accepted record values and ensure both
top-level configuration and dependencies checks fail closed for array inputs.
- Around line 43-54: Update readRootJson to parse tsconfig*.json files with the
existing json5 dependency so comments and trailing commas are accepted, while
continuing to use strict JSON.parse validation for package.json. Preserve the
current invalid-configuration violation handling and return behavior for both
parser paths.
In `@scripts/sourceBoundaries/sourceDiscovery.ts`:
- Around line 207-222: Update discoverSourceFiles and/or discoverDirectory so
missing scanned roots such as scripts or src are converted into
SourceBoundaryViolation entries rather than allowing lstat ENOENT to reject the
scan. Preserve scanning of existing directories and ensure the function returns
the normal SourceDiscovery result with an actionable violation for each absent
root.
In `@src/server/platform/configuration/webConfiguration.ts`:
- Around line 294-306: Update WebAuthnRelyingPartyConfigurationInput validation
to validate the RP name before invoking createWebAuthnRelyingPartyConfiguration,
routing RP-name failures through the RP-name environment variable and preserving
its existing validation path. Keep origin-related factory failures attributed to
the validated origins field/environment variable, while RP ID and RP-name errors
use their corresponding inputs instead of always returning
configurationError(originsField, "inconsistent").
In `@src/server/trpc/procedureErrorPolicy.ts`:
- Around line 241-249: Update the runtimeProcedureExpectedErrorPolicy lookup in
the surrounding error-policy logic to use Object.hasOwn before reading
expectedErrors, ensuring inherited keys such as toString, constructor, and
valueOf are treated as unregistered while preserving registered-route behavior
and the existing INTERNAL_SERVER_ERROR fallback.
In `@tsconfig.json`:
- Around line 26-32: Remove the project references from the root tsconfig
configuration unless these child configs are being fully converted to composite
projects. If retaining them, configure every referenced project consistently
with composite enabled and its own tsBuildInfoFile, and ensure the complete
reference set supports tsc --build.
In `@tsconfig.scripts.json`:
- Line 9: Remove the frontend/src/globals.d.ts entry from the files list in
tsconfig.scripts.json, leaving the scripts program limited to declarations it
actually uses and preserving the existing tsconfig.node.json boundary.
In `@tsconfig.worker.json`:
- Around line 7-12: Update the include list in tsconfig.worker.json to remove
the nonexistent src/app/worker.ts entry and include the existing worker
implementation at backend/src/services/jobExecutionQueue/worker.ts, ensuring the
worker typecheck targets the actual worker root.
---
Nitpick comments:
In @.oxlintrc.json:
- Line 279: Update the no-restricted-globals configuration in the browser
override to use the object form with checkGlobalObject enabled, while retaining
Bun, Buffer, and process as restricted globals. Match the existing
contracts/shared override configuration so member access through globalThis or
window is also reported.
In `@scripts/checkSourceBoundaries.ts`:
- Around line 39-46: Replace the sequential per-file processing loop in the
source scan with bounded-concurrency mapping over discovery.files, including
validateSourceFile, file reading, parseSourceAnalysis, and downstream import
checks. Preserve each file’s importer association and existing violation
collection behavior, then retain the final sorting so output remains
deterministic.
In `@scripts/documentation/configurationMarkdown.test.ts`:
- Around line 24-103: Add a test case in the “application configuration
Markdown” suite using an entry whose description contains a pipe, backslash, and
newline. Render it with renderConfiguration and assert the generated Markdown
row contains the correctly escaped pipe and backslash with the newline
collapsed, covering markdownTableCell’s escaping behavior and order.
In `@scripts/sourceBoundaries/boundaryConfiguration.test.ts`:
- Around line 8-14: Centralize the byte-identical temporaryProject fixture in
scripts/sourceBoundaries/testSupport.ts after confirming that module path
complies with the scripts boundary policy. Remove the local helper and import
the shared temporaryProject in
scripts/sourceBoundaries/boundaryConfiguration.test.ts:8-14,
checkerIntegration.test.ts:8-14, importTargetValidation.test.ts:8-14, and
sourceDiscovery.test.ts:8-14.
In `@scripts/sourceBoundaries/importGraph.test.ts`:
- Around line 495-509: Add a test near the existing parseSourceAnalysis tests
that supplies invalid TypeScript/JavaScript source and asserts the returned
promise rejects with the expected parse error. Cover the null Babel-result
failure path if it is separately reachable, while preserving
parseSourceAnalysis’s rejection behavior so boundary checking remains explicit.
In `@scripts/sourceBoundaries/importTargetValidation.test.ts`:
- Around line 233-272: Add an explicit precondition in the test before invoking
checkSourceBoundaries that computes the key with legacyScriptImportKey and
verifies it is defined and present in legacyScriptImportAllowlist. Use the exact
key shape returned by legacyScriptImportKey, then retain the existing
symbolic-link violation assertion.
In `@scripts/sourceBoundaries/importTargetValidation.ts`:
- Around line 22-65: The duplicated component-walk logic in the two validation
functions should be extracted into one shared helper that accepts the target,
importer context, message set, and precomputed repository real path. Make both
callers use the same lstat error policy, symbolic-link and file/directory
checks, and realpath containment validation, while preserving their distinct
messages; resolve realpath(projectRoot) once per run and pass it to the helper.
In `@scripts/sourceBoundaries/lintConfiguration.test.ts`:
- Around line 110-119: Update the assertion in the runOxlint test to keep
verifying exitCode 0 while replacing exact output equality with checks that
output does not contain the specific boundary-rule identifiers. Allow unrelated
oxlint notices or warnings without failing the test.
In `@scripts/sourceBoundaries/policy.test.ts`:
- Around line 571-585: Update the test “freezes the exact legacy script
coexistence allowlist” to assert an inline snapshot of the sorted keys in
legacyScriptImportAllowlist, rather than relying only on its size. Retain the
existing validation assertions and ensure the snapshot exposes exact added or
removed entries and any legitimate count changes.
In `@scripts/sourceBoundaries/runtimeOwnerAnalysis.ts`:
- Around line 92-112: Define a module-level ReadonlySet containing the runtime
owner names Bun, Deno, and process, then update isRuntimeEnvironmentOwner to
reuse it for both identifier and member-property checks instead of constructing
arrays on each call. Preserve the existing matching behavior and integrate with
the nearby runtimeGlobalRootNames style.
In `@scripts/sourceBoundaries/sourceDiscovery.test.ts`:
- Around line 146-167: Strengthen the test around checkSourceBoundaries so it
explicitly asserts the expected violation for the nested tools/evil.ts source,
not only the tools root-directory violation. Add an expectation that identifies
the nested file and its intended message, using true if unknown roots should be
discovered or false if they should remain excluded.
In `@src/app/trpcHttpHandler.ts`:
- Around line 245-252: Update the surrounding factory to rename the inner
wrapper function to handleTrpcHttpRequest and return dispatchTrpcHttpRequest
directly, removing the redundant repeated signature and argument-forwarding
wrapper while preserving the existing request handling behavior.
In `@src/contracts/contractRegistry.ts`:
- Around line 14-21: Add a uniqueness assertion next to
assertProcedureContractErrors for registeredProcedureContracts so duplicate
procedure name values fail during documentation artifact generation. Reuse the
existing procedure-contract validation behavior or shared symbol from
assertProcedureExpectedErrorPolicy where appropriate, and ensure unique names
continue through the existing registration flow unchanged.
- Around line 14-22: Make the exported procedureContracts deeply immutable by
freezing each ProcedureContract entry, including its errors array where
applicable, before freezing the outer registeredProcedureContracts array. Update
the registration flow around registeredProcedureContracts and preserve
assertProcedureContractErrors before applying the freezes.
In `@src/server/platform/configuration/configurationRegistry.test.ts`:
- Around line 12-37: Add a direct set-equality assertion in the test “accounts
for every accepted environment name exactly once” comparing
applicationConfigurationEnvironmentNames with the registry’s environmentName
values, while preserving the existing uniqueness checks.
In `@src/server/platform/configuration/webConfiguration.test.ts`:
- Around line 348-377: Add a case to the keyringCases array using
serializedKeyring with a key whose decoded keyBase64 length is not 32 bytes,
then keep it within the existing expectConfigurationError loop for
MIRA_DASHBOARD_TOTP_KEYRING. Use the existing encodedKey helper or an equivalent
fixture to represent the invalid AES-256 key size.
In `@src/server/platform/configuration/webConfiguration.ts`:
- Around line 244-267: Update webAuthnOrigins to return an Object.freeze-wrapped
values array after validation, matching the immutable result of
trustedProxyAddresses. Preserve the existing parsing and validation behavior
while ensuring callers cannot mutate the returned origin list.
In `@src/server/platform/observability/structuredLogger.ts`:
- Around line 301-320: Hoist a single TextEncoder instance to module scope and
reuse it in serializeRecord for both serialized and bounded byte-length checks,
removing the per-call allocations while preserving the existing size-limit
behavior.
In `@src/server/test/system/serverFoundation.test.ts`:
- Around line 249-267: After the existing wait in the cancellation test, add a
short settle delay so any additional server log records are emitted before
parsing logLines and asserting the exact count. Apply the same quiescence wait
to the analogous test block around the second referenced assertion, while
preserving the existing record expectations.
- Around line 157-170: The duplicated structured-logger setup should be
centralized in a capturing counterpart to createTestStructuredLogger within
requestContext.ts. Add and reuse that shared fixture at
src/server/test/system/serverFoundation.test.ts lines 157-170, 214-227, 272-285,
and 353-366, and src/app/trpcHttpHandler.test.ts lines 46-59 and 121-134,
preserving access to the captured log lines at each call site.
In `@src/server/trpc/procedureErrorPolicy.test.ts`:
- Around line 135-156: Add a positive subscription-iteration test alongside the
existing runtimeIdentity case, using a registered policy path such as
events.stream that declares TOO_MANY_REQUESTS. Make the async generator yield
once and then throw TRPCError with that code, iterate twice, and assert the
captured failure remains TRPCError with TOO_MANY_REQUESTS rather than being
converted to INTERNAL_SERVER_ERROR.
In `@src/server/trpc/procedureErrorPolicy.ts`:
- Around line 21-177: Add a concise comment adjacent to
procedureExpectedErrorPolicy explaining why each policy errors array
intentionally duplicates the corresponding contract.errors list and must be
authored independently as a server-owned allowlist. Clarify whether the purpose
is independent policy enforcement rather than drift detection, without changing
the policy entries.
In `@src/shared/configuration/applicationConfigurationRegistry.ts`:
- Around line 326-336: Update configurationMetadata so the error thrown when no
registry entry matches environmentName includes the requested environment name,
while preserving the existing error condition and return behavior for successful
lookups.
In `@src/shared/encoding.ts`:
- Around line 6-23: Replace the TextEncoder-based byte-length calculation in the
keyring-bound logic of totpSecretCipher with the shared utf8ByteLength function
from encoding.ts. Preserve the existing serializedKeyRing value and bound
behavior while consolidating all applicable measurements onto the manual
implementation.
In `@tsconfig.contracts.json`:
- Around line 3-13: Add a short comment near the compilerOptions/include
configuration documenting that src/shared must remain environment-neutral
because lib ["ESNext"] and types [] exclude DOM and Node globals, including the
manual UTF-8 byte counting constraint. Do not change the current exclude
patterns unless src/shared gains .tsx files; if that occurs, align the contract
and browser exclude lists.
🪄 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: 01a41f44-dcb4-4679-935a-a4c379be3e0d
⛔ Files ignored due to path filters (2)
docs/generated/README.mdis excluded by!**/generated/**and included by**/*docs/generated/configuration.mdis excluded by!**/generated/**and included by**/*
📒 Files selected for processing (83)
.github/workflows/dashboard-checks.yml.oxlintrc.jsondocs/architecture/greenfield-rewrite/application-architecture.mddocs/architecture/greenfield-rewrite/progress.mddocs/architecture/greenfield-rewrite/runtime-and-delivery.mdpackage.jsonscripts/buildFrontend.tsscripts/checkSourceBoundaries.tsscripts/documentation/artifacts.test.tsscripts/documentation/artifacts.tsscripts/documentation/configurationMarkdown.test.tsscripts/documentation/markdown.tsscripts/frontendBuild.tsscripts/sourceBoundaries/boundaryConfiguration.test.tsscripts/sourceBoundaries/boundaryConfiguration.tsscripts/sourceBoundaries/checkerIntegration.test.tsscripts/sourceBoundaries/externalAuthorityPolicy.tsscripts/sourceBoundaries/importGraph.test.tsscripts/sourceBoundaries/importGraph.tsscripts/sourceBoundaries/importTargetValidation.test.tsscripts/sourceBoundaries/importTargetValidation.tsscripts/sourceBoundaries/lintConfiguration.test.tsscripts/sourceBoundaries/policy.test.tsscripts/sourceBoundaries/policy.tsscripts/sourceBoundaries/policyTypes.tsscripts/sourceBoundaries/runtimeAuthorityAnalysis.tsscripts/sourceBoundaries/runtimeCodeAuthorityAnalysis.tsscripts/sourceBoundaries/runtimeOwnerAnalysis.tsscripts/sourceBoundaries/sourceAst.tsscripts/sourceBoundaries/sourceBoundaryPaths.tsscripts/sourceBoundaries/sourceDirectives.tsscripts/sourceBoundaries/sourceDiscovery.test.tsscripts/sourceBoundaries/sourceDiscovery.tsscripts/sourceBoundaries/sourceTopologyPolicy.tssrc/app/environmentSource.tssrc/app/server.tssrc/app/trpcHttpHandler.test.tssrc/app/trpcHttpHandler.tssrc/contracts/contractRegistry.test.tssrc/contracts/contractRegistry.tssrc/contracts/registry.tssrc/server/domains/realtime/procedures.test.tssrc/server/domains/security/authenticationLifecycle.rateLimit.test.tssrc/server/domains/security/authenticationWorkGate.test.tssrc/server/domains/security/authenticationWorkGate.webAuthn.test.tssrc/server/domains/security/mfa/totpSecretCipher.tssrc/server/platform/configuration/applicationConfigurationError.tssrc/server/platform/configuration/configurationRegistry.test.tssrc/server/platform/configuration/webConfiguration.test.tssrc/server/platform/configuration/webConfiguration.tssrc/server/platform/errors/safeFailure.test.tssrc/server/platform/errors/safeFailure.tssrc/server/platform/observability/effectLogger.test.tssrc/server/platform/observability/effectLogger.tssrc/server/platform/observability/structuredLogger.test.tssrc/server/platform/observability/structuredLogger.tssrc/server/platform/realtime/eventPumpService.tssrc/server/platform/runtime/applicationRuntime.test.tssrc/server/platform/runtime/applicationRuntime.tssrc/server/test/contracts/trpcErrors.test.tssrc/server/test/support/requestContext.tssrc/server/test/system/serverAutomationSecurity.test.tssrc/server/test/system/serverAutomationSecurityLeaseInvalidation.test.tssrc/server/test/system/serverFoundation.test.tssrc/server/test/system/serverGatewayCredentialVerification.test.tssrc/server/test/system/serverShutdown.test.tssrc/server/trpc/appRouter.test.tssrc/server/trpc/context.test.tssrc/server/trpc/context.tssrc/server/trpc/procedureErrorPolicy.test.tssrc/server/trpc/procedureErrorPolicy.tssrc/server/trpc/trpc.test.tssrc/server/trpc/trpc.tssrc/shared/configuration/applicationConfigurationRegistry.tssrc/shared/encoding.test.tssrc/shared/encoding.tstsconfig.browser.jsontsconfig.contracts.jsontsconfig.jsontsconfig.node.jsontsconfig.scripts.jsontsconfig.server.jsontsconfig.worker.json
💤 Files with no reviewable changes (1)
- tsconfig.node.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze JavaScript and TypeScript
🧰 Additional context used
🪛 ast-grep (0.45.0)
src/app/environmentSource.ts
[error] 22-24: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const environmentName of configurationEnvironmentNamesForRole(role)) {
environment[environmentName] = process.env[environmentName];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
src/server/test/contracts/trpcErrors.test.ts
[error] 56-70: Avoid SQL injection
Context: publicProcedure.query(() => {
const error = authenticationPolicyError(
"step_up_required",
"Recent authentication is required"
);
const { cause } = error;
if (cause === undefined) {
throw new Error("Authentication policy cause is missing");
}
Object.assign(cause, {
message: sentinel,
reason: "unknown_policy_reason",
});
throw error;
})
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-typescript)
🪛 OpenGrep (1.26.0)
scripts/sourceBoundaries/externalAuthorityPolicy.ts
[ERROR] 55-55: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
src/server/platform/configuration/webConfiguration.ts
[ERROR] 180-180: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/test/support/requestContext.ts (1)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the observed and expected counts in the failure message.
The helper throws a fixed message. A CI failure then does not show whether the count was short, exceeded, or unstable. Add the counts and the captured records to the message to make the failure diagnosable.
♻️ Proposed change
- throw new Error("Test log records did not reach a stable expected count"); + throw new Error( + `Test log records did not reach a stable expected count: expected ${String( + expectedCount + )}, observed ${String(logLines.length)}: ${JSON.stringify(logLines)}` + );🤖 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/server/test/support/requestContext.ts` around lines 92 - 107, Update waitForTestLogQuiescence to include the observed logLines.length, expectedCount, and captured logLines records in the final thrown error message, while preserving the existing stability-check 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.
Nitpick comments:
In `@src/server/test/support/requestContext.ts`:
- Around line 92-107: Update waitForTestLogQuiescence to include the observed
logLines.length, expectedCount, and captured logLines records in the final
thrown error message, while preserving the existing stability-check behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0017a086-8e3e-4da4-b676-0ccc73161488
⛔ 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 (40)
.oxlintrc.jsonbackend/src/requestPolicy/evaluator.tsbackend/test/utilityBehavior.test.tsdocs/architecture/greenfield-rewrite/application-architecture.mddocs/architecture/greenfield-rewrite/progress.mddocs/architecture/greenfield-rewrite/runtime-and-delivery.mdpackage.jsonscripts/checkSourceBoundaries.tsscripts/documentation/configurationMarkdown.test.tsscripts/sourceBoundaries/boundaryConfiguration.test.tsscripts/sourceBoundaries/boundaryConfiguration.tsscripts/sourceBoundaries/checkerIntegration.test.tsscripts/sourceBoundaries/importGraph.test.tsscripts/sourceBoundaries/importTargetValidation.test.tsscripts/sourceBoundaries/importTargetValidation.tsscripts/sourceBoundaries/lintConfiguration.test.tsscripts/sourceBoundaries/policy.test.tsscripts/sourceBoundaries/runtimeOwnerAnalysis.tsscripts/sourceBoundaries/sourceDiscovery.test.tsscripts/sourceBoundaries/sourceDiscovery.tsscripts/sourceBoundaries/testSupport.tssrc/app/trpcHttpHandler.test.tssrc/app/trpcHttpHandler.tssrc/contracts/contractRegistry.test.tssrc/contracts/contractRegistry.tssrc/contracts/registry.tssrc/server/domains/security/mfa/totpSecretCipher.tssrc/server/platform/configuration/configurationRegistry.test.tssrc/server/platform/configuration/webConfiguration.test.tssrc/server/platform/configuration/webConfiguration.tssrc/server/platform/observability/structuredLogger.tssrc/server/test/support/requestContext.tssrc/server/test/system/serverFoundation.test.tssrc/server/trpc/procedureErrorPolicy.test.tssrc/server/trpc/procedureErrorPolicy.tssrc/shared/configuration/applicationConfigurationRegistry.tstsconfig.contracts.jsontsconfig.jsontsconfig.scripts.jsontsconfig.worker.json
🚧 Files skipped from review as they are similar to previous changes (28)
- tsconfig.contracts.json
- tsconfig.scripts.json
- src/contracts/contractRegistry.ts
- scripts/sourceBoundaries/lintConfiguration.test.ts
- scripts/documentation/configurationMarkdown.test.ts
- package.json
- src/server/trpc/procedureErrorPolicy.test.ts
- src/contracts/contractRegistry.test.ts
- .oxlintrc.json
- src/app/trpcHttpHandler.test.ts
- tsconfig.worker.json
- src/server/platform/configuration/webConfiguration.test.ts
- src/server/platform/configuration/configurationRegistry.test.ts
- src/app/trpcHttpHandler.ts
- scripts/sourceBoundaries/checkerIntegration.test.ts
- scripts/sourceBoundaries/runtimeOwnerAnalysis.ts
- scripts/sourceBoundaries/sourceDiscovery.ts
- src/contracts/registry.ts
- src/server/platform/configuration/webConfiguration.ts
- src/server/trpc/procedureErrorPolicy.ts
- docs/architecture/greenfield-rewrite/progress.md
- src/shared/configuration/applicationConfigurationRegistry.ts
- src/server/domains/security/mfa/totpSecretCipher.ts
- scripts/sourceBoundaries/policy.test.ts
- docs/architecture/greenfield-rewrite/application-architecture.md
- scripts/sourceBoundaries/importGraph.test.ts
- src/server/platform/observability/structuredLogger.ts
- docs/architecture/greenfield-rewrite/runtime-and-delivery.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: frontend-checks
- GitHub Check: Analyze JavaScript and TypeScript
🔇 Additional comments (14)
backend/src/requestPolicy/evaluator.ts (1)
102-105: LGTM!backend/test/utilityBehavior.test.ts (1)
1281-1281: LGTM!scripts/checkSourceBoundaries.ts (1)
25-52: LGTM!Also applies to: 54-140, 157-174
scripts/sourceBoundaries/boundaryConfiguration.test.ts (1)
9-76: LGTM!Also applies to: 148-173, 175-294, 296-329
scripts/sourceBoundaries/boundaryConfiguration.ts (3)
154-207: LGTM!Also applies to: 371-381, 408-419
25-152: 🗄️ Data Integrity & IntegrationReviewed TypeScript configuration policies match the referenced configuration files.
4-4: 🗄️ Data Integrity & IntegrationNo change needed.
jsonc-parseris declared as3.3.1in bothpackage.jsonandbun.lock, andjsonc-parser@3.3.1exposesparse(text, errors?, options?).> Likely an incorrect or invalid review comment.scripts/sourceBoundaries/importTargetValidation.test.ts (1)
7-8: LGTM!Also applies to: 231-243
scripts/sourceBoundaries/importTargetValidation.ts (1)
9-44: LGTM!Also applies to: 46-81, 106-141
scripts/sourceBoundaries/sourceDiscovery.test.ts (1)
7-7: LGTM!Also applies to: 10-30, 179-181
scripts/sourceBoundaries/testSupport.ts (1)
1-15: LGTM!src/server/test/support/requestContext.ts (1)
58-85: LGTM!src/server/test/system/serverFoundation.test.ts (1)
21-25: LGTM!Also applies to: 157-157, 173-173, 201-201, 226-226, 243-243, 287-287, 308-308, 327-327
tsconfig.json (1)
21-37: 🗄️ Data Integrity & IntegrationNo change needed. The repository-wide
tsconfig.jsonlegacy source scan does not find enums, namespaces,import =assignments, or constructor parameter properties.
1 participant
Summary
ManagedRuntime, correlate application-handled HTTP requests, safely classify failures, and enforce exact declared tRPC error policiesBehavior and regression coverage
jsonc-parser@3.3.1is exact-pinned as a development dependency so TypeScript configuration comments and trailing commas are parsed correctly whilepackage.jsonremains strict JSONVerification
bun run lintbun run format:checkbun run build:frontendbun run test:frontend:coverage— 705 passedbun run build:backendbun run test:backend:coverage— 738 passedbun run test:boundaries,bun run test:qualification,bun run test:server,bun run test:server:docs,bun run test:server:toolingAdditional gates:
bun install --frozen-lockfile— no driftbun run check:boundariesbun run docs:checkbun run db:checkgit diff --checkRisk checklist
.envfiles, database dumps, or runtime state committedDeployment / operations
Notes for reviewers
tsc -ppartitions, and on the fail-closed source-discovery/import-target behavior. The mixed-ambient root is intentionally not a standalonetscgate; all six strict partitions are checked independently.jsonc-parseris the only dependency/lockfile delta and is exact-pinned for deterministic tooling.fc238b3f01cd4427bde17d71b1d42bd51307aa04; that lower head remains unchanged.