refactor: consolidate duplicated code across Maestro - #1232
refactor: consolidate duplicated code across Maestro#1232jSydorowicz21 wants to merge 161 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 632 files, which is 332 over the limit of 300. To get a review, narrow the scope: Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (661)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review please |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b18223da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| BUILD_VERSION: ${{ steps.version.outputs.VERSION }} | ||
| run: | | ||
| .github/scripts/rebuild-and-verify-native.sh x64 | ||
| .github/scripts/verify-native-arch.sh --rebuild --arch x64 --platform mac |
There was a problem hiding this comment.
The release job was not migrated along with this Bun-dependent rebuild step: .github/workflows/release.yml still runs npm ci at line 166, does not run setup-bun, and this commit deletes package-lock.json. I checked the local npm ci behavior, which reports that it can only install with an existing package-lock.json or npm-shrinkwrap.json, so every release matrix leg stops before packaging; even if that install were bypassed, verify-native-arch.sh --rebuild now invokes bunx without provisioning Bun.
Useful? React with 👍 / 👎.
| @@ -1,21 +1,21 @@ | |||
| /** | |||
| * Cue Engine Core - thin façade for Maestro Cue event-driven automation. | |||
| * Cue Engine Core — thin façade for Maestro Cue event-driven automation. | |||
There was a problem hiding this comment.
Replace this U+2014 character with ASCII punctuation. The repository instructions in /workspace/Maestro/AGENTS.md explicitly prohibit U+2014 and U+2013 anywhere, including code comments, and this commit adds many more occurrences that should be swept at the same time.
Useful? React with 👍 / 👎.
| if (upload.bytes.byteLength > MAX_FEEDBACK_GITHUB_CONTENT_BYTES) { | ||
| throw new Error( | ||
| `Feedback upload exceeds GitHub's ${MAX_FEEDBACK_GITHUB_CONTENT_BYTES}-byte content limit.` | ||
| ); |
There was a problem hiding this comment.
When a user selects an image between 1 MB and 10 MB, FeedbackChatView.tsx accepts it and explicitly advertises a 10 MB per-image limit, but this new validation rejects the upload before invoking GitHub and causes feedback submission to fail. The uploader limit needs to match the renderer contract, or the renderer must reject and describe the smaller limit before submission.
Useful? React with 👍 / 👎.
| message: `Add feedback screenshot ${Date.now()}-${index}`, | ||
| content: base64, | ||
| }), | ||
| 'utf8' | ||
| ); | ||
| const uploadResult = await execFileNoThrow( | ||
| 'gh', | ||
| [ | ||
| 'api', | ||
| `repos/${owner}/${ATTACHMENTS_REPO}/contents/${repoPath}`, | ||
| '--method', | ||
| 'PUT', | ||
| '--input', | ||
| payloadPath, | ||
| ], | ||
| undefined, | ||
| getExpandedEnv() | ||
| bytes, | ||
| branch: 'main', | ||
| }, |
There was a problem hiding this comment.
For an existing maestro-feedback-attachments repository whose default branch is master or another user-configured name, this newly supplied branch: 'main' makes every Contents API upload target a branch that may not exist. The previous upload omitted the branch and therefore used the repository default; resolve that default or leave the branch unspecified instead of assuming main.
Useful? React with 👍 / 👎.
| const request = buildFeedbackGitHubContentRequest(upload); | ||
| const payloadPath = path.join(os.tmpdir(), `maestro-feedback-content-${Date.now()}.json`); | ||
| await fs.writeFile(payloadPath, JSON.stringify(request.body), 'utf8'); |
There was a problem hiding this comment.
When two feedback uploads begin in the same millisecond, such as submissions from separate Maestro windows, both calls use the same temporary JSON path. One call can overwrite or unlink the file while the other gh api --input invocation is reading it, causing the wrong content to be uploaded or the second upload to fail; use a collision-resistant temporary filename.
Useful? React with 👍 / 👎.
| @@ -559,7 +596,7 @@ export const SETTINGS_METADATA: Record<string, SettingMetadata> = { | |||
| description: | |||
| 'Default shell for terminal sessions. Auto-detected from $SHELL on Unix, PowerShell on Windows.', | |||
| type: 'string', | |||
| default: getDefaultShell(), | |||
| default: 'bash', | |||
There was a problem hiding this comment.
On Windows, and on Unix systems whose login shell is not bash, maestro-cli settings list --defaults now reports the literal metadata placeholder instead of the runtime default. settings-list.ts still reads meta.default for the displayed value, defaultValue, and isDefault, while this commit moved platform resolution into getSettingDefault(); use that resolver there as settings get and settings reset already do.
Useful? React with 👍 / 👎.
|
|
||
| const values: T[] = []; | ||
| for (const entry of candidate) { | ||
| if (!decodeElement(entry)) return null; |
There was a problem hiding this comment.
If one persisted playbook, feedback draft, or submitted issue is malformed or uses an older shape, this helper returns null for the entire array. Each caller converts that miss to [] and its next create or autosave operation writes the shortened list back, deleting every otherwise valid sibling record; reject or quarantine only the bad element, or refuse writes after a decode failure, instead of treating the whole file as empty.
Useful? React with 👍 / 👎.
| new RegExp(`^ ${escapeRegExp(key)}\\s*:`).test(line) | ||
| ); | ||
| const replacement = ` ${key}: ${values[key]}\n`; |
There was a problem hiding this comment.
When a valid cue.yaml indents children of settings: with four spaces, this patcher only recognizes exactly two-space keys and appends new keys at a different indentation level. parseAndValidate() then rejects the resulting YAML, while CueEngine.saveSettings() catches the error and still updates only its in-memory state, so the Settings UI reports no target and the apparent change disappears after restart; detect and retain the existing block indentation instead of hard-coding two spaces.
Useful? React with 👍 / 👎.
|
Too many files changed for review. ( |
|
@codex give this a greptile style review with a score out of 5 and why |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22823fe905
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Verify native module architecture (Linux x64) | ||
| if: matrix.platform == 'linux' | ||
| run: .github/scripts/verify-native-arch.sh x64 | ||
| run: .github/scripts/verify-native-arch.sh --arch x64 --platform linux |
There was a problem hiding this comment.
The release workflow still runs npm ci at .github/workflows/release.yml:165-166, but this commit deletes package-lock.json and commits only bun.lock, so every release matrix leg exits before reaching this changed verification step. Confirmed with npm 11.4.2: npm ci --dry-run reports that an existing package lock or shrinkwrap file is required. Update the release install and cache steps to use Bun, or retain a synchronized npm lockfile.
Useful? React with 👍 / 👎.
| echo "✓ $mod is correctly built for $ARCH" | ||
|
|
||
| echo "Rebuilding native modules for $PLATFORM/$ARCH..." | ||
| bunx electron-rebuild --arch="$ARCH" --force |
There was a problem hiding this comment.
When the macOS or Linux packaging sections pass --rebuild, this new path invokes bunx, but the inspected release workflow only configures Node and never runs oven-sh/setup-bun; the regular CI workflow explicitly performs that setup. On release runners without a preinstalled Bun binary, all macOS and Linux packaging jobs fail here with bunx: command not found; either set up the pinned Bun version in release.yml or keep this release helper on npx.
Useful? React with 👍 / 👎.
| @@ -1,21 +1,21 @@ | |||
| /** | |||
| * Cue Engine Core - thin façade for Maestro Cue event-driven automation. | |||
| * Cue Engine Core — thin façade for Maestro Cue event-driven automation. | |||
There was a problem hiding this comment.
The repository AGENTS.md states, "NEVER use U+2014 (em dash) or U+2013 (en dash) anywhere," including code comments, documentation, tests, and UI copy. This changed line introduces U+2014, and the commit adds the prohibited characters across many other changed source and test lines as well. Replace them with plain hyphens, commas, parentheses, or separate sentences as required by the repository guideline.
Useful? React with 👍 / 👎.
| "scripts": { | ||
| "dev": "node scripts/dev.mjs", | ||
| "dev:prod-data": "cross-env USE_PROD_DATA=1 node scripts/dev.mjs", | ||
| "dev": "bun scripts/dev.mjs", |
There was a problem hiding this comment.
The root README and CONTRIBUTING.md still instruct new contributors to run npm install followed by npm run dev, and neither lists Bun as a prerequisite. This changed script now immediately executes the external bun binary, which npm does not install, so that documented setup fails with bun: command not found on a normal Node-only environment. Either retain Node/npm for this entry point or update the root setup contract and declare a pinned Bun requirement.
Useful? React with 👍 / 👎.
| connected: (message) => | ||
| hasString(message, 'clientId') && | ||
| hasString(message, 'message') && | ||
| hasBoolean(message, 'authenticated'), |
There was a problem hiding this comment.
Every browser connection receives the connected payload constructed at src/main/web-server/routes/wsRoute.ts:112-116, which does not contain authenticated. This new validator therefore rejects the server's actual connection confirmation before useWebSocket can process it, leaving the hook in connecting, reporting isConnected as false, and never starting its ping interval. Either include the authentication state in the producer or make this field optional in the shared protocol.
Useful? React with 👍 / 👎.
|
Hi @jSydorowicz21, thanks a lot for taking on this cleanup - consolidating 134 audited priorities with a full ledger and rollback boundaries is a genuinely large, careful effort and it's much appreciated. 🙏 Before we can move to approval, there are a couple of things to sort out: 1. Merge conflictsThe branch currently has conflicts against 2. Release workflow was not migrated to BunThis one is a real blocker. The PR deletes
CI passing doesn't exercise this path, so every release matrix leg would break. Please migrate 3. Forbidden em/en dashes were introduced
4. Codex findings worth addressingThe
One meta noteThis PR touches 603 files, which pushed both CodeRabbit and Greptile past their file limits so neither could actually review it. If it's feasible to split along the playbook-stage boundaries you've already defined, smaller PRs would get real automated coverage and be far safer to land. Not a hard requirement, but worth considering. Thanks again for the work here - once the rebase is clean and the release/dash/codex items are addressed, I'm happy to take another pass. |
41fc74b to
4693184
Compare
4693184 to
2ad6941
Compare
2 participants
This PR is ready for review after repository-wide deduplication, a fresh rebase onto current
rc, six completed independent review passes, repaired review findings, focused regressions, production builds, full local pre-push validation, and green GitHub Actions.Summary
This implements the cleanup audit recorded in
dedup-report.mdandPlans/dedup-playbook/IMPLEMENTATION-LEDGER.md.build/archive/**andbuild/new-icon/**design sourcesThe ledger contains every priority's disposition, evidence, rollback boundary, verification notes, and playbook stage.
Wave 15 follow-up
Six additional saturation audits found priorities P135-P147. Twelve were implemented and one was retained:
.husky/_shims while preserving the native root hooksshared/agentIdsimport path to the canonical agent registryui:render-unsafeunchanged because removing it requires a host-major compatibility plan and would invalidate current v1-targeted pluginsAdversarial review caught the plugin capability-major incompatibility and malformed nested JSON block handling. The capability removal was fully reverted; malformed records now fall back safely.
Current rc integration
Rebased the complete branch onto current
origin/rcatfbc664ff43fd2781196f090c3dd83de77dc81ac3. Current head is2ad6941e024e4c0585ead0e3155655aceec00f4e;origin/rcis an ancestor and GitHub reports the PR merge state as clean.The rebase conflicts were resolved against current contracts. Six independent review passes inspected main/shared/parser behavior, renderer behavior, release/build/package configuration, plugin/security contracts, rebase conflict resolutions, tests, documentation claims, and the final aggregate diff.
Confirmed review findings were repaired in
4693184,a556421,f31f385, and2ad6941:package-lock.jsonremovalVerification
The scoped plugin E2E attempt timed out on unrelated existing broker/scheduler failures and is not claimed as a passing gate. Existing non-failing Vite CSS/chunk warnings and Node localstorage-file test warnings remain.
Review guidance
Review is easiest by playbook stage or focused commit rather than as one aggregate diff. Wave 15 implementation is
362f6de; ledger reconciliation is1cc52d9; validation evidence ise73bdc9; the current rc rebase spans the rewritten branch; final review repairs are4693184,a556421,f31f385, and2ad6941.