Skip to content

Catch more exceptions #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 30, 2025
Merged

Catch more exceptions #11

merged 1 commit into from
May 30, 2025

Conversation

@Nyholm
Copy link
Member

@Nyholm Nyholm commented May 29, 2025

Q A
Bug fix? yes
New feature? no
Docs? no
Issues
License MIT

We should return -32002 if resource is not found

@Nyholm Nyholm requested a review from chr-hertel as a code owner May 29, 2025 20:25
@Nyholm Nyholm changed the title Catch more exception Catch more exceptions May 29, 2025
chr-hertel
chr-hertel reviewed May 30, 2025
View reviewed changes
src/mcp-sdk/src/Server/JsonRpcHandler.php Outdated
Comment on lines 81 to 84
} catch (ResourceNotFoundException $e) {
$this->logger->warning(\sprintf('Failed to find resource: %s', $e->getMessage()), ['exception' => $e]);

return $this->encodeResponse(new Error($message->id ?? 0, Error::RESOURCE_NOT_FOUND, $e->getMessage()));
Copy link
Member

@chr-hertel chr-hertel May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a design point of view this is too high for this specific exception - i would argue that it should be the responsibility of a resource specific handler to convert to an Error and the JsonRpcHandler should know nothing about specific capabilities

Copy link
Member

@chr-hertel chr-hertel May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least the RequestHandlerInterface should support that by signature

    public function createResponse(Request $message): Response|Error;

but we could also think of ErrorResponse or sth 🤔

Copy link
Member Author

@Nyholm Nyholm May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

Thank you for pointing it out.

Copy link
Member Author

@Nyholm Nyholm May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR updated

@Nyholm Nyholm force-pushed the feat-exceptions branch from d2de1a4 to cf61f6c Compare May 30, 2025 10:32
@Nyholm
Copy link
Member Author

Nyholm commented May 30, 2025

I also made some classes with public properties "read-only"

chr-hertel
chr-hertel approved these changes May 30, 2025
View reviewed changes
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

@Nyholm Nyholm force-pushed the feat-exceptions branch from 157ab7e to c4bc8e1 Compare May 30, 2025 11:18
@Nyholm Nyholm merged commit 19535a6 into main May 30, 2025
24 checks passed
@Nyholm Nyholm deleted the feat-exceptions branch May 30, 2025 11:18
chr-hertel pushed a commit that referenced this pull request Jun 6, 2025
@OskarStark
feat: add o1-mini and o1-preview OpenAI models (#11)
f38a005
chr-hertel added a commit that referenced this pull request Jun 13, 2025
@OskarStark @chr-hertel
fix: remove composer.lock (#11)
2004ec6 
* Fix: Remove `composer.lock`

* fix: ignore composer.lock

---------

Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
chr-hertel added a commit that referenced this pull request Jun 22, 2025
@chr-hertel
refactor: follow up on example structure to have them cleaner and mor…
a616a7d 
…e separated (#11)
chr-hertel added a commit that referenced this pull request Jun 22, 2025
@chr-hertel
feat: integrate demo app
62a8d3c 
* demo-repo/restructure: (31 commits)
  refactor: restructure into demo folder
  chore: update to llm-chain 0.22 (#27)
  refactor: more error handling in vidoe example
  chore: symfony 7.3 update
  fix: pin chroma db version and clean up (#26)
  feat: add demo of GPT vision capabilities based on video stream (#22)
  chore: dependecy update (#23)
  feat: extend wikipedia system prompt by tools (#21)
  chore: update to lib v0.19 (#20)
  refactor: optimize audio chat ui (#19)
  feat: audio example (#18)
  chore: updating dependencies (#17)
  chore: composer update incl twig cve patch (#16)
  chore: llm chain update with system_prompt support (#15)
  chore: install bundle 0.12 (#14)
  chore: update to llm-chain 0.11.0 (#13)
  chore: library update (#12)
  refactor: follow up on example structure to have them cleaner and more separated (#11)
  fix: typed animation only on xhr responses (#10)
  refactor: moving classes to a more component like structure (#9)
  ...
chr-hertel added a commit that referenced this pull request Jun 22, 2025
@chr-hertel
feat: integrate demo app
c2f94e4 
* demo-repo/restructure: (31 commits)
  refactor: restructure into demo folder
  chore: update to llm-chain 0.22 (#27)
  refactor: more error handling in vidoe example
  chore: symfony 7.3 update
  fix: pin chroma db version and clean up (#26)
  feat: add demo of GPT vision capabilities based on video stream (#22)
  chore: dependecy update (#23)
  feat: extend wikipedia system prompt by tools (#21)
  chore: update to lib v0.19 (#20)
  refactor: optimize audio chat ui (#19)
  feat: audio example (#18)
  chore: updating dependencies (#17)
  chore: composer update incl twig cve patch (#16)
  chore: llm chain update with system_prompt support (#15)
  chore: install bundle 0.12 (#14)
  chore: update to llm-chain 0.11.0 (#13)
  chore: library update (#12)
  refactor: follow up on example structure to have them cleaner and more separated (#11)
  fix: typed animation only on xhr responses (#10)
  refactor: moving classes to a more component like structure (#9)
  ...
chr-hertel added a commit that referenced this pull request Jun 23, 2025
@chr-hertel
feat: integrate demo app
03dcf05 
* demo-repo/restructure: (31 commits)
  refactor: restructure into demo folder
  chore: update to llm-chain 0.22 (#27)
  refactor: more error handling in vidoe example
  chore: symfony 7.3 update
  fix: pin chroma db version and clean up (#26)
  feat: add demo of GPT vision capabilities based on video stream (#22)
  chore: dependecy update (#23)
  feat: extend wikipedia system prompt by tools (#21)
  chore: update to lib v0.19 (#20)
  refactor: optimize audio chat ui (#19)
  feat: audio example (#18)
  chore: updating dependencies (#17)
  chore: composer update incl twig cve patch (#16)
  chore: llm chain update with system_prompt support (#15)
  chore: install bundle 0.12 (#14)
  chore: update to llm-chain 0.11.0 (#13)
  chore: library update (#12)
  refactor: follow up on example structure to have them cleaner and more separated (#11)
  fix: typed animation only on xhr responses (#10)
  refactor: moving classes to a more component like structure (#9)
  ...
chr-hertel added a commit that referenced this pull request Jun 23, 2025
@chr-hertel
feature #35: integrate demo application
f54efd0 
* integrate-demo: (31 commits)
  refactor: restructure into demo folder
  chore: update to llm-chain 0.22 (#27)
  refactor: more error handling in vidoe example
  chore: symfony 7.3 update
  fix: pin chroma db version and clean up (#26)
  feat: add demo of GPT vision capabilities based on video stream (#22)
  chore: dependecy update (#23)
  feat: extend wikipedia system prompt by tools (#21)
  chore: update to lib v0.19 (#20)
  refactor: optimize audio chat ui (#19)
  feat: audio example (#18)
  chore: updating dependencies (#17)
  chore: composer update incl twig cve patch (#16)
  chore: llm chain update with system_prompt support (#15)
  chore: install bundle 0.12 (#14)
  chore: update to llm-chain 0.11.0 (#13)
  chore: library update (#12)
  refactor: follow up on example structure to have them cleaner and more separated (#11)
  fix: typed animation only on xhr responses (#10)
  refactor: moving classes to a more component like structure (#9)
  ...
@chr-hertel chr-hertel added the MCP SDK Issues & PRs about the MCP SDK label Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chr-hertel chr-hertel chr-hertel approved these changes

Assignees

No one assigned

Labels

MCP SDK Issues & PRs about the MCP SDK

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Nyholm @chr-hertel