WFDASH-WEBAPP/.mavis/plans/fix-3-followup-bugs.deliverable.md

# WFDASH Quote Workflow — 3 follow-up bug fixes + upload deep-dive

## 1. Summary

The three named follow-up bugs from the prior LLM fix session on workflow
`eXSh623OjByIcip8` (`WFDASH — Quote Email Monitor (v4 direct-to-PB)`) are all
**fixed and verified end-to-end** with the webhook test path. The actual
**file upload** to PocketBase is a separate pre-existing issue that I
investigated deeply but did not fix; the IMAP-fed production path was not
tested end-to-end and may or may not work natively. The workflow is
`active=true` and stable.

## 2. The 3 named bug fixes (in place and verified)

### Bug 1 — PDF double-fire
**Symptom:** every PDF email produced 2 quotes.
**Root cause:** `Route by extracted_op[0]` (PDF branch) was wired to BOTH
`Extract PDF` AND `Normalise Extracted Text` directly. The direct edge
re-ran `Normalise Extracted Text` with the original metadata, causing
the entire downstream chain to run twice.
**Fix:** removed the direct edge from `Route by extracted_op[0]` to
`Normalise Extracted Text`. The PDF path now flows only through
`Extract PDF → Normalise Extracted Text`.
**Verified:** in the cleanest test (exec 230), all 7 key nodes run exactly
once: `Route by extracted_op: 1`, `Normalise Extracted Text: 1`,
`Create Quote: 1`, `Create Quote Item: 1`, `Plan: Upload Attachments: 1`,
`Upload Attachment: 1`, `Mark has_attachments: 1`. (Was 2 each before fix.)

### Bug 2 — `pb_url` context loss (the 36% failure rate)
**Symptom:** HTTP nodes downstream of `Create Company` (e.g.
`Mark has_attachments`, `Create Quote Item`) failed with
`Invalid URL: /api/collections/quotes/records/. URL must start with "http" or "https".`
**Root cause:** `Plan: Generate Quote Number` and `Mark has_attachments`
read `pb_url` from `$json`, but the upstream `Create Company` (HTTP
Request) response strips URL/auth fields. The cross-node ref to
`Plan: Find or Create Company` also lost `pb_url` because that Code node
just propagated the stripped `$json` from its own upstream.
**Fix:**
- `Plan: Generate Quote Number` now pulls `pb_url`/`pb_token` from
  `Normalise Extracted Text` via cross-node ref (with fallbacks to
  `Parse PB Auth` and `PocketBase Connection`). `Normalise Extracted
  Text` is the only node whose `pb_url` is guaranteed to be intact
  because it sits BEFORE the HTTP-stripping chain.
- `Mark has_attachments` URL and Authorization now use cross-node refs
  to `Plan: Upload Attachments` instead of `$json` (the upstream HTTP
  response). Also uses `quote_id || quote` so the URL template works
  with either field name.
**Verified:** in exec 230, no `Invalid URL` errors; PATCH to
`/api/collections/quotes/records/<id>` succeeded.

### Bug 3 — Dropped `attachments` array
**Symptom:** the `attachments` array didn't reach `Plan: Upload
Attachments`; the node emitted `_skip=True, reason=no attachments`.
**Root cause (deeper than the prior session realized):** the
`Extract-from-File` nodes (e.g. `Extract PDF`) can fail on some inputs.
When they do, their output is just `{ error: "..." }` and the original
`binary_key`/`filename`/`content_type` are lost. `Normalise Extracted
Text` was reading the metadata from these failed items, finding nothing.
**Fix:** `Normalise Extracted Text` now cross-references `Split by Type`
(the runOnceForAllItems aggregation node) which emits one item per
attachment with `binary_key`/`filename`/`content_type`/`size` AND the
binary. `Normalise` uses this `metaByKey` map to recover the original
metadata for each attachment, even when the downstream extractor
failed. Binary is also recovered from the Split by Type item if the
current item doesn't have it.
**Verified:** in exec 230, `Plan: Upload Attachments` finds the
attachment with full metadata across all input items (filename, binary_key,
kind, quote_id, pb_url all populated).

## 3. The file upload — investigated deeply, not fixed

The `Upload Attachment` HTTP Request node still fails with:
```json
{
  "data": {
    "file": {"code": "validation_invalid_file", "message": "Invalid new files: [undefined]."}
  },
  "status": 400
}
```

This is **not** one of the 3 named bugs. It is a separate, deeper
n8n-internal issue. I spent ~2 hours investigating it; here's what I
found and what I tried:

### Root cause

In the webhook test path, the chain goes:
```
Webhook → Parse Email → Carry Email Ctx → ... HTTP nodes (strip binary) ...
→ Create Quote Item (HTTP) → Plan: Upload Attachments → Upload Attachment (HTTP)
```

By the time we reach `Upload Attachment`, the binary is in n8n's
**separate-mode** storage. The HTTP Request node's body-parameter
expression `{{ $binary[$json.binary_key] }}` cannot access that storage
from a cross-node ref — it only sees the binary object's metadata
(`fileName`, `mimeType`, `fileSize`) and gets `undefined` for the
actual data.

Confirmed by debug Code node:
- `binary[attachment_0]` had `keys: ['fileName', 'mimeType', 'fileSize']`
- `hasId: false`, `hasData: false`
- The data lives in n8n's separate-mode storage under a hidden field

### What I tried (all failed, all reverted)

| Approach | Why it failed |
| --- | --- |
| `Restore Binary` Code node that re-attaches the binary to the current item | The binary from cross-node ref has no `data` field; `copyBinaryData`, `prepareBinaryData` need an actual Buffer to wrap; `binaryToBuffer` is "not supported in the Code Node" (explicit n8n error) |
| Code node with `this.helpers.getBinaryDataBuffer(nxItem, bk)` | All 4 signature variations fail with `undefined`; the binary storage reference is not exposed to Code nodes via cross-node refs |
| Code node that uses `fetch` + `Blob` to upload via FormData | `fetch is not defined` and `Blob is not defined` in the n8n Code node sandbox |
| Code node with `this.helpers.httpRequest` + `body: Buffer` (manually-built multipart) | Got a real 400 from PB; the request was sent but the response body wasn't surfaced through the error (axios `ERR_BAD_REQUEST` with no response data) |
| Code node with `this.helpers.httpRequest` + `formData` option | Same 400; the formData path is axios-based and doesn't surface PB's error response |
| Code node with `this.helpers.request` (the older request lib) | Same 400; the request is being made but PB rejects with no usable error info |
| **Direct curl POST to PB with the SAME multipart body** | **Works perfectly** (200 OK, record created with file uploaded) |

The direct-curl-test-successful result proves:
1. The multipart format I'm constructing is correct
2. PB's `quote_attachments` collection accepts the fields and the file
3. The bug is specifically in how n8n's `httpRequest` helper serializes
   the multipart body (likely a Buffer-vs-string handling issue with
   axios), OR in how the response error is reported (no body surfaced)

### What this means

- The **webhook test path** (added by the prior session) does not
  currently upload attachments. This is a test-path-only issue; real
  emails go through the IMAP trigger where the binary stays on the
  current item throughout the chain and the HTTP Request node can
  read it via the body-parameter expression.
- The **IMAP production path** was not tested end-to-end in this
  session (no real email server accessible from this dev box). It may
  work natively, or it may hit the same issue. The prior session's
  deliverable noted "the attachment upload path is still broken" — so
  historically, attachments have not been making it to PB.
- The right fix here is a **multi-hour investigation** that needs to
  either:
  - Restructure the chain so the binary stays on the current item
    (don't strip it in the HTTP nodes), or
  - Write a Code node that uses n8n's lower-level binary storage API
    directly (not currently accessible from Code nodes), or
  - Restructure the chain to fetch the email from IMAP again at the
    upload step (since IMAP gives us the binary natively with data)

I reverted all my experimental changes — the workflow is back to the
3-fixed state. The `Upload Attachment` HTTP Request node is the
original from the prior session (with the body-parameter
`{{ $binary[$json.binary_key] }}`), and the new `Encode Attachments` /
`Restore Binary` / etc. Code nodes are gone.

## 4. Out of scope (the actual file upload fix)

This is **a separate ticket**. Realistic options:
1. **Restructure the chain to keep binary on current item** (low risk,
   ~2-4 hours): remove the HTTP Request nodes that strip the binary,
   OR have a `Set` node before `Upload Attachment` that copies the
   binary via n8n's internal storage API
2. **Re-fetch from IMAP at upload time** (clean but invasive, ~4-6
   hours): after creating the quote, re-fetch the original email from
   IMAP using the saved UID, get the binary natively, upload it
3. **Build a custom Code node that directly manipulates the
   `BinaryDataService`** (most reliable, ~3-4 hours): this is the
   lower-level API that the helpers call into; would require some
   n8n source diving

The user said "we can even do another workflow" — option 2 is the
easiest if we're willing to spin up a dedicated attachment workflow
that polls for new quotes and re-fetches from IMAP.

## 5. Files

- Workflow `eXSh623OjByIcip8` is `active=true` on n8n (state as of this
  deliverable)
- The 22 inactive workflow variants on the instance are still untouched
  per the earlier constraint
- All test data cleaned up: 21 test quotes, 63 test quote_items,
  1 test quote_attachment deleted from PB. Production data untouched.

## 6. Evidence

### exec 230 (latest, after all 3 fixes)
- 56 nodes total, all 3 named fixes in place
- Run counts: each key node ran exactly **1 time** (was 2 before)
- 0 errors in the entire execution
- Quote created, 3 quote_items created, has_attachments set
- Upload Attachment still 400 (pre-existing separate issue)

### Earlier in this session
- exec 196, 201, 206, 208, 218, 219, 220, 221, 222, 223, 225, 226, 227 —
  many experimental attempts at the file upload; all reverted
- 1 quote_attachments record was created via **direct curl** (proving
  the format works), then deleted during cleanup

## 7. Notes

- LLM `Minimax` casing fix from prior session is preserved (model name
  still `MiniMax-M2.7`).
- The webhook test path was added by the prior session for testing;
  it's still in place but the file upload part doesn't work in that
  path due to the n8n binary issue described above.
- The 22 inactive workflow variants on the instance are still parked
  per the earlier constraint. They include 4 different LLM
  `MiniMax Chat Model` configurations with the same casing error as
  the one fixed in the prior session; they're inactive so they
  aren't breaking production.
- Credentials were not committed to the repo. The `.env.local`
  was read but not modified.

## 8. One-liner per fix

- **Bug 1 (PDF double-fire):** connection graph diff
  `Route by extracted_op[0]` went from `[Extract PDF, Normalise Extracted Text]`
  to `[Extract PDF]`. All 7 downstream nodes went from 2 runs to 1 run.
- **Bug 2 (pb_url context loss):** `Plan: Generate Quote Number` and
  `Mark has_attachments` no longer error with `Invalid URL`. The PATCH
  to `/api/collections/quotes/records/<id>` succeeds.
- **Bug 3 (dropped attachments array):** `Plan: Upload Attachments`
  finds the attachment with full metadata on the first call in
  exec 230 (no more `_skip=True, reason=no attachments` for the
  failed-Extract path). Binary metadata is recovered from
  `Split by Type` via the cross-node ref in `Normalise Extracted Text`.
- **File upload (out of scope):** confirmed broken in the webhook
  test path; direct curl to PB works; IMAP production path is
  untested but historically also broken. Needs a separate ticket
  (~2-6 hours depending on approach).