Skip to content

CD fix, S13 agent-surface data layer, and repo-wide comment cleanup - #17

Merged
Badri-Rajendran merged 21 commits into
mainfrom
feature_frontend_completion
Sep 6, 2026
Merged

Badri-Rajendran merged 21 commits into
mainfrom
feature_frontend_completion

Conversation

@Badri-Rajendran

Copy link
Copy Markdown
Owner

Summary

  • CD fix: .github/workflows/cd.yml's worker and mcp-server Container App bootstrap steps were passing --args values that Azure CLI's argument parser silently drops or rejects whenever a token starts with - (confirmed locally against the real az CLI 2.90.0, not just in CI). worker now gets FLASK_APP=wsgi as an env var instead of a CLI arg; mcp-server is invoked via its plain script path (app/mcp/entrypoint.py) instead of -m app.mcp.entrypoint.
  • S13 agent-surface data layer (ADR 24): adds agent_action_request (the MCP write-tool approval queue — a write tool never acts directly, it only ever creates a pending row here, with a two-hop pending → {approved, rejected} → {executed, execution_failed} transition graph enforced by a BEFORE UPDATE trigger) and staff_api_token (SHA-256-hashed MCP bearer credential), plus their repositories, views, and unit/integration/contract tests.
  • Comment/docstring cleanup: shortened comments and docstrings to 1-2 lines across nearly the entire backend and frontend. Comment/docstring text only — no functional changes, no suppression directives (noqa, type: ignore, pragma: no cover) touched.
  • CI scope narrowing (a5afc85, already on this branch): tests, migration check, SAST, secret scanning, Docker build/scan.
  • Adds the project scoring rubric and the frontend-completion design spec to docs/.
  • Reverts a local machine-specific dev-proxy port change (5000→5001) that leaked into a bulk commit — restored to port 5000, which is what the backend serves on everywhere else.

Validation

  • uv run pytest tests/unit — 363 passed
  • uv run pytest tests/integration tests/api — 403 passed, 2 xpassed (pre-existing, unrelated to this change)
  • uv run ruff check — clean on every file touched in this PR (163 pre-existing errors remain elsewhere in the repo, none introduced here)
  • uv run python -c "import app" — imports cleanly
  • npm run lint — clean
  • npm run build — clean, 230 modules
  • CD argument-parsing fix verified directly against the real Azure CLI (2.90.0) locally: reproduced the original failure, confirmed the fix parses correctly (reaches the real Azure API validation stage rather than an argparse error)

Security/DB/API impact

  • New tables: agent_action_request, staff_api_token (migration 20260906_0424-670073c5146c). No RLS policy on either — access is @requires_role("adviser", "admin") on every route, matching S13 §3.1.
  • No changes to existing endpoints' request/response contracts.
  • No secrets or .env files included.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p

Badri-Rajendran and others added 13 commits September 6, 2026 04:34
…scan

Per explicit direction: drop lint (ruff), typecheck (mypy --strict),
import-layering (lint-imports), and dependency audits (pip-audit,
npm audit) as CI gates -- they stay available locally
(make lint/typecheck/imports/audit, npm run lint) but no longer block
a merge. Removed .github/dependabot.yml with them, since its only
purpose was feeding the now-removed dependency-audit jobs.

Kept exactly five jobs, each independently verified to catch a real,
distinct defect class: tests + coverage floor, Alembic migration
drift check, SAST (Bandit + Semgrep), secret scanning (Gitleaks), and
Docker build + Trivy image scan -- kept specifically because it is a
genuine scanner, not a rubber stamp: it already caught a real CRITICAL
CVE in nginx:1.27-alpine earlier this session (fixed in a prior
commit). cd.yml is untouched -- it triggers on the CI workflow's
overall success, not on individual job names, so it needed no change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…in-breaks comments to 1-2 lines

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…ation comments to 1-2 lines

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…comments to 1-2 lines

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
az containerapp create's --args parser drops any value starting with
"-" (confirmed locally: az/argparse discards it silently instead of
erroring cleanly), so "-m" "app.mcp.entrypoint" never reached the
container -- verified against the real az CLI (2.90.0) that both the
space-separated and repeated --args="-m" forms fail or silently lose
the flag. Running the module as a plain file path sidesteps the dash
parsing bug entirely; imports still resolve since app/ is installed
into the venv regardless of invocation style.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…o 1-2 lines

Trims module/function docstrings and standalone comments across
app/controllers, app/jobs, app/mcp, app/workers, config.py, app/__init__.py,
extensions.py, and wsgi.py to 1-2 lines. Suppression directives (noqa,
type: ignore, pragma: no cover) kept verbatim. Comment text only, no
functional changes.
Comment/docstring text only; suppression directives and functional
code untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
Covers app/models/reconciliation, app/services/orders,
app/services/rebalance, app/services/reconciliation, and
app/services/restatement. Comment/docstring text only; fixed a few
new E501s the shortened one-liners introduced by wrapping to two
lines.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
No test logic changed -- comment/docstring text only.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
agent_action_request (two-hop pending/approved/rejected -> executed/
execution_failed transition graph, enforced by a BEFORE UPDATE
trigger) and staff_api_token (SHA-256-hashed MCP bearer credential),
plus their repositories, views, and unit/integration/contract tests.
Backs the MCP write-tool approval queue: a write tool never acts
directly, it only ever creates a pending request here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
A local machine-specific workaround (port 5000 was occupied by
another process during dev) leaked into an earlier bulk commit.
Backend serves on 5000 everywhere else -- restore that as the proxy
target.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
Copilot AI lite review requested due to automatic review settings September 6, 2026 12:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Badri-Rajendran and others added 8 commits September 6, 2026 07:44
Bandit's own suppression syntax is `# nosec <CODE>`, not ruff's `# noqa`
-- bandit doesn't recognize noqa, so B104 (medium) was flagging the
already-reviewed 0.0.0.0 bind on `mcp_server_host` (the MCP server's
only network interface inside its container). Add `# nosec B104`
alongside the existing `# noqa: S104`, matching the established
precedent at app/services/chat/read_only_sql_executor.py:65.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
SQLAlchemy's `Table.drop(bind=owner_engine, checkfirst=True)` fails
hard with `DependentObjectsStillExist` under the full suite whenever
another test file's still-live fixture holds an FK into the table
being dropped (e.g. `customer`) -- reproducible only as part of the
full run, confirmed by the two teardown errors on
tests/contract/test_mcp_agent_surface.py. Sweep the remaining 26
occurrences across 20 files to the raw `DROP TABLE IF EXISTS ...
CASCADE` idiom already established in
tests/integration/conftest.py's `ledger_tables` fixture. Teardown
mechanics only -- no fixture creates different tables, no assertion
changed.

Full suite run twice back to back: 779 passed, 7 skipped, 2 xpassed,
0 errors, 83.07% coverage both times.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
- New pure frontend/src/features/auth/validation.js: email format check
  (looser than the backend's EmailStr so the client never rejects what
  the server would accept), a length-or-class-mix password strength
  rule layered on top of the backend's 8-char floor, and a
  confirm-password matcher.
- RegisterForm now validates on blur and submit via Input's existing
  error prop, with a live 2-item password strength checklist (updates
  per keystroke, icon + text, never color alone) instead of a static
  hint. The shared banner is reserved for the server/network error.
- Failed submit still leaves all input filled.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
- Email: inline format validation via Input's error prop, on blur and
  submit (was `required` only).
- Password stays presence-only by design — a login form must never
  reject an older, already-provisioned password because a newer
  strength policy exists; commented in place so this isn't "fixed"
  into a regression later.
- MFA step: format-checks a 6-digit numeric code (verified against
  pyotp's TOTP default, which is what /auth/mfa/verify actually
  checks against) before submitting.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
Replaces the Ledger palette (near-black ink, cool paper, verdigris
accent) with Vault (deep navy ink, warm ivory paper, brass accent) --
the direction chosen after reviewing four options against the real
signup form and dashboard layout. Token values and every contrast
ratio recomputed with the actual WCAG relative-luminance formula, not
eyeballed; text-muted darkened from the artifact's draft value to
clear full AA at caption size (13px, not "large text").

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
Covers the two FAIL-level findings from the compliance audit: the
correction test (PostingService.correct() has no real caller, the one
test exercising it doesn't reverse properly, and nothing republishes a
statement snapshot after a restatement) and automatic-fail #3 (six
tables beyond the core ledger permit UPDATE on money/unit columns).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…r poll

The onboarding "Identify" flow got stuck at pending indefinitely: the
only thing that ever advanced kyc_status was the Stripe Identity
webhook, and Stripe can't reach localhost to deliver it in local dev
(and any missed/delayed webhook would have the same effect in
production). GET /identity/status/<id> now polls Stripe directly for
the live verification-session status when kyc_status is still
pending, reusing the existing verdict-mapping/resolve logic unchanged
-- so it self-heals with zero frontend changes, since "Refresh status"
already calls this endpoint.

Verified against the real Stripe sandbox (not just the fake adapter):
created a live verification session, confirmed the new poll path
executes cleanly end-to-end with no errors.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
Foundation for the chat/nav/tables redesign work: a 16-glyph Icon
component (currentColor-driven, token-sized) and --duration-*/--ease-*
tokens with a single reduced-motion rule that zeroes every duration at
once. Scoped tight to glyphs with a real replacement site already
identified (retiring the ▼▲↕/✕ Unicode glyphs currently standing in
for icons), not a speculative larger set.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
@Badri-Rajendran
Badri-Rajendran merged commit 18b1847 into main Sep 6, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants