PST Query — Code Review Summary (2026-08-29)

Overview

PST Query (pstq) is an offline, read-only CLI that indexes one Outlook PST into a disposable SQLite + FTS5 cache and exposes deterministic, agent-oriented JSON retrieval. The codebase is roughly 2,900 lines of source across five modules (cli.py, index.py, pst.py, metadata.py) with an equal amount of tests (~2,860 lines, 113 test functions) and a configured target of 100% coverage.

This is well-engineered code. It has clear module boundaries, immutable dataclasses, careful separation between the read-only pypff adapter and the cache layer, atomic cache swaps via temp-file-and-rename, source-change detection guarding every sync, exclusive-create output for extracted attachments, and genuinely thorough docstrings that double as the agent contract. The findings below are refinements, not signs of a troubled codebase. Only one is a correctness bug of note, and it is confined to a diagnostic command.

Note: the review is static. The environment had no runnable Python interpreter, so the suite could not be executed here; the task records claim uv run tox passes at 100% coverage.

What is done well

Safety model. PST is always opened read-only; the SQLite cache is explicitly disposable and rebuilt on schema/cleaner-version drift.
Atomicity. Imports and incremental syncs write to a sibling temp DB and os.replace into place only after re-verifying the source is unchanged (_assert_source_unchanged).
Adapter isolation. pst.py never leaks pypff objects; property access is defensively wrapped, and store identity is derived from PidTagRecordKey.
Determinism. All JSON output is sorted-key, indented, and body-free where the contract promises it; stable STORE_UID:NID identifiers throughout.
Documentation. Command docstrings specify request, result schema, limits, and source/cache access. Architectural decisions are captured as ADRs under docs/adr/.

Findings

HighBug

1 · `snapshot` silently writes partial snapshots on traversal error

TASK-002

pstq/metadata.py — inspect_pst() · pstq/cli.py — snapshot()

inspect_pst wraps the folder walk in a broad try/except that appends to scan_errors and continues, then builds a snapshot from whatever partial data was gathered. The snapshot command writes report.snapshot and prints "Wrote snapshot with N messages" without ever checking scan_errors.

A walk that fails part-way through a large or partly corrupt PST therefore yields a truncated snapshot reported as complete. Since snapshots feed compare-snapshots, a truncated file mis-classifies large numbers of messages as missing/new — defeating the purpose of the diff. The inspect command prints scan errors; the durable-artifact path does not.

report = inspect_pst(path, sample_size=0)
write_snapshot(report.snapshot, output)          # scan_errors never inspected
click.echo(f"Wrote snapshot with {report.message_count} messages to {output}")

Fix: refuse to write (exit non-zero, standard error envelope) when scan_errors is non-empty; leave no partial file behind.

MediumBug / Docs

2 · `attachment` overwrite behavior contradicts its documentation

TASK-003

pstq/pst.py:343 · pstq/cli.py:505 · README.md

extract_attachment opens the target with exclusive creation (destination.open("xb")) and re-raises FileExistsError, so an existing file is never replaced. The command help states the opposite: "names a new output file; existing files may be replaced by the operating system."

Additionally, FileExistsError is an OSError, which the CLI maps to the source_error code — misattributing an output-path conflict to the PST source. Pick one contract (reject vs. overwrite), make code/help/README agree, and give the conflict a distinct error code.

MediumPerformance

3 · `thread` loads the entire store to reconstruct one conversation

TASK-005

pstq/index.py — get_thread() / _thread_members()

Each thread call runs SELECT … FROM message WHERE store_uid = ? across every message and builds Message-ID/reference maps and a neighbor graph in Python over the whole archive. Cost is O(total messages) in time and memory per lookup, and the relationship columns are unindexed. Correct, but it does not scale to large PSTs (the sample archive is ~2.4 GB). Seed the graph from a bounded candidate set (topic / conversation-index root) and/or index the relationship columns.

MediumPackaging

4 · `pypff` is required at runtime but undeclared; install docs mislead

TASK-006

pyproject.toml · uv.lock · README.md

The tool cannot touch a PST without pypff, yet it appears in neither pyproject.toml nor uv.lock, while the README leads with pip install pstq. Following the README yields a package that imports fine but fails at first PST access. Document the libpff prerequisite and the real install path (devcontainer / pinned wheel), or declare it as a dependency/extra. Also drop the stray blank lines in the pyproject classifiers/dependencies arrays.

MediumHygiene

5 · Local scratch files committed to the repository

TASK-004

test.json · test_config.yaml

test.json is an ~80 KB dump of a single retrieved message including the raw HTML body of what looks like a real sample-archive message; test_config.yaml is a developer config pointing at machine-local temp/ paths. Both are tracked at the repo root. Remove them from version control and extend .gitignore. Prefer a sanitized template if an example config is wanted.

LowPolish

6 · Fragile `--json` error detection & silent no-command invocation

TASK-007

pstq/cli.py — CliContractGroup / _json_requested / main()

The error-envelope format is chosen by "--json" in args over raw argv, decoupled from Click's parsing: a positional value equal to --json would flip a text-mode command into JSON error output. Separately, invoke_without_command=True with argument pass-through means running pstq with no subcommand exits 0 silently and a mistyped command is swallowed as an onacol config token. Tie format detection to the parsed option and show help/usage when no subcommand resolves.

Findings at a glance

#	Finding	Severity	Type	Task
1	snapshot writes partial data on scan error	High	Bug	TASK-002
2	attachment overwrite contract vs. docs / error code	Medium	Bug / Docs	TASK-003
3	thread reconstruction scans whole store	Medium	Performance	TASK-005
4	pypff undeclared; install docs mislead	Medium	Packaging	TASK-006
5	committed local scratch files	Medium	Hygiene	TASK-004
6	--json detection & no-command UX	Low	Polish	TASK-007

Observations (no task)

Body cleaner is English/Outlook-only. clean_body recognizes only -----Original Message----- and English From/Sent/To/Subject header blocks. Other locales/clients pass through uncleaned. This is an acknowledged, intentional conservative design (per TASK-001.11) — noted for awareness.
Incremental sync copies the full cache each time. _incremental_sync performs a full sqlite backup of the existing index before applying deltas. This is the price of the atomic-swap guarantee and is safe; worth remembering for very large caches (the sample index is ~259 MB).
FTS error detection is string-based. search_messages classifies an FTS syntax failure by substring-matching the SQLite error text. Pragmatic and adequate, but tied to message wording.

Generated as a static review of PST Query. Actionable items are tracked as TASK-002 through TASK-007 in backlog/tasks/.

Overview

What is done well

Findings

1 · snapshot silently writes partial snapshots on traversal error

2 · attachment overwrite behavior contradicts its documentation

3 · thread loads the entire store to reconstruct one conversation

4 · pypff is required at runtime but undeclared; install docs mislead