Conversation
Adds comprehensive unit tests for `RemoteSigningService` covering: - Policy evaluation and enforcement (allow/deny). - Human confirmation workflows (pending/approve/reject). - Audit logging of security-critical events. - Handling of expired approval requests. Tests are implemented using `vitest` syntax but shimmed to run via `bun test` to bypass environment-specific module resolution issues.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| it("fails if request expired", async () => { | ||
| // Mock Date.now to simulate expiration | ||
| vi.useFakeTimers(); | ||
| const now = Date.now(); | ||
|
|
||
| const policy: SigningPolicy = { | ||
| ...createDefaultPolicy(), | ||
| requireHumanConfirmation: true, | ||
| }; | ||
| service.updatePolicy(policy); | ||
|
|
||
| const request = makeRequest(); | ||
| await service.submitSigningRequest(request); | ||
|
|
||
| // Fast forward past expiration (timeout is 1000ms) | ||
| vi.setSystemTime(now + 2000); | ||
|
|
||
| const result = await service.approveRequest(request.requestId); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toContain("Approval expired"); | ||
| expect(service.getPendingApprovals()).toHaveLength(0); | ||
|
|
||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Potential test reliability issue due to timer handling
In the test for expired approvals (it("fails if request expired", ...)), vi.useFakeTimers() is used and then reset with vi.useRealTimers() at the end. If an exception occurs before the reset, subsequent tests may be affected by lingering fake timers, leading to unreliable results.
Recommended solution:
Wrap the test logic in a try/finally block to ensure timers are always reset:
it("fails if request expired", async () => {
vi.useFakeTimers();
try {
// ... test logic ...
} finally {
vi.useRealTimers();
}
});This guarantees cleanup even if an assertion fails or an error is thrown.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new suite of unit tests for the RemoteSigningService. The primary goal is to thoroughly validate the service's behavior in managing transaction signing requests from sandboxed agents, with a strong focus on security, user interaction, and accountability. These tests ensure the robust and correct operation of the service's core functionalities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of unit tests for the RemoteSigningService. The tests cover policy enforcement, human confirmation flows, and audit logging, which is great. My review focuses on improving the robustness and maintainability of these new tests. I've suggested a few changes: ensuring timer mocks are always cleaned up to prevent test flakiness, and strengthening assertions around audit logging to make the tests more precise and less prone to missing regressions. Overall, this is a solid addition for ensuring the service's reliability.
| let service: RemoteSigningService; | ||
| let signer: MockSigner; | ||
| let auditLog: SandboxAuditLog; | ||
| let auditLogSpy: ReturnType<typeof vi.spyOn>; |
There was a problem hiding this comment.
To ensure test isolation and prevent fake timers from leaking between tests (which can cause difficult-to-debug failures), it's a best practice to reset timers in an afterEach hook. This guarantees cleanup even if a test fails mid-execution. You can then remove the explicit vi.useRealTimers() calls from individual tests.
let auditLogSpy: ReturnType<typeof vi.spyOn>;
afterEach(() => {
vi.useRealTimers();
});| it("logs submission", async () => { | ||
| const request = makeRequest(); | ||
| await service.submitSigningRequest(request); | ||
| expect(auditLogSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: "signing_request_submitted" }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The current assertion is weak because a successful submission that is auto-approved should generate two audit log events: signing_request_submitted followed by signing_request_approved. This test only checks for the first event and ignores the second, and doesn't check the total number of calls. This could allow regressions in logging to go unnoticed.
By asserting the exact number of calls and the content of each call in order, the test becomes much more robust. This same pattern of more specific audit log assertions should be applied to other tests in this suite (e.g., for policy rejections, manual approvals, etc.).
it("logs submission and auto-approval", async () => {
const request = makeRequest();
await service.submitSigningRequest(request);
expect(auditLogSpy).toHaveBeenCalledTimes(2);
expect(auditLogSpy).toHaveBeenNthCalledWith(
1,
expect.objectContaining({ type: "signing_request_submitted" })
);
expect(auditLogSpy).toHaveBeenNthCalledWith(
2,
expect.objectContaining({ type: "signing_request_approved" })
);
});| expect(auditLogSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: "policy_decision" }), | ||
| ); |
There was a problem hiding this comment.
While this assertion is correct, it could be made more robust by also asserting the number of times the spy was called. This ensures no other unexpected log events were fired during the policy update.
expect(auditLogSpy).toHaveBeenCalledTimes(1);
expect(auditLogSpy).toHaveBeenCalledWith(
expect.objectContaining({ type: "policy_decision" })
);1 participant
This change adds a new test file
src/services/remote-signing-service.test.tsto verify the behavior of theRemoteSigningService. This service handles transaction signing requests from sandboxed agents, enforcing security policies and requiring human confirmation for high-value or sensitive operations.The tests cover:
Due to environment limitations with
vitestconfiguration loading, the tests use avishim to run successfully withbun testwhile maintaining standard Vitest syntax for future compatibility.PR created automatically by Jules for task 12041147106033532443 started by @Dexploarer