feat: add reportable flag for agents and repo metadata to internal errors #76
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When the MCP server encounters an internal_error, it now automatically files a GitHub issue on supermodeltools/mcp with reproduction context (error type, code, message, details, platform info). Deduplication: - Session-level: same error code won't file twice per process lifetime - GitHub-level: searches for existing open issues before creating Uses gh CLI (fire-and-forget, non-blocking). If gh is unavailable or fails, the error response is unaffected. Closes #74
WalkthroughAdds optional Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
[auto-report] unknown-error: ... instead of [auto-report] UNKNOWN_ERROR: ...
Replace auto-filing machinery with simple metadata on error responses. Internal errors now include reportable: true, repo, and a suggestion with a link to the issues page. Agents can choose to file an issue based on their own permissions and context. Removes error-reporter.ts (auto-filing via gh CLI) in favor of surfacing intent in the conversation context. Closes #74
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/tools/create-supermodel-graph.ts`:
- Around line 371-379: The error object zipError currently includes the full
local path in details: { directory }, which can leak usernames/paths; change the
code that builds zipError (and any related reporting before
reportError/asErrorResult) to replace directory with a sanitized value (e.g.,
use path.basename(directory) or a short hash like
crypto.createHash('sha256').update(directory).digest('hex').slice(0,8')) and
store it as sanitizedDirectory or directoryIdentifier in the details field so
reports no longer contain full local paths (update references around zipError,
reportError, and asErrorResult accordingly).
In `@src/utils/error-reporter.ts`:
- Around line 15-42: The current reportError function marks errors as seen by
calling reportedThisSession.add(error.code) before attempting ghExec, which
means transient failures will permanently suppress retries; move the
deduplication step so the code only adds error.code to reportedThisSession after
a successful check/create (i.e., after confirming no existing issue or after
successfully creating the issue), or alternatively perform the add only on
success and remove/error-roll back reportedThisSession in the catch block;
adjust logic inside reportError around the ghExec calls (and keep logger,
buildIssueBody, REPO, and ghExec usage unchanged) so failures do not leave
reportedThisSession containing the code.
…ests
- SERVER_ERROR suggestion now includes explicit retry guidance alongside
the issue-reporting link, since this error is marked recoverable: true
- Export classifyApiError for direct testing
- Add 12 tests covering reportable metadata:
- Internal errors (UNKNOWN_ERROR, SERVER_ERROR, API_ERROR) include
reportable: true, repo, and suggestion with issues link
- Client errors (auth, forbidden, not_found, rate_limit, timeout,
network) do NOT include reportable flag
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/create-supermodel-graph.ts (1)
904-913: Don’t mark unhandled 4xx as reportable.
Right now HTTP 400/422 land in the default branch and getreportable: true, which conflicts with the “server-side only” rule. Example: a bad request should not trigger bug reporting. Consider gatingreportable(and the bug-report suggestion) tostatus >= 500.🛠️ Example fix (gate reportable to server errors)
- default: - return { - type: 'internal_error', - message: `API request failed with HTTP ${status}.`, - code: 'API_ERROR', - recoverable: false, - reportable: true, - repo: 'supermodeltools/mcp', - suggestion: 'This may be a bug. If you want our team to look into it, open an issue at https://github.com/supermodeltools/mcp/issues with the error details.', - details: { httpStatus: status }, - }; + default: { + const isServerError = status >= 500; + return { + type: isServerError ? 'internal_error' : 'validation_error', + message: `API request failed with HTTP ${status}.`, + code: 'API_ERROR', + recoverable: false, + reportable: isServerError ? true : undefined, + repo: isServerError ? 'supermodeltools/mcp' : undefined, + suggestion: isServerError + ? 'This may be a bug. If you want our team to look into it, open an issue at https://github.com/supermodeltools/mcp/issues with the error details.' + : 'Check the request parameters and base URL configuration.', + details: { httpStatus: status }, + }; + }
🤖 Fix all issues with AI agents
In `@src/tools/create-supermodel-graph.ts`:
- Around line 375-377: The suggestion text hardcodes "open an issue" and is
duplicated in multiple places (e.g., the object with properties
reportable/repo/suggestion in create-supermodel-graph.ts and the other
occurrences mentioned); extract a shared constant (e.g.,
REPORT_ISSUE_AND_PR_MSG) that includes both "open an issue" and fork/PR
contribution guidance, then replace the repeated literal suggestion strings with
that constant (update any exports/imports if used across modules). Ensure you
update all instances referenced (the current occurrence and the repeats at the
other locations) so the message is consistent and DRY while preserving repo
context in the surrounding object (reportable/repo).
♻️ Duplicate comments (1)
src/tools/create-supermodel-graph.ts (1)
378-378: Sanitizedirectorybefore putting it indetails.
Full local paths can leak if an agent copies the error details into a public issue.basename(directory)(or a short hash) is safer.🔧 Example fix
- details: { directory, errorType: error.name || 'Error' }, + details: { directory: basename(directory), errorType: error.name || 'Error' },
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/create-supermodel-graph.ts (1)
903-913: API_ERROR marks 4xx client failures as reportable.
Line 903-913 appliesreportable: trueto any unhandled HTTP status, so 400/422/etc get flagged as server bugs. That conflicts with the requirement to report only internal/server errors.🔧 Suggested fix (gate reportable on 5xx)
- default: - return { - type: 'internal_error', - message: `API request failed with HTTP ${status}.`, - code: 'API_ERROR', - recoverable: false, - reportable: true, - repo: 'supermodeltools/mcp', - suggestion: 'This may be a bug. If you want our team to look into it, open an issue at https://github.com/supermodeltools/mcp/issues with the error details.', - details: { httpStatus: status }, - }; + default: { + const reportable = status >= 500; + return { + type: 'internal_error', + message: `API request failed with HTTP ${status}.`, + code: 'API_ERROR', + recoverable: reportable, + ...(reportable + ? { + reportable: true, + repo: 'supermodeltools/mcp', + suggestion: 'This may be a bug. If you want our team to look into it, open an issue at https://github.com/supermodeltools/mcp/issues with the error details.', + } + : { + suggestion: 'Check request parameters or base URL; this appears to be a client-side error.', + }), + details: { httpStatus: status }, + }; + }
♻️ Duplicate comments (1)
src/tools/create-supermodel-graph.ts (1)
371-378: Directory path still leaks into error details.
Line 371-378 keeps the full local path indetails.directory, which can end up in filed issues. Please sanitize (e.g., basename or short hash).🔧 Suggested fix (sanitize directory)
- details: { directory, errorType: error.name || 'Error' }, + details: { directory: basename(directory), errorType: error.name || 'Error' },
- Gate reportable on 5xx only (4xx is now validation_error, not reportable) - Restore retry advice on SERVER_ERROR (recoverable: true needs retry guidance) - Sanitize directory path in error details (basename only, no local path leak) - Extract REPORT_REPO and REPORT_SUGGESTION constants (DRY) - Update suggestion to include full loop: issue + fork + PR - Add 4 tests: reportable present on internal errors, absent on client errors, path sanitization Addresses CodeRabbit comments on PR #76.
Collaborator
Author
Addressed all CodeRabbit findings + internal feedbackPushed CodeRabbit fixes:
Internal feedback fixes:
Direction update:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/tools/create-supermodel-graph.test.ts`:
- Around line 167-182: The test uses a platform-specific path and guards
assertions behind a conditional, causing flaky/quiet passes; update the test
that calls handler and mockClient.generateSupermodelGraph to (1) use a
platform-independent non-existent path (e.g., build one under os.tmpdir() via
path.join(os.tmpdir(), 'nonexistent-<random>') and ensure it does not exist
before calling handler) and (2) remove the surrounding if check so you assert
parsed.error.type === 'internal_error' first and then unconditionally check
parsed.error.reportable, parsed.error.repo and parsed.error.suggestion contents;
reference the handler invocation and parsed.error.type when making these
changes.
🧹 Nitpick comments (1)
src/tools/create-supermodel-graph.ts (1)
896-905: SERVER_ERROR suggestion has redundant guidance.The
REPORT_SUGGESTIONconstant already contains the full "open an issue at https://github.com/supermodeltools/mcp/issues ... or fork the repo and open a PR" guidance. But here in the SERVER_ERROR case (lines 903), you're using a customsuggestionstring that includes similar wording. This means:
- You have two places to update if the URL changes
- The SERVER_ERROR suggestion includes extra retry advice which is good, but duplicates the issue/PR guidance
Consider composing the suggestion to avoid duplication:
♻️ Optional: Compose suggestion from constant
return { type: 'internal_error', message: `Supermodel API server error (HTTP ${status}).`, code: 'SERVER_ERROR', recoverable: true, reportable: true, repo: REPORT_REPO, - suggestion: 'The API may be temporarily unavailable. Wait a few minutes and retry. If persistent, open an issue at https://github.com/supermodeltools/mcp/issues with the error details, or fork the repo and open a PR with a fix.', + suggestion: `The API may be temporarily unavailable. Wait a few minutes and retry. If persistent, ${REPORT_SUGGESTION.toLowerCase()}`, details: { httpStatus: status }, };
Agents now receive guidance on connect telling them to check error.reportable and error.repo, and if they have GitHub access, to file an issue or fork + PR a fix. This bridges the gap between the structured error metadata and agent action — without this instruction, agents have no protocol- level reason to act on the reportable flag.
Tell agents to search existing issues before filing to avoid duplicate reports when multiple agents hit the same bug.
Replace flaky handler tests that depended on /dev/null (Unix-only, conditional assertions) with deterministic handler-level checks. The reportable field coverage is fully handled by the classifyApiError unit tests which don't depend on filesystem state. Addresses CodeRabbit comment on PR #76.
jonathanpopham
changed the title
Change repo from "supermodeltools/mcp" to "https://github.com/supermodeltools/mcp.git" so agents can clone/fork directly without guessing the hosting platform.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/tools/create-supermodel-graph.ts`:
- Around line 906-915: The default branch currently marks isServerError (status
>= 500) as reportable but leaves recoverable: false and omits retry guidance;
update the default branch in src/tools/create-supermodel-graph.ts (the switch
handling HTTP status with variables isServerError and status) to set
recoverable: true when isServerError, and add the same retry suggestion/handling
used for the 500/502/503/504 case (include suggestion text like retrying or
exponential backoff) while still keeping reportable/repo/REPORT_SUGGESTION for
server errors.
Align the default branch with the explicit 500-504 cases: unhandled 5xx (507, 511, etc.) are now recoverable: true with retry guidance in the suggestion. 4xx remains recoverable: false. Addresses CodeRabbit comment on PR #76.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Internal errors now include
reportable: true,repo, and asuggestionthat guides agents to file an issue, fork the repo, or open a PR with a fix. This surfaces the intent in the agent's conversation context without requiringghCLI or auto-filing machinery.What's in the error response
{ "error": { "type": "internal_error", "code": "UNKNOWN_ERROR", "message": "An unexpected error occurred.", "recoverable": false, "reportable": true, "repo": "supermodeltools/mcp", "suggestion": "This may be a bug in the MCP server. You can help by opening an issue at https://github.com/supermodeltools/mcp/issues with the error details, or fork the repo and open a PR with a fix." } }Which errors are reportable
SERVER_ERRORAPI_ERRORZIP_CREATION_FAILEDUNKNOWN_ERRORINVALID_API_KEYFORBIDDENRATE_LIMITEDAdditional fixes
validation_error, notinternal_errordetails.directoryusesbasename()to avoid leaking local pathsTest plan
Closes #74
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.