feat(interview): Candidate AI Code Interview tool (all 5 slices)

[asabaylus]

Summary

teamhero interview is a candidate-facing AI-coding take-home experience, not a grading tool. The feature treats the interview as a project the candidate works on with whatever AI tooling they prefer, in a time-box, on their own machine — and only then, as a separate step, produces an advisory audit of how they collaborated with the AI.

The framing matters. The product is the candidate experience: a fair, modern, native generative-AI coding situation. The audit is a side effect that helps the hiring manager remember and reason about the session — it is never a score, never a ranking, and never determinative.

How it works end-to-end

1. Hiring manager bootstraps a role. teamhero interview bootstrap (interactive wizard, or headless flags for CI) generates a starter project tailored to the role's stack, domain, and a one-sentence feature spec. The output is a real working project with a failing test, a CLAUDE.md, a GLOSSARY.md, and 2+ deep modules — sized to fit a 60/90/120-minute window. See docs/interview-classification-rationale.md for the full pedagogical and ethical framing.

2. Candidate receives the kit and the project. Bundled with the project is teamhero-interview-kit/ containing:

   • INTERVIEW_RULES.md — what the candidate can and can't do (short version: use any AI tool you like).
   • RUBRIC_OVERVIEW.md — a plain-language explanation of the 9 observation dimensions, given to the candidate up-front. Nothing is hidden from them.
   • PRIVACY_RELEASE.md — explicit consent form: what is recorded, how it may and may not be used, and the appeal / deletion mechanism. The start.sh recording script will refuse to run until this is signed.

3. Candidate works the project. They run start.sh, which starts an asciinema recording, hands them a shell, and lets them work as they normally would. Any AI tool is fair game — Claude Code, Cursor, Copilot, Cody, plain ChatGPT-in-a-browser. The cost of any AI usage they accumulate on their personal accounts during the interview is reimbursed via a $50 gift card so that tool choice stays an honest signal of preference and not a wallet-driven compromise.

4. Candidate finishes. They run end.sh, which packages the recording + their commits into the repo for handoff.

5. Hiring manager runs the audit. teamhero interview grade --candidate "Jane" --repo <url> produces a two-tier markdown audit:

   • summary.md — narrative observations across 9 dimensions, plus a mandatory manager sign-off section the manager must fill in with a categorical recommendation (Hire / Hire with notes / No hire) and reasoning ≥ 20 chars.
   • audit.md — the full reasoning chain, every raw measurement, the exact bias-guard prompt used.

   Every audit ships with an ADVISORY banner pinned above the content — "the audit is advisory; the candidate is a person, not a score; the audit is one factor among many." The audit is never returned to the candidate by default (GDPR Art. 15 caveat is documented for the EU/UK case).

6. Hiring manager reviews the cohort. teamhero interview cohort --role <slug> rolls up all candidates for a role into a rankless listing with sign-off status — no Score, no Total, no Rank columns by design.

What ships in this PR

Layer	Components
Candidate-facing	teamhero-interview-kit/ — POSIX privacy gate, start.sh / end.sh, INTERVIEW_RULES.md, RUBRIC_OVERVIEW.md, PRIVACY_RELEASE.md, .claude/ templates
Hiring-manager-facing (TUI)	Interactive huh wizard for bootstrap; phased progress display for grade (clone → collect-evidence → extract-measurements → observe → audit-write); glamour-rendered audit preview with pinned ADVISORY banner; shared interviewStyles palette
Engine (TS)	Rubric module (9 dims, 2 groups, 3 evidence modes); project validator + generator (Mode A and B); role config; 4 evidence collectors + 4 deterministic extractors; AI observer with bias guard + strict scoreless schema; audit writer with sign-off validator; rankless cohort summary
Documentation	docs/interview-rubric.md, docs/interview-classification-rationale.md, skills/teamhero-interview/SKILL.md (the conversational wrapper), README section 4

Ethical commitments — preserved end-to-end in code, schema, and docs

• Observations, not scores. Strict OpenAI json_schema with additionalProperties: false, plus a second-layer rejectIfScored validator that throws if the parsed response contains any forbidden field.
• Bias diversification, not elimination. The docs and skill never claim the AI is unbiased — only that its biases are different from any individual reviewer's.
• Mandatory human-in-the-loop. Every audit requires a manager sign-off with a categorical recommendation and ≥ 20-char reasoning summary.
• Interviewer-bias guard verbatim in every observation prompt (unit-tested).
• Session recording URL never reaches the LLM — stored in frontmatter only.
• Mandatory ADVISORY banner on summary, audit, cohort report, and at the start of every grade run.

Manual QA Instructions

A bundled smoke script walks through the candidate-experience and hiring-manager flows without spending money on OpenAI:

just build-all                                          # builds the Go TUI + TS engine
./scripts/manual-test-interview.sh                      # walks five interactive steps

The script requires a TTY (it bails on piped stdin) and uses --mode-analysis human-only for the grade step so no API key is needed.

What to verify at each step

1. teamhero interview bootstrap with no flags drops into the wizard. Try each rubric mode: default (skips both follow-ups), custom (reveals a text area for the prompt), default+jd (reveals a file picker; validates the path exists). Press Ctrl+C from the first screen — it must exit cleanly with no partial output.

2. Headless bootstrap is unchanged. The script then runs teamhero interview bootstrap --headless --role manual-test-role ... --output-dir /tmp/.... No TUI should render. Inspect the output directory:

   • CLAUDE.md and GLOSSARY.md present
   • 2+ deep modules in the generated source
   • A failing test (the candidate's starting point)
   • LOC of starter scaffold roughly 400–700
   • teamhero-interview-kit/ contents copied in

3. teamhero interview grade with --mode-analysis human-only should show:

   • The ADVISORY banner at the start
   • A phased progress display: clone → collect-evidence → extract-measurements → observe → audit-write. (In human-only mode the observe phase is a near-instant no-op.)
   • A glamour-rendered preview of summary.md with the ADVISORY banner pinned at the top, before any candidate name

4. Sign-off gating. Open the generated summary.md / audit.md. Confirm:

   • ADVISORY banner is at the top of both files
   • Sign-off section is present and requires a categorical recommendation
   • The audit.json contains no score / weighted_total / band fields
   • Session recording URL (if you passed one) appears in YAML frontmatter only, never in the body

5. teamhero interview cohort --role <slug> should render a rankless listing — no Score, Total, or Rank columns.

Optional deeper verification

just test-all                  # 1660 TS pass / 0 fail; full Go suite green
just tui-test -cover           # confirm Go coverage stays ≥ 85% (currently 87.7%)

Candidate-facing kit (independent of the audit flow)

To verify the candidate experience without running a full grade:

cd /tmp/manual-test-role/teamhero-interview-kit/
./start.sh                     # should refuse — PRIVACY_RELEASE.md is unsigned
# Edit PRIVACY_RELEASE.md to add a name + date under "## Signed" / "## Date"
SKIP_RECORD=1 ./start.sh       # privacy gate now passes; asciinema is skipped in test mode

References

• Planning doc — docs/2026-05-09-candidate-interview-grader-plan.md (now includes a full Implementation Progress table)
• Rubric — docs/interview-rubric.md
• Defensibility / ethics framing — docs/interview-classification-rationale.md
• Candidate rules — teamhero-interview-kit/INTERVIEW_RULES.md
• Rubric (plain-language for the candidate) — teamhero-interview-kit/RUBRIC_OVERVIEW.md
• Privacy release — teamhero-interview-kit/PRIVACY_RELEASE.md
• Conversational wrapper — skills/teamhero-interview/SKILL.md

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Full "teamhero interview" flow: interactive bootstrap wizard or equivalent headless flags, headless review (phased progress display, glamour-rendered audit preview, mandatory ADVISORY banner), and cohort summary generation (COHORT.md).

• Documentation

  • Expanded Quick Start, rubric, rationale, interview kit, privacy release, and skill guidance emphasizing observation-only outputs (no numeric scores) and required human sign-off.

• Scripts

  • Add start/end session scripts and a manual test script to validate interview workflows and artifacts (summary/audit/cohort).

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a complete interview-review feature: CLI/TUI verbs (bootstrap, review, cohort), headless Bun scripts, TypeScript services for bootstrap and review, evidence collectors/extractors, OpenAI observer with strict schema and bias guard, audit/cohort writers with ADVISORY banner and sign-off, kit scripts/docs, and extensive tests.

Changes

Interview Reviewer

Layer / File(s)	Summary
CLI & TUI entrypoints src/cli/index.ts, tui/interview.go, tui/*_bootstrap*.go, tui/interview_review.go, tui/interview_cohort.go, tui/main.go	Registers interview subcommand, forwards help/flags, adds headless verbs and TUI dispatching with preview/progress rendering.
Role config & rubric src/services/interview/bootstrap/role-config.ts, src/services/interview/shared/rubric.ts	Adds RoleConfig types/validation, Project/Analysis/Rubric modes, and a fixed rubric catalog with evidence modes.
Bootstrap generation src/services/interview/bootstrap/project-generator.ts, src/services/interview/bootstrap/openai-generator-client.ts, src/services/interview/bootstrap/orchestrator.ts, scripts/run-interview-bootstrap.ts	Generator client (OpenAI), safe file path handling, retries, validators, runBootstrap orchestrator, and headless bootstrap script.
Evidence collectors src/services/interview/review/collectors/asciinema.ts, jsonl-log.ts, transcript.ts, git-history.ts	Parsers that normalize terminal, prompt/tool, transcript, and git history into EvidenceEvent streams.
Deterministic extractors src/services/interview/review/extractors/*	extractVerification, extractTestPass, extractThroughput, extractRiskAwareness compute Measurements from evidence.
AI observer & orchestration src/services/interview/review/ai-observer.ts, src/services/interview/review/review-orchestrator.ts, scripts/run-interview-review.ts	Builds observation-only prompts (bias guard), strict response schema, OpenAI client, and reviewCandidate orchestration: clone/read, collect, measure, observe or human-only, write audits.
Audit & cohort writers src/services/interview/review/audit-writer.ts, src/services/interview/cohort/cohort-summary.ts, scripts/run-interview-cohort.ts	Render/write summary.md/audit.md/audit.json with ADVISORY banner and sign-off, produce COHORT.md roll-ups.
TUI wizard & preview tui/interview_bootstrap_form.go, tui/interview_bootstrap_wizard.go, tui/interview_preview.go, tui/interview_progress.go, tui/interview_styles.go	Interactive huh bootstrap wizard, validators, glamour preview (pinned advisory), progress model, and styles.
Interview kit & scripts teamhero-interview-kit/*, teamhero-interview-kit/lib/privacy-gate.sh, scripts/manual-test-interview.sh, teamhero-interview-kit/start.sh, end.sh	Kit assets, Claude hooks/settings, privacy release template, start/end scripts, privacy gate, and manual smoke-test script.
Docs & skill docs/*, README.md, skills/teamhero-interview/SKILL.md	Adds classification rationale, rubric, reviewer plan, README quick start, and Claude skill docs enforcing observations-not-scores and human sign-off.
Tests tests/unit/**, tui/*_test.go	Extensive unit and edge tests for generators, validators, collectors, extractors, observer, audit writer, cohort, TUI, and kit scripts.
Utilities / CI scripts/run-tests.sh, .coderabbit.yaml	Improved test summary parsing and CodeRabbit config for automated reviews.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as teamhero CLI
  participant TUI as Go TUI
  participant Bun as Bun scripts
  participant Svc as TS Services
  participant OpenAI as OpenAI Responses API
  participant FS as Filesystem

  CLI->>TUI: run `teamhero interview review ...`
  TUI->>Bun: invoke `scripts/run-interview-review.ts` (flags)
  Bun->>Svc: reviewCandidate(input, deps)
  Svc->>FS: clone/read repo, read evidence files
  Svc->>Svc: compute measurements (extractors)
  Svc->>OpenAI: observe(prompt with strict json_schema)
  OpenAI-->>Svc: structured observations
  Svc->>FS: write summary.md, audit.md, audit.json, evidence/
  Bun-->>TUI: return artifact paths
  TUI-->>CLI: render preview + ADVISORY banner

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

"A rabbit scrawls a checklist, bright and fleet,
Through prompts and logs and tests it nibbles neat.
ADVISORY pinned, observations not score,
Papers and audits across the floor.
🐇✨"

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/slice-1-foundation

[coderabbitai]

Actionable comments posted: 4

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (18)

teamhero-interview-kit/lib/privacy-gate.sh-16-16 (1)

16-16: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Move set -u inside the conditional execution block to avoid mutating the caller's environment.

The global set -u at line 16 is applied whenever this file is sourced as a library, which unexpectedly enables the nounset option in any caller that doesn't already have it enabled. This violates the library pattern and can break scripts that rely on unset variable behavior.

Move the set -u declaration inside the conditional check at lines 72–75 so it only applies during direct script execution:

Suggested fix

-set -u
-
# extract_section <file> <heading> — prints the body that follows a given

 # When invoked directly (not sourced), run the check on $1 and exit with the
 # function's return code.
 if [ "${BASH_SOURCE[0]:-$0}" = "$0" ]; then
+    set -u
     check_privacy_release_signed "${1:-}"
     exit $?
 fi

🤖 Prompt for AI Agents

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

In `@teamhero-interview-kit/lib/privacy-gate.sh` at line 16, Remove the top-level
"set -u" so the library doesn't change the caller's shell options, and instead
enable nounset only inside the script's main execution guard (the conditional
that checks whether the file is run directly vs sourced); place "set -u" at the
start of that guard block (and leave it out of any exported functions or
top-level code) so nounset applies only during direct execution and not when the
file is sourced.

src/services/interview/bootstrap/project-validator.ts-21-29 (1)

21-29: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent recursive walk from following symlink cycles

walk uses statSync, which follows symlinks. A cyclic symlink inside generated project contents can cause unbounded recursion.

Proposed fix

-import { existsSync, readFileSync, readdirSync, statSync } from "node:fs";
+import { existsSync, lstatSync, readFileSync, readdirSync } from "node:fs";
@@
 	for (const entry of readdirSync(dir)) {
 		if (entry === "node_modules" || entry === ".git") continue;
 		const full = join(dir, entry);
-		const s = statSync(full);
+		const s = lstatSync(full);
+		if (s.isSymbolicLink()) continue;
 		if (s.isDirectory()) walk(full, out);
 		else out.push(full);
 	}

🤖 Prompt for AI Agents

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

In `@src/services/interview/bootstrap/project-validator.ts` around lines 21 - 29,
The walk function can follow symlinks because it uses statSync, causing
potential cycles; change walk(dir: string, out: string[] = []) to skip symbolic
links (use lstatSync and if entry is a symlink, skip or optionally resolve
safely) or add a visited set of realpaths to prevent revisiting; specifically
update the logic around readdirSync + statSync to use lstatSync and either
ignore entries where lstatSync(...).isSymbolicLink() or track realpath
(fs.realpathSync) in a visited Set inside walk (or an additional parameter) and
return early when a path is already visited to prevent unbounded recursion.

src/services/interview/shared/events.ts-30-41 (1)

30-41: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add validation for step and status fields in parseInterviewEvent

The function accepts {"type":"progress"} without validating required fields step and status, allowing malformed events to pass type checking at runtime. The message and progress optional fields are also unvalidated if present.

Proposed fix

 export function parseInterviewEvent(line: string): InterviewEvent | null {
 	let value: unknown;
 	try {
 		value = JSON.parse(line);
 	} catch {
 		return null;
 	}
 	if (!value || typeof value !== "object") return null;
-	const type = (value as { type?: unknown }).type;
+	const obj = value as Record<string, unknown>;
+	const type = obj.type;
 	if (typeof type !== "string") return null;
 	if (!KNOWN_EVENT_TYPES.includes(type as InterviewEvent["type"])) return null;
-	return value as InterviewEvent;
+	if (type === "progress") {
+		const step = obj.step;
+		const status = obj.status;
+		if (typeof step !== "string") return null;
+		if (
+			status !== "start" &&
+			status !== "update" &&
+			status !== "done" &&
+			status !== "error"
+		) {
+			return null;
+		}
+		if (obj.message !== undefined && typeof obj.message !== "string") return null;
+		if (obj.progress !== undefined && typeof obj.progress !== "number") return null;
+		return obj as InterviewProgressEvent;
+	}
+	return null;
 }

🤖 Prompt for AI Agents

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

In `@src/services/interview/shared/events.ts` around lines 30 - 41,
parseInterviewEvent currently accepts {"type":"progress"} without validating
required fields; update parseInterviewEvent to, after confirming type is
"progress", check that (value as any).step exists and is a string and (value as
any).status exists and is a string and matches the allowed progress statuses
(use your existing InterviewEvent status union or a small array of allowed
status strings), and also validate optional fields: if message is present it
must be a string, and if progress is present it must be a number; reject (return
null) for any type-mismatches so that only well-formed InterviewEvent objects
(for all types, but especially "progress") are returned while still using
KNOWN_EVENT_TYPES and casting to InterviewEvent when validation passes.

src/services/interview/assess/extractors/verification.ts-58-71 (1)

58-71: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Interleaving metric ignores the advertised 30-second window.

The code only checks prompt→test adjacency, but never validates the 30s condition described in the comment and fact label context.

Proposed fix

 	let interleavedAfterPrompt = 0;
 	for (let i = 1; i < merged.length; i++) {
 		const cur = merged[i];
 		const prev = merged[i - 1];
 		if (cur.type === "command" && prev.type === "prompt") {
-			interleavedAfterPrompt += 1;
+			const deltaMs =
+				new Date(cur.timestamp).getTime() - new Date(prev.timestamp).getTime();
+			if (Number.isFinite(deltaMs) && deltaMs >= 0 && deltaMs <= 30_000) {
+				interleavedAfterPrompt += 1;
+			}
 		}
 	}

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/extractors/verification.ts` around lines 58 -
71, The interleaving count currently only checks adjacency between merged
entries but ignores the intended 30-second window; update the loop that computes
interleavedAfterPrompt (which iterates over merged built from prompts and
testRuns) to parse timestamps to numeric time and only increment
interleavedAfterPrompt when cur.type === "command" && prev.type === "prompt" &&
(new Date(cur.timestamp).getTime() - new Date(prev.timestamp).getTime()) <=
30000; ensure merged remains sorted by timestamp before this check.

src/services/interview/bootstrap/role-config.ts-121-126 (1)

121-126: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate and harden readRoleConfig instead of unchecked cast.

JSON.parse(... as RoleConfig) allows malformed shape through (or throws unwrapped parse errors), which makes downstream failures harder to diagnose.

Proposed fix

 export function readRoleConfig(dir: string): RoleConfig | null {
 	const path = join(dir, ROLE_CONFIG_FILENAME);
 	if (!existsSync(path)) return null;
-	const body = readFileSync(path, "utf8");
-	return JSON.parse(body) as RoleConfig;
+	const body = readFileSync(path, "utf8");
+	let parsed: unknown;
+	try {
+		parsed = JSON.parse(body);
+	} catch (error) {
+		throw new Error(`Invalid ${ROLE_CONFIG_FILENAME}: malformed JSON`);
+	}
+	const result = validateRoleConfig(parsed as RoleConfig);
+	if (!result.ok) {
+		throw new Error(
+			`Invalid ${ROLE_CONFIG_FILENAME}: ${result.failures.join("; ")}`,
+		);
+	}
+	return parsed as RoleConfig;
 }

🤖 Prompt for AI Agents

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

In `@src/services/interview/bootstrap/role-config.ts` around lines 121 - 126,
readRoleConfig currently does an unchecked JSON.parse and casts to RoleConfig;
wrap the read+parse in a try/catch to surface parse errors and then validate the
parsed object’s shape (check required properties and their types expected by
RoleConfig) before returning it — if validation fails return null or throw a
descriptive Error with context including ROLE_CONFIG_FILENAME and the path, so
callers don’t receive a malformed RoleConfig instance; implement this inside
readRoleConfig.

src/services/interview/assess/collectors/transcript.ts-24-37 (1)

24-37: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Transcript parser is too brittle for common speaker labels and invalid anchor timestamps.

The bare-speaker regex can skip valid lines (e.g., Speaker 1: ...), and an invalid sessionStartIso can throw on toISOString().

Proposed fix

-const BARE = /^([A-Za-z][A-Za-z .'-]+?):\s+(.+)$/;
+const BARE = /^([^:\[\]#][^:]{0,79}?):\s+(.+)$/;

 function toIsoFromHMS(hms: string, sessionStartIso: string): string {
 	const parts = hms.split(":").map(Number);
@@
-	const base = new Date(sessionStartIso).getTime();
-	return new Date(base + totalSeconds * 1000).toISOString();
+	const parsedBase = new Date(sessionStartIso).getTime();
+	const base = Number.isFinite(parsedBase)
+		? parsedBase
+		: new Date("1970-01-01T00:00:00.000Z").getTime();
+	return new Date(base + totalSeconds * 1000).toISOString();
 }

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/collectors/transcript.ts` around lines 24 - 37,
The bare-speaker regex (BARE) is too strict (misses labels like "Speaker 1") and
toIsoFromHMS can throw if sessionStartIso or parsed HMS are invalid; update BARE
to allow digits and optional numeric suffixes (e.g., change /^([A-Za-z][A-Za-z
.'-]+?):\s+(.+)$/ to accept names with digits and leading words such as "Speaker
1"), and harden toIsoFromHMS: validate sessionStartIso by parsing new
Date(sessionStartIso) and checking for NaN, validate each HMS part with
Number.isFinite before computing totalSeconds, and if either the base date or
HMS parts are invalid return a safe fallback (e.g., the original sessionStartIso
or new Date().toISOString()) instead of calling toISOString on an invalid date;
reference TIMESTAMPED and BARE for regex fixes and toIsoFromHMS for the
parsing/validation changes.

src/services/interview/assess/extractors/test-pass.ts-20-42 (1)

20-42: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add timeout and error handling to test runner processes to prevent grader hangs.

Both spawnSync() calls lack timeout configuration and don't capture r.error, which can cause the grader to block indefinitely if a test process hangs or fails to spawn.

Proposed fix

+const TEST_RUN_TIMEOUT_MS = 120_000;
+
 const realRunner: TestRunner = (repoDir) => {
 	if (existsSync(join(repoDir, "go.mod"))) {
 		const r = spawnSync("go", ["test", "./..."], {
 			cwd: repoDir,
 			encoding: "utf8",
+			timeout: TEST_RUN_TIMEOUT_MS,
+			killSignal: "SIGKILL",
 		});
 		return {
 			passed: countMatches(r.stdout ?? "", /^ok\s+/gm),
 			failed: countMatches(r.stdout ?? "", /^FAIL\s+/gm),
-			output: `${r.stdout ?? ""}\n${r.stderr ?? ""}`,
+			output: `${r.stdout ?? ""}\n${r.stderr ?? ""}${
+				r.error ? `\n${r.error.message}` : ""
+			}`,
 		};
 	}
 	if (existsSync(join(repoDir, "package.json"))) {
 		const r = spawnSync("bun", ["test"], {
 			cwd: repoDir,
 			encoding: "utf8",
+			timeout: TEST_RUN_TIMEOUT_MS,
+			killSignal: "SIGKILL",
 		});
-		const combined = `${r.stdout ?? ""}\n${r.stderr ?? ""}`;
+		const combined = `${r.stdout ?? ""}\n${r.stderr ?? ""}${
+			r.error ? `\n${r.error.message}` : ""
+		}`;

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/extractors/test-pass.ts` around lines 20 - 42,
The spawnSync calls in src/services/interview/assess/extractors/test-pass.ts
(the blocks that run "go test ./..." and "bun test") need timeout and error
handling to avoid hangs: add a timeout option (e.g. 30000 ms) and a larger
maxBuffer to both spawnSync invocations, check r.error and r.status (or
r.signal) and include r.error/message/signal in the returned output string, and
treat a timeout/spawn failure as test failures (set failed>0 or a specific flag)
by returning a non-zero failed count when r.error or r.signal is present; update
the returned objects for the functions that use the "r" result so both
passed/failed and output reflect these error/timeout conditions.

src/services/interview/assess/collectors/git-history.ts-14-18 (1)

14-18: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace execSync with execFileSync to avoid shell string construction.

Building a shell command string for execSync is unnecessary and fragile. Pass arguments directly to the git executable via execFileSync, which avoids shell interpretation entirely and is the standard Node.js best practice for spawning processes.

Proposed fix

-import { execSync } from "node:child_process";
+import { execFileSync } from "node:child_process";
@@
 const realRunner: GitRunner = (args, cwd) =>
-	execSync(`git ${args.map((a) => `'${a.replace(/'/g, "'\\''")}'`).join(" ")}`, {
+	execFileSync("git", args, {
 		cwd,
 		encoding: "utf8",
 	});

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/collectors/git-history.ts` around lines 14 -
18, The current realRunner builds a shell command string and calls execSync,
which is fragile; change realRunner to call execFileSync directly with the git
executable and the args array (e.g. execFileSync('git', args, { cwd, encoding:
'utf8' })), remove the manual quoting/escaping logic, and ensure execFileSync is
imported from child_process; keep the GitRunner signature (args, cwd) and
preserve options like cwd and encoding.

scripts/run-interview-grade.ts-7-7 (1)

7-7: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove override: true to prevent CI/shell-provided environment variables from being overwritten.

The override: true option causes dotenv to replace any existing process.env values with those from the .env file. This will clobber runtime-provided secrets and configuration (e.g., from CI systems), breaking expected behavior in non-local environments. Use the default behavior instead.

Proposed fix

-loadDotenv({ override: true });
+loadDotenv();

🤖 Prompt for AI Agents

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

In `@scripts/run-interview-grade.ts` at line 7, Call to loadDotenv({ override:
true }) in scripts/run-interview-grade.ts is forcing .env values to overwrite
existing environment variables; remove the override option so dotenv uses
default behavior and does not clobber CI/shell-provided vars. Locate the
loadDotenv invocation in that file (the loadDotenv symbol) and change it to
loadDotenv() or remove the override property so existing process.env entries
remain intact.

src/services/interview/assess/collectors/jsonl-log.ts-28-30 (1)

28-30: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle missing interview.log as empty evidence instead of throwing.

A missing log file currently throws before parsing starts, which can fail an entire grade run for absent optional evidence.

💡 Proposed fix

 export function parseInterviewLog(path: string): InterviewLogParseResult {
-	const body = readFileSync(path, "utf8");
+	let body = "";
+	try {
+		body = readFileSync(path, "utf8");
+	} catch (err) {
+		if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+			return { prompts: [], toolUses: [] };
+		}
+		throw err;
+	}
 	const lines = body.split("\n").filter((l) => l.trim().length > 0);

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/collectors/jsonl-log.ts` around lines 28 - 30,
The parseInterviewLog function currently calls readFileSync and throws if the
file is missing; update parseInterviewLog to treat a missing interview.log as
empty evidence instead of failing: check for file existence (e.g., using
fs.existsSync) or wrap readFileSync in a try/catch, and if the file is absent or
unreadable return an empty InterviewLogParseResult (empty lines/entries) rather
than letting the exception propagate; keep the rest of parsing logic intact so
parseInterviewLog and its callers receive an empty result when interview.log is
not present.

scripts/run-interview-bootstrap.ts-81-93 (1)

81-93: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate timeBox and mode flag values before constructing RoleConfig.

The current parsing allows invalid values (e.g., --time-box abc, unknown --mode-*) to pass as typed config and fail later in less clear ways.

💡 Proposed fix

 function buildConfig(flags: ParsedFlags): RoleConfig | string {
@@
-	const timeBox = Number.parseInt(flags.timeBox ?? "90", 10);
+	const timeBox = Number.parseInt(flags.timeBox ?? "90", 10);
+	if (!Number.isFinite(timeBox) || timeBox < 30 || timeBox > 240) {
+		return "Invalid --time-box; expected an integer between 30 and 240";
+	}
+
+	const projectModes = new Set<ProjectMode>(["A", "B"]);
+	const analysisModes = new Set<AnalysisMode>(["ai-assisted", "human-only"]);
+	const rubricModes = new Set<RubricMode>(["default", "custom", "default+jd"]);
+
+	if (!projectModes.has(flags.modeProject as ProjectMode)) {
+		return "Invalid --mode-project; expected A or B";
+	}
+	if (!analysisModes.has(flags.modeAnalysis as AnalysisMode)) {
+		return "Invalid --mode-analysis; expected ai-assisted or human-only";
+	}
+	if (!rubricModes.has(flags.modeRubric as RubricMode)) {
+		return "Invalid --mode-rubric; expected default, custom, or default+jd";
+	}

🤖 Prompt for AI Agents

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

In `@scripts/run-interview-bootstrap.ts` around lines 81 - 93, Before constructing
RoleConfig, validate flags.timeBox and each mode flag: ensure timeBox
(flags.timeBox) parses to a positive integer and reject non-numeric or <=0
values (set default only after validation), and verify flags.modeProject,
flags.modeAnalysis, and flags.modeRubric are valid members of the corresponding
enums/types (ProjectMode, AnalysisMode, RubricMode); if any validation fails,
log a clear error and exit/throw rather than letting invalid values flow into
the RoleConfig. Perform these checks just above where timeBox is parsed and the
config object is created (referencing the timeBox variable and the RoleConfig
construction), and normalize/cast only after validation so RoleConfig gets only
safe, well-typed values.

teamhero-interview-kit/start.sh-57-59 (1)

57-59: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Write .interview-state/started-at only after recorder preflight passes.

Right now, a missing asciinema still leaves a start marker behind. That creates a false “session started” state even though recording never began.

Suggested reorder

-# Indicate we passed the gate so end.sh can verify the kit was started.
-mkdir -p "$REPO_ROOT/.interview-state"
-date -u +%FT%TZ > "$REPO_ROOT/.interview-state/started-at"
-
 if [ "${SKIP_RECORD:-0}" = "1" ]; then
+    mkdir -p "$REPO_ROOT/.interview-state"
+    date -u +%FT%TZ > "$REPO_ROOT/.interview-state/started-at"
     echo "✓ Privacy gate passed. SKIP_RECORD=1 — not starting asciinema."
     exit 0
 fi
@@
 if ! command -v "$ASCIINEMA_BIN" >/dev/null 2>&1; then
@@
     exit 1
 fi
 
+# Indicate we passed all prerequisites and are starting recording.
+mkdir -p "$REPO_ROOT/.interview-state"
+date -u +%FT%TZ > "$REPO_ROOT/.interview-state/started-at"
+
 CAST_PATH="$REPO_ROOT/terminal.cast"

Also applies to: 66-79

🤖 Prompt for AI Agents

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

In `@teamhero-interview-kit/start.sh` around lines 57 - 59, The start marker file
".interview-state/started-at" is written before the recorder preflight
completes, causing false "session started" states if asciinema or recorder setup
fails; move the mkdir -p "$REPO_ROOT/.interview-state" and the date -u ... >
"$REPO_ROOT/.interview-state/started-at" lines out of the current top-level
start flow and into the success branch that runs after the recorder
preflight/asciinema checks complete (i.e., only execute these lines when the
script confirms recorder is present and recording has been started), and apply
the same reorder for the duplicate block around lines 66-79 so the marker is
only created on successful recorder initialization.

src/services/interview/assess/collectors/asciinema.ts-68-85 (1)

68-85: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Parse input chunks character-by-character to avoid missed command submits.

evt.data may contain multi-character chunks (e.g., pasted text or cmd\r in one event). The current branch logic treats the whole chunk as one token, which can fail to emit commands correctly.

Suggested fix

-		const data = evt.data;
-		if (data === "\r" || data === "\n") {
-			if (buffer.trim().length > 0) {
-				commands.push({
-					type: "command",
-					timestamp: isoFromUnix(baseEpoch + evt.delta),
-					source: "terminal.cast",
-					command: buffer,
-					pauseSecondsBeforeEnter: Math.max(0, evt.delta - lastKeyDelta),
-				});
-			}
-			buffer = "";
-		} else if (data === "�" || data === "\b") {
-			// Backspace
-			buffer = buffer.slice(0, -1);
-			lastKeyDelta = evt.delta;
-		} else if (data.charCodeAt(0) >= 32 || data === "\t") {
-			buffer += data;
-			lastKeyDelta = evt.delta;
-		} else {
-			// Control codes: arrows, escape sequences, etc. Skip silently.
-		}
+		for (const ch of evt.data) {
+			if (ch === "\r" || ch === "\n") {
+				if (buffer.trim().length > 0) {
+					commands.push({
+						type: "command",
+						timestamp: isoFromUnix(baseEpoch + evt.delta),
+						source: "terminal.cast",
+						command: buffer,
+						pauseSecondsBeforeEnter: Math.max(0, evt.delta - lastKeyDelta),
+					});
+				}
+				buffer = "";
+				continue;
+			}
+			if (ch === "\u007f" || ch === "\b") {
+				buffer = buffer.slice(0, -1);
+				lastKeyDelta = evt.delta;
+				continue;
+			}
+			if (ch === "\t" || ch.charCodeAt(0) >= 32) {
+				buffer += ch;
+				lastKeyDelta = evt.delta;
+			}
+		}

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/collectors/asciinema.ts` around lines 68 - 85,
The event handler currently treats evt.data as a single token, which misses
command boundaries when evt.data contains multiple characters (pastes or
"cmd\r"); update the processing block that uses data, buffer, commands,
isoFromUnix, lastKeyDelta and evt.delta to iterate over each character in
evt.data (e.g., for (const ch of data) { ... }) and run the existing branches
per-character (newline -> push command with timestamp from isoFromUnix(baseEpoch
+ evt.delta), backspace -> slice buffer, printable/tab -> append to buffer and
set lastKeyDelta = evt.delta), ensuring buffer resets after a submitted command
so multi-char chunks are parsed correctly.

src/services/interview/bootstrap/orchestrator.ts-36-49 (1)

36-49: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Wrap generation/persist steps to preserve the RunBootstrapResult contract.

generateProject(...) and writeRoleConfig(...) can throw; right now that escapes as an unhandled rejection instead of returning a structured failure. This makes bootstrap failure handling brittle at the CLI boundary.

Proposed fix

-	const generation = await generateProject(config, options.client, {
-		kitTemplateDir: options.kitTemplateDir,
-		maxAttempts: options.maxAttempts,
-	});
+	let generation;
+	try {
+		generation = await generateProject(config, options.client, {
+			kitTemplateDir: options.kitTemplateDir,
+			maxAttempts: options.maxAttempts,
+		});
+	} catch (error) {
+		return {
+			ok: false,
+			attempts: 0,
+			failures: [
+				error instanceof Error ? error.message : String(error),
+			],
+		};
+	}
 	if (!generation.ok) {
 		return {
 			ok: false,
 			attempts: generation.attempts,
 			failures: generation.failures,
 		};
 	}
-
-	writeRoleConfig(config.outputDir, config);
+	try {
+		writeRoleConfig(config.outputDir, config);
+	} catch (error) {
+		return {
+			ok: false,
+			attempts: generation.attempts,
+			failures: [
+				error instanceof Error ? error.message : String(error),
+			],
+		};
+	}

🤖 Prompt for AI Agents

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

In `@src/services/interview/bootstrap/orchestrator.ts` around lines 36 - 49, The
code calls generateProject(...) and writeRoleConfig(...) which can throw, so
wrap the generation and persistence steps in a try/catch around the calls to
generateProject and writeRoleConfig inside the function that returns
RunBootstrapResult; on any thrown error return a RunBootstrapResult-shaped
failure (ok: false) including generation.attempts/failures if available or
default values and include the caught error message/context in the failures
array or an error field; ensure the function still returns the existing
successful shape when generation.ok is true and only writes role config inside
the try block to avoid unhandled rejections (refer to generateProject,
writeRoleConfig, and the RunBootstrapResult contract).

tui/interview_bootstrap_wizard.go-249-251 (1)

249-251: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate wizard launch on interactive TTY before entering interactive flow.

The dispatcher comment says wizard mode is for TTY callers, but the current condition only checks len(args) == 0. In non-interactive contexts this can trigger unexpected wizard execution instead of deterministic headless behavior.

Proposed fix

 	// No flags at all → interactive wizard.
 	if len(args) == 0 {
+		if fi, err := os.Stdin.Stat(); err != nil || (fi.Mode()&os.ModeCharDevice) == 0 {
+			fmt.Fprintln(stderr, "no interactive TTY detected; pass headless flags")
+			return 1
+		}
 		res, err := launcher.Launch()

🤖 Prompt for AI Agents

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

In `@tui/interview_bootstrap_wizard.go` around lines 249 - 251, The wizard launch
is currently gated only by len(args) == 0 which can run the interactive
launcher.Launch() in non-interactive contexts; change the condition to require
an interactive TTY as well (e.g., check that stdin is a terminal via
os.Stdin/terminal/term IsTerminal equivalents) so that the code only calls
launcher.Launch() when len(args) == 0 AND stdin is a TTY; otherwise proceed with
the deterministic headless path. Ensure you update the condition around
launcher.Launch() and keep the existing dispatcher comment consistent.

src/services/interview/cohort/cohort-summary.ts-39-43 (1)

39-43: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate audit.json frontmatter shape before adding records

On Line 39, JSON.parse is cast directly to { frontmatter: AuditFrontmatter }. A parseable but malformed file (e.g., missing candidate or signed_off) will be pushed and can fail later in renderRow (Line 53+).

Suggested fix

-			const parsed = JSON.parse(body) as { frontmatter: AuditFrontmatter };
-			records.push({
-				frontmatter: parsed.frontmatter,
-				summaryPath: join(entry, "summary.md"),
-			});
+			const parsed = JSON.parse(body) as {
+				frontmatter?: Partial<AuditFrontmatter>;
+			};
+			const fm = parsed.frontmatter;
+			if (
+				!fm ||
+				typeof fm.candidate !== "string" ||
+				typeof fm.date !== "string" ||
+				typeof fm.signed_off !== "boolean"
+			) {
+				continue;
+			}
+			records.push({
+				frontmatter: fm as AuditFrontmatter,
+				summaryPath: join(entry, "summary.md"),
+			});

🤖 Prompt for AI Agents

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

In `@src/services/interview/cohort/cohort-summary.ts` around lines 39 - 43, The
code currently casts JSON.parse(body) to { frontmatter: AuditFrontmatter } and
unconditionally pushes it into records; instead validate the parsed object has a
frontmatter and required AuditFrontmatter fields (e.g., candidate, signed_off,
etc.) before pushing to records to avoid later failures in renderRow; locate the
parsing block around parsed = JSON.parse(body) and replace the blind cast with
runtime checks that parsed?.frontmatter exists and that the required keys are
present (or skip/log/throw for malformed audit.json), then push only when valid
and keep summaryPath as before.

src/services/interview/assess/audit-writer.ts-53-73 (1)

53-73: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Quote/escape YAML scalar frontmatter values

On Lines 55–73, frontmatter fields are emitted as raw strings. Values containing :, #, newlines, or quotes can corrupt parsing or inject unintended keys.

Suggested fix

 function yaml(value: AuditFrontmatter): string {
+	const q = (s: string): string => JSON.stringify(s);
 	const lines: string[] = ["---"];
-	lines.push(`tags: [${value.tags.join(", ")}]`);
-	lines.push(`candidate: ${value.candidate}`);
-	lines.push(`role: ${value.role}`);
-	lines.push(`date: ${value.date}`);
-	lines.push(`rubric_version: ${value.rubric_version}`);
-	lines.push(`rubric_mode: ${value.rubric_mode}`);
+	lines.push(`tags: [${value.tags.map(q).join(", ")}]`);
+	lines.push(`candidate: ${q(value.candidate)}`);
+	lines.push(`role: ${q(value.role)}`);
+	lines.push(`date: ${q(value.date)}`);
+	lines.push(`rubric_version: ${q(value.rubric_version)}`);
+	lines.push(`rubric_mode: ${q(value.rubric_mode)}`);
 	lines.push(`signed_off: ${value.signed_off}`);
 	if (value.recommendation !== undefined) {
-		lines.push(`recommendation: ${value.recommendation}`);
+		lines.push(`recommendation: ${q(value.recommendation)}`);
 	}
 	if (value.session_recording_url !== undefined) {
-		lines.push(`session_recording_url: ${value.session_recording_url}`);
+		lines.push(`session_recording_url: ${q(value.session_recording_url)}`);
 	}
 	if (value.session_platform !== undefined) {
-		lines.push(`session_platform: ${value.session_platform}`);
+		lines.push(`session_platform: ${q(value.session_platform)}`);
 	}
 	if (value.session_date !== undefined) {
-		lines.push(`session_date: ${value.session_date}`);
+		lines.push(`session_date: ${q(value.session_date)}`);
 	}

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/audit-writer.ts` around lines 53 - 73, The
yaml(value: AuditFrontmatter) function emits frontmatter scalars unescaped which
can break YAML when values contain characters like :, #, quotes, or newlines;
update it to sanitize/quote every scalar field (tags can remain array syntax but
each tag must be quoted) by adding a small helper (e.g. escapeYamlScalar) and
use it for candidate, role, date, rubric_version, rubric_mode, signed_off,
recommendation, session_recording_url, session_platform, and session_date; the
helper should wrap strings in double quotes and escape internal double quotes
and newlines (or alternatively call a stable YAML serializer) so the generated
frontmatter is always safe to parse.

src/services/interview/assess/grade-orchestrator.ts-194-209 (1)

194-209: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

candidate_id can collide and overwrite same-day runs

On Line 194, candidate_id is date-granular (<slug>-YYYY-MM-DD). Re-grading the same candidate on the same day will target the same default output directory and overwrite prior artifacts.

Suggested fix

+function nowIdSuffix(): string {
+	return new Date().toISOString().replace(/[:.]/g, "-");
+}
...
-			candidate_id: `${slug(input.candidateName)}-${todayIso()}`,
+			candidate_id: `${slug(input.candidateName)}-${nowIdSuffix()}`,

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/grade-orchestrator.ts` around lines 194 - 209,
The current candidate_id uses date granularity
(slug(input.candidateName)-todayIso()) which causes same-day runs to collide;
update the candidate_id generation in the result object to append a
higher-resolution unique suffix (e.g., timestamp or a UUID/nanoid) so it becomes
slug(input.candidateName)-todayIso()-<unique>, and keep the outputDir fallback
logic (the outputDir variable that uses input.outputDir ?? join(...,
result.candidate_id)) unchanged so that default directories are distinct per
run; locate the candidate_id assignment and modify it to include the unique
suffix (or call a helper like Date.now() or uuid()) so re-grades on the same day
do not overwrite prior artifacts.

🟡 Minor comments (15)

src/cli/index.ts-192-206 (1)

192-206: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Block interview in the report reserved-verb guard too.

After adding the top-level interview verb here, the misuse guard in report (Line 157) still only blocks doctor/setup. teamhero report interview now slips through as report args instead of producing the helpful redirect error.

Suggested fix

-			const subcommands = ["doctor", "setup"];
+			const subcommands = ["doctor", "setup", "interview"];

🤖 Prompt for AI Agents

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

In `@src/cli/index.ts` around lines 192 - 206, The new top-level command declared
via program.command("interview") is not included in the reserved-verb check
inside the report misuse guard, so "teamhero report interview" bypasses the
redirect; update the report command's misuse guard to include "interview"
alongside "doctor" and "setup" (i.e., add "interview" to the reserved verbs
list/check used by the report handler) so calls matching "interview" are
redirected the same way; locate the guard logic in the report command and adjust
its reservation check to include the "interview" verb (the new interview command
is created with program.command("interview") and launches spawnTui).

tests/unit/services/interview/bootstrap/project-validator.spec.ts-59-61 (1)

59-61: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove test-file line adjustment from fixture LOC calculation

The fixture includes 5 test-file lines in the currentLoc estimate, but the validator only counts source files (via the !TEST_NAME_PATTERN filter). This causes the padding calculation to undershoot by 5 lines.

-	const currentLoc = o.deepModuleCount * 100 + (o.withFailingTests ? 5 : 0);
+	const currentLoc = o.deepModuleCount * 100;

🤖 Prompt for AI Agents

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

In `@tests/unit/services/interview/bootstrap/project-validator.spec.ts` around
lines 59 - 61, The test fixture currently adds 5 lines for test files to the LOC
estimate (in the currentLoc calculation using o.deepModuleCount * 100 +
(o.withFailingTests ? 5 : 0)), but the validator filters out test files via
TEST_NAME_PATTERN and only counts source files, causing an undershoot; remove
the test-file adjustment so currentLoc is computed from o.deepModuleCount only
(e.g., drop the +(o.withFailingTests ? 5 : 0)) so padding/remaining calculations
align with the validator's source-file counting.

tui/interview_progress.go-87-92 (1)

87-92: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Set phase to running on update events to ensure correct visual state

When an update event arrives before a start event, the phase incorrectly displays as pending (○ glyph) despite progress being reported. Update the "update" case to transition from pending to running.

Proposed fix

 	case "update":
+		if m.status[evt.Step] == interviewPhasePending {
+			m.status[evt.Step] = interviewPhaseRunning
+		}
 		// keep running, just record message for tooltip-style display
 		if evt.Message != "" {
 			m.messages[evt.Step] = evt.Message
 		}

🤖 Prompt for AI Agents

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

In `@tui/interview_progress.go` around lines 87 - 92, The "update" event handler
currently only records tooltip messages (m.messages[evt.Step]) but doesn't
change the visual phase; modify the "update" case so that when an update arrives
it sets m.phase to PhaseRunning (or at least transitions from PhasePending to
PhaseRunning) so progress is shown as running; keep the existing message
recording logic for evt.Step and evt.Message and ensure you reference the
"update" case, m.phase, and the PhasePending/PhaseRunning constants when making
the change.

src/services/interview/assess/extractors/throughput.ts-13-13 (1)

13-13: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Test-run detection is narrower than the rest of the assessment pipeline.

TEST_RUN currently misses common runners (e.g., jest, vitest, cargo test, just test), so “Time to first test run” can be silently omitted for valid sessions.

Proposed fix

-const TEST_RUN = /^\s*(bun|npm|yarn|pnpm)\s+(run\s+)?test\b|^\s*go\s+test\b|^\s*pytest\b/;
+const TEST_RUN =
+	/^\s*((bun|npm|yarn|pnpm)\s+(run\s+)?test|go\s+test|pytest|jest|vitest|cargo\s+test|just\s+test)\b/;

🤖 Prompt for AI Agents

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

In `@src/services/interview/assess/extractors/throughput.ts` at line 13,
TEST_RUN's regex is too narrow and misses many common test runners; update the
TEST_RUN constant to include additional runners such as jest, vitest, mocha,
ava, tap, cypress, playwright, cargo (cargo test), and just (just test) while
preserving existing matches for package-manager-prefixed invocations
(bun|npm|yarn|pnpm with optional "run"). Modify the regex used in
src/services/interview/assess/extractors/throughput.ts (constant TEST_RUN) to
match both direct runner commands and package-manager-prefixed forms, and
consider making it case-insensitive or allowing optional whitespace so commands
like "npm run test", "yarn jest", "cargo test", and "just test" are all
detected.

teamhero-interview-kit/INTERVIEW_RULES.md-41-43 (1)

41-43: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add language identifiers to fenced code blocks.

These fences trigger markdownlint MD040; adding bash keeps docs lint-clean and consistent.

♻️ Proposed fix

-   ```
+   ```bash
    git push origin HEAD
    ```
...
-```
+```bash
 brew install asciinema

...
- +bash
sudo apt install asciinema # or your distro's package manager

...
-   ```
+   ```bash
   sudo apt update
   sudo apt install asciinema git
   ```

Also applies to: 50-52, 56-58, 68-71

🤖 Prompt for AI Agents

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

In `@teamhero-interview-kit/INTERVIEW_RULES.md` around lines 41 - 43, Update the
Markdown fenced code blocks in INTERVIEW_RULES.md to include the bash language
identifier: change ``` to ```bash for the snippets containing "git push origin
HEAD", "brew install asciinema", "sudo apt install asciinema", and the block
with "sudo apt update / sudo apt install asciinema git" (also apply to the other
occurrences you noted at lines 50-52, 56-58, 68-71) so the file passes
markdownlint MD040 and renders correctly.

skills/teamhero-interview/SKILL.md-50-52 (1)

50-52: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Specify language for CLI code fences.

Adding bash to these fenced blocks resolves markdownlint MD040 warnings.

♻️ Proposed fix

-```
+```bash
 teamhero interview bootstrap

...
- +bash
teamhero interview bootstrap --headless
--role --stack --domain --feature ""
--time-box
--mode-project A|B
--mode-analysis ai-assisted|human-only
--mode-rubric default|custom|default+jd
--output-dir
[--jd-path ] [--custom-prompt ""] [--role-title "<title>"]

...
-```
+```bash
teamhero interview grade --candidate "Jane Doe" --repo <url> \
  [--transcript <file>] [--interviewer-notes <file>] \
  [--session-recording-url <url>] [--session-platform zoom|teams|meet|other] \
  [--session-date YYYY-MM-DD] [--output-dir <path>]

...
- +bash
teamhero interview cohort --role [--order alphabetical|chronological]

Also applies to: 63-72, 81-86, 99-101

🤖 Prompt for AI Agents

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

In `@skills/teamhero-interview/SKILL.md` around lines 50 - 52, Add the language
tag "bash" to each CLI code fence in SKILL.md so the fenced blocks containing
commands like "teamhero interview bootstrap", "teamhero interview bootstrap
--headless ...", "teamhero interview grade --candidate ...", and "teamhero
interview cohort --role ..." start with ```bash instead of ``` to satisfy
markdownlint MD040; update every similar CLI fence (including the other
occurrences mentioned) consistently.

tui/interview_bootstrap_form.go-261-267 (1)

261-267: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Treat whitespace-only input as empty in nonEmpty validation.

Inputs like " " currently pass validation and can propagate invalid wizard values.

💡 Proposed fix

 import (
 	"errors"
 	"fmt"
+	"strings"
@@
 func nonEmpty(field string) func(string) error {
 	return func(s string) error {
-		if s == "" {
+		if strings.TrimSpace(s) == "" {
 			return fmt.Errorf("%s is required", field)
 		}
 		return nil
 	}
 }

🤖 Prompt for AI Agents

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

In `@tui/interview_bootstrap_form.go` around lines 261 - 267, The nonEmpty
validator currently treats whitespace-only strings as valid; update the nonEmpty
function (nonEmpty(field string) func(string) error) to trim whitespace (e.g.,
strings.TrimSpace) from the input before checking emptiness, returning
fmt.Errorf("%s is required", field) when the trimmed value is empty; add the
strings import if missing so the validator rejects inputs like "   ".

docs/2026-05-09-candidate-interview-grader-plan.md-273-273 (1)

273-273: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add language identifiers to fenced code blocks.

These unlabeled fences trigger MD040 and reduce syntax highlighting/readability in rendered docs.

Suggested fix pattern

-```
+```text
 ...
-```
+```

Use bash, markdown, typescript, or text as appropriate per block.

Also applies to: 296-296, 528-528, 541-541, 672-672, 681-681, 694-694

🤖 Prompt for AI Agents

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

In `@docs/2026-05-09-candidate-interview-grader-plan.md` at line 273, Several
fenced code blocks in docs/2026-05-09-candidate-interview-grader-plan.md are
unlabeled and trigger MD040; edit each fenced block (lines referenced in the
review: 273, 296, 528, 541, 672, 681, 694) and add an appropriate language
identifier after the opening backticks (e.g., ```bash, ```markdown,
```typescript, or ```text) to enable syntax highlighting—scan the surrounding
content to choose the correct language for each block and replace each opening
"```" with "```<language>" while leaving the closing "```" unchanged.

docs/2026-05-09-candidate-interview-grader-plan.md-533-535 (1)

533-535: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

The list-roles/list-candidates status is internally inconsistent.

Line 533-535 labels these verbs as v1.5, while Line 770+ says they are still deferred. Please align one source of truth so readers don’t misinterpret shipped scope.

Also applies to: 770-775

🤖 Prompt for AI Agents

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

In `@docs/2026-05-09-candidate-interview-grader-plan.md` around lines 533 - 535,
The document shows inconsistent status for the commands teamhero interview
list-roles and teamhero interview list-candidates (they're marked v1.5 in one
place but listed as deferred elsewhere); pick the correct status and make both
references consistent: update the v1.5 label if they are still deferred or
move/remove them from the deferred section if they are shipped, editing the
occurrences that mention teamhero interview list-roles and teamhero interview
list-candidates so the "shipped" label and the deferred list (the later block
that currently lists them) match exactly.

scripts/manual-test-interview.sh-62-63 (1)

62-63: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don’t silently swallow step failures in a smoke script.

Using || true at these execution points can hide real regressions and make the run look successful when core flows failed. Keep the script interactive, but surface non-zero exits explicitly.

Suggested adjustment

+run_step() {
+  if ! "$@"; then
+    local code=$?
+    echo "⚠ Command failed with exit code $code: $*" >&2
+    echo "   Continue only if this failure was expected." >&2
+  fi
+}
+
-"$BIN" interview bootstrap || true
+run_step "$BIN" interview bootstrap
 ...
-"$BIN" interview bootstrap \
+run_step "$BIN" interview bootstrap \
   --headless --no-confirm \
   --role manual-test-role --stack TypeScript --domain Manual \
   --feature "Verify the headless path still works" \
   --mode-project A --mode-analysis human-only --mode-rubric default \
-  --output-dir "$TMPDIR/manual-test-role" || true
+  --output-dir "$TMPDIR/manual-test-role"
 ...
-"$BIN" interview grade \
+run_step "$BIN" interview grade \
   --candidate "Manual Test Candidate" \
   --repo "https://example.com/fake-repo" \
-  --output-dir "$TMPDIR/grade-output" || true
+  --output-dir "$TMPDIR/grade-output"
 ...
-"$BIN" interview cohort --role manual-test-role || true
+run_step "$BIN" interview cohort --role manual-test-role

Also applies to: 68-73, 83-87, 99-99

🤖 Prompt for AI Agents

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

In `@scripts/manual-test-interview.sh` around lines 62 - 63, The script is
swallowing errors by appending "|| true" to key steps (e.g. the "$BIN interview
bootstrap" invocation and the other occurrences at the indicated ranges); remove
those "|| true" suffixes and instead check the command exit status: after
invoking "$BIN interview bootstrap" (and the other similar commands), capture
the exit code and if non-zero print a clear error message and either exit the
script or prompt the user to continue (interactive mode) so failures are
surfaced explicitly; look for occurrences of "$BIN" invocations with "|| true"
and replace them with explicit exit-code handling or an interactive confirmation
flow.

tui/interview_bootstrap_wizard.go-262-267 (1)

262-267: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard against nil Options before validation/run.

If a launcher returns Confirmed=true with Options=nil, this path can panic in validation or runner invocation. Add a defensive check and fail cleanly.

Proposed fix

 		if !res.Confirmed {
 			return 0
 		}
 		opts := res.Options
+		if opts == nil {
+			fmt.Fprintln(stderr, "wizard returned no options")
+			return 1
+		}
 		if msg := ValidateBootstrapOptions(opts); msg != "" {
 			fmt.Fprintln(stderr, msg)
 			return 1

🤖 Prompt for AI Agents

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

In `@tui/interview_bootstrap_wizard.go` around lines 262 - 267, The code assumes
res.Options is non-nil before calling ValidateBootstrapOptions and runner.Run;
add a nil check for opts (res.Options) right after assigning opts := res.Options
and before calling ValidateBootstrapOptions: if opts == nil {
fmt.Fprintln(stderr, "no options provided"); return 1 }, so the function fails
cleanly instead of panicking; keep using stderr/stdout and still call
ValidateBootstrapOptions and runner.Run only when opts is non-nil.

tui/interview_grade.go-32-39 (1)

32-39: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Handle missing flag values before assignment in ParseGradeFlags.

When a flag is followed by another flag token, the parser currently treats that token as a value and returns a misleading later error. Add a guard so the parser returns the correct “requires a value” error immediately.

Proposed fix

 			if i+1 >= len(args) {
 				return nil, fmt.Sprintf("flag %s requires a value", a)
 			}
 			val := args[i+1]
+			if strings.HasPrefix(val, "--") {
+				return nil, fmt.Sprintf("flag %s requires a value", a)
+			}
 			switch a {

🤖 Prompt for AI Agents

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

In `@tui/interview_grade.go` around lines 32 - 39, In ParseGradeFlags: the current
logic only checks bounds for args[i+1] but can treat a subsequent flag token as
a value; update the guard inside the case handling ("--repo", "--candidate",
"--transcript", "--interviewer-notes", "--session-recording-url",
"--session-platform", "--session-date", "--output-dir", "--local-repo-path") to
not only ensure i+1 < len(args) but also verify args[i+1] is not a flag (e.g.,
starts with "-"); if it is a flag, return the same "flag %s requires a value"
error immediately instead of assigning val and letting later code fail. Locate
this logic in ParseGradeFlags and add the extra check before setting val :=
args[i+1].

tui/interview_cohort.go-24-37 (1)

24-37: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject flag-like tokens as missing values in ParseCohortFlags.

ParseCohortFlags currently accepts another flag token as a value (for example, --role --order ...) and then fails later with a misleading error. Validate the consumed value before assigning it.

Proposed fix

 		case "--role", "--role-dir", "--order":
 			if i+1 >= len(args) {
 				return nil, fmt.Sprintf("flag %s requires a value", a)
 			}
 			val := args[i+1]
+			if strings.HasPrefix(val, "--") {
+				return nil, fmt.Sprintf("flag %s requires a value", a)
+			}
 			switch a {

🤖 Prompt for AI Agents

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

In `@tui/interview_cohort.go` around lines 24 - 37, In ParseCohortFlags, the code
currently assigns args[i+1] to opts.Role/RoleDir/Order even if that token is
another flag (e.g. "--role --order"), causing confusing errors later; fix it by
validating val before assignment: after val := args[i+1], check if
strings.HasPrefix(val, "-") (or specifically "--") and if so return the same
missing-value error (e.g. fmt.Sprintf("flag %s requires a value", a)); otherwise
proceed to set opts.Role, opts.RoleDir or opts.Order and increment i. Ensure to
import strings if needed.

src/services/interview/cohort/cohort-summary.ts-42-42 (1)

42-42: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use POSIX path joining for markdown links

On Line 42, join(entry, "summary.md") may produce \ on Windows, which can render as broken/awkward links in markdown.

Suggested fix

-import { join } from "node:path";
+import { join, posix } from "node:path";
...
-				summaryPath: join(entry, "summary.md"),
+				summaryPath: posix.join(entry, "summary.md"),

🤖 Prompt for AI Agents

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

In `@src/services/interview/cohort/cohort-summary.ts` at line 42, The summaryPath
construction uses path.join(entry, "summary.md") which will produce backslashes
on Windows and break markdown links; change it to use POSIX joining instead
(e.g., use path.posix.join or equivalent) when building summaryPath in
cohort-summary.ts so the resulting link always contains forward slashes; update
imports/use of the path module accordingly and ensure the variable name
summaryPath still receives the new POSIX-joined value (reference symbols:
summaryPath, join, entry).

tui/interview_bootstrap.go-127-129 (1)

127-129: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate that --jd-path actually exists in headless mode

Lines 127–129 only require a non-empty value. A typo/path miss will pass validation and later run without JD context.

Suggested fix

 	if opts.ModeRubric == "default+jd" && strings.TrimSpace(opts.JDPath) == "" {
 		return "--mode-rubric 'default+jd' requires --jd-path"
 	}
+	if opts.ModeRubric == "default+jd" {
+		info, err := os.Stat(opts.JDPath)
+		if err != nil || info.IsDir() {
+			return "--jd-path must point to an existing file"
+		}
+	}
 	return ""

🤖 Prompt for AI Agents

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

In `@tui/interview_bootstrap.go` around lines 127 - 129, The validation in the
condition checking opts.ModeRubric and opts.JDPath only ensures that --jd-path
is not empty, but it should also verify that the specified path actually exists
to prevent runtime errors in headless mode. Update the code to check if the file
at opts.JDPath exists using appropriate file existence checking methods (e.g.,
os.Stat in Go) and return an error if the file does not exist alongside the
current validation in the logic handling the mode-checking.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c9c03249-2c4d-43c6-ac4c-46fe9bc90861

📥 Commits

Reviewing files that changed from the base of the PR and between aba1227 and 106d3ac.

📒 Files selected for processing (74)

• README.md
• docs/2026-05-09-candidate-interview-grader-plan.md
• docs/interview-classification-rationale.md
• docs/interview-rubric.md
• scripts/manual-test-interview.sh
• scripts/run-interview-bootstrap.ts
• scripts/run-interview-cohort.ts
• scripts/run-interview-grade.ts
• scripts/run-tests.sh
• skills/teamhero-interview/SKILL.md
• src/cli/index.ts
• src/services/interview/assess/ai-observer.ts
• src/services/interview/assess/audit-writer.ts
• src/services/interview/assess/collectors/asciinema.ts
• src/services/interview/assess/collectors/git-history.ts
• src/services/interview/assess/collectors/jsonl-log.ts
• src/services/interview/assess/collectors/transcript.ts
• src/services/interview/assess/extractors/risk-awareness.ts
• src/services/interview/assess/extractors/test-pass.ts
• src/services/interview/assess/extractors/throughput.ts
• src/services/interview/assess/extractors/verification.ts
• src/services/interview/assess/grade-orchestrator.ts
• src/services/interview/assess/types.ts
• src/services/interview/bootstrap/openai-generator-client.ts
• src/services/interview/bootstrap/orchestrator.ts
• src/services/interview/bootstrap/project-generator.ts
• src/services/interview/bootstrap/project-validator.ts
• src/services/interview/bootstrap/role-config.ts
• src/services/interview/cohort/cohort-summary.ts
• src/services/interview/shared/events.ts
• src/services/interview/shared/rubric.ts
• teamhero-interview-kit/.claude/CLAUDE.md
• teamhero-interview-kit/.claude/settings.json
• teamhero-interview-kit/INTERVIEW_RULES.md
• teamhero-interview-kit/PRIVACY_RELEASE.md
• teamhero-interview-kit/RUBRIC_OVERVIEW.md
• teamhero-interview-kit/end.sh
• teamhero-interview-kit/lib/privacy-gate.sh
• teamhero-interview-kit/start.sh
• tests/unit/cli/interview.spec.ts
• tests/unit/services/interview/assess/ai-observer.spec.ts
• tests/unit/services/interview/assess/audit-writer.spec.ts
• tests/unit/services/interview/assess/collectors.spec.ts
• tests/unit/services/interview/assess/extractors.spec.ts
• tests/unit/services/interview/assess/grade-orchestrator.spec.ts
• tests/unit/services/interview/bootstrap/orchestrator.spec.ts
• tests/unit/services/interview/bootstrap/project-generator.spec.ts
• tests/unit/services/interview/bootstrap/project-validator.spec.ts
• tests/unit/services/interview/bootstrap/role-config.spec.ts
• tests/unit/services/interview/cohort/cohort-summary.spec.ts
• tests/unit/services/interview/cohort/skill.spec.ts
• tests/unit/services/interview/kit/kit-smoke.spec.ts
• tests/unit/services/interview/kit/privacy-gate.spec.ts
• tests/unit/services/interview/shared/events.spec.ts
• tests/unit/services/interview/shared/rubric.spec.ts
• tui/interview.go
• tui/interview_bootstrap.go
• tui/interview_bootstrap_form.go
• tui/interview_bootstrap_test.go
• tui/interview_bootstrap_wizard.go
• tui/interview_bootstrap_wizard_test.go
• tui/interview_cohort.go
• tui/interview_cohort_test.go
• tui/interview_grade.go
• tui/interview_grade_test.go
• tui/interview_preview.go
• tui/interview_preview_test.go
• tui/interview_progress.go
• tui/interview_progress_test.go
• tui/interview_styles.go
• tui/interview_styles_test.go
• tui/interview_test.go
• tui/main.go
• tui/manual_test_script_test.go

[coderabbitai]

Actionable comments posted: 18

♻️ Duplicate comments (1)

src/services/interview/review/review-orchestrator.ts (1)

45-47: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Use execFileSync/arg array to remove command-injection risk.

Line 45 builds a shell command with interpolated repoUrl/destDir; this allows shell-breaking payloads. Use argument-array execution instead.

Suggested fix

-import { execSync } from "node:child_process";
+import { execFileSync } from "node:child_process";
...
 const defaultCloner: Cloner = (repoUrl, destDir) => {
-	execSync(`git clone --depth=50 '${repoUrl}' '${destDir}'`, {
+	execFileSync("git", ["clone", "--depth=50", repoUrl, destDir], {
 		stdio: "inherit",
 	});
 };

#!/bin/bash
# Verify unsafe shell-interpolated exec usage in the reviewed file.
rg -nP 'execSync\s*\(\s*`git clone' src/services/interview/review/review-orchestrator.ts
rg -nP '\bexecSync\s*\(' src/services/interview/review/review-orchestrator.ts

🤖 Prompt for AI Agents

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

In `@src/services/interview/review/review-orchestrator.ts` around lines 45 - 47,
The git clone call in review-orchestrator.ts uses execSync with a
shell-interpolated command (execSync(`git clone --depth=50 '${repoUrl}'
'${destDir}'`, ...)) which is vulnerable to command injection; replace it with a
non-shell, argument-array style call (e.g., execFileSync or spawnSync) passing
["git", "clone", "--depth=50", repoUrl, destDir] and preserve stdio: "inherit"
semantics; ensure you remove the shell=true behavior, pass repoUrl and destDir
as separate args, and adjust any error handling around the exec in the
function/method that contains this execSync invocation.

🧹 Nitpick comments (1)

tests/unit/cli/unknown-subcommand.spec.ts (1)

75-100: ⚡ Quick win

Known-subcommand guard test is slightly environment-dependent.

Line 90 drives run(..., sub, "--help") into TUI passthrough, so this “guard-stage” unit test can depend on local binary/runtime state. Consider isolating guard logic from TUI execution in this test path to keep it fully deterministic.

🤖 Prompt for AI Agents

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

In `@tests/unit/cli/unknown-subcommand.spec.ts` around lines 75 - 100, The test is
hitting TUI passthrough when calling run([... sub, "--help"]) making it
environment-dependent; modify the test to isolate the guard by stubbing out the
TUI execution in the deps returned by makeDeps (e.g., set deps.spawnTui to a
no-op or a function that throws a deterministic sentinel) so run() exercises
only the guard logic and never invokes the real TUI, keep the existing
errorSpy/exitSpy handling and assertions around run and unknown-subcommand
messages.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/2026-05-09-candidate-interview-reviewer-plan.md`:
- Line 273: The markdown contains unannotated fenced code blocks (``` ) that
trigger MD040; update each fenced block to include a language identifier (e.g.,
```bash, ```text, ```markdown) so tooling can correctly lint and highlight them.
Locate every triple-backtick fence in the document (all occurrences of ``` ) and
replace the opening fence with an appropriate language token consistent across
similar examples (use bash for shell commands, text for plain output, markdown
for nested markdown examples, etc.), and ensure the same convention is applied
to the other occurrences noted in the review.
- Line 757: Update the stale module path in the implementation progress table:
replace the incorrect `src/services/interview/{bootstrap,assess,cohort,shared}/`
reference with the correct module path that uses `review` (e.g.,
`src/services/interview/{bootstrap,assess,cohort,review}/`) so it matches the
rest of the document and the shipped flow; ensure the `teamhero interview` CLI
subcommand line and any adjacent references use the `review` segment
consistently.

In `@scripts/manual-test-interview.sh`:
- Around line 83-86: The review invocation using "$BIN" interview review
currently omits the analysis mode flag so it may call OpenAI; modify the command
calling "$BIN" interview review (the block with --candidate and --repo) to
include --mode-analysis human-only so the script enforces human-only analysis
and avoids any API usage/cost paths.

In `@scripts/run-interview-review.ts`:
- Around line 53-56: The CLI currently only checks that at least one of
flags.repo or flags.localRepoPath is provided, but allows both; change the check
to require exactly one (XOR). Replace the existing if-condition with a test that
fails when both are present or both absent (e.g. if ((!!flags.repo) ===
(!!flags.localRepoPath)) { ... }), and update the error message to state
"Provide exactly one of --repo <url> or --local-repo-path <dir>" before calling
process.exit(1); keep references to flags.repo and flags.localRepoPath so
reviewers can find the change.

In `@skills/teamhero-interview/SKILL.md`:
- Around line 78-86: The heading "teamhero interview review <repo-url>" is
inconsistent with the actual CLI invocation which uses flags; update the
SKILL.md heading to match the documented usage by changing it to something like
"teamhero interview review --repo <url>" (or show both forms: positional and
flag) and ensure the examples and paragraph describe the same form; edit the
heading text around the `teamhero interview review` section and any nearby
examples so they reference the `--repo <url>` flag (and other flags)
consistently with the code examples and the documented invocation.
- Around line 50-52: Add the missing fenced-code language identifiers by
changing the triple-backtick fences for the command snippets to specify "bash"
(e.g., the blocks containing "teamhero interview bootstrap", "teamhero interview
bootstrap --headless \", "teamhero interview review --candidate \"Jane Doe\"
--repo <url> \", "teamhero interview cohort --role <slug> [--order
alphabetical|chronological]" and the other similar command blocks at lines
referenced) so each code fence begins with ```bash to satisfy markdownlint
MD040; update every occurrence noted (around lines 50-52, 63-72, 81-86, 99-101)
consistently.

In `@src/cli/index.ts`:
- Around line 229-255: The nested-subcommand rejection logic was left using a
hardcoded allowlist (e.g., only "doctor"/"setup") so "teamhero report interview"
bypasses the guard; update the nested-subcommand check to reuse the shared
KNOWN_SUBCOMMANDS constant instead of the ad-hoc array: locate the guard in the
run function that inspects the first/second args and replace the hardcoded
allowed-subcommands with KNOWN_SUBCOMMANDS (so validations for
first/second-level subcommands remain consistent).

In `@src/services/interview/review/ai-observer.ts`:
- Around line 192-201: The current rejectIfScored() string-scans JSON which
false-positives when forbidden names appear inside string values; instead
parse/inspect the response structurally and recursively check object keys
against FORBIDDEN_FIELDS. Update rejectIfScored to: safely parse or coerce the
incoming response to an object (handle non-objects by returning), walk nested
objects and arrays, and if any object has a property name equal to one of
FORBIDDEN_FIELDS (use direct key comparison, not regex), throw the existing
Error message; also catch and rethrow/handle JSON parse errors consistently so
you don't silently ignore malformed input.

In `@src/services/interview/review/audit-writer.ts`:
- Around line 53-76: The yaml function is vulnerable because it interpolates
user-adjacent strings (AuditFrontmatter fields like candidate, role, tags,
recommendation, session_* and session URLs) directly into YAML, allowing invalid
frontmatter or injected keys; fix by switching from manual string interpolation
to safe YAML serialization (e.g., use a YAML library like js-yaml oryaml and
call safeDump/safeLoad for the AuditFrontmatter object) or, if you must
hand-roll it, ensure every scalar is properly quoted and escaped (escape
backslashes, quotes and newlines, emit tags as a YAML sequence not an unquoted
bracket list, and only emit optional keys when present). Update the yaml
function to construct a plain JS object from AuditFrontmatter and pass it to the
serializer (or replace each push with properly quoted/escaped values and tags
emitted as a sequence) so all fields are safely encoded.

In `@src/services/interview/review/collectors/asciinema.ts`:
- Around line 67-86: The code in the event-processing block for evt.data assumes
a single-character payload and misses cases where evt.data contains multiple
characters (e.g., "npm test\r"); update the logic in the handler around
variables buffer, lastKeyDelta, evt.delta, baseEpoch and commands so you iterate
over each character in evt.data (for example for (const ch of evt.data)) and
apply the same branches: treat '\r' and '\n' as Enter (trim buffer, push a
command with timestamp = isoFromUnix(baseEpoch + evt.delta) and
pauseSecondsBeforeEnter = Math.max(0, evt.delta - lastKeyDelta)), treat '�' and
'\b' as backspace (slice buffer and set lastKeyDelta), treat printable chars
(charCode >= 32 or '\t') as append to buffer and set lastKeyDelta; ensure buffer
is reset after pushing a command so subsequent characters are processed
correctly.

In `@src/services/interview/review/collectors/git-history.ts`:
- Around line 14-18: The realRunner implementation currently builds a shell
command string and calls execSync (risking injection/quoting bugs); change it to
call execFileSync with the program "git" and pass the args array directly (i.e.
execFileSync("git", args, { cwd, encoding: "utf8" })) so you remove the manual
quoting/map and avoid shell parsing; update any type/return handling in the
GitRunner wrapper accordingly.

In `@src/services/interview/review/collectors/transcript.ts`:
- Around line 27-37: Validate the sessionStartIso at the start of toIsoFromHMS:
parse it (e.g., new Date(sessionStartIso)) and check
isFinite(date.getTime())/isNaN before using getTime(); if invalid, throw a clear
Error mentioning "invalid sessionStartIso" and include the raw value. Update the
same validation pattern where sessionStartIso is used later in this file (the
second occurrence around lines 51-52) so both code paths fail fast with the same
clear error message; reference function toIsoFromHMS and the other usage site to
locate and mirror the change.

In `@src/services/interview/review/extractors/test-pass.ts`:
- Around line 20-34: The spawnSync invocations in test-pass.ts that run the Go
and Bun tests (the calls to spawnSync("go", ["test", "./..."], { cwd: repoDir,
encoding: "utf8" }) and spawnSync("bun", ["test"], { cwd: repoDir, encoding:
"utf8" })) lack timeout and maxBuffer safeguards; update both calls to include
sensible timeout and maxBuffer options (e.g., timeout: 120000, maxBuffer: 10 *
1024 * 1024) so the subprocesses cannot hang or overflow stdout/stderr, keep the
rest of the options (cwd, encoding) and ensure the rest of the code that reads
r.stdout/r.stderr and computes passed/failed via countMatches remains unchanged.

In `@src/services/interview/review/extractors/verification.ts`:
- Around line 58-71: The interleaving count currently treats any adjacent
prompt→command pair in the merged array as "within 30 seconds" but doesn't
enforce the 30s boundary; update the loop that computes interleavedAfterPrompt
(which iterates merged, uses cur/prev and checks cur.type === "command" &&
prev.type === "prompt") to also compute the timestamp difference (e.g., parse
cur.timestamp and prev.timestamp to milliseconds via Date) and only increment
interleavedAfterPrompt when the difference is <= 30000 ms; keep the existing
sort by timestamp and ensure you handle timestamp parsing robustly (use
getTime() or valueOf()) so only prompt→test pairs within the 30-second window
are counted.

In `@src/services/interview/review/review-orchestrator.ts`:
- Around line 83-85: Replace the ad-hoc date logic: remove todayIso() and any
manual string concatenation like `${sessionDate}T09:00:00Z`; instead import and
use the date-utils helpers (resolveStartISO, resolveEndISO, resolveEndEpochMs,
formatDateUTC) so timezone-safe boundaries and display formatting are
consistent; update the places referenced (the todayIso function and the
constructions around sessionDate at the other locations) to call
resolveStartISO/resolveEndISO for interval boundaries and formatDateUTC for any
display strings or ISO formatting.
- Around line 39-42: The Cloner and ReviewDependencies types are port interfaces
and must be moved into the shared core types file; remove their local
declarations (e.g., the Cloner type and the ReviewDependencies type block) and
declare them in src/core/types.ts instead, then import and use those types in
review-orchestrator.ts (and any other files that used the local definitions) so
all boundary port interfaces live in the central types module and usages
reference the imported Cloner and ReviewDependencies symbols.

In `@tests/unit/services/interview/cohort/cohort-summary.spec.ts`:
- Around line 96-109: The test currently asserts status words only; instead,
assert the actual glyph characters so the UI signal can't be removed silently:
update the test using the existing helpers (makeCandidate, loadCohort,
renderCohortSummary) to check for the exact glyphs "✅" for reviewed Alice and
"⏳" for pending Bob (e.g. expect(body).toMatch(/Alice.*✅/) and
expect(body).toMatch(/Bob.*⏳/)), while retaining the existing checks for
"Reviewed"/"Pending" and "Hire" to keep coverage of both glyph and text.

In `@tests/unit/services/interview/review/ai-observer.spec.ts`:
- Around line 123-132: The test currently only checks
OBSERVATION_RESPONSE_SCHEMA.additionalProperties at the top level; add a
recursive schema traversal (e.g., a helper named assertNoAdditionalProperties or
traverseSchema) that walks OBSERVATION_RESPONSE_SCHEMA and asserts
additionalProperties === false for every object schema node it encounters
(inspect properties, patternProperties, additionalProperties, items,
allOf/anyOf/oneOf, not, definitions/$defs, etc.), and also ensure the banned
field names ("score","weighted_total","raw_total","band","signal_count") are not
present anywhere by checking keys at each visited node; call this helper from
the "declares strict json_schema..." test to harden the assertion.

---

Duplicate comments:
In `@src/services/interview/review/review-orchestrator.ts`:
- Around line 45-47: The git clone call in review-orchestrator.ts uses execSync
with a shell-interpolated command (execSync(`git clone --depth=50 '${repoUrl}'
'${destDir}'`, ...)) which is vulnerable to command injection; replace it with a
non-shell, argument-array style call (e.g., execFileSync or spawnSync) passing
["git", "clone", "--depth=50", repoUrl, destDir] and preserve stdio: "inherit"
semantics; ensure you remove the shell=true behavior, pass repoUrl and destDir
as separate args, and adjust any error handling around the exec in the
function/method that contains this execSync invocation.

---

Nitpick comments:
In `@tests/unit/cli/unknown-subcommand.spec.ts`:
- Around line 75-100: The test is hitting TUI passthrough when calling run([...
sub, "--help"]) making it environment-dependent; modify the test to isolate the
guard by stubbing out the TUI execution in the deps returned by makeDeps (e.g.,
set deps.spawnTui to a no-op or a function that throws a deterministic sentinel)
so run() exercises only the guard logic and never invokes the real TUI, keep the
existing errorSpy/exitSpy handling and assertions around run and
unknown-subcommand messages.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f1afff1b-ff4d-4824-b702-affafb49c191

📥 Commits

Reviewing files that changed from the base of the PR and between 106d3ac and ffe507a.

📒 Files selected for processing (37)

• .coderabbit.yaml
• README.md
• docs/2026-05-09-candidate-interview-reviewer-plan.md
• scripts/manual-test-interview.sh
• scripts/run-interview-review.ts
• skills/teamhero-interview/SKILL.md
• src/cli/index.ts
• src/services/interview/bootstrap/openai-generator-client.ts
• src/services/interview/review/ai-observer.ts
• src/services/interview/review/audit-writer.ts
• src/services/interview/review/collectors/asciinema.ts
• src/services/interview/review/collectors/git-history.ts
• src/services/interview/review/collectors/jsonl-log.ts
• src/services/interview/review/collectors/transcript.ts
• src/services/interview/review/extractors/risk-awareness.ts
• src/services/interview/review/extractors/test-pass.ts
• src/services/interview/review/extractors/throughput.ts
• src/services/interview/review/extractors/verification.ts
• src/services/interview/review/review-orchestrator.ts
• src/services/interview/review/types.ts
• tests/unit/cli/unknown-subcommand.spec.ts
• tests/unit/services/discrepancy.spec.ts
• tests/unit/services/interview/cohort/cohort-summary.spec.ts
• tests/unit/services/interview/cohort/skill.spec.ts
• tests/unit/services/interview/review/ai-observer.spec.ts
• tests/unit/services/interview/review/audit-writer.spec.ts
• tests/unit/services/interview/review/collectors.spec.ts
• tests/unit/services/interview/review/extractors.spec.ts
• tests/unit/services/interview/review/review-orchestrator.spec.ts
• tui/interview.go
• tui/interview_progress.go
• tui/interview_review.go
• tui/interview_review_test.go
• tui/interview_styles.go
• tui/interview_test.go
• tui/main.go
• tui/manual_test_script_test.go

✅ Files skipped from review due to trivial changes (3)

• .coderabbit.yaml
• README.md
• tests/unit/services/interview/review/extractors.spec.ts

🚧 Files skipped from review as they are similar to previous changes (4)

• tui/main.go
• tests/unit/services/interview/cohort/skill.spec.ts
• tui/interview_styles.go
• tui/interview_progress.go

[coderabbitai]

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.

---

Generating unit tests... This may take up to 20 minutes.

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (1)

src/services/interview/bootstrap/project-generator.ts (1)

95-99: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Top-level delete guard is ineffective for paths like /usr and /etc.

Line 98 checks parent === abs, which only catches filesystem roots. It does not block top-level directories, so clearOutputDir() can still recursively delete unsafe system paths if misconfigured.

🔧 Suggested fix

-import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path";
+import { dirname, isAbsolute, join, parse, relative, resolve, sep } from "node:path";
...
 function assertSafeToClear(outputDir: string): void {
 	const abs = resolve(outputDir);
-	const root = resolve(abs, "/");
+	const root = parse(abs).root;
 	if (abs === root || abs === sep) {
 		throw new Error(`Refusing to clear filesystem root: ${abs}`);
 	}
 	const home = homedir();
 	if (home && abs === resolve(home)) {
 		throw new Error(`Refusing to clear home directory: ${abs}`);
 	}
-	// Refuse a top-level path like /usr, /etc, /home — anything where the
-	// path has no parent beyond the root.
-	const parent = dirname(abs);
-	if (parent === abs) {
-		throw new Error(`Refusing to clear root-level path: ${abs}`);
+	// Refuse top-level paths like /usr, /etc, /home.
+	if (dirname(abs) === root) {
+		throw new Error(`Refusing to clear top-level path: ${abs}`);
 	}
 }

🤖 Prompt for AI Agents

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

In `@src/services/interview/bootstrap/project-generator.ts` around lines 95 - 99,
The current check in clearOutputDir uses dirname(abs) and only throws when
parent === abs, which misses top-level children like "/usr" (whose parent is "/"
not equal to abs); update the guard to also detect paths directly under the
filesystem root by comparing parent to the path root: require
path.parse(abs).root and throw if parent === abs || parent ===
path.parse(abs).root; reference the clearOutputDir function and the local
variables abs and parent and use path.parse(abs).root to identify root-level
parents.

🧹 Nitpick comments (1)

src/services/interview/review/extractors/test-pass.ts (1)

14-17: ⚡ Quick win

Move TestRunner port type to src/core/types.ts.

This defines a new port-like interface locally; per repo architecture rules, keep port interfaces centralized in core types and import it here.

As per coding guidelines, "ALL port interfaces must be defined in src/core/types.ts — never create new types files".

🤖 Prompt for AI Agents

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

In `@src/services/interview/review/extractors/test-pass.ts` around lines 14 - 17,
The type alias TestRunner is a port interface that must be centralized instead
of being defined locally; remove the local export of TestRunner and instead add
the identical exported type declaration to the core types module (types.ts),
then import { TestRunner } into this file and use it in place of the removed
local type; ensure the signature stays { readonly passed: number; readonly
failed: number; readonly output: string } and that the core types module exports
it for other modules to consume.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@src/services/interview/bootstrap/project-generator.ts`:
- Around line 27-33: Move the GeneratorClient interface declaration into the
shared core types module (the central core types file) so it lives with other
port interfaces; export it from there and remove the local declaration in
project-generator.ts. Update all files that reference GeneratorClient (and
ensure GeneratedProject and RoleConfig are imported or re-exported from the core
types file) to import the interface from the shared core types module instead of
the local service file. Ensure the generate method signature and readonly
modifiers remain identical when relocating.
- Around line 109-120: The copyDir function currently uses statSync(s) which
follows symlinks; change it to use lstatSync(s) and explicitly skip entries
where lstatSync(s).isSymbolicLink() to avoid copying files outside
kitTemplateDir or entering symlink loops. In function copyDir, replace statSync
with lstatSync, add a guard like "if (st.isSymbolicLink()) continue", and keep
the existing directory handling (mkdirSync + recursive copy) and file writing
(mkdirSync(dirname(d)) + writeFileSync) for non-symlink entries.

In `@src/services/interview/review/audit-writer.ts`:
- Around line 57-68: The yamlScalar function detects values needing quoting but
only escapes backslashes and double quotes, so control characters like newline,
carriage return and tab remain raw and produce invalid YAML; update yamlScalar
to additionally replace control characters (e.g. \n, \r, \t, \b, \f, and any
other non-printable controls you expect) with their corresponding escape
sequences before wrapping in quotes (while still escaping backslashes and double
quotes), ensuring the returned double-quoted scalar contains valid YAML escape
sequences; modify the replacement logic used when building the escaped variable
in yamlScalar to run a single pass that maps those control characters to "\\n",
"\\r", "\\t", "\\b", "\\f", etc., then return the quoted string.
- Around line 70-76: yamlTag currently detects whitespace (including
newlines/tabs) but only escapes backslashes and double quotes; update yamlTag to
additionally replace control whitespace characters with proper escape sequences
before wrapping in quotes: after computing escaped = value.replace(/\\/g,
"\\\\").replace(/"/g, '\\"'), also replace newline (\n) with "\\n", carriage
return (\r) with "\\r", and tab (\t) with "\\t" (and any other control chars you
deem necessary) so the returned quoted string is fully escaped; keep the
existing regex and return logic but perform these extra replacements on the
escaped variable inside the yamlTag function.

In `@src/services/interview/review/collectors/git-history.ts`:
- Around line 27-31: The current try/catch around runner(["log",
`--format=${FORMAT}`, "--numstat"], repoDir) swallows all errors and returns [];
change it to only convert known benign git cases (e.g., "does not have any
commits", "unknown revision", or specific exit codes from git that indicate
empty history) into [], and rethrow any other exceptions so real failures
(permissions, broken git, unexpected runtime errors) surface; specifically wrap
the call to runner in a try, inspect the thrown error's message/exit code/stderr
and return [] only for the recognized benign messages, otherwise throw the
original error so higher-level handlers (in-report placeholder / appendix / CLI
stderr) can handle it.

---

Duplicate comments:
In `@src/services/interview/bootstrap/project-generator.ts`:
- Around line 95-99: The current check in clearOutputDir uses dirname(abs) and
only throws when parent === abs, which misses top-level children like "/usr"
(whose parent is "/" not equal to abs); update the guard to also detect paths
directly under the filesystem root by comparing parent to the path root: require
path.parse(abs).root and throw if parent === abs || parent ===
path.parse(abs).root; reference the clearOutputDir function and the local
variables abs and parent and use path.parse(abs).root to identify root-level
parents.

---

Nitpick comments:
In `@src/services/interview/review/extractors/test-pass.ts`:
- Around line 14-17: The type alias TestRunner is a port interface that must be
centralized instead of being defined locally; remove the local export of
TestRunner and instead add the identical exported type declaration to the core
types module (types.ts), then import { TestRunner } into this file and use it in
place of the removed local type; ensure the signature stays { readonly passed:
number; readonly failed: number; readonly output: string } and that the core
types module exports it for other modules to consume.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 20c0ad49-c16c-47ab-bd28-dd3a3617b024

📥 Commits

Reviewing files that changed from the base of the PR and between ffe507a and 790b9e8.

📒 Files selected for processing (25)

• scripts/run-interview-bootstrap.ts
• scripts/run-interview-review.ts
• src/services/interview/bootstrap/orchestrator.ts
• src/services/interview/bootstrap/project-generator.ts
• src/services/interview/bootstrap/project-validator.ts
• src/services/interview/bootstrap/role-config.ts
• src/services/interview/cohort/cohort-summary.ts
• src/services/interview/review/audit-writer.ts
• src/services/interview/review/collectors/git-history.ts
• src/services/interview/review/collectors/jsonl-log.ts
• src/services/interview/review/collectors/transcript.ts
• src/services/interview/review/extractors/test-pass.ts
• src/services/interview/review/extractors/verification.ts
• src/services/interview/review/review-orchestrator.ts
• src/services/interview/shared/events.ts
• teamhero-interview-kit/lib/privacy-gate.sh
• teamhero-interview-kit/start.sh
• tests/unit/services/interview/bootstrap/project-validator.spec.ts
• tests/unit/services/interview/review/audit-writer.spec.ts
• tests/unit/services/interview/review/review-orchestrator.spec.ts
• tui/interview_bootstrap.go
• tui/interview_bootstrap_form.go
• tui/interview_bootstrap_wizard.go
• tui/interview_bootstrap_wizard_test.go
• tui/interview_progress.go

🚧 Files skipped from review as they are similar to previous changes (17)

• src/services/interview/shared/events.ts
• src/services/interview/review/collectors/jsonl-log.ts
• src/services/interview/review/collectors/transcript.ts
• tests/unit/services/interview/bootstrap/project-validator.spec.ts
• scripts/run-interview-review.ts
• teamhero-interview-kit/lib/privacy-gate.sh
• tests/unit/services/interview/review/review-orchestrator.spec.ts
• tests/unit/services/interview/review/audit-writer.spec.ts
• tui/interview_progress.go
• tui/interview_bootstrap_wizard.go
• src/services/interview/bootstrap/role-config.ts
• tui/interview_bootstrap.go
• src/services/interview/cohort/cohort-summary.ts
• tui/interview_bootstrap_form.go
• tui/interview_bootstrap_wizard_test.go
• src/services/interview/bootstrap/project-validator.ts
• src/services/interview/review/review-orchestrator.ts

[coderabbitai]

✅ Unit tests committed locally. Commit: cf8229bec27921480f52ef5cbb9385964b7e1eb0

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

tests/unit/services/interview/cohort/cohort-summary-edge-cases.spec.ts (3)

156-173: ⚡ Quick win

Chronological test does not actually prove session_date precedence.

Right now date and session_date are identical for both candidates, so sorting by date would also pass. Make them intentionally conflict so this test catches regressions.

Suggested test hardening

 			makeCandidate(dir, "carol", {
 				candidate: "Carol",
-				date: "2026-05-20",
+				date: "2026-04-01",
 				session_date: "2026-05-20",
 			});
 			makeCandidate(dir, "alice", {
 				candidate: "Alice",
-				date: "2026-05-01",
+				date: "2026-06-30",
 				session_date: "2026-05-01",
 			});

🤖 Prompt for AI Agents

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

In `@tests/unit/services/interview/cohort/cohort-summary-edge-cases.spec.ts`
around lines 156 - 173, The test "orders chronologically using session_date when
available" currently uses identical date and session_date values so it doesn't
prove session_date precedence; update the test data created via makeCandidate
for "carol" and "alice" so their date and session_date intentionally conflict
(e.g., set Carol.date before Alice.date but Carol.session_date after
Alice.session_date) so renderCohortSummary(..., {order: "chronological"}) must
rely on session_date to produce Alice before Carol; keep using loadCohort and
the same expectation on body.indexOf.

---

132-135: ⚡ Quick win

Body-wide negative date assertion is brittle.

expect(body).not.toContain("2026-05-10") can fail if that value appears elsewhere (metadata/footer) even when the Interviewed column is correct. Scope the check to Alice’s row.

Suggested assertion narrowing

 			const body = renderCohortSummary("senior-backend", loadCohort(dir));
-			expect(body).toContain("2026-06-01");
-			// The submission date should NOT appear in place of the session date
-			expect(body).not.toContain("2026-05-10");
+			const aliceRow = body.split("\n").find((line) => line.includes("Alice")) ?? "";
+			expect(aliceRow).toContain("2026-06-01");
+			expect(aliceRow).not.toContain("2026-05-10");

🤖 Prompt for AI Agents

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

In `@tests/unit/services/interview/cohort/cohort-summary-edge-cases.spec.ts`
around lines 132 - 135, The negative date assertion is too broad: instead of
using expect(body).not.toContain("2026-05-10"), narrow the check to Alice's
table row by locating the row that contains "Alice" (e.g., parse the HTML in
body and select the tr or nearest container with text "Alice") and assert that
that specific row does not contain "2026-05-10"; update the test to query the
Alice row from body and replace the global not.toContain check with an assertion
against that row's text content.

---

207-210: ⚡ Quick win

The decimal-number assertion can create false failures.

expect(body).not.toMatch(/\d+\.\d+/) is broader than the requirement and could fail on unrelated decimals. Header assertions already validate the column contract well.

Suggested cleanup

 			expect(body).not.toMatch(/\|\s*Score\s*\|/i);
 			expect(body).not.toMatch(/\|\s*Total\s*\|/i);
 			expect(body).not.toMatch(/\|\s*Rank\s*\|/i);
-			expect(body).not.toMatch(/\d+\.\d+/); // no decimal numbers (scores)

🤖 Prompt for AI Agents

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

In `@tests/unit/services/interview/cohort/cohort-summary-edge-cases.spec.ts`
around lines 207 - 210, The test contains an overly broad negative assertion
expect(body).not.toMatch(/\d+\.\d+/) which can fail on unrelated decimals;
remove that assertion from cohort-summary-edge-cases.spec.ts (the
expect(body).not.toMatch(/\d+\.\d+/) line) or, if you want to keep a
decimal-specific check, replace it with a narrow pattern that matches decimal
values inside table cells (e.g., a cell-boundary regex like
/\|\s*\d+\.\d+\s*\|/) so only score-like decimals are asserted against; keep the
existing header assertions as they already validate the column contract.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@tests/unit/services/interview/bootstrap/project-generator-security.spec.ts`:
- Around line 109-125: The test expects generateProject(role({ outputDir: dir
}), clientFor(malicious)) to reject on paths containing a NUL byte, so add an
explicit NUL-byte validation in the path-resolution logic inside generateProject
(the code path that processes GeneratedProject.files -> file.path) to detect
"\0" (or char code 0) and throw a specific, named error (e.g., InvalidPathError
or a new PathValidationError) before any FS operations; then update the test to
assert that generateProject rejects with that specific error type/message
instead of a generic toThrow(), referencing generateProject, GeneratedProject,
and the file.path handling code for location.

In `@tests/unit/services/interview/bootstrap/role-config-edge-cases.spec.ts`:
- Around line 199-213: The test "stores optional jdPath and customPrompt when
present" only sets and asserts jdPath but not customPrompt; update the test to
actually include a customPrompt in the RoleConfig passed to writeRoleConfig and
add an assertion that readRoleConfig(dir)?.customPrompt equals that value (or
alternatively rename the test to only mention jdPath). Locate the test block in
role-config-edge-cases.spec.ts and modify the cfg object creation (RoleConfig)
to include customPrompt and add an expect(...) assertion after reading via
readRoleConfig(dir) to verify persistence, or change the test description to
match the current assertions.

---

Nitpick comments:
In `@tests/unit/services/interview/cohort/cohort-summary-edge-cases.spec.ts`:
- Around line 156-173: The test "orders chronologically using session_date when
available" currently uses identical date and session_date values so it doesn't
prove session_date precedence; update the test data created via makeCandidate
for "carol" and "alice" so their date and session_date intentionally conflict
(e.g., set Carol.date before Alice.date but Carol.session_date after
Alice.session_date) so renderCohortSummary(..., {order: "chronological"}) must
rely on session_date to produce Alice before Carol; keep using loadCohort and
the same expectation on body.indexOf.
- Around line 132-135: The negative date assertion is too broad: instead of
using expect(body).not.toContain("2026-05-10"), narrow the check to Alice's
table row by locating the row that contains "Alice" (e.g., parse the HTML in
body and select the tr or nearest container with text "Alice") and assert that
that specific row does not contain "2026-05-10"; update the test to query the
Alice row from body and replace the global not.toContain check with an assertion
against that row's text content.
- Around line 207-210: The test contains an overly broad negative assertion
expect(body).not.toMatch(/\d+\.\d+/) which can fail on unrelated decimals;
remove that assertion from cohort-summary-edge-cases.spec.ts (the
expect(body).not.toMatch(/\d+\.\d+/) line) or, if you want to keep a
decimal-specific check, replace it with a narrow pattern that matches decimal
values inside table cells (e.g., a cell-boundary regex like
/\|\s*\d+\.\d+\s*\|/) so only score-like decimals are asserted against; keep the
existing header assertions as they already validate the column contract.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 04c43fb4-79c1-440c-a31f-06f8b7db413e

📥 Commits

Reviewing files that changed from the base of the PR and between 790b9e8 and cf8229b.

📒 Files selected for processing (7)

• src/cli/index.ts
• tests/unit/services/interview/bootstrap/openai-generator-client.spec.ts
• tests/unit/services/interview/bootstrap/project-generator-security.spec.ts
• tests/unit/services/interview/bootstrap/role-config-edge-cases.spec.ts
• tests/unit/services/interview/cohort/cohort-summary-edge-cases.spec.ts
• tests/unit/services/interview/review/asciinema-edge-cases.spec.ts
• tests/unit/services/interview/review/audit-writer-edge-cases.spec.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/cli/index.ts