Feat/cli#29
Merged
Merged
Conversation
…f nesting
The CLI PR bundled several config edits unrelated to (or harmful to) the CLI.
Revert them while preserving the one change that is actually required:
- forge-agent-registry.yaml: fully reverted. It had replaced published package
names (rusticai-vertexai, rusticai-demos, ...) with developer-local wheel
paths under /home/nihal/..., which break for every other user/CI, and added a
MemoryAgent/uniko entry belonging to a separate feature.
- agent-dependencies.yaml:
- Restore provided_type on all resolvers. It is an optional field but a live
join key: api/catalog.go skips entries with empty provided_type, so removing
it silently empties blueprint dependency providers and the
/dependencies/provided-type endpoints.
- Restore path_base: /tmp (had been widened to /, exposing the whole root FS).
- Revert unrelated default/model changes (llm default back to the local qwen
starter; anthropic model ids and prefixes) and drop the stray
asynchronous/uniko additions.
- KEEP the conf: nesting for LiteLLM resolvers. LiteLLMResolver.__init__ only
accepts (model, conf={}); connection settings must live under conf: or the
resolver raises TypeError on instantiation. Added a note documenting this.
- Remove repo-root .python-version (Go CLI scope; Python 3.13 is auto-detected).
GuildSubscription.Close() called close(msgChan)/close(errChan) while the forward() goroutine could still be mid-send on those channels. Because forward's select can pick the source-channel case even after cancel(), it could execute `msgChan <- msg` on a just-closed channel and panic with "send on closed channel". The unguarded sends also meant a stalled consumer (buffer full) left the goroutine blocked forever, leaking it. Make forward() the sole closer of msgChan/errChan via defer, so a close can never race an in-flight send, and guard every send with ctx.Done() so Close() unblocks a stalled forward instead of leaking it. Close() now only cancels the context and closes the underlying subscription, and is idempotent via sync.Once. Adds subscription_test.go with race-tested coverage: concurrent Close during active forwarding (the panic regression), Close unblocking a stalled forward (the leak), and forward exiting when the source closes. The first test panics against the old Close() and passes against the fix.
…splay - message_builder: getIDGen swallowed the NewGemstoneGenerator error and left idGen nil, so any later Generate() would nil-panic. Return the error and propagate it from every Build* helper (all already return error). Also stop discarding the conversation-ID Generate() errors, which could silently produce a zero conversation ID on ErrClockMovedBackwards. - guild_run: msg.Timestamp is milliseconds (GemstoneID uses UnixMilli), but the REPL decoded it with time.Unix(sec, 0), rendering every timestamp ~55000 years in the future. Use time.UnixMilli. - guild_run: UserProxyAgent detection hardcoded "test-user" instead of the configured UserID, so --user-id <other> never matched the name branch. Match config.UserID. - guild_run: --dependency-config pointed at conf/dependencies.yaml, which does not exist; the file is conf/agent-dependencies.yaml. Fix the path. - guild_runtime.buildAgentNameMap assigned spec.Agents[0].Name to every non-manager agent, mislabeling any multi-agent guild, and built an agentsByName map it never used. Match runtime IDs to spec names by name containment and only when the match is unique; drop the dead map.
Address the linters enabled in .golangci-lint.yml (gofmt, gosimple, errcheck,
unparam, ...) for the new guild CLI:
- errcheck: handle previously-ignored errors — log the syscall.Kill/Wait
failures in kill(), report showAgentStatus errors from the /status command,
and make the best-effort blueprint-sniff json.Unmarshal ignores explicit
(_ =) with a comment in guild_run/inspect/validate and LoadGuild.
- unparam: drop the unused spec *protocol.GuildSpec parameter that was threaded
through displayMessages into printMessage but never read.
- gosimple (S1009): drop redundant nil checks before len() on maps in
guild_inspect; convert two no-argument fmt.Printf calls to Print/Println.
- gofmt: fix the misaligned guildBackend/... var block and replace interface{}
with any across the new files.
Docs: move GUILD_CLI_README.md out of the repo root (which has no other .md
docs) into forge-go/initial-docs/guild-cli.md, alongside the existing forge-go
design doc, and strip developer-local paths (/home/nihal/...), stale "this was
fixed" notes, and the incorrect Go 1.21 prerequisite (module targets 1.25).
Note: go.mod still carries unused charmbracelet/bubbletea indirect deps from an
abandoned TUI experiment; `go mod tidy` should be run in a networked
environment to drop them (could not run offline here).
- guild_runtime_test.go: table of LoadGuild cases — direct JSON spec, blueprint wrapper unwrapping (the "spec" nesting), YAML falling through the JSON sniff to the extension parser, missing file, and unsupported extension. - guild_run_test.go: findForgeRoot walking up to the forge-go module, finding it in a forge-go/ subdir from the repo root, ignoring an unrelated module, and returning "" when absent. - guilds/react_guild.json, guilds/uniko_research_guild.json: add the missing trailing newline.
The guild CLI landed with almost no tests (cli 8.3% / command 14.3%), including the message-builder id-generator fix and the buildAgentNameMap fix, both at 0%. Add deterministic unit tests, backed by miniredis + httptest, plus small same-package refactors that make the command helpers testable. Refactor (command/guild_run.go, internal only, no external API change): - Thread an io.Writer through printMessage/showAgentStatus/sendChatMessage/ displayMessages so output can be captured with a bytes.Buffer. - Add unexported guildRuntime and messageSource interfaces so the helpers can run against a fake; *cli.GuildRuntime and *cli.GuildSubscription satisfy them. - Extract selectUserMessageTopic from runGuildREPL as a pure function. cli tests (now 70.7%): all message builders + ParseMessageFromJSON; miniredis coverage of GetAgentStatuses, buildAgentNameMap (the fix: unique-substring match, ambiguous falls back to raw ID), waitForGuildRunning, PublishMessage, Subscribe; httptest coverage of postJSON, createBlueprint, launchFromBlueprint, LaunchGuild, seedAgentRegistry, waitForReady; plus NewGuildRuntime, reserveLocalAddr, Shutdown. Start()/kill() are left to integration (real process). command tests (now 64.0%): printMessage (all skip branches, payload formats, truncation, sender resolution, verbose, routing history), validateGuild (all rules + blueprint + errors), inspectGuild (all sections + blueprint + errors), selectUserMessageTopic, detectPython (fake interpreters on PATH), and the io.Writer/fake-runtime helpers. runGuildREPL's server-driven body stays uncovered (needs a real forge binary); its early error path is exercised.
Contributor
Author
|
@Nihal-Srivastava05 some refactoring and additions to your PR and rebased it to current main |
The Windows CI legs (go-native-build, go-cross-build) failed to compile because cli/guild_runtime.go used the Unix-only syscall.SysProcAttr.Setpgid and syscall.Kill directly. Move the process-group handling behind setProcessGroup/killProcessGroup helpers split across process_unix.go and process_windows.go, so main.go builds on every target platform. Also fix the errcheck failures reported by go-tests: guard the deferred runtime.Shutdown() and route the miniredis seed calls through a seedStatus test helper that fails on error.
The blueprint launch handler merged and schema-validated the configuration
bag but never substituted its values into the guild spec, so mustache
{{ }} placeholders in agent names, properties, and routing steps leaked
verbatim into the launched guild. The Python API server renders at this
point via GuildBuilder._from_spec_dict(...).build_spec().
Add guild.RenderConfiguration, a render-only helper (no defaults/deps/
validation, no-op on empty configuration), and call it in
handleLaunchGuildFromBlueprint right after the spec is decoded.
Add an end-to-end test replicating the rustic-ai blueprint-vars test and
expanding it to assert the rendered agent name, properties, and routing
step (which the Python test never checked), plus per-launch overrides and
ill-typed config rejection. A skipped test documents the shared
mustache/chevron HTML-escaping bug, pending a fix in both renderers.
handleSpawn only populated the spawn response PID inside a block gated on the supervisor reporting the agent as "running", then polled GetPID. A short-lived or instantly-crashing agent could exit before that check: the monitor had already cleared the live PID and flipped state to restarting, so the handler skipped the block and replied Success=true with PID=0. The race made TestHandler_SpawnWithoutGuildStore_UsesFallback (agent is /bin/echo) flaky on slower CI runners while passing locally. Track a LaunchPID on ManagedAgent that records the most recent launch PID and is not cleared on exit, exposed via ProcessSupervisor.GetLaunchPID. handleSpawn now always reports a PID for a successful launch, preferring the live PID and falling back to the launch PID. Add a unit test pinning the invariant that the launch PID survives ClearPID.
The guild CLI previously launched the embedded forge server with an embedded Redis broker while its `--backend` flag (defaulting to "nats") was ignored, and always talked to Redis directly for messaging and agent status. That silently diverged from how rustic-ui launches forge, which uses the NATS data plane. Make the CLI backend-aware and default it to NATS: - Start(): pass `--backend <backend>` through to the server. For NATS, reserve a port and boot the server's embedded NATS via `--embedded-nats-addr`, then connect the CLI's own nats.Conn. Set FORGE_EXTRA_DEPS=rusticai-nats and RUSTIC_AI_NATS_MSG_TTL (30 days) to match the desktop app. An external NATS URL is still honored. - getMessagingBackend(): build and cache a NATS or Redis messaging backend to match the transport the server was launched with. - GetAgentStatuses(): under NATS, enumerate the JetStream "agent-status" KV bucket (filtered by the sanitized guild prefix) since there is no wildcard GET; the Redis KEYS path is unchanged. - Shutdown(): drain/close the messaging backend and NATS connection. The Redis path remains fully supported via `--backend redis`.
Python agents are launched with `uvx --with … python -m …`, but the command passed no interpreter constraint, so uv selected the host's newest Python. On a host whose default is 3.14 (outside forge-python's requires-python of >=3.13,<3.14) uv could not find a pyzmq wheel and fell back to a source build that fails, leaving agents in a restart loop. Let uv act as the version manager instead of inheriting the host default: - registry.ResolveCommand: insert `--python <value>` into the uvx command (RuntimeUVX and default branches). The value comes from FORGE_UV_PYTHON via the new registry.UVPython() helper, mirroring how FORGE_PYTHON_PKG and FORGE_EXTRA_DEPS are already read. Empty preserves prior behaviour (uv chooses). Accepts a uv Python request: "3.13", ">=3.13,<3.14", or a path. - forge server / forge client: add --uv-python, which sets FORGE_UV_PYTHON for the process that spawns agents (incl. the in-process client). - forge guild run: add --uv-python, threaded via RuntimeConfig.UVPython and passed to the embedded server subprocess. With `--uv-python 3.13` the agent env builds on 3.13 (prebuilt pyzmq wheel) regardless of the host default, removing the source-build failure.
…gent
The REPL slept a fixed 2s after launch, then published a UserProxyAgent
creation request and polled GetAgentStatuses for it to appear. But
agent-process liveness ("running" in the status store) is set the moment
the agent process spawns, well before the GuildManagerAgent initializes
the guild (self.guild is not None). Firing the UPA request during that
window makes the manager reject it with "Guild is not initialized", so
the UPA is never created and user messages have nothing to unwrap and
forward — the echo never comes back.
Mirror rustic-ui, which defers creating the UPA / enabling messaging
until the guild reports healthy: add WaitForGuildReady, which polls the
server's HTTP guild endpoint until the store-level guild status reaches
"running" (only set after the manager launched the guild and agents are
healthy), and gate UPA creation on it instead of the fixed sleep. On
timeout it warns and continues, preserving best-effort behaviour.
Verified end-to-end on the echo guild: zero "Guild is not initialized"
errors (was 3), the UserProxyAgent is created on the first try, and the
message round-trips back through the echo agent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.