-
Notifications
You must be signed in to change notification settings - Fork 56
phase 0: scaffold + layer guard + ci wiring #326
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
41ae013
scaffold hexagonal layer dirs
EnjoyBacon7 3bb378d
add layer import guard
EnjoyBacon7 4eed409
ci: run layer guard, lint, and unit tests on refactor branches
EnjoyBacon7 aec0af8
add refactoring decision log
EnjoyBacon7 9774a97
docs: add strategy + workflow to docs/refactoring/, fix decision log …
andyne13 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| name: Layer guard | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - "refactor/hexagonal" | ||
| - "refactor/phase-**" | ||
| pull_request: | ||
| branches: | ||
| - "refactor/hexagonal" | ||
|
|
||
| jobs: | ||
| layer-import-guard: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
| - name: Run layer guard | ||
| run: python scripts/check_layer_imports.py |
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
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,66 @@ | ||
| # Refactoring Decision Log | ||
|
|
||
| Records **why** decisions were made that deviate from or extend the refactoring | ||
| docs. When a decision changes the plan, update the strategy/workflow docs to | ||
| reflect the new reality — then log the reasoning here so future readers know | ||
| why the docs changed. | ||
|
|
||
| Source abbreviations: | ||
| - STRATEGY = `docs/refactoring/REFACTORING_STRATEGY_v1.md` | ||
| - WORKFLOW = `docs/refactoring/REFACTORING_DEV_WORKFLOW.md` | ||
|
|
||
| --- | ||
|
|
||
| ## Phase 0 — Scaffold + import guard + CI wiring (2026-04-21) | ||
|
|
||
| **1. The guard ignores files outside the four new layer roots.** | ||
| Files under `openrag/components/`, `openrag/routers/`, `openrag/models/`, | ||
| `openrag/config/`, `openrag/utils/` are skipped. | ||
| - Why: Phase 0's verification requires existing tests to keep passing. If the | ||
| guard ran against legacy code, every old import that doesn't fit the new | ||
| rules would trip the check and block the phase. Legacy code gets migrated in | ||
| Phases 5–12 and the guard picks those files up as they move into the new | ||
| layer roots. | ||
| - Alternative considered: whitelist-only enforcement on new code (same idea, | ||
| different framing). What we chose is "enforce wherever the file lives in one | ||
| of the four roots", which is simpler. | ||
|
|
||
| **2. Split CI into `layer_guard.yml` + extending existing `lint.yml` and | ||
| `unit_tests.yml`, instead of one new `refactor-ci.yml`.** | ||
| WORKFLOW's CI example is a single file with three jobs (`unit-tests`, | ||
| `layer-guard`, `docker-build`). We took a different shape. | ||
| - Why: We already have a well-set-up `unit_tests.yml` and `lint.yml`. Creating | ||
| a parallel `refactor-ci.yml` with its own unit-tests job would duplicate the | ||
| uv setup and caching. Extending the existing files adds a few lines of | ||
| config and reuses everything. | ||
| - Alternative considered: follow the WORKFLOW example literally. Rejected for | ||
| the duplication reason above. Trade-off is that refactor-specific CI isn't | ||
| all in one file. | ||
|
|
||
| **3. `docker-build` CI check NOT wired in Phase 0.** | ||
| WORKFLOW lists it as a required check. | ||
| - Why: Existing `build.yml` and `build_dev.yml` workflows push images to ghcr, | ||
| which isn't what we want on every refactor push. A lightweight "docker build | ||
| only, don't push" check needs a new job. Deferred to keep Phase 0 scope | ||
| tight. Docker build was verified locally on the phase-0 tree. | ||
| - Alternative considered: add the job in this phase. Rejected for scope. | ||
| Follow-up: add a `docker-build` job in a separate PR, modelled on the | ||
| WORKFLOW CI example. | ||
|
|
||
| **4. Decision log policy: log reasoning, update docs.** | ||
| When a decision deviates from the strategy/workflow docs, update the docs to | ||
| match reality, then record the reasoning here. | ||
| - Why: The docs should always reflect the current plan. The log captures | ||
| why the plan changed, not what the plan is. | ||
|
|
||
| --- | ||
|
|
||
| ## Template for future entries | ||
|
|
||
| ``` | ||
| ## 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]. | ||
| ``` | ||
|
Comment on lines
+60
to
+66
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. Add a language to the fenced template block. This trips markdownlint MD040; use 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].🧰 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 |
||
Oops, something went wrong.
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.
🧩 Analysis chain
🏁 Script executed:
Repository: 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:
Remove the directory-level ignore pattern so
.gitkeepcan be re-included.Line 62's
/services/pattern prevents Git from traversing into the directory, making the!/services/.gitkeepnegation 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
🤖 Prompt for AI Agents