[PPL Lint] Context-fed rules, hover card fixes, and a headless lint API for cross-repo CI

[Hanyu-W]

Description

1. Node-safe headless lint API — extracts the bundle deserializer and the parse-and-lint core so the opensearch-project/sql CI can run OSD's production detectors against a candidate grammar bundle, without launching OSD, Monaco, or a browser. runtime_lint.ts now imports that shared core instead of keeping its own duplicate copies. Needs no new @osd/monaco export.

2. Two context-fed rules + their host probes

• enabled-false-object (warning) — a reference into an object mapped enabled: false. Verified on 3.7: returns null with HTTP 200 and no error. Invisible without tooling, since _field_caps strips the attribute (which is also why typeMap can't detect it).
• wildcard-source-zero-match (info) — a source= wildcard matching no visible index, with up to five "did you mean" candidates. Its hover rendering already shipped on main with no rule producing it.

Both probes reuse existing read-only routes — no new endpoint. Both rules self-suppress when their input is absent. No new worker or context plumbing; both slots are already declared, serialized, and rehydrated on main.

3. Hover card fixes

• The card no longer vanishes the instant the pointer leaves it, so its doc link is reachable. With fixedOverflowWidgets the card renders outside the editor's rect, and Monaco's mouseleave handler then hides it unconditionally — a path neither hover.sticky nor hidingDelay affects. A capture-phase guard swaps the instant hide for a 600 ms grace timer, then replays the event so Monaco still does the real hide. No private Monaco API used.
• One exported ruleIdOf replaces two private copies in the code-action and hover providers.

The HoverFacts.operation rendering fix that was originally here now arrives via #12411, so it is no longer part of this diff.

4. rex-scan-cost advisory rule — rex/parse/grok run their pattern over a source field for every input row; on a text mapping the engine loads _source, decompresses, and evaluates per document (the dominant cost in the pattern-prefilter RFC, opensearch-project/sql#5608). This surfaces that cost.

Advisory by construction: info severity, shipped disabled, no quick-fix. Extraction is row-preserving — planned as a Calcite Project, not a Filter — so a non-matching document is kept with a null extracted value, and a where inserted before the extraction would delete those rows. That is a result change, not an optimization, so the rule only educates and states the caveat inline.

When the pattern begins with a clean literal token, the message names it plus both filter forms and their opposite failure directions (like is an exact substring but is not pushed down on text; match_phrase is pushed down but matches whole analyzer tokens, so it misses values where the token is glued into a larger one). The token is emitted only when it is provably a substring of every string the pattern can match; a top-level alternation, a leading anchor/metaclass/group, an underscore-glued or non-ASCII token, a stopword, a pure-numeric token, or a token below the length floor all bail to no hint. decodePatternLiteral and leadingLiteralToken join findPatternLiteral in pattern_literal.ts, shared with invalid-capture-group-name.

Issues Resolved

N/A — follow-up to #12394 and #12411.

Screenshot

Captured live in Discover's PPL editor against a local 3.8 cluster, on an index whose mapping has an object mapped enabled: false.

1. enabled-false-object — the hover names the field and its enclosing object.

2. wildcard-source-zero-match — "matched 0 of 7 visible indices."

3. Same rule with candidates — "Did you mean one of: screenshot-logs-2026.04.05, screenshot-logs-2026.04.06?"

4. The operation line (context, not this PR) — "Your query — Affects the filter in this query." Captured before the rebase, when the renderFactsLine branch for HoverFacts.operation was part of this diff; that fix now comes from #12411, and the wording there reads "The flagged operation is a filter."

The hover-card reachability fix is not screenshotted: it only manifests with fixedOverflowWidgets, which Explore's query panel sets and Discover's editor does not, so the guard correctly no-ops on the surface above. Its unit test reproduces Monaco's real DOM and fails against the naive selector.

Testing the changes

yarn test:jest packages/osd-monaco/src/ppl src/plugins/data/public/ppl_lint \
  src/plugins/data/public/antlr/opensearch_ppl src/plugins/data/public/ui/query_editor \
  src/plugins/explore/public/components/query_panel src/plugins/query_enhancements

2125 tests pass across 160 suites; yarn typecheck adds no new errors (verified with tsc -b --force, since the cached run reports success without compiling).

Manual (needs a mapping with an enabled: false object):

1. Discover or Explore, PPL selected, dataset whose mapping has an enabled: false object.
2. source=<index> | where <thatObject>.<field> = 1 → warning, hover names the field and its enclosing object.
3. source=no-such-prefix-* → info, hover reads "matched 0 of N visible indices".
4. Hover any marker, move onto the card, then toward "Learn more" — it should stay open long enough to click.
5. rex-scan-cost ships disabled; enable it in Advanced Settings, then source=<index> | rex field=<textField> "error (?<code>[0-9]+)" → info, naming the error token and both filter forms.

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7294d5a.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
	null	low	Base64-encoded strings appear in the `sourceBuilder` fields of Calcite physical plan fixtures. These are serialized Calcite query expressions captured from real explain output, not payloads. Context (field names, surrounding structure, companion fixture deep_pipe.json) confirms legitimacy. Flagged because base64-encoded blobs in committed test data warrant maintainer awareness.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

✅ All unit and integration tests passing

🔗 Workflow run · commit 54b3dd83710f0b98b8d861f9d2c4cf3074718a4e

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 54b3dd8)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Explain-backed lint layer and attribution Relevant files: packages/osd-monaco/src/ppl/lint/explain/explain_attribution.ts packages/osd-monaco/src/ppl/lint/explain/attribution/candidates.ts packages/osd-monaco/src/ppl/lint/explain/explain_cache.ts packages/osd-monaco/src/ppl/lint/explain/tests/explain_attribution.test.ts packages/osd-monaco/src/ppl/lint/explain/tests/explain_cache.test.ts packages/osd-monaco/src/ppl/lint/explain/tests/explain_detectors.test.ts packages/osd-monaco/src/ppl/language.ts packages/osd-monaco/src/ppl/language_explain_layer.test.ts Sub-PR theme: Pattern-literal rules (rex-scan-cost, invalid-capture-group-name) Relevant files: packages/osd-monaco/src/ppl/lint/pattern_literal.ts packages/osd-monaco/src/ppl/lint/rules/rex_scan_cost.ts packages/osd-monaco/src/ppl/lint/rules/tests/invalid_capture_group_name.test.ts Sub-PR theme: Headless lint API extraction and plugin wiring Relevant files: src/plugins/data/public/antlr/opensearch_ppl/runtime_lint.ts src/plugins/query_enhancements/public/plugin.tsx

⚡ Recommended focus areas for review Possible race on abort refcount When a signalled subscriber's abort listener fires after the shared fetch has already resolved (or is about to), liveSubscribers is decremented but settled may still be false briefly, or the promise chain has resolved but the abort listener races the .then that removes the listener. The aborting subscriber's outer promise is resolved with an error even though the shared fetch produced a valid result — this is documented behavior, but note that if signal fires after settled = true, controller?.abort() will still be called (harmless), and the caller receives an error resolution while co-subscribers get the plan. Verify this is intentional for callers that abort as a cleanup after they've already received data. return new Promise<ExplainResolution>((resolve) => { const onAbort = () => { liveSubscribers--; if (liveSubscribers === 0 && !settled) { controller?.abort(); } // This caller is done regardless of what the shared fetch does next; // co-subscribers keep their own pending resolution. resolve({ status: 'error', error: new Error('explain request aborted') }); }; signal.addEventListener('abort', onAbort, { once: true }); promise.then((resolution) => { signal.removeEventListener('abort', onAbort); resolve(resolution); }); }); Timeout leak on race resolution In ProbeBudget.explain, when the timeoutResult promise wins the Promise.race, the underlying explainCache.resolveResult promise is not awaited or cancelled here (the AbortController is aborted, but the pending promise from resolveResult may resolve later). The clearTimeout call also only runs after Promise.race resolves. If the cache request never settles (e.g., subscribers refcount keeps it alive), no leak occurs, but the deadline branch relies on controller.abort() propagating to the shared fetch — which it does not when other subscribers still hold the pending entry. In that case the probe returns an error but the underlying request continues, which is acceptable but worth confirming. const remainingMs = Math.max(1, this.deadline - Date.now()); let timeout: ReturnType<typeof setTimeout> | undefined; const timeoutResult = new Promise<ExplainResolution>((resolve) => { timeout = setTimeout(() => { controller?.abort(); resolve({ status: 'error', error: new Error('probe wall-clock budget exceeded') }); }, remainingMs); }); // Prepare the raw probe text the same way the baseline was prepared (source // prepend + injected filters) so its plan is comparable to the baseline's; // key the cache on the time-stripped variant, exactly like the baseline. const prepared = this.inputs.prepareExplainQuery?.(query) ?? { query, cacheKey: query, }; const resolution = await Promise.race([ explainCache.resolveResult(this.inputs.http, prepared.query, this.inputs.dataSourceId, { partition: 'probe', signal: controller?.signal, cacheKey: prepared.cacheKey, }), timeoutResult, ]); if (timeout) { clearTimeout(timeout); } return this.inputs.isCurrent() ? resolution : { status: 'error', error: new Error('stale lint generation') }; } Stale generation check race layerExplainDiagnostics calls renderLintMarkers(model, [...staticDiagnostics, ...state.immediateDiagnostics]) after isLintPassStale was checked prior to the _explain call but not immediately before this render. A model edit or new lint pass that occurs between the resolution check and this render could cause immediate diagnostics to be rendered on a stale generation, overwriting a newer pass's markers. Consider re-checking isLintPassStale immediately before each renderLintMarkers call in this async path. }; const state = createExplainAttributionState(attributionInputs); // Fast, and the shared first pass for Thorough: render the uniquely-sourced // findings immediately (fixes withheld until a probe confirms them). renderLintMarkers(model, [...staticDiagnostics, ...state.immediateDiagnostics]);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 54b3dd8
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Abort fetch when first caller pre-aborted When every subscriber uses an already-aborted signal, subscribe returns error resolutions without incrementing liveSubscribers, leaving pending populated and the underlying fetch running forever (no abort ever triggers controller.abort(), and the .then finalizer only removes the entry after the fetch settles). Explicitly abort the controller when the initial subscribe call cannot join with any signal (all-aborted first-caller case), or count the aborted signal as a transient subscriber that immediately decrements to trigger cleanup. packages/osd-monaco/src/ppl/lint/explain/explain_cache.ts [211-225] let liveSubscribers = 0; const subscribe = (signal?: AbortSignal): Promise<ExplainResolution> => { if (!signal) { liveSubscribers++; return promise; } if (signal.aborted) { - // Never joined: no refcount change, and the caller gets the same - // error-shaped resolution an abort mid-flight would have produced. + if (liveSubscribers === 0 && !settled) { + controller?.abort(); + pending.delete(k); + } return Promise.resolve({ status: 'error', error: new Error('explain request aborted'), } as ExplainResolution); } liveSubscribers++; Suggestion importance[1-10]: 7 __ Why: Valid edge case: if the very first (and only) subscriber uses an already-aborted signal, liveSubscribers stays 0, the fetch runs to completion but nobody cares, and pending isn't cleared until the fetch settles. The suggestion identifies a real resource leak, though the impact is bounded (fetch will eventually settle and clean up).	Medium
	Fix malformed MDS URL construction The MDS route segment appears malformed: appending /dataSourceMDSId= to the base URL produces a path segment like .../indices.getFieldMapping/dataSourceMDSId=abc rather than the expected query-param or route-parameter form. Verify against the actual proxy route contract; the current construction likely 404s on any MDS request, silently masking disabled-object detection. src/plugins/data/public/ppl_lint/disabled_object_fields.ts [84-88] const mdsId = indexPattern.dataSourceRef?.id; -const url = mdsId - ? `${DSL_MAPPING_URL}/dataSourceMDSId=${encodeURIComponent(mdsId)}` - : DSL_MAPPING_URL; -const resp = await http.get(url, { query: { index: pattern } }); +const query: Record<string, string> = { index: pattern }; +if (mdsId) { + query.dataSourceMDSId = mdsId; +} +const resp = await http.get(DSL_MAPPING_URL, { query }); Suggestion importance[1-10]: 6 __ Why: The suggestion raises a legitimate concern about the URL construction using /dataSourceMDSId=<id> as a path segment, which looks unusual. However, without access to the actual proxy route contract, it's unclear whether this is a bug or an intentional convention used elsewhere in the codebase. The verification-style nature limits the score.	Low
General	Unquote backtick-quoted field identifiers bareFieldSource returns the field name via getText(), which for a backtick-quoted identifier like raw log returns the quoted form. Downstream typeMap.get(field) is presumably keyed by the unquoted name, so a quoted field will fail lookup and silently suppress the finding. Strip surrounding backticks (as candidates.ts#fieldName does) before returning. packages/osd-monaco/src/ppl/lint/rules/rex_scan_cost.ts [85-97] function bareFieldSource( command: ParserRuleContext, ruleNameToIndex: RuleNameToIndex ): string | undefined { const expression = findChildByRule(command, ruleNameToIndex, 'expression'); if (!expression) { return undefined; } const fieldExprs = findAllDescendantsByRule(expression, ruleNameToIndex, 'fieldExpression'); if (fieldExprs.length !== 1 || fieldExprs[0].getText() !== expression.getText()) { return undefined; } - return fieldExprs[0].getText(); + const text = fieldExprs[0].getText(); + return text.startsWith('`') && text.endsWith('`') ? text.slice(1, -1) : text; } Suggestion importance[1-10]: 6 __ Why: Legitimate concern: if a user writes parse `raw log` "...", the returned field name would include backticks and typeMap.get() would miss it, silently suppressing the finding. The candidates.ts file already handles this via fieldName(), so consistency argues for the fix.	Low
	Skip backtick-quoted regions when splitting pipe Splitting on the first | character is unsafe: a source name (or any content before the first pipe) could contain a | inside a quoted string or backticked identifier. Additionally, the comment claims field comparisons like | where source=prod are untouched, but if the query has no pipe at all yet contains source= in an unusual position, behavior may still be surprising. Consider ensuring the pipe search skips over backtick-quoted regions. src/plugins/query_enhancements/public/ppl_lint/explain_query_preparer.ts [39-40] function backtickWrapSource(queryString: string): string { - const pipeIndex = queryString.indexOf('|'); + // Find first pipe that isn't inside backticks + let pipeIndex = -1; + let inBackticks = false; + for (let i = 0; i < queryString.length; i++) { + if (queryString[i] === '`') inBackticks = !inBackticks; + else if (queryString[i] === '|' && !inBackticks) { pipeIndex = i; break; } + } const prefix = pipeIndex === -1 ? queryString : queryString.slice(0, pipeIndex); const suffix = pipeIndex === -1 ? '' : queryString.slice(pipeIndex); - const updatedPrefix = prefix.replace( - /(\bsource\s*=\s*)(`[^`]+`|[^\s|,]+(?:\s*,\s*[^\s|,]+)*,?)/i, - (_match, srcPrefix, sourceValue) => { + // ... rest unchanged +} Suggestion importance[1-10]: 5 __ Why: A valid edge case: a backticked identifier containing | before the first real pipe would be split incorrectly. However, this is an unusual case for a source= clause and the existing tests do not exercise it, so impact is moderate.	Low
	Avoid shared regex state across calls CAPTURE_GROUP_OPENER is a module-level regex with the /g flag, so its lastIndex state is shared across all invocations of extractGroups. Although extractGroups resets lastIndex = 0 at the start, this is not thread/reentrancy safe and can cause subtle bugs if extractGroups is ever called from within itself (or during iteration). Prefer constructing a new RegExp per call, or use matchAll which does not carry state. packages/osd-monaco/src/ppl/lint/rules/invalid_capture_group_name.ts [29] -const CAPTURE_GROUP_OPENER = /\(\?(P?)<([^>]*)>/g; +function extractGroups(literalRaw: string): ExtractedGroup[] { + const groups: ExtractedGroup[] = []; + const re = /\(\?(P?)<([^>]*)>/g; + let match: RegExpExecArray | null; + while ((match = re.exec(literalRaw)) !== null) { + // ... existing body + } + return groups; +} Suggestion importance[1-10]: 4 __ Why: The concern is legitimate for shared regex state with /g flag, but extractGroups explicitly resets lastIndex = 0 and JavaScript is single-threaded, making reentrancy the only real concern which is unlikely here. Minor maintainability improvement.	Low
	Tighten residual-filter rel matching *The comment claims suffix-matching is anchored and cannot collide with names that merely contain the operator, but endsWith('Filter') still matches operators like BloomFilter or any future Filter rel, and endsWith('Calc') matches any rel ending in "Calc". Consider matching against an explicit allowlist of rel operator names (or a stricter pattern) to avoid mis-classifying unrelated rels as residual-filter carriers. packages/osd-monaco/src/ppl/lint/explain/explain_tree_utils.ts [79-90] -const RESIDUAL_FILTER_REL_SUFFIXES = ['Calc', 'Filter']; +const RESIDUAL_FILTER_REL_OPS = new Set([ + 'EnumerableCalc', + 'LogicalCalc', + 'EnumerableFilter', + 'LogicalFilter', +]); -/** - * True when a rel operator name belongs to the Calc/Filter family — the only - * rels that can carry a residual (coordinator-side) filter condition. Join rels - * also serialize a `condition` attribute (their join predicate), but a join - * condition always evaluates at the coordinator by design and must not be - * reported as a filter that failed to push down. - */ export function isResidualFilterRelOp(relOp: string): boolean { - return RESIDUAL_FILTER_REL_SUFFIXES.some((suffix) => relOp.endsWith(suffix)); + return RESIDUAL_FILTER_REL_OPS.has(relOp); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly notes that endsWith('Filter') could match unrelated rels like BloomFilter. However, the existing comment acknowledges the suffix approach and Calcite rel naming conventions make this a low-probability issue in practice. An explicit allowlist would be more robust but is a minor improvement.	Low
	Prevent cache key collisions from delimiter chars The cache key concatenates dataSourceId and query with ::, which can collide when a data source id or query text contains ::. Use a delimiter or length-prefixing that cannot appear in the inputs, or JSON-encode the components, to avoid cross-key collisions where different (dataSourceId, query) pairs hash to the same key. packages/osd-monaco/src/ppl/lint/explain/explain_cache.ts [124-134] private key( query: string, dataSourceId: string | undefined, partition: 'baseline' | 'probe' ): string { - // Probe verdicts depend on the outcome detector; version the key so a - // detector change never reuses a stale probe result. Baseline plans are the - // raw `_explain`, independent of the detector, so they carry no version. - const version = partition === 'probe' ? `::outcomes-${EXPLAIN_OUTCOME_DETECTOR_VERSION}` : ''; - return `${dataSourceId ?? '__local__'}${version}::${query}`; + const version = partition === 'probe' ? `outcomes-${EXPLAIN_OUTCOME_DETECTOR_VERSION}` : ''; + return JSON.stringify([dataSourceId ?? '__local__', version, query]); } Suggestion importance[1-10]: 3 __ Why: The :: collision is theoretically possible but extremely unlikely in practice for dataSourceId (typically UUIDs/simple ids) and query text collisions would require crafted inputs; the risk is minor and the JSON.stringify alternative adds overhead.	Low
	Guard against fused reserved keywords in field pattern The ADDITIVE_RE is constructed once at module load but extractGroups in another file uses a global regex with lastIndex. Since ADDITIVE_RE here is used only with exec on a fresh string (not stateful via /g), that's fine — but ensure the pattern's FIELD group cannot match a leading operator keyword like NOT. The current FIELD pattern [A-Za-z_][\w.]* will match NOTage as a single token if the caller ever passes text with fused keywords. Consider tightening by rejecting names that begin with reserved keywords, or documenting that the caller must pass tokenized text only. packages/osd-monaco/src/ppl/lint/explain/explain_quick_fix.ts [65] -const ADDITIVE_RE = new RegExp(`^${FIELD}\\s*([+-])\\s*${UINT}\\s*${CMP}\\s*${SINT}$`); +// Field pattern: rejects fused reserved keywords (NOT, AND, OR) at the start +const FIELD_RESERVED = '(?!(?:NOT|AND|OR)(?![\\w.]))'; +const ADDITIVE_RE = new RegExp(`^${FIELD_RESERVED}${FIELD}\\s*([+-])\\s*${UINT}\\s*${CMP}\\s*${SINT}$`, 'i'); Suggestion importance[1-10]: 3 __ Why: The PR's own comments and tests (NOTage-2>30) explicitly state that the caller passes only single comparison node text without fused keywords, and a test verifies this case returns undefined because NOTage is not a mapped field. The suggestion adds defense-in-depth but is largely redundant given the type-map lookup guard.	Low
	Preserve no-context lint call semantics Previously, lint() short-circuited to this.analyzer.lint(content) with no context to preserve default behavior. Now it always passes rebuildRunContext(context), which returns undefined when context is missing — this changes the arity of the call. Confirm analyzer.lint(content, undefined) behaves identically to analyzer.lint(content); otherwise consumers not sending context will see behavior changes. packages/osd-monaco/src/ppl/worker/ppl.worker.ts [37-41] -async lint(...): Promise<LintResult> { +async lint( + content: string, + context?: SerializableLintContext + ): Promise<LintResult> { if (!this.analyzer) { this.analyzer = getPPLLanguageAnalyzer(); } - return this.analyzer.lint(content, rebuildRunContext(context)); + const runContext = rebuildRunContext(context); + return runContext + ? this.analyzer.lint(content, runContext) + : this.analyzer.lint(content); } Suggestion importance[1-10]: 3 __ Why: Since context is typed as optional (context?: SerializableLintContext), calling analyzer.lint(content, undefined) should be semantically equivalent to analyzer.lint(content) in TypeScript. The suggestion is a verification request with limited actual impact.	Low
	Ensure timeout timer is cleared reliably validate races the validator against a timeout but does not clear the timeout when the validator rejects. Because the catch block returns undefined without going through the finally clearTimeout path when the timeout was already assigned but the race resolves via rejection before timing out, the pending timer can still fire and hold the event loop. Ensure the timeout is always cleared regardless of outcome (the current finally handles the resolve/timeout paths, but the rejection path in Promise.race can leave the timer pending until the deadline). packages/osd-monaco/src/ppl/lint/explain/explain_attribution.ts [239-252] async validate(queries: string[]): Promise<boolean[] | undefined> { if ( queries.length === 0 || queries.some((query) => !query) || !this.inputs.isCurrent() || Date.now() >= this.deadline ) { return undefined; } let timeout: ReturnType<typeof setTimeout> | undefined; const timeoutResult = new Promise<undefined>((resolve) => { - timeout = setTimeout(() => resolve(undefined), Math.max(1, this.deadline - Date.now())); + timeout = setTimeout(() => { + timeout = undefined; + resolve(undefined); + }, Math.max(1, this.deadline - Date.now())); }); Suggestion importance[1-10]: 2 __ Why: The existing finally block already calls clearTimeout(timeout) regardless of resolve/reject outcome, so the timer is cleared correctly. The suggestion's reasoning about the rejection path is incorrect.	Low
	Respect caller-provided pipe-first flag lintWithGrammar spreads context into the runLint options and then overrides grammarSurface, grammarHash, and isPipeFirst. If a caller passes a context that already contains grammarSurface (e.g. 'compiled'), the spread order correctly overrides it, but if the caller passes isPipeFirst explicitly, it is also overridden by the locally-computed value. This may be intentional, but consider preferring the caller's isPipeFirst when provided, since the caller may have more accurate information (e.g. after column remapping in a pipeline). src/plugins/data/public/antlr/opensearch_ppl/headless_ppl_lint.ts [178-183] +context: { + ...context, + grammarSurface: 'runtime-bundle', + grammarHash: grammar.grammarHash, + isPipeFirst: context?.isPipeFirst ?? isPipeFirst, +}, - Suggestion importance[1-10]: 2 __ Why: The locally-computed isPipeFirst is derived deterministically from the query text and is the authoritative value for this parse. Preferring a caller-provided value could introduce inconsistency with how the query was actually parsed.	Low

---

Previous suggestions

Suggestions up to commit 7294d5a

Category	Suggestion	Impact
Possible issue	Fix malformed data source URL construction The MDS id is appended as a path segment after a slash but uses = as if it were a query parameter, producing a malformed URL like /api/directquery/dsl/indices.getFieldMapping/dataSourceMDSId=. Either append it as a proper query parameter or use the correct path convention expected by the route. src/plugins/data/public/ppl_lint/disabled_object_fields.ts [85-88] -const url = mdsId - ? `${DSL_MAPPING_URL}/dataSourceMDSId=${encodeURIComponent(mdsId)}` - : DSL_MAPPING_URL; +const url = DSL_MAPPING_URL; +const query: Record<string, string> = { index: pattern }; +if (mdsId) { + query.dataSourceMDSId = mdsId; +} +const resp = await http.get(url, { query }); Suggestion importance[1-10]: 8 __ Why: The URL construction ${DSL_MAPPING_URL}/dataSourceMDSId=${...} uses = after a slash which looks malformed. However, this may be the actual convention used by the directquery route; without seeing that route's definition, the concern is plausible but not definitively confirmed. Still, it flags a likely real bug worth reviewing.	Medium
	Guard against missing dot in path collectDottedPathNodes may yield non-dotted names too (bare fields, per the test "does not flag a bare field with no dotted path"). When path contains no ., indexOf('.') returns -1 and path.slice(0, -1) produces the field name minus its last character — which could coincidentally match a disabled root and false-fire. Guard on the presence of a dot before computing root. packages/osd-monaco/src/ppl/lint/rules/enabled_false_object.ts [32-37] for (const node of collectDottedPathNodes(tree, ruleNameToIndex)) { const path = node.getText(); - const root = path.slice(0, path.indexOf('.')); + const dotIndex = path.indexOf('.'); + if (dotIndex < 0) { + continue; + } + const root = path.slice(0, dotIndex); if (!disabled.has(root)) { continue; } Suggestion importance[1-10]: 7 __ Why: Valid concern: if collectDottedPathNodes returns non-dotted names, path.slice(0, -1) would truncate the last character which could accidentally match. The test "does not flag a bare field" passes only if the collector filters or by accident; adding an explicit guard is defensive and correct.	Medium
General	Prevent unhandled rejection when timeout wins race When validateGeneratedQueries rejects, Promise.race rejects and the finally block runs, but the timeout still fires later since it is only cleared in finally (which does clear it) — however, if the timeout wins the race first, validateGeneratedQueries's eventual rejection becomes an unhandled promise rejection. Attach a .catch(() => undefined) to the validator promise to suppress this. packages/osd-monaco/src/ppl/lint/explain/explain_attribution.ts [249-257] let timeout: ReturnType<typeof setTimeout> | undefined; const timeoutResult = new Promise<undefined>((resolve) => { timeout = setTimeout(() => resolve(undefined), Math.max(1, this.deadline - Date.now())); }); try { const result = await Promise.race([ - this.inputs.validateGeneratedQueries(queries), + this.inputs.validateGeneratedQueries(queries).catch(() => undefined), timeoutResult, ]); Suggestion importance[1-10]: 6 __ Why: Valid concern: if the timeout wins and validateGeneratedQueries later rejects, it becomes an unhandled rejection. The try/catch around the race only catches the winner. Adding .catch on the validator promise is a reasonable defensive fix.	Low
	Track probe request cancellation for supersession The abort controller for the baseline explain request is stored on lintAbortControllers, but the isolation probes inside runExplainIsolation create their own controllers internally that aren't tracked here. If a new lint pass starts during the probe phase, abortInFlightExplain only aborts the baseline controller (which is already settled by then), leaving probe requests to complete uselessly. Consider passing an outer signal into the isolation pass or tracking probe controllers as well. packages/osd-monaco/src/ppl/language.ts [482-491] +const controller = typeof AbortController === 'undefined' ? undefined : new AbortController(); +if (controller) { + lintAbortControllers.set(model.id, controller); +} +const resolution = await explainCache.resolveResult( + http, + prepared.query, + lintContext?.dataSourceId, + { cacheKey: prepared.cacheKey, signal: controller?.signal } +); - Suggestion importance[1-10]: 5 __ Why: Valid observation: probe requests inside runExplainIsolation are not tracked by lintAbortControllers, so supersession relies only on isCurrent() guards rather than actual request cancellation. However, isCurrent guards within the probes do abandon stale work, mitigating the impact.	Low
	Remove dead fallback after shape validation isValidBundleShape already asserts literalNames and symbolicNames are present arrays, so the || [] fallback is dead code that also weakens the type narrowing. More importantly, buildSymbolicNameToTokenType(bundle.symbolicNames) below is called with the original (unmapped) array — if a symbolic name is the empty string, it will be skipped by the if (name) check there, which is fine, but the two symbolicNames values are now inconsistent (one has nulls, one has empty strings). This is harmless today but a subtle footgun. src/plugins/data/public/antlr/opensearch_ppl/ppl_grammar_deserialize.ts [174-175] -const literalNames = (bundle.literalNames || []).map((n) => (n === '' ? null : n)); -const symbolicNames = (bundle.symbolicNames || []).map((n) => (n === '' ? null : n)); +const literalNames = bundle.literalNames.map((n) => (n === '' ? null : n)); +const symbolicNames = bundle.symbolicNames.map((n) => (n === '' ? null : n)); Suggestion importance[1-10]: 4 __ Why: Valid observation: since isValidBundleShape already validates these arrays, the || [] fallback is redundant. Minor code cleanliness improvement.	Low
	Deduplicate names before capping Deduplicate names before sorting: an index can appear both as itself and via an alias/data-stream entry in the resolve_index response, which would inflate the count against the 5000-cap and produce duplicate entries the rule then treats as distinct visible indices. src/plugins/data/public/ppl_lint/visible_indices.ts [50-53] -if (names.length > MAX_VISIBLE_INDICES) { +const unique = Array.from(new Set(names)); +if (unique.length > MAX_VISIBLE_INDICES) { return []; } -return names.sort(); +return unique.sort(); Suggestion importance[1-10]: 4 __ Why: Deduplication is a reasonable minor improvement, but the resolve_index API typically returns disjoint buckets for indices/aliases/data_streams, so duplicates are unlikely in practice. Marginal impact.	Low
	Decrement refcount when signalless subscriber settles A signal-less subscriber increments liveSubscribers but never decrements it, which is intentional to pin the request open. However, since the promise is returned directly without any .finally handler to decrement on settlement, the counter is fine — but this behavior effectively means once any signal-less caller subscribes, no combination of aborts can cancel the fetch. Document or consider decrementing on promise settlement to allow cancellation when the pinning caller is done. packages/osd-monaco/src/ppl/lint/explain/explain_cache.ts [211-216] let liveSubscribers = 0; const subscribe = (signal?: AbortSignal): Promise<ExplainResolution> => { if (!signal) { liveSubscribers++; - return promise; + return promise.finally(() => { liveSubscribers--; }); } Suggestion importance[1-10]: 3 __ Why: The signal-less pinning behavior is intentional and documented; changing it could break the design's cancellation semantics. The suggestion itself acknowledges this is intentional, making it low-impact.	Low
	Use the default constant for unknown modes The mode normalization silently coerces any non-'thorough' value (including a valid 'fast' string, but also typos like 'Thorough' with different casing) to 'fast'. However, since DEFAULT_EXPLAIN_MODE is 'fast', the fallback is consistent — but the docstring says "Defaults to DEFAULT_EXPLAIN_MODE when unset or malformed". Consider explicitly comparing against both known modes and defaulting via the constant to keep behavior aligned if DEFAULT_EXPLAIN_MODE ever changes. src/plugins/data/public/ppl_lint/lint_overrides.ts [75-79] if ( stored && typeof stored === 'object' && Array.isArray((stored as { rules?: unknown }).rules) ) { const rawMode = (stored as { mode?: unknown }).mode; + const mode: ExplainMode = + rawMode === 'thorough' || rawMode === 'fast' ? rawMode : DEFAULT_EXPLAIN_MODE; return { - mode: rawMode === 'thorough' ? 'thorough' : 'fast', + mode, rules: (stored as { rules: StoredRule[] }).rules, }; } Suggestion importance[1-10]: 3 __ Why: Minor consistency improvement; current behavior is functionally equivalent since DEFAULT_EXPLAIN_MODE is 'fast', but explicit validation against known modes is slightly more robust to future changes.	Low
	Avoid shared regex lastIndex state CAPTURE_GROUP_OPENER is a module-level regex with the g flag, and while lastIndex is reset at the start of extractGroups, this function is invoked per parse-tree command in a loop. If the code path ever becomes reentrant (e.g. via a rule that recurses or is called concurrently in workers sharing module state), the shared lastIndex could cause skipped matches. Consider creating a fresh regex per call or using matchAll to avoid the shared-state hazard. packages/osd-monaco/src/ppl/lint/rules/invalid_capture_group_name.ts [47-68] -while ((match = CAPTURE_GROUP_OPENER.exec(literalRaw)) !== null) { +const opener = /\(\?(P?)<([^>]*)>/g; +let match: RegExpExecArray | null; +while ((match = opener.exec(literalRaw)) !== null) { const isPythonOpener = match[1] === 'P'; const name = match[2]; Suggestion importance[1-10]: 3 __ Why: The concern is theoretical: lastIndex is reset at the start of extractGroups, and JavaScript is single-threaded, so reentrancy is unlikely. A local regex would be slightly cleaner but not a real bug.	Low
	Reduce querySelector calls on hot event path node.querySelector(HOVER_BODY_SELECTOR) runs on every mouseleave and mouseenter event dispatched to the overflow container, which can include unrelated widgets. Since mouseleave/enter events fire frequently during pointer movement across many child overflow widgets, calling querySelector on each event target on the capture path may become a hot path. Consider caching the current hover widget node (updated via a subtree observer or Monaco widget-visibility hook) or checking a cheaper condition first. src/plugins/data/public/ui/query_editor/lint_hover_persistence.ts [88-96] const hoverWidgetOf = (node: EventTarget | null): Element | null => { if (!(node instanceof Element)) { return null; + } + // Cheap check first: descendant hover body implies this is the widget wrapper. + const body = node.firstElementChild; + if (body && body.classList?.contains('monaco-hover')) { + return node; } if (node.querySelector(HOVER_BODY_SELECTOR)) { return node; } return node.closest(HOVER_BODY_SELECTOR); }; Suggestion importance[1-10]: 3 __ Why: The performance concern is speculative; querySelector on a small overflow-widget subtree is unlikely to be a real hot path, and the suggested optimization adds complexity for negligible gain.	Low
	Prevent infinite recursion on cyclic arrays valueText recurses into arrays without cycle protection, and safeStringify on objects with cyclic references will throw and return '' — losing information. Since rel nodes can reference other rels via inputs, consider tracking visited nodes to avoid infinite recursion should an array contain a self-reference. packages/osd-monaco/src/ppl/lint/explain/explain_tree_utils.ts [16-27] -export function valueText(value: unknown): string { +export function valueText(value: unknown, seen: WeakSet<object> = new WeakSet()): string { if (typeof value === 'string') { return value; } if (Array.isArray(value)) { - return value.map(valueText).join('\n'); + if (seen.has(value)) return ''; + seen.add(value); + return value.map((v) => valueText(v, seen)).join('\n'); } if (value && typeof value === 'object') { return safeStringify(value); } return value == null ? '' : String(value); } Suggestion importance[1-10]: 2 __ Why: Explain plan responses are JSON-serialized from the server, so they cannot contain cyclic references. The cycle protection is unnecessary defensive coding.	Low

[github-actions]

Persistent review updated to latest commit 54b3dd8