From d71fd8469d3a18cabb7c95d52a8bc149618c4305 Mon Sep 17 00:00:00 2001 From: Bibhas Debnath Date: Thu, 21 May 2026 14:04:38 +0530 Subject: [PATCH 1/3] Auto approve access request to Hellosource and Helloaggregate Signed-off-by: Bibhas Debnath --- .cursor/skills/create-naysayer-rule/SKILL.md | 160 +++++++++++++++++ e2e/hello_access_request_test.go | 33 ++++ .../dataverse-source-hellosource/mbramle.yaml | 2 + .../scenario.yaml | 14 ++ .../tvaldez.yaml | 2 + .../scenario.yaml | 14 ++ .../dataverse-source-hellosource/jkimura.yaml | 2 + .../dataverse-source-hellosource/mbramle.yaml | 2 + .../scenario.yaml | 14 ++ .../tvaldez.yaml | 2 + .../dataverse-source-hellosource/mbramle.yaml | 2 + .../scenario.yaml | 14 ++ .../dataverse-source-hellosource/mbramle.yaml | 2 + .../scenario.yaml | 15 ++ .../dataverse-source-hellosource/mbramle.yaml | 2 + .../scenario.yaml | 15 ++ .../dataverse-source-hellosource/mbramle.yaml | 2 + .../source/hellosource/misc/uncovered.yaml | 1 + .../scenario.yaml | 15 ++ .../dataverse-source-hellosource/mbramle.yaml | 2 + .../49_access_request_deletion/scenario.yaml | 14 ++ .../groups/other-group/mbramle.yaml | 2 + .../scenario.yaml | 15 ++ internal/rules/access_request/rule.go | 164 +++++++++++++++++ internal/rules/access_request/rule_test.go | 170 ++++++++++++++++++ internal/rules/registry.go | 13 ++ rules.yaml | 27 +++ 27 files changed, 720 insertions(+) create mode 100644 .cursor/skills/create-naysayer-rule/SKILL.md create mode 100644 e2e/hello_access_request_test.go create mode 100644 e2e/testdata/scenarios/42_access_request_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/42_access_request_hellosource/scenario.yaml create mode 100644 e2e/testdata/scenarios/43_access_request_helloaggregate/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml create mode 100644 e2e/testdata/scenarios/43_access_request_helloaggregate/scenario.yaml create mode 100644 e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/jkimura.yaml create mode 100644 e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/44_access_request_multi_hellosource/scenario.yaml create mode 100644 e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml create mode 100644 e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/45_access_request_multi_cross_dp/scenario.yaml create mode 100644 e2e/testdata/scenarios/46_access_request_name_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/46_access_request_name_mismatch/scenario.yaml create mode 100644 e2e/testdata/scenarios/47_access_request_data_product_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/47_access_request_data_product_mismatch/scenario.yaml create mode 100644 e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/misc/uncovered.yaml create mode 100644 e2e/testdata/scenarios/48_access_request_with_uncovered_file/scenario.yaml create mode 100644 e2e/testdata/scenarios/49_access_request_deletion/before/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml create mode 100644 e2e/testdata/scenarios/49_access_request_deletion/scenario.yaml create mode 100644 e2e/testdata/scenarios/50_access_request_wrong_path/after/dataproducts/source/hellosource/access-requests/groups/other-group/mbramle.yaml create mode 100644 e2e/testdata/scenarios/50_access_request_wrong_path/scenario.yaml create mode 100644 internal/rules/access_request/rule.go create mode 100644 internal/rules/access_request/rule_test.go diff --git a/.cursor/skills/create-naysayer-rule/SKILL.md b/.cursor/skills/create-naysayer-rule/SKILL.md new file mode 100644 index 0000000..768089e --- /dev/null +++ b/.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/_rule.go` | +| Section + MR context (warehouses, consumers) | `internal/rules/warehouse/`, `internal/rules/dataproduct_consumer/` | `internal/rules//` 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//`. + +## 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/.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 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//rule_test.go` — table tests for approve vs manual_review; test name/description constants. + +**E2E:** [e2e/README.md](e2e/README.md) — `e2e/testdata/scenarios/_/` 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/ -v +go test ./internal/rules/... -v +go test ./e2e -v -run TestE2E_Scenarios/ +``` + +## 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/ | diff --git a/e2e/hello_access_request_test.go b/e2e/hello_access_request_test.go new file mode 100644 index 0000000..0a87ac2 --- /dev/null +++ b/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) + }) + } +} diff --git a/e2e/testdata/scenarios/42_access_request_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/42_access_request_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..db8448e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/42_access_request_hellosource/scenario.yaml b/e2e/testdata/scenarios/42_access_request_hellosource/scenario.yaml new file mode 100644 index 0000000..6f4fecb --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/43_access_request_helloaggregate/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml b/e2e/testdata/scenarios/43_access_request_helloaggregate/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml new file mode 100644 index 0000000..02f91e2 --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/43_access_request_helloaggregate/scenario.yaml b/e2e/testdata/scenarios/43_access_request_helloaggregate/scenario.yaml new file mode 100644 index 0000000..0df9164 --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/jkimura.yaml b/e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/jkimura.yaml new file mode 100644 index 0000000..f03d6e2 --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/44_access_request_multi_hellosource/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..db8448e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/44_access_request_multi_hellosource/scenario.yaml b/e2e/testdata/scenarios/44_access_request_multi_hellosource/scenario.yaml new file mode 100644 index 0000000..e7e2e3b --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml b/e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/tvaldez.yaml new file mode 100644 index 0000000..02f91e2 --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/45_access_request_multi_cross_dp/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..db8448e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/45_access_request_multi_cross_dp/scenario.yaml b/e2e/testdata/scenarios/45_access_request_multi_cross_dp/scenario.yaml new file mode 100644 index 0000000..789f28c --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/46_access_request_name_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/46_access_request_name_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..4f48f07 --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/46_access_request_name_mismatch/scenario.yaml b/e2e/testdata/scenarios/46_access_request_name_mismatch/scenario.yaml new file mode 100644 index 0000000..ad2b735 --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/47_access_request_data_product_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/47_access_request_data_product_mismatch/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..1d5de6e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/47_access_request_data_product_mismatch/scenario.yaml b/e2e/testdata/scenarios/47_access_request_data_product_mismatch/scenario.yaml new file mode 100644 index 0000000..2a1389a --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..db8448e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/misc/uncovered.yaml b/e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/misc/uncovered.yaml new file mode 100644 index 0000000..20e9ff3 --- /dev/null +++ b/e2e/testdata/scenarios/48_access_request_with_uncovered_file/after/dataproducts/source/hellosource/misc/uncovered.yaml @@ -0,0 +1 @@ +foo: bar diff --git a/e2e/testdata/scenarios/48_access_request_with_uncovered_file/scenario.yaml b/e2e/testdata/scenarios/48_access_request_with_uncovered_file/scenario.yaml new file mode 100644 index 0000000..5c1f339 --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/49_access_request_deletion/before/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml b/e2e/testdata/scenarios/49_access_request_deletion/before/dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/mbramle.yaml new file mode 100644 index 0000000..db8448e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/49_access_request_deletion/scenario.yaml b/e2e/testdata/scenarios/49_access_request_deletion/scenario.yaml new file mode 100644 index 0000000..9f75146 --- /dev/null +++ b/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" diff --git a/e2e/testdata/scenarios/50_access_request_wrong_path/after/dataproducts/source/hellosource/access-requests/groups/other-group/mbramle.yaml b/e2e/testdata/scenarios/50_access_request_wrong_path/after/dataproducts/source/hellosource/access-requests/groups/other-group/mbramle.yaml new file mode 100644 index 0000000..db8448e --- /dev/null +++ b/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 diff --git a/e2e/testdata/scenarios/50_access_request_wrong_path/scenario.yaml b/e2e/testdata/scenarios/50_access_request_wrong_path/scenario.yaml new file mode 100644 index 0000000..bf37087 --- /dev/null +++ b/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" diff --git a/internal/rules/access_request/rule.go b/internal/rules/access_request/rule.go new file mode 100644 index 0000000..becde5e --- /dev/null +++ b/internal/rules/access_request/rule.go @@ -0,0 +1,164 @@ +package access_request + +import ( + "path/filepath" + "strings" + + "github.com/redhat-data-and-ai/naysayer/internal/rules/common" + "github.com/redhat-data-and-ai/naysayer/internal/rules/shared" + "gopkg.in/yaml.v3" +) + +const ( + ruleName = "hello_access_request" + + helloaggregatePrefix = "dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/" + hellosourcePrefix = "dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/" + + dataProductHelloaggregate = "helloaggregate" + dataProductHellosource = "hellosource" +) + +// Rule auto-approves access-request YAML files for helloaggregate and hellosource data products. +type Rule struct { + *common.BaseRule + *common.ValidationHelper +} + +// NewRule creates a new access request rule instance. +func NewRule() *Rule { + return &Rule{ + BaseRule: common.NewBaseRule( + ruleName, + "Auto-approves access-request files under helloaggregate and hellosource when the MR contains only those files and YAML fields match the path", + ), + ValidationHelper: common.NewValidationHelper(), + } +} + +// GetCoveredLines returns line ranges this rule validates. +func (r *Rule) GetCoveredLines(filePath string, fileContent string) []shared.LineRange { + if !r.isAccessRequestFile(filePath) { + return nil + } + + if strings.TrimSpace(fileContent) == "" { + return []shared.LineRange{{StartLine: 1, EndLine: 1, FilePath: filePath}} + } + + return r.GetFullFileCoverage(filePath, fileContent) +} + +// ValidateLines validates access-request file content and MR scope. +func (r *Rule) ValidateLines(filePath string, fileContent string, lineRanges []shared.LineRange) (shared.DecisionType, string) { + if !r.isAccessRequestFile(filePath) { + return r.CreateManualReviewResult("Not an allowed access-request file path") + } + + if reason := r.validateMRContainsOnlyAccessRequests(); reason != "" { + return r.CreateManualReviewResult(reason) + } + + if strings.TrimSpace(fileContent) == "" { + return r.CreateManualReviewResult("Access-request file deletion or empty content requires manual review") + } + + expectedProduct, ok := r.expectedDataProduct(filePath) + if !ok { + return r.CreateManualReviewResult("Access-request file path is not under an allowed data product directory") + } + + var doc struct { + Name string `yaml:"name"` + DataProduct string `yaml:"data_product"` + } + if err := yaml.Unmarshal([]byte(fileContent), &doc); err != nil { + return r.CreateManualReviewResult("Failed to parse access-request YAML content") + } + + if doc.Name == "" || doc.DataProduct == "" { + return r.CreateManualReviewResult("Access-request YAML must contain name and data_product fields") + } + + expectedName := r.nameFromFilename(filePath) + if expectedName == "" { + return r.CreateManualReviewResult("Could not derive expected name from filename") + } + + if doc.Name != expectedName { + return r.CreateManualReviewResult( + "name field '" + doc.Name + "' does not match filename '" + expectedName + "'", + ) + } + + if doc.DataProduct != expectedProduct { + return r.CreateManualReviewResult( + "data_product '" + doc.DataProduct + "' does not match expected '" + expectedProduct + "' for this path", + ) + } + + return r.CreateApprovalResult("Auto-approved: valid access-request for " + expectedProduct) +} + +func (r *Rule) isAccessRequestFile(filePath string) bool { + if filePath == "" { + return false + } + + normalized := filepath.ToSlash(filePath) + lower := strings.ToLower(normalized) + + if !strings.HasSuffix(lower, ".yaml") && !strings.HasSuffix(lower, ".yml") { + return false + } + + return strings.HasPrefix(lower, helloaggregatePrefix) || strings.HasPrefix(lower, hellosourcePrefix) +} + +func (r *Rule) expectedDataProduct(filePath string) (string, bool) { + lower := strings.ToLower(filepath.ToSlash(filePath)) + switch { + case strings.HasPrefix(lower, helloaggregatePrefix): + return dataProductHelloaggregate, true + case strings.HasPrefix(lower, hellosourcePrefix): + return dataProductHellosource, true + default: + return "", false + } +} + +func (r *Rule) nameFromFilename(filePath string) string { + base := filepath.Base(filePath) + lower := strings.ToLower(base) + + switch { + case strings.HasSuffix(lower, ".yaml"): + return base[:len(base)-5] + case strings.HasSuffix(lower, ".yml"): + return base[:len(base)-4] + default: + return "" + } +} + +func (r *Rule) validateMRContainsOnlyAccessRequests() string { + mrCtx := r.GetMRContext() + if mrCtx == nil { + return "MR context not available" + } + + for _, change := range mrCtx.Changes { + path := change.NewPath + if path == "" { + path = change.OldPath + } + if path == "" { + continue + } + if !r.isAccessRequestFile(path) { + return "MR contains files outside allowed access-request paths: " + path + } + } + + return "" +} diff --git a/internal/rules/access_request/rule_test.go b/internal/rules/access_request/rule_test.go new file mode 100644 index 0000000..4a76ac3 --- /dev/null +++ b/internal/rules/access_request/rule_test.go @@ -0,0 +1,170 @@ +package access_request + +import ( + "testing" + + "github.com/redhat-data-and-ai/naysayer/internal/gitlab" + "github.com/redhat-data-and-ai/naysayer/internal/rules/shared" + "github.com/stretchr/testify/assert" +) + +const ( + testUserHellosource = "mbramle" + testUserHelloaggregate = "tvaldez" +) + +func TestRule_Name(t *testing.T) { + assert.Equal(t, "hello_access_request", NewRule().Name()) +} + +func TestRule_isAccessRequestFile(t *testing.T) { + rule := NewRule() + + tests := []struct { + path string + expected bool + }{ + { + "dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/" + testUserHelloaggregate + ".yaml", + true, + }, + { + "dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/" + testUserHellosource + ".yaml", + true, + }, + { + "dataproducts/aggregate/helloaggregate/groups/dataverse-aggregate-helloaggregate/user.yaml", + false, + }, + { + "dataproducts/source/hellosource/access-requests/groups/other-group/user.yaml", + false, + }, + {"dataproducts/analytics/prod/product.yaml", false}, + {"", false}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + assert.Equal(t, tt.expected, rule.isAccessRequestFile(tt.path)) + }) + } +} + +func TestRule_ValidateLines(t *testing.T) { + hellosourcePath := "dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/" + testUserHellosource + ".yaml" + helloaggregatePath := "dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/" + testUserHelloaggregate + ".yaml" + + validHellosource := "name: " + testUserHellosource + "\ndata_product: hellosource\n" + validHelloaggregate := "name: " + testUserHelloaggregate + "\ndata_product: helloaggregate\n" + + tests := []struct { + name string + filePath string + content string + mrChanges []gitlab.FileChange + wantDecision shared.DecisionType + wantSubstr string + }{ + { + name: "valid hellosource access request", + filePath: hellosourcePath, + content: validHellosource, + mrChanges: []gitlab.FileChange{ + {NewPath: hellosourcePath}, + }, + wantDecision: shared.Approve, + wantSubstr: "Auto-approved", + }, + { + name: "valid helloaggregate access request", + filePath: helloaggregatePath, + content: validHelloaggregate, + mrChanges: []gitlab.FileChange{ + {NewPath: helloaggregatePath}, + }, + wantDecision: shared.Approve, + wantSubstr: "helloaggregate", + }, + { + name: "name mismatch", + filePath: hellosourcePath, + content: "name: wrong\ndata_product: hellosource\n", + mrChanges: []gitlab.FileChange{ + {NewPath: hellosourcePath}, + }, + wantDecision: shared.ManualReview, + wantSubstr: "does not match filename", + }, + { + name: "data_product mismatch", + filePath: hellosourcePath, + content: "name: " + testUserHellosource + "\ndata_product: helloaggregate\n", + mrChanges: []gitlab.FileChange{ + {NewPath: hellosourcePath}, + }, + wantDecision: shared.ManualReview, + wantSubstr: "data_product", + }, + { + name: "MR with extra file", + filePath: hellosourcePath, + content: validHellosource, + mrChanges: []gitlab.FileChange{ + {NewPath: hellosourcePath}, + {NewPath: "dataproducts/source/hellosource/dev/product.yaml"}, + }, + wantDecision: shared.ManualReview, + wantSubstr: "outside allowed access-request paths", + }, + { + name: "empty file", + filePath: hellosourcePath, + content: "", + mrChanges: []gitlab.FileChange{ + {NewPath: hellosourcePath}, + }, + wantDecision: shared.ManualReview, + wantSubstr: "deletion or empty", + }, + { + name: "missing MR context", + filePath: hellosourcePath, + content: validHellosource, + mrChanges: nil, + wantDecision: shared.ManualReview, + wantSubstr: "MR context not available", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rule := NewRule() + if tt.mrChanges != nil { + rule.SetMRContext(&shared.MRContext{Changes: tt.mrChanges}) + } + + decision, reason := rule.ValidateLines( + tt.filePath, + tt.content, + []shared.LineRange{{StartLine: 1, EndLine: 2, FilePath: tt.filePath}}, + ) + + assert.Equal(t, tt.wantDecision, decision) + assert.Contains(t, reason, tt.wantSubstr) + }) + } +} + +func TestRule_GetCoveredLines(t *testing.T) { + rule := NewRule() + path := "dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/" + testUserHellosource + ".yaml" + content := "name: " + testUserHellosource + "\ndata_product: hellosource\n" + + ranges := rule.GetCoveredLines(path, content) + assert.Len(t, ranges, 1) + assert.Equal(t, 1, ranges[0].StartLine) + assert.Equal(t, shared.CountLines(content), ranges[0].EndLine) + + assert.Nil(t, rule.GetCoveredLines("dataproducts/analytics/prod/product.yaml", content)) +} diff --git a/internal/rules/registry.go b/internal/rules/registry.go index e0c70a6..657b3b7 100644 --- a/internal/rules/registry.go +++ b/internal/rules/registry.go @@ -6,6 +6,7 @@ import ( "github.com/redhat-data-and-ai/naysayer/internal/config" "github.com/redhat-data-and-ai/naysayer/internal/gitlab" "github.com/redhat-data-and-ai/naysayer/internal/logging" + "github.com/redhat-data-and-ai/naysayer/internal/rules/access_request" "github.com/redhat-data-and-ai/naysayer/internal/rules/codeowners" "github.com/redhat-data-and-ai/naysayer/internal/rules/common" "github.com/redhat-data-and-ai/naysayer/internal/rules/dataproduct_consumer" @@ -149,6 +150,18 @@ func (r *RuleRegistry) registerBuiltInRules() { Category: "tag", }) + // Access request files for helloaggregate and hellosource + _ = r.RegisterRule(&RuleInfo{ + Name: "hello_access_request", + Description: "Auto-approves access-request YAML files for helloaggregate and hellosource when the MR contains only those files", + Version: "1.0.0", + Factory: func(client gitlab.GitLabClient) shared.Rule { + return access_request.NewRule() + }, + Enabled: true, + Category: "hello_access_request", + }) + } // RegisterRule registers a new rule in the registry diff --git a/rules.yaml b/rules.yaml index 7283f05..3fe3c10 100644 --- a/rules.yaml +++ b/rules.yaml @@ -167,6 +167,33 @@ files: enabled: true auto_approve: true + # Access request files - helloaggregate and hellosource only + - name: "access_requests_helloaggregate" + path: "dataproducts/aggregate/helloaggregate/access-requests/groups/dataverse-aggregate-helloaggregate/" + filename: "*.{yaml,yml}" + parser_type: yaml + enabled: true + sections: + - name: full_file + yaml_path: . + rule_configs: + - name: hello_access_request + enabled: true + auto_approve: true + + - name: "access_requests_hellosource" + path: "dataproducts/source/hellosource/access-requests/groups/dataverse-source-hellosource/" + filename: "*.{yaml,yml}" + parser_type: yaml + enabled: true + sections: + - name: full_file + yaml_path: . + rule_configs: + - name: hello_access_request + enabled: true + auto_approve: true + # CODEOWNERS file - Auto-approve when synced with developers.yaml or groups/*.yaml - name: "codeowners_file" path: "**/" From b854c9a1a2133b7d6f247dcf6ab6b339ec5cdb56 Mon Sep 17 00:00:00 2001 From: Bibhas Debnath Date: Thu, 21 May 2026 14:10:58 +0530 Subject: [PATCH 2/3] added before folders Signed-off-by: Bibhas Debnath --- e2e/diff_generator.go | 5 +++++ .../scenarios/42_access_request_hellosource/before/.gitkeep | 0 .../43_access_request_helloaggregate/before/.gitkeep | 0 .../44_access_request_multi_hellosource/before/.gitkeep | 0 .../45_access_request_multi_cross_dp/before/.gitkeep | 0 .../46_access_request_name_mismatch/before/.gitkeep | 0 .../47_access_request_data_product_mismatch/before/.gitkeep | 0 .../48_access_request_with_uncovered_file/before/.gitkeep | 0 .../scenarios/49_access_request_deletion/after/.gitkeep | 0 .../scenarios/50_access_request_wrong_path/before/.gitkeep | 0 10 files changed, 5 insertions(+) create mode 100644 e2e/testdata/scenarios/42_access_request_hellosource/before/.gitkeep create mode 100644 e2e/testdata/scenarios/43_access_request_helloaggregate/before/.gitkeep create mode 100644 e2e/testdata/scenarios/44_access_request_multi_hellosource/before/.gitkeep create mode 100644 e2e/testdata/scenarios/45_access_request_multi_cross_dp/before/.gitkeep create mode 100644 e2e/testdata/scenarios/46_access_request_name_mismatch/before/.gitkeep create mode 100644 e2e/testdata/scenarios/47_access_request_data_product_mismatch/before/.gitkeep create mode 100644 e2e/testdata/scenarios/48_access_request_with_uncovered_file/before/.gitkeep create mode 100644 e2e/testdata/scenarios/49_access_request_deletion/after/.gitkeep create mode 100644 e2e/testdata/scenarios/50_access_request_wrong_path/before/.gitkeep diff --git a/e2e/diff_generator.go b/e2e/diff_generator.go index bd5830d..2b4043f 100644 --- a/e2e/diff_generator.go +++ b/e2e/diff_generator.go @@ -76,6 +76,11 @@ func buildFileMap(rootDir string) (map[string]string, error) { return nil } + // Placeholder files used only so Git tracks empty scenario directories + if info.Name() == ".gitkeep" { + return nil + } + // Get relative path from root relPath, err := filepath.Rel(rootDir, path) if err != nil { diff --git a/e2e/testdata/scenarios/42_access_request_hellosource/before/.gitkeep b/e2e/testdata/scenarios/42_access_request_hellosource/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/43_access_request_helloaggregate/before/.gitkeep b/e2e/testdata/scenarios/43_access_request_helloaggregate/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/44_access_request_multi_hellosource/before/.gitkeep b/e2e/testdata/scenarios/44_access_request_multi_hellosource/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/45_access_request_multi_cross_dp/before/.gitkeep b/e2e/testdata/scenarios/45_access_request_multi_cross_dp/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/46_access_request_name_mismatch/before/.gitkeep b/e2e/testdata/scenarios/46_access_request_name_mismatch/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/47_access_request_data_product_mismatch/before/.gitkeep b/e2e/testdata/scenarios/47_access_request_data_product_mismatch/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/48_access_request_with_uncovered_file/before/.gitkeep b/e2e/testdata/scenarios/48_access_request_with_uncovered_file/before/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/49_access_request_deletion/after/.gitkeep b/e2e/testdata/scenarios/49_access_request_deletion/after/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/e2e/testdata/scenarios/50_access_request_wrong_path/before/.gitkeep b/e2e/testdata/scenarios/50_access_request_wrong_path/before/.gitkeep new file mode 100644 index 0000000..e69de29 From 9198b4225ef0d082548c6d9e68726749b14ac86c Mon Sep 17 00:00:00 2001 From: Bibhas Debnath Date: Thu, 21 May 2026 14:50:34 +0530 Subject: [PATCH 3/3] some equalfold change Signed-off-by: Bibhas Debnath --- internal/rules/access_request/rule.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/rules/access_request/rule.go b/internal/rules/access_request/rule.go index becde5e..e52bf05 100644 --- a/internal/rules/access_request/rule.go +++ b/internal/rules/access_request/rule.go @@ -85,13 +85,13 @@ func (r *Rule) ValidateLines(filePath string, fileContent string, lineRanges []s return r.CreateManualReviewResult("Could not derive expected name from filename") } - if doc.Name != expectedName { + if !strings.EqualFold(doc.Name, expectedName) { return r.CreateManualReviewResult( "name field '" + doc.Name + "' does not match filename '" + expectedName + "'", ) } - if doc.DataProduct != expectedProduct { + if !strings.EqualFold(doc.DataProduct, expectedProduct) { return r.CreateManualReviewResult( "data_product '" + doc.DataProduct + "' does not match expected '" + expectedProduct + "' for this path", )