Repository navigation
Bad Code Cleanup (Cycle 0029) - #16
Merged
Merged
Conversation
Bundle all 4 remaining bad-code items into one cycle: depth limits, YAML library, GitHub API validation, god class decomposition.
1. Directory walks: maxDepth=10 and symlink guard on collectMarkdownFiles (index.ts) and collectFiles (drift.ts) 2. Frontmatter: new src/frontmatter.ts using yaml library. Workspace delegates to it. Removes all manual string-slicing YAML. 3. GitHub API: Zod schemas validate issue and comment responses. No more any casts. Test mocks updated with required id field.
Extracted two focused modules from src/index.ts: - src/renderers.ts (188 lines): renderBearing, renderWitnessDoc, renderDesignDoc, renderRetroDoc, titleCase - src/frontmatter.ts (100 lines): readFrontmatter, updateFrontmatter, readBody, updateBody, readHeading (using yaml library) src/index.ts reduced from 854 to 604 lines (30% reduction). Workspace delegates to these modules. Public API unchanged.
Hill met. Bad-code lane cleared: depth limits, YAML library, GitHub API validation, god class decomposition. Workspace 854->604 lines. 127 tests pass. No drift.
Summary by CodeRabbitRelease Notes
WalkthroughThis PR documents and implements a comprehensive cleanup addressing four technical debts: introducing recursion depth limits and symlink guards to directory traversal, replacing manual YAML string-slicing with the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index.ts (2)
551-567: 🧹 Nitpick | 🔵 TrivialDuplicate logic:
collectMarkdownFilesanddrift.ts::collectFilesare near-identical.Both functions implement the same depth-limiting, symlink-skipping recursive directory traversal. The only difference is the file filter predicate. This is textbook DRY violation.
Consider extracting a shared
walkDirectoryhelper that accepts a predicate, used by both modules.♻️ Proposed unified implementation
Create a shared utility (e.g., in a
src/fs-utils.ts):export function walkDirectory( root: string, predicate: (entry: Dirent) => boolean, maxDepth = 10 ): string[] { if (maxDepth <= 0 || !existsSync(root)) { return []; } const files: string[] = []; for (const entry of readdirSync(root, { withFileTypes: true })) { const path = resolve(root, entry.name); if (entry.isDirectory() && !entry.isSymbolicLink()) { files.push(...walkDirectory(path, predicate, maxDepth - 1)); } else if (predicate(entry)) { files.push(path); } } return files.sort((a, b) => a.localeCompare(b)); }Then in
src/index.ts:const collectMarkdownFiles = (root: string, maxDepth = 10) => walkDirectory(root, (e) => e.isFile() && e.name.endsWith('.md'), maxDepth);And in
src/drift.ts:const collectTestFiles = (root: string) => walkDirectory(root, (e) => e.isFile() && /\.(?:test|spec)\.[cm]?[jt]sx?$/.test(e.name));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 551 - 567, collectMarkdownFiles duplicates logic in drift.ts::collectFiles; extract a generic walkDirectory(root, predicate, maxDepth=10) helper (e.g., in src/fs-utils.ts) that performs the depth-limited, symlink-skipping recursion and returns a sorted list, then reimplement collectMarkdownFiles as a thin wrapper calling walkDirectory(root, e => e.isFile() && e.name.endsWith('.md'), maxDepth) and update drift.ts to use walkDirectory with its test/spec filename predicate to remove duplicated traversal code.
542-542:⚠️ Potential issue | 🟡 MinorSurviving
anycast contradicts PR objective.The PR claims to remove
anycasts, yeterror: anypersists here. This is the exact pattern you're supposedly eliminating.🔧 Proposed fix using `unknown` with type narrowing
- } catch (error: any) { - if (error.killed || error.signal === 'SIGTERM') { + } catch (error: unknown) { + const execError = error as { killed?: boolean; signal?: string; stdout?: string; stderr?: string }; + if (execError.killed || execError.signal === 'SIGTERM') { throw new MethodError(`Command timed out: ${fullCommand}`); } - return (error.stdout ?? '') + (error.stderr ?? ''); + return (execError.stdout ?? '') + (execError.stderr ?? ''); }Or better yet, define an interface for
ExecFileErrorand narrow properly:interface ExecFileError { killed?: boolean; signal?: string; stdout?: string; stderr?: string; } function isExecFileError(err: unknown): err is ExecFileError { return typeof err === 'object' && err !== null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` at line 542, The catch clause currently types the caught value as `any` (catch (error: any)), which violates the PR goal; change it to `catch (error: unknown)` and add a type guard (e.g., `isExecFileError`) or an `ExecFileError` interface to narrow `error` before reading properties like `killed`, `signal`, `stdout`, or `stderr`; update any code in the same scope that references those properties to first assert `isExecFileError(error)` (or otherwise safely handle non-object errors) so there are no `any` casts left and all property access is type-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapters/github.ts`:
- Around line 14-17: GitHubCommentResponseSchema is too strict (requires user
object) and will throw on comments with user: null; update the schema to allow
user to be nullable (e.g., user: z.object({ login: z.string() }).nullable()) and
then change the parsing/processing path where
GitHubCommentResponseSchema.parse(...) is called (and where c.user.login is
accessed) to defensively handle null users—either filter out comments with null
user before accessing login or use a safe accessor and provide a fallback (or
mark as deleted/system). Also wrap the parse call in a try-catch to
log/paranoia-handle malformed payloads instead of crashing. Ensure you modify
the symbols GitHubCommentResponseSchema and the code that iterates over parsed
comments (the parse(...) call and the c.user.login usage) accordingly.
In `@src/frontmatter.ts`:
- Around line 56-63: The code duplicates the FM_END search that parseDoc already
computes; modify parseDoc to return the frontmatter end offset or the extracted
frontmatter block (e.g., return an object with endOffset or frontmatterText),
then update the caller in src/frontmatter.ts to use that returned end offset or
block instead of re-running doc.raw.indexOf(FM_END,...). Replace the indexOf
logic inside the if (doc.raw.startsWith(FM_DELIMITER)) branch with the value
from parseDoc and call writeFileSync(path, serializeDoc(merged, afterFm),
'utf8') using the slice computed from the returned end offset or the returned
afterFm directly.
- Around line 30-33: The current loop in frontmatter.ts converts every YAML
value to String(value), which corrupts arrays/objects; instead stop coercing and
preserve original types by changing the frontmatter record type from
Record<string, string> to Record<string, unknown> and assign values directly
(use parsed and frontmatter as-is), then update any consumers such as
updateFrontmatter to accept Record<string, unknown>; alternatively, if you
prefer rejecting non-primitives, add a runtime check in the loop that throws
when a value is an object/array (Array.isArray(value) || typeof value ===
'object') and keep the Record<string, string> contract—pick one approach and
apply it consistently to parsed, frontmatter, and updateFrontmatter.
- Around line 77-91: updateBody currently calls parseDoc(path) then calls
readHeading(path), causing two readFileSync calls and unnecessary I/O; also
readHeading may return '' and produce a stray "# " line in the output. Fix by
extracting the heading from the already-parsed doc (use the parsed document
content returned by parseDoc instead of calling readHeading) and only write the
heading block if the extracted heading is non-empty; update updateBody to use
that in the writeFileSync paths so you avoid a second file read and prevent
emitting an empty "# " heading line.
In `@src/renderers.ts`:
- Line 59: The title assignment using readHeading(options.cycle.designDoc) can
throw if the file is missing; update the logic in renderers.ts (where title is
computed) to guard the call to readHeading — either check file existence
(fs.existsSync or similar) or wrap readHeading(...) in a try/catch and on
error/falsy result fall back to titleCase(options.cycle.slug), mirroring the
error-handling approach used in renderBearing so missing designDoc files don't
throw.
- Line 153: Multiple places call readHeading without guarding against ENOENT
(e.g., the line setting const title = readHeading(options.cycle.designDoc) ||
titleCase(options.cycle.slug)), which repeats in renderBearing,
renderWitnessDoc, and renderRetroDoc; create a small helper (e.g.,
safeReadHeading or getTitleFromDocument) that accepts the designDoc path and a
fallback slug, wraps the readHeading call inside a try/catch that returns
undefined or the slug-derived title on ENOENT/any error, then replace the direct
readHeading usage in renderBearing, renderWitnessDoc, renderRetroDoc (and this
line) to call the helper so the ENOENT handling is centralized and duplicated
try/catch blocks are removed.
- Around line 19-22: The render code is performing synchronous file I/O via
readHeading(cycle.designDoc) inside latestShips.map (used to build shipLines),
which can throw ENOENT and crash rendering; change this by either (A) wrapping
the readHeading call in a try/catch inside the map so any exception falls back
to titleCase(cycle.slug) (e.g., call readHeading inside try and on catch use
titleCase), or (B) remove file I/O from the renderer by pre-fetching headings
upstream and passing a heading property on each cycle into the render function
(use that heading if present otherwise call titleCase); update shipLines
construction accordingly to reference the safe/externally-provided heading
instead of directly calling readHeading.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 551-567: collectMarkdownFiles duplicates logic in
drift.ts::collectFiles; extract a generic walkDirectory(root, predicate,
maxDepth=10) helper (e.g., in src/fs-utils.ts) that performs the depth-limited,
symlink-skipping recursion and returns a sorted list, then reimplement
collectMarkdownFiles as a thin wrapper calling walkDirectory(root, e =>
e.isFile() && e.name.endsWith('.md'), maxDepth) and update drift.ts to use
walkDirectory with its test/spec filename predicate to remove duplicated
traversal code.
- Line 542: The catch clause currently types the caught value as `any` (catch
(error: any)), which violates the PR goal; change it to `catch (error: unknown)`
and add a type guard (e.g., `isExecFileError`) or an `ExecFileError` interface
to narrow `error` before reading properties like `killed`, `signal`, `stdout`,
or `stderr`; update any code in the same scope that references those properties
to first assert `isExecFileError(error)` (or otherwise safely handle non-object
errors) so there are no `any` casts left and all property access is type-safe.
🪄 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: c0644b55-9ed7-4100-a705-9024a2841f77
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
docs/design/0029-bad-code-cleanup/bad-code-cleanup.mddocs/method/backlog/bad-code/PROCESS_manual-yaml-frontmatter-parsing.mddocs/method/backlog/bad-code/PROCESS_no-depth-limit-in-directory-walk.mddocs/method/backlog/bad-code/PROCESS_unvalidated-github-api-responses.mddocs/method/backlog/bad-code/PROCESS_workspace-god-class.mddocs/method/retro/0029-bad-code-cleanup/bad-code-cleanup.mddocs/method/retro/0029-bad-code-cleanup/witness/verification.mdpackage.jsonsrc/adapters/github.tssrc/drift.tssrc/frontmatter.tssrc/index.tssrc/renderers.tstests/github-adapter.test.ts
💤 Files with no reviewable changes (4)
- docs/method/backlog/bad-code/PROCESS_no-depth-limit-in-directory-walk.md
- docs/method/backlog/bad-code/PROCESS_workspace-god-class.md
- docs/method/backlog/bad-code/PROCESS_unvalidated-github-api-responses.md
- docs/method/backlog/bad-code/PROCESS_manual-yaml-frontmatter-parsing.md
Comment on lines
+14
to
+17
| const GitHubCommentResponseSchema = z.array(z.object({ | ||
| user: z.object({ login: z.string() }), | ||
| body: z.string(), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GitHub API return null for the user field in issue comments when the user is deleted?
💡 Result:
Usually no: when a GitHub personal account is deleted, GitHub keeps existing issue/PR comments and re-associates them with the ghost user rather than removing them, so API consumers generally still see a user object (for ghost) on those comments. [1], [2]
However, in real-world API responses there are cases where GitHub returns a null user for records tied to a deleted/blocked user, so you should defensively handle user: null even if your primary expectation is user.login == "ghost". [3]
Sources: GitHub REST “issue comments” docs (response shape includes user) [1]; GitHub docs on account deletion associating comments with ghost [2]; downstream integration reports of null users from GitHub API [3].
🏁 Script executed:
fd "github.ts" --type fRepository: flyingrobots/method
Length of output: 85
🏁 Script executed:
cat -n src/adapters/github.ts | head -60Repository: flyingrobots/method
Length of output: 1992
🏁 Script executed:
cat -n src/adapters/github.ts | grep -A 20 "fetchComments"Repository: flyingrobots/method
Length of output: 2020
The schema validation will detonate on user: null — this is a production bug.
GitHub's API genuinely returns user: null for comments from deleted accounts or system messages. Your Zod schema at line 15 demands z.object({ login: z.string() }), which will fail catastrophically when GitHubCommentResponseSchema.parse(data) encounters these records on line 198. There is zero defensive handling—no try-catch, no filtering, no null checks before accessing c.user.login on line 200.
This isn't theoretical. Repositories with any history have deleted users. Your sync will crater.
Required fix
const GitHubCommentResponseSchema = z.array(z.object({
- user: z.object({ login: z.string() }),
+ user: z.object({ login: z.string() }).nullable(),
body: z.string(),
})); private async fetchComments(number: number): Promise<{ user: string; body: string }[]> {
const data = await this.ghFetch(`/repos/${this.owner}/${this.repo}/issues/${number}/comments`);
const parsed = GitHubCommentResponseSchema.parse(data);
- return parsed.map((c) => ({
- user: c.user.login,
+ return parsed
+ .filter((c) => c.user !== null)
+ .map((c) => ({
+ user: c.user!.login,
body: c.body,
}));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapters/github.ts` around lines 14 - 17, GitHubCommentResponseSchema is
too strict (requires user object) and will throw on comments with user: null;
update the schema to allow user to be nullable (e.g., user: z.object({ login:
z.string() }).nullable()) and then change the parsing/processing path where
GitHubCommentResponseSchema.parse(...) is called (and where c.user.login is
accessed) to defensively handle null users—either filter out comments with null
user before accessing login or use a safe accessor and provide a fallback (or
mark as deleted/system). Also wrap the parse call in a try-catch to
log/paranoia-handle malformed payloads instead of crashing. Ensure you modify
the symbols GitHubCommentResponseSchema and the code that iterates over parsed
comments (the parse(...) call and the c.user.login usage) accordingly.
Comment on lines
+30
to
+33
| if (parsed !== null && typeof parsed === 'object') { | ||
| for (const [key, value] of Object.entries(parsed)) { | ||
| frontmatter[key] = String(value); | ||
| } |
There was a problem hiding this comment.
Lossy String() coercion silently corrupts non-scalar YAML values.
If frontmatter contains arrays, nested objects, or typed values (e.g., tags: [a, b]), String(value) produces "a,b" or "[object Object]". On round-trip via updateFrontmatter, the original structure is destroyed.
Either:
- Validate that values are primitives and throw on complex types
- Preserve original types in the record (change signature to
Record<string, unknown>) - Document this limitation explicitly
🛡️ Option 1: Validate and reject complex types
for (const [key, value] of Object.entries(parsed)) {
- frontmatter[key] = String(value);
+ if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') {
+ frontmatter[key] = String(value);
+ } else {
+ throw new Error(`Unsupported frontmatter value type for key "${key}"`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (parsed !== null && typeof parsed === 'object') { | |
| for (const [key, value] of Object.entries(parsed)) { | |
| frontmatter[key] = String(value); | |
| } | |
| if (parsed !== null && typeof parsed === 'object') { | |
| for (const [key, value] of Object.entries(parsed)) { | |
| if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') { | |
| frontmatter[key] = String(value); | |
| } else { | |
| throw new Error(`Unsupported frontmatter value type for key "${key}"`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontmatter.ts` around lines 30 - 33, The current loop in frontmatter.ts
converts every YAML value to String(value), which corrupts arrays/objects;
instead stop coercing and preserve original types by changing the frontmatter
record type from Record<string, string> to Record<string, unknown> and assign
values directly (use parsed and frontmatter as-is), then update any consumers
such as updateFrontmatter to accept Record<string, unknown>; alternatively, if
you prefer rejecting non-primitives, add a runtime check in the loop that throws
when a value is an object/array (Array.isArray(value) || typeof value ===
'object') and keep the Record<string, string> contract—pick one approach and
apply it consistently to parsed, frontmatter, and updateFrontmatter.
Comment on lines
+56
to
+63
| if (doc.raw.startsWith(FM_DELIMITER)) { | ||
| const end = doc.raw.indexOf(FM_END, FM_DELIMITER.length); | ||
| if (end !== -1) { | ||
| const afterFm = doc.raw.slice(end + FM_END.length); | ||
| writeFileSync(path, serializeDoc(merged, afterFm), 'utf8'); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant delimiter search — parseDoc already computed this.
The indexOf(FM_END, ...) logic is duplicated from parseDoc. Consider having parseDoc return the end offset or the original frontmatter block directly to avoid repeated parsing of the same content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontmatter.ts` around lines 56 - 63, The code duplicates the FM_END
search that parseDoc already computes; modify parseDoc to return the frontmatter
end offset or the extracted frontmatter block (e.g., return an object with
endOffset or frontmatterText), then update the caller in src/frontmatter.ts to
use that returned end offset or block instead of re-running
doc.raw.indexOf(FM_END,...). Replace the indexOf logic inside the if
(doc.raw.startsWith(FM_DELIMITER)) branch with the value from parseDoc and call
writeFileSync(path, serializeDoc(merged, afterFm), 'utf8') using the slice
computed from the returned end offset or the returned afterFm directly.
Comment on lines
+77
to
+91
| export function updateBody(path: string, newBody: string): void { | ||
| const doc = parseDoc(path); | ||
| const heading = readHeading(path); | ||
|
|
||
| if (doc.raw.startsWith(FM_DELIMITER)) { | ||
| const end = doc.raw.indexOf(FM_END, FM_DELIMITER.length); | ||
| if (end !== -1) { | ||
| const fmBlock = doc.raw.slice(0, end + FM_END.length); | ||
| writeFileSync(path, `${fmBlock}\n# ${heading}\n\n${newBody.trim()}\n`, 'utf8'); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| writeFileSync(path, `# ${heading}\n\n${newBody.trim()}\n`, 'utf8'); | ||
| } |
There was a problem hiding this comment.
Double file read: parseDoc then readHeading both call readFileSync.
Line 78 reads the file via parseDoc, then line 79 reads it again via readHeading. Wasteful I/O.
Additionally, if the original file has no heading, readHeading returns '', and line 85/90 writes # — an empty heading line that may confuse downstream parsers.
♻️ Extract heading from already-read content
export function updateBody(path: string, newBody: string): void {
const doc = parseDoc(path);
- const heading = readHeading(path);
+ const heading = extractHeadingFromContent(doc.body);
if (doc.raw.startsWith(FM_DELIMITER)) {
const end = doc.raw.indexOf(FM_END, FM_DELIMITER.length);
if (end !== -1) {
const fmBlock = doc.raw.slice(0, end + FM_END.length);
- writeFileSync(path, `${fmBlock}\n# ${heading}\n\n${newBody.trim()}\n`, 'utf8');
+ const headingLine = heading ? `# ${heading}\n\n` : '';
+ writeFileSync(path, `${fmBlock}\n${headingLine}${newBody.trim()}\n`, 'utf8');
return;
}
}
- writeFileSync(path, `# ${heading}\n\n${newBody.trim()}\n`, 'utf8');
+ const headingLine = heading ? `# ${heading}\n\n` : '';
+ writeFileSync(path, `${headingLine}${newBody.trim()}\n`, 'utf8');
}
+function extractHeadingFromContent(body: string): string {
+ for (const line of body.split(/\r?\n/u)) {
+ if (line.startsWith('# ')) {
+ return line.slice(2).trim();
+ }
+ }
+ return '';
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function updateBody(path: string, newBody: string): void { | |
| const doc = parseDoc(path); | |
| const heading = readHeading(path); | |
| if (doc.raw.startsWith(FM_DELIMITER)) { | |
| const end = doc.raw.indexOf(FM_END, FM_DELIMITER.length); | |
| if (end !== -1) { | |
| const fmBlock = doc.raw.slice(0, end + FM_END.length); | |
| writeFileSync(path, `${fmBlock}\n# ${heading}\n\n${newBody.trim()}\n`, 'utf8'); | |
| return; | |
| } | |
| } | |
| writeFileSync(path, `# ${heading}\n\n${newBody.trim()}\n`, 'utf8'); | |
| } | |
| export function updateBody(path: string, newBody: string): void { | |
| const doc = parseDoc(path); | |
| const heading = extractHeadingFromContent(doc.body); | |
| if (doc.raw.startsWith(FM_DELIMITER)) { | |
| const end = doc.raw.indexOf(FM_END, FM_DELIMITER.length); | |
| if (end !== -1) { | |
| const fmBlock = doc.raw.slice(0, end + FM_END.length); | |
| const headingLine = heading ? `# ${heading}\n\n` : ''; | |
| writeFileSync(path, `${fmBlock}\n${headingLine}${newBody.trim()}\n`, 'utf8'); | |
| return; | |
| } | |
| } | |
| const headingLine = heading ? `# ${heading}\n\n` : ''; | |
| writeFileSync(path, `${headingLine}${newBody.trim()}\n`, 'utf8'); | |
| } | |
| function extractHeadingFromContent(body: string): string { | |
| for (const line of body.split(/\r?\n/u)) { | |
| if (line.startsWith('# ')) { | |
| return line.slice(2).trim(); | |
| } | |
| } | |
| return ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontmatter.ts` around lines 77 - 91, updateBody currently calls
parseDoc(path) then calls readHeading(path), causing two readFileSync calls and
unnecessary I/O; also readHeading may return '' and produce a stray "# " line in
the output. Fix by extracting the heading from the already-parsed doc (use the
parsed document content returned by parseDoc instead of calling readHeading) and
only write the heading block if the extracted heading is non-empty; update
updateBody to use that in the writeFileSync paths so you avoid a second file
read and prevent emitting an empty "# " heading line.
Comment on lines
+19
to
+22
| const shipLines = latestShips.map(cycle => { | ||
| const title = readHeading(cycle.designDoc) || titleCase(cycle.slug); | ||
| return `- \`${cycle.name}\`: ${title}`; | ||
| }); |
There was a problem hiding this comment.
File I/O inside a "render" function — unexpected side effect and uncaught ENOENT risk.
readHeading(cycle.designDoc) performs synchronous file reads. If designDoc references a missing file, readFileSync throws ENOENT, crashing the render. The || titleCase(...) fallback only catches empty strings, not exceptions.
Consider either:
- Wrapping in try/catch with fallback to
titleCase - Pre-fetching headings upstream and passing them in
🛡️ Defensive try/catch fallback
const shipLines = latestShips.map(cycle => {
- const title = readHeading(cycle.designDoc) || titleCase(cycle.slug);
+ let title: string;
+ try {
+ title = readHeading(cycle.designDoc) || titleCase(cycle.slug);
+ } catch {
+ title = titleCase(cycle.slug);
+ }
return `- \`${cycle.name}\`: ${title}`;
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderers.ts` around lines 19 - 22, The render code is performing
synchronous file I/O via readHeading(cycle.designDoc) inside latestShips.map
(used to build shipLines), which can throw ENOENT and crash rendering; change
this by either (A) wrapping the readHeading call in a try/catch inside the map
so any exception falls back to titleCase(cycle.slug) (e.g., call readHeading
inside try and on catch use titleCase), or (B) remove file I/O from the renderer
by pre-fetching headings upstream and passing a heading property on each cycle
into the render function (use that heading if present otherwise call titleCase);
update shipLines construction accordingly to reference the
safe/externally-provided heading instead of directly calling readHeading.
| testResult: string; | ||
| driftResult: string; | ||
| }): string { | ||
| const title = readHeading(options.cycle.designDoc) || titleCase(options.cycle.slug); |
There was a problem hiding this comment.
Same ENOENT risk with readHeading — consider consistent error handling.
Same concern as renderBearing: if cycle.designDoc points to a non-existent file, this throws.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderers.ts` at line 59, The title assignment using
readHeading(options.cycle.designDoc) can throw if the file is missing; update
the logic in renderers.ts (where title is computed) to guard the call to
readHeading — either check file existence (fs.existsSync or similar) or wrap
readHeading(...) in a try/catch and on error/falsy result fall back to
titleCase(options.cycle.slug), mirroring the error-handling approach used in
renderBearing so missing designDoc files don't throw.
| outcome?: Outcome; | ||
| witnessDir: string; | ||
| }): string { | ||
| const title = readHeading(options.cycle.designDoc) || titleCase(options.cycle.slug); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same ENOENT risk — third occurrence of unguarded readHeading.
Pattern repeats across renderBearing, renderWitnessDoc, and renderRetroDoc. Consider a helper that wraps the try/catch logic once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderers.ts` at line 153, Multiple places call readHeading without
guarding against ENOENT (e.g., the line setting const title =
readHeading(options.cycle.designDoc) || titleCase(options.cycle.slug)), which
repeats in renderBearing, renderWitnessDoc, and renderRetroDoc; create a small
helper (e.g., safeReadHeading or getTitleFromDocument) that accepts the
designDoc path and a fallback slug, wraps the readHeading call inside a
try/catch that returns undefined or the slug-derived title on ENOENT/any error,
then replace the direct readHeading usage in renderBearing, renderWitnessDoc,
renderRetroDoc (and this line) to call the helper so the ENOENT handling is
centralized and duplicated try/catch blocks are removed.
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clears the bad-code lane — all 4 items resolved:
collectMarkdownFilesandcollectFileshavemaxDepth=10and skip symlinkssrc/frontmatter.tsusesyamlpackage; all manual string-slicing removedanycastssrc/renderers.ts(188 lines) andsrc/frontmatter.ts(100 lines); Workspace reduced from 854 to 604 linesTest plan