Repository navigation
Strip narration comments and close the doc-coverage gaps #41
Description
Part of the whole-repo quality audit (2026-09-30). Comment hygiene per @antislop-code, doc coverage per docs/dev/standards.md §5.1-§5.4. No code behaviour changes.
What to build
Comments that only earn their line by explaining why; every .rs file carrying its mandated //! module doc; every real pub item documented.
Deletions (narration, dividers above the name they repeat, decision-log, stale history)
- 48 test-module
// ── fn_name ──dividers, each sitting directly above thefnit names, acrossiptables.rs,policy_route.rs,dns_parser.rs,models.rs,auto-discover/daemon.rs,forwarding.rs,lab-lib/docker.rs,api.rs,natmap/docker.rs,tests/auto_discover/mod.rs. Keep the two that carry a reason (iptables.rs:94explains the pure-builder split,iptables.rs:1113explains why the manager is not tested) and the §5.3-mandated top-level dividers. - 22 line-by-line narrations of shell arrays in
tests/natmap_docker.rs, plus a parenthetical change-history aside. - 19 narrations in
crates/natmap/src/iptables.rs, including one stale reference:// cmd_success logs on failnames a function that does not exist (it isrun_success), and one wrong comment:// Delete by line number from bottom updescribes behaviour the code does not have. - 14 narrations of match arms and asserts in
crates/auto-discover/src/daemon.rs. - Over-explained blocks in
crates/lab-lib/src/port.rs(a 7-line load-check preamble, a 5-line retry explanation) and three decorative box banners. - 9 narrations in
crates/natmap/src/api.rs, including a 4-line block duplicated verbatim at two sites. - 7 in
crates/natmap/src/daemon.rs, 6 incrates/lab-lib/src/docker.rs, 4 insrc/main.rs, 8 of history inprotocol.rsanddocs/dev/modules.md. - Seven
///doc comments that only restate the signature or the field name.
Additions (the standards the code currently misses)
- 23 files missing the
//!module doc §5.1 mandates:natmap/src/{completions,api,consts,policy_route}.rs,auto-discover/src/{daemon,config,cli}.rs,src/{consts,cli}.rs,natmap/tests/{model,cli}.rs,tests/natmap_docker.rs,tests/cf2ansible.rs, and all eighttests/auto_discover/*.rs. - 19 real
pubitems missing///per §5.2, concentrated inauto-discover/src/config.rs(17),natmap/src/{daemon,api,policy_route,models}.rs,auto-discover/src/daemon.rs,lab-lib/src/{lib,consts,port,protocol}.rs,src/{lib,cmd/mod}.rs,natmap/src/{cli,completions,consts}.rs, and thetests/auto_discover/mod.rshelpers. crates/natmap/src/command.rsis named explicitly in §5.3 and exceeds 200 lines, but has zero section dividers. Add them at the natural boundaries.
Corrections to stale docs
docs/dev/modules.md:42citesnatmap/src/port.rsandauto-discover/src/port.rs; neither file exists.docs/dev/modules.md:82,245carry change-history parentheticals.docs/dev/testing.md:53claims 88 inline unit tests. The real count is 421 tests and roughly 9761 test lines. Fix the number.
Acceptance criteria
- Every remaining comment explains why, verified against §5.4
- Every
.rsfile starts with//! - Every
pubitem outside alib.rsmodule list has a///; thepub modlines in the fourlib.rsfiles are documented too -
docs/dev/modules.mdcites only files that exist - No production code changed
Blocked by
None (can start immediately).
Estimated: -132 comment lines, +docs.
Activity
docs: strip the narration, add the docs the standard mandates
Merge docs/41-comment-hygiene into v0.2: strip the narration, add the…
FAZuH commentedon Oct 1, 2026
Landed on
v0.2as745fa3a, mergede2b6c07. Closed by hand: a closing keyword only fires from commits reaching the default branch.No executable code changed, and that is verified rather than asserted
Stripping
//,///and//!from every touched file and comparing the whitespace-normalised remainder againstHEADleaves 39 of 40.rsfiles byte-identical. The one that differs does so only by a#shell comment inside a raw string intests/auto_discover/preserve_src_ip.rs, which is not Rust.Line count, measured
The additions are not optional.
standards.md§5.1 and §5.2 mandate 21//!module docs and 43///on realpubitems, so they were not trimmed to flatter the net number. Each is one line; the eight that run to two carry a second fact.Two opposite actions on the same pattern, and why
// --- Section ---dividers tocrates/natmap/src/command.rs, which §5.3 names explicitly and which is 434 lines with none. They name a section, so they navigate the file.fnthey named. A name above the thing it names repeats the next line.Three survive, all earning their line:
iptables.rs:94andpolicy_route.rs:14name the pure-builder split and why it exists, andlab-lib/src/docker.rs:298names a fixtures group. Zero box-drawn====banners remain in the tree.Kept against the ticket's instruction
iptables.rsrule_exists: the ticket called// cmd_success logs on failstale, which is correct. It was replaced, not deleted, with the reason the helper usesrunoverexec:-Cexits non-zero when the rule is absent, and that is the answer rather than a failure.flush_chain: a two-line note added on thelet _ =swallow, which is deliberate and reads as an oversight without it.lab-lib/src/port.rs: retry preamble 5 lines to 3, load-check note 7 to 4. The "load check, not reproduction" distinction is the reason that test can never be red.natmap_docker.rsloopback-MASQUERADE header: left intact. It is the packet-level explanation of why the rule exists and the bug it reproduces.Stale items from earlier tickets, folded in
Seven, all the same class of defect:
models.rs:154(claimedpreserve_src_ipskips a MASQUERADE on a DNAT mapping, which has none — that is the hairpin path, andHairpinConfig::lan_cidrat:196already said it correctly); fourpolicy_route.rsdividers naming functions that no longer exist;docs/dev/testing.md:179("& sleep 2" has been false since #47);docs/dev/testing.md:178("--test-threads=1" is enforced by nothing — now says so and cites #54);preserve_src_ip.rs:206; anddocs/dev/modules.md:42.Verification
./dev.sh format,./dev.sh lint,./dev.sh testall exit 0. 463 passed, 0 failed, 10 targets, at loadavg 1.41/2.67 before and 3.02/2.92 after.The 462 figure I quoted in the brief was wrong. It was arithmetic on per-ticket deltas rather than a measured run, and the honest total is 463. It was caught by stashing the diff and measuring the base at the same 463, which also confirms this change did not move the count. Recording it because a number I asserted without measuring is exactly the error this batch has been correcting.
Skipped, with a ticket
The per-file test counts in
docs/dev/testing.mdnow claim 45 integration, 94 Docker and 5 doc tests, and are further out of date than they were. Restating them needs a measured run per target, not a comment pass, and a wrong number is worse than a stale one. Filed separately.