feat(runtime): add Phase 3 development readiness - #124
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change separates semantic-only development from real-provider qualification. It updates delivery sequencing and dependency gates, adds a development pre-run API with exact approvals, and supports witnessed terminal rejection with replay and readback behavior. ChangesSemantic development handoff
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Arye
participant DevelopmentPreRun
participant ScriptedLedger
Arye->>DevelopmentPreRun: create semantic-only preview
Arye->>DevelopmentPreRun: submit exact approvals
DevelopmentPreRun->>ScriptedLedger: submit terminal acknowledgement
ScriptedLedger-->>DevelopmentPreRun: return witnessed intake result
DevelopmentPreRun-->>Arye: return result or readback
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: 852d3f4cb9
ℹ️ 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: 6
🧹 Nitpick comments (3)
packages/runtime-contracts/tests/development-pre-run.test.mjs (1)
155-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a manifest approval reused across a different
manifestId.The current cases cover approval substitution and copied carriers. They do not cover a manifest approval that is reused for a second preview that keeps the same
manifestDigestand changesmanifestId. That case passes today, because the approval subject omitsmanifestId. See the root cause at Lines 128-144 ofpackages/runtime-contracts/src/development-pre-run.ts. Add the case so the fix stays covered.💚 Proposed additional case
for (const forbidden of ['configureProvider', 'enableProvider', 'dispatch', 'execute']) assert.equal(forbidden in profile, false); }); + +test('a provider-manifest approval does not carry over to a different manifestId', () => { + const { profile, manifestApproval } = approvedFixture(); + const other = profile.preview({ + envelope: envelopeInput(), + providerManifest: { ...manifest(), manifestId: 'provider/development/authority/other' }, + }); + assert.equal(other.ok, true); + const proposalApproval = profile.approveProposal({ principal: 'principal/arye', preview: other.value }); + assert.equal(proposalApproval.ok, true); + assert.equal( + profile.submit({ + preview: other.value, + proposalApproval: proposalApproval.value, + manifestApproval, + terminalAck: 'accepted', + }).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/development-pre-run.test.mjs` around lines 155 - 177, Add a test case in the development intake rejection test around profile.submit that creates a second preview with the same manifestDigest but a different manifestId, reuses the original manifest approval, and asserts submission is rejected. Keep the existing approval-substitution and copied-preview cases unchanged.packages/runtime-contracts/src/development-pre-run.ts (1)
191-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn an input failure for malformed
successorCutandterminalAck.Lines 207-210 validate
terminalAckandsuccessorCutshape. A failure of these checks returnsFC-AUTHORITYwithEXACT_DEVELOPMENT_APPROVALS_REQUIRED. That message reports an authority denial for an input defect.ledger.intakereports the same defects asFC-INPUT/INVALID_INTAKE. Split the checks so the family matches the 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 `@packages/runtime-contracts/src/development-pre-run.ts` around lines 191 - 215, The submit method’s malformed successorCut or terminalAck inputs currently return the authority-denial failure; split these shape validations from the approval checks and return FC-INPUT with INVALID_INTAKE for invalid terminalAck/successorCut values, while preserving FC-AUTHORITY / EXACT_DEVELOPMENT_APPROVALS_REQUIRED for missing or unrecognized approval data.packages/local-file-providers/tests/local-file-ledger.test.mjs (1)
296-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd replay coverage for the local-file rejection path.
The test covers first-write and readback. It does not cover the new idempotency comparison at Lines 104-114 of
packages/local-file-providers/src/local-file-intake.ts. Two cases stay untested: an identical rejected replay must return the stored result, and a replay withterminalAck: 'accepted'must fail withINTAKE_REQUEST_MISMATCH. A case forterminalAck: 'rejected'with asuccessorCutmust fail withINVALID_INTAKE.💚 Proposed additional assertions
assert.equal('run' in rejected.value, false); assert.deepEqual(intake.read(digest('c')).value.result, rejected.value); + assert.deepEqual( + intake.create({ compositionDigest: digest('c'), acknowledgementDigest: digest('d'), terminalAck: 'rejected' }), + rejected, + ); + const replayed = intake.create({ + compositionDigest: digest('c'), + acknowledgementDigest: digest('d'), + terminalAck: 'accepted', + }); + assert.equal(replayed.ok, false); + assert.equal(replayed.error.code, 'INTAKE_REQUEST_MISMATCH'); + const invalid = intake.create({ + compositionDigest: digest('e'), + acknowledgementDigest: digest('f'), + terminalAck: 'rejected', + successorCut: 'cut/one', + }); + assert.equal(invalid.ok, false); + assert.equal(invalid.error.code, 'INVALID_INTAKE'); });🤖 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 296 - 316, Extend the rejection test for local-file intake to cover replay validation in createLocalFileIntakeForConformance: assert an identical terminalAck:'rejected' request returns the stored result, a replay with terminalAck:'accepted' fails with INTAKE_REQUEST_MISMATCH, and a rejected replay including successorCut fails with INVALID_INTAKE.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/delivery/greenfield/decisions.md`:
- Around line 110-115: Rewrite the paragraph so GF-020, GF-025, and GF-026 each
have an independent start condition matching track.json: GF-020 requires GF-019
and GF-022, GF-025 requires GF-010, GF-012, and GF-022, and GF-026 requires
GF-013 and GF-022. Preserve the existing provider-gate and recompose/refresh
requirements without implying that local file ledger selection scopes all three
stories.
In `@docs/delivery/greenfield/dependency-dag.md`:
- Around line 123-130: The dependency readiness guidance must evaluate each edge
according to its declared type rather than treating every dependency as a merge
requirement. In docs/delivery/greenfield/dependency-dag.md lines 123-130,
require predecessor containment for implementation edges, exact conformance for
evidence edges, recorded owner and DR basis for decision edges, and merge
containment only for merge edges. In docs/delivery/greenfield/stories/GF-023.md
lines 82-84, require GF-014, GF-019, and GF-021 as implementation predecessors
in the execution base, while requiring the GF-022 decision basis to be recorded
and revalidated rather than merged as a story.
In `@docs/delivery/greenfield/stories/GF-023.md`:
- Around line 159-161: Update the GF-023 acceptance criteria and related
readiness text to replace the obsolete six dependency evidence/gate references
with the four current dependency records, and identify the GF-022 decision
record separately from those evidence records. Ensure all graph-count references
consistently describe the current four-edge semantic-only posture.
In `@docs/delivery/greenfield/track.json`:
- Line 91: Update the track.json exit_gate machine-readable requirements to
include providerEnabled: false, dispatchEnabled: false,
fail-closed-no-autonomous-restore, exact-preview recomposition, and refreshed
approvals against qualified manifests before real intake. Represent these as
distinct development handoff and full supported-profile closure predicates,
while preserving the existing preview, approval, witnessed intake, rejection,
and provider-qualification requirements.
In `@packages/runtime-contracts/src/development-pre-run.ts`:
- Around line 128-144: Update the provider-manifest approval flow around the
approval construction and submit validation so the approved subject includes
preview.manifestId, not just preview.manifestDigest. Add a shared helper for
deriving the provider-manifest subject, use it in both approve and submit, and
make submit compare the manifestApproval.subjectDigest against that derived
subject while preserving existing scope checks.
In `@packages/runtime-contracts/src/ledger.ts`:
- Around line 599-607: Update the validation condition in the terminal
acknowledgement intake flow to reject any defined empty successorCut by checking
request.successorCut !== undefined before applying nonEmpty. Preserve the
existing rejection of successorCut with a rejected terminalAck and all other
validation behavior.
---
Nitpick comments:
In `@packages/local-file-providers/tests/local-file-ledger.test.mjs`:
- Around line 296-316: Extend the rejection test for local-file intake to cover
replay validation in createLocalFileIntakeForConformance: assert an identical
terminalAck:'rejected' request returns the stored result, a replay with
terminalAck:'accepted' fails with INTAKE_REQUEST_MISMATCH, and a rejected replay
including successorCut fails with INVALID_INTAKE.
In `@packages/runtime-contracts/src/development-pre-run.ts`:
- Around line 191-215: The submit method’s malformed successorCut or terminalAck
inputs currently return the authority-denial failure; split these shape
validations from the approval checks and return FC-INPUT with INVALID_INTAKE for
invalid terminalAck/successorCut values, while preserving FC-AUTHORITY /
EXACT_DEVELOPMENT_APPROVALS_REQUIRED for missing or unrecognized approval data.
In `@packages/runtime-contracts/tests/development-pre-run.test.mjs`:
- Around line 155-177: Add a test case in the development intake rejection test
around profile.submit that creates a second preview with the same manifestDigest
but a different manifestId, reuses the original manifest approval, and asserts
submission is rejected. Keep the existing approval-substitution and
copied-preview cases 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4786d46a-f7f1-40d0-b995-5ccaa1a19c48
📒 Files selected for processing (19)
docs/delivery/README.mddocs/delivery/greenfield/README.mddocs/delivery/greenfield/decisions.mddocs/delivery/greenfield/delivery-policy.mddocs/delivery/greenfield/dependency-dag.mddocs/delivery/greenfield/phase-orchestration.mddocs/delivery/greenfield/stories/GF-023.mddocs/delivery/greenfield/stories/GF-024.mddocs/delivery/greenfield/stories/GF-025.mddocs/delivery/greenfield/stories/GF-026.mddocs/delivery/greenfield/stories/README.mddocs/delivery/greenfield/track.jsonpackages/local-file-providers/src/local-file-intake.tspackages/local-file-providers/tests/local-file-ledger.test.mjspackages/runtime-contracts/src/development-pre-run.tspackages/runtime-contracts/src/index.tspackages/runtime-contracts/src/ledger.tspackages/runtime-contracts/tests/development-pre-run.test.mjspackages/runtime-contracts/tests/ledger-contract.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/runtime-contracts/src/development-pre-run.ts (1)
279-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate malformed input from missing approval authority.
Lines 279-282 validate the shape of
terminalAckandsuccessorCut. A malformed value returnsFC-AUTHORITY/EXACT_DEVELOPMENT_APPROVALS_REQUIRED. The same class of input error returnsFC-INPUT/INVALID_INTAKEin the local-file intake path. A caller that branches onfamilycannot tell a bad request from an unauthorized one, and an audit reader sees an authority denial that never happened.Report the disposition and cut checks under
FC-INPUT, and keepFC-AUTHORITYfor the preview and approval membership checks.♻️ Proposed split
if ( !data || typeof data.preview !== 'object' || data.preview === null || !previews.has(data.preview) || typeof data.proposalApproval !== 'object' || data.proposalApproval === null || !approvalBinding?.proposalApprovals.has(data.proposalApproval) || typeof data.manifestApproval !== 'object' || data.manifestApproval === null || - !approvalBinding.manifestApprovals.has(data.manifestApproval) || - (data.terminalAck !== 'accepted' && data.terminalAck !== 'rejected') || - (data.terminalAck === 'rejected' && data.successorCut !== undefined) || - (data.successorCut !== undefined && - (typeof data.successorCut !== 'string' || data.successorCut.length === 0 || data.successorCut.length > 512)) + !approvalBinding.manifestApprovals.has(data.manifestApproval) ) return fail('FC-AUTHORITY', 'EXACT_DEVELOPMENT_APPROVALS_REQUIRED'); + if ( + (data.terminalAck !== 'accepted' && data.terminalAck !== 'rejected') || + (data.terminalAck === 'rejected' && data.successorCut !== undefined) || + (data.successorCut !== undefined && + (typeof data.successorCut !== 'string' || data.successorCut.length === 0 || data.successorCut.length > 512)) + ) + return fail('FC-INPUT', 'INVALID_INTAKE');Update the tests that assert the current family for these cases.
🤖 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/development-pre-run.ts` around lines 279 - 284, Update the validation branch in the development pre-run flow around terminalAck and successorCut so malformed disposition or cut values return FC-INPUT/INVALID_INTAKE, matching the local-file intake path. Keep FC-AUTHORITY/EXACT_DEVELOPMENT_APPROVALS_REQUIRED exclusively for preview and approval membership checks, and update tests asserting the affected error family.packages/runtime-contracts/tests/development-pre-run.test.mjs (2)
135-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the negative manifest coverage to every validated field.
manifestinpackages/runtime-contracts/src/development-pre-run.ts(Lines 120-139) rejects fourteen field conditions. This test negates onlyproviderEnabled. A regression that drops thedispatchEnabled,recovery,lineage.kind,runtimeAuthority.kind,scope, or authority-array checks stays undetected. ThemanifestBytes(changes)fixture already supports each case.💚 Proposed table-driven coverage
assert.equal( profile.preview({ envelope: envelopeInput(), providerManifestBytes: manifestBytes({ providerEnabled: true }) }).ok, false, ); + for (const change of [ + { dispatchEnabled: true }, + { recovery: 'autonomous-restore' }, + { lineage: { kind: 'derived' } }, + { runtimeAuthority: { kind: 'subprocess' } }, + { scope: { phase: 2, purpose: 'development-only' } }, + { scope: { phase: 3, purpose: 'production' } }, + { credentialAuthority: ['secret'] }, + { externalServiceAuthority: ['service'] }, + { filesystemAuthority: ['/tmp'] }, + { nativePermissionPostures: ['granted'] }, + { networkAuthority: ['example.test'] }, + { subprocessAuthority: ['sh'] }, + ]) + assert.equal( + profile.preview({ envelope: envelopeInput(), providerManifestBytes: manifestBytes(change) }).ok, + false, + `manifest must be rejected: ${JSON.stringify(change)}`, + );🤖 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/development-pre-run.test.mjs` around lines 135 - 162, Expand the negative manifest assertions in the provider manifest digest test to cover every rejection condition enforced by manifest, including dispatchEnabled, recovery, lineage.kind, runtimeAuthority.kind, scope, and all authority-array validations. Use the existing manifestBytes(changes) fixture in table-driven cases, and assert each modified manifest preview returns ok false while preserving the existing providerEnabled and providerManifest rejection checks.
233-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for a verifier consumed by an unusable profile.
This case reuses the verifier on a second profile that has a valid ledger, so it passes today. It does not cover construction with an invalid or missing
ledger. That path also consumes the verifier, as flagged inpackages/runtime-contracts/src/development-pre-run.tsLines 210-219. Add a case that constructs a profile with an invalid ledger and then asserts a later valid profile still accepts approvals from the same authority.🤖 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/development-pre-run.test.mjs` around lines 233 - 252, The tests currently cover verifier reuse after a valid profile but not after failed construction. Add a regression case near the existing secondProfile scenario that creates a development pre-run profile with an invalid or missing ledger using the same authority.verifier, then constructs a valid profile with a scripted ledger and verifies its preview approvals and submit path succeed with that authority.packages/local-file-providers/tests/local-file-ledger.test.mjs (1)
355-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for an unknown
terminalAckvalue.The
terminalAck: 'rejected'withsuccessorCutcase is already covered. Add a test that expectsINVALID_INTAKEand confirms that no entry persists.🤖 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 355 - 363, Add a negative test alongside the existing intake validation tests that calls create with an unknown terminalAck value such as “rejected” together with successorCut, asserts the FC-INPUT/INVALID_INTAKE error, and verifies via intake.read that no entry was persisted.
🤖 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 `@packages/runtime-contracts/src/development-pre-run.ts`:
- Around line 210-219: Only add the verifier to claimedApprovalVerifiers after
confirming the configuration has a usable scripted ledger. Update the claim
condition near approvalBinding to require ledger in addition to verifier and
approvalBinding, while preserving the existing one-time claim behavior for valid
configurations.
---
Nitpick comments:
In `@packages/local-file-providers/tests/local-file-ledger.test.mjs`:
- Around line 355-363: Add a negative test alongside the existing intake
validation tests that calls create with an unknown terminalAck value such as
“rejected” together with successorCut, asserts the FC-INPUT/INVALID_INTAKE
error, and verifies via intake.read that no entry was persisted.
In `@packages/runtime-contracts/src/development-pre-run.ts`:
- Around line 279-284: Update the validation branch in the development pre-run
flow around terminalAck and successorCut so malformed disposition or cut values
return FC-INPUT/INVALID_INTAKE, matching the local-file intake path. Keep
FC-AUTHORITY/EXACT_DEVELOPMENT_APPROVALS_REQUIRED exclusively for preview and
approval membership checks, and update tests asserting the affected error
family.
In `@packages/runtime-contracts/tests/development-pre-run.test.mjs`:
- Around line 135-162: Expand the negative manifest assertions in the provider
manifest digest test to cover every rejection condition enforced by manifest,
including dispatchEnabled, recovery, lineage.kind, runtimeAuthority.kind, scope,
and all authority-array validations. Use the existing manifestBytes(changes)
fixture in table-driven cases, and assert each modified manifest preview returns
ok false while preserving the existing providerEnabled and providerManifest
rejection checks.
- Around line 233-252: The tests currently cover verifier reuse after a valid
profile but not after failed construction. Add a regression case near the
existing secondProfile scenario that creates a development pre-run profile with
an invalid or missing ledger using the same authority.verifier, then constructs
a valid profile with a scripted ledger and verifies its preview approvals and
submit path succeed with that authority.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1862e6bf-3c27-4aab-974e-a1e256a46d9a
📒 Files selected for processing (7)
docs/delivery/greenfield/stories/GF-023.mdpackages/local-file-providers/src/local-file-intake.tspackages/local-file-providers/tests/local-file-ledger.test.mjspackages/runtime-contracts/src/development-pre-run.tspackages/runtime-contracts/src/ledger.tspackages/runtime-contracts/tests/development-pre-run.test.mjspackages/runtime-contracts/tests/ledger-contract.test.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/runtime-contracts/src/ledger.ts
- packages/runtime-contracts/tests/ledger-contract.test.mjs
- packages/local-file-providers/src/local-file-intake.ts
- docs/delivery/greenfield/stories/GF-023.md
|
Review-body nitpicks are also covered in |
|
Latest review-body coverage is complete in |
1 participant
Summary
development-semantic-onlypre-Run flow with effect-free preview and separate exact proposal/provider-manifest approvalsWhy
Phase 3 implementation was unnecessarily coupled to machine-specific provider and witness-root qualification. This creates the minimum safe semantic development path without weakening the production authority or recovery model.
Safety boundary and impact
The development profile uses only the scripted ledger, keeps
providerEnabledanddispatchEnabledfalse, and fails closed with no autonomous restore. GF-020/GF-025/GF-026 remain mandatory before real-provider activation, autonomous recovery, supported-profile claims, or full Phase 2 closure.Validation
pnpm delivery:checkpnpm check(21 tasks passed)git diff --checkSummary by CodeRabbit
New Features
Documentation
Tests