feat: Add date and time awareness tool (#20)#49
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new Helsinki Datetime Tool
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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
🧹 Nitpick comments (3)
tests/tools/test_datetime_tool.py (2)
4-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return annotations and docstrings to the new tests.
Both test functions are new Python functions, but they currently omit
-> Noneand the required docstrings. As per coding guidelines, "Use type hints for all Python function parameters and return types." and "Use Google-style docstrings for all Python functions and classes."🤖 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/tools/test_datetime_tool.py` around lines 4 - 13, Add missing return annotations and docstrings to the new test functions in the datetime tool tests. Update test_returns_string and test_includes_timezone to explicitly return None and include concise Google-style docstrings that describe what each test verifies. Keep the existing assertions unchanged and ensure the function signatures in test_datetime_tool.py follow the project’s typing and documentation guidelines.Source: Coding guidelines
4-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake these assertions deterministic.
These checks would still pass if the tool returned the wrong Helsinki clock time but kept an
EET/EESTsuffix. Please freeze a known UTC instant and assert the exact rendered string for at least one winter and one summer case.🤖 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/tools/test_datetime_tool.py` around lines 4 - 13, The datetime tool tests are nondeterministic because they only check type, length, and timezone suffix, so they can pass even with the wrong clock time. Update the tests in test_returns_string and test_includes_timezone to freeze a fixed UTC instant and assert the exact rendered Helsinki datetime string for one winter case and one summer case. Use get_current_datetime.invoke as the target under test and keep the assertions fully exact instead of checking only for EET/EEST.tapio/tools/datetime_tool.py (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize the new function docstring to the repo’s Python standard.
get_current_datetimehas the return annotation already, but the new Python guideline also requires the repository’s docstring format and sections for functions. As per coding guidelines, "Use type hints for all Python function parameters and return types." and "Use Google-style docstrings for all Python functions and classes."🤖 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 `@tapio/tools/datetime_tool.py` around lines 11 - 21, The new get_current_datetime function docstring is not in the repository’s required Python style. Update its documentation to match the project’s Google-style function docstrings used elsewhere in tapio/tools, keeping the existing return annotation and adding the standard function docstring sections/wording expected by the repo. Ensure the final docstring is consistent with the Python guidelines for get_current_datetime and similar functions.Source: Coding guidelines
🤖 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 `@tapio/tools/datetime_tool.py`:
- Around line 10-23: The datetime tool contract does not match the requested
API: `get_current_datetime()` returns a human-readable timestamp, but the issue
expects `get_current_date()` with an ISO-formatted date plus weekday. Update the
tool in `datetime_tool.py` to expose the correct function name and return a
machine-friendly date string suitable for comparisons and deadline logic, while
keeping the Helsinki timezone behavior in the date computation.
- Line 7: Avoid resolving HELSINKI_TZ at import time in datetime_tool.py, since
ZoneInfo("Europe/Helsinki") can fail before ALL_TOOLS is built in minimal
runtimes. Update the datetime_tool module to either add tzdata as a runtime
dependency or load the zone lazily with a safe fallback, and make sure any
initialization in the module-level constants or datetime tool setup does not
require ZoneInfo to succeed during import.
---
Nitpick comments:
In `@tapio/tools/datetime_tool.py`:
- Around line 11-21: The new get_current_datetime function docstring is not in
the repository’s required Python style. Update its documentation to match the
project’s Google-style function docstrings used elsewhere in tapio/tools,
keeping the existing return annotation and adding the standard function
docstring sections/wording expected by the repo. Ensure the final docstring is
consistent with the Python guidelines for get_current_datetime and similar
functions.
In `@tests/tools/test_datetime_tool.py`:
- Around line 4-13: Add missing return annotations and docstrings to the new
test functions in the datetime tool tests. Update test_returns_string and
test_includes_timezone to explicitly return None and include concise
Google-style docstrings that describe what each test verifies. Keep the existing
assertions unchanged and ensure the function signatures in test_datetime_tool.py
follow the project’s typing and documentation guidelines.
- Around line 4-13: The datetime tool tests are nondeterministic because they
only check type, length, and timezone suffix, so they can pass even with the
wrong clock time. Update the tests in test_returns_string and
test_includes_timezone to freeze a fixed UTC instant and assert the exact
rendered Helsinki datetime string for one winter case and one summer case. Use
get_current_datetime.invoke as the target under test and keep the assertions
fully exact instead of checking only for EET/EEST.
🪄 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: b53c3867-20ab-4b83-922d-5ed76727a446
📒 Files selected for processing (3)
tapio/tools/__init__.pytapio/tools/datetime_tool.pytests/tools/test_datetime_tool.py
|
Forgot to remove the TODO (That is the issue that Sonar Cloud is showing) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/tools/test_datetime_tool.py (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test signature and docstring.
test_when_tzdata_missingintroduces an untypedmonkeypatchparameter and a free-form docstring. Please add a parameter annotation,-> None, and convert the docstring to the required format. As per coding guidelines,**/*.pyshould use type hints for all function parameters/returns and documented Python docstrings.🤖 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/tools/test_datetime_tool.py` around lines 19 - 25, Update test_when_tzdata_missing to follow the Python typing/docstring guidelines: add a type annotation for the monkeypatch parameter, add an explicit -> None return annotation, and rewrite the docstring into the required documented Python style used in tests. Keep the change localized to this test function in tests/tools/test_datetime_tool.py.Source: Coding guidelines
tapio/tools/datetime_tool.py (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBring these function docstrings in line with the repo standard.
_get_helsinki_tz()is undocumented, andget_current_datetime()still uses a plainReturns:section instead of the required:return:-style docs. Please convert both docstrings to the project format so the tool contract is documented consistently. As per coding guidelines,**/*.pyfunctions need Google-style docstrings with documented return values and exceptions.Also applies to: 15-25
🤖 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 `@tapio/tools/datetime_tool.py` around lines 7 - 11, The datetime tool docstrings are not following the repo’s Python docstring standard. Update `_get_helsinki_tz()` to add a Google-style docstring that documents its return value and any fallback behavior, and update `get_current_datetime()` to replace the plain `Returns:` section with the required `:return:`-style documentation. Keep the formatting consistent with the project’s `**/*.py` docstring convention and make sure both functions are documented clearly.Source: Coding guidelines
🤖 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 `@tests/tools/test_datetime_tool.py`:
- Around line 27-29: The current test only verifies that
get_current_datetime.invoke returns a non-empty string, so it does not cover the
UTC fallback contract. Update the test around get_current_datetime to assert
that the rendered datetime includes the UTC timezone when _get_helsinki_tz()
degrades, using the existing get_current_datetime symbol so the fallback
behavior is explicitly validated.
---
Nitpick comments:
In `@tapio/tools/datetime_tool.py`:
- Around line 7-11: The datetime tool docstrings are not following the repo’s
Python docstring standard. Update `_get_helsinki_tz()` to add a Google-style
docstring that documents its return value and any fallback behavior, and update
`get_current_datetime()` to replace the plain `Returns:` section with the
required `:return:`-style documentation. Keep the formatting consistent with the
project’s `**/*.py` docstring convention and make sure both functions are
documented clearly.
In `@tests/tools/test_datetime_tool.py`:
- Around line 19-25: Update test_when_tzdata_missing to follow the Python
typing/docstring guidelines: add a type annotation for the monkeypatch
parameter, add an explicit -> None return annotation, and rewrite the docstring
into the required documented Python style used in tests. Keep the change
localized to this test function in tests/tools/test_datetime_tool.py.
🪄 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: 2396dbd8-00a9-442c-98bf-1d6e8e3952b8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomltapio/tools/datetime_tool.pytests/tools/test_datetime_tool.py
|
@brylie |
set -o pipefail: the default shell is bash -e {0} with no pipefail, so
the | tee pipelines added for output capture were silently swallowing
prek/mypy/pyrefly/pytest's real exit code -- confirmed against PR #49's
actual run logs, which show "shell: /usr/bin/bash -e {0}". Without this
fix the lint job in particular would never fail again, undoing the
honest-status fix from the prior commit.
Randomize the GITHUB_OUTPUT heredoc delimiter: a fixed delimiter could
appear verbatim in captured tool output and prematurely close the
heredoc, letting the remaining (attacker-controlled, since it's PR file
content) lines be parsed as additional GITHUB_OUTPUT entries.
* Make CI failures honest and actionable
The lint job's "Lint (prek)" check showed green even when prek itself
failed, because continue-on-error masked it from the job's own status
-- only the report job's gate caught it, with no indication of why.
Remove continue-on-error from the (single-step) lint job so it fails
visibly when prek does.
Also surface the actual failure output instead of just pass/fail:
mypy and pyrefly findings now get inline ::error annotations on the
Files changed tab, and prek/mypy/pyrefly/pytest output is captured
and included in the job summary and sticky PR comment, alongside the
local command to reproduce and fix each one.
* Improve Dockerfile comments for clarity on Yarn source removal and zstd installation
* Fix two real CI bugs in the new output-capture steps
set -o pipefail: the default shell is bash -e {0} with no pipefail, so
the | tee pipelines added for output capture were silently swallowing
prek/mypy/pyrefly/pytest's real exit code -- confirmed against PR #49's
actual run logs, which show "shell: /usr/bin/bash -e {0}". Without this
fix the lint job in particular would never fail again, undoing the
honest-status fix from the prior commit.
Randomize the GITHUB_OUTPUT heredoc delimiter: a fixed delimiter could
appear verbatim in captured tool output and prematurely close the
heredoc, letting the remaining (attacker-controlled, since it's PR file
content) lines be parsed as additional GITHUB_OUTPUT entries.
The "report" job's PR comment step was failing silently (403 Resource not accessible by integration) for every fork-originated PR, since GITHUB_TOKEN is read-only for pull_request runs triggered by forks. Confirmed against PR #49 (a fork PR): the comment body was built correctly but the POST itself was rejected, and continue-on-error hid the failure entirely -- no comment ever appeared. Split the work in two: "report" now builds the comment body and uploads it as an artifact instead of posting it directly, dropping its pull-requests: write permission since it no longer needs it. A new workflow, comment-on-pr.yml, triggers on workflow_run once CI completes and posts/updates the comment from there -- workflow_run always runs with the base repo's own permissions using this file as committed on the default branch, regardless of which fork triggered the original run, so it works for both fork and same-repo PRs without granting fork code any elevated token access.
|
👋 Hi @AlyanPremani05, thanks for the pull request! Before this can be merged, please address: Lint (prek). Details and local reproduction commands are below, with links to the relevant CONTRIBUTING.md section for each. CI Failures❌ prekRun Test Coverage Report✅ 87% coverage (required: ≥80%) Coverage meets the project threshold. 🎉 |
|
Hei @AlyanPremani05. I've made some changes to CI in the main branch and updated your working branch with those change. Now, the CI failures are more helpful, including line-level feedback and a summary comment. Please pull the most recent changes into your local branch and then fix the lint issues, so we can merge this branch. Thanks! |
|
|
@brylie Pulled your latest CI changes and fixed the lint issues .This should be good to go I guess. let me know if anything else comes up. |
2 participants
Closes #20
Summary
Adds a
get_current_datetime()tool that returns the current date and timein Helsinki (Finnish) local time, as a
langchain_core@tool-decoratedfunction.
tapio/tools/datetime_tool.py—get_current_datetime()tool, returns ahuman-readable Helsinki local time string (e.g. "Tuesday, 27 June 2026,
14:32 EEST")
tapio/tools/__init__.py— exportsALL_TOOLSlist containing the new tooltests/tools/test_datetime_tool.py— tests that the tool returns a stringand includes a timezone abbreviation (EET/EEST)
Summary by CodeRabbit
tzdataruntime dependency.