refactor: hex-audit PRs + code review fixes#46
Merged
flyingrobots merged 21 commits intomainfrom Mar 8, 2026
Merged
Conversation
Create test/helpers/ with shared utilities previously duplicated across views.test.ts, DashboardApp.test.ts and integration.test.ts: - snapshot.ts: makeSnapshot() builder + entity factories - keys.ts: makeKey() / makeResize() keyboard input helpers - ansi.ts: strip() ANSI escape-code removal - ports.ts: mock port factories for GraphContext, IntakePort, GraphPort, SubmissionPort All 671 tests pass. Zero logic changes.
Add sliceDate(), groupBy(), indexBy(), stripPrefix() to the existing view-helpers.ts module. Replace manual Map-building loops and toISOString().slice(0,10) patterns across: - src/tui/render-status.ts (4 sliceDate, 4 groupBy) - src/tui/bijou/views/lineage-view.ts (1 sliceDate, 1 groupBy) - src/tui/bijou/views/submissions-view.ts (1 sliceDate, 2 groupBy) - src/tui/bijou/views/dashboard-view.ts (1 groupBy) - src/tui/bijou/views/roadmap-view.ts (2 groupBy) All 671 tests pass. Zero logic changes.
Add assertNodeExists() to validators.ts and use it across 5 CLI command files, replacing 15 inline hasNode() + throw NOT_FOUND checks: - coordination.ts (1 instance) - dashboard.ts (2 instances) - link.ts (3 instances) - suggestions.ts (2 instances) - traceability.ts (7 instances) Also simplify assertCampaignPrefix (link.ts) and assertDecomposeFrom/To (traceability.ts) to delegate to assertPrefixOneOf instead of re-implementing the logic. All 671 tests pass. Zero logic changes.
Create src/ports/SecretPort.ts with the SecretPort interface, extracted from VaultSecretAdapter.ts (infrastructure layer). - VaultSecretAdapter & InMemorySecretAdapter now implement SecretPort - AnthropicLlmAdapter imports SecretPort from ports, not infra - Backward-compatible type alias exported from VaultSecretAdapter.ts - analyze.ts CLI command (imports VaultSecretAdapter class) unchanged Fixes hexagonal architecture violation: interfaces belong in ports, not in infrastructure adapters. All 671 tests pass.
Apply Interface Segregation Principle — split monolithic RoadmapPort into three focused sub-interfaces: - RoadmapQueryPort: getQuests, getQuest, getOutgoingEdges - RoadmapMutationPort: upsertQuest, addEdge - RoadmapSyncPort: sync RoadmapPort is retained as the composite (extends all three) for backward compatibility and for the WarpRoadmapAdapter implementation. Domain services now depend on the narrowest port they need: - SovereigntyService, IntakeService → RoadmapQueryPort - TriageService → RoadmapQueryPort & RoadmapMutationPort - CoordinatorService → RoadmapPort (needs all three) Test mocks trimmed to match their narrowed interfaces. All 671 tests pass. Zero logic changes.
H1 — GuildSealService: Extract filesystem and crypto RNG operations to KeyringStoragePort + FsKeyringAdapter. GuildSealService now depends solely on the port interface; no node:fs, node:path, or node:crypto imports remain in src/domain/. H2 — TestFileParser: Move TypeScript Compiler API logic from src/domain/services/analysis/TestFileParser.ts to TsCompilerTestParserAdapter in the infrastructure layer, behind a TestParserPort interface. Original domain file replaced with a deprecation tombstone. All 671 tests pass. Zero node:* imports in src/domain/.
Cover assertNodeExists, assertPrefix, assertMinLength, assertPrefixOneOf, and parseHours with 16 unit tests.
…r param (PR4 #13–#14) - Mark re-exported SecretAdapter type alias as @deprecated - Rename secretAdapter → secretPort in AnthropicLlmAdapter to match port naming
Replace filesystem-specific terms (file, path, rename) with storage-agnostic phrasing in the port interface documentation.
getNodeProps() returns Record<string, unknown> in v13, not Map. The mock was returning new Map() which would break any future test that accesses properties via bracket notation.
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces port-based architecture patterns by creating new abstractions (KeyringStoragePort, SecretPort, TestParserPort) and refactoring existing services to depend on these ports. It splits RoadmapPort into query/mutation/sync variants, migrates GuildSealService to use a filesystem keyring adapter, and consolidates repeated test infrastructure into shared helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
- Add missing opts param to filterSnapshot mock (matches GraphContext interface) - Remove filesystem references from GuildSealService JSDoc (domain must not leak infrastructure details)
Use GraphContext['filterSnapshot'] type assertion instead of inline parameter annotation to satisfy no-unused-vars while preserving correct arity in the mock type.
![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
🧹 Nitpick comments (1)
src/infrastructure/adapters/FsKeyringAdapter.ts (1)
98-121: Consider adding debug logging for best-effort operations.The empty catch blocks in
removePrivateKeyandrestoreRetiredPrivateKeyare intentionally best-effort, but silently swallowing errors can make debugging harder if key files get stuck in unexpected states.If a logging port is available, consider emitting debug-level messages on failure. Otherwise, this is acceptable as-is for cleanup paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/FsKeyringAdapter.ts` around lines 98 - 121, The empty catch blocks in removePrivateKey and restoreRetiredPrivateKey swallow errors; change them to catch (err) and emit a debug-level message including err so failures are visible during debugging: inside removePrivateKey and restoreRetiredPrivateKey, capture the exception as err and call a logging sink if present (e.g. this.logger?.debug?.(`removePrivateKey failed for ${agentId}:`, err) or this.log?.debug), otherwise fall back to console.debug(`...`, err); keep the operations best-effort (do not rethrow) and avoid changing retirePrivateKey behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/services/GuildSealService.ts`:
- Around line 89-106: Before registering the new public key, check the loaded
keyring (this.storage.loadKeyring()) for any existing active entry for the same
agentId and mark it inactive (or set active=false) so you don't create a second
active key; specifically, inside the try block before keyring.entries.set(...)
find entries where entry.agentId === agentId && entry.active === true and set
entry.active = false, then call this.storage.saveKeyring(keyring) (or save once
after making both the deactivation and the new entry) so
keyIdForAgent()/rotateKey() remain unambiguous; keep using
writePrivateKey(agentId, privateKeyHex) semantics and ensure the rollback in the
catch still removes the .sk if the keyring save fails.
- Around line 153-180: The sequence currently calls retirePrivateKey(agentId,
suffix) then writePrivateKeyOverwrite(agentId, privateKeyHex) before updating
and saving the keyring, which can leave the live .sk missing if the write fails;
move the writePrivateKeyOverwrite call into the try block so it runs after you
update keyring.entries and call this.storage.saveKeyring(keyring), and in the
catch ensure you both remove the new private key (this.storage.removePrivateKey)
and restore the retired one (this.storage.restoreRetiredPrivateKey) and also
revert any modified keyring.entries so the old key remains active (undo changes
to keyring.entries for currentKeyId and newKeyId) to maintain consistency
between the filesystem and the keyring.
In `@src/infrastructure/adapters/TsCompilerTestParserAdapter.ts`:
- Around line 90-92: The ImportRef currently conflates namespace imports with
default imports; update the ImportRef interface to add a namespaceImport?:
string field and in TsCompilerTestParserAdapter replace the
ts.isNamespaceImport(bindings) branch (where it currently sets defaultImport =
bindings.name.text) to set namespaceImport = bindings.name.text instead, leaving
defaultImport only for ts.isIdentifier/default import cases; also adjust any
places that construct or consume ImportRef to account for the new
namespaceImport field so downstream analysis uses namespaceImport when present.
- Around line 208-218: Called test-framework property-access methods (e.g.,
matchers/mocks) are being pushed into calledMethods; update the property-access
branch in TsCompilerTestParserAdapter where ts.isPropertyAccessExpression(expr)
is handled to first compute the full call name via getCallName(node or expr) and
skip pushing expr.name.text when isTestFrameworkCall(callName) returns true. In
short: replace the unconditional calledMethods.push(expr.name.text) with a guard
that calls getCallName(...) and only pushes when isTestFrameworkCall(...) is
false, keeping existing behavior for non-framework chains.
- Around line 39-45: The parser is hard-coding ts.ScriptKind.TSX when calling
ts.createSourceFile (in TsCompilerTestParserAdapter), which misparses plain .ts
files; change the call to derive the script kind from filePath (e.g., use
ts.getScriptKindFromFileName(filePath) or a small helper that maps extensions
.ts/.tsx/.js/.jsx to the correct ts.ScriptKind) and pass that value into
ts.createSourceFile (keep setParentNodes true and other args unchanged) so
sourceFile is created with the correct script kind for each test file.
In `@src/ports/KeyringStoragePort.ts`:
- Around line 17-65: The KeyringStoragePort must expose atomic mutation
semantics rather than a separate loadKeyring()/saveKeyring() pair: add a single
transactional API (eg. updateKeyring or runKeyringTransaction) that accepts a
mutator function which receives the current Keyring and returns a new Keyring
(or signals abort), and ensure the implementation performs the Keyring update
and any private-key lifecycle calls (retirePrivateKey, writePrivateKeyOverwrite,
restoreRetiredPrivateKey, writePrivateKey, removePrivateKey) atomically or rolls
back on failure; alternatively provide a CAS-style
saveKeyringIfUnchanged(oldKeyring, newKeyring) combined with a lock/lease
mechanism, and document that GuildSealService must use the new transactional
method instead of loadKeyring/saveKeyring to avoid lost updates and half-applied
rotations.
In `@test/helpers/snapshot.ts`:
- Line 37: Replace the time-dependent Date.now() defaults used for the asOf
property in the shared fixture definitions with a fixed, deterministic constant
(e.g., DEFAULT_AS_OF_TS) declared at the top of the file; update every
occurrence of asOf: Date.now() (the shared fixture builders in this file) to use
that constant and ensure tests that need a specific time explicitly
pass/override asOf. This keeps the shared fixtures deterministic across runs
while allowing targeted overrides where real timestamps are required.
---
Nitpick comments:
In `@src/infrastructure/adapters/FsKeyringAdapter.ts`:
- Around line 98-121: The empty catch blocks in removePrivateKey and
restoreRetiredPrivateKey swallow errors; change them to catch (err) and emit a
debug-level message including err so failures are visible during debugging:
inside removePrivateKey and restoreRetiredPrivateKey, capture the exception as
err and call a logging sink if present (e.g.
this.logger?.debug?.(`removePrivateKey failed for ${agentId}:`, err) or
this.log?.debug), otherwise fall back to console.debug(`...`, err); keep the
operations best-effort (do not rethrow) and avoid changing retirePrivateKey
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 762cbca5-9fc0-47ae-a6ba-ff06053861e4
📒 Files selected for processing (49)
CHANGELOG.mdsrc/cli/commands/analyze.tssrc/cli/commands/artifact.tssrc/cli/commands/coordination.tssrc/cli/commands/dashboard.tssrc/cli/commands/link.tssrc/cli/commands/submission.tssrc/cli/commands/suggestions.tssrc/cli/commands/traceability.tssrc/cli/validators.tssrc/domain/services/CoordinatorService.tssrc/domain/services/GuildSealService.tssrc/domain/services/IntakeService.tssrc/domain/services/SovereigntyService.tssrc/domain/services/TriageService.tssrc/domain/services/analysis/TestFileParser.tssrc/infrastructure/adapters/AnthropicLlmAdapter.tssrc/infrastructure/adapters/FsKeyringAdapter.tssrc/infrastructure/adapters/TsCompilerTestParserAdapter.tssrc/infrastructure/adapters/VaultSecretAdapter.tssrc/infrastructure/adapters/WarpRoadmapAdapter.tssrc/ports/KeyringStoragePort.tssrc/ports/RoadmapPort.tssrc/ports/SecretPort.tssrc/ports/TestParserPort.tssrc/tui/bijou/__tests__/DashboardApp.test.tssrc/tui/bijou/__tests__/integration.test.tssrc/tui/bijou/__tests__/views.test.tssrc/tui/bijou/views/dashboard-view.tssrc/tui/bijou/views/lineage-view.tssrc/tui/bijou/views/roadmap-view.tssrc/tui/bijou/views/submissions-view.tssrc/tui/render-status.tssrc/tui/view-helpers.tstest/helpers/ansi.tstest/helpers/keys.tstest/helpers/ports.tstest/helpers/snapshot.tstest/unit/CoordinatorService.POWERLEVEL.test.tstest/unit/CoordinatorService.test.tstest/unit/GuildSealService.test.tstest/unit/IntakeService.test.tstest/unit/SovereigntyService.test.tstest/unit/TestFileParser.test.tstest/unit/TriageService.test.tstest/unit/key-rotation.test.tstest/unit/multibase.test.tstest/unit/validators.test.tstest/unit/view-helpers.test.ts
- GuildSealService.generateKeypair: check keyring for existing active key before writing .sk file (prevents orphaned dual-active-key state) - GuildSealService.rotateKey: move writePrivateKeyOverwrite inside try/catch so rollback covers write failures - TsCompilerTestParserAdapter: derive ScriptKind from file extension instead of hard-coding TSX (fixes generic misparse in .ts files) - TsCompilerTestParserAdapter: distinguish namespace imports from default imports via new ImportRef.namespaceImport field - TsCompilerTestParserAdapter: filter test framework property-access chains (expect.toBe, vi.fn) from calledMethods to prevent score inflation in AST layer - test/helpers/snapshot.ts: replace Date.now() with deterministic FIXED_TS constant (2024-01-01T00:00:00Z) in all entity builders
Owner
Author
|
All actionable CodeRabbit findings addressed in b999159: Fixed (6 of 7):
Not actioned (1 of 7):
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
view-helpers.test.ts(22) andvalidators.test.ts(16)stripPrefix/indexByfrom view-helpersNumber.isFiniteguard tosliceDate(preventsRangeErroronNaN/Infinity)@deprecatedtag onSecretAdapterre-export aliassecretAdapter→secretPortparam inAnthropicLlmAdapterimport typeforRoadmapPortinWarpRoadmapAdapterKeyringStoragePort(no "file"/"path" leaking into port)mockGraphPortreturningMapinstead ofRecord(git-warp v13 API)Stats
Test plan
npm run build— zero errorsnpm run lint— cleannpm run test:local— 709/709 passSummary by CodeRabbit
Release Notes
New Features
Refactoring
Tests