Add runtime settings and discovery surfaces#663
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive machine-readable contract for runtime settings and discovery surfaces across the Melix CLI and HTTP gateway. It implements a hierarchical settings store with a defined precedence (CLI flags, environment variables, project-level, and user-level settings) and adds several discovery endpoints, including info, capabilities, instructions, and configuration metadata. The review feedback identifies several areas for improvement: hardening the manual CLI argument parsing logic to prevent incorrect token consumption, using more reliable methods for determining the current working directory, preventing silent truncation during integer type coercion, refining version string detection in pyproject.toml, and utilizing monotonic clocks for accurate latency measurements.
| } else if token.hasPrefix("--") { | ||
| optionArguments.append(token) | ||
| let valueIndex = index + 1 | ||
| guard valueIndex < tail.count else { | ||
| throw MelixCLIError.missingValue(token) | ||
| } | ||
| optionArguments.append(tail[valueIndex]) | ||
| index += 1 |
There was a problem hiding this comment.
This manual flag separation logic is fragile. It assumes that any flag starting with -- (except for the explicitly handled --json) always takes exactly one argument. If an unknown boolean flag is provided, or if a flag is misspelled (e.g., --jsno), this logic will incorrectly consume the next token as its value, which often results in confusing 'missing required KEY/VALUE' errors because a positional argument was stolen. Consider using a more robust argument parsing strategy or explicitly checking against a set of known flags that require values.
| } | ||
|
|
||
| private func projectRootURL() -> URL { | ||
| let rawPath = environment["PWD"]?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" |
There was a problem hiding this comment.
Using the PWD environment variable to determine the current working directory can be unreliable, as it may not be synchronized with the process's actual working directory if it has changed. FileManager.default.currentDirectoryPath is the standard and more reliable source of truth for the process's current directory in Swift.
| let rawPath = environment["PWD"]?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| let rawPath = fileManager.currentDirectoryPath |
| private func coerce(_ rawValue: Any, definition: MelixRuntimeSettingMetadata) throws -> Any { | ||
| switch definition.valueType { | ||
| case "int": | ||
| if let number = rawValue as? NSNumber { | ||
| return NSNumber(value: number.intValue) | ||
| } | ||
| if let value = rawValue as? Int { | ||
| return NSNumber(value: value) | ||
| } | ||
| if let string = rawValue as? String, let parsed = Int(string) { | ||
| return NSNumber(value: parsed) | ||
| } |
There was a problem hiding this comment.
The coercion logic for int types silently truncates floating-point numbers (e.g., 2.9 becomes 2). This can lead to unexpected behavior if a user provides a non-integer value for a setting that strictly expects an integer. It is safer to validate that the number is actually an integer before coercing it.
| private func coerce(_ rawValue: Any, definition: MelixRuntimeSettingMetadata) throws -> Any { | |
| switch definition.valueType { | |
| case "int": | |
| if let number = rawValue as? NSNumber { | |
| return NSNumber(value: number.intValue) | |
| } | |
| if let value = rawValue as? Int { | |
| return NSNumber(value: value) | |
| } | |
| if let string = rawValue as? String, let parsed = Int(string) { | |
| return NSNumber(value: parsed) | |
| } | |
| private func coerce(_ rawValue: Any, definition: MelixRuntimeSettingMetadata) throws -> Any { | |
| switch definition.valueType { | |
| case "int": | |
| if let number = rawValue as? NSNumber { | |
| let doubleValue = number.doubleValue | |
| guard doubleValue == floor(doubleValue) else { | |
| break | |
| } | |
| return NSNumber(value: number.intValue) | |
| } | |
| if let value = rawValue as? Int { | |
| return NSNumber(value: value) | |
| } | |
| if let string = rawValue as? String, let parsed = Int(string) { | |
| return NSNumber(value: parsed) | |
| } |
| if let text = try? String(contentsOf: pyprojectURL, encoding: .utf8) { | ||
| for line in text.split(separator: "\n") { | ||
| let trimmed = line.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard trimmed.hasPrefix("version") else { |
There was a problem hiding this comment.
The check trimmed.hasPrefix("version") is too broad and will match keys like version-control or versioning in pyproject.toml. It should be refined to ensure it only matches the specific version key, typically by checking for a following space or equals sign.
| guard trimmed.hasPrefix("version") else { | |
| guard trimmed.hasPrefix("version ") || trimmed.hasPrefix("version=") else { |
| } | ||
|
|
||
| private func handleDiscoveryWellKnown() async throws -> HTTPResponse { | ||
| let startedAt = Date() |
There was a problem hiding this comment.
Using Date() for latency measurements is discouraged because wall-clock time is not monotonic and can jump due to system clock adjustments (e.g., NTP synchronization). For accurate duration measurements, use ContinuousClock (available in Swift 5.7+) or DispatchTime.
Melix PR Scoped Performance Report
Changed Files
Swift CLI JSON envelope encoding
|
Zigfreidish
left a comment
Zigfreidish
left a comment
There was a problem hiding this comment.
Review: Add runtime settings and discovery surfaces
The overall design is solid — a curated registry with explicit precedence (CLI flag → env → project → user → default), stable schema versions, and thorough test coverage for parsing, mutation, and HTTP discovery. Two correctness issues need to be addressed before merging.
Issue 1 — Hardcoded version in HTTP discovery (RuntimeDiscoveryPayloads.swift)
wellKnownPayload() returns "version": "0.0.0-dev" unconditionally, while the CLI infoPayload() calls installedVersion() which reads pyproject.toml. Consumers of /.well-known/melix.json will always see 0.0.0-dev regardless of the actual installed version. Either expose installedVersion() to HTTPRuntimeDiscoveryPayloads (or pass a version string at construction time), or omit the version key from the HTTP well-known payload if it can't be resolved reliably in that context.
Issue 2 — Dead ternary branches in the runner (MelixCLI.swift)
Several case blocks in the runner have ternary expressions where both branches are identical, so the json flag has no effect on the non-JSON path:
// settingsShow
return options.json ? try prettyJSON(payload) : try prettyJSON(payload)
// info, capabilities, instructions, schema, configMetadata — same patternsettingsSet, settingsValidate, and settingsReset do have distinct branches (human-readable text vs JSON), so the intent is clearly there. The discovery commands need either a human-readable fallback or the guards in the parsers (which already enforce --json for most commands) should be relied upon and the dead branch removed.
Minor observations (non-blocking)
writeSettingsDocumentfirst-run:melixHome.writeAtomically(_:to:)will fail if~/.melix/doesn't exist yet on a fresh install. IfMelixHomeinitialization guarantees the directory exists this is fine — worth confirming.projectRootURL()fallback: Falls back toFileManager.default.currentDirectoryPathwhenPWDis unset. This is standard practice but worth noting that in sandboxed or non-TTY contextscurrentDirectoryPathmay not match the user's working directory._parse_relaxed_object_argumentscomma limitation (also noted in #868): values containing commas will be misparse. Acceptable for a fallback path; a doc comment or test documenting the known limitation would help.
What's good
- Settings precedence logic and source metadata are clean and deterministic.
validate()is non-throwing and aggregates all errors rather than failing fast — good UX for misconfigured environments.- Model alias discovery correctly passes through local paths and full
owner/modelIDs without clobbering them. - HTTP discovery endpoints are correctly added to the
authorizationRoutehealth bypass so they don't require auth. - Test coverage is comprehensive across parser, runner, and HTTP handler layers.
Please fix the two issues above and this is ready to merge.
Generated by Claude Code
Zigfreidish
left a comment
Zigfreidish
left a comment
There was a problem hiding this comment.
The architecture is solid — stable schema versions, clear precedence chain (CLI flag → env → project → user → default), atomic writes, and thorough test coverage (95%+ changed-line coverage on both packages). Two things to address before merge:
-
Dead-code ternaries (
MelixCLI.swift): every new runner case hasoptions.json ? prettyJSON(payload) : prettyJSON(payload)— both branches are the same call. See inline comment. -
Hardcoded version in HTTP well-known payload (
RuntimeDiscoveryPayloads.swift): the gateway always returns"0.0.0-dev"while the CLI correctly readspyproject.toml. Any client that checks the version via/.well-known/melix.jsonwill always see a dev version. See inline comment.
Generated by Claude Code
There was a problem hiding this comment.
Both branches of the ternary are identical — plain-text path is dead code.
Every new discovery/settings runner case does:
return options.json ? try prettyJSON(payload) : try prettyJSON(payload)Both arms call prettyJSON, so the options.json flag has no effect on output format. This is misleading since it implies a human-readable non-JSON path exists. Since the CLI parsers already require --json for most of these commands (throwing a usage error otherwise), the simplest fix is to drop the ternary and call try prettyJSON(payload) unconditionally. For the commands where --json is optional (settingsSet, settingsValidate, settingsReset), consider wiring up a concise text rendering in the else branch, or document that JSON-only output is the intended contract.
Generated by Claude Code
There was a problem hiding this comment.
version is hardcoded as "0.0.0-dev" in the HTTP well-known payload.
"version": "0.0.0-dev",The CLI MelixRuntimeDiscoveryBuilder.infoPayload() reads the real version from pyproject.toml via installedVersion(). The HTTP gateway always reports 0.0.0-dev, making /.well-known/melix.json unreliable for any client that checks the installed version.
MelixRuntimeDiscoveryBuilder already encapsulates this logic; the simplest fix is to expose its infoPayload() from the HTTP layer too (or factor installedVersion() into MelixRuntimeDiscoveryContracts so both callers share it).
Generated by Claude Code
Zigfreidish
force-pushed
the
codex/issue-641-runtime-discovery
branch
from
19a5ddf to
842c40c
Compare
2 participants
Summary
MELIX_HOME/~/.melix, add project settings overrides, and report setting sources and probe timings./.well-known/melix.json,/api/capabilities,/api/instructions, and/api/config-metadata.Closes #641.
Plan or Spec
docs/plans/2026-05-11-runtime-settings-discovery-surfaces.mdCommands Run
Coverage and Metrics
settings_resolve_mssettings_write_mssettings_validate_msdiscovery_build_msoperator.discovery_well_known_latency_msoperator.discovery_capabilities_latency_msoperator.discovery_instructions_latency_msoperator.discovery_config_metadata_latency_msKnown Gaps
make bootstrap,make proto,make py-test, andmake integration-testwere not run; this change is scoped to Swift CLI/control-plane discovery surfaces and does not change protobuf schemas or Python code.Evidence Checklist
N/Ais stated explicitly with the reason.