fix(kb): the dj1l cases discriminate, and a replaced rule cannot silently become an add

Five findings from the second review round, and the first is this PR's own
standard applied to its own P1.

**The two fixes improvise-dj1l names had no discriminating test.** Reverting
`has_key` to unification, or dropping `canon_key` from the duplicate gate, left
kb-test at 27 passed. Case 11 was carried by `check_total` refusing with a
DIFFERENT message, and case 10 by the other half of the fix. Both now assert
which refusal fired (`said()`), plus two cases aimed at each half: a rm key with
a variable in a non-final position must be refused as ABSENT rather than
matching everything, and a keyed FACT whose key holds a variable must ADD
without excluding the siblings it would unify with. Each proved by reverting
exactly one fix and watching exactly that case fail.

**Keying a rule on its whole clause made `replace` on an edited rule silently
become `add`** -- both arms live, the stale one still firing, and `check_total`
cannot see it because `NO + NF` is correct arithmetic for an add. Reachable on
the 15 `kb_error/1` arms and the `kb_text/2` rules, so editing the check
improvise-2f97 adds is the first thing to hit it. `check_rule_replace` refuses
and names the route (rm, then add); the negative arm -- replace of a rule
present verbatim -- is tested, or the refusal would just be "replace never
works on rules".

**The `decision(` arm was dead code.** `cited_name` took the first field, and
the repo spells `decision(<ticket>, <name>)`, whose ticket id fails
`name_shaped/1` on the hyphen -- so that marker could never fire for a real
citation. A second `cited_name` clause takes the second field. All three
markers are now exercised.

Minor, from the same round: `_JS_TEST` no longer accepts `test.only`, which
disables every other test in its file, with a case asserting a heading cannot
be satisfied by one; and `node-test-test.sh`'s last case no longer duplicates
an earlier one.

Filed rather than fixed: improvise-w8ih, extracting `web/assemble.sh`'s gate so
its own absence is visible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-20 17:01:33 -07:00
parent 0b7a2351b5
commit cd094b6dfb
6 changed files with 201 additions and 39 deletions
+5
View File
@@ -221,6 +221,7 @@ doctrine(design_against_the_model_before_implementing,'AGENTS.md has always said
doctrine(a_key_rule_must_ask_whether_the_relation_is_multi_valued,'scripts/kb keyed every clause on the functor plus all arguments but the last, which is right for doctrine(Name,Text) and wrong for any RELATION: crate_depends_on(A,B) then keyed on A, so a crate depending on two crates read as one key twice and was unaddable. Found by trying to model a proposed crate, not by any test -- the tool had only ever been pointed at single-valued facts. The fix cannot read the KB own functional/1, because that check keys an arity-3 predicate on its FIRST argument, which is right for crate/3 and wrong for decision/3 where one ticket holds several decisions under different names. So key_arity/2 is a hand-maintained table and anything unlisted keys on the WHOLE head, which is the safe direction: an unlisted relation accepts many clauses and only an exact repeat is a duplicate.').
doctrine(a_local_shell_is_not_the_ci_shell,'zsh ABORTS on an unmatched glob and bash leaves it as a literal, so a CI step written and probed in zsh can fail in a way no developer shell can reproduce. node --test web/tests/*.test.js under bash therefore globbed the literal itself, matched nothing, reported 0 tests and exited 0 -- both node steps gated nothing the moment their directory moved (improvise-vz54). Local shell vs CI shell is a fault-injection axis in its own right: probe a CI line under the shell the workflow names, not the one you are typing in. The fix moves the glob into scripts/node-test.sh so the expansion is the script\'s and can be refused.').
doctrine(a_convention_that_keeps_a_checker_quiet_is_the_one_that_blinds_it,'context/test-evidence used #### for browser tests precisely BECAUSE --check-evidence resolved names against Rust fn names only, so a ### heading naming a Playwright test would be reported as an orphan. The workaround made every browser entry invisible, and a deleted test left its heading promising coverage for a whole release (improvise-5ray). Cure: collect ### and #### alike and decide by SHAPE -- a bare identifier is a Rust test, a quoted string is a JS test, anything else is prose naming nothing. When a checker forces a convention on the thing it checks, ask what that convention hides.').
doctrine(a_refusal_is_evidence_only_when_it_is_the_one_you_named,'Two kb-test cases passed against a tree with the fix they guarded reverted, because a DIFFERENT guard refused the same delta and the assertion was only \"fails, and the kb is unchanged\" (review of PR #69). A tool with several guards will usually refuse a broken delta somehow, so \"it was refused\" is nearly free and says nothing about the fix under test. Assert the REFUSAL MESSAGE, or construct a case only the fix can catch -- and prove it by reverting that one fix and watching the case fail. The same trap in reverse: a case asserting an OUTCOME (the add lands and every sibling survives) can discriminate where a message cannot, because what a wrong key destroys is the outcome.').
work_done('improvise-8tk',bug_fix,'ZZ/:wq quit even when the save failed, silently discarding the model. Save now aborts the effect batch on failure; save_to_current_path/1 returns Result<String,String>; three tests in ui/effect.rs.').
work_done('improvise-907',bug_fix,'Import wizard step machine redesign: validation_error() gate at ReviewProposals (no-category and over-limit), history-stack previous() bound to Left on every step, and Done made a real confirmation step holding the built workbook. Superseded improvise-gfq, improvise-o1c, improvise-c6j.').
work_done('improvise-6sq',wont_do,'Merging EditOrDrill and EnterEditAtCursorCmd into one EnterEdit{mode} is not safe: they differ in WHEN the cell value is read, not in a mode parameter. Added ui/app.rs::tests::i_and_o_diverge_on_an_empty_records_grid as executable evidence.').
@@ -560,6 +561,10 @@ name_char('_').
name_shaped(A):-atom_chars(A,B),B=[C|D],lower_or_digit(C),forall(member(E,B),name_char(E)),memberchk('_',D).
citation_token(A,B):-(sub_atom(A,C,1,D,')')->true;C=999),(sub_atom(A,E,1,F,',')->true;E=999),G is min(C,E),G>0,G<120,sub_atom(A,0,G,H,B).
cited_name(A,B,C):-citation_marker(D,B),sub_atom(A,E,F,G,D),sub_atom(A,H,G,0,I),citation_token(I,C),name_shaped(C).
cited_name(A,decision,B):-sub_atom(A,C,D,E,'decision('),sub_atom(A,F,E,0,G),decision_name(G,B),name_shaped(B).
citation_resolves(doctrine,A):-doctrine(A,B).
citation_resolves(invariant,A):-invariant(A,B).
citation_resolves(decision,A):-decision(B,A,C).
citation_body(A,B):-sub_atom(A,C,1,D,')'),!,C>0,C<200,sub_atom(A,0,C,E,B).
trim_left(A,B):-sub_atom(A,0,1,C,' ')->sub_atom(A,1,D,0,E),trim_left(E,B);B=A.
decision_name(A,B):-citation_body(A,C),sub_atom(C,D,1,E,','),!,F is D+1,sub_atom(C,F,G,0,H),trim_left(H,B).
+10 -4
View File
@@ -254,11 +254,12 @@ JS_ROOTS = [os.path.join("harness", "tests"), os.path.join("web", "tests")]
_RUST_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
_QUOTED_NAME = re.compile(r'^"(.+)"$')
# Only a RUNNABLE LEAF test. `.skip`, `.todo` and `.describe` are excluded --
# a quoted heading must not be satisfiable by disabled coverage or by an empty
# suite, which is the same hole the node wrapper had (review, PR #69).
# Only a RUNNABLE LEAF test. `.skip`, `.todo`, `.describe` and `.only` are all
# excluded: the first three name no executed assertion, and `.only` disables
# every OTHER test in its file, so a heading satisfied by one is a heading
# satisfied by a file that stopped running (review, PR #69).
_JS_TEST = re.compile(
r"""(?:^|[^\w.])(?:test|it)(?:\.only)?\s*\(\s*(['"`])(.+?)\1"""
r"""(?:^|[^\w.])(?:test|it)\s*\(\s*(['"`])(.+?)\1"""
)
@@ -717,6 +718,11 @@ SELF_TEST_EVIDENCE = [
'test.skip("a skipped one", () => {});\ntest.todo("a todo one");\n',
["a skipped one", "a todo one"], []),
("a heading cannot be satisfied by a test.only, which disables its file",
'#### "the only one left running"\n', "",
'test.only("the only one left running", () => {});\n',
["the only one left running"], []),
("a heading cannot be satisfied by an empty suite's name",
'#### "a suite with nothing in it"\n', "",
'test.describe("a suite with nothing in it", () => {});\n',
+21
View File
@@ -49,6 +49,7 @@ run(Mode, KbFile, DeltaFile, OutFile) :-
check_self_consistent(Funcs, New),
conflicts(Funcs, Old, New, Clashes),
gate(Mode, Clashes),
check_rule_replace(Funcs, Mode, Old, New),
foldl_merge(Funcs, New, Mode, Old, Merged),
expected_total(Funcs, Mode, Old, New, Expect),
check_total(Merged, Expect),
@@ -197,6 +198,26 @@ conflicts(Funcs, Old, New, Clashes) :-
member(O, Old), clause_key(Funcs, O, K2), same_key(K, K2) ),
Clashes).
% A rule keys on its whole clause, so an EDITED rule matches nothing and
% `replace` silently becomes `add` -- both arms then live, and the stale one
% goes on firing beside the new one. check_total cannot see it: NO + NF is
% correct arithmetic for an add. So replace refuses a rule it cannot find and
% names the route, which is rm then add (improvise-dj1l, review of PR #69).
check_rule_replace(Funcs, replace, Old, New) :-
!,
findall(T,
( member(T, New), T = (_ :- _),
clause_key(Funcs, T, K),
\+ ( member(O, Old), clause_key(Funcs, O, K2), same_key(K, K2) ) ),
Fresh),
( Fresh == []
-> true
; format("kb-merge: REFUSED -- replace cannot find these rules, so it would ADD them beside the ones already there:~n", []),
forall(member(T, Fresh), format(" ~q~n", [T])),
die('a rule is keyed on its whole clause, so an edited body is a NEW clause: kb rm the old one, then kb add')
).
check_rule_replace(_, _, _, _).
gate(add, []) :- !.
gate(add, Clashes) :-
!,
+103 -5
View File
@@ -18,6 +18,11 @@ bad() { printf ' FAIL %s\n' "$1"; fail=$((fail+1)); }
fresh() { cp "$KB" "$work/kb.pl"; }
kb() { KB_FILE="$work/kb.pl" "$here/kb" "$@"; }
unchanged() { diff -q "$KB" "$work/kb.pl" >/dev/null; }
# A refusal is only evidence when it is the refusal you NAMED: two of these
# cases passed against a tree with the fix reverted, because a DIFFERENT guard
# caught the same delta and the assertion was only "fails and kb unchanged"
# (review of PR #69). said() asserts which one fired.
said() { grep -qF "$2" "$1"; }
echo "kb-test: the refusals"
@@ -313,7 +318,11 @@ fresh
printf 'probe_fact(a).\nprobe_fact(b).\n' > "$work/facts.pl"
kb add "$work/facts.pl" >/dev/null 2>&1
printf 'probe_fact(X) :- doctrine(X,_).\n' > "$work/rule.pl"
if kb replace "$work/rule.pl" >/dev/null 2>&1; then
# `add`, not `replace`: the rule is new, and a rule that is not already there
# is what case 15 makes replace refuse. This is also the discriminating door --
# under unification `conflicts` sees the non-ground key match both facts and
# refuses the add outright, so the reverted fix fails HERE.
if kb add "$work/rule.pl" >/dev/null 2>&1; then
kept=0
grep -q '^probe_fact(a)\.$' "$work/kb.pl" && grep -q '^probe_fact(b)\.$' "$work/kb.pl" && kept=1
if [ "$kept" = 1 ]; then
@@ -329,19 +338,21 @@ if kb replace "$work/rule.pl" >/dev/null 2>&1; then
bad "the rule was not written at all"
fi
else
bad "replace refused a rule over a predicate holding facts"
bad "add refused a rule over a predicate holding facts: a variable key matched them"
fi
# 11. The duplicate gate must still fire once keys are compared as variants:
# two clauses that really are the same key are still two clauses for one key.
fresh
printf 'probe_rule(X) :- doctrine(X,_).\nprobe_rule(Z) :- doctrine(Z,_).\n' > "$work/same.pl"
if kb add "$work/same.pl" >/dev/null 2>&1; then
if kb add "$work/same.pl" >"$work/out" 2>&1; then
bad "add accepted the same clause twice in one delta"
elif unchanged; then
elif ! unchanged; then
bad "add refused a duplicate but MODIFIED the kb"
elif said "$work/out" "names one key twice"; then
ok "add still refuses one key named twice, compared as variants"
else
bad "add refused a duplicate but MODIFIED the kb"
bad "the delta was refused, but not BY THE DUPLICATE GATE (canon_key is unreached)"
fi
# 12. The arithmetic check, proved by injecting the fault it exists for: a
@@ -400,5 +411,92 @@ else
bad "the citation check refuses a name that DOES resolve"
fi
# 14. The case only same_key/2 can catch. A rm key with a variable in a
# NON-final position -- work_done keys on two arguments -- matches nothing
# as a variant and everything by unification. Under unification it removed
# an arbitrary bug_fix entry and reported success; here it must be refused
# as ABSENT, by that message and no other.
fresh
printf 'work_done(_,bug_fix,_).\n' > "$work/wild.pl"
if kb rm "$work/wild.pl" >"$work/out" 2>&1; then
bad "rm removed clauses by unifying a variable key against them"
elif ! unchanged; then
bad "rm refused a wildcard key but MODIFIED the kb"
elif said "$work/out" "no such clause to remove"; then
ok "a key with a variable in it matches nothing, rather than everything"
else
bad "the wildcard rm was refused, but not as ABSENT (same_key is unreached)"
fi
# 15. A rule keys on its whole clause, so an EDITED rule matches nothing and
# replace would silently become add -- leaving the stale arm firing beside
# the new one, with correct arithmetic (found by review on PR #69).
fresh
printf 'probe_rule(X) :- doctrine(X,_).\n' > "$work/rule1.pl"
printf 'probe_rule(X) :- doctrine(X,_), X \\== b.\n' > "$work/rule2.pl"
kb add "$work/rule1.pl" >/dev/null 2>&1
before="$(kb count | grep '^total=' | cut -d= -f2)"
if kb replace "$work/rule2.pl" >"$work/out" 2>&1; then
after="$(kb count | grep '^total=' | cut -d= -f2)"
bad "replace ADDED an edited rule beside the old one ($before -> $after)"
elif said "$work/out" "kb rm the old one"; then
ok "replace refuses a rule it cannot find, and names the route"
else
bad "the edited rule was refused, but without naming rm/add"
fi
# The negative arm: replace of a rule that IS there must still work, or the
# refusal above would just be "replace never works on rules".
if kb replace "$work/rule1.pl" >/dev/null 2>&1; then
ok "replace of an unchanged rule is still accepted"
else
bad "replace refuses a rule that is present verbatim"
fi
# 16. All three citation markers must FIRE. doctrine( and invariant( take the
# name as the first field; decision( takes the SECOND, because the repo
# spells decision(<ticket>, <name>) and a ticket id is not name-shaped --
# so a single-field rule left that marker unreachable (review, PR #69).
cat > "$work/cite-d.pl" <<'EOF'
doctrine(a_probe_citing_a_doctrine,'cites doctrine(a_name_that_was_retired), which does not exist').
EOF
cat > "$work/cite-i.pl" <<'EOF'
doctrine(a_probe_citing_an_invariant,'cites invariant(a_rule_that_was_never_written), which does not exist').
EOF
cat > "$work/cite-x.pl" <<'EOF'
doctrine(a_probe_citing_a_decision,'cites decision(improvise-nope, a_name_that_was_never_decided), which does not exist').
EOF
fresh
markers=0
for m in d i x; do
kb check --with "$work/cite-$m.pl" >/dev/null 2>&1 || markers=$((markers+1))
done
if [ "$markers" -eq 3 ]; then
ok "all three citation markers are reachable, not just the first"
else
bad "only $markers of 3 citation markers fire"
fi
# 17. The case aimed at has_key/2 itself. A keyed FACT with a variable in a
# key position -- work_done keys on (id, kind) -- is a key no existing
# clause is a variant of, so the add must land and every sibling survive.
# Under unification the fold excludes the clauses the key happens to match
# and the add is refused instead (by the total, which is the backstop, not
# the fix). Asserted as the OUTCOME, because that is what a wrong has_key
# destroys.
fresh
before="$(kb count | grep '^work_done/3=' | cut -d= -f2)"
printf "work_done(_,maintenance,'a probe clause with a variable id').\n" > "$work/wildfact.pl"
if kb add "$work/wildfact.pl" >"$work/out" 2>&1; then
after="$(kb count | grep '^work_done/3=' | cut -d= -f2)"
if [ "$((before + 1))" = "$after" ]; then
ok "a fact whose key holds a variable adds without excluding its siblings"
else
bad "adding one clause moved work_done/3 from $before to $after"
fi
else
bad "add refused a key holding a variable: the fold matched by unification"
fi
printf '\nkb-test: %d passed, %d failed\n' "$pass" "$fail"
[ "$fail" -eq 0 ]
+13 -4
View File
@@ -101,11 +101,19 @@ run_case "an empty suite is refused" 1 "$work/suite/*.test.js"
run_case "a describe holding a real test is not called barren" 0 \
"$work/shapes/*.test.js"
# A barren file hiding among real ones is the shape a refactor leaves: the run
# passes, the count is healthy, and one file asserts nothing.
# A degenerate file hiding among real ones is the shape a refactor leaves: the
# run passes, the count is healthy, and one file asserts nothing. node's TAP
# does not attribute a passing test to its file, so a whole-run count cannot
# see either of these -- which is why the guard runs one file at a time.
run_case "a barren file among passing ones is still named" 1 \
"$work/tests/green.test.js" "$work/empty/nothing.test.js"
run_case "a skip-only file among passing ones is still named" 1 \
"$work/tests/green.test.js" "$work/skipped/skipped.test.js"
run_case "a todo-only file among passing ones is still named" 1 \
"$work/tests/green.test.js" "$work/todo/todo.test.js"
# Two globs where the SECOND dies is the shape a directory move makes: the
# first still matches, so a check over the union would pass.
run_case "one dead pattern among several is refused" 1 \
@@ -114,9 +122,10 @@ run_case "one dead pattern among several is refused" 1 \
run_case "no argument at all is a usage error, not a pass" 2
# The literal the caller would pass if they forgot to quote AND the glob
# matched: ordinary filenames, which expand to themselves.
# matched: ordinary filenames, which expand to themselves -- several of them,
# since one is what the accepted case above already passes.
run_case "already-expanded filenames still run" 0 \
"$work/tests/green.test.js"
"$work/tests/green.test.js" "$work/shapes/shapes.test.js"
printf '\nnode-test-test: %d passed, %d failed\n' "$pass" "$fail"
[ "$fail" -eq 0 ]
+49 -26
View File
@@ -15,12 +15,13 @@
# mentioning one, by `test.skip`/`test.todo` and by an empty `test.describe` --
# every one of which executes no assertion (found by review on PR #69).
#
# Two measurements make the report usable, both on node 24.20. `# pass` is 0
# for a skip-only file, a todo-only file and an empty suite. And a file that
# registered NO test is reported as one passing test WHOSE NAME IS THE FILE
# PATH, which is why `# tests` alone reads as the file count and a
# minimum-count assertion over it is satisfied by the very file it should
# reject. That naming convention is an assumption about node, so
# Two measurements make the report usable, both on node 24.20, and both are
# taken PER FILE. `# pass` is 0 for a skip-only file, a todo-only file and an
# empty suite -- read over a whole run it would be masked by a passing
# neighbour. And a file that registered NO test is reported as one passing test
# WHOSE NAME IS THE FILE PATH, which is why `# tests` alone reads as the file
# count and a minimum-count assertion over it is satisfied by the very file it
# should reject. That naming convention is an assumption about node, so
# scripts/node-test-test.sh pins it: if a future node stops doing it, the
# self-test fails rather than the guard silently going quiet.
set -uo pipefail
@@ -49,27 +50,39 @@ done
tap="$(mktemp)"
trap 'rm -f "$tap"' EXIT
# Two reporters: the readable one to the terminal, TAP to a file, so the run
# can be read back without downgrading what a human sees.
node --test \
--test-reporter=spec --test-reporter-destination=stdout \
--test-reporter=tap --test-reporter-destination="$tap" \
"${files[@]}"
status=$?
[ "$status" -eq 0 ] || exit "$status"
passed="$(sed -n 's/^# pass \([0-9][0-9]*\)$/\1/p' "$tap" | tail -1)"
if [ -z "$passed" ]; then
echo "node-test: could not read a pass count out of node's TAP output" >&2
exit 1
fi
if [ "$passed" -eq 0 ]; then
echo "node-test: the run exited 0 with nothing passing (skipped, todo or an empty suite)" >&2
exit 1
fi
# ONE FILE PER RUN. node's TAP does not attribute a passing test to the file it
# came from -- three files produce three flat `ok` lines named after the tests
# -- so a whole-run `# pass` count cannot tell a fully-skipped file from its
# passing neighbours, and a skip-only file rode in on a sibling (found by
# review on PR #69). node already forks a child per file, so this changes what
# is OBSERVABLE, not how the tests run.
status=0
total=0
silent=()
barren=()
for f in "${files[@]}"; do
node --test \
--test-reporter=spec --test-reporter-destination=stdout \
--test-reporter=tap --test-reporter-destination="$tap" \
"$f"
ran=$?
if [ "$ran" -ne 0 ]; then
status="$ran"
continue
fi
passed="$(sed -n 's/^# pass \([0-9][0-9]*\)$/\1/p' "$tap" | tail -1)"
if [ -z "$passed" ]; then
echo "node-test: could not read a pass count out of node's TAP output for $f" >&2
status=1
continue
fi
total=$((total + passed))
if [ "$passed" -eq 0 ]; then
silent+=("$f")
continue
fi
# A file that registered NO test is reported as one passing test whose name
# is the FILE PATH, which is why `# tests` reads as the file count.
awk -v want="$f" '
/^(ok|not ok) [0-9]+ - / {
desc = $0
@@ -80,10 +93,20 @@ for f in "${files[@]}"; do
END { exit found ? 0 : 1 }
' "$tap" && barren+=("$f")
done
if [ "${#silent[@]}" -gt 0 ]; then
echo "node-test: these files passed nothing (skipped, todo or an empty suite):" >&2
printf ' %s\n' "${silent[@]}" >&2
fi
if [ "${#barren[@]}" -gt 0 ]; then
echo "node-test: these files registered no test, so running them asserted nothing:" >&2
printf ' %s\n' "${barren[@]}" >&2
fi
if [ "$status" -ne 0 ]; then
exit "$status"
fi
if [ "${#silent[@]}" -gt 0 ] || [ "${#barren[@]}" -gt 0 ]; then
exit 1
fi
echo "node-test: $passed test(s) passed over ${#files[@]} file(s)"
echo "node-test: $total test(s) passed over ${#files[@]} file(s)"