diff --git a/.agents/skills/gitops-principles/SKILL.md b/.agents/skills/gitops-principles/SKILL.md new file mode 100644 index 00000000..81f3459a --- /dev/null +++ b/.agents/skills/gitops-principles/SKILL.md @@ -0,0 +1,19 @@ +# GitOps Principles Skill — prei + +## When to load +- Before editing anything under `.github/workflows/` +- Before touching deployment config, the GitOps manifest repo, or `docker-compose.yml` +- When reasoning about rollback, canary, or progressive-delivery behavior + +## Principles + +1. **Git is the source of truth.** Config, workflows, deployment state — all in git. Never modify running infrastructure directly. +2. **Immutable artifacts.** The Docker image is the deployable unit. Test the artifact, not the source. Never rebuild for deployment. +3. **PR gates are deploy gates.** Every merge to `main` is a deploy candidate. Broken `main` blocks all PRs — fix main CI before merging anything else. +4. **Declarative pipelines.** Workflows describe DESIRED STATE: what artifacts, what gates, what triggers. Not imperative scripts. +5. **Artifact verification.** Every PR that touches workflows or deployment config must be verified against the live container via `post-deployment.yml`. +6. **Rollback is `git revert`.** Rollback to a previous commit on the GitOps manifest repo. The rollback job in `post-deployment.yml` is a safety net, not the primary mechanism. +7. **Observability built-in.** Every workflow step logs `job-start` / `job-finish` timestamps. Build times, test results, deploy status are traceable. +8. **Progressive delivery.** Canary → staging → production (see `docs/DEPLOYMENT_STRATEGY.md`). Never ship to 100% in one step. +9. **Naming is infrastructure.** PR titles follow Conventional Commits. Commit messages describe intent. Tags trigger deployments. +10. **Branch discipline.** All work happens on feature branches off `main` (trunk-based development, short-lived). Never commit directly to `main`. Branch naming: `feat/`, `fix/`, `chore/`, `docs/`. Every branch opens a PR through CI gates before merge. diff --git a/.github/scripts/flaky_report.py b/.github/scripts/flaky_report.py new file mode 100644 index 00000000..200f43c9 --- /dev/null +++ b/.github/scripts/flaky_report.py @@ -0,0 +1,140 @@ +#!/usr/bin/env python3 +"""Detect flaky tests from a pytest --report-log JSONL file. + +A test is "flaky this run" when pytest-rerunfailures had to rerun it +(outcome == "rerun" on an earlier "call" report) and the final "call" +report for the same nodeid passed. Tests that still fail after retries +are real failures, not flaky ones, and are left alone. + +Modes: + --mode report Print a markdown summary (and append to + $GITHUB_STEP_SUMMARY if set). Does not touch the ledger. + --mode write Same summary, plus updates docs/quality/flaky_tests.json + and tests/.flaky_quarantine.txt. Intended to run only + from the single main-branch writer job. +""" + +from __future__ import annotations + +import argparse +import json +import os +import sys +from datetime import datetime, timezone +from pathlib import Path + +DEFAULT_LEDGER = Path("docs/quality/flaky_tests.json") +DEFAULT_QUARANTINE = Path("tests/.flaky_quarantine.txt") +DEFAULT_THRESHOLD = 3 + + +def find_flaky_nodeids(report_log_path: Path) -> list[str]: + """Return nodeids whose call reports show rerun-then-pass this run.""" + call_outcomes: dict[str, list[str]] = {} + + if not report_log_path.exists(): + return [] + + with report_log_path.open() as f: + for line in f: + line = line.strip() + if not line: + continue + record = json.loads(line) + if record.get("$report_type") != "TestReport": + continue + if record.get("when") != "call": + continue + nodeid = record.get("nodeid") + outcome = record.get("outcome") + if nodeid is None or outcome is None: + continue + call_outcomes.setdefault(nodeid, []).append(outcome) + + flaky = [] + for nodeid, outcomes in call_outcomes.items(): + if "rerun" in outcomes and outcomes[-1] == "passed": + flaky.append(nodeid) + return sorted(flaky) + + +def load_ledger(ledger_path: Path) -> dict: + if not ledger_path.exists(): + return {} + with ledger_path.open() as f: + data: dict = json.load(f) + return data + + +def update_ledger(ledger: dict, flaky_nodeids: list[str], threshold: int) -> dict: + now = datetime.now(timezone.utc).isoformat() + for nodeid in flaky_nodeids: + entry = ledger.get(nodeid, {"count": 0, "first_seen": now}) + entry["count"] = entry.get("count", 0) + 1 + entry.setdefault("first_seen", now) + entry["last_seen"] = now + entry["quarantined"] = entry["count"] >= threshold + ledger[nodeid] = entry + return dict(sorted(ledger.items())) + + +def write_quarantine(ledger: dict, quarantine_path: Path) -> None: + quarantined = sorted( + nodeid for nodeid, entry in ledger.items() if entry.get("quarantined") + ) + quarantine_path.parent.mkdir(parents=True, exist_ok=True) + with quarantine_path.open("w") as f: + for nodeid in quarantined: + f.write(f"{nodeid}\n") + + +def render_summary(flaky_nodeids: list[str], ledger: dict) -> str: + if not flaky_nodeids: + return "No flaky tests detected this run.\n" + + lines = [ + "| Test | Cumulative count | Quarantined |", + "| --- | --- | --- |", + ] + for nodeid in flaky_nodeids: + entry = ledger.get(nodeid, {}) + count = entry.get("count", "1 (this run)") + quarantined = "yes" if entry.get("quarantined") else "no" + lines.append(f"| `{nodeid}` | {count} | {quarantined} |") + return "\n".join(lines) + "\n" + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--report-log", type=Path, required=True) + parser.add_argument("--mode", choices=["report", "write"], required=True) + parser.add_argument("--ledger", type=Path, default=DEFAULT_LEDGER) + parser.add_argument("--quarantine", type=Path, default=DEFAULT_QUARANTINE) + parser.add_argument("--threshold", type=int, default=DEFAULT_THRESHOLD) + args = parser.parse_args() + + flaky_nodeids = find_flaky_nodeids(args.report_log) + ledger = load_ledger(args.ledger) + + if args.mode == "write": + ledger = update_ledger(ledger, flaky_nodeids, args.threshold) + args.ledger.parent.mkdir(parents=True, exist_ok=True) + with args.ledger.open("w") as f: + json.dump(ledger, f, indent=2, sort_keys=True) + f.write("\n") + write_quarantine(ledger, args.quarantine) + + summary = render_summary(flaky_nodeids, ledger) + sys.stdout.write(summary) + + step_summary = os.environ.get("GITHUB_STEP_SUMMARY") + if step_summary: + with open(step_summary, "a") as f: + f.write("\n## Flaky test report\n\n") + f.write(summary) + + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/ci-quality.yml b/.github/workflows/ci-quality.yml index dee98b90..9789aac5 100644 --- a/.github/workflows/ci-quality.yml +++ b/.github/workflows/ci-quality.yml @@ -104,6 +104,9 @@ jobs: coverage run --parallel-mode -m pytest tests/ core/tests/ tests_bdd/ prei/pipeline/tests/ \ -q --tb=short \ -k "not e2e and not docker and not integration and not container and not startup and not add_to_pipeline and not acceptance and not export and not unreachable_url" + - name: Flaky test report + if: always() + run: python .github/scripts/flaky_report.py --report-log .pytest-report.jsonl --mode report - name: Upload coverage data uses: actions/upload-artifact@v7 with: @@ -111,6 +114,13 @@ jobs: path: .coverage.* include-hidden-files: true retention-days: 1 + - name: Upload test report log + if: always() + uses: actions/upload-artifact@v7 + with: + name: report-log-unit + path: .pytest-report.jsonl + retention-days: 1 tests-integration: name: "🔗 Integration Tests" @@ -132,6 +142,9 @@ jobs: coverage run --parallel-mode -m pytest tests/ core/tests/ \ -q --tb=short \ -k "integration" + - name: Flaky test report + if: always() + run: python .github/scripts/flaky_report.py --report-log .pytest-report.jsonl --mode report - name: Upload coverage data uses: actions/upload-artifact@v7 with: @@ -139,6 +152,13 @@ jobs: path: .coverage.* include-hidden-files: true retention-days: 1 + - name: Upload test report log + if: always() + uses: actions/upload-artifact@v7 + with: + name: report-log-integration + path: .pytest-report.jsonl + retention-days: 1 tests-e2e: name: "🌐 E2E Tests" @@ -160,6 +180,9 @@ jobs: coverage run --parallel-mode -m pytest tests/ \ -q --tb=short \ -k "e2e or docker or container or startup or add_to_pipeline or export" + - name: Flaky test report + if: always() + run: python .github/scripts/flaky_report.py --report-log .pytest-report.jsonl --mode report - name: Upload coverage data uses: actions/upload-artifact@v7 with: @@ -167,6 +190,13 @@ jobs: path: .coverage.* include-hidden-files: true retention-days: 1 + - name: Upload test report log + if: always() + uses: actions/upload-artifact@v7 + with: + name: report-log-e2e + path: .pytest-report.jsonl + retention-days: 1 typecheck: name: "🔷 Typecheck" @@ -221,6 +251,69 @@ jobs: - name: Verify KPI calculations run: python -m pytest tests/test_finance_math.py -v --tb=short + # ── Authenticated security scan (ephemeral CI instance, not live deploy) ── + # See docs/KNOWN_LIMITATIONS.md LIMIT-22: this scans a freshly-migrated, + # freshly-seeded CI-only instance, not the real deployment. The existing + # unauthenticated full scan in post-deployment.yml still runs against the + # live artifact and is unchanged. + + zap-authenticated-scan: + name: "🛡️ Authenticated ZAP Scan" + runs-on: ubuntu-latest + env: + DJANGO_SETTINGS_MODULE: investor_app.settings_test + SECRET_KEY: ci-zap-scan-key + DATABASE_URL: "sqlite:////tmp/zap_scan.sqlite3" + ZAP_AUTH_USERNAME: zap-ci-scan-only + steps: + - uses: actions/checkout@v7 + - uses: ./.github/actions/python-setup + - name: Generate throwaway scan credential + run: | + echo "ZAP_AUTH_PASSWORD=$(python3 -c 'import secrets; print(secrets.token_urlsafe(24))')" >> "$GITHUB_ENV" + - name: Install dependencies + run: pip install -r requirements.txt -q + - name: Migrate + run: python manage.py migrate --noinput + - name: Collect static files + run: python manage.py collectstatic --noinput --verbosity 0 + - name: Seed ZAP scan user + run: python manage.py seed_zap_scan_user + - name: Render ZAP auth context + run: | + CREDS_B64=$(printf 'username=%s&password=%s' "$ZAP_AUTH_USERNAME" "$ZAP_AUTH_PASSWORD" | base64 -w0) + sed "s/__ZAP_AUTH_CREDS_B64__/$CREDS_B64/" .zap/prei-auth-context.xml > .zap/prei-auth-context-runtime.xml + - name: Start server + run: nohup python manage.py runserver 0.0.0.0:8000 > server.log 2>&1 & + - name: Wait for healthy + run: | + set -e + HEALTHY=0 + for i in $(seq 1 60); do + STATUS=$(curl -sf -o /dev/null -w "%{http_code}" http://localhost:8000/health/ 2>/dev/null || echo "000") + if [ "$STATUS" = "200" ]; then + echo "Healthy after ~$((i * 2))s" + HEALTHY=1 + break + fi + sleep 2 + done + if [ "$HEALTHY" != "1" ]; then + echo "::error::Server never became healthy within 120s" + cat server.log || true + exit 1 + fi + - name: OWASP ZAP Authenticated Full Scan + uses: zaproxy/action-full-scan@v0.13.0 + with: + target: "http://localhost:8000" + allow_issue_writing: false + fail_action: true + cmd_options: "-a -n /zap/wrk/.zap/prei-auth-context-runtime.xml -U zap-ci-scan-only -I" + - name: Server logs + if: always() + run: cat server.log || true + # ── Coverage combine (gate: all test jobs must pass) ────────────────────── coverage: @@ -253,7 +346,7 @@ jobs: pr-gates-pass: name: "All Gates Passed" - needs: [pr-title, lint, secrets, django-checks, tests-unit, tests-integration, tests-e2e, typecheck, acceptance-check, finance-math, coverage] + needs: [pr-title, lint, secrets, django-checks, tests-unit, tests-integration, tests-e2e, typecheck, acceptance-check, finance-math, coverage, zap-authenticated-scan] runs-on: ubuntu-latest if: always() steps: @@ -271,6 +364,7 @@ jobs: if [ "${{ needs.acceptance-check.result }}" != "success" ]; then FAILED="$FAILED acceptance"; fi if [ "${{ needs.finance-math.result }}" != "success" ]; then FAILED="$FAILED finance-math"; fi if [ "${{ needs.coverage.result }}" != "success" ]; then FAILED="$FAILED coverage"; fi + if [ "${{ needs.zap-authenticated-scan.result }}" != "success" ]; then FAILED="$FAILED zap-authenticated-scan"; fi if [ -n "$FAILED" ]; then echo "❌ Failed gates:${FAILED}" exit 1 diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 5d089a87..24fd9511 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -105,7 +105,7 @@ jobs: needs: build-image runs-on: ubuntu-latest permissions: - contents: read + contents: write packages: read steps: - name: "job-start" @@ -216,6 +216,29 @@ jobs: prei-live-test \ python -m pytest tests_bdd/ -q --tb=short 2>&1 echo "=== All BDD acceptance tests passed ===" + - name: Extract test report log + if: always() + run: docker cp prei-live-test:/app/.pytest-report.jsonl .pytest-report.jsonl 2>/dev/null || true + - name: Update flaky test ledger + if: always() + run: | + if [ -f .pytest-report.jsonl ]; then + python3 .github/scripts/flaky_report.py --report-log .pytest-report.jsonl --mode write + else + echo "No report log found, skipping flaky ledger update" + fi + - name: Commit flaky test ledger + if: always() + run: | + if git diff --quiet -- docs/quality/flaky_tests.json tests/.flaky_quarantine.txt; then + echo "No ledger changes" + else + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + git add docs/quality/flaky_tests.json tests/.flaky_quarantine.txt + git commit -m "chore(ci): update flaky test ledger [skip ci]" + git push origin HEAD:main + fi - name: "job-finish" if: always() run: | diff --git a/.github/workflows/post-deployment.yml b/.github/workflows/post-deployment.yml index 73c349ee..f6afeaf9 100644 --- a/.github/workflows/post-deployment.yml +++ b/.github/workflows/post-deployment.yml @@ -107,7 +107,7 @@ jobs: runs-on: ubuntu-latest steps: - name: OWASP ZAP Full Scan - uses: zaproxy/action-full-scan@v1 + uses: zaproxy/action-full-scan@v0.13.0 with: target: ${{ needs.smoke.outputs.target }} allow_issue_writing: false diff --git a/.gitignore b/.gitignore index 2457272e..1fcb087f 100644 --- a/.gitignore +++ b/.gitignore @@ -82,4 +82,6 @@ test-report.md verification-output.md verification-report.md .coverage +.pytest-report.jsonl +.zap/prei-auth-context-runtime.xml drafts/ diff --git a/.zap/prei-auth-context.xml b/.zap/prei-auth-context.xml new file mode 100644 index 00000000..cc45b942 --- /dev/null +++ b/.zap/prei-auth-context.xml @@ -0,0 +1,77 @@ + + + + + prei-ci + 1 + prei ephemeral CI instance - authenticated scan + true + http://localhost:8000.* + http://localhost:8000/accounts/logout.* + + org.zaproxy.zap.model.StandardParameterParser + {"kvps":"&","kvs":"=","struct":[]} + + + org.zaproxy.zap.model.StandardParameterParser + {"kvps":"&","kvs":"=","struct":[]} + + + 2 + EACH_RESP + + + + 60 + REQUESTS +
+ http://localhost:8000/accounts/login/ + http://localhost:8000/accounts/login/ + csrfmiddlewaretoken=&username={%username%}&password={%password%} +
+ + + 0 + + + 0 + +
+ + AND + -1 + + + + + 1;true;emFwLWNpLXNjYW4tb25seQ==;2;__ZAP_AUTH_CREDS_B64__ + + 1 + + RESPONSE + \Q/accounts/logout/\E + \Qname="password"\E + 60 + REQUESTS + + + diff --git a/AGENTS.md b/AGENTS.md index 51a5a6d3..2a857682 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ ## Project Identity - prei: passive real estate investment analytics for buy-and-hold investors. -- Stack: Python 3.14, Django 5.2, DRF, SQLite (default/alpha), Postgres (reserved for post-MVP production — see docker-compose.yml). +- Stack: see requirements.txt/pyproject.toml. Postgres is reserved for post-MVP production — see docker-compose.yml. - Constraints: Decimal money; service-layer boundaries; no Bootstrap. - Design: custom design system with CSS custom properties (tokens.css + base.css). @@ -29,16 +29,7 @@ 9. Use uppercase in PR title description — the first word after `type(scope):` must be lowercase (see `docs/PR_STANDARD.md`). ## GitOps Principles -1. **Git is the source of truth.** Config, workflows, deployment state — all in git. Never modify running infrastructure directly. -2. **Immutable artifacts.** The Docker image is the deployable unit. Test the artifact, not the source. Never rebuild for deployment. -3. **PR gates are deploy gates.** Every merge to `main` is a deploy candidate. Broken `main` blocks all PRs — fix main CI before merging anything else. -4. **Declarative pipelines.** Workflows describe DESIRED STATE: what artifacts, what gates, what triggers. Not imperative scripts. -5. **Artifact verification.** Every PR that touches workflows or deployment config must be verified against the live container via `post-deployment.yml`. -6. **Rollback is `git revert`.** Rollback to a previous commit on the GitOps manifest repo. The rollback job in `post-deployment.yml` is a safety net, not the primary mechanism. -7. **Observability built-in.** Every workflow step logs `job-start` / `job-finish` timestamps. Build times, test results, deploy status are traceable. -8. **Progressive delivery.** Canary → staging → production (see `docs/DEPLOYMENT_STRATEGY.md`). Never ship to 100% in one step. -9. **Naming is infrastructure.** PR titles follow Conventional Commits. Commit messages describe intent. Tags trigger deployments. -10. **Branch discipline.** All work happens on feature branches off `main` (trunk-based development, short-lived). Never commit directly to `main`. Branch naming: `feat/`, `fix/`, `chore/`, `docs/`. Every branch opens a PR through CI gates before merge. +See `gitops-principles` skill — load before touching workflows, deployment config, or the GitOps manifest repo. ## Context Files | File | Why | diff --git a/conftest.py b/conftest.py index ce172ac7..e273d021 100644 --- a/conftest.py +++ b/conftest.py @@ -2,6 +2,7 @@ import os from decimal import Decimal +from pathlib import Path import pytest from django.contrib.auth import get_user_model @@ -11,11 +12,37 @@ User = get_user_model() +QUARANTINE_FILE = Path(__file__).parent / "tests" / ".flaky_quarantine.txt" + def pytest_configure(config) -> None: # noqa: ARG001 os.environ.setdefault("DJANGO_SETTINGS_MODULE", "investor_app.settings_test") +def pytest_collection_modifyitems(config, items) -> None: # noqa: ARG001 + """Quarantine tests flagged as flaky (see docs/quality/flaky_tests.json). + + Quarantined tests keep running and reporting but never fail the build, + so a known-flaky test can't block a PR while it's being fixed. + """ + if not QUARANTINE_FILE.exists(): + return + quarantined = { + line.strip() + for line in QUARANTINE_FILE.read_text().splitlines() + if line.strip() + } + if not quarantined: + return + marker = pytest.mark.xfail( + reason="quarantined: flaky, see docs/quality/flaky_tests.json", + strict=False, + ) + for item in items: + if item.nodeid in quarantined: + item.add_marker(marker) + + @pytest.fixture def user(db): return User.objects.create_user( diff --git a/core/management/commands/seed_zap_scan_user.py b/core/management/commands/seed_zap_scan_user.py new file mode 100644 index 00000000..4a11a802 --- /dev/null +++ b/core/management/commands/seed_zap_scan_user.py @@ -0,0 +1,38 @@ +import os + +from django.contrib.auth import get_user_model +from django.core.management.base import BaseCommand, CommandError + + +class Command(BaseCommand): + help = ( + "Idempotently seed a low-privilege user for authenticated OWASP ZAP " + "scans. Reads credentials from ZAP_AUTH_USERNAME/ZAP_AUTH_PASSWORD " + "env vars. Intended for ephemeral CI databases only." + ) + + def handle(self, *args, **options): + username = os.environ.get("ZAP_AUTH_USERNAME") + password = os.environ.get("ZAP_AUTH_PASSWORD") + if not username or not password: + raise CommandError( + "ZAP_AUTH_USERNAME and ZAP_AUTH_PASSWORD env vars are required" + ) + + User = get_user_model() + user, created = User.objects.get_or_create( + username=username, + defaults={ + "is_staff": False, + "is_superuser": False, + "is_active": True, + }, + ) + user.is_staff = False + user.is_superuser = False + user.is_active = True + user.set_password(password) + user.save() + + action = "Created" if created else "Updated" + self.stdout.write(self.style.SUCCESS(f"{action} ZAP scan user '{username}'")) diff --git a/design.md b/design.md index 68ecdf5c..df035d74 100644 --- a/design.md +++ b/design.md @@ -1,74 +1,92 @@ -# Design: Phase B — Financial Math +# Design: Phase C (partial) — Deployment Reliability -### B-1: IRR reference implementation -`ref_irr(cashflows: list[Decimal]) -> Decimal` in `tests/finance_reference.py` -is independent of `numpy_financial` (unlike production's `irr()`, which wraps -it). It brackets a sign change in `NPV(r) = Σ cashflows[t] / (1+r)^t` over a -coarse grid (`r ∈ (-0.9999, 10)`, step `0.01`), then bisects within the -bracket to a `1e-7` tolerance. Returns `Decimal("0")` when no sign change is -found (no real root), mirroring production's existing NaN/Inf fallback. +### C-2: Authenticated ZAP scan runs against an ephemeral CI instance -### B-2: Expanded edge-case coverage -`tests/test_finance_math.py`'s case lists (`NOI_CASES`, `CAP_RATE_CASES`, -`COC_CASES`, `DSCR_CASES`, `ONE_PCT_CASES`, `GRM_CASES`, new `IRR_CASES`) were -expanded to 50+ rows each, organized by category: normal/typical, zero in -each param position, negative in each param position, extreme magnitude, -currency sub-cent precision, boundary/threshold, and int-vs-Decimal coercion. -`ref_one_percent_rule`/`ref_gross_rent_multiplier` were updated to raise -`ValueError` under the same zero/negative conditions as production, so -zero/negative edge cases can't silently diverge between "production raises" -and "reference returns a value." +Authenticating against the real deployed environment (what +`post-deployment.yml` targets) would require provisioning a scan account and +credentials on that live environment — the same category of infra gap as +C-1/C-3. Instead, a new `zap-authenticated-scan` job in `ci-quality.yml` runs +the authenticated scan against an ephemeral instance spun up inside the job +itself: fresh migrated SQLite DB, `runserver` bound to `0.0.0.0:8000`, scan +account seeded via a new idempotent management command +(`core/management/commands/seed_zap_scan_user.py`, mirroring the +`get_or_create`/`update_or_create` pattern in `seed_markets.py`). +`ZAP_AUTH_USERNAME` is a fixed, non-secret literal in the workflow; +`ZAP_AUTH_PASSWORD` is generated fresh each run (`secrets.token_urlsafe(24)` +in a "Generate throwaway scan credential" step, exported via `$GITHUB_ENV`) — +not a GitHub secret, since the DB is a throwaway SQLite file destroyed at job +end and the value never needs to be reused across runs. -### B-3: No workflow change -`ci-quality.yml`'s `finance-math` job already runs the whole of -`tests/test_finance_math.py`; B-1/B-2 adding IRR cases to that same file -extends the existing gate automatically. +`.zap/prei-auth-context.xml` defines form-based auth against +`/accounts/login/`, with logged-in/out indicator regexes (presence/absence of +`/accounts/logout/` and the password field) and a `` entry containing +a `__ZAP_AUTH_CREDS_B64__` placeholder in place of a literal credential. A +"Render ZAP auth context" step substitutes the placeholder with a base64 +`username=...&password=...` blob built from that run's generated password, +writing the result to `.zap/prei-auth-context-runtime.xml` (gitignored, +never committed) — rather than injecting via a `-P username=... -P +password=...` CLI flag as originally sketched, since `zap-full-scan.py` +doesn't expose a documented flag for that. This avoids ever committing a +credential-shaped value to git history; an earlier version of this file did +embed a static credential blob directly, which GitGuardian correctly flagged +as a hardcoded secret during PR review, prompting this runtime-templating +fix. -### B-4: Derivation docstrings -`noi`, `cap_rate`, `cash_on_cash`, `dscr`, `irr` in -`investor_app/finance/utils.py` gained full docstrings (formula + "Derivation:" -paragraph + Args/Returns), following the Args/Returns/Raises style already -used by `one_percent_rule`/`gross_rent_multiplier`. Those two also gained a -one-line derivation note for completeness. No function bodies changed — -docstrings only, verified via `ast.parse` + full test rerun. +The job runs `zap-full-scan.py -a -n +/zap/wrk/.zap/prei-auth-context-runtime.xml -U zap-ci-scan-only` and is a +required check in `pr-gates-pass`, so it runs +pre-merge on every PR — a stronger "shift security left" posture than the +existing unauthenticated scan, which only runs post-deployment. +`post-deployment.yml`'s scan is left unchanged (defense in depth: one +authenticated pre-merge scan, one unauthenticated scan of the real artifact). -### Underwriting.py: float → Decimal, dedup -`UnderwritingInput`/`UnderwritingMetrics` (`prei/pipeline/handlers/underwriting.py`) -became `Decimal`-typed pydantic models — pydantic v2 coerces int/float/str into -`Decimal` fields natively, so existing bare-numeric call sites keep working -unchanged. The local duplicate `cap_rate()` was deleted; the module now -imports `cap_rate`/`cash_on_cash`/`to_decimal` from `investor_app.finance.utils` -directly. `cash_on_cash_yield()` keeps its distinct name and semantics -(all-cash acquisition yield: NOI over price+rehab, no debt service netted -out) but delegates its division through the canonical `cash_on_cash()` instead -of reimplementing `/` locally. The remaining composition helpers -(`gross_potential_rent`, `effective_gross_income`, `total_operating_expenses`, -`net_operating_income`, `max_allowable_offer`) converted their arithmetic to -`Decimal` via the reused `to_decimal()` helper; `solve_underwriting()` uses -`.quantize()` instead of `round()` for the final output. +Scope decision recorded in `docs/KNOWN_LIMITATIONS.md` LIMIT-22. -`orchestrator.py`'s `UnderwritingInput` construction boundary (`price * 0.012`/ -`price * 0.004` tax/insurance defaults) was wrapped in `Decimal("0.012")`/ -`Decimal("0.004")` arithmetic against a `to_decimal(canonical.price or 0.0)`- -coerced price, reusing `to_decimal()` rather than reinventing coercion. +### C-4: Flaky test detection, ledger, and quarantine -**Two non-obvious risks found during implementation:** -- `pytest.approx()` compared against a `Decimal` actual is - fragile, not uniformly broken — it silently short-circuits via exact - equality for representable values but raises `TypeError` on near-matches - (`abs(expected - actual)` can't mix `float` and `Decimal`). Every affected - assertion site was fixed by wrapping the Decimal actual in `float(...)` - rather than relying on which literals happen to match exactly. -- `Decimal * float` arithmetic (not just comparison) raises `TypeError` - unconditionally. Test-file call sites that computed derived values inline - (e.g. `uw.mao * 1.15`, `low.mao * 0.07 / 0.10`) needed `float(...)`-wrapping - of the Decimal operand before the float arithmetic. -`prei/pipeline/handlers/offer.py`'s `OfferInput.mao: float` field is untouched -by this — pydantic coerces a `Decimal` input to `float` automatically since no -arithmetic happens before construction at the remaining safe call sites. +`pytest.ini`'s `addopts` gains `--report-log=.pytest-report.jsonl`, provided +by the `pytest-reportlog` plugin (not built into pytest core — this was a +wrong assumption in the original plan, corrected during implementation once +`--report-log` failed with "unrecognized arguments"). With +`pytest-rerunfailures` already wired (`--reruns 1 --reruns-delay 5`), a +rerun-then-pass test shows up in the report log as two `"call"`-phase +reports for the same nodeid: an intermediate `"rerun"` outcome followed by a +final `"passed"` outcome. -### Documentation-only additions -Two new `docs/KNOWN_LIMITATIONS.md` entries (LIMIT-20, LIMIT-21) record the -issues found but deliberately not fixed in this PR: the bare-function vs. -`calculate_*` contract divergence plus the duplicate `score_listing_v2` -functions, and `offer.py`'s remaining float-currency issue. +`.github/scripts/flaky_report.py` parses that JSONL file and detects exactly +that signature (`find_flaky_nodeids`). Two modes: + +- `--mode report`: prints a markdown summary (also appended to + `$GITHUB_STEP_SUMMARY` when set) without touching the ledger. Runs in + `ci-quality.yml`'s `tests-unit`/`tests-integration`/`tests-e2e` jobs on + every PR — visibility without any write contention between concurrent PR + runs or fork-PR permission issues. +- `--mode write`: same summary, plus updates `docs/quality/flaky_tests.json` + (nodeid → `count`, `first_seen`, `last_seen`, `quarantined`) and, once a + nodeid's cumulative count reaches the threshold (default 3), adds it to + `tests/.flaky_quarantine.txt`. Runs only from `docker-publish.yml`'s + `live-test` job (push-to-`main` only) — the single writer for the shared + ledger, avoiding merge conflicts between concurrent PRs. + +Because `live-test`'s BDD suite runs inside a Docker container (`docker exec +... pytest tests_bdd/`), the report log is written to `/app/.pytest-report.jsonl` +inside the container (picked up automatically from `pytest.ini`'s `addopts`, +since the image includes it) and copied out via `docker cp` before the +container is torn down. If the ledger or quarantine file changed, the job +bot-commits as `github-actions[bot]` with `chore(ci): update flaky test +ledger [skip ci]` and pushes directly to `main` (the job's `permissions` gains +`contents: write` for this). + +A new root `conftest.py` hook, `pytest_collection_modifyitems`, reads +`tests/.flaky_quarantine.txt` (if present) and marks matching nodeids +`pytest.mark.xfail(strict=False)` at collection time — quarantined tests keep +running and reporting on every run, but a known-flaky test can never fail the +build while it's being fixed. Verified end-to-end with a throwaway +deliberately-failing test converting to `1 xfailed`. + +### Residual gap + +The `docker-publish.yml` bot-commit step only runs on push-to-`main`, so it +can't be exercised from a PR branch. This is flagged in the PR description as +something to watch on the first post-merge run, not claimed as pre-merge +verified. diff --git a/docs/KNOWN_LIMITATIONS.md b/docs/KNOWN_LIMITATIONS.md index 3bba7212..aeb8f01c 100644 --- a/docs/KNOWN_LIMITATIONS.md +++ b/docs/KNOWN_LIMITATIONS.md @@ -236,6 +236,30 @@ This means a user who runs the API pre-`populate_growth_areas` gets empty result --- +### [LIMIT-22] 🟡 HIGH — Authenticated OWASP ZAP scan runs against an ephemeral CI instance, not the live deployment + +**Location:** `.github/workflows/ci-quality.yml` — `zap-authenticated-scan` job; `.zap/prei-auth-context.xml`; `core/management/commands/seed_zap_scan_user.py` + +**Impact:** `docs/TOP_01_PLAN.md` Phase C (C-2) calls for the full OWASP ZAP scan to be made auth-aware so it can reach pages behind `/accounts/login/`. Authenticating against the *real* deployed environment (the target of `post-deployment.yml`'s existing unauthenticated scan) would require provisioning a scan-only account and credentials on that live environment — this repo has no deploy-pipeline or production-DB access to do that safely, the same infra gap documented for C-1 (canary) and C-3 (SLO dashboard) in `docs/TOP_01_PLAN.md`'s "What You Can't Ship Yet" section. + +**Workaround:** The authenticated scan instead runs pre-merge, inside a new PR-gate job (`zap-authenticated-scan`) against a fresh, ephemeral instance: a newly migrated SQLite DB seeded with a throwaway, low-privilege account via `manage.py seed_zap_scan_user`, torn down at job end. This is strictly a "shift security left" improvement (every PR, not just post-deploy) and involves no production credentials. `post-deployment.yml`'s existing unauthenticated full scan against the live artifact is unchanged, so defense in depth against the real deployment is preserved. + +**Fix tracked in:** Revisit once C-1/C-3 infrastructure (progressive delivery + monitoring stack) exists and a safe way to provision/rotate a live-environment scan account is designed. Not yet filed as a GitHub issue. + +--- + +### [LIMIT-23] 🟡 HIGH — Authenticated ZAP scan's first real run surfaced 10 unresolved WARN-level findings, currently non-blocking + +**Location:** `.github/workflows/ci-quality.yml` — `zap-authenticated-scan` job (`cmd_options` includes `-I`); application code (headers/cookies/templates, not yet touched) + +**Impact:** Once authentication started working (LIMIT-22), the scan reached pages behind `/accounts/login/` for the first time (`/dashboard`, `/growth-explorer`, `/leasing`, `/pipeline//`, etc.) and found 10 categories of WARN-level alerts that had never been scanned before: Cookie No HttpOnly Flag [10010], X-Content-Type-Options Header Missing [10021], Server Leaks Version Information [10036], CSP Header Not Set [10038], Permissions Policy Header Not Set [10063], HTTP Only Site [10106], Session ID Transmitted Insecurely [40013], Sub Resource Integrity Attribute Missing [90003], Cross-Origin-Resource-Policy Header Missing or Invalid [90004], Insecure HTTP Method - PUT [90028]. Zero FAIL-level alerts. `zap-full-scan.py` exits non-zero on any WARN by default, which would have blocked every future PR on pre-existing gaps unrelated to their changes. + +**Workaround:** `-I` (`--ignore-warn`) added to the scan's `cmd_options` so the job only fails on FAIL-level alerts; WARN findings still appear in the job's ZAP log/report for visibility, they just don't block merges. + +**Fix tracked in:** Not yet filed. Recommended as a dedicated hardening PR: Django `SECURE_*` settings + `django-csp`/`django-permissions-policy` middleware for the header findings, `SESSION_COOKIE_HTTPONLY`/`SESSION_COOKIE_SECURE` for the cookie/session findings, `integrity` attributes on external `