refactor: one home for the passport obligation test - #366
Conversation
|
@coderabbitai review Two things to push on:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes live passport-obligation checks through ChangesPassport obligation consolidation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The consolidation does not leave an identified behavior or integration risk requiring changes before merge. 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
git diff --unified=80 HEAD^ HEAD -- \
CLAUDE.md \
crates/dpp-node/src/plugins.rs \
crates/dpp-plugin-host/src/host.rs \
crates/dpp-vault/src/domain/service/mod.rs \
dpp-integrator/src/handlers/product_groups.rs || true
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== Renderer context ==='
renderer=$(fd -a -t f 'product_groups.rs' . | rg 'dpp-integrator/src/handlers/product_groups\.rs$' | head -n1 || true)
if [ -n "$renderer" ]; then
nl -ba "$renderer" | sed -n '230,300p'
fi
echo '=== Plugin host outline and predicate context ==='
host=$(fd -a -t f 'host.rs' . | rg 'crates/dpp-plugin-host/src/host\.rs$' | head -n1 || true)
if [ -n "$host" ]; then
ast-grep outline "$host" --items all --match 'passport_obligation_live|instruments' || true
rg -n -C 12 '\bpassport_obligation_live\s*\(' "$host" || true
fi
echo '=== Vault service outline and predicate context ==='
vault=$(fd -a -t f 'mod.rs' . | rg 'crates/dpp-vault/src/domain/service/mod\.rs$' | head -n1 || true)
if [ -n "$vault" ]; then
ast-grep outline "$vault" --items all --match 'passport_obligation_live|instruments' || true
rg -n -C 12 '\bpassport_obligation_live\s*\(' "$vault" || true
fi
echo '=== All core predicate invocations ==='
rg -n -C 4 '\.passport_obligation_live\s*\(' -g '*.rs' . || true
echo '=== Remaining conjunction candidates ==='
rg -n -U -P '(?s)determinable_for\s*\([^)]*\)\s*\.\s*is_empty\s*\(\).*?passport_required_for\s*\(' -g '*.rs' . || trueLength of output: 63773
The wrappers in I also found no remaining source occurrence of the prior hand-written No findings on these points. 🐇 ✏️ Learnings added
|
|
@coderabbitai review (Second ask — the first, with the The two things worth pushing on, repeated from above:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== Shared predicate definition and documentation ==='
rg -n -C 12 '\bpassport_obligation_live\s*\(' -g '*.rs' . || true
echo '=== Renderer context ==='
renderer=$(fd -a -t f 'product_groups.rs' . | rg 'dpp-integrator/src/handlers/product_groups\.rs$' | head -n1 || true)
if [ -n "$renderer" ]; then
nl -ba "$renderer" | sed -n '240,285p'
fi
echo '=== Plugin-host wrapper and call sites ==='
host=$(fd -a -t f 'host.rs' . | rg 'crates/dpp-plugin-host/src/host\.rs$' | head -n1 || true)
if [ -n "$host" ]; then
rg -n -C 10 '\bpassport_obligation_live\s*\(' "$host" || true
fi
echo '=== Vault wrapper and call sites ==='
vault=$(fd -a -t f 'mod.rs' . | rg 'crates/dpp-vault/src/domain/service/mod\.rs$' | head -n1 || true)
if [ -n "$vault" ]; then
rg -n -C 10 '\bpassport_obligation_live\s*\(' "$vault" || true
fi
echo '=== Remaining same-expression candidates ==='
rg -n -U -P '(?s)determinable_for\s*\([^)]*\)\s*\.\s*is_empty\s*\(\).*?(?:&&|and)\s*.*?passport_required_for\s*\(' -g '*.rs' . || true
rg -n -U -P '(?s)passport_required_for\s*\([^)]*\).*?(?:&&|and)\s*.*?determinable_for\s*\([^)]*\)\s*\.\s*is_empty\s*\(' -g '*.rs' . || true
echo '=== Focused diff ==='
git diff --unified=20 HEAD^ HEAD -- \
CLAUDE.md \
crates/dpp-node/src/plugins.rs \
crates/dpp-plugin-host/src/host.rs \
crates/dpp-vault/src/domain/service/mod.rs \
dpp-integrator/src/handlers/product_groups.rs || trueLength of output: 48843
I agree with the reporting decision. I agree with the wrapper decision. I also searched the Rust sources for the former conjunction in both operand orders. I found no remaining candidate. No findings from this review. I used the existing PR-specific guidance for these two decisions. 🐇 🧠 Learnings used
|
ab3f901 to
6f63700
Compare
Closes #208 and closes #213.
One PR rather than two because #213's own next-steps sequence them together — "delete all four copies in the same change so they cannot be half-migrated" — and the rule reads better landing with its first worked example than on its own.
#208 was not blocked, and had not been for a while
Both issues describe this as waiting on a core release. It is not:
InstrumentCatalog::passport_obligation_liveis already indpp-domain0.20.0, which is the version pinned atCargo.toml:55. Its body is character-for-character the conjunction this repo restates:So this needed no core change, no release and no repin. It is a delete against a dependency already in the lock file, and it has been available since 0.20.0 landed.
Only four of six uses are gates
determinable_forhas six call sites. Four are the conjunction and are folded here. The fifth must not be, and core's own documentation draws the line:dpp-integrator'sproduct_groups.rsservesdeterminableas its own field besiderequiredandfrom. Those are different questions — an obligation can exist while the implementing acts that would make it determinable do not — and collapsing them would make a reporting endpoint answer one boolean where it currently answers two honest ones. Left alone deliberately; a find-and-replace across all six would have broken it.The sixth is a comment.
Under one name
passport_determinableindpp-plugin-hostandpassport_obligation_liveindpp-vaultmeant the same thing under different names, which #213 calls out as "its own tax on a reader trying to establish whether the gates agree". Both are nowpassport_obligation_live, and both are one line delegating to core.The two wrappers stay rather than being deleted, and that is not laziness:
dpp-plugin-hostcannot seedpp-types, and each wrapper holds the crate-localinstruments()lookup. Wrapping the call is a seam; restating the&&was the duplication. After this, the rule has exactly one home.The ESPR reasoning those doc comments carried — Arts. 24-25 bind while imposing no passport, and Art. 9(4)(b) discharges the duty through EPREL for the ecodesign/energy-labelling pair — is in core's own doc on the method, so it is a pointer here rather than a third copy of the argument.
#213: the boundary test, recorded
#213 was decided on 2026-09-03 (option 1) and its third step was "record the boundary test above where the next person will look", which never happened.
CLAUDE.mdnow carries it beside the Golden Rule and the Core Purity Rule:With the counter-example in each direction, because "is it about passports" gives the wrong answer both ways:
snapshot_json_keyis about passports and changes when the object-storage layout changes, so it stays; the obligation test is about passports and changes when an implementing act comes into force, so it is core's.And the reason it matters, which is the part #213 is really about: CI can see a duplicated shape and cannot see a duplicated rule.
every_published_object_shape_has_a_namefails the build on a copied struct. N copies of a predicate are N individually correct files that compile, pass, and diverge the first time somebody adds a condition to one. The three instances on record —PROTECTED_PATCH_FIELDSrestated three entries short and making protected fields writable, a query parameter spelled three ways, and this conjunction in four places — are named there so the next person meets them before writing the fourth.Verified
just checkgreen.Behaviour is unchanged by construction — every folded site now calls a function whose body is the expression it replaced.
Summary by CodeRabbit