From 7672e0922fcdc2264546084c610bc0c9f9dbae97 Mon Sep 17 00:00:00 2001 From: Virgil Date: Mon, 30 Mar 2026 14:32:01 +0000 Subject: [PATCH] fix(ax): align plan IDs and validation helpers Co-Authored-By: Virgil --- docs/RFC.md | 1 + pkg/agentic/dispatch_sync.go | 2 +- pkg/agentic/paths_test.go | 3 +-- pkg/agentic/plan.go | 36 +++++++++++++--------------------- pkg/agentic/plan_crud_test.go | 17 +++++++--------- pkg/agentic/plan_logic_test.go | 2 +- pkg/agentic/plan_test.go | 31 +++++++++++++++-------------- pkg/agentic/prep.go | 22 ++++++++++++++------- pkg/monitor/monitor.go | 2 +- 9 files changed, 57 insertions(+), 59 deletions(-) diff --git a/docs/RFC.md b/docs/RFC.md index dd08e82..9a3ee97 100644 --- a/docs/RFC.md +++ b/docs/RFC.md @@ -435,6 +435,7 @@ func (s *PrepSubsystem) gitCmd(ctx context.Context, dir string, args ...string) ## Changelog - 2026-03-30: main now logs startup failures with structured context, and the workspace contract reference restored usage-example comments for the Action lifecycle messages. +- 2026-03-30: plan IDs now come from core.ID(), workspace prep validates org/repo names with core.ValidateName, and plan paths use core.SanitisePath. - 2026-03-29: cmd/core-agent no longer rewrites `os.Args` before startup. The binary-owned commands now use named handlers, keeping the entrypoint on Core CLI primitives instead of repo-local argument mutation. - 2026-03-29: brain/provider.go no longer imports net/http for Gin handlers. Handler responses now use named status constants and shared response helpers. HTTP remains intentionally centralised in pkg/agentic/transport.go. - 2026-03-26: WIP — net/http consolidated to transport.go (ONE file). net/url + io/fs eliminated. RFC-025 updated with 3 new quality gates (net/http, net/url, io/fs). 1:1 test + example test coverage. Array[T].Deduplicate replaces custom helpers. diff --git a/pkg/agentic/dispatch_sync.go b/pkg/agentic/dispatch_sync.go index df46f8c..f1c9607 100644 --- a/pkg/agentic/dispatch_sync.go +++ b/pkg/agentic/dispatch_sync.go @@ -22,7 +22,7 @@ type DispatchSyncInput struct { // DispatchSyncResult is the output of a synchronous task run. // -// if result.OK { fmt.Println("done:", result.Status) } +// if result.OK { core.Print(nil, "done: %s", result.Status) } type DispatchSyncResult struct { OK bool Status string diff --git a/pkg/agentic/paths_test.go b/pkg/agentic/paths_test.go index f151ac8..ba22518 100644 --- a/pkg/agentic/paths_test.go +++ b/pkg/agentic/paths_test.go @@ -158,8 +158,7 @@ func TestPlan_ValidPlanStatus_Bad(t *testing.T) { func TestPlan_GeneratePlanID_Good(t *testing.T) { id := generatePlanID("Fix the login bug in auth service") - assert.True(t, len(id) > 0) - assert.True(t, strings.Contains(id, "fix-the-login-bug")) + assertCoreIDFormat(t, id) } // --- DefaultBranch --- diff --git a/pkg/agentic/plan.go b/pkg/agentic/plan.go index 1cd1294..93baf38 100644 --- a/pkg/agentic/plan.go +++ b/pkg/agentic/plan.go @@ -4,8 +4,6 @@ package agentic import ( "context" - "crypto/rand" - "encoding/hex" "time" core "dappco.re/go/core" @@ -14,7 +12,7 @@ import ( // Plan represents an implementation plan for agent work. // -// plan := &Plan{ID: "migrate-core-abc", Title: "Migrate Core", Status: "draft", Objective: "..."} +// plan := &Plan{ID: "id-1-a3f2b1", Title: "Migrate Core", Status: "draft", Objective: "..."} // writePlan(PlansRoot(), plan) type Plan struct { ID string `json:"id"` @@ -58,7 +56,7 @@ type PlanCreateInput struct { // PlanCreateOutput is the output for agentic_plan_create. // -// out := agentic.PlanCreateOutput{Success: true, ID: "migrate-pkg-agentic-abc123"} +// out := agentic.PlanCreateOutput{Success: true, ID: "id-1-a3f2b1"} type PlanCreateOutput struct { Success bool `json:"success"` ID string `json:"id"` @@ -67,14 +65,14 @@ type PlanCreateOutput struct { // PlanReadInput is the input for agentic_plan_read. // -// input := agentic.PlanReadInput{ID: "migrate-pkg-agentic-abc123"} +// input := agentic.PlanReadInput{ID: "id-1-a3f2b1"} type PlanReadInput struct { ID string `json:"id"` } // PlanReadOutput is the output for agentic_plan_read. // -// out := agentic.PlanReadOutput{Success: true, Plan: agentic.Plan{ID: "migrate-pkg-agentic-abc123"}} +// out := agentic.PlanReadOutput{Success: true, Plan: agentic.Plan{ID: "id-1-a3f2b1"}} type PlanReadOutput struct { Success bool `json:"success"` Plan Plan `json:"plan"` @@ -82,7 +80,7 @@ type PlanReadOutput struct { // PlanUpdateInput is the input for agentic_plan_update. // -// input := agentic.PlanUpdateInput{ID: "migrate-pkg-agentic-abc123", Status: "verified"} +// input := agentic.PlanUpdateInput{ID: "id-1-a3f2b1", Status: "verified"} type PlanUpdateInput struct { ID string `json:"id"` Status string `json:"status,omitempty"` @@ -103,14 +101,14 @@ type PlanUpdateOutput struct { // PlanDeleteInput is the input for agentic_plan_delete. // -// input := agentic.PlanDeleteInput{ID: "migrate-pkg-agentic-abc123"} +// input := agentic.PlanDeleteInput{ID: "id-1-a3f2b1"} type PlanDeleteInput struct { ID string `json:"id"` } // PlanDeleteOutput is the output for agentic_plan_delete. // -// out := agentic.PlanDeleteOutput{Success: true, Deleted: "migrate-pkg-agentic-abc123"} +// out := agentic.PlanDeleteOutput{Success: true, Deleted: "id-1-a3f2b1"} type PlanDeleteOutput struct { Success bool `json:"success"` Deleted string `json:"deleted"` @@ -126,7 +124,7 @@ type PlanListInput struct { // PlanListOutput is the output for agentic_plan_list. // -// out := agentic.PlanListOutput{Success: true, Count: 2, Plans: []agentic.Plan{{ID: "migrate-pkg-agentic-abc123"}}} +// out := agentic.PlanListOutput{Success: true, Count: 2, Plans: []agentic.Plan{{ID: "id-1-a3f2b1"}}} type PlanListOutput struct { Success bool `json:"success"` Count int `json:"count"` @@ -361,21 +359,15 @@ func (s *PrepSubsystem) planList(_ context.Context, _ *mcp.CallToolRequest, inpu // --- Helpers --- func planPath(dir, id string) string { - // Sanitise ID to prevent path traversal - safe := core.PathBase(id) - if safe == "." || safe == ".." || safe == "" { - safe = "invalid" - } + safe := core.SanitisePath(id) return core.JoinPath(dir, core.Concat(safe, ".json")) } -func generatePlanID(title string) string { - slug := sanitisePlanSlug(title) - - // Append short random suffix for uniqueness - b := make([]byte, 3) - rand.Read(b) - return core.Concat(slug, "-", hex.EncodeToString(b)) +// generatePlanID returns a Core ID for a plan file. +// +// id := generatePlanID("Migrate Core") // "id-1-a3f2b1" +func generatePlanID(_ string) string { + return core.ID() } // readPlanResult reads and decodes a plan file as core.Result. diff --git a/pkg/agentic/plan_crud_test.go b/pkg/agentic/plan_crud_test.go index 7b0100f..7d9dfa9 100644 --- a/pkg/agentic/plan_crud_test.go +++ b/pkg/agentic/plan_crud_test.go @@ -43,7 +43,7 @@ func TestPlan_PlanCreate_Good(t *testing.T) { require.NoError(t, err) assert.True(t, out.Success) assert.NotEmpty(t, out.ID) - assert.Contains(t, out.ID, "migrate-core") + assertCoreIDFormat(t, out.ID) assert.NotEmpty(t, out.Path) assert.True(t, fs.Exists(out.Path)) @@ -367,8 +367,7 @@ func TestPlan_PlanCreate_Ugly_VeryLongTitle(t *testing.T) { require.NoError(t, err) assert.True(t, out.Success) assert.NotEmpty(t, out.ID) - // The slug portion should be truncated - assert.LessOrEqual(t, len(out.ID), 50, "ID should be reasonably short") + assertCoreIDFormat(t, out.ID) } func TestPlan_PlanCreate_Ugly_UnicodeTitle(t *testing.T) { @@ -383,6 +382,7 @@ func TestPlan_PlanCreate_Ugly_UnicodeTitle(t *testing.T) { require.NoError(t, err) assert.True(t, out.Success) assert.NotEmpty(t, out.ID) + assertCoreIDFormat(t, out.ID) // Should be readable from disk assert.True(t, fs.Exists(out.Path)) } @@ -498,8 +498,8 @@ func TestPlan_ValidPlanStatus_Ugly_UnicodeStatus(t *testing.T) { } func TestPlan_ValidPlanStatus_Ugly_NearMissStatus(t *testing.T) { - assert.False(t, validPlanStatus("Draft")) // capital D - assert.False(t, validPlanStatus("DRAFT")) // all caps + assert.False(t, validPlanStatus("Draft")) // capital D + assert.False(t, validPlanStatus("DRAFT")) // all caps assert.False(t, validPlanStatus("in-progress")) // hyphen instead of underscore assert.False(t, validPlanStatus(" draft")) // leading space assert.False(t, validPlanStatus("draft ")) // trailing space @@ -508,16 +508,13 @@ func TestPlan_ValidPlanStatus_Ugly_NearMissStatus(t *testing.T) { // --- generatePlanID Bad/Ugly --- func TestPlan_GeneratePlanID_Bad(t *testing.T) { - // Empty title — slug will be empty, but random suffix is still appended id := generatePlanID("") - assert.NotEmpty(t, id, "should still generate an ID with random suffix") - assert.Contains(t, id, "-", "should have random suffix separated by dash") + assertCoreIDFormat(t, id) } func TestPlan_GeneratePlanID_Ugly(t *testing.T) { - // Title with only special chars — slug will be empty id := generatePlanID("!@#$%^&*()") - assert.NotEmpty(t, id, "should still generate an ID with random suffix") + assertCoreIDFormat(t, id) } // --- planList Bad/Ugly --- diff --git a/pkg/agentic/plan_logic_test.go b/pkg/agentic/plan_logic_test.go index 5c2145e..f091e7c 100644 --- a/pkg/agentic/plan_logic_test.go +++ b/pkg/agentic/plan_logic_test.go @@ -19,7 +19,7 @@ func TestPlan_PlanPath_Good_BasicFormat(t *testing.T) { } func TestPlan_PlanPath_Good_NestedIDStripped(t *testing.T) { - // PathBase strips directory component — prevents path traversal + // SanitisePath strips directory components — prevents path traversal result := planPath("/plans", "../../../etc/passwd") assert.Equal(t, "/plans/passwd.json", result) } diff --git a/pkg/agentic/plan_test.go b/pkg/agentic/plan_test.go index a135833..1e2485c 100644 --- a/pkg/agentic/plan_test.go +++ b/pkg/agentic/plan_test.go @@ -11,6 +11,16 @@ import ( "github.com/stretchr/testify/require" ) +func assertCoreIDFormat(t *testing.T, id string) { + t.Helper() + parts := strings.Split(id, "-") + if assert.Len(t, parts, 3) { + assert.Equal(t, "id", parts[0]) + assert.NotEmpty(t, parts[1]) + assert.NotEmpty(t, parts[2]) + } +} + func TestPlan_PlanPath_Good(t *testing.T) { assert.Equal(t, "/tmp/plans/my-plan-abc123.json", planPath("/tmp/plans", "my-plan-abc123")) assert.Equal(t, "/data/test.json", planPath("/data", "test")) @@ -134,28 +144,19 @@ func TestPlan_WriteRead_Good_Roundtrip(t *testing.T) { assert.Equal(t, "Working on it", read.Phases[1].Notes) } -func TestPlan_GeneratePlanID_Good_Slugifies(t *testing.T) { +func TestPlan_GeneratePlanID_Good_CoreFormat(t *testing.T) { id := generatePlanID("Add Unit Tests for Agentic") - assert.True(t, strings.HasPrefix(id, "add-unit-tests-for-agentic"), "got: %s", id) - // Should have random suffix - parts := strings.Split(id, "-") - assert.True(t, len(parts) >= 5, "expected slug with random suffix, got: %s", id) + assertCoreIDFormat(t, id) } -func TestPlan_GeneratePlanID_Good_TruncatesLong(t *testing.T) { +func TestPlan_GeneratePlanID_Good_IgnoresTitleLength(t *testing.T) { id := generatePlanID("This is a very long title that should be truncated to a reasonable length for file naming purposes") - // Slug part (before random suffix) should be <= 30 chars - lastDash := strings.LastIndex(id, "-") - slug := id[:lastDash] - assert.True(t, len(slug) <= 36, "slug too long: %s (%d chars)", slug, len(slug)) + assertCoreIDFormat(t, id) } -func TestPlan_GeneratePlanID_Good_HandlesSpecialChars(t *testing.T) { +func TestPlan_GeneratePlanID_Good_IgnoresSpecialChars(t *testing.T) { id := generatePlanID("Fix bug #123: auth & session!") - assert.True(t, strings.Contains(id, "fix-bug"), "got: %s", id) - assert.NotContains(t, id, "#") - assert.NotContains(t, id, "!") - assert.NotContains(t, id, "&") + assertCoreIDFormat(t, id) } func TestPlan_GeneratePlanID_Good_Unique(t *testing.T) { diff --git a/pkg/agentic/prep.go b/pkg/agentic/prep.go index a1309ab..c535fc0 100644 --- a/pkg/agentic/prep.go +++ b/pkg/agentic/prep.go @@ -373,7 +373,19 @@ func workspaceDir(org, repo string, input PrepInput) (string, error) { // r := workspaceDirResult("core", "go-io", PrepInput{Issue: 15}) // if r.OK { wsDir := r.Value.(string) } func workspaceDirResult(org, repo string, input PrepInput) core.Result { - base := core.JoinPath(WorkspaceRoot(), org, repo) + orgName := core.ValidateName(org) + if !orgName.OK { + err, _ := orgName.Value.(error) + return core.Result{Value: core.E("workspaceDir", "invalid org name", err), OK: false} + } + + repoName := core.ValidateName(repo) + if !repoName.OK { + err, _ := repoName.Value.(error) + return core.Result{Value: core.E("workspaceDir", "invalid repo name", err), OK: false} + } + + base := core.JoinPath(WorkspaceRoot(), orgName.Value.(string), repoName.Value.(string)) switch { case input.PR > 0: return core.Result{Value: core.JoinPath(base, core.Sprintf("pr-%d", input.PR)), OK: true} @@ -417,12 +429,8 @@ func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolReques metaDir := workspaceMetaDir(wsDir) out := PrepOutput{WorkspaceDir: wsDir, RepoDir: repoDir} - // Source repo path — sanitise to prevent path traversal - repoName := core.PathBase(input.Repo) - if repoName == "." || repoName == ".." || repoName == "" { - return nil, PrepOutput{}, core.E("prep", core.Concat("invalid repo name: ", input.Repo), nil) - } - repoPath := core.JoinPath(s.codePath, input.Org, repoName) + // Source repo path — org and repo were validated by workspaceDirResult. + repoPath := core.JoinPath(s.codePath, input.Org, input.Repo) // Ensure meta directory exists if r := fs.EnsureDir(metaDir); !r.OK { diff --git a/pkg/monitor/monitor.go b/pkg/monitor/monitor.go index 389a775..8b2b0b5 100644 --- a/pkg/monitor/monitor.go +++ b/pkg/monitor/monitor.go @@ -22,7 +22,7 @@ import ( // fs provides unrestricted filesystem access (root "/" = no sandbox). // // r := fs.Read(core.Concat(wsRoot, "/", name, "/status.json")) -// if text, ok := resultString(r); ok { json.Unmarshal([]byte(text), &st) } +// if text, ok := resultString(r); ok { _ = core.JSONUnmarshalString(text, &st) } var fs = agentic.LocalFs() type channelSender interface {