From 1feefc13e0322c11df5ac2468c10d58e58a6ff8b Mon Sep 17 00:00:00 2001 From: Gaylon Vorwaller Date: Thu, 27 Aug 2026 11:20:08 -0400 Subject: [PATCH 1/2] Fix hosted review policy context --- internal/api/project_routes.go | 6 ++++- internal/serve/context.go | 29 ++++++++++++++++++++ internal/serve/handlers_reviews.go | 8 +----- internal/serve/handlers_reviews_test.go | 35 +++++++++++++++++++++++++ internal/serve/handlers_transitions.go | 29 +++----------------- 5 files changed, 74 insertions(+), 33 deletions(-) diff --git a/internal/api/project_routes.go b/internal/api/project_routes.go index 6c324b6f..52f9f917 100644 --- a/internal/api/project_routes.go +++ b/internal/api/project_routes.go @@ -20,6 +20,7 @@ package api import ( "net/http" + "github.com/marcus/td/internal/reviewpolicy" "github.com/marcus/td/internal/serve" "github.com/marcus/td/internal/serverdb" ) @@ -64,7 +65,10 @@ func (s *Server) wrapServeHandler( DB: liveDB, SessionID: TdWatchSessionFromCtx(r.Context()), BaseDir: "", // td-sync mode: no on-disk td root - Config: serve.HandlerConfig{ + Config: serve.HandlerConfig{ + // Hosted projects have no local feature config. Match td's + // documented default instead of silently reverting to strict. + ReviewPolicyMode: reviewpolicy.ModeTrusted, // NotifyChange intentionally nil: td-sync uses post-commit // action_log -> events.db promotion (Stream 3) instead of the // SSE/autosync notification path used by local `td serve`. diff --git a/internal/serve/context.go b/internal/serve/context.go index 9b3c7d14..8e71a9a5 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" ) @@ -20,6 +22,12 @@ type HandlerConfig struct { TitleMin int TitleMax int + // ReviewPolicyMode supplies the effective review policy when the handler + // has no on-disk td root, as is the case for td-sync project routes. An + // empty value resolves from BaseDir when available, then falls back to td's + // documented default (trusted). + ReviewPolicyMode reviewpolicy.Mode + // NotifyChange, when non-nil, is invoked by write handlers after a // successful mutation. The local `td serve` Server uses this to broadcast // SSE refresh events and trigger debounced autosync. td-sync leaves this @@ -28,6 +36,27 @@ type HandlerConfig struct { NotifyChange func() } +// reviewPolicyModeFor resolves review policy consistently for local td serve +// and remote td-sync handlers. Before remote contexts could carry policy, an +// empty BaseDir silently selected strict mode even though new td projects +// default to trusted; that made session-dependent actions such as Approve +// disappear from td-watch. +func reviewPolicyModeFor(ctx HandlerContext) reviewpolicy.Mode { + if ctx.Config.ReviewPolicyMode != "" { + if mode, err := reviewpolicy.ParseMode(string(ctx.Config.ReviewPolicyMode)); err == nil { + return mode + } + return reviewpolicy.ModeStrict + } + if ctx.BaseDir != "" { + if mode, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { + return mode + } + return reviewpolicy.ModeStrict + } + return reviewpolicy.ModeTrusted +} + // 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..14469df3 100644 --- a/internal/serve/handlers_reviews_test.go +++ b/internal/serve/handlers_reviews_test.go @@ -1076,6 +1076,41 @@ 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_ExplicitStrictOverridesRemoteDefault(t *testing.T) { + ctx := HandlerContext{Config: HandlerConfig{ReviewPolicyMode: reviewpolicy.ModeStrict}} + 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 From 8c4ef76fe33a06b3ccde641abda924f67122c126 Mon Sep 17 00:00:00 2001 From: Gaylon Vorwaller Date: Thu, 27 Aug 2026 11:22:37 -0400 Subject: [PATCH 2/2] Respect hosted review policy overrides --- internal/api/project_routes.go | 6 +---- internal/serve/context.go | 29 ++++++------------------- internal/serve/handlers_reviews_test.go | 5 +++-- 3 files changed, 11 insertions(+), 29 deletions(-) diff --git a/internal/api/project_routes.go b/internal/api/project_routes.go index 52f9f917..6c324b6f 100644 --- a/internal/api/project_routes.go +++ b/internal/api/project_routes.go @@ -20,7 +20,6 @@ package api import ( "net/http" - "github.com/marcus/td/internal/reviewpolicy" "github.com/marcus/td/internal/serve" "github.com/marcus/td/internal/serverdb" ) @@ -65,10 +64,7 @@ func (s *Server) wrapServeHandler( DB: liveDB, SessionID: TdWatchSessionFromCtx(r.Context()), BaseDir: "", // td-sync mode: no on-disk td root - Config: serve.HandlerConfig{ - // Hosted projects have no local feature config. Match td's - // documented default instead of silently reverting to strict. - ReviewPolicyMode: reviewpolicy.ModeTrusted, + Config: serve.HandlerConfig{ // NotifyChange intentionally nil: td-sync uses post-commit // action_log -> events.db promotion (Stream 3) instead of the // SSE/autosync notification path used by local `td serve`. diff --git a/internal/serve/context.go b/internal/serve/context.go index 8e71a9a5..7c411a09 100644 --- a/internal/serve/context.go +++ b/internal/serve/context.go @@ -22,12 +22,6 @@ type HandlerConfig struct { TitleMin int TitleMax int - // ReviewPolicyMode supplies the effective review policy when the handler - // has no on-disk td root, as is the case for td-sync project routes. An - // empty value resolves from BaseDir when available, then falls back to td's - // documented default (trusted). - ReviewPolicyMode reviewpolicy.Mode - // NotifyChange, when non-nil, is invoked by write handlers after a // successful mutation. The local `td serve` Server uses this to broadcast // SSE refresh events and trigger debounced autosync. td-sync leaves this @@ -37,24 +31,15 @@ type HandlerConfig struct { } // reviewPolicyModeFor resolves review policy consistently for local td serve -// and remote td-sync handlers. Before remote contexts could carry policy, an -// empty BaseDir silently selected strict mode even though new td projects -// default to trusted; that made session-dependent actions such as Approve -// disappear from td-watch. +// 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 ctx.Config.ReviewPolicyMode != "" { - if mode, err := reviewpolicy.ParseMode(string(ctx.Config.ReviewPolicyMode)); err == nil { - return mode - } - return reviewpolicy.ModeStrict - } - if ctx.BaseDir != "" { - if mode, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { - return mode - } - return reviewpolicy.ModeStrict + if mode, err := features.ResolveReviewPolicyMode(ctx.BaseDir); err == nil { + return mode } - return reviewpolicy.ModeTrusted + return reviewpolicy.ModeStrict } // HandlerContext is the request-scoped dependency bundle that pure handler diff --git a/internal/serve/handlers_reviews_test.go b/internal/serve/handlers_reviews_test.go index 14469df3..cac5a607 100644 --- a/internal/serve/handlers_reviews_test.go +++ b/internal/serve/handlers_reviews_test.go @@ -1104,8 +1104,9 @@ func TestAvailableTransitions_RemoteContextUsesTrustedDefault(t *testing.T) { t.Fatalf("remote trusted context should offer approve with attestation; transitions=%v", actions) } -func TestReviewPolicyModeFor_ExplicitStrictOverridesRemoteDefault(t *testing.T) { - ctx := HandlerContext{Config: HandlerConfig{ReviewPolicyMode: reviewpolicy.ModeStrict}} +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) }