<!-- task=m-principal-provider-credential-slot-routing/01_principal_store plan=1 tag=REVIEW_API -->

# Principal Store Dialect and Lifecycle Repair

## For the Implementing Agent

Filling the implementation-owned sections of `CODE_REVIEW-cloud-G08.md` is mandatory. Run every verification command, paste actual stdout/stderr, keep both active files in place, and report ready for review; only the code-review skill may finalize, rename logs, write `complete.log`, or archive the task. If blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields. Do not ask the user, call user-input tools, create control-plane stop files, classify the next state, archive logs, or write `complete.log`.

## Background

The first review found that the SQLite tests pass while the configured PostgreSQL path uses incompatible DDL and bind placeholders. It also found incomplete revoke/CAS behavior, inactive-principal list failures, and verification output that does not match the repository module path. This follow-up repairs those failures without expanding into credential slots, projection, or management operations.

## Archive Evidence Snapshot

- Prior task: `agent-task/m-principal-provider-credential-slot-routing/01_principal_store/`
- Prior plan: `agent-task/m-principal-provider-credential-slot-routing/01_principal_store/plan_local_G06_0.log`
- Prior review: `agent-task/m-principal-provider-credential-slot-routing/01_principal_store/code_review_cloud_G06_0.log`
- Verdict: FAIL
- Required findings:
  - PostgreSQL DDL and query placeholders are not executable through `pgx`, and unknown schemes containing `.db` can be misclassified as SQLite.
  - Disabled tokens cannot be revoked, CAS ignores affected-row count, and stale/concurrent results are not reliably fenced.
  - `ListPrincipals` scans nullable joined token columns into non-null Go fields after disable/revoke.
  - Recorded `ip/apps/...` verification output conflicts with module `iop` and fresh reviewer output.
- Affected files: `apps/control-plane/internal/credentialstore/store.go`, `apps/control-plane/internal/credentialstore/schema.go`, `apps/control-plane/internal/credentialstore/principal.go`, `apps/control-plane/internal/credentialstore/principal_test.go`, and the active review evidence file.
- Fresh reviewer evidence: `go vet ./apps/control-plane/...`, `go test -count=1 -race ./apps/control-plane/internal/credentialstore`, `go test -count=1 ./apps/control-plane/...`, `go mod tidy -diff`, and `git diff --check` exited zero; Go printed `iop/apps/...` package paths. These passing SQLite tests do not cover the Required failures above.
- Roadmap carryover: keep `principal-store` / SDD S01 targeted; do not update the roadmap until a later PASS completion event.

## Roadmap Targets

- Milestone: `agent-roadmap/phase/operational-observability-provider-management/milestones/principal-provider-credential-slot-routing.md`
- Milestone link: [Milestone document](agent-roadmap/phase/operational-observability-provider-management/milestones/principal-provider-credential-slot-routing.md)
- Task ids:
  - `principal-store`: principal and IOP token hash ledger
- Completion mode: check-on-pass

## Analysis

### Files Read

- `go.mod`
- `apps/control-plane/cmd/control-plane/main.go`
- `apps/control-plane/cmd/control-plane/server.go`
- `apps/control-plane/cmd/control-plane/config_test.go`
- `apps/control-plane/cmd/control-plane/credential_store_test.go`
- `apps/control-plane/internal/credentialstore/store.go`
- `apps/control-plane/internal/credentialstore/schema.go`
- `apps/control-plane/internal/credentialstore/principal.go`
- `apps/control-plane/internal/credentialstore/principal_test.go`

### SDD Criteria

- SDD: `agent-roadmap/sdd/operational-observability-provider-management/principal-provider-credential-slot-routing/SDD.md`; status `[approved]`, lock released.
- Target: S01 / `principal-store`.
- Evidence Map: S01-S03 Control Plane repository lifecycle evidence, narrowed here to S01 one-time raw-token return, digest-only persistence, monotonic revision, irreversible revoke, and restart persistence.
- The checklist therefore keeps SQL dialect correctness, atomic lifecycle fencing, inactive-list behavior, persistence, leak checks, and fresh executable evidence in one repair packet.

### Verification Context

- No separate verification handoff was supplied. Repository-native fallback came from `go.mod`, the Control Plane command/store/tests, the control-plane/testing domain rules, and `agent-test/local/rules.md` plus `agent-test/local/control-plane-smoke.md`.
- Runner: `/config/workspace/iop-s0`; branch `feature/multi-provider-protocol-profile-native-messages`; HEAD `3c92d407`; intentional dirty worktree containing this task's source and artifact changes.
- Toolchain: `/config/.local/bin/go`, Go `1.26.2 linux/arm64`, GOROOT `/config/opt/go` for a module declaring Go 1.24.
- Use executable caches `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build` and `GOCACHE=/config/workspace/iop-s0/.cache/go-cache`.
- Package verification needs no external service or credential. PostgreSQL statement generation is covered deterministically by dialect/binding tests; the local full-cycle uses a temporary SQLite file and the compiled Control Plane entrypoint.
- Preconditions: workspace cache directories exist; temporary runtime files stay under `mktemp -d` and are removed by a trap. External runner, Docker, private endpoint, and provider credentials are not required.
- Gap: no live PostgreSQL server is declared by the local profile. The repair must make PostgreSQL DDL and placeholder generation explicit and unit-testable instead of claiming SQLite execution proves PostgreSQL syntax.
- Confidence: high for the reproduced SQL/lifecycle/list/evidence failures and the proposed deterministic regression boundary.

### Test Coverage Gaps

- Existing tests cover empty URL, invalid/unreachable configured URLs, SQLite migration/reopen, active issuance, direct active revoke, and package race execution.
- Missing: PostgreSQL-safe DDL/binding assertions, unknown `.db` scheme rejection, disabled-to-revoked transition, affected-row CAS loss, concurrent same-revision mutation, and listing a principal without an active token.
- Existing logger evidence is vacuous because the observed logger is not connected to the store. Static no-log behavior may remain, but the follow-up must not present empty observation as evidence of an executed logging path.
- Existing command tests start in-process listeners. The final verification adds one compiled CLI startup/shutdown cycle with a temporary configured database.

### Symbol References

- No rename or removal is required. `credentialstore.Open` is called by `apps/control-plane/cmd/control-plane/server.go`; Store query methods are currently internal to the credential-store package and tests.

### Split Judgment

- Keep one plan. Dialect selection/migration, transaction queries, revision fencing, and persistence/list tests are one principal-ledger correctness boundary; splitting them could allow a store that migrates but cannot execute lifecycle queries or vice versa.
- The task directory has no runtime predecessor dependency (`01_principal_store`).

### Scope Rationale

- Included: SQL dialect selection/binding/migration, principal-token transition/query behavior, focused regressions, and trusted Control Plane startup evidence.
- Excluded: provider credential slot schema, model bindings, projection generation, public management API/CLI, Edge auth, proto/contracts, and agent-spec updates; those belong to later milestone subtasks.
- Do not change `agent-ops/rules/common/**`, `agent-ops/skills/common/**`, roadmap, SDD, contract, or spec documents.

### Final Routing

- `status=routed`, `evaluation_mode=isolated-reassessment`, finalizer=`finalize-task-policy.sh`, mode=`pair`.
- Build closures: scope/context/verification/evidence/ownership/decision are true. Scores `1+2+2+2+1=G08`; base=`local-fit`, route=`recovery-boundary`, lane=`cloud`, canonical=`PLAN-cloud-G08.md`.
- Review closures are true. Scores `1+2+2+2+1=G08`; route=`official-review`, lane=`cloud`, canonical=`CODE_REVIEW-cloud-G08.md`.
- `large_indivisible_context=false`; positive loop risks=`temporal_state`, `concurrent_consistency`, `variant_product` (3); `review_rework_count=1`; `evidence_integrity_failure=true`; capability gap none.

## Implementation Checklist

- [ ] Make PostgreSQL/SQLite scheme selection, DDL, migrations, and query binding executable without fallback, with deterministic dialect regressions.
- [ ] Enforce exact one-row revision/status CAS, disabled-to-revoked lifecycle, and inactive-principal listing, with sequential, concurrent, and reopen tests.
- [ ] Run fresh package, race, vet, module, diff, and compiled Control Plane startup verification and record exact `iop/...` stdout/stderr.
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.

### [REVIEW_API-1] Make SQL dialect handling executable

**Problem:** `apps/control-plane/internal/credentialstore/store.go:37-41` can classify an unknown scheme containing `.db` as SQLite. `schema.go:23-46` sends `DATETIME` DDL to PostgreSQL, while `schema.go:107` and `principal.go:173` use SQLite `?` placeholders through `pgx`; a reachable configured PostgreSQL database therefore cannot complete the planned migration/query path.

**Solution:** Keep the selected dialect on `Store`, make schema statements explicit per dialect (or use types valid in both), execute statements individually in an idempotent migration boundary, and centralize placeholder binding so PostgreSQL receives `$1...$n` while SQLite keeps supported placeholders. Classify SQLite only for `file:` URLs or paths without `://`; never infer it from a `.db` substring in another scheme. Preserve `(nil, nil)` only for an exactly empty URL and never fall back from configured PostgreSQL.

Before (`apps/control-plane/internal/credentialstore/store.go:36-43`):

```go
case strings.HasPrefix(lower, "postgres://") || strings.HasPrefix(lower, "postgresql://"):
    return "pgx", nil
case strings.HasPrefix(lower, "file:") || strings.Contains(lower, ".db") || !strings.ContainsAny(lower, "://"):
    return "sqlite", nil
```

After:

```go
case strings.HasPrefix(lower, "postgres://") || strings.HasPrefix(lower, "postgresql://"):
    return dialectPostgres, nil
case strings.HasPrefix(lower, "file:") || !strings.Contains(lower, "://"):
    return dialectSQLite, nil
default:
    return "", unsupportedSchemeError(trimmed)
```

```go
type Store struct {
    db      *sql.DB
    dialect string
}

func (s *Store) bind(query string) string {
    return bindQuery(s.dialect, query)
}
```

**Modified Files and Checklist:**

- [ ] `apps/control-plane/internal/credentialstore/store.go`: retain dialect, reject ambiguous schemes, and provide deterministic query binding.
- [ ] `apps/control-plane/internal/credentialstore/schema.go`: emit executable idempotent statements and dialect-correct column inspection.
- [ ] `apps/control-plane/internal/credentialstore/principal.go`: send every parameterized query through the store binder.
- [ ] `apps/control-plane/internal/credentialstore/principal_test.go`: add scheme, bind-order, schema, migration/reopen, and query-path regressions.

**Test Strategy:** Add `TestDialectFromURLRejectsUnknownSchemeWithDBSuffix`, `TestBindQueryUsesPostgresOrdinals`, and table-driven dialect schema assertions that forbid `DATETIME`/`?` in PostgreSQL statements. Keep SQLite open/migrate/reopen tests to prove the shared execution path. No live PostgreSQL service is assumed; the dialect surface must be explicit enough for deterministic tests to validate every generated statement and bind form.

**Verification:** `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go test -count=1 ./apps/control-plane/internal/credentialstore -run 'Dialect|Bind|Schema|Open|Reopen'` exits zero.

### [REVIEW_API-2] Repair lifecycle CAS and inactive listing

**Problem:** `apps/control-plane/internal/credentialstore/principal.go:282-285` documents revoke from active or disabled but only accepts active. `casStatus` checks status before revision and ignores `RowsAffected` at lines 306-322, so a stale or concurrent loser is not guaranteed to return a typed CAS failure. `ListPrincipals` uses a left join but scans nullable token fields into `string`/`int64` at lines 203-206, which fails once no active token remains.

**Solution:** Perform one status-and-revision-fenced update, require exactly one affected row, and classify zero-row results from the current durable state without reporting success. Allow revoke from both active and disabled while keeping revoked terminal. Scan every optional joined token field through nullable values and return the principal with zero active-token metadata when none exists. Keep digest-only persistence and one-time raw-token return unchanged.

Before (`apps/control-plane/internal/credentialstore/principal.go:282-319`):

```go
func (s *Store) RevokeToken(ctx context.Context, principalID, tokenRef string, currentRevision int64) (*TokenRecord, error) {
    return s.casStatus(ctx, principalID, tokenRef, currentRevision, StatusActive, StatusRevoked)
}

_, err := tx.ExecContext(ctx,
    `UPDATE tokens SET status=?, revision=?, updated_at=? WHERE principal_id=? AND token_ref=? AND revision=?`,
    toStatus, newRev, now, principalID, tokenRef, rev,
)
```

After:

```go
result, err := tx.ExecContext(ctx, s.bind(`
    UPDATE tokens SET status=?, revision=?, updated_at=?, revoked_at=?
    WHERE principal_id=? AND token_ref=? AND revision=? AND status IN (?,?)`), args...)
if err != nil {
    return err
}
affected, err := result.RowsAffected()
if err != nil || affected != 1 {
    return classifyTransitionMiss(ctx, tx, principalID, tokenRef, currentRevision)
}
```

**Modified Files and Checklist:**

- [ ] `apps/control-plane/internal/credentialstore/principal.go`: implement exact affected-row CAS, active/disabled revoke, terminal revoked classification, and nullable active-token list scanning.
- [ ] `apps/control-plane/internal/credentialstore/principal_test.go`: cover stale revision precedence, revoke-after-disable, concurrent same-revision mutation, list-after-disable/revoke, and persistence after reopen.

**Test Strategy:** Add `TestRevokeDisabledToken`, `TestTokenMutationLosingCASDoesNotSucceed`, and `TestListPrincipalsWithoutActiveToken`. Update the stale-revision assertion to require `ErrRevisionMismatch` when the supplied revision is stale, prove exactly one concurrent mutation succeeds, prove the loser returns a typed non-success, and re-open the file store to confirm the final status/revision is durable and revoked remains terminal.

**Verification:** `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go test -count=1 -race ./apps/control-plane/internal/credentialstore` exits zero with fresh execution.

### [REVIEW_API-3] Rebuild trusted verification evidence

**Problem:** `CODE_REVIEW-cloud-G06.md:112-167` records `ip/apps/...` as actual stdout even though `go.mod:1` declares `module iop`; fresh reviewer commands print `iop/apps/...`. The changed `apps/control-plane/cmd/control-plane` startup path also lacks a compiled entrypoint cycle required by the testing domain rule.

**Solution:** Run the exact commands below after the repair, paste literal stdout/stderr into `CODE_REVIEW-cloud-G08.md`, and do not reconstruct package paths or durations. Build the Control Plane into a temporary directory, start it with a temporary configured SQLite database and ephemeral listeners, verify credential-store readiness/database creation, send SIGINT, and require clean shutdown. Do not write runtime tools or evidence into tracked repository paths.

**Modified Files and Checklist:**

- [ ] `agent-task/m-principal-provider-credential-slot-routing/01_principal_store/CODE_REVIEW-cloud-G08.md`: fill implementation notes, deviations, decisions, and literal command output.

**Test Strategy:** No new production test file is owned by this item; REVIEW_API-1/2 add regressions. This item verifies evidence provenance and the actual command entrypoint.

**Verification:** Run every command in `Final Verification`; every command exits zero and recorded Go package paths begin with `iop/`.

## Modified Files Summary

| File | Items |
|------|-------|
| `apps/control-plane/internal/credentialstore/store.go` | REVIEW_API-1 |
| `apps/control-plane/internal/credentialstore/schema.go` | REVIEW_API-1 |
| `apps/control-plane/internal/credentialstore/principal.go` | REVIEW_API-1, REVIEW_API-2 |
| `apps/control-plane/internal/credentialstore/principal_test.go` | REVIEW_API-1, REVIEW_API-2 |
| `agent-task/m-principal-provider-credential-slot-routing/01_principal_store/CODE_REVIEW-cloud-G08.md` | REVIEW_API-1, REVIEW_API-2, REVIEW_API-3 |

## Final Verification

Run from `/config/workspace/iop-s0`; fresh Go test output is required and cached-only output is not acceptable.

1. `command -v go && go version && go env GOROOT` — resolves the declared Go runner.
2. `mkdir -p .cache/go-build .cache/go-cache` — creates executable workspace-local cache paths.
3. `gofmt -w apps/control-plane/internal/credentialstore/store.go apps/control-plane/internal/credentialstore/schema.go apps/control-plane/internal/credentialstore/principal.go apps/control-plane/internal/credentialstore/principal_test.go` — formats only owned Go files.
4. `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go test -count=1 ./apps/control-plane/internal/credentialstore -run 'Dialect|Bind|Schema|Open|Reopen|RevokeDisabled|LosingCAS|ListPrincipalsWithoutActiveToken'` — targeted dialect and lifecycle regressions pass.
5. `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go test -count=1 -race ./apps/control-plane/internal/credentialstore` — the complete principal ledger lifecycle and concurrency suite passes.
6. `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go vet ./apps/control-plane/...` — no vet findings.
7. `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go test -count=1 ./apps/control-plane/...` — all Control Plane packages pass and output canonical `iop/apps/...` paths.
8. `GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go mod tidy -diff` — exits zero with no dependency diff.
9. Run this local compiled-entrypoint cycle; it must print a credential-store-ready log line, create the temporary DB, and exit cleanly after SIGINT:

```bash
runtime_dir="$(mktemp -d)"
runtime_pid=""
cleanup_runtime() {
  if [ -n "$runtime_pid" ] && kill -0 "$runtime_pid" 2>/dev/null; then
    kill -INT "$runtime_pid"
    wait "$runtime_pid"
  fi
  rm -rf "$runtime_dir"
}
trap cleanup_runtime EXIT
GOTMPDIR=/config/workspace/iop-s0/.cache/go-build GOCACHE=/config/workspace/iop-s0/.cache/go-cache go build -o "$runtime_dir/control-plane" ./apps/control-plane/cmd/control-plane
IOP_LISTEN=127.0.0.1:0 IOP_WIRE_LISTEN=127.0.0.1:0 IOP_EDGE_WIRE_LISTEN=127.0.0.1:0 IOP_DATABASE_URL="$runtime_dir/credentials.db" "$runtime_dir/control-plane" serve --config "" >"$runtime_dir/stdout.log" 2>"$runtime_dir/stderr.log" &
runtime_pid=$!
for attempt in $(seq 1 50); do
  if [ -f "$runtime_dir/credentials.db" ]; then
    break
  fi
  if ! kill -0 "$runtime_pid" 2>/dev/null; then
    sed -n '1,160p' "$runtime_dir/stdout.log" "$runtime_dir/stderr.log"
    exit 1
  fi
  sleep 0.1
done
test -f "$runtime_dir/credentials.db"
rg --fixed-strings 'control-plane credential store ready' "$runtime_dir/stdout.log" "$runtime_dir/stderr.log"
kill -INT "$runtime_pid"
wait "$runtime_pid"
runtime_pid=""
```

10. `git diff --check` — no whitespace errors.

After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.
