feat(youtube): Youtube enhancements - #305
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🐉 eve review — ✅ APPROVE · 7 findings
Previous runs (10)
|
📝 WalkthroughWalkthroughThe PR adds queue-backed YouTube jobs, scoped ask sessions, multi-video QA, transcript import/export, captions-only transcription, direct extension player control, cache utilities, provider environment propagation, service-user ownership, and launchd plist permission hardening. ChangesYouTube runtime services and entrypoints
Ask sessions, QA, and transcript workflows
Extension player and interface updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![eve-bot-lovinka[bot]](https://avatars.githubusercontent.com/in/4092731?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐉 eve review — 🟡 Review comments
e5d6a60· 4 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🔵 Low | 2 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Review complete. I posted 4 findings to PR #305:
All findings are advisory. The review also noted that 1 changed file has no corresponding test updates. |
Review fixes — round 1Four threads from @eve-bot-lovinka. Two produced code changes, two are rebutted with the reasoning below. Commits:
1. Sensitive params redacted from the API but stored in the databaseContext: The observation is factually right and the suggested remedy is impossible. const question = typeof params.question === "string" ? params.question : "";
// ...
if (!question.trim()) {
throw new Error(`qa job ${ctx.job.id}: missing question in params`);
}( Redaction is scoped to the read surface for a reason: Code before const SENSITIVE_PARAM_KEYS = ["holdId", "creditCost", "question", "presetInstructions"] as const;Code after // Redaction covers the read surface, not storage. `question` / `presetInstructions`
// ARE the job's input — the `qa` stage reads them back out of `params_json` and
// throws without them (`Youtube.stages.qa`) — so they have to be persisted. What
// this prevents is one user's question travelling back out of the multi-user HTTP
// API on someone else's job listing; the file itself is per-install under
// `~/.genesis-tools/youtube/`.
const SENSITIVE_PARAM_KEYS = ["holdId", "creditCost", "question", "presetInstructions"] as const;How fixed: documented the threat model at the constant so the asymmetry reads as deliberate. 2. Ambient user fallback couples QueueService to the request-context ALSContext: Making The legitimate part is that a forgotten Code before const userId = input.userId ?? getRequestContext()?.userId ?? null;
return this.pipeline.enqueue({Code after const userId = input.userId ?? getRequestContext()?.userId ?? null;
if (userId === null) {
// Not fatal: `pipeline.enqueue` accepts a null owner and anonymous HTTP
// paths use it deliberately. It IS worth a line in the log, because the
// other way to get here is a new CLI/MCP caller that forgot
// `withConsoleContext`, and the symptom (no ai_calls rows, job invisible
// to its owner) is otherwise silent.
logger.warn({ target, targetKind, stages }, "youtube queue: enqueuing an unowned job (no userId in scope)");
}
return this.pipeline.enqueue({How fixed: warn once at the choke point; kept the nullable contract. 3. Unusable password hash uses random entropy not persisted anywhereContext: The randomness is the security property, not overhead. A fixed "impossible hash" constant has a plaintext that existed when it was generated and lives in repo history; whoever recovers it can log in as the console account, which owns every CLI/MCP job. Hashing entropy that is never returned or stored means no plaintext exists at all. The cheaper-sentinel variant also breaks the login path. On cost: this executes once per database, on first console use. Confidence: 92% — 4. No test accompanies the 46 new lines in player-control.tsContext: The seek clamp is genuine behavior worth pinning. The obstacle was that the module reaches into Code before export function seekPlayerTo(seconds: number): boolean {
const video = findPlayerVideo();
if (!video || !Number.isFinite(seconds)) {
return false;
}
const target = Math.max(0, seconds);
const { duration } = video;
video.currentTime = Number.isFinite(duration) ? Math.min(target, duration) : target;
return true;
}Code after export function seekTargetFor(seconds: number, duration: number): number | null {
if (!Number.isFinite(seconds)) {
return null;
}
const target = Math.max(0, seconds);
// Live streams report Infinity and a not-yet-loaded video reports NaN — in both
// cases there is no end to clamp against, so only a finite duration caps the seek.
return Number.isFinite(duration) ? Math.min(target, duration) : target;
}
export function seekPlayerTo(seconds: number): boolean {
const video = findPlayerVideo();
if (!video) {
return false;
}
const target = seekTargetFor(seconds, video.duration);
if (target === null) {
return false;
}
video.currentTime = target;
return true;
}How fixed: new Verification
|
|
Your rebuttals are sound and the fixes are well-targeted. #1 (sensitive params) — The documentation fix is exactly right. The throw at #2 (ambient user fallback) — The warn is the right fix. You've preserved the legitimate null-owner paths ( #3 (unusable password hash) — Your rebuttal is correct. A constant sentinel has a plaintext somewhere in history; entropy that's never returned means no plaintext exists. The cost runs once per database, and #4 (player-control tests) — The split is clean and follows the existing extension test pattern. The one test failure ( All four threads closed with well-reasoned outcomes. The commits are targeted, the rebuttals cite the exact lines that prove the constraint, and the verification is clean. |
|
Delta review complete. I posted 2 findings to the new hunks in PR #305:
One finding from the previous review (the regex DoS vulnerability) was addressed in this push. All findings are advisory. |
Review fixes — round 2Two new threads from @eve-bot-lovinka, both on round 1's own changes. Both accepted. Commits:
5. JSDoc should document the null return caseContext: Fair — "can't be honoured" was carrying three distinct outcomes. The rewrite names all of them, not just the null case, since the uncapped-on-non-finite-duration branch is the one a reader is most likely to mistake for a bug. Code before /**
* Where a seek to `seconds` lands on a video of `duration`, or null when the
* request can't be honoured. Split out of `seekPlayerTo` so the clamping rules
* are pinned without a DOM, the same way `player-chapters` exports
* `tickPositionPct`.
*/Code after /**
* Where a seek to `seconds` lands on a video of `duration`. Split out of
* `seekPlayerTo` so the clamping rules are pinned without a DOM, the same way
* `player-chapters` exports `tickPositionPct`.
*
* @returns The target clamped into `[0, duration]`, uncapped when `duration` is
* non-finite (live stream, or a video that hasn't loaded its metadata), or null
* when `seconds` itself is non-finite — assigning that to `currentTime` throws.
*/How fixed: documented each return outcome and why the null one exists. 6. New logging behavior lacks test coverageContext: Asserting "the warn fired" would pin a log string rather than the behavior, and it would keep passing if the ownership resolution underneath it broke. The genuinely untested thing is that resolution: Code before No coverage. Code after it("owns a job by the ambient request user when the caller passes none", async () => {
const owned = await withRequestContext({ db: fixture.db, userId: 42 }, async () =>
fixture.queue.enqueue({ target: "ambient-owner", stages: ["metadata"] })
);
expect(owned.job?.userId).toBe(42);
});
it("prefers an explicit userId over the ambient one", async () => {
const owned = await withRequestContext({ db: fixture.db, userId: 42 }, async () =>
fixture.queue.enqueue({ target: "explicit-owner", stages: ["metadata"], userId: 7 })
);
expect(owned.job?.userId).toBe(7);
});
it("still enqueues, unowned, when there is no user anywhere in scope", async () => {
const result = fixture.queue.enqueue({ target: "unowned", stages: ["metadata"] });
expect(result.job?.userId).toBeNull();
});How fixed: three tests over the Verification
|
|
Both fixes are clean and show good judgment. #5 (JSDoc for #6 (ownership resolution tests) — You made the right call testing the resolution instead of the log line. Asserting "the warn fired" would couple the test to a string and keep passing if the fallback chain broke. The three tests pin the actual contract: explicit beats ambient, ambient works, and null still succeeds (which guards against a future "require userId" change). The ambient case is the proof the ALS works — Both commits are comment-only or test-only, so the verification is appropriately scoped. The review loop is converging well — round 2's threads are refinements on round 1's own changes, which is exactly what you want to see. |
|
The PR review subagent failed: it returned an empty result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/utils/ai/provider-env.ts`:
- Around line 38-43: Update the value check in the provider environment export
loop to ignore strings containing only whitespace before assigning them to
configured. Preserve non-empty provider keys and the existing configured mapping
behavior in the surrounding env.ai flow.
In `@src/youtube/lib/__tests__/ask-pipeline.test.ts`:
- Around line 216-218: Correct the getAskSession call in the test to pass the
session owner/user ID first and the session ID second, matching the method’s
getAskSession(userId, id) contract. Use the corresponding owner value from the
session fixture rather than relying on both values being 1.
In `@src/youtube/lib/__tests__/ask-sessions-db.test.ts`:
- Around line 14-15: Rename the test suite, descriptions, and local variables in
the ask-session tests from thread/threads to session/sessions, including the
touchAskThread reference, so no stale API naming remains. Increase the ordering
test’s sleep interval beyond 2 ms enough to ensure updated_at advances before
asserting listAskSessions ordering.
In `@src/youtube/lib/ask-answer.ts`:
- Around line 93-110: In the indexing flow around the budget calculation and
searchedVideoIds construction, replace the redundant maxIndex ternary with
nullish coalescing and build a Set from skippedUnindexed before filtering. Use
Set.has for the skip lookup while preserving the existing inclusion behavior.
In `@src/youtube/lib/ask-scope.ts`:
- Around line 56-67: Update resolveAskScope’s input.channel branch to normalize
a bare channel handle to the canonical `@-prefixed` form before calling
yt.videos.list and storing the returned channel value. Preserve already-prefixed
handles, or reject invalid non-@ input, so channel lookup and scopeValue
consistently match database rows.
In `@src/youtube/lib/ask-session.ts`:
- Around line 125-137: Update the ask flow around answerOverVideos so the user
message is not persisted before an answer is guaranteed to succeed. Persist the
user and assistant turns together only after answerOverVideos returns
successfully, or otherwise remove the user row on failure, while preserving
touchAskSession for successful sessions.
- Around line 81-87: Update the cache-refresh condition in the ask-session flow
around resolveAskScope and setAskSessionVideoIds to compare the contents of
scope.videoIds with session.videoIds, not only their lengths. Rewrite the stored
video IDs whenever membership differs, while preserving the existing return
value and avoiding writes when both arrays contain the same IDs.
In `@src/youtube/lib/cache-ops.ts`:
- Around line 29-68: Update deletePath to return zero when the referenced file
does not exist, only calculating bytes and unlinking after confirming existence.
Adjust clearVideoBinaries so deletedCount and freedBytes reflect actual files
removed rather than merely non-null database paths, while still clearing stale
database references as appropriate; preserve the existing audio, video, and
thumbnail option handling.
In `@src/youtube/lib/db.ts`:
- Around line 693-738: Wrap the entire CREATE, INSERT, and DROP sequence in the
ask-threads-to-sessions migration callback passed to runMigration with
this.db.transaction(...), matching the atomic rebuild pattern used by
qa-chunks-unique-include-source and add-artifact-access. Ensure all statements
execute within the transaction so any failure rolls back the complete migration
and preserves the legacy tables for retry.
In `@src/youtube/lib/qa.types.ts`:
- Around line 27-28: Update QaService.ask() to pass model: opts.model when
creating the query embedder, honoring the bucket contract declared by the model
field in qa.types.ts. In src/youtube/lib/qa.types.ts lines 27-28, retain the
advertised model option; in src/youtube/lib/__tests__/qa.test.ts lines 63-84,
make fake vectors model-dependent and assert the query embedder receives
"custom-embedder".
In `@src/youtube/lib/queue.ts`:
- Around line 298-373: The blocking queue APIs must stop polling indefinitely
when a job row is absent. In src/youtube/lib/queue.ts lines 298-373, update
waitForJob to reject or return an explicit not-found result when
db.getJob(jobId) is null, and provide a bounded default timeout for bare callers
such as streamJobToCompletion; in src/youtube/lib/queue.ts lines 289-292, update
allWatchedJobsFinal so missing job IDs are treated as terminal and the watch
loop emits watch:done.
In `@src/youtube/lib/server/routes/pipeline.ts`:
- Around line 83-90: Update the POST cancel handler around QueueService.cancel
so a null result returns a 404 response, matching the existing GET :id and
:id/activity handlers. Preserve the current successful response with the job
payload for found jobs and continue using CORS_HEADERS.
- Around line 22-32: Validate that body.stages is an array before calling
toJobStages in the request handler, and return the existing 400 error response
for malformed or missing stages. Keep the later enqueue flow intact, but avoid
using toJobStages for the route payload conversion when the guard is only needed
for early validation.
In `@src/youtube/lib/youtube.ts`:
- Around line 589-646: Extract the repeated parameter parsing and invalid-value
warning logic from createPipelineHandlers into a reusable parseJobParam helper.
Use it for sources, forceReindex, provider, model, discover’s limit and
includeShorts, and captionsOnly, preserving each parameter’s existing type guard
and stage-specific warning context; remove the duplicated raw-value and warning
blocks from the handlers.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b70de92e-364f-4342-86af-de33a8248876
📒 Files selected for processing (64)
src/utils/DashboardApp/launchd.tssrc/utils/ai/__tests__/provider-env.test.tssrc/utils/ai/provider-env.tssrc/youtube/commands/__tests__/channels.test.tssrc/youtube/commands/__tests__/console-user-fake.tssrc/youtube/commands/__tests__/download.test.tssrc/youtube/commands/__tests__/pipeline.test.tssrc/youtube/commands/_shared/utils.tssrc/youtube/commands/cache.tssrc/youtube/commands/channels.tssrc/youtube/commands/download.tssrc/youtube/commands/pipeline.tssrc/youtube/commands/transcribe.tssrc/youtube/extension/__tests__/player-control.test.tssrc/youtube/extension/content-script.tssrc/youtube/extension/player-control.tssrc/youtube/extension/popup/popup.csssrc/youtube/extension/shared/messages.tssrc/youtube/extension/side-panel/playlist-panel.tsxsrc/youtube/extension/side-panel/side-panel.tsxsrc/youtube/lib/__tests__/ask-pipeline.test.tssrc/youtube/lib/__tests__/ask-sessions-db.test.tssrc/youtube/lib/__tests__/ask-threads-db.test.tssrc/youtube/lib/__tests__/collection-ask.test.tssrc/youtube/lib/__tests__/config-foundations.test.tssrc/youtube/lib/__tests__/legacy-schema-upgrade.test.tssrc/youtube/lib/__tests__/qa-channel.test.tssrc/youtube/lib/__tests__/qa.test.tssrc/youtube/lib/__tests__/queue.test.tssrc/youtube/lib/__tests__/service-user.test.tssrc/youtube/lib/__tests__/transcript-clock.test.tssrc/youtube/lib/__tests__/transcripts.test.tssrc/youtube/lib/__tests__/users.test.tssrc/youtube/lib/ask-answer.tssrc/youtube/lib/ask-scope.tssrc/youtube/lib/ask-session.tssrc/youtube/lib/cache-ops.tssrc/youtube/lib/collection-ask.tssrc/youtube/lib/config.tssrc/youtube/lib/db.tssrc/youtube/lib/db.types.tssrc/youtube/lib/job-activity.tssrc/youtube/lib/jobs.types.tssrc/youtube/lib/pipeline.tssrc/youtube/lib/qa.tssrc/youtube/lib/qa.types.tssrc/youtube/lib/queue.tssrc/youtube/lib/server/app.tssrc/youtube/lib/server/routes/cache.tssrc/youtube/lib/server/routes/channels.tssrc/youtube/lib/server/routes/collections.tssrc/youtube/lib/server/routes/pipeline.tssrc/youtube/lib/server/routes/videos.tssrc/youtube/lib/service-user.tssrc/youtube/lib/transcript-export.tssrc/youtube/lib/transcripts.tssrc/youtube/lib/transcripts.types.tssrc/youtube/lib/types.tssrc/youtube/lib/users.tssrc/youtube/lib/users.types.tssrc/youtube/lib/youtube.tssrc/youtube/ui/api.client.tssrc/youtube/ui/components/shared/collection-ask-panel.tsxsrc/youtube/ui/components/shared/collection-ui.ts
💤 Files with no reviewable changes (1)
- src/youtube/lib/tests/ask-threads-db.test.ts
Review fixes — round 3CodeRabbit's full pass: 14 threads. 12 accepted, 2 skipped with reasons below. Commits:
7. Filter whitespace-only values in
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Relevant commits present =='
git log --oneline -30 | rg '94ee2f9dc|32a6eeedf|65987b5a0|9f665a095|26f6b3685|7fbcbbc77|de2fafbf1|b0f82e1f6' || true
printf '%s\n' '== Source maps =='
ast-grep outline src/utils/ai/provider-env.ts --items all
ast-grep outline src/youtube/lib/ask-answer.ts --items all
ast-grep outline src/youtube/lib/ask-scope.ts --items all
ast-grep outline src/youtube/lib/ask-session.ts --items all
ast-grep outline src/youtube/lib/cache-ops.ts --items all
ast-grep outline src/youtube/lib/queue.ts --items all
ast-grep outline src/youtube/lib/qa.ts --items all
ast-grep outline src/youtube/lib/server/routes/pipeline.ts --items all
ast-grep outline src/youtube/lib/youtube.ts --items all
printf '%s\n' '== Relevant source/test occurrences =='
rg -n -C 4 \
'collectConfiguredProviderEnv|listConfiguredEnvKeys|getTrimmed|isNonEmpty|normaliseHandle|const budget|skippedUnindexed|const changed|answerOverVideos|deletePath|ask-threads|ask-sessions|createEmbedder|waitForJob|allWatchedJobsFinal|target \(string\)|cancel\(|getAskSession|parseJobParam|captionsOnly|includeShorts' \
src/utils/ai/provider-env.ts src/utils/env/env-core.ts \
src/youtube/lib/ask-answer.ts src/youtube/lib/ask-scope.ts src/youtube/lib/ask-session.ts \
src/youtube/lib/cache-ops.ts src/youtube/lib/db.ts src/youtube/lib/qa.ts src/youtube/lib/queue.ts \
src/youtube/lib/server/routes/pipeline.ts src/youtube/lib/youtube.ts \
src/youtube/lib/__tests__/ask-pipeline.test.ts src/youtube/lib/__tests__/ask-sessions-db.test.ts \
src/youtube/lib/__tests__/queue.test.ts src/youtube/lib/__tests__/qa.test.ts \
src/youtube/lib/__tests__/legacy-schema-upgrade.test.ts 2>/dev/null || trueLength of output: 68523 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Locate AI environment wrappers =='
rg -n -C 4 'listConfiguredEnvKeys|getByEnvKey' src/utils
printf '%s\n' '== Queue wait/watch and terminal-state handling =='
sed -n '299,419p' src/youtube/lib/queue.ts
sed -n '546,574p' src/youtube/lib/queue.ts
printf '%s\n' '== Atomic migration body =='
sed -n '693,747p' src/youtube/lib/db.ts
printf '%s\n' '== QA model selection and relevant tests =='
sed -n '188,224p' src/youtube/lib/qa.ts
rg -n -A20 -B5 'requested embedder model bucket|custom-embedder|createEmbedderCalls' src/youtube/lib/__tests__/qa.test.ts
printf '%s\n' '== Focused regression test evidence =='
rg -n -A24 -B4 \
'orphan|failed|membership|channel.*session|normaliseHandle|no longer exists|missing job|unknown.*job|malformed|stale.*reference|freed|atomic|migration|25' \
src/youtube/lib/__tests__ src/youtube/lib/server 2>/dev/null || true
printf '%s\n' '== Deferred parse/warn blocks within the pipeline handler =='
sed -n '390,490p' src/youtube/lib/youtube.ts
sed -n '570,670p' src/youtube/lib/youtube.tsLength of output: 193502
I did not independently run the test suite in this sandbox; the source-level inspection is consistent with your reported verification. 🐇✅ ✏️ Learnings added
|
There was a problem hiding this comment.
🐉 eve review — 🔴 Changes requested
b0f82e1· 5 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🔵 Low | 2 |
|
Delta review completed for PR #305, covering only
|
|
Full review recovery completed for
|
Review fixes — round 3Five findings from 1. Job reads and cancel were reachable across usersContext:
Code before const jobs = yt.queue.list({ status: status ?? undefined, limit, redact: true });
const result = yt.queue.get(id, { redact: true });
const rows = yt.queue.activity(id);
const job = yt.queue.cancel(id);Code after // Resolved once, for every branch below.
const actor = resolveJobActor(req, url, yt.db);
const jobs = yt.queue.list({ status: status ?? undefined, limit, redact: true, actor });
const result = yt.queue.get(id, { redact: true, actor });
const rows = yt.queue.activity(id, actor);
const job = yt.queue.cancel(id, actor);How fixed: in the lib, not the route, so the CLI/HTTP/MCP doors all inherit it. export type JobActor = { kind: "operator" } | { kind: "user"; userId: number };A union rather than an optional function actorOwnsJob(job: PipelineJob, actor: JobActor): boolean {
return actor.kind === "operator" || job.userId === actor.userId;
}A foreign job is reported absent, never forbidden — 403 on someone else's id and 404 on an unused one would be an existence oracle over the jobs table. The strict equality also keeps unowned CLI jobs ( Confidence: 97% — two of the three new ownership tests fail when 2. Invalid stage values and malformed JSON returned 500Context: The previous guard only proved Code before const body = (await req.json()) as EnqueueBody;
if (typeof body.target !== "string" || !Array.isArray(body.stages)) {
return jsonError("target (string) and stages (array of stage names) are required", 400);
}Code after let raw: unknown;
try {
raw = await req.json();
} catch (err) {
logger.debug({ err, path: url.pathname }, "youtube API: pipeline enqueue body was not valid JSON");
return jsonError("request body must be valid JSON", 400);
}
const body = parseEnqueueBody(raw);
if (!body.ok) {
return jsonError(body.error, 400);
}How fixed: a non-throwing Confidence: 95% — 8 assertions across 6 cases, and the well-formed-body case still returns 200 so the validation cannot pass by rejecting everything. 3. Migration does not use the persisted frameworkContext: Your reading is right: Code (unchanged) private runMigration(_name: string, apply: () => void): void {
try {
apply();
} catch (error) {
throw new Error(`migration "${_name}" failed: …`);
}
}Why not here: two checkable facts. What this PR does fix is the part that made the rebuild genuinely unsafe: it is wrapped in Confidence: 90% — counts are from 4. Cancel's 404 was undocumentedContext: Code before responses: {
"200": jsonResponse("Cancelled job", {
type: "object",
properties: { job: { oneOf: [ref("PipelineJob"), { type: "null" }] } },
required: ["job"],
}),
},Code after responses: {
"200": jsonResponse("Cancelled job", {
type: "object",
properties: { job: ref("PipelineJob") },
required: ["job"],
}),
"404": errorResponse,
},How fixed: dropping the null branch matters as much as adding the 404 — it only existed because the handler used to return Confidence: 99% — mechanical, and it now matches the handler in the same PR. 5. No route tests for the new error responsesContext: Correct that the file only covered How fixed: 2 tests to 12, on a Confidence: 95% — 12 pass, 39 assertions; the two ownership tests were verified to fail against a stubbed predicate. Verification
|
|
Round 3 is substantially stronger: it closes the actual authorization boundary rather than only attributing jobs.
Verification is credible: type checking and formatting pass, and the remaining live OpenAI TTS failure is independently reproducible without these commits and attributable to the key’s missing audio scope. |
There was a problem hiding this comment.
🐉 eve review — 🔴 Changes requested
69f4a8e· 5 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🔵 Low | 3 |
|
Delta review completed for PR #305, covering only
|
Review fixes — round 4Five findings from Commit: Both security findings are regressions of my own round-3 change, so they are the headline. 1. A
|
|
Round 4 correctly closes two serious regressions introduced by the round-3 authorization work.
The remaining TTS failure continues to be an external credential-scope problem in a live-network test, not evidence against this commit. |
|
@eve-bot-lovinka review Run 3 on this PR is recorded as All findings from the last completed review have since been fixed and pushed:
Every review thread has a reply, and CI is green on the current head. Requesting a fresh review so the verdict reflects it. |
There was a problem hiding this comment.
🐉 eve review — 🟡 Review comments
3daf40d· 4 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🔵 Low | 1 |
|
PR #305 received a full deep review across 68 changed files.
|
Both indexed then called qa.ask inline, skipping citation enrichment, so the same question answered differently depending on the surface. analyze --ask now prints the titles and deep links its --json already carried.
add/list/show/watch/cancel/stats, --json and --jsonl per the repo idiom. Enqueue runs under the console service user so CLI work is attributed the same way HTTP work is.
One-shot and --session turns both answer through answerOverVideos, so the citation shape matches the HTTP route. --dir imports into the DB first rather than growing a second retrieval engine.
transcripts export|import|show over the one renderer; config get|set thin over yt.config, warning that a running server caches config in memory.
Seven workflow-shaped tools rather than one per HTTP route; admin, billing, cache and config writes are deliberately not exposed. Every tool is a thin call into the same core the CLI uses.
- cap lazy indexing at MAX_LAZY_INDEX_PER_ASK in the shared answering layer; a channel ask could otherwise embed thousands of transcripts before answering - redact audioPath/videoPath/thumbPath from MCP video responses - --stream with --json emitted text then JSON; machine-readable output wins - config set validated against the current value's runtime shape, unwrapping comment-json's boxed scalars first Also extends the CLAUDE.md thin-adapter rule from commands to HTTP routes and MCP, per the YoutubePipelineCliAskMcp plan.
…the transcript alone
…target, raise MethodNotFound
… out of the default stages
…ired MCP arguments
1f8569b to
c3f5b07
Compare
Review fixes — round 8 (+ rebase onto the new master)Seven new threads (t45-t51), all consequences of the previous two rounds' own changes. All seven addressed. Commit: ❗ The branch is now rebased onto master 45. Strict numeric parsing still omits queue priorityVerdict: Accepted. I claimed the helper covered the queue command's numeric arguments and left one out. .option("--priority <n>", "Higher runs first", (value) => parseNonNegativeInt(value, "--priority"))In the option parser rather than the action, since commander surfaces the throw with the flag name already in it, and 46. Route watch results through the output abstractionVerdict: Accepted. stdout is right for this command — the events are its result — but raw Note this is the opposite call from 47. Validate all required MCP arguments at runtimeVerdict: Accepted — I added
48. Reuse the shared integer parser and reject unsafe integersVerdict: Accepted — the
49. Command tests for every strict numeric argumentVerdict: Accepted. New malformed suffix · zero/negative/whitespace limits · unsafe integer · valid limit reaching 50. Exercise the changed MCP handler end to endVerdict: Accepted, taking the extract-the-dispatcher option. export async function callMcpTool(yt: Youtube, name: string, args: Record<string, unknown>): Promise<ToolResult>
51. Generic "no test changes" nagVerdict: Covered by 50. That file now holds 16 tests across the clamp, the advertised surface, and the dispatcher. Verification (after rebase, on master
|
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/youtube/lib/youtube.ts (1)
680-694: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
answerOverVideosis called withoutctx.signal, so a cancelledqajob keeps indexing and answering.
answerOverVideosacceptssignaland uses it forthrowIfAborted()before each lazy index and forwards it intoyt.qa.index(...); omitting it means a cancel landing mid-answer is only observed after this handler returns — after the embedding/completion spend.🐛 Proposed fix
const result = await answerOverVideos({ yt: this, videoIds, question, topK, providerChoice, presetInstructions, sources, lang, + signal: ctx.signal, });🤖 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/youtube/lib/youtube.ts` around lines 680 - 694, Pass the active cancellation signal from the current handler context into the answerOverVideos call in the qa answering flow. Use the existing ctx.signal value so answerOverVideos can abort before lazy indexing and propagate cancellation through yt.qa.index, while leaving the other answer parameters unchanged.
🤖 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/youtube/commands/__tests__/pipeline.test.ts`:
- Around line 22-66: Extract the duplicated fake pipeline and 18-field
PipelineJob builder into a shared fakePipelineFake helper alongside
console-user-fake.ts, returning pipeline, jobs, and calls while preserving
input-derived targetKind and allowing a status option. In
src/youtube/commands/__tests__/pipeline.test.ts lines 22-66, replace the inline
fake with the helper; do the same in
src/youtube/commands/__tests__/download.test.ts lines 25-62 while retaining only
setVideoPinned/getJob extras on fakeDb; and in
src/youtube/commands/__tests__/channels.test.ts lines 37-65, invoke the helper
with status: "pending" for enqueue-only assertions.
In `@src/youtube/commands/ask.ts`:
- Around line 44-46: Update the --limit and --top-k option parsers in the ask
command to validate values as positive integers before storing them. Reuse the
queue command’s positive-integer parser or add a local wrapper around
Number.parseInt that rejects non-numeric and non-positive inputs, ensuring
invalid values fail during argument parsing before reaching resolveAskScope or
retrieval.
In `@src/youtube/commands/channels.ts`:
- Around line 152-185: Update the non-sync enqueue flow in the
withConsoleContext callback to preserve and return the job IDs collected before
any handle fails, including when result.job is missing. Replace the throw-only
behavior around result.job with a partial-result path that lets the caller
report the already enqueued IDs while retaining the existing IDs for successful
handles.
In `@src/youtube/commands/config.ts`:
- Line 125: Remove the `as never` assertion from the `yt.config.set` call in the
configuration command. Update `YoutubeConfig.set` to use a key-generic signature
tying `K extends keyof YoutubeConfigShape` to `YoutubeConfigShape[K]`, and
narrow `unwrapped` against `typeof current` before calling it so the compiler
enforces the key/value relationship.
In `@src/youtube/commands/queue.ts`:
- Around line 249-283: The watch command reads the wrong Commander option
property for --no-children. Update WatchOpts and the queue.watch invocation in
the watch action to use opts.children, setting followChildren based on
opts.children !== false so the flag disables child following.
In `@src/youtube/extension/side-panel/side-panel.tsx`:
- Around line 444-448: Update the debug message in the seek function to
accurately cover both failure causes: no video element and an unusable seek
target. Keep the existing seconds context and seekPlayerTo behavior unchanged.
In `@src/youtube/lib/__tests__/qa.test.ts`:
- Around line 63-90: Remove the as never assertion from providerChoice in the
retrieves-only-requested-embedder-model-bucket test, and construct or type the
fixture using the providerChoice type expected by QaService.ask while preserving
the existing test values and behavior.
In `@src/youtube/lib/cache-ops.ts`:
- Around line 13-27: Replace the per-video listTranscripts reduction in
buildCacheStatsBase with one database-level aggregate count of transcripts,
using an existing aggregate/query helper or adding one that counts all
transcripts for the relevant video set. Update both cache-stats callers to use
this aggregate while preserving the returned CacheStatsBase fields; avoid
querying transcripts once per video from listCacheVideos results.
In `@src/youtube/lib/config.ts`:
- Around line 18-27: The DEFAULT_YOUTUBE_CONFIG entry for ai must remain empty
or provider-agnostic so fresh installs and tests do not require Grok. Remove the
built-in grok/grok-4.5 mapping from DEFAULT_YOUTUBE_CONFIG, and place that
task-specific mapping only in the local configuration where it can be explicitly
overridden.
In `@src/youtube/lib/mcp/server.ts`:
- Around line 397-403: Update the jobId handling in the dispatcher around
yt.queue.get so that whenever the jobId argument is provided, it must be a
number; reject string, null, and other malformed values with the same validation
error behavior used by the other arguments instead of falling through to the
queue depth summary. Preserve the existing lookup and “Job not found” response
for valid numeric job IDs.
In `@src/youtube/lib/server/auth.ts`:
- Around line 176-184: Update the documentation for requireUser to accurately
list all token sources resolved by extractPresentedToken, including the ?key=
query parameter alongside the existing Authorization Bearer header and
?access_token= fallback. Keep the existing prefix-handling and non-throwing
behavior descriptions unchanged.
In `@src/youtube/lib/server/routes/pipeline.ts`:
- Around line 33-41: Replace the inline req.json() try/catch in the pipeline
enqueue handler with the shared safeJsonBody helper from body.ts, adding the
import and passing the request, logger, and pathname as required. Treat an
undefined result as the existing 400 invalid-body response, preserving the
current behavior for malformed or non-object JSON.
In `@src/youtube/lib/transcript-export.ts`:
- Around line 272-279: Normalize parsed.channel with the existing
normaliseHandle logic before persistence in the transcript export flow. Reuse
the resulting canonical ChannelHandle for both opts.db.upsertChannel and the
channelHandle field in opts.db.upsertVideo, rather than casting the raw parsed
value directly.
In `@src/youtube/lib/transcripts.ts`:
- Around line 171-193: Update the conflicting-options branch in the transcript
flow to use a distinct info-log message from the genuine captions-miss path
after tryCaptions. Keep the existing “youtube captions-only miss” message only
for the branch where caption lookup was attempted and returned no result, while
preserving the NoCaptionsError behavior and relevant videoId context in both
branches.
In `@src/youtube/lib/youtube.ts`:
- Around line 407-415: Update the limit validation in the discover job flow
around params.limit so it accepts only positive safe integers, rejecting NaN,
zero, negative, fractional, and unsafe numeric values before calling
channels.sync or deps.listChannelVideos. Preserve the existing undefined
behavior and invalid-value warning, including the job ID and value type.
---
Outside diff comments:
In `@src/youtube/lib/youtube.ts`:
- Around line 680-694: Pass the active cancellation signal from the current
handler context into the answerOverVideos call in the qa answering flow. Use the
existing ctx.signal value so answerOverVideos can abort before lazy indexing and
propagate cancellation through yt.qa.index, while leaving the other answer
parameters unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 032d1ae7-4759-45f3-906c-3482d9c5f38b
📒 Files selected for processing (83)
CLAUDE.mdsrc/utils/DashboardApp/launchd.tssrc/utils/ai/__tests__/provider-env.test.tssrc/utils/ai/provider-env.tssrc/youtube/README.mdsrc/youtube/commands/__tests__/analyze.test.tssrc/youtube/commands/__tests__/cache.test.tssrc/youtube/commands/__tests__/channels.test.tssrc/youtube/commands/__tests__/console-user-fake.tssrc/youtube/commands/__tests__/download.test.tssrc/youtube/commands/__tests__/pipeline.test.tssrc/youtube/commands/__tests__/queue-args.test.tssrc/youtube/commands/_shared/utils.tssrc/youtube/commands/analyze.tssrc/youtube/commands/ask.tssrc/youtube/commands/cache.tssrc/youtube/commands/channels.tssrc/youtube/commands/config.tssrc/youtube/commands/download.tssrc/youtube/commands/mcp.tssrc/youtube/commands/pipeline.tssrc/youtube/commands/queue.tssrc/youtube/commands/transcribe.tssrc/youtube/commands/transcripts.tssrc/youtube/extension/__tests__/player-control.test.tssrc/youtube/extension/content-script.tssrc/youtube/extension/player-control.tssrc/youtube/extension/popup/popup.csssrc/youtube/extension/shared/messages.tssrc/youtube/extension/side-panel/playlist-panel.tsxsrc/youtube/extension/side-panel/side-panel.tsxsrc/youtube/index.tssrc/youtube/lib/__tests__/ask-pipeline.test.tssrc/youtube/lib/__tests__/ask-sessions-db.test.tssrc/youtube/lib/__tests__/ask-sessions-migration.test.tssrc/youtube/lib/__tests__/ask-threads-db.test.tssrc/youtube/lib/__tests__/collection-ask.test.tssrc/youtube/lib/__tests__/config-foundations.test.tssrc/youtube/lib/__tests__/legacy-schema-upgrade.test.tssrc/youtube/lib/__tests__/qa-channel.test.tssrc/youtube/lib/__tests__/qa.test.tssrc/youtube/lib/__tests__/queue.test.tssrc/youtube/lib/__tests__/service-user.test.tssrc/youtube/lib/__tests__/transcript-clock.test.tssrc/youtube/lib/__tests__/transcript-import-validation.test.tssrc/youtube/lib/__tests__/transcripts.test.tssrc/youtube/lib/__tests__/users.test.tssrc/youtube/lib/ask-answer.tssrc/youtube/lib/ask-scope.tssrc/youtube/lib/ask-session.tssrc/youtube/lib/cache-ops.tssrc/youtube/lib/collection-ask.tssrc/youtube/lib/config.tssrc/youtube/lib/db.tssrc/youtube/lib/db.types.tssrc/youtube/lib/job-activity.tssrc/youtube/lib/jobs.types.tssrc/youtube/lib/mcp/__tests__/server.test.tssrc/youtube/lib/mcp/server.tssrc/youtube/lib/pipeline.tssrc/youtube/lib/qa.tssrc/youtube/lib/qa.types.tssrc/youtube/lib/queue.tssrc/youtube/lib/server/__tests__/queue-route.test.tssrc/youtube/lib/server/app.tssrc/youtube/lib/server/auth.tssrc/youtube/lib/server/openapi.tssrc/youtube/lib/server/routes/cache.tssrc/youtube/lib/server/routes/channels.tssrc/youtube/lib/server/routes/collections.tssrc/youtube/lib/server/routes/pipeline.tssrc/youtube/lib/server/routes/videos.tssrc/youtube/lib/service-user.tssrc/youtube/lib/transcript-export.tssrc/youtube/lib/transcripts.tssrc/youtube/lib/transcripts.types.tssrc/youtube/lib/types.tssrc/youtube/lib/users.tssrc/youtube/lib/users.types.tssrc/youtube/lib/youtube.tssrc/youtube/ui/api.client.tssrc/youtube/ui/components/shared/collection-ask-panel.tsxsrc/youtube/ui/components/shared/collection-ui.ts
💤 Files with no reviewable changes (1)
- src/youtube/lib/tests/ask-threads-db.test.ts
| const fakePipeline = { | ||
| // Typed as the real return so this mock can't silently drift from | ||
| // `Pipeline.enqueue` again. It used to hand back a bare job, while | ||
| // production destructures `{ job }`, so every command test threw | ||
| // "enqueue returned no job". | ||
| enqueue: (input: unknown): EnqueuePipelineResult => { | ||
| calls.enqueue.push(input); | ||
| const job: PipelineJob = { | ||
| id: jobs.length + 1, | ||
| targetKind: (input as { targetKind: PipelineJob["targetKind"] }).targetKind, | ||
| target: (input as { target: string }).target, | ||
| stages: (input as { stages: JobStage[] }).stages, | ||
| currentStage: null, | ||
| status: "completed", | ||
| error: null, | ||
| progress: 1, | ||
| progressMessage: null, | ||
| parentJobId: null, | ||
| userId: null, | ||
| workerId: null, | ||
| claimedAt: null, | ||
| createdAt: "2026-04-01", | ||
| updatedAt: "2026-04-01", | ||
| completedAt: "2026-04-01", | ||
| priority: 50, | ||
| params: null, | ||
| fingerprint: null, | ||
| }; | ||
| jobs.push(job); | ||
|
|
||
| return { job, reused: false, queuePosition: jobs.length }; | ||
| }, | ||
| setGlobalConcurrencyOverride: (value: number | null) => { | ||
| calls.concurrency.push(value); | ||
| }, | ||
| start: async () => { | ||
| calls.start++; | ||
| }, | ||
| getJob: (id: number) => jobs.find((job) => job.id === id) ?? null, | ||
| on: (event: string) => { | ||
| calls.on.push(event); | ||
|
|
||
| return () => undefined; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the fake pipeline/job builder into one shared test helper. All three files hand-roll the same enqueue: (input) => EnqueuePipelineResult fake plus the full 18-field PipelineJob literal, and they have already diverged (targetKind hardcoded to "video" in one, derived from input in the others; status: "completed" vs "pending"). A shared factory alongside console-user-fake.ts — e.g. fakePipelineFake({ status }) returning { pipeline, jobs, calls } — keeps the mock in step with Pipeline.enqueue, which is exactly the drift the inline comments say bit these tests before.
src/youtube/commands/__tests__/pipeline.test.ts#L22-L66: move this variant (input-derivedtargetKind) into the shared helper and consume it here.src/youtube/commands/__tests__/download.test.ts#L25-L62: replace the local fake with the shared helper; keep only thesetVideoPinned/getJobextras onfakeDb.src/youtube/commands/__tests__/channels.test.ts#L37-L65: replace the local fake with the shared helper, passingstatus: "pending"for the enqueue-only assertions.
📍 Affects 3 files
src/youtube/commands/__tests__/pipeline.test.ts#L22-L66(this comment)src/youtube/commands/__tests__/download.test.ts#L25-L62src/youtube/commands/__tests__/channels.test.ts#L37-L65
🤖 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/youtube/commands/__tests__/pipeline.test.ts` around lines 22 - 66,
Extract the duplicated fake pipeline and 18-field PipelineJob builder into a
shared fakePipelineFake helper alongside console-user-fake.ts, returning
pipeline, jobs, and calls while preserving input-derived targetKind and allowing
a status option. In src/youtube/commands/__tests__/pipeline.test.ts lines 22-66,
replace the inline fake with the helper; do the same in
src/youtube/commands/__tests__/download.test.ts lines 25-62 while retaining only
setVideoPinned/getJob extras on fakeDb; and in
src/youtube/commands/__tests__/channels.test.ts lines 37-65, invoke the helper
with status: "pending" for enqueue-only assertions.
| .option("--limit <n>", "Cap channel members, newest first", (value) => Number.parseInt(value, 10)) | ||
| .option("--session <name>", "Keep conversational memory under this session name") | ||
| .option("--top-k <n>", "Chunks to retrieve", (value) => Number.parseInt(value, 10)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is there an existing validated numeric parser for youtube CLI options?
rg -nP -C4 'parsePositive|Number\.parseInt\(' src/youtube/commands --type=ts | head -80
fd -a 'queue-args.test.ts' src/youtubeRepository: genesiscz/GenesisTools
Length of output: 7318
Validate --limit and --top-k before storing them. Number.parseInt accepts NaN, and those values flow into resolveAskScope and retrieval. Reuse the queue command’s positive-integer parser or a local wrapper so non-numeric or non-positive inputs fail at parse time.
🤖 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/youtube/commands/ask.ts` around lines 44 - 46, Update the --limit and
--top-k option parsers in the ask command to validate values as positive
integers before storing them. Reuse the queue command’s positive-integer parser
or add a local wrapper around Number.parseInt that rejects non-numeric and
non-positive inputs, ensuring invalid values fail during argument parsing before
reaching resolveAskScope or retrieval.
| if (!opts.sync) { | ||
| // Console context so these jobs get a real owner instead of a NULL | ||
| // user_id — `QueueService.enqueue` reads the owner from this ALS. | ||
| const enqueuedJobIds = await withConsoleContext(yt.db, async () => { | ||
| const jobIds: number[] = []; | ||
|
|
||
| for (const handle of targets) { | ||
| const result = yt.queue.enqueue({ | ||
| targetKind: "channel", | ||
| target: handle, | ||
| stages: ["discover", "metadata"], | ||
| params: { | ||
| limit: opts.limit, | ||
| includeShorts: opts.includeShorts, | ||
| }, | ||
| }); | ||
|
|
||
| if (!result.job) { | ||
| throw new Error(`channel sync enqueue returned no job for ${handle}`); | ||
| } | ||
|
|
||
| jobIds.push(result.job.id); | ||
| } | ||
|
|
||
| return jobIds; | ||
| }); | ||
|
|
||
| await renderOrEmit({ | ||
| text: `Enqueued job ids: ${enqueuedJobIds.join(", ")}`, | ||
| json: { enqueuedJobIds }, | ||
| flags: cmd.optsWithGlobals(), | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether QueueService.enqueue falls back to the ambient request context for the owner.
fd -t f 'queue.ts' src/youtube/lib --exec-batch ast-grep outline --items all
rg -nP -C8 '\benqueue\s*\(' src/youtube/lib/queue.ts
rg -nP -C4 'getRequestContext|userId' src/youtube/lib/queue.tsRepository: genesiscz/GenesisTools
Length of output: 8710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the enqueue return type and any code paths that can omit `job`.
fd -t f 'pipeline.types.ts' 'src/youtube/lib' 'src/youtube' --exec-batch sh -lc 'echo "== {} =="; ast-grep outline "{}" --match EnqueuePipelineResult --view expanded || true'
fd -t f 'pipeline.ts' 'src/youtube/lib' --exec-batch sh -lc 'echo "== {} =="; rg -n -C6 "return .*job|job\\s*[:?]|EnqueuePipelineResult|enqueue\\(" "{}" || true'
# Re-read the relevant queue implementation around enqueue to confirm the current contract.
sed -n '89,132p' src/youtube/lib/queue.tsRepository: genesiscz/GenesisTools
Length of output: 5254
Return the ids already enqueued if one handle fails. If result.job is null partway through the loop, the throw drops the ids created earlier, so the caller can’t tell which channels were queued before the failure.
🤖 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/youtube/commands/channels.ts` around lines 152 - 185, Update the non-sync
enqueue flow in the withConsoleContext callback to preserve and return the job
IDs collected before any handle fails, including when result.job is missing.
Replace the throw-only behavior around result.job with a partial-result path
that lets the caller report the already enqueued IDs while retaining the
existing IDs for successful handles.
| return; | ||
| } | ||
|
|
||
| await yt.config.set(key, unwrapped as never); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Drop the as never cast.
as never is the same escape hatch as as any — it erases exactly the mismatch the shape check above tries to approximate. Give YoutubeConfig.set a key-generic signature (set<K extends keyof YoutubeConfigShape>(key: K, value: YoutubeConfigShape[K])) and narrow unwrapped against typeof current there, so the compiler carries the invariant instead of a comment.
Based on learnings: as never type assertions are treated as the same code smell as as any in this codebase. As per coding guidelines: "Never use as any; use type narrowing, type guards, or explicit interfaces".
🤖 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/youtube/commands/config.ts` at line 125, Remove the `as never` assertion
from the `yt.config.set` call in the configuration command. Update
`YoutubeConfig.set` to use a key-generic signature tying `K extends keyof
YoutubeConfigShape` to `YoutubeConfigShape[K]`, and narrow `unwrapped` against
`typeof current` before calling it so the compiler enforces the key/value
relationship.
Sources: Coding guidelines, Learnings
| queue | ||
| .command("watch [ids...]") | ||
| .description("Stream job events until they finish; no ids watches everything active") | ||
| .option("--jsonl", "One JSON object per line") | ||
| .option("--timeout <sec>", "Give up after N seconds") | ||
| .option("--no-children", "Do not follow jobs spawned by the watched ones") | ||
| .action(async (ids: string[], opts: WatchOpts) => { | ||
| const yt = await getYoutube(); | ||
| // Strict, because dropping an unparseable id silently widens the command: | ||
| // with no ids left, `watch` means "every active job", so `queue watch typo` | ||
| // would quietly stream the whole queue instead of failing. | ||
| const invalid = ids.filter((id) => positiveInt(id) === null); | ||
|
|
||
| if (invalid.length > 0) { | ||
| out.error(`Not a job id: ${invalid.join(", ")}. Pass numeric ids, or none to watch everything active.`); | ||
| process.exitCode = 1; | ||
| return; | ||
| } | ||
|
|
||
| const jobIds = ids.map((id) => Number.parseInt(id, 10)); | ||
| const timeoutSec = opts.timeout === undefined ? null : positiveInt(opts.timeout); | ||
|
|
||
| if (opts.timeout !== undefined && timeoutSec === null) { | ||
| // Silently dropping this disabled the safety timeout entirely, which | ||
| // is the opposite of what the flag was reached for. | ||
| out.error(`--timeout must be a positive whole number of seconds, got "${opts.timeout}".`); | ||
| process.exitCode = 1; | ||
| return; | ||
| } | ||
|
|
||
| for await (const event of yt.queue.watch({ | ||
| actor: CLI_ACTOR, | ||
| ...(jobIds.length > 0 ? { jobIds } : {}), | ||
| followChildren: opts.noChildren !== true, | ||
| ...(timeoutSec === null ? {} : { timeoutMs: timeoutSec * 1000 }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
commander negatable option --no-foo stores opts.foo false
💡 Result:
In Commander.js, defining a boolean option with a leading --no- (e.g., --no-foo) allows users to explicitly set the corresponding option value to false [1][2]. Key behaviors of this feature include: 1. Negation: When the flag --no-foo is used on the command line, the value of opts.foo is set to false [1][2]. 2. Default Behavior: If the option is defined as --no-foo without a corresponding positive option (like --foo), Commander automatically sets the default value of opts.foo to true [1][2]. 3. Interaction with Defaults: If you define an option with a specific default value (e.g., program.option('--no-foo', 'description', true)), the negatable flag will still flip that value to false when used. However, note that some users have noted ambiguity regarding whether the default itself should be considered inverted; official behavior maintains that --no-foo defines an option named "foo," and the default value acts as the starting value for that option [3]. Example usage: program.option('--no-sauce', 'Remove sauce').parse; // If --no-sauce is not used, options.sauce is true. // If --no-sauce is used, options.sauce is false.
Citations:
- 1: https://github.com/tj/commander.js/tree/master?tab=readme-ov-file
- 2: https://github.com/tj/commander.js/blob/v15.0.0/Readme.md
- 3: --no is bugged with default arguments tj/commander.js#2068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package manager files ==\n'
git ls-files package.json bun.lockb bun.lock yarn.lock pnpm-lock.yaml 2>/dev/null || true
printf '\n== relevant file excerpt ==\n'
sed -n '1,360p' src/youtube/commands/queue.ts | cat -n
printf '\n== commander references ==\n'
rg -n 'commander|option\("--no-|noChildren|children' package.json src -g '!**/dist/**' -g '!**/build/**'Repository: genesiscz/GenesisTools
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== commander package version candidates ==\n'
fd -a 'package.json' . | xargs -r rg -n '"commander"|`@commander`|option\("--no-'
printf '\n== WatchOpts definition and usage ==\n'
rg -n 'interface WatchOpts|type WatchOpts|noChildren|children|followChildren' src/youtube/commands/queue.ts src -g '!**/dist/**' -g '!**/build/**'Repository: genesiscz/GenesisTools
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package.json commander version ==\n'
rg -n '"commander"|`@commander`' package.json bun.lock bun.lockb yarn.lock pnpm-lock.yaml 2>/dev/null || true
printf '\n== queue.ts WatchOpts and option usage ==\n'
sed -n '1,340p' src/youtube/commands/queue.ts | nl -ba | sed -n '1,340p' | rg -n 'WatchOpts|noChildren|children|followChildren|--no-children|option\("--no-children"'Repository: genesiscz/GenesisTools
Length of output: 2388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,340p' src/youtube/commands/queue.ts | cat -n | sed -n '1,340p' | rg -n 'interface WatchOpts|jsonl|timeout|noChildren|children|followChildren|--no-children|command\("watch'Repository: genesiscz/GenesisTools
Length of output: 1776
🌐 Web query:
Commander 14 negatable option --no-children stores option under positive key children false
💡 Result:
In the Commander.js library, a negatable option defined as --no-children does indeed store its value under the positive key children [1][2]. When you define an option with a --no- prefix, Commander.js maps that option to the base name (in this case, children) and assigns it a boolean value [1][2]. Specifically, if the user provides the --no-children flag, the children property in the options object will be set to false [1][2]. Additionally, if a negatable option is defined alone without a corresponding positive option (e.g., --no-children without --children), Commander.js historically defaults the value of the children property to true [1][2]. This behavior is a built-in feature designed to support common CLI conventions where a flag might be enabled by default and the user can pass a --no- variant to disable it [1][2]. To avoid having the option default to true, you can explicitly define a default value or include both the positive and negative forms, which allows Commander.js to manage the state more predictably [1][3][4].
Citations:
- 1: https://github.com/tj/commander.js/tree/master?tab=readme-ov-file
- 2: https://github.com/tj/commander.js/blob/v15.0.0/Readme.md
- 3: fixes behavior of --no-* options tj/commander.js#795
- 4: Make order not matter for adding combo positive/negative options tj/commander.js#2405
Use children for this option key
--no-children maps to opts.children === false in Commander 14, so opts.noChildren never gets set and followChildren stays enabled. Update WatchOpts and read opts.children !== false instead.
🤖 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/youtube/commands/queue.ts` around lines 249 - 283, The watch command
reads the wrong Commander option property for --no-children. Update WatchOpts
and the queue.watch invocation in the watch action to use opts.children, setting
followChildren based on opts.children !== false so the flag disables child
following.
| @@ -156,7 +181,7 @@ export function resolveUser(req: Request, url: URL, db: YoutubeDatabase): YtUser | |||
| * prefix are ignored here — they may be service keys handled elsewhere. | |||
| */ | |||
| export function requireUser(req: Request, url: URL, db: YoutubeDatabase): YtUser | Response { | |||
| const presented = extractBearerToken(req) ?? url.searchParams.get("access_token"); | |||
| const presented = extractPresentedToken(req, url); | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
requireUser's doc comment no longer matches its token sources. It still names only Bearer + ?access_token=, but it now resolves through extractPresentedToken, which also accepts ?key= — the exact drift this refactor was meant to eliminate.
📝 Proposed doc fix
- * Token source: `Authorization: Bearer ytu_…` header first, then
- * `?access_token=` query param (WS-style fallback). Tokens without the `ytu_`
- * prefix are ignored here — they may be service keys handled elsewhere.
+ * Token source: every channel `extractPresentedToken` accepts (Bearer header,
+ * then `?access_token=`, then `?key=`). Tokens without the `ytu_` prefix are
+ * ignored here — they may be service keys handled elsewhere.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Returns the authenticated user, or a ready 401 JSON Response. Never throws. | |
| * | |
| * Token source: every channel `extractPresentedToken` accepts (Bearer header, | |
| * then `?access_token=`, then `?key=`). Tokens without the `ytu_` prefix are | |
| * ignored here — they may be service keys handled elsewhere. | |
| */ | |
| export function requireUser(req: Request, url: URL, db: YoutubeDatabase): YtUser | Response { | |
| const presented = extractPresentedToken(req, url); |
🤖 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/youtube/lib/server/auth.ts` around lines 176 - 184, Update the
documentation for requireUser to accurately list all token sources resolved by
extractPresentedToken, including the ?key= query parameter alongside the
existing Authorization Bearer header and ?access_token= fallback. Keep the
existing prefix-handling and non-throwing behavior descriptions unchanged.
| let raw: unknown; | ||
|
|
||
| try { | ||
| raw = await req.json(); | ||
| } catch (err) { | ||
| logger.debug({ err, path: url.pathname }, "youtube API: pipeline enqueue body was not valid JSON"); | ||
|
|
||
| return jsonError("request body must be valid JSON", 400); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the shared safeJsonBody helper instead of an inline req.json() try/catch.
src/youtube/lib/server/body.ts already wraps this (strict SafeJSON.parse + logger.debug on malformed input). Since a malformed body and a non-object body both end in the same 400 here, safeJsonBody returning undefined slots in without changing responses.
Based on learnings: route files under src/youtube/lib/server/routes/*.ts should import and reuse the shared safeJsonBody helper from src/youtube/lib/server/body.ts rather than duplicating JSON-parsing logic.
🤖 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/youtube/lib/server/routes/pipeline.ts` around lines 33 - 41, Replace the
inline req.json() try/catch in the pipeline enqueue handler with the shared
safeJsonBody helper from body.ts, adding the import and passing the request,
logger, and pathname as required. Treat an undefined result as the existing 400
invalid-body response, preserving the current behavior for malformed or
non-object JSON.
Source: Learnings
| opts.db.upsertChannel({ handle: parsed.channel as ChannelHandle }); | ||
| opts.db.upsertVideo({ | ||
| id: parsed.videoId, | ||
| channelHandle: parsed.channel as ChannelHandle, | ||
| title: parsed.title, | ||
| uploadDate: parsed.uploadDate, | ||
| durationSec: parsed.durationSec, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize parsed.channel before writing it to the database.
parsed.channel comes straight from a hand-editable export file with no format validation beyond typeof === "string", then gets cast as ChannelHandle and written via upsertChannel/upsertVideo. A file missing the @ prefix (or otherwise non-canonical) creates a channel/video row under a handle that won't match the @-prefixed convention used everywhere else, breaking later channels.ensure()/videos.list({ channel }) lookups.
🛠️ Proposed fix
+import { normaliseHandle } from "`@app/youtube/lib/queue`";
+
...
- opts.db.upsertChannel({ handle: parsed.channel as ChannelHandle });
+ const channelHandle = normaliseHandle(parsed.channel) as ChannelHandle;
+ opts.db.upsertChannel({ handle: channelHandle });
opts.db.upsertVideo({
id: parsed.videoId,
- channelHandle: parsed.channel as ChannelHandle,
+ channelHandle,Based on learnings, "any channel-scope scopeValue that will be persisted... must be normalized to the canonical @-prefixed ChannelHandle form. Use the existing normaliseHandle logic (defined in src/youtube/lib/queue.ts...)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opts.db.upsertChannel({ handle: parsed.channel as ChannelHandle }); | |
| opts.db.upsertVideo({ | |
| id: parsed.videoId, | |
| channelHandle: parsed.channel as ChannelHandle, | |
| title: parsed.title, | |
| uploadDate: parsed.uploadDate, | |
| durationSec: parsed.durationSec, | |
| }); | |
| const channelHandle = normaliseHandle(parsed.channel) as ChannelHandle; | |
| opts.db.upsertChannel({ handle: channelHandle }); | |
| opts.db.upsertVideo({ | |
| id: parsed.videoId, | |
| channelHandle, | |
| title: parsed.title, | |
| uploadDate: parsed.uploadDate, | |
| durationSec: parsed.durationSec, | |
| }); |
🤖 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/youtube/lib/transcript-export.ts` around lines 272 - 279, Normalize
parsed.channel with the existing normaliseHandle logic before persistence in the
transcript export flow. Reuse the resulting canonical ChannelHandle for both
opts.db.upsertChannel and the channelHandle field in opts.db.upsertVideo, rather
than casting the raw parsed value directly.
Source: Learnings
| if (opts.captionsOnly && opts.forceTranscribe) { | ||
| logger.warn( | ||
| { videoId: opts.videoId }, | ||
| "youtube transcribe received conflicting captionsOnly and forceTranscribe options" | ||
| ); | ||
| logger.info({ videoId: opts.videoId }, "youtube captions-only miss"); | ||
| throw new NoCaptionsError(opts.videoId); | ||
| } | ||
|
|
||
| if (!opts.forceTranscribe) { | ||
| const fromCaptions = await this.tryCaptions({ videoId: opts.videoId, lang: opts.lang }); | ||
|
|
||
| if (fromCaptions) { | ||
| return fromCaptions; | ||
| } | ||
|
|
||
| if (opts.captionsOnly) { | ||
| logger.info({ videoId: opts.videoId }, "youtube captions-only miss"); | ||
| throw new NoCaptionsError(opts.videoId); | ||
| } | ||
|
|
||
| opts.onProgress?.({ phase: "audio", message: "no captions available — preparing AI transcription" }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Distinguish the conflicting-options log from the real captions-miss log.
The captionsOnly && forceTranscribe branch logs the same "youtube captions-only miss" info message (line 176) as the genuine miss branch (line 188) after tryCaptions actually runs — but this branch never attempts a caption lookup at all. Reusing the message makes it impossible to tell from logs alone which condition threw NoCaptionsError.
♻️ Proposed fix
if (opts.captionsOnly && opts.forceTranscribe) {
logger.warn(
{ videoId: opts.videoId },
"youtube transcribe received conflicting captionsOnly and forceTranscribe options"
);
- logger.info({ videoId: opts.videoId }, "youtube captions-only miss");
throw new NoCaptionsError(opts.videoId);
}As per path instructions, "Log enough context to reconstruct execution from logs alone, including decision branches, external-resource accesses, configuration resolution, and result counts."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (opts.captionsOnly && opts.forceTranscribe) { | |
| logger.warn( | |
| { videoId: opts.videoId }, | |
| "youtube transcribe received conflicting captionsOnly and forceTranscribe options" | |
| ); | |
| logger.info({ videoId: opts.videoId }, "youtube captions-only miss"); | |
| throw new NoCaptionsError(opts.videoId); | |
| } | |
| if (!opts.forceTranscribe) { | |
| const fromCaptions = await this.tryCaptions({ videoId: opts.videoId, lang: opts.lang }); | |
| if (fromCaptions) { | |
| return fromCaptions; | |
| } | |
| if (opts.captionsOnly) { | |
| logger.info({ videoId: opts.videoId }, "youtube captions-only miss"); | |
| throw new NoCaptionsError(opts.videoId); | |
| } | |
| opts.onProgress?.({ phase: "audio", message: "no captions available — preparing AI transcription" }); | |
| } | |
| if (opts.captionsOnly && opts.forceTranscribe) { | |
| logger.warn( | |
| { videoId: opts.videoId }, | |
| "youtube transcribe received conflicting captionsOnly and forceTranscribe options" | |
| ); | |
| throw new NoCaptionsError(opts.videoId); | |
| } | |
| if (!opts.forceTranscribe) { | |
| const fromCaptions = await this.tryCaptions({ videoId: opts.videoId, lang: opts.lang }); | |
| if (fromCaptions) { | |
| return fromCaptions; | |
| } | |
| if (opts.captionsOnly) { | |
| logger.info({ videoId: opts.videoId }, "youtube captions-only miss"); | |
| throw new NoCaptionsError(opts.videoId); | |
| } | |
| opts.onProgress?.({ phase: "audio", message: "no captions available — preparing AI transcription" }); | |
| } |
🤖 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/youtube/lib/transcripts.ts` around lines 171 - 193, Update the
conflicting-options branch in the transcript flow to use a distinct info-log
message from the genuine captions-miss path after tryCaptions. Keep the existing
“youtube captions-only miss” message only for the branch where caption lookup
was attempted and returned no result, while preserving the NoCaptionsError
behavior and relevant videoId context in both branches.
Source: Path instructions
| const params = ctx.job.params ?? {}; | ||
| const limit = typeof params.limit === "number" ? params.limit : undefined; | ||
|
|
||
| if (params.limit !== undefined && limit === undefined) { | ||
| logger.warn( | ||
| { jobId: ctx.job.id, valueType: typeof params.limit }, | ||
| "youtube discover ignoring invalid channel sync limit" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
typeof === "number" accepts NaN, 0 and negatives for limit.
params is now free-form on the enqueue path (parseEnqueueBody only requires a plain object), so {"params":{"limit":-1}} or {"limit":0} passes this guard and flows into channels.sync → deps.listChannelVideos. Require a positive safe integer, same as the CLI does for its numeric args.
🛡️ Proposed guard
- const limit = typeof params.limit === "number" ? params.limit : undefined;
+ const limit =
+ typeof params.limit === "number" && Number.isSafeInteger(params.limit) && params.limit > 0
+ ? params.limit
+ : undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const params = ctx.job.params ?? {}; | |
| const limit = typeof params.limit === "number" ? params.limit : undefined; | |
| if (params.limit !== undefined && limit === undefined) { | |
| logger.warn( | |
| { jobId: ctx.job.id, valueType: typeof params.limit }, | |
| "youtube discover ignoring invalid channel sync limit" | |
| ); | |
| } | |
| const params = ctx.job.params ?? {}; | |
| const limit = | |
| typeof params.limit === "number" && Number.isSafeInteger(params.limit) && params.limit > 0 | |
| ? params.limit | |
| : undefined; | |
| if (params.limit !== undefined && limit === undefined) { | |
| logger.warn( | |
| { jobId: ctx.job.id, valueType: typeof params.limit }, | |
| "youtube discover ignoring invalid channel sync limit" | |
| ); | |
| } |
🤖 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/youtube/lib/youtube.ts` around lines 407 - 415, Update the limit
validation in the discover job flow around params.limit so it accepts only
positive safe integers, rejecting NaN, zero, negative, fractional, and unsafe
numeric values before calling channels.sync or deps.listChannelVideos. Preserve
the existing undefined behavior and invalid-value warning, including the job ID
and value type.
1 participant
What
The largest group in the split: a queue/ask rebuild plus extension and server fixes.
Ask / sessions
ask_threadswidened into user-scopedask_sessions, and ask sessions now run over scoped video sets rather than a single video.Queue
qaIndexstage, and the retrieval bucket mismatch it exposed is fixed.Refactor
Extension
Server
Why
The silent captions-miss escalation is the finding worth calling out: it turned a free path into a billed one with no signal at the call site.
Scope
src/youtube/**,src/utils/ai/provider-env.ts,src/utils/DashboardApp/launchd.ts.Split out of #296. File-disjoint from the sibling PRs.
Summary by CodeRabbit