Conversation
Adds declarative trust configuration for WARP graphs — "Integrity without Identity." Operators declare which writers are trusted via a Git-native trust ref at refs/warp/<graph>/trust/root, and verify-audit now produces dual verdicts (integrity + trust). New domain layer: - TrustService (init, update, read, evaluate, diagnose, history) - TrustSchema (Zod v1, canonical JSON, domain-separated SHA-256 digest) - TrustError with 6 error codes - canonicalJson utility for deterministic serialization - RefLayout.buildTrustRef() New CLI: - git warp trust init [--from-writers] [--policy <p>] - git warp trust show - git warp trust doctor [--strict] [--pin <sha>] verify-audit integration: - Dual verdicts: integrityVerdict + trustVerdict - --trust-required flag for CI gates - --trust-ref-tip for pinned verification - Pin resolution: CLI > env (WARP_TRUSTED_ROOT) > live ref - Fail-closed on invalid pins Documentation: - docs/specs/TRUST_MODEL.md (full spec with threat model) - AUDIT_RECEIPT.md Section 9 updated with trust ref details - GUIDE.md Trust Configuration section added - CHANGELOG.md v10.15.0 entry Tests: - TrustSchema unit tests (6 tests) - TrustService unit tests (12 tests) - RefLayout trust ref tests - BATS E2E: cli-trust.bats (12 scenarios)
Pre-push hook requires wildcard casts to carry TODO(ts-cleanup) tags. Also fixes TypeScript strict-mode errors in test files: literal type annotations for version field, optional chaining for nullable results, and typed mock persistence return values.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a declarative trust layer (v11.1.0) for WARP graphs: TrustSchema, TrustService, TrustError, canonical JSON utilities, CLI trust commands (init/show/doctor), trust-aware verify-audit with dual verdicts and pinning, presenter output for trust, extensive docs and tests. Changes
Sequence DiagramssequenceDiagram
participant User as CLI User
participant Handler as Trust Handler
participant TrustSvc as TrustService
participant Persist as Persistence
participant Crypto as CryptoAdapter
User->>Handler: git warp trust init [--from-writers]
activate Handler
alt from-writers mode
Handler->>TrustSvc: enumerate writer refs
TrustSvc->>Persist: list refs by prefix
Persist-->>TrustSvc: writer ref list
Handler->>TrustSvc: initFromWriters(writerIds)
else manual mode
Handler->>TrustSvc: initTrust(config)
end
activate TrustSvc
TrustSvc->>Crypto: computeTrustDigest(canonical JSON)
Crypto-->>TrustSvc: digest (SHA-256)
TrustSvc->>Persist: write blob (trust.json)
TrustSvc->>Persist: write tree
TrustSvc->>Persist: write commit
TrustSvc->>Persist: CAS update trust ref
Persist-->>TrustSvc: commit SHA
deactivate TrustSvc
TrustSvc-->>Handler: { commitSha, snapshotDigest }
deactivate Handler
Handler-->>User: JSON/human-readable output
sequenceDiagram
participant User as CLI User
participant VerifyCmd as Verify-audit Handler
participant AudVerifier as AuditVerifierService
participant TrustSvc as TrustService
participant Persist as Persistence
User->>VerifyCmd: git warp verify-audit [--trust-required] [--trust-ref-tip SHA]
activate VerifyCmd
VerifyCmd->>AudVerifier: verifyAll(graphName, { trustRefTip, trustRequired })
activate AudVerifier
AudVerifier->>AudVerifier: verify chains (integrity check)
AudVerifier->>AudVerifier: compute integrityVerdict
alt trust service available
AudVerifier->>TrustSvc: readTrustConfig() or readTrustConfigAtCommit(trustRefTip)
activate TrustSvc
TrustSvc->>Persist: fetch trust ref/commit
Persist-->>TrustSvc: config, commitSha
deactivate TrustSvc
TrustSvc-->>AudVerifier: TrustAssessment
AudVerifier->>TrustSvc: evaluateWriters(writerIds, config)
TrustSvc-->>AudVerifier: evaluatedWriters, untrustedWriters
AudVerifier->>AudVerifier: compute trustVerdict
else no trust service
AudVerifier->>AudVerifier: set trust = NOT_CONFIGURED_TRUST
end
AudVerifier->>AudVerifier: determine exit code from verdicts & trustRequired
deactivate AudVerifier
AudVerifier-->>VerifyCmd: { chains, trust, integrityVerdict, trustVerdict }
VerifyCmd->>VerifyCmd: render output (dual verdicts + trust block)
deactivate VerifyCmd
VerifyCmd-->>User: human-readable or JSON result + exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@bin/presenters/text.js`:
- Around line 551-567: In appendTrustBlock, guard against missing or non-array
untrustedWriters before accessing its length: when reading t.untrustedWriters
(in function appendTrustBlock) either default it to an empty array or check
Array.isArray(t.untrustedWriters) && t.untrustedWriters.length > 0 before using
.length/join so the code won’t throw if the trust payload omits
untrustedWriters; update the conditional that currently uses
t.untrustedWriters.length and use the safe check or a local const like untrusted
= Array.isArray(t.untrustedWriters) ? t.untrustedWriters : [] then use
untrusted.length and untrusted.join.
In `@docs/GUIDE.md`:
- Around line 1828-1837: The example output is wrong: after calling
trust.initFromWriters(['alice','bob']) then
trust.evaluateWriters(['alice','charlie'], config) the untrusted writer
'charlie' must appear in the evaluation result; update the example
comment/result shown for the call to trust.evaluateWriters (and any accompanying
sample object) so that evaluatedWriters remains ['alice','charlie'] and
untrustedWriters contains ['charlie'], keeping explanations the same.
In `@docs/specs/AUDIT_RECEIPT.md`:
- Around line 337-344: The doc statement claiming "Trust evaluation is pure — no
environment reads, no side effects" is misleading because pin resolution reads
process.env.WARP_TRUSTED_ROOT in AuditVerifierService._evaluateTrust while the
actual writer evaluation (TrustService.evaluateWriters) is pure; update the spec
text to clarify this split by rephrasing to something like "Trust writer
evaluation (TrustService.evaluateWriters) is pure — no environment reads or side
effects; pin resolution (performed in AuditVerifierService._evaluateTrust) may
consult environment variables such as WARP_TRUSTED_ROOT and is an orchestration
concern," and also note that invalid pins fail closed.
In `@docs/specs/TRUST_MODEL.md`:
- Around line 104-106: Add explicit language specifiers "text" to the four
fenced code blocks that currently lack them (the blocks containing ref/layout
examples such as "refs/warp/<graphName>/trust/root → <trust commit SHA>" and the
other pseudocode/commit-structure examples) to satisfy markdownlint MD040;
replace each opening triple-backtick with ```text for the blocks showing ref
layouts and pseudocode so they are marked as non-code text.
- Around line 264-267: Update the `--strict` Mode section to explicitly state
that `--strict` is required for `fail` findings to produce a non-zero exit code
and that without `--strict` neither `fail` nor `warn` affect exit status; also
mention that, unlike `git warp doctor`, `trust doctor --strict` only treats
`fail` (not `warn`) findings as exit-code-triggering so readers understand the
difference between `--strict`, `fail`, `warn`, `trust doctor`, and `git warp
doctor`.
In `@src/domain/services/TrustService.js`:
- Around line 181-201: The epoch regression check in updateTrust currently
compares validated.epoch and current.config.epoch as strings (in updateTrust
after parseTrustConfig), which is fragile; instead, normalize/parse both epoch
values to a monotonic numeric timestamp (e.g., Date.parse or a consistent ISO
parser) before comparing and use that numeric comparison to detect regression;
ensure parseTrustConfig (or just before the comparison in updateTrust)
validates/normalizes epoch formats and throws a TrustError with code
E_TRUST_EPOCH_REGRESSION if the parsed newEpochTimestamp is less than
currentEpochTimestamp, and include newEpoch/currentEpoch (and their parsed
timestamps) in the error context for debugging.
In `@test/bats/cli-trust.bats`:
- Around line 96-107: The test fails because present() writes error payloads to
stderr (see present()) while the test helper _run_json only captures stdout;
update the test helper _run_json to capture both stdout and stderr (redirect
stderr into stdout, e.g. capture 2>&1) so JSON error payloads emitted to stderr
are available in $output, or alternatively modify present() to detect the --json
flag and emit error JSON to stdout instead of stderr; reference present(),
handleTrustShow, and _run_json when making the change so the test can read the
error JSON payload.
🧹 Nitpick comments (12)
src/domain/services/TrustSchema.js (1)
47-58:epochfield lacks ISO-8601 format validation.The schema uses
z.string().min(1)with the message "epoch must be a non-empty ISO-8601 string", but it does not actually validate the format.TrustService.updateTrustrelies on lexicographic comparison (validated.epoch < current.config.epoch) which is only correct for proper ISO-8601 strings. An arbitrary non-empty string would pass schema validation but break epoch regression detection.♻️ Consider adding ISO-8601 validation
- epoch: z.string().min(1, 'epoch must be a non-empty ISO-8601 string'), + epoch: z.string().min(1, 'epoch must be a non-empty ISO-8601 string') + .refine((s) => !Number.isNaN(Date.parse(s)), 'epoch must be a valid ISO-8601 date string'),src/domain/services/TrustService.js (2)
448-462:_checkPolicysilently skips unknown policies.If
config.policyis somehow not one of the two known values (e.g., due to a future schema relaxation or data corruption that bypasses validation), no finding is emitted. Consider adding a fallback warning.♻️ Add a fallback for unknown policies
_checkPolicy(config, findings) { if (config.policy === 'any' || config.policy === 'all_writers_must_be_trusted') { findings.push({ id: 'TRUST_POLICY_SUPPORTED', status: /** `@type` {const} */ ('ok'), message: `Policy "${config.policy}" is supported`, }); + } else { + findings.push({ + id: 'TRUST_POLICY_UNKNOWN', + status: /** `@type` {const} */ ('warn'), + message: `Policy "${config.policy}" is not recognized`, + }); } }
114-148: Duplicate code:initTrustand_writeConfigCommitboth build blob→tree→commit.Lines 121–131 in
initTrustduplicate the blob-write → tree-write → commit pipeline already extracted in_writeConfigCommit(lines 236–248).initTrustcould delegate to_writeConfigCommitinstead.♻️ Reuse _writeConfigCommit in initTrust
async initTrust(config) { const validated = parseTrustConfig(config); const canonical = canonicalizeTrustConfig(validated); const digest = this._crypto ? await computeTrustDigest(canonical, this._crypto) : null; - const blobOid = await this._persistence.writeBlob( - Buffer.from(canonical, 'utf8'), - ); - const treeOid = await this._persistence.writeTree([ - `100644 blob ${blobOid}\t${TRUST_BLOB_NAME}`, - ]); - const commitSha = await this._persistence.commitNodeWithTree({ - treeOid, - parents: [], - message: `trust: init (policy=${validated.policy}, writers=${validated.trustedWriters.length})`, - }); + const commitSha = await this._writeConfigCommit(canonical, [], + `trust: init (policy=${validated.policy}, writers=${validated.trustedWriters.length})`); try {Also applies to: 236-248
bin/cli/commands/trust.js (2)
137-146: Accessing private_trustRefbreaks encapsulation.
trustService._trustRefis accessed directly on line 140. This is also done inAuditVerifierService.js(lines 716–717). Consider exposing a public getter onTrustService(e.g.,get trustRef()) so the ref path can be consumed without relying on the underscore-convention private field.♻️ Add a public getter to TrustService
In
src/domain/services/TrustService.js:constructor({ persistence, graphName, crypto, logger }) { this._persistence = persistence; this._graphName = graphName; this._crypto = crypto || null; this._logger = logger || null; this._trustRef = buildTrustRef(graphName); } + + /** `@returns` {string} The trust ref path */ + get trustRef() { + return this._trustRef; + }Then update consumers to use
trustService.trustRefinstead oftrustService._trustRef.
83-95: Redundant re-read afterinitFromWriters— consider reading from the known commit.After
initFromWritersreturnscommitSha, line 84 does a fullreadTrustConfig()which re-resolves the live ref. Since you already have the commit SHA,readTrustConfigAtCommit(commitSha)would avoid re-resolving the ref and be resilient to (unlikely) races.♻️ Read config from known commit
const { commitSha, snapshotDigest } = await trustService.initFromWriters(writerIds); - const config = (await trustService.readTrustConfig())?.config; + const config = (await trustService.readTrustConfigAtCommit(commitSha))?.config;Same applies in
initManual(line 112).src/domain/services/AuditVerifierService.js (1)
256-269:NOT_CONFIGURED_TRUSTis shared by reference — consider freezing it.
NOT_CONFIGURED_TRUSTis returned directly from_evaluateTrustin multiple code paths (lines 671, 703, 707). Since it contains mutable arrays (evaluatedWriters,untrustedWriters,explanations), a consumer that inadvertently mutates the returned object would corrupt the shared constant for subsequent calls.♻️ Freeze the constant or return a copy
Option A — Freeze:
const NOT_CONFIGURED_TRUST = Object.freeze({ status: 'not_configured', source: 'none', sourceDetail: null, ref: null, commit: null, policy: null, - evaluatedWriters: [], - untrustedWriters: [], - explanations: [], + evaluatedWriters: Object.freeze([]), + untrustedWriters: Object.freeze([]), + explanations: Object.freeze([]), snapshotDigest: null, });Option B — Return a shallow copy in
_evaluateTrust:- return NOT_CONFIGURED_TRUST; + return { ...NOT_CONFIGURED_TRUST, evaluatedWriters: [], untrustedWriters: [], explanations: [] };test/bats/helpers/seed-trust-graph.js (1)
12-13: Missing guard forREPO_PATHenv var.If
REPO_PATHis unset,repoPathwill beundefined, causing a confusing error fromGitPlumbing. A quick guard would make failures obvious.🛡️ Suggested guard
const projectRoot = process.env.PROJECT_ROOT || resolve(import.meta.dirname, '../../..'); const repoPath = process.env.REPO_PATH; +if (!repoPath) { + throw new Error('REPO_PATH environment variable is required'); +}bin/cli/commands/verify-audit.js (1)
53-72: Redundant full verification when filtering by writer.When
writerFilteris set,verifyChainruns for the single writer, thenverifyAllruns the entire verification across all writers just to extract trust metadata. For repos with many writers, this doubles the work.Consider extracting trust evaluation into a standalone call (e.g.,
trustService.evaluate(...)) that doesn't require re-verifying all chains.test/bats/cli-trust.bats (1)
15-19:_run_jsonsilently swallows all stderr, which can hide real failures.While suppressing stderr is necessary for clean JSON parsing, consider redirecting stderr to a separate variable (e.g.,
stderr_output) or to a temp file for debugging when tests fail unexpectedly.test/unit/domain/services/TrustService.test.js (3)
370-376: The "purity" assertion doesn't actually prove absence of env reads.Calling the same function twice with the same input and checking for equal output only proves determinism, not that
process.envis untouched. A side-effect-free env could still be read without affecting the result. Consider spying onprocess.envaccess via aProxyorvi.spyOnto make this assertion meaningful.💡 Proposed stronger purity assertion
it('is pure — no env reads', () => { - // evaluateWriters takes explicit config, doesn't read process.env - const config = validConfig(); - const result1 = service.evaluateWriters(['alice'], config); - const result2 = service.evaluateWriters(['alice'], config); - expect(result1).toEqual(result2); + const envSpy = vi.spyOn(process, 'env', 'get'); + const config = validConfig(); + service.evaluateWriters(['alice'], config); + expect(envSpy).not.toHaveBeenCalled(); + envSpy.mockRestore(); });Alternatively, if
vi.spyOnonprocess.envgetter is cumbersome, wrappingprocess.envin aProxythat tracks access works too.
486-493: Consider asserting themessageandcontextproperties as well.The test validates
name,code, andinstanceof, but doesn't asserterr.message === 'test'or check that acontextobject is stored when provided. SinceTrustErrorcarries contextual data (e.g.,context.shaas relied upon in line 170), testing the full shape here would give more confidence in the error contract.💡 Suggested addition
it('has correct name and code', () => { - const err = new TrustError('test', { code: 'E_TRUST_SCHEMA_INVALID' }); + const err = new TrustError('test', { code: 'E_TRUST_SCHEMA_INVALID', context: { detail: 'x' } }); expect(err.name).toBe('TrustError'); expect(err.code).toBe('E_TRUST_SCHEMA_INVALID'); + expect(err.message).toBe('test'); + expect(err.context).toEqual({ detail: 'x' }); expect(err instanceof Error).toBe(true); });
290-297: Add test case for equal epoch to clarify allowed behavior.The current test verifies that an older epoch is rejected, but there is no test for an equal epoch (same
'2025-01-01T00:00:00.000Z'as the service initialization). The implementation uses a strict less-than comparison (<), which means equal epochs are allowed and should not triggerE_TRUST_EPOCH_REGRESSION. Adding an explicit test for this case would document this behavior and improve test coverage.
| // Initialize | ||
| await trust.initFromWriters(['alice', 'bob']); | ||
|
|
||
| // Read current config | ||
| const { config, commitSha, snapshotDigest } = await trust.readTrustConfig(); | ||
|
|
||
| // Evaluate writers | ||
| const result = trust.evaluateWriters(['alice', 'charlie'], config); | ||
| // { evaluatedWriters: ['alice', 'charlie'], untrustedWriters: [], explanations: [...] } | ||
|
|
There was a problem hiding this comment.
Incorrect example output: untrustedWriters should contain 'charlie'.
The example initializes trust with ['alice', 'bob'] but then evaluates ['alice', 'charlie']. Since 'charlie' is not in the trusted set, the comment showing untrustedWriters: [] is wrong — it should be ['charlie'].
📝 Proposed fix
-// { evaluatedWriters: ['alice', 'charlie'], untrustedWriters: [], explanations: [...] }
+// { evaluatedWriters: ['alice', 'charlie'], untrustedWriters: ['charlie'], explanations: [...] }🤖 Prompt for AI Agents
In `@docs/GUIDE.md` around lines 1828 - 1837, The example output is wrong: after
calling trust.initFromWriters(['alice','bob']) then
trust.evaluateWriters(['alice','charlie'], config) the untrusted writer
'charlie' must appear in the evaluation result; update the example
comment/result shown for the call to trust.evaluateWriters (and any accompanying
sample object) so that evaluatedWriters remains ['alice','charlie'] and
untrustedWriters contains ['charlie'], keeping explanations the same.
@git-stunts/plumbing stream.collect({ asString: false }) returns
Uint8Array, not Buffer. Uint8Array.toString() ignores encoding args
and produces comma-separated byte values ("123,34,97,..."), causing
JSON.parse to fail at position 3 in CI Docker environments.
TextDecoder.decode() works correctly for both Uint8Array and Buffer
and is available in all target runtimes (Node/Bun/Deno).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/domain/services/TrustService.js`:
- Around line 454-462: The _checkPolicy function currently only pushes an OK
finding for known policies and silently ignores unknown values; update
_checkPolicy to add an else branch that pushes a finding when config.policy is
unrecognized (e.g., id: 'TRUST_POLICY_UNRECOGNIZED' with status 'warning' or
'fail' and a message naming the unknown policy) so the diagnose doctor flags
invalid policy values; modify the method that checks config.policy (the
_checkPolicy function) to detect any value not equal to 'any' or
'all_writers_must_be_trusted' and append the appropriate non-ok finding to the
findings array.
🧹 Nitpick comments (1)
src/domain/services/TrustService.js (1)
114-148:initTrustduplicates the blob→tree→commit pipeline in_writeConfigCommit.Lines 121–131 replicate the same
writeBlob → writeTree → commitNodeWithTreesequence that_writeConfigCommitalready encapsulates (used byupdateTrust). Consider reusing it here to keep one code path for commit creation.♻️ Proposed refactor
async initTrust(config) { const validated = parseTrustConfig(config); const canonical = canonicalizeTrustConfig(validated); const digest = this._crypto ? await computeTrustDigest(canonical, this._crypto) : null; - const blobOid = await this._persistence.writeBlob( - Buffer.from(canonical, 'utf8'), - ); - const treeOid = await this._persistence.writeTree([ - `100644 blob ${blobOid}\t${TRUST_BLOB_NAME}`, - ]); - const commitSha = await this._persistence.commitNodeWithTree({ - treeOid, - parents: [], - message: `trust: init (policy=${validated.policy}, writers=${validated.trustedWriters.length})`, - }); + const commitSha = await this._writeConfigCommit(canonical, [], + `trust: init (policy=${validated.policy}, writers=${validated.trustedWriters.length})`); try {
Release Preflight
If you tag this commit as |
Speculative designs for patch signature verification, quorum for trust mutations, writer revocation, pre-receive hook enforcement, and the allowlist_with_exceptions policy. Informed by shiplog's existing chain/attestation trust model.
Release Preflight
If you tag this commit as |
- Guard untrustedWriters access with optional chaining in text presenter - Add ISO-8601 validation refinement to epoch field in TrustSchema - Use Date.parse() for epoch regression check instead of string comparison - Add else branch in _checkPolicy for unknown policies (warn finding) - DRY initTrust by reusing _writeConfigCommit helper - Add public trustRef getter; update consumers to use it - Freeze NOT_CONFIGURED_TRUST constant (prevent shared-state mutation) - Use readTrustConfigAtCommit in CLI init handlers (avoid re-reading ref) - Fix _run_json BATS helper to capture stderr (present() routes errors there) - Add REPO_PATH guard in seed-trust-graph.js - Add equal-epoch test and improve TrustError assertion coverage
- Clarify trust purity: evaluateWriters is pure; pin resolution reads env - Add language specifiers to 4 fenced code blocks in TRUST_MODEL.md (MD040) - Clarify --strict mode: required for non-zero exit, only fail findings count - Add clarifying comment for evaluateWriters example in GUIDE.md (policy=any)
PR Feedback AddressedCommits Code Fixes (8 items)
Doc Fixes (4 items)
Test Improvements (2 items)
False Positive (1 item)
Skipped (2 nits — marginal value)
All 3657 unit tests pass. Lint, typecheck, and policy checks clean. |
|
@coderabbitai review please 🥕 |
|
✅ 8 code fixes including guard clauses, ISO-8601 validation, DRY improvements, and state protection Great work on maintaining all tests passing and keeping the codebase clean. I'll start a fresh review of the PR now. ✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@bin/cli/commands/trust.js`:
- Around line 55-96: The initFromWriterRefs flow currently ignores the parsed
--policy flag; update handleTrustInit/initFromWriterRefs to either pass the
policy through to trustService.initFromWriters or explicitly reject the
combination. Specifically, read values.policy in handleTrustInit and either (A)
call initFromWriterRefs({ trustService, persistence, graphName, policy:
values.policy }) and update initFromWriterRefs to forward that policy into
trustService.initFromWriters(writerIds, policy) and TrustService.initFromWriters
signature, or (B) detect when values.fromWriters && values.policy is provided
and return a usage error payload/EXIT_CODES.INVALID_ARGUMENT with a clear
message that --from-writers and --policy are mutually exclusive; adjust
TRUST_OPTIONS/trustSchema validation if needed.
In `@bin/presenters/text.js`:
- Around line 593-600: The block that prints dual verdicts uses
payload.integrityVerdict as the sole guard but always renders
payload.trustVerdict, which can produce "undefined"; update the logic in the
presenter (the section that calls appendTrustBlock and the dual-verdict block)
to either check both payload.integrityVerdict and payload.trustVerdict before
pushing the Trust line or render Integrity and Trust independently with their
own guards (use colorVerdict only when the respective payload.integrityVerdict
or payload.trustVerdict is present, and only call lines.push for that verdict).
In `@src/domain/services/AuditVerifierService.js`:
- Around line 42-43: Remove the duplicate JSDoc `@type {TrustAssessment}`
annotation in AuditVerifierService.js by deleting the redundant line so only a
single `@type {TrustAssessment}` remains; locate the duplicate comments near the
top of the file around the AuditVerifierService declarations and remove the
extra annotation to avoid the repeated JSDoc entry.
🧹 Nitpick comments (4)
src/domain/services/TrustSchema.js (1)
47-59: Epoch validation viaDate.parse()is lenient — accepts non-ISO-8601 strings.
Date.parse()accepts many formats beyond ISO-8601 (e.g.,"Jan 1 2024","2024"). The spec (TRUST_MODEL.md §3.1) requires a "Non-empty ISO-8601 timestamp," but the schema only rejects unparseable strings. Consider a stricter regex or format check if you want to enforce ISO-8601 at the parse boundary.Example: tighter ISO-8601 check
- epoch: z.string().min(1, 'epoch must be a non-empty ISO-8601 string') - .refine((s) => !Number.isNaN(Date.parse(s)), 'epoch must be a valid ISO-8601 date string'), + epoch: z.string().min(1, 'epoch must be a non-empty ISO-8601 string') + .refine((s) => { + const ts = Date.parse(s); + if (Number.isNaN(ts)) return false; + // Round-trip check: reject non-ISO formats that Date.parse accepts loosely + return new Date(ts).toISOString() !== 'Invalid Date'; + }, 'epoch must be a valid ISO-8601 date string'),src/domain/services/AuditVerifierService.js (1)
670-705: Non-pin errors silently degrade tonot_configured— potential masking of real failures.When
pinis falsy andreadTrustConfig()throws (e.g., corrupt trust blob, persistence error), the catch at line 688 returnsNOT_CONFIGURED_TRUST. This is a fail-open design per the spec, but it means genuine trust-ref corruption is indistinguishable from "trust not set up" in the verification output. Consider logging the error so operators can diagnose via logs even when the verdict showsnot_configured.Proposed improvement
} catch { // Invalid pin: fail closed, no fallback if (pin) { return { status: 'error', ... }; } + this._logger?.warn('[warp:trust]', { + code: 'TRUST_READ_FAILED', + message: 'Failed to read trust config from live ref; treating as not_configured', + }); return NOT_CONFIGURED_TRUST; }bin/presenters/text.js (1)
494-510:renderTrustShowaccessespayload.configwithout a null guard, unlikerenderTrustInit.The CLI handler should always provide
confighere, but for defensive consistency withrenderTrustInit(which guardspayload.config), consider adding a guard or documenting the contract.bin/cli/commands/trust.js (1)
21-50: Persistence and crypto are created before validating the sub-action.If the sub-action is invalid (or missing), the eagerly-created resources are wasted. Since this is a CLI entry point it's not a performance concern, but moving the validation above the I/O would give a faster "usage error" response and keep the code slightly cleaner.
Optional: validate sub-action early
export default async function handleTrust({ options, args }) { const subAction = args[0]; + if (!['init', 'show', 'doctor'].includes(subAction)) { + return { + payload: { error: { code: 'E_USAGE', message: `Unknown trust sub-action: ${subAction || '(none)'}. Available: init, show, doctor` } }, + exitCode: EXIT_CODES.USAGE, + }; + } const subArgs = args.slice(1); const { persistence } = await createPersistence(options.repo);
| async function handleTrustInit({ trustService, persistence, graphName, args }) { | ||
| const { values } = parseCommandArgs(args, TRUST_OPTIONS, trustSchema); | ||
|
|
||
| if (values.fromWriters) { | ||
| return await initFromWriterRefs({ trustService, persistence, graphName }); | ||
| } | ||
|
|
||
| return await initManual({ trustService, graphName, policy: values.policy || 'any' }); | ||
| } | ||
|
|
||
| /** | ||
| * Seeds trust config from existing writer refs. | ||
| * @param {{ trustService: TrustService, persistence: *, graphName: string }} params | ||
| */ | ||
| async function initFromWriterRefs({ trustService, persistence, graphName }) { | ||
| const prefix = buildWritersPrefix(graphName); | ||
| const refs = await persistence.listRefs(prefix); | ||
| const writerIds = refs | ||
| .map((/** @type {string} */ ref) => ref.slice(prefix.length)) | ||
| .filter((/** @type {string} */ id) => id.length > 0); | ||
|
|
||
| if (writerIds.length === 0) { | ||
| return { | ||
| payload: { error: { code: 'E_NOT_FOUND', message: 'No writers found for graph' } }, | ||
| exitCode: EXIT_CODES.NOT_FOUND, | ||
| }; | ||
| } | ||
|
|
||
| const { commitSha, snapshotDigest } = await trustService.initFromWriters(writerIds); | ||
| const config = (await trustService.readTrustConfigAtCommit(commitSha))?.config; | ||
| return { | ||
| payload: { | ||
| action: 'init', | ||
| graph: graphName, | ||
| commit: commitSha, | ||
| snapshotDigest, | ||
| config, | ||
| seedWriters: writerIds.sort(), | ||
| }, | ||
| exitCode: EXIT_CODES.OK, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find where TrustService is defined
find . -type f -name "*.js" | xargs grep -l "class TrustService"Repository: git-stunts/git-warp
Length of output: 100
🏁 Script executed:
# Search for the initFromWriters method definition
rg "initFromWriters" -A 10 -B 2Repository: git-stunts/git-warp
Length of output: 7712
🏁 Script executed:
# Also check the TRUST_OPTIONS definition to understand all available options
rg "TRUST_OPTIONS" -B 5 -A 10 bin/cli/commands/trust.jsRepository: git-stunts/git-warp
Length of output: 1662
Forward --policy to initFromWriters() or reject the option combination.
The initFromWriters method (src/domain/services/TrustService.js) only accepts writerIds and hardcodes policy: 'any'. When a user runs git warp trust init --from-writers --policy strict, the --policy flag is silently ignored. Since both options are defined in TRUST_OPTIONS, users expect them to work together.
Either modify initFromWriters() to accept and use a policy parameter, or make the flags mutually exclusive by rejecting the combination with a usage error.
🤖 Prompt for AI Agents
In `@bin/cli/commands/trust.js` around lines 55 - 96, The initFromWriterRefs flow
currently ignores the parsed --policy flag; update
handleTrustInit/initFromWriterRefs to either pass the policy through to
trustService.initFromWriters or explicitly reject the combination. Specifically,
read values.policy in handleTrustInit and either (A) call initFromWriterRefs({
trustService, persistence, graphName, policy: values.policy }) and update
initFromWriterRefs to forward that policy into
trustService.initFromWriters(writerIds, policy) and TrustService.initFromWriters
signature, or (B) detect when values.fromWriters && values.policy is provided
and return a usage error payload/EXIT_CODES.INVALID_ARGUMENT with a clear
message that --from-writers and --policy are mutually exclusive; adjust
TRUST_OPTIONS/trustSchema validation if needed.
| appendTrustBlock(lines, payload); | ||
|
|
||
| // Dual verdicts | ||
| if (payload.integrityVerdict) { | ||
| lines.push(''); | ||
| lines.push(`${ANSI_YELLOW}Trust: ${payload.trustWarning.message}${ANSI_RESET}`); | ||
| lines.push(`Integrity: ${colorVerdict(payload.integrityVerdict)}`); | ||
| lines.push(`Trust: ${colorVerdict(payload.trustVerdict)}`); | ||
| } |
There was a problem hiding this comment.
trustVerdict rendered without its own guard.
Line 599 renders payload.trustVerdict inside the if (payload.integrityVerdict) block. If only integrityVerdict is set and trustVerdict is missing, the output would show a red-colored undefined. Consider guarding both or rendering them independently.
Suggested guard
if (payload.integrityVerdict) {
lines.push('');
lines.push(`Integrity: ${colorVerdict(payload.integrityVerdict)}`);
- lines.push(`Trust: ${colorVerdict(payload.trustVerdict)}`);
+ if (payload.trustVerdict) {
+ lines.push(`Trust: ${colorVerdict(payload.trustVerdict)}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| appendTrustBlock(lines, payload); | |
| // Dual verdicts | |
| if (payload.integrityVerdict) { | |
| lines.push(''); | |
| lines.push(`${ANSI_YELLOW}Trust: ${payload.trustWarning.message}${ANSI_RESET}`); | |
| lines.push(`Integrity: ${colorVerdict(payload.integrityVerdict)}`); | |
| lines.push(`Trust: ${colorVerdict(payload.trustVerdict)}`); | |
| } | |
| appendTrustBlock(lines, payload); | |
| // Dual verdicts | |
| if (payload.integrityVerdict) { | |
| lines.push(''); | |
| lines.push(`Integrity: ${colorVerdict(payload.integrityVerdict)}`); | |
| if (payload.trustVerdict) { | |
| lines.push(`Trust: ${colorVerdict(payload.trustVerdict)}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@bin/presenters/text.js` around lines 593 - 600, The block that prints dual
verdicts uses payload.integrityVerdict as the sole guard but always renders
payload.trustVerdict, which can produce "undefined"; update the logic in the
presenter (the section that calls appendTrustBlock and the dual-verdict block)
to either check both payload.integrityVerdict and payload.trustVerdict before
pushing the Trust line or render Integrity and Trust independently with their
own guards (use colorVerdict only when the respective payload.integrityVerdict
or payload.trustVerdict is present, and only call lines.push for that verdict).
Addresses PR #31 nits #15 and #17: - Add standalone evaluateTrust() method to AuditVerifierService, avoiding redundant verifyAll() when filtering by --writer - Move process.env.WARP_TRUSTED_ROOT resolution to CLI boundary via resolveTrustPin() helper, making domain services genuinely pure - Export deriveTrustVerdict() with full JSDoc mapping contract - Add 10 new tests: evaluateTrust unit tests, deriveTrustVerdict exhaustiveness, performance regression guard, domain purity boundary
Release Preflight
If you tag this commit as |
|
Closing this PR in favor of a redesigned trust architecture. Why this is being closedDuring review, I realized this implementation provides a deterministic policy allowlist with good boundaries and UX, but it does not provide cryptographic identity-backed trust. Specifically:
That means this branch is useful as policy configuration, but calling it “trust” would overstate security guarantees. What to keepThe following design improvements are correct and will be carried forward:
Next stepWe are starting a new Trust v1 implementation focused on:
This PR is closed to avoid merging a misleading security model under the @coderabbitai please leave your remarks |
|
Tip For best results, initiate chat on the files or code changes.
Strong reasoning for closureYour assessment is spot-on. The current implementation is indeed a deterministic policy allowlist, not a cryptographic trust system: ✅ Correctly identified gaps:
Shipping this under the name "trust" would create a false sense of security—users might assume cryptographic guarantees that aren't there. Closing it is the right call. Design improvements worth preservingThese elements represent solid architectural wins: ✅ Pin resolution at CLI boundary — keeps domain pure and testable These patterns will serve the redesigned Trust v1 well. Next-generation designYour planned focus areas hit the core requirements for real cryptographic trust:
This foundation will enable actual security guarantees rather than policy configuration. Final thoughtIt takes discipline to close a working, tested PR with 3,657 passing tests because the naming overstates security properties. This decision protects users and maintains the integrity of your security model. The redesigned Trust v1 will be built on a stronger foundation. Good luck with the new architecture! 🐰 |
1 participant
Summary
refs/warp/<graph>/trust/root— "Integrity without Identity"TrustService(591 LOC),TrustSchema,TrustError,canonicalJsonutility, andbuildTrustRef()in RefLayoutgit warp trust {init, show, doctor}subcommands with--from-writers,--policy,--strict,--pinflagsverify-auditnow produces dual verdicts (integrity + trust) with--trust-requiredand--trust-ref-tipflagsdocs/specs/TRUST_MODEL.md, unit tests (TrustSchema + TrustService), BATS E2E tests (12 scenarios)Test plan
cli-trust.bats) — run via Docker in CISummary by CodeRabbit
New Features
Documentation
Tests
Chore