More resiliance and observability to EMMA#57
Merged
GelatoGenesis merged 4 commits intomainfrom Mar 16, 2026
Merged
Conversation
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
📝 WalkthroughWalkthroughIntroduces observability and retry for token pool downloads: DB schema adds observability columns, new stage enum, repository methods for progress/failures, service retry and staleness logic, async downloader requeueing, API retry endpoint, and related tests and utility adjustments. Changes
Sequence DiagramsequenceDiagram
actor User
participant Controller as TokenPoolDownloadController
participant Service as TokenPoolDownloadService
participant Repo as TokenPoolDownloadRepository
participant OpRepo as AllowlistOperationRepository
participant Downloader as TokenPoolAsyncDownloader
participant DB as Database
User->>Controller: POST /allowlists/:allowlistId/token-pool-downloads/token-pool/:tokenPoolId/retry
Controller->>Service: retry(allowlistId, tokenPoolId)
Service->>Repo: getByTokenPoolId(allowlistId, tokenPoolId)
Repo->>DB: SELECT token_pool_download row
DB-->>Repo: row
Repo-->>Service: entity
alt entity exists and retryable
Service->>Repo: recordFailureHistory(tokenPoolId, reason)
Repo->>DB: UPDATE failure_count, last_failure_at, last_failure_reason
DB-->>Repo: OK
Service->>OpRepo: getCreateTokenPoolOperation(allowlistId, tokenPoolId)
OpRepo->>DB: SELECT operation
DB-->>OpRepo: operation data
OpRepo-->>Service: params
Service->>Downloader: start(config, state) or start async with params
Downloader->>Repo: updateProgress / changeStatus...
Repo->>DB: UPDATE progress, stage, timestamps
DB-->>Repo: OK
Downloader-->>Service: { entity, state }
Service->>Service: entityToApiModel(entity) (compute stale, retryable, rawStatus)
Service-->>Controller: TokenPoolDownloadResponseApiModel
else missing or not retryable
Service-->>Controller: throw NotFoundException or BadRequestException
end
Controller-->>User: 200 OK or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
migrations/20260316120000-token-pool-download-observability.js (1)
22-23: Avoid logging full SQL migration content.Logging full SQL payloads adds noise and can expose internal schema details in logs; this is safe to remove.
Proposed change
- console.log('received data: ' + data);Also applies to: 37-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/20260316120000-token-pool-download-observability.js` around lines 22 - 23, Remove the full SQL content being printed in migrations/20260316120000-token-pool-download-observability.js by deleting the console.log('received data: ' + data) statement (and the similar logging at the later occurrence around lines 37-38); if visibility is needed, replace with a minimal, non-sensitive message (e.g., indicate migration step name or "received data (truncated)" and/or a byte/row count) rather than printing the entire SQL payload to logs.src/api/token-pool-download/token-pool-download.service.ts (2)
259-273: Double JSON.parse on the sameoperation.params.The params are parsed once inside
find()(line 261) to match thetokenPoolId, and then parsed again at line 272 for the return value. Consider caching the parsed result.♻️ Proposed refactor to avoid double parsing
- const operation = operations.find((candidate) => { + let parsedParams: { + id?: string; + tokenIds?: string; + contract: string; + blockNo: number; + consolidateBlockNo?: number | null; + } | null = null; + const operation = operations.find((candidate) => { try { - return JSON.parse(candidate.params)?.id === tokenPoolId; + const params = JSON.parse(candidate.params); + if (params?.id === tokenPoolId) { + parsedParams = params; + return true; + } + return false; } catch (e) { return false; } }); if (!operation) { throw new NotFoundException( `Create token pool operation for token pool ${tokenPoolId} does not exist`, ); } return { - params: JSON.parse(operation.params), + params: parsedParams!, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/token-pool-download/token-pool-download.service.ts` around lines 259 - 273, The code double-parses operation.params when locating the matching operation and again when returning params; update the logic around operations.find (the callback that currently JSON.parse(candidate.params) to compare to tokenPoolId) to parse once and cache the parsed value (e.g., store parsed object on the matched operation variable or a local map) so that after you check for !operation and throw the NotFoundException you return the cached parsed params instead of calling JSON.parse(operation.params) a second time; ensure the comparison still guards parse errors (catching exceptions) as before.
199-203: Consider logging the JSON parse failure for observability.The graceful fallback to
{ raw: progress }is appropriate, but silently swallowing the exception makes debugging harder if progress data is consistently malformed.📝 Proposed improvement to add logging
try { return JSON.parse(progress); } catch (e) { + // Log at debug level - malformed progress JSON shouldn't alarm but is useful for debugging + this.logger?.debug?.(`Failed to parse progress JSON: ${e.message}`); return { raw: progress }; }Note: You'd need to inject a
Loggerinstance if not already available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/token-pool-download/token-pool-download.service.ts` around lines 199 - 203, The try/catch that calls JSON.parse(progress) should log the parse failure for observability instead of silently swallowing it: inject or obtain a Logger instance in the TokenPoolDownloadService (or the service class containing the JSON.parse(progress) call), and inside the catch block call logger.warn/error with a clear message including the raw progress value and the caught exception (e.g., logger.error('Failed to parse progress JSON', { progress, err })). Keep the existing fallback return { raw: progress } after logging.migrations/sqls/20260316120000-token-pool-download-observability-up.sql (1)
1-14: LGTM! Schema extension aligns with observability requirements.The column types are appropriate:
BIGINTfor millisecond timestampsINT NOT NULL DEFAULT 0for counters to avoid null handlingLONGTEXTfor potentially large error messages and progress JSONVARCHAR(64)for bounded stage enum valuesOptional consideration: If you anticipate frequent queries filtering by
stage(e.g., finding all jobs in a particular stage for monitoring dashboards), consider adding an index:CREATE INDEX idx_token_pool_download_stage ON token_pool_download(stage);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/sqls/20260316120000-token-pool-download-observability-up.sql` around lines 1 - 14, The migration adds a new nullable VARCHAR(64) column stage to token_pool_download but doesn't add an index; if you expect frequent filtering by stage, add a separate migration step to create an index on token_pool_download(stage) (e.g., run a CREATE INDEX idx_token_pool_download_stage ON token_pool_download(stage) in this migration or an immediately subsequent one) to improve query performance and monitoring dashboards.
🤖 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/api/token-pool-download/token-pool-download.service.ts`:
- Around line 147-152: After calling
tokenPoolDownloadRepository.getByTokenPoolId the returned variable refreshed may
be null; update the code that calls getByTokenPoolId (the logic surrounding
tokenPoolAsyncDownloader.start and the subsequent return
this.entityToApiModel(refreshed)) to explicitly handle a null result—either
throw a descriptive error (e.g., NotFound) or return an appropriate null/404
response—before calling entityToApiModel so entityToApiModel is never invoked
with null; reference tokenPoolDownloadRepository.getByTokenPoolId and
entityToApiModel when implementing the guard.
In `@src/app.utils.ts`:
- Around line 5-11: The bigInt2Number helper currently returns Number(value)
which can yield NaN or Infinity and leak into downstream logic; update
bigInt2Number to coerce only after the null/undefined guard, then compute const
n = Number(value) and return undefined unless Number.isFinite(n) is true (i.e.,
return n only for finite numeric results); reference the function name
bigInt2Number when making this change so callers continue to receive undefined
for non-finite coercions.
---
Nitpick comments:
In `@migrations/20260316120000-token-pool-download-observability.js`:
- Around line 22-23: Remove the full SQL content being printed in
migrations/20260316120000-token-pool-download-observability.js by deleting the
console.log('received data: ' + data) statement (and the similar logging at the
later occurrence around lines 37-38); if visibility is needed, replace with a
minimal, non-sensitive message (e.g., indicate migration step name or "received
data (truncated)" and/or a byte/row count) rather than printing the entire SQL
payload to logs.
In `@migrations/sqls/20260316120000-token-pool-download-observability-up.sql`:
- Around line 1-14: The migration adds a new nullable VARCHAR(64) column stage
to token_pool_download but doesn't add an index; if you expect frequent
filtering by stage, add a separate migration step to create an index on
token_pool_download(stage) (e.g., run a CREATE INDEX
idx_token_pool_download_stage ON token_pool_download(stage) in this migration or
an immediately subsequent one) to improve query performance and monitoring
dashboards.
In `@src/api/token-pool-download/token-pool-download.service.ts`:
- Around line 259-273: The code double-parses operation.params when locating the
matching operation and again when returning params; update the logic around
operations.find (the callback that currently JSON.parse(candidate.params) to
compare to tokenPoolId) to parse once and cache the parsed value (e.g., store
parsed object on the matched operation variable or a local map) so that after
you check for !operation and throw the NotFoundException you return the cached
parsed params instead of calling JSON.parse(operation.params) a second time;
ensure the comparison still guards parse errors (catching exceptions) as before.
- Around line 199-203: The try/catch that calls JSON.parse(progress) should log
the parse failure for observability instead of silently swallowing it: inject or
obtain a Logger instance in the TokenPoolDownloadService (or the service class
containing the JSON.parse(progress) call), and inside the catch block call
logger.warn/error with a clear message including the raw progress value and the
caught exception (e.g., logger.error('Failed to parse progress JSON', {
progress, err })). Keep the existing fallback return { raw: progress } after
logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fed74a62-0531-4bc5-9672-18350cdf60ba
📒 Files selected for processing (13)
migrations/20260316120000-token-pool-download-observability.jsmigrations/sqls/20260316120000-token-pool-download-observability-down.sqlmigrations/sqls/20260316120000-token-pool-download-observability-up.sqlsrc/api/token-pool-download/model/token-pool-download-response-api.model.tssrc/api/token-pool-download/token-pool-download.controller.tssrc/api/token-pool-download/token-pool-download.service.spec.tssrc/api/token-pool-download/token-pool-download.service.tssrc/app.utils.tssrc/repository/token-pool-download/token-pool-download-stage.tssrc/repository/token-pool-download/token-pool-download.entity.tssrc/repository/token-pool-download/token-pool-download.repository.tssrc/token-pool/token-pool-async-downloader.tssrc/token-pool/token-pool-downloader.service.ts
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/api/token-pool-download/token-pool-download.service.spec.ts (2)
184-241: Add assertion thatrecordFailureHistoryis not called for FAILED status.When the entity status is
FAILED, the service skips recording failure history (line 121-126 in service). The test should verify this behavior to catch regressions.💚 Suggested enhancement
).rejects.toThrow( 'Token pool download with ID pool-1 no longer exists after retry', ); + expect(tokenPoolDownloadRepository.recordFailureHistory).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/token-pool-download/token-pool-download.service.spec.ts` around lines 184 - 241, Add an assertion in the test that tokenPoolDownloadRepository.recordFailureHistory is not called when the entity.status is TokenPoolDownloadStatus.FAILED: after calling service.retry(...) expect(tokenPoolDownloadRepository.recordFailureHistory).not.toHaveBeenCalled(). This references the mocked tokenPoolDownloadRepository.recordFailureHistory and the TokenPoolDownloadService.retry behavior for FAILED status to ensure recording failure history is skipped.
160-161: Strengthen assertion to verifyrecordFailureHistoryarguments.The test only verifies that
recordFailureHistorywas called, but doesn't validate the arguments. This could miss bugs where the wrongtokenPoolIdorfailureReasonis passed.💚 Suggested enhancement
- expect(tokenPoolDownloadRepository.recordFailureHistory).toHaveBeenCalled(); + expect(tokenPoolDownloadRepository.recordFailureHistory).toHaveBeenCalledWith({ + tokenPoolId: 'pool-1', + failureReason: expect.stringContaining('has not reported progress for more than 20 minutes'), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/token-pool-download/token-pool-download.service.spec.ts` around lines 160 - 161, The test currently only checks that tokenPoolDownloadRepository.recordFailureHistory was called; update the assertion to verify the actual arguments passed by replacing or adding an expectation that tokenPoolDownloadRepository.recordFailureHistory was called with the correct tokenPoolId and failure details (e.g., use the test's tokenPool.id and the expected failureReason or pattern) — target the tokenPoolDownloadRepository.recordFailureHistory call and assert e.g. .toHaveBeenCalledWith(tokenPool.id, expect.objectContaining({ failureReason: expectedFailureReason })) so the test fails if the wrong id or reason is sent; you can still keep the existing expectation verifying tokenPoolAsyncDownloader.start was called with the expected payload.src/api/token-pool-download/token-pool-download.service.ts (1)
264-278: Avoid double JSON.parse on the same string.The
paramsstring is parsed once in thefindcallback (line 266) to locate the operation, then parsed again at line 277. This is redundant.♻️ Suggested refactor to parse once
- const operation = operations.find((candidate) => { + let parsedParams: Record<string, unknown> | undefined; + const operation = operations.find((candidate) => { try { - return JSON.parse(candidate.params)?.id === tokenPoolId; + const parsed = JSON.parse(candidate.params); + if (parsed?.id === tokenPoolId) { + parsedParams = parsed; + return true; + } + return false; } catch (e) { return false; } }); - if (!operation) { + if (!operation || !parsedParams) { throw new NotFoundException( `Create token pool operation for token pool ${tokenPoolId} does not exist`, ); } return { - params: JSON.parse(operation.params), + params: parsedParams as { + tokenIds?: string; + contract: string; + blockNo: number; + consolidateBlockNo?: number | null; + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/token-pool-download/token-pool-download.service.ts` around lines 264 - 278, The code redundantly calls JSON.parse on operation.params twice; change the operations.find usage to parse once and preserve the parsed result for reuse—e.g., when using operations.find((candidate) => { const parsed = safeParse(candidate.params); if (parsed?.id === tokenPoolId) { candidate.__parsedParams = parsed; return true } return false }), then after the NotFoundException check return { params: operation.__parsedParams } (or alternatively have find return an object like { operation, parsedParams }) so you only parse operation.params a single time; update references to use the stored parsed object instead of calling JSON.parse again (referencing operations, operation, tokenPoolId).src/app.utils.spec.ts (1)
17-40: Add resilience tests for non-serializable error payloads.Given this utility is used in catch paths, add cases ensuring
stringifyErrordoes not fail on problematic inputs (e.g., BigInt in metadata, circular cause graphs, Symbol values).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.utils.spec.ts` around lines 17 - 40, Add resilience tests and harden stringifyError to handle non-serializable metadata and cyclic/odd causes: write unit tests feeding stringifyError with metadata containing BigInt, Symbol, and a cause graph with a cycle and ensure it returns a string (not throw); then modify stringifyError to (1) safely serialize metadata by converting BigInt to string and Symbols to their description or toString, (2) guard against circular references when traversing causes using a WeakSet/Set of seen objects to break cycles, and (3) wrap metadata and cause processing in try/catch and fall back to a safe placeholder like "<unserializable>" for values that still throw — target the stringifyError function and any helper used to format metadata/cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 28: The package.json dependency "@6529-collections/allowlist-lib" is set
to "0.0.131" but package-lock.json still references "0.0.124"; update the
lockfile to match by either running npm install (or npm ci after installing) to
regenerate package-lock.json so it reflects 0.0.131, or if 0.0.131 isn't
available, revert the version in package.json back to "0.0.124" so both files
are synchronized.
In `@src/app.utils.ts`:
- Around line 71-73: The Prettier violation is caused by the multiline ternary
that handles error.cause; locate the code that calls stringifyError and
references error.cause (the ternary producing `? \` Cause:
${stringifyError(error.cause)}\` : ''`) and reformat it to satisfy
Prettier—either collapse the ternary onto a single line or replace it with a
short if/else or concatenation so the expression is formatted consistently
(preserve the same output string but ensure the expression around error.cause
and stringifyError is on one line).
- Around line 57-86: The current stringifyError implementation uses raw
JSON.stringify on unknown values (metadata entries, message fallback, and nested
cause) which can throw or return non-strings; update stringifyError to always
return a string by wrapping all JSON.stringify calls in try/catch and falling
back to String(value) when JSON fails or returns undefined, and add simple
circular-reference protection for recursive stringifyError calls (e.g., accept
an optional WeakSet seen param, add objects to it and detect cycles to return
"[Circular]"). Ensure all places that build code, metadata, message and cause
use the safe stringify helper so stringifyError never throws and always returns
a plain string.
---
Nitpick comments:
In `@src/api/token-pool-download/token-pool-download.service.spec.ts`:
- Around line 184-241: Add an assertion in the test that
tokenPoolDownloadRepository.recordFailureHistory is not called when the
entity.status is TokenPoolDownloadStatus.FAILED: after calling
service.retry(...)
expect(tokenPoolDownloadRepository.recordFailureHistory).not.toHaveBeenCalled().
This references the mocked tokenPoolDownloadRepository.recordFailureHistory and
the TokenPoolDownloadService.retry behavior for FAILED status to ensure
recording failure history is skipped.
- Around line 160-161: The test currently only checks that
tokenPoolDownloadRepository.recordFailureHistory was called; update the
assertion to verify the actual arguments passed by replacing or adding an
expectation that tokenPoolDownloadRepository.recordFailureHistory was called
with the correct tokenPoolId and failure details (e.g., use the test's
tokenPool.id and the expected failureReason or pattern) — target the
tokenPoolDownloadRepository.recordFailureHistory call and assert e.g.
.toHaveBeenCalledWith(tokenPool.id, expect.objectContaining({ failureReason:
expectedFailureReason })) so the test fails if the wrong id or reason is sent;
you can still keep the existing expectation verifying
tokenPoolAsyncDownloader.start was called with the expected payload.
In `@src/api/token-pool-download/token-pool-download.service.ts`:
- Around line 264-278: The code redundantly calls JSON.parse on operation.params
twice; change the operations.find usage to parse once and preserve the parsed
result for reuse—e.g., when using operations.find((candidate) => { const parsed
= safeParse(candidate.params); if (parsed?.id === tokenPoolId) {
candidate.__parsedParams = parsed; return true } return false }), then after the
NotFoundException check return { params: operation.__parsedParams } (or
alternatively have find return an object like { operation, parsedParams }) so
you only parse operation.params a single time; update references to use the
stored parsed object instead of calling JSON.parse again (referencing
operations, operation, tokenPoolId).
In `@src/app.utils.spec.ts`:
- Around line 17-40: Add resilience tests and harden stringifyError to handle
non-serializable metadata and cyclic/odd causes: write unit tests feeding
stringifyError with metadata containing BigInt, Symbol, and a cause graph with a
cycle and ensure it returns a string (not throw); then modify stringifyError to
(1) safely serialize metadata by converting BigInt to string and Symbols to
their description or toString, (2) guard against circular references when
traversing causes using a WeakSet/Set of seen objects to break cycles, and (3)
wrap metadata and cause processing in try/catch and fall back to a safe
placeholder like "<unserializable>" for values that still throw — target the
stringifyError function and any helper used to format metadata/cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc2088ab-5238-4eaf-88a9-07ef8d3a1f1d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.jsonsrc/api/token-pool-download/token-pool-download.service.spec.tssrc/api/token-pool-download/token-pool-download.service.tssrc/app.utils.spec.tssrc/app.utils.ts
| }, | ||
| "dependencies": { | ||
| "@6529-collections/allowlist-lib": "0.0.130", | ||
| "@6529-collections/allowlist-lib": "0.0.131", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the library version exists on npm and check for any changelog or breaking changes.
# Check if version 0.0.131 exists on npm
echo "=== Checking if version 0.0.131 exists on npm ==="
npm view `@6529-collections/allowlist-lib`@0.0.131 version 2>/dev/null || echo "Version not found on npm registry"
# Show recent versions for context
echo ""
echo "=== Recent versions of the package ==="
npm view `@6529-collections/allowlist-lib` versions --json 2>/dev/null | jq '.[-5:]' || echo "Could not fetch versions"
# List all imports from this library in the codebase to identify dependencies
echo ""
echo "=== Imports from `@6529-collections/allowlist-lib` in codebase ==="
rg -n "from '@6529-collections/allowlist-lib" --type ts | head -30Repository: 6529-Collections/allowlist-api
Length of output: 4737
🏁 Script executed:
# Check if there's npm registry configuration
echo "=== Checking for .npmrc ==="
cat .npmrc 2>/dev/null || echo "No .npmrc file found"
echo ""
echo "=== Current package.json dependencies (lines around 28) ==="
sed -n '20,35p' package.json
echo ""
echo "=== Checking for lock files ==="
ls -la package-lock.json yarn.lock 2>/dev/null | head -5
echo ""
echo "=== Git log for package.json (recent changes) ==="
git log --oneline -n 10 -- package.json 2>/dev/null || echo "Git history not available"
echo ""
echo "=== Check if this is in a monorepo ==="
find . -maxdepth 3 -name "package.json" -type f | grep -v node_modules | head -10Repository: 6529-Collections/allowlist-api
Length of output: 1456
🏁 Script executed:
#!/bin/bash
# Check package-lock.json for the library version
echo "=== Checking package-lock.json for `@6529-collections/allowlist-lib` ==="
grep -A 10 '"@6529-collections/allowlist-lib"' package-lock.json | head -20
echo ""
echo "=== Checking if the package resolves in lock file ==="
jq '.packages["node_modules/@6529-collections/allowlist-lib"].version' package-lock.json 2>/dev/null || echo "Could not extract version from package-lock.json"
echo ""
echo "=== Attempting to fetch from GitHub Package Registry ==="
# Try to fetch package info from GitHub registry (requires auth token, but let's check if it's discoverable)
curl -s -H "Accept: application/vnd.npm.install-v1+json" \
"https://npm.pkg.github.com/@6529-collections/allowlist-lib" 2>/dev/null | jq '.versions | keys | .[-5:]' 2>/dev/null || echo "Could not access GitHub Package Registry (authentication may be required)"
echo ""
echo "=== Check if node_modules has the package installed ==="
ls -la node_modules/@6529-collections/allowlist-lib/ 2>/dev/null | head -5 || echo "Package not in node_modules (expected - sandbox environment)"Repository: 6529-Collections/allowlist-api
Length of output: 848
Synchronize package.json and package-lock.json for @6529-collections/allowlist-lib.
The package.json declares version 0.0.131, but package-lock.json is still locked to 0.0.124. This mismatch will cause inconsistent dependency resolution across environments. Regenerate the lock file by running npm install (or npm ci after install) to ensure package-lock.json reflects the version specified in package.json, or revert package.json to 0.0.124 if 0.0.131 is not yet available on GitHub Package Registry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 28, The package.json dependency
"@6529-collections/allowlist-lib" is set to "0.0.131" but package-lock.json
still references "0.0.124"; update the lockfile to match by either running npm
install (or npm ci after installing) to regenerate package-lock.json so it
reflects 0.0.131, or if 0.0.131 isn't available, revert the version in
package.json back to "0.0.124" so both files are synchronized.
Comment on lines
+57
to
86
| if (error instanceof Error || (error && typeof error === 'object')) { | ||
| const code = | ||
| typeof error.code === 'string' && error.code.length | ||
| ? `[${error.code}] ` | ||
| : ''; | ||
| const metadata = | ||
| error.metadata && typeof error.metadata === 'object' | ||
| ? Object.entries(error.metadata) | ||
| .filter(([, value]) => value !== undefined) | ||
| .map(([key, value]) => `${key}=${JSON.stringify(value)}`) | ||
| .join(', ') | ||
| : ''; | ||
| const details = metadata ? ` (${metadata})` : ''; | ||
| const cause = | ||
| error.cause !== undefined | ||
| ? ` Cause: ${stringifyError(error.cause)}` | ||
| : ''; | ||
| const message = | ||
| typeof error.message === 'string' && error.message.length | ||
| ? error.message | ||
| : JSON.stringify(error); | ||
|
|
||
| return `${code}${message}${details}${cause}`; | ||
| } | ||
|
|
||
| if (error === undefined) { | ||
| return 'undefined'; | ||
| } else { | ||
| return JSON.stringify(error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does JSON.stringify throw on BigInt and circular references, and can it return undefined for Symbol/function/undefined values?
💡 Result:
- BigInt: Yes.
JSON.stringify()throws aTypeErrorif it encounters aBigInt(unless you convert it yourself viatoJSON/replacer). (tc39.es) - Circular (cyclic) references: Yes.
JSON.stringify()throws aTypeErrorwhen the value is or contains a cyclic structure. (tc39.es) - Return value
undefined(top-level): Yes. If the top-level input is (or is transformed into) an unrepresentable value likeundefinedor a function (and per the algorithm, non-JSON primitives fall through to “returnundefined”), thenJSON.stringify(...)returnsundefined. (tc39.es) - Object vs array handling for
undefined/function: In objects, such properties are omitted; in arrays, they becomenull. (tc39.es)
Citations:
- 1: https://tc39.es/ecma262/multipage/structured-data.html
- 2: https://tc39.es/ecma262/multipage/structured-data.html
- 3: https://tc39.es/ecma262/multipage/structured-data.html
- 4: https://tc39.es/ecma262/multipage/structured-data.html
🏁 Script executed:
# First, let's find the stringifyError function and confirm line numbers
cd /tmp && find . -name "app.utils.ts" -type f 2>/dev/null | head -5Repository: 6529-Collections/allowlist-api
Length of output: 56
🏁 Script executed:
# List all TypeScript files to locate the correct path
git ls-files | grep -E "(app\.utils|token-pool-downloader|runner\.service)" | head -20Repository: 6529-Collections/allowlist-api
Length of output: 225
🏁 Script executed:
# Check the stringifyError function in app.utils.ts
wc -l src/app.utils.tsRepository: 6529-Collections/allowlist-api
Length of output: 93
🏁 Script executed:
# Read the stringifyError function
cat -n src/app.utils.ts | sed -n '50,90p'Repository: 6529-Collections/allowlist-api
Length of output: 1398
🏁 Script executed:
# Check the usage in runner.service.ts around lines 78-85
sed -n '70,95p' src/runner/runner.service.tsRepository: 6529-Collections/allowlist-api
Length of output: 810
🏁 Script executed:
# Check the usage in token-pool-downloader.service.ts around lines 206-220
sed -n '200,230p' src/token-pool/token-pool-downloader.service.tsRepository: 6529-Collections/allowlist-api
Length of output: 773
Harden stringifyError so error serialization never throws or returns non-string.
Lines 66, 77, and 85 use raw JSON.stringify(...) on unknown values. This can throw (e.g., BigInt, circular refs) or return undefined (e.g., Symbol), breaking error persistence at src/token-pool/token-pool-downloader.service.ts:206 and src/runner/runner.service.ts:82, where callers expect a string for errorReason.
Wrap JSON.stringify calls in try-catch, return String(value) as fallback, and add circular reference detection for recursive calls.
🧰 Tools
🪛 ESLint
[error] 71-73: Replace ⏎········?··Cause:·${stringifyError(error.cause)}⏎······· with ·?··Cause:·${stringifyError(error.cause)}``
(prettier/prettier)
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 71-71: Unexpected negated condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.utils.ts` around lines 57 - 86, The current stringifyError
implementation uses raw JSON.stringify on unknown values (metadata entries,
message fallback, and nested cause) which can throw or return non-strings;
update stringifyError to always return a string by wrapping all JSON.stringify
calls in try/catch and falling back to String(value) when JSON fails or returns
undefined, and add simple circular-reference protection for recursive
stringifyError calls (e.g., accept an optional WeakSet seen param, add objects
to it and detect cycles to return "[Circular]"). Ensure all places that build
code, metadata, message and cause use the safe stringify helper so
stringifyError never throws and always returns a plain string.
Comment on lines
+71
to
+73
| error.cause !== undefined | ||
| ? ` Cause: ${stringifyError(error.cause)}` | ||
| : ''; |
There was a problem hiding this comment.
Fix the Prettier violation in the cause ternary.
Static analysis reports a formatting error at Line 71-Line 73 (prettier/prettier). This can fail lint/CI for the PR.
Minimal formatting fix
- const cause =
- error.cause !== undefined
- ? ` Cause: ${stringifyError(error.cause)}`
- : '';
+ const cause = error.cause !== undefined ? ` Cause: ${stringifyError(error.cause)}` : '';📝 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.
Suggested change
| error.cause !== undefined | |
| ? ` Cause: ${stringifyError(error.cause)}` | |
| : ''; | |
| const cause = error.cause !== undefined ? ` Cause: ${stringifyError(error.cause)}` : ''; |
🧰 Tools
🪛 ESLint
[error] 71-73: Replace ⏎········?··Cause:·${stringifyError(error.cause)}⏎······· with ·?··Cause:·${stringifyError(error.cause)}``
(prettier/prettier)
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 71-71: Unexpected negated condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.utils.ts` around lines 71 - 73, The Prettier violation is caused by
the multiline ternary that handles error.cause; locate the code that calls
stringifyError and references error.cause (the ternary producing `? \` Cause:
${stringifyError(error.cause)}\` : ''`) and reformat it to satisfy
Prettier—either collapse the ternary onto a single line or replace it with a
short if/else or concatenation so the expression is formatted consistently
(preserve the same output string but ensure the expression around error.cause
and stringifyError is on one line).
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 by CodeRabbit
New Features
Database
Tests