test(core): add edge case tests for simulation failures - #263
Conversation
|
@neyij Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds shared simulation-failure assertions in the RPC client, propagates thrown RPC errors through extension flows, and updates tests for rejected and code-specific simulation failures. ChangesSimulation Error Handling Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 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 `@src/core/extension.ts`:
- Around line 102-112: The catch blocks around simulateExtension, the later
polling path, and the related extension flow are treating every thrown exception
as a simulation warning, which incorrectly swallows non-simulation failures.
Update the handlers in extension.ts to catch only the dedicated simulation error
type for client.simulateExtension and related polling logic, and rethrow or
propagate unexpected exceptions such as invalid secret keys, account lookup
failures, and RPC outages. Keep the warning/result conversion only for expected
simulation failures, and use the existing logger and contractId-based messaging
to preserve accurate context.
In `@tests/core/extension.test.ts`:
- Around line 200-222: The simulation-failure test for extendEntries should also
verify that no partial database write happens when submit fails before the
transaction starts. Update the test around extendEntries and
getEntriesForContract to assert extension_history remains empty after the mocked
Simulation failed path, matching the no-partial-write guarantee already covered
in the send-failure case.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c63beb41-46e8-45ca-a843-0dd0e1f1dd06
📒 Files selected for processing (4)
src/core/extension.tssrc/rpc/client.tstests/core/extension.test.tstests/rpc/client.test.ts
📜 Review details
🔇 Additional comments (3)
src/rpc/client.ts (2)
15-29: LGTM!Also applies to: 353-355, 401-401, 467-467
302-304: 🎯 Functional CorrectnessVerify the new throw contract for
getMonitoredKeys.src/core/watch.ts:139is the only current call site and awaits this directly, so simulation failures will bubble out of the watch path unless that caller handles them.tests/rpc/client.test.ts (1)
359-374: LGTM!
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34377146 | Triggered | Generic High Entropy Secret | 3240c4e | tests/core/vault.test.ts | View secret |
| 34377147 | Triggered | Generic High Entropy Secret | 3240c4e | tests/core/vault.test.ts | View secret |
| 34377148 | Triggered | Generic High Entropy Secret | 3240c4e | tests/core/vault.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Hi @neyij, thanks for the PR! I noticed that the CI is failing. I investigated this and found a few things missing:
Please apply the following patch to fix these issues. You can save this to a file (e.g. fix.patch) and run git apply fix.patch, then commit and push. \\diff
/**
@@ -217,8 +213,6 @@ describe("Core Extension Logic", () => {
diff --git a/tests/e2e/daemon-execution.test.ts b/tests/e2e/daemon-execution.test.ts
\\ Thanks! |
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 (2)
tests/core/extension.test.ts (1)
256-262: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep a warning-log assertion on this rejection path.
Issue
#137requires the mock failure cases to verify readable warning logs. This update only checks the returned error, so a regression inlogger.warn(...)would now pass unnoticed. Mock the child logger rather than dropping the assertion entirely.🤖 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 `@tests/core/extension.test.ts` around lines 256 - 262, The simulation failure test in simulateExtension is missing the warning-log assertion that Issue `#137` requires. Keep the rejection-path check on the returned error, but also mock the child logger used by simulateExtension and verify logger.warn is called with a readable warning message when mockSimulateExtension rejects. Use the existing simulateExtension test case and associated logger mock setup to restore the assertion instead of removing it.src/core/extension.ts (1)
324-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the actual key source that failed.
When a channel slot exists but the fallback
resolveSecretKey(policy.keypair_source)also returnsnull, Line 337 still reports"channel pool". That sends operators to the wrong secret source during triage. Include both attempted sources, or track which lookup actually failed.🤖 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 `@src/core/extension.ts` around lines 324 - 338, The error path in the key-resolution flow is reporting the wrong source when `resolveSecretKey` fails after a `slot.keypairSource` attempt and then a fallback `resolveSecretKey(policy.keypair_source)` attempt. Update the logic around `resolveSecretKey`, `slot.keypairSource`, and `policy.keypair_source` so the pushed `result.errors` message identifies the actual failed source (or lists both attempted sources) instead of always using `"channel pool"` whenever `pool` exists.
🤖 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 `@src/rpc/client.ts`:
- Around line 684-690: The success payload returned from the transaction flow is
using the new resource field names, but downstream extension handling still
expects the legacy keys. Update the return shape in pollTransaction() and/or
normalize the result in submitExtension() so extendEntries() can read cpuInsns
and memBytes without losing resource usage data, while keeping the existing
cpuInstructions and memoryBytes fields available for newer callers.
In `@tests/rpc/client.test.ts`:
- Around line 421-427: The simulateExtension test is still asserting a resolved
failure object, but StellarRpcClient.simulateExtension now throws on simulation
failures via assertSimulationSuccess. Update the test named "simulateExtension
propagates error when RPC simulation fails" to expect a rejected promise instead
of checking result.success/result.error, using the simulateExtension call on
simFailClient and verifying it rejects with "Simulation failed". Keep the
assertion aligned with the current behavior of
StellarRpcClient.simulateExtension.
---
Outside diff comments:
In `@src/core/extension.ts`:
- Around line 324-338: The error path in the key-resolution flow is reporting
the wrong source when `resolveSecretKey` fails after a `slot.keypairSource`
attempt and then a fallback `resolveSecretKey(policy.keypair_source)` attempt.
Update the logic around `resolveSecretKey`, `slot.keypairSource`, and
`policy.keypair_source` so the pushed `result.errors` message identifies the
actual failed source (or lists both attempted sources) instead of always using
`"channel pool"` whenever `pool` exists.
In `@tests/core/extension.test.ts`:
- Around line 256-262: The simulation failure test in simulateExtension is
missing the warning-log assertion that Issue `#137` requires. Keep the
rejection-path check on the returned error, but also mock the child logger used
by simulateExtension and verify logger.warn is called with a readable warning
message when mockSimulateExtension rejects. Use the existing simulateExtension
test case and associated logger mock setup to restore the assertion instead of
removing it.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f98f26e-441c-45b2-a289-871d66516042
📒 Files selected for processing (4)
src/core/extension.tssrc/rpc/client.tstests/core/extension.test.tstests/rpc/client.test.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CI Pipeline / build-and-test (22.x): test(core): add edge case tests for simulation failures
Conclusion: failure
##[group]Run npm run test -- --coverage
�[36;1mnpm run test -- --coverage�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
> sorokeep@0.1.2 test
> vitest run --coverage
�[1m�[46m RUN �[49m�[22m �[36mv3.2.6 �[39m�[90m/home/runner/work/sorokeep/sorokeep�[39m
�[2mCoverage enabled with �[22m�[33mv8�[39m
�[32m✓�[39m tests/alerts/resource.test.ts �[2m(�[22m�[2m21 tests�[22m�[2m)�[22m�[32m 51�[2mms�[22m�[39m
�[90mstdout�[2m | tests/core/monitor.test.ts�[2m > �[22m�[2mrunMonitorCycle�[2m > �[22m�[2mError handling and fault isolation�[2m > �[22m�[2mcontinues checking other contracts when one RPC call fails
�[22m�[39mError: RPC timeout
at /home/runner/work/sorokeep/sorokeep/tests/core/monitor.test.ts:597:40
at file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:155:11
at file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:752:26
at file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1897:20
at new Promise (<anonymous>)
at runWithTimeout (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1863:10)
at runTest (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1574:12)
at processTicksAndRejections (node:internal/process/task_queues:103:5)
at runSuite (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1729:8)
at runSuite (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1729:8)
�[90mstdout�[2m | tests/core/monitor.test.ts�[2m > �[22m�[2mrunMonitorCycle�[2m > �[22m�[2mError handling and fault isolation�[2m > �[22m�[2mcollects error message referencing the failing contract ID
�[22m�[39mError: Connection refused
at /home/runner/work/sorokeep/sorokeep/tests/core/monitor.test.ts:611:48
at file:///home/runner/work/sorokeep/sorokeep/n...
GitHub Actions: CI Pipeline / 0_build-and-test (22.x).txt: test(core): add edge case tests for simulation failures
Conclusion: failure
##[group]Run npm run test -- --coverage
�[36;1mnpm run test -- --coverage�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
> sorokeep@0.1.2 test
> vitest run --coverage
�[1m�[46m RUN �[49m�[22m �[36mv3.2.6 �[39m�[90m/home/runner/work/sorokeep/sorokeep�[39m
�[2mCoverage enabled with �[22m�[33mv8�[39m
�[32m✓�[39m tests/alerts/resource.test.ts �[2m(�[22m�[2m21 tests�[22m�[2m)�[22m�[32m 51�[2mms�[22m�[39m
�[90mstdout�[2m | tests/core/monitor.test.ts�[2m > �[22m�[2mrunMonitorCycle�[2m > �[22m�[2mError handling and fault isolation�[2m > �[22m�[2mcontinues checking other contracts when one RPC call fails
�[22m�[39mError: RPC timeout
at /home/runner/work/sorokeep/sorokeep/tests/core/monitor.test.ts:597:40
at file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:155:11
at file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:752:26
at file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1897:20
at new Promise (<anonymous>)
at runWithTimeout (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1863:10)
at runTest (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1574:12)
at processTicksAndRejections (node:internal/process/task_queues:103:5)
at runSuite (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1729:8)
at runSuite (file:///home/runner/work/sorokeep/sorokeep/node_modules/@vitest/runner/dist/chunk-hooks.js:1729:8)
�[90mstdout�[2m | tests/core/monitor.test.ts�[2m > �[22m�[2mrunMonitorCycle�[2m > �[22m�[2mError handling and fault isolation�[2m > �[22m�[2mcollects error message referencing the failing contract ID
�[22m�[39mError: Connection refused
at /home/runner/work/sorokeep/sorokeep/tests/core/monitor.test.ts:611:48
at file:///home/runner/work/sorokeep/sorokeep/n...
🧰 Additional context used
🪛 GitHub Actions: CI Pipeline / 0_build-and-test (22.x).txt
tests/rpc/client.test.ts
[error] 363-363: Assertion failed: expected error to include Expired sequence number but received assertSimulationSuccess is not defined.
[error] 369-369: Assertion failed: expected error to include Insufficient wallet balance but received assertSimulationSuccess is not defined.
[error] 375-375: Assertion failed: expected error to include Invalid footprint key but received assertSimulationSuccess is not defined.
src/rpc/client.ts
[error] 433-433: ReferenceError: assertSimulationSuccess is not defined (thrown in StellarRpcClient.simulateExtension).
[error] 481-481: ReferenceError: assertSimulationSuccess is not defined (thrown in StellarRpcClient.submitExtension).
[error] 547-547: ReferenceError: assertSimulationSuccess is not defined (thrown in StellarRpcClient.submitRestore).
🪛 GitHub Actions: CI Pipeline / build-and-test (22.x)
tests/rpc/client.test.ts
[error] 355-380: StellarRpcClient submitExtension tests failed due to ReferenceError: assertSimulationSuccess is not defined (expected success / specific simulation error messages).
src/rpc/client.ts
[error] 433-433: ReferenceError: assertSimulationSuccess is not defined (in simulateExtension).
[error] 481-481: ReferenceError: assertSimulationSuccess is not defined (in submitExtension).
[error] 547-547: ReferenceError: assertSimulationSuccess is not defined (in submitRestore).
🔇 Additional comments (1)
src/core/extension.ts (1)
109-118: Already noted: these broadcatchblocks still collapse unrelated client failures intoSimulation warningresults.Also applies to: 157-167, 412-423
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 (2)
tests/core/extension.test.ts (1)
256-262: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep a warning-log assertion on this rejection path.
Issue
#137requires the mock failure cases to verify readable warning logs. This update only checks the returned error, so a regression inlogger.warn(...)would now pass unnoticed. Mock the child logger rather than dropping the assertion entirely.🤖 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 `@tests/core/extension.test.ts` around lines 256 - 262, The simulation failure test in simulateExtension is missing the warning-log assertion that Issue `#137` requires. Keep the rejection-path check on the returned error, but also mock the child logger used by simulateExtension and verify logger.warn is called with a readable warning message when mockSimulateExtension rejects. Use the existing simulateExtension test case and associated logger mock setup to restore the assertion instead of removing it.src/core/extension.ts (1)
324-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the actual key source that failed.
When a channel slot exists but the fallback
resolveSecretKey(policy.keypair_source)also returnsnull, Line 337 still reports"channel pool". That sends operators to the wrong secret source during triage. Include both attempted sources, or track which lookup actually failed.🤖 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 `@src/core/extension.ts` around lines 324 - 338, The error path in the key-resolution flow is reporting the wrong source when `resolveSecretKey` fails after a `slot.keypairSource` attempt and then a fallback `resolveSecretKey(policy.keypair_source)` attempt. Update the logic around `resolveSecretKey`, `slot.keypairSource`, and `policy.keypair_source` so the pushed `result.errors` message identifies the actual failed source (or lists both attempted sources) instead of always using `"channel pool"` whenever `pool` exists.
🤖 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 `@src/rpc/client.ts`:
- Around line 684-690: The success payload returned from the transaction flow is
using the new resource field names, but downstream extension handling still
expects the legacy keys. Update the return shape in pollTransaction() and/or
normalize the result in submitExtension() so extendEntries() can read cpuInsns
and memBytes without losing resource usage data, while keeping the existing
cpuInstructions and memoryBytes fields available for newer callers.
In `@tests/rpc/client.test.ts`:
- Around line 421-427: The simulateExtension test is still asserting a resolved
failure object, but StellarRpcClient.simulateExtension now throws on simulation
failures via assertSimulationSuccess. Update the test named "simulateExtension
propagates error when RPC simulation fails" to expect a rejected promise instead
of checking result.success/result.error, using the simulateExtension call on
simFailClient and verifying it rejects with "Simulation failed". Keep the
assertion aligned with the current behavior of
StellarRpcClient.simulateExtension.
---
Outside diff comments:
In `@src/core/extension.ts`:
- Around line 324-338: The error path in the key-resolution flow is reporting
the wrong source when `resolveSecretKey` fails after a `slot.keypairSource`
attempt and then a fallback `resolveSecretKey(policy.keypair_source)` attempt.
Update the logic around `resolveSecretKey`, `slot.keypairSource`, and
`policy.keypair_source` so the pushed `result.errors` message identifies the
actual failed source (or lists both attempted sources) instead of always using
`"channel pool"` whenever `pool` exists.
In `@tests/core/extension.test.ts`:
- Around line 256-262: The simulation failure test in simulateExtension is
missing the warning-log assertion that Issue `#137` requires. Keep the
rejection-path check on the returned error, but also mock the child logger used
by simulateExtension and verify logger.warn is called with a readable warning
message when mockSimulateExtension rejects. Use the existing simulateExtension
test case and associated logger mock setup to restore the assertion instead of
removing it.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f98f26e-441c-45b2-a289-871d66516042
📒 Files selected for processing (4)
src/core/extension.tssrc/rpc/client.tstests/core/extension.test.tstests/rpc/client.test.ts
📜 Review details
🔇 Additional comments (1)
src/core/extension.ts (1)
109-118: Already noted: these broadcatchblocks still collapse unrelated client failures intoSimulation warningresults.Also applies to: 157-167, 412-423
🛑 Comments failed to post (2)
src/rpc/client.ts (1)
684-690: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize the success resource field names before returning here.
pollTransaction()now returnscpuInstructions/memoryBytes, butextendEntries()still readstxResult.cpuInsns/txResult.memByteswhen computing anomalies and writing extension history. Successful extension submissions will therefore drop resource usage data entirely. Either emit the legacy keys here too, or map them insubmitExtension()before returning.🤖 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 `@src/rpc/client.ts` around lines 684 - 690, The success payload returned from the transaction flow is using the new resource field names, but downstream extension handling still expects the legacy keys. Update the return shape in pollTransaction() and/or normalize the result in submitExtension() so extendEntries() can read cpuInsns and memBytes without losing resource usage data, while keeping the existing cpuInstructions and memoryBytes fields available for newer callers.tests/rpc/client.test.ts (1)
421-427: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update this test to assert rejection, not a failure result.
StellarRpcClient.simulateExtension()now throws viaassertSimulationSuccess(...)on simulation failures, so this path no longer resolves to{ success: false, error }. Useawait expect(...).rejects.toThrow("Simulation failed")here 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 `@tests/rpc/client.test.ts` around lines 421 - 427, The simulateExtension test is still asserting a resolved failure object, but StellarRpcClient.simulateExtension now throws on simulation failures via assertSimulationSuccess. Update the test named "simulateExtension propagates error when RPC simulation fails" to expect a rejected promise instead of checking result.success/result.error, using the simulateExtension call on simFailClient and verifying it rejects with "Simulation failed". Keep the assertion aligned with the current behavior of StellarRpcClient.simulateExtension.
3 participants
Closes #137