feat: deliver Phase 2 envelope and intake foundations - #123
Conversation
📝 WalkthroughWalkthroughThis change adds runtime source, envelope, provider-admission, and qualification contracts. It adds conformance provenance tracking and filesystem-backed local providers with witnesses, recovery, artifact lifecycle controls, package boundary enforcement, and Phase 2 delivery requirements. ChangesRuntime contracts and qualification
Local-file providers
Package wiring and delivery
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 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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: d17a269c0b
ℹ️ 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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
tools/repo-guard/bin/check-package-boundaries.mjs-203-211 (1)
203-211: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThe dist check accepts traversal outside
dist.
startsWith('./dist/')accepts./dist/../src/internal.js. The error text states that exports must resolve only from dist, but the check does not prevent escape. Reject any target that contains a..segment.Also note two fail-closed effects of
plainExportMap: a nested conditional export map (for example{ "./x": { "import": "./dist/x.js" } }) and anulltarget both produce the "exports must resolve only from dist" error. Confirm that no package needs those forms.🛡️ Proposed fix
+const distTarget = (value) => value.startsWith('./dist/') && !value.split('/').includes('..'); + function plainExportMap(value) { return ( typeof value === 'object' && value !== null && !Array.isArray(value) && Object.values(value).every((entry) => typeof entry === 'string') ); }if ( manifest.exports && !( - (typeof manifest.exports === 'string' && manifest.exports.startsWith('./dist/')) || - (plainExportMap(manifest.exports) && - Object.values(manifest.exports).every((value) => value.startsWith('./dist/'))) + (typeof manifest.exports === 'string' && distTarget(manifest.exports)) || + (plainExportMap(manifest.exports) && Object.values(manifest.exports).every(distTarget)) ) )Also applies to: 236-243
🤖 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 `@tools/repo-guard/bin/check-package-boundaries.mjs` around lines 203 - 211, Update the dist-target validation in the manifest exports check to reject any export target containing a parent-directory (“..”) path segment, not merely targets prefixed with “./dist/”, so paths cannot escape dist. Preserve the existing fail-closed behavior of plainExportMap for nested conditional maps and null targets, and verify that no supported package relies on those forms.docs/delivery/README.md-102-103 (1)
102-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the repeated "either".
The sentence uses "either" twice for a three-part condition. Use "any" for both parts.
📝 Proposed wording
-- Complete GF-023 and GF-024 from those current qualification facts. Phase 3 is not ready while - either external qualification task or either remaining Phase 2 story is incomplete. +- Complete GF-023 and GF-024 from those current qualification facts. Phase 3 is not ready while + any external qualification task or any remaining Phase 2 story is incomplete.🤖 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 `@docs/delivery/README.md` around lines 102 - 103, Update the Phase 3 readiness sentence in the delivery documentation to replace both uses of “either” with “any,” preserving the existing three-part incompleteness condition and wording otherwise.Source: Linters/SAST tools
packages/local-file-providers/tests/local-file-provider.test.mjs-6-15 (1)
6-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that
encodeSourceRequestsucceeded in the helper.The helper reads
.valuewithout checkingok. IfencodeSourceRequestrejects an out-of-rangedeadlineorlimititself,request(...)returnsundefined. The negative assertions at Line 41 then pass because the frame isundefined, not becausevalidateStructuredFileSourceRequestenforced its bounds. Assert the encode result so each case exercises the adapter bound.💚 Proposed fix
-const request = (deadline, limit) => - runtime.encodeSourceRequest({ - version: 'jig.source.v1', - sourceIdentity: 'source/structured-json-file-source', - basis: { track: 'track/one' }, - track: 'track/one', - deadline, - retry: { ordinal: 0, limit }, - predecessor: null, - }).value; +const request = (deadline, limit) => { + const encoded = runtime.encodeSourceRequest({ + version: 'jig.source.v1', + sourceIdentity: 'source/structured-json-file-source', + basis: { track: 'track/one' }, + track: 'track/one', + deadline, + retry: { ordinal: 0, limit }, + predecessor: null, + }); + assert.equal(encoded.ok, true); + return encoded.value; +};If
encodeSourceRequestlegitimately rejects these values, split the suite: assert the encoder failure for those inputs, and keepvalidateStructuredFileSourceRequestcases that the encoder accepts.🤖 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 `@packages/local-file-providers/tests/local-file-provider.test.mjs` around lines 6 - 15, Update the request helper around encodeSourceRequest to assert its result is successful before reading .value, so rejected deadline or limit inputs fail the test instead of producing undefined. If the encoder rejects those boundary inputs, separate the encoder-failure assertions from validator cases using values encodeSourceRequest accepts.packages/runtime-contracts/package.json-6-9 (1)
6-9: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winEnforce the qualification trust boundary for relative imports.
src/index.tsdoes not re-exportexecutionClaimsorcertificateClaims. The allow-list restricts the friend subpath tojig-conformance, andpnpm checkruns the guard. However, the guard scans onlysrc/**/*.[cm]?tsand skips all relative imports. This test bypasses the friend-subpath check with../../runtime-contracts/dist/qualification-registry.js. Replace this access with an approved friend helper, or extend the guard and document this deliberate white-box exception.🤖 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 `@packages/runtime-contracts/package.json` around lines 6 - 9, Enforce the qualification trust boundary by updating packages/conformance/tests/conformance.test.mjs:620-622 to use the approved qualification friend helper/subpath instead of the relative qualification-registry dist import. packages/runtime-contracts/package.json:6-9 and packages/runtime-contracts/src/qualification-certificate.ts:1-1 require no direct change unless the helper is not currently exported; in that case, expose the approved helper through the existing qualification-certificate entry point rather than permitting the relative import.packages/runtime-contracts/tests/source-plan-public.test.mjs-8-14 (1)
8-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test does not prove special-key rejection.
Adding a key only to
policy.capacitiesmakes the capacity map larger than the reserve map.parsePlanthen fails on the cardinality ruleObject.keys(capacities).length !== Object.keys(reserves).length, before any key name is examined. The assertion therefore passes for every added key name, including a benign one such ascpu2.To test the key-name rule, add the matching
reservesentry. Be aware thatconstructormatches the current resource-key regex/^[a-z][a-z0-9-]{0,31}$/uinpackages/runtime-contracts/src/source.tsLine 291, so the strengthened test states the intended contract and may require an implementation change.💚 Proposed stronger test
for (const key of ['__proto__', 'constructor', 'prototype']) { const value = structuredClone(plan); Object.defineProperty(value.policy.capacities, key, { value: 2, enumerable: true }); + Object.defineProperty(value.policy.reserves, key, { value: 1, enumerable: true }); assert.equal(runtime.validateSourcePlan(value).ok, 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 `@packages/runtime-contracts/tests/source-plan-public.test.mjs` around lines 8 - 14, Update the test around “R03 second-review RED” to add a matching entry in policy.reserves for each injected special key, keeping capacities and reserves cardinalities equal so validation reaches key-name checking. Ensure the test still expects validation to fail for __proto__, constructor, and prototype, and update the source-plan key validation used by parsePlan if needed so all three special keys are rejected despite the current resource-key regex.packages/runtime-contracts/tests/provider-admission.test.mjs-74-83 (1)
74-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTwo assertions pass for a reason other than the stated one.
Line 80 sets
maxAgeMs: 1whileobservedAt - stored.observedAtis 100. The call therefore fails the age check as well as the non-positive outcome check, so the assertion does not isolate the outcome rule.Line 82 passes only two keys to
admit.fields(input, ['basis','maxAgeMs','observedAt','proof'])returnsundefinedfor that shape, so the result isINVALID_ADMISSION, not the intended capability mismatch. Thebasis-mismatch path stays uncovered.Assert the error codes so each rule is checked separately.
💚 Proposed fix
- assert.equal(bounded.admit({ basis, proof: proof.value, observedAt: 1_200, maxAgeMs: 1 }).ok, false, outcome); + assert.deepEqual( + bounded.admit({ basis, proof: proof.value, observedAt: 1_200, maxAgeMs: 86_400_000 }), + { ok: false, error: { family: 'FC-AUTHORITY', code: 'POSITIVE_EXACT_PROOF_REQUIRED' } }, + outcome, + ); } - assert.equal(fixture.admit({ basis: { ...basis, capability: 'capability/other' }, proof: started.value }).ok, false); + assert.deepEqual( + fixture.admit({ + basis: { ...basis, capability: 'capability/other' }, + proof: started.value, + observedAt: 1_200, + maxAgeMs: 86_400_000, + }), + { ok: false, error: { family: 'FC-AUTHORITY', code: 'POSITIVE_EXACT_PROOF_REQUIRED' } }, + );🤖 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 `@packages/runtime-contracts/tests/provider-admission.test.mjs` around lines 74 - 83, Update the admission assertions in the provider admission test to isolate each rule and verify its error code: use a maxAgeMs value that permits the 100ms age difference when testing negative, timeout, and exhausted outcomes, then assert the corresponding non-positive-outcome rejection code. For the capability mismatch case, pass a complete admission input including proof, observedAt, and maxAgeMs, and assert the capability-mismatch error code rather than only checking ok is false.
🧹 Nitpick comments (27)
packages/local-file-providers/tests/local-file-provider.test.mjs (1)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the inclusive boundary cases.
validateStructuredFileSourceRequestacceptsSOURCE_WAIT_MIN_MS,SOURCE_WAIT_MAX_MS,SOURCE_RETRY_MIN, andSOURCE_RETRY_MAX. The suite tests only the values just outside those bounds. Add the exact accepted endpoints so an off-by-one change to a comparison operator fails the suite.💚 Proposed addition
test('the adapter boundary retains the fixed wait and retry ranges', () => { assert.equal(provider.validateStructuredFileSourceRequest(request(900_000, 3)).ok, true); + for (const [wait, retry] of [ + [5_000, 3], + [7_200_000, 3], + [900_000, 1], + [900_000, 5], + ]) + assert.equal(provider.validateStructuredFileSourceRequest(request(wait, retry)).ok, true); for (const [wait, retry] of [🤖 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 `@packages/local-file-providers/tests/local-file-provider.test.mjs` around lines 33 - 42, Add inclusive boundary assertions to the test named “the adapter boundary retains the fixed wait and retry ranges” by checking requests using SOURCE_WAIT_MIN_MS, SOURCE_WAIT_MAX_MS, SOURCE_RETRY_MIN, and SOURCE_RETRY_MAX, and assert each is accepted. Keep the existing just-outside rejection cases unchanged.packages/local-file-providers/src/index.ts (2)
171-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the terminal expression; the trailing-input branch is unreachable.
At Line 173 both
parsed === truebranches returnundefined, so theindex === text.lengthtest never changes the result. Trailing input is still rejected later byJSON.parseinstrictJson, so behavior is correct, but the expression suggests a check that does not exist.♻️ Proposed simplification
- const parsed = value(0); - whitespace(); - return parsed === true && index === text.length ? undefined : parsed === true ? undefined : parsed || undefined; + const parsed = value(0); + // Trailing input and other syntax faults are rejected by JSON.parse in strictJson. + return typeof parsed === 'string' ? parsed : undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-file-providers/src/index.ts` around lines 171 - 173, In the parser return logic around value and strictJson, remove the redundant index === text.length condition and collapse the terminal expression to return parsed when truthy and undefined otherwise, preserving JSON.parse’s later rejection of trailing input.
247-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth branches return the same failure, so the gate result is discarded.
structuredFileProviderGateis evaluated at Line 247, but Lines 263-264 return an identicalPROVIDER_UNAVAILABLE_UNQUALIFIEDfailure for both the failed and the successful gate. Theif (!gate.ok)check has no observable effect. If the intent is to prove the exact manifest binding on every call, keep the call and add a comment that states the check is assertion-only. If the intent is to distinguish an invalid binding from an unqualified provider, return distinct codes.♻️ Option: distinguish binding failure from unqualified state
- if (!gate.ok) return fail('FC-MECHANISM', 'PROVIDER_UNAVAILABLE_UNQUALIFIED'); - return fail('FC-MECHANISM', 'PROVIDER_UNAVAILABLE_UNQUALIFIED'); + if (!gate.ok) return fail('FC-AUTHORITY' as never, 'EXACT_MANIFEST_BINDING_REQUIRED'); + // Eligibility proven; reachability stays closed until live qualification. + return fail('FC-MECHANISM', 'PROVIDER_UNAVAILABLE_UNQUALIFIED');Note that the second option changes the failure family, and
packages/local-file-providers/tests/local-file-provider.test.mjsLines 17-21 assert the current shape. Confirm the intended contract before you change it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-file-providers/src/index.ts` around lines 247 - 264, Resolve the redundant result handling after structuredFileProviderGate: either document the assertion-only intent and remove the ineffective if branch while preserving the existing failure contract, or return a distinct binding-failure code for !gate.ok and retain PROVIDER_UNAVAILABLE_UNQUALIFIED only for successful qualification. Use the gate result and update the related local-file-provider tests if the failure shape changes.tools/repo-guard/tests/check-package-boundaries.test.mjs (1)
140-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive test for the permitted friend subpath.
The test proves the deny path. The allow path, where
friendSubpaths[specifier] === manifest.name, stays untested. A regression that denies the owner package would pass this suite.💚 Proposed additional test
+test('permits the qualification friend subpath from conformance', () => { + const errors = withPackages((root) => + writeFileSync( + join(root, 'packages', 'conformance', 'src', 'index.ts'), + "import '`@agentic-workflow-kit/jig-runtime-contracts/qualification-certificate`';\n", + ), + ); + assert.equal( + errors.some((error) => error.includes('restricted friend subpath')), + 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 `@tools/repo-guard/tests/check-package-boundaries.test.mjs` around lines 140 - 148, Add a positive test alongside the existing restricted-subpath test that configures or uses a package whose manifest name matches the relevant friendSubpaths owner, imports the qualification-certificate subpath from that package, and asserts no boundary errors are returned. Keep the existing deny-path coverage unchanged and exercise the friendSubpaths[specifier] === manifest.name allow condition.packages/conformance/tests/conformance.test.mjs (3)
645-651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion message claims more coverage than the test provides.
Line 645 removes one field,
candidateTree. The message at Line 649 states that every exact conformance-subject field is required before carrier registration. The test verifies that claim for one field only. A regression that drops the check onmanifestDigest,topologyVersion, orrecordedAtstill passes.Loop over the subject field names and assert rejection for each omission. That also removes the need for the lint-appeasement assertion at Line 651.
💚 Proposed per-field coverage
- const { candidateTree, ...incompleteSubject } = exactClaims.subject; - assert.equal( - friend.recordExactStructuredFileExecution({ ...exactClaims, subject: incompleteSubject }), - undefined, - 'every exact conformance-subject field is required before carrier registration', - ); - assert.equal(candidateTree, hash); + for (const field of Object.keys(exactClaims.subject)) { + const { [field]: removed, ...incompleteSubject } = exactClaims.subject; + assert.equal( + friend.recordExactStructuredFileExecution({ ...exactClaims, subject: incompleteSubject }), + undefined, + `omitting ${field} must prevent carrier registration`, + ); + }🤖 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 `@packages/conformance/tests/conformance.test.mjs` around lines 645 - 651, Update the test around recordExactStructuredFileExecution to iterate over every exactClaims.subject field, omit each field individually, and assert that each incomplete subject is rejected before carrier registration. Preserve the existing assertion message, and remove the candidateTree-specific destructuring and lint-appeasement assertion.
494-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdentify the replacement case in the assertion message.
The loop tries four substitutions per port. Every iteration uses the same message,
${port}. A failure therefore does not say whether removal, the shallow clone, the altered key, or the cross-port record stopped failing the gate.Name each case, and assert the reason string so that the gate fails for the intended cause rather than an unrelated one.
♻️ Proposed labelled cases
- for (const replacement of [undefined, { ...record }, { ...record, key: `${record.key}/altered` }, crossPort]) { + const cases = [ + ['removed', undefined], + ['cloned', { ...record }], + ['altered-key', { ...record, key: `${record.key}/altered` }], + ['cross-port', crossPort], + ]; + for (const [label, replacement] of cases) { const mutated = [...complete]; if (replacement === undefined) mutated.splice(index, 1); else mutated[index] = replacement; - assert.equal(conformance.evaluateProduct(mutated, routes, providerSubject).passed, false, `${port}`); + assert.equal(conformance.evaluateProduct(mutated, routes, providerSubject).passed, false, `${port}:${label}`); }🤖 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 `@packages/conformance/tests/conformance.test.mjs` around lines 494 - 503, Update the replacement loop in the observed-record mutation test to associate each substitution with a distinct case label, including removal, shallow clone, altered key, and cross-port record. Use the label in the assertion message and assert the expected reason string so each iteration verifies the gate fails for its intended cause rather than only checking passed is false.
569-577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd two missing negative cases for
evaluateProvider.The forged-record case at Lines 572-573 covers the identity check well. Two branches of
evaluateProviderstay untested:
- A subject whose
recorderIdentityis notrecorder/jig-conformance/v1. That is the first half of the condition atpackages/conformance/src/index.tsLine 812.- An input that holds both the observation and its source record. The source record is not in
providerRecorderEvidence, so the gate must fail. This case also pins the behavior discussed forpackages/conformance/src/index.tsLines 632-641, where the observation and its source sharekeyandbytes.💚 Proposed added cases
const forged = { ...observation.record, independentRecorder: 'recorder/jig-conformance/v1' }; assert.equal(conformance.evaluateProvider('PORT-SOURCE', [forged], providerSubject).passed, false); + assert.equal( + conformance.evaluateProvider('PORT-SOURCE', [observation.record], { ...providerSubject, recorderIdentity: 'independent' }) + .passed, + false, + 'a subject recorder identity other than the conformance recorder must fail', + ); + assert.equal( + conformance.evaluateProvider('PORT-SOURCE', [observation.record, source[0]], providerSubject).passed, + false, + 'an unbranded source record alongside the observation must fail', + ); assert.equal(🤖 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 `@packages/conformance/tests/conformance.test.mjs` around lines 569 - 577, Add two negative assertions covering evaluateProvider: use a provider subject with a recorderIdentity other than recorder/jig-conformance/v1, and evaluate an input containing both the observation and its source record so the providerRecorderEvidence gate rejects it. Add these cases alongside the existing PORT-SOURCE forged-record assertions and verify each result has passed equal to false.packages/conformance/src/structured-file-qualification.ts (2)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the exact qualification constants instead of repeating the literals.
resourceDigest,capability, andpolicyMinimumare literal here, andsnapshotQualificationClaimsinpackages/runtime-contracts/src/qualification-registry.tscompares against the same three literals. The test atpackages/conformance/tests/conformance.test.mjsLines 596 and 641 repeats them again. A change to any one copy makesrecordExactStructuredFileExecutionreturnundefined, and the qualification then fails with no message that names the mismatch.Export the expected values from the qualification registry and import them 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 `@packages/conformance/src/structured-file-qualification.ts` around lines 14 - 19, Export the shared qualification constants used by snapshotQualificationClaims in qualification-registry.ts, then import and use those constants for resourceDigest, capability, and policyMinimum in recordExactStructuredFileExecution instead of duplicating literals. Update the conformance test references to consume the same exported values so all qualification checks remain synchronized.
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
statuscheck is unreachable.
observeProvidersetsstatus: 'pass'on the record it returns (packages/conformance/src/index.tsLine 636). Whenobserved.okistrue,observed.record.status !== 'pass'is therefore alwaysfalse. Keep the check only if you intend it as a guard against a future change inobserveProvider, and record that intent in a comment.🤖 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 `@packages/conformance/src/structured-file-qualification.ts` around lines 12 - 13, The status check in the structured-file qualification flow is redundant because observeProvider currently guarantees pass status. Update the condition near observeProvider to remove observed.record.status !== 'pass', or retain it only with a nearby comment explicitly documenting it as a future-change guard.packages/runtime-contracts/src/qualification-certificate.ts (1)
15-23: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider consuming the carrier when a certificate is minted.
mintQualificationCertificatereads the carrier but never removes it fromexecutionClaims. One recorded execution can therefore mint an unlimited number of independent certificates. If a certificate is meant to attest one qualification execution, delete the carrier entry after a successful mint.If repeated minting is intentional, record that decision in the comment at Lines 3-6.
♻️ Proposed single-use carrier
const input = executionClaims.get(carrier); const snapshot = snapshotQualificationClaims(input); if (!snapshot) return undefined; + executionClaims.delete(carrier); const certificate = Object.freeze({});🤖 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 `@packages/runtime-contracts/src/qualification-certificate.ts` around lines 15 - 23, Update mintQualificationCertificate to consume the carrier after a successful certificate mint by deleting its entry from executionClaims once the snapshot is validated and certificateClaims is populated. Preserve the existing undefined returns for invalid carriers or missing snapshots; only successfully minted certificates should make the carrier unusable for later minting.packages/runtime-contracts/tests/fixtures/hostile-source-corpus.json (1)
2-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the hostile corpus to the defenses the contracts actually implement.
The four frames cover truncation, a duplicate key, and two version mismatches. The source and envelope contracts also defend against prototype-polluting keys, excessive nesting, oversized strings, non-object roots, and trailing content. No frame exercises those paths, so a regression in them stays invisible.
Add frames such as a
__proto__key, a deeply nested payload beyond the depth limit, a bare array root, and a frame with trailing bytes after the closing brace.🤖 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 `@packages/runtime-contracts/tests/fixtures/hostile-source-corpus.json` around lines 2 - 7, Extend the hostile corpus in the frames fixture with cases for prototype-polluting keys, nesting beyond the supported depth limit, oversized strings, a non-object root such as a bare array, and trailing bytes after a valid closing brace. Keep the existing truncation, duplicate-key, and version-mismatch frames unchanged, and ensure each added frame directly exercises the corresponding source or envelope contract defense.packages/conformance/src/index.ts (1)
811-820: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the provenance check depend on identity explicitly, and reject an empty match set.
Two properties of this expression are implicit:
- The
.everycallback callsproviderRecorderEvidence.has(record)on elements returned bysnapshotArray(records). The check only works ifsnapshotArraypreserves object identity. The test atpackages/conformance/tests/conformance.test.mjsLine 569 depends on that. IfsnapshotArrayis later changed to copy elements, the gate can never pass. The failure is closed, not open, but the coupling is invisible at this call site..everyreturnstruefor an empty array. Ifrecordsholds no record ofsuite, the provenance check adds no reason.suiteGateat Line 797 still pushesmissing:${suite}, so the gate does not pass. The provenance check itself is vacuous in that case.Iterate the raw
recordsfor the WeakSet check, and require at least one match.♻️ Proposed explicit provenance check
- if ( - expectedSubject.recorderIdentity !== PROVIDER_RECORDER_ID || - !snapshotArray(records) - ?.filter((record) => { - const snapshot = snapshotRecord(record); - return snapshot?.suite === suite; - }) - .every((record) => typeof record === 'object' && record !== null && providerRecorderEvidence.has(record)) - ) - reasons.push('missing:independent-recorder-provenance'); + // The WeakSet lookup requires the original object identity, so iterate `records` directly. + const forSuite = (Array.isArray(records) ? records : []).filter( + (record) => snapshotRecord(record)?.suite === suite, + ); + if ( + expectedSubject.recorderIdentity !== PROVIDER_RECORDER_ID || + forSuite.length === 0 || + !forSuite.every( + (record) => typeof record === 'object' && record !== null && providerRecorderEvidence.has(record), + ) + ) + reasons.push('missing:independent-recorder-provenance');🤖 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 `@packages/conformance/src/index.ts` around lines 811 - 820, Update the provenance condition in the suite gate to inspect raw records directly rather than relying on snapshotArray, preserving object identity for providerRecorderEvidence.has. Collect or track records whose snapshotRecord suite matches suite, and require at least one matching record with every match being a valid object present in providerRecorderEvidence. Keep the existing expectedSubject.recorderIdentity check and missing:independent-recorder-provenance reason.packages/runtime-contracts/tests/fixtures/provider-authority-manifest.wire (1)
1-1: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the wire fixture from the JSON fixture.
JSON.parseaccepts the trailing newline, but the test hashes and compares exact bytes, so the newline is intentional. Generate the canonical wire bytes fromprovider-authority-manifest.json, including its final newline, instead of maintaining duplicate fixtures.🤖 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 `@packages/runtime-contracts/tests/fixtures/provider-authority-manifest.wire` at line 1, Replace the manually maintained wire content in provider-authority-manifest.wire with an automatically derived fixture from provider-authority-manifest.json, preserving canonical JSON bytes and the source file’s trailing newline. Update the relevant fixture-generation or test setup so exact-byte hashing and comparison continue to use the generated wire representation rather than duplicate content.packages/runtime-contracts/src/envelope.ts (1)
378-391: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShare the demand memo across top-level calls.
validateSourcePlanrejects unknown dependencies and dependency cycles, so the non-null assertion is safe for validated plans. The localmemostill repeats shared dependency traversal for each story and resource class. Hoist it outside the resource loop.🤖 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 `@packages/runtime-contracts/src/envelope.ts` around lines 378 - 391, In validateSourcePlan, create one demand memo before the RESOURCE_CLASSES loop and reuse it for every top-level demandAt call. Remove the per-call default memo allocation at the loop site while preserving demandAt’s dependency caching and existing validation assumptions.packages/runtime-contracts/tests/ledger-contract.test.mjs (1)
375-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the rejected
readPreflightinputs and the variant split.The new test covers absence, exact readback, and immutability. Two branches of
readPreflightstay uncovered: theINVALID_PREFLIGHT_READfailure for an empty key or an unknown variant, and the variant separation of the key namespace.💚 Proposed additional assertions
assert.deepEqual(ledger.readPreflight('provider/readback/1', 'start'), committed); + assert.deepEqual(ledger.readPreflight('provider/readback/1', 'result'), { + ok: true, + value: { kind: 'absent' }, + }); + for (const invalid of [ + ['', 'start'], + ['provider/readback/1', 'other'], + ]) + assert.deepEqual(ledger.readPreflight(...invalid), { + ok: false, + error: { family: 'FC-INPUT', code: 'INVALID_PREFLIGHT_READ' }, + }); });🤖 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 `@packages/runtime-contracts/tests/ledger-contract.test.mjs` around lines 375 - 392, Add assertions to the existing semantic ledger test around readPreflight to cover INVALID_PREFLIGHT_READ for both an empty key and an unknown variant, and verify that entries with the same key but different variants remain separate. Reuse the existing committed preflight setup and assert the expected failure/readback results without changing the current absence, exact-readback, or immutability coverage.packages/runtime-contracts/tests/envelope-policy.test.mjs (1)
6-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne envelope fixture is defined twice. Both test files build the same
plan,policy,profile,artifacts,setup,ruleSurface, andguidanceinput. Any change to the envelope contract requires two edits, and the copies can drift.
packages/runtime-contracts/tests/envelope-policy.test.mjs#L6-L56: movepolicyandinput()into a shared fixture module, for exampletests/fixtures/envelope-input.mjs, and import them here.packages/runtime-contracts/tests/envelope-policy-review.test.mjs#L9-L58: delete the localplan()andinput()and import the shared fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/envelope-policy.test.mjs` around lines 6 - 56, Centralize the duplicated envelope fixture in packages/runtime-contracts/tests/fixtures/envelope-input.mjs by moving the policy object and input() factory from packages/runtime-contracts/tests/envelope-policy.test.mjs lines 6-56, then import them in that test. In packages/runtime-contracts/tests/envelope-policy-review.test.mjs lines 9-58, remove the local plan() and input() definitions and import the shared fixture instead.packages/runtime-contracts/src/qualification-registry.ts (1)
55-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCommit identifiers receive no shape validation.
DIGEST_FIELDSincludescandidateTreeandexecutionBaseTree, so both are checked against/^[0-9a-f]{64}$/u.candidateCommit,executionBaseCommit, andmergeBaseCommitpass with anysafeTextvalue up to 256 characters. Git commit identifiers have the same hexadecimal shape as the tree identifiers, so the asymmetry appears unintended and it weakens the subject binding.Add the three commit fields to
DIGEST_FIELDS, or add a separate commit-shape check if abbreviated or SHA-1 identifiers must remain valid.♻️ Proposed change
const DIGEST_FIELDS = new Set<keyof QualificationSubject>([ 'buildDigest', + 'candidateCommit', 'candidateContentDigest', 'candidateTree', 'catalogDigest', 'environmentDigest', + 'executionBaseCommit', 'executionBaseTree', 'fixtureDigest', 'manifestDigest', + 'mergeBaseCommit', 'providerBuildDigest', 'toolchainDigest', ]);Also applies to: 109-113
🤖 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 `@packages/runtime-contracts/src/qualification-registry.ts` around lines 55 - 66, Extend validation for the commit identifier fields in the qualification subject schema: update DIGEST_FIELDS to include candidateCommit, executionBaseCommit, and mergeBaseCommit so they receive the same hexadecimal shape validation as tree identifiers, unless the implementation explicitly supports abbreviated or SHA-1 forms and therefore requires a separate commit-specific check.packages/runtime-contracts/src/source.ts (3)
124-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider aligning
exactwithentryFieldsprototype checks.
exactaccepts any non-array object.entryFieldsadditionally requiresObject.getPrototypeOf(value) === Object.prototypeand descriptor-safe fields. Today everyexactcaller receives values thatdecodeFrameorsnapshotproduced, so the input is already a plain data object. The weaker check is safe now, but it becomes a hazard if a future caller passes raw caller-owned input toexactdirectly. Add the prototype guard, or document the precondition onexact.🤖 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 `@packages/runtime-contracts/src/source.ts` around lines 124 - 130, Strengthen exact’s object validation to match entryFields by requiring Object.getPrototypeOf(value) === Object.prototype before accepting the record. Keep the existing array, null, and key-set checks unchanged, ensuring non-plain or caller-owned objects return undefined.
467-479: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFreeze nested decoded payloads before returning them.
frozenfreezes only the top level.basis(Line 471) andcontent(Line 559) keep the decoded object graph mutable. A caller can mutaterequest.basis.trackorexchange.content.titleafter validation, so the returned value no longer matchesrequestBasisDigestorcontentDigest. The tests assertObject.isFrozen(result.value)andObject.isFrozen(result.value.plan)only, so this gap is not covered.Add a deep freeze for the decoded subtrees.
♻️ Proposed deep-freeze helper
const frozen = <T>(value: T): T => Object.freeze(value); +const deepFrozen = <T>(value: T): T => { + if (typeof value !== 'object' || value === null) return value; + for (const key of Object.keys(value as object)) + deepFrozen((value as Record<string, unknown>)[key]); + return Object.freeze(value); +};Then use
deepFrozen(raw.basis)anddeepFrozen(raw.content)when building the returned request and exchange.Also applies to: 550-575
🤖 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 `@packages/runtime-contracts/src/source.ts` around lines 467 - 479, Deep-freeze the decoded nested payloads before returning them: update the request construction around sourceIdentity to use deepFrozen(raw.basis), and the exchange construction around content to use deepFrozen(raw.content). Reuse the proposed deep-freeze helper and preserve the existing top-level frozen wrappers and digest values.
287-300: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueResource-key regex admits
constructor.
numericMapandreserveMapaccept any key that matches/^[a-z][a-z0-9-]{0,31}$/u. The stringconstructormatches.Object.fromEntriesthen creates an ownconstructordata property on the returned map. The value stays plain data, so there is no prototype pollution, and later lookups useObject.hasOwn. No functional defect exists today. If resource identifiers must exclude object-shaped names, add an explicit deny list.Also applies to: 302-306
🤖 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 `@packages/runtime-contracts/src/source.ts` around lines 287 - 300, Update the key validation in numericMap and reserveMap to explicitly reject constructor and any other object-shaped resource names required by the contract, while preserving the existing regex and amount validation. Apply the same deny list consistently in both functions before creating the frozen map.packages/local-file-providers/src/path-confinement.ts (1)
34-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
node:cryptoforsha256.
createHash('sha256')provides the same digest without maintaining a large compression implementation. This helper runs in resource-key and per-record digest paths. The package targets Node>=22and has no import policy that limits it tonode:fs. Document such a restriction besidesha256if an authority decision adds 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 `@packages/local-file-providers/src/path-confinement.ts` around lines 34 - 96, Replace the manual compression implementation in sha256 with node:crypto’s createHash('sha256') API, updating imports and returning the digest as the existing lowercase hexadecimal string. Preserve the helper’s Uint8Array input and all callers’ resource-key and per-record digest behavior; do not retain the constants, padding, or block-processing logic.packages/local-file-providers/src/local-file-artifact.ts (3)
178-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
localFileArtifactRouteduplicatesrouteContext.Lines 181-186 repeat the exact holder classification in
routeContextat lines 766-772. Two copies of the routing rule can diverge.contextForat line 347 usesrouteContext, and the exported function uses its own copy.Build the exported function on
routeContext.♻️ Proposed fix
export function localFileArtifactRoute( holder: unknown, ): LocalFileArtifactResult<Readonly<{ context: Context; root: string }>> { - const context: Context | undefined = - typeof holder === 'string' && PROTECTED.has(holder) - ? 'protected' - : typeof holder === 'string' && DISPOSABLE.has(holder) - ? 'disposable' - : undefined; + const context = routeContext(holder); return context ? ok(Object.freeze({ context, root: LOCAL_FILE_ARTIFACT_ROOTS[context] })) : fail('FC-SUBJECT', 'UNKNOWN_HOLDER_CLASS'); }🤖 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 `@packages/local-file-providers/src/local-file-artifact.ts` around lines 178 - 190, Update localFileArtifactRoute to derive its context by calling the existing routeContext helper instead of duplicating the PROTECTED and DISPOSABLE holder classification. Preserve the current success result with the resolved context and corresponding LOCAL_FILE_ARTIFACT_ROOTS entry, and retain the UNKNOWN_HOLDER_CLASS failure for undefined routing results.
175-175: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
canonicalisJSON.stringify, so it is order-sensitive and not canonical.
canonicaldecides identity in many places: the witness comparison at line 301,fact.bindingat line 316, the prior-binding comparisons at lines 417, 546, and 597, the snapshot digest at line 641, and the proof basis at line 925. Two objects with the same members in a different insertion order produce different strings.The current call sites build objects from fixed literal or
fields(...)name order, so the behavior is stable today. The name still states a guarantee that the function does not provide, and the ledger and registry paths use the codecstageDigestcanonical form instead. A later refactor that reorders one object literal changes every derived digest.Either rename it to
serialize, or route it through the same codec canonical JSON thatpackages/local-file-providers/src/local-file-registry.tsuses viastagedDigest.🤖 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 `@packages/local-file-providers/src/local-file-artifact.ts` at line 175, Resolve the misleading canonicalization in the `canonical` helper: either rename it to `serialize` and update all visible call sites, including witness, binding, prior-binding, snapshot, and proof-basis comparisons, or replace its implementation with the codec’s order-independent canonical JSON used by `stagedDigest` in the registry path. Keep identity and digest behavior consistent across these usages.
116-174: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse
node:cryptofor SHA-256.This removes the hand-written implementation and makes the digest behavior explicit. Keep the existing
node:fsimport and remove the local compression code.🤖 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 `@packages/local-file-providers/src/local-file-artifact.ts` around lines 116 - 174, Replace the hand-written SHA-256 implementation in hash with node:crypto’s SHA-256 API, preserving support for string and Uint8Array inputs and the existing hexadecimal digest output. Keep the node:fs import unchanged and remove the local constants, padding, compression, and state-processing logic.packages/local-file-providers/tests/local-file-artifact.test.mjs (1)
147-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the exported helpers
localFileArtifactRouteandvalidateLocalFileArtifactPut.
packages/local-file-providers/src/local-file-artifact.tsexports both functions at lines 178 and 193. This suite exercisescontextFor, which uses the internalrouteContext, but no test calls either exported helper.localFileArtifactRoutealso returns the selected absolute root, so a defect there is not caught by the oracle tests, which use temporary roots.Add assertions for a protected holder, a disposable holder, an unknown holder, and a
validateLocalFileArtifactPutcall with a protected holder.🤖 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 `@packages/local-file-providers/tests/local-file-artifact.test.mjs` around lines 147 - 180, Add coverage in the existing local-file artifact test suite for the exported helpers localFileArtifactRoute and validateLocalFileArtifactPut: assert protected and disposable holder routing, unknown-holder handling, and validation of a put using a protected holder, including the selected absolute root returned by localFileArtifactRoute.packages/local-file-providers/tests/local-file-ledger.test.mjs (1)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the codec through its package specifier.
Declare
@agentic-workflow-kit/jig-codecunderdevDependenciesofpackages/local-file-providers, then import it by package specifier. The test-only relative path couples the test to the codec output directory. The package-boundary checker does not inspect test 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 `@packages/local-file-providers/tests/local-file-ledger.test.mjs` around lines 17 - 25, Update the local-file-ledger test to import the codec via the `@agentic-workflow-kit/jig-codec` package specifier instead of the relative codec/dist path, and add `@agentic-workflow-kit/jig-codec` to packages/local-file-providers devDependencies. Leave the other runtime and provider imports unchanged.packages/local-file-providers/src/node.d.ts (1)
1-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the intentional omission of
@types/node.This package and the lockfile contain no
@types/nodereference. Add a short comment innode.d.tsif this dependency exclusion is intentional.🤖 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 `@packages/local-file-providers/src/node.d.ts` around lines 1 - 46, Add a brief comment at the top of node.d.ts documenting that the local Node.js declarations intentionally replace `@types/node` because the package and lockfile exclude that dependency. Leave the existing declarations unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2057d0d-9572-49a3-947d-3086c703f0e7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
docs/delivery/README.mdpackages/conformance/src/index.tspackages/conformance/src/structured-file-qualification.tspackages/conformance/tests/conformance.test.mjspackages/local-file-providers/package.jsonpackages/local-file-providers/src/index.tspackages/local-file-providers/src/local-file-artifact.tspackages/local-file-providers/src/local-file-intake.tspackages/local-file-providers/src/local-file-ledger.tspackages/local-file-providers/src/local-file-preflight.tspackages/local-file-providers/src/local-file-registry.tspackages/local-file-providers/src/local-file-snapshot.tspackages/local-file-providers/src/local-file-witness.tspackages/local-file-providers/src/node.d.tspackages/local-file-providers/src/path-confinement.tspackages/local-file-providers/tests/fixtures/local-file-artifact-routing.jsonpackages/local-file-providers/tests/fixtures/local-file-ledger-oracle.jsonpackages/local-file-providers/tests/local-file-artifact.test.mjspackages/local-file-providers/tests/local-file-ledger.test.mjspackages/local-file-providers/tests/local-file-provider.test.mjspackages/local-file-providers/tsconfig.jsonpackages/runtime-contracts/package.jsonpackages/runtime-contracts/src/envelope.tspackages/runtime-contracts/src/index.tspackages/runtime-contracts/src/ledger.tspackages/runtime-contracts/src/provider.tspackages/runtime-contracts/src/qualification-certificate.tspackages/runtime-contracts/src/qualification-registry.tspackages/runtime-contracts/src/repository-policy-catalogue.tspackages/runtime-contracts/src/source.tspackages/runtime-contracts/tests/envelope-policy-review.test.mjspackages/runtime-contracts/tests/envelope-policy.test.mjspackages/runtime-contracts/tests/fixtures/envelope-bounds-oracle.jsonpackages/runtime-contracts/tests/fixtures/hostile-source-corpus.jsonpackages/runtime-contracts/tests/fixtures/provider-authority-manifest.jsonpackages/runtime-contracts/tests/fixtures/provider-authority-manifest.wirepackages/runtime-contracts/tests/fixtures/source-contract-oracle.jsonpackages/runtime-contracts/tests/ledger-contract.test.mjspackages/runtime-contracts/tests/provider-admission.test.mjspackages/runtime-contracts/tests/source-contract.test.mjspackages/runtime-contracts/tests/source-plan-public.test.mjstools/repo-guard/bin/check-package-boundaries.mjstools/repo-guard/tests/check-package-boundaries.test.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/local-file-providers/src/path-confinement.ts (1)
390-428: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse atomic publication and close all descriptors on failure.
writeCreateOnlyJsonwrites directly to the final path. A crash beforefsyncSync(descriptor)completes can leave partial JSON at that path. A retry then returnsALREADY_EXISTS, so the record cannot be repaired. Write to a temporary file, sync it, publish it atomically without replacement, and sync the parent directory. Also closedirectoryDescriptorin afinallyblock; an exception fromfsyncSync(directoryDescriptor)currently leaks it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-file-providers/src/path-confinement.ts` around lines 390 - 428, Update writeCreateOnlyJson to stage the JSON in a uniquely named temporary file within the confined parent directory, write and fsync that file, then atomically publish it at the final path without replacement before syncing the parent directory. Track both file descriptors and the temporary path, and ensure all descriptors are closed and any unpublished temporary file is removed on every failure, including errors during directory fsync; preserve ALREADY_EXISTS mapping for an existing final path.
♻️ Duplicate comments (1)
packages/local-file-providers/src/path-confinement.ts (1)
301-374:⚠️ Potential issue | 🟠 Major
READ_FAILEDstill hides missing files behind trust and corruption failures.The single
try/catcharound Lines 304-372 maps every failure mode tofail('FC-MECHANISM', 'READ_FAILED'): a missing file (ENOENTfromlstatSync), a missing parent directory, an intentionalthrow new Error('untrusted directory')from the directory-identity check, aJSON.parsefailure, and asnapshotthrow on hostile input. The confirmed downstream consumer inpackages/local-file-providers/src/local-file-preflight.tstreats this code as absence: "const decoded = readJsonFile(root, [resourceKey(key),${variant}.json]); if (!decoded.ok) return decoded.error.code === 'READ_FAILED' ? ok(Object.freeze({ kind: 'absent' })) : decoded;" A tampered or corrupted file that fails a trust check (bad JSON, non-canonical bytes, a swapped directory) is indistinguishable from a file that was never written, so a tamperedresult.jsoncan be silently treated as absent instead of surfacing an unverifiable/trust failure.This is the same root cause flagged in the earlier review of this function (then at different line numbers); the fix has not been applied to the rewritten body.
🛡️ Proposed fix to separate absence from corruption/trust failures
let descriptor: number | undefined; try { const directoryParts = parts.slice(0, -1); - const directories = [ - root, - ...directoryParts.map((_, index) => `${root}/${directoryParts.slice(0, index + 1).join('/')}`), - ].map((directory) => { - const stat = lstatSync(directory); - if (!stat.isDirectory() || stat.isSymbolicLink() || realpathSync(directory) !== directory) - throw new Error('untrusted directory'); - return Object.freeze({ directory, dev: stat.dev, ino: stat.ino }); - }); - const pathStat = lstatSync(path.value); + let directories: ReadonlyArray<{ directory: string; dev: number; ino: number }>; + let pathStat: ReturnType<typeof lstatSync>; + try { + directories = [ + root, + ...directoryParts.map((_, index) => `${root}/${directoryParts.slice(0, index + 1).join('/')}`), + ].map((directory) => { + const stat = lstatSync(directory); + if (!stat.isDirectory() || stat.isSymbolicLink() || realpathSync(directory) !== directory) + throw Object.assign(new Error('untrusted directory'), { untrusted: true }); + return Object.freeze({ directory, dev: stat.dev, ino: stat.ino }); + }); + pathStat = lstatSync(path.value); + } catch (error) { + const code = typeof error === 'object' && error !== null && 'code' in error ? (error as { code?: unknown }).code : undefined; + if ((error as { untrusted?: boolean })?.untrusted) return fail('FC-TRUST', 'UNTRUSTED_PATH_COMPONENT'); + return fail('FC-MECHANISM', code === 'ENOENT' ? 'FILE_ABSENT' : 'READ_FAILED'); + } if ( !pathStat.isFile() || @@ closeSync(descriptor); descriptor = undefined; const text = new TextDecoder('utf-8', { fatal: true }).decode(bytes); if (!text.endsWith('\n')) return fail('FC-TRUST', 'UNTRUSTED_FILE'); - const value = snapshot(JSON.parse(text)); - return `${canonicalText(value)}\n` === text ? ok(value) : fail('FC-TRUST', 'UNTRUSTED_FILE'); + try { + const value = snapshot(JSON.parse(text)); + return `${canonicalText(value)}\n` === text ? ok(value) : fail('FC-TRUST', 'UNTRUSTED_FILE'); + } catch { + return fail('FC-TRUST', 'UNTRUSTED_FILE'); + } } catch { return fail('FC-MECHANISM', 'READ_FAILED'); } finally { if (descriptor !== undefined) closeSync(descriptor); }🤖 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 `@packages/local-file-providers/src/path-confinement.ts` around lines 301 - 374, Update readJsonFile so only genuine absence conditions, such as ENOENT for the target file or its parent directories, return FC-MECHANISM/READ_FAILED. Preserve and propagate FC-TRUST failures for untrusted path components, file identity changes, malformed or non-canonical JSON, and hostile snapshot input instead of allowing the broad catch to classify them as absent; narrow the catch or explicitly distinguish these failure paths while keeping descriptor cleanup intact.
🧹 Nitpick comments (1)
packages/local-file-providers/tests/local-file-ledger.test.mjs (1)
388-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the fence assertion for a repeated witness-floor advance.
The test proves that
advanceWitnessFloorrepairs a lagging witness. It does not pin the fence contract for a second call. Assert that the second call returnsFC-FENCE/WITNESS_ALREADY_CURRENT. This locks the idempotent recovery behaviour thatpackages/local-file-providers/src/local-file-registry.tsLine 242 implements.💚 Proposed additional assertion
assert.deepEqual(restored.advanceWitnessFloor(binding), { ok: true, value: undefined }); + assert.deepEqual(restored.advanceWitnessFloor(binding), { + ok: false, + error: { family: 'FC-FENCE', code: 'WITNESS_ALREADY_CURRENT' }, + }); assert.equal(restored.readback({ binding, position: 1 }).value.kind, 'committed');🤖 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 `@packages/local-file-providers/tests/local-file-ledger.test.mjs` around lines 388 - 398, Extend the test after the successful restored.advanceWitnessFloor call to invoke restored.advanceWitnessFloor(binding) a second time and assert it returns { ok: false, error: { family: 'FC-FENCE', code: 'WITNESS_ALREADY_CURRENT' } }, preserving the existing readback assertion.
🤖 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.
Outside diff comments:
In `@packages/local-file-providers/src/path-confinement.ts`:
- Around line 390-428: Update writeCreateOnlyJson to stage the JSON in a
uniquely named temporary file within the confined parent directory, write and
fsync that file, then atomically publish it at the final path without
replacement before syncing the parent directory. Track both file descriptors and
the temporary path, and ensure all descriptors are closed and any unpublished
temporary file is removed on every failure, including errors during directory
fsync; preserve ALREADY_EXISTS mapping for an existing final path.
---
Duplicate comments:
In `@packages/local-file-providers/src/path-confinement.ts`:
- Around line 301-374: Update readJsonFile so only genuine absence conditions,
such as ENOENT for the target file or its parent directories, return
FC-MECHANISM/READ_FAILED. Preserve and propagate FC-TRUST failures for untrusted
path components, file identity changes, malformed or non-canonical JSON, and
hostile snapshot input instead of allowing the broad catch to classify them as
absent; narrow the catch or explicitly distinguish these failure paths while
keeping descriptor cleanup intact.
---
Nitpick comments:
In `@packages/local-file-providers/tests/local-file-ledger.test.mjs`:
- Around line 388-398: Extend the test after the successful
restored.advanceWitnessFloor call to invoke
restored.advanceWitnessFloor(binding) a second time and assert it returns { ok:
false, error: { family: 'FC-FENCE', code: 'WITNESS_ALREADY_CURRENT' } },
preserving the existing readback assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c184fa62-95b4-4b4d-9ebb-7909194e4b12
📒 Files selected for processing (7)
packages/local-file-providers/src/local-file-intake.tspackages/local-file-providers/src/local-file-ledger.tspackages/local-file-providers/src/local-file-preflight.tspackages/local-file-providers/src/local-file-registry.tspackages/local-file-providers/src/local-file-snapshot.tspackages/local-file-providers/src/path-confinement.tspackages/local-file-providers/tests/local-file-ledger.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/local-file-providers/src/local-file-snapshot.ts
- packages/local-file-providers/src/local-file-preflight.ts
- packages/local-file-providers/src/local-file-intake.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/local-file-providers/src/path-confinement.ts (1)
420-471: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFix: a post-link cleanup failure reports a write failure even though the write already succeeded.
linkSync(temporaryFile, path.value)at Line 442 is the durability commit point: the content was already flushed and fsynced to the temp file (Line 439) before the link. OncelinkSyncreturns,path.valueexists with correct content andreadJsonFilecan read it back correctly. However, if any of the following steps throws —unlinkSync(temporaryFile),rmdirSync(temporaryDirectory),openSync(parent, ...),fsyncSync(directoryDescriptor), orcloseSync(directoryDescriptor)— the function falls into thecatchblock and returnsfail('FC-MECHANISM', 'WRITE_FAILED')(Line 471), misreporting a successful write as failed.This is not a purely theoretical corner case for this call site.
local-file-ledger.tsandlocal-file-registry.tscallwriteCreateOnlyJsonforrecordsand immediately propagate any failure (if (!stored.ok) return stored;), without theALREADY_EXISTStolerance used forintentswrites. A spurious failure here surfaces as a hard error to the ledger/registry caller for an append that actually completed.Track whether the link already succeeded, and treat later cleanup failures as non-fatal for the return value.
🛠️ Proposed fix to stop reporting failure after a successful link
let descriptor: number | undefined; let directoryDescriptor: number | undefined; let temporaryFile: string | undefined; let temporaryDirectory: string | undefined; + let linked = false; try { ... linkSync(temporaryFile, path.value); + linked = true; unlinkSync(temporaryFile); temporaryFile = undefined; rmdirSync(temporaryDirectory); temporaryDirectory = undefined; const parent = path.value.slice(0, path.value.lastIndexOf('/')); directoryDescriptor = openSync(parent, constants.O_RDONLY | constants.O_DIRECTORY); fsyncSync(directoryDescriptor); closeSync(directoryDescriptor); directoryDescriptor = undefined; return ok(undefined); } catch (error) { for (const open of [descriptor, directoryDescriptor]) { if (open === undefined) continue; try { closeSync(open); } catch {} } if (temporaryFile !== undefined) { try { unlinkSync(temporaryFile); } catch {} } if (temporaryDirectory !== undefined) { try { rmdirSync(temporaryDirectory); } catch {} } + if (linked) return ok(undefined); const code = errorCode(error); return fail(code === 'EEXIST' ? 'FC-FENCE' : 'FC-MECHANISM', code === 'EEXIST' ? 'ALREADY_EXISTS' : 'WRITE_FAILED'); }🤖 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 `@packages/local-file-providers/src/path-confinement.ts` around lines 420 - 471, Update the write flow around linkSync in writeCreateOnlyJson to track whether the durability commit has succeeded. Once linkSync returns, preserve a successful result even if temporary-file cleanup or parent-directory fsync/close operations throw; retain cleanup attempts and existing error reporting for failures that occur before the link succeeds.
🧹 Nitpick comments (1)
packages/local-file-providers/src/path-confinement.ts (1)
408-473: 🩺 Stability & Availability | 🔵 TrivialVerify staging-directory reconciliation on startup.
If the process crashes between
mkdtempSync(Line 430) and the final cleanup, or betweenlinkSync(Line 442) andrmdirSync(Line 445), an orphanedstaging/write-*directory and payload file remain on disk. Nothing in this file reconciles or prunes stale staging directories on startup. Over repeated crash/retry cycles this accumulates disk usage under the confined root.Confirm whether a startup or periodic reconciliation pass removes stale entries under
staging, similar to the reconciliation described for the artifact provider in this PR.🤖 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 `@packages/local-file-providers/src/path-confinement.ts` around lines 408 - 473, Add startup or periodic reconciliation for the staging area used by writeCreateOnlyJson, removing stale staging/write-* directories and their payload files left by interrupted writes while preserving active entries. Anchor the implementation to writeCreateOnlyJson and the existing initialization or reconciliation flow in this module, and ensure cleanup remains confined to the configured root.
🤖 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.
Outside diff comments:
In `@packages/local-file-providers/src/path-confinement.ts`:
- Around line 420-471: Update the write flow around linkSync in
writeCreateOnlyJson to track whether the durability commit has succeeded. Once
linkSync returns, preserve a successful result even if temporary-file cleanup or
parent-directory fsync/close operations throw; retain cleanup attempts and
existing error reporting for failures that occur before the link succeeds.
---
Nitpick comments:
In `@packages/local-file-providers/src/path-confinement.ts`:
- Around line 408-473: Add startup or periodic reconciliation for the staging
area used by writeCreateOnlyJson, removing stale staging/write-* directories and
their payload files left by interrupted writes while preserving active entries.
Anchor the implementation to writeCreateOnlyJson and the existing initialization
or reconciliation flow in this module, and ensure cleanup remains confined to
the configured root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a43d67ec-b1a1-4c51-9019-54482ce765e3
📒 Files selected for processing (25)
docs/delivery/README.mdpackages/conformance/tests/conformance.test.mjspackages/local-file-providers/src/index.tspackages/local-file-providers/src/local-file-artifact.tspackages/local-file-providers/src/local-file-intake.tspackages/local-file-providers/src/local-file-ledger.tspackages/local-file-providers/src/local-file-preflight.tspackages/local-file-providers/src/local-file-registry.tspackages/local-file-providers/src/local-file-witness.tspackages/local-file-providers/src/node.d.tspackages/local-file-providers/src/path-confinement.tspackages/local-file-providers/tests/fixtures/local-file-ledger-oracle.jsonpackages/local-file-providers/tests/local-file-artifact.test.mjspackages/local-file-providers/tests/local-file-ledger.test.mjspackages/local-file-providers/tests/local-file-provider.test.mjspackages/runtime-contracts/src/envelope.tspackages/runtime-contracts/src/provider.tspackages/runtime-contracts/src/qualification-certificate.tspackages/runtime-contracts/src/qualification-registry.tspackages/runtime-contracts/src/source.tspackages/runtime-contracts/tests/envelope-policy.test.mjspackages/runtime-contracts/tests/provider-admission.test.mjspackages/runtime-contracts/tests/source-plan-public.test.mjstools/repo-guard/bin/check-package-boundaries.mjstools/repo-guard/tests/check-package-boundaries.test.mjs
🚧 Files skipped from review as they are similar to previous changes (20)
- docs/delivery/README.md
- packages/runtime-contracts/tests/source-plan-public.test.mjs
- packages/local-file-providers/tests/fixtures/local-file-ledger-oracle.json
- packages/local-file-providers/tests/local-file-provider.test.mjs
- packages/conformance/tests/conformance.test.mjs
- packages/local-file-providers/src/local-file-witness.ts
- packages/local-file-providers/src/local-file-preflight.ts
- packages/runtime-contracts/src/qualification-certificate.ts
- packages/runtime-contracts/tests/provider-admission.test.mjs
- packages/local-file-providers/tests/local-file-artifact.test.mjs
- tools/repo-guard/bin/check-package-boundaries.mjs
- packages/local-file-providers/tests/local-file-ledger.test.mjs
- packages/local-file-providers/src/local-file-intake.ts
- packages/local-file-providers/src/node.d.ts
- packages/local-file-providers/src/index.ts
- packages/local-file-providers/src/local-file-ledger.ts
- packages/runtime-contracts/src/envelope.ts
- packages/local-file-providers/src/local-file-registry.ts
- packages/runtime-contracts/src/source.ts
- packages/local-file-providers/src/local-file-artifact.ts
1 participant
Summary
Verification
pnpm delivery:check— PASS (48 stories, 7 phases)pnpm check— PASS (21/21 tasks)git diff --check— PASS34f29484a29436447c76f381e2f9c940493bfd05Required before Phase 3
JIG_DATA_HOMEin the execution environment, provision${JIG_DATA_HOME}/work-sources/work-plan.json, and record current exact GF-020 source/provider qualification evidenceJIG_WITNESS_ROOTto an absolute canonical independently administered mount, verify backup/filesystem separation, and record current GF-025/GF-026 qualification evidenceThe durable task record is in
docs/delivery/README.md. This PR intentionally keeps all unqualified providers unavailable and does not claim that the Phase 2 exit gate is closed.