docs: add a development spec; sync the RPM spec with the tree - #10
docs: add a development spec; sync the RPM spec with the tree#10davdunc wants to merge 5 commits into
Conversation
The repo had no development spec — CONTRIBUTING.md covered environment setup and quality gates, but nothing stated module boundaries, the provider contract, path ownership, or what "shipped" means for a command. docs/SPEC.md records the rules that already hold and names the ones nothing enforces. The import graph is verified, not aspirational: paths imports nothing internal, analysis never imports data, nothing imports cli. That second one is what keeps tests/test_analysis.py hermetic, so it is worth stating. Six known gaps are listed rather than hidden. Two matter most, and they are the same failure twice: blotter is advertised by PR #2 and ships as a module but is not registered as a command, and the RPM spec's Requires/Commands drift from pyproject and --help with nothing checking either. packaging/tradekit.spec had gone stale at 0.1.0 across four merged PRs: - Version 0.1.0 -> 0.3.0 - Add Requires: python3-matplotlib >= 3.11.1 (blotter PNG rendering) - Correct the command list: add news, regime, sandwich, second-day - Personal_AI_Infrastructure -> LifeOS in the description - Move completions to %{_datadir}/bash-completion/completions - Fix completion generation and %check: both invoked the buildroot entry point with no PYTHONPATH and could never have imported the package - Stop masking %check with `|| :` — a smoke test that cannot fail is not a test - Drop %config(noreplace) from a .example file; it is documentation Not validated with rpmspec/rpmbuild — neither is installed here.
Three fixes to the previous commit's work, all found by re-checking claims against the code rather than against intuition. The %check comment asserted the CLI smoke test "would have caught an unregistered command." It would not. `tradekit --help` exits 0 whether or not a module is wired to a @cli.command(), which is exactly how blotter shipped unreachable in the first place. Replaced with what the test actually catches — an import-time error or a missing runtime dep — and a pointer to gap G1. A spec that documents a check as stronger than it is, is worse than one that omits it. Requires: python3-matplotlib now carries its justification. The dependency is real even though no command exposes blotter: the module ships in the package and %pyproject_check_import imports it during the build. Without the note the next person reading the gap list would reasonably delete the Requires and break %check. SPEC.md gains an import rule and two corrections: - New rule 6 — import public names across package boundaries. reports/html.py:323 does `from tradekit.data.finviz import _trade_review_day_dir` while the public paths.trade_review_day_dir is what it wants. Recorded as G7. This is NOT a layering violation, reports may import data; it couples a renderer to another package's private helper. finviz's own wrapper delegates to paths, so the "nothing outside paths.py builds a path" rule does hold. - G2 was understated: the version disagrees in four places, not three. CHANGELOG's top heading is the fourth. Nothing reads __version__ at all.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds comprehensive development documentation (SPEC.md) and syncs the RPM spec to 0.3.0. The documentation is thorough and valuable for the project.
Critical Issue
Version Mismatch Blocks Build: The spec declares Version 0.3.0, but according to SPEC.md gap G2, pyproject.toml is at 0.2.0. This causes Source0 to resolve to a non-existent git tag, making the spec unbuildable. Per the documented release checklist in SPEC.md (lines 206-214), pyproject.toml must be bumped and the tag created before this spec can successfully build.
Recommendation
Following the release checklist documented in this PR: bump pyproject.toml to 0.3.0 and create the v0.3.0 tag before merging, or revert the spec Version to 0.2.0 to match the current state.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
|
||
| Name: tradekit | ||
| Version: 0.1.0 | ||
| Version: 0.3.0 |
There was a problem hiding this comment.
🛑 Build Failure: Version 0.3.0 does not match pyproject.toml (0.2.0). Source0 resolves to ` which will fail when the v0.3.0 tag doesn't exist. This makes the spec unbuildable until pyproject.toml is bumped and the tag is created, violating the release checklist requirement that all version locations must agree before tagging.
…down CI caught this: `ruff format --check .` formats Python inside markdown code fences, so docs/SPEC.md is subject to the formatter. `ruff check` is not — it skips markdown entirely, which is why a local `ruff check docs` reported "no Python files found" and the problem reached CI. Recorded in SPEC.md's CI gate table so the next person writing a ```python block in docs does not lose a CI cycle to it.
Found while checking whether this PR's CI failure was self-inflicted. It is
not: the same 3 tests in tests/test_reporting.py fail on origin/main, verified
by checking main out and running them there.
The cause is a half-finished security fix. reporting/ingest.py:34 sets
DEFAULT_ACCOUNT_KINDS = {} with the comment "Load account mappings from
environment or config file instead of hardcoding". Removing hardcoded broker
account ids from a public repo was right. The replacement loader was never
written, so ingest.py:82 falls back to AccountKind.LIVE for every account and
a SIM account reports as LIVE.
That matters more than a red CI badge. LIVE and SIM carry independent risk
units, and telling them apart is the one distinction the whole risk model
rests on. Right now it silently does not hold.
Recorded as G8 rather than fixed here — this PR is documentation, and the fix
is a behaviour change that deserves its own review.
CI note — the 3 test failures are pre-existing on
|
Amazon Q flagged that the spec declares 0.3.0 while pyproject sits at 0.2.0, so Source0 resolves to a tag that does not exist and the spec cannot build. Correct, and it is a fair catch on this PR specifically: SPEC.md's own release checklist says bump CHANGELOG, then pyproject, then the spec. The previous commit did step three alone. - CHANGELOG: [Unreleased] -> [0.3.0], populated from what actually landed after the 0.2.0 tag — the canonical reporting layer (#8), the blotter module (#2, #7), XDG paths (#4), the PAI -> LifeOS rebrand, and the CVE dependency upgrade (#6). The blotter entry says plainly that the module ships without a registered command (gap G1) rather than implying a working feature. - pyproject: 0.2.0 -> 0.3.0. - __init__: __version__ now derives from installed distribution metadata with a dev fallback for an uninstalled source checkout. This deletes the fourth copy rather than updating it, which closes G2 structurally instead of resetting the clock on the same drift. The importlib.metadata import sits at the top of __init__ with os and sys, ahead of the vendor sys.path hook — it is stdlib, so it does not need the hook, and placing it after would trip E402. Verified the hook still works: finvizfinance and ta both import. ruff check and ruff format --check are clean. The 3 test_reporting failures are unchanged and pre-existing on main (gap G8). A tag still has to be cut at merge for Source0 to resolve.
Addressed in 6c9ef0d — version aligned across all four locations@Amazon-Q-Developer's catch is correct, and it lands squarely on this PR: SPEC.md's own release checklist says bump Rather than reverting the spec to 0.2.0, I took the other branch of the recommendation and completed the release:
Reverting to 0.2.0 would have left the spec describing code that is not in the 0.2.0 tarball — the matplotlib dependency, four commands added since, and the XDG layout. The forward bump is the coherent option.
G2 is now marked closed in SPEC.md, with the residual noted: nothing yet asserts the three files stay in agreement, so without the CI check from G4 it will reopen. Still outstanding
|
1 participant
Why
The repo had no development spec.
CONTRIBUTING.mdcovers environment setup and quality gates, but nothing stated module boundaries, the provider contract, path ownership, or what "shipped" means for a command. Separately,packaging/tradekit.spechad gone stale at0.1.0across four merged PRs.docs/SPEC.md(new)Records the rules that already hold, and names the ones nothing enforces. The import graph is verified, not aspirational — derived by grepping actual imports:
analysisnever importingdatais what keepstests/test_analysis.pyhermetic — synthetic DataFrames, no network, no keys. That is worth stating as a rule.Deliberately does not duplicate
falcon-suite-compatibility.md(cross-repo contracts C1/C2/C3) or ADR 0001 (why XDG). Scope table at the top says which document owns what.Seven known gaps recorded rather than hidden
Most notable, and worth a look before merge:
blotteris documented but unreachable. PR feat(reports): round-trip blotter command (tradekit blotter DATE) #2 advertisestradekit blotter DATEandreports/blotter.pyships, but no@cli.command()registers it and--helpdoes not list it. It is not imported anywhere either.pyproject.toml0.2.0 ·__init__.py0.1.0 ·tradekit.spec0.3.0 ·CHANGELOG.md0.2.0. Nothing reads__version__.Requires:/Commands:lists are documented but unenforced, so they will drift.G1 and G4 are the same failure twice: a claim published with no check that the claim is true.
packaging/tradekit.spec0.1.0→0.3.0,%changelogentry addedRequires: python3-matplotlib >= 3.11.1— genuinely needed:tradekit.reports.blotterships and%pyproject_check_importimports it during the build, even though no command exposes it. Comment explains this so it is not "cleaned up" on the strength of G1Commands:list —news,regime,sandwich,second-daywere missingPersonal_AI_Infrastructure→LifeOSin%description%{_datadir}/bash-completion/completions%check— both invoked the buildroot entry point with noPYTHONPATHand could never have imported the package%checkwith|| :. A smoke test that cannot fail is not a test%config(noreplace)from a.examplefile; it is documentation, not configurationBefore this can build
Version: 0.3.0has no matching git tag.Source0resolves tov%{version}, so the release needs tagging — andpyproject.tomlandCHANGELOG.mdbumping with it — beforerpmbuildor COPR will work. Flagged in the spec header.rpmspecandrpmbuildare not installed on the machine this was written on, so nothing has parsed it. Worth anrpmlintpass before merge.Test plan
ruff checkpassestradekit --help(17 commands)tradekit.reports.blotterconfirmed importable, justifying the matplotlib deprpmlint packaging/tradekit.specrpmbuild -baon a machine with the tooling🤖 Generated with Claude Code
https://claude.ai/code/session_01BcNHYKg3TET8SzbFdyZPZb