Skip to content

fix(attribution): make git attribution opt-in by default - #1335

Merged
kevincodex1 merged 1 commit into
Twigpine:mainfrom
chioarub:fix/1326-git-attribution-opt-in
May 26, 2026
Merged

kevincodex1 merged 1 commit into
Twigpine:mainfrom
chioarub:fix/1326-git-attribution-opt-in

Conversation

@chioarub

Copy link
Copy Markdown
Contributor

Summary

  • Make local commit and PR attribution opt-in by default.
  • Preserve explicit attribution via settings.attribution.commit and settings.attribution.pr.
  • Preserve deprecated includeCoAuthoredBy: true as an explicit compatibility opt-in.
  • Update settings/help wording to reflect the privacy-preserving default.

Why

Partially addresses #1326. Private, regulated, and client-sensitive repositories may not allow AI/provider/model metadata in Git history or PR text unless explicitly enabled.

Behavior

  • Default local commit attribution: off
  • Default local PR attribution: off
  • Explicit custom attribution: honored
  • Deprecated includeCoAuthoredBy: true: keeps old default behavior
  • Deprecated includeCoAuthoredBy: false: off
  • Remote session URL attribution remains unchanged because it is separate from local Git authoring metadata.

Out of scope

  • Memory write approval
  • Auto-dream / memory persistence policy
  • Repo-local policy files
  • Commit-msg hook enforcement

Testing

  • bun test src/utils/attribution.test.ts - passed, 20 pass / 0 fail.
  • bun test src/commands/commit-message/commit-message.test.ts - passed, 7 pass / 0 fail.
  • git diff --check - passed.
  • bun run build - passed.
  • bun test --max-concurrency=1 - local run reaches the changed commit-message tests successfully, but fails on 5 unrelated local/environment-sensitive tests: Gemini credential resolution, conversation recovery thinking-block filtering, startup provider alias resolution, and Bash command-not-found formatting. The CI-specific commit-message failure from the previous PR was fixed by making the test cache cleanup resilient when the cache object is absent.
  • bun run typecheck - fails on existing repo-wide typecheck baseline with many unrelated missing-module/type errors.

Partially addresses #1326.

@gnanam1990 gnanam1990 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed current head 43eeb30. The change is narrowly scoped and correctly makes local commit/PR attribution opt-in by default while preserving explicit settings.attribution.*, deprecated includeCoAuthoredBy: true, and remote-session URL attribution.

Local validation passed: bun test src/utils/attribution.test.ts src/commands/commit-message/commit-message.test.ts (27 pass). CI is green.

No blocking findings. This only addresses the git-attribution half of #1326; memory write approval remains separate follow-up work as noted in the PR.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No issues here, LGTM.

@kevincodex1 kevincodex1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@kevincodex1
kevincodex1 merged commit 6bc050e into Twigpine:main May 26, 2026
2 checks passed
discopops pushed a commit to discopops/openclaude that referenced this pull request May 28, 2026
Gravirei added a commit to Gravirei/openclaude that referenced this pull request May 28, 2026
- fix(autocompact): retry circuit breaker after cooldown (Twigpine#1375)
- fix(provider): require API key input when adding OpenGateway (Twigpine#1384)
- fix(provider): allow remote Ollama without OPENAI_API_KEY (Twigpine#952)
- fix(codex-stream): recover tool args delivered only via done events (Twigpine#1262)
- fix: route MiniMax compacting through Anthropic-compatible API (Twigpine#1154)
- fix(thinking): disable thinking for unsupported Ollama models (Twigpine#1376)
- feat(agents): set active session agent from agents menu (Twigpine#1349)
- fix(repl): show permission prompts while draft input is present (Twigpine#1393)
- fix(model): include profile models in descriptor picker (Twigpine#1361)
- Improve warning notice formatting (Twigpine#1415)
- fix(codex): allow credential storage fallback (Twigpine#1347)
- fix(attribution): make git attribution opt-in by default (Twigpine#1335)
- fix(agent): allow custom model overrides (Twigpine#1337)
- feat(query): robust multi-lingual and structural continuation nudge (Twigpine#1280)
- fix(watchers): debounce skills and settings reload bursts (Twigpine#1370)
- feat: configure API retry backoff (Twigpine#370) (Twigpine#1095)
- chore(main): release 0.15.0 (Twigpine#1325)
- ci: retrigger CodeQL after action download outage (Twigpine#1374)
- Fix launcher heap setup for long sessions (Twigpine#1242)
hotmanxp added a commit to hotmanxp/openclaude that referenced this pull request Jun 2, 2026
Apply from upstream commit 6bc050e, adapted for OpenCC branding:
- commit-message.ts: flip default for includeCoAuthoredBy (opt-in, not opt-out)
- attribution.ts: return '' early when includeCoAuthoredBy is not explicitly true;
  preserve OpenCC URL in defaultAttribution
- settings/types.ts: update describe() text to match new opt-in semantics
  ("Unspecified fields are off by default; set a non-empty string to opt in.")
- attribution.test.ts: new regression tests for opt-in path

Skipped: commit-message.test.ts (references upstream-only
setClaudeConfigHomeDirForTesting helper that doesn't exist in OpenCC's envUtils).

Refs upstream PR Twigpine#1335.
kevincodex1 pushed a commit that referenced this pull request Jun 17, 2026
#1396)

* feat(memory): add memory.autoWrite alias for autoMemoryEnabled (#1326)

The attribution half of #1326 was fixed via #1335 (merged 2026-05-26).
The memory half — '[memory writes should be] explicit and configurable'
with the exact shape `memory.autoWrite` requested by the issue —
remains.

Rather than parallel-tracking a new key, alias `memory.autoWrite` to
the existing `autoMemoryEnabled` opt-out and document the relationship.
Either key opts out; when both are set, the more restrictive (false)
value wins so a parent-scope opt-out can't be silently re-enabled by a
narrower memory.autoWrite: true.

The new `memory` namespace is intentional — future opt-in fields
(approval gates, etc.) can be added under it without claiming a new
top-level key each time.

- types.ts: add `memory.autoWrite` to the settings schema; cross-link
  to autoMemoryEnabled in the description.
- paths.ts isAutoMemoryEnabled: read both keys; opt-out wins on
  conflict; default unchanged (enabled).
- paths.test.ts (new): pins default, both opt-out paths, both opt-in
  paths, opt-out-wins-on-conflict in both directions, env-var still
  overrides settings.

Tests 7/7 green. Default behavior unchanged — this is purely an
additive discoverable alias for governance / regulated / client-
sensitive repos that prefer the namespaced shape called out in the
issue.

* fix(memory): evaluate autoWrite opt-out across raw settings sources (#1326)

isAutoMemoryEnabled() read the already-merged settings object, so source
precedence had already collapsed same-key values before the "false wins"
rule applied: a lower-priority memory.autoWrite/autoMemoryEnabled: false
opt-out was silently overwritten by a higher-priority true, re-enabling
auto-memory against the stated governance guarantee.

Evaluate the opt-out across the raw per-source settings instead, via
getEnabledSettingSources() + getSettingsForSource() (per-source cached, so
the hot path stays cheap). A single false in any source now wins, so a
parent-scope opt-out cannot be re-enabled by a narrower scope flipping the
key to true.

Test now drives per-source fixtures and covers the cross-source precedence
case (lower-priority false beats higher-priority true) for both keys.

* test(memory): stop the autoWrite test leaking settings mocks across files

The previous test mock.module()'d both settings.js and constants.js. bun's
mock.restore() does not undo mock.module(), so the constants.js stub leaked
into later serial test files and broke flagSettings.test.ts (its cache-busted
settings import still resolved the mocked getEnabledSettingSources).

Drive the real getEnabledSettingSources() via setAllowedSettingSources()
instead of mocking constants, stub only getSettingsForSource, and re-register
the real settings module after each test so nothing leaks. Coverage is
unchanged (per-source fixtures + cross-source precedence cases).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants