[marcusrbrown]

Fixes #1517.

The withheld-run preflight now reads effective Git configuration, including includeIf, worktree, global, and system scopes, and detects bare as well as URL-scoped HTTP extraheaders. Name-only queries and static diagnostics avoid logging credential values or raw probe errors.

Verification failures deny with accurate repair guidance and numeric exit codes. Canonical non-repository errors tolerate preceding warnings but reject misleading substrings and ambiguous fatal lines. Missing Git and genuinely non-repository workspaces remain warned compatibility exceptions. Nested-checkout discovery and other credential mechanisms remain outside this check.

CI disables checkout credential persistence for pull_request only, with a regression test tied to the actual runtime credential policy. Bootstrap tests separately verify preflight wiring without depending on ambient checkout credentials.

Release policy

This action remains in the unstable 0.x series. Release-producing breaking changes advance the minor version (0.110.x → 0.111.0), while explicit skip/build(dev) suppressions remain intact. Production commit analysis and the release preview agree, and BREAKING CHANGES notes remain present. Tests invoke the installed analyzer and notes generator directly; no release was triggered.

Verification

46 preflight tests including 19 isolated real-Git cases; 15 bootstrap tests; 25 workflow tests; 54 release-policy/preview tests. Full workspace and script suites, typecheck, lint, and build pass. dist/main.js is rebuilt.

BREAKING CHANGE: Credential-withheld runs now reject HTTP extraheaders from all effective Git configuration scopes and deny when verification cannot complete. Reused runners may need persisted headers removed or Git configuration/repository errors repaired. No override input is added.

Downstream migration

Tracked in #1598.

[fro-bot]

Verdict: PASS

The core change is sound and strictly strengthens the preflight. Dropping --local in favor of the unscoped --includes read closes the includeIf vector, the (.*\.)? subsection group closes the bare http.extraheader vector, and converting the catch-alls from fail-open ok() to fail-closed err() is the right direction for a check whose entire purpose is proving a credential is absent.

Two design details deserve credit because they carry the safety argument:

• Stage ordering. Running the config query before rev-parse means a global/system header is caught even when the workspace is not a repository, so the non-repo exemption at src/services/setup/git-credential-check.ts:117 cannot be used to skip the config check.
• rev-parse as the backstop. If Git refuses to read local config (dubious ownership, unreadable .git), stage 1 can return exit 1 with empty stdout — which looks like "no match". Stage 2 then fails with a non-canonical 128 and denies. The two stages together make that hole non-exploitable. That is not obvious from reading stage 1 alone and is worth keeping in the file comments.

Verification I ran (not a suite run): bunx vitest run src/services/setup/git-credential-check.integration.test.ts — 18/18 pass on this runner, confirming the real-git exit-code assumptions (config exit 1 on no-match, remote get-url exit 2 on missing origin, 128 + canonical stderr on non-repo) hold against the installed Git. I also ran git config --includes --name-only --get-regexp '^http\.(.*\.)?extraheader$' in this checkout: exit 1, confirming GitHub-hosted runner images carry no global/system extraheader and the widened scope will not false-positive on hosted infra. dist/main.js contains the new strings, so it is genuinely rebuilt.

Blocking issues

None.

Non-blocking concerns

1. Remediation text is wrong for the three "verification failed" paths. ERR_CONFIG_VERIFICATION_FAILED, ERR_REPO_CONTEXT_VERIFICATION_FAILED, and ERR_ORIGIN_VERIFICATION_FAILED (git-credential-check.ts:18-32) all end with "set persist-credentials: false on actions/checkout". But these fire for malformed config, dubious ownership, invalid GIT_DIR, and (on Git < 2.7) a missing remote get-url subcommand — none of which that instruction fixes. Paired with warnings that deliberately carry no exit code and no stderr, an operator gets a confident-sounding but wrong instruction and zero diagnostics. Suggest including the non-sensitive exitCode in the warning (git config parse errors report file and line, never values) and rewording these three to "verification could not complete" rather than implying a persisted credential.

2. startsWith on rev-parse stderr is positional. git-credential-check.ts:117 requires the canonical fatal to be the first bytes of stderr. Any preceding line — a warning: from global config, for example — shifts it and converts a genuinely non-repository workspace into a hard denial. LC_ALL=C fixes the wording but not the position. .includes(NOT_A_REPOSITORY_STDERR_PREFIX) would be equally strict against the impostor cases the tests already cover (fatal: not a git repository: /bad/gitdir, dubious ownership) while surviving a prefixed stderr.

3. Global/system widening has no escape hatch. On a reused self-hosted runner, a corporate-proxy http.extraheader in global or system scope now hard-fails every withheld run, permanently, with no input to override. The wiki change documents the consequence honestly, which is good. Since this ships in a released action consumed outside this repo, consider whether an explicit, audited override input is needed, or at minimum call this out as a breaking-behavior note in the release notes.

4. ci.yaml:204 couples to classifyEvent implicitly. persist-credentials: ${{ github.event_name != 'pull_request' }} is correct today because resolveResponseDelivery (packages/runtime/src/agent/response-delivery.ts:30-44) classifies only pull_request/issue_comment/issues as withheld, and pull_request is this workflow's only withheld trigger. If merge_group or push ever joins that set, this expression goes stale and CI breaks at runtime with the new fail-closed denial. A comment pointing at classifyEvent would make the coupling discoverable.

5. hasEmbeddedCredential port false-positive (pre-existing, unchanged). git-credential-check.ts:163-171 takes the first @ in the string; for https://host:8443/owner/re@po the "credential section" becomes host:8443/owner/re, which contains : and denies. Fail-closed and vanishingly unlikely for an origin URL — noting only so it is a known property rather than a surprise.

Missing tests

• Non-Error throw. isMissingGitError returns false for a thrown string or object, so the run denies. Every throw-path test (git-credential-check.test.ts:131, :219, :303) throws an Error. One case throwing a non-Error would pin the fail-closed behavior of that branch.
• Prefixed rev-parse stderr. No case covers the canonical fatal preceded by another stderr line (concern #2). A unit case with stderr: 'warning: something\nfatal: not a git repository (or any of the parent directories): .git' would make the positional assumption explicit — either as a deliberately-asserted deny, or as the trigger to switch to .includes.
• Documented out-of-scope boundary. Nested/submodule checkouts are declared out of scope in the doc comment but have no test. A negative integration case (sub-repo with a header, parent clean → allows) would pin the boundary so a future reader does not assume coverage that is not there.
• scripts/fro-bot-workflow.test.ts:697-703 is tautological. persists is derived from excludedEvent, and the expectation re-derives the same value from the literal 'pull_request', so the loop cannot fail independently of the assertion three lines above it. It also does not assert that the excluded set matches classifyEvent's affected triggers, which is the invariant that actually matters (concern #4). Importing the withheld-trigger list, or asserting against resolveResponseDelivery('pull_request', 'github').credential === 'withhold', would give the test real drift-detection power.

Risk assessment (MED): likelihood of regression, security exposure, or blast radius

Security exposure moves strictly downward — every change either widens detection or converts fail-open to fail-closed, and no previously-denied input becomes allowed. The residual fail-open (ENOENT/missing git) requires pre-action control of PATH, which implies workflow-write access and therefore more capability than this check defends against.

The risk is availability, not exposure, and the blast radius is wide: this runs in runBootstrap before anything else, in a released action, gating every pull_request, issue_comment, and issues run for every consumer. Inputs that previously passed silently — a malformed include, a dubious-ownership workspace, an unreadable GIT_DIR, a host-level proxy header — now hard-fail with core.setFailed. That is the intended contract, and it is the right contract, but combined with concern #1 the failures will be hard for a consumer to self-diagnose. Rated MED rather than LOW for that reason, not because the change is unsafe.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	34552874537
Cache	hit
Session	ses_f71cc8e29ffewh6mCahVk76AHf

[fro-bot]

Verdict: PASS

Re-review of 2512533 against my prior (dismissed) review. Every item I raised is either fixed or deliberately and defensibly declined, and two of the fixes are better than what I suggested.

Resolved:

• Remediation text (was concern #1). The three verification-failure constants no longer tell operators to set persist-credentials: false for problems that instruction cannot fix (git-credential-check.ts:18-32). Each now names the actual failure class. The {exitCode} diagnostic is attached at :115, :141, :166. Critically, the tests pin this as a contract rather than trusting the wording: git-credential-check.test.ts:188-190 asserts the config-failure error does not contain persist-credentials: false and does not contain "found", so a future edit cannot silently reintroduce the misdiagnosis. And toHaveBeenCalledWith(expect.any(String), {exitCode: 128}) is exact-match on the metadata object, which pins that the warning carries the exit code and nothing else — no stderr leak. That is the right assertion shape for a diagnostics-vs-secrecy tradeoff.

• Positional stderr match (was concern #2). isCanonicalNonRepositoryStderr (:41-44) is stricter than the .includes I proposed. Splitting on /\r\n|\n/, filtering to lines that start with fatal:, and requiring exactly one such line rejects three distinct impostor classes: an inline mention of the canonical phrase inside a non-fatal line, a different real fatal preceded by that inline mention, and two genuine fatal lines where only one is canonical. Requiring a single fatal line is the part I did not think of — ambiguity fails closed instead of the check picking a winner. All five cases are tested (:261-352), including CRLF.

• Workflow test tautology (was missing test #4). scripts/fro-bot-workflow.test.ts now derives the trigger set from the workflow's own on: mapping via declaredTriggers() and compares the YAML persist decision against resolveResponseDelivery(...).credential for each. Both oracles are now real and independent, so if merge_group or push ever joins classifyEvent's affected set, this test fails instead of CI breaking at runtime. The ci.yaml:204 comment pointing at classifyEvent closes concern #4's discoverability half.

• Missing tests #1-#3. Non-Error throws are covered twice, including a plain object that mimics the missing-git shape with both message and code: 'ENOENT' (:161-174) — that one matters, because it pins that the fail-open exemption requires a real Error instance and cannot be spoofed by shape. The nested-checkout boundary test (git-credential-check.integration.test.ts:510-543) is a proper positive/negative pair: parent allows, child independently denies. That proves the limitation is a scope boundary rather than a broken check, which a one-sided test would not have shown.

Verification I ran (one named file, not a suite): bunx vitest run scripts/fro-bot-workflow.test.ts — 25/25 pass. This tested a specific new risk, not general health: the test now imports ../packages/runtime/src/agent/response-delivery.js across the package boundary from scripts/, and a resolution failure there would break the whole test:scripts job. It resolves, and the policy cross-check holds for all four triggers ci.yaml declares (merge_group, pull_request, push, workflow_dispatch). I also grepped dist/main.js for the three new error strings (all present) and the \r\n|\n split plus the fatal: line filter (present, bundler-renamed), confirming dist carries the new logic and not just the new strings.

Blocking issues

None.

Non-blocking concerns

1. A behavior break is shipping as a patch release. .releaserc.yaml uses the conventionalcommits preset with no rule promoting fix, so fix(setup): ... produces a patch. The PR's own Compatibility section states that "withheld runs that previously passed may now fail" — global/system headers, malformed config, dubious ownership, and invalid GIT_DIR now hard-fail runBootstrap for every consumer on pull_request/issue_comment/issues. Consumers tracking a major tag or auto-merging patches pick this up with no signal beyond a "Bug Fixes" changelog line. The Compatibility text lives in the PR description, which only reaches consumers through the fail-soft, model-generated narration flow. Recommend adding a BREAKING CHANGE: footer to the squash-merge commit body so the release is versioned to match its actual blast radius. This is the maintainer's call at merge time, not a code defect, hence non-blocking.

2. No escape hatch for host-level headers (was concern #3, declined). The PR explicitly states "No override input is added," and the wiki documents the reused-runner consequence. I accept the reasoning — an override input on a fail-closed security preflight is itself an attack surface, and the correct fix for a self-hosted runner is to remove the header from the scope that carries it. Restating only so the decision is on the record rather than an oversight. It reinforces concern #1: a consumer hitting this has no workaround, which makes correct release signaling matter more.

3. hasEmbeddedCredential port false-positive (pre-existing, unchanged). git-credential-check.ts:184-191 takes the first @; for https://host:8443/owner/re@po the credential section becomes host:8443/owner/re, which contains : and denies. Fail-closed and implausible for an origin URL. Noted so it is a known property, not a future surprise.

4. Indented fatal lines fall through to deny. isCanonicalNonRepositoryStderr requires line.startsWith('fatal:') with no trim, so a leading-whitespace fatal yields zero matches and denies. This is the safe direction and I am not aware of Git indenting that message, so no change requested — recording it as a known property of the parser.

Missing tests

None. All four gaps from my prior review are closed, and the new cases test behavior contracts (error wording must not say X; warning metadata must be exactly {exitCode}) rather than restating the implementation. Coverage is now 46 preflight cases including 19 real-Git ones.

Risk assessment (MED): likelihood of regression, security exposure, or blast radius

Security exposure moves strictly downward and is now better than at my last review: the stderr parser tightened rather than loosened while gaining the prefixed-warning tolerance, and the Error-instance requirement on the fail-open exemption is now explicitly spoofing-resistant. No previously-denied input becomes allowed by this delta.

Residual risk remains availability, not exposure, and the blast radius is unchanged — this gates every withheld run for every consumer of a released action. Diagnosability, which drove my previous MED rating, is materially better: operators now get an accurate failure class plus a numeric exit code. I am holding at MED rather than dropping to LOW solely because of concern #1 — the change is correct, but at patch severity a consumer's first signal may be a red CI run rather than a release note. Fix the version signaling and this is comfortably LOW.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	34554612437
Cache	hit
Session	ses_f71cc8e29ffewh6mCahVk76AHf

[fro-bot]

Verdict: PASS

Third pass. The security-critical code is byte-identical to what I approved in my second review — git-credential-check.ts (+134/-33), its two test files, bootstrap.test.ts, ci.yaml, and dist/main.js all carry the same diff stats. The only new work is 6b0d75e, which resolves the sole open concern from that review: version signaling for a behavior-breaking change.

The resolution is better than what I asked for. I suggested a BREAKING CHANGE: footer to force a major. The author added the footer and made the 0.x policy explicit: breaking: true → minor in .releaserc.yaml, so 0.110.x → 0.111.0 rather than 1.0.0. That is the correct reading of semver for a pre-1.0 project — minor is the breaking-change vehicle in 0.x — and my original suggestion would have forced a premature 1.0.0. The consumer-facing signal that actually matters is the BREAKING CHANGES section in the release notes, and release-policy.test.ts:116-142 pins that it survives, by running the real release-notes-generator and asserting the section and its text are present. That directly closes the "consumers get no signal beyond a Bug Fixes line" failure mode I described.

The rule ordering is load-bearing, and the author got a subtle detail right. Placing breaking: true first is required, not cosmetic. In @semantic-release/commit-analyzer, compareReleaseTypes(current, candidate) returns !current || indexOf(candidate) < indexOf(current). Because release: false is falsy, a false already in hand is overridden by any later matching rule — so if the suppressions were listed before the breaking rule, build(dev)! and skip! would resolve to minor instead of being suppressed. I initially believed order was irrelevant here and read node_modules/@semantic-release/commit-analyzer/lib/{analyze-commit,compare-release-types}.js to check; the code confirms the author's comment, not my assumption. Better still, the ordering is protected by behavior rather than by the comment: reordering the rules would flip build(dev)! and skip! from null to minor in the matrix, failing the test.

Verification I ran (one named file): bunx vitest run scripts/release/release-policy.test.ts — 17/17 pass. This tested a specific claim, not general health: that the real analyzer, driven by the checked-in .releaserc.yaml rather than a hand-typed copy, actually produces minor for fix!/feat!/footer/build(deps)!/docs(readme)! and null for build(dev)!/skip!, and that preview.ts agrees on all 16 cases. Loading the rules via loadReleasercPolicy() from the real file is what makes this a drift detector instead of a restatement — a future .releaserc.yaml edit is exercised here.

The preview.ts changes also fix a latent bug I had not caught: previously skip!: and build(dev)!: returned major from the preview because the breaking check ran before the suppression branches, contradicting production. Hoisting the suppressions corrects that.

semantic-release-plugins.d.ts is a clean way to type two untyped transitive plugins — exported interfaces consumed via import type, all readonly, no as any / @ts-ignore / global augmentation. Compliant with the project's type-suppression ban.

Blocking issues

None.

Non-blocking concerns

1. computeNextVersion's major-clamp is the one deliberate divergence from semantic-release, and it is the one thing with no parity test. preview.ts:141-146 returns 0.31.0 for computeNextVersion('0.30.10', 'major'), while semantic-release given release type major would run semver.inc('0.30.10', 'major') and produce 1.0.0. This is unreachable today (no rule path yields major), so there is no live bug. But the guard exists specifically for "any other caller," and in exactly that scenario it makes the preview disagree with production. release-policy.test.ts establishes parity for analyzeReleaseType against the real analyzer; nothing does the same for computeNextVersion. Consider either dropping the clamp (unreachable, and its absence would make a stray major loudly visible in preview output) or noting in the comment that it is an intentional preview-only divergence. Also worth remembering that going to 1.0.0 later requires editing two places, not one.

2. The release-policy test depends on transitive dependencies. createRequire(createRequire(import.meta.url).resolve('semantic-release')) resolves @semantic-release/commit-analyzer and @semantic-release/release-notes-generator through semantic-release's own require context. The comment documents why, and the technique correctly survives non-hoisted installs. Still, the test's correctness now rests on packages that are not declared dependencies. Adding both as explicit devDependencies would make the coupling intentional rather than incidental, at the cost of a version-pinning obligation. Judgement call; I would not block on it.

3. Carry-forward known properties (unchanged, no action requested). hasEmbeddedCredential treats any : before the first @ as a credential, so a URL with an explicit port and an @ in the path is a false-positive deny. isCanonicalNonRepositoryStderr requires fatal: at column zero, so an indented fatal line denies. Both fail closed and both are recorded so they read as deliberate rather than as surprises.

Missing tests

None. The 16-case matrix cross-checks both oracles per case, the notes-preservation test covers the specific risk introduced by demoting breaking to minor, and preview.test.ts gained targeted cases for the breaking-vs-suppression precedence plus the 0.x clamp and the post-1.0 path. Combined with the 46 preflight cases and 25 workflow cases from earlier commits, the behavior surface introduced by this PR is covered.

Risk assessment (LOW): likelihood of regression, security exposure, or blast radius

Dropping from MED to LOW, as I committed to in my previous review conditional on the signaling fix.

Security exposure is unchanged from my last pass and remains strictly improved over main: every credential-check change either widens detection or converts fail-open to fail-closed, and no previously-denied input becomes allowed. Nothing in 6b0d75e touches that code.

The residual risk was never exploitability — it was consumers hitting a fail-closed denial with no warning and no workaround. That is now addressed on both halves: the failure messages name the actual fault class and carry a numeric exit code, and the release will be versioned as a minor with a BREAKING CHANGES section in the notes, verified by a test against the real notes generator. The remaining blast radius is a deliberate, documented, and now correctly-signaled behavior change, which is ordinary release risk rather than a defect in this PR.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	34557415090
Cache	hit
Session	ses_f71cc8e29ffewh6mCahVk76AHf

[fro-bot]

Verdict: PASS

Fourth pass. The security-critical surface is unchanged for the third review running — git-credential-check.ts (+134/-33), its two test files (+423/-58, +544), bootstrap.test.ts (+126), ci.yaml (+2), docs/wiki (+1/-1), and dist/main.js (+17/-18) all carry identical diff stats to what I approved in reviews 002 and 003. e29db9e touches only release tooling.

Concern #1 from review 003 is resolved, and resolved the better of the two ways I offered. The major === 0 clamp is gone from computeNextVersion; preview.ts:143 now returns ${major + 1}.0.0 unconditionally, so the function is once again a faithful model of semver.inc. The 0.x policy now lives in exactly one place — the breaking: true → minor rule in .releaserc.yaml — rather than being enforced redundantly at the arithmetic layer. That removes the "two places to edit for 1.0.0" trap I flagged, and it puts the policy at the layer that actually decides release type instead of the layer that does arithmetic.

The parity gap I identified is now closed with a real oracle. release-policy.test.ts:151-176 cross-checks computeNextVersion against semver.inc across 3 versions × 3 release types, resolving semver through the same host-require chain as the plugins so it is the actual incrementer a release would run through — not a reimplementation. The comment at :162-163 states the invariant directly: "semantic-release's own incrementer would not apply a 0.x clamp, so computeNextVersion must not invent one either." preview.test.ts:199-208 was updated in lockstep to expect 1.0.0, so there is no stale assertion contradicting the new behavior.

The end-to-end test models this PR's own merge commit. release-policy.test.ts:178-200 runs fix(setup)!: ... with a BREAKING CHANGE: footer through the real analyzer, then through both semver.inc and computeNextVersion, asserting 0.110.0 → 0.111.0 and explicitly not 1.0.0. Since the PR title now carries the ! marker and the body carries the footer, that test exercises the exact commit shape the squash merge will produce. Composing the two real oracles end-to-end is stronger than checking each in isolation, because it catches a disagreement in the handoff between them.

Verification I ran (one named file): bunx vitest run scripts/release/release-policy.test.ts — 28/28 pass, up from 17 at my last review. This tested two specific new risks rather than general health: that semver resolves through hostRequire and that the CJS-ESM interop (semverModule.default.inc) actually yields a callable, and that the 9-case arithmetic matrix agrees with the real incrementer. Both hold.

Blocking issues

None.

Non-blocking concerns

1. Retracting my concern #2 from review 003. I flagged that the test rests on undeclared transitive dependencies and suggested declaring them. I checked package.json and that framing was wrong: @semantic-release/npm and @semantic-release/github are already listed as plugins in .releaserc.yaml while being undeclared, so relying on semantic-release's bundled default plugins is this repo's established, load-bearing pattern — the release itself already depends on it. semver, commit-analyzer, and release-notes-generator are the same class of dependency. Declaring them explicitly would add a version-pinning obligation without removing any real fragility, and would risk the test resolving a different copy than the one the release actually uses, which would defeat the point of the oracle. No action; recorded so the decision is deliberate.

2. Carry-forward known properties (unchanged, no action requested). hasEmbeddedCredential treats any : before the first @ as a credential, so a remote URL with an explicit port and an @ in the path is a false-positive deny. isCanonicalNonRepositoryStderr requires fatal: at column zero, so an indented fatal line denies. Both fail closed, both are pre-existing or deliberate, and both are recorded across reviews so they read as known properties rather than latent surprises.

Missing tests

None. Every gap I have raised across four reviews is closed. The release-tooling surface now has three independent real oracles — the analyzer for release type, semver.inc for arithmetic, and the notes generator for the BREAKING CHANGES section — each driven by the checked-in .releaserc.yaml rather than a hand-typed copy, so config edits are exercised rather than silently diverging. The credential-check surface retains its 46 preflight cases including 19 real-Git ones.

Risk assessment (LOW): likelihood of regression, security exposure, or blast radius

Holding at LOW.

Security exposure is unchanged across the last three commits and remains strictly improved over main: every credential-check change either widens detection or converts fail-open to fail-closed, and no previously-denied input becomes allowed. Nothing in 6b0d75e or e29db9e touches that code.

The delta in this pass is release tooling only, and it reduces risk rather than adding it — one fewer place where the preview can disagree with production, and a real-oracle test guarding the remaining arithmetic. The residual blast radius is the intended behavior change itself: fail-closed denials for host-level headers and unreadable Git state, now accurately diagnosed, correctly versioned as a 0.x minor, and carrying a BREAKING CHANGES section verified against the real notes generator. That is ordinary, well-signaled release risk, not a defect.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	34558451912
Cache	hit
Session	ses_f71cc8e29ffewh6mCahVk76AHf