fix(completions): resolve Windows compatibility issues in zsh-installer tests #373
Conversation
WalkthroughModified file system permission checking in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)test/core/completions/installers/zsh-installer.test.ts (1)
🔇 Additional comments (5)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/file-system.ts (1)
88-119: Fix non-file path handling logic in canWriteFile.Lines 92-94 return
truewhen the path exists but is not a file (e.g., directory, symlink). This contradicts the function name and behavior: you cannot write to a directory in the same way you write to a file. Both call sites in the codebase (src/core/update.tsandsrc/core/completions/installers/zsh-installer.ts) expect file paths only.Change the logic to return
falsefor non-file paths, or rename the function if it's intentionally designed to handle both files and directories.
🧹 Nitpick comments (9)
src/commands/workflow/types.ts (1)
8-8: Consider usingas constfor literal type inference.The
SCHEMA_VERSIONconstant could benefit fromas constto enable literal type inference, making it more type-safe when used in schema validation.🔎 Proposed refinement
-export const SCHEMA_VERSION = 'poc-1'; +export const SCHEMA_VERSION = 'poc-1' as const;test/core/completions/installers/zsh-installer.test.ts (2)
196-208: Unix test path may not reliably fail in all environments.The Unix invalid path
/root/invalid/nonexistent/pathassumes/root/will deny write access, but this may not be true in all test environments (e.g., when tests run as root in containerized CI). Consider using a path under read-only filesystems like/sys/invalidor/proc/invalidfor more reliable failure.🔎 Alternative approach
// Create installer with non-existent/invalid home directory // Use a path that will fail on both Unix and Windows const invalidPath = process.platform === 'win32' ? 'Z:\\nonexistent\\invalid\\path' // Non-existent drive letter on Windows - : '/root/invalid/nonexistent/path'; // Permission-denied path on Unix + : '/sys/invalid/path'; // Read-only filesystem on Unix const invalidInstaller = new ZshInstaller(invalidPath);
508-519: Same reliability concern for Unix test path.This test has the same potential issue as the installation error test above -
/root/invalid/pathmay not reliably fail in all environments. Consider using/sys/invalid/pathfor consistency.POC_IMPLEMENTATION.md (1)
73-73: Optional: Address markdownlint warnings for better markdown hygiene.Static analysis flagged several markdown style issues:
- Line 73: Fenced code block missing language specifier (add
textorplaintext)- Multiple lines: Using
**bold**for section headings instead of proper heading levels (###)These don't affect readability but would improve markdown compliance and semantic structure.
🔎 Example fixes
For line 73:
-``` +```text .openspec/ ├── currentFor milestone subsections (example at line 181):
-**Deliverable** +### DeliverableAlso applies to: 117-253
src/commands/workflow/status.ts (1)
13-20: Consider reusing the existingfileExistsutility.A
fileExistshelper already exists insrc/utils/file-system.ts(lines 75-85) with the same implementation pattern. Consider importing and usingFileSystem.fileExiststo reduce code duplication across the codebase.src/commands/workflow/task.ts (2)
12-12: Unused import:SCHEMA_VERSION.
SCHEMA_VERSIONis imported but never used in this file. ThesaveTasksfunction instate.tsalready handles setting the schema version internally.🔎 Proposed fix
-import { Task, TaskStatus, SCHEMA_VERSION } from './types.js'; +import { Task, TaskStatus } from './types.js';
52-56: Inconsistent error handling compared torunTaskComplete.When a task is not found,
runTaskComplete(line 25) helpfully lists available tasks, butrunTaskStartdoes not. Consider adding the same helpful output for consistency.🔎 Proposed fix
const task = state.activeChange.tasks.find((t) => t.id === taskId); if (!task) { console.log(chalk.red(`Task "${taskId}" not found.`)); + console.log(chalk.dim('Available tasks: ' + state.activeChange.tasks.map((t) => t.id).join(', '))); process.exit(1); }src/commands/workflow/state.ts (2)
136-142: Unused parameter:notes.The
notesparameter is declared but never used in the function body. Either remove it or implement the intended functionality (e.g., storing notes on the task object).🔎 Proposed fix (if notes are not needed)
export async function updateTaskStatus( root: string, changeId: string, taskId: string, - status: Task['status'], - notes?: string + status: Task['status'] ): Promise<void> {
59-83: Add try-catch error handling around yaml.load() calls to handle malformed YAML gracefully.yaml.load() throws YAMLException on malformed content, and currently the
loadMeta()andloadTasks()functions (lines 65 and 81) have no error handling for parsing failures. For a more robust PoC, wrap these calls in try-catch blocks to log errors or return sensible defaults instead of letting exceptions propagate to callers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
POC_IMPLEMENTATION.md(1 hunks)package.json(2 hunks)src/cli/index.ts(2 hunks)src/commands/workflow/change.ts(1 hunks)src/commands/workflow/index.ts(1 hunks)src/commands/workflow/phase.ts(1 hunks)src/commands/workflow/state.ts(1 hunks)src/commands/workflow/status.ts(1 hunks)src/commands/workflow/task.ts(1 hunks)src/commands/workflow/types.ts(1 hunks)src/utils/file-system.ts(2 hunks)test/core/completions/installers/zsh-installer.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
POC_IMPLEMENTATION.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
POC_IMPLEMENTATION.md
📚 Learning: 2025-11-25T01:08:02.839Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Use `@/openspec/AGENTS.md` to learn how to create and apply change proposals, spec format and conventions, and project structure and guidelines
Applied to files:
POC_IMPLEMENTATION.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Applied to files:
POC_IMPLEMENTATION.md
📚 Learning: 2025-11-25T01:08:02.839Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Always open `@/openspec/AGENTS.md` when the request mentions planning or proposals (words like proposal, spec, change, plan), introduces new capabilities, breaking changes, architecture shifts, or performance/security work, or sounds ambiguous and needs the authoritative spec before coding
Applied to files:
POC_IMPLEMENTATION.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Use kebab-case, verb-led change IDs: `add-`, `update-`, `remove-`, `refactor-`
Applied to files:
src/commands/workflow/change.ts
🧬 Code graph analysis (6)
src/commands/workflow/phase.ts (3)
src/commands/workflow/types.ts (2)
PhaseId(10-10)Task(14-19)src/utils/file-system.ts (1)
fileExists(76-86)src/commands/workflow/state.ts (3)
getChangePath(30-32)loadWorkflowState(96-124)updatePhase(126-134)
src/commands/workflow/status.ts (3)
src/utils/file-system.ts (1)
fileExists(76-86)src/commands/workflow/types.ts (3)
PhaseId(10-10)Task(14-19)StatusOutput(45-58)src/commands/workflow/state.ts (2)
getChangePath(30-32)loadWorkflowState(96-124)
src/commands/workflow/task.ts (2)
src/commands/workflow/state.ts (4)
loadWorkflowState(96-124)updateTaskStatus(136-157)loadTasks(75-83)saveTasks(85-94)src/commands/workflow/types.ts (2)
Task(14-19)TaskStatus(12-12)
src/commands/workflow/change.ts (1)
src/commands/workflow/state.ts (3)
createChange(167-185)listChanges(187-194)saveActiveChangeId(52-57)
src/cli/index.ts (5)
src/commands/workflow/index.ts (9)
runStatus(7-7)runPhaseAdvance(8-8)runChangeCreate(9-9)runChangeList(9-9)runChangeSwitch(9-9)runTaskComplete(10-10)runTaskStart(10-10)runTaskNext(10-10)runTaskAdd(10-10)src/commands/workflow/status.ts (1)
runStatus(115-194)src/commands/workflow/phase.ts (1)
runPhaseAdvance(74-134)src/commands/workflow/change.ts (3)
runChangeCreate(11-31)runChangeList(33-50)runChangeSwitch(52-62)src/commands/workflow/task.ts (4)
runTaskComplete(14-42)runTaskStart(44-71)runTaskNext(73-111)runTaskAdd(113-143)
src/commands/workflow/state.ts (2)
src/utils/file-system.ts (1)
fileExists(76-86)src/commands/workflow/types.ts (6)
MetaFile(26-33)Task(14-19)TasksFile(21-24)SCHEMA_VERSION(8-8)WorkflowState(40-43)PhaseId(10-10)
🪛 markdownlint-cli2 (0.18.1)
POC_IMPLEMENTATION.md
73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
117-117: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
181-181: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
189-189: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
195-195: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
200-200: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
206-206: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
210-210: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
215-215: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
221-221: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
227-227: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
231-231: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
236-236: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
243-243: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
250-250: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
253-253: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (22)
test/core/completions/installers/zsh-installer.test.ts (1)
652-653: LGTM - Cross-platform path separator handling.The regex pattern correctly matches both forward and backslash separators, making the test compatible with Windows paths. This aligns with the PR objectives.
src/utils/file-system.ts (1)
96-114: LGTM - Cross-platform permission checking.The use of
fs.access()withW_OKflag correctly addresses Windows compatibility issues wherestats.modedoesn't reliably indicate write permissions. The parent directory check for non-existent files is also appropriate.src/commands/workflow/index.ts (1)
1-11: LGTM - Clean barrel export pattern.The re-export structure provides a single entry point for workflow commands, which is consumed appropriately by
src/cli/index.ts. The export pattern is clean and follows common TypeScript conventions.src/cli/index.ts (2)
17-27: LGTM - Clean import structure.The imports correctly match the exports from
src/commands/workflow/index.ts.
460-472: LGTM - Workflow CLI commands properly structured.The workflow command group is well-structured with:
- Consistent error handling across all commands
- Appropriate [PoC] label in descriptions
- Proper option typing and passing
- Clean delegation to workflow command functions
The
task addcommand correctly handles the optional acceptance criteria array withoptions?.ac || [].src/commands/workflow/phase.ts (2)
33-72: LGTM - Phase validation logic is sound.The transition validation correctly enforces:
draft → plan: Requires plan.md fileplan → implement: Requires tasks with acceptance criteriaimplement → done: Requires all tasks to be completeThe logic appropriately blocks advancement when preconditions aren't met.
74-134: LGTM - Phase advancement logic is well-structured.The implementation correctly:
- Validates an active change exists before advancing
- Determines target phase (explicit or next)
- Prevents backward transitions
- Validates all intermediate transitions when jumping phases
- Provides clear error messages with actionable blockers
The non-null assertion at line 113 is safe because the loop condition guarantees
getNextPhase(phase)returns a value before reachingtargetPhase.package.json (1)
63-63: js-yaml and @types/js-yaml versions are current and secure. Bothjs-yaml@^4.1.1and@types/js-yaml@^4.0.9match the latest available versions. The js-yaml version includes the patch for CVE-2025-64718 (prototype pollution vulnerability), and the type definitions have no known vulnerabilities.src/commands/workflow/status.ts (4)
56-94: LGTM!The
getNextActionfunction comprehensively handles all phase states with appropriate guidance messages and includes a sensible fallback for unknown states.
96-113: LGTM!Both helper functions are correctly implemented with clear priority logic in
getNextTaskand comprehensive status counting ingetTaskProgress.
115-157: LGTM!The
runStatusfunction correctly loads state, computes derived values, and handles both JSON and human-readable output modes. The non-null assertion on line 154 is safe given the surrounding conditional check forstate.activeChange.
196-214: LGTM!The formatting helper functions are well-structured using typed
Recordmappings for color functions.src/commands/workflow/change.ts (2)
33-50: LGTM!The
runChangeListfunction handles both output modes correctly and gracefully handles the empty list case.
52-62: LGTM!The
runChangeSwitchfunction properly validates the change exists before switching and provides helpful error output with available changes.src/commands/workflow/task.ts (4)
14-42: LGTM!The
runTaskCompletefunction properly validates state, prevents duplicate completions, and provides clear feedback with appropriate error handling.
73-111: LGTM!The
runTaskNextfunction correctly handles both output modes and appropriately usesreturninstead ofprocess.exitfor the no-active-change case, since this is a query operation.
113-143: LGTM!The
runTaskAddfunction properly validates for duplicate task IDs and correctly persists the new task. The check for onlyactiveChangeIdis sufficient here sinceloadTaskshandles missing files gracefully.
145-153: LGTM!The
formatStatushelper is correctly implemented. Note: this is similar toformatTaskStatusinstatus.ts, which could be extracted to a shared utility if the codebase grows.src/commands/workflow/state.ts (4)
20-41: LGTM!The constants clearly define the state directory structure, and the path helpers are straightforward. Consider exporting
fileExistsor consolidating withFileSystem.fileExistsfromsrc/utils/file-system.tsto reduce duplication across the codebase.
96-124: LGTM!The
loadWorkflowStatefunction correctly handles all edge cases: no active change, missing meta file, and missing tasks. The state composition is clean and well-structured.
126-134: LGTM!The
updatePhasefunction correctly validates the change exists before updating and properly updates the timestamp.
187-194: LGTM!The
listChangesfunction correctly handles the case where the changes directory doesn't exist and properly filters for directory entries.
POC_IMPLEMENTATION.md
Outdated
| - `plan` → `implement` → `done` | ||
|
|
||
| Phase transitions SHALL be explicit (represented in persisted state) and, when performed via `openspec phase advance`, SHALL be blocked when preconditions are not met. | ||
|
|
||
| #### Scenario: Create starts in `plan` | ||
|
|
||
| - **WHEN** `openspec change create <title>` is executed | ||
| - **THEN** the change SHALL be created with `currentPhaseId = "plan"` | ||
| - **AND** the created change SHALL become the active change |
There was a problem hiding this comment.
Phase list inconsistency with implementation.
The documentation states the FSM has phases plan → implement → done (line 48) and that new changes start in plan (line 56). However, the implementation in src/commands/workflow/phase.ts line 14 defines PHASE_ORDER as ['draft', 'plan', 'implement', 'done'] and includes validation for draft → plan transitions (lines 41-46).
Impact: The documentation doesn't match the implemented behavior, which could confuse developers using this PoC.
Update the documentation to include the draft phase or remove it from the implementation for consistency.
🤖 Prompt for AI Agents
In POC_IMPLEMENTATION.md around lines 48 to 56, the doc describes the FSM as
"plan → implement → done" and says new changes start in "plan", but the code
implements a 'draft' phase (PHASE_ORDER = ['draft','plan','implement','done']
and draft→plan validation). Update the documentation to reflect the actual FSM
by adding the 'draft' phase (e.g., "draft → plan → implement → done") and change
the create scenario to state that new changes start in "draft" (or alternatively
remove 'draft' from the implementation if you prefer code change); ensure the
phase list and the create-start phase are consistent across the doc.
src/commands/workflow/change.ts
Outdated
| console.log(chalk.dim('State directory: .openspec/changes/' + id)); | ||
| console.log(chalk.dim('Phase: plan')); | ||
| console.log(); | ||
| console.log(chalk.cyan('Next:'), 'Add tasks to .openspec/changes/' + id + '/tasks.yaml'); | ||
| console.log(chalk.cyan('Then:'), 'Run `openspec wf phase advance` to move to implement'); |
There was a problem hiding this comment.
Incorrect phase displayed in output message.
Line 23 shows Phase: plan but createChange initializes the phase as 'draft' (see src/commands/workflow/state.ts line 175). The guidance on line 25 also suggests adding tasks immediately, but the workflow requires creating plan.md first before advancing from draft to plan phase.
🔎 Proposed fix
console.log();
console.log(chalk.dim('State directory: .openspec/changes/' + id));
- console.log(chalk.dim('Phase: plan'));
+ console.log(chalk.dim('Phase: draft'));
console.log();
- console.log(chalk.cyan('Next:'), 'Add tasks to .openspec/changes/' + id + '/tasks.yaml');
- console.log(chalk.cyan('Then:'), 'Run `openspec wf phase advance` to move to implement');
+ console.log(chalk.cyan('Next:'), 'Create plan.md in .openspec/changes/' + id);
+ console.log(chalk.cyan('Then:'), 'Run `openspec wf phase advance` to move to plan');🤖 Prompt for AI Agents
In src/commands/workflow/change.ts around lines 22 to 26, the output incorrectly
shows "Phase: plan" and tells the user to add tasks immediately, but
createChange initializes the phase as "draft" and the workflow requires creating
plan.md before moving to plan; update the printed phase to "draft" and change
the guidance: instruct the user to create .openspec/changes/<id>/plan.md first
(or run the command that generates it), then add tasks to
.openspec/changes/<id>/tasks.yaml and finally run `openspec wf phase advance` to
move to implement/plan as appropriate.
src/commands/workflow/phase.ts
Outdated
| async function fileExists(filePath: string): Promise<boolean> { | ||
| try { | ||
| await fs.access(filePath); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid code duplication - reuse FileSystemUtils.fileExists.
This fileExists helper duplicates the logic already present in src/utils/file-system.ts (lines 76-86). Code duplication can lead to inconsistencies when bugs are fixed in one location but not the other.
🔎 Proposed fix
Add import at top of file:
import { loadWorkflowState, updatePhase, getChangePath } from './state.js';
import { PhaseId, Task } from './types.js';
+import { FileSystemUtils } from '../../utils/file-system.js';Remove local implementation and use the utility:
-async function fileExists(filePath: string): Promise<boolean> {
- try {
- await fs.access(filePath);
- return true;
- } catch {
- return false;
- }
-}
-
async function validateTransition(
from: PhaseId,
to: PhaseId,
tasks: Task[],
changeId: string
): Promise<string[]> {
const blockers: string[] = [];
if (from === 'draft' && to === 'plan') {
// Requires plan.md to exist
const planPath = path.join(getChangePath('.', changeId), 'plan.md');
- if (!(await fileExists(planPath))) {
+ if (!(await FileSystemUtils.fileExists(planPath))) {
blockers.push('No plan.md found. Create a plan before advancing.');
}
}📝 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.
| async function fileExists(filePath: string): Promise<boolean> { | |
| try { | |
| await fs.access(filePath); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| async function validateTransition( | |
| from: PhaseId, | |
| to: PhaseId, | |
| tasks: Task[], | |
| changeId: string | |
| ): Promise<string[]> { | |
| const blockers: string[] = []; | |
| if (from === 'draft' && to === 'plan') { | |
| // Requires plan.md to exist | |
| const planPath = path.join(getChangePath('.', changeId), 'plan.md'); | |
| if (!(await FileSystemUtils.fileExists(planPath))) { | |
| blockers.push('No plan.md found. Create a plan before advancing.'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/commands/workflow/phase.ts around lines 24 to 31, the local async
fileExists helper duplicates src/utils/file-system.ts; remove the local
implementation and instead import and call the existing utility. Add an import
for FileSystemUtils (or the exported fileExists) from "src/utils/file-system" at
the top of the file, delete the local fileExists function, and replace any calls
to the local helper with FileSystemUtils.fileExists(...) (or fileExists(...) if
the function is exported directly) so the module reuses the single canonical
implementation.
src/commands/workflow/state.ts
Outdated
| export function slugify(text: string): string { | ||
| return text | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, '-') | ||
| .replace(/^-+|-+$/g, '') | ||
| .substring(0, 50); | ||
| } |
There was a problem hiding this comment.
Potential edge case: slugify may return an empty string.
If the input consists entirely of non-alphanumeric characters (e.g., emojis, non-Latin scripts), the function returns an empty string, which could cause issues when used as a change ID (empty directory name, path traversal concerns with ..).
🔎 Proposed fix
export function slugify(text: string): string {
- return text
+ const slug = text
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '')
.substring(0, 50);
+ if (!slug) {
+ throw new Error('Unable to generate valid ID from title. Use alphanumeric characters.');
+ }
+ return slug;
}🤖 Prompt for AI Agents
In src/commands/workflow/state.ts around lines 159–165, slugify can return an
empty string for inputs with no ASCII alphanumerics; update it to guarantee a
non-empty, safe change ID by falling back when the result is empty (for example
append a deterministic suffix like a short timestamp or hash or static token
such as "change"), ensure the fallback also removes any "." or ".." segments to
prevent path traversal, and keep the overall length limit (e.g., 50 chars) after
applying the fallback.
src/commands/workflow/state.ts
Outdated
| export async function createChange(root: string, title: string): Promise<string> { | ||
| const id = slugify(title); | ||
| const now = new Date().toISOString(); | ||
|
|
||
| const meta: MetaFile = { | ||
| schemaVersion: SCHEMA_VERSION, | ||
| id, | ||
| title, | ||
| currentPhaseId: 'draft', | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }; | ||
|
|
||
| await saveMeta(root, id, meta); | ||
| await saveTasks(root, id, []); | ||
| await saveActiveChangeId(root, id); | ||
|
|
||
| return id; | ||
| } |
There was a problem hiding this comment.
No collision detection for duplicate change IDs.
If a user creates two changes with the same or similar titles that slugify to the same ID, the second will silently overwrite the first. Consider checking if the change already exists before creating.
🔎 Proposed fix
export async function createChange(root: string, title: string): Promise<string> {
const id = slugify(title);
+ const existingMeta = await loadMeta(root, id);
+ if (existingMeta) {
+ throw new Error(`Change "${id}" already exists. Choose a different title.`);
+ }
const now = new Date().toISOString();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/commands/workflow/state.ts around lines 167 to 185, the createChange
function currently slugifies the title to produce an id but does not guard
against collisions, allowing a new change to silently overwrite an existing one;
update the function to check whether a change with the generated id already
exists before saving (e.g., attempt to read/load the meta file or check the
expected file path), and if it exists either throw a clear error or generate a
unique id (such as appending/incrementing a suffix) and use that instead; ensure
the chosen behavior is consistent with the codebase (prefer throwing an explicit
error to avoid surprise overwrites) and only proceed to saveMeta, saveTasks, and
saveActiveChangeId after confirming the id is unique or after creating a
non-conflicting id.
src/commands/workflow/status.ts
Outdated
| async function getPhaseBlockers(phase: PhaseId, tasks: Task[], changeId: string): Promise<string[]> { | ||
| const blockers: string[] = []; | ||
|
|
||
| if (phase === 'draft') { | ||
| // draft -> plan requires plan.md | ||
| const planPath = path.join(getChangePath('.', changeId), 'plan.md'); |
There was a problem hiding this comment.
Hardcoded root path may cause issues.
The function receives changeId as a parameter but hardcodes '.' for the root path in getChangePath. If this function is ever called from a context where the root is not the current directory, the path will be incorrect.
🔎 Proposed fix
-async function getPhaseBlockers(phase: PhaseId, tasks: Task[], changeId: string): Promise<string[]> {
+async function getPhaseBlockers(phase: PhaseId, tasks: Task[], changeId: string, root: string = '.'): Promise<string[]> {
const blockers: string[] = [];
if (phase === 'draft') {
// draft -> plan requires plan.md
- const planPath = path.join(getChangePath('.', changeId), 'plan.md');
+ const planPath = path.join(getChangePath(root, changeId), 'plan.md');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/commands/workflow/status.ts around lines 22 to 27, getPhaseBlockers
currently calls getChangePath with a hardcoded '.' which can produce incorrect
paths; modify the function to avoid the hardcoded root by either (A) adding a
rootPath parameter to getPhaseBlockers and pass that into
getChangePath(rootPath, changeId), or (B) if a global/root helper exists, call
that (e.g., getRootPath() or process.cwd()) instead of '.'; update all call
sites to provide the appropriate root if you add the parameter.
…er tests - Fix canWriteFile to use fs.access with W_OK flag instead of Unix-style permission bits (stats.mode & 0o222) which don't work on Windows - Update test paths to use platform-specific invalid paths that fail on both Unix and Windows - Use regex for path separator matching in test assertions
TabishB
force-pushed
the
youthful-wescoff
branch
from
1659554 to
54031b9
Compare
Summary
canWriteFileto usefs.accesswithW_OKflag instead of Unix-style permission bits (stats.mode & 0o222) which don't work reliably on WindowsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.