chore: sync workflow templates#869
Conversation
Automated sync from stranske/Workflows Template hash: 1243c652ce3c Changes synced from sync-manifest.yml
stranske
added
sync
stranske
had a problem deploying
to
agent-standard
GitHub Actions
Failure
📝 WalkthroughWalkthrough
ChangesLangChain client builder and dynamic model resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/llm_provider.py`:
- Around line 638-642: The analyze_completion() method uses getattr() with a
default parameter that calls _configured_langchain_model() on every invocation,
but since _model_name is guaranteed to be set by _get_client() before
analyze_completion() is reached, this creates unnecessary registry lookups on a
frequently-called path. Replace the getattr() pattern with direct attribute
access to self._model_name (around line 638 and also at line 722-726 where the
same pattern applies) since the attribute is guaranteed to already exist,
eliminating the redundant function call and improving performance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d8d40aed-b145-48f8-aebf-b468411eb007
📒 Files selected for processing (2)
scripts/langchain/progress_reviewer.pytools/llm_provider.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
stranske/Workflows(auto-detected)stranske/Template(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Python CI / python 3.13
- GitHub Check: Python CI / python 3.12
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In the Manager-Database repository, use Prefect 2.x and import schedules from
prefect.client.schemas.schedules
Files:
scripts/langchain/progress_reviewer.pytools/llm_provider.py
🔀 Multi-repo context stranske/Template, stranske/Workflows
Perfect! I now have a complete picture of the cross-repository impact. Let me document my findings:
Linked repositories findings
stranske/Template
Critical Breaking Change in llm_provider.py:
Template's current OpenAIProvider._get_client() [::stranske/Template::] directly instantiates ChatOpenAI with a hardcoded model string:
return ChatOpenAI(
model="gpt-5.1-codex",
api_key=os.environ.get("OPENAI_API_KEY"),
temperature=0.1,
)However, Workflows' refactored version [::stranske/Workflows::] delegates client creation to build_chat_client() and expects to extract the model name from the returned ClientInfo object:
resolved = build_chat_client(provider="openai", model=model_name)
if resolved:
self._model_name = resolved.model
return resolved.clientIssue: The Workflows version calls _configured_langchain_model("openai", fallback="gpt-5.1-codex") [::stranske/Workflows::] on line 601 of llm_provider.py, but Template does not have this function. When Template syncs this code, it will fail with NameError: name '_configured_langchain_model' is not defined.
Similarly for AnthropicProvider._get_client() [::stranske/Workflows::] on line 672, which calls _configured_langchain_model("anthropic", fallback="claude-sonnet-4-5-20250929").
Secondary Issue - Model Name Reporting:
In both providers' analyze_completion() method, Workflows now uses:
model_name=getattr(
self,
"_model_name",
_configured_langchain_model("openai", fallback="gpt-5.1-codex"),
),[::stranske/Workflows::] (lines 641 and 721)
While Template hardcodes:
model_name="gpt-5.1-codex" # Actual model used by OpenAIProvider[::stranske/Template::]
This means the synced code will again call the non-existent _configured_langchain_model() function.
Impact on progress_reviewer.py:
Template's progress_reviewer.py line 430 currently imports from tools.langchain_client:
from tools.langchain_client import build_chat_client[::stranske/Template::]
Workflows changes this to import from a new wrapper:
from scripts.langchain._llm_client import build_client[::stranske/Workflows::] (line 430)
Both build_chat_client and build_client return ClientInfo objects with compatible interfaces, so this change should be forward-compatible. However, Template must have the _llm_client module available at that import path.
🔇 Additional comments (2)
tools/llm_provider.py (1)
45-52: LGTM!Also applies to: 596-606, 667-677
scripts/langchain/progress_reviewer.py (1)
430-434: LGTM!
| model_name=getattr( | ||
| self, | ||
| "_model_name", | ||
| _configured_langchain_model("openai", fallback="gpt-5.1-codex"), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify call sites using eager default expression
rg -n 'getattr\(\s*self,\s*"_model_name",\s*_configured_langchain_model' tools/llm_provider.py
# Demonstrate Python eager evaluation of function-call args (language behavior)
python - <<'PY'
def fallback():
print("fallback evaluated")
return "fallback"
class Obj:
_model_name = "already-set"
print(getattr(Obj(), "_model_name", fallback()))
PYRepository: stranske/Collab-Admin
Length of output: 47
🏁 Script executed:
#!/bin/bash
# List files to understand repo structure
git ls-files | head -20
# Look for llm_provider files
find . -name "*llm_provider*" -o -name "*langchain*" | head -20Repository: stranske/Collab-Admin
Length of output: 996
🏁 Script executed:
#!/bin/bash
# Check if tools directory exists
ls -la tools/ 2>/dev/null || echo "tools/ does not exist"
# List Python files in root and common locations
find . -maxdepth 3 -name "*.py" -type f | grep -E "(llm|provider|langchain)" | head -20Repository: stranske/Collab-Admin
Length of output: 1795
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review
echo "=== Lines 638-642 ==="
sed -n '638,642p' tools/llm_provider.py
echo ""
echo "=== Lines 719-723 ==="
sed -n '719,723p' tools/llm_provider.py
echo ""
echo "=== Check for _configured_langchain_model definition ==="
grep -n "_configured_langchain_model" tools/llm_provider.py | head -20Repository: stranske/Collab-Admin
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Look at the context around line 641 to understand when _model_name gets set
echo "=== Context around line 641 (OpenAI) ==="
sed -n '595,650p' tools/llm_provider.py
echo ""
echo "=== Context around line 722 (Anthropic) ==="
sed -n '665,730p' tools/llm_provider.pyRepository: stranske/Collab-Admin
Length of output: 4903
🏁 Script executed:
#!/bin/bash
# Look at the _configured_langchain_model function definition
echo "=== _configured_langchain_model function ==="
sed -n '45,80p' tools/llm_provider.pyRepository: stranske/Collab-Admin
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Check how often analyze_completion is called and confirm _model_name flow
echo "=== Check if _model_name is always set before getattr call ==="
sed -n '620,645p' tools/llm_provider.py
# Count how many times analyze_completion is called in the codebase
echo ""
echo "=== Call sites of analyze_completion ==="
rg -n "\.analyze_completion\(" --type py | head -20Repository: stranske/Collab-Admin
Length of output: 1527
🏁 Script executed:
#!/bin/bash
# Trace the exact control flow - when is analyze_completion called?
echo "=== Callers of analyze_completion in llm_provider.py ==="
sed -n '10,25p' tools/llm_provider.py
echo ""
echo "=== Line 935 context ==="
sed -n '930,945p' tools/llm_provider.py
echo ""
echo "=== Line 949 context ==="
sed -n '945,955p' tools/llm_provider.pyRepository: stranske/Collab-Admin
Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Verify that _model_name IS always set before analyze_completion returns CompletionAnalysis
echo "=== Full analyze_completion flow for OpenAI ==="
sed -n '612,655p' tools/llm_provider.py
# Check if there's any path where analyze_completion exits before setting _model_name
echo ""
echo "=== Check exception handling path ==="
sed -n '650,660p' tools/llm_provider.pyRepository: stranske/Collab-Admin
Length of output: 2053
🏁 Script executed:
#!/bin/bash
# Demonstrate the eager evaluation behavior with the exact code pattern
python3 << 'PY'
def expensive_fallback():
print("WARNING: expensive_fallback was evaluated")
return "fallback_value"
class Provider:
def __init__(self):
self._model_name = "already_set"
# Current pattern - default is ALWAYS evaluated
print("=== Current getattr() pattern ===")
provider = Provider()
result = getattr(provider, "_model_name", expensive_fallback())
print(f"Result: {result}")
print("\n=== Proposed pattern - lazy evaluation ===")
provider2 = Provider()
result2 = getattr(provider2, "_model_name", None) or expensive_fallback()
print(f"Result: {result2}")
# Verify the attribute IS set after _get_client
print("\n=== Checking if _model_name is always set when analyze_completion runs ===")
provider3 = Provider()
# Simulating: if _get_client() returns successfully, _model_name is already set
# Then analyze_completion calls getattr without needing the fallback
print(f"_model_name exists: {hasattr(provider3, '_model_name')}")
print(f"_model_name value: {getattr(provider3, '_model_name', 'NOT_SET')}")
PYRepository: stranske/Collab-Admin
Length of output: 357
Optimize model name resolution to avoid unnecessary registry lookups.
The getattr() pattern evaluates _configured_langchain_model(...) as a default argument on every call to analyze_completion(), even though _model_name is guaranteed to be set by _get_client() before reaching this code. This causes redundant registry lookups on a frequently-called path.
Proposed fix
- model_name=getattr(
- self,
- "_model_name",
- _configured_langchain_model("openai", fallback="gpt-5.1-codex"),
- ),
+ model_name=(
+ getattr(self, "_model_name", None)
+ or _configured_langchain_model("openai", fallback="gpt-5.1-codex")
+ ),- model_name=getattr(
- self,
- "_model_name",
- _configured_langchain_model("anthropic", fallback="claude-sonnet-4-5-20250929"),
- ),
+ model_name=(
+ getattr(self, "_model_name", None)
+ or _configured_langchain_model(
+ "anthropic", fallback="claude-sonnet-4-5-20250929"
+ )
+ ),Also applies to: 722–726
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/llm_provider.py` around lines 638 - 642, The analyze_completion()
method uses getattr() with a default parameter that calls
_configured_langchain_model() on every invocation, but since _model_name is
guaranteed to be set by _get_client() before analyze_completion() is reached,
this creates unnecessary registry lookups on a frequently-called path. Replace
the getattr() pattern with direct attribute access to self._model_name (around
line 638 and also at line 722-726 where the same pattern applies) since the
attribute is guaranteed to already exist, eliminating the redundant function
call and improving performance.
1 participant
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Source SHA:
680260c913e1c136e0b0ba11c07a760e23c26b0cTemplate hash:
1243c652ce3cSync branch:
sync/workflows-1243c652ce3cConsumer repo:
stranske/Collab-AdminManifest:
.github/sync-manifest.ymlSummary by CodeRabbit