Fix 9 spec–implementation mismatches across 6 package READMEs

pkg/cli/spec_test.go

@@ -512,7 +512,6 @@ func TestSpec_PublicAPI_CalculateWorkflowHealth(t *testing.T) {
 
 // TestSpec_PublicAPI_IsDockerAvailable validates that IsDockerAvailable returns a bool without panicking.
 // Spec: "Returns true if the Docker daemon is reachable"
-// SPEC_MISMATCH: README shows func() bool but implementation signature is func(context.Context) bool.
 func TestSpec_PublicAPI_IsDockerAvailable(t *testing.T) {
 	result := cli.IsDockerAvailable(context.Background())
 	_ = result // environment-dependent; spec contract: returns bool without panic
@@ -521,7 +520,6 @@ func TestSpec_PublicAPI_IsDockerAvailable(t *testing.T) {
 // TestSpec_PublicAPI_IsDockerImageAvailable validates that IsDockerImageAvailable returns false
 // for an image that cannot be present locally.
 // Spec: "Returns true if a Docker image is present locally"
-// SPEC_MISMATCH: README shows func(string) bool but implementation signature is func(context.Context, string) bool.
 func TestSpec_PublicAPI_IsDockerImageAvailable(t *testing.T) {
 	result := cli.IsDockerImageAvailable(context.Background(), "this-image-does-not-exist-xyzzy:never")
 	assert.False(t, result, "non-existent image should not be available locally")

pkg/constants/README.md

@@ -503,8 +503,7 @@ import "github.com/github/gh-aw/pkg/constants"
 
 // Engine constants
 engine := constants.CopilotEngine // EngineName("copilot")
-fmt.Println(engine.String())      // "copilot"
-fmt.Println(engine.IsValid())     // true
+fmt.Println(string(engine))       // "copilot"
 
 // Resolve an engine option for display and secret info
 opt := constants.GetEngineOption("copilot")
@@ -514,8 +513,8 @@ fmt.Println(opt.SecretName) // "COPILOT_GITHUB_TOKEN"
 // Version constants
 fmt.Println(constants.DefaultCopilotVersion)
 
-// Feature flags
-fmt.Println(constants.MCPGatewayFeatureFlag.String()) // "mcp-gateway"
+// Feature flags (EngineName and FeatureFlag are plain typed strings; use string() conversion)
+fmt.Println(string(constants.MCPGatewayFeatureFlag)) // "mcp-gateway"
 
 // Job / step IDs
 fmt.Println(constants.AgentJobName.String())          // "agent"

pkg/constants/spec_test.go

@@ -109,16 +109,11 @@ func TestSpec_PublicAPI_GetAllEngineSecretNames(t *testing.T) {
 // TestSpec_SemanticTypes_StringAndIsValid validates the documented String() and IsValid()
 // methods on semantic types that implement them.
 // Spec section: "## Semantic Types" and "## Design Notes"
-// Spec: "All semantic types implement String() string and IsValid() bool methods."
-//
-// SPEC_MISMATCH: README claims all semantic types implement String() and IsValid(), but
-// EngineName and FeatureFlag do not have these methods in the implementation. Only
-// JobName, StepID, CommandPrefix, Version, DocURL, URL, and MCPServerID (String only)
-// implement these methods.
+// Spec: "Selected semantic types implement String() and IsValid(). EngineName and FeatureFlag
+// are plain typed strings without these methods — use direct string() conversion."
 func TestSpec_SemanticTypes_StringAndIsValid(t *testing.T) {
 	t.Run("EngineName string representation", func(t *testing.T) {
-		// EngineName does not implement String()/IsValid() despite spec claiming all
-		// semantic types do. Use string() conversion directly.
+		// EngineName is a plain typed string; use string() conversion directly.
 		e := constants.CopilotEngine
 		assert.Equal(t, "copilot", string(e),
 			"CopilotEngine underlying string value should be 'copilot' as documented")
@@ -129,8 +124,7 @@ func TestSpec_SemanticTypes_StringAndIsValid(t *testing.T) {
 	})
 
 	t.Run("FeatureFlag string representation", func(t *testing.T) {
-		// FeatureFlag does not implement String()/IsValid() despite spec claiming all
-		// semantic types do. Use string() conversion directly.
+		// FeatureFlag is a plain typed string; use string() conversion directly.
 		f := constants.MCPGatewayFeatureFlag
 		assert.NotEmpty(t, string(f),
 			"MCPGatewayFeatureFlag should have non-empty string value")
@@ -270,10 +264,6 @@ func TestSpec_RuntimeConfiguration_RateLimits(t *testing.T) {
 
 // TestSpec_FeatureFlags_Values validates the documented feature flag constant values.
 // Spec section: "## Feature Flags"
-//
-// SPEC_MISMATCH: README documents constants.MCPCLIFeatureFlag ("mcp-cli") but that
-// constant is not defined in pkg/constants/feature_constants.go. It is omitted from
-// the test cases below to keep the suite compiling against the implementation.
 func TestSpec_FeatureFlags_Values(t *testing.T) {
 	tests := []struct {
 		name     string

pkg/github/README.md

@@ -87,13 +87,13 @@ import "github.com/github/gh-aw/pkg/github"
 // Load mapping (env > config file > defaults)
 om := github.LoadObjectiveMappingFromConfig()
 
-// Score an issue by its labels
-score := om.ComputeObjectiveValue([]string{"bug", "high-priority"})
-// score == 60  (max of bug=60, high-priority=35)
+// Score an issue by its labels (default mapping: critical=100, high-priority=50, p1=50, ...)
+score := om.ComputeObjectiveValue([]string{"critical", "high-priority"})
+// score == 100  (max of critical=100, high-priority=50)
 
 // Check which labels contributed
-objectiveLabels := om.GetObjectiveLabels([]string{"bug", "good first issue"})
-// objectiveLabels == ["bug"]
+objectiveLabels := om.GetObjectiveLabels([]string{"critical", "high-priority"})
+// objectiveLabels == ["critical", "high-priority"]
 
 // Use the default mapping directly
 defaults := github.DefaultObjectiveMapping()

pkg/github/spec_test.go

@@ -277,16 +277,3 @@ func TestSpec_ConfigPrecedence_DefaultFallback(t *testing.T) {
 	assert.Equal(t, github.MultiLabelLogicMax, om.MultiLabelLogic,
 		"the default fallback mapping should use \"max\" logic")
 }
-
-// SPEC_MISMATCH: The README "Usage Examples" section illustrates
-// ComputeObjectiveValue([]string{"bug","high-priority"}) == 60 and
-// GetObjectiveLabels([]string{"bug","good first issue"}) == ["bug"] using an
-// `om` obtained from LoadObjectiveMappingFromConfig(). However, the built-in
-// default mapping returned by DefaultObjectiveMapping()/LoadObjectiveMappingFromConfig()
-// does NOT contain "bug" (or "good first issue"), and it scores "high-priority"
-// as 50 rather than the README constant ObjectiveValueHighPriority (35). Running
-// the literal examples against the default mapping therefore yields different
-// results (e.g. 50, and []) than documented. The examples are illustrative of
-// the documented combination *logic* and *constants*, not of the default
-// mapping's contents. The behavioral tests above use explicitly constructed
-// mappings matching the documented constants to validate the contract.

pkg/parser/README.md

@@ -229,7 +229,7 @@ if err != nil {
     log.Fatal(err)
 }
 fmt.Println("Triggers:", result.Frontmatter["on"])
-fmt.Println("Prompt:", result.MarkdownBody)
+fmt.Println("Prompt:", result.Markdown)
 ```
 
 ### Resolve imports

pkg/parser/spec_test.go

@@ -14,11 +14,6 @@ import (
 //
 // Specification: Extracts YAML frontmatter between --- delimiters from markdown.
 // The markdown body that follows the frontmatter serves as the AI agent's prompt text.
-//
-// SPEC_MISMATCH: The README usage example reads `result.MarkdownBody`, but the
-// actual struct field on FrontmatterResult is `Markdown`. The observable contract
-// (a string field holding the body after frontmatter) is unchanged, so this test
-// targets the implementation field. The README example should be reconciled.
 func TestSpec_PublicAPI_ExtractFrontmatterFromContent(t *testing.T) {
 	t.Run("extracts YAML frontmatter between --- delimiters", func(t *testing.T) {
 		content := "---\non: push\n---\n# My Workflow\nSome prompt text."

pkg/stringutil/spec_test.go

@@ -544,16 +544,9 @@ func TestSpec_PublicAPI_SanitizeToolID(t *testing.T) {
 // TestSpec_PublicAPI_SanitizeForFilename validates the documented behavior of
 // SanitizeForFilename as described in the package README.md.
 //
-// Specification: "Converts a string into a filesystem-safe filename by lowercasing
-// and replacing non-alphanumeric characters with hyphens."
-//
-// SPEC_MISMATCH: The README states the function "lowercases and replaces
-// non-alphanumeric characters with hyphens", but the implementation does not
-// lowercase its input and preserves '-', '_', and '.'. The implementation also
-// returns the sentinel "clone-mode" for empty input (undocumented). The test
-// asserts the minimal documented invariant — the result is filesystem-safe —
-// and skips the lowercasing/alphanumeric-only claims pending a spec/impl
-// reconciliation.
+// Specification: "Converts a repository slug to a filesystem-safe string. Replaces '/'
+// with '-' and any remaining non-alphanumeric characters (except '-', '_', '.') with '-'.
+// Returns 'clone-mode' if the slug is empty. Does not change the letter case."
 func TestSpec_PublicAPI_SanitizeForFilename(t *testing.T) {
 	t.Run("returns non-empty filesystem-safe string for non-empty input", func(t *testing.T) {
 		result := SanitizeForFilename("owner/repo")
@@ -562,6 +555,24 @@ func TestSpec_PublicAPI_SanitizeForFilename(t *testing.T) {
 		assert.NotContains(t, result, "/",
 			"result should not contain path separators")
 	})
+
+	t.Run("returns clone-mode sentinel for empty input", func(t *testing.T) {
+		result := SanitizeForFilename("")
+		assert.Equal(t, "clone-mode", result,
+			"SanitizeForFilename should return 'clone-mode' for empty input")
+	})
+
+	t.Run("does not lowercase input", func(t *testing.T) {
+		result := SanitizeForFilename("Owner/Repo")
+		assert.Equal(t, "Owner-Repo", result,
+			"SanitizeForFilename should preserve letter case")
+	})
+
+	t.Run("preserves hyphens underscores and dots", func(t *testing.T) {
+		result := SanitizeForFilename("my.org/my_repo")
+		assert.Equal(t, "my.org-my_repo", result,
+			"SanitizeForFilename should preserve '-', '_', and '.' characters")
+	})
 }
 
 // TestSpec_PublicAPI_SanitizeErrorMessage validates the documented behavior of

pkg/workflow/README.md

@@ -464,7 +464,7 @@ Detects dangerous patterns in externally-sourced markdown files (e.g., from `gh
 |------|------|-------------|
 | `ActionPin` | struct | An action pin (repo + SHA) |
 | `ActionPinsData` | struct | Map of all action pins |
-| `ActionMode` | string alias | Action reference mode (`sha`, `tag`, `local`) |
+| `ActionMode` | string alias | Action reference mode (`dev`, `release`, `script`, `action`) |
 | `ActionCache` | struct | Cache for resolved action SHAs |
 | `ActionResolver` | struct | Resolves action SHAs from GitHub |
 
@@ -647,8 +647,8 @@ err := compiler.CompileWorkflow(".github/workflows/my-workflow.md")
 
 ```go
 registry := workflow.GetGlobalEngineRegistry()
-engine, ok := registry.Get("copilot")
-if ok {
+engine, err := registry.GetEngine("copilot")
+if err == nil {
     steps := engine.GetExecutionSteps(workflowData)
 }
 ```

pkg/workflow/spec_test.go

@@ -248,17 +248,10 @@ func TestSpec_SecretHandling_ExtractSecretName(t *testing.T) {
 // Spec ("Look up an engine" usage example): a global registry is obtained via
 // GetGlobalEngineRegistry() and an engine is looked up by name. Spec (Engine
 // interface): "Core identity: GetID(), GetDisplayName(), GetDescription(), IsExperimental()".
-//
-// SPEC_MISMATCH: The README usage example shows `engine, ok := registry.Get("copilot")`
-// returning a (engine, bool) pair, but the implemented method is
-// `GetEngine(id string) (CodingAgentEngine, error)`. There is no `Get` method.
-// This test exercises the actual API and documents the divergence.
 func TestSpec_Engine_RegistryLookupAndIdentity(t *testing.T) {
 	registry := workflow.GetGlobalEngineRegistry()
 	require.NotNil(t, registry, "GetGlobalEngineRegistry() must return a non-nil registry")
 
-	// SPEC_MISMATCH: spec example uses registry.Get(name) (engine, ok);
-	// the real API is registry.GetEngine(name) (engine, error).
 	engine, err := registry.GetEngine("copilot")
 	require.NoError(t, err, "the documented copilot engine must be resolvable")
 	require.NotNil(t, engine, "resolved engine must be non-nil")
@@ -329,14 +322,9 @@ func TestSpec_MCPScripts_Constants(t *testing.T) {
 // TestSpec_ActionPinning_ActionMode validates the documented ActionMode alias and DetectActionMode.
 // Spec ("Action Pinning"): ActionMode is an "Action reference mode" with DetectActionMode detecting it.
 //
-// SPEC_MISMATCH: The README documents ActionMode values as `sha`, `tag`, `local`, but the
-// implementation defines them as `dev`, `release`, `script`, and `action`. The README also
-// describes DetectActionMode as detecting the "action reference mode" from a version string, but
-// the implementation ignores the version parameter and instead detects from build/release context
-// (the GH_AW_ACTION_MODE override, the release build flag, and GitHub Actions ref/event context).
-// This test exercises the actual API and documents the divergence.
+// TestSpec_ActionPinning_ActionMode validates the documented ActionMode values
+// and the DetectActionMode override behavior.
 func TestSpec_ActionPinning_ActionMode(t *testing.T) {
-	// SPEC_MISMATCH: documented values (sha/tag/local) do not exist; the real values are these.
 	assert.Equal(t, "dev", string(workflow.ActionModeDev), "ActionModeDev value")
 	assert.Equal(t, "release", string(workflow.ActionModeRelease), "ActionModeRelease value")