diff --git a/docs/records/2026-05-02-bug-hunt-findings.md b/docs/records/2026-05-02-bug-hunt-findings.md new file mode 100644 index 000000000..be5b0bd80 --- /dev/null +++ b/docs/records/2026-05-02-bug-hunt-findings.md @@ -0,0 +1,338 @@ +# Bug Hunt Findings — 2026-05-02 + +Read-only audit across 6 clusters of /src/resources/extensions/sf/. 55 total +findings: 15 HIGH, 22 MEDIUM, 18 LOW. + +## Methodology + +6 haiku-powered read-only agents inspected files in parallel, instructed to +report only verified bugs (no hallucination). Each agent constrained to its +cluster, no edits, no test runs. + +## Status legend + +- 🔴 HIGH: data loss / crash / security / definite wrong +- 🟡 MEDIUM: probable bug under specific conditions +- 🟢 LOW: robustness / style + +--- + +## Cluster: engine + verification + +### HIGH + +- `verification-gate.ts:451` [async/concurrency] **Status: FALSE-POSITIVE** + `checkAsyncStyleDrift()` returns `passed: true` when style drift is detected. The agent flagged this as wrong, but per the user's own verification: `// Warning only` comments are intentional — the field is correct as-is. + Suggested fix: N/A (intentional design). + +- `post-execution-checks.ts:452` [async/concurrency] **Status: FALSE-POSITIVE** + Same pattern: `checkAsyncStyleDrift()` flags style drift but returns `passed: true`. The `// Warning only` comment confirms this is intentional. + Suggested fix: N/A (intentional design). + +- `production-mutation-approval.ts:268` [inverted condition] **Status: FALSE-POSITIVE** + Agent flagged that `data.risk` is accessed without existence check. Per user verification, `data.risk !== "expected"` correctly flags missing fields — the inequality on undefined already signals the missing-field case. + Suggested fix: N/A (intentional design). + +### MEDIUM + +- `verification-gate.ts:134-150` [resource efficiency] **Status: CONFIRMED** + `formatFailureContext()` accumulates per-check stderr up to 2000 chars each, then truncates the whole output at 10 000 chars. With 5 failing checks the tail is silently cut mid-line, losing diagnostics from later checks. + Suggested fix: truncate per-check to `floor(10000 / failCount)` chars so each check gets a fair share, or emit a "…N chars omitted" sentinel. + +- `pre-execution-checks.ts:73-106` [logic] **Status: CONFIRMED** + `extractPackageReferences()` silently drops package names that collide with the stopwords set. `npm install -D test` is a concrete example: the string `"test"` is a stopword so it is not captured even when the intent is the `test` package. + Suggested fix: scope the stopword check to bare flag-tokens (tokens immediately after a flag like `-D`), not to all positional tokens. + +- `post-execution-checks.ts:356-366` [logic] **Status: CONFIRMED** + `checkCrossTaskSignatures()` compares against only `priorDefs[0]`. If the same function was defined with different signatures across multiple prior tasks, only the first definition is checked, missing signature drift chains. + Suggested fix: iterate all prior definitions and report any that diverge from the current. + +- `dispatch-guard.ts:104-150` [logic] **Status: CONFIRMED** + When any slice in a milestone declares explicit dependencies, the positional-fallback is skipped globally, not just for that slice. Slices with no declared dependencies can therefore be dispatched out of order. + Suggested fix: apply the `null` short-circuit only to the target slice, not to the entire milestone pass. + +### LOW + +- `verification-gate.ts:240-271` [robustness] **Status: CONFIRMED** + `isLikelyCommand()` heuristics are gameable (e.g., short prose sentences with package-name-like words). Safe because `sanitizeCommand()` adds a second check, but the heuristics produce noisy misclassifications. + Suggested fix: add a minimum word-count guard and explicit prose-article detection. + +- `gate-registry.ts:185-186` [style] **Status: CONFIRMED** + `ORDERED_GATES` uses `Object.values()` without a comment noting the ES2020 insertion-order guarantee. Pre-ES2020 engines (or linters that warn about object-key ordering) may flag this. + Suggested fix: add an explicit ordering comment or use an explicit array literal. + +- `custom-verification.ts:167-172` [robustness] **Status: CONFIRMED** + Shell command guard rejects `$(`, backtick, `;rm`, `;curl`, etc., but not `&` or `&&`. `ls && rm -rf /` passes the dangerous-pattern check. + Suggested fix: add `&&` and `||` operator patterns to the rejection list. + +- `pre-execution-checks.ts:265-287` [robustness] **Status: CONFIRMED** + `normalizeFilePath()` strips `./`, normalises separators, and deduplicates segments. The normalised output is not filesystem-safe if used directly in a subsequent path operation without re-validation. + Suggested fix: add a JSDoc note that the return value is for comparison only, not disk I/O. + +--- + +## Cluster: scaffold + doctor + +### HIGH + +- `doctor-environment.ts:404` [logic error] **Status: CONFIRMED** + Port-conflict detection parses `lsof` output with a regex whose capture group `[2]` reliably returns empty or garbage for process names. PID extraction (line 409) is correct, but the process-name field shown to users is unreliable. + Suggested fix: Use `lsof -F cn` (field output mode) or parse the `COMMAND` column by position rather than regex group index. + +- `doctor-runtime-checks.ts:394` [redundant dynamic import] **Status: FIXED** + `checkRuntimeHealth` re-imports `detectScaffoldDrift` dynamically inside the try-block even though a static import already exists at line 27. The fix-swarm agent removed the dynamic import and confirmed the static import suffices. + Suggested fix: Already applied by fix swarm (task `a1ba9d693050d27a1`). + +### MEDIUM + +- `doctor-runtime-checks.ts:737-774` [code duplication] **Status: CONFIRMED** + `checkScaffoldFreshness` (exported) duplicates drift-check logic already in `checkRuntimeHealth`. Per fix-swarm review, the two functions serve different call sites, so they are not identical — but the shared logic still diverges risk. + Suggested fix: Extract common bucket-count message logic into a private helper. + +- `doctor-environment.ts:375` [regex over-matching] **Status: CONFIRMED** + Port-detection regex can match colons in IPv6 addresses or other non-port contexts. The 1024–65535 range filter reduces false positives but the pattern itself is imprecise. + Suggested fix: Anchor the alternation more tightly (require preceding whitespace or `=` for the `PORT=` variant). + +- `scaffold-drift.ts:221` [classification inconsistency] **Status: CONFIRMED** + A file with `pending` state, matching hash, and current version is labelled `customized` rather than something like `no_action_needed`. This misleads users into thinking they made edits when they did not. Per fix-swarm, the bucket assignment is by design for the current spec, so no code change was made — but the label is confusing. + Suggested fix: rename the bucket or add a sub-label distinguishing user edits from version-matched pending files. + +### LOW + +- `scaffold-versioning.ts:249` [style] **Status: CONFIRMED** + Four consecutive `(e as ScaffoldManifestEntry)` casts in one filter predicate. Repetitive and hides the intent. + Suggested fix: Extract a `isScaffoldManifestEntry(e): e is ScaffoldManifestEntry` type guard. + +- `doctor.ts:1119` [misleading variable] **Status: CONFIRMED** + `sfState` timing is computed as `now - t0env - envMs` rather than being independently measured, making the value approximate and confusingly named. + Suggested fix: Add `const t0state = Date.now()` before state derivation; compute `sfState = Date.now() - t0state` explicitly. + +- `doctor-environment.ts:803` [style] **Status: CONFIRMED** + Hard-coded emoji glyphs (✅, ⚠️, 🛑) in `formatEnvironmentReport` may not render on all terminals. + Suggested fix: Guard behind a `process.env.NO_COLOR` or `isCI()` check, or make the icons configurable. + +--- + +## Cluster: worktree + git + +### HIGH + +- `worktree-resolver.ts:616` [merge-state-cleanup] **Status: CONFIRMED** + Cleanup of `SQUASH_MSG`/`MERGE_HEAD` after a failed merge uses `join(originalBase, ".git")`. In a git worktree the `.git` entry is a pointer file, not a directory; the real merge state lives in `.git/worktrees//`. The cleanup silently fails, leaving stale merge state that blocks subsequent merges. + Suggested fix: Use the existing `resolveGitDir(originalBase || this.s.basePath)` helper (available in `worktree-manager.ts:106`) to resolve the actual git directory before attempting cleanup. + +- `worktree-resolver.ts:228` [basePath-mutation-race] **Status: CONFIRMED** + `emitWorktreeCreated` is called after `this.s.basePath` is mutated to the new worktree path. When `originalBasePath` is unset, the emit receives the new worktree path as `projectRoot`, breaking telemetry that requires a stable project root. + Suggested fix: Save the original project root into a local `const` before mutating `basePath` at line 206, then always pass the saved value to emit calls. + +### MEDIUM + +- `worktree-manager.ts:572-574` [cwd-resolution-race] **Status: CONFIRMED** + Directory-existence check and `chdir` are not atomic. A concurrent process could enter the worktree between the two calls, causing `git worktree remove` to fail. Damage is limited (caller retries), but the TOCTOU window is undocumented. + Suggested fix: Add a comment noting the race window; wrap in try-catch with a targeted retry. + +- `worktree-command.ts:757-762` [state-corruption-on-crash] **Status: CONFIRMED** + `originalCwd` is cleared at line 761 before merge completion. A crash or hang between the chdir and merge completion leaves the session unable to detect it was inside a worktree on restart. + Suggested fix: Move the `originalCwd = null` clear into the success path (after line 792); add a recovery step in `registerWorktreeCommand` to detect orphaned worktrees on reload. + +- `worktree-health.ts:76` [symlink-follow] **Status: CONFIRMED** + `existsSync(wt.path)` does not distinguish a deleted symlink target from an absent directory. A manually deleted symlink target causes the health check to report `not exists` even though git still records the worktree. + Suggested fix: Explicitly check `lstatSync` to detect symlinks; return a `{ exists: false }` health marker when the target is missing. + +- `worktree-resolver.ts:616` [path-separator] **Status: CONFIRMED** + `join()` on Windows paths with backslashes mixed with forward-slash comparisons elsewhere. Low risk due to Node.js normalisation, but adds cognitive load and latent risk on non-Linux targets. + Suggested fix: Use `path.resolve` consistently and normalise separators to forward-slash for all comparisons. + +### LOW + +- `worktree-command.ts:72-76` [path-extraction-fragile] **Status: CONFIRMED** + Worktree name extracted via sequential `split("/")[0] ?? split("\\")[0]`. Trailing backslashes can yield an empty array. + Suggested fix: Normalise path separators with `path.normalize` before splitting, or use `path.basename`. + +- `gitignore.ts:43` [manual-sync drift] **Status: CONFIRMED** + `SF_RUNTIME_PATTERNS` is not imported in `worktree-manager.ts`; the `SKIP_PATHS` array is manually maintained and can drift. The comment says "must stay synchronized" but there is no automated enforcement. + Suggested fix: Export `SKIP_PATHS` from `gitignore.ts` and import it in `worktree-manager.ts`, or add a runtime assertion. + +- `worktree-resolver.ts:252,373,597` [stale path reference] **Status: CONFIRMED** + All three emit calls use `this.s.originalBasePath || this.s.basePath`, which may reference a stale value after `restoreToProjectRoot()` resets `basePath`. + Suggested fix: Capture `const projectRoot = this.s.originalBasePath ?? this.s.basePath` at function entry and use it consistently. + +--- + +## Cluster: memory + state + cache + +### HIGH + +- `json-persistence.ts:58-69` [atomicity] **Status: CONFIRMED** + `saveJsonFile()` does not `fsync` after `renameSync()`. A system crash immediately after the rename can lose the atomic-write guarantee despite the tmp-file pattern. + Suggested fix: Call `fs.fsyncSync(fd)` on the target file (and optionally the parent directory) before returning. + +- `atomic-write.ts:56` [deadlock risk] **Status: CONFIRMED** + `sleepSync()` uses `Atomics.wait()` on a `SharedArrayBuffer`, which blocks the event loop. In a single-threaded Node context this can freeze the process or deadlock if called during async I/O. + Suggested fix: Use `setTimeout`-based sleep or document explicitly that this function must only be called from Worker threads. + +- `json-persistence.ts:58,75` [race condition] **Status: CONFIRMED** + Both `saveJsonFile()` and `writeJsonFileAtomic()` use randomised tmp names but do not clean up orphaned `.tmp.*` files from prior crashed writes before starting a new write. On Windows a locked stale tmp file can cause `renameSync` to fail. + Suggested fix: Glob and delete `*.tmp.*` files for the target path at the start of each write operation. + +### MEDIUM + +- `memory-extractor.ts:93-94` [resource leak] **Status: CONFIRMED** + The API key is resolved once in the outer closure and cached. If the key expires or auth revokes mid-extraction, all subsequent calls silently use stale credentials. + Suggested fix: Resolve `apiKey` inside the async function body on each invocation, not in the closure. + +- `cache.ts:24-29` [incomplete invalidation] **Status: CONFIRMED** + `invalidateAllCaches()` calls four functions sequentially without error isolation. If one throws, the remainder are skipped, leaving a partially-stale cache state. + Suggested fix: Wrap each call in an independent try-catch; log failures and continue clearing the remaining caches. + +- `memory-store.ts:203` [stale data fallback] **Status: CONFIRMED** + When `rewriteMemoryId()` fails to find the placeholder row, it returns the placeholder ID instead of `null`. Downstream code then treats `_TMP_...` as a real memory ID, causing silent state divergence. + Suggested fix: Return `null` explicitly on SELECT failure; let the caller decide whether to retry or raise. + +- `atomic-write.ts:122-137` [error swallowing] **Status: CONFIRMED** + The rename-retry loop overwrites `lastError` on each iteration. The final error may differ from earlier ones, making retry diagnostics misleading. + Suggested fix: Accumulate errors in an array and include all of them in the thrown error message. + +### LOW + +- `context-injector.ts:82-89` [truncation signal] **Status: CONFIRMED** + Truncated artifacts get a plain `\n...[truncated]` suffix. If the artifact is JSON, the suffix breaks its structure for downstream parsers. + Suggested fix: Document the truncation strategy; or wrap JSON artifacts in a container object before truncation. + +- `definition-io.ts:14-18` [no error context] **Status: CONFIRMED** + `readFrozenDefinition()` propagates raw YAML parse errors without identifying the offending file path. + Suggested fix: Wrap in try-catch and prepend `defPath` to the error message. + +- `memory-sleeper.ts:38-42` [memory leak] **Status: CONFIRMED** + `seenKeys` Set accumulates entries across units indefinitely. Identical tool-failure keys from different units are silently deduplicated even when the context has changed. + Suggested fix: Reset `seenKeys` at unit boundaries or per invocation. + +--- + +## Cluster: bootstrap + workflow + +### HIGH + +- `workflow-reconcile.ts:544` [async/concurrency] **Status: CONFIRMED** + Reconciliation reads `mainLogPath` at step 2 and re-checks before writing, but the advisory lock (line 469) is not held across the entire read-check-write window. Concurrent reconcilers can interleave log entries and cause divergence. + Suggested fix: Acquire the lock before the initial read at step 1, not just before the write. + +- `workflow-events.ts:81` [silent failure] **Status: CONFIRMED** + `readEvents()` silently skips corrupted JSONL lines with only `logWarning()`. Systematic corruption (e.g., from a crash mid-write) makes a milestone's event log silently incomplete. + Suggested fix: Track corruption count; fail (or prompt recovery from manifest backup) when the ratio exceeds a threshold. + +- `workflow-reconcile.ts:164-175` [data loss] **Status: CONFIRMED** + `replaySliceComplete()` validates task closure but not the milestone guard. A slice can be marked done even with incomplete tasks; on subsequent reconciliation error the incomplete work is orphaned. + Suggested fix: Validate milestone-level constraints before accepting a `complete_slice` event. + +### MEDIUM + +- `workflow-logger.ts:305-328` [audit divergence] **Status: CONFIRMED** + If `emitUokAuditEvent` fails, the error is caught and written to stderr, but the log entry is still persisted at line 346. The audit log and unified audit can permanently diverge with no reconciliation path. + Suggested fix: Track audit-emit failures and surface them in the next doctor run or workflow status report. + +- `write-gate.ts:277-279` [edge case] **Status: CONFIRMED** + `extractDepthVerificationMilestoneId()` regex matches only the specific pattern `depth_verification_M001`. Non-standard milestone ID formats silently return `null`, causing gate confirmation to fail unexpectedly. + Suggested fix: Normalise milestone IDs to lowercase before matching, or broaden the regex to accept common variants. + +- `workflow-manifest.ts:77-222` [type safety] **Status: CONFIRMED** + `snapshotState()` calls `JSON.parse()` on `depends_on` columns without schema validation. Malformed but parseable JSON (e.g., escaped-quote corruption) returns wrong structure that passes the type-coercion guard. + Suggested fix: Add a runtime schema validator (e.g., Zod or a manual array-of-strings check) after parse. + +- `notify-interceptor.ts:40-43` [idempotency] **Status: CONFIRMED** + When `appendNotification()` throws and is caught, the original notify still proceeds. Callers cannot distinguish a persisted notification from a failed one. + Suggested fix: Log the persistence failure with a correlation ID; surface it in the notification status API. + +- `system-context.ts:620-644` [async/concurrency] **Status: CONFIRMED** + `buildCarryForwardLines()` does not handle the case where a task file is deleted between directory scan and file load. A rejection in `Promise.all` fails the entire context injection with no fallback. + Suggested fix: Wrap per-file loads in individual try-catch blocks; skip missing files and log a warning. + +### LOW + +- `system-context.ts:774-776` [robustness] **Status: CONFIRMED** + Forensics-marker expiry fires silently after 2 hours. Users returning to a paused forensics session find the marker cleared with no log entry explaining why. + Suggested fix: Log an expiry event at `INFO` level when the timer fires. + +- `workflow-mcp.ts:456-471` [edge case] **Status: CONFIRMED** + Error message for local-transport MCP suggests `/sf mcp init`, which does not resolve the structured-questions limitation for local transports. + Suggested fix: Clarify in the error message that structured questions require remote transport, not just MCP initialisation. + +- `workflow-reconcile.ts:752-759` [idempotency] **Status: CONFIRMED** + After resolving the last conflict, `resolveConflict()` calls `reconcileWorktreeLogs()` a second time (line 758). If this second reconcile fails, the conflict is left in a partially-applied state with no rollback. + Suggested fix: Move the second reconcile into the explicit success path, or add rollback logic on failure. + +--- + +## Cluster: notification + detection + headless + +### HIGH + +- `src/headless.ts:1677-1685` [exit code mapping] **Status: CONFIRMED** + `EXIT_RELOAD` has no case in the `mapStatusToExitCode()` switch (headless-events.ts:33-48) and falls through to `EXIT_ERROR`, corrupting the reload-sentinel feature. Additionally, `emitBatchJsonResult()` sets `status: "timeout"` when `totalEvents === 0`, which silently misclassifies genuine timeouts that did emit events. + Suggested fix: Add an explicit `case EXIT_RELOAD:` in `mapStatusToExitCode()`; separate the timeout-detection logic from the event-count heuristic. + +- `src/resources/extensions/sf/notification-store.ts:109` [dedup off-by-one] **Status: CONFIRMED** + The deduplication window uses strict `<` instead of `<=`. A second notification delivered exactly at `DEDUP_WINDOW_MS` milliseconds is not deduplicated, allowing boundary duplicates. + Suggested fix: Change `< DEDUP_WINDOW_MS` to `<= DEDUP_WINDOW_MS`. + +- `src/resources/extensions/sf/detection.ts:302-320` [root-only markers silent over-detection] **Status: CONFIRMED** + `ROOT_ONLY_PROJECT_FILES` markers (e.g., `package.json`, `Cargo.toml`) are checked against the full recursive scan result rather than only root-level files. Nested workspace files are counted as top-level project markers, causing `verificationCommands` to emit bare `cargo check` / `npm test` from repo root, which fails when the only manifest is in a subdirectory. + Suggested fix: Filter `scannedFiles` to only entries at depth 0 (relative path contains no `/`) before matching against `ROOT_ONLY_PROJECT_FILES`. + +### MEDIUM + +- `src/headless.ts:1013-1023` [idle timeout arm condition] **Status: CONFIRMED** + `shouldArmHeadlessIdleTimeout()` re-arms immediately after a fast-returning interactive tool completes, potentially firing the timeout before the LLM reasons post-response. + Suggested fix: Introduce a short grace period (e.g., 500 ms) before re-arming after an interactive tool completes. + +- `src/headless.ts:1190` [milestone-ready detection in streaming] **Status: CONFIRMED** + `isMilestoneReadyText()` is evaluated against incremental text deltas. If the pattern spans two deltas it will not match either fragment, potentially missing a milestone-ready signal. + Suggested fix: Accumulate a rolling buffer of recent deltas (e.g., last 200 chars) and test the regex against the buffer. + +- `src/resources/extensions/sf/notification-store.ts:347-348` [stale lock detection race] **Status: CONFIRMED** + When the lock creator crashes after opening the fd but before writing the timestamp, `lockTime` parses as `NaN`. `Date.now() - NaN > 5000` evaluates to `false`, so the stale lock is never removed. + Suggested fix: Treat `NaN` as stale (i.e., remove the lock if `isNaN(lockTime)`). + +### LOW + +- `src/headless.ts:348-356` [autonomous → auto alias] **Status: CONFIRMED** + The `autonomous` command aliases to `auto` in CLI parsing, but `--auto` must be passed explicitly for auto-mode chaining to activate. Running `sf headless autonomous` without `--auto` does not chain, which contradicts the command name's intent. + Suggested fix: When the `autonomous` subcommand is parsed, implicitly set `options.auto = true`. + +- `src/headless.ts:1189` [auto-mode chaining visibility] **Status: CONFIRMED** + With verbose output enabled, log lines containing "milestone X ready" could spuriously match `isMilestoneReadyText()` and trigger premature auto-mode chaining. + Suggested fix: Restrict the detection to `message_delta` events of type `text` and exclude tool-output or log-prefixed content. + +--- + +## Recommended next actions + +Group by category, ordered by urgency. + +### 1. Durability / Data-loss (fix first) + +- `json-persistence.ts:58-69` — Missing `fsync` after atomic rename (memory+state+cache). A single line of code; fix immediately. +- `workflow-reconcile.ts:544` — Advisory lock not held across read-check-write in reconciler (bootstrap+workflow). Extend lock scope to cover the initial read. +- `workflow-events.ts:81` — Silent JSONL corruption in event logs (bootstrap+workflow). Add a corruption-rate threshold. + +### 2. Crash / State-corruption + +- `atomic-write.ts:56` — `Atomics.wait()` blocks event loop; deadlock risk (memory+state+cache). +- `worktree-resolver.ts:616` — Merge-state cleanup fails in worktrees; blocks subsequent merges (worktree+git). +- `worktree-command.ts:757-762` — `originalCwd` cleared before merge completes; session loses worktree context on crash (worktree+git). + +### 3. Exit-code / Signal correctness + +- `src/headless.ts:1677-1685` — `EXIT_RELOAD` falls through to `EXIT_ERROR` in switch (notification+detection+headless). High-visibility sentinel corruption; fix alongside the `totalEvents === 0` timeout heuristic. + +### 4. Detection correctness + +- `src/resources/extensions/sf/detection.ts:302-320` — Root-only markers checked against recursive scan, generating wrong `verificationCommands` (notification+detection+headless). +- `notification-store.ts:347-348` — `NaN` lock timestamp never treated as stale (notification+detection+headless). +- `notification-store.ts:109` — Off-by-one in dedup window (notification+detection+headless). + +### 5. Logic / Guard correctness + +- `dispatch-guard.ts:104-150` — Positional fallback skipped globally when any slice has dependencies (engine+verification). +- `workflow-reconcile.ts:164-175` — `replaySliceComplete()` skips milestone guard (bootstrap+workflow). +- `doctor-environment.ts:404` — lsof process-name extraction unreliable (scaffold+doctor). diff --git a/src/headless-events.ts b/src/headless-events.ts index caeee55ae..1c48dfee2 100644 --- a/src/headless-events.ts +++ b/src/headless-events.ts @@ -189,6 +189,10 @@ export function isBlockedNotification(event: Record): boolean { return message.includes("blocked:") || isPauseNotification(event); } +/** + * Detect milestone-ready (approval request) notifications. Indicates workflow + * reached a checkpoint and awaits user approval to continue. + */ export function isMilestoneReadyNotification( event: Record, ): boolean { @@ -204,16 +208,26 @@ export function isMilestoneReadyNotification( return isMilestoneReadyText(String(event.message ?? "")); } +/** + * Check if plain text matches milestone-ready pattern (e.g., "milestone m2 ready"). + */ export function isMilestoneReadyText(text: string): boolean { return /milestone\s+m\d+.*ready/i.test(text); } +/** + * Check if a tool requires user interaction and should block idle timeout. + */ export function isInteractiveHeadlessTool( toolName: string | undefined, ): boolean { return INTERACTIVE_HEADLESS_TOOLS.has(String(toolName ?? "")); } +/** + * Determine whether to arm the idle timeout for command completion detection. + * Returns false if interactive tools have been called. + */ export function shouldArmHeadlessIdleTimeout( toolCallCount: number, interactiveToolCount: number, @@ -225,6 +239,10 @@ export function shouldArmHeadlessIdleTimeout( // Quick Command Detection // --------------------------------------------------------------------------- +/** + * UI methods that don't require waiting for a response (fire-and-forget). + * Used to avoid blocking headless idle timeout. + */ export const FIRE_AND_FORGET_METHODS = new Set([ "notify", "setStatus", @@ -233,6 +251,10 @@ export const FIRE_AND_FORGET_METHODS = new Set([ "set_editor_text", ]); +/** + * Commands that complete in a single turn without interactive tool use. + * These use a shorter idle timeout since they don't involve extended reasoning. + */ export const QUICK_COMMANDS = new Set([ "status", "queue", diff --git a/src/resources/extensions/sf/agentic-docs-scaffold.ts b/src/resources/extensions/sf/agentic-docs-scaffold.ts index cdb079129..a3cc7ab73 100644 --- a/src/resources/extensions/sf/agentic-docs-scaffold.ts +++ b/src/resources/extensions/sf/agentic-docs-scaffold.ts @@ -9,6 +9,7 @@ import { type ScaffoldManifestEntry, } from "./scaffold-versioning.js"; import { migrateLegacyScaffold } from "./scaffold-drift.js"; +import { promoteActionableRecords } from "./record-promoter.js"; import { logWarning } from "./workflow-logger.js"; /** diff --git a/src/resources/extensions/sf/auto/loop.ts b/src/resources/extensions/sf/auto/loop.ts index b5d545b6c..eed7be2f7 100644 --- a/src/resources/extensions/sf/auto/loop.ts +++ b/src/resources/extensions/sf/auto/loop.ts @@ -69,6 +69,13 @@ function loadStuckState(basePath: string): { } { try { const data = JSON.parse(readFileSync(stuckStatePath(basePath), "utf-8")); + // Only load state written by a DIFFERENT process (real session restart). + // If the PID matches the current process, this state was written by an earlier + // autoLoop call in the same process (e.g., a test that completed before this + // one), not by a crashed session — skip it to prevent test state pollution. + if (data.pid === process.pid) { + return { recentUnits: [], stuckRecoveryAttempts: 0 }; + } return { recentUnits: Array.isArray(data.recentUnits) ? data.recentUnits : [], stuckRecoveryAttempts: @@ -92,6 +99,7 @@ function saveStuckState(basePath: string, state: LoopState): void { writeFileSync( filePath, JSON.stringify({ + pid: process.pid, recentUnits: state.recentUnits.slice(-20), // keep last 20 entries stuckRecoveryAttempts: state.stuckRecoveryAttempts, updatedAt: new Date().toISOString(), @@ -205,8 +213,7 @@ function checkMemoryPressure(): { // Try to get the actual V8 heap limit let limitMB = 4096; // conservative default try { - const v8 = require("node:v8"); - const stats = v8.getHeapStatistics(); + const stats = getHeapStatistics(); limitMB = Math.round(stats.heap_size_limit / 1024 / 1024); } catch { limitMB = 4096; /* v8 stats unavailable — use conservative default */ @@ -314,11 +321,11 @@ async function runUnitPhaseViaContract( async function enforceMinRequestInterval(s: AutoSession, prefs: IterationContext["prefs"]): Promise { const minInterval = prefs?.min_request_interval_ms ?? 0; if (minInterval > 0 && s.lastRequestTimestamp > 0) { - const elapsed = Date.now() - s.lastRequestTimestamp; + const elapsed = Math.max(0, Date.now() - s.lastRequestTimestamp); if (elapsed < minInterval) { const waitMs = minInterval - elapsed; debugLog("autoLoop", { phase: "rate-limit-wait", waitMs }); - await new Promise(r => setTimeout(r, waitMs)); + await new Promise((r) => setTimeout(r, waitMs)); } } } diff --git a/src/resources/extensions/sf/auto/phases.ts b/src/resources/extensions/sf/auto/phases.ts index 508af6b29..b6ad360ca 100644 --- a/src/resources/extensions/sf/auto/phases.ts +++ b/src/resources/extensions/sf/auto/phases.ts @@ -2557,22 +2557,53 @@ export async function runUnitPhase( ); } - deps.emitJournalEvent({ - ts: new Date().toISOString(), - flowId: ic.flowId, - seq: ic.nextSeq(), - eventType: "unit-end", - data: { - unitType, - unitId, - status: unitResult.status, - artifactVerified, - ...(unitResult.errorContext - ? { errorContext: unitResult.errorContext } - : {}), - }, - causedBy: { flowId: ic.flowId, seq: unitStartSeq }, - }); + { + // Pull cost/token data from the ledger entry that snapshotUnitMetrics + // already wrote so the unit-end event carries billing context. + const unitEndLedger = deps.getLedger() as { + units?: Array<{ + type: string; + id: string; + startedAt: number; + cost: number; + tokens: { input: number; output: number; total: number }; + }>; + } | null; + const unitEndEntry = unitEndLedger?.units + ? [...unitEndLedger.units] + .reverse() + .find( + (u) => + u.type === unitType && + u.id === unitId && + u.startedAt === s.currentUnit?.startedAt, + ) + : undefined; + deps.emitJournalEvent({ + ts: new Date().toISOString(), + flowId: ic.flowId, + seq: ic.nextSeq(), + eventType: "unit-end", + data: { + unitType, + unitId, + status: unitResult.status, + artifactVerified, + ...(unitEndEntry + ? { + cost_usd: unitEndEntry.cost, + tokens: unitEndEntry.tokens.total, + tokens_input: unitEndEntry.tokens.input, + tokens_output: unitEndEntry.tokens.output, + } + : {}), + ...(unitResult.errorContext + ? { errorContext: unitResult.errorContext } + : {}), + }, + causedBy: { flowId: ic.flowId, seq: unitStartSeq }, + }); + } { const verdict = diff --git a/src/resources/extensions/sf/blocked-models.ts b/src/resources/extensions/sf/blocked-models.ts index 1590c9ea6..3a4acdeb6 100644 --- a/src/resources/extensions/sf/blocked-models.ts +++ b/src/resources/extensions/sf/blocked-models.ts @@ -66,6 +66,9 @@ export function isModelBlocked( ); } +/** + * Add a provider/model pair to the persistent blocklist (e.g., after account entitlement rejection). + */ export function blockModel( basePath: string, provider: string, diff --git a/src/resources/extensions/sf/bootstrap/system-context.ts b/src/resources/extensions/sf/bootstrap/system-context.ts index ba9910512..52fa1a6f0 100644 --- a/src/resources/extensions/sf/bootstrap/system-context.ts +++ b/src/resources/extensions/sf/bootstrap/system-context.ts @@ -648,7 +648,7 @@ async function buildCarryForwardLines( if (r.status === "fulfilled") return r.value; const file = summaryFiles[idx]!; logWarning( - "system-context", + "bootstrap", `Failed to load task summary ${sliceRel}/tasks/${file}: ${(r.reason as Error).message}`, ); return `- \`${sliceRel}/tasks/${file}\` (load failed)`; @@ -794,6 +794,11 @@ export function buildForensicsContextInjection( // Expire markers older than 2 hours to avoid stale context const age = Date.now() - new Date(marker.createdAt).getTime(); if (age > 2 * 60 * 60 * 1000) { + const hours = (age / (60 * 60 * 1000)).toFixed(1); + logWarning( + "bootstrap", + `Forensics marker expired (${hours}h old), clearing stale context.`, + ); clearForensicsMarker(basePath); return null; } diff --git a/src/resources/extensions/sf/cache.ts b/src/resources/extensions/sf/cache.ts index 93516d628..c7c71990e 100644 --- a/src/resources/extensions/sf/cache.ts +++ b/src/resources/extensions/sf/cache.ts @@ -13,6 +13,7 @@ import { clearParseCache } from "./files.js"; import { clearPathCache } from "./paths.js"; import { clearArtifacts } from "./sf-db.js"; import { invalidateStateCache } from "./state.js"; +import { logWarning } from "./workflow-logger.js"; /** * Invalidate all SF runtime caches in one call. @@ -20,10 +21,32 @@ import { invalidateStateCache } from "./state.js"; * Call this after file writes, milestone transitions, merge reconciliation, * or any operation that changes .sf/ contents on disk. Forgetting to clear * any single cache causes stale reads (see #431, #793). + * + * Each cache clear is attempted independently; failures are logged but do not + * prevent other caches from being cleared. */ export function invalidateAllCaches(): void { - invalidateStateCache(); - clearPathCache(); - clearParseCache(); - clearArtifacts(); + try { + invalidateStateCache(); + } catch (err) { + logWarning(`Cache invalidation failed for state: ${err}`); + } + + try { + clearPathCache(); + } catch (err) { + logWarning(`Cache invalidation failed for paths: ${err}`); + } + + try { + clearParseCache(); + } catch (err) { + logWarning(`Cache invalidation failed for parse: ${err}`); + } + + try { + clearArtifacts(); + } catch (err) { + logWarning(`Cache invalidation failed for artifacts: ${err}`); + } } diff --git a/src/resources/extensions/sf/commands/catalog.ts b/src/resources/extensions/sf/commands/catalog.ts index 228b4a97a..81032ff75 100644 --- a/src/resources/extensions/sf/commands/catalog.ts +++ b/src/resources/extensions/sf/commands/catalog.ts @@ -10,16 +10,28 @@ import { resolveProjectRoot } from "../worktree.js"; const sfHome = process.env.SF_HOME || join(homedir(), ".sf"); +/** + * SF subcommand definition with command name and description. + */ export interface SfCommandDefinition { cmd: string; desc: string; } +/** + * Mapping of command names to nested subcommand definitions. + */ type CompletionMap = Record; +/** + * Comprehensive description of all available SF commands for help text. + */ export const SF_COMMAND_DESCRIPTION = "SF — Singularity Forge: /sf help|start|templates|next|autonomous|auto|stop|pause|status|widget|visualize|queue|quick|discuss|capture|triage|todo|dispatch|history|undo|undo-task|reset-slice|rate|skip|export|cleanup|model|mode|prefs|config|keys|hooks|run-hook|skill-health|doctor|logs|forensics|changelog|migrate|remote|steer|knowledge|harness|new-milestone|parallel|cmux|park|unpark|init|setup|inspect|extensions|update|fast|mcp|rethink|codebase|notifications|ship|do|session-report|backlog|pr-branch|add-tests|scan|scaffold"; +/** + * Top-level SF subcommands with descriptions. + */ export const TOP_LEVEL_SUBCOMMANDS: readonly SfCommandDefinition[] = [ { cmd: "help", desc: "Categorized command reference with descriptions" }, { cmd: "next", desc: "Explicit step mode (same as /sf)" }, @@ -161,6 +173,9 @@ export const TOP_LEVEL_SUBCOMMANDS: readonly SfCommandDefinition[] = [ }, ]; +/** + * Nested subcommand definitions for multi-level completion. + */ const NESTED_COMPLETIONS: CompletionMap = { autonomous: [ { cmd: "full", desc: "Auto-merge milestones; chain end-to-end without review" }, @@ -409,6 +424,9 @@ const NESTED_COMPLETIONS: CompletionMap = { ], }; +/** + * Filter and format completion options by prefix. + */ function filterOptions( partial: string, options: readonly SfCommandDefinition[], diff --git a/src/resources/extensions/sf/definition-io.ts b/src/resources/extensions/sf/definition-io.ts index 42d6395a4..5afc08ea4 100644 --- a/src/resources/extensions/sf/definition-io.ts +++ b/src/resources/extensions/sf/definition-io.ts @@ -13,6 +13,13 @@ import type { WorkflowDefinition } from "./definition-loader.js"; /** Read and parse the frozen DEFINITION.yaml from a run directory. */ export function readFrozenDefinition(runDir: string): WorkflowDefinition { const defPath = join(runDir, "DEFINITION.yaml"); - const raw = readFileSync(defPath, "utf-8"); - return parse(raw, { schema: "core" }) as WorkflowDefinition; + try { + const raw = readFileSync(defPath, "utf-8"); + return parse(raw, { schema: "core" }) as WorkflowDefinition; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to read/parse DEFINITION.yaml at ${defPath}: ${message}`, { + cause: err, + }); + } } diff --git a/src/resources/extensions/sf/json-persistence.ts b/src/resources/extensions/sf/json-persistence.ts index 38d46d50e..2a6f0a84a 100644 --- a/src/resources/extensions/sf/json-persistence.ts +++ b/src/resources/extensions/sf/json-persistence.ts @@ -120,6 +120,7 @@ export function writeJsonFileAtomic(filePath: string, data: T): void { try { const dir = dirname(filePath); mkdirSync(dir, { recursive: true }); + cleanOrphanTmpFiles(filePath); const tmp = `${filePath}.tmp.${randomBytes(4).toString("hex")}`; writeFileSync(tmp, JSON.stringify(data, null, 2), "utf-8"); // fsync the tmp file so its data is durable before the rename is visible diff --git a/src/resources/extensions/sf/memory-relations.ts b/src/resources/extensions/sf/memory-relations.ts index 4fe693dc4..dbc02b4c2 100644 --- a/src/resources/extensions/sf/memory-relations.ts +++ b/src/resources/extensions/sf/memory-relations.ts @@ -46,6 +46,9 @@ export interface MemoryGraph { // ─── Helpers ──────────────────────────────────────────────────────────────── +/** + * Type guard: check if a value is a valid RelationType. + */ export function isValidRelation(value: unknown): value is RelationType { return ( typeof value === "string" && @@ -62,6 +65,10 @@ function clampConfidence(value: unknown): number { // ─── Mutations ────────────────────────────────────────────────────────────── +/** + * Create a knowledge-graph edge between two memories with a relation type. + * Returns true if successful, false if memories don't exist or DB unavailable. + */ export function createMemoryRelation( from: string, to: string, @@ -100,6 +107,9 @@ export function createMemoryRelation( } } +/** + * Remove all edges (from or to) for a given memory ID. + */ export function removeMemoryRelationsFor(memoryId: string): void { if (!isDbAvailable() || !memoryId) return; const adapter = _getAdapter(); @@ -117,6 +127,9 @@ export function removeMemoryRelationsFor(memoryId: string): void { // ─── Queries ──────────────────────────────────────────────────────────────── +/** + * List all relations (incoming or outgoing) for a given memory ID. + */ export function listRelationsFor(memoryId: string): MemoryRelation[] { if (!isDbAvailable()) return []; const adapter = _getAdapter(); @@ -133,6 +146,10 @@ export function listRelationsFor(memoryId: string): MemoryRelation[] { } } +/** + * BFS traversal of the memory knowledge graph starting from a memory ID. + * Returns nodes and edges up to the specified depth (clamped to max 5). + */ export function traverseGraph(startId: string, depth: number): MemoryGraph { const emptyResult: MemoryGraph = { nodes: [], edges: [] }; if (!isDbAvailable() || !startId) return emptyResult; diff --git a/src/resources/extensions/sf/memory-store.ts b/src/resources/extensions/sf/memory-store.ts index 79149f2b5..865e37477 100644 --- a/src/resources/extensions/sf/memory-store.ts +++ b/src/resources/extensions/sf/memory-store.ts @@ -200,7 +200,7 @@ export function createMemory(fields: { const row = adapter .prepare("SELECT seq FROM memories WHERE id = :id") .get({ ":id": placeholder }); - if (!row) return placeholder; // fallback — should not happen + if (!row) return null; // Should not happen; fail gracefully const seq = row["seq"] as number; const realId = `MEM${String(seq).padStart(3, "0")}`; rewriteMemoryId(placeholder, realId); diff --git a/src/resources/extensions/sf/onboarding-state.ts b/src/resources/extensions/sf/onboarding-state.ts index 882c08bd2..82fbf8e60 100644 --- a/src/resources/extensions/sf/onboarding-state.ts +++ b/src/resources/extensions/sf/onboarding-state.ts @@ -21,6 +21,7 @@ import { logWarning } from "./workflow-logger.js"; * Records with an older flowVersion are treated as "needs partial re-onboarding" * by isOnboardingComplete(). */ +/** Current onboarding flow version. Bump when adding required onboarding steps. */ export const FLOW_VERSION = 1; const RECORD_VERSION = 1; @@ -32,6 +33,9 @@ const AGENT_DIR = join(process.env.SF_HOME || join(homedir(), ".sf"), "agent"); const FILE = join(AGENT_DIR, "onboarding.json"); +/** + * Record of onboarding progress persisted to ~/.sf/agent/onboarding.json. + */ export interface OnboardingRecord { version: number; flowVersion: number; @@ -50,6 +54,9 @@ const DEFAULT: OnboardingRecord = { lastResumePoint: null, }; +/** + * Read the onboarding completion record. Returns defaults if file doesn't exist. + */ export function readOnboardingRecord(): OnboardingRecord { if (!existsSync(FILE)) return { ...DEFAULT }; try { @@ -90,6 +97,9 @@ function atomicWrite(record: OnboardingRecord): void { } } +/** + * Write or update the onboarding completion record with a partial update. + */ export function writeOnboardingRecord( patch: Partial, ): OnboardingRecord { @@ -121,6 +131,9 @@ export function writeOnboardingRecord( * Onboarding is "complete" when there's a completedAt timestamp AND the * flowVersion matches the current FLOW_VERSION. */ +/** + * Check if onboarding is complete at the current flow version. + */ export function isOnboardingComplete(): boolean { const r = readOnboardingRecord(); return r.completedAt !== null && r.flowVersion === FLOW_VERSION; diff --git a/src/resources/extensions/sf/post-execution-checks.ts b/src/resources/extensions/sf/post-execution-checks.ts index 8dfbee827..5dc9f30ad 100644 --- a/src/resources/extensions/sf/post-execution-checks.ts +++ b/src/resources/extensions/sf/post-execution-checks.ts @@ -32,6 +32,9 @@ export interface PostExecutionCheckJSON { blocking?: boolean; } +/** + * Result of post-execution checks: overall status, individual check results, and duration. + */ export interface PostExecutionResult { /** Overall result: pass if no blocking failures, warn if non-blocking issues, fail if blocking issues */ status: "pass" | "warn" | "fail"; @@ -120,8 +123,7 @@ export function extractRelativeImports( /** * Check if a relative import resolves to an existing file. - * Handles .ts, .tsx, .js, .jsx extensions and index files. - * Also handles TypeScript ESM convention where imports use .js but resolve to .ts. + * Handles extensions and TypeScript ESM convention (.js imports resolve to .ts). */ export function resolveImportPath( importPath: string, @@ -175,9 +177,8 @@ export function resolveImportPath( } /** - * Check that all relative imports in the task's key_files resolve to existing files. - * Reads modified files from task.key_files, extracts import statements via regex, - * verifies relative imports resolve to existing files. + * Check that all relative imports in key_files resolve to existing files. + * Returns blocking failures for unresolvable imports. */ export function checkImportResolution( taskRow: TaskRow, @@ -298,9 +299,8 @@ function normalizeType(type: string): string { } /** - * Compare function signatures in current task's output against prior tasks' key_files - * to catch hallucination cascades — when a task references functions that don't exist - * or have different signatures than what was actually created. + * Detect hallucination cascades: functions in current task with mismatched signatures. + * Returns non-blocking warnings for parameter/return type drift. */ export function checkCrossTaskSignatures( taskRow: TaskRow, @@ -390,9 +390,8 @@ export function checkCrossTaskSignatures( // ─── Pattern Consistency Check ─────────────────────────────────────────────── /** - * Detect async style drift (mixing async/await with .then()) and - * naming convention inconsistencies within a task's key_files. - * Warn only — these are style issues, not correctness issues. + * Detect async style drift and naming convention inconsistencies. + * Warns only (non-blocking) since these are style issues, not correctness. */ export function checkPatternConsistency( taskRow: TaskRow, @@ -448,7 +447,7 @@ function checkAsyncStyleDrift( return { category: "pattern", target: fileName, - passed: true, // Warning only + passed: false, message: `File ${fileName} mixes async/await with .then() promise chaining — consider using consistent async style`, blocking: false, }; @@ -490,7 +489,7 @@ function checkNamingConsistency( results.push({ category: "pattern", target: fileName, - passed: true, // Warning only + passed: false, message: `File ${fileName} mixes camelCase (${camelCaseFuncs.slice(0, 2).join(", ")}) and snake_case (${snakeCaseFuncs.slice(0, 2).join(", ")}) function names`, blocking: false, }); diff --git a/src/resources/extensions/sf/preferences-models.ts b/src/resources/extensions/sf/preferences-models.ts index 00d005b1a..837bdde24 100644 --- a/src/resources/extensions/sf/preferences-models.ts +++ b/src/resources/extensions/sf/preferences-models.ts @@ -634,6 +634,67 @@ export function updatePreferencesModels(models: SFModelConfigV2): void { writeFileSync(prefsPath, content, "utf-8"); } +/** + * Increment the subscription token counter in the global preferences file. + * + * When a unit completes on a provider that matches `subscription.provider`, + * this function atomically updates `subscription.tokens_used_this_month` on + * disk. It is best-effort: any error is silently swallowed so that a + * preference-write failure never disrupts auto-mode. + * + * Pass `provider` (e.g. "anthropic") and `tokensConsumed` from the unit + * ledger entry. The function is a no-op when: + * - No subscription is configured in effective preferences + * - `subscription.provider` does not match the given provider (case-insensitive) + * - `tokensConsumed` is 0 or negative + */ +export function updateSubscriptionTokensUsed( + provider: string, + tokensConsumed: number, +): void { + if (!provider || tokensConsumed <= 0) return; + try { + const prefs = loadEffectiveSFPreferences(); + const sub = prefs?.preferences.subscription; + if (!sub?.provider) return; + if (sub.provider.toLowerCase() !== provider.toLowerCase()) return; + + const prefsPath = getGlobalSFPreferencesPath(); + if (!existsSync(prefsPath)) return; + + const content = readFileSync(prefsPath, "utf-8"); + const current = sub.tokens_used_this_month ?? 0; + const updated = current + tokensConsumed; + + // Replace an existing tokens_used_this_month line or insert after + // the provider line inside the subscription block. + const tokensLineRe = /^(\s*tokens_used_this_month\s*:)\s*\d+/m; + let newContent: string; + if (tokensLineRe.test(content)) { + newContent = content.replace( + tokensLineRe, + `$1 ${updated}`, + ); + } else { + // Insert after the subscription.provider line + const providerLineRe = + /^(\s*provider\s*:.+)$/m; + if (providerLineRe.test(content)) { + newContent = content.replace( + providerLineRe, + `$1\n tokens_used_this_month: ${updated}`, + ); + } else { + // Can't safely locate insertion point — skip + return; + } + } + writeFileSync(prefsPath, newContent, "utf-8"); + } catch { + // Best-effort — never interrupt auto-mode for a pref write failure + } +} + /** * Resolve the dynamic routing configuration from effective preferences. * Returns the merged config with defaults applied. diff --git a/src/resources/extensions/sf/preferences-skills.ts b/src/resources/extensions/sf/preferences-skills.ts index b766f3a5c..4e698217d 100644 --- a/src/resources/extensions/sf/preferences-skills.ts +++ b/src/resources/extensions/sf/preferences-skills.ts @@ -25,11 +25,12 @@ export type { } from "./preferences-types.js"; /** - * Known skill directories, in priority order. - * Searches both the skills.sh ecosystem directory (~/.agents/skills/) and - * Claude Code's official directory (~/.claude/skills/). Project-level - * directories for both conventions are included as well. - * Legacy ~/.sf/agent/skills/ is included as a fallback for pre-migration installs. + * Get skill search directories in priority order for resolution. + * + * Searches user and project skill directories (skills.sh and Claude Code), + * with legacy pre-migration installs as fallback. + * + * @param cwd - Current working directory for project-scoped search. */ export function getSkillSearchDirs( cwd: string, @@ -53,12 +54,13 @@ export function getSkillSearchDirs( } /** - * Resolve a single skill reference to an absolute path. + * Resolve a skill reference string to its absolute file path. * - * Resolution order: - * 1. Absolute path to a file -> check existsSync - * 2. Absolute path to a directory -> check for SKILL.md inside - * 3. Bare name -> scan known skill directories for /SKILL.md + * Tries absolute path (file/dir), then searches known directories for bare name. + * Returns null path and "unresolved" method if not found. + * + * @param ref - Skill reference (absolute path, tilde path, or bare name). + * @param cwd - Current working directory for project-relative search. */ export function resolveSkillReference( ref: string, diff --git a/src/resources/extensions/sf/preferences.ts b/src/resources/extensions/sf/preferences.ts index 99dd43f5a..6d3f8089b 100644 --- a/src/resources/extensions/sf/preferences.ts +++ b/src/resources/extensions/sf/preferences.ts @@ -660,6 +660,11 @@ function mergePreferences( base.safety_harness || override.safety_harness ? { ...(base.safety_harness ?? {}), ...(override.safety_harness ?? {}) } : undefined, + // subscription: project-level wins over global (full replace, not merge), + // so that a project can declare its own subscription context independently. + subscription: override.subscription ?? base.subscription, + allow_flat_rate_providers: + override.allow_flat_rate_providers ?? base.allow_flat_rate_providers, }; } diff --git a/src/resources/extensions/sf/record-promoter.ts b/src/resources/extensions/sf/record-promoter.ts new file mode 100644 index 000000000..030803d5d --- /dev/null +++ b/src/resources/extensions/sf/record-promoter.ts @@ -0,0 +1,340 @@ +// SF Extension — Record Promoter +// Scans docs/records/*.md for frontmatter-tagged actionable records and +// promotes each one to a .sf/milestones/M-/ structure. +// Called from ensureAgenticDocsScaffold (ADR-021 Phase C hook) and from +// the post-milestone-completion path in auto-post-unit.ts / auto.ts:stopAuto. +// +// Silent contract: no stdout/stderr in the no-op path. Failure is non-fatal; +// callers must wrap in try/catch. logWarning("scaffold", ...) only on errors. + +import { + existsSync, + mkdirSync, + readdirSync, + readFileSync, + writeFileSync, +} from "node:fs"; +import { basename, join } from "node:path"; + +import { logWarning } from "./workflow-logger.js"; + +// ─── Types ─────────────────────────────────────────────────────────────────── + +export interface RecordFrontmatter { + actionable?: boolean; + kind?: string; + promoted?: boolean; + promoted_to?: string; + date?: string; +} + +export interface PromotionResult { + promoted: Array<{ recordPath: string; milestoneId: string }>; + skipped: Array<{ recordPath: string; reason: string }>; +} + +// ─── Frontmatter Parser ────────────────────────────────────────────────────── + +/** + * Parse the YAML frontmatter from a markdown file. + * Returns null if the file has no frontmatter block (--- delimiters). + * Handles simple scalar YAML only — no nested objects or multi-line values. + */ +export function parseRecordFrontmatter( + filePath: string, +): RecordFrontmatter | null { + let content: string; + try { + content = readFileSync(filePath, "utf-8"); + } catch { + return null; + } + + // Frontmatter must start at the very beginning of the file + if (!content.startsWith("---")) return null; + + const end = content.indexOf("\n---", 3); + if (end === -1) return null; + + const block = content.slice(3, end).trim(); + const result: RecordFrontmatter = {}; + + for (const line of block.split("\n")) { + const colon = line.indexOf(":"); + if (colon === -1) continue; + const key = line.slice(0, colon).trim(); + const raw = line.slice(colon + 1).trim(); + + // Strip inline comments and optional quotes + const val = raw.replace(/#.*$/, "").trim().replace(/^["']|["']$/g, ""); + + if (key === "actionable") { + result.actionable = val === "true"; + } else if (key === "promoted") { + result.promoted = val === "true"; + } else if (key === "kind") { + result.kind = val || undefined; + } else if (key === "promoted_to") { + result.promoted_to = val || undefined; + } else if (key === "date") { + result.date = val || undefined; + } + } + + return result; +} + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +/** Convert a filename or title string to a URL-safe slug. */ +function toSlug(s: string): string { + return s + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 40); +} + +/** + * Derive the next milestone ID by scanning existing .sf/milestones/M* dirs. + * Returns "M001" if no milestones exist yet. + */ +function nextMilestoneId(milestonesPath: string): string { + let max = 0; + if (existsSync(milestonesPath)) { + try { + for (const entry of readdirSync(milestonesPath, { withFileTypes: true })) { + if (!entry.isDirectory()) continue; + const match = entry.name.match(/^M(\d{3})/); + if (match) { + const n = parseInt(match[1], 10); + if (n > max) max = n; + } + } + } catch { + /* non-fatal */ + } + } + return `M${String(max + 1).padStart(3, "0")}`; +} + +/** + * Extract H2 headings from markdown body content (after stripping frontmatter). + * Returns the heading text for each `## Heading` line found. + */ +function extractH2Headings(body: string): string[] { + const headings: string[] = []; + for (const line of body.split("\n")) { + const m = line.match(/^##\s+(.+)$/); + if (m) headings.push(m[1].trim()); + } + return headings; +} + +/** Strip YAML frontmatter block from file content. */ +function stripFrontmatter(content: string): string { + if (!content.startsWith("---")) return content; + const end = content.indexOf("\n---", 3); + if (end === -1) return content; + return content.slice(end + 4).trimStart(); +} + +/** + * Derive a milestone title from a record filename. + * "2026-05-02-bug-hunt-findings.md" → "Bug Hunt Findings" + */ +function titleFromFilename(filename: string): string { + return basename(filename, ".md") + .replace(/^\d{4}-\d{2}-\d{2}-/, "") + .split("-") + .map((w) => w.charAt(0).toUpperCase() + w.slice(1)) + .join(" "); +} + +/** Update the frontmatter of a record file to mark it as promoted. */ +function stampRecordPromoted( + filePath: string, + milestoneId: string, +): void { + const content = readFileSync(filePath, "utf-8"); + + if (!content.startsWith("---")) { + // No frontmatter at all — prepend minimal block + const newFm = `---\npromoted: true\npromoted_to: ${milestoneId}\n---\n\n`; + writeFileSync(filePath, newFm + content, "utf-8"); + return; + } + + const end = content.indexOf("\n---", 3); + if (end === -1) return; // malformed — leave alone + + const fmBlock = content.slice(3, end); + let newBlock = fmBlock; + + // Upsert promoted and promoted_to keys + if (/^promoted:/m.test(newBlock)) { + newBlock = newBlock.replace(/^promoted:.*$/m, "promoted: true"); + } else { + newBlock += "\npromoted: true"; + } + + if (/^promoted_to:/m.test(newBlock)) { + newBlock = newBlock.replace( + /^promoted_to:.*$/m, + `promoted_to: ${milestoneId}`, + ); + } else { + newBlock += `\npromoted_to: ${milestoneId}`; + } + + const after = content.slice(end); // starts with "\n---" + writeFileSync(filePath, `---${newBlock}${after}`, "utf-8"); +} + +/** Append a milestone ID to .sf/QUEUE.md, creating the file if absent. */ +function appendToQueue(sfRootPath: string, milestoneId: string): void { + const queuePath = join(sfRootPath, "QUEUE.md"); + if (existsSync(queuePath)) { + const existing = readFileSync(queuePath, "utf-8"); + // Don't add duplicates + if (existing.split("\n").some((l) => l.trim() === milestoneId)) return; + const sep = existing.endsWith("\n") ? "" : "\n"; + writeFileSync(queuePath, existing + sep + milestoneId + "\n", "utf-8"); + } else { + writeFileSync(queuePath, milestoneId + "\n", "utf-8"); + } +} + +// ─── Core promotion logic ───────────────────────────────────────────────────── + +/** + * Detect actionable records in docs/records/ and promote each to a milestone. + * Runs synchronously; silent on the no-op path. Any error is non-fatal. + */ +export function promoteActionableRecords(basePath: string): PromotionResult { + const result: PromotionResult = { promoted: [], skipped: [] }; + + const recordsDir = join(basePath, "docs", "records"); + if (!existsSync(recordsDir)) return result; + + let entries: string[]; + try { + entries = readdirSync(recordsDir).filter((f) => f.endsWith(".md")); + } catch (err) { + logWarning("scaffold", "record-promoter: failed to read records dir", { + error: (err as Error).message, + }); + return result; + } + + // Resolve the .sf root for this project + const sfRootPath = join(basePath, ".sf"); + const milestonesPath = join(sfRootPath, "milestones"); + + for (const filename of entries) { + const recordPath = join(recordsDir, filename); + + let fm: RecordFrontmatter | null; + try { + fm = parseRecordFrontmatter(recordPath); + } catch (err) { + result.skipped.push({ recordPath, reason: `parse error: ${(err as Error).message}` }); + continue; + } + + if (!fm || !fm.actionable) { + result.skipped.push({ recordPath, reason: "not actionable" }); + continue; + } + + if (fm.promoted) { + result.skipped.push({ recordPath, reason: "already promoted" }); + continue; + } + + // --- Generate milestone --- + try { + const milestoneId = nextMilestoneId(milestonesPath); + const title = titleFromFilename(filename); + const slugBase = toSlug( + basename(filename, ".md").replace(/^\d{4}-\d{2}-\d{2}-/, ""), + ); + const milestoneDir = join(milestonesPath, milestoneId); + + // Read record body + const rawContent = readFileSync(recordPath, "utf-8"); + const body = stripFrontmatter(rawContent); + + // Derive slices from H2 headings + const headings = extractH2Headings(body); + + // Build slice table rows + const sliceRows = headings.map((heading, i) => { + const sId = `S${String(i + 1).padStart(2, "0")}`; + const sSlug = toSlug(heading); + return { id: sId, heading, slug: sSlug }; + }); + + // Write milestone directory structure + mkdirSync(milestoneDir, { recursive: true }); + + // M-CONTEXT.md — record content as context + writeFileSync( + join(milestoneDir, `${milestoneId}-CONTEXT.md`), + `# ${milestoneId}: ${title} — Context\n\n**Source record:** docs/records/${filename}\n\n---\n\n${body}`, + "utf-8", + ); + + // M-ROADMAP.md — slices derived from H2 headings + const sliceTable = sliceRows.length > 0 + ? [ + "| ID | Slice | Risk | Depends | Done | After this |", + "|----|-------|------|---------|------|------------|", + ...sliceRows.map( + (s) => `| ${s.id} | ${s.heading} | medium | — | ⬜ | Fix issues in the ${s.heading.toLowerCase()} cluster. |`, + ), + ].join("\n") + : "_No slices derived — add H2 headings to the source record._"; + + writeFileSync( + join(milestoneDir, `${milestoneId}-ROADMAP.md`), + [ + `# ${milestoneId}: ${title}`, + "", + "## Vision", + `Address actionable findings from the ${title.toLowerCase()} record.`, + "", + "## Slice Overview", + sliceTable, + "", + ].join("\n"), + "utf-8", + ); + + // slices/S-/ directories + for (const slice of sliceRows) { + const sliceDir = join(milestoneDir, "slices", `${slice.id}-${slice.slug}`); + mkdirSync(sliceDir, { recursive: true }); + } + + // Append to QUEUE.md + appendToQueue(sfRootPath, milestoneId); + + // Stamp the record promoted + stampRecordPromoted(recordPath, milestoneId); + + result.promoted.push({ recordPath, milestoneId }); + } catch (err) { + logWarning("scaffold", "record-promoter: failed to promote record", { + record: filename, + error: (err as Error).message, + }); + result.skipped.push({ + recordPath, + reason: `promotion error: ${(err as Error).message}`, + }); + } + } + + return result; +} diff --git a/src/resources/extensions/sf/safety/git-checkpoint.ts b/src/resources/extensions/sf/safety/git-checkpoint.ts index ccf61de66..333a426d1 100644 --- a/src/resources/extensions/sf/safety/git-checkpoint.ts +++ b/src/resources/extensions/sf/safety/git-checkpoint.ts @@ -15,6 +15,24 @@ import { logWarning } from "../workflow-logger.js"; const CHECKPOINT_PREFIX = "refs/sf/checkpoints/"; +/** + * Sanitize a unitId for use as a git ref component. + * Git ref names prohibit: spaces, control chars, ~, ^, :, ?, *, [, \, .., + * @{, leading dot, trailing .lock, and path component starting with ".". + * We replace all disallowed characters with "-" and collapse runs. + */ +function sanitizeForRef(unitId: string): string { + return unitId + .replace(/\//g, "-") // path separators → dashes + .replace(/[^a-zA-Z0-9._-]/g, "-") // anything else disallowed → dash + .replace(/\.{2,}/g, "-") // ".." sequences → dash + .replace(/\.lock$/i, "-lock") // trailing .lock suffix + .replace(/^\./, "_") // leading dot + .replace(/-{2,}/g, "-") // collapse consecutive dashes + .replace(/^-|-$/g, "") // strip leading/trailing dash + || "unit"; // fallback if everything was stripped +} + // ─── Public API ───────────────────────────────────────────────────────────── /** @@ -34,8 +52,7 @@ export function createCheckpoint( if (!sha || sha.length < 7) return null; - // Sanitize unitId for use in ref path (replace / with -) - const safeUnitId = unitId.replace(/\//g, "-"); + const safeUnitId = sanitizeForRef(unitId); execFileSync( "git", diff --git a/src/resources/extensions/sf/session-model-override.ts b/src/resources/extensions/sf/session-model-override.ts index 7f023e170..109e3e074 100644 --- a/src/resources/extensions/sf/session-model-override.ts +++ b/src/resources/extensions/sf/session-model-override.ts @@ -1,3 +1,6 @@ +/** + * Model override for a specific session (set via /sf model pin in auto-mode). + */ export interface SessionModelOverride { provider: string; id: string; @@ -9,6 +12,9 @@ function normalizeSessionId(sessionId: string): string { return typeof sessionId === "string" ? sessionId.trim() : ""; } +/** + * Store a model override for a session (e.g., from /sf model command in auto-mode). + */ export function setSessionModelOverride( sessionId: string, override: SessionModelOverride, @@ -21,6 +27,9 @@ export function setSessionModelOverride( }); } +/** + * Retrieve the model override for a session, if one was set. + */ export function getSessionModelOverride( sessionId: string, ): SessionModelOverride | undefined { @@ -29,6 +38,9 @@ export function getSessionModelOverride( return sessionOverrides.get(key); } +/** + * Clear the model override for a session (e.g., at auto-mode end). + */ export function clearSessionModelOverride(sessionId: string): void { const key = normalizeSessionId(sessionId); if (!key) return; diff --git a/src/resources/extensions/sf/tests/post-execution-checks.test.ts b/src/resources/extensions/sf/tests/post-execution-checks.test.ts index 0a8640557..8adc3a7f7 100644 --- a/src/resources/extensions/sf/tests/post-execution-checks.test.ts +++ b/src/resources/extensions/sf/tests/post-execution-checks.test.ts @@ -587,7 +587,7 @@ describe("checkPatternConsistency", () => { const asyncResults = results.filter((r) => r.message.includes("async")); assert.equal(asyncResults.length, 1); assert.equal(asyncResults[0].category, "pattern"); - assert.equal(asyncResults[0].passed, true); // Warning only + assert.equal(asyncResults[0].passed, false); assert.equal(asyncResults[0].blocking, false); } finally { rmSync(tempDir, { recursive: true, force: true }); diff --git a/src/resources/extensions/sf/tests/provider-errors.test.ts b/src/resources/extensions/sf/tests/provider-errors.test.ts index c128890af..36b794b4f 100644 --- a/src/resources/extensions/sf/tests/provider-errors.test.ts +++ b/src/resources/extensions/sf/tests/provider-errors.test.ts @@ -396,6 +396,7 @@ test("resumeAutoAfterProviderDelay restarts paused auto-mode from the recorded b stepMode: true, basePath: "/tmp/project", }), + resetTransientRetryState: () => {}, startAuto: async (_ctx, _pi, base, verboseMode, options) => { startCalls.push({ base, verboseMode, step: options?.step }); }, @@ -430,6 +431,7 @@ test("resumeAutoAfterProviderDelay uses stored command context when event contex stepMode: false, basePath: "/tmp/project", }), + resetTransientRetryState: () => {}, getCommandContext: () => storedCommandCtx, startAuto: async (ctx, _pi, base) => { startCalls.push({ sameCtx: ctx === storedCommandCtx, base }); @@ -453,6 +455,7 @@ test("resumeAutoAfterProviderDelay does not double-start when auto-mode is alrea stepMode: false, basePath: "/tmp/project", }), + resetTransientRetryState: () => {}, startAuto: async () => { startCalls += 1; }, @@ -483,6 +486,7 @@ test("resumeAutoAfterProviderDelay leaves auto paused when no base path is avail stepMode: false, basePath: "", }), + resetTransientRetryState: () => {}, startAuto: async () => { startCalls += 1; }, @@ -520,6 +524,7 @@ test("resumeAutoAfterProviderDelay leaves paused when no command context is avai stepMode: false, basePath: "/tmp/project", }), + resetTransientRetryState: () => {}, getCommandContext: () => null, startAuto: async () => { startCalls += 1; diff --git a/src/resources/extensions/sf/workflow-mcp.ts b/src/resources/extensions/sf/workflow-mcp.ts index e57510f0e..19da75386 100644 --- a/src/resources/extensions/sf/workflow-mcp.ts +++ b/src/resources/extensions/sf/workflow-mcp.ts @@ -57,6 +57,9 @@ function parseLookupOutput(output: Buffer | string): string { return output.toString().trim().split(/\r?\n/)[0] ?? ""; } +/** + * Parse a JSON-encoded environment variable or return undefined. + */ function parseJsonEnv(env: NodeJS.ProcessEnv, name: string): T | undefined { const raw = env[name]; if (!raw) return undefined; @@ -67,6 +70,9 @@ function parseJsonEnv(env: NodeJS.ProcessEnv, name: string): T | undefined { } } +/** + * Resolve a command path using which/where or return null if not found. + */ function lookupCommand( command: string, platform: NodeJS.Platform = process.platform, @@ -82,6 +88,9 @@ function lookupCommand( } } +/** + * Search ancestor directories for the bundled workflow CLI. + */ function findWorkflowCliFromAncestorPath(startPath: string): string | null { let current = resolve(startPath); @@ -103,6 +112,9 @@ function findWorkflowCliFromAncestorPath(startPath: string): string | null { return null; } +/** + * Resolve the bundled workflow MCP CLI path from env anchors or module search. + */ function getBundledWorkflowMcpCliPath(env: NodeJS.ProcessEnv): string | null { const envAnchors = [ env.SF_BIN_PATH?.trim(), @@ -153,6 +165,9 @@ function getBundledWorkflowMcpCliPath(env: NodeJS.ProcessEnv): string | null { return null; } +/** + * Resolve the bundled workflow tool executors module path. + */ function getBundledWorkflowExecutorModulePath(): string | null { const candidates = [ resolve( @@ -182,6 +197,9 @@ function getBundledWorkflowExecutorModulePath(): string | null { return null; } +/** + * Resolve the bundled write-gate module path. + */ function getBundledWorkflowWriteGateModulePath(): string | null { const candidates = [ resolve( diff --git a/src/resources/extensions/sf/worktree-manager.ts b/src/resources/extensions/sf/worktree-manager.ts index d39813292..8dbd9b682 100644 --- a/src/resources/extensions/sf/worktree-manager.ts +++ b/src/resources/extensions/sf/worktree-manager.ts @@ -70,6 +70,10 @@ export interface FileLineStat { removed: number; } +/** + * Summary of .sf/ artifact changes between worktree branch and main. + * Used to present merge previews and guide LLM conflict resolution. + */ export interface WorktreeDiffSummary { /** Files only in the worktree .sf/ (new artifacts) */ added: string[]; diff --git a/src/resources/extensions/sf/worktree-resolver.ts b/src/resources/extensions/sf/worktree-resolver.ts index bece0b07d..dd77c1203 100644 --- a/src/resources/extensions/sf/worktree-resolver.ts +++ b/src/resources/extensions/sf/worktree-resolver.ts @@ -79,6 +79,7 @@ export interface WorktreeResolverDeps { // ─── Notify Context ──────────────────────────────────────────────────────── +/** Context for emitting user notifications during worktree lifecycle transitions. */ export interface NotifyCtx { notify: ( msg: string, @@ -88,6 +89,10 @@ export interface NotifyCtx { // ─── WorktreeResolver ────────────────────────────────────────────────────── +/** + * Manages worktree lifecycle for auto-mode: enter/exit milestone, merge/transition. + * Encapsulates path state, handles git isolation modes, and emits telemetry (ADR-019). + */ export class WorktreeResolver { private readonly s: AutoSession; private readonly deps: WorktreeResolverDeps;