refactor(artifact-ref): dedupe validators, split registered_contract, flatten conditionals - #209
Conversation
…o content_contract artifact_ref.rs, dag_coordinator.rs, run_commit.rs, and session_evidence.rs each reimplemented valid_digest (byte-identical to content_contract::is_digest) and, in dag_coordinator.rs, the entire sha256_hex algorithm a second time. run_commit.rs also duplicated valid_run_identity, which had no shared home at all. - add is_run_identity to content_contract.rs as the canonical impl - capability.rs re-exports it alongside the existing is_digest/require_exact_keys - artifact_ref.rs, run_commit.rs, session_evidence.rs alias-import instead of reimplementing; exact_object_keys becomes a thin wrapper over require_exact_keys (error text preserved via map_err so no test assertions change) - dag_coordinator.rs gets its own #[path] content_contract leaf module (matches the existing artifact_ref.rs/capability.rs convention) rather than a new dependency edge on capability.rs, since it had none before Part of the artifact_ref.rs health-score remediation (repo's worst defect score, 1.85/10, co-changes with 26 files). repin --write resynced the two orchestration/ registry digest pins that track these files' content hashes.
…olicy expected_understanding_quadrant (artifact_ref.rs) and classify (understanding_quadrant.rs) implemented the identical criticality/confidence -> quadrant-label policy twice, with the same 50/50 thresholds. understanding_ quadrant.rs already depended on artifact_ref.rs (VerifiedArtifact), so this consolidates in that existing direction: artifact_ref.rs's version becomes pub(crate), understanding_quadrant.rs aliases it back to the local name `classify` so none of its call sites (including tests) change. Tried the same treatment for method_catalog.rs's CARD_FIELDS constant and reverted it: crates/code-intel-cli/tests/*.rs each declare their own curated #[path = "../src/X.rs"] module set per integration-test binary, and most binaries that include artifact_ref.rs don't also include method_catalog.rs. A crate::method_catalog:: reference from artifact_ref.rs broke ~9 test binaries with E0433. This is the actual reason artifact_ref.rs/capability.rs duplicate content_contract.rs via #[path] instead of sharing it normally: content_contract has zero internal deps so it's safe to redeclare per file, but feature modules like method_catalog/understanding_quadrant are not universally wired into every test binary's module tree.
…amily registered_contract was a single 282-line function (CCN 36, cognitive complexity 71) matching artifactSchema+type tuples against ~33 known ArtifactContract entries plus a native-code fallback. Split into six private *_family_contract helpers (repository/diagnosis/orientation/ retirement/run-delivery/method-decision), chained with .or_else(), plus the native-code fallback and a single unregistered_contract_error() helper for the two identical error-message call sites. Every arm's schema/type/max_bytes/validate_payload is unchanged -- this regroups match arms into smaller functions, it doesn't touch validation behavior. Deliberately kept as private fns in the same file rather than separate modules: crates/code-intel-cli/tests/*.rs each hand-wire their own #[path] module set per binary (see the code-intel-test-harness- module-wiring note from the previous commit), and new file-based submodules split out of artifact_ref.rs would hit the same E0433 breakage across the ~9 test binaries that include artifact_ref.rs without also declaring the new module.
…onals Flattens the 6-12-boolean-operator if-conditions repowise flagged across artifact_ref.rs's validators into named `*_is_valid` predicate helpers, applying De Morgan's laws to turn each err-triggering OR-chain into a positive AND-chain: sentrux_command_result_is_valid, retirement_ticket_template_header_is_valid, surgery_plan_shape_is_valid, project_orientation_shape_is_valid, understanding_quadrant_identity_is_valid, orientation_benchmark_observations_header_is_valid, orientation_benchmark_report_is_valid, repository_iteration_provenance_is_valid, run_timing_telemetry_policy_is_valid, run_timing_event_is_valid, light_speed_report_is_valid, path_syntax_is_portable, path_component_is_unambiguous. Pure extraction, no behavior change -- every clause and its logical negation was carried over 1:1. bin suite still 629/629, full test suite 3623/3623.
…ated run_commit.rs is named verbatim in the frozen source sets of two retirement-packet verifiers (Test-HospitalRetirementPacket.ps1's E08, Test-IndexRetirementPacket.ps1's E10). Repinning is unrelated to the retirement subject matter, but the packets hash the file byte-for-byte, so participating this file in the digest/run-identity dedup (commit 77521a9) flipped both packets to "stale relative to its frozen source set" even though nothing about hospital diagnosis or the committed-only index changed. Restored the two local fn bodies verbatim from 77521a9's parent and dropped the `is_digest as valid_digest, is_run_identity as valid_run_identity` import aliases, so run_commit.rs is now byte-identical to its pre-refactor version. Every other file from the dedup commit (content_contract.rs, capability.rs, artifact_ref.rs, dag_coordinator.rs, session_evidence.rs, understanding_quadrant.rs) keeps the shared helpers -- this file is the one deliberate exception. Verified: full suite still 3623/3623, and legacy/scripts/tests/test-retirement-packets.ps1 now passes clean (8 packets, 2 audits, 0 known-blocked -- E08 and E10 both PASS).
|
✅ Health of changed files: 4.3 (unchanged) 📋 At a glance ✅ Health gate: passed 🔎 More signals (2)🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (5 with dependents)"]
f_crates_code_intel_cli_src_artifact_ref_rs[".../src/artifact_ref.rs 🔥"]:::changed
f_crates_code_intel_cli_src_capability_rs[".../src/capability.rs 🔥"]:::changed
f_crates_code_intel_cli_src_dag_coordinator_rs[".../src/dag_coordinator.rs 🔥"]:::changed
f_crates_code_intel_cli_src_session_evidence_rs[".../src/session_evidence.rs 🔥"]:::changed
f_crates_code_intel_cli_src_understanding_quadrant_rs[".../src/understanding_quadrant.rs"]:::changed
end
f_crates_code_intel_cli_src_admissibility_rs[".../src/admissibility.rs"]
f_crates_code_intel_cli_src_artifact_ref_rs --> f_crates_code_intel_cli_src_admissibility_rs
f_crates_code_intel_cli_src_assistance_adapter_rs[".../src/assistance_adapter.rs"]
f_crates_code_intel_cli_src_artifact_ref_rs --> f_crates_code_intel_cli_src_assistance_adapter_rs
f_crates_code_intel_cli_src_builtin_provider_evidence_rs[".../src/builtin_provider_evidence.rs"]
f_crates_code_intel_cli_src_artifact_ref_rs --> f_crates_code_intel_cli_src_builtin_provider_evidence_rs
f_crates_code_intel_cli_src_capability_inventory_rs[".../src/capability_inventory.rs"]
f_crates_code_intel_cli_src_artifact_ref_rs --> f_crates_code_intel_cli_src_capability_inventory_rs
f_crates_code_intel_cli_src_capability_rs --> f_crates_code_intel_cli_src_admissibility_rs
f_crates_code_intel_cli_src_artifact_index_rs[".../src/artifact_index.rs"]
f_crates_code_intel_cli_src_capability_rs --> f_crates_code_intel_cli_src_artifact_index_rs
f_crates_code_intel_cli_src_capability_rs --> f_crates_code_intel_cli_src_builtin_provider_evidence_rs
f_crates_code_intel_cli_src_compatibility_retirement_gate_rs[".../src/compatibility_retirement_gate.rs"]
f_crates_code_intel_cli_src_capability_rs --> f_crates_code_intel_cli_src_compatibility_retirement_gate_rs
f_crates_code_intel_cli_src_authoritative_run_execution_kernel_rs[".../authoritative_run/execution_kernel.rs"]
f_crates_code_intel_cli_src_dag_coordinator_rs --> f_crates_code_intel_cli_src_authoritative_run_execution_kernel_rs
f_crates_code_intel_cli_src_dag_run_rs[".../src/dag_run.rs"]
f_crates_code_intel_cli_src_dag_coordinator_rs --> f_crates_code_intel_cli_src_dag_run_rs
f_crates_code_intel_cli_src_main_rs[".../src/main.rs"]
f_crates_code_intel_cli_src_dag_coordinator_rs --> f_crates_code_intel_cli_src_main_rs
f_crates_code_intel_cli_src_session_evidence_rs --> f_crates_code_intel_cli_src_main_rs
f_crates_code_intel_cli_src_understanding_quadrant_rs --> f_crates_code_intel_cli_src_capability_inventory_rs
more(["+16 more dependents"])
PR --> more
classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Solid arrows: code that imports the changed files (25 direct dependents, from the last indexed snapshot). Dashed: history/tests. 🔥 Hotspots touched (5)
2 more
📊 See the full report for this PR |
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request centralizes digest and run-identity validation, reorganizes artifact contract lookup, extracts focused validators, reuses quadrant classification, and updates orchestration integrity records. ChangesContract validation refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
CI's cargo fmt -p code-intel -- --check caught unformatted diffs from the refactor commits (wrapped boolean chains, split match arms, require_exact_keys call, one stray blank line). No semantic change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/code-intel-cli/src/artifact_ref.rs (3)
1387-1413: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRecord the array invariant that the
unwrap()at line 1387 depends on.
project_orientation_shape_is_validis now the only guard for theas_array().unwrap()at line 1387. I confirmed the extraction is safe today: the helper proves all seven unwrapped fields are arrays at lines 1399-1408, and line 1367 returns early on failure.The coupling is implicit. If a later edit removes one array check from the helper, line 1387 panics on untrusted artifact input. Replace the
unwrap()with anexpectthat names the guarantee, so the dependency is visible at both sites.♻️ Proposed change to record the invariant
- for (index, claim) in value[field].as_array().unwrap().iter().enumerate() { + for (index, claim) in value[field] + .as_array() + .expect("project_orientation_shape_is_valid proved this field is an array") + .iter() + .enumerate() + {🤖 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 `@crates/code-intel-cli/src/artifact_ref.rs` around lines 1387 - 1413, Replace the `as_array().unwrap()` in the claim-validation loop with an `expect` message that explicitly states `project_orientation_shape_is_valid` guarantees the field is an array. Preserve the existing validation flow and error propagation while making this invariant visible at the unwrap site.
3097-3115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe path predicates preserve traversal safety. Consider documenting the second path validator.
I traced the Artifact Ref path flow.
path_syntax_is_portabledoes not reject.or..itself, butportable_relative_pathrejectsComponent::CurDirandComponent::ParentDirat lines 3073-3080 before the join at line 3094. Absolute paths and Windows drive prefixes are rejected twice. No traversal sequence survives.Two small points:
- Line 3102 is redundant.
!value.starts_with('/')already excludes any value that starts with//. Keeping both conditions is acceptable for a behavior-preserving extraction.- This file now holds two different portable-path rule sets:
path_syntax_is_portablehere, andvalidate_portable_pathat lines 1155-1169 for retirement paths. They enforce different rules.validate_portable_pathrejects a trailing/and rejects.and..components directly, and it does not test for NUL. A reader may assume the two are interchangeable. Add a short comment on each that names its call site and states why the rules differ.🤖 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 `@crates/code-intel-cli/src/artifact_ref.rs` around lines 3097 - 3115, Document both portable-path validators: add concise comments above path_syntax_is_portable and validate_portable_path naming their respective call sites and clarifying why their rule sets differ. Preserve the existing validation behavior, including the redundant starts_with("//") check.
198-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe decomposition preserves the contract lookup semantics.
The let-else at lines 199-204 reproduces the previous behavior: a missing or non-string
artifactSchemaortypenow returns the same unregistered-contract error that the old catch-all arm produced. Theor_elsechain is lazy, and the family schema sets are disjoint, so the chain order cannot change which contract is selected. Every family helper ends in_ => None, and line 221 ends inok_or_else(unregistered_contract_error), so no unmatched pair can reach a permissive default.One optional follow-up: each arm still repeats its schema and type literal twice, once in the match pattern and once in the returned
ArtifactContractfields. A static table of(schema, type, max_bytes, validate_payload)tuples per family would remove that duplication and make the echo automatic. The PR defers registry redesign, so this can wait for that separate work.🤖 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 `@crates/code-intel-cli/src/artifact_ref.rs` around lines 198 - 228, The review identifies only an optional future refactor: leave registered_contract and the current family helper lookup behavior unchanged. Do not introduce a static registry or modify the contract-selection semantics in this change.crates/code-intel-cli/src/dag_coordinator.rs (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the repeated
#[path]module import indag_coordinator.rs.
content_contract.rsis also included bycapability.rs,artifact_ref.rs,audit_report/mod.rs, and integration tests, so this#[path]import serves a live consumer. Add a short comment above the import noting the parallel test/consumer import pattern to avoid future duplicate removal.🤖 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 `@crates/code-intel-cli/src/dag_coordinator.rs` around lines 7 - 9, add a concise comment directly above the #[path = "content_contract.rs"] module declaration in dag_coordinator.rs, noting that content_contract.rs is intentionally imported here alongside parallel consumer and test imports. Leave the module declaration and aliased imports unchanged.Source: Coding guidelines
🤖 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 `@crates/code-intel-cli/src/content_contract.rs`:
- Around line 210-218: Add focused unit tests for is_run_identity covering the
required dag-v1: prefix, rejecting an empty tail, requiring an even-length tail,
and accepting only lowercase hexadecimal characters. Include valid and invalid
cases for each rule while preserving the existing implementation.
---
Nitpick comments:
In `@crates/code-intel-cli/src/artifact_ref.rs`:
- Around line 1387-1413: Replace the `as_array().unwrap()` in the
claim-validation loop with an `expect` message that explicitly states
`project_orientation_shape_is_valid` guarantees the field is an array. Preserve
the existing validation flow and error propagation while making this invariant
visible at the unwrap site.
- Around line 3097-3115: Document both portable-path validators: add concise
comments above path_syntax_is_portable and validate_portable_path naming their
respective call sites and clarifying why their rule sets differ. Preserve the
existing validation behavior, including the redundant starts_with("//") check.
- Around line 198-228: The review identifies only an optional future refactor:
leave registered_contract and the current family helper lookup behavior
unchanged. Do not introduce a static registry or modify the contract-selection
semantics in this change.
In `@crates/code-intel-cli/src/dag_coordinator.rs`:
- Around line 7-9: add a concise comment directly above the #[path =
"content_contract.rs"] module declaration in dag_coordinator.rs, noting that
content_contract.rs is intentionally imported here alongside parallel consumer
and test imports. Leave the module declaration and aliased imports unchanged.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: c4c2013d-8109-4622-88bb-ae1e8b947607
📒 Files selected for processing (8)
crates/code-intel-cli/src/artifact_ref.rscrates/code-intel-cli/src/capability.rscrates/code-intel-cli/src/content_contract.rscrates/code-intel-cli/src/dag_coordinator.rscrates/code-intel-cli/src/session_evidence.rscrates/code-intel-cli/src/understanding_quadrant.rsorchestration/integrations.jsonorchestration/internalization/mindwalk.json
- content_contract.rs: add focused unit tests for is_run_identity (dag-v1: prefix, empty tail, even-length tail, lowercase-hex-only) - artifact_ref.rs: replace as_array().unwrap() in the project-orientation claim loop with an expect() naming the invariant (project_orientation_shape_is_valid proves it) - artifact_ref.rs: document why path_syntax_is_portable and validate_portable_path are two separate rule sets, not duplicates - dag_coordinator.rs: comment the repeated content_contract.rs #[path] import, pointing at the test-harness-module-wiring constraint Left the registered_contract static-table suggestion alone -- reviewer's own text scopes that to the registry-redesign work tracked in #206. 3771/3771 tests pass (up from 3623: the new content_contract tests run once per test binary that #[path]-includes the module).
…Rabbit fixes The atomic-capability-contract test (windows-build-test-package CI check) compares each integration's declared toolchainDigests against a live SHA-256 of its toolchainDigestEvidence inputs. artifact_ref.rs changed in the last two commits (cargo fmt fixups, then the unwrap->expect and path-validator doc comments from the CodeRabbit review) without a resync -- code-intel repin --write doesn't cover this field, it's a separate hand-maintained pin verified only by legacy/scripts/tests/test-atomic-capability-contract.ps1. Recomputed the file's SHA-256 and updated all three integrations that declare it as toolchain evidence: delivery.light-speed-measure, compatibility.retirement-gate, compatibility.retirement-ticket-template. Verified: test-atomic-capability-contract.ps1 now reports ok:true, full suite 3771/3771, test-retirement-packets.ps1 8 packets + 2 audits clean.
Code Intel change risk
Top signals
revspec: |
1 participant
What
Remediates
artifact_ref.rs, the repo's worst-health file (repowiseget_healthworst score: 1.85/10), via the four safe steps identified in a health-scan-driven audit:valid_digest/valid_run_identity/sha256_hex/exact_object_keys-style helpers into the existing leaf modulecontent_contract.rs, re-exported throughcapability.rs, and consumed via#[path]/alias-import fromartifact_ref.rs,dag_coordinator.rs,session_evidence.rs,run_commit.rs(later reverted, see below), andunderstanding_quadrant.rs(sharesexpected_understanding_quadrantclassification policy instead of re-implementing it).registered_contract(CCN 36, 282 lines, one bigartifactSchema+typematch) into six private*_family_contracthelpers (repository/diagnosis/orientation/retirement/run-delivery/method-decision) chained with.or_else(), plus a sharedunregistered_contract_error()for the two identical error-message call sites. Every arm'sschema/type/max_bytes/validate_payloadis unchanged — this only regroups match arms, it doesn't touch validation behavior.*_is_validpredicate helpers via De Morgan's law (eachif (A) || (B) || (C) { return Err(...) }OR-chain becomesif !xyz_is_valid(...) { return Err(...) }with an equivalent AND-chain) — same logic, readable names, zero behavior change.run_commit.rs's participation in step 1 (this commit).run_commit.rsis named verbatim in the frozen source sets of two retirement-packet verifiers (Test-HospitalRetirementPacket.ps1's E08,Test-IndexRetirementPacket.ps1's E10), which hash it byte-for-byte as part of proving those packets' evidence hasn't drifted. Deduping its two validators flipped both packets to "stale relative to its frozen source set" even though nothing about hospital diagnosis or the committed-only index actually changed. Restored the file byte-identical to its pre-refactor version (verified viagit show 77521a9 -- crates/code-intel-cli/src/run_commit.rs); every other file from step 1 keeps the shared helpers.artifact_ref.rsgoes from ~3597 lines to ~3450.Why
Health scan flagged
artifact_ref.rsas the lowest-scoring file in the repo (1.85/10), driven by a large duplicate-validator footprint, one very high-CCN dispatch function, and a cluster of complex boolean conditionals. This branch fixes the three items that were safe to fix mechanically (pure extraction/dedup, no behavior change, each step build+test-verified independently).A fourth finding —
orchestration/integrations.jsonandsentrux.jsonhand-maintaining a parallel contract registry with no sync mechanism againstartifact_ref.rs'sregistered_contract— was not touched here; it needs a real design decision, not a mechanical fix, and is tracked separately: #206.Also filed #207 documenting repowise
dry_violationfindings from this pass that are not real duplicates (convergent shape, divergent semantics) — flagged so a future dedup pass doesn't merge them incorrectly.Verification (this branch, HEAD)
cargo test -p code-intel: 3623/3623 passingcode-intel run execute ... --manifest orchestration/integrations.json): exit 0,outcome: completed, zero domain/process failures, sentrux ratchet passlegacy/scripts/tests/test-retirement-packets.ps1: 8 packets + 2 audits pass, 0 known-blocked (confirmed E08/E10 both PASS after the run_commit.rs revert; both were red before it, root-caused via a clean-baseline worktree comparison againsta9baf61)Reviewer notes
run_commit.rsstill has its own localvalid_digest/valid_run_identitycopies, deliberately, not an oversight — see commit 776296c's message. Please don't "clean this up" again without regenerating the E08/E10 retirement packets first (legacy/tools/compatibility/New-HospitalRetirementPacket.ps1/New-IndexRetirementPacket.ps1), since regeneration re-executes livecargo testfixtures and isn't purely mechanical.method_catalog::CARD_FIELDSfromartifact_ref.rswas reverted —crates/code-intel-cli/tests/*.rsintegration binaries each hand-wire their own#[path]module set, and most binaries wiring inartifact_ref.rsdon't also wire inmethod_catalog.rs, so it broke 9 test binaries with E0433. Kept the inline 17-field list instead; documented as a reusable gotcha.