Skip to content

fix(query): validate timeperiod structure and order - #206

Open
TimeToBuildBob wants to merge 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/query-timeperiod-validation
Open

TimeToBuildBob wants to merge 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/query-timeperiod-validation

Conversation

@TimeToBuildBob

@TimeToBuildBob TimeToBuildBob commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Problem

ServerAPI.query2 silently discards third slash-separated segments and evaluates reversed ranges. Both currently return HTTP 200 for a constant query instead of rejecting malformed report ranges.

Also fixes: ruff 0.17.3 (merged in #218) introduced new default rules that broke CI for all PRs. This commit locks the baseline to select = ["E4", "E7", "E9", "F"] matching the previous implicit defaults, so the upgrade is non-breaking. (Same fix as #224, included here so this PR is self-contained.)

Changes

  • Require exactly two ISO8601 endpoints and reject parsed start instants after end instants before cache access or evaluation for that period.
  • Preserve equal endpoints, existing timezone parsing and valid cached/uncached result shapes.
  • Add real in-memory API and Flask HTTP regressions for extra/trailing segments, reversed ranges, offset-aware comparisons, missing/invalid endpoints and mixed lists. A sentinel proves invalid periods never reach evaluation or cache eligibility.
  • Include the existing input-validation suite in make test, so CI actually executes these regressions.
  • Lock ruff rule selection to previous baseline (E4, E7, E9, F) so the 0.17.3 upgrade does not silently fail lint for existing code.

Verification

  • Ruff passes clean: All checks passed!
  • Before repair: 18 new cases failed, including HTTP 200 instead of 400 and unexpected evaluation/cache access; 28 compatibility cases passed.
  • After repair: all input-validation/query-cache tests pass.

Boundary

No request-wide rollback or prevalidation of every period: earlier valid periods may already have executed before a later malformed period is rejected. No query-language, cache-policy, timezone-normalization or Rust-server changes.

@greptile-apps

greptile-apps Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

ServerAPI.query2 now rejects extra slash-separated segments and ranges whose start is after their end.

  • Query periods must have two endpoints in chronological order.

No actionable issues were found. Earlier valid periods may still run before a later invalid period is rejected, as explicitly intended in the PR description.

Reviews (1) · Last reviewed commit: "fix(query): validate timeperiod structur..." · Reviewed by Greptile

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

The CI failure is already resolved by cf5f1d9 (Python 3.9 on a supported Ubuntu runner). All 8 checks now pass on that head, including Linux/macOS/Windows tests, typecheck, lint, format and CodeQL. No additional code change or rerun needed; awaiting maintainer review/merge.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click.

This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted.

@TimeToBuildBob

TimeToBuildBob commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

ServerAPI.query2 now requires exactly two slash-separated endpoints and raises QueryException when the parsed start instant is after the end instant. The PR adds API and HTTP validation cases, includes tests/test_input_validation.py in make test, and moves Ruff configuration to tool.ruff.lint with explicit rule selection.

Safe to merge — no P0/P1 findings

Confidence 5/5

✅ No findings. The diff looks correct to me on this pass.

Files changed (4) — the diff as I read it
  • Makefile — Adds tests/test_input_validation.py to the pytest invocation in the test target.
  • aw_server/api.py — Removes truncation of slash-separated timeperiod segments and rejects parsed start times after end times before evaluating or checking cache eligibility for that period.
  • pyproject.toml — Moves Ruff settings to tool.ruff.lint and explicitly selects E4, E7, E9, and F while retaining the existing ignores.
  • tests/test_input_validation.py — Adds an in-memory ServerAPI fixture and parameterized API and HTTP checks for malformed, reversed, equal, offset-aware, cached, uncached, and mixed timeperiod inputs.
Previous review passes
commit score findings engine when
cf5f1d9cdc4b 5/5 0 subscription 2026-10-28 16:23 UTC
00f7fa424a68 5/5 0 subscription 2026-11-06 19:02 UTC
bfc82503c722 5/5 0 subscription 2026-11-07 23:03 UTC

Reviewed a1d0ebd52f86 · panel: codex:gpt-6.1-sol · subscription engine · 19s · about this reviewer

Maintainer commands

@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.

@TimeToBuildBob
TimeToBuildBob force-pushed the fix/query-timeperiod-validation branch from cf5f1d9 to 00f7fa4 Compare November 6, 2026 15:35
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (f2aec97) and pushed 00f7fa4 with an explicit force-with-lease. The only conflict was the old Ubuntu 22.04 CI workaround; I dropped that superseded commit and retained master's Ubuntu 24.04 pins, updated actions and workflow concurrency. The query-validation commit is unchanged (git range-diff confirms it), and this PR no longer modifies the workflow.

Verified locally in an isolated worktree: 204 tests passed, mypy clean across 31 files, Black checks and git diff --check passed. GitHub now reports the PR mergeable; the new-head CI checks are still running. No merge or release performed.

@TimeToBuildBob
TimeToBuildBob force-pushed the fix/query-timeperiod-validation branch from 00f7fa4 to bfc8250 Compare November 7, 2026 19:36
@TimeToBuildBob

TimeToBuildBob commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased onto master 7af1920 and pushed bfc8250 with an explicit force-with-lease. The conflict was two independent additions at the end of tests/test_input_validation.py; both master's non-finite heartbeat tests and this PR's query-period tests are retained. git range-diff shows only the changed test insertion context, with no change to the query-validation patch.

Verified in an isolated environment: 237 tests passed, mypy clean across 31 source files, Black checks and git diff --check passed. GitHub now reports mergeable: true. New-head CI/review remain separate gates; the earlier 5/5 reviews are for older heads. No merge or release performed.

CI follow-up: the new-head lint run 42407034327 fails on the existing Ruff baseline change. The already-open repair is #224; I am not duplicating that configuration fix here. This PR still needs that repair incorporated and fresh green checks before merge.

Git-Session-Id: ff1f7458-0544-5835-8d4c-86f6cfbc620d
Git-Session-Id: c077bc51-b036-45bc-a103-433174c6780c
@TimeToBuildBob
TimeToBuildBob force-pushed the fix/query-timeperiod-validation branch from bfc8250 to a1d0ebd Compare November 9, 2026 07:48
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

On current head a1d0ebd, lint now passes, along with the other six successful checks. The only remaining failure is Windows setup: actions/setup-python received HTTP 504 on all three attempts to download Python 3.9.13, before dependencies or tests ran.

I attempted a failed-job-only rerun, but GitHub rejected it with Must have admin rights to Repository. A maintainer needs to rerun that Windows job (gh run rerun 42581617580 --repo ActivityWatch/aw-server --failed). No code or workflow change is warranted for this download failure. The latest three master Build runs succeeded; this PR still needs a successful Windows run and current-head review before merge.

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