fix(ci): gate carto/main PRs on CARTO feature tests and harden the sync resolver - #124
Open
mateo-di wants to merge 2 commits into
Open
fix(ci): gate carto/main PRs on CARTO feature tests and harden the sync resolver#124mateo-di wants to merge 2 commits into
mateo-di wants to merge 2 commits into
Conversation
…nc resolver Three measures against the failure class that shipped the PR #121 wiring regression (streaming_iterator.py resolved against upstream while the auto-merged handler.py kept passing a CARTO-only kwarg - caught only by cloud-native integration tests, 3 repos downstream): 1. New carto-feature-tests.yml: runs the unit suites mirroring every file listed in .github/carto-features.yml (plus an import smoke) on all PRs to carto/main. Upstream's test workflows never trigger for carto/main, so until now zero Python tests ran on any PR here. Directory-level mirroring deliberately includes auto-merged sibling files' tests. The ready-checker requires all checks green, so this also gates sync-ready. 2. Resolver's manifest verification is now blocking (exit 1) instead of warning-only. Claude updates the manifest itself when upstream legitimately substitutes a feature, so a missing pattern at this point means a feature was dropped unaccounted for. 3. Resolver prompt gains a cross-file wiring check directive: verify call sites and attribute assignments across the whole package (including git-auto-merged files) after resolving each conflicted file, and re-read every file of a multi-file manifest feature together.
The runner's bare python3 has no PyYAML; the project venv does (litellm core dependency).
Collaborator
Author
|
Status: blocked on #121 merging first — and the red check here is the gate working as intended. The Evidence it's drift and not a workflow bug: the identical suites pass 971/971 on the Merge order: #121 first (replaces both code and tests with the consistent v1.92.0 state), then re-run checks here — the |
This was referenced Jul 17, 2026
mateo-di
added a commit
that referenced
this pull request
Aug 4, 2026
…sts (#125) * fix(ci): restore agent reaction to test failures via CARTO Feature Tests The upstream-sync fixer and ready-checker were wired to react to "LiteLLM Mock Tests" and "LiteLLM Linting" workflow_run completions. Both are dead on carto/main: Mock Tests is upstream-deprecated (workflow_dispatch only) and Linting only triggers for upstream's own branches. So no test ever ran on a carto/main sync PR, the fixer never fired, and the only failure signal was cloud-native integration tests three repos downstream (see the v1.92.0 sync, where three broken CARTO wirings shipped that way). Wire both workflows to "CARTO Feature Tests" (carto-feature-tests.yml, CARTO's own unit-test gate) instead: - ready-checker: replace the two dead names with Docker CI + Feature Tests. workflow_run only re-invokes ready-checker when a listed workflow completes, so listing Feature Tests is what makes sync-ready wait for the tests rather than just the build. - ci-fixer: same trigger list; swap the log-extraction query from the dead Mock Tests workflow to Feature Tests; align the prompt's local verification with the gate's real command (uv + make install-test-deps + manifest-derived scope) instead of the stale pip/pytest invocation. Supersedes #123 (which only removed the dead names). Depends on the CARTO Feature Tests workflow from #124. * docs(ci): align sync resolver/fixer/analyzer prompts with wiring-aware reasoning (#126) 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.
mateo-di
marked this pull request as ready for review
August 4, 2026 12:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Long-term fix for the failure class behind the PR #121 regression (
streaming_iterator.pyresolved to upstream's version while auto-mergedhandler.pykept passing a CARTO-only kwarg — every streaming request 500'd, caught only by cloud-native integration tests days later). Root-cause analysis showed every sync automation "passed" because none of them execute code: presence-greps passed while wiring broke, and upstream's unit-test workflows never trigger forcarto/main(theirpull_requestbranch filters only cover upstream branches), so zero Python tests run on any PR here.Changes
1. New
carto-feature-tests.yml— execution gate on everycarto/mainPR.github/carto-features.yml: for each feature file, runs the mirrored directory undertests/test_litellm/(walking up to the nearest existing dir). Currently resolves to 6 dirs:litellm_core_utils,llms/azure,llms/databricks/chat,llms/oci/chat,llms/snowflake/chat,responses/litellm_completion_transformation.handler.py) whose wiring can break when a conflicted neighbor is resolved against them — exactly the PR 🔄 Upstream Sync: LiteLLM v1.92.0 #121 blind spot.import litellmsmoke check.make install-test-deps(upstream-maintained target), actions pinned to the same SHAs upstream's workflows use.carto-upstream-sync-ready-checker.ymlrequires all PR checks green before addingsync-ready, this gate automatically blocks the automated release chain (n8n → cloud-native PR) on failure.2. Resolver manifest verification is now blocking
Previously warning-only ("Claude may have intentionally changed code") — it could never fail the build. Claude updates the manifest itself when upstream legitimately substitutes a feature (it did so correctly in PR #121 for the OCI tool-call feature), so a pattern still missing after resolution means a feature was dropped unaccounted for → fail before completing the merge.
3. Resolver prompt: cross-file wiring check directive
Instructs Claude to verify call sites and
self.<attr>assignments across the whole package — including git-auto-merged files — after resolving each conflicted file, and to re-read all files of a multi-file manifest feature together. Documents the PR #121 incident inline as the motivating example.Test plan
run:blocks passbash -ncarto-feature-tests.yml(it touches.github/workflows/carto-feature-tests.yml) — verify the job runs green on carto/main's codeFollow-ups (separate, after PR #121 merges, to avoid conflicts with the open sync)
carto-features.ymlwith wiring-level patterns (e.g. thelitellm_completion_requestparam + call-site patterns for Redis Session Storage,handler.pyadded to itsfiles:)tests/test_litellm/responses/litellm_completion_transformation/