fix: add missing target/target-repo/allowed-repos to safe output schemas

[dsyme]

Summary

Adds missing target, target-repo, and allowed-repos properties to the safe-output JSON schema entries that already support them at the Go and CJS runtime layer. Before this fix, valid cross-repository configurations (e.g. target: "*", target-repo: owner/repo) were rejected at compile time with Unknown properties errors because the schema declared additionalProperties: false but omitted those fields.

Changes

pkg/parser/schemas/main_workflow_schema.json

Added missing schema properties to the following safe output types:

Safe Output Type	Added Fields
resolve-pull-request-review-thread	target, target-repo, allowed-repos
assign-milestone	target, allowed-repos
hide-comment	target, allowed-repos
link-sub-issue	target, allowed-repos
close-discussion	allowed-repos
update-discussion	allowed-repos
close-pull-request	allowed-repos
mark-pull-request-as-ready-for-review	allowed-repos
add-reviewer	allowed-repos
assign-to-agent	allowed-repos
update-pull-request	allowed-repos

Also updated the resolve-pull-request-review-thread description to document cross-repository support.

pkg/parser/schema_safe_outputs_target_test.go (new)

Adds two test functions:

• TestMainWorkflowSchema_SafeOutputsTargetProperties — 25 sub-tests asserting each affected safe output type accepts target, target-repo, and allowed-repos in the schema.
• TestMainWorkflowSchema_SafeOutputsRejectsUnknownProperties — 4 sub-tests asserting additionalProperties: false is still enforced (misspelled or unknown keys are rejected).

docs/adr/36636-sync-safe-output-schema-with-target-config.md (new)

Draft ADR documenting the decision to keep the JSON schema as a faithful mirror of SafeOutputTargetConfig, the alternatives considered (relaxing additionalProperties, schema generation from Go structs), and the consequences.

Root Cause

The Go parser (SafeOutputTargetConfig / ParseTargetConfig) and the CJS runtime (repo_helpers.cjs) already accepted target, target-repo, and allowed-repos for cross-repository operations. The JSON schema — validated with additionalProperties: false — was the only layer that rejected them, causing spurious compile-time failures for valid workflow configurations.

Impact

• No runtime behavior changes; the fix is purely additive to the schema.
• Existing workflows are unaffected.
• Valid cross-repository safe-output configurations now compile without errors.

Generated by PR Description Updater for issue #36636 · sonnet46 599.3K · ◷

[Copilot]

Pull request overview

This PR fixes JSON schema drift for several safe-outputs configurations by adding missing target, target-repo, and/or allowed-repos properties that the Go workflow compiler/runtime already supports, preventing compile-time "Unknown properties" validation errors for otherwise-valid workflows.

Changes:

• Extend pkg/parser/schemas/main_workflow_schema.json to include target / target-repo / allowed-repos on the affected safe output schemas (while keeping additionalProperties: false behavior).
• Add unit tests to ensure safe outputs that support these fields are accepted by schema validation and that unknown fields are still rejected.

Show a summary per file

File	Description
pkg/parser/schemas/main_workflow_schema.json	Adds missing target-related properties to multiple safe-output schema definitions to align with existing Go/runtime support.
pkg/parser/schema_safe_outputs_target_test.go	Adds regression tests ensuring schemas accept target / target-repo / allowed-repos where supported and still reject unknown fields.

Copilot's findings

Tip

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

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

[github-actions]

✅ smoke-ci: safeoutputs CLI comment + comment-memory run (26886641653)

Generated by 🧪 Smoke CI for issue #36636 · ◷

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

🧪 Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 80/100 — Excellent

Analyzed 2 test function(s) with 28 table-driven subtests: 2 design tests (behavioral contracts), 0 implementation tests, 0 guideline violations.

📊 Metrics & Test Classification (2 tests analyzed)

Metric	Value
New/modified tests analyzed	2 (28 subtests across both table-driven tests)
✅ Design tests (behavioral contracts)	2 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	2 (100%)
Duplicate test clusters	0
Test inflation detected	⚠️ Yes — 370 test lines vs 99 schema lines (≈3.7:1); justified by multi-output regression coverage
🚨 Coding-guideline violations	0

Test Classification Details

Test	File	Classification	Issues Detected
TestMainWorkflowSchema_SafeOutputsTargetProperties	pkg/parser/schema_safe_outputs_target_test.go:14	✅ Design	24 table rows; verifies target/target-repo/allowed-repos accepted by schema for each safe output type
TestMainWorkflowSchema_SafeOutputsRejectsUnknownProperties	pkg/parser/schema_safe_outputs_target_test.go:296	✅ Design	4 table rows; verifies additionalProperties: false constraint rejects unknown fields

Language Support

Tests analyzed:

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

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both test functions enforce behavioral contracts: schema acceptance of valid fields and schema rejection of unknown fields. The //go:build !integration tag is present. No mock libraries used. Assertion messages are descriptive. The 3.7:1 inflation ratio is accepted because the test covers 24+ safe output types against a JSON schema change with no corresponding production .go file.

📖 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.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 860.6K · ◷

[github-actions]

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both tests enforce behavioral contracts with table-driven coverage of happy paths and error cases.

[github-actions]

Skills-Based Review 🧠

Applied /diagnose and /tdd — approving. Root cause is properly addressed (schema–implementation drift), fix is additive with low regression risk, and a purpose-built regression test suite accompanies the change.

📋 Key Themes & Highlights

Key Themes

• Root cause fixed, not just symptom: The target/target-repo/allowed-repos fields were already supported by SafeOutputTargetConfig in Go and the CJS runtime; adding them to the JSON schema is the correct fix.
• Comprehensive positive coverage: All 25 affected safe output types get regression tests — any future schema drift will be caught immediately.
• Minor gap in negative tests: The additionalProperties: false guard is only verified for 4 of 25 types (see inline comment).
• add-comment test slightly under-specified: Only target is tested; target-repo and allowed-repos are schema-supported but not exercised (see inline comment).

Positive Highlights

• ✅ t.Parallel() throughout — fast test execution
• ✅ Clear comment header explaining the regression scenario and why the test exists
• ✅ Both positive and negative test functions in the same file — easy to find
• ✅ Schema changes are surgical and well-described

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M

[github-actions]

[/tdd] The add-comment test only exercises target, but the schema also defines target-repo and allowed-repos for this type. Extending the test case to include all three fields would give full parity with the other 24 entries and close a small coverage gap.

💡 Suggested extension

{
    name: "add-comment with target, target-repo, and allowed-repos",
    safeOutputs: map[string]any{
        "add-comment": map[string]any{
            "max":           5,
            "target":        "*",
            "target-repo":   "github/github",
            "allowed-repos": []any{"github/docs"},
        },
    },
},

This also serves as a live spec that add-comment supports cross-repo targeting.

[github-actions]

[/tdd] The negative-property test (TestMainWorkflowSchema_SafeOutputsRejectsUnknownProperties) only covers 4 of the 25 types exercised in the positive test. While sampling is pragmatic, the 4 chosen types (resolve-pull-request-review-thread, assign-milestone, hide-comment, link-sub-issue) are a bit arbitrary. Consider adding a few more from distinct schema shapes (e.g. add-comment, push-to-pull-request-branch) to increase confidence that additionalProperties: false holds broadly.

💡 Why this matters

Schema definitions are hand-edited JSON, so it's easy to accidentally omit "additionalProperties": false when adding new property blocks. A broader sample catches that class of drift early.

[github-actions]

🏗️ Design Decision Gate — ADR Required

This PR adds 469 new lines in business-logic directories (pkg/parser/...) but did not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/36636-sync-safe-output-schema-with-target-config.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next

1. Review the draft ADR committed to your branch — it was generated from the PR diff and PR description.
2. Complete the missing sections — confirm the decision rationale, refine the alternatives (especially whether schema auto-generation should become a follow-up), and adjust consequences as needed.
3. Commit the finalized ADR to docs/adr/ on your branch.
4. Reference the ADR in this PR body by adding a line such as:

   ADR: ADR-36636: Keep the Safe-Output JSON Schema in Sync with SafeOutputTargetConfig

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

This change closes a drift between the JSON schema and the Go/CJS runtime — exactly the kind of "why does the schema look this way?" decision future contributors will want recorded. ADRs create a searchable, permanent record of why the codebase looks the way it does.

📋 Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

• Context — What is the problem? What forces are at play?
• Decision — What did you decide? Why?
• Alternatives Considered — What else could have been done?
• Consequences — What are the trade-offs (positive and negative)?

ADRs are stored in docs/adr/ numbered by PR number (e.g., 36636-...md).

🔒 This gate remains blocking until an ADR is linked in the PR body.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · opus48 4.1M · ◷