Skip to content

Commit 482ae9e

Browse files
HumanBean17claude
andcommitted
address code review: risks, decisions, first-match-wins, resolution-base
- Add "Risks and mitigations" section with symlink, performance, boundary - Add "Decisions taken" section documenting key design rationale - Clarify first-match-wins for nested configs in walk-up algorithm - Clarify $HOME boundary is inclusive (check it, don't go past) - Document resolution-base difference between YAML and CLI source_root - Soften AGENTS.md section list to match actual completed proposals Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 9e0a4df commit 482ae9e

2 files changed

Lines changed: 29 additions & 6 deletions

File tree

AGENTS.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,10 +156,11 @@ loosening). Use judgement.
156156

157157
When brainstorming produces a spec/design, write it as a proposal in
158158
`propose/active/<TOPIC>-PROPOSE.md` using the format established in
159-
`propose/completed/` (see any file there for the section structure:
160-
Status, Problem Statement, Proposed Solution, Scope, Schema / Ontology /
161-
Re-index impact, Tests / Validation, Open Questions, Out of scope,
162-
Sequencing / Migration). Do NOT use a brainstorming skill's default
159+
`propose/completed/` (open a completed proposal there and match its
160+
section structure, headings, and level of detail — specific headings vary
161+
by proposal but always include: TL;DR, design principles, proposed
162+
surface/solution, risks and mitigations, decisions taken, tests, out of
163+
scope, migration plan). Do NOT use a brainstorming skill's default
163164
`docs/superpowers/specs/` location.
164165

165166
## Per-PR agent task contract

propose/active/DIRS-HIERARCHY-PROPOSE.md

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ New function `discover_project_root(start: Path) -> Path | None` in `config.py`:
6464
- Starts from `start` (typically cwd)
6565
- Checks for `.java-codebase-rag.yml` or `.java-codebase-rag.yaml` in the current directory
6666
- If not found, moves to parent and repeats
67-
- **Boundary conditions**: stops before reaching `$HOME` (does not check `$HOME` itself), stops at filesystem root
67+
- **First match wins** (closest to cwd): if nested configs exist at multiple levels (e.g. `System-A/.java-codebase-rag.yml` and `IdeaProjects/.java-codebase-rag.yml`), the one closest to cwd is used. This mirrors git's behavior when nested `.git` directories exist.
68+
- **Boundary conditions**: stops at `$HOME` (inclusive — checks `$HOME` itself but does not go past it), stops at filesystem root. Rationale: `$HOME` is the natural project root on macOS/Linux workstations. On CI/CD (`/root`, `/home/runner`) this is equally appropriate. Configs above `$HOME` are almost certainly unrelated to the current project.
6869
- Returns the directory containing the config file, or `None`
6970

7071
The function is a pure discovery step — it finds where the config lives, nothing more. It does not parse the config or resolve source roots.
@@ -82,6 +83,8 @@ source_root: ../
8283
8384
Resolution is straightforward: `Path(config_dir) / source_root`. For the example above, if config is at `system-D-context/.java-codebase-rag.yml`, then `source_root: ../` resolves to `System-D/`.
8485

86+
**Note on resolution base**: the YAML `source_root` field resolves relative to the config file's directory, while the CLI `--source-root` flag resolves relative to cwd. These are intentionally different resolution bases — the YAML field is a portable declaration ("my code is one level up from this config"), while the CLI flag is an absolute or cwd-relative override. The precedence table in §3.3 handles priority; the resolution base difference is a non-issue because each source resolves independently before comparison.
87+
8588
### 3.3 Full precedence chain for source root
8689

8790
| Priority | Source | Example |
@@ -164,7 +167,26 @@ None — all key decisions resolved during brainstorming.
164167
- Changes to indexing, query, or graph-building logic
165168
- `init` command behavior changes (beyond the parent-config warning)
166169

167-
## 9. Migration plan — 1 PR
170+
## 9. Risks and mitigations
171+
172+
| Risk | Mitigation |
173+
|---|---|
174+
| Walk-up finds wrong config in a shared parent (e.g. `IdeaProjects/.java-codebase-rag.yml` when user meant `System-A/`) | `init` warns when a parent config exists. First-match-wins means the closest config is always preferred. If a stray config exists at a high level, it's only found when no closer config exists. |
175+
| Symlink cycles during walk-up | `Path.resolve()` canonicalizes the path before walking. The `parent` chain on resolved paths cannot cycle. |
176+
| Performance of filesystem stat calls in deep directory trees | Each step is a single `is_file()` check. Even at 20 levels deep, this is negligible compared to the embedding/indexing work the tool already does. |
177+
| `$HOME` boundary stops too early or too late | `$HOME` is checked inclusively (a config at `$HOME` itself is found). This covers the common macOS case where projects live under `~/Projects/`. Going past `$HOME` would risk picking up system-level or unrelated configs. |
178+
| Nested configs create confusion (which one is active?) | First-match-wins is simple and matches git's behavior. The tool can log which config file it discovered to aid debugging. |
179+
180+
## 10. Decisions taken
181+
182+
1. **First match wins** — closest config to cwd, not "most specific" or "deepest". Matches git behavior. No heuristic for picking among multiple configs.
183+
2. **`$HOME` is inclusive boundary** — check `$HOME` itself, don't go past it. Avoids finding configs in `/` or system directories.
184+
3. **YAML field named `source_root`** — same name as the CLI flag for conceptual consistency, despite different resolution bases. The alternative (`project_root`, `code_dir`) would add a new concept where none is needed.
185+
4. **Walk-up is a separate pre-step** — not integrated into `resolve_operator_config()`. Cleaner separation, easier to test, lower risk to existing resolution logic.
186+
5. **No changes to `init`** — `init` creates config + index as before. The walk-up only helps find existing configs from subdirectories.
187+
6. **No `--walk-up` opt-out flag** — walk-up is always-on when no explicit source root is given. If a user hits the wrong config, the fix is to move or remove the stray config file, not to add a flag.
188+
189+
## 11. Migration plan — 1 PR
168190

169191
Single PR containing:
170192
1. `discover_project_root()` function in `config.py`

0 commit comments

Comments
 (0)