feat: timezone tool support and webhook trigger endpoint#678
feat: timezone tool support and webhook trigger endpoint#678zmanian wants to merge 6 commits intonearai:mainfrom
Conversation
…earai#657) On Windows, Instant::now() - Duration panics when system uptime is less than the subtracted duration. Replace with checked_sub() which returns None instead of panicking. Includes regression test that verifies checked_sub doesn't panic even with an absurdly large duration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…earai#666) The environment variable security option (choice 1) in the setup wizard didn't initialize secrets_crypto, causing API keys entered afterward to silently fail to save. Now initializes crypto, auto-saves the master key to ~/.ironclaw/.env, and shows a masked confirmation of the saved key. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nearai#662) - now: accepts optional timezone param, returns utc_iso + local_iso - convert: new operation for timezone-to-timezone conversion - format: new operation with strftime formatting + timezone - parse: accepts naive datetimes with timezone context - diff: uses flexible timestamp parsing - Adds chrono-tz dependency for IANA timezone support - 11 unit tests covering all operations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, and reasoning (nearai#648) - Gate RuleBasedEvaluator with #[cfg(test)], delete unused LlmEvaluator - Delete unused chunk_by_paragraphs() and its tests from chunker - Delete unused install_bundled_channel_from_artifacts() from extensions - Remove unused safety: Arc<SafetyLayer> field from Reasoning struct and propagate removal through all callers (compaction, heartbeat, dispatcher, worker, commands, builder, app, registry) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 delivers a set of targeted improvements and essential maintenance, addressing several critical issues and streamlining the codebase. It resolves a Windows-specific panic related to time calculations, enhances the setup wizard's handling of API keys for better security and persistence, and significantly upgrades the Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a good set of fixes and improvements. The panic on Windows related to Instant subtraction is correctly addressed using checked_sub, and the new regression test is a great addition. The setup wizard enhancement for API keys and the major improvements to the time tool with timezone support are both excellent. The dead code cleanup also significantly improves maintainability. I've added a few comments regarding the test suite, where using a fallback for time calculations could lead to tests silently passing under incorrect conditions. Explicitly failing the test in these setup scenarios would make them more robust, aligning with the guideline to prefer expect() for test setups dependent on environmental factors.
Note: Security Review did not run due to the size of the PR.
| created_at: std::time::Instant::now() | ||
| .checked_sub(std::time::Duration::from_secs(600)) | ||
| .unwrap_or_else(std::time::Instant::now), |
There was a problem hiding this comment.
The use of unwrap_or_else(std::time::Instant::now) could cause this test to silently check the wrong logic. If checked_sub returns None (e.g., on a system with low uptime), created_at will be set to Instant::now(). This would make the session not expired, and the test would proceed to validate the logic for a non-expired session, contrary to the test's intent.
To make the test more robust, it's better to explicitly fail with a clear message if the required test state (a past timestamp) cannot be created.
| created_at: std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(600)) | |
| .unwrap_or_else(std::time::Instant::now), | |
| created_at: std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(600)) | |
| .expect("System uptime must be > 10 minutes for this test to be valid"), |
References
- In tests, when setting up a state that depends on environmental factors (e.g., system uptime for time calculations), prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
| created_at: std::time::Instant::now() | ||
| .checked_sub(std::time::Duration::from_secs(600)) | ||
| .unwrap_or_else(std::time::Instant::now), |
There was a problem hiding this comment.
Similar to the previous comment, using unwrap_or_else here could lead to the test silently passing with the wrong logic if checked_sub underflows. The test is intended to check behavior for an expired session, but the fallback would create a non-expired one.
To ensure the test is reliable, it should panic with a descriptive message if the test setup cannot be achieved.
| created_at: std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(600)) | |
| .unwrap_or_else(std::time::Instant::now), | |
| created_at: std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(600)) | |
| .expect("System uptime must be > 10 minutes for this test to be valid"), |
References
- In tests, when setting up a state that depends on environmental factors (e.g., system uptime for time calculations), prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
| session.last_activity = std::time::Instant::now() | ||
| .checked_sub(std::time::Duration::from_secs(10)) | ||
| .unwrap_or_else(std::time::Instant::now); |
There was a problem hiding this comment.
This test simulates a stale session by setting last_activity to a time in the past. The unwrap_or_else fallback could cause last_activity to be set to Instant::now() if the subtraction underflows, which would make the session appear fresh. The test would then check the wrong condition and might pass incorrectly.
It's better to make the test fail explicitly if the required past timestamp cannot be created.
| session.last_activity = std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(10)) | |
| .unwrap_or_else(std::time::Instant::now); | |
| session.last_activity = std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(10)) | |
| .expect("System uptime must be > 10 seconds for this test to be valid"); |
References
- In tests, when setting up a state that depends on environmental factors (e.g., system uptime for time calculations), prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
| session.last_activity = std::time::Instant::now() | ||
| .checked_sub(std::time::Duration::from_secs(60)) | ||
| .unwrap_or_else(std::time::Instant::now); |
There was a problem hiding this comment.
The fallback unwrap_or_else(std::time::Instant::now) could cause this test to check the wrong logic. The test intends to push last_activity into the past to observe the change after touch(). If the subtraction underflows, last_activity will be Instant::now(), and the test's assertion might not be testing what's intended.
To make the test more robust, it should fail explicitly if the setup condition cannot be met.
| session.last_activity = std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(60)) | |
| .unwrap_or_else(std::time::Instant::now); | |
| session.last_activity = std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(60)) | |
| .expect("System uptime must be > 60 seconds for this test to be valid"); |
References
- In tests, when setting up a state that depends on environmental factors (e.g., system uptime for time calculations), prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
| let past = std::time::Instant::now() | ||
| .checked_sub(std::time::Duration::from_secs(60)) | ||
| .unwrap_or_else(std::time::Instant::now); |
There was a problem hiding this comment.
This test sets up stale sessions by creating a timestamp in the past. If checked_sub underflows, the fallback to Instant::now() would create non-stale sessions, causing the test to check the wrong logic and potentially pass when it shouldn't.
To ensure the test's correctness, it's better to explicitly fail if the past timestamp cannot be created.
| let past = std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(60)) | |
| .unwrap_or_else(std::time::Instant::now); | |
| let past = std::time::Instant::now() | |
| .checked_sub(std::time::Duration::from_secs(60)) | |
| .expect("System uptime must be > 60 seconds for this test to be valid"); |
References
- In tests, when setting up a state that depends on environmental factors (e.g., system uptime for time calculations), prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
…#663) Replace all 49 occurrences of datetime('now') with strftime('%Y-%m-%dT%H:%M:%fZ', 'now') in libSQL migrations so new rows use the same RFC 3339 format as application code (fmt_ts). Add tracing::warn to parse_timestamp naive fallback branches so old-format timestamps are logged when encountered. 5 regression tests covering RFC 3339, naive, and invalid timestamp parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gers (nearai#651) Public endpoint (no auth token) protected by per-routine webhook secrets with constant-time comparison via subtle::ConstantTimeEq. Matches path against routines with Trigger::Webhook, validates X-Webhook-Secret header, and fires the routine through the message pipeline. Returns 404/401/200 as appropriate. 2 unit tests for secret comparison and path matching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Addresses two open issues (rebased from original 6-issue PR; #657/#648/#663/#666 are now in separate PRs):
convert,formatoperations, IANA timezone parsing viachrono-tzPOST /api/webhooks/{path}endpoint for routine webhook triggers with constant-time secret validationTest plan
cargo clippy --all --all-featurespasses with zero warningscargo test --libpasses (2557 tests)Removed from this PR (now in separate PRs)
Closes #662, closes #651
Generated with Claude Code