fix(ci): gate issue runs and avoid persisted credentials - #3878
Conversation
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Two changes, both load-bearing. Worth stating why, because the diff reads smaller than it is.
persist-credentials: false on the content job isn't defense-in-depth garnish — the pinned agent requires it. fro-bot/agent@a31e0ad classifies pull_request, issues, and issue_comment as withhold triggers (packages/runtime/src/agent/response-delivery.ts), and src/harness/phases/bootstrap.ts:81 runs a fail-closed preflight that greps http.*.extraheader and calls setFailed if it finds one. Without this line, every PR-review and issue run in this repo dies before the model starts. I confirmed the fix works from inside it: this run has no extraheader in local git config and reached you anyway.
The author_association gate closes the real hole. github.event.issue.body flows into WIKI_QUERY_BODY and into the agent prompt; an outside contributor could previously seed that channel by opening an issue. The || '' guard fails closed on a missing association, and the idiom matches the comment branch exactly. edited doesn't widen it — the payload carries the author's association, and editing someone else's issue already requires triage rights.
Blocking issues
None.
Non-blocking concerns
1. .github/workflows/fro-bot.yaml:494-498 — the wiki sync now rides on anonymous fetch, and fails silently. Dropping the credential means git ls-remote origin data and git fetch origin data resolve unauthenticated. That works today because the repo is public (verified in this run — the sync landed, knowledge/ is dirty). But the if ... fi guard swallows a failure with no output, so on a visibility flip the content job would run on stale knowledge/ and say nothing about it. The observe job already solved this at lines 887-897: ::warning:: on fetch failure, an explicit message when the branch is absent. Mirror that shape in the content job's sync step so a silent context degradation becomes a visible one.
2. Credential scope is job-wide; the agent's classification is per-event. This job serves five events. Three are withhold. The other two — pull_request_review_comment and discussion_comment — fall through to deferred-or-unknown → provision/model-gh, meaning the agent expects to drive gh directly. gh auth comes from the github-token input, not git config, so API work survives. A git push from the checkout does not. Nothing in the current prompts asks for one, so this is latent rather than broken, but it's an undocumented asymmetry sitting in a security-relevant expression. Pick one and write it down: either uniform-false is the intended posture (my preference — content mode delivers through the response file, not through git), or scope per-trigger the way fro-bot/dashboard does with a contains(fromJSON([...]), github.event_name) expression. Right now the file asserts neither.
3. README.md:211 still advertises the old trigger surface. The table lists Fro Bot's triggers as "Issues, PR events, schedule, dispatch, workflow_call" and line 21 says the bot "triages issues." That's now trusted-author issues only. An outside contributor opening an issue gets a silent skip with no explanation anywhere in the docs. Per this repo's own completion criteria, a behavior change wants the doc line to follow.
4. scripts/fro-bot-workflow.test.ts:322-350 — the full-predicate snapshot earns nothing. It whitespace-normalizes the entire if expression and compares it to a literal. The three tests immediately after it already assert every clause that matters. The snapshot adds no signal those don't, and it will break on any future predicate edit — a new event branch, a reordered clause — forcing a mechanical copy-paste update that no reviewer will actually read. That's a test that trains people to ignore it. copilot-instructions.md asks for behavior-level assertions over implementation-coupled ones; this is the coupled kind.
5. scripts/fro-bot-workflow.test.ts:377-378 pins the absence of hardening. Asserting persist-credentials is toBeUndefined() on remediate and observe encodes "these must stay credentialed" as a contract. For fro-bot-remediate that's genuinely true — output-mode: branch-pr means the agent pushes, so the credential is required. Assert that positively with the reason in a comment; a future reader will otherwise read toBeUndefined() as an oversight and delete it. For fro-bot-observe there's no such constraint: it runs working-dir, and its only writes go through the Octokit path in commit-metadata.ts. The assertion there locks in a posture nobody chose, and will fight whoever tries to harden it next.
Missing tests
None required. The five new tests cover the trust-gate clause, the preserved bot exclusions, the untouched comment branch, and the checkout inputs — that's the reachable surface for a static contract test. The PR description is honest that these assert workflow structure, not GHA evaluation, which is the right disclosure: contains(fromJSON('[...]'), x) array membership and expression case-insensitivity go untested here and can't be tested here. That's acceptable — the identical idiom already ships on the comment branch and has runtime evidence behind it.
Risk assessment: LOW
The trust gate strictly narrows an existing trigger; failure mode is a skipped run, not an unauthorized one. The credential change is required by the pinned agent's own preflight, so the alternative isn't "safer," it's "the job doesn't run." The one behavior that genuinely changed hands — the data-branch fetch losing its credential — was verified working in this run, and its only failure path is a public-to-private transition that would degrade context quietly rather than leak anything. Both privileged jobs are untouched. Nothing here widens the attacker-reachable surface; it shrinks it in two directions at once.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/.github |
| Run ID | 34399362166 |
| Cache | hit |
| Session | ses_f7834ef30ffeW5FFMWvdmY1PvY |
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Re-review of d5679d9. Four of five prior findings resolved, one rebutted correctly. I verified the new shell against the real remote rather than trusting the fixtures:
git ls-remote --exit-code <public repo> data → 0
git ls-remote --exit-code <public repo> no-such-branch → 2
git ls-remote --exit-code <unreachable repo> data → 128
That's the load-bearing claim of the fix, and it holds. The elif -eq 2 branch really does isolate "branch not established" from "probe failed," so a visibility flip lands in the else with a warning instead of masquerading as an empty data branch. This is a better shape than the observe-job pattern I pointed you at — structured exit-code discrimination beats scraping stderr, and it keeps remote URLs out of the log entirely. set -e placement is correct throughout: cmd || var=$? doesn't trip errexit, if git fetch is exempt as a condition, and git restore still hard-fails. tsc --noEmit clean, 40/40 tests pass.
On finding 4, you're right and I was wrong. A full-predicate pin does catch a class the per-clause toContain assertions cannot: the correct gate text attached to the wrong branch, or an || true appended to one clause. Those read as passing substring checks. The comment you added makes the test's job legible to the next reader, which was the actual defect. Keeping it is the correct call.
Finding 5 is fixed the right way — not.toBe(false) with the reason inline admits both the actions/checkout default and an explicit true, and the toBeDefined() guard closes the vacuous-pass hole on a step rename. Dropping the observe assertion was right.
The shell-flow fixtures are the strongest addition here. Executing the workflow's actual run: block against a bounded fake git means the test tracks the workflow instead of a copy of it — that closes the gap I'd written off as untestable.
Blocking issues
None.
Non-blocking concerns
1. .github/workflows/fro-bot.yaml:494-510 — making the fetch failure visible also made it fail-open, and the wiki write downstream doesn't know. Before this commit, a failed git fetch origin data left git restore --source FETCH_HEAD to fail, which failed the step and killed the job — nothing got written. Now the failure warns and the job continues with knowledge/ at main's state.
That matters because of what runs later. Ingest wiki insight changes calls wiki-ingest.ts → commitWikiChanges (packages/wiki-write-core/src/wiki-ingest.ts:303), which builds a tree on data's current HEAD and writes working-tree content for the changed paths (createTree / createCommit / updateRef at 358-373). getChangedWikiPaths scopes that to files the agent actually edited, so the blast radius is bounded — but for those pages, a main-derived body would overwrite whatever data holds. data → main promotes weekly, so "stale" can mean several days of wiki updates silently reverted on the touched pages.
Narrow trigger (transport failure after a successful probe), recoverable via git history, and the Check Wiki Authority gate sits downstream. Not worth blocking. But it's the one thing I'd want fixed before this shell pattern gets copied into the other fleet repos, because there it'll land next to the same ingest step.
Concrete fix: give the sync step an id: wiki-sync, write synced=true|false to $GITHUB_OUTPUT on each branch, and add steps.wiki-sync.outputs.synced == 'true' to the ingest step's existing if:. The exit-2 case should count as synced — there's nothing on data to lose, and commitWikiChanges bootstraps the branch itself. Only the two warning paths should suppress the write. The warning stays; the lost update doesn't happen.
2. scripts/fro-bot-workflow.test.ts:429 — first bash-dependent test in the suite. Existing shell-outs in scripts/*.test.ts all target node (rollout-tracker-snapshot.test.ts:51, wiki-query.test.ts:337), which is portable by construction. This one needs bash plus a working chmod +x. CI is ubuntu-latest across every job in main.yaml, and your verification block shows macOS, so both platforms in actual use are fine. Flagging only so the constraint is a known one rather than a discovered one — if a Windows contributor ever runs pnpm test, this is where it breaks.
3. Optional: the probe warning reports an exit code, not a reason. ::warning::data branch probe failed (exit 128) tells an operator the class of failure but not which one. Capturing stderr and appending a truncated slice would name it outright. Genuinely optional — with credentials withheld there's no token in the remote URL to leak, but 128 is already the actionable signal, and the current form has zero leak surface. Take it or leave it.
Missing tests
None. The five shell-flow cases cover the branch matrix exactly: success, absence, probe error, fetch error, and the preserved hard failure on restore. The last one is the important one — it pins that set -e didn't accidentally convert a real failure into a warning, which is the failure mode this kind of hardening usually introduces. If you take finding 1, the synced output wants a sixth case asserting false on the two warning paths.
Risk assessment: LOW
Unchanged from the first pass, and the delta reduces risk rather than adding to it. The trust gate still narrows a trigger, the credential change is still required by the pinned agent's fail-closed preflight, and both privileged jobs are still untouched. The one new behavior — proceeding past a failed data-branch fetch — trades a hard failure for a visible warning plus a bounded, recoverable stale-write window on an error path that requires a transport fault to reach. README now matches what the workflow actually does, which closes the honesty gap. The daemon reports its own degradation now instead of drifting quietly.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/.github |
| Run ID | 34403381757 |
| Cache | hit |
| Session | ses_f7834ef30ffeW5FFMWvdmY1PvY |
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Third pass, on e7254e1. You solved my lost-update finding by a different route than I proposed — instead of gating the write, you made the write unreachable. That's the stronger move, and I traced the whole chain to confirm it holds:
Sync wiki from data branch exits 1 → job fails at that step → Ingest wiki insight changes (line 659) carries success() && in its if:, which is now false → no commitWikiChanges. Detect wiki insight changes still runs on always() && !cancelled(), but it only shells a git diff into a hash output. Nothing reaches createTree/updateRef. Fail-closed confirmed structurally, not assumed.
The trace addition to the fake git is the right way to prove it. expect(result.trace).toEqual(['ls-remote']) pins that a probe failure never attempts the fetch, and ['ls-remote', 'fetch'] pins that a fetch failure never attempts the restore. Ordering assertions on a shell control-flow fixture — that's the part most people skip, and it's exactly what makes the exit-code discrimination trustworthy rather than incidental. 40/40 pass, tsc --noEmit clean.
Worth stating what the net position is now, because three commits of churn obscures it: the pre-PR baseline treated a transport failure as "no data branch" and continued silently. A 128 from ls-remote was indistinguishable from a 2. That was the actual latent bug in this workflow, and it predates every line of this PR. It's now named and fatal.
Blocking issues
None.
Non-blocking concerns
1. fro-bot-observe still runs the old pattern, and it's the job where this actually bites. You hardened the content job. Lines 709-714 (fro-bot-remediate) and 859-864 (fro-bot-observe) still carry the original:
if git ls-remote --exit-code origin data >/dev/null 2>&1; then
git fetch origin data
git restore --source FETCH_HEAD --worktree -- knowledge
fiFor fro-bot-remediate this is close to harmless — it restores knowledge/ to HEAD before the agent runs (line 757) and has no ingest step, so a stale sync costs context quality and nothing else.
fro-bot-observe is the problem. It has Ingest wiki insight changes at line 1104, gated on schedule/workflow_dispatch, and it runs daily. Trace the 128 path: probe fails → the if is false → sync silently skipped → agent edits knowledge/ against a main-derived tree → detect fires → ingest writes working-tree content onto data HEAD. That is precisely the lost-update I described last round, still live, on the job that executes it every day, against a main that can lag data by a full weekly promotion cycle.
Its fetch-failure path is already fail-closed by accident — git restore --source FETCH_HEAD fails when the fetch didn't land, and it's the last command in the block. Only the probe path is silent. So the gap is narrow and the fix is the block you already wrote.
Pre-existing, and propagating it is scope creep on a PR that's about issue triggers and credentials. I'd open a follow-up rather than grow this one. But it belongs on the record now, because this PR establishes the known-good shape and leaves the highest-value callsite on the old one — and the next reader will reasonably assume all three sync steps behave alike.
2. Fail-closed buys correctness with availability, and there's an in-repo precedent for the surgical version. A transport blip now kills the entire content run — including a PR review, which needs the wiki only as context enrichment, never as authority. There's no retry on an event-driven trigger, so the PR author gets a red check and no review.
docs/solutions/integration-issues/wiki-lint-authoritative-data-snapshot-reporting-2026-05-02.md records this exact tradeoff being resolved the other way: "Fail-fast restore handling without a report" is listed under What Didn't Work, and wiki-lint.yaml uses continue-on-error: true on the restore plus if: steps.restore-wiki.outcome == 'success' on the consumer, so the run still produces its product while the unsafe step is skipped.
I'm not asking you to flip it. For a control plane, "must not corrupt authority" outranking "must always answer" is a defensible ordering, and your version is meaningfully simpler than plumbing a synced output. But the two resolutions sit in the same repo now pointing opposite directions, and that learning doc is the first thing someone will find when they hit this next. One sentence in the step comment naming the choice — authority integrity over review availability — would keep the next reader from reading it as an oversight.
3. Two vacuous assertions. expect(result.stdout).not.toContain('::warning::') appears in four tests, and the step no longer emits ::warning:: on any path. In the two failure tests it's meaningful — it pins that the error path uses ::error:: + exit 1 rather than reverting to warn-and-continue, which is a real anti-regression guard. In the two success tests (status 0) it can never fail. Harmless, but they read as coverage they aren't providing.
Missing tests
None. The five-case matrix plus the trace ordering assertions covers the branch structure completely. If you take finding 1, the observe-job sync step wants the same fixture pointed at it — the harness is already generic over runScript, so it's a steps.find() change and nothing more.
Risk assessment: LOW
Lower than the previous pass. The stale-write window I flagged in the content job is closed outright rather than narrowed, and the closure is structural — verified through the success() gate, not inferred. The trust gate and credential withholding are unchanged and still correct. What remains is a pre-existing silent-skip on the observe job's probe path, which this PR neither introduced nor widened, and an availability posture that trades a rare failed run for a class of quiet corruption. Both privileged jobs keep their credentials for the pushes that need them. The daemon now refuses to speak from a memory it can't verify — which is the right instinct for something that writes to its own wiki.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/.github |
| Run ID | 34407652929 |
| Cache | hit |
| Session | ses_f7834ef30ffeW5FFMWvdmY1PvY |
2 participants
Summary
Restrict issue-triggered content runs to trusted authors and keep checkout credentials ephemeral.
Changes
OWNER,MEMBER, orCOLLABORATORissue-author association before checkout.Verification
pnpm bootstrappnpm check-typespnpm lintpnpm testactionlint .github/workflows/fro-bot.yamlgit diff --check|| truebypass.These checks cover workflow configuration and shell control flow, not a GitHub-hosted event matrix.
Post-Deploy Monitoring & Validation