Skip to content

Fix saved custom org state across project switches - #40

Open
shipdocs wants to merge 1 commit into
HKUDS:mainfrom
shipdocs:agent/fix-saved-org-project-switch
Open

Fix saved custom org state across project switches#40
shipdocs wants to merge 1 commit into
HKUDS:mainfrom
shipdocs:agent/fix-saved-org-project-switch

Conversation

@shipdocs

Copy link
Copy Markdown

What changed

  • reload the active saved organization whenever the Office UI activates a different project engine
  • keep malformed or missing org-index state from crashing a project switch
  • add a regression test proving a corporate-configured delegate is rebound to the active custom org

Why

The Office UI could remain in org/custom mode while a newly activated project engine still carried the built-in Corporate configuration. The UI and active-org index then pointed at the saved organization, but organization mutations were rejected as org_read_only.

Impact

Saved custom organizations remain editable after switching projects. The built-in Corporate architecture stays read-only.

Validation

  • python -m pytest -q tests/test_org_saved_crud.py tests/test_role_update_handler.py tests/test_parallel_runtime_isolation.py -k 'org or project_switch or update_role'
  • 23 passed, 16 deselected
  • live Office UI validation confirmed Splinter remained custom after switching to ShipDocs and the Add role control was enabled

@shipdocs
shipdocs marked this pull request as ready for review August 14, 2026 12:17

@LZH-YS1998 LZH-YS1998 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying bug is real, and the happy-path regression test passes on current main, but this activation path is not atomic and can leave project/runtime state inconsistent. I am requesting changes for these blocking cases:

  1. _on_service_engine_activated() installs the new handler engine after OfficeServiceContext.activate_project() has already installed active_engine, then _load_active_org_config_into_engine() calls org_engine.reload_from_config() outside its try. If reload raises, _handle_switch_project() returns ok=false and keeps the client mapped to the old project, while both handler.engine and services_context.active_engine remain on the new project. I reproduced this on current main with an injected reload failure. Subsequent session/work-item requests can therefore target a project different from the one shown by the UI. Please make preparation/commit atomic (or roll both bindings back on failure), and add a regression test for this path.

  2. The new callback is synchronous and replaces the engine config without respecting the shared config_lock. Project switching intentionally preserves background tasks, and org mutations use that lock. A switch can therefore rebind/reload a cached delegate while another org operation or live project runtime is using it. Please make the activation hook awaitable (or otherwise serialize this restore under config_lock) and cover the concurrent/background-task case.

  3. The PR description mentions malformed/missing org_index.yaml, but the test only covers a valid index and valid saved org. In those failure cases the switch succeeds while global mode remains org/custom and the delegate remains corporate; is_custom_org_editable() then returns false and org/talent operations fail with org_read_only. Please define and test a consistent fallback: recover the active org, fail the switch atomically with a visible error, or normalize the mode/profile instead of leaving contradictory state.

Verification performed against merge commit rehearsal on current main: merge is clean; the PR test plus relevant project-switch/org regressions pass (50 tests); three fault-injection cases above reproduce the inconsistent states. GitHub currently reports no CI checks for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants