CANP-WP-0005: document and detect inclusion diamonds
Section 10.4 said nothing about inclusion being textual and repeated, so an author met the behaviour only by reading rendered output carefully. That silence was the defect. Writing it up got the mechanism wrong at first. I documented a dependency "reached by two paths" through two include chains, and wrote a detector that counted composer calls to match. Tested it against the real case that motivated the task — helix/repo-advance — and it did not fire, though the conventions block still rendered twice. The actual mechanism is subtler. helix/commit-sync declares its own `conventions` input; when composed, resolution passes the including package's already-resolved values down by name, so it *inherits* the outer text rather than resolving its own include, and renders it again. Nothing was included twice; the text appeared twice regardless. Both mechanisms are now documented with worked diagrams. duplicate_inclusions detects both — included values that are equal, and an included value contained within another — and resolve, render and eval warn. Nothing is deduplicated, as leaned. Deduplicating means choosing which occurrence survives, since position in a prompt carries meaning, and deciding what happens when two paths select different versions of the same dependency, which section 10.3 permits. That is resolver behaviour and section 10.4 keeps composition declarative. Verified by reintroducing the diamond in a scratch copy of repo-advance, which warns, and confirming the shipped factored collection stays silent. Tests 90 -> 95. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bjefh8NUiEiahN4JLwoSKM Assistant: claude-code Assistant-Model: opus Assistant-Process: 388925@bnt-lap001 Assistant-Session: 3507023f-e0fd-4a1e-9d90-a0d4217d1502
This commit is contained in:
parent
e16bf024d4
commit
75e0597019
5 changed files with 180 additions and 7 deletions
|
|
@ -741,6 +741,60 @@ that the including package does not supply is an error naming both packages.
|
|||
Implementations MUST detect inclusion cycles and report them rather than
|
||||
recursing.
|
||||
|
||||
#### Inclusion is textual and repeated
|
||||
|
||||
Inclusion is not deduplicated, and there are two ways the same text ends up
|
||||
rendered twice.
|
||||
|
||||
**Two inputs include the same dependency.** The values are equal and both are
|
||||
substituted:
|
||||
|
||||
```text
|
||||
review/change
|
||||
├── style → include style/house
|
||||
└── preamble → include style/house ← the same text again
|
||||
```
|
||||
|
||||
**An included package inherits an outer input of the same name.** This one is
|
||||
easy to miss, because nothing was included twice:
|
||||
|
||||
```text
|
||||
review/change
|
||||
├── conventions → include team/conventions ← rendered here
|
||||
└── routine → include review/commit-routine
|
||||
└── {{ conventions }} ← inherited (§ 10.4), rendered again
|
||||
```
|
||||
|
||||
`review/commit-routine` declares its own `conventions` input. When it is
|
||||
composed, resolution passes the including package's already-resolved values
|
||||
down by name, so it inherits the outer text rather than resolving its own — and
|
||||
renders it a second time. No dependency was included twice; the text still
|
||||
appears twice.
|
||||
|
||||
Nothing is wrong with either package. The duplication is a property of how they
|
||||
were composed.
|
||||
|
||||
Deduplication is deliberately not specified. It would require deciding which
|
||||
occurrence survives — position in a prompt carries meaning — and what happens
|
||||
when the two paths select *different versions* of the same dependency (§ 10.3
|
||||
permits that: one path may pin `1.0.0` while another asks for `newest`).
|
||||
Resolving that is a resolver's job, and § 10.4 keeps composition declarative.
|
||||
|
||||
The fix belongs to the author, and it is usually a better factoring. Extract the
|
||||
part that is genuinely shared into its own fragment and include it once at each
|
||||
level that needs it, rather than composing a whole package that carries it:
|
||||
|
||||
```text
|
||||
review/change
|
||||
├── style → include style/house
|
||||
└── routine → include review/commit-routine (no style of its own)
|
||||
```
|
||||
|
||||
A tool that renders a package SHOULD report when an included value will render
|
||||
more than once — whether because two inputs resolved to the same text, or
|
||||
because one included value contains another — since the author usually did not
|
||||
intend it.
|
||||
|
||||
CPF does **not** define template inheritance. A package does not extend
|
||||
another, override its sections, or inherit its inputs. Composition is by
|
||||
reference only, for four reasons drawn from this specification and from
|
||||
|
|
|
|||
|
|
@ -79,6 +79,10 @@ rather than resolved by guessing.
|
|||
print qualified `<registry>:<id>` references.
|
||||
- `include` defaults are satisfied (inclusion is deterministic); `derive`
|
||||
defaults are reported, not run. Inclusion cycles are detected and named.
|
||||
- `resolve`, `render` and `eval` warn when an included value will render more
|
||||
than once — two inputs including the same thing, or an included package
|
||||
inheriting an outer input of the same name. Nothing is deduplicated; the fix
|
||||
is a better factoring.
|
||||
- `eval` runs the deterministic render checks of any eval declaring the
|
||||
`canned-prompts/eval-rubric/v0.1` schema, and reports output criteria as
|
||||
declared but not run. Unrecognized schemas are skipped, not rejected. A
|
||||
|
|
|
|||
|
|
@ -783,6 +783,45 @@ def report_missing_dependencies(catalog: Path, manifest: dict[str, Any]) -> None
|
|||
)
|
||||
|
||||
|
||||
def duplicate_inclusions(resolution: Resolution) -> list[str]:
|
||||
"""Included values that will render more than once (§ 10.4).
|
||||
|
||||
Two ways this happens, and both are the same defect to an author:
|
||||
|
||||
- two inputs include the same dependency, so the values are equal;
|
||||
- an included package inherits an outer input of the same name and renders
|
||||
it again, so one included value *contains* another.
|
||||
|
||||
The second is the common one and is easy to miss: nothing was included
|
||||
twice, but the text appears twice all the same.
|
||||
"""
|
||||
included = {
|
||||
name: str(resolution.values[name])
|
||||
for name, origin in resolution.origins.items()
|
||||
if origin.startswith("included from") and name in resolution.values
|
||||
}
|
||||
duplicated: set[str] = set()
|
||||
for name, value in included.items():
|
||||
if not value.strip():
|
||||
continue
|
||||
for other, other_value in included.items():
|
||||
if other != name and (value == other_value or value in other_value):
|
||||
duplicated.add(name)
|
||||
return sorted(duplicated)
|
||||
|
||||
|
||||
def report_repeated_inclusions(resolution: Resolution) -> None:
|
||||
repeated = duplicate_inclusions(resolution)
|
||||
if repeated:
|
||||
print(
|
||||
"rendered more than once: "
|
||||
+ ", ".join(repeated)
|
||||
+ " — inclusion is textual and is not deduplicated (§ 10.4); "
|
||||
"extract the shared part into its own fragment",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
|
||||
def report_skipped(skipped: list[str]) -> None:
|
||||
if skipped:
|
||||
print(
|
||||
|
|
@ -1154,7 +1193,9 @@ def cmd_render(args: argparse.Namespace) -> None:
|
|||
manifest = validate_package(package_dir)
|
||||
template_path = safe_relative_file(package_dir, manifest["template"], "template")
|
||||
raw = supplied_values(args.set_values)
|
||||
resolution = resolve_inputs(manifest, raw, composer=CatalogComposer(catalog))
|
||||
composer = CatalogComposer(catalog)
|
||||
resolution = resolve_inputs(manifest, raw, composer=composer)
|
||||
report_repeated_inclusions(resolution)
|
||||
|
||||
template = template_path.read_text(encoding="utf-8")
|
||||
used = set(PLACEHOLDER_RE.findall(template))
|
||||
|
|
@ -1176,7 +1217,9 @@ def cmd_resolve(args: argparse.Namespace) -> None:
|
|||
package_dir, _ = resolve_installed(catalog, args.id, args.version)
|
||||
manifest = validate_package(package_dir)
|
||||
raw = supplied_values(args.set_values)
|
||||
resolution = resolve_inputs(manifest, raw, composer=CatalogComposer(catalog))
|
||||
composer = CatalogComposer(catalog)
|
||||
resolution = resolve_inputs(manifest, raw, composer=composer)
|
||||
report_repeated_inclusions(resolution)
|
||||
|
||||
if args.json:
|
||||
print(
|
||||
|
|
@ -1291,9 +1334,9 @@ def cmd_eval(args: argparse.Namespace) -> None:
|
|||
if spec.get("example")
|
||||
else {}
|
||||
)
|
||||
resolution = resolve_inputs(
|
||||
manifest, values, composer=CatalogComposer(catalog)
|
||||
)
|
||||
composer = CatalogComposer(catalog)
|
||||
resolution = resolve_inputs(manifest, values, composer=composer)
|
||||
report_repeated_inclusions(resolution)
|
||||
missing = sorted(set(PLACEHOLDER_RE.findall(template)) - set(resolution.values))
|
||||
rendered = "" if missing else render_template(template, resolution.values)
|
||||
if missing:
|
||||
|
|
|
|||
|
|
@ -867,3 +867,50 @@ def test_bad_index_format_is_rejected(tmp_path: Path) -> None:
|
|||
(store / "index.yaml").write_text("format: something/else\nentries: []\n", encoding="utf-8")
|
||||
with pytest.raises(cp.CannedPromptError, match="unsupported index format"):
|
||||
cp.read_index(store)
|
||||
|
||||
|
||||
# --- inclusion diamonds (§ 10.4) ---
|
||||
|
||||
def test_no_duplicate_when_inclusions_differ() -> None:
|
||||
resolution = cp.Resolution(
|
||||
values={"a": "alpha text", "b": "beta text"},
|
||||
origins={"a": "included from x/a", "b": "included from x/b"},
|
||||
)
|
||||
assert cp.duplicate_inclusions(resolution) == []
|
||||
|
||||
|
||||
def test_two_inputs_including_the_same_thing_is_flagged() -> None:
|
||||
resolution = cp.Resolution(
|
||||
values={"a": "shared text", "b": "shared text"},
|
||||
origins={"a": "included from x/shared", "b": "included from x/shared"},
|
||||
)
|
||||
assert cp.duplicate_inclusions(resolution) == ["a", "b"]
|
||||
|
||||
|
||||
def test_nested_inheritance_duplication_is_flagged() -> None:
|
||||
"""The common case: an included package inherits an outer input by name."""
|
||||
resolution = cp.Resolution(
|
||||
values={"conventions": "RULES", "routine": "step one\nRULES\nstep two"},
|
||||
origins={
|
||||
"conventions": "included from team/conventions",
|
||||
"routine": "included from team/routine",
|
||||
},
|
||||
)
|
||||
assert cp.duplicate_inclusions(resolution) == ["conventions"]
|
||||
|
||||
|
||||
def test_supplied_values_are_not_flagged() -> None:
|
||||
"""Only inclusions are checked; a caller repeating text is their business."""
|
||||
resolution = cp.Resolution(
|
||||
values={"a": "same", "b": "same"},
|
||||
origins={"a": "supplied", "b": "supplied"},
|
||||
)
|
||||
assert cp.duplicate_inclusions(resolution) == []
|
||||
|
||||
|
||||
def test_empty_inclusion_is_not_flagged() -> None:
|
||||
resolution = cp.Resolution(
|
||||
values={"a": "", "b": "anything"},
|
||||
origins={"a": "included from x/empty", "b": "included from x/b"},
|
||||
)
|
||||
assert cp.duplicate_inclusions(resolution) == []
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ type: workplan
|
|||
title: "Document and detect inclusion diamonds"
|
||||
domain: agents
|
||||
repo: canned-prompts
|
||||
status: proposed
|
||||
status: finished
|
||||
owner: codex
|
||||
topic_slug: practice
|
||||
created: "2026-09-06"
|
||||
|
|
@ -20,7 +20,7 @@ Residual from `CANP-WP-0004` (`origin: residual`, `origin_ref: CANP-WP-0004`).
|
|||
|
||||
```task
|
||||
id: CANP-WP-0005-T01
|
||||
status: todo
|
||||
status: done
|
||||
priority: medium
|
||||
state_hub_task_id: "74234a7a-c3bd-5bf7-a40d-21ddf208cfb5"
|
||||
```
|
||||
|
|
@ -47,3 +47,28 @@ Two questions, and the second depends on the first:
|
|||
purpose.
|
||||
|
||||
Leaning: document it now, warn at validation time, and do not deduplicate.
|
||||
|
||||
## Outcome
|
||||
|
||||
Done as leaned, with one correction found by testing.
|
||||
|
||||
**The documented mechanism was wrong at first.** § 10.4 was written describing a
|
||||
dependency "reached by two paths" through two `include` chains, and the detector
|
||||
counted composer calls to match. Tested against the real case that motivated the
|
||||
task — `helix/repo-advance` — and it did not fire, even though the conventions
|
||||
block still rendered twice.
|
||||
|
||||
The actual mechanism is different and more subtle. `helix/commit-sync` declares
|
||||
its own `conventions` input; when composed, resolution passes the including
|
||||
package's already-resolved values down by name (§ 10.4 inheritance), so it
|
||||
*inherits* the outer text rather than resolving its own include — and renders it
|
||||
a second time. Nothing was included twice. The text appeared twice regardless.
|
||||
|
||||
Both mechanisms are now documented with worked diagrams, and
|
||||
`duplicate_inclusions` detects both: included values that are equal, and an
|
||||
included value contained within another. `resolve`, `render` and `eval` warn;
|
||||
nothing is deduplicated.
|
||||
|
||||
Verified by reintroducing the diamond in a scratch copy of `repo-advance` — it
|
||||
warns — and confirming the shipped factored collection stays silent.
|
||||
Tests 90 → 95.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue