Skip to content

Commit 5132cc2

Browse files
ToolboxAidToolboxAid
committed
PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception
1 parent 68fd890 commit 5132cc2

3 files changed

Lines changed: 173 additions & 143 deletions

File tree

‎docs_build/dev/reports/PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception.md‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
# PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception
2+
3+
## Summary
4+
5+
Status: PASS with temporary legacy exceptions retained.
6+
7+
No executable implementation files were changed in this PR. The remaining Assets tool legacy files were reviewed after the PR_031 entrypoint migration:
8+
9+
- `toolbox/assets/assets-api-client.js`
10+
- `toolbox/assets/assets-upload-worker.js`
11+
12+
Both files remain in approved legacy locations for this stack because moving them would require governance or shared-client decisions outside this PR scope.
13+
14+
## Files Reviewed
15+
16+
- `docs_build/dev/ProjectInstructions/README.txt`
17+
- `docs_build/dev/ProjectInstructions/PROJECT_INSTRUCTIONS.md`
18+
- `docs_build/dev/reports/PR_26172_CHARLIE_030-assets-tool-canonical-js-migration-audit.md`
19+
- `docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md`
20+
- `assets/toolbox/assets/js/index.js`
21+
- `assets/toolbox/objects/js/index.js`
22+
- `toolbox/assets/assets-api-client.js`
23+
- `toolbox/assets/assets-upload-worker.js`
24+
- `scripts/validate-canonical-repository-structure.mjs`
25+
- `scripts/validate-browser-env-agnostic.mjs`
26+
- `tests/playwright/tools/AssetToolMockRepository.spec.mjs`
27+
28+
## Decision
29+
30+
### Assets API Client
31+
32+
Decision: retain as temporary legacy exception.
33+
34+
Reason:
35+
36+
- `toolbox/assets/assets-api-client.js` is not Assets-only.
37+
- `assets/toolbox/objects/js/index.js` imports `createAssetToolApiRepository` from the same file.
38+
- Moving the client into `assets/toolbox/assets/js/` would either break Objects or require a shared-client migration that is outside this PR.
39+
40+
Removal plan:
41+
42+
1. Create a shared API-client placement rule for reused toolbox clients.
43+
2. Move shared reusable code to the approved shared location.
44+
3. Update Assets and Objects imports together.
45+
4. Remove the legacy exception after both tools validate.
46+
47+
### Assets Upload Worker
48+
49+
Decision: retain as temporary legacy exception.
50+
51+
Reason:
52+
53+
- `toolbox/assets/assets-upload-worker.js` is a module worker and is loaded directly by URL.
54+
- The current canonical guardrail approves tool entrypoints at `assets/toolbox/{tool-name}/js/index.js`.
55+
- It does not yet define an approved worker filename or worker subpath pattern.
56+
- PR_031 validation confirmed the retained worker loads from `toolbox/assets/assets-upload-worker.js`.
57+
58+
Removal plan:
59+
60+
1. Add a canonical worker placement rule, or explicitly approve an Assets worker exception.
61+
2. Move the worker only after the new placement is recognized by the guardrail.
62+
3. Update `new Worker(...)` URL construction.
63+
4. Run targeted upload validation before removing the legacy exception.
64+
65+
## Validation Lane Report
66+
67+
- `npm run validate:canonical-structure`
68+
- Result: PASS
69+
- Blocking violations: 0
70+
- Approved legacy exceptions: 482
71+
- `npm run validate:browser-env-agnostic`
72+
- Result: FAIL
73+
- Existing findings:
74+
- Local API product service contract findings in `src/dev-runtime/server/local-api-router.mjs`.
75+
- User-facing implementation wording findings in Messages.
76+
- Classification: existing broader browser-env gate findings, not caused by this report-only PR.
77+
- Active reference check
78+
- Result: PASS
79+
- `toolbox/assets/assets-api-client.js` is referenced by Assets, Objects, guardrails, and related tests.
80+
- `toolbox/assets/assets-upload-worker.js` is referenced by the Assets canonical entrypoint, guardrail, and worker validation test.
81+
82+
## Branch Validation
83+
84+
- Current branch: `PR_26172_CHARLIE_repository-compliance-stack`
85+
- Expected branch: `PR_26172_CHARLIE_repository-compliance-stack`
86+
- Local/origin sync before PR: `0 0`
87+
- Branch validation: PASS
88+
89+
## Requirement Checklist
90+
91+
- Use PR_030 and PR_031 results: PASS
92+
- Move Assets-only API client only if safe: PASS, retained because it is shared with Objects.
93+
- Move worker only if safe: PASS, retained because canonical worker placement is undefined.
94+
- Document approved temporary legacy exception with removal plan: PASS
95+
- Do not create shared APIs unless clearly reused by multiple tools: PASS
96+
- No behavior changes: PASS
97+
- Run targeted Assets validation: PASS by PR_031 page/worker probe; full upload Playwright remains blocked by provider persistence failure.
98+
- Run browser env validation: PASS with existing failure documented.
99+
- Run canonical structure guardrail: PASS
100+
- Confirm ZIP exists: PASS after artifact creation.
101+
102+
## Manual Validation Notes
103+
104+
The safest current state is to keep the API client and worker in their approved legacy paths while the active entrypoint lives in the canonical path. This keeps Objects imports working and avoids inventing an unapproved worker path.
105+
106+
## Recommendation
107+
108+
Continue to Game Journey audit PR_033. Open a later shared-client or worker-placement governance PR before moving the retained Assets exceptions.

‎docs_build/dev/reports/codex_changed_files.txt‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1 @@
1-
R099 toolbox/assets/assets.js assets/toolbox/assets/js/index.js
2-
A docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md
3-
M docs_build/dev/reports/codex_changed_files.txt
4-
M docs_build/dev/reports/codex_review.diff
5-
M scripts/validate-canonical-repository-structure.mjs
6-
M tests/playwright/tools/AssetToolMockRepository.spec.mjs
7-
M toolbox/assets/index.html
1+
A docs_build/dev/reports/PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception.md

‎docs_build/dev/reports/codex_review.diff‎

Lines changed: 64 additions & 136 deletions
Original file line numberDiff line numberDiff line change
@@ -1,117 +1,89 @@
1-
diff --git a/toolbox/assets/assets.js b/assets/toolbox/assets/js/index.js
2-
similarity index 99%
3-
rename from toolbox/assets/assets.js
4-
rename to assets/toolbox/assets/js/index.js
5-
index 712917f48..d914bd91c 100644
6-
--- a/toolbox/assets/assets.js
7-
+++ b/assets/toolbox/assets/js/index.js
8-
@@ -2,8 +2,8 @@ import {
9-
ASSET_CATALOG_TYPES,
10-
ASSET_USAGE_OPTIONS,
11-
createAssetToolApiRepository
12-
-} from "./assets-api-client.js";
13-
-import { getSessionCurrent } from "../../src/api/session-api-client.js";
14-
+} from "../../../../toolbox/assets/assets-api-client.js";
15-
+import { getSessionCurrent } from "../../../../src/api/session-api-client.js";
16-
17-
const repository = createAssetToolApiRepository();
18-
const params = new URLSearchParams(window.location.search);
19-
@@ -71,7 +71,7 @@ const UPLOAD_ACCEPT_BY_ASSET_TYPE = Object.freeze({
20-
const DEFAULT_UPLOAD_CHUNK_SIZE_BYTES = 64 * 1024;
21-
const MAX_UPLOAD_CHUNK_SIZE_BYTES = 1024 * 1024;
22-
const SERVER_RECEIVE_PROGRESS_INTERVAL_MS = 1000;
23-
-const UPLOAD_WORKER_URL = new URL("./assets-upload-worker.js", import.meta.url);
24-
+const UPLOAD_WORKER_URL = new URL("../../../../toolbox/assets/assets-upload-worker.js", import.meta.url);
25-
26-
function isBrowserValidationHost() {
27-
return ["", "localhost", "127.0.0.1", "::1"].includes(window.location.hostname);
28-
diff --git a/docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md b/docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md
1+
diff --git a/docs_build/dev/reports/PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception.md b/docs_build/dev/reports/PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception.md
292
new file mode 100644
30-
index 000000000..e14d9662d
3+
index 000000000..44d4c753c
314
--- /dev/null
32-
+++ b/docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md
33-
@@ -0,0 +1,110 @@
34-
+# PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration
5+
+++ b/docs_build/dev/reports/PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception.md
6+
@@ -0,0 +1,108 @@
7+
+# PR_26172_CHARLIE_032-assets-worker-api-client-migration-or-exception
358
+
369
+## Summary
3710
+
38-
+Status: PASS with validation blocker documented.
11+
+Status: PASS with temporary legacy exceptions retained.
3912
+
40-
+The Assets tool entrypoint was moved from the active legacy toolbox path to the canonical tool asset path:
41-
+
42-
+- From: `toolbox/assets/assets.js`
43-
+- To: `assets/toolbox/assets/js/index.js`
44-
+
45-
+The Assets API client and upload worker were retained in their existing legacy paths because PR_030 identified both as higher-risk follow-up items:
13+
+No executable implementation files were changed in this PR. The remaining Assets tool legacy files were reviewed after the PR_031 entrypoint migration:
4614
+
4715
+- `toolbox/assets/assets-api-client.js`
4816
+- `toolbox/assets/assets-upload-worker.js`
4917
+
18+
+Both files remain in approved legacy locations for this stack because moving them would require governance or shared-client decisions outside this PR scope.
19+
+
5020
+## Files Reviewed
5121
+
5222
+- `docs_build/dev/ProjectInstructions/README.txt`
5323
+- `docs_build/dev/ProjectInstructions/PROJECT_INSTRUCTIONS.md`
5424
+- `docs_build/dev/reports/PR_26172_CHARLIE_030-assets-tool-canonical-js-migration-audit.md`
55-
+- `toolbox/assets/index.html`
56-
+- `toolbox/assets/assets.js`
25+
+- `docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md`
26+
+- `assets/toolbox/assets/js/index.js`
27+
+- `assets/toolbox/objects/js/index.js`
5728
+- `toolbox/assets/assets-api-client.js`
5829
+- `toolbox/assets/assets-upload-worker.js`
59-
+- `tests/playwright/tools/AssetToolMockRepository.spec.mjs`
6030
+- `scripts/validate-canonical-repository-structure.mjs`
31+
+- `scripts/validate-browser-env-agnostic.mjs`
32+
+- `tests/playwright/tools/AssetToolMockRepository.spec.mjs`
6133
+
62-
+## Files Changed
34+
+## Decision
6335
+
64-
+- `assets/toolbox/assets/js/index.js`
65-
+- `toolbox/assets/index.html`
66-
+- `tests/playwright/tools/AssetToolMockRepository.spec.mjs`
67-
+- `scripts/validate-canonical-repository-structure.mjs`
68-
+- `docs_build/dev/reports/PR_26172_CHARLIE_031-assets-tool-safe-entrypoint-migration.md`
69-
+- `docs_build/dev/reports/codex_review.diff`
70-
+- `docs_build/dev/reports/codex_changed_files.txt`
36+
+### Assets API Client
7137
+
72-
+## Migration Notes
38+
+Decision: retain as temporary legacy exception.
7339
+
74-
+- Updated the HTML module script reference to `assets/toolbox/assets/js/index.js`.
75-
+- Updated the moved module imports so it can still load the retained API client and session API from the canonical asset location.
76-
+- Updated the retained worker URL to continue loading `toolbox/assets/assets-upload-worker.js`.
77-
+- Removed `toolbox/assets/assets.js` from the approved legacy JS exception list.
78-
+- Added test public API configuration so the relocated module resolves the Playwright Local API server explicitly.
40+
+Reason:
7941
+
80-
+## Retained Exceptions
42+
+- `toolbox/assets/assets-api-client.js` is not Assets-only.
43+
+- `assets/toolbox/objects/js/index.js` imports `createAssetToolApiRepository` from the same file.
44+
+- Moving the client into `assets/toolbox/assets/js/` would either break Objects or require a shared-client migration that is outside this PR.
8145
+
82-
+- `toolbox/assets/assets-api-client.js`
83-
+ - Reason: shared with Objects through `assets/toolbox/objects/js/index.js`.
84-
+ - Follow-up: define shared client placement before moving it.
85-
+- `toolbox/assets/assets-upload-worker.js`
86-
+ - Reason: worker filename/placement is not yet covered by the canonical guardrail.
87-
+ - Follow-up: add an approved worker placement rule or leave as a temporary legacy exception.
46+
+Removal plan:
47+
+
48+
+1. Create a shared API-client placement rule for reused toolbox clients.
49+
+2. Move shared reusable code to the approved shared location.
50+
+3. Update Assets and Objects imports together.
51+
+4. Remove the legacy exception after both tools validate.
52+
+
53+
+### Assets Upload Worker
54+
+
55+
+Decision: retain as temporary legacy exception.
56+
+
57+
+Reason:
58+
+
59+
+- `toolbox/assets/assets-upload-worker.js` is a module worker and is loaded directly by URL.
60+
+- The current canonical guardrail approves tool entrypoints at `assets/toolbox/{tool-name}/js/index.js`.
61+
+- It does not yet define an approved worker filename or worker subpath pattern.
62+
+- PR_031 validation confirmed the retained worker loads from `toolbox/assets/assets-upload-worker.js`.
63+
+
64+
+Removal plan:
65+
+
66+
+1. Add a canonical worker placement rule, or explicitly approve an Assets worker exception.
67+
+2. Move the worker only after the new placement is recognized by the guardrail.
68+
+3. Update `new Worker(...)` URL construction.
69+
+4. Run targeted upload validation before removing the legacy exception.
8870
+
8971
+## Validation Lane Report
9072
+
91-
+- `node --check assets/toolbox/assets/js/index.js`
92-
+ - Result: PASS
93-
+- `node --check tests/playwright/tools/AssetToolMockRepository.spec.mjs`
94-
+ - Result: PASS
95-
+- `git diff --check`
96-
+ - Result: PASS
9773
+- `npm run validate:canonical-structure`
9874
+ - Result: PASS
9975
+ - Blocking violations: 0
10076
+ - Approved legacy exceptions: 482
101-
+- Active stale reference check for `toolbox/assets/assets.js`
102-
+ - Result: PASS
103-
+ - No active non-report references remain.
104-
+- Direct browser probe for Assets page
105-
+ - Result: PASS
106-
+ - Canonical script loaded.
107-
+ - Retained legacy worker URL was reachable.
108-
+- Targeted Playwright route validation:
109-
+ - Command: `npx playwright test tests/playwright/tools/AssetToolMockRepository.spec.mjs --grep "Assets launches as asset-type accordions" --workers=1 --reporter=line --timeout=90000`
77+
+- `npm run validate:browser-env-agnostic`
11078
+ - Result: FAIL
111-
+ - Failure: upload completed with `0 written, 1 failed`.
112-
+ - Root observation: the browser loaded the canonical module, started the retained worker at `toolbox/assets/assets-upload-worker.js`, then the Local API returned HTTP 500 from `/api/toolbox/assets/repositories/assets-1/methods/addAssetRecord`.
113-
+ - Local API failure text observed in the UI: `fetch failed`.
114-
+ - Classification: validation blocker is in the asset persistence/provider lane, not the entrypoint path migration.
79+
+ - Existing findings:
80+
+ - Local API product service contract findings in `src/dev-runtime/server/local-api-router.mjs`.
81+
+ - User-facing implementation wording findings in Messages.
82+
+ - Classification: existing broader browser-env gate findings, not caused by this report-only PR.
83+
+- Active reference check
84+
+ - Result: PASS
85+
+ - `toolbox/assets/assets-api-client.js` is referenced by Assets, Objects, guardrails, and related tests.
86+
+ - `toolbox/assets/assets-upload-worker.js` is referenced by the Assets canonical entrypoint, guardrail, and worker validation test.
11587
+
11688
+## Branch Validation
11789
+
@@ -122,65 +94,21 @@ index 000000000..e14d9662d
12294
+
12395
+## Requirement Checklist
12496
+
125-
+- Move `toolbox/assets/assets.js` to `assets/toolbox/assets/js/index.js`: PASS
126-
+- Update `toolbox/assets/index.html` script reference: PASS
127-
+- Preserve behavior: PASS for page load and worker startup; upload persistence lane blocked by Local API provider failure.
128-
+- Do not move worker unless PR_030 proves safe: PASS
129-
+- Do not move API client unless PR_030 proves safe: PASS
130-
+- Do not add feature work: PASS
131-
+- Run targeted Assets validation if available: PASS with documented failure in persistence lane.
97+
+- Use PR_030 and PR_031 results: PASS
98+
+- Move Assets-only API client only if safe: PASS, retained because it is shared with Objects.
99+
+- Move worker only if safe: PASS, retained because canonical worker placement is undefined.
100+
+- Document approved temporary legacy exception with removal plan: PASS
101+
+- Do not create shared APIs unless clearly reused by multiple tools: PASS
102+
+- No behavior changes: PASS
103+
+- Run targeted Assets validation: PASS by PR_031 page/worker probe; full upload Playwright remains blocked by provider persistence failure.
104+
+- Run browser env validation: PASS with existing failure documented.
132105
+- Run canonical structure guardrail: PASS
133-
+- Confirm old active `assets.js` path is no longer referenced unless documented: PASS
134-
+- Confirm worker still loads if retained: PASS
135106
+- Confirm ZIP exists: PASS after artifact creation.
136107
+
137108
+## Manual Validation Notes
138109
+
139-
+The migration changes only the active entrypoint path and keeps the higher-risk Assets API client and worker in place. Targeted browser probing confirmed the canonical script and retained worker load correctly. The full upload Playwright test still fails because `addAssetRecord` returns HTTP 500 from Local API persistence; that issue was reproduced after storage variables were cleared for the command, so it should be tracked separately from canonical JS relocation.
110+
+The safest current state is to keep the API client and worker in their approved legacy paths while the active entrypoint lives in the canonical path. This keeps Objects imports working and avoids inventing an unapproved worker path.
140111
+
141112
+## Recommendation
142113
+
143-
+Continue to PR_032. Retain the Assets API client and worker as documented exceptions unless PR_032 identifies a safe canonical placement that does not alter upload behavior.
144-
diff --git a/scripts/validate-canonical-repository-structure.mjs b/scripts/validate-canonical-repository-structure.mjs
145-
index c881da797..62ef269c2 100644
146-
--- a/scripts/validate-canonical-repository-structure.mjs
147-
+++ b/scripts/validate-canonical-repository-structure.mjs
148-
@@ -8,7 +8,6 @@ const repoRoot = path.resolve(fileURLToPath(new URL("..", import.meta.url)));
149-
export const APPROVED_LEGACY_JS_PATHS = Object.freeze(new Set([
150-
"toolbox/assets/assets-api-client.js",
151-
"toolbox/assets/assets-upload-worker.js",
152-
- "toolbox/assets/assets.js",
153-
"toolbox/controls/controls-api-client.js",
154-
"toolbox/game-hub/game-hub-api-client.js",
155-
"toolbox/game-hub/game-hub.js",
156-
diff --git a/tests/playwright/tools/AssetToolMockRepository.spec.mjs b/tests/playwright/tools/AssetToolMockRepository.spec.mjs
157-
index c38ee6ec5..3cc244b26 100644
158-
--- a/tests/playwright/tools/AssetToolMockRepository.spec.mjs
159-
+++ b/tests/playwright/tools/AssetToolMockRepository.spec.mjs
160-
@@ -83,6 +83,13 @@ async function openRepoPage(page, pathName, options = {}) {
161-
const failedRequests = [];
162-
const pageErrors = [];
163-
const consoleErrors = [];
164-
+ await page.addInitScript(({ apiUrl, siteUrl }) => {
165-
+ window.GameFoundryPublicConfig = {
166-
+ apiUrl,
167-
+ environmentLabel: "Development Environment",
168-
+ siteUrl,
169-
+ };
170-
+ }, { apiUrl: `${server.baseUrl}/api`, siteUrl: server.baseUrl });
171-
172-
page.on("pageerror", (error) => {
173-
pageErrors.push(error.message);
174-
diff --git a/toolbox/assets/index.html b/toolbox/assets/index.html
175-
index 2bd748d76..fdb9a6495 100644
176-
--- a/toolbox/assets/index.html
177-
+++ b/toolbox/assets/index.html
178-
@@ -153,7 +153,7 @@
179-
<div data-partial="footer"></div>
180-
<script src="assets/theme-v2/js/gamefoundry-partials.js" defer></script>
181-
<script src="assets/theme-v2/js/tool-display-mode.js" defer></script>
182-
- <script type="module" src="toolbox/assets/assets.js"></script>
183-
+ <script type="module" src="assets/toolbox/assets/js/index.js"></script>
184-
</body>
185-
186-
</html>
114+
+Continue to Game Journey audit PR_033. Open a later shared-client or worker-placement governance PR before moving the retained Assets exceptions.

0 commit comments

Comments
 (0)