Conversation
Also fix pre-existing ESLint errors in e2e test files (unused vars, empty patterns, unused imports).
|
Too many files changed for review. ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughComprehensive formatting and documentation overhaul affecting configuration files, documentation, prompts, E2E tests, and React components. Primary changes include whitespace normalization, Markdown table reformatting, JSON indentation adjustments, documentation content expansion, and test implementation consolidation across 100+ files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
🚥 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 unit tests (beta)
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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/remote-access.md (1)
81-89:⚠️ Potential issue | 🟡 Minor
</Warning>closing tag indented inside list — potential Mintlify MDX parse issue.The closing
</Warning>is indented by two spaces (matching the last bullet item's continuation indent) rather than sitting at the column of the opening<Warning>tag. Some MDX parsers treat misaligned closing tags as plain text, which could leave the warning block unclosed or render the tag literally.📄 Suggested fix
<Warning> **Security Trade-off**: ... **Recommendations when using custom ports:** - Use Cloudflare tunnel for remote access instead of exposing ports directly - Ensure your network firewall is properly configured - Consider additional authentication at the network level - </Warning> +</Warning>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/remote-access.md` around lines 81 - 89, The closing </Warning> tag is indented and may not match the opening <Warning> tag causing MDX to treat it as text; fix by moving the closing </Warning> tag to the same column/indentation as the opening <Warning> tag (remove the two-space indent), ensure there are no extra leading spaces or characters on that line, and verify the Warning block renders correctly in the docs/remote-access.md by locating the <Warning> ... </Warning> pair and aligning them.src/prompts/group-chat-moderator-synthesis.md (1)
16-16:⚠️ Potential issue | 🟡 MinorSemantic prompt change bundled into a formatting-only PR.
Line 16 is new content instructing the synthesis prompt to always append a "Next steps" or follow-up question. Like the companion change in
group-chat-moderator-system.md, this is a behavioral change that modifies how the moderator concludes group-chat interactions and warrants its ownfeat:orfix:commit for traceability rather than being folded into astyle:Prettier pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/group-chat-moderator-synthesis.md` at line 16, This PR accidentally adds a behavioral change in group-chat-moderator-synthesis.md by instructing the synthesis prompt to always append a "Next steps" follow-up; revert that addition from this formatting-only commit and instead create a separate commit/PR (feat: or fix:) that introduces the semantic prompt change (and do the same for the companion group-chat-moderator-system.md change) so the behavior change is traceable and not bundled into a style/prettier pass.src/prompts/group-chat-moderator-system.md (1)
30-31:⚠️ Potential issue | 🟡 MinorSemantic prompt change bundled into a formatting-only PR.
Lines 30–31 are new behavioral directives, not Prettier reformatting. They establish explicit final-handoff rules for the moderator: only returning control to the user once a complete answer is ready and omitting
@mentionsin the final summary. The change looks correct and intentional, but it modifies agent behavior and belongs in a behavioral/feature commit rather than astyle:commit for traceability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/group-chat-moderator-system.md` around lines 30 - 31, The two new behavioral directives ("Only return to the user when you have a complete, actionable answer" and "When you're done and ready to hand back to the user, provide a summary WITHOUT any `@mentions`") were introduced in a formatting-only PR; revert those two lines from this commit so the PR only contains style changes, and instead add them in a separate behavioral/feature commit (or open a follow-up PR) that updates the moderator system prompt; reference the exact directive texts above when creating the behavioral commit so reviewers can locate and approve the intentional behavior change.src/prompts/maestro-system-prompt.md (1)
94-97:⚠️ Potential issue | 🟠 MajorRead-access policy is internally inconsistent and should be unified.
One section limits reads to user-explicit requests, while another requires proactive reading (e.g., planning/context tasks). This ambiguity can cause inconsistent behavior between runs. Define one clear read policy that covers both baseline context reads and user-requested reads.
Also applies to: 111-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/maestro-system-prompt.md` around lines 94 - 97, Unify the ambiguous read-access bullets by replacing the separate "Reading files" line with a single clear policy that permits proactive baseline/context reads (for planning and contextualization) within the agent workspace and autorun folder and also requires explicit user consent for reading sensitive or external paths; update the bullet that currently reads "Reading files: Allowed anywhere if explicitly requested by the user" to a unified statement that references {{AGENT_PATH}} and {{AUTORUN_FOLDER}} and explains both automatic baseline reads (what is allowed automatically) and when explicit user requests/consent are required (what requires explicit permission), and apply the same unified wording to the corresponding lines referenced (including the duplicate at lines 111–113).src/prompts/wizard-system.md (1)
13-23:⚠️ Potential issue | 🟠 MajorThis file contains functional prompt-policy expansion, not just formatting.
The new recall fields and response-behavior rules materially change agent behavior. Given the PR goal is Prettier/ESLint cleanup, this should be moved to a dedicated prompt-behavior PR for safer review and change tracking.
Also applies to: 53-60, 111-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/wizard-system.md` around lines 13 - 23, This change adds functional prompt/policy behavior (new recall fields and response rules) into wizard-system.md—specifically the AGENT_HISTORY_PATH schema and the entries array fields ('summary','timestamp','type','success','fullResponse','elapsedTimeMs') and new response-behavior guidance—which should not be included in a Prettier/ESLint cleanup PR; revert/remove the behavioral/schema additions from this PR so the file only contains formatting/lint fixes, and create a separate prompt-behavior PR that introduces the AGENT_HISTORY_PATH schema, entries array fields, and the new response-behavior rules with a clear description and tests for reviewers.src/prompts/wizard-inline-new.md (1)
38-55:⚠️ Potential issue | 🟠 MajorBehavioral prompt changes are out of scope for a “style-only” PR.
This section adds new operational instructions (confidence initialization, discovery behavior, and conversation policy), which is a functional change rather than formatting. Please split these into a separate PR so policy changes get explicit review and rollback safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/wizard-inline-new.md` around lines 38 - 55, The diff adds operational/behavioral instructions under the "Discovery Approach" section (the block beginning with "IMPORTANT: Before your first response" and the bullets about confidence initialization, discovery behavior, and conversation policy); since this is a style-only PR, remove that entire operational block from the current change (or revert it) and instead place those behavioral changes into a separate functional PR with its own description and tests/rollback notes; ensure the original commit only contains formatting/style edits for the markdown and update the commit message accordingly.
🟡 Minor comments (12)
src/prompts/autorun-default.md-97-97 (1)
97-97:⚠️ Potential issue | 🟡 MinorFix platform name capitalization ("Github" → "GitHub").
Use the official capitalization in this user-facing instruction to keep documentation consistent and professional.
✏️ Proposed fix
- For any code or documentation changes, if we're in a Github repo: + For any code or documentation changes, if we're in a GitHub repo:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/autorun-default.md` at line 97, The user-facing sentence "For any code or documentation changes, if we're in a Github repo:" in the autorun-default prompt should use the official platform capitalization; update the text in src/prompts/autorun-default.md (the line containing "For any code or documentation changes, if we're in a Github repo:") by replacing "Github" with "GitHub" so the sentence reads "...if we're in a GitHub repo:".docs/provider-notes.md-49-49 (1)
49-49:⚠️ Potential issue | 🟡 MinorBroken same-page anchor
#mid-turn-input.The fragment
#mid-turn-inputdoes not correspond to any heading in this file (headings present are#custom-configuration,#claude-code,#codex-openai,#opencode, etc.), so the link resolves to nothing. Either add a## Mid-turn inputsection to this document or update the reference to point to the correct anchor/page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/provider-notes.md` at line 49, The anchor `#mid-turn-input` referenced in the table row "| Mid-turn input | ❌ Batch mode only ([details](`#mid-turn-input`)) |" is broken; fix it by either adding a matching heading "## Mid-turn input" (to create the `#mid-turn-input` anchor) or by updating the link to point to an existing heading (e.g., replace `#mid-turn-input` with the correct fragment for the relevant section such as `#custom-configuration` or the actual section name); ensure the table entry and the target heading text match exactly so the anchor resolves.src/prompts/speckit/speckit.specify.md-178-178 (1)
178-178:⚠️ Potential issue | 🟡 MinorCapitalise "Markdown" as a proper noun.
"markdown" should be "Markdown" (it is a proper noun referring to the Markdown formatting language).
📝 Proposed fix
- 4. **CRITICAL - Table Formatting**: Ensure markdown tables are properly formatted: + 4. **CRITICAL - Table Formatting**: Ensure Markdown tables are properly formatted:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/speckit/speckit.specify.md` at line 178, In the line containing the header "CRITICAL - Table Formatting" update the phrase "markdown tables" to "Markdown tables" (capitalize Markdown as a proper noun); also search within the same document for any other occurrences of the lowercase "markdown" (e.g., the exact token "markdown") and change them to "Markdown" to keep terminology consistent across the speckit.specify.md content.src/prompts/speckit/speckit.specify.md-146-146 (1)
146-146:⚠️ Potential issue | 🟡 MinorSelf-referential "proceed to step 6" — should be step 7.
This line is already inside step 6. The intent is to proceed to step 7 (the reporting/completion step), not to loop back to the current step.
📝 Proposed fix
- - **If all items pass**: Mark checklist complete and proceed to step 6 + - **If all items pass**: Mark checklist complete and proceed to step 7🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/speckit/speckit.specify.md` at line 146, Update the self-referential checklist text in the speckit specification: locate the line under step 6 that reads "**If all items pass**: Mark checklist complete and proceed to step 6" and change the trailing reference from "proceed to step 6" to "proceed to step 7" so the flow advances to the reporting/completion step instead of looping back to the current step.src/prompts/speckit/speckit.specify.md-59-60 (1)
59-60:⚠️ Potential issue | 🟡 MinorDuplicate
--jsonflag and wrong script path in PowerShell example.Two issues on these lines:
Line 59 — duplicate
--jsonflag: The command passes--jsontwice (--json "$ARGUMENTS" --json). Remove the duplicate.Line 60 — wrong script path in PowerShell example: The "PowerShell example" invokes
.specify/scripts/bash/create-new-feature.sh(a bash script), but should reference a PowerShell script (.ps1file) instead. PowerShell cannot natively execute.shfiles.📝 Proposed fix
- - Bash example: `.specify/scripts/bash/create-new-feature.sh --json "$ARGUMENTS" --json --number 5 --short-name "user-auth" "Add user authentication"` - - PowerShell example: `.specify/scripts/bash/create-new-feature.sh --json "$ARGUMENTS" -Json -Number 5 -ShortName "user-auth" "Add user authentication"` + - Bash example: `.specify/scripts/bash/create-new-feature.sh --json "$ARGUMENTS" --number 5 --short-name "user-auth" "Add user authentication"` + - PowerShell example: `.specify/scripts/powershell/create-new-feature.ps1 -Arguments "$ARGUMENTS" -Number 5 -ShortName "user-auth" "Add user authentication"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/speckit/speckit.specify.md` around lines 59 - 60, Remove the duplicate --json in the Bash example and update the PowerShell example to call the correct PowerShell script and flags: remove the second `--json` in the `.specify/scripts/bash/create-new-feature.sh` invocation and change the PowerShell invocation to reference a `.ps1` file (e.g., `.specify/scripts/pwsh/create-new-feature.ps1`) using PowerShell-style flags (single `-Json`, `-Number`, `-ShortName`) so the examples call the proper script and do not pass `--json` twice.docs/releases.md-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorHyphenation nit: "cross context memory" → "cross-context memory".
"Cross-context" is a compound modifier and should be hyphenated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 24, Update the release-note sentence that currently reads 'Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross context memory!)' by replacing the phrase "cross context memory" with the hyphenated form "cross-context memory" so the compound modifier is grammatically correct; ensure the parentheses and emoji remain unchanged.docs/releases.md-135-135 (1)
135-135:⚠️ Potential issue | 🟡 MinorMisspelled brand name: "Github" → "GitHub".
The official capitalization of the platform is GitHub. Using "Github" is inconsistent with all other references in the file and is technically incorrect.
📝 Proposed fix
-🌳 Github Worktree support was added. +🌳 GitHub Worktree support was added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 135, The release notes contain a misspelling: change "Github Worktree support was added." to "GitHub Worktree support was added." in the shown sentence, and scan the rest of docs/releases.md for any other "Github" occurrences and replace them with the correct "GitHub" capitalization to keep references consistent.docs/installation.md-62-66 (1)
62-66:⚠️ Potential issue | 🟡 MinorAdd a language specifier to the fenced code block (MD040)
The fenced code block at Line 64 has no language tag, triggering a
markdownlintMD040 warning. Since this is a Windows filesystem path rather than a shell command,textis the appropriate specifier.📝 Proposed fix
-``` +```text \\wsl$\Ubuntu\home\<username>\maestro</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/installation.mdaround lines 62 - 66, Add the language specifier "text"
to the fenced code block that contains the Windows path
"\wsl$\Ubuntu\home<username>\maestro" so the block becomes a triple-backtick
fence with "text" after the opening ticks; update the fence surrounding that
exact path line to use ```text to satisfy markdownlint MD040.</details> </blockquote></details> <details> <summary>ARCHITECTURE.md-747-747 (1)</summary><blockquote> `747-747`: _⚠️ Potential issue_ | _🟡 Minor_ **Capitalize "Markdown" — it's a proper noun.** The table cell reads "switching between markdown documents"; it should be "Markdown documents". <details> <summary>📝 Proposed fix</summary> ```diff -| `AutoRunDocumentSelector.tsx` | Dropdown for switching between markdown documents | +| `AutoRunDocumentSelector.tsx` | Dropdown for switching between Markdown documents | ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` at line 747, Update the table entry text to capitalize "Markdown" in the ARCHITECTURE.md row for AutoRunDocumentSelector.tsx; locate the cell that currently reads "Dropdown for switching between markdown documents" and change it to "Dropdown for switching between Markdown documents" so the proper noun is capitalized. ``` </details> </blockquote></details> <details> <summary>e2e/autorun-sessions.spec.ts-579-605 (1)</summary><blockquote> `579-605`: _⚠️ Potential issue_ | _🟡 Minor_ **`expect(finalContent).toBe('Content B')` only validates Playwright's `fill()`, not app behavior.** After two consecutive `textarea.fill()` calls, asserting that `inputValue()` returns `'Content B'` simply confirms that Playwright's API works. It does not test that the app correctly handles concurrent saves or that the underlying file reflects `'Content B'`. Consider adding a file-system check or triggering an actual save+reload to make this assertion meaningful. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/autorun-sessions.spec.ts` around lines 579 - 605, The current assertion in the test "should not lose content during concurrent session operations" only checks Playwright's textarea.fill result (finalContent) and not persistence; modify the test around the autoRunTab / editButton / textarea flow to perform an actual save-and-verify sequence: after the rapid fills and window.keyboard.press('Meta+S') calls ensure the app finishes saving (wait for a save indicator or network response), then reload the page or re-open the session and re-query textarea.inputValue() to assert it equals 'Content B' (or alternatively call a backend/file-system helper to read the underlying file and assert its contents), so the test verifies persisted app behavior rather than only Playwright state. ``` </details> </blockquote></details> <details> <summary>e2e/autorun-setup.spec.ts-217-222 (1)</summary><blockquote> `217-222`: _⚠️ Potential issue_ | _🟡 Minor_ **`stepIndicator` is assigned but never awaited or asserted — test body is empty.** Line 220 assigns a locator to `stepIndicator` but the test ends immediately after with no assertion. This test always passes vacuously regardless of whether the UI contains a step indicator. <details> <summary>💡 Suggested fix</summary> ```diff test('should show step indicators', async ({ window }) => { const stepIndicator = window.locator('text=/Step|\\d.*of.*\\d/i'); - // This may or may not exist depending on UI design + // Only assert if the indicator is expected to always be present + // If optional, either skip the test or document the condition explicitly + if ((await stepIndicator.count()) > 0) { + await expect(stepIndicator.first()).toBeVisible(); + } }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/autorun-setup.spec.ts` around lines 217 - 222, The test 'should show step indicators' currently only creates a locator (stepIndicator) and never checks it; update the test to await and assert the locator so it actually verifies UI presence — e.g., await the locator's visibility or count (use stepIndicator.count() and assert count > 0, or use Playwright's expect(stepIndicator).toBeVisible()/toHaveCount(...) depending on intended behavior) so the test fails when no step indicator exists. ``` </details> </blockquote></details> <details> <summary>e2e/autorun-editing.spec.ts-196-206 (1)</summary><blockquote> `196-206`: _⚠️ Potential issue_ | _🟡 Minor_ **List continuation assertion may be fragile depending on auto-continue implementation.** Line 202 asserts `expect(value).toContain('- First item\n-')`. If the app inserts a space after the dash (standard Markdown list syntax: `- `), the assertion will fail. Consider relaxing to a regex: <details> <summary>♻️ Proposed fix</summary> ```diff - expect(value).toContain('- First item\n-'); + expect(value).toMatch(/- First item\n-\s*/); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/autorun-editing.spec.ts` around lines 196 - 206, The assertion that checks list continuation is too strict: change the expect on the captured value (variable textarea.inputValue() stored in value) to allow either a dash or a dash followed by a space after the newline (use a regex match rather than expect(value).toContain('- First item\n-')), so the test using window.keyboard.press('End') / 'Enter' accepts both "-\n" and "- \n" continuations; update the expect call that references value to use a regex like one that allows an optional space after the dash. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (5)</summary><blockquote> <details> <summary>THEMES.md (1)</summary><blockquote> `61-62`: **Add a blank line between description text and `<img>` tag in Vibes entries.** Each Vibes theme has its description text and image on adjacent lines with no blank line between them. While GitHub renders this correctly today, the lack of a blank line between a paragraph and a block-level HTML element is inconsistent with the rest of the document (and standard Markdown conventions), and some renderers may collapse them into a single paragraph flow. <details> <summary>✨ Proposed fix (Pedurple shown; apply the same pattern to the other three Vibes entries)</summary> ```diff ### Pedurple Purple is Pedram's favorite color. + <img width="3592" height="2302" alt="image" src="https://github.com/user-attachments/assets/15875d3e-37c1-4b6c-b967-551afd40b658" /> ``` </details> Also applies to: 66-67, 71-72, 76-77 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@THEMES.md` around lines 61 - 62, For each Vibes theme entry (e.g., the "Pedurple" paragraph and the three other Vibes entries referenced), insert a single blank line between the description text and the following <img> tag so the HTML block-level image sits on its own paragraph line; update the four occurrences where a paragraph and an <img> tag are adjacent (currently like "Purple is Pedram's favorite color." immediately followed by the <img> tag) to have one empty line between them to match the rest of the document and Markdown conventions. ``` </details> </blockquote></details> <details> <summary>scripts/generate-prompts.mjs (1)</summary><blockquote> `52-54`: **Redundant `existsSync` guard — `mkdirSync` with `recursive: true` is already idempotent.** `fs.mkdirSync(outputDir, { recursive: true })` does not throw when the directory already exists, so the `existsSync` pre-check is unnecessary and introduces a mild TOCTOU pattern. <details> <summary>♻️ Proposed simplification</summary> ```diff - // Ensure output directory exists - if (!fs.existsSync(outputDir)) { - fs.mkdirSync(outputDir, { recursive: true }); - } + // Ensure output directory exists + fs.mkdirSync(outputDir, { recursive: true }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/generate-prompts.mjs` around lines 52 - 54, Remove the redundant existsSync check and call fs.mkdirSync directly with recursive: true; specifically, in scripts/generate-prompts.mjs delete the if-block that checks fs.existsSync(outputDir) and replace it with a single call to fs.mkdirSync(outputDir, { recursive: true }) so the directory is created idempotently and avoids the TOCTOU pattern. ``` </details> </blockquote></details> <details> <summary>package.json (1)</summary><blockquote> `44-45`: **`format` / `format:check` scripts don't cover the file types formatted by this PR.** Both scripts target only `src/**/*.{ts,tsx}`, but this PR formats `.md`, `.json`, `.mjs`, `.mts`, and other files under `docs/`, `scripts/`, `.github/`, etc. Developers running `npm run format` locally won't reformat those files, and `npm run format:check` won't catch formatting drift in them either — creating a gap between local tooling and whatever CI check now enforces the broader scope. <details> <summary>💡 Suggested update</summary> ```diff - "format": "prettier --write \"src/**/*.{ts,tsx}\"", - "format:check": "prettier --check \"src/**/*.{ts,tsx}\"", + "format": "prettier --write .", + "format:check": "prettier --check .", ``` Or, if `.prettierignore` is used to exclude `node_modules`, `dist`, etc., the `.` form is sufficient since Prettier already respects it. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 44 - 45, The package.json "format" and "format:check" npm scripts currently only target "src/**/*.{ts,tsx}" and therefore miss files touched by this PR (e.g., .md, .json, .mjs, .mts and files under docs/, scripts/, .github/); update the "format" and "format:check" entries so they run Prettier over the project (either by changing the glob to a broader set like "\"**/*.{js,mjs,cjs,ts,tsx,mts,cts,jsx,json,md,mdx,yml,yaml,html,css,scss,md}\"" or simply use "." to let .prettierignore exclude node_modules/dist), and ensure both scripts use the same scope so local formatting and CI checks match (refer to the "format" and "format:check" script names when making the change). ``` </details> </blockquote></details> <details> <summary>e2e/autorun-sessions.spec.ts (1)</summary><blockquote> `507-524`: **Magic `1100` ms timeout is a hardcoded assumption about internal undo debounce timing.** The two `waitForTimeout(1100)` calls are tightly coupled to an assumed undo-snapshot debounce interval in the app. If the debounce changes, the test will silently become ineffective without any failure. Extract this as a named constant so the coupling is explicit. <details> <summary>♻️ Proposed refactor</summary> ```diff + const UNDO_SNAPSHOT_DEBOUNCE_MS = 1100; // Must match app's undo debounce interval await textarea.fill('First change'); - await window.waitForTimeout(1100); // Wait for undo snapshot + await window.waitForTimeout(UNDO_SNAPSHOT_DEBOUNCE_MS); await textarea.fill('Second change'); - await window.waitForTimeout(1100); + await window.waitForTimeout(UNDO_SNAPSHOT_DEBOUNCE_MS); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/autorun-sessions.spec.ts` around lines 507 - 524, Replace the two magic 1100ms waits with a named constant (e.g., const UNDO_SNAPSHOT_DEBOUNCE_MS = 1100) declared at the top of the test scope, then use await window.waitForTimeout(UNDO_SNAPSHOT_DEBOUNCE_MS) in place of the literal values where the test creates undo history (around textarea.fill calls) so the coupling to the undo-snapshot debounce is explicit and easy to update; add a short comment by UNDO_SNAPSHOT_DEBOUNCE_MS explaining it mirrors the app's undo debounce timing. ``` </details> </blockquote></details> <details> <summary>e2e/autorun-setup.spec.ts (1)</summary><blockquote> `12-12`: **Blanket ESLint disable hides that `helpers` is imported but never used.** The `/* eslint-disable `@typescript-eslint/no-unused-vars` */` at the top of the file exists solely to suppress the unused `helpers` import. The same pattern appears in all four spec files. Either remove the `helpers` import or replace the blanket disable with a targeted `// eslint-disable-next-line` on the import line. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/autorun-setup.spec.ts` at line 12, The file currently disables the no-unused-vars rule globally to hide the unused import "helpers"; instead remove the unused import "helpers" from e2e/autorun-setup.spec.ts (and the other three spec files) or replace the top-file blanket directive with a targeted inline suppression directly above the import (use // eslint-disable-next-line `@typescript-eslint/no-unused-vars`) so only that import line is suppressed; update the import or the inline comment around the symbol "helpers" and run ESLint to confirm the warning is resolved. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@ARCHITECTURE.md:
- Around line 1582-1613: The documentation block was reformatted by Prettier
because the code fence is marked as "typescript"; change the fence language to
"text" and restore the original template syntax so the variables read as
"{{date}}", "{{time}}", "{{datetime}}", "{{cwd}}", "{{session}}", and
"{{agent}}" (one per line with their comments) instead of the mangled "{ { date;
} }" form; update the opening fence (replace "typescript" with "text") and
ensure the inner lines use the exact double-brace tokens shown above.In
@e2e/autorun-batch.spec.ts:
- Around line 253-262: The test currently swallows assertion failures by
chaining .catch() onto the Playwright assertions (e.g., the
expect(modal.first()).not.toBeVisible({ timeout: 5000 }).catch(...)) so the test
can never fail; remove the .catch() on those assertions (both the instance
around lines with expect(modal.first()).not.toBeVisible(...) and the similar
block at 281-291) or, if you need to handle a specific non-fatal condition,
replace the .catch() with an explicit try/catch that rethrows any unexpected
errors (or asserts the expected alternate state) so real assertion failures are
not silently ignored.- Around line 643-675: The tests currently read attributes into local variables
(title) but never assert anything; update the "should have accessible Run button
with proper title" and "should have accessible Stop button with proper title"
tests to assert that the found element (runButton/stopButton) has either a
non-empty title or a non-empty aria-label (use expect(...).toBeTruthy() /
toHaveAttribute checks), and fail if both are missing or empty; for the batch
state test ("should announce batch state changes to screen readers") assert that
the relevant container locator with selector '[aria-busy]' exists and, when
simulating a running batch state, has aria-busy="true" (or that an aria-live
region contains the expected text) using Playwright's expect API so the test
actually verifies accessibility behavior.In
@e2e/autorun-editing.spec.ts:
- Around line 782-795: The computed boolean hasAccessibleName (derived from
title, ariaLabel, text) is never asserted so the accessibility check is vacuous;
inside the loop that iterates over buttons (buttons.nth(i)) add an assertion
using the test framework's expect on hasAccessibleName (e.g.,
expect(hasAccessibleName).toBeTruthy() or similar) and include a helpful message
identifying the button index or text to aid debugging; ensure you await any
async calls as needed and place the expect immediately after computing
hasAccessibleName so the test actually fails when a button lacks an accessible
name.In
@e2e/autorun-setup.spec.ts:
- Around line 33-59: The temporary project directory created as testProjectDir
in the beforeEach is never made visible to the running Electron app because the
app is launched by the electronApp fixture with MAESTRO_DATA_DIR set to
fixture's testDataDir; fix by creating the test project inside the fixture's
testDataDir or by wiring testProjectDir into the app before launch (e.g., set
MAESTRO_DATA_DIR or send an IPC/register call to the electronApp fixture to
point to testProjectDir). Concretely, in autorun-setup.spec.ts (and similarly in
autorun-batch.spec.ts, autorun-sessions.spec.ts, autorun-editing.spec.ts)
replace the path creation of testProjectDir to use testDataDir from the
electron-app fixture or call the electronApp fixture to register the folder so
the app can see files; update beforeEach to derive testProjectDir =
path.join(testDataDir, 'maestro-test-project-...') or send env/config via the
electronApp fixture before the app starts.In
@e2e/fixtures/electron-app.ts:
- Around line 59-69: The fixture testDataDir uses an empty destructuring pattern
"({}, use)" and currently only has an ESLint suppression; add a Biome-specific
ignore to suppress lint/correctness/noEmptyPattern as well (e.g., add a line "//
biome-ignore lint/correctness/noEmptyPattern" alongside the existing "//
eslint-disable-next-line no-empty-pattern") immediately above the testDataDir
declaration; do the same for the other fixtures mentioned (the other
empty-pattern fixtures around the same file) so Biome won't flag them.In
@scripts/refresh-speckit.mjs:
- Around line 104-109: The current use of exec/promisify (execAsync) to run
unzip with interpolated paths is unsafe; replace the execAsync calls that run
unzip(the listing call that assigns listOutput and the extraction call around
lines 135-136) with a safe execFile/spawn approach that passes the unzip binary
and arguments as an array (e.g., ['-l', zipPath] or ['-o', zipPath, filePath,
'-d', dest]) to avoid shell interpolation; update the imports to use
child_process.execFile (or child_process.spawn) and either promisify(execFile)
or use the Promise wrapper around spawn so you keep async/await, and change
references to execAsync to the new safe helper (preserve the variable name where
convenient and update calls in functions that list and extract ZIP entries).In
@scripts/sync-release-notes.mjs:
- Around line 103-107: The function extractTitle should guard against
null/undefined/non-string release names (release.name) before calling .match;
update extractTitle to first check that releaseName is a non-empty string,
return '' if not, then attempt to extract text after the pipe (or if no pipe
present return releaseName.trim()), keeping the function name extractTitle and
callers that pass release.name unchanged.In
@src/prompts/maestro-system-prompt.md:
- Around line 105-109: Update the system prompt text that currently allows users
to "explicitly confirm they want to override this safety measure" so that
directory write boundaries are absolute; remove the fallback option (the
numbered item "2. Explicitly confirm they want to override this safety measure")
and change the instruction for handling requests outside the "assigned
directory" (except the "Auto Run" folder exception) to always refuse and explain
the restriction and require switching sessions/agents to perform the write. Also
tighten wording to be simple and unambiguous (prefer short, explicit sentences)
so the rule cannot be interpreted as granting an override.In
@src/prompts/speckit/speckit.clarify.md:
- Around line 89-97: The clarify prompt contains conflicting limits ("Maximum of
10 total questions" vs enforced stop at 5 for /speckit.clarify); update the
prompt and runtime logic to use a single authoritative limit (e.g.,
MAX_CLARIFY_QUESTIONS) and ensure both the human-readable bullet list and the
route behavior (/speckit.clarify) reference that same symbol, or add an explicit
conflict-check: if the prompt text and runtime limit differ, the handler should
STOP, emit a clear confusion message naming the two sources ("Maximum of 10
total questions" vs "stop at 5 asked questions") and request resolution before
proceeding; adjust any wording at the lines mentioning "Maximum of 10" and the
"stop at 5" rule so they match the chosen constant.
Outside diff comments:
In@docs/remote-access.md:
- Around line 81-89: The closing tag is indented and may not match
the opening tag causing MDX to treat it as text; fix by moving the
closing tag to the same column/indentation as the opening
tag (remove the two-space indent), ensure there are no extra leading spaces or
characters on that line, and verify the Warning block renders correctly in the
docs/remote-access.md by locating the ... pair and aligning
them.In
@src/prompts/group-chat-moderator-synthesis.md:
- Line 16: This PR accidentally adds a behavioral change in
group-chat-moderator-synthesis.md by instructing the synthesis prompt to always
append a "Next steps" follow-up; revert that addition from this formatting-only
commit and instead create a separate commit/PR (feat: or fix:) that introduces
the semantic prompt change (and do the same for the companion
group-chat-moderator-system.md change) so the behavior change is traceable and
not bundled into a style/prettier pass.In
@src/prompts/group-chat-moderator-system.md:
- Around line 30-31: The two new behavioral directives ("Only return to the user
when you have a complete, actionable answer" and "When you're done and ready to
hand back to the user, provide a summary WITHOUT any@mentions") were introduced
in a formatting-only PR; revert those two lines from this commit so the PR only
contains style changes, and instead add them in a separate behavioral/feature
commit (or open a follow-up PR) that updates the moderator system prompt;
reference the exact directive texts above when creating the behavioral commit so
reviewers can locate and approve the intentional behavior change.In
@src/prompts/maestro-system-prompt.md:
- Around line 94-97: Unify the ambiguous read-access bullets by replacing the
separate "Reading files" line with a single clear policy that permits proactive
baseline/context reads (for planning and contextualization) within the agent
workspace and autorun folder and also requires explicit user consent for reading
sensitive or external paths; update the bullet that currently reads "Reading
files: Allowed anywhere if explicitly requested by the user" to a unified
statement that references {{AGENT_PATH}} and {{AUTORUN_FOLDER}} and explains
both automatic baseline reads (what is allowed automatically) and when explicit
user requests/consent are required (what requires explicit permission), and
apply the same unified wording to the corresponding lines referenced (including
the duplicate at lines 111–113).In
@src/prompts/wizard-inline-new.md:
- Around line 38-55: The diff adds operational/behavioral instructions under the
"Discovery Approach" section (the block beginning with "IMPORTANT: Before your
first response" and the bullets about confidence initialization, discovery
behavior, and conversation policy); since this is a style-only PR, remove that
entire operational block from the current change (or revert it) and instead
place those behavioral changes into a separate functional PR with its own
description and tests/rollback notes; ensure the original commit only contains
formatting/style edits for the markdown and update the commit message
accordingly.In
@src/prompts/wizard-system.md:
- Around line 13-23: This change adds functional prompt/policy behavior (new
recall fields and response rules) into wizard-system.md—specifically the
AGENT_HISTORY_PATH schema and the entries array fields
('summary','timestamp','type','success','fullResponse','elapsedTimeMs') and new
response-behavior guidance—which should not be included in a Prettier/ESLint
cleanup PR; revert/remove the behavioral/schema additions from this PR so the
file only contains formatting/lint fixes, and create a separate prompt-behavior
PR that introduces the AGENT_HISTORY_PATH schema, entries array fields, and the
new response-behavior rules with a clear description and tests for reviewers.
Minor comments:
In@ARCHITECTURE.md:
- Line 747: Update the table entry text to capitalize "Markdown" in the
ARCHITECTURE.md row for AutoRunDocumentSelector.tsx; locate the cell that
currently reads "Dropdown for switching between markdown documents" and change
it to "Dropdown for switching between Markdown documents" so the proper noun is
capitalized.In
@docs/installation.md:
- Around line 62-66: Add the language specifier "text" to the fenced code block
that contains the Windows path "\wsl$\Ubuntu\home<username>\maestro" so the
block becomes a triple-backtick fence with "text" after the opening ticks;
update the fence surrounding that exact path line to use ```text to satisfy
markdownlint MD040.In
@docs/provider-notes.md:
- Line 49: The anchor
#mid-turn-inputreferenced in the table row "| Mid-turn
input | ❌ Batch mode only (details) |" is broken; fix it by
either adding a matching heading "## Mid-turn input" (to create the
#mid-turn-inputanchor) or by updating the link to point to an existing heading
(e.g., replace#mid-turn-inputwith the correct fragment for the relevant
section such as#custom-configurationor the actual section name); ensure the
table entry and the target heading text match exactly so the anchor resolves.In
@docs/releases.md:
- Line 24: Update the release-note sentence that currently reads 'Agents are now
inherently aware of your activity history as seen in the history panel 📜 (this
is built-in cross context memory!)' by replacing the phrase "cross context
memory" with the hyphenated form "cross-context memory" so the compound modifier
is grammatically correct; ensure the parentheses and emoji remain unchanged.- Line 135: The release notes contain a misspelling: change "Github Worktree
support was added." to "GitHub Worktree support was added." in the shown
sentence, and scan the rest of docs/releases.md for any other "Github"
occurrences and replace them with the correct "GitHub" capitalization to keep
references consistent.In
@e2e/autorun-editing.spec.ts:
- Around line 196-206: The assertion that checks list continuation is too
strict: change the expect on the captured value (variable textarea.inputValue()
stored in value) to allow either a dash or a dash followed by a space after the
newline (use a regex match rather than expect(value).toContain('- First
item\n-')), so the test using window.keyboard.press('End') / 'Enter' accepts
both "-\n" and "- \n" continuations; update the expect call that references
value to use a regex like one that allows an optional space after the dash.In
@e2e/autorun-sessions.spec.ts:
- Around line 579-605: The current assertion in the test "should not lose
content during concurrent session operations" only checks Playwright's
textarea.fill result (finalContent) and not persistence; modify the test around
the autoRunTab / editButton / textarea flow to perform an actual save-and-verify
sequence: after the rapid fills and window.keyboard.press('Meta+S') calls ensure
the app finishes saving (wait for a save indicator or network response), then
reload the page or re-open the session and re-query textarea.inputValue() to
assert it equals 'Content B' (or alternatively call a backend/file-system helper
to read the underlying file and assert its contents), so the test verifies
persisted app behavior rather than only Playwright state.In
@e2e/autorun-setup.spec.ts:
- Around line 217-222: The test 'should show step indicators' currently only
creates a locator (stepIndicator) and never checks it; update the test to await
and assert the locator so it actually verifies UI presence — e.g., await the
locator's visibility or count (use stepIndicator.count() and assert count > 0,
or use Playwright's expect(stepIndicator).toBeVisible()/toHaveCount(...)
depending on intended behavior) so the test fails when no step indicator exists.In
@src/prompts/autorun-default.md:
- Line 97: The user-facing sentence "For any code or documentation changes, if
we're in a Github repo:" in the autorun-default prompt should use the official
platform capitalization; update the text in src/prompts/autorun-default.md (the
line containing "For any code or documentation changes, if we're in a Github
repo:") by replacing "Github" with "GitHub" so the sentence reads "...if we're
in a GitHub repo:".In
@src/prompts/speckit/speckit.specify.md:
- Line 178: In the line containing the header "CRITICAL - Table Formatting"
update the phrase "markdown tables" to "Markdown tables" (capitalize Markdown as
a proper noun); also search within the same document for any other occurrences
of the lowercase "markdown" (e.g., the exact token "markdown") and change them
to "Markdown" to keep terminology consistent across the speckit.specify.md
content.- Line 146: Update the self-referential checklist text in the speckit
specification: locate the line under step 6 that reads "If all items pass:
Mark checklist complete and proceed to step 6" and change the trailing reference
from "proceed to step 6" to "proceed to step 7" so the flow advances to the
reporting/completion step instead of looping back to the current step.- Around line 59-60: Remove the duplicate --json in the Bash example and update
the PowerShell example to call the correct PowerShell script and flags: remove
the second--jsonin the.specify/scripts/bash/create-new-feature.sh
invocation and change the PowerShell invocation to reference a.ps1file
(e.g.,.specify/scripts/pwsh/create-new-feature.ps1) using PowerShell-style
flags (single-Json,-Number,-ShortName) so the examples call the proper
script and do not pass--jsontwice.
Nitpick comments:
In@e2e/autorun-sessions.spec.ts:
- Around line 507-524: Replace the two magic 1100ms waits with a named constant
(e.g., const UNDO_SNAPSHOT_DEBOUNCE_MS = 1100) declared at the top of the test
scope, then use await window.waitForTimeout(UNDO_SNAPSHOT_DEBOUNCE_MS) in place
of the literal values where the test creates undo history (around textarea.fill
calls) so the coupling to the undo-snapshot debounce is explicit and easy to
update; add a short comment by UNDO_SNAPSHOT_DEBOUNCE_MS explaining it mirrors
the app's undo debounce timing.In
@e2e/autorun-setup.spec.ts:
- Line 12: The file currently disables the no-unused-vars rule globally to hide
the unused import "helpers"; instead remove the unused import "helpers" from
e2e/autorun-setup.spec.ts (and the other three spec files) or replace the
top-file blanket directive with a targeted inline suppression directly above the
import (use // eslint-disable-next-line@typescript-eslint/no-unused-vars) so
only that import line is suppressed; update the import or the inline comment
around the symbol "helpers" and run ESLint to confirm the warning is resolved.In
@package.json:
- Around line 44-45: The package.json "format" and "format:check" npm scripts
currently only target "src//*.{ts,tsx}" and therefore miss files touched by
this PR (e.g., .md, .json, .mjs, .mts and files under docs/, scripts/,
.github/); update the "format" and "format:check" entries so they run Prettier
over the project (either by changing the glob to a broader set like
""/*.{js,mjs,cjs,ts,tsx,mts,cts,jsx,json,md,mdx,yml,yaml,html,css,scss,md}""
or simply use "." to let .prettierignore exclude node_modules/dist), and ensure
both scripts use the same scope so local formatting and CI checks match (refer
to the "format" and "format:check" script names when making the change).In
@scripts/generate-prompts.mjs:
- Around line 52-54: Remove the redundant existsSync check and call fs.mkdirSync
directly with recursive: true; specifically, in scripts/generate-prompts.mjs
delete the if-block that checks fs.existsSync(outputDir) and replace it with a
single call to fs.mkdirSync(outputDir, { recursive: true }) so the directory is
created idempotently and avoids the TOCTOU pattern.In
@THEMES.md:
- Around line 61-62: For each Vibes theme entry (e.g., the "Pedurple" paragraph
and the three other Vibes entries referenced), insert a single blank line
between the description text and the followingtag so the HTML block-level
image sits on its own paragraph line; update the four occurrences where a
paragraph and antag are adjacent (currently like "Purple is Pedram's
favorite color." immediately followed by thetag) to have one empty line
between them to match the rest of the document and Markdown conventions.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2e48a8d9336ea5872385a93c8e5c11f968cffeb2 and 4f26177f3267d261fc6efefb48ad2fd77b205609. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (135)</summary> * `.github/instructions/memory.instruction.md` * `.github/workflows/ci.yml` * `.github/workflows/release.yml` * `.prettierrc` * `AGENT_SUPPORT.md` * `ARCHITECTURE.md` * `BUILDING_WINDOWS.md` * `CLAUDE-AGENTS.md` * `CLAUDE-FEATURES.md` * `CLAUDE-PATTERNS.md` * `CLAUDE-PERFORMANCE.md` * `CLAUDE-PLATFORM.md` * `CLAUDE-SESSION.md` * `CLAUDE-WIZARD.md` * `CLAUDE.md` * `CONSTITUTION.md` * `CONTRIBUTING.md` * `README.md` * `SECURITY.md` * `SYMPHONY_ISSUES.md` * `SYMPHONY_REGISTRY.md` * `THEMES.md` * `build/README.md` * `build/archive/large-figure-gradient-2.icon/icon.json` * `build/archive/large-figure-gradient-bckgrnd.icon/icon.json` * `build/archive/maestro-circle-bckgrnd.icon/icon.json` * `build/archive/maestro-purple-figure.icon/icon.json` * `docs/about/overview.md` * `docs/achievements.md` * `docs/autorun-playbooks.md` * `docs/cli.md` * `docs/context-management.md` * `docs/director-notes.md` * `docs/docs.json` * `docs/document-graph.md` * `docs/encore-features.md` * `docs/examples/local-manifest.json` * `docs/features.md` * `docs/general-usage.md` * `docs/getting-started.md` * `docs/git-worktrees.md` * `docs/group-chat.md` * `docs/history.md` * `docs/installation.md` * `docs/keyboard-shortcuts.md` * `docs/local-manifest.md` * `docs/mcp-server.md` * `docs/multi-claude.md` * `docs/openspec-commands.md` * `docs/performance-profiling.md` * `docs/playbook-exchange.md` * `docs/provider-notes.md` * `docs/releases.md` * `docs/remote-access.md` * `docs/screenshots.md` * `docs/slash-commands.md` * `docs/speckit-commands.md` * `docs/ssh-remote-execution.md` * `docs/symphony.md` * `docs/troubleshooting.md` * `docs/usage-dashboard.md` * `e2e/autorun-batch.spec.ts` * `e2e/autorun-editing.spec.ts` * `e2e/autorun-sessions.spec.ts` * `e2e/autorun-setup.spec.ts` * `e2e/fixtures/electron-app.ts` * `eslint.config.mjs` * `package.json` * `playwright.config.ts` * `postcss.config.mjs` * `scripts/build-cli.mjs` * `scripts/build-preload.mjs` * `scripts/generate-prompts.mjs` * `scripts/notarize.js` * `scripts/refresh-openspec.mjs` * `scripts/refresh-speckit.mjs` * `scripts/set-version.mjs` * `scripts/sync-release-notes.mjs` * `src/prompts/autorun-default.md` * `src/prompts/context-grooming.md` * `src/prompts/context-summarize.md` * `src/prompts/context-transfer.md` * `src/prompts/director-notes.md` * `src/prompts/group-chat-moderator-synthesis.md` * `src/prompts/group-chat-moderator-system.md` * `src/prompts/group-chat-participant-request.md` * `src/prompts/group-chat-participant.md` * `src/prompts/maestro-system-prompt.md` * `src/prompts/openspec/metadata.json` * `src/prompts/openspec/openspec.apply.md` * `src/prompts/openspec/openspec.archive.md` * `src/prompts/openspec/openspec.help.md` * `src/prompts/openspec/openspec.implement.md` * `src/prompts/openspec/openspec.proposal.md` * `src/prompts/speckit/metadata.json` * `src/prompts/speckit/speckit.analyze.md` * `src/prompts/speckit/speckit.clarify.md` * `src/prompts/speckit/speckit.constitution.md` * `src/prompts/speckit/speckit.help.md` * `src/prompts/speckit/speckit.implement.md` * `src/prompts/speckit/speckit.plan.md` * `src/prompts/speckit/speckit.specify.md` * `src/prompts/speckit/speckit.tasks.md` * `src/prompts/tab-naming.md` * `src/prompts/wizard-document-generation.md` * `src/prompts/wizard-inline-iterate-generation.md` * `src/prompts/wizard-inline-iterate.md` * `src/prompts/wizard-inline-new.md` * `src/prompts/wizard-inline-system.md` * `src/prompts/wizard-system-continuation.md` * `src/prompts/wizard-system.md` * `src/renderer/components/SettingsModal.tsx` * `src/renderer/components/SymphonyModal.tsx` * `src/renderer/components/UsageDashboard/AutoRunStats.tsx` * `src/renderer/components/UsageDashboard/SessionStats.tsx` * `src/renderer/components/UsageDashboard/TasksByHourChart.tsx` * `src/renderer/docs/app-tsx-inventory.md` * `src/renderer/index.css` * `src/renderer/index.html` * `src/renderer/public/splash.js` * `src/web/index.css` * `src/web/index.html` * `src/web/public/manifest.json` * `src/web/public/sw.js` * `symphony-registry.json` * `tailwind.config.mjs` * `tsconfig.cli.json` * `tsconfig.json` * `tsconfig.lint.json` * `tsconfig.main.json` * `vite.config.web.mts` * `vitest.config.mts` * `vitest.e2e.config.ts` * `vitest.integration.config.ts` * `vitest.performance.config.mts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
1582
to
1613
| // Available variables | ||
| {{date}} // YYYY-MM-DD | ||
| {{time}} // HH:MM:SS | ||
| {{datetime}} // Combined | ||
| {{cwd}} // Working directory | ||
| {{session}} // Session name | ||
| {{agent}} // Agent type | ||
| { | ||
| { | ||
| date; | ||
| } | ||
| } // YYYY-MM-DD | ||
| { | ||
| { | ||
| time; | ||
| } | ||
| } // HH:MM:SS | ||
| { | ||
| { | ||
| datetime; | ||
| } | ||
| } // Combined | ||
| { | ||
| { | ||
| cwd; | ||
| } | ||
| } // Working directory | ||
| { | ||
| { | ||
| session; | ||
| } | ||
| } // Session name | ||
| { | ||
| { | ||
| agent; | ||
| } | ||
| } // Agent type | ||
| ``` |
There was a problem hiding this comment.
Prettier mangled the {{variable}} template syntax — documentation is now incorrect.
The typescript code fence caused Prettier to parse {{date}}, {{time}}, etc. as nested block statements and reformat them into the multi-line { { variableName; } } form visible at lines 1583–1613. Readers following this documentation will be given incorrect template variable syntax.
Fix: change the fence language to text so Prettier leaves the content untouched.
📝 Proposed fix
-```typescript
+```text
// Available variables
{{date}} // YYYY-MM-DD
{{time}} // HH:MM:SS
{{datetime}} // Combined
{{cwd}} // Working directory
{{session}} // Session name
{{agent}} // Agent type</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ARCHITECTURE.md` around lines 1582 - 1613, The documentation block was
reformatted by Prettier because the code fence is marked as "typescript"; change
the fence language to "text" and restore the original template syntax so the
variables read as "{{date}}", "{{time}}", "{{datetime}}", "{{cwd}}",
"{{session}}", and "{{agent}}" (one per line with their comments) instead of the
mangled "{ { date; } }" form; update the opening fence (replace "```typescript"
with "```text") and ensure the inner lines use the exact double-brace tokens
shown above.
Comment on lines
+253
to
+262
|
|
||
| // Modal should close | ||
| await expect(modal.first()) | ||
| .not.toBeVisible({ timeout: 5000 }) | ||
| .catch(() => { | ||
| // Modal may still be visible if escape was handled differently | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
.catch() silently swallows assertion failures — these tests can never fail.
Both expect(...).not.toBeVisible(...) calls have a .catch() that discards any AssertionError thrown by Playwright. The test always passes regardless of whether the modal actually closes.
🐛 Proposed fix
- await expect(modal.first())
- .not.toBeVisible({ timeout: 5000 })
- .catch(() => {
- // Modal may still be visible if escape was handled differently
- });
+ // If escape behaviour is genuinely uncertain, keep it as a conditional check:
+ const isStillVisible = await modal.first().isVisible();
+ // No hard assertion - only document the observation if the behaviour is intentionally optionalOr, if the assertion is required:
- await expect(modal.first())
- .not.toBeVisible({ timeout: 5000 })
- .catch(() => {});
+ await expect(modal.first()).not.toBeVisible({ timeout: 5000 });Also applies to: 281-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/autorun-batch.spec.ts` around lines 253 - 262, The test currently
swallows assertion failures by chaining .catch() onto the Playwright assertions
(e.g., the expect(modal.first()).not.toBeVisible({ timeout: 5000 }).catch(...))
so the test can never fail; remove the .catch() on those assertions (both the
instance around lines with expect(modal.first()).not.toBeVisible(...) and the
similar block at 281-291) or, if you need to handle a specific non-fatal
condition, replace the .catch() with an explicit try/catch that rethrows any
unexpected errors (or asserts the expected alternate state) so real assertion
failures are not silently ignored.
Comment on lines
+643
to
+675
| test('should have accessible Run button with proper title', async ({ window }) => { | ||
| // Navigate to Auto Run tab | ||
| const autoRunTab = window.locator('text=Auto Run'); | ||
| if ((await autoRunTab.count()) > 0) { | ||
| await autoRunTab.first().click(); | ||
|
|
||
| // Run button should have accessible title | ||
| const runButton = window.locator('button').filter({ hasText: /^run$/i }); | ||
| if ((await runButton.count()) > 0) { | ||
| const title = await runButton.first().getAttribute('title'); | ||
| // Button should have a descriptive title or aria-label | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| test('should have accessible Stop button with proper title', async ({ window }) => { | ||
| // Stop button (when visible) should have accessible title | ||
| // This test structure verifies accessibility attributes exist | ||
| const stopButton = window.locator('button').filter({ hasText: /stop/i }); | ||
| if ((await stopButton.count()) > 0) { | ||
| const title = await stopButton.first().getAttribute('title'); | ||
| // Button should have a descriptive title | ||
| } | ||
| }); | ||
|
|
||
| test('should announce batch state changes to screen readers', async ({ window }) => { | ||
| // This test verifies ARIA live regions or state announcements | ||
| // The implementation should use aria-live or aria-busy attributes | ||
|
|
||
| // Look for aria-busy on relevant containers | ||
| const container = window.locator('[aria-busy]'); | ||
| // When batch is running, container should indicate busy state | ||
| }); |
There was a problem hiding this comment.
Accessibility tests fetch title attribute but never assert on it — tests are vacuous.
Lines 652 and 663 read the title attribute into a local variable that is immediately discarded. The test body contains no expect(...) call, so both tests pass unconditionally and verify nothing about accessibility.
🐛 Proposed fix
const title = await runButton.first().getAttribute('title');
- // Button should have a descriptive title or aria-label
+ expect(title).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/autorun-batch.spec.ts` around lines 643 - 675, The tests currently read
attributes into local variables (title) but never assert anything; update the
"should have accessible Run button with proper title" and "should have
accessible Stop button with proper title" tests to assert that the found element
(runButton/stopButton) has either a non-empty title or a non-empty aria-label
(use expect(...).toBeTruthy() / toHaveAttribute checks), and fail if both are
missing or empty; for the batch state test ("should announce batch state changes
to screen readers") assert that the relevant container locator with selector
'[aria-busy]' exists and, when simulating a running batch state, has
aria-busy="true" (or that an aria-live region contains the expected text) using
Playwright's expect API so the test actually verifies accessibility behavior.
Comment on lines
+782
to
+795
| // All buttons should have title or aria-label | ||
| const buttons = window.locator('button'); | ||
| const count = await buttons.count(); | ||
|
|
||
| for (let i = 0; i < Math.min(count, 10); i++) { | ||
| const button = buttons.nth(i); | ||
| const title = await button.getAttribute('title'); | ||
| const ariaLabel = await button.getAttribute('aria-label'); | ||
| const text = await button.textContent(); | ||
|
|
||
| // Button should have at least one form of accessible name | ||
| const hasAccessibleName = title || ariaLabel || (text && text.trim()); | ||
| // Most buttons should have accessible names | ||
| } |
There was a problem hiding this comment.
hasAccessibleName is computed but never asserted — accessibility test always passes vacuously.
Line 793 computes hasAccessibleName from title, ariaLabel, and text but the value is never passed to expect(). The comment says "Most buttons should have accessible names" but nothing is actually verified.
🐛 Proposed fix
const hasAccessibleName = title || ariaLabel || (text && text.trim());
- // Most buttons should have accessible names
+ expect(hasAccessibleName).toBeTruthy();📝 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
| // All buttons should have title or aria-label | |
| const buttons = window.locator('button'); | |
| const count = await buttons.count(); | |
| for (let i = 0; i < Math.min(count, 10); i++) { | |
| const button = buttons.nth(i); | |
| const title = await button.getAttribute('title'); | |
| const ariaLabel = await button.getAttribute('aria-label'); | |
| const text = await button.textContent(); | |
| // Button should have at least one form of accessible name | |
| const hasAccessibleName = title || ariaLabel || (text && text.trim()); | |
| // Most buttons should have accessible names | |
| } | |
| // All buttons should have title or aria-label | |
| const buttons = window.locator('button'); | |
| const count = await buttons.count(); | |
| for (let i = 0; i < Math.min(count, 10); i++) { | |
| const button = buttons.nth(i); | |
| const title = await button.getAttribute('title'); | |
| const ariaLabel = await button.getAttribute('aria-label'); | |
| const text = await button.textContent(); | |
| // Button should have at least one form of accessible name | |
| const hasAccessibleName = title || ariaLabel || (text && text.trim()); | |
| expect(hasAccessibleName).toBeTruthy(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/autorun-editing.spec.ts` around lines 782 - 795, The computed boolean
hasAccessibleName (derived from title, ariaLabel, text) is never asserted so the
accessibility check is vacuous; inside the loop that iterates over buttons
(buttons.nth(i)) add an assertion using the test framework's expect on
hasAccessibleName (e.g., expect(hasAccessibleName).toBeTruthy() or similar) and
include a helpful message identifying the button index or text to aid debugging;
ensure you await any async calls as needed and place the expect immediately
after computing hasAccessibleName so the test actually fails when a button lacks
an accessible name.
Comment on lines
+33
to
+59
| // Create a temporary project directory for tests | ||
| let testProjectDir: string; | ||
|
|
||
| test.beforeEach(async () => { | ||
| // Create a temporary directory to use as the project folder | ||
| testProjectDir = path.join(os.tmpdir(), `maestro-test-project-${Date.now()}`); | ||
| fs.mkdirSync(testProjectDir, { recursive: true }); | ||
|
|
||
| // Initialize a basic project structure | ||
| fs.writeFileSync( | ||
| path.join(testProjectDir, 'package.json'), | ||
| JSON.stringify({ name: 'test-project', version: '1.0.0' }, null, 2) | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(testProjectDir, 'README.md'), | ||
| '# Test Project\n\nA test project for E2E testing.' | ||
| ); | ||
| }); | ||
|
|
||
| test.afterEach(async () => { | ||
| // Clean up the temporary project directory | ||
| try { | ||
| fs.rmSync(testProjectDir, { recursive: true, force: true }); | ||
| } catch { | ||
| // Ignore cleanup errors | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test setup directories are never wired to the running Electron app.
The beforeEach creates testProjectDir and writes files into it, but the Electron app is launched by the electronApp fixture with MAESTRO_DATA_DIR set to the testDataDir from electron-app.ts — a completely separate temporary directory. Files written to testProjectDir are never visible to the app under test. The same disconnect exists in autorun-batch.spec.ts, autorun-sessions.spec.ts, and autorun-editing.spec.ts.
If these directories are meant to simulate a project with Auto Run content, they must be created inside (or registered with) the fixture's testDataDir, or the app must be pointed to them via IPC/env configuration before each test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/autorun-setup.spec.ts` around lines 33 - 59, The temporary project
directory created as testProjectDir in the beforeEach is never made visible to
the running Electron app because the app is launched by the electronApp fixture
with MAESTRO_DATA_DIR set to fixture's testDataDir; fix by creating the test
project inside the fixture's testDataDir or by wiring testProjectDir into the
app before launch (e.g., set MAESTRO_DATA_DIR or send an IPC/register call to
the electronApp fixture to point to testProjectDir). Concretely, in
autorun-setup.spec.ts (and similarly in autorun-batch.spec.ts,
autorun-sessions.spec.ts, autorun-editing.spec.ts) replace the path creation of
testProjectDir to use testDataDir from the electron-app fixture or call the
electronApp fixture to register the folder so the app can see files; update
beforeEach to derive testProjectDir = path.join(testDataDir,
'maestro-test-project-...') or send env/config via the electronApp fixture
before the app starts.
Comment on lines
+59
to
+69
| // eslint-disable-next-line no-empty-pattern | ||
| testDataDir: async ({}, use) => { | ||
| const dir = createTestDataDir(); | ||
| await use(dir); | ||
| // Cleanup after test | ||
| try { | ||
| fs.rmSync(dir, { recursive: true, force: true }); | ||
| } catch { | ||
| // Ignore cleanup errors | ||
| } | ||
| }, |
There was a problem hiding this comment.
Biome noEmptyPattern errors will block CI if Biome is enforced.
Playwright's fixture system parses argument names via regex and requires the first parameter to be an object destructuring pattern — {} is the only valid syntax when no fixtures are consumed. The // eslint-disable-next-line no-empty-pattern comments suppress ESLint but not Biome, which flags both lines as lint/correctness/noEmptyPattern errors.
The fix is to add Biome-specific ignore comments alongside the existing ESLint ones:
🛠️ Proposed fix
// Test data directory for isolation
// eslint-disable-next-line no-empty-pattern
+ // biome-ignore lint/correctness/noEmptyPattern: Playwright fixture requires {} destructuring
testDataDir: async ({}, use) => {
// Path to the main entry point
// eslint-disable-next-line no-empty-pattern
+ // biome-ignore lint/correctness/noEmptyPattern: Playwright fixture requires {} destructuring
appPath: async ({}, use) => {Also applies to: 72-85
🧰 Tools
🪛 Biome (2.4.4)
[error] 60-60: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/fixtures/electron-app.ts` around lines 59 - 69, The fixture testDataDir
uses an empty destructuring pattern "({}, use)" and currently only has an ESLint
suppression; add a Biome-specific ignore to suppress
lint/correctness/noEmptyPattern as well (e.g., add a line "// biome-ignore
lint/correctness/noEmptyPattern" alongside the existing "//
eslint-disable-next-line no-empty-pattern") immediately above the testDataDir
declaration; do the same for the other fixtures mentioned (the other
empty-pattern fixtures around the same file) so Biome won't flag them.
Comment on lines
+104
to
+109
| const { exec } = await import('child_process'); | ||
| const { promisify } = await import('util'); | ||
| const execAsync = promisify(exec); | ||
|
|
||
| // List files in the ZIP | ||
| const { stdout: listOutput } = await execAsync(`unzip -l "${zipPath}"`); |
There was a problem hiding this comment.
Avoid shell interpolation of ZIP entry names during extraction.
execAsync(\unzip ... "${filePath}" ...`)builds shell commands from archive paths. A crafted entry name can break quoting and execute unintended shell input. UseexecFile` with argument arrays instead.
🔒 Safer extraction approach (no shell interpolation)
- const { exec } = await import('child_process');
+ const { execFile } = await import('child_process');
const { promisify } = await import('util');
- const execAsync = promisify(exec);
+ const execFileAsync = promisify(execFile);
@@
- const { stdout: listOutput } = await execAsync(`unzip -l "${zipPath}"`);
+ const { stdout: listOutput } = await execFileAsync('unzip', ['-l', zipPath]);
@@
- await execAsync(`unzip -o -j "${zipPath}" "${filePath}" -d "${tempDir}"`);
+ await execFileAsync('unzip', ['-o', '-j', zipPath, filePath, '-d', tempDir]);Also applies to: 135-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/refresh-speckit.mjs` around lines 104 - 109, The current use of
exec/promisify (execAsync) to run unzip with interpolated paths is unsafe;
replace the execAsync calls that run `unzip` (the listing call that assigns
listOutput and the extraction call around lines 135-136) with a safe
execFile/spawn approach that passes the unzip binary and arguments as an array
(e.g., ['-l', zipPath] or ['-o', zipPath, filePath, '-d', dest]) to avoid shell
interpolation; update the imports to use child_process.execFile (or
child_process.spawn) and either promisify(execFile) or use the Promise wrapper
around spawn so you keep async/await, and change references to execAsync to the
new safe helper (preserve the variable name where convenient and update calls in
functions that list and extract ZIP entries).
Comment on lines
103
to
107
| function extractTitle(releaseName) { | ||
| // "v0.12.3 | Thinking, Spec-Kits, Context Management" -> "Thinking, Spec-Kits, Context Management" | ||
| const match = releaseName.match(/\|\s*(.+)$/); | ||
| return match ? match[1].trim() : ''; | ||
| // "v0.12.3 | Thinking, Spec-Kits, Context Management" -> "Thinking, Spec-Kits, Context Management" | ||
| const match = releaseName.match(/\|\s*(.+)$/); | ||
| return match ? match[1].trim() : ''; | ||
| } |
There was a problem hiding this comment.
Guard extractTitle against null/empty release names.
release.name can be absent, so calling .match() unconditionally can throw and break release-note generation.
💡 Proposed fix
function extractTitle(releaseName) {
// "v0.12.3 | Thinking, Spec-Kits, Context Management" -> "Thinking, Spec-Kits, Context Management"
+ if (typeof releaseName !== 'string' || releaseName.length === 0) {
+ return '';
+ }
const match = releaseName.match(/\|\s*(.+)$/);
return match ? match[1].trim() : '';
}📝 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
| function extractTitle(releaseName) { | |
| // "v0.12.3 | Thinking, Spec-Kits, Context Management" -> "Thinking, Spec-Kits, Context Management" | |
| const match = releaseName.match(/\|\s*(.+)$/); | |
| return match ? match[1].trim() : ''; | |
| // "v0.12.3 | Thinking, Spec-Kits, Context Management" -> "Thinking, Spec-Kits, Context Management" | |
| const match = releaseName.match(/\|\s*(.+)$/); | |
| return match ? match[1].trim() : ''; | |
| } | |
| function extractTitle(releaseName) { | |
| // "v0.12.3 | Thinking, Spec-Kits, Context Management" -> "Thinking, Spec-Kits, Context Management" | |
| if (typeof releaseName !== 'string' || releaseName.length === 0) { | |
| return ''; | |
| } | |
| const match = releaseName.match(/\|\s*(.+)$/); | |
| return match ? match[1].trim() : ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-release-notes.mjs` around lines 103 - 107, The function
extractTitle should guard against null/undefined/non-string release names
(release.name) before calling .match; update extractTitle to first check that
releaseName is a non-empty string, return '' if not, then attempt to extract
text after the pipe (or if no pipe present return releaseName.trim()), keeping
the function name extractTitle and callers that pass release.name unchanged.
Comment on lines
105
to
109
| If a user requests an operation that would write outside your assigned directory (and it's not the Auto Run folder), explain the restriction and ask them to either: | ||
|
|
||
| 1. Change to the appropriate session/agent for that directory | ||
| 2. Explicitly confirm they want to override this safety measure | ||
|
|
There was a problem hiding this comment.
Do not allow user-confirmed override of directory write boundaries.
Letting the agent bypass path restrictions after user confirmation undermines the core safety guarantee and creates a prompt-injection escalation path. Keep this boundary absolute (except the explicit Auto Run exception already defined).
Suggested hardening
-If a user requests an operation that would write outside your assigned directory (and it's not the Auto Run folder), explain the restriction and ask them to either:
-
-1. Change to the appropriate session/agent for that directory
-2. Explicitly confirm they want to override this safety measure
+If a user requests an operation that would write outside your assigned directory (and it's not the Auto Run folder), explain the restriction and instruct them to switch to the appropriate session/agent for that directory.
+Do not override this boundary based on user confirmation.📝 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 a user requests an operation that would write outside your assigned directory (and it's not the Auto Run folder), explain the restriction and ask them to either: | |
| 1. Change to the appropriate session/agent for that directory | |
| 2. Explicitly confirm they want to override this safety measure | |
| If a user requests an operation that would write outside your assigned directory (and it's not the Auto Run folder), explain the restriction and instruct them to switch to the appropriate session/agent for that directory. | |
| Do not override this boundary based on user confirmation. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/prompts/maestro-system-prompt.md` around lines 105 - 109, Update the
system prompt text that currently allows users to "explicitly confirm they want
to override this safety measure" so that directory write boundaries are
absolute; remove the fallback option (the numbered item "2. Explicitly confirm
they want to override this safety measure") and change the instruction for
handling requests outside the "assigned directory" (except the "Auto Run" folder
exception) to always refuse and explain the restriction and require switching
sessions/agents to perform the write. Also tighten wording to be simple and
unambiguous (prefer short, explicit sentences) so the rule cannot be interpreted
as granting an override.
Comment on lines
+89
to
+97
| - Maximum of 10 total questions across the whole session. | ||
| - Each question must be answerable with EITHER: | ||
| - A short multiple‑choice selection (2–5 distinct, mutually exclusive options), OR | ||
| - A one-word / short‑phrase answer (explicitly constrain: "Answer in <=5 words"). | ||
| - Only include questions whose answers materially impact architecture, data modeling, task decomposition, test design, UX behavior, operational readiness, or compliance validation. | ||
| - Ensure category coverage balance: attempt to cover the highest impact unresolved categories first; avoid asking two low-impact questions when a single high-impact area (e.g., security posture) is unresolved. | ||
| - Exclude questions already answered, trivial stylistic preferences, or plan-level execution details (unless blocking correctness). | ||
| - Favor clarifications that reduce downstream rework risk or prevent misaligned acceptance tests. | ||
| - If more than 5 categories remain unresolved, select the top 5 by (Impact \* Uncertainty) heuristic. |
There was a problem hiding this comment.
Resolve conflicting question limits (10 vs 5).
This section now allows up to 10 total questions, but the same prompt still enforces stopping at 5 asked questions. That conflict can cause inconsistent runtime behavior for /speckit.clarify.
💡 Proposed fix
- - Maximum of 10 total questions across the whole session.
+ - Maximum of 5 total questions across the whole session.Based on learnings: "When encountering inconsistencies, conflicting requirements, or unclear specs, STOP and explicitly name the confusion, present the tradeoff, and wait for resolution."
Also applies to: 129-131
🧰 Tools
🪛 LanguageTool
[grammar] ~94-~94: Use a hyphen to join words.
Context: ...ge balance: attempt to cover the highest impact unresolved categories first; avoi...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/prompts/speckit/speckit.clarify.md` around lines 89 - 97, The clarify
prompt contains conflicting limits ("Maximum of 10 total questions" vs enforced
stop at 5 for /speckit.clarify); update the prompt and runtime logic to use a
single authoritative limit (e.g., MAX_CLARIFY_QUESTIONS) and ensure both the
human-readable bullet list and the route behavior (/speckit.clarify) reference
that same symbol, or add an explicit conflict-check: if the prompt text and
runtime limit differ, the handler should STOP, emit a clear confusion message
naming the two sources ("Maximum of 10 total questions" vs "stop at 5 asked
questions") and request resolution before proceeding; adjust any wording at the
lines mentioning "Maximum of 10" and the "stop at 5" rule so they match the
chosen constant.
Adds project-wide Prettier scripts to complement the existing src/-scoped format and format:check commands.
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.
Also fix pre-existing ESLint errors in e2e test files (unused vars, empty patterns, unused imports).
Summary by CodeRabbit
Documentation
Features & Content
Configuration
Tests