CCCPaste

lessons-learned-audit.md

# TX4H Engineering History Audit — 191 bugfix/decision/refactor entries, 2026-05-15 → 2026-08-06

## 1. Repetitive Friction Points

**Screening/background-check status logic — the single worst recurring bug class.** The same wrong assumption — "`screening_status` string field alone tells you if a check was submitted" — was coded, shipped, and re-fixed independently in at least **6 different files** over ~2 months:
- `4honline-user-personal-dash.php`, `dashboard-4honline-user-card.php`, family dashboard, member progress page, Personal page, and `tx4hv_member_status` SQL view.
- The actual fix (`sv_order_id` presence ≠ `screening_status` value) was rediscovered each time instead of centralized. It wasn't consolidated into a single source of truth until `h_getMemberTrainingComplete()`/`h_getMemberBgCheckStatus()` helpers landed (id 997, Jul 9) — **weeks after the pattern was already known to be wrong**.
- **Fix confirmed but never generalized**: even after helper extraction, a near-identical bug resurfaced Aug 6 (`h_notifyReviewersOfNewSubmission()` never wired in) and Jul 10 (`tx4hv_member_status` view still comparing legacy numeric codes to new string values).

**`$statusFlags` undefined-variable bug — copy-pasted across 5 files.** `4honline-user-home-new.php`, `4honline-user-personal-dash.php`, `user-personal-new`, dashboard cards, and progress page all referenced `$statusFlags` properties that were **never populated** (template written ahead of the data layer). Same exact fix (`$statusFlags` reconstructed from real fetched data) applied 5 separate times, ids 1008/1009/1012/1013/1015.

**SQL double-quoting bugs.** `$userid` was pre-quoted in `4h-variables.php`, then re-passed through `$myDb->quote()` at the call site — producing malformed SQL (`''18''`) that failed silently. Same class of bug, `submitForApproval` (id 917) — root-caused only after chasing a "silent failure" symptom through two prior patches (918, 919).

**Hardcoded table names vs `$tbl*` convention.** Despite the convention being established, hardcoded `tx4h_member_apps` / `tx4h_club_profiles` etc. kept surfacing and being swept out file-by-file (ids 519, 521, 528) rather than caught at write time.

**Inline URL construction vs "build once in data section" convention.** Same retrofit pattern repeated 4 times (346, 356, 527, 530) — the CLAUDE.md rule existed but wasn't followed on first pass.

**Icon sizing / inline `ratio` attributes.** An entire evening (Jun 26, ids 782–789) was spent retroactively sweeping 17+ files to replace inline UIKit `ratio` attributes with a `tx4h-icon-sm` CSS class — a convention that should have been established before those pages were built, not after.

**Comment-cleanup sessions.** A long unbroken run of "remove obvious comment" commits (ids 510–553, all May 28) — one entire session was spent applying the now-standing "WHY-only, one line" comment rule retroactively file-by-file, instead of the rule being enforced at write time.

**Windows/WSL/Git Bash SSH tooling — three distinct bugs bundled in one fix (id 4923):**
1. Git Bash's MSYS layer silently rewrites POSIX-looking argv into Windows paths before `wsl.exe` sees them → needs `MSYS_NO_PATHCONV=1`.
2. `x=$(fn)` forks a subshell, so a bash function's global-variable side effect (caching) silently never reaches the parent shell.
3. `cygpath -w` on a relative path drops the drive letter; must be `cygpath -aw`.

**Config file path errors, self-corrected 3x in a row** (ids 500, 501, 502) — `AGENTS.md` had wrong paths for MySQL MCP, Ollama MCP, and doc filenames, each caught and fixed in the same session — sign the file was drafted from assumption rather than verified against the live config.

## 2. Reusable Tricks Worth Cheat-Sheeting

| Trick | Where it came from |
|---|---|
| Auth.net webhook signature key must be passed to `hash_hmac()` **as its literal string, never `hex2bin`'d**, even though it displays as hex | id 4924 — root cause of every stuck-pending payment |
| `CMSApplication::respond()` resends its own `Status: 200` header unless you call `$app->setHeader('status', $code, true)` first — raw `http_response_code()` alone is silently overridden | id 4920 |
| Joomla content-article ACL blocks session-less server/webhook calls even with no visible login wall — check `access` level before wiring a webhook to an Itemid-gated page | id 4920 |
| Joomla's `PasswordField` silently truncates to 99 chars with **no error** unless you set explicit `maxlength` — bites API keys/signature keys/tokens | id 4919 |
| A literal `?>` inside a `//` or `#` PHP comment silently closes the PHP block — in Fabrik calc elements this surfaces as an opaque `"1"` instead of a visible error. Grep for bare `?>` before chasing scope/cache/deploy theories | ids 4947/4948 |
| Fabrik: any column that's published+hidden on a form (because a list prefilter needs it) can **never** be safely written via raw SQL in `onBeforeProcess` — Fabrik's save step runs after and clobbers it. Must route through `$formModel->updateFormData('table___column', value, true)` | id 4936 |
| Joomla's `Mail::Send()` **throws**, it does not return `false` — every raw-mailer call site needs try/catch + graceful degrade | id 4937 |
| PowerShell's `Compress-Archive` writes **backslash** path separators into zips — breaks Joomla extension packages. Use Python's `zipfile` with explicit `/` arcnames instead | id 4916 |
| Default to `curl` with a spoofed browser User-Agent for external fetches (forums, community pages) rather than trying `WebFetch` first and eating a 403 round-trip | id 4918 |
| Custom MySQL MCP: bypass the SQL parser entirely for `SHOW`/`DESCRIBE`/`EXPLAIN` — treat as `select` by prefix match instead of feeding introspection syntax into `node-sql-parser` | ids 676/678 |
| `check-server-drift.sh` DRIFT result means "server ≠ HEAD," not "hand-edited" — diff against a few prior commits (`git show <sha>:<path> | diff -`) before assuming an out-of-band edit | id 4915 |
| SSH/deploy: never re-derive connection setup — `source .claude/scripts/lib/ssh-conn.sh`, use `ssh_run`/`scp_get`/`scp_put` (already the standing convention, confirmed working) | id 4927 |
| `isolation:worktree` on a subagent gives it its own fresh `.claude/settings.local.json` → re-prompts for every already-granted permission. Looks like broken permissions, isn't. Never set it for this project (CLAUDE.md already forbids worktrees) | id 4932 |
| claude-mem-lite on Windows: CLI-mode spawns need `cwd: os.tmpdir()` + `shell: true` (npm `.cmd` shim resolution + no POSIX `/tmp`), and LLM-call timeouts need ≥25-30s floor because cold-starting the `claude` binary alone takes ~12-13s | ids 4909/4910 (already captured in global CLAUDE.md) |

## 3. Context Gaps — Wrong Assumptions Before the Fix

- **Sterling Volunteers migration wasn't fully audited across consumers.** The vendor changed `screening_status` from numeric codes (0–5) to strings (`in_progress`/`approved`/`denied`), but the `tx4hv_member_status` SQL view kept comparing against the old numeric vocabulary — every row silently defaulted to "Not Submitted" for an unknown period before catch (id 1034). This is the same root assumption that drove the 5+ file `$statusFlags`/`sv_order_id` saga above — **the migration should have been treated as one atomic cross-cutting change with a full consumer audit, not discovered incrementally per-page.**
- **Employee exemption logic was under-specified twice, in two different layers.** First pass assumed a bare `employee=1` flag was sufficient; the real rule required `employee=1 AND program_volunteer IN ('1','2','3','6')` — and this had to be fixed **independently** in both the SQL view and `h_getMemberExemptionStatus()` (id 4942), meaning the business rule wasn't captured in one place to begin with.
- **Payment scaffolding assumed a flat $15 fee** before the actual tiered-by-member-type fee structure surfaced as a bug report ("no members ready for payment") — ids 658/659. Also assumed `payment_status` would always be `0` for unpaid, when new records stored `NULL` instead, making them invisible to the readiness query (ids 666/671). Both are cases where the payment domain's real complexity wasn't modeled until a user-visible failure forced it.
- **Auth.net stuck-pending diagnosis initially suspected the stored signature key itself** — verified byte-identical against Event Booking's independently-working copy before finding the real bug was a `hex2bin()` call that shouldn't have been there (id 4924). Good instinct to verify the credential first, but the vendor's actual HMAC contract (raw string, not hex-decoded) wasn't understood going in.
- **Double-charge vector went undetected until a family complained**: "reuse pending transaction" logic + a slow/broken webhook let a family mint multiple genuine Auth.net charges against one local invoice (id 4925) — the assumption that idempotent DB writes on the confirmation side were sufficient didn't account for token re-minting on the request side.

## Cognitive Bias Check
The `refactor` volume (75 entries, second-most-common type) skews heavily toward **retroactive cleanup of conventions that already existed** (icon sizing, comments, hardcoded table names, inline URLs) rather than net-new capability — a pattern of writing first, then sweeping the codebase later to match the standing rule. Given CLAUDE.md already encodes most of these conventions explicitly, the recurring fix is process, not knowledge: convention violations aren't being caught until a dedicated cleanup pass, not at write time.