Repository navigation
get_review_files can silently return empty or incomplete cross-version results on Swarm v11 #45
Description
Summary
get_review_files (and therefore the query_reviews action=files tool) passes the Swarm GET /reviews/{id}/files?from=&to= response through as-is. On Swarm (API v11) we observed this endpoint returning silently wrong cross-version results, and the pass-through layer has no way to detect or flag it — an AI agent consumes the answer as ground truth.
Observed behavior
On a production Swarm v11 instance, a review with ~24 versions backed by shelves:
- Three adjacent version ranges returned 0 changed files even though real changes existed between those versions.
- Two large ranges (involving re-shelves that grew/shrank the shelf from ~700 to ~1300 files) omitted entries: the API reported 473 and 472 files where the authoritative count was 582 and 515.
Verification method: for each suspect range we compared against
p4 diff2 -Od -q "//depot/path/...@=<fromCL>" "//depot/path/...@=<toCL>"which correctly found the changes every time (e.g. 34/3/1 files for the "empty" ranges). A later server-side Swarm fix made the API return complete results matching p4 diff2 — so the root cause was Swarm-side, but during the incident the MCP layer reported "no changes" with HTTP 200, limited=false, no error, no messages.
Why this matters for an MCP server
- Agents doing "review version N vs N+1" conclude "no changes" and skip the diff entirely — a silent correctness failure, not an error.
- There is no metadata the caller can use to distinguish "genuinely no changes" from "server returned incomplete data".
- The failure clustered around versions whose shelves had accumulated historical files from non-replacing re-shelves; other teams' Swarm installs can plausibly hit the same state.
Suggestion
At minimum, treat this failure mode as documented/known. Potentially in p4mcp:
- Validate that requested versions exist in the review and carry consistent
change/pendingmetadata before trusting a cross-version result. - For shelf-backed versions, optionally cross-check suspicious results (e.g. empty non-initial range, or large inventory delta vs. small reported delta) against
p4 diff2 ...@=CLand surface a flag/error on mismatch. - Related feature request that would build on this validation: Feature: bounded structured diffs for review files (pagination, filtering, consistency checks) #44.
Happy to contribute the validation/fallback logic — we run it in a fork and it has caught exactly this case in production.
Activity
github-actions commentedon Oct 8, 2026
A Jira ticket has been created: P4MCP-279
yan-xdt commentedon Oct 8, 2026
Root cause identified (update): the server-side setting
max_changelist_files.Per the official docs (Changelist files limit):
The value lives in
SWARM_ROOT/data/config.php:When unset, the default is 1000.
The documented effects are UI-oriented: changelists/reviews with many files display slower, smaller values "can prevent files from being displayed", and since 2025.1 exceeding the limit disables the Request Review button (to avoid running out of memory).
Changing it requires an admin config-cache reload.
How this matches our incident:
limited=true, at most 1000 entries) — the documented-ish part.files?from=&to=endpoint, however, returned wrong results with no signal at all:HTTP 200,limited=false, emptyerror/messages. Ranges whose real changes fell outside the truncated listing came back as 0 files; large reshuffles came back missing entries.p4 diff2.The documentation/API gap this issue should track: the docs describe the UI effects of
max_changelist_files, but as far as we can find they do not mention that the REST cross-version files endpoint can return silently empty or incomplete results when the underlying changelists exceed the limit, and the API response provides no truncation indicator for this case.Suggested remedies (any subset):
max_changelist_files's effect on the RESTfilesendpoints.limited=trueor an explicit flag) when the cross-version comparison was affected by the cap.p4 diff2as described in the issue body — useful regardless, since deployments keep the default.