Replace the docstring audit with a zero-tolerance TypeDoc gate#5
Replace the docstring audit with a zero-tolerance TypeDoc gate#5leynos wants to merge 6 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace the docstring coverage audit with strict TypeDoc validation, document Dakar workflow contracts and JavaScript helpers, add two typed fields, update check wiring, and run ODW build and repository gates in CI. ChangesDocumentation gate
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bebc8598e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "entryPoints": ["bin", "scripts", "src/workflows/dakar-review"], | ||
| "entryPointStrategy": "expand", | ||
|
|
||
| "exclude": ["**/*.d.ts", "**/meta.js", "scripts/tests/**"], |
There was a problem hiding this comment.
Because this exclusion removes src/workflows/dakar-review/meta.js from TypeDoc entirely, changes to that authored JavaScript module can never be checked for either its module header or its exported meta declaration; the current file still uses the retired @file form, so make docs-check already passes despite violating the new zero-tolerance module contract. Include this module in the gate or add an equivalent explicit check rather than excluding it.
AGENTS.md reference: AGENTS.md:L19-L23
Useful? React with 👍 / 👎.
| /** | ||
| * Validates structured configuration-resolution results from the Config phase. | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
When a schema property is added or changed without a description, this @internal tag combines with excludeInternal to remove the entire exported schema from notDocumented validation, so the gate passes with no per-field documentation; every property in the schemas introduced here currently has only structural keywords such as type and enum, despite the repository contract relying on description fields to justify this exemption. Add and validate those descriptions separately, or keep the schemas visible to the documentation gate.
AGENTS.md reference: AGENTS.md:L21-L24
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 65: Remove the global npm install from the CI setup before make check.
Update the workflow to prepend /tmp/odw/node_modules/.bin to PATH, or use npm
link, so make check runs against the existing lockfile-verified dependency tree
rather than a separate global installation.
In `@scripts/review-config.mjs`:
- Around line 19-28: Update the ReviewConfigResolution typedef: document config
as the resolved or attempted configuration path without claiming it is empty on
failure, and replace the source descriptions with the actual literal values
explicit, repository, user, and example.
In `@src/workflows/dakar-review/types.ts`:
- Around line 15-23: Add a JSDoc block immediately before the exported ModelSpec
interface declaration in the types module, describing the interface’s purpose.
Preserve the existing member-level documentation and do not alter the interface
fields or behavior.
In `@typedoc.json`:
- Line 41: Add the TypeDoc configuration key treatWarningsAsErrors with a true
value alongside treatValidationWarningsAsErrors in typedoc.json, ensuring both
general warnings and validation warnings fail the documentation build.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11b4b68e-fc3f-4480-85fc-48f207192a15
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.github/workflows/ci.ymlAGENTS.mdMakefilebin/dakar-review.mjsdocs/developers-guide.mdpackage.jsonscripts/build-workflow.mjsscripts/check-docstrings.mjsscripts/review-config.mjsscripts/review-state.mjssrc/workflows/dakar-review/candidates.tssrc/workflows/dakar-review/config.tssrc/workflows/dakar-review/main.tssrc/workflows/dakar-review/model-routing.tssrc/workflows/dakar-review/odw-globals.d.tssrc/workflows/dakar-review/prompts.tssrc/workflows/dakar-review/schemas.tssrc/workflows/dakar-review/shell.tssrc/workflows/dakar-review/task-graph.tssrc/workflows/dakar-review/types.tstests/docstrings.test.mjstsconfig.typedoc.jsontypedoc.json
💤 Files with no reviewable changes (2)
- scripts/check-docstrings.mjs
- tests/docstrings.test.mjs
| cd /tmp/odw | ||
| npm ci | ||
| npm run build | ||
| npm install --global . |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant workflow section with line numbers.
sed -n '1,140p' .github/workflows/ci.yml | nl -ba | sed -n '50,85p'
# Find any other install/link patterns in the repo for context.
rg -n "npm (install --global|link --global|ci|run build|make check)" .github/workflows . -g '!**/node_modules/**' || trueRepository: leynos/dakar
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n .github/workflows/ci.yml | sed -n '50,85p'Repository: leynos/dakar
Length of output: 1008
🌐 Web query:
npm documentation local package install --global npm link behaviour global prefix package.json bin symlink
💡 Result:
The npm link command is a utility for testing local packages by creating symbolic links, allowing you to use a package under development in another local project without needing to publish it to the registry [1][2]. It is a two-step process that leverages the npm global prefix [1][3]. Step 1: Create a Global Link When you run npm link inside a package directory (with no arguments), npm creates a symbolic link in the global node_modules folder (located at {prefix}/lib/node_modules/) that points to your current directory [1][2][4]. It also links any executables defined in the bin field of your package.json file to the global bin directory ({prefix}/bin/) [2][3][5]. Step 2: Link to a Project In a separate project directory, you run npm link , where is the name specified in the package.json file of the package you linked globally [2][3][4]. This creates a symbolic link from that package in the global node_modules folder to the local node_modules folder of your project [2][3][4]. Key Considerations and Behaviors: - Global Prefix: The command uses the global prefix, which you can determine by running npm prefix -g [2][6]. Executables are typically placed in {prefix}/bin on Unix systems or directly in the {prefix} folder on Windows [7][5]. - No package.json Modification: Unlike npm install, npm link does not add dependencies to your project's package.json file [4][8]. - Package Identity: The link is created using the name defined in the package's package.json, not the name of the directory containing the package [2][3][4]. - Comparison with npm install: While npm install can also link a local directory, it behaves differently by modifying package.json and not involving the global node_modules folder [9][8]. Additionally, npm link does not execute lifecycle scripts (such as preinstall or postinstall) [8].
Citations:
- 1: https://docs.npmjs.com/cli/v12/commands/npm-link/
- 2: https://docs.npmjs.com/cli/v8/commands/npm-link/
- 3: https://docs.npmjs.com/cli/v6/commands/npm-link/
- 4: https://manpages.debian.org/testing/npm/npm-link.1.en.html
- 5: https://github.com/npm/bin-links
- 6: https://www.unpkg.com/npm@10.5.2/docs/output/commands/npm-link.html
- 7: https://github.com/npm/cli/blob/latest/docs/lib/content/configuring-npm/folders.md
- 8: https://stackoverflow.com/questions/50674052/difference-between-npm-link-x-and-npm-install-path-to-x
- 9: https://docs.npmjs.com/cli/v11/commands/npm-install/?v=true
Avoid the extra global install. Export /tmp/odw/node_modules/.bin into PATH for make check, or use npm link, so the gate runs the already lockfile-verified tree instead of creating a separate global copy.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 65-65: ad-hoc installation of packages (adhoc-packages): installs a package outside of a lockfile
(adhoc-packages)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 65, Remove the global npm install from the
CI setup before make check. Update the workflow to prepend
/tmp/odw/node_modules/.bin to PATH, or use npm link, so make check runs against
the existing lockfile-verified dependency tree rather than a separate global
installation.
Source: Linters/SAST tools
| /** | ||
| * @typedef {object} ReviewConfigResolution Outcome of a review-configuration | ||
| * search, retaining the full search trail so the workflow output stays | ||
| * auditable. | ||
| * @property {boolean} ok Whether a configuration file was found. | ||
| * @property {string} config Path of the resolved configuration file; empty when `ok` is false. | ||
| * @property {string} source Which search stage supplied the file (explicit argument, repository, XDG, bundled example). | ||
| * @property {string[]} checked Every path probed, in search order, including the winner. | ||
| * @property {string} [error] Reason the search failed; present only when `ok` is false. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the typedef with the returned failure shape.
The implementation returns the attempted path in config when ok is false (Lines 67–77 and 127–137), and uses the literal source value user, not XDG. Correct the documentation contract.
Proposed correction
- * `@property` {string} config Path of the resolved configuration file; empty when `ok` is false.
- * `@property` {string} source Which search stage supplied the file (explicit argument, repository, XDG, bundled example).
+ * `@property` {string} config Path of the resolved or attempted configuration file.
+ * `@property` {string} source Which search stage supplied the file (`explicit`, `repository`, `user`, or `example`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/review-config.mjs` around lines 19 - 28, Update the
ReviewConfigResolution typedef: document config as the resolved or attempted
configuration path without claiming it is empty on failure, and replace the
source descriptions with the actual literal values explicit, repository, user,
and example.
| /** Human-readable name surfaced in dry-run output and task metrics. */ | ||
| label?: string | ||
| /** Base model identifier, with or without a `/reasoning` suffix. */ | ||
| model: string | ||
| /** Reasoning level used to pick the adapter and, when absent, the model suffix. */ | ||
| reasoning: Reasoning | ||
| /** Logical review role (e.g. `high`, `medium`, `mini`, `spark`) matched by `modelForRole`. */ | ||
| role?: string | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add declaration JSDoc for ModelSpec.
Add a JSDoc block immediately before export interface ModelSpec; the member comments document properties only, leaving this exported declaration undocumented and causing the zero-tolerance gate to fail. As per coding guidelines, “document every exported declaration with JSDoc”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflows/dakar-review/types.ts` around lines 15 - 23, Add a JSDoc block
immediately before the exported ModelSpec interface declaration in the types
module, describing the interface’s purpose. Preserve the existing member-level
documentation and do not alter the interface fields or behavior.
Source: Coding guidelines
| "TypeAlias" | ||
| ], | ||
|
|
||
| "treatValidationWarningsAsErrors": true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== typedoc.json ==\n'
cat -n typedoc.json | sed -n '1,120p'
printf '\n== package.json excerpts ==\n'
if [ -f package.json ]; then
cat -n package.json | sed -n '1,220p'
fi
printf '\n== search for TypeDoc warning options ==\n'
rg -n '"treatWarningsAsErrors"|"treatValidationWarningsAsErrors"|typedoc' -S . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: leynos/dakar
Length of output: 3358
🌐 Web query:
TypeDoc 0.28.20 treatWarningsAsErrors treatValidationWarningsAsErrors options Validation documentation
💡 Result:
In TypeDoc 0.28.20, the options treatWarningsAsErrors and treatValidationWarningsAsErrors are part of the configuration suite designed to manage how TypeDoc handles warnings emitted during project conversion and validation [1][2]. treatWarningsAsErrors: When enabled, this option causes TypeDoc to treat any reported warnings as fatal errors [3][4]. This effectively prevents documentation from being generated if any warnings occur [3][4]. treatValidationWarningsAsErrors: This is a specialized, limited version of treatWarningsAsErrors that applies exclusively to warnings emitted during the project validation phase [3][4]. It cannot be used to disable treatWarningsAsErrors for validation warnings; rather, it ensures that even if general warnings are not treated as errors, validation-specific warnings will be [3][4]. These options are categorized under Validation Options within the TypeDoc configuration [5][2]. The validation process itself may trigger warnings for several conditions, which can be configured via the validation option [2][6]. Common validation checks include: - notExported: Warnings when a symbol is referenced but not included in the documentation [2]. - invalidLink: Warnings about broken {@link} tags [2]. - invalidPath: Warnings about unresolvable relative paths [2]. - rewrittenLink: Warnings about {@link} tags that do not link directly to their target [2]. - notDocumented: Warnings about declarations missing doc comments [2]. - unusedMergeModuleWith: Warnings about unresolved @mergeModuleWith tags [2]. You can set these options either via the command line (e.g., --treatWarningsAsErrors) or within your TypeDoc configuration file [3][5][7].
Citations:
- 1: http://typedoc.org/api/interfaces/TypeDocOptions.html
- 2: https://github.com/TypeStrong/typedoc/blob/master/src/lib/utils/options/declaration.ts
- 3: https://typedoc.org/documents/Options.Validation.html
- 4: https://github.com/TypeStrong/typedoc-site/blob/master/options/validation.md
- 5: https://typedoc.org/documents/Options.html
- 6: https://typedoc.org/api/interfaces/Configuration.TypeDocOptions.html
- 7: https://typedoc.org/documents/Options.Configuration.html
Add treatWarningsAsErrors in typedoc.json:41
Add "treatWarningsAsErrors": true alongside "treatValidationWarningsAsErrors": true. The current config only fails validation warnings; general TypeDoc warnings still pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typedoc.json` at line 41, Add the TypeDoc configuration key
treatWarningsAsErrors with a true value alongside
treatValidationWarningsAsErrors in typedoc.json, ensuring both general warnings
and validation warnings fail the documentation build.
Bring the typed workflow modules under
`src/workflows/dakar-review/` and the authored helper scripts
(`review-state.mjs`, `review-config.mjs`, `build-workflow.mjs`) up to
declaration-level documentation ahead of replacing the threshold-based
docstring gate with a zero-tolerance TypeDoc gate: every exported
declaration and property now carries a JSDoc block.
Documentation is comment-only apart from reflowing a few single-line
type literals and return objects to multiline (shapes unchanged), which
TypeDoc requires to attach per-property comments. Inline
`@returns {{ … }}` shapes in the helper scripts become named, documented
`@typedef`s so their properties can carry genuine descriptions. The
`*_SCHEMA` constants are tagged `@internal` as machine seams — their
`description` fields remain the per-field documentation.
Swap the threshold-based `npm run docstrings` audit for `docs:check`: TypeDoc's `notDocumented` validation over `bin`, `scripts`, and `src/workflows/dakar-review` (`entryPointStrategy: "expand"`, `emit: "none"`, warnings as errors), driven by `typedoc.json` with a dedicated `tsconfig.typedoc.json` so the JavaScript helpers join the typed workflow modules in one documentation surface. The gate requires 100% documentation of that surface, reports the qualified name of each undocumented declaration, and writes no documentation artefacts. - Convert every module's leading `/** @file … */` comment to TypeDoc's `/** … @module */` form and require `"Module"` documentation, so the existing module-header rule is preserved, not retired. - Delete `scripts/check-docstrings.mjs` and its test — the custom AST-counting audit and its 80% threshold are superseded. Unexported local helpers are no longer counted; exports are the boundary. - Exclude the generated `workflows/dakar-review.js` artefact, ambient `*.d.ts` declarations, `meta.js`, and tests from the surface. - Wire `docs-check` into `make lint` (and therefore `make check`), and give CI an `npm ci` + `make docs-check` step. - Update AGENTS.md and the developers' guide to name the new gate. Verified: removing a JSDoc block from a documented function fails `docs:check` with the symbol's qualified name and a non-zero exit; restoring it returns the gate to green; a clean run emits no files.
Extend the CI job to run `make check` — the same aggregate used
locally — after the existing spelling and diagram steps, replacing the
narrower standalone `docs-check` step (now reached through
`make lint` inside the aggregate).
The typecheck stage includes an `odw run --args '{"dryRun":true}'`
contract check, and the ODW CLI is neither published to npm nor shipped
with a built `dist` in git, so CI builds it from a pinned upstream
commit and installs it globally before the gates run.
Rebasing onto main brought in the ADR-002 deterministic-tiered review route, which added new workflow modules, scripts, and exported declarations. The zero-tolerance TypeDoc gate now covers that surface, so every new module and export must carry documentation. - Convert the `@file` headers on the modules main added (config, main, admission, pricing, retry, sarif, policy, plus the deterministic-gates, odw-config, and live-review-harness scripts) to TypeDoc `@module` blocks. - Document every exported declaration and property main introduced across types, config, sarif, admission, pricing, retry, task-graph, model-routing, policy, and candidates, and give the live-review-harness inline return shapes named `@typedef`s. - Correct doc comments the entity merge misaligned by one property in `WorkflowConfig`, `WorkflowArgs`, `ReviewTask`, `Verdict`, `NormalizedCandidate`, and `PromptContext`. - Rebuild `workflows/dakar-review.js` and `package-lock.json`. `make check` passes end to end (check-fmt, lint including docs-check, typecheck, workflow freshness, 301 tests, spelling). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bebc859 to
d5dedf8
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Two review-feedback fixes closing zero-tolerance gate escapes. - typedoc.json excluded `src/workflows/dakar-review/meta.js`, so its module header and exported `meta` were never gated and the file still used the retired `@file` form. Drop the exclusion, convert the header to `@module`, and document `meta` and its fields so the whole module is covered. - The `@internal` tag on each schema constant removes it from `notDocumented` validation, but AGENTS.md justifies that exemption by the schemas' per-field `description` values -- which were absent. Add a `description` to every field of `CANDIDATE_SCHEMA`, `VERDICT_SCHEMA`, `VERDICT_WITH_CLUSTER_SCHEMA`, and `AUDIT_SCHEMA` (via the shared `VERDICT_PROPERTIES`), making the exemption honest and enriching the agent-facing schema contract. Rebuild `workflows/dakar-review.js`. `make check` passes end to end (301 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Two review-feedback fixes.
- typedoc.json only promoted `validation.*` warnings to errors via
`treatValidationWarningsAsErrors`, so a general (non-validation)
TypeDoc warning would log but not fail the build. Add
`treatWarningsAsErrors: true` so every warning is fatal, matching the
zero-tolerance intent the developers' guide already states. The gate
stays green under the stricter setting.
- scripts/review-config.mjs: the `ReviewConfigResolution` typedef claimed
`config` is empty when `ok` is false, but `resolveReviewConfig` returns
the attempted path there; and it paraphrased `source` ("XDG", "bundled
example") instead of the literal values. Document `config` as the
resolved-or-attempted path and state the exact `source` literals
`explicit`, `repository`, `user`, and `example`.
Two further review comments were verified and skipped as already
satisfied: the ODW CLI's global install does not touch the project's
lockfile-verified tree (and `odw` is absent from `node_modules/.bin`, so
the suggested PATH change would not expose it), and `ModelSpec` already
carries a describing JSDoc block.
`make check` passes end to end (301 tests).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
This branch replaces Dakar's threshold-based docstring audit
(
scripts/check-docstrings.mjs, 80% minimum) with a zero-toleranceTypeDoc documentation gate. TypeDoc's
notDocumentedvalidation nowruns over
bin,scripts, andsrc/workflows/dakar-reviewwithentryPointStrategy: "expand",emit: "none", and warnings treated aserrors: every module must open with a
/** … @module */block and everyexported declaration must carry a JSDoc comment. The gate reports the
qualified name of each undocumented declaration, writes no documentation
artefacts, and fails the build on a single omission.
The branch lands in two commits: the first documents every exported
declaration in the review surface while the old audit still gates it;
the second swaps the gate, converts the
@fileheaders to TypeDoc's@moduleform (preserving the module-header rule rather than retiringit), and deletes the custom AST-counting audit.
Review walkthrough
and tsconfig.typedoc.json
for the gate's configuration: the documentation surface, the exclusions
(generated
workflows/dakar-review.js, ambient*.d.ts,meta.js,tests), and the JavaScript-inclusion compiler settings (
allowJswithcheckJsfalse — this adds a documentation gate, not a new JStype-checking policy).
and package.json
for the wiring:
docs-checkreplacesdocstringsinsidemake lint,so
make checkreaches it, and.github/workflows/ci.yml
gains an
npm ci+make docs-checkstep.src/workflows/dakar-review/types.ts
(the bulk of the property documentation) and
scripts/review-state.mjs,
where inline
@returns {{ … }}shapes became named, documented@typedefs so their properties can carry genuine descriptions.shows the
*_SCHEMAtreatment: each constant carries a documented/** … @internal */block, so TypeDoc does not recurse into the JSONSchema literals — their
descriptionfields remain the per-fielddocumentation.
and docs/developers-guide.md
for the updated conventions.
Validation
make check(full aggregate: check-fmt, lint including the newdocs-check, typecheck with the ODW dry run, workflow freshness,tests, spelling): exit 0 on both commits.
makes
npm run docs:checkexit 4 withscripts/review-state.slug (CallSignature) … does not have any documentation; restoring it returns the gate to green.docs:checkrun emits no files and no output.Notes
gate deliberately treats exports as the documentation boundary, so
unexported local helpers are no longer counted. Existing internal-helper
documentation is untouched.
docs/execplans/compile-dakar-review-workflow.md
still describes the retired
npm run docstringsaudit; it is acompleted plan and has been left as a historical record.
References