fix: enable runtime evidence profiles - #242

openapi/agh.json:1

• The OpenAPI schema allows any string for runtime.mode, but server-side validation only accepts default or evidence (and treats empty as default after normalization). This mismatch will generate overly-permissive TS types (mode: string) and can lead clients to send values that pass schema validation but are rejected by the API; consider adding an enum (and a default) for runtime.mode matching server behavior. Also consider whether runtime (and/or mode) should truly be required in the OpenAPI contract if the server can safely default it when omitted.

internal/store/globaldb/global_db_task_orchestration_schema_test.go (1)

80-98: ⚡ Quick win

Cover upgraded legacy profile rows, not just the DDL text.

This only proves the statements mention runtime_mode. It doesn’t prove that an existing task_execution_profiles row from the pre-runtime_mode schema upgrades and reopens with runtime_mode = "default", which is the behavior this PR relies on.

A small observed-history case here would catch migration drift much earlier: seed a legacy profile row, run OpenGlobalDB, reopen once more, and assert the migrated row loads with RuntimeModeDefault. As per coding guidelines, "Migration drift fixes require observed-history tests. Cover fresh DB, upgrade/reopen, and the real recorded migration prefix that failed."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/globaldb/global_db_task_orchestration_schema_test.go` around
lines 80 - 98, Add an observed-history test that seeds a legacy
task_execution_profiles row (one that lacks the
taskExecutionProfileRuntimeModeColumn), then calls OpenGlobalDB to run
migrations, reopens the DB, and asserts the migrated row now has
RuntimeModeDefault; specifically, in the same test file augment the existing
case by inserting a pre-migration profile row into the DB (using the legacy
schema), call OpenGlobalDB (the function that applies the migrations), reopen
the database connection, query the task_execution_profiles row and assert its
RuntimeMode equals RuntimeModeDefault, and keep using the existing helpers like
taskOrchestrationProfileMigrationSchemaStatements and
schemaStatementsContainColumn to locate the migration context.

internal/store/globaldb/global_db_task_profile_test.go (1)

90-98: ⚡ Quick win

Assert that partial updates reset runtime.mode back to default.

This update intentionally omits Runtime, but the test never checks that the previously stored evidence mode is cleared. A regression that accidentally preserves opt-in evidence mode would still pass here.

Suggested assertion

 		if !updated.UpdatedAt.Equal(updatedAt) {
 			t.Fatalf("updated.UpdatedAt = %v, want %v", updated.UpdatedAt, updatedAt)
 		}
+		if got, want := updated.Runtime.Mode, taskpkg.RuntimeModeDefault; got != want {
+			t.Fatalf("updated.Runtime.Mode = %q, want %q", got, want)
+		}
 		assertStringSliceGlobal(t, updated.Worker.AllowedAgentNames, []string{"runner"})
 		assertStringSliceGlobal(t, updated.Review.AllowedAgentNames, nil)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/globaldb/global_db_task_profile_test.go` around lines 90 - 98,
The test updates an ExecutionProfile omitting Runtime but never asserts that the
previously set runtime.mode (and any opt-in evidence mode) was cleared; after
calling UpsertExecutionProfile (the updated variable), add an assertion that
updated.Runtime.Mode equals the default runtime mode (e.g.,
taskpkg.RuntimeModeDefault or the zero value) and that any
Runtime.Evidence/opt-in evidence field is nil or reset, so partial updates do
not preserve an opt-in evidence mode.

internal/daemon/task_role_runtime_test.go (1)

24-42: ⚡ Quick win

Use Should ... subtest names in the table.

The new cases use empty, simple, and similar labels, which breaks the repo’s required test naming pattern. Rename them to Should quote empty input, Should quote metacharacters, and so on.

As per coding guidelines, "**/*_test.go: MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/daemon/task_role_runtime_test.go` around lines 24 - 42, Rename the
test case names in the tests table to follow the "Should ..." pattern so
subtests use descriptive names; update each entry in the tests slice (the struct
with name, value, want) to e.g. "Should quote empty input", "Should quote simple
input", "Should quote metacharacters", "Should quote single-quote input"
(keeping the same value and want expectations) so that t.Run(tt.name, ...) still
exercises shellQuoteSimple correctly and matches the repository test naming
guideline.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes