Implementation review: PB-018, PR 685

Field	Value
Backlog item	PB-018 Publish matches Fabric items by id, not display name (feature FEAT-FAB-06)
Pull request	#685, merged as 2d8b8f91
Package	pr-ready/PB-018.md
Reviewer	Engineering quality review thread. It gave #685 its quality verdict (PASS) but didn't write it.
Date	2026-10-26
Environment	Linux container, main d04934c3, Python venv for the engine. No tenant, no Windows.

Outcome#

Accepted with Documented Limitations (recommended).

All three acceptance criteria are met with tests that fail on the code before #685. The package's manual check on a tenant (section 13) hasn't been run. FEAT-FAB-06 already says so ("not yet checked against a live tenant"), and its next action puts publish after a rename into the tenant smoke test.

Recommended by: Engineering quality review. Decided by (product owner): Chris, "I accept all 4" on 2026-10-26 (project message cmsg_018T4o2pQdpbLBNhHpgVNNGpREfN9JPWyocAdUEQR2tzj9), which took this outcome.

Acceptance criteria#

#	Criterion (copied from the backlog item)	Result	Evidence
1	Test: publish, rename locally, publish again: one item, updated.	Met	`test_publish_renames_the_fabric_item_after_a_rename_here_and_makes_no_second_one` (sidecar/tests/test_notebook_files.py): same item id, renamed to the new name, one item in the workspace. On the pre-#685 code it fails with `nb-2 != nb-1` (a second item).
2	Test: recorded item deleted: new item created with a note.	Met	`test_when_the_linked_item_is_gone_publish_checks_the_one_with_its_name`: with the recorded item and any same-name item gone, publish makes a new item and returns "was deleted, so a new one was made" in `note`; the toast shows it (app/ui/app.js publishNotebook). On the old code it fails with `KeyError: 'note'`.
3	Existing publish tests pass.	Met	`pytest tests/test_notebook_files.py tests/test_notebook_publish.py tests/test_cloud.py`: 47 passed on main d04934c3.

Checks#

Check	Result	Evidence or note
Scope matches the item and package	Met	Publish (`cloud_service._publish`), Run in Fabric (publishes through `_publish`), and the schedule lookups (`notebook_schedules`, `delete_notebook_schedule`) use `_published_item` first. Other item types are untouched.
New tests fail without the change, pass with it	Met	Swapping in the pre-#685 `cloud_service.py`, `fabric_api.py`, `notebook_files.py` and `notebook_files_service.py`: all 3 new tests fail (`assert True is False`, `KeyError: 'note'`, `nb-2 != nb-1`). With main's files, they pass.
Full suites pass (pytest, UI, cargo)	Partly	The publish, notebook files and cloud suites pass (47). The full suites ran green on #685 at its quality review; cargo isn't buildable in this container (no gdk-3.0), and #685 doesn't touch Rust.
Reachable where docs say; Preview behavior right; packaged into the engine	Met	Notebook ribbon > Publish (docs/notebooks.md, publish step 4). Engine-only change, packaged with the sidecar.
OneLake data safety	Met	Updating by id stays inside the publish workspace: `fabric_api.get_notebook` returns None when the item's `workspaceId` differs, so a recorded id from another workspace is never written. A rename in Fabric is kept unless the notebook was renamed here (`_fabric_item_name` compares with the name stored at the last publish). Someone else's same-name item is no longer overwritten when the linked item was renamed in Fabric (`test_publish_updates_the_linked_item_when_it_was_renamed_in_fabric`).
Authentication and distribution	Met	No new scope: GET and PATCH on the notebook item use the existing Fabric item scopes.
No unproven local-equals-Fabric claim; Preview tags kept where untested	Met	The inventory says it isn't checked on a tenant.
Docs match the code; linked; strict docs build passes	Met	docs/notebooks.md step 4 matches `_published_item` and `_fabric_item_name` (rename here renames in Fabric, a Fabric rename is kept, a deleted item is replaced and the toast says so).
Accessibility of new controls	n/a	No new controls; the note is in the existing toast.
Inventory and backlog updated; `product_data.py` consistent	Partly	#685 updated FEAT-FAB-06 in FEATURE_INVENTORY.md only, not in FEATURE_INVENTORY.json, so the next `product_data.py render` (merge of #720) put back "Items matched by display name". This review's branch restores it in the JSON. `product_data.py check` says "Product data is consistent" either way, because it doesn't compare the rendered tables. PRODUCT_BACKLOG.md still lists PB-018 as Proposed (line 582).

Findings#

#	Severity (blocking / should fix / note)	Finding	Where	Status
1	note	PB-018's backlog state is still Proposed although #685 implemented it.	docs/product/PRODUCT_BACKLOG.md line 582	Open: set it with the product owner's decision
2	should fix	FEAT-FAB-06's update from #685 was lost on main: it was made in the generated Markdown, and the JSON source kept the old text.	docs/product/FEATURE_INVENTORY.json	Fixed on this review's branch (JSON updated, tables rendered)
3	note	Package section 8 allows one item GET per publish; publish also reads the local `published/<id>.json` for the last name. That's a file read, not an API call, so it stays within the requirement.	cloud_service._fabric_item_name	No change needed

Documented limitations (only for Accepted with Documented Limitations)#

Limitation	Where users are told	Backlog item that tracks it
Publish after a rename hasn't been checked against a live tenant (package section 13).	FEAT-FAB-06 known limitations, docs/product/FEATURE_INVENTORY.md	The tenant smoke test (FD-101, readiness gate G1), named in FEAT-FAB-06's next action

Inventory and backlog changes#

Features changed: FEAT-FAB-06: limitation, next action and #685 restored in FEATURE_INVENTORY.json and rendered.
Backlog item state: PB-018 Proposed → Accepted, on the product owner's decision.