SUP-2261: reconcile jarvos-coding + jarvos-skills into public (public canonical) - #70
Conversation
…canonical, bidirectional) Bring the public repo's @jarvos/coding and @jarvos/skills modules to parity with the clawd-root build, merging both sides — nothing clobbered. Public stays canonical; where clawd was ahead it is layered in; where public was ahead (host adapters / MCP + skill descriptors / skill-install engine / strict pack validation) it is preserved. @jarvos/coding - adapters/hosts.js: UNION — keep public's MCP/skill descriptors + host-adapter factories (createClaudeCodeHostAdapter/createCodexHostAdapter/createCodingHostAdapter), add clawd's continuity contract block, continuityAdapterMatrix, and hermes/personality hosts. - review-engine, lifecycle/policy, session-state, submission-gate: take clawd supersets (gate-equivalents, issue-branch lifecycle, goal-alignment gate key, continuity). - index.js: UNION barrel — clawd's new exports (live adapters, goal-alignment, gate-equivalents, issue-branch lifecycle, continuityAdapterMatrix) plus public's host-adapter/MCP/skill exports. - additive layer brought in (closure required to load the reconciled barrel): adapters/live/* (8 live adapters) + features/goal-alignment. This materially lands SUP-2263's live-adapter layer so a fresh install can run a real issue, not just a mock. @jarvos/skills - src/index.js: UNION — keep public's skill-install engine (getManifest/getSkill/ listSkills/installSkills/validateBundle) + strict assertPackManifest + win32-aware commandExists, and add clawd's workspace/lossless-claw suite (initJarvosWorkspace, evaluateLosslessClawHealth, inspectLosslessClaw, enableLosslessClawInOpenClawConfig) plus detection.files support; detectPackEnvironment/buildInstallPlan upgraded to the superset. The local-openclaw/v0-5-0 pack DATA defers with the rest of the additive layer (bringing it would force loosening the test-pinned strict validator). Tests: `npm test` (bootstrap + modules-smoke + cli-smoke + structure) green; @jarvos/coding 9/9 and @jarvos/skills suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive "live adapter" infrastructure for jarvos-coding, enabling executable workflows for Git branching, pull-request operations, code review stages, and issue tracking. It adds goal-alignment evaluation gates to block/escalate merges when PR intent is unclear, extends the review engine with gate-equivalent command surfaces, defines issue-branch lifecycle state machines, and initializes workspace setup with lossless-claw OpenClaw plugin configuration in jarvos-skills. ChangesLive Adapters, Goal Alignment, Lifecycle & Workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7315f174f6
ℹ️ 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".
| function createLivePaperclipTracker(options = {}) { | ||
| const prLink = resolvePrLink(options.prLink); |
There was a problem hiding this comment.
When callers use the exported buildLiveCodingAdapters() without injecting prLink, this constructor line immediately calls resolvePrLink() and requires clawd's scripts/lib/paperclip-pr-link.js; that file is not present in this public package, so live adapter construction throws before any issue run can start. Keep the clawd-only require lazy inside claimIssue/verifyAndClose/sweep, or require an injected tracker implementation at the builder boundary.
Useful? React with 👍 / 👎.
| const comment = options.closeComment | ||
| || `[jarvos-coding] verifyAndClose: closing ${identifier} after merge of ${input.pullRequest?.url || input.branch || 'branch'}.`; | ||
| const result = prLink.transitionIssue(identifier, closeStatus, comment); |
There was a problem hiding this comment.
In the normal live flow openPullRequest() returns an open PR, postMergeSweep skips because there is no merge evidence, and then this method still transitions the Paperclip issue to done. That closes real tracker issues as soon as a PR is opened rather than after merge/post-merge verification, so gate this transition on merged PR evidence or a successful post-merge sweep.
Useful? React with 👍 / 👎.
| const result = run(executable, args, { | ||
| cwd, | ||
| env, |
There was a problem hiding this comment.
When the live branch stage creates a new worktree, the orchestrator passes it along as branchResult.worktreeDir, but this runner ignores the payload and always uses only the static options.cwd. With default live adapters, clawpatch/autoreview therefore run in the original process checkout instead of the issue branch worktree, producing review evidence for the wrong diff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/jarvos-coding/src/lifecycle/policy.js (1)
255-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the exact issue-token matcher for branch hygiene.
This stage still relies on
branchMentionsIssue(), which is a raw substring check, sofeature/SUP-123-fixsatisfies issueSUP-12. That weakens the gate compared withbranchIsIssueNamed()in this file andbranchSatisfiesIdentifier()inmodules/jarvos-coding/src/features/submission-gate/index.jsLines 75-83.Suggested fix
function hasBranchHygiene(input = {}) { const git = input.git || {}; const clean = git.clean === true || evidencePassed(git.status, SUBMISSION_GATE_STATUS_POLICIES.branch_hygiene); const hasBranch = Boolean(git.branch); const hasBase = Boolean(git.baseBranch || git.upstream || git.baseRef); - const issueNamed = git.issueNamed === true || branchMentionsIssue(git.branch, input); + const identifier = input.issue?.identifier || input.issueIdentifier || input.issue?.id; + const issueNamed = git.issueNamed === true || branchIsIssueNamed(git.branch, identifier); const hasIntendedScope = Array.isArray(git.intendedFiles) && git.intendedFiles.length > 0; return clean && hasBranch && hasBase && issueNamed && hasIntendedScope; }🤖 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-coding/src/lifecycle/policy.js` around lines 255 - 262, hasBranchHygiene currently uses branchMentionsIssue() (a raw substring check) which allows partial matches like "feature/SUP-123-fix" matching "SUP-12"; update hasBranchHygiene to use the exact-token matcher instead (replace branchMentionsIssue(git.branch, input) with the stricter branchIsIssueNamed(...) or branchSatisfiesIdentifier(...) implementation used elsewhere) so the issueNamed check uses the exact issue-token logic; ensure you pass the same arguments expected by the chosen matcher (git.branch and the input/context) and remove the substring-based check.modules/jarvos-skills/src/index.js (1)
121-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed
detection.filesentries during manifest validation.
assertPackManifest()never validates the newpack.detection.filesshape. A pack can pass validation here and then crash later whendetectPackEnvironment()readsfile.name/file.pathon Lines 243-248. Fail fast here with the same per-entry checks you already apply todetection.commands.Suggested fix
if (!pack.detection || typeof pack.detection !== 'object' || Array.isArray(pack.detection)) { throw new Error(`jarvOS skill pack ${pack.name} must declare a detection object`); } if (!Array.isArray(pack.detection.commands)) { throw new Error(`jarvOS skill pack ${pack.name} must declare detection.commands`); } + if (pack.detection.files !== undefined && !Array.isArray(pack.detection.files)) { + throw new Error(`jarvOS skill pack ${pack.name} detection.files must be an array`); + } @@ const detectionNames = new Set(); for (const command of pack.detection.commands) { @@ detectionNames.add(command.name); } + const detectionFiles = new Set(); + for (const file of pack.detection.files || []) { + if (!file || typeof file !== 'object' || Array.isArray(file)) { + throw new Error(`${pack.name} detection file entries must be objects`); + } + for (const key of ['name', 'path']) { + if (typeof file[key] !== 'string' || file[key].trim() === '') { + throw new Error(`${pack.name} detection file missing ${key}`); + } + } + if (detectionFiles.has(file.name)) { + throw new Error(`${pack.name} duplicates detection file ${file.name}`); + } + detectionFiles.add(file.name); + }Also applies to: 241-249
🤖 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-skills/src/index.js` around lines 121 - 127, The manifest validation in assertPackManifest() currently checks pack.detection.commands but never validates pack.detection.files, which later causes detectPackEnvironment() to read file.name/file.path and potentially crash; update assertPackManifest() to require pack.detection.files is an array and iterate each entry to ensure it is an object (not an array) and that each entry has string-typed name and path properties (mirroring the existing checks for detection.commands), throwing a descriptive Error including pack.name when any file entry is malformed so invalid packs fail fast.
🧹 Nitpick comments (2)
modules/jarvos-coding/src/adapters/hosts.js (2)
61-64: ⚡ Quick winUse the continuity block as the single source of truth here.
continuityAdapterMatrix()now mixesentryBehavior,requiredTools, andcontinuityto describe continuity capabilities. That can drift silently as the contract evolves, especially now thatcontinuity.readOnEntryandcontinuity.writeCheckpointexist explicitly.Suggested direction
- readOnEntry: contract.entryBehavior.readSessionState, + readOnEntry: contract.continuity.readOnEntry.length > 0, hydrateLiveArtifact: contract.entryBehavior.readLiveArtifactFromPointer, - writeCheckpoint: contract.requiredTools.includes('jarvos_session_state.write'), + writeCheckpoint: contract.continuity.writeCheckpoint.length > 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/jarvos-coding/src/adapters/hosts.js` around lines 61 - 64, The continuityAdapterMatrix() should treat the contract.continuity block as the single source of truth: replace readOnEntry and writeCheckpoint to read from contract.continuity (use contract.continuity.readOnEntry and contract.continuity.writeCheckpoint) instead of contract.entryBehavior.readSessionState or checking contract.requiredTools; keep hydrateLiveArtifact mapped from contract.entryBehavior.readLiveArtifactFromPointer only if continuity does not provide an explicit alternative, and preserve freshSessionSurvival as contract.continuity.freshSessionSurvival.
55-78: ⚡ Quick winNarrow the
catchto unsupported-host cases only.This currently turns any exception from
codingHostAdapterContract()intostatus: 'unsupported'. A regression in the contract for a supported host would be silently misreported instead of failing fast.Suggested direction
function continuityAdapterMatrix(hosts = SUPPORTED_HOSTS) { return hosts.map((host) => { - try { - const contract = codingHostAdapterContract(host); - return { - host, - normalizedHost: contract.host, - status: 'supported', - readOnEntry: contract.entryBehavior.readSessionState, - hydrateLiveArtifact: contract.entryBehavior.readLiveArtifactFromPointer, - writeCheckpoint: contract.requiredTools.includes('jarvos_session_state.write'), - freshSessionSurvival: contract.continuity.freshSessionSurvival, - ownerAction: null, - }; - } catch (error) { - const normalizedHost = normalizeHost(host); + const normalizedHost = normalizeHost(host); + if (!SUPPORTED_HOSTS.includes(normalizedHost)) { return { host, normalizedHost, status: 'unsupported', readOnEntry: false, @@ freshSessionSurvival: null, ownerAction: `Add ${articleFor(normalizedHost)} ${normalizedHost} host adapter or mark the lane out of scope before assigning shared-brain continuity work.`, }; } + + const contract = codingHostAdapterContract(normalizedHost); + return { + host, + normalizedHost: contract.host, + status: 'supported', + readOnEntry: contract.entryBehavior.readSessionState, + hydrateLiveArtifact: contract.entryBehavior.readLiveArtifactFromPointer, + writeCheckpoint: contract.requiredTools.includes('jarvos_session_state.write'), + freshSessionSurvival: contract.continuity.freshSessionSurvival, + ownerAction: null, + }; }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/jarvos-coding/src/adapters/hosts.js` around lines 55 - 78, The catch around codingHostAdapterContract(host) is too broad and turns any thrown error into an unsupported-host response; update the try/catch so only the specific "unsupported host" error is handled and converted to the fallback object (use error.name === 'UnsupportedHostError' or a bespoke isUnsupportedHost(error) check), and rethrow any other errors so regressions in codingHostAdapterContract surface; continue to call normalizeHost(host) and articleFor(normalizedHost) only in the unsupported-host branch and keep the returned properties (status:'unsupported', readOnEntry:false, etc.) there.
🤖 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-coding/src/adapters/live/branch.js`:
- Around line 74-100: The code currently falls back to attaching the existing
branch on any failure creating the branch; change this so you only attach when
the git create failure indicates the branch already exists. After calling
run('git', ['worktree', 'add', '-b', branch, worktreeDir, baseRef], ...) inspect
the result (e.g., add.status and add.stderr/message) for the specific "branch
already exists" condition (match the git error text like "A branch named" /
"already exists" or equivalent) and only then run the attach sequence
(run('git', ['worktree', 'add', worktreeDir, branch], ...)). If the error is
anything else (invalid baseRef, fetch failure, etc.) return/throw a failure
including add.stderr so the caller sees the real error instead of attaching the
wrong commit history. Ensure you reference the existing variables add, attach,
branch, baseRef, worktreeDir and preserve BRANCH_SCHEMA_VERSION behavior when
creating/returning success.
In `@modules/jarvos-coding/src/adapters/live/run.js`:
- Around line 17-24: The child process env passed to spawnSync is currently
options.env which replaces the entire environment; update the call that
constructs the spawnSync options (the object containing cwd, encoding, timeout,
maxBuffer, input, env) to merge process.env with options.env so overrides in
options.env apply but existing vars like PATH remain (e.g. use Object.assign({},
process.env, options.env) or {...process.env, ...(options.env || {})});
reference the spawnSync invocation and the options.env usage and ensure you
handle a missing/undefined options.env safely.
In `@modules/jarvos-coding/src/features/goal-alignment/index.js`:
- Around line 217-225: isGoalClarityInteraction uses issueLabel() which always
returns a presentation fallback, so the current identifier check never lets the
PR-only match path run; update isGoalClarityInteraction to only derive an
identifier when the caller actually supplied an issue identifier (e.g., check
properties like issue.identifier || issue.number || issue.trackerIssueNumber)
instead of unconditionally calling issueLabel(issue), so set identifier =
null/undefined when no explicit id is present and keep matching logic against
payload.issueIdentifier only when an explicit identifier exists; keep the
prUrlForGoalAlignment(detail) usage as-is.
In `@modules/jarvos-coding/src/features/review-engine/index.js`:
- Around line 191-206: interpolateCommand currently only maps issueIdentifier
from input.issueIdentifier or input.issue?.identifier; update the replacements
object in interpolateCommand to accept the same issue-id shapes used elsewhere
by also checking input.issueId, input.issue?.id, and input.issue?.key (in
addition to input.issueIdentifier and input.issue?.identifier) so that
{issueIdentifier} is populated for all supported shapes and the placeholder does
not remain literal; modify the branch that builds replacements (inside
interpolateCommand) to prefer these values in a sensible order (e.g.,
input.issueIdentifier || input.issueId || input.issue?.identifier ||
input.issue?.id || input.issue?.key) while keeping existing keys unchanged.
In `@modules/jarvos-coding/src/lifecycle/policy.js`:
- Around line 386-393: In evaluateIssueBranchReviewEvidence, include the CI
checks' autoreview value as a fallback when computing holisticReview: update the
holisticReview assignment in the function (evaluateIssueBranchReviewEvidence) to
OR in checks.autoreview (e.g. reviewEvidence.localAutoreview ||
reviewEvidence.autoreview || reviewEvidence.holisticReview ||
reviewEvidence.equivalentReview || checks?.autoreview) so the evaluator accepts
the submission-gate shape where holistic review is recorded under
checks.autoreview.
- Around line 501-503: The currentState fallback uses 'missing_issue_intake'
which is not present in ISSUE_BRANCH_LIFECYCLE_STATES or
issueBranchLifecycleContract().states, causing contract validation to fail;
change the fallback to a valid exported lifecycle state (for example replace the
literal 'missing_issue_intake' with ISSUE_BRANCH_LIFECYCLE_STATES[0] or another
specific member from ISSUE_BRANCH_LIFECYCLE_STATES), or alternatively add
'missing_issue_intake' to the exported ISSUE_BRANCH_LIFECYCLE_STATES and
issueBranchLifecycleContract().states so the fallback is a declared/valid state;
update the line that defines currentState accordingly (referencing currentState
and ISSUE_BRANCH_LIFECYCLE_STATES).
In `@modules/jarvos-skills/src/index.js`:
- Around line 154-156: Validate each entry in skill.optionalRequires the same
way you validate skill.requires: ensure skill.optionalRequires is an array
(already done) and then iterate its items to confirm each is a string and exists
in pack.detection.commands; if any item is not a string or not found in
pack.detection.commands throw an Error with the same style message used for
requires (e.g., `${pack.name}/${skill.name} optionalRequires[...] must be a
valid command`), and update the missingOptionalCommands logic (where
missingOptionalCommands is built) to only consider valid string command names
that are present in pack.detection.commands.
- Around line 493-513: The returned ok flag contradicts the computed state in
evaluateLosslessClawHealth(): currently ok is set to state !== 'degraded', which
still marks 'missing' and 'disabled' as healthy. Update the returned object so
ok reflects true only for healthy installations (e.g., set ok to state ===
'healthy' or an equivalent expression), ensuring callers of
evaluateLosslessClawHealth() correctly treat 'missing', 'disabled', and
'degraded' as non-ok.
- Around line 264-286: The code treats any non-'missing' losslessClaw state as
installed; change the detection logic so LOSSLESS_CLAW_PLUGIN_ID is considered
present only when inspectLosslessClaw returns a healthy state (e.g.,
losslessClaw.state === 'ok' or another explicit healthy state your inspector
uses). Update the losslessDetected assignment (the losslessClaw variable and the
losslessDetected boolean) to check for that healthy state before excluding
LOSSLESS_CLAW_PLUGIN_ID from missingCommands/missingOptionalCommands; keep using
inspectLosslessClaw, LOSSLESS_CLAW_PLUGIN_ID, losslessClaw and losslessDetected
to locate and modify the code.
---
Outside diff comments:
In `@modules/jarvos-coding/src/lifecycle/policy.js`:
- Around line 255-262: hasBranchHygiene currently uses branchMentionsIssue() (a
raw substring check) which allows partial matches like "feature/SUP-123-fix"
matching "SUP-12"; update hasBranchHygiene to use the exact-token matcher
instead (replace branchMentionsIssue(git.branch, input) with the stricter
branchIsIssueNamed(...) or branchSatisfiesIdentifier(...) implementation used
elsewhere) so the issueNamed check uses the exact issue-token logic; ensure you
pass the same arguments expected by the chosen matcher (git.branch and the
input/context) and remove the substring-based check.
In `@modules/jarvos-skills/src/index.js`:
- Around line 121-127: The manifest validation in assertPackManifest() currently
checks pack.detection.commands but never validates pack.detection.files, which
later causes detectPackEnvironment() to read file.name/file.path and potentially
crash; update assertPackManifest() to require pack.detection.files is an array
and iterate each entry to ensure it is an object (not an array) and that each
entry has string-typed name and path properties (mirroring the existing checks
for detection.commands), throwing a descriptive Error including pack.name when
any file entry is malformed so invalid packs fail fast.
---
Nitpick comments:
In `@modules/jarvos-coding/src/adapters/hosts.js`:
- Around line 61-64: The continuityAdapterMatrix() should treat the
contract.continuity block as the single source of truth: replace readOnEntry and
writeCheckpoint to read from contract.continuity (use
contract.continuity.readOnEntry and contract.continuity.writeCheckpoint) instead
of contract.entryBehavior.readSessionState or checking contract.requiredTools;
keep hydrateLiveArtifact mapped from
contract.entryBehavior.readLiveArtifactFromPointer only if continuity does not
provide an explicit alternative, and preserve freshSessionSurvival as
contract.continuity.freshSessionSurvival.
- Around line 55-78: The catch around codingHostAdapterContract(host) is too
broad and turns any thrown error into an unsupported-host response; update the
try/catch so only the specific "unsupported host" error is handled and converted
to the fallback object (use error.name === 'UnsupportedHostError' or a bespoke
isUnsupportedHost(error) check), and rethrow any other errors so regressions in
codingHostAdapterContract surface; continue to call normalizeHost(host) and
articleFor(normalizedHost) only in the unsupported-host branch and keep the
returned properties (status:'unsupported', readOnEntry:false, etc.) there.
🪄 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: 75bec1a2-9e95-4ae7-a512-15a02e8672e7
📒 Files selected for processing (16)
modules/jarvos-coding/src/adapters/hosts.jsmodules/jarvos-coding/src/adapters/live/branch.jsmodules/jarvos-coding/src/adapters/live/clawpatch-review.jsmodules/jarvos-coding/src/adapters/live/fixer.jsmodules/jarvos-coding/src/adapters/live/holistic-review.jsmodules/jarvos-coding/src/adapters/live/index.jsmodules/jarvos-coding/src/adapters/live/paperclip-tracker.jsmodules/jarvos-coding/src/adapters/live/pull-request.jsmodules/jarvos-coding/src/adapters/live/run.jsmodules/jarvos-coding/src/features/goal-alignment/index.jsmodules/jarvos-coding/src/features/review-engine/index.jsmodules/jarvos-coding/src/features/session-state/index.jsmodules/jarvos-coding/src/features/submission-gate/index.jsmodules/jarvos-coding/src/index.jsmodules/jarvos-coding/src/lifecycle/policy.jsmodules/jarvos-skills/src/index.js
| // Create a new branch in its own worktree off the base ref. | ||
| const add = run('git', ['worktree', 'add', '-b', branch, worktreeDir, baseRef], { | ||
| cwd: repoRootDir, | ||
| timeoutMs: 120000, | ||
| allowFail: true, | ||
| }); | ||
|
|
||
| if (add.status === 0) { | ||
| return { | ||
| schemaVersion: BRANCH_SCHEMA_VERSION, | ||
| status: 'created', | ||
| mode: 'branch', | ||
| ok: true, | ||
| branch, | ||
| baseRef, | ||
| worktreeDir, | ||
| }; | ||
| } | ||
|
|
||
| // The branch may already exist (e.g. a resumed run). Attach a worktree to | ||
| // the existing branch — keeping its name meaningful for the later PR — | ||
| // rather than failing the stage. | ||
| const attach = run('git', ['worktree', 'add', worktreeDir, branch], { | ||
| cwd: repoRootDir, | ||
| timeoutMs: 120000, | ||
| allowFail: true, | ||
| }); |
There was a problem hiding this comment.
Only attach an existing branch when branch creation failed for that reason.
Any git worktree add -b ... <baseRef> failure currently falls through to attaching branch. If the real problem is an invalid baseRef or a fetch/update failure, this can still attach an older local branch and report success from the wrong commit history.
Suggested fix
const add = run('git', ['worktree', 'add', '-b', branch, worktreeDir, baseRef], {
cwd: repoRootDir,
timeoutMs: 120000,
allowFail: true,
});
if (add.status === 0) {
return {
schemaVersion: BRANCH_SCHEMA_VERSION,
status: 'created',
mode: 'branch',
ok: true,
branch,
baseRef,
worktreeDir,
};
}
+ const addError = `${add.stderr || ''}\n${add.stdout || ''}`;
+ const branchAlreadyExists = /already exists/u.test(addError);
+ if (!branchAlreadyExists) {
+ return {
+ schemaVersion: BRANCH_SCHEMA_VERSION,
+ status: 'failed',
+ ok: false,
+ branch,
+ baseRef,
+ worktreeDir,
+ error: addError.trim(),
+ };
+ }
+
const attach = run('git', ['worktree', 'add', worktreeDir, branch], {
cwd: repoRootDir,
timeoutMs: 120000,
allowFail: true,
});🤖 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-coding/src/adapters/live/branch.js` around lines 74 - 100, The
code currently falls back to attaching the existing branch on any failure
creating the branch; change this so you only attach when the git create failure
indicates the branch already exists. After calling run('git', ['worktree',
'add', '-b', branch, worktreeDir, baseRef], ...) inspect the result (e.g.,
add.status and add.stderr/message) for the specific "branch already exists"
condition (match the git error text like "A branch named" / "already exists" or
equivalent) and only then run the attach sequence (run('git', ['worktree',
'add', worktreeDir, branch], ...)). If the error is anything else (invalid
baseRef, fetch failure, etc.) return/throw a failure including add.stderr so the
caller sees the real error instead of attaching the wrong commit history. Ensure
you reference the existing variables add, attach, branch, baseRef, worktreeDir
and preserve BRANCH_SCHEMA_VERSION behavior when creating/returning success.
| const res = spawnSync(cmd, args, { | ||
| cwd: options.cwd, | ||
| encoding: 'utf8', | ||
| timeout: options.timeoutMs || DEFAULT_TIMEOUT_MS, | ||
| maxBuffer: options.maxBuffer || DEFAULT_MAX_BUFFER, | ||
| input: options.input, | ||
| env: options.env, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.spawnSync, does providing options.env replace the child process environment instead of merging with process.env?
💡 Result:
In Node.js, the options.env property in child_process.spawnSync (and other spawn methods) replaces the environment variables of the child process rather than merging with process.env [1][2]. The env option defaults to process.env if it is not provided [1][3]. However, if you explicitly supply an object to the env option, only the key-value pairs contained within that object will be visible to the child process [2]. The child process will not inherit any variables from the parent process's process.env unless they are manually included in the object you pass [2]. To effectively "merge" environments, you must manually spread or copy the existing variables into the new environment object: const { spawnSync } = require('node:child_process'); spawnSync('command', { env: {...process.env, NEW_VARIABLE: 'value' } });
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: node:child_process always inherits parent env variables denoland/deno#27343
- 3: https://r2.nodejs.org/docs/v22.17.1/api/child_process.html
Merge options.env with process.env before calling spawnSync.
spawnSync treats options.env as the complete child environment; if callers pass only overrides, variables like PATH can be dropped and git / gh / npx resolution can fail.
Suggested fix
const res = spawnSync(cmd, args, {
cwd: options.cwd,
encoding: 'utf8',
timeout: options.timeoutMs || DEFAULT_TIMEOUT_MS,
maxBuffer: options.maxBuffer || DEFAULT_MAX_BUFFER,
input: options.input,
- env: options.env,
+ env: options.env ? { ...process.env, ...options.env } : process.env,
});📝 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.
| const res = spawnSync(cmd, args, { | |
| cwd: options.cwd, | |
| encoding: 'utf8', | |
| timeout: options.timeoutMs || DEFAULT_TIMEOUT_MS, | |
| maxBuffer: options.maxBuffer || DEFAULT_MAX_BUFFER, | |
| input: options.input, | |
| env: options.env, | |
| }); | |
| const res = spawnSync(cmd, args, { | |
| cwd: options.cwd, | |
| encoding: 'utf8', | |
| timeout: options.timeoutMs || DEFAULT_TIMEOUT_MS, | |
| maxBuffer: options.maxBuffer || DEFAULT_MAX_BUFFER, | |
| input: options.input, | |
| env: options.env ? { ...process.env, ...options.env } : process.env, | |
| }); |
🤖 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-coding/src/adapters/live/run.js` around lines 17 - 24, The
child process env passed to spawnSync is currently options.env which replaces
the entire environment; update the call that constructs the spawnSync options
(the object containing cwd, encoding, timeout, maxBuffer, input, env) to merge
process.env with options.env so overrides in options.env apply but existing vars
like PATH remain (e.g. use Object.assign({}, process.env, options.env) or
{...process.env, ...(options.env || {})}); reference the spawnSync invocation
and the options.env usage and ensure you handle a missing/undefined options.env
safely.
| function isGoalClarityInteraction(interaction, issue = {}, detail = {}) { | ||
| if (!interaction || interaction.kind !== 'request_confirmation') return false; | ||
| if (String(interaction.status || '').toLowerCase() !== 'pending') return false; | ||
| const payload = interaction.payload || {}; | ||
| if (payload.purpose !== 'goal_clarity') return false; | ||
| const identifier = issueLabel(issue); | ||
| const url = prUrlForGoalAlignment(detail); | ||
| return (!identifier || payload.issueIdentifier === identifier) | ||
| && (!url || payload.prUrl === url); |
There was a problem hiding this comment.
Don't use the presentation fallback as a matching key.
issueLabel() always falls back to "the tracked issue", so the !identifier branch here never triggers. That means findPendingGoalClarityInteraction() cannot match by PR alone when the caller omits issue data, and it can reopen duplicate goal-clarity requests for the same PR until the tracker issue is populated.
Suggested fix
function isGoalClarityInteraction(interaction, issue = {}, detail = {}) {
if (!interaction || interaction.kind !== 'request_confirmation') return false;
if (String(interaction.status || '').toLowerCase() !== 'pending') return false;
const payload = interaction.payload || {};
if (payload.purpose !== 'goal_clarity') return false;
- const identifier = issueLabel(issue);
+ const identifier = issue.identifier || issue.key || issue.id || '';
const url = prUrlForGoalAlignment(detail);
return (!identifier || payload.issueIdentifier === identifier)
&& (!url || payload.prUrl === url);
}📝 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 isGoalClarityInteraction(interaction, issue = {}, detail = {}) { | |
| if (!interaction || interaction.kind !== 'request_confirmation') return false; | |
| if (String(interaction.status || '').toLowerCase() !== 'pending') return false; | |
| const payload = interaction.payload || {}; | |
| if (payload.purpose !== 'goal_clarity') return false; | |
| const identifier = issueLabel(issue); | |
| const url = prUrlForGoalAlignment(detail); | |
| return (!identifier || payload.issueIdentifier === identifier) | |
| && (!url || payload.prUrl === url); | |
| function isGoalClarityInteraction(interaction, issue = {}, detail = {}) { | |
| if (!interaction || interaction.kind !== 'request_confirmation') return false; | |
| if (String(interaction.status || '').toLowerCase() !== 'pending') return false; | |
| const payload = interaction.payload || {}; | |
| if (payload.purpose !== 'goal_clarity') return false; | |
| const identifier = issue.identifier || issue.key || issue.id || ''; | |
| const url = prUrlForGoalAlignment(detail); | |
| return (!identifier || payload.issueIdentifier === identifier) | |
| && (!url || payload.prUrl === url); | |
| } |
🤖 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-coding/src/features/goal-alignment/index.js` around lines 217
- 225, isGoalClarityInteraction uses issueLabel() which always returns a
presentation fallback, so the current identifier check never lets the PR-only
match path run; update isGoalClarityInteraction to only derive an identifier
when the caller actually supplied an issue identifier (e.g., check properties
like issue.identifier || issue.number || issue.trackerIssueNumber) instead of
unconditionally calling issueLabel(issue), so set identifier = null/undefined
when no explicit id is present and keep matching logic against
payload.issueIdentifier only when an explicit identifier exists; keep the
prUrlForGoalAlignment(detail) usage as-is.
| function interpolateCommand(command = [], input = {}) { | ||
| const replacements = { | ||
| base: input.base || input.baseRef || '', | ||
| baseRef: input.baseRef || input.base || '', | ||
| branch: input.branch || '', | ||
| issueIdentifier: input.issueIdentifier || input.issue?.identifier || '', | ||
| mergeSha: input.mergeSha || input.mergeCommit || input.pullRequest?.mergeSha || '', | ||
| pullRequestNumber: input.pullRequestNumber || input.pullRequest?.number || '', | ||
| pullRequestUrl: input.pullRequestUrl || input.pullRequest?.url || '', | ||
| }; | ||
|
|
||
| return cloneCommand(command).map((part) => part.replace(/\{([a-zA-Z0-9_]+)\}/g, (match, key) => ( | ||
| Object.prototype.hasOwnProperty.call(replacements, key) && replacements[key] | ||
| ? String(replacements[key]) | ||
| : match | ||
| ))); |
There was a problem hiding this comment.
Accept the same issue-id shapes the rest of the gate stack accepts.
{issueIdentifier} only reads input.issueIdentifier / input.issue?.identifier, but the surrounding contracts also accept issue.id, issue.key, and issueId (modules/jarvos-coding/src/lifecycle/policy.js Lines 236-245 and modules/jarvos-coding/src/features/submission-gate/index.js Lines 85-102). With those normal shapes, the Hermes / Paperclip commands here keep the literal placeholder and dispatch against the wrong parent issue.
Suggested fix
const replacements = {
base: input.base || input.baseRef || '',
baseRef: input.baseRef || input.base || '',
branch: input.branch || '',
- issueIdentifier: input.issueIdentifier || input.issue?.identifier || '',
+ issueIdentifier: input.issueIdentifier
+ || input.issueId
+ || input.issue?.identifier
+ || input.issue?.key
+ || input.issue?.id
+ || '',
mergeSha: input.mergeSha || input.mergeCommit || input.pullRequest?.mergeSha || '',
pullRequestNumber: input.pullRequestNumber || input.pullRequest?.number || '',
pullRequestUrl: input.pullRequestUrl || input.pullRequest?.url || '',
};📝 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 interpolateCommand(command = [], input = {}) { | |
| const replacements = { | |
| base: input.base || input.baseRef || '', | |
| baseRef: input.baseRef || input.base || '', | |
| branch: input.branch || '', | |
| issueIdentifier: input.issueIdentifier || input.issue?.identifier || '', | |
| mergeSha: input.mergeSha || input.mergeCommit || input.pullRequest?.mergeSha || '', | |
| pullRequestNumber: input.pullRequestNumber || input.pullRequest?.number || '', | |
| pullRequestUrl: input.pullRequestUrl || input.pullRequest?.url || '', | |
| }; | |
| return cloneCommand(command).map((part) => part.replace(/\{([a-zA-Z0-9_]+)\}/g, (match, key) => ( | |
| Object.prototype.hasOwnProperty.call(replacements, key) && replacements[key] | |
| ? String(replacements[key]) | |
| : match | |
| ))); | |
| function interpolateCommand(command = [], input = {}) { | |
| const replacements = { | |
| base: input.base || input.baseRef || '', | |
| baseRef: input.baseRef || input.base || '', | |
| branch: input.branch || '', | |
| issueIdentifier: input.issueIdentifier | |
| || input.issueId | |
| || input.issue?.identifier | |
| || input.issue?.key | |
| || input.issue?.id | |
| || '', | |
| mergeSha: input.mergeSha || input.mergeCommit || input.pullRequest?.mergeSha || '', | |
| pullRequestNumber: input.pullRequestNumber || input.pullRequest?.number || '', | |
| pullRequestUrl: input.pullRequestUrl || input.pullRequest?.url || '', | |
| }; | |
| return cloneCommand(command).map((part) => part.replace(/\{([a-zA-Z0-9_]+)\}/g, (match, key) => ( | |
| Object.prototype.hasOwnProperty.call(replacements, key) && replacements[key] | |
| ? String(replacements[key]) | |
| : match | |
| ))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/jarvos-coding/src/features/review-engine/index.js` around lines 191 -
206, interpolateCommand currently only maps issueIdentifier from
input.issueIdentifier or input.issue?.identifier; update the replacements object
in interpolateCommand to accept the same issue-id shapes used elsewhere by also
checking input.issueId, input.issue?.id, and input.issue?.key (in addition to
input.issueIdentifier and input.issue?.identifier) so that {issueIdentifier} is
populated for all supported shapes and the placeholder does not remain literal;
modify the branch that builds replacements (inside interpolateCommand) to prefer
these values in a sensible order (e.g., input.issueIdentifier || input.issueId
|| input.issue?.identifier || input.issue?.id || input.issue?.key) while keeping
existing keys unchanged.
| function evaluateIssueBranchReviewEvidence(input = {}) { | ||
| const reviewEvidence = input.reviewEvidence || input.review || {}; | ||
| const pullRequest = input.pullRequest || input.pr || {}; | ||
| const checks = reviewEvidence.checks || input.checks?.ci || pullRequest.checks; | ||
| const holisticReview = reviewEvidence.localAutoreview | ||
| || reviewEvidence.autoreview | ||
| || reviewEvidence.holisticReview | ||
| || reviewEvidence.equivalentReview; |
There was a problem hiding this comment.
Read holistic review from checks.autoreview too.
The submission-gate contract in this same file records the holistic review at checks.autoreview (Lines 149-152), but this evaluator only looks under reviewEvidence.*. With the normal gate-shaped input, evaluateIssueBranchLifecycle() will keep reporting reviewEvidence.holisticReview missing unless callers duplicate the same evidence into a second field.
🤖 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-coding/src/lifecycle/policy.js` around lines 386 - 393, In
evaluateIssueBranchReviewEvidence, include the CI checks' autoreview value as a
fallback when computing holisticReview: update the holisticReview assignment in
the function (evaluateIssueBranchReviewEvidence) to OR in checks.autoreview
(e.g. reviewEvidence.localAutoreview || reviewEvidence.autoreview ||
reviewEvidence.holisticReview || reviewEvidence.equivalentReview ||
checks?.autoreview) so the evaluator accepts the submission-gate shape where
holistic review is recorded under checks.autoreview.
| const currentState = [...ISSUE_BRANCH_LIFECYCLE_STATES] | ||
| .reverse() | ||
| .find((state) => milestones[state]) || 'missing_issue_intake'; |
There was a problem hiding this comment.
currentState can return a value that is not in the exported lifecycle enum.
When no milestone is met, this falls back to 'missing_issue_intake', but that key is absent from ISSUE_BRANCH_LIFECYCLE_STATES and from issueBranchLifecycleContract().states. Since both are public exports, consumers validating currentState against the contract will reject a valid evaluator result.
🤖 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-coding/src/lifecycle/policy.js` around lines 501 - 503, The
currentState fallback uses 'missing_issue_intake' which is not present in
ISSUE_BRANCH_LIFECYCLE_STATES or issueBranchLifecycleContract().states, causing
contract validation to fail; change the fallback to a valid exported lifecycle
state (for example replace the literal 'missing_issue_intake' with
ISSUE_BRANCH_LIFECYCLE_STATES[0] or another specific member from
ISSUE_BRANCH_LIFECYCLE_STATES), or alternatively add 'missing_issue_intake' to
the exported ISSUE_BRANCH_LIFECYCLE_STATES and
issueBranchLifecycleContract().states so the fallback is a declared/valid state;
update the line that defines currentState accordingly (referencing currentState
and ISSUE_BRANCH_LIFECYCLE_STATES).
| if (skill.optionalRequires !== undefined && !Array.isArray(skill.optionalRequires)) { | ||
| throw new Error(`${pack.name}/${skill.name} optionalRequires must be an array`); | ||
| } |
There was a problem hiding this comment.
Validate each optionalRequires entry, not just the array container.
Right now any value inside optionalRequires is accepted. Non-string entries or typos then flow into missingOptionalCommands on Lines 282-286, and unlike requires they are never checked against pack.detection.commands. Mirror the per-item validation you already do for requires.
Also applies to: 282-286
🤖 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-skills/src/index.js` around lines 154 - 156, Validate each
entry in skill.optionalRequires the same way you validate skill.requires: ensure
skill.optionalRequires is an array (already done) and then iterate its items to
confirm each is a string and exists in pack.detection.commands; if any item is
not a string or not found in pack.detection.commands throw an Error with the
same style message used for requires (e.g., `${pack.name}/${skill.name}
optionalRequires[...] must be a valid command`), and update the
missingOptionalCommands logic (where missingOptionalCommands is built) to only
consider valid string command names that are present in pack.detection.commands.
| const losslessClaw = pack.name === 'local-openclaw' | ||
| ? inspectLosslessClaw({ | ||
| homeDir: options.homeDir, | ||
| workspaceRoot: options.workspaceRoot, | ||
| openclawStateDir: options.openclawStateDir, | ||
| commandsPresent: options.commandsPresent, | ||
| environment, | ||
| now: options.now, | ||
| }) | ||
| : null; | ||
| const losslessDetected = Boolean(losslessClaw && losslessClaw.state !== 'missing'); | ||
|
|
||
| const skills = pack.skills.map((skill) => { | ||
| const missingCommands = skill.requires.filter((commandName) => { | ||
| if (commandName === LOSSLESS_CLAW_PLUGIN_ID && losslessDetected) return false; | ||
| const command = commandMap.get(commandName); | ||
| return !command || !command.present; | ||
| }); | ||
| const missingOptionalCommands = (skill.optionalRequires || []).filter((commandName) => { | ||
| if (commandName === LOSSLESS_CLAW_PLUGIN_ID && losslessDetected) return false; | ||
| const command = commandMap.get(commandName); | ||
| return !command || !command.present; | ||
| }); |
There was a problem hiding this comment.
Don't treat disabled or degraded lossless-claw as installed.
losslessDetected is true for every state except 'missing'. That means a broken OpenClaw setup can still clear LOSSLESS_CLAW_PLUGIN_ID from missingCommands, and the doctor/install-plan path in modules/jarvos-skills/scripts/install-skills.js:115-124 will report a healthier plan than the actual runtime can support.
Suggested fix
- const losslessDetected = Boolean(losslessClaw && losslessClaw.state !== 'missing');
+ const losslessDetected = Boolean(losslessClaw && losslessClaw.state === 'healthy');📝 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.
| const losslessClaw = pack.name === 'local-openclaw' | |
| ? inspectLosslessClaw({ | |
| homeDir: options.homeDir, | |
| workspaceRoot: options.workspaceRoot, | |
| openclawStateDir: options.openclawStateDir, | |
| commandsPresent: options.commandsPresent, | |
| environment, | |
| now: options.now, | |
| }) | |
| : null; | |
| const losslessDetected = Boolean(losslessClaw && losslessClaw.state !== 'missing'); | |
| const skills = pack.skills.map((skill) => { | |
| const missingCommands = skill.requires.filter((commandName) => { | |
| if (commandName === LOSSLESS_CLAW_PLUGIN_ID && losslessDetected) return false; | |
| const command = commandMap.get(commandName); | |
| return !command || !command.present; | |
| }); | |
| const missingOptionalCommands = (skill.optionalRequires || []).filter((commandName) => { | |
| if (commandName === LOSSLESS_CLAW_PLUGIN_ID && losslessDetected) return false; | |
| const command = commandMap.get(commandName); | |
| return !command || !command.present; | |
| }); | |
| const losslessClaw = pack.name === 'local-openclaw' | |
| ? inspectLosslessClaw({ | |
| homeDir: options.homeDir, | |
| workspaceRoot: options.workspaceRoot, | |
| openclawStateDir: options.openclawStateDir, | |
| commandsPresent: options.commandsPresent, | |
| environment, | |
| now: options.now, | |
| }) | |
| : null; | |
| const losslessDetected = Boolean(losslessClaw && losslessClaw.state === 'healthy'); | |
| const skills = pack.skills.map((skill) => { | |
| const missingCommands = skill.requires.filter((commandName) => { | |
| if (commandName === LOSSLESS_CLAW_PLUGIN_ID && losslessDetected) return false; | |
| const command = commandMap.get(commandName); | |
| return !command || !command.present; | |
| }); | |
| const missingOptionalCommands = (skill.optionalRequires || []).filter((commandName) => { | |
| if (commandName === LOSSLESS_CLAW_PLUGIN_ID && losslessDetected) return false; | |
| const command = commandMap.get(commandName); | |
| return !command || !command.present; | |
| }); |
🤖 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-skills/src/index.js` around lines 264 - 286, The code treats
any non-'missing' losslessClaw state as installed; change the detection logic so
LOSSLESS_CLAW_PLUGIN_ID is considered present only when inspectLosslessClaw
returns a healthy state (e.g., losslessClaw.state === 'ok' or another explicit
healthy state your inspector uses). Update the losslessDetected assignment (the
losslessClaw variable and the losslessDetected boolean) to check for that
healthy state before excluding LOSSLESS_CLAW_PLUGIN_ID from
missingCommands/missingOptionalCommands; keep using inspectLosslessClaw,
LOSSLESS_CLAW_PLUGIN_ID, losslessClaw and losslessDetected to locate and modify
the code.
| let state = 'missing'; | ||
| if (installedOrDetected && slot !== LOSSLESS_CLAW_PLUGIN_ID) { | ||
| state = 'disabled'; | ||
| } else if (slot === LOSSLESS_CLAW_PLUGIN_ID) { | ||
| state = reasons.length > 0 ? 'degraded' : 'healthy'; | ||
| } | ||
|
|
||
| return { | ||
| state, | ||
| ok: state !== 'degraded', | ||
| pluginId: LOSSLESS_CLAW_PLUGIN_ID, | ||
| commandPresent, | ||
| slot, | ||
| installPath, | ||
| installPathExists, | ||
| extensionExists, | ||
| summaryModel, | ||
| unsafeSummaryModel, | ||
| autoUpdateEnabled, | ||
| reasons, | ||
| }; |
There was a problem hiding this comment.
ok currently contradicts the returned health state.
When the plugin is 'missing' or 'disabled', this still returns ok: true even though reasons contains failure messages. Since evaluateLosslessClawHealth() is exported, callers branching on ok will misclassify unhealthy setups.
Suggested fix
return {
state,
- ok: state !== 'degraded',
+ ok: state === 'healthy',
pluginId: LOSSLESS_CLAW_PLUGIN_ID,📝 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.
| let state = 'missing'; | |
| if (installedOrDetected && slot !== LOSSLESS_CLAW_PLUGIN_ID) { | |
| state = 'disabled'; | |
| } else if (slot === LOSSLESS_CLAW_PLUGIN_ID) { | |
| state = reasons.length > 0 ? 'degraded' : 'healthy'; | |
| } | |
| return { | |
| state, | |
| ok: state !== 'degraded', | |
| pluginId: LOSSLESS_CLAW_PLUGIN_ID, | |
| commandPresent, | |
| slot, | |
| installPath, | |
| installPathExists, | |
| extensionExists, | |
| summaryModel, | |
| unsafeSummaryModel, | |
| autoUpdateEnabled, | |
| reasons, | |
| }; | |
| let state = 'missing'; | |
| if (installedOrDetected && slot !== LOSSLESS_CLAW_PLUGIN_ID) { | |
| state = 'disabled'; | |
| } else if (slot === LOSSLESS_CLAW_PLUGIN_ID) { | |
| state = reasons.length > 0 ? 'degraded' : 'healthy'; | |
| } | |
| return { | |
| state, | |
| ok: state === 'healthy', | |
| pluginId: LOSSLESS_CLAW_PLUGIN_ID, | |
| commandPresent, | |
| slot, | |
| installPath, | |
| installPathExists, | |
| extensionExists, | |
| summaryModel, | |
| unsafeSummaryModel, | |
| autoUpdateEnabled, | |
| reasons, | |
| }; |
🤖 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-skills/src/index.js` around lines 493 - 513, The returned ok
flag contradicts the computed state in evaluateLosslessClawHealth(): currently
ok is set to state !== 'degraded', which still marks 'missing' and 'disabled' as
healthy. Update the returned object so ok reflects true only for healthy
installations (e.g., set ok to state === 'healthy' or an equivalent expression),
ensuring callers of evaluateLosslessClawHealth() correctly treat 'missing',
'disabled', and 'degraded' as non-ok.
…ex review) Address Codex P1s on PR #70 in the additive live-adapter layer: - paperclip-tracker: defer clawd's paperclip-pr-link require until a live method runs (lazy + memoized) so buildLiveCodingAdapters() is constructible in the public package where clawd scripts are absent (previously threw at construction). - paperclip-tracker.verifyAndClose: never transition an issue to done without merge evidence; return status 'deferred' on an open PR. Prevents premature close — the same dangerous class as a false-close. - add test/live-adapters.test.js covering construction-without-clawd, defer-on-open, close-on-merged, and sweep-no-op-on-unmerged. Deferred to SUP-2263 live-adapter hardening (tracked): clawpatch-review runner honoring branchResult.worktreeDir (P2) — needs review-engine payload threading and the live review path is not yet exercised by any public consumer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @chatgpt-codex-connector — all three addressed:
Full |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a43e6df0db
ℹ️ 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".
| } | ||
|
|
||
| function initJarvosWorkspace(options = {}) { | ||
| const pack = options.pack || loadPack(options.packName || 'local-openclaw'); |
There was a problem hiding this comment.
When callers use the exported initJarvosWorkspace() without an explicit pack/packName, this attempts to load packs/local-openclaw.json, but the packaged modules/jarvos-skills/packs directory only contains obsidian-default.json. That makes the default initializer throw ENOENT before it can create any workspace, so either ship the local-openclaw pack or default to an included pack / require the caller to choose one.
Useful? React with 👍 / 👎.
What & why
First build step of the v0.5.0 consolidation (epic SUP-2259). The public repo and the clawd-root build had diverged; this brings public's
@jarvos/codingand@jarvos/skillsmodules to parity by merging both sides — nothing clobbered. Public stays canonical.This unblocks the ASAP v0.5 cut. It was hand-reconciled in a clean checkout because the coding runtime currently can't target a non-clawd repo for execution (the fix for that is the follow-up PR, sub-goal A).
@jarvos/coding
adapters/hosts.js— UNION: keep public's MCP/skill descriptors + host-adapter factories (createClaudeCodeHostAdapter/createCodexHostAdapter/createCodingHostAdapter); add clawd'scontinuitycontract block,continuityAdapterMatrix, andhermes/personalityhosts.review-engine,lifecycle/policy,session-state,submission-gate— clawd supersets (gate-equivalents, issue-branch lifecycle, goal-alignment gate key, continuity). Verified no public-only export/function dropped.index.js— UNION barrel: clawd's new exports + public's host-adapter/MCP/skill exports.adapters/live/*×8 +features/goal-alignment) — brought in as the require-closure needed to load the reconciled barrel. This materially lands SUP-2263's live-adapter layer, so a fresh install runs a real issue, not just a mock.@jarvos/skills
src/index.js— UNION: keep public's skill-install engine (getManifest/getSkill/listSkills/installSkills/validateBundle) + strictassertPackManifest+ win32-awarecommandExists; add clawd's workspace/lossless suite (initJarvosWorkspace,evaluateLosslessClawHealth,inspectLosslessClaw,enableLosslessClawInOpenClawConfig) anddetection.filessupport;detectPackEnvironment/buildInstallPlanupgraded to the superset.local-openclaw/v0-5-0pack data defers with the rest of the additive layer (bringing it now would force loosening the test-pinned strict validator). Engine capability is preserved; only the pack JSON defers.Tests
npm test(bootstrap smoke + modules-smoke + cli-smoke + structure smoke): green (60/60 structure, 55/55 modules-smoke).@jarvos/codingmodule tests: 9/9.@jarvos/skillssuite: PASS.Scope notes
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements