Skip to content

feat(send): add no-change-comment option#27

Merged
omarkohl merged 2 commits into
mainfrom
jip/add-no-change-comment-option/zuxozyyt
Jul 4, 2026
Merged

feat(send): add no-change-comment option#27
omarkohl merged 2 commits into
mainfrom
jip/add-no-change-comment-option/zuxozyyt

Conversation

@omarkohl

@omarkohl omarkohl commented Jul 4, 2026

Copy link
Copy Markdown
Owner

This is a stacked PR1. Only review commit 527d62c.

PRs:


Description

Repeated "no code changes" comments on rebase-only updates add noise
for reviewers, but suppressing them entirely would make a jip send
indistinguishable from a force-push made outside jip. Let users pick
the trade-off: default (formatted comment), short (one plain line),
or none — via --no-change-comment or the config files.

Closes #20

Co-Authored-By: Claude Fable 5 [email protected]

Footnotes

  1. A stacked PR is a pull request that depends on other pull requests. The current PR depends on the ones listed below it and MUST NOT be merged before they are merged. The PRs listed above the current one in turn depend on it and won't be merged until the current one is. Learn more about why and how to review.

@qodo-code-review

qodo-code-review Bot commented Jul 4, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

Grey Divider


Action required

1. no_change_comment option missing 📎 Requirement gap ≡ Correctness
Description
The PR introduces --no-change-comment / no-change-comment (kebab-case), but the compliance
requirement explicitly calls for a no_change_comment configuration option. Users following the
required no_change_comment name will not be able to configure the behavior as specified.
Code

cmd/send.go[R43-62]

+	sendCmd.Flags().String("no-change-comment", "default", "Comment posted when an updated PR has no code changes: default (formatted comment), short (one plain line), or none")

	_ = sendCmd.RegisterFlagCompletionFunc("base", completeJJBookmarks)
+	_ = sendCmd.RegisterFlagCompletionFunc("no-change-comment",
+		cobra.FixedCompletions([]string{"default", "short", "none"}, cobra.ShellCompDirectiveNoFileComp))
+}
+
+// sendConfigKeys lists the send flags that may be set from config files.
+// Per-invocation flags (--dry-run, --existing) are deliberately excluded.
+var sendConfigKeys = map[string]bool{
+	"base":              true,
+	"remote":            true,
+	"upstream":          true,
+	"draft":             true,
+	"no-stack":          true,
+	"rebase":            true,
+	"diff-since-jip":    true,
+	"reviewer":          true,
+	"no-change-comment": true,
+}
Evidence
PR Compliance ID 1 requires a no_change_comment configuration option. The updated CLI and config
keys are implemented as no-change-comment instead, and there is no occurrence of
no_change_comment in the PR branch files cited.

Add configurable no-change comment behavior via no_change_comment option
cmd/send.go[43-62]
docs/reference.md[36-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The compliance requirement expects a `no_change_comment` config option name, but the implementation only supports `no-change-comment`.

## Issue Context
The new flag and config plumbing use kebab-case (`no-change-comment`). To meet the requirement, the code should accept `no_change_comment` (snake_case) as an equivalent alias (at least for config files; optionally also for flags/docs).

## Fix Focus Areas
- cmd/send.go[43-80]
- docs/reference.md[36-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unknown config key fails 🐞 Bug ☼ Reliability
Description
applySendConfig aborts jip send on any config key not in sendConfigKeys, so a
.jip.toml/global config containing an extra key (typo, future-version key, or unrelated setting)
will prevent sending entirely. This is triggered because config.Load returns all top-level TOML
keys without filtering.
Code

cmd/send.go[R67-71]

+	for _, key := range slices.Sorted(maps.Keys(cfg)) {
+		if !sendConfigKeys[key] {
+			return fmt.Errorf("unsupported config key %q (supported: %s)",
+				key, strings.Join(slices.Sorted(maps.Keys(sendConfigKeys)), ", "))
+		}
Evidence
The send command loads all keys from config files and then immediately errors out on the first key
not present in the allowlist, which makes jip send fail even if the unknown key is unrelated to
send behavior.

cmd/send.go[64-79]
internal/config/config.go[38-66]
internal/config/config.go[69-93]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`applySendConfig` returns an error for any config key not in `sendConfigKeys`, which makes `jip send` fail if the config file contains any extra keys (including keys from newer jip versions, typos, or unrelated settings).

### Issue Context
Config is loaded from global + repo TOML, merged, and then applied to flags. Since config is user-/repo-controlled and likely to evolve, strict failure on unknown keys makes configs fragile and breaks forward/backward compatibility.

### Fix Focus Areas
- cmd/send.go[64-80]

### Suggested change
- Change `applySendConfig` to *skip* unknown keys instead of returning an error.
 - Option A: ignore silently.
 - Option B (preferred): emit a warning to stderr/output listing the ignored keys, but continue.
- Keep strict errors for invalid values for known keys (i.e., preserve the existing `flags.Set` error handling).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@omarkohl

omarkohl commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Changes since last push

No code changes (likely just a rebase).


View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).
View the diff locally (will only work if you fetched the older commit at some point):
git range-diff main aa5b97d 72e74d8
jj interdiff -f aa5b97d -t 72e74d8

@omarkohl omarkohl force-pushed the jip/add-no-change-comment-option/zuxozyyt branch from aa5b97d to 72e74d8 Compare July 4, 2026 08:12
@omarkohl

omarkohl commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Ignoring both Qodo comments. The first one because kebap case is fine and the second because on the one hand it belongs in the parent commit and on the other failing for unknown keys protects the user from typos and jip behaving in an unexpected manner. Using a newer config with an older jip version seems unlikely.

@omarkohl omarkohl force-pushed the jip/add-no-change-comment-option/zuxozyyt branch from 72e74d8 to 8e1dfa5 Compare July 4, 2026 09:02
@omarkohl

omarkohl commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Changes since last push

No code changes (likely just a rebase).


View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).
View the diff locally (will only work if you fetched the older commit at some point):
git range-diff main 72e74d8 8e1dfa5
jj interdiff -f 72e74d8 -t 8e1dfa5

omarkohl and others added 2 commits July 4, 2026 11:31
Load flag defaults from ~/.config/jip/config.toml (personal) and
.jip.toml in the repo root (team), repo overriding global and CLI
flags overriding both, so workflow flags like --rebase don't have
to be repeated on every invocation.

Closes #24

Co-Authored-By: Claude Fable 5 <[email protected]>
Repeated "no code changes" comments on rebase-only updates add noise
for reviewers, but suppressing them entirely would make a jip send
indistinguishable from a force-push made outside jip. Let users pick
the trade-off: default (formatted comment), short (one plain line),
or none — via --no-change-comment or the config files.

Closes #20

Co-Authored-By: Claude Fable 5 <[email protected]>
@omarkohl omarkohl force-pushed the jip/add-no-change-comment-option/zuxozyyt branch from 8e1dfa5 to 527d62c Compare July 4, 2026 09:32
@omarkohl

omarkohl commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Changes since last push

No code changes (likely just a rebase).


View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).
View the diff locally (will only work if you fetched the older commit at some point):
git range-diff main 8e1dfa5 527d62c
jj interdiff -f 8e1dfa5 -t 527d62c

@omarkohl omarkohl merged commit 527d62c into main Jul 4, 2026
1 check passed
@omarkohl omarkohl deleted the jip/add-no-change-comment-option/zuxozyyt branch July 4, 2026 09:32
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.

Reduce noise from no-change comments

1 participant