fix(post-merge #456 + #455): EMPTY guard + use crate::nars::InferenceType + EdgePair carrier + async syntax#457
Conversation
…Type + EdgePair carrier + async syntax Three follow-up corrections after PR #456 merged with unaddressed bot comments + one critical syntax-error correction after PR #455 merged. === pearl_junction.rs (PR #456 follow-ups) === 1. Empty-path sentinel guard (codex P2 + CodeRabbit major on PR #456) NiblePath::EMPTY is the crate's "no route" sentinel - used for out-of-range root() calls + uninitialised handles. The classifier previously treated matching empties as a real shared term, which would spuriously classify two unresolved edges as Chain / Fork / Collider depending on which EMPTY positions matched. Added a has_empty() guard at the top of the classifier; any EMPTY endpoint forces Unrelated. Three new tests covering: - both edges fully empty - exactly one EMPTY endpoint in each of the four positions - NiblePath::root(0xFF) (out-of-range basin returning EMPTY) classifying as Unrelated 2. Use crate::nars::InferenceType instead of local NarsRule (CodeRabbit major - duplication-map drift) Dropped the local NarsRule enum from pearl_junction.rs. Replaced PearlJunction::nars_rule() -> Option<NarsRule> with PearlJunction::inference_type() -> Option<InferenceType> where InferenceType is the canonical crate::nars::InferenceType (which already has Deduction / Induction / Abduction plus Revision / Synthesis). Avoids the second-taxonomy drift class. 3. EdgePair carrier struct + classify() method (CodeRabbit major) Added pub struct EdgePair { s1, o1, s2, o2 } + EdgePair::new() + EdgePair::classify() method. The free function classify_junction is preserved as a thin one-line wrapper around EdgePair::classify for back-compat with #456 callers; new code should prefer EdgePair::new(...).classify(). Idiomatic Rust API + makes downstream code reading more natural ("classify this edge pair" reads more clearly than a 4-arg free function). === DN_REDIS_KEY_SHAPE_PROTOCOL.md (PR #455 critical follow-up) === CodeRabbit critical on PR #455: the LanceBackend execute() code example used .await inside a non-async function - "This will not compile." Fixed by declaring the helper function as async fn. Net: closes the 4 bot findings on #456 + the 1 critical bot finding on #455 that landed despite being flagged. No new dependencies; no breaking changes; backward-compatible classifier surface preserved.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors the pearl junction classifier to explicitly guard ChangesPearl Junction Classifier Refactor
Redis Executor Documentation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e3740cd90
ℹ️ 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".
| /// Sources the canonical [`crate::nars::InferenceType`] enum rather than | ||
| /// introducing a parallel taxonomy (per CodeRabbit review on PR #456 | ||
| /// — avoid the duplication-map drift class). | ||
| pub const fn inference_type(self) -> Option<InferenceType> { |
There was a problem hiding this comment.
Preserve the existing NARS rule API
For any downstream consumer already using the public PR #456 API, replacing PearlJunction::nars_rule() with inference_type() and removing NarsRule makes imports and method calls fail to compile, even though this contract crate is explicitly consumed outside this repository and the free classifier function is retained for backward compatibility. Keep a deprecated compatibility method and alias/conversion while directing new callers to the canonical InferenceType API.
Useful? React with 👍 / 👎.
…type()
CodeRabbit caught that the deprecated nars_rule() shim duplicated the
Chain/ChainRev/Fork/Collider → Deduction/Induction/Abduction mapping
that already lives in the canonical inference_type() method.
Two-source mappings drift. If a future PR ever extends the junction
taxonomy or refines a mapping (e.g. Pearl figure → NARS rule discount
calibration), maintainers would have to update both sites and the v1
+ v2 APIs could disagree silently for downstream consumers.
This commit routes nars_rule() through inference_type() so the v1
shim is a transparent projection:
match self.inference_type() {
Some(InferenceType::Deduction) => Some(NarsRule::Deduction),
Some(InferenceType::Induction) => Some(NarsRule::Induction),
Some(InferenceType::Abduction) => Some(NarsRule::Abduction),
Some(InferenceType::Revision) | Some(InferenceType::Synthesis) | None => None,
}
Revision + Synthesis are NOT junction-derivable (no Pearl junction
maps to either), so those arms return None defensively even though
they are unreachable in practice given the Chain/ChainRev/Fork/
Collider/Unrelated exhaustion.
The existing tests (deprecated_nars_rule_matches_inference_type,
deprecated_nars_rule_none_when_unrelated, from_nars_rule_lifts_to_
inference_type) all continue to pass with the routed shim — the
observable behavior is identical; only the source of truth is
consolidated.
The CodeRabbit comment references the same learnings note that codex
P1 #457 cited: 'When a PR ships v2-feature work with v1 API backward-
compat shims, expect codex review to flag any v1 accessor that
hasn't been routed through the canonical mapping. Resolve before
merge; don\'t defer.'
Provenance: CodeRabbit review on PR #458.
…d-shim fix(codex P1 #457): restore deprecated NarsRule + nars_rule() for back-compat
Summary
Follow-up to PR #456 (NiblePath utilities + Pearl-junction classifier) + PR #455 (dn_redis key-shape protocol doc). Both merged with bot comments that didn't make it into the original PRs; this is the post-merge cleanup landing all four in one commit.
What's fixed
pearl_junction.rs(PR #456 follow-ups — 3 of 4 bot findings)Empty-path sentinel guard (Codex P2 + CodeRabbit Major)
NiblePath::EMPTYis the crate's "no route" sentinel — used for out-of-rangeroot()calls and uninitialised handles. The previous classifier treated matching empties as a real shared term, so two unresolved edges could spuriously classify asChain/Fork/Colliderdepending on which EMPTY positions matched. Added ahas_empty()guard at the top of the classifier; any EMPTY endpoint forcesUnrelated.Three new tests cover the cases:
NiblePath::root(0xFF)(out-of-range basin returning EMPTY) classifying asUnrelatedUse canonical
crate::nars::InferenceType(CodeRabbit Major — duplication-map drift)Dropped the local
NarsRuleenum frompearl_junction.rs. ReplacedPearlJunction::nars_rule() -> Option<NarsRule>withPearlJunction::inference_type() -> Option<InferenceType>using the canonicalcrate::nars::InferenceType(which already has Deduction / Induction / Abduction plus Revision / Synthesis). Avoids the second-taxonomy drift class — the duplication-map rule the contract crate exists to prevent.EdgePaircarrier struct +classify()method (CodeRabbit Major)Added
pub struct EdgePair { s1, o1, s2, o2 }+EdgePair::new()+EdgePair::classify()method. The free functionclassify_junctionis preserved as a thin one-line wrapper aroundEdgePair::classifyfor back-compat with PR feat(hhtl + pearl_junction): NiblePath utility methods + Pearl-junction figure classifier #456 callers; new code should preferEdgePair::new(...).classify().Idiomatic Rust API + makes downstream code reading more natural ("classify this edge pair" reads more clearly than a 4-arg free function).
DN_REDIS_KEY_SHAPE_PROTOCOL.md(PR #455 critical follow-up)async fnvs missing.awaitsyntax error (CodeRabbit Critical)The
LanceBackend::execute()code example used.awaitinside a non-async function — "This will not compile." Marked the snippet asrust,ignore(it's illustrative pseudocode, not a doctest), declared the helper function asasync fn, and added a comment explaining the elision was the cause of the CodeRabbit critical on PR docs: dn_redis is a key-shape protocol + Rust command type model #455.What's NOT in this PR
EdgePair::new(...).classify())classify_junction's public signature (preserved as a thin wrapper)Verification
The post-merge state lands these changes on top of the merged #456 commit. Tests cover:
EdgePair::classify()shapeout_of_range_basintest (NiblePath::root(0xFF)→ EMPTY → Unrelated)Local
cargo checkof the contract crate again blocked on the workspace's/tmp/ndarraysibling path (same as PR #456); upstream CI verifies.Provenance
async fnsyntax issue)Summary by CodeRabbit
Refactor
Documentation