fix: the 2026-10-17 review's medium findings (reinstall approval, setup tests, pagination, AppService scope)

.agents/skills/deploy-and-verify/SKILL.md

@@ -13,8 +13,11 @@ log or store credentials.
    carries the target instance's vendor prefix (instances refuse other vendors' prefixes; `npm run setup` renames it).
 2. **Find the alias** — `npx now-sdk auth --list`; ask the user which alias to use if more than one fits.
 3. **Install** — `npm run deploy -- --auth <alias>`. Install is asynchronous; the CLI prints a rollback link on success.
-   On "Could not determine app installation status", retry once with `--reinstall` after telling the user it removes
-   instance-only changes to the app. On "Unable to install third party application", see step 1.
+   On "Could not determine app installation status", check the execution tracker link first; do **not** retry with
+   `--reinstall` on your own. `--reinstall` uninstalls the app first, which deletes everything on the instance that is
+   not in source (records in the app's tables, instance-only changes). Explain that to the user, name the instance,
+   and wait for their explicit approval before you retry once with `--reinstall`. On "Unable to install third party
+   application", see step 1.
 4. **ATF** — `npm run test:instance -- --auth <alias> -o json`. If it answers "Scheduled test/suite execution is disabled",
    the instance needs `sn_atf.runner.enabled` and then `sn_atf.schedule.enabled` set to `true` — ask the user before
    changing instance properties. Report the pass/fail counts and the result URL.

.claude/skills/deploy-and-verify/SKILL.md

@@ -13,8 +13,11 @@ log or store credentials.
    carries the target instance's vendor prefix (instances refuse other vendors' prefixes; `npm run setup` renames it).
 2. **Find the alias** — `npx now-sdk auth --list`; ask the user which alias to use if more than one fits.
 3. **Install** — `npm run deploy -- --auth <alias>`. Install is asynchronous; the CLI prints a rollback link on success.
-   On "Could not determine app installation status", retry once with `--reinstall` after telling the user it removes
-   instance-only changes to the app. On "Unable to install third party application", see step 1.
+   On "Could not determine app installation status", check the execution tracker link first; do **not** retry with
+   `--reinstall` on your own. `--reinstall` uninstalls the app first, which deletes everything on the instance that is
+   not in source (records in the app's tables, instance-only changes). Explain that to the user, name the instance,
+   and wait for their explicit approval before you retry once with `--reinstall`. On "Unable to install third party
+   application", see step 1.
 4. **ATF** — `npm run test:instance -- --auth <alias> -o json`. If it answers "Scheduled test/suite execution is disabled",
    the instance needs `sn_atf.runner.enabled` and then `sn_atf.schedule.enabled` set to `true` — ask the user before
    changing instance properties. Report the pass/fail counts and the result URL.

.github/PULL_REQUEST_TEMPLATE.md

@@ -5,7 +5,7 @@
 <!-- Build, lint, and — for behaviour changes — the instance release/patch and ATF result. -->
 
 ## Checklist
-- [ ] `npm run lint` and `npm run build` pass
+- [ ] `npm run lint`, `npm test` and `npm run build` pass
 - [ ] `src/fluent/generated/keys.ts` changes are included (no hand edits)
 - [ ] No `Table()`/`Record()` definitions removed without saying so above
 - [ ] Behaviour changes have an ATF test, and were run on a test instance

.github/workflows/ci.yml

@@ -38,6 +38,10 @@ jobs:
       - name: Lint
         run: npm run lint
 
+      # Runs scripts/setup.mjs on temporary copies of the checkout (tests/setup.test.mjs).
+      - name: Test
+        run: npm test
+
       - name: Agent skills are in sync (.agents/skills -> .claude/skills)
         run: npm run sync:skills -- --check
 

AGENTS.md

@@ -22,6 +22,7 @@ Request flow: React page → `fetch()` with `X-UserToken: window.g_ck` → Scrip
 ```bash
 npm ci                                   # install exact dependency versions
 npm run lint                             # ESLint (client, server, scripts)
+npm test                                 # node:test suite in tests/ (setup.mjs on a temporary copy)
 npm run build                            # compile Fluent + bundle the UI (writes src/fluent/generated/keys.ts)
 npm run build:ci                         # build --frozenKeys: fails if keys.ts is stale (CI uses this)
 npm run deploy -- --auth <alias>         # now-sdk install to a dev/test instance
@@ -54,14 +55,20 @@ for the SDK (`now-sdk`, from github.com/ServiceNow/sdk) teaches the same workflo
    `npm run setup -- --scope x_<vendor>_<app> --name "<Name>"` once; never hand-edit the scope.
 2. **Never delete a `Table()`, `Record()`, `BusinessRule()` or other record definition without asking the user.**
    Removing it from source makes the next install delete it on the instance (tracked in `keys.ts`). Use `Now.del()` /
-   `npx now-sdk explain now-del-guide` when deletion is intended.
+   `npx now-sdk explain now-del-guide` when deletion is intended. The same applies to `npm run deploy -- --reinstall`:
+   it uninstalls the app first and deletes everything on the instance that is not in source, including the records in
+   the app's tables. Never run it without the user's explicit approval for that instance; telling them is not enough.
 3. **Commit `src/fluent/generated/keys.ts`.** It maps every `Now.ID[...]` key to a stable sys_id. Never edit it by hand,
    never rename an existing `Now.ID` key (that creates a new record and orphans the old one). CI builds with `--frozenKeys`.
 4. **Fluent files are declarative.** Every top-level statement must be an SDK call or an `export const x = SdkCall(...)`;
    plain `const` declarations, string concatenation in properties and other statements fail the build. Use one string
    literal per property.
-5. **Script Includes load independently.** Each `.server.js` file defines its own constants (`var FP = 'x_snc_example_';`);
-   globals from one Script Include are not visible in another. Call them by API name: `new x_snc_example.AppService()`.
+5. **Script Includes keep their constants private.** Wrap each `.server.js` file in a function that returns the class,
+   so the class is the only name the file declares (see `AppService.server.js`):
+   `var AppService = (function() { var FP = 'x_snc_example_'; … return AppService; })();`.
+   Top-level `var` constants may be shared with other Script Includes that run in the same transaction, so a second
+   file declaring `FP` or `MAX_NAME` could change this one's values. Each file defines the constants it needs; call
+   other Script Includes by API name: `new x_snc_example.AppService()`.
 6. **Data access goes through `GlideRecordSecure`** in anything reachable from REST or the UI, so ACLs apply to the caller.
    Plain `GlideRecord` bypasses ACLs; use it only where elevated access is intended and say why in a comment.
 7. **REST routes stay thin** and delegate to `AppService`, which validates input and returns `{ status, body }`.
@@ -83,7 +90,7 @@ for the SDK (`now-sdk`, from github.com/ServiceNow/sdk) teaches the same workflo
   four record ACLs, `AppService` methods, REST routes, an ATF test, then register the file in `src/fluent/index.now.ts`.
   Use the object form `actions: { read: true }` — it controls *cross-scope* access, so grant writes only when another scope
   needs them. Keep `allowWebServiceAccess: true` if the REST Table API must reach the table.
-- **Add a Script Include:** code in `src/server/<Name>.server.js` (`Class.create()` pattern), declaration in
+- **Add a Script Include:** code in `src/server/<Name>.server.js` (`Class.create()` inside a function wrapper, rule 5), declaration in
   `src/fluent/script-includes/` with `script: Now.include('../../server/<Name>.server.js')`, `apiName: 'x_snc_example.<Name>'`,
   `accessibleFrom: 'package_private'` unless other scopes call it.
 - **Add a REST route:** add to the `routes` of `src/fluent/rest-api/app-api.now.ts` with its own `$id`, `method`, `path`
@@ -104,7 +111,7 @@ for the SDK (`now-sdk`, from github.com/ServiceNow/sdk) teaches the same workflo
 
 ## Done means
 
-- `npm run lint` and `npm run build` pass, and `keys.ts` changes are committed with the code that caused them.
+- `npm run lint`, `npm test` and `npm run build` pass, and `keys.ts` changes are committed with the code that caused them.
 - For behaviour changes: deployed to a dev instance and `npm run test:instance` passes; new behaviour has an ATF test.
 - Docs that describe the change (`README.md`, this file, `TROUBLESHOOTING.md`, `CHANGELOG.md` under `[Unreleased]`)
   are updated in the same change.

CHANGELOG.md

@@ -7,6 +7,11 @@ version needs manual work to adopt it.
 ## [Unreleased]
 
 ### Added
+- `npm test` (`node:test`, run in CI): `tests/setup.test.mjs` runs `scripts/setup.mjs` on temporary copies of the
+  repository, with and without git, and checks the scope and name rewrite, the new `scopeId`, the lockfile,
+  CHANGELOG and generated-keys reset, and the refusals (unsafe scopes and names, a dirty tree, a second run without
+  `--force`, which must write nothing). The setup tests skip themselves once an app is set up, and `setup.mjs` leaves
+  `tests/setup.test.mjs` as written.
 - Supply-chain baseline for CI and releases: `npm ci --ignore-scripts`, no npm cache in the release and
   instance-deploy jobs, Node 24.21.0 by exact version for releases, `.npmrc` `min-release-age=7`, Dependabot
   cooldowns (7 days, 14 for npm majors), job-level permissions and a zizmor job.
@@ -18,6 +23,21 @@ version needs manual work to adopt it.
 - `SECURITY.md` explains the advisories reported in the SDK's development dependencies: they do not reach the
   installed app, are fixed by SDK updates, and are not overridden by default.
 
+### Fixed
+- The `deploy-and-verify` skill no longer lets an agent retry with `--reinstall` after only telling the user: it must
+  wait for explicit approval, because `--reinstall` uninstalls the app and deletes its table records. `AGENTS.md`
+  rule 2 and `TROUBLESHOOTING.md` say the same.
+- The item list no longer shows a row twice (duplicate React keys) when rows are created between two "Load more"
+  requests, and a create, update or delete no longer drops the pages loaded with "Load more": pages are merged by
+  `sys_id` and every loaded page is reloaded after a change (`src/client/services/pagination.js`, unit-tested).
+  `AppService.listItems` breaks `sys_created_on` ties by `sys_id`, so rows created in the same second keep one order
+  across pages. Built and unit-tested; not yet exercised on an instance.
+- `AppService.server.js` no longer declares its constants (`FP`, `CATEGORIES`, `MAX_NAME`, …) at the top level of the
+  scope, where another Script Include declaring the same names could change AppService's validation. The file is now
+  wrapped in a function that returns the class; `AGENTS.md` rule 5 and `TROUBLESHOOTING.md` teach the same pattern.
+  Behaviour is unchanged (a vm comparison of the old and new file gave identical results); `tests/app-service.test.mjs`
+  checks that only `AppService` is declared. Not yet run on an instance.
+
 ## [1.0.0] - 2026-10-10
 
 > Brazil-ready, agent-neutral, verified on an instance.

CONTRIBUTING.md

@@ -21,7 +21,7 @@ possible variation.
 
 1. Fork, then branch from `main`.
 2. `npm ci`, make the change, keep the `x_snc_example` scope prefix.
-3. `npm run lint && npm run build`; commit any `src/fluent/generated/keys.ts` changes with the code.
+3. `npm run lint && npm test && npm run build`; commit any `src/fluent/generated/keys.ts` changes with the code.
 4. If you changed `.agents/skills/`, run `npm run sync:skills`.
 5. For behaviour changes, deploy to a test instance (under a scope with your vendor prefix — `npm run setup` on a
    scratch copy) and run the ATF suite. Say which release and patch you tested in the PR.

README.md

@@ -148,6 +148,7 @@ env_vars = ["SERVICENOW_INSTANCE_URL", "SERVICENOW_USERNAME", "SERVICENOW_PASSWO
 |---|---|
 | `npm run setup -- --scope … --name …` | Turn the template into your app (once) |
 | `npm run lint` | ESLint for the UI, the Script Include and the scripts |
+| `npm test` | Unit tests (`node:test`) in `tests/`, including `npm run setup` on a temporary copy |
 | `npm run build` | Compile Fluent and bundle the UI into `dist/` |
 | `npm run build:ci` | Build with `--frozenKeys` (fails if `keys.ts` is stale) |
 | `npm run deploy -- --auth <alias>` | Install on an instance |
@@ -156,7 +157,7 @@ env_vars = ["SERVICENOW_INSTANCE_URL", "SERVICENOW_USERNAME", "SERVICENOW_PASSWO
 
 ## CI deploy (optional)
 
-`.github/workflows/ci.yml` lints, builds with frozen keys and packages on every pull request and every push to
+`.github/workflows/ci.yml` lints, runs `npm test`, builds with frozen keys and packages on every pull request and every push to
 `main`. Its `instance-test` job also deploys to a **test** instance on pushes to `main` and runs the ATF suite once
 you add the repository variable
 `SN_SDK_INSTANCE_URL` and the secrets `SN_SDK_OAUTH_CLIENT_ID` / `SN_SDK_OAUTH_CLIENT_SECRET` (OAuth client

RELEASING.md

@@ -15,7 +15,7 @@ instance first.
 
 On the release branch, with a clean tree:
 
-1. `npm ci && npm run lint && npm run build:ci && npm run sync:skills -- --check`
+1. `npm ci && npm run lint && npm test && npm run build:ci && npm run sync:skills -- --check`
 2. Verify on an instance (a PDI is fine), in a scratch copy so the template scope stays `x_snc_example`:
    ```bash
    rsync -a --exclude node_modules --exclude .git ./ /tmp/sn-smoke/ && cd /tmp/sn-smoke && npm ci

TROUBLESHOOTING.md

@@ -15,7 +15,8 @@ their own company code (system property `glide.appcreator.company.code`), so the
 Install runs asynchronously and polls the instance. Check the execution tracker link the CLI prints (or
 `sys_execution_tracker` on the instance). If the upload succeeded but polling failed, retry once with
 `npm run deploy -- --auth <alias> --reinstall`. `--reinstall` uninstalls first, which deletes anything changed on the
-instance that is not in source.
+instance that is not in source, including the records in the app's tables. Coding agents must get your explicit
+approval before they run it (AGENTS.md rule 2).
 
 ## `now-sdk auth` or install fails with 401
 
@@ -58,7 +59,8 @@ so every platform script API it calls must be declared. Add the API named in the
 **If the call is still refused after deploying:** the denied call made the platform create its own privilege record
 with status *requested*, and installing the app keeps that record instead of your *allowed* one. Approve it (System
 Applications → Application Cross-Scope Access, set Status to Allowed) or redeploy with `--reinstall` on a development
-instance.
+instance. `--reinstall` deletes the records in the app's tables; coding agents must get your explicit approval before
+they run it (AGENTS.md rule 2).
 
 Prefer to find these before deploying to a strict instance: run the app's flows and the ATF suite with
 `runtimeAccessTracking: "permissive"` and check the log for `Security restricted: Execute operation on API '…' … was
@@ -82,5 +84,6 @@ granted` — but note that permissive recording on Brazil Patch 0 missed some AP
 
 ## "AppService is not defined" at runtime
 
-Script Includes are called by API name: `new x_snc_example.AppService()`. Each Script Include file must define its own
-constants (`var FP = 'x_snc_example_';`); globals from one are not visible in another.
+Script Includes are called by API name: `new x_snc_example.AppService()`. The file must still declare the class under
+the Script Include's name: `var AppService = (function() { … return AppService; })();`. Each Script Include file
+defines its own constants inside that function (`var FP = 'x_snc_example_';`); never rely on another file's constants.

eslint.config.js

@@ -40,7 +40,7 @@ export default [
 
     // Repository tooling (Node).
     {
-        files: ['scripts/**/*.mjs', 'eslint.config.js'],
+        files: ['scripts/**/*.mjs', 'tests/**/*.mjs', 'eslint.config.js'],
         ...js.configs.recommended,
         languageOptions: { ecmaVersion: 'latest', sourceType: 'module', globals: { ...globals.node } },
     },

package.json

@@ -15,6 +15,7 @@
         "build": "now-sdk build",
         "build:ci": "now-sdk build --frozenKeys",
         "lint": "eslint .",
+        "test": "node --test tests/*.test.mjs",
         "deploy": "now-sdk install",
         "test:instance": "now-sdk cicd testsuite run --test-suite-name \"Example App\"",
         "pack": "now-sdk pack",

scripts/setup.mjs

@@ -25,8 +25,10 @@ const SKIP_FILES = new Set([
     'RELEASING.md',
     'docs/compatibility.md',
     'scripts/setup.mjs',
+    'tests/setup.test.mjs',
     join('docs', 'compatibility.md'),
     join('scripts', 'setup.mjs'),
+    join('tests', 'setup.test.mjs'),
 ]);
 const TEMPLATE_SCOPE = 'x_snc_example';
 const TEMPLATE_REPO = 'jschuller/sn-app-template';

src/client/components/App.jsx

@@ -1,5 +1,6 @@
 import React from 'react';
 import { ApiService, PAGE_SIZE } from '../services/ApiService.js';
+import { loadPages, mergeById } from '../services/pagination.js';
 import { ItemForm } from './ItemForm.jsx';
 import { ItemTable } from './ItemTable.jsx';
 import './App.css';
@@ -10,35 +11,55 @@ export function App() {
     const [loading, setLoading] = React.useState(true);
     const [error, setError] = React.useState('');
     const [version, setVersion] = React.useState(0);
+    // Pages the user has loaded with "Load more"; a reload after a change keeps all of them.
+    const pageCount = React.useRef(1);
+    // Bumped by every reload: a "Load more" answer that a reload overtook is dropped.
+    const generation = React.useRef(0);
+    const reloading = React.useRef(false);
+    const loadingMore = React.useRef(false);
 
-    // (Re)load the first page whenever `version` changes.
+    // (Re)load every loaded page whenever `version` changes (first render, and after each create, update or delete).
     React.useEffect(() => {
         let cancelled = false;
-        ApiService.listItems(0)
-            .then((page) => {
+        const run = (generation.current += 1);
+        reloading.current = true;
+        loadPages(ApiService.listItems, pageCount.current, PAGE_SIZE)
+            .then(({ rows, hasMore: more }) => {
                 if (cancelled) return;
-                setItems(page);
-                setHasMore(page.length === PAGE_SIZE);
+                setItems(rows);
+                setHasMore(more);
                 setError('');
             })
             .catch((err) => !cancelled && setError(err.message))
-            .finally(() => !cancelled && setLoading(false));
+            .finally(() => {
+                if (generation.current === run) reloading.current = false;
+                if (!cancelled) setLoading(false);
+            });
         return () => {
             cancelled = true;
         };
     }, [version]);
 
     async function loadMore() {
+        // One request at a time, and none while a reload is replacing the list.
+        if (reloading.current || loadingMore.current) return;
+        loadingMore.current = true;
+        const startedIn = generation.current;
         try {
             const page = await ApiService.listItems(items.length);
-            setItems((current) => [...current, ...page]);
+            if (generation.current !== startedIn) return;
+            pageCount.current += 1;
+            // Rows created since the last request shift the offsets: merge by sys_id so none is listed twice.
+            setItems((current) => mergeById(current, page));
             setHasMore(page.length === PAGE_SIZE);
         } catch (err) {
             setError(err.message);
+        } finally {
+            loadingMore.current = false;
         }
     }
 
-    // Runs an API call, shows its error, and reloads the list. Returns true on success.
+    // Runs an API call, shows its error, and reloads the loaded pages. Returns true on success.
     async function run(action) {
         try {
             await action();