feat: Commit, Push and Create PR actions - #13
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Git subsystem: process runner with buffer/timeouts, Git core service, GitManager orchestration, Codex-based commit/PR generation, WS API + server wiring, client UI integration, contracts/schemas for stacked git actions, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (ChatView)
participant WS as WebSocket Server
participant GM as GitManager
participant GC as GitCoreService
participant Proc as ProcessRunner
participant Codex as Codex Service
participant GH as GitHub CLI
Client->>WS: git.runStackedAction(action, cwd)
WS->>GM: runStackedAction()
rect rgba(100,150,200,0.5)
Note over GM: Commit Step
GM->>GC: prepareCommitContext(cwd)
GC-->>GM: stagedSummary, stagedPatch
GM->>Codex: generateCommitMessage(diff)
Codex-->>GM: subject, body
GM->>Proc: git commit -m "..."
Proc-->>GM: commit result
end
rect rgba(100,200,150,0.5)
Note over GM: Push Step (if requested)
GM->>GC: pushCurrentBranch(cwd, upstream?)
GC-->>GM: push result
end
rect rgba(200,150,100,0.5)
Note over GM: PR Step (if requested)
GM->>GC: readRangeContext(base, head)
GC-->>GM: commitSummary, diffSummary, diffPatch
GM->>Codex: generatePrContent(rangeContext)
Codex-->>GM: title, body
GM->>GH: gh pr list --head branch
GH-->>GM: existing PRs
alt PR exists
GM->>GH: gh pr view PR_NUMBER
GH-->>GM: PR details
else
GM->>GH: gh pr create --title "..." --body file://tmp
GH-->>GM: new PR info
end
end
GM-->>WS: GitRunStackedActionResult
WS-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
Greptile OverviewGreptile SummaryThis PR implements comprehensive Git stacked actions (commit, push, create PR) with AI-powered text generation for commit messages and PR content. The implementation spans server-side orchestration, robust process execution, WebSocket/IPC integration, and a polished React UI with real-time progress tracking. Key changes:
Architecture aligns with project priorities:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant GitActionsControl
participant NativeApi
participant GitManager
participant GitCore
participant CodexTextGenerator
participant GitCLI
participant GitHubCLI
User->>GitActionsControl: Click "Commit and create PR"
GitActionsControl->>GitActionsControl: Open modal, set action
User->>GitActionsControl: Confirm action
GitActionsControl->>NativeApi: git.runStackedAction(commit)
NativeApi->>GitManager: runStackedAction(commit)
GitManager->>GitCore: statusDetails(cwd)
GitCore->>GitCLI: git status --porcelain=2
GitCLI-->>GitCore: status output
GitCore-->>GitManager: branch, upstream info
GitManager->>GitCore: prepareCommitContext(cwd)
GitCore->>GitCLI: git add -A
GitCore->>GitCLI: git diff --cached
GitCLI-->>GitCore: staged changes
GitCore-->>GitManager: stagedSummary, stagedPatch
GitManager->>CodexTextGenerator: generateCommitMessage()
CodexTextGenerator->>CodexTextGenerator: Write temp schema file
CodexTextGenerator->>GitCLI: codex exec --output-schema
GitCLI-->>CodexTextGenerator: JSON response
CodexTextGenerator->>CodexTextGenerator: Parse, sanitize, cleanup
CodexTextGenerator-->>GitManager: {subject, body}
GitManager->>GitCore: commit(cwd, subject, body)
GitCore->>GitCLI: git commit -m subject -m body
GitCLI-->>GitCore: success
GitCore-->>GitManager: {commitSha}
GitManager-->>NativeApi: commit result
NativeApi-->>GitActionsControl: Update progress: commit completed
GitActionsControl->>NativeApi: git.runStackedAction(commit_push)
NativeApi->>GitManager: runStackedAction(commit_push)
GitManager->>GitCore: pushCurrentBranch(cwd)
GitCore->>GitCLI: git push -u origin branch
GitCLI-->>GitCore: success
GitCore-->>GitManager: {status: pushed, branch}
GitManager-->>NativeApi: push result
NativeApi-->>GitActionsControl: Update progress: push completed
GitActionsControl->>NativeApi: git.runStackedAction(commit_push_pr)
NativeApi->>GitManager: runStackedAction(commit_push_pr)
GitManager->>GitHubCLI: gh pr list --head branch
GitHubCLI-->>GitManager: [] (no existing PR)
GitManager->>GitCore: readRangeContext(cwd, baseBranch)
GitCore->>GitCLI: git log, git diff
GitCLI-->>GitCore: commit history, diff
GitCore-->>GitManager: rangeContext
GitManager->>CodexTextGenerator: generatePrContent()
CodexTextGenerator->>GitCLI: codex exec --output-schema
GitCLI-->>CodexTextGenerator: JSON response
CodexTextGenerator-->>GitManager: {title, body}
GitManager->>GitHubCLI: gh pr create --title --body-file
GitHubCLI-->>GitManager: PR URL
GitManager->>GitHubCLI: gh pr view --web
GitHubCLI-->>GitManager: success
GitManager-->>NativeApi: pr result
NativeApi-->>GitActionsControl: Update progress: PR created
GitActionsControl->>User: Show completion with PR link
Last reviewed commit: dd92084 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/server/src/processRunner.ts`:
- Around line 128-131: Attach an 'error' listener to the child's stdin and use
the write callback to handle possible write errors before calling end: in
processRunner.ts, before calling child.stdin.write(...) register
child.stdin.once("error", err => {/* reject/forward error or cleanup and ensure
promise rejects */}), call child.stdin.write(options.stdin, (err) => { if (err)
{ /* handle/reject/cleanup */ } child.stdin.end(); }); and remove the
unconditional child.stdin.end() so we only end after the write completes; ensure
any error handling forwards the error to the same rejection/cleanup path used by
child.once("error") for the spawned process.
🧹 Nitpick comments (2)
packages/contracts/src/git.test.ts (1)
11-71: LGTM!The tests provide good coverage for schema validation, including whitespace trimming and nested field parsing.
Consider adding negative test cases to verify that invalid inputs are rejected (e.g., invalid action strings, missing required fields). This would strengthen the contract validation.
,
apps/server/src/codexTextGenerator.ts (1)
84-103: Consider consolidating duplicate sanitization logic.
sanitizeCommitSubjecthere (lines 84-95) andsanitizeCommitMessageingitManager.ts(lines 99-110) perform nearly identical operations: extracting the first line, removing trailing periods, and truncating to 72 characters. This duplication could lead to divergent behavior over time.Consider extracting a shared utility or having
gitManager.tsrely on the already-sanitized output fromCodexTextGeneratorwithout additional sanitization.
Add Commit, Push, and Create PR actions by introducing
|
Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ChatView.tsx`:
- Around line 395-424: When a git status fetch succeeds we need to clear any
previous error so the error banner doesn't persist; inside the load async
function (the one that calls api.git.status with gitCwd) after successfully
calling setGitStatus(nextStatus) also call setGitActionError(null) (guarded by
the same !cancelled check) so successful refreshes remove stale errors; update
the useEffect's load success branch in ChatView.tsx (the load function /
useEffect that references api, gitCwd, setGitStatus, setGitActionError)
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/server/src/git.test.ts`:
- Around line 460-466: The test invokes git with a single-quoted remote path
which fails on Windows; update the call that runs git(tmp.path, `remote add
origin '${remote.path}'`) to wrap the path using JSON.stringify(remote.path)
(i.e., produce a double-quoted, escaped string) so Windows cmd handles spaces
correctly; modify the test where makeTmpDir()/remote and the git(...) invocation
are used (see git(tmp.path, `remote add origin ...`), initRepoWithCommit,
createGitBranch) to pass the JSON.stringify-wrapped path instead of single
quotes.
In `@apps/server/src/git.ts`:
- Around line 245-251: The current early-return treats a branch as
"skipped_up_to_date" when details.hasUpstream && details.aheadCount === 0 even
if it is behind; update the condition to also require details.behindCount === 0
so we only mark truly up-to-date branches. Change the if that checks
details.hasUpstream and details.aheadCount to: details.hasUpstream &&
details.aheadCount === 0 && details.behindCount === 0 (leaving the returned
object with branch and optional upstreamBranch unchanged).
- Around line 129-143: The timeout check must be unconditional: in
runGitOrThrow, always throw when result.timedOut by calling
normalizeGitExecutionError(args, result) (or similar) before considering
options.allowNonZeroExit; then keep the existing non-zero exit handling for
result.code when options.allowNonZeroExit is false. Update the logic in
runGitOrThrow (referencing runGitOrThrow, RunGitOptions,
options.allowNonZeroExit, result.timedOut, result.code, and
normalizeGitExecutionError) so timeouts are detected and thrown unconditionally
while allowing suppressed non-zero exit codes only when appropriate.
In `@apps/server/src/gitManager.ts`:
- Around line 402-423: Both runGh and runGhStdout currently call this.run("gh",
args, { cwd }) and can hang; add an explicit timeoutMs option to those calls.
Define a clear constant (e.g. GH_CLI_TIMEOUT_MS = 30_000) near the top of the
module and pass it into this.run as { cwd, timeoutMs: GH_CLI_TIMEOUT_MS } in
both runGh and runGhStdout so gh CLI invocations time out predictably. Ensure
the constant is used in both functions and adjust any types if needed to match
ProcessRunOptions.
🧹 Nitpick comments (1)
apps/server/src/gitManager.ts (1)
175-183: If no commit is created, skip push/PR to avoid empty actions.When
runCommitStepreturnsskipped_no_changes, the current flow still pushes and can attempt PR creation. That can create confusing “no‑op” PRs or unnecessary network calls.
Co-authored-by: codex <codex@users.noreply.github.com>
| const fail = (error: Error): void => { | ||
| child.kill("SIGTERM"); | ||
| finalize(() => { | ||
| reject(error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🟡 Medium
src/processRunner.ts:123 The fail function clears the timeout that would escalate to SIGKILL, so if the child ignores SIGTERM, the parent hangs forever. Consider adding a force-kill timer in fail similar to the timeout logic (lines 108-110).
- const fail = (error: Error): void => {
- child.kill("SIGTERM");
- finalize(() => {
- reject(error);
- });
+ const fail = (error: Error): void => {
+ child.kill("SIGTERM");
+ const killTimer = setTimeout(() => {
+ child.kill("SIGKILL");
+ }, 1_000);
+ finalize(() => {
+ clearTimeout(killTimer);
+ reject(error);
+ });🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/processRunner.ts around lines 123-128:
The `fail` function clears the timeout that would escalate to `SIGKILL`, so if the child ignores `SIGTERM`, the parent hangs forever. Consider adding a force-kill timer in `fail` similar to the timeout logic (lines 108-110).
| stream: "stdout" | "stderr", | ||
| chunk: Buffer | string, | ||
| ): Error | null => { | ||
| const text = chunk.toString(); |
There was a problem hiding this comment.
🟢 Low
src/processRunner.ts:134 Using chunk.toString() per Buffer can split multi‑byte UTF‑8 and corrupt text; consider StringDecoder to assemble stdout/stderr safely.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/processRunner.ts around line 134:
Using `chunk.toString()` per `Buffer` can split multi‑byte UTF‑8 and corrupt text; consider `StringDecoder` to assemble `stdout`/`stderr` safely.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/git.ts (1)
50-86:⚠️ Potential issue | 🟠 MajorAdd output caps to avoid unbounded git command buffering.
runGitaccumulates stdout/stderr without limits; large diffs can spike memory and make behavior unpredictable under load. Consider enforcing a max buffer (similar toprocessRunner) or streaming to disk with a safe cap.🔧 Suggested fix (bounded output)
+const DEFAULT_GIT_MAX_BUFFER_BYTES = 8 * 1024 * 1024; + -function runGit(args: readonly string[], cwd: string, timeoutMs = 30_000): Promise<TerminalCommandResult> { +function runGit( + args: readonly string[], + cwd: string, + timeoutMs = 30_000, + maxBufferBytes = DEFAULT_GIT_MAX_BUFFER_BYTES, +): Promise<TerminalCommandResult> { return new Promise((resolve, reject) => { const child = spawn("git", args, { cwd, env: process.env, stdio: ["ignore", "pipe", "pipe"], }); let stdout = ""; let stderr = ""; let timedOut = false; + let stdoutBytes = 0; + let stderrBytes = 0; + let settled = false; const timeout = setTimeout(() => { timedOut = true; child.kill("SIGTERM"); setTimeout(() => { if (!child.killed) child.kill("SIGKILL"); }, 1_000).unref(); }, timeoutMs); + const fail = (error: Error) => { + if (settled) return; + settled = true; + clearTimeout(timeout); + child.kill("SIGTERM"); + reject(error); + }; + child.stdout?.on("data", (chunk: Buffer) => { - stdout += chunk.toString(); + const text = chunk.toString(); + stdout += text; + stdoutBytes += Buffer.byteLength(text); + if (stdoutBytes > maxBufferBytes) { + fail( + new Error( + `${commandLabel(args)} exceeded stdout buffer limit (${maxBufferBytes} bytes).`, + ), + ); + } }); child.stderr?.on("data", (chunk: Buffer) => { - stderr += chunk.toString(); + const text = chunk.toString(); + stderr += text; + stderrBytes += Buffer.byteLength(text); + if (stderrBytes > maxBufferBytes) { + fail( + new Error( + `${commandLabel(args)} exceeded stderr buffer limit (${maxBufferBytes} bytes).`, + ), + ); + } }); child.on("error", (error) => { clearTimeout(timeout); reject(error); }); child.on("close", (code, signal) => { + if (settled) return; + settled = true; clearTimeout(timeout); resolve({ stdout, stderr, code: code ?? null, signal: signal ?? null, timedOut }); }); }); }As per coding guidelines: Maintain predictable behavior under load and during failures (session restarts, reconnects, partial streams).
Co-authored-by: codex <codex@users.noreply.github.com>
| await runGitOrThrow(cwd, args, { allowNonZeroExit }); | ||
| } | ||
|
|
||
| async gitStdout(cwd: string, args: readonly string[], allowNonZeroExit = false): Promise<string> { |
There was a problem hiding this comment.
🟡 Medium
src/git.ts:359 gitStdout discards stderr even on success, so the truncation warning from runGit is lost. Consider checking if output was truncated and either throwing an error or returning a flag so callers know the data is incomplete.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 359:
`gitStdout` discards `stderr` even on success, so the truncation warning from `runGit` is lost. Consider checking if output was truncated and either throwing an error or returning a flag so callers know the data is incomplete.
| }, | ||
| ); | ||
|
|
||
| const rawOutput = (await fs.readFile(outputPath, "utf8")).trim(); |
There was a problem hiding this comment.
🟠 High
src/codexTextGenerator.ts:145 fs.readFile at line 145 has no size limit, unlike the maxBufferBytes guard on stdout/stderr in runProcess. Consider adding a file size check (via fs.stat) before reading to prevent OOM if codex produces unexpectedly large output.
- const rawOutput = (await fs.readFile(outputPath, "utf8")).trim();
+ const MAX_OUTPUT_BYTES = 8 * 1024 * 1024;
+ const stat = await fs.stat(outputPath);
+ if (stat.size > MAX_OUTPUT_BYTES) {
+ throw new Error(`Codex output exceeded size limit (${MAX_OUTPUT_BYTES} bytes).`);
+ }
+ const rawOutput = (await fs.readFile(outputPath, "utf8")).trim();🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/codexTextGenerator.ts around line 145:
`fs.readFile` at line 145 has no size limit, unlike the `maxBufferBytes` guard on stdout/stderr in `runProcess`. Consider adding a file size check (via `fs.stat`) before reading to prevent OOM if `codex` produces unexpectedly large output.
- include open PR metadata in git status with graceful `gh` failure handling - centralize git command execution in `GitCoreService` via `runProcess` with truncation support - add PTY spawn-helper permission fixes, shell fallback retries, and tests for new behavior
| } | ||
|
|
||
| async createWorktree(input: GitCreateWorktreeInput): Promise<GitCreateWorktreeResult> { | ||
| const sanitizedBranch = input.newBranch.replace(/\//g, "-"); |
There was a problem hiding this comment.
🟡 Medium
src/git.ts:382 Default worktreePath can collide across repos and branches (sanitized names and shared basenames). Suggest deriving a unique, deterministic path (e.g., hash of full repo path + branch) or document if intentional.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 382:
Default `worktreePath` can collide across repos and branches (sanitized names and shared basenames). Suggest deriving a unique, deterministic path (e.g., hash of full repo path + branch) or document if intentional.
| args.push("-m", trimmedBody); | ||
| } | ||
| await this.git(cwd, args); | ||
| const commitSha = trimStdout(await this.gitStdout(cwd, ["rev-parse", "HEAD"])); |
There was a problem hiding this comment.
🟢 Low
src/git.ts:256 Race condition: another commit between git commit and git rev-parse HEAD could return the wrong SHA. Consider using git rev-parse HEAD output from the commit command itself, or use git commit --porcelain to get the SHA atomically.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 256:
Race condition: another commit between `git commit` and `git rev-parse HEAD` could return the wrong SHA. Consider using `git rev-parse HEAD` output from the commit command itself, or use `git commit --porcelain` to get the SHA atomically.
| const worktreeMap = new Map<string, string>(); | ||
| if (worktreeList.code === 0) { | ||
| let currentPath: string | null = null; | ||
| for (const line of worktreeList.stdout.split("\n")) { |
There was a problem hiding this comment.
🟡 Medium
src/git.ts:347 On Windows, split("\n") leaves trailing \r in paths, causing fs.existsSync to fail. Consider using split(/\r?\n/) instead.
| for (const line of worktreeList.stdout.split("\n")) { | |
| for (const line of worktreeList.stdout.split(/\r?\n/)) { |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 347:
On Windows, `split("\n")` leaves trailing `\r` in paths, causing `fs.existsSync` to fail. Consider using `split(/\r?\n/)` instead.
| } | ||
|
|
||
| async removeWorktree(input: GitRemoveWorktreeInput): Promise<void> { | ||
| await executeGit(input.cwd, ["worktree", "remove", input.path], { |
There was a problem hiding this comment.
🟢 Low
src/git.ts:402 Consider adding -- before input.path to prevent paths starting with - from being interpreted as git options.
| await executeGit(input.cwd, ["worktree", "remove", input.path], { | |
| await executeGit(input.cwd, ["worktree", "remove", "--", input.path], { |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 402:
Consider adding `--` before `input.path` to prevent paths starting with `-` from being interpreted as git options.
- Replace custom inline SVGs with Lucide commit, push, and GitHub icons - Keep git action behavior unchanged while applying minor style cleanup
- Add a Git action confirmation modal with live commit/push/PR progress states - Accept optional `commitMessage` input and skip AI message generation when provided - Expand server and contracts tests for custom commit message handling Co-authored-by: codex <codex@users.noreply.github.com>
- Move git menu, modal, and stacked action logic out of `ChatView` - Render a new `GitActionsControl` wired with `api` and `gitCwd`
- Split Git actions into context-aware Commit, Push, and PR menu items - Add modal action selection with clearer availability and disabled-state guidance - Migrate git status and immediate actions to React Query and add a custom GitHub icon Co-authored-by: codex <codex@users.noreply.github.com>
- Add `gpt-5.3-codex-spark` with medium reasoning effort to commit/PR generation calls - Inject process runner into `CodexTextGenerator` and add tests for model selection and failure passthrough - Update PR modal to use status/result PR URLs and keep a single Open PR action path
- Remove branch badge and terminal toggle from ChatView header - Keep GitActionsControl in the header action area - Adjust GitActionsControl button styles to emphasize completed action state Co-authored-by: codex <codex@users.noreply.github.com>
- Render `step.detail` only for failed step states - Keep failed detail text styled in warning red tone Co-authored-by: codex <codex@users.noreply.github.com>
|
@greptileai review |
| if (upstreamRef) { | ||
| const upstreamBranch = extractBranchFromRef(upstreamRef); | ||
| if (upstreamBranch.length > 0 && upstreamBranch !== branch) { | ||
| return upstreamBranch; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium
src/gitManager.ts:455 When upstreamRef is origin/main but no local main branch exists, extractBranchFromRef returns main, causing git log main..HEAD to fail. Consider returning the full remote ref (e.g., origin/main) instead of stripping the remote prefix.
- if (upstreamRef) {
- const upstreamBranch = extractBranchFromRef(upstreamRef);
- if (upstreamBranch.length > 0 && upstreamBranch !== branch) {
- return upstreamBranch;
- }
- }
+ if (upstreamRef && upstreamRef !== `origin/${branch}`) {
+ return upstreamRef;
+ }🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/gitManager.ts around lines 455-460:
When `upstreamRef` is `origin/main` but no local `main` branch exists, `extractBranchFromRef` returns `main`, causing `git log main..HEAD` to fail. Consider returning the full remote ref (e.g., `origin/main`) instead of stripping the remote prefix.
| return `${truncated}\n\n[truncated]`; | ||
| } | ||
|
|
||
| async function writeTempFile(prefix: string, content: string): Promise<string> { |
There was a problem hiding this comment.
🟢 Low
src/codexTextGenerator.ts:80 If fs.writeFile partially fails (e.g., disk full), the temp file may be created but its path is never returned, leaving an orphan. Consider wrapping the write in try/catch and unlinking on failure before rethrowing.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/codexTextGenerator.ts around line 80:
If `fs.writeFile` partially fails (e.g., disk full), the temp file may be created but its path is never returned, leaving an orphan. Consider wrapping the write in try/catch and unlinking on failure before rethrowing.
Makes declarative settings real: a plugin declares a schema and the HOST renders the form. The plugin ships no form, no RPC, no storage. The loader now synthesises a settings page from `definition.settings`, so the declaration is no longer inert (the previous commit shipped the declaration with nothing consuming it). Generated before register(), so a plugin declaring both gets its generated page first, then its own; the page id is fixed so a plugin's own registerSettingsPage cannot shadow it. PluginSettingsPage wires the concurrency story end to end: the draft carries a revision, every save sends it back as expectedRevision, and a stale save is rejected by the server rather than silently clobbering another tab. The server's message is surfaced verbatim because "does not match the schema" and "changed elsewhere" need different actions from the user. The form opens on incompatible stored data and says so, rather than refusing — that is exactly when the user needs it to repair the values, and the stored data is preserved until a valid save (Sol spec review MUST pingdotgg#13's recovery half). This also makes the previously-vacuous test assertable. It now asserts the generated page appears, and is verified deletion-sensitive: removing the synthesis fails both new tests. `declared: false` (no schema, or plugin disabled) renders an explanation rather than an empty form. Gates: typecheck 0 errors; web 1319 passed; server 1672 passed.
Sol spec review MUST pingdotgg#13 (detection half). The store recorded a schema fingerprint from the first commit but nothing ever read it back, so an upgrade that changed a plugin's schema under already-stored values went undetected — the values were read as if they still matched, which is exactly the silent misread the fingerprint exists to prevent. readDraft now surfaces the fingerprint that produced the stored values, and the settings RPC compares it against the plugin's CURRENT schema. A mismatch marks the draft incompatible, which the settings page already renders as "these settings need attention" with the form open for repair. The stored data is preserved on mismatch, not discarded: the user may need to read the old values to reconstruct the new ones, and a plugin that briefly downgrades should not lose its config. Recovery is a valid save, which rewrites both the values and the fingerprint. Tests verified deletion-sensitive: returning a null fingerprint from readDraft fails both fingerprint tests. Remaining for MUST pingdotgg#13: hostApi.settings.get should fail InvalidStored on drift rather than relying on decode alone. The decode already fails for an incompatible shape in the common case; the fingerprint makes it explicit. Tracked, not claimed. Gates: typecheck 0 errors; server 1675 passed.
Sol spec review MUST pingdotgg#13, plugin-read half. hostApi.settings.get now rejects stored values whose fingerprint does not match the plugin's current schema, instead of relying on decode to notice. Decode is not sufficient and that is the whole point: a widening change (adding an optional field, relaxing a filter) still decodes cleanly, so the plugin would silently run on values written for a shape it no longer declares. Comparing the fingerprint makes the mismatch the fact being checked rather than a side effect of decoding. The error carries no values — only the plugin id. KNOWN GAP, stated rather than implied: this path has no test. I wrote one with a fixture that probes settings.get from a service and records the Exit tag to its data dir; the probe did not get scheduled within the test's wait, and the "<probe never ran>" guard I built into it caught that rather than letting it pass vacuously against whatever I asserted. Rather than ship a test that cannot reliably observe the behaviour — or one that asserts only "the plugin activated", which would pass with this check deleted — the tests are removed and the gap recorded. Store-level fingerprint behaviour IS covered and verified deletion-sensitive (PluginSettingsStore.test.ts); what is untested is specifically the capability read path rejecting on drift. Needs a fixture harness that can await a forked service. Gates: typecheck 0 errors; server 1675 passed.
Makes declarative settings real: a plugin declares a schema and the HOST renders the form. The plugin ships no form, no RPC, no storage. The loader now synthesises a settings page from `definition.settings`, so the declaration is no longer inert (the previous commit shipped the declaration with nothing consuming it). Generated before register(), so a plugin declaring both gets its generated page first, then its own; the page id is fixed so a plugin's own registerSettingsPage cannot shadow it. PluginSettingsPage wires the concurrency story end to end: the draft carries a revision, every save sends it back as expectedRevision, and a stale save is rejected by the server rather than silently clobbering another tab. The server's message is surfaced verbatim because "does not match the schema" and "changed elsewhere" need different actions from the user. The form opens on incompatible stored data and says so, rather than refusing — that is exactly when the user needs it to repair the values, and the stored data is preserved until a valid save (Sol spec review MUST pingdotgg#13's recovery half). This also makes the previously-vacuous test assertable. It now asserts the generated page appears, and is verified deletion-sensitive: removing the synthesis fails both new tests. `declared: false` (no schema, or plugin disabled) renders an explanation rather than an empty form. Gates: typecheck 0 errors; web 1319 passed; server 1672 passed.
Sol spec review MUST pingdotgg#13 (detection half). The store recorded a schema fingerprint from the first commit but nothing ever read it back, so an upgrade that changed a plugin's schema under already-stored values went undetected — the values were read as if they still matched, which is exactly the silent misread the fingerprint exists to prevent. readDraft now surfaces the fingerprint that produced the stored values, and the settings RPC compares it against the plugin's CURRENT schema. A mismatch marks the draft incompatible, which the settings page already renders as "these settings need attention" with the form open for repair. The stored data is preserved on mismatch, not discarded: the user may need to read the old values to reconstruct the new ones, and a plugin that briefly downgrades should not lose its config. Recovery is a valid save, which rewrites both the values and the fingerprint. Tests verified deletion-sensitive: returning a null fingerprint from readDraft fails both fingerprint tests. Remaining for MUST pingdotgg#13: hostApi.settings.get should fail InvalidStored on drift rather than relying on decode alone. The decode already fails for an incompatible shape in the common case; the fingerprint makes it explicit. Tracked, not claimed. Gates: typecheck 0 errors; server 1675 passed.
Sol spec review MUST pingdotgg#13, plugin-read half. hostApi.settings.get now rejects stored values whose fingerprint does not match the plugin's current schema, instead of relying on decode to notice. Decode is not sufficient and that is the whole point: a widening change (adding an optional field, relaxing a filter) still decodes cleanly, so the plugin would silently run on values written for a shape it no longer declares. Comparing the fingerprint makes the mismatch the fact being checked rather than a side effect of decoding. The error carries no values — only the plugin id. KNOWN GAP, stated rather than implied: this path has no test. I wrote one with a fixture that probes settings.get from a service and records the Exit tag to its data dir; the probe did not get scheduled within the test's wait, and the "<probe never ran>" guard I built into it caught that rather than letting it pass vacuously against whatever I asserted. Rather than ship a test that cannot reliably observe the behaviour — or one that asserts only "the plugin activated", which would pass with this check deleted — the tests are removed and the gap recorded. Store-level fingerprint behaviour IS covered and verified deletion-sensitive (PluginSettingsStore.test.ts); what is untested is specifically the capability read path rejecting on drift. Needs a fixture harness that can await a forked service. Gates: typecheck 0 errors; server 1675 passed.
Makes declarative settings real: a plugin declares a schema and the HOST renders the form. The plugin ships no form, no RPC, no storage. The loader now synthesises a settings page from `definition.settings`, so the declaration is no longer inert (the previous commit shipped the declaration with nothing consuming it). Generated before register(), so a plugin declaring both gets its generated page first, then its own; the page id is fixed so a plugin's own registerSettingsPage cannot shadow it. PluginSettingsPage wires the concurrency story end to end: the draft carries a revision, every save sends it back as expectedRevision, and a stale save is rejected by the server rather than silently clobbering another tab. The server's message is surfaced verbatim because "does not match the schema" and "changed elsewhere" need different actions from the user. The form opens on incompatible stored data and says so, rather than refusing — that is exactly when the user needs it to repair the values, and the stored data is preserved until a valid save (Sol spec review MUST pingdotgg#13's recovery half). This also makes the previously-vacuous test assertable. It now asserts the generated page appears, and is verified deletion-sensitive: removing the synthesis fails both new tests. `declared: false` (no schema, or plugin disabled) renders an explanation rather than an empty form. Gates: typecheck 0 errors; web 1319 passed; server 1672 passed.
Sol spec review MUST pingdotgg#13 (detection half). The store recorded a schema fingerprint from the first commit but nothing ever read it back, so an upgrade that changed a plugin's schema under already-stored values went undetected — the values were read as if they still matched, which is exactly the silent misread the fingerprint exists to prevent. readDraft now surfaces the fingerprint that produced the stored values, and the settings RPC compares it against the plugin's CURRENT schema. A mismatch marks the draft incompatible, which the settings page already renders as "these settings need attention" with the form open for repair. The stored data is preserved on mismatch, not discarded: the user may need to read the old values to reconstruct the new ones, and a plugin that briefly downgrades should not lose its config. Recovery is a valid save, which rewrites both the values and the fingerprint. Tests verified deletion-sensitive: returning a null fingerprint from readDraft fails both fingerprint tests. Remaining for MUST pingdotgg#13: hostApi.settings.get should fail InvalidStored on drift rather than relying on decode alone. The decode already fails for an incompatible shape in the common case; the fingerprint makes it explicit. Tracked, not claimed. Gates: typecheck 0 errors; server 1675 passed.
Sol spec review MUST pingdotgg#13, plugin-read half. hostApi.settings.get now rejects stored values whose fingerprint does not match the plugin's current schema, instead of relying on decode to notice. Decode is not sufficient and that is the whole point: a widening change (adding an optional field, relaxing a filter) still decodes cleanly, so the plugin would silently run on values written for a shape it no longer declares. Comparing the fingerprint makes the mismatch the fact being checked rather than a side effect of decoding. The error carries no values — only the plugin id. KNOWN GAP, stated rather than implied: this path has no test. I wrote one with a fixture that probes settings.get from a service and records the Exit tag to its data dir; the probe did not get scheduled within the test's wait, and the "<probe never ran>" guard I built into it caught that rather than letting it pass vacuously against whatever I asserted. Rather than ship a test that cannot reliably observe the behaviour — or one that asserts only "the plugin activated", which would pass with this check deleted — the tests are removed and the gap recorded. Store-level fingerprint behaviour IS covered and verified deletion-sensitive (PluginSettingsStore.test.ts); what is untested is specifically the capability read path rejecting on drift. Needs a fixture harness that can await a forked service. Gates: typecheck 0 errors; server 1675 passed.
Makes declarative settings real: a plugin declares a schema and the HOST renders the form. The plugin ships no form, no RPC, no storage. The loader now synthesises a settings page from `definition.settings`, so the declaration is no longer inert (the previous commit shipped the declaration with nothing consuming it). Generated before register(), so a plugin declaring both gets its generated page first, then its own; the page id is fixed so a plugin's own registerSettingsPage cannot shadow it. PluginSettingsPage wires the concurrency story end to end: the draft carries a revision, every save sends it back as expectedRevision, and a stale save is rejected by the server rather than silently clobbering another tab. The server's message is surfaced verbatim because "does not match the schema" and "changed elsewhere" need different actions from the user. The form opens on incompatible stored data and says so, rather than refusing — that is exactly when the user needs it to repair the values, and the stored data is preserved until a valid save (Sol spec review MUST pingdotgg#13's recovery half). This also makes the previously-vacuous test assertable. It now asserts the generated page appears, and is verified deletion-sensitive: removing the synthesis fails both new tests. `declared: false` (no schema, or plugin disabled) renders an explanation rather than an empty form. Gates: typecheck 0 errors; web 1319 passed; server 1672 passed.
Sol spec review MUST pingdotgg#13 (detection half). The store recorded a schema fingerprint from the first commit but nothing ever read it back, so an upgrade that changed a plugin's schema under already-stored values went undetected — the values were read as if they still matched, which is exactly the silent misread the fingerprint exists to prevent. readDraft now surfaces the fingerprint that produced the stored values, and the settings RPC compares it against the plugin's CURRENT schema. A mismatch marks the draft incompatible, which the settings page already renders as "these settings need attention" with the form open for repair. The stored data is preserved on mismatch, not discarded: the user may need to read the old values to reconstruct the new ones, and a plugin that briefly downgrades should not lose its config. Recovery is a valid save, which rewrites both the values and the fingerprint. Tests verified deletion-sensitive: returning a null fingerprint from readDraft fails both fingerprint tests. Remaining for MUST pingdotgg#13: hostApi.settings.get should fail InvalidStored on drift rather than relying on decode alone. The decode already fails for an incompatible shape in the common case; the fingerprint makes it explicit. Tracked, not claimed. Gates: typecheck 0 errors; server 1675 passed.
Sol spec review MUST pingdotgg#13, plugin-read half. hostApi.settings.get now rejects stored values whose fingerprint does not match the plugin's current schema, instead of relying on decode to notice. Decode is not sufficient and that is the whole point: a widening change (adding an optional field, relaxing a filter) still decodes cleanly, so the plugin would silently run on values written for a shape it no longer declares. Comparing the fingerprint makes the mismatch the fact being checked rather than a side effect of decoding. The error carries no values — only the plugin id. KNOWN GAP, stated rather than implied: this path has no test. I wrote one with a fixture that probes settings.get from a service and records the Exit tag to its data dir; the probe did not get scheduled within the test's wait, and the "<probe never ran>" guard I built into it caught that rather than letting it pass vacuously against whatever I asserted. Rather than ship a test that cannot reliably observe the behaviour — or one that asserts only "the plugin activated", which would pass with this check deleted — the tests are removed and the gap recorded. Store-level fingerprint behaviour IS covered and verified deletion-sensitive (PluginSettingsStore.test.ts); what is untested is specifically the capability read path rejecting on drift. Needs a fixture harness that can await a forked service. Gates: typecheck 0 errors; server 1675 passed.
Makes declarative settings real: a plugin declares a schema and the HOST renders the form. The plugin ships no form, no RPC, no storage. The loader now synthesises a settings page from `definition.settings`, so the declaration is no longer inert (the previous commit shipped the declaration with nothing consuming it). Generated before register(), so a plugin declaring both gets its generated page first, then its own; the page id is fixed so a plugin's own registerSettingsPage cannot shadow it. PluginSettingsPage wires the concurrency story end to end: the draft carries a revision, every save sends it back as expectedRevision, and a stale save is rejected by the server rather than silently clobbering another tab. The server's message is surfaced verbatim because "does not match the schema" and "changed elsewhere" need different actions from the user. The form opens on incompatible stored data and says so, rather than refusing — that is exactly when the user needs it to repair the values, and the stored data is preserved until a valid save (Sol spec review MUST pingdotgg#13's recovery half). This also makes the previously-vacuous test assertable. It now asserts the generated page appears, and is verified deletion-sensitive: removing the synthesis fails both new tests. `declared: false` (no schema, or plugin disabled) renders an explanation rather than an empty form. Gates: typecheck 0 errors; web 1319 passed; server 1672 passed.
Sol spec review MUST pingdotgg#13 (detection half). The store recorded a schema fingerprint from the first commit but nothing ever read it back, so an upgrade that changed a plugin's schema under already-stored values went undetected — the values were read as if they still matched, which is exactly the silent misread the fingerprint exists to prevent. readDraft now surfaces the fingerprint that produced the stored values, and the settings RPC compares it against the plugin's CURRENT schema. A mismatch marks the draft incompatible, which the settings page already renders as "these settings need attention" with the form open for repair. The stored data is preserved on mismatch, not discarded: the user may need to read the old values to reconstruct the new ones, and a plugin that briefly downgrades should not lose its config. Recovery is a valid save, which rewrites both the values and the fingerprint. Tests verified deletion-sensitive: returning a null fingerprint from readDraft fails both fingerprint tests. Remaining for MUST pingdotgg#13: hostApi.settings.get should fail InvalidStored on drift rather than relying on decode alone. The decode already fails for an incompatible shape in the common case; the fingerprint makes it explicit. Tracked, not claimed. Gates: typecheck 0 errors; server 1675 passed.
Sol spec review MUST pingdotgg#13, plugin-read half. hostApi.settings.get now rejects stored values whose fingerprint does not match the plugin's current schema, instead of relying on decode to notice. Decode is not sufficient and that is the whole point: a widening change (adding an optional field, relaxing a filter) still decodes cleanly, so the plugin would silently run on values written for a shape it no longer declares. Comparing the fingerprint makes the mismatch the fact being checked rather than a side effect of decoding. The error carries no values — only the plugin id. KNOWN GAP, stated rather than implied: this path has no test. I wrote one with a fixture that probes settings.get from a service and records the Exit tag to its data dir; the probe did not get scheduled within the test's wait, and the "<probe never ran>" guard I built into it caught that rather than letting it pass vacuously against whatever I asserted. Rather than ship a test that cannot reliably observe the behaviour — or one that asserts only "the plugin activated", which would pass with this check deleted — the tests are removed and the gap recorded. Store-level fingerprint behaviour IS covered and verified deletion-sensitive (PluginSettingsStore.test.ts); what is untested is specifically the capability read path rejecting on drift. Needs a fixture harness that can await a forked service. Gates: typecheck 0 errors; server 1675 passed.
Makes declarative settings real: a plugin declares a schema and the HOST renders the form. The plugin ships no form, no RPC, no storage. The loader now synthesises a settings page from `definition.settings`, so the declaration is no longer inert (the previous commit shipped the declaration with nothing consuming it). Generated before register(), so a plugin declaring both gets its generated page first, then its own; the page id is fixed so a plugin's own registerSettingsPage cannot shadow it. PluginSettingsPage wires the concurrency story end to end: the draft carries a revision, every save sends it back as expectedRevision, and a stale save is rejected by the server rather than silently clobbering another tab. The server's message is surfaced verbatim because "does not match the schema" and "changed elsewhere" need different actions from the user. The form opens on incompatible stored data and says so, rather than refusing — that is exactly when the user needs it to repair the values, and the stored data is preserved until a valid save (Sol spec review MUST pingdotgg#13's recovery half). This also makes the previously-vacuous test assertable. It now asserts the generated page appears, and is verified deletion-sensitive: removing the synthesis fails both new tests. `declared: false` (no schema, or plugin disabled) renders an explanation rather than an empty form. Gates: typecheck 0 errors; web 1319 passed; server 1672 passed.
Sol spec review MUST pingdotgg#13 (detection half). The store recorded a schema fingerprint from the first commit but nothing ever read it back, so an upgrade that changed a plugin's schema under already-stored values went undetected — the values were read as if they still matched, which is exactly the silent misread the fingerprint exists to prevent. readDraft now surfaces the fingerprint that produced the stored values, and the settings RPC compares it against the plugin's CURRENT schema. A mismatch marks the draft incompatible, which the settings page already renders as "these settings need attention" with the form open for repair. The stored data is preserved on mismatch, not discarded: the user may need to read the old values to reconstruct the new ones, and a plugin that briefly downgrades should not lose its config. Recovery is a valid save, which rewrites both the values and the fingerprint. Tests verified deletion-sensitive: returning a null fingerprint from readDraft fails both fingerprint tests. Remaining for MUST pingdotgg#13: hostApi.settings.get should fail InvalidStored on drift rather than relying on decode alone. The decode already fails for an incompatible shape in the common case; the fingerprint makes it explicit. Tracked, not claimed. Gates: typecheck 0 errors; server 1675 passed.
Sol spec review MUST pingdotgg#13, plugin-read half. hostApi.settings.get now rejects stored values whose fingerprint does not match the plugin's current schema, instead of relying on decode to notice. Decode is not sufficient and that is the whole point: a widening change (adding an optional field, relaxing a filter) still decodes cleanly, so the plugin would silently run on values written for a shape it no longer declares. Comparing the fingerprint makes the mismatch the fact being checked rather than a side effect of decoding. The error carries no values — only the plugin id. KNOWN GAP, stated rather than implied: this path has no test. I wrote one with a fixture that probes settings.get from a service and records the Exit tag to its data dir; the probe did not get scheduled within the test's wait, and the "<probe never ran>" guard I built into it caught that rather than letting it pass vacuously against whatever I asserted. Rather than ship a test that cannot reliably observe the behaviour — or one that asserts only "the plugin activated", which would pass with this check deleted — the tests are removed and the gap recorded. Store-level fingerprint behaviour IS covered and verified deletion-sensitive (PluginSettingsStore.test.ts); what is untested is specifically the capability read path rejecting on drift. Needs a fixture harness that can await a forked service. Gates: typecheck 0 errors; server 1675 passed.
* docs: spec for preview servers in the T3 Code web app Approved spec covering the Servers right-panel view, Moatless-owned preview tabs backed by Redis, and an iframe renderer that lets the existing browser panel work outside Electron. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat: add the servers.* contract group Three reads over the servers a thread's environment declares: list them, watch their status, and follow one's log. Field names are taken verbatim from the host's own server record so the server side is a rename-free serialization. Nothing here starts, stops or reconfigures a server; those methods are specified elsewhere and deliberately out of scope. The preview module docstring said the preview is desktop-only and that the desktop renderer mediates. Both stop being true in the same change, so the docstring now describes the surface by capability rather than by client. * feat: carry the servers.* group through the client runtime and reference server Client-side atoms for the three reads, with the log subscription folding lines into a bounded buffer so a remounted panel keeps what it received rather than starting over. The reference server declares no thread servers - it runs threads on the machine it is on - so it answers the list empty and both subscriptions silent. A hosted environment answers the same three methods with real data. Also splits the preview runtime capability in two. One boolean was answering both "can this runtime show a page" and "can this runtime read the page it shows", which are the same question only on the desktop app. * feat: host preview pages in a sandboxed frame on the web The web app could not show a task's page at all. It can now: one frame per open tab, mounted at the app root so it survives the panel collapsing, and positioned by the same surface rect the desktop webview uses. The frame is driven and never read. Navigation writes to the server and the frame follows; refresh replaces the element because a cross-origin frame has no reload and reassigning src would grow the parent's history; back and forward are absent rather than dead. Two things a frame cannot report, the panel says from elsewhere. A page that is not there is explained by what its server says about itself, which is better than a net error - "installing" is an answer no net error carries. A page that renders nothing while its server says it is running gets a hint offered as a hint, because a frame-ancestors refusal and the preview host's own 401 look identical from outside and neither fires an event. * feat: add a Servers view to the right panel One row per server the thread's environment declares, with its status kept current by the subscription, its log on demand, and an Open that hands the URL to a browser tab. Everything in it is a read - a row that says failed offers its log and no button. Restarting a server is a write and lands elsewhere. The right panel's persisted state moves to version 8 for the new surface kind, and its migration now drops surfaces whose kind this build does not know. That is what makes the version bump safe to downgrade away from: the rest of the workspace survives and only the unknown tab is lost. * test: cover the browser preview surface and the servers view Two products share these schemas and no test process, so the seam is a set of real Moatless responses checked in here and decoded by the schemas themselves. When its projection changes the fixture changes with it in one commit, and the decode test is what fails if the two drift. The rest covers what the change actually promises: the capability answers three runtimes, the chrome row omits controls rather than disabling them, the frame re-keys instead of reassigning src, exactly one browser host renders, and the panel state survives the version bump while dropping a kind this build does not know. * fix: state the environment's absence rather than implying it from a row The never-provisioned fixture claimed a server is listed as `stopped` with no URL. It is not. Moatless resolves status config-first from a NotFound pod, which falls back to `starting` with the ingress URL the port will have — so the panel showed `starting` forever for an environment that does not exist, and offered an Open button pointing at a 502. The fixture now carries what the backend produces, and the panel states the environment's own status above the list instead of leaving it to be inferred from rows that cannot say it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: track the fork's own delta, not just what upstream does to it The policy could answer "who wins this conflict" but not "what did we change", and the second is the question you are actually asking when git stops on a line you do not recognise. The path table only ever listed files we expected to fight over — a much smaller set than the files we changed. The whole thread-servers group, around forty files, appeared nowhere. §3 is that missing list, and a fifth hazard in the preamble names the failure it prevents: a conflict resolved toward upstream because nobody could tell our line was deliberate. Two consequences: the checklist asks for a row in the same commit as the change rather than at the next merge, and reading the inventory is now a step in resolving a conflict rather than something to remember. Also adds path-policy rows for the highest-risk of those files — `apps/server/src/ws.ts` and `RpcAuthorization.ts`, the only upstream server files the fork touches, where the resolution is to take theirs and re-add three `servers.*` entries. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary by CodeRabbit
New Features
Chores
Tests