Extract core runtime utilities for script reuse#300
Extract core runtime utilities for script reuse#300HardlyDifficult wants to merge 6 commits intomainfrom
Conversation
Add opinionated core utilities for continuous loops, sync state persistence, and SHA-256 hashing so downstream repos can remove duplicated script infrastructure. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @HardlyDifficult's task —— View job Code Review Complete ✅Todo List
Review FeedbackOverall AssessmentThis PR introduces well-structured utility functions that appear to solve legitimate runtime needs for Canton scripts. The code follows TypeScript best practices and maintains good separation of concerns. Code Quality & Best Practices ✅Strengths:
Potential Issues & Suggestions1. File System Race Conditions (
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTwo new utility modules are introduced: a continuous loop runner for periodic task execution with graceful shutdown handling, and a sync state tracker for persisting ledger synchronization progress to disk with atomic writes and in-memory caching. Both utilities are exported through the core utils index. Changes
Sequence Diagram(s)sequenceDiagram
participant Process
participant ContinuousLoop
participant RunCycle
participant Filesystem
Process->>ContinuousLoop: start runContinuousLoop(options)
ContinuousLoop->>Process: attach SIGINT/SIGTERM handlers
loop every intervalSeconds
ContinuousLoop->>RunCycle: call runCycle(isShutdownRequested)
RunCycle-->>ContinuousLoop: completes / throws
alt error
ContinuousLoop->>ContinuousLoop: call onCycleError(error)
end
ContinuousLoop->>ContinuousLoop: await interval sleep (resolvable)
alt SIGINT/SIGTERM received
Process->>ContinuousLoop: signal handler sets shutdownRequested and resolves sleep
ContinuousLoop->>ContinuousLoop: break loop
end
end
ContinuousLoop->>Process: remove signal handlers
ContinuousLoop->>RunCycle: call onShutdown() (if provided) and await
ContinuousLoop-->>Process: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Comment |
![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
🤖 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/core/utils/continuous-loop.ts`:
- Around line 20-68: The runContinuousLoop function must validate the
intervalSeconds input to avoid hot loops; at the top of runContinuousLoop
(before any await or setTimeout use) assert that intervalSeconds is a finite
number > 0 (e.g., Number.isFinite(intervalSeconds) && intervalSeconds > 0) and
if not, throw a RangeError with a clear message; this prevents passing invalid
values into setTimeout (used where sleepTimeout is set) and avoids
immediate-resolving timers that create CPU-pegging loops.
In `@src/core/utils/index.ts`:
- Around line 1-5: This change only updated exports (e.g., waitForCondition,
SyncStateTracker, hashValue) but the PR did not run the full verification suite;
run "npm run lint && npm run build && npm test" locally, fix any lint/build/test
failures, and push the fixes so CI passes; also ensure the repository
CI/workflow is configured to run lint, build, and test for PRs and include test
logs in the PR description so reviewers can confirm all checks passed.
In `@src/core/utils/sync-state-tracker.ts`:
- Around line 46-64: The constructor currently assumes an existing directory is
writable and sets directoryWritable = true, which makes isWritable() return true
even when writes will fail; modify the SyncStateTracker constructor to perform
an explicit write-access check after ensuring the directory exists: after the
mkdirSync attempt (or if fs.existsSync(stateDir) is true) call
fs.accessSync(stateDir, fs.constants.W_OK) inside a try/catch and set isWritable
= false on any error so directoryWritable correctly reflects writability; keep
stateFilePath assignment and isWritable() method unchanged but ensure
updateState() will rely on the corrected directoryWritable flag.
| export async function runContinuousLoop(options: ContinuousLoopOptions): Promise<void> { | ||
| const { intervalSeconds, runCycle, onShutdown, onCycleError } = options; | ||
|
|
||
| let shutdownRequested = false; | ||
| let sleepResolve: (() => void) | null = null; | ||
| let sleepTimeout: NodeJS.Timeout | null = null; | ||
|
|
||
| const handleShutdown = (_signal: string): void => { | ||
| shutdownRequested = true; | ||
| if (sleepTimeout) { | ||
| clearTimeout(sleepTimeout); | ||
| sleepTimeout = null; | ||
| } | ||
| if (sleepResolve) { | ||
| sleepResolve(); | ||
| sleepResolve = null; | ||
| } | ||
| }; | ||
|
|
||
| const sigintHandler = (): void => handleShutdown('SIGINT'); | ||
| const sigtermHandler = (): void => handleShutdown('SIGTERM'); | ||
|
|
||
| process.on('SIGINT', sigintHandler); | ||
| process.on('SIGTERM', sigtermHandler); | ||
|
|
||
| const isShutdownRequested = (): boolean => shutdownRequested; | ||
|
|
||
| try { | ||
| for (;;) { | ||
| if (isShutdownRequested()) { | ||
| break; | ||
| } | ||
| try { | ||
| await runCycle(isShutdownRequested); | ||
| } catch (error) { | ||
| onCycleError?.(error); | ||
| } | ||
|
|
||
| if (isShutdownRequested()) { | ||
| break; | ||
| } | ||
| await new Promise<void>((resolve) => { | ||
| sleepResolve = resolve; | ||
| sleepTimeout = setTimeout(() => { | ||
| sleepTimeout = null; | ||
| sleepResolve = null; | ||
| resolve(); | ||
| }, intervalSeconds * 1000); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine its structure
find . -name "continuous-loop.ts" -type fRepository: Fairmint/canton-node-sdk
Length of output: 103
🏁 Script executed:
# Look at the ContinuousLoopOptions definition
rg -n "interface ContinuousLoopOptions|type ContinuousLoopOptions" -A 10 --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 1261
🏁 Script executed:
# Check the full content of the continuous-loop.ts file
cat -n src/core/utils/continuous-loop.ts 2>/dev/null || echo "File not found"Repository: Fairmint/canton-node-sdk
Length of output: 2852
🏁 Script executed:
# Find all call sites of runContinuousLoop
rg -nP 'runContinuousLoop\s*\(' -B 2 -A 3 --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 562
🏁 Script executed:
# Search for imports of runContinuousLoop in the codebase
rg -n "runContinuousLoop" --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 302
🏁 Script executed:
# Also check if there are any tests that might show expected usage
find . -name "*.test.ts" -o -name "*.spec.ts" | head -20Repository: Fairmint/canton-node-sdk
Length of output: 1148
🏁 Script executed:
# Search for continuous-loop usage in tests
rg -n "continuous-loop\|runContinuousLoop" --type=ts -lRepository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Search for files that import from the utils index where runContinuousLoop is exported
rg -n "from.*utils|from.*core" --type=ts | grep -v node_modules | head -30Repository: Fairmint/canton-node-sdk
Length of output: 3792
🏁 Script executed:
# Look for any import of ContinuousLoopOptions as well
rg -n "ContinuousLoopOptions" --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 379
🏁 Script executed:
# Check git to see if this function has recent usage
git log --oneline -p --all -- src/core/utils/continuous-loop.ts 2>/dev/null | head -100Repository: Fairmint/canton-node-sdk
Length of output: 2649
🏁 Script executed:
# Check if there are any tests for continuous-loop
find . -name "*continuous*" -type f | grep -i testRepository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Also check if the function appears anywhere else in comments, docs, or examples
rg -i "continuous.*loop|runContinuousLoop" --type=md --type=txtRepository: Fairmint/canton-node-sdk
Length of output: 50
Add validation for intervalSeconds to prevent hot loops from invalid values.
The intervalSeconds parameter lacks type-level constraints and is passed directly to setTimeout at line 67. Invalid inputs (≤0, NaN, or non-finite) would cause setTimeout to resolve immediately, creating a CPU-pegging hot loop. Add a guard at function entry:
Proposed fix
export async function runContinuousLoop(options: ContinuousLoopOptions): Promise<void> {
const { intervalSeconds, runCycle, onShutdown, onCycleError } = options;
+ if (!Number.isFinite(intervalSeconds) || intervalSeconds <= 0) {
+ throw new Error('intervalSeconds must be a positive, finite number');
+ }📝 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.
| export async function runContinuousLoop(options: ContinuousLoopOptions): Promise<void> { | |
| const { intervalSeconds, runCycle, onShutdown, onCycleError } = options; | |
| let shutdownRequested = false; | |
| let sleepResolve: (() => void) | null = null; | |
| let sleepTimeout: NodeJS.Timeout | null = null; | |
| const handleShutdown = (_signal: string): void => { | |
| shutdownRequested = true; | |
| if (sleepTimeout) { | |
| clearTimeout(sleepTimeout); | |
| sleepTimeout = null; | |
| } | |
| if (sleepResolve) { | |
| sleepResolve(); | |
| sleepResolve = null; | |
| } | |
| }; | |
| const sigintHandler = (): void => handleShutdown('SIGINT'); | |
| const sigtermHandler = (): void => handleShutdown('SIGTERM'); | |
| process.on('SIGINT', sigintHandler); | |
| process.on('SIGTERM', sigtermHandler); | |
| const isShutdownRequested = (): boolean => shutdownRequested; | |
| try { | |
| for (;;) { | |
| if (isShutdownRequested()) { | |
| break; | |
| } | |
| try { | |
| await runCycle(isShutdownRequested); | |
| } catch (error) { | |
| onCycleError?.(error); | |
| } | |
| if (isShutdownRequested()) { | |
| break; | |
| } | |
| await new Promise<void>((resolve) => { | |
| sleepResolve = resolve; | |
| sleepTimeout = setTimeout(() => { | |
| sleepTimeout = null; | |
| sleepResolve = null; | |
| resolve(); | |
| }, intervalSeconds * 1000); | |
| }); | |
| export async function runContinuousLoop(options: ContinuousLoopOptions): Promise<void> { | |
| const { intervalSeconds, runCycle, onShutdown, onCycleError } = options; | |
| if (!Number.isFinite(intervalSeconds) || intervalSeconds <= 0) { | |
| throw new Error('intervalSeconds must be a positive, finite number'); | |
| } | |
| let shutdownRequested = false; | |
| let sleepResolve: (() => void) | null = null; | |
| let sleepTimeout: NodeJS.Timeout | null = null; | |
| const handleShutdown = (_signal: string): void => { | |
| shutdownRequested = true; | |
| if (sleepTimeout) { | |
| clearTimeout(sleepTimeout); | |
| sleepTimeout = null; | |
| } | |
| if (sleepResolve) { | |
| sleepResolve(); | |
| sleepResolve = null; | |
| } | |
| }; | |
| const sigintHandler = (): void => handleShutdown('SIGINT'); | |
| const sigtermHandler = (): void => handleShutdown('SIGTERM'); | |
| process.on('SIGINT', sigintHandler); | |
| process.on('SIGTERM', sigtermHandler); | |
| const isShutdownRequested = (): boolean => shutdownRequested; | |
| try { | |
| for (;;) { | |
| if (isShutdownRequested()) { | |
| break; | |
| } | |
| try { | |
| await runCycle(isShutdownRequested); | |
| } catch (error) { | |
| onCycleError?.(error); | |
| } | |
| if (isShutdownRequested()) { | |
| break; | |
| } | |
| await new Promise<void>((resolve) => { | |
| sleepResolve = resolve; | |
| sleepTimeout = setTimeout(() => { | |
| sleepTimeout = null; | |
| sleepResolve = null; | |
| resolve(); | |
| }, intervalSeconds * 1000); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/continuous-loop.ts` around lines 20 - 68, The
runContinuousLoop function must validate the intervalSeconds input to avoid hot
loops; at the top of runContinuousLoop (before any await or setTimeout use)
assert that intervalSeconds is a finite number > 0 (e.g.,
Number.isFinite(intervalSeconds) && intervalSeconds > 0) and if not, throw a
RangeError with a clear message; this prevents passing invalid values into
setTimeout (used where sleepTimeout is set) and avoids immediate-resolving
timers that create CPU-pegging loops.
src/core/utils/index.ts
Outdated
| export { waitForCondition, type WaitForConditionOptions } from './polling'; | ||
| export { extractString, hasStringProperty, isNonEmptyString, isNumber, isRecord, isString } from './type-guards'; | ||
| export { runContinuousLoop, type ContinuousLoopOptions } from './continuous-loop'; | ||
| export { SyncStateTracker, type SyncState, type SyncStateTrackerOptions } from './sync-state-tracker'; | ||
| export { hashValue } from './hash'; |
There was a problem hiding this comment.
Please run the full required checks for this change.
PR notes only lint/build ran and tests were not completed; guideline requires full lint/build/test.
As per coding guidelines, "Run npm run lint && npm run build && npm test after any code change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/index.ts` around lines 1 - 5, This change only updated exports
(e.g., waitForCondition, SyncStateTracker, hashValue) but the PR did not run the
full verification suite; run "npm run lint && npm run build && npm test"
locally, fix any lint/build/test failures, and push the fixes so CI passes; also
ensure the repository CI/workflow is configured to run lint, build, and test for
PRs and include test logs in the PR description so reviewers can confirm all
checks passed.
| constructor(options: SyncStateTrackerOptions) { | ||
| const stateDir = options.stateDirectory ?? getDefaultStateDirectory(); | ||
|
|
||
| let isWritable = true; | ||
| if (!fs.existsSync(stateDir)) { | ||
| try { | ||
| fs.mkdirSync(stateDir, { recursive: true }); | ||
| } catch { | ||
| isWritable = false; | ||
| } | ||
| } | ||
|
|
||
| this.directoryWritable = isWritable; | ||
| this.stateFilePath = path.join(stateDir, `${options.network}-${options.provider}.json`); | ||
| } | ||
|
|
||
| isWritable(): boolean { | ||
| return this.directoryWritable; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's view the actual file to see the current state
head -100 src/core/utils/sync-state-tracker.tsRepository: Fairmint/canton-node-sdk
Length of output: 2644
🏁 Script executed:
#!/bin/bash
# Find all usages of isWritable() to understand downstream reliance
rg -nP '\.isWritable\s*\(' -C3 --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Try different patterns to find isWritable() usages
rg 'isWritable' --type=ts -nRepository: Fairmint/canton-node-sdk
Length of output: 349
🏁 Script executed:
#!/bin/bash
# View the rest of the file to see full updateState method and any other relevant code
tail -50 src/core/utils/sync-state-tracker.tsRepository: Fairmint/canton-node-sdk
Length of output: 1301
Add explicit write permission check for existing directories to prevent silent persistence failures.
If the directory exists but lacks write permissions, isWritable() will incorrectly return true, causing updateState() to silently fail when attempting file writes. Add an explicit access check using fs.accessSync() with fs.constants.W_OK after the mkdir attempt.
Proposed fix
constructor(options: SyncStateTrackerOptions) {
const stateDir = options.stateDirectory ?? getDefaultStateDirectory();
let isWritable = true;
if (!fs.existsSync(stateDir)) {
try {
fs.mkdirSync(stateDir, { recursive: true });
} catch {
isWritable = false;
}
}
+
+ if (isWritable) {
+ try {
+ fs.accessSync(stateDir, fs.constants.W_OK);
+ } catch {
+ isWritable = false;
+ }
+ }
this.directoryWritable = isWritable;
this.stateFilePath = path.join(stateDir, `${options.network}-${options.provider}.json`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/sync-state-tracker.ts` around lines 46 - 64, The constructor
currently assumes an existing directory is writable and sets directoryWritable =
true, which makes isWritable() return true even when writes will fail; modify
the SyncStateTracker constructor to perform an explicit write-access check after
ensuring the directory exists: after the mkdirSync attempt (or if
fs.existsSync(stateDir) is true) call fs.accessSync(stateDir, fs.constants.W_OK)
inside a try/catch and set isWritable = false on any error so directoryWritable
correctly reflects writability; keep stateFilePath assignment and isWritable()
method unchanged but ensure updateState() will rely on the corrected
directoryWritable flag.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
resetState() only cleared the in-memory cache but left the state file on disk. Subsequent getState() calls would re-read the stale file, making resetState() effectively a no-op. Now unlinkSync the file too.
This comment has been minimized.
This comment has been minimized.
|
Claude finished @HardlyDifficult's task —— View job Code Review Complete ✅Todo List
Review FeedbackOverall AssessmentThis PR introduces well-structured utility functions that appear to solve legitimate runtime needs for Canton scripts. The code follows TypeScript best practices and maintains good separation of concerns. Code Quality & Best Practices ✅Strengths:
Potential Issues & Suggestions1. Input Validation Missing (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/utils/sync-state-tracker.ts`:
- Line 59: The code currently builds this.stateFilePath by interpolating
options.network and options.provider directly, which allows path traversal;
update the constructor/initialization that sets stateFilePath to sanitize and
validate options.network and options.provider before joining with stateDir:
either strip path separators and ".." (e.g., replace / and \ with '-', reject
empty or '.'/'..'), or enforce a strict whitelist regex (e.g.,
/^[a-zA-Z0-9._-]+$/) and throw on invalid values; alternatively use
path.basename on each value to drop directory components and then join with
stateDir and the `.json` suffix so stateFilePath always stays inside stateDir.
Ensure any validation error surfaces clearly from the constructor that sets
stateFilePath.
---
Duplicate comments:
In `@src/core/utils/sync-state-tracker.ts`:
- Around line 46-60: The constructor currently assumes existing state
directories are writable; update the SyncStateTracker constructor to explicitly
check write permissions for the resolved stateDir (use fs.access or
fs.accessSync with fs.constants.W_OK) and set this.directoryWritable = false if
the access check fails or throws; keep the existing mkdirSync logic for
non-existent dirs but after ensuring existence always perform the
write-permission check before setting directoryWritable and ensure updateState()
relies on this flag to avoid silent failures (refer to constructor,
SyncStateTrackerOptions, directoryWritable, stateFilePath, and updateState).
| } | ||
|
|
||
| this.directoryWritable = isWritable; | ||
| this.stateFilePath = path.join(stateDir, `${options.network}-${options.provider}.json`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the sync-state-tracker.ts file
cat -n src/core/utils/sync-state-tracker.tsRepository: Fairmint/canton-node-sdk
Length of output: 5026
🏁 Script executed:
# Search for type definitions of SyncStateTrackerOptions
rg -nP 'interface\s+SyncStateTrackerOptions|type\s+SyncStateTrackerOptions' --type=ts -A 10Repository: Fairmint/canton-node-sdk
Length of output: 1107
🏁 Script executed:
# Search for SyncStateTracker instantiations
rg -nP 'new\s+SyncStateTracker|SyncStateTracker\(' --type=ts -B 3 -A 3Repository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Check for any sanitization or validation functions already in the codebase
rg -nP 'sanitize|validate.*path|pathTraversal' --type=ts -iRepository: Fairmint/canton-node-sdk
Length of output: 2372
🏁 Script executed:
# Search for SyncStateTracker references with broader pattern
rg -nP 'SyncStateTracker' --type=ts -B 2 -A 2Repository: Fairmint/canton-node-sdk
Length of output: 1581
🏁 Script executed:
# Look in test files to see how it's constructed
find . -name '*.test.ts' -o -name '*.spec.ts' | xargs grep -l 'SyncStateTracker' 2>/dev/null | head -5Repository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Check if there are any tests for sync-state-tracker
fd -e 'test.ts' -e 'spec.ts' sync-state-trackerRepository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Search for instantiation patterns more broadly
rg -nP 'new\s+SyncStateTracker|SyncStateTracker\s*\(' --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Find all test files in the repository
fd -e 'test.ts' -e 'spec.ts' | head -20Repository: Fairmint/canton-node-sdk
Length of output: 1108
🏁 Script executed:
# Check if sync-state-tracker has any tests at all
find . -path '*/test*' -o -path '*/spec*' | xargs grep -l 'SyncStateTracker' 2>/dev/nullRepository: Fairmint/canton-node-sdk
Length of output: 52
🏁 Script executed:
# Search in the entire codebase for where SyncStateTracker is instantiated
rg -nP 'SyncStateTracker\s*\{|new\s+SyncStateTracker' --type=ts -A 5Repository: Fairmint/canton-node-sdk
Length of output: 527
🏁 Script executed:
# Let's verify what path.join does with path traversal attempts
node <<'EOF'
const path = require('path');
// Test cases
const stateDir = '/home/user/.canton-sync-state';
// Case 1: Normal input
const normal = path.join(stateDir, 'mainnet-intellect.json');
console.log('Normal:', normal);
// Case 2: Traversal attempt
const traversal = path.join(stateDir, '../../etc-passwd.json');
console.log('Traversal attempt:', traversal);
// Case 3: Another traversal
const traversal2 = path.join(stateDir, '../../../sensitive.json');
console.log('Traversal attempt 2:', traversal2);
// Case 4: What if network contains traversal
const network = '../../etc';
const provider = 'passwd';
const result = path.join(stateDir, `${network}-${provider}.json`);
console.log('Network with traversal:', result);
EOFRepository: Fairmint/canton-node-sdk
Length of output: 252
🏁 Script executed:
# Check if there are any validation or sanitization of network/provider elsewhere
rg -nP 'network|provider' --type=ts -B 2 -A 2 src/core/ | head -100Repository: Fairmint/canton-node-sdk
Length of output: 5798
🏁 Script executed:
# Look for how these values might be passed from external sources
rg -nP 'SyncStateTrackerOptions|network.*provider' --type=ts -B 5 -A 5 | head -80Repository: Fairmint/canton-node-sdk
Length of output: 5780
Sanitize network and provider inputs to prevent path traversal.
The network and provider values are directly interpolated into the file path without validation. Node.js path.join() does not prevent path traversal sequences; malicious input like ../../etc-passwd would escape the intended stateDir. Since this is exported as a public API, downstream consumers could pass untrusted input, potentially writing state files outside the intended directory.
🛡️ Proposed fix to sanitize inputs
constructor(options: SyncStateTrackerOptions) {
const stateDir = options.stateDirectory ?? getDefaultStateDirectory();
+
+ // Sanitize inputs to prevent path traversal
+ const sanitize = (s: string): string => s.replace(/[^a-zA-Z0-9_-]/g, '_');
+ const safeNetwork = sanitize(options.network);
+ const safeProvider = sanitize(options.provider);
let isWritable = true;
if (!fs.existsSync(stateDir)) {
try {
fs.mkdirSync(stateDir, { recursive: true });
} catch {
isWritable = false;
}
}
this.directoryWritable = isWritable;
- this.stateFilePath = path.join(stateDir, `${options.network}-${options.provider}.json`);
+ this.stateFilePath = path.join(stateDir, `${safeNetwork}-${safeProvider}.json`);
}📝 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.
| this.stateFilePath = path.join(stateDir, `${options.network}-${options.provider}.json`); | |
| constructor(options: SyncStateTrackerOptions) { | |
| const stateDir = options.stateDirectory ?? getDefaultStateDirectory(); | |
| // Sanitize inputs to prevent path traversal | |
| const sanitize = (s: string): string => s.replace(/[^a-zA-Z0-9_-]/g, '_'); | |
| const safeNetwork = sanitize(options.network); | |
| const safeProvider = sanitize(options.provider); | |
| let isWritable = true; | |
| if (!fs.existsSync(stateDir)) { | |
| try { | |
| fs.mkdirSync(stateDir, { recursive: true }); | |
| } catch { | |
| isWritable = false; | |
| } | |
| } | |
| if (isWritable) { | |
| try { | |
| fs.accessSync(stateDir, fs.constants.W_OK); | |
| } catch { | |
| isWritable = false; | |
| } | |
| } | |
| this.directoryWritable = isWritable; | |
| this.stateFilePath = path.join(stateDir, `${safeNetwork}-${safeProvider}.json`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/sync-state-tracker.ts` at line 59, The code currently builds
this.stateFilePath by interpolating options.network and options.provider
directly, which allows path traversal; update the constructor/initialization
that sets stateFilePath to sanitize and validate options.network and
options.provider before joining with stateDir: either strip path separators and
".." (e.g., replace / and \ with '-', reject empty or '.'/'..'), or enforce a
strict whitelist regex (e.g., /^[a-zA-Z0-9._-]+$/) and throw on invalid values;
alternatively use path.basename on each value to drop directory components and
then join with stateDir and the `.json` suffix so stateFilePath always stays
inside stateDir. Ensure any validation error surfaces clearly from the
constructor that sets stateFilePath.
…dateState Move this.cachedState assignment before the try block so in-memory progress is preserved when filesystem writes fail, matching the stated intent of the catch block comment.
|
Claude finished @HardlyDifficult's task —— View job Code Review Complete ✅Todo List
Review FeedbackOverall AssessmentThis PR introduces well-structured utility functions that appear to solve legitimate runtime needs for Canton scripts. The code follows TypeScript best practices and maintains good separation of concerns. Code Quality & Best Practices ✅Strengths:
Potential Issues & Suggestions1. Input Validation Missing (
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/utils/sync-state-tracker.ts`:
- Around line 1-3: Run the full CI checks locally for the change in
src/core/utils/sync-state-tracker.ts: execute "npm run lint && npm run build &&
npm test" and fix any lint/build/test failures that appear from the edits to the
imports or related code, then push the fixes and update the PR status to
indicate the checks passed; specifically run the lint/build/test sequence after
modifying sync-state-tracker.ts (the module importing fs, os, path) and include
the resulting test output or remediation commits in the PR.
---
Duplicate comments:
In `@src/core/utils/sync-state-tracker.ts`:
- Around line 46-59: The constructor in SyncStateTracker currently sets
directoryWritable to true without verifying write permission; after the
mkdirSync attempt (or if the directory already exists) perform an explicit
write-access check using fs.accessSync(dir, fs.constants.W_OK) inside a
try/catch and set isWritable = false if access throws, so directoryWritable
reflects real writability before updateState() uses stateFilePath; ensure you
reference the existing symbols (constructor, SyncStateTrackerOptions,
directoryWritable, stateFilePath, updateState) and only change the logic around
the mkdir/permission branch to catch and handle access errors.
- Around line 58-60: The constructor of SyncStateTracker builds stateFilePath
from untrusted options.network and options.provider, allowing path traversal;
update SyncStateTracker to validate/sanitize those inputs before joining (e.g.,
allowlist or normalize and strip path separators, or use path.basename/encode to
remove ../ sequences) so stateFilePath = path.join(stateDir,
`${safeNetwork}-${safeProvider}.json`); apply the same sanitization where
SyncStateTracker is instantiated and ensure directoryWritable logic still
references the sanitized values.
| import * as fs from 'fs'; | ||
| import * as os from 'os'; | ||
| import * as path from 'path'; |
There was a problem hiding this comment.
Run the full test command sequence for this change.
npm test is still pending, but the guidelines require running lint/build/test after any code change. Please run the full sequence and update the PR status.
As per coding guidelines "Run npm run lint && npm run build && npm test after any code change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/sync-state-tracker.ts` around lines 1 - 3, Run the full CI
checks locally for the change in src/core/utils/sync-state-tracker.ts: execute
"npm run lint && npm run build && npm test" and fix any lint/build/test failures
that appear from the edits to the imports or related code, then push the fixes
and update the PR status to indicate the checks passed; specifically run the
lint/build/test sequence after modifying sync-state-tracker.ts (the module
importing fs, os, path) and include the resulting test output or remediation
commits in the PR.
Remove hash export from node-sdk core utils so hashing remains a local canton concern for now. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @HardlyDifficult's task —— View job Code Review Complete ✅Todo List
Review FeedbackOverall AssessmentThis PR introduces well-structured utility functions that appear to solve legitimate runtime needs for Canton scripts. The code follows TypeScript best practices and maintains good separation of concerns. Code Quality & Best Practices ✅Strengths:
Potential Issues & Suggestions1. Input Validation Missing (
|
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/utils/index.ts`:
- Around line 3-4: The PR didn't run the required verification steps for the
exports in src/core/utils/index.ts (symbols: runContinuousLoop,
ContinuousLoopOptions, SyncStateTracker, SyncState, SyncStateTrackerOptions);
run the full suite locally: npm run lint && npm run build && npm test, fix any
lint/build/test failures that appear (adjust code or tests as needed), commit
the fixes, and update the PR with the test/lint/build results and any changes
made so reviewers can validate the pass.
Clear cached state and state file via resetState() when lastSuccessfulOffset > currentLedgerEnd, so the reset detection is durable even if the caller crashes before calling updateState().
|
Claude finished @HardlyDifficult's task —— View job Code Review Complete ✅Todo List
Review FeedbackOverall AssessmentThis PR introduces well-structured utility functions that appear to solve legitimate runtime needs for Canton scripts. The code follows TypeScript best practices and maintains good separation of concerns. Code Quality & Best Practices ✅Strengths:
Potential Issues & Suggestions1. Input Validation Missing (
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/utils/sync-state-tracker.ts`:
- Around line 83-89: The current validation in the sync-state-tracker is only
checking types but not that offsets are non-negative integers; update the guard
in the function that reads state (used by getStartingOffset) to ensure
state.lastSuccessfulOffset and state.ledgerEndAtSync pass Number.isInteger(...)
and are >= 0 (in addition to being numbers), and keep the existing check for
state.lastUpdated being a string; if either offset fails these non-negative
integer checks, return null so getStartingOffset won't receive negative or
fractional offsets.
---
Duplicate comments:
In `@src/core/utils/sync-state-tracker.ts`:
- Around line 46-60: The constructor in SyncStateTracker currently sets
directoryWritable based only on mkdir success and builds stateFilePath by
interpolating options.network/options.provider directly; update it to explicitly
verify write permission (use fs.access or fs.accessSync with fs.constants.W_OK)
against the resolved stateDir after ensuring the directory exists and set
directoryWritable accordingly, and sanitize the network and provider inputs
before joining into stateFilePath (e.g., allow only a safe whitelist/regex like
alphanumeric, dash, underscore or use path.basename to strip traversal) so that
stateFilePath = path.join(resolvedStateDir,
`${sanitizedNetwork}-${sanitizedProvider}.json`); reference the SyncStateTracker
constructor, directoryWritable, stateFilePath, options.network and
options.provider when making these changes.
| if ( | ||
| typeof state.lastSuccessfulOffset !== 'number' || | ||
| typeof state.ledgerEndAtSync !== 'number' || | ||
| typeof state.lastUpdated !== 'string' | ||
| ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating non-negative integers.
The validation checks types but doesn't verify that lastSuccessfulOffset and ledgerEndAtSync are non-negative integers. Negative or fractional values could cause unexpected behavior in getStartingOffset.
🛡️ Optional enhancement
if (
typeof state.lastSuccessfulOffset !== 'number' ||
typeof state.ledgerEndAtSync !== 'number' ||
- typeof state.lastUpdated !== 'string'
+ typeof state.lastUpdated !== 'string' ||
+ !Number.isInteger(state.lastSuccessfulOffset) ||
+ !Number.isInteger(state.ledgerEndAtSync) ||
+ state.lastSuccessfulOffset < 0 ||
+ state.ledgerEndAtSync < 0
) {
return null;
}📝 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.
| if ( | |
| typeof state.lastSuccessfulOffset !== 'number' || | |
| typeof state.ledgerEndAtSync !== 'number' || | |
| typeof state.lastUpdated !== 'string' | |
| ) { | |
| return null; | |
| } | |
| if ( | |
| typeof state.lastSuccessfulOffset !== 'number' || | |
| typeof state.ledgerEndAtSync !== 'number' || | |
| typeof state.lastUpdated !== 'string' || | |
| !Number.isInteger(state.lastSuccessfulOffset) || | |
| !Number.isInteger(state.ledgerEndAtSync) || | |
| state.lastSuccessfulOffset < 0 || | |
| state.ledgerEndAtSync < 0 | |
| ) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/sync-state-tracker.ts` around lines 83 - 89, The current
validation in the sync-state-tracker is only checking types but not that offsets
are non-negative integers; update the guard in the function that reads state
(used by getStartingOffset) to ensure state.lastSuccessfulOffset and
state.ledgerEndAtSync pass Number.isInteger(...) and are >= 0 (in addition to
being numbers), and keep the existing check for state.lastUpdated being a
string; if either offset fails these non-negative integer checks, return null so
getStartingOffset won't receive negative or fractional offsets.
If the user-provided onCycleError handler throws, the exception would propagate out of the catch block and terminate the continuous loop. Wrapping it in its own try-catch ensures the loop remains resilient.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
Claude finished @HardlyDifficult's task —— View job Code Review Complete ✅Todo List
Review FeedbackOverall AssessmentThis PR introduces well-structured utility functions that appear to solve legitimate runtime needs for Canton scripts. The code follows TypeScript best practices and maintains good separation of concerns. Code Quality & Best Practices ✅Strengths:
Potential Issues & Suggestions1. Input Validation Missing (
|
|
Superseded by upgraded @hardlydifficult/daemon and @hardlydifficult/state-tracker packages. We no longer need to carry these runtime utilities in canton-node-sdk. |
2 participants
Summary
src/core/utilsforrunContinuousLoop,SyncStateTracker, andhashValuesrc/core/utils/index.tsso downstream repos can consume them directly from@fairmint/canton-node-sdkcantonTest plan
yarn lintyarn buildyarn test(fails in local integration suites without localnet/auth services; sameApiError: HTTP undefinedand keycloak auth failures seen pre-change)Made with Cursor
Summary by CodeRabbit
Note
Low Risk
Adds new, self-contained utilities and exports without changing existing runtime behavior; main risk is edge cases around process signal handling and local filesystem write permissions.
Overview
Adds reusable runtime utilities to
src/core/utils:runContinuousLoopfor running a periodic async cycle with SIGINT/SIGTERM graceful shutdown, optional cleanup, and per-cycle error handling.Introduces
SyncStateTrackerto persist and restore ledger sync progress via a local JSON state file (configurable directory/env override), including reset logic when saved offsets exceed current ledger end, and exports both utilities fromsrc/core/utils/index.tsfor downstream consumption.Written by Cursor Bugbot for commit c3362b8. This will update automatically on new commits. Configure here.