From 04b13d6bebbd861fd14e445691fd2d4d37f292d0 Mon Sep 17 00:00:00 2001 From: Mateo Di Loreto Date: Fri, 17 Jul 2026 15:35:01 -0300 Subject: [PATCH] docs(ci): align sync resolver/fixer/analyzer prompts with wiring-aware reasoning The v1.92.0 sync shipped three CARTO features that passed every automated check yet were functionally broken (dropped call site across an auto-merged file, an orphaned helper with no caller, and a store/lookup key that mismatched after upstream changed id encoding). None were catchable by the prompts' existing "does the pattern still grep" verification, and two of the prompts' heuristics actively caused the loss. This aligns the prompts with the reasoning that actually found and fixed those bugs. Resolver prompt: - Verbatim-first rule: restore CARTO blocks byte-identical from carto/main (diff-verified); adapt only where an upstream API change makes verbatim impossible, minimally and marked # CARTO PATCH. Replaces "preserve the BEHAVIOR, not necessarily the exact file versions", which licensed the paraphrase that dropped a session read-path. - Remove harmful heuristics: delete "bigger file = probably correct" and reframe "upstream TAG code WORKS" as "works for upstream's call graph, not necessarily CARTO's - re-verify callers, attributes, data formats". - Add three post-resolution checks the grep cannot do: orphan sweep (helper with no caller), cross-file wiring (auto-merged siblings), and cross-version data-flow (format drift across the version boundary). - Reference the regression canaries as the definition of "wired correctly". Fixer prompt: - Same verbatim-first rule, orphan sweep, and call-graph reframing. - Manifest-aware loop guard: the "3+ fixes -> sync entire file from upstream" escape hatch now excludes manifest files; for those, take upstream as base and re-apply the CARTO block verbatim (blind sync is how wirings get erased). - Log extraction / verification already retargeted to CARTO Feature Tests in the agent-reaction PR this is stacked on. Analyzer prompt: - Judge PRESERVED_CARTO by WIRING, not string presence: an orphaned helper, a missing call site, or broken data-flow is INCORRECTLY_DROPPED even when the def/pattern greps OK. It had reported the dead session read as PASS. CARTO_UPSTREAM_SYNC.md: - New troubleshooting section documenting the three v1.92.0 wiring-loss classes with detection commands. Manifest wiring-pattern enrichment (adding call-site patterns to carto-features.yml) is deliberately deferred to a follow-up to avoid clobbering the manifest changes on the open v1.92.0 sync PR. --- .github/workflows/CARTO_UPSTREAM_SYNC.md | 49 ++++++++++ .../carto-upstream-sync-ci-fixer.yml | 42 ++++++++- ...-upstream-sync-customizations-analyzer.yml | 22 +++++ .../carto-upstream-sync-resolver.yml | 94 ++++++++++++++++++- 4 files changed, 200 insertions(+), 7 deletions(-) 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. ---