Skip to content

fix(validate-manifest): make the rule run, and stop it over-reporting - #191

Open
mnaoumov wants to merge 3 commits into
obsidianmd:masterfrom
mnaoumov:validate-manifest
Open

mnaoumov wants to merge 3 commits into
obsidianmd:masterfrom
mnaoumov:validate-manifest

Conversation

@mnaoumov

@mnaoumov mnaoumov commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@tgrosinger tgrosinger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directionally this looks good to me, but I'll defer to @saberzero1 for the actual code review.

Comment thread lib/rules/validateManifest.ts Outdated
Comment thread lib/rules/validateManifest.ts Outdated
Comment thread docs/rules/validate-manifest.md

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

lib/rules/validateManifest.ts currently passes an optional allowedWords[key] into a parameter typed as string[], which will fail TypeScript type-checking and should be coalesced to [].

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes the validate-manifest rule so it actually runs on manifest.json (by configuring ESLint to parse it as strict JSON via @eslint/json), and tightens the rule’s behavior/options to reduce over-reporting and add targeted escape hatches.

Changes:

  • Move obsidianmd/validate-manifest out of the JS/TS globbed rules and into a manifest.json-scoped flat-config block using language: "json/json".
  • Refactor validateManifest to operate on @eslint/json’s Momoa AST and add rule options (allowedWords, ignore) plus expanded tests.
  • Update docs and config tests to document/guard the file-scoped behavior.
File summaries
File Description
tests/validateManifest.test.ts Runs the rule under the JSON language and adds coverage for ignore/allowedWords and JSON-specific behavior.
tests/recommendedConfig.test.ts Adds regression tests to ensure validate-manifest is enabled for manifest.json and not for source files.
README.md Documents that recommended config lints non-source JSON files and requires running ESLint from project root.
lib/rules/validateManifest.ts Reworks the rule to use @eslint/json AST, adds options/schema, and reduces false positives.
lib/index.ts Adds a manifest.json-scoped flat-config block enabling validate-manifest under json/json.
eslint.config.mjs Disables validate-manifest for this repo’s own manifest.json fixture.
docs/rules/validate-manifest.md Documents new options and how to configure them in flat config.
docs/configuration.md Documents that recommended config includes manifest.json linting and how file-scoped rules work.
Review details

Suppressed comments (1)

lib/rules/validateManifest.ts:340

  • Same as above: allowedWords[key] can be undefined, which is not assignable to the string[] parameter type of hasForbiddenWords. Coalesce it to [] to satisfy the types.
                                    word: hasForbiddenWords(
                                        valueNode.value,
                                        allowedWords[key],
                                    )[1],
                                    key,
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/rules/validateManifest.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Address the validation gaps and documentation issues before approval.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Reject regional-indicator and keycap emoji sequences

lib/​rules/​validateManifest.ts:37

This pattern does not reject every emoji: regional-indicator flags such as 🇺🇸 and keycap sequences such as 1️⃣ contain none of Extended_Pictographic, Cc, Cf, or Cs, so descriptions containing them pass despite the comment and tests saying emoji are forbidden. Include the regional-indicator and emoji selector characters (and keep a regression test) so the new allowlist does not let these sequences through.

Medium severity Validate empty-string manifest keys

lib/​rules/​validateManifest.ts:249

The key && guard lets a valid JSON member with an empty-string key bypass the disallowed-key check. expectedType is then undefined, so { "": 1 } is silently ignored even though "" is not in the manifest schema; test Object.hasOwn unconditionally.

Low severity Import json in the plugin configuration example

docs/​configuration.md:313

This example refers to json in plugins but never imports it (the only import shown earlier is obsidianmd). Copying this configuration block therefore throws ReferenceError: json is not defined before ESLint can load it; add the required import json from "@eslint/json" to the example or explicitly document that prerequisite.

The rule could not report anything in `configs.recommended`.

It registered a `Program` visitor and read `Program.body[0].expression` as an
ESTree `ObjectExpression`. Under `@eslint/json`'s `json/json` language -- the
language this plugin already uses for its `package.json` block -- the visitor key
is `Document` and the AST is Momoa, so `Program` never fired.

It looked correct only because its tests ran `@typescript-eslint/parser` with
`extraFileExtensions: [".json"]`, where TypeScript parses a `.json` filename in
JSON mode and does yield `ExpressionStatement -> ObjectExpression` -- an AST no
real lint run produces for this file. Ported to the Momoa AST
(`Document -> Object -> Member`, key at `member.name.value`), with the tests
moved onto the JSON language.

It was also scoped to globs it could never match: it lived in
`recommendedPluginRulesConfigBase`, which is only spread into the
`**/*.{js,cjs,mjs,jsx}` and `**/*.{ts,cts,mts,tsx}` blocks. It now gets a
file-scoped block, modelled on the existing `package.json` one. Measured before
and after on the same fixture: ESLint previously answered "File ignored because
no matching configuration was supplied" for manifest.json, and now reports.

Two checks also report on manifests the community directory itself passed
untouched.

`descriptionFormat` required `^[A-Za-z0-9\s.,!?'"-]+$`, which rejects backticks,
em dashes, parentheses, colons and slashes. Seven live listings carry those and
the directory flagged none, so as written the check is noise rather than a gate.
It now rejects emoji and invisible characters -- `\p{Extended_Pictographic}`,
`\p{Cc}`, `\p{Cf}`, `\p{Cs}` -- and accepts ordinary punctuation. Length, leading
capital and trailing period are unchanged. Behaviour change worth calling out:
the existing "description with special characters is forbidden" case used
`@ # $ % ^ & *`, which now passes; that case moves to the valid set and an
invisible-character case takes its place.

`FORBIDDEN_WORDS` defaults are unchanged -- `obsidian` and `plugin` are still
banned in `id`, `name` and `description`. What is new is a way to opt out, for
findings that can never be acted on: a published plugin id can never change, so
a plugin whose id contains `obsidian` carries it forever.

The bypass has to be config-level. A manifest cannot carry an ignore marker of
its own -- the only marker ESLint understands is a comment, a comment in strict
JSON is a parse error, and it would also stop Obsidian loading the plugin, since
both the loader and the directory use `JSON.parse`. An ignore key survives
parsing but ships to every user and is itself reported as a disallowed key. So
the rule gains two options:

    allowedWords: { id?: string[], name?: string[], description?: string[] }
    ignore: ("noForbiddenWords" | "descriptionFormat")[]

Both are narrow by construction and tested as such: `allowedWords` un-bans one
word in one field and leaves every other field reporting, and ignoring one check
leaves the other running.

`tests/recommendedConfig.test.ts` gains a "file-scoped rules" block that resolves
the config for manifest.json, which is the only assertion that would have caught
the scoping defect.

Docs. `manifest.json` is matched by name, so ESLint has to be pointed at the
project root: an `eslint src` lint script never visits it and the check silently
does nothing. README and configuration.md say so, under a new "Files linted
beyond your source" section.

Two existing statements in configuration.md are now wrong and are corrected:

- The scanner-equivalent config switched `validate-manifest` off inside a block
  globbed to `**/*.{ts,...,jsx}`. manifest.json matches no source glob, so that
  "off" never reached the rule -- harmless while the rule could not run, and
  wrong now. Moved to a `files: ["manifest.json"]` block.
- The "using only obsidianmd rules" caveat said the rule "will work on .json
  files only if you have a JSON parser configured". It now shows the config block
  that actually enables it, and notes that `ruleConfigs` deliberately does not
  carry it, since those presets only hold rules that apply to source files.
… of crashing

Review follow-up. Three points from the review, plus one from the bot.

A manifest key that names an `Object.prototype` member aborted the rule for the
whole file. `key in allAllowedKeys` answers true for `constructor`,
`__proto__`, `toString` and `hasOwnProperty`, so the lookup below it returned
the inherited member -- a function, or `Object.prototype` itself -- and
`expectedType.includes(actualType)` threw `TypeError: expectedType.includes is
not a function`. The manifest got no `disallowedKey` report at all, which is
what an ordinary unknown key such as `eslintIgnore` correctly produces. In the
community-scanner context this rule exists for, a submitted plugin could turn a
lint into a hard rule error that way.

`Object.hasOwn(allAllowedKeys, key)` at the check routes such a key into the
`disallowedKey` branch and the lookup is never reached. `allAllowedKeys` is now
built with a null prototype as well, so the lookup could not resolve to an
inherited member even if it were reached. Four regression cases pin it, one per
name; the existing `eslintIgnore` case cannot, since that name is not on
`Object.prototype`.

The forbidden-words check ran twice per reported field -- once as the branch
condition, once for the word in the message. It is hoisted above the branch
chain and runs once. `hasForbiddenWords` returned `[boolean, string]` to serve
those two callers; it is now `findForbiddenWords`, returning the words or
`null`, which is the whole of what the single caller needs. This also removes
both call sites the review bot flagged for passing an optional
`allowedWords[key]` into a `string[]` parameter. That flag was wrong on its own
terms -- the parameter has a default, so `string[] | undefined` is assignable
and `tsc` was clean before this commit as well -- but the sites are gone either
way.

The `allowedWords` example in the rule doc omitted the `plugins` block, so the
snippet could not be used as shown. It now matches the one in
`docs/configuration.md`.
…ring key

A flag is two regional indicators and a keycap is a digit, U+FE0F and
U+20E3. None of those code points is Extended_Pictographic, so a
description carrying either passed the emoji check. The pattern now
lists them explicitly.

The disallowed-key check was guarded by `key &&`, so a member named ""
skipped it, then found no expected type and was ignored. It is now
reported like any other unknown key.

The manifest.json config snippets in docs/configuration.md and the
rule doc used `json` without importing it.
@mnaoumov

mnaoumov commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, all three were real. Fixed in 88ad302, and the branch is rebased on master.

  • Flags and keycaps: a flag is two regional indicators and a keycap is a digit plus U+FE0F and U+20E3, and none of those is Extended_Pictographic. The pattern now also matches \p{Regional_Indicator}, U+FE0F and U+20E3. I added tests for 🇺🇸 and 1️⃣, and checked that plain punctuation, accented letters and CJK text still pass.
  • Empty-string key: dropped the key && guard, so "": 1 is now reported as a disallowed key. Added a test for it.
  • json import: the snippet in docs/configuration.md now has import json from "@eslint/json". The snippet in docs/rules/validate-manifest.md had the same gap and imports both json and obsidianmd now.

I also ran it through ESLint itself with the documented config block, on a manifest with a flag in the description, a "" key and a constructor key. All three are reported.

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.

4 participants