fix(den): widen config_object.search_text and bound the derived projection - #3386
Open
benjaminshafii wants to merge 1 commit into
Open
fix(den): widen config_object.search_text and bound the derived projection#3386benjaminshafii wants to merge 1 commit into
benjaminshafii wants to merge 1 commit into
Conversation
…ction A GitHub skill import (POST /v1/plugins/import-mcps-from-github-url) crashed with MySQL 1406 "Data too long for column 'search_text'". The skill projection carries the whole SKILL.md body, but search_text was TEXT (65,535 bytes) and nothing on the import path capped it: the 1 MiB request guard only covers POST /v1/config-objects, while importGithubPluginMcps calls createConfigObject in process. Migration 0048 widened config_object_version.raw_source_text and normalized_payload_json for this exact failure and missed this sibling column, so the full SKILL.md persisted in the version row and blew up on the projection. - widen config_object.search_text to mediumtext (migration 0050) - clamp the derived title to varchar(255); the non-skill branch took an unclamped "<plugin> / <server>" name or an arbitrary file's first line - enforce the 1 MiB payload cap on imported and connector-synced skills so an oversized repository returns 400 skill_source_too_large instead of a 500 - clamp non-skill projections to the same budget; search_text ships inside list responses, so mediumtext alone would allow 16 MB payloads
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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
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.
The crash
Production Sentry,
den-api,POST /v1/plugins/import-mcps-from-github-url:config_object.search_textwasTEXT(65,535 bytes), but for skills the projection is the entireSKILL.mdbody (store.tsderiveSkillProjection), and nothing on the import path capped it. The 1 MiBrequest guard only protects
POST /v1/config-objects;importGithubPluginMcpscallscreateConfigObjectin process and bypasses it. MySQL/PlanetScale run strict mode, so the row is rejected rather than truncated.
For a skill row this was the only column that could overflow —
titleis validated to 64 chars anddescriptionto 1024 — which is why the Sentry params contained the whole SKILL.md.This is the sibling that d2b6199 / migration
0048missed. That commit widenedconfig_object_version.raw_source_textandnormalized_payload_jsontomediumtextfor exactly thisfailure, so the full SKILL.md saved fine in the version row (MEDIUMTEXT, ~12.58 MB of plaintext) and blew
up on the projection row (TEXT, 64 KiB).
Changes
den-db/src/schema/sharables/plugin-arch.tstext("search_text")->mediumtext("search_text")den-db/drizzle/0050_lean_lockjaw.sqlALTER TABLE config_object MODIFY COLUMN search_text mediumtext;plugin-system/store.tsclampConfigObjectTitle—titleisvarchar(255)and the non-skill branch derived it from an unclamped"<plugin> / <server>"name or an arbitrary connector file's first line, so it could 1406 on the same insertplugin-system/store.tsderiveSkillProjection— an oversized repository now returns400 skill_source_too_largeinstead of a driver 500plugin-system/store.tsclampSearchProjection— non-skill projections held to the same budget;search_textships inside list responses, somediumtextalone would allow 16 MB payloadsplugin-system/schemas.ts65535->configObjectInputMaxPayloadBytes(now exported)Tests run
New:
ee/apps/den-api/test/plugin-system-config-object-projection-size-db.test.ts(5 cases, real MySQL 8.4).The red run before the fix reproduced the Sentry event verbatim — same statement, same column list,
errno: 1406, code: ER_DATA_TOO_LONG, column 'search_text'. Attribution was verified by toggling thecolumn type back on the test database:
search_text=textsearch_text=mediumtext1406 ... 'search_text'U+FFFDAlso run:
npx tsc -p tsconfig.json --noEmitinee/apps/den-apiandee/packages/den-db— both clean0050_lean_lockjaw.sqlapplied against a pre-0050database (search_text->mediumtext, 16,777,215)db:migrate/db:pushon a scratch database agrees with the schema (no pending diff on re-generate)plugin-system-*(access, create-bundle, cross-org-idor,marketplace-defaults, marketplace-seeding-db, config-object-ownership),
github-plugin-import-schema,github-plugin-mcp-auth,github-discovery,connector-cleanup,deprecated-skill-hubsHonest gaps
proof. Driving the import through the desktop/cloud UI would need a Den server plus a public repo
shipping a >64 KiB skill — happy to do it if a reviewer wants it.
test/mcp-agent-config-policy.test.tshas 1 pre-existing failure (an import-route capability-searchassertion). Confirmed identical on unmodified
origin/dev— unrelated to this change, not fixed here.Deploy note
The
ALTERwidens a column onconfig_object(rewrite; no index onsearch_text). Backward compatible —widening only. On PlanetScale it goes through a deploy request.
Review asks
Please don't rubber-stamp this. Specific things worth pushing back on:
0048.The alternative was truncating
search_textto 64 KiB with no migration. Is a rewrite onconfig_objectacceptable at current table size?clampSearchProjection(1 MiB, lossy) for non-skill objects vs a hard 400 like skills get. I choselossy because a connector-synced file has no authored contract to reject, but that is a judgement call.
400 skill_source_too_largeis a behaviour change for connector sync: a >1 MiB skill file nowfails its sync instead of erroring later at the driver. Confirm that is the failure mode we want.