diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..8f4b0cd --- /dev/null +++ b/.dockerignore @@ -0,0 +1,13 @@ +.git +.github +*.md +!forge-*/ +LICENSE +Makefile +.goreleaser.yaml +install.sh +forge +forge-cli/forge +docs/ +coverage.out +*.test diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 2ae7626..205e862 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -7,6 +7,7 @@ on: permissions: contents: write + packages: write jobs: release: @@ -28,3 +29,52 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} HOMEBREW_TAP_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} + + docker: + name: Docker Image + runs-on: ubuntu-latest + needs: release + steps: + - uses: actions/checkout@v4 + + - name: Docker meta + id: meta + uses: docker/metadata-action@v5 + with: + images: ghcr.io/${{ github.repository }} + tags: | + type=semver,pattern=v{{version}} + type=semver,pattern=v{{major}}.{{minor}} + type=semver,pattern=v{{major}} + type=raw,value=latest + + - uses: docker/setup-qemu-action@v3 + + - uses: docker/setup-buildx-action@v3 + + - name: Login to GHCR + uses: docker/login-action@v3 + with: + registry: ghcr.io + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Extract version info + id: version + run: | + echo "version=${GITHUB_REF_NAME#v}" >> "$GITHUB_OUTPUT" + echo "commit=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT" + + - name: Build and push + uses: docker/build-push-action@v6 + with: + context: . + platforms: linux/amd64,linux/arm64 + push: true + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + build-args: | + VERSION=${{ steps.version.outputs.version }} + COMMIT=${{ steps.version.outputs.commit }} + cache-from: type=gha + cache-to: type=gha,mode=max diff --git a/.gitignore b/.gitignore index 113b199..8815ab0 100644 --- a/.gitignore +++ b/.gitignore @@ -34,6 +34,7 @@ profile.cov # Binary /forge +/forge-cli/forge # Build output .forge-output/ diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 0000000..a13225f --- /dev/null +++ b/Dockerfile @@ -0,0 +1,53 @@ +# ── Build stage ──────────────────────────────────────────────── +FROM --platform=$BUILDPLATFORM golang:1.25-alpine AS builder + +ARG TARGETOS +ARG TARGETARCH +ARG VERSION=dev +ARG COMMIT=none + +WORKDIR /src + +# Copy go.work and all module go.mod/go.sum files first for layer caching +COPY go.work ./ +COPY forge-core/go.mod forge-core/go.sum ./forge-core/ +COPY forge-cli/go.mod forge-cli/go.sum ./forge-cli/ +COPY forge-plugins/go.mod forge-plugins/go.sum ./forge-plugins/ +COPY forge-skills/go.mod forge-skills/go.sum ./forge-skills/ +COPY forge-ui/go.mod forge-ui/go.sum ./forge-ui/ + +# Download dependencies (cached unless go.mod/go.sum change) +RUN --mount=type=cache,target=/go/pkg/mod \ + go work sync && \ + cd forge-core && go mod download && \ + cd ../forge-cli && go mod download && \ + cd ../forge-plugins && go mod download && \ + cd ../forge-skills && go mod download && \ + cd ../forge-ui && go mod download + +# Copy full source +COPY forge-core/ ./forge-core/ +COPY forge-cli/ ./forge-cli/ +COPY forge-plugins/ ./forge-plugins/ +COPY forge-skills/ ./forge-skills/ +COPY forge-ui/ ./forge-ui/ + +# Build static binary for target platform +RUN --mount=type=cache,target=/go/pkg/mod \ + --mount=type=cache,target=/root/.cache/go-build \ + CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} \ + go build -ldflags "-s -w -X main.version=${VERSION} -X main.commit=${COMMIT}" \ + -o /out/forge ./forge-cli/cmd/forge + +# ── Runtime stage ───────────────────────────────────────────── +FROM alpine:3.22.4 + +RUN apk add --no-cache ca-certificates git tzdata && \ + adduser -D -h /home/forge forge + +COPY --from=builder /out/forge /usr/local/bin/forge + +USER forge +WORKDIR /home/forge + +ENTRYPOINT ["forge"] diff --git a/docs/commands.md b/docs/commands.md index 33e019e..32f31da 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -458,7 +458,7 @@ forge key list Manage agent skills. ```bash -# Add a skill from the registry +# Add a skill from the registry (prompts for env vars, merges egress domains) forge skills add # List available skills diff --git a/docs/dashboard.md b/docs/dashboard.md index 5c6ebdc..664558b 100644 --- a/docs/dashboard.md +++ b/docs/dashboard.md @@ -117,6 +117,8 @@ The Skill Builder uses the agent's own LLM provider to power a chat conversation | Script preview | View generated helper scripts alongside the SKILL.md | | Validation | Server-side validation checks name format, required fields, egress domain declarations, and name uniqueness | | One-click save | Save the validated skill directly to the agent's `skills/` directory | +| Egress injection | Automatically merges declared `egress_domains` into `forge.yaml` allowlist on save | +| Env var handling | Writes user-provided env vars to `.env`; prompts for missing required vars after save | ### Workflow @@ -125,7 +127,8 @@ The Skill Builder uses the agent's own LLM provider to power a chat conversation 3. **Iterate** — the AI asks about requirements, security constraints, and env vars 4. **Review** — inspect the generated SKILL.md and scripts in the preview panel 5. **Validate** — check for errors and warnings before saving -6. **Save** — writes `skills/{name}/SKILL.md` and `skills/{name}/scripts/` to the agent directory +6. **Save** — writes `skills/{name}/SKILL.md` and `skills/{name}/scripts/` to the agent directory, merges egress domains into `forge.yaml`, and writes env vars to `.env` +7. **Configure** — if the skill requires env vars that aren't set, the UI shows input fields to provide them and re-save ### Validation Rules @@ -150,7 +153,7 @@ The validator enforces the [SKILL.md format](skills.md): | `GET` | `/api/agents/{id}/skill-builder/context` | Returns the system prompt used for skill generation | | `POST` | `/api/agents/{id}/skill-builder/chat` | Streams an LLM conversation via SSE (accepts `messages` array) | | `POST` | `/api/agents/{id}/skill-builder/validate` | Validates a SKILL.md and optional scripts | -| `POST` | `/api/agents/{id}/skill-builder/save` | Saves a validated skill to `skills/{name}/` | +| `POST` | `/api/agents/{id}/skill-builder/save` | Saves a validated skill to `skills/{name}/`, merges egress domains, writes env vars; returns `SkillSaveResult` with `path`, `egress_added`, `env_configured`, `env_missing` | ## Architecture diff --git a/docs/deployment.md b/docs/deployment.md index 7509497..d7f5741 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -4,7 +4,28 @@ Forge agents can be packaged as container images and deployed to Docker, Kubernetes, or air-gapped environments. -## Building Container Images +## Pre-built Docker Image + +Forge publishes multi-architecture Docker images (linux/amd64, linux/arm64) to GitHub Container Registry on every release: + +```bash +# Pull the latest release +docker pull ghcr.io/initializ/forge:latest + +# Pin to a specific version +docker pull ghcr.io/initializ/forge:v1.2.3 + +# Run with your agent directory mounted +docker run -v /path/to/agent:/home/forge/agent -w /home/forge/agent \ + -e OPENAI_API_KEY=sk-... \ + ghcr.io/initializ/forge:latest run --host 0.0.0.0 +``` + +Tags follow the pattern `v1.2.3`, `v1.2`, `v1`, and `latest`. + +The image is built from a multi-stage Dockerfile in the repository root — `golang:1.25-alpine` for the build stage (static binary, `CGO_ENABLED=0`) and `alpine:3.21` for the runtime with `ca-certificates`, `git`, and `tzdata`. The container runs as a non-root `forge` user. + +## Building Agent Container Images ```bash # Build a container image (auto-detects Docker/Podman/Buildah) diff --git a/docs/runtime.md b/docs/runtime.md index bc0f4e2..7c0d4dd 100644 --- a/docs/runtime.md +++ b/docs/runtime.md @@ -19,7 +19,11 @@ The core agent loop follows a simple pattern: User message → Memory → LLM → tool_calls? → Execute tools → LLM → ... → text → Done ``` -The loop terminates when `len(ToolCalls) == 0`. Tool calls are always executed even if `FinishReason` is `"stop"` — this prevents orphaned function calls that would cause API rejection on session recovery. +The loop terminates when `len(ToolCalls) == 0`. `FinishReason` is intentionally ignored — some providers return `"stop"` even when tool calls are present. Only the tool call list determines whether execution continues. + +### Session Recovery Deduplication + +When a session is recovered from disk (e.g., after a premature loop exit), the executor checks whether the recovered conversation already ends with an identical user message. If so, the duplicate is skipped to prevent the same message from appearing twice in the context window. This handles the common case where a user retries the same prompt after a crash or timeout. ### Q&A Nudge Suppression diff --git a/docs/skills.md b/docs/skills.md index f82f601..a39e26d 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -97,7 +97,7 @@ forge skills validate forge skills audit --embedded ``` -`forge skills add` copies the skill's SKILL.md and any associated scripts into your project's `skills/` directory. It validates binary and environment requirements, checks for existing values in your environment, `.env` file, and encrypted secrets, and prompts only for truly missing values with a suggestion to use `forge secrets set` for sensitive keys. +`forge skills add` copies the skill's SKILL.md and any associated scripts into your project's `skills/` directory. It validates binary and environment requirements, checks for existing values in your environment, `.env` file, and encrypted secrets, and prompts only for truly missing values with a suggestion to use `forge secrets set` for sensitive keys. If the skill declares `egress_domains`, they are automatically merged into the `forge.yaml` `egress.allowed_domains` list (deduplicated and sorted). ## Skills as First-Class Tools @@ -516,7 +516,11 @@ forge build ## Skill Builder (Web UI) -The [Web Dashboard](dashboard.md#skill-builder) includes an AI-powered Skill Builder that generates valid SKILL.md files and helper scripts through a conversational interface. It uses the agent's own LLM provider and includes server-side validation before saving to the agent's `skills/` directory. +The [Web Dashboard](dashboard.md#skill-builder) includes an AI-powered Skill Builder that generates valid SKILL.md files and helper scripts through a conversational interface. It uses the agent's own LLM provider and includes server-side validation before saving to the agent's `skills/` directory. On save, the builder automatically parses the skill's requirements and: + +- **Merges egress domains** into `forge.yaml` `egress.allowed_domains` (deduplicated) +- **Writes user-provided env vars** to `.env` (skipping keys already present) +- **Reports missing env vars** so the user can provide values and re-save --- ← [Architecture](architecture.md) | [Back to README](../README.md) | [Tools](tools.md) → diff --git a/forge-cli/cmd/skill_env.go b/forge-cli/cmd/skill_env.go new file mode 100644 index 0000000..05ed160 --- /dev/null +++ b/forge-cli/cmd/skill_env.go @@ -0,0 +1,294 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/initializ/forge/forge-cli/runtime" + "github.com/initializ/forge/forge-skills/contract" + "github.com/initializ/forge/forge-skills/parser" + forgeui "github.com/initializ/forge/forge-ui" +) + +// SkillRequirementsInfo holds parsed requirements from a SKILL.md. +type SkillRequirementsInfo struct { + EnvReqs *contract.EnvRequirements + EgressDomains []string +} + +// ParseSkillRequirements parses a SKILL.md string and extracts env requirements +// and egress domains from its frontmatter metadata. +func ParseSkillRequirements(skillMD string) *SkillRequirementsInfo { + entries, meta, err := parser.ParseWithMetadata(strings.NewReader(skillMD)) + if err != nil || meta == nil { + return &SkillRequirementsInfo{} + } + + reqs, egressDomains, _ := parser.ExtractForgeReqs(meta) + + info := &SkillRequirementsInfo{ + EgressDomains: egressDomains, + } + + if reqs != nil && reqs.Env != nil { + info.EnvReqs = reqs.Env + } + + _ = entries // entries not needed here + return info +} + +// MergeEgressDomains reads forge.yaml, adds new domains to the egress +// allowed_domains list (dedup + sort), and writes back. Returns the list +// of newly added domains. +func MergeEgressDomains(agentDir string, domains []string) ([]string, error) { + if len(domains) == 0 { + return nil, nil + } + + yamlPath := filepath.Join(agentDir, "forge.yaml") + data, err := os.ReadFile(yamlPath) + if err != nil { + return nil, fmt.Errorf("reading forge.yaml: %w", err) + } + + content := string(data) + lines := strings.Split(content, "\n") + + // Find existing allowed_domains and collect them + existing := make(map[string]bool) + allowedIdx := -1 // line index of "allowed_domains:" + lastDomainIdx := -1 // line index of last " - domain" entry + indent := "" + + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "allowed_domains:") { + allowedIdx = i + // Detect indentation of the line containing allowed_domains + indent = line[:len(line)-len(strings.TrimLeft(line, " \t"))] + continue + } + if allowedIdx >= 0 && i > allowedIdx { + // We're inside the allowed_domains block + if strings.HasPrefix(trimmed, "- ") { + domain := strings.TrimSpace(strings.TrimPrefix(trimmed, "- ")) + existing[domain] = true + lastDomainIdx = i + } else if trimmed == "" { + // empty line, continue scanning + continue + } else { + // Hit a non-list line → end of allowed_domains block + break + } + } + } + + // Determine which domains are new + var added []string + for _, d := range domains { + if !existing[d] { + added = append(added, d) + } + } + + if len(added) == 0 { + return nil, nil + } + sort.Strings(added) + + if allowedIdx >= 0 { + // Insert new domains after the last existing domain entry + insertIdx := lastDomainIdx + 1 + if lastDomainIdx < allowedIdx { + // allowed_domains: exists but has no entries yet + insertIdx = allowedIdx + 1 + } + // Detect the list item indentation from existing entries + itemIndent := indent + " " + if lastDomainIdx > allowedIdx { + existingLine := lines[lastDomainIdx] + itemIndent = existingLine[:len(existingLine)-len(strings.TrimLeft(existingLine, " \t"))] + } + + var newLines []string + for _, d := range added { + newLines = append(newLines, itemIndent+"- "+d) + } + + // Splice into the lines array + result := make([]string, 0, len(lines)+len(newLines)) + result = append(result, lines[:insertIdx]...) + result = append(result, newLines...) + result = append(result, lines[insertIdx:]...) + lines = result + } else { + // No egress section with allowed_domains found — append one. + // Check if there's an "egress:" section + egressIdx := -1 + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == "egress:" || strings.HasPrefix(trimmed, "egress:") { + egressIdx = i + break + } + } + + if egressIdx >= 0 { + // Find where to insert allowed_domains under egress + insertIdx := egressIdx + 1 + // Skip existing egress sub-keys + egressIndent := lines[egressIdx][:len(lines[egressIdx])-len(strings.TrimLeft(lines[egressIdx], " \t"))] + subIndent := egressIndent + " " + for insertIdx < len(lines) { + trimmed := strings.TrimSpace(lines[insertIdx]) + if trimmed == "" { + insertIdx++ + continue + } + lineIndent := lines[insertIdx][:len(lines[insertIdx])-len(strings.TrimLeft(lines[insertIdx], " \t"))] + if len(lineIndent) <= len(egressIndent) && trimmed != "" { + break + } + insertIdx++ + } + var newLines []string + newLines = append(newLines, subIndent+"allowed_domains:") + for _, d := range added { + newLines = append(newLines, subIndent+" - "+d) + } + + result := make([]string, 0, len(lines)+len(newLines)) + result = append(result, lines[:insertIdx]...) + result = append(result, newLines...) + result = append(result, lines[insertIdx:]...) + lines = result + } else { + // No egress section at all — append both egress and allowed_domains + lines = append(lines, "egress:") + lines = append(lines, " allowed_domains:") + for _, d := range added { + lines = append(lines, " - "+d) + } + } + } + + if err := os.WriteFile(yamlPath, []byte(strings.Join(lines, "\n")), 0o644); err != nil { + return nil, fmt.Errorf("writing forge.yaml: %w", err) + } + + return added, nil +} + +// AppendEnvVars appends key=value pairs to .env, skipping keys already present. +// Returns the list of keys that were actually written. +func AppendEnvVars(agentDir string, vars map[string]string, skillName string) ([]string, error) { + if len(vars) == 0 { + return nil, nil + } + + envPath := filepath.Join(agentDir, ".env") + existing, _ := runtime.LoadEnvFile(envPath) + + // Also check for encrypted secret placeholders + secretKeys := loadSecretPlaceholders(envPath) + + var toWrite []string + for k := range vars { + if existing[k] != "" || secretKeys[k] { + continue + } + toWrite = append(toWrite, k) + } + + if len(toWrite) == 0 { + return nil, nil + } + sort.Strings(toWrite) + + f, err := os.OpenFile(envPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return nil, fmt.Errorf("opening .env: %w", err) + } + defer func() { _ = f.Close() }() + + // Check if we need a leading newline + if info, _ := f.Stat(); info != nil && info.Size() > 0 { + // Read last byte to check if file ends with newline + rf, _ := os.Open(envPath) + if rf != nil { + buf := make([]byte, 1) + if n, _ := rf.ReadAt(buf, info.Size()-1); n == 1 && buf[0] != '\n' { + _, _ = fmt.Fprintln(f) + } + _ = rf.Close() + } + } + + _, _ = fmt.Fprintf(f, "# Required by %s skill\n", skillName) + var written []string + for _, k := range toWrite { + _, _ = fmt.Fprintf(f, "%s=%s\n", k, vars[k]) + written = append(written, k) + } + + return written, nil +} + +// CheckMissingEnv checks OS env + .env + secret placeholders and returns +// entries for env vars that are still missing. +func CheckMissingEnv(agentDir string, envReqs *contract.EnvRequirements) []forgeui.SkillEnvEntry { + if envReqs == nil { + return nil + } + + envPath := filepath.Join(agentDir, ".env") + dotEnv, _ := runtime.LoadEnvFile(envPath) + secretKeys := loadSecretPlaceholders(envPath) + + isSet := func(key string) bool { + if os.Getenv(key) != "" { + return true + } + if dotEnv[key] != "" { + return true + } + return secretKeys[key] + } + + var missing []forgeui.SkillEnvEntry + + for _, env := range envReqs.Required { + if !isSet(env) { + missing = append(missing, forgeui.SkillEnvEntry{Name: env, Kind: "required"}) + } + } + + // For one_of groups, check if at least one is set + if len(envReqs.OneOf) > 0 { + hasOne := false + for _, env := range envReqs.OneOf { + if isSet(env) { + hasOne = true + break + } + } + if !hasOne { + for _, env := range envReqs.OneOf { + missing = append(missing, forgeui.SkillEnvEntry{Name: env, Kind: "one_of"}) + } + } + } + + for _, env := range envReqs.Optional { + if !isSet(env) { + missing = append(missing, forgeui.SkillEnvEntry{Name: env, Kind: "optional"}) + } + } + + return missing +} diff --git a/forge-cli/cmd/skill_env_test.go b/forge-cli/cmd/skill_env_test.go new file mode 100644 index 0000000..2ea043f --- /dev/null +++ b/forge-cli/cmd/skill_env_test.go @@ -0,0 +1,370 @@ +package cmd + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/initializ/forge/forge-skills/contract" + forgeui "github.com/initializ/forge/forge-ui" +) + +func TestParseSkillRequirements(t *testing.T) { + skillMD := `--- +name: test-skill +description: A test skill +metadata: + forge: + requires: + env: + required: + - MY_API_KEY + optional: + - MY_DEBUG + egress_domains: + - api.example.com + - cdn.example.com +--- + +# Test Skill + +## Tool: test_tool + +A test tool. +` + info := ParseSkillRequirements(skillMD) + + if info.EnvReqs == nil { + t.Fatal("expected EnvReqs to be non-nil") + } + if len(info.EnvReqs.Required) != 1 || info.EnvReqs.Required[0] != "MY_API_KEY" { + t.Errorf("Required = %v, want [MY_API_KEY]", info.EnvReqs.Required) + } + if len(info.EnvReqs.Optional) != 1 || info.EnvReqs.Optional[0] != "MY_DEBUG" { + t.Errorf("Optional = %v, want [MY_DEBUG]", info.EnvReqs.Optional) + } + if len(info.EgressDomains) != 2 { + t.Errorf("EgressDomains = %v, want 2 entries", info.EgressDomains) + } +} + +func TestParseSkillRequirementsNoMetadata(t *testing.T) { + skillMD := `--- +name: simple-skill +description: No forge metadata +--- + +## Tool: simple + +A simple tool. +` + info := ParseSkillRequirements(skillMD) + if info.EnvReqs != nil { + t.Errorf("expected nil EnvReqs for skill without forge metadata, got %v", info.EnvReqs) + } + if len(info.EgressDomains) != 0 { + t.Errorf("expected no egress domains, got %v", info.EgressDomains) + } +} + +func TestMergeEgressDomains(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "forge.yaml") + + initial := `agent_id: test-agent +version: 0.1.0 +model: + provider: openai + name: gpt-4o +egress: + profile: standard + mode: allowlist + allowed_domains: + - api.openai.com + - api.tavily.com +` + if err := os.WriteFile(yamlPath, []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + + added, err := MergeEgressDomains(dir, []string{"api.example.com", "api.openai.com", "cdn.example.com"}) + if err != nil { + t.Fatal(err) + } + + // api.openai.com already exists, so only 2 should be added + if len(added) != 2 { + t.Fatalf("added = %v, want 2 new domains", added) + } + if added[0] != "api.example.com" || added[1] != "cdn.example.com" { + t.Errorf("added = %v, want [api.example.com cdn.example.com]", added) + } + + // Verify the file content + data, err := os.ReadFile(yamlPath) + if err != nil { + t.Fatal(err) + } + content := string(data) + if !strings.Contains(content, "- api.example.com") { + t.Error("file should contain api.example.com") + } + if !strings.Contains(content, "- cdn.example.com") { + t.Error("file should contain cdn.example.com") + } + // Original domains should still be there + if !strings.Contains(content, "- api.openai.com") { + t.Error("file should still contain api.openai.com") + } +} + +func TestMergeEgressDomainsNoEgressSection(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "forge.yaml") + + initial := `agent_id: test-agent +version: 0.1.0 +model: + provider: openai + name: gpt-4o +` + if err := os.WriteFile(yamlPath, []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + + added, err := MergeEgressDomains(dir, []string{"api.example.com"}) + if err != nil { + t.Fatal(err) + } + + if len(added) != 1 { + t.Fatalf("added = %v, want 1", added) + } + + data, err := os.ReadFile(yamlPath) + if err != nil { + t.Fatal(err) + } + content := string(data) + if !strings.Contains(content, "egress:") { + t.Error("file should contain egress section") + } + if !strings.Contains(content, "allowed_domains:") { + t.Error("file should contain allowed_domains") + } + if !strings.Contains(content, "- api.example.com") { + t.Error("file should contain api.example.com") + } +} + +func TestMergeEgressDomainsAllExist(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "forge.yaml") + + initial := `egress: + allowed_domains: + - api.openai.com +` + if err := os.WriteFile(yamlPath, []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + + added, err := MergeEgressDomains(dir, []string{"api.openai.com"}) + if err != nil { + t.Fatal(err) + } + if added != nil { + t.Errorf("expected nil for no new domains, got %v", added) + } +} + +func TestMergeEgressDomainsEmpty(t *testing.T) { + dir := t.TempDir() + added, err := MergeEgressDomains(dir, nil) + if err != nil { + t.Fatal(err) + } + if added != nil { + t.Errorf("expected nil, got %v", added) + } +} + +func TestAppendEnvVars(t *testing.T) { + dir := t.TempDir() + envPath := filepath.Join(dir, ".env") + + // Create initial .env + if err := os.WriteFile(envPath, []byte("EXISTING_KEY=value\n"), 0o644); err != nil { + t.Fatal(err) + } + + written, err := AppendEnvVars(dir, map[string]string{ + "NEW_KEY": "new_value", + "EXISTING_KEY": "should_skip", + "ANOTHER_KEY": "another_value", + }, "test-skill") + if err != nil { + t.Fatal(err) + } + + if len(written) != 2 { + t.Fatalf("written = %v, want 2 keys", written) + } + + // Verify file content + data, err := os.ReadFile(envPath) + if err != nil { + t.Fatal(err) + } + content := string(data) + if !strings.Contains(content, "ANOTHER_KEY=another_value") { + t.Error("file should contain ANOTHER_KEY=another_value") + } + if !strings.Contains(content, "NEW_KEY=new_value") { + t.Error("file should contain NEW_KEY=new_value") + } + if !strings.Contains(content, "# Required by test-skill skill") { + t.Error("file should contain comment with skill name") + } + // Existing key should not be duplicated + if strings.Count(content, "EXISTING_KEY") != 1 { + t.Error("EXISTING_KEY should appear exactly once") + } +} + +func TestAppendEnvVarsNoFile(t *testing.T) { + dir := t.TempDir() + + written, err := AppendEnvVars(dir, map[string]string{ + "NEW_KEY": "value", + }, "test-skill") + if err != nil { + t.Fatal(err) + } + + if len(written) != 1 { + t.Fatalf("written = %v, want 1", written) + } + + data, err := os.ReadFile(filepath.Join(dir, ".env")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "NEW_KEY=value") { + t.Error("file should contain NEW_KEY=value") + } +} + +func TestAppendEnvVarsEmpty(t *testing.T) { + dir := t.TempDir() + written, err := AppendEnvVars(dir, nil, "test") + if err != nil { + t.Fatal(err) + } + if written != nil { + t.Errorf("expected nil, got %v", written) + } +} + +func TestCheckMissingEnv(t *testing.T) { + dir := t.TempDir() + envPath := filepath.Join(dir, ".env") + if err := os.WriteFile(envPath, []byte("PRESENT_KEY=value\n"), 0o644); err != nil { + t.Fatal(err) + } + + // Set an OS env var for testing + t.Setenv("OS_KEY", "os_value") + + reqs := &contract.EnvRequirements{ + Required: []string{"PRESENT_KEY", "MISSING_KEY", "OS_KEY"}, + OneOf: []string{"ONE_A", "ONE_B"}, + Optional: []string{"OPT_MISSING"}, + } + + missing := CheckMissingEnv(dir, reqs) + + // PRESENT_KEY is in .env → not missing + // OS_KEY is in OS env → not missing + // MISSING_KEY → missing (required) + // ONE_A, ONE_B → both missing (one_of group, neither set) + // OPT_MISSING → missing (optional) + + expected := []forgeui.SkillEnvEntry{ + {Name: "MISSING_KEY", Kind: "required"}, + {Name: "ONE_A", Kind: "one_of"}, + {Name: "ONE_B", Kind: "one_of"}, + {Name: "OPT_MISSING", Kind: "optional"}, + } + + if len(missing) != len(expected) { + t.Fatalf("missing = %v, want %v", missing, expected) + } + + for i, got := range missing { + if got.Name != expected[i].Name || got.Kind != expected[i].Kind { + t.Errorf("missing[%d] = %v, want %v", i, got, expected[i]) + } + } +} + +func TestCheckMissingEnvNil(t *testing.T) { + dir := t.TempDir() + missing := CheckMissingEnv(dir, nil) + if missing != nil { + t.Errorf("expected nil for nil reqs, got %v", missing) + } +} + +func TestCheckMissingEnvOneOfSatisfied(t *testing.T) { + dir := t.TempDir() + envPath := filepath.Join(dir, ".env") + if err := os.WriteFile(envPath, []byte("ONE_B=value\n"), 0o644); err != nil { + t.Fatal(err) + } + + reqs := &contract.EnvRequirements{ + OneOf: []string{"ONE_A", "ONE_B"}, + } + + missing := CheckMissingEnv(dir, reqs) + if len(missing) != 0 { + t.Errorf("expected no missing when one_of is satisfied, got %v", missing) + } +} + +func TestMergeEgressDomainsWithEgressNoAllowedDomains(t *testing.T) { + dir := t.TempDir() + yamlPath := filepath.Join(dir, "forge.yaml") + + initial := `agent_id: test-agent +egress: + profile: standard + mode: allowlist +` + if err := os.WriteFile(yamlPath, []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + + added, err := MergeEgressDomains(dir, []string{"api.example.com"}) + if err != nil { + t.Fatal(err) + } + + if len(added) != 1 || added[0] != "api.example.com" { + t.Fatalf("added = %v, want [api.example.com]", added) + } + + data, err := os.ReadFile(yamlPath) + if err != nil { + t.Fatal(err) + } + content := string(data) + if !strings.Contains(content, "allowed_domains:") { + t.Error("file should contain allowed_domains") + } + if !strings.Contains(content, "- api.example.com") { + t.Error("file should contain api.example.com") + } +} diff --git a/forge-cli/cmd/skills.go b/forge-cli/cmd/skills.go index 3709a53..f594773 100644 --- a/forge-cli/cmd/skills.go +++ b/forge-cli/cmd/skills.go @@ -218,6 +218,19 @@ func runSkillsAdd(cmd *cobra.Command, args []string) error { } } + // Merge egress domains into forge.yaml + if len(info.EgressDomains) > 0 { + added, mergeErr := MergeEgressDomains(wd, info.EgressDomains) + if mergeErr != nil { + fmt.Printf("\n Warning: could not update egress domains: %s\n", mergeErr) + } else if len(added) > 0 { + fmt.Println("\n Egress domains added to forge.yaml:") + for _, d := range added { + fmt.Printf(" + %s\n", d) + } + } + } + fmt.Printf("\nSkill %q added successfully.\n", info.DisplayName) return nil } diff --git a/forge-cli/cmd/ui.go b/forge-cli/cmd/ui.go index f3b5f6b..a77aed5 100644 --- a/forge-cli/cmd/ui.go +++ b/forge-cli/cmd/ui.go @@ -13,6 +13,7 @@ import ( "github.com/initializ/forge/forge-cli/internal/tui" "github.com/initializ/forge/forge-cli/runtime" "github.com/initializ/forge/forge-core/llm" + "github.com/initializ/forge/forge-core/llm/oauth" "github.com/initializ/forge/forge-core/llm/providers" coreruntime "github.com/initializ/forge/forge-core/runtime" "github.com/initializ/forge/forge-core/util" @@ -141,33 +142,41 @@ func runUI(cmd *cobra.Command, args []string) error { return fmt.Errorf("loading config: %w", err) } - // Load .env + // Load .env — force-set values so __oauth__ sentinels from prior + // handler calls don't block real keys from encrypted secrets. envPath := filepath.Join(opts.AgentDir, ".env") envVars, err := runtime.LoadEnvFile(envPath) if err != nil { return fmt.Errorf("loading env: %w", err) } for k, v := range envVars { - if os.Getenv(k) == "" { - _ = os.Setenv(k, v) + _ = os.Setenv(k, v) + } + + // Clear __oauth__ sentinels so OverlaySecretsToEnv can replace them + // with real keys from the encrypted secrets store. + for _, key := range []string{"OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GEMINI_API_KEY"} { + if os.Getenv(key) == "__oauth__" { + _ = os.Unsetenv(key) } } // Overlay encrypted secrets runtime.OverlaySecretsToEnv(cfg, opts.AgentDir) - // Build env map for model resolution + // Build env map for model resolution. OS env takes priority over .env + // file because OverlaySecretsToEnv may have replaced sentinels with + // real keys from the encrypted store. envMap := make(map[string]string) for k, v := range envVars { - envMap[k] = v + if v != "__oauth__" { + envMap[k] = v + } } - // Include OS env vars that may have been set by overlay for _, kv := range os.Environ() { parts := strings.SplitN(kv, "=", 2) if len(parts) == 2 { - if _, exists := envMap[parts[0]]; !exists { - envMap[parts[0]] = parts[1] - } + envMap[parts[0]] = parts[1] } } @@ -176,11 +185,39 @@ func runUI(cmd *cobra.Command, args []string) error { return fmt.Errorf("unable to resolve model configuration") } - mc.Client.Model = forgeui.SkillBuilderCodegenModel(mc.Provider, mc.Client.Model) - - client, err := providers.NewClient(mc.Provider, mc.Client) - if err != nil { - return fmt.Errorf("creating LLM client: %w", err) + // Resolve OAuth credentials when the API key is the __oauth__ sentinel + // or empty. OAuth/Codex backend has its own model constraints, so + // skip the codegen model upgrade for OAuth clients. + var client llm.Client + needsOAuth := mc.Provider == "openai" && (mc.Client.APIKey == "" || mc.Client.APIKey == "__oauth__") + if needsOAuth { + token, oauthErr := oauth.LoadCredentials(mc.Provider) + if oauthErr == nil && token != nil && token.RefreshToken != "" { + oauthCfg := oauth.OpenAIConfig() + baseURL := token.BaseURL + if baseURL == "" { + baseURL = oauthCfg.BaseURL + } + mc.Client.APIKey = token.AccessToken + mc.Client.BaseURL = baseURL + client = providers.NewOAuthClient(mc.Client, mc.Provider, oauthCfg) + } else if mc.Client.APIKey == "" || mc.Client.APIKey == "__oauth__" { + // No API key and OAuth failed — surface the error instead + // of silently falling through to a client with no auth. + if oauthErr != nil { + return fmt.Errorf("loading OAuth credentials: %w", oauthErr) + } + return fmt.Errorf("no OpenAI API key or OAuth credentials found; run 'forge init' with OAuth or set OPENAI_API_KEY") + } + } + if client == nil { + // Only upgrade to the codegen model for non-OAuth (API key) clients. + mc.Client.Model = forgeui.SkillBuilderCodegenModel(mc.Provider, mc.Client.Model) + var clientErr error + client, clientErr = providers.NewClient(mc.Provider, mc.Client) + if clientErr != nil { + return fmt.Errorf("creating LLM client: %w", clientErr) + } } // Build chat request with system prompt + conversation messages @@ -223,31 +260,55 @@ func runUI(cmd *cobra.Command, args []string) error { } // Build the SkillSaveFunc for saving generated skills. - skillSaveFunc := func(opts forgeui.SkillSaveOptions) error { + skillSaveFunc := func(opts forgeui.SkillSaveOptions) (*forgeui.SkillSaveResult, error) { skillDir := filepath.Join(opts.AgentDir, "skills", opts.SkillName) if err := os.MkdirAll(skillDir, 0o755); err != nil { - return fmt.Errorf("creating skill directory: %w", err) + return nil, fmt.Errorf("creating skill directory: %w", err) } skillPath := filepath.Join(skillDir, "SKILL.md") if err := os.WriteFile(skillPath, []byte(opts.SkillMD), 0o644); err != nil { - return fmt.Errorf("writing SKILL.md: %w", err) + return nil, fmt.Errorf("writing SKILL.md: %w", err) } if len(opts.Scripts) > 0 { scriptsDir := filepath.Join(skillDir, "scripts") if err := os.MkdirAll(scriptsDir, 0o755); err != nil { - return fmt.Errorf("creating scripts directory: %w", err) + return nil, fmt.Errorf("creating scripts directory: %w", err) } for filename, content := range opts.Scripts { scriptPath := filepath.Join(scriptsDir, filename) if err := os.WriteFile(scriptPath, []byte(content), 0o755); err != nil { - return fmt.Errorf("writing script %s: %w", filename, err) + return nil, fmt.Errorf("writing script %s: %w", filename, err) } } } - return nil + result := &forgeui.SkillSaveResult{ + Path: "skills/" + opts.SkillName + "/SKILL.md", + } + + // Parse skill requirements from SKILL.md + reqInfo := ParseSkillRequirements(opts.SkillMD) + + // Write user-provided env vars to .env + if len(opts.EnvVars) > 0 { + written, _ := AppendEnvVars(opts.AgentDir, opts.EnvVars, opts.SkillName) + result.EnvConfigured = written + } + + // Merge egress domains into forge.yaml + if len(reqInfo.EgressDomains) > 0 { + added, _ := MergeEgressDomains(opts.AgentDir, reqInfo.EgressDomains) + result.EgressAdded = added + } + + // Check for missing env vars + if reqInfo.EnvReqs != nil { + result.EnvMissing = CheckMissingEnv(opts.AgentDir, reqInfo.EnvReqs) + } + + return result, nil } server := forgeui.NewUIServer(forgeui.UIServerConfig{ diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 7ec6a67..827f38c 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -1603,7 +1603,8 @@ func (r *Runner) createProviderClient(provider string, cfg llm.ClientConfig) (ll // Check for stored OAuth credentials — but only if no real API key is // configured. The "__oauth__" sentinel means the user chose OAuth auth // during init, so we should load the actual token from the credential store. - if provider == "openai" && (cfg.APIKey == "" || cfg.APIKey == "__oauth__") { + needsOAuth := provider == "openai" && (cfg.APIKey == "" || cfg.APIKey == "__oauth__") + if needsOAuth { token, err := oauth.LoadCredentials(provider) if err == nil && token != nil && token.RefreshToken != "" { oauthCfg := oauth.OpenAIConfig() @@ -1620,6 +1621,14 @@ func (r *Runner) createProviderClient(provider string, cfg llm.ClientConfig) (ll cfg.BaseURL = baseURL return providers.NewOAuthClient(cfg, provider, oauthCfg), nil } + // No API key and OAuth failed — surface the error instead of + // creating a client with no auth that will fail with 401. + if cfg.APIKey == "" || cfg.APIKey == "__oauth__" { + if err != nil { + return nil, fmt.Errorf("loading OAuth credentials: %w", err) + } + return nil, fmt.Errorf("no OpenAI API key or OAuth credentials found; run 'forge init' with OAuth or set OPENAI_API_KEY") + } } return providers.NewClient(provider, cfg) diff --git a/forge-core/runtime/loop.go b/forge-core/runtime/loop.go index 63a5f15..e92fd50 100644 --- a/forge-core/runtime/loop.go +++ b/forge-core/runtime/loop.go @@ -157,8 +157,26 @@ func (e *LLMExecutor) Execute(ctx context.Context, task *a2a.Task, msg *a2a.Mess } } - // Append the new user message - mem.Append(a2aMessageToLLM(*msg)) + // Append the new user message, but skip if the recovered session + // already ends with an identical user message (avoids duplicates + // when users retry after a premature loop exit). + newMsg := a2aMessageToLLM(*msg) + if recovered { + msgs := mem.Messages() + // Find the last user message in the recovered session. + lastUserIdx := -1 + for j := len(msgs) - 1; j >= 0; j-- { + if msgs[j].Role == llm.RoleUser { + lastUserIdx = j + break + } + } + if lastUserIdx < 0 || msgs[lastUserIdx].Content != newMsg.Content { + mem.Append(newMsg) + } + } else { + mem.Append(newMsg) + } // Build tool definitions var toolDefs []llm.ToolDefinition @@ -248,10 +266,10 @@ func (e *LLMExecutor) Execute(ctx context.Context, task *a2a.Task, msg *a2a.Mess // Append assistant message to memory mem.Append(resp.Message) - // Check if we're done (no tool calls). - // Always execute tool calls even when finish_reason is "stop" — - // otherwise we persist an assistant message with orphaned function - // calls that the Responses API will reject on session recovery. + // Check if we're done: the definitive signal is the absence of tool + // calls. FinishReason is unreliable — some providers return "stop" + // even when tool calls are present, and others return empty/non-standard + // values. Only the tool call list determines whether execution continues. if len(resp.Message.ToolCalls) == 0 { // If the LLM stopped after executing tools, send a continuation // nudge. This catches cases where the LLM reports findings instead diff --git a/forge-core/runtime/loop_test.go b/forge-core/runtime/loop_test.go index 435d44f..1b58047 100644 --- a/forge-core/runtime/loop_test.go +++ b/forge-core/runtime/loop_test.go @@ -352,6 +352,80 @@ func TestLLMErrorReturnsFriendlyMessage(t *testing.T) { } } +func TestToolCallsExecutedWhenFinishReasonStop(t *testing.T) { + // Regression: some providers return FinishReason="stop" even when + // tool_calls are present. The loop must execute those tool calls + // instead of exiting prematurely. + callCount := 0 + toolExecuted := false + + client := &mockLLMClient{ + chatFunc: func(ctx context.Context, req *llm.ChatRequest) (*llm.ChatResponse, error) { + callCount++ + if callCount == 1 { + // Return tool call with FinishReason "stop" (the buggy case) + return &llm.ChatResponse{ + Message: llm.ChatMessage{ + Role: llm.RoleAssistant, + ToolCalls: []llm.ToolCall{ + { + ID: "call_1", + Type: "function", + Function: llm.FunctionCall{ + Name: "test_tool", + Arguments: `{"input":"hello"}`, + }, + }, + }, + }, + FinishReason: "stop", // Bug trigger: stop + tool_calls + }, nil + } + // Second call: final response after tool execution + return &llm.ChatResponse{ + Message: llm.ChatMessage{Role: llm.RoleAssistant, Content: "Tool executed successfully."}, + FinishReason: "stop", + }, nil + }, + } + + tools := &mockToolExecutor{ + executeFunc: func(ctx context.Context, name string, arguments json.RawMessage) (string, error) { + toolExecuted = true + return "tool result", nil + }, + toolDefs: []llm.ToolDefinition{ + {Type: "function", Function: llm.FunctionSchema{Name: "test_tool"}}, + }, + } + + executor := NewLLMExecutor(LLMExecutorConfig{ + Client: client, + Tools: tools, + }) + + task := &a2a.Task{ID: "stop-with-tools"} + msg := &a2a.Message{ + Role: a2a.MessageRoleUser, + Parts: []a2a.Part{a2a.NewTextPart("do it")}, + } + + resp, err := executor.Execute(context.Background(), task, msg) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp == nil { + t.Fatal("expected response, got nil") + } + + if !toolExecuted { + t.Error("tool was not executed despite being in tool_calls (FinishReason=stop bug)") + } + if callCount != 2 { + t.Errorf("expected 2 LLM calls (tool call + final), got %d", callCount) + } +} + // ─── Workflow Tracker Tests ────────────────────────────────────────── func TestToolPhaseClassification(t *testing.T) { diff --git a/forge-skills/parser/parser.go b/forge-skills/parser/parser.go index f8ee0d2..618639d 100644 --- a/forge-skills/parser/parser.go +++ b/forge-skills/parser/parser.go @@ -118,7 +118,7 @@ func ParseWithMetadata(r io.Reader) ([]contract.SkillEntry, *contract.SkillMetad var forgeReqs *contract.SkillRequirements var egressDomains []string if meta != nil { - forgeReqs, egressDomains, _ = extractForgeReqs(meta) + forgeReqs, egressDomains, _ = ExtractForgeReqs(meta) } bodyStr := strings.TrimSpace(string(body)) @@ -216,9 +216,9 @@ func validateCategoryAndTags(meta *contract.SkillMetadata) error { return nil } -// extractForgeReqs extracts SkillRequirements, egress_domains, and guardrails from the generic +// ExtractForgeReqs extracts SkillRequirements, egress_domains, and guardrails from the generic // metadata map by re-marshaling metadata["forge"] through yaml round-trip into ForgeSkillMeta. -func extractForgeReqs(meta *contract.SkillMetadata) (*contract.SkillRequirements, []string, *contract.SkillGuardrailConfig) { +func ExtractForgeReqs(meta *contract.SkillMetadata) (*contract.SkillRequirements, []string, *contract.SkillGuardrailConfig) { if meta == nil || meta.Metadata == nil { return nil, nil, nil } diff --git a/forge-ui/discovery.go b/forge-ui/discovery.go index 5a4692c..c5b608b 100644 --- a/forge-ui/discovery.go +++ b/forge-ui/discovery.go @@ -111,8 +111,12 @@ func (s *Scanner) scanDir(dir string) (*AgentInfo, error) { } // detectExternalAgent checks whether an agent in dir is running externally -// (started via CLI rather than the UI). It reads .forge/serve.json and -// verifies the port is still listening. Returns the port and true if running. +// (started via CLI rather than the UI). It reads .forge/serve.json, verifies +// PID liveness, and probes the port. Returns the port and true if running. +// +// If the PID from serve.json is no longer alive, the state file is stale — +// it is removed and the agent is reported as stopped. This prevents ghost +// agents from appearing after a crash or unclean shutdown. func detectExternalAgent(dir string) (int, bool) { statePath := filepath.Join(dir, ".forge", "serve.json") data, err := os.ReadFile(statePath) @@ -125,6 +129,12 @@ func detectExternalAgent(dir string) (int, bool) { return 0, false } + // Check PID liveness first — if the process is dead, serve.json is stale. + if state.PID > 0 && !pidAlive(state.PID) { + _ = os.Remove(statePath) + return 0, false + } + host := state.Host if host == "" { host = "127.0.0.1" @@ -133,6 +143,7 @@ func detectExternalAgent(dir string) (int, bool) { // Verify the port is actually listening (fast TCP probe). conn, err := net.DialTimeout("tcp", fmt.Sprintf("%s:%d", host, state.Port), 500*time.Millisecond) if err != nil { + // PID may be alive but port not yet ready, or process is zombie. return 0, false } _ = conn.Close() diff --git a/forge-ui/discovery_test.go b/forge-ui/discovery_test.go index 912a38c..3caae50 100644 --- a/forge-ui/discovery_test.go +++ b/forge-ui/discovery_test.go @@ -136,6 +136,35 @@ model: } } +func TestDetectExternalAgentStalePID(t *testing.T) { + dir := t.TempDir() + forgeDir := filepath.Join(dir, ".forge") + if err := os.MkdirAll(forgeDir, 0o755); err != nil { + t.Fatal(err) + } + + // Write serve.json with a dead PID (PID 2147483647 should not exist). + writeFile(t, filepath.Join(forgeDir, "serve.json"), `{"pid":2147483647,"port":9999,"host":"127.0.0.1"}`) + + port, ok := detectExternalAgent(dir) + if ok { + t.Errorf("expected false for stale PID, got port %d", port) + } + + // serve.json should have been cleaned up. + if _, err := os.Stat(filepath.Join(forgeDir, "serve.json")); !os.IsNotExist(err) { + t.Error("expected stale serve.json to be removed") + } +} + +func TestDetectExternalAgentNoStateFile(t *testing.T) { + dir := t.TempDir() + port, ok := detectExternalAgent(dir) + if ok { + t.Errorf("expected false when no serve.json, got port %d", port) + } +} + func TestScanEmptyDir(t *testing.T) { root := t.TempDir() diff --git a/forge-ui/handlers_skill_builder.go b/forge-ui/handlers_skill_builder.go index 3c20b80..46e497f 100644 --- a/forge-ui/handlers_skill_builder.go +++ b/forge-ui/handlers_skill_builder.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/initializ/forge/forge-cli/runtime" + "github.com/initializ/forge/forge-core/llm/oauth" "github.com/initializ/forge/forge-core/types" ) @@ -71,32 +72,55 @@ func (s *UIServer) handleSkillBuilderProvider(w http.ResponseWriter, r *http.Req } provider := cfg.Model.Provider - model := SkillBuilderCodegenModel(provider, cfg.Model.Name) // Load the agent's .env and encrypted secrets so we can check for API keys // that aren't in the UI process's own environment. envPath := filepath.Join(agentDir, ".env") envVars, _ := runtime.LoadEnvFile(envPath) for k, v := range envVars { + // Don't pollute process env with __oauth__ sentinels — they block + // OverlaySecretsToEnv from replacing them with real keys later. + if v == "__oauth__" { + continue + } if os.Getenv(k) == "" { _ = os.Setenv(k, v) } } runtime.OverlaySecretsToEnv(cfg, agentDir) - // Check if the provider's API key env var is set + // Check if the provider's API key env var is set (excluding __oauth__ sentinel) + keyVal := func(k string) bool { + v := os.Getenv(k) + return v != "" && v != "__oauth__" + } hasKey := false + isOAuth := false switch provider { case "openai": - hasKey = os.Getenv("OPENAI_API_KEY") != "" + hasKey = keyVal("OPENAI_API_KEY") + if !hasKey { + // Check for stored OAuth credentials + if token, err := oauth.LoadCredentials("openai"); err == nil && token != nil && token.RefreshToken != "" { + hasKey = true + isOAuth = true + } + } case "anthropic": - hasKey = os.Getenv("ANTHROPIC_API_KEY") != "" + hasKey = keyVal("ANTHROPIC_API_KEY") case "gemini": - hasKey = os.Getenv("GEMINI_API_KEY") != "" + hasKey = keyVal("GEMINI_API_KEY") case "ollama": hasKey = true // Ollama doesn't need an API key default: - hasKey = os.Getenv("LLM_API_KEY") != "" || os.Getenv("MODEL_API_KEY") != "" + hasKey = keyVal("LLM_API_KEY") || keyVal("MODEL_API_KEY") + } + + // OAuth/Codex backend has model restrictions — use agent's configured model. + // API key clients get upgraded to a stronger codegen model. + model := cfg.Model.Name + if !isOAuth { + model = SkillBuilderCodegenModel(provider, model) } writeJSON(w, http.StatusOK, map[string]any{ @@ -243,19 +267,17 @@ func (s *UIServer) handleSkillBuilderSave(w http.ResponseWriter, r *http.Request return } - err := s.cfg.SkillSaveFunc(SkillSaveOptions{ + saveResult, err := s.cfg.SkillSaveFunc(SkillSaveOptions{ AgentDir: agentDir, SkillName: req.SkillName, SkillMD: req.SkillMD, Scripts: req.Scripts, + EnvVars: req.EnvVars, }) if err != nil { writeError(w, http.StatusInternalServerError, "saving skill: "+err.Error()) return } - writeJSON(w, http.StatusOK, map[string]string{ - "status": "saved", - "path": "skills/" + req.SkillName + "/SKILL.md", - }) + writeJSON(w, http.StatusOK, saveResult) } diff --git a/forge-ui/handlers_skill_builder_test.go b/forge-ui/handlers_skill_builder_test.go index 2b2bab2..3217816 100644 --- a/forge-ui/handlers_skill_builder_test.go +++ b/forge-ui/handlers_skill_builder_test.go @@ -37,12 +37,17 @@ model: return nil } - mockSave := func(opts SkillSaveOptions) error { + mockSave := func(opts SkillSaveOptions) (*SkillSaveResult, error) { skillDir := filepath.Join(opts.AgentDir, "skills", opts.SkillName) if err := os.MkdirAll(skillDir, 0o755); err != nil { - return err + return nil, err } - return os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(opts.SkillMD), 0o644) + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(opts.SkillMD), 0o644); err != nil { + return nil, err + } + return &SkillSaveResult{ + Path: "skills/" + opts.SkillName + "/SKILL.md", + }, nil } srv := NewUIServer(UIServerConfig{ @@ -77,8 +82,11 @@ func TestSkillBuilderProvider(t *testing.T) { if resp["provider"] != "openai" { t.Errorf("provider = %q, want %q", resp["provider"], "openai") } - if resp["model"] != "gpt-4.1" { - t.Errorf("model = %q, want %q", resp["model"], "gpt-4.1") + // Model is gpt-4.1 (API key codegen upgrade) or the agent's configured + // model (OAuth — Codex backend has model restrictions). + model := resp["model"].(string) + if model != "gpt-4.1" && model != "gpt-4o" { + t.Errorf("model = %q, want %q or %q", model, "gpt-4.1", "gpt-4o") } } @@ -337,13 +345,13 @@ A new tool. t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) } - var resp map[string]string + var resp SkillSaveResult if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { t.Fatalf("decode error: %v", err) } - if resp["status"] != "saved" { - t.Errorf("status = %q, want %q", resp["status"], "saved") + if resp.Path != "skills/new-skill/SKILL.md" { + t.Errorf("path = %q, want %q", resp.Path, "skills/new-skill/SKILL.md") } // Verify file was created diff --git a/forge-ui/process.go b/forge-ui/process.go index b322467..7e8be88 100644 --- a/forge-ui/process.go +++ b/forge-ui/process.go @@ -30,20 +30,37 @@ func NewPortAllocator(basePort int) *PortAllocator { } } -// Allocate returns the next available port. +// Allocate returns the next available port. It verifies the port is actually +// free (not just absent from the used map) by attempting a TCP listen. This +// prevents collisions with externally-started agents or other processes that +// the PortAllocator doesn't know about (e.g., after a UI restart). func (pa *PortAllocator) Allocate() int { pa.mu.Lock() defer pa.mu.Unlock() port := pa.basePort for { if _, ok := pa.used[port]; !ok { + if portFree(port) { + pa.used[port] = struct{}{} + return port + } + // Port is in use by another process; mark it and skip. pa.used[port] = struct{}{} - return port } port++ } } +// portFree checks whether a TCP port is available by attempting to listen on it. +func portFree(port int) bool { + ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port)) + if err != nil { + return false + } + _ = ln.Close() + return true +} + // Release frees a port for reuse. func (pa *PortAllocator) Release(port int) { pa.mu.Lock() diff --git a/forge-ui/process_test.go b/forge-ui/process_test.go index aea2951..bc2d23c 100644 --- a/forge-ui/process_test.go +++ b/forge-ui/process_test.go @@ -8,19 +8,20 @@ func TestPortAllocator(t *testing.T) { pa := NewPortAllocator(9100) p1 := pa.Allocate() - if p1 != 9100 { - t.Errorf("first port = %d, want 9100", p1) + if p1 < 9100 { + t.Errorf("first port = %d, want >= 9100", p1) } p2 := pa.Allocate() - if p2 != 9101 { - t.Errorf("second port = %d, want 9101", p2) + if p2 <= p1 { + t.Errorf("second port = %d, want > %d", p2, p1) } - pa.Release(9100) + // Release first port and re-allocate — should get it back. + pa.Release(p1) p3 := pa.Allocate() - if p3 != 9100 { - t.Errorf("after release, port = %d, want 9100", p3) + if p3 != p1 { + t.Errorf("after release, port = %d, want %d", p3, p1) } } @@ -43,12 +44,12 @@ func TestProcessManagerStartExecError(t *testing.T) { t.Fatal("expected error from non-existent binary") } - // Port should have been released. - pm.ports.mu.Lock() - _, used := pm.ports.used[9100] - pm.ports.mu.Unlock() - if used { - t.Error("expected port 9100 to be released after failure") + // The allocated port should have been released (not in allocated map). + pm.mu.Lock() + _, tracked := pm.allocated["test-agent"] + pm.mu.Unlock() + if tracked { + t.Error("expected agent port to be released from allocated map after failure") } } diff --git a/forge-ui/static/dist/app.js b/forge-ui/static/dist/app.js index a9510a5..6eed95a 100644 --- a/forge-ui/static/dist/app.js +++ b/forge-ui/static/dist/app.js @@ -195,11 +195,13 @@ async function validateSkillBuilderMD(agentId, skillMD, scripts) { return res.json(); } -async function saveSkillBuilder(agentId, skillName, skillMD, scripts) { +async function saveSkillBuilder(agentId, skillName, skillMD, scripts, envVars) { + const body = { skill_name: skillName, skill_md: skillMD, scripts }; + if (envVars && Object.keys(envVars).length > 0) body.env_vars = envVars; const res = await fetch(`/api/agents/${agentId}/skill-builder/save`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ skill_name: skillName, skill_md: skillMD, scripts }), + body: JSON.stringify(body), }); const data = await res.json(); if (!res.ok) throw new Error(data.error || `Save failed: ${res.status}`); @@ -2098,6 +2100,7 @@ function SkillBuilderPage({ agentId }) { const [activeTab, setActiveTab] = useState('skill.md'); const [validation, setValidation] = useState(null); const [saveStatus, setSaveStatus] = useState(null); + const [envInputs, setEnvInputs] = useState({}); const [error, setError] = useState(null); const abortRef = useRef(null); const chatEndRef = useRef(null); @@ -2234,13 +2237,16 @@ function SkillBuilderPage({ agentId }) { } try { - const result = await saveSkillBuilder(agentId, skillName, skillMD, scripts); + // Pass any user-provided env vars + const envVars = Object.keys(envInputs).length > 0 ? envInputs : undefined; + const result = await saveSkillBuilder(agentId, skillName, skillMD, scripts, envVars); setSaveStatus(result); + setEnvInputs({}); setError(null); } catch (err) { setError('Save failed: ' + err.message); } - }, [agentId, skillMD, scripts]); + }, [agentId, skillMD, scripts, envInputs]); const handleKeyDown = useCallback((e) => { if (e.key === 'Enter' && !e.shiftKey) { @@ -2371,6 +2377,30 @@ function SkillBuilderPage({ agentId }) { ${saveStatus && html`
Saved to ${saveStatus.path} + ${saveStatus.egress_added && saveStatus.egress_added.length > 0 && html` +
Egress domains added: ${saveStatus.egress_added.join(', ')}
+ `} + ${saveStatus.env_configured && saveStatus.env_configured.length > 0 && html` +
Env vars configured: ${saveStatus.env_configured.join(', ')}
+ `} +
+ `} + ${saveStatus && saveStatus.env_missing && saveStatus.env_missing.filter(e => e.kind !== 'optional').length > 0 && html` +
+
Missing environment variables:
+ ${saveStatus.env_missing.filter(e => e.kind !== 'optional').map(entry => html` +
+ + setEnvInputs(prev => ({...prev, [entry.name]: e.target.value}))} /> +
+ `)} +
`} diff --git a/forge-ui/types.go b/forge-ui/types.go index ba940d3..e2c598e 100644 --- a/forge-ui/types.go +++ b/forge-ui/types.go @@ -97,11 +97,26 @@ type SkillSaveOptions struct { SkillName string SkillMD string Scripts map[string]string + EnvVars map[string]string // env vars to write to .env } -// SkillSaveFunc saves a generated skill to disk. +// SkillSaveResult holds the result of saving a skill, including env/egress changes. +type SkillSaveResult struct { + Path string `json:"path"` + EgressAdded []string `json:"egress_added,omitempty"` + EnvConfigured []string `json:"env_configured,omitempty"` + EnvMissing []SkillEnvEntry `json:"env_missing,omitempty"` +} + +// SkillEnvEntry describes a missing environment variable requirement. +type SkillEnvEntry struct { + Name string `json:"name"` + Kind string `json:"kind"` // "required", "one_of", "optional" +} + +// SkillSaveFunc saves a generated skill to disk and configures env/egress. // Injected by forge-cli. -type SkillSaveFunc func(opts SkillSaveOptions) error +type SkillSaveFunc func(opts SkillSaveOptions) (*SkillSaveResult, error) // SkillBuilderChatRequest is the POST body for the skill builder chat endpoint. type SkillBuilderChatRequest struct { @@ -119,6 +134,7 @@ type SkillBuilderSaveRequest struct { SkillName string `json:"skill_name"` SkillMD string `json:"skill_md"` Scripts map[string]string `json:"scripts,omitempty"` + EnvVars map[string]string `json:"env_vars,omitempty"` } // SkillValidationResult holds the result of validating a SKILL.md.