Skip to content

fix(config): validate TOML files and base URLs - #194

Open
okcomputer2000 wants to merge 2 commits into
fmagent-project:mainfrom
okcomputer2000:fix-configure-llm-validation
Open

fix(config): validate TOML files and base URLs#194
okcomputer2000 wants to merge 2 commits into
fmagent-project:mainfrom
okcomputer2000:fix-configure-llm-validation

Conversation

@okcomputer2000

Copy link
Copy Markdown
Contributor

Summary

Fix LLM configuration handling for empty TOML files, quoted TOML keys, and invalid base URL ports.

Changes

  • Allow existing empty fm-agent.toml files to be initialized.
  • Update bare, basic quoted, and literal quoted TOML keys without creating duplicate entries.
  • Validate URL hostnames and ports before saving configuration.
  • Preserve existing formatting, comments, backups, and atomic writes.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3f88ba05f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/configure_llm.py Outdated
Comment on lines +93 to +99
hostname = parsed.hostname
parsed.port # Accessing .port validates non-numeric and out-of-range ports.
except ValueError as exc:
raise ConfigWizardError(
f"Base URL must be an absolute http(s) URL with a valid hostname and port, got: {url!r}"
) from exc
if parsed.scheme not in ("http", "https") or not hostname:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate hostname syntax before accepting base URLs

For inputs like https://exa mple.com or https://_bad, urlparse(...).hostname still returns a non-empty string, so this check accepts and saves base URLs that common HTTP/URL clients reject later. Since this path is intended to validate hostnames before writing TOML/OpenCode config, add an explicit hostname/IP validation step rather than treating any non-empty parsed hostname as valid.

Useful? React with 👍 / 👎.

@okcomputer2000

Copy link
Copy Markdown
Contributor Author

Description

This PR fixes three configuration-handling issues in src/configure_llm.py.

The implementation keeps the existing format-preserving TOML editor and changes only the validation and key-matching logic required for these cases.

Modification approach

  • Distinguish an absent TOML file from an existing empty TOML file.

    • The write entry points now check Path.is_file() before reading the file.
    • An existing empty file is passed to the normal TOML update flow and initialized with an [llm] table.
    • A missing file is still rejected to avoid guessing or creating an unrelated project configuration.
  • Recognize all supported TOML key forms.

    • The key matcher now accepts:
      • bare keys, such as name;
      • basic quoted keys, such as "name";
      • literal quoted keys, such as 'name'.
    • Matched key tokens are parsed with tomllib to obtain their semantic key name.
    • The original key spelling, quoting style, indentation, spacing, and trailing comments are preserved.
    • The editor tracks semantic keys, so quoted and unquoted representations update one entry instead of appending a duplicate key.
    • Basic quoted key escapes, such as "na\u006de", are interpreted correctly.
  • Validate URL hostnames before saving configuration.

    • validate_base_url() continues to require an absolute http or https URL.
    • It now checks that parsed.hostname is present and syntactically valid.
    • IPv4 and IPv6 addresses are validated with Python's standard ipaddress module.
    • DNS hostnames are checked by label, including label length, allowed characters, and hyphen placement.
    • IDNA hostnames and portless HTTP(S) URLs remain supported.
  • Validate URL ports at the configuration boundary.

    • The implementation explicitly accesses parsed.port, which makes Python's URL parser reject non-numeric, negative, and out-of-range ports.
    • Those parsing errors are converted into ConfigWizardError before TOML or OpenCode configuration is written.

The current behavior treats http://example.com: as a portless URL because urllib.parse returns None for that port value. If the project decides that an explicitly empty port must be rejected, a small additional check can reject authorities ending in :.

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.

1 participant