sales-trainer/docs/engineering-log/2026-08-13-idea-implementation-audit.md

# 2026-08-13 — Idea-to-implementation audit

## Scope

Compared `IDEA.md`, `README.md`, `docs/PLAN.md`, and `docs/FUTURE_WORK.md` against the Vue frontend, Flask routes/services, JSON storage, deployment files, and all executable test scripts. No production data or credentials were inspected.

## Architecture traced

- Admin setup: `GroupBuilder.vue` → `POST /api/groups` → document parser/`GroupStore` → `POST /api/groups/<gid>/analyze` → `Analyzer` → `PersonaGenerator` → report → JSON group storage.
- Training: `Training.vue` → `Personas.vue` → `Chat.vue` → chat start/send routes → `Simulator.persona_reply` + `Simulator.evaluate_turn` → `SessionStore` → debrief/board/analytics.
- Trainee loop backend: `/api/me/weak-areas` + `/api/me/personas/generate` → weak-area analyzer/own-persona generator → private group. API clients exist, but no Vue view invokes them.
- Persistence: one JSON file per entity, atomic file replacement and in-process per-path locks.

## Verification performed

- All 12 executable backend scripts passed: M0, M1, setup, security, scenario, E2E, IP protection, SaaS tenant, user journey, routes, resume/decision, variant.
- `python -m compileall app scripts` passed.
- `npm run build` passed (1,772 modules; production bundle generated).
- Flask smoke test passed: `/health` 200, SPA `/` 200, unauthenticated `/api/groups` 401.
- Docker image was not built because Docker is not installed on this Mac.
- Real-provider LLM behavior was not exercised because no live key was used.

## Findings

### P0 — Any authenticated user can take over an arbitrary account

`auth_routes.py:56-72` accepts a client-supplied `username` in `/api/auth/setup` and passes it to `complete_setup()` instead of deriving the target exclusively from `current_user()`. It also does not require the caller's `must_setup` flag. A fresh isolated-data runtime probe confirmed that an ordinary user can target `admin`, replace its email/password, and then log in as that super-admin (setup 200; takeover login 200). This is an immediate deployment blocker.

### P0 — Tenant admins can reset credentials across tenants

`admin_routes.py:141-177` loads the requested user but never verifies that a non-super-admin actor and target share the same `org_id`. Password and email mutation therefore cross tenant boundaries. A runtime probe confirmed an admin in `org-two` could reset a user in `org-default` and then log in as that victim (update 200; hijacked login 200).

### P0 — Tenant admins can provision users into another tenant

`admin_routes.py:87-126` trusts the request's `org_id` for existing organizations. The super-admin guard applies only when `new_org=true`. A runtime probe confirmed an `org-two` admin could create a user directly inside `org-default` (create 201; new-user login 200), bypassing tenant membership and seat ownership boundaries.

### P0 — Admin secret fields leak from analyze response

`group_routes.py:233-243` returns the full `group`, `sales_kit`, and full personas after analysis, without applying the admin redaction used by the GET routes at `group_routes.py:253-265`. A runtime probe as a non-super admin confirmed `pains` and `tolerance` are present in the analyze response, while the subsequent list endpoint strips them. This defeats the documented IP boundary during the normal group-creation flow.

### P1 — Persona replies render as raw JSON with compliant real LLMs

`simulator.py:43-48` instructs the role-play model to return a JSON object, but `simulator.py:142-149` now treats the entire response as plain text instead of parsing `reply`. A deterministic probe returned the full `{"reply":...,"decision":...,"mood":...}` blob as the customer message. The mock also returns JSON (`scripts/mock_llm.py:113-120`), but tests only assert that `reply` is truthy, so this escaped detection.

### P1 — Normal auto-finish bypasses the promised final judge debrief

Every turn is judged by `evaluate_turn`; when it says buy/walk, `chat_routes.py:301-318` builds `_build_abbrev_debrief` instead of calling `Simulator.judge`. That abbreviated report uses fixed scores (60/25) and generic coaching (`chat_routes.py:62-118`). The detailed final judge exists only in the separate finish endpoint (`chat_routes.py:325-367`), but the current `Chat.vue` has no finish action. Therefore the normal flow does not deliver the README promise of a separately judged, message-specific scored/coached debrief.

### P1 — Admin UI offers chat actions that backend rejects

Admin-facing persona screens link to the trainee chat (`GroupEdit.vue:40-44`, `Personas.vue:42-45`), while chat start/send/finish routes require role `user` (`chat_routes.py:137-140`, `208-211`, `325-328`). Admins following the visible action receive 403.

### P1 — Claimed training loop is backend-only

README lines 32-33 promise weak-area analysis and user-generated personas. Backend routes are implemented (`me_routes.py:51-59`, `83-122`) and API client methods exist (`frontend/src/api/index.js:58-60`), but no Vue view calls them. `MyBoard.vue:1-55` only shows counts and sessions. Trainees cannot use the promised weak-area lock/manual persona generation through the product UI.

### P1 — Users cannot manually finish a stalled conversation or revisit a completed debrief

The API client and backend expose manual finish (`frontend/src/api/index.js:55`; `chat_routes.py:325-367`), but `Chat.vue` never calls it and only transitions to done when a send response reports `finished` (`Chat.vue:155-165`). If the per-turn judge never returns buy/walk, the user has no end-session control. After leaving a completed chat, `MyBoard.vue:22-28` provides no session-detail link and the API client has no finished-session detail method, so transcripts and coaching cannot be revisited from the UI.

### P1 — “15 personas” is not an enforced invariant

`persona_generator.py:35-53` retries, then accepts any non-empty list; a runtime probe with an LLM returning one persona produced a ready result containing one persona. The normalization can also underfill tiers (`persona_generator.py:55-90`). This conflicts with the central 15-persona/5-per-tier promise. Current E2E passes only because the mock always supplies 15.

### P0 — One-shot identity collides across groups and is raceable

`SessionStore.create` keys the business rule by `(user_id, persona_id)` but omits `group_id` (`sessions.py:33-40`), while independently generated groups reuse IDs such as `persona-01` (`persona_generator.py:67`). A runtime probe confirmed finishing `persona-01` in one group blocks `persona-01` in another unrelated group. Active-session discovery has the same missing scope (`sessions.py:69-75`). The check and random-file creation are also separate operations: a synchronized 8-thread probe created eight sessions for one user/persona. The implementation therefore both over-blocks legitimate training and under-blocks concurrent duplicates.

### P1 — Bootstrap credentials are privileged before mandatory setup

`factory.py:13-31` creates a known bootstrap super-admin and marks it `must_setup`, but `require_auth` never enforces that flag (`helpers.py:44-63`). The independent runtime probe confirmed the pre-setup bootstrap account could perform a privileged group creation. A fresh Internet-facing deployment can therefore be claimed before the operator changes credentials.

### P1 — Tenant deactivation does not revoke existing tokens

Organization activity is checked at login (`auth/users.py:172-186`) but not in `require_auth` (`helpers.py:44-63`). An isolated runtime probe confirmed a token issued before tenant deactivation continued to access `/api/auth/me` with 200 afterward. With the default 24-hour JWT lifetime, disabling an organization does not promptly disable its sessions.

### P1 — Production container runs Flask development server

Docker executes `python run.py` (`Dockerfile:29-39`), and `run.py:13-20` uses `app.run`. Flask itself warns this is not a production server. It may work for a small internal pilot but is not production-grade serving, graceful lifecycle, or concurrency management.

### P1 — Trainee-created variants mutated the shared corporate group (resolved in Sprint 1 follow-up)

The original audit found that the variant route appended directly to the shared group's personas. The follow-up now calls `get_or_create_private_group()` for trainee actors, stores the variant there, returns the target group ID, and routes the UI to that group (`backend/app/api/group_routes.py:373-402`, `backend/app/services/groups.py:74-102`, `backend/tests/test_group_redaction.py:180-242`). Admin-created variants still extend the shared admin pool by design. Collection-level scan/create atomicity remains deferred to Sprint 2.

### P2 — Upload size configuration is unused

`Config.UPLOAD_MAX_MB` is defined (`config.py:62`) but never applied to Flask `MAX_CONTENT_LENGTH` or route-level size validation. File type and path checks exist, but upload memory/disk consumption is effectively unbounded at the application layer.

### P2 — Product copy and dead append flow contradict the decided UX

The intended design says auto-create 15 with no add-more flow, but `GroupBuilder.vue:12` and `GroupEdit.vue:21-23` still tell admins to click an absent “create more” button. `GroupEdit.vue:83-92` retains an unreachable append function, and the backend still supports `?append=true` (`group_routes.py:183-190`).

If append mode is later re-exposed, it also concatenates newly generated personas without replacing IDs (`group_routes.py:219-228`) while generation restarts at `persona-01` (`persona_generator.py:55-68`). This would create duplicate Vue keys, ambiguous persona lookup, and shared one-shot history.

### P2 — Deployment defaults are unsafe if operators omit configuration

`config.py:50` falls back to a short public development JWT secret; runtime emitted an insecure HMAC key warning. This should fail closed outside development rather than silently issue production tokens with the fallback.

### P2 — Non-UTF-8 file parsing imports an undeclared dependency

`file_parser.py:26` imports `chardet`, but `requirements.txt:1-9` installs `charset-normalizer` instead. The reviewer probe confirmed a non-UTF-8 input reaches `ModuleNotFoundError: chardet`. UTF-8 inputs pass, which is why the current scripts do not expose the deployment defect.

### P2 — Settings password change is wired to first-time setup and fails

`Settings.vue:88-102` calls `/auth/setup` without `accepted_terms`, while that route requires the field at `auth_routes.py:65-69`. The same route is also semantically unsafe for repeat password changes and caused the P0 arbitrary-target takeover above. Password change needs a separate current-user endpoint with appropriate current-password/session safeguards.

### P2 — Docker build context can include local secrets

The Dockerfile copies the complete backend directory (`Dockerfile:26-27`), and the repository has no `.dockerignore`. Git ignore rules do not constrain Docker build context, so a local `backend/.env` or other untracked development artifact can be copied into an image layer. Add a restrictive `.dockerignore` and verify the built image contains no local configuration or credentials.

### P2 — Active sessions are displayed as losses

`/api/chat/sessions` returns active and finished sessions, but `MyBoard.vue:27` renders every outcome other than `won` as `lost`. Since active sessions have `outcome: null` (`sessions.py:49-53`), unfinished training is falsely reported as a loss instead of an active/resume state.

### Reviewer claim rejected — analytics CSV Bearer header is correct

One reviewer reported a malformed authorization header because its tool output had redacted the token scheme. Direct byte/codepoint inspection of `Analytics.vue:92` confirmed the source contains the correct `Bearer ${auth.token}` template string, and the frontend production build succeeds. This is not a defect. The separate signed-link route remains unreachable without Bearer auth because route decorators run before query-token verification, but the currently shipped frontend uses Bearer auth directly.

## Capability verdict

Core concept is substantially implemented: roles, group creation, file parsing, LLM sales-kit/persona generation, tiers, scenarios, chat/resume, per-turn decision, variants, session results, analytics, and static deployment all exist and pass mock-based tests.

However, the application must not be deployed to untrusted users in its current state. Independent review plus isolated-data reproduction confirmed arbitrary super-admin takeover and two cross-tenant administration bypasses. Beyond those security blockers, the product path has a secret-data leak, likely raw-JSON customer messages with a compliant real model, generic rather than final-judge coaching, inaccessible weak-area training UI, a broken one-shot identity invariant, and a development server in production packaging. Passing mock scripts materially overstate readiness because none cover these attack paths.

## Recommended order

1. Freeze deployment and fix `/auth/setup` arbitrary-target takeover; add an ordinary-user-to-super-admin regression test.
2. Enforce tenant scope on admin user creation/update and add cross-org denial tests.
3. Enforce `must_setup` server-side, remove known bootstrap privilege exposure, and re-check organization activity on authenticated requests.
4. Fix analyze-response redaction and add a regression test.
5. Redesign session identity as `(org_id, user_id, group_id, persona_id)` and enforce creation atomically.
6. Align persona prompt/response parsing and make auto-finish invoke the final judge; test exact visible messages and non-generic coaching.
7. Remove admin chat affordances or deliberately support admin preview mode; wire weak-area/manual persona generation into the trainee UI.
8. Enforce exactly 15/5-per-tier or make partial generation explicit and recoverable.
9. Add a transactional database, production WSGI server, upload limit, fail-closed JWT configuration, dependency lock, and CI security gates.
10. Run real-provider, hostile-input, concurrency, and mobile browser QA after the above.