Skip to content

Add schema and integration coverage for safe-outputs.create-pull-request.allowed-base-branches#27885

Merged
pelikhan merged 4 commits intomainfrom
copilot/fix-allowed-base-branches-error
Apr 22, 2026
Merged

Add schema and integration coverage for safe-outputs.create-pull-request.allowed-base-branches#27885
pelikhan merged 4 commits intomainfrom
copilot/fix-allowed-base-branches-error

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 22, 2026

gh aw compile was reported to reject allowed-base-branches under safe-outputs.create-pull-request as an unknown property. This PR adds targeted coverage to ensure schema validation and compile behavior continue to accept that field.

  • Regression coverage

    • Added a parser validation test in pkg/parser/schema_location_test.go:
      • TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_AcceptsAllowedBaseBranchesInCreatePullRequest
    • The test validates a realistic frontmatter shape using:
      • safe-outputs.create-pull-request.allowed-base-branches: [main, release/*]
  • Main JSON schema coverage

    • Added a schema structure test in pkg/parser/schema_test.go:
      • TestMainWorkflowSchema_CreatePullRequestAllowedBaseBranches
    • Verifies allowed-base-branches is present under safe-outputs.create-pull-request in main_workflow_schema.json with the expected array[string] shape.
  • Workflow integration coverage

    • Added a compile integration test in pkg/cli/compile_integration_test.go:
      • TestCompileSafeOutputsCreatePullRequestAllowedBaseBranches
    • Verifies a workflow using allowed-base-branches compiles successfully and the generated lock file includes handler config containing allowed_base_branches.
  • Scope

    • No runtime/compiler behavior changes.
    • Test-only update to prevent reintroduction of this validation failure.
"safe-outputs": map[string]any{
  "create-pull-request": map[string]any{
    "allowed-base-branches": []any{"main", "release/*"},
  },
}

🤖 Smoke CI passed — run 24807327721

Generated by Smoke CI for issue #27885 · ● 399.5K ·

Copilot AI changed the title [WIP] Fix unknown property error for allowed-base-branches in safe-outputs Add parser regression coverage for safe-outputs.create-pull-request.allowed-base-branches Apr 22, 2026
Copilot AI requested a review from pelikhan April 22, 2026 18:24
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot ensure the field is present in the main JSON schema, and add a workflow integration test

@pelikhan pelikhan marked this pull request as ready for review April 22, 2026 22:54
Copilot AI review requested due to automatic review settings April 22, 2026 22:54
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot ensure the field is present in the main JSON schema, and add a workflow integration test

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a targeted regression test to ensure ValidateMainWorkflowFrontmatterWithSchemaAndLocation accepts safe-outputs.create-pull-request.allowed-base-branches, preventing gh aw compile schema validation from rejecting the field as unknown.

Changes:

  • Added a new unit test covering safe-outputs.create-pull-request.allowed-base-branches acceptance in main workflow frontmatter schema validation.
Show a summary per file
File Description
pkg/parser/schema_location_test.go Adds a regression test ensuring allowed-base-branches is accepted under safe-outputs.create-pull-request.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +283 to +297
"permissions": map[string]any{
"contents": "read",
"pull-requests": "read",
},
"engine": map[string]any{
"id": "copilot",
"model": "gpt-5.4",
},
"network": map[string]any{
"allowed": []any{"defaults"},
},
"tools": map[string]any{
"edit": map[string]any{},
"bash": true,
},
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regression test includes several unrelated schema fields (permissions/engine/network/tools). That makes the test more likely to fail due to future schema changes in those areas rather than the intended safe-outputs.create-pull-request.allowed-base-branches validation. Consider trimming frontmatter to only the minimal fields required for a valid workflow plus the safe-outputs.create-pull-request.allowed-base-branches shape (similar to the smaller fixtures used in other tests in this file) so the test isolates the regression being covered.

Suggested change
"permissions": map[string]any{
"contents": "read",
"pull-requests": "read",
},
"engine": map[string]any{
"id": "copilot",
"model": "gpt-5.4",
},
"network": map[string]any{
"allowed": []any{"defaults"},
},
"tools": map[string]any{
"edit": map[string]any{},
"bash": true,
},

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

Excellent test quality

Metric Value
New/modified tests analyzed 1
✅ Design tests (behavioral contracts) 1 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 1 (100%)
Duplicate test clusters 0
Test inflation detected Yes — 33 test lines / 0 production lines changed (expected for a regression test PR)
🚨 Coding-guideline violations None

Test Classification Details

Test File Classification Issues Detected
TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_AcceptsAllowedBaseBranchesInCreatePullRequest pkg/parser/schema_location_test.go:277 ✅ Design No invalid-value rejection test; see suggestion below

Observations

TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_AcceptsAllowedBaseBranchesInCreatePullRequest

Classification: Design test (behavioral contract)

What design invariant does this test enforce? That a frontmatter containing safe-outputs.create-pull-request.allowed-base-branches is accepted as valid by the schema validator — a direct regression guard for the bug being fixed.

What would break if deleted? A schema regression that re-rejects allowed-base-branches would go undetected.

Build tag: ✅ //go:build !integration present (pre-existing on the file)

Assertion message: ✅ Descriptive message passed to t.Fatalf

Suggested enhancement (not required to merge): Add a complementary negative test asserting that an invalid value for allowed-base-branches (e.g. an integer instead of a string array) is rejected by the validator. This would complete the contract: "valid → accepted, invalid → rejected".

// Suggested companion test
func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_RejectsInvalidAllowedBaseBranches(t *testing.T) {
    frontmatter := map[string]any{
        // ... minimal valid base ...
        "safe-outputs": map[string]any{
            "create-pull-request": map[string]any{
                "allowed-base-branches": "main", // string instead of array — should fail
            },
        },
    }
    err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/test/workflow.md")
    if err == nil {
        t.Fatalf("expected validation error for non-array allowed-base-branches, got nil")
    }
}

Note on test inflation: The inflation ratio is technically infinite (33 test lines, 0 production lines changed), which triggers the 10-point penalty. This is expected — the PR's explicit purpose is to add a regression test for a fix already present in the schema. The penalty is applied mechanically but does not indicate a quality problem.


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 1 test — unit (//go:build !integration)
  • 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). The single regression test enforces a clear behavioral contract. Consider adding a negative-case test for full contract coverage.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §24806855155

🧪 Test quality analysis by Test Quality Sentinel · ● 436.5K ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The regression test enforces a clear behavioral contract (valid config is accepted by the schema validator). Optional: consider adding a negative-case companion test to fully cover the contract.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 22, 2026

@copilot ensure the field is present in the main JSON schema, and add a workflow integration test

Addressed in fd99d12. Added a parser schema test that asserts allowed-base-branches exists under safe-outputs.create-pull-request in main_workflow_schema.json, and added an integration compile test that verifies a workflow using this field compiles and includes allowed_base_branches in handler config.

@copilot ensure the field is present in the main JSON schema, and add a workflow integration test

Implemented in fd99d12. The update includes both requested checks: (1) schema-level assertion that allowed-base-branches is present in the main workflow schema under create-pull-request, and (2) a workflow compile integration test covering this field end-to-end.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/repos/actions/github-script/git/ref/tags/v9
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Add parser regression coverage for safe-outputs.create-pull-request.allowed-base-branches Add schema and integration coverage for safe-outputs.create-pull-request.allowed-base-branches Apr 22, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Smoke CI completed successfully!

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Smoke CI — Run complete. View run

Generated by Smoke CI for issue #27885 · ● 399.5K ·

@pelikhan pelikhan merged commit d3b9fec into main Apr 22, 2026
23 of 25 checks passed
@pelikhan pelikhan deleted the copilot/fix-allowed-base-branches-error branch April 22, 2026 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh aw compile reports allowed-base-branches as unknown for safe-outputs.create-pull-reques

3 participants