feature(ci): validate PPL linter compatibility across grammar surfaces

[Hanyu-W]

Description

Adds [Linter] PPL grammar compatibility, which validates the SQL candidate
runtime grammar with the paired OpenSearch Dashboards production headless
linter.

• Pairs SQL main or X.Y with the same OSD branch and records both SHAs.
• Exports the grammar directly from :ppl; no cluster or backend execution.
• Runs 24 trigger/control cases for the 12 default-enabled catalog rules from
  OSD #12526.
• Enforces coveredRuleIds == catalogRuleIds - excludedRuleIds. The four
  default-off headless rules and two explain-backed rules are excluded with
  reasons; missing or stale classifications fail.
• Uses exact OSD diagnostic counts. Error-recovered parse trees remain valid.

command-suggestion is the thirteenth default-enabled check, but it is a
syntax-channel feature rather than a catalog detector and is out of scope.

Supports #5405.

Testing

• node --test scripts/ppl-lint/__tests__/*.test.mjs (37 passing)
• ./gradlew :ppl:test --tests org.opensearch.sql.ppl.autocomplete.PPLGrammarBundleExporterTest
• actionlint .github/workflows/ppl-lint-grammar-compatibility.yml
• git diff --check

Check List

• New functionality includes testing and documentation.
• No new PPL command or public API is introduced.
• Commits are signed per the DCO.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 38144b1.

Path	Line	Severity	Description
.github/workflows/ppl-lint-grammar-compatibility.yml	136	medium	workflow_dispatch accepts user-supplied osd_repo and osd_ref that are used to check out and bootstrap an arbitrary GitHub repository via 'yarn osd bootstrap --prefer-offline', which executes postinstall scripts from attacker-controlled code. While restricted to workflow_dispatch and documented as non-required, this is a potential code-execution vector for anyone with dispatch access.
.github/workflows/ppl-lint-grammar-compatibility.yml	167	medium	Yarn is installed globally via 'npm install --global yarn@$yarn_version' where the version string is extracted from the checked-out OSD repository's package.json. With a workflow_dispatch osd_repo override pointing to a malicious fork, an attacker controls the installed yarn version.
scripts/ppl-lint-rule-validation.sh	349	low	OSD_REPO_URL environment variable fully overrides the git clone target URL before any repository validation runs, allowing the managed OSD checkout to be redirected to an arbitrary host independently of the --osd-repository argument that feeds the security checks.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 2 | 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.

[RyanL1997]

This PR needs a reconstruction:

• first we need to clearify the scope of this testing: it should be catching the grammar changes that impacts the rules we are having for linter in OSD
• the CI workflows should handle the version dynamically - e.g. at the future versions this change will be in the next release version 3.9 and when that happen, we should automatically picking up both SQL version and OSD version from that branch instead of main
• No need to implement more ITs for the existing grammar this is out of the scope of this change. It should be ONLY focusing on catching the linter related grammar change by the OSD validator, not the SQL plugin itself.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b360c80)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The temporary file cleanup in the finally block may fail silently if the file was successfully moved. After a successful atomic move, the temporary file no longer exists, so deleteIfExists will return false but not throw. However, if the move fails and the finally block attempts deletion, any IOException during deletion is swallowed. This could leave orphaned temporary files on disk when deletion fails after a failed write or move. } finally { if (!moved) { Files.deleteIfExists(temporary); } Possible Issue The script uses git clone --filter=blob:none --no-checkout --depth 1 to create a shallow clone, then git fetch --depth 1 origin "$OSD_CHECKOUT_REF" to fetch a specific ref. If OSD_CHECKOUT_REF is a branch name that has moved since the initial clone, the fetch may fail or retrieve an unexpected commit because shallow clones with depth 1 do not guarantee ref resolution consistency across multiple fetch operations. This can cause the script to check out a stale or incorrect OSD revision. git clone --filter=blob:none --no-checkout --depth 1 "$OSD_REPO_URL" "$OSD_ROOT" else git -C "$OSD_ROOT" rev-parse --is-inside-work-tree >/dev/null 2>&1 || die "managed OSD path is not a Git checkout: $OSD_ROOT" if git -C "$OSD_ROOT" remote get-url origin >/dev/null 2>&1; then git -C "$OSD_ROOT" remote set-url origin "$OSD_REPO_URL" else git -C "$OSD_ROOT" remote add origin "$OSD_REPO_URL" fi fi log "Resolving managed OSD checkout at $OSD_REPOSITORY_INPUT@$OSD_CHECKOUT_REF" git -C "$OSD_ROOT" fetch --depth 1 origin "$OSD_CHECKOUT_REF" git -C "$OSD_ROOT" checkout --detach FETCH_HEAD Possible Issue The resolveModule function iterates through multiple candidate paths and catches MODULE_NOT_FOUND errors to continue searching. However, if a candidate path exists but has a different error (e.g., syntax error in the module file itself), the function will fail immediately with a structural error instead of trying remaining candidates. This means a corrupted or malformed file at an earlier candidate path can prevent discovery of a valid module at a later candidate path. function resolveModule(requireFromOsd, osdRoot, name) { for (const candidate of moduleCandidates(osdRoot, name)) { try { return requireFromOsd.resolve(candidate); } catch (error) { if (error?.code !== 'MODULE_NOT_FOUND') { fail(`Could not resolve OSD module ${name}: ${error.message}`); } } } return undefined; }

[github-actions]

Persistent review updated to latest commit 9b93621

[github-actions]

Persistent review updated to latest commit 38144b1

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b360c80

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Enforce required OSD SHA field The validation only checks osd.sha mismatch when it exists in the target file, but doesn't enforce that osd.sha must be present. This could allow targets without OSD SHA to pass validation, potentially causing inconsistent provenance tracking. scripts/ppl-lint/validate-osd-grammar.mjs [80-94] function loadTarget(file, osdSha) { const raw = object(readJson(file, 'target'), 'target'); const sql = object(raw.sql, 'target.sql'); const osd = object(raw.osd, 'target.osd'); ... - if (osd.sha && osd.sha !== osdSha) fail('target.osd.sha does not match --osd-sha.'); + const targetOsdSha = string(osd.sha, 'target.osd.sha'); + if (targetOsdSha !== osdSha) fail('target.osd.sha does not match --osd-sha.'); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that osd.sha is conditionally validated only when present, which could allow missing SHA values to pass. Enforcing this field as required improves provenance tracking consistency and prevents potential validation gaps.	Medium
	Strengthen version suffix validation The version validation logic has a subtle flaw: it accepts versions like 3.8.0- or 3.8.0+ (with trailing delimiter but no suffix). The second condition should verify that when a delimiter exists, it's followed by at least one character. scripts/ppl-lint-rule-validation.sh [134-144] normalize_version() { local raw="$1" local normalized="${raw%%[-+]*}" if [[ ! "$normalized" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then die "invalid product version: $raw" fi - if [[ "$raw" != "$normalized" && "$raw" != "$normalized"-* && "$raw" != "$normalized"+* ]]; then - die "invalid product version: $raw" + if [[ "$raw" != "$normalized" ]]; then + if [[ ! "$raw" =~ ^[0-9]+\.[0-9]+\.[0-9]+[-+][0-9A-Za-z] ]]; then + die "invalid product version: $raw" + fi fi printf '%s\n' "$normalized" } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a valid edge case where versions like 3.8.0- would pass validation. The improved regex pattern correctly enforces that delimiters must be followed by at least one alphanumeric character, improving input validation robustness.	Low
	Handle temporary file cleanup failures The temporary file cleanup in the finally block may fail silently if Files.deleteIfExists throws an exception. Consider logging or handling potential cleanup failures to avoid leaving orphaned temporary files when the filesystem is in an error state. ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleExporter.java [45-60] Path temporary = Files.createTempFile(parent, "." + output.getFileName().toString() + ".", ".tmp"); boolean moved = false; try { Files.writeString( temporary, serializeBundle(bundle), StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING); moveIntoPlace(temporary, output); moved = true; } finally { if (!moved) { - Files.deleteIfExists(temporary); + try { + Files.deleteIfExists(temporary); + } catch (IOException cleanupError) { + // Log or handle cleanup failure + } } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that Files.deleteIfExists can throw IOException, but the impact is low since temporary files in system temp directories are typically cleaned up automatically. The improvement is marginal for this use case.	Low

---

Previous suggestions

Suggestions up to commit b360c80

Category	Suggestion	Impact
General	Handle cleanup exceptions in finally block The temporary file cleanup in the finally block may fail silently if Files.deleteIfExists throws an exception. Consider logging or handling potential IOException during cleanup to ensure visibility of cleanup failures, especially in CI environments where orphaned temporary files could accumulate. ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleExporter.java [45-60] Path temporary = Files.createTempFile(parent, "." + output.getFileName().toString() + ".", ".tmp"); boolean moved = false; try { Files.writeString( temporary, serializeBundle(bundle), StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING); moveIntoPlace(temporary, output); moved = true; } finally { if (!moved) { - Files.deleteIfExists(temporary); + try { + Files.deleteIfExists(temporary); + } catch (IOException e) { + System.err.println("Failed to delete temporary file: " + temporary + " - " + e.getMessage()); + } } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that Files.deleteIfExists can throw IOException, which would be silently swallowed in the finally block. However, the impact is limited since this is cleanup code and the primary operation has already failed. Logging would improve observability but isn't critical for correctness.	Low
	Fix version validation for semantic versioning The version normalization logic may incorrectly reject valid semantic versions with multiple pre-release or build metadata segments (e.g., 3.8.0-alpha.1+build.123). The pattern check "$raw" != "$normalized"-* will fail for versions like 3.8.0-rc.1 because the wildcard doesn't match the dot. Consider using a more robust regex pattern to validate the entire version string. scripts/ppl-lint-rule-validation.sh [134-144] normalize_version() { local raw="$1" local normalized="${raw%%[-+]*}" if [[ ! "$normalized" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then die "invalid product version: $raw" fi - if [[ "$raw" != "$normalized" && "$raw" != "$normalized"-* && "$raw" != "$normalized"+* ]]; then + if [[ "$raw" != "$normalized" && ! "$raw" =~ ^[0-9]+\.[0-9]+\.[0-9]+([-+][0-9A-Za-z.-]+)?$ ]]; then die "invalid product version: $raw" fi printf '%s\n' "$normalized" } Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the existing validation logic. The current code correctly validates versions with pre-release/build metadata using pattern matching with "$normalized"-* and "$normalized"+*. The proposed regex is redundant since the existing checks already handle these cases properly. The example 3.8.0-rc.1 would pass the current validation.	Low

Suggestions up to commit 38144b1

Category	Suggestion	Impact
Security	Add depth limit to tree traversal The hasErrorNode function traverses parse trees without depth limits, which could cause stack overflow or excessive memory consumption on deeply nested or malformed trees. Add a maximum depth check to prevent resource exhaustion from pathological inputs. scripts/ppl-lint/validate-osd-grammar.mjs [281-290] -function hasErrorNode(tree) { - const pending = [tree]; +function hasErrorNode(tree, maxDepth = 1000) { + const pending = [[tree, 0]]; while (pending.length) { - const node = pending.pop(); + const [node, depth] = pending.pop(); + if (depth > maxDepth) throw new Error('parse tree exceeds maximum depth'); if (!node || typeof node !== 'object') continue; if (node.constructor?.name === 'ErrorNode') return true; - if (Array.isArray(node.children)) pending.push(...node.children); + if (Array.isArray(node.children)) { + pending.push(...node.children.map(child => [child, depth + 1])); + } } return false; } Suggestion importance[1-10]: 6 __ Why: Adding a depth limit to prevent stack overflow on pathological parse trees is a reasonable defensive measure. However, the function uses iteration (not recursion), so stack overflow is not a concern. The memory consumption concern is valid but unlikely with typical grammar trees.	Low
General	Handle workflow cancellation signals The workflow step starts a Gradle process in the background but only traps EXIT for cleanup. If the workflow is cancelled or times out, the trap may not execute, leaving orphaned Gradle processes. Add traps for INT and TERM signals to ensure cleanup on cancellation. .github/workflows/ppl-lint-grammar-compatibility.yml [238-249] - name: Capture candidate runtime grammar if: ${{ steps.capability.outputs.available == 'true' }} timeout-minutes: 25 run: | set -euo pipefail ./gradlew :opensearch-sql-plugin:run --no-daemon > ppl-grammar-cluster.log 2>&1 & gradle_pid=$! cleanup() { kill "$gradle_pid" 2>/dev/null || true wait "$gradle_pid" 2>/dev/null || true } - trap cleanup EXIT + trap cleanup EXIT INT TERM Suggestion importance[1-10]: 5 __ Why: Adding INT and TERM traps improves cleanup reliability on workflow cancellation. However, GitHub Actions typically sends TERM which the existing EXIT trap should handle. The improvement is marginal but valid for defensive scripting.	Low
	Increase cluster health check timeout The pre-flight check for port 9200 availability uses a 2-second timeout, which may be insufficient for slow networks or systems under load. This could cause false positives where a legitimate cluster is starting but hasn't responded yet. Consider increasing the timeout or adding retry logic to ensure reliable detection. scripts/ppl-lint-rule-validation.sh [403-405] -if curl --fail --silent --max-time 2 "http://127.0.0.1:9200/_cluster/health" >/dev/null 2>&1; then +if curl --fail --silent --connect-timeout 2 --max-time 5 "http://127.0.0.1:9200/_cluster/health" >/dev/null 2>&1; then die "port 9200 already serves an OpenSearch cluster; refusing to capture from an unknown process" fi Suggestion importance[1-10]: 4 __ Why: The suggestion to increase --max-time from 2 to 5 seconds is reasonable for reliability, but the existing code already uses --max-time 2 which is adequate for a pre-flight check. The impact is minor since this is just a safety check before starting the cluster.	Low
	Fix timeout calculation baseline The cluster readiness loop uses SECONDS for timeout tracking, which is a Bash built-in that counts seconds since shell start. If the script runs in a long-lived shell or after other operations, SECONDS may already be large, causing incorrect timeout calculations. Use a dedicated timestamp variable instead. scripts/ppl-lint-rule-validation.sh [412-426] -deadline=$((SECONDS + STARTUP_TIMEOUT)) +start_time=$SECONDS +deadline=$((start_time + STARTUP_TIMEOUT)) while true; do if ! kill -0 "$GRADLE_PID" 2>/dev/null; then wait "$GRADLE_PID" 2>/dev/null || gradle_status=$? GRADLE_PID="" die "Gradle run exited before cluster readiness (status ${gradle_status:-0}); see $CLUSTER_LOG" fi if curl --fail --silent --show-error --connect-timeout 2 --max-time 5 \ "http://127.0.0.1:9200/_cluster/health" >/dev/null 2>&1; then break fi (( SECONDS < deadline )) || die "cluster did not become ready within ${STARTUP_TIMEOUT}s; see $CLUSTER_LOG" sleep 2 done Suggestion importance[1-10]: 2 __ Why: The concern about SECONDS being large in long-lived shells is theoretically valid, but the script uses set -euo pipefail and runs as a standalone script (not sourced), so SECONDS starts from 0. The improved_code still references SECONDS in the comparison, making the change ineffective.	Low

[RyanL1997]

This asserts a contract OSD deliberately does not hold. buildRuntimeTree in headless_ppl_lint.ts documents the opposite: it keeps error-recovered trees on purpose because "the lint rules are written to walk it best-effort", citing eval x = <field> + 1 as a query the engine accepts but the runtime ATN cannot fully parse.

So a grammar change where the parser recovers but every lint rule still behaves correctly fails here, carrying no information about linter breakage. The diagnostic-count comparison below is the real signal — I would drop this gate or make it non-fatal.

Also node.constructor?.name === 'ErrorNode' is name-based type detection, fragile across antlr4ng versions and under bundling.

[Hanyu-W]

Removed the parse-cleanliness gate and the ErrorNode name check. The adapter now relies only on lintQueryWithBundle() diagnostic behavior, so an error-recovered tree passes when the production OSD rules still behave correctly.

[RyanL1997]

The cluster is not needed. PPLGrammarBundleBuilder.build() is a pure static function over the generated ANTLR classes — no Settings, no NodeClient, no cluster state, and getBundle() is a static singleton. The bundle is a build artifact of the .g4 files.

Booting the plugin (25-minute step timeout, 120x5s readiness poll, port-9200 guard, PID traps, cluster log) to curl static JSON is the biggest cost and flake surface in the PR. A small JavaExec in :ppl that writes ppl-grammar-bundle.json replaces all of it, deletes ~150 lines of bash across this file and the wrapper script, and takes the job from ~45 min to a few. It also moots the analyzer findings about PID reuse and trap handling.

[Hanyu-W]

Replaced the development cluster and REST capture with :ppl:exportPplGrammarBundle, backed directly by PPLGrammarBundleBuilder.getBundle(). The workflow and local wrapper no longer use port 9200, polling, curl, PID traps, or cluster logs.

[RyanL1997]

Weakest of three disagreeing capability probes — this is the false-green path.

This accepts only .ts and .js. The wrapper (ppl-lint-rule-validation.sh:386-391) also accepts bare and .mjs. The adapter (validate-osd-grammar.mjs:127-130) accepts bare via existsSync, which additionally matches a directory.

So if OSD refactors headless_ppl_lint.ts into headless_ppl_lint/index.ts, this returns false, the adapter writes skipReason: osd-headless-grammar-api-unavailable, and the job exits 0 — green, indistinguishable from the legitimate old-branch skip, while the API is present and working.

Suggest deleting this step and letting the adapter be the single source of truth; it already handles the absent-module case at :437-440 and returns 0 with the skip report.

[Hanyu-W]

Deleted the workflow-level file probe and its conditional skip path. Every run now delegates capability and validation to the adapter, so the workflow cannot independently produce a false-green absence decision.

[RyanL1997]

Third variant of the capability probe: bare via existsSync (matches a directory), .js, .ts — no .mjs.

This is the most permissive of the three and, being colocated with the code that actually loads the module, the only one that can be correct by construction. Make it the only one — see the comment on ppl-lint-grammar-compatibility.yml:211.

[Hanyu-W]

Consolidated resolution and loading in the adapter. Only a true failure to resolve the headless entry point becomes a legacy skip; import failures, transitive dependency failures, and invalid exports now fail structurally.

[RyanL1997]

Second variant of the capability probe: bare, .ts, .js, .mjs — but all via -f, so unlike the adapter it never matches a directory.

Three probes, three different answers for the same OSD checkout. A .mjs module is available here and unavailable to the workflow; a directory module is available to the adapter and unavailable to both of these. Collapse to the adapter's check.

[Hanyu-W]

Removed the shell capability probe as well. The local wrapper now follows the same path as CI and always delegates the availability decision to the adapter's resolver/loader.

[RyanL1997]

No coverage floor. This requires at least one case, and the loop below requires trigger+control per selected rule — but nothing requires a minimum number of rules.

Truncate grammar-cases.json to a single rule and the report reads rules: {selected: 1, passed: 1} and goes green. Combined with the missing reverse-coverage check against the OSD catalog, deleting a failing case is a valid way to make CI pass.

Asserting the covered set against catalogIds minus an explicit allow-list fixes both at once.

[Hanyu-W]

Fixed in a7e3b2f. The adapter now enforces coveredRuleIds == catalogIds - excludedRuleIds, with a required reason for every exclusion. Deleting a required rule’s cases, adding an unclassified catalog rule, leaving a stale exclusion, overlapping covered/excluded sets, or omitting a trigger/control pair now fails structurally. The required set is the default-enabled catalog rules from OSD #12526.

[Hanyu-W]

@RyanL1997 Thank you for the feedback. I have pushed changes to address the review, please re-review when you get a chance. Github Actions are currently down preventing me from testing the ci, will update this comment with CI testing results once Github Actions is fixed.

EDIT: Github Actions is up, this is what the CI looks like now:

[github-actions]

Persistent review updated to latest commit b360c80

[github-actions]

Persistent review updated to latest commit b360c80