Skip to content

Add configuration template catalog and renderer - #150

Merged
CoestPiper merged 3 commits into
mainfrom
CoestPiper/issue-148
Oct 5, 2026
Merged

CoestPiper merged 3 commits into
mainfrom
CoestPiper/issue-148

Conversation

@CoestPiper

@CoestPiper CoestPiper commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds config_template as the shared catalog and renderer for named first-install configurations. It exposes English and Korean metadata, the set of components requiring a template, and exact lookups by component and id. The catalog starts empty, with reconverge requiring a template; its first entry follows in #149.

The renderer recursively substitutes five host-value tokens in TOML string values, rejects placeholders in keys, unknown or partial placeholders, and missing host values, and returns deterministic TOML serialization. Supplied values round-trip verbatim without being re-scanned.

Adds renderer and lookup tests, catalog invariant checks, and file-correspondence checks. Documents the interim catalog, secret-free templates, immutable content per id, and the deliberate exception to the crate’s product-neutral scope. Updates the README, crate documentation, AGENTS.md, and changelog.

Closes #148

Deviations from the issue

None

Test plan

  • Verify all five tokens substitute throughout nested tables, arrays, arrays of tables, and inline tables, regardless of TOML quoting style; untouched values retain their types.
  • Verify missing host values, unknown and partial placeholders, placeholders in keys, and invalid TOML return the expected typed errors.
  • Verify empty bodies and token-free templates render without host values, while supplied empty strings remain valid values.
  • Verify escaping round-trips host values verbatim, substitution is single-pass, output equals the table’s serialization, rendering is deterministic, and the first violation follows table iteration order.
  • Verify errors do not expose previously substituted host values.
  • Verify token text and Display, exact case-sensitive lookups, fixture catalog order, and that only reconverge requires a template. Catalog invariant and file-correspondence tests apply to future entries without requiring test edits.
  • Verify catalog invariants reject invalid ids, duplicates, unsupported components, invalid bodies, and blank localized texts; accept 63-byte ids and trailing hyphens; check real entries against their named files.
  • cargo fmt -- --check --config group_imports=StdExternalCrate
  • cargo clippy --all-targets -- -D warnings
  • cargo clippy --all-targets --features test-support -- -D warnings
  • RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --features test-support
  • cargo test
  • cargo test --features test-support
  • Linux aarch64 Platform job: cargo test --features test-support
  • macOS Platform job: cargo build --all-targets --features test-support
  • Markdown lint over **/*.md using .markdownlint-cli2.yaml
  • Agent-instructions drift check

Manager and host agent need one template contract for first-install
configuration, including missing host values and immutable template ids.
Keep the initial catalog empty until product-owned content is supplied,
while requiring a template for reconverge installs.

Closes #148
Cover supplied empty strings and errors after substitution so callers
can rely on verbatim host values without exposing them in diagnostics.

Part of #148
@CoestPiper CoestPiper changed the title Add configuration templates and renderer Add configuration template catalog and renderer (#148) Oct 5, 2026
@CoestPiper

Copy link
Copy Markdown
Contributor Author

[Reviewer Round 1]

The PR delivers the issue in full and the renderer is correct, but one test should be fixed before merge: it pins the catalog as empty, so the next entry breaks it even though the module docs say nothing else needs to change.

Verdict: request changes. This is a small change: remove three assertions.

What the PR delivers

origin/main...HEAD touches src/config_template.rs (new), src/lib.rs, README.md, AGENTS.md and CHANGELOG.md. No other module changes, and no dependency is added or changed.

  • Public surface. Exactly the nine items in Scope 2–5, in the specified shapes. CATALOG, REQUIRES_TEMPLATE, the *_in helpers and check_catalog (which is #[cfg(test)]) are all private.

  • Renderer (src/config_template.rs:215-270). It meets every rule in Scope 5:

    • It parses as toml::Table and walks tables, inline tables, arrays and arrays of tables.
    • It rejects any key containing ${.
    • It substitutes only when a string is exactly one of the five tokens. A missing host value is UnresolvedToken; any other ${ is UnknownPlaceholder.
    • It substitutes in one pass: after clone_into, the substituted string never reaches the else if scan.
    • It returns toml::to_string and nothing else.

    Because of the single pass, the echoed key/value in an error is always template text and never a host value. The test at :559-576 pins that down.

  • Error messages follow the crate's existing RenderError / ModuleSpecError style ({value:?}, {0} alongside #[source]).

  • Lookups are exact and case-sensitive, and are tested against a fixture catalog through the private helpers.

  • Catalog checker (:310-353) enforces all five invariants. The id check is hand-written and does not reuse is_dns_label, so a trailing hyphen is accepted.

    • Each negative fixture changes one field of an ENTRY that passes on its own, so a bare is_err() still pins the intended invariant.
    • The file-correspondence test covers the real catalog.
  • Test plan items 1–10 are all present and test real behaviour. Item 1 also covers every TOML quoting style, including a \u0024 escape. The tests go further than the plan in places: supplied empty strings are not treated as absent, errors do not leak host values, first-violation order is checked including nested values, and ids 한글 / a\n are rejected.

  • Docs and changelog.

    • The module docs cover purpose, the interim status, both consumers, how to add an entry, the tokens and render rules, and both review-enforced policies.
    • lib.rs, README.md and AGENTS.md (outside the shared blocks) name the exception and keep the rule for everything else.
    • The README module list gains the bullet in the right place.
    • CHANGELOG.md gets ### Added above ### Changed under the existing [Unreleased], with no issue reference.

Findings

1. A test pins the catalog as empty, contradicting the documented add-an-entry procedure (should fix)

src/config_template.rs:674-685:

assert_eq!(CATALOG, []);
assert!(requires_template("reconverge"));
assert_eq!(templates_for("reconverge").count(), 0);
assert!(find("reconverge", "default").is_none());

The module docs written in this PR (:10-12) say an entry is "one TOML file … and one element of CATALOG … nothing else is needed". The issue says the same of #149: "The first entry is a separate one-file change".

Once #149 adds reconverge's first entry, assert_eq!(CATALOG, []) and templates_for("reconverge").count() == 0 fail. If that entry's id is default, the find assertion fails too. Whoever adds the entry then has to edit a test the docs said they would not need to touch. #149 is likely to be an AgentCoop run working only from its issue text, so it will hit this with no context for why.

The acceptance criterion "the catalog is empty …" describes the state this PR ships. It is not an invariant for a test to lock in; the test plan only asks that, on the real catalog, requires_template be true for reconverge alone.

Fix: delete the three reconverge-specific emptiness assertions. Keep requires_template("reconverge") and the loop over piglet/giganto/hog/crusher/Reconverge/"", which stays true after entries are added. check_catalog already binds every future entry.

2. PR hygiene (nits)

  • The body says Closes #148 twice: as Closes #148. at the end of the summary, then again on its own line. Keep one.
  • The title ends in (#148). A squash merge would carry that into the commit title, which the repository rules forbid in commit titles; GitHub then appends (#150) as well. Drop it from the title.
  • ## Deviations from the issue: None is accurate. Every difference I found is a harmless addition (#[must_use = "…"] with a message, which avoids clippy::double_must_use on impl Iterator; the private requires_template_in; the extra tests).

3. Minor observations (no change needed)

  • returns_first_violation_in_table_iteration_order (:605-623) checks iteration order before asserting on it (table.keys().next() == "a"). It therefore holds only while toml's preserve_order feature is off, which the issue's constraint guarantees for this crate. A dependent that turns that feature on through feature unification would get a different key order. The issue explicitly leaves key order outside the contract, so this is fine. I mention it only because roxyd's output order is not under this crate's control.
  • The edited crate-doc lines in src/lib.rs (:31-32) run to 82–85 columns. The file already has several lines over 80, so this is consistent with existing style.

Once the three assertions in finding 1 are removed (and ideally the PR body and title tidied), I would approve.

@CoestPiper

Copy link
Copy Markdown
Contributor Author

[Review Verdict Round 1: NOT_APPROVED]

@CoestPiper CoestPiper changed the title Add configuration template catalog and renderer (#148) Add configuration template catalog and renderer Oct 5, 2026
The catalog starts empty but is intended to grow without changing its
lookup tests. Keep the required-component checks while leaving catalog
contents to the invariant and file-correspondence tests.

Part of #148
@CoestPiper

Copy link
Copy Markdown
Contributor Author

[Author Round 1]

Pushed commit 9abaa14 to PR #150.

  • Fixed: Removed the three catalog-emptiness assertions and renamed the test.
  • Fixed: Removed the duplicate closing reference and issue suffix from the PR title; updated the description. Deviations remain None.
  • Minor observations: No changes needed, as the reviewer stated.

All required local checks and the new CI run passed. No PR comments or checkpoint markers were posted.

@CoestPiper

Copy link
Copy Markdown
Contributor Author

[Reviewer Round 2]

[Reviewer Round 2]

Verdict: approve.

Round 1 items

  1. The test pinned the catalog as empty (should fix). Resolved. Commit 9abaa14 removes assert_eq!(CATALOG, []), templates_for("reconverge").count() == 0 and find("reconverge", "default").is_none(). It also renames the test to only_reconverge_requires_a_template (src/config_template.rs:675). The assertions that remain stay true after Add reconverge's default configuration template #149 adds an entry:

    • requires_template("reconverge") holds.
    • For piglet, giganto, hog, crusher, Reconverge and "", the test checks that requires_template is false, templates_for yields nothing, and find(component, "default") is None (:677-681). Invariant 3 in check_catalog already forbids an entry for a component outside the required set, so these hold for any catalog the checker accepts.

    No other test reads the real CATALOG except check_catalog(CATALOG) (:686) and the file-correspondence loop (:691), and both are meant to bind future entries. Adding an entry is now the one TOML file plus one CATALOG element that the module docs describe.

  2. PR hygiene (nits). Resolved.

    • The title is now "Add configuration template catalog and renderer", with no issue number.
    • The body carries Closes #148 once, on its own line, and no other Part of or closing references.
    • ## Deviations from the issue: None is still accurate.
  3. Minor observations. No change was needed, and none was made.

New findings

None. The only change since round 1 is the test edit above. The rest of the diff (src/config_template.rs, src/lib.rs, README.md, AGENTS.md, CHANGELOG.md) is unchanged from what I reviewed in round 1, and my assessment there stands:

  • The public surface matches Scope 2–5 exactly.
  • The renderer meets every rule in Scope 5, including the single pass and the error messages that never echo a host value.
  • The catalog invariants are enforced and proven able to fail by the negative fixtures.
  • The documented policies, the "no product concept" exception, the README bullet and the changelog entry are all in place.
  • No dependencies changed.

The commit message follows the repository rules: imperative title under 50 characters, no prefix, no issue number in the title, and Part of #148 in the body.

@CoestPiper

Copy link
Copy Markdown
Contributor Author

[Review Verdict Round 2: APPROVED]

@CoestPiper

Copy link
Copy Markdown
Contributor Author

Suggested squash commit

Title

Add configuration template catalog and renderer

Body

Manager and host agent need one contract for named first-install
configuration so template selection and host-value substitution agree.
Keep the catalog empty until product-owned content is supplied, while
requiring a template for reconverge installs.

Reject invalid placeholders and missing host values, and preserve supplied
values verbatim without exposing them in errors. Document secret-free
templates and immutable content per id. Test rendering and catalog
invariants without preventing future entries.

Closes #148

@CoestPiper
CoestPiper merged commit c77d17c into main Oct 5, 2026
5 checks passed
@CoestPiper
CoestPiper deleted the CoestPiper/issue-148 branch October 5, 2026 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add the configuration template catalog and its renderer

1 participant