chore: sync workflow templates#875
Conversation
Automated sync from stranske/Workflows Template hash: d0d4ad2bdd25 Changes synced from sync-manifest.yml
stranske
added
sync
stranske
had a problem deploying
to
agent-standard
GitHub Actions
Failure
📝 WalkthroughWalkthroughThree independent changes: CI action pins are bumped for ChangesCI Action Pin Updates
Design System Kit
LLM Client and Provider Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🤖 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 `@design-system/ds_streamlit.py`:
- Around line 109-117: The notice function's docstring documents that the action
parameter accepts markdown, but the implementation on line 116 escapes the
action string before rendering it, which prevents markdown from being formatted.
To align the behavior with the documentation, remove the escape() call from the
act variable assignment that processes the action parameter, allowing markdown
formatting to work as documented while still ensuring safe HTML rendering for
the action content.
- Around line 180-186: The humanize_id function's fallback path still exposes
raw internal ID information despite the "never show raw keys" contract.
Currently, when the raw ID is not in the mapping, the function attempts to
extract a "human-ish segment" by processing the raw string, which can still leak
internal identifiers. Replace the fallback logic so that whenever a mapping
entry doesn't exist for the given raw ID, always return a generic safe value
like "item" instead of processing and returning parts of the raw ID string. This
ensures complete suppression of raw keys in all cases.
In `@design-system/PRESENTATION_PATTERNS.md`:
- Line 83: The section headings at lines 83 and 94 violate Markdown linting
standard MD022 by lacking required blank lines above them. Add a blank line
before the section heading that starts with "## Streamlit design kit (most of
the fleet is Streamlit)" on line 83, and also add a blank line before the
section heading on line 94 to comply with Markdown formatting rules.
In `@design-system/README.md`:
- Line 50: The README.md file contains incorrect source-of-truth information in
the sync instructions around line 50. The text currently states "Update it here
first," but since this repository is a consumer of synced templates from
stranske/Workflows (the actual source-of-truth), this guidance is incorrect and
can cause local drift. Revise the instruction to clearly direct contributors to
make updates in the stranske/Workflows repository first, then rely on the sync
workflow to propagate those changes downstream to this consumer copy.
In `@tools/llm_provider.py`:
- Around line 47-53: The `is None` guard in the return statement of
`_configured_langchain_model` is unreachable dead code because
`configured_model_for_provider` is typed to return a string and never returns
`None` - it returns an empty string instead when no model is resolved. Replace
the conditional check `return fallback if configured is None else configured`
with `return configured or fallback` to correctly match the actual contract and
handle the empty string case using truthiness evaluation.
🪄 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: 925ad65e-0c48-4cdc-84b6-09368bfaaef4
📒 Files selected for processing (10)
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.ymldesign-system/PRESENTATION_PATTERNS.mddesign-system/README.mddesign-system/components.cssdesign-system/ds_streamlit.pydesign-system/tokens.cssscripts/langchain/progress_reviewer.pytools/langchain_client.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
🧰 Additional context used
📓 Path-based instructions (7)
{pyproject.toml,.github/workflows/*.{yml,yaml}}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
{pyproject.toml,.github/workflows/*.{yml,yaml}}: Ensure coverage thresholds inpyproject.toml([tool.coverage.report] fail_under) match thecoverage-minsettings in CI workflow files to avoid threshold conflicts
The Manager-Database repository has a coverage threshold of 75%
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When a GitHub Actions workflow fails with
startup_failure, check for invalid YAML syntax, top-levelpermissions:blocks onworkflow_callreusable workflows (which conflicts with caller permissions), invalid permission scopes, or circular workflow references
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
.github/workflows/**/*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
Reference reusable workflows with
@mainto match first-party consumer defaults, unless intentionally pinning to an exact commit SHA for a controlled reason.
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
.github/workflows/agents-*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
Synced workflows (
agents-*.yml,autofix.yml) should be fixed instranske/Workflows, not locally in the consumer repo.
Files:
.github/workflows/agents-guard.yml
.github/workflows/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
First-party consumers should reference reusable workflows with
@mainunless intentionally pinning to an exact commit SHA for a controlled reason.
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
{.github/workflows/agents-*.yml,.github/workflows/autofix.yml,.github/codex/**/*.{md,txt,prompt},**/*.sync.*}
📄 CodeRabbit inference engine (AGENTS.md)
Synced workflow files, agent prompts, scripts, and consumer docs should be fixed in
stranske/Workflowsthrough the sync manifest, not edited locally.
Files:
.github/workflows/agents-guard.yml
**/*.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.pydesign-system/ds_streamlit.pytools/langchain_client.pytools/llm_provider.py
🪛 LanguageTool
design-system/README.md
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...m/and distributed by Maint 68 through.github/sync-manifest.yml`. Update it here firs...
(GITHUB)
🪛 markdownlint-cli2 (0.22.1)
design-system/PRESENTATION_PATTERNS.md
[warning] 83-83: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 94-94: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔀 Multi-repo context stranske/Template
Now I have enough context to provide a comprehensive analysis. Let me compile the findings:
Linked repositories findings
[::stranske/Template::]
Critical consumers of tools.langchain_client module:
-
scripts/langchain/_llm_client.py(lines 67, 73) — Wrapsbuild_chat_clientandbuild_chat_clientswith fallback handling. This is the canonical abstraction layer for LLM client construction used by agent scripts.- Calls:
from tools.langchain_client import build_chat_client(line 67) - Calls:
from tools.langchain_client import build_chat_clients(line 73) - This module is the primary consumer that handles import failures gracefully
- Calls:
-
scripts/langchain/followup_issue_generator.py— Importsbuild_chat_clientdirectly fromtools.langchain_client -
tools/llm_provider.py(lines for OpenAIProvider and AnthropicProvider) — The PR changes these classes to usebuild_chat_client()instead of directChatOpenAI/ChatAnthropicimports- Critical change:
OpenAIProvider._get_client()andAnthropicProvider._get_client()now delegate totools.langchain_client.build_chat_client()rather than directly importing langchain classes - This creates a dependency inversion where provider implementations depend on
langchain_clientfor client instantiation
- Critical change:
Design-system module:
design-system/ds_streamlit.py(170 lines) — Already exists in Template. The PR syncs an updated version with new helper functions:inject_theme(),empty_state(),notice(),error(),translate_error(),dev_note(),diagnostics_expander(),availability_badge(),humanize_badge()- Currently not imported by any Python files in the Template repo (confirmed: no design-system imports found)
- This suggests the module is available for future consumption by Streamlit apps but not yet actively used
Workflow integration:
.github/actions/setup-api-client— Referenced in updated workflows (agents-guard.yml,maint-76-claude-code-review.yml)- The action installs pinned
@octokitdependencies and exports authentication tokens - Updated commit SHAs indicate the action implementation was revised in the Workflows source
- The action installs pinned
Import path consolidation:
The PR's change to scripts/langchain/progress_reviewer.py (attempting to import from scripts.langchain._llm_client instead of tools.langchain_client) is compatible because _llm_client.py already exists as the canonical wrapper with proper fallback handling.
Potential integration points that require verification:
- All scripts using
scripts.langchain._llm_client.get_llm_client()orbuild_client()will now work via the local wrapper, improving import resilience - The updated
tools/llm_provider.pyconsolidates client instantiation intolangchain_client.build_chat_client(), making provider initialization more uniform but dependent on successful client construction - Design system files are ready for adoption but currently dormant
🔇 Additional comments (8)
.github/workflows/agents-guard.yml (1)
114-114: LGTM!Also applies to: 183-183
.github/workflows/maint-76-claude-code-review.yml (1)
192-192: LGTM!design-system/PRESENTATION_PATTERNS.md (1)
1-105: Cross-file alignment verified: all pattern-to-code contracts are met byds_streamlit.py.The six patterns (P1–P6) and their Streamlit helpers are fully implemented in
design-system/ds_streamlit.py(lines 56–186):
- P1
inject_theme()✓- P2
empty_state()✓- P3
notice(),error(),translate_error()✓- P4
dev_note(),diagnostics_expander()✓- P5
availability_badge()✓- P6
humanize_id()✓The CSS class names referenced in web patterns (
.ds,.theme-air,.theme-paper,.notice--error,.badge,.empty-state) are provided by the companion CSS files (tokens.css / components.css, not in review scope). The finding-to-pattern map and rollout sequence are actionable and grounded in the 2026-06 UX-Review baseline.design-system/tokens.css (1)
1-77: LGTM!design-system/components.css (1)
1-99: LGTM!tools/langchain_client.py (1)
207-209: LGTM!Also applies to: 236-263, 297-336, 350-352, 389-449, 489-551
tools/llm_provider.py (1)
599-611: LGTM!Also applies to: 643-651, 672-686, 728-734, 985-986, 1013-1014
scripts/langchain/progress_reviewer.py (1)
430-434: LGTM!
| def notice(kind: str, title: str = "", body: str = "", action: str | None = None) -> None: | ||
| """P3/P4 — the one container for user-facing messages. kind in | ||
| {error,warn,info,ok}. `action` is optional remediation (markdown).""" | ||
| import streamlit as st | ||
|
|
||
| color, bg, ic = _NOTICE_STYLE.get(kind, _NOTICE_STYLE["info"]) | ||
| head = f"<strong>{escape(str(title))}</strong><br>" if title else "" | ||
| act = f"<div style='margin-top:6px'>{escape(str(action))}</div>" if action else "" | ||
| safe_body = escape(str(body)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
action markdown contract is currently broken.
The docstring says action accepts markdown, but Line 116 escapes it before rendering inside HTML, so markdown never formats.
🛠️ Proposed fix (align behavior to docs)
-def notice(kind: str, title: str = "", body: str = "", action: str | None = None) -> None:
- """P3/P4 — the one container for user-facing messages. kind in
- {error,warn,info,ok}. `action` is optional remediation (markdown)."""
+def notice(kind: str, title: str = "", body: str = "", action: str | None = None) -> None:
+ """P3/P4 — the one container for user-facing messages. kind in
+ {error,warn,info,ok}. `action` is optional remediation text."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def notice(kind: str, title: str = "", body: str = "", action: str | None = None) -> None: | |
| """P3/P4 — the one container for user-facing messages. kind in | |
| {error,warn,info,ok}. `action` is optional remediation (markdown).""" | |
| import streamlit as st | |
| color, bg, ic = _NOTICE_STYLE.get(kind, _NOTICE_STYLE["info"]) | |
| head = f"<strong>{escape(str(title))}</strong><br>" if title else "" | |
| act = f"<div style='margin-top:6px'>{escape(str(action))}</div>" if action else "" | |
| safe_body = escape(str(body)) | |
| def notice(kind: str, title: str = "", body: str = "", action: str | None = None) -> None: | |
| """P3/P4 — the one container for user-facing messages. kind in | |
| {error,warn,info,ok}. `action` is optional remediation text.""" | |
| import streamlit as st | |
| color, bg, ic = _NOTICE_STYLE.get(kind, _NOTICE_STYLE["info"]) | |
| head = f"<strong>{escape(str(title))}</strong><br>" if title else "" | |
| act = f"<div style='margin-top:6px'>{escape(str(action))}</div>" if action else "" | |
| safe_body = escape(str(body)) |
🤖 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 `@design-system/ds_streamlit.py` around lines 109 - 117, The notice function's
docstring documents that the action parameter accepts markdown, but the
implementation on line 116 escapes the action string before rendering it, which
prevents markdown from being formatted. To align the behavior with the
documentation, remove the escape() call from the act variable assignment that
processes the action parameter, allowing markdown formatting to work as
documented while still ensuring safe HTML rendering for the action content.
| def humanize_id(raw: str, mapping: Mapping[str, str] | None = None) -> str: | ||
| """P6 — decode an internal id to a human label; never show raw keys.""" | ||
| if mapping and raw in mapping: | ||
| return mapping[raw] | ||
| # Best-effort: take a trailing human-ish segment, strip hashes. | ||
| tail = str(raw).replace("_", " ").split(":")[-1].strip() | ||
| return tail or "item" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
humanize_id() can still leak raw internal IDs.
The fallback path may return near-raw identifiers, which conflicts with the stated “never show raw keys” contract.
🛠️ Proposed safer fallback
from html import escape
+import re
...
def humanize_id(raw: str, mapping: Mapping[str, str] | None = None) -> str:
"""P6 — decode an internal id to a human label; never show raw keys."""
if mapping and raw in mapping:
return mapping[raw]
- # Best-effort: take a trailing human-ish segment, strip hashes.
- tail = str(raw).replace("_", " ").split(":")[-1].strip()
- return tail or "item"
+ # Best-effort: trailing segment, normalize separators, and hide hash-like ids.
+ tail = str(raw).replace("_", " ").split(":")[-1].strip()
+ if re.fullmatch(r"[0-9a-fA-F-]{8,}", tail):
+ return "item"
+ return tail or "item"🤖 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 `@design-system/ds_streamlit.py` around lines 180 - 186, The humanize_id
function's fallback path still exposes raw internal ID information despite the
"never show raw keys" contract. Currently, when the raw ID is not in the
mapping, the function attempts to extract a "human-ish segment" by processing
the raw string, which can still leak internal identifiers. Replace the fallback
logic so that whenever a mapping entry doesn't exist for the given raw ID,
always return a generic safe value like "item" instead of processing and
returning parts of the raw ID string. This ensures complete suppression of raw
keys in all cases.
| | LMS #351 root JSON 404 / empty surfaces | P2 (+ deployment) | | ||
| | Default dark theme (TMP/PA/MD/IMI) | P1 | | ||
|
|
||
| ## Streamlit design kit (most of the fleet is Streamlit) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines above section headings to comply with Markdown formatting rules.
Lines 83 and 94 have section headings that lack the required blank line above them per Markdown linting standard MD022.
📝 Proposed fix for Markdown formatting
| Default dark theme (TMP/PA/MD/IMI) | P1 |
+
## Streamlit design kit (most of the fleet is Streamlit) - `humanize_id(raw, mapping)` (P6)
Graduate this kit + the CSS into `Workflows/templates/consumer-repo/design-system/` so maint-68 syncs it fleet-wide.
+
## Rollout sequenceAlso applies to: 94-94
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 83-83: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@design-system/PRESENTATION_PATTERNS.md` at line 83, The section headings at
lines 83 and 94 violate Markdown linting standard MD022 by lacking required
blank lines above them. Add a blank line before the section heading that starts
with "## Streamlit design kit (most of the fleet is Streamlit)" on line 83, and
also add a blank line before the section heading on line 94 to comply with
Markdown formatting rules.
Source: Linters/SAST tools
|
|
||
| ## Status & next step | ||
|
|
||
| This kit is managed from `Workflows/templates/consumer-repo/design-system/` and distributed by Maint 68 through `.github/sync-manifest.yml`. Update it here first, then let the sync workflow replace consumer copies. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix source-of-truth direction in sync instructions.
Line 50 says “Update it here first,” but this repo is a consumer of synced templates. That guidance can cause local drift and overwritten edits; the instruction should point contributors to update stranske/Workflows first, then sync downstream.
✍️ Proposed doc fix
-This kit is managed from `Workflows/templates/consumer-repo/design-system/` and distributed by Maint 68 through `.github/sync-manifest.yml`. Update it here first, then let the sync workflow replace consumer copies.
+This kit is managed from `stranske/Workflows/templates/consumer-repo/design-system/` and distributed by Maint 68 through `.github/sync-manifest.yml`. Update it in `stranske/Workflows` first, then let the sync workflow propagate changes to consumer copies.🧰 Tools
🪛 LanguageTool
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...m/and distributed by Maint 68 through.github/sync-manifest.yml`. Update it here firs...
(GITHUB)
🤖 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 `@design-system/README.md` at line 50, The README.md file contains incorrect
source-of-truth information in the sync instructions around line 50. The text
currently states "Update it here first," but since this repository is a consumer
of synced templates from stranske/Workflows (the actual source-of-truth), this
guidance is incorrect and can cause local drift. Revise the instruction to
clearly direct contributors to make updates in the stranske/Workflows repository
first, then rely on the sync workflow to propagate those changes downstream to
this consumer copy.
| def _configured_langchain_model(provider: str, *, fallback: str) -> str: | ||
| try: | ||
| from tools.llm_registry import configured_model_for_provider | ||
| except ImportError: | ||
| return fallback | ||
| configured = configured_model_for_provider(provider, fallback=fallback) | ||
| return fallback if configured is None else configured |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the llm_registry module
find . -name "*llm_registry*" -type fRepository: stranske/Collab-Admin
Length of output: 183
🏁 Script executed:
#!/bin/bash
# Examine the _configured_langchain_model function and its context
cat -n tools/llm_provider.py | head -80Repository: stranske/Collab-Admin
Length of output: 3522
🏁 Script executed:
#!/bin/bash
# Look for the configured_model_for_provider function definition
if [ -f "tools/llm_registry.py" ]; then
cat -n tools/llm_registry.py
else
rg "configured_model_for_provider" -B 2 -A 10
fiRepository: stranske/Collab-Admin
Length of output: 12817
🏁 Script executed:
#!/bin/bash
# Examine how _configured_langchain_model is used in _get_client
rg "_get_client|_configured_langchain_model" tools/llm_provider.py -A 5 -B 2Repository: stranske/Collab-Admin
Length of output: 3505
is None guard is unreachable — function never returns None, only empty string when fallback is blocked.
configured_model_for_provider is typed -> str and returns an empty string "" (line 205 of llm_registry.py) when no model can be resolved, never None. The is None check at line 53 is therefore dead code. The empty string does get caught by the downstream if not model_name: check in _get_client, so behavior is correct, but the guard signals a contract mismatch. Use return configured or fallback instead to match the actual contract:
configured = configured_model_for_provider(provider, fallback=fallback)
-return fallback if configured is None else configured
+return configured or fallback📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _configured_langchain_model(provider: str, *, fallback: str) -> str: | |
| try: | |
| from tools.llm_registry import configured_model_for_provider | |
| except ImportError: | |
| return fallback | |
| configured = configured_model_for_provider(provider, fallback=fallback) | |
| return fallback if configured is None else configured | |
| def _configured_langchain_model(provider: str, *, fallback: str) -> str: | |
| try: | |
| from tools.llm_registry import configured_model_for_provider | |
| except ImportError: | |
| return fallback | |
| configured = configured_model_for_provider(provider, fallback=fallback) | |
| return configured or fallback |
🤖 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 47 - 53, The `is None` guard in the
return statement of `_configured_langchain_model` is unreachable dead code
because `configured_model_for_provider` is typed to return a string and never
returns `None` - it returns an empty string instead when no model is resolved.
Replace the conditional check `return fallback if configured is None else
configured` with `return configured or fallback` to correctly match the actual
contract and handle the empty string case using truthiness evaluation.
|
Closing as superseded by newer sync wave in PR #880 after the Workflows source fix merged. |
1 participant
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Source SHA:
dfe0854ae9b1ba1c616e4b57fb498f283ea3216fTemplate hash:
d0d4ad2bdd25Sync branch:
sync/workflows-d0d4ad2bdd25Consumer repo:
stranske/Collab-AdminManifest:
.github/sync-manifest.ymlSummary by CodeRabbit
New Features
Chores