docs: document summary definitions on custom attestation types - #384
docs: document summary definitions on custom attestation types#384dangrondahl wants to merge 2 commits into
Conversation
Custom attestation types can define a summary: an ordered list of named jq expressions that Kosli renders as labeled rows on the attestation detail page instead of only raw JSON. - getting_started/attestations.md: new "Summaries" subsection in the Custom accordion, covering the CLI flags and the rendering behavior (versioning, render-time evaluation, N/A, URL links, array payloads). - tutorials/attest_custom.md: the tutorial type is now created with --summary entries so the walkthrough ends on a readable summary. - administration/managing_custom_attestation_types/overview.md: summary added to the "Each type can include" list, plus a Terraform example. Refs #364
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
Claude finished @dangrondahl's task in 2m 49s —— View job Doc review — PR #384
Re-reviewed after Verified
FindingsImprovement
Suggestion
Not verified
Overall: ready to merge once finding 1 is settled. The diff also quietly fixes two pre-existing copy-paste bugs in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <Note> | ||
| The summary is part of the versioned type definition, so changing it creates a new version of the | ||
| attestation type — exactly like changing the schema or the jq rules. Removing `summary` from a type | ||
| that had one clears the summary, and its attestations fall back to showing the jq evaluation results | ||
| as a pass/fail checklist. | ||
| </Note> |
There was a problem hiding this comment.
Critical — this contradicts the versioning statement added in getting_started/attestations.md.
Here: "changing it creates a new version of the attestation type ... Removing summary from a type that had one clears the summary, and its attestations fall back to showing the jq evaluation results as a pass/fail checklist."
In getting_started/attestations.md:388-390: "Changing it creates a new version of the type ... Each attestation keeps the version of the type it was reported against."
Both can't be true for pre-existing attestations. If attestations are pinned to the version they were reported against, then removing the summary only affects attestations reported after the change — existing ones keep rendering their summary.
| <Note> | |
| The summary is part of the versioned type definition, so changing it creates a new version of the | |
| attestation type — exactly like changing the schema or the jq rules. Removing `summary` from a type | |
| that had one clears the summary, and its attestations fall back to showing the jq evaluation results | |
| as a pass/fail checklist. | |
| </Note> | |
| <Note> | |
| The summary is part of the versioned type definition, so changing it creates a new version of the | |
| attestation type — exactly like changing the schema or the jq rules. Removing `summary` from a type | |
| that had one clears the summary on the new version, so attestations reported against it fall back to | |
| showing the jq evaluation results as a pass/fail checklist. Attestations reported against an earlier | |
| version keep the summary that version defined. | |
| </Note> |
Worth confirming the actual behavior with engineering before applying — if the detail page renders against the current type version rather than the pinned one, then it's the sentence in attestations.md that needs fixing instead.
There was a problem hiding this comment.
Settled from the implementation rather than by guessing: src/model/attestations_model/attestation.py resolves find_by_id(self.type_id).get_versioned_type(self.type_version).evaluate_summary(...), so the detail page renders against the pinned version, not the current one. The handover doc for kosli-dev/server#6145 lists "render/pinning tests" too.
So the sentence in attestations.md was the correct one, and your suggested rewording of this <Note> is right — applied as written, plus I made the same point explicit in attestations.md: "Each attestation is displayed using the version of the type it was reported against, so editing a summary does not change how existing attestations render."
Same source turned up one thing neither of us had right: link rendering is narrower than "a valid URL". The hardening commit on #6428 requires a string value with an http/https scheme, specifically so non-string values don't 500 and javascript:/data: values can't become clickable — which supersedes the is_valid_url annotation convention the issue mentioned. Both pages now say http:// or https:// instead.
| - So `32 / 1209 * 100 <= 5` evaluates to `2.64 <= 5` which is `true` | ||
|
|
||
|
|
||
| #### Summaries |
There was a problem hiding this comment.
Improvement — this anchor is the target of both new cross-links, but it lives inside a collapsed <Accordion>.
administration/managing_custom_attestation_types/overview.md:20 and tutorials/attest_custom.md:40 both point at /getting_started/attestations#summaries. Mintlify <Accordion> is collapsed unless defaultOpen is set, so a reader following either link may land on the page with the content still hidden and no scroll target — the id existing in the DOM isn't sufficient if the accordion doesn't auto-expand on hash navigation.
Please verify in mint dev by loading http://localhost:3000/getting_started/attestations#summaries as a fresh navigation (not by clicking around after the accordion is already open). If it doesn't expand:
- add
defaultOpento the Custom accordion, or - promote the summaries content to a real
###section outside the<AccordionGroup>.
Separately: this is the only #### in the file and it follows ## Attestation types at line 253, so it skips ###. Since the surrounding accordions use no headings at all, ### would fit the document outline better.
There was a problem hiding this comment.
Confirmed, and worse than a maybe. I loaded the page under headless Chrome with #summaries as a fresh navigation and dumped the post-hydration DOM: every accordion still reported aria-expanded="false", byte-identical to a load with no hash. The accordion does not auto-expand, so both cross-links were landing readers on collapsed content.
Took your second option rather than defaultOpen, since defaultOpen would leave the Custom accordion permanently open while its siblings stay shut, and this content is the main documentation for the feature — it deserves a TOC entry. It's now a top-level ## Summarizing custom attestations section after the </AccordionGroup>, with a two-line pointer left inside the Custom accordion. That also removes the ####-skipping-### problem you flagged. Both cross-links updated to #summarizing-custom-attestations; mint broken-links is clean apart from the pre-existing /getting_started/service-accounts in working_with_controls.mdx.
| - A **summary** (optional) — ordered, labeled jq expressions that Kosli renders as rows on the attestation detail page | ||
|
|
||
| At least one of the two must be provided. | ||
| At least one of the schema and the jq rules must be provided. The summary is independent of both: |
There was a problem hiding this comment.
Suggestion — "one of the schema and the jq rules" reads awkwardly.
| At least one of the schema and the jq rules must be provided. The summary is independent of both: | |
| At least one of the schema or the jq rules must be provided. The summary is independent of both: |
There was a problem hiding this comment.
Applied as suggested.
| The two `--summary` entries are optional, but they're worth adding: each one is a | ||
| `'NAME=EXPRESSION'` pair whose [jq expression](https://jqlang.org/manual/) is evaluated against the | ||
| attestation data, and Kosli renders the results as labeled rows on the attestation detail page. So | ||
| the attestation you report below opens on a readable `Coverage: 92` / `Tool: pytest-cov` summary | ||
| instead of only the raw JSON. See | ||
| [Summaries](/getting_started/attestations#summaries) for the details, and | ||
| [`kosli create attestation-type`](/client_reference/kosli_create_attestation-type) for the | ||
| `--summary-json` alternative. |
There was a problem hiding this comment.
Improvement — Diátaxis: this is explanation inside a tutorial step.
Eight lines and three outbound links explaining an optional flag, at step 1 of a tutorial whose goal is "report a custom attestation". Tutorials should keep the learner moving and defer the "why" to a single link. The mechanics ('NAME=EXPRESSION', jq evaluation, --summary-json) are already in getting_started/attestations#summaries and client_reference/kosli_create_attestation-type.
| The two `--summary` entries are optional, but they're worth adding: each one is a | |
| `'NAME=EXPRESSION'` pair whose [jq expression](https://jqlang.org/manual/) is evaluated against the | |
| attestation data, and Kosli renders the results as labeled rows on the attestation detail page. So | |
| the attestation you report below opens on a readable `Coverage: 92` / `Tool: pytest-cov` summary | |
| instead of only the raw JSON. See | |
| [Summaries](/getting_started/attestations#summaries) for the details, and | |
| [`kosli create attestation-type`](/client_reference/kosli_create_attestation-type) for the | |
| `--summary-json` alternative. | |
| The `--summary` entries are optional. Each is a `'NAME=EXPRESSION'` pair, so the attestation you | |
| report below opens on a readable `Coverage: 92` / `Tool: pytest-cov` summary instead of raw JSON — | |
| see [Summaries](/getting_started/attestations#summaries). |
Also: line 25 still describes this as "a minimal coverage-report type", which no longer quite matches now that it carries two summary entries.
There was a problem hiding this comment.
Applied your condensed version verbatim, and changed "a minimal coverage-report type" to "a small coverage-report type" on line 25.
| ```bash | ||
| kosli create attestation-type coverage-metrics | ||
| --jq=".code.lines.missed / .code.lines.total * 100 <= 5" | ||
| --summary="Lines missed=.code.lines.missed" | ||
| --summary="Lines total=.code.lines.total" | ||
| ``` |
There was a problem hiding this comment.
Suggestion — the command isn't copy-pasteable: no line continuations.
Four lines with no trailing \, so pasting this into a shell runs kosli create attestation-type coverage-metrics and then three "command not found" errors. This matches the existing block at lines 331-334 (same pre-existing flaw), and the tutorial at tutorials/attest_custom.md:28-33 gets it right — worth fixing both blocks in this accordion while you're here.
| ```bash | |
| kosli create attestation-type coverage-metrics | |
| --jq=".code.lines.missed / .code.lines.total * 100 <= 5" | |
| --summary="Lines missed=.code.lines.missed" | |
| --summary="Lines total=.code.lines.total" | |
| ``` | |
| ```bash | |
| kosli create attestation-type coverage-metrics \ | |
| --jq=".code.lines.missed / .code.lines.total * 100 <= 5" \ | |
| --summary="Lines missed=.code.lines.missed" \ | |
| --summary="Lines total=.code.lines.total" | |
| ``` |
Minor, in the same area: this re-declares coverage-metrics, which was already created without a summary at line 332. A half-sentence noting that re-running the command updates the type (creating a new version) would prevent a reader wondering whether they now have two types.
There was a problem hiding this comment.
Applied, and fixed both blocks in the accordion — the kosli attest custom block just below at 336-342 had the same problem. Worth noting the rest of the page already gets this right (e.g. lines 50-56 use \ with a trailing ...), so these two were the outliers.
Also added your half-sentence on re-declaration: "Re-running kosli create attestation-type for a name that already exists updates that type rather than creating a second one — see the note on versioning below."
- Move the summaries content out of the collapsed "Custom" accordion into a top-level "Summarizing custom attestations" section. Verified with headless Chrome that a cold load of the in-accordion anchor left every accordion at aria-expanded="false", so the deep link both cross-links rely on landed on hidden content. Also fixes the ####-after-## skip. - Fix the versioning contradiction: attestation.py resolves get_versioned_type(self.type_version) before evaluating, so a rendered attestation uses the type version it was reported against. Both pages now say that, and the Terraform note scopes the fallback to the new version. - Narrow the URL claim: link rendering requires a string value with an http/https scheme (kosli-dev/server#6428 hardening), not "any valid URL". - Trim the tutorial paragraph to one sentence plus one link, and drop "minimal" now that the type carries summary entries. - "At least one of the schema or the jq rules" (was "and"). - Add \ line continuations to the two command blocks in the Custom accordion, and note that re-running create updates an existing type. Refs #364
|
All five inline threads answered and pushed (b5e...see latest commit). Replying here to the one finding that had no inline anchor, plus what came out of verifying the rest. The two substantive items were both real. The versioning contradiction resolved in favor of the The anchor concern was not a maybe — a headless-Chrome load of Checking those also caught something the review didn't: "a value that is a valid URL renders as a link" was too broad. The hardening commit on kosli-dev/server#6428 requires a string value with an On the "reference material in a getting-started page" finding — I mostly disagree, and changed nothing for it. Of the three overlaps named: URL-as-link is not in The drift risk is worth naming, though: the generated CLI page is regenerated from the CLI repo, so if the flag semantics change the hand-written section won't follow. That's an argument for keeping the flag mechanics thin here rather than for trimming the behavior, which is what I've done. Also noted and agreed on the British "labelled" in the generated pages — upstream fix, out of scope here. |
| than as a broken detail page later. | ||
| - An expression that returns `null`, or that fails against a particular attestation's data, renders | ||
| as `N/A`. The rest of the summary still renders. | ||
| - A value that is a string beginning with `http://` or `https://` renders as a clickable link, as |
There was a problem hiding this comment.
Improvement — three different descriptions of the same linkification rule.
This says a value linkifies when it's "a string beginning with http:// or https://", and asserts it works "as annotation values do" — but line 246 of this same file says annotation values linkify when they are "valid URLs", and terraform-reference/resources/custom_attestation_type.mdx:189 says "values that are valid URLs render as links". overview.md:83 repeats the narrow phrasing.
The two rules differ for real inputs (ftp://…, mailto:…, a bare example.com), so one of them is wrong. Pick the accurate one and use it in all three places:
- if the scheme check is the real behavior, line 246 and the Terraform provider description need updating (the latter upstream in the provider repo, since that page is generated);
- if "valid URL" is the real behavior, this line and
overview.md:83should say that.
| } | ||
| ``` | ||
|
|
||
| ### With a summary |
There was a problem hiding this comment.
Suggestion — this heading reads as a fourth mutually-exclusive option.
The three preceding ### headings under Create a custom attestation type enumerate the alternatives of one choice: With schema and jq rules / With jq rules only / With schema only. With a summary is orthogonal — it combines with any of the three, and the example below in fact pairs it with jq rules only.
The body text at line 18-21 makes the independence clear, but a reader scanning the right-hand ToC sees four peers and infers a fourth alternative. Consider ### Adding a summary (verb phrase breaks the "With …" series), or promoting it to its own ## after the three shape variants.
1 participant
Closes #364
What
Documents
summaryon custom attestation types — the ordered list of named jq expressions that Kosli renders as labeled rows on the attestation detail page instead of only raw JSON.The issue was filed when
summarywas API-only and recommended holding back the authoring instructions. Both surfaces have since shipped (--summary/--summary-jsononkosli create attestation-type, and thesummaryattribute onkosli_custom_attestation_type), so all four open tasks are covered.Changes
getting_started/attestations.md— new#### Summariessubsection in the Custom accordion: what a summary is, the--summaryCLI flags, the JSON form, and the behavior worth knowing (part of the versioned type definition, evaluated at display time, invalid jq rejected at creation,N/Afallback, URLs render as links, one summary group per element for array payloads).tutorials/attest_custom.md— step 1 now creates thecoverage-reporttype with--summary "Coverage=.coverage"/--summary "Tool=.tool", so the walkthrough ends on a readable summary rather than raw JSON.administration/managing_custom_attestation_types/overview.md—summaryadded to the "Each type can include" list (and the "at least one of the two" sentence reworded accordingly), plus a With a summary Terraform example usingjsonencode()and thefile()variant.No new pages, so no
config/navigation.jsonchange.Verification
mint broken-links— the only reported link is the pre-existing/getting_started/service-accountsintutorials/working_with_controls.mdx, untouched here.mint dev: they compile, and the new#summariesheading inside the accordion does get an anchor (id="summaries"), which the tutorial and Terraform page link to.