Deferred from helmcode/nan-devops#361 by an explicit call to stop polishing a PR that changes no deployed byte. Everything here is comment text or failure-output ergonomics: nothing in this list lets a broken config pass green. All the functional findings from that review were fixed before merge.
Pointers that do not resolve, or claim more than the code does
.github/workflows/hook-tests.yml, the router_settings rules step: "The suite above" points one step short (the previous step is the resolution model, not the live-values suite, which is two up). Name the file instead of using "above". The same line says "Two of the alias rules iterate zero times over real config today", which is false and contradicts the docstring it cites: they iterate once and never match.tests/usage-hook/test_litellm_usage_hook_generator.py, the _GATED_DIRS escape paragraph: says the escape is "TWO edits and goes green". It no longer is, since the anchoring check eight lines below closes exactly that; and "its two run steps" went stale in the commit that added the third. The paragraph 100 lines above already states the remaining open path correctly.- Same file: the two near-duplicate
SCOPED TO ... paragraphs still both exist, saying the same thing about host-firewall/traefik/watchdog eight lines apart.
- Same file: the replacement for "eight invariants" reads badly and runs to 111 characters ("until invariants that moved out of the usage-hook suite moved to").
tests/router-settings/invariants.py: "that wrong number shipped in the same paragraph arguing a false citation is a real defect" is not accurate. The wrong number was in invariants.py and the argument in litellm_resolution.py..github/workflows/hook-tests.yml: "Eight of these invariants used to live inside the suite above" still carries the count that was removed from two other places.
Failure output
tests/router-settings/test_invariants.py: 13 of 26 case labels are duplicated (alias_bad_shape x4, banned_surface x3, bad_value x3, ...), so a red names the rule but not WHICH case broke. In a suite whose thesis is that every rule names its case, the failure does not name the case. A short tag per case (alias_bad_shape[list-model]) fixes it.tests/router-settings/test_router_settings.py: the parenthetical added about the stale rule count lands mid-sentence and pushes the line to 96 characters.
Why this is separate
PR #361 went through five review rounds and twelve full reviews. The last two rounds found real defects, and they were fixed, but the remainder is documentation quality on a test harness. The deployed configuration is semantically identical to main across all four documents, verified with a positive control, so none of this is in the path of serving traffic.
Related: helmcode/nan-devops#361, #77 (the routes that PR closed), #76.
Deferred from helmcode/nan-devops#361 by an explicit call to stop polishing a PR that changes no deployed byte. Everything here is comment text or failure-output ergonomics: nothing in this list lets a broken config pass green. All the functional findings from that review were fixed before merge.
Pointers that do not resolve, or claim more than the code does
.github/workflows/hook-tests.yml, therouter_settings rulesstep: "The suite above" points one step short (the previous step is the resolution model, not the live-values suite, which is two up). Name the file instead of using "above". The same line says "Two of the alias rules iterate zero times over real config today", which is false and contradicts the docstring it cites: they iterate once and never match.tests/usage-hook/test_litellm_usage_hook_generator.py, the_GATED_DIRSescape paragraph: says the escape is "TWO edits and goes green". It no longer is, since the anchoring check eight lines below closes exactly that; and "its two run steps" went stale in the commit that added the third. The paragraph 100 lines above already states the remaining open path correctly.SCOPED TO ...paragraphs still both exist, saying the same thing about host-firewall/traefik/watchdog eight lines apart.tests/router-settings/invariants.py: "that wrong number shipped in the same paragraph arguing a false citation is a real defect" is not accurate. The wrong number was ininvariants.pyand the argument inlitellm_resolution.py..github/workflows/hook-tests.yml: "Eight of these invariants used to live inside the suite above" still carries the count that was removed from two other places.Failure output
tests/router-settings/test_invariants.py: 13 of 26 case labels are duplicated (alias_bad_shapex4,banned_surfacex3,bad_valuex3, ...), so a red names the rule but not WHICH case broke. In a suite whose thesis is that every rule names its case, the failure does not name the case. A short tag per case (alias_bad_shape[list-model]) fixes it.tests/router-settings/test_router_settings.py: the parenthetical added about the stale rule count lands mid-sentence and pushes the line to 96 characters.Why this is separate
PR #361 went through five review rounds and twelve full reviews. The last two rounds found real defects, and they were fixed, but the remainder is documentation quality on a test harness. The deployed configuration is semantically identical to main across all four documents, verified with a positive control, so none of this is in the path of serving traffic.
Related: helmcode/nan-devops#361, #77 (the routes that PR closed), #76.