fix: model orchestrator + cli as first-class scan targets (WO-WZ-023)
- resolveScanRoots recognizes the orchestrator layout (backend/go.mod + frontend/src) so backendDir resolves to backend/ — fixes the go.mod check, classifies the frontend as a standalone app (not a plugin), and routes tsc through the plain typecheck. Clears three orchestrator false-positives. - check 2 (RBAC): only require MethodRoles for proto services a target actually serves (mounted connect handler). The orchestrator carries but does not serve the blockninja.v1 surface (714 false 'missing' -> 0). Exclude cli (definitions-only, vendors a registry client) from check 2. - Skip buf-generated gen/ output in the placeholder check (real proto enum COMING_SOON values are not TODO debt). - Normalize the color allowlist path so theme-card.tsx swatch previews match regardless of scan root. - Whitelist the gitignored, tool-regenerated styles/package-lock.json. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Alex Dunmow
parent
44d60b5f7b
commit
2c7fe9aa62
@ -16,10 +16,25 @@ import (
|
||||
// finding.
|
||||
const coreSDKModulePath = "git.dev.alexdunmow.com/block/core"
|
||||
|
||||
// isDefinitionsOnlySDKTarget reports whether the backend target is the shared
|
||||
// proto SDK (core): it carries proto but serves nothing, so it has neither an
|
||||
// RBAC interceptor nor a designated serving package prefix. Such a target is
|
||||
// out of scope for check 2 — see coreSDKModulePath.
|
||||
// definitionsOnlyModulePaths are Go modules that carry proto service/rpc
|
||||
// definitions but serve NONE of them — RBAC is enforced in the repos that
|
||||
// actually serve the procedures (cms for blockninja.v1, orchestrator for
|
||||
// orchestrator.v1). Scanning them in isolation would flag every procedure as
|
||||
// "missing from MethodRoles", a not-applicable result rather than a finding.
|
||||
//
|
||||
// - core: the shared proto SDK.
|
||||
// - cli: the ninja developer CLI (WO-WZ-023). It vendors
|
||||
// orchestrator/v1/plugin_registry.proto to generate a CLIENT for the
|
||||
// registry; it has no server and no RBAC interceptor.
|
||||
var definitionsOnlyModulePaths = map[string]bool{
|
||||
coreSDKModulePath: true,
|
||||
"git.dev.alexdunmow.com/block/cli": true,
|
||||
}
|
||||
|
||||
// isDefinitionsOnlySDKTarget reports whether the backend target carries proto
|
||||
// but serves nothing, so it has neither an RBAC interceptor nor a designated
|
||||
// serving package prefix. Such a target is out of scope for check 2 — see
|
||||
// definitionsOnlyModulePaths.
|
||||
func isDefinitionsOnlySDKTarget(target backendScanTarget) bool {
|
||||
if len(target.allowedPackagePrefixes) > 0 {
|
||||
return false // a designated serving backend (cms/orchestrator)
|
||||
@ -31,7 +46,7 @@ func isDefinitionsOnlySDKTarget(target backendScanTarget) bool {
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
return modPath == coreSDKModulePath
|
||||
return definitionsOnlyModulePaths[modPath]
|
||||
}
|
||||
|
||||
func init() {
|
||||
@ -43,6 +58,12 @@ func init() {
|
||||
protoMethods := make([]string, 0)
|
||||
rbacMethods := make(map[string]string)
|
||||
protoMethodSet := make(map[string]bool)
|
||||
// protoMethodSetAll holds every declared proto procedure (unfiltered
|
||||
// by served-surface). Stale detection compares RBAC entries against
|
||||
// this full set so an RBAC role for a real-but-unmounted proto method
|
||||
// (e.g. orchestrator.v1.DNSService, declared + roled but not served)
|
||||
// is not mistaken for a dangling entry.
|
||||
protoMethodSetAll := make(map[string]bool)
|
||||
requiresRPCDiscovery := false
|
||||
var rbacSummaries []rbacTargetSummary
|
||||
var definitionsOnlySDKLabels []string
|
||||
@ -70,6 +91,20 @@ func init() {
|
||||
if err != nil {
|
||||
rep.Fatal("failed to parse proto files for %s: %v", target.displayOrRoot(), err)
|
||||
}
|
||||
// Record the FULL declared surface for stale detection, then
|
||||
// scope the missing-RBAC check to the procedures this target
|
||||
// actually SERVES. A serving backend may carry proto for
|
||||
// services it never mounts (for frontend codegen); those are
|
||||
// not real RBAC gaps. See extractServedServiceKeys.
|
||||
summary.protoMethodsAll = append(summary.protoMethodsAll, targetProtoMethods...)
|
||||
for _, pm := range targetProtoMethods {
|
||||
protoMethodSetAll[pm] = true
|
||||
}
|
||||
servedKeys, err := extractServedServiceKeys(target.root)
|
||||
if err != nil {
|
||||
rep.Fatal("failed to scan served handlers for %s: %v", target.displayOrRoot(), err)
|
||||
}
|
||||
targetProtoMethods = filterProceduresToServed(targetProtoMethods, servedKeys)
|
||||
summary.protoMethods = append(summary.protoMethods, targetProtoMethods...)
|
||||
for _, pm := range targetProtoMethods {
|
||||
if protoMethodSet[pm] {
|
||||
@ -99,6 +134,22 @@ func init() {
|
||||
if err != nil {
|
||||
rep.Fatal("failed to parse proto files for %s: %v", target.displayOrRoot(), err)
|
||||
}
|
||||
// Same served-surface scoping as the interceptor branch above:
|
||||
// keep only procedures whose service this target actually mounts.
|
||||
// This is the case the orchestrator hits — its RBAC interceptor
|
||||
// lives under backend/ (so hasCoreRBAC is false at this root),
|
||||
// and it carries the full blockninja.v1 + helpdesk.v1 proto while
|
||||
// serving only orchestrator.v1. The full surface is retained for
|
||||
// stale detection.
|
||||
summary.protoMethodsAll = append(summary.protoMethodsAll, targetProtoMethods...)
|
||||
for _, pm := range targetProtoMethods {
|
||||
protoMethodSetAll[pm] = true
|
||||
}
|
||||
servedKeys, err := extractServedServiceKeys(target.root)
|
||||
if err != nil {
|
||||
rep.Fatal("failed to scan served handlers for %s: %v", target.displayOrRoot(), err)
|
||||
}
|
||||
targetProtoMethods = filterProceduresToServed(targetProtoMethods, servedKeys)
|
||||
summary.protoMethods = append(summary.protoMethods, targetProtoMethods...)
|
||||
for _, pm := range targetProtoMethods {
|
||||
if protoMethodSet[pm] {
|
||||
@ -128,8 +179,12 @@ func init() {
|
||||
if err != nil {
|
||||
rep.Fatal("failed to parse plugin proto files for %s: %v", target.display, err)
|
||||
}
|
||||
// Plugins serve every procedure they declare — no served-surface
|
||||
// filtering. The full and served sets are identical here.
|
||||
summary.protoMethods = append(summary.protoMethods, pluginProtoMethods...)
|
||||
summary.protoMethodsAll = append(summary.protoMethodsAll, pluginProtoMethods...)
|
||||
for _, pm := range pluginProtoMethods {
|
||||
protoMethodSetAll[pm] = true
|
||||
if protoMethodSet[pm] {
|
||||
continue
|
||||
}
|
||||
@ -168,9 +223,13 @@ func init() {
|
||||
}
|
||||
}
|
||||
|
||||
// Stale = an RBAC entry that matches no DECLARED proto method at
|
||||
// all (compare against the full surface, not the served subset —
|
||||
// a role for a real-but-unmounted proto method is intentional, not
|
||||
// dangling).
|
||||
var stale []string
|
||||
for m := range rbacMethods {
|
||||
if !protoMethodSet[m] {
|
||||
if !protoMethodSetAll[m] {
|
||||
if isExcludedServiceMethod(m) {
|
||||
continue
|
||||
}
|
||||
@ -192,7 +251,9 @@ func init() {
|
||||
if isExcludedServiceMethod(method) {
|
||||
continue
|
||||
}
|
||||
if !containsString(summary.protoMethods, method) {
|
||||
// Compare against the full declared surface (see the
|
||||
// global stale note above), not the served subset.
|
||||
if !containsString(summary.protoMethodsAll, method) {
|
||||
summary.staleCount++
|
||||
}
|
||||
}
|
||||
|
||||
15
colors.go
15
colors.go
@ -187,6 +187,21 @@ func init() {
|
||||
}
|
||||
|
||||
func isColorAllowed(relPath string) bool {
|
||||
// The color allowlist keys are source-relative (e.g.
|
||||
// "components/settings/theme-card.tsx"), but relPath is computed relative to
|
||||
// whatever scan root the caller used. That root varies by repo: the CMS
|
||||
// frontend scan root is "web/src" (relPath = "components/..."), while the
|
||||
// orchestrator's is "frontend" (relPath = "src/components/..."). Trim any of
|
||||
// these leading source prefixes so one allowlist entry matches regardless of
|
||||
// which repo is the primary scan target. Mirrors isAllowedPlaceholderFile's
|
||||
// web/src trimming.
|
||||
normalized := filepath.ToSlash(relPath)
|
||||
normalized = strings.TrimPrefix(normalized, "web/src/")
|
||||
normalized = strings.TrimPrefix(normalized, "frontend/src/")
|
||||
normalized = strings.TrimPrefix(normalized, "src/")
|
||||
if colorAllowedFiles[normalized] {
|
||||
return true
|
||||
}
|
||||
if colorAllowedFiles[relPath] {
|
||||
return true
|
||||
}
|
||||
|
||||
@ -40,6 +40,15 @@ var placeholderCopyPatterns = []*regexp.Regexp{
|
||||
var rePlaceholderComment = regexp.MustCompile(`(?i)\bplaceholder\b`)
|
||||
var rePlaceholderStringLiteral = regexp.MustCompile("`[^`]*\\bplaceholder\\b[^`]*`|\"[^\"]*\\bplaceholder\\b[^\"]*\"|'[^']*\\bplaceholder\\b[^']*'")
|
||||
|
||||
// isGeneratedProtoOutput reports whether path lives under a `gen/` directory
|
||||
// segment — the buf codegen output tree (e.g. `.../lib/api/gen/blockninja/v1/`).
|
||||
// Scoped to the placeholder/"ship the feature now" check only: generated proto
|
||||
// code is never hand-authored, so its symbols (COMING_SOON enum values, etc.)
|
||||
// must not be flagged as placeholder debt.
|
||||
func isGeneratedProtoOutput(path string) bool {
|
||||
return strings.Contains(filepath.ToSlash(path), "/gen/")
|
||||
}
|
||||
|
||||
func checkComingSoon(webSrcDir string) []comingSoonViolation {
|
||||
var violations []comingSoonViolation
|
||||
|
||||
@ -61,6 +70,14 @@ func checkComingSoon(webSrcDir string) []comingSoonViolation {
|
||||
if strings.HasSuffix(path, ".test.ts") || strings.HasSuffix(path, ".test.tsx") {
|
||||
return nil
|
||||
}
|
||||
// Skip buf codegen output (any `gen/` directory segment, e.g.
|
||||
// lib/api/gen/blockninja/v1/*_pb.d.ts). These files are regenerated by
|
||||
// buf and must never be hand-edited; their enum values like
|
||||
// SYSTEM_PAGE_TYPE_COMING_SOON / SITE_MODE_COMING_SOON are real proto
|
||||
// symbols, not authored placeholder debt.
|
||||
if isGeneratedProtoOutput(path) {
|
||||
return nil
|
||||
}
|
||||
|
||||
relPath, _ := filepath.Rel(webSrcDir, path)
|
||||
if relPath == "" {
|
||||
@ -141,7 +158,8 @@ func checkPlaceholderLanguage(roots []todoScanRoot) []comingSoonViolation {
|
||||
slashPath := filepath.ToSlash(path)
|
||||
if strings.Contains(slashPath, "/generated/") ||
|
||||
strings.HasSuffix(slashPath, ".pb.go") ||
|
||||
strings.HasSuffix(slashPath, ".connect.go") {
|
||||
strings.HasSuffix(slashPath, ".connect.go") ||
|
||||
isGeneratedProtoOutput(path) {
|
||||
return nil
|
||||
}
|
||||
if strings.Contains(filepath.ToSlash(path), "cmd/check-safety/") {
|
||||
|
||||
@ -213,6 +213,17 @@ type lockfileViolation struct {
|
||||
file string
|
||||
}
|
||||
|
||||
// allowedLockfiles are non-pnpm lockfiles intentionally tolerated, keyed by
|
||||
// repo-relative slash path. These live in gitignored tool working dirs (never
|
||||
// committed) and are regenerated by an external toolchain we don't control, so
|
||||
// they are not part of the repo's dependency surface.
|
||||
var allowedLockfiles = map[string]bool{
|
||||
// cms/styles is a gitignored tailwind-service working dir; its
|
||||
// package-lock.json is regenerated by the tailwind tooling and never
|
||||
// committed (see cms/.gitignore).
|
||||
"styles/package-lock.json": true,
|
||||
}
|
||||
|
||||
func checkLockfiles(repoRoot string) []lockfileViolation {
|
||||
var violations []lockfileViolation
|
||||
|
||||
@ -226,7 +237,11 @@ func checkLockfiles(repoRoot string) []lockfileViolation {
|
||||
base := filepath.Base(path)
|
||||
if base == "package-lock.json" || base == "yarn.lock" {
|
||||
relPath, _ := filepath.Rel(repoRoot, path)
|
||||
violations = append(violations, lockfileViolation{file: relPath})
|
||||
rel := filepath.ToSlash(relPath)
|
||||
if allowedLockfiles[rel] {
|
||||
return nil
|
||||
}
|
||||
violations = append(violations, lockfileViolation{file: rel})
|
||||
}
|
||||
return nil
|
||||
}); err != nil {
|
||||
|
||||
112
proto_rbac.go
112
proto_rbac.go
@ -711,6 +711,118 @@ func normalizeRoleName(raw string) string {
|
||||
return strings.ToLower(raw)
|
||||
}
|
||||
|
||||
// servedHandlerRefPattern matches a PACKAGE-QUALIFIED connect handler
|
||||
// constructor reference, e.g. `orchestratorv1connect.NewSettingsServiceHandler`.
|
||||
//
|
||||
// A connect-go service is served by a repo iff its generated
|
||||
// New{Service}Handler constructor is referenced (called) in that repo's
|
||||
// HAND-WRITTEN backend code — that reference is how the handler gets mounted on
|
||||
// the mux. The generated `*.connect.go` stubs DEFINE that constructor for every
|
||||
// service the repo carries proto for, so a repo that vendors the whole
|
||||
// blockninja.v1 surface only for frontend codegen (the orchestrator) still
|
||||
// SERVES just the subset it actually mounts.
|
||||
//
|
||||
// We keep the package selector (`…connect`) rather than only the short service
|
||||
// name because two proto packages can declare services that share a short name
|
||||
// (`blockninja.v1.SettingsService` vs `orchestrator.v1.SettingsService`); the
|
||||
// selector is what disambiguates which one is really mounted.
|
||||
var servedHandlerRefPattern = regexp.MustCompile(`([A-Za-z0-9_]+connect)\.New([A-Za-z0-9_]+)ServiceHandler\b`)
|
||||
|
||||
// extractServedServiceKeys scans a backend target's HAND-WRITTEN Go source for
|
||||
// package-qualified connect handler mounts and returns the set of served
|
||||
// services, keyed as "<connectPkg>|<ServiceName>" (e.g.
|
||||
// "orchestratorv1connect|SettingsService").
|
||||
//
|
||||
// Generated `*.connect.go` stubs and `_test.go` files are skipped: the stubs
|
||||
// only DEFINE the constructors (unqualified `func New…ServiceHandler`) and would
|
||||
// otherwise mark every carried service as served; tests may reference handlers
|
||||
// for services that aren't mounted in the real server.
|
||||
func extractServedServiceKeys(root string) (map[string]bool, error) {
|
||||
served := make(map[string]bool)
|
||||
err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if info.IsDir() {
|
||||
switch info.Name() {
|
||||
case ".git", ".worktrees", "vendor", "node_modules":
|
||||
return filepath.SkipDir
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if !strings.HasSuffix(path, ".go") {
|
||||
return nil
|
||||
}
|
||||
if strings.HasSuffix(path, ".connect.go") || strings.HasSuffix(path, "_test.go") {
|
||||
return nil
|
||||
}
|
||||
data, err := os.ReadFile(path)
|
||||
if err != nil {
|
||||
return nil // skip unreadable files
|
||||
}
|
||||
for _, m := range servedHandlerRefPattern.FindAllSubmatch(data, -1) {
|
||||
connectPkg := string(m[1])
|
||||
service := string(m[2]) + "Service"
|
||||
served[connectPkg+"|"+service] = true
|
||||
}
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return served, nil
|
||||
}
|
||||
|
||||
// procedureServedKey maps a proto procedure ("/pkg.sub.ServiceName/Method") to
|
||||
// the served-service key produced by extractServedServiceKeys, deriving the
|
||||
// connect package name from the proto package exactly the way
|
||||
// protoc-gen-connect-go does: strip the dots and append "connect"
|
||||
// (blockninja.v1 -> blockninjav1connect, orchestrator.v1 ->
|
||||
// orchestratorv1connect). Returns false for procedures without a package.
|
||||
func procedureServedKey(procedure string) (string, bool) {
|
||||
trimmed := strings.TrimPrefix(procedure, "/")
|
||||
parts := strings.SplitN(trimmed, "/", 2)
|
||||
if len(parts) != 2 || parts[0] == "" {
|
||||
return "", false
|
||||
}
|
||||
serviceFull := parts[0] // e.g. orchestrator.v1.SettingsService
|
||||
lastDot := strings.LastIndex(serviceFull, ".")
|
||||
if lastDot <= 0 || lastDot == len(serviceFull)-1 {
|
||||
return "", false
|
||||
}
|
||||
protoPkg := serviceFull[:lastDot] // orchestrator.v1
|
||||
serviceName := serviceFull[lastDot+1:] // SettingsService
|
||||
connectPkg := strings.ReplaceAll(protoPkg, ".", "") + "connect"
|
||||
return connectPkg + "|" + serviceName, true
|
||||
}
|
||||
|
||||
// filterProceduresToServed keeps only procedures whose service is actually
|
||||
// mounted (served) by the target, per the served-service key set. This is what
|
||||
// scopes check 2 to a repo's real serving surface: a repo may CARRY proto for
|
||||
// procedures it never mounts (the orchestrator carries all of blockninja.v1 and
|
||||
// helpdesk.v1 for frontend codegen but serves only orchestrator.v1), and those
|
||||
// unmounted procedures are not real RBAC gaps.
|
||||
//
|
||||
// If the served set is empty — no recognised handler mounts, e.g. a target that
|
||||
// mounts via a pattern we don't detect — the procedures are returned UNCHANGED
|
||||
// so we never silently hide a genuine missing-RBAC finding.
|
||||
func filterProceduresToServed(procedures []string, served map[string]bool) []string {
|
||||
if len(served) == 0 {
|
||||
return procedures
|
||||
}
|
||||
out := make([]string, 0, len(procedures))
|
||||
for _, p := range procedures {
|
||||
key, ok := procedureServedKey(p)
|
||||
if !ok {
|
||||
continue
|
||||
}
|
||||
if served[key] {
|
||||
out = append(out, p)
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func isExcludedServiceMethod(method string) bool {
|
||||
parts := strings.Split(method, "/")
|
||||
if len(parts) < 3 {
|
||||
|
||||
28
targets.go
28
targets.go
@ -28,12 +28,19 @@ type frontendScanTarget struct {
|
||||
}
|
||||
|
||||
type rbacTargetSummary struct {
|
||||
label string
|
||||
label string
|
||||
// protoMethods is the SERVED procedure surface (filtered to services the
|
||||
// target actually mounts) — it drives the missing-RBAC (permission_denied)
|
||||
// finding.
|
||||
protoMethods []string
|
||||
roleByMethod map[string]string
|
||||
missingCount int
|
||||
staleCount int
|
||||
requiresRPCs bool
|
||||
// protoMethodsAll is the FULL declared procedure surface (unfiltered) — it
|
||||
// drives stale-entry detection, so an RBAC role for a real-but-unmounted
|
||||
// proto method is not reported as stale.
|
||||
protoMethodsAll []string
|
||||
roleByMethod map[string]string
|
||||
missingCount int
|
||||
staleCount int
|
||||
requiresRPCs bool
|
||||
}
|
||||
|
||||
func (t backendScanTarget) displayOrRoot() string {
|
||||
@ -62,6 +69,17 @@ func resolveScanRoots(target string) (backendDir string, repoRoot string, err er
|
||||
return filepath.Join(absTarget, "backend"), absTarget, nil
|
||||
}
|
||||
|
||||
// Orchestrator layout — same standalone-app shape as the CMS above, but the
|
||||
// frontend lives at frontend/src (not web/src) and proto/buf sit at the repo
|
||||
// root. Without this clause the orchestrator root resolves as its own backend
|
||||
// (backendDir == repoRoot): backend/go.mod is reported missing, and
|
||||
// frontend/src is treated as a plugin frontend so the plugin-only rules fire
|
||||
// on a first-class app.
|
||||
if fileExists(filepath.Join(absTarget, "backend", "go.mod")) && dirExists(filepath.Join(absTarget, "frontend", "src")) &&
|
||||
(fileExists(filepath.Join(absTarget, "buf.yaml")) || dirExists(filepath.Join(absTarget, "proto"))) {
|
||||
return filepath.Join(absTarget, "backend"), absTarget, nil
|
||||
}
|
||||
|
||||
if filepath.Base(absTarget) == "backend" && fileExists(filepath.Join(absTarget, "go.mod")) {
|
||||
parent := filepath.Clean(filepath.Join(absTarget, ".."))
|
||||
if fileExists(filepath.Join(parent, "buf.yaml")) || dirExists(filepath.Join(parent, "proto")) {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user