Skip to content

feat: render shared operator surface - #57

Open
any-victor wants to merge 1 commit into
mainfrom
feat/operator-surface
Open

feat: render shared operator surface#57
any-victor wants to merge 1 commit into
mainfrom
feat/operator-surface

Conversation

@any-victor

Copy link
Copy Markdown
Contributor

Why

Qctl still copied mounted invocation/no--- policy across its skill and installed instructions, and its only operator-doc test checked one version line. Command prose could drift from Clap without failing CI.

What

  • Pin ctl-core 0.5.0 for runtime and enable surface only in test builds.
  • Add consumer-owned skill and instruction templates under .ctl/operator/.
  • Render the skill version, mounted examples, no--- policy, and visible command inventories from the real Clap graph.
  • Preserve qctl domain prose and the root -f/--file contract.
  • Replace the version-only assertion with byte-exact committed-render checks for both operator documents.
  • Update contributor guidance, README ownership, and the architecture boundary allow-list.
  • Add a patch changeset for the release lane.

Evidence

  • mise run verify: all unit, integration, doc, operator-render, rustfmt, and denied-warning Clippy checks pass.
  • mise run build: optimized release binary compiles without the test-only Surface/MiniJinja feature.
  • cargo package --allow-dirty --locked: packaged crate compiles from its tarball.
  • cargo tree --edges normal -i minijinja: the release dependency graph contains no MiniJinja.

@kodus-hostinger

kodus-hostinger Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review

Incremental · 1 open

@kody review after new commits. @kody full re-reads the whole PR.

Summary

What these patches actually do

Render pipeline lives in the binary's test tree, not in the build.
src/main.rs gains #[cfg(test)] mod operator_docs;, so the whole MiniJinja/Surface render path is compiled only for cargo test. The release graph keeps ctl-core at default-features = false, features = ["app", "usage"]; the surface feature and minijinja = "=2.24.0" arrive exclusively through [dev-dependencies]. Because Cargo unifies features per-graph, the surface code is present when tests build the lib/bin, but absent from cargo build --release — this is what makes the "no MiniJinja in the release tree" claim hold.

Golden-file semantics with an accept switch.
operator_docs.rs reads the two .ctl/operator/*.jinja sources, renders them through ctl_core::surface::environment() (which supplies the ctl/version.md.jinja, ctl/invocation.md.jinja, and ctl/commands.md.jinja macros), and byte-compares against skills/qctl/SKILL.md and src/instructions.md. UPDATE_OPERATOR_DOCS=1 rewrites the committed files in place before asserting — effectively an insta-style --accept for these two documents, documented in AGENTS.md.

Version coupling is now enforced.
The skill's front-matter version: is rendered from env!("CARGO_PKG_VERSION") via version_line. Any future version bump (including the one this changeset triggers) will fail operator_docs until skills/qctl/SKILL.md is re-rendered. Worth confirming the release lane runs the update step, or the first automated bump after merge breaks CI.

Test coverage moved rather than shrunk.
bundled_skill_names_the_package_version is deleted from tests/cli.rs, but the version assertion is now implied by the byte-exact render. The remaining instructions_prints_the_installed_contract_exactly still compares the binary's stdout to src/instructions.md, so the chain template → committed file → runtime output stays closed end to end.

Content changes in the rendered documents

Both documents pick up a generated ## Commands table sourced from the visible Clap subcommands, and a dedicated ## Invocation section carrying the mounted examples plus the Never mise run q -- rule (previously embedded as step 2 of the skill's numbered workflow). The instructions file's fenced block also gains an sh language hint.

Two behavioral notes on the generated table: the Aliases column renders for every row because no subcommand declares aliases, so it is currently pure overhead; and each row's Purpose is the Clap about string, which means help-text edits now propagate into both operator documents and will fail the byte-exact test until re-rendered — that is the intended coupling, but it makes about strings part of the documentation contract.

One paragraph was dropped from src/instructions.md and not re-homed in the template: the note that mise github: cannot install a binary until a tagged GitHub Release exists, and that operators should fall back to a local or cargo install --git build. The skill's compatibility: line still references the same condition, so the guidance is only partially preserved. If the omission is deliberate (superseded by the generic "call qctl directly" sentence), fine; if not, it belongs in .ctl/operator/instructions.md.jinja.

Boundary and packaging

tests/architecture.rs adds operator_docs.rs to the allow-list of files exempt from the "domain modules return data, ctl-core owns presentation" rule — correct, since the module is presentation-adjacent test scaffolding, though it does mean the file is unchecked by that guard going forward.

The templates live under .ctl/operator/, outside src/. cargo package only builds the tarball, it does not run tests, so the missing templates in the published archive would not surface there. If anyone later runs cargo test from an unpacked crate tarball, crate_file will panic on the missing .ctl/operator/*.jinja paths unless those files are added to include/kept out of exclusion rules.

Files

File Status +
.changeset/operator-surface.md added +5 −0
.ctl/operator/SKILL.md.jinja added +88 −0
.ctl/operator/instructions.md.jinja added +141 −0
AGENTS.md modified +1 −0
Cargo.toml modified +3 −1
README.md modified +5 −0
skills/qctl/SKILL.md modified +31 −7
src/instructions.md modified +25 −8
src/main.rs modified +3 −0
src/operator_docs.rs added +63 −0
tests/architecture.rs modified +4 −1
tests/cli.rs modified +0 −13

Open

File Severity Finding
src/operator_docs.rs high Rendered templates lose the trailing newline, breaking the byte-exact operator-doc comparison and the instructions CLI test.

History

When Pass Open
24 Aug 06:15 UTC Incremental 1

Inspect this review

Comment thread src/operator_docs.rs
Comment on lines +18 to +27
fn render_template(relative: &str, context: &Value) -> String {
let source = crate_file(relative);
let environment = ctl_core::surface::environment()
.unwrap_or_else(|error| panic!("operator environment: {error}"));
environment
.template_from_named_str(relative, &source)
.unwrap_or_else(|error| panic!("parse {relative}: {error}"))
.render(context)
.unwrap_or_else(|error| panic!("render {relative}: {error}"))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Bug high

Trailing-newline mismatch: render() output is byte-compared against the committed file, but MiniJinja strips the template's final newline by default (keep_trailing_newline is false) while skills/qctl/SKILL.md and src/instructions.md end with a newline. Both new tests fail on a clean checkout, and regenerating with UPDATE_OPERATOR_DOCS=1 writes a src/instructions.md without a trailing newline, breaking instructions_prints_the_installed_contract_exactly (tests/cli.rs:526-529); normalize the trailing newline before comparing or writing, or enable keep_trailing_newline on the environment.

Suggested change
fn render_template(relative: &str, context: &Value) -> String {
let source = crate_file(relative);
let environment = ctl_core::surface::environment()
.unwrap_or_else(|error| panic!("operator environment: {error}"));
environment
.template_from_named_str(relative, &source)
.unwrap_or_else(|error| panic!("parse {relative}: {error}"))
.render(context)
.unwrap_or_else(|error| panic!("render {relative}: {error}"))
}
.render(context)
.map(|rendered| format!("{}\n", rendered.trim_end_matches('\n')))
.unwrap_or_else(|error| panic!("render {relative}: {error}"))

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.

1 participant