diff --git a/src/resources/extensions/sf/self-feedback.js b/src/resources/extensions/sf/self-feedback.js index 0d6507929..8570bf499 100644 --- a/src/resources/extensions/sf/self-feedback.js +++ b/src/resources/extensions/sf/self-feedback.js @@ -46,6 +46,7 @@ import { resolveSelfFeedbackEntry, } from "./sf-db.js"; import { createMemory } from "./memory-store.js"; +import { logWarning } from "./workflow-logger.js"; const SELF_FEEDBACK_HEADER = "# SF Self-Feedback\n\n" + @@ -394,12 +395,96 @@ function readActiveUnit(basePath) { return undefined; } } +/** + * Allowed top-level domains for self-feedback `kind` strings (sf-mp4rxkwt-sfthez). + * + * Why an enum + structured shape: free-form `kind` made the threshold-promoter + * cluster by exact-string and almost never fire — every entry has a unique + * suffix. With a `domain:family[:specific]` shape, the promoter can cluster + * by `domain:family` prefix and detectors gain a stable taxonomy. Reflection + * passes can also reason about whole domains rather than handfuls of unique + * strings. Existing entries with single-segment kinds remain readable + * (we only validate NEW writes via recordSelfFeedback) so the migration is + * non-destructive. + * + * Adding a new domain: add it here AND name the family/specific convention + * you're introducing in the entry's PDD so future operators can find it. + */ +const ALLOWED_KIND_DOMAINS = new Set([ + "gap", + "architecture-defect", + "architectural-risk", + "inconsistency", + "runaway-loop", + "runaway-guard-hard-pause", + "schema-drift", + "janitor-gap", + "upstream-rollup", + "reflection", + "copilot-parity-gaps", + "gap-audit-orphan-prompt", + "gap-audit-orphan-command", + "flow-audit", + "executor-refused", + "solver-missing-checkpoint", + "self-feedback-resolution", +]); + +const KIND_SEGMENT_RE = /^[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/; + +/** + * Validate a kind string. Accepts: + * `domain` — single-segment legacy shape, requires the + * domain to be in the allow-list. We accept this + * so existing detectors that file e.g. + * `runaway-guard-hard-pause` keep working until + * they're migrated to a `domain:family` form. + * `domain:family` — two-segment canonical shape. Both segments + * kebab-case. Domain must be in the allow-list. + * `domain:family:specific` — three-segment specific shape, all kebab. + * + * Returns `null` when valid; an error message string when not. The string + * surface is so recordSelfFeedback callers (and tests) can see WHY a write + * was refused instead of guessing. + */ +function validateKind(kind) { + if (typeof kind !== "string" || kind.length === 0) { + return "kind must be a non-empty string"; + } + const segments = kind.split(":"); + if (segments.length < 1 || segments.length > 3) { + return `kind "${kind}" must have 1, 2, or 3 colon-separated segments (got ${segments.length})`; + } + for (const seg of segments) { + if (!KIND_SEGMENT_RE.test(seg)) { + return `kind "${kind}" segment "${seg}" must be kebab-case (lowercase letters/digits, internal hyphens)`; + } + } + if (!ALLOWED_KIND_DOMAINS.has(segments[0])) { + return `kind "${kind}" uses unknown domain "${segments[0]}"; valid domains: ${Array.from(ALLOWED_KIND_DOMAINS).sort().join(", ")}`; + } + return null; +} + // ─── Public API ──────────────────────────────────────────────────────────── /** * Record a self-feedback entry to the appropriate channel (project or upstream). * Non-fatal — write errors are swallowed and null is returned on failure. + * + * Kind validation (sf-mp4rxkwt-sfthez): the entry's `kind` MUST satisfy + * validateKind — see ALLOWED_KIND_DOMAINS + KIND_SEGMENT_RE. Writes with + * malformed kinds return null and emit a warning to workflow-logger. */ export function recordSelfFeedback(entry, basePath = process.cwd()) { + const kindError = validateKind(entry?.kind); + if (kindError) { + try { + logWarning("self-feedback", `recordSelfFeedback rejected: ${kindError}`); + } catch { + // best-effort — never block on logger failure + } + return null; + } try { const occurredIn = entry.occurredIn ?? readActiveUnit(basePath); const persisted = { diff --git a/src/resources/extensions/sf/tests/self-feedback-db.test.mjs b/src/resources/extensions/sf/tests/self-feedback-db.test.mjs index f304f41aa..f448735f6 100644 --- a/src/resources/extensions/sf/tests/self-feedback-db.test.mjs +++ b/src/resources/extensions/sf/tests/self-feedback-db.test.mjs @@ -55,7 +55,7 @@ test("recordSelfFeedback_when_db_available_writes_sqlite_and_versioned_projectio const result = recordSelfFeedback( { - kind: "test-feedback", + kind: "gap:test-feedback", severity: "medium", summary: "DB backed feedback", evidence: "unit test", @@ -83,7 +83,7 @@ test("readAllSelfFeedback_imports_legacy_jsonl_once_when_db_available", () => { const legacyEntry = { id: "legacy-1", ts: "2026-05-07T00:00:00.000Z", - kind: "legacy-feedback", + kind: "gap:legacy-feedback", severity: "high", blocking: true, summary: "Legacy JSONL entry", @@ -109,7 +109,7 @@ test("markResolved_when_db_available_updates_sqlite_and_markdown_projection", () const project = makeForgeProject(); const result = recordSelfFeedback( { - kind: "resolvable-feedback", + kind: "gap:resolvable-feedback", severity: "high", summary: "Resolve through DB", }, @@ -144,7 +144,7 @@ test("markResolved_rejects_resolution_with_non_canonical_evidence_kind", () => { const project = makeForgeProject(); const result = recordSelfFeedback( { - kind: "writer-layer-test", + kind: "gap:writer-layer-test", severity: "medium", summary: "Must be rejected when evidence kind is non-canonical", }, @@ -195,7 +195,7 @@ test("markResolved_accepts_each_canonical_evidence_kind", () => { ]; for (const evidence of kinds) { const filed = recordSelfFeedback( - { kind: `accepts-${evidence.kind}`, severity: "low", summary: "x" }, + { kind: `gap:accepts-${evidence.kind}`, severity: "low", summary: "x" }, project, ); const ok = markResolved( @@ -223,7 +223,7 @@ test("markResolved_rejects_agent_fix_with_nonexistent_commit_sha", async () => { }); const filed = recordSelfFeedback( - { kind: "commit-verif-test", severity: "high", summary: "x" }, + { kind: "gap:commit-verif-test", severity: "high", summary: "x" }, project, ); @@ -261,7 +261,7 @@ test("markResolved_accepts_agent_fix_with_no_commit_sha_or_ungrokable_path", () const project = makeForgeProject(); // No git init → ungrokable → carve-out allows the resolution const filed = recordSelfFeedback( - { kind: "no-git-test", severity: "low", summary: "x" }, + { kind: "gap:no-git-test", severity: "low", summary: "x" }, project, ); const ok = markResolved( @@ -277,7 +277,7 @@ test("markResolved_accepts_agent_fix_with_no_commit_sha_or_ungrokable_path", () // agent-fix without any commitSha is also accepted (commit not always // the right artifact — summaryNarrative or testPath may be) const filed2 = recordSelfFeedback( - { kind: "no-sha-test", severity: "low", summary: "x" }, + { kind: "gap:no-sha-test", severity: "low", summary: "x" }, project, ); const ok2 = markResolved( @@ -369,11 +369,67 @@ test("markResolved_memory_mirror_handles_human_clear_without_commit_tags", async ); }); +test("recordSelfFeedback_kind_validation_accepts_canonical_shapes", () => { + const project = makeForgeProject(); + const cases = [ + "gap", // 1-segment legacy (allowed-domain) + "gap:routing", // 2-segment domain:family + "gap:routing:tiebreak-cost-only", // 3-segment domain:family:specific + "architecture-defect:foo", // hyphenated domain works + "architecture-defect:solver:executor-conflation", + "runaway-loop", // 1-segment legacy + "upstream-rollup:gap-audit-orphan-prompt", + ]; + for (const kind of cases) { + const result = recordSelfFeedback( + { kind, severity: "low", summary: `test ${kind}` }, + project, + ); + assert.ok(result, `expected ${kind} to be accepted`); + assert.equal(result.entry.kind, kind); + } +}); + +test("recordSelfFeedback_kind_validation_rejects_malformed", () => { + const project = makeForgeProject(); + const cases = [ + { kind: "" }, // empty + { kind: "Unknown" }, // not in domain allow-list + { kind: "gap:" }, // empty family + { kind: ":family" }, // empty domain + { kind: "gap:routing:tiebreak:extra" },// 4 segments + { kind: "gap:Routing" }, // uppercase in family (not kebab) + { kind: "gap:routing_cost" }, // underscore (not kebab) + { kind: "gap:-routing" }, // leading hyphen + { kind: "gap:routing-" }, // trailing hyphen + { kind: "9gap:routing" }, // domain starts with digit + { kind: "weird-domain:family" }, // unknown domain + ]; + for (const { kind } of cases) { + const result = recordSelfFeedback( + { kind, severity: "low", summary: `should reject ${kind}` }, + project, + ); + assert.equal(result, null, `expected ${JSON.stringify(kind)} to be rejected`); + } +}); + +test("recordSelfFeedback_kind_validation_rejects_non_string", () => { + const project = makeForgeProject(); + for (const kind of [undefined, null, 42, ["gap"], { domain: "gap" }]) { + const result = recordSelfFeedback( + { kind, severity: "low", summary: "non-string kind" }, + project, + ); + assert.equal(result, null, `expected non-string ${JSON.stringify(kind)} to be rejected`); + } +}); + test("markResolved_appends_resolution_event_to_jsonl_audit_log", () => { const project = makeForgeProject(); const result = recordSelfFeedback( { - kind: "audit-trail", + kind: "gap:audit-trail", severity: "high", summary: "Resolution must land in JSONL too", }, @@ -479,7 +535,7 @@ test("compactSelfFeedbackMarkdown_when_projection_stale_rewrites_from_sqlite", ( const project = makeForgeProject(); const result = recordSelfFeedback( { - kind: "stale-projection", + kind: "gap:stale-projection", severity: "medium", summary: "Projection should be repaired", },