feat: HMAC-SHA256 sync auth with replay protection (SHIELD, v10.6.0) #23
Conversation
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
Add --diff and --diff-limit flags to `git warp seek` that show which nodes/edges were added/removed and which properties changed between ticks, with old/new values. - WarpGraph.getStateSnapshot(): defensive copy of materialized state - computeStructuralDiff(): materialize before/after, call diffStates() - ASCII renderer: colored +/-/~ lines in Changes (baseline: ...) section - JSON payload: structuralDiff, diffBaseline, baselineTick, truncated - 8 unit tests, 7 renderer tests, 4 BATS E2E tests - SEEKDIFF milestone (4 tasks) added to roadmap, all closed
📝 WalkthroughWalkthroughAdds SEEKDIFF structural state-diffing and CLI rendering, implements SHIELD HMAC-SHA256 sync authentication with replay protection, exposes WarpGraph.getStateSnapshot, wires auth into server/client sync, updates docs/roadmap/changelog, adds extensive tests, and removes the standalone roadmap script. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Handler as Seek Handler\n(bin/warp-graph.js)
participant Warp as WarpGraph\n(src/domain/WarpGraph.js)
participant StateDiff as StateDiff\n(src/domain/services/StateDiff.js)
participant Renderer as ASCII Renderer\n(src/visualization/renderers/ascii/seek.js)
participant Auth as SyncAuthService\n(src/domain/services/SyncAuthService.js)
participant Server as HttpSyncServer\n(src/domain/services/HttpSyncServer.js)
CLI->>Handler: invoke seek (--diff [--diff-limit])
Handler->>Warp: ensure materialized state / getStateSnapshot()
Warp-->>Handler: state snapshot
Handler->>StateDiff: diffStates(baseline, current)
StateDiff-->>Handler: structural diff
Handler->>Handler: applyDiffLimit() → payload.structuralDiff
alt sync over HTTP with auth
Handler->>Auth: signSyncRequest(...) (client)
Handler->>Server: HTTP sync request with auth headers and body
Server->>Auth: verify(request) (server)
Auth-->>Server: verification result
Server-->>Handler: sync response
end
Handler->>Renderer: formatStructuralDiff(payload)
Renderer-->>Handler: formatted output
Handler-->>CLI: display seek output + structural diff
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/warp-graph.js (1)
129-178:⚠️ Potential issue | 🟡 Minor
--diffand--diff-limitflags are missing from the help text.The
HELP_TEXTunder "Seek options:" doesn't list the new--diffor--diff-limit=Nflags. Users relying on--helpwon't discover these capabilities.📝 Proposed fix
Seek options: --tick <N|+N|-N> Jump to tick N, or step forward/backward --latest Clear cursor, return to present --save <name> Save current position as named cursor --load <name> Restore a saved cursor --list List all saved cursors --drop <name> Delete a saved cursor + --diff Show structural diff (nodes/edges/props added/removed) + --diff-limit <N> Cap diff output lines (default: 2000) `;
🤖 Fix all issues with AI agents
In `@bin/warp-graph.js`:
- Around line 2319-2321: The status action for bare `seek` ignores the `--diff`
flag because `seekSpec` is not passed into handleSeekStatus; update the call
that returns handleSeekStatus({ graph, graphName, persistence, activeCursor,
ticks, maxTick, perWriter, frontierHash }) to thread the existing seekSpec
through (e.g., add seekSpec to the object) and update handleSeekStatus's
signature and internal logic to act on seekSpec (or, if intended, add a clear
warning/log when seekSpec/--diff is provided but not applicable); reference
handleSeekStatus and the variable seekSpec when making the change.
In `@src/domain/WarpGraph.js`:
- Around line 2819-2825: getStateSnapshot currently immediately returns null
when this._cachedState is falsy, skipping auto-materialization; change it so
that if this._cachedState is falsy you first check the autoMaterialize flag
(e.g., this.autoMaterialize or this._options.autoMaterialize) and if
autoMaterialize is true call and await this._ensureFreshState(), then return the
cloned state from cloneStateV5 if it exists; only return null early when
autoMaterialize is false and _cachedState remains null. Make sure to reference
getStateSnapshot, this._cachedState and this._ensureFreshState when updating the
flow.
In `@src/visualization/renderers/ascii/seek.js`:
- Around line 342-373: In buildStructuralDiffLines, the "... and X more changes"
message under the branch where totalEntries > maxLines undercounts and omits the
diff-limit hint when payload.truncated is true; change the logic so the
"remaining" calculation uses the full payload totalChanges (or totalChanges -
shown) instead of entries.length - maxLines and include the muted "(use
--diff-limit to increase)" hint when truncated === true; update the branch that
currently checks if (totalEntries > maxLines) to compute remaining =
totalChanges - shown (or totalChanges - shownChanges if shown is different) and
push the appropriate colors.muted message that includes the diff-limit guidance
when truncated is true.
🧹 Nitpick comments (3)
src/visualization/renderers/ascii/seek.js (1)
268-295: Avoid magic number for diff line cap.Keep the view limit in sync with the exported constant to prevent divergence.
♻️ Suggested tweak
- const sdLines = buildStructuralDiffLines(payload, 20); + const sdLines = buildStructuralDiffLines(payload, MAX_DIFF_LINES);bin/warp-graph.js (2)
2520-2537: Double materialization:computeStructuralDiffmaterializes atcurrentTick, then the caller materializes again.In all three action paths (latest, load, tick),
computeStructuralDiffalready callsgraph.materialize({ ceiling: currentTick })at line 2532. Then the caller immediately re-materializes at the same ceiling (e.g., line 2293 for tick, line 2260 for load, line 2210 for latest). The graph is effectively materialized at the target tick twice.Consider having
computeStructuralDiffreturn the after-state node/edge counts so the caller can skip the redundant materialization+getNodes+getEdges, or document that the graph is left materialized atcurrentTickso the caller can rely on that.
2548-2574: Comment says "proportionally" but truncation is sequential/greedy.Line 2558 says "Truncate each category proportionally" but the
capclosure consumes from a sharedremainingbudget in fixed order: nodes.added → nodes.removed → edges.added → edges.removed → props.set → props.removed. A large first category will starve later ones entirely.If that's the intended behavior (prioritize showing node changes over edge/prop changes), update the comment to say "sequentially" or "in priority order." If proportional distribution is desired, the algorithm needs a different approach.
📝 Fix the misleading comment
- // Truncate each category proportionally, keeping order + // Truncate sequentially (nodes → edges → props), keeping sort order within each category
B1: reject --diff-limit=0 (must be positive integer) P1-P2: skip redundant re-materialization when --diff already materialized P3: short-circuit empty diff when prevTick === currentTick U1: add --diff/--diff-limit to help text U2: reject --diff on non-navigating actions (save/drop/list/clear-cache) U3: combined truncation message when display + data limits both active U4/N3: fix misleading "proportionally" comment to "greedy" N1: proper JSDoc type for getStateSnapshot N2: remove dead formatStructuralDiff on status render path N4: hoist MAX_DIFF_LINES constant, eliminate magic number N5-N6: proper JSDoc types for diff renderer params N7-N8: 8 new tests (4 BATS + 3 renderer + 1 truncation edge case)
- Fix JSDoc import path for WarpStateV5 in CLI typedef (bin/ -> ../src/) - Add structural diff fields to SeekPayload typedef - Extract buildTruncationHint() to stay under ESLint complexity limit - Add @type {*} casts in test fixtures for partial payloads - Restore JSDoc on buildStructuralDiffLines after refactor
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bin/warp-graph.js`:
- Around line 2084-2091: The DIFF_ACTIONS set currently contains 'status' so
passing --diff with a seek/status action (spec.action handled by
handleSeekStatus) is accepted but no diff is produced; either remove 'status'
from DIFF_ACTIONS so usageError is thrown for spec.action === 'status', or
implement diff generation in handleSeekStatus so the --diff flag is honored.
Locate the DIFF_ACTIONS definition and the condition that throws usageError, and
then either (A) delete 'status' from DIFF_ACTIONS (ensuring usageError(`--diff
cannot be used with --${spec.action}`) runs for seek/status) or (B) update
handleSeekStatus to detect spec.diff and call the same diff computation/output
path used by the tick/load handlers so --diff produces structural output. Ensure
tests or manual runs validate the chosen behavior.
In `@src/visualization/renderers/ascii/seek.js`:
- Around line 335-341: Remove the orphaned JSDoc block that precedes the current
buildStructuralDiffLines implementation (the leftover comment referring to
buildTruncationHint around the earlier comment block), so delete the detached
comment lines describing "Builds structural diff lines for the seek dashboard"
(the JSDoc at lines just above buildStructuralDiffLines) and keep the valid
JSDoc already present above the actual buildStructuralDiffLines function; ensure
no other comment content needed by buildTruncationHint or
buildStructuralDiffLines is removed.
🧹 Nitpick comments (1)
src/visualization/renderers/ascii/seek.js (1)
20-20: Consider splitting theSeekPayloadtypedef across multiple lines for readability.This single-line typedef with 12+ fields is difficult to scan and maintain. A multi-line
@typedefwith individual@propertyannotations would be easier to review and extend.
Previously returned null when no cached state existed, even with autoMaterialize enabled. Now delegates to _ensureFreshState() which triggers auto-materialization when the flag is set. Addresses PR #23 review comment (coderabbit).
DIFF_ACTIONS no longer includes 'status'. Running `git warp seek --diff` without --tick/--latest/--load now fails fast with a clear usage error instead of silently ignoring the flag. Addresses PR #23 review comment (coderabbit).
Leftover from extracting the function out of buildStructuralDiffLines. The actual function already has its own JSDoc at lines 356-360. Addresses PR #23 review comment (coderabbit).
Addresses PR #23 review nitpick (coderabbit).
The server may reset (ECONNRESET) or break the pipe (EPIPE) depending on platform timing. Both are valid rejection behaviors.
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/warp-graph.js (1)
2025-2088:⚠️ Potential issue | 🟡 Minor
--diff-limitis silently ignored without--diff.Right now the flag can be provided alone and becomes a no-op. Consider tracking explicit usage and rejecting when
--diffisn’t set.🐛 Proposed fix
function parseSeekArgs(args) { /** `@type` {SeekSpec} */ const spec = { action: 'status', // status, tick, latest, save, load, list, drop, clear-cache tickValue: null, name: null, noPersistentCache: false, diff: false, diffLimit: 2000, }; + let diffLimitProvided = false; for (let i = 0; i < args.length; i++) { const arg = args[i]; @@ } else if (arg === '--diff-limit' || arg.startsWith('--diff-limit=')) { handleDiffLimitFlag(arg, args, i, spec); + diffLimitProvided = true; if (arg === '--diff-limit') { i += 1; } @@ const DIFF_ACTIONS = new Set(['tick', 'latest', 'load']); if (spec.diff && !DIFF_ACTIONS.has(spec.action)) { throw usageError(`--diff cannot be used with --${spec.action}`); } + if (diffLimitProvided && !spec.diff) { + throw usageError('--diff-limit requires --diff'); + } return spec; }
🤖 Fix all issues with AI agents
In `@bin/warp-graph.js`:
- Around line 1967-1989: The handleDiffLimitFlag function currently uses
parseInt which silently truncates floats (e.g., "1.5" -> 1); change the parsing
to use Number(raw) (or parseFloat(raw)) and then validate that the result is a
finite integer and >= 1 before assigning to spec.diffLimit so inputs like
"--diff-limit=1.5" are rejected; update validation to check Number.isFinite(n)
&& Number.isInteger(n) && n >= 1 and keep the existing usageError on invalid
values.
In `@src/visualization/renderers/ascii/seek.js`:
- Around line 354-372: In buildTruncationHint, clamp the computed "more changes"
numbers to avoid negatives: when computing omitted for the first branch (inside
buildTruncationHint) use a non-negative clamp like Math.max(0, (totalChanges ||
0) - shown) and likewise clamp the third-branch expression ((totalChanges || 0)
- (shownChanges || 0)); also ensure the second branch uses Math.max(0,
totalEntries - maxLines) so all returned counts are never negative.
Add SyncAuthService with canonical payload signing, nonce-based replay defense, key-id rotation support, and enforce/log-only upgrade mode. 65 new tests (42 unit + 15 integration + 8 E2E), zero regressions.
flyingrobots
changed the title
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/domain/services/HttpSyncServer.js`:
- Around line 183-195: The initAuth function currently trusts any truthy
auth.mode and returns it, causing invalid strings to be treated like 'log-only';
update initAuth to validate auth.mode against the allowed values 'enforce' and
'log-only' (e.g., check if auth.mode === 'enforce' or auth.mode === 'log-only')
and only set authMode to one of those; if auth.mode is missing or invalid,
choose a safe default (for example null or 'enforce' per project policy) before
returning { auth: new SyncAuthService(auth), authMode: validatedMode } to
prevent accidental log-only behavior.
In `@src/domain/services/SyncAuthService.js`:
- Around line 329-376: In verify(), move the call to this._reserveNonce(nonce)
so it runs after the await this._verifySignature(...) succeeds: keep header,
freshness, key resolution and signature verification in order, then call
_reserveNonce(nonce) and handle its failure (increment
this._metrics.authFailCount, log 'sync auth: replay detected' with { keyId,
nonce } and return the nonceResult) only after sigResult.ok; update the success
path to return { ok: true } only when signature verification and nonce
reservation both succeed. Reference: verify, _reserveNonce, _resolveKey,
_verifySignature.
…t guards, truncation clamping (v10.6.1)
Release Preflight
If you tag this commit as |
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
serve({ auth }): Server-side auth via{ keys, mode }— instantiatesSyncAuthServiceinternallysyncWith(url, { auth }): Client-side signing via{ secret, keyId }— signs each outgoing request with fresh nonceSyncAuthServerOptions,SyncAuthClientOptionsinindex.d.tsNew files
src/domain/services/SyncAuthService.jstest/unit/domain/services/SyncAuthService.test.jstest/unit/domain/services/HttpSyncServer.auth.test.jstest/unit/domain/WarpGraph.syncAuth.test.jsModified files
src/domain/services/HttpSyncServer.jscheckBodySize(), added auth step via_checkAuth()src/domain/WarpGraph.jsauthtoserve()/syncWith(), extractedbuildSyncAuthHeaders()index.d.tsSyncAuthServerOptions,SyncAuthClientOptions, updated method signaturesSECURITY.mddocs/GUIDE.mdCHANGELOG.mdROADMAP.mdpackage.jsonTest plan
SyncAuthService.test.js— 42 tests (canonical payload, path canonicalization, signing, all reject/happy paths, metrics, constructor)HttpSyncServer.auth.test.js— 15 tests (enforce, log-only, backward compat, DoS guard ordering)WarpGraph.syncAuth.test.js— 8 E2E tests (enforce+matching key, no auth, wrong secret, wrong key-id, log-only, no-auth server, fresh nonces, multi-key)tsc --noEmit)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores