Skip to content

fix(resources): skip post-fetch resolution for direct_proxy reads - #5463

Closed
Ewertonslv wants to merge 3 commits into
IBM:mainfrom
Ewertonslv:fix/5451-direct-proxy-resource-read
Closed

fix(resources): skip post-fetch resolution for direct_proxy reads#5463
Ewertonslv wants to merge 3 commits into
IBM:mainfrom
Ewertonslv:fix/5451-direct-proxy-resource-read

Conversation

@Ewertonslv

Copy link
Copy Markdown

🔗 Related Issue

Closes #5451

📝 Summary

In direct_proxy gateway mode, ResourceService.read_resource fetches the
resource live from the upstream and builds the final TextResourceContents /
BlobResourceContents. Execution then fell through into the shared post-fetch
RESOLVE CONTENT block, whose first branch runs getattr(content, "id")
unconditionally. Those MCP-compliant models (mcpgateway/common/models.py) have
no id field, so every direct_proxy text or blob read raised
AttributeError: 'TextResourceContents' object has no attribute 'id'. The
transport layer catches the exception and returns empty content, so the client
silently receives text="" instead of the fetched payload.

Root cause

The direct_proxy branch never marked its content as already-resolved — the
# Skip the rest of the DB lookup logic comment did not actually skip anything.

Fix

Track whether content came from the direct_proxy branch (direct_proxy_read) and
skip the metadata→content resolution for it. The gateway access check, the
resource access check, and the post-fetch plugin hooks all still run — only the
id-based invoke_resource re-resolution is bypassed, since the payload is final.

Tests (before → after)

The existing happy-path direct_proxy tests masked the bug by monkey-patching
id-bearing subclasses over the real models, so production classes never took the
failing path. This PR adds two regression tests exercising the real
TextResourceContents / BlobResourceContents:

  • Without the fix: both raise AttributeError at the getattr(content, "id") call.
  • With the fix: text read returns the live text, image/png blob read returns its
    blob, and invoke_resource is asserted not re-awaited.

pytest tests/unit/mcpgateway/services/test_resource_service.py passes locally
(full file green, including module doctests via --doctest-modules).

📏 Reviewability

  • This PR has one clear purpose
  • The linked issue is not labeled triage
  • Unrelated bugs or improvements are tracked in separate issues/PRs
  • Tests are included with the code they validate
  • If AI-assisted, I understand and can explain the generated changes

🏷️ Type of Change

  • Bug fix

In direct_proxy gateway mode, read_resource builds the final
TextResourceContents/BlobResourceContents from the live upstream fetch,
but execution then fell into the post-fetch "RESOLVE CONTENT" block,
which calls getattr(content, "id") unconditionally. Those MCP-compliant
models have no `id` field, so every direct_proxy text or blob read
raised AttributeError, was swallowed at the transport layer, and
returned empty content to the client.

Track whether the content was produced by the direct_proxy branch and
skip the metadata->content resolution for it, keeping the gateway
access and resource access checks and the post-fetch hooks intact.

Adds regression tests that exercise the real (id-less) production models
instead of the id-bearing test subclasses that previously masked the bug.

Closes IBM#5451

Signed-off-by: Ewerton Silva <ewertoncom297@gmail.com>
@Ewertonslv

Copy link
Copy Markdown
Author

Correction to my checklist above: I ticked "the linked issue is not labeled triage", but #5451 is in fact still labeled triage — I misread the label state when opening this. Apologies. I've commented on the issue with the root-cause analysis so a maintainer can triage it, and I'm happy for this PR to sit until that happens, or to close it if the approach isn't wanted.

For coordination: this and #5467 were opened the same day and both touch read_resource in resource_service.py, but they fix different bugs on different code paths and their hunks don't overlap (lines ~2417 vs ~2430), so they merge cleanly in either order and neither depends on the other. Kept separate deliberately, per the one-concern rule. DCO is signed off.

Resolve conflict in resource_service.py: keep the direct_proxy short-circuit
(issue IBM#5451) alongside upstream's _set_gateway_content template-placeholder
guard.
@jonpspri jonpspri added the COULD P3: Nice-to-have features with minimal impact if left out; included if time permits label Aug 31, 2026
@jonpspri

Copy link
Copy Markdown
Collaborator

Hi @Ewertonslv — sincere apologies for leaving this PR open for so long without a proper review or merge. That's on us, and we're sorry for the lack of responsiveness.

We're closing this PR now as part of a housekeeping pass on long-open contributions. The linked issue (#5451) remains open, so the need is still tracked.

If you're still interested in contributing this change, please feel free to:

  1. Rebase your branch on the current main
  2. Re-open this PR (or open a fresh one)

We genuinely appreciate your contribution and hope to give it the attention it deserves. Thank you for taking the time to contribute to ContextForge! 🙏

@Ewertonslv

Copy link
Copy Markdown
Author

Thanks @jonpspri — no problem at all, and thanks for the transparency about the housekeeping pass.

Rebased onto current main (ffcc0822) and re-verified. GitHub would not let me re-open this PR after the rebase (force-pushing the head branch makes a closed PR non-reopenable), so I took the "or open a fresh one" path: #6503.

Same change, only rebased. One conflict, resolved by keeping both sides — main grew the _set_gateway_content helper right above the if isinstance(content, ...) branch, so the helper is untouched and the direct_proxy_read guard now sits in front of it.

The bug is still present on today's main, and the regression tests still prove it: with the production hunk reverted they fail with AttributeError: 'TextResourceContents' object has no attribute 'id'; with it applied they pass, the full test_resource_service.py is green (309 tests), and ruff/black are clean. Details in #6503.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

COULD P3: Nice-to-have features with minimal impact if left out; included if time permits

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: direct_proxy resource read raises AttributeError ('…ResourceContents' has no attribute 'id') and returns empty content

2 participants