docs(src): reconcile every docstring with the code it describes - #1173
Merged
Conversation
Brings `interrogate src/` from 87.5% to 100.0%. Every gap was outside ruff's reach: pydocstyle's D1xx rules do not descend into function bodies and skip `_name` members, so all 192 nodes were nested functions (route handlers inside create_app, closures, callbacks) or module-level private helpers. No top-level public function, class, or module docstring was missing. Purely additive: 211 insertions, 0 deletions. No existing docstring, comment, or line of code is modified. The 84 sites that already carried an explanatory `#` comment keep it, with a one-line summary added above. Verified: interrogate 100%, ruff check + format, pyright 0 errors, 4661 tests passing, coverage 99.99%, docs lint + mkdocs --strict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audited all 1455 docstrings in src/ against their implementations and fixed the ones that misdescribed the code. Also removed comments that merely restated their own docstring and condensed padded comment blocks. Docstrings only — no executable line changes. Verified structurally rather than by eye: for all 52 touched files, parsing HEAD and the working copy, stripping every module/class/function docstring, and comparing the ASTs gives an exact match. What changed: - 152 docstrings that made claims the code does not honour: exceptions and HTTP status codes never raised, locking/ordering/thread-affinity that isn't real, parameters the body ignores, caller behaviour attributed to the callee, and "still ahead"/"not wired up yet" notes for work that has since shipped. - 48 comments that restated their own docstring, deleted. - 27 padded comment blocks condensed. Issue refs, safety rationale, ordering warnings, and cross-references were preserved throughout; net -167 comment lines. Several docstrings overstated a safety property — claiming a gate "fails closed", a value is redacted, or a symlink target is "never followed/read" where the code does otherwise. Those now describe actual behaviour, including the limitation, so the prose can be trusted as written. 14 findings were code defects rather than prose drift. Per maintainer direction those are documented as-is here and written up separately for triage rather than fixed in a docs change. interrogate: still 100.0%. just check: green (4661 passed, coverage 99.99%, pyright 0 errors). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
clauster releases are changesets-only: a user-facing change needs a fragment, or it ships with Advisory only — this check never blocks the merge. |
schubydoo
added
the
no-changelog
schubydoo
marked this pull request as ready for review
July 31, 2026 02:07
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
schubydoo
added a commit
that referenced
this pull request
Jul 31, 2026
Findings 0, 0b and 7 from `scratch/docstring-audit-findings.md`. Each fix also deletes the limitation paragraph #1173 deliberately added to its docstring and restates the guarantee that now holds — fixing the code alone would have re-introduced drift in the more dangerous direction. Finding 0 — setup wizard returned 500 instead of 403 on a non-ASCII token. `secrets.compare_digest` raises TypeError on a non-ASCII str, so `?token=%C3%A9` escaped as an unhandled 500. This gate fronts the FIRST-RUN wizard, which is unauthenticated by construction, binds non-loopback in the Docker image, and gates a config writer — an unhandled raise on attacker-controlled input on the one surface with no auth in front of it. Never a bypass; it 500'd rather than granting. `isascii()` is checked first, which leaks only whether the input was ASCII, never any part of the token. Both the GET and the POST CSRF path are covered. Finding 0b — a secret one level deep came back unmasked. The hint reached scalar leaves and list elements, but a nested dict re-derived it from its own keys, so {"auth": {"value": "sk-live-..."}} leaked — not an exotic shape, it is how a lot of MCP server configs are written, and this is the code-executing config-write tier. A secret-shaped key now marks its whole subtree. Blast radius traced, since the audit flagged it as unconfirmed: six `cw.` call sites — MCP listings (x3), settings misc, subagent frontmatter, and `config_write_mcp_cli._is_redacted`, which is the only one where the change alters a decision rather than display output. `redact.py`'s same-named line-oriented function is untouched. Finding 7 — LIST read a plugin symlink's out-of-tree target. `is_file()` and `read_bytes()` both follow symlinks and both ran before `_is_read_only_file` tested `is_symlink()`, so the target's frontmatter description reached the listing — a stated containment boundary that GET/PUT/DELETE honoured and LIST did not. Classified before the read; the entry is still surfaced as plugin-owned and non-editable, just without a description it should never have read. Sibling sweep (the audit suggested it): `config_write_skills` already classifies first in both places and carries a comment naming the hazard; `config_write_hooks` reads one known settings path rather than iterating user-supplied entries, so neither is exposed. Subagents was the only gap. just check green: 4678 passed, coverage 99.99%, interrogate src/ 100.0%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
schubydoo
added a commit
that referenced
this pull request
Jul 31, 2026
…1174) Findings **finding 0**, **finding 0b** and **finding 7** from the docstring audit — the three security-boundary defects. Follows #1173, which documented them accurately; each fix here also deletes the limitation paragraph that PR added and restates the guarantee that now holds, per the audit's reverse index. ## Finding 0 — setup wizard returned **500 instead of 403** on a non-ASCII token `secrets.compare_digest` raises `TypeError` on a non-ASCII `str`, so `?token=%C3%A9` escaped as an unhandled 500. This gate fronts the **first-run wizard**: unauthenticated by construction, binds non-loopback in the Docker image, and gates a config writer. So it was an unhandled raise on attacker-controlled input on the one surface with no auth in front of it. **Never a bypass** — it 500'd rather than granting. `isascii()` is checked first, which leaks only whether the input was ASCII, never any part of the token. The expected token is `secrets.token_urlsafe(32)`, ASCII by construction, so no legitimate token can be rejected. Both the GET and the POST CSRF path are covered. ## Finding 0b — a secret one level deep came back **unmasked** The hint reached scalar leaves and list elements, but a nested dict re-derived it from its own keys: ``` flat under auth -> {'auth': '********'} masked NESTED under auth -> {'auth': {'value': 'sk-live-…'}} LEAKED ``` Not an exotic shape — it is how a lot of MCP server configs are written, and this is the code-executing config-write tier. A secret-shaped key now marks its whole subtree. **Blast radius traced** (the audit flagged it as unconfirmed): six `cw.redact_secrets` call sites — MCP listings ×3, settings misc, subagent frontmatter, and `config_write_mcp_cli`, the only one where the change alters a *decision* rather than display output, and in the fail-closed direction. `redact.py`'s same-named line-oriented function is untouched. ## 7 — LIST read a plugin symlink's **out-of-tree target** The module docstring promises a symlink's target is *never followed/read*. GET, PUT and DELETE honoured that; LIST did not, surfacing the target's frontmatter `description`. Classification now happens before anything stats the target. **Sibling sweep** (the audit suggested it): `config_write_skills` already classifies first in both places and carries a comment naming the hazard; `config_write_hooks` reads one known settings path rather than iterating user-supplied entries. Subagents was the only gap. ## Review round already applied `clauster-backend-reviewer` found two real defects, both reproduced before accepting: - **The wider mask broke the MCP write surface** — a regression this branch introduced. A user-chosen server *name* like `oauth-gw` matched the secret-key regex and masked its own structural `type`, which `_entry_transport` then rejects, 422ing every write in the scope. Fixed with per-entry redaction so a name never enters the hint chain. - **`_list_agents` still *followed* the symlink** — `is_file()` ran before the guard and stats the target, so the new docstring's "never followed" was an overclaim, and it silently dropped dangling symlinks from LIST while GET still reported them present. ## Gates `just check` exit 0 · **4680 passed** · coverage 99.98% · `interrogate src/` **100.0%** --------- Co-authored-by: Schuby <12485317+schubydoo@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
schubydoo
added a commit
that referenced
this pull request
Jul 31, 2026
`--max-turns 60` turned out to be the **binding limit on real pull requests**, not a runaway guard. Two runs in the docstring-audit series died with `"subtype": "error_max_turns"` partway through reading the diff, at roughly 8 minutes each — #1173, and #1177 this week. ## Why this is worth fixing rather than retrying The failure mode is worse than getting no review. The action still posts its progress comment, so the PR gains a Claude comment carrying an unticked checklist and an error line. At a glance it **looks reviewed** while containing no findings at all — the same "looks reviewed when it wasn't" hazard the `Fail if nothing was posted` step was added to catch, arriving by a different route. Both times the maintainer had to spot it and clean up by hand. ## Why 60 is too low Reading a multi-file diff is turn-expensive — chunked `Read`s plus `gh pr diff` — and this reviewer is additionally asked to fetch the rules file from the default branch, anchor every finding as an inline comment, and submit exactly one grouped review. 60 turns does not cover that on anything but a small change. ## Why 150 and not more 150 keeps the cap **meaningful** instead of making it dead weight. Measured ~8s per turn on the failing runs, so it binds at roughly 20 minutes — still inside the job's existing `timeout-minutes: 30`. The job timeout remains the hard backstop, and the `Fail if nothing was posted` step is untouched, so the older failure (a run reporting success while publishing nothing) is still caught. Going much higher would let the turn cap drift past the job timeout, at which point it stops guarding anything. ## Checks - `uvx zizmor .github/workflows/claude-review.yml` — no findings (3 pre-existing suppressions) - `actionlint .github/workflows/claude-review.yml` — clean, with `shellcheck` on PATH so the embedded `run:` blocks were linted too - Parsed the workflow back to confirm the value lands as `--max-turns 150` and `timeout-minutes` is still 30 No changeset: CI-only, no user-facing effect — `no-changelog`. --------- Co-authored-by: Schuby <12485317+schubydoo@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
schubydoo
added a commit
that referenced
this pull request
Jul 31, 2026
…1158 (#1190) Documentation audit of every merged PR from #1158 onward, ahead of the 1.0.1 deploy. ## What I checked | PRs | Verdict | | --- | --- | | #1160–#1166, #1169, #1179, #1186, #1187 | Dependency bumps and CI — no user-facing surface | | #1158, #1167, #1168 | Documentation PRs already | | #1173 | Docstrings only, proven code-free by AST comparison | | #1177, #1185 | Docs updated in their own PRs — generated tables confirmed still in sync | | **#1174, #1183** | Changed `src/` with **no** doc update — audited in detail | ## Two real gaps, both in `troubleshooting.md` That page is where a user lands when something breaks, which is what makes these worth fixing before a deploy. **Config errors (#1183, finding 1).** A YAML *syntax* error used to traceback out of every CLI verb, including `doctor`. It now exits `2` with one line, and `doctor` gained a third `config` detail: `config is not valid YAML: ...`. The page described two failure shapes and said *"the `invalid config` detail names the offending key"* — true for a schema error, but not for a parse error, which names a line and column. It is now three shapes in a table, each with what is wrong and where the message points, and the doctor-row table lists the new string. **Claustrum startup (#1183, finding 12).** `_resolve_binary` — claustrum simply not installed, the commonest real failure — used to raise without setting `_error`, so `/healthz` reported `running:false, error:null` and the reason survived only in the server log. It now carries the reason. The "cannot connect to claustrum" section sent readers to the daemon log. It now starts with `/healthz`, which is both faster and where the answer actually is. ## What I verified rather than assumed Every documented string is grepped from the code, not paraphrased: ``` ops.py:91 f"no config found: {exc}" ops.py:94 f"invalid config: {exc}" ops.py:100 f"config is not valid YAML: {exc}" __main__.py f"clauster: config error: {exc}" (x2) claustrum_daemon.py:179 "error": self._error ``` ## Three undocumented surfaces — two now explained, one deliberately not Nothing had *drifted* for these, because none of them were documented. But two produce a UI state a user reads as a fault: - **A plugin-provided subagent** (#1174, finding 7) lists with no description and refuses edit and delete. Both are deliberate — a plugin owns the file, and Clauster never follows the symlink, so it genuinely does not know what the subagent does. Undocumented, that is a blank row you cannot fix. Now explained, plus a troubleshooting row. - **An unreadable skill** (#1183, finding 13) still appears, carrying an error instead of the missing information, and never hides the others. One sentence. Deliberately **not** documented, and worth recording why: - **The listing JSON keys** (`frontmatter_error`, `files_error`, `has_skill_md`). The OpenAPI schema already describes them, and prose that repeats a schema is a second copy that drifts. The behaviour is user-visible; the key names are not. - **`install-service` rejecting a `"` in a path** (#1183, finding 9). Windows forbids `"` in filenames, so a real Windows path never trips it — it is reachable only by rendering a Windows unit from Linux, and the error already says what is wrong. ## Also checked, no gap - The secret-masking sentence in the config-editor guide stays true under the widened mask (#1174, finding 0b). - `max_bridges` (#1177) and the config-editor bounds (#1185) were updated in their own PRs; `gen_config_reference.py --check` confirms the generated tables are in sync. ## Checks `just docs-lint` and the strict `mkdocs build` both clean. `no-changelog`: documentation only, no behaviour change. --------- Co-authored-by: Schuby <12485317+schubydoo@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reconciles every docstring in
src/with the code it actually describes. Documentation only — no behaviour change.Proven docstring-only, not asserted
Every
.pyfile in the diff was parsed, had its docstrings stripped, and compared as an AST against its base version:Re-run it yourself — the script is in
scratch/docstring-audit-findings.mdunder Structural check.What this contains
f84b9641, which added the 192 docstringsinterrogatehad flagged as missing.This is the part worth understanding before reviewing. Where the code has a real defect, the docstring now documents the defect accurately rather than restating an intent the code does not honour — that was the explicit call: document as-is, triage separately. Prose describing an unhonoured guarantee is exactly what this audit existed to eliminate.
The consequence, recorded in
scratch/docstring-audit-findings.md: the moment a defect is fixed, its docstring becomes wrong again — in the more dangerous direction, understating a guarantee that now holds. The audit file carries a reverse index mapping each code fix to the docstrings that must move with it.16 code findings came out of this audit and are tracked separately; all have now been triaged with decisions recorded. They land in follow-up PRs, not here.
Gates
interrogate src/— 100.0%just check— exit 0 · 4672 passed · coverage 99.99%main, 0 behindno-changelog: no user-facing behaviour changes.