diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 196bb0015..9cc08c884 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -372,7 +372,11 @@ const SETTINGS_SCHEMA = { label: 'Attribution', category: 'General', requiresRestart: false, - default: {}, + // Match `normalizeGitCoAuthor`'s runtime defaults so the IDE + // schema publishes the same "enabled by default" hint users see + // at runtime. The empty-object form here would silently lose + // editor-surfaced defaults. + default: { commit: true, pr: true }, description: 'Attribution added to git commits and pull requests created through Qwen Code.', showInDialog: false, diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 29685ffac..10a426b2e 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -2452,6 +2452,39 @@ describe('ShellTool', () => { ); }); + // A `-b 'flag'` mention literally inside the outer `--body "..."` + // text must NOT be picked as the body argument: the trailer + // would land mid-body, corrupting the user-approved command. + // Mirrors addCoAuthorToGitCommit's nested-match check. + it('should pick the OUTER --body when an inner -b appears in body text', async () => { + const command = + 'gh pr create --title "x" --body "docs mention -b \'flag\' here"'; + const invocation = shellTool.build({ command, is_background: false }); + const promise = invocation.execute(mockAbortSignal); + resolveExecutionPromise({ + rawOutput: Buffer.from(''), + output: '', + exitCode: 0, + signal: null, + error: null, + aborted: false, + pid: 12345, + executionMethod: 'child_process', + }); + await promise; + + const calls = mockShellExecutionService.mock.calls; + const cmd = calls[calls.length - 1]?.[0] as string; + // The trailer must appear AFTER the closing `"` of the outer + // body, not between `flag` and `here`. + expect(cmd).toMatch( + /--body "docs mention -b 'flag' here[\s\S]*Generated with Qwen Code"/, + ); + expect(cmd).not.toMatch( + /-b 'flag[\s\S]*Generated with Qwen Code[\s\S]*' here"/, + ); + }); + it('should append attribution to gh pr create --body when pr enabled', async () => { const command = 'gh pr create --title "x" --body "Summary"'; const invocation = shellTool.build({ command, is_background: false }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 571c963e3..346334d81 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -1802,14 +1802,17 @@ export class ShellToolInvocation extends BaseToolInvocation< const attributionService = CommitAttributionService.getInstance(); if (!commitCreated) { - // No new commit landed (nothing staged, hook rejected, or user - // reset right after). Drop the per-file attributions but keep the - // "since last commit" prompt counters intact — the user's next - // attempt should still credit prompts that happened during this - // failed try. - if (attributionService.hasAttributions()) { - attributionService.clearAttributions(false); - } + // HEAD didn't move in this cwd. Possible causes: + // 1. Commit failed (hook rejected, nothing staged, etc.) + // 2. User did `git commit && git reset HEAD~1` — HEAD reverted + // 3. Submodule case (`cd submodule && git commit`) — the inner + // repo's HEAD moved, ours didn't + // We can't tell these apart reliably from here. Dropping the + // per-file attributions on (1)/(2) is fine in isolation, but on + // (3) we'd silently lose the user's outer-repo edits even though + // none of them were committed. Leave attributions intact instead: + // a later successful commit will overwrite the counters and the + // accumulated aiContribution still represents real AI work. return; } @@ -2015,17 +2018,38 @@ export class ShellToolInvocation extends BaseToolInvocation< }; try { - // The two `rev-parse` calls are independent — fan out so we - // don't pay the spawn latency twice serially. Same for the - // three diff calls below once we know which form to use. - const [hasParentOutput, repoRootOutput] = await Promise.all([ - runGit('rev-parse --verify HEAD~1'), - runGit('rev-parse --show-toplevel'), - ]); - // hasParent fails for shallow clones where the parent was - // pruned, which is fine — diff-tree --root is a safe fallback - // that diffs against the empty tree. + // The four `rev-parse`-shaped calls are independent — fan out so + // we don't pay the spawn latency serially. Same for the three + // diff calls below once we know which form to use. + // - `rev-parse --verify HEAD~1`: probe whether the parent OBJECT + // is locally available (fails in shallow clones where the + // parent was pruned). + // - `rev-list --count HEAD`: total commits reachable. `=== 1` + // means HEAD truly is the root commit (no parent SHA recorded + // on HEAD), where `diff-tree --root` is correct. `> 1` means + // HEAD has a parent recorded — if --verify also failed, this + // is a shallow clone and we must NOT fall back to --root + // (which would diff against the empty tree and over-attribute). + const [hasParentOutput, commitCountOutput, repoRootOutput] = + await Promise.all([ + runGit('rev-parse --verify HEAD~1'), + runGit('rev-list --count HEAD'), + runGit('rev-parse --show-toplevel'), + ]); const hasParent = hasParentOutput.length > 0; + const totalCommits = parseInt(commitCountOutput.trim(), 10); + const isTrueRootCommit = + Number.isFinite(totalCommits) && totalCommits === 1; + // Shallow clone: HEAD has a parent but the object isn't local. + // Bail rather than over-attribute via --root. + if (!hasParent && !isTrueRootCommit) { + debugLogger.warn( + 'getCommittedFileInfo: HEAD~1 unreadable but commit is not the ' + + 'true root (shallow clone?); skipping attribution to avoid ' + + 'attributing the entire commit contents.', + ); + return empty; + } // Capture the repo root so the attribution service can // reconcile paths from `git diff` (relative to the toplevel) // against absolute paths recorded by the edit/write tools. @@ -2365,12 +2389,34 @@ export class ShellToolInvocation extends BaseToolInvocation< }; const bodyDoubleMatch = lastMatch(segment.matchAll(bodyDoublePattern)); const bodySingleMatch = lastMatch(segment.matchAll(bodySinglePattern)); + // Pick whichever match appears LAST in the segment, regardless of + // quote style — but reject any candidate that's nested inside the + // other's range. For `gh pr create --body "docs mention -b 'flag'"` + // the inner `-b 'flag'` is INSIDE the outer `--body "..."`; without + // a nesting check the inner (later) `-b` would win and the trailer + // would be spliced into the body text rather than appended after it. + const bodyMatchRange = (m: RegExpMatchArray | null) => + m ? { start: m.index ?? 0, end: (m.index ?? 0) + m[0].length } : null; + const bodyIsInside = ( + inner: RegExpMatchArray | null, + outer: RegExpMatchArray | null, + ): boolean => { + const i = bodyMatchRange(inner); + const o = bodyMatchRange(outer); + return !!(i && o && i.start >= o.start && i.end <= o.end); + }; let bodyMatch: RegExpMatchArray | null; if (bodyDoubleMatch && bodySingleMatch) { - bodyMatch = - (bodyDoubleMatch.index ?? 0) > (bodySingleMatch.index ?? 0) - ? bodyDoubleMatch - : bodySingleMatch; + if (bodyIsInside(bodySingleMatch, bodyDoubleMatch)) { + bodyMatch = bodyDoubleMatch; + } else if (bodyIsInside(bodyDoubleMatch, bodySingleMatch)) { + bodyMatch = bodySingleMatch; + } else { + bodyMatch = + (bodyDoubleMatch.index ?? 0) > (bodySingleMatch.index ?? 0) + ? bodyDoubleMatch + : bodySingleMatch; + } } else { bodyMatch = bodyDoubleMatch ?? bodySingleMatch; } diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index f3eac002b..56059da92 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -67,6 +67,10 @@ }, "gitCoAuthor": { "description": "Attribution added to git commits and pull requests created through Qwen Code.", + "default": { + "commit": true, + "pr": true + }, "anyOf": [ { "type": "boolean" diff --git a/scripts/generate-settings-schema.ts b/scripts/generate-settings-schema.ts index 3d21a065a..6da2c8f5d 100644 --- a/scripts/generate-settings-schema.ts +++ b/scripts/generate-settings-schema.ts @@ -148,7 +148,7 @@ function convertSettingToJsonSchema( break; } - // Add default value for simple types only + // Add default value for simple and object types if (setting.default !== undefined && setting.default !== null) { const defaultVal = setting.default; if ( @@ -159,6 +159,14 @@ function convertSettingToJsonSchema( schema.default = defaultVal; } else if (Array.isArray(defaultVal) && defaultVal.length > 0) { schema.default = defaultVal; + } else if ( + typeof defaultVal === 'object' && + !Array.isArray(defaultVal) && + Object.keys(defaultVal).length > 0 + ) { + // Non-empty plain object — publish so IDE editors can surface the + // default value (e.g. `{commit: true, pr: true}` for gitCoAuthor). + schema.default = defaultVal; } } @@ -166,11 +174,20 @@ function convertSettingToJsonSchema( // later expanded into an object), wrap with `anyOf` so existing values // in users' settings.json don't trip the IDE schema validator while // they wait for our migration to rewrite them on the next launch. + // + // Lift `description` and `default` to the outer (anyOf) level so IDE + // editors that surface schema-driven defaults / descriptions still see + // them — burying these behind `anyOf[N]` makes most validators ignore + // the `default`, which loses the "enabled by default" hint for any + // setting using `legacyTypes`. if (setting.legacyTypes && setting.legacyTypes.length > 0) { const description = schema.description; + const defaultVal = schema.default; delete schema.description; + delete schema.default; return { ...(description ? { description } : {}), + ...(defaultVal !== undefined ? { default: defaultVal } : {}), anyOf: [...setting.legacyTypes.map((t) => ({ type: t })), schema], }; }