fix(skill_hub): cap the catalog search query string at the hub edge limit - #348
Open
LivXue wants to merge 1 commit into
Open
fix(skill_hub): cap the catalog search query string at the hub edge limit#348LivXue wants to merge 1 commit into
LivXue wants to merge 1 commit into
Conversation
…imit The hub is fronted by a load balancer that answers 403 Forbidden once a request's query string passes 2048 bytes. search() put the caller's query into q verbatim, and that query carries a retrieval query as long as a user's whole message, so a long prompt did not search on a prefix, it lost hub discovery outright: the router swallows a source failure into an empty candidate list, leaving one warning line as the only symptom. Non-ASCII made the real ceiling far lower than it looks. One CJK character costs 9 bytes encoded (three UTF-8 bytes, three characters each), so the budget was about 226 characters of Chinese, under which essentially every non-trivial task ran with no hub candidates at all. Trim q to the longest prefix that fits, measured in encoded width rather than character count, and size its budget from what the other params leave over. Log what was dropped so a trimmed query does not read as a whole one. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
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.
Summary
SkillHubClient.searchput the caller's query into theqparameter verbatim. Thedeployed hub is fronted by a load balancer (
server: awselb/2.0) that answers403 Forbidden-- not414-- once a request's query string passes 2048 bytes, andqcarries a retrieval query that can be as long as a user's whole message. So a long
prompt did not search on a prefix; it lost hub discovery outright. The failure is quiet:
SkillForgeRouter._safe_searchturns a source error into an empty candidate list, so theonly symptom is one warning line and skills silently never being found.
Non-ASCII made the real ceiling far lower than it looks. One CJK character costs 9 bytes
encoded (three UTF-8 bytes, three
%XXcharacters each), so the budget was about 226characters of Chinese -- under which essentially every non-trivial task ran with no hub
candidates at all.
The fix trims
qto the longest prefix that fits, measured in encoded width rather thancharacter count, and sizes its budget from what the other params leave over rather than
assuming
qis alone. It also logs what was dropped, so a trimmed query does not read asa whole one. Measured boundary against the deployed hub: a 2048-byte query string is
served, 2049 is refused.
Two decisions worth naming, so the next reader does not re-derive them:
wire contract, and the client is the one place every caller passes through.
cut a
%XXescape in half and produce invalid UTF-8.Type
Verification
Reproduced against the deployed hub before the fix, bisecting the boundary: a query
string of 2048 bytes returns 200, 2049 returns 403, with
server: awselb/2.0on therefusal. In CJK terms, 226 characters pass (2045 bytes) and 227 fail (2054 bytes).
End-to-end through the real client, on the prompt shape that produced the original 403
(an 860-character mixed Chinese/ASCII query):
The same query untrimmed is a 4951-byte query string and returns 403.
Full suite, and the three new tests proved not to be theatre by reverting only the
client change and watching them go red:
The 72 failures are pre-existing on the base commit and unrelated to this change
(
test_tui_rpc_session.py,test_cli_cron_commands.py,test_cli_import_commands.py,test_cli_onboard_commands.py, and four single-test files).Disclosed gaps:
make lintalso runslint-tui, which could not run here becauseeslintis not installed in this checkout; this change is Python-only and touches noTypeScript.
mypyis not part of this project's dev dependencies, so no type check beyondruff was available. No docs changed: nothing in
docs/stated a length contract for thisparameter, so no text went stale.
Risk
Behaviour change: a query whose encoded form exceeds the budget is now truncated instead
of being sent whole. Before this change such a request was refused outright and the
candidate list came back empty, so a truncated query is strictly more useful than the
status quo -- there is no case that previously worked and now returns less. Queries inside
the budget are byte-identical to before, which the pre-existing envelope test pins.
The parameter order in the emitted URL changes (
qnow serialises aftercategoryandsort) because the budget is computed from what the other params consume. Queryparameters are order-independent on this endpoint.
Rollback is reverting the commit; there is no migration, no persisted state, and no
config surface involved.
Related Issues
N/A