feat(coupling): complete server-only protected materialization - #3
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces the coupling server with a Linux materialization service. It adds credential-backed key brokering, DPoP authentication, sandboxed rendition and attribution workflows, deterministic archive generation, control-plane orchestration, hardened deployment scripts, and server-only native watermark codecs. ChangesMaterialization service
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
@coderabbitai full review |
|
@codex review |
✅ Action performedFull review finished. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aaaa6f874
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
src/materializationService.ts-1462-1473 (1)
1462-1473: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
materializerIdis validated only inside the consume call.
requireString(input.materializerId, ...)runs at Line 1471, but the rawinput.materializerIdis what's sent to the upload-ticket and completion endpoints. Normalize once at the top ofmaterializeCapabilityJoband reuse it.Also applies to: 1594-1634
🤖 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/materializationService.ts` around lines 1462 - 1473, Normalize and validate input.materializerId once at the start of materializeCapabilityJob, then reuse that normalized value throughout the job. Replace the raw input.materializerId used by the upload-ticket and completion endpoint flows, as well as the consumeCapability call, while preserving the existing materializerId validation behavior.src/materializationServer.test.ts-127-156 (1)
127-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest name claims the maximum boundary but exercises half of it.
MAXIMUM_CAPABILITY_BYTESis 2 MiB while this uses 1 MiB. Worth noting the real boundary is unreachable:MAXIMUM_RESPONSE_BYTESequalsMAXIMUM_CAPABILITY_BYTES, so a 2 MiB capability plus the surrounding JSON envelope trips the response-size check inreadJsonResponsebeforerequireBase64Urlever sees it. Either rename this to reflect the value tested, or add a case at 2 MiB to pin the intended behavior.🤖 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/materializationServer.test.ts` around lines 127 - 156, Update the boundary test around createMaterializationWorker so its name and capability length accurately reflect the 1 MiB value currently exercised, or add a separate 2 MiB case that verifies the intended maximum behavior while accounting for readJsonResponse’s response-size limit before requireBase64Url validation.src/materializationServer.ts-618-628 (1)
618-628: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject non-string subject fields at the HTTP boundary.
The broker rejects empty strings and unsafe epochs, butString(...)still turns arrays/objects into valid-looking IDs ("[object Object]","a,b"), so malformed JSON can reachprepareSubjectas if it were real input. Validate the raw body here instead of relying on coercion.🤖 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/materializationServer.ts` around lines 618 - 628, Update the request handling around readBoundedRequestJson and input.keyBroker.prepareSubject to validate buyerId, creatorId, jobId, and productId as actual non-empty strings before calling the broker. Remove String coercion, reject arrays, objects, and other non-string values at the HTTP boundary using the existing request error response pattern, while preserving numeric keyEpoch validation and the successful prepareSubject flow.src/linuxAttributionWorker.py-33-49 (1)
33-49: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPartially-read keys aren't zeroized on a truncated request.
If
read_exactraises mid-comprehension while buildingkeys(line 46), the file-key bytearrays already read for earlier candidates are discarded without being wiped — inconsistent with the explicit zeroization elsewhere in this file (lines 118, 174-176).🔒 Proposed fix to zero partially-read keys on failure
def read_request(): header_length = struct.unpack(">I", read_exact(4))[0] if header_length <= 0 or header_length > MAX_HEADER_BYTES: raise ValueError("attribution pipe header length is invalid") header = json.loads(read_exact(header_length)) if header.get("schemaVersion") != 2: raise ValueError("attribution pipe schema is unsupported") assets = header.get("assets") candidates = header.get("candidates") if not isinstance(assets, list) or not 1 <= len(assets) <= 512: raise ValueError("assets are outside the attribution limit") if not isinstance(candidates, list) or not 1 <= len(candidates) <= 4096: raise ValueError("candidates are outside the attribution limit") - keys = [bytearray(read_exact(FILE_KEY_BYTES)) for _ in candidates] + keys = [] + try: + for _ in candidates: + keys.append(bytearray(read_exact(FILE_KEY_BYTES))) + except Exception: + for file_key in keys: + for index in range(len(file_key)): + file_key[index] = 0 + raise if sys.stdin.buffer.read(1): raise ValueError("attribution pipe frame contains trailing bytes") return assets, candidates, keys🤖 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/linuxAttributionWorker.py` around lines 33 - 49, Update read_request so key bytearrays accumulated during the candidates read are zeroized if read_exact fails partway through the comprehension. Replace the direct comprehension with cleanup-safe accumulation, wipe all already-read keys in the exception path, then re-raise the original failure; preserve the existing successful return behavior and trailing-byte validation.src/linuxTreeSandbox.ts-78-79 (1)
78-79: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait
child.stdin.end()before waiting onchild.exited. If the flush is async,Promise.all([...])can race with the final write and truncate the worker request.🤖 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/linuxTreeSandbox.ts` around lines 78 - 79, Update the request-writing flow around child.stdin.write and child.stdin.end so stdin closure is awaited before awaiting child.exited. Ensure the final serialized request is fully flushed before worker completion is observed, preserving the existing request and exit handling.src/linuxCodecSandbox.ts-188-223 (1)
188-223: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRun the container as non-root and cap swap
docker runstill defaults to root here, and without--memory-swap=256mthe 256m limit can spill into host swap on swap-enabled machines. The same isolation flags are duplicated inlinuxTreeSandbox.ts, so a shared helper would keep both sandboxes aligned.🤖 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/linuxCodecSandbox.ts` around lines 188 - 223, Update the Docker arguments in the sandbox command around PINNED_PYTHON_IMAGE to run as a non-root user and add a memory-swap limit matching the existing 256m memory limit. Extract the shared isolation arguments into a reusable helper and apply it in both linuxCodecSandbox and linuxTreeSandbox so their security settings remain aligned.yucp_coupling/guard.c-1997-1998 (1)
1997-1998: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe placement-epoch search loops execute exactly once.
placement_epoch < 1umeans only epoch0is ever tried in bothxg_0124andxg_0125, so the multi-epoch carrier search the surrounding structure implies is inert — the loops are just obfuscated single passes. Either raise the bound to the intended epoch count (and keep encoder/decoder in lockstep) or drop the loop and pass0udirectly.Note the encoder and decoder must always agree on this bound; a future change to one side alone silently breaks recovery of previously published assets.
Also applies to: 2144-2145
🤖 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 `@yucp_coupling/guard.c` around lines 1997 - 1998, The placement-epoch loops in xg_0124 and xg_0125 are hardcoded to a single iteration; either replace the bound with the intended shared epoch count on both encoder and decoder paths, or remove the loops and pass 0u directly. Keep both sides synchronized so published assets remain recoverable.yucp_coupling/guard.c-1710-1724 (1)
1710-1724: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
callocfor the slot table to get a checked multiplication.
vl.n > SIZE_MAX / 3ubounds the element count but notslot_capacity * sizeof(size_t), so the allocation size can still wrap.callocperforms the overflow-checked product.🛡️ Proposed fix
- size_t* valid_slots = (size_t*)malloc(slot_capacity * sizeof(size_t)); + size_t* valid_slots = (size_t*)calloc(slot_capacity, sizeof(size_t));🤖 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 `@yucp_coupling/guard.c` around lines 1710 - 1724, Replace the valid_slots allocation in the slot-capacity handling with calloc so the multiplication by sizeof(size_t) is overflow-checked, while preserving the existing capacity validation, cleanup, and failure return behavior.Source: Linters/SAST tools
yucp_coupling/build.sh-41-60 (1)
41-60: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBuild breaks if the repository path contains spaces.
LINK_FLAGSandSODIUM_CFLAGSembed$HERE/$SODIUM_PREFIXand are deliberately word-split at Line 60, so any space in the checkout path splits the--version-script=and-Iarguments. Keep the path-bearing arguments as separate quoted words.♻️ Proposed change
-LINK_FLAGS="$LINK_MODE -pthread -Wl,-z,relro,-z,now,-z,noexecstack -Wl,--version-script=$HERE/yucp_coupling.exports.map" +LINK_FLAGS="$LINK_MODE -pthread -Wl,-z,relro,-z,now,-z,noexecstack" @@ -cc $CFLAGS $SODIUM_CFLAGS "$SRC" $LINK_FLAGS -o "$OUT_LIB" $SODIUM_LINK -lm +cc $CFLAGS -I"$SODIUM_PREFIX/include" "$SRC" $LINK_FLAGS \ + "-Wl,--version-script=$HERE/yucp_coupling.exports.map" \ + -o "$OUT_LIB" "$SODIUM_LINK" -lm🤖 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 `@yucp_coupling/build.sh` around lines 41 - 60, Update the build command using LINK_FLAGS and SODIUM_CFLAGS so path-bearing version-script and include arguments remain separate quoted words when $HERE or $SODIUM_PREFIX contains spaces. Preserve the existing linker flags and vendored static archive usage, and avoid relying on word splitting of those variables.
🧹 Nitpick comments (21)
src/materializationService.test.ts (2)
322-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCatch-all branch masks unexpected URLs.
Any URL that doesn't match the earlier branches is answered with a capability-consume payload, so a wrong-URL regression would still pass. Assert the consume path explicitly and return a 404 otherwise.
🤖 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/materializationService.test.ts` around lines 322 - 357, The request handler in the test’s URL-matching mock should only return the capability-consume payload for the expected consume endpoint. Add an explicit assertion for that URL or branch, and return a 404 response for all unmatched URLs so incorrect requests cannot be accepted.
178-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture has no
classification: 'common'file, so source assembly of common content is untested.Adding one common file (and asserting it lands in the assembled tree) would cover the gap flagged in
src/materializationService.ts, whereloadSourceManifestreturns only protected files.🤖 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/materializationService.test.ts` around lines 178 - 215, Extend the sourceManifest fixture with a file classified as 'common', including its chunk metadata and content, then update the source assembly assertions to verify that file appears in the assembled tree. Keep the existing protected-file coverage intact while exercising the common-content path used by loadSourceManifest.src/rendition.test.ts (1)
16-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate PNG fixture helpers.
crc32,pngChunk, andmakeGrayPngare byte-identical to those insrc/linuxRendition.realtest.ts. Extract into a shared test fixture module to keep the two suites in sync.🤖 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/rendition.test.ts` around lines 16 - 59, Extract the duplicate crc32, pngChunk, and makeGrayPng helpers from rendition.test.ts into a shared test fixture module, then import and reuse them in both rendition.test.ts and linuxRendition.realtest.ts. Preserve their current behavior and remove the duplicate local definitions so the suites stay synchronized.src/linuxRendition.realtest.ts (1)
112-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuntime path resolution repeated four times; digest pinned only in the first test.
Hoist the
runtimePath/expected-digest resolution into a module-level helper so every rendition test runs against a verified runtime binary rather than whatever.sohappens to be on disk.Also applies to: 191-199, 250-258, 305-313
🤖 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/linuxRendition.realtest.ts` around lines 112 - 123, Create a module-level helper in linuxRendition.realtest.ts that resolves the runtimePath, reads and normalizes the expected digest, verifies the binary hash, and returns the verified runtime path. Replace the repeated runtimePath and digest-resolution blocks in all four rendition tests with calls to this helper, ensuring each test validates the runtime binary before using it.src/rendition.ts (1)
263-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
argumentsis misleading when a real runtime path is used.When
runtimePathis set the sandbox is invoked directly withruntimePath, yetrequest.argumentsclaims adocker runcommand line. The field only reaches an injectedrunCodec, so the branch is effectively cosmetic and easy to misread as the real execution contract.🤖 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/rendition.ts` around lines 263 - 298, Update the LinuxCodecRequest construction in the codec batch loop so arguments reflects the actual execution path: remove the cosmetic docker command-line branch when runtimePath is provided, and align the request metadata with the direct runtimePath-based sandbox invocation while preserving the injected runCodec behavior.src/materializationService.ts (1)
1094-1108: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCache reuse/unlink is TOCTOU-prone across concurrent workers.
hashFile→unlink→ laterrenameon a sharedchunk-cacheroot races if more than one job or process ever sharesworkRoot. Today the worker is single-flight, but the rename-into-place path is already atomic; consider skipping the pre-emptiveunlinkand letting therenameoverwrite, so a concurrent reader never observes a missing cache entry.🤖 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/materializationService.ts` around lines 1094 - 1108, Remove the pre-emptive unlink operations from the cache reuse logic around hashFile, including the catch-path unlink, and let the existing atomic rename-into-place operation replace stale or invalid entries. Preserve returning destination for matching cached chunks while ensuring concurrent readers are not exposed to a missing cache entry.src/serverArchitecture.test.ts (1)
41-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePython workers are outside the forbidden-string sweep.
linuxCodecWorker.pyandlinuxAttributionWorker.pyare production sources in the same boundary but are never scanned. Extend the glob to cover them if the intent is a repository-wide guarantee.🤖 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/serverArchitecture.test.ts` around lines 41 - 49, Extend the production source collection used by the server architecture test to include linuxCodecWorker.py and linuxAttributionWorker.py, ensuring the existing forbidden-string assertions scan these Python workers along with the current sources. Keep the forbidden list and assertion behavior unchanged.src/materializationServer.test.ts (1)
365-376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-string architecture assertions duplicate
serverArchitecture.test.ts.These forbidden-string scans belong with the other architecture guards rather than inside a behavioral handler test; keeping them in one place avoids drift.
🤖 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/materializationServer.test.ts` around lines 365 - 376, Move the source-string architecture assertions for “/v2/materializations/renditions” and “serviceSecret” out of the handler test and into the existing architecture guard suite in serverArchitecture.test.ts. Keep the legacy request status assertion in the behavioral test, and preserve both forbidden-string checks in their centralized location.src/linuxCodecSandbox.test.ts (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases for the non-JSON fallback.
Only
'not-json'is covered. Empty string, valid JSON with an unrecognizederrorType, and a mismatchedschemaVersionall reach the same fallback path and are worth pinning so a future refactor cannot silently leak private detail through the default branch.🤖 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/linuxCodecSandbox.test.ts` around lines 24 - 27, Add negative test cases alongside the existing classifyLinuxCodecFailure('not-json') assertion in linuxCodecSandbox.test.ts for an empty string, valid JSON with an unrecognized errorType, and valid JSON with a mismatched schemaVersion. Assert that each input returns MATERIALIZATION_CODEC_SANDBOX_FAILED through the non-JSON fallback path.src/materializationServer.ts (2)
129-139: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
stderris piped but never drained.
docker infofailures write diagnostics to stderr; since nothing reads that pipe, a verbose failure can block the child until the 10s kill timer fires. Either drain it alongside stdout or setstderr: 'ignore'.♻️ Drain stderr
const timer = setTimeout(() => child.kill(), 10_000); try { - const [exitCode, stdout] = await Promise.all([ - child.exited, - new Response(child.stdout).text(), - ]); + const [exitCode, stdout] = await Promise.all([ + child.exited, + new Response(child.stdout).text(), + new Response(child.stderr).text(), + ]);🤖 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/materializationServer.ts` around lines 129 - 139, Update the Docker subprocess handling around child.spawn and the Promise.all that awaits child.exited and stdout so the piped child.stderr stream is also drained, preventing verbose failures from blocking. Preserve the existing stdout and exit-code behavior; alternatively configure stderr as ignored if diagnostics are intentionally discarded.
642-648: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff64 MiB attribution bodies are fully buffered and amplified in memory.
Each request holds the streamed chunks, the concatenated
Uint8Array, the decoded UTF-8 string, and oneBufferper decoded asset — roughly 3–4× the body size resident. With no concurrency cap onBun.serve, a handful of simultaneous authenticated requests at the limit can exhaust the materializer's heap. Consider a lower per-asset cap plus an in-flight request semaphore.Also applies to: 660-702
🤖 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/materializationServer.ts` around lines 642 - 648, Reduce the attribution request body limit used by readBoundedRequestJson and add an in-flight request semaphore around the attribution handling flow beginning near derivedKeys and failureCode. Acquire the semaphore before buffering the request, reject or fail requests when capacity is unavailable, and release it on every completion path, including errors, while preserving the existing attribution failure handling.src/keyBroker.test.ts (1)
1-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the
normalizeFilesduplicate/ordering edge case.Once the ordering-check fix in
keyBroker.ts(normalizeFiles) lands, a regression test asserting that two entries differing only by incidental whitespace (e.g."Assets/x.png "followed by"Assets/x.png") are rejected would guard against reintroducing the bug.🤖 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/keyBroker.test.ts` around lines 1 - 97, The key broker tests lack regression coverage for the duplicate/ordering edge case in normalizeFiles. Add a test that supplies protectedFiles with paths differing only by incidental whitespace, such as “Assets/x.png ” followed by “Assets/x.png”, and assert that the derivation flow rejects the input after normalization; keep the test focused on this duplicate detection behavior.src/linuxTreeSandbox.ts (2)
80-87: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResponse is fully buffered before the size limit is enforced.
new Response(child.stdout).text()materializes the entire output in memory beforeBuffer.byteLength(...) > MAX_RESPONSE_BYTESis checked, so the guard doesn't actually bound peak memory usage — it only rejects after the fact.🤖 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/linuxTreeSandbox.ts` around lines 80 - 87, Update the output handling around child.stdout and child.stderr so response size is enforced while streaming, before the complete content is materialized. Preserve the existing MAX_RESPONSE_BYTES rejection behavior and ensure the bounded output remains available for subsequent processing alongside exitCode.
143-179: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNo host-side verification of the built rendition against its self-reported digest.
buildPersonalizedTreeArchivereturnsrenditionPath/renditionSha256purely from the worker's self-report; the host process never re-readsrendition.zipto confirm the digest, unlike file extraction which is verified byte-for-byte inside the worker. If downstream consumers rely solely on this value, a compromised or buggy worker could report a mismatched digest undetected at this layer.🤖 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/linuxTreeSandbox.ts` around lines 143 - 179, Update buildPersonalizedTreeArchive to read the generated rendition at renditionPath and compute its SHA-256 digest on the host before returning. Compare the computed digest with result.renditionSha256 and throw the existing invalid-result error on mismatch; return the worker-reported digest only after verification succeeds.src/linuxCodecSandbox.ts (1)
62-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFailure classification is coupled to exact native error-message text.
classifyLinuxCodecFailuredistinguishesMATERIALIZATION_CODEC_ASSET_UNCARRYABLEfromMATERIALIZATION_CODEC_NATIVE_ENCODE_FAILEDpurely by matching the literal string'native encode failed with status 1'. If the worker/native codec's error message format changes, this silently misclassifies failures (business-relevant "uncarryable" cases could be reported as generic encode failures, or vice versa).Since
linuxCodecWorker.pyisn't in this review batch, please confirm the exact error string/format it emits for the "uncarryable" case remains stable, or consider emitting a structured discriminant field (e.g.,errorReason: 'uncarryable') instead of a status-embedded message.🤖 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/linuxCodecSandbox.ts` around lines 62 - 100, Update classifyLinuxCodecFailure to avoid identifying uncarryable assets solely through the exact parsed.error text. Use a stable structured discriminant emitted by the worker, such as parsed.errorReason === 'uncarryable', to return MATERIALIZATION_CODEC_ASSET_UNCARRYABLE, while retaining native encode classification for other encode failures and the existing fallback behavior.src/attribution.ts (2)
66-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueResult entries are returned unvalidated.
Only the array length and schema version are checked; each element is cast to
AttributionResultwithout verifyingassetPath/assetType/matchedshapes, and callers may treat a malformedmatchedas truthy. Consider per-entry validation before returning.🤖 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/attribution.ts` around lines 66 - 85, Update parseResults to validate every payload.results entry before returning it, confirming assetPath and assetType have the expected shapes and matched is a valid boolean or expected value. Reject any malformed entry with the existing invalid-response error, while preserving the current schema, length, and credentialEnvironment checks.
123-157: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider running the container as a non-root user.
The hardening set is strong (no network, read-only, cap-drop ALL, no-new-privileges, pids/memory/cpu limits), but the process still runs as the image's default UID (root). Adding
--userwith a fixed non-root UID/GID closes the remaining gap cheaply.♻️ Proposed change
'--security-opt', 'no-new-privileges', + '--user', + '65534:65534', '--pids-limit',🤖 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/attribution.ts` around lines 123 - 157, Update the Docker argument list in the Bun.spawn invocation to add --user with a fixed non-root UID/GID before PINNED_PYTHON_IMAGE, ensuring the worker container runs without root privileges while preserving the existing hardening options.src/attribution.test.ts (2)
118-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative attribution case.
Both tests only assert
matched: trueon the correct key. A case with a mismatchedcandidateFileKeysentry (or an unwatermarked PNG) assertingmatched: falsewould guard against a decoder that trivially returns the first candidate.🤖 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/attribution.test.ts` around lines 118 - 151, Add a negative case in the attribution test around attributeArchiveAssets, using a mismatched candidateFileKeys entry or unwatermarked PNG, and assert the returned attribution result has matched: false. Keep the existing positive attribution assertions unchanged and ensure the case verifies that an unrelated candidate is not attributed.
60-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests hard-require Docker and a prebuilt
.so.Both cases silently depend on
yucp_coupling/out/linux-x64/Release/yucp_coupling.soexisting and a working Docker daemon; without them the failure surfaces as an opaque non-zero exit fromdocker run. Consider gating withtest.skipIf(...)on runtime presence, or asserting the artifact exists up front so the failure message is actionable.🤖 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/attribution.test.ts` around lines 60 - 124, Make the server attribution tests around materializePersonalizedZip and attributeArchiveAssets explicitly handle missing prerequisites. Check for the required yucp_coupling.so artifact and usable Docker before running these cases, using test.skipIf(...) or an equivalent upfront assertion with an actionable message; avoid allowing setup failures to surface only as opaque docker run errors.yucp_coupling/guard.c (1)
807-842: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWorst case is 12 full embed+extract passes over the image.
When no profile verifies, this loop runs
WM_IMG_PROFILE_COUNTiterations, each doing a full-imagememcpyplus forward/inverse DCT over every selected block, then another full extraction pass. AtWM_IMG_MAX_PIXELSscale that is a substantial per-asset cost on the materialization path. Consider ordering profiles by expected success and/or bounding the retry count for large images.Note: the Cppcheck "memory leak"/"syntax error" hints on Lines 829/836 are parse artifacts of the multi-line call —
original_pxis released in thedoneblock.🤖 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 `@yucp_coupling/guard.c` around lines 807 - 842, Reduce the worst-case retry cost in the profile verification loop around wm_img_embed_px and wm_img_extract_px by trying profiles in expected-success order and/or limiting retries for large images. Preserve verification correctness by retaining the successful profile path and ensuring the bounded loop still uses the existing original_px restoration and done cleanup.Source: Linters/SAST tools
yucp_coupling/build.sh (1)
62-71: 📐 Maintainability & Code Quality | 🔵 TrivialThe committed SHA-256 record depends on toolchain reproducibility.
The script overwrites
yucp_coupling.sha256on every build with no comparison against the committed value, and nothing pins the compiler or enables deterministic flags. Anyone rebuilding with a differentccwill produce a different hash than the one the runtime validation expects. Consider pinning the build in a container image and adding a verify mode that fails when the produced hash differs from the recorded one.🤖 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 `@yucp_coupling/build.sh` around lines 62 - 71, The build script’s SHA-256 output is not reproducibility-checked and can overwrite the committed record with compiler-dependent results. Update the build flow around HASH and SHA_FILE to support a verification mode that compares the produced hash with the existing recorded value and fails on mismatch, while preserving normal build output; also make the compilation deterministic by using pinned toolchain/container settings or deterministic compiler flags already appropriate to this script.
🤖 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 `@deploy/initialize-local-master-credential.sh`:
- Around line 16-26: Update the existing-target branch in
initialize-local-master-credential.sh to inspect the credential JSON for the
requested epoch before exiting successfully. Preserve the invalid path and
permission checks, but when ${epoch} is absent, generate the new credential and
merge it with all existing JSON keys, replacing the current single-entry output
path; only exit 0 when the requested epoch is already present.
In `@src/attribution.ts`:
- Around line 158-176: Update the sandboxed worker flow around child.exited and
the stdout/stderr Response reads to enforce a timeout and bounded output. Add a
deadline that kills the child when it expires, ensure the caller receives a
timeout failure, and read/truncate worker output to the configured maximum
before buffering or parsing; preserve existing exit-code errors and parseResults
behavior for successful, bounded responses.
In `@src/keyBroker.ts`:
- Around line 123-167: Update normalizeFiles to track the previously normalized
path as each file is processed, and compare that trimmed value with the current
normalizedPath in the ordering/duplicate check. Replace the raw files[index -
1]?.normalizedPath lookup while preserving the existing compareUtf8 >= 0
rejection and path validation behavior.
In `@src/linuxCodecSandbox.ts`:
- Around line 241-267: Update the child.stdin handling in the codec execution
flow so the asynchronous stdin end/drain operation is awaited before
frame.fill(0) runs. Preserve the existing finally-based cleanup and ensure the
frame is cleared only after all input bytes have been consumed.
In `@src/linuxCodecWorker.py`:
- Around line 318-323: Update the batch-processing flow around process_file in
the top-level handler so every key in keys is zeroized even when processing an
entry raises. Ensure cleanup runs for unprocessed keys before the exception is
serialized and the handler exits, while preserving the existing sorted results
behavior on success.
- Around line 203-216: Update the nested ZIP extraction flow around
source_archive.read(info) to use a bounded read that cannot materialize more
than the permitted per-entry or remaining total byte limits, rather than
trusting info.file_size. Detect oversized decompressed output during reading,
and update total_bytes using len(data) after the bounded read; retain the
existing invalid-length validation and archive-writing behavior for entries
within both caps.
In `@src/linuxTreeSandbox.ts`:
- Around line 64-94: Update runWorker to enforce a wall-clock timeout while
awaiting the spawned Docker worker, including termination of the child process
when the timeout expires. Ensure the timeout path rejects with a clear error and
does not leave stdout, stderr, or the materialization request hanging; preserve
the existing response-size and nonzero-exit handling for workers that finish in
time.
- Around line 24-61: Update the Docker argument list returned by the
container-command builder to run as a non-root user by adding the appropriate
--user setting, and add --memory-swap 512m alongside the existing --memory 512m
limit. Preserve the existing sandbox mounts, capabilities, and other resource
restrictions.
In `@src/materializationServer.ts`:
- Around line 951-952: Update the materialization server host configuration to
default MATERIALIZATION_HEALTH_HOST to 127.0.0.1 instead of 0.0.0.0, while
preserving the existing readTrimmedEnv override so broader binding requires
explicit configuration.
- Around line 318-340: Update postJson to pass an AbortSignal to
fetchImplementation, combining the worker shutdown signal with a bounded
per-request timeout so hung control-plane calls terminate. Ensure timeout or
cancellation errors propagate through runOnce while preserving existing response
validation and JSON handling.
- Around line 471-481: Update the polling wait Promise around the abort listener
to retain the listener callback and explicitly remove it when the timeout
completes, while preserving cleanup on abort. Ensure both timer and abort paths
clear the timeout as appropriate, resolve only once, and detach the listener
from signal so repeated polling does not accumulate registrations.
In `@src/materializationService.ts`:
- Around line 395-414: Update every fetchFn request in the materialization flow,
including capability consume, manifest retrieval, per-chunk download, rendition
upload, and completion, to pass an AbortSignal.timeout value. Use the standard
request timeout for control-plane requests and a larger timeout budget for
chunk/download and upload streams, ensuring no request can hang the worker loop
indefinitely.
- Around line 1031-1073: Update loadSourceManifest to retain the complete
manifest file list alongside the protected-file validation, rather than
returning only protectedFiles. Change assembleSourceTree to iterate that full
file-list property so common and protected entries are both downloaded and
written, while preserving protected-file capability checks and existing manifest
metadata.
In `@src/rendition.ts`:
- Around line 342-354: The attribution-record construction in completeRendition
must fail when the protected-file lookup is missing instead of defaulting
materializerType to 'png' or sourceSha256 to ''. Reuse the suggested
protected-file map to perform one lookup per output, validate that the matching
source exists, and use its assetType and sourceSha256 for the record.
- Around line 306-318: Validate each codec result’s normalizedPath against the
requested protectedFiles set before using it. In the loop over
codecResults.flatMap(result => result.files), resolve or look up the result by
the validated requested path and reject or skip any path not present in that
set, including traversal segments, before path.join and readFile; use the
validated path for output access.
---
Minor comments:
In `@src/linuxAttributionWorker.py`:
- Around line 33-49: Update read_request so key bytearrays accumulated during
the candidates read are zeroized if read_exact fails partway through the
comprehension. Replace the direct comprehension with cleanup-safe accumulation,
wipe all already-read keys in the exception path, then re-raise the original
failure; preserve the existing successful return behavior and trailing-byte
validation.
In `@src/linuxCodecSandbox.ts`:
- Around line 188-223: Update the Docker arguments in the sandbox command around
PINNED_PYTHON_IMAGE to run as a non-root user and add a memory-swap limit
matching the existing 256m memory limit. Extract the shared isolation arguments
into a reusable helper and apply it in both linuxCodecSandbox and
linuxTreeSandbox so their security settings remain aligned.
In `@src/linuxTreeSandbox.ts`:
- Around line 78-79: Update the request-writing flow around child.stdin.write
and child.stdin.end so stdin closure is awaited before awaiting child.exited.
Ensure the final serialized request is fully flushed before worker completion is
observed, preserving the existing request and exit handling.
In `@src/materializationServer.test.ts`:
- Around line 127-156: Update the boundary test around
createMaterializationWorker so its name and capability length accurately reflect
the 1 MiB value currently exercised, or add a separate 2 MiB case that verifies
the intended maximum behavior while accounting for readJsonResponse’s
response-size limit before requireBase64Url validation.
In `@src/materializationServer.ts`:
- Around line 618-628: Update the request handling around readBoundedRequestJson
and input.keyBroker.prepareSubject to validate buyerId, creatorId, jobId, and
productId as actual non-empty strings before calling the broker. Remove String
coercion, reject arrays, objects, and other non-string values at the HTTP
boundary using the existing request error response pattern, while preserving
numeric keyEpoch validation and the successful prepareSubject flow.
In `@src/materializationService.ts`:
- Around line 1462-1473: Normalize and validate input.materializerId once at the
start of materializeCapabilityJob, then reuse that normalized value throughout
the job. Replace the raw input.materializerId used by the upload-ticket and
completion endpoint flows, as well as the consumeCapability call, while
preserving the existing materializerId validation behavior.
In `@yucp_coupling/build.sh`:
- Around line 41-60: Update the build command using LINK_FLAGS and SODIUM_CFLAGS
so path-bearing version-script and include arguments remain separate quoted
words when $HERE or $SODIUM_PREFIX contains spaces. Preserve the existing linker
flags and vendored static archive usage, and avoid relying on word splitting of
those variables.
In `@yucp_coupling/guard.c`:
- Around line 1997-1998: The placement-epoch loops in xg_0124 and xg_0125 are
hardcoded to a single iteration; either replace the bound with the intended
shared epoch count on both encoder and decoder paths, or remove the loops and
pass 0u directly. Keep both sides synchronized so published assets remain
recoverable.
- Around line 1710-1724: Replace the valid_slots allocation in the slot-capacity
handling with calloc so the multiplication by sizeof(size_t) is
overflow-checked, while preserving the existing capacity validation, cleanup,
and failure return behavior.
---
Nitpick comments:
In `@src/attribution.test.ts`:
- Around line 118-151: Add a negative case in the attribution test around
attributeArchiveAssets, using a mismatched candidateFileKeys entry or
unwatermarked PNG, and assert the returned attribution result has matched:
false. Keep the existing positive attribution assertions unchanged and ensure
the case verifies that an unrelated candidate is not attributed.
- Around line 60-124: Make the server attribution tests around
materializePersonalizedZip and attributeArchiveAssets explicitly handle missing
prerequisites. Check for the required yucp_coupling.so artifact and usable
Docker before running these cases, using test.skipIf(...) or an equivalent
upfront assertion with an actionable message; avoid allowing setup failures to
surface only as opaque docker run errors.
In `@src/attribution.ts`:
- Around line 66-85: Update parseResults to validate every payload.results entry
before returning it, confirming assetPath and assetType have the expected shapes
and matched is a valid boolean or expected value. Reject any malformed entry
with the existing invalid-response error, while preserving the current schema,
length, and credentialEnvironment checks.
- Around line 123-157: Update the Docker argument list in the Bun.spawn
invocation to add --user with a fixed non-root UID/GID before
PINNED_PYTHON_IMAGE, ensuring the worker container runs without root privileges
while preserving the existing hardening options.
In `@src/keyBroker.test.ts`:
- Around line 1-97: The key broker tests lack regression coverage for the
duplicate/ordering edge case in normalizeFiles. Add a test that supplies
protectedFiles with paths differing only by incidental whitespace, such as
“Assets/x.png ” followed by “Assets/x.png”, and assert that the derivation flow
rejects the input after normalization; keep the test focused on this duplicate
detection behavior.
In `@src/linuxCodecSandbox.test.ts`:
- Around line 24-27: Add negative test cases alongside the existing
classifyLinuxCodecFailure('not-json') assertion in linuxCodecSandbox.test.ts for
an empty string, valid JSON with an unrecognized errorType, and valid JSON with
a mismatched schemaVersion. Assert that each input returns
MATERIALIZATION_CODEC_SANDBOX_FAILED through the non-JSON fallback path.
In `@src/linuxCodecSandbox.ts`:
- Around line 62-100: Update classifyLinuxCodecFailure to avoid identifying
uncarryable assets solely through the exact parsed.error text. Use a stable
structured discriminant emitted by the worker, such as parsed.errorReason ===
'uncarryable', to return MATERIALIZATION_CODEC_ASSET_UNCARRYABLE, while
retaining native encode classification for other encode failures and the
existing fallback behavior.
In `@src/linuxRendition.realtest.ts`:
- Around line 112-123: Create a module-level helper in
linuxRendition.realtest.ts that resolves the runtimePath, reads and normalizes
the expected digest, verifies the binary hash, and returns the verified runtime
path. Replace the repeated runtimePath and digest-resolution blocks in all four
rendition tests with calls to this helper, ensuring each test validates the
runtime binary before using it.
In `@src/linuxTreeSandbox.ts`:
- Around line 80-87: Update the output handling around child.stdout and
child.stderr so response size is enforced while streaming, before the complete
content is materialized. Preserve the existing MAX_RESPONSE_BYTES rejection
behavior and ensure the bounded output remains available for subsequent
processing alongside exitCode.
- Around line 143-179: Update buildPersonalizedTreeArchive to read the generated
rendition at renditionPath and compute its SHA-256 digest on the host before
returning. Compare the computed digest with result.renditionSha256 and throw the
existing invalid-result error on mismatch; return the worker-reported digest
only after verification succeeds.
In `@src/materializationServer.test.ts`:
- Around line 365-376: Move the source-string architecture assertions for
“/v2/materializations/renditions” and “serviceSecret” out of the handler test
and into the existing architecture guard suite in serverArchitecture.test.ts.
Keep the legacy request status assertion in the behavioral test, and preserve
both forbidden-string checks in their centralized location.
In `@src/materializationServer.ts`:
- Around line 129-139: Update the Docker subprocess handling around child.spawn
and the Promise.all that awaits child.exited and stdout so the piped
child.stderr stream is also drained, preventing verbose failures from blocking.
Preserve the existing stdout and exit-code behavior; alternatively configure
stderr as ignored if diagnostics are intentionally discarded.
- Around line 642-648: Reduce the attribution request body limit used by
readBoundedRequestJson and add an in-flight request semaphore around the
attribution handling flow beginning near derivedKeys and failureCode. Acquire
the semaphore before buffering the request, reject or fail requests when
capacity is unavailable, and release it on every completion path, including
errors, while preserving the existing attribution failure handling.
In `@src/materializationService.test.ts`:
- Around line 322-357: The request handler in the test’s URL-matching mock
should only return the capability-consume payload for the expected consume
endpoint. Add an explicit assertion for that URL or branch, and return a 404
response for all unmatched URLs so incorrect requests cannot be accepted.
- Around line 178-215: Extend the sourceManifest fixture with a file classified
as 'common', including its chunk metadata and content, then update the source
assembly assertions to verify that file appears in the assembled tree. Keep the
existing protected-file coverage intact while exercising the common-content path
used by loadSourceManifest.
In `@src/materializationService.ts`:
- Around line 1094-1108: Remove the pre-emptive unlink operations from the cache
reuse logic around hashFile, including the catch-path unlink, and let the
existing atomic rename-into-place operation replace stale or invalid entries.
Preserve returning destination for matching cached chunks while ensuring
concurrent readers are not exposed to a missing cache entry.
In `@src/rendition.test.ts`:
- Around line 16-59: Extract the duplicate crc32, pngChunk, and makeGrayPng
helpers from rendition.test.ts into a shared test fixture module, then import
and reuse them in both rendition.test.ts and linuxRendition.realtest.ts.
Preserve their current behavior and remove the duplicate local definitions so
the suites stay synchronized.
In `@src/rendition.ts`:
- Around line 263-298: Update the LinuxCodecRequest construction in the codec
batch loop so arguments reflects the actual execution path: remove the cosmetic
docker command-line branch when runtimePath is provided, and align the request
metadata with the direct runtimePath-based sandbox invocation while preserving
the injected runCodec behavior.
In `@src/serverArchitecture.test.ts`:
- Around line 41-49: Extend the production source collection used by the server
architecture test to include linuxCodecWorker.py and linuxAttributionWorker.py,
ensuring the existing forbidden-string assertions scan these Python workers
along with the current sources. Keep the forbidden list and assertion behavior
unchanged.
In `@yucp_coupling/build.sh`:
- Around line 62-71: The build script’s SHA-256 output is not
reproducibility-checked and can overwrite the committed record with
compiler-dependent results. Update the build flow around HASH and SHA_FILE to
support a verification mode that compares the produced hash with the existing
recorded value and fails on mismatch, while preserving normal build output; also
make the compilation deterministic by using pinned toolchain/container settings
or deterministic compiler flags already appropriate to this script.
In `@yucp_coupling/guard.c`:
- Around line 807-842: Reduce the worst-case retry cost in the profile
verification loop around wm_img_embed_px and wm_img_extract_px by trying
profiles in expected-success order and/or limiting retries for large images.
Preserve verification correctness by retaining the successful profile path and
ensuring the bounded loop still uses the existing original_px restoration and
done cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
yucp_coupling/guard.c (2)
1874-1897: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win40-iteration bisection with
pow()over every vertex, repeated per embed attempt.The inner loop evaluates
pow(uu, gamma)for every vertex in the bin on each of 40 bisection steps, so a single embed costs ~40·Npow()calls.xg_0124then runs up to 3 attempts × 2 carrier ranks of embed plus a verification decode each time, and the codec sandbox is--cpus 1. For a multi-million-vertex FBX this dominates materialization latency.Cheap mitigations: exit the bisection early once
fabs(gm - target)is within tolerance (typically ~10 iterations rather than 40), and hoist the per-binuvalues into the array once instead of recomputing(pr[r].k - blo) / spanon every iteration.🤖 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 `@yucp_coupling/guard.c` around lines 1874 - 1897, Optimize the exponent bisection in the embedding loop by precomputing each bin’s clamped normalized u values before iterating on gamma, then reuse them for every mean calculation instead of recomputing from pr[r].k. Add an early exit when fabs(gm - target) reaches an appropriate tolerance, while preserving the existing target selection and bisection behavior for bins that do not converge early.
325-338: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep a v2 decode path for mesh watermarks
yucp_coupling/guard.c:325-338The image profile reshuffle is compatible (
WM_IMG_PROFILES[0]matches the old fixed channel/coeff/qstep setup andWM_EPOCHstays0), butyucp.wm.mesh.embed.v3changes the HKDF domain and makes existing mesh marks undecodable. Add a v2 fallback in the decoder if old FBX watermarks must survive upgrades.🤖 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 `@yucp_coupling/guard.c` around lines 325 - 338, Preserve backward compatibility for existing mesh watermarks by adding a decoder fallback that derives the mesh seed with the legacy “yucp.wm.mesh.embed.v2” HKDF domain when the current v3 decode path fails. Keep the existing v3 path as the primary behavior and limit the fallback to mesh decoding; update the relevant decoder flow rather than changing wm_basis_seed’s current profile behavior.
♻️ Duplicate comments (3)
src/linuxTreeSandbox.ts (2)
64-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo timeout on the spawned Docker worker.
Unresolved from a prior review:
runWorkerawaitschild.exitedwith no bound, so a stalleddocker runhangs the materialization request indefinitely.Bun.spawn's built-intimeout/killSignaloptions can address this directly.⏱️ Proposed fix
const child = Bun.spawn(treeWorkerArguments(input), { stderr: 'pipe', stdin: 'pipe', stdout: 'pipe', + timeout: 60_000, + killSignal: 'SIGKILL', });🤖 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/linuxTreeSandbox.ts` around lines 64 - 94, Update runWorker to configure a finite Bun.spawn timeout and appropriate killSignal for the Docker worker, using the existing worker lifecycle options so stalled docker runs are terminated and child.exited cannot hang indefinitely.
17-62: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRun this container as non-root and cap swap.
Unresolved from a prior review: no
--useris set (default Python image runs as root), and--memory-swapis omitted, letting Docker grant swap up to 2× the 512m--memorylimit.🔒 Proposed fix
'--memory', '512m', + '--memory-swap', + '512m', + '--user', + '65534:65534', '--cpus', '1',🤖 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/linuxTreeSandbox.ts` around lines 17 - 62, Update treeWorkerArguments to run the container as a non-root user by adding the appropriate Docker --user setting, and cap total memory including swap by setting --memory-swap to the intended limit alongside the existing --memory 512m option. Preserve the current container mounts, capabilities, and resource limits.src/linuxCodecSandbox.ts (1)
241-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAwait
child.stdin.end()before clearingframe.Same issue flagged previously for this file:
FileSink.end()/write()can be asynchronous, so zeroingframeinfinallyimmediately afterwrite/endrisks corrupting in-flight codec input.🔒 Proposed fix
try { child.stdin.write(frame); - child.stdin.end(); + await child.stdin.end(); } finally { frame.fill(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 `@src/linuxCodecSandbox.ts` around lines 241 - 251, Update the child stdin write flow around Bun.spawn so it awaits child.stdin.end() before clearing frame. Keep frame.fill(0) in the finally block, but ensure both the write and stream finalization complete before releasing the frame contents.
🧹 Nitpick comments (17)
src/rendition.test.ts (1)
258-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
rejects.not.toThrow('private codec details')is a near-tautological assertion.
RenditionStageFailurealways has the fixed message'Rendition failed at a classified execution stage', so this passes regardless of leakage. Assert the message (and thatcauseis not serialized) explicitly instead.♻️ Proposed tightening
await expect(failure).rejects.toMatchObject({ errorCode: 'MATERIALIZATION_CODEC_BATCH_FAILED', + message: 'Rendition failed at a classified execution stage', }); - await expect(failure).rejects.not.toThrow('private codec details');🤖 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/rendition.test.ts` around lines 258 - 261, Update the RenditionStageFailure rejection assertions in the relevant test to verify the exact public error message explicitly and confirm that the serialized error does not include a cause field or private codec details. Replace the tautological rejects.not.toThrow assertion while preserving the existing MATERIALIZATION_CODEC_BATCH_FAILED errorCode check.src/materializationService.test.ts (1)
178-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture has no
classification: 'common'file, so the source-assembly path is untested.Adding one common entry (with its own chunk and
logicalFiles: 2) would have caught the dropped-common-files defect inassembleSourceTree, and it's the only regression guard for that path.Also applies to: 363-375
🤖 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/materializationService.test.ts` around lines 178 - 215, The source manifest fixture in the materialization test only covers protected files, leaving the common-file branch of assembleSourceTree untested. Extend the manifest’s files array with a separate classification: 'common' entry, including its own chunk metadata and logicalFiles: 2, and provide the corresponding source object data needed by the fixture while preserving the existing protected-file setup.src/materializationServer.ts (4)
129-139: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
stderr: 'pipe'is never consumed.The stderr pipe is opened but never read, so the child can block on a full stderr buffer (bounded only by the 10s kill) and the stream is left undrained. Since the error output is unused,
'ignore'is simpler.♻️ Drop the unused pipe
const child = Bun.spawn(['docker', 'info', '--format', format], { - stderr: 'pipe', + stderr: 'ignore', stdin: 'ignore', stdout: 'pipe', });🤖 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/materializationServer.ts` around lines 129 - 139, Update the Bun.spawn options in the Docker info execution flow to set stderr to 'ignore' instead of opening an unconsumed pipe. Keep the existing stdout capture, timeout, and child.exited handling unchanged.
221-233: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResponse size is only enforced after full buffering.
When the control plane replies with chunked encoding (no
content-length),response.text()buffers the whole body before theMAXIMUM_RESPONSE_BYTEScheck, so the limit cannot prevent memory exhaustion.readBoundedRequestJsonalready does this correctly with an incremental reader — consider reusing that streaming approach here.🤖 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/materializationServer.ts` around lines 221 - 233, Update readJsonResponse to enforce MAXIMUM_RESPONSE_BYTES while reading the response stream incrementally, rather than calling response.text() before validation. Reuse the bounded streaming approach from readBoundedRequestJson, preserving the existing content-length validation and oversized-response error behavior.
703-811: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUp to 4096 synchronous key derivations per attribution request block the event loop.
deriveFileKeysis called once per candidate inside a synchronousmap, so a maximum-size request performs 4096 derivations with no yielding, stalling health checks and other routes on the same server. Consider lowering the candidate ceiling or batching derivations with periodic yields.🤖 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/materializationServer.ts` around lines 703 - 811, Update the attribution candidate processing around the synchronous candidates.map and deriveFileKeys call to prevent up to 4096 derivations from blocking the event loop. Preserve validation and derived-key behavior while either reducing the accepted candidate limit or converting processing to an asynchronous loop that yields periodically between bounded batches.
1020-1028: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAsync signal handler rejection is unobserved.
shutdownreturns a promise thatprocess.oncediscards; ifserver.stop()rejects it surfaces as an unhandled rejection instead of a logged shutdown failure.♻️ Catch and log
const shutdown = async () => { abortController.abort(); - await server.stop(); + try { + await server.stop(); + } catch (error) { + console.error( + JSON.stringify({ + event: 'materialization.shutdown_failed', + materializerId: config.materializerId, + }) + ); + } };🤖 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/materializationServer.ts` around lines 1020 - 1028, Update the shutdown handler around shutdown and the SIGINT/SIGTERM process.once registrations so rejections from server.stop() are caught and logged as shutdown failures instead of becoming unhandled promise rejections. Preserve the existing abort and server-stop ordering, and use the surrounding module’s established logging mechanism.src/serverArchitecture.test.ts (1)
25-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGlob
*.tsis non-recursive, so nested sources escape the forbidden-token scan.Any file under
src/<subdir>/is silently excluded, making this guardrail pass for code it should reject. Use**/*.ts.🔧 Cover the whole tree
- ...new Bun.Glob('*.ts').scanSync({ + ...new Bun.Glob('**/*.ts').scanSync({🤖 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/serverArchitecture.test.ts` around lines 25 - 39, Update the Bun.Glob pattern in the productionSources collection to recursively match TypeScript files throughout src and its subdirectories, while preserving the existing test-file filters and file-reading logic.src/materializationServer.test.ts (2)
44-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing negative cases for the OS/architecture branch.
validateDockerIsolationInforejects non-linux and non-x64 hosts, but no test covers that branch. Add cases foroperatingSystem: 'windows'andarchitecture: 'aarch64'.🤖 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/materializationServer.test.ts` around lines 44 - 69, The validateDockerIsolationInfo tests cover rootless and cgroup failures but omit invalid platform values. Add negative cases asserting that operatingSystem 'windows' and architecture 'aarch64' each throw the expected validation error, while preserving the existing valid Linux x86_64 case.
363-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion checks zeroization, not derivation.
The handler's
finallyblock wipesderivedKeysbefore this runs, sotoEqual(new Uint8Array(32))only proves the buffer was zeroed. If the intent is to verify the derived key material, capture a copy insideattributeAssets; otherwise a comment clarifying the intent avoids future confusion.🤖 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/materializationServer.test.ts` around lines 363 - 364, Update the test around attributeAssets to distinguish derivation from zeroization: capture a copy of the derived key material inside attributeAssets before the handler’s finally block wipes derivedKeys, then assert the captured copy for derivation and retain the existing derivedKeys assertion only as the zeroization check.src/linuxTreeWorker.py (1)
211-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailures surface only as a bare exception class name.
Every distinct validation failure in this worker raises
ValueError, so the caller seesValueErrorwith no discriminator — unlike the codec and attribution workers, which emit structured JSON. Consider emitting the same{"errorType", "error", "schemaVersion"}shape; the messages here are already static strings with no paths or secrets.🤖 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/linuxTreeWorker.py` around lines 211 - 216, Update the __main__ exception handler in linuxTreeWorker.py to emit structured JSON matching the codec and attribution workers, including errorType, error, and schemaVersion fields. Use the caught exception’s class name and message, serialize the payload to stderr, and preserve the existing nonzero exit behavior.yucp_coupling/build.sh (1)
41-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: validate
yucp_coupling.exports.mapalongside the libsodium precondition.The vendored libsodium archive gets an explicit, well-messaged existence check, but the version script referenced in
LINK_FLAGSdoes not — a missing map surfaces as a raw linker error. Also consider-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2to avoid a redefinition warning on toolchains that predefine it (Ubuntu/Debian GCC do at-O2).♻️ Proposed change
+if [ ! -f "$HERE/yucp_coupling.exports.map" ]; then + echo "Missing the symbol version script" >&2 + exit 1 +fi if [ ! -f "$SODIUM_PREFIX/lib/libsodium.a" ] || [ ! -d "$SODIUM_PREFIX/include" ]; then🤖 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 `@yucp_coupling/build.sh` around lines 41 - 55, Validate the version-script file referenced by LINK_FLAGS alongside the existing SODIUM_PREFIX checks in the build precondition, emitting a clear error and exiting before linking when it is missing. Also update the CFLAGS assignment to undefine any pre-existing FORTIFY_SOURCE definition before setting it to 2, avoiding redefinition warnings.src/attribution.test.ts (2)
222-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo negative-path coverage.
Both tests only assert successful attribution. The security-relevant property of this module is that it does not attribute — an unrelated asset, a wrong file key, or a candidate whose
attributionTokenHashdoesn't match should all yieldmatched: false. Worth adding at least a wrong-key case, since that exercises the hash re-verification atlinuxAttributionWorker.pyLine 162.🤖 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/attribution.test.ts` around lines 222 - 237, The attribution tests only cover successful matches and need negative-path coverage. Extend the tests around attributeArchiveAssets to include at least a wrong candidateFileKeys/file key case, asserting the result is not attributed with matched: false; also cover an unrelated asset or mismatched attributionTokenHash if supported by the existing fixtures, exercising the hash re-verification behavior.
61-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this Docker/native-runtime integration test out of the default suite.
This test needs Docker and
yucp_coupling/out/linux-x64/Release/yucp_coupling.so, but it lives insrc/attribution.test.tsand will run underbun test. The repo already uses.realtest.tsfor this kind of integration coverage, so rename it or add a runtime skip guard.🤖 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/attribution.test.ts` around lines 61 - 72, Move the Docker/native-runtime test identified by “identifies a personalized output through its v2 derivation record” out of the default Bun test suite by placing it in a .realtest.ts file, or add an equivalent runtime skip guard when Docker or yucp_coupling.so is unavailable; preserve the test’s existing assertions and behavior when the required runtime is present.yucp_coupling/guard.c (3)
1997-2001: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe placement-epoch loops iterate exactly once.
placement_epoch < 1uhere and at Line 2144 inxg_0125means only epoch 0 is ever tried — the multi-epoch search the surrounding structure implies doesn't happen. It's consistent between encoder and decoder today, so nothing is broken, but it's a trap: bumping one bound without the other makes previously-embedded meshes silently undecodable.Either collapse to a plain epoch-0 call, or introduce a shared
WM_MESH_PLACEMENT_EPOCHSconstant used by both sites so they can't drift.♻️ Shared bound
+#define WM_MESH_PLACEMENT_EPOCHS 1u ... - for (uint8_t placement_epoch = 0u; placement_epoch < 1u && !verified; + for (uint8_t placement_epoch = 0u; + placement_epoch < WM_MESH_PLACEMENT_EPOCHS && !verified; placement_epoch++) {🤖 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 `@yucp_coupling/guard.c` around lines 1997 - 2001, Replace the duplicated placement-epoch bound in both the encoder loop and decoder function xg_0125 with a shared WM_MESH_PLACEMENT_EPOCHS constant, preserving the current single-epoch behavior by setting it to 1u. Use that constant wherever placement_epoch is bounded so the two paths cannot drift.
1710-1724: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
callocfor the slot array so the× sizeof(size_t)product is overflow-checked.Line 1710 guards
vl.n > SIZE_MAX / 3, which protectsslot_capacityitself, butslot_capacity * sizeof(size_t)multiplies by another 8 and is unguarded. Not reachable given FBX size limits, butcallocmakes it free.Separately, worth recording as a comment: the carrier's correctness depends on
wm_mesh_scalar_set_bitnever changing a coordinate's finiteness (it only touches mantissa bit 12/41, never the exponent), sovalid_countis identical on the encode and decode sides. That invariant is load-bearing for blind recovery and currently undocumented.♻️ Proposed change
- size_t* valid_slots = (size_t*)malloc(slot_capacity * sizeof(size_t)); + size_t* valid_slots = (size_t*)calloc(slot_capacity, sizeof(size_t));🤖 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 `@yucp_coupling/guard.c` around lines 1710 - 1724, Replace the valid_slots allocation in the surrounding guard function with calloc so the element-count and sizeof(size_t) multiplication are overflow-checked, while preserving the existing failure cleanup and return behavior. Also add a concise comment documenting that wm_mesh_scalar_set_bit only changes mantissa bits, preserves coordinate finiteness, and therefore keeps valid_count identical during encoding and decoding.Source: Linters/SAST tools
1573-1600: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGroup detection silently assumes same-property vertices are contiguous in
vl.Both passes advance only while
vl->d[end].p == property, so ifwm_collect_vertsever emits a property's vertices in more than one run, that property yields multiple groups, the size ranking shifts, and encode/decode can select different carriers — an unrecoverable watermark rather than a loud failure. Nothing in the code enforces the invariant.The comparator itself is fine:
orderis unique so it's a strict total order, making theqsortresult deterministic despite qsort not being stable.Consider asserting the invariant (or grouping via a property→index map) so a future change to
wm_collect_vertscan't break attribution silently.🤖 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 `@yucp_coupling/guard.c` around lines 1573 - 1600, Update wm_vlist_keep_mesh_rank to validate that each property appears in only one contiguous run across vl->d before ranking groups, and fail loudly if a property reappears after another property. Apply the validation consistently with the existing grouping passes, preserving the current comparator and deterministic ordering; alternatively, replace contiguous-run grouping with property-based aggregation if that is the established design.src/attribution.ts (1)
123-157: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winContainer hardening is strong but the worker still runs as the image's default user, and mount specs are built by string interpolation.
Two smaller gaps in an otherwise well-locked-down invocation:
- No
--user, so the Python worker runs as root inside the container. With--cap-drop ALL,--read-only, andno-new-privilegesthe blast radius is small, but--user 65534:65534costs nothing here since every mount is read-only and scratch is a tmpfs.type=bind,source=${input.runtimePath},...is caller-supplied and unescaped. There's no shell (argv array), but a,or=in the path silently corrupts the mount option list. Prefer validatingruntimePath(absolute, no,) before use.🔒️ Proposed change
'--rm', '--interactive', + '--user', + '65534:65534', '--network', 'none',🤖 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/attribution.ts` around lines 123 - 157, Harden the Bun.spawn Docker invocation by adding the fixed non-root user 65534:65534 before the image argument. Validate input.runtimePath before constructing its bind-mount specification, requiring an absolute path and rejecting paths containing commas or other characters that would corrupt Docker mount options; preserve the existing read-only mount 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 @.env.example:
- Line 10: Update the MATERIALIZATION_RUNTIME_LIBRARY_PATH example to use the
current ca-coupling checkout layout and point to the built
yucp_coupling/out/linux-x64/Release/yucp_coupling.so artifact instead of the
obsolete /opt/yucp/coupling path.
In `@deploy/run-local-wsl-materializer.sh`:
- Around line 26-34: Export MATERIALIZATION_WORK_ROOT using the resolved
work_root value before invoking the local materializer server, ensuring the
server uses the same directory that was created and locked. Update the launch
flow around the work_root initialization and flock setup without changing the
existing lock behavior.
In `@src/attribution.ts`:
- Around line 70-84: Validate each element of payload.results before returning
from the response parser: require matched to be boolean, verify assetPath
matches the normalized requested asset path from attributeArchiveAssets, and
ensure attributionId is one of that asset’s submitted candidates. Reject the
response with the existing invalid-response error when any element fails
validation, while preserving the existing schema, length, and
credentialEnvironment checks.
In `@src/linuxAttributionWorker.py`:
- Around line 135-172: Update read_request to reject requests whose assets and
candidates counts exceed a defined maximum product before entering the nested
processing loops. Preserve existing validation and error behavior, and ensure
the guard prevents the assets × candidates native-decoding work from starting
for oversized requests.
In `@src/linuxCodecSandbox.ts`:
- Around line 188-223: Harden the Docker invocation in the Linux codec sandbox
by adding a non-root `--user` setting and explicitly limiting `--memory-swap` to
the same 256m value as `--memory`. Update the `arguments` list used by the
sandbox container while preserving the existing resource and security options.
- Around line 241-245: Add a finite wall-clock timeout and appropriate
killSignal to the Bun.spawn options in the Docker launch flow around child,
ensuring stalled docker run processes are terminated so child.exited cannot
remain pending indefinitely.
- Around line 17-23: Update runLinuxCodecSandbox() to pass the request’s
environment to Bun.spawn as an explicit minimal env, preserving environment
isolation while including at least PATH. Use the environment configured by
createLinuxCodecRequest() rather than allowing the docker process to inherit the
server environment.
In `@src/linuxCodecWorker.py`:
- Around line 297-303: Update the output handling near the return payload to
hash the file incrementally instead of reading it entirely into output_bytes.
Reuse the streaming digest pattern from linuxTreeWorker.py, accumulating the
byte count and SHA-256 digest while reading bounded chunks, then populate
outputBytes and outputSha256 from those streaming results while preserving the
existing attributionTokenHex and normalizedPath values.
- Around line 220-241: The entry staging flow around entry_input, entry_output,
and couple_path can exceed the codec sandbox’s 16 MiB /tmp capacity for accepted
100 MiB entries. Remove the redundant entry_input write/copy and write data
directly to entry_output, or configure the scratch directory on storage with
sufficient capacity, while preserving couple_path decoding and coupled_entries
handling.
In `@src/materializationServer.test.ts`:
- Around line 127-156: Update the capability fixture in the test "accepts the
largest capability permitted by the protected-file contract" to use the actual
MAXIMUM_CAPABILITY_BYTES limit of 2 MiB, so the test exercises the advertised
boundary while preserving its existing assertions and setup.
In `@src/rendition.ts`:
- Line 251: Enforce the computed required flag in the materialization handling
around the file entries at this location and the corresponding 306–309 flow.
When an asset is marked required and its materialization result is uncarryable,
throw MATERIALIZATION_CODEC_ASSET_UNCARRYABLE instead of skipping it; preserve
the existing skip behavior for optional assets.
In `@yucp_coupling/guard.c`:
- Around line 2010-2033: Update the retry loop around wm_mesh_run so every
attempt starts from pristine mesh data rather than the vertices modified by a
prior attempt. Re-parse or restore br from b0 at the beginning of each attempt
while preserving the existing carrier_index and verification flow.
- Around line 478-491: Update WM_IMG_PROFILES to prevent channel 3 profiles from
being selected for RGBA data with constant alpha. Either remove the channel-3
entries entirely or gate their use on detecting genuinely non-constant source
alpha before encoding; preserve the existing colour-channel profile selection
and self-verification behavior.
---
Outside diff comments:
In `@yucp_coupling/guard.c`:
- Around line 1874-1897: Optimize the exponent bisection in the embedding loop
by precomputing each bin’s clamped normalized u values before iterating on
gamma, then reuse them for every mean calculation instead of recomputing from
pr[r].k. Add an early exit when fabs(gm - target) reaches an appropriate
tolerance, while preserving the existing target selection and bisection behavior
for bins that do not converge early.
- Around line 325-338: Preserve backward compatibility for existing mesh
watermarks by adding a decoder fallback that derives the mesh seed with the
legacy “yucp.wm.mesh.embed.v2” HKDF domain when the current v3 decode path
fails. Keep the existing v3 path as the primary behavior and limit the fallback
to mesh decoding; update the relevant decoder flow rather than changing
wm_basis_seed’s current profile behavior.
---
Duplicate comments:
In `@src/linuxCodecSandbox.ts`:
- Around line 241-251: Update the child stdin write flow around Bun.spawn so it
awaits child.stdin.end() before clearing frame. Keep frame.fill(0) in the
finally block, but ensure both the write and stream finalization complete before
releasing the frame contents.
In `@src/linuxTreeSandbox.ts`:
- Around line 64-94: Update runWorker to configure a finite Bun.spawn timeout
and appropriate killSignal for the Docker worker, using the existing worker
lifecycle options so stalled docker runs are terminated and child.exited cannot
hang indefinitely.
- Around line 17-62: Update treeWorkerArguments to run the container as a
non-root user by adding the appropriate Docker --user setting, and cap total
memory including swap by setting --memory-swap to the intended limit alongside
the existing --memory 512m option. Preserve the current container mounts,
capabilities, and resource limits.
---
Nitpick comments:
In `@src/attribution.test.ts`:
- Around line 222-237: The attribution tests only cover successful matches and
need negative-path coverage. Extend the tests around attributeArchiveAssets to
include at least a wrong candidateFileKeys/file key case, asserting the result
is not attributed with matched: false; also cover an unrelated asset or
mismatched attributionTokenHash if supported by the existing fixtures,
exercising the hash re-verification behavior.
- Around line 61-72: Move the Docker/native-runtime test identified by
“identifies a personalized output through its v2 derivation record” out of the
default Bun test suite by placing it in a .realtest.ts file, or add an
equivalent runtime skip guard when Docker or yucp_coupling.so is unavailable;
preserve the test’s existing assertions and behavior when the required runtime
is present.
In `@src/attribution.ts`:
- Around line 123-157: Harden the Bun.spawn Docker invocation by adding the
fixed non-root user 65534:65534 before the image argument. Validate
input.runtimePath before constructing its bind-mount specification, requiring an
absolute path and rejecting paths containing commas or other characters that
would corrupt Docker mount options; preserve the existing read-only mount
behavior.
In `@src/linuxTreeWorker.py`:
- Around line 211-216: Update the __main__ exception handler in
linuxTreeWorker.py to emit structured JSON matching the codec and attribution
workers, including errorType, error, and schemaVersion fields. Use the caught
exception’s class name and message, serialize the payload to stderr, and
preserve the existing nonzero exit behavior.
In `@src/materializationServer.test.ts`:
- Around line 44-69: The validateDockerIsolationInfo tests cover rootless and
cgroup failures but omit invalid platform values. Add negative cases asserting
that operatingSystem 'windows' and architecture 'aarch64' each throw the
expected validation error, while preserving the existing valid Linux x86_64
case.
- Around line 363-364: Update the test around attributeAssets to distinguish
derivation from zeroization: capture a copy of the derived key material inside
attributeAssets before the handler’s finally block wipes derivedKeys, then
assert the captured copy for derivation and retain the existing derivedKeys
assertion only as the zeroization check.
In `@src/materializationServer.ts`:
- Around line 129-139: Update the Bun.spawn options in the Docker info execution
flow to set stderr to 'ignore' instead of opening an unconsumed pipe. Keep the
existing stdout capture, timeout, and child.exited handling unchanged.
- Around line 221-233: Update readJsonResponse to enforce MAXIMUM_RESPONSE_BYTES
while reading the response stream incrementally, rather than calling
response.text() before validation. Reuse the bounded streaming approach from
readBoundedRequestJson, preserving the existing content-length validation and
oversized-response error behavior.
- Around line 703-811: Update the attribution candidate processing around the
synchronous candidates.map and deriveFileKeys call to prevent up to 4096
derivations from blocking the event loop. Preserve validation and derived-key
behavior while either reducing the accepted candidate limit or converting
processing to an asynchronous loop that yields periodically between bounded
batches.
- Around line 1020-1028: Update the shutdown handler around shutdown and the
SIGINT/SIGTERM process.once registrations so rejections from server.stop() are
caught and logged as shutdown failures instead of becoming unhandled promise
rejections. Preserve the existing abort and server-stop ordering, and use the
surrounding module’s established logging mechanism.
In `@src/materializationService.test.ts`:
- Around line 178-215: The source manifest fixture in the materialization test
only covers protected files, leaving the common-file branch of
assembleSourceTree untested. Extend the manifest’s files array with a separate
classification: 'common' entry, including its own chunk metadata and
logicalFiles: 2, and provide the corresponding source object data needed by the
fixture while preserving the existing protected-file setup.
In `@src/rendition.test.ts`:
- Around line 258-261: Update the RenditionStageFailure rejection assertions in
the relevant test to verify the exact public error message explicitly and
confirm that the serialized error does not include a cause field or private
codec details. Replace the tautological rejects.not.toThrow assertion while
preserving the existing MATERIALIZATION_CODEC_BATCH_FAILED errorCode check.
In `@src/serverArchitecture.test.ts`:
- Around line 25-39: Update the Bun.Glob pattern in the productionSources
collection to recursively match TypeScript files throughout src and its
subdirectories, while preserving the existing test-file filters and file-reading
logic.
In `@yucp_coupling/build.sh`:
- Around line 41-55: Validate the version-script file referenced by LINK_FLAGS
alongside the existing SODIUM_PREFIX checks in the build precondition, emitting
a clear error and exiting before linking when it is missing. Also update the
CFLAGS assignment to undefine any pre-existing FORTIFY_SOURCE definition before
setting it to 2, avoiding redefinition warnings.
In `@yucp_coupling/guard.c`:
- Around line 1997-2001: Replace the duplicated placement-epoch bound in both
the encoder loop and decoder function xg_0125 with a shared
WM_MESH_PLACEMENT_EPOCHS constant, preserving the current single-epoch behavior
by setting it to 1u. Use that constant wherever placement_epoch is bounded so
the two paths cannot drift.
- Around line 1710-1724: Replace the valid_slots allocation in the surrounding
guard function with calloc so the element-count and sizeof(size_t)
multiplication are overflow-checked, while preserving the existing failure
cleanup and return behavior. Also add a concise comment documenting that
wm_mesh_scalar_set_bit only changes mantissa bits, preserves coordinate
finiteness, and therefore keeps valid_count identical during encoding and
decoding.
- Around line 1573-1600: Update wm_vlist_keep_mesh_rank to validate that each
property appears in only one contiguous run across vl->d before ranking groups,
and fail loudly if a property reappears after another property. Apply the
validation consistently with the existing grouping passes, preserving the
current comparator and deterministic ordering; alternatively, replace
contiguous-run grouping with property-based aggregation if that is the
established design.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@deploy/run-local-wsl-materializer.sh`:
- Around line 16-22: After the individual validations for source_proxy_port and
control_proxy_port, reject configurations where both ports are equal by emitting
an error to stderr and exiting with status 64. Add this check before either
socat instance starts, while preserving the existing range validation behavior.
- Around line 12-15: Update the windows_host validation in
run-local-wsl-materializer.sh to validate each IPv4 octet is within 0–255, not
merely one or more digits. Keep invalid addresses on the existing error path
that prints the diagnostic and exits with status 64.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 384c2b63-c0e6-4ec0-b400-19365d06343a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
deploy/prepare-local-wsl-materializer.shdeploy/run-local-wsl-materializer.shdeploy/run-local-wsl-materializer.test.tsdeploy/yucp-materializer.servicepackage.jsonsrc/env.test.tssrc/linuxRendition.realtest.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/serverArchitecture.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- deploy/prepare-local-wsl-materializer.sh
- package.json
- src/serverArchitecture.test.ts
- deploy/yucp-materializer.service
- src/env.test.ts
- src/materializationServer.test.ts
- src/materializationService.test.ts
- src/materializationService.ts
- src/materializationServer.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63c978ae9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/materializationServer.ts (1)
571-608: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFailure reporting is bound to the shutdown signal. On SIGTERM,
parentSignalis already aborted, so theFAIL_PATHpost aborts immediately and the job stays leased until expiry. Consider using a detached signal (timeout only) for the terminal failure report.🤖 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/materializationServer.ts` around lines 571 - 608, The failure-reporting request in the catch block must not reuse the already-aborted parentSignal during shutdown. Update the postJson call for FAIL_PATH to use a detached signal governed only by the existing control-plane timeout, while preserving the failure payload and cleanup behavior in the surrounding finally block.
🧹 Nitpick comments (3)
src/materializationService.ts (1)
78-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing this helper with
materializationServer.ts.postJsoninsrc/materializationServer.ts(Lines 358-395) re-implements the same parent-signal + timeout + dispose logic; extracting one utility keeps the abort semantics from drifting.🤖 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/materializationService.ts` around lines 78 - 102, Extract createBoundedRequestSignal into a shared utility and update both createBoundedRequestSignal callers, including postJson in materializationServer.ts, to use it. Preserve the existing parent-abort propagation, timeout reason, and dispose behavior without maintaining duplicate implementations.src/nativeHardening.test.ts (1)
124-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against missing markers so the alpha-channel assertion can't pass vacuously. If either
indexOfreturns-1(marker renamed),sliceyields an empty/incorrect region andnot.toMatchtrivially passes.💚 Proposed fix
- const profiles = guardSource.slice( - guardSource.indexOf('static const wm_img_profile WM_IMG_PROFILES[]'), - guardSource.indexOf('`#define` WM_IMG_PROFILE_COUNT') - ); + const start = guardSource.indexOf( + 'static const wm_img_profile WM_IMG_PROFILES[]' + ); + const end = guardSource.indexOf('`#define` WM_IMG_PROFILE_COUNT'); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + const profiles = guardSource.slice(start, 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 `@src/nativeHardening.test.ts` around lines 124 - 132, Update the profiles extraction in the “never writes image attribution into the alpha channel” test to validate that both marker lookups succeed before slicing. Fail the test explicitly when either marker is missing, then retain the existing assertion against the extracted WM_IMG_PROFILES region.src/serverArchitecture.test.ts (1)
140-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 145 asserts the test file contains its own literal — a tautology. It passes by construction and adds no coverage; the recursive-scan behavior is already exercised by Line 26.
🤖 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/serverArchitecture.test.ts` around lines 140 - 148, Remove the self-referential architectureTestSource assertion for "new Bun.Glob('**/*.ts')" from the test, since it only verifies the test file contains its own literal. Keep the serverSource environment-host assertion and the existing recursive-scan coverage unchanged.
🤖 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 `@deploy/initialize-local-master-credential.sh`:
- Around line 30-58: Serialize the entire credential read–merge–rename
transaction in the initialization flow, using a lock associated with target so
concurrent runs cannot overwrite epoch rotations. Replace the shared
`${target}.tmp` temporary path with a unique per-process temporary file, then
atomically rename it while the lock is held and ensure cleanup and lock release
on success or failure.
In `@src/linuxCodecSandbox.ts`:
- Around line 226-228: Update the output-directory setup before
createLinuxCodecRequest and Docker spawning so the recursively created codec
output tree is writable by uid 65534, either by applying sandbox-accessible
permissions during creation or by chowning the directory afterward; preserve the
existing request validation flow.
In `@src/linuxCodecWorker.py`:
- Around line 60-70: Move the trailing-byte validation using
sys.stdin.buffer.read(1) inside the existing try block in read_request(), so a
ValueError for extra frame bytes triggers the current key-zeroization except
handler. Preserve the existing trailing-bytes error and successful return
behavior.
In `@src/sandboxIsolation.ts`:
- Line 7: Update the PATH assignment in the sandbox environment configuration to
always use the fixed DEFAULT_PATH allowlist instead of inheriting
environment.PATH; ensure sandbox subprocesses cannot resolve commands through
user-writable directories or empty path components.
---
Outside diff comments:
In `@src/materializationServer.ts`:
- Around line 571-608: The failure-reporting request in the catch block must not
reuse the already-aborted parentSignal during shutdown. Update the postJson call
for FAIL_PATH to use a detached signal governed only by the existing
control-plane timeout, while preserving the failure payload and cleanup behavior
in the surrounding finally block.
---
Nitpick comments:
In `@src/materializationService.ts`:
- Around line 78-102: Extract createBoundedRequestSignal into a shared utility
and update both createBoundedRequestSignal callers, including postJson in
materializationServer.ts, to use it. Preserve the existing parent-abort
propagation, timeout reason, and dispose behavior without maintaining duplicate
implementations.
In `@src/nativeHardening.test.ts`:
- Around line 124-132: Update the profiles extraction in the “never writes image
attribution into the alpha channel” test to validate that both marker lookups
succeed before slicing. Fail the test explicitly when either marker is missing,
then retain the existing assertion against the extracted WM_IMG_PROFILES region.
In `@src/serverArchitecture.test.ts`:
- Around line 140-148: Remove the self-referential architectureTestSource
assertion for "new Bun.Glob('**/*.ts')" from the test, since it only verifies
the test file contains its own literal. Keep the serverSource environment-host
assertion and the existing recursive-scan coverage unchanged.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 089dc174-fd00-4879-99eb-2b067b9d09b0
⛔ Files ignored due to path filters (1)
yucp_coupling/out/linux-x64/Release/yucp_coupling.sois excluded by!**/*.so
📒 Files selected for processing (25)
.env.exampledeploy/initialize-local-master-credential.shdeploy/run-local-wsl-materializer.shdeploy/run-local-wsl-materializer.test.tssrc/attribution.test.tssrc/attribution.tssrc/keyBroker.test.tssrc/keyBroker.tssrc/linuxAttributionWorker.pysrc/linuxCodecSandbox.test.tssrc/linuxCodecSandbox.tssrc/linuxCodecWorker.pysrc/linuxTreeSandbox.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/nativeHardening.test.tssrc/rendition.test.tssrc/rendition.tssrc/sandboxIsolation.tssrc/serverArchitecture.test.tsyucp_coupling/build.shyucp_coupling/guard.cyucp_coupling/out/linux-x64/Release/yucp_coupling.sha256
🚧 Files skipped from review as they are similar to previous changes (12)
- yucp_coupling/out/linux-x64/Release/yucp_coupling.sha256
- src/keyBroker.test.ts
- src/materializationService.test.ts
- src/linuxTreeSandbox.ts
- src/rendition.ts
- src/attribution.test.ts
- yucp_coupling/build.sh
- src/materializationServer.test.ts
- src/keyBroker.ts
- src/attribution.ts
- src/linuxAttributionWorker.py
- yucp_coupling/guard.c
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fbb079dd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/materializationBuilds.ts (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePython test-exclusion pattern only matches suffix convention.
EXCLUDED_SOURCE_FILE_PATTERNSexcludes_test.py$but not the common pytest prefix convention (test_*.py). If a future Python test file is added using the prefix style, it would be included in thehelperdigest, causing the build identity to change on test-only edits.♻️ Suggested broadening
const EXCLUDED_SOURCE_FILE_PATTERNS = [ /\.test\.tsx?$/, /\.realtest\.tsx?$/, /_test\.py$/, + /^test_.*\.py$/, /\.pyc$/, ];🤖 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/materializationBuilds.ts` around lines 11 - 16, Update EXCLUDED_SOURCE_FILE_PATTERNS to also exclude Python files using the pytest prefix convention, matching test_*.py while preserving the existing _test.py exclusion.src/materializationBuilds.test.ts (1)
15-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid path-aware hashing coverage, but exclusion-path branches are untested.
The test thoroughly proves order-independence and that most implementation inputs affect the
helper/codec/runtimedigests. However, two exclusion-sensitive branches inmaterializationBuilds.tsare never exercised:
src/nested/ignored_test.py(the_test.py$Python exclusion pattern) is created in the fixture but never mutated/asserted against, unlikesrc/ignored.test.tsat Lines 56-65.- The symlink-rejection and path-escape guards in
collectRuntimeSourceEntries(materializationBuilds.ts Lines 64-66, 75-81) have no corresponding test.Given this digest feeds mandatory self-verification/signed receipts, gaps here are worth closing.
♻️ Suggested additions
+ await writeFile( + path.join(first.root, 'src', 'nested', 'ignored_test.py'), + 'test-only-change\n' + ); + expect( + await computeMaterializationBuilds({ + runtimePath: first.runtimePath, + serviceRoot: first.root, + }) + ).toEqual(baseline);Also applies to: 41-115
🤖 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/materializationBuilds.test.ts` around lines 15 - 38, Extend the materialization build tests to mutate src/nested/ignored_test.py and assert the relevant digest remains unchanged, covering the _test.py exclusion alongside the existing ignored.test.ts case. Add focused coverage for collectRuntimeSourceEntries that verifies symlinked entries are rejected and paths escaping the source root are excluded or rejected according to the existing contract.
🤖 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 `@deploy/initialize-local-master-credential.test.sh`:
- Around line 17-18: Update the background initializer wait logic to collect the
exit statuses for both first_pid and second_pid before triggering set -e
termination. Ensure both processes are reaped even when the first wait fails,
then fail the test if either status is nonzero while preserving the existing
cleanup behavior.
In `@src/sandboxIsolation.test.ts`:
- Around line 25-32: Update the test named “uses a fixed non-root identity with
bounded swap” to assert the complete security flag/value pairs, verifying --user
is paired with 65534:65534 and --memory-swap is paired with the expected memory
value derived from 768m, rather than checking individual tokens.
In `@src/sandboxMounts.ts`:
- Around line 121-148: Update the file-staging flow around copyFile so
chmod(destinationPath, 0o444) runs after both the new-file and EEXIST
deduplication branches complete. Preserve the existing validation of matching
staged content, and add a regression test covering a pre-created matching
destination with mode 0666 that is returned with read-only permissions.
---
Nitpick comments:
In `@src/materializationBuilds.test.ts`:
- Around line 15-38: Extend the materialization build tests to mutate
src/nested/ignored_test.py and assert the relevant digest remains unchanged,
covering the _test.py exclusion alongside the existing ignored.test.ts case. Add
focused coverage for collectRuntimeSourceEntries that verifies symlinked entries
are rejected and paths escaping the source root are excluded or rejected
according to the existing contract.
In `@src/materializationBuilds.ts`:
- Around line 11-16: Update EXCLUDED_SOURCE_FILE_PATTERNS to also exclude Python
files using the pytest prefix convention, matching test_*.py while preserving
the existing _test.py exclusion.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90ee8862-5d73-4dbc-b4d5-e8a87ff99c64
📒 Files selected for processing (21)
deploy/initialize-local-master-credential.shdeploy/initialize-local-master-credential.test.shsrc/attribution.tssrc/keyBroker.test.tssrc/keyBroker.tssrc/linuxCodecSandbox.test.tssrc/linuxCodecSandbox.tssrc/linuxCodecWorker.pysrc/linuxTreeSandbox.tssrc/linuxTreeWorker.pysrc/linuxWorkers.test.tssrc/linuxWorkers_test.pysrc/materializationBuilds.test.tssrc/materializationBuilds.tssrc/materializationServer.tssrc/rendition.tssrc/sandboxIsolation.test.tssrc/sandboxIsolation.tssrc/sandboxMounts.test.tssrc/sandboxMounts.tssrc/serverArchitecture.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/sandboxIsolation.ts
- src/linuxCodecSandbox.test.ts
- src/keyBroker.test.ts
- deploy/initialize-local-master-credential.sh
- src/linuxTreeSandbox.ts
- src/serverArchitecture.test.ts
- src/keyBroker.ts
- src/rendition.ts
- src/attribution.ts
- src/linuxCodecWorker.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 487917ff89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/boundedJson.ts`:
- Around line 1-7: Validate input.maximumBytes at the start of
readBoundedJsonResponse before using it for any size checks, rejecting values
that are not finite non-negative safe integers, including NaN, Infinity,
negatives, and unsafe integers. Preserve the existing bounded JSON response
behavior for valid limits.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83c1aedc-39a9-4792-91c4-0e055b6524c4
📒 Files selected for processing (11)
deploy/initialize-local-master-credential.test.shsrc/boundedJson.tssrc/linuxTreeSandbox.test.tssrc/linuxTreeSandbox.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/sandboxIsolation.test.tssrc/sandboxMounts.test.tssrc/sandboxMounts.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/sandboxIsolation.test.ts
- src/sandboxMounts.test.ts
- deploy/initialize-local-master-credential.test.sh
- src/materializationService.test.ts
- src/sandboxMounts.ts
- src/materializationServer.ts
- src/materializationServer.test.ts
- src/materializationService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c44aba33d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/sandboxContainer.ts (1)
68-71: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSuccess path is still blocked on the stderr read (previously flagged, not applied).
await stderrruns unconditionally before checkingoutcome.exitCode === 0. If the bounded stderr read rejects (oversized/garbled daemon output), a container that was actually removed successfully will still throw a cleanup failure. Check success first and only await/parse stderr for non-zero exits.🛡️ Proposed fix: check success before awaiting stderr
- const errorText = await stderr; if (outcome.exitCode === 0) { + void stderr.catch(() => undefined); return; } + const errorText = await stderr.catch(() => '');🤖 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/sandboxContainer.ts` around lines 68 - 71, Update the cleanup flow around outcome.exitCode so the successful exit check occurs before awaiting stderr. Return immediately when exitCode is 0, and only await and process stderr for non-zero exits, preserving the existing failure handling.
🧹 Nitpick comments (18)
src/materializationChunkCache.ts (1)
1393-1406: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
dependencies.now()is sampled twice, so the metadata record and the in-memory index can disagree.If the injected clock advances between Lines 1396 and 1403, the persisted
lastAccessedAtand the index entry diverge, and eviction ordering then depends on whether the index was rebuilt from disk. Sample once.♻️ Sample the clock once
+ const lastAccessedAt = dependencies.now(); await dependencies.writeJsonAtomic(metadataPath(chunkId), { bytes: actual.bytes, chunkId, - lastAccessedAt: dependencies.now(), + lastAccessedAt, schemaVersion: 1, sha256: actual.sha256, } satisfies MetadataRecord); index.set(chunkId, { bytes: actual.bytes, chunkId, - lastAccessedAt: dependencies.now(), + lastAccessedAt, objectPath: destination, sha256: actual.sha256, });🤖 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/materializationChunkCache.ts` around lines 1393 - 1406, In the materialization flow around the metadata write and index.set call, sample dependencies.now() once into a shared timestamp before constructing either record. Reuse that timestamp for both the persisted MetadataRecord and the in-memory index entry so lastAccessedAt remains identical.src/materializationChunkCache.test.ts (2)
160-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text assertions will break on any harmless rename.
expect(source.match(/scanCacheEntries\(\)/g)).toHaveLength(2)counts the declaration plus its single call site, so renaming the helper or adding a second legitimate call fails the test without any behavioral regression. A behavioral assertion (e.g. commit N chunks with an injectedstatfs/readdirspy and assert the scan count stays constant) would encode the same intent more durably.🤖 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/materializationChunkCache.test.ts` around lines 160 - 167, Replace the brittle source-text assertions in the “does not rescan every committed object for each capacity decision” test with a behavioral test: commit multiple chunks using injected statfs/readdir spies, then assert cache scanning remains bounded or constant rather than counting scanCacheEntries() occurrences. Retain coverage that capacity-generation.json is used through observable behavior, not source-text matching.
640-693: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWall-clock choreography here is flake-prone.
The test depends on
Bun.sleep(45)landing inside a 90 ms injected deletion; on a loaded CI runner the secondmaintain()can start after the first finished, and thetoHaveLength(1)assertion then still passes for the wrong reason (or fails if both evict). Consider gating with an explicit "deletion in progress" promise that the second maintenance awaits, instead of a sleep.🤖 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/materializationChunkCache.test.ts` around lines 640 - 693, Replace the fixed Bun.sleep(45) timing in the “keeps the kernel capacity lock for the complete slow operation” test with an explicit synchronization gate: start the second maintain only after deletionStarted confirms deletion has begun, and keep deletion blocked until the concurrent maintenance has reached the intended contention point. Remove the wall-clock dependency while preserving the assertion that exactly one chunk is evicted.src/materializationServer.ts (1)
512-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe consecutive-stale-renewal guard is unreachable.
armLeaseDeadline(leaseExpiresAt)(Line 553) abortsmaterializationAbortimmediately whenleaseExpiresAt - Date.now() <= 0, and the very nextmaterializationSignal.throwIfAborted()(Line 554) exits the loop. So theleaseExpiresAt <= Date.now()branch at Lines 555-567 — and thereforemaximumConsecutiveStaleRenewals— can never increment past the first stale response.materializationServer.test.tsLine 499 confirms this by asserting exactly one renewal request.Either drop the counter and the constant, or move the tolerance into
armLeaseDeadlineso a single stale expiry does not immediately kill the job.🤖 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/materializationServer.ts` around lines 512 - 567, The consecutive-stale-renewal counter in renewLease is unreachable because armLeaseDeadline aborts before the stale-response branch can run. Remove consecutiveStaleRenewals and maximumConsecutiveStaleRenewals, along with the related stale-expiry handling, while preserving validation, lease deadline arming, and abort behavior for expired leases.src/materializationService.ts (2)
1990-1996: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftA fresh chunk cache is constructed per job, forcing a full object-store rescan each time.
createMaterializationChunkCacherunsrefreshCacheIndexunder the capacity lock during construction (materializationChunkCache.tsLines 1184-1186), and the in-process index lives inside that closure, so a per-job instance always starts cold and re-stats plus re-reads metadata for every committed object. At the configured 7 GiB quota with ~256 KiB chunks that is ~28kstat+ ~28kreadFilebefore the first byte is downloaded, on every job.
startMaterializationServer(materializationServer.tsLine 1207) already builds one long-lived cache; consider threading that instance throughmaterializeCapabilityJob(with the current construction kept as a fallback for tests) so the warm index is reused.🤖 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/materializationService.ts` around lines 1990 - 1996, Reuse the long-lived chunk cache created by startMaterializationServer by threading that instance into materializeCapabilityJob and using it instead of constructing a new cache for each job. Preserve the existing createMaterializationChunkCache construction as a fallback when no cache is supplied, including its current failure classification, so tests and standalone callers continue to work.
1730-1732: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
localeComparebreaks the byte-ordering convention used everywhere else.Chunk ids are validated lowercase hex so the practical outcome is the same today, but
localeCompareis locale- and ICU-version dependent, whereascompareNormalizedPathsUtf8(Line 54) and the cache'scompareUtf8deliberately sort by UTF-8 bytes. Download order feeds eviction recency, so keep it deterministic across hosts.♻️ Use the existing byte comparison
- const chunks = [...uniqueChunks.values()].sort((left, right) => - left.id.localeCompare(right.id) - ); + const chunks = [...uniqueChunks.values()].sort((left, right) => + compareNormalizedPathsUtf8(left.id, right.id) + );🤖 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/materializationService.ts` around lines 1730 - 1732, Update the sort comparator for uniqueChunks in the materialization flow to use the existing UTF-8 byte-order comparison utility, compareNormalizedPathsUtf8 or the established equivalent, instead of left.id.localeCompare(right.id). Preserve sorting by chunk ID and ensure the order remains deterministic across hosts.deploy/initialize-local-master-credential.test.sh (1)
1-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider covering the symlink/permission-rejection branches.
The suite exercises epoch validation and concurrent rotation well, but doesn't test the credential-path symlink rejection or wrong-permission rejection branches in
initialize-local-master-credential.sh. Worth adding given this script guards a security-sensitive credential file.🤖 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 `@deploy/initialize-local-master-credential.test.sh` around lines 1 - 39, Extend the test coverage for initialize-local-master-credential.sh to exercise both security rejection branches: pre-create the credential path as a symlink and verify initialization fails, then create the credential file with unsafe permissions and verify it is rejected. Keep the existing epoch, concurrency, and cleanup assertions intact, using isolated temporary HOME state where needed.src/linuxCodecSandbox.test.ts (1)
136-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnnecessary
as unknown as {…}casts for already-exported symbols.
parseLinuxCodecResult,prepareLinuxCodecSandboxInput, andrunLinuxCodecContainerare all exported from./linuxCodecSandbox; importing them directly removes the hand-written structural types that can silently drift from the real signatures.Also applies to: 179-183, 211-229
🤖 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/linuxCodecSandbox.test.ts` around lines 136 - 144, The tests unnecessarily access exported linuxCodecSandbox functions through unknown structural casts. Import parseLinuxCodecResult, prepareLinuxCodecSandboxInput, and runLinuxCodecContainer directly from ./linuxCodecSandbox, then call them without hand-written type assertions at the referenced test sections.src/attribution.ts (1)
268-286: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCodec error codes escape from the attribution path.
Both helpers throw
LinuxCodecFailure('MATERIALIZATION_CODEC_SANDBOX_FAILED', …).src/linuxTreeSandbox.tswraps these intoLinuxTreeSandboxFailurefor exactly this reason; attribution surfaces a codec code for an attribution worker fault, which misroutes error classification and metrics. Wrap with an attribution-specific error here.🤖 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/attribution.ts` around lines 268 - 286, Update the attribution flow around writeLinuxCodecProcessInput and collectLinuxCodecProcessOutput to catch LinuxCodecFailure exceptions and wrap them in the attribution-specific error type and code, preserving the original error as context. Ensure codec error codes do not escape attribution worker failures, matching the wrapping behavior used by LinuxTreeSandbox.src/attribution.test.ts (1)
34-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text assertions are brittle.
These grep the implementation file rather than exercising behavior, so a formatting change (e.g.
frame ?. fill(0)) fails the test while a genuine regression that keeps the string passes. Behavioral coverage for the same properties already exists insrc/linuxCodecSandbox.test.ts(termination/draining/zeroization); consider asserting behavior via injected stubs 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 `@src/attribution.test.ts` around lines 34 - 55, The tests in the attribution worker response-boundary suite rely on brittle source-text matching instead of runtime behavior. Replace the assertions in the two tests with behavioral tests using injected stubs, covering bounded input/output handling and container naming, cleanup, and frame zeroization; reuse the existing patterns from linuxCodecSandbox.test.ts rather than checking implementation strings.src/linuxTreeSandbox.ts (1)
174-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
JSON.parse(stdout)result is cast toTwith no shape check insiderunWorker.Both callers validate the fields they read, so this is contained today, but a malformed/oversized-but-under-cap payload reaching
JSON.parsethrows a rawSyntaxErrorrather than aLinuxTreeSandboxFailure, bypassing the error-code contract the class establishes. Consider wrapping the parse.🤖 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/linuxTreeSandbox.ts` around lines 174 - 186, Wrap the JSON.parse call in runWorker with error handling so malformed worker output does not escape as a raw SyntaxError. Convert parse failures into the established LinuxTreeSandboxFailure type and preserve its error-code contract, while keeping valid JSON returned as T unchanged.src/linuxWorkers.test.ts (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded test count makes this assertion churn on every added Python test.
'Ran 10 tests'must be edited wheneverlinuxWorkers_test.pygrows, and a failure surfaces as a substring mismatch rather than the actual Python failure output. Asserting the exit code plus a non-zero run count keeps the signal without the coupling.♻️ Proposed change
- expect(`${stdout}${stderr}`).toContain('Ran 10 tests'); - expect(exitCode).toBe(0); + const output = `${stdout}${stderr}`; + expect(output).toMatch(/Ran [1-9]\d* tests/); + expect(output).toContain('OK'); + expect(exitCode).toBe(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 `@src/linuxWorkers.test.ts` around lines 34 - 35, Update the test assertion near the Python worker execution to stop checking the exact “Ran 10 tests” string. Keep the exitCode success assertion, and instead verify the combined stdout/stderr indicates a non-zero test count while preserving the failure output for diagnosis.src/linuxCodecWorker.py (1)
317-324: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUncarryable fallback emits the raw source archive, bypassing the normalization just performed.
shutil.copyfile(source_path, output_path)replaces the sanitized/rebuilt archive with the untrusted original, so normalized entry names (backslash rewriting, directory-suffix handling) and the deterministic rebuild are discarded for optional ZIPs with no carryable asset. Since the rebuilt archive already passed every validation, writing it out keeps outputs uniform and byte-deterministic.♻️ Proposed change
if coupled_entries == 0: if required: raise RuntimeError("nested ZIP contains no carryable visual asset") - shutil.copyfile(source_path, output_path) return "uncarryable"🤖 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/linuxCodecWorker.py` around lines 317 - 324, Update the optional no-carryable-asset branch in the nested ZIP handling logic around the coupled_entries check to write the already normalized and deterministically rebuilt archive to output_path instead of copying source_path. Preserve the existing required error and "uncarryable" return behavior.src/materializationBuilds.test.ts (2)
233-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-text scraping does not verify the symlink rejection.
This asserts that
materializationBuilds.tscontainslstat/isSymbolicLink()strings; it stays green if the guard becomes unreachable or the error is swallowed, and breaks on any harmless rename. Exercise the boundary instead by symlinking a required input in the fixture and asserting the rejection.💚 Proposed behavioral test
- it('rejects symbolic links at the required build-input boundary', async () => { - const source = await Bun.file( - new URL('./materializationBuilds.ts', import.meta.url) - ).text(); - const boundary = source.slice( - source.indexOf('async function requiredFileEntry'), - source.indexOf('export async function computeMaterializationBuilds') - ); - - expect(boundary).toContain('await lstat(filePath)'); - expect(boundary).toContain('details.isSymbolicLink()'); - }); + it('rejects symbolic links at the required build-input boundary', async () => { + const fixture = await createFixture(); + try { + const lockPath = path.join(fixture.root, 'bun.lock'); + const realLockPath = path.join(fixture.root, 'bun.lock.real'); + await rename(lockPath, realLockPath); + await symlink(realLockPath, lockPath); + + await expect( + computeMaterializationBuilds({ + runtimePath: fixture.runtimePath, + serviceRoot: fixture.root, + }) + ).rejects.toThrow('not a regular file'); + } finally { + await rm(fixture.root, { force: true, recursive: true }); + } + });Requires adding
renameandsymlinkto thenode:fs/promisesimport.🤖 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/materializationBuilds.test.ts` around lines 233 - 244, Replace the source-text assertion in the “rejects symbolic links at the required build-input boundary” test with a behavioral fixture: create or rename a required build input, symlink it using the node:fs/promises helpers, invoke computeMaterializationBuilds, and assert that it rejects. Update the relevant imports to include rename and symlink, while preserving the existing fixture setup and validating the actual boundary behavior rather than implementation text.
53-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the optional-cast indirection; the symbol is a direct export.
pinMaterializationBuildArtifactsis exported from./materializationBuildsand is already called directly at Line 123, so theas unknown as { …?: … }cast plus theif (!…) return;early exit is dead scaffolding that also lets the test pass vacuously if the export ever disappears at runtime.🤖 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/materializationBuilds.test.ts` around lines 53 - 74, Use the direct pinMaterializationBuildArtifacts export in the test instead of retrieving it through an optional cast. Remove the local type assertion, optional property typing, and the if (!pinMaterializationBuildArtifacts) early return, while preserving the existing direct invocation and assertions.src/sandboxContainer.test.ts (1)
1-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for stderr-read failures on successful removal.
Coverage gap: no test exercises a rejecting/oversized stderr stream paired with
exitCode === 0, which is exactly the scenario the still-open ordering bug insandboxContainer.ts(Line 68) would break.🤖 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/sandboxContainer.test.ts` around lines 1 - 50, Add a regression test in the “sandbox container cleanup” suite for a successful forced removal whose process exits with code 0 but whose stderr stream rejects or exceeds the supported read limit. Assert cleanup resolves successfully, covering the stderr-read ordering behavior in removeSandboxContainer.src/sandboxMounts.ts (2)
151-157: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull in-memory read for dedup comparison on large files.
On the
EEXIST(concurrent-stager) path, both source and destination are fully read into memory viareadFileto compare bytes. For large materialization assets (textures, meshes), this doubles peak memory per concurrent staging collision. A streaming hash comparison (e.g.,crypto.createHashpiped from both file streams) would avoid buffering entire files.♻️ Sketch using streaming hashes
- const [sourceBytes, destinationBytes] = await Promise.all([ - readFile(input.sourcePath), - readFile(destinationPath), - ]); - if (!sourceBytes.equals(destinationBytes)) { + const [sourceHash, destinationHash] = await Promise.all([ + hashFile(input.sourcePath), + hashFile(destinationPath), + ]); + if (sourceHash !== destinationHash) { throw new Error('The staged sandbox file does not match its source'); }🤖 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/sandboxMounts.ts` around lines 151 - 157, Update the EEXIST comparison in the sandbox staging flow around sourcePath and destinationPath to avoid loading both files with readFile; compare their contents using streaming hashes or an equivalent chunked stream-based approach, and retain the existing mismatch error behavior.
15-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the three near-duplicate recursive tree walkers.
exposeReadonlyEntry,restorePrivateSandboxEntry, andexposeWritableEntryall repeat the same lstat → reject-symlink → recurse-into-directory → chmod skeleton, differing only in the target permission bits and ENOENT tolerance. Since this is security-sensitive permission-setting code, duplication increases the risk that a future symlink-check or ENOENT fix only lands in one copy.♻️ Sketch of a shared walker
+async function walkSandboxTree( + entryPath: string, + options: { + tolerateMissing?: boolean; + chmodDir: (mode: number) => number; + chmodFile: (mode: number) => number; + } +): Promise<void> { + // shared lstat/symlink/recurse logic, delegating final chmod to callers +}Also applies to: 39-69, 84-104
🤖 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/sandboxMounts.ts` around lines 15 - 31, Consolidate exposeReadonlyEntry, restorePrivateSandboxEntry, and exposeWritableEntry around one shared recursive walker that performs lstat, rejects symbolic links, recurses through directories, and applies caller-specified file and directory permissions. Preserve each existing function’s permission values and ENOENT tolerance by passing those behaviors into the shared helper, while retaining the current errors and security checks.
🤖 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 `@deploy/yucp-materializer.service`:
- Line 8: Update the WorkingDirectory setting in the materializer service unit
to /home/yucp/ca-coupling, matching the checkout path used by
prepare-local-wsl-materializer.sh, run-local-wsl-materializer.sh, and
.env.example so ExecStart can locate the project files.
In `@src/attribution.ts`:
- Around line 300-303: Update the cleanup call to removeSandboxContainer within
the try/finally flow so its rejection is swallowed and cannot replace the
original attribution error; await it with a catch that resolves to undefined
while preserving the existing conditional containerName check.
In `@src/linuxCodecSandbox.ts`:
- Around line 431-439: Update the catch block around the sandbox operation to
suppress any rejection from removeContainer, ensuring cleanup failure cannot
replace the original cause or classified LinuxCodecFailure. Preserve the
existing rethrow for LinuxCodecFailure and wrapping behavior for other causes.
---
Duplicate comments:
In `@src/sandboxContainer.ts`:
- Around line 68-71: Update the cleanup flow around outcome.exitCode so the
successful exit check occurs before awaiting stderr. Return immediately when
exitCode is 0, and only await and process stderr for non-zero exits, preserving
the existing failure handling.
---
Nitpick comments:
In `@deploy/initialize-local-master-credential.test.sh`:
- Around line 1-39: Extend the test coverage for
initialize-local-master-credential.sh to exercise both security rejection
branches: pre-create the credential path as a symlink and verify initialization
fails, then create the credential file with unsafe permissions and verify it is
rejected. Keep the existing epoch, concurrency, and cleanup assertions intact,
using isolated temporary HOME state where needed.
In `@src/attribution.test.ts`:
- Around line 34-55: The tests in the attribution worker response-boundary suite
rely on brittle source-text matching instead of runtime behavior. Replace the
assertions in the two tests with behavioral tests using injected stubs, covering
bounded input/output handling and container naming, cleanup, and frame
zeroization; reuse the existing patterns from linuxCodecSandbox.test.ts rather
than checking implementation strings.
In `@src/attribution.ts`:
- Around line 268-286: Update the attribution flow around
writeLinuxCodecProcessInput and collectLinuxCodecProcessOutput to catch
LinuxCodecFailure exceptions and wrap them in the attribution-specific error
type and code, preserving the original error as context. Ensure codec error
codes do not escape attribution worker failures, matching the wrapping behavior
used by LinuxTreeSandbox.
In `@src/linuxCodecSandbox.test.ts`:
- Around line 136-144: The tests unnecessarily access exported linuxCodecSandbox
functions through unknown structural casts. Import parseLinuxCodecResult,
prepareLinuxCodecSandboxInput, and runLinuxCodecContainer directly from
./linuxCodecSandbox, then call them without hand-written type assertions at the
referenced test sections.
In `@src/linuxCodecWorker.py`:
- Around line 317-324: Update the optional no-carryable-asset branch in the
nested ZIP handling logic around the coupled_entries check to write the already
normalized and deterministically rebuilt archive to output_path instead of
copying source_path. Preserve the existing required error and "uncarryable"
return behavior.
In `@src/linuxTreeSandbox.ts`:
- Around line 174-186: Wrap the JSON.parse call in runWorker with error handling
so malformed worker output does not escape as a raw SyntaxError. Convert parse
failures into the established LinuxTreeSandboxFailure type and preserve its
error-code contract, while keeping valid JSON returned as T unchanged.
In `@src/linuxWorkers.test.ts`:
- Around line 34-35: Update the test assertion near the Python worker execution
to stop checking the exact “Ran 10 tests” string. Keep the exitCode success
assertion, and instead verify the combined stdout/stderr indicates a non-zero
test count while preserving the failure output for diagnosis.
In `@src/materializationBuilds.test.ts`:
- Around line 233-244: Replace the source-text assertion in the “rejects
symbolic links at the required build-input boundary” test with a behavioral
fixture: create or rename a required build input, symlink it using the
node:fs/promises helpers, invoke computeMaterializationBuilds, and assert that
it rejects. Update the relevant imports to include rename and symlink, while
preserving the existing fixture setup and validating the actual boundary
behavior rather than implementation text.
- Around line 53-74: Use the direct pinMaterializationBuildArtifacts export in
the test instead of retrieving it through an optional cast. Remove the local
type assertion, optional property typing, and the if
(!pinMaterializationBuildArtifacts) early return, while preserving the existing
direct invocation and assertions.
In `@src/materializationChunkCache.test.ts`:
- Around line 160-167: Replace the brittle source-text assertions in the “does
not rescan every committed object for each capacity decision” test with a
behavioral test: commit multiple chunks using injected statfs/readdir spies,
then assert cache scanning remains bounded or constant rather than counting
scanCacheEntries() occurrences. Retain coverage that capacity-generation.json is
used through observable behavior, not source-text matching.
- Around line 640-693: Replace the fixed Bun.sleep(45) timing in the “keeps the
kernel capacity lock for the complete slow operation” test with an explicit
synchronization gate: start the second maintain only after deletionStarted
confirms deletion has begun, and keep deletion blocked until the concurrent
maintenance has reached the intended contention point. Remove the wall-clock
dependency while preserving the assertion that exactly one chunk is evicted.
In `@src/materializationChunkCache.ts`:
- Around line 1393-1406: In the materialization flow around the metadata write
and index.set call, sample dependencies.now() once into a shared timestamp
before constructing either record. Reuse that timestamp for both the persisted
MetadataRecord and the in-memory index entry so lastAccessedAt remains
identical.
In `@src/materializationServer.ts`:
- Around line 512-567: The consecutive-stale-renewal counter in renewLease is
unreachable because armLeaseDeadline aborts before the stale-response branch can
run. Remove consecutiveStaleRenewals and maximumConsecutiveStaleRenewals, along
with the related stale-expiry handling, while preserving validation, lease
deadline arming, and abort behavior for expired leases.
In `@src/materializationService.ts`:
- Around line 1990-1996: Reuse the long-lived chunk cache created by
startMaterializationServer by threading that instance into
materializeCapabilityJob and using it instead of constructing a new cache for
each job. Preserve the existing createMaterializationChunkCache construction as
a fallback when no cache is supplied, including its current failure
classification, so tests and standalone callers continue to work.
- Around line 1730-1732: Update the sort comparator for uniqueChunks in the
materialization flow to use the existing UTF-8 byte-order comparison utility,
compareNormalizedPathsUtf8 or the established equivalent, instead of
left.id.localeCompare(right.id). Preserve sorting by chunk ID and ensure the
order remains deterministic across hosts.
In `@src/sandboxContainer.test.ts`:
- Around line 1-50: Add a regression test in the “sandbox container cleanup”
suite for a successful forced removal whose process exits with code 0 but whose
stderr stream rejects or exceeds the supported read limit. Assert cleanup
resolves successfully, covering the stderr-read ordering behavior in
removeSandboxContainer.
In `@src/sandboxMounts.ts`:
- Around line 151-157: Update the EEXIST comparison in the sandbox staging flow
around sourcePath and destinationPath to avoid loading both files with readFile;
compare their contents using streaming hashes or an equivalent chunked
stream-based approach, and retain the existing mismatch error behavior.
- Around line 15-31: Consolidate exposeReadonlyEntry,
restorePrivateSandboxEntry, and exposeWritableEntry around one shared recursive
walker that performs lstat, rejects symbolic links, recurses through
directories, and applies caller-specified file and directory permissions.
Preserve each existing function’s permission values and ENOENT tolerance by
passing those behaviors into the shared helper, while retaining the current
errors and security checks.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8675b81-eade-4c44-a392-1c5e4a93eb66
⛔ Files ignored due to path filters (18)
bun.lockis excluded by!**/*.lockyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/out/copilot-probe/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/copilot-probe/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.sois excluded by!**/*.soyucp_coupling/out/linux-x64/Release/yucp_coupling.sois excluded by!**/*.soyucp_coupling/out/win-x64/Release/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/win-x64/Release/runtime-helper/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/out/win-x64/Release/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/yucp_coupling.exports.mapis excluded by!**/*.map
📒 Files selected for processing (229)
.env.exampledeploy/initialize-local-master-credential.shdeploy/initialize-local-master-credential.test.shdeploy/prepare-local-wsl-materializer.shdeploy/run-local-wsl-materializer.shdeploy/run-local-wsl-materializer.test.tsdeploy/yucp-materializer.servicepackage.jsonsrc/attestation.native.test.tssrc/attestation.test.tssrc/attestation.tssrc/attribution.test.tssrc/attribution.tssrc/boundedJson.test.tssrc/boundedJson.tssrc/codecLimits.test.tssrc/codecLimits.tssrc/config.tssrc/couplingSeed.test.tssrc/couplingSeed.tssrc/dpopSigner.test.tssrc/dpopSigner.tssrc/env.test.tssrc/env.tssrc/ffi.test.tssrc/ffi.tssrc/keyBroker.test.tssrc/keyBroker.tssrc/linuxAttributionWorker.pysrc/linuxCodecSandbox.test.tssrc/linuxCodecSandbox.tssrc/linuxCodecWorker.pysrc/linuxRendition.realtest.tssrc/linuxTreeSandbox.test.tssrc/linuxTreeSandbox.tssrc/linuxTreeWorker.pysrc/linuxWorkers.test.tssrc/linuxWorkers_test.pysrc/materializationBuilds.test.tssrc/materializationBuilds.tssrc/materializationChunkCache.test.tssrc/materializationChunkCache.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/materialize.test.tssrc/materialize.tssrc/nativeExports.test.tssrc/nativeHardening.test.tssrc/nativePlatform.test.tssrc/nativePlatform.tssrc/publicAuthority.tssrc/rendition.test.tssrc/rendition.tssrc/roundtrip.test.tssrc/runtimeArtifacts.test.tssrc/runtimeArtifacts.tssrc/runtimeTokens.test.tssrc/runtimeTokens.tssrc/sandboxContainer.test.tssrc/sandboxContainer.tssrc/sandboxImage.test.tssrc/sandboxImage.tssrc/sandboxIsolation.test.tssrc/sandboxIsolation.tssrc/sandboxMounts.test.tssrc/sandboxMounts.tssrc/server.nativeAvailability.test.tssrc/server.tssrc/serverArchitecture.test.tssrc/yucpTrust.tssrc/zipArchive.test.tssrc/zipArchive.tsyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/core.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aegis128l.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aegis256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aes256gcm.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_chacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha512256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box_curve25519xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box_curve25519xsalsa20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_hchacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_hsalsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_ristretto255.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa2012.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa208.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_generichash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_generichash_blake2b.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash_sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash_sha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_blake2b.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_hkdf_sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_hkdf_sha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kx.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_onetimeauth.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_onetimeauth_poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_argon2i.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_argon2id.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_scryptsalsa208sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_curve25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_ristretto255.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox_xsalsa20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretstream_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_shorthash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_shorthash_siphash24.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign_edwards25519sha512batch.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_chacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa2012.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa208.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_xchacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_xsalsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_16.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_32.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_64.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/export.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes_internal_random.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes_sysrandom.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/runtime.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/utils.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/version.hyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/static/libsodium.pdbyucp_coupling/_deps/minhook/include/MinHook.hyucp_coupling/_deps/minhook/src/buffer.cyucp_coupling/_deps/minhook/src/buffer.hyucp_coupling/_deps/minhook/src/hde/hde32.cyucp_coupling/_deps/minhook/src/hde/hde32.hyucp_coupling/_deps/minhook/src/hde/hde64.cyucp_coupling/_deps/minhook/src/hde/hde64.hyucp_coupling/_deps/minhook/src/hde/pstdint.hyucp_coupling/_deps/minhook/src/hde/table32.hyucp_coupling/_deps/minhook/src/hde/table64.hyucp_coupling/_deps/minhook/src/hook.cyucp_coupling/_deps/minhook/src/trampoline.cyucp_coupling/_deps/minhook/src/trampoline.hyucp_coupling/build.ps1yucp_coupling/build.shyucp_coupling/coupling_runtime.cyucp_coupling/coupling_runtime.expyucp_coupling/coupling_runtime.libyucp_coupling/guard.cyucp_coupling/out/copilot-probe/yucp_coupling.compile.pdbyucp_coupling/out/copilot-probe/yucp_coupling.pdbyucp_coupling/out/copilot-probe/yucp_coupling.sha256yucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.sha256yucp_coupling/out/linux-x64/Release/yucp_coupling.sha256yucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.compile.pdbyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.pdbyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.sha256yucp_coupling/out/win-x64/Release/yucp_coupling.compile.pdbyucp_coupling/out/win-x64/Release/yucp_coupling.pdbyucp_coupling/out/win-x64/Release/yucp_coupling.sha256yucp_coupling/yucp_coupling.defyucp_coupling/yucp_coupling.runtime-helper.def
💤 Files with no reviewable changes (18)
- src/nativePlatform.test.ts
- src/nativeExports.test.ts
- src/env.ts
- src/attestation.test.ts
- src/attestation.native.test.ts
- src/ffi.test.ts
- src/publicAuthority.ts
- src/nativePlatform.ts
- src/runtimeArtifacts.test.ts
- src/ffi.ts
- src/materialize.test.ts
- src/couplingSeed.test.ts
- src/materialize.ts
- src/roundtrip.test.ts
- src/couplingSeed.ts
- src/attestation.ts
- src/config.ts
- src/runtimeArtifacts.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013f149113
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/linuxCodecWorker.py (1)
271-315: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPeak memory/scratch usage per nested entry is ~2× the 100 MiB entry cap.
For one entry the worker holds
data(up toMAX_NESTED_ZIP_ENTRY_BYTES= 100 MiB) pluscoupled(another full copy) in RSS, and simultaneously stagesentry_inputandentry_outputon/tmp.entry_inputis written only to becopyfile'd toentry_outputand then deleted — it is pure overhead. In a memory- and tmpfs-capped codec sandbox this is the difference between succeeding and an OOM/ENOSPCon an otherwise valid archive.Writing
datastraight toentry_outputhalves the scratch footprint and removes a full 100 MiB copy.🛡️ Proposed fix
if asset_type is not None: - entry_input = os.path.join( - scratch, - f"{index:05d}.source.{asset_type}", - ) entry_output = os.path.join( scratch, f"{index:05d}.coupled.{asset_type}", ) try: - with open(entry_input, "wb") as entry_file: + with open(entry_output, "wb") as entry_file: entry_file.write(data) - shutil.copyfile(entry_input, entry_output) decoded_token = couple_path( runtime, asset_type, entry_output, token_hex, file_key, ) if decoded_token is not None: with open(entry_output, "rb") as entry_file: coupled = entry_file.read() total_bytes = total_bytes - len(data) + len(coupled) if total_bytes > MAX_NESTED_ZIP_TOTAL_BYTES: raise ValueError( "nested ZIP exceeds its expanded size limit" ) data = coupled coupled_entries += 1 else: uncarryable_visual_entries += 1 finally: - for staged_path in (entry_input, entry_output): - try: - os.remove(staged_path) - except FileNotFoundError: - pass + try: + os.remove(entry_output) + except FileNotFoundError: + pass🤖 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/linuxCodecWorker.py` around lines 271 - 315, Remove the redundant entry_input staging from the nested-entry processing around couple_path: write data directly to entry_output, pass that path to couple_path, and retain the existing coupled readback and cleanup behavior. Update the finally block to remove only entry_output while preserving archive output and size-limit handling.src/attribution.ts (1)
300-303: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCleanup rejection in
finallystill masks the original failure.
removeSandboxContainercan reject (docker daemon unreachable — likely exactly when the run already failed), replacing the real attribution error. Swallow it.🛡️ Proposed fix
try { if (containerName) { - await removeSandboxContainer({ containerName }); + await removeSandboxContainer({ containerName }).catch(() => undefined); } } finally {🤖 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/attribution.ts` around lines 300 - 303, Update the cleanup flow in the surrounding try/finally block so a rejection from removeSandboxContainer does not escape or replace the original attribution error. Catch and swallow cleanup failures while preserving the existing cleanup attempt and the primary error outcome.src/linuxCodecSandbox.ts (1)
431-439: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winContainer cleanup failure still replaces the classified codec failure.
await removeContainer(...)insidecatchpropagates its own rejection, discardingcauseand theLinuxCodecErrorCode.🛡️ Proposed fix
} catch (cause) { - await removeContainer(input.request.containerName); + await removeContainer(input.request.containerName).catch(() => undefined); if (cause instanceof LinuxCodecFailure) {🤖 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/linuxCodecSandbox.ts` around lines 431 - 439, Update the catch block around the sandbox operation to ensure removeContainer failure cannot replace the original cause or its LinuxCodecFailure classification. Isolate and safely handle cleanup errors around removeContainer, then preserve the existing LinuxCodecFailure rethrow and wrapping behavior for the original cause.
🧹 Nitpick comments (26)
src/attribution.test.ts (2)
14-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the PNG fixture helpers into a shared test module.
crc32,pngChunk,makeGrayPng, andsha256Hexare byte-for-byte duplicates of the versions insrc/rendition.test.ts. A singlesrc/testPngFixtures.ts(or similar) keeps the two suites from drifting on fixture format.Also applies to: 156-183
🤖 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/attribution.test.ts` around lines 14 - 32, Extract the duplicated PNG fixture helpers crc32, pngChunk, makeGrayPng, and sha256Hex from attribution.test.ts and rendition.test.ts into a shared test module such as testPngFixtures.ts. Export and reuse those shared helpers in both test suites, removing their local duplicate implementations while preserving byte-for-byte fixture behavior.
34-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text assertions are brittle guardrails.
These tests read
attribution.tsand match implementation substrings, so they break on any harmless rename/reformat while proving nothing about runtime behavior. The bounded-IO and container-cleanup contracts are better asserted by injecting a spawn stub and observing the argv/cleanup calls, assandboxContainer.test.tsalready does.🤖 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/attribution.test.ts` around lines 34 - 55, Replace the source-text assertions in the attribution worker tests with runtime tests that inject a spawn stub, invoke the worker behavior, and observe bounded input/output handling plus container naming and cleanup. Follow the dependency-injection and call-observation pattern used by sandboxContainer.test.ts, asserting the resulting argv and removeSandboxContainer call rather than matching implementation substrings.src/rendition.test.ts (1)
95-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the digest-mismatch branch too.
Only the byte-count mismatch is asserted. The SHA-256 comparison is the part that actually detects tampered output, and it's currently unexercised.
💚 Suggested addition
await expect( verifyRenditionOutputFile({ expectedBytes: bytes.byteLength + 1, expectedSha256: sha256Hex(bytes), outputPath, }) ).rejects.toThrow('length did not match'); + await expect( + verifyRenditionOutputFile({ + expectedBytes: bytes.byteLength, + expectedSha256: 'ff'.repeat(32), + outputPath, + }) + ).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 `@src/rendition.test.ts` around lines 95 - 118, Extend the test around verifyRenditionOutputFile to also assert rejection when expectedSha256 differs from the file’s actual sha256Hex, while keeping expectedBytes equal to bytes.byteLength. Verify the rejection matches the digest-mismatch error message.src/sandboxContainer.test.ts (1)
4-6: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the input validation guards.
removeSandboxContainerrejects malformed container names and out-of-rangecleanupTimeoutMs(the name regex is what keeps attacker-controlled strings out of thedocker rmargv), but no test exercises either path.♻️ Suggested additional cases
it('rejects a malformed container name before spawning', async () => { let spawned = false; await expect( removeSandboxContainer({ containerName: 'yucp-codec-0123; rm -rf /', spawn: () => { spawned = true; return { exited: Promise.resolve(0) }; }, }) ).rejects.toThrow('invalid'); expect(spawned).toBeFalse(); }); it('rejects an out-of-range cleanup timeout', async () => { await expect( removeSandboxContainer({ cleanupTimeoutMs: 60_000, containerName, spawn: () => ({ exited: Promise.resolve(0) }), }) ).rejects.toThrow('timeout is invalid'); });🤖 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/sandboxContainer.test.ts` around lines 4 - 6, Add tests in the “sandbox container cleanup” suite covering both validation guards in removeSandboxContainer: verify a malformed containerName rejects with an invalid-name error and does not invoke spawn, and verify an out-of-range cleanupTimeoutMs rejects with the timeout validation error. Reuse the existing containerName fixture and preserve current valid cleanup coverage.src/materializationBuilds.test.ts (2)
233-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert both slice markers were found.
A
-1from eitherindexOfyields a bogus slice, so the symlink-boundary guard could pass without inspectingrequiredFileEntry.💚 Proposed fix
- const boundary = source.slice( - source.indexOf('async function requiredFileEntry'), - source.indexOf('export async function computeMaterializationBuilds') - ); + const start = source.indexOf('async function requiredFileEntry'); + const end = source.indexOf( + 'export async function computeMaterializationBuilds' + ); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + const boundary = source.slice(start, 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 `@src/materializationBuilds.test.ts` around lines 233 - 244, Update the test case around the requiredFileEntry boundary to assert that both source.indexOf markers return valid positions before slicing. Ensure the test fails when either marker is missing, while preserving the existing lstat and details.isSymbolicLink checks on the extracted boundary.
53-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the optional cast; the same export is called directly at Line 122.
The
as unknown as { pin...?: ... }shim plus existence guard adds noise and would mask a missing export as a pass-throughreturn. Use the typed module export consistently.♻️ Proposed simplification
- const pinMaterializationBuildArtifacts = ( - materializationBuildsModule as unknown as { - pinMaterializationBuildArtifacts?: (input: {...}) => Promise<{...}>; - } - ).pinMaterializationBuildArtifacts; - expect(pinMaterializationBuildArtifacts).toBeFunction(); - if (!pinMaterializationBuildArtifacts) { - return; - } - const pinned = await pinMaterializationBuildArtifacts({ + const pinned = + await materializationBuildsModule.pinMaterializationBuildArtifacts({🤖 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/materializationBuilds.test.ts` around lines 53 - 74, Update the materializationBuilds test to use the typed pinMaterializationBuildArtifacts module export directly, matching the invocation later in the test. Remove the unknown optional cast, function-existence assertion, and early return so a missing export fails the test rather than being silently skipped.src/linuxCodecSandbox.test.ts (1)
359-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-slicing test can silently degrade if either marker moves.
If
indexOfreturns-1(rename/reorder), the slice covers an unintended region and thelstatguard is no longer actually verified. Assert both offsets first.💚 Proposed fix
- const validator = source.slice( - source.indexOf('export async function validateLinuxCodecOutputLengths'), - source.indexOf('export async function runLinuxCodecSandbox') - ); + const start = source.indexOf( + 'export async function validateLinuxCodecOutputLengths' + ); + const end = source.indexOf('export async function runLinuxCodecSandbox'); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + const validator = source.slice(start, 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 `@src/linuxCodecSandbox.test.ts` around lines 359 - 370, Update the source-slicing test around the validator marker lookups to store both indexOf results and assert each is non-negative before calling slice. Keep the existing lstat/stat assertions unchanged so the test fails explicitly if either validateLinuxCodecOutputLengths or runLinuxCodecSandbox marker moves.src/sandboxMounts.ts (1)
131-163: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAtomic publish via
link()looks right; the dedup comparison is the remaining cost.Two full
readFilecalls buffer both copies in memory on everyEEXIST, which scales with worker/runtime artifact size and multiplies under concurrent stagers (seesandboxMounts.test.tsstaging-race test). Compare sizes first and fall back to a streaming hash.♻️ Proposed change
const destination = await lstat(destinationPath); if (!destination.isFile() || destination.isSymbolicLink()) { throw new Error('The staged sandbox file is invalid'); } - const [sourceBytes, destinationBytes] = await Promise.all([ - readFile(input.sourcePath), - readFile(destinationPath), - ]); - if (!sourceBytes.equals(destinationBytes)) { + if (destination.size !== source.size) { + throw new Error('The staged sandbox file does not match its source'); + } + const [sourceDigest, destinationDigest] = await Promise.all([ + fileDigest(input.sourcePath), + fileDigest(destinationPath), + ]); + if (sourceDigest !== destinationDigest) { throw new Error('The staged sandbox file does not match its source'); }with a small helper:
import { createHash } from 'node:crypto'; import { createReadStream } from 'node:fs'; import { pipeline } from 'node:stream/promises'; async function fileDigest(filePath: string): Promise<string> { const hash = createHash('sha256'); await pipeline(createReadStream(filePath), hash); return hash.digest('hex'); }🤖 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/sandboxMounts.ts` around lines 131 - 163, Replace the full-buffer read comparison in the EEXIST handling of the sandbox staging function with a size-first check, then compare matching files using streaming SHA-256 digests. Add and use a small fileDigest helper based on createReadStream, createHash, and pipeline; preserve the existing invalid-destination and mismatch errors while avoiding simultaneous whole-file buffering.src/sandboxMounts.test.ts (1)
97-125: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win64 MiB × 8 concurrent stagers makes this test memory-hungry.
Each
EEXISTloser instageReadonlySandboxFilereads both source and destination fully into memory (readFile×2), so seven losers can hold ~900 MiB transiently plus ~512 MiB of temp copies on disk. A few MiB is enough to exercise the race window without risking OOM on constrained CI runners.♻️ Proposed change
- const bytes = Buffer.alloc(64 * 1024 * 1024, 0x5a); + const bytes = Buffer.alloc(4 * 1024 * 1024, 0x5a);🤖 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/sandboxMounts.test.ts` around lines 97 - 125, Reduce the test fixture size in the concurrent stageReadonlySandboxFile test while preserving the eight-way race and completeness assertions; use a payload of only a few MiB instead of 64 MiB so loser retries do not create excessive memory or disk usage.src/materializationChunkCache.test.ts (2)
653-706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWall-clock sleeps make this lock test timing-sensitive.
The 90 ms injected deletion plus the 45 ms gap before the second
maintain()assumes the slow eviction is still in flight; a loaded CI runner can invert that ordering and flipcommittedObjects. A deterministic gate (resolve the deletion only after the secondmaintain()has been started) would remove the flake window.🤖 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/materializationChunkCache.test.ts` around lines 653 - 706, Make the lock test deterministic by replacing the wall-clock sleeps in “keeps the kernel capacity lock for the complete slow operation” with a synchronization gate: have the injected removeObject deletion pause on a promise, start the concurrent maintain() while that deletion is blocked, then resolve the gate and await both maintenance operations. Preserve the existing eviction and committedObjects assertions.
160-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two tests assert on the implementation's source text, not behavior.
toContain('withInProcessFileLock(lockPath, (signal) =>')and thescanCacheEntries()occurrence count break on any harmless reformat/rename while proving nothing about runtime semantics. The behavioral equivalents already exist elsewhere in this file (serializes capacity work through one kernel-held lock,keeps the kernel capacity lock for the complete slow operation). Consider dropping these or replacing them with an assertion on observed lock/scan counts via injected dependencies.🤖 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/materializationChunkCache.test.ts` around lines 160 - 180, Replace the source-text assertions in the two tests around Linux lock acquisition and cache scanning with behavioral tests. Reuse the existing lock/scan instrumentation or injected dependencies to observe actual lock serialization and scan counts, matching the behaviors covered by “serializes capacity work through one kernel-held lock” and “keeps the kernel capacity lock for the complete slow operation”; remove brittle source-string and occurrence-count checks.src/materializationService.test.ts (1)
255-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a behavioral NFC-collision test over a source-text match.
This asserts the literal expression rather than the guarantee. A manifest containing both NFC and NFD spellings of the same path, expected to be rejected, would keep passing across refactors and actually prove the contract.
🤖 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/materializationService.test.ts` around lines 255 - 263, Replace the source-text assertion in the “normalizes source-manifest collision keys before case folding” test with a behavioral case: construct a manifest containing NFC and NFD spellings of the same path, invoke the relevant materialization flow, and assert that the collision is rejected. Preserve the test’s focus on normalization before case folding and avoid matching implementation source text.src/linuxCodecWorker.py (1)
256-259: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
.upper()case-folding can reject legitimate archives.Unicode uppercasing is not length-preserving (
ß→SS,fi→FI), so distinct entry names can fold to the samecollision_keyand trip the duplicate-path error.str.casefold()is the intended primitive for case-insensitive matching and avoids the expansion collisions thatupper()introduces.♻️ Proposed change
- collision_key = normalized.upper() + collision_key = normalized.casefold()🤖 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/linuxCodecWorker.py` around lines 256 - 259, Update the duplicate-path normalization in the ZIP validation flow to use normalized.casefold() instead of normalized.upper() when assigning collision_key. Keep the existing seen_paths membership check, ValueError, and set insertion unchanged.src/serverArchitecture.test.ts (1)
153-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelf-referential assertion can never fail.
The test reads its own source and asserts it contains
new Bun.Glob('**/*.ts'), which is trivially true by construction and guards nothing. TheserverSourceassertion below is the only meaningful check here.♻️ Proposed cleanup
- const architectureTestSource = readFileSync( - path.join(repoRoot, 'src', 'serverArchitecture.test.ts'), - 'utf8' - ); - - expect(architectureTestSource).toContain("new Bun.Glob('**/*.ts')"); expect(serverSource).toContain( "readTrimmedEnv(env, 'MATERIALIZATION_HEALTH_HOST') ?? '127.0.0.1'" );🤖 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/serverArchitecture.test.ts` around lines 153 - 167, Remove the self-referential architectureTestSource read and its assertion for `new Bun.Glob('**/*.ts')` from the test `uses recursive source scanning and a loopback health default`. Keep the meaningful `serverSource` assertion verifying the `MATERIALIZATION_HEALTH_HOST` loopback default.src/materializationBuilds.ts (1)
207-241: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueStartup prune deletes every
instance-*directory, including one another process is actively using.
pruneObsoletePinnedArtifactsruns before this process creates its own directory, so it only stays safe while exactly one materializer perMATERIALIZATION_WORK_ROOTcan run. The systemd unit'sflock --no-fork --nonblockenforces that on the deployed path, but any manual/second start against the same work root would pull the pinned workers andyucp_coupling.soout from under a live instance (files are already open-read at0o400, but subsequentlstat/CDLLloads would fail).Consider skipping directories whose per-instance lock is still held, or documenting the single-instance requirement next to
workRoot.🤖 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/materializationBuilds.ts` around lines 207 - 241, The startup cleanup in pruneObsoletePinnedArtifacts must not remove an instance-* directory that another materializer is actively using. Add or reuse a per-instance lock for each pinned directory, and prune only directories whose lock can be acquired; preserve cleanup of stale unlocked directories and ensure pinMaterializationBuildArtifacts establishes the lock before the directory becomes eligible for use.src/boundedJson.test.ts (1)
5-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the response was actually left unconsumed.
The test name claims the limit is rejected "before consuming the response", but only the error message is checked — the same assertion would pass if the body had been fully read first.
♻️ Proposed strengthening
async (maximumBytes) => { + const response = Response.json({ success: true }); await expect( - readBoundedJsonResponse(Response.json({ success: true }), { + readBoundedJsonResponse(response, { errorPrefix: 'Control-plane response', maximumBytes, }) ).rejects.toThrow('maximum byte limit is invalid'); + expect(response.bodyUsed).toBe(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 `@src/boundedJson.test.ts` around lines 5 - 15, Strengthen the parameterized test for readBoundedJsonResponse by asserting that the response body remains unconsumed after the invalid maximumBytes rejection. Keep the existing error assertion and verify the Response body’s unread state, such as its bodyUsed property, remains false.src/linuxTreeSandbox.ts (1)
174-190: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOnly the stdin path yields
LinuxTreeSandboxFailure; output and exit failures escape as plainError.
writeLinuxCodecProcessInputfailures are wrapped at Line 172, butcollectLinuxTreeProcessOutputrejections, the nonzero-exit error, andJSON.parsesyntax errors propagate unwrapped — so callers get three different shapes anderrorCode: 'MATERIALIZATION_TREE_SANDBOX_FAILED'is present only for one of them. Wrap the whole worker interaction uniformly.♻️ Proposed change
} catch (cause) { await removeSandboxContainer({ containerName }).catch(() => undefined); - throw cause; + throw cause instanceof LinuxTreeSandboxFailure + ? cause + : new LinuxTreeSandboxFailure(cause); }🤖 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/linuxTreeSandbox.ts` around lines 174 - 190, Update the worker interaction surrounding collectLinuxTreeProcessOutput, the nonzero-exit check, and JSON.parse so all failures are wrapped in the existing LinuxTreeSandboxFailure shape with errorCode MATERIALIZATION_TREE_SANDBOX_FAILED, matching writeLinuxCodecProcessInput failures. Preserve sandbox cleanup and the original cause while ensuring callers receive the same failure type for stdin, output, exit, and parse errors.src/linuxTreeSandbox.test.ts (1)
81-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-text assertions are brittle proxies for behavior.
These three tests match raw
linuxTreeSandbox.tstext, including exact spacing and occurrence counts. Any reformat or benign rename breaks them, and none of them proves the runtime property they describe. The last one in particular is testable directly:runWorker's cleanup path can be exercised by injecting a failing worker plus a rejecting container removal and asserting the original cause propagates.🤖 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/linuxTreeSandbox.test.ts` around lines 81 - 111, Replace the three raw source-text assertions in the tests with behavior-focused tests: exercise the guarded process-input path and verify its failure behavior, validate the shared sandbox work-directory behavior through the relevant exported API, and invoke runWorker with a failing worker plus rejecting container cleanup to assert the original worker error propagates. Avoid exact source formatting, occurrence counts, and implementation-text matching.deploy/run-local-wsl-materializer.sh (1)
125-134: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRequire Bash 5.1+ for
wait -n -p
wait -n -pneeds Bash 5.1, so this will fail on older WSL distros before the supervision loop starts. Add a version check up front if those environments need to be supported.♻️ Optional guard
set -euo pipefail + +if (( BASH_VERSINFO[0] < 5 || (BASH_VERSINFO[0] == 5 && BASH_VERSINFO[1] < 1) )); then + echo "Bash 5.1 or newer is required." >&2 + exit 78 +fi🤖 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 `@deploy/run-local-wsl-materializer.sh` around lines 125 - 134, Add an upfront Bash version check before the supervision logic using wait -n -p, and exit with a clear error when the running Bash is older than 5.1. Keep the existing process-waiting behavior unchanged for supported Bash versions.src/linuxWorkers.test.ts (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHard-coded test count makes this wrapper break whenever a Python test is added.
linuxWorkers_test.pycurrently has exactly 10 tests, so this passes today, but the assertion carries no signal beyond "discovery found something" and forces an unrelated edit on every new case. Assert a non-zero count plusOKinstead.♻️ Proposed refactor
- expect(`${stdout}${stderr}`).toContain('Ran 10 tests'); + const output = `${stdout}${stderr}`; + expect(output).toMatch(/Ran [1-9]\d* tests/); + expect(output).toContain('OK'); expect(exitCode).toBe(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 `@src/linuxWorkers.test.ts` at line 34, Replace the hard-coded “Ran 10 tests” assertion in the linuxWorkers test wrapper with assertions that the combined stdout/stderr reports a non-zero test count and contains “OK”.src/linuxTreeWorker.py (2)
98-113: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the destination file when the digest check fails.
copy_verifiedwrites the full copy before comparing digests, so a mismatch leaves a partially-trusted file atdestination. The job aborts today, but the file lingers in/inputfor the rest of the container's lifetime.♻️ Proposed refactor
if digest.hexdigest() != expected_digest: + os.unlink(destination) raise ValueError("protected source digest did not match")🤖 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/linuxTreeWorker.py` around lines 98 - 113, Update copy_verified so a digest mismatch removes the newly written destination file before propagating the ValueError. Keep the successful-copy return path unchanged, and ensure cleanup targets only the destination created by this invocation.
62-69: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReject non-object request bodies explicitly.
json.loadshappily returns a list, string, or number, andrequest.get(...)then raisesAttributeError, which the top-level handler reports as an opaqueAttributeErrorinstead of the intendedValueError("unsupported schema")contract.♻️ Proposed refactor
request = json.loads(raw) + if not isinstance(request, dict): + raise ValueError("invalid request") if request.get("schemaVersion") != 3:🤖 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/linuxTreeWorker.py` around lines 62 - 69, Update read_request to validate that the json.loads result is an object/dictionary before calling request.get; reject lists, strings, numbers, and other non-object bodies with ValueError("unsupported schema"), while preserving the existing schemaVersion validation for object requests.src/linuxRendition.realtest.ts (2)
115-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the repeated
runtimePathresolution to module scope.The same nine-line
path.resolve(...)appears in all five tests. A single module-level constant (plus a sharedcreateJobInputhelper for the near-identicalmaterializePersonalizedZippayloads) would cut most of this file's bulk.♻️ Proposed refactor
+const runtimePath = path.resolve( + import.meta.dir, + '..', + 'yucp_coupling', + 'out', + 'linux-x64', + 'Release', + 'yucp_coupling.so' +); + describe('real Linux rendition sandbox', () => { test( 'creates different verified outputs for two buyers without returning key material', async () => { - const runtimePath = path.resolve( - import.meta.dir, - '..', - 'yucp_coupling', - 'out', - 'linux-x64', - 'Release', - 'yucp_coupling.so' - );Also applies to: 196-204, 255-263, 310-318, 365-373
🤖 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/linuxRendition.realtest.ts` around lines 115 - 123, Hoist the repeated path.resolve expression into a single module-level runtimePath constant in src/linuxRendition.realtest.ts, then reuse it in all five tests, including the locations around materializePersonalizedZip. Also extract the near-identical materializePersonalizedZip payload construction into a shared createJobInput helper and update each test to use it.
10-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
zlib.crc32here instead of the hand-rolled helper. This file already targets Bun, which exposes it vianode:zlib, so the built-in keeps the checksum code out of the test.🤖 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/linuxRendition.realtest.ts` around lines 10 - 19, Replace the hand-rolled crc32 helper with Bun’s built-in crc32 from node:zlib, updating its call sites to use the imported function and removing the local helper implementation.deploy/run-local-wsl-materializer.test.ts (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the script read into a shared helper.
The same
readFileis repeated in all three tests. A module-level lazy loader (orbeforeAll) removes the duplication and the repeated path literal.♻️ Proposed refactor
+const scriptPath = path.join(import.meta.dir, 'run-local-wsl-materializer.sh'); +let cached: string | undefined; +const loadScript = async () => (cached ??= await readFile(scriptPath, 'utf8')); + describe('local WSL materializer entry point', () => { test('uses the isolated runtime proxy ports instead of fixed shared ports', async () => { - const source = await readFile( - path.join(import.meta.dir, 'run-local-wsl-materializer.sh'), - 'utf8' - ); + const source = await loadScript();Also applies to: 24-28, 42-46
🤖 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 `@deploy/run-local-wsl-materializer.test.ts` around lines 6 - 10, Extract the repeated script-loading logic from the three tests into a shared module-level lazy helper or beforeAll setup. Reuse that helper in the tests, including the cases around the existing first test and the other two readFile calls, while keeping the same script path and UTF-8 decoding behavior.src/linuxWorkers_test.py (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fake
resourcemodule is inserted intosys.modulespermanently.Once any test loads an attribution/codec worker on a platform without
resource, every later import in the session resolves to this two-attribute stub. Scoping it to the load (or registering anaddCleanupfrom the caller) keeps the interpreter state honest.🤖 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/linuxWorkers_test.py` around lines 18 - 30, Update load_worker so the fallback resource module is only present for the duration of the worker import: track whether the stub was inserted, execute spec.loader.exec_module(module), then restore the prior sys.modules state by removing the stub afterward. Preserve an existing resource module unchanged and ensure cleanup occurs even if module loading raises.
🤖 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 `@deploy/yucp-materializer.service`:
- Line 15: Ensure the %h/.local/share/yucp-materializer directory exists before
systemd applies ReadWritePaths=, since ExecStartPre runs too late. Update the
unit configuration to use an appropriate StateDirectory= or deploy-time
creation, or mark the ReadWritePaths= entry optional with a “-” prefix if the
directory may be absent.
In `@src/linuxTreeWorker.py`:
- Line 210: Update the umask configuration in the worker initialization to use
0o077 instead of 0o000, ensuring files and directories created by the worker are
not world-accessible while preserving the existing creation flow.
In `@src/materializationChunkCache.ts`:
- Around line 1397-1410: Capture dependencies.now() once before writing the
metadata in the materialization flow, then reuse that timestamp for both the
persisted MetadataRecord and the index.set entry. Update the relevant code
surrounding metadataPath(chunkId) and index.set so both lastAccessedAt values
are identical.
In `@src/sandboxContainer.ts`:
- Around line 72-83: Update the failure handling around the stderr read in the
sandbox container cleanup flow so readBoundedUtf8Stream errors cannot replace
the cleanup failure message. Preserve the missing-container detection when
stderr is successfully read, but catch read failures and continue throwing the
status-based error that includes outcome.exitCode.
In `@src/sandboxIsolation.ts`:
- Around line 55-62: Update requireDockerBindSource to reject any colon that is
not the Windows drive-letter colon at index 1, while preserving valid Unix
absolute paths and Windows drive-prefixed paths. Keep the existing comma,
null-byte, and absolute-path validation intact.
---
Duplicate comments:
In `@src/attribution.ts`:
- Around line 300-303: Update the cleanup flow in the surrounding try/finally
block so a rejection from removeSandboxContainer does not escape or replace the
original attribution error. Catch and swallow cleanup failures while preserving
the existing cleanup attempt and the primary error outcome.
In `@src/linuxCodecSandbox.ts`:
- Around line 431-439: Update the catch block around the sandbox operation to
ensure removeContainer failure cannot replace the original cause or its
LinuxCodecFailure classification. Isolate and safely handle cleanup errors
around removeContainer, then preserve the existing LinuxCodecFailure rethrow and
wrapping behavior for the original cause.
In `@src/linuxCodecWorker.py`:
- Around line 271-315: Remove the redundant entry_input staging from the
nested-entry processing around couple_path: write data directly to entry_output,
pass that path to couple_path, and retain the existing coupled readback and
cleanup behavior. Update the finally block to remove only entry_output while
preserving archive output and size-limit handling.
---
Nitpick comments:
In `@deploy/run-local-wsl-materializer.sh`:
- Around line 125-134: Add an upfront Bash version check before the supervision
logic using wait -n -p, and exit with a clear error when the running Bash is
older than 5.1. Keep the existing process-waiting behavior unchanged for
supported Bash versions.
In `@deploy/run-local-wsl-materializer.test.ts`:
- Around line 6-10: Extract the repeated script-loading logic from the three
tests into a shared module-level lazy helper or beforeAll setup. Reuse that
helper in the tests, including the cases around the existing first test and the
other two readFile calls, while keeping the same script path and UTF-8 decoding
behavior.
In `@src/attribution.test.ts`:
- Around line 14-32: Extract the duplicated PNG fixture helpers crc32, pngChunk,
makeGrayPng, and sha256Hex from attribution.test.ts and rendition.test.ts into a
shared test module such as testPngFixtures.ts. Export and reuse those shared
helpers in both test suites, removing their local duplicate implementations
while preserving byte-for-byte fixture behavior.
- Around line 34-55: Replace the source-text assertions in the attribution
worker tests with runtime tests that inject a spawn stub, invoke the worker
behavior, and observe bounded input/output handling plus container naming and
cleanup. Follow the dependency-injection and call-observation pattern used by
sandboxContainer.test.ts, asserting the resulting argv and
removeSandboxContainer call rather than matching implementation substrings.
In `@src/boundedJson.test.ts`:
- Around line 5-15: Strengthen the parameterized test for
readBoundedJsonResponse by asserting that the response body remains unconsumed
after the invalid maximumBytes rejection. Keep the existing error assertion and
verify the Response body’s unread state, such as its bodyUsed property, remains
false.
In `@src/linuxCodecSandbox.test.ts`:
- Around line 359-370: Update the source-slicing test around the validator
marker lookups to store both indexOf results and assert each is non-negative
before calling slice. Keep the existing lstat/stat assertions unchanged so the
test fails explicitly if either validateLinuxCodecOutputLengths or
runLinuxCodecSandbox marker moves.
In `@src/linuxCodecWorker.py`:
- Around line 256-259: Update the duplicate-path normalization in the ZIP
validation flow to use normalized.casefold() instead of normalized.upper() when
assigning collision_key. Keep the existing seen_paths membership check,
ValueError, and set insertion unchanged.
In `@src/linuxRendition.realtest.ts`:
- Around line 115-123: Hoist the repeated path.resolve expression into a single
module-level runtimePath constant in src/linuxRendition.realtest.ts, then reuse
it in all five tests, including the locations around materializePersonalizedZip.
Also extract the near-identical materializePersonalizedZip payload construction
into a shared createJobInput helper and update each test to use it.
- Around line 10-19: Replace the hand-rolled crc32 helper with Bun’s built-in
crc32 from node:zlib, updating its call sites to use the imported function and
removing the local helper implementation.
In `@src/linuxTreeSandbox.test.ts`:
- Around line 81-111: Replace the three raw source-text assertions in the tests
with behavior-focused tests: exercise the guarded process-input path and verify
its failure behavior, validate the shared sandbox work-directory behavior
through the relevant exported API, and invoke runWorker with a failing worker
plus rejecting container cleanup to assert the original worker error propagates.
Avoid exact source formatting, occurrence counts, and implementation-text
matching.
In `@src/linuxTreeSandbox.ts`:
- Around line 174-190: Update the worker interaction surrounding
collectLinuxTreeProcessOutput, the nonzero-exit check, and JSON.parse so all
failures are wrapped in the existing LinuxTreeSandboxFailure shape with
errorCode MATERIALIZATION_TREE_SANDBOX_FAILED, matching
writeLinuxCodecProcessInput failures. Preserve sandbox cleanup and the original
cause while ensuring callers receive the same failure type for stdin, output,
exit, and parse errors.
In `@src/linuxTreeWorker.py`:
- Around line 98-113: Update copy_verified so a digest mismatch removes the
newly written destination file before propagating the ValueError. Keep the
successful-copy return path unchanged, and ensure cleanup targets only the
destination created by this invocation.
- Around line 62-69: Update read_request to validate that the json.loads result
is an object/dictionary before calling request.get; reject lists, strings,
numbers, and other non-object bodies with ValueError("unsupported schema"),
while preserving the existing schemaVersion validation for object requests.
In `@src/linuxWorkers_test.py`:
- Around line 18-30: Update load_worker so the fallback resource module is only
present for the duration of the worker import: track whether the stub was
inserted, execute spec.loader.exec_module(module), then restore the prior
sys.modules state by removing the stub afterward. Preserve an existing resource
module unchanged and ensure cleanup occurs even if module loading raises.
In `@src/linuxWorkers.test.ts`:
- Line 34: Replace the hard-coded “Ran 10 tests” assertion in the linuxWorkers
test wrapper with assertions that the combined stdout/stderr reports a non-zero
test count and contains “OK”.
In `@src/materializationBuilds.test.ts`:
- Around line 233-244: Update the test case around the requiredFileEntry
boundary to assert that both source.indexOf markers return valid positions
before slicing. Ensure the test fails when either marker is missing, while
preserving the existing lstat and details.isSymbolicLink checks on the extracted
boundary.
- Around line 53-74: Update the materializationBuilds test to use the typed
pinMaterializationBuildArtifacts module export directly, matching the invocation
later in the test. Remove the unknown optional cast, function-existence
assertion, and early return so a missing export fails the test rather than being
silently skipped.
In `@src/materializationBuilds.ts`:
- Around line 207-241: The startup cleanup in pruneObsoletePinnedArtifacts must
not remove an instance-* directory that another materializer is actively using.
Add or reuse a per-instance lock for each pinned directory, and prune only
directories whose lock can be acquired; preserve cleanup of stale unlocked
directories and ensure pinMaterializationBuildArtifacts establishes the lock
before the directory becomes eligible for use.
In `@src/materializationChunkCache.test.ts`:
- Around line 653-706: Make the lock test deterministic by replacing the
wall-clock sleeps in “keeps the kernel capacity lock for the complete slow
operation” with a synchronization gate: have the injected removeObject deletion
pause on a promise, start the concurrent maintain() while that deletion is
blocked, then resolve the gate and await both maintenance operations. Preserve
the existing eviction and committedObjects assertions.
- Around line 160-180: Replace the source-text assertions in the two tests
around Linux lock acquisition and cache scanning with behavioral tests. Reuse
the existing lock/scan instrumentation or injected dependencies to observe
actual lock serialization and scan counts, matching the behaviors covered by
“serializes capacity work through one kernel-held lock” and “keeps the kernel
capacity lock for the complete slow operation”; remove brittle source-string and
occurrence-count checks.
In `@src/materializationService.test.ts`:
- Around line 255-263: Replace the source-text assertion in the “normalizes
source-manifest collision keys before case folding” test with a behavioral case:
construct a manifest containing NFC and NFD spellings of the same path, invoke
the relevant materialization flow, and assert that the collision is rejected.
Preserve the test’s focus on normalization before case folding and avoid
matching implementation source text.
In `@src/rendition.test.ts`:
- Around line 95-118: Extend the test around verifyRenditionOutputFile to also
assert rejection when expectedSha256 differs from the file’s actual sha256Hex,
while keeping expectedBytes equal to bytes.byteLength. Verify the rejection
matches the digest-mismatch error message.
In `@src/sandboxContainer.test.ts`:
- Around line 4-6: Add tests in the “sandbox container cleanup” suite covering
both validation guards in removeSandboxContainer: verify a malformed
containerName rejects with an invalid-name error and does not invoke spawn, and
verify an out-of-range cleanupTimeoutMs rejects with the timeout validation
error. Reuse the existing containerName fixture and preserve current valid
cleanup coverage.
In `@src/sandboxMounts.test.ts`:
- Around line 97-125: Reduce the test fixture size in the concurrent
stageReadonlySandboxFile test while preserving the eight-way race and
completeness assertions; use a payload of only a few MiB instead of 64 MiB so
loser retries do not create excessive memory or disk usage.
In `@src/sandboxMounts.ts`:
- Around line 131-163: Replace the full-buffer read comparison in the EEXIST
handling of the sandbox staging function with a size-first check, then compare
matching files using streaming SHA-256 digests. Add and use a small fileDigest
helper based on createReadStream, createHash, and pipeline; preserve the
existing invalid-destination and mismatch errors while avoiding simultaneous
whole-file buffering.
In `@src/serverArchitecture.test.ts`:
- Around line 153-167: Remove the self-referential architectureTestSource read
and its assertion for `new Bun.Glob('**/*.ts')` from the test `uses recursive
source scanning and a loopback health default`. Keep the meaningful
`serverSource` assertion verifying the `MATERIALIZATION_HEALTH_HOST` loopback
default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai full review @codex review |
|
✅ Action performedFull review finished. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
src/nativeHardening.test.ts (1)
93-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegex leaves
xg_0115–xg_0121unchecked.
0[0-9a-f]|1[0-4]stops atxg_0114, so the seven identifiers just below the server range would slip through. The exact-export assertion at Line 269 is a backstop, but a non-exported leftover helper would not be caught.♻️ Widen the range
- expect(guardSource).not.toMatch(/\bxg_01(?:0[0-9a-f]|1[0-4])\b/); + expect(guardSource).not.toMatch(/\bxg_01(?:0[0-9a-f]|1[0-9a-f]|2[01])\b/);🤖 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/nativeHardening.test.ts` at line 93, Widen the forbidden-identifier regex in the guardSource assertion so it also matches xg_0115 through xg_0121, while preserving the existing coverage for xg_0100 through xg_0114.src/materializationChunkCache.ts (1)
1146-1163: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftEvery verified read bumps the global capacity generation, forcing other cache instances to full-rescan.
beginCacheIndexMutationpublishesdirty: truewithgeneration + 1andfinishCacheIndexMutationpublishes it again — two fsync'd atomic writes plus directory syncs per chunk read. Worse, any other cache instance sharingworkRootsees a new generation and re-runsscanCacheEntries()(stat + metadata read per object) on its next operation. At 256 KiB chunks and multi-GiB packages this is the hot path.Consider only bumping the generation when the index membership actually changes (add/evict), and publishing
lastAccessedAtrefreshes as metadata-only writes that consumers merge without invalidating the whole index.🤖 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/materializationChunkCache.ts` around lines 1146 - 1163, Update the verified-read path around beginCacheIndexMutation, index.set, and finishCacheIndexMutation so refreshing lastAccessedAt does not bump the global generation or trigger full rescans. Keep generation changes and mutation publication for actual index membership changes such as add or evict, and persist/read access-time metadata through a metadata-only path that other cache instances can merge without invalidating the index.src/linuxRendition.realtest.ts (1)
10-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
crc32,pngChunk,makeGrayPng, andsha256Hexare byte-for-byte duplicates of the helpers insrc/attribution.test.ts. Both suites depend on the fixture producing a PNG the native codec accepts, so a fix applied to one copy and not the other yields confusing divergent failures. Extract them into a shared test fixture module.🤖 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/linuxRendition.realtest.ts` around lines 10 - 57, Extract the duplicated crc32, pngChunk, makeGrayPng, and sha256Hex helpers from src/linuxRendition.realtest.ts and src/attribution.test.ts into a shared test fixture module. Update both suites to import and reuse those shared symbols, preserving the existing PNG generation and hashing behavior.src/linuxTreeSandbox.test.ts (1)
81-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSource-text assertions are brittle. These three tests read
linuxTreeSandbox.tsand match literal substrings (including exact formatting like'await removeSandboxContainer({ containerName }).catch(() => undefined);'and a regex counting'.sandbox-work'occurrences). A reformat or a semantically-equivalent refactor breaks them while a genuine regression (e.g. cleanup rejection propagating) would still pass. Prefer behavioral coverage —runWorkeralready accepts injectable inputs elsewhere in this suite viaspawn.🤖 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/linuxTreeSandbox.test.ts` around lines 81 - 111, Replace the three source-text assertions in the linuxTreeSandbox tests with behavioral tests that exercise runWorker through its injectable spawn dependency. Verify guarded process input, shared sandbox work-directory behavior, and preservation of the worker failure when removeSandboxContainer cleanup rejects, without depending on source formatting or literal occurrence counts.src/linuxCodecSandbox.ts (1)
301-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefault worker path is duplicated.
path.join(import.meta.dir, 'linuxCodecWorker.py')appears in bothcreateLinuxCodecRequestandrunLinuxCodecSandbox; since the latter always stages and passes an explicitworkerPath, the fallback in the former is effectively dead. Extract a singledefaultCodecWorkerPath()helper (or drop the fallback) so the two cannot drift.Also applies to: 479-484
🤖 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/linuxCodecSandbox.ts` around lines 301 - 302, Consolidate the duplicated linuxCodecWorker.py path handling between createLinuxCodecRequest and runLinuxCodecSandbox by introducing one shared defaultCodecWorkerPath() helper, or remove the unreachable fallback from createLinuxCodecRequest. Ensure both code paths use the same worker path source so the defaults cannot diverge.src/attribution.test.ts (1)
34-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSource-text assertions duplicate the pattern in
linuxTreeSandbox.test.ts. Matching literals such as'await removeSandboxContainer({ containerName }).catch(() => undefined);'and'frame?.fill(0)'againstattribution.tsbreaks on reformatting and proves nothing about runtime behavior. Same recommendation as the tree sandbox suite: assert on observable behavior via injected spawn/cleanup doubles.🤖 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/attribution.test.ts` around lines 34 - 57, Replace the source-text assertions in the attribution worker tests with runtime behavior tests using injected spawn and cleanup doubles, following the approach in linuxTreeSandbox.test.ts. Exercise the attribution worker with controlled child-process input/output and verify the bounded boundary helpers, named container cleanup, and frame zeroing through observable calls and effects rather than matching literals in attribution.ts.src/linuxCodecSandbox.test.ts (2)
422-433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAsserting on implementation source text is brittle and proves nothing behavioral.
Slicing
linuxCodecSandbox.tsbetween twoexport async functionmarkers breaks on any rename, reordering, or formatter change, and it still would not catchlstatbeing called on the wrong path. Exercise the behavior instead: pointnormalizedPathat a symlink and assertvalidateLinuxCodecOutputLengthsrejects.♻️ Sketch
- it('checks the output entry without following symbolic links', async () => { - const source = await Bun.file( - new URL('./linuxCodecSandbox.ts', import.meta.url) - ).text(); - const validator = source.slice( - source.indexOf('export async function validateLinuxCodecOutputLengths'), - source.indexOf('export async function runLinuxCodecSandbox') - ); - - expect(validator).toContain('await lstat(outputPath)'); - expect(validator).not.toContain('await stat(outputPath)'); - }); + it('rejects an output entry that is a symbolic link', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'yucp-codec-output-link-')); + try { + const target = path.join(root, 'target.bin'); + await mkdir(path.join(root, 'Assets'), { recursive: true }); + await writeFile(target, new Uint8Array([1, 2, 3])); + await symlink(target, path.join(root, 'Assets', 'output.png')); + + await expect( + validateLinuxCodecOutputLengths(root, [ + { + attributionTokenHex: '00'.repeat(8), + normalizedPath: 'Assets/output.png', + outputBytes: 3, + outputSha256: '11'.repeat(32), + status: 'coupled', + }, + ]) + ).rejects.toThrow(); + } finally { + await rm(root, { force: true, recursive: true }); + } + });(requires importing
symlinkfromnode:fs/promises; skip onwin32where symlink creation needs privileges)🤖 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/linuxCodecSandbox.test.ts` around lines 422 - 433, Replace the source-text inspection test with a behavioral test for validateLinuxCodecOutputLengths: create a temporary regular file and a symlink, set normalizedPath to the symlink, and assert validation rejects it; import symlink from node:fs/promises and skip the test on win32 where symlink creation may require privileges.
140-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFour near-identical
as unknown ascasts to reach module internals.Each call site re-declares a large structural type for the same private functions. Extract one test-local
internalsaccessor (or export the symbols under an__testingnamespace) so the shapes are declared once and drift is caught by the compiler.Also applies to: 183-187, 215-233, 274-292
🤖 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/linuxCodecSandbox.test.ts` around lines 140 - 148, Consolidate the repeated private-function access in the linuxCodecSandbox tests by defining one test-local internals accessor with the shared structural type, then reuse it in the cases around parseLinuxCodecResult and the other noted call sites. Remove the four duplicated as unknown as declarations while preserving compiler-checked signatures for all accessed internals.src/sandboxMounts.test.ts (1)
97-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win64 MiB × 8 concurrent stagers risks a flaky, slow test.
With the current dedup path each losing caller buffers two full copies, so this test allocates on the order of a gigabyte and runs under Bun's default per-test timeout. A few MiB is enough to exercise the interleaving; pair it with an explicit timeout.
♻️ Proposed change
- it('publishes one complete staged file to concurrent callers', async () => { + it('publishes one complete staged file to concurrent callers', async () => { @@ - const bytes = Buffer.alloc(64 * 1024 * 1024, 0x5a); + const bytes = Buffer.alloc(4 * 1024 * 1024, 0x5a); @@ - }); + }, 30_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 `@src/sandboxMounts.test.ts` around lines 97 - 125, Reduce the test fixture in “publishes one complete staged file to concurrent callers” from 64 MiB to a few MiB while preserving the concurrent staging assertions, and add an explicit sufficiently generous timeout to the test so it remains reliable under Bun’s default limits.src/sandboxIsolation.test.ts (1)
51-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
as unknown ascast hides an untyped injection point.If
dockerIsolationArgumentsgenuinely accepts an optionalhostIdentity, declare it in the exported parameter type so the test type-checks the real contract; otherwise this test can keep passing after the option is renamed or removed.🤖 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/sandboxIsolation.test.ts` around lines 51 - 77, Update the exported parameter type for dockerIsolationArguments to explicitly declare the optional hostIdentity field, including uid, gid, and platform, so the test can invoke the real typed contract. Remove the test’s as unknown as cast and preserve the existing Linux user mapping behavior.src/sandboxMounts.ts (1)
147-157: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDedup path loads both files fully into memory.
stageReadonlySandboxFileis used for large artifacts — the test atsandboxMounts.test.tsLine 101 stages 64 MiB with 8 concurrent callers, so seven of them each buffer ~128 MiB simultaneously. Compare size first and then stream a digest instead ofreadFile.🛡️ Proposed change
- const destination = await lstat(destinationPath); - if (!destination.isFile() || destination.isSymbolicLink()) { - throw new Error('The staged sandbox file is invalid'); - } - const [sourceBytes, destinationBytes] = await Promise.all([ - readFile(input.sourcePath), - readFile(destinationPath), - ]); - if (!sourceBytes.equals(destinationBytes)) { - throw new Error('The staged sandbox file does not match its source'); - } + const destination = await lstat(destinationPath); + if (!destination.isFile() || destination.isSymbolicLink()) { + throw new Error('The staged sandbox file is invalid'); + } + if (destination.size !== source.size) { + throw new Error('The staged sandbox file does not match its source'); + } + const [sourceDigest, destinationDigest] = await Promise.all([ + streamedSha256(input.sourcePath), + streamedSha256(destinationPath), + ]); + if (sourceDigest !== destinationDigest) { + throw new Error('The staged sandbox file does not match its source'); + }with a small helper that pipes
createReadStreamintocreateHash('sha256').🤖 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/sandboxMounts.ts` around lines 147 - 157, Update stageReadonlySandboxFile to avoid loading both artifacts fully into memory: compare source and destination sizes first, then compute and compare SHA-256 digests by streaming each file through a small createReadStream/createHash helper. Preserve the existing invalid-destination validation and mismatch errors, and use the size check to reject differing files before hashing.src/sandboxContainer.test.ts (1)
7-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the status-1-with-unrelated-stderr case.
The suite pins status 125 (reject) and status 1 + "No such container" (suppress), but nothing asserts that a status 1 with any other stderr still rejects. That is the exact boundary where an over-broad suppression regex would silently swallow real cleanup failures.
🤖 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/sandboxContainer.test.ts` around lines 7 - 30, Add a test alongside the existing removeSandboxContainer cases that uses exit status 1 with stderr unrelated to the Docker “No such container” response and asserts rejection with the cleanup error. Keep the existing status-125 rejection and already-removed suppression tests unchanged.src/materializationBuilds.test.ts (2)
53-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the optional-export gymnastics and import the function directly.
The cast plus
if (!pin...) return;silently turns a missing export into a passing test, and line 123 already calls it directly off the namespace.♻️ Proposed simplification
- const pinMaterializationBuildArtifacts = ( - materializationBuildsModule as unknown as { - pinMaterializationBuildArtifacts?: (input: { - runtimePath: string; - serviceRoot: string; - workRoot: string; - }) => Promise<{ - builds: unknown; - runtimePath: string; - serviceRoot: string; - workerPaths: { - attribution: string; - codec: string; - tree: string; - }; - }>; - } - ).pinMaterializationBuildArtifacts; - expect(pinMaterializationBuildArtifacts).toBeFunction(); - if (!pinMaterializationBuildArtifacts) { - return; - } - const pinned = await pinMaterializationBuildArtifacts({ + const pinned = await pinMaterializationBuildArtifacts({with
import { computeMaterializationBuilds, pinMaterializationBuildArtifacts } from './materializationBuilds';at the top.🤖 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/materializationBuilds.test.ts` around lines 53 - 74, Import pinMaterializationBuildArtifacts directly alongside computeMaterializationBuilds from materializationBuilds, then remove the namespace cast, optional-function assertion, and early return. Use the direct import in the existing test call so a missing export causes the test to fail.
233-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the symlink rejection behaviorally.
Matching implementation source text passes even if the guard is moved or neutered. Creating a symlinked
bun.lock(or runtime.so) and expecting the rejection tests the actual boundary.🧪 Proposed test
- const source = await Bun.file( - new URL('./materializationBuilds.ts', import.meta.url) - ).text(); - const boundary = source.slice( - source.indexOf('async function requiredFileEntry'), - source.indexOf('export async function computeMaterializationBuilds') - ); - - expect(boundary).toContain('await lstat(filePath)'); - expect(boundary).toContain('details.isSymbolicLink()'); + const fixture = await createFixture(); + try { + const lockPath = path.join(fixture.root, 'bun.lock'); + await rm(lockPath); + await symlink(path.join(fixture.root, 'package.json'), lockPath); + await expect( + computeMaterializationBuilds({ + runtimePath: fixture.runtimePath, + serviceRoot: fixture.root, + }) + ).rejects.toThrow('not a regular file'); + } finally { + await rm(fixture.root, { force: true, recursive: true }); + }(add
symlinkto thenode:fs/promisesimport)🤖 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/materializationBuilds.test.ts` around lines 233 - 244, Replace the source-text assertions in the test “rejects symbolic links at the required build-input boundary” with a behavioral test that creates a symlinked required input such as bun.lock or the runtime .so, invokes the relevant materialization-build API, and asserts it rejects. Import and use the existing filesystem promises symlink facility as needed, while preserving cleanup and the test’s boundary coverage.src/boundedJson.test.ts (1)
4-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the actual size bound. Only invalid-limit inputs are tested; the streaming cap, the
content-lengthrejection, and invalid UTF-8 are untested despite being the security-relevant behavior.💚 Suggested additional cases
); + + it('rejects a body larger than the limit', async () => { + await expect( + readBoundedJsonResponse(new Response('x'.repeat(64)), { + errorPrefix: 'Control-plane response', + maximumBytes: 8, + }) + ).rejects.toThrow('too large'); + }); + + it('rejects an oversized declared content length', async () => { + await expect( + readBoundedJsonResponse( + new Response('{}', { headers: { 'content-length': '9999' } }), + { errorPrefix: 'Control-plane response', maximumBytes: 8 } + ) + ).rejects.toThrow('too large'); + }); });🤖 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/boundedJson.test.ts` around lines 4 - 16, Add tests for readBoundedJsonResponse covering the valid maximumBytes streaming cap, rejection when content-length exceeds the limit, and invalid UTF-8 response bodies; assert each case rejects with the appropriate error while preserving the existing invalid-limit coverage.
🤖 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 @.env.example:
- Line 16: The MATERIALIZATION_MASTER_EPOCH_KEYS_FILE example must include the
systemd unit directory in the credential path. Update its value to point under
/run/credentials/yucp-materializer.service/ while preserving the
materialization-master-epochs filename.
In `@deploy/initialize-local-master-credential.test.sh`:
- Line 9: Update the test script’s PATH setup to avoid hard-coded host-specific
directories, preserving the existing environment PATH and only adding the
required Bun location through a portable mechanism. Ensure the script works
across CI and developer machines where Bun may be installed in different
locations.
In `@deploy/run-local-wsl-materializer.sh`:
- Around line 122-134: Add an explicit Bash version guard before the wait block
containing wait -n -p, requiring Bash 5.1 or newer and exiting with a clear
error message when unsupported. Keep the existing completed_pid and proxy
termination handling unchanged for supported Bash versions.
In `@src/linuxTreeSandbox.ts`:
- Around line 162-190: Update the outer error-handling flow around the worker
interaction in the relevant sandbox method so every failure from writing input,
collecting output, non-zero exit handling, or JSON.parse is wrapped in
LinuxTreeSandboxFailure. Remove the inner write-specific wrapping and preserve
cleanup via removeSandboxContainer before rethrowing the classified failure.
In `@src/linuxWorkers_test.py`:
- Around line 195-196: Update the test around worker.main() to assert the
specific exception expected from the umask failure, such as struct.error or
ValueError, rather than the broad Exception type. Use assertRaisesRegex if the
error message must also be validated, while preserving the existing test setup
and execution path.
- Around line 191-193: Update the test setup in
test_tree_worker_sets_a_private_creation_mask and the corresponding setup around
the second os.umask assignment to patch the process-wide os.umask with
addCleanup restoration. Also restore sys.stdin through the existing
replace_stdin cleanup mechanism instead of assigning it directly, while
preserving the tests’ observation behavior.
In `@src/sandboxImage.ts`:
- Around line 2-3: Update the PINNED_PYTHON_IMAGE constant to use the official
published digest for python:3.13.11-slim-bookworm, keeping the existing tag and
pinned-image format unchanged.
In `@src/sandboxMounts.ts`:
- Around line 77-104: Update exposeWritableEntry so the 0o777 directory and
0o666 file permission widening is applied only on non-Linux platforms, while
preserving the existing traversal and validation behavior. Use the
platform-detection mechanism already used by dockerIsolationArguments rather
than changing prepareWritableSandboxDirectory.
---
Nitpick comments:
In `@src/attribution.test.ts`:
- Around line 34-57: Replace the source-text assertions in the attribution
worker tests with runtime behavior tests using injected spawn and cleanup
doubles, following the approach in linuxTreeSandbox.test.ts. Exercise the
attribution worker with controlled child-process input/output and verify the
bounded boundary helpers, named container cleanup, and frame zeroing through
observable calls and effects rather than matching literals in attribution.ts.
In `@src/boundedJson.test.ts`:
- Around line 4-16: Add tests for readBoundedJsonResponse covering the valid
maximumBytes streaming cap, rejection when content-length exceeds the limit, and
invalid UTF-8 response bodies; assert each case rejects with the appropriate
error while preserving the existing invalid-limit coverage.
In `@src/linuxCodecSandbox.test.ts`:
- Around line 422-433: Replace the source-text inspection test with a behavioral
test for validateLinuxCodecOutputLengths: create a temporary regular file and a
symlink, set normalizedPath to the symlink, and assert validation rejects it;
import symlink from node:fs/promises and skip the test on win32 where symlink
creation may require privileges.
- Around line 140-148: Consolidate the repeated private-function access in the
linuxCodecSandbox tests by defining one test-local internals accessor with the
shared structural type, then reuse it in the cases around parseLinuxCodecResult
and the other noted call sites. Remove the four duplicated as unknown as
declarations while preserving compiler-checked signatures for all accessed
internals.
In `@src/linuxCodecSandbox.ts`:
- Around line 301-302: Consolidate the duplicated linuxCodecWorker.py path
handling between createLinuxCodecRequest and runLinuxCodecSandbox by introducing
one shared defaultCodecWorkerPath() helper, or remove the unreachable fallback
from createLinuxCodecRequest. Ensure both code paths use the same worker path
source so the defaults cannot diverge.
In `@src/linuxRendition.realtest.ts`:
- Around line 10-57: Extract the duplicated crc32, pngChunk, makeGrayPng, and
sha256Hex helpers from src/linuxRendition.realtest.ts and
src/attribution.test.ts into a shared test fixture module. Update both suites to
import and reuse those shared symbols, preserving the existing PNG generation
and hashing behavior.
In `@src/linuxTreeSandbox.test.ts`:
- Around line 81-111: Replace the three source-text assertions in the
linuxTreeSandbox tests with behavioral tests that exercise runWorker through its
injectable spawn dependency. Verify guarded process input, shared sandbox
work-directory behavior, and preservation of the worker failure when
removeSandboxContainer cleanup rejects, without depending on source formatting
or literal occurrence counts.
In `@src/materializationBuilds.test.ts`:
- Around line 53-74: Import pinMaterializationBuildArtifacts directly alongside
computeMaterializationBuilds from materializationBuilds, then remove the
namespace cast, optional-function assertion, and early return. Use the direct
import in the existing test call so a missing export causes the test to fail.
- Around line 233-244: Replace the source-text assertions in the test “rejects
symbolic links at the required build-input boundary” with a behavioral test that
creates a symlinked required input such as bun.lock or the runtime .so, invokes
the relevant materialization-build API, and asserts it rejects. Import and use
the existing filesystem promises symlink facility as needed, while preserving
cleanup and the test’s boundary coverage.
In `@src/materializationChunkCache.ts`:
- Around line 1146-1163: Update the verified-read path around
beginCacheIndexMutation, index.set, and finishCacheIndexMutation so refreshing
lastAccessedAt does not bump the global generation or trigger full rescans. Keep
generation changes and mutation publication for actual index membership changes
such as add or evict, and persist/read access-time metadata through a
metadata-only path that other cache instances can merge without invalidating the
index.
In `@src/nativeHardening.test.ts`:
- Line 93: Widen the forbidden-identifier regex in the guardSource assertion so
it also matches xg_0115 through xg_0121, while preserving the existing coverage
for xg_0100 through xg_0114.
In `@src/sandboxContainer.test.ts`:
- Around line 7-30: Add a test alongside the existing removeSandboxContainer
cases that uses exit status 1 with stderr unrelated to the Docker “No such
container” response and asserts rejection with the cleanup error. Keep the
existing status-125 rejection and already-removed suppression tests unchanged.
In `@src/sandboxIsolation.test.ts`:
- Around line 51-77: Update the exported parameter type for
dockerIsolationArguments to explicitly declare the optional hostIdentity field,
including uid, gid, and platform, so the test can invoke the real typed
contract. Remove the test’s as unknown as cast and preserve the existing Linux
user mapping behavior.
In `@src/sandboxMounts.test.ts`:
- Around line 97-125: Reduce the test fixture in “publishes one complete staged
file to concurrent callers” from 64 MiB to a few MiB while preserving the
concurrent staging assertions, and add an explicit sufficiently generous timeout
to the test so it remains reliable under Bun’s default limits.
In `@src/sandboxMounts.ts`:
- Around line 147-157: Update stageReadonlySandboxFile to avoid loading both
artifacts fully into memory: compare source and destination sizes first, then
compute and compare SHA-256 digests by streaming each file through a small
createReadStream/createHash helper. Preserve the existing invalid-destination
validation and mismatch errors, and use the size check to reject differing files
before hashing.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15e26669-d006-42f3-bd7d-cc70a2ec80fc
⛔ Files ignored due to path filters (18)
bun.lockis excluded by!**/*.lockyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/out/copilot-probe/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/copilot-probe/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.sois excluded by!**/*.soyucp_coupling/out/linux-x64/Release/yucp_coupling.sois excluded by!**/*.soyucp_coupling/out/win-x64/Release/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/win-x64/Release/runtime-helper/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/out/win-x64/Release/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/yucp_coupling.exports.mapis excluded by!**/*.map
📒 Files selected for processing (229)
.env.exampledeploy/initialize-local-master-credential.shdeploy/initialize-local-master-credential.test.shdeploy/prepare-local-wsl-materializer.shdeploy/run-local-wsl-materializer.shdeploy/run-local-wsl-materializer.test.tsdeploy/yucp-materializer.servicepackage.jsonsrc/attestation.native.test.tssrc/attestation.test.tssrc/attestation.tssrc/attribution.test.tssrc/attribution.tssrc/boundedJson.test.tssrc/boundedJson.tssrc/codecLimits.test.tssrc/codecLimits.tssrc/config.tssrc/couplingSeed.test.tssrc/couplingSeed.tssrc/dpopSigner.test.tssrc/dpopSigner.tssrc/env.test.tssrc/env.tssrc/ffi.test.tssrc/ffi.tssrc/keyBroker.test.tssrc/keyBroker.tssrc/linuxAttributionWorker.pysrc/linuxCodecSandbox.test.tssrc/linuxCodecSandbox.tssrc/linuxCodecWorker.pysrc/linuxRendition.realtest.tssrc/linuxTreeSandbox.test.tssrc/linuxTreeSandbox.tssrc/linuxTreeWorker.pysrc/linuxWorkers.test.tssrc/linuxWorkers_test.pysrc/materializationBuilds.test.tssrc/materializationBuilds.tssrc/materializationChunkCache.test.tssrc/materializationChunkCache.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/materialize.test.tssrc/materialize.tssrc/nativeExports.test.tssrc/nativeHardening.test.tssrc/nativePlatform.test.tssrc/nativePlatform.tssrc/publicAuthority.tssrc/rendition.test.tssrc/rendition.tssrc/roundtrip.test.tssrc/runtimeArtifacts.test.tssrc/runtimeArtifacts.tssrc/runtimeTokens.test.tssrc/runtimeTokens.tssrc/sandboxContainer.test.tssrc/sandboxContainer.tssrc/sandboxImage.test.tssrc/sandboxImage.tssrc/sandboxIsolation.test.tssrc/sandboxIsolation.tssrc/sandboxMounts.test.tssrc/sandboxMounts.tssrc/server.nativeAvailability.test.tssrc/server.tssrc/serverArchitecture.test.tssrc/yucpTrust.tssrc/zipArchive.test.tssrc/zipArchive.tsyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/core.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aegis128l.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aegis256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aes256gcm.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_chacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha512256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box_curve25519xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box_curve25519xsalsa20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_hchacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_hsalsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_ristretto255.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa2012.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa208.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_generichash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_generichash_blake2b.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash_sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash_sha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_blake2b.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_hkdf_sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_hkdf_sha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kx.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_onetimeauth.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_onetimeauth_poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_argon2i.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_argon2id.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_scryptsalsa208sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_curve25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_ristretto255.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox_xsalsa20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretstream_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_shorthash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_shorthash_siphash24.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign_edwards25519sha512batch.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_chacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa2012.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa208.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_xchacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_xsalsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_16.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_32.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_64.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/export.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes_internal_random.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes_sysrandom.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/runtime.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/utils.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/version.hyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/static/libsodium.pdbyucp_coupling/_deps/minhook/include/MinHook.hyucp_coupling/_deps/minhook/src/buffer.cyucp_coupling/_deps/minhook/src/buffer.hyucp_coupling/_deps/minhook/src/hde/hde32.cyucp_coupling/_deps/minhook/src/hde/hde32.hyucp_coupling/_deps/minhook/src/hde/hde64.cyucp_coupling/_deps/minhook/src/hde/hde64.hyucp_coupling/_deps/minhook/src/hde/pstdint.hyucp_coupling/_deps/minhook/src/hde/table32.hyucp_coupling/_deps/minhook/src/hde/table64.hyucp_coupling/_deps/minhook/src/hook.cyucp_coupling/_deps/minhook/src/trampoline.cyucp_coupling/_deps/minhook/src/trampoline.hyucp_coupling/build.ps1yucp_coupling/build.shyucp_coupling/coupling_runtime.cyucp_coupling/coupling_runtime.expyucp_coupling/coupling_runtime.libyucp_coupling/guard.cyucp_coupling/out/copilot-probe/yucp_coupling.compile.pdbyucp_coupling/out/copilot-probe/yucp_coupling.pdbyucp_coupling/out/copilot-probe/yucp_coupling.sha256yucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.sha256yucp_coupling/out/linux-x64/Release/yucp_coupling.sha256yucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.compile.pdbyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.pdbyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.sha256yucp_coupling/out/win-x64/Release/yucp_coupling.compile.pdbyucp_coupling/out/win-x64/Release/yucp_coupling.pdbyucp_coupling/out/win-x64/Release/yucp_coupling.sha256yucp_coupling/yucp_coupling.defyucp_coupling/yucp_coupling.runtime-helper.def
💤 Files with no reviewable changes (17)
- src/nativeExports.test.ts
- src/publicAuthority.ts
- src/attestation.native.test.ts
- src/nativePlatform.test.ts
- src/nativePlatform.ts
- src/runtimeArtifacts.test.ts
- src/roundtrip.test.ts
- src/materialize.test.ts
- src/materialize.ts
- src/env.ts
- src/ffi.test.ts
- src/ffi.ts
- src/attestation.test.ts
- src/config.ts
- src/attestation.ts
- src/couplingSeed.ts
- src/couplingSeed.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9caeca54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai full review @codex review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9caeca54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
deploy/yucp-materializer.service (1)
9-9: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
WorkingDirectorystill points to/opt/yucp/ca-coupling, inconsistent with the rest of this PR's deployment paths.
.env.examplein this same PR setsMATERIALIZATION_RUNTIME_LIBRARY_PATH=/home/yucp/ca-coupling/..., and this same unit uses%helsewhere (lines 11, 17) implying the checkout lives under the service user's home directory. WithWorkingDirectory=/opt/yucp/ca-coupling,ExecStart'sbun run startwill fail to locatepackage.json/the entrypoint unless a separate, undocumented step populates/opt/yucp/ca-coupling. A prior review flagged this exact mismatch and it was marked as addressed in earlier commits, but the final state still shows the old value.🔧 Proposed fix
-WorkingDirectory=/opt/yucp/ca-coupling +WorkingDirectory=%h/ca-coupling#!/bin/bash # Confirm the deployment checkout path used elsewhere in the repo. rg -n "ca-coupling" -C1 deploy .env.example 2>/dev/null🤖 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 `@deploy/yucp-materializer.service` at line 9, Update the WorkingDirectory setting in the deploy/yucp-materializer.service unit to use the checkout path under the service user's home directory, consistent with the %h-based paths and MATERIALIZATION_RUNTIME_LIBRARY_PATH. Keep ExecStart unchanged so bun runs from the corrected project directory.
🧹 Nitpick comments (21)
src/sandboxMounts.test.ts (1)
111-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value64 MiB × 8 concurrent copies is a heavy fixture for a unit test. This writes ~0.5 GiB to
tmpdir()per run and will be slow on constrained CI. A few MiB is usually enough to keep the copy window open; consider shrinking unless the large size is required to reproduce the race reliably.🤖 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/sandboxMounts.test.ts` around lines 111 - 139, Reduce the `bytes` fixture in `publishes one complete staged file to concurrent callers` from 64 MiB to a few MiB while preserving the concurrent-copy assertions and race coverage; no larger size is needed unless required by the reproduction.src/sandboxIsolation.test.ts (1)
25-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlatform-conditional expectation mirrors the implementation.
expectedUserrecomputes the same branch assandboxUserIdentity, so this case can't catch a flipped mapping. The third test pins Linux; consider adding aplatform: 'win32'case pinning65534:65534and dropping the conditional here.🤖 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/sandboxIsolation.test.ts` around lines 25 - 49, Update the rootless-daemon identity test to use a fixed expected identity instead of recomputing the platform branch from sandboxUserIdentity. Add a separate test invoking dockerIsolationArguments with platform set to win32 and assert the user identity is 65534:65534, while retaining the Linux case assertion for 0:0.src/materializationServer.test.ts (2)
795-991: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis case asserts ~8 independent contracts in ~200 lines. Health, subject-prepare authz, malformed subject bodies, attribution success, malformed candidates, legacy-route 404, and source-text checks are all in one
it. A failure anywhere hides the rest. Splitting per contract (sharing the handler via a factory) would localize failures.🤖 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/materializationServer.test.ts` around lines 795 - 991, Split the monolithic test around the health and authenticated subject preparation flow into separate tests for each independent contract: health, subject authorization, malformed subject bodies, successful attribution, malformed candidates, legacy-route rejection, and source-text assertions. Add a shared handler factory or equivalent setup so each test can create the required handler without duplicating configuration, while preserving all existing assertions and behavior.
321-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffWall-clock lease tests are flaky under load. These rely on real timers with narrow margins (1 s lease vs. renewal deadline, 4.5 s safety abort inside a 6 s timeout,
Bun.sleep(leaseDurationMs + 100)), so a stalled CI runner can produce spurious failures rather than real regressions. Consider injecting a clock/timer seam into the worker so the renewal scheduler can be driven deterministically.Also applies to: 461-527, 529-606, 608-647
🤖 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/materializationServer.test.ts` around lines 321 - 382, Replace wall-clock timing in the lease-related tests around createMaterializationWorker with an injectable clock/timer seam in the worker. Update the renewal scheduler and affected tests at the referenced scenarios to use controlled time advancement rather than real sleeps, narrow deadlines, or safety abort timeouts, while preserving assertions that renewal occurs before expiry and lease expiration behavior remains correct.src/linuxAttributionWorker.py (3)
226-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuff BLE001 here is a false positive. This is the process-level error serializer that converts any failure into the structured stderr frame the TypeScript side parses; narrowing it would break the contract. Consider
# noqa: BLE001with a short reason to keep the lint clean.🤖 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/linuxAttributionWorker.py` around lines 226 - 235, Add a targeted `# noqa: BLE001` with a brief justification to the process-level `except Exception as error` handler surrounding `main()`, preserving broad exception capture so every failure remains serialized into the structured stderr contract.Source: Linters/SAST tools
196-198: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse
hmac.compare_digestfor the token-hash equality.!=on the hex strings short-circuits on the first differing character. The exposure is limited here, but constant-time comparison is the idiomatic default for authentication-token material.♻️ Proposed change
+import hmac ... - token_hash = hashlib.sha256(token_hex.encode("ascii")).hexdigest() - if token_hash != candidate.get("attributionTokenHash"): + token_hash = hashlib.sha256(token_hex.encode("ascii")).hexdigest() + if not hmac.compare_digest( + token_hash, candidate.get("attributionTokenHash") + ): continue🤖 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/linuxAttributionWorker.py` around lines 196 - 198, Replace the direct token-hash inequality check in the attribution-token matching loop with hmac.compare_digest, ensuring the hashlib-generated token_hash is compared against candidate.get("attributionTokenHash") using a constant-time equality result while preserving the existing continue behavior for mismatches.
43-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe 512/4096 bounds are unreachable once the product guard applies. With
MAX_CANDIDATE_EVALUATIONS = 64and both lists required to be non-empty,len(assets)can never exceed 64. The wider limits read as a contract that the worker does not actually honor; consider dropping them to the effective bound or adding a comment explaining they mirrorattribution.ts.🤖 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/linuxAttributionWorker.py` around lines 43 - 48, The validation around the assets and candidates bounds is inconsistent with MAX_CANDIDATE_EVALUATIONS: because both lists are non-empty, the product guard already limits each list to at most 64. Update the checks to reflect the effective worker contract, or add a concise comment documenting that the 512/4096 limits intentionally mirror attribution.ts.src/linuxCodecSandbox.test.ts (1)
422-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text assertion is brittle. Slicing between two
indexOfhits on function names silently degrades if either function is renamed or reordered (indexOfreturns-1, andslicethen covers the wrong span). Prefer asserting the behavior — e.g. pointnormalizedPathat a symlink to an oversized file and expect the length check to fail — rather than grepping the implementation.🤖 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/linuxCodecSandbox.test.ts` around lines 422 - 433, Replace the source-text assertion in the “checks the output entry without following symbolic links” test with a behavioral test: create a symlink at normalizedPath pointing to an oversized file, run validateLinuxCodecOutputLengths, and assert that validation fails based on the link’s own metadata. Remove the fragile indexOf/slice inspection and direct lstat/stat string checks.src/sandboxMounts.ts (1)
169-179: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe dedup path reads both files fully into memory. For the worker/runtime binaries this stages (tens of MiB), every concurrent loser allocates two full buffers and reads 2× the file. Since
link()publishes atomically and the destination is0444, comparing size plus a streamed SHA-256 — or simply trusting the existing entry — would avoid the allocation spike without weakening the check.♻️ Cheaper equivalence check
- const [sourceBytes, destinationBytes] = await Promise.all([ - readFile(input.sourcePath), - readFile(destinationPath), - ]); - if (!sourceBytes.equals(destinationBytes)) { + if (destination.size !== source.size) { + throw new Error('The staged sandbox file does not match its source'); + } + const [sourceDigest, destinationDigest] = await Promise.all([ + fileSha256(input.sourcePath), + fileSha256(destinationPath), + ]); + if (sourceDigest !== destinationDigest) { throw new Error('The staged sandbox file does not match its source'); }🤖 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/sandboxMounts.ts` around lines 169 - 179, Update the deduplication validation around destination and source reads to avoid loading both files fully into memory; compare file sizes first and compute a streamed SHA-256 for each file only when needed, preserving rejection of mismatched staged files and the existing atomic publish behavior.src/linuxRendition.realtest.ts (1)
115-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
runtimePathis resolved identically in all five tests. Hoist it to a module-level constant.♻️ Proposed change
+const RUNTIME_PATH = path.resolve( + import.meta.dir, + '..', + 'yucp_coupling', + 'out', + 'linux-x64', + 'Release', + 'yucp_coupling.so' +);Also applies to: 196-204, 255-263, 310-318, 365-373
🤖 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/linuxRendition.realtest.ts` around lines 115 - 123, Hoist the shared runtimePath resolution into a module-level constant in src/linuxRendition.realtest.ts, then replace the duplicated local declarations in all five tests with that constant while preserving the existing resolved path.src/linuxWorkers.test.ts (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Ran 12 testsbreaks whenever a worker test is added. It currently matches, but consider asserting on/Ran \d+ tests/plusOKinstead of a fixed count.♻️ Proposed change
- expect(`${stdout}${stderr}`).toContain('Ran 12 tests'); + expect(`${stdout}${stderr}`).toMatch(/Ran \d+ tests/); + expect(`${stdout}${stderr}`).toContain('OK'); expect(exitCode).toBe(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 `@src/linuxWorkers.test.ts` around lines 34 - 35, Update the linux worker test assertion combining stdout and stderr to match the dynamic test summary with a /Ran \d+ tests/ pattern and separately assert that the output contains OK, while preserving the exitCode success assertion.src/linuxCodecSandbox.ts (1)
117-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
frame.fill(0)runs in two nestedfinallyblocks.writeLinuxCodecProcessInputalready zeroes the frame, sorunLinuxCodecContainer's ownfinallyis redundant (and zeroes a buffer the caller may still hold a reference to for other reasons). Keep ownership in one place.Also applies to: 149-151, 440-442
🤖 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/linuxCodecSandbox.ts` around lines 117 - 152, Remove the redundant frame cleanup from the finally block in runLinuxCodecContainer and any corresponding duplicate locations, leaving writeLinuxCodecProcessInput as the sole owner of frame.fill(0). Preserve the existing process cleanup and error-handling behavior while avoiding mutation of a caller-held buffer outside that function.src/linuxTreeSandbox.ts (1)
154-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree nested
tryblocks for two concerns. The inner classify-onlytrycan be folded into the middle one:♻️ Proposed change
try { try { - try { - ... - return JSON.parse(stdout) as T; - } catch (cause) { - throw classifyLinuxTreeSandboxFailure(cause); - } + ... + return JSON.parse(stdout) as T; } catch (cause) { await removeSandboxContainer({ containerName }).catch(() => undefined); - throw cause; + throw classifyLinuxTreeSandboxFailure(cause); } } finally {🤖 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/linuxTreeSandbox.ts` around lines 154 - 198, Fold the classify-only inner try/catch into the surrounding cleanup try in the Linux tree worker flow. Keep the tree process execution and result handling unchanged, ensure removeSandboxContainer still runs on failures, and classify the caught cause via classifyLinuxTreeSandboxFailure before rethrowing it.yucp_coupling/build.sh (1)
49-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider pinning/verifying the vendored
libsodium.achecksum.The build hard-fails if the vendored archive is missing but doesn't verify its contents. Given this PR already computes and publishes a SHA-256 for the built runtime, extending the same discipline to the vendored static dependency would catch accidental corruption or tampering before it's silently linked into the shipped artifact.
🔧 Proposed check
if [ ! -f "$SODIUM_PREFIX/lib/libsodium.a" ] || [ ! -d "$SODIUM_PREFIX/include" ]; then echo "Missing the vendored Linux x64 libsodium dependency" >&2 exit 1 fi +EXPECTED_SODIUM_SHA256="<pin-the-known-good-hash-here>" +ACTUAL_SODIUM_SHA256="$(sha256sum "$SODIUM_PREFIX/lib/libsodium.a" | awk '{print $1}')" +if [ "$ACTUAL_SODIUM_SHA256" != "$EXPECTED_SODIUM_SHA256" ]; then + echo "Vendored libsodium.a checksum mismatch" >&2 + exit 1 +fi🤖 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 `@yucp_coupling/build.sh` around lines 49 - 63, Extend the dependency validation around SODIUM_LINK in build.sh to verify the vendored libsodium.a against a pinned SHA-256 checksum before linking it. Add or reuse a checked-in expected checksum and fail with a clear error when the archive is missing or its digest does not match, while preserving the existing include-directory validation and runtime checksum publication.src/boundedJson.test.ts (1)
4-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the actual size bound.
Only the limit-validation guard is exercised. The security-relevant paths — oversized streamed body (no
content-length), spoofed/oversizedcontent-length, invalid UTF-8, and a successful parse — are untested.🤖 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/boundedJson.test.ts` around lines 4 - 16, Extend the bounded JSON response tests around readBoundedJsonResponse to cover an oversized streamed body without content-length, a spoofed or oversized content-length header, invalid UTF-8 input, and a valid response that parses successfully. Assert each relevant rejection or successful parsed result, while retaining the existing maximumBytes validation coverage.src/keyBroker.ts (1)
69-88: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winZeroize already-parsed epoch keys when a later entry is rejected.
Line 79 wipes only the offending key; keys accepted earlier in the loop stay resident after the throw. Cheap to make the whole failure path clean.
🔒 Proposed fix
const keys = new Map<number, Uint8Array>(); - for (const [epochText, encoded] of Object.entries(parsed)) { - if ( - !/^(0|[1-9]\d*)$/.test(epochText) || - typeof encoded !== 'string' || - !BASE64URL_PATTERN.test(encoded) - ) { - throw new Error('Materialization master epoch credential has an invalid entry'); - } - const key = Buffer.from(encoded, 'base64url'); - if (key.byteLength !== SHA256_BYTES) { - key.fill(0); - throw new Error('Materialization master epoch credential has an invalid entry'); - } - keys.set(Number(epochText), key); - } + try { + for (const [epochText, encoded] of Object.entries(parsed)) { + if ( + !/^(0|[1-9]\d*)$/.test(epochText) || + typeof encoded !== 'string' || + !BASE64URL_PATTERN.test(encoded) + ) { + throw new Error('Materialization master epoch credential has an invalid entry'); + } + const key = Buffer.from(encoded, 'base64url'); + if (key.byteLength !== SHA256_BYTES) { + key.fill(0); + throw new Error('Materialization master epoch credential has an invalid entry'); + } + keys.set(Number(epochText), key); + } + } catch (error) { + for (const key of keys.values()) { + key.fill(0); + } + throw 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 `@src/keyBroker.ts` around lines 69 - 88, Update the parsed-key validation loop in the credential parsing function to zeroize every key already stored in keys before throwing when a later entry is invalid or has the wrong length. Preserve wiping the offending key and ensure the no-keys error path remains unchanged.src/rendition.ts (1)
300-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
request.arguments/containerNameare only meaningful for the injectedrunCodecpath.When
input.runCodecis absent,runLinuxCodecSandboxbuilds its own invocation and thisLinuxCodecRequestis discarded, so thedocker run ...argv here is effectively test-only scaffolding that can drift from the real sandbox command. Consider building the request only wheninput.runCodecis set.🤖 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/rendition.ts` around lines 300 - 343, Update the codec batch loop around runRenditionStage so LinuxCodecRequest is constructed only when input.runCodec is provided. Pass that request to input.runCodec in the injected path, while leaving the runLinuxCodecSandbox path to build its own invocation without request.arguments or containerName scaffolding.src/materializationService.ts (1)
1730-1732: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse byte ordering instead of
localeComparefor chunk ids.
String.prototype.localeCompareis locale/ICU-dependent; every other ordering in this stack (compareNormalizedPathsUtf8,compareUtf8inmaterializationChunkCache.ts) uses UTF-8 byte comparison. Only download batching order depends on it today, but the inconsistency is easy to remove.♻️ Proposed change
- const chunks = [...uniqueChunks.values()].sort((left, right) => - left.id.localeCompare(right.id) - ); + const chunks = [...uniqueChunks.values()].sort((left, right) => + compareNormalizedPathsUtf8(left.id, right.id) + );🤖 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/materializationService.ts` around lines 1730 - 1732, Update the chunk sorting in the download batching flow around uniqueChunks to use the existing UTF-8 byte comparison utility instead of left.id.localeCompare(right.id). Match the ordering behavior used by compareNormalizedPathsUtf8 and compareUtf8, preserving ascending chunk-id order.src/materializationChunkCache.ts (1)
438-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe composed in-process signal is never aborted, so
signal.throwIfAborted()is dead.
withInProcessFileLockhands the operation a freshnew AbortController().signalthat nothing ever aborts, so Line 446 can never throw and the kernel-lock abort signal (fromwithLinuxFileLock) is the only live one. Harmless today, but it reads as if queue cancellation is wired up when it isn't.🤖 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/materializationChunkCache.ts` around lines 438 - 449, Update withDefaultFileLock to remove the ineffective signal.throwIfAborted() call from the withInProcessFileLock callback, since that composed signal is never aborted. Preserve the existing withLinuxFileLock invocation and lock behavior on Linux, as well as the direct in-process path on other platforms.src/materializationChunkCache.test.ts (1)
258-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese assert on implementation source text rather than behavior.
toContain('withInProcessFileLock(lockPath, (signal) =>')and thescanCacheEntries()occurrence count break on any harmless rename/reformat while proving nothing about runtime behavior. The lock-composition property is already observable (a single kernel lock per path, in-process queueing), and the rescan property could be asserted by countingreaddir/statcalls through an injected dependency.🤖 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/materializationChunkCache.test.ts` around lines 258 - 278, Replace the source-text assertions in the tests around the lock and capacity behavior with runtime-focused tests. Exercise the materialization cache’s lock acquisition to verify concurrent operations on the same path share a single kernel lock while remaining queued in process, and inject or spy on the filesystem dependency to assert capacity decisions do not repeatedly call readdir/stat for every committed object. Remove brittle checks based on withInProcessFileLock, scanCacheEntries(), and capacity-generation.json source-text counts.src/materializationService.test.ts (1)
481-510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejecting case to the order-independence test.
The test only proves reordered-but-matching input passes; a
assertProtectedFilesMatchthat ignoredsha256/materializerTypeentirely would still pass. A second expectation with a mismatched digest (or duplicatenormalizedPath) would lock the contract down.🤖 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/materializationService.test.ts` around lines 481 - 510, The order-independence test around assertProtectedFilesMatch should also verify rejection when matched paths have an incorrect sha256 or materializerType, such as by adding a second expectation that throws for a mismatched digest or duplicate normalizedPath. Keep the existing reordered matching assertion unchanged.
🤖 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 `@src/attribution.ts`:
- Around line 304-310: Update the cleanup finally block around
restorePrivateSandboxTree so a rejection from restoring inputDirectory is
swallowed, preserving the original operation failure. Keep scratch removal in
its nested finally so rm still runs regardless of restore success, matching the
existing non-throwing cleanup behavior of removeSandboxContainer.
In `@src/sandboxMounts.ts`:
- Around line 69-82: Update restorePrivateSandboxEntry, used by
restorePrivateSandboxTree cleanup, so symlinks and other non-regular entries are
skipped or otherwise handled without throwing. Preserve the existing ENOENT
tolerance and ensure cleanup failures do not replace the original worker error
from finally blocks.
---
Duplicate comments:
In `@deploy/yucp-materializer.service`:
- Line 9: Update the WorkingDirectory setting in the
deploy/yucp-materializer.service unit to use the checkout path under the service
user's home directory, consistent with the %h-based paths and
MATERIALIZATION_RUNTIME_LIBRARY_PATH. Keep ExecStart unchanged so bun runs from
the corrected project directory.
---
Nitpick comments:
In `@src/boundedJson.test.ts`:
- Around line 4-16: Extend the bounded JSON response tests around
readBoundedJsonResponse to cover an oversized streamed body without
content-length, a spoofed or oversized content-length header, invalid UTF-8
input, and a valid response that parses successfully. Assert each relevant
rejection or successful parsed result, while retaining the existing maximumBytes
validation coverage.
In `@src/keyBroker.ts`:
- Around line 69-88: Update the parsed-key validation loop in the credential
parsing function to zeroize every key already stored in keys before throwing
when a later entry is invalid or has the wrong length. Preserve wiping the
offending key and ensure the no-keys error path remains unchanged.
In `@src/linuxAttributionWorker.py`:
- Around line 226-235: Add a targeted `# noqa: BLE001` with a brief
justification to the process-level `except Exception as error` handler
surrounding `main()`, preserving broad exception capture so every failure
remains serialized into the structured stderr contract.
- Around line 196-198: Replace the direct token-hash inequality check in the
attribution-token matching loop with hmac.compare_digest, ensuring the
hashlib-generated token_hash is compared against
candidate.get("attributionTokenHash") using a constant-time equality result
while preserving the existing continue behavior for mismatches.
- Around line 43-48: The validation around the assets and candidates bounds is
inconsistent with MAX_CANDIDATE_EVALUATIONS: because both lists are non-empty,
the product guard already limits each list to at most 64. Update the checks to
reflect the effective worker contract, or add a concise comment documenting that
the 512/4096 limits intentionally mirror attribution.ts.
In `@src/linuxCodecSandbox.test.ts`:
- Around line 422-433: Replace the source-text assertion in the “checks the
output entry without following symbolic links” test with a behavioral test:
create a symlink at normalizedPath pointing to an oversized file, run
validateLinuxCodecOutputLengths, and assert that validation fails based on the
link’s own metadata. Remove the fragile indexOf/slice inspection and direct
lstat/stat string checks.
In `@src/linuxCodecSandbox.ts`:
- Around line 117-152: Remove the redundant frame cleanup from the finally block
in runLinuxCodecContainer and any corresponding duplicate locations, leaving
writeLinuxCodecProcessInput as the sole owner of frame.fill(0). Preserve the
existing process cleanup and error-handling behavior while avoiding mutation of
a caller-held buffer outside that function.
In `@src/linuxRendition.realtest.ts`:
- Around line 115-123: Hoist the shared runtimePath resolution into a
module-level constant in src/linuxRendition.realtest.ts, then replace the
duplicated local declarations in all five tests with that constant while
preserving the existing resolved path.
In `@src/linuxTreeSandbox.ts`:
- Around line 154-198: Fold the classify-only inner try/catch into the
surrounding cleanup try in the Linux tree worker flow. Keep the tree process
execution and result handling unchanged, ensure removeSandboxContainer still
runs on failures, and classify the caught cause via
classifyLinuxTreeSandboxFailure before rethrowing it.
In `@src/linuxWorkers.test.ts`:
- Around line 34-35: Update the linux worker test assertion combining stdout and
stderr to match the dynamic test summary with a /Ran \d+ tests/ pattern and
separately assert that the output contains OK, while preserving the exitCode
success assertion.
In `@src/materializationChunkCache.test.ts`:
- Around line 258-278: Replace the source-text assertions in the tests around
the lock and capacity behavior with runtime-focused tests. Exercise the
materialization cache’s lock acquisition to verify concurrent operations on the
same path share a single kernel lock while remaining queued in process, and
inject or spy on the filesystem dependency to assert capacity decisions do not
repeatedly call readdir/stat for every committed object. Remove brittle checks
based on withInProcessFileLock, scanCacheEntries(), and capacity-generation.json
source-text counts.
In `@src/materializationChunkCache.ts`:
- Around line 438-449: Update withDefaultFileLock to remove the ineffective
signal.throwIfAborted() call from the withInProcessFileLock callback, since that
composed signal is never aborted. Preserve the existing withLinuxFileLock
invocation and lock behavior on Linux, as well as the direct in-process path on
other platforms.
In `@src/materializationServer.test.ts`:
- Around line 795-991: Split the monolithic test around the health and
authenticated subject preparation flow into separate tests for each independent
contract: health, subject authorization, malformed subject bodies, successful
attribution, malformed candidates, legacy-route rejection, and source-text
assertions. Add a shared handler factory or equivalent setup so each test can
create the required handler without duplicating configuration, while preserving
all existing assertions and behavior.
- Around line 321-382: Replace wall-clock timing in the lease-related tests
around createMaterializationWorker with an injectable clock/timer seam in the
worker. Update the renewal scheduler and affected tests at the referenced
scenarios to use controlled time advancement rather than real sleeps, narrow
deadlines, or safety abort timeouts, while preserving assertions that renewal
occurs before expiry and lease expiration behavior remains correct.
In `@src/materializationService.test.ts`:
- Around line 481-510: The order-independence test around
assertProtectedFilesMatch should also verify rejection when matched paths have
an incorrect sha256 or materializerType, such as by adding a second expectation
that throws for a mismatched digest or duplicate normalizedPath. Keep the
existing reordered matching assertion unchanged.
In `@src/materializationService.ts`:
- Around line 1730-1732: Update the chunk sorting in the download batching flow
around uniqueChunks to use the existing UTF-8 byte comparison utility instead of
left.id.localeCompare(right.id). Match the ordering behavior used by
compareNormalizedPathsUtf8 and compareUtf8, preserving ascending chunk-id order.
In `@src/rendition.ts`:
- Around line 300-343: Update the codec batch loop around runRenditionStage so
LinuxCodecRequest is constructed only when input.runCodec is provided. Pass that
request to input.runCodec in the injected path, while leaving the
runLinuxCodecSandbox path to build its own invocation without request.arguments
or containerName scaffolding.
In `@src/sandboxIsolation.test.ts`:
- Around line 25-49: Update the rootless-daemon identity test to use a fixed
expected identity instead of recomputing the platform branch from
sandboxUserIdentity. Add a separate test invoking dockerIsolationArguments with
platform set to win32 and assert the user identity is 65534:65534, while
retaining the Linux case assertion for 0:0.
In `@src/sandboxMounts.test.ts`:
- Around line 111-139: Reduce the `bytes` fixture in `publishes one complete
staged file to concurrent callers` from 64 MiB to a few MiB while preserving the
concurrent-copy assertions and race coverage; no larger size is needed unless
required by the reproduction.
In `@src/sandboxMounts.ts`:
- Around line 169-179: Update the deduplication validation around destination
and source reads to avoid loading both files fully into memory; compare file
sizes first and compute a streamed SHA-256 for each file only when needed,
preserving rejection of mismatched staged files and the existing atomic publish
behavior.
In `@yucp_coupling/build.sh`:
- Around line 49-63: Extend the dependency validation around SODIUM_LINK in
build.sh to verify the vendored libsodium.a against a pinned SHA-256 checksum
before linking it. Add or reuse a checked-in expected checksum and fail with a
clear error when the archive is missing or its digest does not match, while
preserving the existing include-directory validation and runtime checksum
publication.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5a2d8ff-77ac-4979-b733-902629ed2cef
⛔ Files ignored due to path filters (18)
bun.lockis excluded by!**/*.lockyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.dllis excluded by!**/*.dllyucp_coupling/out/copilot-probe/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/copilot-probe/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.sois excluded by!**/*.soyucp_coupling/out/linux-x64/Release/yucp_coupling.sois excluded by!**/*.soyucp_coupling/out/win-x64/Release/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/win-x64/Release/runtime-helper/coupling_runtime.objis excluded by!**/*.objyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/out/win-x64/Release/yucp_coupling.dllis excluded by!**/*.dllyucp_coupling/yucp_coupling.exports.mapis excluded by!**/*.map
📒 Files selected for processing (229)
.env.exampledeploy/initialize-local-master-credential.shdeploy/initialize-local-master-credential.test.shdeploy/prepare-local-wsl-materializer.shdeploy/run-local-wsl-materializer.shdeploy/run-local-wsl-materializer.test.tsdeploy/yucp-materializer.servicepackage.jsonsrc/attestation.native.test.tssrc/attestation.test.tssrc/attestation.tssrc/attribution.test.tssrc/attribution.tssrc/boundedJson.test.tssrc/boundedJson.tssrc/codecLimits.test.tssrc/codecLimits.tssrc/config.tssrc/couplingSeed.test.tssrc/couplingSeed.tssrc/dpopSigner.test.tssrc/dpopSigner.tssrc/env.test.tssrc/env.tssrc/ffi.test.tssrc/ffi.tssrc/keyBroker.test.tssrc/keyBroker.tssrc/linuxAttributionWorker.pysrc/linuxCodecSandbox.test.tssrc/linuxCodecSandbox.tssrc/linuxCodecWorker.pysrc/linuxRendition.realtest.tssrc/linuxTreeSandbox.test.tssrc/linuxTreeSandbox.tssrc/linuxTreeWorker.pysrc/linuxWorkers.test.tssrc/linuxWorkers_test.pysrc/materializationBuilds.test.tssrc/materializationBuilds.tssrc/materializationChunkCache.test.tssrc/materializationChunkCache.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/materialize.test.tssrc/materialize.tssrc/nativeExports.test.tssrc/nativeHardening.test.tssrc/nativePlatform.test.tssrc/nativePlatform.tssrc/publicAuthority.tssrc/rendition.test.tssrc/rendition.tssrc/roundtrip.test.tssrc/runtimeArtifacts.test.tssrc/runtimeArtifacts.tssrc/runtimeTokens.test.tssrc/runtimeTokens.tssrc/sandboxContainer.test.tssrc/sandboxContainer.tssrc/sandboxImage.test.tssrc/sandboxImage.tssrc/sandboxIsolation.test.tssrc/sandboxIsolation.tssrc/sandboxMounts.test.tssrc/sandboxMounts.tssrc/server.nativeAvailability.test.tssrc/server.tssrc/serverArchitecture.test.tssrc/yucpTrust.tssrc/zipArchive.test.tssrc/zipArchive.tsyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Debug/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/Win32/Release/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/core.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aegis128l.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aegis256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_aes256gcm.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_chacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_aead_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_auth_hmacsha512256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box_curve25519xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_box_curve25519xsalsa20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_hchacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_hsalsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_ristretto255.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa2012.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_core_salsa208.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_generichash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_generichash_blake2b.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash_sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_hash_sha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_blake2b.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_hkdf_sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kdf_hkdf_sha512.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_kx.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_onetimeauth.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_onetimeauth_poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_argon2i.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_argon2id.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_pwhash_scryptsalsa208sha256.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_curve25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_scalarmult_ristretto255.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretbox_xsalsa20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_secretstream_xchacha20poly1305.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_shorthash.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_shorthash_siphash24.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign_ed25519.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_sign_edwards25519sha512batch.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_chacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa2012.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_salsa208.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_xchacha20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_stream_xsalsa20.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_16.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_32.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/crypto_verify_64.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/export.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes_internal_random.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/randombytes_sysrandom.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/runtime.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/utils.hyucp_coupling/_deps/libsodium-msvc/libsodium/include/sodium/version.hyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.ilkyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Debug/v143/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v142/static/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.expyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/dynamic/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/ltcg/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/ltcg/libsodium.pdbyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/static/libsodium.libyucp_coupling/_deps/libsodium-msvc/libsodium/x64/Release/v143/static/libsodium.pdbyucp_coupling/_deps/minhook/include/MinHook.hyucp_coupling/_deps/minhook/src/buffer.cyucp_coupling/_deps/minhook/src/buffer.hyucp_coupling/_deps/minhook/src/hde/hde32.cyucp_coupling/_deps/minhook/src/hde/hde32.hyucp_coupling/_deps/minhook/src/hde/hde64.cyucp_coupling/_deps/minhook/src/hde/hde64.hyucp_coupling/_deps/minhook/src/hde/pstdint.hyucp_coupling/_deps/minhook/src/hde/table32.hyucp_coupling/_deps/minhook/src/hde/table64.hyucp_coupling/_deps/minhook/src/hook.cyucp_coupling/_deps/minhook/src/trampoline.cyucp_coupling/_deps/minhook/src/trampoline.hyucp_coupling/build.ps1yucp_coupling/build.shyucp_coupling/coupling_runtime.cyucp_coupling/coupling_runtime.expyucp_coupling/coupling_runtime.libyucp_coupling/guard.cyucp_coupling/out/copilot-probe/yucp_coupling.compile.pdbyucp_coupling/out/copilot-probe/yucp_coupling.pdbyucp_coupling/out/copilot-probe/yucp_coupling.sha256yucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.sha256yucp_coupling/out/linux-x64/Release/yucp_coupling.sha256yucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.compile.pdbyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.pdbyucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.sha256yucp_coupling/out/win-x64/Release/yucp_coupling.compile.pdbyucp_coupling/out/win-x64/Release/yucp_coupling.pdbyucp_coupling/out/win-x64/Release/yucp_coupling.sha256yucp_coupling/yucp_coupling.defyucp_coupling/yucp_coupling.runtime-helper.def
💤 Files with no reviewable changes (16)
- src/publicAuthority.ts
- src/nativePlatform.ts
- src/materialize.test.ts
- src/couplingSeed.ts
- src/env.ts
- src/couplingSeed.test.ts
- src/attestation.test.ts
- src/roundtrip.test.ts
- src/nativeExports.test.ts
- src/attestation.native.test.ts
- src/ffi.test.ts
- src/materialize.ts
- src/nativePlatform.test.ts
- src/attestation.ts
- src/config.ts
- src/ffi.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 595bbf4c1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review @codex review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 595bbf4c1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review Please review the latest incremental fixes for Linux workspace admission, committed-result finality, cleanup diagnostics, portable runtime configuration, safe sandbox restoration, and zero-byte cache reservations. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b96749349
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/materializationWorkspaceCapacity.test.ts (1)
99-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a failure-path reservation test
Exerciseoperationthrowing so this cleanup path is covered and quota reuse after a failed archive write is protected.🤖 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/materializationWorkspaceCapacity.test.ts` around lines 99 - 114, Add a test alongside the existing withWorkspaceReservation success case that makes the operation callback throw, asserts the error propagates, and verifies the reservation is released so a subsequent reservation can reuse the quota. Use the existing cache, workspace paths, and event tracking in the test setup, and retain cleanup in the finally block.src/linuxTreeWorker.py (1)
232-241: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant full re-read to compute the rendition digest.
The archive is fully streamed/written once during
build_rendition, then re-read in its entirety here just to computerenditionSha256/renditionBytes. Tracking a runninghashlib.sha256()and byte counter insideBoundedArchiveFile.write()would give the same deterministic result without a second full-file I/O pass — relevant for large renditions on this hot path.♻️ Sketch: hash incrementally in the wrapper
class BoundedArchiveFile: def __init__(self, output, maximum_bytes): self.output = output self.maximum_bytes = maximum_bytes + self.digest = hashlib.sha256() + self.size = 0 def write(self, data): if self.output.tell() + len(data) > self.maximum_bytes: raise ValueError("rendition archive exceeds its reserved capacity") - return self.output.write(data) + written = self.output.write(data) + self.digest.update(data) + self.size += len(data) + return writtenThen use
bounded_archive.digest.hexdigest()/bounded_archive.sizeafter thewith zipfile.ZipFile(...)block instead of re-readingRENDITION_PATH.🤖 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/linuxTreeWorker.py` around lines 232 - 241, Update BoundedArchiveFile.write() to maintain a running hashlib.sha256 digest and byte count for every written chunk, exposing them through the wrapper’s digest and size state. In build_rendition, use bounded_archive.digest.hexdigest() and bounded_archive.size after the ZipFile write completes, and remove the full-file re-read used to compute renditionSha256 and renditionBytes.
🤖 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/linuxTreeWorker.py`:
- Around line 232-241: Update BoundedArchiveFile.write() to maintain a running
hashlib.sha256 digest and byte count for every written chunk, exposing them
through the wrapper’s digest and size state. In build_rendition, use
bounded_archive.digest.hexdigest() and bounded_archive.size after the ZipFile
write completes, and remove the full-file re-read used to compute
renditionSha256 and renditionBytes.
In `@src/materializationWorkspaceCapacity.test.ts`:
- Around line 99-114: Add a test alongside the existing withWorkspaceReservation
success case that makes the operation callback throw, asserts the error
propagates, and verifies the reservation is released so a subsequent reservation
can reuse the quota. Use the existing cache, workspace paths, and event tracking
in the test setup, and retain cleanup in the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7142fed-33dc-4f51-89ea-24c3c1149932
📒 Files selected for processing (16)
.env.examplesrc/attribution.test.tssrc/attribution.tssrc/linuxTreeSandbox.tssrc/linuxTreeWorker.pysrc/materializationChunkCache.test.tssrc/materializationChunkCache.tssrc/materializationServer.test.tssrc/materializationServer.tssrc/materializationService.test.tssrc/materializationService.tssrc/materializationWorkspaceCapacity.test.tssrc/rendition.tssrc/sandboxMounts.test.tssrc/sandboxMounts.tssrc/serverArchitecture.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/attribution.test.ts
- src/sandboxMounts.test.ts
- src/materializationService.test.ts
- src/sandboxMounts.ts
- src/rendition.ts
- src/serverArchitecture.test.ts
- src/materializationServer.test.ts
- src/attribution.ts
- src/materializationChunkCache.test.ts
- src/materializationChunkCache.ts
- src/materializationServer.ts
- src/materializationService.ts
1 participant
Summary
Why
Real JAMMR assets exposed valid files that did not contain enough capacity for the fixed PNG and mesh profiles. The materializer rejected those assets with
MATERIALIZATION_CODEC_ASSET_UNCARRYABLE. The codec now selects a bounded profile that fits each supported asset while preserving deterministic attribution and blind recovery.Validation
bun run typecheckbun testwith 36 passing testsbun run test:linux-renditionwith 4 passing real testsda18c771ed89d1648b679c88cbb95d06a6b32aedfcbe024517cfb4f3cc91bb95Boundaries
This repository remains the only owner of proprietary coupling code. Creator Assistant calls the authenticated server interface and contains no coupling implementation.
Coordinated changes
Summary by CodeRabbit
MATERIALIZATION_*configuration.