fix: resolve all pre-existing backend test failures#261
Conversation
- registry.js: remove duplicate annotateTtlWarning function declaration - contract.js: remove duplicate SERVICE_MAX_TTL/SERVICE_TTL_WARNING_LEDGERS exports - registry.test.js: remove duplicate const declarations (mockGetCurrentLedgerSequence, SERVICE_MAX_TTL, SERVICE_TTL_WARNING_LEDGERS) - services.js: add missing recordPaymentOnChain() call in creditPayment() after all guards pass All 12 test files, 166 tests now pass.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMoves backend config validation into ChangesDeferred Config Validation and Startup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)backend/src/routes/registry.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path backend/src/routes/registry.test.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path backend/src/config.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.14][ERROR]: unable to find a config; path
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 (1)
backend/src/index.js (1)
19-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winWhitelist explicit fields in
--print-config
stellarandcontractare public-only today, but serializing whole objects makes future secret additions easy to expose. Whitelist the exact fields instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/index.js` around lines 19 - 30, The --print-config output is serializing whole `stellar` and `contract` objects, which can accidentally expose future secrets; update the config printing logic in `backend/src/index.js` to whitelist only the explicitly allowed fields. Keep the existing `x402` field mapping as-is, and limit `stellar` and `contract` to their current public properties by name so the printed config remains safe even if those objects gain new secret values later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/config.js`:
- Around line 163-167: The fatal startup summary in the config validation path
is built only from the missing environment variables, so format/validation
failures like PAYMENT_ADDRESS are reported with an empty “missing required
environment variables” list. Update the aggregated failure message in the config
validation logic to summarize `errors` as well as `missing`, using the existing
`errors.length > 0` block and `log.fatal` call so the fatal output reflects the
actual invalid inputs.
In `@backend/src/index.js`:
- Around line 12-40: The --print-config branch in index.js bypasses
configuration validation and can exit successfully even with invalid settings.
Move validateConfig(logger) so it runs before the early return for
process.argv.includes("--print-config"), or otherwise ensure the same validation
path is executed before printing the config. Keep the existing print-and-exit
behavior in the CLI path, but only after validateConfig has passed.
---
Nitpick comments:
In `@backend/src/index.js`:
- Around line 19-30: The --print-config output is serializing whole `stellar`
and `contract` objects, which can accidentally expose future secrets; update the
config printing logic in `backend/src/index.js` to whitelist only the explicitly
allowed fields. Keep the existing `x402` field mapping as-is, and limit
`stellar` and `contract` to their current public properties by name so the
printed config remains safe even if those objects gain new secret values later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91546ae2-b843-4803-9552-18bd3d1ed666
📒 Files selected for processing (5)
backend/src/config.jsbackend/src/config.test.jsbackend/src/index.jsbackend/src/routes/registry.jsbackend/src/routes/registry.test.js
💤 Files with no reviewable changes (1)
- backend/src/routes/registry.js
| if (errors.length > 0) { | ||
| log.fatal( | ||
| { missingVars: missing, errors }, | ||
| `Server startup failed: missing required environment variables: ${missing.join(', ')}`, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Build the fatal summary from errors, not just missing.
When PAYMENT_ADDRESS is the only invalid input, this logs missing required environment variables: with an empty list even though the real failure is a format error. That makes the new aggregated startup output misleading.
Suggested fix
if (errors.length > 0) {
+ const summary =
+ missing.length > 0
+ ? `Server startup failed: ${errors.join('; ')}`
+ : `Server startup failed: ${errors[0]}`;
log.fatal(
{ missingVars: missing, errors },
- `Server startup failed: missing required environment variables: ${missing.join(', ')}`,
+ summary,
);
process.exit(1);
}📝 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 (errors.length > 0) { | |
| log.fatal( | |
| { missingVars: missing, errors }, | |
| `Server startup failed: missing required environment variables: ${missing.join(', ')}`, | |
| ); | |
| if (errors.length > 0) { | |
| const summary = | |
| missing.length > 0 | |
| ? `Server startup failed: ${errors.join('; ')}` | |
| : `Server startup failed: ${errors[0]}`; | |
| log.fatal( | |
| { missingVars: missing, errors }, | |
| summary, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/config.js` around lines 163 - 167, The fatal startup summary in
the config validation path is built only from the missing environment variables,
so format/validation failures like PAYMENT_ADDRESS are reported with an empty
“missing required environment variables” list. Update the aggregated failure
message in the config validation logic to summarize `errors` as well as
`missing`, using the existing `errors.length > 0` block and `log.fatal` call so
the fatal output reflects the actual invalid inputs.
| if (process.argv.includes("--print-config")) { | ||
| console.log( | ||
| JSON.stringify( | ||
| { | ||
| nodeEnv: config.nodeEnv, | ||
| port: config.port, | ||
| logLevel: config.logLevel, | ||
| stellar: config.stellar, | ||
| contract: config.contract, | ||
| x402: { | ||
| facilitatorUrl: config.x402.facilitatorUrl, | ||
| searchPrice: config.x402.searchPrice, | ||
| weatherPrice: config.x402.weatherPrice, | ||
| payTo: config.x402.payTo, | ||
| }, | ||
| corsOrigin: config.corsOrigin, | ||
| jsonBodyLimit: config.jsonBodyLimit, | ||
| trustProxy: config.trustProxy, | ||
| rateLimit: config.rateLimit, | ||
| demoRun: config.demoRun, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| validateConfig(logger); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run validateConfig(logger) before the --print-config early exit.
Right now the new CLI path prints and exits 0 even when the same environment would fail normal startup, so the dry-run path never actually validates config.
Suggested fix
+validateConfig(logger);
+
if (process.argv.includes("--print-config")) {
console.log(
JSON.stringify(
{
@@
);
process.exit(0);
}
-
-validateConfig(logger);📝 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 (process.argv.includes("--print-config")) { | |
| console.log( | |
| JSON.stringify( | |
| { | |
| nodeEnv: config.nodeEnv, | |
| port: config.port, | |
| logLevel: config.logLevel, | |
| stellar: config.stellar, | |
| contract: config.contract, | |
| x402: { | |
| facilitatorUrl: config.x402.facilitatorUrl, | |
| searchPrice: config.x402.searchPrice, | |
| weatherPrice: config.x402.weatherPrice, | |
| payTo: config.x402.payTo, | |
| }, | |
| corsOrigin: config.corsOrigin, | |
| jsonBodyLimit: config.jsonBodyLimit, | |
| trustProxy: config.trustProxy, | |
| rateLimit: config.rateLimit, | |
| demoRun: config.demoRun, | |
| }, | |
| null, | |
| 2, | |
| ), | |
| ); | |
| process.exit(0); | |
| } | |
| validateConfig(logger); | |
| validateConfig(logger); | |
| if (process.argv.includes("--print-config")) { | |
| console.log( | |
| JSON.stringify( | |
| { | |
| nodeEnv: config.nodeEnv, | |
| port: config.port, | |
| logLevel: config.logLevel, | |
| stellar: config.stellar, | |
| contract: config.contract, | |
| x402: { | |
| facilitatorUrl: config.x402.facilitatorUrl, | |
| searchPrice: config.x402.searchPrice, | |
| weatherPrice: config.x402.weatherPrice, | |
| payTo: config.x402.payTo, | |
| }, | |
| corsOrigin: config.corsOrigin, | |
| jsonBodyLimit: config.jsonBodyLimit, | |
| trustProxy: config.trustProxy, | |
| rateLimit: config.rateLimit, | |
| demoRun: config.demoRun, | |
| }, | |
| null, | |
| 2, | |
| ), | |
| ); | |
| process.exit(0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/index.js` around lines 12 - 40, The --print-config branch in
index.js bypasses configuration validation and can exit successfully even with
invalid settings. Move validateConfig(logger) so it runs before the early return
for process.argv.includes("--print-config"), or otherwise ensure the same
validation path is executed before printing the config. Keep the existing
print-and-exit behavior in the CLI path, but only after validateConfig has
passed.
2 participants
Uh oh!
There was an error while loading. Please reload this page.