feat(SUP-3464): add jarvos control plane core - #99
Conversation
|
Warning Review limit reached
Next review available in: 45 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds the new ChangesControl Plane Module
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Requester
participant ControlPlane
participant PolicyEngine
participant Registry
participant Store
participant Manager
Requester->>ControlPlane: Submit structured request
ControlPlane->>PolicyEngine: Evaluate policy
ControlPlane->>Registry: Select executable manager
ControlPlane->>Store: Deduplicate and acquire fenced lease
ControlPlane->>Manager: Execute fenced command
Manager-->>ControlPlane: Return execution result
ControlPlane->>Manager: Verify side effect
Manager-->>ControlPlane: Return verification result
ControlPlane->>Store: Commit evidence and lifecycle state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (1)
modules/jarvos-control-plane/README.md (1)
90-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid top-level
awaitwith CommonJSrequire.Node.js does not support top-level
awaitin CommonJS modules (which therequireusage implies). If a user copies and pastes this snippet into a standalone.jsscript, it will throw aSyntaxError. Consider wrapping the execution in an async IIFE so the example runs seamlessly out of the box.♻️ Proposed refactor
-await reconciler.reconcileRequest(request); +(async () => { + await reconciler.reconcileRequest(request); +})();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/jarvos-control-plane/README.md` around lines 90 - 145, Update the README example around createMemoryStore, createReconciler, and reconcileRequest to avoid top-level await in the CommonJS snippet. Wrap the asynchronous setup and reconciler.reconcileRequest(request) call in an async IIFE so the copied standalone .js example executes without a syntax error.
🤖 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 `@modules/jarvos-control-plane/src/contracts/index.js`:
- Around line 89-101: Update canonicalResourceKey to build the key with the
existing stableStringify helper over the ordered tuple [resource.machineId,
resource.type, resource.id] instead of delimiter concatenation. Preserve the
current validation and canonicalMutationKey behavior, including its
mutationClass suffix handling.
- Around line 224-238: Update createCommand to always derive resourceKey,
specDigest, and actionKey from the source fields instead of accepting
caller-provided values. Extend validateRecord to recompute and compare all three
canonical values—resourceKey, specDigest, and buildActionKey(record)—and reject
mismatches, including for imported records.
- Around line 375-395: Update lifecycleTransition to enforce legal status
changes using an allowed-transition map keyed by the command’s current status.
Reject any destination not permitted, including all transitions out of terminal
statuses such as satisfied, failed, and policy_denied, before cloning or
mutating the command; preserve the existing unknown-status validation and valid
transition behavior.
- Around line 350-370: Update toPublicProjection to reject or safely default
unknown options.maxSensitivity values before comparing sensitivity ranks,
ensuring invalid requests cannot expose the full record. Replace direct delete
projection[field.path] field redaction with nested-path deletion so paths such
as principal.credentials.token are removed correctly, and apply this redaction
before returning the projection.
- Around line 333-346: Update validateManagerManifest to require
supportedCoreVersions to be an array before accepting the manifest. Add this
validation alongside the existing manifest field checks so malformed string or
object values are rejected during validation, preventing versionCompatible from
calling .some() on invalid input.
In `@modules/jarvos-control-plane/src/reconciliation/index.js`:
- Around line 57-65: Update the existing-command status filter in the
reconciliation flow to exclude conflict and deferred outcomes from
deduplication, alongside failed, unverifiable, and expired. Ensure commands with
those statuses proceed as new retry attempts while preserving deduplication for
other active or successful statuses.
- Around line 118-121: Wrap the verifier invocation in the reconciliation flow
around verifier and lifecycleTransition so rejected or thrown verifier errors
are caught before the command remains in the persisted verifying state. Convert
failures through the existing evidence-rejection response path, ensuring
reconcileRequest returns a terminal outcome rather than allowing the verifier
error to escape.
- Around line 69-74: Update the reconciliation flow around acquireLease and
commitEvidence to release the acquired lease on every terminal path, including
deferred execution, failures, evidence rejection, and successful completion.
Ensure cleanup runs after each outcome while preserving the existing result
handling, so mutation keys are not blocked until TTL expiry.
In `@modules/jarvos-control-plane/src/registry/index.js`:
- Around line 87-110: The registration flow around mutationOwnershipKey and
managers.set must stage candidate ownership without mutating ownership, detect
all conflicts, and only commit staged keys after the registration remains
executable. Before committing, remove every ownership entry previously belonging
to manifest.managerId so re-registration drops removed mutation classes; then
atomically add the candidate keys, while leaving ownership unchanged for
conflicted or observation-only registrations.
In `@modules/jarvos-control-plane/src/storage/index.js`:
- Around line 79-95: Validate in acquireLease that input.holder is non-empty
before checking the existing lease, and reject missing or empty holders with the
established invalid-input error behavior. Preserve the existing lease-conflict
comparison and acquisition flow for valid holders.
- Around line 56-59: Update putRecord so that when replacing an existing
command, it removes the prior command’s actionKey mapping before registering the
new actionKey; preserve the existing record storage and append behavior.
- Around line 96-105: Update the lease construction in the acquire flow to
derive the default expiresAt from the injected now clock rather than Date.now(),
and use a nullish/default-preserving check for ttlMs so an explicit zero
produces immediate expiration instead of falling back to 60000.
- Around line 111-124: The commitEvidence function must reject submissions from
leases that have expired, in addition to validating the current lease ID and
fence. Check the lease’s validity/expiration before putRecord(record), and
return the existing stale_fence rejection result with evidence.rejected logging
when the lease is no longer valid.
- Around line 155-158: Update createFileStore so startup state is reconstructed
by replaying journal.ndjson after loading state.json, or make state.json the
authoritative durable source and ensure mutations cannot be lost between state
writes and journal appends. Preserve the existing initialState fallback while
handling missing or corrupted state.json through the selected durability
strategy.
- Around line 150-180: Update createFileStore and its wrapped lease-write path
to use a cross-process lock or CAS for each state mutation, reloading the latest
state before applying and persisting the operation so concurrent stores sharing
rootDir cannot grant the same lease. In
modules/jarvos-control-plane/test/concurrency.test.js lines 10-36, replace the
single in-memory-store race with a race between separate file stores using one
rootDir, asserting that only one lease is granted.
- Around line 150-158: Update createFileStore and its persistence path to
coordinate concurrent instances sharing the same directory, using cross-instance
locking or compare-and-swap before committing state. Ensure lease/fence updates
detect conflicts and never unconditionally overwrite a newer state snapshot;
preserve the existing store behavior when no concurrent writer is present.
In `@tests/modules-smoke-test.js`:
- Around line 500-502: Update the smoke test’s control-plane import in the try
block to require the package entrypoint through its package name or package
root, rather than directly loading src/index.js; keep the existing
createRegistry call and assertions unchanged.
---
Nitpick comments:
In `@modules/jarvos-control-plane/README.md`:
- Around line 90-145: Update the README example around createMemoryStore,
createReconciler, and reconcileRequest to avoid top-level await in the CommonJS
snippet. Wrap the asynchronous setup and reconciler.reconcileRequest(request)
call in an async IIFE so the copied standalone .js example executes without a
syntax error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c91d3e2-cc93-45d9-ba62-559c8ba00eeb
📒 Files selected for processing (17)
docs/architecture/control-plane.mdmodules/README.mdmodules/jarvos-control-plane/README.mdmodules/jarvos-control-plane/package.jsonmodules/jarvos-control-plane/src/contracts/index.jsmodules/jarvos-control-plane/src/index.jsmodules/jarvos-control-plane/src/policy/index.jsmodules/jarvos-control-plane/src/reconciliation/index.jsmodules/jarvos-control-plane/src/registry/index.jsmodules/jarvos-control-plane/src/storage/index.jsmodules/jarvos-control-plane/test/concurrency.test.jsmodules/jarvos-control-plane/test/contracts.test.jsmodules/jarvos-control-plane/test/reconciliation.test.jsmodules/jarvos-control-plane/test/storage.test.jspackage.jsonscripts/smoke-test.shtests/modules-smoke-test.js
| function canonicalResourceKey(resource = {}) { | ||
| if (!isObject(resource)) throw new Error('resource must be an object'); | ||
| for (const field of ['machineId', 'type', 'id']) { | ||
| if (!resource[field]) throw new Error(`resource.${field} is required`); | ||
| } | ||
| return `${resource.machineId}:${resource.type}:${resource.id}`; | ||
| } | ||
|
|
||
| function canonicalMutationKey({ machineId, resource, mutationClass }) { | ||
| if (!mutationClass) throw new Error('mutationClass is required'); | ||
| const base = resource ? canonicalResourceKey(resource) : machineId; | ||
| if (!base) throw new Error('machineId or resource is required'); | ||
| return `${base}:${mutationClass}`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use collision-safe encoding for canonical keys.
The unrestricted components are joined with :, so distinct resources can share a key—for example, machineId="a:b", type="c" and machineId="a", type="b:c". This can merge action deduplication or mutation ownership across resources.
Use a canonical tuple encoding such as stableStringify([machineId, type, id]) rather than delimiter concatenation.
🤖 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 `@modules/jarvos-control-plane/src/contracts/index.js` around lines 89 - 101,
Update canonicalResourceKey to build the key with the existing stableStringify
helper over the ordered tuple [resource.machineId, resource.type, resource.id]
instead of delimiter concatenation. Preserve the current validation and
canonicalMutationKey behavior, including its mutationClass suffix handling.
| resourceKey: input.resourceKey || canonicalResourceKey(input.resource), | ||
| mutationClass: input.mutationClass, | ||
| desiredGeneration: input.desiredGeneration, | ||
| commandSpec, | ||
| specDigest: commandSpecDigest(commandSpec), | ||
| actionKey: input.actionKey || null, | ||
| status: input.status || 'requested', | ||
| lifecycle: input.lifecycle || [], | ||
| leaseId: input.leaseId || null, | ||
| fence: input.fence || null, | ||
| checkpoint: input.checkpoint || null, | ||
| approval: input.approval || null, | ||
| }; | ||
| record.actionKey = record.actionKey || buildActionKey(record); | ||
| validateRecord(record); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject non-canonical command identity fields.
createCommand accepts an arbitrary actionKey, while validateRecord merely computes buildActionKey(record) without comparing it. It also does not verify resourceKey and specDigest against their source fields. Imported records can therefore bypass deduplication by supplying inconsistent derived values.
Always derive these fields during creation and compare all three during validation.
Also applies to: 293-329
🤖 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 `@modules/jarvos-control-plane/src/contracts/index.js` around lines 224 - 238,
Update createCommand to always derive resourceKey, specDigest, and actionKey
from the source fields instead of accepting caller-provided values. Extend
validateRecord to recompute and compare all three canonical values—resourceKey,
specDigest, and buildActionKey(record)—and reject mismatches, including for
imported records.
| function validateManagerManifest(manifest = {}) { | ||
| if (!isObject(manifest)) throw new Error('manager manifest must be an object'); | ||
| if (manifest.schemaVersion !== MANAGER_SCHEMA_VERSION) throw new Error(`Unsupported manager schemaVersion: ${manifest.schemaVersion}`); | ||
| assertCompatibleVersion(manifest.contractVersion); | ||
| if (!manifest.managerId) throw new Error('managerId is required'); | ||
| if (!Array.isArray(manifest.capabilities)) throw new Error('capabilities must be an array'); | ||
| if (!Array.isArray(manifest.mutationClasses)) throw new Error('mutationClasses must be an array'); | ||
| for (const mutation of manifest.mutationClasses) { | ||
| if (!mutation || !mutation.class) throw new Error('mutationClasses entries require class'); | ||
| if (!mutation.resourceType) throw new Error(`mutation ${mutation.class} requires resourceType`); | ||
| } | ||
| if (manifest.trust && !['data-only', 'trusted'].includes(manifest.trust.level)) { | ||
| throw new Error('trust.level must be data-only or trusted'); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate supportedCoreVersions as an array.
A manifest containing a string or object passes this validation, then versionCompatible in modules/jarvos-control-plane/src/registry/index.js Line 16 calls .some() and crashes registration instead of returning an incompatible health-only entry.
Proposed validation
if (!manifest.managerId) throw new Error('managerId is required');
+ if (!Array.isArray(manifest.supportedCoreVersions)) {
+ throw new Error('supportedCoreVersions must be an array');
+ }
if (!Array.isArray(manifest.capabilities)) throw new Error('capabilities must be an array');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function validateManagerManifest(manifest = {}) { | |
| if (!isObject(manifest)) throw new Error('manager manifest must be an object'); | |
| if (manifest.schemaVersion !== MANAGER_SCHEMA_VERSION) throw new Error(`Unsupported manager schemaVersion: ${manifest.schemaVersion}`); | |
| assertCompatibleVersion(manifest.contractVersion); | |
| if (!manifest.managerId) throw new Error('managerId is required'); | |
| if (!Array.isArray(manifest.capabilities)) throw new Error('capabilities must be an array'); | |
| if (!Array.isArray(manifest.mutationClasses)) throw new Error('mutationClasses must be an array'); | |
| for (const mutation of manifest.mutationClasses) { | |
| if (!mutation || !mutation.class) throw new Error('mutationClasses entries require class'); | |
| if (!mutation.resourceType) throw new Error(`mutation ${mutation.class} requires resourceType`); | |
| } | |
| if (manifest.trust && !['data-only', 'trusted'].includes(manifest.trust.level)) { | |
| throw new Error('trust.level must be data-only or trusted'); | |
| } | |
| function validateManagerManifest(manifest = {}) { | |
| if (!isObject(manifest)) throw new Error('manager manifest must be an object'); | |
| if (manifest.schemaVersion !== MANAGER_SCHEMA_VERSION) throw new Error(`Unsupported manager schemaVersion: ${manifest.schemaVersion}`); | |
| assertCompatibleVersion(manifest.contractVersion); | |
| if (!manifest.managerId) throw new Error('managerId is required'); | |
| if (!Array.isArray(manifest.supportedCoreVersions)) { | |
| throw new Error('supportedCoreVersions must be an array'); | |
| } | |
| if (!Array.isArray(manifest.capabilities)) throw new Error('capabilities must be an array'); | |
| if (!Array.isArray(manifest.mutationClasses)) throw new Error('mutationClasses must be an array'); | |
| for (const mutation of manifest.mutationClasses) { | |
| if (!mutation || !mutation.class) throw new Error('mutationClasses entries require class'); | |
| if (!mutation.resourceType) throw new Error(`mutation ${mutation.class} requires resourceType`); | |
| } | |
| if (manifest.trust && !['data-only', 'trusted'].includes(manifest.trust.level)) { | |
| throw new Error('trust.level must be data-only or trusted'); | |
| } |
🤖 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 `@modules/jarvos-control-plane/src/contracts/index.js` around lines 333 - 346,
Update validateManagerManifest to require supportedCoreVersions to be an array
before accepting the manifest. Add this validation alongside the existing
manifest field checks so malformed string or object values are rejected during
validation, preventing versionCompatible from calling .some() on invalid input.
| function toPublicProjection(record, options = {}) { | ||
| const projection = clone(record); | ||
| const maxLevel = options.maxSensitivity || 'internal'; | ||
| const order = new Map(SENSITIVITY_LEVELS.map((level, index) => [level, index])); | ||
| if (order.get(record.sensitivity && record.sensitivity.level || 'internal') > order.get(maxLevel)) { | ||
| return { | ||
| schemaVersion: record.schemaVersion, | ||
| contractVersion: record.contractVersion, | ||
| type: record.type, | ||
| id: record.id, | ||
| sensitivity: record.sensitivity, | ||
| redacted: true, | ||
| }; | ||
| } | ||
| if (!options.includeAdapterExtensions) delete projection.adapterExtensions; | ||
| if (projection.sensitivity && projection.sensitivity.fields) { | ||
| for (const field of projection.sensitivity.fields) { | ||
| if (field.level === 'secret' || field.level === 'private') { | ||
| delete projection[field.path]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make sensitivity projection fail closed.
An unknown maxSensitivity has no rank, causing the comparison at Line 354 to return false and expose the complete record. Field-level redaction also uses delete projection[field.path], which does not remove nested paths such as principal.credentials.token.
Validate the requested level and apply nested-path redaction before returning the projection.
🤖 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 `@modules/jarvos-control-plane/src/contracts/index.js` around lines 350 - 370,
Update toPublicProjection to reject or safely default unknown
options.maxSensitivity values before comparing sensitivity ranks, ensuring
invalid requests cannot expose the full record. Replace direct delete
projection[field.path] field redaction with nested-path deletion so paths such
as principal.credentials.token are removed correctly, and apply this redaction
before returning the projection.
| function lifecycleTransition(command, status, patch = {}) { | ||
| if (!COMMAND_STATUSES.includes(status)) throw new Error(`Unknown command status: ${status}`); | ||
| const next = clone(command); | ||
| next.status = status; | ||
| next.updatedAt = normalizeTimestamp(patch.at); | ||
| next.lifecycle = Array.isArray(next.lifecycle) ? next.lifecycle : []; | ||
| next.lifecycle.push({ | ||
| status, | ||
| at: next.updatedAt, | ||
| actor: patch.actor || null, | ||
| runId: patch.runId || null, | ||
| requestId: next.requestId || null, | ||
| generation: next.desiredGeneration || null, | ||
| checkpoint: patch.checkpoint || null, | ||
| reason: patch.reason || null, | ||
| }); | ||
| if (patch.checkpoint) next.checkpoint = patch.checkpoint; | ||
| if (patch.leaseId) next.leaseId = patch.leaseId; | ||
| if (patch.fence) next.fence = patch.fence; | ||
| validateRecord(next); | ||
| return next; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce legal lifecycle transitions.
This accepts any known destination status regardless of the current state, allowing terminal commands such as satisfied, failed, or policy_denied to transition back to dispatching or executed. That undermines lifecycle and evidence integrity.
Define an allowed-transition map and reject transitions from terminal states.
🤖 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 `@modules/jarvos-control-plane/src/contracts/index.js` around lines 375 - 395,
Update lifecycleTransition to enforce legal status changes using an
allowed-transition map keyed by the command’s current status. Reject any
destination not permitted, including all transitions out of terminal statuses
such as satisfied, failed, and policy_denied, before cloning or mutating the
command; preserve the existing unknown-status validation and valid transition
behavior.
Changes requested — independent high-tier review found 7 P1 defectsRan the independent Codex review ( Proven by execution (reproduced twice)P1 findings
P2
Test-quality note (the reason this got through)All 17 tests pass, but they cannot detect the defects above. The "concurrency" test runs two microtasks against one in-memory Not a defect: DispositionSetting back to |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
566a7b2 to
755e2ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/modules-smoke-test.js (1)
501-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the package entrypoint instead of importing its source directly.
Requiring
src/index.jsbypassesmodules/jarvos-control-plane/package.json, so this smoke test can pass even if the published entrypoint configuration is broken.♻️ Proposed fix
- const controlPlane = require(path.join(ROOT, 'modules/jarvos-control-plane/src/index.js')); + const controlPlane = require(path.join(ROOT, 'modules/jarvos-control-plane'));🤖 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 `@tests/modules-smoke-test.js` at line 501, Update the controlPlane import in the smoke test to load the jarvos-control-plane package through its package entrypoint rather than directly requiring src/index.js. Keep the existing controlPlane usage unchanged and ensure module resolution exercises the package.json entrypoint configuration.Source: Linters/SAST tools
🧹 Nitpick comments (1)
modules/jarvos-control-plane/src/storage/index.js (1)
113-121: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
load()fully re-parses and replays the entire journal on every single call, including reads.
mutate()andread()both callload(), andload()always runsparseJournal+replayJournalfrom scratch (line 121). This makes everygetRecord,acquireLease,commitEvidence, etc. an O(n) operation over the full journal history, with no compaction/checkpoint mechanism — cost grows unbounded as the journal accumulates. Notably,state.jsonis written atomically on everypersist()(125-127) but is never read back anywhere in this file, so all that I/O buys no reduction in replay cost.Consider using
state.jsonas a periodic checkpoint (snapshot + last-replayed sequence) and only replaying journal entries appended after that checkpoint, with a background/opportunistic compaction to truncate the journal once checkpointed.Also applies to: 129-131
🤖 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 `@modules/jarvos-control-plane/src/storage/index.js` around lines 113 - 121, Update createFileStore’s load and persistence flow so state.json is used as a checkpoint containing the saved state and last-replayed journal sequence, rather than always replaying the entire journal. In load(), read and validate the checkpoint, replay only journal entries after its sequence, and preserve the initial-state fallback when no valid checkpoint exists. Add opportunistic or background compaction that safely writes an updated checkpoint and truncates only the journal entries already included in it, while keeping mutate/read behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/jarvos-control-plane/src/reconciliation/index.js`:
- Around line 99-104: Release the lease acquired before selecting the manager
execution port before returning from the unavailable-port branch in the
reconciliation flow. Update the branch around managers[selected.managerId] and
executeFenced so it calls store.releaseLease with the same resource and
mutationClass lease key used by the other terminal paths, while preserving the
deferred transition and response.
- Around line 57-69: Remove the non-atomic fallback around
store.putCommandIfAbsent and make that capability mandatory for injected stores.
Validate its presence during reconciliation/store initialization and fail fast
when unavailable, then have reconcileRequest use the atomic insertion method
directly so concurrent actionKey requests remain deduplicated and retryable
statuses are not incorrectly blocked.
In `@modules/jarvos-control-plane/src/storage/index.js`:
- Around line 116-120: Update withLock to detect and reclaim orphaned lock files
before timing out: when exclusive creation encounters EEXIST, inspect lockPath
metadata and remove the lock if it exceeds the configured lockTimeoutMs, then
retry acquisition. Preserve immediate propagation of non-EEXIST errors and
ensure cleanup still closes and unlinks only the lock acquired by the current
invocation.
- Around line 122-128: Update persist so failures during the state.json
temporary-file write, fsync, or rename are caught after the journal append has
been successfully fsynced; log the state persistence error and allow the
mutation to continue. Preserve cleanup and directory fsync behavior where
applicable, and add a regression test covering a successful journal append
followed by a state-file failure.
---
Duplicate comments:
In `@tests/modules-smoke-test.js`:
- Line 501: Update the controlPlane import in the smoke test to load the
jarvos-control-plane package through its package entrypoint rather than directly
requiring src/index.js. Keep the existing controlPlane usage unchanged and
ensure module resolution exercises the package.json entrypoint configuration.
---
Nitpick comments:
In `@modules/jarvos-control-plane/src/storage/index.js`:
- Around line 113-121: Update createFileStore’s load and persistence flow so
state.json is used as a checkpoint containing the saved state and last-replayed
journal sequence, rather than always replaying the entire journal. In load(),
read and validate the checkpoint, replay only journal entries after its
sequence, and preserve the initial-state fallback when no valid checkpoint
exists. Add opportunistic or background compaction that safely writes an updated
checkpoint and truncates only the journal entries already included in it, while
keeping mutate/read behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f74a14-76d1-40d9-94d0-0d8c553c291c
📒 Files selected for processing (17)
docs/architecture/control-plane.mdmodules/README.mdmodules/jarvos-control-plane/README.mdmodules/jarvos-control-plane/package.jsonmodules/jarvos-control-plane/src/contracts/index.jsmodules/jarvos-control-plane/src/index.jsmodules/jarvos-control-plane/src/policy/index.jsmodules/jarvos-control-plane/src/reconciliation/index.jsmodules/jarvos-control-plane/src/registry/index.jsmodules/jarvos-control-plane/src/storage/index.jsmodules/jarvos-control-plane/test/concurrency.test.jsmodules/jarvos-control-plane/test/contracts.test.jsmodules/jarvos-control-plane/test/reconciliation.test.jsmodules/jarvos-control-plane/test/storage.test.jspackage.jsonscripts/smoke-test.shtests/modules-smoke-test.js
🚧 Files skipped from review as they are similar to previous changes (12)
- modules/jarvos-control-plane/src/index.js
- modules/jarvos-control-plane/package.json
- package.json
- scripts/smoke-test.sh
- modules/jarvos-control-plane/test/concurrency.test.js
- modules/jarvos-control-plane/test/reconciliation.test.js
- modules/jarvos-control-plane/README.md
- modules/README.md
- modules/jarvos-control-plane/src/policy/index.js
- modules/jarvos-control-plane/src/registry/index.js
- docs/architecture/control-plane.md
- modules/jarvos-control-plane/src/contracts/index.js
|
Round-2 independent review of this merged core: round-1 P1s are confirmed genuinely fixed (re-probed empirically). Residual defects found in the fixes — two P1s (stale-lock recovery TOCTOU race; append-after-torn-journal-frame permanently bricks the store, reproduced on |
Summary
Verification
Known local failure
Issue
Summary by CodeRabbit
New Features
@jarvos/control-planemodule for portable machine-wide AI coordination.Documentation
Tests