diff --git a/docs/architecture/audit-2026-06.md b/docs/architecture/audit-2026-06.md index 1d58910..f00aa1d 100644 --- a/docs/architecture/audit-2026-06.md +++ b/docs/architecture/audit-2026-06.md @@ -353,3 +353,114 @@ will mark its return value tainted — including sanitizers, whose whole job is make that value safe. Read the SARIF `codeFlows` before acting on a `clear-text-*` alert; the source node, not the sink, is what tells you whether the finding is real. + +## September 2026 — tech-debt audit (appended 2026-09-04) + +A tech-debt sweep run by Claude (Opus 5) against `main` @ `2833bcf`, after +fast-forwarding a nine-commit-stale local checkout. Every finding below was +checked against the June and July sections first, so nothing already closed, +rejected by ADR, or verified false is re-flagged here. + +**Verdict: the codebase is in good shape.** Zero `TODO`/`FIXME`/`HACK` markers in +application code, no coverage `omit` list hiding anything from the 89% floor, +three skipped tests (two of them `UPDATE_GOLDEN` guards), no `xfail`/`skipif` +anywhere, 28 modern dependency pins with nothing a major version behind, and +consistent 3.11 targeting across Dockerfile / ruff / mypy / CI. 44,968 lines of +application code against 37,754 lines of tests. The register itself is working: +it correctly steered this pass away from three dead ends. + +### Fixed in this pass + +| PR | Ticket | What landed | +|---|---|---| +| [#116](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/pull/116) | DEPLOY-005 | Pinned universe source lists now resolve from `UNIVERSE_SOURCE_DIR` (anchored to `PROJECT_ROOT`) instead of `UNIVERSE_DIR` (which follows `DATA_DIR`). See below — this was a production defect, not debt. | +| [#117](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/pull/117) | QUAL-008 | `.pre-commit-config.yaml` pinned `ruff-pre-commit` at v0.15.1 while `constraints.txt` pinned `ruff==0.16.3`. Bumped, and `test_supply_chain_policy.py` now enforces the alignment that the config file's own comment had only asked for. | +| [#118](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/pull/118) | DOC-004 | This register section. | + +#### DEPLOY-005 — the one real defect + +`refresh_universe_files()` could not rebuild the three Hemant universes in any +deployment that sets `DATA_DIR`. The pinned source lists were resolved from +`UNIVERSE_DIR`, which follows `DATA_DIR`; but `COPY . .` puts them at +`/app/data/universes/`, so a container with `DATA_DIR=/data` looked on the data +volume and raised `FileNotFoundError`. Because +[render.yaml](../../render.yaml) runs the cron as +`sh -c "python -c '...refresh_universe_files()' && run_daily_scan"` on an +**ephemeral filesystem with no disk at all**, the refresh raised every night and +the `&&` meant the daily scan never executed. The documented first-deploy seeding +step in [operations.md](../operations.md) had the same failure. + +The blueprint comment showed how the gap was reasoned into existence — *"the +nifty/fno lists are downloaded, not baked into the image"*. True for NIFTY/F&O; +the Hemant lists **are** baked in, just at a path nothing looked at. + +Fixed by separating the two concerns onto different anchors — inputs are code and +follow `PROJECT_ROOT`; outputs are deployment state and follow `DATA_DIR`. That +also closed a second-order smell: source and output being one path meant every +refresh rewrote the file it had just read, so the working tree went dirty after +any app run and every committed row carried an absolute developer path in its +`source` column. + +The regression test runs in a **subprocess** with a relocated `DATA_DIR`. That is +deliberate: the faulty binding happened at *import* time, so monkeypatching an +already-imported process would have proved nothing. Confirmed failing before the +change with the exact production error, and passing after. + +**Caveat recorded honestly:** the finding was traced statically through committed +files and reproduced in-process; Docker is not installed on the development +machine, so the container-level run was not performed. If the Render blueprint +was never applied, this was latent rather than live. + +### Open, ranked + +Priority = (Impact + Risk) x (6 - Effort). The top three were filed as issues +straight after this audit and are in flight: [#119](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/issues/119) OBS-004 +(universe erosion, [#124](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/pull/124)), [#120](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/issues/120) QUAL-009 +(app.py coverage, [#123](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/pull/123)), and [#121](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/issues/121) SEC-004 +(pytest out of the production image, [#122](https://github.com/DoRmAmMu1997/Streamlit-Scanner-App/pull/122)). The rest are +recorded here rather than ticketed, so a future pass can pick them up with the +evidence already gathered. + +| Finding | Evidence | Priority | +|---|---|---| +| **Silent universe erosion.** A symbol missing from Dhan's master becomes `missing_security_id` and is filtered out of every scan, but `universe_status()` has exactly one non-test consumer — `ui/status_panel.py`. The headless daily job and `backend/notifications/` never look at `mapping_status`, so a shrinking universe is invisible in production. Measured drift: `hemant_good_200` went 6 to 8 unmapped of 262 during this session. Root cause verified as a genuine vendor change (`JBCHEPHARM` and `GUJGASLTD` are absent from the 2026-08-24 master by symbol *and* company name; the snapshot itself is intact at 213,213 rows), **not** a code bug. | `backend/universe_loader.py:67`, `ui/status_panel.py:83` | 32 | +| **`app.py` sits outside the coverage gate.** `--cov=backend --cov=screeners --cov=ui` omits the 927-line entrypoint despite ~14 test modules exercising it. The command string is asserted verbatim by a policy test, so changing it means re-baselining the floor and co-updating that test in the same PR. | `.github/workflows/quality-and-security.yml:52`, `tests/test_supply_chain_policy.py:55` | 32 | +| **`pytest` ships in the production image.** Declared as a runtime dependency; the Dockerfile installs `requirements.txt` only. Already correctly listed in `requirements-dev.txt`, so this is a duplicate that only widens the production surface. | `requirements.txt:46` | 25 | +| **Score-resolution guard asymmetry.** `_rank_score_and_source` routes `final_score` through `_finite_decimal` (whose docstring names `Decimal('NaN')` as the threat); `_result_score` returns it raw, although the sibling `confidence` path two lines later *does* guard. `_finite_decimal` is byte-identical in three modules. | `backend/scanning/comparison.py:348` vs `backend/storage/repository.py:477` | 25 | +| **The IPO SDK runner lost the shared error contract.** The four `_default_run` copies carry `is_error` / `CLINotFoundError` / `ProcessError` / usage-limit handling 1/2/2/18, 1/2/2/13 and 1/2/2/11 times — and **0/0/0/0** in the IPO extractor. A rate limit the other three surface as a typed error escapes it as a garbage string that fails later in JSON parsing. *This is not a re-proposal of the ADR-rejected boilerplate dedup*: the new information is that the per-agent decision produced a gap in a runner added after that ADR. | `backend/ipo/agents/financial_extractor.py:1692` | 24 | +| **`requirements-optional.txt` is unpinned and never audited.** Bare `TA-Lib` / `pandas_ta`, absent from `constraints.txt`, so `pip_audit -r constraints.txt` never sees them. Compounded by the fact that no test exercises the accelerated branches (`indicators.py:581,601,621,660,708,771,1016,1118`) because CI never installs the packages — the path that runs on a developer machine with TA-Lib installed is never executed by the suite. | `requirements-optional.txt` | 24 | +| **GitHub Actions pinned by mutable tag, one major behind**, with no `.github/dependabot.yml`. `test_supply_chain_policy.py` asserts nothing about action versions or SHA pinning. | `.github/workflows/quality-and-security.yml:28,31,91` | 20 | +| **Documentation drift.** 12 screener modules are documented as 11 in four places and `screeners/ipo_screener.py` is absent from the catalog; the README structure tree omits the entire `backend/ipo/` subsystem and 6 of 16 `ui/` pages; `auth-003-handoff.md:61` documents `count_admins()` when the real name is `count_user_role_admins()`; the HLD roadmap still lists shipped AUTH-003 as backlog; AGENTS.md predates IPO-012/SEC-003. Every markdown link across the four index files does resolve, though. | see each | 20 | +| **Provenance validation implemented three times with divergent rules.** The technical and 67 agents share five identical functions differing in two lines: the 67 agent redacts and length-caps a cached `source_label`, the technical agent does neither. Severity nuance: the technical agent's labels are hardcoded constants, so this is a defense-in-depth inconsistency on a cache-rehydration path, **not** a live secret leak. A third implementation with a third digest rule lives in the repository layer. | `backend/technical/technical_agent.py:922` vs `backend/sixty_seven/agent.py:432` | 18 | +| **The flakiest test in the suite.** A wall-clock assertion (`time.monotonic() - started < 0.20`) against a real `time.sleep(0.25)`. | `tests/test_daily_data_loader.py:485` | 15 | +| **`export_module_compat` is dead code.** Defined to absorb the five-line alias tail all 11 screeners repeat; a repo-wide grep finds exactly one occurrence — its own definition. Its docstring concedes it, and `screeners/ipo_screener.py:317` has already drifted from the convention. Wire it up or delete it. | `backend/scanner_base.py:426` | 10 | +| **`backend/ipo/repository.py` is a service wearing a repository's name.** *Not* an AGENTS.md section 4 violation — checked specifically. But it owns the transaction (`with session_factory()` appears 47 times), does DNS/HTTP/filesystem work between transactions, and needs lazy imports to break an acknowledged extractor-to-repository cycle. Three files named `repository` do three different jobs. Rename to `service.py`. | `backend/ipo/repository.py:1843` | 10 | + +### Local environment drift (not repo debt, but it blocks two gates) + +The development machine runs **Python 3.13.13 with numpy 2.5.2**, while the repo +pins `numpy==2.4.6` and targets 3.11/3.12. Both consequences below were +reproduced on an untouched `main`, so neither is caused by the changes above: + +- `python -m mypy` fails before checking anything — + `numpy/__init__.pyi:737: error: Type statement is only supported in Python 3.12 + and greater`. It passes cleanly with `--python-version 3.13`. +- `pre-commit run --all-files` cannot clone its hook environments + (`InvalidManifestError: ....pre-commit-hooks.yaml is not a file`). CI runs only + `pre_commit validate-config`, which passes. + +Installing the pinned toolchain into a dedicated virtualenv would restore both. + +### Findings verified FALSE in this pass (do not re-flag) + +1. **"The deferred `scan_results(symbol, signal_date)` composite index was never + built."** False. It shipped with VALID-001 — declared at + `backend/storage/models.py:274`, created at + `migrations/versions/20260618valid001_create_signal_forward_returns.py:84`, and + asserted by the migration drift guard at + `tests/test_scan_storage_migrations.py:147`. The doc lines that read "deferred + to VALID-*" are historical notes that VALID-001 then delivered. + +Lesson recorded, and it is the one this register keeps teaching: a design doc's +"deferred" note describes the world at authoring time. Check whether the +follow-up ticket landed before treating it as open work.