diff --git a/.agents/skills/labkit-app-builder/SKILL.md b/.agents/skills/labkit-app-builder/SKILL.md index b4b8490..c01f457 100644 --- a/.agents/skills/labkit-app-builder/SKILL.md +++ b/.agents/skills/labkit-app-builder/SKILL.md @@ -113,12 +113,16 @@ Build the app in this order: 5. Move GUI-free calculations below the app `end` as app-local functions. 6. Extract production helpers into an app-owned package when the app is too large for a readable single entry point. -7. Do not add new `private/` runners, `*Workflow.m` string-dispatch adapters, +7. Do not treat migration as complete by moving a large `private/run*App.m` + body wholesale into `+ui/runApp.m`. Extract GUI-free `+ops`, `+view`, + `+export`, `+io`, or `+state` behavior first and add tests that call those + functions directly. +8. Do not add new `private/` runners, `*Workflow.m` string-dispatch adapters, fixed `+app` package names, or app-local public helper packages. -8. Render prepared data through `labkit.ui` helpers; keep analysis out of UI helpers. -9. Add export builders before CSV/PNG writing so output contracts can be tested. -10. Add focused tests with synthetic fixtures or minimal generated data. -11. Update human docs for user-facing behavior and scoped `AGENTS.md` only when rules change. +9. Render prepared data through `labkit.ui` helpers; keep analysis out of UI helpers. +10. Add export builders before CSV/PNG writing so output contracts can be tested. +11. Add focused tests with synthetic fixtures or minimal generated data. +12. Update human docs for user-facing behavior and scoped `AGENTS.md` only when rules change. ## Validation diff --git a/apps/AGENTS.md b/apps/AGENTS.md index cd35fac..2437d5d 100644 --- a/apps/AGENTS.md +++ b/apps/AGENTS.md @@ -29,6 +29,11 @@ Apps are first-class deliverables. Do not treat them as examples for a hidden pl - Callback-heavy migrated apps should move app-owned production code into these package components instead of adding new `private/` runners or string-dispatch workflow adapters. +- Do not satisfy a private-runner migration by moving the whole runner into + `+ui/runApp.m`. Extract GUI-free calculations, display formatting, export + builders, app-local IO normalization, and default state/result structs into + focused app-owned packages first, and add tests that call those functions + directly. - Do not add new `*Workflow.m` files or app-owned `+core/dispatch.m` string routers. - When a public app file grows large, prefer moving GUI-free app-owned calculations, export builders, formatting utilities, deterministic image/signal transforms, and focused control construction into `apps///+/...`. diff --git a/docs/apps.md b/docs/apps.md index eeb5ea1..0b7c9f7 100644 --- a/docs/apps.md +++ b/docs/apps.md @@ -123,6 +123,29 @@ Existing DIC and wearable `private/` runners are migration debt, not the preferred app structure. Do not add app-owned `+core/dispatch.m` string routers to new app work. +## Migration Quality Gate + +A runner migration is not complete when it only moves a large +`apps//private/run*App.m` body into `+/+ui/runApp.m`. Move +GUI-free responsibilities first, and keep the runner focused on orchestration: +state setup, control construction, callback wiring, user alerts, log wording, +refresh ordering, and app launch/debug behavior. + +Before deleting a private runner debt item, extract directly testable behavior +into app-owned package functions: + +```text ++ops deterministic calculations and image/signal transforms ++view summaries, display rows, plot-data preparation, and table formatting ++export CSV/image export builders and output contracts ++io dialog-result normalization, app-local file readers, and path defaults ++state app-local default result/state structs +``` + +Focused unit tests should call those package functions directly. GUI structural +tests are still required for layout and callback wiring, but they are not enough +to prove that a migration reduced complexity. + ## New App Checklist Define these before adding controls or helpers: diff --git a/docs/architecture.md b/docs/architecture.md index 888d415..3058742 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -109,6 +109,11 @@ apps/wearable/private/ Exit condition: migrate the remaining DIC and wearable app bodies/helpers into app-owned packages under their owning app folders, with public entry points owning GUI state, callbacks, debug launch routing, and user-facing log wording. +Moving a large `private/run*App.m` body wholesale into `+ui/runApp.m` does not +meet this exit condition. Runner migrations should first split deterministic +calculations, display formatting, export builders, app-local IO normalization, +and default state/result structs into focused app-owned package functions with +direct tests. The remaining runner should be orchestration code. Allowed electrochemistry string-dispatch debt: none. The former app-owned `+core/dispatch.m` routers have been replaced by component-local package diff --git a/tests/AGENTS.md b/tests/AGENTS.md index a6350f9..6aafd36 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -20,6 +20,10 @@ Tests mirror source ownership. Do not create a parallel runner framework unless trace capture, GUI fixture setup, and component snapshots. - Do not move app-specific formulas, expected scientific values, result schemas, or export columns into shared test helpers. - Boundary tests may require app-owned logic to stay under the owning app tree, but should not require GUI-free helpers to remain inside the public app entry-point file or assert exact app-private helper file lists. +- Runner-migration tests should not rely only on GUI structural launches. When + migration creates an app-owned package for DIC or wearable apps, add unit + tests that call non-UI package functions such as `+ops`, `+view`, `+export`, + `+io`, or `+state` directly. - UI public-surface tests should assert the layered `labkit.ui.app/view/tool/diag` facade and keep low-level controls, row resize, panel internals, and popout implementation private. - GUI smoke/debug tests may assert that every app supports debug launch and visible startup trace, but should not claim full interactive workflow validation. - When one test file grows too broad, add new focused `test_*.m` files instead of appending unrelated coverage. diff --git a/tests/integration/project/ProjectDebtGuardrailTest.m b/tests/integration/project/ProjectDebtGuardrailTest.m index c0215c5..7deebe1 100644 --- a/tests/integration/project/ProjectDebtGuardrailTest.m +++ b/tests/integration/project/ProjectDebtGuardrailTest.m @@ -36,6 +36,21 @@ function oversizedAppEntrypointDebtIsRemoved(testCase) fprintf('Entrypoint size debt inventory: %d files over 500 lines.\n', numel(actual)); end + function oversizedRunnerDebtDoesNotGrow(testCase) + root = setupLabKitTestPath(); + expectedFiles = expectedOversizedRunnerDebtFiles(); + actualFiles = collectOversizedAppRunners(root, 500); + unexpectedFiles = setdiff(actualFiles, expectedFiles); + testCase.verifyTrue(isempty(unexpectedFiles), ... + ['expected-debt: oversized app runners should not grow. ' ... + 'Split deterministic behavior into app-owned +ops/+view/+export/+io/+state ' ... + 'before moving runner bodies. Files: ' ... + strjoin(cellstr(unexpectedFiles), ', ')]); + + fprintf('Oversized runner debt inventory: %d files over 500 lines.\n', ... + numel(actualFiles)); + end + function oldRunnerDependenciesAreRemoved(testCase) root = setupLabKitTestPath(); @@ -96,6 +111,38 @@ function appWorkflowDispatchDebtDoesNotGrow(testCase) fprintf('Workflow dispatch debt inventory: %d Workflow files, %d +core dispatch files.\n', ... numel(workflowFiles), numel(dispatchFiles)); end + + function dicWearableMigrationsHaveDirectPackageTests(testCase) + root = setupLabKitTestPath(); + packageRoots = collectDicWearableAppPackageRoots(root); + missing = strings(1, 0); + + for k = 1:numel(packageRoots) + packageRoot = packageRoots(k); + nonUiComponents = collectNonUiPackageComponents(packageRoot); + [family, namespace] = appPackageFamilyAndNamespace(root, packageRoot); + if isempty(nonUiComponents) + missing(end+1) = string(relativePath(root, packageRoot)) + ... + " -> missing non-UI package component"; %#ok + continue; + end + + if ~packageNamespaceHasDirectUnitTest(root, family, namespace) + missing(end+1) = string(relativePath(root, packageRoot)) + ... + " -> missing direct unit test for " + namespace + ... + ".(ops|view|export|io|state)"; %#ok + end + end + + testCase.verifyTrue(isempty(missing), ... + ['DIC and wearable app package migrations need directly tested ' ... + 'non-UI app-owned functions; GUI structural tests alone do not prove ' ... + 'runner complexity was reduced. Findings: ' ... + strjoin(cellstr(missing), ', ')]); + + fprintf('DIC/wearable migrated app package inventory: %d package roots.\n', ... + numel(packageRoots)); + end end end @@ -170,6 +217,23 @@ function appWorkflowDispatchDebtDoesNotGrow(testCase) files = collectRelativeFiles(root, fullfile(root, 'apps', '**', 'private', '*.m')); end +function files = collectOversizedAppRunners(root, maxLines) + files = strings(1, 0); + entries = [ ... + dir(fullfile(root, 'apps', '**', 'private', 'run*App.m')); ... + dir(fullfile(root, 'apps', '**', '+ui', 'runApp.m'))]; + for k = 1:numel(entries) + if entries(k).isdir + continue; + end + filepath = fullfile(entries(k).folder, entries(k).name); + if countFileLines(filepath) > maxLines + files(end+1) = string(relativePath(root, filepath)); %#ok + end + end + files = unique(files); +end + function files = collectRelativeFiles(root, pattern) entries = dir(pattern); files = strings(1, 0); @@ -182,6 +246,73 @@ function appWorkflowDispatchDebtDoesNotGrow(testCase) files = unique(files); end +function packageRoots = collectDicWearableAppPackageRoots(root) + packageRoots = strings(1, 0); + families = ["dic", "wearable"]; + for family = families + familyRoot = fullfile(root, 'apps', char(family)); + if ~isfolder(familyRoot) + continue; + end + + apps = dir(familyRoot); + for iApp = 1:numel(apps) + appDir = apps(iApp); + if ~appDir.isdir || any(strcmp(appDir.name, {'.', '..', 'private'})) + continue; + end + + packageDirs = dir(fullfile(appDir.folder, appDir.name, '+*')); + for iPackage = 1:numel(packageDirs) + packageDir = packageDirs(iPackage); + if packageDir.isdir && startsWith(packageDir.name, '+') + packageRoots(end+1) = string(fullfile( ... + packageDir.folder, packageDir.name)); %#ok + end + end + end + end + packageRoots = unique(packageRoots); +end + +function components = collectNonUiPackageComponents(packageRoot) + components = strings(1, 0); + componentNames = ["+ops", "+view", "+export", "+io", "+state"]; + for k = 1:numel(componentNames) + componentRoot = fullfile(packageRoot, char(componentNames(k))); + files = dir(fullfile(componentRoot, '*.m')); + if isfolder(componentRoot) && any(~[files.isdir]) + components(end+1) = componentNames(k); %#ok + end + end +end + +function [family, namespace] = appPackageFamilyAndNamespace(root, packageRoot) + rel = string(relativePath(root, packageRoot)); + parts = split(rel, '/'); + family = parts(2); + packageName = parts(end); + namespace = extractAfter(packageName, 1); +end + +function tf = packageNamespaceHasDirectUnitTest(root, family, namespace) + testRoot = fullfile(root, 'tests', 'unit', 'apps', char(family)); + if ~isfolder(testRoot) + tf = false; + return; + end + + pattern = [char(namespace) '\.(ops|view|export|io|state)\.']; + testFiles = collectTextFiles(testRoot); + tf = false; + for k = 1:numel(testFiles) + if ~isempty(regexp(fileread(testFiles{k}), pattern, 'once')) + tf = true; + return; + end + end +end + function files = expectedAppPrivateDebtFiles() files = [ ... "apps/dic/private/alignMovingToReference.m", ... @@ -231,6 +362,15 @@ function appWorkflowDispatchDebtDoesNotGrow(testCase) "apps/wearable/private/runECGPrintApp.m"]; end +function files = expectedOversizedRunnerDebtFiles() + files = [ ... + "apps/dic/private/runDICPreprocessApp.m", ... + "apps/electrochem/cic/+cic/+ui/runApp.m", ... + "apps/electrochem/csc/+csc/+ui/runApp.m", ... + "apps/electrochem/eis/+eis/+ui/runApp.m", ... + "apps/wearable/private/runECGPrintApp.m"]; +end + function assertExpectedDebt(testCase, actualFiles, expectedMax, label) testCase.verifyTrue(numel(actualFiles) <= expectedMax, ... sprintf('%s. Current count %d exceeds expected debt %d. Files: %s', ...