v1.0.0-pre-release-assessment

V1.0.0 Pre-Release Assessment

Pragmatic-Programmer-lens audit of duplication, best practices, test coverage, and code quality before the v1.0.0 lock-in.

Audit performed against an isolated checkout of main at fa33721 (D:/dev/mads/pour-assess). All findings cite that tree. v0.2.0 carry-overs are explicitly tagged.

cargo test on main: 576 / 576 pass in ~2.9s. No ignored tests, no compile warnings of note.

Headline

The core capture loop is solid and the test suite is genuinely strong (~28k LOC, ~17k of it tests). What stands in the way of a clean v1.0.0 are not bugs — it is structural debt that will calcify the moment we promise stability:

  1. Three god-modules (tui/form.rs 3461, tui/configure.rs 2469, main.rs 1711) with tangled concerns.
  2. One copy-pasted atomic-write block reproduced ~17× across config.rs plus parallel duplicates in data/{cache,presets,field_presets,history}.rs.
  3. A non-atomic atomic write on Windows (util::atomic_replace) — the foundation every persistence path stands on.
  4. A pervasive enum-of-FieldType model that forces shotgun surgery in 7+ files for every new field type.
  5. The single largest TUI module (tui/configure.rs) has effectively zero rendering or key-handler tests.

None of these are show-stoppers. All of them are exactly the kind of thing v1.0.0 is the last cheap moment to fix.

Resolved (carried from v0.2.0)

These v0.2.0 open items appear to be addressed on main:

  • serde_yaml removed (custom YAML serialization in output/frontmatter.rs)
  • API key migrated to secrets.toml with env-var > secrets > config fallback
  • Percent-encoded API URLs for vault paths with spaces
  • config_version parsing with #[serde(default)] and leading-zero rejection
  • validate_form skips hidden fields; partition_fields filters them from output

Open Bugs

Anything user-visible or correctness-affecting. Bold items are v0.2.0 carry-overs.

Critical

  • util::atomic_replace is not atomic on Windows (src/util.rs:9-20). The doc comment admits a window where dst does not exist between remove_file and rename. A crash or power loss during any config or cache save can leave the user with only config.toml.tmp and no config.toml. Next launch fails. This is the foundation of every persistence path in the app — config, presets, field presets, history, cache. Replace with MoveFileExW + MOVEFILE_REPLACE_EXISTING (or the atomicwrites / fs_err crate). Release-blocker on Windows.
  • FS-transport path traversal on the write path (src/transport/fs.rs:30-36). resolve_path does no .. check. create_file, append_to_file, append_under_heading all use it. Only list_directory_* validates ... A crafted module path or template can write outside the vault. Carry-over from v0.2.0; still not fixed on the write path.

High

  • Cursor arithmetic assumes ASCII (src/tui/form.rs:2205, 2268, 2309, 2385). cursor_position is treated as a byte offset adjusted by ±1. The first emoji or non-Latin character in a textarea backspaces mid-codepoint and panics on value.remove(pos - 1). move_cursor_vertically (3435–3461) compounds it across lines.
  • Strftime injection in render_path and render_append_template (src/output/template.rs:31, 101). now.format(template) runs on the raw user template before placeholder substitution, so any literal % is interpreted by chrono. Carry-over.
  • YAML escaping gaps in format_scalar (src/output/frontmatter.rs:147). Newlines, backslashes, and YAML reserved words are not escaped consistently. Field name validation does not enforce YAML-safe identifiers — a field named name: "x" can break frontmatter outright. Carry-over.
  • Comma-in-value heuristic (src/output/frontmatter.rs:53-60). "a, b, c" becomes a YAML list, "a,b,c" stays a string. Same value, two outputs depending on a literal trailing space. Non-orthogonal field semantics. Carry-over.
  • Silent persistence failures (src/main.rs:378, 410, 432, 481, 621, 717, 728, 766; src/data/history.rs:148, 166, 466). let _ = e; after presets.save(), history summary writes, and similar — comment cites raw-mode terminal, but the user has no signal a save dropped. Surface via app.deferred_stderr or status-bar toast.
  • Duplicate field names within a module silently overwrite in the output HashMap. Sub-field/template duplicates are checked, module-level is not. Carry-over.
  • prefix_len byte-vs-display-width drift — non-ASCII prompts cause cursor drift. Carry-over.
  • eprintln! during raw mode in autocreate.rs garbles the terminal. Carry-over.

Medium

  • resp.text().await.unwrap_or_default() on API error paths (src/transport/api.rs:132, 166, 191, 291, 317) — user sees "API failed (500): " with no body context when the body itself failed to read. Show a placeholder instead.
  • 5-second blanket timeout on every API call (src/transport/api.rs:80). Slow vault directory listings reliably fail. Make per-request, or raise for listing endpoints.

Open Code Quality

A. God-modules

Three files dominate. Splitting them is the largest single quality gain available before v1.0.0.

  • src/tui/form.rs (3461 LOC): top-level render dispatch, field-row rendering, callout-title overlay, preset-save overlay, select dropdown, textarea editor, composite editor, field-preset picker, sub-form, submit-button rendering, and a 960-line handle_key. Natural seams: form/render.rs, form/render_field.rs, form/overlays/{preset,callout_title,sub_form,composite,field_preset_picker}.rs, form/editor/{textarea,select}.rs, form/key_dispatch.rs.
  • src/tui/configure.rs (2469 LOC): ~1000-line handle_key plus six render functions and an auto-save block that duplicates main.rs::build_*_updates. Natural seams: configure/render/{settings,fields,sub_fields,browser,confirm}.rs, configure/key/{settings,fields,sub_fields,browser}.rs, configure/auto_save.rs.
  • src/main.rs (1711 LOC): bootstrap (1–132), event loop (134–356), and 21 handlers including ~180 lines of settings→Updates conversion. Should be ~80 LOC. Natural seams: cli/args.rs, cli/handlers/{preset,configure,module,sub_field,template}.rs, cli/updates.rs.
  • src/config.rs (~2700 LOC): 32 pub mutation methods on Config. Two responsibilities welded together — a parsed-config value and a TOML-backed store. Extract ConfigStore (or submodules per aggregate: module_ops, field_ops, sub_field_ops, vault_ops, secrets_ops, template_ops).
  • src/app.rs (1265 LOC) — App is also a settings-builder factory. build_field_settings, build_sub_field_settings, init_vault_configure, init_new_module_configure belong next to ConfigureState, not on App.

B. Duplication (DRY violations)

  • 17× atomic-write block in src/config.rs (453, 550, 694, 895, 969, 1044, 1208, 1269, 1335, 1512, 1545, 1677, 1798, 1894, 1960, 2037, 2123). Identical: with_extension("toml.tmp") → fs::write → atomic_replace. Only one site cleans up the orphan tmp file on failure. The other 16 leak config.toml.tmp if rename fails. Replace with one helper: Config::edit<F: FnOnce(&mut DocumentMut) -> Result<()>>(&self, f).
  • splice_under_heading is duplicated between src/transport/api.rs:345-410 and the matching block inside src/transport/fs.rs:140-202. A bug fix in one will silently miss the other. Extract to output/markdown.rs or transport/heading.rs.
  • Settings → Updates conversion is duplicated. main.rs:1491-1567 build_module_updates ≈ tui/configure.rs:64-147 auto_save_module_settings. Same shape repeats for vault and sub-field updates. Collapse to one function per *Updates type.
  • Three identical JSON-store implementations: src/data/cache.rs:71-80, src/data/presets.rs:72-81, src/data/field_presets.rs:71-80 — same create_dir_all → to_string_pretty → tmp-write → atomic_replace. Same load_from/load shape. Generic JsonStore<T: Serialize + DeserializeOwned + Default> solves all three.
  • Module-ordering logic duplicated at src/app.rs:417-439 and src/init.rs:74-91 (apply module_order, then alpha-sort the remainder). One bug fix, two places.
  • Two {{key}} substitution loops at src/output/template.rs:43-46 (render_path) and 147-184 (render_append_template) with subtly different rules. Easy to drift apart.

C. Orthogonality / Leaky Abstractions

  • Adding a FieldType is shotgun surgery. Edits required in: config.rs (enum + validation), app.rs (build_field_settings, init_form), tui/configure.rs:212-330 (build_field_updates), tui/form.rs (render + key handling), output/mod.rs::partition_fields, output/template.rs, visibility.rs. The enum-of-types model worked great for v0.x; at v1.0.0, it is the single largest source of feature-velocity drag. Replace with a FieldType trait dispatching render, key_event, validate, to_yaml_value — or accept that v1 ships with the enum and freeze the field-type set.
  • TUI knows about transport. src/main.rs:182-184 calls fetch_dynamic_options from inside the event loop; main.rs:298 builds obsidian:// URIs there too. Push behind an Action → service-layer dispatch.
  • output/mod.rs:39-50 reaches into module.icon and module.daily_link to inject frontmatter keys. Adding a third such key requires editing write_create. Module-level frontmatter contributions should be config-driven.
  • Two definitions of "special YAML word" — YAML_RESERVED in output/frontmatter.rs:147 vs the comma-split list flag in tui/form.rs:746. Render and write disagree about the same value.

D. Error Handling / Panic Discipline

  • expect("reqwest client build failed") at src/transport/api.rs:82. Carry-over. The justifying comment doesn't make it safe — propagate as Result<Self, _>.
  • Three "module already verified above" expect()s at src/config.rs:1186, 1868, 2227. Hold now, but any refactor that adds an early return between check and unwrap silently turns into a panic. Programming-by-coincidence in textbook form.
  • active_field.unwrap() at src/tui/form.rs:1688. The guard above (is_composite && composite_open) does not prove active_field.is_some(). Reachable via state desync.
  • Audit and either justify with // SAFETY: or replace every remaining unwrap()/expect() outside tests/. Establish a project standard: panic on programmer error, propagate on user/IO error.

E. Public Surface Area

  • src/lib.rs re-exports everything pub. Every field on App, FormState, ConfigureState, BrowserState is pub. There is no encapsulation. Once shipped at v1, locking these down forces churn across every screen. Demote to pub(crate) now; promote individual items as need arises.
  • ApiClient::base_url (src/transport/api.rs:92-94) exists only for tests. Gate behind #[cfg(any(test, feature = "test-utils"))]. Same audit for FsWriter::base_path (src/transport/fs.rs:21) — verify whether any prod caller needs it.
  • Four parallel Action enums (src/tui/mod.rs:60-133: Action, DashboardAction, FormAction, ConfigureAction) translated 1:1 by the dispatch in mod.rs:148-. Adding a top-level action means editing all four. Either unify, or push dispatch into the screen modules.

F. Naming, Dead Code, Broken Windows

  • Inline #[cfg(test)] mod tests at src/autocreate.rs:253-333. Last remaining inline test module. Project convention is dedicated tests/ mirrors. Extract.
  • format_value is just format_scalar with a one-line wrapper (src/output/frontmatter.rs:127-144). Dead indirection.
  • Presets::empty() / FieldPresets::empty() (src/data/presets.rs:44-54, src/data/field_presets.rs:50-56) silently save() to an empty PathBuf if used by accident. Footgun by design — gate save() on a non-empty path or remove empty().
  • BrowserState.loading never set to true. Carry-over.
  • Browse errors swallowed via unwrap_or_default(). Carry-over.
  • Stale /// doc comment at src/main.rs:932 precedes handle_add_field but describes a different function. Stale comment is a broken window.
  • TODO on VaultConfig. Carry-over from v0.2.0.

G. Resource & Concurrency

  • Orphan .tmp cleanup is missing at 16 of 17 atomic-write call sites in config.rs, plus the three data/ JSON stores. Repeated rename failures accumulate orphans. Folds into the "one helper" remediation above.
  • event::poll(100ms) blocks a tokio worker (src/main.rs:140-153). ADR-003 covers the synchronous-TUI-async-IO choice; verify the runtime flavor matches the assumption. Unlikely to bite, worth confirming.

Open Tests

cargo test: 576/576 pass. The suite is well-organized (mirroring src/ structure per CLAUDE.md), uses real tempfile integration where it matters, and the output/orchestration.rs E2E coverage (1220 LOC) is genuinely impressive. The gaps are concentrated.

Coverage Gaps

  • src/tui/configure.rs (2469 LOC) has zero rendering / key-handler tests. tests/app_configure.rs (496 LOC) covers init_configure, init_vault_configure, init_new_module_configure, build_field_settings only. render_configure, handle_key, sync_scroll_offset, auto_save_module_settings are dark matter. Carry-over from v0.2.0; single biggest test hole heading into v1.0.0.
  • src/transport/api.rs (410 LOC) is thinly tested (40 LOC). Constructor + one happy-path check_connection. create_file, append_under_heading, list_directory*, encode_vault_path, all error paths (4xx/5xx, timeout, malformed JSON, missing heading) — untested.
  • src/util.rs::atomic_replace has no direct tests. Given the criticality (every persistence path), at minimum: round-trip success on Unix and Windows, behaviour when target is missing, behaviour when target is a directory.
  • No CLI-level E2E tests. tests/main.rs is 23 LOC of one key-event smoke check. There is no test that walks pour <module> → form → submit → file written. The output/orchestration.rs tests come close but enter below the CLI seam.
  • No configure-flow integration test (dashboard → configure → edit module path → save → reload).
  • No show_when rendering test at the TUI layer. App-level visibility logic is well-covered (tests/visibility.rs, 344 LOC); the rendering integration is not.

Test Quality

  • Brittle error-message assertions. ~50 places use .contains("at least one field")-style asserts on error strings, especially in tests/config.rs and tests/config_update.rs. Renaming an error message means chasing the assertion downstream. Switch to typed error variants (ConfigError::AtLeastOneField) or a constant table.
  • TUI tests verify state changes, not rendered output. Most tests/tui_*.rs cases assert active_field incremented or an Action emitted. None render-snapshot. Adding insta for ratatui buffer snapshots on a handful of golden screens (form open, form with sub-form overlay, dashboard, configure list) would catch entire classes of layout regression.

Test Organization

  • Strong: tests mirror src/ per CLAUDE.md. The only divergence is the inline #[cfg(test)] in autocreate.rs (covered above), and the absence of a tests/tui_configure.rs mirror.

Open Docs

Carry-overs and new findings.

  • CHANGELOG.md — empty? Verify. v0.1.0 → v0.2.0 → v1.0.0 entries needed.
  • pour init in README Quick Start. Carry-over.
  • Field name interpolation ({{field_name}} in paths) documented. Carry-over.
  • Consolidated keyboard-shortcut reference. Carry-over.
  • Dashboard keybindings (e, v, n, o, r, Ctrl+Up/Down). Carry-over.
  • "Open in Obsidian" feature (o key). Carry-over.
  • open and percent-encoding in tech stack. Carry-over.
  • src/util.rs in architecture overview. Carry-over.
  • date_format in config reference. Carry-over.
  • README config_version migration claim — aspirational, not real. Either implement or soften the claim. Carry-over.
  • Design spec deviation annotations sweep. Carry-over.
  • NOTES index, vault README, design spec section numbering fixes. Carry-over.
  • Append-mode filesystem fallback behavior documented. Carry-over.
  • 3-tier vs 4-tier fallback terminology unified across docs. Carry-over.
  • ADR-004 (or update ADR-001) covering the resolved-on-main items: secrets.toml flow, percent-encoding, config_version semantics.
  • Project standards doc (the v1.0.0 "clamp down" deliverable) — error-handling policy, pub discipline, file-size budget, test-coverage minimum per new module, atomic-write helper as the single sanctioned pattern.

Suggested ordering, smallest-blast-radius first:

  1. Foundations (must-fix before tag)

    • Replace util::atomic_replace with a real Windows-atomic primitive.
    • Add .. traversal check to transport/fs.rs::resolve_path write paths.
    • Direct tests for atomic_replace and resolve_path.

  2. De-duplication (mechanical, high leverage)

    • One Config::edit helper, collapse 17 atomic-write blocks.
    • Generic JsonStore<T>, collapse three data/* saves.
    • Extract splice_under_heading to one place.
    • Collapse build_*_updates duplication between main.rs and tui/configure.rs.

  3. Encapsulation (the v1.0.0 lock-in moment)

    • Demote pub → pub(crate) across lib.rs and state structs.
    • Gate ApiClient::base_url (and any siblings) behind cfg(test) / test-utils feature.
    • Audit and resolve every non-test unwrap()/expect().

  4. Decomposition (largest quality win)

    • Split tui/form.rs, tui/configure.rs, main.rs along the seams identified above.
    • Move App::build_*_settings next to ConfigureState.

  5. Test the dark matter

    • Add tests/tui_configure.rs for handle_key + render — target ~40–60 cases.
    • Expand tests/transport/api.rs for non-happy paths.
    • One CLI-level E2E test per write mode.
    • Add insta snapshot tests for 4–6 golden TUI screens.

  6. Carry-over bugs (strftime, YAML escape, comma heuristic, duplicate field names, cursor width, eprintln! in raw mode) — fold into the relevant decomposition / dedup work.

  7. Field-type model decision (deliberate, before tag) — either invest in a FieldType trait or freeze the enum at v1.0.0 and document the lock-in.

  8. Docs sweep + project-standards doc — closes the loop on "clamp down on project standards" for v1.0.0.

What is not on This List

  • Async refresh of dynamic selects — deferred per ADR-003-Synchronous-TUI-Async-Operations.
  • History pruning, undo before write, plugin/extension system — out of v1.0.0 scope.
  • Theme configurability — design-spec deviation, acceptable for v1.0.0.

Review against the working web branch before tag — feature work in flight may resolve or mutate items here.