Skip to content

Commit 985c6a9

Browse files
author
DavidQ
committed
Harden Asset Manager add/remove guards without schema changes - PR_11_316
1 parent 86bcddf commit 985c6a9

7 files changed

Lines changed: 309 additions & 8 deletions

File tree

docs/dev/codex_commands.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,3 +152,10 @@ PR_11_315
152152
```bash
153153
npx @openai/codex run --model gpt-5.3-codex --reasoning medium "Implement PR_11_315: Enable core asset management actions in Asset Manager V2 (add/remove entries) with strict required-field validation and workspace persistence."
154154
```
155+
156+
---
157+
PR_11_316
158+
159+
```bash
160+
npx @openai/codex run --model gpt-5.3-codex --reasoning medium "Implement PR_11_316: Harden Asset Manager V2 add/remove actions by rejecting duplicate ids, blank/whitespace fields, and missing remove ids with actionable status messages while preserving valid persistence behavior."
161+
```

docs/dev/commit_comment.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
Enable Asset Manager V2 add/remove asset entries with strict validation and workspace persistence - PR 11.315
1+
Harden Asset Manager V2 add/remove rejection paths and preserve valid persistence behavior - PR 11.316
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# PR_11_316 Report
2+
3+
## Purpose
4+
Harden Asset Manager V2 add/remove behavior with explicit rejection paths while preserving existing valid persistence behavior.
5+
6+
## Files Changed
7+
- `tools/asset-manager-v2/index.js`
8+
- `tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs`
9+
- `docs/pr/PR_11_316_ASSET_MANAGER_ADD_REMOVE_HARDENING/PLAN_PR.md`
10+
- `docs/pr/PR_11_316_ASSET_MANAGER_ADD_REMOVE_HARDENING/BUILD_PR.md`
11+
- `docs/dev/reports/PR_11_316_report.md`
12+
- `docs/dev/codex_commands.md`
13+
- `docs/dev/commit_comment.txt`
14+
15+
## Implementation Summary
16+
- Add action now reports exactly which required field(s) are missing when id/label/kind/path are blank or whitespace-only.
17+
- Duplicate asset id add remains blocked with clear status.
18+
- Remove-by-id remains blocked with clear status when id is missing or not found.
19+
- Valid add/remove still flows through existing validation + session persistence path.
20+
- No schema or manifest contract changes.
21+
22+
## Validation Commands
23+
- `node --check tools/asset-manager-v2/index.js` -> **PASS**
24+
- `node --check tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs` -> **PASS**
25+
- `node tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs` -> **PASS**
26+
27+
## Targeted Test Output
28+
- `tmp/pr_11_316_asset_manager_add_remove_results.json`
29+
- Verified checks:
30+
- duplicate id rejected
31+
- blank/whitespace-only required field rejected
32+
- remove missing id rejected
33+
- valid add persists
34+
- valid remove persists
35+
36+
## Full Samples Smoke Decision
37+
- **Skipped** full samples smoke test.
38+
- Reason: change is strictly scoped to `asset-manager-v2` add/remove validation and has focused runtime test coverage with syntax checks.
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# BUILD_PR_11_316
2+
3+
## Implementation
4+
- Updated `tools/asset-manager-v2/index.js` add/remove hardening:
5+
- blank or whitespace-only add fields are rejected with explicit missing-field messaging
6+
- duplicate asset id rejection remains explicit and blocking
7+
- remove-by-id rejection remains explicit when id is not found
8+
- valid add/remove continues through existing load/validate/persist flow
9+
- Added targeted runtime test:
10+
- `tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs`
11+
- validates rejected add/remove actions and successful persistence path
12+
13+
## Validation
14+
- `node --check tools/asset-manager-v2/index.js`
15+
- `node --check tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs`
16+
- `node tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs`
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# PLAN_PR_11_316
2+
3+
## Purpose
4+
Harden Asset Manager V2 add/remove validation behavior while preserving valid persistence behavior and leaving import/export contracts unchanged.
5+
6+
## Scope
7+
- `tools/asset-manager-v2/index.js`
8+
- `tests/runtime/V2AssetManagerAddRemoveHardening.test.mjs`
9+
- `docs/dev/reports/PR_11_316_report.md`
10+
- `docs/dev/codex_commands.md`
11+
- `docs/dev/commit_comment.txt`
12+
13+
## Steps
14+
1. Tighten add-field rejection messaging for blank/whitespace-only values.
15+
2. Keep duplicate asset id rejection explicit and unchanged in behavior.
16+
3. Keep remove-by-id rejection explicit when the id does not exist.
17+
4. Add targeted runtime test coverage for duplicate, blank field, missing remove id, and valid add/remove persistence.
18+
5. Run targeted syntax and targeted Asset Manager runtime test only.
Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
import assert from "node:assert/strict";
2+
import fs from "node:fs";
3+
import path from "node:path";
4+
import vm from "node:vm";
5+
import { execFileSync } from "node:child_process";
6+
import { fileURLToPath, pathToFileURL } from "node:url";
7+
8+
const __filename = fileURLToPath(import.meta.url);
9+
const __dirname = path.dirname(__filename);
10+
const repoRoot = path.resolve(__dirname, "..", "..");
11+
const toolPath = path.join(repoRoot, "tools", "asset-manager-v2", "index.js");
12+
const resultsPath = path.join(repoRoot, "tmp", "pr_11_316_asset_manager_add_remove_results.json");
13+
14+
function checkSyntax(filePath) {
15+
execFileSync(process.execPath, ["--check", filePath], {
16+
cwd: repoRoot,
17+
stdio: ["ignore", "pipe", "pipe"]
18+
});
19+
}
20+
21+
function createElementRecord(id = "") {
22+
return {
23+
id,
24+
value: "",
25+
textContent: "",
26+
hidden: false,
27+
type: "",
28+
children: [],
29+
listeners: {},
30+
append(...nodes) {
31+
this.children.push(...nodes);
32+
},
33+
appendChild(node) {
34+
this.children.push(node);
35+
return node;
36+
},
37+
replaceChildren(...nodes) {
38+
this.children = [...nodes];
39+
},
40+
addEventListener(eventName, handler) {
41+
this.listeners[eventName] = handler;
42+
},
43+
click() {
44+
if (typeof this.listeners.click === "function") {
45+
this.listeners.click();
46+
}
47+
}
48+
};
49+
}
50+
51+
function createHarness() {
52+
const elementRecords = new Map();
53+
const storage = new Map();
54+
const validSession = {
55+
version: "v2",
56+
toolId: "asset-manager-v2",
57+
payloadJson: {
58+
assetCatalog: {
59+
name: "Catalog",
60+
entries: [
61+
{ id: "ship-1", label: "Ship", kind: "svg", path: "assets/ship.svg" }
62+
]
63+
}
64+
}
65+
};
66+
storage.set("ctx-1", JSON.stringify(validSession));
67+
68+
const document = {
69+
title: "",
70+
body: {
71+
dataset: {}
72+
},
73+
getElementById(id) {
74+
if (!elementRecords.has(id)) {
75+
elementRecords.set(id, createElementRecord(id));
76+
}
77+
return elementRecords.get(id);
78+
},
79+
createElement(tagName) {
80+
const element = createElementRecord(tagName);
81+
element.tagName = tagName;
82+
return element;
83+
}
84+
};
85+
86+
const window = {
87+
location: {
88+
href: "https://example.test/tools/asset-manager-v2/index.html?hostContextId=ctx-1"
89+
},
90+
addEventListener() {},
91+
sessionStorage: {
92+
getItem(key) {
93+
return storage.has(key) ? storage.get(key) : null;
94+
},
95+
setItem(key, value) {
96+
storage.set(key, String(value));
97+
}
98+
}
99+
};
100+
101+
const context = vm.createContext({
102+
window,
103+
document,
104+
URL,
105+
JSON,
106+
Error,
107+
console: { log() {}, error() {} }
108+
});
109+
110+
const rawSource = fs.readFileSync(toolPath, "utf8");
111+
const classSource = rawSource.replace(/\nnew AssetBrowserV2\(\);\s*$/u, "\n");
112+
vm.runInContext(`${classSource}\nthis.AssetBrowserV2 = AssetBrowserV2;`, context);
113+
const instance = new context.AssetBrowserV2();
114+
115+
return { instance, elementRecords, storage, rawSource };
116+
}
117+
118+
function setFormValues(elementRecords, values) {
119+
if (!elementRecords.has("assetManagerV2AddId")) elementRecords.set("assetManagerV2AddId", createElementRecord("assetManagerV2AddId"));
120+
if (!elementRecords.has("assetManagerV2AddLabel")) elementRecords.set("assetManagerV2AddLabel", createElementRecord("assetManagerV2AddLabel"));
121+
if (!elementRecords.has("assetManagerV2AddKind")) elementRecords.set("assetManagerV2AddKind", createElementRecord("assetManagerV2AddKind"));
122+
if (!elementRecords.has("assetManagerV2AddPath")) elementRecords.set("assetManagerV2AddPath", createElementRecord("assetManagerV2AddPath"));
123+
elementRecords.get("assetManagerV2AddId").value = values.id;
124+
elementRecords.get("assetManagerV2AddLabel").value = values.label;
125+
elementRecords.get("assetManagerV2AddKind").value = values.kind;
126+
elementRecords.get("assetManagerV2AddPath").value = values.path;
127+
}
128+
129+
export function run() {
130+
checkSyntax(toolPath);
131+
const { instance, elementRecords, storage, rawSource } = createHarness();
132+
if (!elementRecords.has("assetManagerV2ActionStatus")) {
133+
elementRecords.set("assetManagerV2ActionStatus", createElementRecord("assetManagerV2ActionStatus"));
134+
}
135+
const actionStatus = elementRecords.get("assetManagerV2ActionStatus");
136+
const hostContextId = "ctx-1";
137+
138+
setFormValues(elementRecords, {
139+
id: "dup-asset",
140+
label: "Duplicate",
141+
kind: "png",
142+
path: "assets/duplicate.png"
143+
});
144+
instance.addAssetEntry();
145+
const afterFirstAdd = JSON.parse(storage.get(hostContextId));
146+
setFormValues(elementRecords, {
147+
id: "dup-asset",
148+
label: "Duplicate Again",
149+
kind: "png",
150+
path: "assets/duplicate-again.png"
151+
});
152+
instance.addAssetEntry();
153+
const duplicateMessage = actionStatus.textContent;
154+
const afterDuplicateAttempt = JSON.parse(storage.get(hostContextId));
155+
156+
setFormValues(elementRecords, {
157+
id: " ",
158+
label: "Label",
159+
kind: "svg",
160+
path: "assets/x.svg"
161+
});
162+
instance.addAssetEntry();
163+
const whitespaceMessage = actionStatus.textContent;
164+
const afterWhitespaceAttempt = JSON.parse(storage.get(hostContextId));
165+
166+
instance.removeAssetEntryById("missing-asset-id");
167+
const missingRemoveMessage = actionStatus.textContent;
168+
const afterMissingRemoveAttempt = JSON.parse(storage.get(hostContextId));
169+
170+
setFormValues(elementRecords, {
171+
id: "new-asset",
172+
label: "New Asset",
173+
kind: "png",
174+
path: "assets/new.png"
175+
});
176+
instance.addAssetEntry();
177+
const afterValidAdd = JSON.parse(storage.get(hostContextId));
178+
179+
instance.removeAssetEntryById("new-asset");
180+
const afterValidRemove = JSON.parse(storage.get(hostContextId));
181+
182+
const summary = {
183+
generatedAt: new Date().toISOString(),
184+
checks: {
185+
duplicateIdRejected: duplicateMessage === "Add blocked. Asset id 'dup-asset' already exists.",
186+
duplicateIdDidNotMutateEntries: afterDuplicateAttempt.payloadJson.assetCatalog.entries.length === afterFirstAdd.payloadJson.assetCatalog.entries.length,
187+
whitespaceOnlyFieldRejected: whitespaceMessage === "Add blocked. Missing required field(s): id.",
188+
whitespaceRejectDidNotMutateEntries: afterWhitespaceAttempt.payloadJson.assetCatalog.entries.length === afterDuplicateAttempt.payloadJson.assetCatalog.entries.length,
189+
removeMissingIdRejected: missingRemoveMessage === "Remove blocked. Asset id 'missing-asset-id' was not found.",
190+
removeMissingDidNotMutateEntries: afterMissingRemoveAttempt.payloadJson.assetCatalog.entries.length === afterWhitespaceAttempt.payloadJson.assetCatalog.entries.length,
191+
validAddStillPersists: afterValidAdd.payloadJson.assetCatalog.entries.some((entry) => entry.id === "new-asset"),
192+
validRemoveStillPersists: !afterValidRemove.payloadJson.assetCatalog.entries.some((entry) => entry.id === "new-asset"),
193+
hasNoWhitespaceFallbackPath: !rawSource.includes("payloadJson: {}")
194+
}
195+
};
196+
197+
fs.mkdirSync(path.dirname(resultsPath), { recursive: true });
198+
fs.writeFileSync(resultsPath, `${JSON.stringify(summary, null, 2)}\n`, "utf8");
199+
200+
for (const [key, value] of Object.entries(summary.checks)) {
201+
assert.equal(value, true, `${key} failed`);
202+
}
203+
204+
return summary;
205+
}
206+
207+
if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
208+
try {
209+
const summary = run();
210+
console.log(JSON.stringify(summary, null, 2));
211+
} catch (error) {
212+
console.error(error);
213+
process.exitCode = 1;
214+
}
215+
}

tools/asset-manager-v2/index.js

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,13 @@ class AssetBrowserV2 {
132132
const label = typeof document.getElementById("assetManagerV2AddLabel").value === "string" ? document.getElementById("assetManagerV2AddLabel").value.trim() : "";
133133
const kind = typeof document.getElementById("assetManagerV2AddKind").value === "string" ? document.getElementById("assetManagerV2AddKind").value.trim() : "";
134134
const path = typeof document.getElementById("assetManagerV2AddPath").value === "string" ? document.getElementById("assetManagerV2AddPath").value.trim() : "";
135-
if (!id || !label || !kind || !path) {
136-
return { ok: false, message: "Add blocked. id, label, kind, and path are required.", entry: null };
135+
const missingFields = [];
136+
if (!id) missingFields.push("id");
137+
if (!label) missingFields.push("label");
138+
if (!kind) missingFields.push("kind");
139+
if (!path) missingFields.push("path");
140+
if (missingFields.length > 0) {
141+
return { ok: false, message: `Add blocked. Missing required field(s): ${missingFields.join(", ")}.`, entry: null };
137142
}
138143
return { ok: true, message: "", entry: { id, label, kind, path } };
139144
}
@@ -167,8 +172,9 @@ class AssetBrowserV2 {
167172
this.setActionStatus(newEntry.message);
168173
return;
169174
}
170-
if (this.currentSessionContext.payloadJson.assetCatalog.entries.some((entry) => entry && typeof entry === "object" && !Array.isArray(entry) && typeof entry.id === "string" && entry.id.trim() === newEntry.entry.id)) {
171-
this.setActionStatus(`Add blocked. Asset id '${newEntry.entry.id}' already exists.`);
175+
const normalizedId = newEntry.entry.id.trim();
176+
if (this.currentSessionContext.payloadJson.assetCatalog.entries.some((entry) => entry && typeof entry === "object" && !Array.isArray(entry) && typeof entry.id === "string" && entry.id.trim() === normalizedId)) {
177+
this.setActionStatus(`Add blocked. Asset id '${normalizedId}' already exists.`);
172178
return;
173179
}
174180
const nextSessionContext = this.cloneSessionValue(this.currentSessionContext);
@@ -199,15 +205,16 @@ class AssetBrowserV2 {
199205
this.setActionStatus("Remove blocked. Asset id is required.");
200206
return;
201207
}
208+
const normalizedId = assetId.trim();
202209
const nextSessionContext = this.cloneSessionValue(this.currentSessionContext);
203210
const startLength = nextSessionContext.payloadJson.assetCatalog.entries.length;
204-
nextSessionContext.payloadJson.assetCatalog.entries = nextSessionContext.payloadJson.assetCatalog.entries.filter((entry) => !entry || typeof entry !== "object" || Array.isArray(entry) || typeof entry.id !== "string" || entry.id.trim() !== assetId.trim());
211+
nextSessionContext.payloadJson.assetCatalog.entries = nextSessionContext.payloadJson.assetCatalog.entries.filter((entry) => !entry || typeof entry !== "object" || Array.isArray(entry) || typeof entry.id !== "string" || entry.id.trim() !== normalizedId);
205212
if (nextSessionContext.payloadJson.assetCatalog.entries.length === startLength) {
206-
this.setActionStatus(`Remove blocked. Asset id '${assetId.trim()}' was not found.`);
213+
this.setActionStatus(`Remove blocked. Asset id '${normalizedId}' was not found.`);
207214
return;
208215
}
209216
this.loadContract(nextSessionContext);
210-
this.setActionStatus(`Asset '${assetId.trim()}' removed.`);
217+
this.setActionStatus(`Asset '${normalizedId}' removed.`);
211218
}
212219

213220
logStructuredError(type, message, details) {

0 commit comments

Comments
 (0)