Skip to content

fix(charmlint): docs and licence rules are monorepo-aware (#65) - #207

Closed
tonyandrewmeyer wants to merge 1 commit into
mainfrom
claude/brave-lovelace-a9ceyr
Closed

tonyandrewmeyer wants to merge 1 commit into
mainfrom
claude/brave-lovelace-a9ceyr

Conversation

@tonyandrewmeyer

Copy link
Copy Markdown
Owner

Closes #65.

What changed

DOC002–DOC005 (installation / configuration / usage / troubleshooting docs) looked only in <charm>/docs, and STR001 looked only for <charm>/LICENSE or <charm>/LICENCE. A monorepo keeps its charms under charms/<name>/ with a single shared docs/ tree and one licence at the repository root, so every charm in such a repository was reported as undocumented and unlicensed — five false positives per charm, none of them actionable.

CharmContext now records the enclosing repo_root and exposes search_roots():

  • repo_root is the nearest ancestor holding a .git entry. It is tested with exists() rather than is_dir(), because .git is a plain file in linked worktrees and submodules, which are repository roots just the same.
  • search_roots() returns the charm directory first, then the repository root when it is a different directory.

The docs and licence rules walk that list. Two consequences worth stating explicitly: charm-local assets still win (a charm with its own docs/installation.md is unaffected), and a charm outside a repository gets a one-element list, so its behaviour is byte-for-byte what it was before.

The same change is mirrored in the Rust implementation (models.rs, context.rs, rules.rs) so the two do not drift.

One incidental tidy-up inside the function I was already editing: the keyword in content test moves out of the try block in _check_doc_topic, per the CLAUDE.md convention of minimising code inside try/except. No behaviour change.

Why it is worth doing

It is a false-positive class, which is the expensive kind for a linter the agent reads: five unactionable findings per charm in a monorepo train the agent (and the human) to discount DOC and STR output generally. The repository-root walk is also the zero-config fix — nothing to configure, and it matches where the META docs link already points.

Scope I deliberately left out

  • The config-key alternative. The issue offered either a repo-root walk or "a config/flag for the docs location". I did the walk only. It covers the layout in the report without asking the user for anything; a docs-dir key in .charmlint.yaml would be the right follow-up if someone keeps docs somewhere other than <root>/docs, but adding a config surface speculatively seemed worse than not having one.
  • DOC001 (no README) is unchanged, on purpose. A shared root README.md does not document a specific charm, so falling back there would turn a true positive into a miss. Same reasoning for STR002 (icon) and the tests/ checks — those are genuinely per-charm.
  • cargo fmt. The crate is already 73 diffs away from rustfmt-clean in files I did not touch, so running it would have been a repo-wide reformat. I matched the surrounding hand-formatting instead. Flagging it as pre-existing rather than silently leaving it.

Things I am less sure about

  • Repeated tree walks. _check_doc_topic is called once per topic, so a monorepo's root docs/ tree is now walked four times instead of zero. The charm-local tree was already walked four times, so this is the existing shape rather than a new one, and on a realistic docs tree it is a few milliseconds. But if the Rust implementation's sub-30 ms budget matters on a large monorepo, the better fix is to read the docs content into CharmContext once at build time and have all four rules share it. I did not do that here because it widens the change past the bug.
  • .git as the repository marker. Charms vendored into a non-git tree (an extracted tarball, a hg checkout) still get the old behaviour. That seemed like the right trade against guessing a root from, say, the presence of a top-level charms/ directory.

How I verified it

make check is clean on the branch:

uv run ruff check src tests
All checks passed!
uv run ruff format --check src tests
uv run ty check src
All checks passed!
========== 8499 passed, 13 skipped, 1 warning in 169.54s (0:02:49) ==========
TOTAL                                              37845   2718    93%
charmlint-rs: test result: ok. 93 passed; 0 failed
             test result: ok. 8 passed; 0 failed   (lint_integration)
quickpack-rs: test result: ok. 61 passed; 0 failed
cantrip-kdiag: ok (4 packages)

make check does not cover everything CI does, so I also ran the rest of the CI gates:

  • uv run pytest tests/integration -n auto → 193 passed, 14 skipped
  • cargo test --all-targets on both crates → green
  • make docs-check-strict → docs: rebuilt HTML matches committed output

Coverage total is unchanged at 93% because --cov=cantrip does not measure the charmlint package; the new tests are counted by neither the before nor the after figure.

Tests are negative-checked. Reverting src/charmlint/ and re-running the new Python tests fails 8 of 12; the 4 that still pass are the regression guards, which are supposed to hold either way. Twelve equivalent tests exist on the Rust side. Between them they cover the shared-docs layout, partial shared docs (only the documented topic stops being flagged), shared LICENSE and LICENCE, .git-as-a-file, charm-local docs still winning, search_roots() directly including the collapse when the charm is the repository root, and the two regression guards (charm outside a repository, repository with no shared assets).

No user-facing surface changed — no CLI flag, env var, slash command or config key — and the user docs carry no rule catalogue, so there is nothing under docs/src/ to update. CHANGELOG.md has an ## Unreleased entry.

🤖 Generated with Claude Code

https://claude.ai/code/session_018wfGexXpXdSyPkTuxqAXnF


Generated by Claude Code

DOC002-DOC005 (installation / configuration / usage / troubleshooting
docs) looked only in `<charm>/docs`, and STR001 looked only for
`<charm>/LICENSE`.  A monorepo keeps its charms under `charms/<name>/`
with one shared `docs/` tree and one licence at the repository root, so
every charm in such a repository was reported as undocumented and
unlicensed — five false positives per charm.

`CharmContext` now records the enclosing `repo_root` (the nearest
ancestor with a `.git` entry, tested with `exists()` so linked worktrees
and submodules count) and exposes `search_roots()`: the charm directory
first, then the repository root when it differs.  The docs and licence
rules walk that list, so charm-local assets still win and a charm
outside a repository behaves exactly as before.

Mirrored in the Rust implementation so the two stay in step.  The
`_check_doc_topic` keyword test also moves out of the `try` block,
per the convention of minimising code inside try/except.

Twelve tests per implementation cover the shared-docs and shared-licence
layouts, the `.git`-as-a-file worktree case, `search_roots()` itself, and
regression guards that a charm outside a repository, and a repository
with no shared assets, are still flagged.

Closes #65

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018wfGexXpXdSyPkTuxqAXnF

Copy link
Copy Markdown
Owner Author

The warden check is failing on this PR, and it is not this PR's failure. Flagging it rather than fixing it here, because the fix belongs in warden.toml and this PR touches neither that file nor any workflow.

What is failing

warden exits 1 with both triggers erroring identically:

Trigger security-review failed: InvalidPiModelSelectorError:
  Pi runtime model for security-review must use provider/model format: claude-sonnet-4-20250514
Trigger charm-review failed: InvalidPiModelSelectorError:
  Pi runtime model for charm-review must use provider/model format: claude-sonnet-4-20250514
##[error]All 2 trigger(s) failed

Why it is not this PR's

warden.toml sets model = "claude-sonnet-4-20250514" and has done since 4cb4857 (2026-08-25, unchanged). getsentry/warden was bumped 0.49.0 → 0.50.0 in 2d9763f (#203, merged 2026-10-01), and 0.50.0 requires the model selector to carry a provider prefix. So the config and the action version are incompatible as of yesterday, independently of any PR's diff.

The reason this surfaces here rather than on #203 itself is that the workflow's if: skips dependabot (WARDEN_ANTHROPIC_API_KEY is not exposed to it). Warden runs 191–194 — every run since the bump — are all skipped. This PR is run 195, the first run to actually execute warden since 0.50.0 landed, so it is the first to hit the config break. It will fail the same way on every subsequent non-dependabot PR until warden.toml is updated.

I have not spent a re-run on it: the error is a deterministic config-parse failure with an identified root cause, so a re-run would reproduce it exactly, and the run history above establishes the "not this PR's" case more strongly than a re-run would.

Proposed patch (not applied here)

 [defaults]
-model = "claude-sonnet-4-20250514"
+model = "anthropic/claude-sonnet-4-20250514"

One caveat on that patch, stated plainly because I could not verify it: the error text asks for provider/model but does not name the accepted provider tokens, and warden's own repository is outside this session's access, so anthropic is my inference from the model being an Anthropic one rather than something I confirmed against 0.50.0's release notes. Worth a glance at those before landing it. Also worth considering whether to pin a current model while touching the line — claude-sonnet-4-20250514 is some way behind now.

A separate, pre-existing issue in the same log

Warden cannot post its check runs at all:

POST /repos/tonyandrewmeyer/cantrip/check-runs - 403
warden: WARN: Failed to create core check: HttpError: Resource not accessible by integration

.github/workflows/warden.yaml grants contents: read and pull-requests: write but not checks: write. This is independent of the model-selector break and predates it — it would still be there after the patch above. Mentioning it because it means warden's per-skill checks have been silently absent rather than passing.

This PR's own status: everything else is green — Lint, Type Check, Build, Docs, both Rust crates, E2E, zizmor, Dependency Review, Workflow Security. The two Python test matrix jobs were still running when I wrote this. Given failCheck = false and requestChanges = false in warden.toml, warden looks intended as advisory, so I do not think this red check should block the merge — but that is your call, and the config break is worth fixing on its own account since it affects every PR.


Generated by Claude Code

@tonyandrewmeyer
tonyandrewmeyer deleted the claude/brave-lovelace-a9ceyr branch October 2, 2026 23:24
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.

charmlint: DOC002/DOC005 (and likely others) are not monorepo-aware

2 participants