Skip to content

Commit c2d2d6e

Browse files
coopernetesclaude
coopernetes
and
claude
committed
feat: align S&F and proxy client output for consistency
- Remove all inline rp.sendMessage streaming lines from S&F hooks (AuthorEmailValidationHook, CommitMessageValidationHook, CheckUserPushPermissionHook, DiffScanningHook, ProxyPreReceiveHook, DiffGenerationHook) — validation summary now carries all output - Add buildValidationSummary to the S&F rejection path so fail output shows passing steps before the failure block, matching proxy mode - Fix hook issue names to canonical step names (checkAuthorEmails, checkCommitMessages, scanDiff) so summary labels render correctly - Strip [git-proxy] prefix from all sideband messages - Add getStepName() overrides on proxy filters with canonical camelCase names matching S&F hook step names - Rewrite buildValidationSummary to emit two-liner format (🔑 Checking X...\n ✅ result) matching S&F live streaming style - Update FilterChainIntegrationTest step name assertions - Add proxy streaming constraint note to CLAUDE.md - Simplify test/push-pass.sh and test/proxy-pass.sh with trap cleanup and auto-approval via dashboard API Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent dfa293c commit c2d2d6e

33 files changed

Lines changed: 255 additions & 342 deletions

‎CLAUDE.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,14 @@ Two proxy modes, both configurable per-provider:
1717
- **Store-and-forward** (`/push/<provider>/<owner>/<repo>.git`) — JGit ReceivePack receives the push locally, runs a pre-receive hook chain (`AuthorEmailValidationHook` → `CommitMessageValidationHook` → `ValidationVerifierHook`), then `ForwardingPostReceiveHook` pushes upstream using the client's credentials.
1818
- **Transparent proxy** (`/proxy/<provider>/<owner>/<repo>.git`) — Jetty's `ProxyServlet` forwards the request; a servlet filter chain (`ParseGitRequestFilter` → `EnrichPushCommitsFilter` → validation filters) inspects the pack data before it reaches the upstream.
1919

20+
## Client output — streaming constraint
21+
22+
**Store-and-forward** uses JGit `ReceivePack` pre-receive hooks. Each hook can call `rp.sendMessage()` at any point and the message streams to the git client immediately as a sideband progress packet (`remote: …`). This is how per-step progress lines are sent live.
23+
24+
**Transparent proxy** uses servlet filters. The HTTP response is a single buffered reply — there is no mechanism to stream partial output mid-filter-chain. Validation filters must _accumulate_ their result and return; `ValidationSummaryFilter` (order 4999) and `PushFinalizerFilter` (order 5000) collect everything and write one response at the end using `sendGitError`.
25+
26+
**Consequence for UX work:** to make proxy output look like S&F streaming, use `buildValidationSummary()` which formats each step in the same two-line style (`🔑 Checking X...\n ✅ result`) and is sent as a single batch. Do NOT try to stream from individual proxy filters — it will not work.
27+
2028
## Reference implementation
2129

2230
The Node.js original lives at `/home/tom/repos/git-proxy`. Refer to it for the Action/Step model, Sink interface, and filter chain patterns when porting features.

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/approval/ServiceNowApprovalGateway.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,10 @@ public ServiceNowApprovalGateway(String serviceNowBaseUrl, String serviceNowCred
2929
public ApprovalResult waitForApproval(String pushId, ProgressSender progress, Duration timeout) {
3030
// TODO: Create a ServiceNow change request ticket and poll for approval
3131
// String ticketId = createChangeRequest(pushId);
32-
// progress.send("[git-proxy] ServiceNow ticket created: " + ticketId);
32+
// progress.send("ServiceNow ticket created: " + ticketId);
3333
// return pollTicketApproval(ticketId, progress, timeout);
3434
log.info("ServiceNow approval stub: would create ticket for push {}, returning APPROVED", pushId);
35-
progress.send("[git-proxy] ServiceNow approval stub: auto-approving push " + pushId);
35+
progress.send("ServiceNow approval stub: auto-approving push " + pushId);
3636
return ApprovalResult.APPROVED;
3737
}
3838
}

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/approval/UiApprovalGateway.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ var record = pushStore.findById(pushId).orElse(null);
4343
Thread.sleep(POLL_INTERVAL.toMillis());
4444
elapsedSecs += POLL_INTERVAL.toSeconds();
4545
long remainingSecs = (deadlineMs - System.currentTimeMillis()) / 1000;
46-
progress.send(String.format(
47-
"[git-proxy] Awaiting review... (%ds elapsed, ~%ds remaining)", elapsedSecs, remainingSecs));
46+
progress.send(
47+
String.format("Awaiting review... (%ds elapsed, ~%ds remaining)", elapsedSecs, remainingSecs));
4848
} catch (InterruptedException e) {
4949
Thread.currentThread().interrupt();
5050
return ApprovalResult.TIMED_OUT;

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java‎

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -75,50 +75,38 @@ var record = pushStore.findById(validationRecordId).orElse(null);
7575

7676
// Safety net: already approved before this hook ran (race condition or re-push)
7777
if (record.getStatus() == PushStatus.APPROVED) {
78-
sendAndFlush(
79-
rp,
80-
msgOut,
81-
color(GREEN, "[git-proxy] " + sym(HEAVY_CHECK_MARK) + " Push already approved — forwarding"));
78+
sendAndFlush(rp, msgOut, color(GREEN, "" + sym(HEAVY_CHECK_MARK) + " Push already approved — forwarding"));
8279
return;
8380
}
8481

8582
// All clean pushes are BLOCKED pending human review
8683
if (record.getStatus() == PushStatus.BLOCKED) {
8784
sendAndFlush(
88-
rp,
89-
msgOut,
90-
color(YELLOW, "[git-proxy] " + sym(WARNING) + " Push requires review. Waiting for approval..."));
91-
sendAndFlush(rp, msgOut, color(CYAN, "[git-proxy] " + sym(KEY) + " Push ID: " + validationRecordId));
85+
rp, msgOut, color(YELLOW, "" + sym(WARNING) + " Push requires review. Waiting for approval..."));
86+
sendAndFlush(rp, msgOut, color(CYAN, "" + sym(KEY) + " Push ID: " + validationRecordId));
9287
if (serviceUrl != null) {
93-
sendAndFlush(
94-
rp,
95-
msgOut,
96-
color(CYAN, "[git-proxy] Review at: " + serviceUrl + "/#/push/" + validationRecordId));
88+
sendAndFlush(rp, msgOut, color(CYAN, " Review at: " + serviceUrl + "/#/push/" + validationRecordId));
9789
}
9890
if (record.getBlockedMessage() != null) {
99-
sendAndFlush(rp, msgOut, color(YELLOW, "[git-proxy] Reason: " + record.getBlockedMessage()));
91+
sendAndFlush(rp, msgOut, color(YELLOW, " Reason: " + record.getBlockedMessage()));
10092
}
10193

10294
ApprovalResult result =
10395
approvalGateway.waitForApproval(validationRecordId, msg -> sendAndFlush(rp, msgOut, msg), timeout);
10496

10597
switch (result) {
10698
case APPROVED ->
107-
sendAndFlush(
108-
rp,
109-
msgOut,
110-
color(GREEN, "[git-proxy] " + sym(HEAVY_CHECK_MARK) + " Push approved by reviewer"));
99+
sendAndFlush(rp, msgOut, color(GREEN, "" + sym(HEAVY_CHECK_MARK) + " Push approved by reviewer"));
111100
case REJECTED -> {
112101
var updated = pushStore.findById(validationRecordId).orElse(null);
113102
String reason = updated != null && updated.getAttestation() != null
114103
? updated.getAttestation().getReason()
115104
: "Push rejected by reviewer";
116-
sendAndFlush(
117-
rp, msgOut, color(RED, "[git-proxy] " + sym(CROSS_MARK) + " Push rejected: " + reason));
105+
sendAndFlush(rp, msgOut, color(RED, "" + sym(CROSS_MARK) + " Push rejected: " + reason));
118106
rejectAll(commands, reason);
119107
}
120108
case CANCELED -> {
121-
sendAndFlush(rp, msgOut, color(YELLOW, "[git-proxy] " + sym(WARNING) + " Push canceled"));
109+
sendAndFlush(rp, msgOut, color(YELLOW, "" + sym(WARNING) + " Push canceled"));
122110
rejectAll(commands, "Push canceled");
123111
}
124112
case TIMED_OUT -> {
@@ -127,8 +115,8 @@ var record = pushStore.findById(validationRecordId).orElse(null);
127115
msgOut,
128116
color(
129117
RED,
130-
"[git-proxy] " + sym(CROSS_MARK) + " Approval timed out after "
131-
+ timeout.toMinutes() + " minutes"));
118+
"" + sym(CROSS_MARK) + " Approval timed out after " + timeout.toMinutes()
119+
+ " minutes"));
132120
rejectAll(commands, "Approval timed out");
133121
}
134122
}

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/AuthorEmailValidationHook.java‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,5 @@
11
package org.finos.gitproxy.git;
22

3-
import static org.finos.gitproxy.git.GitClient.AnsiColor.*;
4-
import static org.finos.gitproxy.git.GitClient.SymbolCodes.*;
5-
import static org.finos.gitproxy.git.GitClient.color;
6-
import static org.finos.gitproxy.git.GitClient.sym;
7-
83
import java.util.ArrayList;
94
import java.util.Collection;
105
import java.util.List;
@@ -37,8 +32,6 @@ public class AuthorEmailValidationHook implements PreReceiveHook {
3732

3833
@Override
3934
public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
40-
rp.sendMessage(color(CYAN, "[git-proxy] " + sym(KEY) + " Checking author emails..."));
41-
4235
var check = new AuthorEmailCheck(commitConfig);
4336
Repository repo = rp.getRepository();
4437
List<Violation> allViolations = new ArrayList<>();
@@ -48,18 +41,11 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
4841
try {
4942
List<Violation> violations = check.check(getCommits(repo, cmd));
5043
for (Violation v : violations) {
51-
rp.sendMessage(
52-
color(RED, "[git-proxy] " + sym(CROSS_MARK) + " " + v.subject() + " — " + v.reason()));
53-
validationContext.addIssue("AuthorEmail", v.reason(), v.formattedDetail());
44+
validationContext.addIssue("checkAuthorEmails", v.reason(), v.formattedDetail());
5445
allViolations.add(v);
5546
}
56-
if (violations.isEmpty()) {
57-
rp.sendMessage(color(GREEN, "[git-proxy] " + sym(HEAVY_CHECK_MARK) + " emails OK"));
58-
}
5947
} catch (Exception e) {
6048
log.error("Failed to validate author emails for {}", cmd.getRefName(), e);
61-
rp.sendMessage(color(
62-
YELLOW, "[git-proxy] " + sym(WARNING) + " Could not validate emails: " + e.getMessage()));
6349
}
6450
}
6551

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/CheckEmptyBranchHook.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
public class CheckEmptyBranchHook implements PreReceiveHook {
3535

3636
private static final int STEP_ORDER = 2050;
37-
private static final String STEP_NAME = "CheckEmptyBranchHook";
37+
private static final String STEP_NAME = "checkEmptyBranch";
3838

3939
private final PushContext pushContext;
4040

@@ -56,7 +56,7 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
5656
msg = "Push blocked: Commit data not found. Please contact an administrator for support.";
5757
}
5858

59-
rp.sendMessage(color(RED, "[git-proxy] " + sym(NO_ENTRY) + " " + msg));
59+
rp.sendMessage(color(RED, "" + sym(NO_ENTRY) + " " + msg));
6060
cmd.setResult(ReceiveCommand.Result.REJECTED_OTHER_REASON, msg);
6161
// Chain stops once a command is rejected; remaining commands will be skipped
6262
return;

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/CheckHiddenCommitsHook.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
public class CheckHiddenCommitsHook implements PreReceiveHook {
4545

4646
private static final int STEP_ORDER = 2060;
47-
private static final String STEP_NAME = "CheckHiddenCommitsHook";
47+
private static final String STEP_NAME = "checkHiddenCommits";
4848

4949
private final PushContext pushContext;
5050

@@ -76,8 +76,8 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
7676
+ " and pushed to the remote.\n"
7777
+ "Please get approval on the commits, push them and try again.";
7878

79-
rp.sendMessage(color(RED, "[git-proxy] " + sym(NO_ENTRY) + " Push blocked — hidden commits detected"));
80-
rp.sendMessage(color(YELLOW, "[git-proxy] " + sym(WARNING) + " " + msg));
79+
rp.sendMessage(color(RED, "" + sym(NO_ENTRY) + " Push blocked — hidden commits detected"));
80+
rp.sendMessage(color(YELLOW, " " + sym(WARNING) + " " + msg));
8181

8282
for (ReceiveCommand cmd : commands) {
8383
if (cmd.getResult() == ReceiveCommand.Result.NOT_ATTEMPTED) {

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/CheckUserPushPermissionHook.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
3939
if (pushUser == null || pushUser.isEmpty()) {
4040
log.debug("No push user found in repo config, skipping permission check");
4141
pushContext.addStep(PushStep.builder()
42-
.stepName("CheckUserPushPermissionHook")
42+
.stepName("checkUserPermission")
4343
.stepOrder(STEP_ORDER)
4444
.status(StepStatus.PASS)
4545
.build());
@@ -54,7 +54,7 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
5454
RED,
5555
null);
5656
validationContext.addIssue("CheckUserPushPermissionHook", "User does not exist: " + pushUser, detail);
57-
rp.sendMessage(color(RED, "[git-proxy] " + sym(CROSS_MARK) + " " + pushUser + " — not registered"));
57+
rp.sendMessage(color(RED, " " + sym(CROSS_MARK) + " " + pushUser + " — not registered"));
5858
return;
5959
}
6060

@@ -66,14 +66,13 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
6666
RED,
6767
null);
6868
validationContext.addIssue("CheckUserPushPermissionHook", "User not authorized: " + pushUser, detail);
69-
rp.sendMessage(color(RED, "[git-proxy] " + sym(CROSS_MARK) + " " + pushUser + " — not authorized"));
69+
rp.sendMessage(color(RED, " " + sym(CROSS_MARK) + " " + pushUser + " — not authorized"));
7070
return;
7171
}
7272

7373
log.debug("Push user {} is authorized", pushUser);
74-
rp.sendMessage(color(GREEN, "[git-proxy] " + sym(HEAVY_CHECK_MARK) + " " + pushUser + " authorized"));
7574
pushContext.addStep(PushStep.builder()
76-
.stepName("CheckUserPushPermissionHook")
75+
.stepName("checkUserPermission")
7776
.stepOrder(STEP_ORDER)
7877
.status(StepStatus.PASS)
7978
.build());

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/CommitMessageValidationHook.java‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,5 @@
11
package org.finos.gitproxy.git;
22

3-
import static org.finos.gitproxy.git.GitClient.AnsiColor.*;
4-
import static org.finos.gitproxy.git.GitClient.SymbolCodes.CROSS_MARK;
5-
import static org.finos.gitproxy.git.GitClient.SymbolCodes.HEAVY_CHECK_MARK;
6-
import static org.finos.gitproxy.git.GitClient.SymbolCodes.KEY;
7-
import static org.finos.gitproxy.git.GitClient.SymbolCodes.WARNING;
8-
import static org.finos.gitproxy.git.GitClient.color;
9-
import static org.finos.gitproxy.git.GitClient.sym;
10-
113
import java.util.ArrayList;
124
import java.util.Collection;
135
import java.util.List;
@@ -40,8 +32,6 @@ public class CommitMessageValidationHook implements PreReceiveHook {
4032

4133
@Override
4234
public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
43-
rp.sendMessage(color(CYAN, "[git-proxy] " + sym(KEY) + " Checking commit messages..."));
44-
4535
var check = new CommitMessageCheck(commitConfig);
4636
Repository repo = rp.getRepository();
4737
List<Violation> allViolations = new ArrayList<>();
@@ -51,18 +41,11 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
5141
try {
5242
List<Violation> violations = check.check(getCommits(repo, cmd));
5343
for (Violation v : violations) {
54-
rp.sendMessage(
55-
color(RED, "[git-proxy] " + sym(CROSS_MARK) + " " + v.subject() + " — " + v.reason()));
56-
validationContext.addIssue("CommitMessage", v.reason(), v.formattedDetail());
44+
validationContext.addIssue("checkCommitMessages", v.reason(), v.formattedDetail());
5745
allViolations.add(v);
5846
}
59-
if (violations.isEmpty()) {
60-
rp.sendMessage(color(GREEN, "[git-proxy] " + sym(HEAVY_CHECK_MARK) + " messages OK"));
61-
}
6247
} catch (Exception e) {
6348
log.error("Failed to validate commit messages for {}", cmd.getRefName(), e);
64-
rp.sendMessage(color(
65-
YELLOW, "[git-proxy] " + sym(WARNING) + " Could not validate messages: " + e.getMessage()));
6649
}
6750
}
6851

‎jgit-proxy-core/src/main/java/org/finos/gitproxy/git/DiffGenerationHook.java‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
11
package org.finos.gitproxy.git;
22

3-
import static org.finos.gitproxy.git.GitClient.AnsiColor.*;
4-
import static org.finos.gitproxy.git.GitClient.SymbolCodes.*;
5-
63
import java.io.IOException;
74
import java.util.Collection;
85
import lombok.RequiredArgsConstructor;
@@ -70,15 +67,9 @@ public void onPreReceive(ReceivePack rp, Collection<ReceiveCommand> commands) {
7067
private void generatePushDiff(
7168
ReceivePack rp, Repository repo, String refName, String commitFrom, String commitTo, boolean isNewBranch) {
7269
try {
73-
String baseLabel = isNewBranch ? "(empty tree)" : commitFrom.substring(0, 7);
74-
rp.sendMessage(CYAN + "[git-proxy] " + LINK.emoji() + " Generating push diff " + baseLabel + ".."
75-
+ commitTo.substring(0, 7) + RESET);
76-
7770
String diff = CommitInspectionService.getFormattedDiff(repo, commitFrom, commitTo);
7871
int lines = diff.isEmpty() ? 0 : (int) diff.lines().count();
7972

80-
rp.sendMessage(CYAN + "[git-proxy] " + lines + " line(s) of diff" + RESET);
81-
8273
PushStep step = PushStep.builder()
8374
.stepName(STEP_NAME_PUSH_DIFF)
8475
.stepOrder(3000)
@@ -92,8 +83,7 @@ private void generatePushDiff(
9283
pushContext.addStep(step);
9384
} catch (IOException e) {
9485
log.error("Failed to generate push diff for {}", refName, e);
95-
rp.sendMessage(YELLOW + "[git-proxy] " + WARNING.emoji() + " Could not generate push diff: "
96-
+ e.getMessage() + RESET);
86+
log.warn("Could not generate push diff for {}: {}", refName, e.getMessage());
9787
}
9888
}
9989

@@ -123,13 +113,9 @@ private void generateDefaultBranchDiff(ReceivePack rp, Repository repo, String p
123113
String defaultBranchTip = defaultRef.getObjectId().name();
124114
String shortBranch = shortenRef(defaultBranch);
125115

126-
rp.sendMessage(CYAN + "[git-proxy] " + LINK.emoji() + " Generating diff vs " + shortBranch + RESET);
127-
128116
String diff = CommitInspectionService.getFormattedDiff(repo, defaultBranchTip, commitTo);
129117
int lines = diff.isEmpty() ? 0 : (int) diff.lines().count();
130118

131-
rp.sendMessage(CYAN + "[git-proxy] " + lines + " line(s) vs " + shortBranch + RESET);
132-
133119
PushStep step = PushStep.builder()
134120
.stepName(STEP_NAME_BRANCH_DIFF)
135121
.stepOrder(3001)

0 commit comments

Comments
 (0)