feat: alpha upload V2 pipeline with hardened telemetry contract

[WellDunDun]

Implements the complete alpha remote data pipeline (V2 push architecture) with lossless canonical ingest staging, hardened telemetry contract, and constitutional evolution guards.

Key additions: alpha-upload module with queue, staging, payload builders, and retry-safe client; Zod-validated telemetry schemas with execution_fact_id and partial push support; constitutional evolution for skill descriptions; comprehensive e2e/integration tests; dashboard Status page and observability layer.

All changes maintain append-only JSONL, SQLite materialization, and zero runtime dependencies in the core CLI.

[coderabbitai]

Actionable comments posted: 16

♻️ Duplicate comments (12)

skill/Workflows/Doctor.md (1)

19-55: ⚠️ Potential issue | 🟡 Minor

The example JSON still shows only 4 checks but claims total: 10.

The checks array in the example contains 4 entries (config, log_session_telemetry, hook_settings, dashboard_freshness_mode) but the summary object reports "total": 10 and the documentation states "10 checks total currently" on line 82. Either show all 10 checks in the example or clearly mark it as truncated with ... and a comment like // ... 6 more checks.

As per coding guidelines, "skill/**/*.md: Skill workflow documents for AI agents. Review for: Valid code examples".

📝 Suggested fix: Mark the example as truncated

     {
       "name": "dashboard_freshness_mode",
       "status": "warn",
       "message": "Dashboard still uses legacy JSONL watcher invalidation"
-    }
+    },
+    // ... 6 more checks (log_skill_usage, log_all_queries, log_evolution_audit, 
+    // evolution_audit, skill_version_sync, version_up_to_date)
   ],

Or show all 10 checks explicitly so the example is a valid, complete reference.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skill/Workflows/Doctor.md` around lines 19 - 55, The example JSON in
Doctor.md has a mismatch: the "checks" array lists 4 entries while
"summary.total" is 10 (and the doc text says "10 checks total"); fix by either
adding the remaining six check objects to the "checks" array so it truly
contains 10 entries (update the check objects to match existing schema) or
explicitly mark the example as truncated by replacing the omitted entries with
an ellipsis/comment (e.g., add a placeholder like // ... 6 more checks) and keep
"summary.total": 10; ensure you modify the "checks" array and/or the
"summary.total" field consistently so the example is accurate.

cli/selftune/localdb/materialize.ts (1)

96-103: ⚠️ Potential issue | 🟠 Major

Boundary safety still needs key-level comparison.

Lines 96-103 and 125-141 only compare boundary row counts. That can still miss data loss when SQLite and JSONL hold different rows at jsonlMax but happen to have the same count, and it can also block safe rebuilds when the JSONL boundary contains duplicate records that INSERT OR IGNORE later dedupes. Compare the distinct conflict keys at the boundary timestamp instead of raw counts.

Also applies to: 125-141

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/localdb/materialize.ts` around lines 96 - 103, The current
boundary check only compares jsonlBoundaryCount and the SQLite boundary count
which can miss mismatched rows or false positives due to duplicates; instead,
compute the set of distinct conflict keys for records at jsonlMax (use readJsonl
output 'records' filtered by jsonlMax and map to the conflict key fields) and
compute the equivalent distinct key set from SQLite's boundary rows, then
compare the two sets for equality (not just lengths) before deciding whether a
rebuild is safe or blocked; update the logic that sets
jsonlMax/jsonlBoundaryCount and the corresponding SQLite-boundary comparison
(lines around where jsonlMax, jsonlBoundaryCount are used and the later block at
125-141) to use distinct key sets so INSERT OR IGNORE deduplication and
differing rows are handled correctly.

cli/selftune/localdb/direct-write.ts (1)

184-196: ⚠️ Potential issue | 🟠 Major

Keep the direct-write telemetry row monotonic.

This upsert still does last-write-wins and never refreshes cwd, transcript_path, or source. cli/selftune/localdb/materialize.ts Lines 502-518 already guard the same row by timestamp, so a replayed or out-of-order direct write can regress session_telemetry even though rebuilds no longer do.

🛠 Proposed fix

       ON CONFLICT(session_id) DO UPDATE SET
         timestamp = excluded.timestamp,
+        cwd = COALESCE(excluded.cwd, session_telemetry.cwd),
+        transcript_path = COALESCE(excluded.transcript_path, session_telemetry.transcript_path),
+        source = COALESCE(excluded.source, session_telemetry.source),
         tool_calls_json = excluded.tool_calls_json,
         total_tool_calls = excluded.total_tool_calls,
         bash_commands_json = excluded.bash_commands_json,
         skills_triggered_json = excluded.skills_triggered_json,
         skills_invoked_json = excluded.skills_invoked_json,
         assistant_turns = excluded.assistant_turns,
         errors_encountered = excluded.errors_encountered,
         transcript_chars = excluded.transcript_chars,
         last_user_query = excluded.last_user_query,
         input_tokens = COALESCE(excluded.input_tokens, session_telemetry.input_tokens),
         output_tokens = COALESCE(excluded.output_tokens, session_telemetry.output_tokens)
+      WHERE session_telemetry.timestamp IS NULL
+         OR excluded.timestamp >= session_telemetry.timestamp

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/localdb/direct-write.ts` around lines 184 - 196, The upsert
currently allows out-of-order writes to regress immutable metadata (cwd,
transcript_path, source) because DO UPDATE blindly overwrites those columns;
change the UPDATE assignments for cwd, transcript_path, and source to only
accept excluded values when excluded.timestamp >= session_telemetry.timestamp
(e.g., use CASE WHEN excluded.timestamp >= session_telemetry.timestamp THEN
excluded.cwd ELSE session_telemetry.cwd END and similarly for transcript_path
and source) so these metadata fields remain monotonic while keeping the existing
COALESCE/last-write behavior for other columns.

tests/init/alpha-onboarding-e2e.test.ts (1)

71-80: ⚠️ Potential issue | 🟠 Major

Avoid a literal live-looking key in the fixture.

st_live_abc123xyz already matches secret-scanning heuristics, so this test can block CI or force an allowlist for a fake credential. Build the value at runtime while keeping the st_live_ prefix.

Suggested fix

+    const testApiKey = ["st_live", "TEST_ONLY_E2E"].join("_");
     const config2 = runInit(
       makeInitOpts({
         alpha: true,
         alphaEmail: "user@example.com",
-        alphaKey: "st_live_abc123xyz",
+        alphaKey: testApiKey,
         force: true,
       }),
     );
 
-    expect(config2.alpha?.api_key).toBe("st_live_abc123xyz");
+    expect(config2.alpha?.api_key).toBe(testApiKey);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/init/alpha-onboarding-e2e.test.ts` around lines 71 - 80, The test is
using a literal "live-looking" API key; change the fixture to construct the key
at runtime instead of hardcoding "st_live_abc123xyz" so it won't trigger secret
scanners: in the test that calls runInit(makeInitOpts(...)) (symbols: runInit,
makeInitOpts, config2) replace the alphaKey value with a runtime-built string
that preserves the "st_live_" prefix plus a generated or static non-secret
suffix (e.g., concatenation of "st_live_" with a random UUID or "testkey") and
assert against that constructed value when checking config2.alpha?.api_key.

cli/selftune/index.ts (1)

611-660: ⚠️ Potential issue | 🟠 Major

Reject invalid stored API keys before staging anything.

This guard only checks for a non-empty string. A manually edited or corrupted config.alpha.api_key like "bad_key_format" still reaches runUploadCycle(), which prepares/enqueues rows before remote auth fails. Reuse isValidApiKeyFormat() or gate on getAlphaLinkState(identity) === "ready" here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/index.ts` around lines 611 - 660, The code calls runUploadCycle
with identity.api_key after only checking for non-empty strings; reject invalid
stored API keys earlier by validating identity.api_key before calling
runUploadCycle — e.g., use isValidApiKeyFormat(identity.api_key) or require
getAlphaLinkState(identity) === "ready" (both available utilities) and, if
invalid, print the same enrolled:true JSON status with zeros and the existing
error message about missing/invalid credentials and exit; update the branch
around readAlphaIdentity(SELFTUNE_CONFIG_PATH) to gate runUploadCycle on that
validation so no staging/preparation occurs with a malformed key.

cli/selftune/init.ts (3)

452-462: ⚠️ Potential issue | 🟠 Major

runInit() still ignores alpha mutations on existing configs.

cliMain() now bypasses its own "already initialized" exit when alpha flags are present, but this guard still returns the stored config before opts.alpha / opts.noAlpha are applied. Existing installs still need --force for alpha enrollment changes to take effect.

Suggested fix

 export function runInit(opts: InitOptions): SelftuneConfig {
   const { configDir, configPath, force } = opts;
+  const hasAlphaMutation =
+    opts.alpha ||
+    opts.noAlpha ||
+    opts.alphaEmail !== undefined ||
+    opts.alphaName !== undefined ||
+    opts.alphaKey !== undefined;
 
   // If config exists and no --force, return existing
-  if (!force && existsSync(configPath)) {
+  if (!force && !hasAlphaMutation && existsSync(configPath)) {
     const raw = readFileSync(configPath, "utf-8");
     try {
       return JSON.parse(raw) as SelftuneConfig;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/init.ts` around lines 452 - 462, runInit() currently returns the
existing JSON config early when !force && existsSync(configPath), which ignores
alpha flag mutations from cliMain() (opts.alpha / opts.noAlpha); change this
flow so that after reading and parsing the existing config (in runInit), detect
if opts.alpha or opts.noAlpha were passed and differ from the stored
SelftuneConfig.alpha value and, if so, update the in-memory config and persist
it (overwrite configPath) instead of returning immediately; keep the same error
handling for invalid JSON and still respect force to fully reinitialize when
requested.

---

648-648: ⚠️ Potential issue | 🔴 Critical

Redact alpha.api_key before printing the config.

Line 648 emits the full config after enrollment. Because Lines 565-566 persist config.alpha.api_key, selftune init leaks the live bearer token to stdout, shell history, CI logs, and agent transcripts.

Suggested fix

-  console.log(JSON.stringify(config, null, 2));
+  const printableConfig: SelftuneConfig =
+    config.alpha?.api_key !== undefined
+      ? { ...config, alpha: { ...config.alpha, api_key: "<redacted>" } }
+      : config;
+  console.log(JSON.stringify(printableConfig, null, 2));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/init.ts` at line 648, The printed config currently exposes the
live bearer token; before calling console.log(JSON.stringify(config, null, 2))
redact or remove the secret by mutating or cloning the config object (e.g., copy
config to safeConfig and set safeConfig.alpha.api_key = '[REDACTED]' or delete
safeConfig.alpha.api_key) and then JSON.stringify and print safeConfig instead;
ensure you reference the same variable name "config" and the exact print site to
avoid leaking the token.

---

499-501: ⚠️ Potential issue | 🟠 Major

Validate and merge the alpha identity before the first write.

The base config is persisted at Lines 499-501 before the alpha branch validates --alpha-email / --alpha-key. That leaves partial reinitializations on disk when enrollment fails, and the fresh identity object here also drops stored fields such as api_key and cloud_user_id unless the caller passes them again.

Also applies to: 533-566

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/init.ts` around lines 499 - 501, The current flow writes the
base config (using mkdirSync(configDir) and writeFileSync(configPath,
JSON.stringify(config,...)) before validating/merging the alpha identity, which
can leave a partial config on disk and drop stored fields (e.g. api_key,
cloud_user_id); update the init flow to validate the alpha flags and merge the
resulting identity into the in-memory config.identity (preserving any existing
stored fields) before any write to disk, and only call mkdirSync/writeFileSync
after successful alpha enrollment/validation; apply the same change for the
later block referenced around lines 533-566 so both write paths validate and
merge alpha identity first.

cli/selftune/alpha-upload/build-payloads.ts (2)

94-109: ⚠️ Potential issue | 🟠 Major

This builder is still lossy for evolution evidence.

These entries preserve timestamp, skill_path, and details, but the call at Line 128 hands them to buildPushPayloadV2(), which currently serializes canonical.evolution_evidence without those fields in cli/selftune/canonical-export.ts (Lines 82-121). The staging row stays intact here and then loses data during payload assembly.

Also applies to: 128-128

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/build-payloads.ts` around lines 94 - 109, The
evolution evidence being constructed in build-payloads (the evidenceEntries
object pushed in buildPushPayloadV2's input) includes fields like timestamp,
skill_path and details but those fields are dropped when
canonical.evolution_evidence is serialized in buildPushPayloadV2 /
cli/selftune/canonical-export.ts; fix by updating the canonical export
serialization logic (the code that builds canonical.evolution_evidence in
canonical-export.ts) to include and preserve timestamp, skill_path, details (and
other optional fields such as original_text, proposed_text, eval_set,
validation, evidence_id, confidence, rationale, stage, target) so the payload
consumer receives the full EvolutionEvidenceEntry shape, or alternatively change
buildPushPayloadV2 to accept and forward the full evidenceEntries object
unchanged.

---

78-80: ⚠️ Potential issue | 🟠 Major

Do not advance lastSeq past skipped staging rows.

Malformed JSON and invalid evolution_evidence rows are skipped with continue, but lastSeq still comes from the last fetched row. Once the caller persists that cursor, those rows are lost permanently instead of being retried or surfaced. Track the last accepted local_seq or fail the batch.

Also applies to: 91-91, 128-129

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/build-payloads.ts` around lines 78 - 80, The loop
that sets lastSeq currently advances using the last fetched row even when rows
are skipped (e.g., parsed JSON invalid or invalid evolution_evidence), which
loses those rows; change this by introducing a lastAcceptedSeq (or
acceptedLocalSeq) variable and only update it when a row is successfully parsed
and accepted (inside the for loop where safeParseJson(...) and
evolution_evidence validation pass), leave it unchanged when you continue, and
after the loop assign lastSeq from lastAcceptedSeq (or fail the batch if no rows
were accepted) so the persisted cursor comes from the last accepted local_seq
rather than the last fetched row.

tests/alpha-upload/integration.test.ts (1)

166-186: ⚠️ Potential issue | 🟠 Major

Seed evolution_evidence with the real V2 identifier/envelope.

This helper still inserts legacy evidence rows: record_id is ${proposal_id}:${stage}:... and record_json omits evidence_id. That means the integration suite is not exercising the same staging shape as the production pipeline, so evidence-contract regressions can slip through unnoticed. Please reuse the production staging helper here, or derive the same deterministic evidence_id and store it in both record_id and record_json. The copied helper in tests/alpha-upload/e2e.test.ts needs the same change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/alpha-upload/integration.test.ts` around lines 166 - 186, The seeded
evolution evidence helper stageEvolutionEvidence is inserting legacy rows
without the V2 envelope/evidence_id; update it to compute the same deterministic
evidence_id the production staging uses (e.g., derive evidence_id from
proposal_id, stage and timestamp) and then use that evidence_id in both the
record_id and inside record_json (add an evidence_id field in the record object
and set record_id to that envelope value) so the tests exercise the V2 staging
shape; also apply the identical change to the duplicated helper in
tests/alpha-upload/e2e.test.ts or refactor both tests to call the shared
production staging helper.

tests/alpha-upload/flush.test.ts (1)

389-391: ⚠️ Potential issue | 🟡 Minor

Drop the remaining non-null assertion in this expectation.

If getPending() stops being called, queue.calls.getPending[0]! throws before the assertion explains what broke. Guard the first call, then assert its first argument.

Suggested fix

-    expect(queue.calls.getPending[0]![0]).toBe(2);
+    const firstGetPending = queue.calls.getPending[0];
+    expect(firstGetPending).toBeDefined();
+    expect(firstGetPending?.[0]).toBe(2);

As per coding guidelines, tests/**/*.ts: "Optional chaining (result?.) not non-null assertions (result!.)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/alpha-upload/flush.test.ts` around lines 389 - 391, The test currently
uses a non-null assertion on queue.calls.getPending[0] which can throw before
the assertion; change it to guard the first call and then assert its first
argument by using optional chaining on queue.calls.getPending (or check that
queue.calls.getPending?.[0] is defined) and then assert the element at index 0;
update the expectation around the first call to avoid the `!` operator
(referencing flushQueue and queue.calls.getPending) so the test fails with a
clear assertion rather than a runtime exception.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 284-297: The useEffect that syncs the "proposal" query param is
declared after conditional early returns, violating hook order; move the
useEffect (the effect referencing activeProposal, searchParams, setSearchParams)
up so it runs unconditionally in the component body (e.g., alongside other
hooks) but keep its internal logic conditional (check activeProposal and current
first), and then remove any duplicate declarations and the original late
useEffect block to ensure hook order remains stable.

In `@cli/selftune/alpha-identity.ts`:
- Around line 64-65: The writeAlphaIdentity function currently creates the
directory with mkdirSync(dirname(configPath), { recursive: true }) and writes
the config with writeFileSync(configPath, JSON.stringify(config, null, 2),
"utf-8") but does not enforce file permissions; update writeAlphaIdentity to
write the file with restrictive permissions by passing the mode option (0o600)
to writeFileSync (or, alternatively, call chmodSync(configPath, 0o600)
immediately after writing) so the saved config containing api_key is created
with owner-read/write only; reference writeAlphaIdentity, configPath,
writeFileSync (and mkdirSync/chmodSync if using the alternative).

In `@cli/selftune/alpha-upload/client.ts`:
- Around line 63-66: When a parsed JSON satisfies isPushUploadResult but came
from a non-OK HTTP response, preserve the HTTP status by attaching it to the
returned object so downstream logic (e.g., flushQueue) doesn't treat it as
status 0; modify the branch in client.ts that checks isPushUploadResult(parsed)
to set parsed._status = response.status (or otherwise copy the response status
into the PushUploadResult) before returning, ensuring any validated error body
retains the original HTTP status for retry/exhaustion logic.

In `@cli/selftune/alpha-upload/flush.ts`:
- Around line 104-106: The exhausted-retries branch uses markFailedSafely (which
delegates to queue.markFailed) but that SQLite adapter only updates rows with
status "sending", so items at pre-send "pending" remain in pending; change the
branch handling when item.attempts >= maxRetries to perform a dedicated
pending→failed transition (either extend queue.markFailed to accept a
from-status or add a new queue method like
markPendingFailed/transitionPendingToFailed) and ensure the item is removed from
pending and recorded as failed (increment summary.failed remains), so pending
rows don't persist and block future uploads.

In `@cli/selftune/alpha-upload/index.ts`:
- Around line 80-86: The function prepareUploads currently declares unused
parameters _userId, _agentType, and _selftuneVersion; remove these parameters
from the prepareUploads signature (and any corresponding destructuring) and
update the call site in runUploadCycle to stop passing them, or alternatively if
they are needed later, use them (e.g., include them in PrepareResult
construction or internal logic) — adjust both prepareUploads and runUploadCycle
so the parameter list and calls match and eliminate the dead variables.
- Around line 169-174: The code is unsafely casting getPendingUploads(...) to
ContractQueueItem[]; update queueOps.getPending to perform runtime
validation/normalization instead of the assertion: call getPendingUploads(db,
limit), iterate its QueueItem[] result and validate/convert each item’s
payload_type and status into the contract-safe AlphaPayloadType and
QueueItemStatus (or filter out/throw on invalid values), and return the
resulting ContractQueueItem[]; keep queueOps and the other functions
(markSending, markSent, markFailed) unchanged but reference ContractQueueItem in
the mapping so callers receive a properly validated array.

In `@cli/selftune/alpha-upload/queue.ts`:
- Around line 140-147: The upsert into upload_watermarks currently overwrites
last_uploaded_id unconditionally, allowing regressions; modify the DO UPDATE
clause in the db.run calls that insert from maxByType (where variables
payloadType, maxId, now are used) so that last_uploaded_id is set to the greater
of the existing value and the incoming value (i.e., use MAX(existing,
excluded.last_uploaded_id) or equivalent) and do the same change for the second
identical upsert occurrence elsewhere in the file so watermarks remain
monotonic.

In `@cli/selftune/index.ts`:
- Around line 663-664: The switch branch that calls process.exit(result.failed >
0 ? 1 : 0) does not explicitly terminate the switch, causing a
noFallthroughSwitchClause lint failure for the subsequent upload case; update
the switch case that logs the result (the branch containing
console.log(JSON.stringify(result, null, 2)) and process.exit(...)) to
explicitly terminate—either add a break (or return) immediately after the
process.exit(...) call or replace process.exit with a return so the switch
cannot fall through into the upload case.

In `@cli/selftune/localdb/materialize.ts`:
- Around line 44-49: PROTECTED_TABLES is now unused and duplicates metadata
found in protectedTables; remove the dead constant or derive protectedTables
from it to keep a single source of truth. Update the module so either (a) delete
the PROTECTED_TABLES declaration and use only protectedTables everywhere, or (b)
keep PROTECTED_TABLES as the canonical const and build protectedTables from it
(preserving override/override-aware logic), making sure references to
EVOLUTION_AUDIT_LOG, EVOLUTION_EVIDENCE_LOG, ORCHESTRATE_RUN_LOG and the
tsColumn/table fields remain consistent; adjust any code that currently
references protectedTables/PROTECTED_TABLES so only the canonical symbol is
used.

In `@cli/selftune/localdb/queries.ts`:
- Around line 641-644: The age calculation can produce NaN when row.created_at
is invalid; update the logic that computes ageMs (the block using
row.created_at, ageMs, and Math.floor) to validate the parsed date before using
it: create a Date from row.created_at, check if date.getTime() is NaN (or use
Number.isFinite/Number.isNaN on the timestamp), and if invalid return null (or
another explicit fallback) instead of proceeding to Math.floor; otherwise
compute and return Math.floor((Date.now() - date.getTime()) / 1000).

In `@cli/selftune/orchestrate.ts`:
- Around line 479-482: The two non-null assertions on Map#get results (const
setA = evalSets.get(skillNames[i])! and const setB =
evalSets.get(skillNames[j])!) must be replaced with explicit guards: call
evalSets.get(...) into potentially undefined variables (setA, setB) and then
check (e.g., if (!setA || !setB) continue or throw a descriptive Error) before
using them; update the nested loop in orchestrate.ts that iterates skillNames to
perform these runtime null checks instead of using `!` so strict null checking
passes.

In `@cli/selftune/status.ts`:
- Around line 418-430: The status code only sets alphaInfo when
alphaIdentity.enrolled is true, which drops unenrolled-but-linked identities;
change the logic that creates AlphaStatusInfo so it runs whenever
readAlphaIdentity(...) returns a non-null alphaIdentity (not just when
alphaIdentity.enrolled is true), set enrolled to alphaIdentity.enrolled, call
getAlphaLinkState(alphaIdentity) to populate linkState, and still populate
stats/lastError/lastSuccess callers (getQueueStats, getLastUploadError,
getLastUploadSuccess) before calling formatAlphaStatus(alphaInfo) so
"linked_not_enrolled" is displayed correctly.

In `@cli/selftune/utils/selftune-meta.ts`:
- Around line 32-34: The current return unsafely casts config.agent_type to
SelftuneConfig["agent_type"] even when the string may not be one of the allowed
union members; change the logic in this function to validate config.agent_type
against the allowed values (e.g., an array or Set of
["claude_code","codex","opencode","openclaw","unknown"]) and only return it when
included, otherwise return fallback; reference config.agent_type,
SelftuneConfig["agent_type"], and fallback to locate and update the code
(implement a simple inclusion check or a type guard instead of the direct cast).

In `@scripts/sync-skill-version.ts`:
- Around line 37-43: The replacement always injects hardcoded '\n', which
normalizes CRLF and causes unnecessary file rewrites; change the logic that
builds updated (where content.replace/frontmatterRegex and updatedFrontmatter
are used) to preserve the original newline sequence captured from the matched
frontmatter (e.g., extract the newline from the frontmatterRegex match groups or
detect the linebreak used in content) and use that sequence instead of '\n' when
joining the frontmatter boundaries, then write to skillPath and log pkgVersion
as before.

In `@skill/Workflows/Evolve.md`:
- Around line 206-224: The doc and code disagree on the description length
limit: Workflows/Evolve.md says ≤1024 chars but constitutional.ts enforces 8192;
make them consistent by changing the implementation to enforce 1024 characters
(update the constant/variable used in constitutional.ts, e.g.,
MAX_DESCRIPTION_LENGTH / the validateProposalLength or validateProposal
function) and update any related violation messages/tests that reference the
limit so they report "≤1024 characters" exactly. Ensure the same symbol(s)
(constant name and the validator function) are updated so both behavior and
error text match the documentation.

In `@tests/alpha-upload/build-payloads.test.ts`:
- Around line 29-34: The test setup currently swallows all errors when executing
POST_MIGRATION_INDEXES via db.run, which hides real DDL problems; change the
code so index creation errors are not silently ignored—remove the broad
try/catch around db.run for POST_MIGRATION_INDEXES (or replace it with handling
that only ignores the specific "already exists" error), ensuring any other
SQL/DDL errors thrown by db.run surface and fail the test immediately.

---

Duplicate comments:
In `@cli/selftune/alpha-upload/build-payloads.ts`:
- Around line 94-109: The evolution evidence being constructed in build-payloads
(the evidenceEntries object pushed in buildPushPayloadV2's input) includes
fields like timestamp, skill_path and details but those fields are dropped when
canonical.evolution_evidence is serialized in buildPushPayloadV2 /
cli/selftune/canonical-export.ts; fix by updating the canonical export
serialization logic (the code that builds canonical.evolution_evidence in
canonical-export.ts) to include and preserve timestamp, skill_path, details (and
other optional fields such as original_text, proposed_text, eval_set,
validation, evidence_id, confidence, rationale, stage, target) so the payload
consumer receives the full EvolutionEvidenceEntry shape, or alternatively change
buildPushPayloadV2 to accept and forward the full evidenceEntries object
unchanged.
- Around line 78-80: The loop that sets lastSeq currently advances using the
last fetched row even when rows are skipped (e.g., parsed JSON invalid or
invalid evolution_evidence), which loses those rows; change this by introducing
a lastAcceptedSeq (or acceptedLocalSeq) variable and only update it when a row
is successfully parsed and accepted (inside the for loop where
safeParseJson(...) and evolution_evidence validation pass), leave it unchanged
when you continue, and after the loop assign lastSeq from lastAcceptedSeq (or
fail the batch if no rows were accepted) so the persisted cursor comes from the
last accepted local_seq rather than the last fetched row.

In `@cli/selftune/index.ts`:
- Around line 611-660: The code calls runUploadCycle with identity.api_key after
only checking for non-empty strings; reject invalid stored API keys earlier by
validating identity.api_key before calling runUploadCycle — e.g., use
isValidApiKeyFormat(identity.api_key) or require getAlphaLinkState(identity) ===
"ready" (both available utilities) and, if invalid, print the same enrolled:true
JSON status with zeros and the existing error message about missing/invalid
credentials and exit; update the branch around
readAlphaIdentity(SELFTUNE_CONFIG_PATH) to gate runUploadCycle on that
validation so no staging/preparation occurs with a malformed key.

In `@cli/selftune/init.ts`:
- Around line 452-462: runInit() currently returns the existing JSON config
early when !force && existsSync(configPath), which ignores alpha flag mutations
from cliMain() (opts.alpha / opts.noAlpha); change this flow so that after
reading and parsing the existing config (in runInit), detect if opts.alpha or
opts.noAlpha were passed and differ from the stored SelftuneConfig.alpha value
and, if so, update the in-memory config and persist it (overwrite configPath)
instead of returning immediately; keep the same error handling for invalid JSON
and still respect force to fully reinitialize when requested.
- Line 648: The printed config currently exposes the live bearer token; before
calling console.log(JSON.stringify(config, null, 2)) redact or remove the secret
by mutating or cloning the config object (e.g., copy config to safeConfig and
set safeConfig.alpha.api_key = '[REDACTED]' or delete safeConfig.alpha.api_key)
and then JSON.stringify and print safeConfig instead; ensure you reference the
same variable name "config" and the exact print site to avoid leaking the token.
- Around line 499-501: The current flow writes the base config (using
mkdirSync(configDir) and writeFileSync(configPath, JSON.stringify(config,...))
before validating/merging the alpha identity, which can leave a partial config
on disk and drop stored fields (e.g. api_key, cloud_user_id); update the init
flow to validate the alpha flags and merge the resulting identity into the
in-memory config.identity (preserving any existing stored fields) before any
write to disk, and only call mkdirSync/writeFileSync after successful alpha
enrollment/validation; apply the same change for the later block referenced
around lines 533-566 so both write paths validate and merge alpha identity
first.

In `@cli/selftune/localdb/direct-write.ts`:
- Around line 184-196: The upsert currently allows out-of-order writes to
regress immutable metadata (cwd, transcript_path, source) because DO UPDATE
blindly overwrites those columns; change the UPDATE assignments for cwd,
transcript_path, and source to only accept excluded values when
excluded.timestamp >= session_telemetry.timestamp (e.g., use CASE WHEN
excluded.timestamp >= session_telemetry.timestamp THEN excluded.cwd ELSE
session_telemetry.cwd END and similarly for transcript_path and source) so these
metadata fields remain monotonic while keeping the existing COALESCE/last-write
behavior for other columns.

In `@cli/selftune/localdb/materialize.ts`:
- Around line 96-103: The current boundary check only compares
jsonlBoundaryCount and the SQLite boundary count which can miss mismatched rows
or false positives due to duplicates; instead, compute the set of distinct
conflict keys for records at jsonlMax (use readJsonl output 'records' filtered
by jsonlMax and map to the conflict key fields) and compute the equivalent
distinct key set from SQLite's boundary rows, then compare the two sets for
equality (not just lengths) before deciding whether a rebuild is safe or
blocked; update the logic that sets jsonlMax/jsonlBoundaryCount and the
corresponding SQLite-boundary comparison (lines around where jsonlMax,
jsonlBoundaryCount are used and the later block at 125-141) to use distinct key
sets so INSERT OR IGNORE deduplication and differing rows are handled correctly.

In `@skill/Workflows/Doctor.md`:
- Around line 19-55: The example JSON in Doctor.md has a mismatch: the "checks"
array lists 4 entries while "summary.total" is 10 (and the doc text says "10
checks total"); fix by either adding the remaining six check objects to the
"checks" array so it truly contains 10 entries (update the check objects to
match existing schema) or explicitly mark the example as truncated by replacing
the omitted entries with an ellipsis/comment (e.g., add a placeholder like //
... 6 more checks) and keep "summary.total": 10; ensure you modify the "checks"
array and/or the "summary.total" field consistently so the example is accurate.

In `@tests/alpha-upload/flush.test.ts`:
- Around line 389-391: The test currently uses a non-null assertion on
queue.calls.getPending[0] which can throw before the assertion; change it to
guard the first call and then assert its first argument by using optional
chaining on queue.calls.getPending (or check that queue.calls.getPending?.[0] is
defined) and then assert the element at index 0; update the expectation around
the first call to avoid the `!` operator (referencing flushQueue and
queue.calls.getPending) so the test fails with a clear assertion rather than a
runtime exception.

In `@tests/alpha-upload/integration.test.ts`:
- Around line 166-186: The seeded evolution evidence helper
stageEvolutionEvidence is inserting legacy rows without the V2
envelope/evidence_id; update it to compute the same deterministic evidence_id
the production staging uses (e.g., derive evidence_id from proposal_id, stage
and timestamp) and then use that evidence_id in both the record_id and inside
record_json (add an evidence_id field in the record object and set record_id to
that envelope value) so the tests exercise the V2 staging shape; also apply the
identical change to the duplicated helper in tests/alpha-upload/e2e.test.ts or
refactor both tests to call the shared production staging helper.

In `@tests/init/alpha-onboarding-e2e.test.ts`:
- Around line 71-80: The test is using a literal "live-looking" API key; change
the fixture to construct the key at runtime instead of hardcoding
"st_live_abc123xyz" so it won't trigger secret scanners: in the test that calls
runInit(makeInitOpts(...)) (symbols: runInit, makeInitOpts, config2) replace the
alphaKey value with a runtime-built string that preserves the "st_live_" prefix
plus a generated or static non-secret suffix (e.g., concatenation of "st_live_"
with a random UUID or "testkey") and assert against that constructed value when
checking config2.alpha?.api_key.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 74160e1e-ef7f-4842-b236-8867dc231a95

📥 Commits

Reviewing files that changed from the base of the PR and between 91ba870 and 0b27dfa.

📒 Files selected for processing (34)

• AGENTS.md
• README.md
• apps/local-dashboard/src/pages/SkillReport.tsx
• cli/selftune/alpha-identity.ts
• cli/selftune/alpha-upload-contract.ts
• cli/selftune/alpha-upload/build-payloads.ts
• cli/selftune/alpha-upload/client.ts
• cli/selftune/alpha-upload/flush.ts
• cli/selftune/alpha-upload/index.ts
• cli/selftune/alpha-upload/queue.ts
• cli/selftune/dashboard-server.ts
• cli/selftune/eval/synthetic-evals.ts
• cli/selftune/evolution/evolve-body.ts
• cli/selftune/index.ts
• cli/selftune/init.ts
• cli/selftune/localdb/direct-write.ts
• cli/selftune/localdb/materialize.ts
• cli/selftune/localdb/queries.ts
• cli/selftune/orchestrate.ts
• cli/selftune/routes/skill-report.ts
• cli/selftune/status.ts
• cli/selftune/utils/selftune-meta.ts
• docs/design-docs/evolution-pipeline.md
• scripts/sync-skill-version.ts
• skill/Workflows/Doctor.md
• skill/Workflows/Evolve.md
• skill/Workflows/Sync.md
• tests/alpha-upload/build-payloads.test.ts
• tests/alpha-upload/e2e.test.ts
• tests/alpha-upload/flush.test.ts
• tests/alpha-upload/integration.test.ts
• tests/alpha-upload/queue.test.ts
• tests/evolution/constitutional.test.ts
• tests/init/alpha-onboarding-e2e.test.ts

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

useEffect called after early returns violates Rules of Hooks.

The useEffect at line 284 is placed after conditional returns (lines 195, 203, 218, 231, 249), meaning React's hook order varies between renders. This can cause runtime errors or stale closures.

Proposed fix — move hook to top with conditional body

 export function SkillReport() {
   const { name } = useParams<{ name: string }>()
   const [searchParams, setSearchParams] = useSearchParams()
   const { data, isPending, isError, error, refetch } = useSkillReport(name)

+  // Derive proposal selection — safe to compute even when data is loading
+  const evolution = data?.evolution ?? []
+  const proposalIds = new Set(evolution.map((entry) => entry.proposal_id))
+  const requestedProposal = searchParams.get("proposal")
+  const activeProposal = requestedProposal && proposalIds.has(requestedProposal)
+    ? requestedProposal
+    : (evolution.length > 0 ? evolution[0].proposal_id : null)
+
+  useEffect(() => {
+    if (!data) return // skip URL sync until data loads
+    const current = searchParams.get("proposal")
+    if (activeProposal && current !== activeProposal) {
+      const next = new URLSearchParams(searchParams)
+      next.set("proposal", activeProposal)
+      setSearchParams(next, { replace: true })
+      return
+    }
+    if (!activeProposal && current) {
+      const next = new URLSearchParams(searchParams)
+      next.delete("proposal")
+      setSearchParams(next, { replace: true })
+    }
+  }, [activeProposal, data, searchParams, setSearchParams])
+
   if (!name) {
     return (
       // ...

Then remove the duplicate declarations and the useEffect from lines 278-297.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/local-dashboard/src/pages/SkillReport.tsx` around lines 284 - 297, The
useEffect that syncs the "proposal" query param is declared after conditional
early returns, violating hook order; move the useEffect (the effect referencing
activeProposal, searchParams, setSearchParams) up so it runs unconditionally in
the component body (e.g., alongside other hooks) but keep its internal logic
conditional (check activeProposal and current first), and then remove any
duplicate declarations and the original late useEffect block to ensure hook
order remains stable.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

This branch still leaves exhausted rows in pending.

markFailedSafely() delegates to queue.markFailed(), but the SQLite adapter in cli/selftune/alpha-upload/queue.ts only updates rows whose status is already "sending". In this pre-send branch, that update is a no-op, so exhausted items stay pending and can starve newer uploads at the front of each batch.

Suggested fix

     if (item.attempts >= maxRetries) {
+      if (!queue.markSending(item.id)) {
+        console.error(`[alpha upload] Failed to mark queue item ${item.id} as sending`);
+        summary.failed++;
+        continue;
+      }
       markFailedSafely("exhausted retries");
       summary.failed++;
       continue;
     }

If you do not want to bounce through "sending", the cleaner fix is a dedicated pending→failed queue transition.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/flush.ts` around lines 104 - 106, The
exhausted-retries branch uses markFailedSafely (which delegates to
queue.markFailed) but that SQLite adapter only updates rows with status
"sending", so items at pre-send "pending" remain in pending; change the branch
handling when item.attempts >= maxRetries to perform a dedicated pending→failed
transition (either extend queue.markFailed to accept a from-status or add a new
queue method like markPendingFailed/transitionPendingToFailed) and ensure the
item is removed from pending and recorded as failed (increment summary.failed
remains), so pending rows don't persist and block future uploads.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Remove or use the unused parameters.

_userId, _agentType, and _selftuneVersion are passed from runUploadCycle but never used. If these are future placeholders, consider removing them until needed to reduce dead code and unnecessary computation in the caller.

♻️ Suggested fix

 export function prepareUploads(
   db: Database,
-  _userId: string,
-  _agentType: string,
-  _selftuneVersion: string,
   canonicalLogPath?: string,
 ): PrepareResult {

And update the call site in runUploadCycle:

     const prepared = prepareUploads(
       db,
-      userId,
-      agentType,
-      selftuneVersion,
       options.canonicalLogPath,
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/index.ts` around lines 80 - 86, The function
prepareUploads currently declares unused parameters _userId, _agentType, and
_selftuneVersion; remove these parameters from the prepareUploads signature (and
any corresponding destructuring) and update the call site in runUploadCycle to
stop passing them, or alternatively if they are needed later, use them (e.g.,
include them in PrepareResult construction or internal logic) — adjust both
prepareUploads and runUploadCycle so the parameter list and calls match and
eliminate the dead variables.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Unsafe cast bypasses union type validation.

The cast at line 33 doesn't verify that config.agent_type is actually one of the valid union members ("claude_code" | "codex" | "opencode" | "openclaw" | "unknown"). A corrupted config with "foobar" would pass through unchecked.

Proposed fix — validate against known values

+const VALID_AGENT_TYPES = new Set<SelftuneConfig["agent_type"]>([
+  "claude_code", "codex", "opencode", "openclaw", "unknown"
+]);
+
 export function readConfiguredAgentType(
   configPath: string,
   fallback: SelftuneConfig["agent_type"] = "unknown",
 ): SelftuneConfig["agent_type"] {
   try {
     const config = JSON.parse(readFileSync(configPath, "utf-8")) as {
       agent_type?: unknown;
     };
-    return typeof config.agent_type === "string"
-      ? (config.agent_type as SelftuneConfig["agent_type"])
-      : fallback;
+    if (typeof config.agent_type === "string" && VALID_AGENT_TYPES.has(config.agent_type as SelftuneConfig["agent_type"])) {
+      return config.agent_type as SelftuneConfig["agent_type"];
+    }
+    return fallback;
   } catch {
     return fallback;
   }
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	return typeof config.agent_type === "string"
	? (config.agent_type as SelftuneConfig["agent_type"])
	: fallback;
	const VALID_AGENT_TYPES = new Set<SelftuneConfig["agent_type"]>([
	"claude_code", "codex", "opencode", "openclaw", "unknown"
	]);
	export function readConfiguredAgentType(
	configPath: string,
	fallback: SelftuneConfig["agent_type"] = "unknown",
	): SelftuneConfig["agent_type"] {
	try {
	const config = JSON.parse(readFileSync(configPath, "utf-8")) as {
	agent_type?: unknown;
	};
	if (typeof config.agent_type === "string" && VALID_AGENT_TYPES.has(config.agent_type as SelftuneConfig["agent_type"])) {
	return config.agent_type as SelftuneConfig["agent_type"];
	}
	return fallback;
	} catch {
	return fallback;
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/utils/selftune-meta.ts` around lines 32 - 34, The current return
unsafely casts config.agent_type to SelftuneConfig["agent_type"] even when the
string may not be one of the allowed union members; change the logic in this
function to validate config.agent_type against the allowed values (e.g., an
array or Set of ["claude_code","codex","opencode","openclaw","unknown"]) and
only return it when included, otherwise return fallback; reference
config.agent_type, SelftuneConfig["agent_type"], and fallback to locate and
update the code (implement a simple inclusion check or a type guard instead of
the direct cast).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Preserve original newline style when rebuilding frontmatter (Line 37).

Using hardcoded \n can normalize CRLF files and trigger noisy rewrites even when content is semantically unchanged.

Patch to preserve matched newline style

-const frontmatterRegex = /^---\r?\n([\s\S]*?)\r?\n---/;
+const frontmatterRegex = /^(---)(\r?\n)([\s\S]*?)(\r?\n)(---)/;
 const frontmatterMatch = content.match(frontmatterRegex);
 if (!frontmatterMatch) {
   console.error(`ERROR: No YAML frontmatter found in ${skillPath}`);
   process.exit(1);
 }
 
-const frontmatter = frontmatterMatch[1];
+const [, openFence, newlineA, frontmatter, newlineB, closeFence] = frontmatterMatch;
 const versionRegex = /^(\s*version:\s*).+$/m;
 if (!versionRegex.test(frontmatter)) {
   console.error(`ERROR: No version frontmatter found in ${skillPath}`);
   process.exit(1);
 }
 
 const updatedFrontmatter = frontmatter.replace(versionRegex, `$1${pkgVersion}`);
-const updated = content.replace(frontmatterRegex, `---\n${updatedFrontmatter}\n---`);
+const updated = content.replace(
+  frontmatterRegex,
+  `${openFence}${newlineA}${updatedFrontmatter}${newlineB}${closeFence}`,
+);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/sync-skill-version.ts` around lines 37 - 43, The replacement always
injects hardcoded '\n', which normalizes CRLF and causes unnecessary file
rewrites; change the logic that builds updated (where
content.replace/frontmatterRegex and updatedFrontmatter are used) to preserve
the original newline sequence captured from the matched frontmatter (e.g.,
extract the newline from the frontmatterRegex match groups or detect the
linebreak used in content) and use that sequence instead of '\n' when joining
the frontmatter boundaries, then write to skillPath and log pkgVersion as
before.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Character limit mismatch: documentation says 1024, implementation uses 8192.

Line 212 states descriptions must be "≤1024 characters" but the implementation in constitutional.ts enforces an 8192 character limit. This will cause confusion.

-1. **Size constraint** — description must be ≤1024 characters and within
+1. **Size constraint** — description must be ≤8192 characters and within
    0.3x–3.0x word count of the original.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	### 4b. Constitutional Pre-Validation Gate
	Before any LLM-based validation, each proposal passes through a
	deterministic constitutional check that rejects obviously bad proposals
	at zero cost. Four principles are enforced:
	1. **Size constraint** — description must be ≤1024 characters and within
	0.3x–3.0x word count of the original.
	2. **No XML injection** — reject proposals containing XML/HTML tags.
	3. **No unbounded broadening** — reject bare "all", "any", "every",
	"everything" unless qualified by enumeration markers ("including",
	"such as", "like", "e.g.", or a comma-separated list).
	4. **Anchor preservation** — if the original contains `USE WHEN` trigger
	phrases or `$skillName` references, those must appear in the proposal.
	If a proposal fails any principle, it is rejected with a descriptive
	violation message and the pipeline retries (if iterations remain).
	For body evolution (`evolve body`), only the size constraint applies.
	### 4b. Constitutional Pre-Validation Gate
	Before any LLM-based validation, each proposal passes through a
	deterministic constitutional check that rejects obviously bad proposals
	at zero cost. Four principles are enforced:
	1. **Size constraint** — description must be ≤8192 characters and within
	0.3x–3.0x word count of the original.
	2. **No XML injection** — reject proposals containing XML/HTML tags.
	3. **No unbounded broadening** — reject bare "all", "any", "every",
	"everything" unless qualified by enumeration markers ("including",
	"such as", "like", "e.g.", or a comma-separated list).
	4. **Anchor preservation** — if the original contains `USE WHEN` trigger
	phrases or `$skillName` references, those must appear in the proposal.
	If a proposal fails any principle, it is rejected with a descriptive
	violation message and the pipeline retries (if iterations remain).
	For body evolution (`evolve body`), only the size constraint applies.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skill/Workflows/Evolve.md` around lines 206 - 224, The doc and code disagree
on the description length limit: Workflows/Evolve.md says ≤1024 chars but
constitutional.ts enforces 8192; make them consistent by changing the
implementation to enforce 1024 characters (update the constant/variable used in
constitutional.ts, e.g., MAX_DESCRIPTION_LENGTH / the validateProposalLength or
validateProposal function) and update any related violation messages/tests that
reference the limit so they report "≤1024 characters" exactly. Ensure the same
symbol(s) (constant name and the validator function) are updated so both
behavior and error text match the documentation.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

cli/selftune/orchestrate.ts (1)

479-484: ⚠️ Potential issue | 🟡 Minor

Remove non-null assertions; use guards instead.

Map#get() returns T | undefined. The ! operator bypasses strict null checking, which violates the coding guideline for cli/selftune/**/*.ts.

Suggested fix

   for (let i = 0; i < skillNames.length; i++) {
     for (let j = i + 1; j < skillNames.length; j++) {
-      const setA = evalSets.get(skillNames[i])!;
-      const setB = evalSets.get(skillNames[j])!;
+      const setA = evalSets.get(skillNames[i]);
+      const setB = evalSets.get(skillNames[j]);
+      if (!setA || !setB) continue;
 
       if (setA.size === 0 || setB.size === 0) continue;

As per coding guidelines: cli/selftune/**/*.ts: Use TypeScript with strict null checking and explicit type annotations in CLI code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/orchestrate.ts` around lines 479 - 484, The loop uses non-null
assertions on Map.get for evalSets (const setA = evalSets.get(skillNames[i])!
and setB similarly); change these to safe guards by removing the ! and checking
for undefined before using size — e.g., obtain setA and setB without assertions
(const setA = evalSets.get(skillNames[i]) and const setB =
evalSets.get(skillNames[j])) and add a guard like `if (!setA || !setB ||
setA.size === 0 || setB.size === 0) continue` so you never dereference an
undefined Map value; update any subsequent uses of setA/setB in the loop body
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/init/alpha-consent.test.ts`:
- Around line 43-72: Add tests in tests/init/alpha-consent.test.ts that exercise
the JSON-parse error paths: create a config.json containing malformed JSON and
assert readAlphaIdentity(configPath) returns null; then create another malformed
config.json and call writeAlphaIdentity(configPath, alpha) with a valid
AlphaIdentity and assert the call does not throw and the resulting file contains
a valid JSON alpha block (read back and verify enrolled/user_id/email).
Reference readAlphaIdentity, writeAlphaIdentity and AlphaIdentity in the new
tests to cover both helpers' parse-failure branches.
- Around line 68-70: Replace all non-null assertion operators used in this test
(e.g., result!, config.alpha!, identity!, etc.) with optional chaining so
assertions tolerate undefined; for example update expectations that reference
result!, config.alpha!, identity! to use result?.property,
config.alpha?.property, identity?.property and adjust .toBe / .toEqual checks
accordingly across the referenced assertions (lines around the result assertions
and the other occurrences listed) to satisfy the tests/**/*.ts guideline.

---

Duplicate comments:
In `@cli/selftune/orchestrate.ts`:
- Around line 479-484: The loop uses non-null assertions on Map.get for evalSets
(const setA = evalSets.get(skillNames[i])! and setB similarly); change these to
safe guards by removing the ! and checking for undefined before using size —
e.g., obtain setA and setB without assertions (const setA =
evalSets.get(skillNames[i]) and const setB = evalSets.get(skillNames[j])) and
add a guard like `if (!setA || !setB || setA.size === 0 || setB.size === 0)
continue` so you never dereference an undefined Map value; update any subsequent
uses of setA/setB in the loop body accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 07d3ce89-80f3-4f7c-9754-bc1f81e297a1

📥 Commits

Reviewing files that changed from the base of the PR and between 0b27dfa and 719a1e4.

📒 Files selected for processing (4)

• cli/selftune/dashboard-server.ts
• cli/selftune/index.ts
• cli/selftune/orchestrate.ts
• tests/init/alpha-consent.test.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/selftune/init.ts`:
- Around line 565-569: The current sequence writes the config with
writeFileSync(configPath, ...) then calls chmodSync(configPath, 0o600) which can
leave a window where the file exists with default perms; to fix, implement an
atomic write: serialize config.alpha into a temp file (e.g., configPath +
".tmp"), create that temp file with restricted permissions (mode 0o600) before
writing, write the JSON to the temp file, fsync/close it, then rename/move the
temp file to configPath (overwriting), and only perform chmodSync(configPath,
0o600) as a fallback if rename semantics on the platform require it; update the
logic around opts.alphaKey and use configPath, writeFileSync/rename/chmodSync in
that order to ensure the API key never exists in a file with wider permissions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bee17133-35a8-4a9f-9cb3-56be3ad4f7a7

📥 Commits

Reviewing files that changed from the base of the PR and between 719a1e4 and 7a3e119.

📒 Files selected for processing (1)

• cli/selftune/init.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/trust-floor/health.test.ts`:
- Around line 33-82: Add a new test case alongside the existing "returns all
expected fields" that simulates a degraded SPA asset scenario by calling
startDashboardServer with an invalid/missing spaDir (or spaDir: undefined) and
openBrowser: false, then fetch /api/health and assert status is 200, body.spa is
false, and all runtime identity fields still exist and are well-typed
(workspace_root, git_sha, db_path, log_dir, config_dir), watcher_mode is one of
["jsonl","none"], process_mode is "test", and host/port are present and valid;
reuse the same HealthResponse assertions as in the original test to ensure the
health contract remains stable even on this error path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1cf992e9-74ac-44db-a134-b664f5be40e7

📥 Commits

Reviewing files that changed from the base of the PR and between 7a3e119 and a8c5aee.

📒 Files selected for processing (1)

• tests/trust-floor/health.test.ts

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Add one degraded/error-path case for /api/health.

This test only validates the success path. Add a non-happy scenario (e.g., missing SPA assets) and assert the health contract remains stable (status 200, spa === false, runtime fields still present).

💡 Example test addition

 describe("/api/health runtime identity", () => {
   it("returns all expected fields", async () => {
@@
     expect(body.port).toBeGreaterThan(0);
   });
+
+  it("reports spa=false when SPA assets are missing", async () => {
+    const emptySpaDir = mkdtempSync(join(tmpdir(), "selftune-health-empty-"));
+    let localServer: Awaited<ReturnType<typeof startDashboardServer>> | null = null;
+    try {
+      localServer = await startDashboardServer({
+        port: 0,
+        host: "127.0.0.1",
+        spaDir: emptySpaDir,
+        openBrowser: false,
+        runtimeMode: "test",
+        overviewLoader: () => ({
+          overview: {
+            telemetry: [],
+            skills: [],
+            evolution: [],
+            counts: { telemetry: 0, skills: 0, evolution: 0, evidence: 0, sessions: 0, prompts: 0 },
+            unmatched_queries: [],
+            pending_proposals: [],
+          },
+          skills: [],
+        }),
+      });
+
+      const res = await fetch(`http://127.0.0.1:${localServer.port}/api/health`);
+      expect(res.status).toBe(200);
+      const body: HealthResponse = await res.json();
+      expect(body.spa).toBe(false);
+      expect(body.process_mode).toBe("test");
+    } finally {
+      if (localServer) await localServer.stop();
+      rmSync(emptySpaDir, { recursive: true, force: true });
+    }
+  });
 });

As per coding guidelines, tests/**/*.ts: "Coverage of error paths, not just happy paths".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/trust-floor/health.test.ts` around lines 33 - 82, Add a new test case
alongside the existing "returns all expected fields" that simulates a degraded
SPA asset scenario by calling startDashboardServer with an invalid/missing
spaDir (or spaDir: undefined) and openBrowser: false, then fetch /api/health and
assert status is 200, body.spa is false, and all runtime identity fields still
exist and are well-typed (workspace_root, git_sha, db_path, log_dir,
config_dir), watcher_mode is one of ["jsonl","none"], process_mode is "test",
and host/port are present and valid; reuse the same HealthResponse assertions as
in the original test to ensure the health contract remains stable even on this
error path.

[coderabbitai]

Actionable comments posted: 7

♻️ Duplicate comments (1)

cli/selftune/alpha-upload/client.ts (1)

71-75: ⚠️ Potential issue | 🟠 Major

Preserve _status on validated PushUploadResult responses.

When the server returns a valid PushUploadResult (e.g., { success: false, errors: [...] }) on a 2xx response, the parsed object is returned without _status if the server didn't include it. Downstream retry logic in flushQueue uses _status to determine retryability—missing status may cause incorrect behavior.

Suggested fix

       if (isPushUploadResult(parsed)) {
-        return parsed;
+        return {
+          ...parsed,
+          _status: parsed._status ?? response.status,
+        };
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/client.ts` around lines 71 - 75, When a parsed
response passes isPushUploadResult(validation) but lacks a `_status` field,
populate `_status` on that object (e.g., set parsed._status = parsed._status ??
res.status) before returning so downstream retry logic in flushQueue can rely on
it; update the try block that parses body and checks isPushUploadResult to
ensure `_status` is preserved/filled when missing.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/exec-plans/active/agent-first-alpha-onboarding.md`:
- Around line 34-46: Add blank lines before and after each Markdown heading to
satisfy markdownlint rule MD022; specifically, update headings such as
"Deliverable" and "Acceptance" (and any other headings in this document) so
there is an empty line above and below the heading text and its surrounding
content, ensuring consistent spacing throughout the file to remove linter
warnings.

In `@docs/exec-plans/active/alpha-simplification-program.md`:
- Around line 281-283: The three related-plan links currently use absolute local
filesystem paths; update them to relative repository paths so they work for
other contributors by replacing the absolute links for
alpha-rollout-data-loop-plan.md, cloud-auth-unification-for-alpha.md, and
dashboard-data-integrity-recovery.md with relative links (e.g.,
./alpha-rollout-data-loop-plan.md or ../active/<filename>.md) ensuring the link
text remains the same and points to the files under docs/exec-plans/active.

In `@docs/exec-plans/active/cloud-auth-unification-for-alpha.md`:
- Around line 120-130: Replace the machine-specific absolute paths in the doc
(the lines containing
`/Users/danielpetro/conductor/workspaces/selftune-cloud-app/gwangju-v1` and
`/Users/danielpetro/conductor/workspaces/selftune/miami`) with repo-root
relative paths (e.g., `workspaces/selftune-cloud-app/gwangju-v1` and
`workspaces/selftune/miami`) and scan the file for any other absolute
`/Users/danielpetro/...` occurrences to convert them similarly so the docs are
portable and do not expose local directory structure.
- Around line 359-360: Change the two absolute-filepath links to relative links
for portability: replace the link targets referencing
alpha-rollout-data-loop-plan.md and dashboard-data-integrity-recovery.md so they
use relative paths (e.g., ./alpha-rollout-data-loop-plan.md and
./dashboard-data-integrity-recovery.md or simply the filenames) in the
cloud-auth-unification-for-alpha.md content.

In `@docs/exec-plans/active/output-quality-loop-prereqs.md`:
- Line 5: The markdown reference uses a machine-local absolute path in
docs/exec-plans/active/output-quality-loop-prereqs.md (the Related link to
prd-output-quality-loop.md); change that absolute path to a repository-relative
link pointing to the doc within the repo (e.g., replace
"/Users/.../strategy/prd-output-quality-loop.md" with the repo-relative path to
prd-output-quality-loop.md such as "strategy/prd-output-quality-loop.md" or
"../strategy/prd-output-quality-loop.md" depending on desired relative location)
so the link resolves correctly in GitHub/docs rendering.

In `@tests/alpha-upload/flush.test.ts`:
- Around line 3-4: Remove the unused import symbol FlushOptions from the import
statement in tests/alpha-upload/flush.test.ts; keep the required import
flushQueue (and any other used types) and update the import line that currently
reads "import { type FlushOptions, flushQueue } from
'../../cli/selftune/alpha-upload/flush.js';" so it only imports symbols actually
used in the test (e.g., "import { flushQueue } ..."), ensuring no other
references to FlushOptions remain in the file.
- Around line 410-413: Replace the non-null assertion on the test assertion
(queue.calls.getPending[0]![0]) with a safe guard/optional chaining like the
pattern used elsewhere in this file: check that queue.calls.getPending[0] is
defined (or use optional chaining result?.[0]) before accessing [0], then assert
its value equals 2; update the assertion in the test that calls flushQueue so it
does not use the `!` operator and instead guards the array access on
queue.calls.getPending (refer to flushQueue and queue.calls.getPending in this
test).

---

Duplicate comments:
In `@cli/selftune/alpha-upload/client.ts`:
- Around line 71-75: When a parsed response passes
isPushUploadResult(validation) but lacks a `_status` field, populate `_status`
on that object (e.g., set parsed._status = parsed._status ?? res.status) before
returning so downstream retry logic in flushQueue can rely on it; update the try
block that parses body and checks isPushUploadResult to ensure `_status` is
preserved/filled when missing.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d92b9f03-93e6-4b9f-a74d-3a8e3f409a48

📥 Commits

Reviewing files that changed from the base of the PR and between a8c5aee and 827ed6e.

📒 Files selected for processing (6)

• cli/selftune/alpha-upload/client.ts
• docs/exec-plans/active/agent-first-alpha-onboarding.md
• docs/exec-plans/active/alpha-simplification-program.md
• docs/exec-plans/active/cloud-auth-unification-for-alpha.md
• docs/exec-plans/active/output-quality-loop-prereqs.md
• tests/alpha-upload/flush.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add blank lines around headings per markdownlint (MD022).

Static analysis flagged multiple headings missing blank lines. This applies throughout the file (e.g., lines 34, 43, 52, 57, etc.). Adding blank lines improves readability and silences the linter.

Example fix pattern:

 ### Deliverable
+
 - a short flow spec covering:
   ...

+
 ### Acceptance
+
 - no ambiguity about where browser handoff happens

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	### Deliverable
	- a short flow spec covering:
	- user says "set up selftune"
	- agent checks local config
	- if not linked, agent explains the cloud enrollment step
	- user signs in / enrolls / issues credential
	- agent stores credential locally
	- agent finishes setup and verifies upload readiness
	### Acceptance
	- no ambiguity about where browser handoff happens
	- no ambiguity about what the agent asks the user
	- no ambiguity about when the flow returns to local CLI mode
	### Deliverable
	- a short flow spec covering:
	- user says "set up selftune"
	- agent checks local config
	- if not linked, agent explains the cloud enrollment step
	- user signs in / enrolls / issues credential
	- agent stores credential locally
	- agent finishes setup and verifies upload readiness
	### Acceptance
	- no ambiguity about where browser handoff happens
	- no ambiguity about what the agent asks the user
	- no ambiguity about when the flow returns to local CLI mode

🧰 Tools

🪛 markdownlint-cli2 (0.21.0)

[warning] 34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 43-43: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/agent-first-alpha-onboarding.md` around lines 34 - 46,
Add blank lines before and after each Markdown heading to satisfy markdownlint
rule MD022; specifically, update headings such as "Deliverable" and "Acceptance"
(and any other headings in this document) so there is an empty line above and
below the heading text and its surrounding content, ensuring consistent spacing
throughout the file to remove linter warnings.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use relative paths for related plan links.

Same issue as in the auth unification doc—absolute paths won't work for other contributors:

-- [alpha-rollout-data-loop-plan.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/alpha-rollout-data-loop-plan.md)
-- [cloud-auth-unification-for-alpha.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/cloud-auth-unification-for-alpha.md)
-- [dashboard-data-integrity-recovery.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/dashboard-data-integrity-recovery.md)
+- [alpha-rollout-data-loop-plan.md](./alpha-rollout-data-loop-plan.md)
+- [cloud-auth-unification-for-alpha.md](./cloud-auth-unification-for-alpha.md)
+- [dashboard-data-integrity-recovery.md](./dashboard-data-integrity-recovery.md)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- [alpha-rollout-data-loop-plan.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/alpha-rollout-data-loop-plan.md)
	- [cloud-auth-unification-for-alpha.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/cloud-auth-unification-for-alpha.md)
	- [dashboard-data-integrity-recovery.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/dashboard-data-integrity-recovery.md)
	- [alpha-rollout-data-loop-plan.md](./alpha-rollout-data-loop-plan.md)
	- [cloud-auth-unification-for-alpha.md](./cloud-auth-unification-for-alpha.md)
	- [dashboard-data-integrity-recovery.md](./dashboard-data-integrity-recovery.md)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/alpha-simplification-program.md` around lines 281 -
283, The three related-plan links currently use absolute local filesystem paths;
update them to relative repository paths so they work for other contributors by
replacing the absolute links for alpha-rollout-data-loop-plan.md,
cloud-auth-unification-for-alpha.md, and dashboard-data-integrity-recovery.md
with relative links (e.g., ./alpha-rollout-data-loop-plan.md or
../active/<filename>.md) ensuring the link text remains the same and points to
the files under docs/exec-plans/active.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Replace absolute paths with relative paths.

Lines 120 and 130 contain machine-specific absolute paths (/Users/danielpetro/conductor/workspaces/...). These won't resolve for other contributors and leak local directory structure. Use relative paths from the repo root instead.

-### `/Users/danielpetro/conductor/workspaces/selftune-cloud-app/gwangju-v1`
+### `selftune-cloud-app` (cloud repo)

...

-### `/Users/danielpetro/conductor/workspaces/selftune/miami`
+### `selftune` (this repo)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	### `/Users/danielpetro/conductor/workspaces/selftune-cloud-app/gwangju-v1`
	Owns:
	- canonical user/session/org auth model
	- alpha enrollment state
	- upload credential issuance and revocation
	- protected operator surfaces
	- upload auth verification
	### `/Users/danielpetro/conductor/workspaces/selftune/miami`
	### `selftune-cloud-app` (cloud repo)
	Owns:
	- canonical user/session/org auth model
	- alpha enrollment state
	- upload credential issuance and revocation
	- protected operator surfaces
	- upload auth verification
	### `selftune` (this repo)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/cloud-auth-unification-for-alpha.md` around lines 120
- 130, Replace the machine-specific absolute paths in the doc (the lines
containing
`/Users/danielpetro/conductor/workspaces/selftune-cloud-app/gwangju-v1` and
`/Users/danielpetro/conductor/workspaces/selftune/miami`) with repo-root
relative paths (e.g., `workspaces/selftune-cloud-app/gwangju-v1` and
`workspaces/selftune/miami`) and scan the file for any other absolute
`/Users/danielpetro/...` occurrences to convert them similarly so the docs are
portable and do not expose local directory structure.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Related plans also use absolute paths.

Same issue as above—use relative paths for portability:

-- [alpha-rollout-data-loop-plan.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/alpha-rollout-data-loop-plan.md)
-- [dashboard-data-integrity-recovery.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/dashboard-data-integrity-recovery.md)
+- [alpha-rollout-data-loop-plan.md](./alpha-rollout-data-loop-plan.md)
+- [dashboard-data-integrity-recovery.md](./dashboard-data-integrity-recovery.md)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- [alpha-rollout-data-loop-plan.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/alpha-rollout-data-loop-plan.md)
	- [dashboard-data-integrity-recovery.md](/Users/danielpetro/conductor/workspaces/selftune/miami/docs/exec-plans/active/dashboard-data-integrity-recovery.md)
	- [alpha-rollout-data-loop-plan.md](./alpha-rollout-data-loop-plan.md)
	- [dashboard-data-integrity-recovery.md](./dashboard-data-integrity-recovery.md)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/cloud-auth-unification-for-alpha.md` around lines 359
- 360, Change the two absolute-filepath links to relative links for portability:
replace the link targets referencing alpha-rollout-data-loop-plan.md and
dashboard-data-integrity-recovery.md so they use relative paths (e.g.,
./alpha-rollout-data-loop-plan.md and ./dashboard-data-integrity-recovery.md or
simply the filenames) in the cloud-auth-unification-for-alpha.md content.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Replace machine-local absolute path with repo-relative doc link.

Line 5 points to a local filesystem path (/Users/...), which is non-portable and breaks in GitHub/docs rendering. Use a repository-relative path instead.

Proposed fix

-**Related:** [prd-output-quality-loop.md](/Users/danielpetro/Documents/Projects/FOSS/selftune/strategy/prd-output-quality-loop.md)
+**Related:** [prd-output-quality-loop.md](../../../strategy/prd-output-quality-loop.md)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	**Related:** [prd-output-quality-loop.md](/Users/danielpetro/Documents/Projects/FOSS/selftune/strategy/prd-output-quality-loop.md)
	**Related:** [prd-output-quality-loop.md](../../../strategy/prd-output-quality-loop.md)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/output-quality-loop-prereqs.md` at line 5, The
markdown reference uses a machine-local absolute path in
docs/exec-plans/active/output-quality-loop-prereqs.md (the Related link to
prd-output-quality-loop.md); change that absolute path to a repository-relative
link pointing to the doc within the repo (e.g., replace
"/Users/.../strategy/prd-output-quality-loop.md" with the repo-relative path to
prd-output-quality-loop.md such as "strategy/prd-output-quality-loop.md" or
"../strategy/prd-output-quality-loop.md" depending on desired relative location)
so the link resolves correctly in GitHub/docs rendering.

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cli/selftune/evolution/evolve.ts (1)

496-523: ⚠️ Potential issue | 🟠 Major

Pareto mode bypasses the constitutional gate.

The standard retry loop rejects proposals with checkConstitution(...) before validation, but Pareto candidates go straight from created to _validateProposal(). In --pareto mode, an XML-tagged or anchor-dropping proposal can still be selected and deployed. Apply the same reject/audit/evidence flow here before validation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/evolution/evolve.ts` around lines 496 - 523, In the
viableCandidates loop before calling _validateProposal, run the same
constitutional check used in the retry flow (call checkConstitution or the
equivalent reject logic for proposals) and if it fails, mark the proposal
rejected: call recordAudit with the "rejected" status and the same
rejectAuditDetails, emit recordEvidence with stage "rejected" (include
rationale/details/original_text/proposed_text/eval_set as the other flow does),
and skip calling _validateProposal for that proposal; ensure you reuse the same
rejection helper or messages so Pareto candidates follow the identical reject →
audit → evidence flow as non-Pareto proposals.

♻️ Duplicate comments (5)

tests/init/alpha-consent.test.ts (1)

43-72: ⚠️ Potential issue | 🟡 Minor

Add malformed-JSON branch tests for alpha identity helpers.

The suite still does not assert parse-failure behavior: malformed config.json should make readAlphaIdentity return null, and should make writeAlphaIdentity throw.

Proposed test additions

 describe("readAlphaIdentity", () => {
+  test("returns null when config contains invalid JSON", () => {
+    const configPath = join(tmpDir, "config.json");
+    writeFileSync(configPath, "{invalid-json", "utf-8");
+    expect(readAlphaIdentity(configPath)).toBeNull();
+  });
+
   test("returns null when config does not exist", () => {
     const result = readAlphaIdentity(join(tmpDir, "nonexistent.json"));
     expect(result).toBeNull();
   });

 describe("writeAlphaIdentity", () => {
+  test("throws when existing config is invalid JSON", () => {
+    const configPath = join(tmpDir, "config.json");
+    writeFileSync(configPath, "{invalid-json", "utf-8");
+    const identity: AlphaIdentity = {
+      enrolled: true,
+      user_id: "new-uuid",
+      email: "new@example.com",
+      consent_timestamp: "2026-03-18T00:00:00Z",
+    };
+    expect(() => writeAlphaIdentity(configPath, identity)).toThrow(
+      "Unable to update alpha identity",
+    );
+  });
+
   test("writes alpha block to new config file", () => {

As per coding guidelines, tests/**/*.ts: "Coverage of error paths, not just happy paths."

Also applies to: 74-112

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/init/alpha-consent.test.ts` around lines 43 - 72, Add tests that cover
malformed JSON error paths: in tests/init/alpha-consent.test.ts add a case where
config.json contains invalid JSON and assert readAlphaIdentity(configPath)
returns null, and another case where writeAlphaIdentity(configPath, alpha) is
invoked when the config file contains malformed JSON and assert it throws.
Locate the test suite around describe("readAlphaIdentity") and add these
assertions referencing readAlphaIdentity and writeAlphaIdentity to ensure parse
failures return null for reads and throw for writes.

cli/selftune/alpha-upload/client.ts (1)

69-73: ⚠️ Potential issue | 🟠 Major

Preserve response.status on validated JSON bodies.

This branch returns parsed as-is. A 2xx body like { success: false, errors: [...] } therefore loses _status, and downstream retry logic can no longer distinguish a server-declared failure from a transport error. Return the parsed result with _status: parsed._status ?? response.status.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/client.ts` around lines 69 - 73, The parsed JSON
branch currently returns the raw parsed object and can drop the HTTP status;
update the code path that checks isPushUploadResult(parsed) (the branch
returning parsed) to merge in the response status before returning, e.g. return
an object with all parsed fields and _status: parsed._status ?? response.status
so downstream retry logic can see the server-declared status.

cli/selftune/orchestrate.ts (1)

1010-1014: ⚠️ Potential issue | 🟠 Major

Use a neutral fallback for agentType.

This still reports uploads as claude_code when agent_type is missing from config. The shared helper already falls back to "unknown", so forcing "claude_code" here reintroduces the cross-agent mislabeling issue for older or partially initialized installs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/orchestrate.ts` around lines 1010 - 1014, The upload is forcing
agentType to "claude_code" which overrides the shared helper's neutral fallback;
update the runUploadCycle call to stop forcing that value by replacing
agentType: readConfiguredAgentType(SELFTUNE_CONFIG_PATH, "claude_code") with
either agentType: readConfiguredAgentType(SELFTUNE_CONFIG_PATH) so the helper's
built-in "unknown" default is used, or explicitly pass "unknown" (agentType:
readConfiguredAgentType(SELFTUNE_CONFIG_PATH, "unknown")) to avoid mislabeling
uploads.

cli/selftune/evolution/constitutional.ts (1)

37-46: ⚠️ Potential issue | 🟡 Minor

Compute sentence offsets from the original text.

offset += sentence.length + 1 only works if the split consumed exactly one whitespace character. Multiple spaces or newlines shift the offsets and can make the broadening check inspect the wrong sentence.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/evolution/constitutional.ts` around lines 37 - 46, The
sentenceContaining function incorrectly assumes a single-space separator by
doing offset += sentence.length + 1; instead, compute sentence boundaries from
the original text so offsets are accurate: re-scan text with a
sentence-splitting regex (use global/exec or matchAll) and use each match's
index to determine start/end, or use text.indexOf(sentence, offset) to find the
actual start; update sentenceContaining to use those real start indices when
comparing matchIndex. Ensure you update references to the sentences loop so it
uses the match.index (or the indexOf result) rather than incrementing by +1.

tests/alpha-upload/e2e.test.ts (1)

107-127: ⚠️ Potential issue | 🟠 Major

Use V2 deterministic evidence shape in stageEvolutionEvidence fixture.

Line 110/Line 126 currently stage evolution_evidence with a custom record_id and without evidence_id in record_json. That diverges from production staging semantics and can mask integration regressions in this E2E suite.

Suggested fix

+import { generateEvidenceId } from "../../cli/selftune/alpha-upload/stage-canonical.js";
...
 function stageEvolutionEvidence(db: Database, count: number): void {
   for (let i = 0; i < count; i++) {
-    const recordId = `e2e-prop-${i}:deployed:2026-01-01T00:00:00Z`;
-    const record = {
+    const baseRecord = {
       skill_name: "Research",
       proposal_id: `e2e-prop-${i}`,
+      skill_path: "/tmp/SKILL.md",
       target: "description",
       stage: "deployed",
       rationale: "improved accuracy",
       confidence: 0.85,
       timestamp: "2026-01-01T00:00:00.000Z",
       original_text: "old",
       proposed_text: "new",
     };
+    const evidenceId = generateEvidenceId(baseRecord as Record<string, unknown>);
+    const record = { ...baseRecord, evidence_id: evidenceId };
     db.run(
       `INSERT OR IGNORE INTO canonical_upload_staging
         (record_kind, record_id, record_json, staged_at)
        VALUES (?, ?, ?, ?)`,
-      ["evolution_evidence", recordId, JSON.stringify(record), new Date().toISOString()],
+      ["evolution_evidence", evidenceId, JSON.stringify(record), new Date().toISOString()],
     );
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/alpha-upload/e2e.test.ts` around lines 107 - 127,
stageEvolutionEvidence currently writes a custom record_id and omits evidence_id
in the record_json; update the fixture (function stageEvolutionEvidence) to use
the V2 deterministic evidence shape by generating a deterministic evidence_id,
setting record_id to the V2 pattern (e.g.,
evidence-{evidence_id}:deployed:2026-01-01T00:00:00Z) and adding an evidence_id
property inside the record JSON payload along with the existing fields
(skill_name, proposal_id, target, stage, rationale, confidence, timestamp,
original_text, proposed_text) so the staged row matches production staging
semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/selftune/evolution/constitutional.ts`:
- Around line 119-124: The current regex used to find dollar-anchored tokens
(const dollarRefs = original.match(/\$\w+/g)) only captures a prefix and misses
characters like '-' in slugs; update the pattern to match full anchor tokens
(e.g., use /\$[A-Za-z0-9_-]+/g) so dollarRefs contains complete tokens like
$test-skill, then keep the existing loop that checks if proposed.includes(ref)
and pushes to violations unchanged (ensure you replace the regex where
dollarRefs is declared).

In `@cli/selftune/localdb/materialize.ts`:
- Around line 125-141: The current condition treats a boundary-count mismatch as
a conflict even when JSONL is ahead of SQLite; update the warning condition so
the boundary-count comparison is only checked when sqliteMax === jsonlMax.
Concretely, change the combined if that uses jsonlMax, newerCount,
sqliteBoundaryCount, and jsonlBoundaryCount (the block that calls warnings.push)
to only include the sqliteBoundaryCount !== jsonlBoundaryCount check when
sqliteMax === jsonlMax (e.g. replace the third clause with (sqliteMax ===
jsonlMax && sqliteBoundaryCount !== jsonlBoundaryCount)) so rebuilds aren’t
blocked when jsonlMax > sqliteMax.

In `@packages/telemetry-contract/tests/compatibility.test.ts`:
- Around line 98-125: The tests "unresolved parent references pass (invocation
references session_id not in sessions)" and "prompts with unresolved session_id
pass validation" can vacuously pass if the mapped arrays are empty; add explicit
non-empty preconditions by asserting the mapped arrays contain at least one
element before the for-loops (e.g., assert invSessionIds.length > 0 in the first
test and promptSessionIds.length > 0 in the second test), so that the subsequent
checks against sessionIds and the safeParse validation actually exercise
unresolved-reference behavior on partialPushUnresolvedParents.
- Around line 45-57: The tests "execution_fact_id is required on execution
facts" and "execution_fact_id rejects empty string" currently only assert
result.success is false; update them to assert the Zod error specifically
targets the execution_fact_id field so other schema failures don't mask
regressions. After calling PushPayloadV2Schema.safeParse(badPayload), assert
result.error exists and that result.error.issues (or
result.error.flatten()/format()) contains an issue whose path equals
['canonical','execution_facts',0,'execution_fact_id'] (and optionally assert the
issue.message or code indicates a missing/empty value) instead of only checking
result.success.

In `@tests/autonomy-proof.test.ts`:
- Line 265: The variable _auditLogPath is declared but never used in the test;
remove the unused declaration (the line creating _auditLogPath) from
tests/autonomy-proof.test.ts so the test relies on readAuditTrail()'s injected
default path instead of keeping an unused _auditLogPath variable.

---

Outside diff comments:
In `@cli/selftune/evolution/evolve.ts`:
- Around line 496-523: In the viableCandidates loop before calling
_validateProposal, run the same constitutional check used in the retry flow
(call checkConstitution or the equivalent reject logic for proposals) and if it
fails, mark the proposal rejected: call recordAudit with the "rejected" status
and the same rejectAuditDetails, emit recordEvidence with stage "rejected"
(include rationale/details/original_text/proposed_text/eval_set as the other
flow does), and skip calling _validateProposal for that proposal; ensure you
reuse the same rejection helper or messages so Pareto candidates follow the
identical reject → audit → evidence flow as non-Pareto proposals.

---

Duplicate comments:
In `@cli/selftune/alpha-upload/client.ts`:
- Around line 69-73: The parsed JSON branch currently returns the raw parsed
object and can drop the HTTP status; update the code path that checks
isPushUploadResult(parsed) (the branch returning parsed) to merge in the
response status before returning, e.g. return an object with all parsed fields
and _status: parsed._status ?? response.status so downstream retry logic can see
the server-declared status.

In `@cli/selftune/evolution/constitutional.ts`:
- Around line 37-46: The sentenceContaining function incorrectly assumes a
single-space separator by doing offset += sentence.length + 1; instead, compute
sentence boundaries from the original text so offsets are accurate: re-scan text
with a sentence-splitting regex (use global/exec or matchAll) and use each
match's index to determine start/end, or use text.indexOf(sentence, offset) to
find the actual start; update sentenceContaining to use those real start indices
when comparing matchIndex. Ensure you update references to the sentences loop so
it uses the match.index (or the indexOf result) rather than incrementing by +1.

In `@cli/selftune/orchestrate.ts`:
- Around line 1010-1014: The upload is forcing agentType to "claude_code" which
overrides the shared helper's neutral fallback; update the runUploadCycle call
to stop forcing that value by replacing agentType:
readConfiguredAgentType(SELFTUNE_CONFIG_PATH, "claude_code") with either
agentType: readConfiguredAgentType(SELFTUNE_CONFIG_PATH) so the helper's
built-in "unknown" default is used, or explicitly pass "unknown" (agentType:
readConfiguredAgentType(SELFTUNE_CONFIG_PATH, "unknown")) to avoid mislabeling
uploads.

In `@tests/alpha-upload/e2e.test.ts`:
- Around line 107-127: stageEvolutionEvidence currently writes a custom
record_id and omits evidence_id in the record_json; update the fixture (function
stageEvolutionEvidence) to use the V2 deterministic evidence shape by generating
a deterministic evidence_id, setting record_id to the V2 pattern (e.g.,
evidence-{evidence_id}:deployed:2026-01-01T00:00:00Z) and adding an evidence_id
property inside the record JSON payload along with the existing fields
(skill_name, proposal_id, target, stage, rationale, confidence, timestamp,
original_text, proposed_text) so the staged row matches production staging
semantics.

In `@tests/init/alpha-consent.test.ts`:
- Around line 43-72: Add tests that cover malformed JSON error paths: in
tests/init/alpha-consent.test.ts add a case where config.json contains invalid
JSON and assert readAlphaIdentity(configPath) returns null, and another case
where writeAlphaIdentity(configPath, alpha) is invoked when the config file
contains malformed JSON and assert it throws. Locate the test suite around
describe("readAlphaIdentity") and add these assertions referencing
readAlphaIdentity and writeAlphaIdentity to ensure parse failures return null
for reads and throw for writes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0eb9f437-a048-4b55-b412-15cdf427bb48

📥 Commits

Reviewing files that changed from the base of the PR and between 827ed6e and 794084d.

📒 Files selected for processing (23)

• cli/selftune/alpha-upload/client.ts
• cli/selftune/evolution/constitutional.ts
• cli/selftune/evolution/evolve.ts
• cli/selftune/hooks/skill-eval.ts
• cli/selftune/ingestors/claude-replay.ts
• cli/selftune/localdb/materialize.ts
• cli/selftune/orchestrate.ts
• cli/selftune/routes/overview.ts
• packages/telemetry-contract/tests/compatibility.test.ts
• tests/alpha-upload/e2e.test.ts
• tests/alpha-upload/flush.test.ts
• tests/alpha-upload/staging.test.ts
• tests/alpha-upload/status.test.ts
• tests/autonomy-proof.test.ts
• tests/evolution/constitutional.test.ts
• tests/hooks/prompt-log.test.ts
• tests/hooks/session-stop.test.ts
• tests/hooks/skill-eval.test.ts
• tests/ingestors/claude-replay.test.ts
• tests/init/alpha-consent.test.ts
• tests/localdb/read-queries.test.ts
• tests/localdb/write.test.ts
• tests/trust-floor/rebuild-preflight.test.ts

💤 Files with no reviewable changes (3)

• cli/selftune/routes/overview.ts
• tests/hooks/session-stop.test.ts
• tests/localdb/write.test.ts

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cli/selftune/evolution/evolve.ts (1)

907-950: ⚠️ Potential issue | 🟡 Minor

Count gate-validation calls in llmCallCount.

This branch runs another model-backed validation pass but never updates the counter. EvolveResult.llmCallCount, the TUI footer, and the CLI summary will all underreport usage whenever --gate-model is enabled.

Suggested fix

     if (options.gateModel && lastProposal && lastValidation?.improved) {
       tui.step(`Gate validation (${options.gateModel})...`);
+      const gateBatchCount = Math.ceil(evalSet.length / TRIGGER_CHECK_BATCH_SIZE);
       gateValidation = await _gateValidateProposal(lastProposal, evalSet, agent, options.gateModel);
+      llmCallCount += gateBatchCount * 2 * VALIDATION_RUNS;
       tui.done(
         `Gate (${options.gateModel}): improved=${gateValidation.improved}, net_change=${gateValidation.net_change.toFixed(3)}`,
       );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/evolution/evolve.ts` around lines 907 - 950, The gate validation
branch calls the model but doesn't increment the global LLM counter, so update
EvolveResult.llmCallCount when performing the gate validation: after obtaining
gateValidation (the result of the gate-model validation call in evolve.ts)
increment the local llmCallCount variable (or add gateValidation.llmCalls if
that field exists) before recording audit/evidence and returning via withStats;
ensure the final object passed to withStats includes the updated llmCallCount so
the TUI footer/CLI summary reflect the extra gate-model usage.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/selftune/alpha-upload/client.ts`:
- Around line 62-68: The empty-body success branch in the function in client.ts
returns an object without the _status field while other branches include it;
update that branch to include the same _status value used elsewhere (e.g., the
HTTP status code variable or constant used in this function) alongside success,
push_id and errors so the returned shape matches other paths (locate the branch
checking body.length === 0 and add _status with the existing status code source
to the returned object).

In `@cli/selftune/evolution/constitutional.ts`:
- Around line 39-49: The splitting regex in sentenceContaining() incorrectly
places the abbreviation guard before the punctuation match, so abbreviations
like "e.g." still break sentences; change the split to only break where the
following token looks like the start of a new sentence (e.g. an uppercase letter
or digit/quote), for example replace the current regex
/(?<![a-z])(?<=[.!?])\s+/i with a lookahead-based one such as
/(?<=[.!?])\s+(?=[A-Z0-9"'\u2018\u201C])/ (or equivalent) in the sentences =
text.split(...) line so abbreviations don't cause mid-sentence splits and
sentenceContaining() returns the correct sentence.
- Around line 121-126: The current check uses proposed.includes(ref) which
matches substrings (so "$test" matches "$test2"); instead, extract dollar tokens
from both strings and compare tokens for exact equality: use the same regex used
for dollarRefs to build a Set of tokens from proposed (e.g., call it
proposedDollarRefs) and then test membership with proposedDollarRefs.has(ref),
pushing to violations when missing; update the block handling dollarRefs,
keeping variable names dollarRefs, proposed, original, and violations so the
change is easy to locate.

In `@cli/selftune/localdb/materialize.ts`:
- Around line 506-521: The upsert currently writes excluded.skills_invoked_json
(and similarly skills_triggered_json) unconditionally, so NULL in a partial push
can erase existing values; update the ON CONFLICT ... DO UPDATE SET clause to
use COALESCE(excluded.skills_invoked_json,
session_telemetry.skills_invoked_json) and
COALESCE(excluded.skills_triggered_json,
session_telemetry.skills_triggered_json) (matching the pattern used for
input_tokens/output_tokens) so existing non-null values in session_telemetry are
preserved when the excluded value is NULL.

In `@tests/alpha-upload/e2e.test.ts`:
- Around line 134-142: Make buildQueueOps async and load the queue helpers via
dynamic import: replace the require("../../cli/selftune/alpha-upload/queue.js")
call with await import("../../cli/selftune/alpha-upload/queue.js") inside
buildQueueOps, then return the same object using the imported markSending,
markSent, markFailed functions (keeping getPending -> getPendingUploads(db,
limit)). Update all call sites to use await buildQueueOps(db) so callers receive
the resolved QueueOperations.

---

Outside diff comments:
In `@cli/selftune/evolution/evolve.ts`:
- Around line 907-950: The gate validation branch calls the model but doesn't
increment the global LLM counter, so update EvolveResult.llmCallCount when
performing the gate validation: after obtaining gateValidation (the result of
the gate-model validation call in evolve.ts) increment the local llmCallCount
variable (or add gateValidation.llmCalls if that field exists) before recording
audit/evidence and returning via withStats; ensure the final object passed to
withStats includes the updated llmCallCount so the TUI footer/CLI summary
reflect the extra gate-model usage.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c5012a7d-88f7-41ef-849e-4c2be66bfedd

📥 Commits

Reviewing files that changed from the base of the PR and between 794084d and 99eabe1.

📒 Files selected for processing (9)

• cli/selftune/alpha-upload/client.ts
• cli/selftune/evolution/constitutional.ts
• cli/selftune/evolution/evolve.ts
• cli/selftune/localdb/materialize.ts
• cli/selftune/orchestrate.ts
• packages/telemetry-contract/tests/compatibility.test.ts
• tests/alpha-upload/e2e.test.ts
• tests/autonomy-proof.test.ts
• tests/init/alpha-consent.test.ts

💤 Files with no reviewable changes (1)

• tests/autonomy-proof.test.ts

[coderabbitai]

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/selftune/alpha-upload/client.ts`:
- Around line 62-66: In the early-return inside the body.length === 0 branch,
replace the unsafe cast (payload as { push_id?: string }).push_id with a runtime
type guard: check payload for a string-valued push_id (e.g., verify payload is
an object and typeof payload.push_id === 'string') and assign that value or
undefined to a local push_id variable, then return push_id instead of the cast;
this ensures the push_id extraction in the client.ts function is validated at
runtime.

In `@cli/selftune/evolution/evolve.ts`:
- Around line 667-677: The evidence entry for the constitutional rejection is
missing original_text and proposed_text; update the recordEvidence call in
evolve.ts (the block that currently sets timestamp, proposal_id, skill_name,
skill_path, target, stage, rationale, confidence, details) to also include
original_text and proposed_text pulled from the proposal object (e.g.,
original_text: proposal.original_text and proposed_text: proposal.proposed_text,
or use safe fallbacks like empty strings if those properties can be undefined)
so it matches other rejection entries and downstream consumers receive
consistent fields.

In `@docs/exec-plans/active/user-owned-session-data-and-org-visible-outcomes.md`:
- Around line 196-199: Add a DB-level index/constraint plan to support
owner-scoped raw/session reads referenced in the API routes: create an index on
the owner column used by raw/session tables (and a composite index on (owner,
push_id) where joins trace raw data via push_id), add matching partial indexes
if org-scoped derived/aggregate routes use different predicates, and enforce a
foreign-key or not-null constraint on owner in raw/session tables so owner
filters cannot be bypassed; update the docs to call out these indexes and
constraints and ensure the API handlers that filter by owner (raw/session read
paths) rely on these indexed columns to avoid full-table scans.
- Around line 177-185: Update the backfill and constraint rollout section to
explicitly handle missing or mismatched ownership mappings: describe deriving
uploaded_by_user_id and owner_user_id from raw_pushes.user_id but falling back
to a sentinel value (e.g., NULL or a designated system_user_id) when
raw_pushes.user_id is missing or cannot be mapped, record those fallback cases
for auditability, and add a clear phased plan for enforcing NOT NULL and FK
constraints on owner_user_id/uploaded_by_user_id in the raw/session canonical
tables (include when to run a validation pass that rejects or remediates
mismatches, when to convert fallbacks to real IDs, and when to switch ON NOT
NULL/FK enforcement), plus a note to only add visibility where later sharing is
possible; reference the exact fields owner_user_id, uploaded_by_user_id and
source raw_pushes.user_id in the doc text so implementers can locate the
relevant schema/backfill code.
- Around line 96-104: The doc lacks an explicit rule that owner_user_id and
uploaded_by_user_id must be set server‑side from the authenticated session and
not trusted from the client; update the text around owner_user_id,
uploaded_by_user_id, and visibility to state that the server derives
owner_user_id from the local session identity and uploaded_by_user_id from the
authenticated cloud user (and ignores any client-supplied values), and describe
enforcement (reject or overwrite client-supplied owner/uploaded fields) in the
upload/ingest flow (the handler that processes session pushes); also clarify
visibility allowed values (private, org_shared, promoted) and any server-side
checks that map authenticated identity to org membership before setting
org-visible outcomes.

In `@tests/alpha-upload/e2e.test.ts`:
- Around line 597-624: The test contains an inline INSERT into
canonical_upload_staging that duplicates existing staging logic; replace this
inline loop with a call to the shared helper stageSessions to keep staging
consistent (either call stageSessions(db, 1, 10) if it supports an offset or
add/extend stageSessions(db, count, startIndex = 0) to accept a start index and
then call stageSessions(db, 1, 10)); ensure the inserted record uses the same
JSON shape and fields as stageSessions (record_kind/session JSON keys like
session_id, schema_version, normalizer_version, platform, capture_mode, etc.) so
behavior remains identical.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 90360c9b-0dd5-47a4-81be-9ee1d046cf62

📥 Commits

Reviewing files that changed from the base of the PR and between 99eabe1 and ba29ade.

📒 Files selected for processing (6)

• cli/selftune/alpha-upload/client.ts
• cli/selftune/evolution/constitutional.ts
• cli/selftune/evolution/evolve.ts
• cli/selftune/localdb/materialize.ts
• docs/exec-plans/active/user-owned-session-data-and-org-visible-outcomes.md
• tests/alpha-upload/e2e.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n cli/selftune/alpha-upload/client.ts | head -100

Repository: selftune-dev/selftune

Length of output: 3998

---

🏁 Script executed:

cat -n cli/selftune/alpha-upload/client.ts | sed -n '40,90p'

Repository: selftune-dev/selftune

Length of output: 1893

---

🏁 Script executed:

# Search for PushUploadResult type definition
rg -A 5 "type PushUploadResult|interface PushUploadResult" cli/

Repository: selftune-dev/selftune

Length of output: 410

---

🏁 Script executed:

# Search for the function signature containing line 62-66
ast-grep --pattern $'const $_ = ($_) => {
  $$$
  if (body.length === 0) {
    $$$
  }
  $$$
}'

Repository: selftune-dev/selftune

Length of output: 47

---

Use type guard instead of unsafe cast for push_id extraction (line 65).

(payload as { push_id?: string }).push_id bypasses runtime validation. Since payload is Record<string, unknown>, the cast provides no actual type safety. Narrow with a type check to ensure push_id is a string or undefined at runtime:

Suggested patch

       if (body.length === 0) {
+        const pushId =
+          typeof payload.push_id === "string" ? payload.push_id : undefined;
         return {
           success: true,
-          push_id: (payload as { push_id?: string }).push_id,
+          push_id: pushId,
           errors: [],
           _status: response.status,
         };
       }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (body.length === 0) {
	return {
	success: true,
	push_id: (payload as { push_id?: string }).push_id,
	errors: [],
	if (body.length === 0) {
	const pushId =
	typeof payload.push_id === "string" ? payload.push_id : undefined;
	return {
	success: true,
	push_id: pushId,
	errors: [],
	_status: response.status,
	};
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/client.ts` around lines 62 - 66, In the
early-return inside the body.length === 0 branch, replace the unsafe cast
(payload as { push_id?: string }).push_id with a runtime type guard: check
payload for a string-valued push_id (e.g., verify payload is an object and
typeof payload.push_id === 'string') and assign that value or undefined to a
local push_id variable, then return push_id instead of the cast; this ensures
the push_id extraction in the client.ts function is validated at runtime.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Evidence entry missing original_text and proposed_text fields.

Other rejection evidence entries (e.g., Pareto path at lines 526-539, confidence rejection at lines 699-709) include original_text and proposed_text. This constitutional rejection entry omits them, which could cause inconsistencies for downstream consumers.

Suggested fix

           recordEvidence({
             timestamp: new Date().toISOString(),
             proposal_id: proposal.proposal_id,
             skill_name: skillName,
             skill_path: skillPath,
             target: "description",
             stage: "rejected",
             rationale: proposal.rationale,
             confidence: proposal.confidence,
             details: feedbackReason,
+            original_text: proposal.original_description,
+            proposed_text: proposal.proposed_description,
           });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/evolution/evolve.ts` around lines 667 - 677, The evidence entry
for the constitutional rejection is missing original_text and proposed_text;
update the recordEvidence call in evolve.ts (the block that currently sets
timestamp, proposal_id, skill_name, skill_path, target, stage, rationale,
confidence, details) to also include original_text and proposed_text pulled from
the proposal object (e.g., original_text: proposal.original_text and
proposed_text: proposal.proposed_text, or use safe fallbacks like empty strings
if those properties can be undefined) so it matches other rejection entries and
downstream consumers receive consistent fields.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Define server-authoritative ownership assignment to prevent spoofing.

The doc introduces owner_user_id/uploaded_by_user_id but does not explicitly state these are derived from authenticated identity on the server (not client payload). Without that invariant, raw ownership can be forged.

Proposed doc patch

 ### New Fields

 Add the following as appropriate:

 - `owner_user_id`
 - `uploaded_by_user_id`
 - `visibility`

 Suggested semantics:
@@
 - `visibility`: `private`, `org_shared`, or `promoted`
+
+Trust boundary:
+
+- `owner_user_id` and `uploaded_by_user_id` must be server-assigned from the
+  authenticated principal/session context.
+- clients must not be allowed to arbitrarily set or override ownership fields
+  in push payloads.

Also applies to: 142-144

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/user-owned-session-data-and-org-visible-outcomes.md`
around lines 96 - 104, The doc lacks an explicit rule that owner_user_id and
uploaded_by_user_id must be set server‑side from the authenticated session and
not trusted from the client; update the text around owner_user_id,
uploaded_by_user_id, and visibility to state that the server derives
owner_user_id from the local session identity and uploaded_by_user_id from the
authenticated cloud user (and ignores any client-supplied values), and describe
enforcement (reject or overwrite client-supplied owner/uploaded fields) in the
upload/ingest flow (the handler that processes session pushes); also clarify
visibility allowed values (private, org_shared, promoted) and any server-side
checks that map authenticated identity to org membership before setting
org-visible outcomes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Backfill/constraint rollout is incomplete for null and mismatch cases.

Phase 1 says to derive ownership from raw_pushes.user_id, but it does not define handling for missing/invalid mappings or when to enforce NOT NULL/FK constraints. This can leave partially attributed rows and weaken the privacy model.

Proposed doc patch

 Backfill:

 - derive `uploaded_by_user_id` from `raw_pushes.user_id`
 - derive `owner_user_id` initially from the same source for current alpha data
+- define fallback handling for rows where `raw_pushes.user_id` is null/missing
+  (e.g., quarantine table + explicit remediation)
+- add rollout order: nullable column -> backfill -> validation query gates ->
+  `NOT NULL` + FK constraints

 Completion:

 - every raw/session canonical row can be attributed to a user

Also applies to: 188-189

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/user-owned-session-data-and-org-visible-outcomes.md`
around lines 177 - 185, Update the backfill and constraint rollout section to
explicitly handle missing or mismatched ownership mappings: describe deriving
uploaded_by_user_id and owner_user_id from raw_pushes.user_id but falling back
to a sentinel value (e.g., NULL or a designated system_user_id) when
raw_pushes.user_id is missing or cannot be mapped, record those fallback cases
for auditability, and add a clear phased plan for enforcing NOT NULL and FK
constraints on owner_user_id/uploaded_by_user_id in the raw/session canonical
tables (include when to run a validation pass that rejects or remediates
mismatches, when to convert fallbacks to real IDs, and when to switch ON NOT
NULL/FK enforcement), plus a note to only add visibility where later sharing is
possible; reference the exact fields owner_user_id, uploaded_by_user_id and
source raw_pushes.user_id in the doc text so implementers can locate the
relevant schema/backfill code.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Add required index/constraint plan for owner-scoped reads.

The plan requires owner-filtered raw reads, but there is no explicit index strategy or DB-level guardrails. Add this to avoid regressions and full scans once data volume grows.

Suggested additions

 Actions:

 - update API routes to filter raw/session data by owner
 - keep org filters for derived/aggregate routes
 - add targeted joins where raw data must be traced back through `push_id`
+- add composite indexes for hot paths (example: `(org_id, owner_user_id, created_at)`)
+- add DB constraints/checks so `visibility` values are bounded and ownership FK integrity is enforced

Also applies to: 260-267

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/exec-plans/active/user-owned-session-data-and-org-visible-outcomes.md`
around lines 196 - 199, Add a DB-level index/constraint plan to support
owner-scoped raw/session reads referenced in the API routes: create an index on
the owner column used by raw/session tables (and a composite index on (owner,
push_id) where joins trace raw data via push_id), add matching partial indexes
if org-scoped derived/aggregate routes use different predicates, and enforce a
foreign-key or not-null constraint on owner in raw/session tables so owner
filters cannot be bypassed; update the docs to call out these indexes and
constraints and ensure the API handlers that filter by owner (raw/session read
paths) rely on these indexed columns to avoid full-table scans.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Extract inline staging to helper for consistency.

This inline record insertion duplicates stageSessions logic. Consider calling the helper instead:

-    for (let i = 10; i < 11; i++) {
-      const sid = `e2e-session-${i}`;
-      db.run(
-        `INSERT OR IGNORE INTO canonical_upload_staging
-          (record_kind, record_id, record_json, session_id, staged_at)
-         VALUES (?, ?, ?, ?, ?)`,
-        [
-          "session",
-          sid,
-          JSON.stringify({
-            record_kind: "session",
-            schema_version: "2.0",
-            ...
-          }),
-          sid,
-          new Date().toISOString(),
-        ],
-      );
-    }
+    // Stage additional sessions with offset IDs to avoid collision
+    stageSessions(db, 1); // or add an offset parameter to stageSessions

Alternatively, extend stageSessions(db, count, startIndex = 0) to support offset IDs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/alpha-upload/e2e.test.ts` around lines 597 - 624, The test contains an
inline INSERT into canonical_upload_staging that duplicates existing staging
logic; replace this inline loop with a call to the shared helper stageSessions
to keep staging consistent (either call stageSessions(db, 1, 10) if it supports
an offset or add/extend stageSessions(db, count, startIndex = 0) to accept a
start index and then call stageSessions(db, 1, 10)); ensure the inserted record
uses the same JSON shape and fields as stageSessions (record_kind/session JSON
keys like session_id, schema_version, normalizer_version, platform,
capture_mode, etc.) so behavior remains identical.

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cli/selftune/observability.ts (1)

129-160: ⚠️ Potential issue | 🟠 Major

Blocking checks still count as a successful doctor() run.

checkHookInstallation() and checkConfigHealth() now emit guidance.blocking = true, but doctor() still marks the run healthy when there are no fail statuses. Missing hooks/config therefore still exits with code 0, so automation can treat an unusable install as healthy.

Either promote blocking bootstrap states to fail, or include guidance.blocking in the healthy / exit-code calculation.

As per coding guidelines: "cli/**/*.ts: Proper process.exit() with correct codes (0=success, 1=error)`."

Also applies to: 222-263, 505-529

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/observability.ts` around lines 129 - 160, The doctor() health
check currently treats any non-`fail` status as healthy, so blocking guidance
emitted by checkHookInstallation() and checkConfigHealth() (guidance.blocking =
true) still results in a successful run/exit code 0; update doctor() to consider
guidance.blocking as an error condition by either (A) converting checks that set
guidance.blocking into status = "fail" inside checkHookInstallation() /
checkConfigHealth(), or (B) changing doctor()'s health calculation to treat any
guidance.blocking === true as unhealthy (causing doctor() to return non-zero /
call process.exit(1)); ensure this change updates the overall health boolean and
the process.exit code path so automation sees blocking states as failures.

♻️ Duplicate comments (5)

cli/selftune/status.ts (1)

432-446: 🧹 Nitpick | 🔵 Trivial

Unenrolled-but-linked identities still render as "not enrolled".

When alphaIdentity exists with enrolled: false but has a cloud_user_id (i.e., getAlphaLinkState() returns "linked_not_enrolled"), this block skips building alphaInfo, so formatAlphaStatus(null) displays generic "not enrolled" guidance instead of the more specific "linked (not enrolled)" state with targeted remediation.

Consider building alphaInfo for any non-null alphaIdentity to surface the actual link state:

♻️ Suggested approach

     const alphaIdentity = readAlphaIdentity(SELFTUNE_CONFIG_PATH);
     let alphaInfo: AlphaStatusInfo | null = null;
-    if (alphaIdentity?.enrolled) {
+    if (alphaIdentity) {
+      const linkState = getAlphaLinkState(alphaIdentity);
       alphaInfo = {
-        enrolled: true,
-        linkState: getAlphaLinkState(alphaIdentity),
+        enrolled: alphaIdentity.enrolled === true,
+        linkState,
         guidance: getAlphaGuidance(alphaIdentity),
-        stats: getQueueStats(db),
-        lastError: getLastUploadError(db),
-        lastSuccess: getLastUploadSuccess(db),
+        stats: alphaIdentity.enrolled ? getQueueStats(db) : { pending: 0, sending: 0, sent: 0, failed: 0 },
+        lastError: alphaIdentity.enrolled ? getLastUploadError(db) : null,
+        lastSuccess: alphaIdentity.enrolled ? getLastUploadSuccess(db) : null,
       };
     }

This would require changing AlphaStatusInfo.enrolled back to boolean or creating a union type.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/status.ts` around lines 432 - 446, The code currently only
constructs alphaInfo when alphaIdentity?.enrolled is true, causing
unenrolled-but-linked identities to be rendered as generic "not enrolled";
update the logic to build alphaInfo for any non-null alphaIdentity (use
readAlphaIdentity and alphaIdentity directly) by setting
AlphaStatusInfo.enrolled = alphaIdentity.enrolled (boolean) and populating
linkState via getAlphaLinkState(alphaIdentity), guidance via
getAlphaGuidance(alphaIdentity), and stats/lastError/lastSuccess via
getQueueStats(db), getLastUploadError(db), getLastUploadSuccess(db); if
necessary, change the AlphaStatusInfo type (or its enrolled field) from
optional/union to a boolean so formatAlphaStatus(alphaInfo) can display the
specific "linked_not_enrolled" state.

cli/selftune/alpha-upload/build-payloads.ts (1)

102-117: ⚠️ Potential issue | 🟠 Major

Evidence entries still lose timestamp, skill_path, and details in the final payload.

The evidenceEntries array here correctly includes all fields, but buildPushPayloadV2() in canonical-export.ts (lines 103-114) only emits a subset: evidence_id, skill_name, proposal_id, target, stage, rationale, confidence, original_text, proposed_text, eval_set_json, validation_json.

The fields timestamp, skill_path, and details are dropped during serialization, making the upload lossy despite staging the full record.

#!/bin/bash
# Verify which fields buildPushPayloadV2 actually emits for evolution_evidence
rg -A 20 'evolution_evidence:.*map' cli/selftune/canonical-export.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/alpha-upload/build-payloads.ts` around lines 102 - 117,
buildPushPayloadV2's evolution_evidence mapping currently omits timestamp,
skill_path, and details when serializing evidenceEntries; update the
evolution_evidence map inside buildPushPayloadV2 (canonical-export.ts) to
include these fields (timestamp, skill_path, details) alongside the existing
keys (evidence_id, skill_name, proposal_id, target, stage, rationale,
confidence, original_text, proposed_text, eval_set_json, validation_json) so the
full staged record from evidenceEntries is preserved in the final payload.

cli/selftune/init.ts (1)

568-573: 🧹 Nitpick | 🔵 Trivial

Brief window with default file permissions before chmodSync.

writeFileSync then chmodSync leaves a momentary gap where the config file containing api_key has wider permissions. Low risk for local CLI, but atomic write (temp file with restricted perms, then rename) would eliminate it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/selftune/init.ts` around lines 568 - 573, Currently writeFileSync
followed by chmodSync briefly leaves the config file with default permissions;
change the flow to perform an atomic write: create a temporary file next to
configPath (e.g., configPath + ".tmp"), open it with restrictive mode (0o600) or
set the mode before writing, write the JSON for config.alpha (the same data
written where validatedAlphaIdentity is used), fsync and close the temp file,
then rename the temp file to configPath atomically (renameSync). Remove the
separate chmodSync call and ensure this sequence is used only when opts.alphaKey
is present so the sensitive api_key never exists with broader permissions.

tests/alpha-upload/build-payloads.test.ts (1)

29-34: ⚠️ Potential issue | 🟡 Minor

Silent error suppression for index DDL can hide real failures.

POST_MIGRATION_INDEXES already uses CREATE INDEX IF NOT EXISTS, so catching all errors here masks legitimate schema problems (e.g., referencing renamed columns). Let the errors propagate to fail fast.

Suggested fix

   for (const idx of POST_MIGRATION_INDEXES) {
-    try {
-      db.run(idx);
-    } catch {
-      /* already exists OK */
-    }
+    db.run(idx);
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/alpha-upload/build-payloads.test.ts` around lines 29 - 34, The loop
that applies index DDL suppresses all errors and should not do that—remove the
try/catch around db.run(idx) in the loop that iterates POST_MIGRATION_INDEXES so
any real schema errors (e.g., referencing renamed columns) will surface and fail
the test; keep using the existing CREATE INDEX IF NOT EXISTS SQL but let db.run
throw rather than swallowing exceptions.

tests/init/alpha-onboarding-e2e.test.ts (1)

79-84: ⚠️ Potential issue | 🟡 Minor

Avoid live-looking API key literals in test fixtures.

st_live_abc123xyz triggers secret scanning (Gitleaks flagged line 79). Build the value at runtime to avoid CI blocks or allowlist maintenance.

+    const testApiKey = ["st_live", "TEST_ONLY_E2E"].join("_");
     const config2 = runInit(
       makeInitOpts({
         alpha: true,
         alphaEmail: "user@example.com",
-        alphaKey: "st_live_abc123xyz",
+        alphaKey: testApiKey,
         force: true,
       }),
     );
 
-    expect(config2.alpha?.api_key).toBe("st_live_abc123xyz");
+    expect(config2.alpha?.api_key).toBe(testApiKey);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/init/alpha-onboarding-e2e.test.ts` around lines 79 - 84, The test uses
a live-looking literal "st_live_abc123xyz" which triggers secret scanners;
instead, construct the API key at runtime and use that constructed value in both
the request fixture (alphaKey) and the assertion (expect(...).toBe(...)). Locate
the test function in alpha-onboarding-e2e.test.ts that sets alphaKey and the
subsequent expectation on config2.alpha?.api_key, replace the literal with a
dynamically concatenated string (e.g., pieces joined at runtime) so the value is
not a single secret-looking literal while preserving the exact expected value
for the assertion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/local-dashboard/src/components/runtime-footer.tsx`:
- Around line 4-14: The type guard isHealthResponse currently rejects valid
HealthResponse objects because it checks watcher_mode === "sqlite" || "jsonl"
while the contract defines watcher_mode as "jsonl" | "none"; update the check in
isHealthResponse to accept "jsonl" or "none" (so valid responses with
watcher_mode: "none" pass), and ensure the HealthResponse type/import used by
isHealthResponse matches the dashboard-contract.ts definition; refer to the
isHealthResponse function and HealthResponse type when making the change.

In `@cli/selftune/observability.ts`:
- Around line 408-413: Change the guidance for the failCheck with code
"alpha_queue_failures" so its primary next_command points at the recovery action
("selftune alpha upload") instead of "selftune doctor"; leave "selftune doctor"
in suggested_commands as a follow-up. Update the object assigned to
failCheck.guidance (the properties
code/message/next_command/suggested_commands/blocking) for the
alpha_queue_failures check so the agent is directed to run selftune alpha upload
first and only run doctor afterwards if the upload does not resolve the issue.
- Around line 490-492: The early-return in checkCloudLinkHealth suppresses the
"not_linked" state by returning [] when an AlphaIdentity exists locally but
lacks enrolled/cloud_user_id/api_key; change the guard so you only return []
when identity is null (i.e., if (!identity) return []), and always call
getAlphaLinkState(identity) for non-null identities so the function can emit the
not_linked HealthCheck. Update checkCloudLinkHealth to remove the complex falsy
check and rely on getAlphaLinkState to classify enrollment/linkage.

In `@skill/SKILL.md`:
- Around line 107-110: The routing table exposes the AlphaUpload handler but the
CLI command and flags are missing from documentation; update SKILL.md Quick
Reference to include the "selftune alpha upload" command and its synopsis, then
create or update the corresponding workflow doc (skill/Workflows/* for
AlphaUpload or the direct-command section) to list all flags (e.g.,
--alpha-email, --alpha-key, any others used by AlphaUpload in index.ts), a short
flags table, and example invocations; ensure the workflow routing table entry
and any references to the AlphaUpload symbol (handler/class/function name) match
the CLI name in index.ts so docs and routing stay in sync.
- Around line 124-137: The routing table in SKILL.md has ambiguous trigger
terms: the token "replay" is listed for both the Replay and Sync workflows and
"autonomous" is listed for both Cron and Orchestrate, which will break dispatch;
update the table so each workflow has distinct trigger terms (e.g., remove
"replay" from one row and keep it only for Replay, and keep "autonomous" only
for either Cron or Orchestrate), or explicitly add a precedence note for
ambiguous tokens; make the change in the rows referencing Replay, Sync, Cron and
Orchestrate so the agent routing is unambiguous.

---

Outside diff comments:
In `@cli/selftune/observability.ts`:
- Around line 129-160: The doctor() health check currently treats any non-`fail`
status as healthy, so blocking guidance emitted by checkHookInstallation() and
checkConfigHealth() (guidance.blocking = true) still results in a successful
run/exit code 0; update doctor() to consider guidance.blocking as an error
condition by either (A) converting checks that set guidance.blocking into status
= "fail" inside checkHookInstallation() / checkConfigHealth(), or (B) changing
doctor()'s health calculation to treat any guidance.blocking === true as
unhealthy (causing doctor() to return non-zero / call process.exit(1)); ensure
this change updates the overall health boolean and the process.exit code path so
automation sees blocking states as failures.

---

Duplicate comments:
In `@cli/selftune/alpha-upload/build-payloads.ts`:
- Around line 102-117: buildPushPayloadV2's evolution_evidence mapping currently
omits timestamp, skill_path, and details when serializing evidenceEntries;
update the evolution_evidence map inside buildPushPayloadV2
(canonical-export.ts) to include these fields (timestamp, skill_path, details)
alongside the existing keys (evidence_id, skill_name, proposal_id, target,
stage, rationale, confidence, original_text, proposed_text, eval_set_json,
validation_json) so the full staged record from evidenceEntries is preserved in
the final payload.

In `@cli/selftune/init.ts`:
- Around line 568-573: Currently writeFileSync followed by chmodSync briefly
leaves the config file with default permissions; change the flow to perform an
atomic write: create a temporary file next to configPath (e.g., configPath +
".tmp"), open it with restrictive mode (0o600) or set the mode before writing,
write the JSON for config.alpha (the same data written where
validatedAlphaIdentity is used), fsync and close the temp file, then rename the
temp file to configPath atomically (renameSync). Remove the separate chmodSync
call and ensure this sequence is used only when opts.alphaKey is present so the
sensitive api_key never exists with broader permissions.

In `@cli/selftune/status.ts`:
- Around line 432-446: The code currently only constructs alphaInfo when
alphaIdentity?.enrolled is true, causing unenrolled-but-linked identities to be
rendered as generic "not enrolled"; update the logic to build alphaInfo for any
non-null alphaIdentity (use readAlphaIdentity and alphaIdentity directly) by
setting AlphaStatusInfo.enrolled = alphaIdentity.enrolled (boolean) and
populating linkState via getAlphaLinkState(alphaIdentity), guidance via
getAlphaGuidance(alphaIdentity), and stats/lastError/lastSuccess via
getQueueStats(db), getLastUploadError(db), getLastUploadSuccess(db); if
necessary, change the AlphaStatusInfo type (or its enrolled field) from
optional/union to a boolean so formatAlphaStatus(alphaInfo) can display the
specific "linked_not_enrolled" state.

In `@tests/alpha-upload/build-payloads.test.ts`:
- Around line 29-34: The loop that applies index DDL suppresses all errors and
should not do that—remove the try/catch around db.run(idx) in the loop that
iterates POST_MIGRATION_INDEXES so any real schema errors (e.g., referencing
renamed columns) will surface and fail the test; keep using the existing CREATE
INDEX IF NOT EXISTS SQL but let db.run throw rather than swallowing exceptions.

In `@tests/init/alpha-onboarding-e2e.test.ts`:
- Around line 79-84: The test uses a live-looking literal "st_live_abc123xyz"
which triggers secret scanners; instead, construct the API key at runtime and
use that constructed value in both the request fixture (alphaKey) and the
assertion (expect(...).toBe(...)). Locate the test function in
alpha-onboarding-e2e.test.ts that sets alphaKey and the subsequent expectation
on config2.alpha?.api_key, replace the literal with a dynamically concatenated
string (e.g., pieces joined at runtime) so the value is not a single
secret-looking literal while preserving the exact expected value for the
assertion.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a0fa3c6b-62d8-461e-926c-0ba076fcd9ff

📥 Commits

Reviewing files that changed from the base of the PR and between ba29ade and 071d2bb.

📒 Files selected for processing (19)

• apps/local-dashboard/src/components/runtime-footer.tsx
• cli/selftune/agent-guidance.ts
• cli/selftune/alpha-upload/build-payloads.ts
• cli/selftune/alpha-upload/stage-canonical.ts
• cli/selftune/index.ts
• cli/selftune/init.ts
• cli/selftune/observability.ts
• cli/selftune/status.ts
• cli/selftune/types.ts
• scripts/validate-subagent-docs.ts
• skill/SKILL.md
• skill/Workflows/Doctor.md
• skill/Workflows/Initialize.md
• skill/agents/evolution-reviewer.md
• tests/alpha-upload/build-payloads.test.ts
• tests/alpha-upload/integration.test.ts
• tests/alpha-upload/status.test.ts
• tests/init/alpha-onboarding-e2e.test.ts
• tests/observability.test.ts

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

skill/Workflows/Initialize.md (2)

174-176: ⚠️ Potential issue | 🟡 Minor

Fix hook snippet path to match init.ts source.

This section still points agents to assets/settings_snippet.json, but init hook merge behavior is documented against skill/settings_snippet.json. Update the hook bullet to the actual source file to avoid troubleshooting drift.

Suggested doc fix

-Templates for each project type are in the `assets/` directory:
-- `assets/settings_snippet.json` — hooks for Claude Code projects
+Templates for each project type are in the `assets/` directory:
+- `skill/settings_snippet.json` — hooks for Claude Code projects
 - `assets/activation-rules-default.json` — default auto-activation rule configuration

Based on learnings, Hook behavior changes in hooks/*.ts must be documented in skill/Workflows/Initialize.md and reflected in skill/settings_snippet.json.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skill/Workflows/Initialize.md` around lines 174 - 176, The docs in
Workflows/Initialize.md list the hook template as assets/settings_snippet.json
but init.ts and the actual hook merge behavior use skill/settings_snippet.json;
update the bullet to reference skill/settings_snippet.json and ensure the
documentation describes current hook behavior from hooks/*.ts (and, if
necessary, sync any differences between skill/settings_snippet.json and the
hooks/*.ts logic so the file name and described behavior match init.ts's
implementation).

---

209-212: ⚠️ Potential issue | 🟡 Minor

Add blank lines around fenced code in the list item.

The fenced block under “Store credential locally” should be separated with blank lines to stay markdownlint-clean (MD031).

Suggested doc fix

 4. **Store credential locally**:
+
    ```bash
    selftune init --alpha --alpha-email <user-email> --alpha-key <st_live_credential>
    ```
+

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skill/Workflows/Initialize.md` around lines 209 - 212, In Initialize.md under
the "Store credential locally" list item, add a blank line before the opening
fenced code block and a blank line after the closing triple backticks so the
code block is separated from list text (fixes MD031); locate the fenced block
shown as "selftune init --alpha --alpha-email <user-email> --alpha-key
<st_live_credential>" and insert the blank lines immediately surrounding that
code fence.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/design-docs/alpha-remote-data-contract.md`:
- Around line 233-237: Update the doc to reflect the actual retry semantics:
change the description of the flush flow so it clarifies that flushQueue (the
flush function) queries upload_queue for status = 'pending' only, performs all
retry attempts and backoff inside a single flushQueue call, and when an item
exhausts retries it is marked terminally as status = 'failed' (with attempts,
last_attempt_at, last_error set) rather than being returned to 'pending' for
future flushes; also clarify that successful responses (201 or 409) set status =
'sent' and sent_at, and classify HTTP codes (429, 5xx as retryable within the
same flushQueue run; 401, 403 as non-retryable and terminal).

---

Duplicate comments:
In `@skill/Workflows/Initialize.md`:
- Around line 174-176: The docs in Workflows/Initialize.md list the hook
template as assets/settings_snippet.json but init.ts and the actual hook merge
behavior use skill/settings_snippet.json; update the bullet to reference
skill/settings_snippet.json and ensure the documentation describes current hook
behavior from hooks/*.ts (and, if necessary, sync any differences between
skill/settings_snippet.json and the hooks/*.ts logic so the file name and
described behavior match init.ts's implementation).
- Around line 209-212: In Initialize.md under the "Store credential locally"
list item, add a blank line before the opening fenced code block and a blank
line after the closing triple backticks so the code block is separated from list
text (fixes MD031); locate the fenced block shown as "selftune init --alpha
--alpha-email <user-email> --alpha-key <st_live_credential>" and insert the
blank lines immediately surrounding that code fence.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eee9ef69-376f-4370-b8de-fba14ef258a3

📥 Commits

Reviewing files that changed from the base of the PR and between 071d2bb and 986ffa7.

📒 Files selected for processing (2)

• docs/design-docs/alpha-remote-data-contract.md
• skill/Workflows/Initialize.md