Auto approve access request to Hellosource and Helloaggregate

.cursor/skills/create-naysayer-rule/SKILL.md

@@ -0,0 +1,160 @@
+---
+name: create-naysayer-rule
+description: >-
+  Authors new Naysayer validation rules in Go following existing patterns:
+  shared.Rule interface, registry registration, rules.yaml section wiring,
+  unit tests, and e2e scenarios. Use when adding a validation rule, extending
+  auto-approval, wiring rules.yaml, or implementing GetCoveredLines/ValidateLines.
+---
+
+# Create Naysayer Rule
+
+## Before starting
+
+1. Read [docs/RULE_CREATION_GUIDE.md](docs/RULE_CREATION_GUIDE.md) for full detail.
+2. Find the closest existing rule and mirror its structure (see archetypes below).
+3. Confirm the rule name ends with `_rule` and matches registry + `rules.yaml` exactly.
+
+## Choose an archetype
+
+| Need | Copy from | Package layout |
+|------|-----------|----------------|
+| Auto-approve safe/metadata files | `internal/rules/common/metadata_rule.go` | `common` or embed `common.NewBaseRule` |
+| Single-file, path/pattern checks | `internal/rules/service_account_rule.go` | `internal/rules/<name>_rule.go` |
+| Section + MR context (warehouses, consumers) | `internal/rules/warehouse/`, `internal/rules/dataproduct_consumer/` | `internal/rules/<name>/` with `rule.go`, optional `types.go`, `validator.go` |
+| Full-file YAML CR validation | `internal/rules/tag/`, `internal/rules/masking/` | Subpackage + `Validator`, `SetMRContext` |
+
+**Default for non-trivial logic:** subdirectory under `internal/rules/<name>/`.
+
+## Required interface
+
+Every rule implements `shared.Rule` in [internal/rules/shared/types.go](internal/rules/shared/types.go):
+
+- `Name() string` — stable ID, e.g. `tag_rule`
+- `Description() string`
+- `GetCoveredLines(filePath, fileContent string) []LineRange` — return `nil` if rule does not apply
+- `ValidateLines(filePath, fileContent string, lineRanges []LineRange) (DecisionType, string)` — `shared.Approve` or `shared.ManualReview`
+
+Optional: `ContextAwareRule` with `SetMRContext(*MRContext)` when the rule needs other MR files (see `tag`, `codeowners`, `warehouse`).
+
+Embed `common.BaseRule` via `common.NewBaseRule(name, description)` for name/description and `GetFullFileCoverage()`.
+
+## Implementation checklist
+
+```
+- [ ] 1. Implement rule (correct archetype)
+- [ ] 2. Unit tests: table-driven `ValidateLines` + `GetCoveredLines` edge cases
+- [ ] 3. Register in internal/rules/registry.go → registerBuiltInRules()
+- [ ] 4. Wire rules.yaml (see below)
+- [ ] 5. If rule-specific config: internal/config/types.go + factory reads config.Load()
+- [ ] 6. Optional: docs/rules/<RULE_NAME>.md
+- [ ] 7. E2E scenario under e2e/testdata/scenarios/
+- [ ] 8. go test ./internal/rules/... && make test-e2e (or targeted -run)
+```
+
+## Register the rule
+
+In [internal/rules/registry.go](internal/rules/registry.go), add to `registerBuiltInRules()`:
+
+```go
+_ = r.RegisterRule(&RuleInfo{
+    Name:        "my_rule",
+    Description: "Human-readable description",
+    Version:     "1.0.0",
+    Factory: func(client gitlab.GitLabClient) shared.Rule {
+        return mypackage.NewRule(client) // or NewRule(cfg) if config-driven
+    },
+    Enabled:  true,
+    Category: "validation", // match siblings: warehouse, masking, auto_approval, etc.
+})
+```
+
+Name must be unique; grep the repo for collisions before registering.
+
+## Wire rules.yaml
+
+Use **`rule_configs`** (not `rule_names`). Shape from [rules.yaml](rules.yaml) and [internal/config/sections.go](internal/config/sections.go):
+
+```yaml
+files:
+  - name: "my_file_type"
+    path: "dataproducts/**/"
+    filename: "*.{yaml,yml}"
+    parser_type: yaml
+    enabled: true
+    sections:
+      - name: my_section
+        yaml_path: .          # or dotted path, e.g. warehouses
+        rule_configs:
+          - name: my_rule
+            enabled: true
+        auto_approve: false   # true only when safe to auto-approve on pass
+```
+
+**Strict policy:** Any changed file/line not covered by an enabled section → manual review. New file patterns need an explicit `files:` entry; do not rely on implicit coverage.
+
+When enabling a new file type in production `rules.yaml`, set `enabled: true` on the file block and add matching e2e coverage.
+
+## Patterns to follow from existing rules
+
+### GetCoveredLines
+
+- Return `nil` when the rule does not apply to the path/content.
+- Full-file rules: use `common.BaseRule.GetFullFileCoverage` or count lines like `tag_rule`.
+- Deleted files: still return a minimal range so `ValidateLines` runs (see `tag/rule.go`).
+
+### ValidateLines
+
+- Early exit: `return shared.Approve, "Not a <x> file"` when non-applicable.
+- Deletions / security-sensitive ops → `ManualReview` with a clear reason.
+- Parse failures → `ManualReview`, not panic.
+- Approve messages should be specific enough for MR comments.
+
+### Config-driven rules
+
+Examples: `toc_approval_rule`, `dataproduct_consumer_rule` — read `config.Load()` in the registry `Factory`, not inside `ValidateLines` on every call.
+
+## Tests
+
+**Unit:** `internal/rules/<pkg>/rule_test.go` — table tests for approve vs manual_review; test name/description constants.
+
+**E2E:** [e2e/README.md](e2e/README.md) — `e2e/testdata/scenarios/<NN>_<name>/` with `before/`, `after/`, `scenario.yaml`:
+
+```yaml
+name: "my_scenario"
+description: "..."
+expected:
+  decision: "approve"   # or manual_review
+  approved: true        # or false
+  comment_contains:
+    - "expected substring"
+```
+
+Enable the rule on the file type in `rules.yaml` (or a test-only override in `e2e/rules.yaml` if the scenario needs it).
+
+## Verify
+
+```bash
+go test ./internal/rules/<pkg> -v
+go test ./internal/rules/... -v
+go test ./e2e -v -run TestE2E_Scenarios/<scenario_name>
+```
+
+## Anti-patterns
+
+- Do not implement legacy `Applies` / `ShouldApprove` (outdated; section manager uses `GetCoveredLines` / `ValidateLines`).
+- Do not use `rule_names` in YAML — use `rule_configs` with `name` + `enabled`.
+- Do not register without `rules.yaml` wiring — rule will never run on MRs.
+- Do not auto-approve destructive changes (deletes, privilege grants) without matching existing rules' conservatism.
+
+## Reference map
+
+| Topic | Location |
+|-------|----------|
+| Full guide | docs/RULE_CREATION_GUIDE.md |
+| Rule docs | docs/rules/*.md |
+| Interface | internal/rules/shared/types.go |
+| Base helpers | internal/rules/common/base.go |
+| Config schema | internal/config/sections.go |
+| Production wiring | rules.yaml |
+| E2E | e2e/README.md, e2e/testdata/scenarios/ |

e2e/hello_access_request_test.go

@@ -0,0 +1,33 @@
+package e2e
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// helloAccessRequestScenarios are E2E cases for the hello_access_request rule.
+var helloAccessRequestScenarios = []string{
+	"42_access_request_hellosource",
+	"43_access_request_helloaggregate",
+	"44_access_request_multi_hellosource",
+	"45_access_request_multi_cross_dp",
+	"46_access_request_name_mismatch",
+	"47_access_request_data_product_mismatch",
+	"48_access_request_with_uncovered_file",
+	"49_access_request_deletion",
+	"50_access_request_wrong_path",
+}
+
+// TestE2E_HelloAccessRequest runs all hello_access_request E2E scenarios in isolation.
+func TestE2E_HelloAccessRequest(t *testing.T) {
+	for _, dir := range helloAccessRequestScenarios {
+		t.Run(dir, func(t *testing.T) {
+			scenarioDir := filepath.Join("testdata", "scenarios", dir)
+			scenario, err := LoadScenario(scenarioDir)
+			require.NoError(t, err, "load scenario %s", dir)
+			runScenario(t, *scenario)
+		})
+	}
+}

e2e/testdata/scenarios/42_access_request_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: hellosource

e2e/testdata/scenarios/42_access_request_hellosource/scenario.yaml

@@ -0,0 +1,14 @@
+name: "hello_access_request_hellosource"
+description: "Valid hellosource access-request file should be auto-approved"
+
+expected:
+  decision: Approve
+  approved: true
+  comment_contains:
+    - "Auto-approved"
+
+mr_metadata:
+  title: "Add hellosource access request for mbramle"
+  author: "testuser"
+  source_branch: "feature/access-request-mbramle"
+  target_branch: "main"

e2e/testdata/scenarios/43_access_request_helloaggregate/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml

@@ -0,0 +1,2 @@
+name: tvaldez
+data_product: helloaggregate

e2e/testdata/scenarios/43_access_request_helloaggregate/scenario.yaml

@@ -0,0 +1,14 @@
+name: "hello_access_request_helloaggregate"
+description: "Valid helloaggregate access-request file should be auto-approved"
+
+expected:
+  decision: Approve
+  approved: true
+  comment_contains:
+    - "Auto-approved"
+
+mr_metadata:
+  title: "Add helloaggregate access request for tvaldez"
+  author: "testuser"
+  source_branch: "feature/access-request-tvaldez"
+  target_branch: "main"

e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/jkimura.yaml

@@ -0,0 +1,2 @@
+name: jkimura
+data_product: hellosource

e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: hellosource

e2e/testdata/scenarios/44_access_request_multi_hellosource/scenario.yaml

@@ -0,0 +1,14 @@
+name: "hello_access_request_multi_hellosource"
+description: "Multiple valid hellosource access-request files in one MR should be auto-approved"
+
+expected:
+  decision: Approve
+  approved: true
+  comment_contains:
+    - "Auto-approved"
+
+mr_metadata:
+  title: "Add hellosource access requests for mbramle and jkimura"
+  author: "testuser"
+  source_branch: "feature/access-requests-multi-hellosource"
+  target_branch: "main"

e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml

@@ -0,0 +1,2 @@
+name: tvaldez
+data_product: helloaggregate

e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: hellosource

e2e/testdata/scenarios/45_access_request_multi_cross_dp/scenario.yaml

@@ -0,0 +1,14 @@
+name: "hello_access_request_multi_cross_dp"
+description: "Valid access-request files for helloaggregate and hellosource in one MR should be auto-approved"
+
+expected:
+  decision: Approve
+  approved: true
+  comment_contains:
+    - "Auto-approved"
+
+mr_metadata:
+  title: "Add cross data product access requests"
+  author: "testuser"
+  source_branch: "feature/access-requests-cross-dp"
+  target_branch: "main"

e2e/testdata/scenarios/46_access_request_name_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: wrongname
+data_product: hellosource

e2e/testdata/scenarios/46_access_request_name_mismatch/scenario.yaml

@@ -0,0 +1,15 @@
+name: "hello_access_request_name_mismatch"
+description: "Access-request with name field not matching filename requires manual review"
+
+expected:
+  decision: ManualReview
+  approved: false
+  comment_contains:
+    - "Manual review required"
+    - "does not match filename"
+
+mr_metadata:
+  title: "Add hellosource access request with invalid name"
+  author: "testuser"
+  source_branch: "feature/access-request-name-mismatch"
+  target_branch: "main"

e2e/testdata/scenarios/47_access_request_data_product_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: helloaggregate

e2e/testdata/scenarios/47_access_request_data_product_mismatch/scenario.yaml

@@ -0,0 +1,15 @@
+name: "hello_access_request_data_product_mismatch"
+description: "Access-request with data_product not matching path requires manual review"
+
+expected:
+  decision: ManualReview
+  approved: false
+  comment_contains:
+    - "Manual review required"
+    - "data_product"
+
+mr_metadata:
+  title: "Add hellosource access request with wrong data_product"
+  author: "testuser"
+  source_branch: "feature/access-request-dp-mismatch"
+  target_branch: "main"

e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: hellosource

e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/misc/uncovered.yaml

@@ -0,0 +1 @@
+foo: bar

e2e/testdata/scenarios/48_access_request_with_uncovered_file/scenario.yaml

@@ -0,0 +1,15 @@
+name: "hello_access_request_with_uncovered_file"
+description: "MR with valid access-request plus an uncovered file requires manual review"
+
+expected:
+  decision: ManualReview
+  reason: "Uncovered changes require manual review"
+  approved: false
+  comment_contains:
+    - "Manual review required"
+
+mr_metadata:
+  title: "Add access request with unrelated config file"
+  author: "testuser"
+  source_branch: "feature/access-request-plus-uncovered"
+  target_branch: "main"

e2e/testdata/scenarios/49_access_request_deletion/before/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: hellosource

e2e/testdata/scenarios/49_access_request_deletion/scenario.yaml

@@ -0,0 +1,14 @@
+name: "hello_access_request_deletion"
+description: "Deleting an access-request file requires manual review"
+
+expected:
+  decision: ManualReview
+  approved: false
+  comment_contains:
+    - "Manual review required"
+
+mr_metadata:
+  title: "Remove hellosource access request for mbramle"
+  author: "testuser"
+  source_branch: "feature/remove-access-request-mbramle"
+  target_branch: "main"

e2e/testdata/scenarios/50_access_request_wrong_path/after/dataproducts/source/hellosource/access-requests/groups/other-group/mbramle.yaml

@@ -0,0 +1,2 @@
+name: mbramle
+data_product: hellosource

e2e/testdata/scenarios/50_access_request_wrong_path/scenario.yaml

@@ -0,0 +1,15 @@
+name: "hello_access_request_wrong_path"
+description: "Access-request file outside allowed group path requires manual review"
+
+expected:
+  decision: ManualReview
+  reason: "Uncovered changes require manual review"
+  approved: false
+  comment_contains:
+    - "Manual review required"
+
+mr_metadata:
+  title: "Add access request under wrong group path"
+  author: "testuser"
+  source_branch: "feature/access-request-wrong-path"
+  target_branch: "main"