From b394d44f0b949076b5c49b184891ed6d3107c888 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 08:31:57 +0000 Subject: [PATCH 01/17] Tune CodeRabbit review from a 90-day audit of PR reviews Add .coderabbit.yaml and extend REVIEW.md with review-process rules, platform facts CodeRabbit repeatedly got wrong, and checks for issues human reviewers caught that it missed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 104 +++++++++++++++++++++++++++++++++++++++++++++++ AGENTS.md | 15 ++++++- REVIEW.md | 99 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 197 insertions(+), 21 deletions(-) create mode 100644 .coderabbit.yaml diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000..63bdcbe0 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,104 @@ +# 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 + + auto_review: + enabled: true + drafts: false + # 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 + + # CodeRabbit excludes images by default; plugin icons need checking against + # the icon rules in REVIEW.md. + path_filters: + - "plugins/**/icon.svg" + + 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. + - path: "plugins/**/dataStreams/**" + instructions: | + Post-request scripts run once per page, and the page results are + concatenated. Flag logic that needs the full result set. Do not + recommend turning on paging as a fix for that logic. Flag counts + calculated from capped lists, and timeframe filters that apply only + one bound. + - path: "plugins/**/defaultContent/**" + instructions: | + Tile parameters belong under dataSourceConfig. Every variable a tile + uses must be declared. SmartViz colour maps match keys + case-insensitively. gaugeBands[].to is a 0-1 fraction. Flag overview + tiles that send one request per object. + - 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 + duplicates content found elsewhere. + + 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 and the test plan section + has real content. Fail if the body is free text, or is still the + template picker from .github/pull_request_template.md. + + tools: + # Flags the top-level return the platform expects in preRequest.js. + biome: + enabled: false + # Grammar suggestions duplicated the REVIEW.md wording rules. + languagetool: + enabled: false + # Raised prompt-injection warnings on ordinary skill documentation. + skillspector: + 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..cc13d510 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -1,40 +1,61 @@ 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. +- **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 one, with its headings present and the test plan filled in. 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 `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. Do flag a new plugin that arrives at any other version (for example `1.0.14`) and ask for `1.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 inside a plugin folder: 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. @@ -43,6 +64,27 @@ When suggesting changes: - 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. + +- **Post-request scripts run once per page.** A `postRequestScript` sees only its own page, and the rows from each page are concatenated. Logic that needs the whole result set (de-duplication, folding related records, totals, unions) is a real bug when the stream pages; flag it. Turning on paging is therefore not a free fix for a script that aggregates. +- **Paging config is not proof the API pages.** A `paging` block can just chain a second request through `pagingContext`. `mode: none` with a documented limitation is acceptable when the endpoint ignores paging parameters, since declaring paging the endpoint ignores re-fetches the same page until the page cap. +- **Shapes parse and format values.** + - `date` and `timestamp` columns accept ISO 8601 strings, `YYYY-MM-DD HH:MM:SS+00`-style strings, and numbers when the column declares an `inputFormat` such as `unixSeconds`. Only raw epoch numbers with no `inputFormat` need converting. + - `bytes`, `kilobytes`, `megabytes`, `gigabytes` and the other data-size shapes all auto-scale. Only flag a shape that doesn't match the unit the API returns. + - `decimalPlaces`, `thousandsSeparator` and `formatExpression` inside a shape are display formatting, not pre-formatting. So is a `Math.round` that matches the column's `decimalPlaces`. + - 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.** 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. +- **`autocomplete` fields store objects.** The stored value is an array of `{ value, label }` objects, even when single-select. Interpolating it straight into a header, path or query sends `[object Object]`; it needs `.value`. +- **`preRequestScript` needs `showScripting: true`.** Without `base.config.showScripting: true` in `metadata.json`, the platform silently skips the pre-request script and every request goes out unauthenticated. +- **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 ### icon.png/svg @@ -50,17 +92,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 +111,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 +126,9 @@ 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. +- 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. +- 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. 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 `inputFormat`, 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 the API caps (e.g. a 2,000-row maximum), and filtering that happens in a script after a capped request. Both silently undercount; the filter or count belongs in the API request. +- 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 content that duplicates another reference, the PR templates or REVIEW.md; each fact should live in one place. +- 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. From b656bd8e2049ff724083053a2f541d33eb7e07e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 10:29:51 +0000 Subject: [PATCH 02/17] Clarify the tags rule and flag cspell.json anywhere Tags should group related streams: neither one tag on every stream nor many single-stream tags. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .claude/skills/build-plugin/references/data-streams.md | 2 +- REVIEW.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/skills/build-plugin/references/data-streams.md b/.claude/skills/build-plugin/references/data-streams.md index ba9fbf80..2c41cb3c 100644 --- a/.claude/skills/build-plugin/references/data-streams.md +++ b/.claude/skills/build-plugin/references/data-streams.md @@ -254,7 +254,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. Do not use the plugin name as a tag. --- diff --git a/REVIEW.md b/REVIEW.md index cc13d510..25618325 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -42,7 +42,7 @@ Versions are compared against `main`, because merging is the only point at which - Follow existing formatting in the repo - Use consistent naming -- Do not introduce formatting tools or config files unless explicitly requested. This includes `cspell.json`, linter configs or similar inside a plugin folder: flag them for removal. +- 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. @@ -138,7 +138,7 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - 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. +- 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 possible, 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. 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. From 08d649c94e0013ba763a76a6d5b01f5e7cbd2298 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 11:57:40 +0000 Subject: [PATCH 03/17] Drop the icon path filter from CodeRabbit config A lone include pattern in path_filters limits reviews to matching paths only, so CodeRabbit skipped every other file. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 63bdcbe0..4b725a1d 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -28,11 +28,6 @@ reviews: # left final commits unreviewed on several PRs. auto_pause_after_reviewed_commits: 15 - # CodeRabbit excludes images by default; plugin icons need checking against - # the icon rules in REVIEW.md. - path_filters: - - "plugins/**/icon.svg" - path_instructions: - path: "plugins/**" instructions: | From facca04fa658a59488e84e96bed4f3e152b87c37 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:02:38 +0000 Subject: [PATCH 04/17] Move platform facts into the build-plugin references Authoring agents read the references, not REVIEW.md or the CodeRabbit config, so the per-page script, paging, timeframe-bound, capped-results and dashboard facts now live there. REVIEW.md and .coderabbit.yaml point to them instead of restating them. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .../build-plugin/references/data-streams.md | 12 +++++++++++- .../build-plugin/references/oob-content.md | 4 +++- .coderabbit.yaml | 16 +++++++--------- REVIEW.md | 10 +++++----- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/.claude/skills/build-plugin/references/data-streams.md b/.claude/skills/build-plugin/references/data-streams.md index 2c41cb3c..b20e9819 100644 --- a/.claude/skills/build-plugin/references/data-streams.md +++ b/.claude/skills/build-plugin/references/data-streams.md @@ -58,6 +58,8 @@ 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.** Many endpoints return at most a fixed number of rows (e.g. 2,000), whatever paging is set to. A count of the rows that came back, or a filter applied in the tile or a script after the request, then silently undercounts: one shipped plugin showed 303 of 601 SLA breaches this way. If the endpoint caps results, the filter or count has to be an API parameter, or the stream has to page through everything. + ### 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. @@ -388,6 +390,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 +532,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 +794,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 +931,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 d7ce525e..215ed33a 100644 --- a/.claude/skills/build-plugin/references/oob-content.md +++ b/.claude/skills/build-plugin/references/oob-content.md @@ -121,7 +121,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": { @@ -249,6 +249,8 @@ Common tiles, with the **exact** column names the visualisation must reference: - **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 a scoped stream sends one request per object in scope, so a summary of 50 environments is 50 API calls every time the dashboard loads. 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. diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 4b725a1d..976618ad 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -40,17 +40,15 @@ reviews: before claiming a field, shape or value is unsupported. - path: "plugins/**/dataStreams/**" instructions: | - Post-request scripts run once per page, and the page results are - concatenated. Flag logic that needs the full result set. Do not - recommend turning on paging as a fix for that logic. Flag counts - calculated from capped lists, and timeframe filters that apply only - one bound. + 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: | - Tile parameters belong under dataSourceConfig. Every variable a tile - uses must be declared. SmartViz colour maps match keys - case-insensitively. gaugeBands[].to is a 0-1 fraction. Flag overview - tiles that send one request per object. + Check tiles against .claude/skills/build-plugin/references/oob-content.md + (Dashboard rules and dataSourceConfig) and the SmartViz notes in + REVIEW.md's Platform facts. - path: ".claude/skills/**" instructions: | These are instructions for AI agents, not plugin files. Do not apply diff --git a/REVIEW.md b/REVIEW.md index 25618325..6f8e04aa 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -66,21 +66,21 @@ When suggesting changes: ## Platform facts -SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as established; each was confirmed against live deployments. +SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as established; each was confirmed against live deployments. Where a fact links to the build-plugin references, the reference has the detail. -- **Post-request scripts run once per page.** A `postRequestScript` sees only its own page, and the rows from each page are concatenated. Logic that needs the whole result set (de-duplication, folding related records, totals, unions) is a real bug when the stream pages; flag it. Turning on paging is therefore not a free fix for a script that aggregates. -- **Paging config is not proof the API pages.** A `paging` block can just chain a second request through `pagingContext`. `mode: none` with a documented limitation is acceptable when the endpoint ignores paging parameters, since declaring paging the endpoint ignores re-fetches the same page until the page cap. +- **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 numbers when the column declares an `inputFormat` such as `unixSeconds`. Only raw epoch numbers with no `inputFormat` need converting. - `bytes`, `kilobytes`, `megabytes`, `gigabytes` and the other data-size shapes all auto-scale. Only flag a shape that doesn't match the unit the API returns. - `decimalPlaces`, `thousandsSeparator` and `formatExpression` inside a shape are display formatting, not pre-formatting. So is a `Math.round` that matches the column's `decimalPlaces`. - 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.** 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. +- **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. +- **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.** The stored value is an array of `{ value, label }` objects, even when single-select. Interpolating it straight into a header, path or query sends `[object Object]`; it needs `.value`. - **`preRequestScript` needs `showScripting: true`.** Without `base.config.showScripting: true` in `metadata.json`, the platform silently skips the pre-request script and every request goes out unauthenticated. - **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. From 2cd0458df16f50cb0200dc4f78f1fcbb59962c2a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:07:48 +0000 Subject: [PATCH 05/17] Keep Biome and SkillSpector enabled in CodeRabbit Biome's script and duplicate-key checks outweigh its one false finding on preRequest.js. SkillSpector guards skills that outside users install. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 976618ad..08d7c036 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -73,15 +73,15 @@ reviews: template picker from .github/pull_request_template.md. tools: - # Flags the top-level return the platform expects in preRequest.js. - biome: - enabled: false - # Grammar suggestions duplicated the REVIEW.md wording rules. + # Biome stays on (the default) for its script and duplicate-key checks. + # It reports "Illegal return statement" on preRequest.js, where the + # platform wraps the script in a function: dismiss that finding. + # 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 - # Raised prompt-injection warnings on ordinary skill documentation. - skillspector: - enabled: false knowledge_base: code_guidelines: From 19839c6f30411450eb6f445fa61fdf7c4b9d7ba5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:09:22 +0000 Subject: [PATCH 06/17] Review draft PRs and soften tag reuse guidance Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .claude/skills/build-plugin/references/data-streams.md | 2 +- .coderabbit.yaml | 2 +- REVIEW.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/skills/build-plugin/references/data-streams.md b/.claude/skills/build-plugin/references/data-streams.md index b20e9819..d0b238bd 100644 --- a/.claude/skills/build-plugin/references/data-streams.md +++ b/.claude/skills/build-plugin/references/data-streams.md @@ -256,7 +256,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 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. 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. --- diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 08d7c036..efcf2a70 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -23,7 +23,7 @@ reviews: auto_review: enabled: true - drafts: false + 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 diff --git a/REVIEW.md b/REVIEW.md index 6f8e04aa..28db8ede 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -138,7 +138,7 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - 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 - 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 possible, and never use the plugin's name in a tag. +- 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. 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. From e78a1b3e09d5ae60ff6996d05fc5c36103c8b79d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:19:25 +0000 Subject: [PATCH 07/17] Only require a testing section in templates that have one The Miscellaneous template has no test plan, so the PR template check failed every miscellaneous PR. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index efcf2a70..9007e4b5 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -68,9 +68,13 @@ reviews: 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 and the test plan section - has real content. Fail if the body is free text, or is still the - template picker from .github/pull_request_template.md. + 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. 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. From 493d69a45b480ac66bdef6a31e0fc16ebe47ae8f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:20:07 +0000 Subject: [PATCH 08/17] Address CodeRabbit review on the review tuning - Distinguish total-result caps from per-page caps - Limit the one-request-per-object note to httpRequestScopedSingle - Note that the none timeframe needs supportsNoneTimeframe - Use inputPattern, the documented date shape option - Suppress Biome's top-level return finding via a path instruction Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .claude/skills/build-plugin/references/data-streams.md | 5 ++++- .claude/skills/build-plugin/references/oob-content.md | 2 +- .coderabbit.yaml | 7 +++++-- REVIEW.md | 6 +++--- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.claude/skills/build-plugin/references/data-streams.md b/.claude/skills/build-plugin/references/data-streams.md index d0b238bd..7f074dc3 100644 --- a/.claude/skills/build-plugin/references/data-streams.md +++ b/.claude/skills/build-plugin/references/data-streams.md @@ -58,7 +58,10 @@ 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.** Many endpoints return at most a fixed number of rows (e.g. 2,000), whatever paging is set to. A count of the rows that came back, or a filter applied in the tile or a script after the request, then silently undercounts: one shipped plugin showed 303 of 601 SLA breaches this way. If the endpoint caps results, the filter or count has to be an API parameter, or the stream has to page through everything. +**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 diff --git a/.claude/skills/build-plugin/references/oob-content.md b/.claude/skills/build-plugin/references/oob-content.md index 3ad84126..e712daaa 100644 --- a/.claude/skills/build-plugin/references/oob-content.md +++ b/.claude/skills/build-plugin/references/oob-content.md @@ -255,7 +255,7 @@ Common tiles, with the **exact** column names the chart's `field`s must use: - **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 a scoped stream sends one request per object in scope, so a summary of 50 environments is 50 API calls every time the dashboard loads. Prefer a single list call, or a property captured at import, for account-wide summaries; keep per-object streams for drilldown dashboards. +- **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. diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 9007e4b5..2ceffe39 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -38,6 +38,9 @@ reviews: .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 @@ -78,8 +81,8 @@ reviews: tools: # Biome stays on (the default) for its script and duplicate-key checks. - # It reports "Illegal return statement" on preRequest.js, where the - # platform wraps the script in a function: dismiss that finding. + # 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 diff --git a/REVIEW.md b/REVIEW.md index 28db8ede..941433ab 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -71,7 +71,7 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - **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 numbers when the column declares an `inputFormat` such as `unixSeconds`. Only raw epoch numbers with no `inputFormat` need converting. + - `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. - `bytes`, `kilobytes`, `megabytes`, `gigabytes` and the other data-size shapes all auto-scale. Only flag a shape that doesn't match the unit the API returns. - `decimalPlaces`, `thousandsSeparator` and `formatExpression` inside a shape are display formatting, not pre-formatting. So is a `Math.round` that matches the column's `decimalPlaces`. - SmartViz applies shapes whichever renderer a tile uses. @@ -140,13 +140,13 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - description - Typically one sentence only. No full stop at the end. Add relevant clarifications in brackets. Never use two sentences. - 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. 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 - 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 timestamps the `date` shape can parse (see [Platform facts](#platform-facts)). If the upstream API returns Unix timestamps and the column has no `inputFormat`, the script must convert them. This is a Minor finding. +- 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 the API caps (e.g. a 2,000-row maximum), and filtering that happens in a script after a capped request. Both silently undercount; the filter or count belongs in the API request. From 839c356ecbb2a5fa5e620d31d5f03be197901193 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:27:22 +0000 Subject: [PATCH 09/17] Address second CodeRabbit review - Treat script rounding as a Minor finding, not display formatting - Exempt per-page caps from the capped-results rule - Reserve the Miscellaneous template for PRs that don't change plugins Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 7 +++++-- REVIEW.md | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 2ceffe39..0de1af02 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -75,8 +75,11 @@ reviews: 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. Fail if the body is free text, - or is still the template picker from + 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). Fail if + the body is free text, or is still the template picker from .github/pull_request_template.md. tools: diff --git a/REVIEW.md b/REVIEW.md index 941433ab..3db1e530 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -73,7 +73,7 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - **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. - `bytes`, `kilobytes`, `megabytes`, `gigabytes` and the other data-size shapes all auto-scale. Only flag a shape that doesn't match the unit the API returns. - - `decimalPlaces`, `thousandsSeparator` and `formatExpression` inside a shape are display formatting, not pre-formatting. So is a `Math.round` that matches the column's `decimalPlaces`. + - `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. @@ -149,7 +149,7 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - 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 the API caps (e.g. a 2,000-row maximum), and filtering that happens in a script after a capped request. Both silently undercount; the filter or count belongs in the API request. +- 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. From 1685d1704322feef316be5a7bef1df2ccb2c7d65 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:34:16 +0000 Subject: [PATCH 10/17] Document {{{{raw}}}} escaping for template text in default content Any {{...}} other than the templating tokens is substituted when default content is installed, so SmartViz templates such as labelFormat must be wrapped in {{{{raw}}}}. Caught once by CodeRabbit (#151) but not written down; EcoStruxure and LaunchDarkly still ship unwrapped templates. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .claude/skills/build-plugin/references/oob-content.md | 8 ++++++++ .coderabbit.yaml | 5 +++-- REVIEW.md | 1 + 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/.claude/skills/build-plugin/references/oob-content.md b/.claude/skills/build-plugin/references/oob-content.md index e712daaa..f06f93e1 100644 --- a/.claude/skills/build-plugin/references/oob-content.md +++ b/.claude/skills/build-plugin/references/oob-content.md @@ -336,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/.coderabbit.yaml b/.coderabbit.yaml index 0de1af02..2de9ea0c 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -50,8 +50,9 @@ reviews: - path: "plugins/**/defaultContent/**" instructions: | Check tiles against .claude/skills/build-plugin/references/oob-content.md - (Dashboard rules and dataSourceConfig) and the SmartViz notes in - REVIEW.md's Platform facts. + (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 diff --git a/REVIEW.md b/REVIEW.md index 3db1e530..fe3b47fc 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -127,6 +127,7 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - 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. From 63db0408abb536f43279581929878d6930277023 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:49:43 +0000 Subject: [PATCH 11/17] Say how declared repository-wide PRs get past CI The scope check in pr-run.yaml doesn't read the PR description, so a declared repository-wide change still fails it and needs a maintainer merge. Document that rather than imply CI allows it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 4 +++- REVIEW.md | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 2de9ea0c..53a8863e 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -79,7 +79,9 @@ reviews: 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). Fail if + (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. diff --git a/REVIEW.md b/REVIEW.md index fe3b47fc..a812a47f 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -16,7 +16,7 @@ These rules govern how findings are raised. They exist because the same mistakes ## 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 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. +- **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 one, with its headings present and the test plan filled in. 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 From adc94f2acd9aa8aac6dc7a16ef5b829d8fc2ba0c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 12:51:25 +0000 Subject: [PATCH 12/17] Let CodeRabbit approve low-risk PRs Approve once the latest commit is reviewed, threads are resolved and pre-merge checks pass, with merge risk at or below low. Never approve changes to the review rules, CI or agent skills, and don't let authors approve their own PRs by command. Describe what makes a plugin change risky so the merge-risk rating means something here. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 53a8863e..7050f711 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -21,6 +21,41 @@ 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 From bd14bdf643eecad62418d08298ba37aae2554c03 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 14:22:15 +0000 Subject: [PATCH 13/17] Only require a test plan for templates that have one Matches the PR template check in .coderabbit.yaml. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- REVIEW.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEW.md b/REVIEW.md index a812a47f..c70f981b 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -17,7 +17,7 @@ These rules govern how findings are raised. They exist because the same mistakes ## 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 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 one, with its headings present and the test plan filled in. 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. +- **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 From 2ba899b1467bd40c629c1b74a16160646de4c032 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 14:30:33 +0000 Subject: [PATCH 14/17] Scope the new-plugin version rule to the folder's major version A new v2/ plugin starts at 2.0.0, not 1.0.0, so the rule no longer conflicts with the major-version-matches-folder rule below it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- REVIEW.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEW.md b/REVIEW.md index c70f981b..f3c8292c 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -25,7 +25,7 @@ Versions are compared against `main`, because merging is the only point at which - **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. Do flag a new plugin that arrives at any other version (for example `1.0.14`) and ask for `1.0.0`. +- **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. 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. From 0bc8e946071c3c92b0a689d89498b61cdc913945 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 14:37:01 +0000 Subject: [PATCH 15/17] Match the deploy skill's new-plugin version to the folder A new v2/ plugin must start at 2.0.0 or CI's major-version check fails. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .claude/skills/deploy-plugin/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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/` From 34a11dff7163280e4716e85b18fba02686b60c46 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 14:43:09 +0000 Subject: [PATCH 16/17] Don't flag REVIEW.md's checklist copies as duplication REVIEW.md restates rules briefly for reviewers on purpose. Only flag explanations copied between references, and point to AGENTS.md for which file is authoritative. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- .coderabbit.yaml | 4 +++- REVIEW.md | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 7050f711..40d87f2d 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -93,7 +93,9 @@ reviews: 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 - duplicates content found elsewhere. + 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: diff --git a/REVIEW.md b/REVIEW.md index f3c8292c..2448a2b4 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -177,6 +177,6 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab ## Skills and agent instructions (.claude/skills/, AGENTS.md) -- Flag content that duplicates another reference, the PR templates or REVIEW.md; each fact should live in one place. +- 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. From fd23ec69db6b1cbe20b426f44113ddddb43a12b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 15:11:04 +0000 Subject: [PATCH 17/17] Point REVIEW.md platform facts at the references to avoid drift Facts the build-plugin references already document are now one-line pointers, and a duplicate of the first-review rule is removed. Facts documented nowhere else stay, with a note to swap them for pointers once a reference covers them. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FsDuPT1b16nKH9GT1WWhws --- REVIEW.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/REVIEW.md b/REVIEW.md index 2448a2b4..3d510a6e 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -62,17 +62,16 @@ When suggesting changes: - 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. Where a fact links to the build-plugin references, the reference has the detail. +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. - - `bytes`, `kilobytes`, `megabytes`, `gigabytes` and the other data-size shapes all auto-scale. Only flag a shape that doesn't match the unit the API returns. + - 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. @@ -81,8 +80,8 @@ SquaredUp behaviour that reviews have repeatedly got wrong. Treat these as estab - **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.** The stored value is an array of `{ value, label }` objects, even when single-select. Interpolating it straight into a header, path or query sends `[object Object]`; it needs `.value`. -- **`preRequestScript` needs `showScripting: true`.** Without `base.config.showScripting: true` in `metadata.json`, the platform silently skips the pre-request script and every request goes out unauthenticated. +- **`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