Skip to content

fix(system-agent): resolve the platform owner through ADMIN_USERNAME, not a literal "admin" (#3262) - #3268

Merged
vybe merged 4 commits into
devfrom
feature/3262-system-agent-owner
Oct 7, 2026
Merged

vybe merged 4 commits into
devfrom
feature/3262-system-agent-owner

Conversation

@dolho

@dolho dolho commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The system agent, the Cornelius seed and the default system seed each hard-coded the owner username "admin". The admin account is created as utils.admin_identity.admin_username(), which honours ADMIN_USERNAME. On an ADMIN_USERNAME=root install:
    • the system agent was never created (Admin user 'admin' not found);
    • the Cornelius seed and the default system seed deferred forever with "Admin user not present yet (pre-setup)", even though setup had completed.
  • All three now call admin_username() at use time; the constants SYSTEM_AGENT_OWNER, CORNELIUS_OWNER and SEED_OWNER are removed. bug(security): fresh install leaves setup_completed=false with a real admin — unauthenticated /api/setup/admin-password overwrites the admin password #2381 fixed the same mismatch in routers/setup.py.
  • The issue named only the system agent; the two seeders are the same bug and are fixed here with the reviewer's agreement.
  • Existing installs are unaffected: register_agent_owner never re-owns an agent_ownership row that already exists (on IntegrityError it only sets is_system).

Changes

  • services/system_agent_service.py: create path (lookup, error message, MCP key owner, ownership, default permissions) and the running-branch re-registration.
  • services/cornelius_agent_service.py, services/system_seed_service.py: the owner lookup.
  • Comments naming the removed constants: services/onboarding_service.py, routers/settings/retention.py, tests/unit/test_ent319_first_run_front_desk.py.
  • New tests/unit/test_3262_admin_username_owner.py and its tests/registry.json entry.

Test Plan

  • cd tests && pytest unit/test_3262_admin_username_owner.py: 5 passed. Reverting each service file separately fails its own tests (system agent 3, Cornelius 1, system seed 1).
  • Every unit test touching these services, admin_identity or onboarding (19 files), shuffled with --randomly-seed=12345: 480 passed.
  • Live, on a local install with ADMIN_USERNAME=root and fresh data:
    • dev: boot logs System agent: create_failed - Failed to create system agent: Admin user 'admin' not found, and both seeds log "deferring" although setup_completed is true and root can log in.
    • This branch: on the first boot System agent: created, and /api/system-agent/status reports owner root, running, is_system: true. Both seeds proceed instead of deferring.

Fixes #3262

🤖 Generated with Claude Code

… not a literal "admin" (#3262)

The system agent, the Cornelius seed and the default system seed each
hard-coded the owner username "admin". The admin account is created as
utils.admin_identity.admin_username(), which honours ADMIN_USERNAME, so
on an ADMIN_USERNAME=root install the system agent was never created
("Admin user 'admin' not found") and both seeds deferred forever as if
setup had not run. #2381 fixed the same mismatch in routers/setup.py.

All three now call admin_username() at use time. Existing installs are
unaffected: register_agent_owner never re-owns an existing row.

Fixes #3262

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dolho
dolho requested a review from vybe October 6, 2026 10:01
@vybe

vybe commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

merge-train (2026-10-06): ejected — rides the next train once fixed.

The owner fix is right, but it unblocks a stale persisted first-run verdict. _resolve_first_run_verdict (src/backend/services/system_seed_service.py:109-131) computes and persists first_run_fresh before either seeder reaches its owner check. On a non-default ADMIN_USERNAME install that first booted empty, that pass stored true and both seeders then deferred forever at get_user_by_username("admin") (the bug this PR fixes), so cornelius_seeded / default_system_seeded were never set. After this PR the owner is found, fresh=True comes from the stored verdict (the live count_non_system_agents() is bypassed when fresh is not None, cornelius_agent_service.py:113), and both seeders provision Cornelius + the bundled default system onto a mature, hand-built fleet. This is the "do NOT surprise an established install" case the seeder's own comment forbids, and it contradicts "existing installs are unaffected".

Suggested shape: don't persist a verdict while the owner row is absent (return None, persist nothing); and reconcile already-stale rows — stored true with neither seed flag set and count_non_system_agents() > 0 → persist false. Plus a test for exactly that state. Minor: tests/unit/test_3262_admin_username_owner.py:78-84 stubs __truediv__ on an instance, so the raise is a TypeError not the FileNotFoundError the comment promises; pytest.raises(Exception) hides it.

@vybe vybe added the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

@AndriiPasternak31 AndriiPasternak31 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.

+1 to @vybe's ejection. I reproduced it by running the real ensure_first_run_seeded() from the stale state:

  • Start: ADMIN_USERNAME=root, first_run_fresh="true" saved at first boot, neither cornelius_seeded nor default_system_seeded set, 7 hand-built agents.
  • dev: Cornelius is not provisioned, and the system seed returns deferred.
  • This PR: Cornelius _provision runs once, the system seed passes its owner check and moves on to deploying the manifest, and brain_orb_enabled is switched on.
    So vybe's (a)–(c) are needed: no saved verdict while the owner row is missing, reconcile already-stale rows, and a test from exactly that state. If that takes a while, the system-agent half is safe to ship on its own, because it has no saved verdict. Hold back the two seeder lines.

One more of the same class that isn't in vybe's note: event_dispatch_service.py:178 mints the EVT-001 loopback JWT with "sub": "admin". get_current_user looks that username up (dependencies.py:682) and returns 401 when it's missing, so on a root install every event-subscription dispatch should fail. I traced this in code and did not run it live. The fix is one line, admin_username(). Fold it in here, or file a follow-up.

Smaller:

  • database.py:873/907 still read os.getenv("ADMIN_USERNAME", "admin") raw, while admin_username() strips whitespace. With ADMIN_USERNAME=" root", the user is created as " root" and every caller looks up "root". Follow-up material.
  • The merge conflict is only tests/registry.json (#3243's entry landed in the same spot). Keep both entries.

What checks out: the owner swap itself is right. Each fix reverted turns its tests red as claimed (3/1/1), and 201 related tests pass. There is no admin-gate change: the system key is still scope=system. ADMIN_USERNAME renders as root in all three compose files and is in .env.example.

dolho and others added 2 commits October 7, 2026 11:07
…n verdict (#3262)

Resolving the owner through ADMIN_USERNAME unblocked a verdict stored while
the owner lookup was failing: an ADMIN_USERNAME=root install that first
booted empty persisted first_run_fresh=true, both seeders then deferred
forever, and neither seed flag was set. Finding the owner would have
provisioned Cornelius and the default system onto a mature fleet.

- _resolve_first_run_verdict persists nothing while the owner row is absent.
- A stored "true" with neither seed flag set and non-system agents present
  is reconciled to "false".
- EVT-001 loopback JWT is minted for admin_username(), not a literal
  "admin" (get_current_user 401'd every event dispatch on a root install).
- The system-agent owner test stubs Path with a class, so the raise is the
  promised FileNotFoundError rather than a TypeError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ent-owner

# Conflicts:
#	tests/registry.json
@github-actions github-actions Bot removed the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 7, 2026
@dolho

dolho commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the 10-06 review (vybe's ejection + @AndriiPasternak31's review) in fbeb1f2d4, and merged dev in 8306a8252.

Stale first-run verdict, items (a)–(c)

  • (a) _resolve_first_run_verdict persists nothing while the owner row is absent. It returns None, and both seeders already treat None as "decide later".
  • (b) A stored true with neither cornelius_seeded nor default_system_seeded set, while count_non_system_agents() > 0, is reconciled to false. Both seeders then mark themselves seeded without provisioning. A real first run that created agents sets at least one of the two flags, so only the deferral in (a) can produce this state.
  • (c) test_a_stale_fresh_verdict_does_not_seed_a_hand_built_fleet runs the real ensure_first_run_seeded() from exactly that state (ADMIN_USERNAME=root, verdict true, no flags, 7 agents). Nothing is provisioned and the verdict becomes false. Two companion tests cover the other cases: an empty install with a stored true still seeds, and no verdict is stored before setup.

Loopback JWT: event_dispatch_service._get_internal_token now mints sub=admin_username(). There's a test for it.

Minor: the system-agent owner test stubs Path with a class, so the raise is the promised FileNotFoundError, matched by message. Before, __truediv__ on a SimpleNamespace instance raised a TypeError.

Pin moved: test_orchestrator_reuses_stored_verdict now also sets cornelius_seeded. Once a seed flag is set the verdict is final and no recount happens. Without any flag, the new reconcile is allowed to count.

Conflict: tests/registry.json, both entries kept.

Mutation check: with the two service files reverted, 3 of the new tests go red. Related suites pass: ent124, 3262, ent614, 2973, ent751, ent319, cornelius, 2215, onboarding — 296 passed.

Left as a follow-up: database.py:873/907 still reads os.getenv("ADMIN_USERNAME", "admin") without stripping it (the " root" case).

🤖 Generated with Claude Code

@vybe vybe 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.

merge-train: batch validated on train/20261007-0858 (train #3329, green)

@vybe
vybe merged commit 032238a into dev Oct 7, 2026
22 checks passed
@vybe

vybe commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

merge-train (2026-10-07): merged as part of train #3329 (green). Before the squash, dev was merged into this branch because an earlier sibling had appended to the same files (tests/registry.json and/or docs/memory/feature-flows*.md). Both sides were kept, nothing else was changed.

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.

3 participants