feat(server): add protocol discovery and persistent identity - #1753
wutongyuonce wants to merge 7 commits into
Conversation
Teingi
left a comment
There was a problem hiding this comment.
Two findings below: a concurrent-startup failure and a lower-priority identity-reset validation issue.
| "identity reset requires --maintenance-confirmed after stopping every Server process" | ||
| ) | ||
| with server_settings_context(env_file=env_file) as settings: | ||
| if isinstance(settings.database, SQLiteConfig) and settings.database.is_in_memory: |
There was a problem hiding this comment.
[P3] Reject SQLite memory URIs before resetting identity
The persistence guard misses accepted SQLite memory URIs such as sqlite+aiosqlite:///file:deployment?mode=memory&cache=shared&uri=true. With this URL, identity-reset --maintenance-confirmed exits successfully and prints a UUID, but reopening the repository yields a different UUID because the database disappeared when the command closed it. Please reject these memory URI forms too, so the command does not report a durable identity rotation that was never persisted.
There was a problem hiding this comment.
Fixed in 154f6a5. SQLiteConfig.is_in_memory remains the single authority and now recognizes URI memory databases when uri=true with either mode=memory or file::memory:. The CLI continues to consume that property rather than parsing URLs itself. Added configuration tests for both accepted URI forms and a CLI regression using the reported URL; identity-reset now exits 2 with the persistent-database error instead of printing a UUID.
There was a problem hiding this comment.
The reported uri=true cases are fixed, but supported alternatives still bypass the guard. For example, sqlite+aiosqlite:///file:deployment?mode=memory&cache=shared&uri=1 is an in-memory database: SQLAlchemy also accepts 1, yes, and on as true. On 1f58fe26, identity-reset --maintenance-confirmed exits successfully and prints a UUID, but reopening yields a different UUID. file:%3Amemory%3A?cache=shared&uri=true has the same result. Please classify the effective SQLite URI, or reject these spellings before resetting, so nonpersistent databases consistently fail the guard.
There was a problem hiding this comment.
@Teingi Fixed in d0872c4. The shared SQLiteConfig.is_in_memory authority now uses SQLAlchemy's effective connection arguments, so its URI boolean coercion is the same as the actual driver. It then classifies the decoded SQLite file URI, covering uri=1/yes/on and file:%3Amemory%3A without duplicating a boolean parser in the CLI. Pool selection and offline maintenance guards continue to consume this single classification.
The reported CLI inputs reproduced the incorrect success before the fix and now exit 2 with the persistent-database error. The existing configuration tests cover true/false aliases and encoded memory paths. Focused identity/discovery/generator/CLI/SQLite tests: 212 passed; related runtime/Server e2e: 24 passed, 3 OceanBase cases deselected. make check, make contract-test, docs build, and package build passed. The bilingual design and PR SPEC are updated; fresh hosted CI is pending.
|
@Teingi Thanks for the review. Both findings are fixed in
I also updated the English/Chinese lifecycle docs and the PR description. Local validation passed: 191 focused tests, |
| async with open_server_identity_repository(config.database) as identity_repository: | ||
| server_id = await identity_repository.load_or_create() |
There was a problem hiding this comment.
The identity database is closed before the runtime opens it. With shared-memory SQLite, two apps can read the same Scope but return different server_ids. Please initialize the identity using the runtime-owned primary database.
There was a problem hiding this comment.
Fixed in 1f58fe2. Server startup now initializes and loads the identity through runtime.primary_database, borrowing the same database that owns Scope data for the Runtime's lifetime. The separate identity database opener is retained only for the offline CLI. Identity schema creation remains idempotent, and initialization/loading errors still abort startup before readiness.
I reproduced the reported mismatch before the fix. The regression now creates a Scope through one app, reads it through a second app sharing the same SQLite memory URI, and checks that both advertise the same server_id. It also verifies that closing one app preserves the surviving app's data and identity, while reopening after all connections close starts a new database and identity.
|
|
||
| _SCHEMA_VERSION = {"major": 1, "minor": 0} | ||
| _FEATURE_CONTRACTS = { | ||
| "access.principal": { |
There was a problem hiding this comment.
Could we declare feature versions and operation membership in OpenAPI and generate this metadata? For example:
x-powercontext-feature-contracts:
scope.selection: {major: 1, minor: 0}
paths:
/v1/scopes:
get:
operationId: list_scopes
x-powercontext-feature-contracts: [scope.selection]This keeps membership alongside the API contract, while version bumps remain explicit.
There was a problem hiding this comment.
Implemented in 1f58fe2 using the suggested extension. The OpenAPI root now declares each feature's explicit major/minor version, and the governed operations declare membership with x-powercontext-feature-contracts.
make api-generate validates and collects these declarations into generated FEATURE_CONTRACTS; Server discovery now consumes that metadata instead of maintaining a separate Python taxonomy. Version bumps remain explicit, and the existing advertised versions and operation lists are preserved. Generator tests cover version edits, multiple memberships, and rejection of invalid versions, undefined/duplicate memberships, and features without operations. The bilingual docs and PR design/authority table are updated as well.
Teingi
left a comment
There was a problem hiding this comment.
Re-reviewed 1f58fe26. The concurrent schema initialization issue is fixed. Two lower-priority issues remain: SQLite memory-URI detection (follow-up in the existing thread) and feature-version validation below.
| or not name.strip() | ||
| or not isinstance(version, dict) | ||
| or set(version) != {"major", "minor"} | ||
| or any(type(value) is not int or value < 0 for value in version.values()) |
There was a problem hiding this comment.
[P3] Reject feature versions that ServerInfo cannot represent
The generator accepts major: 0, but the generated ContractVersion model requires major >= 1. Setting memory.explicit.major to 0 in the OpenAPI extension successfully generates the files, then Server startup fails with a validation error at feature_contracts.memory.explicit.version.major. The checked-in versions are valid, so this affects subsequent contract edits. Validate major >= 1 and minor >= 0 during generation to prevent emitting metadata the Server cannot load.
There was a problem hiding this comment.
@Teingi Fixed in d0872c4. Feature declaration validation now requires major >= 1 and minor >= 0, with strict integer checks retained, so major=0 fails with ContractGenerationError instead of emitting metadata that ServerInfo cannot represent.
I extended the existing invalid-declaration parameterization for major zero/negative/boolean and minor negative/boolean values. The major=0 case failed against the previous generator and passes after the fix; valid major=1/minor=0 remains covered by the existing generation fixtures. make contract-test passed (50 tests and generated-artifact checks), and the bilingual design/PR SPEC now state the numeric bounds explicitly.
Classify memory storage using dialect connection arguments and decoded SQLite URIs so offline reset cannot claim a nonpersistent rotation. Reject feature major zero during generation, matching the discovery model. Extend existing regressions and document both validation boundaries.
Which issue or RFC does this PR close?
Closes #1655.
Rationale for this change
Desktop and other remote consumers need a small authenticated handshake that identifies a PowerContext Server, describes the stable protocol contracts it implements, and lets clients detect whether they are still connected to the same logical deployment. Existing health, capability, and access endpoints answer different questions and do not provide a durable deployment identity.
SPEC and current design
The authoritative wire contract is
openapi/powercontext.yaml. The lifecycle, compatibility, backup/restore, and clone contract is documented indocs/en/development/remote-access-implementation.mdand its Chinese counterpart.User-visible behavior
server.observecan callGET /v1/server-infoand receiveschema_version,product,server_id,package_version,api_contract_version, andfeature_contracts.1.0; the API contract is1.2; the advertised operation groups areaccess.principal,scope.selection, andmemory.explicit, each at1.0.server_id.powercontext server identity-reset --maintenance-confirmedbefore serving clients. Logical data imports preserve no identity unless they includepc_server_identity.PowerContextClient.get_server_info()exposes the generated response model and accepts future optional fields allowed by a compatible schema minor version.Rules and invariants
major >= 1andminor >= 0. A major change may remove or incompatibly change governed semantics; a minor change is backward compatible.schema_versiongoverns the response shape and field semantics.api_contract_versionis derived from OpenAPIinfo.version. Each feature version governs exactly its listed OpenAPI operation IDs.x-powercontext-feature-contractsdeclares each feature version explicitly; the same extension on operations declares membership. The API generator validates these declarations, rejecting feature versions outside the response model's bounds before emitting metadata, and produces the metadata consumed by Server discovery. Membership changes do not automatically bump versions.server_idis an opaque deployment correlation identifier. It is neither the Accessdeployment_idnor proof of trust.Failure, concurrency, and resource semantics
Non-goals
Authority and evidence
powercontext.server.infoprojects generated metadatapowercontext.server.identityWhat changes are included in this PR?
GET /v1/server-infodiscovery to the OpenAPI 1.2 contract and generated Python/host operation catalogs.server_id, and the initial feature contracts.powercontext server identity-reset --maintenance-confirmedfor rotating the identity of an offline independent clone.Are there any user-facing changes?
Yes. This is an additive API change:
GET /v1/server-info(server.observe).PowerContextClient.get_server_info().powercontext server identity-reset --maintenance-confirmed.pc_server_identity.There are no breaking API changes.
How was this change tested?
Local validation on macOS / Python 3.13.13:
make contract-test— 50 passed; Python and JavaScript generated-artifact checks passed.make check— formatting, lint, type checks, lock validation, workflow pins, and 29 integration-manifest tests passed.uv run --frozen python -m pytest -q tests/e2e/test_builtin_runtime.py tests/e2e/test_runtime_server.py -k 'not oceanbase'— 24 passed, 3 OceanBase cases deselected.make docs-test— static build and 832-page link verification passed.make buildandgit diff --checkpassed.make unit-testdid not pass locally: 2,943 passed, 57 skipped, 33 failed, and 57 errors. All failures/errors are in the existing native-code parser tests. The unchanged parser worker callsresource.setrlimit(resource.RLIMIT_AS, (1073741824, 1073741824)); that call independently raisesValueError: current limit exceeds maximum limiton this macOS host. No native-code implementation or tests are changed in this PR update.Hosted checks for the latest commit
d0872c49are pending. The hosted matrix covers Python 3.11–3.14, real OceanBase/SQLite acceptance, seekdb code suites, service/portability checks, and integration packages. The latest real OceanBase/seekdb and full Python version matrix have not been executed locally.AI usage statement
OpenAI Codex (GPT-5 and GPT-6 families) was used for repository analysis, implementation, test drafting, and validation. The resulting contract, code, tests, and documentation were reviewed against the issue, maintainer feedback, repository conventions, and the checks listed above.