Repository navigation
Treat an absent storyboard key as no change - #203
Conversation
orch.py and util/gcs_wrapper.py contained two copies of the same GCS IAM signing-credential caching logic. PR #195 fixed only the gcs_wrapper.py copy, leaving the hot path in orch.py (upload URLs, single GETs, and batch signing) running the stale, un-locked copy with dead refresh-on-expiry logic. Delete the duplicate caching logic in orch.py and delegate to the single public get_signing_context() in util/gcs_wrapper.py. In addition, fix util/gcs_wrapper.py to detect non-service-account ADC credentials from local development and raise a clear, actionable RuntimeError explaining that URL signing requires a service account identity (pointing to impersonation or service account key instructions in DEVELOPING.md) rather than raising an unhelpful AttributeError. TAG=agy CONV=ec862d59-a9a7-4a7c-9eae-b59095e8cd08
SM-6: In orch.py, _write_project_doc and _write_editor_project_doc previously coerced an absent storyboard key in a PATCH payload to [], causing keep_ids to be empty and deleting every scene document in the scenes subcollection. Distinguish absent storyboard from an explicit empty list. When 'storyboard' is omitted from the PATCH payload, leave the scenes subcollection completely untouched. An explicit 'storyboard': [] continues to delete all scenes. Existing create semantics are preserved. Reachability: Not reachable from the shipped UI: there is exactly one project PATCH call site (ui/.../config.ts:1293) and it always sends a full cloned ProjectConfig where storyboard is required (config.ts:295) and initialised to [] (config.ts:604). Every .patch( in ui/src, scripts and tools was audited. It matters because a partial PATCH is the most natural third-party call to make against a documented endpoint. Stacked on #198.
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
Review: merge after small fixesMulti-agent review (reviewer → independent critique agent re-verifying each claim against the code). The mechanism is the simplest thing that works — Worth doing: pin the asymmetry this createsThe default PATCH still does a full Storyboard becomes the one field with PATCH semantics inside a PUT-shaped endpoint. I'd keep it that way — widening the fix is a much bigger change than this PR should carry — but it should be deliberate rather than latent:
Suggested deletions (~90 of 178 test lines)
That leaves absent→keep and explicit- Nits
Explicitly not recommended
Process notes
|
Consolidated ReviewVerdict: Merge after one small fix, once #198 has landed. This note reconciles the 19:50 comment on this PR against head Do before merge
Optional, does not block
Rejected or superseded, do not re-litigate
Evidence
Way forward
|
…o fix/project-patch-absent-storyboard
…o fix/project-patch-absent-storyboard
The base branch was changed.
|
Still need readability approvals from:
|
2 similar comments
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Readability approvals granted:
|
![gps-readability-bot[bot]](https://avatars.githubusercontent.com/u/106399418?s=60&v=4){kind=small}
| if create or 'storyboard' in payload: | ||
| scenes = payload.get('storyboard') | ||
| if not isinstance(scenes, list): | ||
| scenes = [] |
There was a problem hiding this comment.
If a caller sends "storyboard": null (or another non-list value) in a PATCH payload—which is common in clients that serialize unset optional fields as null—'storyboard' in payload evaluates to True, scenes is coerced to [], keep_ids becomes empty, and every scene in projects/<id>/scenes is still deleted. Additionally, when create=True and storyboard is omitted or None, root['storyboard'] = [] is already set on line 1182 and the loop body is a no-op.
Checking isinstance(scenes, list) directly ensures the scenes subcollection is only written or pruned when an explicit list ([...] or []) is provided:
| if create or 'storyboard' in payload: | |
| scenes = payload.get('storyboard') | |
| if not isinstance(scenes, list): | |
| scenes = [] | |
| scenes = payload.get('storyboard') | |
| if isinstance(scenes, list): |
There was a problem hiding this comment.
Agreed, and applied. I reproduced it on the PR head: PATCH {"name": "n", "storyboard": null} deleted all 3 scenes on every route.
_write_project_doc now writes and prunes scenes only when isinstance(scenes, list) is true. An explicit [] still clears the storyboard. For create=True with the storyboard omitted, the result is the same as before (root storyboard: [], no scene writes). I also updated the docstring.
New test test_project_patch_non_list_storyboard_leaves_scenes_intact runs 3 routes × 3 values (null, a string, an object). It fails 9/9 against the previous PR head and passes with the fix. Full Python suite is green.
| if 'storyboard' in payload: | ||
| scenes = payload.get('storyboard') | ||
| if not isinstance(scenes, list): | ||
| scenes = [] | ||
| root_updates[field_path.FieldPath('storyboard').to_api_repr()] = [] |
There was a problem hiding this comment.
Similar to _write_project_doc, if a PATCH payload to the editor endpoint includes "storyboard": null (or a non-list value), 'storyboard' in payload is True and scenes = [] deletes every scene in the subcollection. Furthermore, because lines 1218–1221 copy all payload keys into root_updates, setting root_updates['storyboard'] = [] unconditionally before checking isinstance(scenes, list) ensures a non-list storyboard value in payload can never overwrite the root document's storyboard: [] placeholder:
| if 'storyboard' in payload: | |
| scenes = payload.get('storyboard') | |
| if not isinstance(scenes, list): | |
| scenes = [] | |
| root_updates[field_path.FieldPath('storyboard').to_api_repr()] = [] | |
| root_updates[field_path.FieldPath('storyboard').to_api_repr()] = [] | |
| scenes = payload.get('storyboard') | |
| if isinstance(scenes, list): |
There was a problem hiding this comment.
Agreed, and applied as suggested. The editor path now sets the root storyboard placeholder to [] unconditionally, before the isinstance(scenes, list) check. As a result, a non-list value copied from payload can never land on the root document, and scenes are written or pruned only for an explicit list.
Mutation check: dropping that unconditional line makes the editor/?view=editor cases of both the new non-list test and the shrink test fail (8 failures).
| scenes = _scenes_docs(fake_db, project_id) | ||
| assert len(scenes) == 3 | ||
| assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2'] |
There was a problem hiding this comment.
Because _read_project_doc (GET /api/projects/<id>) unconditionally overwrites data['storyboard'] from scenes_ref.stream(), the GET assertion on line 1708 would still pass even if _write_editor_project_doc marked the root document's storyboard field with DELETE_FIELD. Asserting on the stored root document in fake_db directly verifies the root storyboard == [] preservation invariant across all three routes:
| scenes = _scenes_docs(fake_db, project_id) | |
| assert len(scenes) == 3 | |
| assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2'] | |
| assert fake_db.collection('projects').docs[project_id]['storyboard'] == [] | |
| scenes = _scenes_docs(fake_db, project_id) | |
| assert len(scenes) == 3 | |
| assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2'] |
There was a problem hiding this comment.
Good point: GET rebuilds storyboard from the subcollection, so it would hide this regression. I added the direct fake_db root assertion here (with a short comment explaining why). The new non-list test uses the same check.
| scenes = _scenes_docs(fake_db, project_id) | ||
| assert len(scenes) == 3 | ||
| assert set(scenes.keys()) == {'000000', '000001', '000002'} |
There was a problem hiding this comment.
In _write_editor_project_doc, root_updates.update(...) initially copies the full payload['storyboard'] list into root_updates before root_updates['storyboard'] = [] resets it to an empty list. Asserting that the stored root document in fake_db still has storyboard == [] after a PATCH that supplies scenes verifies that scenes are never persisted inline on the root document:
| scenes = _scenes_docs(fake_db, project_id) | |
| assert len(scenes) == 3 | |
| assert set(scenes.keys()) == {'000000', '000001', '000002'} | |
| assert fake_db.collection('projects').docs[project_id]['storyboard'] == [] | |
| scenes = _scenes_docs(fake_db, project_id) | |
| assert len(scenes) == 3 | |
| assert set(scenes.keys()) == {'000000', '000001', '000002'} |
There was a problem hiding this comment.
Applied. The shrink test now asserts the stored root storyboard == [] after a PATCH that supplies scenes. It catches the regression: removing the editor path's root_updates['storyboard'] = [] makes this test fail for /editor and ?view=editor, because the scenes are then stored inline on the root.
A PATCH carrying "storyboard": null (or any non-list value) was treated like [] and deleted every scene. Both the full and editor write paths now touch the scenes subcollection only when storyboard is a list, and the editor path always pins the root storyboard placeholder to [] so a non-list value can never be stored on the root document. Tests assert the stored root placeholder directly (GET rebuilds storyboard from the subcollection and would mask a regression) and cover null, string and object storyboard values on all three PATCH routes. Addresses review feedback from victor-paunescu on #203.
4 participants
Summary
Fixes SM-6: In
orch.py, both_write_project_docand_write_editor_project_docpreviously coerced an absentstoryboardkey in a PATCH payload to[]. This resulted in an emptykeep_idsset, inadvertently deleting every scene document in thescenessubcollection (e.g.PATCH {"name": "Renamed"}orPATCH {}wiped all scenes).This change distinguishes an absent
storyboardkey from an explicit empty list:'storyboard'is omitted from the PATCH payload, thescenessubcollection is left completely untouched. In_write_editor_project_doc, omittingstoryboardalso preserves the root doc'sstoryboard: []without marking itDELETE_FIELD.inputConfig) are still removed. Editor PATCH additionally preserves omittedinputConfig; other omitted mutable root fields retain their replacement behavior.'storyboard': []continues to delete all scenes.create=True) are preserved.Reachability
Not reachable from the shipped UI: there is exactly one project PATCH call site (
ui/.../config.ts:1293) and it always sends a full clonedProjectConfigwherestoryboardis required (config.ts:295) and initialised to[](config.ts:604). Every.patch(inui/src,scriptsandtoolswas audited. It matters because a partialPATCHis the most natural third-party call to make against a documented endpoint.Note on Ownership & Identity Check
Investigation into per-user ownership check on
orch.py:1434-1438:README.md("Projects and generated media are shared across everyone who is admitted. Scene Machine is built for a trusted team: any admitted user can see, open, edit, and delete any project... There is no per-user or per-group ownership.") and in the docstring ofproject_detail_handlerinorch.py:1403-1408("SHARED-TEAM MODEL (intentional): there is deliberately NO per-user ownership check on any method. Every IAP-admitted user may read, edit and delete every project (createdBy is a display label, not an access gate)").AUTH_MODE=none(local dev),_request_email()returnsNone, so projects have no owner recorded.test_projects_crud_flow_under_iapintest/test_frontdoor_data.py) explicitly assert that User 2 may modify User 1's project.Tests
Extended
test/test_frontdoor_data.pyusing the in-fileFakeUiDbandFakeBatchcovering:PATCH {"name": "Renamed"}leaves all 3 scenes intact (the regression);PATCH {}leaves scenes intact;PATCH {"storyboard": []}still deletes all scenes (must not regress);PATCHwith 3 scenes when 5 exist deletes exactly the trailing 2 (control);_write_project_doc(/api/projects/<id>) and_write_editor_project_doc(/api/projects/<id>/editorand?view=editor) paths.Stacked on #198 (
refactor/dedupe-gcs-signing).Follow-up validation
inputConfig, while both editor routes preserve it.main.