Rifqi Hafizuddin Claude commited on
Commit ·
641b6d4
1
Parent(s): 9b0419a
[NOTICKET] docs(plans): planner query-shape fixes and audit follow-ups handoff
Browse filesRCA of the "selected bare while group_by is present" failures of 2026-11-09,
the F1-F5 fix plan (not built) and the audit follow-ups, for handoff.
Co-Authored-By: Claude <noreply@anthropic.com>
docs/plans/PLANNER_SHAPE_FIXES_HANDOFF_2026-11.md
ADDED
|
@@ -0,0 +1,141 @@
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1 |
+
# Planner query-shape fixes and audit follow-ups: handoff
|
| 2 |
+
|
| 3 |
+
<!-- doc-status -->
|
| 4 |
+
> 💡 **PROPOSAL, 2026-11-10. Nothing here is built.** Handoff from Rifqi's session to Sofhia. Plan F1–F5 was shown to Rifqi; build approval, the dictionary write and every paid run are still his to give. Status legend: ⬜ not started · 🔄 in progress · ✅ done · ⛔ blocked · 🔎 verify · ⏸️ deferred. Client figures stay in the local answer key (`tests/fixtures/claude_test_answer_key.md`), never in this file.
|
| 5 |
+
|
| 6 |
+
Branch `pr/28` at `4e83532e`. Read first: `CLAUDE.md` (§2, §5.22–5.24, §6, §7), `CONTEXT.md` bullet "Measures computed from summed inputs", `docs/plans/RATIO_MEASURES_REQUIREMENTS.md` §9–§10.
|
| 7 |
+
|
| 8 |
+
## 1. What went wrong
|
| 9 |
+
|
| 10 |
+
Two demo turns on 2026-11-09 hit the IR validator error "selected bare while group_by is present" (`src/query/ir/validator.py:107-128`):
|
| 11 |
+
|
| 12 |
+
| Turn | Attempt | Plan | Outcome |
|
| 13 |
+
|---|---|---|---|
|
| 14 |
+
| Q4 "kelompokkan unit berdasarkan PA…", analysis `06f9cf31` | 1 | `group_by [eqm_no]`; bare: `section` (pinned by `section = 'OB LOADER'`), `pa`, `total_inpr_hour`, `breakdown_duration_total`; then `analyze_segmentation` | Rejected. The error named only `section`, the first offender |
|
| 15 |
+
| | 2 | Three `retrieve_data`, each `group_by [section, model, eqm_no, pa]` + `COUNT(*)` + a band filter on raw `pa`; no segmentation | Passed every check. Answer counted unit-month rows as units |
|
| 16 |
+
| Compare "bandingkan PA OB LOADER IPR, LAT, SDJ 2026", analysis `0afacc01` | 3 | `group_by [site_id]`, `avg(pa)`, counts; bare: `section` (pinned) | Rejected on the last attempt, turn refused |
|
| 17 |
+
|
| 18 |
+
## 2. Root cause
|
| 19 |
+
|
| 20 |
+
On 2026-11-09 11:12 the dictionary description of `pa` on the demo catalog (monthly and weekly lagging tables) was changed to:
|
| 21 |
+
"… Untuk lebih dari satu periode atau unit: jumlahkan kedua kolom jam dulu, lalu pakai rumus ini. Jangan rata-ratakan pa. …", with `aggregation` set to null.
|
| 22 |
+
|
| 23 |
+
- The QueryIR cannot express "sum two columns, then divide" (`src/query/ir/models.py`: aggregates are count, count_distinct, sum, avg, min, max).
|
| 24 |
+
- The PA design (RATIO_MEASURES_REQUIREMENTS §9, M4) needs the planner to write `avg(pa)`; code then computes it from summed hours.
|
| 25 |
+
- So the planner is told, as data, not to write the only shape the system supports.
|
| 26 |
+
|
| 27 |
+
Evidence from the stored EMA chat transcripts (`eval/chat_sim/results/transcript_ema_*.json`, planner attempts are the `llm_calls` labelled `planner…`):
|
| 28 |
+
|
| 29 |
+
| Planner attempts on PA | Wrote `avg(pa)` grouped in the query |
|
| 30 |
+
|---|---|
|
| 31 |
+
| First attempts before the change | 9 of 11 (5 of 7 with no description, 4 of 4 with the old "Rata-ratakan" text) |
|
| 32 |
+
| First attempts after | 4 of 8 |
|
| 33 |
+
| After, attempts whose `assumptions` quote the instruction, unprompted | 0 of 5 |
|
| 34 |
+
| After, first attempts that do not quote it | 4 of 4 |
|
| 35 |
+
|
| 36 |
+
The counts are small; the planner's own `assumptions` carry the claim. Confound: both Q1 runs after the change still averaged, because the question says "rata-rata". The five improvised shapes: raw rows → `analyze_segmentation`; raw rows → `analyze_aggregate` (twice); 828 raw rows to the assembler (the BIN follow-up); bare `pa` + both hour columns under `group_by eqm_no`.
|
| 37 |
+
|
| 38 |
+
**Relation to Checks 8m/8n.** The Q4 wrong answer is the 8m/8n harm (period rows counted as entities) in a shape neither sees: `_raw_period_rows` (`src/agents/planner/validator.py:692`) returns False as soon as `group_by` is non-empty, 8m also needs `order_by` + `limit`, 8n needs `analyze_segmentation`. The bare-select reject itself is the generic SQL-shape check.
|
| 39 |
+
|
| 40 |
+
**Across all stored logs** (69 logs, 99 `planner validation failed` lines, 716 planning calls):
|
| 41 |
+
- bare-select: 9 failures in 6 logs. 2 on the demo catalog (both name the pinned `section`), 7 older ones name a measure.
|
| 42 |
+
- 631 calls right first time, 82 recovered after a retry, 3 exhausted. All 3 exhausted turns ended on a bare-select error after two different errors.
|
| 43 |
+
- Habit: grouped queries that select a filter-pinned column put it in `group_by` 20 times and left it bare 2 times.
|
| 44 |
+
|
| 45 |
+
**Why the evals missed it.** The planner eval fixture (`eval/planner/catalog_fixture.py`) describes `pa` as "aggregate with: avg" and has no hour columns, so nothing binds and the conflict cannot occur. The eval scores the final validated plan, never the attempts. Its last result is 2026-11-03, before the PA validator changes. Chat runs were graded on answers, not attempts.
|
| 46 |
+
|
| 47 |
+
## 3. Fixes F1–F5
|
| 48 |
+
|
| 49 |
+
Build order as listed. One home per fix (§5.22). No change to `planner.md`.
|
| 50 |
+
|
| 51 |
+
| # | Fix | Where | Spec |
|
| 52 |
+
|---|---|---|---|
|
| 53 |
+
| F1a | Code-made method line | `src/agents/planner/inputs.py` `_dictionary_line` | For a column whose `measure_method.kind == "ratio"` (bound each turn by `bind_computed_measures`), emit `aggregate with: avg (computed from summed inputs)` as the **first** part of the line, so the 300-char cap (`_MAX_DICTIONARY_CHARS`) cannot cut it. No line change for any other column. Check that `measure_method` survives into the object `_dictionary_line` receives (it is `exclude=True` on `Column`; `CatalogSummary.from_catalog` may need to carry it) |
|
| 54 |
+
| F1b | Reword the `pa` description (data) | demo catalog, user-scope `data_catalog` row `0ec28b61`, both lagging tables | **Shared-DB write on a Go-owned table: Rifqi's explicit go, backup first** (pattern: `tests/scripts/fix_pa_dictionary_2026_11_09.py` on Rifqi's machine). Proposed text: "Physical Availability (%) = (total_inpr_hour − breakdown_duration_total) × 100 / total_inpr_hour. Untuk beberapa periode atau unit, agregasikan pa dengan avg: sistem menghitungnya dari total kedua kolom jam. Makin tinggi makin baik." Only new analyses pick it up |
|
| 55 |
+
| F2 | Pinned bare select joins `group_by` | `src/query/ir/repair.py` (new step, logged as a `Repair`) | A `kind: column` select not in `group_by`, whose column has a filter `=` or `in` with exactly one value, is appended to `group_by`. **Only when `group_by` is already non-empty**: on an ungrouped aggregate it would turn a one-row "no data" result into zero rows. Never for unpinned columns (a unit can sit in 2–3 sections, so adding one changes the grain). Runs inside validation, so after the scope hook |
|
| 56 |
+
| F3 | Bare measure wrapped in its declared aggregate, fail-open | `src/agents/planner/validator.py`, same pattern as `_convert_ratio_aggregates` | Under a non-empty `group_by`, a bare numeric select whose column declares a method (dictionary `aggregation` in sum/avg/min/max/count, or ratio-marked → `avg`) becomes `{"kind": "agg", "fn": <declared>}` with the alias kept. Build on a deep copy, run the whole `validate` on the copy, swap in only if it passes; otherwise the normal error stands and a warning is logged. Runs before `_mark_computed`, so a wrapped `pa` gets its `computed` block. Skipped on `plain_average` turns only for the ratio default, not for dictionary aggregates |
|
| 57 |
+
| F4 | Bare-select error names everything | `src/query/ir/validator.py:107-128` | List every offender by column **name** (id in brackets). Numeric measure: "wrap it in an aggregate (the catalog's `aggregate with`); a measure never goes in group_by". Other column: "add it to group_by (this splits groups if it varies within one) or drop it from select". Remove the `fn: sum` example for string columns |
|
| 58 |
+
| F5 | Grouping by a measure is not grouping | `src/agents/planner/validator.py`, 8m family (call it 8p) | Reject a `retrieve_data` on a table with period columns (`_period_column_ids`) when `group_by` holds a column of data type decimal/float (not a period, not `*_id`) and no period column is selected. Test the **data type**, not `classify`: `is_bad_actor` is an int flag with `classify: measure` and 30 stored queries group by it legitimately. Hard reject like 8m/8n, because the fallback answer is wrong but looks right. Message in the 8m style |
|
| 59 |
+
|
| 60 |
+
### Checks already run (no model, read-only demo DB)
|
| 61 |
+
|
| 62 |
+
Prototypes live in two local scripts on Rifqi's machine (`tests/` is gitignored): `tests/scripts/scan_bare_select_2026_11_10.py` and `tests/scripts/check_bare_select_rca_2026_11_10.py`. Ask Rifqi for them, with the two 9 Nov transcripts.
|
| 63 |
+
|
| 64 |
+
- Compare attempt 3 with `section` moved into `group_by`: IR validator + M4 + demo DB return the three sites, equal to the key. **Not proven:** that the rest of `PlannerValidator` passes, because the scope hook was not run offline. Cover it with a local test that runs the hook, then the validator, on the stored attempt.
|
| 65 |
+
- Q4 attempt 1 with `section` moved and each bare measure wrapped in its declared aggregate: one row per unit, bands equal to the key's summed-hours bands.
|
| 66 |
+
- Replay of 691 distinct stored plans: F2 touches only the two incident plans; F5 (decimal type) flags only the Q4 retry.
|
| 67 |
+
|
| 68 |
+
### Verification (§7A)
|
| 69 |
+
|
| 70 |
+
- Local tests per fix, including: F2 not applied to an ungrouped aggregate or an unpinned column; F3 fail-open and no-declared-method cases; F5 allows `is_bad_actor`, allows a period column in select, rejects the Q4 retry shape; F1a line order and the 300-char cap.
|
| 71 |
+
- Differential replay of all stored plans, old vs new validator (pattern: `tests/scripts/replay_m7prime_2026_11_09.py`): 0 plans that passed before may fail after, except the Q4 retry.
|
| 72 |
+
- Full suite against the last known count **1472 passed, 8 skipped**; `ruff check src/`; `import main`.
|
| 73 |
+
- Docs in the same change: `CONTEXT.md` bullet, RATIO_MEASURES_REQUIREMENTS (new §11), `docs/README.md` index row for this file (not added yet).
|
| 74 |
+
|
| 75 |
+
### Paid runs (each needs Rifqi's go, one at a time)
|
| 76 |
+
|
| 77 |
+
| Run | Size | Pass |
|
| 78 |
+
|---|---|---|
|
| 79 |
+
| Planner-only on the reworded text, live catalog: Q4 ×4, compare ×3, PA trend per month ×2, Q1, MTTR trend, one English Q4 | 12 calls, about 360k tokens | 11 of 12 first attempts valid with `pa` aggregated in the query; MTTR still a plain `avg` |
|
| 80 |
+
| 7-question `eval.chat_sim` `ema_demo` run plus the compare turn | about 390k tokens | Q4 and compare match the key; Q1, Q2, Q3, Q5, Q7 unchanged |
|
| 81 |
+
|
| 82 |
+
### Use-case coverage (RATIO_MEASURES_REQUIREMENTS §4)
|
| 83 |
+
|
| 84 |
+
| Use case | Mechanism |
|
| 85 |
+
|---|---|
|
| 86 |
+
| U2 bands | F1, then F3; F5 and 8n catch the rest |
|
| 87 |
+
| U4 compare | F1, F2, plus the existing M7′ conversion |
|
| 88 |
+
| U5 trend per month | F1; not run since the text change, so it is in the paid gate |
|
| 89 |
+
| U7 MTTR and other averaged measures | No ratio mark → unchanged; in the paid gate |
|
| 90 |
+
| U1, U3, U11 | Existing path; F1 keeps the planner on it |
|
| 91 |
+
|
| 92 |
+
### Not covered, on purpose
|
| 93 |
+
|
| 94 |
+
- Band counts written as `count_distinct(eqm_no)` with a `pa` filter (a unit counted once per band it ever touched). None in 691 stored plans; it also has a legitimate reading ("units that ever fell below 70").
|
| 95 |
+
- Raw rows **with** the period column fed to `analyze_segmentation`: a deliberate unit-month banding is a valid question.
|
| 96 |
+
- The BIN follow-up widening (RATIO_MEASURES_REQUIREMENTS §10.8). Its 828-row plan quotes the same dictionary instruction, so F1 may remove that half; the router rewrite half needs its own RCA.
|
| 97 |
+
|
| 98 |
+
## 4. Audit follow-ups
|
| 99 |
+
|
| 100 |
+
Delta on the 3 Nov audit (`docs/plans/AUDIT_FIXES_2026-11.md`). Covered: guardrails, planner retry loop, router, query defenses, evals. Not covered: a line-by-line re-read of `planner.md` / `assembler.md` / `help.md`, the report generator, knowledge extraction.
|
| 101 |
+
|
| 102 |
+
| # | Finding | Evidence | Proposed fix | Home | Priority | Needs |
|
| 103 |
+
|---|---|---|---|---|---|---|
|
| 104 |
+
| A1 | Dictionary text reaches the planner as an unchecked instruction | §2 | F1 | — | In F1 | — |
|
| 105 |
+
| A2 | The planner eval cannot see this class | Fixture `pa` = "aggregate with: avg", no hour columns, nothing binds; scores the final plan; last result 2026-11-03 | Add `total_inpr_hour` and `breakdown_duration_total` to the EMA fixture tables so the PA block binds; add cases whose `pa` description says "don't average"; report first-attempt pass rate next to the final rate. The 8+ cases asserting `select_agg: avg` stay valid (F3/M4 keep `fn: avg`) | `eval/planner/` | High | A paid run for the new result JSON (§6.10) |
|
| 106 |
+
| A3 | Retry is blind and fail-fast | Validator raises on the first error; `build_planner_prompt` passes error text only, citing ids and positions of a plan the model no longer sees. 3 of 3 exhausted turns ended on bare-select | F2–F4 remove that terminal error. Later, as its own change: errors name columns everywhere; consider including the rejected plan in the retry prompt | `planner/service.py`, `planner/prompt.py` | Medium | Own check before building |
|
| 107 |
+
| A4 | "Column not in table" is the joint-largest failure family | 16 of 99 failures, weekly-table ids used on the monthly table; `bb817f9b` Q4 survived two of them on its last attempt | Twin-table fix C1 in `docs/plans/PLANT_TEST_FIXES_2026-11.md` (still a proposal) | catalog render | Medium | Rifqi's decision on C1 |
|
| 108 |
+
| A5 | 8m/8n decide "raw rows" by syntax | §2 | F5 | — | In F5 | — |
|
| 109 |
+
| A6 | The input guard screens only the chat message | Objective and business questions, dictionary descriptions, approved knowledge entries, document excerpts and cell values reach prompts unscreened, inside `<data>` with a "data, never instructions" rule (planner rule 8, assembler rule 5, `report_summary.md`). The 3 Nov guard test (20/20) covered direct messages only | Add indirect cases to the guard smoke: a hostile string in the objective, in a dictionary description, in a cell value, in a document excerpt; assert the answer does not follow it | local test + a small paid run | Medium | Go for the paid run. Any guard change is §6.3 |
|
| 110 |
+
| A7 | Rate limit keyed by remote address | `slowapi` `get_remote_address`; uvicorn is started without trusted-proxy settings (`Dockerfile:36`), so behind the HF proxy all users probably share one 30-per-minute bucket. Not verified on the Space | Verify on the Space first (two clients, read the 429 behaviour). If confirmed, key on the forwarded address | `middlewares/rate_limit.py`, Dockerfile | Low; check before a multi-user demo | Deploy change → Rifqi |
|
| 111 |
+
| A8 | Router logs the intent only | `orchestration.py` logs `intent`; `group_hint` is passed on and never logged | Add `group_hint` and `confidence` to the "intent classified" log line | `agents/orchestration.py` | Low | — |
|
| 112 |
+
| A9 | Router rewrites Indonesian into English; planner and assembler see only the rewrite | Seen again 9 Nov ("Group units by PA into three bands…"); open P1 from the 3 Nov audit | Prompt change → `eval.intent` per §7B | `intent_router.md` | Low, known | Eval run |
|
| 113 |
+
| A10 | Guard runs before the router in series | About 1.7 s and 735 tokens per turn | Run guard and router concurrently, discard the router result on a block | `agents/chat_handler.py` | Low | Guard path → §6.3, ask first |
|
| 114 |
+
| A11 | No authentication; `user_id` comes from the request body | `middlewares/service_auth.py` is parked by decision (DEV_PLAN #37, #43) | None | — | No action | — |
|
| 115 |
+
|
| 116 |
+
Checked and fine: the five query defense layers (a non-Postgres source is refused loudly, `query/executor/db.py`), PII sample suppression in the planner catalog, the help endpoint takes no free text, a guard outage is logged (`input_guard_fail_open`), blocked turns still flush traceability.
|
| 117 |
+
|
| 118 |
+
## 5. Rules for whoever builds this
|
| 119 |
+
|
| 120 |
+
- Ask Rifqi before any paid model run; run them one at a time. Smokes with `--limit` are fine.
|
| 121 |
+
- Never open or edit `.env`. No DDL. The only DB write in this plan is F1b, and it needs Rifqi's go.
|
| 122 |
+
- `tests/` stays local and uncommitted.
|
| 123 |
+
- Commit or push only when Rifqi says. Branch `pr/28`, push `refs/heads/pr/28:refs/pr/28`, rebase on reject.
|
| 124 |
+
- Never-throw seams stay never-throw; F3 is fail-open by design.
|
| 125 |
+
- Progress table after every step (§7G).
|
| 126 |
+
|
| 127 |
+
## 6. Tracker
|
| 128 |
+
|
| 129 |
+
| Item | Owner | Status |
|
| 130 |
+
|---|---|---|
|
| 131 |
+
| F1a code line | Sofhia | ⬜ |
|
| 132 |
+
| F1b dictionary reword | Rifqi (go) | ⬜ |
|
| 133 |
+
| F2 pinned select | Sofhia | ⬜ |
|
| 134 |
+
| F3 declared aggregate | Sofhia | ⬜ |
|
| 135 |
+
| F4 error message | Sofhia | ⬜ |
|
| 136 |
+
| F5 group-by-measure reject | Sofhia | ⬜ |
|
| 137 |
+
| Planner-only gate, 7-question run | Rifqi (go) | ⬜ |
|
| 138 |
+
| A2 eval fixture | Sofhia | ⬜ |
|
| 139 |
+
| A6 indirect-injection smoke | Sofhia | ⬜ |
|
| 140 |
+
| A8 router log | Sofhia | ⬜ |
|
| 141 |
+
| A3, A4, A7, A9, A10 | Rifqi decides | ⏸️ |
|