[nadeem4]

Design: docs/architecture/wave_b.md (design PR #310), section 10 "Learn from corrections", decision D55, row F1 of the PR plan. Builds on Q1 (#337): the correction is filed with Proposal.build(...) + nl2sql.review.queue.propose(ctx, proposal).

What changed

• engine.feedback.correct(trace_id, *, sql=None, plan=None, question=None, note=None, user_context) -> Proposal (nl2sql/feedback/corrections.py, new; api/feedback_api.py only adds the correct method that calls it, record/_propose untouched).
  1. The run: its kept trace gives the question (question overrides) and its one datasource; a run on several datasources is refused. The run is rated down with the note unless it already is (through engine.feedback.record).
  2. A plan fix runs as a pinned plan, run_query(plan=SuppliedPlan(...), execute=False, user_context=corrector), as Wave A's seeding does: run.before, plan.after, the logical validator (with the corrector's scope) and the generator, no model call. A rejected plan is refused with the run's messages.
  3. A SQL fix must pass inspect_verified_sql (exactly one read-only query, the curated config's own check) and every table it reads must be allowed by the corrector's Scope.
  4. The SQL goes through hooks_of(ctx).call("execute.before", ...) with the corrector as the caller and no approvals: a Deny, a hook failure or a NeedsApproval refuses it.
  5. It runs once through the adapter as CuratedAPI.test does (read-only, first batch only, nothing kept, no artifact written). A failure is refused with the database's message; an empty result is allowed.
  6. The proposal: kind="verified_query", origin="correction", change in curated.yaml's shape (id slugged from the question, the question, the SQL, the note or the placeholder as description), tables every datasource.schema.table the fix and the original SQL read, evidence {trace_id, original_sql, ran, row_count, columns}. Nothing is written to a person's file before accept.
• nl2sql feedback correct TRACE_ID (--sql FILE | --plan FILE) [--question TEXT] [--note TEXT] --as ROLE [--user ID] (cli/commands/feedback.py). A plan file is JSON or YAML. A refusal prints the reason and exits 1; no fix or both exits 2.
• Boundary test lists: feedback/corrections.py joins ACCESS_DECISIONS (it asks a Scope), and FeedbackAPI.correct joins WAVE_B_READ_METHODS (a required UserContext caller).

Where the code differs from the doc (small gaps filled; the code on main wins)

• Refusal type. The design names none. CorrectionRefused(ValueError) lives in feedback/corrections.py, with code (no_run, multi_datasource, invalid_plan, not_read_only, denied, hook_denied, needs_approval, failed) and messages. It is a ValueError, so F1 adds no ErrorCode and no top-level export (both are B0's files).
• A run with no trace. The default settings.traces.mode (on_failure) keeps no trace of a successful run, so the run's stored rating is the fallback for the question, datasource and SQL. Neither is no_run.
• Placeholders. A SQL fix with :name placeholders is refused (not_read_only): it runs once with no values; the reviewer adds params on accept.
• tables includes the original SQL's tables, because evidence.original_sql shows that SQL; a caller denied any of them does not see the proposal. propose would complete the fix's tables anyway; F1 passes them itself, as Q1's hand-off asks.
• The rating is recorded before the fix is checked (step 1 of the design), so a refused fix still leaves the thumbs-down; a multi-datasource run is refused before rating.
• Only execute.before is asked (step 4); execute.after is not.

For C2

C2's test_correction_check_is_cost_guarded can call engine.feedback.correct(trace_id, sql=..., user_context=...) and expect nl2sql.feedback.corrections.CorrectionRefused with code == "needs_approval" (over_limit: confirm, the message holds the hook's reason) or code == "hook_denied" (refuse, the message is the hook's Deny reason). The correction asks hooks_of(ctx).call("execute.before", trace_id=..., question=..., user=..., datasource_id=..., sub_query=<id "correction">, plan=<PlanModel or None>, sql=..., params={}, tables=[...], approvals=[]), so the built-in policy.cost applies as soon as the runtime puts it first in that runner. A test needs a kept trace (write one to engine._runtime.traces, as tests/unit/test_corrections.py::_run does) or a stored rating.

Tests

packages/nl2sql/tests/unit/test_corrections.py (new, 25 tests): every test the row names (test_plan_fix_goes_through_validator_and_generator, test_rejected_plan_fix_is_refused_with_the_messages, test_sql_fix_must_be_one_read_only_select, test_sql_fix_on_a_denied_table_is_refused, test_correction_denied_by_an_execute_before_hook_is_refused, test_correction_needing_approval_is_refused, test_failing_fix_is_refused_with_the_database_message, test_multi_datasource_run_is_refused, test_correction_records_a_down_rating, test_nothing_is_written_before_accept), plus test_a_caller_denied_a_table_the_fix_names_does_not_see_it, test_a_caller_denied_a_table_the_original_sql_names_does_not_see_it, the proposal's shape, the no-trace fallback, the caller and one-fix checks, and the CLI. Written first and seen failing (no module). No model is called: a plan fix builds model clients pointed at a dead local address and calls none.

Docs

observability/feedback.md (new section "Correcting an answer"), configuration/review.md (corrections file proposals; evidence; source refs), api/core/public-facade.md (feedback.correct), README.md (the command), and F1's "Built" note in architecture/wave_b.md section 10.

Checks

• tests/unit/test_corrections.py and tests/architecture/test_boundaries.py: 78 passed. Also run locally, as they share the touched files: test_review_queue.py, test_playground_feedback.py, test_feedback_store.py: passed.
• mkdocs build --strict: clean.
• Full unit suite: left to CI (owner rule).

[nadeem4]

Review of F1 (corrections)

On the PR head, tests/unit/test_corrections.py and tests/architecture/test_boundaries.py pass (78 passed). The design steps, names, docs and C2 hand-off match wave_b.md section 10 and D55. I wrote throwaway tests against this branch and reproduced the first three findings below.

1. BLOCKER: a corrector gets the original question and SQL of a run on tables their role is denied. feedback/corrections.py:245-261 checks scope only for the fix's tables. As analyst (salaries denied), engine.feedback.correct("t1", sql="SELECT SUM(total) AS revenue FROM orders", user_context=ANALYST) on a run whose SQL is SELECT SUM(amount) AS pay FROM salaries succeeds. The returned Proposal has target = the run's question and evidence.original_sql = the salaries SQL. The queue then hides the row from that same caller (tables includes salaries), but the return value has already given it to them. Any trace id, or a rating with no trace, works. The multi_datasource refusal (line 95) also names the run's datasources before any scope check.
   Fix: in correct, before rating or reading anything else, require the corrector's Scope to reach the run's datasource(s) and every table the original SQL reads (_check_scope(ctx, ds, original_pairs, user)). If the original SQL can't be parsed (line 267 currently swallows that), refuse, or leave original_sql out of the evidence. Add the test the PR is missing: test_a_caller_denied_the_original_sqls_tables_cannot_correct_it. The existing ..._original_sql_names_does_not_see_it test corrects as ADMIN, so it never covers this path.

2. BLOCKER: re-filing the same fix from a second run swaps in the new run's evidence but keeps the old tables. The proposal id hashes kind, datasource, target and change, so two corrections with the same question and the same fix share an id. review/queue.py propose (the existing.status == "proposed" branch) updates only evidence. Repro: correct t1 (original SQL on orders), then t2 (original SQL on salaries), both as ADMIN with the same fix. The stored row has tables == ["shop.main.orders"] but evidence.original_sql == "SELECT SUM(amount) AS revenue FROM salaries", and engine.proposals.list(user_context=ANALYST) returns it. F1 is the first origin whose evidence carries table-bearing text, so this gap is now live.
   Fix: in propose, when refreshing evidence, also union the tables: existing.model_copy(update={"evidence": proposal.evidence, "tables": sorted({*existing.tables, *proposal.tables})}). Or keep the first evidence. Add a test with two traces.

3. SHOULD-FIX: a refused correction still overwrites the run's rating, even when the caller has no access. _rate_down (line 248) runs before any scope or hook check. Repro: the run is rated up, then ANALYST calls correct(trace_id, sql="SELECT SUM(amount) FROM salaries"). It is refused denied, but the stored rating is now down. Any caller who knows a trace id can flip other users' ratings and inject a note.
   Fix: rate after the run-level scope check from finding 1 (the datasource and the original tables). That keeps step 1's intent ("a refused fix still leaves the thumbs-down") for callers who could see the run.

4. SHOULD-FIX: user-supplied SQL can call any function, and table-valued functions bypass the table scope. inspect_verified_sql (in config/curated.py) blocks only write nodes. That was enough for admin-authored curated.yaml, but F1 runs the check on SQL from any role. Repro on SQLite as ANALYST: SELECT name FROM pragma_table_info('salaries') is accepted (ran, row_count: 2). tables doesn't list salaries, so the analyst also sees the proposal. The same shape on other adapters: Postgres pg_terminate_backend(...), pg_cancel_backend, dblink_exec / dblink(...) (a separate connection that isn't read-only), set_config(...); DuckDB read_csv('/path') / read_text. All of these pass a "one read-only SELECT" check, and default_transaction_read_only doesn't stop most of them. The model path can't reach them because of ALLOWED_FUNCTIONS; this path can.
   Fix (in corrections.py, not the shared curated check): after inspect_verified_sql, walk the tree and refuse as not_read_only:

   • any exp.Anonymous function (sqlglot doesn't know it);
   • any exp.Table whose source isn't a plain identifier (a table-valued function);
   • optionally, any function outside an allow-list derived from ALLOWED_FUNCTIONS.

   Add parametrized cases for pragma_table_info(...) and an anonymous function.

5. NIT: a trailing comment after ; is refused as two statements. corrections.py:256 strips ; only at the very end, so SELECT * FROM orders; -- x gives "found 2" (not_read_only). Strip with sqlglot, or say in the message that only one statement is allowed.

6. NIT: the fallback error path can return raw driver text. corrections.py:212-213 returns str(error) for an exception the adapter didn't wrap, while the frame path uses safe_message. Use the adapter's or the SDK's driver_message (or a generic "the SQL failed") so connection strings or host names can't reach the caller.

7. NIT: a NeedsApproval from run.before / plan.after comes back as invalid_plan. On a plan fix it surfaces through result.errors (_sql_of_plan, line 147) rather than needs_approval. This doesn't affect C2, which is on execute.before, but mapping ErrorCode.NEEDS_APPROVAL to needs_approval would keep the codes truthful.

Invariants all hold: no dialect SQL outside adapters, lazy imports keep import nl2sql light, no os.environ writes, and the CLI uses only engine.feedback.correct. Docs are updated in this PR. The C2 contract (CorrectionRefused.code and the execute.before field names) matches the hand-off.

Verdict: fix first. Findings 1 and 2 are cross-role disclosures of SQL text that D60's scope filter is meant to prevent.

[nadeem4]

Thanks for the review. All seven are fixed in b2e53a6, each with a test written first and seen failing.

1. BLOCKER, the run's scope comes first. _find_run(ctx, trace_id, user) now checks the corrector's Scope before it does anything else: before the multi_datasource refusal and before the down rating. The scope must reach every datasource the run read and every table its SQL read. A datasource that is not configured counts as unreachable. If the run's SQL cannot be parsed, the correction is refused rather than having the SQL dropped from the evidence. These refusals are denied with a message that names no datasource, table or question.
   • Tests: test_a_caller_denied_the_original_sqls_tables_cannot_correct_it (an analyst correcting an admin's run on salaries), test_a_rated_run_on_a_denied_table_cannot_be_corrected (the no-trace path), test_a_run_on_a_datasource_the_caller_cannot_read_is_refused_without_naming_it, test_a_run_whose_sql_cannot_be_read_is_refused.
   • test_multi_datasource_run_is_refused now uses two configured datasources.
2. BLOCKER, refiled tables. In review/queue.py, propose now stores sorted({*existing.tables, *proposal.tables}) alongside the new evidence. It is a one-line change. Test: test_refiling_a_fix_from_another_run_keeps_every_table, which corrects t1 (orders) and then t2 (salaries) with the same fix; the stored tables holds both, and the analyst no longer sees the row.
3. SHOULD-FIX, rating order. The run is rated down only after the run-level scope check passes. The test above also asserts that the up rating and its note are left unchanged.
4. SHOULD-FIX, tables and functions in a SQL fix. This is in corrections.py only; the shared curated check is untouched.
   • A person's SQL fix must read plain tables. Any exp.Table whose source is not an identifier is refused, which covers table-valued functions.
   • Every exp.Func it calls must be on an allow-list:
     • ALLOWED_FUNCTIONS, under the names sqlglot gives them in the datasource's dialect (_known_functions, derived by parsing each one, so no dialect name appears in the engine);
     • plus STRUCTURAL_FUNCTIONS: CAST, TRY_CAST, CASE, IF, EXISTS, and the current date and time.
   • Unknown (Anonymous) functions are refused, and AND/OR are skipped as operators. A refusal is not_read_only.
   • The engine's own rendering of a plan fix is not held to this, since ALLOWED_FUNCTIONS already bounds it.
   • Tests: pragma_table_info('salaries'), read_csv(...), an unknown function, and ROW_NUMBER(). test_sql_fix_with_ordinary_expressions_runs checks that CASE, COALESCE, ROUND, CAST, LOWER, COUNT, IN, NOT, OR and EXISTS still pass.
5. NIT, trailing comment. _one_statement uses sqlglot's tokenizer to drop a trailing ; and the comment after it. Test: test_a_trailing_comment_is_not_a_second_statement.
6. NIT, driver text. An exception the adapter did not wrap is logged, and the caller is told only "the database driver raised ". Test: test_an_unwrapped_driver_error_is_not_shown.
7. NIT, approval codes. A NEEDS_APPROVAL error (or status == "needs_approval") from a plan fix's run now maps to needs_approval. Test: test_a_plan_fix_needing_approval_is_needs_approval.

Docs updated: F1's Built note in wave_b.md, observability/feedback.md (the steps table) and configuration/review.md (the refiled-tables rule).

Checks: test_corrections.py, test_review_queue.py and test_boundaries.py give 117 passed. mkdocs build --strict is clean. I did not run the full suite locally; CI runs it.

[nadeem4]

Follow-up in cac4136. CI on the first push failed one test: test_hooks_modify_only_before_the_validator in tests/architecture/test_hooks_boundary.py. That test reads a runner call only when the runner is named hooks, so hooks_of(ctx).call("execute.before", ...) did not count as one. It is now hooks = hooks_of(ctx); hooks.call(...), as evaluation/seed.py does. Locally, tests/architecture/ and test_corrections.py give 93 passed.