Keep review and tests local and reconcile documentation

[tomqwu]

Summary:

• Apply the owner's latest clarification: code review, unit tests, and E2E tests run locally. There is no hosted AI review and Ollama is not a code-review provider.
• Remove codex-review.yml and its 46 provider-specific test cases; add seven local policy regression cases.
• Complete the documentation reconciliation: current testing and local review guides, README, contributor and agent rules, historical report/specification labels, and unavailable index entries.

Changed files:

• 35 Markdown files, one new local policy test file, removal of the hosted review workflow and its test file.
• No application, mobile, dependency, or surviving workflow changes.
• Disable hosted Ollama review and the obsolete Playwright workflow in GitHub. Do not read/delete credentials or change branch protection.

Validation for HEAD 422dc59, base e7b97e2:

• Local make test-all: 1430 passed, 21 skipped. Unit 386; API 444; CLI 16; integration 325; web 225; contract 1; Playwright 33. Church and basketball playbooks ran in both API and browser tiers.
• Command: env VIRTUAL_ENV=/Users/tomwu/Projects/SignUpFlow-production/.venv EMAIL_ENABLED=false SMS_ENABLED=false PYTEST_ADDOPTS='--show-capture=no --color=no' uv tool run --from poetry==1.8.5 make test-all
• Log: /tmp/signupflow-local-review-all.log. Executed in the SignUpFlow checkout. Tests ran before commit; final tracked source was staged unchanged before the full run completed. Black formatting of the new test was followed by a successful focused rerun.
• Fast unit iteration: 385 passed, 21 skipped, 1 slow test deselected. Full unit tier above includes the slow test.
• New policy tests initially failed on the old configuration; fixed a comment-matching false positive, then all seven passed.
• Black, Ruff, actionlint, whitespace and added-secret review passed. All 65 added relative links resolve.
• Local code review by the builder: complete diff and affected workflow/test/instruction sources inspected; no blocking findings. This is not an independent reviewer or GitHub-attested execution. Local results and review apply to the head/base recorded above.

Limitations:

• Local scoped mypy reported 90 errors in unchanged api/models.py and api/security.py, including missing stubs. API code, pyproject.toml, poetry.lock and ci.yml are unchanged from base. Do not describe this local type-check as passing.
• Test warnings: one API Pydantic deprecation warning and 238 web warnings. Existing duplicate test-contract Makefile recipe warning remains.
• No mobile source changes; Flutter tests were not rerun for this correction.
• Historical documentation is not production/security/deployment certification. External URLs and unrelated legacy report links were not certified live.
• Live main protection returned 404 and rulesets were empty. No protection changes. Retired AI/E2E workflow history may remain visible.
• Merge only after current-head hosted static checks pass and GitHub reports mergeable; no hosted AI review is required or requested.

[github-actions]

AI review (Ollama: glm-5.3-flash)

Head: 8113031
Base: e7b97e2

{
  "verdict": "SAFE TO MERGE",
  "summary": "Docs-only reconciliation (33 Markdown files; no code, test, or workflow changes) that establishes docs/TESTING.md as the current testing/merge guide, removes stale static test counts and outdated 'routers not registered' claims from README/AGENTS/CLAUDE/copilot docs, aligns CONTRIBUTING merge rules with agent guidance, and labels historical reports and spec CI proposals as superseded without rewriting original evidence. The content is consistent with the accepted repository policy (local test execution, hosted checks limited to static analysis, PostgreSQL migration validation, and AI review) and does not weaken or remove any test coverage. No security defects, secrets, tenant-isolation changes, or broken contracts found. Local results (1469 passed, 21 skipped; tier counts sum correctly to 1469) are author-reported evidence claims, not independently verified. The main residual risk is the flipped router-state claim (billing/notification registered under /api/v1, SMS at /api/sms), which cannot be confirmed from this docs-only diff; it is plausibly a stale-doc fix per the author's stated api/main.py check, so confirm registration at HEAD at merge time. Remaining findings are nonblocking documentation polish: contradictory 'Current' badges on unavailable historical index entries and unverifiable make targets/anchors referenced by the new guide.",
  "findings": [
    {
      "priority": "P3",
      "path": "docs/INDEX.md",
      "line": 90,
      "detail": "Rows for files marked '(historical file unavailable)' retain old status badges such as '✅ Current', '✅ Complete', '✅ Most current', and '✅ Verified'. The new 'Historical Catalog (2025-10-27)' header disclaims these labels, but the per-row badges still assert currency/verification for files that do not exist. Proposed fix: change those status cells to '📦 Historical/unavailable' so a reader who skips the header is not misled."
    },
    {
      "priority": "P3",
      "path": "README.md",
      "line": 367,
      "detail": "This PR flips four entry-point docs (README.md, AGENTS.md, CLAUDE.md, .github/copilot-instructions.md) from 'billing/email/SMS/notification routers not registered' to 'billing and notification registered under /api/v1, SMS mounted at /api/sms, email service-backed'. No code is in the diff, so this factual claim is unverifiable here; it is plausibly correcting stale docs, but before merge confirm the include_router registrations in api/main.py at HEAD so the docs do not assert live billing/SMS functionality that is actually disabled."
    },
    {
      "priority": "P3",
      "path": "docs/TESTING.md",
      "line": 30,
      "detail": "The new canonical guide references make targets (test-unit-fast, test-web, test-contract, test-e2e, test-mobile) and the distinction that legacy 'make test' runs tests/comprehensive_test_suite.py. These cannot be verified from a docs-only diff. Confirm each target exists in the Makefile (e.g., make -n <target>) so the guide does not document broken local test commands; also confirm the Playwright 1.60.0 pin matches the version actually used for the reported e2e tier."
    },
    {
      "priority": "P3",
      "path": "README.md",
      "line": 390,
      "detail": "The new tenant-isolation caveat links to docs/playbooks/README.md#known-boundaries-and-release-blockers; the anchor's existence is not verifiable from this diff. Confirm the heading matches so the link (which now carries the important 'coverage is not a claim of complete tenant isolation' caveat) does not resolve to a broken anchor."
    }
  ]
}

Diff-only advisory review; CI and GitHub mergeability remain mandatory.

[github-actions]

AI review (Ollama: glm-5.3-flash)

Head: 88ea1bf
Base: e7b97e2

{
  "verdict": "SAFE TO MERGE",
  "summary": "Documentation-only reconciliation across 33 Markdown files; no code, workflow, migration, or configuration changes, so there is no direct correctness, security, or tenant-isolation impact. The change strengthens rather than weakens expectations (CONTRIBUTING now requires 'make test-all' before every PR instead of only for cross-layer changes) and adds honesty hedges ('Coverage is not a claim of complete tenant isolation'). It correctly documents that Actions runs static checks/migration validation/AI review, not tests — consistent with the accepted repository policy; absence of hosted tests is not a blocker. Historical reports and superseded CI proposals are labeled without rewriting evidence, and dead index links are neutralized as text. Two nonblocking accuracy items remain: (1) the PR description records validation against SHA 8113031... while the pushed head is 88ea1bf..., and the 'committed tree is identical' assertion is self-reported — evidence should be re-confirmed against the actual head SHA per the PR's own merge checklist; (2) the flipped router-state claims (billing/notifications under /api/v1, SMS at /api/sms, email service-backed) cannot be verified in diff-only review since api/main.py is not in the diff — confirm include_router calls match before merge so the docs do not replace one false claim with another. Minor cosmetic inconsistencies in docs/INDEX.md status icons are nonblocking.",
  "findings": [
    {
      "priority": "P2",
      "path": "docs/TESTING.md",
      "line": 69,
      "detail": "The PR description reports validation for HEAD 8113031060e4e80c47b8a60c94c20d0e30d8f55f, but the pushed head is 88ea1bf8ee88f06b70068c77d11ab7305293e770. The claim that 'final committed documentation is identical to the source validated' is self-reported and not verifiable here, and the mismatch violates the PR's own step 2 ('Record ... the pushed head SHA'). Because the PR is docs-only the risk is low, but rerun or re-confirm make test-all against the actual head SHA and update the evidence record before merging."
    },
    {
      "priority": "P2",
      "path": "README.md",
      "line": 367,
      "detail": "The rewritten 'Disabled Features' text asserts billing/notification routers are registered under /api/v1, SMS is mounted at /api/sms, and email is service-backed (same claim repeated in AGENTS.md, CLAUDE.md, .github/copilot-instructions.md). api/main.py is not part of this diff, so diff-only review cannot confirm these statements. If registration is not actually present, this replaces stale docs with new misinformation about live attack surfaces. Fix: verify include_router calls and the SMS prefix in api/main.py match the documented paths before merge, or soften the wording to conditional."
    },
    {
      "priority": "P3",
      "path": "docs/INDEX.md",
      "line": 117,
      "detail": "Several entries marked 'historical file unavailable' retain affirmative status icons (e.g., RECAPTCHA_TEST_RESULTS '✅ Tested', ADMIN_TABS_STRUCTURE '✅ Reference', FEATURE_ROADMAP_ANALYSIS '✅ Planning doc', GAP_ANALYSIS_SUMMARY '✅ Most recent'). This is visually contradictory even though the section header disclaims old labels. Fix: normalize these status cells to 'Historical snapshot'."
    },
    {
      "priority": "P3",
      "path": "docs/TESTING.md",
      "line": 64,
      "detail": "The line 'The reviewer accepts local-only execution but still flags incorrect code, security defects...' embeds a description of the AI reviewer's decision posture in repository docs. It currently matches actual policy, but codifying reviewer behavior in-repo invites drift and could be mistaken for an attempt to define review outcomes. Fix: describe only what hosted checks run and defer review criteria to the workflow/review process."
    },
    {
      "priority": "P3",
      "path": "docs/TESTING.md",
      "line": 10,
      "detail": "Documenting 'poetry run pip install playwright==1.60.0' outside the Poetry lockfile institutionalizes a fragile setup that silently breaks after dependency syncs. Fix: add playwright to Poetry dev dependencies (or a dedicated extras group) so 'poetry install' yields a working browser tier, and keep the manual step only as a fallback."
    }
  ]
}

Diff-only advisory review; CI and GitHub mergeability remain mandatory.