Skip to content

Close the polish items the reviews left - #88

Merged
hdkiller merged 8 commits into
developfrom
codex/review-polish
Sep 10, 2026
Merged

hdkiller merged 8 commits into
developfrom
codex/review-polish

Conversation

@hdkiller

Copy link
Copy Markdown
Contributor

Summary

One bounded maintenance pass closing the small polish items the cold reviews
of the earlier pull requests left. No behaviour changes: the JSON envelope,
the operation list, the exit codes, and every golden output are exactly what
they were.

Scripts and workflow edits in this pull request are pre-approved by the
operator for this run.

What changed

Tests

  • crates/openpapir-cli/tests/bench.rs: the count assertions look each
    measurement up by the name it was measured under instead of by its position
    in the list, so an invocation added or reordered cannot move an assertion
    onto another one. IMPORT_BATCH is asserted equal to the core
    limits::MAX_IMPORT_FILES at compile time rather than trusted to stay in
    step by hand.
  • crates/openpapir-cli/tests/property.rs: a new guard,
    the_command_tree_declares_no_hidden_argument_or_subcommand. The man page
    derivation skips a hidden subcommand (manpage.rs) and a hidden argument
    (clap_mangen), so a hidden one would be accepted at the command line and
    would appear in no page, and the walk the property reads its known names
    from would then call a real name an undefined one. The bash completion
    script is generated from the same definition by a generator that renders
    every subcommand and every long flag whether hidden or not, so it is the
    second opinion: every command it declares must have a page, and every long
    flag it declares for a command must be in that page. Verified by marking one
    subcommand and one flag hidden locally: both fail the guard, and the tree is
    otherwise identical apart from the two differences the generators own
    themselves (clap's help and its mirrors, and the --version the man page
    derivation adds to every page), which the test documents and excludes.
    The walker trim set also gains ,, so \fB\-h\fR, reduces to h rather
    than to the h, artefact.

Core

  • One shared case-insensitive fold, records::fold and
    records::folded_contains, used by both case list --query and search.
    Byte-identical behaviour: both sites folded with str::to_lowercase before
    and fold with the same call now. No golden changed.
  • archive/import.rs: one sentence on Histories stating what it holds in
    memory (one entry per distinct digest an import event names, each holding
    that digest's own events).
  • records/search.rs: the doc comment now states that a record directory that
    cannot be listed reads as empty, exactly as it does for case list, and
    that no absence of hits may be read as evidence.

Docs

  • docs/archive-layout.md: one sentence for the other direction of a purge
    that stops between the two passes, an object whose unlink fails after its
    derived record was removed, which is left without one until the next
    archive derive.
  • docs/releasing.md: the two script and workflow facts below.

Packaging

  • scripts/package-release.sh: --describe folds its sentence to at most 76
    columns as it is written, at spaces only, so a backticked path is never
    split and the width does not drift as the tables the sentence names grow.
    The longest line is now 76 columns; it was 110.
  • .github/workflows/ci.yml: the packaging step removes its staging directory
    before staging, so the step can be run again in a working copy that has
    already run it rather than being refused as a staging that already holds
    files.

Verification

  • ./scripts/check.sh && cargo build --release --locked && cargo run --locked -p openpapir-cli -- capabilities --json
  • cargo clippy --workspace --all-targets --locked --target x86_64-pc-windows-msvc -- -D warnings
  • cargo +1.88 check --workspace --all-targets --locked
  • cargo llvm-cov --workspace --locked --fail-under-lines 90: 96.54% of lines.
  • OPENPAPIR_BENCH_CASES=20 cargo test -p openpapir-cli --test bench -- --ignored,
    so the keyed assertions are exercised rather than only compiled.
  • shellcheck scripts/package-release.sh and actionlint.

Goldens changed

None.

Exceptions to owned paths

None. docs/releasing.md and .github/workflows/ci.yml are both listed.

No CHANGELOG.md entry: nothing here changes the CLI contract, which is what
CONTRIBUTING.md says is logged today, and openpapir-core has no published
version, so the two new crate-private helpers are not contributor-visible.

@hdkiller

Copy link
Copy Markdown
Contributor Author

Review findings addressed at the new head.

  1. Blocking, the help exemption. is_help_mirror now requires a name to be a derivable mirror before it is left out of the comparison: at the first help in the path, the part before it has to be a declared command, and the part after it has to be empty, or help again, or name a declared command under that same parent. openpapir-help-case-create and openpapir-archive-help-check still qualify, so the mid-path mirrors are still excluded, while a hidden help-topics does not, because the tree holds no openpapir topics. Proven with the three scratch cases: #[command(name = "help-topics", hide = true)] on Skill fails with the man page stream holds a page for openpapir-help-topics, #[command(hide = true)] on Skill fails on openpapir-skill, #[arg(long, hide = true)] on capabilities' --json fails with openpapir-capabilities declares an argument no page renders: ["--json"], and the unmodified tree passes.

  2. declared_commands now enters each command into the map from its own label as the label is read, and the opts line extends that entry, so a command declares itself whether or not its block turns out to name a flag.

  3. fold_prose records whether -f was already set and restores it, so it settles nothing for the rest of the script.

Rebased onto develop after #87. Both sides kept: the sentence now folds THIRD-PARTY-NOTICES.md in and stays at 76 columns, where it would otherwise have run to 110, and the notice lint step in CI is untouched. Re-verified on the rebased tree: ./scripts/check.sh, release build, capabilities --json, Windows-target clippy, cargo +1.88 check, coverage 96.54% lines, shellcheck, actionlint, and the packaging step run twice in a row in the same working copy.

@hdkiller
hdkiller merged commit d3b1400 into develop Sep 10, 2026
14 checks passed
@hdkiller
hdkiller deleted the codex/review-polish branch September 10, 2026 14:18
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.

1 participant