Repository navigation
Add multidimensional trust and promotion gates - #73
Conversation
📝 WalkthroughWalkthroughThe PR replaces aggregate verification with five trust dimensions, adds immutable evidence-backed transitions and promotion assessments, exposes trust operations through CLI/MCP, and requires release manifest v2 bundles to include hash-bound trust data and assessment artifacts. ChangesTrust and promotion lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CLI_MCP_Verify
participant Application
participant Store
Client->>CLI_MCP_Verify: submit trust_status or transition_trust
CLI_MCP_Verify->>Application: dispatch validated request
Application->>Store: read evidence or persist transition
Store-->>Application: trust history or transition snapshot
Application-->>CLI_MCP_Verify: canonical trust response
CLI_MCP_Verify-->>Client: structured result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d736c97cd
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
migrations/0014_multidimensional_trust.sql (1)
39-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winColumn/JSON agreement relies on byte-identical array rendering.
json_extract(request_json, '$.evidence_artifact_hashes')returns SQLite's own minified JSON text, which is compared as text againstevidence_artifact_hashes_jsonproduced byserde_json::to_stringinsrc/store/mod.rs. They agree today only because both emit space-free arrays; any formatting change on either side turns a valid insert into an opaque CHECK failure. Normalizing both sides removes the coupling.♻️ Compare normalized JSON on both sides
AND json_extract(request_json, '$.evidence_artifact_hashes') - IS evidence_artifact_hashes_json + IS json_extract(evidence_artifact_hashes_json, '$')🤖 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 `@migrations/0014_multidimensional_trust.sql` around lines 39 - 40, Update the CHECK constraint comparing evidence_artifact_hashes_json with request_json to compare normalized JSON values rather than raw serialized text, and ensure the value produced in src/store/mod.rs is normalized consistently before storage. Preserve validation of equivalent arrays regardless of whitespace or formatting differences.src/domain/trust.rs (2)
1095-1111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest helper breaks for
id_suffix >= 16.
format!("{id_suffix:x}").repeat(64)yields 128 characters once the suffix needs two hex digits, sodecision_hashstops being a valid 64-char hash. Current callers pass 1 and 2, but a future case would fail confusingly.♻️ Pad to a single-width nibble
- decision_hash: format!("{id_suffix:x}").repeat(64), + decision_hash: format!("{id_suffix:02x}").repeat(32),🤖 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 `@src/domain/trust.rs` around lines 1095 - 1111, Update the decision test helper’s decision_hash construction to always format id_suffix as a single hexadecimal nibble before repeating it 64 times, preserving a valid 64-character hash and making values at or above 16 fail clearly rather than producing an oversized hash.
673-757: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccepted-status lists are hand-maintained duplicates of the enum vocabularies. 8000
Each profile spells out string literals (e.g. the fidelity "not rejected" set and the "declared coverage" set). Adding a new
FidelityTrustStatusorCoverageTrustStatusvariant silently widens or narrows a policy without a compile error. Consider deriving these from the enums (e.g. filterFidelityTrustStatus::ALLexcludingRejected,CoverageTrustStatus::ALLexcludingUnknown) so the policy stays exhaustive by construction.🤖 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 `@src/domain/trust.rs` around lines 673 - 757, Replace the hard-coded fidelity and coverage status string lists in the profile policy with values derived from their enum vocabularies, such as filtering FidelityTrustStatus::ALL to exclude Rejected and CoverageTrustStatus::ALL to exclude Unknown. Update the affected Experimental, Publication, and Upstream entries while preserving each profile’s existing exclusions and accepted-status behavior.src/domain/mod.rs (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-export
MAX_TRUST_HISTORYfromcrate::domain.
src/domain/mod.rsre-exports the trust module and schema-related symbols, but external callers must still refer tocrate::domain::trust::MAX_TRUST_HISTORYor repeat256when checking trust history/evidence artifact limits.🤖 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 `@src/domain/mod.rs` at line 24, Update the public exports in the domain module around the trust module declaration to re-export MAX_TRUST_HISTORY from crate::domain, allowing external callers to access the constant directly while preserving the existing trust module API.src/app.rs (1)
10231-10249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one more forgery case: a mutated axis status.
The test covers
assessment_hashsubstitution. Sincevalidate_promotion_assessmentalso compares each per-dimension status andeligibleagainst the assessment, adding a case that flips, for example,definition_statustoupstream_acceptedwhile leaving the hash intact would pin the status-binding half of that check too.🤖 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 `@src/app.rs` around lines 10231 - 10249, Extend the release integrity test around changed_metadata_manifest to add a forgery case that changes one axis status, such as definition_status, to upstream_accepted while preserving the assessment_hash. Write the mutated manifest, verify the restarted bundle offline with verify_release_bundle_integrity, and assert it returns MCL_RELEASE_SEMANTIC_CLOSURE_INVALID, matching the existing assessment-hash case.tests/mcp_stdio.rs (1)
1331-1363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for the two new verify actions.
claim_statushas an exhaustive forbidden-field loop (Lines 439-483) assertingMCL_MCP_FIELD_FORBIDDENfor every cross-action field. The newly hand-writtenreject_presentlists fortransition_trustandtrust_statusinsrc/mcp/mod.rshave no equivalent coverage, so a missing or misspelled rejection would go unnoticed. A conflict case (replaying this transition with a stalefrom_status, expectingMCL_TRUST_TRANSITION_CONFLICT) would also pin the ledger head guard through the MCP surface.Want me to draft those cases?
🤖 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 `@tests/mcp_stdio.rs` around lines 1331 - 1363, The MCP tests around the existing transition_trust success and idempotency assertions need negative coverage for both new verify actions. Add exhaustive forbidden-field cases matching the claim_status coverage for transition_trust and trust_status, asserting MCL_MCP_FIELD_FORBIDDEN, and add a replay using a stale from_status that asserts MCL_TRUST_TRANSITION_CONFLICT while exercising the ledger head guard.
🤖 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/architecture/TRUST_MODEL.md`:
- Around line 45-60: Update the TRUST_MODEL.md command examples to match the
existing VerifyAction parser surface: use the documented
TrustStatus/FidelityStatusOptions command and only its actual option names, and
replace or remove the unsupported transition-trust example and flags unless
corresponding parser symbols are added.
In `@src/app.rs`:
- Around line 1254-1297: Update transition_trust to require
request.reviewer_identity to match actor before validation or persistence, using
the existing identity-mismatch validation pattern such as review_fidelity.
Ensure this check applies consistently to definition, reuse, and covered
promotions, and preserve the actor-bound reviewer identity through
TrustDecision, trust status, and release manifest flows.
In `@src/domain/trust.rs`:
- Around line 445-446: Update bounded_nonempty, used by the reviewer_identity
and reason validation in the trust transition, to measure length in Unicode code
points rather than UTF-8 bytes. Preserve the existing non-empty and
maximum-bound checks so non-ASCII values valid under the committed schemas and
SQL constraint are accepted.
In `@tests/phase1_cli.rs`:
- Around line 1104-1105: Expand the promotion assertions in the affected test
cases around initial_trust to validate experimental, publication, and upstream
entries by their profile values rather than relying on promotion[0] ordering.
Also assert that the reuse and coverage axes remain unchanged across the
definition transition, preserving detection of omitted, reordered, or cross-axis
mutations.
---
Nitpick comments:
In `@migrations/0014_multidimensional_trust.sql`:
- Around line 39-40: Update the CHECK constraint comparing
evidence_artifact_hashes_json with request_json to compare normalized JSON
values rather than raw serialized text, and ensure the value produced in
src/store/mod.rs is normalized consistently before storage. Preserve validation
of equivalent arrays regardless of whitespace or formatting differences.
In `@src/app.rs`:
- Around line 10231-10249: Extend the release integrity test around
changed_metadata_manifest to add a forgery case that changes one axis status,
such as definition_status, to upstream_accepted while preserving the
assessment_hash. Write the mutated manifest, verify the restarted bundle offline
with verify_release_bundle_integrity, and assert it returns
MCL_RELEASE_SEMANTIC_CLOSURE_INVALID, matching the existing assessment-hash
case.
In `@src/domain/mod.rs`:
- Line 24: Update the public exports in the domain module around the trust
module declaration to re-export MAX_TRUST_HISTORY from crate::domain, allowing
external callers to access the constant directly while preserving the existing
trust module API.
In `@src/domain/trust.rs`:
- Around line 1095-1111: Update the decision test helper’s decision_hash
construction to always format id_suffix as a single hexadecimal nibble before
repeating it 64 times, preserving a valid 64-character hash and making values at
or above 16 fail clearly rather than producing an oversized hash.
- Around line 673-757: Replace the hard-coded fidelity and coverage status
string lists in the profile policy with values derived from their enum
vocabularies, such as filtering FidelityTrustStatus::ALL to exclude Rejected and
CoverageTrustStatus::ALL to exclude Unknown. Update the affected Experimental,
Publication, and Upstream entries while preserving each profile’s existing
exclusions and accepted-status behavior.
In `@tests/mcp_stdio.rs`:
- Around line 1331-1363: The MCP tests around the existing transition_trust
success and idempotency assertions need negative coverage for both new verify
actions. Add exhaustive forbidden-field cases matching the claim_status coverage
for transition_trust and trust_status, asserting MCL_MCP_FIELD_FORBIDDEN, and
add a replay using a stale from_status that asserts
MCL_TRUST_TRANSITION_CONFLICT while exercising the ledger head guard.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f58e7e1a-bdbd-4126-8c4c-12dda25aac5c
📒 Files selected for processing (26)
docs/architecture/TRUST_MODEL.mddocs/decisions/ADR-0018-multidimensional-trust-and-promotion-profiles.mddocs/dev-diary/0040-one-proof-five-questions.mddocs/dev-diary/README.mddocs/implementation/RELEASE_CHECKLIST.mddocs/implementation/STATUS.mddocs/trust/RELEASES.mdmigrations/0014_multidimensional_trust.sqlschemas/release/release-manifest-2.schema.jsonschemas/trust/promotion-assessment-1.schema.jsonschemas/trust/trust-status-1.schema.jsonschemas/trust/trust-transition-1.schema.jsonscripts/release-pilot-a.shsrc/app.rssrc/cli/mod.rssrc/comparator_export.rssrc/corpus_export.rssrc/domain/mod.rssrc/domain/release.rssrc/domain/trust.rssrc/mcp/mod.rssrc/release.rssrc/rl_export.rssrc/store/mod.rstests/mcp_stdio.rstests/phase1_cli.rs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b44cb7d42
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe9fb69102
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 694ee09e44
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 145ec435fb
ℹ️ 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".
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Change class
Affected trust boundary: exact formalization trust derivation, migration 14, release promotion, and database-independent v2 release verification.
TDD evidence
Red evidence: issue #67 adversarial criteria plus review reproductions for cross-axis history limits, invalid offline transitions, actor spoofing, kernel/fidelity legacy histories, and unbounded release projection.
Green evidence: exact-head CI run 30180744201 passes all five required jobs. Local final validation passes 201 library tests, 1 Lean worker test, 4 MCP tests, and 21 CLI tests.
Commands and results:
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --all-targets --all-featuresMathematical verification
Verifier and result: CI run 30180744201 is green. This change creates no mathematical proof, refutation, fidelity, or Comparator authority.
Compatibility and provenance
release_manifest/1private bundle bytes and legacy closure semantics remain unchanged.Risk and recovery
Known limitations: definition and reuse remain human-reviewed judgments; issues #68 and #69 add deeper definition-review and upstream workflows. Future graphical clients must render the canonical five-axis CLI/MCP representation without collapsing it to a Boolean.
Rollback or recovery plan: preserve the additive migration and immutable evidence/transition rows; revert application behavior only with a verified database backup and do not reinterpret emitted v2 manifests. Integrity failures direct operators to quarantine affected histories and restore exact backups.
Follow-up work: #68, then #69.
Review readiness
Closes #67
Summary by CodeRabbit
New Features
Documentation