Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion .claude/skills/pr-push/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,18 @@ Commit any uncommitted changes, run lint checks, fix any issues, and push the cu
git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null
```

If this succeeds (e.g., returns `origin/my-branch` or `someuser/my-branch`), the branch already has an upstream. Just push:
If this succeeds (e.g., returns `origin/my-branch` or `someuser/my-branch`), the branch already has an upstream. Push to it:

```
git push --force-with-lease
```

**Permission fallback:** If the push fails with a permission error (e.g., the branch tracks `upstream` but the current account lacks write access to `dyad-sh/dyad`), fall back to pushing to `origin` instead:

```
git push --force-with-lease -u origin HEAD
```

b. If there is NO upstream, check if a PR already exists and determine which remote it was opened from:

First, get the PR's head repository as `owner/repo`:
Expand Down Expand Up @@ -181,6 +187,8 @@ Commit any uncommitted changes, run lint checks, fix any issues, and push the cu
gh pr edit --add-label "cc:request"
```

**Permission fallback:** If this fails with a permission error (common for bot/fork accounts that lack label permissions on the upstream repo), skip label addition silently and note in the summary that the label could not be added. Do NOT fail the workflow over a label.

9. **Remove review-issue label:**

After pushing, remove the `needs-human:review-issue` label if it exists (this label indicates the issue needed human review before work started, which is now complete):
Expand Down
14 changes: 13 additions & 1 deletion rules/git-workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ When pushing changes and creating PRs:
2. If the branch hasn't been pushed before, default to pushing to `origin` (the fork `wwwillchen/dyad`), then create a PR from the fork to the upstream repo (`dyad-sh/dyad`).
3. If you cannot push to the fork due to permissions, push directly to `upstream` (`dyad-sh/dyad`) as a last resort.

**Bot account push permissions:** The `wwwillchen-bot` account does NOT have write access to `upstream` (`dyad-sh/dyad`). If a branch tracks `upstream` (e.g., `upstream/claude/...`), pushing will fail with a permission error. In this case, push to `origin` (the bot's fork at `wwwillchen-bot/dyad`) instead:

```bash
git push --force-with-lease -u origin HEAD
```

This overrides the branch's tracking remote. Always check which remote `origin` points to (`git remote -v`) — for bot workspaces, `origin` is typically the bot's fork, not the upstream repo.

## `gh pr create` branch detection

If `gh pr create` says `you must first push the current branch to a remote` even though `git push -u` succeeded, create the PR with an explicit head ref:
Expand Down Expand Up @@ -60,12 +68,16 @@ gh api graphql --input .claude/tmp/resolve_thread.json

## Adding labels to PRs

`gh pr edit --add-label` fails with a GraphQL "Projects (classic)" deprecation error on repos that had classic projects. Use the REST API instead:
`gh pr edit --add-label` can fail for two reasons:

1. **GraphQL "Projects (classic)" deprecation error** on repos that had classic projects. Use the REST API instead:

```bash
gh api repos/dyad-sh/dyad/issues/{PR_NUMBER}/labels -f "labels[]=label-name"
```

2. **Bot account permission errors:** The `wwwillchen-bot` account (and similar bot/fork accounts) may not have permission to add labels on the upstream repo (`dyad-sh/dyad`). Both `gh pr edit --add-label` and the REST API will fail with 403/permission errors. In this case, skip label addition and note it in the PR summary rather than failing the workflow. Labels can be added later by a maintainer with appropriate permissions.

## CI file access (claude-code-action)

In CI, `claude-code-action` restricts file access to the repo working directory (e.g., `/home/runner/work/dyad/dyad`). Skills that save intermediate files (like PR diffs) must use `./filename` (current working directory), **never** `/tmp/`. Using `/tmp/` causes errors like: `cat in '/tmp/pr_*_diff.patch' was blocked. For security, Claude Code may only concatenate files from the allowed working directories`.
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/chat_stream_handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ vi.mock("../ipc/utils/git_utils", () => ({
gitSetRemoteUrl: vi.fn(),
gitStatus: vi.fn().mockResolvedValue([]),
getGitUncommittedFiles: vi.fn().mockResolvedValue([]),
hasStagedChanges: vi.fn().mockResolvedValue(true),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While this mock is necessary, the pull request would be strengthened by adding a new test case to verify the new behavior. Specifically, a test that mocks hasStagedChanges to return false and asserts that gitCommit is not called, ensuring the commit-skipping logic is working as intended.

}));

// Mock paths module to control getDyadAppPath
Expand Down
85 changes: 50 additions & 35 deletions src/ipc/processors/response_processor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
gitRemove,
gitAddAll,
getGitUncommittedFiles,
hasStagedChanges,
} from "../utils/git_utils";
import { readSettings } from "@/main/settings";
import { writeMigrationFile } from "../utils/file_utils";
Expand Down Expand Up @@ -558,45 +559,59 @@ export async function processFullResponseActions(
let message = chatSummary
? `[dyad] ${chatSummary} - ${changes.join(", ")}`
: `[dyad] ${changes.join(", ")}`;
// Use chat summary, if provided, or default for commit message
let commitHash = await gitCommit({
path: appPath,
message,
});
logger.log(`Successfully committed changes: ${changes.join(", ")}`);

// Check for any uncommitted changes after the commit
uncommittedFiles = await getGitUncommittedFiles({ path: appPath });
// Verify there are actual staged changes before committing.
// Files may be "written" with identical content (e.g. regenerated by the AI),
// resulting in no actual git diff and causing "nothing to commit" errors.
// We check staged changes specifically (not the entire working tree) to avoid
// false positives from unrelated unstaged/untracked files.
const staged = await hasStagedChanges({ path: appPath });
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Don't derive updatedFiles from staged git state; ignored-file writes will now be treated as if nothing changed.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/processors/response_processor.ts, line 568:

<comment>Don't derive `updatedFiles` from staged git state; ignored-file writes will now be treated as if nothing changed.</comment>

<file context>
@@ -558,45 +559,59 @@ export async function processFullResponseActions(
+      // resulting in no actual git diff and causing "nothing to commit" errors.
+      // We check staged changes specifically (not the entire working tree) to avoid
+      // false positives from unrelated unstaged/untracked files.
+      const staged = await hasStagedChanges({ path: appPath });
+      if (!staged) {
+        logger.log(
</file context>
Fix with Cubic

if (!staged) {
logger.log(
"No actual git changes detected after staging (files may have been rewritten with identical content), skipping commit",
);
hasChanges = false;
} else {
// Use chat summary, if provided, or default for commit message
let commitHash = await gitCommit({
path: appPath,
message,
});
logger.log(`Successfully committed changes: ${changes.join(", ")}`);

if (uncommittedFiles.length > 0) {
// Stage all changes
await gitAddAll({ path: appPath });
try {
commitHash = await gitCommit({
path: appPath,
message: message + " + extra files edited outside of Dyad",
amend: true,
});
logger.log(
`Amend commit with changes outside of dyad: ${uncommittedFiles.join(", ")}`,
);
} catch (error) {
// Just log, but don't throw an error because the user can still
// commit these changes outside of Dyad if needed.
logger.error(
`Failed to commit changes outside of dyad: ${uncommittedFiles.join(", ")}`,
);
extraFilesError = (error as any).toString();
// Check for any uncommitted changes after the commit
uncommittedFiles = await getGitUncommittedFiles({ path: appPath });

if (uncommittedFiles.length > 0) {
// Stage all changes
await gitAddAll({ path: appPath });
try {
commitHash = await gitCommit({
path: appPath,
message: message + " + extra files edited outside of Dyad",
amend: true,
});
logger.log(
`Amend commit with changes outside of dyad: ${uncommittedFiles.join(", ")}`,
);
} catch (error) {
// Just log, but don't throw an error because the user can still
// commit these changes outside of Dyad if needed.
logger.error(
`Failed to commit changes outside of dyad: ${uncommittedFiles.join(", ")}`,
);
extraFilesError = (error as any).toString();
}
}
}

// Save the commit hash to the message
await db
.update(messages)
.set({
commitHash: commitHash,
})
.where(eq(messages.id, messageId));
// Save the commit hash to the message
await db
.update(messages)
.set({
commitHash: commitHash,
})
.where(eq(messages.id, messageId));
}
}
logger.log("mark as approved: hasChanges", hasChanges);
// Update the message to approved
Expand Down
23 changes: 23 additions & 0 deletions src/ipc/utils/git_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,29 @@ export async function isGitStatusClean({
}
}

export async function hasStagedChanges({
path,
}: {
path: string;
}): Promise<boolean> {
const settings = readSettings();
if (settings.enableNativeGit) {
// git diff --cached --quiet exits with 1 if there are staged changes, 0 if none
const result = await execGit(["diff", "--cached", "--quiet"], path);
if (result.exitCode !== 0 && result.exitCode !== 1) {
throw new Error(
`Failed to check staged changes: ${result.stderr.trim() || result.stdout.trim()}`,
);
}
return result.exitCode === 1;
} else {
const statusMatrix = await git.statusMatrix({ fs, dir: path });
// row[1] = HEAD status, row[3] = stage status
// If stage differs from HEAD, there are staged changes
return statusMatrix.some((row) => row[3] !== row[1]);
Comment on lines +278 to +281
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic statusMatrix.some((row) => row[3] !== row[1]) is correct but a bit cryptic. For better maintainability, consider adding a more detailed comment explaining why this comparison works for detecting staged changes with isomorphic-git's status matrix. This would help future developers understand the different cases (new, modified, deleted files) and the meaning of the numeric status codes.

    const statusMatrix = await git.statusMatrix({ fs, dir: path });
    // row[1] = HEAD status, row[3] = stage status.
    // If stage differs from HEAD, there are staged changes. This works because of the
    // status codes used by isomorphic-git's statusMatrix:
    // - head: 0=absent, 1=present
    // - stage: 0=absent, 1=identical to HEAD, 2=added, 3=modified
    // This correctly covers new (0 vs 2), modified (1 vs 3), and deleted (1 vs 0)
    // staged files, while ignoring unstaged changes.
    return statusMatrix.some((row) => row[3] !== row[1]);
References
  1. This comment aligns with the principle of favoring readability over conciseness, as detailed in the rule about explicit if/else blocks, by suggesting adding comments to clarify cryptic logic for better maintainability.

}
}

export async function gitCommit({
path,
message,
Expand Down
Loading