From a5fd530e332763b6baf0c3a057afe564b67ffd38 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Thu, 2 Apr 2026 11:08:58 +0800 Subject: [PATCH 01/10] Add PPL bugfix harness and /ppl-bugfix slash command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ppl-bugfix-harness.md: systematic bugfix SOP distilled from 15+ historical commits, covering triage, TDD-style fix paths (A-E by bug layer), test templates (UT/IT/YAML), verification, PR creation, decision log, and post-PR follow-up - .claude/commands/ppl-bugfix.md: slash command that auto-resolves issue↔PR, dispatches subagent with worktree isolation and scoped permissions, handles both initial fix and follow-up flows - CLAUDE.md: reference to /ppl-bugfix as the entry point for PPL bugs - .gitignore: allow .claude/commands/ and settings.json to be tracked Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 111 ++++++++++ .gitignore | 5 +- CLAUDE.md | 4 + ppl-bugfix-harness.md | 383 +++++++++++++++++++++++++++++++++ 4 files changed, 502 insertions(+), 1 deletion(-) create mode 100644 .claude/commands/ppl-bugfix.md create mode 100644 ppl-bugfix-harness.md diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md new file mode 100644 index 00000000000..62e8b52add9 --- /dev/null +++ b/.claude/commands/ppl-bugfix.md @@ -0,0 +1,111 @@ +--- +allowed-tools: Agent, Read, Bash(gh:*), Bash(git:*) +description: Run the PPL bugfix harness for a GitHub issue or follow up on an existing PR +--- + +Fix a PPL bug or follow up on an existing PR using the harness in `ppl-bugfix-harness.md`. + +## Input + +- `/ppl-bugfix #1234` or `/ppl-bugfix 1234` — issue number +- `/ppl-bugfix PR#5678` or `/ppl-bugfix pr 5678` — PR number +- `/ppl-bugfix https://github.com/opensearch-project/sql/issues/1234` — URL + +If no argument given, ask for an issue or PR number. + +## Step 1: Resolve Issue ↔ PR + +```bash +# Issue → PR +gh pr list --search "Resolves #" --json number,url,state --limit 5 + +# PR → Issue +gh pr view --json body | jq -r '.body' | grep -oP 'Resolves #\K[0-9]+' +``` + +| State | Action | +|-------|--------| +| Issue exists, no PR | **Initial Fix** (Step 2A) | +| Issue exists, open PR found | **Follow-up** (Step 2B) | +| PR provided directly | **Follow-up** (Step 2B) | + +## Step 2A: Initial Fix + +1. `gh issue view ` +2. `Read ppl-bugfix-harness.md` +3. Dispatch subagent: + +``` +Agent( + mode: "acceptEdits", + isolation: "worktree", + name: "bugfix-", + description: "PPL bugfix #", + prompt: " + + Follow Phase 0 through Phase 3 in order. + Phase 0.3 defines TDD execution flow. Do NOT skip any phase. + Post the Decision Log (Phase 3.4) before completing." +) +``` + +The `isolation: "worktree"` creates a temporary git worktree branched from the current HEAD. The agent works in a clean, isolated copy of the repo — no interference with the user's working directory. If the agent makes changes, the worktree path and branch are returned in the result. + +4. Report back: classification, fix summary, PR URL, **worktree branch name**, items needing human attention. + +## Step 2B: Follow-up + +1. Load PR state: +```bash +gh pr view --json title,body,state,reviews,comments,statusCheckRollup,mergeable +gh pr checks +``` + +2. Categorize: + +| Signal | Type | +|--------|------| +| `statusCheckRollup` has failures | CI failure | +| `reviews` has CHANGES_REQUESTED | Review feedback | +| `mergeable` is CONFLICTING | Merge conflict | +| All pass + approved | Ready — run `gh pr ready` | + +3. `Read ppl-bugfix-harness.md` +4. Dispatch follow-up subagent: + +``` +Agent( + mode: "acceptEdits", + isolation: "worktree", + name: "bugfix--followup", + description: "PPL bugfix # followup", + prompt: " + PR: (), Issue: # + Follow-up type: + Read the Decision Log PR comment FIRST before making any changes. + Checkout the PR branch before starting: git checkout " +) +``` + +5. Report back: what was addressed, current PR state, whether another round is needed. + +## Subagent Lifecycle + +Subagents are task-scoped. They complete and release context — they cannot poll for events. + +``` +Agent A (Phase 0-3) → creates PR → completes + (CI runs, reviewers comment, conflicts arise) +Agent B (Phase 3.5) → handles feedback → completes + (repeat as needed) +Agent N (Phase 3.5) → gh pr ready → done +``` + +Context is preserved across agents via: +- **Decision Log** (PR comment) — single source of truth for rejected alternatives, pitfalls, design rationale +- **GitHub state** (PR diff, review comments, CI logs) — reconstructed by each follow-up agent + +## Rules + +- Always inline harness content in the subagent prompt — subagents cannot read skill files +- If bug is not reproducible (Phase 0.1), stop and report — do not proceed +- Issue ↔ PR auto-resolution means the user never needs to track PR numbers manually diff --git a/.gitignore b/.gitignore index 329348a7c12..34b41594bf0 100644 --- a/.gitignore +++ b/.gitignore @@ -53,6 +53,9 @@ http-client.env.json .factorypath # Coding agent files (could be symlinks) -.claude +.claude/* +!.claude/settings.json +!.claude/commands/ +.claude/settings.local.json .clinerules memory-bank \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index 4629dcf5fe7..3e7b58e4bdb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,10 @@ plugin (OpenSearch plugin entry point, Guice DI wiring) - **PhysicalPlan** implements `Iterator` for streaming execution - **Guice** dependency injection in `OpenSearchPluginModule` +## Fixing PPL Bugs + +Use the `/ppl-bugfix #` slash command to fix PPL bugs. It dispatches a subagent with scoped permissions that follows all phases in `ppl-bugfix-harness.md` — from Phase 0 (Triage & Classification) through Phase 4 (Retrospective & Improvement). The full harness content is inlined into the subagent prompt to guarantee compliance. If a user asks to fix a PPL bug without using the slash command, remind them to use `/ppl-bugfix` or invoke it on their behalf. + ## Adding New PPL Commands Follow the checklist in `docs/dev/ppl-commands.md`: diff --git a/ppl-bugfix-harness.md b/ppl-bugfix-harness.md new file mode 100644 index 00000000000..17fd2682d3b --- /dev/null +++ b/ppl-bugfix-harness.md @@ -0,0 +1,383 @@ +# PPL Bugfix Harness + +> Systematic bugfix workflow distilled from 15+ historical commits. Invoke via `/ppl-bugfix #`. + +--- + +## Phase 0: Triage & Classification + +### 0.1 Reproduce the Bug + +```bash +# Minimal PPL query +POST /_plugins/_ppl/_query +{ "query": "" } + +# Or via integration test +./gradlew :integ-test:integTest -Dtests.class="*" -Dtests.method="" +``` + +**If not reproducible on latest `main`**: + +| Finding | Action | +|---------|--------| +| Already fixed by another commit | Comment with fixing commit hash, close as duplicate | +| Only on older version | Comment with version where it's fixed, close as resolved | +| Intermittent / environment-specific | Label `flaky` or `needs-info`, do NOT close | +| Insufficient info to reproduce | Comment asking for repro steps, label `needs-info` | + +In all cases, **stop and report back** — do not proceed to Phase 1. + +### 0.2 Classify and Route + +Identify the bug layer, then use the routing table to determine the fix path and required tests: + +| Layer | Typical Symptoms | Fix Path | Required Tests | +|-------|-----------------|----------|---------------| +| **Grammar/Parser** | `SyntaxCheckException`, unrecognized syntax | Path A | AstBuilderTest | +| **AST/Functions** | Parses OK but AST wrong, function output wrong | Path B | CalcitePPL\*Test + IT + YAML | +| **Semantic Analysis** | `SemanticCheckException`, type mismatch | Path C | Dedicated UT + CalcitePPLIT + YAML | +| **Type System** | Field type lost, implicit conversion errors | Path C | Dedicated UT + CalcitePPLIT + YAML | +| **Optimizer** | Plan bloat, predicate pushdown failure | Path D | CalcitePPL\*Test + CalcitePPLIT + NoPushdownIT + YAML | +| **Execution** | Wrong results, OOM, memory leaks | Path E | IT + YAML | +| **DI / Resource** | NPE, extensions not loaded, long-running OOM | Path E | IT + YAML | + +Record before proceeding: + +``` +Bug Layer: +Fix Path: +Issue: #XXXX +``` + +### 0.3 Execution Flow + +Phases interleave TDD-style — write a failing test first, then fix, then complete test coverage: + +``` +Phase 0: Triage → Classify → Route + → Phase 2 (partial): Write FAILING test reproducing the bug + → Phase 1: Implement fix via routed Path + → Phase 2 (remaining): Happy path, edge cases, YAML REST test + → Phase 3: Verify → Commit → PR → Decision Log +``` + +--- + +## Phase 1: Fix Implementation + +### Path A — Grammar / Parser + +1. **Update grammar files** (must stay in sync): + - `language-grammar/src/main/antlr4/OpenSearchPPLParser.g4` (primary) + - `ppl/src/main/antlr/OpenSearchPPLParser.g4` + - `async-query-core/src/main/antlr/OpenSearchPPLParser.g4` (if applicable) +2. **Regenerate**: `./gradlew generateGrammarSource` +3. **Update AstBuilder**: `ppl/.../parser/AstBuilder.java` — modify `visit*()` to match new rule +4. **Test**: `AstBuilderTest` using `assertEqual(pplQuery, expectedAstNode)` pattern + +> Ref: `734394d` — fixed `renameClasue` → `renameClause` across 3 grammars + AstBuilder + +### Path B — AST / Function Implementation + +1. **Locate**: AST nodes in `core/.../ast/tree/`, functions in `core/.../expression/function/` or `PPLBuiltinOperators` +2. **Fix**: Watch Visitor pattern — changes may need syncing to `AbstractNodeVisitor`, `Analyzer`, `CalciteRelNodeVisitor`, `PPLQueryDataAnonymizer` +3. **Test**: `verifyLogical()`, `verifyPPLToSparkSQL()`, `verifyResult()` + +> Ref: `26674f9` — rex nested capture group fix, ordinal index → named group extraction + +### Path C — Type System / Semantic Analysis + +1. **Locate**: `OpenSearchTypeFactory.java` (Calcite type factory), `Analyzer.java` / `ExpressionAnalyzer.java` +2. **Fix**: Preserve nullable semantics when overriding Calcite methods; protect UDT from `leastRestrictive()` downgrade +3. **Test (critical)**: Cover type preservation, nullable propagation, fallback to parent, mixed types — every edge case + +> Ref: `ada2e34` — UNION lost timestamp UDT, fixed `leastRestrictive()`, added 8 UTs + 4 ITs + +### Path D — Optimizer / Predicate Pushdown + +1. **Locate**: `PredicateAnalyzer.java`, `LogicalPlanOptimizer`, `QueryService.java` +2. **Fix**: Watch `nullAs` semantics (TRUE/FALSE/UNKNOWN); for plan bloat consider Calcite rules like `FilterMergeRule` +3. **Verify**: `EXPLAIN` output comparison + integration test result correctness + +> Ref: `b4df010` — `isnotnull()` not pushed down with multiple `!=`; `e045d15` — multi-filter OOM, inserted `FilterMergeRule` + +### Path E — Execution / Resource Management + +1. **Locate**: DQE operators in `dqe/`, `OpenSearchExecutionEngine.java`, `SQLPlugin.java`, `OpenSearchPluginModule.java` +2. **Common patterns**: + + | Problem | Fix | Example | + |---------|-----|---------| + | Cache key collision | `IndexReader.CacheHelper.getKey()` | `97d5d26` | + | Memory leak (no eviction) | Close listener + upper bound | `97d5d26` | + | Unbounded growth | `MAX_CAPACITY` check, throw with user guidance | `f024b4f` | + | Non-singleton repeated registration | `@Singleton`; `put` instead of `compute/append` | `90393bf` | + | DI not injected | Holder class → Guice → constructor injection | `f6be830` | + +> Ref: `f024b4f` — `LongOpenHashSet` capacity 1024→8 (8KB→64B per group), 8M cap on 5 HashMap variants + +--- + +## Phase 2: Writing Tests + +### 2.1 Test Templates + +**Unit test** (extend `CalcitePPLAbstractTest`): +```java +public class CalcitePPLYourFixTest extends CalcitePPLAbstractTest { + public CalcitePPLYourFixTest() { + super(CalciteAssert.SchemaSpec.SCOTT_WITH_TEMPORAL); + } + + @Before + public void init() { + doReturn(true).when(settings) + .getSettingValue(Settings.Key.CALCITE_ENGINE_ENABLED); + } + + @Test + public void testBugScenario() { + verifyLogical("source=EMP | where SAL > 1000", + "LogicalFilter(condition=[>($5, 1000)])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"); + } +} +``` + +**Integration test** (extend `CalcitePPLIT`): +```java +public class CalcitePPLYourFixIT extends CalcitePPLIT { + @Override + public void init() throws IOException { + super.init(); + enableCalcite(); + } + + @Test + public void testBugFixEndToEnd() throws IOException { + JSONObject result = executeQuery("source= | "); + verifySchema(result, schema("field", "alias", "type")); + verifyDataRows(result, rows("expected_value_1"), rows("expected_value_2")); + } +} +``` + +**YAML REST test** — place at `integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/.yml`, auto-discovered by `RestHandlerClientYamlTestSuiteIT`: +```yaml +setup: + - do: + indices.create: + index: test_issue_ + body: + settings: { number_of_shards: 1, number_of_replicas: 0 } + mappings: { properties: { : { type: } } } + - do: + query.settings: + body: { transient: { plugins.calcite.enabled: true } } +--- +teardown: + - do: + query.settings: + body: { transient: { plugins.calcite.enabled: false } } +--- +"": + - skip: { features: [headers, allowed_warnings] } + - do: + bulk: { index: test_issue_, refresh: true, body: ['{"index": {}}', '{"": ""}'] } + - do: + headers: { Content-Type: 'application/json' } + ppl: { body: { query: "source=test_issue_ | " } } + - match: { total: } + - length: { datarows: } +``` + +### 2.2 Test Checklist + +- [ ] Failing test reproducing the original bug (write FIRST, before fixing) +- [ ] Happy path after fix +- [ ] Edge cases (null, empty, extreme volumes) +- [ ] YAML REST test (named by issue number) +- [ ] Optimizer bugs: both pushdown enabled and disabled +- [ ] Type system bugs: nullable / non-nullable combinations +- [ ] New AST nodes: update `PPLQueryDataAnonymizerTest` + +--- + +## Phase 3: Verification & Submission + +### 3.1 Local Verification + +```bash +./gradlew spotlessApply # 1. Format +./gradlew ::test --tests "" # 2. Affected module UT +./gradlew test # 3. Full UT regression +./gradlew :integ-test:integTest -Dtests.class="*" # 4. New IT +./gradlew :integ-test:integTest # 5. Full IT regression +./gradlew :integ-test:yamlRestTest # 6. YAML REST tests +# If grammar modified: +./gradlew generateGrammarSource && ./gradlew :ppl:test # 7. Parser tests +``` + +### 3.2 Commit & Push + +The subagent runs in an isolated **git worktree** branched from HEAD. All changes are on a dedicated branch that does not affect the user's working directory. + +```bash +# Commit with DCO sign-off +git add +git commit -s -m "[BugFix] Fix (#)" + +# Sync with main (merge, not rebase — PRs use squash-merge) +git fetch origin && git merge origin/main +# Re-run tests if merge brought changes + +# Push to personal fork (NOT origin if it points to upstream) +git remote -v # confirm your fork remote +git push -u +``` + +### 3.3 Create Draft PR + +```bash +gh pr create --draft --title "[BugFix] Fix (#)" --body "$(cat <<'EOF' +### Description + + +### Related Issues +Resolves # + +### Check List +- [x] New functionality includes testing +- [x] Javadoc added for new public API +- [x] Commits signed per DCO (`-s`) +- [x] `spotlessCheck` passed +- [x] Unit tests passed +- [x] Integration tests passed +- [ ] Grammar changes: all three `.g4` files synced (if applicable) +EOF +)" +``` + +### 3.4 Persist Decision Context + +Before the subagent completes, persist the *why* behind key decisions — the PR diff only shows the *what*. The **PR comment is the single source of truth** — it survives across sessions, GA runs, and is visible to both human reviewers and follow-up agents. + +```bash +gh pr comment --body "$(cat <<'EOF' +## Decision Log +**Root Cause**: +**Approach**: +**Alternatives Rejected**: +**Pitfalls**: +**Things to Watch**: +EOF +)" +``` + +--- + +## Phase 3.5: Post-PR Follow-up + +Runs as a **separate subagent** invocation — the original agent has completed. Invoke via `/ppl-bugfix #` or `/ppl-bugfix PR#` (auto-detects follow-up mode when a PR exists). + +### 3.5.1 Reconstruct Context + +The follow-up agent runs in a fresh worktree. First checkout the PR branch, then load state: + +```bash +# Checkout the PR branch in this worktree +gh pr checkout + +# Load PR state and Decision Log +gh pr view --json title,body,state,reviews,comments,statusCheckRollup,mergeable +gh pr checks +# Read the Decision Log comment FIRST — contains rejected alternatives and pitfalls +gh api repos///pulls//comments +``` + +### 3.5.2 Handle Review Feedback + +For each comment, **cross-check against the Decision Log first**: + +| Type | Action | +|------|--------| +| Code change | If already rejected in Decision Log, reply with reasoning. Otherwise make the change, new commit, push | +| Question | Reply with explanation — Decision Log often has the answer | +| Nit | Fix if trivial | +| Disagreement | Reply with Decision Log reasoning; if reviewer insists, escalate to user | + +```bash +git add && git commit -s -m "Address review feedback: " +git push +``` + +### 3.5.3 Handle CI Failures + +```bash +gh pr checks # Identify failures +gh run view --log-failed # Read logs +# Test failure → fix locally, push new commit +# Spotless → ./gradlew spotlessApply, push +# Flaky → gh run rerun --failed +``` + +### 3.5.4 Handle Merge Conflicts + +```bash +git fetch origin && git merge origin/main # Resolve conflicts +./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest # Re-verify +git commit -s -m "Resolve merge conflicts with main" +git push +``` + +### 3.5.5 Mark Ready + +```bash +gh pr ready +``` + +--- + +## Phase 4: Retrospective + +After each bugfix, check if the harness itself needs updating: + +- **Template wrong** (API name, assertion field)? → Fix in Phase 2 templates +- **New bug pattern** not covered? → Add fix path in Phase 1 or symptom in Quick Reference +- **Verification gap** caused rework? → Add step to 3.1 or 2.2 checklist +- **Representative fix**? → Add row to Case Index below + +--- + +## Quick Reference: Symptom → Fix Path + +``` +SyntaxCheckException / unrecognized syntax → Path A: Grammar/Parser +SemanticCheckException / type mismatch → Path C: Type System / Analysis +Field type wrong (timestamp→string) → Path C: check leastRestrictive / coercion +EXPLAIN shows predicate not pushed down → Path D: Optimizer / Pushdown +Multi-condition query: missing/extra rows → Path D: PredicateAnalyzer nullAs handling +OOM / memory growth over time → Path E: singletons, cache eviction, bounds +NPE in Transport layer → Path E: DI / Guice injection chain +"node must be boolean/number, found XXX" → Path B: OpenSearchJsonContent parse*Value +Regex/function extraction offset → Path B: ordinal vs named references +``` + +--- + +## Appendix: Case Index + +| Commit | Bug | Layer | Tests | +|--------|-----|-------|-------| +| `ada2e34` | UNION loses UDT type | Type System | 8 UT + 4 IT | +| `26674f9` | rex capture group index shift | AST/Functions | Multiple UTs | +| `b4df010` | isnotnull not pushed down with != | Optimizer | 2 UT + IT | +| `e045d15` | Multiple filters OOM | Optimizer | 26 output updates | +| `f024b4f` | High-cardinality GROUP BY OOM | Execution | Benchmark | +| `97d5d26` | OrdinalMap cache collision + leak | Execution | — | +| `90393bf` | Non-singleton ExecutionEngine leak | Resource | — | +| `f6be830` | Transport extensions not injected | DI | — | +| `734394d` | Grammar rule typo | Grammar | — | +| `246ed0d` | Float precision flaky test | Test Infra | — | +| `d56b8fa` | Wildcard index type conflict | Value Parsing | 3 UT + 1 IT + 1 YAML | From d4b9b01ed7548f714602244cb73013b4e06ff237 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 14:21:12 +0800 Subject: [PATCH 02/10] Improve PPL bugfix command: permission modes, parallel dispatch, file reorg - Add --safe/--yolo permission mode flags (default: bypassPermissions) - Support multiple issue/PR references for parallel processing - Move harness files to .claude/harness/ directory - Add ppl-bugfix-followup.md for post-PR follow-up workflow - Add .claude/settings.json with pre-approved tool permissions - Add CLAUDE_GUIDE.md documenting all slash commands Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 91 ++++++++++--------- .claude/harness/ppl-bugfix-followup.md | 70 ++++++++++++++ .../harness/ppl-bugfix-harness.md | 77 ++-------------- .claude/settings.json | 26 ++++++ .gitignore | 1 + CLAUDE.md | 2 +- CLAUDE_GUIDE.md | 54 +++++++++++ 7 files changed, 212 insertions(+), 109 deletions(-) create mode 100644 .claude/harness/ppl-bugfix-followup.md rename ppl-bugfix-harness.md => .claude/harness/ppl-bugfix-harness.md (84%) create mode 100644 .claude/settings.json create mode 100644 CLAUDE_GUIDE.md diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md index 62e8b52add9..026ddf6a821 100644 --- a/.claude/commands/ppl-bugfix.md +++ b/.claude/commands/ppl-bugfix.md @@ -3,17 +3,44 @@ allowed-tools: Agent, Read, Bash(gh:*), Bash(git:*) description: Run the PPL bugfix harness for a GitHub issue or follow up on an existing PR --- -Fix a PPL bug or follow up on an existing PR using the harness in `ppl-bugfix-harness.md`. +Fix a PPL bug or follow up on an existing PR using the harness in `.claude/harness/ppl-bugfix-harness.md`. ## Input -- `/ppl-bugfix #1234` or `/ppl-bugfix 1234` — issue number -- `/ppl-bugfix PR#5678` or `/ppl-bugfix pr 5678` — PR number +Accepts one or more issue/PR references. Multiple references are processed in parallel (each gets its own subagent + worktree). + +- `/ppl-bugfix #1234` — single issue +- `/ppl-bugfix PR#5678` — single PR +- `/ppl-bugfix #1234 #5678 PR#9012` — multiple in parallel - `/ppl-bugfix https://github.com/opensearch-project/sql/issues/1234` — URL +Optional mode flag (append to any of the above): +- `--safe` — `acceptEdits` mode. Auto-approve file edits only, Bash commands require manual approval. (Most conservative) +- `--yolo` — `bypassPermissions` mode. Fully trusted, no prompts. Subagent runs in an isolated worktree so this is safe. (Default) + +Examples: +- `/ppl-bugfix #1234` — single issue, defaults to yolo +- `/ppl-bugfix #1234 #5678 --yolo` — two issues in parallel +- `/ppl-bugfix PR#5293 PR#5300` — two PRs in parallel +- `/ppl-bugfix #1234 PR#5678 --safe` — mix of issue and PR + If no argument given, ask for an issue or PR number. -## Step 1: Resolve Issue ↔ PR +## Step 0: Resolve Permission Mode + +Parse the mode flag from the input arguments: + +| Flag | Mode | +|------|------| +| `--safe` | `acceptEdits` | +| `--yolo` | `bypassPermissions` | +| _(no flag)_ | `bypassPermissions` (default) | + +Use the resolved mode as the `mode` parameter when dispatching the subagent in Step 2A/2B. + +## Step 1: Resolve Each Reference + +For each issue/PR reference in the input, resolve its state. Run these lookups in parallel when there are multiple references. ```bash # Issue → PR @@ -29,64 +56,44 @@ gh pr view --json body | jq -r '.body' | grep -oP 'Resolves #\K[0-9] | Issue exists, open PR found | **Follow-up** (Step 2B) | | PR provided directly | **Follow-up** (Step 2B) | -## Step 2A: Initial Fix +## Step 2: Dispatch Subagents -1. `gh issue view ` -2. `Read ppl-bugfix-harness.md` -3. Dispatch subagent: +Dispatch one subagent per reference. When there are multiple references, dispatch all subagents in a single message (parallel execution). + +### 2A: Initial Fix ``` Agent( - mode: "acceptEdits", + mode: "", isolation: "worktree", name: "bugfix-", description: "PPL bugfix #", - prompt: " + + prompt: "Read .claude/harness/ppl-bugfix-harness.md and follow it to fix GitHub issue #. Follow Phase 0 through Phase 3 in order. Phase 0.3 defines TDD execution flow. Do NOT skip any phase. Post the Decision Log (Phase 3.4) before completing." ) ``` -The `isolation: "worktree"` creates a temporary git worktree branched from the current HEAD. The agent works in a clean, isolated copy of the repo — no interference with the user's working directory. If the agent makes changes, the worktree path and branch are returned in the result. - -4. Report back: classification, fix summary, PR URL, **worktree branch name**, items needing human attention. - -## Step 2B: Follow-up - -1. Load PR state: -```bash -gh pr view --json title,body,state,reviews,comments,statusCheckRollup,mergeable -gh pr checks -``` - -2. Categorize: - -| Signal | Type | -|--------|------| -| `statusCheckRollup` has failures | CI failure | -| `reviews` has CHANGES_REQUESTED | Review feedback | -| `mergeable` is CONFLICTING | Merge conflict | -| All pass + approved | Ready — run `gh pr ready` | - -3. `Read ppl-bugfix-harness.md` -4. Dispatch follow-up subagent: +### 2B: Follow-up ``` Agent( - mode: "acceptEdits", + mode: "", isolation: "worktree", - name: "bugfix--followup", + name: "bugfix-", description: "PPL bugfix # followup", - prompt: " + prompt: "Read .claude/harness/ppl-bugfix-followup.md and follow it. PR: (), Issue: # - Follow-up type: - Read the Decision Log PR comment FIRST before making any changes. - Checkout the PR branch before starting: git checkout " + Follow-up type: " ) ``` -5. Report back: what was addressed, current PR state, whether another round is needed. +## Step 3: Report Back + +After all subagents complete, report a summary for each: +- Classification, fix summary, PR URL, worktree path and branch, items needing human attention (2A) +- What was addressed, current PR state, whether another round is needed (2B) ## Subagent Lifecycle @@ -106,6 +113,8 @@ Context is preserved across agents via: ## Rules -- Always inline harness content in the subagent prompt — subagents cannot read skill files +- Subagent reads `.claude/harness/ppl-bugfix-harness.md` and fetches issue/PR details itself — do NOT inline content into the prompt - If bug is not reproducible (Phase 0.1), stop and report — do not proceed - Issue ↔ PR auto-resolution means the user never needs to track PR numbers manually +- **Do NOT use `mode: "auto"` for subagents** — `auto` mode does not work for subagents; Bash commands still require manual approval. Only `bypassPermissions` reliably skips permission checks. +- **Always dispatch subagent** — even for trivial follow-ups (remove co-author, force push). Do NOT run commands directly in the main session; subagents with `bypassPermissions` skip permission prompts, the main session does not. diff --git a/.claude/harness/ppl-bugfix-followup.md b/.claude/harness/ppl-bugfix-followup.md new file mode 100644 index 00000000000..697d49c806e --- /dev/null +++ b/.claude/harness/ppl-bugfix-followup.md @@ -0,0 +1,70 @@ +# PPL Bugfix Follow-up + +--- + +## Reconstruct Context + +The follow-up agent runs in a fresh worktree. First checkout the PR branch, then load state: + +```bash +# Checkout the PR branch in this worktree +gh pr checkout + +# Load PR state and Decision Log +gh pr view --json title,body,state,reviews,comments,statusCheckRollup,mergeable +gh pr checks +# Read the Decision Log comment FIRST — contains rejected alternatives and pitfalls +gh api repos///pulls//comments +``` + +## Handle Review Feedback + +For each comment, **cross-check against the Decision Log first**: + +| Type | Action | +|------|--------| +| Code change | If already rejected in Decision Log, reply with reasoning. Otherwise make the change, new commit, push | +| Question | Reply with explanation — Decision Log often has the answer | +| Nit | Fix if trivial | +| Disagreement | Reply with Decision Log reasoning; if reviewer insists, escalate to user | + +```bash +git add && git commit -s -m "Address review feedback: " +git push +``` + +## Clean Up Commit History + +When you need to amend a commit (e.g. remove Co-Authored-By, reword message) and the branch has a merge commit on top, don't try `git reset --soft origin/main` — it will include unrelated changes if main has moved. Instead cherry-pick the fix onto latest main: + +```bash +git checkout -B clean-branch origin/main +git cherry-pick +git commit --amend -s -m "" +git push clean-branch: --force +``` + +## Handle CI Failures + +```bash +gh pr checks # Identify failures +gh run view --log-failed # Read logs +# Test failure → fix locally, push new commit +# Spotless → ./gradlew spotlessApply, push +# Flaky → gh run rerun --failed +``` + +## Handle Merge Conflicts + +```bash +git fetch origin && git merge origin/main # Resolve conflicts +./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest # Re-verify +git commit -s -m "Resolve merge conflicts with main" +git push +``` + +## Mark Ready + +```bash +gh pr ready +``` diff --git a/ppl-bugfix-harness.md b/.claude/harness/ppl-bugfix-harness.md similarity index 84% rename from ppl-bugfix-harness.md rename to .claude/harness/ppl-bugfix-harness.md index 17fd2682d3b..1c4e408aaf5 100644 --- a/ppl-bugfix-harness.md +++ b/.claude/harness/ppl-bugfix-harness.md @@ -1,11 +1,17 @@ # PPL Bugfix Harness -> Systematic bugfix workflow distilled from 15+ historical commits. Invoke via `/ppl-bugfix #`. - --- ## Phase 0: Triage & Classification +### 0.0 Load Issue Context + +```bash +gh issue view --repo opensearch-project/sql +``` + +Read the issue title, description, and any reproduction steps before proceeding. + ### 0.1 Reproduce the Bug ```bash @@ -221,10 +227,8 @@ teardown: ### 3.2 Commit & Push -The subagent runs in an isolated **git worktree** branched from HEAD. All changes are on a dedicated branch that does not affect the user's working directory. - ```bash -# Commit with DCO sign-off +# Commit with DCO sign-off — do NOT add Co-Authored-By lines git add git commit -s -m "[BugFix] Fix (#)" @@ -277,68 +281,6 @@ EOF --- -## Phase 3.5: Post-PR Follow-up - -Runs as a **separate subagent** invocation — the original agent has completed. Invoke via `/ppl-bugfix #` or `/ppl-bugfix PR#` (auto-detects follow-up mode when a PR exists). - -### 3.5.1 Reconstruct Context - -The follow-up agent runs in a fresh worktree. First checkout the PR branch, then load state: - -```bash -# Checkout the PR branch in this worktree -gh pr checkout - -# Load PR state and Decision Log -gh pr view --json title,body,state,reviews,comments,statusCheckRollup,mergeable -gh pr checks -# Read the Decision Log comment FIRST — contains rejected alternatives and pitfalls -gh api repos///pulls//comments -``` - -### 3.5.2 Handle Review Feedback - -For each comment, **cross-check against the Decision Log first**: - -| Type | Action | -|------|--------| -| Code change | If already rejected in Decision Log, reply with reasoning. Otherwise make the change, new commit, push | -| Question | Reply with explanation — Decision Log often has the answer | -| Nit | Fix if trivial | -| Disagreement | Reply with Decision Log reasoning; if reviewer insists, escalate to user | - -```bash -git add && git commit -s -m "Address review feedback: " -git push -``` - -### 3.5.3 Handle CI Failures - -```bash -gh pr checks # Identify failures -gh run view --log-failed # Read logs -# Test failure → fix locally, push new commit -# Spotless → ./gradlew spotlessApply, push -# Flaky → gh run rerun --failed -``` - -### 3.5.4 Handle Merge Conflicts - -```bash -git fetch origin && git merge origin/main # Resolve conflicts -./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest # Re-verify -git commit -s -m "Resolve merge conflicts with main" -git push -``` - -### 3.5.5 Mark Ready - -```bash -gh pr ready -``` - ---- - ## Phase 4: Retrospective After each bugfix, check if the harness itself needs updating: @@ -381,3 +323,4 @@ Regex/function extraction offset → Path B: ordinal vs named r | `734394d` | Grammar rule typo | Grammar | — | | `246ed0d` | Float precision flaky test | Test Infra | — | | `d56b8fa` | Wildcard index type conflict | Value Parsing | 3 UT + 1 IT + 1 YAML | +| `5a78b78` | Boolean coercion from numeric in wildcard queries | Value Parsing | 3 UT + 1 IT + 1 YAML | diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000000..160f8548c50 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,26 @@ +{ + "permissions": { + "allow": [ + "Bash(./gradlew *)", + "Bash(gh issue:*)", + "Bash(gh pr:*)", + "Bash(gh api:*)", + "Bash(gh search:*)", + "Bash(gh run:*)", + "Bash(git add:*)", + "Bash(git commit:*)", + "Bash(git stash:*)", + "Bash(git show:*)", + "Bash(git diff:*)", + "Bash(git status:*)", + "Bash(git log:*)", + "Bash(git branch:*)", + "Bash(git remote:*)", + "Bash(git fetch:*)", + "Bash(git checkout:*)", + "Bash(git push:*)", + "Bash(git cherry-pick:*)", + "Bash(git reset:*)" + ] + } +} diff --git a/.gitignore b/.gitignore index 34b41594bf0..bf9002f999d 100644 --- a/.gitignore +++ b/.gitignore @@ -56,6 +56,7 @@ http-client.env.json .claude/* !.claude/settings.json !.claude/commands/ +!.claude/harness/ .claude/settings.local.json .clinerules memory-bank \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index 3e7b58e4bdb..8a61c46bc1c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -125,7 +125,7 @@ plugin (OpenSearch plugin entry point, Guice DI wiring) ## Fixing PPL Bugs -Use the `/ppl-bugfix #` slash command to fix PPL bugs. It dispatches a subagent with scoped permissions that follows all phases in `ppl-bugfix-harness.md` — from Phase 0 (Triage & Classification) through Phase 4 (Retrospective & Improvement). The full harness content is inlined into the subagent prompt to guarantee compliance. If a user asks to fix a PPL bug without using the slash command, remind them to use `/ppl-bugfix` or invoke it on their behalf. +Use the `/ppl-bugfix #` slash command to fix PPL bugs. It dispatches a subagent in an isolated worktree that reads `.claude/harness/ppl-bugfix-harness.md` and follows all phases — from Phase 0 (Triage & Classification) through Phase 4 (Retrospective & Improvement). If a user asks to fix a PPL bug without using the slash command, remind them to use `/ppl-bugfix` or invoke it on their behalf. ## Adding New PPL Commands diff --git a/CLAUDE_GUIDE.md b/CLAUDE_GUIDE.md new file mode 100644 index 00000000000..f5eff489d65 --- /dev/null +++ b/CLAUDE_GUIDE.md @@ -0,0 +1,54 @@ +# Claude Commands + +Slash commands for Claude Code in this repository. Use them in any Claude Code session. + +## `/ppl-bugfix` + +Fix a PPL bug end-to-end or follow up on an existing PR. + +**Usage:** + +``` +/ppl-bugfix #1234 # Single issue +/ppl-bugfix PR#5678 # Single PR follow-up +/ppl-bugfix #1234 #5678 PR#9012 # Multiple in parallel +/ppl-bugfix # By URL +``` + +**Permission mode flags** (optional, append to any input): + +| Flag | Mode | Description | +|------|------|-------------| +| `--safe` | `acceptEdits` | File edits auto-approved, Bash commands need manual approval | +| `--yolo` | `bypassPermissions` | No prompts at all — subagent runs in isolated worktree (default) | + +**What it does:** + +1. Resolves issue/PR linkage automatically +2. For new issues: dispatches a subagent in an isolated git worktree that follows the full bugfix harness (triage → fix → test → PR) +3. For existing PRs: handles CI failures, review feedback, merge conflicts, or marks as ready + +**Related files:** [`.claude/harness/ppl-bugfix-harness.md`](.claude/harness/ppl-bugfix-harness.md) + +--- + +## `/dedupe` + +Find duplicate GitHub issues for a given issue. + +**Usage:** + +``` +/dedupe 1234 +``` + +**What it does:** + +1. Reads the target issue +2. Runs 3+ parallel search strategies to find potential duplicates (only older issues) +3. Verifies candidates by reading each one +4. Posts a structured comment on the issue listing 1-3 confirmed duplicates (if any) + +**Skips:** closed issues, broad feedback issues, issues already checked + +**Related files:** [`scripts/comment-on-duplicates.sh`](scripts/comment-on-duplicates.sh) From b9584ee8c26d1359cf1be8b5f9b1bf4be4d54b5d Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 14:36:36 +0800 Subject: [PATCH 03/10] Fix follow-up harness to check PR comments including bot feedback The Reconstruct Context section now explicitly loads all PR comments (bot and human) and categorizes bot suggestions as actionable review feedback, not just CI checks and human reviews. Signed-off-by: Heng Qian --- .claude/harness/ppl-bugfix-followup.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/.claude/harness/ppl-bugfix-followup.md b/.claude/harness/ppl-bugfix-followup.md index 697d49c806e..3a230169961 100644 --- a/.claude/harness/ppl-bugfix-followup.md +++ b/.claude/harness/ppl-bugfix-followup.md @@ -10,16 +10,27 @@ The follow-up agent runs in a fresh worktree. First checkout the PR branch, then # Checkout the PR branch in this worktree gh pr checkout -# Load PR state and Decision Log -gh pr view --json title,body,state,reviews,comments,statusCheckRollup,mergeable +# Load PR state — reviews, CI, mergeability +gh pr view --json title,body,state,reviews,statusCheckRollup,mergeable gh pr checks -# Read the Decision Log comment FIRST — contains rejected alternatives and pitfalls -gh api repos///pulls//comments + +# Load ALL comments — includes bot comments (Code-Diff-Analyzer, PR Reviewer Guide, Code Suggestions) and human comments +gh pr view --json comments --jq '.comments[] | {author: .author.login, body: .body}' ``` +Categorize ALL signals — not just CI and human reviews: + +| Signal | Type | +|--------|------| +| `statusCheckRollup` has failures | CI failure | +| `reviews` has CHANGES_REQUESTED | Review feedback | +| `mergeable` is CONFLICTING | Merge conflict | +| Bot comments with actionable suggestions | Review feedback (treat like human review) | +| All pass + approved | Ready — run `gh pr ready` | + ## Handle Review Feedback -For each comment, **cross-check against the Decision Log first**: +For each comment (human OR bot), **cross-check against the Decision Log first**: | Type | Action | |------|--------| From 57bfdb02865e1e3ba79adcab5d2df53e5fb8f4d7 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 14:42:41 +0800 Subject: [PATCH 04/10] Address review feedback from bot comments on PR #5307 - Add missing git merge permission to settings.json (used in harness merge steps) - Search all PR closing keyword variants (Resolves/Fixes/Closes) for issue-PR linkage - Replace grep -oP with portable grep -oE for macOS compatibility - Use --force-with-lease instead of --force in cherry-pick cleanup flow - Make post-merge test re-run mandatory with explicit command - Add --repo opensearch-project/sql to gh pr create for correct targeting Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 6 ++++-- .claude/harness/ppl-bugfix-followup.md | 2 +- .claude/harness/ppl-bugfix-harness.md | 5 +++-- .claude/settings.json | 1 + 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md index 026ddf6a821..9950f1d129f 100644 --- a/.claude/commands/ppl-bugfix.md +++ b/.claude/commands/ppl-bugfix.md @@ -43,11 +43,13 @@ Use the resolved mode as the `mode` parameter when dispatching the subagent in S For each issue/PR reference in the input, resolve its state. Run these lookups in parallel when there are multiple references. ```bash -# Issue → PR +# Issue → PR (check multiple closing keyword variants) gh pr list --search "Resolves #" --json number,url,state --limit 5 +gh pr list --search "Fixes #" --json number,url,state --limit 5 +gh pr list --search "Closes #" --json number,url,state --limit 5 # PR → Issue -gh pr view --json body | jq -r '.body' | grep -oP 'Resolves #\K[0-9]+' +gh pr view --json body | jq -r '.body' | grep -oiE '(resolves|fixes|closes) #[0-9]+' | grep -oE '[0-9]+' ``` | State | Action | diff --git a/.claude/harness/ppl-bugfix-followup.md b/.claude/harness/ppl-bugfix-followup.md index 3a230169961..6e0ce0a8ff2 100644 --- a/.claude/harness/ppl-bugfix-followup.md +++ b/.claude/harness/ppl-bugfix-followup.md @@ -52,7 +52,7 @@ When you need to amend a commit (e.g. remove Co-Authored-By, reword message) and git checkout -B clean-branch origin/main git cherry-pick git commit --amend -s -m "" -git push clean-branch: --force +git push clean-branch: --force-with-lease ``` ## Handle CI Failures diff --git a/.claude/harness/ppl-bugfix-harness.md b/.claude/harness/ppl-bugfix-harness.md index 1c4e408aaf5..84e45a973c5 100644 --- a/.claude/harness/ppl-bugfix-harness.md +++ b/.claude/harness/ppl-bugfix-harness.md @@ -234,7 +234,8 @@ git commit -s -m "[BugFix] Fix (#)" # Sync with main (merge, not rebase — PRs use squash-merge) git fetch origin && git merge origin/main -# Re-run tests if merge brought changes +# Always re-run tests after merge — even a trivial merge can introduce regressions +./gradlew test && ./gradlew :integ-test:integTest -Dtests.class="*" # Push to personal fork (NOT origin if it points to upstream) git remote -v # confirm your fork remote @@ -244,7 +245,7 @@ git push -u ### 3.3 Create Draft PR ```bash -gh pr create --draft --title "[BugFix] Fix (#)" --body "$(cat <<'EOF' +gh pr create --draft --repo opensearch-project/sql --title "[BugFix] Fix (#)" --body "$(cat <<'EOF' ### Description diff --git a/.claude/settings.json b/.claude/settings.json index 160f8548c50..b1fbf3ef548 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -19,6 +19,7 @@ "Bash(git fetch:*)", "Bash(git checkout:*)", "Bash(git push:*)", + "Bash(git merge:*)", "Bash(git cherry-pick:*)", "Bash(git reset:*)" ] From 638f0ce2d53d0d4cc599c0a73e418ddc574c01fe Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 15:54:37 +0800 Subject: [PATCH 05/10] Tighten git permissions: restrict push and reset to safe variants - Replace broad `git push:*` with `git push -u:*` and `git push --force-with-lease:*` to prevent accidental force-pushes to upstream - Restrict `git reset:*` to `git reset --soft:*` since the harness only uses soft reset - Update followup harness push commands to use `-u` flag consistently Signed-off-by: Heng Qian --- .claude/harness/ppl-bugfix-followup.md | 4 ++-- .claude/settings.json | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.claude/harness/ppl-bugfix-followup.md b/.claude/harness/ppl-bugfix-followup.md index 6e0ce0a8ff2..1de9a27c4bd 100644 --- a/.claude/harness/ppl-bugfix-followup.md +++ b/.claude/harness/ppl-bugfix-followup.md @@ -41,7 +41,7 @@ For each comment (human OR bot), **cross-check against the Decision Log first**: ```bash git add && git commit -s -m "Address review feedback: " -git push +git push -u ``` ## Clean Up Commit History @@ -71,7 +71,7 @@ gh run view --log-failed # Read logs git fetch origin && git merge origin/main # Resolve conflicts ./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest # Re-verify git commit -s -m "Resolve merge conflicts with main" -git push +git push -u ``` ## Mark Ready diff --git a/.claude/settings.json b/.claude/settings.json index b1fbf3ef548..eae8ab7e33d 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -18,10 +18,11 @@ "Bash(git remote:*)", "Bash(git fetch:*)", "Bash(git checkout:*)", - "Bash(git push:*)", + "Bash(git push -u:*)", + "Bash(git push --force-with-lease:*)", "Bash(git merge:*)", "Bash(git cherry-pick:*)", - "Bash(git reset:*)" + "Bash(git reset --soft:*)" ] } } From 6b29b60587501ffe32909c8e9b70f0ff8445588b Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 16:21:38 +0800 Subject: [PATCH 06/10] Add retrospective step to follow-up harness Followup agent now reflects on each feedback comment to identify gaps in the harness that should have prevented the issue, and fixes them in the same commit. Signed-off-by: Heng Qian --- .claude/harness/ppl-bugfix-followup.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.claude/harness/ppl-bugfix-followup.md b/.claude/harness/ppl-bugfix-followup.md index 1de9a27c4bd..96470496530 100644 --- a/.claude/harness/ppl-bugfix-followup.md +++ b/.claude/harness/ppl-bugfix-followup.md @@ -79,3 +79,16 @@ git push -u ```bash gh pr ready ``` + +## Retrospective + +After handling follow-up, reflect on the feedback received and check if it reveals gaps in the harness or command: + +For each comment addressed (bot or human): +- **Does the feedback point to a pattern the harness should have prevented?** → Add guidance to the relevant Phase in `ppl-bugfix-harness.md` +- **Was this a repeated mistake across PRs?** → Add to Quick Reference or Case Index +- **Did the harness template produce the problematic code?** → Fix the template directly +- **Was a permission or tool missing?** → Add to `.claude/settings.json` +- **Did the follow-up workflow itself miss this signal?** → Update this file + +If any improvement is needed, make the edit and include it in the same commit. From ce54987ecc7a11b03703a6a92962cae9fc69ee0d Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 16:31:49 +0800 Subject: [PATCH 07/10] Streamline ppl-bugfix workflow: fix 5 cross-cutting issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add fork remote discovery step to both harness and followup (replace placeholder with concrete instructions) - Remove redundant follow-up type from command dispatch — subagent discovers it from PR state via followup harness categorization table - Fix Phase 0.1: remove fake REST API call, use integration test only - Add no-coauthor rule to followup harness (was only in harness) - Add --repo flag to gh pr checkout in followup for worktree context Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 3 +-- .claude/harness/ppl-bugfix-followup.md | 15 ++++++++++++--- .claude/harness/ppl-bugfix-harness.md | 18 ++++++++++-------- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md index 9950f1d129f..7be9ae5adbc 100644 --- a/.claude/commands/ppl-bugfix.md +++ b/.claude/commands/ppl-bugfix.md @@ -86,8 +86,7 @@ Agent( name: "bugfix-", description: "PPL bugfix # followup", prompt: "Read .claude/harness/ppl-bugfix-followup.md and follow it. - PR: (), Issue: # - Follow-up type: " + PR: (), Issue: #" ) ``` diff --git a/.claude/harness/ppl-bugfix-followup.md b/.claude/harness/ppl-bugfix-followup.md index 96470496530..b9eb18d6296 100644 --- a/.claude/harness/ppl-bugfix-followup.md +++ b/.claude/harness/ppl-bugfix-followup.md @@ -1,5 +1,9 @@ # PPL Bugfix Follow-up +## Rules + +- Do NOT add `Co-Authored-By` lines in commits — only DCO `Signed-off-by` + --- ## Reconstruct Context @@ -10,6 +14,11 @@ The follow-up agent runs in a fresh worktree. First checkout the PR branch, then # Checkout the PR branch in this worktree gh pr checkout +# Resolve fork remote — the worktree may only have origin (upstream) +git remote -v +# If no fork remote exists, add it: +git remote add fork https://github.com//sql.git + # Load PR state — reviews, CI, mergeability gh pr view --json title,body,state,reviews,statusCheckRollup,mergeable gh pr checks @@ -41,7 +50,7 @@ For each comment (human OR bot), **cross-check against the Decision Log first**: ```bash git add && git commit -s -m "Address review feedback: " -git push -u +git push -u fork ``` ## Clean Up Commit History @@ -52,7 +61,7 @@ When you need to amend a commit (e.g. remove Co-Authored-By, reword message) and git checkout -B clean-branch origin/main git cherry-pick git commit --amend -s -m "" -git push clean-branch: --force-with-lease +git push fork clean-branch: --force-with-lease ``` ## Handle CI Failures @@ -71,7 +80,7 @@ gh run view --log-failed # Read logs git fetch origin && git merge origin/main # Resolve conflicts ./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest # Re-verify git commit -s -m "Resolve merge conflicts with main" -git push -u +git push -u fork ``` ## Mark Ready diff --git a/.claude/harness/ppl-bugfix-harness.md b/.claude/harness/ppl-bugfix-harness.md index 84e45a973c5..37f47fa104d 100644 --- a/.claude/harness/ppl-bugfix-harness.md +++ b/.claude/harness/ppl-bugfix-harness.md @@ -15,11 +15,7 @@ Read the issue title, description, and any reproduction steps before proceeding. ### 0.1 Reproduce the Bug ```bash -# Minimal PPL query -POST /_plugins/_ppl/_query -{ "query": "" } - -# Or via integration test +# Write a failing integration test, or run an existing one ./gradlew :integ-test:integTest -Dtests.class="*" -Dtests.method="" ``` @@ -237,9 +233,15 @@ git fetch origin && git merge origin/main # Always re-run tests after merge — even a trivial merge can introduce regressions ./gradlew test && ./gradlew :integ-test:integTest -Dtests.class="*" -# Push to personal fork (NOT origin if it points to upstream) -git remote -v # confirm your fork remote -git push -u +# Resolve fork remote — find or add it +# 1. Check if a fork remote already exists (not opensearch-project/sql) +git remote -v +# 2. If only origin (upstream) exists, add the fork: +git remote add fork https://github.com//sql.git +# Use the git user name to infer the fork owner, or fall back to "qianheng-aws" + +# Push +git push -u fork ``` ### 3.3 Create Draft PR From 45bbb51e6b10fa27902e04a3af49a7bc105ec369 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 16:35:39 +0800 Subject: [PATCH 08/10] Phase 4: clarify that harness improvements go in the same PR Signed-off-by: Heng Qian --- .claude/harness/ppl-bugfix-harness.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.claude/harness/ppl-bugfix-harness.md b/.claude/harness/ppl-bugfix-harness.md index 37f47fa104d..ac13eff36b4 100644 --- a/.claude/harness/ppl-bugfix-harness.md +++ b/.claude/harness/ppl-bugfix-harness.md @@ -293,6 +293,8 @@ After each bugfix, check if the harness itself needs updating: - **Verification gap** caused rework? → Add step to 3.1 or 2.2 checklist - **Representative fix**? → Add row to Case Index below +Include harness improvements in the same PR as the bugfix — they share the same review context. + --- ## Quick Reference: Symptom → Fix Path From 42caa29c0638342cb03c66ebcc8f131b9bd124e8 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 3 Apr 2026 17:55:14 +0800 Subject: [PATCH 09/10] Enforce hard stop when bug is already fixed on main Subagent was ignoring the "stop and report back" instruction and still creating regression tests + PRs for bugs that no longer reproduce. - Harness: clarify definition of "not reproducible", replace soft "stop and report back" with explicit HARD STOP + forbidden actions - Skill: add CRITICAL instruction in subagent dispatch prompt so it sees the constraint before even reading the harness Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 4 +++- .claude/harness/ppl-bugfix-harness.md | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md index 7be9ae5adbc..41111b8e341 100644 --- a/.claude/commands/ppl-bugfix.md +++ b/.claude/commands/ppl-bugfix.md @@ -73,7 +73,9 @@ Agent( prompt: "Read .claude/harness/ppl-bugfix-harness.md and follow it to fix GitHub issue #. Follow Phase 0 through Phase 3 in order. Phase 0.3 defines TDD execution flow. Do NOT skip any phase. - Post the Decision Log (Phase 3.4) before completing." + CRITICAL: If Phase 0.1 determines the bug is already fixed on main, HARD STOP. + Do NOT write tests, do NOT create a PR — just comment/close the issue and report back. + If the bug IS reproducible, post the Decision Log (Phase 3.4) before completing." ) ``` diff --git a/.claude/harness/ppl-bugfix-harness.md b/.claude/harness/ppl-bugfix-harness.md index ac13eff36b4..2d06a6f9264 100644 --- a/.claude/harness/ppl-bugfix-harness.md +++ b/.claude/harness/ppl-bugfix-harness.md @@ -19,16 +19,16 @@ Read the issue title, description, and any reproduction steps before proceeding. ./gradlew :integ-test:integTest -Dtests.class="*" -Dtests.method="" ``` -**If not reproducible on latest `main`**: +**If the bug does not reproduce on latest `main`** — meaning the test scenario runs but produces correct results instead of the reported error — the bug is already fixed. This is distinct from "can't set up the scenario" (which is an infra issue). | Finding | Action | |---------|--------| -| Already fixed by another commit | Comment with fixing commit hash, close as duplicate | -| Only on older version | Comment with version where it's fixed, close as resolved | +| Already fixed by another commit | `gh issue comment` with fixing commit/PR, then `gh issue close` | +| Only on older version | `gh issue comment` with version where it's fixed, then `gh issue close` | | Intermittent / environment-specific | Label `flaky` or `needs-info`, do NOT close | | Insufficient info to reproduce | Comment asking for repro steps, label `needs-info` | -In all cases, **stop and report back** — do not proceed to Phase 1. +**HARD STOP**: Do NOT proceed to Phase 1, 2, or 3. Do NOT write tests. Do NOT create a PR. Report back with the finding and the action taken (comment/close). Your job is done. ### 0.2 Classify and Route From e2ff86439cfeba9fff977ce074e4242ff7128e9f Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Sat, 4 Apr 2026 14:49:33 +0800 Subject: [PATCH 10/10] Streamline ppl-bugfix harness and CLAUDE.md after eval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Harness: 369→151 lines. Move fix paths, test templates, symptom table, and case index to ppl-bugfix-reference.md (loaded on demand). Add completion gate with 8 mandatory deliverables. Add guardrails for runaway agents. Merge overlapping checklists. CLAUDE.md: trim build commands, remove v2 engine references, simplify ppl-bugfix section. Command: add bypassPermissions allow-list note. Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 2 + .claude/harness/ppl-bugfix-harness.md | 300 +++++------------------- .claude/harness/ppl-bugfix-reference.md | 148 ++++++++++++ CLAUDE.md | 58 ++--- 4 files changed, 224 insertions(+), 284 deletions(-) create mode 100644 .claude/harness/ppl-bugfix-reference.md diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md index 41111b8e341..e12cefce9c8 100644 --- a/.claude/commands/ppl-bugfix.md +++ b/.claude/commands/ppl-bugfix.md @@ -18,6 +18,8 @@ Optional mode flag (append to any of the above): - `--safe` — `acceptEdits` mode. Auto-approve file edits only, Bash commands require manual approval. (Most conservative) - `--yolo` — `bypassPermissions` mode. Fully trusted, no prompts. Subagent runs in an isolated worktree so this is safe. (Default) +> **Note**: `bypassPermissions` skips the interactive prompt but still respects the allow-list in `~/.claude/settings.json`. Ensure git/gh write commands are in the global allow-list. + Examples: - `/ppl-bugfix #1234` — single issue, defaults to yolo - `/ppl-bugfix #1234 #5678 --yolo` — two issues in parallel diff --git a/.claude/harness/ppl-bugfix-harness.md b/.claude/harness/ppl-bugfix-harness.md index 2d06a6f9264..5845385b550 100644 --- a/.claude/harness/ppl-bugfix-harness.md +++ b/.claude/harness/ppl-bugfix-harness.md @@ -1,253 +1,95 @@ # PPL Bugfix Harness ---- - -## Phase 0: Triage & Classification +## Phase 0: Triage -### 0.0 Load Issue Context +### 0.1 Load & Reproduce ```bash gh issue view --repo opensearch-project/sql ``` -Read the issue title, description, and any reproduction steps before proceeding. - -### 0.1 Reproduce the Bug - -```bash -# Write a failing integration test, or run an existing one -./gradlew :integ-test:integTest -Dtests.class="*" -Dtests.method="" -``` +Write a failing test or run an existing one to reproduce the bug on `main`. -**If the bug does not reproduce on latest `main`** — meaning the test scenario runs but produces correct results instead of the reported error — the bug is already fixed. This is distinct from "can't set up the scenario" (which is an infra issue). +If the bug **does not reproduce** (correct results, not infra failure): | Finding | Action | |---------|--------| -| Already fixed by another commit | `gh issue comment` with fixing commit/PR, then `gh issue close` | -| Only on older version | `gh issue comment` with version where it's fixed, then `gh issue close` | -| Intermittent / environment-specific | Label `flaky` or `needs-info`, do NOT close | -| Insufficient info to reproduce | Comment asking for repro steps, label `needs-info` | +| Already fixed | `gh issue comment` + `gh issue close` | +| Older version only | `gh issue comment` + `gh issue close` | +| Intermittent | Label `flaky` or `needs-info`, do NOT close | +| Can't reproduce | Comment asking for repro steps, label `needs-info` | -**HARD STOP**: Do NOT proceed to Phase 1, 2, or 3. Do NOT write tests. Do NOT create a PR. Report back with the finding and the action taken (comment/close). Your job is done. +**HARD STOP** — do not proceed. Report back. -### 0.2 Classify and Route +### 0.2 Classify -Identify the bug layer, then use the routing table to determine the fix path and required tests: +Identify the bug layer (Grammar, AST/Functions, Type System, Optimizer, Execution, DI/Resource) and record it. Consult `.claude/harness/ppl-bugfix-reference.md` for fix-path-specific guidance if needed. -| Layer | Typical Symptoms | Fix Path | Required Tests | -|-------|-----------------|----------|---------------| -| **Grammar/Parser** | `SyntaxCheckException`, unrecognized syntax | Path A | AstBuilderTest | -| **AST/Functions** | Parses OK but AST wrong, function output wrong | Path B | CalcitePPL\*Test + IT + YAML | -| **Semantic Analysis** | `SemanticCheckException`, type mismatch | Path C | Dedicated UT + CalcitePPLIT + YAML | -| **Type System** | Field type lost, implicit conversion errors | Path C | Dedicated UT + CalcitePPLIT + YAML | -| **Optimizer** | Plan bloat, predicate pushdown failure | Path D | CalcitePPL\*Test + CalcitePPLIT + NoPushdownIT + YAML | -| **Execution** | Wrong results, OOM, memory leaks | Path E | IT + YAML | -| **DI / Resource** | NPE, extensions not loaded, long-running OOM | Path E | IT + YAML | +### 0.3 Guardrails -Record before proceeding: +Stop and report back if: +- Root cause unclear after reading 15+ source files +- Fix breaks 5+ unrelated tests +- Same build error 3 times in a row -``` -Bug Layer: -Fix Path: -Issue: #XXXX -``` - -### 0.3 Execution Flow - -Phases interleave TDD-style — write a failing test first, then fix, then complete test coverage: +### 0.4 Execution Flow ``` -Phase 0: Triage → Classify → Route - → Phase 2 (partial): Write FAILING test reproducing the bug - → Phase 1: Implement fix via routed Path - → Phase 2 (remaining): Happy path, edge cases, YAML REST test - → Phase 3: Verify → Commit → PR → Decision Log +Triage → Write FAILING test → Fix → Remaining tests → Verify → Commit → PR → Decision Log → Completion Gate ``` --- -## Phase 1: Fix Implementation - -### Path A — Grammar / Parser - -1. **Update grammar files** (must stay in sync): - - `language-grammar/src/main/antlr4/OpenSearchPPLParser.g4` (primary) - - `ppl/src/main/antlr/OpenSearchPPLParser.g4` - - `async-query-core/src/main/antlr/OpenSearchPPLParser.g4` (if applicable) -2. **Regenerate**: `./gradlew generateGrammarSource` -3. **Update AstBuilder**: `ppl/.../parser/AstBuilder.java` — modify `visit*()` to match new rule -4. **Test**: `AstBuilderTest` using `assertEqual(pplQuery, expectedAstNode)` pattern - -> Ref: `734394d` — fixed `renameClasue` → `renameClause` across 3 grammars + AstBuilder - -### Path B — AST / Function Implementation - -1. **Locate**: AST nodes in `core/.../ast/tree/`, functions in `core/.../expression/function/` or `PPLBuiltinOperators` -2. **Fix**: Watch Visitor pattern — changes may need syncing to `AbstractNodeVisitor`, `Analyzer`, `CalciteRelNodeVisitor`, `PPLQueryDataAnonymizer` -3. **Test**: `verifyLogical()`, `verifyPPLToSparkSQL()`, `verifyResult()` - -> Ref: `26674f9` — rex nested capture group fix, ordinal index → named group extraction - -### Path C — Type System / Semantic Analysis - -1. **Locate**: `OpenSearchTypeFactory.java` (Calcite type factory), `Analyzer.java` / `ExpressionAnalyzer.java` -2. **Fix**: Preserve nullable semantics when overriding Calcite methods; protect UDT from `leastRestrictive()` downgrade -3. **Test (critical)**: Cover type preservation, nullable propagation, fallback to parent, mixed types — every edge case - -> Ref: `ada2e34` — UNION lost timestamp UDT, fixed `leastRestrictive()`, added 8 UTs + 4 ITs - -### Path D — Optimizer / Predicate Pushdown - -1. **Locate**: `PredicateAnalyzer.java`, `LogicalPlanOptimizer`, `QueryService.java` -2. **Fix**: Watch `nullAs` semantics (TRUE/FALSE/UNKNOWN); for plan bloat consider Calcite rules like `FilterMergeRule` -3. **Verify**: `EXPLAIN` output comparison + integration test result correctness +## Phase 1: Fix -> Ref: `b4df010` — `isnotnull()` not pushed down with multiple `!=`; `e045d15` — multi-filter OOM, inserted `FilterMergeRule` - -### Path E — Execution / Resource Management - -1. **Locate**: DQE operators in `dqe/`, `OpenSearchExecutionEngine.java`, `SQLPlugin.java`, `OpenSearchPluginModule.java` -2. **Common patterns**: - - | Problem | Fix | Example | - |---------|-----|---------| - | Cache key collision | `IndexReader.CacheHelper.getKey()` | `97d5d26` | - | Memory leak (no eviction) | Close listener + upper bound | `97d5d26` | - | Unbounded growth | `MAX_CAPACITY` check, throw with user guidance | `f024b4f` | - | Non-singleton repeated registration | `@Singleton`; `put` instead of `compute/append` | `90393bf` | - | DI not injected | Holder class → Guice → constructor injection | `f6be830` | - -> Ref: `f024b4f` — `LongOpenHashSet` capacity 1024→8 (8KB→64B per group), 8M cap on 5 HashMap variants +Find and fix the root cause. Consult `.claude/harness/ppl-bugfix-reference.md` for path-specific patterns and examples. --- -## Phase 2: Writing Tests - -### 2.1 Test Templates - -**Unit test** (extend `CalcitePPLAbstractTest`): -```java -public class CalcitePPLYourFixTest extends CalcitePPLAbstractTest { - public CalcitePPLYourFixTest() { - super(CalciteAssert.SchemaSpec.SCOTT_WITH_TEMPORAL); - } - - @Before - public void init() { - doReturn(true).when(settings) - .getSettingValue(Settings.Key.CALCITE_ENGINE_ENABLED); - } - - @Test - public void testBugScenario() { - verifyLogical("source=EMP | where SAL > 1000", - "LogicalFilter(condition=[>($5, 1000)])\n" - + " LogicalTableScan(table=[[scott, EMP]])\n"); - } -} -``` - -**Integration test** (extend `CalcitePPLIT`): -```java -public class CalcitePPLYourFixIT extends CalcitePPLIT { - @Override - public void init() throws IOException { - super.init(); - enableCalcite(); - } - - @Test - public void testBugFixEndToEnd() throws IOException { - JSONObject result = executeQuery("source= | "); - verifySchema(result, schema("field", "alias", "type")); - verifyDataRows(result, rows("expected_value_1"), rows("expected_value_2")); - } -} -``` - -**YAML REST test** — place at `integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/.yml`, auto-discovered by `RestHandlerClientYamlTestSuiteIT`: -```yaml -setup: - - do: - indices.create: - index: test_issue_ - body: - settings: { number_of_shards: 1, number_of_replicas: 0 } - mappings: { properties: { : { type: } } } - - do: - query.settings: - body: { transient: { plugins.calcite.enabled: true } } ---- -teardown: - - do: - query.settings: - body: { transient: { plugins.calcite.enabled: false } } ---- -"": - - skip: { features: [headers, allowed_warnings] } - - do: - bulk: { index: test_issue_, refresh: true, body: ['{"index": {}}', '{"": ""}'] } - - do: - headers: { Content-Type: 'application/json' } - ppl: { body: { query: "source=test_issue_ | " } } - - match: { total: } - - length: { datarows: } -``` +## Phase 2: Tests -### 2.2 Test Checklist +Consult `.claude/harness/ppl-bugfix-reference.md` for test templates. -- [ ] Failing test reproducing the original bug (write FIRST, before fixing) -- [ ] Happy path after fix -- [ ] Edge cases (null, empty, extreme volumes) -- [ ] YAML REST test (named by issue number) -- [ ] Optimizer bugs: both pushdown enabled and disabled -- [ ] Type system bugs: nullable / non-nullable combinations -- [ ] New AST nodes: update `PPLQueryDataAnonymizerTest` +Required deliverables: +- Failing test reproducing the bug (written BEFORE the fix) +- Unit tests covering happy path and edge cases +- Integration test (`*IT.java` extending `CalcitePPLIT`) +- YAML REST test at `integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/.yml` --- -## Phase 3: Verification & Submission +## Phase 3: Verify & Submit -### 3.1 Local Verification +### 3.1 Verify ```bash -./gradlew spotlessApply # 1. Format -./gradlew ::test --tests "" # 2. Affected module UT -./gradlew test # 3. Full UT regression -./gradlew :integ-test:integTest -Dtests.class="*" # 4. New IT -./gradlew :integ-test:integTest # 5. Full IT regression -./gradlew :integ-test:yamlRestTest # 6. YAML REST tests -# If grammar modified: -./gradlew generateGrammarSource && ./gradlew :ppl:test # 7. Parser tests +./gradlew spotlessApply +./gradlew ::test --tests "" +./gradlew test +./gradlew :integ-test:integTest -Dtests.class="*" ``` -### 3.2 Commit & Push +Run `./gradlew :integ-test:yamlRestTest` if YAML tests were added. Run `./gradlew generateGrammarSource && ./gradlew :ppl:test` if grammar was modified. + +### 3.2 Commit & PR ```bash -# Commit with DCO sign-off — do NOT add Co-Authored-By lines git add git commit -s -m "[BugFix] Fix (#)" - -# Sync with main (merge, not rebase — PRs use squash-merge) git fetch origin && git merge origin/main -# Always re-run tests after merge — even a trivial merge can introduce regressions ./gradlew test && ./gradlew :integ-test:integTest -Dtests.class="*" -# Resolve fork remote — find or add it -# 1. Check if a fork remote already exists (not opensearch-project/sql) -git remote -v -# 2. If only origin (upstream) exists, add the fork: +# Resolve fork remote (check git remote -v; add if missing) git remote add fork https://github.com//sql.git -# Use the git user name to infer the fork owner, or fall back to "qianheng-aws" - -# Push git push -u fork ``` -### 3.3 Create Draft PR +Do NOT add Co-Authored-By lines. Use the git user name to infer the fork owner, or fall back to "qianheng-aws". ```bash -gh pr create --draft --repo opensearch-project/sql --title "[BugFix] Fix (#)" --body "$(cat <<'EOF' +gh pr create --draft --repo opensearch-project/sql \ + --title "[BugFix] Fix (#)" \ + --body "$(cat <<'EOF' ### Description @@ -256,19 +98,17 @@ Resolves # ### Check List - [x] New functionality includes testing -- [x] Javadoc added for new public API - [x] Commits signed per DCO (`-s`) - [x] `spotlessCheck` passed - [x] Unit tests passed - [x] Integration tests passed -- [ ] Grammar changes: all three `.g4` files synced (if applicable) EOF )" ``` -### 3.4 Persist Decision Context +### 3.3 Decision Log -Before the subagent completes, persist the *why* behind key decisions — the PR diff only shows the *what*. The **PR comment is the single source of truth** — it survives across sessions, GA runs, and is visible to both human reviewers and follow-up agents. +Post as a PR comment: ```bash gh pr comment --body "$(cat <<'EOF' @@ -284,48 +124,28 @@ EOF --- -## Phase 4: Retrospective +## Completion Gate -After each bugfix, check if the harness itself needs updating: +Do NOT report "done" until every item below is checked. List each in your final report: -- **Template wrong** (API name, assertion field)? → Fix in Phase 2 templates -- **New bug pattern** not covered? → Add fix path in Phase 1 or symptom in Quick Reference -- **Verification gap** caused rework? → Add step to 3.1 or 2.2 checklist -- **Representative fix**? → Add row to Case Index below +- [ ] **Unit tests**: New test class or methods +- [ ] **Integration test**: New `*IT.java` test +- [ ] **YAML REST test**: `issues/.yml` +- [ ] **spotlessApply**: Ran successfully +- [ ] **Tests pass**: Affected modules +- [ ] **Commit**: DCO sign-off, `[BugFix]` prefix, no Co-Authored-By +- [ ] **Draft PR**: `--draft`, body contains `Resolves #` +- [ ] **Decision Log**: PR comment posted -Include harness improvements in the same PR as the bugfix — they share the same review context. +If any item is blocked, report which and why. --- -## Quick Reference: Symptom → Fix Path - -``` -SyntaxCheckException / unrecognized syntax → Path A: Grammar/Parser -SemanticCheckException / type mismatch → Path C: Type System / Analysis -Field type wrong (timestamp→string) → Path C: check leastRestrictive / coercion -EXPLAIN shows predicate not pushed down → Path D: Optimizer / Pushdown -Multi-condition query: missing/extra rows → Path D: PredicateAnalyzer nullAs handling -OOM / memory growth over time → Path E: singletons, cache eviction, bounds -NPE in Transport layer → Path E: DI / Guice injection chain -"node must be boolean/number, found XXX" → Path B: OpenSearchJsonContent parse*Value -Regex/function extraction offset → Path B: ordinal vs named references -``` +## Phase 4: Retrospective ---- +- [ ] Symptom in Quick Reference? Add if missing. +- [ ] Classification correct? Fix routing if misleading. +- [ ] Test template worked as-is? Fix if broken. +- [ ] New pattern? Add to Case Index. -## Appendix: Case Index - -| Commit | Bug | Layer | Tests | -|--------|-----|-------|-------| -| `ada2e34` | UNION loses UDT type | Type System | 8 UT + 4 IT | -| `26674f9` | rex capture group index shift | AST/Functions | Multiple UTs | -| `b4df010` | isnotnull not pushed down with != | Optimizer | 2 UT + IT | -| `e045d15` | Multiple filters OOM | Optimizer | 26 output updates | -| `f024b4f` | High-cardinality GROUP BY OOM | Execution | Benchmark | -| `97d5d26` | OrdinalMap cache collision + leak | Execution | — | -| `90393bf` | Non-singleton ExecutionEngine leak | Resource | — | -| `f6be830` | Transport extensions not injected | DI | — | -| `734394d` | Grammar rule typo | Grammar | — | -| `246ed0d` | Float precision flaky test | Test Infra | — | -| `d56b8fa` | Wildcard index type conflict | Value Parsing | 3 UT + 1 IT + 1 YAML | -| `5a78b78` | Boolean coercion from numeric in wildcard queries | Value Parsing | 3 UT + 1 IT + 1 YAML | +Include harness improvements in the same PR. diff --git a/.claude/harness/ppl-bugfix-reference.md b/.claude/harness/ppl-bugfix-reference.md new file mode 100644 index 00000000000..615f91fb75f --- /dev/null +++ b/.claude/harness/ppl-bugfix-reference.md @@ -0,0 +1,148 @@ +# PPL Bugfix Reference + +Consult this file when you need fix-path-specific guidance or test templates. + +--- + +## Fix Path Reference + +### Path A — Grammar / Parser + +1. Update grammar files (must stay in sync): + - `language-grammar/src/main/antlr4/OpenSearchPPLParser.g4` (primary) + - `ppl/src/main/antlr/OpenSearchPPLParser.g4` + - `async-query-core/src/main/antlr/OpenSearchPPLParser.g4` (if applicable) +2. Regenerate: `./gradlew generateGrammarSource` +3. Update AstBuilder: `ppl/.../parser/AstBuilder.java` +4. Test: `AstBuilderTest` + +### Path B — AST / Function Implementation + +1. AST nodes in `core/.../ast/tree/`, functions in `core/.../expression/function/` or `PPLBuiltinOperators` +2. Watch Visitor pattern — sync `AbstractNodeVisitor`, `Analyzer`, `CalciteRelNodeVisitor`, `PPLQueryDataAnonymizer` +3. Test: `verifyLogical()`, `verifyPPLToSparkSQL()`, `verifyResult()` + +### Path C — Type System / Semantic Analysis + +1. `OpenSearchTypeFactory.java`, `Analyzer.java`, `ExpressionAnalyzer.java` +2. Preserve nullable semantics; protect UDT from `leastRestrictive()` downgrade +3. Test: type preservation, nullable propagation, mixed types + +### Path D — Optimizer / Predicate Pushdown + +1. `PredicateAnalyzer.java`, `LogicalPlanOptimizer`, `QueryService.java` +2. Watch `nullAs` semantics; for plan bloat consider `FilterMergeRule` +3. Verify: `EXPLAIN` output + integration test correctness + +### Path E — Execution / Resource Management + +1. `OpenSearchExecutionEngine.java`, `SQLPlugin.java`, `OpenSearchPluginModule.java` +2. Common patterns: cache key collision, memory leak, unbounded growth, non-singleton, DI not injected + +--- + +## Test Templates + +**Unit test** (extend `CalcitePPLAbstractTest`): +```java +public class CalcitePPLYourFixTest extends CalcitePPLAbstractTest { + public CalcitePPLYourFixTest() { + super(CalciteAssert.SchemaSpec.SCOTT_WITH_TEMPORAL); + } + + @Before + public void init() { + doReturn(true).when(settings) + .getSettingValue(Settings.Key.CALCITE_ENGINE_ENABLED); + } + + @Test + public void testBugScenario() { + verifyLogical("source=EMP | where SAL > 1000", + "LogicalFilter(condition=[>($5, 1000)])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"); + } +} +``` + +**Integration test** (extend `CalcitePPLIT`): +```java +public class CalcitePPLYourFixIT extends CalcitePPLIT { + @Override + public void init() throws IOException { + super.init(); + enableCalcite(); + } + + @Test + public void testBugFixEndToEnd() throws IOException { + JSONObject result = executeQuery("source= | "); + verifySchema(result, schema("field", "alias", "type")); + verifyDataRows(result, rows("expected_value_1"), rows("expected_value_2")); + } +} +``` + +**YAML REST test** — place at `integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/.yml`: +```yaml +setup: + - do: + indices.create: + index: test_issue_ + body: + settings: { number_of_shards: 1, number_of_replicas: 0 } + mappings: { properties: { : { type: } } } + - do: + query.settings: + body: { transient: { plugins.calcite.enabled: true } } +--- +teardown: + - do: + query.settings: + body: { transient: { plugins.calcite.enabled: false } } +--- +"": + - skip: { features: [headers, allowed_warnings] } + - do: + bulk: { index: test_issue_, refresh: true, body: ['{"index": {}}', '{"": ""}'] } + - do: + headers: { Content-Type: 'application/json' } + ppl: { body: { query: "source=test_issue_ | " } } + - match: { total: } + - length: { datarows: } +``` + +--- + +## Symptom → Fix Path + +``` +SyntaxCheckException / unrecognized syntax → Path A +SemanticCheckException / type mismatch → Path C +Field type wrong (timestamp→string) → Path C +EXPLAIN shows predicate not pushed down → Path D +Multi-condition query: missing/extra rows → Path D +OOM / memory growth over time → Path E +NPE in Transport layer → Path E +"node must be boolean/number, found XXX" → Path B +Regex/function extraction offset → Path B +``` + +--- + +## Case Index + +| Commit | Bug | Layer | Tests | +|--------|-----|-------|-------| +| `ada2e34` | UNION loses UDT type | Type System | 8 UT + 4 IT | +| `26674f9` | rex capture group index shift | AST/Functions | Multiple UTs | +| `b4df010` | isnotnull not pushed down with != | Optimizer | 2 UT + IT | +| `e045d15` | Multiple filters OOM | Optimizer | 26 output updates | +| `f024b4f` | High-cardinality GROUP BY OOM | Execution | Benchmark | +| `97d5d26` | OrdinalMap cache collision + leak | Execution | — | +| `90393bf` | Non-singleton ExecutionEngine leak | Resource | — | +| `f6be830` | Transport extensions not injected | DI | — | +| `734394d` | Grammar rule typo | Grammar | — | +| `246ed0d` | Float precision flaky test | Test Infra | — | +| `d56b8fa` | Wildcard index type conflict | Value Parsing | 3 UT + 1 IT + 1 YAML | +| `5a78b78` | Boolean coercion from numeric in wildcard queries | Value Parsing | 3 UT + 1 IT + 1 YAML | diff --git a/CLAUDE.md b/CLAUDE.md index 8a61c46bc1c..03f88a60042 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,46 +9,16 @@ OpenSearch SQL plugin — enables SQL and PPL (Piped Processing Language) querie ## Build Commands ```bash -# Full build (compiles, tests, checks) -./gradlew build - -# Fast build (skip integration tests) -./gradlew build -x integTest - -# Build specific module -./gradlew :core:build -./gradlew :sql:build -./gradlew :ppl:build - -# Run unit tests only -./gradlew test - -# Run a single unit test class -./gradlew :core:test --tests "org.opensearch.sql.analysis.AnalyzerTest" - -# Run integration tests -./gradlew :integ-test:integTest - -# Run a single integration test -./gradlew :integ-test:integTest -Dtests.class="*QueryIT" - -# Skip Prometheus if unavailable -./gradlew :integ-test:integTest -DignorePrometheus - -# Code formatting -./gradlew spotlessCheck # Check -./gradlew spotlessApply # Auto-fix - -# Regenerate ANTLR parsers from grammar files -./gradlew generateGrammarSource - -# Run plugin locally with OpenSearch -./gradlew :opensearch-sql-plugin:run -./gradlew :opensearch-sql-plugin:run -DdebugJVM # With remote debug on port 5005 - -# Run doctests -./gradlew :doctest:doctest -./gradlew :doctest:doctest -Pdocs=search # Single file +./gradlew build # Full build (compiles, tests, checks) +./gradlew build -x integTest # Fast build (skip integration tests) +./gradlew :core:build # Build specific module +./gradlew test # Unit tests only +./gradlew :core:test --tests "*.AnalyzerTest" # Single test class +./gradlew :integ-test:integTest # Integration tests +./gradlew :integ-test:integTest -Dtests.class="*QueryIT" # Single IT +./gradlew spotlessCheck # Check formatting +./gradlew spotlessApply # Auto-fix formatting +./gradlew generateGrammarSource # Regenerate ANTLR parsers ``` ## Code Style @@ -125,7 +95,7 @@ plugin (OpenSearch plugin entry point, Guice DI wiring) ## Fixing PPL Bugs -Use the `/ppl-bugfix #` slash command to fix PPL bugs. It dispatches a subagent in an isolated worktree that reads `.claude/harness/ppl-bugfix-harness.md` and follows all phases — from Phase 0 (Triage & Classification) through Phase 4 (Retrospective & Improvement). If a user asks to fix a PPL bug without using the slash command, remind them to use `/ppl-bugfix` or invoke it on their behalf. +Use `/ppl-bugfix #` to fix PPL bugs. It dispatches a subagent in an isolated worktree with a structured harness covering triage, fix, tests, and PR creation. ## Adding New PPL Commands @@ -146,11 +116,11 @@ Follow `docs/dev/ppl-functions.md`. Three approaches: ## Calcite Engine -The project has two execution engines: the legacy **v2 engine** and the newer **Calcite engine** (Apache Calcite-based). Calcite is toggled via `plugins.calcite.enabled` setting (default: off in production, toggled per-test in integration tests). +The execution engine is Apache Calcite-based, toggled via `plugins.calcite.enabled` (default: off in production, toggled per-test in integration tests). - In integration tests, call `enableCalcite()` in `init()` to activate the Calcite path -- Some features (e.g., graphLookup) require pushdown optimization — use `enabledOnlyWhenPushdownIsEnabled()` to skip tests in the `CalciteNoPushdownIT` suite -- `CalciteNoPushdownIT` is a JUnit `@Suite` that re-runs Calcite test classes with pushdown disabled; add new test classes to its `@Suite.SuiteClasses` list +- Some features require pushdown optimization — use `enabledOnlyWhenPushdownIsEnabled()` to skip tests in `CalciteNoPushdownIT` +- `CalciteNoPushdownIT` re-runs Calcite test classes with pushdown disabled; add new test classes to its `@Suite.SuiteClasses` list ## Integration Tests