Skip to content

fix: repair broken core paths (CLI, YNAB import_id, dead code)#13

Open
demonno wants to merge 11 commits into
masterfrom
fix/broken-core-paths
Open

fix: repair broken core paths (CLI, YNAB import_id, dead code)#13
demonno wants to merge 11 commits into
masterfrom
fix/broken-core-paths

Conversation

@demonno
Copy link
Copy Markdown
Owner

@demonno demonno commented Apr 23, 2026

Summary

Executes spec 2026-04-19-fix-broken-core-paths-design / plan 2026-04-19-fix-broken-core-paths.

  • Delete the broken Revolut provider (fields referenced by the reader did not exist on the CSV model; module was never wired into the factory) and drop its entry from the mypy exclude list.
  • Delete the dead Transformer abstract base (no subclasses after Revolut was removed) and its only, skipped, test.
  • Fix YnabAPIBasedRepository._to_ynab so import_id is unique for same-day same-amount transactions — prevents YNAB from silently deduplicating genuine duplicates. New Counter field is private (init=False, repr=False, compare=False).
  • Fix the CLI: drop the --config option (was passing a Path positionally to pydantic-settings and raising ValidationError on every invocation, then swallowing it and leaving state["config"] unset); wire up the import subcommand to actually call import_transactions.

Test plan

  • make install — clean sync
  • make lint — ruff clean
  • make check — mypy clean (15 files, Revolut exclude removed)
  • make test — 4 passed, 0 skipped (was 2 passed, 1 skipped on master)
  • uv run ynab-import --help--config gone; four commands listed
  • New test_import_id_counter_suffixes_collisions + test_counter_is_per_instance lock down counter semantics
  • Smoke: run uv run ynab-import import end-to-end against a real TBC or Swedbank CSV (not run locally — no valid .env)

Deferred (explicit non-goals, tracked for future specs)

  • ynab_api_key is still a plain strsettings.dict() echoes from check and read still print the token. New run_import does not echo settings (scoped mitigation on this branch).
  • Typer callback now constructs Settings() eagerly, so subcommand --help requires valid env. Acceptable per spec.
  • state["verbose"] is set but never read.
  • InMemoryBasedRepository kept (per spec decision) but still unused.

🤖 Generated with Claude Code

demonno and others added 11 commits April 19, 2026 18:41
Design for deleting dead/broken Revolut module and Transformer abstraction,
repairing CLI config loading and the 'import' command, and adding a counter
to prevent import_id collisions in the YNAB repository.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Task-by-task TDD plan with six tasks: baseline check, Revolut deletion,
Transformer deletion, import_id counter (with failing test first),
CLI cleanup, and final smoke verification.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
The Revolut reader and transformer referenced fields that did not exist
on the CSV model and were never wired into the factory. Removing them
also drops the mypy exclude that was masking the broken signatures.

Co-Authored-By: Claude Code <[email protected]>
README advertised Revolut as a supported bank and CLAUDE.md flagged
Revolut amount parsing as a known issue; both are obsolete now that
infra/revolut/ has been removed.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
The Transformer class had no callers after the Revolut removal. Its
utility methods duplicate CSVReader's. Deletes the only (skipped) test
that exercised it.
- Two supported banks (was: three) after Revolut removal.
- Core modules: drop 'transformers' from description — module deleted.
- Provider pattern: drop optional transformers.py step — no callers.
- Known Issues: drop stale skipped-test entry (test was removed with
  the Transformer class).

Co-Authored-By: Claude Opus 4.7 <[email protected]>
YNAB deduplicates by import_id. With only amount and date in the id,
two identical-amount same-day transactions collide and one is silently
dropped on import. Adds a per-repository Counter keyed on
(account_id, amount, iso_date) that appends :N to the id.
Counter is internal bookkeeping, not a constructor argument. Using
init=False/repr=False/compare=False removes it from the public dataclass
surface (__init__, __repr__, __eq__). Renamed to _counter for
signal-to-reader. New test locks in the per-instance-reset semantic that
create_writer in setup.py depends on.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Settings() already loads from env / .env via pydantic-settings; the
--config Path option was passed positionally to Settings and raised
ValidationError on every run. The handler swallowed the error and
continued, so downstream commands crashed on an unset state['config'].

The 'import' command now calls import_transactions, matching
read-write. Renamed the Python-level function from cli to run_import
to avoid the collision with the Typer app variable. Also fixes the
pre-existing conifg→config typo in echo output.
run_import previously echoed 'Import transactions with config {settings.dict()}'
which printed ynab_api_key (stored as plain str) to stdout. read-write echoes
only 'Importing transactions' — match that to avoid leaking the token.

The underlying root fix (wrapping ynab_api_key in SecretStr) belongs to the
settings cleanup spec; this change removes the widened exposure introduced
by the import-command wiring in the previous commit.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
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