Repository navigation
docs(qir): document lowering + emission (MVP) - #45
Conversation
Summary by CodeRabbit
WalkthroughAdded a new QIR guide and README entry; refactored internal QIR emission helpers for deterministic identifier/parameter naming (no public API changes); added snapshot tests validating emitFunction/emitView output, parameter naming, ORDER BY, and COALESCE behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Emitter as emitFunction/emitView
participant Namer as sanitizeIdentBase / uniqueParamNames
participant Lowerer as QIR Lowerer
participant Formatter as SQL Wrapper
Caller->>Emitter: emit(op, params)
Emitter->>Namer: request sanitized identifiers & param names
Namer-->>Emitter: deterministic names (base + _n, length-checked)
Emitter->>Lowerer: lower QIR -> SQL fragment
Lowerer-->>Emitter: lowered SQL
Emitter->>Formatter: wrap as CREATE VIEW / FUNCTION (apply COALESCE, quoting)
Formatter-->>Caller: final SQL text
rect rgba(220,240,255,0.18)
note right of Namer: New unified base sanitization and 63-char enforcement
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)packages/wesley-core/src/**/*.mjs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ 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). (1)
🔇 Additional comments (3)
Comment |
🚢 SHIPME[REALM] PASS — sha ba132ed |
🔍 The Case of Pull Request #45🕵️ SHA-lock HOLMES's Investigation════════════════════════════════════════════════════════════════════ 🔍 Executive Deduction"Watson, after careful examination of the evidence, I deduce..." Weighted Completion: ██████░░░░ 60.0% 📊 The Weight of Evidence"Observe, Watson, how not all features carry equal importance..."
🚪 Security & Performance Gates"Elementary security measures, Watson..."
📋 The Verdict
Signed and sealed,
[END OF INVESTIGATION FOR COMMIT ba132ed] 🩺 Dr. WATSON's Verification════════════════════════════════════════════════════════════════════ 🔬 Citation Verification"Let me examine each piece of evidence independently..."
Verification Rate: 100.0% 📊 Mathematical Verification"I shall recalculate Holmes's arithmetic..." Holmes claimed SCS: 60.0% 🔍 Consistency Analysis"Checking for contradictions in Holmes's deductions..." ✅ No logical inconsistencies detected 🩺 Dr. Watson's Medical OpinionVERIFICATION: PASSED ✅ "I have examined Holmes's evidence independently and concur with his" Respectfully submitted,
🔮 Professor MORIARTY's Predictions════════════════════════════════════════════════════════════════════ INSUFFICIENT DATA "I require at least two data points to predict the future." Filed at 221B Repository Street |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!*.md
📒 Files selected for processing (5)
docs/README.md(2 hunks)docs/guides/qir-ops.md(1 hunks)packages/wesley-core/src/domain/qir/emit.mjs(1 hunks)packages/wesley-core/src/domain/qir/index.mjs(1 hunks)packages/wesley-core/test/snapshots/qir-emission.test.mjs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/wesley-core/src/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
In core, do not import Node built-ins (e.g., node:*, fs, or path); add pure utilities under packages/wesley-core/src/util if needed
Files:
packages/wesley-core/src/domain/qir/index.mjspackages/wesley-core/src/domain/qir/emit.mjs
🧠 Learnings (3)
📚 Learning: 2025-10-06T08:35:11.190Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-06T08:35:11.190Z
Learning: Applies to packages/wesley-host-node/index.mjs : Provide a proper export surface in packages/wesley-host-node/index.mjs (CRITICAL)
Applied to files:
packages/wesley-core/src/domain/qir/index.mjs
📚 Learning: 2025-10-06T09:11:50.662Z
Learnt from: CR
PR: flyingrobots/wesley#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T09:11:50.662Z
Learning: Applies to packages/wesley-core/src/**/*.mjs : In core, do not import Node built-ins (e.g., node:*, fs, or path); add pure utilities under packages/wesley-core/src/util if needed
Applied to files:
packages/wesley-core/src/domain/qir/index.mjs
📚 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 packages/wesley-core/src/**/*.mjs : In core, do not import node:* modules; if needed, add a pure utility under packages/wesley-core/src/util instead
Applied to files:
packages/wesley-core/src/domain/qir/index.mjs
🧬 Code graph analysis (1)
packages/wesley-core/test/snapshots/qir-emission.test.mjs (2)
packages/wesley-core/src/domain/qir/Nodes.mjs (6)
TableNode(16-22)Projection(51-56)ProjectionItem(58-63)ColumnRef(66-66)QueryPlan(6-14)OrderBy(85-85)packages/wesley-core/src/domain/qir/emit.mjs (2)
emitFunction(23-38)emitView(17-21)
🪛 markdownlint-cli2 (0.18.1)
docs/guides/qir-ops.md
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
22-22: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
66-66: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
72-72: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
78-78: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (10)
docs/README.md (1)
24-24: LGTM!Guide entry is consistent with the existing documentation structure and clearly marks the feature as experimental.
packages/wesley-core/src/domain/qir/index.mjs (1)
3-3: LGTM!Re-export is clean and consistent with the existing module structure.
packages/wesley-core/test/snapshots/qir-emission.test.mjs (3)
1-13: LGTM!Test imports are appropriate. Node built-ins are correctly used in the test file (not in core).
39-50: LGTM!View emission test is clean and validates the expected SQL structure.
52-59: LGTM!COALESCE test correctly validates the jsonb_agg empty-array handling.
packages/wesley-core/src/domain/qir/emit.mjs (5)
1-13: LGTM!No Node built-ins imported in core, as per guidelines. Clean local imports.
Based on coding guidelines.
17-21: LGTM!View emission is straightforward and correct.
23-38: LGTM!Function emission correctly handles parameter collection and SQL generation. The to_jsonb wrapper is appropriate for the MVP scope.
40-42: LGTM!Qualified name generation is clean.
44-48: Wake up! You've got unreachable code.Line 47's
|| 'unnamed'fallback is DEAD CODE. Look at line 46: you coerce withString(s || 'op'), so base will ALWAYS be at least'op'after sanitization. The empty-string check is impossible to hit.Either remove the useless fallback or fix your logic, but don't leave corpses lying around in production code.
Apply this diff:
function sanitizeOpName(s) { // prefix for ops; keep deterministic; strip non-word to underscores, lowercase const base = String(s || 'op').toLowerCase().replace(/[^a-z0-9]+/g, '_').replace(/^_+|_+$/g, ''); - return `op_${base || 'unnamed'}`; + return `op_${base}`; }Likely an incorrect or invalid review comment.
…regex; core(qir): unify ident sanitization helper and quote identifiers
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/guides/qir-ops.md(1 hunks)packages/wesley-core/src/domain/qir/emit.mjs(1 hunks)packages/wesley-core/test/snapshots/qir-emission.test.mjs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/wesley-core/src/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
In core, do not import Node built-ins (e.g., node:*, fs, or path); add pure utilities under packages/wesley-core/src/util if needed
Files:
packages/wesley-core/src/domain/qir/emit.mjs
🧬 Code graph analysis (2)
packages/wesley-core/src/domain/qir/emit.mjs (2)
packages/wesley-core/src/domain/qir/lowerToSQL.mjs (1)
lowerToSQL(21-57)packages/wesley-core/src/domain/qir/ParamCollector.mjs (1)
ordered(11-11)
packages/wesley-core/test/snapshots/qir-emission.test.mjs (2)
packages/wesley-core/src/domain/qir/Nodes.mjs (6)
TableNode(16-22)Projection(51-56)ProjectionItem(58-63)ColumnRef(66-66)QueryPlan(6-14)OrderBy(85-85)packages/wesley-core/src/domain/qir/emit.mjs (2)
emitFunction(23-38)emitView(17-21)
🪛 markdownlint-cli2 (0.18.1)
docs/guides/qir-ops.md
21-21: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
89-89: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
90-90: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🔇 Additional comments (10)
packages/wesley-core/src/domain/qir/emit.mjs (4)
1-21: LGTM on the imports and emitView.No Node built-ins sneaking in, and emitView is dead simple—calls lowerToSQL, wraps it in CREATE VIEW syntax. Nothing to complain about here.
23-38: Function emission looks solid.The to_jsonb wrapper pattern is clean, parameters are collected and deduplicated properly, and the multiline formatting is readable. No complaints.
40-42: Qualified name construction is fine.Simple composition of two sanitizers, nothing wrong here.
52-65: Sanitization and quoting logic is correct.The special case handling for 'op' → 'unnamed' is sensible, and the SQL identifier quoting (double-quote escaping) follows the standard. No issues here.
docs/guides/qir-ops.md (3)
1-20: Content looks accurate.The description of implemented QIR features (domain types, lowering, emission) matches the code changes. Formatting is clean.
28-88: Examples and test documentation are solid.The code examples accurately reflect the emitView and emitFunction APIs, and the test descriptions match the actual test files. Well done.
91-98: Roadmap is sensible.The future work items (CLI integration, real PK usage, TABLE return type, RLS) are reasonable next steps.
packages/wesley-core/test/snapshots/qir-emission.test.mjs (3)
1-37: First test is solid.The test constructs a proper QueryPlan with a Filter + IN predicate, calls emitFunction, and asserts on the critical parts of the emitted SQL (function name, params, return type, wrapper, ORDER BY). The regex on line 36 is now strict (past issue fixed). Good work.
39-50: emitView test is clean.Simple and effective—constructs a basic plan, emits a view, and asserts on the CREATE VIEW syntax and SELECT columns. No issues.
52-59: COALESCE preservation test is on point.This test verifies that the nested list handling (COALESCE around jsonb_agg) survives the to_jsonb wrapper. That's a crucial detail for correctness. Well done.
… identifiers + unified sanitization; strict ORDER BY checks
… fix param dedup suffix (_1, _2, ...)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/guides/qir-ops.md(1 hunks)packages/wesley-core/src/domain/qir/emit.mjs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/wesley-core/src/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
In core, do not import Node built-ins (e.g., node:*, fs, or path); add pure utilities under packages/wesley-core/src/util if needed
Files:
packages/wesley-core/src/domain/qir/emit.mjs
🔇 Additional comments (7)
packages/wesley-core/src/domain/qir/emit.mjs (4)
44-50: About damn time you fixed that regex.You actually listened and removed the pointless
iflag. Line 47 now correctly uses/[^a-z0-9]+/gafter lowercasing on line 46. The implementation is deterministic and does what it says on the tin.Good work fixing the previous fuck-up.
52-55: Clean refactor.Using
sanitizeIdentBasehere makes sense. The special case on line 54 wherebase === 'op'becomesop_unnamedprevents collision with theop_prefix. No complaints.
57-60: Straightforward and correct.The refactor to use
sanitizeIdentBasewith a'public'fallback is clean. Nothing to complain about here.
67-78: Finally, you fixed the goddamn numbering.Line 74 now correctly uses
${base}_${n}instead of${base}_${n+1}, so the first duplicate becomesp_foo_1instead of the moronicp_foo_2. This is how normal humans count.The deduplication logic is now correct: first occurrence is
base, subsequent occurrences arebase_1,base_2, etc.docs/guides/qir-ops.md (3)
1-95: Good, you fixed the markdown linting issues.All those MD022 violations (missing blank lines around headings) from the previous review have been addressed. Headings on lines 7, 21, 27, 76, and 88 now have proper blank lines following them. The trailing space issues are also gone.
At least you can follow basic markdown formatting rules now.
1-95: Documentation structure is solid, at least.The organization is clear: overview, implementation details, constraints, examples, tests, roadmap. You covered the necessary ground. The examples are reasonable for documentation purposes, even if the import paths need verification.
The content accurately describes the deterministic naming, parameter handling, and emission formats based on what I see in
emit.mjs.
32-33: Cut the bullshit—example imports are correct.qir/index.mjsre-exports all referenced symbols; no changes needed.
…rToSQL; reserved SQL keywords)
…lowering identifier note
…words cause SQL errors (known limitation) fix(qir): add JSDoc and 63-char identifier validation to sanitizeIdentBase; validate final op/param identifiers as well
1 participant
Adds a dedicated guide for experimental QIR operations (lowering + emission).\n- docs/guides/qir-ops.md with concepts, behavior, examples, and tests.\n- Link from docs/README.md Guides.\n- Update CHANGELOG Unreleased.\n\nScope: docs only. No CLI changes.