Generate users.yaml from config wizard instead of ALLOWED_USER_IDS

[dcellison]

Summary

• Flip error messages and comments in config.py to prefer users.yaml over ALLOWED_USER_IDS
• Replace the ALLOWED_USER_IDS prompt in make config with a user setup flow that generates users.yaml directly
• Three wizard paths: minimal (ID + name), advanced (+ os_user, home_workspace), reconfiguration (skip if exists)
• Remove ALLOWED_USER_IDS from the env dict output
• Skip CLAUDE_USER prompt when advanced mode already set os_user
• Delete users.yaml.example (wizard generates the file, schema in README)
• Add deprecation log.info nudge when ALLOWED_USER_IDS is used without users.yaml
• All string scalars in generated YAML use yaml.dump via _yaml_scalar() to handle YAML 1.1 boolean keywords and special characters
• Backward compatible: existing installs with ALLOWED_USER_IDS continue to work

Test plan

• 26 new tests: TestValidateTelegramId (6), TestValidateDisplayName (7), TestValidateOsUser (6), TestGenerateUsersYaml (7 including roundtrip through _load_user_configs, YAML boolean keywords, and trailing-dots edge case)
• Updated 4 existing tests to remove ALLOWED_USER_IDS references
• Full suite: 1202 tests passing
• Manual: run make config on a fresh tmp dir and verify users.yaml is generated
• Manual: run make config with existing users.yaml and verify it skips user setup

Fixes #193

[dcellison]

Review by Kai

Review

Warning: _generate_users_yaml docstring is stale

install.py (around the _generate_users_yaml docstring):

Uses string formatting rather than yaml.dump() because the output is trivial and this avoids pulling PyYAML into the generator.

yaml was already imported at the top of the same file for the summary read. The rationale is factually wrong and will confuse future readers. This also means yaml.dump() is available and the string-formatting approach is an active choice that needs a different justification (e.g., "keeps output deterministic and readable").

---

Warning: os_user written to YAML without validation

install.py, _cmd_config / _generate_users_yaml:

admin_telegram_id is validated as a positive integer and admin_name is guarded by _validate_display_name, but admin_os_user is embedded raw:

admin_os_user = _prompt("OS user for subprocess isolation", default_os_user) or None
...
lines.append(f"    os_user: {os_user}")

A YAML-safe regex (e.g. ^[a-zA-Z0-9._-]+$) should be applied the same way _validate_display_name is applied to name. OS usernames don't normally contain special characters, but the validation is missing where it exists for every other field.

---

Warning: home_workspace path with embedded # silently truncates in YAML

install.py, _generate_users_yaml:

lines.append(f"    home_workspace: {home_workspace}")

YAML treats # as a comment delimiter in plain scalars. A resolved path like /opt/kai/work #2 would produce:

    home_workspace: /opt/kai/work #2

which parses as /opt/kai/work — silently wrong. Quoting the value (f' home_workspace: "{home_workspace}"') or using yaml.dump for this line would fix it. The telegram_id and os_user cases are safe, but arbitrary filesystem paths are not.

---

Suggestion: test_strips_whitespace documents surprising behavior

tests/test_install.py, TestValidateTelegramId.test_strips_whitespace:

assert _validate_telegram_id(" 123 ") is True

The validator accepts " 123 ", which gets embedded verbatim as telegram_id: 123 in the YAML. YAML parses this correctly as integer 123, so it doesn't break anything today — but the validator accepting padded strings while the embedder doesn't strip them is an inconsistency worth closing. Strip in the validator or the call site.

---

Suggestion: stale comment on /etc/kai/ detection

install.py, _cmd_config:

# /etc/kai/users.yaml is mode 0600 owned by root, so
# Path.exists() returns False for unprivileged users.

Path.exists() returns False when the file is unreadable only if the parent directory is also unreadable. A 0600 file in a world-executable directory returns True for exists() to any user; the PermissionError comes on open(). The comment should say "readable" or acknowledge the nuance to avoid incorrect mental models.

---

Clean

The backward-compat logic, the three wizard paths, the _load_user_configs roundtrip test, and the removal of ALLOWED_USER_IDS from the env dict all look correct. The _block_etc_kai test helper is the right approach for isolating CI from host state.

[dcellison]

Review by Kai

Review

All three Warnings from the prior review are resolved: the docstring now gives the correct rationale, _validate_os_user was added and applied, and home_workspace is wrapped in double-quotes. The backward-compat logic, wizard flow, and tests look solid overall.

Warning: home_workspace double-quote wrapping is incomplete

install.py, _generate_users_yaml:

lines.append(f'    home_workspace: "{home_workspace}"')

The # comment-delimiter issue from the prior review is fixed, but a path containing a literal " character (valid on POSIX — e.g. /opt/kai/home "2") would produce malformed YAML:

    home_workspace: "/opt/kai/home "2""

Real-world risk is low after expanduser().resolve(), but the fix is one line:

lines.append(f"    home_workspace: {yaml.dump(home_workspace, default_flow_style=True).strip()}")

or simply pass the value through yaml.dump({'home_workspace': home_workspace}) and splice the scalar. Given yaml is already imported, using it for this one field is straightforward.

---

Suggestion: _validate_os_user validates stripped value but call site embeds the raw value

install.py, _validate_os_user / _cmd_config (advanced prompt loop):

def _validate_os_user(value: str) -> bool:
    return bool(value.strip()) and _OS_USER_RE.match(value.strip()) is not None

admin_os_user = _prompt("OS user for subprocess isolation", default_os_user) or None
if admin_os_user is None or _validate_os_user(admin_os_user):
    break

" kai " passes validation (checks "kai") but the unstripped value gets embedded as os_user: kai . PyYAML happens to strip plain-scalar whitespace so it parses correctly, but the validator and the call site disagree on what "valid" means. Strip at the call site (admin_os_user = admin_os_user.strip() or None) — same fix the admin_telegram_id path already applies.

---

Clean

The _block_etc_kai monkeypatch helper, the three wizard branches, the roundtrip test through _load_user_configs, removal of ALLOWED_USER_IDS from the env dict, and the deprecation log.info nudge all look correct. The updated Path.exists() comment is more accurate than the prior version.

[dcellison]

Review by Kai

Both warnings from the prior review are fully resolved: yaml.dump() is now used for home_workspace, and admin_os_user is stripped at the call site. The backward-compat logic, wizard flow, and tests are solid.

One new finding:

---

Suggestion: admin_name not stripped at the call site

install.py, _cmd_config, display-name prompt loop:

while True:
    admin_name = _prompt("Admin display name", "admin", required=True)
    if _validate_display_name(admin_name):
        break

_validate_display_name strips before matching, so " alice " passes validation — but the raw unstripped value is passed to _generate_users_yaml and embedded as:

    name:  alice 

YAML plain scalar parsing strips surrounding whitespace, so it parses correctly, and there is no functional bug. But it's the same inconsistency that existed for admin_os_user (fixed in this revision) and for admin_telegram_id (which already calls .strip() explicitly). The one-line fix:

admin_name = _prompt("Admin display name", "admin", required=True).strip()

---

Clean

Everything else looks correct: the yaml.dump path-escaping, the _block_etc_kai test helper, the three wizard branches, the _load_user_configs roundtrip test, removal of ALLOWED_USER_IDS from the env dict, the deprecation nudge, and the updated Path.exists() comment.

[dcellison]

Review by Kai

Review

All three warnings and the two suggestions from prior reviews are resolved. One new finding:

---

Warning: YAML boolean/null keywords pass _validate_display_name and _validate_os_user but round-trip incorrectly

install.py, _generate_users_yaml lines ~735–745, and the two validator regexes:

Both validators use character-class regexes (^[a-zA-Z0-9 _-]+$ and ^[a-zA-Z0-9._-]+$) that accept YAML 1.1 boolean keywords — yes, no, true, false, on, off and their case variants, plus null — without restriction. These are then embedded unquoted:

lines.append(f"    name: {name}")       # name="yes" → yaml.safe_load gives True
lines.append(f"    os_user: {os_user}") # os_user="no" → yaml.safe_load gives False

PyYAML's safe_load (YAML 1.1) parses unquoted yes as True, no as False, null as None, etc. A display name like "Yes" or OS user "no" would pass validation, generate syntactically valid YAML, but fail the roundtrip — _load_user_configs would receive a boolean or None instead of a string.

The home_workspace field already uses yaml.dump() to handle exactly this class of problem. The same treatment is needed here:

lines.append(f"    name: {yaml.dump(name, default_flow_style=True).strip()}")
lines.append(f"    os_user: {yaml.dump(os_user, default_flow_style=True).strip()}")

Alternatively, tighten the validator regexes to reject pure-alpha strings that match the YAML 1.1 bool/null set, though yaml.dump is more future-proof.

---

Clean

The backward-compat logic, three wizard paths, _block_etc_kai monkeypatch helper, _load_user_configs roundtrip test, removal of ALLOWED_USER_IDS from the env dict, the deprecation nudge, admin_name.strip(), admin_os_user.strip(), and the corrected Path.exists() comment are all correct.

[dcellison]

Review by Kai

Review

All warnings from prior reviews are resolved. One new finding.

---

Warning: _yaml_scalar corrupts os_user values that end with ...

install.py, _generate_users_yaml, inner helper _yaml_scalar:

def _yaml_scalar(value: str) -> str:
    return yaml.dump(value, default_flow_style=True).rstrip("\n").removesuffix("...").rstrip("\n")

The .removesuffix("...") was added to strip the YAML document-end marker, but PyYAML does not append \n...\n when serializing a Python str — that only happens for non-string types (None, True, False, integers). For a Python string, yaml.dump always produces <scalar>\n or '<scalar>'\n.

The side effect: any os_user whose serialized form ends with ... (e.g. "alice...") passes _validate_os_user (dots are allowed by _OS_USER_RE), is serialized by yaml.dump as alice...\n, stripped to alice..., then silently truncated to alice by removesuffix. The user never sees a warning.

_validate_display_name excludes ., so name is not affected. Only os_user is vulnerable.

Fix — remove the unnecessary suffix strip:

def _yaml_scalar(value: str) -> str:
    return yaml.dump(value, default_flow_style=True).rstrip("\n")

---

Suggestion: docstring claim about ... is inaccurate

install.py, _generate_users_yaml:

# yaml.dump() appends "\n...\n" (document end marker) for plain
# scalars and "\n" for quoted ones.

This is backwards. PyYAML appends \n...\n for non-string scalars (booleans, numbers, null) — for string scalars it always produces just <value>\n or '<value>'\n. The comment should be corrected alongside removing the .removesuffix call to avoid re-introducing the same mistake later.

---

Clean

The three-path wizard flow, _block_etc_kai monkeypatch helper, admin_name.strip(), admin_os_user.strip(), _load_user_configs roundtrip test, removal of ALLOWED_USER_IDS from the env dict, deprecation log.info, updated Path.exists() comment, and test_yaml_boolean_keywords_roundtrip all look correct.

[dcellison]

Review by Kai

Review

All warnings from the prior review thread are resolved. The code is in good shape. One new finding:

---

Suggestion: PR description test count is stale

The description says "18 new tests: TestValidateTelegramId (6), TestValidateDisplayName (7), TestGenerateUsersYaml (5)", but the diff adds:

• TestValidateTelegramId: 6 ✓
• TestValidateDisplayName: 7 ✓
• TestValidateOsUser: 6 (not mentioned at all)
• TestGenerateUsersYaml: 7 (test_yaml_boolean_keywords_roundtrip and test_os_user_with_trailing_dots_not_corrupted were added during review iterations, but the description wasn't updated)

Total: ~26 new tests. Not a code issue, but the description should be updated before merge to avoid confusing git log readers.

---

Clean

• Backward-compat logic, three wizard paths, _block_etc_kai monkeypatch helper, and _load_user_configs roundtrip test are all correct.
• The /etc/kai/ early-exit in test_reads_existing_defaults is safe without _block_etc_kai because PROJECT_ROOT / "users.yaml" is created first and the /etc/kai/ branch is never reached.
• config.py error message rewording and the log.info deprecation nudge are correct and placed in the right else branch.
• admin_name.strip(), admin_os_user.strip(), yaml.dump-based path escaping, YAML 1.1 boolean keyword quoting via _yaml_scalar, and removal of ALLOWED_USER_IDS from the env dict all look correct.

[dcellison]

Review by Kai

Review

One warning on the _yaml_scalar implementation. Everything else looks clean.

---

Warning: _yaml_scalar still corrupts os_user values ending in ...

install.py, _generate_users_yaml, inner _yaml_scalar:

def _yaml_scalar(value: str) -> str:
    return yaml.dump(value, default_flow_style=True).rstrip("\n").removesuffix("...").rstrip("\n")

For a Python str, PyYAML does not append the \n...\n document-end marker — that only occurs for non-string scalars (True, None, integers). yaml.dump("test...", default_flow_style=True) emits test...\n. After .rstrip("\n") → test..., then .removesuffix("...") → test. An os_user value like "test..." (dots are permitted by _OS_USER_RE) is silently truncated to "test" in the generated YAML.

test_os_user_with_trailing_dots_not_corrupted asserts the roundtrip value equals "test..." and would fail with this implementation.

This was identified as a warning in prior review #5 and declared resolved in review #6, but the current diff still contains the unfixed code — the .removesuffix("...") call and the inaccurate docstring both survived unchanged, suggesting a regression between those review iterations and the current push.

Fix — drop the no-op suffix strip:

def _yaml_scalar(value: str) -> str:
    return yaml.dump(value, default_flow_style=True).rstrip("\n")

Also update the inline comment, which claims ... is appended for "plain scalars" — that is backwards; PyYAML only appends it for non-string types.

---

Clean

• Backward-compat logic, three wizard paths, _block_etc_kai monkeypatch helper, and _load_user_configs roundtrip test are all correct.
• admin_name.strip(), admin_os_user.strip(), YAML 1.1 boolean-keyword quoting, and removal of ALLOWED_USER_IDS from the env dict all look correct.
• The deprecation log.info nudge is correctly placed in the else branch (not reached when users.yaml is present).
• The updated Path.exists() comment, test_reads_existing_defaults fix (pre-seeded users.yaml makes _block_etc_kai unnecessary there), and test_advanced_user_options CLAUDE_USER-omission coverage are all correct.
• Error messages in config.py correctly reframe users.yaml as primary and ALLOWED_USER_IDS as legacy fallback.