-
Notifications
You must be signed in to change notification settings - Fork 0
chore: PR 206 review feedback and declarative PGDATA version-gating #207
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
Closed
Closed
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
7a9755b
chore: restore deleted test suites, project directory structure, and …
sheepdestroyer d239166
docs: update LiteLLM version pinning and test suite paths
sheepdestroyer 4422f46
chore: add bot rebase/conflict guidelines and regression report to .a…
sheepdestroyer 7c457f7
ci: add aiofiles to dependencies and update test paths
sheepdestroyer 4d8cea0
test: wrap test_agy_behavior.py execution in main block to prevent im…
sheepdestroyer e45e7fc
chore: restore category sanitization security patch in memory_mcp.py …
sheepdestroyer 36fa94a
chore: address review feedback, restore async agy log reading and cla…
sheepdestroyer 6cdf604
chore: restore clean secret placeholders in pod.yaml/start-stack.sh a…
sheepdestroyer 660811f
chore: add branch audit plan and restore offload-aa-scores performanc…
sheepdestroyer ad44754
chore: address CodeRabbit review feedback on PR #198
sheepdestroyer b0a8605
perf: optimize async log checking in agy_proxy to seek and read 1KB
sheepdestroyer 186c9d4
chore: address new CodeRabbit and Gemini Code Assist reviews
sheepdestroyer e7792af
fix(chore): resolve broken script paths, sys.path imports and relativ…
sheepdestroyer ba7deff
fix(triage): truncate classifier prompt to 300 chars to avoid cache-r…
sheepdestroyer 9314a5e
fix(triage): map virtual target_model llm-routing-agy to agent-advanc…
sheepdestroyer d45b951
docs: add section 9d documenting live stack tier testing script and s…
sheepdestroyer 1963807
feat(triage): make classifier prompt truncation limit configurable vi…
sheepdestroyer a08d453
chore: address PR review feedback and clean up test boilerplate
sheepdestroyer 2eae001
Address and resolve PR #204 review feedback
sheepdestroyer ee7584d
Fix bash syntax error: remove local outside function
sheepdestroyer 5f86090
chore: address unaddressed PR 205 review findings
sheepdestroyer af6489c
chore: address PR 206 review feedback and implement declarative PGDAT…
sheepdestroyer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| 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 at free-model/roster call sites, specifically `sync_adaptive_router_roster()` and `get_best_free_model()`, rather than inside `compute_free_model_score()`. The fix was to move `_load_aa_scores()` behind `await asyncio.to_thread(...)` and remove the direct synchronous call from the scoring path. | ||
| * **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:** **181 tests passed** successfully (100% success rate). |
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
| 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. | ||
| * **Verify Relocated Files**: While Git's rename detection can assist with directory/file moves during rebase, it is not foolproof. The agent must manually verify relocated files (such as renamed test paths) after rebasing to ensure all changes landed in their correct new locations rather than being lost or orphaned. | ||
|
|
||
| ### 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, addition, or rename are intentional by running `git diff --name-only origin/master...HEAD` and inspecting the content diffs of renamed or moved files. | ||
|
|
||
| ### 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 --name-only origin/master...HEAD` and inspect the full content diff for renamed, deleted, or restored files. | ||
| 5. Only restore files from `master` after confirming the deletion was accidental; do not auto-checkout files that may have been intentionally renamed or removed. | ||
| ``` |
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
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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file was deleted.
Oops, something went wrong.
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
Oops, something went wrong.
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.
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.
The
timezonename is used here but it is not imported in this file, which will raise aNameErrorat runtime when serializing datetimes. Please importtimezonefromdatetime.