Skip to content

design: validate a conservative expression declaration policy - #839

Merged
WaylandYang merged 7 commits into
deeplethe:devfrom
Maya-Kid:codex/stage2-expression-policy
Sep 23, 2026
Merged

WaylandYang merged 7 commits into
deeplethe:devfrom
Maya-Kid:codex/stage2-expression-policy

Conversation

@Maya-Kid

@Maya-Kid Maya-Kid commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

ADR 0032 asks for declaration checks and a usable nested picker, but the standalone model did not exercise the real interface or a real knowledge base. This revision puts the decision in ADR 0049 and replaces the disposable picker with a reusable controlled React editor at the unlisted /kb/$kbId/expression-draft route.

The route uses the existing authenticated ontology/rules APIs, SearchSelect/Dropdown/Input controls, design tokens and both language packs. It only previews an unsaved structured draft. It does not enable unit validation, save rules, change B1 metadata editing or change dependency analysis. Unsupported expressions stay unconverted; incomplete constants and missing references cannot become a zero or a default attribute.

The policy module, Node model tests, PostgreSQL probe and standalone HTML were archived outside the repository and removed from the PR tree. ADR 0049 preserves their historical findings, exact source revision and limitations. Both index tables are updated; 0032 has a dated note beside its unit-check discussion.

Validation at 64206696689f1901f5cfd3420553a26d70b8de3a on latest dev 69560db5969fae8def8c4e84b7572d841e1dca57:

  • pnpm guard, pnpm typecheck, pnpm test (119 passed), and pnpm build passed on Linux, Node 22.23.2 / pnpm 10.2.1.
  • Headless Chrome against the real Linux backend read API-created bases with 30, 300 and 1000 attributes, including duplicate/long bilingual labels, mixed datatypes and absent units. Search reached attribute 0999 by key; keyboard selection/focus, margin, right-nested subtraction/division, internal edits, unfinished numbers, depth-four cap, 390px width, empty bases and SPA KB reset passed. The draft UI sent no write requests.
  • Loading, Chinese UI and actual restricted-KB authorization passed. Read/retry and unknown-shape checks additionally used explicitly injected browser response faults.

Four levels remain a long vertical form on a narrow screen; these checks are not a human usability score. Exact 1, the known-unit allowlist and first-cut ratio policy remain pending. There is no B2 persistence flow or production unit validator. Draft status is intentional.

Refs #488

… rule writes

Signed-off-by: dada-yan <BinjunYann@gmail.com>
Signed-off-by: dada-yan <BinjunYann@gmail.com>
@WaylandYang

Copy link
Copy Markdown
Contributor

The content here is decision material — it states a policy and asks maintainers to approve it — and the thinking in it is good. My objection is only about where it lives.

docs/decisions/README.md states the purpose of that directory plainly: code records what was built and git records when it changed; neither records why, and that directory does. Putting the reasoning in scripts/prototypes/**/README.md and leaving a five-line pointer in the accepted record splits it. A reader who opens the record in six months finds a stub aimed at a directory whose name says "prototype" — with no number, no row in either index table, no status line for an implementing PR to update, and none of the revision discipline the conventions require. The "Decision requested" section is exactly the thing that is supposed to be numbered and indexed.

Please give this its own numbered record in docs/decisions/, and turn the addition to the accepted record into a dated revision note that names it, per Revisions stay in place.

#832 in this same batch is the pattern to copy: it moved its reasoning into docs/decisions/0048-provenance-references-stay-inside-the-knowledge-base.md, added the row to both index tables, opened an issue for the open design question, and left the migration carrying only what SQL has to say.

One thing that is not my call and that I have flagged for the maintainer: whether the executable prototype itself belongs in the repository. scripts/bench/ is tracked, so there is precedent for committed harnesses, but these add a Python harness and a requirements.txt, and that is a policy question rather than a review point.

Specific to this one: the unit policy table and the FOR SHARE versus FOR KEY SHARE transaction boundary are both answers to open questions 0032 already carries ("Units and datatypes have to be checked when the expression is written, and today nothing checks them"). That makes them a revision of 0032 with a record of their own, not an appendix.

@WaylandYang

Copy link
Copy Markdown
Contributor

Adding one more, on scripts/prototypes/expressions/index.html specifically: the picker experiment should not be a standalone page.

The reason for isolating it is understood and half right — policy.mjs is the proposed policy, not the one in force, and 0032's picker is still an unbuilt cut, so there is nothing in RulesPanel to extend and no case for putting an unapproved policy on a production code path before the decision.

But the isolation costs more than it saves, because of what the page is being asked to answer. 0032's open question is "How deep before the picker loses", with the record's own example being that (revenue − cost) / revenue as nested dropdowns is worse than typing it. That is a question about whether the thing is bearable to use — and this page cannot answer it:

  • It runs on three hardcoded attributes (revenue, cost, ratio). A picker gets bad exactly when a real base puts hundreds of attributes behind each <select>, which is the case the experiment never sees.
  • It makes no API call at all, and "save" stringifies into a <pre>.
  • It reimplements every control with document.createElement, with inline hex colours (#111827, #64748b) and font: 16px system-ui. Living outside web/ also puts it outside pnpm guard, so it would be the one piece of interface in the repository that no style rule reaches — while the shadcn migration and the token bridge are the standing direction for everything else.

So what it demonstrates is the interaction shape in a mock, and the shape was not in doubt. Put it in web/ behind a route or a flag, built from the real components and reading a real base's attributes, and the depth question gets a transferable answer — and nothing has to be thrown away when the real picker lands. As a standalone file its fate is to become a second implementation that rots the day cut B2 ships.

The policy module and its tests are a different matter; policy.mjs being separate from the production validator is fine and is not what this note is about.

@WaylandYang

Copy link
Copy Markdown
Contributor

Decision on the committed prototype directories, so all three of these can move at once: the conclusions belong in a numbered record; the harness stays out of the repository.

The test we are applying is not "prototype versus production". It is: after the decision is made, will anyone run this again?

  • If yes, it is a bench and it belongs next to what it measures. scripts/bench/govern.mjs is tracked for exactly that reason — 0025 cites it, and when the governance prompt changes someone reruns it against the labelled pair set. Its value recurs.
  • If no, the finding goes in the record and the scaffolding does not enter git. That was the call on the HotpotQA harness earlier: the durable artifact was the number, not the rig.

There is a second reason, and it is the stronger one: an executable specification that is not the implementation is a second source of truth. This is the thing the codebase avoids everywhere else. 0032 refuses to store an expression as a string and evaluate it at run time; the rule model is structured precisely so that what the page shows is generated from what runs and cannot drift from it. A policy module that encodes the rules beside the validator rather than inside it is that same drift, with the added property that nothing will ever fail when the two disagree.

So: please lift the reasoning and the decisions requested into a numbered record under docs/decisions/, add the row to both index tables, and turn the paragraph added to the accepted record into a dated revision note naming it, per Revisions stay in place. #832 is the pattern — it moved its reasoning into docs/decisions/0048-provenance-references-stay-inside-the-knowledge-base.md, indexed it, and opened an issue for the question it wanted maintainers to settle.

None of this is a judgement on the work itself. The analysis is good and the experiments clearly did their job; the argument is only about what survives them.

Applied to this PR:

  • policy.mjs and policy.test.mjs — the executable spec described above. Whichever way the decision goes, it has no future: approved, the production validator implements it and this drifts; rejected, it is dead. The unit table and the operand rules are the part worth keeping, in the record.
  • test_declaration_lock.py — what it establishes is one sentence ("FOR SHARE blocks a concurrent declaration UPDATE; FOR KEY SHARE does not"), plus the transaction-boundary proposal. Both belong in the record. The script is the means, not the finding.
  • index.html — covered separately above: it should be a route or flag inside web/, built from the real components against a real base's attributes, because the question 0032 actually asks is about depth in a real picker.

…n-policy

Signed-off-by: dada-yan <BinjunYann@gmail.com>
…ation decisions.

Signed-off-by: dada-yan <BinjunYann@gmail.com>
@WaylandYang

Copy link
Copy Markdown
Contributor

This is still marked as a draft, so I have not merged it — a draft is your signal that it is not finished, and flipping that is your call rather than mine.

From my side the rework answers everything I raised: the reasoning is now a numbered record with rows in both index tables, the accepted record carries a dated revision note pointing at it, and the committed harness is gone. CI is green on all four checks and it merges cleanly onto dev.

Mark it ready for review when you consider it done and I will merge it.

@Maya-Kid
Maya-Kid marked this pull request as ready for review September 21, 2026 03:58
Signed-off-by: dada-yan <BinjunYann@gmail.com>

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picking this up after the ready-for-review flip — the rework matches what was asked
for on the 20th/21st, and nothing since then changes the shape: two index-sync
commits to keep 0049's rows next to 0050/0051 as they landed.

Verified on a merge of this branch onto current dev (after #833/#845/#860/#864):
merges cleanly, both README index rows land in order between 0048 and 0050,
pnpm guard 99 files compliant, pnpm test 132 passed (the three new
expressionDraft cases are collected by vitest, not just present), pnpm build
including type-check passes. No backend, no migration, both language packs updated.

What lands is a proposed record, same as 0047/0048/0050 — the exact spelling
of 1, the known-unit allowlist and whether ratios are in the first cut stay open
in the record's own status line for whoever settles them. The unlisted route is
read-only, non-persisting, and says so on screen; it stays unlisted until 0049 is
approved and the implementation folds it into RulesPanel, per the record's own
plan.

LGTM.

@WaylandYang
WaylandYang merged commit e3f5d1c into deeplethe:dev Sep 23, 2026
4 checks passed
@Maya-Kid
Maya-Kid deleted the codex/stage2-expression-policy branch September 26, 2026 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants