Enforce manifest-level rejection of workflows that declare private: true#36227
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
private
There was a problem hiding this comment.
Pull request overview
This PR tightens Agentic Workflow package manifest semantics by rejecting any manifest-listed installable workflow that declares a private frontmatter field (including private: false), both for remote manifest-backed installs (gh aw add / add-wizard) and for local compile-time manifest validation.
Changes:
- Add presence-aware parsing for the
privatefrontmatter field (ExtractWorkflowPrivateSetting) and use it to block manifest-listed workflows that declareprivate. - Extend local manifest compilation validation to inspect manifest-listed (or auto-discovered) installable workflows and fail compilation when any declares
private. - Add CLI-focused tests covering rejection of manifest-listed workflows that set
private: falsein both remote resolution and local compilation.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/packages.go | Introduces presence-aware private extraction helper used by manifest-specific validation. |
| pkg/cli/compile_repository_manifest.go | Extends local manifest validation to read/installable workflow sources and reject private declarations. |
| pkg/cli/compile_repository_manifest_test.go | Adds compilation test ensuring manifest-listed workflow with private: false is rejected. |
| pkg/cli/add_workflow_resolution.go | Rejects manifest-backed workflow selections when the workflow frontmatter declares private. |
| pkg/cli/add_package_manifest.go | Adds shared helper to validate privacy rules across manifest-listed installable workflows. |
| pkg/cli/add_package_manifest_test.go | Adds resolution test ensuring repository package manifests reject private: false. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
| if private, ok := result.Frontmatter["private"]; ok { | ||
| if privateBool, ok := private.(bool); ok { | ||
| return privateBool | ||
| return privateBool, true | ||
| } | ||
| } | ||
|
|
||
| return false, false | ||
| } |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Non-blocking review — two medium-priority issues worth addressing before the code grows more test coverage on top of the current structure.
### Findings summary
Duplicate error-message strings (add_workflow_resolution.go and add_package_manifest.go) — identical fmt.Errorf format strings are copy-pasted across the runtime and compile-time validation paths. Extract them into package-level constants so both sites stay in sync and tests don't need to be updated in two places.
Missing test for private: true manifest path — TestResolveWorkflows_RepositoryPackageRejectsPrivateFalse exists but the private: true branch in ResolveWorkflows produces a distinct error message that is never asserted. A regression in that branch would go undetected.
(The non-bool private value bypass — e.g. private: "false" — is already called out in a prior review comment.)
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.6M
| if hasPrivate { | ||
| manifestPath := joinRepositoryPackagePath(spec.PackagePath, repositoryPackageManifestFileName) | ||
| if privateValue { | ||
| return nil, fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added", manifestPath, resolvedSpec.WorkflowPath) |
There was a problem hiding this comment.
Duplicate error-message strings will silently drift from the compile-time counterpart: the two error templates for private: true and private: false are copy-pasted verbatim from validateManifestInstallableWorkflowPrivacy, so any future wording fix applied to one site won't propagate to the other.
💡 Suggested fix
Extract both templates into package-level constants (or a tiny shared helper) so both validation paths stay in sync:
const (
errManifestPrivateTrue = "invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added"
errManifestPrivateFalse = "invalid Agentic Workflow manifest %q: workflow %q sets private: false; remove the private field because manifest-listed workflows must not declare it"
)Both validateManifestInstallableWorkflowPrivacy and the block added here then reference the same constants, and a test that asserts on the exact string only needs to be updated in one place.
|
|
||
| if spec.FromRepositoryManifest { | ||
| privateValue, hasPrivate := ExtractWorkflowPrivateSetting(string(fetched.Content)) | ||
| if hasPrivate { |
There was a problem hiding this comment.
The private: true manifest-rejection path has no test: the new private: false case is covered by TestResolveWorkflows_RepositoryPackageRejectsPrivateFalse, but the private: true branch (line 220) produces a different error message ("sets private: true and cannot be included") that is never asserted. If that branch were accidentally deleted or its message changed, no test would catch it.
💡 Suggested fix
Add a companion test that sets private: true in the fetched workflow content and asserts the "sets private: true" error string:
func TestResolveWorkflows_RepositoryPackageRejectsPrivateTrue(t *testing.T) {
// ... same stub setup as the PrivateFalse test ...
// workflow content: "---\nprivate: true\n---\n\n# Review\n"
_, err := ResolveWorkflows(context.Background(), []string{"owner/repo"}, false)
require.Error(t, err)
assert.Contains(t, err.Error(), `workflow "workflows/review.md" sets private: true`)
}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — commenting with non-blocking suggestions on test coverage gaps and a potential robustness issue in the new filesystem scanner.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: The
scanLocalRepositoryPackageInstallablePathsauto-discovery fallback has no test; theprivate: truebranch of the manifest-backed install check also lacks a dedicated case. - Duplicated enforcement logic: The install-time path inlines the same two error messages that
validateManifestInstallableWorkflowPrivacyalready encapsulates, creating a maintenance surface. - WalkDir robustness: Returning
walkErrimmediately on any entry error means a single unreadable file aborts the whole compile-time scan.
Positive Highlights
- ✅ Correct root-cause fix: splitting
ExtractWorkflowPrivateinto presence-awareExtractWorkflowPrivateSettingeliminates the boolean-blindness bug cleanly without breaking existing callers. - ✅ Defense-in-depth: two independent enforcement layers (install-time + compile-time) are the right approach.
- ✅ Clear, actionable error messages that tell the author exactly what to change (
remove the private field). - ✅ Tests use real filesystem fixtures and integration-style setup, which gives high confidence for the covered paths.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M
| `), 0o644)) | ||
|
|
||
| _, err = CompileWorkflows(context.Background(), CompileConfig{}) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
[/tdd] The new scanLocalRepositoryPackageInstallablePaths fallback (triggered when manifest.Files and manifest.Includes are both empty) has no test coverage. This is the only code path that actually walks the filesystem, applies deduplication, and skips missing source directories — all independently from the files:-listing path tested here.
💡 Suggested test sketch
Add a test variant that omits files: from aw.yml so the compile-time path falls through to the scanner:
func TestCompileWorkflows_RejectsAutoDiscoveredWorkflowWithPrivate(t *testing.T) {
// Same setup, but aw.yml has no `files:` key
os.WriteFile(..., []byte(`manifest-version: "1"\nname: Repo Assist\n`), 0o644)
// review.md with private: true or private: false lives under workflows/
_, err = CompileWorkflows(context.Background(), CompileConfig{})
require.Error(t, err)
assert.Contains(t, err.Error(), `sets private:`)
}Without this, a regression in WalkDir path normalization or deduplication would be invisible.
| } | ||
|
|
||
| _, err := ResolveWorkflows(context.Background(), []string{"owner/repo"}, false) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
[/tdd] The new test covers private: false through a manifest-backed install, but there's no corresponding test for private: true arriving via FromRepositoryManifest. The check in add_workflow_resolution.go has two branches (privateValue true vs false) — only the false branch is exercised here.
💡 Suggested addition
Add a TestResolveWorkflows_RepositoryPackageRejectsPrivateTrue mirror of this test where workflows/review.md contains private: true. This confirms both branches of the inline check in ResolveWorkflows and guards against the error message being accidentally removed from the true branch.
| if privateValue { | ||
| return nil, fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added", manifestPath, resolvedSpec.WorkflowPath) | ||
| } | ||
| return nil, fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: false; remove the private field because manifest-listed workflows must not declare it", manifestPath, resolvedSpec.WorkflowPath) |
There was a problem hiding this comment.
[/diagnose] The install-time check here is inlined, while the compile-time path delegates to validateManifestInstallableWorkflowPrivacy. This means the same two error message strings exist in two places. If one is updated (e.g. to improve wording), the other is easy to miss.
💡 Suggestion
Consider calling validateManifestInstallableWorkflowPrivacy (or extracting a small checkWorkflowPrivacyForManifest(manifestPath, workflowPath string, content []byte) error helper) from both sites. The inline check here could become:
if spec.FromRepositoryManifest {
manifestPath := joinRepositoryPackagePath(spec.PackagePath, repositoryPackageManifestFileName)
if err := checkWorkflowPrivacyForManifest(manifestPath, resolvedSpec.WorkflowPath, fetched.Content); err != nil {
return nil, err
}
}This keeps the two enforcement layers in sync automatically.
| } | ||
|
|
||
| relativePath, err := filepath.Rel(packageDir, currentPath) | ||
| if err != nil { |
There was a problem hiding this comment.
[/diagnose] filepath.WalkDir returns walkErr to the callback for entries it could not stat (e.g. a broken symlink or a permission-denied file inside the directory). Returning walkErr immediately aborts the entire scan — a single unreadable file fails the whole compilation.
💡 Suggestion
Consider skipping non-fatal walk errors on individual files rather than aborting:
if walkErr != nil {
if d != nil && !d.IsDir() {
return nil // skip unreadable file, keep scanning
}
return walkErr // propagate errors opening directories
}This keeps the scan robust in the presence of broken symlinks or permission quirks in the package tree.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Both tests enforce the behavioral contract that manifest workflows with private: false are rejected. Minor suggestion: add descriptive message arguments to assertions.
|
@copilot apply copilot-review skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot it should be "private: true" instead of "private: false" to disable add/package. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 351ea82. Manifest-backed add/package and compile validation now only block workflows with |
privateprivate: true
aw.ymlpackages can include installable workflows, but onlyprivate: trueshould disable add/package behavior. This change ensures manifest-backed installs and compile-time validation consistently reject workflows that explicitly setprivate: true, while allowingprivate: false.Manifest-backed add / add-wizard
aw.ymlwhen their frontmatter setsprivate: trueprivate: trueblocks installationCompile-time manifest validation
aw.ymlpackagesprivate: trueParsing and error semantics
privatefieldprivate: truein manifest-listed workflowsCoverage
private: trueprivate: trueExample of a now-invalid manifest-listed workflow:
When that file is included by
aw.yml,gh aw add,gh aw add-wizard, and compile-time manifest validation reject it.