diff --git a/internal/serve/context.go b/internal/serve/context.go index 9b3c7d14..7c411a09 100644 --- a/internal/serve/context.go +++ b/internal/serve/context.go @@ -3,6 +3,8 @@ package serve import ( "github.com/marcus/td/internal/config" "github.com/marcus/td/internal/db" + "github.com/marcus/td/internal/features" + "github.com/marcus/td/internal/reviewpolicy" "github.com/marcus/td/internal/workdir" ) @@ -28,6 +30,18 @@ type HandlerConfig struct { NotifyChange func() } +// reviewPolicyModeFor resolves review policy consistently for local td serve +// and remote td-sync handlers. The resolver accepts an empty BaseDir: it still +// honors process-wide environment overrides and otherwise returns td's trusted +// default. Skipping it for remote contexts silently selected strict mode and +// made session-dependent actions such as Approve disappear from td-watch. +func reviewPolicyModeFor(ctx HandlerContext) reviewpolicy.Mode { + if mode, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { + return mode + } + return reviewpolicy.ModeStrict +} + // HandlerContext is the request-scoped dependency bundle that pure handler // functions receive in place of a `*Server` receiver. It lets td-sync mount the // same handlers against per-project DBs without standing up a `*serve.Server`. diff --git a/internal/serve/handlers_reviews.go b/internal/serve/handlers_reviews.go index 699756a8..ee7ff504 100644 --- a/internal/serve/handlers_reviews.go +++ b/internal/serve/handlers_reviews.go @@ -11,7 +11,6 @@ import ( "unicode/utf8" "github.com/marcus/td/internal/db" - "github.com/marcus/td/internal/features" "github.com/marcus/td/internal/models" "github.com/marcus/td/internal/reviewpolicy" ) @@ -153,12 +152,7 @@ func HandleRecordReview(ctx HandlerContext, w http.ResponseWriter, r *http.Reque return } - mode := reviewpolicy.ModeStrict - if ctx.BaseDir != "" { - if m, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { - mode = m - } - } + mode := reviewPolicyModeFor(ctx) // Delegated and trusted both support recording an approval without // closing. Trusted is delegated plus the self-review escape hatch, so the // close-using-recorded-approval path accepts these rows in both modes (see diff --git a/internal/serve/handlers_reviews_test.go b/internal/serve/handlers_reviews_test.go index d93f91b9..cac5a607 100644 --- a/internal/serve/handlers_reviews_test.go +++ b/internal/serve/handlers_reviews_test.go @@ -1076,6 +1076,42 @@ func TestIntegration_CanApprove_AttributionUnlocks(t *testing.T) { } } +// Hosted td-sync handlers have no local BaseDir from which to read feature +// flags. They must still use td's trusted default so an involved browser +// session can reach Approve and provide the required review attestation. +func TestAvailableTransitions_RemoteContextUsesTrustedDefault(t *testing.T) { + _, database, cleanup := setupIntegrationServer(t) + defer cleanup() + + sessionID := "twu_mobile" + issueID := seedInReviewIssue(t, database, sessionID) + issue, err := database.GetIssue(issueID) + if err != nil { + t.Fatal(err) + } + + actions := availableTransitionsFor(HandlerContext{ + DB: database, + SessionID: sessionID, + BaseDir: "", + }, issue) + + for _, action := range actions { + if action == "approve" { + return + } + } + t.Fatalf("remote trusted context should offer approve with attestation; transitions=%v", actions) +} + +func TestReviewPolicyModeFor_RemoteEnvStrictOverridesDefault(t *testing.T) { + t.Setenv("TD_FEATURE_REVIEW_POLICY_MODE", "strict") + ctx := HandlerContext{BaseDir: ""} + if got := reviewPolicyModeFor(ctx); got != reviewpolicy.ModeStrict { + t.Fatalf("mode=%q want strict", got) + } +} + // TestIntegration_Approve_AuditParityWithCLI pins the audit behavior decided // for this epic, on the API side. The rule is about WHO RECORDED the row, not // which flag was used: an approval written by an implementation-involved diff --git a/internal/serve/handlers_transitions.go b/internal/serve/handlers_transitions.go index 4162e54d..86495212 100644 --- a/internal/serve/handlers_transitions.go +++ b/internal/serve/handlers_transitions.go @@ -12,7 +12,6 @@ import ( "unicode/utf8" "github.com/marcus/td/internal/db" - "github.com/marcus/td/internal/features" "github.com/marcus/td/internal/models" "github.com/marcus/td/internal/reviewpolicy" "github.com/marcus/td/internal/workflow" @@ -115,12 +114,7 @@ func serveReviewerDecision(ctx HandlerContext, issue *models.Issue, selfReviewAc // --reviewed-by attestation. Kept as a separate entry point so the many // existing callers that never attribute stay unchanged. func serveReviewerDecisionAttributed(ctx HandlerContext, issue *models.Issue, selfReviewAcknowledged bool, attributedTo string) reviewpolicy.ReviewerEligibility { - mode := reviewpolicy.ModeStrict - if ctx.BaseDir != "" { - if m, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { - mode = m - } - } + mode := reviewPolicyModeFor(ctx) isCreator := issue != nil && issue.CreatorSession != "" && issue.CreatorSession == ctx.SessionID isImplementer := issue != nil && issue.ImplementerSession != "" && issue.ImplementerSession == ctx.SessionID @@ -168,12 +162,7 @@ func serveCloseDecision(ctx HandlerContext, issue *models.Issue, selfReviewAckno // path, close eligibility re-runs the trusted reviewer predicate, so dropping // the attribution here rejects an approval the reviewer check just allowed. func serveCloseDecisionAttributed(ctx HandlerContext, issue *models.Issue, selfReviewAcknowledged bool, attributedTo string) reviewpolicy.CloseEligibility { - mode := reviewpolicy.ModeStrict - if ctx.BaseDir != "" { - if m, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { - mode = m - } - } + mode := reviewPolicyModeFor(ctx) isCreator := issue != nil && issue.CreatorSession != "" && issue.CreatorSession == ctx.SessionID isImplementer := issue != nil && issue.ImplementerSession != "" && issue.ImplementerSession == ctx.SessionID @@ -243,12 +232,7 @@ func canApprove(ctx HandlerContext, issue *models.Issue) bool { // Mode-C (delegated): an active approval lets an eligible closer finish the // close even when they are not the reviewer of record. Mirrors // handledCloseAfterReview. - mode := reviewpolicy.ModeStrict - if ctx.BaseDir != "" { - if m, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { - mode = m - } - } + mode := reviewPolicyModeFor(ctx) if mode == reviewpolicy.ModeDelegated || mode == reviewpolicy.ModeTrusted { if active, err := ctx.DB.GetActiveApprovalReview(issue.ID); err == nil && active != nil { if serveCloseDecision(ctx, issue, false).Allowed { @@ -810,12 +794,7 @@ func handledCloseAfterReview(ctx HandlerContext, w http.ResponseWriter, r *http. if issue.Status != models.StatusInReview { return false } - mode := reviewpolicy.ModeStrict - if ctx.BaseDir != "" { - if m, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { - mode = m - } - } + mode := reviewPolicyModeFor(ctx) // Delegated and trusted both close on a recorded approval. Trusted is // delegated plus the self-review escape hatch (reviewpolicy.evaluateCloseTrusted // Case 1 is identical to delegated's), so withholding this path left the