-
Notifications
You must be signed in to change notification settings - Fork 0
chore: restore reverted test suites, folder structure, logic optimizations, and security patches (v5) #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7a9755b
d239166
4422f46
7c457f7
4d8cea0
e45e7fc
36fa94a
6cdf604
660811f
ad44754
b0a8605
186c9d4
e7792af
ba7deff
9314a5e
d45b951
1963807
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # Git Merges & Regressions Audit Report | ||
|
|
||
| ## Problem Overview | ||
| During the merging of concurrent pull requests, automated conflict-resolution bots (`jules[bot]`) silently discarded or reverted several code blocks and logic optimizations. | ||
|
|
||
| To ensure complete stability, we performed a systematic, automated three-dot diffing audit (`git diff CURRENT_BRANCH...TARGET_BRANCH`) comparing our current workspace against every active and merged remote branch in the repository. | ||
|
|
||
| --- | ||
|
|
||
| ## Branch-by-Branch Audit Checklist | ||
|
|
||
| | Branch Name / PR | Status | Notes / Discrepancies Checked | | ||
| | :--- | :---: | :--- | | ||
| | `origin/cleanup/remove-unused-get-live-gemini-oauth-token` (PR #195) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/perf-optimize-gemini-oauth-token-7488735575783948165` (PR #158) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/refactor-record-tool-usage-17779903941173107649` (PR #157) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/get-pr-status-tests-18330387033569416831` (PR #193) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/perf-async-annotations-4499495305587811104` (PR #172) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/add-gemini-oauth-tests-17524029845400706758` (PR #163) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/dependabot/docker/berriai/litellm-v1.90.2` (PR #192) | **CLEAN** | Diff showed `v1.90.2` LiteLLM image version bump, which is already applied. | | ||
| | `origin/security/fix-memory-mcp-category-injection-14795006529015952965` (PR #173) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/test/add-read-annotations-sync-tests-12043640228306609857` (PR #168) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/test-parse-key-parameterized-14651890987031951450` (PR #169) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/perf/async-quota-exhausted-4722057800691922723` (PR #156) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/perf-benchmark-classifier-14678555128983342452` (PR #155) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/add-test-redis-from-url-12746122485451637611` (PR #174) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/test-is-memory-key-14668590644102848561` (PR #164) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/⚡/optimize-agy-log-read-1519383298987985129` (PR #170) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/add-oauth-status-tests-15591368961301252072` (PR #166) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/chore/test-get-goose-sessions-16558526271462662412` (PR #165) | **CLEAN** | All changes present in current codebase. | | ||
| | `origin/fix-http-client-proliferation-11317864999360875839` | **CLEAN** | HTTP client unification is fully present in our workspace. | | ||
| | `origin/fix/test-record-tool-usage-3580129709200353766` | **CLEAN** | Root-level test files were reorganized to `/tests`, fully preserved. | | ||
| | `origin/fix/secrets-relocation` (PR #184) | **CLEAN** | Checked out placeholders and updated `start-stack.sh`/`pod.yaml` securely. | | ||
| | `origin/perf/offload-aa-scores-sync-16551127323385849872` (PR #98) | **RESTORED** | **Regression Found**: The asynchronous scores loading optimization was lost on master. Re-implemented and verified. | | ||
|
|
||
| --- | ||
|
|
||
| ## Detailed Regression Analysis & Fix: Offload AA Scores Loading | ||
|
|
||
| ### The Regression | ||
| * **File:** `router/main.py` | ||
| * **Issue:** `_load_aa_scores()` (which does a blocking synchronous JSON disk read of `aa_scores.json`) was called directly inside `compute_free_model_score()`. When resolving or syncing OpenRouter free models, this synchronous file read was executed sequentially in a loop, blocking the main event loop. | ||
| * **Merged Fix Lost:** The merged PR #98 had offloaded this operation to a background thread pool using `await asyncio.to_thread(_load_aa_scores)` outside the loops in `sync_adaptive_router_roster` and `get_best_free_model`, but this change was completely discarded during automated conflict resolution. | ||
|
|
||
| ### The Resolution | ||
| 1. **`router/main.py`**: | ||
| * Restored `await asyncio.to_thread(_load_aa_scores)` inside `sync_adaptive_router_roster()` and `get_best_free_model()` if the cache is not yet loaded. | ||
| * Removed the blocking `_load_aa_scores()` call from inside `compute_free_model_score()`. | ||
| 2. **`tests/test_compute_free_model_score.py`**: | ||
| * Updated the test cases to explicitly call `router_main._load_aa_scores()` within the test setups to align with the asynchronous offloading. | ||
|
|
||
| --- | ||
|
|
||
| ## Verification Status | ||
| * **Test Suite:** Ran `PYTHONPATH=router:. pytest` | ||
| * **Result:** **176 tests passed** successfully (100% success rate). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # Analysis of Bot Merge Conflict Regressions & Guardrails | ||
|
|
||
| This document breaks down the root causes of the regressions introduced by the automated agent (`jules[bot]`) during recent merges, compares Git resolution strategies, and outlines prompt modifications and guardrails to prevent future occurrences. | ||
|
|
||
| --- | ||
|
|
||
| ## 1. What Went Wrong? | ||
|
|
||
| The primary failure arose from a combination of **reorganization conflict logic** and **blind resolution choices**: | ||
|
|
||
| ### A. The Directory Rename / Delete Conflict Mismatch | ||
| * **Context**: PR #181 reorganized the repository by moving files at the root (like `test_a2_verify.py` and `host_agy_daemon.py`) into nested directories (`tests/` and `scripts/`). | ||
| * **The Bot's Branch State**: The bot was working on older branches (`perf-optimize-gemini-oauth-token`, `refactor-record-tool-usage`) whose base commits predated the reorganization. | ||
| * **The Failure**: When merging `master` into these older feature branches, Git detected conflict types like `rename/delete` or `directory rename detection`. Because the bot resolved conflicts locally inside the old workspace structure, it staged the **deletion** of the new files inside `tests/` and `scripts/` and re-introduced older, obsolete root-level files. | ||
| * **Result**: Merges from these branches back to `master` cleanly deleted the subfolder-bound tests and re-added old copies at the root. | ||
|
|
||
| ### B. Lack of Cross-Check Verification | ||
| * The bot resolved conflicts file-by-file without compiling the whole project or validating the full test suite (`pytest`) on the combined codebase. | ||
| * It accepted obsolete file versions wholesale (e.g., reverting the dynamic passwords in `pod.yaml` and `start-stack.sh` to hardcoded credentials) because it assumed conflict markers in one block did not impact security/logic blocks elsewhere. | ||
|
|
||
| --- | ||
|
|
||
| ## 2. Which Git Strategy Should Have Been Used? | ||
|
|
||
| Instead of merging `master` directly into older feature branches (which creates complex, multi-directional merge graphs), the following strategies are far safer for LLM agents: | ||
|
|
||
| ### Option A: `git rebase master` (Recommended for Feature Branches) | ||
| Rather than a merge commit, the agent should rebase the feature branch onto the latest `master` commit: | ||
| ```bash | ||
| git checkout feature-branch | ||
| git fetch origin | ||
| git rebase origin/master | ||
| ``` | ||
| * **Why it works**: Rebase replays each feature branch commit one-by-one on top of the reorganized `master` commit. | ||
| * **Rename Tracking**: Git's rename tracking algorithm handles moves seamlessly during a rebase. If a file was renamed from `test_a2_verify.py` to `tests/test_a2_verify.py` on `master`, Git will automatically apply the feature branch's modifications to `tests/test_a2_verify.py` instead of leaving them at the root. | ||
|
Comment on lines
+27
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Soften the rename-handling claim. Git rebase can help, but it does not guarantee that edits will be auto-applied onto renamed paths. Please phrase this as a manual verification step, not an automatic guarantee. Suggested wording-* Git's rename tracking algorithm handles moves seamlessly during a rebase.
+* Git may detect renames during a rebase, but the agent still needs to verify the new path and resolve any rename/delete conflicts manually.🤖 Prompt for AI Agents |
||
|
|
||
| ### Option B: Merge with explicit Merge Drivers | ||
| If rebasing is not used, the agent must inspect renames before committing: | ||
| ```bash | ||
| git diff --name-status origin/master...HEAD | ||
| ``` | ||
| This lists any deleted/added files to quickly verify that no folder moves were silently discarded. | ||
|
|
||
| --- | ||
|
|
||
| ## 3. Recommended Prompt Guardrails and System Rules | ||
|
|
||
| To prevent automated agents from causing directory and code regressions, the following rules should be appended to the agent's instructions (e.g., in `.agents/AGENTS.md` or system guidelines): | ||
|
|
||
| ### Rule 1: Git Conflict Rebase Mandate | ||
| > [!IMPORTANT] | ||
| > When updating a feature branch with `master`/`main` changes, always prefer `git rebase` over `git merge`. If conflicts arise due to renamed directories, do not manually delete folders or re-add root counterparts. Ensure files are modified in their new paths. | ||
|
|
||
| ### Rule 2: Complete Test Suite Verification | ||
| > [!WARNING] | ||
| > Never push conflict resolutions without running the full test suite. | ||
| > * If the workspace previously had $N$ passing tests, the resolved branch must have at least $N$ passing tests. | ||
| > * Confirm all files staged for deletion or addition are intentional using `git diff --stat origin/master`. | ||
|
|
||
| ### Rule 3: File Integrity & Verification Checklist | ||
| Add a post-conflict verification script step to the agent workflow: | ||
| ```bash | ||
| # Verify no files were moved to root unexpectedly | ||
| git status --porcelain | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Proposed Instructions / Prompts for Bot Agents | ||
|
|
||
| When tasking an agent with conflict resolution or merging, use a structured prompt like this: | ||
|
|
||
| ```markdown | ||
| You are resolving merge conflicts for the branch [branch_name]. | ||
|
|
||
| 1. Run `git fetch origin` and `git rebase origin/master`. | ||
| 2. If conflicts arise, inspect whether any files were renamed/moved in master. Apply your changes to the renamed files in their new locations, rather than re-creating them at their old locations. | ||
| 3. Once rebased, run the entire test suite: `pytest`. | ||
| 4. Run `git diff --stat origin/master` and review every file addition/deletion. Verify that no tests or scripts are missing compared to master. | ||
| 5. If any test files or core logic sections are missing, checkout the missing files from master and apply changes cleanly. | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,13 +23,13 @@ jobs: | |
| python-version: '3.11' | ||
|
|
||
| - name: Install dependencies | ||
| run: pip install httpx==0.28.1 pytest pytest-asyncio anyio pyyaml fastapi "pydantic>=2.0,<3.0" uvicorn python-multipart asyncpg langfuse redis | ||
| run: pip install httpx==0.28.1 pytest pytest-asyncio anyio pyyaml fastapi "pydantic>=2.0,<3.0" uvicorn python-multipart asyncpg langfuse redis aiofiles | ||
|
|
||
| - name: Run Unit Tests | ||
| run: CONFIG_PATH=router/config.yaml PYTHONPATH=.:router pytest --ignore=test_agy_behavior.py --ignore=test_agy_tiers.py | ||
| run: CONFIG_PATH=router/config.yaml PYTHONPATH=.:router pytest --ignore=tests/test_agy_behavior.py --ignore=tests/test_agy_tiers.py | ||
|
Comment on lines
28
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check whether the non-ignored "live infra" tests guard themselves (skip/mark) when services are unavailable.
rg -n "pytest.mark.skip|pytest.importorskip|requests.exceptions|ConnectionError|localhost|127.0.0.1" tests/test_antigravity.py tests/test_classifier_accuracy.py tests/test_stream_latency.py 2>/dev/nullRepository: sheepdestroyer/LLM-Routing Length of output: 349 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Show the relevant files with line numbers, but only the first ~220 lines each.
for f in tests/test_antigravity.py tests/test_classifier_accuracy.py tests/test_stream_latency.py; do
echo "===== $f ====="
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
echo
doneRepository: sheepdestroyer/LLM-Routing Length of output: 16376 Exclude 🤖 Prompt for AI Agents |
||
|
|
||
| - name: Run Breaker Verification | ||
| run: python3 verify_breaker.py | ||
| run: PYTHONPATH=. python3 scripts/verification/verify_breaker.py | ||
|
|
||
| - name: Run Integration Verification | ||
| run: python3 test_a2_verify.py | ||
| run: PYTHONPATH=.:router python3 tests/test_a2_verify.py | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the AA-score regression location.
The blocking load lives in
router/main.py:520-545androuter/main.py:1428-1460, not insidecompute_free_model_score(). Reword this section so the audit points at the actual call site; otherwise the recovery note is misleading.Suggested wording
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Linked repositories