feat(graph): GraphView contract + provider/sink registries (#348, B1)#402
Conversation
Typed graph models (NodeStatus/EdgeType enums, Node/Edge/GraphView pydantic models with edge-endpoint validation), async GraphProvider ABC, GraphSink ABC, and name-keyed registries for both. Tests included (21 passing).
Adds a concrete GraphSink.query() that raises NotImplementedError unless "query" is in capabilities, closing the FR-5 gap flagged in §12a iteration 1. Sink export() return-shape docstring and Node.status default were already correct in code; design docs were amended separately in the main checkout.
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #402 — feat(graph): GraphView contract + provider/sink registries (#348, B1)
Summary
This is B1 of the Issue-#348 graph-layer epic: it adds the typed GraphView contract
(Node/Edge/GraphView pydantic models + NodeStatus/EdgeType enums), an async
GraphProvider ABC with a name-keyed registry, and a GraphSink ABC with a
capability-gated query() and its own registry. Contract-only — no routes, providers, or
I/O yet. The PR is clean, well-scoped, and layout-conformant; the verifier confirmed all
its claims (23 tests pass, black/isort/mypy clean, imports resolve, contract behaves as
documented). Recommendation: Request changes (minor) — fix the one real validator bug,
close the three test gaps, and anchor the design-doc references. None break anything today,
but this is the foundational contract three more bolts depend on, so getting it right now
compounds.
Important (should fix)
- [correctness] src/cli_agent_orchestrator/graph/models.py:87 — 🆕
_KIND_PATTERNis
matched with.match()and anchored with$. In Python's default (non-MULTILINE) mode
$also matches just before a trailing\n, sokind="topic\n"passes
validate_kind_shape, defeating the "lowercase-snake-case" invariant and letting a
newline into a value that downstream bolts will likely use as a DOM id / CSS class /
graphviz token. Fix:_KIND_PATTERN.fullmatch(value)or anchor with\Z. (introduced) - [tests] test/graph/test_registries.py — 🆕 Three coverage gaps in the new contract's
tests:- The FR-5 middle branch — a sink that declares
capabilities={"query"}but does
not overridequery()(raises the distinct "declared but did not override"
NotImplementedError) — is never exercised. Add a sink with the capability and no
override, assertNotImplementedError. GraphProvider.project()/GraphSink.export()are defined in test subclasses but
never called or awaited — registry tests assert onlyisinstance+ list
membership. The core "uniformly async, returns emptyGraphView" contract (ADR-7)
has noawaiting test. Add an async test:gv = await get_provider(...).project(); assert gv.nodes == [].- Registry pollution — module-level
_REGISTRY/_SINK_REGISTRYare mutated with no
teardown (no autouse fixture / conftest /clear()). A single run passes because
names are unique, but registrations leak and a same-process rerun hits the duplicate
ValueErrorspuriously. Add an autouse fixture that snapshots+restores both
registries. (introduced)
- The FR-5 middle branch — a sink that declares
- [consistency] src/cli_agent_orchestrator/graph/models.py:44 (and providers/base.py,
sinks/base.py docstrings) — 🆕 Every docstring cross-references design-doc identifiers
(U1,U4,U8,FR-3,FR-4,FR-5,ADR-7) that resolve to nothing in the repo —
a grep across the worktree finds them only in these new files. A reader can't resolve any
of them; if the design doc renumbers, these rot silently with no in-repo anchor. Anchor
them once (e.g. link #348 at module level) or drop the bare codes from per-symbol
docstrings. (introduced) - [security] src/cli_agent_orchestrator/graph/sinks/base.py:253 — 🆕 The
export(view, dest, **options) -> list[str]contract takes a rawdestwith no stated
confinement requirement. A later sink honoring this contract literally could write to
../../…or an absolute path. Design-level, but the contract is being set now: document
in the ABC that implementations MUST resolve+confinedestunder an allowed base
(reject.., absolute escapes, symlinks) and name who owns validation (U4 route vs
sink). (introduced, design-level) - [security] src/cli_agent_orchestrator/graph/models.py:82 — 🆕
attrs: dict[str, Any]
plus unvalidatedid/labelflow verbatim throughto_dict()into "U4's routes and
U8's renderer" (per the docstring) — untrusted, provider-supplied data reaching a web
renderer is a stored/DOM-XSS surface if U8 injectslabel/attrswithout escaping. Mark
these as untrusted in the contract and make escaping a documented obligation of the
renderer/sinks. (introduced, design-level)
Nits (optional)
- [correctness/conventions] models.py:118 —
to_dict()hand-rolls per-field
serialization thatself.model_dump(mode="json")already produces identically (enums →
.value, dicts pass through). Non-idiomatic for this pydantic-heavy repo and silently
drops any field added later toNode/Edge. Prefermodel_dump(mode="json"), or keep
the explicit map with a comment if U4/U8 need a frozen shape. (introduced; flagged by two
reviewers) - [correctness] models.py:118 —
to_dict()returns the model's ownattrs/metadicts
by reference; a caller mutatingview.to_dict()["meta"]["x"]mutates live model
state, and two calls hand out aliased dicts. Copy nested dicts (or usemodel_dump).
(introduced) - [correctness] models.py:108 —
validate_edge_endpointsbuildsnode_idsas a set, so
duplicate node ids collapse silently andto_dict()emits two nodes with the same id
(breaks any id-keyed renderer). Alsoid/labelaccept empty strings. If node-id
uniqueness is a contract invariant, enforce it here. (introduced) - [security] models.py:82,106 — No length/size bounds on
id,label,attrs,meta,
or the node/edge lists — an upstream provider could emit an arbitrarily largeGraphView
(memory/DoS once wired). Considermax_length/max_itemsor a documented size cap.
(introduced, design-level) - [conventions] graph/init.py:10-21 — Top-level re-exports import straight from
.providers.base/.sinks.base, bypassing theproviders/__init__.pyand
sinks/__init__.pyseams defined right beside them. Importing from the subpackage
__init__keeps a single public seam per subpackage (cf.services/memory_archive/).
(introduced) - [consistency] models.py:71 / NodeStatus —
EdgeType.SUPERSEDESand
NodeStatus.PROPOSAL/OBSERVATION/SUPERSEDEDare reserved/unpopulated in this
deliverable (no provider sets a non-default status). Self-documented and cheap to keep —
a scope note, not a defect. (introduced) - [tests] test_models.py —
to_dict()serialize tests only use defaultstatus=active
and emptyattrs; non-default status mapping (PROPOSAL → "proposal") and populated
attrs/metapassthrough are never asserted throughto_dict(). (introduced)
Tests
23 tests added (test/graph/test_models.py = 10 fns, one parametrized ×6 → 15 collected;
test/graph/test_registries.py = 8). Model-validation coverage is solid: kind regex (6
shapes), status/edge-type literals, dangling source and target both rejected, dup +
unregistered for both registries, and the query capability gate (no-capability and
declared-and-overridden). The "23 tests" claim is accurate. Gaps (see Important): the
declared-but-not-overridden query() branch, any awaiting of project(), and registry
teardown/isolation are missing; non-default to_dict() shapes are uncovered.
Verification
Verifier ran the PR's actual code in the worktree (host python 3.12.8, PYTHONPATH=src):
- ✓ VERIFIED "23 tests" —
pytest test/graph/ -v→ 23 passed / 0 failed (0.91s). - ✓ VERIFIED "black/isort/mypy clean" —
black --check"9 files would be left
unchanged";isort --check-onlyexit 0;mypy src/…/graph/"Success: no issues found in
6 source files". - ✓ VERIFIED clean import —
from cli_agent_orchestrator.graph import Node, Edge, GraphView, GraphProvider, GraphSink→ OK. - ✓ VERIFIED contract behavior — dangling edge →
ValidationError; valid
to_dict()→ correct wire shape with enums serialized to.value; provider/sink
register+resolve+instantiate; asyncproject()returns emptyGraphView;query()
without capability →NotImplementedError; declared-but-not-overridden → distinct
NotImplementedError; duplicate register →ValueError.
No claim was refuted. (Note: the verifier's manual driver exercised the
declared-but-not-overridden query() path and the async project() path — the gap is that
the committed test suite doesn't cover them, not that the code is broken.)
Verdict
Request changes (minor) — the PR is clean, verified, and well-scoped, but as the
foundational contract for the #348 epic it's worth fixing the kind trailing-newline
bypass, closing the three test gaps (query-not-overridden branch, async project(),
registry teardown), and anchoring the design-doc references before three more bolts build
on it. The security items are design-level obligations to document now, not code defects
today.
There was a problem hiding this comment.
Pull request overview
This PR introduces the first “graph layer” building blocks for CAO: a typed GraphView contract (nodes/edges/enums) plus name-keyed registries for pluggable async graph providers and export/query-capable sinks, with unit tests validating the contract and registry behavior.
Changes:
- Adds Pydantic models/enums for
Node,Edge, andGraphView(including endpoint validation + wire serialization). - Adds
GraphProvider(async) andGraphSink(capability-gatedquery()) ABCs with simple name registries. - Adds unit tests covering model validation/serialization and registry duplicate/lookup behaviors.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/graph/test_registries.py | Tests provider/sink registries and sink query capability gate. |
| test/graph/test_models.py | Tests Node/Edge/GraphView validation and GraphView.to_dict() wire shape. |
| test/graph/init.py | Establishes test.graph package. |
| src/cli_agent_orchestrator/graph/sinks/base.py | Defines GraphSink ABC, query() capability gate, and sink registry functions. |
| src/cli_agent_orchestrator/graph/sinks/init.py | Exposes sink seam + registry helpers via package exports. |
| src/cli_agent_orchestrator/graph/providers/base.py | Defines async GraphProvider ABC and provider registry functions. |
| src/cli_agent_orchestrator/graph/providers/init.py | Exposes provider seam + registry helpers via package exports. |
| src/cli_agent_orchestrator/graph/models.py | Introduces graph contract models/enums plus edge-endpoint validation and serialization helper. |
| src/cli_agent_orchestrator/graph/init.py | Public graph-layer exports aggregating models and registries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not _KIND_PATTERN.match(value): | ||
| raise ValueError(f"kind must be a non-empty lowercase-snake-case string, got {value!r}") |
…gaps, contract docs Fixes trailing-newline bypass in kind validation, enforces node-id uniqueness, closes three test coverage gaps (query-not-overridden, async project(), registry teardown), anchors design-doc references, and documents the export()/attrs security contracts.
|
Pushed fixes for the review findings in bcee2b9. Important (fixed):
Verify-and-fix (gutosantos82's duplicate-node-id nit): confirmed — uniqueness was not enforced; Nits taken: Documented only (per review guidance): no size cap added — Tests: 30 passed ( Thanks both for the thorough pass — the trailing-newline validator bug and the registry-teardown gap in particular were good catches. |
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #402 — feat(graph): GraphView contract + provider/sink registries (#348, B1)
Summary
This is B1 of the Issue-#348 graph-layer epic: the typed GraphView contract
(Node/Edge/GraphView pydantic models + NodeStatus/EdgeType enums), an async
GraphProvider ABC with a name-keyed registry, and a GraphSink ABC with a
capability-gated query() and its own registry. Contract-only — no routes, providers, or
I/O yet. This is a re-review of commit bcee2b9, which the author already used to
close a prior review round (kind-validator .match()→.fullmatch(), duplicate-node-id
enforcement, capability-gate tests, and a registry-isolation conftest). The verifier ran
the tree end-to-end and confirmed all 7 claims (30 tests pass, black/isort/mypy clean,
the fullmatch fix is genuinely behavior-changing). All previously-important issues are
fixed; only nits remain. Recommendation: Approve with nits.
Important (should fix)
None introduced by this PR that survive verification. (The prior round's important
findings are already fixed in bcee2b9.)
Nits (optional)
- [consistency] src/cli_agent_orchestrator/graph/models.py:5 (also providers/base.py, sinks/base.py) —
All three module docstrings anchor the design toaidlc/spaces/default/intents/260709-graph-layer/,
a path that exists nowhere in the repo. The per-symbol codes (U1, U4, U8, FR-3/4/5,
ADR-7) resolve through that anchor, so a GitHub reader can't resolve any of them today.
The docstrings honestly label it "AIDLC intent, not shipped with the package," so this is
forward-documentation, not a bug — but consider also linking issue #348 as the resolvable
anchor. - [conventions] CHANGELOG.md — no
[Unreleased]/Addedentry for the graph layer (#348).
The repo logs substantial additive modules even when infra-flavored (e.g.examples/fleet
#349/#366 both got entries). Defensible to defer until U4 routes make it user-facing, but
a one-line "add graph layer contract + provider/sink registries (#348)" would match
convention. Not in this PR's diff. - [tests] test/graph/test_registries.py:90 — the no-query-capability test asserts
pytest.raises(NotImplementedError)withoutmatch=. Bothquery()branches raise
NotImplementedError; if the two branches were swapped this test wouldn't catch it. Add
match="does not support"to pin it to the correct branch, mirroring the
declared-not-overridden test. - [tests] coverage nits — no positive case for valid snake_case
kindvariants beyond
"topic"(_KIND_PATTERNallows[a-z][a-z0-9_]*, untested on the accept side for
topic_name/a1);Edge.attrsround-trip never exercised with a non-empty value (Node's
is); fresh-instance-per-get_*behavior not asserted. All low-value. - [correctness] src/cli_agent_orchestrator/graph/models.py:100 —
to_dict()/
model_dump(mode="json")defers failure: a non-JSON-serializable value inattrs/meta
(e.g. aset) constructs the model fine but only blows up at serialization time. By
design for B1 (attrsdocumented as trusted local-provider data); worth a guard/note when
U4/U8 land. - [correctness] src/cli_agent_orchestrator/graph/providers/base.py:37 & sinks/base.py:66 —
register_provider/register_sinkdon't verify the decorated class is a
GraphProvider/GraphSinksubclass at runtime; a mis-annotated class registers silently
and fails only whenproject()/export()is called. mypy catches the common case. Low
value. - [security] src/cli_agent_orchestrator/graph/sinks/base.py:16 — the
dest
path-confinement contract is docstring-only; nothing structurally forces a future sink
author to call it. For the sink-implementation bolt, consider a concrete helper on
GraphSink(e.g._confine(dest, base)wrappingresolve_and_validate_path) plus a
shared traversal-rejection test fixture. Not required for a contract PR. - [conventions] registry pattern divergence — the module-level-dict +
register_/get_/list_
decorator seam is a new pattern for CAO (existing seams are entry-point discovery in
plugins/registry.pyor enum +if/elifdispatch inproviders/manager.py). Clean and
internally consistent; if third-party graph providers/sinks are ever expected, entry-point
discovery would match the plugins seam. Design note, not a blocker.
Pre-existing (not introduced — do not block on this PR)
- [conventions|CI] pyproject.toml:110 —
[tool.mypy] python_version = "2.1.1"is
invalid (mypy expects"3.10"; also mismatchesrequires-python >=3.10). Confirmed
NOT in this PR's diff — it's a pre-existing repo-wide CI bug worth its own fix, not
attributable here. (The verifier's clean mypy run suggests the local/CI invocation targets
thegraph/package specifically and isn't tripped by it, but the setting should still be
corrected separately.)
Tests
Adequate and well-targeted for a contract/registry PR. 30 tests across test_models.py
(Node/Edge/GraphView incl. dup-node-id, dangling endpoints, 7-case parametrized kind
rejection incl. "topic\n", non-default-status serialization) and test_registries.py
(round-trip, dup→ValueError, missing→KeyError, capability gate with the
declared-vs-not-overridden distinction pinned via match="did not override", and async
project() actually awaited). The conftest.py autouse fixture snapshots/restores both
registries — correctly handles the global-mutable-dict leak risk. asyncio_mode="strict"
- explicit
@pytest.mark.asyncioverified inpyproject.toml, so async tests are collected
and awaited (not silently skipped). Every changed behavior has a regression test; only the
nits above.
Verification
Verifier ran the PR worktree end-to-end in a clean container (numpy built fine there — no
GCC issue). Baseline: 30 passed in 1.55s, 0 failed.
- ✓ VERIFIED — 30 tests pass via
pytest test/graph/ -q(confirms author's "30", not the
body's stale "23"). - ✓ VERIFIED —
black --check(10 files unchanged),isort --check-only(clean),
mypy src/…/graph/("Success: no issues found in 6 source files"). - ✓ VERIFIED — kind validator rejects trailing newline; differential proof:
re.match
accepts"topic\n",re.fullmatchrejects it — the fix is genuinely behavior-changing. - ✓ VERIFIED — duplicate node ids raise; async
project()awaitable →GraphView;
query()capability gate raises the correct message on each branch;to_dict()emits
enums as plain strings ("active","relates_to","proposal").
No refutations, no unverified claims.
Verdict
Approve with nits. Clean, well-scoped, fully-verified contract PR; the prior round's
real issues are already fixed in bcee2b9. The remaining items are optional polish and one
pre-existing CI setting outside this diff. Nothing blocks merge.
Brings the branch current with upstream: profile lifecycle (awslabs#395), GraphView contract (awslabs#402), bundled cao-mcp-server launch fix (awslabs#403), and the v2.3.0 release. Feature surface unchanged; only conflict resolution below. Co-authored-by: Kiro Agent <244629292+kiro-agent@users.noreply.github.com> # Conflicts: # CHANGELOG.md # pyproject.toml # uv.lock
Summary
Node/Edge/GraphViewpydantic models,NodeStatus+EdgeTypeenumsGraphProviderABC + registryGraphSinkABC with capability-gatedquery()+ registryFirst Bolt of the Issue-348 graph-layer epic (B1 of 4; next: providers, API+sinks, renderer).
Testing
Part of #348