spec-kitty / spec-kitty/spec-kitty

Follow-up after #4189/#4190/#4194 land: finish shifting the doctrine-artifact resolver/loader abstraction boundary

Open
#4,192 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
1.6k
Forks
165
Avg merge
14h 22m
Merged PRs (30d)
343

Description

## Status update (supersedes the original framing below)

All three originating bugs now have PRs open, and two of the three land the
recommended fix directly:

| Issue | PR | Status | Matches recommended boundary? |
|---|---|---|---|
| #4186 (node inference) | #4190 | open | Orthogonal — implements the missing feature directly. Not part of the resolver/loader-duplication problem. |
| #4187 (validate/loader schema mismatch) | #4189 | open | **Yes** — adds `_validate_org_fragment`, which delegates to the same `load_org_pack` entry point the runtime uses. One loader, two call sites, as recommended. |
| #4185 (directive-ID resolution) | #4194 | open (draft) | **Yes, substantially** — see below. |

**This issue is now a follow-up, gated on #4189, #4190, and #4194 landing** — not
an alternative to them. It tracks the residual structural risk that remains
*after* all three merge, so the abstraction boundary doesn't quietly drift back
open at the next call site the way it did before this investigation.

### What #4194 actually fixes (verified by reading the diff)

- `DirectiveRepository.get()` (`src/charter/offering/directives/repository.py`)
now tries the exact declared ID before falling back to
`normalize_directive_id()` — closing the `.get()`-misses-what-`.list_all()`-finds
bug directly.
- The two independently-authored, ad hoc "try both directions" promotion
resolvers — one inline in `org_charter.py`'s old `_resolve_required_id_to_stem`,
a separate one implicitly used by `interview.py`'s selection promotion — are
deleted and replaced by one shared function,
`kind_vocabulary.resolve_selected_id_to_stem`, called from both
`_promote_interview_selections` (`interview.py`) and
`_normalize_required_ids` (`org_charter.py`).
- Both of those call sites now thread `org_roots`/`layer_roots` via
`resolve_org_root_chain`/`resolve_layer_roots`
(`specify_cli/cli/commands/charter/_layer_roots.py`) — the same helper
`activate`/`deactivate` already used correctly — closing the literal missing
wire that made org-pack IDs unresolvable from this path.
- `resolve_artifact_urn` now falls back to matching by declared `id:` (via
`resolve_config_id`) when filename-stem matching fails, for directives
specifically — closing the stem-vs-id mismatch.
- `DoctrineResolver.directives` (`charter/activation/resolver.py`) was also
tightened to compare exact identities through the same shared vocabulary
instead of its own separate normalization path.

This is a real, well-targeted consolidation — not a point patch on the one
reported call site. It stops short of literally merging `DirectiveRepository`
and `kind_vocabulary`'s resolvers into a single class, which is a reasonable,
lower-risk scope call for a bugfix PR rather than a rewrite.

### What remains open after #4189 + #4190 + #4194 land

1. **`org_roots`/`layer_roots` are still optional parameters that silently
default to "built-in only" if omitted.** `resolve_selected_id_to_stem`,
`resolve_artifact_urn`, and `resolve_config_id` all keep the signature shape
`org_roots: list[Path] | None = None`. #4194 fixes every *current* call site,
but the parameter contract itself still lets a future call site (a new CLI
command, a new migration, a new consumer) omit the argument and silently
degrade to missing the org layer entirely — reproducing this exact bug class
with no type-level guard against it. Recommend either making the parameter
required (no default), or wrapping `doctrine_root`/`org_roots`/`layer_roots`
into one `PackContext`-shaped object that every resolver call takes as a
single required argument, so "which roots to scan" cannot be an
easy-to-forget optional.

2. **`DirectiveRepository.get()` and `kind_vocabulary`'s resolvers are now
behaviorally consistent for the reported bug shape, but are still two
independently-implemented lookup algorithms** (one an in-memory dict keyed
by `id:` with its own `normalize_directive_id` fallback; the other a
filesystem walk matching by stem-then-id via `resolve_artifact_urn`/
`resolve_config_id`). Post-merge, add a cross-cutting regression test
(referenced but not yet confirmed present) asserting: for any org-authored,
non-built-in-shaped directive ID, `DirectiveRepository.get()`,
`kind_vocabulary.resolve_artifact_urn`, `resolve_config_id`, and the
interview/generate promotion path all agree on the same artifact — in one
test, not four isolated ones. This is the exact test *shape* the debugger
lens found missing across the whole suite (see the original investigation
below); confirm #4194's new tests
(`tests/charter/test_directive_identity_mapping.py`,
`tests/specify_cli/cli/commands/test_charter_org_directive_identity.py`,
`tests/doctrine/directives/test_repository.py`) actually close that gap
end-to-end, or add one that does if they test each surface independently.

3. **A "validate implies loadable" architectural test still doesn't exist.**
#4189 fixes the specific DRG-fragment-shape mismatch, but there is still no
general assertion in the suite that *any* pack `pack validate` reports 0
errors on will also load cleanly through `doctor doctrine`/`charter context`
on the same fixture. Recommend one property-style test that generates a
pack, runs it through `pack validate`, and — only if that passes — runs it
through the real runtime loader, failing loudly if the runtime loader
rejects something the validator accepted. This is the general form of what
#4187 was one instance of; closing only the instance leaves the general
validate/load-agreement gap open for the next new artifact kind or field.

4. Confirm #4194's draft status resolves to green on the full frozen-source
run (`make test-fast`, format, lint, typecheck, coverage) before treating
#4185 as closed — the PR body notes this was still in progress as of
filing.

**Action for this issue going forward:** re-triage once #4189, #4190, and
#4194 are merged. If items 1–3 above are addressed by then (either in those
PRs' final form or in follow-up commits), close this issue. If not, convert it
into the specific, scoped tickets for whichever of 1–3 remain.

---

## Original investigation (for context — see status update above for current state)

## Summary

Three bugs found and filed while building an org-doctrine-pack proof of concept —
#4185, #4186, #4187 — were investigated by a four-lens adversarial architecture
review (structure/topology, doctrine/DRG wiring, test-coverage/live-evidence,
decomposition/ownership boundaries), each reading the actual source in this repo
independently before returning a verdict. All four converged on the same
conclusion with no irreconcilable disagreement:

**#4185 and #4187 share one architectural root cause. #4186 does not.**

The root cause: **there is no single canonical implementation of "resolve a
doctrine artifact by ID" or "load/validate an org pack's `drg/fragment.yaml`."**
At least four independent implementations of these two concepts exist in the
codebase, each with its own key-normalization or schema, and they have drifted
out of sync with each other. #4186, by contrast, is a plain missing feature — no
duplicated or drifted implementation is involved.

This issue exists to record the recommended abstraction boundary so a fix to
#4185 doesn't just patch the one broken call site and leave the underlying
duplication in place for the next caller to rediscover.

## The four independent "does this exist" implementations found

1. **`charter.offering.directives.repository.DirectiveRepository`**
(`src/charter/offering/directives/repository.py`), backed by
`BaseDoctrineRepository._key`/`_load()` (`src/charter/offering/base.py`).
Keys the loaded dict by the **raw, unnormalized** `id:` field. But
`DirectiveRepository.get()` pipes its **query** key through
`normalize_directive_id()` (`src/charter/offering/drg/migration/id_normalizer.py`)
before the dict lookup — a transform whose fallback branch
(`raw.upper().replace("-", "_")`) is only guaranteed correct for built-in-shaped
IDs. Any org- or project-authored ID that isn't invariant under that transform
makes `.get(id)` return `None` while `.list_all()` still enumerates it — a
self-contained bug, independent of any org-pack loading, that reproduces
#4185's core symptom on its own.

2. **`charter.activation.kind_vocabulary.resolve_artifact_urn` /
`resolve_config_id`** (`src/charter/activation/kind_vocabulary.py`) — a second,
independently-implemented resolver that matches by **filename stem**, not by
the artifact's `id:` field, walking the filesystem itself via
`_iter_artifact_paths`. This is what `charter generate --from-interview`'s
`required_directives` promotion actually calls (via
`src/specify_cli/doctrine/org_charter.py:_resolve_required_id_to_stem` →
`src/charter/activation/compiler.py:_resolve_config_activated_ids`). Two
compounding defects here: (a) `org_charter.py`'s call site never threads
`org_roots`/`layer_roots` at all — unlike the sibling `activate`/`deactivate`
commands, which correctly assemble org roots via
`src/specify_cli/cli/commands/charter/_layer_roots.py` before calling the same
function — so an org-pack ID can never resolve here regardless of collisions;
and (b) even when `org_roots` is threaded correctly (as `compiler.py` does),
the algorithm matches by file-stem, not by `id:` field, so an org author who
(reasonably) doesn't know or follow the built-in `NNN-slug` filename
convention gets no match anyway.

3. **`charter.offering.drg.org_pack_loader._OrgDRGNode` /
`OrgDRGFragment`** (`src/charter/offering/drg/org_pack_loader.py`) — the
schema the **runtime** org-DRG loader (used by `doctor doctrine`/
`charter context`, via `charter.activation._drg_helpers.load_validated_graph`)
enforces on `drg/fragment.yaml`: `{id: str, kind: }`,
`extra="forbid"`.

4. **`charter.offering.drg.models.DRGNode` / `NodeKind`**
(`src/charter/offering/drg/models.py`) — a fourth, independently-defined
Pydantic schema for "a DRG node": `{urn: str, kind: }`,
also `extra="forbid"`. Until PR #4189 (see below), this was the schema
`pack_validator._validate_drg` (`src/specify_cli/doctrine/pack_validator.py`)
checked — but only against a **different file**, `drg/*.graph.yaml`, which the
runtime loader never reads at all. `pack validate` was therefore not
"validating against the wrong model" so much as never opening the file the
runtime actually consumes.

Layered on top: the actual DRG **merge** implementation
(`charter.offering.drg.merge.merge_three_layers`) is, by contrast, genuinely
singular and well-factored — it is the "one canonical merge" the rest of the
system should be modeled on. The problem is everything upstream of it
(resolution and validation) that doesn't yet route through one door the way the
merge step does.

## The recommended abstraction boundary

- **One canonical directive/artifact-ID resolver.** A single function or class,
constructed once per command invocation with the *full* layer context
(built-in + every configured org root + project root) — not reassembled ad hoc
with an optional `org_roots`/`layer_roots` parameter at each of roughly nine
call sites (`org_charter.py`, `activate.py`, `deactivate.py`, `compiler.py`,
`consistency_check.py`, `drg_activation.py`, migrations, …), where a forgotten
argument silently degrades to "built-in only" instead of failing loudly. Every
caller — repository `.get()`, the interview/generate promotion path, `doctor
doctrine`, `pack validate` — should resolve an ID through this one door. Either
`DoctrineService.directives`/`BaseDoctrineRepository` (which already binds org
roots once, at construction time, rather than per call) should become that
door, with its own `.get()`/`.list_all()` key-normalization made internally
consistent first; or `kind_vocabulary.resolve_artifact_urn` should become it,
with a mandatory (not optional) roots argument and matching against the
artifact's `id:` field as well as filename stem — but not both existing
independently, as today.

- **One canonical DRG-fragment loader/validator.** Exactly one parser for
`drg/fragment.yaml` — `org_pack_loader.load_org_pack`/`OrgDRGFragment` — should
be what both `pack validate` and the runtime loader call. PR #4189 (linked
below) implements exactly this for the validate side by adding
`_validate_org_fragment`, which delegates to `load_org_pack`. That is the
correct shape and, if merged as designed, closes the specific validator/loader
mismatch in #4187. It does not, on its own, address the ID-resolution
duplication in finding 1–2 above, since that is a separate code path
(directives, not DRG fragments).

## Cross-references — prior/in-flight work

- **#4185** — *charter generate/interview cannot resolve org-pack directive IDs* —
**no PR open yet** as of this writing. This is the one still needing the
canonical-resolver fix described above (thread `org_roots` through
`org_charter.py`'s promotion path at minimum; ideally consolidate
`DirectiveRepository`/`resolve_artifact_urn` into one resolver so this class of
bug can't recur at the next call site). Any fix PR for #4185 should be
evaluated against the abstraction boundary above, not just against making the
one reported call site pass.
- **#4186** — *org-pack DRG fragment `nodes: []` doesn't infer nodes* — **PR #4190**
(`[#4186] Infer org-pack DRG nodes from artifact files`) is open and implements
the missing inference feature directly in `org_pack_loader.py`, matching the
documented behavior rather than removing the doc claim. This resolves #4186 on
its own terms and is orthogonal to the ID-resolution/DRG-validation duplication
described above — it does not need to be superseded by this issue's
recommendation, only tracked to completion.
- **#4187** — *pack validate and the runtime DRG loader accept incompatible node
shapes* — **PR #4189** (`[#4187] Validate org DRG fragments through the runtime
loader`) is open and routes `pack_validator` through the same `load_org_pack`
entry point the runtime uses, which is exactly the "one loader" fix this issue
recommends for the DRG-fragment half of the problem. Recommend confirming this
PR is merged and that no second, independent fragment-validation path is
reintroduced elsewhere before considering the DRG-fragment side of this issue
closed.

## Suggested follow-up

1. Land #4189 as-is (or with review) — it already matches the recommended
boundary for DRG-fragment validation.
2. Land #4190 — orthogonal, resolves #4186 on its own.
3. For #4185, do not fix only the reported call site
(`org_charter.py:_resolve_required_id_to_stem`). Fix `DirectiveRepository`'s
internal `.get()`/`.list_all()` key mismatch first (finding 1), then either
consolidate `resolve_artifact_urn` into that repository or make every one of
its call sites take a mandatory, single-source-of-truth roots argument
(finding 2) — with a regression test that constructs a genuinely org-native
(non-built-in-shaped) directive ID and round-trips it through both the
repository and the interview/generate promotion path in one test, since no
existing test in the suite does this today (per the debugger lens's findings
in the linked investigation).
4. Consider a follow-up architectural test: given any org pack that
`pack validate` reports 0 errors on, assert it also loads cleanly through
`doctor doctrine`/`charter context` on the same fixture — closing the general
"validate and load must agree" gap this whole investigation surfaced, not just
the specific instance in #4187.

## Found while

Building a multi-tier (company/division/department/team) org doctrine pack
demonstration (meridian-poc), then running a bounded four-lens adversarial
architecture review specifically on the question "is this a faulty-abstraction
problem?" against
`/Users/robert/spec-kitty-dev/spec-kitty-20260908-235743-zHOnAv/spec-kitty`.

Contributor guide

Open the contributing guide

Research direction

Re-triage after PRs #4189, #4190, and #4194 merge. Read the resolver and loader paths in src/charter/activation/kind_vocabulary.py, src/charter/offering/directives/repository.py, and the related CLI call sites; run the referenced identity tests and the full frozen-source checks. Done means the remaining items 1–3 are addressed or split into specific scoped tickets.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.