phase 0: scaffold + layer guard + ci wiring - #326
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a layered architecture enforcement system for the refactoring phase. It adds a GitHub Actions workflow and Python validation script to check import dependencies across architectural layers, updates CI workflows to trigger on refactor branches, modifies Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
68c8ea4 to
8ee3264
Compare
dc9259f to
c2df02c
Compare
8ee3264 to
aec0af8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 62-64: Remove the directory-level ignore pattern "/services/" so
the negation "!/services/.gitkeep" can take effect: keep only the "/services/*"
ignore entry and the "!/services/.gitkeep" negation, ensuring Git will ignore
all files under services/ but still allow the .gitkeep to be tracked; delete the
"/services/" line from .gitignore.
In `@REFACTORING_DECISION_LOG.md`:
- Around line 71-77: The fenced code block in the REFACTORING_DECISION_LOG.md
template should include a language identifier to satisfy markdownlint MD040;
update the triple-backtick fence around the template block (the block starting
with "## Phase N — [short title]") to use "text" (i.e., ```text) so the non-code
template is explicitly marked as plain text.
In `@scripts/check_layer_imports.py`:
- Around line 73-84: The resolve_relative function mishandles relative imports
from __init__.py by popping "__init__" and then using parts[:-level], which
double-clips the package path; update resolve_relative to treat package anchors
correctly: compute package_parts = parts if last part != "__init__" else
parts[:-1], then compute keep = len(package_parts) - (level - 1) (clamped to
>=0) and set anchor = package_parts[:keep] (or [] if climbing beyond root);
finally, if module is present extend anchor with module.split(".") and return
".".join(anchor). Use the existing symbols resolve_relative, REPO_ROOT,
file_path, level, and module to locate and modify the logic.
- Around line 41-46: The layer detection in layer_of() only recognizes modules
starting with "openrag." and misses top-level imports like "services.foo";
update layer_of(module: str) to also check the case where parts[0] itself is in
LAYERS (i.e., treat "services.foo" as layer "services") in addition to the
existing "openrag" prefix check, return the detected layer string in either
case, and keep returning None when no layer matches; refer to the layer_of
function and the LAYERS set to implement this change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb49c489-a415-4cfb-af58-f33302d87d6b
📒 Files selected for processing (46)
.github/workflows/layer_guard.yml.github/workflows/lint.yml.github/workflows/unit_tests.yml.gitignoreREFACTORING_DECISION_LOG.mdopenrag/api/__init__.pyopenrag/api/dependencies/__init__.pyopenrag/api/middleware/__init__.pyopenrag/api/routers/__init__.pyopenrag/api/routers/admin/__init__.pyopenrag/api/routers/auth/__init__.pyopenrag/api/routers/user/__init__.pyopenrag/api/schemas/__init__.pyopenrag/api/schemas/admin/__init__.pyopenrag/api/schemas/auth/__init__.pyopenrag/api/schemas/user/__init__.pyopenrag/core/__init__.pyopenrag/core/catalog/__init__.pyopenrag/core/chunking/__init__.pyopenrag/core/config/__init__.pyopenrag/core/embeddings/__init__.pyopenrag/core/indexing/__init__.pyopenrag/core/indexing/parsers/__init__.pyopenrag/core/llm/__init__.pyopenrag/core/models/__init__.pyopenrag/core/observability/__init__.pyopenrag/core/ports/__init__.pyopenrag/core/prompts/__init__.pyopenrag/core/rerankers/__init__.pyopenrag/core/retrieval/__init__.pyopenrag/core/utils/__init__.pyopenrag/core/vector_stores/__init__.pyopenrag/core/vlm/__init__.pyopenrag/di/__init__.pyopenrag/services/__init__.pyopenrag/services/auth/__init__.pyopenrag/services/events/__init__.pyopenrag/services/inference/__init__.pyopenrag/services/orchestrators/__init__.pyopenrag/services/persistence/__init__.pyopenrag/services/persistence/migrations/__init__.pyopenrag/services/persistence/migrations/versions/__init__.pyopenrag/services/storage/__init__.pyopenrag/services/workers/__init__.pyopenrag/services/workers/stages/__init__.pyscripts/check_layer_imports.py
| /services/ | ||
| /services/* | ||
| !/services/.gitkeep # Keep the placeholder |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect current output to report `/services/` as the deciding ignore rule.
# After the fix, `/services/.gitkeep` should no longer be ignored.
git check-ignore --no-index -v services/.gitkeep || true
# This should still be ignored by `/services/*`.
git check-ignore --no-index -v services/scratch.txt || trueRepository: linagora/openrag
Length of output: 1781
🌐 Web query:
git ignore directory pattern negation .gitkeep ineffective
💡 Result:
The issue with gitignore directory pattern negation for .gitkeep being ineffective stems from Git's core behavior: Git does not track empty directories and skips ignored directories for performance. You cannot re-include a file (like .gitkeep) if its parent directory is excluded by a prior pattern, as Git won't traverse into ignored directories. Common incorrect pattern: build/ !build/.gitkeep This fails because build/ excludes the entire directory, so Git never checks build/.gitkeep. Correct patterns in root .gitignore (order matters; ignore first, then negate): 1. For simple case (no subdirs): build/* !build/.gitkeep This ignores all files in build/ but allows .gitkeep to be tracked, preserving the directory. 2. If preserving subdirectories too: build/* !build// !build/.gitkeep Or more precisely: build/* !build/.gitkeep !build// Alternative (recommended by some sources): Place a .gitignore inside the directory itself: build/.gitignore: * !.gitignore Then git add build/.gitignore. This tracks the directory via the .gitignore file and ignores everything else inside. After changes, run git rm -r --cached build/ (if previously cached as ignored), then git add build/.gitkeep (or equivalent), and commit. .gitkeep is a convention, not official Git; any placeholder file works, but .gitkeep is standard for intent.
Citations:
- 1: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitignore.html
- 2: https://git-scm.com/docs/gitignore.html
- 3: https://adamj.eu/tech/2023/09/18/git-dont-create-gitkeep/
- 4: https://stackoverflow.com/questions/22082534/git-ignore-to-keep-directory-structure-but-ignore-files
- 5: https://stackoverflow.com/questions/18930103/git-ignore-everything-in-a-directory-except-subfolders
- 6: https://stackoverflow.com/questions/24296724/gitignore-negation-not-working
- 7: https://stackoverflow.com/questions/35278885/gitignore-exclusion-is-not-working-for-a-single-file
- 8: https://stackoverflow.com/questions/26515346/git-ignore-unignore-not-working
- 9: https://thelinuxcode.com/difference-between-gitignore-and-gitkeep-and-how-i-use-both-in-real-repos/
- 10: https://stackoverflow.com/questions/7229885/what-are-the-differences-between-gitignore-and-gitkeep
Remove the directory-level ignore pattern so .gitkeep can be re-included.
Line 62's /services/ pattern prevents Git from traversing into the directory, making the !/services/.gitkeep negation on line 64 ineffective. Use /services/* alone to ignore contents while allowing file-level negations to work.
Proposed fix
-/services/
/services/*
!/services/.gitkeep # Keep the placeholder📝 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.
| /services/ | |
| /services/* | |
| !/services/.gitkeep # Keep the placeholder | |
| /services/* | |
| !/services/.gitkeep # Keep the placeholder |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 62 - 64, Remove the directory-level ignore pattern
"/services/" so the negation "!/services/.gitkeep" can take effect: keep only
the "/services/*" ignore entry and the "!/services/.gitkeep" negation, ensuring
Git will ignore all files under services/ but still allow the .gitkeep to be
tracked; delete the "/services/" line from .gitignore.
| ``` | ||
| ## Phase N — [short title] ([YYYY-MM-DD]) | ||
|
|
||
| **K. [decision in one line].** | ||
| - Why: [what forced the call, what the docs didn't cover]. | ||
| - Alternative considered: [what else was on the table, why it was rejected]. | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced template block.
This trips markdownlint MD040; use text for the non-code template.
Proposed fix
-```
+```text
## Phase N — [short title] ([YYYY-MM-DD])
**K. [decision in one line].**
- Why: [what forced the call, what the docs didn't cover].
- Alternative considered: [what else was on the table, why it was rejected].</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@REFACTORING_DECISION_LOG.md` around lines 71 - 77, The fenced code block in
the REFACTORING_DECISION_LOG.md template should include a language identifier to
satisfy markdownlint MD040; update the triple-backtick fence around the template
block (the block starting with "## Phase N — [short title]") to use "text"
(i.e., ```text) so the non-code template is explicitly marked as plain text.
| def layer_of(module: str) -> str | None: | ||
| """Return the layer name if `module` is openrag.<layer>[...], else None.""" | ||
| parts = module.split(".") | ||
| if len(parts) >= 2 and parts[0] == "openrag" and parts[1] in LAYERS: | ||
| return parts[1] | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full check_layer_imports.py file to understand LAYERS and the function's context
cat -n scripts/check_layer_imports.pyRepository: linagora/openrag
Length of output: 5311
🏁 Script executed:
# Look for what module formats are actually passed to layer_of() - check callers
rg -A 5 "layer_of\(" scripts/check_layer_imports.pyRepository: linagora/openrag
Length of output: 610
🏁 Script executed:
# Search for actual imports in the codebase to see the format used
rg "^from (services|api|components|core|di)\." openrag/ -m 5Repository: linagora/openrag
Length of output: 6640
🏁 Script executed:
# Search for imports from the four layers (core, services, api, di) in files under those directories
rg "^from (core|services|api|di)\." openrag/Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check if any files exist in these layer directories
find openrag -maxdepth 2 -type d -name "core" -o -name "services" -o -name "api" -o -name "di"Repository: linagora/openrag
Length of output: 112
🏁 Script executed:
# Also check if there are any imports starting with openrag. in the layer directories
rg "^from openrag\." openrag/core openrag/services openrag/api openrag/di 2>/dev/null || echo "Likely no such files yet"Repository: linagora/openrag
Length of output: 84
🏁 Script executed:
# Check what files actually exist in these refactoring layer directories
find openrag/core openrag/services openrag/api openrag/di -type f -name "*.py" 2>/dev/nullRepository: linagora/openrag
Length of output: 1488
🏁 Script executed:
# Let's also check the documentation or comments about the refactoring to understand the intent
cat openrag/api/.gitkeep openrag/core/.gitkeep openrag/services/.gitkeep openrag/di/.gitkeep 2>/dev/null || echo "No .gitkeep files"Repository: linagora/openrag
Length of output: 77
🏁 Script executed:
# Check if there's any migration guidance in the main CLAUDE.md or README
grep -i "refactor\|layer\|migration" CLAUDE.md README.md 2>/dev/null | head -20Repository: linagora/openrag
Length of output: 1063
🏁 Script executed:
# Let's look at actual imports in some of the new refactoring layer files to see what format they use
head -50 openrag/core/__init__.pyRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check a service file
head -50 openrag/services/__init__.pyRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check an API file
head -50 openrag/api/__init__.pyRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Look at actual imports in a deeper file
rg "^from\s+(core|services|api|di|openrag)\." openrag/core/ openrag/services/ openrag/api/ openrag/di/ -m 10Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check file sizes to see if they're empty or have content
wc -l openrag/core/__init__.py openrag/services/__init__.py openrag/api/__init__.py openrag/di/__init__.pyRepository: linagora/openrag
Length of output: 176
🏁 Script executed:
# Check a non-empty file deeper in the hierarchy
wc -l openrag/core/models/__init__.py openrag/services/auth/__init__.py openrag/api/routers/__init__.pyRepository: linagora/openrag
Length of output: 171
🏁 Script executed:
# Get actual content from a few of these files
cat openrag/core/models/__init__.pyRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check a file that likely has imports
cat openrag/services/auth/__init__.pyRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check api routers
head -30 openrag/api/routers/__init__.pyRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Find non-empty .py files in the refactoring layer directories
find openrag/core openrag/services openrag/api openrag/di -type f -name "*.py" -size +0 2>/dev/nullRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check if there's any content at all in these directories
find openrag/core openrag/services openrag/api openrag/di -type f ! -name "__init__.py" 2>/dev/null | head -20Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# List all files in the refactoring layer directories
find openrag/core openrag/services openrag/api openrag/di -type f 2>/dev/nullRepository: linagora/openrag
Length of output: 1488
🏁 Script executed:
# Let's verify the intent by checking if there are test files that might import from these layers
find openrag -name "test_*.py" -o -name "*_test.py" | xargs grep -l "from (core|services|api|di)\." 2>/dev/null | head -5Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check the git history or any TODO comments about the refactoring
rg "TODO|FIXME|refactor" scripts/check_layer_imports.pyRepository: linagora/openrag
Length of output: 112
🏁 Script executed:
# Understand the context better: when the check script runs, what would be the format of modules?
# Let's trace through an example: if openrag/api/routers/foo.py has "from services.bar import X"
# What would iter_imports yield and what would be passed to layer_of()?
python3 << 'EOF'
import ast
# Simulate what happens with a top-level import
code = "from services.bar import X"
tree = ast.parse(code)
for node in ast.walk(tree):
if isinstance(node, ast.ImportFrom):
print(f"ImportFrom found: module={node.module}, level={node.level}")
# When level=0 (absolute), node.module is passed to layer_of()
print(f"Would pass to layer_of(): {node.module}")
EOFRepository: linagora/openrag
Length of output: 145
🏁 Script executed:
# Also check what the current layer_of would return for this
python3 << 'EOF'
def layer_of(module: str) -> str | None:
"""Current implementation"""
LAYERS = ("core", "services", "api", "di")
parts = module.split(".")
if len(parts) >= 2 and parts[0] == "openrag" and parts[1] in LAYERS:
return parts[1]
return None
# Test cases
test_cases = [
"services.bar", # Top-level layer import (from learning)
"openrag.services.bar", # openrag-prefixed import
"components.foo", # Legacy import
"core.models.User", # Top-level core import
]
for test in test_cases:
result = layer_of(test)
print(f"layer_of('{test}') = {result}")
EOFRepository: linagora/openrag
Length of output: 205
Detect top-level layer imports alongside openrag.* format.
The layer_of() function currently only recognizes modules prefixed with openrag., but per the project's import convention, migrated code in these refactoring layers will use top-level imports like from services.foo import Bar (without the openrag. prefix). This causes forbidden dependency edges—such as api -> services—to pass undetected.
Proposed fix
def layer_of(module: str) -> str | None:
- """Return the layer name if `module` is openrag.<layer>[...], else None."""
+ """Return the layer name if `module` targets one of the refactor layers."""
parts = module.split(".")
+ if parts and parts[0] in LAYERS:
+ return parts[0]
if len(parts) >= 2 and parts[0] == "openrag" and parts[1] in LAYERS:
return parts[1]
return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check_layer_imports.py` around lines 41 - 46, The layer detection in
layer_of() only recognizes modules starting with "openrag." and misses top-level
imports like "services.foo"; update layer_of(module: str) to also check the case
where parts[0] itself is in LAYERS (i.e., treat "services.foo" as layer
"services") in addition to the existing "openrag" prefix check, return the
detected layer string in either case, and keep returning None when no layer
matches; refer to the layer_of function and the LAYERS set to implement this
change.
| def resolve_relative(file_path: Path, level: int, module: str) -> str: | ||
| """Turn `from ..foo import bar` into an absolute dotted module.""" | ||
| rel = file_path.relative_to(REPO_ROOT).with_suffix("") | ||
| parts = list(rel.parts) | ||
| # __init__.py sits one level shallower | ||
| if parts[-1] == "__init__": | ||
| parts.pop() | ||
| # `level` dots = climb that many packages | ||
| anchor = parts[:-level] if level <= len(parts) else [] | ||
| if module: | ||
| anchor.extend(module.split(".")) | ||
| return ".".join(anchor) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/check_layer_imports.py | head -100Repository: linagora/openrag
Length of output: 4100
🏁 Script executed:
cat -n scripts/check_layer_imports.py | sed -n '100,120p'Repository: linagora/openrag
Length of output: 909
Fix relative import resolution for __init__.py files.
The function incorrectly resolves relative imports from __init__.py files. For example, from .services import X in openrag/core/__init__.py currently resolves to openrag.services instead of the correct openrag.core.services. This happens because the code removes __init__ from parts, then uses parts[:-level] to climb packages—but this double-clips the package path.
The fix correctly distinguishes between the module file path and the package structure. By computing keep = len(package_parts) - (level - 1), it properly treats level=1 as "stay in the current package" rather than "climb one level."
Proposed fix
def resolve_relative(file_path: Path, level: int, module: str) -> str:
"""Turn `from ..foo import bar` into an absolute dotted module."""
rel = file_path.relative_to(REPO_ROOT).with_suffix("")
- parts = list(rel.parts)
- # __init__.py sits one level shallower
- if parts[-1] == "__init__":
- parts.pop()
- # `level` dots = climb that many packages
- anchor = parts[:-level] if level <= len(parts) else []
+ module_parts = list(rel.parts)
+ package_parts = module_parts[:-1]
+ # level=1 means current package; each additional dot climbs one package.
+ keep = len(package_parts) - (level - 1)
+ anchor = package_parts[:keep] if keep > 0 else []
if module:
anchor.extend(module.split("."))
return ".".join(anchor)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check_layer_imports.py` around lines 73 - 84, The resolve_relative
function mishandles relative imports from __init__.py by popping "__init__" and
then using parts[:-level], which double-clips the package path; update
resolve_relative to treat package anchors correctly: compute package_parts =
parts if last part != "__init__" else parts[:-1], then compute keep =
len(package_parts) - (level - 1) (clamped to >=0) and set anchor =
package_parts[:keep] (or [] if climbing beyond root); finally, if module is
present extend anchor with module.split(".") and return ".".join(anchor). Use
the existing symbols resolve_relative, REPO_ROOT, file_path, level, and module
to locate and modify the logic.
2 participants
Phase 0 of the hexagonal refactoring. Purely additive — no existing file touched.
What lands:
core/,services/,api/,di/with__init__.pyin every dir, plusprompts/templates/.scripts/check_layer_imports.py— AST-based guard enforcing the layer dependency rule (core→ nothing,services→core,api→di+core,di→ everything). Ignores legacy paths (components/,routers/,models/, etc.) until they're migrated..github/workflows/layer_guard.yml— runs the guard on pushes/PRs targetingrefactor/hexagonaland anyrefactor/phase-**branch.lint.ymlandunit_tests.ymlto also run on the refactor branches so the cherry-picked skill commit and future phase work get CI coverage.services/gitignore rule to repo root so it no longer matchesopenrag/services/.Verification run locally:
python3 scripts/check_layer_imports.py→ OKuv run ruff check openrag/ tests/→ cleanuv run ruff format --check openrag/ tests/→ cleanpython3 -c "import openrag"→ OKNo runtime behavior change. Merging this unlocks Phase 1 (Registry + exception hierarchy).
Summary by CodeRabbit