fix: rename --approve-updates to --approve and skip safe update enforcement on first compile

[dsyme]

Summary

Two related fixes that were present in the working tree but not included in any previous PR.

1. Rename --approve-updates → --approve

The --approve-updates flag has been renamed to --approve across all three commands that use it:

• gh aw compile
• gh aw run
• gh aw upgrade

2. Skip safe update enforcement on first compile

Previously, when no prior manifest existed (first compilation of a workflow), the compiler would treat it as an empty baseline and flag all new secrets and custom actions with a SECURITY REVIEW REQUIRED warning.

This was overly aggressive: on first compile there is no prior state to compare against, so there is nothing to "enforce". The new behavior skips enforcement entirely when no manifest exists — the newly generated lock file creates the baseline, and enforcement kicks in on subsequent compilations when actual changes are detected.

Files changed

• cmd/gh-aw/main.go — rename flag in compile and run commands; minor help text fix
• pkg/cli/upgrade_command.go — rename flag in upgrade command
• pkg/workflow/safe_update_enforcement.go — skip enforcement when manifest is nil; update remediation message
• pkg/workflow/safe_update_enforcement_test.go — update test expectations
• pkg/cli/compile_safe_update_integration_test.go — update integration test expectations and --approve-updates → --approve

[pelikhan]

@dsyme when a user prompts CCA to generate a new AW, we should generate a warning if the generated AW uses secrets or uncommon actions. This is why we need to support the scenario where no lock file already exists.

[Copilot]

Pull request overview

This PR updates the CLI approval flag naming and adjusts safe-update behavior so first-time compiles (no prior manifest) don’t trigger safe-update enforcement.

Changes:

• Renames the CLI flag --approve-updates to --approve across compile/run/upgrade and updates related tests.
• Skips safe update enforcement when no prior manifest exists, establishing the initial lockfile as the baseline.
• Updates safe-update remediation messaging and integration/unit test expectations accordingly.

Show a summary per file

File	Description
cmd/gh-aw/main.go	Renames compile/run flag to --approve and tweaks help text.
pkg/cli/upgrade_command.go	Renames upgrade flag to --approve and adjusts logging.
pkg/workflow/safe_update_enforcement.go	Skips enforcement when manifest is nil; updates remediation text to --approve.
pkg/workflow/safe_update_enforcement_test.go	Updates unit tests for new first-compile behavior and new flag name in guidance.
pkg/cli/compile_safe_update_integration_test.go	Updates integration tests to expect no first-compile warnings; replaces --approve-updates with --approve.

Copilot's findings

Tip

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

• Files reviewed: 5/5 changed files
• Comments generated: 3

[dsyme]

@pelikhan Got it, these changes were on my machine and seemed like they had not been correctly applied, but they're actually the reverse of the updates you made

@dsyme when a user prompts CCA to generate a new AW, we should generate a warning if the generated AW uses secrets or uncommon actions. This is why we need to support the scenario where no lock file already exists.

How about the case where a lock file without a manifest already exists? This is the case for all repo-assist installs. We were getting a lot of crazy warnings on updates to repo-assist that seemed too much.

[pelikhan]

Turn it off for now, needs more refinement.

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

✅ Excellent

Metric	Value
New/modified tests analyzed	5
✅ Design tests (behavioral contracts)	5 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	5 (100%)
Duplicate test clusters	0
Test inflation detected	⚠️ Yes — safe_update_enforcement_test.go (+30 lines) vs safe_update_enforcement.go (+13 lines) = 2.3:1
🚨 Coding-guideline violations	None

---

Test Classification Details

View All Test Classifications (5 tests)

Test	File	Classification	Notes
TestEnforceSafeUpdate (nil manifest rows ×4 modified)	pkg/workflow/safe_update_enforcement_test.go	✅ Design	Behavioral contract: nil manifest now means legacy lock file → skip enforcement
TestEnforceSafeUpdate (non-nil empty manifest rows ×3 added)	pkg/workflow/safe_update_enforcement_test.go	✅ Design	Behavioral contract: empty &GHAWManifest{} = first compile → enforce. Covers error + no-error paths + GITHUB_TOKEN exemption
TestBuildSafeUpdateError (assertion updated)	pkg/workflow/safe_update_enforcement_test.go	✅ Design	Verifies error message content contains --approve (flag rename reflected in user-facing error)
TestSafeUpdateFirstCompileCreatesBaseline (comment + log updated)	pkg/cli/compile_safe_update_integration_test.go	✅ Design	No behavioral change; comment and log message corrected to match new semantics
TestSafeUpdateManifestIncludesImportedSecret, TestSafeUpdateManifestIncludesTransitivelyImportedSecret	pkg/cli/compile_safe_update_integration_test.go	✅ Design	Flag rename --approve-updates → --approve; behavioral coverage unchanged

---

Observations

⚠️ Minor Test Inflation (safe_update_enforcement_test.go)

The test file received +30 added lines vs the production file's +13 added lines (ratio ≈ 2.3:1, threshold 2:1). However, most of the "extra" test lines are renaming 4 existing table-row cases to reflect changed semantics, plus adding 3 genuinely new table rows for the first-compile path. The inflation is defensible — the semantic change required updating all nil-manifest cases plus covering the new non-nil empty manifest path. No action required, but noted for transparency.

✅ Build Tags — Correct

• pkg/workflow/safe_update_enforcement_test.go: //go:build !integration ✅
• pkg/cli/compile_safe_update_integration_test.go: //go:build integration ✅

✅ Assertion Messages — Present

All testify assertions include descriptive message arguments (e.g., "error message should contain %q", "remediation guidance", "added actions").

✅ No Mock Libraries

No gomock, testify/mock, .EXPECT(), or .On() usage detected.

---

Verdict

✅ Check passed. 0% of new/modified tests are implementation tests (threshold: 30%). The tests correctly reflect the behavioral change: nil manifest (legacy lock file) now skips enforcement, while a non-nil empty manifest (first compile) still enforces. All behavioral contracts are covered — including error paths, the GITHUB_TOKEN exemption, and the user-facing flag name in error messages.

---

📖 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: §24434082533

🧪 Test quality analysis by Test Quality Sentinel · ● 706.2K · ◷

[github-actions]

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new/modified tests are implementation tests (threshold: 30%). All 5 tests verify behavioral contracts, with full error/edge-case coverage and no coding-guideline violations.