#autodeploy Rewrite FreeCAD handlers to use persistent bridge architecture

[aleksclark]

Summary

• Rewrite all FreeCAD handler files (documents.go, objects.go, primitives.go, operations.go, analysis.go, exports.go) to use the persistent f.execPython(ctx, script) bridge pattern instead of one-shot jsonScriptResult/scriptResult
• Python scripts now use _result_ = {...} to return data instead of print(json.dumps(...)); documents stay open in the persistent FreeCAD process and are looked up before opening
• Add saveDocument and closeDocument handlers with tool definitions; remove orphan Sketcher/PartDesign dispatch entries and obsolete test helpers

Test plan

• All 32 freecad package tests pass
• Full project test suite passes (go test ./...)
• go build ./... clean
• Zero LSP diagnostics (errors)

🐘 Generated with Crush

[acmacalister]

Good integration overall — clean architecture, solid dispatch parity, and the extractJSON noise-stripping is a clever approach to FreeCAD's chatty console output. Found a few things worth looking at, mostly around the jsonScriptResult dead branch and a zero-value edge case in optFloat.

[acmacalister]

The two branches of the json.Valid check return the same thing — invalid JSON silently gets returned as a {Data: out, IsError: false} result instead of being flagged. The else-branch should either return an error or at minimum set IsError: true so callers know the script didn't produce valid JSON:

if json.Valid([]byte(out)) {
    return &mcp.ToolResult{Data: out}, nil
}
return &mcp.ToolResult{Data: out, IsError: true}, nil

Right now, if FreeCAD produces unexpected non-JSON output (Python traceback, banner noise that extractJSON couldn't strip), the tool returns it as though it succeeded.

[acmacalister]

optFloat treats v == 0 the same as "not provided", so it's impossible to pass a legitimate zero value — e.g. createBox with length=0 or createCone with radius2=0 (a pointed cone, which is a valid and common shape). The TestOptFloat/zero uses default test is actually encoding this bug as correct behavior.

The right fix is to only fall back to the default when the key is absent:

func optFloat(args map[string]any, key string, def float64) float64 {
    v, ok := args[key]
    if !ok || v == nil {
        return def
    }
    f, err := mcp.ArgFloat64(args, key)
    if err != nil {
        return def
    }
    return f
}

Note that radius2 for cones is documented as "default 0 for pointed cone", so this particularly affects that tool.

[acmacalister]

The mutex in runScript serializes every FreeCAD call globally — only one script can run at a time per integration instance. That's probably fine for a single-user CLI setup (FreeCAD's own Python runtime is single-threaded per process), but worth a comment here to explain the design intent. If concurrent usage is ever expected, the right approach would be a worker pool rather than a simple mutex, since FreeCAD can be invoked as separate processes in parallel.

[acmacalister]

filePath joins dataDir with a user-supplied relative path via filepath.Join, but doesn't prevent traversal outside the data directory. A path like ../../etc/passwd would resolve cleanly to a path outside dataDir. Since this ends up inside a Python script passed to FreeCAD, it could read or write arbitrary files.

func (f *freecad) filePath(name string) string {
    if filepath.IsAbs(name) {
        return name
    }
    resolved := filepath.Join(f.dataDir, name)
    // Prevent traversal outside dataDir for relative paths
    if !strings.HasPrefix(resolved, f.dataDir+string(filepath.Separator)) && resolved != f.dataDir {
        return filepath.Join(f.dataDir, filepath.Base(name))
    }
    return resolved
}

If intentional cross-directory access is needed, consider restricting it to absolute paths only (which are already passed through as-is) and rejecting relative paths that contain ...

[acmacalister]

"freecad" is missing from the integration name list here — the test asserts 30 integrations exist but only checks 28 names. Adding it would catch any accidental future breakage of the freecad default config entry:

for _, name := range []string{"github", ..., "fly", "snowflake", "freecad"} {

The expected credentials map in TestDefaultConfig (around line 258) also doesn't include the freecad entry with its binary_path and data_dir keys.

[acmacalister]

Architecture and bridge lifecycle look solid — the switch to a persistent HTTP+XMLRPC bridge is a big improvement over one-shot process spawning. Three things to look at inline.

[acmacalister]

Using exec.CommandContext(ctx, ...) here ties the bridge process lifetime to the calling request's context. When that context is cancelled or times out, Go will automatically kill the FreeCAD process — but the bridge is supposed to be persistent across many requests. The second call to any tool after the first one's context expires will trigger a full restart.

The bridge startup should use context.Background() and reserve ctx only for the TCP ready-wait loop:

cmd := exec.Command(f.binary, "--console") // #nosec G204

[acmacalister]

v == 0 conflates "not provided" with "explicitly zero". For most numeric dims like length and radius this is fine (zero-size shapes are nonsensical anyway), but it breaks for extrude direction components and for createCone's radius2 — a user can't intentionally pass dx=0 or radius2=0 when the default is already 0, and for torus radius2 they can't override the 2 default back to zero. If the field is truly optional, mcp.ArgFloat64 returning an error is the signal for "not provided" — the || v == 0 guard isn't needed and should be removed.

func optFloat(args map[string]any, key string, def float64) float64 {
    v, err := mcp.ArgFloat64(args, key)
    if err != nil {
        return def
    }
    return v
}

[acmacalister]

The count was updated to 30, but freecad (and acp/snowflake) weren't added to the name list in this loop — so the test still only checks 28 of the 30 integrations. The missing ones won't be caught if someone accidentally removes their default config entry.

Add "freecad" (and any other new ones) to both this loop and the expected map in TestDefaultConfig below, so the assertion actually exercises the new entries.

[acmacalister]

Solid work overall — the bridge architecture is clean, dispatch/compaction parity tests are all there, and the test coverage across handlers is thorough. Two things worth a look inline.

[acmacalister]

The HTTP response status code isn't checked before passing body to parseXMLRPCResponse. If the bridge returns a non-200 (e.g., 404 when the addon isn't loaded, or a 500 from a panic), the XML parser will fail on whatever error HTML the server returned, producing a confusing freecad: parse xmlrpc error with no hint that the HTTP layer is the problem.

A quick status check makes the failure mode much clearer:

if resp.StatusCode != http.StatusOK {
    return "", fmt.Errorf("freecad: bridge returned HTTP %d — is FreeCAD running with the RobustMCPBridge addon?", resp.StatusCode)
}

[acmacalister]

TestConfigure_AutoDetect carries a leftover comment from when this adapter launched a freecad binary. In the current bridge architecture, Configure always succeeds when data_dir resolves (it defaults to ~/FreeCAD), so this test never exercises an error path — it's a no-op assertion.

Either rename it to TestConfigure_DefaultsToLocalhost and assert on fc.host and fc.port values, or add a case that forces the MkdirAll path to fail (e.g., by setting data_dir to a path under a read-only directory).

[acmacalister]

Good work on the persistent bridge architecture — the XMLRPC approach is cleaner than one-shot process spawning, and the dispatch/compaction parity tests are solid. Three small things inline.

[acmacalister]

The count was bumped to 32 but "acp" is still absent from this name list — there are only 31 entries, so acp goes unchecked. If its default config entry ever disappears, no test would catch it.

Just add "acp" to the slice:

for _, name := range []string{"github", ..., "fly", "snowflake", "web", "botidentity", "acp", "freecad"} {

[acmacalister]

When json.Unmarshal fails and raw is non-empty (e.g. a Python traceback from an uncaught exception, or raw stdout the bridge injected before the JSON), this returns {Data: raw, IsError: false}. The LLM sees a success result containing an error string, which is confusing.

Setting IsError: true on this path would make the failure visible:

return &mcp.ToolResult{Data: raw, IsError: true}, nil

[acmacalister]

The direction parameter is read and immediately discarded — the Python script that follows always uses the body's default axis regardless of what was passed. The tools.go description says 'H_Axis' or 'V_Axis' (default 'H_Axis'), so an LLM will pass it expecting it to work, then get a silently different result.

Either wire it into the script or drop it from the tool definition until it's actually implemented.

[acmacalister]

Build passes, full test suite green, lint clean. Two existing open issues still need attention (the IsError false-negative in execPython and the direction no-op in linearPattern), plus one new one I noticed on freecad_list_documents — details inline.

[acmacalister]

The description says "List all FreeCAD CAD document files (.FCStd) in the data directory" but the implementation calls FreeCAD.listDocuments() which returns documents that are currently open in memory — not files on disk. An LLM following this description will call this tool expecting to discover existing files and get back an empty list if no documents have been opened yet.

Either scan dataDir for .FCStd files here, or update the description to say something like "List all FreeCAD documents currently open in the running FreeCAD process."

[acmacalister]

When json.Unmarshal fails and raw is non-empty — e.g. a Python traceback or unexpected stdout the bridge injected — this returns {Data: raw, IsError: false}. The tool appears to have succeeded, but the data field contains an error message. Should be IsError: true when we can't parse the response as the expected bridge envelope:

if err := json.Unmarshal([]byte(raw), &resp); err != nil {
    if raw == "" {
        return &mcp.ToolResult{Data: `{"status":"ok"}`}, nil
    }
    return &mcp.ToolResult{Data: raw, IsError: true}, nil
}

[acmacalister]

The direction parameter is still silently ignored — _, _ = mcp.ArgStr(args, "direction") reads and discards it, and the Python script always uses the body's default axis. tools.go advertises 'H_Axis' or 'V_Axis' (default 'H_Axis'), so callers will pass it and get a result that doesn't match what they asked for.

Worth either wiring it into the script (the PartDesign::LinearPattern object has a Direction property you can set via its Origin axis feature) or dropping the parameter from the tool definition until it's implemented.