Skip to content

Commit 2c77b50

Browse files
ToolboxAidToolboxAid
committed
PR_26172_CHARLIE_038-assets-upload-http-500-investigation
1 parent c3a8b9c commit 2c77b50

3 files changed

Lines changed: 232 additions & 50 deletions

File tree

‎docs_build/dev/reports/PR_26172_CHARLIE_038-assets-upload-http-500-investigation.md‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
# PR_26172_CHARLIE_038-assets-upload-http-500-investigation
2+
3+
## Summary
4+
5+
Status: PASS with blocker documented.
6+
7+
The Assets upload HTTP 500 was reproduced and traced to the Local API data-provider persistence boundary. No executable code was changed because the failure is not caused by the Charlie migration and the safe fix requires a separately scoped persistence/API decision.
8+
9+
## Root Cause
10+
11+
The upload worker successfully loads and returns file bytes to the Assets page. The failure occurs later when the browser calls:
12+
13+
`POST /api/toolbox/assets/repositories/assets-1/methods/addAssetRecord`
14+
15+
The Local API route executes `addAssetRecord`, then persists asset runtime tables through:
16+
17+
- `repositoryMethodRequiresPersistence(methodName)`
18+
- `persistAssetProviderState(action)`
19+
- `persistSupabaseAssetSnapshot(action)`
20+
- `upsertSupabaseProductTables(tables, action)`
21+
- `SupabasePostgresProviderAdapter.upsertProductTables(...)`
22+
23+
The configured product data adapter request fails with:
24+
25+
`fetch failed`
26+
27+
The API response includes the expected boundary rule:
28+
29+
`Browser -> Server API -> Data Source`
30+
31+
## Migration Causality
32+
33+
Caused by migration: NO.
34+
35+
Evidence:
36+
37+
- The canonical Assets entrypoint loads successfully.
38+
- The retained upload worker starts successfully from `toolbox/assets/assets-upload-worker.js`.
39+
- The failure happens only after the worker completes and `addAssetRecord` calls the server data persistence layer.
40+
- The failed response is from Local API persistence, not from a missing script, missing worker, import error, or browser module path.
41+
42+
## Files Reviewed
43+
44+
- `assets/toolbox/assets/js/index.js`
45+
- `toolbox/assets/assets-api-client.js`
46+
- `toolbox/assets/assets-upload-worker.js`
47+
- `tests/playwright/tools/AssetToolMockRepository.spec.mjs`
48+
- `tests/helpers/playwrightRepoServer.mjs`
49+
- `src/api/server-api-client.js`
50+
- `src/dev-runtime/server/local-api-router.mjs`
51+
- `src/dev-runtime/persistence/tool-repositories/assets-mock-repository.js`
52+
- `src/dev-runtime/auth/provider-contract-stubs.mjs`
53+
54+
## Reproduction Evidence
55+
56+
Targeted probe result:
57+
58+
```json
59+
{
60+
"log": "Batch upload complete: 0 written, 1 failed, 0 skipped, 0 warnings.",
61+
"batchLog": [
62+
{
63+
"status": "FAIL",
64+
"text": "FAIL: http-500-probe.png - fetch failed"
65+
}
66+
],
67+
"workers": [
68+
"http://127.0.0.1:<port>/toolbox/assets/assets-upload-worker.js"
69+
],
70+
"failedResponses": [
71+
{
72+
"status": 500,
73+
"url": "http://127.0.0.1:<port>/api/toolbox/assets/repositories/assets-1/methods/addAssetRecord",
74+
"body": "{\"error\":\"fetch failed\",\"ok\":false,\"rule\":\"Browser -> Server API -> Data Source\"}"
75+
}
76+
]
77+
}
78+
```
79+
80+
Provider contract route result:
81+
82+
- `/api/providers/contract`: HTTP 200
83+
- Active provider: `supabase-postgres`
84+
- Status: `ready`
85+
- Diagnostic: runtime account and product data connections are fixed to configured server services.
86+
87+
## Fix Decision
88+
89+
Fix applied: NO.
90+
91+
Reason:
92+
93+
- A safe fix would require deciding how Local API Playwright upload validation should persist asset runtime tables when the configured product data adapter is unavailable or unreachable.
94+
- Options include test-scoped Postgres stubbing, a Local API asset persistence adapter, or improved diagnostics. Each option changes persistence/API behavior and is outside this Charlie retained-exception migration scope.
95+
- Silently bypassing persistence for `addAssetRecord` would violate the active server data boundary and could hide real provider failures.
96+
97+
## Validation Lane Report
98+
99+
- HTTP 500 reproduction: PASS
100+
- Worker load confirmation: PASS
101+
- Root-cause trace through Local API persistence: PASS
102+
- Migration causality check: PASS
103+
- Runtime source changes: PASS, none.
104+
- ZIP artifact exists: PASS after artifact creation.
105+
106+
## Branch Validation
107+
108+
- Current branch: `PR_26172_CHARLIE_repository-compliance-stack`
109+
- Expected branch: `PR_26172_CHARLIE_repository-compliance-stack`
110+
- Local/origin sync before PR: `0 0`
111+
- Branch validation: PASS
112+
113+
## Requirement Checklist
114+
115+
- Determine root cause: PASS
116+
- Confirm whether caused by migration or pre-existing persistence/API issue: PASS
117+
- Fix only if clear and safe: PASS, no fix applied because safe resolution is outside scope.
118+
- Otherwise document blocker: PASS
119+
- Do not merge: PASS
120+
- Produce ZIP artifact: PASS after artifact creation.
121+
122+
## Manual Validation Notes
123+
124+
The blocker should be handled by a separate persistence/API validation PR. The retained Assets worker path is not the source of this HTTP 500.
125+
126+
## Recommendation
127+
128+
Continue to PR_039. Move the shared Assets API client if safe, but keep the upload worker as a temporary exception until worker placement governance and upload persistence validation are separately resolved.

‎docs_build/dev/reports/codex_changed_files.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
A docs_build/dev/reports/PR_26172_CHARLIE_037-retained-exceptions-audit.md
1+
A docs_build/dev/reports/PR_26172_CHARLIE_038-assets-upload-http-500-investigation.md

‎docs_build/dev/reports/codex_review.diff‎

Lines changed: 103 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,112 @@
1-
diff --git a/docs_build/dev/reports/PR_26172_CHARLIE_037-retained-exceptions-audit.md b/docs_build/dev/reports/PR_26172_CHARLIE_037-retained-exceptions-audit.md
1+
diff --git a/docs_build/dev/reports/PR_26172_CHARLIE_038-assets-upload-http-500-investigation.md b/docs_build/dev/reports/PR_26172_CHARLIE_038-assets-upload-http-500-investigation.md
22
new file mode 100644
3-
index 000000000..bd470b359
3+
index 000000000..04ff28a1d
44
--- /dev/null
5-
+++ b/docs_build/dev/reports/PR_26172_CHARLIE_037-retained-exceptions-audit.md
6-
@@ -0,0 +1,74 @@
7-
+# PR_26172_CHARLIE_037-retained-exceptions-audit
5+
+++ b/docs_build/dev/reports/PR_26172_CHARLIE_038-assets-upload-http-500-investigation.md
6+
@@ -0,0 +1,128 @@
7+
+# PR_26172_CHARLIE_038-assets-upload-http-500-investigation
88
+
99
+## Summary
1010
+
11-
+Status: PASS.
11+
+Status: PASS with blocker documented.
1212
+
13-
+This audit reviewed the four retained JavaScript exceptions from the target-tool migration stack and classified each next action.
13+
+The Assets upload HTTP 500 was reproduced and traced to the Local API data-provider persistence boundary. No executable code was changed because the failure is not caused by the Charlie migration and the safe fix requires a separately scoped persistence/API decision.
1414
+
15-
+No implementation files were changed.
15+
+## Root Cause
16+
+
17+
+The upload worker successfully loads and returns file bytes to the Assets page. The failure occurs later when the browser calls:
18+
+
19+
+`POST /api/toolbox/assets/repositories/assets-1/methods/addAssetRecord`
20+
+
21+
+The Local API route executes `addAssetRecord`, then persists asset runtime tables through:
22+
+
23+
+- `repositoryMethodRequiresPersistence(methodName)`
24+
+- `persistAssetProviderState(action)`
25+
+- `persistSupabaseAssetSnapshot(action)`
26+
+- `upsertSupabaseProductTables(tables, action)`
27+
+- `SupabasePostgresProviderAdapter.upsertProductTables(...)`
28+
+
29+
+The configured product data adapter request fails with:
30+
+
31+
+`fetch failed`
32+
+
33+
+The API response includes the expected boundary rule:
34+
+
35+
+`Browser -> Server API -> Data Source`
36+
+
37+
+## Migration Causality
38+
+
39+
+Caused by migration: NO.
40+
+
41+
+Evidence:
42+
+
43+
+- The canonical Assets entrypoint loads successfully.
44+
+- The retained upload worker starts successfully from `toolbox/assets/assets-upload-worker.js`.
45+
+- The failure happens only after the worker completes and `addAssetRecord` calls the server data persistence layer.
46+
+- The failed response is from Local API persistence, not from a missing script, missing worker, import error, or browser module path.
1647
+
1748
+## Files Reviewed
1849
+
19-
+- `docs_build/dev/ProjectInstructions/README.txt`
20-
+- `docs_build/dev/ProjectInstructions/PROJECT_INSTRUCTIONS.md`
21-
+- `project-instructions/addendums/canonical-repository-structure.md`
22-
+- `project-instructions/addendums/legacy-migration-policy.md`
23-
+- `project-instructions/addendums/codex-artifact-and-reporting-standard.md`
24-
+- `toolbox/controls/controls-api-client.js`
50+
+- `assets/toolbox/assets/js/index.js`
2551
+- `toolbox/assets/assets-api-client.js`
2652
+- `toolbox/assets/assets-upload-worker.js`
27-
+- `toolbox/game-journey/game-journey-api-client.js`
28-
+- `assets/toolbox/controls/js/index.js`
29-
+- `assets/toolbox/assets/js/index.js`
30-
+- `assets/toolbox/objects/js/index.js`
31-
+- `assets/toolbox/game-journey/js/index.js`
32-
+- `account/user-controls-page.js`
33-
+- `scripts/validate-canonical-repository-structure.mjs`
34-
+- `scripts/validate-browser-env-agnostic.mjs`
35-
+- `tests/regression/CanonicalRepositoryStructureGuardrail.test.mjs`
36-
+- `tests/dev-runtime/ProductDataProviderContractHardening.test.mjs`
37-
+
38-
+## Classification
39-
+
40-
+| Exception | Current consumers | Decision | Rationale |
41-
+| --- | --- | --- | --- |
42-
+| `toolbox/controls/controls-api-client.js` | Controls tool and `account/user-controls-page.js` | Move to shared | The file is a browser API helper used by more than one surface. `assets/js/shared/` is canonical and already accepted by the structure guardrail. |
43-
+| `toolbox/assets/assets-api-client.js` | Assets tool and Objects tool | Move to shared | The file is shared by multiple tools. Moving to `assets/js/shared/` avoids placing a second JS file under one tool folder and removes one retained exception. |
44-
+| `toolbox/assets/assets-upload-worker.js` | Assets tool only | Owner review required / keep temporary exception | The canonical rules define tool entrypoints and shared JS, but not tool-local worker module placement. Moving now would require either a new worker path rule or a possibly misleading shared location. |
45-
+| `toolbox/game-journey/game-journey-api-client.js` | Game Journey tool | Move to shared if validation stays clean | The file is a browser API helper. It is not currently shared, but moving to `assets/js/shared/` is guardrail-compliant and avoids introducing an unapproved secondary tool-local JS file. |
46-
+
47-
+## Recommended Execution
48-
+
49-
+1. PR_26172_CHARLIE_038: investigate the existing Assets upload HTTP 500 before touching worker placement.
50-
+2. PR_26172_CHARLIE_039: move `assets-api-client.js` to `assets/js/shared/` if PR_038 confirms upload behavior is not path-coupled; keep the worker as a documented exception unless owner approves a worker path rule.
51-
+3. PR_26172_CHARLIE_040: move `controls-api-client.js` and `game-journey-api-client.js` to `assets/js/shared/` if targeted validation passes.
52-
+4. PR_26172_CHARLIE_041: re-run guardrail and report remaining exceptions.
53+
+- `tests/playwright/tools/AssetToolMockRepository.spec.mjs`
54+
+- `tests/helpers/playwrightRepoServer.mjs`
55+
+- `src/api/server-api-client.js`
56+
+- `src/dev-runtime/server/local-api-router.mjs`
57+
+- `src/dev-runtime/persistence/tool-repositories/assets-mock-repository.js`
58+
+- `src/dev-runtime/auth/provider-contract-stubs.mjs`
59+
+
60+
+## Reproduction Evidence
61+
+
62+
+Targeted probe result:
63+
+
64+
+```json
65+
+{
66+
+ "log": "Batch upload complete: 0 written, 1 failed, 0 skipped, 0 warnings.",
67+
+ "batchLog": [
68+
+ {
69+
+ "status": "FAIL",
70+
+ "text": "FAIL: http-500-probe.png - fetch failed"
71+
+ }
72+
+ ],
73+
+ "workers": [
74+
+ "http://127.0.0.1:<port>/toolbox/assets/assets-upload-worker.js"
75+
+ ],
76+
+ "failedResponses": [
77+
+ {
78+
+ "status": 500,
79+
+ "url": "http://127.0.0.1:<port>/api/toolbox/assets/repositories/assets-1/methods/addAssetRecord",
80+
+ "body": "{\"error\":\"fetch failed\",\"ok\":false,\"rule\":\"Browser -> Server API -> Data Source\"}"
81+
+ }
82+
+ ]
83+
+}
84+
+```
85+
+
86+
+Provider contract route result:
87+
+
88+
+- `/api/providers/contract`: HTTP 200
89+
+- Active provider: `supabase-postgres`
90+
+- Status: `ready`
91+
+- Diagnostic: runtime account and product data connections are fixed to configured server services.
92+
+
93+
+## Fix Decision
94+
+
95+
+Fix applied: NO.
96+
+
97+
+Reason:
98+
+
99+
+- A safe fix would require deciding how Local API Playwright upload validation should persist asset runtime tables when the configured product data adapter is unavailable or unreachable.
100+
+- Options include test-scoped Postgres stubbing, a Local API asset persistence adapter, or improved diagnostics. Each option changes persistence/API behavior and is outside this Charlie retained-exception migration scope.
101+
+- Silently bypassing persistence for `addAssetRecord` would violate the active server data boundary and could hide real provider failures.
53102
+
54103
+## Validation Lane Report
55104
+
56-
+- Report exists: PASS
57-
+- No runtime source changed: PASS
58-
+- Active reference audit: PASS
105+
+- HTTP 500 reproduction: PASS
106+
+- Worker load confirmation: PASS
107+
+- Root-cause trace through Local API persistence: PASS
108+
+- Migration causality check: PASS
109+
+- Runtime source changes: PASS, none.
59110
+- ZIP artifact exists: PASS after artifact creation.
60111
+
61112
+## Branch Validation
@@ -67,14 +118,17 @@ index 000000000..bd470b359
67118
+
68119
+## Requirement Checklist
69120
+
70-
+- Audit `toolbox/controls/controls-api-client.js`: PASS
71-
+- Audit `toolbox/assets/assets-api-client.js`: PASS
72-
+- Audit `toolbox/assets/assets-upload-worker.js`: PASS
73-
+- Audit `toolbox/game-journey/game-journey-api-client.js`: PASS
74-
+- Determine migrate now / keep temporary exception / move to shared / owner review required: PASS
121+
+- Determine root cause: PASS
122+
+- Confirm whether caused by migration or pre-existing persistence/API issue: PASS
123+
+- Fix only if clear and safe: PASS, no fix applied because safe resolution is outside scope.
124+
+- Otherwise document blocker: PASS
75125
+- Do not merge: PASS
76126
+- Produce ZIP artifact: PASS after artifact creation.
77127
+
78128
+## Manual Validation Notes
79129
+
80-
+The safest next code changes are shared-client relocations. The Assets worker should remain a temporary exception until the owner approves canonical worker placement or a guardrail update explicitly permits tool-local worker modules.
130+
+The blocker should be handled by a separate persistence/API validation PR. The retained Assets worker path is not the source of this HTTP 500.
131+
+
132+
+## Recommendation
133+
+
134+
+Continue to PR_039. Move the shared Assets API client if safe, but keep the upload worker as a temporary exception until worker placement governance and upload persistence validation are separately resolved.

0 commit comments

Comments
 (0)