Repository navigation
feat: make coupling runtime seed-only - #2
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes and 45 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR narrows coupling handling to seeded v2 PNG/FBX paths, adds a coupling attribution endpoint, updates scan and materialization tests for required seeds, and removes legacy native exports and implementations. ChangesSeeded v2 coupling and attribution
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Remove the blind and legacy text decode surface from the TypeScript FFI and native export table so attribution always requires the per-job seed that placed the mark. Add the seed-iteration attribution route coverage and update the tracked Windows runtime artifacts to match the reduced v2 PNG/FBX native surface. Verification: bun run typecheck; bun test.
79c8f47 to
089fd5b
Compare
💡 Codex Reviewca-coupling/yucp_coupling/guard.c Line 1146 in 79c8f47  Align token length validation with native v2
When an existing caller or control-plane coupling job sends a ca-coupling/src/couplingSeed.test.ts Lines 4 to 5 in 79c8f47  Set the seed on cached config in tests
When another test imports ℹ️ 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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ffi.ts (1)
217-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject legacy-width tokens before calling the v2-only encoder.
This block now sends every token through
xg_0122/xg_0124, but the JS-side validator still accepts 8-64 hex chars. The updated materialization fixture had to drop from 32 to 16 chars for seeded v2 embedding, so longer legacy tokens can now cross the FFI boundary and fail only in native code. Tighten the TypeScript contract to the v2 token width before dispatching 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/ffi.ts` around lines 217 - 229, Reject legacy-width tokens in encodeCouplingAsset before dispatching to the v2 native encoders xg_0122 and xg_0124. Tighten the TypeScript-side validation contract for CouplingEncodeInput.tokenHex so only the v2 token width is accepted, using the existing normalization helpers normalizeTokenHexForFfi and normalizeSeedHexForFfi to enforce the narrower length before building the native pointers and calling loadNativeLibrary().
🤖 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/server.ts`:
- Around line 850-856: Reject oversized candidate sets in the attribute matching
flow instead of truncating them: in the server-side handling around the
candidates cap check and the AttributeCandidate loop, replace the
warning-and-slice behavior with an explicit error response when body.candidates
exceeds MAX_ATTRIBUTE_CANDIDATES. Make the change in the logic that builds the
candidates array so oversized inputs fail fast and do not continue into the
matching path with partial data.
- Around line 947-964: Handle failures in the candidate loop in server.ts
without swallowing service errors: in the logic around deriveCouplingSeedHex and
decodeCouplingAsset, only treat expected “no match” cases as misses, but let
invalid master-key/configuration errors and unexpected decode/runtime exceptions
propagate so the request fails instead of returning matched: false. Use the
existing candidate-matching flow in the block that sets hit and breaks to
separate recoverable per-candidate mismatches from real service failures.
- Around line 917-922: The asset upload validation in the `contentBase64`
handling is too weak because `Buffer.from(..., 'base64')` can accept malformed
input without throwing. Update the `src/server.ts` asset decoding flow around
the `Uint8Array.from(Buffer.from(...))` block to perform strict base64
validation before decoding, and keep the `HttpError` path for invalid
`contentBase64` in the asset write logic.
In `@yucp_coupling/yucp_coupling.def`:
- Around line 3-6: The export table is being renumbered in yucp_coupling.def,
which breaks ordinal-based Windows consumers. Keep the surviving exports
xg_0122, xg_0123, xg_0124, and xg_0125 at their original ordinals instead of
compressing them to `@1-`@4, and leave gaps where the removed legacy exports used
to be. Apply the same ordinal-preserving layout in
yucp_coupling.runtime-helper.def so both DEF files stay aligned.
---
Outside diff comments:
In `@src/ffi.ts`:
- Around line 217-229: Reject legacy-width tokens in encodeCouplingAsset before
dispatching to the v2 native encoders xg_0122 and xg_0124. Tighten the
TypeScript-side validation contract for CouplingEncodeInput.tokenHex so only the
v2 token width is accepted, using the existing normalization helpers
normalizeTokenHexForFfi and normalizeSeedHexForFfi to enforce the narrower
length before building the native pointers and calling loadNativeLibrary().
🪄 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: c018b9d3-7809-41c8-b409-b3f4a21a3dde
⛔ Files ignored due to path filters (4)
yucp_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!**/*.dll
📒 Files selected for processing (16)
coupling_runtime.expcoupling_runtime.libsrc/ffi.tssrc/materialize.test.tssrc/roundtrip.test.tssrc/server.tsyucp_coupling/coupling_runtime.cyucp_coupling/guard.cyucp_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 (1)
- yucp_coupling/guard.c
| xg_0122 @1 | ||
| xg_0123 @2 | ||
| xg_0124 @3 | ||
| xg_0125 @4 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the old ordinals when shrinking the export table.
Reassigning the surviving symbols to @1-@4 breaks any Windows consumer or import library that binds by ordinal. Keep xg_0122-xg_0125 at their previous ordinals and leave gaps for the removed legacy exports; yucp_coupling.runtime-helper.def should mirror that approach.
🤖 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/yucp_coupling.def` around lines 3 - 6, The export table is
being renumbered in yucp_coupling.def, which breaks ordinal-based Windows
consumers. Keep the surviving exports xg_0122, xg_0123, xg_0124, and xg_0125 at
their original ordinals instead of compressing them to `@1-`@4, and leave gaps
where the removed legacy exports used to be. Apply the same ordinal-preserving
layout in yucp_coupling.runtime-helper.def so both DEF files stay aligned.
Reject oversized attribution candidate sets instead of truncating them, validate base64 input strictly, and let non-native service failures abort the request. Align TypeScript token validation with the native v2 16-hex contract and preserve Windows export ordinals for the surviving seeded symbols. Rebuild the Windows native artifacts and add regressions for the review findings.
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 `@src/roundtrip.test.ts`:
- Around line 169-170: The health-check failure path is incorrectly awaiting the
live stderr stream via new Response(service.stderr).text(), which can block
until EOF and hang the test helper; update the timeout handling in the roundtrip
test helper to avoid waiting on the pipe after the service fails to become
healthy, and instead use only non-blocking, already-available error information
from the service/stderr capture when constructing the thrown error message.
- Around line 639-644: The test setup in the asset payload is using an obviously
invalid PNG blob, which makes the failure depend on asset decoding instead of
the intended bad seed path. Update the asset fixture in roundtrip.test.ts to use
a tiny valid PNG payload in the assets array so the 500 internal_error assertion
continues to target the misconfigured COUPLING_WM_MASTER_KEY flow. Keep the
change localized to the leak.png fixture used by this test case.
🪄 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: 622ba45f-215c-4776-b9c5-797c31b69512
⛔ Files ignored due to path filters (4)
yucp_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!**/*.dll
📒 Files selected for processing (16)
coupling_runtime.expcoupling_runtime.libsrc/couplingSeed.test.tssrc/ffi.test.tssrc/ffi.tssrc/nativeExports.test.tssrc/roundtrip.test.tssrc/server.tsyucp_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 skipped from review due to trivial changes (2)
- yucp_coupling/out/win-x64/Release/yucp_coupling.sha256
- yucp_coupling/out/win-x64/Release/runtime-helper/yucp_coupling.sha256
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ffi.ts
- src/server.ts
![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: 8ffc046fd0
ℹ️ 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".
| #define xg_0122 yucp_runtime_helper_encode_seeded_png | ||
| #define xg_0124 yucp_runtime_helper_encode_seeded_fbx |
There was a problem hiding this comment.
This source change removes the runtime-helper legacy wrappers, but the committed Linux runtime-helper artifact was not refreshed: nm -D --defined-only yucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.so still shows xg_0115 and xg_0120 alongside the seeded exports, and the server .so still exports the removed legacy decoders. On Linux clients/deployments this means the shipped binary still contains the pre-seed encoder path even though the TypeScript and Windows artifacts are seed-only, undermining the security goal of this change; please rebuild/update the Linux artifacts and hashes as part of this commit.
Useful? React with 👍 / 👎.
| const candidates: AttributeCandidate[] = []; | ||
| for (const raw of body.candidates) { | ||
| const c = raw as { assetPath?: unknown; licenseSubject?: unknown; tokenHash?: unknown }; | ||
| const assetPath = typeof c.assetPath === 'string' ? c.assetPath.trim() : ''; |
There was a problem hiding this comment.
For attribution candidates coming from Windows manifests or from the same raw paths accepted by /v1/coupling/internal/derive-seeds, this only trims the candidate path before it is later passed to deriveCouplingSeedHex. The seed endpoint normalizes backslashes and leading slashes before HKDF, so a candidate like \Assets\Test\leak.png derives a different seed here than it did when the watermark was placed, causing a real buyer to be reported as unmatched; normalize and validate candidate paths the same way as seed derivation before storing them.
Useful? React with 👍 / 👎.
Capture service stderr asynchronously so health-check timeouts never block on a live stream. Use a valid PNG fixture in the bad seed configuration regression so it tests the intended failure path.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cec78fe70
ℹ️ 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".
| #define xg_0122 yucp_runtime_helper_encode_seeded_png | ||
| #define xg_0124 yucp_runtime_helper_encode_seeded_fbx |
There was a problem hiding this comment.
After dropping the helper legacy exports here, the committed Linux artifacts were not rebuilt: I checked nm -D yucp_coupling/out/linux-x64/Release/runtime-helper/yucp_coupling.so and it still exports xg_0115/xg_0120, while the server .so still exports the legacy decoders xg_0118/xg_0119/xg_0121. On Linux hosts config.dllPath defaults to out/linux-x64/Release/yucp_coupling.so, so Linux deployments still ship the removed legacy/blind runtime even though the TypeScript layer and Windows DLLs are seed-only; rebuild and commit the Linux .so and hash artifacts from this source change.
Useful? React with 👍 / 👎.
| const candidates: AttributeCandidate[] = []; | ||
| for (const raw of body.candidates) { | ||
| const c = raw as { assetPath?: unknown; licenseSubject?: unknown; tokenHash?: unknown }; | ||
| const assetPath = typeof c.assetPath === 'string' ? c.assetPath.trim() : ''; |
There was a problem hiding this comment.
When attribution candidates come from paths with Windows separators or a leading slash, this raw trim() value is fed into deriveCouplingSeedHex, while seed creation elsewhere normalizes the same asset path via normalizeRelativeAssetPath/assertSafeProjectRelativePath. In that case a candidate for \Assets\Test\leak.png derives a different seed than the one used to embed Assets/Test/leak.png, so /v1/coupling/attribute returns matched: false even with the correct buyer and token hash; normalize and validate candidate paths before storing them.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
bun run typecheckbun testSummary by CodeRabbit
/v1/coupling/attribute) that iterates seeded candidates per asset and reportsmatchedresults.assetTypesas PNG/FBX.