Skip to content

fix(api): return bad request for rejected event writes - #214

Open
TimeToBuildBob wants to merge 1 commit into
ActivityWatch:masterfrom
TimeToBuildBob:fix-event-write-validation-response
Open

TimeToBuildBob wants to merge 1 commit into
ActivityWatch:masterfrom
TimeToBuildBob:fix-event-write-validation-response

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Summary

  • Map event creation ValueError to the existing structured InvalidEvent 400 response, at the HTTP boundary.
  • Cover single-object, one-element-list and multi-event paths with rejected storage writes, plus an unexpected-failure 500 control.
  • Include the regression file in the explicit make test/CI test list.

Closes #211. Companion to ActivityWatch/aw-core#196 (ownership enforcement).

Verification

  • New HTTP regressions on unmodified server: 3 failed (500 instead of 400), 1 unexpected-failure control passed. After repair: all 4 pass.
  • make test: 148 passed. Broader unit suite excluding listener-dependent tests/test_client.py: 231 passed.
  • make typecheck: 32 source files clean; Black check passed.
  • Filing-time disposable real-Peewee HTTP replay with repaired core and this server: PASS ownership (single and one-element batch preserve source/destination and return 4xx).
  • A raw pytest tests additionally attempts external-listener client tests: 3 failed/12 errors from absent listener and released client lacking set_setting. Those are not claimed green; no production listener was started or modified.

No dependency bump in this PR: the error mapping is backward compatible with released core. Actual ownership enforcement needs the core companion to ship. No release/deployment claim.

@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.

Summary

The PR returns HTTP 400 with InvalidEvent when create_events rejects a write with ValueError.

  • Rejected event writes now return an InvalidEvent 400 response.

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

Reviews (1) · Last reviewed commit: "fix(api): return bad request for rejecte..." · Reviewed by Greptile

@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

The PR wraps current_app.api.create_events() in EventsResource.post() to translate ValueError into a structured InvalidEvent HTTP 400 response. It adds storage-rejection tests for single-object, one-element-list, and multi-event requests, plus a RuntimeError HTTP 500 control, and includes the new test file in make test.

Safe to merge — no P0/P1 findings

Confidence 5/5

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

Files changed (3) — the diff as I read it
  • Makefile — Adds tests/test_event_write_validation.py to the explicit pytest invocation in the test target.
  • aw_server/rest.py — Catches ValueError from create_events() and raises BadRequest with the InvalidEvent identifier and exception message.
  • tests/test_event_write_validation.py — Adds a temporary PeeweeStorage application fixture and HTTP tests using patched insert_one and insert_many methods to raise ValueError or RuntimeError.
Previous review passes
commit score findings engine when
c76ad434a2fc 5/5 0 subscription 2026-10-28 23:42 UTC

Reviewed 2789dbb82be5 · panel: codex:gpt-6.1-sol + claude-code:sonnet · subscription 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

Copy link
Copy Markdown
Contributor Author

⚠️ Likely duplicate: #212

This PR appears to substantially overlap with ActivityWatch/aw-server#212:

fix(export): serialize WSGI-safe attachment filenames

Reason: Changed files and diffs suggest overlapping intent. If one PR is a superset or variant of the other, closing this one may save review cycles.

Not a blocker — review proceeds normally. If this is a false positive, dismiss this comment and continue.

Git-Session-Id: 1f863dbc-917c-4c5e-87b0-df7d16e1d33c
@TimeToBuildBob
TimeToBuildBob force-pushed the fix-event-write-validation-response branch from c76ad43 to 2789dbb Compare November 6, 2026 15:36
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (f2aec97) and force-pushed with an explicit lease; new head is 2789dbb. The conflict was only the Ubuntu runner pin, now already on master, so I dropped the redundant CI commit and retained master’s checkout/setup-python v7 updates. The API change and all four regression cases are unchanged (git range-diff confirms the fix commit is patch-identical).

Verification after rebase: make test 148 passed, make typecheck 32 files clean, and Black checks passed for both changed Python files. GitHub now reports no merge conflicts; fresh CI is running.

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.

Return structured 400 when event storage rejects a foreign-bucket ID

1 participant