refactor: audit verifier boundary purity + _listWriterIds extraction - #33
Conversation
- Extract _listWriterIds() private helper from verifyAll() for reuse - Move detectTrustWarning() from src/domain/ to CLI boundary (verify-audit.js), eliminating process.env read in domain layer - verifyAll() now accepts trustWarning via options (CLI-injected) - Add domain purity boundary test: grep assertion that src/domain/ never references process.env - Add trustWarning pass-through tests Domain services are now genuinely pure — no ambient runtime state.
|
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. 📝 WalkthroughWalkthroughThe changes relocate trust warning detection from the domain service layer to the CLI command layer, improving separation of concerns by removing environment-variable dependencies from the domain. A new private helper extracts writer IDs in the verification workflow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/cli/commands/verify-audit.js (1)
55-69:⚠️ Potential issue | 🟡 MinorSingle-writer path suppresses
trustWarning.When
--writeris specified (line 55–69),trustWarningis hardcoded tonull(line 68), so users won't see the trust configuration warning even ifWARP_TRUSTED_ROOTis set. If this is intentional, a brief comment explaining why would help future readers. If not, consider callingdetectTrustWarning()here too.
🧹 Nitpick comments (2)
test/unit/domain/services/AuditVerifierService.test.js (1)
787-791: Duplicate of existing test at lines 619–624.The test
'trustWarning is null when no trust config is present'(line 619) already asserts the same thing —verifyAllwithout atrustWarningoption returnsnull. Consider removing one of them.src/domain/services/AuditVerifierService.js (1)
227-234:trustWarningleaks intoverifyChainoptions.
verifyAllpasses the fulloptionsobject (includingtrustWarning) toverifyChain, which only expects{ since?: string }. It's harmless sinceverifyChainignores unknown properties, but for type-safety it would be cleaner to destructure:Proposed fix
async verifyAll(graphName, options = {}) { const writerIds = await this._listWriterIds(graphName); const chains = []; for (const writerId of writerIds.sort()) { - const result = await this.verifyChain(graphName, writerId, options); + const result = await this.verifyChain(graphName, writerId, { since: options.since }); chains.push(result); }
- Single-writer path now includes trustWarning (was hardcoded null)
- verifyAll passes only { since } to verifyChain, not the full options
- Remove duplicate trustWarning-null test (already covered at line 619)
Release Preflight
If you tag this commit as |
1 similar comment
Release Preflight
If you tag this commit as |
1 participant
Summary
_listWriterIds()private helper fromverifyAll()for reusedetectTrustWarning()fromsrc/domain/to CLI boundary (verify-audit.js), eliminatingprocess.envread in domain layerverifyAll()now acceptstrustWarningvia options (CLI-injected)src/domain/never referencesprocess.envNo new trust guarantees or claims — just architectural hygiene.
Test plan
npx eslinton modified files — cleannpx tsc --noEmit— cleanprocess.envinsrc/domain/Summary by CodeRabbit
New Features
Refactor
Tests