Repository navigation
fix: keep word spacing in minified descriptions - #864
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: cda16b523594a5988347f4f4 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Changeset file added - thank you! |
Review SummaryOverall: Findings: None blocking. The Test coverage: Good. The new unit test covers tabs, CRLF, space runs, and leading and trailing whitespace. The second test covers block-string descriptions and Recommendation: Approve Reviewed by Claude Code Sonnet 5.5 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. Straightforward, well-tested fix: normalize_description now trims and collapses whitespace runs to a single space instead of deleting all whitespace, with the snapshot and changeset updated to match, and two new unit tests covering whitespace-run collapsing and @ deprecated reason normalization.
What was reviewed: the regex/trim logic in normalize_description (crates/apollo-mcp-server/src/introspection/minify.rs), the updated minify_schema.snap fixture for correct spacing, the new unit tests for tabs/CRLF/multi-space runs and deprecated-reason normalization, and the changeset entry describing the patch-level fix.
Extended reasoning...
The diff is confined to one small function (normalize_description) plus its tests, a snapshot fixture, and a changeset file — no security-sensitive surface (no auth, crypto, injection, or data exposure concerns). The fix correctly replaces desc.trim() + regex collapse-to-single-space logic in place of the old strip-all-whitespace behavior, matches the PR description exactly, and is backed by new targeted unit tests plus an updated snapshot that visibly demonstrates the corrected output. No outstanding objections appear in the timeline (only automated bot comments after the fix commit), and the change is small, mechanical, and self-contained with clear intent.
DaleSeo
left a comment
DaleSeo
left a comment
There was a problem hiding this comment.
Thanks for finding and fixing this, @inanna-apollo!
| /// Normalize description formatting | ||
| #[allow(clippy::expect_used)] | ||
| fn normalize_description(desc: &str) -> String { | ||
| // LLMs can typically process descriptions just fine without whitespace | ||
| // Each whitespace run collapses to a single space, and leading and trailing whitespace is | ||
| // removed, so descriptions stay on one line while words remain separated. | ||
| static WHITESPACE_PATTERN: OnceLock<Regex> = OnceLock::new(); | ||
| let re = WHITESPACE_PATTERN.get_or_init(|| Regex::new(r"\s+").expect("regex pattern compiles")); | ||
| re.replace_all(desc, "").to_string() | ||
| re.replace_all(desc.trim(), " ").into_owned() | ||
| } |
There was a problem hiding this comment.
I wonder if the standard library can do this without using a regex.
Problem
normalize_descriptioninintrospection/minify.rsdeleted every whitespace run, so minifiedintrospectandsearchoutput ran descriptions together:That is hard to read for people and models alike, and descriptions are often where schemas put essential usage information. For example, Atlassian's beta fields document the
@optIn(to: "...")directive they require only in their description. Merging words also changes how models tokenize them, so the token savings are smaller than they look.Found while running Apollo MCP Server against the Atlassian GraphQL gateway schema.
Fix
Each whitespace run collapses to a single space, and leading and trailing whitespace is trimmed. This covers type, field, argument and input field descriptions, and
@deprecatedreasons.Against the Atlassian schema,
introspect JiraRichTextnow returns:Tests
minify_schemasnapshot. It's the only snapshot that changes.normalize_description_collapses_whitespace_runs, which covers tabs, CRLF, runs of spaces, and leading and trailing whitespace.deprecation_reason_is_normalized, which uses a multi-line block-string description and@deprecatedreason.cargo fmt,cargo clippy --all-targets -- --deny warningsandcargo test -p apollo-mcp-serverpass.Not in scope
Minified descriptions are wrapped in
"..."without escaping, so a description that contains"can't be read unambiguously. That was already true before this change. Fixing it would change the output format, so it's left for a separate change.