Repository navigation
A base can opt into confirming well-grounded mapping definitions, and a person can take any of them back - #1332
Aschenbath wants to merge 1 commit into
Conversation
WaylandYang
left a comment
There was a problem hiding this comment.
This needs a design decision before the code review can finish, so I am putting it here rather than inline.
- Governance. 0043 decision 1 says every review queue but nods goes through the governor and names concept mappings as its cut 2;
docs/design/governance.mdrequires anagent_decisionsrow, a revert and a fuse for every automatic decision. This PR confirms mappings automatically with none of those, and its ADR text declares itself independent of governance without revising 0043. - Direction. 0036's dead ends say exploration-proposed SQL definitions are what automatic confirmation must never ship, and #556 retires this table's role. Automating the retiring path is 600 lines against two records. If the answer is that mappings are configuration and exempt (as #725 puts them on the data-source side), then 0043 and governance.md need the revision note, and an automatic confirm still needs a revert.
Code findings, whichever way that goes:
crates/utopia-store/src/mappings.rs, the upsert:status = EXCLUDED.statusunderdecided_by IS NULL AND status IN ('proposed','confirmed')demotes an effective definition silently. An auto-confirmed GMV that the next exploration scores 0.7, or that is re-explored after the switch is turned off, flips back toproposed; chat stops using it and no ledger event records the change. The ADR text promises the opposite.- The same WHERE clause widens the refresh regardless of the switch: any confirmed row with NULL
decided_by(scripts/bench/ask.mjs:162seeds truth definitions that way) is now rewritten by the next exploration even with the switch off. Before,WHERE status='proposed'protected every confirmed row. migrations/0117_*collides with #1334 and #1131.- The Chinese comment on
propose()recording the DO UPDATE…WHERE / RETURNING trap was replaced by a terse English one; please restore the register, and comment the new struct fields. automatic_definitions_refresh_until_a_person_decidesbundles about nine behaviours in 127 lines; one test per behaviour, and the route test likewise.- Commit messages carry
feat:/fix:/docs:prefixes; one English sentence stating the motivation. - 0011's status line is not updated and the description does not say.
setting_onlyinkbs.rsis an enumeratedis_none()list; a field added toUpdateKbReqlater becomes Editor-settable unless someone remembers to extend it. A comment there at least.
176b756 to
f2bc765
Compare
… a person can take any of them back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Aschenbath <Aschenbath@users.noreply.github.com>
f2bc765 to
9a9d470
Compare
|
Thanks for this. I took the second branch of your design question: mappings are configuration and exempt from the governor. The branch is now one commit on current Governance. 0043 now has a Direction. I don't have a good answer to 0036, and I've written that into 0011 rather than leave it implied. A score of 0.8 cannot catch On the code findings:
One limit I left in place: a revert keeps the current definition, because the row holds one version. If a refresh replaced a good definition, the good one is in the history, and a person gets it back by editing. Checks are in the description. Every target passed except one test that needs |
|
Thanks for working through all of this. The upsert rules, the marker column and the revert read well. I'd still like this to wait for #556: 0036 says `sum(amt_pay)` is what automatic confirmation must never ship, and an off-by-default switch doesn't answer that. As you offered, please close this for now and carry those parts into #556. Things to fix when it comes back:
|
|
Closing as agreed, to come back as part of #556. I'll carry over the upsert rules, the marker column and the revert, and fix the four points you listed there: pre-migration pending hand-edits with NULL |
Closes #504.
What this does
A base can opt into confirming exploration's mapping definitions without a person, and a person can take any of those confirmations back. The switch is
knowledge_bases.auto_confirm_mappings, default off.confidenceandrationaleon the proposal. With the switch on, a proposal confirms itself only when its score is at least 0.8, its rationale is not empty and it has an expression or SQL. Anything less waits for a person, as before.concept_mappings.auto_confirmed_atis set while the current definition stands on an automatic confirmation.decided_by IS NULLcannot be the marker, becausescripts/bench/ask.mjs:162seeds confirmed truth that way.written = false. A refresh that changes a definition keeps the old one inconcept_mapping_revisionswith no actor. Rows a person confirmed, rejected or edited are never touched. That includes an edited proposal that is still pending, and the bench's seeded truth.POST /kbs/{id}/mappings/{mapping_id}/revert(Editor) takes a machine-confirmed definition out of effect. It returns the row to pending under the person's name, so the next exploration does not confirm it again. A row that was not confirmed automatically gets a conflict. The row keeps its definition, because it holds only one version. An older version is in the history, and a person restores it by editing.mapping.auto_confirmedaudit event and each revert withmapping.auto_confirm_reverted, so a failed audit writes nothing. The mappings page marks automatic definitions, shows their evidence and has a Revert button. Text is in English and Chinese.auto_type_resolutionthey get their own switch and noagent_decisionsrow, precedent read or fuse. They keep the audit trail and the revert. The note is in 0043's new## Revisionsand indocs/design/governance.md. 0011 has a dated revision, and its status line now includes this PR.sum(amt_pay)that runs, looks right and is off by a hundred. 0011's revision says so. What limits that risk is the switch being off by default, the audit trail and the revert. The threshold does not, and nothing here claims an accuracy improvement or argues for turning the switch on by default.0124_a_base_can_opt_into_mapping_confirmation.sql. 0117 collided with A regression case can expect no binding while a person's binding remains authoritative #1334 and Govern conflicts in the typed graph and preserve human decisions #1131. The backup schema count is now 111.Why this is over 500 lines
The diff adds 1410 lines: about 410 of product code, about 945 of tests and 55 of documentation. Most of the tests come from splitting each behaviour into its own test, as review asked: 18 store tests, 8 route tests and 4 web tests. The product code is one contract. Shipping the switch without the revert would confirm definitions with no way back. Shipping the gate without the upsert rules would let exploration demote or overwrite rows a person owns. So I could not find a smaller cut that is safe to merge on its own.
How it was checked
All on x86_64 Linux: a Docker container on WSL2 with Rust 1.99.0 and a throwaway
pgvector/pgvector:pg16. The tree is commit9a9d4709. The last amend only added four lines of text to 0011.cargo fmt --all --check: passed.cargo clippy --locked --workspace --all-targets -- -D warnings: passed.cargo run -p utopia-store --example migrateon a fresh database: passed.cargo test --locked --workspace --no-fail-fastwithUTOPIA_TEST_REQUIRE_DB=1: 1777 passed, 6 ignored, and one target did not pass. Inutopia-cli --test restore, 16 of 17 pass.real_postgres_backup_and_restore_require_force_and_preserve_files_on_sql_failurepanics with "pg_dump is required", because my check image has no PostgreSQL client tools. CI installspostgresql-client-16, so CI runs that test and I did not.a_mapping_is_not_a_factalso passed when run on their own.pnpm test: 50 files, 344 tests passed.pnpm build: passed.🤖 Generated with Claude Code