From 08aa56b16c1ca8c86c0fecb3461025c5e24f41bd Mon Sep 17 00:00:00 2001 From: weichen Date: Wed, 16 Sep 2026 01:01:39 +0000 Subject: [PATCH] feat(intentflow): re-authorize steps before confirm and execute Confirmation stays a command receipt; FakeActionOwner now fail-closes on revoke and resource version drift. Co-authored-by: Cursor --- .doc/system-design/module-boundaries.yaml | 1 + internal/app/app.go | 1 + internal/app/intentflow/authorize_test.go | 126 ++++++++++++++++ internal/app/intentflow/owner.go | 37 ++++- internal/app/intentflow/plans.go | 175 ++++++++++++++++++++-- 5 files changed, 323 insertions(+), 17 deletions(-) create mode 100644 internal/app/intentflow/authorize_test.go diff --git a/.doc/system-design/module-boundaries.yaml b/.doc/system-design/module-boundaries.yaml index 770beb63..c87d3d00 100644 --- a/.doc/system-design/module-boundaries.yaml +++ b/.doc/system-design/module-boundaries.yaml @@ -164,6 +164,7 @@ imports: - app - app/chatflow - app/docflow + - app/intentflow - app/stageflow - app/uow - auth diff --git a/internal/app/app.go b/internal/app/app.go index 25bdce9f..52934929 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -193,6 +193,7 @@ func Assemble(cfg *config.Config, svc *bizdev.Service) (*Runtime, error) { rt.Docflow.UoW = uow.NewPG(pool) rt.Docflow.Events = events.PGSink{} rt.IntentPlans = intentflow.NewPlans(rt.Docflow.UoW, intentflow.NewRepository(), intentflow.Options{ + Owner: intentflow.NewFakeActionOwner(), Events: events.PGSink{}, }) } diff --git a/internal/app/intentflow/authorize_test.go b/internal/app/intentflow/authorize_test.go new file mode 100644 index 00000000..77bb135b --- /dev/null +++ b/internal/app/intentflow/authorize_test.go @@ -0,0 +1,126 @@ +package intentflow_test + +import ( + "context" + "errors" + "testing" + + "chat-agent-system/internal/app/intentflow" +) + +func TestConfirmDeniedDoesNotMutatePlan(t *testing.T) { + p, _, owner := testPlans() + ctx := context.Background() + created, err := p.CreatePlan(ctx, draft("digest-a")) + if err != nil { + t.Fatal(err) + } + owner.Denied = true + _, err = p.DecidePlan(ctx, intentflow.DecisionCommand{ + Principal: principal(), PlanID: created.PlanID, ExpectedRevision: created.Revision, + DecisionNonce: "n-deny", DecisionDigest: "d-deny", Kind: intentflow.DecisionConfirm, + }) + if !errors.Is(err, intentflow.ErrPlanActionForbidden) { + t.Fatalf("want forbidden, got %v", err) + } + got, err := p.GetPlan(ctx, principal(), created.PlanID) + if err != nil { + t.Fatal(err) + } + if got.Revision != created.Revision || got.Status != intentflow.PlanAwaitingConfirmation { + t.Fatalf("confirm must be side-effect free on deny: %+v", got) + } +} + +func TestResourceVersionConflictOnConfirm(t *testing.T) { + p, _, owner := testPlans() + ctx := context.Background() + cmd := draft("digest-ver") + cmd.Steps[1].ExpectedResourceVersion = "v1" + created, err := p.CreatePlan(ctx, cmd) + if err != nil { + t.Fatal(err) + } + owner.RequiredVersion = map[string]string{"task.run": "v2"} + _, err = p.DecidePlan(ctx, intentflow.DecisionCommand{ + Principal: principal(), PlanID: created.PlanID, ExpectedRevision: created.Revision, + DecisionNonce: "n-ver", DecisionDigest: "d-ver", Kind: intentflow.DecisionConfirm, + }) + if !errors.Is(err, intentflow.ErrPlanActionVersionConflict) { + t.Fatalf("want version conflict, got %v", err) + } +} + +func TestRevokeBetweenConfirmAndExecute(t *testing.T) { + p, _, owner := testPlans() + ctx := context.Background() + created, err := p.CreatePlan(ctx, draft("digest-rev")) + if err != nil { + t.Fatal(err) + } + confirmed, err := p.DecidePlan(ctx, intentflow.DecisionCommand{ + Principal: principal(), PlanID: created.PlanID, ExpectedRevision: created.Revision, + DecisionNonce: "n-ok", DecisionDigest: "d-ok", Kind: intentflow.DecisionConfirm, + }) + if err != nil { + t.Fatal(err) + } + owner.Denied = true + stepID := confirmed.Steps[0].StepID + claimed, err := p.ClaimStep(ctx, "w1", confirmed.PlanID, stepID) + if err != nil { + t.Fatal(err) + } + if _, err := p.ExecuteClaimed(ctx, claimed); !errors.Is(err, intentflow.ErrPlanActionForbidden) { + t.Fatalf("revoked execute must fail closed, got %v", err) + } +} + +func TestEditBumpsRevisionAndInvalidatesOldCard(t *testing.T) { + p, _, _ := testPlans() + ctx := context.Background() + created, err := p.CreatePlan(ctx, draft("digest-edit")) + if err != nil { + t.Fatal(err) + } + edited, err := p.DecidePlan(ctx, intentflow.DecisionCommand{ + Principal: principal(), PlanID: created.PlanID, ExpectedRevision: created.Revision, + DecisionNonce: "n-edit", DecisionDigest: "d-edit", Kind: intentflow.DecisionEdit, + }) + if err != nil { + t.Fatal(err) + } + if edited.Revision <= created.Revision { + t.Fatalf("edit must bump revision, got %d", edited.Revision) + } + if edited.Status != intentflow.PlanAwaitingConfirmation { + t.Fatalf("edit stays awaiting, got %s", edited.Status) + } + _, err = p.DecidePlan(ctx, intentflow.DecisionCommand{ + Principal: principal(), PlanID: created.PlanID, ExpectedRevision: created.Revision, + DecisionNonce: "n-old", DecisionDigest: "d-old", Kind: intentflow.DecisionConfirm, + }) + if !errors.Is(err, intentflow.ErrPlanStaleRevision) { + t.Fatalf("old card must be stale, got %v", err) + } +} + +func TestIllegalPartialConfirmMissingDependency(t *testing.T) { + p, _, _ := testPlans() + ctx := context.Background() + cmd := draft("digest-dep") + cmd.Steps[1].DependsOn = []string{"stp_missing"} + created, err := p.CreatePlan(ctx, cmd) + if err != nil { + t.Fatal(err) + } + writeID := created.Steps[1].StepID + _, err = p.DecidePlan(ctx, intentflow.DecisionCommand{ + Principal: principal(), PlanID: created.PlanID, ExpectedRevision: created.Revision, + DecisionNonce: "n-part", DecisionDigest: "d-part", Kind: intentflow.DecisionConfirm, + StepIDs: []string{writeID}, + }) + if !errors.Is(err, intentflow.ErrPlanDependencyInvalid) { + t.Fatalf("partial without closure must be invalid, got %v", err) + } +} diff --git a/internal/app/intentflow/owner.go b/internal/app/intentflow/owner.go index c5e1fd5c..d1790251 100644 --- a/internal/app/intentflow/owner.go +++ b/internal/app/intentflow/owner.go @@ -20,11 +20,20 @@ type FenceObserver interface { // FakeActionOwner is the U3 test double. It records executions and receipts // without touching business tables. type FakeActionOwner struct { - mu sync.Mutex - execCount map[string]int - receipts map[string]Receipt - fences map[string]int64 - FailWrite bool + mu sync.Mutex + execCount map[string]int + receipts map[string]Receipt + fences map[string]int64 + FailWrite bool + Denied bool + DeniedKeys map[string]bool + RequiredVersion map[string]string +} + +// StepAuthorizer re-checks the current principal before confirm and execute. +// Confirmation is not authorization. +type StepAuthorizer interface { + Authorize(ctx context.Context, exec StepExec) error } func NewFakeActionOwner() *FakeActionOwner { @@ -44,10 +53,28 @@ func (f *FakeActionOwner) ObserveFence(stepID string, token int64) { f.fences[stepID] = token } +func (f *FakeActionOwner) Authorize(ctx context.Context, exec StepExec) error { + if f == nil { + return ErrPlanActionForbidden + } + f.mu.Lock() + defer f.mu.Unlock() + if f.Denied || f.DeniedKeys[exec.ActionKey] { + return ErrPlanActionForbidden + } + if want, ok := f.RequiredVersion[exec.ActionKey]; ok && exec.ExpectedResourceVersion != "" && exec.ExpectedResourceVersion != want { + return ErrPlanActionVersionConflict + } + return nil +} + func (f *FakeActionOwner) Execute(ctx context.Context, exec StepExec) (Receipt, error) { if f == nil { return Receipt{}, ErrPlanActionForbidden } + if err := f.Authorize(ctx, exec); err != nil { + return Receipt{}, err + } f.mu.Lock() defer f.mu.Unlock() if cur, ok := f.fences[exec.StepID]; ok && exec.FencingToken != cur { diff --git a/internal/app/intentflow/plans.go b/internal/app/intentflow/plans.go index 28c9bcd3..f5136e1a 100644 --- a/internal/app/intentflow/plans.go +++ b/internal/app/intentflow/plans.go @@ -22,6 +22,7 @@ type Plans struct { uow uow.UnitOfWork store Store owner ActionOwner + authz StepAuthorizer events EventSink now func() time.Time newID func(prefix string) string @@ -30,12 +31,13 @@ type Plans struct { } type Options struct { - Owner ActionOwner - Events EventSink - Now func() time.Time - NewID func(prefix string) string - Lease time.Duration - PlanTTL time.Duration + Owner ActionOwner + Authorizer StepAuthorizer + Events EventSink + Now func() time.Time + NewID func(prefix string) string + Lease time.Duration + PlanTTL time.Duration } func NewPlans(unit uow.UnitOfWork, store Store, opt Options) *Plans { @@ -55,7 +57,26 @@ func NewPlans(unit uow.UnitOfWork, store Store, opt Options) *Plans { if ttl <= 0 { ttl = defaultPlanTTL } - return &Plans{uow: unit, store: store, owner: opt.Owner, events: opt.Events, now: now, newID: newID, lease: lease, ttl: ttl} + authz := opt.Authorizer + if authz == nil { + if a, ok := opt.Owner.(StepAuthorizer); ok { + authz = a + } + } + return &Plans{uow: unit, store: store, owner: opt.Owner, authz: authz, events: opt.Events, now: now, newID: newID, lease: lease, ttl: ttl} +} + +func (p *Plans) stepAuthorizer() StepAuthorizer { + if p == nil { + return nil + } + if p.authz != nil { + return p.authz + } + if a, ok := p.owner.(StepAuthorizer); ok { + return a + } + return nil } func randomID(prefix string) string { @@ -296,6 +317,9 @@ func (p *Plans) DecidePlan(ctx context.Context, in DecisionCommand) (*PlanView, if in.ExpectedRevision != plan.Revision { return ErrPlanStaleRevision } + if err := p.authorizeDecision(ctx, plan, in); err != nil { + return err + } dec := &Decision{ DecisionID: p.newID("dec_"), PlanID: plan.PlanID, @@ -329,8 +353,12 @@ func (p *Plans) DecidePlan(ctx context.Context, in DecisionCommand) (*PlanView, from := []PlanStatus{plan.Status} var to PlanStatus switch in.Kind { - case DecisionConfirm: - if plan.Status != PlanAwaitingConfirmation && plan.Status != PlanAwaitingClarification && plan.Status != PlanProposed { + case DecisionConfirm, DecisionConfirmRetry, DecisionContinue: + if in.Kind == DecisionConfirm { + if plan.Status != PlanAwaitingConfirmation && plan.Status != PlanAwaitingClarification && plan.Status != PlanProposed { + return ErrPlanStaleRevision + } + } else if plan.Status != PlanPartiallyCompleted && plan.Status != PlanExecuting { return ErrPlanStaleRevision } if err := applyConfirm(plan, in.StepIDs, in.ExcludeUnselected, now); err != nil { @@ -343,6 +371,22 @@ func (p *Plans) DecidePlan(ctx context.Context, in DecisionCommand) (*PlanView, } cancelUnstarted(plan, now) to = PlanCancelled + case DecisionExclude: + if !isWaitingStatus(plan.Status) && plan.Status != PlanPartiallyCompleted { + return ErrPlanStaleRevision + } + if err := applyExclude(plan, in.StepIDs, now); err != nil { + return err + } + to = plan.Status + if allStepsTerminal(plan) { + to = PlanCancelled + } + case DecisionEdit: + if !isWaitingStatus(plan.Status) { + return ErrPlanStaleRevision + } + to = plan.Status default: return ErrPlanDependencyInvalid } @@ -364,6 +408,106 @@ func (p *Plans) DecidePlan(ctx context.Context, in DecisionCommand) (*PlanView, return view, err } +func (p *Plans) authorizeDecision(ctx context.Context, plan *Plan, in DecisionCommand) error { + switch in.Kind { + case DecisionConfirm, DecisionConfirmRetry, DecisionContinue: + default: + return nil + } + auth := p.stepAuthorizer() + if auth == nil { + return nil + } + ids, err := selectedStepIDs(plan, in.StepIDs) + if err != nil { + return err + } + byID := map[string]Step{} + for _, s := range plan.Steps { + byID[s.StepID] = s + } + for _, id := range ids { + s := byID[id] + if err := auth.Authorize(ctx, StepExec{ + Principal: in.Principal, + PlanID: plan.PlanID, + StepID: s.StepID, + ActionKey: s.ActionKey, + ActionDigest: s.ActionDigest, + ExpectedResourceVersion: s.ExpectedResourceVersion, + }); err != nil { + return err + } + } + return nil +} + +func selectedStepIDs(plan *Plan, selected []string) ([]string, error) { + byID := map[string]*Step{} + for i := range plan.Steps { + byID[plan.Steps[i].StepID] = &plan.Steps[i] + } + if len(selected) == 0 { + ids := make([]string, 0, len(plan.Steps)) + for i := range plan.Steps { + ids = append(ids, plan.Steps[i].StepID) + } + return ids, nil + } + chosen := map[string]bool{} + for _, id := range selected { + if _, ok := byID[id]; !ok { + return nil, ErrPlanDependencyInvalid + } + chosen[id] = true + } + for id := range chosen { + for _, dep := range byID[id].DependsOn { + depStep, ok := byID[dep] + if !ok || (!chosen[dep] && depStep.Status != StepSucceeded && depStep.Status != StepSkipped) { + return nil, ErrPlanDependencyInvalid + } + } + } + ids := make([]string, 0, len(chosen)) + for id := range chosen { + ids = append(ids, id) + } + return ids, nil +} + +func applyExclude(plan *Plan, stepIDs []string, now time.Time) error { + if len(stepIDs) == 0 { + return ErrPlanDependencyInvalid + } + byID := map[string]*Step{} + for i := range plan.Steps { + byID[plan.Steps[i].StepID] = &plan.Steps[i] + } + for _, id := range stepIDs { + s, ok := byID[id] + if !ok { + return ErrPlanDependencyInvalid + } + if s.Status == StepPending || s.Status == StepEligible { + s.Status = StepCancelled + s.UpdatedAt = now + } + } + return nil +} + +func allStepsTerminal(plan *Plan) bool { + for _, s := range plan.Steps { + switch s.Status { + case StepSucceeded, StepFailed, StepDenied, StepSkipped, StepCancelled: + default: + return false + } + } + return true +} + func applyConfirm(plan *Plan, selected []string, excludeUnselected bool, now time.Time) error { byID := map[string]*Step{} for i := range plan.Steps { @@ -383,7 +527,8 @@ func applyConfirm(plan *Plan, selected []string, excludeUnselected bool, now tim } for id := range chosen { for _, dep := range byID[id].DependsOn { - if !chosen[dep] && byID[dep].Status != StepSucceeded && byID[dep].Status != StepSkipped { + depStep, ok := byID[dep] + if !ok || (!chosen[dep] && depStep.Status != StepSucceeded && depStep.Status != StepSkipped) { return ErrPlanDependencyInvalid } } @@ -462,7 +607,7 @@ func (p *Plans) ExecuteClaimed(ctx context.Context, claimed *ClaimedStep) (*Plan if p.owner == nil { return nil, ErrPlanActionForbidden } - rec, err := p.owner.Execute(ctx, StepExec{ + exec := StepExec{ Principal: claimed.Plan.Principal, PlanID: claimed.Plan.PlanID, StepID: claimed.Step.StepID, @@ -471,7 +616,13 @@ func (p *Plans) ExecuteClaimed(ctx context.Context, claimed *ClaimedStep) (*Plan ActionDigest: claimed.Step.ActionDigest, ExpectedResourceVersion: claimed.Step.ExpectedResourceVersion, FencingToken: claimed.FencingToken, - }) + } + if auth := p.stepAuthorizer(); auth != nil { + if err := auth.Authorize(ctx, exec); err != nil { + return nil, err + } + } + rec, err := p.owner.Execute(ctx, exec) if err != nil { return nil, err }