Resolve relation form coverage (2.6.6) - #263
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe parser now supports structured relation roles, kinds, reference markers, record or bracket bodies, primary-key diagnostics, and typed CST accessors. Cursor-based scanning adds recovery-aware parsing. Tests, property generators, documentation, spelling configuration, and quality-gate guidance were updated. Relation Declaration Support
Quality Gate Guidance
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideRefreshes AGENTS.md from upstream with clarified documentation, testing, and tooling guidelines, and adds a detailed ExecPlan document for roadmap item 2.6.6 that specifies how to extend the parser’s relation handling (role/kind/body forms, diagnostics, tests, milestones, and documentation updates). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/parser/tests/relation_proptest.rs Comment on lines +42 to +58 fn new(
role: GeneratedRole,
kind: GeneratedKind,
body: GeneratedBody,
is_ref: bool,
wants_primary_key: bool,
) -> Self {
let has_primary_key =
wants_primary_key && role == GeneratedRole::Input && body == GeneratedBody::Record;
Self {
role,
kind,
body,
is_ref,
has_primary_key,
}
}❌ New issue: Excess Number of Function Arguments |
1 similar comment
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/parser/tests/relation_proptest.rs Comment on lines +42 to +58 fn new(
role: GeneratedRole,
kind: GeneratedKind,
body: GeneratedBody,
is_ref: bool,
wants_primary_key: bool,
) -> Self {
let has_primary_key =
wants_primary_key && role == GeneratedRole::Input && body == GeneratedBody::Record;
Self {
role,
kind,
body,
is_ref,
has_primary_key,
}
}❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Excess Number of Function Argumentssrc/parser/tests/relation_proptest.rs: GeneratedRelation.new What lead to degradation?GeneratedRelation.new has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
leynos
changed the title
be786c0 to
b88449c
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
c45e708 to
1ffb4c1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? docs/differential-datalog-parser-syntax-spec-updated.md (1)
Suggested wording fix-- **Keywords:** `type`, `function`, `extern`, `transformer`, `input`, `output`,
- `internal`,`relation`,`stream`,`multiset`,`index`,`on`,`primary`,`key`,`apply`,`match`,`if`,`else`,`for`,`in`,`then`,`skip`,`true`,`false`,`var`,`mut`,`return`,`break`,`continue`.
+- **Keywords:** `type`, `function`, `extern`, `transformer`, `input`, `output`,
+ `relation`,`stream`,`multiset`,`index`,`on`,`primary`,`key`,`apply`,`match`,`if`,`else`,`for`,`in`,`then`,`skip`,`true`,`false`,`var`,`mut`,`return`,`break`,`continue`.
@@
-- Absence of Role means an internal relation. `internal` is not a reserved
-- keyword.
+- Absence of Role means an internal relation; this is represented implicitly,
+ not by a dedicated `internal` keyword.Also applies to: 256-284 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In src/parser/ast/parse_utils/relation.rs (1)
Proposed wording-//! These helpers parse relation column lists and `primary key` clauses using
-//! `chumsky`, allowing callers to validate structure without manual token
-//! juggling.
-//! They are shared by the span scanner and AST helpers to keep behaviour
-//! consistent.
+//! These helpers parse relation `primary key` clauses using `chumsky`,
+//! allowing callers to validate structure without manual token juggling.🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
relation.rs: - primary_key() now renders chumsky `Simple<SyntaxKind>` recovery errors as human-readable diagnostics (via reason/expected/found + token symbols) instead of Debug struct dumps, since `SyntaxKind` has no `Display`. - Extract a shared `body_inspection_error` helper mapping `MalformedBracket`/`MissingDelimiter` to their error vectors; `body()`, `element_type()`, and `columns()` short-circuit through it before handling their `Record`/`Bracket` success cases. Results and messages unchanged. - relation/tests.rs: `malformed_preamble_delimiters_do_not_leak_body_names` now asserts `relations().is_empty()`, so it fails if any candidate (even nameless) is emitted, keeping the source context. - inspect.rs: expand the tests module `//!` doc to describe the malformed bracket, invalid column list, malformed primary-key binder, and missing delimiter coverage. Docs: - AGENTS.md and developers guide: add `make nixie` (Mermaid validation) to the pre-commit / maintained-Markdown workflow. - users guide: fix the D-REL-008 message to match the scanner output (trailing apostrophe before the closing inline-code marker). - spec + execplan D2: document that the local grammar accepts a comma-separated compound binder list, distinct from upstream's single lambda-style binder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (1)
validated alongside 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines docs/users-guide.md (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In docs/differential-datalog-parser-syntax-spec-updated.md (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In src/parser/ast/relation.rs (1)
♻️ Proposed refactor+fn body_inspection_error(inspection: &inspect::BodyInspection) -> Option<RelationParseErrors> {
+ match inspection {
+ inspect::BodyInspection::MalformedBracket => Some(vec![MALFORMED_BRACKET_BODY.to_string()]),
+ inspect::BodyInspection::MissingDelimiter => Some(vec![MISSING_BODY_DELIMITER.to_string()]),
+ inspect::BodyInspection::Record | inspect::BodyInspection::Bracket(_) => None,
+ }
+}
+
pub fn body(&self) -> Result<RelationBody, RelationParseErrors> {
- match inspect::inspect_body(&self.syntax) {
- inspect::BodyInspection::Record => Ok(RelationBody::Fields(self.columns()?)),
- inspect::BodyInspection::Bracket(element) => Ok(RelationBody::ElementType(element)),
- inspect::BodyInspection::MalformedBracket => {
- Err(vec![MALFORMED_BRACKET_BODY.to_string()])
- }
- inspect::BodyInspection::MissingDelimiter => {
- Err(vec![MISSING_BODY_DELIMITER.to_string()])
- }
- }
+ let inspection = inspect::inspect_body(&self.syntax);
+ if let Some(err) = body_inspection_error(&inspection) {
+ return Err(err);
+ }
+ match inspection {
+ inspect::BodyInspection::Record => Ok(RelationBody::Fields(self.columns()?)),
+ inspect::BodyInspection::Bracket(element) => Ok(RelationBody::ElementType(element)),
+ inspect::BodyInspection::MalformedBracket | inspect::BodyInspection::MissingDelimiter => {
+ unreachable!("handled above")
+ }
+ }
}🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In src/parser/ast/relation/inspect.rs (1)
📝 Proposed doc updatemod tests {
- //! Regression tests for the delimiter guard that stops preamble scanning
- //! before a body delimiter when a malformed node lacks a relation name.
+ //! Regression tests for the delimiter guard that stops preamble scanning
+ //! before a body delimiter when a malformed node lacks a relation name,
+ //! plus `BodyInspection` error-path coverage for malformed bracket
+ //! bodies, invalid record column lists, malformed primary-key binder
+ //! lists, and missing body delimiters.As per coding guidelines, "Every Rust module must begin with a module-level //! comment explaining its purpose and utility." 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR HEAD ==='
git show -s --format='%H%n%s' HEAD
echo
echo '=== Target documentation excerpts ==='
for spec in \
'docs/developers-guide.md:100:145' \
'docs/users-guide.md:105:130' \
'docs/differential-datalog-parser-syntax-spec-updated.md:88:112' \
'docs/differential-datalog-parser-syntax-spec-updated.md:248:278' \
'docs/execplans/2-6-6-resolve-relation-form-coverage.md:38:70'; do
file="${spec%%:*}"
rest="${spec#*:}"
start="${rest%%:*}"
end="${rest##*:}"
echo "--- ${file}, Lines ${start}-${end} ---"
nl -ba "$file" | sed -n "${start},${end}p"
done
echo
echo '=== Relation accessor outline and implementation ==='
ast-grep outline src/parser/ast/relation.rs --items all
nl -ba src/parser/ast/relation.rs | sed -n '85,220p'
echo
echo '=== Inspection test-module documentation ==='
rg -n -C5 'mod tests|Regression tests|BodyInspection|malformed bracket|invalid record|missing body' \
src/parser/ast/relation/inspect.rs
nl -ba src/parser/ast/relation/inspect.rs | sed -n '160,215p'
echo
echo '=== Exact D-REL-008 diagnostic source ==='
rg -n -C4 'D_REL_008|D-REL-008|surrounding' src/parser/span_scanners/relations.rs src/parser
echo
echo '=== Existing body-inspection helper references ==='
rg -n -C3 'inspected_element_type|body_inspection_error|inspect_body' \
src/parser/ast/relation.rs src/parser/ast/relation/inspect.rsLength of output: 1971 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_lines() {
file="$1"
start="$2"
end="$3"
echo "--- ${file}, Lines ${start}-${end} ---"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo '=== PR HEAD ==='
git show -s --format='%H%n%s' HEAD
echo
echo '=== Target documentation excerpts ==='
show_lines docs/developers-guide.md 100 145
show_lines docs/users-guide.md 105 130
show_lines docs/differential-datalog-parser-syntax-spec-updated.md 88 112
show_lines docs/differential-datalog-parser-syntax-spec-updated.md 248 278
show_lines docs/execplans/2-6-6-resolve-relation-form-coverage.md 38 70
echo
echo '=== Relation accessor implementation ==='
show_lines src/parser/ast/relation.rs 85 220
echo
echo '=== Inspection test-module documentation ==='
rg -n -C5 'mod tests|Regression tests|BodyInspection|malformed bracket|invalid record|missing body' \
src/parser/ast/relation/inspect.rs || true
show_lines src/parser/ast/relation/inspect.rs 160 215
echo
echo '=== Exact D-REL-008 diagnostic source ==='
rg -n -C4 'D_REL_008|D-REL-008|surrounding' src/parser/span_scanners/relations.rs src/parser || true
echo
echo '=== Existing body-inspection helper references ==='
rg -n -C3 'inspected_element_type|body_inspection_error|inspect_body' \
src/parser/ast/relation.rs src/parser/ast/relation/inspect.rs || trueLength of output: 40005
Mark these review comments resolved:
No further code changes, AI-agent prompt, suppression message, or follow-up issue is required. Static inspection is complete; retain the repository quality gates as the final runtime validation. |
Refine the developer guide's parser-module and boundary-rule coverage of the relation span scanner so relations.rs (candidate discovery, orchestration, body/primary-key parsing, disambiguation, span collection, recovery), relations/cursor.rs (cursor/trivia/balanced-block mechanics), and relations/preamble.rs (role/kind preamble + D-REL-001..003) each state their ownership, and note the scanner produces spans that ast/relation.rs then exposes as typed accessors. Expand the contributor-workflow section: the required pre-commit sequence (make fmt, check-fmt, lint, test) matching AGENTS.md, and how make lint, make spelling, and make markdownlint relate to the spelling gate, without duplicating the command implementations. Docs only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
117-131: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPre-commit gate contract disagrees between
AGENTS.mdanddocs/developers-guide.md. One root cause: the two files state different rules for whenmake markdownlint/make nixieare required, anddocs/developers-guide.mdexplicitly (and wrongly) claims its shorter list matchesAGENTS.md.
docs/developers-guide.md#L117-L131: Either drop the "matching AGENTS.md" claim or extend the numbered list to the same six unconditional gates AGENTS.md documents; do not presentmake markdownlint/make nixieas merely conditional if AGENTS.md requires them unconditionally.AGENTS.md#L137-L140: If the conditional wording indocs/developers-guide.md(gate on Markdown/Mermaid changes) is the intended policy, update this list to match rather than listing all six gates unconditionally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/developers-guide.md` around lines 117 - 131, Align the pre-commit gate contract between docs/developers-guide.md lines 117-131 and AGENTS.md lines 137-140: choose one policy for make markdownlint and make nixie, then update both files consistently. If the conditional Markdown/Mermaid policy is intended, remove the “matching AGENTS.md” claim and revise AGENTS.md’s unconditional list; otherwise extend the developers guide’s numbered sequence to include both gates unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 117-131: Align the pre-commit gate contract between
docs/developers-guide.md lines 117-131 and AGENTS.md lines 137-140: choose one
policy for make markdownlint and make nixie, then update both files
consistently. If the conditional Markdown/Mermaid policy is intended, remove the
“matching AGENTS.md” claim and revise AGENTS.md’s unconditional list; otherwise
extend the developers guide’s numbered sequence to include both gates
unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b30cd730-e112-4b5d-b8e4-a6e51c978232
📒 Files selected for processing (8)
AGENTS.mddocs/developers-guide.mddocs/differential-datalog-parser-syntax-spec-updated.mddocs/execplans/2-6-6-resolve-relation-form-coverage.mddocs/users-guide.mdsrc/parser/ast/relation.rssrc/parser/ast/relation/inspect.rssrc/parser/ast/relation/tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)
The developer guide claimed its four-item pre-commit sequence matched AGENTS.md while presenting make markdownlint / make nixie as conditional, but AGENTS.md lists all six gates unconditionally. Extend the numbered sequence to fmt, check-fmt, lint, test, markdownlint, nixie and drop the conditional Markdown/Mermaid framing so the two files agree. Docs only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 130-132: Update the documentation sentence so the independent
clauses are separated by a comma: use “Markdown sources, and `make nixie`
validates...” while preserving the existing meaning and command references.
🪄 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 Plus
Run ID: dec60103-2fe1-494f-8ab1-3c90f50b0c00
📒 Files selected for processing (1)
docs/developers-guide.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)
Add the missing comma before "and" joining the two independent clauses describing make markdownlint and make nixie. Docs only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
Summary
docs/execplans/2-6-6-resolve-relation-form-coverage.md— anExecPlan for closing roadmap item
2.6.6and conformance-registeritem
13by widening the relation scanner and typed AST to coverthe full
Role × Kind × {paren, bracket}surface defined indocs/differential-datalog-parser-syntax-spec-updated.md§5.5.AGENTS.mdto the upstream version from the memoryd1-1-1-record-evidence-store-engine-and-migration-policybranch.Plan highlights
The plan was drafted with a planning subagent, validated against the
canonical archived upstream
(
vmware-archive/differential-datalog, Parse.hs, last touched2022-07-23) via firecrawl, then revised after a Logisphere
pre-implementation design review. Key decisions captured:
RelationRole { Input | Output | Internal }andRelationKind { Relation | Stream | Multiset },each paired with a
*_keyword_present()predicate. Replaces theearlier asymmetric
Option<RelationRole>proposal.RelationBodyenum (Fields | ElementType) for the paren/bracketbody forms, with
element_type()returningOption<&str>to honourthe centralized Span→TextRange helper.
5: drop
internalkeyword, correctPrimaryKeyshape, dropmandatory
;, add&ref form, cross-reference attribute prefixsupport.
lambda-style primary-key expressions before any scanner refactor
lands; typed access deferred to follow-up
2.6.6.1.docs) so partial reviewer pushback does not bounce the rest.
proptestover the role × kind × body × ref × pk space is requiredin Milestone 3, not optional.
D-REL-001..D-REL-008) with per-diagnosticrecovery contracts and 23 parameterised
rstestmatrix cases plus4 behavioural cases.
Test plan
make markdownlinton the new ExecPlan andAGENTS.md.spec-delta reconciliation strategy.
criteria.
lambda primary keys (follow-up
2.6.6.1).🤖 Generated with Claude Code
Summary by Sourcery
Document the execution plan and contributor guidelines for expanding relation form coverage and parser alignment work, without implementing any parser changes yet.
Documentation:
References
docs/roadmap.md§2.6.6.docs/parser-conformance-register.mditem 13.docs/execplans/2-6-6-resolve-relation-form-coverage.md.