Conversation
The footer said '_{total} actions have an ActionSpec' but
ListJobHistory is part of KNOWN_ACTION_NAMES even though it has no
ActionSpec and is not in the table. The total catalogue count is
total + 1, so the first sentence must also say total + 1.
Closes lacs-project#455
vladimirrott
left a comment
There was a problem hiding this comment.
Thanks for taking #455 on, and for editing the generator instead of docs/action-reference.md. That file carries a "generated, do not edit by hand" banner and people edit it anyway; you went to crates/sysknife-daemon/tests/action_reference_doc.rs:140, where the sentence is built.
Requesting changes at d93f42de. Two blocking items, and the first one is partly mine.
1. The branch does not compile
Rust's inline format arguments take a bare identifier, so {total + 1} fails while the format string is being parsed. In a worktree detached at d93f42de:
$ out="$(podman run --rm --network=none -v "/tmp/r20p456:/repo:z" -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim cargo test -p sysknife-daemon --test action_reference_doc --offline --no-run 2>&1)"; rc=$?; echo "rc=$rc"; printf '%s\n' "$out" | grep -v '^warning: unused manifest key' | tail -30
rc=101
[... dependency compilation ...]
Compiling sysknife-daemon v0.16.0 (/repo/crates/sysknife-daemon)
Compiling sysknife-brain v0.16.0 (/repo/crates/sysknife-brain)
error: invalid format string: the `+` sign flag must appear after `:` in a format string
--> crates/sysknife-daemon/tests/action_reference_doc.rs:140:25
|
140 | "---\n\n_{total + 1} actions have an `ActionSpec` and are tabled above. The \
| ^ expected `:` before `+` sign flag in format string
|
= note: `+` comes after `:`, try `{:+}` instead of `{+}`
error: could not compile `sysknife-daemon` (test "action_reference_doc") due to 1 previous error
You had no way to see that from the PR page. GitHub parks a first-time contributor's fork runs at action_required until a maintainer approves them, and I had not:
$ gh pr checks 456 --repo lacs-project/sysknife 2>&1 | head -5
no checks reported on the 'fix-action-reference-footer' branch
I have approved all eight runs across this PR and #457, so the board will fill in. Sorry for the round trip; a compiler error should never be something a reviewer tells you about.
2. The footer number is correct today, and the patch makes it wrong
total counts the specs the generator put in the table. ListJobHistory has no ActionSpec, so it is not one of them, and the catalogue holds exactly one more name than the table has rows:
$ tail -3 docs/action-reference.md
---
_191 actions have an `ActionSpec` and are tabled above. The full catalogue (`KNOWN_ACTION_NAMES`) also includes `ListJobHistory`, which the dispatcher handles before the executor, for **192** total._
$ python3 - <<'PY'
import re,pathlib
s=pathlib.Path('crates/sysknife-types/src/lib.rs').read_text()
i=s.index('pub const KNOWN_ACTION_NAMES: &[&str] = &[')
j=s.index('];', i)
body=s[i:j]
names=re.findall(r'"([A-Za-z0-9_]+)"', body)
print("KNOWN_ACTION_NAMES len =", len(names))
print("ListJobHistory present:", "ListJobHistory" in names)
PY
KNOWN_ACTION_NAMES len = 192
ListJobHistory present: True
191 with a spec, 192 in the catalogue, and both figures are already in the sentence. At 736f9451 the generator and the committed document agree:
$ out="$(podman run --rm --network=none -v "/tmp/r20p456:/repo:z" -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim cargo test -p sysknife-daemon --test action_reference_doc --offline 2>&1)"; rc=$?; echo "rc=$rc"; printf '%s\n' "$out" | tail -12
rc=0
Running tests/action_reference_doc.rs (.container-target/debug/deps/action_reference_doc-037fecbc659a973e)
running 7 tests
test diff_message_names_the_missing_tail ... ok
test diff_message_names_the_line_and_both_sides ... ok
test diff_message_returns_none_when_documents_match ... ok
test first_diff_line_reports_a_truncated_document ... ok
test first_diff_line_names_a_differing_line ... ok
test first_diff_line_returns_none_for_identical_documents ... ok
test action_reference_doc_is_current ... ok
test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
To see what your change would produce if it built, I repaired the syntax on top of d93f42de and left your arithmetic alone, so the format string reads _{} and takes total + 1 as an argument:
$ sed -n '139,147p' crates/sysknife-daemon/tests/action_reference_doc.rs
out.push_str(&format!(
"---\n\n_{} actions have an `ActionSpec` and are tabled above. The \
full catalogue (`KNOWN_ACTION_NAMES`) also includes `ListJobHistory`, \
which the dispatcher handles before the executor, for **{}** total._\n",
total + 1,
total + 1
));
out
}
$ out="$(podman run --rm --network=none -v "/tmp/r20p456:/repo:z" -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim cargo test -p sysknife-daemon --test action_reference_doc --offline 2>&1)"; rc=$?; echo "rc=$rc"; printf '%s\n' "$out" | tail -25
rc=101
[... six passing unit tests ...]
test action_reference_doc_is_current ... FAILED
---- action_reference_doc_is_current stdout ----
thread 'action_reference_doc_is_current' (103) panicked at crates/sysknife-daemon/tests/action_reference_doc.rs:209:9:
docs/action-reference.md is out of date with the action catalogue.
First difference at line 401:
committed: _191 actions have an `ActionSpec` and are tabled above. The full catalogue (`KNOWN_ACTION_NAMES`) also includes `ListJobHistory`, which the dispatcher handles before the executor, for **192** total._
generated: _192 actions have an `ActionSpec` and are tabled above. The full catalogue (`KNOWN_ACTION_NAMES`) also includes `ListJobHistory`, which the dispatcher handles before the executor, for **192** total._
test result: FAILED. 6 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
"192 actions have an ActionSpec" is the false sentence, and action_reference_doc_is_current refuses it.
What #455 is asking for
The two numbers in that footer are right by coincidence. Somebody typed + 1, and one action happens to bypass the executor. Nothing compares either figure against KNOWN_ACTION_NAMES, so when a second dispatcher-internal action arrives the document keeps counting one and every gate stays green. The issue carries the mutation that shows it: change total + 1 to total + 2, regenerate with UPDATE_ACTION_REFERENCE=1, then run the test binary normally. Today that sequence passes.
The fix has three parts:
- Move
DISPATCHER_INTERNAL_ACTIONSout ofcrates/sysknife-daemon/tests/action_consistency.rsto somewhere both test binaries can read. Each file undertests/compiles to its own binary, so auseacross them will not build. That is the hour this costs if nobody warns you. - Derive the tabled count and the total from
KNOWN_ACTION_NAMESand that list, and name the bypassing actions from the list rather than spellingListJobHistoryinto the string. - Write the assertion first, watch it fail on the
total + 2mutation, then make it pass.docs/action-reference.mdshould end up byte-identical; only the derivation changes.
Take the issue back if you want it. It is open and unassigned, and the analysis you did to find line 140 still applies. Before the next push, run cargo test -p sysknife-daemon --test action_reference_doc locally: that package needs no GUI libraries and no database, so it builds on a plain Rust toolchain.
One unrelated note. #457 branches off this commit rather than main, so it carries the same compile error into an otherwise separate change. Rebasing #457 onto main clears that.
Context
The action-reference footer test () generates documentation footer text that incorrectly states the count of tabled actions.
Bug
Line 140 said:
But
ListJobHistoryis part ofKNOWN_ACTION_NAMES(the full catalogue) even though it has noActionSpecand is not included in thecatalogue()table. The footer already says the full catalogue total istotal + 1(includingListJobHistory), so the first sentence must also usetotal + 1.Fix
Changed
_{total}to_{total + 1}in the format string, so both mentions of the catalogue total are consistent.Closes #455