Repository navigation
feat: infer ops search_path from schema refs - #463
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughInfer hardened ops Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI: generate-ops"
participant IR as "IR / Metadata"
participant Compiler as "Compiled Ops (QIR)"
participant Infer as "inferOpsSchemaContext"
participant Emitter as "emitFunction / emitView"
participant Lower as "lowerToSQL / TranslateEnv"
CLI->>IR: load IR/metadata
CLI->>Compiler: load compiled ops plans
CLI->>Infer: inferOpsSchemaContext(IR, Compiler, explicitSearchPath?)
Infer-->>CLI: { searchPath, baseSchema }
CLI->>Emitter: emit artifacts with searchPath, baseSchema
Emitter->>Lower: lower QIR (resolveTableRef, renderTableRef)
Lower-->>Emitter: SQL with preserved/validated qualified refs
Emitter-->>CLI: write SQL artifacts (include SET search_path)
sequenceDiagram
participant CI as "CI workflow"
participant Detect as "wesley-generate (detect)"
participant Holmes as "holmes investigate"
participant Cert as "cert-create / cert-verify / cert-badge"
participant Repo as "GitHub (outputs / PR perms)"
CI->>Detect: run detect -> produce schema,bundle outputs
Detect-->>CI: set job outputs (schema, bundle_dir)
CI->>Holmes: run investigate -> write .wesley-cache/holmes-report.json/.md
Holmes-->>CI: report files
CI->>Cert: cert-create reads bundle (and scores) -> build holmes summary
Cert-->>Repo: emit SHIPME artifact and badge (may post PR comment)
CI->>Repo: workflow uses outputs and requires PR write permissions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🔍 The Case of Pull Request #463🕵️ 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: ██████████ 100.0% 🧩 SCS Breakdown
🧪 TCI Breakdown
|
| Component | Risk Share | Points | Count |
|---|---|---|---|
| Drops | 0% | 0 | 0 |
| Renames Without Uid | 0% | 0 | 0 |
| Add Not Null Without Default | 0% | 0 | 0 |
| Non Concurrent Indexes | 0% | 0 | 0 |
📊 The Weight of Evidence
"Observe, Watson, how not all features carry equal importance..."
| Element | Weight | Status | Evidence | Strength | Deduction |
|---|---|---|---|---|---|
| col:User.id | 5 | out/schema.sql:3-3@6336ced | exact | Incomplete | |
| col:User.email | 5 | out/schema.sql:5-5@6336ced | exact | Incomplete | |
| col:User.password_hash | 5 | out/schema.sql:7-7@6336ced | exact | Incomplete | |
| col:User.full_name | 5 | out/schema.sql:8-8@6336ced | exact | Incomplete | |
| col:User.phone | 5 | out/schema.sql:9-9@6336ced | exact | Incomplete | |
| col:User.email_verified | 5 | out/schema.sql:10-10@6336ced | exact | Incomplete | |
| col:User.created_at | 5 | out/schema.sql:11-11@6336ced | exact | Incomplete | |
| col:Product.id | 5 | out/schema.sql:14-14@6336ced | exact | Incomplete | |
| col:Product.sku | 5 | out/schema.sql:16-16@6336ced | exact | Incomplete | |
| col:Product.name | 5 | out/schema.sql:18-18@6336ced | exact | Incomplete | |
| col:Product.slug | 5 | out/schema.sql:19-19@6336ced | exact | Incomplete | |
| col:Product.price_cents | 5 | out/schema.sql:21-21@6336ced | exact | Incomplete | |
| col:Product.stock_quantity | 5 | out/schema.sql:22-22@6336ced | exact | Incomplete | |
| col:Product.published | 5 | out/schema.sql:23-23@6336ced | exact | Incomplete | |
| col:Product.created_at | 5 | out/schema.sql:24-24@6336ced | exact | Incomplete | |
| col:Order.id | 5 | out/schema.sql:27-27@6336ced | exact | Incomplete | |
| col:Order.order_number | 5 | out/schema.sql:29-29@6336ced | exact | Incomplete | |
| col:Order.user_id | 5 | out/schema.sql:31-31@6336ced | exact | Incomplete | |
| col:Order.status | 5 | out/schema.sql:33-33@6336ced | exact | Incomplete | |
| col:Order.total_cents | 5 | out/schema.sql:34-34@6336ced | exact | Incomplete | |
| col:Order.payment_intent_id | 5 | out/schema.sql:35-35@6336ced | exact | Incomplete | |
| col:Order.paid_at | 5 | out/schema.sql:37-37@6336ced | exact | Incomplete | |
| col:Order.created_at | 5 | out/schema.sql:38-38@6336ced | exact | Incomplete | |
| col:OrderItem.id | 5 | out/schema.sql:41-41@6336ced | exact | Incomplete | |
| col:OrderItem.order_id | 5 | out/schema.sql:43-43@6336ced | exact | Incomplete | |
| col:OrderItem.product_id | 5 | out/schema.sql:45-45@6336ced | exact | Incomplete | |
| col:OrderItem.quantity | 5 | out/schema.sql:47-47@6336ced | exact | Incomplete | |
| col:OrderItem.unit_price_cents | 5 | out/schema.sql:48-48@6336ced | exact | Incomplete | |
| tbl:User | 5 | ⛔ Missing | out/tests.sql:11-11@6336ced | exact | Missing |
| tbl:Product | 5 | ⛔ Missing | out/tests.sql:57-57@6336ced | exact | Missing |
| tbl:Order | 5 | ⛔ Missing | out/tests.sql:109-109@6336ced | exact | Missing |
| tbl:OrderItem | 5 | ⛔ Missing | out/tests.sql:161-161@6336ced | exact | Missing |
| col:User.id.pk | 5 | ⛔ Missing | out/tests.sql:203-203@6336ced | exact | Missing |
| col:User.email.unique | 5 | ⛔ Missing | out/tests.sql:204-204@6336ced | exact | Missing |
| col:Product.id.pk | 5 | ⛔ Missing | out/tests.sql:207-207@6336ced | exact | Missing |
| col:Product.sku.unique | 5 | ⛔ Missing | out/tests.sql:208-208@6336ced | exact | Missing |
| col:Product.slug.unique | 5 | ⛔ Missing | out/tests.sql:209-209@6336ced | exact | Missing |
| col:Order.id.pk | 5 | ⛔ Missing | out/tests.sql:212-212@6336ced | exact | Missing |
| col:Order.order_number.unique | 5 | ⛔ Missing | out/tests.sql:213-213@6336ced | exact | Missing |
| col:Order.user_id.fk | 5 | ⛔ Missing | out/tests.sql:214-214@6336ced | exact | Missing |
| col:Order.payment_intent_id.unique | 5 | ⛔ Missing | out/tests.sql:215-215@6336ced | exact | Missing |
| col:OrderItem.id.pk | 5 | ⛔ Missing | out/tests.sql:218-218@6336ced | exact | Missing |
| col:OrderItem.order_id.fk | 5 | ⛔ Missing | out/tests.sql:219-219@6336ced | exact | Missing |
| col:OrderItem.product_id.fk | 5 | ⛔ Missing | out/tests.sql:220-220@6336ced | exact | Missing |
| col:User.id.default | 5 | ⛔ Missing | out/tests.sql:230-230@6336ced | exact | Missing |
| col:User.email_verified.default | 5 | ⛔ Missing | out/tests.sql:234-234@6336ced | exact | Missing |
| col:User.created_at.default | 5 | ⛔ Missing | out/tests.sql:238-238@6336ced | exact | Missing |
| col:Product.id.default | 5 | ⛔ Missing | out/tests.sql:243-243@6336ced | exact | Missing |
| col:Product.stock_quantity.default | 5 | ⛔ Missing | out/tests.sql:247-247@6336ced | exact | Missing |
| col:Product.published.default | 5 | ⛔ Missing | out/tests.sql:251-251@6336ced | exact | Missing |
| col:Product.created_at.default | 5 | ⛔ Missing | out/tests.sql:255-255@6336ced | exact | Missing |
| col:Order.id.default | 5 | ⛔ Missing | out/tests.sql:260-260@6336ced | exact | Missing |
| col:Order.created_at.default | 5 | ⛔ Missing | out/tests.sql:264-264@6336ced | exact | Missing |
| col:OrderItem.id.default | 5 | ⛔ Missing | out/tests.sql:269-269@6336ced | exact | Missing |
| col:User.email.index | 5 | ⛔ Missing | out/tests.sql:281-281@6336ced | exact | Missing |
| col:Product.sku.index | 5 | ⛔ Missing | out/tests.sql:284-284@6336ced | exact | Missing |
| col:Product.slug.index | 5 | ⛔ Missing | out/tests.sql:286-286@6336ced | exact | Missing |
| col:Product.published.index | 5 | ⛔ Missing | out/tests.sql:288-288@6336ced | exact | Missing |
| col:Order.order_number.index | 5 | ⛔ Missing | out/tests.sql:291-291@6336ced | exact | Missing |
| col:Order.user_id.index | 5 | ⛔ Missing | out/tests.sql:293-293@6336ced | exact | Missing |
| col:Order.payment_intent_id.index | 5 | ⛔ Missing | out/tests.sql:295-295@6336ced | exact | Missing |
| col:OrderItem.order_id.index | 5 | ⛔ Missing | out/tests.sql:298-298@6336ced | exact | Missing |
| col:OrderItem.product_id.index | 5 | ⛔ Missing | out/tests.sql:300-300@6336ced | exact | Missing |
🚪 Security & Performance Gates
"Elementary security measures, Watson..."
| Gate | Status | Evidence | Holmes's Ruling |
|---|---|---|---|
| Migration Risk | ✅ | MRI: 0.0% | "Trivial risk" |
| Test Coverage | TCI: 65.0% | "Insufficient coverage" | |
| Sensitive Fields | ⛔ | 1 fields | "1 EXPOSED!" |
| Evidence Quality | ✅ | 91 exact · 0 whole-file · 0 coarse | "All 91 citations resolve to exact line spans." |
📋 The Verdict
"Some clues remain unclear. Address the noted issues."
Signed and sealed,
- S. Holmes, Consulting Detective
[END OF INVESTIGATION FOR COMMIT 6336ced]
🧵 Command Run
- Run ID: run-mn87my98-y3rovn
- Transmutation: holmes-investigate
- Command: investigate
- Status: completed
- Ledger: /home/runner/work/wesley/wesley/test/fixtures/examples/.wesley-cache/ledger
🩺 Dr. WATSON's Verification (click to expand)
🩺 Dr. Watson's Independent Verification Report
Medical Examination of Evidence
- Examination Date: 2026-03-27T01:16:31.616Z
- Patient SHA: 6336ced
🔬 Citation Verification
"Let me examine each piece of evidence independently..."
- Citations Examined: 91
- Verified: 0 ✅
- Failed: 0 ❌
- Unable to Verify: 91
- Exact Subrange Citations: 0
- Whole-file Citations: 0
- Coarse Citations: 0
- Evidence Trust: missing
- Trust Note: No evidence citations were available for trust analysis.
Verification Rate: 0.0%
📊 Mathematical Verification
"I shall recalculate Holmes's arithmetic..."
Holmes claimed SCS: 100.0%
Watson calculates: 0.0%
Difference:
🔍 Consistency Analysis
"Checking for contradictions in Holmes's deductions..."
🩺 Dr. Watson's Medical Opinion
VERIFICATION: CONCERNS NOTED
"While Holmes's methods are generally sound, I have noted some"
"discrepancies that warrant further investigation. No evidence citations were available for trust analysis."
Respectfully submitted,
- Dr. J. Watson, M.D.
Medical Examiner & Verification Specialist
🧵 Command Run
- Run ID: run-mn87nlkm-z5byid
- Transmutation: watson-verify
- Command: verify
- Status: completed
- Ledger: /home/runner/work/wesley/wesley/test/fixtures/examples/.wesley-cache/ledger
🔮 Professor MORIARTY's Predictions (click to expand)
🧠 Professor Moriarty's Temporal Predictions
The Mathematics of Inevitability
- Analysis Date: 2026-03-27T01:17:16.972Z
🔮 Current State
SCS: ██████████ 100.0%
TCI: ███████░░░ 65.0%
MRI: 0.0% risk
Evidence Trust: strong
📈 Velocity Analysis
SCS Velocity: +0.00%/day
Git Activity (window): 24h · commits 4 (0 relevant) · ~4.00 commits/day
↳ Magnitude: ~0 relevant LOC/day across ~0.0 files/day
Activity Index: 7 / 100 (PR 0, Window 17)
Blended Velocity: +0.04%/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% → PASS ✅ (actual 100.0%)
- TCI ≥ 70% → FAIL ❌ (actual 65.0%)
- MRI ≤ 40% → PASS ✅ (actual 0.0%)
- CI Stability ≥ 90% (branch main) → PASS ✅ (actual 91% over ~168h)
- Evidence Trust ≥ moderate → PASS ✅ (actual strong) — All 91 citations resolve to exact line spans.
- Delivery context (last 168h): 12 issues closed · 0 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
03-27: ██████████ 100.0%
03-27: ██████████ 100.0%
03-27: ██████████ 100.0%
"Every problem becomes elementary when reduced to mathematics"
— Professor Moriarty
🧵 Command Run
- Run ID: run-mn87okkq-5n50mx
- Transmutation: moriarty-predict
- Command: predict
- Status: completed
- Ledger: /home/runner/work/wesley/wesley/test/fixtures/examples/.wesley-cache/ledger
Machine-readable reports: holmes-report.json · watson-report.json · moriarty-report.json (see workflow artifacts).
Filed at 221B Repository Street
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47ce1162c5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonl`:
- Line 75: The timestamp field in the JSONL entry uses fractional seconds
("timestamp":"2026-03-25T07:20:33.312Z") but the chronicle contract requires UTC
ISO 8601 timestamps with second precision; update the timestamp value to remove
the fractional seconds ("2026-03-25T07:20:33Z") by editing the "timestamp" field
in the entry, and run the chronicle/CI timestamp validation (or your local JSONL
validator) to ensure no other entries in
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonl contain fractional seconds.
In `@packages/wesley-cli/src/commands/generate-ops.mjs`:
- Around line 424-425: The current logic removes the inferred base schema when
sanitizeSchemaName(ir?.metadata?.schemaName, normalizedSchema) returns the
ops/target schema, causing baseSchema to fall back to 'public' and emit
unqualified refs; change the behavior so that when computing irSchema (using
sanitizeSchemaName) you still add the original ir.metadata.schemaName to
inferred (or preserve the inferred schema) if it equals the target/ops schema so
generated ops use the real schema, not 'public'; update the code paths around
sanitizeSchemaName, the inferred.add(irSchema) call, and the baseSchema fallback
logic (the block that sets baseSchema to 'public') so they preserve and honor
the inferred base schema when it matches targetSchema.
- Around line 417-445: The batch-level baseSchema decision uses the global
planSchemaRefs set, causing one qualified table anywhere to null out baseSchema
for all ops; instead, compute qualified refs per compiled entry by calling
collectPlanSchemaRefs(entry?.plan, localSet) for each entry and use that
per-entry result to decide baseSchema (or null) for that specific op; update the
logic that sets baseSchema/searchPath (references: compiledOps,
collectPlanSchemaRefs, planSchemaRefs, inferredSchemas, baseSchema,
hasQualifiedTableRefs, searchPath, explicit, dedupeSearchPath,
sanitizeSchemaName, ir, normalizedSchema) so the global inferredSchemas remain
for search_path fallback but baseSchema is only nulled for ops whose own plan
contains qualified table refs.
In `@packages/wesley-core/src/domain/qir/lowerToSQL.mjs`:
- Around line 146-149: The splitQualifiedTableRef function should reject
malformed qualified refs instead of collapsing empty segments; remove the
filter(Boolean) behavior and change the logic in splitQualifiedTableRef to split
on '.', trim each part, and return the two-part array only if the original split
yielded exactly two segments and neither trimmed segment is an empty
string—otherwise return null (i.e., detect inputs like "app..orders" or
".orders" as invalid). Ensure you update the check around parts length and
emptiness in splitQualifiedTableRef accordingly.
In `@packages/wesley-core/src/domain/qir/TranslateEnv.mjs`:
- Around line 265-277: splitSchemaQualifiedRef currently filters out empty
segments which causes malformed refs like "app..Product" to be accepted; change
the logic so you first split without filtering and only accept exactly two
non-empty segments: in function splitSchemaQualifiedRef, remove the
.filter(Boolean) behavior and instead check that parts.length === 2 and both
parts[0] and parts[1] are non-empty (trimmed) before returning {schemaName:
parts[0], baseRef: parts[1]}; if that validation fails, return { schemaName:
null, baseRef: ref } to ensure malformed schema-qualified refs are rejected
rather than normalized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f0149d2-0e49-4c83-a337-de95de6ea6c7
📒 Files selected for processing (9)
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonldocs/guides/qir-ops.mdpackages/wesley-cli/src/commands/generate-ops.mjspackages/wesley-cli/src/commands/generate.mjspackages/wesley-cli/test/generate-ops.test.mjspackages/wesley-core/src/domain/qir/TranslateEnv.mjspackages/wesley-core/src/domain/qir/lowerToSQL.mjspackages/wesley-core/test/snapshots/qir-emission.test.mjspackages/wesley-core/test/unit/qir-translate-env.test.mjs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/wesley-cli/src/commands/generate-ops.mjs (2)
153-163:⚠️ Potential issue | 🟠 MajorInfer the hardened schema context per op, not once for the whole batch.
Line 153 computes a single
schemaContextfrom allorderedOps, but emission is still per-entry and can skip failures. With--ops-allow-errors, a bad op can still widenSET search_pathor changebaseSchemafor the ops that do get written, and even without skips every function/view inherits schemas from unrelated ops in the batch. Move this inference into the per-op emission path so each artifact only gets the schemas it actually needs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wesley-cli/src/commands/generate-ops.mjs` around lines 153 - 163, The code currently calls inferOpsSchemaContext once for the whole batch (schemaContext used to set setSearchPath/baseSchema) before calling emitOpArtifacts for orderedOps; change this so you infer the hardened schema context per individual op inside the per-op emission loop: for each entry in orderedOps call inferOpsSchemaContext with that single op (or a minimal compiledOps containing only that op) and then call emitOpArtifacts (or the per-op emitter) passing schemaContext.searchPath and schemaContext.baseSchema for that one artifact; ensure emit logic uses those per-op values so a failing or unrelated op cannot widen SET search_path or change baseSchema for other emitted artifacts (respecting --ops-allow-errors behavior).
610-612:⚠️ Potential issue | 🟠 MajorKeep schema inference aligned with the core qualified-ref parser.
Line 611 still uses
filter(Boolean), soapp..ordersis inferred as schemaapphere even thoughpackages/wesley-core/src/domain/qir/lowerToSQL.mjsandpackages/wesley-core/src/domain/qir/TranslateEnv.mjsnow reject that shape. That lets a malformed op contaminate inferredsearch_path/baseSchemabefore emission. Parse exactly two non-empty segments here too.Suggested fix
function extractSchemaName(tableRef) { - const parts = String(tableRef).split('.').map((part) => part.trim()).filter(Boolean); - return parts.length === 2 ? parts[0] : null; + const parts = String(tableRef).split('.').map((part) => part.trim()); + if (parts.length !== 2) return null; + if (!parts[0] || !parts[1]) return null; + return parts[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wesley-cli/src/commands/generate-ops.mjs` around lines 610 - 612, The extractSchemaName function currently uses String(tableRef).split('.').map(...).filter(Boolean) which accepts inputs like "app..orders"; change it to parse and require exactly two non-empty segments without using filter(Boolean): split on '.', trim parts, then verify parts.length === 2 and both parts[0] and parts[1] are non-empty before returning parts[0]; otherwise return null so the behavior matches the core qualified-ref parser (function extractSchemaName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/wesley-cli/src/commands/generate-ops.mjs`:
- Around line 153-163: The code currently calls inferOpsSchemaContext once for
the whole batch (schemaContext used to set setSearchPath/baseSchema) before
calling emitOpArtifacts for orderedOps; change this so you infer the hardened
schema context per individual op inside the per-op emission loop: for each entry
in orderedOps call inferOpsSchemaContext with that single op (or a minimal
compiledOps containing only that op) and then call emitOpArtifacts (or the
per-op emitter) passing schemaContext.searchPath and schemaContext.baseSchema
for that one artifact; ensure emit logic uses those per-op values so a failing
or unrelated op cannot widen SET search_path or change baseSchema for other
emitted artifacts (respecting --ops-allow-errors behavior).
- Around line 610-612: The extractSchemaName function currently uses
String(tableRef).split('.').map(...).filter(Boolean) which accepts inputs like
"app..orders"; change it to parse and require exactly two non-empty segments
without using filter(Boolean): split on '.', trim parts, then verify
parts.length === 2 and both parts[0] and parts[1] are non-empty before returning
parts[0]; otherwise return null so the behavior matches the core qualified-ref
parser (function extractSchemaName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 409cf87a-1c29-41a8-8d58-7b1748445e8c
📒 Files selected for processing (6)
packages/wesley-cli/src/commands/generate-ops.mjspackages/wesley-cli/test/generate-ops.test.mjspackages/wesley-core/src/domain/qir/TranslateEnv.mjspackages/wesley-core/src/domain/qir/lowerToSQL.mjspackages/wesley-core/test/snapshots/qir-emission.test.mjspackages/wesley-core/test/unit/qir-translate-env.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/wesley-holmes.yml (2)
149-182:⚠️ Potential issue | 🔴 CriticalMissing job dependency:
watson-verifycannot accessneeds.wesley-generate.Line 152 declares
needs: holmes-investigateonly, but lines 169-170 and 181-182 referenceneeds.wesley-generate.outputs.*. GitHub Actions only exposes outputs from jobs explicitly in theneedsarray — this will fail at runtime.🔧 Fix: Add `wesley-generate` to the needs array
watson-verify: name: "🩺 WATSON Verification" runs-on: ubuntu-latest - needs: holmes-investigate + needs: [wesley-generate, holmes-investigate]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/wesley-holmes.yml around lines 149 - 182, The watson-verify job references needs.wesley-generate.outputs.* but its needs array only lists holmes-investigate; update the watson-verify job definition (job name: watson-verify) to include wesley-generate in the needs array (e.g., needs: [holmes-investigate, wesley-generate]) so the referenced outputs (needs.wesley-generate.outputs.bundle_dir and needs.wesley-generate.outputs.schema) are available at runtime.
191-338:⚠️ Potential issue | 🔴 CriticalSame issue:
moriarty-predictcannot accessneeds.wesley-generate.Line 194 declares
needs: watson-verifyonly, but lines 213-214, 233, and 337-338 referenceneeds.wesley-generate.outputs.*. Will fail identically.🔧 Fix: Add `wesley-generate` to the needs array
moriarty-predict: name: "🔮 MORIARTY Predictions" runs-on: ubuntu-latest - needs: watson-verify + needs: [wesley-generate, watson-verify]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/wesley-holmes.yml around lines 191 - 338, The moriarty-predict job is missing the required dependency on wesley-generate so references like needs.wesley-generate.outputs.bundle_dir and needs.wesley-generate.outputs.schema will be undefined; update the moriarty-predict job's needs list (job name: moriarty-predict) to include wesley-generate alongside watson-verify so the outputs used in the HOLMES_SETUP step and the run-holmes-command step resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cert-shipme.yml:
- Around line 22-25: Remove the unnecessary and overly-broad permission grant by
deleting the "pull-requests: write" permission entry from the workflow
permissions block (the YAML key "pull-requests: write"); the PR comment step
only calls Issues API methods
(github.rest.issues.listComments/updateComment/createComment) which are covered
by "issues: write", so remove that key to restore least-privilege.
In `@packages/wesley-cli/src/commands/_cert-utils.mjs`:
- Around line 27-35: The badge currently only uses realm.verdict to decide
PASS/FAIL in buildCertBadge; update it to treat a counterfactual hard-fail as a
fail by computing a counterfactualFail flag (e.g., json?.counterfactual?.gate
=== 'fail') and change okRealm to require realm.verdict === 'PASS' &&
!counterfactualFail so the badge shows FAIL if counterfactual.gate === 'fail';
keep the rest (sha, holmesVerdict, parts) unchanged so HOLMES and sha are still
included.
In `@packages/wesley-cli/src/commands/cert-create.mjs`:
- Around line 283-290: Remove the unnecessary scores guard and pass the original
bundle into the Holmes constructor inside buildShipmeHolmesSummary: stop
returning null when scores is missing, call new Holmes(bundle) (not new Holmes({
...bundle, scores })) and then call investigationData(); ensure the function
still checks for bundle?.evidence but does not require scores so cert.holmes is
populated in bundle-only environments that the Holmes.investigationData() path
supports.
In `@schemas/shipme.schema.json`:
- Around line 75-77: Change the three HOLMES counter properties so they require
integers instead of arbitrary numbers: update verificationCount, gateFailures,
and gateWarnings in the schema to use "type": "integer" (preserving "minimum":
0) so fractional values (e.g., 0.5) are rejected and the contract enforces
whole-number counts.
In `@test/ci-workflows.bats`:
- Around line 77-93: The test checks for consumers of
needs.wesley-generate.outputs.bundle_dir but doesn't verify that the
wesley-generate job actually exposes bundle_dir by mapping
steps.detect.outputs.bundle_dir into its job outputs; update the test to assert
that the producer job's outputs includes a mapping like "outputs: bundle_dir:
${{ steps.detect.outputs.bundle_dir }}" (i.e., add an assertion that grep finds
"outputs.bundle_dir" or the exact "steps.detect.outputs.bundle_dir" within the
job outputs block) so you prove that steps.detect.outputs.bundle_dir is exported
as needs.wesley-generate.outputs.bundle_dir in wesley-holmes.yml.
---
Outside diff comments:
In @.github/workflows/wesley-holmes.yml:
- Around line 149-182: The watson-verify job references
needs.wesley-generate.outputs.* but its needs array only lists
holmes-investigate; update the watson-verify job definition (job name:
watson-verify) to include wesley-generate in the needs array (e.g., needs:
[holmes-investigate, wesley-generate]) so the referenced outputs
(needs.wesley-generate.outputs.bundle_dir and
needs.wesley-generate.outputs.schema) are available at runtime.
- Around line 191-338: The moriarty-predict job is missing the required
dependency on wesley-generate so references like
needs.wesley-generate.outputs.bundle_dir and
needs.wesley-generate.outputs.schema will be undefined; update the
moriarty-predict job's needs list (job name: moriarty-predict) to include
wesley-generate alongside watson-verify so the outputs used in the HOLMES_SETUP
step and the run-holmes-command step resolve correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9b441ad-8f4a-419c-bb5f-a6aa4542c8ec
📒 Files selected for processing (11)
.github/workflows/cert-shipme.yml.github/workflows/wesley-holmes.ymlCHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonlpackages/wesley-cli/src/commands/_cert-utils.mjspackages/wesley-cli/src/commands/cert-badge.mjspackages/wesley-cli/src/commands/cert-create.mjspackages/wesley-cli/src/commands/cert-verify.mjspackages/wesley-cli/test/cert-e2e.batspackages/wesley-cli/test/schema-validator.test.mjsschemas/shipme.schema.jsontest/ci-workflows.bats
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wesley-cli/src/commands/cert-verify.mjs`:
- Around line 54-58: The output currently sets holmesVerdict from raw
json?.holmes?.shipVerdict which can disagree with the normalized policy fields
(holmesPassed, reasons); replace that expression with the normalized value
policy.holmesVerdict so holmesVerdict, holmesPassed and reasons are consistent
(update the object field assignment for holmesVerdict to use
policy.holmesVerdict and keep holmes: json?.holmes || null as the raw payload).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05e458fe-5b51-4f1a-9c21-fff08ab5f713
📒 Files selected for processing (6)
CHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonldocs/blade.mddocs/holmes-policy-spec.mdpackages/wesley-cli/src/commands/_cert-utils.mjspackages/wesley-cli/src/commands/cert-verify.mjspackages/wesley-cli/test/cert-e2e.bats
|
@coderabbitai Thanks. I checked actual repo behavior before narrowing this permission. In run |
|
Tip For best results, initiate chat on the files or code changes.
This is a known GitHub Actions quirk: GitHub's token permission model does not always map 1:1 to the underlying REST API endpoint. Even though PR comments go through the Issues API ( Your stated exit criterion (a reproducible repo-local proof that the narrower scope works end-to-end) is the right bar. The current state is well-justified. No further action needed here. ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wesley-cli/test/cert-e2e.bats`:
- Around line 136-187: The fixture .wesley-cache/bundle.json used in the test
(the bundle JSON created in the cert-e2e.bats diff) is missing the
bundle.testResults payload so Holmes’ verification/execution/conclusion logic
isn’t exercised; update the fixture JSON to include a realistic "testResults"
object under the top-level bundle (e.g., alongside "scores" and "evidence") that
contains representative test suites, test cases and pass/fail outcomes (matching
the files referenced in "evidence.schema.tests" and "evidence.schema.sql") so
that code paths which read bundle.testResults are executed during the e2e tests
and policy-facing summaries are validated.
- Around line 125-223: The test defines duplicate HOLMES fixture builders (both
blocks inside create_holmes_summary_inputs) which will diverge; refactor by
extracting a single parameterized helper (e.g., create_holmes_fixture or a
reusable buildBundleJSON function) and call it twice with different parameters
instead of duplicating the JSON and file stubs inside
create_holmes_summary_inputs; update the function to write schema.sql, tests.sql
and then call the helper to render .wesley-cache/bundle.json and
.wesley-cache/scores.json based on passed parameters
(sha/timestamp/scores/readiness/breakdown/evidence) so all fixture shape changes
are made in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c88acada-9a53-4c76-b74d-4b388cd7e91a
📒 Files selected for processing (9)
.github/workflows/cert-shipme.yml.github/workflows/wesley-holmes.ymlCHANGELOG.mdCHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonlpackages/wesley-cli/src/commands/cert-create.mjspackages/wesley-cli/src/commands/cert-verify.mjspackages/wesley-cli/test/cert-e2e.batsschemas/shipme.schema.jsontest/ci-workflows.bats
|
@coderabbitai review please 🥕 |
🚢 SHIPME[SHIPME] PASS · HOLMES ELEMENTARY — sha 6336ced |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/wesley-holmes.yml (1)
742-750:⚠️ Potential issue | 🟡 MinorDuplicated "Ensure history for MORIARTY" step in the wrong job.
Lines 742–750 are an exact duplicate of lines 222–229. This step belongs in the
moriarty-predictjob (where it already exists), not incomment-report. Thecomment-reportjob doesn't run any MORIARTY commands—it only assembles and posts a PR comment from downloaded artifacts.Delete this orphaned step; it serves no purpose and clutters the workflow.
🗑️ Proposed removal
if (botComment) { await github.rest.issues.updateComment({ owner: context.repo.owner, repo: context.repo.repo, comment_id: botComment.id, body }); } else { await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, body }); } - - name: "🔄 Ensure history for MORIARTY" - shell: bash - run: | - set -euo pipefail - # In case fetch-depth wasn't honored or repo is shallow, try to unshallow - if git rev-parse --is-shallow-repository >/dev/null 2>&1; then - git fetch --prune --unshallow --tags || true - fi - git fetch --prune origin '+refs/heads/*:refs/remotes/origin/*' || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/wesley-holmes.yml around lines 742 - 750, The workflow contains a duplicated step named "🔄 Ensure history for MORIARTY" that is incorrectly present in the comment-report job; remove that orphaned step from the comment-report job so the only instance remains in the moriarty-predict job (keep the existing step in moriarty-predict, delete the block with name "🔄 Ensure history for MORIARTY" under the comment-report job).
♻️ Duplicate comments (2)
packages/wesley-cli/test/cert-e2e.bats (2)
125-286: 🧹 Nitpick | 🔵 TrivialFixture helpers are duplicated—consider consolidating to reduce drift risk.
create_holmes_summary_inputs()(lines 125–222) andcreate_passing_holmes_summary_inputs()(lines 224–286) share ~90% identical JSON structure. When the HOLMES bundle schema evolves, you'll need to update both in lockstep or risk silent test rot.Extract a parameterized helper that accepts score/coverage values and emits the fixture. This is a maintenance hygiene concern, not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wesley-cli/test/cert-e2e.bats` around lines 125 - 286, Both create_holmes_summary_inputs() and create_passing_holmes_summary_inputs() duplicate the same fixture creation logic; refactor by extracting a single parameterized helper (e.g., generate_holmes_summary_fixture) that accepts the varying values (scores.tci, scores.scs.tests, tci.e2e_ops.covered/total, evidence.tests lines, etc.) and is invoked by the two existing functions to write schema.sql, tests.sql, .wesley-cache/bundle.json and .wesley-cache/scores.json; update create_holmes_summary_inputs and create_passing_holmes_summary_inputs to call that helper with the appropriate parameters so the shared JSON structure lives in one place.
136-187: 🧹 Nitpick | 🔵 TrivialFixtures omit
testResults, masking Holmes verification metric paths.Neither
create_holmes_summary_inputsnorcreate_passing_holmes_summary_inputsincludes atestResultsblock in the bundle.Holmes.investigationData()derives verification/execution/conclusion metrics frombundle.testResults. Without it, the E2E tests don't exercise those code paths, which could mask regressions in policy-facing summary behavior.Consider adding a minimal
testResultsto at least one fixture to exercise the full investigation flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wesley-cli/test/cert-e2e.bats` around lines 136 - 187, The fixture bundle JSON is missing a testResults block so Holmes.investigationData() never exercises verification/execution/conclusion metrics; add a minimal testResults object to the fixture produced by create_holmes_summary_inputs (or create_passing_holmes_summary_inputs) that mirrors the shape Holmes expects (e.g., overall verdict/status, at least one test entry and timing/execution fields) so bundle.testResults is present and the full investigation flow runs, ensuring Holmes.investigationData() can compute verification metrics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/wesley-holmes.yml:
- Around line 141-142: The workflow validates and accepts the input schema-path
but never forwards it to the CLI, so update the run step that invokes cli.mjs
(the call that currently passes --bundle-dir, --history-file, --json) to also
pass --schema-path with the value from the action input (schema-path), or
alternatively remove schema-path from the action inputs and callers if the CLI
does not require it; ensure changes reference the run-holmes-command action
input named schema-path and the cli.mjs invocation so the validated path is
actually used by the tool.
In `@packages/wesley-core/src/application/Scoring.mjs`:
- Around line 392-393: The calculateIndexCoverage method currently returns 1
when total === 0, inconsistent with its sibling methods
(calculateTestCoverageDetails, calculateConstraintCoverageDetails,
calculateRelationCoverageDetails, calculateRlsCoverageDetails) which return 0
for empty sets; change calculateIndexCoverage to return total > 0 ? covered /
total : 0 to match the others, and add a short comment above the return
explaining that an empty set yields 0 coverage for consistency with other
coverage calculations (or alternatively update the other methods if you prefer
the vacuous-truth semantics—be consistent across all calculate*Coverage
methods).
In `@packages/wesley-core/test/unit/scoring-engine.test.mjs`:
- Around line 176-177: Add a regression unit test that ensures
calculateIndexCoverage on ScoringEngine returns 0 when there are indexed fields
but no test evidence recorded: create an EvidenceMap (do not add index evidence
but set a sha), build a schema/table with fields including an indexed field (use
createField for 'email' with { indexed: true } and a primary key field), provide
a schema object with getTables -> [table] and table.getFields -> fields,
instantiate new ScoringEngine(evidence) and assert
scoring.calculateIndexCoverage(schema, []) === 0 to lock in the total>0 branch
behavior.
- Line 148: Rename the ambiguous test title to explicitly reflect scenario A (no
indexed fields present): update the test invocation test('TCI treats missing
index obligations as fully covered', ...) to a clearer name like test('TCI
treats absent index obligations (schema has no indexed fields) as fully
covered', ...) so the test for no-index-schema behavior is unambiguous; leave
the test body and assertions unchanged.
---
Outside diff comments:
In @.github/workflows/wesley-holmes.yml:
- Around line 742-750: The workflow contains a duplicated step named "🔄 Ensure
history for MORIARTY" that is incorrectly present in the comment-report job;
remove that orphaned step from the comment-report job so the only instance
remains in the moriarty-predict job (keep the existing step in moriarty-predict,
delete the block with name "🔄 Ensure history for MORIARTY" under the
comment-report job).
---
Duplicate comments:
In `@packages/wesley-cli/test/cert-e2e.bats`:
- Around line 125-286: Both create_holmes_summary_inputs() and
create_passing_holmes_summary_inputs() duplicate the same fixture creation
logic; refactor by extracting a single parameterized helper (e.g.,
generate_holmes_summary_fixture) that accepts the varying values (scores.tci,
scores.scs.tests, tci.e2e_ops.covered/total, evidence.tests lines, etc.) and is
invoked by the two existing functions to write schema.sql, tests.sql,
.wesley-cache/bundle.json and .wesley-cache/scores.json; update
create_holmes_summary_inputs and create_passing_holmes_summary_inputs to call
that helper with the appropriate parameters so the shared JSON structure lives
in one place.
- Around line 136-187: The fixture bundle JSON is missing a testResults block so
Holmes.investigationData() never exercises verification/execution/conclusion
metrics; add a minimal testResults object to the fixture produced by
create_holmes_summary_inputs (or create_passing_holmes_summary_inputs) that
mirrors the shape Holmes expects (e.g., overall verdict/status, at least one
test entry and timing/execution fields) so bundle.testResults is present and the
full investigation flow runs, ensuring Holmes.investigationData() can compute
verification metrics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d51fc6d-337b-4d3c-9243-df98f0edf75a
📒 Files selected for processing (11)
.github/workflows/cert-shipme.yml.github/workflows/wesley-holmes.ymlCHANGELOG.mdCHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonlpackages/wesley-cli/src/commands/cert-create.mjspackages/wesley-cli/src/commands/cert-verify.mjspackages/wesley-cli/test/cert-e2e.batspackages/wesley-core/src/application/Scoring.mjspackages/wesley-core/test/unit/scoring-engine.test.mjsschemas/shipme.schema.jsontest/ci-workflows.bats
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wesley-cli/test/cert-e2e.bats`:
- Around line 453-458: Replace the combined jq patterns that use
'select(has("X")) | .X == Y' with two explicit checks so missing-field vs
bad-value are distinguishable: first run jq -e 'has("holmesPassed")' and then jq
-e '.holmesPassed == false', do the same for 'holmesVerdict' (check
has("holmesVerdict") then '.holmesVerdict == "REQUIRES INVESTIGATION"') and for
'eligibleToShip' (has("eligibleToShip") then '.eligibleToShip == false'); update
the three echo "$output" | jq -e ... lines to perform these two-step assertions
for fields holmesPassed, holmesVerdict, and eligibleToShip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa1465c5-6450-478f-bb0a-92836a34a93b
📒 Files selected for processing (5)
CHANGELOG.mdCHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonlpackages/wesley-cli/test/cert-e2e.batspackages/wesley-core/src/application/Scoring.mjspackages/wesley-core/test/unit/scoring-engine.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 69-74: Remove the PR-thread meta phrase "This keeps the reviewer’s
'field exists vs wrong value' distinction while staying robust when the command
also emits the framework error envelope" and rewrite that sentence to state the
observable effect: that the HOLMES failure-path cert E2E tests now assert
against the first JSON document emitted by `cert-verify --json`, separating
presence checks from value checks for `holmesPassed`, `holmesVerdict`,
`eligibleToShip`, and `reasons`, so the tests remain robust when the command
also emits an outer error envelope (PR `#463`).
In `@packages/wesley-cli/test/cert-e2e.bats`:
- Around line 149-246: The test fixture currently duplicates the same scoring
payload in the .wesley-cache/bundle.json and .wesley-cache/scores.json files;
consolidate by emitting the scores object once and reusing it in both outputs:
generate the scores JSON into a single variable/file (e.g., a SCORE_PAYLOAD
variable or a temporary .wesley-cache/scores.json) and then embed or include
that single source when creating bundle.json (replace the duplicated "scores": {
... } block in the bundle creation with the shared payload reference), ensuring
the same canonical payload is written to both the scores file and the bundle's
top-level scores.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2c748ed-110c-45df-a791-e6bb04f7ef2d
📒 Files selected for processing (5)
CHANGELOG.mdCHRONICLES_OF_THE_MACHINE-KIND_VOL_00000010.jsonlpackages/wesley-cli/test/cert-e2e.batspackages/wesley-core/src/application/Scoring.mjspackages/wesley-core/test/unit/scoring-engine.test.mjs
Summary
search_pathvalues from IR metadata and schema-qualified plan refs--ops-search-pathoverrides while keeping schema-aware base table qualificationTesting
Closes #439