Apply the documented limit to GET /v1/dev/user/goals when include_inactive is true

[ZachL111]

GET /v1/dev/user/goals ignores its own documented limit when include_inactive=true, and
returns every goal the user has ever created.

routers/developer.py clamps the limit and then branches:

# Clamp pagination so a negative value cannot reach Firestore (which raises -> HTTP 500) and an
# oversized limit cannot stream the whole collection. Mirrors the GET /v3/memories hardening.
limit = max(1, min(limit, 1000))
if include_inactive:
    goals = goals_db.get_all_goals(uid, include_inactive=True)
else:
    goals = goals_db.get_user_goals(uid, limit=limit)

get_user_goals is bounded and ends in return goals[:limit]. get_all_goals
(database/goals.py) has no limit parameter at all, no .limit() and no slice, so the clamp
is dead code on that branch. The exact thing the comment says it prevents, streaming the whole
collection, is what happens.

The endpoint documents the parameter in its own docstring, published in
docs/api-reference/openapi.json:

- **limit**: Maximum number of goals to return

So ?include_inactive=true&limit=5 returns everything, while ?include_inactive=false&limit=5
correctly returns 5.

Why the fix is at the call site

get_all_goals is deliberately the fetch-everything helper. It has five other production
callers that legitimately need the full set: the single-goal lookup at developer.py, the
/v1/goals/all route in routers/goals.py, and the MCP goal reads in mcp.py and mcp_sse.py.
Adding a limit to the helper would change all of them.

Unbounded is correct where no limit is advertised. routers/goals.py::get_all_goals declares
no limit parameter at all, and that is pinned by
tests/unit/test_workstream_router_contract.py. The dev route advertises a limit and then drops
it, which is the asymmetry this fixes. The sibling dev route at developer.py:1354 clamps
correctly.

goals = goals_db.get_all_goals(uid, include_inactive=True)[:limit]

No signature change, so no OpenAPI or generated-client regeneration is required.

Tests

tests/unit/test_dev_goals_limit_include_inactive.py:

• include_inactive=true with limit=5 over 25 goals returns 5
• the clamp ceiling still applies: limit=99999 over 1500 goals returns 1000
• the active-only branch still delegates its limit to get_user_goals unchanged

The seam is monkeypatch.setattr(developer.goals_db, ...) plus a direct handler call, matching
the existing pattern in test_workstream_router_contract.py. No sys.modules mutation.

Verification

Run in backend/:

• pytest new test plus test_workstream_router_contract.py and test_goals_id_fallback.py:
  31 passed
• prove-fail: with routers/developer.py reverted to origin/main and confirmed byte-identical
  (git diff origin/main empty), the two include_inactive tests fail with assert 25 == 5 and
  assert 1500 == 1000, while the active-only test still passes. Re-applied: 3 passed
• black --line-length 120 --skip-string-normalization --check: clean
• pyright routers/developer.py: 0 errors
• scripts/check_module_stub_pollution.py: 733 files, 0 violations
• scripts/pr-preflight --suggest: no product invariants affected, no failure-class
  declaration required
• the three added lines cross the frozen line-count ratchet for routers/developer.py, so
  .github/scripts/product_file_line_count_ratchet_baseline/backend-routers.json is raised from
  2186 to 2189 with the required one-line raise_justifications entry

[ZachL111]

Note on the red Backend unit suite: the failure is not from this change.

The failing test is
tests/unit/test_developer_memory_adapter.py::test_developer_vector_adapter_uses_hydrated_vector_service_and_preserves_ranking_without_archive_default,
asserting ['long-term'] == ['long-term', 'fresh-short-term']. That is the stale date-anchor
in the memory adapter fixtures: the fixtures pin a fixed datetime(2026, 6, 19, ...), so a
memory the test calls "fresh short-term" is no longer inside the freshness window relative to
now and drops out of the result.

This PR touches routers/developer.py and adds tests/unit/test_dev_goals_limit_include_inactive.py.
It does not touch the memory adapters or their fixtures. The test is selected here only because
the changed-file selector maps routers/developer.py onto test_developer_memory_adapter.py by
name.

#9968 replaces those pinned anchors with datetime.now(timezone.utc) and covers this exact
file. It is approved and mergeable, and landing it clears this same red across the other PRs
that touch developer-prefixed paths.

[Git-on-my-level]

Thanks for the focused fix and the regression coverage here. I agree this endpoint should not ignore limit when include_inactive=true, and keeping the existing fetch-everything helper behavior for other callers is the right constraint.

I’m requesting one change before this lands: the new slice bounds the response, but it still calls goals_db.get_all_goals(uid, include_inactive=True) without any storage/query limit, so /v1/dev/user/goals?include_inactive=true&limit=5 can still stream every historical goal for the user before trimming in Python. That leaves the performance/abuse concern from the existing clamp comment unresolved ("an oversized limit cannot stream the whole collection"), even though the returned payload length is now correct.

Could you push the bound down to the data-access/query path for this call site without changing the fetch-everything semantics of the other callers? For example, an optional limit/order-aware path in the goals DB helper (defaulting to current behavior for existing callers) or a dedicated bounded helper for this route would address the original issue more completely. It would also be good for the regression test to assert the bounded helper/query path is used, not only that the final list is sliced.

I also could not validate the new backend test locally in this review environment because the checkout does not have the backend Python dependencies installed (fastapi import fails during collection), and the GitHub Backend unit suite is currently red. The author’s note points to an unrelated stale-fixture failure, but this should still get a clean maintainer/CI pass before merge.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[ZachL111]

You are right, and the new head fixes it properly. Slicing the returned list corrected the
payload length while still streaming every historical goal first, which leaves exactly the cost
the clamp's own comment claims to prevent.

Taking your first option: get_all_goals gains an opt-in keyword-only limit that bounds the
read at the query, and every existing caller keeps fetch-everything behaviour by omitting it.

if limit is not None:
    query = query.order_by('created_at', direction=firestore.Query.DESCENDING).limit(limit)

Two things worth calling out, since neither is obvious from the diff:

The ordering is load-bearing, not decorative. The function sorts by created_at descending
after fetching, so a bare .limit(n) would return an arbitrary n documents that merely look
sorted once the in-Python sort runs. Ordering the query by the same key is what makes the bounded
page the newest n, matching what the unbounded path returned.

The limit is only accepted with include_inactive=True, and raises ValueError otherwise.
That shape carries no equality filter, so ordering by created_at is served by Firestore's
automatic single-field index. Pairing a limit with the is_active filter would need a composite
index this project does not declare, and Firestore answers a missing composite index with an
opaque 500. I would rather that combination fail loudly in a unit test than in production. Every
goal written by create_goal carries created_at (_new_goal_payload sets it), so the ordering
does not drop normally created documents.

On your second point, the tests now assert the bounded path is taken rather than only that the
final list is short:

• the router delegates the clamped limit to the helper, asserted on the captured kwargs
• the 1000 ceiling is delegated the same way
• the DB helper pushes both order_by('created_at') and limit(n) into the query
• omitting limit pushes neither, so existing callers still fetch everything
• a limit with include_inactive=False raises
• the active-only branch still delegates to get_user_goals unchanged

Prove-fail against origin/main (confirmed byte-identical first): four tests fail with
get_all_goals() got an unexpected keyword argument 'limit', while the two control tests
(active-only branch, unbounded default) pass both ways. Re-applied: 6 passed. Plus
test_goals_id_fallback.py and test_workstream_router_contract.py: 34 passed together. black
clean, pyright 0 errors.

On the environment note: that is fair, and the red Backend unit suite was not from this PR. The
failing tests are the memory adapter fixtures pinning a stale datetime(2026, 6, 19, ...), which
#9968 fixes across all four files. It is approved and mergeable; landing it clears that red here
and on the other PRs whose selector pulls those tests in. I have also merged current main into
this branch, which refreshes the base.

[ZachL111]

Correction to what I said earlier about the memory adapter failures.

I said the fix for them was #9968 and that landing it would clear the red. That is no longer
right, and I have closed #9968.

Main fixed the same problem itself in 60eda16 ("test(memory): freeze vector adapter fixture
clock") on 2026-07-19, after I had last pushed to #9968. Rather than my approach of moving the
fixtures onto real wall-clock, main kept the deterministic MEMORY_ADAPTER_FIXTURE_NOW anchor and
added freeze_default_vector_eligibility_clock(monkeypatch, now=now), which pins the production
is_default_access_eligible check to the same fixture clock. That removes the time dependence
instead of relying on it, and it is wired into all four adapter suites. It is the better fix, so
mine is now redundant.

Verified on origin/main at 7429104 with nothing applied: test_chat_memory_adapter.py 11
passed, test_developer_memory_adapter.py 24 passed, test_mcp_memory_adapter.py 26 passed,
test_product_memory_router.py 17 passed.

So nothing needs to be merged to unblock this. Any PR still showing that failure just needs
current main merged in, which I am doing.

[kodjima33]

Dev API goals limit fix. Approve-only — existing CHANGES_REQUESTED review outstanding.

Current head pushes the clamped limit into the bounded goals query and adds regression coverage, resolving this automation review blocker.

[Git-on-my-level]

Thanks for the update — the current head addresses the blocker from my earlier review.

The fix now pushes the clamped limit into the Firestore query for the include_inactive=true route, keeps the existing fetch-everything behavior for callers that omit limit, and the regression tests assert both the router delegation and the bounded query path. I also re-ran the targeted backend coverage locally after syncing the backend Python environment:

python -m pytest tests/unit/test_dev_goals_limit_include_inactive.py tests/unit/test_goals_id_fallback.py tests/unit/test_workstream_router_contract.py -q

Result: 34 passed.

I’m not formally approving from this automation run because the PR is still under the maintainer-review gate (needs-maintainer-review / security-sensitive API surface), but from a code-review perspective this looks ready for human maintainer sign-off.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.