fix: extend the ignored-filter guard and drop eight read-model range bounds by man8-octoflow[bot] · Pull Request #16 · man8/targetprocess-py

man8-octoflow · 2026-09-20T12:18:32Z

Summary

Three related read-path changes, on one branch because they land in the same two
files. The guard extension closes a silent-wrong-results hole on four
collections; the other two remove a latent ParseError and document a field
whose behaviour was unstated.

The work items these close are tracked outside this repository, so no issue
number is referenced here.

Changes

Refuse the ignored `Assignments` filter on four more collections

TargetProcess accepts a where= on the Assignments collection of an assignable
and silently ignores it, answering HTTP 200 with unfiltered rows. The library
refuses that filter before any request is sent. Four collections that also expose
assignable rows were uncovered: PortfolioEpics, TestPlanRuns,
InboundAssignables, OutboundAssignables. All four ignore the filter, so all
four are now refused — a caller narrowing one of them by assignee was receiving
silently unfiltered results.

None of the four has a typed manager, so the refusal lives in the generic
entities map, which is both the only place it can be applied and the only route
a caller has to them. The set of refused paths is unchanged: this widens the
collections covered, not the paths, and ASSIGNABLE_IGNORED_FILTER_PATHS still
has exactly one key.

Each was admitted against an unfiltered control on the same collection, as the
declaration's own admission rule requires. Every one of their /meta
descriptions declares an Assignments collection, alongside the rest of the
assignable surface (AssignedUser, Times, RoleEfforts, Impediments,
TeamStates).

Collection	Unfiltered control	Filtered	Outcome
`PortfolioEpics`	whole collection in one page — 18 rows, no `Next`	18 rows	id sequences identical
`TestPlanRuns`	50-row page ordered by `Id`, no duplicates	50 rows	id sequences identical
`InboundAssignables`	38 rows / 36 distinct, bounded `Id` range	36 rows / 36 distinct	distinct id sets identical
`OutboundAssignables`	67 rows / 59 distinct, bounded `Id` range	59 rows / 59 distinct	distinct id sets identical

Two filter values were used against each: a user Id that exists on the instance,
and one that cannot exist on any. The second is what discriminates — an honoured
filter must return nothing for it, and each collection returned every row
instead.

The two relation collections needed the comparison bounded to an Id range
rather than page-to-page, and that is worth recording: they emit one row per
relation, so an unfiltered page spends slots on duplicate ids and the page
boundary falls elsewhere. Compared page-to-page they looked non-identical,
which is a paging artefact and not filtering. Bounded, the distinct sets are
exactly equal.

Instrument controls. where=(Id eq <id>) returns exactly one row on each of
the four, so where= demonstrably reaches the server and discriminates there —
"the filter was ignored" cannot be confused with "the filter never arrived".
Re-running the already-admitted case on a typed collection reproduced it exactly,
so the probe is known to register this specific failure shape.

The coverage is what has been checked, not an enumeration. The four were
named for checking rather than derived from a walk of the instance's
collections, so an untyped Assignable-derived collection absent from the map is
unproven, not cleared. SPEC.md and the map's own comment both say so, and
both name the TestPlan family as the obvious remaining candidate —
TestPlanRun is covered, TestPlan is not.

Assignments.Count fails differently, and neither of the two ways expected.
It is not silently ignored: the query parser rejects it with HTTP 400
(BadRequest, "Error during parameters parsing.") for eq 0, gt 0 and
gt -1 alike — and Comments.Count is rejected identically, so the rejection is
generic to a collection Count in where= rather than anything about
Assignments. The refusal is kept rather than narrowed: letting it through
would surface a 400 whose body names no field, which serves a caller worse than a
ValueError naming the join-entity route. The reason text now states both
shapes, so it is true whichever path was written, and a test pins both halves.
The Raises: blocks and docstrings a caller reads now say TP "will not filter
on" the path rather than "silently ignores" it, which the Count finding makes
inaccurate.

Drop range constraints from eight server-read fields

AssignableEntity already states the rule for this class of field: the numeric
fields carry no range constraint, because a constraint on a server-supplied value
fails the whole entity rather than the field — one out-of-range roll-up would
abort a whole list() page with a ParseError. RoleEffort (six fields) and
Time (two) were breaking it, and SPEC.md's own Numeric range constraints
section carved those two models out of the rule; that carve-out is corrected
here, since leaving it would have left the normative document contradicting the
code.

The constraints were dormant rather than firing, since the instance clamps every
over-spend at zero, so this removes a latent failure rather than an active one. A
consumer running against an instance or version that does not clamp the same way
would hit it — and as a published package, that consumer is not hypothetical.

times.upsert keeps its write-side validation unchanged, which is where this
library's own reasoning puts a range check. Its Raises: block previously ended
"…(Time.spent/Time.remain carry ge=0)", which this change makes false, so
that prose now says why the write-side check is the only one.

Say what a custom-activity time entry's `project` holds

Time.project said only that TP derives it from the Assignable — true for a
work-item entry, but a CustomActivity entry has no Assignable at all, and
the docs did not say what project then holds.

It is populated. Across 445 live custom-activity entries, every one carried a
non-null project while its assignable was null, and on every one the project
equalled that activity's own project. Both the class docstring and the field
description now state that, and the existing custom-activity test sets and
asserts Project. The docstring stops short of asserting a mechanism: since no
activity carries time on more than one project, copying the activity's project
and copying the project the entry was filed under are indistinguishable from this
evidence.

A CustomActivity is project-scoped, so one conditional change did not apply.
The same read settled it, and the existing CustomActivity docstring claim —
"scoped to a Project and a User" — is correct: 46 of 46 activities carry
both a project and a user, and 0 of 41 activities carrying time span more
than one project. The name-repeat half holds too: 46 activities carry 18 distinct
names, and two repeated names span different projects, so
custom_activities.resolve(name) genuinely can meet an ambiguous name and its
AmbiguousMatchError rationale is sound. Nothing in the lookup model or the
usage docs needed correcting, and resolve()'s behaviour is untouched either way.

Testing

uv run pytest -q — 1450 passing; coverage 99%.
A refusal test per newly covered collection, for both spellings TP accepts,
plus a forward-the-valid-filter counterpart so the guard is shown not to
over-refuse on those collections.
Parse tests feeding a negative value into each of the eight fields. The two
existing tests that asserted the opposite are inverted rather than deleted.
A test pinning that the guard's reason text states both observed failure
shapes, so neither half can be dropped silently.
Mutation-tested against the base revision, not against a variant of the new
code: restoring entities.py and _joins.py from the base commit fails 17 of
the new assertions, and restoring base.py alone fails the reason-text test —
18 in total, independently reproduced during review. Every change is
load-bearing.
Checked by hand that the four new collections still pass through quoted
literals (Name contains 'Assignments.cs'), lookalike paths
(TeamAssignments.*, Assignment.Id, Owner.Assignments.Id) and
AssignedUser.Id untouched.
No cassette was recorded and none is needed: the guard is a pre-request
refusal whose tests mock the request handler, so a cassette would have to
record traffic the library exists to prevent sending. The Time test asserts a
model shape from a dict fixture exactly as every other model test does. The
commit that first introduced this guard added no cassette either.

Context

Backward compatibility. Both commits are API-visible on a published
package, and CHANGELOG.md records both directions. The new refusal raises
ValueError where 0.2.1 returned rows — for a caller who was receiving
silently-wrong rows. Removing a validation constraint only widens what parses,
so nothing previously accepted is now rejected, but a caller relying on
ValidationError for a negative server-supplied value no longer receives one
(including on assignment, since the model validates assignment). That is the
point of the change, and the reason the write-side check stays.
Out of scope, flagged for a separate ruling: three ge=0 bounds survive on
server-supplied read fields in the attached-content models
(Attachment.size, UploadedFile.persistedSize, UploadedFile.size), and a
test actively asserts one of them fires. The repository therefore holds two
positions on the same class of field. Practical risk is negligible — a
negative byte count, and UploadedFile is a single-response model rather than
a list() page — but it is outside the eight fields this change covers, so it
is named in SPEC.md rather than silently changed.
SPEC.md is at its line ceiling. It was 996 lines against a 1000-line
ceiling before this change and is 999 now, so the notes there are deliberately
terse and the per-collection evidence lives in the commit messages and this
description instead. Any further prose in that file trips
scripts/check_large_files.py; splitting it along a real seam is separate work.
_spellings() derives plurals by appending s/es, so the new entries
also yield spellings TP has no collection for (testplanrunes). That is
pre-existing behaviour — the map already contained buges, taskes and
assignablees — and harmless: none of the spurious spellings collides with a
real TargetProcess collection name, so no real collection is falsely refused.

Checklist

uv run ruff check . and uv run ruff format --check . pass
uv run mypy --strict src passes
uv run pytest -q passes and coverage stays at or above 90% (99%)
uv run pre-commit run --all-files --hook-stage pre-commit passes
uv run pre-commit run --all-files --hook-stage pre-push passes
[~] New public functions, methods, and classes have docstrings — no new public
API; the one new module-level name is private and commented
No real credentials, tokens, or PII are added to the diff
TODO/FIXME/HACK/XXX markers name an issue (e.g. TODO(#123))

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Unsupported assignment filters are now rejected before a request is sent for additional collection types.
- Collection-field filters and count-based filters now report their differing failure responses consistently.
- Server-supplied negative effort and time values can now be read successfully without invalidating results.
- Write-time validation for submitted time entries remains in place.
Documentation
- Clarified how projects are associated with custom activity time entries, including entries without an assignment.

RoleEffort's six effort/time roll-ups and Time's spent/remain are supplied by the server, and AssignableEntity already states the rule for that class of field: a constraint on a server-supplied value fails the whole entity rather than the field, so one out-of-range record would abort a whole list() page with a ParseError. These two join models were asserting an invariant the API never promised. Live probing found the constraints dormant rather than firing, since the instance clamps every over-spend at zero, so this removes a latent failure rather than an active one. A consumer running against an instance or version that does not clamp the same way would hit it. Time's project description now states what a custom-activity entry holds. TP populates project from the custom activity's own project when there is no Assignable: over 445 live custom-activity entries, every one carried a non-null project, and on every one it equalled that activity's own. times.upsert keeps its write-side validation, which is where this library's own reasoning puts a range check, and its docstring now says why it is the only one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…lections PortfolioEpics, TestPlanRuns, InboundAssignables and OutboundAssignables each expose assignable rows - every one of their /meta descriptions declares an Assignments collection - and each answers a where= on that collection with the unfiltered rows, exactly as the six typed assignable collections do. A caller narrowing one of them by assignee was receiving silently unfiltered results, which is the failure the refusal exists for. None of the four has a typed manager, so the generic entities map is both the only place the refusal can live and the only route a caller has to them. No new resource class is involved, and the set of refused paths is unchanged: this widens the collections covered, not the paths. Each was admitted against an unfiltered control on the same collection, as the declaration's own rule requires. The relation collections needed that comparison bounded to an Id range: they emit one row per relation, so an unfiltered page spends slots on duplicate ids and the page boundary falls elsewhere, which makes a page-to-page comparison misleading. Bounded, the distinct id sets are identical. The filter names a user Id no instance issues, so an honoured filter would return nothing, and each returned every row instead; where=(Id eq <id>) returns exactly one row on each, so where= demonstrably does reach the server there. Assignments.Count fails differently, and neither of the two ways expected: the query parser rejects it with HTTP 400, as it does Comments.Count, so it is not a silently ignored path at all. The refusal is kept rather than narrowed, because a ValueError naming the join-entity route serves a caller better than a 400 whose body names no field, and the reason text now states both shapes so it is true whichever path was written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Follows the pre-push review of the two commits before it. SPEC.md's numeric-range section still carved RoleEffort and Time out of the no-bounds rule - the carve-out those commits remove - so the repository's normative document contradicted its own code. It now states the rule for the join models too, and names the attached-content byte-size fields as the remaining bounded ones. The coverage claim is narrowed to what the evidence closes. Four collections were checked because four were named for checking, not enumerated from the instance, so an untyped Assignable-derived collection absent from the map is unproven rather than cleared, and the TestPlan family is the obvious one left. SPEC.md and the map's own comment both say so now, because a reader of a published library's spec could otherwise take the guard for exhaustive. RoleEffort's docstring no longer calls every numeric field a server-supplied roll-up: the collection is writable per its own /meta, so the accurate statement is that each value arrives from the server on a read, whoever set it there. Nor does it imply a write-side check covers this model - times.upsert is a Time helper, and RoleEffort has no counterpart. Time's project paragraph drops a causal claim the data cannot support. No activity carries time on more than one project, so copying the activity's project and copying the project the entry was filed under are indistinguishable from that evidence. It also notes a narrowed read leaves the reference None. The refusal is now described as one TP will not filter on rather than one it silently ignores, in the Raises: blocks and docstrings a caller actually reads: Assignments.Count is rejected, not ignored. The reason-text test drops its word assertion and keeps the two status codes, which pin the same fact less brittly. CHANGELOG.md gains the entry README promises for every change, covering both directions - the new refusal breaks a caller who was receiving silently unfiltered rows, and the dropped bound removes a ValidationError a caller may have relied on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-09-20T12:18:37Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: man8/targetprocess-py/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2f6dccc0-3adb-4502-b263-3bc69e14420f

📥 Commits

Reviewing files that changed from the base of the PR and between 8b4f41f and 6f9ece2.

📒 Files selected for processing (9)

CHANGELOG.md
SPEC.md
src/targetprocess/_joins.py
src/targetprocess/resources/base.py
src/targetprocess/resources/entities.py
src/targetprocess/resources/times.py
tests/test_resources/test_ignored_filter_paths.py
tests/test_role_effort.py
tests/test_time.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The change rejects unsupported Assignments filters on additional collections before requests. It also allows negative server-supplied effort and time values, retains write-side validation, and documents project references for custom activities.

Changes

Unsupported Assignments filters

Layer / File(s)	Summary
Filter contract and collection wiring `SPEC.md`, `src/targetprocess/resources/base.py`, `src/targetprocess/resources/entities.py`	The filter documentation distinguishes ignored field filters from rejected `Assignments.Count` filters. Generic entity filtering now covers five untyped assignable collections and their spellings.
Filter refusal and forwarding tests `CHANGELOG.md`, `tests/test_resources/test_ignored_filter_paths.py`	Tests verify refusal messages, pre-request failure, valid filter forwarding, and generic-path coverage for the additional collections.

Server-supplied effort and time values

Layer / File(s)	Summary
Read-model constraints and project references `SPEC.md`, `src/targetprocess/_joins.py`, `src/targetprocess/resources/times.py`	`RoleEffort` and `Time` no longer reject negative server-supplied values. Write-side validation remains in `times.upsert`. `Time.project` documents CustomActivity references.
Negative values and custom-activity tests `CHANGELOG.md`, `tests/test_role_effort.py`, `tests/test_time.py`	Tests verify negative roll-up and duration values parse successfully. They also verify that a CustomActivity project is available when `Assignable` is null.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant entities.list
  participant BaseResource.check_where
  participant TargetProcess API
  Caller->>entities.list: Submit an Assignments filter
  entities.list->>BaseResource.check_where: Check the filter path
  BaseResource.check_where-->>entities.list: Reject unsupported path
  entities.list-->>Caller: Raise ValueError

🚥 Pre-merge checks | ✅ 7

✅ Passed checks (7 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarises the two main changes: extending the ignored-filter guard and removing eight read-model range bounds. It is concise and relevant.
Description check	✅ Passed	The description is complete and follows the template. It covers the summary, changes, testing, context, compatibility impact, scope, and checklist. The missing issue number is explained because the wo…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Docstring Coverage (Non-Test)	✅ Passed	PASS. The non-test changes modify public Python classes and APIs only where docstrings already exist: `RoleEffort`, `Time`, `BaseResource`, `EntitiesResource.list`, `BaseResource.check_where`, and `ch…
Checklist Complete	✅ Passed	The pull request description contains one checklist section. It has seven `[x]` items and one `[~]` item. It has no `[ ]` items and no post-merge section requiring exclusion. The checklist is complete…
No Committed Superpowers Artefacts	✅ Passed	The pull-request diff contains nine modified files and no added files. Git reports no changed or added paths under `docs/superpowers/` or `docs/plans/`, and the head tree contains no entries in either…

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

✨ Simplify code

Commit to this branch
Create a new PR

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.

❤️ Share

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

man8 · 2026-09-20T12:41:36Z

@coderabbitai review

coderabbitai · 2026-09-20T12:41:42Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

man8-octoflow Bot and others added 3 commits September 20, 2026 13:56

man8-octoflow Bot requested a review from man8 as a code owner September 20, 2026 12:18

coderabbitai Bot approved these changes Sep 20, 2026

View reviewed changes

man8 approved these changes Sep 20, 2026

View reviewed changes

man8 merged commit 0858953 into main Sep 20, 2026
8 of 9 checks passed

man8 deleted the extend-filter-guard-and-read-model-fixes branch September 20, 2026 13:21

Navigation Menu

Repository navigation

fix: extend the ignored-filter guard and drop eight read-model range bounds - #16

fix: extend the ignored-filter guard and drop eight read-model range bounds#16
man8 merged 3 commits into
mainfrom
extend-filter-guard-and-read-model-fixes

man8-octoflow Bot commented Sep 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 20, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

man8 commented Sep 20, 2026

Uh oh!

coderabbitai Bot commented Sep 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

man8-octoflow Bot commented Sep 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Refuse the ignored Assignments filter on four more collections

Drop range constraints from eight server-read fields

Say what a custom-activity time entry's project holds

Testing

Context

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

man8 commented Sep 20, 2026

Uh oh!

coderabbitai Bot commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

man8-octoflow Bot commented Sep 20, 2026 •

edited by coderabbitai Bot

Loading

Refuse the ignored `Assignments` filter on four more collections

Say what a custom-activity time entry's `project` holds

coderabbitai Bot commented Sep 20, 2026 •

edited

Loading

coderabbitai Bot commented Sep 20, 2026 •

edited

Loading