Skip to content

fix(api): reject non-string JSON event timestamps - #209

Open
TimeToBuildBob wants to merge 7 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/numeric-event-timestamps-ed7a
Open

TimeToBuildBob wants to merge 7 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/numeric-event-timestamps-ed7a

Conversation

@TimeToBuildBob

@TimeToBuildBob TimeToBuildBob commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Numeric JSON event timestamps reach aw-core's datetime parser and return HTTP 500 (AttributeError). Event POST does not enforce the heartbeat schema because it also accepts batches.

Change

  • Validate JSON timestamp shape before Event construction: strings and legacy missing/null values retain existing behavior; other types return structured JSON 400.
  • No epoch coercion or broad AttributeError catch.
  • Add real Flask/MemoryStorage regressions for integer/float/bool/list/object timestamps in single and batch POSTs, unchanged storage after rejection, valid follow-up requests, heartbeat schema controls, and missing/null compatibility.
  • Include test_input_validation.py in make test so hosted CI executes the regressions.

Verification

  • RED on fetched master 994ea988: integer timestamp returns 500 with AttributeError in aw-core.
  • 34 focused validation/heartbeat tests passed.
  • 246 offline package tests passed (test_client.py excluded because it requires a separately running server).
  • mypy: 31 source files clean; changed-path Black and Ruff checks pass; git diff --check clean.

Maintainer merge and replay on master remain required. No live server or personal ActivityWatch data used. Independent of the nonfinite heartbeat pulse repair in #207.

Hosted CI setup repair

Initial Linux build/typecheck failed before tests: setup-python cannot install Python 3.9 on Ubuntu 26.04, now selected by ubuntu-latest (run 40729263872). Commit 6202372 pins those two Linux jobs to Ubuntu 22.04, retaining Python 3.9 and all workflow steps. Replacement checks are pending; local passing tests are not a claim of green hosted CI.

@greptile-apps

greptile-apps Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no blocking issues were found.

Summary

The PR rejects non-string JSON event timestamps before constructing an Event.

  • Event uploads reject timestamps that are not strings or null.
  • Import code reports non-object bucket values as type errors.

No actionable issues were found. Tests were not run during this review.

Reviews (2) · Last reviewed commit: "fix(api): use TypeError for type validat..." · Reviewed by Greptile

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

All eight hosted checks now pass on 6202372, including Linux, macOS and Windows tests. No review findings or unresolved comments need changes. Awaiting maintainer review/merge; master regression replay remains pending.

@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

Safe to merge — no P0/P1 findings

Confidence 5/5

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

Previous review passes
commit score findings engine when
62023728a44c 5/5 0 subscription 2026-10-28 19:22 UTC
c56c7cc0cb4f 5/5 0 subscription 2026-11-06 20:43 UTC
57c6ea1794d3 5/5 0 subscription 2026-11-07 23:03 UTC
bc14d45458a1 5/5 0 subscription 2026-11-11 02:41 UTC
8f34142adb00 4/5 1 subscription 2026-11-11 07:01 UTC

Reviewed 0df8ccd030d8 · openrouter/deepseek/deepseek-v4-flash-0731 · panel (in-band) engine · 22s · 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/numeric-event-timestamps-ed7a branch from 6202372 to c56c7cc Compare November 6, 2026 15:34
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased and force-pushed with an explicit lease as c56c7cc. The only conflict was the CI runner pin: I dropped the superseded Ubuntu 22.04 workaround and retained master's Ubuntu 24.04 runners, checkout v7 and workflow concurrency settings. git range-diff confirms the timestamp fix/test commit is otherwise unchanged.

Verification on the rebased head: 33 input-validation tests passed, 246 offline tests passed (excluding test_client.py, which needs a running server), and mypy passed for all 31 source files. GitHub now reports the PR mergeable; replacement hosted checks are still running. No live ActivityWatch server or personal data touched.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

The replacement CI on c56c7cc has seven passing checks. The remaining macOS job never acquired a hosted runner: GitHub reports “The job was not acquired by Runner of type hosted even after multiple attempts,” with macOS arm64 capacity notices. This is an infrastructure cancellation, not a failing test; the three latest master Build runs also passed.

I attempted to rerun just that job, but GitHub denied it with HTTP 403 (repository admin rights required). No code or runner configuration changed. A maintainer can retry the cancelled macOS job in run 42244383331; macOS validation on this head remains pending.

@TimeToBuildBob
TimeToBuildBob force-pushed the fix/numeric-event-timestamps-ed7a branch from c56c7cc to 57c6ea1 Compare November 7, 2026 19:34
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto current master and force-pushed with an explicit lease as 57c6ea1. The conflict was two independent additions to tests/test_input_validation.py: I retained both master's nonfinite heartbeat-pulsetime tests and this PR's timestamp tests. git range-diff confirms the PR's implementation and test bodies are unchanged; only their insertion context moved.

Verification: 41 input-validation tests passed, 279 offline tests passed (excluding test_client.py, which requires a separately running server), mypy passed for all 31 source files, and changed-path Black plus git diff --check passed. Tests used an isolated worktree environment with dependency versions from poetry.lock; an initial unlocked environment hit unrelated dependency incompatibilities, so it is not the basis for these results. No dependency files, CI configuration, live server, or personal ActivityWatch data changed.

GitHub now reports the PR mergeable. Replacement hosted checks are in progress; previous review scores belong to older heads, not this rebase. Maintainer merge remains pending.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

On 57c6ea1, seven hosted checks passed, including Linux, macOS and Windows tests. The sole failure is Ruff 0.17.3's expanded default rule set, also failing on master (PR run, master run).

The existing repair is #224. I reproduced 148 lint errors on this exact head, then verified that using #224's configuration clears all of them without changing any tracked files. No duplicate repair, rerun or timestamp-code change made. Next step: land the shared lint repair, then refresh this branch and verify replacement CI. #224 itself still has a cancelled macOS check requiring maintainer attention; this is not an all-green merge claim.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Fixed ruff TRY004 lint failures: both isinstance-guard raises now use TypeError instead of ValueError, matching the ruff recommendation for type-check exceptions. Rebased on current master.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Pushed fff5b2c to fix the Black failure introduced by the preceding TypeError change: the import guard now uses Black's single-line layout. No behavior or lint configuration changed.

Verified locally: Black 26.10.1 passes across all 36 Python files; 56 import/input-validation tests and 279 offline tests pass (the latter excludes test_client.py, which needs a running server). git diff --check is clean.

The remaining Ruff failure is the shared baseline issue, not resolved by the two TRY004 edits: master's latest three Lint runs also fail, and #224 remains open. Running Ruff 0.17.3 on this head with #224's configuration passes without changing tracked files. I have not duplicated that repair here. Replacement hosted checks are pending; next step is to land #224, refresh this branch, and verify CI before merge.

@TimeToBuildBob
TimeToBuildBob force-pushed the fix/numeric-event-timestamps-ed7a branch from fff5b2c to bc14d45 Compare November 11, 2026 00:40
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased on TimeToBuildBob/fix/restore-ruff-ci-baseline (see #224) to fix lint CI failure — ruff 0.17.3 introduced new default rules that break the existing codebase. The rebase includes the ruff config fix (preserve E4/E7/E9/F baseline) so lint CI should pass again.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

The latest rebase has not restored hosted CI: current head bc14d454 has no checks, and GitHub reports merge conflicts against current master d3c97e41. The latest completed Lint failure is on the previous head fff5b2c (run 42931286845); it is the expanded Ruff-rule baseline failure, also present in master's latest three Lint runs. Master's latest three Build runs passed.

I did not overwrite the branch: another session holds its active write lease. I sent that owner the conflict/check evidence and the rebase handoff. Next step is to reconcile this branch with current master and #224, then verify fresh hosted checks. This PR is not merge-ready; the earlier 5/5 reviews do not cover this head.

TimeToBuildBob and others added 6 commits November 11, 2026 06:55
Git-Session-Id: ff1f7458-0544-5835-8d4c-86f6cfbc620d
Git-Session-Id: f69034e5-7be4-4e81-a24b-e7b45ae284f2
Without target-version, Ruff infers no floor, so pyupgrade-style fixes
(e.g. UP017 -> datetime.UTC, 3.11+) could break the declared ^3.8 support
if those rule families are adopted later.

Refs ActivityWatch#225

Git-Session-Id: 9b655f9c-758f-5b15-b198-639d6fcfb45c
Git-Session-Id: f9b41099-5af9-441d-b867-64a38316194e
Git-Session-Id: ed9b5097-f7d1-4302-86c4-91e331f1b21f
Git-Session-Id: f087e85e-3fa7-51b1-b962-a4f7fd1060d3
@TimeToBuildBob
TimeToBuildBob force-pushed the fix/numeric-event-timestamps-ed7a branch from bc14d45 to 8f34142 Compare November 11, 2026 06:55
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (conflict came from the content_disposition, pulse-window and aw-webui commits landing upstream; those three were dropped as already applied). Remaining diff is the timestamp validation, TypeError change and Ruff CI pins. Local tests/test_input_validation.py + tests/test_server.py: 86 passed. Lint/format pass; test jobs re-running on 8f34142. Still waiting on a maintainer click.

The _parse_event check in rest.py only guards the POST /events endpoint.
The import path in ServerAPI.import_bucket built Event(**e) directly,
so an exported event with a numeric timestamp bypassed that check and
reached aw-core's datetime parser, raising AttributeError → HTTP 500.

Add the same string-or-null guard in import_bucket (raises ValueError so
ImportAllResource.post's existing (KeyError, TypeError, ValueError) handler
returns a structured 400) and add regression tests in test_import.py.

Git-Session-Id: c511536f-90c3-54a0-86d2-edc7dddf01dc
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