diff --git a/.github/workflows/CARTO_UPSTREAM_SYNC.md b/.github/workflows/CARTO_UPSTREAM_SYNC.md index bdd3ce05c0b8..8460a7e5bbbe 100644 --- a/.github/workflows/CARTO_UPSTREAM_SYNC.md +++ b/.github/workflows/CARTO_UPSTREAM_SYNC.md @@ -351,6 +351,55 @@ The workflow sends Slack notifications to `#cartodb-ops`: 3. Run locally: `make lint && make test-unit` 4. Push fixes to the sync branch +### A CARTO feature broke after a sync (silent wiring loss) + +The v1.92.0 sync is the case study: the merge left CARTO customizations +that PASSED every presence check yet were functionally broken. Three +distinct wiring failures, each invisible to string-grep verification, only +surfaced in cloud-native integration tests three repos downstream. When a +feature misbehaves after a sync but its manifest patterns still grep OK, +check these in order. + +**1. Dropped call site across an auto-merged file.** Git auto-merges files +only one side changed; they are not in the resolver's conflict list, so a +signature rewritten in a conflicted file can leave a caller in an +auto-merged sibling passing a now-removed argument. Symptom: `TypeError: +... got an unexpected keyword argument`. In v1.92.0, `streaming_iterator.py` +(conflicted) lost a param while `handler.py` (auto-merged) kept passing it. +Find it by grepping call sites of any rewritten signature: +```bash +grep -rn "LiteLLMCompletionStreamingIterator(" litellm/ | grep -v "def " +``` + +**2. Orphaned CARTO helper (present but never called).** A helper survives +the merge byte-for-byte, so its `def` pattern greps OK, but the code that +CALLED it was replaced by the upstream version. Symptom: the feature simply +does nothing. In v1.92.0, `_patch_get_session_from_redis` was defined but +had zero callers, so sessions were written to Redis but read from the +batch-delayed DB, and multi-turn conversations lost context. Find orphans: +```bash +for fn in $(git grep -hoE "def (_patch_[a-z_0-9]+|_carto_[a-z_0-9]+)" -- litellm/ | sed -E 's/def //' | sort -u); do + refs=$(git grep -c "$fn" -- litellm/ | awk -F: '{s+=$NF} END {print s}') + defs=$(git grep -c "def $fn" -- litellm/ | awk -F: '{s+=$NF} END {print s}') + [ "$refs" -le "$defs" ] && echo "ORPHAN: $fn" +done +``` + +**3. Cross-version data-format drift.** CARTO code is unchanged and fully +wired, but upstream changed the format of a value flowing through it, so a +stored key no longer matches its lookup. Symptom: silent 100% cache/lookup +miss. In v1.92.0, upstream began b64-encoding response ids; CARTO's Redis +store keyed by the encoded id while the lookup used the decoded id. This +class cannot be found by grep - only by tracing what each feature consumes +and produces across the version boundary, or by a behavioral test. + +**Fix approach for all three:** restore the CARTO block verbatim from +`origin/carto/main` (never paraphrase); adapt only the call site or the +data-format handling, minimally, marked `# CARTO PATCH`. The regression +canaries in `tests/test_litellm/responses/litellm_completion_transformation/` +pin these three wirings; the CARTO Feature Tests gate runs them on every +sync PR, and the CI fixer reacts to that gate's failures. + ### Workflow Not Detecting New Releases 1. Check `gh release list --repo BerriAI/litellm` for the latest non-prerelease, non-draft tag (BerriAI dropped the `-stable` suffix after `v1.83.14-stable`, published 2026-05-02 — releases since are plain `vX.Y.Z` tags) diff --git a/.github/workflows/carto-upstream-sync-ci-fixer.yml b/.github/workflows/carto-upstream-sync-ci-fixer.yml index 59082bb6e01a..4e960e2e1c6f 100644 --- a/.github/workflows/carto-upstream-sync-ci-fixer.yml +++ b/.github/workflows/carto-upstream-sync-ci-fixer.yml @@ -424,8 +424,12 @@ jobs: **Docker Build Failed:** ${{ steps.extract-errors.outputs.docker-failed }} **CARTO Feature Tests Failed:** ${{ steps.extract-errors.outputs.tests-failed }} - **Key insight:** The upstream TAG code is TESTED and WORKING. If something is "missing", - the conflict resolver likely kept an old carto/main version instead of the new upstream TAG. + **Key insight:** Upstream TAG code works for UPSTREAM's call graph, not + necessarily for CARTO's. It is tested, but CARTO adds call sites, + parameters, and stored-data contracts upstream never exercises. When a + fix accepts an upstream file, re-verify CARTO's callers, attributes, and + data formats still line up - a file that imports cleanly can still be + broken at every CARTO call site. --- @@ -439,6 +443,25 @@ jobs: These patterns MUST still exist after your fixes. If a fix would remove a pattern, find an alternative approach that preserves the CARTO feature. + **Restore CARTO code VERBATIM, do not paraphrase.** When a fix needs a + CARTO block back, copy it byte-identical from carto/main + (`git show origin/carto/main:`) - including comments and debug + logging - and confirm with `diff`. Adapt only where an upstream API + change makes verbatim impossible, keep it minimal, and mark it + `# CARTO PATCH`. + + **The manifest grep proves a string EXISTS, not that it is WIRED.** + After your fix, run an orphan sweep - a CARTO helper defined with no + caller is a half-restored feature (the v1.92.0 sync left + `_patch_get_session_from_redis` defined but uncalled): + ```bash + for fn in $(git grep -hoE "def (_patch_[a-z_0-9]+|_carto_[a-z_0-9]+)" -- litellm/ | sed -E 's/def //' | sort -u); do + refs=$(git grep -c "$fn" -- litellm/ | awk -F: '{s+=$NF} END {print s}') + defs=$(git grep -c "def $fn" -- litellm/ | awk -F: '{s+=$NF} END {print s}') + [ "$refs" -le "$defs" ] && echo "ORPHAN: $fn (defined, never called)" + done + ``` + --- ## STEP 0: FIX LOOP DETECTION (DO THIS FIRST!) @@ -449,13 +472,26 @@ jobs: cat /tmp/pr_comments.md | grep -iE "Added.*function|sync.*file|ImportError|fix:" | head -15 ``` - **If same file appears 3+ times → SYNC ENTIRE FILE from upstream TAG:** + **If same file appears 3+ times → break the loop, BUT check the + manifest first.** + + If the file is NOT in any feature's `files:` in + `.github/carto-features.yml`, sync the entire file from upstream TAG: ```bash # ORIG_HEAD = upstream TAG version (pre-merge state) git show ORIG_HEAD:path/to/problematic_file.py > path/to/problematic_file.py git add path/to/problematic_file.py ``` + If the file IS a manifest file, do NOT blindly sync it from upstream - + that is exactly how CARTO wirings get erased. Instead, take upstream's + version as the base and re-apply the CARTO block VERBATIM from + carto/main, then run the orphan/wiring checks: + ```bash + git show ORIG_HEAD: > # upstream base + git show origin/carto/main: | less # copy CARTO blocks back byte-identical + ``` + --- ## STEP 1: READ CI LOGS diff --git a/.github/workflows/carto-upstream-sync-customizations-analyzer.yml b/.github/workflows/carto-upstream-sync-customizations-analyzer.yml index 5659c41dafd0..8552ea7ba71d 100644 --- a/.github/workflows/carto-upstream-sync-customizations-analyzer.yml +++ b/.github/workflows/carto-upstream-sync-customizations-analyzer.yml @@ -446,6 +446,28 @@ jobs: | PRESERVED_CARTO | Full CARTO implementation kept | CARTO code present, upstream version not used | | INCORRECTLY_DROPPED | CARTO feature was lost (BUG!) | CARTO code missing, upstream doesn't provide equivalent | + ## Judge by WIRING, not string presence + + A feature is PRESERVED_CARTO only if it is fully WIRED, not merely + present. Code that greps OK can still be dead. Before classifying any + feature as preserved, verify: + + - **No orphaned helpers:** every CARTO helper the feature defines + (`_patch_*` / `_carto_*`) has at least one caller. A helper defined + with zero call sites means the feature is INCORRECTLY_DROPPED, even + though its `def` string is present. (v1.92.0 left + `_patch_get_session_from_redis` defined but uncalled - a real bug the + string-presence check reported as "preserved".) + - **Call sites intact across auto-merged files:** parameters the + feature relies on are still passed by callers (which may live in + files git auto-merged, not just the conflicted ones). + - **Data-flow contracts hold:** values the feature stores/sends still + match the format the surrounding upstream code now expects (e.g. id + encoding, message shape). + + If any of these fail, the correct decision is INCORRECTLY_DROPPED with + the specific broken wiring named in the evidence. + CONTEXT_EOF # Add CARTO features list diff --git a/.github/workflows/carto-upstream-sync-resolver.yml b/.github/workflows/carto-upstream-sync-resolver.yml index 3f2e17c6701e..ee868320c347 100644 --- a/.github/workflows/carto-upstream-sync-resolver.yml +++ b/.github/workflows/carto-upstream-sync-resolver.yml @@ -770,7 +770,27 @@ jobs: ## CARTO FEATURES TO PRESERVE These are the INTENTIONAL CARTO customizations from merged PRs. - **Preserve the BEHAVIOR, not necessarily the exact file versions.** + + **VERBATIM-FIRST RULE (do not paraphrase CARTO code).** For any file + listed in a feature's `files:` in `.github/carto-features.yml`, + restore the CARTO block BYTE-IDENTICAL from carto/main: + ```bash + git show origin/carto/main: # source of truth for CARTO blocks + ``` + Copy the exact lines (including comments and debug logging) and + confirm with `diff`. Do NOT rewrite from memory or "clean it up" - + a paraphrase that looks equivalent is how wiring silently breaks + (v1.92.0 dropped a session read-path this way while its helper + survived). + + Adaptation is allowed ONLY when an upstream API change makes a + verbatim copy impossible (e.g. a signature or data-format change). + When it is, keep the change minimal (a few lines), mark it + `# CARTO PATCH` with the reason, and prefer adapting the CALL SITE + over rewriting the CARTO block. Worked example of a legitimate + adaptation: v1.92.0 began b64-encoding response ids, so the Redis + session store had to decode `response.id` before keying - the CARTO + helper stayed byte-identical, only the 2 call sites changed. Read the feature context: ```bash @@ -822,6 +842,66 @@ jobs: If ANY patterns are MISSING, restore them from carto/main or re-implement. + **The manifest grep proves a string EXISTS, not that it is WIRED.** + A helper can exist with no caller; a parameter can be accepted but + never passed; a stored key can no longer match its lookup. The three + checks below catch what the grep cannot. Do all three before + completing the merge. + + ### CHECK 1: Orphan sweep (dropped call sites) + For every CARTO helper (functions named `_patch_*` / `_carto_*` and + anything marked `# CARTO`), confirm it still has a caller: + ```bash + for fn in $(git grep -hoE "def (_patch_[a-z_0-9]+|_carto_[a-z_0-9]+)" -- litellm/ | sed -E 's/def //' | sort -u); do + refs=$(git grep -c "$fn" -- litellm/ | awk -F: '{s+=$NF} END {print s}') + defs=$(git grep -c "def $fn" -- litellm/ | awk -F: '{s+=$NF} END {print s}') + [ "$refs" -le "$defs" ] && echo "ORPHAN: $fn (defined, never called)" + done + ``` + An orphaned CARTO helper means its feature is only half-restored (the + v1.92.0 sync left `_patch_get_session_from_redis` defined but uncalled, + so sessions were written to Redis but read from the batch-delayed DB). + Restore the call site verbatim from carto/main. + + ### CHECK 2: Cross-file wiring (auto-merged siblings) + Git auto-merges files only one side changed - they are NOT in your + conflict list, but they can call code you just rewrote. After + resolving each conflicted file, for every signature you changed or + replaced with upstream's, grep the whole repo for its call sites and + confirm every kwarg still exists: + ```bash + grep -rn "(" litellm/ | grep -v "def " + ``` + (v1.92.0: `streaming_iterator.py` was conflicted and lost a param + while the auto-merged `handler.py` kept passing it - every streaming + request then raised TypeError.) For any multi-file manifest feature, + re-read ALL of its `files:` together, including auto-merged ones. + + ### CHECK 3: Cross-version data-flow + For each feature, trace what it CONSUMES and PRODUCES through the new + upstream code - ids, message shapes, metadata keys - not just whether + its text survived. Ask: does the value this CARTO code stores/sends + still match the format the (possibly rewritten) upstream code around + it now expects? (v1.92.0 began b64-encoding response ids: CARTO's + unchanged store keyed Redis by the encoded id while the lookup used + the decoded id, so every session lookup missed.) + + ### Canary tests (the definition of "wired correctly") + These regression tests encode the wirings that broke in the v1.92.0 + sync. The CARTO Feature Tests gate runs them on the PR; your + resolution must keep them green. If a test environment is available, + run them before completing the merge: + ```bash + uv run --no-sync pytest \ + tests/test_litellm/responses/litellm_completion_transformation/test_session_handler.py \ + tests/test_litellm/responses/litellm_completion_transformation/test_tool_call_streaming_transformation.py \ + -q 2>&1 | tail -20 + ``` + They pin: the streaming iterator's `litellm_completion_request` + wiring, the Redis-first session read, and the store-key == + decoded-lookup-key roundtrip. A green grep with a red canary means + the feature is broken - fix the wiring, do not touch the test. + --- ## SYNC-REQUIRED FILES (Always Accept Upstream TAG) @@ -864,10 +944,16 @@ jobs: ## KEY PRINCIPLE - **Upstream TAG code WORKS** - it's from a tested stable release. If something is "missing", - the conflict resolver probably kept an old carto/main version instead of the new upstream TAG. + **Upstream TAG code works for UPSTREAM's call graph - not necessarily + for CARTO's.** It is from a tested stable release, but CARTO adds call + sites, parameters, and stored-data contracts that upstream's tests + never exercise. So when you accept an upstream file, you are NOT done: + you must re-verify that CARTO's callers, attributes, and data formats + still line up (see the wiring and data-flow checks above). A file that + imports cleanly can still be broken at every CARTO call site. - When in doubt: Compare file sizes. Bigger = usually newer = probably correct. + Do NOT choose versions by file size or "bigger = newer". Decide by what + preserves CARTO behavior verbatim while adopting upstream's changes. ---