[nadeem4]

Design: docs/architecture/wave_b.md (Wave B, release 0.8), row Q1 of the PR plan (section 16); section 5 and decisions D41, D42, D43, D60 and D61.

Rebased onto main after K1 (#336) and C1 (#332) merged. Q1 uses K1's glossary appliers (nl2sql/knowledge/apply.py) and write_glossary.

What changed

The queue (nl2sql/review/queue.py)

• propose(ctx, proposal) is how every track files a finding. It handles three cases:
  • Identity: the same change updates the open row's evidence. An accepted or rejected change is never proposed again.
  • Supersede: a new change for the same kind, datasource and target supersedes the open row.
  • Revive: a superseded row that is proposed again is revived.
• It publishes proposal.created when a row is new or revived.
• A caller sees a proposal only when their Scope reaches its datasource and every name in tables. get of a hidden proposal is NOT_FOUND.
• Deciding needs by with an admin role.

Accept and reject (nl2sql/review/accept.py)

• Accepted items are grouped per file.
• Before the first write, accept takes every lock (accept:<real path>, waiting up to 10 s for another caller), re-reads each item as still proposed, and checks every file's hash. A call refused by those checks writes nothing.
• Each file's items are recorded as accepted (stored, decided, proposal.decided published) under the locks, right after that file's write. reject takes the same locks. So an id is accepted only once, and if a later file's write fails, the items already written stay accepted and the rest stay proposed.
• Glossary kinds go through K1's apply_proposal and write_glossary, then the knowledge is reloaded.
• verified_query items run the curated checks on the edited section, then write_curated, then Runtime.replace_curated, then the curated index refresh. The refresh goes through rebuild_index(curated_only=True), which calls refresh_curated.
• An item that fails its checks stays proposed and is listed in refused.
• Accept warns when an earlier knowledge source shadows an item it wrote.
• proposal.decided is published for each decision; the audit observer keeps it.

The curated file (nl2sql/review/curated_file.py)

• curated_path(config) returns None when the section is inline, absent, or the config was built in code.
• canonical_curated gives the canonical text, and write_curated(path, curated, expected_sha256) writes it: atomically, through symlinks, refusing with FILE_CHANGED when the file moved since it was loaded.
• Runtime.curated_sha256 is loaded_sha256(config): the file is read once at build, hashed and parsed. When it no longer holds the section the config loaded, the hash is STALE, so accept refuses with FILE_CHANGED instead of writing over the edit. Runtime.replace_curated(curated, *, sha256) swaps the section and its hash.

The review file (nl2sql/review/review_file.py): render writes one, read reads it back. export and apply are built on them.

The facade: engine.proposals.list, get, accept, reject, export and apply are built (api/proposals_api.py). current(id, *, user_context) is new: what a proposal would replace, as the caller may see it. apply replaces each item's change with the file's; accept(edits=...) still merges.

Thumbs-up

• engine.feedback.record(..., rating="up") files a verified_query proposal. Its tables come from inspect_verified_sql, and its origin is thumbs_up. If filing fails, the failure is logged and the stored rating is still returned.
• feedback/proposals.py now only builds the change: skip_reason and verified_query.
• build_proposals, render_proposals, write_proposals and nl2sql curated proposals export are deleted.

Retention: engine.storage.purge() returns a proposals count: decided rows older than settings.review.max_age_days. nl2sql storage purge prints it.

CLI: nl2sql review list, show, accept (with --all), reject, export and apply. Each takes --as and --user, through K1's caller.

Where the code differs from wave_b.md (small gaps filled)

• Deciding under allow_all: any caller may decide, as K1's reload already does. There are no roles to hold, and the CLI's --as admin is refused under allow_all.
• AcceptReport.rejected is new (a field added to B0's model): it lists the proposals apply rejected.
• apply rejects per item. It makes one accept call, but rejects each item with its own reason, so it does not make the single reject call the design describes.
• Unknown ids:
  • accept lists an unknown or already decided id in refused and does not raise.
  • reject refuses the whole call with NOT_FOUND, because it returns only a list.
• Visibility checks tables only. It checks the datasource and the tables names. It does not check text mentions the way K1's knowledge filter does. propose completes tables from the target and change (named_tables), so a missing name cannot leak a denied table.
• engine.proposals.current is a new read method (it is in the boundary test's D60 list). It replaces logic the CLI would otherwise hold, and the playground and REST will need it in Wave D.
• The curated hash is taken when the runtime is built (components()), not inside the config loader. That keeps the loader and config/models.py untouched (D59).
• The index refresh runs through rebuild_index(..., curated_only=True), so it holds the index lock. A datasource it cannot refresh (for example, one not indexed yet) becomes a warning, not a failure.
• Accept's lock waits up to 10 s for another accept, so two accepts in one engine both land. A second engine that loaded the file earlier gets FILE_CHANGED.
• A ${...} reference in curated.yaml reads as stale. The parsed file is compared with the interpolated section, so a file that holds one refuses every accept. This is documented.
• review/queue.py joins ACCESS_DECISIONS in the boundary test.
• The design row lists observers/audit.py. It needed no change: B0 already marked proposal.decided as audited in the catalog.

Tests (written first)

• New: tests/unit/test_review_queue.py (26). It has every test the row names, plus the CLI and the review fixes (listed after the named tests). Named tests:
  • test_accept_needs_an_admin_role
  • test_accept_verified_query_writes_curated_yaml_and_refreshes
  • test_accept_runs_the_curated_checks_first
  • test_accept_refuses_an_inline_curated_section (also: no curated section, and no yaml knowledge source)
  • test_accept_refuses_a_changed_file
  • test_concurrent_accepts_do_not_lose_a_write (threads in one engine, and two engines)
  • test_accepting_many_writes_each_file_once
  • test_accept_warns_when_a_source_shadows_it
  • test_rejected_change_is_not_proposed_again
  • test_new_change_supersedes_the_open_proposal
  • test_proposals_list_hides_denied_tables
  • test_get_of_a_hidden_proposal_is_not_found
  • test_thumbs_up_creates_one_proposal_per_change
  • test_review_file_round_trip
  • test_apply_reports_unknown_ids
  • test_decided_proposals_are_purged
  • test_curated_proposals_export_is_gone
  • plus test_review_commands_list_show_accept_and_reject
  • review fixes:
    • test_the_same_proposal_accepted_twice_at_once_is_accepted_once and test_a_reject_waits_for_an_accept_in_flight. Both are deterministic: the first call is held inside its decision step, and the second is at the lock before it is released.
    • test_a_failed_second_write_keeps_the_first_files_decisions
    • test_curated_yaml_edited_before_the_engine_was_built_is_not_overwritten
    • test_proposal_tables_cover_every_table_its_change_names
    • test_current_value_is_shown_only_when_the_caller_may_see_it
    • test_a_failed_proposal_never_fails_the_rating
    • test_apply_replaces_a_change_rather_than_merging
• Architecture: test_only_accept_writes_a_persons_file in tests/architecture/test_boundaries.py.
• Updated:
  • test_wave_b_shapes.py: the Q1 stubs are removed.
  • test_readme_cli_drift.py
  • test_trace_result_stores.py and test_phase2_events.py: the proposals purge count.
  • cli/test_config_flag.py: checks review list in place of the deleted command.
• Deleted: test_curated_proposals.py, whose subject is deleted.

Docs

• New: configuration/review.md (the queue, who sees and who decides, the facade, accept as the only writer, the review file, the CLI), with its nav entry.
• Updated:
  • configuration/curated.md: proposals go through the queue, and accept rewrites curated.yaml canonically (a warning that comments are not kept).
  • observability/feedback.md
  • architecture/curated_answers.md: the writer sentence now reads "Nothing writes curated.yaml but a person's accept", and the old export section is marked as replaced.
  • architecture/invariants.md: the new invariant Only an Accept Writes a Person's File.
  • observability/events.md, configuration/storage.md, api/core/public-facade.md, configuration/knowledge.md, and the README's CLI reference.
  • architecture/wave_b.md: the Q1 Built note under the PR plan.

Checks

• Locally, against this worktree's code: tests/architecture/test_boundaries.py and the touched or related files: test_review_queue, test_wave_b_shapes, test_readme_cli_drift, test_trace_result_stores, test_phase2_events, cli/test_storage_purge, cli/test_feedback_command, test_feedback_store, test_cli_curated_validate, test_cli_curated_test, test_curated_startup, test_knowledge_sources, test_cli_knowledge, test_playground_feedback and test_index_curated_refresh. Result: 257 passed, 2 skipped.
• mkdocs build --strict: OK.
• The full suite runs in CI.
• No model call is made, no gold data is touched, and no .env is read.

For later rows (exact names)

• The tables rule, for F1, P1 and L2: a proposal's tables must list every datasource.schema.table its target and change name. A caller sees a proposal only when their scope reaches all of them (D60). propose completes the list from the target and change (named_tables: a table, a column's table, both sides of a join, a term's tables and its columns' tables, and the tables a verified query's SQL reads). Pass tables yourself anyway, and add a test that a caller denied one of them does not see the proposal.

• F1 (corrections):

  • Build Proposal.build(kind="verified_query", origin="correction", change={id, description, questions, sql}, tables=table_names(ctx, ds, inspect_verified_sql(sql, dialect).tables), ...) and file it with nl2sql.review.queue.propose(ctx, proposal).
  • Accept already runs the curated checks and writes curated.yaml.
  • F1 edits api/feedback_api.py after this PR. record now calls self._propose(stored).

• P1 (profiler): build join, column and table proposals the same way, with origin="profiler" and evidence counts, and call propose. Use table_names for tables. Re-profiling updates an open row's evidence, and never revives a rejected one.

• K3: the curated refresh is the call rebuild_index(ctx, datasource_ids=..., curated_only=True) in _refreshed() in nl2sql/review/accept.py. Rename it to refresh there.

• L2: save_choice files a term proposal (target = the term's name, tables from the term) through propose. Accept replaces an existing term of the same name (K1's apply_change). nl2sql review show already shows the current term beside the proposed one.

• Shared names:

  • nl2sql.review.queue: caller, require_admin, visible, who, split_table.
  • nl2sql.review.accept: accept(ctx, ids, by, edits), reject(ctx, reasons, by).

[nadeem4]

Review of Q1 (#337)

Rebase needed. #336 (K1) squash-merged as 9092eacd, so GitHub now shows this PR as CONFLICTING. The squash's tree is identical to 949f6376, the last K1 commit on this branch. That makes the rebase mechanical: git rebase --onto origin/main 949f6376 feat/wave-b-q1. I tried it in a scratch worktree and it applies cleanly, leaving the three Q1 commits with a tree identical to 7f5d286b.

Tests run locally, on the Q1 head with K1's entry point registered: test_review_queue, test_wave_b_shapes, test_readme_cli_drift, test_trace_result_stores, test_phase2_events, cli/test_config_flag and architecture/test_boundaries. Result: 173 passed. I reproduced findings 1 to 3 with throwaway tests (sketched below). None of them is in the PR.

The admin check, scope visibility (list, get, export, --all), reject/apply needing an admin, and the protected-path check on export all look right. So do the AST guard on the two writers, lazy imports (import nl2sql stays light), and the absence of dialect SQL and os.environ writes. The docs are updated in the same PR.

Findings

1. BLOCKER: the decision is recorded after the lock is released, so one id can be accepted (and written) twice. review/accept.py:84-90

   • The for label, applied in written: loop runs put_proposal, decide_proposal("accepted") and _published, and it sits outside the with ExitStack() as held: block.
   • A second accept of the same id can take the freed lock before the first one has marked it. Its _still_proposed (the f1f0930 fix) then still sees proposed, and it applies and writes again.
   • A concurrent reject has the same problem: it never takes the lock, so a rejected row can be flipped to accepted by an accept that passed its re-check earlier.
   • Repro: wrap state.decide_proposal so the first call waits on an event. Thread 1 accepts a column proposal and stops inside the gate. The main thread then accepts the same id. Both reports contain it in accepted, and both have files_written == ["glossary.yaml"].
   • test_the_same_proposal_accepted_twice_at_once_is_accepted_once passes only by timing.
   • Fix: record the decisions (put, decide, publish) inside the with, right after each file's write. Make reject take the same locks, or re-check the status atomically in decide_proposal. Then add the gated test above as a regression test.

2. SHOULD-FIX: "a refused call writes nothing" does not hold when the second file's write fails. review/accept.py:79-82

   • The glossary is written and reloaded first. If _write_curated then raises (NOT_WRITABLE from a PermissionError, or any other OSError), the exception leaves the with. glossary.yaml has been rewritten, but none of its items is marked accepted.
   • Repro: monkeypatch accept_module.write_curated to raise ReviewError(NOT_WRITABLE), then accept [column, verified_query]. It raises, glossary.yaml has changed, and the column proposal is still proposed.
   • Fix: the same move as finding 1. Decide each file's items as soon as that file is written, then let the second failure propagate. Also correct the module docstring and review.md: a call is refused as a whole only for the pre-write checks.
   • A cheaper alternative is to write the curated file first, since it is the one more likely to fail. That only narrows the window; it does not close it.

3. SHOULD-FIX: the curated hash is taken at runtime build, not when the section was loaded, so an edit made in between is silently overwritten. runtime.py:135-142

   • curated_sha256 = file_sha256(path) hashes the file as it is when NL2SQL(config) runs, while self.curated is what load_config parsed earlier.
   • Repro: config = _project(tmp), then add a query by_hand to curated.yaml, then NL2SQL(config) and accept a verified query. Accept returns no FILE_CHANGED, and by_hand is gone from the file.
   • With the CLI the window is small. With the SDK (build a Config, construct the engine later, or several engines from one Config) it is unbounded. Losing a person's edit is exactly what D42's hash check exists to prevent.
   • Fix (keeps D59, no change to the loader):
     • At build, parse the file and compare it with config.curated, re-parsing the file the way the loader parsed it (or comparing canonical forms).
     • If they differ, store a sentinel hash (for example "stale"), so accept refuses with FILE_CHANGED and the fix "reload the engine".
     • Add the repro as a test.

4. SHOULD-FIX (for F1, P1 and L2): a proposal with empty tables is visible to everyone who reaches its datasource. review/queue.py:79-83

   • visible checks only the listed tables, and the PR's own test files a term proposal with tables=().
   • A term or join whose change/target names a denied table would then be shown to a caller who cannot read that table. That breaks D60 ("every table it names").
   • Fix: either make propose refuse a glossary-kind proposal whose tables is empty while its target or change names a table, or reuse K1's knowledge filter on change/target in visible. At minimum, state in the "For later rows" notes and in review.md that tables must list every table the change mentions, and give P1 and L2 a test for it.

5. NIT: review show prints the existing curated query with the same id without a scope check. cli/commands/review.py:85-90

   • The proposal itself is visible, but _current dumps whatever query in curated.yaml has that id, SQL included. That query can read tables the caller is denied. (Thumbs-up ids are slugs of the question, so they collide easily.)
   • Fix: show it only when its tables (inspect_verified_sql, then table_names) pass visible for the caller. Otherwise print "(an existing query you cannot see)".

6. NIT: a failure while filing the thumbs-up proposal fails feedback.record after the rating was stored. api/feedback_api.py:63-64 and _propose

   • Any exception from table_names, propose or the state store surfaces from record, so the REST and playground rating calls return an error for a rating that was in fact saved.
   • Fix: catch the error in record, log it as a warning, and still return the row. The rating is the primary write.

7. NIT: apply cannot remove a key from a change. api/proposals_api.py:144-145 with accept.py:54

   • The edited change from the review file is merged over the stored change ({**stored, **edit}), so a reviewer who deletes a key, for example params, sees it come back.
   • Fix: for apply, treat the file's change as the full replacement. accept(edits=...) can stay a merge, as the design says. Alternatively, document the behaviour in review.md.

8. NIT: the docs mismatch on the curated index refresh. accept.py:16 and review.md

   • Both say IndexingOrchestrator.refresh_curated, while the code calls rebuild_index(curated_only=True).
   • That is fine, and the PR description tells K3 where to rename it. The design table (wave_b.md section 5) still says refresh_curated, so a one-line Built note there would save K3 a search.

Verdict

Fix first.

• Finding 1 is a lost-write and double-write race in the one component whose job is to prevent exactly that.
• Findings 2 and 3 are the same class of problem: a person's file ends up out of step with the queue, or loses an edit.
• All three fixes are small, and each repro is a short test worth keeping.
• Rebase onto main after feat(knowledge): knowledge sources, merge, scope filter, show and export (K1) #336, as described at the top.

[nadeem4]

Thanks for the review. The branch is rebased onto main with git rebase --onto origin/main 949f6376, and each fix below is written test-first, in 90c475f9.

1. BLOCKER, decisions under the lock: fixed.
   • accept now records each file's items (put_proposal, decide_proposal, proposal.decided) inside the locks, right after that file's write (_decide_written).
   • It also re-reads every item as still proposed under the locks.
   • reject takes the same accept:<real path> locks (_lock_all, in one sorted order) and re-checks the status under them.
   • New deterministic tests:
     • test_the_same_proposal_accepted_twice_at_once_is_accepted_once holds the first call inside decide_proposal and waits until the second call is at the lock. The result is one accepted, one refused, and one write_glossary.
     • test_a_reject_waits_for_an_accept_in_flight: the reject gets NOT_FOUND, and the status stays accepted.
2. A failed second write: fixed by the same change. If the curated write fails after the glossary write, the glossary's items are already accepted and the curated ones stay proposed. Test: test_a_failed_second_write_keeps_the_first_files_decisions, which injects NOT_WRITABLE into write_curated. The module docstring, review.md and the invariant now say that a call is refused as a whole only by the pre-write checks.
3. The curated hash at config-load time: fixed.
   • curated_file.loaded_sha256(config) reads the file once, hashes it and parses it.
   • If the parse is not config.curated, the hash is STALE, so accept refuses with FILE_CHANGED.
   • Test: test_curated_yaml_edited_before_the_engine_was_built_is_not_overwritten, your repro.
   • One limit is documented: a ${...} reference in curated.yaml reads as stale.
4. tables covers everything the change names: fixed.
   • propose completes tables with named_tables(ctx, proposal): the table, a column's table, both sides of a join, a term's tables and its columns' tables, and the tables a verified query's SQL reads (inspect_verified_sql).
   • Test: test_proposal_tables_cover_every_table_its_change_names. A term, a join and a query that name salaries are hidden from the analyst.
   • The rule is in review.md, the Built note and the PR body's "For later rows" section, for F1, P1 and L2.
5. NIT, review show: fixed.
   • The lookup moved into the engine as engine.proposals.current(id, *, user_context), through review.queue.current_value.
   • A curated query whose SQL reads a table outside the caller's scope comes back as None.
   • current is in the boundary test's D60 read-method list. The CLI now only prints it.
   • Test: test_current_value_is_shown_only_when_the_caller_may_see_it.
6. NIT, the thumbs-up proposal failing the rating: fixed. record logs a warning and still returns the stored row. Test: test_a_failed_proposal_never_fails_the_rating.
7. NIT, apply merging: fixed. apply calls accept(..., replace=True), so the file's change replaces the stored one; accept(edits=...) still merges, as the design says. Test: test_apply_replaces_a_change_rather_than_merging. review.md is updated.
8. NIT, refresh_curated naming: done. I kept the code and updated the docs. The Q1 Built note in wave_b.md now says the refresh is rebuild_index(curated_only=True), which runs IndexingOrchestrator.refresh_curated (the method section 5 names), and that K3 renames it in _refreshed() in review/accept.py. The accept docstring says the same.

Checks run locally:

• test_review_queue.py (26), architecture/test_boundaries.py, cli/test_config_flag.py, test_wave_b_shapes, test_readme_cli_drift, test_trace_result_stores, test_phase2_events, cli/test_storage_purge, cli/test_feedback_command, test_feedback_store, test_cli_curated_validate, test_curated_startup, test_knowledge_sources, test_cli_knowledge, test_playground_feedback and test_index_curated_refresh: all pass.
• mkdocs build --strict: OK.