diff --git a/.claude/skills/build-plugin/references/data-streams.md b/.claude/skills/build-plugin/references/data-streams.md index ba9fbf80..7f074dc3 100644 --- a/.claude/skills/build-plugin/references/data-streams.md +++ b/.claude/skills/build-plugin/references/data-streams.md @@ -58,6 +58,11 @@ So the rule is **push it down**: if the API can group/filter/sort, let it; other The `cost` example below does exactly this: its optional `groupBy` `choiceChips` param **is** layer-1 push-down. +**Filter and count in the API when the API caps its results.** Check which kind of cap an endpoint has: + +- **A cap on the total result** (e.g. a search that never returns more than 2,000 rows) can't be paged past. A count of the rows that came back, or a filter applied in the tile or a script after the request, silently undercounts: one shipped plugin showed 303 of 601 SLA breaches this way. The filter or count has to be an API parameter. +- **A cap per page** is recovered by [paging](#pagination), provided the endpoint honours its paging parameters. + ### Anti-pattern: five "Cost by X" streams A shipped plugin has five cost streams — `costByProject`, `costByService`, `costDailyTimeline`, `projectCosts`, `projectCostTimeline`. All five issue the **identical** request to `/v1/billing/charges` (same `getArgs`), and each `postRequestScript` does the same thing: parse the NDJSON, filter to `ChargeCategory === 'Usage'`, then group-and-sum by a different key (project / service / day) — two of them additionally filtering to a scoped project. They differ **only** by group key and scope, which is the "consolidate" column above. @@ -254,7 +259,7 @@ result = scoped.map((r) => ({ > ⚠️ Never mention dashboards, tiles, or import — every stream ends up in a dashboard somewhere, so saying so adds no information. This includes rephrasings like "used for import", "used by the X tile", "powers the X dashboard", "for monitoring X". -- `tags` — required; title case (e.g. `"User"`, `"Security"`, `"Analytics"`). Keep to a small, meaningful set, reuse existing tags from other plugins/data streams. Do not use the plugin name as a tag. +- `tags` — required on most streams; title case (e.g. `"User"`, `"Security"`, `"Analytics"`). Tags group related streams in the tile editor, so choose them across the whole plugin: not one tag shared by every stream, and not many tags that each cover a single stream. Reuse existing tags from other plugins/data streams where it makes sense. Do not use the plugin name as a tag. --- @@ -388,6 +393,8 @@ Shows an **Apply** button instead of running on every config change. Use for exp The `paging` block in `config` controls how SquaredUp fetches multiple pages. +Only declare paging an endpoint actually honours: check that its paging parameters change the response. If the endpoint ignores them, SquaredUp re-fetches the same page until the page cap and imports duplicates. `"mode": "none"` with the limit written in the README is the better choice there. Before adding paging to a stream with a post-request script, read the [per-page warning](#post-request-scripts). + **No paging:** ```json @@ -528,6 +535,8 @@ none `none` is only valid when `supportsNoneTimeframe` is set (see below). +**Apply both ends of the range.** A stream that filters by timeframe must pass both the start and the end to the API (or clamp to both in its script). Filtering only on the start, or looping up to now instead of the end, looks right for "last 7 days" but over-returns for closed windows such as `lastMonth` or `lastQuarter`. + JSON-only timeframe properties (not settable via the Save as data stream modal): ```json @@ -788,7 +797,9 @@ Appending a related object's list property this way lets tiles filter and group ## Post-request scripts -Scripts run after the HTTP response is received. Input is `data` (parsed JSON body). Set `result` to an array of row objects. (For a datasource-level script that runs _before_ every request — custom auth flows — see [Pre-request scripts](metadata.md#pre-request-scripts-custom-auth-flows) in metadata.md.) +Scripts run after the HTTP response is received. Input is `data` (parsed JSON body). Set `result` to an array of row objects. + +> ⚠️ **On a paged stream the script runs once per page and sees only that page.** SquaredUp concatenates each page's `result`. Anything that needs the whole result set is wrong once there is a second page: de-duplicating, pairing related records (an alert and the event that clears it), totals, counts or unions. Turning paging on does not fix this, it causes it. Either derive the value from data within each record (one plugin used each alert's own link to its clearing event instead of searching the page for it), push the work to the API, or leave it to the tile's `group`/`aggregate`, which runs on the combined rows. (For a datasource-level script that runs _before_ every request — custom auth flows — see [Pre-request scripts](metadata.md#pre-request-scripts-custom-auth-flows) in metadata.md.) ### Wiring a script to a stream @@ -923,6 +934,8 @@ Return actual JS number primitives for numeric columns — returning `"29.19"` ( ### Deduplication pattern +This de-duplicates within one response. On a paged stream it only removes duplicates within each page (see the warning under [Post-request scripts](#post-request-scripts)). + ```javascript // dataStreams/scripts/myStream.js const seen = new Set(); diff --git a/.claude/skills/build-plugin/references/oob-content.md b/.claude/skills/build-plugin/references/oob-content.md index 98dd7634..f06f93e1 100644 --- a/.claude/skills/build-plugin/references/oob-content.md +++ b/.claude/skills/build-plugin/references/oob-content.md @@ -126,7 +126,7 @@ Single `.dash.json` files reference as `"type": "dashboard"`. Folders map to sub The tile-level `"scope"` block above is for streams that are **natively scoped** through their `matches`. A stream that is unscoped at the definition level (`"matches": "none"`) but exposes an optional `objects` parameter — the consolidation pattern in [data-streams.md](data-streams.md#one-stream-per-shape) — auto-scopes on a perspective a different way: **bind the dashboard variable into the parameter** under `dataStream.dataSourceConfig`. -The key under `dataSourceConfig` is the `name` of the stream's `objects` field. Given a stream with `"ui": [{ "type": "objects", "name": "project", ... }]`, the per-object perspective dashboard binds: +The key under `dataSourceConfig` is the `name` of the stream's `objects` field. Stream parameters on a tile always go under `dataStream.dataSourceConfig`. The platform silently ignores anything placed under `dataStream.config`, so the tile runs with defaults and looks plausible but wrong. Given a stream with `"ui": [{ "type": "objects", "name": "project", ... }]`, the per-object perspective dashboard binds: ```json "dataStream": { @@ -254,6 +254,8 @@ Common tiles, with the **exact** column names the chart's `field`s must use: - **Do not repeat the plugin name in dashboard names.** The name appears beneath the plugin name in the UI — "MyPlugin / Overview" is correct; "MyPlugin / MyPlugin Overview" is redundant. - **Give each dashboard a distinct name.** Perspective tabs sit next to each other — identical names are indistinguishable. - `"variables"` array supports **only one variable** per dashboard. Design each dashboard around a single object type. +- **Every variable a tile references must be declared** in the dashboard's `"variables"` array, otherwise the binding resolves to nothing. +- **Keep request counts low on overview tiles.** A tile over an `httpRequestScopedSingle` stream sends one request per object in scope, so a summary of 50 environments is 50 API calls every time the dashboard loads. (Other scoped streams can send all the objects in one request.) Prefer a single list call, or a property captured at import, for account-wide summaries; keep per-object streams for drilldown dashboards. - **A tile's `"timeframe"` is driven by its data stream, not by style.** If the tile's stream is **timeframe-independent** (`"timeframes": false` — a current-state / snapshot stream that ignores any range), set `"timeframe": "none"` on the tile. If the stream **supports timeframes**, **omit** `timeframe` so the tile tracks the dashboard's timeframe — setting `"none"` here pins the tile and stops it following the dashboard. (This is why most summary tiles, built on snapshot streams, carry `"none"`.) - All tile IDs (`"i"`) must be **genuinely random UUIDs** — generate with `node ".claude/skills/build-plugin/scripts/gen-uuids.js" [N]`. Never invent patterned UUIDs. @@ -334,3 +336,11 @@ These are standalone tiles with no `dataStream`, `scope`, `activePluginConfigIds | `{{scopes.[ScopeName]}}` | A scope by name from `scopes.json` | | `{{dataStreams.[streamName]}}` | The data stream's ID | | `{{variables.[VariableName]}}` | The variable defined on a scope | + +**Escape every other `{{…}}` with `{{{{raw}}}}`.** These tokens are substituted when the default content is installed, and any other `{{…}}` in a `.dash.json` is substituted too, normally to nothing. Text that SmartViz itself fills in at render time therefore has to be wrapped so it reaches the tile intact. Typical cases are `labelFormat`, `cartesianTooltipTemplate` and `statusGridSecondaryTemplate`: + +```json +"labelFormat": "{{{{raw}}}}{{value}}{{{{/raw}}}}" +``` + +A tile whose label or tooltip comes out blank after install usually has an unwrapped template. diff --git a/.claude/skills/deploy-plugin/SKILL.md b/.claude/skills/deploy-plugin/SKILL.md index 64ce57d1..3be0d5c8 100644 --- a/.claude/skills/deploy-plugin/SKILL.md +++ b/.claude/skills/deploy-plugin/SKILL.md @@ -76,7 +76,7 @@ Downstream callers (e.g. Checkpoint A) should check the `valid` field to disting ## Versioning -New plugins start at `1.0.0`. Use semver: +New plugins start at `.0.0`, where `N` matches the `v/` folder (`1.0.0` in `v1/`, `2.0.0` in `v2/`). Use semver: | Change type | Bump | |---|---| @@ -84,7 +84,7 @@ New plugins start at `1.0.0`. Use semver: | New stream, new optional config field, new default content | MINOR (`1.x.0`) | | Deleted/renamed stream, breaking config change | MAJOR (`x.0.0`) | -Versions only matter at merge, so they are compared against `main`. A PR that modifies an existing plugin must end with a `version` higher than the one on `main` — that is **one bump for the whole PR, not one per commit**, so don't bump again on each review round. A brand-new plugin has nothing on `main` to compare against: leave it at `1.0.0` until it merges. +Versions only matter at merge, so they are compared against `main`. A PR that modifies an existing plugin must end with a `version` higher than the one on `main` — that is **one bump for the whole PR, not one per commit**, so don't bump again on each review round. A brand-new plugin has nothing on `main` to compare against: leave it at `.0.0`, matching its `v/` folder, until it merges. **Breaking (MAJOR) changes — do not create a new major version without asking the user first.** It is often possible to avoid the break entirely. If a major version is genuinely needed: - Create a new versioned folder (e.g. `v2/`) rather than modifying `v1/` diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000..40d87f2d --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,147 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# +# CodeRabbit configuration. The review rules themselves live in REVIEW.md so +# that humans, AI coding agents and CodeRabbit all work from one source; keep +# this file to settings and to pointers into that document. +language: en-GB +tone_instructions: >- + Be concise and neutral. Lead with the impact on the plugin user. Prefer one + comment listing every affected file over one comment per file. + +reviews: + profile: chill + high_level_summary: true + collapse_walkthrough: true + changed_files_summary: true + sequence_diagrams: false + estimate_code_review_effort: false + related_prs: false + suggested_labels: false + suggested_reviewers: false # CODEOWNERS already routes reviews + poem: false + in_progress_fortune: false + + # Approve once the latest commit is reviewed, its threads are resolved, + # pre-merge checks pass and merge risk is low. CodeRabbit also requests + # changes while blocking findings are open. It is not a code owner, so it + # does not stand in for a CODEOWNERS review. + auto_approvals: + enabled: true + max_merge_risk: low + # Authors can't use "@coderabbitai approve" on their own PRs. + allow_author_approval: false + overrides: + # Never approve changes to the reviewer's own rules, CI, or the agent + # skills that outside users install. + - paths: + - ".coderabbit.yaml" + - "REVIEW.md" + - "AGENTS.md" + - "CLAUDE.md" + - ".claude/**" + - ".github/**" + enabled: false + merge_risk_instructions: | + Higher risk: + - Changes to a plugin that already exists on main that remove or rename + data streams, source types, object IDs or UI fields, since they break + customers' dashboards or force a reimport. + - Changes to authentication: preRequest.js, OAuth or token handling, + base.config headers and URL patterns. + - Import changes (indexDefinitions, import streams, pageSize), which can + fail or overload a customer's import. + - Anything that sends credentials or tenant data somewhere new. + Lower risk: + - A new plugin, which affects no existing customer. + - Dashboard layout, wording, README and help text changes. + - A version bump with a small, contained fix. + + auto_review: + enabled: true + drafts: true + # Plugins often go through many small review rounds; the default of 5 + # left final commits unreviewed on several PRs. + auto_pause_after_reviewed_commits: 15 + + path_instructions: + - path: "plugins/**" + instructions: | + Apply REVIEW.md, including the "How to review" and "Platform facts" + sections. Before asking for a version bump, check whether + plugins//v/metadata.json is an added file in this PR. If it + is, the plugin is new: it must be .0.0 and needs no bump. + .claude/skills/build-plugin/references/ is the authority on platform + behaviour (shapes, paging, timeframes, imports, dashboards). Check it + before claiming a field, shape or value is unsupported. + Do not report Biome's "Illegal return statement" on preRequest.js or + on scripts under dataStreams/scripts/: the platform runs each script + inside a function, so a top-level return is valid. + - path: "plugins/**/dataStreams/**" + instructions: | + Check scripts, paging, timeframes and capped results against + .claude/skills/build-plugin/references/data-streams.md, in particular + its warnings on per-page post-request scripts, timeframe bounds and + APIs that cap their results. + - path: "plugins/**/defaultContent/**" + instructions: | + Check tiles against .claude/skills/build-plugin/references/oob-content.md + (Dashboard rules, dataSourceConfig and Templating tokens) and the + SmartViz notes in REVIEW.md's Platform facts. Flag any {{...}} that + is not a templating token and not wrapped in {{{{raw}}}}. + - path: ".claude/skills/**" + instructions: | + These are instructions for AI agents, not plugin files. Do not apply + plugin rules (versioning, CODEOWNERS, one plugin per PR) here. Flag + guidance that is wrong, contradicts REVIEW.md or another reference, or + copies an explanation found in another reference. REVIEW.md restates + rules as a short reviewer checklist on purpose; do not flag that + overlap. AGENTS.md says which file is authoritative for each topic. + + pre_merge_checks: + docstrings: + mode: "off" # plugins are JSON plus small scripts; coverage is meaningless here + description: + mode: "off" # replaced by the template check below + custom_checks: + - name: "PR template" + mode: "warning" + instructions: | + The PR description must follow one of the templates in + .github/PULL_REQUEST_TEMPLATE/: "Add a new plugin", "Change to an + existing plugin" or "Miscellaneous change". Pass only if the body + contains that template's section headings. For "Add a new plugin" + the "Test plan" section, and for "Change to an existing plugin" the + "Testing" section, must also have real content. "Miscellaneous + change" has no testing section, so do not require one. Extra + sections the author adds are fine. "Miscellaneous change" is for + PRs that do not add or change a plugin: if files under plugins/ + change, fail it unless the body declares a repository-wide change + (for example, removing one kind of file from every plugin); CI's + scope check will still fail such a PR, and a maintainer merges past + it. Fail if + the body is free text, or is still the template picker from + .github/pull_request_template.md. + + tools: + # Biome stays on (the default) for its script and duplicate-key checks. + # Its "Illegal return statement" false positive on scripts is + # suppressed by the plugins/** path instruction above. + # SkillSpector stays on too: outside users install the skills in + # .claude/skills/, so prompt-injection checks are worth some noise. + # LanguageTool's grammar suggestions cut across the REVIEW.md wording + # rules. + languagetool: + enabled: false + +knowledge_base: + code_guidelines: + enabled: true + filePatterns: + - "REVIEW.md" + - "AGENTS.md" + - ".claude/skills/build-plugin/references/*.md" + learnings: + scope: local + # Several learnings saved from single PRs contradicted REVIEW.md. Give + # maintainers a week to reject a learning before it applies. + approval_delay: 7 diff --git a/AGENTS.md b/AGENTS.md index 837a6a95..9ed4dde2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,7 +29,8 @@ Read the relevant file rather than inferring conventions from nearby plugins: | Document | Authoritative for | | --- | --- | -| [`REVIEW.md`](REVIEW.md) | Conventions for every file in a plugin — naming, metadata, `ui.json` fields, data streams, dashboards, docs. **Read it before editing anything under `plugins/`.** It is phrased for reviewers, but the same rules govern authoring. | +| [`REVIEW.md`](REVIEW.md) | Conventions for every file in a plugin — naming, metadata, `ui.json` fields, data streams, dashboards, docs. **Read it before editing anything under `plugins/`.** It is phrased for reviewers, but the same rules govern authoring. Its *Platform facts* section lists SquaredUp behaviour that is easy to get wrong. | +| [`.coderabbit.yaml`](.coderabbit.yaml) | CodeRabbit's review settings. The review rules stay in `REVIEW.md`; this file holds only settings and short pointers to it. | | [`.claude/skills/deploy-plugin/SKILL.md`](.claude/skills/deploy-plugin/SKILL.md) | CLI commands, `--json` output shapes, and versioning. | | [`.claude/skills/build-plugin/references/`](.claude/skills/build-plugin/references/) | Per-file depth: `metadata.md`, `ui.md`, `data-streams.md`, `index-defs.md`, `oob-content.md`, `readme.md`, `testing.md`. | | [`.github/PULL_REQUEST_TEMPLATE/`](.github/PULL_REQUEST_TEMPLATE/) | What a pull request must cover. Three templates: new plugin, change to an existing plugin, miscellaneous. | @@ -43,7 +44,8 @@ Read the relevant file rather than inferring conventions from nearby plugins: plugins//v/ one folder per plugin, per major version .claude/skills/ build-plugin, deploy-plugin, convert-dashboard .github/workflows/ validate, deploy and clean up PR plugins -REVIEW.md plugin conventions +REVIEW.md plugin conventions (also CodeRabbit's review rules) +.coderabbit.yaml CodeRabbit settings ``` ## Anatomy of a plugin folder @@ -76,3 +78,12 @@ What each file is, in rough order of how universal it is: - **`main-deploy.yaml`** dispatches to `squaredup/plugins-automation` to release on merge. So validate locally before you push — CI runs the same command and will block the PR on the same errors. + +## Responding to CodeRabbit review comments + +CodeRabbit reviews every PR against `REVIEW.md`. Its comments include a "Prompt for AI agents" block, but don't apply it unchecked: + +- **Verify before applying.** If a finding conflicts with `REVIEW.md`, the build-plugin references, or what you saw in a live deployment, reply with the evidence instead of changing the code. CodeRabbit withdraws findings when shown live results. +- **Apply the smallest fix.** Don't take on extra hardening the finding didn't ask for. +- **Don't bump the version per review round.** The rules are in `REVIEW.md` under *Versioning*. A new plugin stays at `.0.0`. +- **Tell the maintainers when a rule is wrong.** If CodeRabbit repeatedly gets a platform fact wrong, propose an addition to *Platform facts* in `REVIEW.md` rather than relying on a CodeRabbit learning. diff --git a/REVIEW.md b/REVIEW.md index ce1ee25d..3d510a6e 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -1,47 +1,88 @@ When reviewing code, focus on: +## How to review + +These rules govern how findings are raised. They exist because the same mistakes recurred across many reviews. + +- **The repository's references beat general knowledge.** `.claude/skills/build-plugin/references/` documents how the SquaredUp platform actually behaves (shapes, paging, timeframes, imports, dashboards). Before claiming something is unsupported, broken or undocumented, check there and in the [Platform facts](#platform-facts) section below. Do not reason from generic JavaScript, generic REST, or a vendor's public docs alone. +- **Live results beat documentation.** Plugins are tested against live APIs. When an author reports what the live API or a deployed dashboard actually did, accept it and withdraw the finding. Do not ask authors to undo behaviour or limits they measured. +- **Uncertain API behaviour is a question, not a defect.** If a finding depends on how the upstream API pages, filters or shapes a field, and neither the diff nor the vendor docs prove it, phrase it as a question at Minor severity. Do not raise hypothetical response variants (missing fields, alternative error bodies, malformed values) unless the docs show them. +- **Calibrate severity.** Major or Critical is for data that is wrong, missing or silently empty, broken auth, leaked secrets, or a failed import or deploy. Convention and wording issues are Minor at most. +- **Raise each pattern once.** When the same issue appears in several files (copied scripts, repeated tiles), post one comment that lists every location rather than one comment per file. +- **Everything in the first review.** Do not add new findings on code that has not changed since the previous review. Do not re-raise a finding the author fixed, deliberately declined, or a maintainer overruled. +- **Do not restate the PR description.** Known limitations, release dependencies and declared exceptions the author has already written down are not findings. +- **Do not edit files that should not be committed.** If a PR contains a file that does not belong (see [Code formatting](#code-formatting)), flag it for removal rather than suggesting changes to its contents. +- **Keep the summary current.** Do not repeat withdrawn or resolved findings in the walkthrough, merge-risk or final summaries. + ## Pull requests -- **One plugin per pull request.** A PR should add or modify a single plugin. If a change spans several plugins, ask the author to split it into one PR per plugin — this keeps review focused, routes CODEOWNERS correctly, and keeps the per-plugin validate/deploy checks meaningful. -- **The correct PR template must be used** — `Add a new plugin`, `Change to an existing plugin`, or `Miscellaneous change` (see `.github/PULL_REQUEST_TEMPLATE/`). PRs raised without the appropriate template may be closed. +- **One plugin per pull request.** A PR should add or modify a single plugin. If a change spans several plugins, ask the author to split it into one PR per plugin. This keeps review focused, routes CODEOWNERS correctly, and keeps the per-plugin validate/deploy checks meaningful. The exception is a repository-wide change that the PR description declares as such (for example, removing a field from every plugin); do not raise this rule on those. CI's scope check does not read the description, so it still fails such a PR and a maintainer merges past it (as with #148 and #178). Do not report that CI failure as a finding. +- **The correct PR template must be used**: `Add a new plugin`, `Change to an existing plugin`, or `Miscellaneous change` (see `.github/PULL_REQUEST_TEMPLATE/`). Check the PR body actually follows the selected template, with its headings present. For `Add a new plugin` the test plan, and for `Change to an existing plugin` the testing section, must be filled in; `Miscellaneous change` has no testing section. A body that is free text, or the template picker with no template applied, does not pass. PRs raised without the appropriate template may be closed. ## Versioning Versions are compared against `main`, because merging is the only point at which a version matters. A PR that modifies an existing plugin must leave `metadata.json` with a `version` higher than the one on `main`. If it doesn't, assume the task is unfinished and prompt to add one. +- **Is this plugin new?** Decide from the diff, not from whether plugin files changed: if `plugins//v/metadata.json` is an **added** file in this PR, the plugin (or major version) is new and none of the bump rules below apply to it. Do not request a version bump on a new plugin, and do not tuck one into another finding. - **One bump per PR, not per commit.** Once the version is above `main`, later commits and review rounds on the same PR do not each need another bump. Do not ask for one. -- **A new plugin stays at `1.0.0`.** There is no version on `main` to compare against, so it stays at `1.0.0` for the life of the PR however many review rounds it takes. Do not ask for a bump. +- **A new plugin stays at `.0.0`**, where `N` matches its folder (`1.0.0` in `v1/`, `2.0.0` in `v2/`). There is no version on `main` to compare against, so it stays there for the life of the PR however many review rounds it takes. Do not ask for a bump. Do flag a new plugin that arrives at any other version (for example `1.0.14`) and ask for `.0.0`. - **The major version must match the folder.** A plugin in `v1/` is `1.x.y`, a plugin in `v2/` is `2.x.y`. A new major version folder therefore starts at `.0.0`, not `1.0.0`. -- Breaking changes (e.g. removing or renaming a data stream, significantly changing UI parameters) require a new major version **folder** (e.g. `plugins/MyPlugin/v2/`), not just a version bump within the existing folder. The old folder must remain to avoid breaking existing users. +- Breaking changes (e.g. removing or renaming a data stream, significantly changing UI parameters) require a new major version **folder** (e.g. `plugins/MyPlugin/v2/`), not just a version bump within the existing folder. The old folder must remain to avoid breaking existing users. This only applies to plugins that already exist on `main`; an unmerged plugin can change freely. +- **Do not ask to rename source types or object IDs on a plugin already on `main`.** It forces every customer to reimport and breaks their dashboards. Raise it only on new plugins. - An earlier major version is not always present here. Some plugins' earlier versions are closed-source high-code plugins maintained outside this repository, so they start at `v2/` (e.g. `plugins/UptimeRobot/`). A missing `v1/` is not automatically a mistake. ## Security -- Check for hardcoded secrets, API keys, or credentials +- Check for hardcoded secrets, API keys, or credentials, including in debug or log output from scripts. +- Flag imported object names or properties built from URLs that may carry tokens or signed query strings (e.g. webhook URLs). +- Flag URL `pattern` validation that would let a user-supplied base URL send credentials to an unintended host. +- Do **not** require HTTPS-only URL patterns. Self-hosted targets are commonly reached over HTTP through a relay agent, and a `ui.json` pattern cannot depend on the auth mode. At most, suggest a note in the docs. ## Code formatting - Follow existing formatting in the repo - Use consistent naming -- Do not introduce formatting tools or config files unless explicitly requested. -- Do not commit **personal** editor or AI tool configuration (e.g. `.claude/settings.json`, anything matching `*.local.json`) — that belongs in the user's home directory. Shared instructions that the whole project relies on are a different thing and are committed deliberately: `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING.md`, `.claude/skills/` and `.vscode/settings.json` all belong here. +- Do not introduce formatting tools or config files unless explicitly requested. This includes `cspell.json`, linter configs or similar, anywhere in the repository: flag them for removal. +- Do not commit **personal** editor or AI tool configuration (e.g. `.claude/settings.json`, anything matching `*.local.json`) — that belongs in the user's home directory. Shared instructions that the whole project relies on are a different thing and are committed deliberately: `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING.md`, `.coderabbit.yaml`, `.claude/skills/` and `.vscode/settings.json` all belong here. +- Flag byte-identical scripts copied across several data streams; a shared script file can be referenced instead. ## CODEOWNERS -- If the user is adding a new plugin, encourage them to update the .github/CODEOWNERS file so they can help review future contributions. +- If the user is adding a new plugin, encourage them to update the `.github/CODEOWNERS` file so they can help review future contributions. This is encouraged, not required: do not raise it as a blocking finding. +- Entries must use the anchored, recursive form `/plugins//`. `plugins//*` only matches direct children, so it misses every file in `v1/`. ## Suggesting changes When suggesting changes: -- Assume they will be reviewed by humans +- Assume they will be reviewed by humans, and often applied verbatim by AI coding agents. Suggest the smallest change that fixes the problem; do not bundle speculative hardening. - Optimise for reviewability - Keep diffs focused and minimal - If a change is non-obvious, add a short comment explaining intent. - Comments should explain **why**, not restate **what** the code does - Use a neutral, professional tone - Avoid humour, sarcasm, or emojis in code comments -- Include all review comments in the first review, do not add additional review comments to code that hasn't changed since the previous commit. + +## Platform facts + +SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as established; each was confirmed against live deployments. To avoid drift, a fact that the build-plugin references document is a one-line pointer here and the reference holds the detail. Facts without a link are not documented anywhere else; when one is added to a reference, replace it here with a pointer. + +- **Post-request scripts run once per page.** Logic that needs the whole result set is a bug on a paged stream, and enabling paging is not a fix for it. See [data-streams.md, Post-request scripts](.claude/skills/build-plugin/references/data-streams.md#post-request-scripts). +- **Paging config is not proof the API pages.** A `paging` block can just chain a second request through `pagingContext`, and `mode: none` with a documented limit is right for an endpoint that ignores paging parameters. See [data-streams.md, Pagination](.claude/skills/build-plugin/references/data-streams.md#pagination). +- **Shapes parse and format values.** + - `date` and `timestamp` columns accept ISO 8601 strings, `YYYY-MM-DD HH:MM:SS+00`-style strings, and Unix timestamps when the column declares an `inputPattern` for them (e.g. `"inputPattern": "t"` for seconds, as OpenRouter's `created` column does). Only raw epoch numbers with no `inputPattern` need converting. + - The data-size shapes (`bytes`, `megabytes` and so on) all auto-scale; only flag one that doesn't match the API's unit. See [data-streams.md, Shapes](.claude/skills/build-plugin/references/data-streams.md#shapes). + - `decimalPlaces`, `thousandsSeparator` and `formatExpression` inside a shape are display formatting, not pre-formatting. Rounding in a script is different: it throws precision away before tiles sum or average the values. When the shape already sets `decimalPlaces`, suggest removing the script rounding as a Minor finding. + - SmartViz applies shapes whichever renderer a tile uses. + - In a `state` shape, `map` lists the raw values for each state (e.g. `{ "success": ["Good"] }`), so the column still emits the raw labels. +- **SmartViz matching and defaults.** SmartViz is documented by the platform's `squaredup skill visualize`, not in this repository; these points are kept here for reviewers only. Colour and value maps (`colors`, `mappedColors`, row highlights) match keys case-insensitively. `gaugeBands[].to` is a 0–1 fraction of the gauge range, not an absolute value. `ringGridMax` defaults to 100. The scalar `comparison` displays the value it is given; it does not calculate a delta. +- **Mustache expressions.** Unset variables resolve to an empty string; they do not throw. Do not suggest `typeof` guards. +- **Configuration validation warns, it does not block.** A failing `configValidation` step shows a warning; the user can still save the data source. +- **Dashboard variables and pickers.** A perspective tile that declares a variable does not run with an empty selection; it shows "No objects selected". Object pickers only offer objects that have been imported. +- **Tile parameters live under `dataSourceConfig`.** Parameters placed under `dataStream.config` on a tile are silently ignored. Flag them. See [oob-content.md](.claude/skills/build-plugin/references/oob-content.md#auto-scoping-a-stream-via-its-objects-filter). +- **`autocomplete` fields store objects**, so interpolating one straight into a header, path or query sends `[object Object]`. See [ui.md, Overview](.claude/skills/build-plugin/references/ui.md#overview) (field value shapes). +- **`preRequestScript` needs `showScripting: true`**, or the script is silently skipped. See [metadata.md, Wiring](.claude/skills/build-plugin/references/metadata.md#wiring). +- **Imports.** Array properties are JSON-stringified when written to the graph, so they cannot be used as correlation join keys. Metadata columns that are not listed are not displayed. The index refreshes on a schedule (hours, not seconds), so fast-changing records such as alerts or incidents belong in live data streams, not imports. ## File-specific guidelines @@ -50,17 +91,18 @@ When suggesting changes: - SVG is preferred - Should be a square icon - Less than 100KB in size +- Should be the vendor's real, current logo. Ask the author to confirm the source if it looks redrawn or generated. ### Metadata (metadata.json) - name - Should generally be the lowercase kebab style version of the displayName, e.g. phare or google-sheets -- displayName - Use the correctly styled/cased official product name for display names, e.g. SharePoint NOT sharepoint +- displayName - Use the correctly styled/cased **current** official product name for display names, e.g. SharePoint NOT sharepoint, Checkmk NOT CheckMK. Check the vendor has not renamed the product. The plugin folder name should use the same casing. - description - One short sentence describing what users can build or monitor. Avoid API or implementation language like Access HaloPSA APIs and query ticket data. Should always be appropriately punctuated, e.g. ending with a full stop. - schemaVersion - Should be 2.0 or higher. -- version - The version number MUST be increased for any change to the plugin. It can never decrease. If a breaking change is made, the major version number of the plugin should be increased - for example, when deleting a data stream or significantly modifying the UI parameters. +- version - See [Versioning](#versioning). - author.type - Should be `community` for community-contributed plugins, or `labs` for SquaredUp-authored experimental plugins. - author.name - Should typically be a GitHub username, prefixed with @ OR an organisation name. For example `@username1` or `Contoso Inc.` -- category - Mandatory. Reuse an existing category from other plugins where possible. +- category - Mandatory. Reuse an existing category from other plugins where possible. Search the `category` values in other plugins' `metadata.json` before calling one invalid. - links - Should typically contain two links, one link with `category: source` linking to the GitHub repository, and another link with `category: documentation` linking to the markdown documentation in the repository. The links can be in any order, and there may be other links. ### UI Configuration (ui.json) @@ -68,8 +110,10 @@ When suggesting changes: - Generally prefer API tokens or OAuth where possible, flag usage of username/password unless the API offers no alternative. - Check that only strictly required fields are marked as `required: true`. Advanced options should never block first-time success. - Flag any field named `ignoreCertificateErrors`, and any `ignoreCertificateErrors` mapping in `base.config` in metadata.json. The platform adds this toggle to every plugin built on WebAPI. The only exception is a literal `false` in `base.config`, which deliberately hides it. +- Use a `toggle` for on/off settings, not a two-option dropdown. +- Question configuration fields that expose internals the user has no reason to change (e.g. page limits). - For each field in the array: - - displayName - First word uppercase, then lowercase (e.g. “Table name", or "API key"). Single value: singular. Multiple values: “(s)" (e.g. Tables name(s)). Do not use “you" “Your" in display names. Keep text neutral, concise, and descriptive. + - displayName - First word uppercase, then lowercase (e.g. “Table name", or "API key"). Single value: singular. Multiple values: “(s)" (e.g. Tables name(s)). Do not use “you" “Your" in display names. Do not repeat the product name (e.g. "API key", not "Contoso API key"). Keep text neutral, concise, and descriptive. - name - Should typically be the camel-cased version of the displayName. - help - Do not use tooltips unless they add specific value. Never state the obvious (e.g. “Enter the API key here"). Start with a verb where possible (e.g. “Supports the ServiceNow filtering definition format"). Encourage the author to include a reference link if relevant `Create an API key in the [Phare portal](https://docs.phare.io/api-reference/introduction)` - placeholder - Mandatory for text fields. Use example placeholders (especially for URLs or values that follow a fixed pattern, e.g. rootly_xxxxxxxxx or https://organisation.atlassian.net) or “Enter the [data source] [info needed in lowercase]". Use default values instead of hint text where a value is commonly the same across environments (e.g. default ports). @@ -81,6 +125,10 @@ When suggesting changes: - Tile names - Use title case - IDs - All `id` fields must be GUIDs (e.g. `"a1b2c3d4-e5f6-7890-abcd-ef1234567890"`), not arbitrary strings. - Tiles on a dashboard should NOT use deprecated streams - these are any streams set to { visibility: { type: 'deprecated' }} +- Variables - Every variable a tile references must be declared on the dashboard. +- Template text - Any `{{…}}` other than the templating tokens (`configId`, `workspaceId`, `scopes.[…]`, `dataStreams.[…]`, `variables.[…]`) is substituted, normally to nothing, when the default content is installed. Flag it unless it is wrapped as `{{{{raw}}}}…{{{{/raw}}}}`. Common places are `labelFormat`, `cartesianTooltipTemplate` and `statusGridSecondaryTemplate`. See [oob-content.md, Templating tokens](.claude/skills/build-plugin/references/oob-content.md#templating-tokens). +- Request volume - Flag overview tiles that send one request per object (per environment, per device) where an imported property or a single list call would do. +- Timeframes - Check the stream's `timeframes` before calling a tile's timeframe redundant or wrong; a stream may only offer one range. ### Out-of-the-box dashboard manifest (defaultContent/\*\*/manifest.json) @@ -88,17 +136,21 @@ When suggesting changes: ### Data streams - (dataStreams/\*.json) -- displayName - Use noun-based names describing the returned data, e.g. Tickets. Avoid verbs such as Get, Fetch, Run, Execute. +- displayName - Use noun-based names describing the returned data, e.g. Tickets. Avoid verbs such as Get, Fetch, Run, Execute. This applies to the data stream's own name, not to its column names. - description - Typically one sentence only. No full stop at the end. Add relevant clarifications in brackets. Never use two sentences. -- tags - Mandatory. Reuse an existing category from other plugins where possible, never use the plugin's name in a tag. -- objectLimit - If a data stream only processes a single object (e.g. uses `objects[0]` in its expression), suggest either using the `httpRequestScopedSingle` dataSource (HTTP Request with Objects (request per object)) OR setting `objectLimit: 1`. -- timeframe - Data streams that don't support time-based filtering must explicitly declare `"none"` as an available timeframe. Omitting this causes UI warnings and incorrect default behaviour in OOB dashboards. -- Numeric values - Return raw numeric values rather than pre-formatted strings (e.g. return `1234.56` not `"£1,234.56"`). Use column format expressions in dashboards for display formatting. +- tags - Required on most data streams. Tags group related streams in the tile editor, so check the set across the plugin rather than stream by stream: flag a tag applied to every stream (it distinguishes nothing) and flag many tags that each cover only one stream (merge them into a broader shared tag). Reuse existing tags from other plugins where it makes sense, and never use the plugin's name in a tag. +- objectLimit - Only applies to scoped streams. If a scoped stream only processes a single object (e.g. uses `objects[0]` in its expression), suggest either using the `httpRequestScopedSingle` dataSource (HTTP Request with Objects (request per object)) OR setting `objectLimit: 1`. Also suggest an `objectLimit` where several objects are joined into one GET request, since long query strings fail with HTTP 414. Never raise it on unscoped (`"matches": "none"`) streams. +- timeframe - Data streams that don't support time-based filtering must explicitly declare that, using either `"timeframes": false` or `"none"` in the timeframe list (which also needs `"supportsNoneTimeframe": true`). Both are accepted; do not ask to switch one for the other. Omitting this causes UI warnings and incorrect default behaviour in OOB dashboards. +- Timeframe filtering - A stream that filters by timeframe must apply both the start and the end. A script that only applies the lower bound, or loops up to now instead of `timeframe.end`, returns wrong rows for closed past windows such as last month. +- Numeric values - Return raw numeric values rather than pre-formatted strings (e.g. return `1234.56` not `"£1,234.56"`). Use column format expressions in dashboards for display formatting. Shape options such as `decimalPlaces` are display formatting and are fine (see [Platform facts](#platform-facts)). +- Sentinel values - Flag API placeholder values (`-1`, `0001-01-01`, `"None"`) passed through as real data; they should become `null`. - Column shapes - Use the `bytes` shape for columns representing byte sizes; SquaredUp will automatically display the most appropriate unit (e.g. 4.2 GB). Check for other applicable semantic shapes such as `timestamp` and `duration`. - List values - A column holding several values (tags, topics, addresses) uses `"shape": ["array", { "itemShape": "" }]`, not a comma-joined string, and not a second display column alongside it. An `objectPropertyPath` column reading a list property declares the same shape. -- Timestamps - SquaredUp expects ISO 8601 strings for timestamp columns. If the upstream API returns Unix timestamps, the script must convert them. +- Timestamps - SquaredUp expects timestamps the `date` shape can parse (see [Platform facts](#platform-facts)). If the upstream API returns Unix timestamps and the column has no `inputPattern`, the script must convert them. This is a Minor finding. - Expressions - Where a transformation can be expressed using `map`, prefer it over a mustache-style value expression, or complex processing in a script for performance. - Deduplication - If multiple data streams share the same API endpoint and differ only by a filter, consider merging them into a single stream with a UI parameter to control the filter. +- Capped results - Flag counts or totals calculated from a list whose total result the API caps (e.g. a search that never returns more than 2,000 rows), and filtering that happens in a script after such a request. Both silently undercount; the filter or count belongs in the API request. A per-page cap that the stream pages past is not a finding. +- Optional request parameters - Omit an optional parameter when it is empty rather than sending an empty string, which many APIs reject. - ui - displayName - First word uppercase, then lowercase (e.g. “Table name"). Single value: singular. Multiple values: “(s)" (e.g. Tables name(s)). Do not use “you" “Your" in display names. Keep text neutral, concise, and descriptive. - help - Use extremely sparingly. Never state the obvious. Only use when something important must be understood. Start with a verb where possible (e.g. “Supports the ServiceNow filtering definition format"). @@ -106,8 +158,10 @@ When suggesting changes: ### Source types (indexDefinitions/) - Name source types after how they are referred to in the upstream product or API (e.g. `agent`, `device`). Do not prefix them with the plugin name (e.g. avoid `NinjaOne Device`). A separate friendly display name can be configured if needed (via custom_types.json). +- Object names - Objects imported under a parent (environments under projects, sites under tenants) need names that stay unique across parents, e.g. ` / `. Repeated names such as "Production" are indistinguishable in pickers. - objectMapping.properties - Never use `id`, `state`, `tags`, or a `__`-prefixed name as a property name; they fail the import at graph write. Rename instead, e.g. `{ "status": "state" }` or `{ "checkTags": "tags" }`. `rawId`, `sourceId`, `sourceType`, `name` and `label` are silently overwritten, so mapping them does nothing. -- pageSize - For APIs where a tenant can hold tens of thousands of objects, check the import stream's `pageSize` against both import limits: ~6MB per page and ~220 pages per import run. See the build-plugin skill's index-defs.md, "Large imports". +- pageSize - For APIs where a tenant can hold tens of thousands of objects, check the import stream's `pageSize` against both import limits: ~6MB per page and ~220 pages per import run. See the build-plugin skill's index-defs.md, "Large imports". An import or list request with no paging at all against a large collection is a Major finding. +- Optional steps - Import steps on endpoints that need extra scopes, permissions or licences should be marked optional so one missing permission doesn't fail the whole import. ### Documentation - (docs/README.md) @@ -117,4 +171,11 @@ When suggesting changes: - Encourage the author to include documentation for all UI fields from metadata.json unless otherwise covered by a tooltip or help text. - Encourage the author to include links to the third-party tool or their documentation when appropriate, e.g. `Browse to [unifi.ui.com](https://unifi.ui.com) > Settings -> API Keys > Create New API Key` - The language should be neutral and clear, providing guidance on how to setup and use the plugin. +- Flag boilerplate that is true of every plugin (e.g. "this plugin is read-only") and filler lines that add nothing. - Note that only the README.md is shown within SquaredUp, so do not apply these rules to other markdown files. The documentation file may use a different filename as long as it matches the URL configured in `metadata.json` under the `links` array. + +## Skills and agent instructions (.claude/skills/, AGENTS.md) + +- Flag explanations that are copied between references or into the PR templates; each explanation should live in one place. A short rule restated in REVIEW.md's checklist is deliberate and is not duplication. +- Treat edits to a skill's `description` frontmatter as significant: it controls when the skill triggers. +- Do not apply plugin rules (versioning, CODEOWNERS, one plugin per PR) to skill or documentation-only changes.