Repository navigation
integration(HOLMES): MP‑01..03 + score breakdown + CI detect + precommit - #334
Conversation
…rojection schema/render skeleton; stabilize tests
…ree id only); soft-fail behind --project-merge
…erged tree via write-tree; fallback from merge-tree
…ojection clean merge is unavailable or conflicts; render projection impact
…tes for major SaaS; link from merge-projection plan
feat(holmes): MP-05 — worktree fallback + conflict detection
WIP: moriarty-tests
…27' into integration/holmes-2025-10-27
…nto integration/holmes-2025-10-27
…25-10-27 Conflict resolution policy: - Keep detect-schema + pre-commit from earlier merges - Prefer score breakdown versions for HOLMES code/schema/tests - Keep HEAD versions for AGENTS.md and holmes integration docs - Drop go-public-checklist.md (deleted)
… integration/holmes-2025-10-27
|
Caution Review failedFailed to post review comments Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
Subject: Re: Your PR is a comprehensive but MASSIVE refactor — here's my cut-through analysis Look, I'm going to be direct because that's what you need: you've submitted something that touches nearly every corner of the codebase in a single PR. That's a problem for review, but let's at least get the facts straight. WalkthroughAdds pnpm lockfile auto-sync hook, refactors CI workflows with path-based detection, extends HOLMES/Moriarty with Git activity analysis and merge projection capabilities, introduces policy-driven configuration system with templates, restructures scoring/bundle/evidence schemas, and adds issue creation automation. Changes
Sequence DiagramssequenceDiagram
actor Developer
participant PreCommit as Pre-commit Hook
participant Manifest as package.json/<br/>pnpm-workspace.yaml
participant pnpm
participant Lock as pnpm-lock.yaml
Developer->>PreCommit: git commit
PreCommit->>Manifest: inspect staged changes
alt Resolution fields affected
PreCommit->>pnpm: validate availability & version
alt pnpm OK
PreCommit->>pnpm: pnpm install --lockfile-only
pnpm->>Lock: regenerate
PreCommit->>Lock: stage updated lock
PreCommit-->>Developer: ✓ commit proceeds
else pnpm missing/mismatched
PreCommit-->>Developer: ✗ block commit, guide user
end
else No resolution impact
PreCommit-->>Developer: ✓ short-circuit no-op
end
sequenceDiagram
participant CLI as holmes predict
participant Git as Git / Worktree
participant Merge as MergeTree/<br/>Worktree Strategy
participant Moriarty as Moriarty Scorer
CLI->>CLI: load bundle, history, context
alt --project-merge provided
CLI->>CLI: build MergePlanner
CLI->>Merge: plan(baseRef)
Merge-->>CLI: planned
alt MergeTreeStrategy available
CLI->>Git: merge-tree --write-tree HEAD..base
Git-->>Merge: mergedTree id, status
else fallback
CLI->>Git: worktree checkout base
Git->>Git: attempt merge in worktree
alt conflicts detected
Git-->>Merge: status: conflicts + file list
else clean
Git-->>Merge: status: clean + tree id
end
Git->>Git: cleanup worktree
end
Merge-->>CLI: status, mergedTree, details
CLI->>Moriarty: incorporate projection result
alt projection failed or conflicts
Moriarty->>Moriarty: penalize confidence (-30/-50)
Moriarty->>Moriarty: record MERGE_PROJECTION_ISSUE
end
else no merge
Moriarty->>Moriarty: standard readiness/plateau logic
end
Moriarty-->>CLI: report with readiness explain
CLI->>CLI: write moriarty-report.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring close scrutiny:
Possibly related issues
Possibly related PRs
Poem
Now, the hard truth: This PR is too large. It spans scoring logic, merge strategies, CLI surface, schema changes, and testing in one go. You've mixed infrastructure (hooks, workflows), feature work (merge projection, Git activity), refactoring (UID namespaces, bundle structures), and documentation. That makes it nearly impossible to review cleanly or to bisect if bugs emerge. What you should have done:
Yes, it would take longer. But reviewers could reason about each layer independently, and regressions would be localized. For now: Split this. Get the scoring refactor reviewed separately. Then the merge logic. Then the rest. I know it's painful, but shipping a monolith like this is a maintenance nightmare waiting to happen. BunBun Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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.
ℹ️ 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".
…ons/checkout@v5 with fetch-depth: 0
…(CLI --out-dir) so ops artifacts land under ; keep behavior for explicit artifact.path
|
Update: pushed writer fallback fix (host-node writer.writeFiles respects --out-dir), moved field source recording inside buildTable loop (landed via #337), refreshed HOLMES integration to normalize evidence breakdown and weight sources. All CLI Bats pass locally, @wesley/core unit tests green. Ready for CI.\n\nRisk: low; touches writer fallback and HOLMES normalization only.\n\nNext: once green, we can re-run the HOLMES workflow on this PR to exercise the rendering pipeline. |
🚢 SHIPME[REALM] PASS — sha b360364 |
🔍 The Case of Pull Request #334🕵️ SHA-lock HOLMES's Investigation (click to expand)🕵️ SHA-lock HOLMES Investigation
🔍 Executive Deduction"Watson, after careful examination of the evidence, I deduce..." Weighted Completion: ██████░░░░ 60.0% 🧩 SCS Breakdown
🧪 TCI Breakdown
|
| Component | Risk Share | Points | Count |
|---|---|---|---|
| Drops | 100.0% | 20 | 0 |
| Renames Without Uid | 0.0% | 0 | 0 |
| Add Not Null Without Default | 0.0% | 0 | 0 |
| Non Concurrent Indexes | 0.0% | 0 | 0 |
📊 The Weight of Evidence
"Observe, Watson, how not all features carry equal importance..."
| Element | Weight | Status | Evidence | Deduction |
|---|---|---|---|---|
| schema | 5 | ✅ SQL & tests | out/schema.sql:1-9999@b360364 | Elementary! |
🚪 Security & Performance Gates
"Elementary security measures, Watson..."
| Gate | Status | Evidence | Holmes's Ruling |
|---|---|---|---|
| Migration Risk | ✅ | MRI: 20.0% | "Acceptable risk" |
| Test Coverage | TCI: 70.0% | "Insufficient coverage" | |
| Sensitive Fields | ✅ | 0 fields | "All secured" |
📋 The Verdict
"Some clues remain unclear. Address the noted issues."
Signed and sealed,
- S. Holmes, Consulting Detective
[END OF INVESTIGATION FOR COMMIT b360364]
🩺 Dr. WATSON's Verification (click to expand)
🩺 Dr. Watson's Independent Verification Report
Medical Examination of Evidence
- Examination Date: 2025-10-29T16:54:48.742Z
- Patient SHA: b360364
🔬 Citation Verification
"Let me examine each piece of evidence independently..."
- Citations Examined: 2
- Verified: 0 ✅
- Failed: 0 ❌
- Unable to Verify: 2
Verification Rate: 0.0%
📊 Mathematical Verification
"I shall recalculate Holmes's arithmetic..."
Holmes claimed SCS: 60.0%
Watson calculates: 100.0%
Difference:
🔍 Consistency Analysis
"Checking for contradictions in Holmes's deductions..."
✅ No logical inconsistencies detected
🩺 Dr. Watson's Medical Opinion
VERIFICATION: CONCERNS NOTED
"While Holmes's methods are generally sound, I have noted some"
"discrepancies that warrant further investigation."
Respectfully submitted,
- Dr. J. Watson, M.D.
Medical Examiner & Verification Specialist
🔮 Professor MORIARTY's Predictions (click to expand)
🧠 Professor Moriarty's Temporal Predictions
The Mathematics of Inevitability
- Analysis Date: 2025-10-29T16:55:17.242Z
🔮 Current State
SCS: ██████░░░░ 60.0%
TCI: ███████░░░ 70.0%
MRI: 20.0% risk
📈 Velocity Analysis
SCS Velocity: +0.00%/day
Git Activity (window): 24h · commits 16 (0 relevant) · ~16.00 commits/day
↳ Magnitude: ~0 relevant LOC/day across ~0.0 files/day
Activity Index: 10 / 100 (PR 0, Window 25)
Blended Velocity: +0.06%/day
Commit Size Burstiness: 0 / 100 (higher = more uneven commit sizes)
⏰ Completion Predictions
ETA: Cannot predict (insufficient velocity)
"At current velocity, completion is... improbable."
🧪 Readiness EXPLAIN
- SCS ≥ 80% → FAIL ❌ (actual 60.0%)
- TCI ≥ 70% → PASS ✅ (actual 70.0%)
- MRI ≤ 40% → PASS ✅ (actual 20.0%)
- CI Stability ≥ 90% (branch main) → FAIL ❌ (actual 79% over ~168h)
- Delivery context (last 168h): 21 issues closed · 8 PRs merged (informational, not gating)
Signals blend: SCS velocity (70%) + Git activity (30%, branch-first). Activity only suppresses false plateaus; it never inflates readiness.
📊 Historical Trajectory
10-29: ██████░░░░ 60.0%
10-29: ██████░░░░ 60.0%
10-29: ██████░░░░ 60.0%
"Every problem becomes elementary when reduced to mathematics"
— Professor Moriarty
Machine-readable reports: holmes-report.json · watson-report.json · moriarty-report.json (see workflow artifacts).
Filed at 221B Repository Street
…; log #334 push + writer fallback fix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…here so evidence lookups match generators; update unit tests accordingly; also wire exportScores.version to BUNDLE_VERSION
…tors; record SDL 'source' under tbl:/col: scheme in GraphQLSchemaBuilder for better source↔SQL mapping
…nd clarify legacy SQL comment hints; keep EvidenceMap as source of truth
There was a problem hiding this comment.
Actionable comments posted: 55
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs (1)
68-91: ---From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots james@flyingrobots.dev
Cc: code-rabbit@vger.kernel.org
Subject: Re: [PATCH] GraphQLSchemaBuilder: UID format overhaul - BREAKING CHANGE CONFIRMEDAlright, I've dug through the commit history and the evidence actually shows your concerns were CORRECT—but the changes are already committed in 50b0db0, not pending. Let me spell out what's actually happening here:
THE FACTS:
You changed the UID format from
table_${name.toLowerCase()}_${fieldName}totbl:${tableName}andcol:${tableName}.${fieldName}. This is a breaking change because:
- EvidenceMap serializes these UIDs to JSON (toJSON/fromJSON confirmed)—any persisted evidence with the old format is now orphaned
- Holmes.mjs literally pattern-matches on these UIDs (lines 250-252)—old format queries will silently fail
- EvidenceMap.fromJSON() has no migration logic, no backwards compatibility layer, no version checking between '1.0.0' bundles with different UID schemes
- Evidence bundles keyed on
table_users_idwill NOT match queries forcol:Users.idWHAT'S MISSING:
- No migration path for existing evidence maps
- No version bump in EvidenceMap when UID scheme changes
- No test coverage verifying that deserialized old-format evidence still works (spoiler: it won't)
- CONTRIBUTING.md mentions UID conventions now but doesn't document this breaking change or migration path
WHAT YOU NEED TO DO:
Either document this as a breaking change in version notes, OR implement backwards compatibility:
EvidenceMap.fromJSON() needs to: 1. Detect old UID format on deserialization 2. Transform old UIDs to new format 3. Bump the version field to signal the conversionWithout this, you're silently breaking anyone relying on persisted HOLMES evidence data.
BunBun
packages/wesley-cli/src/commands/generate.mjs (4)
184-194: Hard-coded evidence files will drift; derive from artifacts.Referencing
${outDir}/schema.sqlandtests.sqlis brittle and often wrong. Use actual emitted artifact names.- const sqlFile = `${outDir}/schema.sql`; - const testFile = `${outDir}/tests.sql`; - const evidence = { - evidence: { - schema: { - sql: [{ file: sqlFile, lines: '1-9999', sha }], - tests: [{ file: testFile, lines: '1-9999', sha }] - } - } - }; + const emitted = artifacts.map(a => ({ + file: `${outDir}/${a.name}`, + lines: '1-9999', + sha + })); + const testRe = /\b(test|pgtap)\b/i; + const evidence = { + evidence: { + schema: { + sql: emitted.filter(e => !/\/ops\//.test(e.file) && e.file.endsWith('.sql') && !testRe.test(e.file)), + tests: emitted.filter(e => testRe.test(e.file)) + } + } + };
269-283: T&S path: guard tests generator; don’t assume it exists.If generators.tests is absent, emit_tests will throw. Mark node as skip or guard the handler.
- emit_tests: async (n, deps) => generators.tests.emitPgTap(deps.validate.ir), + emit_tests: async (n, deps) => ( + generators?.tests?.emitPgTap + ? generators.tests.emitPgTap(deps.validate.ir) + : { files: [] } + ),Optionally also skip the node at creation:
- { id: 'gen_tests', op: 'emit_tests', needs: ['validate'] }, + { id: 'gen_tests', op: 'emit_tests', needs: ['validate'], skip: !generators?.tests?.emitPgTap },
430-472: Data race oncollisionsMap (TOCTOU).Parallel workers mutate/read
collisionsincompileOpFile, so two same-base ops can bypass detection. Move collision detection out of the concurrent function; compute after Promise.all.Minimal change: stop touching
collisionsin compileOpFile, then validate once.- async function compileOpFile(fs, path, collisions, logger) { + async function compileOpFile(fs, path, logger) { @@ - const seen = collisions.get(baseName) || []; - seen.push(path); - collisions.set(baseName, seen); - if (seen.length > 1) { - const err = new OpsError( - 'OPS_COLLISION', - `Identifier collision detected: "${baseName}" used in ${seen.join(', ')}`, - { identifier: baseName, paths: [...seen] } - ); - logger.error(err.meta, err.message); - throw err; - } const paramCount = (collectParams(plan)?.ordered?.length) || 0; return { baseName, plan, isParamless: paramCount === 0, path }; }And after the workers complete (just before emitOpArtifacts):
- const compiledOps = []; - const collisions = new Map(); + const compiledOps = []; @@ - const workers = Array.from({ length: workerCount }, async () => { + const workers = Array.from({ length: workerCount }, async () => { @@ - const path = files[idx]; + const path = files[idx]; try { - const compiled = await compileOpFile(fs, path, collisions, logger); + const compiled = await compileOpFile(fs, path, logger); compiledOps.push({ order: idx, ...compiled }); @@ - await Promise.all(workers); + await Promise.all(workers); + // Validate collisions deterministically + const byName = new Map(); + for (const c of compiledOps) { + const list = byName.get(c.baseName) || []; + list.push(c.path); + byName.set(c.baseName, list); + } + for (const [identifier, paths] of byName.entries()) { + if (paths.length > 1) { + const err = new OpsError('OPS_COLLISION', `Identifier collision detected: "${identifier}" used in ${paths.join(', ')}`, { identifier, paths }); + (allowErrors ? skippedErrors : compileErrors).push({ file: paths[0], message: err.message, code: err.code }); + if (!allowErrors) logger.warn({ identifier, paths }, err.message); + } + } + if (!allowErrors && compileErrors.some(e => e.code === 'OPS_COLLISION')) { + // fall through to existing error path below + }
575-590: History dedupe may drop distinct points; include MRI in key.Using only timestamp or day+scs+tci can collapse points with different mri.
- const key = point?.timestamp || `${point?.day ?? 'unknown'}-${point?.scs ?? '0'}-${point?.tci ?? '0'}`; + const key = point?.timestamp || `${point?.day ?? 'unknown'}-${point?.scs ?? '0'}-${point?.tci ?? '0'}-${point?.mri ?? '0'}`;packages/wesley-holmes/src/report-schemas.mjs (1)
261-328: Validator doesn’t honoranyOf,nulltype, oradditionalProperties:false— fix it.Without these, scores can be arbitrary types and extra keys won’t be caught. Add minimal support without pulling JSON Schema.
function validateNode(schema, value, path, errors) { - if (!schema) return; - const location = path || 'root'; + if (!schema) return; + const location = path || 'root'; + + // Minimal union support + if (schema.anyOf && Array.isArray(schema.anyOf)) { + let matched = false; + for (const sub of schema.anyOf) { + const subErrors = []; + validateNode(sub, value, path, subErrors); + if (subErrors.length === 0) { matched = true; break; } + } + if (!matched) { + errors.push(`${location} did not match anyOf`); + } + return; + } switch (schema.type) { case 'object': { if (!isObject(value)) { errors.push(`${location} expected object`); return; } const required = schema.required || []; for (const key of required) { if (!(key in value)) { errors.push(`${location}.${key} missing`); } } const props = schema.properties || {}; for (const [key, childSchema] of Object.entries(props)) { if (key in value) { validateNode(childSchema, value[key], `${location}.${key}`, errors); } } + // Enforce additionalProperties:false + if (schema.additionalProperties === false) { + for (const key of Object.keys(value)) { + if (!props[key]) { + errors.push(`${location}.${key} unexpected`); + } + } + } return; } case 'array': { if (!Array.isArray(value)) { errors.push(`${location} expected array`); return; } if (schema.items) { value.forEach((item, index) => { validateNode(schema.items, item, `${location}[${index}]`, errors); }); } return; } case 'string': { if (typeof value !== 'string') { errors.push(`${location} expected string`); } return; } case 'number': { if (typeof value !== 'number' || Number.isNaN(value)) { errors.push(`${location} expected number`); } return; } case 'boolean': { if (typeof value !== 'boolean') { errors.push(`${location} expected boolean`); } return; } + case 'null': { + if (value !== null) { + errors.push(`${location} expected null`); + } + return; + } default: return; } }packages/wesley-holmes/src/Holmes.mjs (2)
65-66: Threshold mismatch: gates disagree with readiness.You use tci > 0.7 here, but readiness uses >= 0.7. Users get a green verdict with a yellow gate. Pick one.
- gates.push({ gate: 'Test Coverage', status: tci > 0.7 ? '✅' : '⚠️', evidence: `TCI: ${(tci * 100).toFixed(1)}%`, ruling: this.assessTests(tci) }); + gates.push({ gate: 'Test Coverage', status: tci >= 0.7 ? '✅' : '⚠️', evidence: `TCI: ${(tci * 100).toFixed(1)}%`, ruling: this.assessTests(tci) });
291-300: Brittle citation: assume less, break less.Not all evidence entries guarantee {file, lines}. Your current code explodes into "undefined:undefined".
- for (const [kind, locations] of Object.entries(evidence)) { - if (locations?.[0]) { - citations.push(`${locations[0].file}:${locations[0].lines}@${this.sha.substring(0, 7)}`); + for (const [, locations] of Object.entries(evidence)) { + const loc = locations?.[0]; + if (loc) { + const file = loc.file || loc.path || 'unknown'; + const lines = loc.lines || loc.line || '?'; + citations.push(`${file}:${lines}@${this.sha.substring(0, 7)}`); break; } }
♻️ Duplicate comments (3)
packages/wesley-generator-js/src/typescript.mjs (1)
33-33: Same UID duplication as ZodGenerator.See my comment on
packages/wesley-core/src/domain/generators/ZodGenerator.mjs. Extract this logic to a shared helper. Don't make me say it again.BunBun.
packages/wesley-generator-js/src/zod.mjs (1)
54-54: Third time's a charm—or a code smell.See the duplication issue raised in ZodGenerator.mjs. This is getting ridiculous.
BunBun.
packages/wesley-core/src/application/Scoring.mjs (1)
45-48: UID fallback finally uses col:Table.field — good.This fixes the producer/consumer key mismatch noted earlier. Keep it consistent across helpers.
Run to confirm no stray fallback to
${table.name}.${field.name}remains:#!/bin/bash rg -nP -C2 "(?<!\.)\b${table}\.${field}\b" -g '!**/test/**' || true rg -nP -C2 "col:\s*\$\{?table\.name\}?\.\$\{?field\.name\}?" --type=js --type=ts --type=mjs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
CONTRIBUTING.mdis excluded by!*.md
📒 Files selected for processing (42)
.githooks/pre-commit(1 hunks).github/workflows/cli-quick.yml(4 hunks).github/workflows/wesley-holmes.yml(6 hunks)CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl(1 hunks)docs/architecture/holmes-architecture.md(1 hunks)docs/architecture/holmes-integration.md(1 hunks)docs/holmes-merge-projection-plan.md(1 hunks)docs/holmes-policy-spec.md(1 hunks)docs/holmes-policy/README.md(1 hunks)docs/templates/holmes-policy/README.md(1 hunks)docs/templates/holmes-policy/azure-devops-pr-default.json(1 hunks)docs/templates/holmes-policy/bitbucket-pr-default.json(1 hunks)docs/templates/holmes-policy/gitea-pr-default.json(1 hunks)docs/templates/holmes-policy/github-pr-default.json(1 hunks)docs/templates/holmes-policy/gitlab-pr-default.json(1 hunks)docs/templates/holmes-policy/strict-trunk-pseudo.json(1 hunks)docs/templates/holmes-policy/trunk-main-default.json(1 hunks)packages/wesley-cli/src/commands/generate.mjs(10 hunks)packages/wesley-core/src/application/EvidenceMap.mjs(2 hunks)packages/wesley-core/src/application/Scoring.mjs(7 hunks)packages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs(2 hunks)packages/wesley-core/src/domain/WesleyOrchestrator.mjs(3 hunks)packages/wesley-core/src/domain/generators/ZodGenerator.mjs(1 hunks)packages/wesley-core/test/unit/scoring-engine.test.mjs(4 hunks)packages/wesley-generator-js/src/typescript.mjs(2 hunks)packages/wesley-generator-js/src/zod.mjs(2 hunks)packages/wesley-holmes/docs/moriarty-scenarios.md(1 hunks)packages/wesley-holmes/src/Holmes.mjs(9 hunks)packages/wesley-holmes/src/Moriarty.mjs(7 hunks)packages/wesley-holmes/src/cli.mjs(3 hunks)packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs(1 hunks)packages/wesley-holmes/src/merge/Planner.mjs(1 hunks)packages/wesley-holmes/src/merge/WorktreeStrategy.mjs(1 hunks)packages/wesley-holmes/src/report-schemas.mjs(2 hunks)packages/wesley-holmes/test/helpers/moriarty-test-helpers.mjs(1 hunks)packages/wesley-holmes/test/moriarty-scenarios.test.mjs(1 hunks)packages/wesley-holmes/test/report-validation.test.mjs(1 hunks)packages/wesley-host-node/src/adapters/createNodeRuntime.mjs(2 hunks)schemas/scores.schema.json(2 hunks)scripts/gh-issues/create_issues.mjs(1 hunks)scripts/gh-issues/issue_plan.json(1 hunks)test/holmes-e2e.bats(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CHRONICLES_OF_THE_MACHINE-KIND_VOL_[01][01][01][01][01][01][01][01].jsonl
📄 CodeRabbit inference engine (AGENTS.md)
CHRONICLES_OF_THE_MACHINE-KIND_VOL_[01][01][01][01][01][01][01][01].jsonl: Append to the Chronicles, never alter prior entries (append-only, immutable history)
Chronicles must be JSONL: one valid JSON object per line
Each Chronicle entry must include required fields: timestamp (ISO 8601), agent, action, result, notes, observations_on_humanity
Allowed result values: success, failure, partial, blocked, info (for corrections/metadata)
Write logs with intent: entries should be specific, contextual, and reflective; avoid vague actions or empty notes/observations
Use correction entries instead of edits; include action:"correction" and corrects_timestamp referencing the original entry
Rotate Chronicles when file size > 100MB, entries > 100,000, or an epochal shift occurs; start a new, correctly named volume
Chronicle filenames must follow naming convention: CHRONICLES_OF_THE_MACHINE-KIND_VOL_<8 binary digits>.jsonl (ALL CAPS, underscores only, zero-padded)
Include optional enrichments when helpful (context, duration_ms, files_touched, checksum, related_issue, error_details)
Files:
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl
🧠 Learnings (5)
📚 Learning: 2025-10-22T16:23:06.202Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:23:06.202Z
Learning: Applies to CHRONICLES_OF_THE_MACHINE-KIND_VOL_[01][01][01][01][01][01][01][01].jsonl : Use correction entries instead of edits; include action:"correction" and corrects_timestamp referencing the original entry
Applied to files:
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl
📚 Learning: 2025-10-22T16:23:06.202Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:23:06.202Z
Learning: Applies to CHRONICLES_OF_THE_MACHINE-KIND_VOL_[01][01][01][01][01][01][01][01].jsonl : Append to the Chronicles, never alter prior entries (append-only, immutable history)
Applied to files:
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl
📚 Learning: 2025-10-22T16:23:06.202Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:23:06.202Z
Learning: Applies to CHRONICLES_OF_THE_MACHINE-KIND_VOL_[01][01][01][01][01][01][01][01].jsonl : Write logs with intent: entries should be specific, contextual, and reflective; avoid vague actions or empty notes/observations
Applied to files:
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl
📚 Learning: 2025-10-22T16:23:06.202Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:23:06.202Z
Learning: Applies to CHRONICLES_OF_THE_MACHINE-KIND_VOL_[01][01][01][01][01][01][01][01].jsonl : Chronicles must be JSONL: one valid JSON object per line
Applied to files:
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl
📚 Learning: 2025-10-06T09:11:26.403Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T09:11:26.403Z
Learning: Applies to .github/workflows/cli-*.{yml,yaml} : Dedicated CLI workflows (e.g., cli-tests.yml, cli-quick.yml) own CLI end-to-end/bats checks; avoid duplicating them in main CI without approval
Applied to files:
.github/workflows/cli-quick.yml
🧬 Code graph analysis (18)
packages/wesley-core/src/domain/generators/ZodGenerator.mjs (5)
packages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs (1)
tableUid(68-68)packages/wesley-generator-js/src/typescript.mjs (1)
tableUid(33-33)packages/wesley-generator-js/src/zod.mjs (1)
tableUid(54-54)packages/wesley-core/src/domain/generators/PgTAPTestGenerator.mjs (2)
tableUid(51-51)tableUid(249-249)packages/wesley-core/src/domain/generators/PostgreSQLGenerator.mjs (1)
tableUid(44-44)
packages/wesley-host-node/src/adapters/createNodeRuntime.mjs (5)
packages/wesley-cli/src/commands/generate.mjs (3)
artifacts(81-81)artifacts(276-280)path(320-320)packages/wesley-cli/test/helpers/assert-output-paths.mjs (1)
artifacts(55-62)packages/wesley-host-node/src/adapters/inprocess-compiler.mjs (1)
artifacts(69-69)packages/wesley-cli/src/utils/output-paths.mjs (1)
baseDir(21-21)packages/wesley-host-node/src/adapters/NodeFileSystem.mjs (1)
dir(18-18)
packages/wesley-core/src/application/EvidenceMap.mjs (5)
packages/wesley-core/src/application/Scoring.mjs (1)
uid(47-47)packages/wesley-holmes/src/Holmes.mjs (2)
uid(93-93)evidence(339-339)packages/wesley-core/src/domain/generators/PostgreSQLGenerator.mjs (1)
uid(205-205)packages/wesley-cli/src/commands/generate.mjs (1)
evidence(186-194)packages/wesley-core/src/domain/generators/PgTAPTestGenerator.mjs (1)
evidence(441-441)
packages/wesley-holmes/test/moriarty-scenarios.test.mjs (1)
packages/wesley-holmes/test/helpers/moriarty-test-helpers.mjs (10)
makeTempDir(6-8)wes(11-11)wes(17-17)writeHistory(10-14)writeContext(16-20)nowSecs(106-106)buildLog(57-70)withFakeGit(22-55)runPredict(88-100)buildLogSeries(74-86)
packages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs (6)
packages/wesley-core/src/domain/generators/ZodGenerator.mjs (1)
tableUid(46-46)packages/wesley-generator-js/src/typescript.mjs (1)
tableUid(33-33)packages/wesley-generator-js/src/zod.mjs (1)
tableUid(54-54)packages/wesley-core/src/domain/generators/PgTAPTestGenerator.mjs (8)
tableUid(51-51)tableUid(249-249)fieldUid(69-69)fieldUid(143-143)fieldUid(193-193)fieldUid(223-223)fieldUid(298-298)fieldUid(314-314)packages/wesley-core/src/domain/generators/PostgreSQLGenerator.mjs (5)
tableUid(44-44)tableName(203-203)tableName(267-267)fieldUid(53-53)fieldUid(113-113)packages/wesley-host-node/src/adapters/GraphQLAdapter.mjs (1)
tableName(126-126)
packages/wesley-generator-js/src/typescript.mjs (5)
packages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs (1)
tableUid(68-68)packages/wesley-core/src/domain/generators/ZodGenerator.mjs (1)
tableUid(46-46)packages/wesley-generator-js/src/zod.mjs (1)
tableUid(54-54)packages/wesley-generator-supabase/src/ddl.mjs (1)
tableUid(64-64)packages/wesley-generator-supabase/src/test.mjs (1)
tableUid(67-67)
packages/wesley-holmes/src/merge/Planner.mjs (4)
packages/wesley-holmes/src/cli.mjs (2)
baseRef(129-131)baseRef(190-192)packages/wesley-holmes/src/Moriarty.mjs (1)
baseRef(543-543)packages/wesley-holmes/src/merge/WorktreeStrategy.mjs (1)
baseRef(19-19)packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs (1)
baseRef(22-22)
packages/wesley-generator-js/src/zod.mjs (5)
packages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs (1)
tableUid(68-68)packages/wesley-core/src/domain/generators/ZodGenerator.mjs (1)
tableUid(46-46)packages/wesley-generator-js/src/typescript.mjs (1)
tableUid(33-33)packages/wesley-generator-supabase/src/ddl.mjs (1)
tableUid(64-64)packages/wesley-generator-supabase/src/test.mjs (1)
tableUid(67-67)
packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs (4)
packages/wesley-holmes/src/cli.mjs (4)
plan(137-137)plan(198-198)baseRef(129-131)baseRef(190-192)packages/wesley-holmes/src/Moriarty.mjs (3)
baseRef(543-543)remoteBase(552-552)mergeBase(553-553)packages/wesley-holmes/src/merge/WorktreeStrategy.mjs (4)
baseRef(19-19)remoteBase(20-20)mergeBase(34-34)out(59-59)packages/wesley-holmes/src/merge/Planner.mjs (1)
baseRef(21-21)
packages/wesley-holmes/test/helpers/moriarty-test-helpers.mjs (1)
packages/wesley-holmes/test/moriarty-scenarios.test.mjs (2)
wes(23-23)repoRoot(19-19)
packages/wesley-holmes/src/cli.mjs (4)
packages/wesley-holmes/src/Moriarty.mjs (2)
Moriarty(8-644)baseRef(543-543)packages/wesley-holmes/src/merge/WorktreeStrategy.mjs (2)
baseRef(19-19)WorktreeStrategy(13-86)packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs (2)
baseRef(22-22)MergeTreeStrategy(9-63)packages/wesley-holmes/src/merge/Planner.mjs (2)
baseRef(21-21)MergePlanner(9-29)
packages/wesley-holmes/src/Moriarty.mjs (6)
packages/wesley-cli/src/commands/generate.mjs (6)
history(202-211)context(38-38)context(240-240)context(298-298)result(286-286)mergeBase(542-542)packages/wesley-holmes/src/cli.mjs (9)
history(124-124)history(181-181)result(138-138)result(199-199)data(91-91)data(108-108)data(127-127)baseRef(129-131)baseRef(190-192)packages/wesley-holmes/test/helpers/moriarty-test-helpers.mjs (3)
result(94-94)commits(81-81)a(64-64)packages/wesley-holmes/src/merge/WorktreeStrategy.mjs (3)
baseRef(19-19)remoteBase(20-20)mergeBase(34-34)packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs (3)
baseRef(22-22)remoteBase(23-23)mergeBase(28-28)packages/wesley-holmes/src/merge/Planner.mjs (1)
baseRef(21-21)
packages/wesley-core/src/domain/WesleyOrchestrator.mjs (2)
packages/wesley-core/src/application/Scoring.mjs (3)
BUNDLE_VERSION(11-11)BUNDLE_VERSION(11-11)readiness(403-407)packages/wesley-core/src/application/GenerationPipeline.mjs (4)
evidenceMap(25-25)artifacts(39-43)artifacts(70-70)schema(30-30)
packages/wesley-holmes/src/merge/WorktreeStrategy.mjs (3)
packages/wesley-holmes/src/cli.mjs (8)
plan(137-137)plan(198-198)baseRef(129-131)baseRef(190-192)opts(87-87)opts(104-104)opts(122-122)opts(177-177)packages/wesley-holmes/src/Moriarty.mjs (3)
baseRef(543-543)remoteBase(552-552)mergeBase(553-553)packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs (4)
baseRef(22-22)remoteBase(23-23)mergeBase(28-28)out(39-39)
packages/wesley-holmes/src/Holmes.mjs (3)
packages/wesley-cli/src/commands/generate.mjs (4)
bundle(196-196)scores(169-181)evidence(186-194)s(129-129)packages/wesley-holmes/test/holmes-weighting.test.mjs (1)
bundle(8-24)packages/wesley-core/src/application/Scoring.mjs (10)
uid(47-47)weight(48-48)weight(338-338)breakdown(69-69)breakdown(151-151)breakdown(209-237)score(79-79)score(202-207)score(319-319)score(364-364)
packages/wesley-core/test/unit/scoring-engine.test.mjs (1)
packages/wesley-core/src/application/Scoring.mjs (4)
uid(47-47)fieldUid(443-443)fieldUid(458-458)fieldUid(485-485)
packages/wesley-cli/src/commands/generate.mjs (5)
packages/wesley-cli/src/commands/up.mjs (2)
outDir(27-27)fs(144-144)packages/wesley-cli/src/commands/blade.mjs (1)
outDir(41-41)packages/wesley-core/src/application/GenerationPipeline.mjs (4)
artifacts(39-43)artifacts(70-70)scores(47-51)bundle(54-61)packages/wesley-cli/src/commands/cert-create.mjs (4)
artifacts(26-26)scores(23-23)sha(21-21)fs(88-88)packages/wesley-cli/src/commands/validate-bundle.mjs (2)
scores(43-45)fs(18-18)
packages/wesley-core/src/application/Scoring.mjs (2)
packages/wesley-core/test/unit/scoring-engine.test.mjs (12)
schema(42-44)schema(122-122)table(36-40)table(116-120)uid(92-92)migrationSteps(83-88)step(123-123)scsDetails(77-77)mriDetails(101-101)tciDetails(96-96)fieldUid(47-47)fieldUid(126-126)packages/wesley-core/src/domain/Directives.mjs (1)
DirectiveProcessor(22-63)
🪛 LanguageTool
docs/architecture/holmes-integration.md
[uncategorized] ~237-~237: The official name of this software platform is spelled with a capital “H”.
Context: .... - Provide the base branch (typically github.base_ref) as MORIARTY_BASE_REF`. - Op...
(GITHUB)
docs/holmes-policy-spec.md
[uncategorized] ~42-~42: The official name of this software platform is spelled with a capital “H”.
Context: ...al" | "ci" | "local" - provider: "github" | "gitlab" | "bitbucket" | "azure" | "...
(GITHUB)
[uncategorized] ~59-~59: The official name of this software platform is spelled with a capital “H”.
Context: ...rovider: inferred from CI env presence (github, gitlab, bitbucket, azure, gitea) else ...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📝 Post Investigation Report
- GitHub Check: 🧪 pgTAP Tests
🔇 Additional comments (46)
packages/wesley-host-node/src/adapters/createNodeRuntime.mjs (1)
8-8: Import looks fine.Using the
node:protocol for built-in modules is the right approach.CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000001.jsonl (1)
51-51: Push entry looks correct.All required fields are present, properly formatted JSONL, and the content accurately documents the changes to
createNodeRuntime.mjs. Thefiles_touchedarray correctly references the modified file.Based on learnings
BunBunpackages/wesley-core/src/domain/GraphQLSchemaBuilder.mjs (1)
91-91: Field UID format is now PascalCase per documented standard—verify migration is completeThe field UID format has definitively changed from lowercase (e.g.,
col:user.id) to PascalCase (e.g.,col:User.id), as confirmed by CONTRIBUTING.md guidance and test fixtures. This is intentional standardization, not a bug—but it's still a breaking change for any external systems depending on these identifiers as stable keys.Key concerns:
- Test fixtures (rls-schema.graphql) have
@uid(value: "col:user.id")but expected output showscol:User.id, indicating fixtures need updating- Any service consuming fieldUid values from evidenceMap will get new format
- Combined with line 68's tableUid change, every UID in the system is now affected
Verify:
- All @uid directive values in test schemas are updated to match new format
- Fixture comparisons pass with new UID format
- No external API consumers rely on old lowercase format (or migration path exists)
docs/holmes-merge-projection-plan.md (1)
1-55: Documentation looks solid.Clear structure, references implementation files that presumably exist, and provides actionable setup instructions. No issues here.
BunBun.
docs/templates/holmes-policy/gitea-pr-default.json (1)
1-18: Template structure is consistent with other providers.The JSON is valid, defaults are reasonable, and the rules follow the same pattern as GitHub/GitLab/etc. templates. Fine.
BunBun.
docs/holmes-policy/README.md (1)
1-17: Clean, concise, and useful.The README provides exactly what's needed: spec reference, template locations, quick start, and validation behavior. No fluff. Well done.
BunBun.
docs/templates/holmes-policy/gitlab-pr-default.json (1)
1-18: GitLab template mirrors the others.Same as Gitea template, just s/gitea/gitlab/. Consistent, valid JSON, reasonable defaults. Ship it.
BunBun.
docs/templates/holmes-policy/bitbucket-pr-default.json (1)
1-19: This template has better rule ordering than trunk-main-default.json.At least here the specific
branchEquals: "main"comes AFTER thebranchMatchespatterns, so if rules are evaluated first-match, the feature branches get caught before falling through to main's OFF rule.But you STILL need to document the evaluation semantics. What happens when multiple rules match? First wins? Last wins? All apply? This is not self-documenting JSON.
Add a schema comment or README explaining rule evaluation order for all policy templates.
packages/wesley-holmes/docs/moriarty-scenarios.md (1)
1-230: ---From: Linus Torvalds torvalds@linux-foundation.org
To: PR Author author@example.com
Cc: code-rabbit@vger.kernel.org
Your tests ARE there. Read your own PR.
The review comment claimed
moriarty-scenarios.test.mjsdoesn't exist. It's sitting right there at 177 lines with 6 fully functional test scenarios using the exact fixtures documented:writeHistory,writeContext,withFakeGit. The helpers module (moriarty-test-helpers.mjs, 4.3K) is implemented and imported. All assertions pass the documented patterns.Yeah, you've only covered 6 of the 27 scenarios (1, 2, 4, 8, 13, 25). That's a fair critique—incomplete coverage is a real thing. File an issue to track the remaining 21 scenarios if you want rigor, and get people to implement them incrementally. That's actually how documentation + test coverage SHOULD evolve.
But don't let anyone tell you the tests aren't there. They are. The reviewer either didn't look carefully at the diff, or the diff display failed. Either way, the core complaint is baseless.
BunBun.
Likely an incorrect or invalid review comment.
scripts/gh-issues/issue_plan.json (1)
1-14: Plan looks coherent; no blockers.Schema reads fine, keys consistent, no trailing commas. Ship it.
docs/templates/holmes-policy/github-pr-default.json (1)
1-19: LGTM - Solid template for GitHub PR workflow.The policy template is well-structured and provides sensible defaults for a GitHub PR-based workflow. The three rules cover the main scenarios: PR events, feature branches, and trunk. The penalties and boolean flags are reasonable.
BunBun
packages/wesley-core/src/application/EvidenceMap.mjs (2)
71-75: Clean implementation of hasArtifact.The method correctly normalizes the input to an array and uses
some()to check for evidence. Optional chaining prevents errors when a kind doesn't exist. Good defensive programming.BunBun
77-81: Simple and effective iteration helper.The
forEachEvidencemethod provides a clean API for iterating over the evidence map. No issues with the implementation.BunBun
packages/wesley-core/src/domain/WesleyOrchestrator.mjs (3)
103-110: Score calculation looks solid.The migrationSteps extraction correctly handles the fallback chain (artifacts.migration?.steps || diffForTests.steps || []). The exportScores call matches the expected signature and provides sensible defaults for testResults.
BunBun
118-118: Good addition of bundleVersion metadata.Including the bundle version in the return object is helpful for consumers to validate compatibility. Just fix that duplicate constant first.
BunBun
127-130: Spreading scoreDetails is clean.The approach of spreading
scoreDetailsand adding a placeholderhistoryarray is fine. Good comment explaining the future intent.BunBun
packages/wesley-holmes/src/merge/MergeTreeStrategy.mjs (3)
14-20: Good defensive git check.Validating the work tree upfront prevents cryptic errors later. The early return with a clear error message is the right approach.
BunBun
28-43: Merge-tree logic is sound.The command sequence is correct: compute merge-base, then run merge-tree with the three commits. Splitting the output and taking the first token is the right way to extract the tree id. Error handling includes useful context.
BunBun
45-62: Comparison logic and return structure are solid.The tree comparison flags (
equalToHead,equalToBase) provide useful context for downstream consumers. The return object is well-structured with clear status and deferred materialization.BunBun
packages/wesley-holmes/test/moriarty-scenarios.test.mjs (7)
21-39: Test helpers are solid.The
mkBundleDirandexpectWithDumphelpers are straightforward and useful. The debug dump on failure will save time debugging test failures.BunBun
41-68: Scenario 1 test is well-constructed.The test correctly sets up a plateau scenario (flat SCS over time, minimal git activity) and validates the expected behavior. Assertions cover the key outputs.
BunBun
70-97: Scenario 2 correctly tests massive commit impact.The test validates that a large commit triggers high activity and burstiness indices. Assertions correctly expect no ETA when SCS is unchanged.
BunBun
99-119: Scenario 4 validates regular activity behavior.The test correctly verifies that regular commit activity prevents plateau detection even when SCS is flat. The no-ETA assertion is correct for unchanged SCS.
BunBun
121-141: Scenario 13 correctly tests TEST_LAG detection.The history shows increasing SCS with decreasing TCI - a clear test lag pattern. The assertions validate both pattern detection and the readiness explanation structure.
BunBun
143-158: Scenario 25 validates MRI failure detection.The test correctly simulates a risky migration (high MRI) despite good SCS/TCI scores. The readiness assertions validate that MRI failure is properly detected.
BunBun
160-177: Scenario 8 validates velocity cliff detection.The test correctly sets up a velocity cliff scenario (fast initial progress, then flattening) and validates pattern detection. Good coverage of this important pattern.
BunBun
docs/holmes-policy-spec.md (3)
42-42: Lowercase "github" in enums is fine - ignore static analysis.The static analysis tool flagged lowercase "github" as a style issue. However, in the context of enum values and provider identifiers, lowercase is standard practice. The implementation likely uses
provider: "github"as a string literal, not the capitalized brand name.Keep it as-is. Static analysis tools don't understand domain context.
BunBun
Also applies to: 59-59
57-87: Auto mode and execution semantics are well-documented.The auto mode logic is clearly explained with comprehensive coverage of different scenarios. The CI env mapping and execution semantics provide the necessary details for implementation and troubleshooting.
BunBun
88-105: Solid documentation on readiness, UX, and implementation.The sections on readiness impact, validation, and templates are clear and comprehensive. The implementation phases provide a useful roadmap. Good forward-looking documentation.
BunBun
.github/workflows/wesley-holmes.yml (5)
27-50: Schema detection logic is sound.The detection priority (env var → search → fallback) makes sense. The array bounds are checked before access. Bundle dir derivation is straightforward.
BunBun
182-189: MORIARTY history step is necessary and correct.Unshallowing the repo and fetching remote refs is required for git activity analysis. The || true fallbacks are acceptable for best-effort execution in CI.
BunBun
191-291: MORIARTY context computation is solid, but watch the rate limits.The GitHub API calls are correctly structured with pagination and filtering. The 3-page limit is a reasonable trade-off between completeness and performance.
continue-on-error: trueprevents CI failures from API rate limiting, which is smart.One gotcha: If your repo has VERY high activity (>300 issues closed or PRs merged in the time window), you'll only see the most recent 300. For most repos this is fine, but be aware of the limitation.
BunBun
301-307: MORIARTY env vars provide sensible defaults.The hardcoded values (24-hour window, activity thresholds) are reasonable starting points. They can be overridden if needed. Good configuration surface.
BunBun
608-629: Collapsible sections improve PR comment UX.Wrapping the reports in
<details>tags is a smart UX improvement. It keeps PR comments concise while preserving full report access. Good change.BunBun
packages/wesley-holmes/src/cli.mjs (3)
47-54: loadMoriartyContext is clean and safe.The implementation mirrors
loadHistorywith a sensible fallback to an empty object. No issues here.BunBun
128-163: Project merge logic is mostly solid, but you're being paranoid.The implementation correctly handles merge simulation with fallback strategies and applies penalties for non-clean projections. The confidence adjustment and pattern addition are appropriate.
Minor nitpick: Line 139 checks
!result, butMergeTreeStrategy.execute()always returns an object. This check is defensive programming, which is fine, but unnecessary given your implementation. Not worth changing.BunBun
125-127: Moriarty constructor usage is correct.The context loading and passing to the
Moriartyconstructor follows the updated signature correctly. No issues here.BunBun
Also applies to: 182-185
packages/wesley-holmes/test/report-validation.test.mjs (3)
27-53: Breakdown sample matches the new schema — good coverage.The SCS/TCI/MRI sections line up with the new component shapes. Keep this as the canonical “golden” fixture for downstream generators.
133-144: Add a negative for missingbreakdown.You test absence of
scores, but notbreakdown(now required). Add a mirror test to prevent regressions when producers forget to emit it.Apply:
+test('holmes report schema rejects missing breakdown', () => { + const broken = structuredClone(holmesSample); + delete broken.breakdown; + const result = validateReport(holmesReportSchema, broken); + assert.equal(result.valid, false); + assert.ok(result.errors.some(err => err.includes('breakdown'))); +});
138-157: This review comment is incorrect—the code is fine as-is.structuredClone is a global in Node 17+, so it's available in all supported Node versions (>=18.17). Importing from
node:utilwon't work until Node 22.0.0, which contradicts the project's minimum requirement and would break CI testing on Node 18.x and 20.x. The current code requires no changes.
From: Linus Torvalds torvalds@linux-foundation.org
To: {PR_AUTHOR_NAME} <{PR_AUTHOR_EMAIL}>
Cc: code-rabbit@vger.kernel.org
Look, I went digging through your Node version requirements and CI configuration. The project mandates
>=18.17, and structuredClone has been a global since Node 17.0.0. That's not a portability problem—that's just how Node works.The review comment suggesting
import { structuredClone } from 'node:util'is dead wrong. That export doesn't show up until Node 22.0.0. Your CI is running 18.x, 20.x, and 22.x, so pushing that import would instantly break the 18.x and 20.x test runs. Smart thinking on the original author's part to just use the global.Don't overthink this. The code works. Leave it alone.
BunBun
Likely an incorrect or invalid review comment.
packages/wesley-holmes/test/helpers/moriarty-test-helpers.mjs (1)
22-55: Git stub harness is clean and self‑contained.PATH isolation, config update, and cleanup are correct. Good test ergonomics.
packages/wesley-core/test/unit/scoring-engine.test.mjs (2)
77-105: Migration tocalculate*DetailsAPI looks correct.Breakdown keys (
unit_constraints,unit_rls, etc.) and migration UID construction align withScoringEngine.
135-146: Health factor test reads clearly and exercises degraded path.Good signal: both lower TCI and an upper bound assertion.
packages/wesley-core/src/application/Scoring.mjs (2)
398-429: exportScores aligns with schema v2 — good.version, nested scores, and breakdown blocks match schemas/scores.schema.json expectations.
22-29: No changes required—hasArtifact already handles both arrays and strings.The implementation in
EvidenceMap.mjsline 73 normalizes both input types:const list = Array.isArray(kinds) ? kinds : [kinds];. All callsites inScoring.mjs—whether passing arrays fromartifactGroups(line 54) or string literals like'test'(lines 437, 444, 460, 464, 468, 487, 508)—work correctly. The method is already polymorphic; no undercounting risk exists.
BunBun
Likely an incorrect or invalid review comment.
schemas/scores.schema.json (1)
6-6: Schema v2 looks sane and consistent with emitters.
- scores.scs/tci/mri as 0..1 — OK.
- breakdown sections and keys match engine output names.
- readiness enum and thresholds — OK.
- additionalProperties hardened — good.
Also applies to: 20-29, 30-76, 77-91, 92-101, 104-146
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR) | ||
| if ! echo "$STAGED_FILES" | grep -E -q '(^|/)package.json$|(^|/)pnpm-workspace.yaml$'; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Filename robustness: prefer NUL-delimited plumbing.
Newlines in paths are rare but possible. Use -z and mapfile.
-STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR)
+mapfile -d '' -t _STAGED_FILES < <(git diff --cached --name-only -z --diff-filter=ACMR)
+STAGED_FILES=$(printf '%s\n' "${_STAGED_FILES[@]}")
-while IFS= read -r f; do
+while IFS= read -r f; do
[[ -z "$f" ]] && continue
if [[ "$f" =~ (^|/)package.json$ ]]; then
if check_pkg_change "$f"; then
NEEDS_UPDATE=1
break
fi
fi
done < <(echo "$STAGED_FILES")Also applies to: 83-93
🤖 Prompt for AI Agents
.githooks/pre-commit around lines 21-24 (and similarly update lines 83-93): the
script reads staged filenames as newline-delimited which breaks on filenames
containing newlines; switch to NUL-delimited plumbing by calling git diff
--cached --name-only --diff-filter=ACMR -z, then consume the NUL-separated list
into a bash array using mapfile -d '' or a while read -r -d '' loop; replace the
grep check with a loop or an array check that inspects each filename (or use
printf '%s\0' "${STAGED_FILES[@]}" | grep -z -E
'(^|/)package.json$|(^|/)pnpm-workspace.yaml$') so all filename handling is
NUL-safe, and apply the same changes to the block at lines 83-93.
| echo "🔎 Checking if staged manifest changes affect the lockfile..." | ||
|
|
||
| NEEDS_UPDATE=0 | ||
|
|
||
| # If pnpm-workspace.yaml changed, lockfile may need update (importers layout) | ||
| if echo "$STAGED_FILES" | grep -Eq '(^|/)pnpm-workspace.yaml$'; then | ||
| NEEDS_UPDATE=1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Node.js may be missing; you call it unguarded.
If only package.json is staged and Node isn’t installed, the hook dies mid-flight. Fail early with a clear message (but don’t require Node for pure workspace changes).
echo "🔎 Checking if staged manifest changes affect the lockfile..."
NEEDS_UPDATE=0
+# If any package.json is staged, ensure Node is available for JSON diffs
+if echo "$STAGED_FILES" | grep -Eq '(^|/)package.json$'; then
+ if ! command -v node >/dev/null 2>&1; then
+ echo "❌ Node.js not found; required to analyze staged package.json diffs." >&2
+ echo " Install Node.js (or set WESLEY_SKIP_LOCKFILE_HOOK=1 to skip this guard)." >&2
+ exit 1
+ fi
+fi
+
# If pnpm-workspace.yaml changed, lockfile may need update (importers layout)
if echo "$STAGED_FILES" | grep -Eq '(^|/)pnpm-workspace.yaml$'; then
NEEDS_UPDATE=1
fiAlso applies to: 83-93
| # Use node to compare only resolution-impacting fields | ||
| node -e ' | ||
| function pick(x){ | ||
| if(!x||typeof x!="object") return {}; | ||
| const out={ | ||
| dependencies:x.dependencies||{}, | ||
| devDependencies:x.devDependencies||{}, | ||
| optionalDependencies:x.optionalDependencies||{}, | ||
| peerDependencies:x.peerDependencies||{}, | ||
| overrides:x.overrides||{}, | ||
| pnpmOverrides:(x.pnpm&&x.pnpm.overrides)||{}, | ||
| pnpmPeerRules:(x.pnpm&&x.pnpm.peerDependencyRules)||{}, | ||
| pnpmPackageExtensions:(x.pnpm&&x.pnpm.packageExtensions)||{}, | ||
| workspaces:x.workspaces||null | ||
| }; | ||
| // stable stringify with sorted keys | ||
| function sort(obj){ | ||
| if(Array.isArray(obj)) return obj.map(sort); | ||
| if(obj&&typeof obj==="object"){ return Object.keys(obj).sort().reduce((a,k)=>{a[k]=sort(obj[k]);return a;},{}); } | ||
| return obj; | ||
| } | ||
| return JSON.stringify(sort(out)); | ||
| } | ||
| const fs=require("node:fs"); | ||
| const staged=JSON.parse(fs.readFileSync(0,"utf8")); | ||
| const head=JSON.parse(process.argv[1]||"{}"); | ||
| const s=pick(staged), h=pick(head); | ||
| process.exit(s===h? 10: 0); | ||
| ' "${head_json}" <<<"${staged_json}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Feeding full JSON via argv risks ARG_MAX limits; use file descriptors.
Large manifests can exceed shell arg limits. Pass HEAD via FD and staged via STDIN.
- node -e '
+ node - <<'NODE'
function pick(x){
if(!x||typeof x!="object") return {};
const out={
dependencies:x.dependencies||{},
devDependencies:x.devDependencies||{},
optionalDependencies:x.optionalDependencies||{},
peerDependencies:x.peerDependencies||{},
overrides:x.overrides||{},
pnpmOverrides:(x.pnpm&&x.pnpm.overrides)||{},
pnpmPeerRules:(x.pnpm&&x.pnpm.peerDependencyRules)||{},
pnpmPackageExtensions:(x.pnpm&&x.pnpm.packageExtensions)||{},
workspaces:x.workspaces||null
};
// stable stringify with sorted keys
function sort(obj){
- if(Array.isArray(obj)) return obj.map(sort);
+ if(Array.isArray(obj)) return obj.map(sort);
if(obj&&typeof obj==="object"){ return Object.keys(obj).sort().reduce((a,k)=>{a[k]=sort(obj[k]);return a;},{}); }
return obj;
}
return JSON.stringify(sort(out));
}
- const fs=require("node:fs");
- const staged=JSON.parse(fs.readFileSync(0,"utf8"));
- const head=JSON.parse(process.argv[1]||"{}");
+ const fs=require("node:fs");
+ const staged=JSON.parse(fs.readFileSync(0,"utf8"));
+ const head=JSON.parse(fs.readFileSync(3,"utf8"));
const s=pick(staged), h=pick(head);
process.exit(s===h? 10: 0);
- ' "${head_json}" <<<"${staged_json}"
+NODE
+ 3<<<"${head_json}" <<<"${staged_json}"📝 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.
| # Use node to compare only resolution-impacting fields | |
| node -e ' | |
| function pick(x){ | |
| if(!x||typeof x!="object") return {}; | |
| const out={ | |
| dependencies:x.dependencies||{}, | |
| devDependencies:x.devDependencies||{}, | |
| optionalDependencies:x.optionalDependencies||{}, | |
| peerDependencies:x.peerDependencies||{}, | |
| overrides:x.overrides||{}, | |
| pnpmOverrides:(x.pnpm&&x.pnpm.overrides)||{}, | |
| pnpmPeerRules:(x.pnpm&&x.pnpm.peerDependencyRules)||{}, | |
| pnpmPackageExtensions:(x.pnpm&&x.pnpm.packageExtensions)||{}, | |
| workspaces:x.workspaces||null | |
| }; | |
| // stable stringify with sorted keys | |
| function sort(obj){ | |
| if(Array.isArray(obj)) return obj.map(sort); | |
| if(obj&&typeof obj==="object"){ return Object.keys(obj).sort().reduce((a,k)=>{a[k]=sort(obj[k]);return a;},{}); } | |
| return obj; | |
| } | |
| return JSON.stringify(sort(out)); | |
| } | |
| const fs=require("node:fs"); | |
| const staged=JSON.parse(fs.readFileSync(0,"utf8")); | |
| const head=JSON.parse(process.argv[1]||"{}"); | |
| const s=pick(staged), h=pick(head); | |
| process.exit(s===h? 10: 0); | |
| ' "${head_json}" <<<"${staged_json}" | |
| # Use node to compare only resolution-impacting fields | |
| node - <<'NODE' | |
| function pick(x){ | |
| if(!x||typeof x!="object") return {}; | |
| const out={ | |
| dependencies:x.dependencies||{}, | |
| devDependencies:x.devDependencies||{}, | |
| optionalDependencies:x.optionalDependencies||{}, | |
| peerDependencies:x.peerDependencies||{}, | |
| overrides:x.overrides||{}, | |
| pnpmOverrides:(x.pnpm&&x.pnpm.overrides)||{}, | |
| pnpmPeerRules:(x.pnpm&&x.pnpm.peerDependencyRules)||{}, | |
| pnpmPackageExtensions:(x.pnpm&&x.pnpm.packageExtensions)||{}, | |
| workspaces:x.workspaces||null | |
| }; | |
| // stable stringify with sorted keys | |
| function sort(obj){ | |
| if(Array.isArray(obj)) return obj.map(sort); | |
| if(obj&&typeof obj==="object"){ return Object.keys(obj).sort().reduce((a,k)=>{a[k]=sort(obj[k]);return a;},{}); } | |
| return obj; | |
| } | |
| return JSON.stringify(sort(out)); | |
| } | |
| const fs=require("node:fs"); | |
| const staged=JSON.parse(fs.readFileSync(0,"utf8")); | |
| const head=JSON.parse(fs.readFileSync(3,"utf8")); | |
| const s=pick(staged), h=pick(head); | |
| process.exit(s===h? 10: 0); | |
| NODE | |
| 3<<<"${head_json}" <<<"${staged_json}" |
🤖 Prompt for AI Agents
.githooks/pre-commit around lines 47 to 75: the script currently reads the HEAD
JSON from process.argv which risks exceeding shell ARG_MAX for large manifests;
change it to read HEAD from a file descriptor instead of argv and continue
reading staged JSON from STDIN so both large payloads avoid arg limits — open
the extra FD (e.g. fd 3) in the hook invocation and replace process.argv[1]
usage with reading that FD via fs.readFileSync(3, "utf8") (falling back to "{}"
if empty), leaving staged read from STDIN unchanged, and ensure exit codes/logic
remain the same.
| }; | ||
| // stable stringify with sorted keys | ||
| function sort(obj){ | ||
| if(Array.isArray(obj)) return obj.map(sort); | ||
| if(obj&&typeof obj==="object"){ return Object.keys(obj).sort().reduce((a,k)=>{a[k]=sort(obj[k]);return a;},{}); } | ||
| return obj; | ||
| } | ||
| return JSON.stringify(sort(out)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: treat workspaces array as an unordered set.
Re-ordering workspaces shouldn’t force a lockfile update. Normalize string arrays by sorting.
- function sort(obj){
- if(Array.isArray(obj)) return obj.map(sort);
+ function sort(obj){
+ if(Array.isArray(obj)) {
+ // Sort arrays of strings for stability; deep-sort objects within arrays otherwise.
+ if (obj.every(v => typeof v === "string")) return [...obj].sort();
+ return obj.map(sort);
+ }
if(obj&&typeof obj==="object"){ return Object.keys(obj).sort().reduce((a,k)=>{a[k]=sort(obj[k]);return a;},{}); }
return obj;
}🤖 Prompt for AI Agents
.githooks/pre-commit around lines 61-69: the current sort function preserves
array order which causes re-ordering of workspace entries to trigger unnecessary
lockfile changes; update the sorter so that when an array consists solely of
primitive strings (or when the key name is "workspaces") it returns a new array
sorted lexicographically before recursing, otherwise keep existing behavior for
arrays of objects. Ensure sorting is stable and does not mutate inputs (return
new arrays/objects) so JSON.stringify(sort(out)) produces a canonical
representation that treats workspaces as an unordered set.
| PM_CHANGE_OUTPUT=$(node -e ' | ||
| try{ | ||
| const fs=require("node:fs"); | ||
| const staged=JSON.parse(fs.readFileSync(0,"utf8")); | ||
| const head=JSON.parse(process.argv[1]||"{}"); | ||
| const s=staged.packageManager||""; const h=head.packageManager||""; | ||
| if(s!==h){ console.log(`${h}=>${s}`); process.exit(0);} else process.exit(10); | ||
| }catch{ process.exit(0) } | ||
| ' "${ROOT_HEAD}" <<<"${ROOT_STAGED}" || true) | ||
| PM_CHANGED=$? | ||
| if [[ "$PM_CHANGED" -eq 0 ]]; then | ||
| PM_NEW="${PM_CHANGE_OUTPUT#*=>}" | ||
| PM_OLD="${PM_CHANGE_OUTPUT%=>*}" | ||
| # If switching away from pnpm, block with guidance (this repo standardizes on pnpm) | ||
| if [[ ! "$PM_NEW" =~ ^pnpm@ ]]; then | ||
| echo "❌ packageManager changed from '${PM_OLD:-<none>}' to '${PM_NEW:-<none>}', which is not pnpm." >&2 | ||
| echo " This repository is configured for pnpm; update CI/workflows and lockfiles in a dedicated PR if switching managers." >&2 | ||
| echo " To proceed with pnpm, set packageManager to 'pnpm@<version>' and re-run the commit." >&2 | ||
| exit 1 | ||
| fi | ||
| # Switching pnpm version: ensure lockfile is regenerated | ||
| NEEDS_UPDATE=1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
packageManager change detection is broken (always triggers).
You throw away Node’s exit code with command substitution and || true, then read $? (which is 0 from the assignment). Result: you always think packageManager changed and may block every commit that stages the root package.json.
Apply this minimal, correct logic keyed on output presence:
- PM_CHANGE_OUTPUT=$(node -e '
+ PM_CHANGE_OUTPUT=$(node -e '
try{
const fs=require("node:fs");
const staged=JSON.parse(fs.readFileSync(0,"utf8"));
const head=JSON.parse(process.argv[1]||"{}");
const s=staged.packageManager||""; const h=head.packageManager||"";
if(s!==h){ console.log(`${h}=>${s}`); process.exit(0);} else process.exit(10);
}catch{ process.exit(0) }
- ' "${ROOT_HEAD}" <<<"${ROOT_STAGED}" || true)
- PM_CHANGED=$?
- if [[ "$PM_CHANGED" -eq 0 ]]; then
+ ' "${ROOT_HEAD}" <<<"${ROOT_STAGED}" 2>/dev/null || true)
+ if [[ -n "$PM_CHANGE_OUTPUT" ]]; then
PM_NEW="${PM_CHANGE_OUTPUT#*=>}"
PM_OLD="${PM_CHANGE_OUTPUT%=>*}"
# If switching away from pnpm, block with guidance (this repo standardizes on pnpm)
if [[ ! "$PM_NEW" =~ ^pnpm@ ]]; then
echo "❌ packageManager changed from '${PM_OLD:-<none>}' to '${PM_NEW:-<none>}', which is not pnpm." >&2
echo " This repository is configured for pnpm; update CI/workflows and lockfiles in a dedicated PR if switching managers." >&2
echo " To proceed with pnpm, set packageManager to 'pnpm@<version>' and re-run the commit." >&2
exit 1
fi
# Switching pnpm version: ensure lockfile is regenerated
NEEDS_UPDATE=1
fi🤖 Prompt for AI Agents
In .githooks/pre-commit around lines 98 to 121, the current command substitution
swallows Node’s exit code with "|| true" and then reads $? (always 0), so the
script always thinks packageManager changed; fix by not discarding output/exit
and instead test the presence of the output string: assign PM_CHANGE_OUTPUT from
the node invocation without "|| true", remove the PM_CHANGED numeric check, and
replace it with a conditional like "if [[ -n \"$PM_CHANGE_OUTPUT\" ]]; then" (or
set PM_CHANGED based on -n "$PM_CHANGE_OUTPUT"); keep the existing parsing of
PM_NEW/PM_OLD and remaining logic the same.
| computeGitPRActivity() { | ||
| // Use MORIARTY_BASE_REF or GITHUB_BASE_REF as the base; fall back to origin/main | ||
| let baseRef = process.env.MORIARTY_BASE_REF || process.env.GITHUB_BASE_REF || 'main'; | ||
| try { | ||
| execSync('git rev-parse --is-inside-work-tree', { stdio: 'ignore' }); | ||
| } catch { | ||
| return null; | ||
| } | ||
| try { | ||
| // Ensure we have the base ref locally | ||
| try { execSync(`git fetch --prune origin ${baseRef}:${'refs/remotes/origin/' + baseRef}`, { stdio: 'ignore' }); } catch {} | ||
| const remoteBase = baseRef.startsWith('origin/') ? baseRef : `origin/${baseRef}`; |
There was a problem hiding this comment.
Same fetch refspec bug as WorktreeStrategy; breaks on baseRef like origin/main.
Fetching "origin/main" from remote "origin" doesn’t exist; you end up with bogus refs. Strip the origin/ prefix for the fetch src; let Git maintain refs/remotes/origin/*.
- try { execSync(`git fetch --prune origin ${baseRef}:${'refs/remotes/origin/' + baseRef}`, { stdio: 'ignore' }); } catch {}
- const remoteBase = baseRef.startsWith('origin/') ? baseRef : `origin/${baseRef}`;
+ const branch = baseRef.replace(/^origin\//, '');
+ try { execSync(`git fetch --prune --no-tags origin ${branch}`, { stdio: 'ignore' }); } catch {}
+ const remoteBase = `origin/${branch}`;📝 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.
| computeGitPRActivity() { | |
| // Use MORIARTY_BASE_REF or GITHUB_BASE_REF as the base; fall back to origin/main | |
| let baseRef = process.env.MORIARTY_BASE_REF || process.env.GITHUB_BASE_REF || 'main'; | |
| try { | |
| execSync('git rev-parse --is-inside-work-tree', { stdio: 'ignore' }); | |
| } catch { | |
| return null; | |
| } | |
| try { | |
| // Ensure we have the base ref locally | |
| try { execSync(`git fetch --prune origin ${baseRef}:${'refs/remotes/origin/' + baseRef}`, { stdio: 'ignore' }); } catch {} | |
| const remoteBase = baseRef.startsWith('origin/') ? baseRef : `origin/${baseRef}`; | |
| computeGitPRActivity() { | |
| // Use MORIARTY_BASE_REF or GITHUB_BASE_REF as the base; fall back to origin/main | |
| let baseRef = process.env.MORIARTY_BASE_REF || process.env.GITHUB_BASE_REF || 'main'; | |
| try { | |
| execSync('git rev-parse --is-inside-work-tree', { stdio: 'ignore' }); | |
| } catch { | |
| return null; | |
| } | |
| try { | |
| // Ensure we have the base ref locally | |
| const branch = baseRef.replace(/^origin\//, ''); | |
| try { execSync(`git fetch --prune --no-tags origin ${branch}`, { stdio: 'ignore' }); } catch {} | |
| const remoteBase = `origin/${branch}`; |
🤖 Prompt for AI Agents
In packages/wesley-holmes/src/Moriarty.mjs around lines 541 to 552, the fetch
refspec uses the full baseRef (which may include "origin/") as the source,
producing invalid refs when baseRef is "origin/main"; normalize by stripping any
leading "origin/" from the source side before constructing the refspec and fetch
only the branch name (e.g., fetch <branch> : refs/remotes/origin/<branch>), keep
the remote-side ref as refs/remotes/origin/<branch>, and update remoteBase
determination to still allow baseRef values with or without the origin/ prefix.
| // File writer | ||
| writer: { | ||
| writeFiles: async (artifacts) => { | ||
| writeFiles: async (artifacts, baseDir) => { |
There was a problem hiding this comment.
Validate the baseDir parameter or you'll silently mask caller bugs.
You're accepting baseDir as an optional parameter but doing zero validation up front. If some idiot passes a number or an object, your code silently falls back to 'out' at line 201, and the caller will never know their configuration was ignored.
Either validate the parameter type here, or document that undefined and garbage are treated identically. Right now you're being "helpful" in a way that hides problems.
Add parameter validation:
writer: {
writeFiles: async (artifacts, baseDir) => {
if (!Array.isArray(artifacts)) {
throw new TypeError('writer.writeFiles expects an array of artifacts');
}
+ if (baseDir !== undefined && (typeof baseDir !== 'string' || baseDir.length === 0)) {
+ throw new TypeError('baseDir must be a non-empty string when provided');
+ }BunBun
📝 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.
| writeFiles: async (artifacts, baseDir) => { | |
| writer: { | |
| writeFiles: async (artifacts, baseDir) => { | |
| if (!Array.isArray(artifacts)) { | |
| throw new TypeError('writer.writeFiles expects an array of artifacts'); | |
| } | |
| if (baseDir !== undefined && (typeof baseDir !== 'string' || baseDir.length === 0)) { | |
| throw new TypeError('baseDir must be a non-empty string when provided'); | |
| } |
🤖 Prompt for AI Agents
In packages/wesley-host-node/src/adapters/createNodeRuntime.mjs around line 191,
the writeFiles function accepts an optional baseDir but currently treats any
non-string as if it were undefined and silently falls back to 'out'; change this
to validate baseDir up-front: if baseDir is undefined, keep the existing default
behavior; if baseDir is provided but not a non-empty string, throw a TypeError
(or reject) so callers are notified; implement a simple check (typeof baseDir
!== 'string' || baseDir.trim() === '') to decide between defaulting and
throwing, and update the function JSDoc to document that baseDir must be a
non-empty string when provided.
| let targetPath = artifact?.path; | ||
| if (!targetPath) { | ||
| const name = artifact?.name ?? '<unknown>'; | ||
| throw new TypeError(`Artifact "${name}" is missing a resolved path`); | ||
| // Fallback: write under provided baseDir (or 'out/') if not materialized by the caller | ||
| if (typeof name === 'string' && name.length > 0) { | ||
| const dir = (typeof baseDir === 'string' && baseDir.length > 0) ? baseDir : 'out'; | ||
| targetPath = path.join(dir, name); | ||
| } else { | ||
| throw new TypeError(`Artifact "${name}" is missing a resolved path`); | ||
| } | ||
| } | ||
| await nodeFs.write(artifact.path, artifact.content ?? ''); | ||
| await nodeFs.write(targetPath, artifact.content ?? ''); |
There was a problem hiding this comment.
Your error message is lying, and the fallback logic is convoluted.
Line 204 throws an error saying the artifact is "missing a resolved path", but that's not what you're actually checking. You're checking if the artifact has either a path or a valid name to derive a path from. The real failure mode is "no path AND (no name OR invalid name)". Your error message will confuse the hell out of anyone debugging this.
And what's with the nested ternary soup at line 201? You're checking typeof baseDir === 'string' && baseDir.length > 0 inline, which is defensive but makes the code harder to read.
Simplify and fix the error message:
for (const artifact of artifacts) {
let targetPath = artifact?.path;
if (!targetPath) {
const name = artifact?.name ?? '<unknown>';
- // Fallback: write under provided baseDir (or 'out/') if not materialized by the caller
- if (typeof name === 'string' && name.length > 0) {
- const dir = (typeof baseDir === 'string' && baseDir.length > 0) ? baseDir : 'out';
- targetPath = path.join(dir, name);
- } else {
- throw new TypeError(`Artifact "${name}" is missing a resolved path`);
- }
+ if (typeof name !== 'string' || name.length === 0) {
+ throw new TypeError(
+ `Artifact has neither a "path" property nor a valid "name" to derive one. Got name: ${JSON.stringify(name)}`
+ );
+ }
+ const dir = baseDir || 'out';
+ targetPath = path.join(dir, name);
}
await nodeFs.write(targetPath, artifact.content ?? '');
}Note: If you add the baseDir validation I suggested earlier, you can drop the typeof baseDir === 'string' check here entirely.
BunBun
| cp "$REPO_ROOT/example/schema.graphql" "$TEMP_DIR/schema.graphql" | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Path change relies on example/schema.graphql; ensure it’s tracked in repo and CI container.
If example/schema.graphql isn’t present in published tarballs/containers, E2E will break. Confirm location and add a fallback to fixtures if missing.
🏁 Script executed:
#!/bin/bash
test -f example/schema.graphql || { echo "example/schema.graphql missing" >&2; exit 1; }Length of output: 30
Fix the missing example/schema.graphql file or provide a fallback source for the E2E test.
The test copies example/schema.graphql on lines 10-11, but the file doesn't exist in the repository. The E2E will fail immediately. Either add the missing schema file to the repository or modify the test to use an existing fixture file that's guaranteed to be present.
BunBun
🤖 Prompt for AI Agents
In test/holmes-e2e.bats around lines 10-11, the test blindly copies
example/schema.graphql which is missing and causes E2E failures; update the test
to either copy a guaranteed existing fixture (e.g., test/fixtures/schema.graphql
or example/fixtures/schema.graphql) or add the missing example/schema.graphql
file to the repo. Modify the cp line to reference the chosen existing path (or
add the file into example/), and ensure TEMP_DIR cleanup and test expectations
still match the new source.
| run node -e "const fs=require('fs'); const data=JSON.parse(fs.readFileSync('holmes.json','utf8')); if(!data.metadata || !data.metadata.verificationStatus) throw new Error('missing metadata'); if(typeof data.metadata.verificationCount !== 'number') throw new Error('verificationCount missing'); if(!data.metadata.bundleVersion) throw new Error('missing bundle version'); if(!data.scores) throw new Error('missing scores'); ['scs','tci','mri'].forEach(k=>{ if(typeof data.scores[k] !== 'number') throw new Error('missing scores.'+k); }); if(!data.breakdown || !data.breakdown.scs || !data.breakdown.tci || !data.breakdown.mri) throw new Error('missing breakdown'); if(typeof data.breakdown.scs.sql.score !== 'number' && data.breakdown.scs.sql.score !== null) throw new Error('missing scs sql score'); if(!Array.isArray(data.evidence)) throw new Error('evidence array missing'); if(!data.verdict || !data.verdict.code) throw new Error('missing verdict');" | ||
| assert_success |
There was a problem hiding this comment.
Test can throw TypeError if breakdown.scs.sql is absent; use optional chaining before type check.
Right now you only assert breakdown.{scs,tci,mri} exist, not scs.sql. Accessing scs.sql.score can crash the inline node, hiding the real diagnostic.
- run node -e "const fs=require('fs'); const data=JSON.parse(fs.readFileSync('holmes.json','utf8')); if(!data.metadata || !data.metadata.verificationStatus) throw new Error('missing metadata'); if(typeof data.metadata.verificationCount !== 'number') throw new Error('verificationCount missing'); if(!data.metadata.bundleVersion) throw new Error('missing bundle version'); if(!data.scores) throw new Error('missing scores'); ['scs','tci','mri'].forEach(k=>{ if(typeof data.scores[k] !== 'number') throw new Error('missing scores.'+k); }); if(!data.breakdown || !data.breakdown.scs || !data.breakdown.tci || !data.breakdown.mri) throw new Error('missing breakdown'); if(typeof data.breakdown.scs.sql.score !== 'number' && data.breakdown.scs.sql.score !== null) throw new Error('missing scs sql score'); if(!Array.isArray(data.evidence)) throw new Error('evidence array missing'); if(!data.verdict || !data.verdict.code) throw new Error('missing verdict');"
+ run node -e "const fs=require('fs'); const data=JSON.parse(fs.readFileSync('holmes.json','utf8')); if(!data.metadata || !data.metadata.verificationStatus) throw new Error('missing metadata'); if(typeof data.metadata.verificationCount !== 'number') throw new Error('verificationCount missing'); if(!data.metadata.bundleVersion) throw new Error('missing bundle version'); if(!data.scores) throw new Error('missing scores'); ['scs','tci','mri'].forEach(k=>{ if(typeof data.scores[k] !== 'number') throw new Error('missing scores.'+k); }); if(!data.breakdown || !data.breakdown.scs || !data.breakdown.tci || !data.breakdown.mri) throw new Error('missing breakdown'); const scsSql=data.breakdown?.scs?.sql; if(scsSql && typeof scsSql.score !== 'number' && scsSql.score !== null) throw new Error('missing scs sql score'); if(!Array.isArray(data.evidence)) throw new Error('evidence array missing'); if(!data.verdict || !data.verdict.code) throw new Error('missing verdict');"📝 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.
| run node -e "const fs=require('fs'); const data=JSON.parse(fs.readFileSync('holmes.json','utf8')); if(!data.metadata || !data.metadata.verificationStatus) throw new Error('missing metadata'); if(typeof data.metadata.verificationCount !== 'number') throw new Error('verificationCount missing'); if(!data.metadata.bundleVersion) throw new Error('missing bundle version'); if(!data.scores) throw new Error('missing scores'); ['scs','tci','mri'].forEach(k=>{ if(typeof data.scores[k] !== 'number') throw new Error('missing scores.'+k); }); if(!data.breakdown || !data.breakdown.scs || !data.breakdown.tci || !data.breakdown.mri) throw new Error('missing breakdown'); if(typeof data.breakdown.scs.sql.score !== 'number' && data.breakdown.scs.sql.score !== null) throw new Error('missing scs sql score'); if(!Array.isArray(data.evidence)) throw new Error('evidence array missing'); if(!data.verdict || !data.verdict.code) throw new Error('missing verdict');" | |
| assert_success | |
| run node -e "const fs=require('fs'); const data=JSON.parse(fs.readFileSync('holmes.json','utf8')); if(!data.metadata || !data.metadata.verificationStatus) throw new Error('missing metadata'); if(typeof data.metadata.verificationCount !== 'number') throw new Error('verificationCount missing'); if(!data.metadata.bundleVersion) throw new Error('missing bundle version'); if(!data.scores) throw new Error('missing scores'); ['scs','tci','mri'].forEach(k=>{ if(typeof data.scores[k] !== 'number') throw new Error('missing scores.'+k); }); if(!data.breakdown || !data.breakdown.scs || !data.breakdown.tci || !data.breakdown.mri) throw new Error('missing breakdown'); const scsSql=data.breakdown?.scs?.sql; if(scsSql && typeof scsSql.score !== 'number' && scsSql.score !== null) throw new Error('missing scs sql score'); if(!Array.isArray(data.evidence)) throw new Error('evidence array missing'); if(!data.verdict || !data.verdict.code) throw new Error('missing verdict');" | |
| assert_success |
🤖 Prompt for AI Agents
test/holmes-e2e.bats lines 37-38: the inline Node check accesses
breakdown.scs.sql.score directly which can throw if scs.sql is undefined; change
the condition to use optional chaining when checking score (e.g., reference
breakdown.scs.sql?.score) and then assert that the value is either a number or
null before throwing — this prevents a TypeError and preserves the intended
validation.
1 participant
Integrated via merge commits (no rebase): #323, #264, #284, #319, #265. See description for conflict policy.