Skip to content

fix(hogvm): return null for invalid date strings in python - #111493

Open
fallintoplace wants to merge 1 commit into
PostHog:masterfrom
fallintoplace:fix/python-invalid-date-null
Open

fallintoplace wants to merge 1 commit into
PostHog:masterfrom
fallintoplace:fix/python-invalid-date-null

Conversation

@fallintoplace

Copy link
Copy Markdown

What

  • Return null for invalid date strings in Python's toDate and toDateTime.

Why

Implementation

  • Return None when date parsing fails.

@trunk-io

trunk-io Bot commented Oct 4, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@parameterai

parameterai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Risk: No findings

This PR changes Python HogVM's toDate and toDateTime to return null for unparseable date strings instead of raising a ValueError that aborts execution, matching the TypeScript and Rust VMs (follow-up to #106830). I traced the null return through every consumer — HogQL expression evaluation, the internal date-truncation callers, and the comparison-unification path — and found no security impact: downstream type checks are dict-guarded, explicit raise ValueError("Expected a Date or DateTime") guards still fire where a non-date operand is invalid, and the comparison path uses the unchanged date_string_to_seconds. No security vulnerabilities introduced.

Sentinel reviewed 4d9a533 · Review settings

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (3)
.agents/security.md — configured
.agents/skills/writing-tests/SKILL.md — configured
.agents/skills/writing-code-comments/SKILL.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: fb7661a4-b509-43d7-b663-61208dc7fdef
📥 Commits

Reviewing files that changed from the base of the PR and between 39cc02e and 4d9a533.

📒 Files selected for processing (2)
  • common/hogvm/python/stl/date.py
  • common/hogvm/python/test/test_date.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

For unparseable strings, toDate and toDateTime now return None instead of raising ValueError. Tests check these results and confirm that toUnixTimestamp raises ValueError.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 4d9a5

Invalid date strings now produce null in Python, matching the existing TypeScript behavior. The supplied evidence establishes no material merge risk.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 4d9a5

The change aligns Python’s invalid-date results with the existing TypeScript contract. The inspected execution path retains its function restrictions and resource checks; no material security risk was identified in this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For the inspected registered-call path, the affected scope is existing Python Hog programs supplying date-conversion inputs. Invalid inputs can now produce a null result and allow execution to continue, but the change does not add a callable function or grant additional authority.

Trust Boundaries and Controls

  • observed — Direct STL dispatch still checks disallowed functions and argument bounds before invocation. Existing memory and timeout checks remain in the invocation and stack path; returning None does not bypass these inspected controls.
🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the problem, the user-visible change, and the implementation. It omits the required test rationale and does not select a release-status option, so it does not fully follow the… Add a “How did you test this code?” section with the test rationale and verification steps, and select exactly one option under “Release status.”
Full details: Description check

Explanation

The description explains the problem, the user-visible change, and the implementation. It omits the required test rationale and does not select a release-status option, so it does not fully follow the repository template.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant