fix(hooks): improve agent-scoped hook error messaging#1224
Conversation
edf0a56 to
efe7155
Compare
…rror messaging for agent-scoped hooks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
efe7155 to
9fce99a
Compare
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: This PR is the stronger successor to #1221. It preserves tenant isolation for agent-scoped hooks while improving the backend/frontend path so users get a clearer recovery path instead of the raw sentinel/tenant_id failure.
Risk level: Low
Mandatory gates:
- Duplicate/prior implementation: overlap found with #1221, but this PR is broader and cleaner.
- Project standards: passed; changes stay in the existing hooks gateway/config/UI schema/form surfaces.
- Strategic necessity: clear value; reduces support friction around agent-scoped hook creation without weakening tenant isolation.
- CI/checks: green.
Findings:
- Critical: none.
- Important: none blocking merge.
- Suggestion: after this merges, close #1221 as superseded to avoid two parallel fixes for the same hook tenant_id path.
Verdict: APPROVE
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: Thanks for narrowing this down, but I cannot approve this as-is because it bakes in the wrong product rule: master/system-tenant agent hooks are treated as invalid, while the current store/scope model explicitly supports master/global operation and falls back to the master tenant in multiple paths.nnRisk level: MediumnnMandatory gates:n- Duplicate/prior implementation: overlap found with the closed precursor #1221; this PR is the successor but changes the behavior assumption rather than just fixing context-fill.n- Project standards: issue found — the validation/message contradicts the existing hook scope model.n- Strategic necessity: not justified as written; it would force single-tenant/system users to create a workspace tenant just to use agent hooks.n- CI/checks: greennnFindings:n- Important: internal/hooks/config.go still treats hooks.SentinelTenantID / store.MasterTenantID as invalid for ScopeAgent, and this PR changes the error to say agent-scoped hooks cannot be created in the master/system tenant. That conflicts with the existing tenant model: internal/hooks/types.go documents the sentinel as store.MasterTenantID, internal/store/scope.go falls back to MasterTenantID, and the hook stores intentionally expose tenant-specific + global rows to non-master callers while master scope can operate server-wide. The related discussion in #1241 also needs this clarified before merging.n- Important: internal/gateway/methods/hooks.go replaces a sentinel tenant id with store.TenantIDFromContext(ctx). In a master/system context that can still be MasterTenantID, so the UI/backend path can still hit the validation error; the new message just turns the bug into an intentional limitation. That does not solve the reported single-tenant/system-user case from #1221.nnNext step: either (1) explicitly support master/system-tenant agent hooks by allowing ScopeAgent with MasterTenantID when the request is master-scope and add tests for that path, or (2) document a product decision that system users must create a workspace tenant and update the issue/UX accordingly. Without that decision and tests, this should not merge.
Summary
Agent-scoped hooks cannot be created in the master/system tenant by design (for tenant isolation). This PR improves the error messaging to clearly explain this requirement.
Changes
tenant_idfor both tenant and agent scopestenant_idfrom auth context for agent scopetenant_idschema validationDesign Discussion
This PR assumes that agent-scoped hooks should NOT be allowed in the master/system tenant (by design, for tenant isolation). Is this the intended behavior?
🤖 Generated with Claude Code