Upload 6 files

DATASET_COLLECTION_GUIDANCE.md

@@ -74,6 +74,47 @@ Rationale:
 Priority:  contribution  >  feedback
 ```
 
+### Retraction tombstones
+
+When a user edits a previously submitted quick-feedback rating, the browser
+sends a **retraction tombstone** record before the new rating.  Tombstones
+are stored in the `feedback/` folder alongside normal rating records and
+carry these fields:
+
+| Field | Value |
+|-------|-------|
+| `action` | `"retract"` |
+| `prevSessionId` | `sessionId` of the original rating record |
+| `_dedup_key` | identical to the original record's `_dedup_key` |
+| `_ts` | server-write timestamp — always later than the original |
+| `ratingValue` | *absent* |
+
+Tombstones participate in the LWW loop inside `deduplicate()` because their
+`_ts` is later than the original record's, ensuring the original rating is
+not emitted.  They are then **unconditionally removed from the clean output**:
+a tombstone is never a valid training example.
+
+**Normal terminal state** (edit completed, all three records share the same
+`_dedup_key`):
+
+```
+_ts 100  ratingValue=+1     ← original rating, kept during LWW, superseded
+_ts 200  action="retract"   ← tombstone, LWW suppresses +1; filtered from output
+_ts 201  ratingValue=-1     ← new rating, LWW winner, written to clean output ✓
+```
+
+**Degenerate case** (tombstone wins — follow-up rating never reached the
+server, e.g. network failure after the retraction was sent):
+
+```
+_ts 100  ratingValue=+1     ← original rating, superseded
+_ts 200  action="retract"   ← LWW winner, but FILTERED from clean output
+```
+
+Net result: the original +1 was explicitly retracted, so **no record is
+emitted** for this key.  Training data is never contaminated by either the
+abandoned +1 or the tombstone.
+
 ---
 
 ## 4. Dataset Folder Structure
@@ -127,6 +168,9 @@ Notes
 -----
 * Priority rule: "contribution" beats "feedback" for the same _dedup_key.
 * Records without _dedup_key are retained as-is (legacy, pre-v1.0 records).
+* Retraction tombstones (action="retract") are always excluded from the
+  clean output even if they win the LWW race — they carry no ratingValue
+  and must never enter a training job.
 * Script is idempotent: re-running produces the same output for the same
   dataset state.
 """
@@ -200,6 +244,14 @@ def deduplicate(records: list[dict]) -> list[dict]:
     ``_SOURCE_PRIORITY`` value is kept.  Ties are broken by server-write
     timestamp (``_ts``), keeping the most recent.  This is deterministic:
     given the same input, the output is always the same.
+
+    Retraction tombstones (``action="retract"``) are always excluded from the
+    final output even when they are the last-written record for a given key.
+    A tombstone carries no ``ratingValue`` and must never reach a training
+    job.  The LWW loop above still uses tombstones to suppress an earlier
+    rating (correct behaviour: the tombstone's ``_ts`` is later than the
+    original record's), but the post-loop filter ensures tombstones cannot
+    leak into ``clean_dataset.jsonl``.
     """
     keyed:   dict[str, dict] = {}   # _dedup_key → winning record
     no_key:  list[dict]      = []   # legacy records without _dedup_key
@@ -225,7 +277,24 @@ def deduplicate(records: list[dict]) -> list[dict]:
             if rec.get("_ts", 0) > existing.get("_ts", 0):
                 keyed[dk] = rec
 
-    return list(keyed.values()) + no_key
+    # Post-loop: discard retraction tombstones from the winning set.
+    #
+    # Scenario A (normal edit): user clicks +1 (saved), edits (tombstone
+    # saved at _ts+100ms), then clicks -1 (saved at _ts+101ms).
+    # LWW selects the -1 record. No tombstone in output. ✓
+    #
+    # Scenario B (orphaned tombstone): +1 saved, tombstone saved, but the
+    # follow-up -1 never reaches the server.  LWW selects the tombstone.
+    # Without this filter, clean_dataset.jsonl would contain a record with
+    # action="retract" and ratingValue absent — corrupting any training job.
+    # The filter silently drops the tombstone.  The net effect is correct:
+    # the +1 was explicitly retracted, so no rating is emitted. ✓
+    #
+    # Tombstones are intentionally kept in the keyed dict *during* the loop
+    # because their later _ts must still suppress the earlier +1.  Removal
+    # happens only at this output stage.
+    clean_keyed = [r for r in keyed.values() if r.get("action") != "retract"]
+    return clean_keyed + no_key
 
 
 def write_output(records: list[dict], output_path: Path) -> None:
@@ -292,10 +361,19 @@ def main(argv: list[str] | None = None) -> int:
 
     print(f"Reading records from {local_dir} …")
     all_records = load_all_records(local_dir)
+    retract_raw = sum(1 for r in all_records if r.get("action") == "retract")
     print(f"  {len(all_records)} total records read")
+    if retract_raw:
+        print(
+            f"  {retract_raw} retraction tombstone(s) in raw data "
+            f"(always excluded from clean output)"
+        )
 
     clean = deduplicate(all_records)
-    duplicates_removed = len(all_records) - len(clean)
+    # retract_raw are excluded by deduplicate() before returning; subtract
+    # them so the "duplicates removed" count reflects actual cross-source
+    # duplicates rather than tombstones.
+    duplicates_removed = len(all_records) - retract_raw - len(clean)
     print(f"  {duplicates_removed} duplicate(s) removed (priority rule applied)")
     print(f"  {len(clean)} unique records retained")
 
@@ -348,6 +426,11 @@ Contribution records additionally carry:
 | `_model` | `object \| null` | Model ID and provider |
 | `_consentVersion` | `string` | Consent version string (e.g. `"v1.0"`) |
 
+> **Note — retraction tombstones (`action="retract"`)** are always excluded
+> from `clean_dataset.jsonl` by `deduplicate_dataset.py`.  They are only
+> present in the raw `feedback/` folder.  The `action` field therefore never
+> appears in the clean output described by this table.
+
 ---
 
 ## 7. Enabling Feedback Persistence
@@ -375,7 +458,10 @@ dual-source collection:
 | `app.py` | Added `FEEDBACK_PERSIST_ENABLED` constant |
 | `app.py /v1/contribute` | Every stored JSONL record now carries `_source="contribution"` and `_dedup_key` |
 | `app.py /v1/feedback` | Optional HF persistence via `FEEDBACK_PERSIST_ENABLED`; stored records carry `_source="feedback"` and `_dedup_key` |
+| `app.py /v1/feedback` | Retraction tombstones (`action="retract"`) are detected, validated (`prevSessionId` required), rate-limit-exempt, logged as `feedback.retract`, and committed with `"Retract 1 feedback record"` message |
+| `app.py rate limiter` | `_MAX_RL_ENTRIES` sweep now active in the feedback rate limiter to bound memory under unique-IP floods |
 | `ai-assistant.js detail` | Added `conversationId: _sessionId` to feedback POST payload (critical — server needs this for correct `_dedup_key`) |
 | `ai-assistant.js _feedbackStore` | Added `conversationId` field for self-documenting store entries |
 | `ai-assistant.js tRecords` | Added `_source: 'contribution'` to each record for self-describing POST payload |
-| `DATASET_COLLECTION_GUIDANCE.md` | This document |
+| `deduplicate_dataset.py` | Post-loop filter removes tombstone winners from clean output; `main()` reports tombstone count separately so duplicate counts are accurate |
+| `DATASET_COLLECTION_GUIDANCE.md` | This document; added §3 Retraction tombstones, §6 tombstone exclusion note, §8 this table |

app.py

@@ -1533,41 +1533,86 @@ async def feedback(request: Request) -> JSONResponse:
     except json.JSONDecodeError as exc:
         raise HTTPException(status_code=400, detail=f"Invalid JSON: {exc}") from exc
 
-    # Per-IP rate limit: 30 per hour
+    if not isinstance(payload, dict):
+        raise HTTPException(status_code=422, detail="Feedback body must be a JSON object.")
+
+    # ── Distinguish retraction tombstones from regular ratings ───────────────
+    # Retractions are system-generated housekeeping records that invalidate a
+    # previous rating in the training dataset.  They carry action="retract" and
+    # prevSessionId (pointing to the original record) but NO ratingValue.
+    # Key behavioural differences vs regular feedback:
+    #   1. NOT counted against the per-IP rate limit — a user who edits 10
+    #      answers would otherwise exhaust their 30-slot hourly budget on cleanup
+    #      records alone (10 originals + 10 retractions + 10 new ratings = 30).
+    #   2. Validated differently — prevSessionId is required; ratingValue is absent.
+    #   3. Logged with event "feedback.retract" so operators can distinguish
+    #      retraction volume from new-rating volume in log dashboards.
+    #   4. Committed with a distinct commit_message so the HF repo history is legible.
+    is_retract: bool = payload.get("action") == "retract"
+
     client_ip = _client_ip(request)
-    async with _feedback_rl_lock:
-        now = _time.time()
-        count, window_start = _feedback_rl.get(client_ip, (0, now))
-        if now - window_start > 3600:
-            count, window_start = 0, now
-        count += 1
-        _feedback_rl[client_ip] = (count, window_start)
-        if count > 30:
-            logger.warning(json.dumps({"event": "feedback.ratelimit", "ip": client_ip}))
+
+    if is_retract:
+        # Validate required retraction fields before touching any rate-limit state.
+        if not payload.get("prevSessionId"):
             raise HTTPException(
-                status_code=429,
-                detail="Rate limit exceeded. Maximum 30 feedback submissions per hour.",
-                headers={"Retry-After": "3600"},
+                status_code=422,
+                detail="Retraction records must include a non-empty prevSessionId.",
             )
+        logger.info(
+            json.dumps(
+                {
+                    "event":          "feedback.retract",
+                    "ip":             client_ip,
+                    "prevSessionId":  payload.get("prevSessionId"),
+                    "conversationId": payload.get("conversationId"),
+                    "answerIndex":    payload.get("answerIndex"),
+                    "ts":             payload.get("ts"),
+                    "persist":        FEEDBACK_PERSIST_ENABLED,
+                }
+            )
+        )
+    else:
+        # ── Per-IP rate limit: 30 regular ratings per hour ───────────────────
+        # Retractions are exempt (see above).  The limit guards against
+        # programmatic flooding; normal interactive use stays well under it.
+        async with _feedback_rl_lock:
+            now = _time.time()
+            # Sweep expired entries to bound memory under a unique-IP flood.
+            # _MAX_RL_ENTRIES is the safety ceiling declared at module level.
+            if len(_feedback_rl) >= _MAX_RL_ENTRIES:
+                cutoff = now - 3600
+                expired = [k for k, (_, ws) in _feedback_rl.items() if ws < cutoff]
+                for k in expired:
+                    del _feedback_rl[k]
+            count, window_start = _feedback_rl.get(client_ip, (0, now))
+            if now - window_start > 3600:
+                count, window_start = 0, now
+            count += 1
+            _feedback_rl[client_ip] = (count, window_start)
+            if count > 30:
+                logger.warning(json.dumps({"event": "feedback.ratelimit", "ip": client_ip}))
+                raise HTTPException(
+                    status_code=429,
+                    detail="Rate limit exceeded. Maximum 30 feedback submissions per hour.",
+                    headers={"Retry-After": "3600"},
+                )
 
-    if not isinstance(payload, dict):
-        raise HTTPException(status_code=422, detail="Feedback body must be a JSON object.")
-
-    logger.info(
-        json.dumps(
-            {
-                "event":          "feedback.receive",
-                "ip":             client_ip,
-                "ratingValue":    payload.get("ratingValue"),
-                "model":          payload.get("model"),
-                "conversationId": payload.get("conversationId"),
-                "answerIndex":    payload.get("answerIndex"),
-                "page":           payload.get("page"),
-                "ts":             payload.get("ts"),
-                "persist":        FEEDBACK_PERSIST_ENABLED,
-            }
+        logger.info(
+            json.dumps(
+                {
+                    "event":          "feedback.receive",
+                    "ip":             client_ip,
+                    "ratingValue":    payload.get("ratingValue"),
+                    "model":          payload.get("model"),
+                    "conversationId": payload.get("conversationId"),
+                    "answerIndex":    payload.get("answerIndex"),
+                    "page":           payload.get("page"),
+                    "ts":             payload.get("ts"),
+                    "persist":        FEEDBACK_PERSIST_ENABLED,
+                }
+            )
         )
-    )
 
     # ── Optional HF dataset persistence ─────────────────────────────────────
     # Activated only when FEEDBACK_PERSIST_ENABLED=true AND the dataset repo
@@ -1575,6 +1620,11 @@ async def feedback(request: Request) -> JSONResponse:
     # so that a dataset-write error never breaks the user's rating experience
     # (the keepalive fire-and-forget model means the user won't see a retry UI
     # anyway).  Operators should monitor "feedback.persist_fail" log events.
+    #
+    # Retraction records ARE persisted (when persistence is enabled) because
+    # the training pipeline needs to see them to suppress the original rating.
+    # A retraction that arrives without persistence is a no-op — the original
+    # rating also wasn't persisted, so there is nothing to suppress.
     if FEEDBACK_PERSIST_ENABLED and TRAINING_DATASET_REPO and HF_DATASET_TOKEN:
         try:
             from huggingface_hub import CommitOperationAdd, HfApi
@@ -1594,7 +1644,12 @@ async def feedback(request: Request) -> JSONResponse:
                     "_ts":         int(_time.time() * 1000),
                     # Provenance — training pipelines must discard "feedback"
                     # records whenever a "contribution" record exists for the
-                    # same _dedup_key.  See DATASET_COLLECTION_GUIDANCE.md.
+                    # same _dedup_key.  Retraction tombstones also use
+                    # _source="feedback" so the LWW dedup on _dedup_key works
+                    # correctly (retraction overrides original; new rating
+                    # overrides retraction).  deduplicate_dataset.py filters
+                    # action="retract" winners from the clean output.
+                    # See DATASET_COLLECTION_GUIDANCE.md §3 and §8.
                     "_source":     "feedback",
                     # "{conversationId}:{answerIndex}" — identical key format
                     # to contribution records; equality check is sufficient to
@@ -1608,6 +1663,9 @@ async def feedback(request: Request) -> JSONResponse:
             # Fix 3 (feedback): same event-loop fix as /v1/contribute — offload
             # the synchronous HF HTTP call to a worker thread so the uvicorn
             # loop stays unblocked during the HuggingFace round-trip.
+            commit_msg = (
+                "Retract 1 feedback record" if is_retract else "Add 1 feedback record"
+            )
             await asyncio.to_thread(
                 api.create_commit,
                 repo_id=TRAINING_DATASET_REPO,
@@ -1618,12 +1676,13 @@ async def feedback(request: Request) -> JSONResponse:
                         path_or_fileobj=record.encode(),
                     )
                 ],
-                commit_message="Add 1 feedback record",
+                commit_message=commit_msg,
             )
             logger.info(
                 json.dumps(
                     {
                         "event":       "feedback.persist_ok",
+                        "retract":     is_retract,
                         "dedup_key":   f"{conversation_id}:{answer_index}",
                         "ip":          client_ip,
                     }