feat(config): support safe custom PR base branches - #2
Conversation
…egration branches
… and reopened PRs
…licit base semantics
📝 WalkthroughWalkthroughChangesConfigured PR base routing
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d76657be3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `--state opened`, but glab v1.5x removed it (it now exposes | ||
| // -c/--closed, -M/--merged, -A/--all); passing the unknown flag fails the | ||
| // whole command. Rely on the open-by-default behavior. | ||
| args = append(args, h.repoArgs()...) |
There was a problem hiding this comment.
When the working clone's upstream URL has changed since gate initialization, BuildHost constructs this host from the refreshed Repo.UpstreamURL, but the gate worktree's origin intentionally remains stale. This line scopes only mr list; CreatePR, UpdatePR, GetChecks, and the fallback/log-fetch mr view commands still infer the repository from that stale origin. A run can therefore push and search in the refreshed repository but create/update or monitor an MR in the former repository (or an unrelated MR with the same IID). Scope all GitLab MR/CI operations to the same resolved project.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
AGENTS.md (1)
129-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
AGENTS.mdat the policy level.These additions restate implementation details and enumerate regression tests. Keep the durable rules here, then point to the authoritative source or verification command. This reduces guidance drift.
As per coding guidelines, "
AGENTS.mdmust not repeat what the codebase already shows; point to the authoritative file or command instead."Also applies to: 214-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 129 - 136, Condense the added AGENTS.md guidance to durable policy-level rules, removing implementation details, symbol/path references, and enumerated regression tests. Point readers to the authoritative configuration/documentation source and the relevant verification command instead, while preserving the essential trust, path-matching, and CI-timeout policies.Source: Coding guidelines
internal/daemon/manager_trust_test.go (1)
629-704: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
cancelActiveRunstimeout path.
cancelActiveRunsininternal/daemon/manager.gonow returns an error when a cancelled run does not finish within 30 seconds, andstartRunWithIntentSourceaborts the new run with stagecancel_active_runs. This test exercises the success path, where the cancelled run does finish.The timeout path is a new failure mode that blocks every subsequent run on the branch. Consider a test that makes the first step ignore context cancellation, with the 30-second wait made injectable, so the abort and its telemetry stage are pinned.
Based on learnings: "Always use test-driven development for bug fixes and feature development".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/manager_trust_test.go` around lines 629 - 704, Add a test alongside TestReplacementWaitsForCancelledRunPRContinuityEvidence for the cancelActiveRuns timeout path, using an injectable short wait duration instead of the fixed 30-second delay. Make the first pipeline step ignore context cancellation so the cancelled run remains active, then assert startRunWithIntentSource aborts the replacement with stage cancel_active_runs and verifies the corresponding failure telemetry; keep the test deterministic without waiting 30 seconds.Source: Learnings
internal/daemon/manager.go (2)
232-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the recovered PR-base resolution into a helper.
loadRecoveredConfignow mixes three concerns: trusted-config loading, the protected-branch guard, and a six-branch PR-base continuity resolution. The continuity part spans lines 239-334 and contains four separate fetch-and-resolve paths that differ only in the ref they target and whether they fetch.Move lines 239-334 into a method such as
resolveRecoveredPRBase(ctx, run, repo, workDir, cfg) error.loadRecoveredConfigthen reads as: load config, guard the branch, resolve the base. The branch table becomes reviewable in isolation, and the existing tests keep covering it throughloadRecoveredConfig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/manager.go` around lines 232 - 334, Extract the recovered PR-base continuity logic from loadRecoveredConfig into a helper such as resolveRecoveredPRBase(ctx, run, repo, workDir, cfg) error, preserving all existing branches, fetch timeouts, ref handling, persistence, and error propagation. Leave loadRecoveredConfig responsible for config loading and configuredPRBaseBranchGuard, then invoke the helper and return its error before returning cfg.
786-810: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the two upstream fetch helpers into one.
fetchRunUpstreamBranchandfetchRunUpstreamBranchToPrivateRefshare the whole routing decision and differ only in the destination ref and the git call. A future change to the routing rule must be applied twice.Consider one helper that takes the destination ref, with the tracking-ref path passing
"refs/remotes/origin/"+branch.♻️ Suggested consolidation
func fetchRunUpstreamBranchToRef(ctx context.Context, workDir string, repo *db.Repo, branch, localRef string, private bool) error { originURL, err := git.GetRemoteURL(ctx, workDir, "origin") remote := repo.UpstreamURL if !repo.URLsVerified || strings.TrimSpace(repo.UpstreamURL) == "" || (err == nil && gate.SameRemoteRepository(originURL, repo.UpstreamURL)) { remote = "origin" } if private { return git.FetchRemoteBranchToPrivateRef(ctx, workDir, remote, branch, localRef) } return git.FetchRemoteBranchToRef(ctx, workDir, remote, branch, localRef) }Note that
internal/pipeline/steps/common_git.golines 284-300 hold a second, structurally identical pair. Both layers now carry the same routing rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/manager.go` around lines 786 - 810, Collapse fetchRunUpstreamBranch and fetchRunUpstreamBranchToPrivateRef into one helper that computes the remote once and accepts the destination ref plus the fetch mode; use the tracking-ref path with "refs/remotes/origin/"+branch and preserve the existing FetchRemoteBranchToRef versus FetchRemoteBranchToPrivateRef behavior. Also consolidate the structurally identical helper pair in common_git.go so the upstream routing rule is defined only once per layer.internal/pipeline/steps/intent.go (1)
259-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord when the configured base merge-base cannot be resolved.
For an explicit base, a failed
mergeBaseWithTargetsilently returnsgit.EmptyTreeSHA. The intent diff then covers the whole tree instead of the feature branch delta, and nothing reports why the match quality dropped. Intent is a hint, not a delivery gate, so failing closed is not required. Add a warning so this fallback is visible in logs.♻️ Proposed change
func resolveIntentBaseSHA(ctx context.Context, workDir, baseSHA, baseTarget string, explicitBase bool) string { if explicitBase { if mb := mergeBaseWithTarget(ctx, workDir, baseTarget); mb != "" { return mb } + slog.Warn("intent: could not resolve merge-base with configured pr.base_branch; diffing against the empty tree", + "base_target", baseTarget) return git.EmptyTreeSHA }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/steps/intent.go` around lines 259 - 265, In the resolveIntentBaseSHA function, when handling an explicit base and mergeBaseWithTarget returns an empty string, add a warning log before returning git.EmptyTreeSHA. The warning should indicate that the configured base merge-base could not be resolved so this fallback behavior is visible in logs, helping with debugging when intent diff coverage changes unexpectedly.internal/pipeline/steps/ci_fix.go (1)
26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the repeated explicit-base condition into one local.
Lines 29 and 32 repeat
sctx.Config != nil && sctx.Config.PR.HasExplicitBaseBranch(). One local variable makes the fail-closed policy easier to read and keeps future gates consistent.♻️ Proposed refactor
baseBranch := pipelineBaseBranch(sctx) baseSHA := resolvePipelineBranchBaseSHA(ctx, sctx) rebaseBaseSHA, baseResolved, baseErr := refreshRunBaseBranchTip(ctx, sctx, sctx.Run.BaseSHA, baseBranch) - if sctx.Config != nil && sctx.Config.PR.HasExplicitBaseBranch() && baseErr != nil { + explicitBase := sctx.Config != nil && sctx.Config.PR.HasExplicitBaseBranch() + if explicitBase && baseErr != nil { return false, baseErr } - if mergeConflict && sctx.Config != nil && sctx.Config.PR.HasExplicitBaseBranch() && !baseResolved { + if mergeConflict && explicitBase && !baseResolved { return false, fmt.Errorf("resolve configured pr.base_branch %q for merge-conflict repair", baseBranch) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/steps/ci_fix.go` around lines 26 - 34, Extract the repeated condition sctx.Config != nil && sctx.Config.PR.HasExplicitBaseBranch() that appears in both the line-29 and line-32 if statements into a single local boolean variable. Replace both occurrences of this condition with the variable reference to reduce duplication and improve readability.internal/pipeline/steps/pr_base_test.go (1)
199-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the push case delivered nothing to the remote.
The
prcase checks the provider transcript and provespr createnever ran. Thepushcase checks only the returned error. If a regression pushes first and reports the snapshot error afterward, this test still passes. Capture the remote tip for the run branch before and afterPushStep.Executeand assert it did not move.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/steps/pr_base_test.go` around lines 199 - 218, In the "push" case of the switch statement, add validation that the remote branch tip does not change after executing PushStep.Execute. Capture the remote tip for the run branch before the PushStep.Execute call, then capture it again after execution and assert the two values are identical to prove nothing was delivered to the remote. Keep the existing error validation for the snapshot immutability check unchanged.internal/pipeline/steps/push_test.go (1)
194-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the feature ref keeps its prior value instead of being absent.
setupConfiguredBaseRewrite(t, "")never pushesfeaturetoupstream. Therev-parsetherefore fails because the ref never existed, and it also fails ifupstreambecomes unreadable for any other reason. The assertion cannot distinguish "push was blocked" from "ref was never there".
TestCIStep_RevalidatesConfiguredBaseBeforeRepairPushininternal/pipeline/steps/ci_test.gouses the stronger form: it pushesfeaturefirst, then compares the remote SHA. Please match that shape here.♻️ Proposed change
dir, upstream, baseSHA, headSHA, snapshotSHA := setupConfiguredBaseRewrite(t, "") + gitCmd(t, dir, "push", "origin", "feature:refs/heads/feature") realGit, err := exec.LookPath("git")- if _, err := exec.Command(realGit, "--git-dir="+upstream, "rev-parse", "--verify", "refs/heads/feature").CombinedOutput(); err == nil { - t.Fatal("push mutated the feature branch after configured-base reset") + if got := gitCmd(t, upstream, "rev-parse", "refs/heads/feature"); got != headSHA { + t.Fatalf("push mutated the feature branch to %s after configured-base reset, want %s", got, headSHA) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/steps/push_test.go` around lines 194 - 196, Update the test around setupConfiguredBaseRewrite and the rev-parse assertion to first push the feature branch to upstream and capture its original remote SHA. After the push attempt, resolve the remote feature ref again and assert it still exists with the same SHA, matching the stronger pattern used by TestCIStep_RevalidatesConfiguredBaseBeforeRepairPush.internal/pipeline/steps/ci_test.go (1)
93-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail the fixture on an unknown
replacementvalue.The
switchhandles"sibling"and"backward". Every other value, including a typo, leaves the configured base atsnapshotSHAand performs no rewrite. A test that expects a rewritten base would then assert against an unmodified fixture and could pass for the wrong reason. Callers pass""intentionally, so keep that case explicit.♻️ Proposed guard
case "backward": gitCmd(t, dir, "push", "--force", "origin", baseSHA+":refs/heads/quality-assurance") + case "": + // Leave the configured base at the pushed snapshot. + default: + t.Fatalf("unknown replacement fixture %q", replacement) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/steps/ci_test.go` around lines 93 - 104, Update the replacement setup switch in the CI fixture test to explicitly allow the intentional empty replacement value, while adding a default branch that fails the test for any unknown value. Preserve the existing "sibling" and "backward" behavior and use the test's existing failure mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/axi_guidance_test.go`:
- Line 25: Add the exact phrase “PR base branch” to AGENTS.md, ensuring it
appears on the guidance surface checked by the synchronization test.
In `@internal/pipeline/executor_reconcile_test.go`:
- Around line 214-244: Use a separate fresh deadline for the retry loop around
exec.Respond in the approval validation test, rather than reusing the deadline
from the initial validation wait. Keep the existing retry behavior and timeout
error intact while ensuring the second loop receives its full timeout budget.
In `@internal/pipeline/steps/ci.go`:
- Around line 205-213: Update Execute’s base-branch error handling to return a
hard error only when explicitPRBaseInvariantFailed(baseBranchTipErr) is true.
Keep fetch, resolution, and resolveWindow deadline errors retryable so the next
poll can retry them, matching ReconcileApprovalGate behavior.
In `@internal/pipeline/steps/common_git.go`:
- Around line 219-227: Update mergeBaseWithTarget to stop using len(target) !=
40 as the raw-commit test; add and use isHexCommitSHA to classify only exactly
40-character hexadecimal values as commit SHAs, while preserving origin/<target>
resolution for 40-character branch names containing non-hex characters.
In `@internal/pipeline/steps/host.go`:
- Around line 40-49: Update the recovery fallback in the PRURL handling block to
replace host and repository as an atomic pair only when PRURL resolves to a
complete host and repository identity. Do not assign repo from prRepo when
either value alone is missing; validate that the resolved host and slug are both
present, then update both host and repo together, preserving the existing values
otherwise.
In `@internal/pipeline/steps/pr.go`:
- Around line 92-120: The PR step must preserve an existing pull request when
persisted run identity differs from the current configured base branch. At the
start of the flow containing `FindPR`, inspect `sctx.Run.PRURL` and
`sctx.Run.PRBaseBranch` before querying; when either provides a prior PR or the
persisted base differs from `baseBranch`, reuse the persisted PR or adjust the
lookup to that persisted base instead of allowing `CreatePR` to run for the same
head branch. Keep the current `FindPR` behavior for runs without persisted PR
continuity.
In `@internal/pipeline/steps/rebase.go`:
- Around line 539-545: Remove the unused baseBranch variable and its "main"
fallback from the diff-check setup, leaving resolvePipelineBranchBaseSHA(ctx,
sctx) as the next operation.
- Around line 95-108: Update the remoteBaseBranchAdvanced call in the force-push
approval check to pass baseTarget instead of baseBranch as the parameter. This
ensures the approval guard validates against the correct base target reference
(either RunPRBaseMonitorRef for explicit branches or ResolvedBaseSHA for
snapshots) instead of always checking origin/<baseBranch>, preventing silent
approval skips or inspection of unintended live refs.
- Around line 52-63: The explicit-base refresh path in the rebase step must not
use an unverified base tip. Update the refreshRunBaseBranchTip handling to
retain and validate its resolved boolean, returning an error when it is false
even if no error was returned; only assign the refreshed SHA to baseTarget after
validation. Also revise the mergeBaseWithTarget error in this block so it
describes baseTarget accurately rather than calling it a resolved commit.
In `@internal/pipeline/steps/steps_test.go`:
- Around line 122-129: Replace the bare `git push --force` command in the
resetSource branch with a safer `--force-with-lease` operation that validates
the expected SHA. Capture the current SHA of the branch being reset before the
push, then use `--force-with-lease=refs/heads/<branch>:<expectedSHA>` to anchor
the operation. Instead of calling exec.Command directly, route the command
through the context-aware shellenv helper. Finally, add a regression test case
that advances the destination ref before attempting reset, verifying that the
operation fails and the newer ref is preserved when lease validation prevents
the unsafe push.
In `@internal/scm/github/github.go`:
- Around line 191-247: Update FindPR to always request headRefName and
headRepositoryOwner, including when h.forkOwner is empty. Validate every
candidate’s head branch and source repository against the expected source before
allowing reuse, and make matchesHead enforce that repository identity rather
than accepting all candidates for same-repository searches. Preserve the
existing invalid-response errors and unique-match behavior.
In `@internal/scm/gitlab/gitlab.go`:
- Around line 206-223: Require candidate.IID > 0 in the merge-request validation
loop in internal/scm/gitlab/gitlab.go (lines 206-223), and compare the extracted
URL number against the IID unconditionally. In
internal/scm/gitlab/gitlab_test.go (lines 314-341), add a missing-IID fixture
with a numeric web_url and assert that parsing returns an error and nil PR.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 129-136: Condense the added AGENTS.md guidance to durable
policy-level rules, removing implementation details, symbol/path references, and
enumerated regression tests. Point readers to the authoritative
configuration/documentation source and the relevant verification command
instead, while preserving the essential trust, path-matching, and CI-timeout
policies.
In `@internal/daemon/manager_trust_test.go`:
- Around line 629-704: Add a test alongside
TestReplacementWaitsForCancelledRunPRContinuityEvidence for the cancelActiveRuns
timeout path, using an injectable short wait duration instead of the fixed
30-second delay. Make the first pipeline step ignore context cancellation so the
cancelled run remains active, then assert startRunWithIntentSource aborts the
replacement with stage cancel_active_runs and verifies the corresponding failure
telemetry; keep the test deterministic without waiting 30 seconds.
In `@internal/daemon/manager.go`:
- Around line 232-334: Extract the recovered PR-base continuity logic from
loadRecoveredConfig into a helper such as resolveRecoveredPRBase(ctx, run, repo,
workDir, cfg) error, preserving all existing branches, fetch timeouts, ref
handling, persistence, and error propagation. Leave loadRecoveredConfig
responsible for config loading and configuredPRBaseBranchGuard, then invoke the
helper and return its error before returning cfg.
- Around line 786-810: Collapse fetchRunUpstreamBranch and
fetchRunUpstreamBranchToPrivateRef into one helper that computes the remote once
and accepts the destination ref plus the fetch mode; use the tracking-ref path
with "refs/remotes/origin/"+branch and preserve the existing
FetchRemoteBranchToRef versus FetchRemoteBranchToPrivateRef behavior. Also
consolidate the structurally identical helper pair in common_git.go so the
upstream routing rule is defined only once per layer.
In `@internal/pipeline/steps/ci_fix.go`:
- Around line 26-34: Extract the repeated condition sctx.Config != nil &&
sctx.Config.PR.HasExplicitBaseBranch() that appears in both the line-29 and
line-32 if statements into a single local boolean variable. Replace both
occurrences of this condition with the variable reference to reduce duplication
and improve readability.
In `@internal/pipeline/steps/ci_test.go`:
- Around line 93-104: Update the replacement setup switch in the CI fixture test
to explicitly allow the intentional empty replacement value, while adding a
default branch that fails the test for any unknown value. Preserve the existing
"sibling" and "backward" behavior and use the test's existing failure mechanism.
In `@internal/pipeline/steps/intent.go`:
- Around line 259-265: In the resolveIntentBaseSHA function, when handling an
explicit base and mergeBaseWithTarget returns an empty string, add a warning log
before returning git.EmptyTreeSHA. The warning should indicate that the
configured base merge-base could not be resolved so this fallback behavior is
visible in logs, helping with debugging when intent diff coverage changes
unexpectedly.
In `@internal/pipeline/steps/pr_base_test.go`:
- Around line 199-218: In the "push" case of the switch statement, add
validation that the remote branch tip does not change after executing
PushStep.Execute. Capture the remote tip for the run branch before the
PushStep.Execute call, then capture it again after execution and assert the two
values are identical to prove nothing was delivered to the remote. Keep the
existing error validation for the snapshot immutability check unchanged.
In `@internal/pipeline/steps/push_test.go`:
- Around line 194-196: Update the test around setupConfiguredBaseRewrite and the
rev-parse assertion to first push the feature branch to upstream and capture its
original remote SHA. After the push attempt, resolve the remote feature ref
again and assert it still exists with the same SHA, matching the stronger
pattern used by TestCIStep_RevalidatesConfiguredBaseBeforeRepairPush.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5b99f2b-7b91-41fc-ae41-179fbdf6f301
📒 Files selected for processing (76)
AGENTS.mddocs/src/content/docs/concepts/gate-model.mddocs/src/content/docs/concepts/pipeline.mddocs/src/content/docs/guides/agents.mddocs/src/content/docs/guides/configuration.mddocs/src/content/docs/guides/setup-wizard.mddocs/src/content/docs/guides/troubleshooting.mddocs/src/content/docs/guides/tui.mddocs/src/content/docs/reference/cli.mddocs/src/content/docs/reference/global-config.mddocs/src/content/docs/reference/pipeline-steps.mddocs/src/content/docs/reference/repo-config.mddocs/src/content/docs/start-here/quick-start.mdinternal/bitbucket/client.gointernal/bitbucket/client_test.gointernal/bitbucket/host.gointernal/cli/attach.gointernal/cli/axi_drive.gointernal/cli/axi_guidance.gointernal/cli/axi_guidance_test.gointernal/cli/axi_test.gointernal/cli/wizard.gointernal/cli/wizard_test.gointernal/config/config.gointernal/config/config_pr_test.gointernal/daemon/daemon.gointernal/daemon/helpers_test.gointernal/daemon/manager.gointernal/daemon/manager_repo_refresh_test.gointernal/daemon/manager_test.gointernal/daemon/manager_trust_test.gointernal/daemon/startup_recovery_test.gointernal/daemon/subscribe_recover_test.gointernal/db/run.gointernal/db/run_test.gointernal/db/schema.gointernal/gate/refresh.gointernal/gate/refresh_test.gointernal/git/git.gointernal/pipeline/executor.gointernal/pipeline/executor_reconcile_test.gointernal/pipeline/pipeline.gointernal/pipeline/steps/ci.gointernal/pipeline/steps/ci_fix.gointernal/pipeline/steps/ci_merge_test.gointernal/pipeline/steps/ci_test.gointernal/pipeline/steps/common_git.gointernal/pipeline/steps/demo.gointernal/pipeline/steps/document.gointernal/pipeline/steps/helpers_test.gointernal/pipeline/steps/host.gointernal/pipeline/steps/intent.gointernal/pipeline/steps/intent_integration_test.gointernal/pipeline/steps/lint.gointernal/pipeline/steps/pr.gointernal/pipeline/steps/pr_base_test.gointernal/pipeline/steps/pr_test.gointernal/pipeline/steps/push.gointernal/pipeline/steps/push_test.gointernal/pipeline/steps/rebase.gointernal/pipeline/steps/rebase_local_default_test.gointernal/pipeline/steps/rebase_test.gointernal/pipeline/steps/review.gointernal/pipeline/steps/steps_test.gointernal/pipeline/steps/test.gointernal/pipeline/steps/upstream_test.gointernal/scm/azuredevops/azuredevops.gointernal/scm/azuredevops/azuredevops_test.gointernal/scm/github/github.gointernal/scm/github/github_test.gointernal/scm/gitlab/gitlab.gointernal/scm/gitlab/gitlab_test.gointernal/scm/host.gointernal/skill/skill.gointernal/wizard/wizard.goskills/no-mistakes/SKILL.md
| // auto-rebases and re-pushes such a PR, so the agent runs no command and never | ||
| // hand-rebases, and `no-mistakes rerun` is only the dead-monitor recovery. | ||
| var canonicalStaleMonitorPhrases = []string{ | ||
| "PR base branch", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in AGENTS.md internal/skill/skill.go internal/cli/axi_guidance.go; do
rg -F -q 'PR base branch' "$file" || {
printf 'missing exact phrase in %s\n' "$file" >&2
exit 1
}
doneRepository: Thaloz/thz-no-mistakes
Length of output: 194
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- test ---'
sed -n '1,90p' internal/cli/axi_guidance_test.go
printf '%s\n' '--- exact phrase occurrences ---'
rg -n -F 'PR base branch' AGENTS.md internal/skill/skill.go internal/cli/axi_guidance.go || true
printf '%s\n' '--- related guidance terms in AGENTS.md ---'
rg -n -i 'pr[ -]?base|base branch|base_branch' AGENTS.md || trueRepository: Thaloz/thz-no-mistakes
Length of output: 8359
Add PR base branch to AGENTS.md.
The synchronization test requires this phrase on every guidance surface, and AGENTS.md is missing it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/axi_guidance_test.go` at line 25, Add the exact phrase “PR base
branch” to AGENTS.md, ensuring it appears on the guidance surface checked by the
synchronization test.
| deadline := time.Now().Add(3 * time.Second) | ||
| for step.validationCalls.Load() != 1 && time.Now().Before(deadline) { | ||
| time.Sleep(5 * time.Millisecond) | ||
| } | ||
| if step.validationCalls.Load() != 1 { | ||
| t.Fatal("approval validation did not run") | ||
| } | ||
| select { | ||
| case err := <-done: | ||
| t.Fatalf("transient approval validation ended the run: %v", err) | ||
| case <-time.After(50 * time.Millisecond): | ||
| } | ||
| got, err := database.GetRun(run.ID) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if got.Status != types.RunRunning || got.AwaitingAgentSince == nil { | ||
| t.Fatalf("transient validation changed parked run: status %s awaiting %v", got.Status, got.AwaitingAgentSince) | ||
| } | ||
|
|
||
| step.validationErr.Store(nil) | ||
| for { | ||
| err = exec.Respond(types.StepCI, types.ActionApprove, nil) | ||
| if err == nil { | ||
| break | ||
| } | ||
| if time.Now().After(deadline) { | ||
| t.Fatalf("parked gate was not restored after transient validation: %v", err) | ||
| } | ||
| time.Sleep(5 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reuse of deadline can make the retry loop fail early.
Line 214 computes deadline before the first wait. Line 240 reuses the same deadline for the retry loop. If the first wait consumes the full budget, the retry loop gets no time and the test fails with a misleading message. Compute a fresh deadline for the second loop.
♻️ Proposed fix
step.validationErr.Store(nil)
+ retryDeadline := time.Now().Add(3 * time.Second)
for {
err = exec.Respond(types.StepCI, types.ActionApprove, nil)
if err == nil {
break
}
- if time.Now().After(deadline) {
+ if time.Now().After(retryDeadline) {
t.Fatalf("parked gate was not restored after transient validation: %v", err)
}
time.Sleep(5 * time.Millisecond)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deadline := time.Now().Add(3 * time.Second) | |
| for step.validationCalls.Load() != 1 && time.Now().Before(deadline) { | |
| time.Sleep(5 * time.Millisecond) | |
| } | |
| if step.validationCalls.Load() != 1 { | |
| t.Fatal("approval validation did not run") | |
| } | |
| select { | |
| case err := <-done: | |
| t.Fatalf("transient approval validation ended the run: %v", err) | |
| case <-time.After(50 * time.Millisecond): | |
| } | |
| got, err := database.GetRun(run.ID) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if got.Status != types.RunRunning || got.AwaitingAgentSince == nil { | |
| t.Fatalf("transient validation changed parked run: status %s awaiting %v", got.Status, got.AwaitingAgentSince) | |
| } | |
| step.validationErr.Store(nil) | |
| for { | |
| err = exec.Respond(types.StepCI, types.ActionApprove, nil) | |
| if err == nil { | |
| break | |
| } | |
| if time.Now().After(deadline) { | |
| t.Fatalf("parked gate was not restored after transient validation: %v", err) | |
| } | |
| time.Sleep(5 * time.Millisecond) | |
| } | |
| deadline := time.Now().Add(3 * time.Second) | |
| for step.validationCalls.Load() != 1 && time.Now().Before(deadline) { | |
| time.Sleep(5 * time.Millisecond) | |
| } | |
| if step.validationCalls.Load() != 1 { | |
| t.Fatal("approval validation did not run") | |
| } | |
| select { | |
| case err := <-done: | |
| t.Fatalf("transient approval validation ended the run: %v", err) | |
| case <-time.After(50 * time.Millisecond): | |
| } | |
| got, err := database.GetRun(run.ID) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if got.Status != types.RunRunning || got.AwaitingAgentSince == nil { | |
| t.Fatalf("transient validation changed parked run: status %s awaiting %v", got.Status, got.AwaitingAgentSince) | |
| } | |
| step.validationErr.Store(nil) | |
| retryDeadline := time.Now().Add(3 * time.Second) | |
| for { | |
| err = exec.Respond(types.StepCI, types.ActionApprove, nil) | |
| if err == nil { | |
| break | |
| } | |
| if time.Now().After(retryDeadline) { | |
| t.Fatalf("parked gate was not restored after transient validation: %v", err) | |
| } | |
| time.Sleep(5 * time.Millisecond) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/executor_reconcile_test.go` around lines 214 - 244, Use a
separate fresh deadline for the retry loop around exec.Respond in the approval
validation test, rather than reusing the deadline from the initial validation
wait. Keep the existing retry behavior and timeout error intact while ensuring
the second loop receives its full timeout budget.
| var baseBranchTipErr error | ||
| if baseBranchTip == nil { | ||
| baseBranchTip = func(ctx context.Context) (string, bool) { | ||
| return resolveRunDefaultBranchTip(ctx, sctx, sctx.Run.BaseSHA, sctx.Repo.DefaultBranch) | ||
| tip, resolved, err := refreshRunBaseBranchTip(ctx, sctx, sctx.Run.BaseSHA, pipelineBaseBranch(sctx)) | ||
| if sctx.Config != nil && sctx.Config.PR.HasExplicitBaseBranch() { | ||
| baseBranchTipErr = err | ||
| } | ||
| return tip, resolved | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect refreshRunBaseBranchTip and the explicit-base error classifiers.
set -euo pipefail
rg -n 'func refreshRunBaseBranchTip|func validateExplicitPRBase|func explicitPRBaseInvariantFailed|func pipelineBaseTarget|func pipelineBaseBranch|immutable run snapshot|could not be fetched' internal/pipeline/steps -C 6Repository: Thaloz/thz-no-mistakes
Length of output: 17144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ci.go ---'
sed -n '160,325p' internal/pipeline/steps/ci.go
printf '%s\n' '--- related symbols ---'
rg -n 'baseBranchTipErr|resolveWindow|defaultBaseBranchTipResolveWindow|ReconcileApprovalGate|func \(.*CIStep.*Execute|for .*poll|ticker|time\\.New' internal/pipeline/steps/ci.go internal/pipeline/steps/ci_test.go -C 5
printf '%s\n' '--- test bodies ---'
sed -n '1,225p' internal/pipeline/steps/ci_test.goRepository: Thaloz/thz-no-mistakes
Length of output: 25536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ci.go ---'
sed -n '160,325p' internal/pipeline/steps/ci.go
printf '%s\n' '--- related symbols ---'
rg -n 'baseBranchTipErr|resolveWindow|defaultBaseBranchTipResolveWindow|ReconcileApprovalGate|func \(.*CIStep.*Execute|for .*poll|ticker|time\.New' internal/pipeline/steps/ci.go internal/pipeline/steps/ci_test.go -C 5
printf '%s\n' '--- test bodies ---'
sed -n '1,225p' internal/pipeline/steps/ci_test.goRepository: Thaloz/thz-no-mistakes
Length of output: 25536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ReconcileApprovalGate ---'
sed -n '56,136p' internal/pipeline/steps/ci.go
printf '%s\n' '--- refreshRunBaseBranchTip ---'
sed -n '91,170p' internal/pipeline/steps/common_git.go
printf '%s\n' '--- base-branch timeout and fetch helpers ---'
rg -n 'fetchRunUpstreamBranch(ToPrivateRef)?|defaultBaseBranchTipResolveWindow|context\\.DeadlineExceeded|explicitPRBaseInvariantFailed' internal/pipeline/steps -C 8Repository: Thaloz/thz-no-mistakes
Length of output: 16638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ReconcileApprovalGate ---'
sed -n '56,136p' internal/pipeline/steps/ci.go
printf '%s\n' '--- refreshRunBaseBranchTip ---'
sed -n '91,170p' internal/pipeline/steps/common_git.go
printf '%s\n' '--- base-branch timeout and fetch helpers ---'
rg -n 'fetchRunUpstreamBranch(ToPrivateRef)?|defaultBaseBranchTipResolveWindow|context\.DeadlineExceeded|explicitPRBaseInvariantFailed' internal/pipeline/steps -C 8Repository: Thaloz/thz-no-mistakes
Length of output: 16638
Keep transient base-branch refresh errors retryable.
In Execute, return a hard error only when explicitPRBaseInvariantFailed(baseBranchTipErr) is true. Retry fetch, resolution, and resolveWindow deadline errors on the next poll, consistent with ReconcileApprovalGate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/steps/ci.go` around lines 205 - 213, Update Execute’s
base-branch error handling to return a hard error only when
explicitPRBaseInvariantFailed(baseBranchTipErr) is true. Keep fetch, resolution,
and resolveWindow deadline errors retryable so the next poll can retry them,
matching ReconcileApprovalGate behavior.
| func mergeBaseWithTarget(ctx context.Context, workDir, target string) string { | ||
| target = strings.TrimSpace(target) | ||
| if target == "" { | ||
| return "" | ||
| } | ||
| for _, ref := range []string{"origin/" + defaultBranch, defaultBranch} { | ||
| refs := []string{target} | ||
| if !strings.HasPrefix(target, "refs/") && len(target) != 40 { | ||
| refs = []string{"origin/" + target, target} | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not use length 40 as the test for a commit SHA.
Line 225 treats any target of exactly 40 characters as a raw commit and drops the origin/<target> candidate. A branch name may legally be 40 characters long. For such a base branch that exists only as a remote-tracking ref, merge-base HEAD <name> fails, mergeBaseWithTarget returns "", and the caller silently falls back to run.BaseSHA or the empty tree. The branch-delta scope then changes with no error reported.
Test for hex content, not length.
🐛 Proposed fix
func mergeBaseWithTarget(ctx context.Context, workDir, target string) string {
target = strings.TrimSpace(target)
if target == "" {
return ""
}
refs := []string{target}
- if !strings.HasPrefix(target, "refs/") && len(target) != 40 {
+ if !strings.HasPrefix(target, "refs/") && !isHexCommitSHA(target) {
refs = []string{"origin/" + target, target}
}Add the helper alongside it:
func isHexCommitSHA(s string) bool {
if len(s) != 40 {
return false
}
for _, r := range s {
if !((r >= '0' && r <= '9') || (r >= 'a' && r <= 'f') || (r >= 'A' && r <= 'F')) {
return false
}
}
return true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func mergeBaseWithTarget(ctx context.Context, workDir, target string) string { | |
| target = strings.TrimSpace(target) | |
| if target == "" { | |
| return "" | |
| } | |
| for _, ref := range []string{"origin/" + defaultBranch, defaultBranch} { | |
| refs := []string{target} | |
| if !strings.HasPrefix(target, "refs/") && len(target) != 40 { | |
| refs = []string{"origin/" + target, target} | |
| } | |
| func mergeBaseWithTarget(ctx context.Context, workDir, target string) string { | |
| target = strings.TrimSpace(target) | |
| if target == "" { | |
| return "" | |
| } | |
| refs := []string{target} | |
| if !strings.HasPrefix(target, "refs/") && !isHexCommitSHA(target) { | |
| refs = []string{"origin/" + target, target} | |
| } | |
| func isHexCommitSHA(s string) bool { | |
| if len(s) != 40 { | |
| return false | |
| } | |
| for _, r := range s { | |
| if !((r >= '0' && r <= '9') || (r >= 'a' && r <= 'f') || (r >= 'A' && r <= 'F')) { | |
| return false | |
| } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/steps/common_git.go` around lines 219 - 227, Update
mergeBaseWithTarget to stop using len(target) != 40 as the raw-commit test; add
and use isHexCommitSHA to classify only exactly 40-character hexadecimal values
as commit SHAs, while preserving origin/<target> resolution for 40-character
branch names containing non-hex characters.
| if (repo == "" || host == "") && sctx.Run.PRURL != nil { | ||
| prHost := scm.ResolveHost(sctx.Ctx, *sctx.Run.PRURL) | ||
| repo = github.HostPrefixedSlugForHost(*sctx.Run.PRURL, prHost) | ||
| prRepo := github.HostPrefixedSlugForHost(*sctx.Run.PRURL, prHost) | ||
| // A local bare-gate path can look like owner/repo to RepoSlug even | ||
| // though it has no forge host. In recovery, use the persisted PR | ||
| // identity for both fields rather than sending an unscoped auth | ||
| // check and a filesystem-derived --repo value. | ||
| if repo == "" || host == "" { | ||
| repo = prRepo | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the fallback host and repository identity atomic.
At Line 47, repo is replaced when either value is missing, but host is replaced only when it is empty. If UpstreamURL resolves to a host without a repository slug and PRURL belongs to another GitHub host, the adapter checks authentication for one host and sends --repo for another. This breaks PR continuity recovery. Only replace both values when PRURL resolves to a complete host and repository pair.
Proposed fix
- if repo == "" || host == "" {
- repo = prRepo
- }
- if host == "" {
- host = prHost
+ if prHost != "" && prRepo != "" {
+ host, repo = prHost, prRepo
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (repo == "" || host == "") && sctx.Run.PRURL != nil { | |
| prHost := scm.ResolveHost(sctx.Ctx, *sctx.Run.PRURL) | |
| repo = github.HostPrefixedSlugForHost(*sctx.Run.PRURL, prHost) | |
| prRepo := github.HostPrefixedSlugForHost(*sctx.Run.PRURL, prHost) | |
| // A local bare-gate path can look like owner/repo to RepoSlug even | |
| // though it has no forge host. In recovery, use the persisted PR | |
| // identity for both fields rather than sending an unscoped auth | |
| // check and a filesystem-derived --repo value. | |
| if repo == "" || host == "" { | |
| repo = prRepo | |
| } | |
| if (repo == "" || host == "") && sctx.Run.PRURL != nil { | |
| prHost := scm.ResolveHost(sctx.Ctx, *sctx.Run.PRURL) | |
| prRepo := github.HostPrefixedSlugForHost(*sctx.Run.PRURL, prHost) | |
| // A local bare-gate path can look like owner/repo to RepoSlug even | |
| // though it has no forge host. In recovery, use the persisted PR | |
| // identity for both fields rather than sending an unscoped auth | |
| // check and a filesystem-derived --repo value. | |
| if prHost != "" && prRepo != "" { | |
| host, repo = prHost, prRepo | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/steps/host.go` around lines 40 - 49, Update the recovery
fallback in the PRURL handling block to replace host and repository as an atomic
pair only when PRURL resolves to a complete host and repository identity. Do not
assign repo from prRepo when either value alone is missing; validate that the
resolved host and slug are both present, then update both host and repo
together, preserving the existing values otherwise.
| if forcePush && branch == baseBranch && remoteBaseBranchAdvanced(ctx, sctx.WorkDir, baseBranch, sctx.Run.BaseSHA) { | ||
| findingsJSON, _ := json.Marshal(Findings{ | ||
| Items: []Finding{{ | ||
| Severity: "warning", | ||
| File: filepath.Join("internal", "pipeline", "steps", "rebase.go"), | ||
| Description: fmt.Sprintf("origin/%s advanced after the force push; manual review required before updating the default branch", defaultBranch), | ||
| Description: fmt.Sprintf("origin/%s advanced after the force push; manual review required before updating the PR base branch", baseBranch), | ||
| }}, | ||
| Summary: fmt.Sprintf("remote %s advanced during force push", defaultBranch), | ||
| Summary: fmt.Sprintf("remote %s advanced during force push", baseBranch), | ||
| }) | ||
| return &pipeline.StepOutcome{ | ||
| NeedsApproval: true, | ||
| Findings: string(findingsJSON), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the explicit-base path populates refs/remotes/origin/<base> or only a private ref.
set -euo pipefail
rg -n 'refreshRunBaseBranchTip|RunPRBaseMonitorRef|refs/remotes/origin/' -C 4 internal/pipeline/steps internal/gitRepository: Thaloz/thz-no-mistakes
Length of output: 15227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rebase.go ---'
cat -n internal/pipeline/steps/rebase.go | sed -n '1,330p'
printf '%s\n' '--- common_git.go ---'
cat -n internal/pipeline/steps/common_git.go | sed -n '110,330p'
printf '%s\n' '--- remoteBaseBranchAdvanced definitions and call sites ---'
rg -n 'remoteBaseBranchAdvanced|baseTarget|RunPRBaseMonitorRef' internal/pipeline/steps -C 6
printf '%s\n' '--- focused tests ---'
rg -n 'forcePush|NeedsApproval|base.*advanced|immutable run snapshot|private ref|ResolvedBaseSHA' internal/pipeline/steps/*test.go -C 5Repository: Thaloz/thz-no-mistakes
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
rebase = Path("internal/pipeline/steps/rebase.go").read_text()
common = Path("internal/pipeline/steps/common_git.go").read_text()
call = re.search(
r'if forcePush && branch == baseBranch && '
r'remoteBaseBranchAdvanced\(([^)]*)\)',
rebase,
)
body = re.search(
r'func remoteBaseBranchAdvanced\([^)]*\) bool \{(.*?)\n\}',
rebase,
re.S,
)
assert call, "force-push approval call not found"
assert body, "remoteBaseBranchAdvanced definition not found"
args = [x.strip() for x in call.group(1).split(",")]
function_body = body.group(1)
print("call arguments:", args)
print("uses baseTarget at call:", "baseTarget" in args)
print("hard-coded origin lookup:", '"origin/"+baseBranch' in function_body)
print("explicit-base refresh target:", "ref = git.RunPRBaseMonitorRef(sctx.Run.ID)" in common)
print("explicit-base fetch target:", "fetchRunUpstreamBranchToPrivateRef" in common)
assert "baseTarget" not in args
assert '"origin/"+baseBranch' in function_body
assert "ref = git.RunPRBaseMonitorRef(sctx.Run.ID)" in common
assert "fetchRunUpstreamBranchToPrivateRef" in common
PY
printf '%s\n' '--- pipelineBaseTarget definition ---'
rg -n 'func pipelineBaseTarget|ResolvedBaseSHA|RunPRBaseRef' internal/pipeline/steps -C 5Repository: Thaloz/thz-no-mistakes
Length of output: 20605
Use baseTarget in the force-push approval check.
For an explicit pr.base_branch, the refresh writes to RunPRBaseMonitorRef, while a configured snapshot uses ResolvedBaseSHA; neither requires origin/<baseBranch>. Update remoteBaseBranchAdvanced to resolve baseTarget so the guard cannot silently skip approval or inspect a different live ref.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/steps/rebase.go` around lines 95 - 108, Update the
remoteBaseBranchAdvanced call in the force-push approval check to pass
baseTarget instead of baseBranch as the parameter. This ensures the approval
guard validates against the correct base target reference (either
RunPRBaseMonitorRef for explicit branches or ResolvedBaseSHA for snapshots)
instead of always checking origin/<baseBranch>, preventing silent approval skips
or inspection of unintended live refs.
| // Check if the branch has any diff against the intended PR base. | ||
| // If the diff is empty (e.g. branch was already merged), skip remaining steps. | ||
| defaultBranch := strings.TrimSpace(sctx.Repo.DefaultBranch) | ||
| if defaultBranch == "" { | ||
| defaultBranch = "main" | ||
| baseBranch := pipelineBaseBranch(sctx) | ||
| if baseBranch == "" { | ||
| baseBranch = "main" | ||
| } | ||
| baseSHA := resolveBranchBaseSHA(ctx, sctx.WorkDir, sctx.Run.BaseSHA, defaultBranch) | ||
| baseSHA := resolvePipelineBranchBaseSHA(ctx, sctx) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused baseBranch variable.
resolvePipelineBranchBaseSHA(ctx, sctx) takes no branch argument, so baseBranch and its "main" fallback are never read. golangci-lint reports ineffectual assignment to baseBranch at Line 543, and make lint runs the linter, so this fails local verification.
🐛 Proposed fix
// Check if the branch has any diff against the intended PR base.
// If the diff is empty (e.g. branch was already merged), skip remaining steps.
- baseBranch := pipelineBaseBranch(sctx)
- if baseBranch == "" {
- baseBranch = "main"
- }
baseSHA := resolvePipelineBranchBaseSHA(ctx, sctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if the branch has any diff against the intended PR base. | |
| // If the diff is empty (e.g. branch was already merged), skip remaining steps. | |
| defaultBranch := strings.TrimSpace(sctx.Repo.DefaultBranch) | |
| if defaultBranch == "" { | |
| defaultBranch = "main" | |
| baseBranch := pipelineBaseBranch(sctx) | |
| if baseBranch == "" { | |
| baseBranch = "main" | |
| } | |
| baseSHA := resolveBranchBaseSHA(ctx, sctx.WorkDir, sctx.Run.BaseSHA, defaultBranch) | |
| baseSHA := resolvePipelineBranchBaseSHA(ctx, sctx) | |
| // Check if the branch has any diff against the intended PR base. | |
| // If the diff is empty (e.g. branch was already merged), skip remaining steps. | |
| baseSHA := resolvePipelineBranchBaseSHA(ctx, sctx) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 543-543: ineffectual assignment to baseBranch
(ineffassign)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/steps/rebase.go` around lines 539 - 545, Remove the unused
baseBranch variable and its "main" fallback from the diff-check setup, leaving
resolvePipelineBranchBaseSHA(ctx, sctx) as the next operation.
Source: Linters/SAST tools
| if resetSource := os.Getenv("FAKE_CLI_PR_LIST_RESET_SOURCE"); resetSource != "" { | ||
| resetBranch := os.Getenv("FAKE_CLI_PR_LIST_RESET_BRANCH") | ||
| cmd := exec.Command("git", "push", "--force", "origin", resetSource+":refs/heads/"+resetBranch) | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "reset PR base: %v: %s", err, out) | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target files ---'
git ls-files 'internal/pipeline/steps/steps_test.go' 'internal/pipeline/steps/*.go' 'internal/shellenv/*.go' 'internal/winproc/*.go' 'internal/safeurl/*.go'
printf '%s\n' '--- target file outline ---'
ast-grep outline internal/pipeline/steps/steps_test.go
printf '%s\n' '--- target sections ---'
sed -n '1,230p' internal/pipeline/steps/steps_test.go
printf '%s\n' '--- shellenv symbols ---'
rg -n 'func (ConfigureShellCommand|RunShellCommand|OutputShellCommand|CombinedOutputShellCommand|StartShellCommand|TerminateShellCommandGroup)|package shellenv' internal
printf '%s\n' '--- related test helpers and force pushes ---'
rg -n -C 4 -- '--force|FAKE_CLI_PR_LIST_(RESET_SOURCE|RESET_BRANCH)|git push' internal/pipeline/stepsRepository: Thaloz/thz-no-mistakes
Length of output: 29023
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository status and relevant imports ---'
git status --short
sed -n '1,70p' internal/pipeline/steps/steps_test.go
printf '%s\n' '--- all shellenv package files ---'
fd -t f . | rg '(^|/)(shellenv|winproc)(/|$)|shellenv|winproc'
printf '%s\n' '--- helper definitions and call sites ---'
rg -n -C 6 'ConfigureShellCommand|CombinedOutputShellCommand|OutputShellCommand|RunShellCommand|StartShellCommand|TerminateShellCommandGroup' .Repository: Thaloz/thz-no-mistakes
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact reset helpers ---'
sed -n '100,220p' internal/pipeline/steps/steps_test.go
printf '%s\n' '--- configured-base tests and fixture setup ---'
sed -n '1,290p' internal/pipeline/steps/pr_base_test.go
printf '%s\n' '--- fixture and fake CLI helpers ---'
rg -n -C 8 'func (fakeGH|fakeGit|setupConfiguredBaseRewrite|fakeGH|fakeGit)|FAKE_CLI_PR_LIST|FAKE_CLI_RESET_BASE|origin' internal/pipeline/steps/pr_base_test.go internal/pipeline/steps/ci_test.go internal/pipeline/steps/helpers_test.go
printf '%s\n' '--- relevant imports and git helpers ---'
sed -n '1,90p' internal/pipeline/steps/pr_base_test.go
sed -n '1,180p' internal/pipeline/steps/helpers_test.goRepository: Thaloz/thz-no-mistakes
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- subprocess policy in test package ---'
rg -n 'exec\.Command(Context)?\(' internal/pipeline/steps --glob '*_test.go'
printf '%s\n' '--- git command helper implementation ---'
rg -n -C 10 '^func gitCmd|func .*Git.*Cmd|exec\.Command' internal/pipeline/steps/*_test.go
printf '%s\n' '--- shellenv API implementation ---'
cat -n internal/shellenv/shell_command.go | sed -n '1,75p'Repository: Thaloz/thz-no-mistakes
Length of output: 40225
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- fake CLI environment and process working directory ---'
sed -n '230,275p' internal/pipeline/steps/steps_test.go
sed -n '234,285p' internal/pipeline/steps/helpers_test.go
printf '%s\n' '--- provider and git execution call sites used by the two resets ---'
rg -n -C 12 'gh.*pr.*list|pr list|ls-remote|Run\(.*gh|CommandContext.*gh|CommandContext.*git|func .*Run' internal/pipeline/steps internal/scm internal/git
printf '%s\n' '--- configured-base reset test call graph ---'
rg -n -C 12 'FAKE_CLI_MODE.*git-reset-base|git-reset-base-on-ls-remote|RESET_BASE_SOURCE|RESET_BASE_BRANCH' internal/pipeline/stepsRepository: Thaloz/thz-no-mistakes
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("internal/pipeline/steps/steps_test.go")
lines = p.read_text().splitlines()
targets = [124, 197]
for n in targets:
line = lines[n - 1]
print(f"{p}:{n}: {line.strip()}")
print(" bare_force:", '"--force"' in line and "--force-with-lease" not in line)
print(" explicit_lease:", "--force-with-lease=" in line)
print(" conditional_update_ref:", "update-ref" in line)
print("reset destinations:")
for n in [123, 192]:
print(f"{p}:{n}: {lines[n - 1].strip()}")
print("expected-old-sha inputs:")
for i, line in enumerate(lines, 1):
if 116 <= i <= 205 and ("RESET" in line or "expected" in line.lower()):
print(f"{p}:{i}: {line.strip()}")
PYRepository: Thaloz/thz-no-mistakes
Length of output: 1231
Anchor the test-only forced reset.
Both reset helpers use a bare git push --force against inherited origin, with the destination branch taken from environment input. Use one reset helper with an expected old SHA and an explicit --force-with-lease=refs/heads/<branch>:<expectedSHA>. Validate the temporary remote and route the command through the context-aware shellenv helper. Add a regression case that advances the destination ref before reset; reset must fail and preserve the newer ref.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 123-123: An argument passed to exec.Command/exec.CommandContext is built by concatenating a string literal with dynamic input. If that input is attacker-controlled (and especially when the command is a shell such as sh -c/bash -c), this enables OS command injection. Pass untrusted data as separate, fixed arguments instead of interpolating it into a command string, avoid invoking a shell, and validate/escape the input where a shell is unavoidable.
Context: exec.Command("git", "push", "--force", "origin", resetSource+":refs/heads/"+resetBranch)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-concat-arg-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pipeline/steps/steps_test.go` around lines 122 - 129, Replace the
bare `git push --force` command in the resetSource branch with a safer
`--force-with-lease` operation that validates the expected SHA. Capture the
current SHA of the branch being reset before the push, then use
`--force-with-lease=refs/heads/<branch>:<expectedSHA>` to anchor the operation.
Instead of calling exec.Command directly, route the command through the
context-aware shellenv helper. Finally, add a regression test case that advances
the destination ref before attempting reset, verifying that the operation fails
and the newer ref is preserved when lease validation prevents the unsafe push.
Source: Coding guidelines
| jsonFields := []string{"number", "url"} | ||
| if strings.TrimSpace(base) == "" { | ||
| jsonFields = append(jsonFields, "baseRefName") | ||
| } | ||
| if h.forkOwner != "" { | ||
| jsonFields = "number,url,headRefName,headRepositoryOwner" | ||
| jsonFields = append(jsonFields, "headRefName", "headRepositoryOwner") | ||
| } | ||
| args = append(args, "--state", "open", "--json", jsonFields) | ||
| args = append(args, "--state", "open", "--json", strings.Join(jsonFields, ",")) | ||
| cmd := h.cmd(ctx, "gh", args...) | ||
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("gh pr list: %s: %w", strings.TrimSpace(string(out)), err) | ||
| } | ||
| if strings.TrimSpace(string(out)) == "" { | ||
| return nil, nil | ||
| } | ||
| var prs []struct { | ||
| Number int `json:"number"` | ||
| URL string `json:"url"` | ||
| BaseRefName string `json:"baseRefName"` | ||
| HeadRefName string `json:"headRefName"` | ||
| HeadRepositoryOwner *struct { | ||
| Login string `json:"login"` | ||
| } `json:"headRepositoryOwner"` | ||
| } | ||
| if err := json.Unmarshal(out, &prs); err != nil || len(prs) == 0 { | ||
| return nil, nil | ||
| if err := json.Unmarshal(out, &prs); err != nil { | ||
| return nil, fmt.Errorf("parse gh pr list response: %w", err) | ||
| } | ||
| if prs == nil { | ||
| return nil, errors.New("parse gh pr list response: expected a JSON array") | ||
| } | ||
| var matched *scm.PR | ||
| for _, candidate := range prs { | ||
| pr := &scm.PR{URL: strings.TrimSpace(candidate.URL), BaseBranch: strings.TrimSpace(candidate.BaseRefName)} | ||
| number, numberErr := scm.ExtractPRNumber(pr.URL) | ||
| if pr.URL == "" || numberErr != nil || candidate.Number <= 0 || number != fmt.Sprintf("%d", candidate.Number) { | ||
| return nil, errors.New("parse gh pr list response: invalid pull request") | ||
| } | ||
| pr.Number = number | ||
| if strings.TrimSpace(base) == "" && pr.BaseBranch == "" { | ||
| return nil, errors.New("parse gh pr list response: missing pull request base") | ||
| } | ||
| if pr.BaseBranch == "" { | ||
| pr.BaseBranch = strings.TrimSpace(base) | ||
| } | ||
| if h.forkOwner != "" && (strings.TrimSpace(candidate.HeadRefName) == "" || candidate.HeadRepositoryOwner == nil || strings.TrimSpace(candidate.HeadRepositoryOwner.Login) == "") { | ||
| return nil, errors.New("parse gh pr list response: invalid pull request head") | ||
| } | ||
| if !h.matchesHead(candidate.HeadRefName, candidate.HeadRepositoryOwner, branch) { | ||
| continue | ||
| } | ||
| pr := &scm.PR{URL: strings.TrimSpace(candidate.URL)} | ||
| if candidate.Number > 0 { | ||
| pr.Number = fmt.Sprintf("%d", candidate.Number) | ||
| } else if num, nerr := scm.ExtractPRNumber(pr.URL); nerr == nil { | ||
| pr.Number = num | ||
| } | ||
| if pr.URL == "" { | ||
| return nil, nil | ||
| if matched != nil { | ||
| return nil, errors.New("multiple open pull requests found for source branch") | ||
| } | ||
| return pr, nil | ||
| matched = pr | ||
| } | ||
| return nil, nil | ||
| return matched, nil |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For the GitHub CLI version supported by this repository, verify whether gh pr list --head performs an exact head match and excludes same-name or prefix-matching fork heads. Use official GitHub CLI documentation, source, and release notes.
💡 Result:
The GitHub CLI command gh pr list --head <branch> does not perform a guaranteed "exact match" in the way that excludes all other branches; it relies on GitHub's server-side search/query mechanisms, which can exhibit fuzzy or prefix-matching behavior [1]. Based on GitHub CLI documentation and repository discussions: 1. Server-Side Matching: The --head flag in gh pr list passes the branch name to the underlying GitHub API (either the repository.pullRequests GraphQL field or the search API, depending on other flags used) [1][2]. These server-side mechanisms are known to perform prefix matching for head branch names in certain contexts, rather than strict exact matching [1]. Users have reported that --head foo may return PRs where the head branch starts with "foo" [1]. 2. Lack of Explicit Exact Matching: There is no native flag or configuration in gh pr list to force an "exact match" that strictly excludes branches with the same name prefix [1]. Because this matching happens server-side, gh itself cannot override the API's behavior to enforce an exact constraint [1]. 3. Workarounds: To reliably filter for an exact match, users are advised to perform client-side filtering [1]. You can use the --json flag to output relevant fields (such as headRefName) and pipe the result to jq to perform a precise string comparison [1]. For example: gh pr list --head --json headRefName --jq 'map(select(.headRefName == ""))' 4. Syntax Limitations: The --head flag does not support the owner:branch qualified ref syntax (e.g., gh pr list --head user:branch) for cross-repository fork branch filtering [3][2]. Documentation explicitly notes that this syntax is not supported [3][4]. In summary, gh pr list --head does not perform an exact match, and it does not inherently exclude same-name prefix matches; client-side filtering with jq is the recommended approach to achieve exact matching [1].
Citations:
- 1: PR search by multiple exact branch names cli/cli#10816
- 2:
gh pr list: no support for--head <owner>:<branch>syntax cli/cli#10945 - 3: https://cli.github.com/manual/gh_pr_list
- 4: Add example usage of
--headoption topr listdocs cli/cli#10979
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- github.go symbols and surrounding code ---'
rg -n -A35 -B20 'func \(h \*Host\) FindPR|func \(h \*Host\) matchesHead|forkOwner|FindPR\(' internal/scm/github/github.go internal/scm/github/github_test.go
printf '%s\n' '--- PR reuse/update call sites ---'
rg -n -A25 -B15 'FindPR\(|UpdatePR\(' internal --glob '*.go'
printf '%s\n' '--- GitHub CLI version/config references ---'
rg -n -i 'gh version|github-cli|cli/cli|gh pr list' . --glob '!vendor/**' --glob '!node_modules/**'Repository: Thaloz/thz-no-mistakes
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all FindPR callers ---'
rg -n 'FindPR\(' internal --glob '*.go'
printf '%s\n' '--- BuildHost and GitHub host construction ---'
rg -n -A35 -B15 'func BuildHost|NewWithFork|github\.New|fork_url|ForkURL|Fork' internal/pipeline internal/steps internal/daemon internal/scm --glob '*.go'
printf '%s\n' '--- FindPR implementation and nearby tests ---'
sed -n '185,265p' internal/scm/github/github.go
sed -n '580,725p' internal/scm/github/github_test.goRepository: Thaloz/thz-no-mistakes
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- PR step ---'
sed -n '1,180p' internal/pipeline/steps/pr.go
printf '%s\n' '--- relevant manager continuity section ---'
sed -n '1035,1115p' internal/daemon/manager.go
printf '%s\n' '--- host construction tests and fork routing comments ---'
rg -n -A18 -B8 'BuildHost|NewWithFork|ForkURL' internal/pipeline/steps/*_test.go internal/pipeline/steps/*.goRepository: Thaloz/thz-no-mistakes
Length of output: 50378
IDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External
Reachability path
● Entry
internal/scm/host.go:176
Host: Available returns nil when the host is ready to use, or a descriptive
│
▼
● Sink
internal/scm/github/github.go
Validate the PR source before reuse.
When h.forkOwner == "", FindPR requests no source identity and matchesHead accepts every candidate. Because gh pr list --head <branch> is not an exact source selector, a colliding fork PR can be returned. PRStep can then edit that PR and persist its URL. Request and validate headRefName and the source repository against the expected source before reuse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scm/github/github.go` around lines 191 - 247, Update FindPR to
always request headRefName and headRepositoryOwner, including when h.forkOwner
is empty. Validate every candidate’s head branch and source repository against
the expected source before allowing reuse, and make matchesHead enforce that
repository identity rather than accepting all candidates for same-repository
searches. Preserve the existing invalid-response errors and unique-match
behavior.
Source: MCP tools
| for _, candidate := range mrs { | ||
| pr := candidate.toPR() | ||
| number, numberErr := scm.ExtractPRNumber(pr.URL) | ||
| if pr.URL == "" || numberErr != nil || (candidate.IID > 0 && number != fmt.Sprintf("%d", candidate.IID)) { | ||
| return nil, errors.New("parse glab mr list response: invalid merge request") | ||
| } | ||
| if strings.TrimSpace(base) == "" && pr.BaseBranch == "" { | ||
| return nil, errors.New("parse glab mr list response: missing merge request target branch") | ||
| } | ||
| if pr.BaseBranch == "" { | ||
| pr.BaseBranch = strings.TrimSpace(base) | ||
| } | ||
| if matched != nil { | ||
| return nil, errors.New("multiple open merge requests found for source branch") | ||
| } | ||
| matched = pr | ||
| } | ||
| return pr, nil | ||
| return matched, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject merge requests without an IID.
A response with web_url ending in a number but no iid passes the current condition. toPR then returns a pull request with an empty Number, so the response is accepted without a valid merge-request identity.
internal/scm/gitlab/gitlab.go#L206-L223: Requirecandidate.IID > 0and compare the URL number against the IID unconditionally.internal/scm/gitlab/gitlab_test.go#L314-L341: Add amissing IIDfixture with a valid numericweb_urland assert an error with a nil PR.
Proposed fix
- if pr.URL == "" || numberErr != nil || (candidate.IID > 0 && number != fmt.Sprintf("%d", candidate.IID)) {
+ if candidate.IID <= 0 || pr.URL == "" || numberErr != nil || number != fmt.Sprintf("%d", candidate.IID) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, candidate := range mrs { | |
| pr := candidate.toPR() | |
| number, numberErr := scm.ExtractPRNumber(pr.URL) | |
| if pr.URL == "" || numberErr != nil || (candidate.IID > 0 && number != fmt.Sprintf("%d", candidate.IID)) { | |
| return nil, errors.New("parse glab mr list response: invalid merge request") | |
| } | |
| if strings.TrimSpace(base) == "" && pr.BaseBranch == "" { | |
| return nil, errors.New("parse glab mr list response: missing merge request target branch") | |
| } | |
| if pr.BaseBranch == "" { | |
| pr.BaseBranch = strings.TrimSpace(base) | |
| } | |
| if matched != nil { | |
| return nil, errors.New("multiple open merge requests found for source branch") | |
| } | |
| matched = pr | |
| } | |
| return pr, nil | |
| return matched, nil | |
| for _, candidate := range mrs { | |
| pr := candidate.toPR() | |
| number, numberErr := scm.ExtractPRNumber(pr.URL) | |
| if candidate.IID <= 0 || pr.URL == "" || numberErr != nil || number != fmt.Sprintf("%d", candidate.IID) { | |
| return nil, errors.New("parse glab mr list response: invalid merge request") | |
| } | |
| if strings.TrimSpace(base) == "" && pr.BaseBranch == "" { | |
| return nil, errors.New("parse glab mr list response: missing merge request target branch") | |
| } | |
| if pr.BaseBranch == "" { | |
| pr.BaseBranch = strings.TrimSpace(base) | |
| } | |
| if matched != nil { | |
| return nil, errors.New("multiple open merge requests found for source branch") | |
| } | |
| matched = pr | |
| } | |
| return matched, nil |
📍 Affects 2 files
internal/scm/gitlab/gitlab.go#L206-L223(this comment)internal/scm/gitlab/gitlab_test.go#L314-L341
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scm/gitlab/gitlab.go` around lines 206 - 223, Require candidate.IID
> 0 in the merge-request validation loop in internal/scm/gitlab/gitlab.go (lines
206-223), and compare the extracted URL number against the IID unconditionally.
In internal/scm/gitlab/gitlab_test.go (lines 314-341), add a missing-IID fixture
with a numeric web_url and assert that parsing returns an error and nil PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/scm/gitlab/gitlab.go`:
- Around line 67-78: Update NewForRemote to reject the "http" scheme before
assigning h.host or h.repository, while continuing to accept HTTPS and leaving
HTTP unchanged rather than rewriting it. Adjust
TestCustomRemoteAuthorityScopesAllOperations to assert that HTTP remotes are
rejected before authenticated glab operations can use them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 943b55e7-3e24-4cef-8052-5b2b528b1374
📒 Files selected for processing (14)
AGENTS.mddocs/src/content/docs/reference/pipeline-steps.mddocs/src/content/docs/reference/repo-config.mdinternal/cli/attach.gointernal/cli/root_test.gointernal/cli/wizard.gointernal/pipeline/steps/ci.gointernal/pipeline/steps/ci_fix.gointernal/pipeline/steps/ci_test.gointernal/pipeline/steps/common_git.gointernal/pipeline/steps/host.gointernal/pipeline/steps/steps_test.gointernal/scm/gitlab/gitlab.gointernal/scm/gitlab/gitlab_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/pipeline/steps/host.go
- internal/pipeline/steps/ci.go
- internal/cli/wizard.go
- internal/pipeline/steps/common_git.go
- AGENTS.md
- docs/src/content/docs/reference/pipeline-steps.md
- docs/src/content/docs/reference/repo-config.md
| scheme := strings.ToLower(strings.TrimSpace(parsed.Scheme)) | ||
| if scheme != "http" && scheme != "https" { | ||
| return h | ||
| } | ||
| authority := strings.TrimSpace(parsed.Host) | ||
| if authority == "" { | ||
| return h | ||
| } | ||
| h.host = authority | ||
| if h.projectPath != "" { | ||
| h.repository = scheme + "://" + authority + "/" + h.projectPath | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- internal/scm/gitlab/gitlab.go ---'
sed -n '1,125p' internal/scm/gitlab/gitlab.go
printf '%s\n' '--- relevant tests ---'
sed -n '390,435p' internal/scm/gitlab/gitlab_test.go
printf '%s\n' '--- command construction ---'
rg -n -C 3 'h\.repository|h\.host|repoArgs|glab api|glab auth|NewForRemote' internal/scm/gitlab/gitlab.go internal/scm/gitlab/gitlab_test.go
printf '%s\n' '--- glab references and version metadata ---'
rg -n -i 'glab|gitlab cli|GITLAB_TOKEN' --glob '!internal/scm/gitlab/gitlab.go' --glob '!internal/scm/gitlab/gitlab_test.go' . | head -120Repository: Thaloz/thz-no-mistakes
Length of output: 40343
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,125p' internal/scm/gitlab/gitlab.go
sed -n '390,435p' internal/scm/gitlab/gitlab_test.go
rg -n -C 3 'h\.repository|h\.host|repoArgs|glab api|glab auth|NewForRemote' internal/scm/gitlab/gitlab.go internal/scm/gitlab/gitlab_test.goRepository: Thaloz/thz-no-mistakes
Length of output: 25348
🌐 Web query:
GitLab glab CLI HTTP API URL GITLAB_TOKEN does glab send credentials over http
💡 Result:
Yes, the glab CLI sends your credentials—including those provided via the GITLAB_TOKEN environment variable—over HTTP when communicating with the GitLab API [1][2]. When you use glab, it makes authenticated HTTP requests to the GitLab API [1]. If you set the GITLAB_TOKEN environment variable, glab uses this token as the authentication credential for these requests [3][2]. Specifically, the token is included in the HTTP request headers (typically as a Private-Token or Authorization header) to authenticate the session [2]. Important security considerations: 1. Transport Security: Because these credentials are sent over the network, it is critical that you connect to GitLab instances over HTTPS to ensure the communication is encrypted. 2. Debugging Exposure: If you enable HTTP debugging by setting the GLAB_DEBUG_HTTP environment variable to true, the CLI will print request and response information to your terminal [3][2]. While this is useful for troubleshooting, it may expose your authentication token in plain text in your terminal logs or console output [2]. To avoid storing tokens in environment variables (which can be logged or exposed to other processes), the recommended best practice is to use glab auth login [4][5]. This command securely stores your credentials in your operating system's native keyring (such as the macOS Keychain, Windows Credential Manager, or Linux Secret Service) [4].
Citations:
- 1: https://docs.gitlab.com/cli/api/
- 2: https://gitlab.com/gitlab-org/cli/-/issues/7523
- 3: https://docs.gitlab.com/cli/
- 4: https://docs.gitlab.com/cli/auth/login/
- 5: https://gitlab.com/gitlab-org/cli/-/blob/main/README.md
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
src = Path("internal/scm/gitlab/gitlab.go").read_text()
test = Path("internal/scm/gitlab/gitlab_test.go").read_text()
assert 'if scheme != "http" && scheme != "https"' in src
assert 'h.repository = scheme + "://" + authority + "/" + h.projectPath' in src
assert 'return []string{"--repo", h.repository}' in src
assert 'const repo = "http://gitlab.example:8443/group/project"' in test
assert '"glab mr list --source-branch feature --target-branch main --repo " + repo' in test
assert '"glab mr view 42 --repo " + repo' in test
print("HTTP is accepted, preserved in h.repository, and passed to authenticated glab porcelain commands via --repo.")
PYRepository: Thaloz/thz-no-mistakes
Length of output: 269
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
internal/cli/root_test.go:21
TestRootReattachesActiveRunWhenUpstreamIsUnavailable
│
▼
● Sink
internal/scm/gitlab/gitlab.go
Reject HTTP remotes before invoking authenticated glab commands.
NewForRemote accepts http:// remotes and passes them to authenticated MR/CI commands through --repo. glab sends credentials over that connection, so an on-path attacker can read or modify requests. Reject HTTP before setting h.repository or h.host, and update TestCustomRemoteAuthorityScopesAllOperations to assert rejection. Do not rewrite HTTP to HTTPS.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scm/gitlab/gitlab.go` around lines 67 - 78, Update NewForRemote to
reject the "http" scheme before assigning h.host or h.repository, while
continuing to accept HTTPS and leaving HTTP unchanged rather than rewriting it.
Adjust TestCustomRemoteAuthorityScopesAllOperations to assert that HTTP remotes
are rejected before authenticated glab operations can use them.
1 participant
Intent
Add a backward-compatible trusted repository setting such as pr.base_branch so feature PRs and all base-dependent pipeline operations can target an integration branch like quality-assurance while unset repositories continue using the GitHub default branch. Keep the actual default branch as the security trust root, strictly validate configured targets and shared history, protect both default and configured base branches from direct runs, preserve fork delivery, and use immutable per-run target snapshots where appropriate. Ensure reruns and recovery preserve existing-PR continuity across crashes, legacy records, configuration changes, forge outages, and open/closed/reopened PR states without retargeting or duplicate PR creation. Cover every affected fresh-run, rerun, recovery, Intent, Rebase, CI, PR, identity, cleanup, and failure path with focused tests, and update configuration, setup, troubleshooting, CLI, and release documentation. Keep the change narrowly scoped and validate the exact current pipeline head without weakening checks; additional security scanning can be pursued separately after this capability is working.
What Changed
pr.base_branchconfiguration so PRs and base-dependent pipeline steps can target integration branches while preserving the default branch as the trust root.Risk Assessment
Testing
After baseline repository inspection and provisioning a transient worktree-local Go 1.25 toolchain because Go was absent, I exercised trusted configuration, protected branches, immutable base snapshots, fork/provider routing, delivery revalidation, CI monitoring, persisted PR continuity, reopened/legacy recovery, and approval boundaries; all focused checks passed and the transient toolchain was removed. No screenshot was applicable because this is non-UI pipeline behavior; the reviewer-visible artifact is the simulated provider CLI transcript.
Evidence: Configured integration-branch PR routing transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (15) ✅
internal/daemon/manager.go:958- Fresh replacement runs preserve continuity from only the newest prior run on the branch. If an older QA-targeted PR is closed, a newer staging-based run is recorded, and then the QA PR is reopened, the next replacement examines only the staging record and can create/update a staging PR while the reopened QA PR remains open. Enumerate distinct persisted base candidates for the branch, verify them authoritatively, preserve the sole open PR, and fail closed if multiple bases are open.internal/daemon/manager.go:881- The continuity check treats(nil, nil)fromFindPRas authoritative absence, but GitHub and GitLab currently return(nil, nil)when their successful CLI output is malformed or contains unparseable chatter. After a crash before PR URL persistence, that path drops the saved base and can create a duplicate against the new configured base. Make providerFindPRparsers return errors for malformed/non-empty responses or invalid PR shapes so continuity fails closed on unresolved forge output.🔧 Fix: Preserve PR continuity and reject malformed forge responses
2 errors still open:
internal/daemon/manager.go:884- A persistedmergedstate skips the authoritative head/base lookup. If an older closed PR for the same head/base is reopened after a newer PR was merged, base deduplication selects the newer run, skipsFindPR, and can create a PR against the newly configured base while the reopened PR remains open. Always query each distinct persisted head/base tuple; only use merged state after that lookup proves no open PR exists.internal/daemon/manager.go:841- Legacy runs with a null PR-base snapshot are assigned the repository's current default branch. After an open legacy PR targetingmainexists andinitrefreshes the default branch totrunk, continuity treats that PR as targetingtrunk; because this equals the current base, verification is skipped and a duplicatetrunkPR can be created. Resolve legacy open PRs by head without a synthesized base, obtain their authoritative target branch through the provider boundary, and persist it before continuity selection.🔧 Fix: Harden reopened and legacy PR continuity
1 error still open:
internal/daemon/manager.go:998- Verified-base deduplication keeps the first candidate'sexplicitflag. If a user selects an older legacy null-base run, its base-less lookup can discover the same open staging PR recorded by a newer explicit run; the newer candidate is then discarded, so the replacement persistsexplicit=falseand skips immutable snapshot/shared-history validation for staging after a config change. When verified candidates converge on one base, preserveexplicit=trueif any candidate carries it.🔧 Fix: Preserve explicit semantics across converged PR bases
2 errors still open:
internal/daemon/manager.go:860- Branch-level candidate deduplication discards the duplicate candidate'sexplicitflag. If an older run targetedmainimplicitly,pr.base_branch: mainwas later configured explicitly, and a rerun selects the older run, the implicit preferred candidate wins and the replacement bypasses immutable snapshot/shared-history validation. Merge duplicate metadata soexplicitremains true when any persisted candidate for that branch is explicit.internal/daemon/manager.go:1054- PR continuity evidence is snapshotted beforecancelActiveRunswaits for the existing executor. That executor can start or create its old-base PR after this query; following a configuration change, the replacement never verifies that base and can create a second PR against the new base. Cancel and await the prior run before loading runs and started-PR evidence.🔧 Fix: Preserve cancellation-time PR continuity and explicit base semantics
1 error still open:
internal/daemon/manager.go:1227- An open historical PR whose base was implicit can disable the current explicit configuration when both resolve to the same branch. The continuity candidate'sexplicit=falseoverwrites the currenttrue;ensureConfiguredPRBaseBranchthen clearsResolvedBaseSHA, so this and future replacements use a mutable base without explicit-target shared-history validation. When continuity selects the currently configured branch, preserveexplicit=truefrom the current config before applying the candidate metadata.🔧 Fix: Preserve explicit PR-base semantics across implicit continuity
1 error still open:
internal/daemon/manager.go:817- Whenpr.base_branchis unset, the guard returns before checking the repository default branch. A directgit push gate mainbypasses CLI preflight, starts a default-branch pipeline, and lets PushStep publish pipeline changes to the security trust root. Always rejectbranch == defaultBranchbefore conditionally checking an explicit configured PR base.🔧 Fix: Reject default-branch runs without explicit PR configuration
4 errors still open:
internal/daemon/manager.go:891- Every started PR step is treated as continuity evidence, including steps that completed as skipped before any Find/Create call (for example, an unavailable CLI or unsupported routing). The next replacement then requires the same unavailable host and fails before running, although the completed skip proves no PR was created. Exclude completed skipped PR steps while retaining running/failed ambiguity and persisted PR evidence.internal/daemon/manager.go:239- Recovery validates the newly configured PR base against the old run's HEAD before consulting its persisted immutable base snapshot. If a parked QA-targeted run is recovered after trusted config changes to a valid staging branch with unrelated history, recovery fails even though the preserved QA snapshot remains usable. Guard the current branch name, but resolve the persisted snapshot first; validate the current target only when it supplies a missing same-base snapshot.internal/scm/azuredevops/azuredevops.go:141- Azure FindPR still treats JSONnullas authoritative absence and accepts invalid entries such as[{}]when a base is supplied. During crash recovery before PR URL persistence, this can discard the saved base and permit a duplicate PR against the new configuration. Reject null arrays and PR entries without a valid ID/browsable URL.internal/bitbucket/client.go:136- Bitbucket FindPR treats malformed successful responses such as{}ornullas an emptyvalueslist, and a base-filtered[{}]becomes a PR with no usable identity. Continuity then interprets the unresolved response as no open PR and can create a duplicate against a changed base. Require the expected response shape and validate the returned PR identity before reporting absence or a match.🔧 Fix: Harden PR recovery evidence and provider parsing
1 error still open:
internal/daemon/manager.go:242- Startup recovery still assigns today’s repository default to every legacy null-base run. If a legacy parked CI gate has an open PR targetingmainand the default is renamed totrunk, recovery resumes withtrunkas the pipeline base, so a subsequent CI repair can rebase againsttrunkwhile the PR remains based onmain. For null-base runs with PR evidence, resolve the open PR by head without a base and persist its provider-reported base before falling back to the current default; fail closed on unavailable or ambiguous results.🔧 Fix: Resolve legacy recovery PR bases authoritatively
1 error still open:
internal/daemon/manager.go:962- PR continuity executes provider commands from the bare gate repository. GitLab's FindPR/GetPRState commands do not specify--repoand depend on repository discovery from their working directory, which fails whensafe.bareRepository=explicitis enabled. Consequently, GitLab reruns and legacy recovery with PR evidence fail before continuity can be verified. Pass the available run worktree into this boundary or explicitly scope GitLab commands to the project.🔧 Fix: Scope GitLab PR continuity to explicit repositories
2 errors still open:
internal/daemon/manager.go:1243- A replacement run validates the newly configured PR target before checking persisted open-PR continuity. If an open QA-targeted PR exists and trusted config changes to a missing or history-incompatible staging target, rerun fails here instead of preserving QA, despite staging never becoming the effective target. Guard protected branch names first, verify continuity, then resolve only the selected effective base.internal/daemon/manager.go:1272- When continuity preserves a formerly implicit default branch that differs from today’s default/configured target,explicit=falsemakes this resolution a no-op. The Intent step runs before Rebase, and a supported--skip=rebaserun leaves every later diff owner using a staleorigin/<old-base>ref; after that target is rewritten, commits included in the live PR can be excluded from review. Freshly fetch and resolve an inherited implicit base at the continuity boundary; apply the same rule to the early return in recovery at line 259.🔧 Fix: Preserve continuity before resolving inherited PR bases
3 errors still open:
internal/daemon/manager.go:265- Recovery skips refreshing a persisted implicit base when its name matches today’s explicitpr.base_branch. Only the default branch was fetched earlier, so a parked run targeting QA can resume against a staleorigin/QA, bypassing the current explicit snapshot/shared-history guarantees. Preserve current explicit semantics and resolve the target before returning, as the replacement path does.internal/daemon/manager.go:818- The inherited-base resolver proves only that the fetched ref is a commit, not that it shares history with HEAD. If an old implicit PR base is rewritten to unrelated history and Rebase is skipped, branch-delta helpers fall back toRun.BaseSHAand can exclude changes still represented by the PR. Require a usable merge base at this continuity boundary so the run fails closed.internal/bitbucket/client.go:134- The hardened Bitbucket parser still accepts malformed bodies with trailing JSON or garbage becausedoJSONdecodes only the first value. A response such as{"values":[]} {"values":[...]}is treated as authoritative absence, allowing continuity to create a duplicate PR. Make the shared JSON decoder require EOF after one top-level value.🔧 Fix: Harden recovered PR bases and Bitbucket parsing
3 errors still open:
internal/pipeline/steps/ci.go:233- Moving-base validation is skipped entirely whenci_timeoutis unlimited. If the configured base snapshot is A and the base is later reset to an ancestor P, the PR widens to include P..A, yet the monitor can report checks passed without refreshing or validating the target. Validate explicit bases on every poll; keep only timeout re-arming conditional on a finite timeout.internal/pipeline/steps/common_git.go:121- Finite-timeout refreshes require only some shared history between HEAD and the moving base. Resetting the base from snapshot A to an ancestor or sibling of A still passes, while the live PR includes commits excluded from the A..HEAD review. Require the immutable run snapshot to be an ancestor of the refreshed base before monitoring or conflict repair proceeds.internal/daemon/manager.go:931- Same-base candidate deduplication discards every duplicate run's persisted PR URL/state. If an older URL-less run is selected, a newer same-base run records a PR, and that PR is manually retargeted while remaining open, the base-filtered lookup returns nil and the retained candidate has no URL to verify directly; continuity can then create another PR while the retargeted one remains open. Preserve and verify all distinct persisted PR identities when consolidating a head/base tuple.🔧 Fix: Harden moving-base validation and persisted PR continuity
3 errors still open:
internal/pipeline/steps/pr.go:87- The immutable-base ancestry check runs only during CI polling. If the configured base snapshot is A, review completes, and the target is reset to ancestor P before delivery, Push publishes H and PR discovery/creation targets the mutable branch without validation; with CI skipped, the run completes while the live PR includes unreviewed P..A commits. Validate the explicit base immediately before Push, and again before PR delivery when Push is skipped.internal/pipeline/steps/ci.go:112- Parked CI reconciliation accepts an open PR without revalidating its explicit base. A target can pass polling, reset behind the immutable snapshot while the timeout gate is parked, and then be approved with the widened PR unverified. Refresh and enforce snapshot ancestry for open PRs before allowing the parked approval to proceed.internal/pipeline/steps/ci.go:245- Moving-base validation occurs before terminal PR-state observation. If a PR is merged or closed and its integration branch is then deleted or reset behind the snapshot before the next poll, CI returns the base error without observing the terminal lifecycle state and can fail an otherwise terminal run. Check PR state first and validate the base only while the PR remains open.🔧 Fix: Revalidate PR bases at delivery and approval boundaries
3 issues (2 errors, 1 warning) still open:
internal/pipeline/steps/pr.go:78- PR-base validation occurs before agent-generated PR content. If snapshot A validates, content generation runs, and the target resets behind A before Find/Create, the step can create a widened PR containing unreviewed commits. Revalidate after content generation at the provider-delivery boundary, including immediately before CreatePR.internal/pipeline/steps/ci_fix.go:28- CI repair validates the base before fetching logs and running the fix agent, but pushUpdatedHeadSHA performs no final validation. A target reset behind the snapshot during that work therefore allows the repair push to update the live PR with unreviewed commits. Revalidate in pushUpdatedHeadSHA so every CI repair push shares the pre-push invariant.internal/pipeline/executor.go:1145- Any approval-validation error, including a transient forge or Git outage, consumes the approval and permanently fails the run. Preserve or retry the parked gate for non-fatal validation errors, while continuing to fail on ErrFatalGateReconciliation invariant violations.🔧 Fix: Close PR delivery, CI repair, and approval races
2 errors still open:
internal/pipeline/steps/push.go:97- Explicit-base validation occurs beforeresolveForcePushDecision, which may perform remote reads and fetch/patch-id work. If an existing QA-targeted PR is open and QA resets behind the immutable snapshot during that work,PushCommitstill publishes the head and widens the live PR before the later PR step can reject it. Revalidate after the decision, immediately before each mutatingPushCommit, as the CI-repair push path does.internal/pipeline/steps/ci.go:116- Every explicit-base validation error is wrapped asErrFatalGateReconciliation, including transient fetch or Git transport outages. A routine parked-gate reconciliation during such an outage therefore permanently fails the run instead of preserving the gate. Distinguish snapshot/shared-history invariant violations from transient resolution errors and mark only the former fatal.🔧 Fix: Revalidate pushes and preserve transient CI approvals
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
git status --short, commit inspection, and targeted diff review against6ed880de1b4ac9d28d706ae20e767e5729024891go test ./internal/config -run '^(TestLoadRepoConfig_PRBaseBranch|TestLoadRepoConfig_PRBaseBranchUnsetPreservesDefaultBehavior|TestLoadRepoConfig_PRBaseBranchRejectsMalformedExplicitValues|TestLoadRepoConfig_PRBaseBranchRejectsUnsafeNames|TestEffectiveRepoConfig_PRBaseBranchTrustedOnly)$' -count=1go test ./internal/cli -run '<configured-base preflight, trusted-config, wizard branch-protection selectors>' -count=1go test ./internal/pipeline/steps -run '<configured PR base, Rebase, Push, PR, Intent, and CI selectors>' -count=1go test ./internal/daemon -run '<PR-base protection, continuity, rerun, replacement, and recovery selectors>' -count=1go test ./internal/scm/github ./internal/scm/gitlab ./internal/scm/azuredevops ./internal/bitbucket -run '<base-aware PR discovery and identity selectors>' -count=1go test ./internal/db ./internal/pipeline ./internal/gate -run '<PR persistence, approval reconciliation, and remote identity selectors>' -count=1go test -v ./internal/pipeline/steps -run '^TestResolveBranchBaseSHA_UsesConfiguredPRBase$/^pr_summary_and_routing$' -count=1with the provider transcript captured as evidenceFocused retries ofTestRerunContinuityRecognizesReopenedPRandTestLoadRecoveredConfig_ResolvesLegacyOpenPRBaseAfterDefaultRenameafter fixes✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation