Skip to content

fix(fsck): reject ambiguous arguments and separate failure classes - #13

Merged
farhan-syah merged 3 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/fsck-cli-read-only-proof
Jul 26, 2026
Merged

fix(fsck): reject ambiguous arguments and separate failure classes#13
farhan-syah merged 3 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/fsck-cli-read-only-proof

Conversation

@presempathy-awb

@presempathy-awb presempathy-awb commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

fix(fsck): reject ambiguous arguments and separate failure classes

Summary

pagedb-fsck scanned std::env::args() permissively: the last --realm won,
a --realm with no value fell back to the default realm, unknown --options
became the KEK, and extra positional tokens were ignored. An operator who
mistyped a command got a filesystem diagnosis for a problem that did not exist,
or — worse — a clean report for the wrong realm.

This PR resolves the whole command line into a validated value before any key
decoding, VFS construction, or database open, and makes each failure class the
tool can encounter distinguishable from the others.

fsck is a diagnostic trust boundary. Its output is only useful if "I typed
the command wrong", "the store will not open", and "the store is damaged" are
three different answers.

The grammar

pagedb-fsck <path> [--deep] [--page-size <bytes>] [--realm <hex16>] [<hex-kek>]

Every previously valid invocation still parses, in any argument order. What is
now rejected:

  • a required path slot holding an option token;
  • any option appearing twice;
  • an option that takes a value running off the end, or being handed an option;
  • an unknown option — including single-dash tokens, which previously fell
    through to the positional slot and resurfaced as invalid hex KEK for
    something the operator never meant as a key;
  • more than one positional KEK.

--help / -h prints the grammar on stdout and exits 0. It is answered from
any position and before the path check, because a tool that has just tightened
its grammar has to be able to state that grammar — and a missing path is
exactly when you would ask.

Failures name their specific condition through a typed CliError, not a
string, so the parser's tests assert on the condition rather than on wording.

Exit codes

Previously everything that was not a usage error exited 1: an absent store, a
wrong key, and an actually damaged database were indistinguishable to any
caller that was not a human reading stdout.

Code Meaning
0 Opened cleanly; with --deep, the report was clean
1 An integrity problem was found
2 The command line was invalid; the store was never opened
3 The store could not be opened, or the report could not be written

This also fixes let _ = report.write_text(...): a verdict that could not be
delivered was exiting 0, reporting "clean" to a caller that never saw why.

This is a compatibility break for scripts that treat every non-zero exit as
one class. Those scripts could not have acted on the old code differently
anyway, which is the point.

--page-size

While reworking the argument path: Db::open_read_only was being called with a
hardcoded 4096. Header B lives at byte offset page_size, so fsck could
not open any store not created at 4096 bytes
— it read the wrong bytes and
blamed the header. pagedb supports 4096 through 65536 (tests/page_size_range.rs).

--page-size makes those stores inspectable. A failed open now also names the
page-size and realm assumptions it used, rather than leaving an operator
hunting for corruption that is not there.

This is wider than argument validation, and deliberately so: it is a store the
checker simply could not read.

Read-only contract

The byte-preservation test runs the real binary as a separate process rather
than inferring read-only behaviour from which constructor appears in source:
build a store with one committed key and one sealed, catalog-linked segment;
snapshot every authoritative main.db and seg/** byte; run
pagedb-fsck --deep --realm <zero> <kek>; require success and result: CLEAN;
compare every path and byte.

It now also asserts the snapshot actually contains a segment entry. Without
that, a future layout change would silently reduce the comparison to main.db
alone and the segment half of the claim would pass while covering nothing.

Layout

src/bin/pagedb-fsck.rs becomes a directory module, since the grammar now has
real types:

  • main.rs — entry point and the wasm32 stub, nothing else
  • cli.rs — grammar, CliError, parse, and its unit tests
  • run.rs — exit-code policy, key and realm decoding, open, deep walk

The binary also gains #![warn(clippy::all, clippy::pedantic)]. It had no lint
attributes before, because a binary does not inherit them from lib.rs.

ExitCode is now behind the native cfg, removing an unused-import warning
from wasm32 builds.

Tests

14 parser unit tests beside the code, and 7 integration tests driving the built
binary. The unit tests cover the direction a stricter parser actually
endangers — that valid commands still parse. --deep --realm X KEK,
KEK --realm X --deep, and two further orderings must all yield the identical
CliArgs.

Integration coverage: byte preservation, PAGEDB_KEK fallback, a shallow open,
a non-default page size with and without the override, the rejection table,
--help on both spellings, and the exit-code taxonomy (usage vs. absent store
vs. wrong key vs. a store whose live segment has been damaged).

Each fix was confirmed to fail without itself, not merely observed green:

Stubbed out Tests that fail
is_option restricted back to -- short-option unit test, rejection table
--help handling removed help unit test, help integration test
EXIT_OPERATIONAL collapsed back to 1 exit-code taxonomy, page-size
Page size hardcoded to 4096 page-size

Verification

Run against this branch with main merged in:

cargo nextest run --all-features --no-fail-fast
  442 passed; 4 skipped   (main is at 421)

cargo clippy --all-targets --all-features -- -D warnings
  clean

cargo fmt --all --check
  clean

cargo test --doc --all-features
  clean

cargo check --target wasm32-unknown-unknown --bin pagedb-fsck --features opfs
  clean (4 pre-existing library warnings in journal/segment code, none in the binary)

Scope

No change to page layout, AAD, encryption, allocation, free-list format,
recovery, the public library API, or any dependency. Db::open_read_only, the
retention policy, the VFS, and the deep-walk implementation are untouched — the
only call-site change is passing the requested page size instead of a constant.

The README documents the grammar, the defaults, --page-size, and the exit
codes.

Split pagedb-fsck.rs into a directory module (cli/run/main) so grammar
parsing, execution, and exit-code policy are separately testable.
Adds --page-size for stores created at a non-default page size and a
--help/-h flag, and gives usage errors, an unreadable store, and an
actual integrity failure distinct exit codes (2, 3, 1) instead of one
generic non-zero.
@farhan-syah farhan-syah changed the title fix(fsck): reject ambiguous arguments and prove frozen inspection fix(fsck): reject ambiguous arguments and separate failure classes Jul 26, 2026
@farhan-syah
farhan-syah merged commit 4b5dd19 into NodeDB-Lab:main Jul 26, 2026
18 checks passed
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