Repository navigation
Conversation
…e cap Rank related-doc candidates by section (primary docs' directory, then keyword-named sections, then filename-only matches) before applying the six-entry cap, print the dropped paths per route to stderr, and regenerate site/llm/chatbot-product-map.md. Adds tests for the ranking, the drop report, and the workflow routes keeping manage-record-stages.
… installation pages Without a keyword for the risk-tiering directory, the section ranking put /guide/risk-tiering/manage-risk-tier-templates.html in the filename-only tier and the Risk Tier Templates settings route lost its own guide to document-template pages. /settings/risk-tier-stages also had no related docs at all. Add "tier" -> risk-tiering so both routes list the risk-tiering guides first, with a regression test. Regenerate the map with site/installation populated the way CI does, which restores the section list for the SSO installation page.
juanmleng
left a comment
There was a problem hiding this comment.
Review: #1600 at ee6e908 (merge-base 8ed2521)
Computed by the calibrated-review skill, run by @juanmleng.
Merge with recommendations · no blocking · 2 small fixes · 1 question for you
The ranking fix works: the three workflow routes keep manage-record-stages, and the new tier keyword matches only the two risk-tier routes.
Small fixes: F1 run the generator tests on guide-only PRs (today they're skipped, so the next eviction is auto-committed and fails a later, unrelated PR), F2 drop the brittle first-position assertion in the risk-tier test.
❓ @emmavdh — should new evictions show in the PR diff? Options: commit the dropped list as a sidecar file, or warn only when a route's dropped list changes.
Fix later: F4 alphabetical tiebreak inside a section.
calibrated-review v2 · review · run by @juanmleng · head ee6e908
Agent handoff · calibrated-review v2 · review · ee6e908
calibrated-review: 2
mode: review
pr: validmind/documentation#1600
head: ee6e9086ef83e10bd04acac2cee4a301242f7645
merge_base: 8ed252152bf3f99e1ccb57d3dcb6638e5540968f
run_by: juanmleng
verdict: merge-with-recs
downgraded: 1 # F1 fails gate 2: the eviction needs a future guide PR
items:
- id: F1
class: fix-now
type: correctness
direction: risky
title: Generator tests are skipped on guide-only PRs, so the regression test fires on the wrong PR
scenario: >-
A PR adds site/guide/workflows/approve-workflow-steps.qmd and
archive-workflows.qmd. The preview selector marks it targeted, so the
"Test chatbot product map generator" step is skipped. The map step
regenerates the map without manage-record-stages on the three workflow
routes and the bot auto-commits it. The PR merges green. The next PR that
runs full validation fails test_workflow_routes_keep_manage_record_stages
for an eviction it did not cause. The PR body says the regression test
will flag this case.
evidence:
- .github/workflows/validate-docs-site.yaml:419 gates the test step on targeted != 'true'
- .github/workflows/validate-docs-site.yaml:386 map step is ungated and auto-commits the regenerated map
- .github/scripts/select_docs_preview_targets.py exits 0 (targeted) for "added site/guide/workflows/archive-workflows.qmd"
- with the two pages added, test_workflow_routes_keep_manage_record_stages fails for all three routes
fix: >-
Remove the targeted gate from the test step (no render needed, ~0.1 s)
and run it before the map auto-commit, so a failing test stops the
commit. If that is out of scope, correct the PR body's claim.
done_when: a guide-only PR runs the generator tests before the map is committed
- id: F2
class: fix-now
type: hygiene
direction: safe
title: Risk-tier regression test pins the alphabetically first page
scenario: >-
Adding any risk-tiering guide whose name sorts before "configure-"
(about-..., assign-...) fails
test_risk_tier_templates_route_keeps_risk_tiering_guide although the
route still lists its own guides.
evidence:
- site/scripts/test_generate_chatbot_product_map.py:178 assertEqual(related[0], ".../configure-risk-tier-calculation.html")
- the assertIn on the line above already covers the docstring's intent
fix: >-
Delete the related[0] assertion, or assert that the leading entries are
all under /guide/risk-tiering/.
done_when: the test asserts membership or section, not alphabetical position
- id: F3
class: question
type: risk-appetite
direction: safe
title: The drop report is not visible during review
scenario: >-
The report goes to stderr in a CI step that passes, and it lists all 20
routes the cap already trims on every run. A new eviction looks the same
as the standing ones, and a reviewer reading the PR diff does not see it.
The story asked for the eviction to show during review.
evidence:
- site/scripts/generate_chatbot_product_map.py:795 prints the full report to stderr
- regenerated at head, the report lists 20 routes
fix: >-
Owner @emmavdh decides: commit the dropped list as a sidecar file next to
the map (kept out of the LLM corpus) so evictions appear in the diff, or
emit a ::warning:: only for routes whose dropped list changed.
- id: F4
class: fix-later
type: scope-expansion
direction: safe
title: Within a section, alphabetical order still picks the six
scenario: >-
On routes whose own section has more than six matching pages (workflows,
inventory, attestation templates), path order decides what the assistant
sees. The author notes this under special review.
fix: >-
Ticket: add a small per-route pin list or rank by keyword match in the
page title, so the canonical how-to for a screen is chosen on purpose.
accepted_risks:
- checked: removal of "route.related_docs or" in the render loop
why: route.related_docs is only ever assigned from suggest_related_docs, so the output is unchanged
- checked: the "tier" keyword is a substring match
why: across the frontend snapshot it matches only /settings/risk-tier-templates and /settings/risk-tier-stages
- checked: FAQ pages losing their slot on crowded routes
why: disclosed by the author under special review, and it shows in the drop report
verification:
- run: python3 -m unittest discover -s site/scripts -p 'test_generate_chatbot_product_map.py'
result: 16 passed at head
- run: python3 site/scripts/generate_chatbot_product_map.py
result: one-line diff on the installation page; site/installation not populated locally
- run: add approve-workflow-steps.qmd and archive-workflows.qmd under site/guide/workflows, rerun the tests
result: 3 subtest failures in test_workflow_routes_keep_manage_record_stages
- claim: "Before and after" route table (PR body), workflow rows
result: matches the regenerated map
- claim: "Before and after" route table (PR body), other route swaps
result: NOT CHECKED
- claim: "the drop report and the regression test will flag that" (PR body)
result: false for guide-only PRs, see F1
Validate docs site✓ INFO: A live preview of the docs site is available — Open the preview |
Lighthouse check results✓ INFO: No site pages to audit in this PR. Commit SHA: ee6e908 |
cachafla
left a comment
There was a problem hiding this comment.
Checked head ee6e908: related docs are now ranked by section (the primary doc's own section first, then sections a keyword names, then docs that match only by filename) before the six-entry cap. Docs the cap drops are reported on stderr. The regenerated map looks right: manage-record-stages stays on the workflow routes, and risk-tiering guides are no longer listed under the document settings routes. CI is green, and all 16 tests in test_generate_chatbot_product_map pass locally.
…st on every PR Review follow-ups from Juan: - Break ties inside a tier by position in the docs sidebars instead of path, so each route keeps the pages docs writers list first. Pages missing from the sidebar sort after listed ones. - Write the related docs the cap leaves out to site/llm/chatbot-product-map-dropped.md instead of stderr. The file is committed next to the map, so a new eviction shows in the PR diff. It is not copied into the LLM corpus. - Run the generator tests on every PR, targeted previews included, and before the map auto-commit, so an eviction fails the PR that caused it. Both CI checks now include the dropped file. - The risk-tier regression test checks section order instead of a fixed first page. The workflow regression test explains what to do when it fails.
|
Thanks @juanmleng, this was useful. All four are addressed in 5ae1769. F1: Agreed, and I reproduced it. The generator tests now run on every PR, targeted previews included, and before the map auto-commit, so an eviction fails the PR that caused it. F2: Done. The risk-tier test checks that the route keeps F3: Yes, I want evictions in the diff. The generator now writes the dropped list to F4: I folded this into the PR rather than a follow-up. Inside a tier, ties now break on position in the docs sidebars before path, so each route keeps the pages docs writers list first. Pages that aren't in the sidebar sort last. With your two example pages added, nothing is evicted unless they're placed ahead of |
juanmleng
left a comment
There was a problem hiding this comment.
Thanks Emma — checked 5ae1769: tests now run on every PR before the map auto-commit, the risk-tier test checks order by section, the dropped list is committed and stays out of the LLM corpus, and sidebar order handles the tiebreak. All four items resolved, nothing new from me.
The "integrations" keyword mapped to both /guide/integrations/ and /guide/configuration/. The longer "configuration" segment won on specificity, so Connections, Secrets and Data exports listed organization setup pages and none of their own guides. This was already the case on main. Map "integrations" to /guide/integrations/ only, and add keywords for connections, secrets and data-exports so each screen lists its own guide first. Only those three routes change. Adds a regression test.
|
One more change in 15c9e77. The three integrations settings screens (Connections, Secrets, Data exports) listed only |
Pull Request Description
What and why?
site/scripts/generate_chatbot_product_map.pylists at most six related docs per route insite/llm/chatbot-product-map.md. The ranking scored candidates by the length of the matched keyword segment, but on the workflow routes every candidate matches the sameworkflowssegment, so they all tied and alphabetical path order picked the six. Pages from other sections that only mention workflows in their filename (/faq/,/guide/integrations/,/guide/monitoring/,/guide/risk-tiering/) sorted ahead of most of/guide/workflows/, and/guide/workflows/manage-record-stages.htmldropped out of the map entirely. Nothing reported what the cap left out.This PR changes the ranking and makes evictions visible:
/guide/workflows/for/workflows), then pages that only match a keyword in their filename.site/guide/_sidebar.yamland the other sidebar files), then path. Docs writers already order each sidebar section on purpose, so a route keeps the overview and setup pages listed first. Pages missing from the sidebar sort after the listed ones, so adding a page no longer evicts anything unless it is placed ahead of it in the sidebar.site/llm/chatbot-product-map-dropped.md, committed next to the map. A new eviction shows up as a changed line in the PR diff. The file is not copied into the LLM corpus (llm/render.shcopies only the map, andsite/llm/is its own Quarto project, so it is not published either). The cap stays at six, now theRELATED_DOCS_CAPconstant.RELATED_DOC_KEYWORDSgains"tier": ["risk-tiering"]. Without it,/settings/risk-tier-templateslost its own guide to document-template pages, and/settings/risk-tier-stageshad no related docs at all."integrations"now maps to/guide/integrations/only, plus newconnections,secretsanddata-exportskeywords. It used to map toconfigurationas well, and the longer segment won on specificity, so the three integrations settings screens listed organization setup pages and none of their own guides (also the case on main).python3 site/scripts/generate_chatbot_product_map.py(the CI command), withsite/installationpopulated the way CI does it. The frontend snapshot is unchanged.site/llm/README.mddescribes the ranking and the new file.Before and after for the related docs on the three workflow routes:
/settings/workflows(primary:setting-up-workflows)/faq/faq-workflows.html/guide/integrations/integrations-examples/use-webhooks-with-workflows.html/guide/monitoring/trigger-workflows-on-threshold-breach.html/guide/risk-tiering/set-up-risk-tiering-workflows.html/guide/workflows/assign-workflow-executions.html/guide/workflows/conditional-step-requirements.html/guide/workflows/introduction-to-workflows.html/guide/workflows/workflow-step-types.html/guide/workflows/conditional-step-requirements.html/guide/workflows/workflow-states.html/guide/workflows/manage-record-stages.html/guide/workflows/configure-workflows.html/settings/workflow-states(primary:workflow-states)/guide/workflows/introduction-to-workflows.html/guide/workflows/workflow-step-types.html/guide/workflows/conditional-step-requirements.html/guide/workflows/setting-up-workflows.html/guide/workflows/manage-record-stages.html/guide/workflows/configure-workflows.html/workflows(no primary)/guide/workflows/introduction-to-workflows.html/guide/workflows/workflow-step-types.html/guide/workflows/conditional-step-requirements.html/guide/workflows/workflow-states.html/guide/workflows/setting-up-workflows.html/guide/workflows/manage-record-stages.htmlPrimary docs are unchanged on every route.
Other routes that change, compared with main:
setting-up-the-inventory, record types, fields, segments, record intake, working with the inventory) in place of the record-level pages (archive, interdependencies, layout, overview page, editing, import) and the inventory FAQ./settings/block-library,/settings/document-types,/settings/documents/general,/settings/templates) drop/guide/risk-tiering/manage-risk-tier-templates.htmland the document checker page and pick up the templates overview pages (working-with-document-templates,managing-documents)./settings/risk-tier-templatesand/settings/risk-tier-stageslist the five/guide/risk-tiering/guides first./settings/attestation-templatesswaps/faq/faq-attestations.htmlfor/guide/templates/working-with-document-templates.html./settings/integrations/connections,/secretsand/data-exportsnow list their own guide first (configure-connections,manage-secrets,configure-data-exports), then the other/guide/integrations/pages, in place of six/guide/configuration/pages./analytics,/attestationsand/settings/attestationsonly reorder.How to test
From the repo root, with
site/installationpopulated (CI copies it from validmind/installation; locallymake -C site clone copy-installation), otherwise the SSO installation page loses its section list:python3 site/scripts/generate_chatbot_product_map.py git diff --exit-code -- site/llm/chatbot-product-map.md site/llm/chatbot-product-map-dropped.md python3 -m unittest discover -s site/scripts -p 'test_generate_chatbot_product_map.py' -vThe diff should be empty and all 19 tests should pass. New tests cover same-section pages outranking cross-section filename matches, keyword-section pages outranking filename matches on routes without primary docs, sidebar order breaking ties inside a section, reading the sidebar while skipping commented-out entries, the dropped file listing evicted paths, a regression check that the three workflow routes keep
manage-record-stages.html, and a regression check that/settings/risk-tier-templateslists its risk-tiering guides ahead of other sections, and a regression check that the three integrations screens list their own guide and only/guide/integrations/pages.What needs special review?
faq-workflows,faq-inventory,faq-attestationson/settings/attestation-templates). This is intended. They are listed in the dropped file.site/guide/_sidebar.yamlcan now change the map: moving a page up a section can push another page off a route. The dropped file shows it in the diff, and formanage-record-stageson the workflow routes the regression test fails with a message pointing at the sidebar.test_workflow_routes_keep_manage_record_stages, the fix is a sidebar or ranking decision, not a code bug in that PR./guide/configuration/pages such as private connectivity and organization setup. If any of those belong there, they need their own keyword.Dependencies, breaking changes, and deployment notes
Changes
.github/workflows/validate-docs-site.yaml(generator tests run on every PR, before the map auto-commit) and.github/workflows/full-docs-validation.yaml(tests run first, and the dropped file is checked). No deployment steps.Release notes
Internal: no release notes.
Checklist