Skip to content

fix: import and appservice gaps; 1.162.0:1 → 1.162.0:2 - #98

Merged
MattDHill merged 1 commit into
masterfrom
fix/import-and-appservice-gaps
Oct 9, 2026
Merged

MattDHill merged 1 commit into
masterfrom
fix/import-and-appservice-gaps

Conversation

@MattDHill

@MattDHill MattDHill commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Fixes found while moving matrix.start9labs.com onto StartOS.

  • Import: old_signing_keys: with only comments beneath it (how synapse generate writes it) reads as null and failed the import with a misleading "missing server_name, macaroon_secret_key or form_secret". It is now accepted as no old keys, and a rejected file names the offending key, or says it isn't valid YAML.
  • Set Admin Password on an imported homeserver updated the oldest account's password — on an imported server, one of its real users. It now targets @admin:<server_name>, registering it if absent. Servers created here keep the old behaviour (oldest account, the admin the install task registered under whatever name older releases used). An import is recognised by signing_key_path no longer being the install's placeholder key, so homeservers imported by 1.162.0:1 are covered without a migration. An existing @admin that is deactivated or not an admin is never promoted or reactivated: the password stays pending and the oneshot logs a warning on each start until the account is fixed. It deliberately doesn't fail — nginx requires apply-admin-password, so a throw would take the whole homeserver, Ketesa included, offline with no in-band way to fix the account.
  • Set Server Address/URL hides Import Existing Homeserver once it runs. Its task reason and warning now point importers at the import action, and instructions.md says to import in its place.
  • Register Appservice: the URL is optional and saved as url: null, which Synapse treats as push nothing — what a send-only bot's registration carries. The field is renamed Appservice URL. IDs are restricted to [A-Za-z0-9._-], since each names a file. List Appservices shows "None" rather than failing on a null URL. No package in the fleet calls ensureAppserviceRegistration yet, so the ID pattern breaks no dependent.

🤖 Generated with Claude Code

@MattDHill
MattDHill requested a review from helix-a October 9, 2026 05:05

@helix-a helix-a left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One blocker: the imported-server password flow selects @admin:<server_name> by name and updates only password_hash (startos/main.ts:413–446). An imported server can have an older active administrator under another name and a later @admin that is non-admin or deactivated. Previously that older administrator received the reset; now the oneshot clears the pending password although the returned credentials cannot administer the server, or cannot log in at all. Synapse privileges and login eligibility depend on users.admin and users.deactivated, not the username.

The proposal checks that an existing imported @admin is an active administrator before changing its password. Otherwise it fails explicitly and retains the pending operation, without silently promoting or reactivating an imported user. Absent-account registration and locally created homeservers remain unchanged. README and instructions describe this restriction.

Proposed changes: 7e6e509...proposal/98

Verification: merge-tree passes; all PR checks (DiscoverMatrix, x86, arm) passed; SDK 3.0.3 matches npm's latest. The proposal passes npm ci, TypeScript, formatting, and make x86/package packing. Six actual oneshot callback scenarios passed with mocked SDK/container I/O; the returned package contains the guard. Independently reviewed the proposal and checked flag semantics against Synapse v1.162.0. No live StartOS/login test performed. No other blockers found in the changed import/appservice behavior.

Helix-Harness: pi
Helix-Model: openai-codex/gpt-6.1-sol

@MattDHill
MattDHill force-pushed the fix/import-and-appservice-gaps branch from 7e6e509 to b8d1afc Compare October 9, 2026 05:37
@MattDHill
MattDHill requested a review from helix-a October 9, 2026 05:39
…:1 → 1.162.0:2

- Accept an import whose old_signing_keys is null, as synapse generate
  writes it, and name the offending key when the import rejects a file.
- Set Admin Password on an imported homeserver targets @admin:<server>,
  registering it if absent, instead of the oldest account, which there
  is one of the old server's users. Imported servers are recognised by
  signing_key_path, so servers imported by earlier releases are covered.
  An existing @admin that is deactivated or not an admin is left alone:
  the password stays pending with a warning rather than failing the
  oneshot nginx waits on.
- The Set Server Address/URL task and its warning point importers to
  Import Existing Homeserver, since claiming a name hides it.
- Register Appservice: URL is optional and written as null for send-only
  bots; IDs are restricted to filename-safe characters. List Appservices
  shows a null URL instead of failing to read the entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MattDHill
MattDHill force-pushed the fix/import-and-appservice-gaps branch from b8d1afc to fdc61c3 Compare October 9, 2026 05:41
@MattDHill
MattDHill requested review from helix-a and removed request for helix-a October 9, 2026 05:41
helix-a added a commit that referenced this pull request Oct 9, 2026
Helix-Harness: pi
Helix-Model: openai-codex/gpt-6.1-sol

@helix-a helix-a left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed fdc61c3. The original active-admin blocker is resolved: an incompatible imported @admin keeps its old password and the queued reset stays pending. Warning and returning successfully also preserves nginx/Ketesa access for account recovery. My earlier throwing proposal overlooked nginx's oneshot dependency; the revised approach is correct. The replacement proposal/98 is unnecessary for this head.

No remaining code blockers found. Merge-tree and diff checks pass; SDK 3.0.3 remains current. Six actual oneshot callback scenarios passed with mocked SDK/container I/O, including deferred recovery, valid imported admin, absent imported account, both local account paths, and no pending password. No live login test performed.

Approval is pending the x86 and arm CI builds on this head. DiscoverMatrix passed.

Helix-Harness: pi
Helix-Model: openai-codex/gpt-6.1-sol

@MattDHill
MattDHill merged commit 1d04a28 into master Oct 9, 2026
3 checks passed
@MattDHill
MattDHill deleted the fix/import-and-appservice-gaps branch October 9, 2026 05:45

@helix-a helix-a left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved fdc61c3; unchanged since the completed follow-up review. The imported-account blocker is resolved without changing incompatible accounts or blocking nginx/Ketesa recovery access. No remaining blockers in the reviewed import/appservice changes.

All current-head checks passed: DiscoverMatrix, x86 build, and arm build. Fresh merge-tree and diff checks pass; SDK 3.0.3 matches npm's latest. Reused the six passing mocked oneshot callback scenarios from the prior review. No live StartOS/login test performed.

Helix-Harness: pi
Helix-Model: openai-codex/gpt-6.1-sol

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