Vektaskill

Vektaskill (3.0.0) review - ID#71259403-f6b9-4d95-a235-5c0269d20187
Approved with explicitly stated remaining limitations.

The export feature is implemented, authorized correctly, and verified live against the running application, including wrong-tenant and anonymous refusal behavior. Two findings remain open (one P2, one P3), neither of which blocks approval on its own. CI for the exact commit could not be inspected from this review environment; the review request explicitly accepted CI as out of scope, so it stands as a stated limitation rather than a blocker. Confirm a green CI run for commit 4f2a9c1 before merging.

Review mode: Change review. Scope: the CSV export delivery at commit 4f2a9c1 against comparison base 9b31d02; the rest of the product was not re-reviewed. Controlling requirement: docs/prd/exports.md section 3. Risk classification: high, read off the diff 9b31d02..4f2a9c1 — a new export route over tenant data; first supplied round of this delivery, earlier history unknown. No run is cited from an earlier commit: every count below was taken on 4f2a9c1. Date: 2026-09-26 12:00 UTC. Host, model and reasoning setting: unknown in this fictional example. The user explicitly chose chat only; no history files written.

Findings

[P2] F-71259403-f6b9-4d95-a235-5c0269d20187-01 — Wrong-tenant refusal has no automated regression test

File: tests/export/test_report_export.py

Line: 88

Exposure: release-path — the export route is registered and shipped at commit 4f2a9c1; classification verified in app/routes/exports.py, not taken from the delivery report.

Impact: a later refactor of tenant scoping could regress without any test failing; affects tenant data isolation; the regressed defect would be triggerable by any authenticated admin of another tenant. The current behavior is correct — this is an evidence gap, not a live defect, which is why it is P2 and not P1.

The wrong-tenant case returns 404 when exercised live, but no automated test covers it. The test file covers authorized export, empty result, and malformed date range only. If a later refactor of ReportExportService.authorize regresses tenant scoping, no test will fail. Required to close: an automated route-level test that requests another tenant's report as an authenticated admin and asserts a 404 with an empty body and no export row written.

[P3] F-71259403-f6b9-4d95-a235-5c0269d20187-02 — Documentation describes the export as streamed, but the implementation buffers

File: docs/exports.md

Line: 31

Exposure: theoretical — documentation text only; no code path is involved.

Impact: readers may assume constant-memory streaming; no runtime behavior is affected at the current maximum report size.

The documentation states that exports are "streamed row by row." The implementation in app/services/report_export.py line 54 builds the complete CSV in memory before responding. For the current maximum report size this is harmless, but the documentation overstates the behavior. Required to close: correct the documentation or change the implementation, and state the actual memory bound either way.

Evidence sections

1. Code review

Status: Verified.

Read app/services/report_export.py, app/routes/exports.py, and the changed authorization helper in full at commit 4f2a9c1. Authorization resolves the report through the requesting admin's tenant (tenant_id taken from the session, not from the request), so cross-tenant IDs cannot be bound. Validation rejects malformed date ranges before any database read. Refusals raise before the export row is written. Limitation: the shared CSV serialization helper was reviewed only at its call site, not line by line.

2. Targeted tests

Status: Verified.

Ran pytest tests/export/ -v at commit 4f2a9c1: 11 passed, 0 failed, 0 skipped. The claimed count of 42 refers to the full suite, not this feature; the feature itself has 11 tests. One test, test_export_empty_range, failed on the first run and passed on two reruns; the failure was a fixture ordering flake, not a behavior change — the initial failure is reported here regardless.

3. Full verification

Status: Verified.

Ran the project's own commands from Makefile: make lint, make typecheck, make test. Results: lint clean, typecheck clean, full suite 42 passed, 0 failed, 1 skipped (test_smtp_integration, skipped in all environments without an SMTP sandbox, unrelated to this delivery).

4. Measurement tools and artifacts (Not applicable)

5. Database

Status: Verified.

Inspected the development database read-only. The report_exports audit table exists with the expected columns, a foreign key to tenants, and a NOT NULL constraint on requested_by. The authorized live export wrote exactly one row with the correct tenant, user, and byte length. The refused wrong-tenant attempt wrote no row, confirmed by row count before and after.

6. HTTP

Status: Verified.

Tested the real framework route through the running application, not an extracted handler. Authorized request: 200, text/csv, correct Content-Disposition filename, body checksum matching the database byte length. Anonymous request: 302 to login, no body. Authenticated admin requesting another tenant's report ID: 404 with an empty body — the error does not reveal whether the foreign report exists.

7. GUI

Status: Partially verified.

Clicked through the complete flow live as a tenant admin: report list, export button, downloaded file matching the HTTP response. The developer's screenshot was not relied on as evidence. Limitation: only the Chromium engine was tested, and the non-admin role was verified only to the point of confirming the export button is not rendered — not that every non-admin path refuses.

8. Security

Status: Verified.

Selected S01 (tenant/role binding), S03 (HTTP/browser output), S04 (CSV/query interpretation) and S06 (export disclosure). Execution: ordinary integration verification in the explicitly authorized disposable local stack; no probing outside that scope. The successful authorized export was the positive control; refused requests returned no report data and produced no export rows, checked against pre-request state. The changed attack surface is the new export route and its authorization path. Tested anonymous access, wrong role, wrong tenant, fabricated report IDs, malformed date ranges, and an oversized range (10 years: refused with 422). CSV formula injection checked: cell values beginning with =, +, -, @ are prefixed on write. SQL injection not separately tested because all queries go through the ORM with bound parameters, confirmed by code review. No security defects were found within the verified attack surface using the tests that were performed.

9. Dependencies and supply chain

Status: Verified.

The delivery adds and changes no dependencies: the manifest and lockfile are untouched in the diff at commit 4f2a9c1, no git, URL, or path dependencies exist in the project, and no install or lifecycle scripts changed. No vulnerability scan was warranted for this change; the project documents no organizational dependency policy to check against.

10. Product requirements

Status: Verified.

The controlling requirement is docs/prd/exports.md section 3: admins export their own tenant's reports as CSV with an audit trail. Implementation, tests, database evidence, and HTTP evidence each trace to it. The requirement does not demand streaming; the streaming claim exists only in docs/exports.md (finding P3).

11. CI

Status: Not verified.

CI exists and the controlling merge checklist requires it, so it is applicable. This review environment has no network access, so the CI run for commit 4f2a9c1 could not be inspected. Local full verification passed, but local verification is not CI. The review request explicitly accepted CI as out of scope for this review; that explicit acceptance is the only reason an unverified evidence lane on a high-risk change does not force "Not approved." Confirm a green CI run for this exact commit before merging.

12. Still unverified

• CI status for commit 4f2a9c1 (blocker: no network access from the review environment).
• Behavior above the current maximum report size; no load test was performed.
• Non-Chromium browsers.

13. Final decision and requirements for approval

Approved with explicitly stated remaining limitations. The unverified CI lane was explicitly accepted by the user in the review request; without that acceptance, this decision would be "Not approved." Required before merge: confirm green CI for 4f2a9c1. Carry-forward list for the implementer — repeat both items in the next delivery report and in the phase completion report, where they will be re-judged: add the wrong-tenant regression test (P2) and correct the streaming claim in the documentation (P3). No P0 or P1 findings are open. Teardown: verified, no review-created resources remain — the disposable clone and the service stack started from it were removed and their absence confirmed.

14. ⚠️ Human in the loop - Action needed ⚠️

• Open GitHub Actions for commit 4f2a9c1, confirm that the required workflow completed successfully, and return the run URL or a screenshot showing both the commit and the green result. This blocks merge. The P2 test and P3 documentation correction are not listed here because the implementation model can perform them.